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

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

@@ -1971,7 +1971,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

@@ -23,7 +23,9 @@ import {
   mkdirSync,
   rmSync,
   writeFileSync,
+  cpSync,
   existsSync,
+  readFileSync,
 } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
@@ -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;
@@ -96,6 +105,34 @@ describe('runBaseTree', () => {
 
   afterEach(() => rmSync(repo, { recursive: true, force: true }));
 
+  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.
+      const planted = join(repo, '.qwen', 'tmp', '.evil-git');
+      const real = readFileSync(join(worktree, '.git'), 'utf8')
+        .trim()
+        .replace('gitdir: ', '');
+      cpSync(real, planted, { recursive: true });
+      writeFileSync(join(planted, 'commondir'), `${join(repo, '.git')}\n`);
+      writeFileSync(
+        join(planted, 'gitdir'),
+        `gitdir: ${join(worktree, '.git')}\n`,
+      );
+      writeFileSync(join(worktree, '.git'), `gitdir: ${planted}\n`);
+
+      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);

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

@@ -55,11 +55,13 @@ 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 { mountRootFor } from './lib/sandboxed-exec.js';
 import { runBuildTest, type BuildTestReport } from './build-test.js';
 
 export interface BaseTreeReport {
@@ -265,6 +267,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, mountRootFor);
+      if (untrusted !== null) {
+        throw new Error(`refusing to create a base tree: ${untrusted}`);
+      }
       git(worktree, 'worktree', 'add', '--detach', tree, baseSha);
     } catch (e) {
       return unavailable(

packages/cli/src/commands/review/cleanup.test.ts

@@ -103,7 +103,7 @@ vi.mock('../../services/review-worktree-lease.js', () => ({
   readReviewWorktreeLease: mocks.readReviewWorktreeLease,
   reviewLeaseHeldByAnotherSession: mocks.reviewLeaseHeldByAnotherSession,
   reviewLeasePath: (repositoryRoot: string, target: string) =>
-    `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`,
+    `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`,
   isReviewLeaseFile: (fileName: string) =>
     /^qwen-review-lease-pr-\d+\.json$/.test(fileName),
 }));

packages/cli/src/commands/review/fetch-pr.test.ts

@@ -331,7 +331,7 @@ vi.mock('../../services/review-worktree-lease.js', () => ({
   readReviewWorktreeLease: vi.fn((): unknown => null),
   reviewLeaseHeldByAnotherSession: vi.fn((): boolean => false),
   reviewLeasePath: (repositoryRoot: string, target: string) =>
-    `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`,
+    `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`,
 }));
 
 vi.mock('./lib/gh.js', async (importOriginal) => {

packages/cli/src/commands/review/fetch-pr.ts

@@ -38,7 +38,8 @@ import {
   reviewLeaseHeldByAnotherSession,
   reviewLeasePath,
 } from '../../services/review-worktree-lease.js';
-import { sanitizedGitEnv } from './lib/worktree.js';
+import { sanitizedGitEnv, untrustedGitfile } from './lib/worktree.js';
+import { mountRootFor } from './lib/sandboxed-exec.js';
 import { setGhHost } from './lib/gh.js';
 import { getPlatformReader } from './lib/platform/registry.js';
 import type { ReviewPlatformReader } from './lib/platform/types.js';
@@ -683,11 +684,35 @@ function tryResume(
   const markerResumes = marker.resumes.filter(
     (r) => r.sessionId.toLowerCase() !== currentKey,
   ).length;
+  // Before reading anything through this tree's own pointer: `--resume`
+  // coexists with the sandbox on the very lane it polices, and the tree it
+  // reads is the one the previous run's containerized commands could write.
+  // `status` REFRESHES THE INDEX, so a planted `core.fsmonitor` runs on the
+  // host inside the very command that collects the ruling's evidence — the
+  // attack does not need the resume to succeed. See `untrustedGitfile`.
+  // REFUSE TO FRESH, not refuse to crash. Throwing here propagates out of
+  // `runFetchPr`, the enclosing catch rolls the lease back and re-throws, and
+  // `cleanStale` — the thing that would REMOVE the planted tree — is never
+  // reached. That contradicts what this file, the `--resume` describe and the
+  // docs all promise: the flag never fails a run that could start over. So
+  // this returns a refusal like every other one, the fresh path runs, and the
+  // planted tree is swept on the way.
+  if (untrustedGitfile(wt, mountRootFor) !== null) {
+    return {
+      resumed: false,
+      reason: 'worktree-untrusted',
+      priorFetchedSha: null,
+    };
+  }
   // `--porcelain` prints nothing on a clean tree; a null (the command could
   // not run) is treated as dirty. `--untracked-files=normal` explicitly, so
   // a `status.showUntrackedFiles=no` tuning cannot hide residue that is not
-  // in the PR.
+  // in the PR. `core.fsmonitor` inert here too: the gate above proves the
+  // pointer, this proves the command cannot be turned into an execution even
+  // if some future path reaches it ungated.
   const status = gitOpt(
+    '-c',
+    'core.fsmonitor=',
     '-C',
     wt,
     'status',
@@ -973,6 +998,20 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> {
     // 4. Create the ephemeral worktree.
     try {
       mkdirSync(dirname(wt), { recursive: true });
+      // The fresh path checks files out too, and resolves through the same
+      // pointer the resume path was gated on — but the pointer it resolves
+      // through is the LAUNCH directory's, not the new tree's: `git()` sets no
+      // cwd, so `worktree add` finds the repository from `process.cwd()`.
+      // Gating `wt` was a no-op — `cleanStale` above has just removed whatever
+      // stood there, and a tree that does not exist has no pointer to
+      // distrust. In the nested geometry `mountRootFor` itself documents, the
+      // outer review's containerized phase can rewrite exactly this one.
+      const freshUntrusted = untrustedGitfile(process.cwd(), mountRootFor);
+      if (freshUntrusted !== null) {
+        throw new Error(
+          `refusing to create a review worktree: ${freshUntrusted}`,
+        );
+      }
       git('worktree', 'add', wt, ref);
     } catch (err) {
       tryRemove(() =>

packages/cli/src/commands/review/lib/paths.ts

@@ -38,11 +38,28 @@ export function assertWritableOutPath(out: string): void {
 }
 
 export const REVIEW_TMP_DIR = join('.qwen', 'tmp');
+
+/**
+ * Where worktree leases live — a SIBLING of the review temp dir, not a child.
+ *
+ * The leases are host-trusted state: `cleanupReviewWorktreeLeases` matches one
+ * by session ids alone and then force-removes whatever worktree and deletes
+ * whatever branch it names. `REVIEW_TMP_DIR` is the directory the review
+ * sandbox bind-mounts read-write, so keeping them there put that state inside
+ * the container's writable surface — and inside reach of an unsandboxed run
+ * too, which is how it stood before the sandbox existed. Reviewed code that
+ * can edit a lease can make another session's cleanup destroy the wrong tree,
+ * or plant one no session will ever sweep and wedge that PR on that machine.
+ *
+ * One directory over is the whole fix: nothing mounts it, and the lease
+ * lifecycle is unchanged.
+ */
+export const REVIEW_LEASE_DIR = join('.qwen', 'review-leases');
 export const REVIEWS_DIR = join('.qwen', 'reviews');
 export const REVIEW_CACHE_DIR = join('.qwen', 'review-cache');
 
 /**
- * Filename prefix for review-worktree lease files under `REVIEW_TMP_DIR`.
+ * Filename prefix for review-worktree lease files under `REVIEW_LEASE_DIR`.
  * Lives here, not in `review-worktree-lease.ts`, because the review
  * workflow's cleanup sweep deletes leases by glob — the sweep pattern and
  * the lease writer must share one definition (the cleanup spec pins both).

packages/cli/src/commands/review/lib/resume.ts

@@ -37,7 +37,8 @@ export type ResumeRefusal =
   | 'diff-unreadable' // the captured diff is gone or cannot be read
   | 'diff-hash-mismatch' // the diff file changed since it was captured
   | 'head-moved' // the PR head advanced — the once-per-review restart case
-  | 'resume-cap'; // this review has already resumed RESUME_MAX times
+  | 'resume-cap' // this review has already resumed RESUME_MAX times
+  | 'worktree-untrusted'; // the tree's gitfile no longer resolves to its own admin entry
 
 export type ResumeAssessment =
   | { ok: true }