Normalize committed and worktree verification diffs - #348
Conversation
Engineering review — request changesThe core design is right and I'd take it. One line ( Base & suiteNot stale — merge-base == Committed The design is sound. Merge-base→effective-worktree is the correct single comparison; fail-closed on merge-base resolution reuses #340's classification vocabulary correctly; and human/merge authorization still requires Worth calling out a pre-existing incoherence this quietly fixes: BLOCKER —
|
changed_files |
headline | |
|---|---|---|
main |
['lib/util.py'] (1) |
5 active finding(s) block release… |
| this PR | 18 paths — the whole repo, including samples/support_refund_agent/shipgate.yaml |
This PR edits a release trust root … a coding agent cannot self-approve that change — a human must review it. |
Every tracked file gains a phantom old mode 100644 / new mode 100755 record, the manifest lands in the change set, and an ordinary one-file edit becomes a TRUST-ROOT human-review wall.
On a large repo the same noise will blow _DIFF_METADATA_LIMIT / _DIFF_BODY_LIMIT and hard-stop with exit 2.
This is not a fail-open — but it is the self-block loop-breaker this epic is meant to delete, and it ships green because CI runs on Linux where fileMode is already true.
_reject_unbound_diff_configuration (git.py:1456) does not screen core.*, only diff.* and info/attributes, so nothing catches it.
Fix
Omit core.fileMode from the worktree list rather than forcing it to true — let the repository's own effective setting decide. Verified both directions:
- normal checkout (git default
true): real mode changes still surface withold mode/new mode, sotest_effective_worktree_diff_preserves_rename_and_mode_semanticsstill passes; fileMode=falsecheckout: the phantom records vanish.
If you'd rather keep every diff knob receipt-bound, the alternative is to drop the override entirely and accept that mode changes aren't part of the worktree policy diff (main's behavior). Note the diff already depends on unbound on-disk exec bits, and the overlay identity records the mode independently via stat(), so honoring the repo setting costs nothing you weren't already exposed to.
MEDIUM — the "compatibility reader" for pre-#336 plans cannot succeed
cli/verification.py:531. When worktree_overlay_paths is absent the worker falls back to plan.inputs.changed_paths — but the same commit added git_mode to the overlay rows, so a pre-#336 plan's worktree_overlay_sha256 was computed over rows without that key.
actual_overlay != subject.worktree_overlay_sha256 for every non-empty overlay. The branch only "works" when the overlay is empty, where it is a no-op.
Either delete it and fail with an explicit "this plan predates overlay mode binding — re-run verification prepare", or version the overlay row schema. As written the comment promises compatibility the code cannot deliver.
MEDIUM — a canceled committed change is invisible in the human/agent surface
Probe: a commit adds a wire_transfer function tool to the sample agent, the worktree reverts it → this PR reports changed_files == [] and an empty diff. test_worktree_overlay_identity_preserves_an_effectively_cancelled_path asserts this shape.
Under the stated contract this is correct, and it is well fenced:
_bind_worktree_config_to_headkeeps the manifest bound even when canceled (confirmed — a canceledshipgate.yamlweakening still surfaces);- authorization rejects worktree subjects outright.
But verify is the coding agent's primary signal, and "0 changed files" on a branch with real committed changes reads as "nothing to see." The plan records worktree_overlay_paths; nothing in verifier.json or the headline does.
Suggested: a base note when the merge_base..HEAD path set is not a subset of the effective change set — "N committed change(s) are canceled by uncommitted worktree edits; the committed branch has not been verified."
LOW — prepare reads a full diff body it throws away
cli/verification.py:86: when --base is given, overlay_diff is unused — only the path list is needed. Use working_tree_paths(), which this PR added to the orchestrator for exactly this.
As written, a large HEAD-relative overlay can trip body_limit_exceeded and fail prepare even when the effective merge-base diff is trivially small (i.e. the cancellation case this PR is about), and _reject_index_hidden_capability_paths runs twice.
LOW — git_mode uses any-exec-bit; Git uses owner-exec only
core/verification_identity.py:972 and cli/verification.py:556 both test & 0o111. Git records 100755 off S_IXUSR alone, so a chmod g+x-only file is labeled "100755" here while Git's tree says 100644.
Producer and worker agree, so identity stays self-consistent — but the field name and the _SAFE_WORKTREE_DIFF_CONFIG comment both claim Git semantics. Use & stat.S_IXUSR.
LOW — the binary-hiding guard runs under a different config than the body it certifies
_reject_binary_capability_paths still uses _SAFE_DIFF_CONFIG (git.py:820) while the body and name-status it guards now use _SAFE_WORKTREE_DIFF_CONFIG. Benign today (numstat emits 0\t0 for mode-only changes, not -\t-), but two views of the same comparison should not disagree on config.
NIT — no CHANGELOG entry
## Unreleased has a substantive entry for #340 and none for this. This changes what verify evaluates locally and changes input_set_id / subject_id for every worktree plan (new options.worktree_overlay_paths plus git_mode in overlay rows). Both deserve a line.
Acceptance criteria (#336)
All met except one gap: "add, delete, rename, mode-change, staged-only, unstaged-only, and untracked cases remain fail-closed" — untracked is not covered by the new tests.
Untracked collection is comparison-ref-independent (ls-files --others, git.py:1305), so behavior is unchanged, but a merge-base-relative untracked case is worth one assertion.
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.
Resolves the collision with #347 (atomic current-control pointer). Three semantic conflicts, none of them textual: 1. #347 rewrote the overlay row builder (`_overlay_entry`) to bind entry kind and the executable bit, superseding this branch's `git_mode`. Dropped `git_mode`, the now-dead `StaticInputSnapshot.mode()` it needed, and the test whose owner-execute premise #347 replaces — #347 already covers both the chmod and symlink mutations. 2. #347 exposed `worktree_overlay` so "the same function builds the overlay a plan commits to and the overlay a later reader recomputes", but neither reader was wired to it: `verification worker` still hand-rolled the older row shape. That is a live bug on main — `prepare` then `worker` fails with "worker worktree overlay does not match the plan" on any dirty worktree. Both readers now call the shared builder. 3. #347's current-control reader recomputed the overlay from `inputs.changed_paths`, which this branch redefines as the merge-base- relative evaluated set. Added `plan_worktree_overlay_paths` so the producer, the worker, and the control reader take the HEAD-relative overlay set from one place instead of three. `prepare` also now runs after #347's `begin_current_control` invalidation, so the pointer is still cleared before any input is read.
…fication diffs #348 (issue #336) landed the committed+worktree diff normalization. No version collision this time — it moved no schema identifiers — and only CHANGELOG.md conflicted textually; both unreleased entries are kept. Checked for the semantic conflict #348's own history warns about, since a clean text merge proves nothing when two branches change the same substrate. This branch gates publication on `diff_status.completeness == "complete"` and on a succeeded, non-blocked release decision, and #348 changes how that diff is assembled. Verified end to end across all three shapes it now produces: an uncommitted worktree run, a committed-tree run, and the overlay case #348 exists for — a committed `base..HEAD` range with an uncommitted edit on top. All three report `diff_status: complete` and reach `review_publishable` with publication authorized and merge denied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Why
Verification previously reasoned about committed changes and worktree overlays through partially separate diff views. When the same path appeared in both layers—or a local edit canceled a committed change—the policy diff and verification identity could disagree about the effective state.
This change makes the policy surface reflect exactly what would exist if the current worktree were committed, while preserving a separately content-addressed overlay set for reproducibility and worker validation.
Impact
Local and PR verification now produce one coherent structural diff across committed and uncommitted changes. Cancellation cases disappear from policy evaluation but remain bound in overlay identity, and executable-bit changes are retained deterministically.
Validation
pytest tests/test_packaging.py -q: 8 passedpytest tests/test_adapter_static_only.py -q: passedgit diff --checkpassedcontrol.state=complete,merge_verdict=mergeable,release_decision.decision=passed, 0 blockers, 0 review itemsCloses #336