ISS-7382: resolve review paths against the reviewed worktree

[mikeangstadt]

Purpose

ISS-7382. On any lane running /code-review from a worktree, all six reviewers resolved repo-relative source paths against the invoking session's working directory instead of the checkout under review — they were pointed at a different tree than the diff. They happened to refuse to fabricate; a reviewer that instead reviewed the wrong checkout would return a confident clean report on code it never read, and a clean review is exactly the signal a lane uses to decide it is done. Every worktree-based lane runs /code-review from a non-session cwd, so this was the default configuration.

Root cause: resolve-scope emitted review_root: "" for every scope kind except local PR-head worktree isolation, and both shared_prompt.txt and verifier_prompt.txt read empty as "read repo-relative paths from the working directory". A reviewer/verifier is a spawned Task, so its working directory is the session's checkout.

What changed

• resolve-scope resolves review_root for every scope kind — the PR-head worktree when one was created, otherwise git rev-parse --show-toplevel of the invoking checkout — records the tip it resolved at as review_root_sha, and exits non-zero rather than emit an empty root. Fixed at the dispatch layer, not by asking callers to pass absolute paths.
• Every stage that hands work to an agent re-proves the root (derive-spawn-spec, derive-static-spec, verify-prepare, review-dismissed-prepare): absolute, free of prompt markup, exists, is the root of a git worktree, still on review_root_sha, and contains every non-removed file in diff_data.json.
• The refusal aborts the walker. Those three dispatch stages are on_failure: continue in stages.json, so a plain non-zero return would degrade to the static reviewer table and spawn the same agents against the same wrong tree. A refusal exits REVIEW_ROOT_EXIT_CODE (3) and _execute_stage_inprocess aborts on that code regardless of on_failure. Scoped to that one code, so the stages' existing degradations (malformed coverage plan, missing partitions.json) are untouched.
• The spawn spec carries review_root, and both agent prompts now require it, tell the agent to discard anything it resolved elsewhere, and to stop rather than fall back to its own directory. An empty root is a hard error, never a silent fallback to cwd.
• A positional revision range is rejected. /code-review origin/main...HEAD parsed as a pathspec, matched nothing, and reported clean with no error; resolve-scope now refuses a positional token that is a git ref or range and does not exist as a path, and points at --base.
• The destructive-teardown validator keeps its narrow contract under the name _validated_worktree_path (only <cr_dir>/pr_head_worktree is accepted, because stage_30_footer deletes what it accepts). The widened read-side resolution is a separate function no teardown path consumes. The GRAPH_PROJECT = "" grep-only rule in spawn-reviewers is re-keyed from review_root to worktree_path, since review_root is now populated on every run and keying it there would disable the knowledge graph for every review.

Affected plugins / files

plugins/code-review (3.7.0 → 3.8.0):

