fix(review): keep host-trusted state out of the container's writable surface

.github/workflows/qwen-code-pr-review.yml

@@ -2276,7 +2276,14 @@ jobs:
                 "${GIT_SAFE[@]}" branch -D "$review_ref" ||
                   echo "::warning::could not remove review branch: $review_ref"
               done || true
-          rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true
+          # Both locations: leases moved to `.qwen/review-leases` (out of the
+          # directory the review sandbox mounts read-write), and a runner whose
+          # workspace persists can still be holding one written by an older
+          # build in the old place. `-r`, because the old path is inside that
+          # mounted directory and a reviewed PR can leave a DIRECTORY at the
+          # lease's name, which plain `rm -f` cannot remove.
+          rm -rf .qwen/review-leases/qwen-review-lease-pr-*.json 2>/dev/null || true
+          rm -rf .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true
           echo "review worktrees cleaned"
 
   # A review job that dies abnormally — runner crash, host loss, or the

packages/cli/src/commands/review/base-tree.test.ts

@@ -29,6 +29,8 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { runBaseTree, type BaseTreeReport } from './base-tree.js';
 import { baseWorktreePath } from './lib/paths.js';
+import { adminEntryOf, plantAdminEntry } from './lib/test-utils.js';
+import { runEpochMs } from './lib/prompt-record.js';
 import type { BuildTestReport } from './build-test.js';
 
 const okBuild = {
@@ -43,6 +45,13 @@ const failedBuild = {
   build: [{ command: 'npm run build', exitCode: 2 }],
 } as unknown as BuildTestReport;
 
+// Skipped on win32 for the same reason as the sibling suites: `mountRootFor`
+// refuses every absolute Windows path (a drive letter is a colon), so
+// containment is unavailable there by design and this gate never speaks. The
+// assertion would fail for that reason and nothing else — first inside the
+// merge queue, where that lane actually runs.
+const itWhereContainmentExists = it.skipIf(process.platform === 'win32');
+
 describe('runBaseTree', () => {
   let repo: string;
   let worktree: string;
@@ -61,13 +70,19 @@ describe('runBaseTree', () => {
     return p;
   };
 
+  // Captured ONCE per test, the way `fetch-pr` captures it once per run: the
+  // reuse marker is fenced on the plan's epoch, so a helper that re-captured on
+  // every call would simulate a new run each time and the fast path — the
+  // concurrent-shard guard the reuse test below pins — could never speak.
+  let planPath = '';
   const run = (
     over: { plan?: Record<string, unknown>; worktree?: string } = {},
     build: (w: string) => BuildTestReport = () => okBuild,
   ): BaseTreeReport => {
     const { plan: planOver, ...rest } = over;
+    if (planOver !== undefined || !planPath) planPath = writePlan(planOver);
     return runBaseTree({
-      plan: writePlan(planOver),
+      plan: planPath,
       worktree,
       timeout: 60,
       install: false,
@@ -77,6 +92,7 @@ describe('runBaseTree', () => {
   };
 
   beforeEach(() => {
+    planPath = '';
     repo = mkdtempSync(join(tmpdir(), 'qwen-base-tree-'));
     git(repo, 'init', '-q', '-b', 'main');
     git(repo, 'config', 'user.email', 't@t.t');
@@ -96,6 +112,105 @@ describe('runBaseTree', () => {
 
   afterEach(() => rmSync(repo, { recursive: true, force: true }));
 
+  itWhereContainmentExists(
+    'does not REUSE a base tree whose tracked files were rewritten in the mount',
+    () => {
+      // `rev-parse HEAD` does not move when working files change, and this tree
+      // is a direct child of the directory the sandbox mounts read-write — so
+      // the reviewed PR's own build, which runs before the verifier shards get
+      // here, can overwrite the base checkout's tracked sources with a plain
+      // copy while every pointer and ref condition still passes. Reused, the
+      // A/B compares the PR against a copy of itself: a test the PR breaks
+      // fails identically on both sides, `test-delta` files the regression as
+      // pre-existing, and a real finding against the PR is suppressed.
+      const tree = baseWorktreePath(worktree);
+      const firstBuilds: string[] = [];
+      expect(
+        run({}, (w) => {
+          firstBuilds.push(w);
+          return okBuild;
+        }).available,
+      ).toBe(true);
+      writeFileSync(join(tree, 'a.txt'), 'after\n');
+
+      // Untracked output is what the pipeline's own build leaves here, and it
+      // must not disable reuse: that reintroduces the concurrent-shard clobber
+      // the fast path exists to prevent.
+      mkdirSync(join(tree, 'dist'), { recursive: true });
+      writeFileSync(join(tree, 'dist', 'cli.js'), 'built');
+
+      const rebuilds: string[] = [];
+      const second = run({}, (w) => {
+        rebuilds.push(w);
+        return okBuild;
+      });
+      expect(rebuilds).toEqual([tree]);
+      expect(second.note).not.toContain('reusing it');
+    },
+  );
+
+  itWhereContainmentExists(
+    'does not REUSE a base tree an EARLIER RUN built, whose untracked plants the dirt check cannot see',
+    () => {
+      // `cleanStale` releases the review worktree and its branch but never
+      // `-base`, so this tree stands into the next round with a whole
+      // containerized build/test phase in between — and inside the mount the
+      // reviewed code writes where it likes. What it can drop there is
+      // untracked executable content, `dist/cli.js` and `node_modules/.bin/`
+      // being exactly what a host-side A/B measurement runs, and
+      // `--untracked-files=no` cannot see it: a blanket untracked refusal
+      // would disable every correctly-built tree's reuse and bring back the
+      // concurrent-shard clobber the fast path exists to prevent. So the
+      // marker carries the run that built it, and a stamp from another run is
+      // not a tree this run may certify.
+      const tree = baseWorktreePath(worktree);
+      const builds: string[] = [];
+      const build = (w: string) => {
+        builds.push(w);
+        return okBuild;
+      };
+      expect(run({}, build).available).toBe(true);
+      mkdirSync(join(tree, 'dist'), { recursive: true });
+      writeFileSync(
+        join(tree, 'dist', 'cli.js'),
+        'planted by the reviewed build',
+      );
+
+      // The next run captures its own plan, and the plan's mtime IS the epoch.
+      const later = new Date(Date.now() + 60_000);
+      utimesSync(planPath, later, later);
+
+      const second = run({}, build);
+      expect(second.note).not.toContain('reusing it');
+      expect(builds).toEqual([tree, tree]);
+      // The plant went with the tree it was standing in.
+      expect(existsSync(join(tree, 'dist', 'cli.js'))).toBe(false);
+    },
+  );
+
+  itWhereContainmentExists(
+    'refuses to build through a rewritten review-worktree gitfile',
+    () => {
+      // `worktree add` resolves the repository through the REVIEW worktree's own
+      // gitfile, which lives in the directory the sandbox mounts read-write and
+      // which the build/test phase already ran the PR's code against. It checks
+      // files out, so it runs whatever that pointer leads to, on the host.
+      plantAdminEntry(
+        join(repo, '.qwen', 'tmp', '.evil-git'),
+        adminEntryOf(worktree),
+        worktree,
+        join(repo, '.git'),
+      );
+
+      const r = run();
+      expect(r.available).toBe(false);
+      expect(JSON.stringify(r)).toContain('review temp dir');
+      // The tree was never created, which is what says the spawn never ran —
+      // the note alone reads the same whichever side of it the gate fires on.
+      expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+    },
+  );
+
   it('creates a sibling worktree holding the BASE commit, not the head', () => {
     const r = run();
     expect(r.available).toBe(true);
@@ -140,8 +255,13 @@ describe('runBaseTree', () => {
     expect(second.path).toBe(first.path);
     expect(second.note).toContain('reusing');
     expect(builds).toHaveLength(1); // one install+build, not two
-    // A marker for a DIFFERENT sha (rebase between runs) does not shortcut.
-    writeFileSync(join(first.path!, '.qwen-review-base-ok'), 'f'.repeat(40));
+    // A marker for a DIFFERENT sha (rebase between runs) does not shortcut —
+    // stamped with this run's epoch, so the sha arm is what answers and not the
+    // epoch fence standing in front of it.
+    writeFileSync(
+      join(first.path!, '.qwen-review-base-ok'),
+      `${'f'.repeat(40)}\n${runEpochMs(planPath)}\n`,
+    );
     expect(run({}, build).note).not.toContain('reusing');
   });
 

packages/cli/src/commands/review/base-tree.ts

@@ -55,12 +55,14 @@ import { dirname, join, resolve } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import { baseWorktreePath } from './lib/paths.js';
 import {
+  untrustedGitfile,
   discardWorktree,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
   type SweepResult,
 } from './lib/worktree.js';
 import { runBuildTest, type BuildTestReport } from './build-test.js';
+import { runEpochMs } from './lib/prompt-record.js';
 
 export interface BaseTreeReport {
   /**
@@ -175,11 +177,55 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
   // rebase between runs — falls through to the rebuild below.)
   const marker = () => join(tree, '.qwen-review-base-ok');
   const failedMarker = () => join(tree, '.qwen-review-base-failed');
+  // The run's epoch, the fence the deadline stamps and the session ledger
+  // already key on. A run captures its plan once, so this is stable for the
+  // whole run and different between two.
+  const runEpoch = String(runEpochMs(args.plan));
   try {
+    const stamp = readFileSync(marker(), 'utf8').trim().split('\n');
     if (
       existsSync(tree) &&
-      readFileSync(marker(), 'utf8').trim() === baseSha &&
-      gitOut(tree, 'rev-parse', 'HEAD') === baseSha
+      stamp[0] === baseSha &&
+      // ...and THIS run built it. `cleanStale` never releases `-base`, so an
+      // earlier round's tree stands into this one with a whole containerized
+      // build/test phase in between — long enough for the reviewed code to drop
+      // untracked executables in here, and `dist/cli.js` is what a host-side A/B
+      // runs. `--untracked-files=no` below cannot see them, and refusing any
+      // untracked file at all would disable every legitimate reuse; the stamp is
+      // the provenance that separates the two.
+      stamp[1] === runEpoch &&
+      // The reuse path RETURNS, so the gate below the rebuild never runs for
+      // it — and both facts it reuses on come from inside the mount: the
+      // marker is a file in the base tree, and `rev-parse HEAD` resolves
+      // through that tree's own `.git`. A planted repository answers the right
+      // sha for a working tree holding anything at all, and the A/B's BASE
+      // side is then the reviewed code's own — every "the base behaves
+      // differently" verdict the shard reports would be its author's.
+      //
+      // Not a refusal: an unusable leftover is what the rebuild exists for.
+      // Falling through discards the tree (removing the plant with it) and
+      // creates a fresh one through the review worktree's pointer, which the
+      // gate before `worktree add` checks. Same shape as `scratch-tree`'s
+      // reuse path, for the same reason.
+      untrustedGitfile(tree) === null &&
+      gitOut(tree, 'rev-parse', 'HEAD') === baseSha &&
+      // ...and its CONTENTS must still be the base's. `rev-parse HEAD` does not
+      // move when working files change, and this tree is a direct child of the
+      // directory the sandbox mounts read-write — which is why the pointer gate
+      // above speaks at all. So the reviewed PR's own build, which runs before
+      // the verifier shards get here, can overwrite the base checkout's tracked
+      // sources with a plain copy: every condition above still passes, and the
+      // A/B then compares the PR against a copy of itself. A test the PR breaks
+      // fails identically on both sides, `test-delta` classifies the regression
+      // as pre-existing, and a real finding against the PR is suppressed.
+      //
+      // `--untracked-files=no`: the pipeline's own build leaves `node_modules/`
+      // and `dist/` here, so an untracked-inclusive check would call every
+      // correctly-built tree dirty and disable reuse outright — reintroducing
+      // the concurrent-shard clobber this fast path exists to prevent. Tracked
+      // dirt is what a rewrite leaves, and `npm run build` in this repository
+      // modifies no tracked file, so a legitimate build still reuses.
+      gitOut(tree, 'status', '--porcelain', '--untracked-files=no') === ''
     ) {
       return {
         available: true,
@@ -265,6 +311,16 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
       // Clear a stale base tree left by a crashed run — it would fail `add`. Its
       // stderr is kept, because it is usually what explains that failure.
       sweep = discardWorktree(worktree, tree);
+      // The same question the probe phase asks before its own `worktree add`:
+      // this resolves the repository through the REVIEW worktree's gitfile,
+      // which lives inside the directory the sandbox mounts read-write and
+      // which the build/test phase already gave the reviewed code a chance to
+      // rewrite. `worktree add` checks files out, so it runs whatever that
+      // pointer leads to, on the host. See `untrustedGitfile`.
+      const untrusted = untrustedGitfile(worktree);
+      if (untrusted !== null) {
+        throw new Error(`refusing to create a base tree: ${untrusted}`);
+      }
       git(worktree, 'worktree', 'add', '--detach', tree, baseSha);
     } catch (e) {
       return unavailable(
@@ -335,9 +391,9 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
     }
 
     // The marker is what the fast path above trusts, so it is written only after
-    // a build that succeeded, and it records the SHA it vouches for.
+    // a build that succeeded, and it records the SHA and the run it vouches for.
     try {
-      writeFileSync(marker(), `${baseSha}\n`);
+      writeFileSync(marker(), `${baseSha}\n${runEpoch}\n`);
     } catch {
       // The tree may be too broken to hold a marker; the next shard rebuilds.
     }