Fail closed when verify cannot resolve a comparison base - #305
Fail closed when verify cannot resolve a comparison base#305pengfei-threemoonslab wants to merge 1 commit into
Conversation
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
I recommend changes before merge. The normal path and existing tests are green, but adversarial review reproduced fail-open ref-resolution cases, request-artifact contamination, and control/contract inconsistencies that are not covered by the current suite. Details and concrete remediation are inline.
| state="selection_required", | ||
| candidates=("origin/HEAD",), | ||
| ) | ||
| target_sha = commit_sha(workspace, target) |
There was a problem hiding this comment.
[P1] Resolve fully qualified remote refs. The resolver shortens refs/remotes/origin/main to origin/main before commit resolution. Git DWIM then allows a tag or local branch named origin/main to shadow the remote-tracking ref. I reproduced this with a tag at HEAD: zero-base verification classified head_at_default, emitted no base, and ignored the committed PR diff. Preserve fully qualified refs or immutable resolved SHAs throughout internal operations; shorten only for display.
| if (sha := commit_sha(workspace, candidate)) is not None | ||
| ] | ||
| local_candidates = tuple(candidate for candidate, _ in local_refs) | ||
| if local_refs and all(sha == head_sha for _, sha in local_refs): |
There was a problem hiding this comment.
[P1] A local main cannot prove comparison scope. With no origin, a local main equal to HEAD is treated as proof that HEAD is already at the default. Committing a manifest or policy change directly on main therefore produces base_ref=null and changed_files=[], bypassing trust-root and policy-weakening comparison. This is implicit head-only verification without --no-base; require explicit --base or --no-base when only mutable local refs are available.
| rerun_options=rerun_options, | ||
| ) | ||
| _assert_base_input_failure(verifier) | ||
| _remove_scan_artifacts(out_dir) |
There was a problem hiding this comment.
[P1] Clear every request-scoped artifact. This cleanup is partial and is not applied to every early return. A failed explicit-base run can leave a prior passed report and capability artifacts; verification-base-report.json survives this helper and is advertised by later runs. More seriously, a successful --no-base run can hash that stale base report into a new terminal receipt. Clear all request-scoped outputs before artifact discovery, including the portable base report.
| ) | ||
| _assert_base_input_failure(verifier) | ||
| _remove_scan_artifacts(out_dir) | ||
| _write_artifacts( |
There was a problem hiding this comment.
[P1] Bind failure plans to the requested subject. This artifact write occurs before worktree context is captured or an explicit --head is materialized. I reproduced plans combining a feature head SHA with config/tool hashes from the checked-out main; dirty worktree changes also left request_id unchanged. A containment-invalid tool source additionally escapes as exit 4 with no plan or verify-run. Capture the intended worktree/commit first, or omit content-addressed identity artifacts when that scope cannot be bound.
| ), | ||
| ) | ||
| else: | ||
| action = HumanControlAction( |
There was a problem hiding this comment.
[P1] Preserve the rerun obligation after human routing. This new human action reaches the human-override projection, which leaves control.verify_required=false even though execution failed. Human acknowledgement cannot establish a comparison base; only a new verifier artifact can clear the state. Set verify_required=true for failed human routes and include it in _assert_base_input_failure.
| | `agents-shipgate self-check` | `--json` | | ||
| | `agents-shipgate agent handoff` | `--from`, `--report`, `--verify-run`, `--out`, `--json` | | ||
|
|
||
| For `agents-shipgate verify`, omitting `--base` means safe auto-resolution, |
There was a problem hiding this comment.
[P2] Version and synchronize the breaking CLI behavior. Omitting --base can now change from a scan with exit 0 to a pre-scan failure with exit 2, while contract_version remains 19 despite this document promising breaking changes require a bump. Canonical instructions and exit-code tables also remain inconsistent in AGENTS.md, bundled skills/recipes, llms-full.txt, and .well-known. Bump the runtime contract and synchronize the generated and machine-readable surfaces.
| changed_files=[], | ||
| diff_text="", | ||
| trigger=trigger, | ||
| base_status="ref_missing", |
There was a problem hiding this comment.
[P2] Do not label ambiguity as a missing ref. Every selection_required outcome is serialized as ref_missing, including when origin/main and origin/master both exist but diverge. PR comments and compact diagnostics consequently tell consumers the base is missing rather than requiring human selection. Project the new resolution state accurately, adding or versioning a field if the frozen base_status enum cannot express it.
| candidates=candidates, | ||
| ) | ||
|
|
||
| if _origin_is_configured(workspace): |
There was a problem hiding this comment.
[P2] Make fetch recovery exact and convergent. With origin configured but only origin/trunk fetched, this repeatedly returns fetch_required with a generic expected artifact. Another fetch does not normally create origin/HEAD, and the resolver never considers trunk, so recovery cannot converge. Inventory existing origin/* refs and route ambiguity to human selection, or require an exact origin/HEAD target before exposing agent-owned fetch recovery.
Summary
run_scanwhen committed comparison scope cannot be proved; preserve explicit--no-baseas the only intentional head/worktree-only opt-outProtected
AGENTS.mdandSKILL.mdtrust-root surfaces are intentionally excluded from this PR after preflight required separate human review.Validation
pytest -qsuite passed, including isolated wheel packaginggit diff --checkpassedcontrol.state=complete,release_decision.decision=passed,merge_verdict=mergeablesha256:5526e4e4447036aa86c3c9572209e784c5e871b4001460715d88a67445cb3532