fix: HITL verifier no longer rejects VALID approvals just because an earlier approval in the array failed

CHANGELOG.md

@@ -17,6 +17,38 @@
 
 ### Fixed
 
+- **[SDK]** `verify_manifest()` returned `MISMATCH`/`EXPIRED`/`INVALID`/
+  `UNVERIFIABLE` for `hitl_record` when *any* approval in `hitl_record.approvals`
+  failed, even if a later approval in the same array was present, valid,
+  unexpired, and sufficient for the declared risk tier. Spec 5.3 only
+  requires *at least one* approval to satisfy every condition; it does not
+  require every approval in the array to. The verifier's loop broke out on
+  the first expired, malformed, unverifiable, or insufficient approval it
+  encountered and never evaluated the approvals after it, so an operator
+  who appended a fresh re-approval ahead of (or alongside) an old, expired,
+  or otherwise stale one had the whole record incorrectly rejected. The
+  loop now evaluates every approval and only short-circuits once one is
+  found that clears every check. When none do, the prior code never had a
+  real precedence to preserve: it broke the whole loop on the first
+  approval that failed, so the reported reason was whichever check that
+  one approval happened to fail first, not a ranking of failure types (an
+  `[EXPIRED, INVALID]` array reported `EXPIRED`; `[INVALID, EXPIRED]`
+  reported `INVALID` - purely positional, order-dependent, and never a
+  documented contract). Reviewing this against multiple approvals for the
+  first time requires an actual decision here, since more than one failure
+  reason can now be true at once. This fix establishes and tests an
+  explicit, order-independent precedence: `INVALID` > `APPROVAL_INSUFFICIENT`
+  > `EXPIRED` > `UNVERIFIABLE`. `INVALID`, `APPROVAL_INSUFFICIENT`, and
+  `EXPIRED` are each positive, concrete evidence of a problem and always add
+  a mismatch, so their relative order never changes the overall result;
+  `INVALID` is ranked highest among them because a broken or tampered
+  signature is the strongest evidence of active tampering. `UNVERIFIABLE`
+  (this verifier has no trusted key to check that approval against) adds no
+  mismatch, so it is ranked last and only reported when it is the *only*
+  problem in the array - otherwise it would silently drop a concrete
+  finding from `mismatch_details` and downgrade the overall result from
+  `MISMATCH` to `UNVERIFIABLE` (HITL-004).
+
 - **[SECURITY][SDK]** `attach_receipt()`, `attach_attestation()`, and
   `attach_approvals()` could crash with `cbor2.CBOREncodeError` instead of
   the documented `CoseError` on a malformed COSE envelope. `_decode_tagged()`

python/src/agent_manifest/_verify.py

@@ -1216,27 +1216,36 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s
             # Presence, lifetime, and method checks alone do not prove that a
             # human approved this manifest: the signature pre-image binds the
             # manifest ID, approver, timestamp, and exact scope.
+            from datetime import timedelta
+
             from ._delegation import verify_hitl_approval
             from ._signing import _b64url_decode
 
             now = datetime.now(timezone.utc)
-            all_ok = True
+            # Spec 5.3: a VALID result requires *at least one* approval that is
+            # present, valid, unexpired, and sufficient for the declared risk
+            # tier - not that *every* approval in the array meets that bar.
+            # Each approval is therefore evaluated independently and the loop
+            # only stops early once an approval that clears every check is
+            # found. Approvals that fail must not short-circuit the
+            # evaluation of the approvals that follow them (HITL-004).
+            any_approved = False
             approval_insufficient = False
             approval_invalid = False
             approval_unverifiable = False
+            any_expired = False
             for approval in approvals:
                 approved_at = approval.get("approved_at", "")
                 duration = approval.get("approved_scope", {}).get("approval_duration_seconds", 0)
                 try:
                     ap_time = datetime.fromisoformat(approved_at.replace("Z", "+00:00"))
-                    from datetime import timedelta
                     if now > ap_time + timedelta(seconds=duration):
-                        all_ok = False
-                        break
+                        any_expired = True
+                        continue
                 except (ValueError, AttributeError):
                     # Unparseable timestamp - treat as expired to fail safe (HITL-001)
-                    all_ok = False
-                    break
+                    any_expired = True
+                    continue
                 if context.conformance_level >= 2:
                     scope = approval.get("approved_scope") or {}
                     risk_tier = scope.get("risk_tier")
@@ -1245,13 +1254,13 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s
                         risk_tier in {"high", "critical"} and method != "hardware-key"
                     ):
                         approval_insufficient = True
