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/cleanup.test.ts

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

@@ -330,7 +330,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/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/worktree.test.ts

@@ -29,9 +29,11 @@ import {
   writeFileSync,
 } from 'node:fs';
 import { tmpdir } from 'node:os';
-import { basename, dirname, join } from 'node:path';
+import { basename, dirname, join, relative, sep } from 'node:path';
 import { isolateHostGitConfig } from './test-utils.js';
 import {
+  adminEntryInsideReviewTmp,
+  untrustedGitfile,
   discardWorktree,
   exposeDependencies,
   sanitizedGitEnv,
@@ -1872,3 +1874,154 @@ describe('worktreeCreateFailureDetail', () => {
     );
   });
 });
+
+describe('untrustedGitfile', () => {
+  const made: string[] = [];
+  const tmp = () => {
+    const dir = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-gitfile-')));
+    made.push(dir);
+    return dir;
+  };
+  afterEach(() => {
+    for (const dir of made.splice(0))
+      rmSync(dir, { recursive: true, force: true });
+  });
+
+  /** The pipeline's own shape: a gitfile naming an entry under `.git/worktrees`. */
+  const pipelineTree = (repo: string) => {
+    const tree = join(repo, '.qwen', 'tmp', 'review-pr-1');
+    const entry = join(repo, '.git', 'worktrees', 'review-pr-1');
+    mkdirSync(tree, { recursive: true });
+    mkdirSync(entry, { recursive: true });
+    writeFileSync(join(tree, '.git'), `gitdir: ${entry}\n`);
+    return { tree, entry, mount: () => join(repo, '.qwen', 'tmp') };
+  };
+
+  it('ADMITS an intact pipeline gitfile, relative spelling included', () => {
+    // The admit path, which nothing exercised: every refusal case here would
+    // also refuse under a mutation that mangles the parsed target — realpath
+    // fails, the location check fails closed, and the expected refusal still
+    // happens — while in production the same mangling refuses every healthy
+    // tree. Only asserting the ADMIT distinguishes the two.
+    const repo = tmp();
+    const { tree, entry, mount } = pipelineTree(repo);
+    expect(untrustedGitfile(tree, mount)).toBeNull();
+
+    // Git writes this pointer relative when it can, so the parse has to
+    // resolve against the TREE, not the process cwd.
+    writeFileSync(
+      join(tree, '.git'),
+      `gitdir: ${relative(tree, entry)}\n`.split(sep).join('/'),
+    );
+    expect(untrustedGitfile(tree, mount)).toBeNull();
+  });
+
+  it('refuses the two shapes reviewed code can write', () => {
+    const repo = tmp();
+    const { tree, mount } = pipelineTree(repo);
+    // A pointer into the mount...
+    const planted = join(repo, '.qwen', 'tmp', '.evil-git');
+    mkdirSync(planted, { recursive: true });
+    writeFileSync(join(tree, '.git'), `gitdir: ${planted}\n`);
+    expect(untrustedGitfile(tree, mount)).toContain('review temp dir');
+
+    // ...and `.git` replaced by a directory, which skips every gate written
+    // for the gitfile shape.
+    rmSync(join(tree, '.git'), { force: true });
+    mkdirSync(join(tree, '.git'));
+    expect(untrustedGitfile(tree, mount)).toContain('not the gitfile');
+  });
+
+  it('says nothing at all outside a mount', () => {
+    // A plain checkout's `.git` IS a directory; refusing that would refuse
+    // every ordinary repository.
+    const repo = tmp();
+    mkdirSync(join(repo, '.git'));
+    expect(untrustedGitfile(repo, () => null)).toBeNull();
+  });
+});
+
+describe('adminEntryInsideReviewTmp', () => {
+  const made: string[] = [];
+  const tmp = () => {
+    const dir = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-admin-')));
+    made.push(dir);
+    return dir;
+  };
+  afterEach(() => {
+    for (const dir of made.splice(0))
+      rmSync(dir, { recursive: true, force: true });
+  });
+
+  it('refuses an entry planted inside the mounted directory', () => {
+    // The shape the round-trip gate cannot see: the writer chose BOTH the
+    // gitfile's target and that target's backpointer, so the two agree, no
+    // symlink is involved, and `--show-toplevel` still names the tree. Only
+    // the entry's location tells them apart.
+    const repo = tmp();
+    const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe');
+    const planted = join(repo, '.qwen', 'tmp', '.evil-git');
+    mkdirSync(tree, { recursive: true });
+    mkdirSync(planted, { recursive: true });
+    expect(
+      adminEntryInsideReviewTmp(
+        planted,
+        () => join(repo, '.qwen', 'tmp'),
+        tree,
+      ),
+    ).toBe(true);
+  });
+
+  it('counts the mount root itself, and a child that looks like an escape', () => {
+    // The two shapes a hand-rolled prefix test gets wrong, and both are places
+    // a planted entry can actually sit. The mount ROOT is as writable as
+    // anything under it, and `..evil-git` is a legal filename whose relative
+    // path starts with the characters an escape would.
+    const repo = tmp();
+    const mount = join(repo, '.qwen', 'tmp');
+    const tree = join(mount, 'review-pr-1-probe');
+    const oddly = join(mount, '..evil-git');
+    mkdirSync(tree, { recursive: true });
+    mkdirSync(oddly, { recursive: true });
+    expect(adminEntryInsideReviewTmp(mount, () => mount, tree)).toBe(true);
+    expect(adminEntryInsideReviewTmp(oddly, () => mount, tree)).toBe(true);
+    // ...while a genuine sibling of the mount is still outside.
+    const outside = join(repo, '.qwen', 'review-leases');
+    mkdirSync(outside, { recursive: true });
+    expect(adminEntryInsideReviewTmp(outside, () => mount, tree)).toBe(false);
+  });
+
+  it('admits the real admin entry, which lives under the repository git dir', () => {
+    const repo = tmp();
+    const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe');
+    const real = join(repo, '.git', 'worktrees', 'review-pr-1-probe');
+    mkdirSync(tree, { recursive: true });
+    mkdirSync(real, { recursive: true });
+    expect(
+      adminEntryInsideReviewTmp(real, () => join(repo, '.qwen', 'tmp'), tree),
+    ).toBe(false);
+  });
+
+  it('refuses rather than guesses when the entry cannot be resolved', () => {
+    // Fails CLOSED: "not inside" would be a guess, and the guess that lets a
+    // planted entry through is the one that ends in host execution.
+    const repo = tmp();
+    const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe');
+    mkdirSync(tree, { recursive: true });
+    expect(
+      adminEntryInsideReviewTmp(
+        join(repo, 'gone'),
+        () => join(repo, '.qwen', 'tmp'),
+        tree,
+      ),
+    ).toBe(true);
+  });
+
+  it('has nothing to say about a tree outside any mounted directory', () => {
+    // A local checkout is never mounted, so there is no writable surface for
+    // this question to be about — and answering `true` there would refuse
+    // every ordinary repository.
+    const repo = tmp();
+    expect(adminEntryInsideReviewTmp(repo, () => null, repo)).toBe(false);
+  });
+});

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

