fix: HITL verifier no longer rejects VALID approvals just because an earlier approval in the array failed - #415
Conversation
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
imran-siddique
left a comment
There was a problem hiding this comment.
Approving. Measured against the spec text rather than the PR body, because this widens what the verifier accepts and that direction earns the check.
The rule this is held to, spec/agent-manifest-spec-v0.2.md section 5.3, in the list of conditions a VALID result requires:
If
enforce_hitlistrue, at least one HITL approval is present, valid, not expired, and meets theapproval_methodrequirement for the declaredrisk_tier
"At least one" is unambiguous, so breaking out of the loop on the first bad entry was a defect, and [expired_approval, valid_approval] returning EXPIRED was wrong under the spec we publish.
What I checked beyond the headline, since a loosened check is only safe if the failure path survives it:
- Each mode is still recorded separately,
any_expired,approval_insufficient,approval_unverifiable,approval_invalid, so a record where every approval is bad still reports which way it failed rather than collapsing to one reason. That distinction is the reason this is a fix and not a weakening. - The defensive fallback for a non-empty
approvalswhere every entry fell through is still there. - Unparseable
approved_atstill fails safe as expired (HITL-001). That is the line a later tidy-up would most easily drop, and it is doing real work. - Level 2 still refuses
software-key, and still refuses anything other thanhardware-keyforhighandcriticalrisk tiers, per entry rather than per record.
Merge state. #407 landed in _verify.py a few minutes before this, and the trial merge onto current main is clean with no conflicts in either that file or CHANGELOG.md. The merged tree runs 1572 passed, 10 skipped.
What
verify_manifest()now markshitl_recordasAPPROVEDif any approval inhitl_record.approvalsis valid, unexpired, and sufficient instead of rejecting the whole record the moment it hits the first bad approval.Why
Spec 5.3 says a
VALIDresult only needs at least one good approval. The verifier's loopbreaked on the first expired/invalid/unverifiable/insufficient approval and never looked at the rest of the array. So[expired_approval, valid_approval]was wrongly rejected asEXPIRED, even though the second approval was perfectly valid. Same for an invalid or unverifiable approval placed before a good one. (HITL-004)Spec impact
None
Test plan
pytest -vpassesmypy src/agent_manifestpassesruff check src/ tests/passesCHANGELOG.mdupdatedDCO
All commits in this PR are signed off (
git commit -s). By submitting this PR I certify the Developer Certificate of Origin.