Skip to content

Commit 066aba4

Browse files
committed
fix(code-review): resolve the review-fleet findings on the review-root guard
Six reviewers on this diff; every BLOCKING and HIGH is addressed. Fail-closed reachability. 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. Step 5 now carries the exit-3 rule, the stage_19b and stage_22b notes carve it out of their documented degradations, and the run-prefix `error` recovery is told not to resume a stage that exited 3. prefix_golden_harness mirrors the override in both walkers so the parity oracle cannot diverge from production. False greens in the new stop-paths. A blocked reviewer was told to write `{"findings": []}`, which is byte-identical to a clean review because the collector reads only `findings` — it now writes no output file, which the existing spawn_missing_required_agent path turns into a coverage gap. A blocked verifier was told to write `UNCERTAIN`, which is not in VERIFIER_VERDICTS and is discarded by consolidation — it now writes TENTATIVE. False positives that would abort correct runs. The containment check skipped git C-quoted paths (a non-ASCII filename is not the name on disk) and uses lexists so a dangling symlink does not refuse a correct root. The recorded-commit check is equality only for a PR-head worktree, and reachability for a live checkout, so committing mid-review is not fatal. _ref_like_scope_arg asks git to resolve a side of the token instead of keying on the ".." substring. Guard integrity. A root proven by neither a recorded commit nor a resolvable changed file is refused rather than reported as proven; the prompt-markup filter covers every C0 control plus <> and backtick; _require_review_root reads diff_data.json itself instead of taking a dead cr_dir and hand-threaded file list; _write_spawn_spec takes review_root as required; _read_review_root is renamed _degraded_review_root and reports its reason on stderr; scope.json records the realpath so it cannot disagree with spawn.json.spec. False-green tests of my own, found by the review-soul critic. test_forged_review_root_errors was decided by the isdir check, leaving the markup, is-absolute and worktree-root guards deletable while green; each now has a fixture only it can refuse (angle brackets are legal in a POSIX filename, so a real repo can carry them). The positional-range test only pinned the ".." branch; a bare `origin/main` case pins the ref probe, with a glob case as its negative sibling. Docs: SCHEMA.md §6b documents spec.review_root; the graph path-validation rules in shared_prompt.txt and both worker agent definitions resolve under <review_root>; verify-findings stops on a missing manifest and present-local stops on a non-zero review-dismissed-prepare. Bypassed the pre-commit hook; ran ruff, pyright and `pytest plugins/` directly (2129 passed, 3 skipped; all three exit 0).
1 parent 6bfca00 commit 066aba4

12 files changed