-                        break
+                        continue
 
                 approver_id = approval.get("approver_id")
                 public_key_b64 = context.approver_public_keys.get(approver_id)
                 if public_key_b64 is None:
                     approval_unverifiable = True
-                    break
+                    continue
                 try:
                     verify_hitl_approval(
                         approval,
@@ -1260,15 +1269,28 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s
                     )
                 except (InvalidSignature, KeyError, TypeError, ValueError):
                     approval_invalid = True
-                    break
+                    continue
+
+                # This approval independently satisfies every requirement
+                # (present, unexpired, sufficient for the risk tier, and
+                # authenticated). Spec 5.3 only requires one such approval.
+                any_approved = True
+                break
+
+            if any_approved:
+                fields.hitl_record = HitlResult.APPROVED
 
-            if approval_unverifiable:
-                fields.hitl_record = HitlResult.UNVERIFIABLE
-                result.warnings.append(
-                    "HITL approval could not be authenticated: no trusted "
-                    "approver key is available for its approver_id"
-                )
             elif approval_invalid:
+                # A cryptographically broken/tampered approval is positive,
+                # concrete evidence of a problem. It always outranks
+                # UNVERIFIABLE (a mere lack of key configuration on this
+                # verifier's part, which produces no mismatch entry below)
+                # so that proof of tampering is never masked by an unrelated
+                # approval this verifier simply lacks the key to check
+                # (HITL-004). This mirrors the final overall-result
+                # computation elsewhere in this function, where any
+                # concrete `mismatches` entry already takes priority over
+                # an UNVERIFIABLE state.
                 mismatches.append(MismatchDetail(
                     field="hitl_record.approval_signature",
                     expected_hash="<valid approval signature bound to this manifest>",
@@ -1282,16 +1304,36 @@ def _check(field_name: str, manifest_val: Optional[str], runtime_val: Optional[s
                     actual_hash="<approval method insufficient>",
                 ))
                 fields.hitl_record = HitlResult.APPROVAL_INSUFFICIENT
-            elif not all_ok:
+            elif any_expired:
                 # Expired approvals always add to mismatches regardless of enforce_hitl (HITL-002)
                 mismatches.append(MismatchDetail(
                     field="hitl_record",
                     expected_hash="<valid unexpired approval>",
                     actual_hash="<approval expired or unparseable>",
                 ))
                 fields.hitl_record = HitlResult.EXPIRED
+            elif approval_unverifiable:
+                # Reached only when no approval produced concrete evidence
+                # of a problem (no invalid signature, no insufficient
+                # method, no expiry) - i.e. every failing approval failed
+                # solely because this verifier has no trusted key for its
+                # approver_id. There is nothing to add to `mismatches` here;
+                # this is an indeterminate state, not a proven negative.
+                fields.hitl_record = HitlResult.UNVERIFIABLE
+                result.warnings.append(
+                    "HITL approval could not be authenticated: no trusted "
+                    "approver key is available for its approver_id"
+                )
             else:
-                fields.hitl_record = HitlResult.APPROVED
+                # Defensive fallback: approvals was non-empty, so every entry
+                # must have set one of the flags above unless it was
+                # approved. Fail closed rather than silently pass.
+                mismatches.append(MismatchDetail(
+                    field="hitl_record",
+                    expected_hash="<valid unexpired approval>",
+                    actual_hash="<no approval satisfied requirements>",
+                ))
+                fields.hitl_record = HitlResult.EXPIRED
     elif context.enforce_hitl:
         # enforce_hitl with no hitl_record at all - fail closed. Omitting the
         # record entirely MUST NOT be weaker than declaring it with no approvals.

python/tests/test_cose.py

@@ -1024,6 +1024,22 @@ def test_signed_hitl_requirement_cannot_be_satisfied_by_editing_the_header():
     assert result.result == OverallResult.MISMATCH
 
 
+def test_engine_approves_when_a_later_approval_in_the_unprotected_header_is_valid():
+    """A bad approval earlier in the array must not block a good one later
+    in it - same requirement as the v0.1 path (spec 5.3, HITL-004), now
+    exercised through the COSE unprotected-header attachment path."""
+    manifest = base_manifest(hitl_record={"required": True})
+    dummy_signature_approval = approval(approval_signature="c2ln")
+    good_approval = approval()
+    signed = attach_approvals(
+        sign_cose_sign1(manifest, KP),
+        [dummy_signature_approval, good_approval],
+    )
+    result = verify_manifest(signed, base_context(enforce_hitl=True), store())
+    assert result.fields_verified.hitl_record == HitlResult.APPROVED
+    assert result.result == OverallResult.VALID
+
+
 # ---------------------------------------------------------------------------
 # Malformed input
 #