fix(review): pin the verified git identity across the residue probe (#9557) (#9742)

* fix(review): pin the verified git identity across the residue probe (#9557)

`worktreeResidue` verified a tree's identity once and then ran five commands
that each re-discovered the repository from `cwd`, through the same `.git`
file the check had read — a file writable by anything running as this user.
Two halves, and they close different windows:

- **The gate now requires the gitfile's admin entry to point BACK at this
  tree.** A repository whose `core.worktree` names this path answers
  `--show-toplevel` with this path, so the old gate saw itself while every
  command after it measured the plant's index — which already holds the
  contamination as committed content, so `status` comes back clean for a tree
  with a mutant in it. A planted standalone repo has no admin entry to
  round-trip; `scratch-tree` gates its own reset on exactly this.
- **The verified `--git-dir`/`--work-tree` are pinned onto every later spawn**,
  so a swap that lands AFTER the gate cannot redirect them either.

The reason recorded against doing this — that an explicit pin changes what
`ls-files` resolves, so it needs its own round — was measured and is wrong.
Across a standalone checkout, a linked worktree, a superproject with an
initialised submodule, and a worktree reached through a symlinked ancestor, all
five commands return **byte-identical** output pinned and unpinned. What was
right about it is that it needed measuring, which is what this is.

The backpointer half is pinned by a test that goes red without it. The pin half
is not: it closes a check-then-use window the suite cannot open deterministically
(the swap has to land between two spawns). Its evidence is a shell repro — with
the identity captured, then the gitfile swapped, discovery reports a clean tree
while the pinned commands still report ` M a.ts` and the untracked probe file.
Saying that rather than implying a pin that is not there.

* fix(review): refuse symlink redirects and forged admin entries in the residue probe (#9742)

* fix(review): fail closed on unanchored clean verdicts and steered boundaries (#9742)

The residue probe's identity gate answered CLEAN for shapes it cannot
vouch for. Four closes, all fail-closed:

- A repository answering for a path it does not contain — the common
  dir no literal ancestor of the tree path — is refused before the
  symlink walk. That is the shape a forge uses to steer the walk's own
  stop boundary so it fires before the planted link is lstat'd, and the
  walk's bound escaping to the filesystem root misdiagnosed refusals
  (and reddened the suite on hosts whose tmpdir resolves through a
  symlink).
- A clean measurement no fetched-head record anchored is refused: a
  forged pair answers clean too, so an unanchored empty status
  certifies whichever index the gitfile names. Dirty reports still
  name paths — a forge answers clean, never dirty.
- The agent-prompt wiring fails closed when the plan's fetchedSha is
  absent or malformed instead of degrading to an unpinned probe: every
  worktree-mode fetch writes the field.
- A dangling admin-entry backpointer is "does not point back", not the
  outer catch's "not a git worktree".

Comments now say what the pin actually closes (post-gate identity
redirect, pre-planted shapes) and what it does not (in-window state
presentation by an active writer — cost-raising, not closure; the
structural close is the sandbox boundary, #9556).

Witnessed: every guard flips its focused tests when removed (mutation
probes), including new ones for the case-fold, the unreadable-HEAD arm,
the sha handover seam, and the sha-less production caller. Fixtures
realpath their roots so the suite holds under any tmpdir spelling.

* fix(review): anchor the scratch-tree residue check and name unmeasured reasons (#9742)

Three closes on the round-4 findings, all inside the residue probe's
own footprint:

- The sha-less production caller flattened the tampering signal the
  probe exists to protect: the no-record refusal fired on 100% of
  clean runs, so a note that fires always is a note nobody reads.
  scratch-tree now takes --fetched-sha, agent-prompt welds the plan's
  record into the verifier's command, and a healthy shared tree
  measures clean again — a forged pair is refused at the pin with a
  distinguishable reason instead.
- The unmeasured renderers blamed `git status` for refusals where it
  never failed (the gates run before it; the no-record tail runs after
  it answered CLEAN), sending triagers to debug a git environment
  with nothing wrong. All three renderings now name a reason.
- The ancestor-symlink walk had no witness: both sibling shapes
  refuse at earlier gates, and deleting the arm shipped green
  (measured). A shape that passes the leaf, the self-equality and the
  boundary gate leaves only the walk to refuse it.

* test(review): pin the fetched-sha CLI contract and the weld continuation (#9742)

* test(review): share the forge-territory fixture across the redirect residue tests (#9742)

* fix(review): admit healthy trees spelled through links above the root (#9742)

* fix(review): canonicalise both spellings the containment gate compares (#9742)

* fix(review): admit healthy layouts and refuse unpinned or drifting trees (#9742)

---------

Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>

Author: 3 people

packages/cli/src/commands/review/agent-prompt.test.ts

@@ -25,6 +25,7 @@ import { execFileSync } from 'node:child_process';
 import {
   mkdirSync,
   mkdtempSync,
+  realpathSync,
   rmSync,
   utimesSync,
   writeFileSync,
@@ -1479,7 +1480,7 @@ describe('--roster — every prompt the plan requires, in one call', () => {
     // is launched, which makes this the one place the pipeline can notice that
     // the tree those agents are about to read is not the commit they think it
     // is. A real git worktree, because `git status` is the oracle.
-    const dir = mkdtempSync(join(tmpdir(), 'ap-residue-'));
+    const dir = realpathSync(mkdtempSync(join(tmpdir(), 'ap-residue-')));
     // Ambient host git config (a global `commit.gpgsign` with no key, a
     // `core.hooksPath` that fails) makes the fixture commit throw and reddens
     // this test for reasons the branch never touched — the incident
@@ -1504,15 +1505,18 @@ describe('--roster — every prompt the plan requires, in one call', () => {
       writeFileSync(join(wt, '__probe__.test.ts'), 'it("x", () => {});');
 
       const plan = join(dir, 'plan.json');
-      writeFileSync(
-        plan,
-        JSON.stringify({
-          ...PLAN,
-          worktreePath: wt,
-          prNumber: '9207',
-          ownerRepo: 'QwenLM/qwen-code',
-        }),
-      );
+      const writePlan = (fields: Record<string, unknown>) =>
+        writeFileSync(
+          plan,
+          JSON.stringify({
+            ...PLAN,
+            worktreePath: wt,
+            prNumber: '9207',
+            ownerRepo: 'QwenLM/qwen-code',
+            ...fields,
+          }),
+        );
+      writePlan({ fetchedSha: git('rev-parse', 'HEAD').trim() });
       (agentPromptCommand.handler as (a: unknown) => void)({
         plan,
         roster: true,
@@ -1533,12 +1537,135 @@ describe('--roster — every prompt the plan requires, in one call', () => {
       expect(readFileSync(briefPath(plan, '1b'), 'utf8')).toContain(
         'And right now it is not clean',
       );
+
+      // The handover is the wiring under test: drop it and the brief degrades
+      // in one of two ways, both refused — a WRONG sha (the forge's own)
+      // reaches the pin and is refused there, a MISSING one fails closed
+      // before the probe runs, because every worktree-mode fetch writes the
+      // field and its absence means the plan was tampered with. Either way
+      // the brief carries the unmeasured sentence, never a clean verdict.
+      const briefOf = (fields: Record<string, unknown>) => {
+        writePlan(fields);
+        (agentPromptCommand.handler as (a: unknown) => void)({
+          plan,
+          roster: true,
+        });
+        return readFileSync(briefPath(plan, '1a'), 'utf8');
+      };
+      const wrongSha = briefOf({ fetchedSha: `deadbeef${'0'.repeat(32)}` });
+      expect(wrongSha).toContain('Whether it is clean could not be measured');
+      expect(wrongSha).toContain('not the fetched PR head');
+      // The framing names a reason, not a failed `git status` — the status
+      // never ran for these refusals, and a triager sent to debug the git
+      // environment would find nothing to fix.
+      expect(wrongSha).toContain('(reason: ');
+      expect(wrongSha).not.toContain('(`git status` failed');
+      const noSha = briefOf({});
+      expect(noSha).toContain('Whether it is clean could not be measured');
+      expect(noSha).toContain('no usable record of the fetched head sha');
+      // The stderr warning the handler prints for the same state carries the
+      // same neutral framing.
+      expect(writeStderrLine).toHaveBeenCalledWith(
+        expect.stringContaining('(reason: '),
+      );
     } finally {
       rmSync(dir, { recursive: true, force: true });
       gitIsolation.dispose();
     }
   });
 
+  // A SHA-256 repository is the shape the record validators must admit:
+  // fetch-pr writes `git rev-parse` verbatim, and in that repository class
+  // the answer is 64 hex. Git grew the format late, so probe for support and
+  // skip where it is absent rather than fail a host that cannot build the
+  // fixture.
+  const gitSha256Supported = (() => {
+    try {
+      const probe = mkdtempSync(join(tmpdir(), 'qwen-sha256-probe-'));
+      try {
+        execFileSync('git', ['init', '-q', '--object-format=sha256', probe], {
+          stdio: 'pipe',
+        });
+        return true;
+      } finally {
+        rmSync(probe, { recursive: true, force: true });
+      }
+    } catch {
+      return false;
+    }
+  })();
+
+  it.skipIf(!gitSha256Supported)(
+    'pins a SHA-256 review worktree with the plan’s 64-hex record',
+    () => {
+      // A validator matching only 40-hex shas drops the record this
+      // repository class writes: every worktree-mode round then fails
+      // closed as though the plan were tampered with, and the verifier's
+      // scratch-tree command is built without `--fetched-sha`. The 64-hex
+      // record must reach BOTH the residue pin and the welded command.
+      const gitIsolation = isolateHostGitConfig();
+      const dir = realpathSync(mkdtempSync(join(tmpdir(), 'ap-sha256-')));
+      try {
+        const git = (...args: string[]) =>
+          execFileSync('git', args, { cwd: dir, encoding: 'utf8' });
+        git('init', '-q', '-b', 'main', '--object-format=sha256');
+        git('config', 'user.email', 't@t.t');
+        git('config', 'user.name', 't');
+        writeFileSync(join(dir, 'a.ts'), 'export const x = 1;\n');
+        git('add', '-A');
+        git('commit', '-qm', 'head');
+        const sha64 = git('rev-parse', 'HEAD').trim();
+        expect(sha64).toMatch(/^[0-9a-f]{64}$/);
+        const wt = join(dir, '.qwen', 'tmp', 'review-pr-sha256');
+        git('worktree', 'add', '--detach', '-q', wt, 'HEAD');
+        const plan = join(dir, 'plan.json');
+        writeFileSync(
+          plan,
+          JSON.stringify({
+            ...PLAN,
+            worktreePath: wt,
+            prNumber: '256',
+            ownerRepo: 'QwenLM/qwen-code',
+            fetchedSha: sha64,
+          }),
+        );
+        (agentPromptCommand.handler as (a: unknown) => void)({
+          plan,
+          roster: true,
+        });
+
+        // The record reached the residue pin: the tree at the recorded sha
+        // measures clean instead of being refused for a missing record.
+        const brief = readFileSync(briefPath(plan, '1a'), 'utf8');
+        expect(brief).not.toContain(
+          'Whether it is clean could not be measured',
+        );
+        expect(brief).not.toContain('no usable record of the fetched head');
+        // And it reached the scratch-tree command welded into a verifier
+        // shard's brief — shards launch through the single-role path with
+        // their record key, exactly as the orchestrator runs them.
+        const findings = join(dir, 'findings.md');
+        writeFileSync(findings, '- **[Critical]** probe');
+        (agentPromptCommand.handler as (a: unknown) => void)({
+          plan,
+          role: 'verify',
+          findings,
+        });
+        const recorded = readRecordedPrompts(plan);
+        const verifyKey = [...recorded.keys()].find((k) =>
+          k.startsWith('verify--'),
+        );
+        expect(verifyKey).toBeDefined();
+        expect(
+          readFileSync(briefPath(plan, verifyKey ?? ''), 'utf8'),
+        ).toContain(`--fetched-sha ${sha64}`);
+      } finally {
+        rmSync(dir, { recursive: true, force: true });
+        gitIsolation.dispose();
+      }
+    },
+  );
+
   it('builds and records the whole 3A roster', () => {
     const dir = mkdtempSync(join(tmpdir(), 'ap-roster-'));
     try {
@@ -2839,6 +2966,42 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
     expect(
       buildRoleBrief(PR_PLAN, 'verify', { key: 'verify; rm -rf /' }),
     ).toContain('--label verify__rm_-rf__');
+    // The plan's fetched sha rides along when the plan carries a usable one:
+    // it is the shared-tree residue check's identity anchor, and without it
+    // the check would refuse every healthy run (#9742). Absent or malformed,
+    // nothing is welded — the record-less refusal is the fail-closed shape.
+    const sha = 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef';
+    // Pin the JOINED fragment, not the bare flag: without the continuation
+    // after `--label` the snippet is two statements — the command runs
+    // unpinned and the sha line dies as command-not-found — while
+    // `toContain('--fetched-sha …')` still passes.
+    expect(
+      buildRoleBrief({ ...PR_PLAN, fetchedSha: sha }, 'verify', {
+        key: 'verify--round-2--deadbeef1234',
+      }),
+    ).toContain(
+      `--label verify--round-2--deadbeef1234 \\
+  --fetched-sha ${sha}`,
+    );
+    // A SHA-256 repository records a 64-hex commit; the pipeline's own
+    // shape contract admits both full object-ID lengths, so that record
+    // welds in too — a validator that only matched 40 hex would leave
+    // every SHA-256 review's command unpinned.
+    const sha256 = 'ab'.repeat(32);
+    expect(
+      buildRoleBrief({ ...PR_PLAN, fetchedSha: sha256 }, 'verify', {
+        key: 'verify--round-2--deadbeef1234',
+      }),
+    ).toContain(`--fetched-sha ${sha256}`);
+    // And the sha-less brief must not carry a continuation after the label
+    // either — a dangling one would glue the closing fence onto the command.
+    expect(p).not.toMatch(/--label verify--round-2--deadbeef1234 \\/);
+    expect(p).not.toContain('--fetched-sha');
+    expect(
+      buildRoleBrief({ ...PR_PLAN, fetchedSha: 'not-a-sha' }, 'verify', {
+        key: 'verify--round-2--deadbeef1234',
+      }),
+    ).not.toContain('--fetched-sha');
     // No worktree, no scratch tree — a local or cross-repo review has no
     // pristine sibling to build, and HEAD is not what is under review there.
     expect(buildRoleBrief(PLAN, 'verify')).not.toContain('review scratch-tree');

packages/cli/src/commands/review/agent-prompt.ts

@@ -99,7 +99,11 @@ import { SHA_RE } from './lib/ledger.js';
 import { pathRulesFor } from './lib/path-rules.js';
 import { shellQuotePath } from './lib/shell-quote.js';
 import { inertPath, scratchLabel } from './lib/paths.js';
-import { worktreeResidue, type WorktreeResidue } from './lib/worktree.js';
+import {
+  RESIDUE_PATH_CAP,
+  worktreeResidue,
+  type WorktreeResidue,
+} from './lib/worktree.js';
 import {
   isTerritoryFanOut,
   requiredAgents,
@@ -151,6 +155,8 @@ interface PlanReport {
   prNumber?: unknown;
   ownerRepo?: unknown;
   worktreePath?: unknown;
+  /** The PR head sha fetch-pr recorded — the probe's identity anchor. */
+  fetchedSha?: unknown;
   mergeBaseSha?: unknown;
   host?: unknown;
   repositoryContext?: unknown;
@@ -1316,6 +1322,27 @@ function repositoryContextBlock(context: RepositoryContext): string[] {
   ];
 }
 
+/**
+ * The plan's fetched head sha when it carries a usable one. Absent or
+ * malformed answers nothing rather than a broken anchor: every worktree-mode
+ * fetch writes the field, so both call sites fail closed on that absence,
+ * each in its own way.
+ *
+ * A usable one is a FULL Git object ID: 40 hex for SHA-1 repositories and
+ * 64 for SHA-256 ones — fetch-pr records `git rev-parse` verbatim, and the
+ * pipeline's own shape contract admits both lengths (pr-context's
+ * COMMIT_SHA_RE carries its {40,64} breadth for exactly that class). A
+ * validator matching only the SHA-1 length would drop the record every
+ * SHA-256 review writes, failing closed as though the plan were tampered
+ * with and welding an unpinned scratch-tree command.
+ */
+function fetchedShaOf(report: PlanReport): string | undefined {
+  const sha = report.fetchedSha;
+  return typeof sha === 'string' && /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(sha)
+    ? sha
+    : undefined;
+}
+
 /**
  * The review worktree's residue, or nothing at all when there is no worktree to
  * have any. Resolved against the process cwd, like every other use of
@@ -1325,7 +1352,28 @@ function repositoryContextBlock(context: RepositoryContext): string[] {
 function worktreeResidueOf(report: PlanReport): WorktreeResidue {
   const wt = report.worktreePath;
   if (typeof wt !== 'string' || !wt) return { paths: [], total: 0 };
-  return worktreeResidue(resolve(wt));
+  // Hand over the sha fetch-pr recorded: committing the contamination moves
+  // a forge's HEAD off it, so with it the probe refuses a forged admin entry
+  // (see worktreeResidue). The record raises the plant's cost; it does not
+  // make planting impossible — it is re-read from the plan file at every
+  // invocation, and a same-user writer can rewrite it along with the forge.
+  // Absent or malformed it fails CLOSED: every worktree-mode fetch writes
+  // the field, so a plan that names a worktree without it is tampered or
+  // corrupted, and measuring unpinned would certify whichever index the
+  // gitfile names.
+  const sha = fetchedShaOf(report);
+  if (sha === undefined) {
+    return {
+      paths: [],
+      total: 0,
+      unmeasured:
+        'the plan carries no usable record of the fetched head sha — ' +
+        'every worktree-mode fetch writes one, so its absence means ' +
+        'tampering or corruption, and measuring without it would certify ' +
+        'whichever index the .git gitfile names',
+    };
+  }
+  return worktreeResidue(resolve(wt), RESIDUE_PATH_CAP, sha);
 }
 
 /**
@@ -1380,7 +1428,7 @@ function worktreeEvidenceBlock(
   if (residue?.unmeasured) {
     parts.push(
       '',
-      `**Whether it is clean could not be measured** (\`git status\` failed: ` +
+      `**Whether it is clean could not be measured** (reason: ` +
         `${inertPath(residue.unmeasured)}). That is not the same as clean: treat ` +
         'anything that surprises you in this tree as unverified until you have ' +
         'checked it against `git show HEAD:<path>`.',
@@ -1649,6 +1697,11 @@ export function buildRoleBrief(
       // written into a shell command, and the one function that decides the
       // tree's name is also what keeps a metacharacter out of that command.
       const label = scratchLabel(opts.key ?? role);
+      // The identity anchor fetch-pr recorded, when the plan carries a usable
+      // one: with it the probe pins the shared tree and a healthy run measures
+      // clean — without it the no-record refusal fires on every run, and a
+      // tampering note that fires always is a note nobody reads.
+      const sha = fetchedShaOf(report);
       parts.push(
         '',
         '**Your scratch tree — where every probe, mutant and candidate fix goes.** ' +
@@ -1667,7 +1720,8 @@ export function buildRoleBrief(
         // a bare interpolation, and the failure would be silent — every shard's
         // scratch tree unavailable, every probe demoted to a reading.
         `"\${QWEN_CODE_CLI:-qwen}" review scratch-tree --worktree ${shellQuotePath(resolve(wt))} \\`,
-        `  --label ${label}`,
+        `  --label ${label}${sha === undefined ? '' : ' \\'}`,
+        ...(sha === undefined ? [] : [`  --fetched-sha ${sha}`]),
         '```',
         '',
         'It reports `path` — work there, and leave what you leave: `cleanup` sweeps ' +
@@ -3212,7 +3266,7 @@ function runAgentPrompt(args: AgentPromptArgs): void {
   const residue = worktreeResidueOf(report);
   if (residue.unmeasured) {
     writeStderrLine(
-      `warning: could not measure whether the review worktree is clean (git status failed: ` +
+      `warning: could not measure whether the review worktree is clean (reason: ` +
         `${inertPath(residue.unmeasured)}). Every brief built by this call says so; an unmeasured tree is ` +
         'not a clean one.',
     );