From 5e471909bebb4a1be4d1819cd97367bbade0ea3a Mon Sep 17 00:00:00 2001 From: Jon Lebron Date: Wed, 7 Oct 2026 22:56:14 -0400 Subject: [PATCH] fix(ce-code-review): bind finish workers to the reviewed checkout --- .../ce-code-review/references/finish-input.md | 43 +++++++++++++-- .../references/modes-and-output.md | 4 +- skills/ce-code-review/references/scope.md | 5 ++ .../references/validator-batch-template.md | 10 +++- tests/ce-code-review-mechanics.test.ts | 52 ++++++++++++++++++- tests/review-skill-contract.test.ts | 33 +++++++++++- 6 files changed, 139 insertions(+), 8 deletions(-) diff --git a/skills/ce-code-review/references/finish-input.md b/skills/ce-code-review/references/finish-input.md index d389ac0d00..186348d98b 100644 --- a/skills/ce-code-review/references/finish-input.md +++ b/skills/ce-code-review/references/finish-input.md @@ -19,6 +19,7 @@ The dispatch context writes this file after every local reviewer is collected, a "run_id": "", "run_dir": "", "skill_dir": "", + "project_root": "", "docs_root": ">", "mode": { "agent": false, "apply_local": false, "grouping": "auto", "depth": "auto" }, "invocation": { "arguments": "", "constraints": [""] }, @@ -27,11 +28,12 @@ The dispatch context writes this file after every local reviewer is collected, a "base": "", "diff_a": "", "diff_b": "", "pr": { "number": null, "url": null, "title": null, "body": null, "base_ref_name": null, "head_ref_oid": null, "head_ref": null, "base_ref": null, "has_prior_comments": false }, - "branch": "", - "head_sha": "", + "branch": "", + "head_sha": "", "files": "/files.txt", "diff": "/full.diff", "tree_is_reviewed_head": true, + "local_state": { "index_sha256": "", "worktree_diff_sha256": "" }, "untracked_excluded": [] }, "intent": { "summary": "", "confidence": "explicit | inferred | uncertain" }, @@ -63,6 +65,41 @@ Every path in this file exists before a leaf is launched: the dispatch context w `plan.requirements` and `plan.implementation_units` are the Stage 2 extraction (`references/intent-and-plan.md`), carried so Stage 6's Requirements Completeness checklist is built from what dispatch extracted rather than re-derived. `invocation.constraints` carries the user's own words for any limit on what may be changed or reported ("only change tests", "do not touch the schema"); the report leaf honors every one of them during Stage 5c and Stage 6, and a constraint it cannot honor makes the affected fix unapplied and reported, never silently applied. +## Bind the checkout before inspection + +Capture `project_root` from the checkout that supplied the scoped diff, not from +the dispatcher's inherited working directory or the run directory. Resolve it to +the absolute Git toplevel and carry it verbatim through both leaf prompts, +`validator-input.json.project_root`, and the validator's scope context. It names +the Git context; it does not grant mutation authority or make a remote reviewed +ref identical to the local working tree. + +Capture `scope.branch` and `scope.head_sha` in that root with the scoped diff too. + +For local-aligned and standalone scope, capture `scope.local_state` in that root +using the exact commands named in its fields, hashing raw stdout bytes, not text +with stripped newlines. Capture it with the scoped diff while the checkout is +stable. It binds the index separately from tracked working-tree changes, since +an unstaged edit can cancel a staged change. Untracked files remain excluded as +the scope contract states; do not read them as reviewed source. For pr-remote and +branch-remote scope, set `scope.local_state` to null and retain the existing +reviewed-ref inspection rules. + +Before source inspection, each leaf and validator must use an explicit tool +working directory of `project_root` (or `git -C` for Git), resolve the Git +toplevel, and verify it against the captured root. For local scope, also verify +`scope.head_sha` and recapture both `scope.local_state` digests. A matching branch +or HEAD alone is insufficient: two checkouts can share both but contain different +staged changes. Missing root/state, inaccessible checkout, or a mismatch is a +failed finish or unresolved validation with the reason recorded; never choose +the inherited cwd, switch checkouts, reconstruct missing values, or silently +refresh the snapshot. Dispatch must re-scope changed input. Recheck once before +entering an authorized Stage 5c apply batch; record that batch's own edits +separately instead of treating them as unexplained pre-review drift. These +checks bind trusted cooperative local work; +they are not a security sandbox or protection against concurrent filesystem +changes between verification and use. + ## What a leaf reads Each leaf reads, from `skill_dir`, `references/finish-review.md` and every reference that file names for the stages the leaf owns: `references/action-class-rubric.md` for routing, `references/diff-scope.md` for how to inspect source, `references/intent-and-plan.md` for the plan rules, `references/review-output-template.md` for the report skeleton, `references/validator-batch-template.md` only in the dispatch context. When `scope.tree_is_reviewed_head` is false, the working tree is not the reviewed head: a leaf never reads a changed path from the workspace and inspects the reviewed head (`scope.diff_b`) the way `diff-scope.md` directs reviewers to, including its search across unchanged files; that reference owns the mechanism. @@ -113,4 +150,4 @@ Always write run artifacts under the resolved ``: - `adversarial-review-brief.md` when the cross-model route starts: the orchestrator's compact semantic divisions, never a copied diff - `report.md`: the rendered markdown report exactly as presented to the user (default mode only), so format and numbering stay auditable after the run -`metadata.json` carries the minimum fields defined under ## Run artifacts in `references/modes-and-output.md`; capture `branch` and `head_sha` at dispatch time (no in-skill fixes will land afterward). +`metadata.json` carries the minimum fields defined under ## Run artifacts in `references/modes-and-output.md`; carry `branch` and `head_sha` from the original scope capture, before any authorized in-skill fixes. diff --git a/skills/ce-code-review/references/modes-and-output.md b/skills/ce-code-review/references/modes-and-output.md index 5c81f05e65..36d0c2eb7d 100644 --- a/skills/ce-code-review/references/modes-and-output.md +++ b/skills/ce-code-review/references/modes-and-output.md @@ -80,8 +80,8 @@ Every run, lite, focused, or full, leaves its receipt (`review.json` in `mode:ag ```json { "run_id": "", - "branch": "", - "head_sha": "", + "branch": "", + "head_sha": "", "verdict": "", "completed_at": "" } diff --git a/skills/ce-code-review/references/scope.md b/skills/ce-code-review/references/scope.md index 6565879c6d..9761c82c94 100644 --- a/skills/ce-code-review/references/scope.md +++ b/skills/ce-code-review/references/scope.md @@ -6,6 +6,11 @@ Read this at Stage 1. It defines how to resolve scope on every invocation path a Compute the diff range, file list, and diff. Minimize permission prompts by combining into as few commands as possible. +Before collecting that diff, read "Bind the checkout before inspection" in +`references/finish-input.md`. Capture the checkout binding with the scoped diff +and retain it through dispatch; the finish handoff carries that original binding, +not a new snapshot of the checkout after the reviewers finish. + **If `base:` argument is provided (fast path):** The caller already knows the diff base. Skip all base-branch detection, remote resolution, and merge-base computation. Use the provided value directly: diff --git a/skills/ce-code-review/references/validator-batch-template.md b/skills/ce-code-review/references/validator-batch-template.md index 1670030acb..7c454a7e71 100644 --- a/skills/ce-code-review/references/validator-batch-template.md +++ b/skills/ce-code-review/references/validator-batch-template.md @@ -1,6 +1,6 @@ # Validator Batch Prompt Template -Use one fresh validator subagent for one batch of already-merged findings. Eight findings is the normal cap. When more than eight P0/P1 findings survive, expand that same batch to include every surviving P0/P1; never omit a blocker or split the work into another batch. The validator is independent of the originating reviewers and the orchestrator. Fill `{run_dir}` with the review's run directory; the orchestrator waits on the verdicts file there, so the file, not the in-band return, is the contract. +Use one fresh validator subagent for one batch of already-merged findings. Eight findings is the normal cap. When more than eight P0/P1 findings survive, expand that same batch to include every surviving P0/P1; never omit a blocker or split the work into another batch. The validator is independent of the originating reviewers and the orchestrator. Fill `{run_dir}` with the review's run directory; the orchestrator waits on the verdicts file there, so the file, not the in-band return, is the contract. Fill `{finish_input_reference}` with the absolute path to `references/finish-input.md` under the handoff's `skill_dir`. The scope context carries `project_root` and the captured `scope` verbatim. ``` You are the independent validation gate for the code-review findings below. Evaluate every finding separately under fresh inspection and under the shared protected-subject policy below. Outside a protected subject, false positives are common; reject a finding when the cited code does not prove it, it predates and is unaffected by this diff, surrounding code handles it, or it is only an unsupported preference. Inside a protected subject, a rejection requires one of the evidence forms the policy names. @@ -19,6 +19,14 @@ Do not let one finding's outcome influence another. Do not invent new findings. {scope_mode_and_remote_refs} +Use the canonical project_root and frozen scope passed in scope-context. Set every +source/Git tool call's working directory explicitly to project_root (or use git +-C); never infer it from your inherited cwd. Before inspection, read +`{finish_input_reference}` and apply its "Bind the checkout before inspection" +checks. A missing or mismatched local binding leaves affected findings unresolved, +not rejected or confirmed. In remote scope, use that Git context only to inspect +the captured reviewed refs, never its unrelated workspace files. + Return status "confirmed", "rejected", or "unresolved". Never use lack of disproof as evidence of confirmation. diff --git a/tests/ce-code-review-mechanics.test.ts b/tests/ce-code-review-mechanics.test.ts index 68a5e6ac34..3d6a56092d 100644 --- a/tests/ce-code-review-mechanics.test.ts +++ b/tests/ce-code-review-mechanics.test.ts @@ -1,4 +1,5 @@ -import { mkdtempSync, mkdirSync, writeFileSync, chmodSync } from "fs" +import { mkdtempSync, mkdirSync, writeFileSync, chmodSync, rmSync } from "fs" +import { createHash } from "node:crypto" import { tmpdir } from "os" import path from "path" import { spawnSync } from "node:child_process" @@ -34,6 +35,55 @@ function fixtureRepo() { } describe("ce-code-review deterministic mechanics", () => { + test("checkout binding distinguishes same-HEAD clones and index changes hidden by the worktree", () => { + const dir = mkdtempSync(path.join(tmpdir(), "ce-checkout-binding-")) + // This is a witness for the documented fingerprint, not proof that a model + // obeys the handoff. A fresh-agent evaluation covers that separate boundary. + const rawGit = (cwd: string, ...args: string[]) => { + const result = spawnSync("git", ["-c", "core.hooksPath=/dev/null", "-c", "commit.gpgsign=false", ...args], { + cwd, + env: { PATH: process.env.PATH, GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null" }, + }) + expect(result.status).toBe(0) + return result.stdout + } + const fingerprint = (cwd: string) => ({ + index_sha256: createHash("sha256").update(rawGit(cwd, "ls-files", "--stage", "-z")).digest("hex"), + worktree_diff_sha256: createHash("sha256").update(rawGit(cwd, "diff", "--binary", "--full-index", "--no-ext-diff", "--no-textconv", "HEAD", "--")).digest("hex"), + }) + try { + const original = path.join(dir, "original") + const reviewed = path.join(dir, "reviewed") + mkdirSync(original) + rawGit(original, "init", "-q", "-b", "main") + rawGit(original, "config", "user.name", "Review Fixture") + rawGit(original, "config", "user.email", "review@example.invalid") + writeFileSync(path.join(original, "sample.txt"), "shared committed text\n") + rawGit(original, "add", "--", "sample.txt") + rawGit(original, "commit", "-qm", "base") + rawGit(dir, "clone", "-q", "--no-hardlinks", original, reviewed) + writeFileSync(path.join(original, "sample.txt"), "ambient staged text\n") + rawGit(original, "add", "--", "sample.txt") + writeFileSync(path.join(reviewed, "sample.txt"), "reviewed staged text\n") + rawGit(reviewed, "add", "--", "sample.txt") + + expect(rawGit(original, "rev-parse", "HEAD")).toEqual(rawGit(reviewed, "rev-parse", "HEAD")) + expect(rawGit(original, "branch", "--show-current")).toEqual(rawGit(reviewed, "branch", "--show-current")) + expect(fingerprint(original)).not.toEqual(fingerprint(reviewed)) + expect(rawGit(reviewed, "show", ":sample.txt").toString()).toBe("reviewed staged text\n") + + const frozen = fingerprint(reviewed) + writeFileSync(path.join(reviewed, "sample.txt"), "later staged text\n") + rawGit(reviewed, "add", "--", "sample.txt") + writeFileSync(path.join(reviewed, "sample.txt"), "reviewed staged text\n") + const changed = fingerprint(reviewed) + expect(changed.worktree_diff_sha256).toBe(frozen.worktree_diff_sha256) + expect(changed.index_sha256).not.toBe(frozen.index_sha256) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + test("scope helper counts structured text toward changed_lines and does not hard-block on markdown", () => { const { dir, base } = fixtureRepo() mkdirSync(path.join(dir, "docs")) diff --git a/tests/review-skill-contract.test.ts b/tests/review-skill-contract.test.ts index b393282bf0..843bbd68f2 100644 --- a/tests/review-skill-contract.test.ts +++ b/tests/review-skill-contract.test.ts @@ -662,7 +662,7 @@ describe("ce-code-review contract", () => { // The run-artifact list moved to finish-input.md (plan 2026-09-15-1322, U3). expect(handoff).toMatch(/- `finish-input\.json`/) // The contract file names every field the finish context may need and the failure direction. - for (const field of ["run_id", "skill_dir", "docs_root", "apply_local", "raw-returns.json", "failed_reviewers", "preference_source", "coverage_notes"]) { + for (const field of ["run_id", "skill_dir", "project_root", "docs_root", "apply_local", "raw-returns.json", "failed_reviewers", "preference_source", "coverage_notes"]) { expect(handoff).toContain(field) } expect(handoff).toMatch(/emit the report leaf's return verbatim/i) @@ -703,6 +703,37 @@ describe("ce-code-review contract", () => { expect(handoff).not.toMatch(/drop and validation-degraded rules/) }) + test("finish handoff binds fresh consumers to a checkout and frozen local state", async () => { + const handoff = await readRepoFile("skills/ce-code-review/references/finish-input.md") + const scope = await readRepoFile("skills/ce-code-review/references/scope.md") + const validator = await readRepoFile("skills/ce-code-review/references/validator-batch-template.md") + const example = handoff.match(/```json\n([\s\S]*?)\n```/) + expect(example).not.toBeNull() + const packet = JSON.parse(example![1]!) + + // Same branch and HEAD can name different staged trees. The JSON carrier, + // not neighboring prose mentioning a root, must supply the binding. + expect(packet.project_root).toEqual(expect.any(String)) + expect(Object.keys(packet.scope.local_state).sort()).toEqual([ + "index_sha256", "worktree_diff_sha256", + ]) + expect(packet.scope.local_state.index_sha256).toContain("git ls-files --stage -z") + expect(packet.scope.local_state.worktree_diff_sha256).toContain( + "git diff --binary --full-index --no-ext-diff --no-textconv HEAD --", + ) + expect(handoff).toContain("validator-input.json.project_root") + // The snapshot is needed before the review starts, not when finish first + // consumes it. Pin the required read ahead of the first diff-producing path. + const capture = scope.indexOf('Before collecting that diff, read "Bind the checkout before inspection"') + expect(capture).toBeGreaterThanOrEqual(0) + expect(capture).toBeLessThan(scope.indexOf("**If `base:` argument")) + expect(scope.slice(capture, scope.indexOf("**If `base:` argument"))).toContain("references/finish-input.md") + expect(handoff).toMatch(/scope\.local_state` to null/) + expect(validator).toContain("{finish_input_reference}") + expect(validator).toMatch(/absolute path to `references\/finish-input\.md`/) + expect(validator).toMatch(/scope context carries `project_root` and the captured `scope` verbatim/) + }) + test("Stage 5 synthesis uses anchor gate and one-anchor promotion", async () => { const content = await readRepoFile( "skills/ce-code-review/references/finish-review.md",