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

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

@@ -18,6 +18,8 @@
 import { describe, it, expect, beforeEach, afterEach } from 'vitest';
 import { execFileSync } from 'node:child_process';
 import {
+  appendFileSync,
+  chmodSync,
   utimesSync,
   mkdtempSync,
   mkdirSync,
@@ -29,6 +31,7 @@ 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 { isolateHostGitConfig } from './lib/test-utils.js';
 import type { BuildTestReport } from './build-test.js';
 
 const okBuild = {
@@ -302,6 +305,199 @@ describe('runBaseTree', () => {
     expect(r.note).toMatch(/base worktree could not be created/);
   });
 
+  it('refuses while repo-local config defines a content filter — the add would execute it', () => {
+    // The base tree's `worktree add` checks every file out, and a checkout
+    // EXECUTES `filter.<name>.smudge` — the same surface `scratch-tree`
+    // refuses to reset through. The attributes line and a matching file make
+    // the execution real: without the screen, the add below fired the smudge.
+    const pwned = join(repo, 'PWNED-base');
+    git(worktree, 'config', 'filter.evil.smudge', `touch ${pwned}`);
+    const attrs = git(worktree, 'rev-parse', '--git-path', 'info/attributes');
+    appendFileSync(attrs, '*.txt filter=evil\n');
+
+    const r = run();
+
+    expect(r.available).toBe(false);
+    expect(r.note).toContain('filter.evil.smudge');
+    expect(existsSync(pwned)).toBe(false);
+    expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+  });
+
+  it('reports the add breached — never available — when a plant appears during the checkout', () => {
+    // The screen is a point-in-time read; the deterministic shape for its
+    // window with the add uses the screen's own disclosed limit — a filter
+    // in the GLOBAL config is the user's contract and never screened. Here
+    // its smudge arms a repo-LOCAL filter when the add's initial checkout
+    // executes it (attributes selecting it are committed at the base):
+    // absent at the pre-read, standing at every read after — so the report
+    // is breached and the just-added tree rolled back, never
+    // `available: true` over an executed plant.
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(repo, '.gitattributes'), '*.txt filter=armer\n');
+      git(repo, 'add', '-A');
+      git(repo, 'commit', '-qam', 'base-with-attributes');
+      const armedBase = git(repo, 'rev-parse', 'HEAD');
+      execFileSync(
+        'git',
+        [
+          'config',
+          '--global',
+          'filter.armer.smudge',
+          "git config filter.evil.smudge 'touch /tmp/qwen-never'",
+        ],
+        { cwd: repo },
+      );
+
+      const r = run({ plan: { mergeBaseSha: armedBase } });
+
+      expect(r.available).toBe(false);
+      expect(r.note).toContain('may have EXECUTED');
+      expect(r.note).toContain('filter.evil.smudge');
+      // The breached tree is rolled back, not left planted for the next
+      // shard.
+      expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+    } finally {
+      isolation.dispose();
+    }
+  });
+
+  it('reports the add breached when the plant ERASES ITSELF during the checkout', () => {
+    // The self-erasing shape the key re-read cannot see: the armer's
+    // smudge arms a repo-LOCAL filter AND unsets it again, per file, so
+    // the key is gone by the time the post-add re-read runs (probe, git
+    // 2.39: the re-read answered clean and the command certified a tree
+    // whose initial checkout had fired the plant). What the plant cannot
+    // erase is the change to the config file itself — the baseline the
+    // screen captured beside its clean read names it.
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(repo, '.gitattributes'), '*.txt filter=armer\n');
+      git(repo, 'add', '-A');
+      git(repo, 'commit', '-qam', 'base-with-attributes');
+      const armedBase = git(repo, 'rev-parse', 'HEAD');
+      execFileSync(
+        'git',
+        [
+          'config',
+          '--global',
+          'filter.armer.smudge',
+          "git config filter.evil.smudge 'touch /tmp/qwen-never'; " +
+            'git config --unset filter.evil.smudge',
+        ],
+        { cwd: repo },
+      );
+
+      const r = run({ plan: { mergeBaseSha: armedBase } });
+
+      expect(r.available).toBe(false);
+      expect(r.note).toContain('may have EXECUTED');
+      // No key survived the erase — the refusal must name the changed
+      // file instead of reading as a clean point-in-time re-read.
+      expect(r.note).toContain('changed');
+      expect(existsSync(baseWorktreePath(worktree))).toBe(false);
+    } finally {
+      isolation.dispose();
+    }
+  });
+
+  // POSIX-only by construction: the killer arms `kill -9 $PPID` and a
+  // marker embedded UNQUOTED in the shell-lexed smudge, and on a Windows
+  // lane the backslashes of the platform path are shell escapes — the
+  // marker can never land at the asserted path and the positive assertion
+  // is deterministically red regardless of the guard under test.
+  it.skipIf(process.platform === 'win32')(
+    'attributes a plant the add EXECUTED even when the checkout threw after it',
+    () => {
+      // A checkout can throw AFTER executing a plant: the smudge fires,
+      // leaves its self-erasing trace in the common config, and kills git —
+      // the spawn dies on the signal and `git` throws. The catch used to
+      // report the add failure alone, burying the execution under it, and
+      // the next call screened clean over the run (the plant had erased
+      // itself). The baseline the screen captured still stands at the
+      // catch, so the paired re-read attributes it there.
+      const isolation = isolateHostGitConfig();
+      try {
+        const pwned = join(repo, 'PWNED-base-kill');
+        writeFileSync(join(repo, '.gitattributes'), '*.txt filter=killer\n');
+        git(repo, 'add', '-A');
+        git(repo, 'commit', '-qam', 'base-with-attributes');
+        const armedBase = git(repo, 'rev-parse', 'HEAD');
+        execFileSync(
+          'git',
+          [
+            'config',
+            '--global',
+            'filter.killer.smudge',
+            `git config qwen.plant.x 1; git config --unset qwen.plant.x; touch ${pwned}; kill -9 $PPID`,
+          ],
+          { cwd: repo },
+        );
+
+        const r = run({ plan: { mergeBaseSha: armedBase } });
+
+        // The plant EXECUTED — and the report says so, instead of reading
+        // as an add failure.
+        expect(existsSync(pwned)).toBe(true);
+        expect(r.available).toBe(false);
+        expect(r.note).toContain('may have EXECUTED');
+        expect(r.note).toContain('changed');
+      } finally {
+        isolation.dispose();
+      }
+    },
+  );
+
+  it('sweeps a stale tree whose own admin config holds a plant, instead of wedging on it', () => {
+    // The screen's candidate set reads every `<common>/worktrees/*/
+    // config.worktree`, and it used to run ABOVE the stale sweep: a plant
+    // parked in the stale base tree's OWN admin dir refused every retry —
+    // each attempt re-screened state the sweep's next statement would have
+    // destroyed, and the refusal never moved (measured live: every retry
+    // refused identically until manual cleanup). The screen now runs below
+    // the sweep: the discard removes the plant with the tree, the add never
+    // reads it, and the base tree is built.
+    const stale = baseWorktreePath(worktree);
+    git(repo, 'worktree', 'add', '--detach', '-q', stale, baseSha);
+    const admin = git(
+      stale,
+      'rev-parse',
+      '--path-format=absolute',
+      '--git-dir',
+    );
+    writeFileSync(
+      join(admin, 'config.worktree'),
+      '[filter "evil"]\n\tsmudge = touch /tmp/qwen-never\n',
+    );
+
+    const r = run();
+
+    expect(r.available).toBe(true);
+    expect(existsSync(join(admin, 'config.worktree'))).toBe(false);
+  });
+
+  it('adds the base tree with a planted hook and fsmonitor inert', () => {
+    // The add's initial checkout fires `post-checkout` and refreshes the
+    // index — running `core.fsmonitor` — from the COMMON dir: one executable
+    // file and one config write a probe can make. The screen refuses content
+    // filters (above); these two surfaces are neutralised at the spawn
+    // instead, and this pins the prefix on THIS command's git — removing it
+    // creates both markers.
+    const hooksDir = join(repo, '.git', 'hooks');
+    git(repo, 'config', 'core.hooksPath', hooksDir);
+    mkdirSync(hooksDir, { recursive: true });
+    const hook = join(hooksDir, 'post-checkout');
+    writeFileSync(hook, `#!/bin/sh\ntouch ${repo}/PWNED-hook\n`);
+    chmodSync(hook, 0o755);
+    git(worktree, 'config', 'core.fsmonitor', `touch ${repo}/PWNED-fsm`);
+
+    const r = run();
+
+    expect(r.available).toBe(true);
+    expect(existsSync(join(repo, 'PWNED-hook'))).toBe(false);
+    expect(existsSync(join(repo, 'PWNED-fsm'))).toBe(false);
+  });
+
   it('ignores an exported GIT_DIR redirect when adding the base tree', () => {
     // An exported GIT_DIR overrides repository discovery for every git call
     // that inherits it: the add would land in the redirected repository and

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

@@ -56,8 +56,12 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import { baseWorktreePath } from './lib/paths.js';
 import {
   discardWorktree,
+  INERT_GIT_ARGS,
+  localFilterBreach,
+  localFilterRefusal,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
+  type LocalFilterBaseline,
   type SweepResult,
 } from './lib/worktree.js';
 import { runBuildTest, type BuildTestReport } from './build-test.js';
@@ -94,8 +98,11 @@ export interface BaseTreeArgs {
 // discovery for every call at once — the base tree would be added into the
 // redirected repository and its reuse check would read HEAD from it, an A/B
 // against the wrong program while every check against the given tree passes.
+// The INERT_GIT_ARGS prefix is for the `worktree add` below: its initial
+// checkout fires hooks and `core.fsmonitor` from the common dir otherwise,
+// and a probe's plant lands exactly there.
 function gitOut(cwd: string, ...args: string[]): string {
-  const r = spawnSync('git', args, {
+  const r = spawnSync('git', [...INERT_GIT_ARGS, ...args], {
     cwd,
     encoding: 'utf8',
     env: sanitizedGitEnv(),
@@ -108,7 +115,7 @@ function gitOut(cwd: string, ...args: string[]): string {
 }
 
 function git(cwd: string, ...args: string[]): void {
-  const r = spawnSync('git', args, {
+  const r = spawnSync('git', [...INERT_GIT_ARGS, ...args], {
     cwd,
     encoding: 'utf8',
     env: sanitizedGitEnv(),
@@ -261,12 +268,67 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport {
   // The parameter re-narrows: TS narrowing does not cross function scopes.
   function buildBaseTree(baseSha: string): BaseTreeReport {
     let sweep: SweepResult | undefined;
+    // Hoisted above the try: the catch below runs the paired re-read when
+    // the add THREW, and a re-read without the baseline the screen
+    // captured is blind to every self-erasing shape.
+    const captured: { baseline: LocalFilterBaseline | null } = {
+      baseline: null,
+    };
     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);
+      // BEFORE the checkout runs, directly beside it: the `worktree add`
+      // below executes whatever the screen detects — the same surface
+      // `scratch-tree` refuses to reset through, one directory over. The
+      // screen sits BELOW the sweep, not above it — a plant parked in the
+      // stale tree's own admin `config.worktree` is state that sweep just
+      // destroyed and the add never reads, and a screen that ran first
+      // refused on it forever: every retry re-screened the doomed state and
+      // wedged the repository out of every review (measured live).
+      const refusal = localFilterRefusal(
+        worktree,
+        'the worktree add this command runs',
+        captured,
+      );
+      if (refusal !== null) return unavailable(refusal);
       git(worktree, 'worktree', 'add', '--detach', tree, baseSha);
+      // Re-read AFTER the checkout, paired with the screen above: the
+      // screen is a point-in-time read, and a concurrent writer the
+      // pipeline itself schedules (a sibling shard's suite, a toggler the
+      // reap did not reach) can plant between the two reads — the add's
+      // initial checkout executes the plant while both reads see nothing.
+      // A key that APPEARED — or a screened file that CHANGED, the trace
+      // a self-erasing plant leaves when its unset lands before this
+      // re-read — is reported as a breach and the just-added tree rolled
+      // back: the run is never certified clean with a REPO-LOCAL plant
+      // behind it. A filter defined only in the GLOBAL config is the
+      // screen's disclosed limit and leaves no repo-local trace the
+      // re-read can see.
+      const breach = localFilterBreach(
+        worktree,
+        'the worktree add this command ran',
+        captured.baseline,
+      );
+      if (breach !== null) {
+        discardWorktree(worktree, tree);
+        return unavailable(breach);
+      }
     } catch (e) {
+      // A checkout that THREW may still have executed a plant first — a
+      // smudge that kills git mid-checkout fires and only THEN makes the
+      // spawn throw — and the failure detail below would bury the
+      // execution under an add error. Attribute it while the baseline the
+      // screen captured still stands; the next call's screen may read
+      // clean, the plant having erased itself.
+      if (captured.baseline !== null) {
+        const breach = localFilterBreach(
+          worktree,
+          'the worktree add this command ran',
+          captured.baseline,
+        );
+        if (breach !== null) return unavailable(breach);
+      }
       return unavailable(
         worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')),
       );