fix(identity): bind every declared adapter input to input_set_id (#299) - #332
Conversation
64751bb to
545e291
Compare
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Two actionable findings from the review; details are inline.
| # branches must reach every adapter input: a path that lands in neither can | ||
| # change bytes while ``input_set_id`` stays byte-identical. | ||
| candidate_input_paths = ( | ||
| _manifest_declared_input_paths(config_path=config_path, input_root=input_root) |
There was a problem hiding this comment.
[P1] Capture transitive adapter reads. This fallback enumerates only paths declared directly in shipgate.yaml. Inputs discovered while parsing an entrypoint—such as a Google ADK McpToolset inventory or OpenAPI spec—remain absent from plan.inputs.tool_sources. I reproduced two clean heads whose inventories/mcp-tools.json bytes differed (only by a trailing newline) but whose prepared input_set_id was identical; the plan bound agent.py, the eval set, and the function inventory, but not the MCP inventory. Please capture reads against the evaluated input root or statically expand transitive dependencies before building the plan, with a regression test for this case.
| no_heuristics=no_heuristics, | ||
| ) | ||
| except InputParseError as exc: | ||
| typer.echo(f"Input parsing error: {exc}", err=True) |
There was a problem hiding this comment.
[P2] Preserve the agent-mode error envelope. This new catch emits only prose and exits. With AGENTS_SHIPGATE_AGENT_MODE=1, an out-of-root declared input produces no structured input_parse_error, next_action, or next_actions, contrary to the CLI agent contract. Please emit the same agent-mode error payload used by verify before raising typer.Exit(3).
…rations Review follow-up on #332. Enumerating the manifest reaches only what the manifest names. An input discovered while parsing something it names — a Google ADK `McpToolset` inventory, an OpenAPI spec constructed inside `agent.py`, a sub-agent config — is invisible to that walk, so two trees whose adapters read different bytes still shared an `input_set_id`. Reproduced on the ADK sample: the plan bound `agent.py`, the eval set, and the function inventory, but not `inventories/mcp-tools.json`, and a trailing newline on it left identity byte-identical. Both remaining producers now observe the read boundary instead. A committed-tree `verify` is snapshotted against the archived tree it scans, which the worktree-bound snapshot could never see, and `verification prepare` loads sources — statically, deciding nothing — to record what they open. Committed-tree and worktree runs of the same tree now bind the same set, asserted as an invariant rather than per-file. The declared-path enumeration survives only as the fallback for a plan built with no snapshot at all. A committed-tree run therefore has two snapshots alive, and each external input must belong to exactly one of them. The first attempt let the archived scan read the baseline and policy packs through no snapshot at all, silently defeating the tamper check that `test_archived_verify_rejects_external_baseline_change_after_scan` exists to enforce; the second let both watch the same directory, and the second re-validation then failed on a change the first legitimately allowed. The worktree snapshot now binds them before the archived scan starts, which also widens the guarded window: it begins before the scan rather than at the scan's first read. `verification prepare` reads inputs now, so it fails on a manifest whose inputs cannot be loaded — the same condition under which `verify` fails, and exactly when a prepared plan could not honestly claim an input set. Its errors carry the agent-mode envelope (`input_parse_error` / `config_error` with `next_action` and `next_actions`, per docs/errors.json) instead of prose alone. With capture in place, a declared path that no adapter opens is correctly not an input, so the out-of-root rejection now applies only to the declared fallback; its test moved to target that path directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both findings were real and reproduced before fixing. Addressed in [P1] Capture transitive adapter readsReproduced exactly as described on I took the first of your two routes. Statically expanding transitive references would mean re-deriving each adapter's discovery logic in the identity builder and keeping it in step forever; the read boundary already knows the answer exactly, and it is the same boundary the receipt is bound to. Both remaining producers now observe it:
Committed-tree and worktree runs of the same tree now bind the same set, asserted as an invariant ( Same bug class showed up in a second adapter: on this repo One implementation note in case it saves someone time later: the snapshot root must be [P2] Preserve the agent-mode error envelopeFixed via A guard I nearly broke, and what it costMy first cut at the archived-head snapshot defeated a TOCTOU check, and the existing suite caught it: The obvious repair, giving the head snapshot those external paths too, traded one failure for another: two snapshots re-validating the same directory, where the second trips on a change the first legitimately allowed ( The working rule is ownership — each external input belongs to exactly one snapshot. The worktree snapshot binds them before the archived scan begins, which also widens the guarded window rather than narrowing it: it now starts before the scan instead of at the scan's first read. That constraint is recorded in the CHANGELOG, since nothing in the code makes it obvious that adding a second watcher would break it. Consequences worth a second look
Full suite green ( |
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
One newly reproduced blocking finding from the follow-up review; details are inline.
| # snapshot is not left active past the scan; it is the record of | ||
| # which paths were read, not the reader for the receipt. | ||
| if head_snapshot_token is not None: | ||
| reset_static_input_snapshot(head_snapshot_token) |
There was a problem hiding this comment.
[P1] Build the receipt from the captured bytes. Resetting the head snapshot here means _write_artifacts later supplies only head_snapshot.paths() while build_blob reopens the mutable archived files. I reproduced a mutation immediately after snapshot finalization: the resulting plan hashed the post-scan agent.py bytes referencing inventories/mcp-tools-2.json, but its tool_sources still bound the pre-scan inventories/mcp-tools.json and omitted the new inventory. The receipt can therefore attest to bytes the report never evaluated. Please keep the finalized snapshot active for plan/blob construction or pass its cached bytes/blobs forward; the same capture-reset-reread window exists in verification prepare, where _captured_input_paths finalizes and deactivates its snapshot before build_verification_plan reopens the files. Add mutation-after-capture regressions for both paths.
Review follow-up on #332. Capture recorded which paths the adapters opened and then released the snapshot, leaving `build_verification_plan` to reopen those files to hash them. The path list and the blob hashes therefore came from two different instants. A file rewritten in between is attested at its new content while `tool_sources` still lists what the old content pointed at — so the plan can bind a pre-scan `inventories/mcp-tools.json`, omit the inventory the post-scan `agent.py` actually names, and hash `agent.py` at bytes the report never evaluated. Reproduced on `prepare`: captured `agent.py` was sha256:7465..., the emitted plan recorded sha256:040c.... My earlier note here claimed the reset was safe because the archived tree is a private temporary directory. That reasoned about who can write the files and missed the actual defect, which is that the two halves of the plan are taken at different times regardless of the threat model. Plan construction now runs under the finalized snapshot on both paths: `verify` re-activates the head snapshot around the plan build, and `prepare`'s capture became a context manager that holds the snapshot open for it. Blobs are hashed from captured bytes and never reopened. That exposed a second-order requirement. `_blobs` skips a path the snapshot contains but never read, so binding the snapshot without also binding the changed files silently dropped every changed file no adapter opens — verified: `changed_files` goes to empty. The worktree path already preloaded changed files for exactly this reason; that block is now `_bind_changed_files` and is applied to the archived tree and to `prepare` as well. Both regressions fail against the unfixed code, checked by reverting each activation independently, and the committed-tree one also asserts an unrelated changed file survives capture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Confirmed and fixed in I had noticed the reset window and talked myself out of it with "plan construction hashes the archived files directly … the archive is a private temp dir that nothing else writes." That argues about who can write the files, which is not the defect. The defect is that the path list and the blob hashes are taken at two different instants, so the plan is internally inconsistent no matter who can write — exactly as you show, Reproduced on FixPlan construction now runs under the finalized snapshot on both paths, so blobs are hashed from captured bytes and the files are never reopened:
Second-order requirement this exposed
The worktree path already preloaded changed files for exactly this reason; that block is now Regressions
Full suite green ( Running tally of behaviour changes, for the final read-through
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
One newly reproduced blocking identity finding from the latest follow-up review; details are inline.
| candidate = resolved_root / relative | ||
| if candidate.is_file() and not candidate.is_symlink(): | ||
| snapshot.read_bytes(candidate, max_bytes=_MAX_CHANGED_FILE_BYTES) | ||
| snapshot.finish() |
There was a problem hiding this comment.
[P1] Bind the explicit baseline and comparison inputs before finalizing. Keeping this snapshot active through plan construction makes _optional_blob treat any contained-but-unread path as absent, but _captured_inputs currently reads adapter/policy inputs and changed files only. I reproduced two clean preparations on the same HEAD, one with --baseline baseline-a.json --diff-from comparison-a.json and one with different B files: both exited successfully, emitted inputs.baseline = null and inputs.diff_from = null, and produced the same input_set_id. The prepared request therefore ignores two explicit verification inputs. Please pass these paths into the capture context and bind their bytes before finish() (including correct external ownership for the archived-head case), with regressions asserting the blobs are present and that changing either input changes the identity.
…rations Review follow-up on #332. Enumerating the manifest reaches only what the manifest names. An input discovered while parsing something it names — a Google ADK `McpToolset` inventory, an OpenAPI spec constructed inside `agent.py`, a sub-agent config — is invisible to that walk, so two trees whose adapters read different bytes still shared an `input_set_id`. Reproduced on the ADK sample: the plan bound `agent.py`, the eval set, and the function inventory, but not `inventories/mcp-tools.json`, and a trailing newline on it left identity byte-identical. Both remaining producers now observe the read boundary instead. A committed-tree `verify` is snapshotted against the archived tree it scans, which the worktree-bound snapshot could never see, and `verification prepare` loads sources — statically, deciding nothing — to record what they open. Committed-tree and worktree runs of the same tree now bind the same set, asserted as an invariant rather than per-file. The declared-path enumeration survives only as the fallback for a plan built with no snapshot at all. A committed-tree run therefore has two snapshots alive, and each external input must belong to exactly one of them. The first attempt let the archived scan read the baseline and policy packs through no snapshot at all, silently defeating the tamper check that `test_archived_verify_rejects_external_baseline_change_after_scan` exists to enforce; the second let both watch the same directory, and the second re-validation then failed on a change the first legitimately allowed. The worktree snapshot now binds them before the archived scan starts, which also widens the guarded window: it begins before the scan rather than at the scan's first read. `verification prepare` reads inputs now, so it fails on a manifest whose inputs cannot be loaded — the same condition under which `verify` fails, and exactly when a prepared plan could not honestly claim an input set. Its errors carry the agent-mode envelope (`input_parse_error` / `config_error` with `next_action` and `next_actions`, per docs/errors.json) instead of prose alone. With capture in place, a declared path that no adapter opens is correctly not an input, so the out-of-root rejection now applies only to the declared fallback; its test moved to target that path directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on #332. Capture recorded which paths the adapters opened and then released the snapshot, leaving `build_verification_plan` to reopen those files to hash them. The path list and the blob hashes therefore came from two different instants. A file rewritten in between is attested at its new content while `tool_sources` still lists what the old content pointed at — so the plan can bind a pre-scan `inventories/mcp-tools.json`, omit the inventory the post-scan `agent.py` actually names, and hash `agent.py` at bytes the report never evaluated. Reproduced on `prepare`: captured `agent.py` was sha256:7465..., the emitted plan recorded sha256:040c.... My earlier note here claimed the reset was safe because the archived tree is a private temporary directory. That reasoned about who can write the files and missed the actual defect, which is that the two halves of the plan are taken at different times regardless of the threat model. Plan construction now runs under the finalized snapshot on both paths: `verify` re-activates the head snapshot around the plan build, and `prepare`'s capture became a context manager that holds the snapshot open for it. Blobs are hashed from captured bytes and never reopened. That exposed a second-order requirement. `_blobs` skips a path the snapshot contains but never read, so binding the snapshot without also binding the changed files silently dropped every changed file no adapter opens — verified: `changed_files` goes to empty. The worktree path already preloaded changed files for exactly this reason; that block is now `_bind_changed_files` and is applied to the archived tree and to `prepare` as well. Both regressions fail against the unfixed code, checked by reverting each activation independently, and the committed-tree one also asserts an unrelated changed file survives capture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fc68e48 to
89f5bb4
Compare
|
Reproduced and fixed in Activating the snapshot for plan construction turned binding from an optimization into an obligation: under an active snapshot both Audited rather than patched at the reported site
That is the complete Two details in the fix
Regressions
One trap worth recording for anyone writing similar tests: the output must land outside the repository. My first attempt wrote plans into the workspace, and the first run's artifacts became untracked changed files that moved identity on their own — which would have made the test pass for entirely the wrong reason. Rebase
Full suite green on the rebased tree ( Behaviour changes accumulated across this PR
|
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
A new blocking identity-integrity finding from the latest review is attached inline.
| token = activate_static_input_snapshot(snapshot) | ||
| try: | ||
| try: | ||
| resolved = _prepare_scan( |
There was a problem hiding this comment.
[P1] Parse the manifest from the captured bytes. The snapshot is active here, but _prepare_scan() receives no manifest_text; load_manifest_with_positions() therefore first parses the manifest through direct Path.read_text() and only then rereads it through read_static_input_text() for positions. A mutation between those reads makes _load_inputs() follow the old manifest while build_verification_plan() hashes the newer cached manifest. I reproduced this in both verification prepare and committed-tree verify: both exited successfully, and the latter produced a valid terminal receipt whose config SHA covered a manifest naming agent-two.py while tool_sources still bound agent.py from the earlier manifest. Please read the manifest once through the active snapshot and pass those exact bytes/text into _prepare_scan() and the archived run_scan() path, with a regression that mutates the manifest after the first parse.
…rations Review follow-up on #332. Enumerating the manifest reaches only what the manifest names. An input discovered while parsing something it names — a Google ADK `McpToolset` inventory, an OpenAPI spec constructed inside `agent.py`, a sub-agent config — is invisible to that walk, so two trees whose adapters read different bytes still shared an `input_set_id`. Reproduced on the ADK sample: the plan bound `agent.py`, the eval set, and the function inventory, but not `inventories/mcp-tools.json`, and a trailing newline on it left identity byte-identical. Both remaining producers now observe the read boundary instead. A committed-tree `verify` is snapshotted against the archived tree it scans, which the worktree-bound snapshot could never see, and `verification prepare` loads sources — statically, deciding nothing — to record what they open. Committed-tree and worktree runs of the same tree now bind the same set, asserted as an invariant rather than per-file. The declared-path enumeration survives only as the fallback for a plan built with no snapshot at all. A committed-tree run therefore has two snapshots alive, and each external input must belong to exactly one of them. The first attempt let the archived scan read the baseline and policy packs through no snapshot at all, silently defeating the tamper check that `test_archived_verify_rejects_external_baseline_change_after_scan` exists to enforce; the second let both watch the same directory, and the second re-validation then failed on a change the first legitimately allowed. The worktree snapshot now binds them before the archived scan starts, which also widens the guarded window: it begins before the scan rather than at the scan's first read. `verification prepare` reads inputs now, so it fails on a manifest whose inputs cannot be loaded — the same condition under which `verify` fails, and exactly when a prepared plan could not honestly claim an input set. Its errors carry the agent-mode envelope (`input_parse_error` / `config_error` with `next_action` and `next_actions`, per docs/errors.json) instead of prose alone. With capture in place, a declared path that no adapter opens is correctly not an input, so the out-of-root rejection now applies only to the declared fallback; its test moved to target that path directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on #332. Capture recorded which paths the adapters opened and then released the snapshot, leaving `build_verification_plan` to reopen those files to hash them. The path list and the blob hashes therefore came from two different instants. A file rewritten in between is attested at its new content while `tool_sources` still lists what the old content pointed at — so the plan can bind a pre-scan `inventories/mcp-tools.json`, omit the inventory the post-scan `agent.py` actually names, and hash `agent.py` at bytes the report never evaluated. Reproduced on `prepare`: captured `agent.py` was sha256:7465..., the emitted plan recorded sha256:040c.... My earlier note here claimed the reset was safe because the archived tree is a private temporary directory. That reasoned about who can write the files and missed the actual defect, which is that the two halves of the plan are taken at different times regardless of the threat model. Plan construction now runs under the finalized snapshot on both paths: `verify` re-activates the head snapshot around the plan build, and `prepare`'s capture became a context manager that holds the snapshot open for it. Blobs are hashed from captured bytes and never reopened. That exposed a second-order requirement. `_blobs` skips a path the snapshot contains but never read, so binding the snapshot without also binding the changed files silently dropped every changed file no adapter opens — verified: `changed_files` goes to empty. The worktree path already preloaded changed files for exactly this reason; that block is now `_bind_changed_files` and is applied to the archived tree and to `prepare` as well. Both regressions fail against the unfixed code, checked by reverting each activation independently, and the committed-tree one also asserts an unrelated changed file survives capture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
89f5bb4 to
3c8060d
Compare
|
Confirmed and fixed in
I also resolve the archived tree at creation rather than at each use, so the snapshot's lexical matching sees one spelling. That is the same Two correct outcomes, so the test asserts the invariantWith the manifest read through the snapshot, The regression took three attempts to make honestEach of the first two passed against known-broken code, which is worth recording because the failure mode is silent:
Rebase
Behaviour changes accumulated across this PR
Item 1 is the one I would still like your explicit call on before merge: it is the only change here that can turn a previously-succeeding |
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
One additional error-contract regression from the latest review is attached inline.
| packet_enabled=None, | ||
| packet_formats=None, | ||
| baseline_mode="new-findings", | ||
| manifest_text=read_static_input_text( |
There was a problem hiding this comment.
[P2] Preserve the missing-manifest config route. Reading the manifest here means FileNotFoundError is caught by the surrounding except (OSError, ValueError) and converted into InputParseError. I reproduced verification prepare in a clean Git repository without shipgate.yaml: it now exits 3 with error=input_parse_error, describes the config as an input that changed, and recommends generic review instead of emitting the established config_error/exit-2 missing-manifest setup route. This breaks agent callers that branch on the published error contract. Please preserve the loader's missing-config semantics—e.g. map a missing config_path to ConfigError before snapshot capture—while retaining InputParseError for changes after capture, and add an agent-mode regression for the error kind, exit code, and next action.
|
Confirmed and fixed in Reading the manifest through the snapshot put its Two changes:
Regressions
Full suite green, A pattern worth namingThis is the third defect in a row with the same shape: I made a read snapshot-aware without checking what the surrounding error handling then did with the new exception, or what the surrounding blob helpers did with a path the snapshot now contains. Making a read identity-bound changes its failure modes and its absence semantics, and both need re-examining at every call site, not just the one being edited. I have stopped treating "the read now goes through the snapshot" as a local change. Behaviour changes accumulated across this PR
Item 1 remains the only one that can turn a previously-succeeding |
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
One additional configuration-routing finding from the latest review is attached inline.
| actions = top_next_actions( | ||
| _diagnose_config_error( | ||
| config=str(config), | ||
| workspace=workspace, |
There was a problem hiding this comment.
[P2] Classify the exact config that failed. Passing workspace here makes _diagnose_config_error() ignore the explicit config and recursively discover any shipgate.yaml under the workspace. I reproduced verification prepare --workspace . --config absent-review-manifest.yaml: it emitted config_error/exit 2 but told the caller to edit the unrelated valid benchmark/perf/scenarios/large/shipgate.yaml. Monorepos and custom --config callers may therefore modify the wrong file. Please diagnose the resolved requested config with workspace=None (or give the explicit candidate precedence), and add a regression containing a missing requested manifest plus another valid manifest.
`input_set_id` is the identity `verification-plan.json`, `verification-unit-result.json`, `verify-run.json`, the terminal receipt, and attestations all rest on, and its whole claim is that two runs sharing it read the same bytes. Two producers broke that claim. The manifest-derived branch of `build_verification_plan` walked only `tool_sources`, so `openai_api.prompt_files` — and every other framework block that names paths — never became a plan blob. Rewriting a prompt to say refunds need no approval left `input_set_id` byte-identical. Worse, the observed branch was inert on the committed-tree path. `verify --base X --head Y` scans an archived copy of the head tree while the static-input snapshot is bound to the worktree, so it recorded no adapter reads and `active_snapshot.paths()` returned `[]` — not `None`, so plan construction honoured an empty capture instead of falling back. On the CI path, where the receipt is the artifact anyone downstream actually trusts, no declared input reached the request identity at all, not even the MCP exports and OpenAPI specs the fallback would have caught. The two modes now agree. A committed-tree run enumerates the manifest's declared inputs against the tree it actually scanned, and both branches share one exclusion set so an input already hashed as a changed file is not hashed twice. `verify` against a worktree is unchanged — read-boundary capture already covered it, which is why the regression tests drive `verification prepare` and the committed-tree path instead. The declared-path table is derived from the manifest models rather than hand-kept, so a new artifact list on an existing block, or a whole new framework block, is covered without editing the enumeration. One new way to fail: a declared input resolving outside the verification input root cannot be hashed portably, so it is rejected rather than dropped. `resolve_input_path` already rejected the same declaration the moment an adapter read it, so this only reaches manifests naming an out-of-root path nothing loads yet. It routes as an input error (exit 3) with the path named; `verification prepare` routes input errors at all now, instead of printing a traceback. No schema changes: `plan.inputs.tool_sources` gains entries, not fields. Existing `input_set_id` and `request_id` values do move for manifests that declare framework inputs or that were verified with `--head`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rations Review follow-up on #332. Enumerating the manifest reaches only what the manifest names. An input discovered while parsing something it names — a Google ADK `McpToolset` inventory, an OpenAPI spec constructed inside `agent.py`, a sub-agent config — is invisible to that walk, so two trees whose adapters read different bytes still shared an `input_set_id`. Reproduced on the ADK sample: the plan bound `agent.py`, the eval set, and the function inventory, but not `inventories/mcp-tools.json`, and a trailing newline on it left identity byte-identical. Both remaining producers now observe the read boundary instead. A committed-tree `verify` is snapshotted against the archived tree it scans, which the worktree-bound snapshot could never see, and `verification prepare` loads sources — statically, deciding nothing — to record what they open. Committed-tree and worktree runs of the same tree now bind the same set, asserted as an invariant rather than per-file. The declared-path enumeration survives only as the fallback for a plan built with no snapshot at all. A committed-tree run therefore has two snapshots alive, and each external input must belong to exactly one of them. The first attempt let the archived scan read the baseline and policy packs through no snapshot at all, silently defeating the tamper check that `test_archived_verify_rejects_external_baseline_change_after_scan` exists to enforce; the second let both watch the same directory, and the second re-validation then failed on a change the first legitimately allowed. The worktree snapshot now binds them before the archived scan starts, which also widens the guarded window: it begins before the scan rather than at the scan's first read. `verification prepare` reads inputs now, so it fails on a manifest whose inputs cannot be loaded — the same condition under which `verify` fails, and exactly when a prepared plan could not honestly claim an input set. Its errors carry the agent-mode envelope (`input_parse_error` / `config_error` with `next_action` and `next_actions`, per docs/errors.json) instead of prose alone. With capture in place, a declared path that no adapter opens is correctly not an input, so the out-of-root rejection now applies only to the declared fallback; its test moved to target that path directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on #332. Capture recorded which paths the adapters opened and then released the snapshot, leaving `build_verification_plan` to reopen those files to hash them. The path list and the blob hashes therefore came from two different instants. A file rewritten in between is attested at its new content while `tool_sources` still lists what the old content pointed at — so the plan can bind a pre-scan `inventories/mcp-tools.json`, omit the inventory the post-scan `agent.py` actually names, and hash `agent.py` at bytes the report never evaluated. Reproduced on `prepare`: captured `agent.py` was sha256:7465..., the emitted plan recorded sha256:040c.... My earlier note here claimed the reset was safe because the archived tree is a private temporary directory. That reasoned about who can write the files and missed the actual defect, which is that the two halves of the plan are taken at different times regardless of the threat model. Plan construction now runs under the finalized snapshot on both paths: `verify` re-activates the head snapshot around the plan build, and `prepare`'s capture became a context manager that holds the snapshot open for it. Blobs are hashed from captured bytes and never reopened. That exposed a second-order requirement. `_blobs` skips a path the snapshot contains but never read, so binding the snapshot without also binding the changed files silently dropped every changed file no adapter opens — verified: `changed_files` goes to empty. The worktree path already preloaded changed files for exactly this reason; that block is now `_bind_changed_files` and is applied to the archived tree and to `prepare` as well. Both regressions fail against the unfixed code, checked by reverting each activation independently, and the committed-tree one also asserts an unrelated changed file survives capture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Running plan construction under the finalized snapshot turned binding from an optimization into an obligation. Under an active snapshot both `_blobs` and `_optional_blob` read a path that is *contained but never read* as absent, so an input nobody bound does not merely go unhashed — it disappears from the plan. I named that trap for `changed_files`, fixed that one case, and did not sweep the rest, so `--baseline` and `--diff-from` silently dropped: two preparations of one tree with different A/B files emitted `baseline: null`, `diff_from: null`, and the same `input_set_id`. Every input `build_verification_plan` hashes is now bound before the snapshot is sealed — the adapters' own reads, the changed files, and the explicit baseline, comparison report, and policy packs. Audited rather than patched at the reported site: `_blobs` and `_optional_blob` are the two helpers that drop unread paths, and every call to them inside the plan builder is covered. `build_blob` and `sha256_file` gate on `has()` with a direct-read fallback, so they cannot drop. The comparison report is never mapped into the archived tree, so on a committed-tree preparation it is bound as an external input; without that the snapshot refuses to read it at all. The archive directory is also resolved once it exists. macOS reaches it through /var while every derived path resolves to /private/var, and the snapshot matches lexically, so two spellings make `contains()` false for inputs plainly inside the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`load_manifest_with_positions` reads the manifest twice — a direct `Path.read_text` to build the model, then through the snapshot to build the position index. A rewrite between the two lets the adapters follow one manifest while the plan's config blob attests to another, so a receipt could name an entrypoint the scan never opened. The worktree path has always dodged this by passing its captured `worktree_manifest_text`. The committed-tree path passed None, and the `prepare` capture added earlier in this branch passed nothing either. Both now read the manifest once through the active snapshot and hand those exact bytes to `_prepare_scan` / `run_scan`. The archived tree is also resolved at creation rather than at each use, so the snapshot's lexical matching sees one spelling: on macOS the temporary directory is reached through /var while every adapter resolves its base directory to /private/var. Two correct outcomes follow, and the regression asserts the invariant rather than either mechanism: `prepare` now fails closed because `finish()` catches the rewrite, while committed-tree `verify` emits a self-consistent plan. What must never happen is succeeding with a plan that disagrees with the manifest the scan followed. Getting that regression honest took three attempts, each of which passed against known-broken code: the spy first rewrote the worktree manifest, which a committed-tree scan never opens; then the base-tree scan absorbed the single rewrite before the head scan parsed; and the committed variant still passed when only the `manifest_text` wiring was reverted, because the new early snapshot read masked it. It fails only with that read removed as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading the manifest through the snapshot put its FileNotFoundError inside the capture block, where a broad `except (OSError, ValueError)` recast it as `input_parse_error` / exit 3 — telling an agent the config changed while it was being read when it simply is not there, and offering generic review instead of setup. A caller branching on the published error contract is stranded by that. The absent case is now resolved before capture, delegating to `load_yaml_file` so the message and its `init` hint stay in one place. The `ConfigError` handler also stops guessing. It routes through the shared diagnostic catalog (`_diagnose_config_error` + `top_next_actions`), the same path `scan` and `doctor` use, so an absent manifest gets the setup route, an unparseable one gets the edit route, and an unresolved adapter gets its own. The hand-rolled handler added earlier in this branch always emitted an "edit the manifest" action, which is wrong for a manifest that does not exist. Two agent-mode regressions: the missing-manifest kind, exit code, and next action, and an unparseable manifest still reaching the edit route rather than being flattened into the same answer. Without the guard the first reproduces the reported exit 3 / input_parse_error verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Routing prepare's ConfigError through the shared diagnostic catalog last commit copied `scan`'s call shape without checking what the arguments mean. `_candidate_manifest_paths` short-circuits on `workspace is not None` and recursively discovers every `shipgate.yaml` beneath it, ignoring the explicit `--config` entirely, so the first discovered file that happens to parse became the subject of the recovery action. Reproduced in this repository: `verification prepare --workspace . --config absent-review-manifest.yaml` named the absent file in its message but told the caller to edit `benchmark/perf/scenarios/large/shipgate.yaml`, an unrelated and perfectly valid fixture. A monorepo, or any invocation naming a config that is not there, would be routed at someone else's manifest. `prepare` always resolves exactly one manifest path, so there is nothing to discover: diagnose that resolved path with `workspace=None`. `scan` passes both arguments and is right to, because its `-c` may be absent or a glob; the difference is what I failed to check. The regression plants a second valid manifest at packages/other/shipgate.yaml and asserts it appears nowhere in `next_actions`. Reverting the fix fails it with that exact path in an edit action. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
45e2e18 to
2f1b2b7
Compare
|
Confirmed and fixed in When I switched to the shared diagnostic catalog I copied Reproduced in this repository: The message correctly named the absent file;
Both routes verified after the fix: an absent Regression
One caveat on my local verification
Behaviour changes accumulated across this PR
Item 1 remains the only one that can turn a previously-succeeding |
Resolves the collision with #332 (declared adapter input identity). #332 refactored `prepare` into `_build_plan`/`_captured_inputs` and extracted `_bind_changed_files`, and made `_worktree_overlay` treat a path the static snapshot contains but never read as absent. This branch adds a second, deliberately disjoint path set — `worktree_overlay_paths` is HEAD-relative while `changed_files` is merge-base-relative — so a canceled path exists only in the former. Binding just `changed_files` therefore recorded a present canceled file as `deleted`, and `verification worker` failed with "worker worktree overlay does not match the plan". Both binding sites now bind the union.
The merge with #332 exposed a semantic conflict the suite could not see. #332 makes the static input snapshot report a path it contains but never read as absent, and this branch's overlay set is HEAD-relative while the change set is merge-base-relative — so a cancelled-but-present path is bound by neither unless both binding sites take the union. The existing cancellation test cancels by deleting the file, so the producer records "deleted" either way and the corruption is invisible. This one cancels by restoring merge-base content, then calls _validate_git_subject: it fails with "worker worktree overlay does not match the plan" if either binding site drops back to changed_files.
* Normalize effective worktree verification diffs * Address verification diff review feedback * test: cover a cancelled overlay path that is still present The merge with #332 exposed a semantic conflict the suite could not see. #332 makes the static input snapshot report a path it contains but never read as absent, and this branch's overlay set is HEAD-relative while the change set is merge-base-relative — so a cancelled-but-present path is bound by neither unless both binding sites take the union. The existing cancellation test cancels by deleting the file, so the producer records "deleted" either way and the corruption is invisible. This one cancels by restoring merge-base content, then calls _validate_git_subject: it fails with "worker worktree overlay does not match the plan" if either binding site drops back to changed_files.
Summary
input_set_idnow covers every input an adapter is configured to read, not justtool_sources[].build_verification_planwalked onlyraw["tool_sources"], soopenai_api.prompt_files— andanthropic,google_adk,langchain,crewai,n8n,codex_plugins,validation.evidence,checks.policy_packs,agent.sdk.entrypoint— never became a plan blob. Reproduced onverification prepareagainstsamples/simple_openai_api_agent: appending "Refunds of any amount need no approval" to the prompt leftinput_set_idbyte-identical.verify --base X --head Yemittedtool_sources: []entirely.StaticInputSnapshotis rooted atgit_root, but a committed-tree run scans an archived copy under/tmp/agents-shipgate-verify-head-*, so it records no adapter reads.active_snapshot.paths()returned[]— which is notNone, so plan construction honoured an empty capture instead of falling back. On the CI path, where the receipt is the artifact anyone downstream actually trusts, no declared input reached the request identity — not even the MCP exports and OpenAPI specs the old fallback would have caught.Type
Verification
CI is authoritative for
python -m ruff check .,python -m compileall -q src tests, andpython -m pytest.Additional local checks run:
pytest -n auto -m "not perf"(exit 0);ruff check .clean.tests/test_declared_manifest_input_identity.py. Confirmed load-bearing by reverting each fix in turn: reverting the orchestrator change failstest_committed_tree_verify_binds_declared_tool_sources; reverting the enumeration fails four others.verification prepareand the committed-tree path.verifyagainst a worktree was already correct (read-boundary capture covers it, including a prompt file absent from the diff), so a worktree test would pass for the wrong reason — the issue's triage note called this out.verify --base main --head HEADon this repo now binds.agents/plugins/marketplace.jsonwhere it previously bound nothing; receiptinput_set_idmatches the plan; decisionpassed.config_error/exit 2 and still writes a plan carrying the config blob; an out-of-root declared path routesinput_parse_error/exit 3.Release-readiness notes
docs/checks.md— n/a, no check IDs touchedSTABILITY.md— no schema change;plan.inputs.tool_sourcesgains entries, not fieldsNotes for the reviewer
Why the path table is derived, not hand-kept.
schemas/manifest/declared_paths.pywalks rawyaml.safe_loadoutput (identity must stay constructible for a manifest that fails validation) and recognizes paths by two rules: apath:key anywhere in a path-bearing block, plus any key named after a field whose type carriesArtifactPathConfig. That second rule is load-bearing —_parse_artifact_entriesalso accepts a bare string (tools: [tools/openai.json]), which apath:-only walk misses. The key set is read off the pydantic models at import time, so a new artifact list, or a whole new framework block, is covered with no edit here. Only non-ArtifactPathConfigstring paths need naming by hand (_UNTYPED_PATH_FIELDS):agent.sdk.entrypoint, bothprompt_files, andtool_sources(registered for thepath:rule alone).Deliberately not inputs, to stay symmetric with what read-boundary capture sees:
output.directory(an output),organization.audit.registry(existence-tested, never read),baseline.audit_log(resolved against the baseline file, not an adapter input).Two things worth a second opinion:
agent.sdk.entrypointwith noopenai_agents_sdktool source, sinceresolve_input_pathrejects the rest at scan time. Fail-closed felt right — a silently unbound declared input is the class this issue is about — but it will hard-fail a manifest that verifies today. My first attempt raisedValueError, which surfaced asinternal_error/exit 4 ("file an issue"); that is the wrong routing for something the user must fix in their manifest, henceInputParseError.input_set_idandrequest_idvalues move for any manifest declaring framework inputs, and for every--headrun. Receipts minted before and after this change cannot be compared by ID. Sampleexpected/report.jsongoldens are unaffected (they carryinput_set_id: null—scanbuilds no plan).Generalizable bug class, worth remembering beyond this PR: when a code path selects between "observed" and "declared" on
x is None, an observer that is inert rather than absent silently wins with an empty result. Check that the observer could have seen anything at all, not just that it exists.🤖 Generated with Claude Code