• tools/python/code_review_helpers.py — _git_toplevel, _git_head_at, _diff_changed_files, _require_review_root, _read_review_root, _ref_like_scope_arg, ReviewRootError, REVIEW_ROOT_EXIT_CODE, _validated_review_root → _validated_worktree_path; guards in cmd_resolve_scope, cmd_verify_prepare, cmd_review_dismissed_prepare, cmd_derive_spawn_spec, cmd_derive_static_spec; abort override in _execute_stage_inprocess
• tools/prompts/shared_prompt.txt, tools/prompts/verifier_prompt.txt
• skills/spawn-reviewers/SKILL.md, skills/verify-findings/SKILL.md, commands/start.md
• tools/python/test_code_review_helpers.py, regenerated tools/python/prefix_fixtures/*/expected/{scope,spawn}.json
• .claude-plugin/plugin.json, root CHANGELOG.md

Commands run

uv sync --frozen --group dev            # EXIT=0
uv run ruff check .                     # EXIT=0
uv run pyright                          # EXIT=0
uv run pytest plugins/ -q               # EXIT=0 — 2129 passed, 3 skipped (baseline on main: 2107 passed)
uv run pytest plugins/code-review/tools/python/test_prefix_golden.py -q --update-golden   # goldens regenerated and committed

Counterfactual proof

Each new assertion was proved to fail under the exact mutation it claims to prevent, isolated to the single test with -k, then restored and re-run green.

Test	Mutation	Verbatim failure
TestISS7382ReviewRootDispatch::test_branch_review_pins_the_invoking_checkout_as_review_root	review_root = worktree_path (drop the toplevel fallback) + disable the empty-root error	AssertionError: assert '' == '/private/var.../lane_worktree'
TestISS7382ReviewRootDispatch::test_reviewer_dispatch_against_a_mismatched_checkout_errors	missing = [] in _require_review_root	assert 0 == 1 (before the exit code moved to 3)
TestVerifyPrepareReviewRoot::test_root_missing_the_diffs_files_errors	missing = []	assert 0 == 1
TestISS7382ReviewRootDispatch::test_empty_review_root_is_a_hard_error_not_a_cwd_fallback	empty-root branch returns "" instead of raising	assert 0 == 1
TestISS7382ReviewRootDispatch::test_positional_revision_range_is_rejected	_ref_like_scope_arg returns ""	assert 0 == 1
TestVerifyPrepareReviewRoot::test_pr_head_worktree_moved_off_the_resolved_commit_errors	disable the review_root_sha check	assert 0 == 1
TestVerifyPrepareReviewRoot::test_root_proven_by_nothing_errors	if not recorded_sha and not changed_files: → if False:	assert 0 == 3
TestVerifyPrepareReviewRoot::test_live_checkout_without_the_resolved_commit_errors	drop the _git_commit_present reachability check	assert 0 == 3
TestVerifyPrepareReviewRoot::test_prompt_markup_in_a_real_root_still_errors	if _REVIEW_ROOT_FORBIDDEN.search(raw): → if False:	assert 0 == 3
TestVerifyPrepareReviewRoot::test_relative_review_root_errors	if not os.path.isabs(raw): → if False:	assert 0 == 3
TestVerifyPrepareReviewRoot::test_subdirectory_of_a_checkout_errors	if _git_toplevel(root) != root: → if False:	assert 0 == 3
TestISS7382ReviewRootDispatch::test_positional_bare_ref_is_rejected	drop the git-ref probe in _ref_like_scope_arg	assert 0 == 1

Two rounds of fixtures were corrected because a different guard was satisfying the assertion — this is what the counterfactual is for:

• The mismatched-checkout case also carried a review_root_sha, so the commit check satisfied the same non-zero. Corrected to isolate containment (23bd85f); the commit check keeps its own cases.
• The markup / is-absolute / worktree-root cases were all decided by the "proven by nothing" rule, because their fixtures recorded no commit and no changed files. Each now records a real commit, so only the guard under test can refuse it (066aba4).

One check is deliberately not isolated: os.path.isdir is redundant with the worktree-root check (a nonexistent path cannot be a git worktree root), so no input refuses on it alone. It is kept for its distinct diagnostic, and the refusal behavior is pinned by test_subdirectory_of_a_checkout_errors and test_nonexistent_review_root_errors.

Negative assertions carry positive siblings in the opposite state: test_root_holding_the_diffs_files_is_accepted, test_reviewer_dispatch_against_the_reviewed_checkout_succeeds, test_positional_file_paths_are_still_accepted, test_removed_file_does_not_have_to_exist_under_the_root, and test_the_guarded_dispatch_stages_are_on_failure_continue (which keeps the abort-override test from going vacuous if those stages ever become abort on their own). Exit code 3 is written as a literal in the tests, never imported, so a change to REVIEW_ROOT_EXIT_CODE is detectable.

Review

Six reviewers ran against this branch: Bug Hunter A, Bug Hunter B, Unified Auditor, Impact Analyzer, Design Critic, and the review-soul domain critic. files_to_review was 22 (git diff origin/main...HEAD --name-only), each reviewer was given the lane worktree as an absolute review root, and every one quoted lines from this diff. Every BLOCKING and HIGH is fixed in 066aba4; the highlights:

• Fail-closed was unreachable where it mattered (BLOCKING, found independently by four reviewers). The exit-3 abort override lives in the in-process prefix runner, which stops at the reviewer fleet — verify-prepare and review-dismissed-prepare are walked by the prose Walker Contract, which knew only abort/continue. Walker Contract step 5 now carries the rule, the per-stage notes carve it out of their documented degradations, the run-prefix error recovery is told not to resume a stage that exited 3, and prefix_golden_harness mirrors the override in both walkers.
• The stop-paths I added were themselves false greens (HIGH). A blocked reviewer was told to write {"findings": []} — byte-identical to a clean review, since the collector reads only findings. A blocked verifier was told to write UNCERTAIN, which is not in VERIFIER_VERDICTS and is discarded by consolidation.
• Three ways a correct run could be wrongly aborted (HIGH/MEDIUM): git C-quoted non-ASCII paths, dangling symlinks, and an ordinary commit during the review.
• Two of my own tests could not fail (MEDIUM, review-soul). See the counterfactual section.

Notes

• A scope.json written by 3.7.0 has an empty review_root and no review_root_sha. A resumed run against one now aborts at the first dispatch stage rather than silently reviewing the wrong tree — deliberate, since that artifact records the defect this change fixes. The walker will not re-run resolve-scope over an existing scope.json, so the error message says to start a fresh review rather than offering an unreachable remedy.
• --review-dismissed on a local PR-worktree review now stops rather than running: stage_30_footer has already torn the PR-head worktree down, so a second opinion cannot be formed against the reviewed source. Previously it ran and rejected every finding on the existence check, which is the same outcome without the explanation. present-local/SKILL.md carries the operator handling.
• The code-review plugin version is bumped and the root CHANGELOG.md carries the v3.8.0 entry, per CONTRIBUTING.md.

[shafty023]

Beginning a code-review. Please allow time for me to finish before merging the PR

[shafty023]

The live-checkout branch only proves that review_root_sha is reachable, not that the files reviewers read still match the resolved diff. A commit, reset, or uncommitted edit after resolve-scope can retain that commit and every changed path while moving the source under review, so the fleet can still publish a clean review for a different snapshot. Keep an immutable PR-head worktree for source-reading dispatches (including the already-at-head case), or reject divergence from the pinned source; add a same-path post-resolution drift case.

[shafty023]

The new exit-3 override is what keeps the golden harness from claiming parity while continuing a wrong-worktree refusal, but the parity suite only exercises successful roots. Add focused cases that force exit 3 on an on_failure: continue stage through both harness walkers and assert abort rather than failed_continue; otherwise either branch can drift or disappear with the suite still green.