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

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

@@ -3024,7 +3024,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.ts

@@ -55,12 +55,19 @@ 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 {
+  baseTreeTrustPath,
+  builtTreeRecord,
+  recordBuiltTree,
+  runNonce,
+} from './lib/base-tree-trust.js';
 
 export interface BaseTreeReport {
   /**
@@ -107,6 +114,22 @@ function gitOut(cwd: string, ...args: string[]): string {
   return (r.stdout ?? '').trim();
 }
 
+// The NUL-delimited form: `gitOut`'s `.trim()` would eat a leading-space
+// filename and the record separator is NUL, not whitespace.
+function gitOutZ(cwd: string, ...args: string[]): string {
+  const r = spawnSync('git', args, {
+    cwd,
+    encoding: 'utf8',
+    env: sanitizedGitEnv(),
+  });
+  if (r.error) throw r.error;
+  if (r.status !== 0) {
+    throw new Error(`git ${args.join(' ')} failed: ${r.stderr ?? ''}`);
+  }
+  const out = r.stdout ?? '';
+  return out.endsWith('\0') ? out.slice(0, -1) : out;
+}
+
 function git(cwd: string, ...args: string[]): void {
   const r = spawnSync('git', args, {
     cwd,
@@ -119,6 +142,35 @@ function git(cwd: string, ...args: string[]): void {
   }
 }
 
+// The tree's untracked AND ignored FILES, listed individually. Two
+// `ls-files` calls rather than `git status`: status collapses a directory to
+// one `dir/` entry, and a plant dropped INSIDE a directory the build left —
+// `dist/cli.js`, `node_modules/.bin/<x>`, exactly what a host-side A/B
+// executes — then changes no set membership. `ls-files --others` never
+// collapses. The residual, by design: content rewritten IN PLACE at a
+// recorded path changes no membership either — the path-set limit stated at
+// the subset check below.
+function untrackedPaths(tree: string): string[] {
+  const others = gitOutZ(
+    tree,
+    'ls-files',
+    '-z',
+    '--others',
+    '--exclude-standard',
+  );
+  const ignored = gitOutZ(
+    tree,
+    'ls-files',
+    '-z',
+    '--others',
+    '--ignored',
+    '--exclude-standard',
+  );
+  return [...others.split('\0'), ...ignored.split('\0')]
+    .filter((e) => e !== '')
+    .sort();
+}
+
 export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
   const unavailable = (note: string): BaseTreeReport => ({
     available: false,
@@ -175,20 +227,106 @@ 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 stamp secret, persisted OUTSIDE the mount beside the worktree
+  // leases (see lib/base-tree-trust.ts). The fence used to key on the plan's
+  // mtime, and the plan lives inside the directory the sandbox mounts
+  // read-write — reviewed code could stat it, compute the epoch, and write
+  // it into a forged stamp, after which every reuse check passed on an
+  // attacker-shaped tree. A random per-run secret the mount can neither
+  // read nor write is what a stamp now vouches with. Without it no marker
+  // can be trusted or written, so its absence fails the command, not the
+  // fence.
+  const trustPath = baseTreeTrustPath(worktree, args.plan);
+  let nonce: string;
   try {
-    if (
-      existsSync(tree) &&
-      readFileSync(marker(), 'utf8').trim() === baseSha &&
-      gitOut(tree, 'rev-parse', 'HEAD') === baseSha
-    ) {
-      return {
-        available: true,
-        path: tree,
-        baseSha,
-        build: null,
-        note: `base tree already built at ${baseSha.slice(0, 9)} in ${tree} (reusing it — a concurrent or earlier probe built it)`,
-      };
+    nonce = runNonce(trustPath);
+  } catch (err) {
+    return unavailable(
+      `could not establish the run's trust artifact ${trustPath}: ` +
+        `${(err as Error).message}`,
+    );
+  }
+  try {
+    const stamp = readFileSync(marker(), 'utf8').trim().split('\n');
+    if (existsSync(tree) && stamp[0] === baseSha && stamp[1] === nonce) {
+      // ...and THIS run built it — genuinely: the secret never enters the
+      // mount, so a mount-local writer cannot have refreshed or forged the
+      // stamp. `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.
+      // The reuse path RETURNS, so the gate below the rebuild never runs
+      // for it — and both facts these arms establish come from inside the
+      // mount: `rev-parse HEAD` resolves through that tree's own `.git`,
+      // and its working files are a direct child of the directory the
+      // sandbox mounts read-write. A planted repository answers the right
+      // sha for a working tree holding anything at all; a plain copy
+      // overwrites the tracked sources while HEAD never moves — and the
+      // A/B's BASE side is then the reviewed code's own, so a test the PR
+      // breaks fails identically on both sides and reads as pre-existing.
+      const pointerWhy = untrustedGitfile(tree);
+      if (
+        pointerWhy !== null ||
+        gitOut(tree, 'rev-parse', 'HEAD') !== baseSha
+      ) {
+        // A rewritten pointer or a moved HEAD has NO benign cause: this
+        // run's build does not touch either, and a concurrent A/B does not
+        // move HEAD. Not the busy arm — the discard-and-rebuild below is
+        // exactly what sweeps the plant, and declining here would leave it
+        // standing for the rest of the run.
+      } else {
+        const recorded = builtTreeRecord(trustPath, tree);
+        if (recorded === null || recorded.baseSha !== baseSha) {
+          // A torn or stale record is not a live tree's state — the rebuild
+          // below self-heals it.
+        } else if (
+          // Tracked dirt OR an untracked addition. Both have benign causes
+          // on a tree this run built: codegen and lockfile rewrites from
+          // this run's own build, a concurrent shard's snapshot `--update`
+          // mid-A/B, the A/B's own cache output. Both are also what a plant
+          // looks like — and that ambiguity is exactly why the answer is to
+          // DECLINE rather than to discard: discarding on this signal
+          // sweeps a live tree another shard may be mid-A/B in (the
+          // concurrent-shard clobber this fast path exists to prevent),
+          // while a genuine plant wedged here is never executed, and the
+          // next run's fresh nonce discards it.
+          //
+          // `--untracked-files=no` for the tracked arm: 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 — the untracked surface
+          // is the subset arm's, recorded host-side at marker write (a path
+          // SET per the finding's prescription; content rewritten in place
+          // at a recorded path changes no membership, stated not hidden).
+          gitOut(tree, 'status', '--porcelain', '--untracked-files=no') !==
+            '' ||
+          !untrackedPaths(tree).every((p) => recorded.untracked.includes(p))
+        ) {
+          return unavailable(
+            `the base tree at ${baseSha.slice(0, 9)} was built by this run ` +
+              'but no longer passes a reuse check (a concurrent probe may be ' +
+              'writing it mid-A/B); declining to reuse or discard it — retry ' +
+              'when the probe finishes, or settle the claim by reading',
+          );
+        } else {
+          return {
+            available: true,
+            path: tree,
+            baseSha,
+            build: null,
+            note: `base tree already built at ${baseSha.slice(0, 9)} in ${tree} (reusing it — a concurrent or earlier probe built it)`,
+          };
+        }
+      }
     }
+    // No marker, or a stamp that does not carry this run's secret — an
+    // earlier run's leftover, or a forge computed from the mount-visible
+    // inputs. Not a refusal: an unusable leftover is what the rebuild exists
+    // for. Falling through discards the tree (removing a 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.
   } catch {
     // No marker, unreadable marker, or a tree git cannot answer for: rebuild.
   }
@@ -197,9 +335,16 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
   // the same "unavailable" — and the sweep destroys the evidence tree the
   // failure deliberately leaves standing.
   try {
+    const failed = readFileSync(failedMarker(), 'utf8').trim().split('\n');
     if (
       existsSync(tree) &&
-      readFileSync(failedMarker(), 'utf8').trim() === baseSha
+      failed[0] === baseSha &&
+      // The same fence the ok marker carries, on the path that needs it most:
+      // this branch settles the question with NO build at all, and the file
+      // lives inside the mount — a sha-only marker is one planted line away
+      // from suppressing the A/B lane for the whole round, reading as
+      // infrastructure rather than as an attack.
+      failed[1] === nonce
     ) {
       return {
         available: false,
@@ -265,6 +410,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(
@@ -316,9 +471,11 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
       // Leave the tree standing. A base that does not build is a fact worth
       // looking at by hand, and deleting the evidence to save a directory is a
       // bad trade — `cleanup` sweeps it at the end of the review either way.
-      // The marker makes the failure a SETTLED answer for every later shard.
+      // The marker makes the failure a SETTLED answer for every later shard,
+      // and it carries the run secret for the same reason the ok marker does:
+      // a planted sha-only marker must settle nothing (the fence above).
       try {
-        writeFileSync(failedMarker(), `${baseSha}\n`);
+        writeFileSync(failedMarker(), `${baseSha}\n${nonce}\n`);
       } catch {
         // The tree may be too broken to hold a marker; the next shard repays.
       }
@@ -335,9 +492,14 @@ 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
+    // — the run secret, not the mount-visible epoch it replaced. With it goes
+    // the host-side record of the untracked set this build legitimately left,
+    // so the reuse fence can tell that residue from a plant dropped later
+    // (marker first, so the marker itself is part of the recorded set).
     try {
-      writeFileSync(marker(), `${baseSha}\n`);
+      writeFileSync(marker(), `${baseSha}\n${nonce}\n`);
+      recordBuiltTree(trustPath, tree, baseSha, untrackedPaths(tree));
     } catch {
       // The tree may be too broken to hold a marker; the next shard rebuilds.
     }