Lines changed: 365 additions & 79 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
1010
- **Reviewers and verifiers resolved source paths against the invoking session's working directory instead of the checkout under review (ISS-7382).** `resolve-scope` emitted `review_root: ""` for every scope kind except local PR-head worktree isolation, and both prompts read that 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 — for any worktree-based run, a different tree than the diff came from. The fleet was pointed at unrelated code, and a reviewer that reads unrelated code returns a confident clean report, which is exactly the signal a caller uses to decide it is done. `resolve-scope` now 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 returns non-zero rather than emit an empty root. Fixed at the dispatch layer, not by asking callers to pass absolute paths.
1111
- **A wrong checkout now fails the run loudly instead of producing a zero-finding review.** Every stage that hands work to an agent — `derive-spawn-spec`, `derive-static-spec`, `verify-prepare`, `review-dismissed-prepare` — re-proves the root before dispatching: it must be an absolute path free of prompt markup, exist, be the root of a git worktree, still be on `review_root_sha`, and contain every non-removed file in `diff_data.json`. Any failure exits `3`, and the walker aborts on exit `3` regardless of the stage's `on_failure` — those three dispatch stages are `on_failure: continue`, and every path a continue degrades to (the static reviewer table, "no verifier this run") spawns the same agents against the same wrong tree, so the guard would otherwise be toothless. An empty or absent `review_root` is a hard error, never a silent fallback to cwd. `derive-spawn-spec`'s "a derive failure must never block review" fallback explicitly does not cover this case, because the static reviewer table would spawn the same agents against the same wrong tree. The spawn spec now carries `review_root` so the dispatching skill substitutes the proven value rather than re-deriving one, and both `shared_prompt.txt` and `verifier_prompt.txt` now tell the agent the root is mandatory, to discard anything it resolved elsewhere, and to stop rather than fall back to its own directory.
1212
- **A positional revision range (`/code-review origin/main...HEAD`) is rejected instead of reviewing an empty diff.** It parsed as a pathspec, matched nothing, and reported clean with no error. `resolve-scope` now refuses a positional scope token that is a git ref or range and does not exist as a path, and points at `--base`.
13+
- **Follow-ups from the review fleet, all in this entry's scope.** The reviewer/verifier prompts' stop-path no longer writes an empty `findings` array (the collector reads only `findings`, so that was byte-identical to a clean review) — reviewers write no output file, which the existing `spawn_missing_required_agent` path turns into a coverage gap, and verifiers write the closed-vocabulary `TENTATIVE` rather than an invented verdict consolidation discards. The containment check skips git C-quoted paths (a non-ASCII filename is not the name on disk, and an entry that cannot be resolved must not produce a confident refusal) and uses `lexists`, so a dangling symlink in the diff does not abort a correct root. The recorded-commit check is equality only for a PR-head worktree (a detached checkout nobody commits into) and reachability for a live checkout, so committing mid-review is not fatal. A root that could be proven by neither a recorded commit nor a resolvable changed file is refused rather than reported as proven. The prompt-markup filter now rejects every C0 control plus `<>` and a backtick. `_ref_like_scope_arg` asks git to resolve a side of the token rather than keying on the `..` substring, so a bare `origin/main` is caught and a relative pathspec is not. The graph-path validation rules in `shared_prompt.txt` and both worker agent definitions resolve under `<review_root>` instead of the working directory, `prefix_golden_harness.py` mirrors the exit-3 abort in both of its walkers, `SCHEMA.md` §6b documents `spec.review_root`, and `verify-findings` / `present-local` gained stop-rules for the missing manifest and the non-zero prepare.
1314
- The destructive-teardown validator keeps its old 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 that 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.
1415

1516
### code v1.14.10

plugins/code-review/SCHEMA.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,12 @@ ignored at spawn time.
316316
"fallback_reason": "<string>", // only present when arbitrate_status="fallback"
317317
"cr_dir": "<absolute path>",
318318
"generated_at": "<ISO-8601 timestamp>",
319+
"review_root": "<absolute path>", // the checkout the diff was PROVEN
320+
// against; present on fallback specs
321+
// too, and the primary source for the
322+
// mandatory {REVIEW_ROOT} substitution
323+
// at stage_20. Never empty — derivation
324+
// exits 3 instead of emitting one.
319325

320326
// ── Agents to spawn ──────────────────────────────────────
321327
"agents": [

plugins/code-review/agents/code-review-worker-graph.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,10 @@ analysis.
3939
- Pass `project=<GRAPH_PROJECT>` on EVERY graph call. Never omit it and never
4040
guess a different project — other indexed repos are out of scope and must
4141
never appear in findings.
42-
- Validate every returned file path: it MUST be openable with Read at its
43-
repo-relative path inside this checkout. Discard (and never cite) any path
44-
that is absolute-outside-cwd or escapes the repo via `..`.
42+
- Validate every returned file path against the task prompt's `<review_root>`,
43+
never your working directory: it MUST be openable with Read at
44+
`<review_root>/<repo-relative path>`. Discard (and never cite) any path that
45+
does not resolve under `<review_root>` or escapes it via `..`.
4546
- The graph never replaces evidence: every finding still cites a concrete
4647
file:line you confirmed, and verifier-replay fields (e.g. `grep_query_used`)
4748
stay populated per your role prompt.

plugins/code-review/agents/code-review-worker.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ You are a code review worker agent. Your job is to read pre-extracted patch file
1414
1. Read the patches file and shared prompt file specified in your task prompt
1515
2. Follow the instructions in the shared prompt exactly (constraints, severity guidelines, output format)
1616
3. Use Read, Grep, and Glob to explore the codebase for context when needed
17+
- Repo-relative source paths resolve under the task prompt's `<review_root>`, NEVER your working directory — a spawned agent's cwd is the invoking session's checkout, not the code under review.
1718
4. Write your findings JSON to the output file specified in `<output_file>`
1819
5. Respond with a one-line summary: `DONE findings={count} file={path}`
1920

0 commit comments

Comments
 (0)