Skip to content

Fail closed when verify cannot resolve a comparison base - #305

Draft
pengfei-threemoonslab wants to merge 1 commit into
mainfrom
codex/fix-zero-base-fail-open
Draft

Fail closed when verify cannot resolve a comparison base#305
pengfei-threemoonslab wants to merge 1 commit into
mainfrom
codex/fix-zero-base-fail-open

Conversation

@pengfei-threemoonslab

Copy link
Copy Markdown
Contributor

Summary

  • replace nullable zero-base auto-detection with explicit selected / head-at-default / fetch-required / selection-required states
  • stop before run_scan when committed comparison scope cannot be proved; preserve explicit --no-base as the only intentional head/worktree-only opt-out
  • distinguish shallow missing history from unrelated histories and emit consistent failed verifier, plan, verify-run, handoff, and PR-comment artifacts without a release decision
  • record additive base-resolution audit options in the verification plan
  • update public CLI/contract documentation without changing top-level schema versions

Protected AGENTS.md and SKILL.md trust-root surfaces are intentionally excluded from this PR after preflight required separate human review.

Validation

  • full pytest -q suite passed, including isolated wheel packaging
  • Ruff lint and format checks passed
  • git diff --check passed
  • committed-tree Shipgate verification: control.state=complete, release_decision.decision=passed, merge_verdict=mergeable
  • verification receipt reproduced successfully: sha256:5526e4e4447036aa86c3c9572209e784c5e871b4001460715d88a67445cb3532

@pengfei-threemoonslab pengfei-threemoonslab left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread STABILITY.md
| `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,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant