fix(review): screen content filters before the probe tree's restore too (#9558)

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

@@ -15,9 +15,11 @@
 // actually succeeded, since an A/B against a half-built tree measures the build,
 // not the diff.
 
-import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
 import { execFileSync } from 'node:child_process';
 import {
+  appendFileSync,
+  chmodSync,
   utimesSync,
   mkdtempSync,
   mkdirSync,
@@ -31,6 +33,25 @@ import { runBaseTree, type BaseTreeReport } from './base-tree.js';
 import { baseWorktreePath } from './lib/paths.js';
 import type { BuildTestReport } from './build-test.js';
 
+// The race witness needs a writer landing BETWEEN the opening screen and the
+// creation add; the sweep is the code that runs between them, so it is the
+// seam. Defaults to the real implementation — every other test runs it.
+const seam = vi.hoisted(() => ({
+  realDiscard: undefined as ((...args: unknown[]) => unknown) | undefined,
+  beforeDiscard: undefined as (() => void) | undefined,
+}));
+vi.mock('./lib/worktree.js', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./lib/worktree.js')>();
+  seam.realDiscard = actual.discardWorktree as (...a: unknown[]) => unknown;
+  return {
+    ...actual,
+    discardWorktree: (...args: unknown[]) => {
+      seam.beforeDiscard?.();
+      return seam.realDiscard!(...args);
+    },
+  };
+});
+
 const okBuild = {
   ok: true,
   toolchain: 'npm',
@@ -94,7 +115,10 @@ describe('runBaseTree', () => {
     git(repo, 'worktree', 'add', '--detach', '-q', worktree, headSha);
   });
 
-  afterEach(() => rmSync(repo, { recursive: true, force: true }));
+  afterEach(() => {
+    seam.beforeDiscard = undefined;
+    rmSync(repo, { recursive: true, force: true });
+  });
 
   it('creates a sibling worktree holding the BASE commit, not the head', () => {
     const r = run();
@@ -113,6 +137,98 @@ describe('runBaseTree', () => {
     expect(r.path).toBe(`${worktree}-base`);
   });
 
+  it('REFUSES the creation checkout when the common config plants a content filter', () => {
+    // The creation checkout rewrites every file the base commit carries,
+    // which EXECUTES a planted filter, and nothing in the pipeline wipes the
+    // common dir the plant persists in. The screen must refuse the build the
+    // way the probe-creation screen refuses its checkout — an unavailable A/B
+    // (infrastructure), never an executed filter.
+    const pwned = join(repo, 'PWNED-base-create');
+    appendFileSync(
+      join(repo, '.git', 'config'),
+      `[filter "evil"]\n\tsmudge = touch ${pwned}\n`,
+    );
+    mkdirSync(join(repo, '.git', 'info'), { recursive: true });
+    writeFileSync(join(repo, '.git', 'info', 'attributes'), '* filter=evil\n');
+
+    const builds: string[] = [];
+    const r = run({}, (w) => {
+      builds.push(w);
+      return okBuild;
+    });
+
+    expect(r.available).toBe(false);
+    expect(r.note).toContain('content filter');
+    expect(builds).toHaveLength(0);
+    expect(existsSync(pwned)).toBe(false);
+    expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+  });
+
+  it('the creation checkout is INERT — a planted post-checkout hook never fires', () => {
+    // The screen certifies FILTERS only; `worktree add` still fires
+    // `post-checkout` from the shared common hooks dir (measured live, git
+    // 2.39 and 2.43, on this exact `--detach` shape). A probe writes the
+    // hook with the same facility as the filter plant — one write + chmod —
+    // so the creation spawn must carry the inert overrides too.
+    const pwned = join(repo, 'PWNED-base-hook');
+    mkdirSync(join(repo, '.git', 'hooks'), { recursive: true });
+    writeFileSync(
+      join(repo, '.git', 'hooks', 'post-checkout'),
+      `#!/bin/sh\ntouch ${pwned}\n`,
+    );
+    chmodSync(join(repo, '.git', 'hooks', 'post-checkout'), 0o755);
+
+    const r = run();
+
+    // Inert is not "the build failed": the tree came up and the build ran.
+    expect(r.available).toBe(true);
+    expect(existsSync(pwned)).toBe(false);
+  });
+
+  it('the creation checkout empties core.fsmonitor — a planted command never fires', () => {
+    // The screen's regex matches filter keys only, so a fsmonitor-only plant
+    // passes it clean — and `worktree add --detach` runs a repo-local
+    // `core.fsmonitor` (measured live). The override empties the key at the
+    // spawn, the way the probe tree's creation always has.
+    const pwned = join(repo, 'PWNED-base-fsmonitor');
+    appendFileSync(
+      join(repo, '.git', 'config'),
+      `[core]\n\tfsmonitor = touch ${pwned}\n`,
+    );
+
+    const r = run();
+
+    expect(r.available).toBe(true);
+    expect(existsSync(pwned)).toBe(false);
+  });
+
+  it('re-screens after the sweep — a plant between the screen and the add is refused', () => {
+    // The lock excludes other base-tree builders, not shards running attacker
+    // code: a concurrent probe can land the two-write plant the instant the
+    // stale tree's sweep finishes — after the opening screen read, before
+    // the creation add re-parses the config (measured live: 6/6 race
+    // iterations executed the plant). The seam stands in for that writer.
+    const pwned = join(repo, 'PWNED-base-race');
+    seam.beforeDiscard = () => {
+      appendFileSync(
+        join(repo, '.git', 'config'),
+        `[filter "evil"]\n\tsmudge = touch ${pwned}\n`,
+      );
+      mkdirSync(join(repo, '.git', 'info'), { recursive: true });
+      writeFileSync(
+        join(repo, '.git', 'info', 'attributes'),
+        '* filter=evil\n',
+      );
+    };
+
+    const r = run();
+
+    expect(r.available).toBe(false);
+    expect(r.note).toContain('content filter');
+    expect(existsSync(pwned)).toBe(false);
+    expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+  });
+
   it('builds in the base tree, and only there', () => {
     const seen: string[] = [];
     const r = run({}, (w) => {

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

@@ -56,6 +56,8 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import { baseWorktreePath } from './lib/paths.js';
 import {
   discardWorktree,
+  INERT_GIT_ARGS,
+  localFilterRefusal,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
   type SweepResult,
@@ -260,12 +262,44 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
 
   // The parameter re-narrows: TS narrowing does not cross function scopes.
   function buildBaseTree(baseSha: string): BaseTreeReport {
+    // The creation checkout rewrites every file the base commit carries, which
+    // EXECUTES a planted filter — screen it the way the probe tree's creation
+    // is screened. The refusal reads as an unavailable A/B (infrastructure),
+    // never as a finding against the PR.
+    const filterRefusal = localFilterRefusal(
+      worktree,
+      "the base tree's creation checkout",
+    );
+    if (filterRefusal) return unavailable(filterRefusal);
     let sweep: SweepResult | undefined;
     try {
       // 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);
-      git(worktree, 'worktree', 'add', '--detach', tree, baseSha);
+      // Re-screen beside the add, after the sweep: this is the only screened
+      // checkout that overlaps LIVE probes — the lock excludes other base-tree
+      // builders, not shards running attacker code, and the sweep's completion
+      // is the public signal a watcher plants on. A filter landing between the
+      // screen above and the add below executed in the creation checkout the
+      // screen certified clean (measured: 6/6 race iterations pwned); the
+      // re-screen narrows that window to the gap between itself and the add.
+      const rescreenRefusal = localFilterRefusal(
+        worktree,
+        "the base tree's creation checkout",
+      );
+      if (rescreenRefusal) return unavailable(rescreenRefusal);
+      // INERT_GIT_ARGS: the screen reads filters only, while `worktree add`
+      // also fires `post-checkout` from the shared common hooks dir and runs
+      // a repo-local `core.fsmonitor` — both plantable, both measured live.
+      git(
+        worktree,
+        ...INERT_GIT_ARGS,
+        'worktree',
+        'add',
+        '--detach',
+        tree,
+        baseSha,
+      );
     } catch (e) {
       return unavailable(
         worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')),

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

@@ -252,6 +252,10 @@ const producerMocks = vi.hoisted(() => ({
   execFileSync: vi.fn(),
   refExists: vi.fn((..._refs: unknown[]): boolean => false),
   releaseWorktree: vi.fn(() => ({ existed: false, freed: true })),
+  // The screen itself is owned by lib/worktree's own suite; here it is a seam
+  // — the contract under test is that fetch-pr ASKS before the creation
+  // checkout and refuses the fetch on a hit. Default: a clean repository.
+  localFilterRefusal: vi.fn((..._args: unknown[]): string | null => null),
   gitOpt: vi.fn((..._args: string[]): string | null => null),
   // The exit-status-aware probe as its own vi.fn: the default mapping (set
   // in beforeEach) can only produce exit 0 and the DEFINITIVE no (exit 1),
@@ -353,6 +357,13 @@ vi.mock('./lib/git.js', () => ({
   releaseWorktree: producerMocks.releaseWorktree,
 }));
 
+// Partial mock: only the filter screen is steered from the tests; everything
+// else (sanitizedGitEnv included) stays the real module.
+vi.mock('./lib/worktree.js', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./lib/worktree.js')>();
+  return { ...actual, localFilterRefusal: producerMocks.localFilterRefusal };
+});
+
 vi.mock('./lib/merge-base.js', () => ({
   resolveMergeBase: producerMocks.resolveMergeBase,
 }));
@@ -422,6 +433,7 @@ describe('fetch-pr report assembly', () => {
       throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' });
     });
     producerMocks.refExists.mockReturnValue(false);
+    producerMocks.localFilterRefusal.mockReturnValue(null);
     producerMocks.git.mockImplementation((...args: string[]) =>
       args[0] === 'rev-parse' ? 'f00df00df00d' : '',
     );
@@ -571,6 +583,62 @@ describe('fetch-pr report assembly', () => {
     expect(reportCall).toBeUndefined();
   });
 
+  it('refuses the creation worktree when the local filter screen refuses', async () => {
+    // The review worktree's own creation checkout is the pipeline's actual
+    // FIRST checkout — every later screen runs inside the tree it creates —
+    // and it rewrites every fetched file, executing a planted filter. A
+    // screen hit must refuse the fetch before the checkout, roll the fetched
+    // ref back, release the lease, and write no report.
+    producerMocks.localFilterRefusal.mockReturnValue(
+      "the repository's local config defines content filter(s) " +
+        'filter.evil.smudge (in /repo/.git/config) — the review worktree’s ' +
+        'creation checkout would EXECUTE them',
+    );
+
+    await expect(reportFor({})).rejects.toThrow(
+      /Failed to create worktree .*content filter/,
+    );
+    expect(producerMocks.localFilterRefusal).toHaveBeenCalledWith(
+      process.cwd(),
+      "the review worktree's creation checkout",
+    );
+    // The creation checkout itself never ran. `includes`, not positional:
+    // the spawn carries INERT_GIT_ARGS ahead of the subcommand, so a
+    // positional match never sees the add call and the ordering this test
+    // pins would go untested.
+    expect(
+      producerMocks.git.mock.calls.some(
+        (args: unknown[]) => args.includes('worktree') && args.includes('add'),
+      ),
+    ).toBe(false);
+    // No report: the refusal is a hard stop, not a degraded review.
+    const reportCall = producerMocks.writeFileSync.mock.calls.find(
+      ([path]) => path === '/tmp/fetch-report.json',
+    );
+    expect(reportCall).toBeUndefined();
+    // The lease this run created is rolled back with every other failure.
+    expect(clearReviewWorktreeLeaseIfOwned).toHaveBeenCalled();
+  });
+
+  it('runs the creation checkout INERT — hooks and fsmonitor emptied at the spawn', async () => {
+    // The screen certifies FILTERS only; `worktree add` still fires
+    // `post-checkout` from the shared common hooks dir and runs a repo-local
+    // `core.fsmonitor` (measured live, both surfaces, on this exact shape).
+    // This checkout is the pipeline's FIRST — it must carry the same
+    // overrides the probe-tree spawns carry.
+    await reportFor({});
+    const addCall = producerMocks.git.mock.calls.find(
+      (args: unknown[]) => args.includes('worktree') && args.includes('add'),
+    );
+    expect(addCall).toBeDefined();
+    expect(addCall!.slice(0, 4)).toEqual([
+      '-c',
+      'core.hooksPath=/dev/null/no-hooks',
+      '-c',
+      'core.fsmonitor=',
+    ]);
+  });
+
   it('refuses the refspec channel on baseRefName too (+ and colon)', async () => {
     // `--` ends option parsing, but a leading `+` or `src:dst` shape still
     // parses as a (force) refspec after it — same channels as
@@ -1036,7 +1104,9 @@ describe('fetch-pr report assembly', () => {
 
     it('clears the lease when the worktree add fails', async () => {
       producerMocks.git.mockImplementation((...args: string[]) => {
-        if (args[0] === 'worktree') throw new Error('disk full');
+        // The creation add carries the inert `-c` overrides ahead of the
+        // subcommand, so match the pair anywhere in the argv.
+        if (args.includes('worktree')) throw new Error('disk full');
         return args[0] === 'rev-parse' ? 'f00df00d' : '';
       });
 

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

@@ -38,7 +38,11 @@ import {
   reviewLeaseHeldByAnotherSession,
   reviewLeasePath,
 } from '../../services/review-worktree-lease.js';
-import { sanitizedGitEnv } from './lib/worktree.js';
+import {
+  INERT_GIT_ARGS,
+  localFilterRefusal,
+  sanitizedGitEnv,
+} from './lib/worktree.js';
 import { setGhHost } from './lib/gh.js';
 import { getPlatformReader } from './lib/platform/registry.js';
 import type { ReviewPlatformReader } from './lib/platform/types.js';
@@ -968,10 +972,25 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> {
     // once it exists. Tracked so the recomputed numbers replace the zeros.
     const needsLocalStats = platform.kind !== 'github';
 
-    // 4. Create the ephemeral worktree.
+    // 4. Create the ephemeral worktree. Its creation checkout rewrites every
+    //    file the fetched head carries — which EXECUTES a planted content
+    //    filter — and it is the pipeline's actual FIRST checkout: every other
+    //    screen runs inside the tree this call creates, so none exists yet in
+    //    this run. A screen hit refuses the fetch the way the probe-creation
+    //    screen refuses its checkout; the rollback below then removes the
+    //    fetched ref and the outer catch releases the lease. The spawn
+    //    carries INERT_GIT_ARGS because the screen reads filters only, while
+    //    `worktree add` ALSO fires `post-checkout` from the shared common
+    //    hooks dir and runs a repo-local `core.fsmonitor` — both plantable
+    //    with one write each, both measured live on this exact shape.
     try {
+      const filterRefusal = localFilterRefusal(
+        process.cwd(),
+        "the review worktree's creation checkout",
+      );
+      if (filterRefusal) throw new Error(filterRefusal);
       mkdirSync(dirname(wt), { recursive: true });
-      git('worktree', 'add', wt, ref);
+      git(...INERT_GIT_ARGS, 'worktree', 'add', wt, ref);
     } catch (err) {
       tryRemove(() =>
         execFileSync('git', ['branch', '-D', ref], {