@@ -34,6 +34,7 @@ import {
 } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { basename, dirname, isAbsolute, join, resolve, sep } from 'node:path';
+import { isSubpath } from '@qwen-code/qwen-code-core';
 import { readWorkspacePackages } from './workspaces.js';
 
 export type SweepResult = ReturnType<typeof spawnSync>;
@@ -109,6 +110,115 @@ const GIT_ENV_EXEC = [
   'XDG_CONFIG_HOME',
 ];
 
+/**
+ * Why a tree's own gitfile cannot be trusted to resolve a host-side git write.
+ *
+ * Returns a refusal, or null when there is nothing to police. Every host-side
+ * git command that WRITES resolves the repository through the `.git` of the
+ * tree it runs in, and for every tree this pipeline builds that `.git` sits
+ * inside the directory the sandbox mounts read-write. Two shapes reach a
+ * planted repository from there, and both end in a `filter.<x>.smudge`
+ * running on the host:
+ *
+ * - the gitfile rewritten to an admin entry planted under the same mount, and
+ * - the gitfile REPLACED by a `.git` directory, which is a repository of the
+ *   writer's own and skips every identity gate written for the gitfile shape.
+ *
+ * Neither is a shape the pipeline creates under the mount — its trees come
+ * from `git worktree add`, which writes a gitfile pointing at
+ * `<repo>/.git/worktrees/` — so refusing both costs nothing. Outside a mount
+ * this says nothing at all: a plain checkout's `.git` IS a directory, and
+ * refusing that would refuse every ordinary repository.
+ */
+export function untrustedGitfile(
+  tree: string,
+  // Required, not defaulted to `mountRootFor`: importing it here would close
+  // an ESM cycle — `sandboxed-exec` already imports `redirectedAncestor` from
+  // this module — and a cycle whose only symptom appears at module-init time
+  // is not worth a saved argument.
+  mountRoot: (cwd: string) => string | null,
+): string | null {
+  const root = mountRoot(tree);
+  if (root === null) return null;
+  const dotGit = join(tree, '.git');
+  let stat;
+  try {
+    stat = lstatSync(dotGit);
+  } catch {
+    return `${tree} has no .git to resolve`;
+  }
+  if (!stat.isFile()) {
+    return `${tree}'s .git is not the gitfile the pipeline created, and the tree is inside the review temp dir`;
+  }
+  let target: string;
+  try {
+    const raw = readFileSync(dotGit, 'utf8').trim();
+    if (!raw.startsWith('gitdir:')) {
+      return `${tree}'s .git does not name an admin entry`;
+    }
+    target = resolve(tree, raw.slice('gitdir:'.length).trim());
+  } catch (e) {
+    return e instanceof Error ? e.message : String(e);
+  }
+  if (adminEntryInsideReviewTmp(target, mountRoot, tree)) {
+    return `${tree}'s admin entry is inside the review temp dir, where the reviewed code can rewrite it`;
+  }
+  return null;
+}
+
+/**
+ * Whether a linked worktree's admin entry sits where reviewed code can write it.
+ *
+ * The gitfile inside a pipeline tree cannot move — git requires `<tree>/.git` —
+ * and that tree is inside the directory the sandbox bind-mounts read-write, so
+ * containerized code (and, before the sandbox, any code the review ran) can
+ * rewrite it to `gitdir: <a directory it planted under the same mount>`. Put a
+ * `config` carrying `filter.<x>.smudge` in that directory and the next
+ * HOST-side `git checkout` executes it — out of the container, as the review
+ * user, on CI with the runner's tokens.
+ *
+ * The identity gates that already exist do not catch this, and cannot: the
+ * planted directory's own backpointer round-trips because the same writer
+ * chose it, `--show-toplevel` still prints the tree the gitfile sits in, and
+ * no symlink is involved anywhere. Asking git for the common dir does not help
+ * either — that answer is resolved THROUGH the rewritten gitfile, so it comes
+ * from the same hand.
+ *
+ * The question that does have an honest answer is about location, not
+ * content: a real linked worktree's admin entry lives under
+ * `<repo>/.git/worktrees/`, which is outside the mounted directory; a planted
+ * one has to be inside it, because that is the only place the writer can
+ * reach. So refuse an admin entry that resolves inside the review temp dir,
+ * and let every other check stand as it is.
+ */
+export function adminEntryInsideReviewTmp(
+  gitDir: string,
+  mountRoot: (cwd: string) => string | null,
+  tree: string,
+): boolean {
+  const root = mountRoot(tree);
+  if (root === null) return false;
+  let real: string;
+  let realRoot: string;
+  try {
+    real = realpathSync(resolve(gitDir));
+    realRoot = realpathSync(root);
+  } catch {
+    // Unresolvable is not a licence to proceed: the caller's other gates
+    // report it, and answering "not inside" here would be a guess.
+    return true;
+  }
+  // `isSubpath`, not a hand-rolled prefix test. The first cut was hand-rolled
+  // and failed open at both boundaries of the question it was asking: an entry
+  // whose realpath IS the mount root produced an empty relative path and was
+  // read as "outside" — while the root is exactly as writable as anything
+  // under it — and a direct child legitimately named `..evil-git` produced a
+  // relative path starting with `..`, which the prefix test read as an escape.
+  // Both are places a planted entry can sit, and both are why path containment
+  // belongs in one tested helper rather than in each caller's arithmetic.
+  return isSubpath(realRoot, real);
+}
+
 /**
  * The first symlink at or above `dir`, or null when every component is real.
  *