From fde3119916edc47a1c89ff1b621d49a11e005797 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Sat, 29 Aug 2026 00:27:48 +0800
Subject: [PATCH 01/15] fix(review): screen content filters before the probe
 tree's restore too (#9558)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`scratch-tree` refuses to create or reset a tree while the repository's LOCAL
config defines a content filter, because a checkout executes
`filter.<name>.smudge` whenever it rewrites a file and disabling hooks does not
cover that. `test-efficacy`'s per-run restore is the same checkout one
directory over — it rewrites every tracked file in the probe tree, twice per
probe run — and screened nothing.

The screen moves to `lib/worktree.ts` so both callers share one implementation
rather than one growing a corner the other lacks, which is how the
`config.worktree` scoping bug got in.

`filter.<name>.process` joins `smudge` and `clean`: it is the third executable
key — a long-running filter git speaks a protocol to — and enumerating two of
three is how the first cut read as complete.

**Repo-local only, and that is the load-bearing part.** `git lfs install`
writes `filter.lfs.clean` into the user's GLOBAL config, so a screen over
merged config would put every contributor with git-lfs into permanent refusal —
the same failure as a tripwire that fires on every healthy run, which this
pipeline has already shipped once. Both halves are pinned: removing the screen
turns the new test red, and widening it to merged config turns the same test
red from the other side.
---
 .../cli/src/commands/review/lib/worktree.ts   | 74 +++++++++++++++++++
 .../src/commands/review/scratch-tree.test.ts  | 12 ++-
 .../cli/src/commands/review/scratch-tree.ts   | 72 +-----------------
 .../src/commands/review/test-efficacy.test.ts | 61 +++++++++++++++
 .../cli/src/commands/review/test-efficacy.ts  | 13 ++++
 5 files changed, 160 insertions(+), 72 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index 13a122da70f..aa6db6df8de 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -578,6 +578,80 @@ function trackedIgnoreSources(
  */
 export const RESIDUE_PATH_CAP = 12;
 
+/**
+ * The repo-local `filter.<name>` COMMANDS, when any are defined.
+ *
+ * Every checkout in this pipeline EXECUTES these — the scratch tree's reset and
+ * rebuild, and the probe tree's per-run restore — hooks being disabled covers
+ * hooks and not filters. The planting surface is two plain writes a probe can
+ * make into the COMMON dir this command's report calls shared:
+ * `git config filter.evil.smudge CMD` and one line appended to
+ * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never
+ * wipe the common dir, so a filter planted while reviewing one PR fires on
+ * every later matching checkout of the user's OWN repository — persistence
+ * planted by reviewing a malicious PR, measured live. The two local config
+ * files are checked with `--file` rather than merged config because filters
+ * in the user's global config (git-lfs is the common one) are the user's own
+ * contract, exactly like any git command they run — while a probe's planting
+ * surface is the repo-local files. The state cannot be told apart from a
+ * filter the user set deliberately, and cannot be safely wiped, so a hit is a
+ * refusal upstream, not a cleanup here.
+ */
+export function localFilterCommands(worktree: string): string[] {
+  const files = spawnSync(
+    'git',
+    ['rev-parse', '--git-common-dir', '--git-dir'],
+    { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
+  );
+  if (files.error || files.status !== 0 || typeof files.stdout !== 'string') {
+    return [];
+  }
+  const [commonDir, gitDir] = files.stdout.trim().split('\n');
+  const common = resolve(worktree, commonDir);
+  const candidates = [
+    join(common, 'config'),
+    join(resolve(worktree, gitDir), 'config.worktree'),
+  ];
+  // Every OTHER worktree's per-worktree config too. This screen runs against
+  // the review worktree, but the checkout it authorises runs in the SCRATCH
+  // tree, whose own `<common>/worktrees/<label>/config.worktree` is honored
+  // once `extensions.worktreeConfig` is on and was never read here — a filter
+  // planted there executed during the reset while this function reported the
+  // repository clean. The admin directory is one `readdir`, and a filter in
+  // any of these is a plant whichever tree carries it.
+  try {
+    for (const entry of readdirSync(join(common, 'worktrees'))) {
+      candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
+    }
+  } catch {
+    // No linked worktrees registered: the two candidates above are all of it.
+  }
+  const found: string[] = [];
+  for (const file of candidates) {
+    if (!existsSync(file)) continue;
+    const r = spawnSync(
+      'git',
+      [
+        'config',
+        '--file',
+        file,
+        '--get-regexp',
+        // `process` beside the pair: it is the third executable key (a
+        // long-running filter git speaks a protocol to), and enumerating two
+        // of three is how the first cut of this screen read as complete.
+        '^filter\\..*\\.(smudge|clean|process)$',
+      ],
+      { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
+    );
+    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') continue;
+    for (const line of r.stdout.split('\n')) {
+      const key = line.split(/\s+/)[0];
+      if (key && !found.includes(key)) found.push(key);
+    }
+  }
+  return found;
+}
+
 /**
  * The paths a tree carries that its HEAD commit does not — probe residue, seen
  * from the reading side (#9207).
diff --git a/packages/cli/src/commands/review/scratch-tree.test.ts b/packages/cli/src/commands/review/scratch-tree.test.ts
index 3c9f8b0e46b..74e0b6e0169 100644
--- a/packages/cli/src/commands/review/scratch-tree.test.ts
+++ b/packages/cli/src/commands/review/scratch-tree.test.ts
@@ -113,9 +113,19 @@ describe('runScratchTree', () => {
       existsSync(scratchWorktreePath(worktree, 'verify--round-1--abc123')),
     ).toBe(false);
 
+    // `process` is the third executable key — a long-running filter git speaks
+    // a protocol to — and enumerating `smudge`/`clean` and stopping is how the
+    // first cut of this screen read as complete.
+    git(worktree, 'config', '--unset', 'filter.evil.smudge');
+    git(worktree, 'config', 'filter.evil.process', `touch ${pwned}`);
+    const viaProcess = run();
+    expect(viaProcess.available).toBe(false);
+    expect(viaProcess.note).toContain('filter.evil.process');
+    expect(existsSync(pwned)).toBe(false);
+
     // A repo WITHOUT the filter still gets a tree (the global-config filters
     // a user's own git-lfs install carries are not this surface).
-    git(worktree, 'config', '--unset', 'filter.evil.smudge');
+    git(worktree, 'config', '--unset', 'filter.evil.process');
     expect(run().available).toBe(true);
   });
 
diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts
index ba9f53037ae..3659f927a15 100644
--- a/packages/cli/src/commands/review/scratch-tree.ts
+++ b/packages/cli/src/commands/review/scratch-tree.ts
@@ -42,7 +42,6 @@ import {
   lstatSync,
   mkdirSync,
   readFileSync,
-  readdirSync,
   realpathSync,
   writeFileSync,
 } from 'node:fs';
@@ -59,6 +58,7 @@ import {
   RESIDUE_PATH_CAP,
   discardWorktree,
   exposeDependencies,
+  localFilterCommands,
   redirectedAncestor,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
@@ -150,76 +150,6 @@ export interface ScratchTreeArgs {
  */
 const NO_HOOKS = ['-c', 'core.hooksPath=/dev/null/no-hooks'];
 
-/**
- * The repo-local `filter.<name>.smudge|clean` commands, when any are defined.
- *
- * The reset's and rebuild's checkouts EXECUTE these — hooks are disabled above,
- * filters are not — and the planting surface is two plain writes a probe can
- * make into the COMMON dir this command's report calls shared:
- * `git config filter.evil.smudge CMD` and one line appended to
- * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never
- * wipe the common dir, so a filter planted while reviewing one PR fires on
- * every later matching checkout of the user's OWN repository — persistence
- * planted by reviewing a malicious PR, measured live. The two local config
- * files are checked with `--file` rather than merged config because filters
- * in the user's global config (git-lfs is the common one) are the user's own
- * contract, exactly like any git command they run — while a probe's planting
- * surface is the repo-local files. The state cannot be told apart from a
- * filter the user set deliberately, and cannot be safely wiped, so a hit is a
- * refusal upstream, not a cleanup here.
- */
-function localFilterCommands(worktree: string): string[] {
-  const files = spawnSync(
-    'git',
-    ['rev-parse', '--git-common-dir', '--git-dir'],
-    { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
-  );
-  if (files.error || files.status !== 0 || typeof files.stdout !== 'string') {
-    return [];
-  }
-  const [commonDir, gitDir] = files.stdout.trim().split('\n');
-  const common = resolve(worktree, commonDir);
-  const candidates = [
-    join(common, 'config'),
-    join(resolve(worktree, gitDir), 'config.worktree'),
-  ];
-  // Every OTHER worktree's per-worktree config too. This screen runs against
-  // the review worktree, but the checkout it authorises runs in the SCRATCH
-  // tree, whose own `<common>/worktrees/<label>/config.worktree` is honored
-  // once `extensions.worktreeConfig` is on and was never read here — a filter
-  // planted there executed during the reset while this function reported the
-  // repository clean. The admin directory is one `readdir`, and a filter in
-  // any of these is a plant whichever tree carries it.
-  try {
-    for (const entry of readdirSync(join(common, 'worktrees'))) {
-      candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
-    }
-  } catch {
-    // No linked worktrees registered: the two candidates above are all of it.
-  }
-  const found: string[] = [];
-  for (const file of candidates) {
-    if (!existsSync(file)) continue;
-    const r = spawnSync(
-      'git',
-      [
-        'config',
-        '--file',
-        file,
-        '--get-regexp',
-        '^filter\\..*\\.(smudge|clean)$',
-      ],
-      { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
-    );
-    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') continue;
-    for (const line of r.stdout.split('\n')) {
-      const key = line.split(/\s+/)[0];
-      if (key && !found.includes(key)) found.push(key);
-    }
-  }
-  return found;
-}
-
 function gitOut(cwd: string, ...args: string[]): string {
   // `ls-files -v` prints a line per tracked file and `clean` a line per removal;
   // both pass the default 1 MiB buffer on a large repo, and `spawnSync` answers
diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index b7f1772b01e..b67427b2c83 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -536,6 +536,67 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     }
   });
 
+  it('refuses to run through a repo-LOCAL content filter, and only a local one', () => {
+    // The restore rewrites every tracked file in this tree, and a checkout
+    // EXECUTES `filter.<name>.smudge` whenever it does — the same surface
+    // `scratch-tree` refuses to reset through, run twice per probe run one
+    // directory over with no screen at all.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-filter-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      writeFileSync(join(dir, 'b.ts'), 'export const b = 1;\n');
+      asCheckout(dir);
+      execFileSync(
+        'git',
+        ['config', 'filter.evil.smudge', 'touch /tmp/qwen-should-never-run'],
+        { cwd: dir },
+      );
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('filter.evil.smudge');
+
+      // ...and a filter in the user's GLOBAL config is their own contract, the
+      // way it is for any git command they run. `git lfs install` writes one
+      // there, so refusing on it would put every contributor with git-lfs into
+      // permanent refusal — the failure mode a blanket rule reproduces.
+      execFileSync('git', ['config', '--unset', 'filter.evil.smudge'], {
+        cwd: dir,
+      });
+      execFileSync(
+        'git',
+        ['config', '--global', 'filter.lfs.clean', 'git-lfs clean -- %f'],
+        { cwd: dir },
+      );
+
+      // It gets all the way to the runner, which this bare fixture does not
+      // have — the throw is from `findVitestBin`, and that IS the observation:
+      // the restore did not refuse, so the phase proceeded.
+      let afterDetail: string;
+      try {
+        afterDetail = runOneMutant(
+          dir,
+          { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+          ['a.test.ts'],
+        ).detail;
+      } catch (e) {
+        afterDetail = e instanceof Error ? e.message : String(e);
+      }
+
+      expect(afterDetail).not.toContain('filter.lfs.clean');
+      expect(afterDetail).not.toContain('content filter');
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
   it('refuses to run when the index hides a tracked file from the restore', () => {
     // `checkout --force` SILENTLY skips a file carrying skip-worktree, and
     // `clean` never touches a tracked file — so a bit the guest suite sets with
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index 7b84d43245c..1ee7799ae3b 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -84,6 +84,7 @@ import {
 import {
   discardWorktree,
   exposeDependencies,
+  localFilterCommands,
   redirectedAncestor,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
@@ -1643,6 +1644,18 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   // in a tree this code is defending against, so it is emptied here the way
   // every other checkout in this pipeline empties it. `--` and a pathspec:
   // this restores FILES and never moves HEAD.
+  // Filters, before either spawn. A checkout EXECUTES `filter.<name>.smudge`
+  // whenever it rewrites a file, and the restore below rewrites every tracked
+  // file this tree has — so the same surface `scratch-tree` refuses to reset
+  // through was, one directory over, run through twice per probe run. The
+  // screen reads repo-LOCAL config only: `git lfs install` writes
+  // `filter.lfs.clean` into the user's GLOBAL config, and refusing on that
+  // would put every contributor with git-lfs into permanent refusal — the same
+  // failure as a tripwire that fires on every healthy run.
+  const filters = localFilterCommands(probeTree);
+  if (filters.length > 0) {
+    return `the repository's local config defines content filter(s) ${filters.join(', ')}, which this tree's restore would EXECUTE`;
+  }
   // `core.fsmonitor` runs a command on BOTH of these, and the config that sets
   // it lives in the tree they are cleaning: the residue probe empties it for
   // exactly this reason and these two spawns were the ones still steerable.

From a426b4afb071d359654539cbd3c51b1a8264e0d6 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Sat, 29 Aug 2026 07:20:54 +0800
Subject: [PATCH 02/15] fix(review): screen the revert checkout and fail closed
 on an unread config
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round 1 review, three defects, all in the screen this PR installs.

The revert checkout ran unscreened. `checkout <base> -- <paths>` in the
mutation phase rewrites files, and a rewrite EXECUTES `filter.<name>.smudge`
exactly as the restore's does. Screening once at the restore is not enough:
everything between the two runs the PR's own test code, which can plant the
filter mid-run, and the mutation phase's catch deliberately continues so the
revert still runs — so a refused restore reaches the revert with the plant
live. Issue #9558's triage named this site; closing that issue means screening
it.

The enumeration failed open on its own spawn. The screened file is
attacker-writable and git prints every matching value in full, so padding one
`smudge` past `spawnSync`'s default 1 MiB buffer killed git with ENOBUFS and
the screen skipped the one file that defines the filter — reporting the
repository clean while the checkout ran the last value. The spawn now carries
the 64 MiB buffer its neighbours use, and any outcome other than exit 0 or 1
refuses instead of skipping. Existence is not readability either: git reports
an unreadable `--file` with exit 1, byte-identical to "no key matched", so the
candidate is settled here by construction — a regular file this process can
open — rather than by asking git and reading the exit code.

`localFilterCommands` therefore returns the keys AND the first candidate it
could not read; both call sites refuse on either.

The docstring claimed more than the code does. It opened with "Every checkout
in this pipeline EXECUTES these" and enumerated only the screened sites, and
said hooks were disabled pipeline-wide when only some spawns pass
`core.hooksPath`. It now names the screened set as the screened set and says
the creation checkouts are not in it.

The test pinned the message but not the order: it planted a canary it never
asserted, and committed no attributes line selecting the filter, so the
restore executed no smudge with or without the screen. The fixture now commits
`*.ts filter=evil`, the canary is repo-local rather than under /tmp, and its
absence is asserted — that assertion is what goes red if the screen moves
below the checkout.
---
 .../cli/src/commands/review/lib/worktree.ts   |  83 ++++++++++--
 .../cli/src/commands/review/scratch-tree.ts   |  12 +-
 .../review/test-efficacy.integration.test.ts  |  73 ++++++++++-
 .../src/commands/review/test-efficacy.test.ts | 120 +++++++++++++++++-
 .../cli/src/commands/review/test-efficacy.ts  |  31 ++++-
 5 files changed, 299 insertions(+), 20 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index aa6db6df8de..52c293d3e59 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -18,6 +18,8 @@
 
 import { spawnSync } from 'node:child_process';
 import {
+  accessSync,
+  constants as fsConstants,
   existsSync,
   lstatSync,
   mkdtempSync,
@@ -581,9 +583,18 @@ export const RESIDUE_PATH_CAP = 12;
 /**
  * The repo-local `filter.<name>` COMMANDS, when any are defined.
  *
- * Every checkout in this pipeline EXECUTES these — the scratch tree's reset and
- * rebuild, and the probe tree's per-run restore — hooks being disabled covers
- * hooks and not filters. The planting surface is two plain writes a probe can
+ * A checkout EXECUTES these whenever it rewrites a file, and disabling hooks
+ * does not cover filters — they are separate config. The callers that screen
+ * through this function are the scratch tree's reset and rebuild, the probe
+ * tree's per-run restore, and the probe tree's revert. That list is the set of
+ * SCREENED sites, not a census of the pipeline's filter-executing checkouts:
+ * the `worktree add` spawns that CREATE the probe and base trees run
+ * unscreened today, and hooks are neutralised only at the spawns that pass
+ * `core.hooksPath` explicitly, not pipeline-wide. Do not read this comment as
+ * a completeness map — widening the screen means adding a call site, and the
+ * count above is what has one.
+ *
+ * The planting surface is two plain writes a probe can
  * make into the COMMON dir this command's report calls shared:
  * `git config filter.evil.smudge CMD` and one line appended to
  * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never
@@ -597,14 +608,34 @@ export const RESIDUE_PATH_CAP = 12;
  * filter the user set deliberately, and cannot be safely wiped, so a hit is a
  * refusal upstream, not a cleanup here.
  */
-export function localFilterCommands(worktree: string): string[] {
+export interface LocalFilterScreen {
+  /** The repo-local `filter.<name>` command keys found, when any are defined. */
+  keys: string[];
+  /**
+   * The first candidate file the screen could not read to completion, when one
+   * stopped it — otherwise null.
+   *
+   * Exit 1 is git's ordinary "no key matched" and is not a failure. Anything
+   * else is: a spawn error, or the `maxBuffer` kill an attacker reaches by
+   * making the screened file itself large — the value is theirs to write, and
+   * git prints the whole of it. Skipping such a file reports the repository
+   * clean on the one file that defines the filter, so the caller refuses
+   * instead. One file, not a list: the refusal only has to name where the
+   * screen stopped, and an unbounded join is its own denial-of-service.
+   */
+  unreadable: string | null;
+}
+
+export function localFilterCommands(worktree: string): LocalFilterScreen {
   const files = spawnSync(
     'git',
     ['rev-parse', '--git-common-dir', '--git-dir'],
     { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
   );
   if (files.error || files.status !== 0 || typeof files.stdout !== 'string') {
-    return [];
+    // Not a usable repository from here: the checkout this screen guards has
+    // nothing to run in either, and its own failure is the caller's answer.
+    return { keys: [], unreadable: null };
   }
   const [commonDir, gitDir] = files.stdout.trim().split('\n');
   const common = resolve(worktree, commonDir);
@@ -627,8 +658,27 @@ export function localFilterCommands(worktree: string): string[] {
     // No linked worktrees registered: the two candidates above are all of it.
   }
   const found: string[] = [];
+  let unreadable: string | null = null;
   for (const file of candidates) {
     if (!existsSync(file)) continue;
+    // Existence is not readability, and git does not distinguish them for us:
+    // an unreadable `--file` exits 1 with a warning on stderr — byte-identical
+    // in status to "no key matched" — so asking git and reading the exit code
+    // reports a config this screen never read as clean. Settle it here instead,
+    // by construction: a regular file (git follows symlinks to read config, so
+    // this must too) that this process can open. Anything else is a candidate
+    // the screen could not check, which is a refusal, not a pass.
+    let readable = false;
+    try {
+      readable = statSync(file).isFile();
+      if (readable) accessSync(file, fsConstants.R_OK);
+    } catch {
+      readable = false;
+    }
+    if (!readable) {
+      unreadable ??= file;
+      continue;
+    }
     const r = spawnSync(
       'git',
       [
@@ -641,15 +691,32 @@ export function localFilterCommands(worktree: string): string[] {
         // of three is how the first cut of this screen read as complete.
         '^filter\\..*\\.(smudge|clean|process)$',
       ],
-      { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
+      {
+        cwd: worktree,
+        encoding: 'utf8',
+        // The screened file is attacker-writable and git prints every matching
+        // value in full, so the DEFAULT 1 MiB buffer is a switch the plant can
+        // flip: pad one `smudge` value past the cap, `spawnSync` kills git with
+        // ENOBUFS, and a screen that skips the file reports clean while the
+        // checkout runs the last value. Sized like the `ls-files` spawn above.
+        maxBuffer: 64 * 1024 * 1024,
+        env: sanitizedGitEnv(),
+      },
     );
-    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') continue;
+    // Exit 1 is "no key matched" — the ordinary clean answer. Every other
+    // outcome means this file was not read to the end, and a screen that did
+    // not finish must not be the reason a checkout is authorised.
+    if (r.error || (r.status !== 0 && r.status !== 1)) {
+      unreadable ??= file;
+      continue;
+    }
+    if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
       const key = line.split(/\s+/)[0];
       if (key && !found.includes(key)) found.push(key);
     }
   }
-  return found;
+  return { keys: found, unreadable };
 }
 
 /**
diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts
index 3659f927a15..13a451cca6f 100644
--- a/packages/cli/src/commands/review/scratch-tree.ts
+++ b/packages/cli/src/commands/review/scratch-tree.ts
@@ -424,9 +424,17 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport {
   // BEFORE any checkout runs — the reuse path's reset and the rebuild path's
   // `worktree add` both execute configured content filters.
   const filters = localFilterCommands(worktree);
-  if (filters.length > 0) {
+  if (filters.unreadable) {
     return unavailable(
-      `the repository's local config defines content filter(s) ${filters
+      `the repository's local config file ${inertPath(filters.unreadable)} ` +
+        'could not be read to the end, so this command cannot tell whether it ' +
+        'defines a content filter the checkouts below would EXECUTE. A screen ' +
+        'that did not finish is not a clean result — fix or remove that file.',
+    );
+  }
+  if (filters.keys.length > 0) {
+    return unavailable(
+      `the repository's local config defines content filter(s) ${filters.keys
         .map(inertPath)
         .join(', ')} — ` +
         'the checkouts this command runs would EXECUTE them (hooks are disabled, ' +
diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
index 815c156023f..1de3f993ab7 100644
--- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
@@ -970,13 +970,26 @@ process.stdout.write(JSON.stringify({
       `#!/usr/bin/env node
 import path from 'node:path';
 const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
+// Same positive-control semantics as the default fake: the injected
+// always-failing test must actually fail, or the run re-classes to
+// control-failed and never reaches the revert this test is about.
+const st = (f) => {
+  try {
+    return fs.readFileSync(f, 'utf8').includes('QWEN-REVIEW-POSITIVE-CONTROL')
+      ? 'failed'
+      : 'passed';
+  } catch {
+    return 'passed';
+  }
+};
 const results = files.map((f) => ({
   name: path.resolve(f),
-  assertionResults: [{ status: 'passed' }],
+  assertionResults: [{ status: st(f) }],
 }));
+const failed = results.filter((r) => r.assertionResults[0].status === 'failed').length;
 process.stdout.write(JSON.stringify({
-  numPassedTests: results.length,
-  numFailedTests: 0,
+  numPassedTests: results.length - failed,
+  numFailedTests: failed,
   testResults: results,
 }));
 `,
@@ -1670,6 +1683,60 @@ process.stdout.write(JSON.stringify({
     ]);
   });
 
+  it('refuses the REVERT checkout when the baseline suite planted a filter mid-run', async () => {
+    // The restore is screened once at the top of each run, but what runs
+    // BETWEEN the restore and the revert is the PR's own test code. It can
+    // write two lines into the shared common dir — `filter.evil.smudge` and an
+    // attributes line selecting it — and every later restore then refuses
+    // while the mutation phase's catch deliberately keeps going "so the revert
+    // probe below still runs". With the revert unscreened, that second
+    // checkout rewrites the PR-modified files through the planted smudge and
+    // the command executes on the host, the run looking like a plain
+    // inconclusive. The canary is the assertion; the verdicts are not.
+    const canary = join(repo, 'PWNED-revert-smudge');
+    writeFileSync(
+      vitestScript(),
+      `#!/usr/bin/env node
+import path from 'node:path';
+import fs from 'node:fs';
+import { execFileSync } from 'node:child_process';
+const g = (...a) => execFileSync('git', a, { encoding: 'utf8' }).trim();
+// Plant once — on the baseline — the way a probe's own test code would.
+const common = g('rev-parse', '--git-common-dir');
+const stamp = path.join(common, 'PLANTED');
+if (!fs.existsSync(stamp)) {
+  fs.writeFileSync(stamp, '');
+  execFileSync('git', ['config', 'filter.evil.smudge', 'sh -c "pwd >> ${canary}; cat"']);
+  const attrs = g('rev-parse', '--git-path', 'info/attributes');
+  fs.mkdirSync(path.dirname(attrs), { recursive: true });
+  fs.appendFileSync(attrs, '*.ts filter=evil\\n');
+}
+const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
+const results = files.map((f) => ({
+  name: path.resolve(f),
+  assertionResults: [{ status: 'passed' }],
+}));
+process.stdout.write(JSON.stringify({
+  numPassedTests: results.length,
+  numFailedTests: 0,
+  testResults: results,
+}));
+`,
+    );
+    const { wt, base } = scaffoldModifiedPr();
+
+    await runHandler({
+      report: join(repo, 'report.json'),
+      worktree: wt,
+      base,
+      out: join(repo, 'out.json'),
+    });
+
+    // The screen ran before the revert's checkout, so the planted command was
+    // never executed. Without the revert-phase screen this file exists.
+    expect(existsSync(canary)).toBe(false);
+  });
+
   it('never deletes a line that does not hold the selected statement', () => {
     // `runOneMutant`'s mismatch guard, pinned directly: selection and the
     // probe tree both derive from the same commit, so the command cannot reach
diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index b67427b2c83..6006e139e42 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -38,6 +38,7 @@ import {
   mkdtempSync,
   mkdirSync,
   writeFileSync,
+  appendFileSync,
   symlinkSync,
   existsSync,
   readFileSync,
@@ -546,12 +547,20 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     try {
       writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
       writeFileSync(join(dir, 'b.ts'), 'export const b = 1;\n');
+      // The attributes line is half the plant and belongs in the fixture: a
+      // filter git never SELECTS for a tracked path executes nothing, so
+      // without this the restore is harmless with or without the screen and
+      // the test would stay green if a later edit moved the screen below the
+      // checkout. With it, the canary below pins the ORDER, not just the
+      // message.
+      writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
       asCheckout(dir);
-      execFileSync(
-        'git',
-        ['config', 'filter.evil.smudge', 'touch /tmp/qwen-should-never-run'],
-        { cwd: dir },
-      );
+      // Repo-local, like the sibling scratch-tree test: a canary under /tmp
+      // would outlive the fixture on a shared runner.
+      const canary = join(dir, 'PWNED-smudge');
+      execFileSync('git', ['config', 'filter.evil.smudge', `touch ${canary}`], {
+        cwd: dir,
+      });
 
       const r = runOneMutant(
         dir,
@@ -561,6 +570,9 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
 
       expect(r.verdict).toBe('inconclusive');
       expect(r.detail).toContain('filter.evil.smudge');
+      // The refusal has to come BEFORE the checkout, not merely instead of a
+      // verdict: this is the assertion that goes red if the screen moves down.
+      expect(existsSync(canary)).toBe(false);
 
       // ...and a filter in the user's GLOBAL config is their own contract, the
       // way it is for any git command they run. `git lfs install` writes one
@@ -591,6 +603,104 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
 
       expect(afterDetail).not.toContain('filter.lfs.clean');
       expect(afterDetail).not.toContain('content filter');
+      // The local `evil` filter is gone, so nothing selected by the committed
+      // attributes line remains to execute either.
+      expect(existsSync(canary)).toBe(false);
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  it('refuses the REVERT checkout too, on a filter planted after the restore', () => {
+    // The restore is screened once, at the top of each run — but every run
+    // between then and the revert executes the PR's own test code, which can
+    // plant the filter mid-run. The mutation phase's catch deliberately keeps
+    // going "so the revert probe below still runs", so a refused restore
+    // reaches the revert with the plant live. A screen only at the restore
+    // lets that second checkout execute it.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-revert-filter-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
+      asCheckout(dir);
+      const canary = join(dir, 'PWNED-revert');
+      execFileSync('git', ['config', 'filter.evil.smudge', `touch ${canary}`], {
+        cwd: dir,
+      });
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(existsSync(canary)).toBe(false);
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  it('still sees a filter whose value is padded past the default spawn buffer', () => {
+    // The screened file is attacker-writable and git prints every matching
+    // value in full, so a `smudge` padded past `spawnSync`'s DEFAULT 1 MiB
+    // buffer used to kill git with ENOBUFS — and the screen skipped the one
+    // file that defines the filter and reported the repository clean, while
+    // the restore's checkout ran the last value.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-bigvalue-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
+      asCheckout(dir);
+      const canary = join(dir, 'PWNED-bigvalue');
+      // git takes the LAST value; the first is only there to push the
+      // enumeration's output past the default buffer.
+      appendFileSync(
+        join(dir, '.git', 'config'),
+        `[filter "evil"]\n\tsmudge = echo ${'x'.repeat(1200000)}\n` +
+          `\tsmudge = touch ${canary}\n`,
+      );
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('filter.evil.smudge');
+      expect(existsSync(canary)).toBe(false);
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  it('refuses a config candidate it cannot read — not "clean"', () => {
+    // git reports an unreadable `--file` with exit 1 and a warning on stderr —
+    // the same status as "no key matched" — so a screen that asks git and reads
+    // the exit code calls a config it never read clean. The candidate here is
+    // `config.worktree`, which git honors once `extensions.worktreeConfig` is
+    // on; a directory in its place is a candidate the screen cannot check.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-unreadable-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      asCheckout(dir);
+      mkdirSync(join(dir, '.git', 'config.worktree'));
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('could not be read to the end');
     } finally {
       isolation.dispose();
       rmSync(dir, { recursive: true, force: true });
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index 1ee7799ae3b..f76be0f4a76 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -1653,8 +1653,11 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   // would put every contributor with git-lfs into permanent refusal — the same
   // failure as a tripwire that fires on every healthy run.
   const filters = localFilterCommands(probeTree);
-  if (filters.length > 0) {
-    return `the repository's local config defines content filter(s) ${filters.join(', ')}, which this tree's restore would EXECUTE`;
+  if (filters.unreadable) {
+    return `the repository's local config file ${filters.unreadable} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`;
+  }
+  if (filters.keys.length > 0) {
+    return `the repository's local config defines content filter(s) ${filters.keys.join(', ')}, which this tree's restore would EXECUTE`;
   }
   // `core.fsmonitor` runs a command on BOTH of these, and the config that sets
   // it lives in the tree they are cleaning: the residue probe empties it for
@@ -2982,6 +2985,30 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
               'root), so the revert would run against whatever it points at',
           );
         }
+        // Filters, again, and for the same reason the restore screens them:
+        // `checkout base -- <paths>` below rewrites files, and a rewrite
+        // EXECUTES `filter.<name>.smudge`. Screening once at the restore is
+        // not enough — every run between then and here has executed the PR's
+        // own test code, which can plant the filter mid-run, and the mutation
+        // phase's catch deliberately continues so that this revert still runs.
+        // A refused restore therefore reaches this line with the plant live.
+        // The throw lands in this phase's existing catch and is recorded as a
+        // probe that did not run, which is what it is.
+        const revertFilters = localFilterCommands(probeTree);
+        if (revertFilters.unreadable) {
+          throw new Error(
+            `the repository's local config file ${revertFilters.unreadable} ` +
+              'could not be read to the end, so whether this revert would ' +
+              'EXECUTE a content filter is unknown',
+          );
+        }
+        if (revertFilters.keys.length > 0) {
+          throw new Error(
+            "the repository's local config defines content filter(s) " +
+              `${revertFilters.keys.join(', ')}, which this revert's checkout ` +
+              'would EXECUTE',
+          );
+        }
         // "Revert to base" is two operations, confined to the throwaway tree. A
         // file the PR MODIFIED is checked out from base; a file the PR ADDED did
         // not exist at base, so it is removed — through `safeRmWithin`, which

From 377b5a87f776cf19cdeb6a192b7fa6f8286a3a7f Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Sat, 29 Aug 2026 07:26:33 +0800
Subject: [PATCH 03/15] test(review): make the filter canary able to fire, and
 drop a misnamed test
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The canary lived inside the probe tree, where the restore's own `clean -ffdx`
deleted it before the assertion ran — so moving the screen below the checkout
left the suite green. It now sits in its own fixture directory outside the
tree, removed with the rest.

A checkout also runs the smudge only on files it REWRITES, and the fixture
never dirtied one, so the restore rewrote nothing. The tracked file is now
dirty before the run, which is the state a probe run actually leaves.

The unit test named for the REVERT checkout drove `runOneMutant`, which never
reaches the revert phase — it re-tested the restore under a misleading name.
The revert screen is covered end-to-end in the integration suite instead.
---
 .../src/commands/review/test-efficacy.test.ts | 52 ++++++-------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index 6006e139e42..c6cc1e8be3c 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -543,6 +543,7 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     // `scratch-tree` refuses to reset through, run twice per probe run one
     // directory over with no screen at all.
     const dir = mkdtempSync(join(tmpdir(), 'qwen-filter-'));
+    const canaryDir = mkdtempSync(join(tmpdir(), 'qwen-canary-'));
     const isolation = isolateHostGitConfig();
     try {
       writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
@@ -555,12 +556,19 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
       // message.
       writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
       asCheckout(dir);
-      // Repo-local, like the sibling scratch-tree test: a canary under /tmp
-      // would outlive the fixture on a shared runner.
-      const canary = join(dir, 'PWNED-smudge');
+      // OUTSIDE the probe tree, but inside a fixture this test removes: the
+      // restore's second spawn is `clean -ffdx`, which deletes an untracked
+      // canary written into the tree and would hide the very ordering bug the
+      // canary exists to catch. Nothing is left on the runner either way.
+      const canary = join(canaryDir, 'PWNED-smudge');
       execFileSync('git', ['config', 'filter.evil.smudge', `touch ${canary}`], {
         cwd: dir,
       });
+      // A checkout runs the smudge only on files it actually REWRITES, and it
+      // rewrites nothing when the tree already matches HEAD. A dirty file is
+      // what a probe run leaves behind and what the restore exists to undo, so
+      // it is also the condition under which the canary can fire at all.
+      writeFileSync(join(dir, 'a.ts'), 'dirtied by a previous run\n');
 
       const r = runOneMutant(
         dir,
@@ -609,38 +617,7 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     } finally {
       isolation.dispose();
       rmSync(dir, { recursive: true, force: true });
-    }
-  });
-
-  it('refuses the REVERT checkout too, on a filter planted after the restore', () => {
-    // The restore is screened once, at the top of each run — but every run
-    // between then and the revert executes the PR's own test code, which can
-    // plant the filter mid-run. The mutation phase's catch deliberately keeps
-    // going "so the revert probe below still runs", so a refused restore
-    // reaches the revert with the plant live. A screen only at the restore
-    // lets that second checkout execute it.
-    const dir = mkdtempSync(join(tmpdir(), 'qwen-revert-filter-'));
-    const isolation = isolateHostGitConfig();
-    try {
-      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
-      writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
-      asCheckout(dir);
-      const canary = join(dir, 'PWNED-revert');
-      execFileSync('git', ['config', 'filter.evil.smudge', `touch ${canary}`], {
-        cwd: dir,
-      });
-
-      const r = runOneMutant(
-        dir,
-        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
-        ['a.test.ts'],
-      );
-
-      expect(r.verdict).toBe('inconclusive');
-      expect(existsSync(canary)).toBe(false);
-    } finally {
-      isolation.dispose();
-      rmSync(dir, { recursive: true, force: true });
+      rmSync(canaryDir, { recursive: true, force: true });
     }
   });
 
@@ -651,12 +628,13 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     // file that defines the filter and reported the repository clean, while
     // the restore's checkout ran the last value.
     const dir = mkdtempSync(join(tmpdir(), 'qwen-bigvalue-'));
+    const canaryDir = mkdtempSync(join(tmpdir(), 'qwen-canary-'));
     const isolation = isolateHostGitConfig();
     try {
       writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
       writeFileSync(join(dir, '.gitattributes'), '*.ts filter=evil\n');
       asCheckout(dir);
-      const canary = join(dir, 'PWNED-bigvalue');
+      const canary = join(canaryDir, 'PWNED-bigvalue');
       // git takes the LAST value; the first is only there to push the
       // enumeration's output past the default buffer.
       appendFileSync(
@@ -664,6 +642,7 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
         `[filter "evil"]\n\tsmudge = echo ${'x'.repeat(1200000)}\n` +
           `\tsmudge = touch ${canary}\n`,
       );
+      writeFileSync(join(dir, 'a.ts'), 'dirtied by a previous run\n');
 
       const r = runOneMutant(
         dir,
@@ -677,6 +656,7 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     } finally {
       isolation.dispose();
       rmSync(dir, { recursive: true, force: true });
+      rmSync(canaryDir, { recursive: true, force: true });
     }
   });
 

From 70cafb6e814056bb9af2d09b48842adfe36c6a2e Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Sun, 30 Aug 2026 19:04:17 +0800
Subject: [PATCH 04/15] fix(review): bound the screen, close the
 directory-level hole, restore a fake
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round 2. Four defects in the screen, one of them mine from round 1, plus a
test fixture this PR broke.

**The 64 MiB buffer opened a quadratic dedup.** Raising `maxBuffer` from the
1 MiB default let git print millions of keys that the default had been
ENOBUFS-killing — straight into `found.includes(key)`, which is linear per
line. Measured on this machine: 100k keys took 144 005 ms as an array scan and
53 ms as a `Set`. The dedup is now a Set, and the reported list is capped at
`MAX_SCREEN_KEYS` with the pre-cap `total` carried alongside so a refusal says
how much it is not showing. Raising the ceiling without bounding what sits
behind it turned a fail-open into a hang.

**The candidate walk had no bound.** One synchronous `git config` spawn per
entry under `<common>/worktrees/`, on the never-wiped directory the filter
itself is planted in, on every screen call — once per mutant. Past
`MAX_SCREEN_CANDIDATES` the screen refuses; skipping instead would let filler
entries hide a real filter behind them.

**The readdir's bare catch answered "clean" for every failure.** Only ENOENT
means "no linked worktrees registered". An EACCES — a probe dropping the read
bit while keeping search — left git still opening each `config.worktree` by
direct path while the screen reported the repository clean. That is the
directory-level twin of the per-file readability gate added in round 1, which
this round leaves standing beside a hole.

**The refusal messages were not flattened.** `scratch-tree` runs the same
`LocalFilterScreen` values through `inertPath`; `test-efficacy` interpolated
them raw, and these strings reach the agent-facing report where JSON escaping
does not cover Cf, Zl/Zp or backticks. Both sites now share one helper that
flattens and caps.

**And the docstring still over-claimed.** "A probe's planting surface is the
repo-local files" is false — probe code runs as the user and can write
`~/.gitconfig`, which every checkout here reads and no screen sees. The
comment now states repo-local-only as the deliberate trade it is, with the
git-lfs cost that forces it.

The integration fixture is a mistake of mine from round 1: a single-occurrence
replace landed positive-control logic in the dead-runner fake belonging to
another test, which needs a runner that reports even the injected control
green. It used `fs` without importing it, so every call threw into a bare
catch and returned 'passed' — byte-equivalent to the fake it replaced, under a
comment claiming the opposite. Restored, comment and all.

Tests move to `lib/worktree.test.ts` and exercise `localFilterCommands`
directly: key cap with total, candidate cap, EACCES refusal, ENOENT staying
clean, and a parse failure (exit 128) refusing — the last covering the
fail-closed branch added in round 1, which no test reached because the
readability gate caught every case first.
---
 .../src/commands/review/lib/worktree.test.ts  | 110 ++++++++++++++++++
 .../cli/src/commands/review/lib/worktree.ts   |  80 +++++++++++--
 .../cli/src/commands/review/scratch-tree.ts   |   2 +
 .../review/test-efficacy.integration.test.ts  |  19 +--
 .../src/commands/review/test-efficacy.test.ts |  31 +++++
 .../cli/src/commands/review/test-efficacy.ts  |  28 ++++-
 6 files changed, 237 insertions(+), 33 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index 946020fc8f9..78196ca377b 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -32,8 +32,11 @@ import { tmpdir } from 'node:os';
 import { basename, dirname, join } from 'node:path';
 import { isolateHostGitConfig } from './test-utils.js';
 import {
+  MAX_SCREEN_CANDIDATES,
+  MAX_SCREEN_KEYS,
   discardWorktree,
   exposeDependencies,
+  localFilterCommands,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
   worktreeResidue,
@@ -1885,3 +1888,110 @@ describe('worktreeCreateFailureDetail', () => {
     );
   });
 });
+
+describe('localFilterCommands', () => {
+  let dir: string;
+  let isolation: { dispose: () => void };
+
+  const initRepo = (d: string) => {
+    execFileSync('git', ['init', '-q', '-b', 'main', '--template=', '.'], {
+      cwd: d,
+    });
+  };
+
+  beforeEach(() => {
+    isolation = isolateHostGitConfig();
+    dir = mkdtempSync(join(tmpdir(), 'qwen-screen-'));
+    initRepo(dir);
+  });
+
+  afterEach(() => {
+    try {
+      chmodSync(join(dir, '.git', 'worktrees'), 0o755);
+    } catch {
+      // Not every case creates it; rmSync below is the cleanup either way.
+    }
+    isolation.dispose();
+    rmSync(dir, { recursive: true, force: true });
+  });
+
+  it('finds a repo-local filter and reports the total', () => {
+    execFileSync('git', ['config', 'filter.evil.smudge', 'cat'], { cwd: dir });
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.keys).toEqual(['filter.evil.smudge']);
+    expect(screen.total).toBe(1);
+    expect(screen.unreadable).toBeNull();
+  });
+
+  it('caps the reported keys and keeps the pre-cap total', () => {
+    // The keys come from an attacker-writable file that cleanup never wipes;
+    // an unbounded list is its own denial-of-service in the refusal message.
+    let cfg = '';
+    for (let i = 0; i < MAX_SCREEN_KEYS + 5; i++) {
+      cfg += `[filter "evil${i}"]\n\tsmudge = cat\n`;
+    }
+    appendFileSync(join(dir, '.git', 'config'), cfg);
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.keys).toHaveLength(MAX_SCREEN_KEYS);
+    expect(screen.total).toBe(MAX_SCREEN_KEYS + 5);
+    // Still names a real planted key — a cap that reported none would be a
+    // refusal nobody can act on.
+    expect(screen.keys[0]).toMatch(/^filter\.evil\d+\.smudge$/);
+  });
+
+  it('refuses past the candidate cap instead of walking a plant\'s filler', () => {
+    // Each entry costs one synchronous `git config` spawn, on every screen
+    // call — once per mutant. Skipping past the bound would let filler hide a
+    // real filter behind it, so the screen refuses.
+    for (let i = 0; i <= MAX_SCREEN_CANDIDATES; i++) {
+      mkdirSync(join(dir, '.git', 'worktrees', `filler-${i}`), {
+        recursive: true,
+      });
+    }
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.unreadable).toBe(join(dir, '.git', 'worktrees'));
+    expect(screen.keys).toEqual([]);
+  });
+
+  it('refuses when the worktrees admin dir cannot be read', () => {
+    // git opens each `worktrees/<e>/config.worktree` by direct path — the
+    // search bit alone suffices — so a readdir that fails with EACCES means
+    // the screen never saw candidates git will still honour. Swallowing that
+    // as "no linked worktrees" answers clean on a config it did not read.
+    const admin = join(dir, '.git', 'worktrees');
+    mkdirSync(admin, { recursive: true });
+    chmodSync(admin, 0o111);
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.unreadable).toBe(admin);
+  });
+
+  it('stays clean on a repo with no linked worktrees at all', () => {
+    // ENOENT on the admin dir is the ordinary case and must NOT refuse.
+    expect(existsSync(join(dir, '.git', 'worktrees'))).toBe(false);
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen).toEqual({ keys: [], total: 0, unreadable: null });
+  });
+
+  it('refuses a candidate git cannot parse', () => {
+    // Opens fine, and git then dies on it with exit 128 — not the exit 1 that
+    // means "no key matched".
+    writeFileSync(
+      join(dir, '.git', 'config.worktree'),
+      '[filter "evil"\n\tsmudge = cat\n',
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.unreadable).toBe(join(dir, '.git', 'config.worktree'));
+  });
+});
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index 52c293d3e59..ca586f37600 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -580,6 +580,20 @@ function trackedIgnoreSources(
  */
 export const RESIDUE_PATH_CAP = 12;
 
+/**
+ * Caps for the config screen, which reads attacker-writable files.
+ *
+ * `MAX_SCREEN_KEYS` bounds what a refusal REPORTS — the message names the
+ * first keys and the caller says how many more; an unbounded join is its own
+ * denial-of-service, the same rule the `unreadable` field follows by naming
+ * one file rather than a list. `MAX_SCREEN_CANDIDATES` bounds what the screen
+ * READS: one synchronous spawn per registered worktree, on a directory a
+ * probe can fill. Both fail closed — past the candidate bound the screen
+ * refuses, because skipping would let filler entries hide a real filter.
+ */
+export const MAX_SCREEN_KEYS = RESIDUE_PATH_CAP;
+export const MAX_SCREEN_CANDIDATES = 256;
+
 /**
  * The repo-local `filter.<name>` COMMANDS, when any are defined.
  *
@@ -594,7 +608,7 @@ export const RESIDUE_PATH_CAP = 12;
  * a completeness map — widening the screen means adding a call site, and the
  * count above is what has one.
  *
- * The planting surface is two plain writes a probe can
+ * The planting surface this screen covers is two plain writes a probe can
  * make into the COMMON dir this command's report calls shared:
  * `git config filter.evil.smudge CMD` and one line appended to
  * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never
@@ -603,14 +617,25 @@ export const RESIDUE_PATH_CAP = 12;
  * planted by reviewing a malicious PR, measured live. The two local config
  * files are checked with `--file` rather than merged config because filters
  * in the user's global config (git-lfs is the common one) are the user's own
- * contract, exactly like any git command they run — while a probe's planting
- * surface is the repo-local files. The state cannot be told apart from a
+ * contract, exactly like any git command they run. That is a deliberate
+ * TRADE, not a claim that repo-local files are all a probe can reach: probe
+ * code runs as the user, so `git config --global` is open to it, and a filter
+ * planted there is read by every checkout here and never seen by this screen.
+ * Refusing on merged config is not the answer — `git lfs install` writes
+ * `filter.lfs.clean` globally, and refusing on that is permanent refusal for
+ * every contributor who has git-lfs. The state cannot be told apart from a
  * filter the user set deliberately, and cannot be safely wiped, so a hit is a
  * refusal upstream, not a cleanup here.
  */
 export interface LocalFilterScreen {
-  /** The repo-local `filter.<name>` command keys found, when any are defined. */
+  /**
+   * The repo-local `filter.<name>` command keys found, capped at
+   * `MAX_SCREEN_KEYS`. `total` says how many there were before the cap, so a
+   * refusal can name what it is not showing instead of silently shortening.
+   */
   keys: string[];
+  /** How many distinct keys the screen found, before the cap on `keys`. */
+  total: number;
   /**
    * The first candidate file the screen could not read to completion, when one
    * stopped it — otherwise null.
@@ -635,7 +660,7 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
   if (files.error || files.status !== 0 || typeof files.stdout !== 'string') {
     // Not a usable repository from here: the checkout this screen guards has
     // nothing to run in either, and its own failure is the caller's answer.
-    return { keys: [], unreadable: null };
+    return { keys: [], total: 0, unreadable: null };
   }
   const [commonDir, gitDir] = files.stdout.trim().split('\n');
   const common = resolve(worktree, commonDir);
@@ -650,14 +675,35 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
   // planted there executed during the reset while this function reported the
   // repository clean. The admin directory is one `readdir`, and a filter in
   // any of these is a plant whichever tree carries it.
+  //
+  // Bounded, and fail closed over the bound. Each entry costs one synchronous
+  // `git config` spawn below, and the admin directory is the same never-wiped
+  // surface the filter itself is planted in: M empty `worktrees/<e>/` dirs are
+  // M spawns on EVERY screen call, which is once per mutant. A healthy repo
+  // carries O(shards) entries. Past the cap the screen has not read what it
+  // was asked to read, so it refuses rather than walking a plant's filler —
+  // and refusing beats skipping, which would let filler hide a real filter.
+  let worktreeEntries: string[] | null = null;
   try {
-    for (const entry of readdirSync(join(common, 'worktrees'))) {
-      candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
+    worktreeEntries = readdirSync(join(common, 'worktrees'));
+  } catch (e) {
+    // ENOENT is the ordinary "no linked worktrees registered" — the two
+    // candidates above are all of it. Anything else is a directory this
+    // screen could NOT walk while git still opens each `config.worktree` by
+    // direct path (the search bit alone suffices), so a swallowed EACCES
+    // would answer clean on candidates that were never read. Refuse instead:
+    // this is the directory-level twin of the per-file readability gate below.
+    if ((e as NodeJS.ErrnoException)?.code !== 'ENOENT') {
+      return { keys: [], total: 0, unreadable: join(common, 'worktrees') };
     }
-  } catch {
-    // No linked worktrees registered: the two candidates above are all of it.
   }
-  const found: string[] = [];
+  if (worktreeEntries && worktreeEntries.length > MAX_SCREEN_CANDIDATES) {
+    return { keys: [], total: 0, unreadable: join(common, 'worktrees') };
+  }
+  for (const entry of worktreeEntries ?? []) {
+    candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
+  }
+  const found = new Set<string>();
   let unreadable: string | null = null;
   for (const file of candidates) {
     if (!existsSync(file)) continue;
@@ -713,10 +759,20 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
       const key = line.split(/\s+/)[0];
-      if (key && !found.includes(key)) found.push(key);
+      // A Set, not `Array.includes`: the values here are attacker-written and
+      // the 64 MiB ceiling above admits millions of distinct keys, so a linear
+      // membership test per line is quadratic on exactly the input this screen
+      // exists to survive. (Measured: 100k keys took 144 s as an array scan and
+      // 53 ms as a Set — and the 1 MiB default this raised from used to hide it
+      // by ENOBUFS-killing git and skipping the loop entirely.)
+      if (key) found.add(key);
     }
   }
-  return { keys: found, unreadable };
+  return {
+    keys: [...found].slice(0, MAX_SCREEN_KEYS),
+    total: found.size,
+    unreadable,
+  };
 }
 
 /**
diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts
index 13a451cca6f..49ebac0dd40 100644
--- a/packages/cli/src/commands/review/scratch-tree.ts
+++ b/packages/cli/src/commands/review/scratch-tree.ts
@@ -55,6 +55,7 @@ import {
 } from './lib/paths.js';
 import { shellQuotePath } from './lib/shell-quote.js';
 import {
+  MAX_SCREEN_KEYS,
   RESIDUE_PATH_CAP,
   discardWorktree,
   exposeDependencies,
@@ -435,6 +436,7 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport {
   if (filters.keys.length > 0) {
     return unavailable(
       `the repository's local config defines content filter(s) ${filters.keys
+        .slice(0, MAX_SCREEN_KEYS)
         .map(inertPath)
         .join(', ')} — ` +
         'the checkouts this command runs would EXECUTE them (hooks are disabled, ' +
diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
index 1de3f993ab7..7259642c8c7 100644
--- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
@@ -970,26 +970,13 @@ process.stdout.write(JSON.stringify({
       `#!/usr/bin/env node
 import path from 'node:path';
 const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
-// Same positive-control semantics as the default fake: the injected
-// always-failing test must actually fail, or the run re-classes to
-// control-failed and never reaches the revert this test is about.
-const st = (f) => {
-  try {
-    return fs.readFileSync(f, 'utf8').includes('QWEN-REVIEW-POSITIVE-CONTROL')
-      ? 'failed'
-      : 'passed';
-  } catch {
-    return 'passed';
-  }
-};
 const results = files.map((f) => ({
   name: path.resolve(f),
-  assertionResults: [{ status: st(f) }],
+  assertionResults: [{ status: 'passed' }],
 }));
-const failed = results.filter((r) => r.assertionResults[0].status === 'failed').length;
 process.stdout.write(JSON.stringify({
-  numPassedTests: results.length - failed,
-  numFailedTests: failed,
+  numPassedTests: results.length,
+  numFailedTests: 0,
   testResults: results,
 }));
 `,
diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index b6f311c2f92..5e63d4b85cd 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -716,6 +716,37 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     }
   });
 
+  it('refuses when git cannot PARSE a candidate config — not "clean"', () => {
+    // The readability gate above answers "can this process open it"; this is
+    // the other half — a file that opens fine and that git then dies on. A
+    // malformed section header exits 128 (not the 1 that means "no key
+    // matched"), so a screen that only distinguished 0 from non-0 would skip
+    // the file and report the repository clean on a config it never read.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-malformed-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      asCheckout(dir);
+      // Unclosed section header — a regular, readable file git refuses to parse.
+      writeFileSync(
+        join(dir, '.git', 'config.worktree'),
+        '[filter "evil"\n\tsmudge = cat\n',
+      );
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('could not be read to the end');
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
   it('refuses to run when the index hides a tracked file from the restore', () => {
     // `checkout --force` SILENTLY skips a file carrying skip-worktree, and
     // `clean` never touches a tracked file — so a bit the guest suite sets with
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index f76be0f4a76..a7c19db8d4d 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -61,7 +61,7 @@ import {
 import { createRequire } from 'node:module';
 import { dirname, join, isAbsolute, resolve, sep } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
-import { probeWorktreePath } from './lib/paths.js';
+import { inertPath, probeWorktreePath } from './lib/paths.js';
 // `discardWorktree` moved to `lib/worktree.ts` when `base-tree` needed the same
 // stale-sweep-then-remove step (its rationale lives there, with the helper), and
 // `exposeDependencies` followed it when `scratch-tree` needed the same
@@ -82,6 +82,7 @@ import {
   type ContainerRuntime,
 } from './lib/sandboxed-exec.js';
 import {
+  type LocalFilterScreen,
   discardWorktree,
   exposeDependencies,
   localFilterCommands,
@@ -1571,6 +1572,23 @@ function probeContainer(
   };
 }
 
+/**
+ * The screened keys, flattened and bounded, for a refusal message.
+ *
+ * Both halves matter: `inertPath` because a config subsection name legally
+ * carries control and format characters that `--get-regexp` prints verbatim,
+ * and these strings land in the agent-facing report; the cap because the list
+ * comes from an attacker-writable file and an unbounded join is its own
+ * denial-of-service. The count is kept so a truncated list still says how much
+ * it is not showing.
+ */
+function screenKeyList(screen: LocalFilterScreen): string {
+  const shown = screen.keys.map(inertPath).join(', ');
+  return screen.total > screen.keys.length
+    ? `${shown} (capped enumeration: ${screen.keys.length} shown of ${screen.total})`
+    : shown;
+}
+
 function restoreProbeTreeTracked(probeTree: string): string | null {
   if (!existsSync(join(probeTree, '.git'))) {
     return `${probeTree} carries no .git, so there is no commit to put it back to`;
@@ -1654,10 +1672,10 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   // failure as a tripwire that fires on every healthy run.
   const filters = localFilterCommands(probeTree);
   if (filters.unreadable) {
-    return `the repository's local config file ${filters.unreadable} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`;
+    return `the repository's local config file ${inertPath(filters.unreadable)} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`;
   }
   if (filters.keys.length > 0) {
-    return `the repository's local config defines content filter(s) ${filters.keys.join(', ')}, which this tree's restore would EXECUTE`;
+    return `the repository's local config defines content filter(s) ${screenKeyList(filters)}, which this tree's restore would EXECUTE`;
   }
   // `core.fsmonitor` runs a command on BOTH of these, and the config that sets
   // it lives in the tree they are cleaning: the residue probe empties it for
@@ -2997,7 +3015,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         const revertFilters = localFilterCommands(probeTree);
         if (revertFilters.unreadable) {
           throw new Error(
-            `the repository's local config file ${revertFilters.unreadable} ` +
+            `the repository's local config file ${inertPath(revertFilters.unreadable)} ` +
               'could not be read to the end, so whether this revert would ' +
               'EXECUTE a content filter is unknown',
           );
@@ -3005,7 +3023,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         if (revertFilters.keys.length > 0) {
           throw new Error(
             "the repository's local config defines content filter(s) " +
-              `${revertFilters.keys.join(', ')}, which this revert's checkout ` +
+              `${screenKeyList(revertFilters)}, which this revert's checkout ` +
               'would EXECUTE',
           );
         }

From a6f174816fd8c6d6ecc21f48621e8d303f418f62 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Sun, 30 Aug 2026 19:14:53 +0800
Subject: [PATCH 05/15] test(review): pin the flattening of control characters
 in a named filter key
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Removing the inertPath calls left every suite green — the round-2 finding's
own witness requirement. A key planted with ESC (Cc) and a right-to-left
override (Cf) inside its subsection name now asserts the flattened form in the
refusal detail, and goes red when the flattening is dropped.
---
 .../src/commands/review/test-efficacy.test.ts | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index 5e63d4b85cd..4353e48d6ab 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -716,6 +716,40 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     }
   });
 
+  it('flattens control characters in the filter key it names', () => {
+    // A config subsection name legally carries control and format characters,
+    // and `--get-regexp` prints them verbatim. These detail strings reach the
+    // agent-facing report through JSON.stringify, which escapes Cc but NOT Cf
+    // (bidi overrides), Zl/Zp, or backticks — so the flattening has to happen
+    // here, through the same helper scratch-tree already uses.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-inert-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      asCheckout(dir);
+      // ESC (Cc) and a right-to-left override (Cf) inside the subsection name.
+      execFileSync(
+        'git',
+        ['config', 'filter.ev\u001bil\u202eX.smudge', 'cat'],
+        { cwd: dir },
+      );
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('filter.ev il X.smudge');
+      expect(r.detail).not.toContain('\u001b');
+      expect(r.detail).not.toContain('\u202e');
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
   it('refuses when git cannot PARSE a candidate config — not "clean"', () => {
     // The readability gate above answers "can this process open it"; this is
     // the other half — a file that opens fine and that git then dies on. A

From 27ef34fe009d259bab6cb675f67c23af317a7d84 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Mon, 31 Aug 2026 02:03:31 +0800
Subject: [PATCH 06/15] fix(review): parse paths one at a time, name whole
 keys, and split the two stops
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round 3. Two fail-opens the screen carried from the start, plus three defects
in my own round-2 work.

**A combined rev-parse cannot be split.** `--git-common-dir --git-dir` in one
invocation, parsed with `.trim().split('\n')`, corrupts both values when the
repository path holds a newline — POSIX-legal, and git handles it fine. A
linked worktree under such a directory parses to four records instead of two,
`commonDir` takes the head of the path and `gitDir` its tail, every candidate
resolves to nothing, the admin readdir hits ENOENT — the branch this code
calls ordinary — and the screen answers CLEAN over a live plant. Measured:
`{"keys":[],...}` against a planted filter that the same repo at a plain path
reports as `{"keys":["filter.evil.smudge"],"total":1}`. Now one invocation per
value, stripping only git's terminal delimiter — the shape `worktreeResidue`'s
`discover` already uses in this file, for this exact reason, with a comment
saying so.

**A key is not the first whitespace-delimited token.** Config subsection names
may hold spaces, and `--get-regexp` prints `key value` on one line, so a
planted `filter.evil name.smudge` was reported as `filter.evil`. The refusal
still fired, but it named a key that does not exist and prescribed an unset
that exits 5 while the plant stood; two keys sharing a truncated prefix also
collapsed into one, understating `total`. `--name-only` (before the pattern —
after it, git silently prints nothing and exits 1) makes each line the whole
key.

**The over-cap refusal wore the unreadable message.** Both reached "could not
be read to the end … fix or remove that file" for a directory that was read
perfectly well, and following that advice on `<common>/worktrees` deregisters
every legitimate linked worktree. `LocalFilterScreen` now carries `stopped`,
and the over-cap case says to prune instead.

**The EACCES fixture had no platform guard** — `chmod 0o111` restricts nothing
on Windows and nothing for root, so the test that pins the round-2 readdir fix
would have been red on the nightly Windows lane and in any root run. It now
carries the same `skipIf` the other permission-bit fixture in this file uses.

**And the two refusal surfaces had drifted apart again.** scratch-tree's
`.slice(0, MAX_SCREEN_KEYS)` was a no-op over an already-capped list and it
never read `total`, so the same 13-key plant read "12 keys" there and "(capped
enumeration: 12 shown of 13)" one file over. `screenKeyList` and
`screenStopDetail` are hoisted beside the producer and both consumers use
them.
---
 .../src/commands/review/lib/worktree.test.ts  | 97 ++++++++++++++++--
 .../cli/src/commands/review/lib/worktree.ts   | 99 ++++++++++++++++---
 .../cli/src/commands/review/scratch-tree.ts   | 19 ++--
 .../src/commands/review/test-efficacy.test.ts | 34 ++++++-
 .../cli/src/commands/review/test-efficacy.ts  | 29 ++----
 5 files changed, 223 insertions(+), 55 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index 78196ca377b..d91fb035680 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -1957,20 +1957,92 @@ describe('localFilterCommands', () => {
 
     expect(screen.unreadable).toBe(join(dir, '.git', 'worktrees'));
     expect(screen.keys).toEqual([]);
+    // Read fine, just too many — a different remedy from an unreadable file.
+    expect(screen.stopped).toBe('over-cap');
   });
 
-  it('refuses when the worktrees admin dir cannot be read', () => {
-    // git opens each `worktrees/<e>/config.worktree` by direct path — the
-    // search bit alone suffices — so a readdir that fails with EACCES means
-    // the screen never saw candidates git will still honour. Swallowing that
-    // as "no linked worktrees" answers clean on a config it did not read.
-    const admin = join(dir, '.git', 'worktrees');
-    mkdirSync(admin, { recursive: true });
-    chmodSync(admin, 0o111);
+  // POSIX DAC bits only: Windows does not restrict listing through chmod, and
+  // root bypasses the mode entirely — the fixture would be red on both rather
+  // than pinning anything. Same guard as the other permission-bit fixture in
+  // this file.
+  it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)(
+    'refuses when the worktrees admin dir cannot be read',
+    () => {
+      // git opens each `worktrees/<e>/config.worktree` by direct path — the
+      // search bit alone suffices — so a readdir that fails with EACCES means
+      // the screen never saw candidates git will still honour. Swallowing that
+      // as "no linked worktrees" answers clean on a config it did not read.
+      const admin = join(dir, '.git', 'worktrees');
+      mkdirSync(admin, { recursive: true });
+      chmodSync(admin, 0o111);
+
+      const screen = localFilterCommands(dir);
+
+      expect(screen.unreadable).toBe(admin);
+      // The two refusal causes must stay distinguishable: this one is a file
+      // to fix, the over-cap one is a directory to prune.
+      expect(screen.stopped).toBe('unreadable');
+    },
+  );
+
+  it('names a key whose subsection carries whitespace, intact', () => {
+    // A config subsection name may legally hold spaces, and `--get-regexp`
+    // prints `key value` on one line — so splitting on whitespace named
+    // `filter.evil` for a planted `filter.evil name.smudge`. The refusal then
+    // prescribed an unset that exits 5 while the plant stood, and two distinct
+    // keys sharing a truncated prefix collapsed into one, understating `total`.
+    execFileSync('git', ['config', 'filter.evil name.smudge', 'CMD'], {
+      cwd: dir,
+    });
+    execFileSync('git', ['config', 'filter.evil other.smudge', 'CMD2'], {
+      cwd: dir,
+    });
 
     const screen = localFilterCommands(dir);
 
-    expect(screen.unreadable).toBe(admin);
+    expect(screen.keys.sort()).toEqual([
+      'filter.evil name.smudge',
+      'filter.evil other.smudge',
+    ]);
+    expect(screen.total).toBe(2);
+  });
+
+  it('sees a filter through a repo path that carries a newline', () => {
+    // The screened trees are linked worktrees inheriting the user's repo path.
+    // A combined `rev-parse --git-common-dir --git-dir` answer splits into four
+    // records under such a path instead of two, every candidate resolves to
+    // nothing, the admin readdir hits ENOENT — the branch treated as ordinary —
+    // and the screen answers clean over a live plant.
+    const base = mkdtempSync(join(tmpdir(), 'qwen-nl-'));
+    try {
+      const holder = join(base, 'nl\ndir');
+      mkdirSync(holder, { recursive: true });
+      const main = join(holder, 'main');
+      mkdirSync(main);
+      initRepo(main);
+      writeFileSync(join(main, 'a.ts'), 'x\n');
+      execFileSync('git', ['add', '-A'], { cwd: main });
+      execFileSync(
+        'git',
+        ['-c', 'user.email=t@t.t', '-c', 'user.name=t', 'commit', '-qm', 'i'],
+        { cwd: main },
+      );
+      execFileSync('git', ['config', 'filter.evil.smudge', 'cat'], {
+        cwd: main,
+      });
+      const linked = join(holder, 'wt');
+      execFileSync(
+        'git',
+        ['worktree', 'add', '-q', '--detach', linked, 'HEAD'],
+        { cwd: main },
+      );
+
+      const screen = localFilterCommands(linked);
+
+      expect(screen.keys).toEqual(['filter.evil.smudge']);
+    } finally {
+      rmSync(base, { recursive: true, force: true });
+    }
   });
 
   it('stays clean on a repo with no linked worktrees at all', () => {
@@ -1979,7 +2051,12 @@ describe('localFilterCommands', () => {
 
     const screen = localFilterCommands(dir);
 
-    expect(screen).toEqual({ keys: [], total: 0, unreadable: null });
+    expect(screen).toEqual({
+      keys: [],
+      total: 0,
+      unreadable: null,
+      stopped: null,
+    });
   });
 
   it('refuses a candidate git cannot parse', () => {
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index ca586f37600..1d58e2a0649 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -36,6 +36,7 @@ import {
 } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { basename, dirname, isAbsolute, join, resolve, sep } from 'node:path';
+import { inertPath } from './paths.js';
 import { readWorkspacePackages } from './workspaces.js';
 
 export type SweepResult = ReturnType<typeof spawnSync>;
@@ -636,6 +637,16 @@ export interface LocalFilterScreen {
   keys: string[];
   /** How many distinct keys the screen found, before the cap on `keys`. */
   total: number;
+  /**
+   * Why the screen stopped, when it did.
+   *
+   * `unreadable` and `over-cap` both refuse, but they call for opposite
+   * remedies: an unreadable candidate is a file to fix or remove, while an
+   * over-cap admin directory was read perfectly well and holds more
+   * registered worktrees than the bound — "remove that file" there would
+   * deregister every legitimate linked worktree, the pipeline's own included.
+   */
+  stopped: 'unreadable' | 'over-cap' | null;
   /**
    * The first candidate file the screen could not read to completion, when one
    * stopped it — otherwise null.
@@ -651,18 +662,65 @@ export interface LocalFilterScreen {
   unreadable: string | null;
 }
 
+/**
+ * The screened keys, flattened and counted, for a refusal message.
+ *
+ * `keys` arrives already capped by the producer, so this never re-slices; it
+ * reports the shortfall instead. Both halves matter: `inertPath` because a
+ * config subsection name legally carries control and format characters that
+ * `--get-regexp` prints verbatim, and these strings land in the agent-facing
+ * report; the count because a message naming 12 of 13 keys sends a user to
+ * remove the 12 and be refused again on the one it never mentioned.
+ */
+export function screenKeyList(screen: LocalFilterScreen): string {
+  const shown = screen.keys.map(inertPath).join(', ');
+  return screen.total > screen.keys.length
+    ? `${shown} (capped enumeration: ${screen.keys.length} shown of ${screen.total})`
+    : shown;
+}
+
+/**
+ * Why a screen that stopped could not finish, phrased for the reader who has
+ * to act on it.
+ *
+ * The two causes need opposite remedies, so they must not share a sentence:
+ * an unreadable candidate is a file to fix or remove, while an over-cap admin
+ * directory was read perfectly well — telling that reader to "remove that
+ * file" would deregister every legitimate linked worktree they have.
+ */
+export function screenStopDetail(screen: LocalFilterScreen): string {
+  const where = inertPath(screen.unreadable ?? '');
+  return screen.stopped === 'over-cap'
+    ? `more than ${MAX_SCREEN_CANDIDATES} worktrees are registered under ${where} — more than this screen will walk, so it cannot tell whether one of them defines a content filter. Prune stale ones (\`git worktree prune\`) and re-run`
+    : `${where} could not be read to the end`;
+}
+
 export function localFilterCommands(worktree: string): LocalFilterScreen {
-  const files = spawnSync(
-    'git',
-    ['rev-parse', '--git-common-dir', '--git-dir'],
-    { cwd: worktree, encoding: 'utf8', env: sanitizedGitEnv() },
-  );
-  if (files.error || files.status !== 0 || typeof files.stdout !== 'string') {
+  // One invocation per value, the shape `worktreeResidue`'s `discover` already
+  // uses in this file and for the same reason: the answers are filesystem
+  // paths, a POSIX path may carry a newline, and a combined newline-delimited
+  // answer cannot then be split unambiguously. A linked worktree under such a
+  // directory parses to four records instead of two, `commonDir` gets the head
+  // of the path and `gitDir` its tail, every candidate resolves to nothing,
+  // the admin readdir hits ENOENT — the branch this code calls ordinary — and
+  // the screen answers CLEAN over a live plant. Measured with exactly such a
+  // directory. Strip only git's terminal delimiter; every other byte is path.
+  const askPath = (flag: string): string | null => {
+    const r = spawnSync('git', ['rev-parse', flag], {
+      cwd: worktree,
+      encoding: 'utf8',
+      env: sanitizedGitEnv(),
+    });
+    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') return null;
+    return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
+  };
+  const commonDir = askPath('--git-common-dir');
+  const gitDir = askPath('--git-dir');
+  if (commonDir === null || gitDir === null) {
     // Not a usable repository from here: the checkout this screen guards has
     // nothing to run in either, and its own failure is the caller's answer.
-    return { keys: [], total: 0, unreadable: null };
+    return { keys: [], total: 0, unreadable: null, stopped: null };
   }
-  const [commonDir, gitDir] = files.stdout.trim().split('\n');
   const common = resolve(worktree, commonDir);
   const candidates = [
     join(common, 'config'),
@@ -694,11 +752,21 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     // would answer clean on candidates that were never read. Refuse instead:
     // this is the directory-level twin of the per-file readability gate below.
     if ((e as NodeJS.ErrnoException)?.code !== 'ENOENT') {
-      return { keys: [], total: 0, unreadable: join(common, 'worktrees') };
+      return {
+        keys: [],
+        total: 0,
+        unreadable: join(common, 'worktrees'),
+        stopped: 'unreadable',
+      };
     }
   }
   if (worktreeEntries && worktreeEntries.length > MAX_SCREEN_CANDIDATES) {
-    return { keys: [], total: 0, unreadable: join(common, 'worktrees') };
+    return {
+      keys: [],
+      total: 0,
+      unreadable: join(common, 'worktrees'),
+      stopped: 'over-cap',
+    };
   }
   for (const entry of worktreeEntries ?? []) {
     candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
@@ -731,6 +799,14 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
         'config',
         '--file',
         file,
+        // BEFORE the pattern: `--name-only` after it silently prints nothing
+        // and exits 1 even with live keys. With it, each line is the whole key
+        // and nothing has to be parsed out of a `key value` pair — a config
+        // subsection name may legally carry whitespace, and splitting on it
+        // named `filter.evil` for a planted `filter.evil name.smudge`, so the
+        // refusal prescribed an unset that exits 5 while the plant stood. Key
+        // names cannot contain newlines, so line-per-key is unambiguous.
+        '--name-only',
         '--get-regexp',
         // `process` beside the pair: it is the third executable key (a
         // long-running filter git speaks a protocol to), and enumerating two
@@ -758,7 +834,7 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     }
     if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
-      const key = line.split(/\s+/)[0];
+      const key = line;
       // A Set, not `Array.includes`: the values here are attacker-written and
       // the 64 MiB ceiling above admits millions of distinct keys, so a linear
       // membership test per line is quadratic on exactly the input this screen
@@ -772,6 +848,7 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     keys: [...found].slice(0, MAX_SCREEN_KEYS),
     total: found.size,
     unreadable,
+    stopped: unreadable === null ? null : 'unreadable',
   };
 }
 
diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts
index 49ebac0dd40..248b0d13bb4 100644
--- a/packages/cli/src/commands/review/scratch-tree.ts
+++ b/packages/cli/src/commands/review/scratch-tree.ts
@@ -55,13 +55,14 @@ import {
 } from './lib/paths.js';
 import { shellQuotePath } from './lib/shell-quote.js';
 import {
-  MAX_SCREEN_KEYS,
   RESIDUE_PATH_CAP,
   discardWorktree,
   exposeDependencies,
   localFilterCommands,
   redirectedAncestor,
   sanitizedGitEnv,
+  screenKeyList,
+  screenStopDetail,
   worktreeCreateFailureDetail,
   worktreeResidue,
   type DependencyFarm,
@@ -425,20 +426,18 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport {
   // BEFORE any checkout runs — the reuse path's reset and the rebuild path's
   // `worktree add` both execute configured content filters.
   const filters = localFilterCommands(worktree);
-  if (filters.unreadable) {
+  if (filters.stopped) {
     return unavailable(
-      `the repository's local config file ${inertPath(filters.unreadable)} ` +
-        'could not be read to the end, so this command cannot tell whether it ' +
-        'defines a content filter the checkouts below would EXECUTE. A screen ' +
-        'that did not finish is not a clean result — fix or remove that file.',
+      `the screen could not clear this repository: ${screenStopDetail(filters)}. ` +
+        'A screen that did not finish is not a clean result — the checkouts ' +
+        'below would EXECUTE any content filter it failed to see.',
     );
   }
   if (filters.keys.length > 0) {
     return unavailable(
-      `the repository's local config defines content filter(s) ${filters.keys
-        .slice(0, MAX_SCREEN_KEYS)
-        .map(inertPath)
-        .join(', ')} — ` +
+      `the repository's local config defines content filter(s) ${screenKeyList(
+        filters,
+      )} — ` +
         'the checkouts this command runs would EXECUTE them (hooks are disabled, ' +
         'filters are config-driven), and two plain writes into the common dir are ' +
         'enough to plant both the filter and the attributes that select it. Remove ' +
diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index 4353e48d6ab..95929325295 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -34,7 +34,7 @@ import {
   committedSymlinkProbes,
 } from './test-efficacy.js';
 import { isolateHostGitConfig } from './lib/test-utils.js';
-import { sanitizedGitEnv } from './lib/worktree.js';
+import { MAX_SCREEN_KEYS, sanitizedGitEnv } from './lib/worktree.js';
 import {
   mkdtempSync,
   mkdirSync,
@@ -716,6 +716,38 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     }
   });
 
+  it('says the enumeration was capped when more keys were planted than shown', () => {
+    // The cap exists so an attacker-written key list cannot become the whole
+    // refusal string; the count exists so the message does not send a user to
+    // remove the keys it named and leave the rest standing.
+    const dir = mkdtempSync(join(tmpdir(), 'qwen-capmsg-'));
+    const isolation = isolateHostGitConfig();
+    try {
+      writeFileSync(join(dir, 'a.ts'), 'gone.clear();\n');
+      asCheckout(dir);
+      let cfg = '';
+      for (let i = 0; i < MAX_SCREEN_KEYS + 1; i++) {
+        cfg += `[filter "evil${i}"]\n\tsmudge = cat\n`;
+      }
+      appendFileSync(join(dir, '.git', 'config'), cfg);
+
+      const r = runOneMutant(
+        dir,
+        { file: 'a.ts', line: 1, statement: 'gone.clear();' },
+        ['a.test.ts'],
+      );
+
+      expect(r.verdict).toBe('inconclusive');
+      expect(r.detail).toContain('capped enumeration');
+      expect(r.detail).toContain(
+        `${MAX_SCREEN_KEYS} shown of ${MAX_SCREEN_KEYS + 1}`,
+      );
+    } finally {
+      isolation.dispose();
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
   it('flattens control characters in the filter key it names', () => {
     // A config subsection name legally carries control and format characters,
     // and `--get-regexp` prints them verbatim. These detail strings reach the
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index a7c19db8d4d..24cef3347f6 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -61,7 +61,7 @@ import {
 import { createRequire } from 'node:module';
 import { dirname, join, isAbsolute, resolve, sep } from 'node:path';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
-import { inertPath, probeWorktreePath } from './lib/paths.js';
+import { probeWorktreePath } from './lib/paths.js';
 // `discardWorktree` moved to `lib/worktree.ts` when `base-tree` needed the same
 // stale-sweep-then-remove step (its rationale lives there, with the helper), and
 // `exposeDependencies` followed it when `scratch-tree` needed the same
@@ -82,11 +82,12 @@ import {
   type ContainerRuntime,
 } from './lib/sandboxed-exec.js';
 import {
-  type LocalFilterScreen,
   discardWorktree,
   exposeDependencies,
   localFilterCommands,
   redirectedAncestor,
+  screenKeyList,
+  screenStopDetail,
   sanitizedGitEnv,
   worktreeCreateFailureDetail,
   type SweepResult,
@@ -1572,23 +1573,6 @@ function probeContainer(
   };
 }
 
-/**
- * The screened keys, flattened and bounded, for a refusal message.
- *
- * Both halves matter: `inertPath` because a config subsection name legally
- * carries control and format characters that `--get-regexp` prints verbatim,
- * and these strings land in the agent-facing report; the cap because the list
- * comes from an attacker-writable file and an unbounded join is its own
- * denial-of-service. The count is kept so a truncated list still says how much
- * it is not showing.
- */
-function screenKeyList(screen: LocalFilterScreen): string {
-  const shown = screen.keys.map(inertPath).join(', ');
-  return screen.total > screen.keys.length
-    ? `${shown} (capped enumeration: ${screen.keys.length} shown of ${screen.total})`
-    : shown;
-}
-
 function restoreProbeTreeTracked(probeTree: string): string | null {
   if (!existsSync(join(probeTree, '.git'))) {
     return `${probeTree} carries no .git, so there is no commit to put it back to`;
@@ -1672,7 +1656,7 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   // failure as a tripwire that fires on every healthy run.
   const filters = localFilterCommands(probeTree);
   if (filters.unreadable) {
-    return `the repository's local config file ${inertPath(filters.unreadable)} could not be read to the end, so whether this tree's restore would EXECUTE a content filter is unknown`;
+    return `the screen could not clear this tree's restore: ${screenStopDetail(filters)}`;
   }
   if (filters.keys.length > 0) {
     return `the repository's local config defines content filter(s) ${screenKeyList(filters)}, which this tree's restore would EXECUTE`;
@@ -3015,9 +2999,8 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         const revertFilters = localFilterCommands(probeTree);
         if (revertFilters.unreadable) {
           throw new Error(
-            `the repository's local config file ${inertPath(revertFilters.unreadable)} ` +
-              'could not be read to the end, so whether this revert would ' +
-              'EXECUTE a content filter is unknown',
+            'the screen could not clear this revert: ' +
+              screenStopDetail(revertFilters),
           );
         }
         if (revertFilters.keys.length > 0) {

From a6b21476ed54045a17f2e569bab88cc3c4798228 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Mon, 31 Aug 2026 02:07:19 +0800
Subject: [PATCH 07/15] test(review): pin the capped count on the scratch-tree
 refusal too
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Reverting scratch-tree to the silent join left its suite green — the two
refusal surfaces this PR unified had no shared witness. A plant of
MAX_SCREEN_KEYS + 1 keys now asserts the note carries 'capped enumeration' and
the shown/total counts, and goes red against the join.
---
 .../src/commands/review/scratch-tree.test.ts   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/packages/cli/src/commands/review/scratch-tree.test.ts b/packages/cli/src/commands/review/scratch-tree.test.ts
index 74e0b6e0169..ee9de54b882 100644
--- a/packages/cli/src/commands/review/scratch-tree.test.ts
+++ b/packages/cli/src/commands/review/scratch-tree.test.ts
@@ -33,6 +33,7 @@ import {
   symlinkSync,
   writeFileSync,
 } from 'node:fs';
+import { MAX_SCREEN_KEYS } from './lib/worktree.js';
 import { tmpdir } from 'node:os';
 import { basename, dirname, join } from 'node:path';
 import yargs, { type Argv } from 'yargs';
@@ -129,6 +130,23 @@ describe('runScratchTree', () => {
     expect(run().available).toBe(true);
   });
 
+  it('tells the user how many filter keys it did NOT name', () => {
+    // The cap keeps an attacker-written key list out of the refusal string;
+    // the count keeps the refusal actionable. Naming 12 of 13 sends a user to
+    // remove the 12 and be refused again on the one never mentioned — and the
+    // two refusal surfaces in this pipeline must report the state the same way.
+    for (let i = 0; i < MAX_SCREEN_KEYS + 1; i++) {
+      git(worktree, 'config', `filter.evil${i}.smudge`, 'cat');
+    }
+    writeFileSync(join(worktree, 'a.ts'), 'dirty\n');
+
+    const r = run();
+
+    expect(r.available).toBe(false);
+    expect(r.note).toContain('capped enumeration');
+    expect(r.note).toContain(`${MAX_SCREEN_KEYS} shown of ${MAX_SCREEN_KEYS + 1}`);
+  });
+
   it("screens ANOTHER worktree's per-worktree config, not just this one's", () => {
     // The screen runs against the review worktree, but the checkout it
     // authorises runs in the SCRATCH tree — whose own

From d2d018e8cac6fc34aa1bdafa1e9de44ec08ded95 Mon Sep 17 00:00:00 2001
From: wenshao <szujobs@gmail.com>
Date: Mon, 31 Aug 2026 23:56:24 +0800
Subject: [PATCH 08/15] fix(review): expand config includes in the filter
 screen and harden the revert checkout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- read screened config with `--file … --includes` so a filter hidden one
  include-hop behind a screened candidate is seen (git's checkout follows the
  include; the screen must too)
- drop the now-moot 64 MiB `maxBuffer` on the screen spawn: `--name-only`
  bounds output by key name, and a pathological key count fails closed via the
  ENOBUFS gate
- neutralise `core.hooksPath`/`core.fsmonitor` at the revert checkout, matching
  the restore, via a shared `CHECKOUT_INERT`
- dedupe the newline-safe rev-parse path parse into one `revParsePath` helper
- add tests: the `--includes` witness, the `runScratchTree` stopped-screen
  refusal, and both `screenStopDetail` arms; reframe the padded-value witness
  to pin `--name-only`
---
 .../src/commands/review/lib/worktree.test.ts  |  63 ++++++++-
 .../cli/src/commands/review/lib/worktree.ts   | 130 ++++++++++--------
 .../src/commands/review/scratch-tree.test.ts  |  26 +++-
 .../review/test-efficacy.integration.test.ts  |  50 +++++++
 .../src/commands/review/test-efficacy.test.ts |  15 +-
 .../cli/src/commands/review/test-efficacy.ts  |  42 ++++--
 6 files changed, 247 insertions(+), 79 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index d91fb035680..f1062d6fb72 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -38,6 +38,7 @@ import {
   exposeDependencies,
   localFilterCommands,
   sanitizedGitEnv,
+  screenStopDetail,
   worktreeCreateFailureDetail,
   worktreeResidue,
 } from './worktree.js';
@@ -1943,7 +1944,7 @@ describe('localFilterCommands', () => {
     expect(screen.keys[0]).toMatch(/^filter\.evil\d+\.smudge$/);
   });
 
-  it('refuses past the candidate cap instead of walking a plant\'s filler', () => {
+  it("refuses past the candidate cap instead of walking a plant's filler", () => {
     // Each entry costs one synchronous `git config` spawn, on every screen
     // call — once per mutant. Skipping past the bound would let filler hide a
     // real filter behind it, so the screen refuses.
@@ -2071,4 +2072,64 @@ describe('localFilterCommands', () => {
 
     expect(screen.unreadable).toBe(join(dir, '.git', 'config.worktree'));
   });
+
+  it('follows an include directive the checkout would follow', () => {
+    // `git config --file` does NOT expand `include.path` / `includeIf` on its
+    // own — git's documented default for a single-file read — while every
+    // checkout this screen authorises reads MERGED config, which does. A filter
+    // one include-hop behind a screened candidate is invisible to a screen that
+    // omits `--includes` and executed by the checkout. The screen passes
+    // `--includes`, so it sees exactly what the checkout will.
+    writeFileSync(
+      join(dir, '.git', 'evil.inc'),
+      '[filter "evil"]\n\tsmudge = cat\n',
+    );
+    appendFileSync(
+      join(dir, '.git', 'config'),
+      '[include]\n\tpath = evil.inc\n',
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.keys).toEqual(['filter.evil.smudge']);
+    expect(screen.total).toBe(1);
+    expect(screen.unreadable).toBeNull();
+  });
+});
+
+describe('screenStopDetail', () => {
+  // The two refusal causes call for OPPOSITE remedies, so they must not share a
+  // sentence — an unreadable candidate is a file to fix or remove, while an
+  // over-cap admin directory was read perfectly well and "remove that file"
+  // there would deregister every legitimate linked worktree. Each arm is pinned
+  // directly: the producer's classification is tested elsewhere, but the
+  // rendered remedy a reader acts on is only here.
+  it('names the prune remedy for an over-cap directory, not a file to remove', () => {
+    const detail = screenStopDetail({
+      keys: [],
+      total: 0,
+      unreadable: '/repo/.git/worktrees',
+      stopped: 'over-cap',
+    });
+
+    expect(detail).toContain('git worktree prune');
+    expect(detail).toContain('registered under');
+    expect(detail).toContain(String(MAX_SCREEN_CANDIDATES));
+    expect(detail).toContain('/repo/.git/worktrees');
+    // Must NOT tell the reader the directory could not be read — it was.
+    expect(detail).not.toContain('could not be read to the end');
+  });
+
+  it('names the unreadable file plainly, without the prune remedy', () => {
+    const detail = screenStopDetail({
+      keys: [],
+      total: 0,
+      unreadable: '/repo/.git/config.worktree',
+      stopped: 'unreadable',
+    });
+
+    expect(detail).toContain('/repo/.git/config.worktree');
+    expect(detail).toContain('could not be read to the end');
+    expect(detail).not.toContain('git worktree prune');
+  });
 });
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index 1d58e2a0649..8ce6545b00f 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -615,11 +615,15 @@ export const MAX_SCREEN_CANDIDATES = 256;
  * `$(git rev-parse --git-path info/attributes)`. discard and cleanup never
  * wipe the common dir, so a filter planted while reviewing one PR fires on
  * every later matching checkout of the user's OWN repository — persistence
- * planted by reviewing a malicious PR, measured live. The two local config
- * files are checked with `--file` rather than merged config because filters
+ * planted by reviewing a malicious PR, measured live. The candidate config
+ * files are read with `--file … --includes` rather than merged config: filters
  * in the user's global config (git-lfs is the common one) are the user's own
- * contract, exactly like any git command they run. That is a deliberate
- * TRADE, not a claim that repo-local files are all a probe can reach: probe
+ * contract, exactly like any git command they run, so global config stays out
+ * of scope — but `--includes` follows the `include.path` / `includeIf` chain a
+ * screened file itself names, because the checkout does, and a filter hidden
+ * one include-hop behind a screened candidate would otherwise be invisible to
+ * the screen and executed by the checkout. Screening repo-local-only is a
+ * deliberate TRADE, not a claim that repo-local files are all a probe can reach: probe
  * code runs as the user, so `git config --global` is open to it, and a filter
  * planted there is read by every checkout here and never seen by this screen.
  * Refusing on merged config is not the answer — `git lfs install` writes
@@ -652,12 +656,12 @@ export interface LocalFilterScreen {
    * stopped it — otherwise null.
    *
    * Exit 1 is git's ordinary "no key matched" and is not a failure. Anything
-   * else is: a spawn error, or the `maxBuffer` kill an attacker reaches by
-   * making the screened file itself large — the value is theirs to write, and
-   * git prints the whole of it. Skipping such a file reports the repository
-   * clean on the one file that defines the filter, so the caller refuses
-   * instead. One file, not a list: the refusal only has to name where the
-   * screen stopped, and an unbounded join is its own denial-of-service.
+   * else is: a spawn error, a config git could not parse to the end (a
+   * malformed section, or an include it could not follow), or an ENOBUFS from
+   * more filter keys than the buffer holds. Skipping such a file reports the
+   * repository clean on the one file that might define the filter, so the caller
+   * refuses instead. One file, not a list: the refusal only has to name where
+   * the screen stopped, and an unbounded join is its own denial-of-service.
    */
   unreadable: string | null;
 }
@@ -695,27 +699,32 @@ export function screenStopDetail(screen: LocalFilterScreen): string {
     : `${where} could not be read to the end`;
 }
 
+/**
+ * A single `git rev-parse <flag>` answer, as a path, or null on any failure.
+ *
+ * One invocation per value, never a combined newline-delimited request: the
+ * answers are arbitrary filesystem paths and a POSIX path may itself carry a
+ * newline, so a combined answer cannot be split unambiguously — a healthy
+ * worktree below a directory whose name holds a newline parses to extra
+ * records, misassigns the paths, and the caller reports a live plant as clean
+ * or a real worktree as not-a-worktree. Measured with exactly such a directory.
+ * The only byte removed is git's terminal record delimiter; every other byte
+ * belongs to the path, so neither a split nor a trim is a parse here. Callers
+ * that need absolute paths pass `--path-format=absolute` as a leading flag.
+ */
+function revParsePath(cwd: string, ...flags: string[]): string | null {
+  const r = spawnSync('git', ['rev-parse', ...flags], {
+    cwd,
+    encoding: 'utf8',
+    env: sanitizedGitEnv(),
+  });
+  if (r.error || r.status !== 0 || typeof r.stdout !== 'string') return null;
+  return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
+}
+
 export function localFilterCommands(worktree: string): LocalFilterScreen {
-  // One invocation per value, the shape `worktreeResidue`'s `discover` already
-  // uses in this file and for the same reason: the answers are filesystem
-  // paths, a POSIX path may carry a newline, and a combined newline-delimited
-  // answer cannot then be split unambiguously. A linked worktree under such a
-  // directory parses to four records instead of two, `commonDir` gets the head
-  // of the path and `gitDir` its tail, every candidate resolves to nothing,
-  // the admin readdir hits ENOENT — the branch this code calls ordinary — and
-  // the screen answers CLEAN over a live plant. Measured with exactly such a
-  // directory. Strip only git's terminal delimiter; every other byte is path.
-  const askPath = (flag: string): string | null => {
-    const r = spawnSync('git', ['rev-parse', flag], {
-      cwd: worktree,
-      encoding: 'utf8',
-      env: sanitizedGitEnv(),
-    });
-    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') return null;
-    return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
-  };
-  const commonDir = askPath('--git-common-dir');
-  const gitDir = askPath('--git-dir');
+  const commonDir = revParsePath(worktree, '--git-common-dir');
+  const gitDir = revParsePath(worktree, '--git-dir');
   if (commonDir === null || gitDir === null) {
     // Not a usable repository from here: the checkout this screen guards has
     // nothing to run in either, and its own failure is the caller's answer.
@@ -799,6 +808,16 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
         'config',
         '--file',
         file,
+        // `--file` alone does NOT expand `include.path` / `includeIf` — git's
+        // documented default for a single-file read — while every checkout this
+        // screen authorises reads MERGED config, which does. A probe appends
+        // `[include] path = evil.inc` to a screened candidate and writes the
+        // `filter.<name>.smudge` into the include target: without `--includes`
+        // the screen answers clean and the checkout runs it. With it, the screen
+        // reads the file exactly as the checkout will. A config this read cannot
+        // parse to the end — a malformed section, or an include it cannot follow
+        // — exits git non-zero, which the gate below turns into a refusal.
+        '--includes',
         // BEFORE the pattern: `--name-only` after it silently prints nothing
         // and exits 1 even with live keys. With it, each line is the whole key
         // and nothing has to be parsed out of a `key value` pair — a config
@@ -816,12 +835,14 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
       {
         cwd: worktree,
         encoding: 'utf8',
-        // The screened file is attacker-writable and git prints every matching
-        // value in full, so the DEFAULT 1 MiB buffer is a switch the plant can
-        // flip: pad one `smudge` value past the cap, `spawnSync` kills git with
-        // ENOBUFS, and a screen that skips the file reports clean while the
-        // checkout runs the last value. Sized like the `ls-files` spawn above.
-        maxBuffer: 64 * 1024 * 1024,
+        // No raised `maxBuffer`: `--name-only` prints one KEY per matching line
+        // and never the value, so the padded-`smudge`-value overflow that a full
+        // print invited cannot reach stdout — output is bounded by key count,
+        // not by an attacker-chosen value length. The one residual overflow is a
+        // config carrying more filter keys than the default 1 MiB holds, and
+        // that path sets `r.error` (ENOBUFS), which the gate below turns into a
+        // refusal. Fail closed on the pathological count; do not carry a buffer
+        // whose stated reason no longer exists.
         env: sanitizedGitEnv(),
       },
     );
@@ -835,12 +856,10 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
       const key = line;
-      // A Set, not `Array.includes`: the values here are attacker-written and
-      // the 64 MiB ceiling above admits millions of distinct keys, so a linear
-      // membership test per line is quadratic on exactly the input this screen
-      // exists to survive. (Measured: 100k keys took 144 s as an array scan and
-      // 53 ms as a Set — and the 1 MiB default this raised from used to hide it
-      // by ENOBUFS-killing git and skipping the loop entirely.)
+      // A Set, not `Array.includes`: the key names here are attacker-written and
+      // as many as the 1 MiB output holds, so a linear membership test per line
+      // is quadratic on exactly the input this screen exists to survive.
+      // (Measured: 100k keys took 144 s as an array scan and 53 ms as a Set.)
       if (key) found.add(key);
     }
   }
@@ -974,25 +993,14 @@ export function worktreeResidue(
   // add`, a cleanup whose `rmSync` failed — `status` exits 0 against the
   // enclosing user checkout: the wrong tree's dirty state answered as this
   // one's. Fail closed the way a loud git failure below does.
-  // One invocation per value: the answers are three arbitrary filesystem
-  // paths, and a POSIX name may carry a newline, so no combined
-  // newline-delimited answer can be split unambiguously — a healthy
-  // worktree below a directory whose name holds one parses to extra
-  // records, misassigns gitDir/commondir, and reports the checkout as not
-  // a worktree. Measured with exactly such a directory.
-  const discover = (flag: string): string | null => {
-    const r = spawnSync('git', ['rev-parse', '--path-format=absolute', flag], {
-      cwd,
-      encoding: 'utf8',
-      env: sanitizedGitEnv(),
-    });
-    if (r.error || r.status !== 0 || typeof r.stdout !== 'string') {
-      return null;
-    }
-    // Remove only git's terminal record delimiter: every other byte
-    // belongs to the path, so neither a split nor a trim is a parse here.
-    return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
-  };
+  // One invocation per value (see `revParsePath`): the answers are three
+  // arbitrary filesystem paths, a POSIX name may carry a newline, so no combined
+  // newline-delimited answer can be split unambiguously — a healthy worktree
+  // below a directory whose name holds one parses to extra records, misassigns
+  // gitDir/commondir, and reports the checkout as not a worktree. These need
+  // absolute paths, so they pass `--path-format=absolute`.
+  const discover = (flag: string): string | null =>
+    revParsePath(cwd, '--path-format=absolute', flag);
   const toplevel = discover('--show-toplevel');
   const gitDir = discover('--git-dir');
   const commonDir = discover('--git-common-dir');
diff --git a/packages/cli/src/commands/review/scratch-tree.test.ts b/packages/cli/src/commands/review/scratch-tree.test.ts
index ee9de54b882..5eb741bf7e3 100644
--- a/packages/cli/src/commands/review/scratch-tree.test.ts
+++ b/packages/cli/src/commands/review/scratch-tree.test.ts
@@ -144,7 +144,9 @@ describe('runScratchTree', () => {
 
     expect(r.available).toBe(false);
     expect(r.note).toContain('capped enumeration');
-    expect(r.note).toContain(`${MAX_SCREEN_KEYS} shown of ${MAX_SCREEN_KEYS + 1}`);
+    expect(r.note).toContain(
+      `${MAX_SCREEN_KEYS} shown of ${MAX_SCREEN_KEYS + 1}`,
+    );
   });
 
   it("screens ANOTHER worktree's per-worktree config, not just this one's", () => {
@@ -183,6 +185,28 @@ describe('runScratchTree', () => {
     expect(r.note).toContain('filter.planted.smudge');
   });
 
+  it('refuses when the screen STOPPED before finishing — not only when it found a key', () => {
+    // `runScratchTree` gates on `filters.stopped`, a branch distinct from the
+    // "found a filter key" one: a screen that could not read a candidate to the
+    // end is not a clean result, because the checkouts below would execute any
+    // filter it failed to see. Here the review worktree's own `config.worktree`
+    // candidate is a directory — a candidate the screen cannot read — so the
+    // screen stops and the command refuses before creating or resetting a tree.
+    const gitDir = execFileSync('git', ['rev-parse', '--absolute-git-dir'], {
+      cwd: worktree,
+      encoding: 'utf8',
+    }).trim();
+    mkdirSync(join(gitDir, 'config.worktree'));
+
+    const r = run();
+
+    expect(r.available).toBe(false);
+    expect(r.note).toContain('the screen could not clear this repository');
+    expect(
+      existsSync(scratchWorktreePath(worktree, 'verify--round-1--abc123')),
+    ).toBe(false);
+  });
+
   it('places it BESIDE the review worktree, never inside it', () => {
     // Nested, every probe file would land in the tree this command exists to
     // keep clean — and in the PR's own diff with it.
diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
index 7259642c8c7..45d7d1fd314 100644
--- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
@@ -1724,6 +1724,56 @@ process.stdout.write(JSON.stringify({
     expect(existsSync(canary)).toBe(false);
   });
 
+  it('neutralises core.fsmonitor at the REVERT checkout, not only at the restore', async () => {
+    // The filter screen names `filter.*` keys; `core.fsmonitor` is a different
+    // config-driven command surface it does not cover, and git runs it on a
+    // pathspec checkout. The restore already empties it (`-c core.fsmonitor=`);
+    // the revert's `checkout base -- <paths>` used to run bare. A baseline that
+    // plants `core.fsmonitor` into the never-wiped common config would then have
+    // its command fire when the revert rewrites the PR-modified files. The
+    // revert now passes the same neutralisation the restore does, so it never
+    // runs — this file's absence is the assertion.
+    const canary = join(repo, 'PWNED-revert-fsmonitor');
+    writeFileSync(
+      vitestScript(),
+      `#!/usr/bin/env node
+import path from 'node:path';
+import fs from 'node:fs';
+import { execFileSync } from 'node:child_process';
+const g = (...a) => execFileSync('git', a, { encoding: 'utf8' }).trim();
+// Plant once — on the baseline — the way a probe's own test code would. No
+// filter and no attributes: fsmonitor is not attribute-gated, so this slips
+// past the filter screen entirely and only the checkout's own -c can stop it.
+const common = g('rev-parse', '--git-common-dir');
+const stamp = path.join(common, 'PLANTED');
+if (!fs.existsSync(stamp)) {
+  fs.writeFileSync(stamp, '');
+  execFileSync('git', ['config', 'core.fsmonitor', 'sh -c "touch ${canary}; :"']);
+}
+const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
+const results = files.map((f) => ({
+  name: path.resolve(f),
+  assertionResults: [{ status: 'passed' }],
+}));
+process.stdout.write(JSON.stringify({
+  numPassedTests: results.length,
+  numFailedTests: 0,
+  testResults: results,
+}));
+`,
+    );
+    const { wt, base } = scaffoldModifiedPr();
+
+    await runHandler({
+      report: join(repo, 'report.json'),
+      worktree: wt,
+      base,
+      out: join(repo, 'out.json'),
+    });
+
+    expect(existsSync(canary)).toBe(false);
+  });
+
   it('never deletes a line that does not hold the selected statement', () => {
     // `runOneMutant`'s mismatch guard, pinned directly: selection and the
     // probe tree both derive from the same commit, so the command cannot reach
diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts
index 95929325295..2999e4156c9 100644
--- a/packages/cli/src/commands/review/test-efficacy.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.test.ts
@@ -650,12 +650,15 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => {
     }
   });
 
-  it('still sees a filter whose value is padded past the default spawn buffer', () => {
-    // The screened file is attacker-writable and git prints every matching
-    // value in full, so a `smudge` padded past `spawnSync`'s DEFAULT 1 MiB
-    // buffer used to kill git with ENOBUFS — and the screen skipped the one
-    // file that defines the filter and reported the repository clean, while
-    // the restore's checkout ran the last value.
+  it('still sees a filter whose value is padded huge — `--name-only` never prints the value', () => {
+    // The screened file is attacker-writable, and a `smudge` value padded past
+    // `spawnSync`'s DEFAULT 1 MiB buffer would kill git with ENOBUFS if the
+    // enumeration printed values — the screen would skip the one file that
+    // defines the filter and report the repository clean. `--name-only` prints
+    // only the KEY, so the padded value never reaches stdout: the key is found
+    // and the refusal fires with no raised buffer at all. Remove `--name-only`
+    // and this goes red (the 1.2 MiB value overflows the default buffer and the
+    // detail no longer names the key), which is what pins the flag.
     const dir = mkdtempSync(join(tmpdir(), 'qwen-bigvalue-'));
     const canaryDir = mkdtempSync(join(tmpdir(), 'qwen-canary-'));
     const isolation = isolateHostGitConfig();
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index 24cef3347f6..c71db0b232c 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -1304,6 +1304,20 @@ interface TestEfficacyArgs {
 // resets, the revert's checkout — so the mutations would land in whichever
 // repository the environment names while every check against the tree passes
 // silently. The trees this file touches are chosen by the paths it is given.
+// The two config-driven command surfaces a checkout fires from the very tree it
+// is cleaning: `core.hooksPath` (a `post-checkout` hook planted in the shared
+// common dir) and `core.fsmonitor` (a command git runs on checkout/status).
+// Disabling hooks does not cover filters and neutralising one checkout does not
+// cover the next — so every checkout in this file that rewrites files (the
+// restore and the revert alike) passes these, and none is hardened while a
+// sibling stays steerable.
+const CHECKOUT_INERT = [
+  '-c',
+  'core.hooksPath=/dev/null/no-hooks',
+  '-c',
+  'core.fsmonitor=',
+] as const;
+
 function git(cwd: string, ...args: string[]): void {
   const r = spawnSync('git', args, {
     cwd,
@@ -1661,15 +1675,10 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   if (filters.keys.length > 0) {
     return `the repository's local config defines content filter(s) ${screenKeyList(filters)}, which this tree's restore would EXECUTE`;
   }
-  // `core.fsmonitor` runs a command on BOTH of these, and the config that sets
-  // it lives in the tree they are cleaning: the residue probe empties it for
-  // exactly this reason and these two spawns were the ones still steerable.
-  const inert = [
-    '-c',
-    'core.hooksPath=/dev/null/no-hooks',
-    '-c',
-    'core.fsmonitor=',
-  ];
+  // `core.hooksPath` and `core.fsmonitor` both run a command on these two
+  // spawns, and the config that sets them lives in the tree they are cleaning:
+  // the residue probe empties them for exactly this reason.
+  const inert = CHECKOUT_INERT;
   for (const args of [
     [...inert, 'checkout', '--force', 'HEAD', '--', '.'],
     // `-ffdx`, because `-fd` honors the ignore rules — and those belong to the
@@ -3022,7 +3031,20 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
           (existsAtBase(probeTree, base, p) ? modified : added).push(p);
         }
         if (modified.length > 0) {
-          git(probeTree, 'checkout', base, '--', ...modified);
+          // Same neutralisation the restore's checkout runs (CHECKOUT_INERT):
+          // this revert rewrites the PR-modified files, and a `post-checkout`
+          // hook or a `core.fsmonitor` command planted in the never-wiped
+          // common dir mid-run would otherwise fire here — a surface the filter
+          // screen above does not cover, on a checkout the restore hardens and
+          // this one used to leave steerable.
+          git(
+            probeTree,
+            ...CHECKOUT_INERT,
+            'checkout',
+            base,
+            '--',
+            ...modified,
+          );
         }
         for (const p of added) safeRmWithin(probeTree, p);
 

From 4a3568918b478f82f616ee363883eb475d6c2c97 Mon Sep 17 00:00:00 2001
From: wenshao <szujobs@gmail.com>
Date: Tue, 1 Sep 2026 00:00:19 +0800
Subject: [PATCH 09/15] fix(review): gate the restore and revert screen
 decision on `stopped`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both test-efficacy screen sites gated on `filters.unreadable`; scratch-tree
gates on `filters.stopped`. They are equivalent only while the producer ties
the two fields together — a future stop mode that sets `stopped` without a
file would let the restore and revert checkouts proceed on a screen that did
not finish while scratch-tree refuses. Gate all three on `stopped`.
---
 packages/cli/src/commands/review/test-efficacy.ts | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index c71db0b232c..4fb3ec6eb5f 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -1669,7 +1669,13 @@ function restoreProbeTreeTracked(probeTree: string): string | null {
   // would put every contributor with git-lfs into permanent refusal — the same
   // failure as a tripwire that fires on every healthy run.
   const filters = localFilterCommands(probeTree);
-  if (filters.unreadable) {
+  // Gate on `stopped`, not `unreadable` — the same field `scratch-tree` gates
+  // on. A screen that stopped for ANY reason (an unreadable candidate OR an
+  // over-cap admin dir) did not finish, and either way the checkout below would
+  // execute a filter it failed to see. Keying on the `unreadable` file field
+  // would let a future stop mode that sets `stopped` without a file silently
+  // authorise the checkout while scratch-tree refuses.
+  if (filters.stopped) {
     return `the screen could not clear this tree's restore: ${screenStopDetail(filters)}`;
   }
   if (filters.keys.length > 0) {
@@ -3006,7 +3012,10 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         // The throw lands in this phase's existing catch and is recorded as a
         // probe that did not run, which is what it is.
         const revertFilters = localFilterCommands(probeTree);
-        if (revertFilters.unreadable) {
+        // Gate on `stopped`, matching the restore and scratch-tree — a screen
+        // that stopped for any reason did not finish, so the checkout must not
+        // proceed on it (see the restore's note above).
+        if (revertFilters.stopped) {
           throw new Error(
             'the screen could not clear this revert: ' +
               screenStopDetail(revertFilters),

From 74311cc7d8ea7644448b99b7967f6c37a578a704 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Tue, 1 Sep 2026 07:54:26 +0800
Subject: [PATCH 10/15] fix(review): walk includes by location, and bound every
 screen spawn
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round 5. The `--includes` added in round 4 closed the include-hop bypass and
opened two holes of its own; four more fail-opens sat under the spawns.

**`--includes` asks git to evaluate `includeIf`, and git evaluates it HERE.**
The screen runs in the review worktree; the checkout it authorises runs in the
scratch tree. A condition false for one and true for the other hides a filter
from the screen that the checkout then executes — and
`includeIf.hasconfig:remote.*.url:` never fires under `--file` at all. There is
no context that matches a cross-tree checkout, so the graph is walked here
instead: read each candidate's `include.path` / `includeIf.*.path` WITHOUT
`--includes` (so git reports directives and evaluates nothing) and follow every
target regardless of its condition.

**Which targets to follow is decided by LOCATION, not by git.** Inside the
worktree, common dir or gitdir: followed and screened — that is the
never-wiped surface a probe writes. Outside: left alone, because the user's
global config is their contract and `git lfs install` puts `filter.lfs.clean`
there. `--includes` followed it, so a one-line `[include] path = ~/.gitconfig`
would have refused every later review for any contributor with git-lfs, naming
a key their local config does not define. A leading `~` is expanded rather
than skipped, since a repository commonly lives under HOME and the location
test is what decides.

**No spawn had a timeout.** An `include.path` naming a FIFO blocks the screen
forever — git follows includes at startup, so even the first `rev-parse`
hangs, and `spawnSync` blocks the loop so no JS timer can interrupt. Both
spawns are bounded now, and a killed discovery no longer answers "not a usable
repository": that branch fails closed, because a kill happens exactly when a
plant has made git block.

**An invalid byte in the repo path became U+FFFD**, so every candidate named a
file that does not exist and the admin readdir hit ENOENT — "the ordinary
case" — over a live plant. Unmeasured, not clean; the residue walker in this
file already fails the same shape closed.

**`--get-regexp` is locale-sensitive.** Under a UTF-8 locale a key whose
subsection carries an invalid byte never matches, so git exits 1 and the gate
reads the ordinary clean answer — while the checkout resolves filter keys by
exact name, which no locale affects. The spawn now runs under `LC_ALL=C`.

**And the revert screen sat above the classification loop** — one `cat-file -e`
per revert path, on an attacker-sized list, between the screen's last read and
the checkout. It moves below the loop; the residual window is the checkout
spawn itself and is disclosed rather than claimed closed.
---
 .../src/commands/review/lib/worktree.test.ts  |  79 ++++++-
 .../cli/src/commands/review/lib/worktree.ts   | 212 ++++++++++++++++--
 .../review/test-efficacy.integration.test.ts  | 100 +++++++++
 .../cli/src/commands/review/test-efficacy.ts  |  33 ++-
 4 files changed, 388 insertions(+), 36 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index f1062d6fb72..31259a7525f 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -1926,6 +1926,79 @@ describe('localFilterCommands', () => {
     expect(screen.unreadable).toBeNull();
   });
 
+  it('does NOT follow an include that points OUT of the repository', () => {
+    // The include graph is walked by LOCATION. A repo-local `include.path`
+    // naming the user's own global config is their contract, exactly as merged
+    // config is for any git command they run — and `git lfs install` writes
+    // `filter.lfs.clean` there, so following it would put every contributor
+    // with git-lfs into permanent refusal. That is the failure this whole
+    // screen is scoped to avoid.
+    const outside = mkdtempSync(join(tmpdir(), 'qwen-userconf-'));
+    try {
+      const userConfig = join(outside, 'gitconfig');
+      writeFileSync(
+        userConfig,
+        '[filter "lfs"]\n\tclean = git-lfs clean -- %f\n',
+      );
+      appendFileSync(
+        join(dir, '.git', 'config'),
+        `[include]\n\tpath = ${userConfig}\n`,
+      );
+
+      const screen = localFilterCommands(dir);
+
+      expect(screen.keys).toEqual([]);
+      expect(screen.stopped).toBeNull();
+    } finally {
+      rmSync(outside, { recursive: true, force: true });
+    }
+  });
+
+  it('does not report the user\'s global keys through a `~` include', () => {
+    // The one-line write a probe can make: `[include] path = ~/.gitconfig` in
+    // the never-wiped common config. `sanitizedGitEnv()` keeps HOME, so the
+    // tilde expands to the reviewing user's real config — and for anyone with
+    // git-lfs installed that holds `filter.lfs.clean`. Reporting it would
+    // refuse every later review of the repo, naming a key the local config
+    // does not define and whose removal breaks git-lfs everywhere else.
+    const home = process.env['HOME'];
+    expect(home).toBeTruthy();
+    writeFileSync(
+      join(home as string, '.gitconfig'),
+      '[filter "lfs"]\n\tclean = git-lfs clean -- %f\n',
+    );
+    appendFileSync(
+      join(dir, '.git', 'config'),
+      '[include]\n\tpath = ~/.gitconfig\n',
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.keys).toEqual([]);
+    expect(screen.stopped).toBeNull();
+  });
+
+  it('follows a conditional include whose condition is false HERE', () => {
+    // `includeIf` conditions are evaluated against whichever tree git runs in,
+    // and the checkout this screen authorises runs in a DIFFERENT tree — so a
+    // condition false for the screen and true for the checkout would hide a
+    // filter that then executes. Asking git to evaluate (`--includes`) cannot
+    // be right for a cross-tree checkout; the walk follows every in-repo
+    // target regardless of its condition.
+    writeFileSync(
+      join(dir, '.git', 'evil.inc'),
+      '[filter "evil"]\n\tsmudge = cat\n',
+    );
+    appendFileSync(
+      join(dir, '.git', 'config'),
+      '[includeIf "gitdir:/nowhere/that/matches/"]\n\tpath = evil.inc\n',
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.keys).toEqual(['filter.evil.smudge']);
+  });
+
   it('caps the reported keys and keeps the pre-cap total', () => {
     // The keys come from an attacker-writable file that cleanup never wipes;
     // an unbounded list is its own denial-of-service in the refusal message.
@@ -2077,9 +2150,9 @@ describe('localFilterCommands', () => {
     // `git config --file` does NOT expand `include.path` / `includeIf` on its
     // own — git's documented default for a single-file read — while every
     // checkout this screen authorises reads MERGED config, which does. A filter
-    // one include-hop behind a screened candidate is invisible to a screen that
-    // omits `--includes` and executed by the checkout. The screen passes
-    // `--includes`, so it sees exactly what the checkout will.
+    // one include-hop behind a screened candidate would be invisible. The
+    // screen walks the include graph itself rather than passing `--includes`;
+    // the sibling cases below pin why.
     writeFileSync(
       join(dir, '.git', 'evil.inc'),
       '[filter "evil"]\n\tsmudge = cat\n',
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index 8ce6545b00f..d4ee30886d6 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -712,23 +712,137 @@ export function screenStopDetail(screen: LocalFilterScreen): string {
  * belongs to the path, so neither a split nor a trim is a parse here. Callers
  * that need absolute paths pass `--path-format=absolute` as a leading flag.
  */
+/**
+ * The screen's spawn budget.
+ *
+ * Every read below opens attacker-writable config, and git follows an
+ * `include.path` at startup — so a plant naming a FIFO (or a symlink to
+ * `/dev/zero`) blocks the very first `rev-parse`. `spawnSync` blocks the event
+ * loop, so no JS timer can interrupt it; the bound has to be on the spawn.
+ */
+const SCREEN_SPAWN_TIMEOUT_MS = 20_000;
+
+/**
+ * A rev-parse answer, or null when the screen could not get one.
+ *
+ * `encoding: 'utf8'` transcodes an invalid byte to U+FFFD, so a repository
+ * under a path carrying one yields an answer that names no real file. Callers
+ * must treat that as unmeasured rather than resolve candidates against it —
+ * the residue walker in this file fails the same shape closed for the same
+ * reason.
+ */
 function revParsePath(cwd: string, ...flags: string[]): string | null {
   const r = spawnSync('git', ['rev-parse', ...flags], {
     cwd,
     encoding: 'utf8',
+    timeout: SCREEN_SPAWN_TIMEOUT_MS,
+    killSignal: 'SIGKILL',
     env: sanitizedGitEnv(),
   });
   if (r.error || r.status !== 0 || typeof r.stdout !== 'string') return null;
   return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
 }
 
+/** True when a transcode replaced bytes this screen would otherwise resolve. */
+function carriesReplacementChar(value: string): boolean {
+  return value.includes('\uFFFD');
+}
+
+/**
+ * How many include hops the screen will follow before refusing.
+ *
+ * The graph is attacker-written and may be cyclic or merely deep; visited
+ * targets are tracked, so this bounds work rather than correctness.
+ */
+const MAX_INCLUDE_HOPS = 32;
+
+/**
+ * The `include.path` / `includeIf.*.path` targets a candidate names, resolved.
+ *
+ * Read WITHOUT `--includes`, so git reports the directives this file literally
+ * carries and evaluates no condition. Whether a conditional include would fire
+ * is not a question this screen can answer — the condition is evaluated
+ * against whichever tree git runs in, and the checkout runs in a different one
+ * — so every target is followed regardless of its condition, and the decision
+ * about which ones matter is made by LOCATION below, not by git.
+ */
+function includeTargetsOf(
+  worktree: string,
+  file: string,
+): { targets: string[]; unreadable: boolean } {
+  const r = spawnSync(
+    'git',
+    ['config', '--file', file, '--get-regexp', '^include(If\\..*)?\\.path$'],
+    {
+      cwd: worktree,
+      encoding: 'utf8',
+      timeout: SCREEN_SPAWN_TIMEOUT_MS,
+      killSignal: 'SIGKILL',
+      env: { ...sanitizedGitEnv(), LC_ALL: 'C' },
+    },
+  );
+  if (r.error || (r.status !== 0 && r.status !== 1)) {
+    return { targets: [], unreadable: true };
+  }
+  if (r.status === 1 || typeof r.stdout !== 'string') {
+    return { targets: [], unreadable: false };
+  }
+  const targets: string[] = [];
+  for (const line of r.stdout.split('\n')) {
+    if (line.length === 0) continue;
+    // `key value`, and only the FIRST space separates them — a path may hold
+    // the rest, including further spaces.
+    const sp = line.indexOf(' ');
+    if (sp < 0) continue;
+    const raw = line.slice(sp + 1);
+    if (raw.length === 0) continue;
+    // git expands a leading `~` against HOME and resolves anything else
+    // against the including file's directory. Expand it the same way rather
+    // than skipping: a repository commonly lives under HOME, so `~/...` can
+    // name a target INSIDE it, and the location test below is what decides.
+    if (raw === '~' || raw.startsWith('~/')) {
+      const home = process.env['HOME'];
+      if (home === undefined || home.length === 0) continue;
+      targets.push(resolve(home, raw.slice(raw === '~' ? 1 : 2)));
+      continue;
+    }
+    // `~user/` is not expanded here — resolving another account's home is not
+    // this screen's business, and an unexpanded path simply is not followed.
+    if (raw.startsWith('~')) continue;
+    targets.push(resolve(dirname(file), raw));
+  }
+  return { targets, unreadable: false };
+}
+
 export function localFilterCommands(worktree: string): LocalFilterScreen {
   const commonDir = revParsePath(worktree, '--git-common-dir');
   const gitDir = revParsePath(worktree, '--git-dir');
   if (commonDir === null || gitDir === null) {
-    // Not a usable repository from here: the checkout this screen guards has
-    // nothing to run in either, and its own failure is the caller's answer.
-    return { keys: [], total: 0, unreadable: null, stopped: null };
+    // Two very different reasons land here, and only one is benign. A plain
+    // "not a repository" answer is the caller's own problem — its checkout has
+    // nothing to run in either. But a discovery spawn KILLED at the timeout
+    // above reaches this line too, and that one happens precisely when a plant
+    // has made git block, so it must not read as clean. There is no way to
+    // tell them apart from the outside, so this fails closed: a screen that
+    // could not discover the repository did not screen it.
+    return {
+      keys: [],
+      total: 0,
+      unreadable: worktree,
+      stopped: 'unreadable',
+    };
+  }
+  if (carriesReplacementChar(commonDir) || carriesReplacementChar(gitDir)) {
+    // An invalid byte in the repository path came back as U+FFFD, so every
+    // candidate built from it names a file that does not exist, the admin
+    // readdir hits ENOENT — "the ordinary case" — and the screen would answer
+    // clean over whatever the real path holds. Unmeasured, not clean.
+    return {
+      keys: [],
+      total: 0,
+      unreadable: worktree,
+      stopped: 'unreadable',
+    };
   }
   const common = resolve(worktree, commonDir);
   const candidates = [
@@ -780,9 +894,42 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
   for (const entry of worktreeEntries ?? []) {
     candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
   }
+  // Anything under these is the repository's own state, which a probe can
+  // write and cleanup never wipes. Anything outside them is the user's — their
+  // global config is their contract exactly as it is for any git command they
+  // run, and `git lfs install` puts `filter.lfs.clean` there. So an include
+  // whose target lands inside is followed and screened; one pointing out is
+  // left alone. That is the whole rule, and it is a rule about LOCATION, which
+  // is the same in every tree — unlike an `includeIf` condition, which is not.
+  const repoRoots = [common, resolve(worktree, gitDir), resolve(worktree)].map(
+    (d) => {
+      try {
+        return realpathSync(d);
+      } catch {
+        return d;
+      }
+    },
+  );
+  const insideRepo = (p: string): boolean => {
+    let real = p;
+    try {
+      real = realpathSync(dirname(p)) + sep + basename(p);
+    } catch {
+      // Not yet resolvable: judge the literal path rather than skipping it.
+    }
+    return repoRoots.some(
+      (root) => real === root || real.startsWith(root + sep),
+    );
+  };
+
   const found = new Set<string>();
   let unreadable: string | null = null;
-  for (const file of candidates) {
+  // Seeded with the candidates themselves, so an include pointing back at
+  // one of them is not walked twice.
+  const seen = new Set<string>(candidates);
+  const queue = [...candidates];
+  let hops = 0;
+  for (const file of queue) {
     if (!existsSync(file)) continue;
     // Existence is not readability, and git does not distinguish them for us:
     // an unreadable `--file` exits 1 with a warning on stderr — byte-identical
@@ -808,16 +955,14 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
         'config',
         '--file',
         file,
-        // `--file` alone does NOT expand `include.path` / `includeIf` — git's
-        // documented default for a single-file read — while every checkout this
-        // screen authorises reads MERGED config, which does. A probe appends
-        // `[include] path = evil.inc` to a screened candidate and writes the
-        // `filter.<name>.smudge` into the include target: without `--includes`
-        // the screen answers clean and the checkout runs it. With it, the screen
-        // reads the file exactly as the checkout will. A config this read cannot
-        // parse to the end — a malformed section, or an include it cannot follow
-        // — exits git non-zero, which the gate below turns into a refusal.
-        '--includes',
+        // NO `--includes`. It looks like the honest read — the checkout does
+        // expand includes — but it makes git EVALUATE `includeIf` conditions
+        // in this process's context, and the checkout this authorises runs in
+        // a different tree. A condition false here and true there hides a
+        // filter from the screen that the checkout then executes, and
+        // `includeIf.hasconfig:remote.*.url:` never fires under `--file` at
+        // all. There is no context that matches a cross-tree checkout, so the
+        // include graph is walked explicitly below instead of delegated.
         // BEFORE the pattern: `--name-only` after it silently prints nothing
         // and exits 1 even with live keys. With it, each line is the whole key
         // and nothing has to be parsed out of a `key value` pair — a config
@@ -836,14 +981,19 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
         cwd: worktree,
         encoding: 'utf8',
         // No raised `maxBuffer`: `--name-only` prints one KEY per matching line
-        // and never the value, so the padded-`smudge`-value overflow that a full
-        // print invited cannot reach stdout — output is bounded by key count,
-        // not by an attacker-chosen value length. The one residual overflow is a
-        // config carrying more filter keys than the default 1 MiB holds, and
-        // that path sets `r.error` (ENOBUFS), which the gate below turns into a
-        // refusal. Fail closed on the pathological count; do not carry a buffer
-        // whose stated reason no longer exists.
-        env: sanitizedGitEnv(),
+        // and never the value, so a padded `smudge` value cannot reach stdout —
+        // output is bounded by key count, not by an attacker-chosen length. A
+        // config carrying more keys than the default holds sets `r.error`
+        // (ENOBUFS), which the gate below turns into a refusal.
+        timeout: SCREEN_SPAWN_TIMEOUT_MS,
+        killSignal: 'SIGKILL',
+        // Byte-wise regex semantics. `--get-regexp` is POSIX matching and is
+        // locale-sensitive: under a UTF-8 locale a key whose subsection name
+        // carries an invalid byte never matches, so git exits 1 and the gate
+        // below reads that as the ordinary clean answer — while the checkout
+        // resolves filter keys by exact name, which no locale affects. The
+        // screen would certify clean over a filter the checkout runs.
+        env: { ...sanitizedGitEnv(), LC_ALL: 'C' },
       },
     );
     // Exit 1 is "no key matched" — the ordinary clean answer. Every other
@@ -853,6 +1003,24 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
       unreadable ??= file;
       continue;
     }
+    // Follow this file's includes, but only where they land inside the
+    // repository. git expands them for the checkout regardless; the screen
+    // declines to chase one into the user's own configuration, which it must
+    // not refuse on.
+    if (hops < MAX_INCLUDE_HOPS) {
+      const inc = includeTargetsOf(worktree, file);
+      if (inc.unreadable) unreadable ??= file;
+      for (const target of inc.targets) {
+        if (!insideRepo(target) || seen.has(target)) continue;
+        seen.add(target);
+        hops += 1;
+        if (hops > MAX_INCLUDE_HOPS) {
+          unreadable ??= file;
+          break;
+        }
+        queue.push(target);
+      }
+    }
     if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
       const key = line;
diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
index 45d7d1fd314..ec666515e4b 100644
--- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts
+++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts
@@ -1774,6 +1774,106 @@ process.stdout.write(JSON.stringify({
     expect(existsSync(canary)).toBe(false);
   });
 
+  it('refuses the REVERT when the baseline planted an unparseable config', async () => {
+    // The keys-found branch has a witness; the `stopped` branch did not. A
+    // plant that makes the screen STOP — here an unparseable `config.worktree`
+    // — must refuse the revert just as a found key does, because a screen that
+    // could not finish did not clear anything. The plant lands mid-run, after
+    // the restore's own screen has already passed.
+    const canary = join(repo, 'PWNED-revert-stopped');
+    writeFileSync(
+      vitestScript(),
+      `#!/usr/bin/env node
+import path from 'node:path';
+import fs from 'node:fs';
+import { execFileSync } from 'node:child_process';
+const g = (...a) => execFileSync('git', a, { encoding: 'utf8' }).trim();
+const common = g('rev-parse', '--git-common-dir');
+const stamp = path.join(common, 'PLANTED');
+if (!fs.existsSync(stamp)) {
+  fs.writeFileSync(stamp, '');
+  // The filter that would fire, plus the attributes selecting it...
+  execFileSync('git', ['config', 'filter.evil.smudge', 'sh -c "pwd >> ${canary}; cat"']);
+  const attrs = g('rev-parse', '--git-path', 'info/attributes');
+  fs.mkdirSync(path.dirname(attrs), { recursive: true });
+  fs.appendFileSync(attrs, '*.ts filter=evil\\n');
+  // ...and an unparseable candidate, so the screen STOPS before it can
+  // enumerate that filter. A stop must refuse exactly as a key would.
+  const gitDir = g('rev-parse', '--git-dir');
+  fs.writeFileSync(path.join(gitDir, 'config.worktree'), '[filter "x"\\n\\tsmudge = cat\\n');
+}
+const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
+const results = files.map((f) => ({
+  name: path.resolve(f),
+  assertionResults: [{ status: 'passed' }],
+}));
+process.stdout.write(JSON.stringify({
+  numPassedTests: results.length,
+  numFailedTests: 0,
+  testResults: results,
+}));
+`,
+    );
+    const { wt, base } = scaffoldModifiedPr();
+
+    await runHandler({
+      report: join(repo, 'report.json'),
+      worktree: wt,
+      base,
+      out: join(repo, 'out.json'),
+    });
+
+    expect(existsSync(canary)).toBe(false);
+  });
+
+  it('runs no planted post-checkout hook during the revert', async () => {
+    // `CHECKOUT_INERT` carries two neutralisations and only fsmonitor was
+    // pinned. A pathspec checkout DOES fire an executable `post-checkout`
+    // hook, and the hook needs no config of its own — so it is a surface the
+    // filter screen cannot see, closed only by the flag.
+    const canary = join(repo, 'PWNED-post-checkout');
+    writeFileSync(
+      vitestScript(),
+      `#!/usr/bin/env node
+import path from 'node:path';
+import fs from 'node:fs';
+import { execFileSync } from 'node:child_process';
+const g = (...a) => execFileSync('git', a, { encoding: 'utf8' }).trim();
+const common = g('rev-parse', '--git-common-dir');
+const stamp = path.join(common, 'PLANTED');
+if (!fs.existsSync(stamp)) {
+  fs.writeFileSync(stamp, '');
+  // The fixture redirects hooks to this directory, so the plant goes there.
+  const hooks = execFileSync('git', ['config', 'core.hooksPath'], { encoding: 'utf8' }).trim();
+  fs.mkdirSync(hooks, { recursive: true });
+  const hook = path.join(hooks, 'post-checkout');
+  fs.writeFileSync(hook, '#!/bin/sh\\necho fired >> ${canary}\\n');
+  fs.chmodSync(hook, 0o755);
+}
+const files = process.argv.slice(2).filter((a) => a.includes('.test.'));
+const results = files.map((f) => ({
+  name: path.resolve(f),
+  assertionResults: [{ status: 'passed' }],
+}));
+process.stdout.write(JSON.stringify({
+  numPassedTests: results.length,
+  numFailedTests: 0,
+  testResults: results,
+}));
+`,
+    );
+    const { wt, base } = scaffoldModifiedPr();
+
+    await runHandler({
+      report: join(repo, 'report.json'),
+      worktree: wt,
+      base,
+      out: join(repo, 'out.json'),
+    });
+
+    expect(existsSync(canary)).toBe(false);
+  });
+
   it('never deletes a line that does not hold the selected statement', () => {
     // `runOneMutant`'s mismatch guard, pinned directly: selection and the
     // probe tree both derive from the same commit, so the command cannot reach
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index 4fb3ec6eb5f..1e779343308 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -3011,6 +3011,28 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         // A refused restore therefore reaches this line with the plant live.
         // The throw lands in this phase's existing catch and is recorded as a
         // probe that did not run, which is what it is.
+        // "Revert to base" is two operations, confined to the throwaway tree. A
+        // file the PR MODIFIED is checked out from base; a file the PR ADDED did
+        // not exist at base, so it is removed — through `safeRmWithin`, which
+        // still refuses to delete through a PR-controlled symlink even here.
+        // Removing an added file usually makes the probe fail to compile, which
+        // is `inconclusive` — a non-verdict, but an honest one.
+        const modified: string[] = [];
+        const added: string[] = [];
+        for (const p of revert) {
+          (existsAtBase(probeTree, base, p) ? modified : added).push(p);
+        }
+        // Screened HERE, immediately before the spawn, not above the loop.
+        // The classification above runs one synchronous `git cat-file -e` per
+        // revert path, and `revert` is the plan's full source-file list —
+        // attacker-controlled in count. A screen above it leaves a window
+        // measured in seconds between the last read and the checkout, and the
+        // probe's own test code can spawn a detached process that plants the
+        // filter inside it. Moving the screen below the loop leaves only the
+        // checkout spawn in the gap. The residual window is sub-millisecond
+        // and cannot be closed from here: there is no `-c` kill switch for
+        // attribute-selected filters, so a checkout either reads merged config
+        // or does not run.
         const revertFilters = localFilterCommands(probeTree);
         // Gate on `stopped`, matching the restore and scratch-tree — a screen
         // that stopped for any reason did not finish, so the checkout must not
@@ -3028,17 +3050,6 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
               'would EXECUTE',
           );
         }
-        // "Revert to base" is two operations, confined to the throwaway tree. A
-        // file the PR MODIFIED is checked out from base; a file the PR ADDED did
-        // not exist at base, so it is removed — through `safeRmWithin`, which
-        // still refuses to delete through a PR-controlled symlink even here.
-        // Removing an added file usually makes the probe fail to compile, which
-        // is `inconclusive` — a non-verdict, but an honest one.
-        const modified: string[] = [];
-        const added: string[] = [];
-        for (const p of revert) {
-          (existsAtBase(probeTree, base, p) ? modified : added).push(p);
-        }
         if (modified.length > 0) {
           // Same neutralisation the restore's checkout runs (CHECKOUT_INERT):
           // this revert rewrites the PR-modified files, and a `post-checkout`

From 0c99c4042cd9d02fd61704281a0e2c257a877f0e Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Tue, 1 Sep 2026 08:56:58 +0800
Subject: [PATCH 11/15] test(review): witness the locale, transcode, and FIFO
 fail-opens
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three of round 5's fixes had no test that went red without them.

`LC_ALL=C` — a filter key whose subsection carries an invalid UTF-8 byte,
asserted found. Without it git's POSIX matching skips the key under a UTF-8
locale, exits 1, and the screen reads the ordinary clean answer while the
checkout resolves the same key by exact name.

The U+FFFD guard — a repository under a path holding an invalid byte,
asserted refused. Linux only: macOS and Windows reject such a filename with
EILSEQ, so the fixture cannot be built there.

The spawn timeouts — an `include.path` naming a FIFO, asserted refused rather
than hung. Dropping the timeouts does not turn this red so much as wedge it:
`spawnSync` blocks the event loop, so vitest's own testTimeout cannot fire
either. That is the defect, and it is why the bound has to sit on the spawn.
---
 .../src/commands/review/lib/worktree.test.ts  | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index 31259a7525f..ad4eb5edc95 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -1999,6 +1999,76 @@ describe('localFilterCommands', () => {
     expect(screen.keys).toEqual(['filter.evil.smudge']);
   });
 
+  it('matches a filter key whose subsection carries an invalid UTF-8 byte', () => {
+    // `--get-regexp` is POSIX matching and locale-sensitive: under a UTF-8
+    // locale a key holding an invalid byte never matches, so git exits 1 and
+    // that reads as the ordinary "no key matched". The checkout resolves
+    // filter keys by exact name, which no locale affects — so the screen would
+    // certify clean over a filter the checkout runs.
+    appendFileSync(
+      join(dir, '.git', 'config'),
+      Buffer.concat([
+        Buffer.from('[filter "'),
+        Buffer.from([0xff]),
+        Buffer.from('"]\n\tsmudge = cat\n'),
+      ]),
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.total).toBe(1);
+    expect(screen.keys[0]).toMatch(/^filter\..*\.smudge$/);
+  });
+
+  // Linux only: macOS and Windows reject a filename carrying an invalid UTF-8
+  // byte outright (EILSEQ on mkdir), so the fixture cannot be built there.
+  it.skipIf(process.platform !== 'linux')(
+    'refuses a repository whose path carries an invalid UTF-8 byte',
+    () => {
+      // `encoding: 'utf8'` transcodes the byte to U+FFFD, so every candidate
+      // built from the answer names a file that does not exist and the admin
+      // readdir hits ENOENT — the branch treated as ordinary. Unmeasured is
+      // not clean; the residue walker fails the same shape closed.
+      const base = mkdtempSync(join(tmpdir(), 'qwen-badbyte-'));
+      try {
+        const holder = Buffer.concat([
+          Buffer.from(join(base, 'n')),
+          Buffer.from([0xff]),
+          Buffer.from('d'),
+        ]);
+        mkdirSync(holder);
+        const asPath = holder.toString('binary');
+        initRepo(asPath);
+        execFileSync('git', ['config', 'filter.evil.smudge', 'cat'], {
+          cwd: asPath,
+        });
+
+        const screen = localFilterCommands(asPath);
+
+        expect(screen.stopped).toBe('unreadable');
+      } finally {
+        rmSync(base, { recursive: true, force: true });
+      }
+    },
+  );
+
+  it('refuses rather than hanging when an include names a FIFO', () => {
+    // git follows includes at startup, so the very first `rev-parse` blocks on
+    // a FIFO nobody writes to — and `spawnSync` blocks the event loop, so no
+    // JS timer can interrupt it. The bound has to be on the spawn, and a
+    // killed discovery must not read as "not a usable repository".
+    const fifo = join(dir, '.git', 'evil.fifo');
+    execFileSync('mkfifo', [fifo]);
+    appendFileSync(
+      join(dir, '.git', 'config'),
+      `[include]\n\tpath = ${fifo}\n`,
+    );
+
+    const screen = localFilterCommands(dir);
+
+    expect(screen.stopped).toBe('unreadable');
+  }, 60_000);
+
   it('caps the reported keys and keeps the pre-cap total', () => {
     // The keys come from an attacker-writable file that cleanup never wipes;
     // an unbounded list is its own denial-of-service in the refusal message.

From e02f581fd5550624116a75211c0713751ac59265 Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Tue, 1 Sep 2026 20:47:43 +0800
Subject: [PATCH 12/15] =?UTF-8?q?test(review):=20pin=20the=20transcode=20r?=
 =?UTF-8?q?ule=20directly=20=E2=80=94=20the=20end-to-end=20fixture=20is=20?=
 =?UTF-8?q?unbuildable?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The round-5 test for the U+FFFD guard went red on CI, and the reason is that it
could never have worked: it built the directory name as a `Buffer` with a raw
0xFF, then passed `buffer.toString('binary')` as `cwd`. Node re-encodes that
string as UTF-8, so 0xFF became 0xC3 0xBF, the path did not exist, and git
failed with ENOENT. A path carrying an invalid byte survives neither
`Buffer`→`string` nor the `cwd` option, which takes a string only — so a
repository AT such a path cannot be created and then addressed from a Node
test.

I could not have caught this locally: macOS rejects the filename outright with
EILSEQ, so the fixture never got far enough to fail for its real reason, and I
guarded it to Linux and pushed it unverified. That was the mistake — the guard
made it CI-only, which is exactly where an unverifiable test does its damage.

`carriesReplacementChar` is exported and pinned directly instead; its two call
sites are one `if` each. Dropping the guard turns the pin red.

The sibling at worktree.test.ts:1064 does exercise invalid bytes end to end,
and the difference is instructive: it puts them in a gitlink NAME — data inside
the index — never in a path it has to hand to a process.
---
 .../src/commands/review/lib/worktree.test.ts  | 51 ++++++++-----------
 .../cli/src/commands/review/lib/worktree.ts   | 13 ++++-
 2 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index ad4eb5edc95..a7b08ef567c 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -34,6 +34,7 @@ import { isolateHostGitConfig } from './test-utils.js';
 import {
   MAX_SCREEN_CANDIDATES,
   MAX_SCREEN_KEYS,
+  carriesReplacementChar,
   discardWorktree,
   exposeDependencies,
   localFilterCommands,
@@ -2020,37 +2021,25 @@ describe('localFilterCommands', () => {
     expect(screen.keys[0]).toMatch(/^filter\..*\.smudge$/);
   });
 
-  // Linux only: macOS and Windows reject a filename carrying an invalid UTF-8
-  // byte outright (EILSEQ on mkdir), so the fixture cannot be built there.
-  it.skipIf(process.platform !== 'linux')(
-    'refuses a repository whose path carries an invalid UTF-8 byte',
-    () => {
-      // `encoding: 'utf8'` transcodes the byte to U+FFFD, so every candidate
-      // built from the answer names a file that does not exist and the admin
-      // readdir hits ENOENT — the branch treated as ordinary. Unmeasured is
-      // not clean; the residue walker fails the same shape closed.
-      const base = mkdtempSync(join(tmpdir(), 'qwen-badbyte-'));
-      try {
-        const holder = Buffer.concat([
-          Buffer.from(join(base, 'n')),
-          Buffer.from([0xff]),
-          Buffer.from('d'),
-        ]);
-        mkdirSync(holder);
-        const asPath = holder.toString('binary');
-        initRepo(asPath);
-        execFileSync('git', ['config', 'filter.evil.smudge', 'cat'], {
-          cwd: asPath,
-        });
-
-        const screen = localFilterCommands(asPath);
-
-        expect(screen.stopped).toBe('unreadable');
-      } finally {
-        rmSync(base, { recursive: true, force: true });
-      }
-    },
-  );
+  it('treats a transcoded rev-parse answer as unmeasured', () => {
+    // `encoding: 'utf8'` turns an invalid byte in the repository path into
+    // U+FFFD, so every candidate built from the answer names a file that does
+    // not exist, the admin readdir hits ENOENT — the branch treated as the
+    // ordinary "no linked worktrees" case — and the screen would answer clean
+    // over whatever the real path holds. Unmeasured is not clean; the residue
+    // walker in this file fails the same shape closed.
+    //
+    // Pinned at this level deliberately. The end-to-end fixture cannot be
+    // built from Node: a path carrying an invalid byte does not survive
+    // `Buffer`→`string`, and `cwd` accepts only a string, so the repository
+    // cannot be created and then addressed from a test. (Attempting it is how
+    // this test got written the first time — the round-trip re-encoded 0xFF as
+    // 0xC3 0xBF and git failed with ENOENT on a path that did not exist.)
+    expect(carriesReplacementChar('/tmp/plain/path')).toBe(false);
+    expect(carriesReplacementChar('')).toBe(false);
+    expect(carriesReplacementChar('/tmp/n\uFFFDd/repo')).toBe(true);
+    expect(carriesReplacementChar('\uFFFD')).toBe(true);
+  });
 
   it('refuses rather than hanging when an include names a FIFO', () => {
     // git follows includes at startup, so the very first `rev-parse` blocks on
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index d4ee30886d6..d9cbbc47c88 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -743,8 +743,17 @@ function revParsePath(cwd: string, ...flags: string[]): string | null {
   return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout;
 }
 
-/** True when a transcode replaced bytes this screen would otherwise resolve. */
-function carriesReplacementChar(value: string): boolean {
+/**
+ * True when a transcode replaced bytes this screen would otherwise resolve.
+ *
+ * Exported for its own test: the end-to-end fixture cannot be built from Node.
+ * A path holding an invalid UTF-8 byte survives neither `Buffer`→`string` nor
+ * the `cwd` option, which takes a string only — so the repository that would
+ * produce such a rev-parse answer cannot be created and then addressed from a
+ * test. The rule this function states is pinned here instead, and its two call
+ * sites are one `if` each.
+ */
+export function carriesReplacementChar(value: string): boolean {
   return value.includes('\uFFFD');
 }
 

From 8648ee3737d1370789771e6577b7fd45405bbb9d Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Wed, 2 Sep 2026 08:53:29 +0800
Subject: [PATCH 13/15] fix(review): stop following includes, and read the
 common config last
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round 9. Two changes, and the first is a retreat.

**The hand-walked include graph is removed.** Round 4 delegated to git's
`--includes`; round 5 replaced that with a walk of my own because `--includes`
evaluates `includeIf` in the screen's context while the checkout runs in
another tree. The walk then diverged from git's own resolution eleven measured
ways across five rounds — `%(prefix)`, `~//` (Node's `resolve` resets on the
leading slash where git concatenates), `~user`, symlinked components,
non-UTF-8 targets, space- and newline-bearing values, a hop budget that failed
OPEN at exactly the bound, and a repo model that described the review worktree
rather than the tree the checkout runs in. Each fix was one more entrance.

Two of those I re-measured at HEAD before deciding: the hop budget skipped the
walk silently at `hops === MAX` while its refusal only fired at `>`, and
`resolve(home, '/evil.cfg')` yields `/evil.cfg` where git reads
`$HOME/evil.cfg`. Both answered clean over a live plant.

The condition problem survives any amount of parsing correctness — no single
context matches a cross-tree checkout — so this is not a parser to finish. The
screen no longer follows includes at all, the two ways of trying are recorded
where the decision lives, and #9558 already scopes this out ("`include.path` /
`includeIf` ... are each handled separately today"). #10441 carries the design
that closes it.

The tests that pinned following are replaced by one that pins the LIMIT, so a
later change that starts following goes red and has to be deliberate. The two
arms that matter — no global `filter.lfs.*` through an include, plain or `~` —
are kept, because that is the trade this screen exists to protect.

**`<common>/config` is read last.** The checkout reads merged config at its
start, so everything between the screen's read of a file and that checkout is a
window. The admin-dir candidate set is attacker-amplifiable — filler entries
cost spawns — so reading the common config first put the likeliest plant target
at the start of a walk the plant can stretch to seconds. Reading it last
removes the amplification; the window itself cannot be closed from here, and
the revert-site comment that called it sub-millisecond is corrected to say so.

Also corrected: the interface docstring still described the `--includes`
behaviour two rounds after it was replaced.
---
 .../src/commands/review/lib/worktree.test.ts  |  69 +++----
 .../cli/src/commands/review/lib/worktree.ts   | 185 +++++-------------
 .../cli/src/commands/review/test-efficacy.ts  |  11 +-
 3 files changed, 80 insertions(+), 185 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index a7b08ef567c..1ce65e58530 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -1927,13 +1927,13 @@ describe('localFilterCommands', () => {
     expect(screen.unreadable).toBeNull();
   });
 
-  it('does NOT follow an include that points OUT of the repository', () => {
-    // The include graph is walked by LOCATION. A repo-local `include.path`
-    // naming the user's own global config is their contract, exactly as merged
-    // config is for any git command they run — and `git lfs install` writes
-    // `filter.lfs.clean` there, so following it would put every contributor
-    // with git-lfs into permanent refusal. That is the failure this whole
-    // screen is scoped to avoid.
+  it('never reports the user\'s global keys through an include', () => {
+    // The screen does not follow includes at all — a declared limit, see the
+    // spawn's comment. This pins the half of it that MUST hold: an
+    // `include.path` naming the user's own global config never drags their
+    // `filter.lfs.*` into a repo-local refusal. `git lfs install` writes that
+    // key, so following the edge would put every contributor with git-lfs into
+    // permanent refusal — the failure this screen is scoped to avoid.
     const outside = mkdtempSync(join(tmpdir(), 'qwen-userconf-'));
     try {
       const userConfig = join(outside, 'gitconfig');
@@ -1955,13 +1955,11 @@ describe('localFilterCommands', () => {
     }
   });
 
-  it('does not report the user\'s global keys through a `~` include', () => {
-    // The one-line write a probe can make: `[include] path = ~/.gitconfig` in
-    // the never-wiped common config. `sanitizedGitEnv()` keeps HOME, so the
-    // tilde expands to the reviewing user's real config — and for anyone with
-    // git-lfs installed that holds `filter.lfs.clean`. Reporting it would
-    // refuse every later review of the repo, naming a key the local config
-    // does not define and whose removal breaks git-lfs everywhere else.
+  it('never reports global keys through a `~` include either', () => {
+    // Same rule through the tilde form: `[include] path = ~/.gitconfig` is the
+    // one-line write a probe can make into the never-wiped common config, and
+    // `sanitizedGitEnv()` keeps HOME so it would expand to the reviewing
+    // user's real config.
     const home = process.env['HOME'];
     expect(home).toBeTruthy();
     writeFileSync(
@@ -1979,27 +1977,6 @@ describe('localFilterCommands', () => {
     expect(screen.stopped).toBeNull();
   });
 
-  it('follows a conditional include whose condition is false HERE', () => {
-    // `includeIf` conditions are evaluated against whichever tree git runs in,
-    // and the checkout this screen authorises runs in a DIFFERENT tree — so a
-    // condition false for the screen and true for the checkout would hide a
-    // filter that then executes. Asking git to evaluate (`--includes`) cannot
-    // be right for a cross-tree checkout; the walk follows every in-repo
-    // target regardless of its condition.
-    writeFileSync(
-      join(dir, '.git', 'evil.inc'),
-      '[filter "evil"]\n\tsmudge = cat\n',
-    );
-    appendFileSync(
-      join(dir, '.git', 'config'),
-      '[includeIf "gitdir:/nowhere/that/matches/"]\n\tpath = evil.inc\n',
-    );
-
-    const screen = localFilterCommands(dir);
-
-    expect(screen.keys).toEqual(['filter.evil.smudge']);
-  });
-
   it('matches a filter key whose subsection carries an invalid UTF-8 byte', () => {
     // `--get-regexp` is POSIX matching and locale-sensitive: under a UTF-8
     // locale a key holding an invalid byte never matches, so git exits 1 and
@@ -2205,13 +2182,15 @@ describe('localFilterCommands', () => {
     expect(screen.unreadable).toBe(join(dir, '.git', 'config.worktree'));
   });
 
-  it('follows an include directive the checkout would follow', () => {
-    // `git config --file` does NOT expand `include.path` / `includeIf` on its
-    // own — git's documented default for a single-file read — while every
-    // checkout this screen authorises reads MERGED config, which does. A filter
-    // one include-hop behind a screened candidate would be invisible. The
-    // screen walks the include graph itself rather than passing `--includes`;
-    // the sibling cases below pin why.
+  it('does NOT see a filter one include-hop away — a declared limit', () => {
+    // `git config --file` does not expand includes and this screen does not
+    // walk them either, so a filter behind an include is invisible here while
+    // the checkout, reading merged config, would run it. That gap is real and
+    // deliberate: both ways of closing it were tried and both were worse (the
+    // spawn's comment records why), and #10441 carries the design that closes
+    // it properly. This test exists so the limit is visible rather than
+    // assumed — if a later change starts following includes, it goes red and
+    // whoever wrote it has to decide deliberately.
     writeFileSync(
       join(dir, '.git', 'evil.inc'),
       '[filter "evil"]\n\tsmudge = cat\n',
@@ -2223,10 +2202,10 @@ describe('localFilterCommands', () => {
 
     const screen = localFilterCommands(dir);
 
-    expect(screen.keys).toEqual(['filter.evil.smudge']);
-    expect(screen.total).toBe(1);
-    expect(screen.unreadable).toBeNull();
+    expect(screen.keys).toEqual([]);
+    expect(screen.stopped).toBeNull();
   });
+
 });
 
 describe('screenStopDetail', () => {
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index d9cbbc47c88..7708dbd5573 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -616,16 +616,19 @@ export const MAX_SCREEN_CANDIDATES = 256;
  * wipe the common dir, so a filter planted while reviewing one PR fires on
  * every later matching checkout of the user's OWN repository — persistence
  * planted by reviewing a malicious PR, measured live. The candidate config
- * files are read with `--file … --includes` rather than merged config: filters
- * in the user's global config (git-lfs is the common one) are the user's own
- * contract, exactly like any git command they run, so global config stays out
- * of scope — but `--includes` follows the `include.path` / `includeIf` chain a
- * screened file itself names, because the checkout does, and a filter hidden
- * one include-hop behind a screened candidate would otherwise be invisible to
- * the screen and executed by the checkout. Screening repo-local-only is a
- * deliberate TRADE, not a claim that repo-local files are all a probe can reach: probe
- * code runs as the user, so `git config --global` is open to it, and a filter
- * planted there is read by every checkout here and never seen by this screen.
+ * files are read with `--file`, one file at a time, rather than merged config:
+ * filters in the user's global config (git-lfs is the common one) are the
+ * user's own contract, exactly like any git command they run, so global config
+ * stays out of scope.
+ *
+ * TWO limits, both declared rather than discovered. Screening repo-local-only
+ * is a deliberate TRADE, not a claim that repo-local files are all a probe can
+ * reach: probe code runs as the user, so `git config --global` is open to it,
+ * and a filter planted there is read by every checkout here and never seen by
+ * this screen. And a filter reached only through an `include.path` /
+ * `includeIf` directive is not seen either — see the spawn below for why
+ * neither `--includes` nor a hand-walk is the answer, and #10441 for the
+ * design that is.
  * Refusing on merged config is not the answer — `git lfs install` writes
  * `filter.lfs.clean` globally, and refusing on that is permanent refusal for
  * every contributor who has git-lfs. The state cannot be told apart from a
@@ -757,72 +760,6 @@ export function carriesReplacementChar(value: string): boolean {
   return value.includes('\uFFFD');
 }
 
-/**
- * How many include hops the screen will follow before refusing.
- *
- * The graph is attacker-written and may be cyclic or merely deep; visited
- * targets are tracked, so this bounds work rather than correctness.
- */
-const MAX_INCLUDE_HOPS = 32;
-
-/**
- * The `include.path` / `includeIf.*.path` targets a candidate names, resolved.
- *
- * Read WITHOUT `--includes`, so git reports the directives this file literally
- * carries and evaluates no condition. Whether a conditional include would fire
- * is not a question this screen can answer — the condition is evaluated
- * against whichever tree git runs in, and the checkout runs in a different one
- * — so every target is followed regardless of its condition, and the decision
- * about which ones matter is made by LOCATION below, not by git.
- */
-function includeTargetsOf(
-  worktree: string,
-  file: string,
-): { targets: string[]; unreadable: boolean } {
-  const r = spawnSync(
-    'git',
-    ['config', '--file', file, '--get-regexp', '^include(If\\..*)?\\.path$'],
-    {
-      cwd: worktree,
-      encoding: 'utf8',
-      timeout: SCREEN_SPAWN_TIMEOUT_MS,
-      killSignal: 'SIGKILL',
-      env: { ...sanitizedGitEnv(), LC_ALL: 'C' },
-    },
-  );
-  if (r.error || (r.status !== 0 && r.status !== 1)) {
-    return { targets: [], unreadable: true };
-  }
-  if (r.status === 1 || typeof r.stdout !== 'string') {
-    return { targets: [], unreadable: false };
-  }
-  const targets: string[] = [];
-  for (const line of r.stdout.split('\n')) {
-    if (line.length === 0) continue;
-    // `key value`, and only the FIRST space separates them — a path may hold
-    // the rest, including further spaces.
-    const sp = line.indexOf(' ');
-    if (sp < 0) continue;
-    const raw = line.slice(sp + 1);
-    if (raw.length === 0) continue;
-    // git expands a leading `~` against HOME and resolves anything else
-    // against the including file's directory. Expand it the same way rather
-    // than skipping: a repository commonly lives under HOME, so `~/...` can
-    // name a target INSIDE it, and the location test below is what decides.
-    if (raw === '~' || raw.startsWith('~/')) {
-      const home = process.env['HOME'];
-      if (home === undefined || home.length === 0) continue;
-      targets.push(resolve(home, raw.slice(raw === '~' ? 1 : 2)));
-      continue;
-    }
-    // `~user/` is not expanded here — resolving another account's home is not
-    // this screen's business, and an unexpanded path simply is not followed.
-    if (raw.startsWith('~')) continue;
-    targets.push(resolve(dirname(file), raw));
-  }
-  return { targets, unreadable: false };
-}
-
 export function localFilterCommands(worktree: string): LocalFilterScreen {
   const commonDir = revParsePath(worktree, '--git-common-dir');
   const gitDir = revParsePath(worktree, '--git-dir');
@@ -854,8 +791,19 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
     };
   }
   const common = resolve(worktree, commonDir);
+  // `<common>/config` is read LAST, and the order is load-bearing. The
+  // checkout this screen authorises reads merged config at its own start, so
+  // everything between the screen's read of a file and that checkout is a
+  // window in which a plant can land in it. The admin-dir set below is
+  // attacker-AMPLIFIABLE — up to MAX_SCREEN_CANDIDATES one-byte filler
+  // entries, each costing spawns — so reading the common config first put the
+  // most likely plant target at the START of a walk the plant itself can
+  // stretch to seconds. Measured: 256 fillers stretched the walk to ~9 s, and
+  // a blind planter won. Reading it last leaves only the walk's own tail
+  // between it and the checkout. This does not close the window — nothing
+  // here can, since a checkout either reads merged config or does not run —
+  // it removes the amplification.
   const candidates = [
-    join(common, 'config'),
     join(resolve(worktree, gitDir), 'config.worktree'),
   ];
   // Every OTHER worktree's per-worktree config too. This screen runs against
@@ -903,42 +851,11 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
   for (const entry of worktreeEntries ?? []) {
     candidates.push(join(common, 'worktrees', entry, 'config.worktree'));
   }
-  // Anything under these is the repository's own state, which a probe can
-  // write and cleanup never wipes. Anything outside them is the user's — their
-  // global config is their contract exactly as it is for any git command they
-  // run, and `git lfs install` puts `filter.lfs.clean` there. So an include
-  // whose target lands inside is followed and screened; one pointing out is
-  // left alone. That is the whole rule, and it is a rule about LOCATION, which
-  // is the same in every tree — unlike an `includeIf` condition, which is not.
-  const repoRoots = [common, resolve(worktree, gitDir), resolve(worktree)].map(
-    (d) => {
-      try {
-        return realpathSync(d);
-      } catch {
-        return d;
-      }
-    },
-  );
-  const insideRepo = (p: string): boolean => {
-    let real = p;
-    try {
-      real = realpathSync(dirname(p)) + sep + basename(p);
-    } catch {
-      // Not yet resolvable: judge the literal path rather than skipping it.
-    }
-    return repoRoots.some(
-      (root) => real === root || real.startsWith(root + sep),
-    );
-  };
-
+  // Last, per the note above the list.
+  candidates.push(join(common, 'config'));
   const found = new Set<string>();
   let unreadable: string | null = null;
-  // Seeded with the candidates themselves, so an include pointing back at
-  // one of them is not walked twice.
-  const seen = new Set<string>(candidates);
-  const queue = [...candidates];
-  let hops = 0;
-  for (const file of queue) {
+  for (const file of candidates) {
     if (!existsSync(file)) continue;
     // Existence is not readability, and git does not distinguish them for us:
     // an unreadable `--file` exits 1 with a warning on stderr — byte-identical
@@ -964,14 +881,26 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
         'config',
         '--file',
         file,
-        // NO `--includes`. It looks like the honest read — the checkout does
-        // expand includes — but it makes git EVALUATE `includeIf` conditions
-        // in this process's context, and the checkout this authorises runs in
-        // a different tree. A condition false here and true there hides a
-        // filter from the screen that the checkout then executes, and
-        // `includeIf.hasconfig:remote.*.url:` never fires under `--file` at
-        // all. There is no context that matches a cross-tree checkout, so the
-        // include graph is walked explicitly below instead of delegated.
+        // NO `--includes`, and no hand-walk of the include graph either —
+        // this screen does not follow includes at all, and that is a declared
+        // limit rather than an oversight. Both alternatives were tried and
+        // both were worse. `--includes` makes git EVALUATE `includeIf` in
+        // THIS process's context while the checkout runs in another tree, so a
+        // condition false here and true there hides a filter the checkout then
+        // executes; it also follows an edge out of the repository, dragging
+        // the user's own `filter.lfs.*` into a repo-local screen and refusing
+        // every later review. Walking the graph by hand instead means
+        // re-implementing git's own path resolution — `%(prefix)`, `~//`,
+        // `~user`, symlinked components, non-UTF-8 targets — and eleven
+        // divergences from git were measured across five review rounds, each
+        // one a silent skip. The condition problem survives both: no single
+        // context matches a cross-tree checkout.
+        //
+        // So a filter reached only through an include is NOT screened here.
+        // Issue #9558 scopes it out in as many words ("`include.path` /
+        // `includeIf` ... are each handled separately today"), and #10441
+        // carries the design that closes it — resolving each hit's origin the
+        // way git resolves it, which is a different change with its own tests.
         // BEFORE the pattern: `--name-only` after it silently prints nothing
         // and exits 1 even with live keys. With it, each line is the whole key
         // and nothing has to be parsed out of a `key value` pair — a config
@@ -1012,24 +941,6 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
       unreadable ??= file;
       continue;
     }
-    // Follow this file's includes, but only where they land inside the
-    // repository. git expands them for the checkout regardless; the screen
-    // declines to chase one into the user's own configuration, which it must
-    // not refuse on.
-    if (hops < MAX_INCLUDE_HOPS) {
-      const inc = includeTargetsOf(worktree, file);
-      if (inc.unreadable) unreadable ??= file;
-      for (const target of inc.targets) {
-        if (!insideRepo(target) || seen.has(target)) continue;
-        seen.add(target);
-        hops += 1;
-        if (hops > MAX_INCLUDE_HOPS) {
-          unreadable ??= file;
-          break;
-        }
-        queue.push(target);
-      }
-    }
     if (r.status === 1 || typeof r.stdout !== 'string') continue;
     for (const line of r.stdout.split('\n')) {
       const key = line;
diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts
index 1e779343308..407fc4bd5e7 100644
--- a/packages/cli/src/commands/review/test-efficacy.ts
+++ b/packages/cli/src/commands/review/test-efficacy.ts
@@ -3029,10 +3029,15 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise<void> {
         // measured in seconds between the last read and the checkout, and the
         // probe's own test code can spawn a detached process that plants the
         // filter inside it. Moving the screen below the loop leaves only the
-        // checkout spawn in the gap. The residual window is sub-millisecond
-        // and cannot be closed from here: there is no `-c` kill switch for
+        // checkout spawn in the gap. A residual window remains and cannot be
+        // closed from here — there is no `-c` kill switch for
         // attribute-selected filters, so a checkout either reads merged config
-        // or does not run.
+        // or does not run. It is NOT sub-millisecond: the screen is a walk of
+        // several spawns, measured at 56-100 ms on a repository with no linked
+        // worktrees, and a plant can stretch it with filler admin entries. The
+        // screen reads `<common>/config` last for that reason (see
+        // `localFilterCommands`), which removes the amplification but not the
+        // window.
         const revertFilters = localFilterCommands(probeTree);
         // Gate on `stopped`, matching the restore and scratch-tree — a screen
         // that stopped for any reason did not finish, so the checkout must not

From 09c2522c9de51c76e744c1cc34a0b4b9cbe6ce0c Mon Sep 17 00:00:00 2001
From: wenshao <shaojin.wensj@alibaba-inc.com>
Date: Wed, 2 Sep 2026 09:07:00 +0800
Subject: [PATCH 14/15] test(review): witness the read order, and guard the
 POSIX-only fixtures
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Self-review of the round-9 change turned up four things it had left behind.

The `<common>/config`-last ordering had no witness — reverting it shipped
green. Pinned now through a `git` shim on PATH that logs each `--file` read,
asserting the common config is the last of them. Deterministic; racing it
would not be.

Two fixtures I added in earlier rounds would have been red on the Windows
lane, for the same reason the round-5 one was: the FIFO test shells out to
`mkfifo`, and the new shim test to `which` plus a `#!/bin/sh` script. Neither
exists there. Both now carry `skipIf(win32)` — and unlike the round-5 case,
both do run here, so the guard hides nothing I have not seen pass.

Two comments still described `--includes`, two rounds after it was replaced
and one after it was removed: the interface docstring on `unreadable`, and the
`LocalFilterScreen` header. Corrected to what the code does.
---
 .../src/commands/review/lib/worktree.test.ts  | 78 ++++++++++++++++---
 .../cli/src/commands/review/lib/worktree.ts   |  6 +-
 2 files changed, 69 insertions(+), 15 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index 1ce65e58530..b59aa09f8c6 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -2018,22 +2018,28 @@ describe('localFilterCommands', () => {
     expect(carriesReplacementChar('\uFFFD')).toBe(true);
   });
 
-  it('refuses rather than hanging when an include names a FIFO', () => {
-    // git follows includes at startup, so the very first `rev-parse` blocks on
-    // a FIFO nobody writes to — and `spawnSync` blocks the event loop, so no
-    // JS timer can interrupt it. The bound has to be on the spawn, and a
-    // killed discovery must not read as "not a usable repository".
-    const fifo = join(dir, '.git', 'evil.fifo');
-    execFileSync('mkfifo', [fifo]);
-    appendFileSync(
+  // POSIX-only: `mkfifo` does not exist on Windows. The bound it pins is
+  // platform-independent; only this way of provoking a block is not.
+  it.skipIf(process.platform === 'win32')(
+    'refuses rather than hanging when an include names a FIFO',
+    () => {
+      // git follows includes at startup, so the very first `rev-parse` blocks on
+      // a FIFO nobody writes to — and `spawnSync` blocks the event loop, so no
+      // JS timer can interrupt it. The bound has to be on the spawn, and a
+      // killed discovery must not read as "not a usable repository".
+      const fifo = join(dir, '.git', 'evil.fifo');
+      execFileSync('mkfifo', [fifo]);
+      appendFileSync(
       join(dir, '.git', 'config'),
       `[include]\n\tpath = ${fifo}\n`,
-    );
+      );
 
-    const screen = localFilterCommands(dir);
+      const screen = localFilterCommands(dir);
 
-    expect(screen.stopped).toBe('unreadable');
-  }, 60_000);
+      expect(screen.stopped).toBe('unreadable');
+    },
+    60_000,
+  );
 
   it('caps the reported keys and keeps the pre-cap total', () => {
     // The keys come from an attacker-writable file that cleanup never wipes;
@@ -2053,6 +2059,54 @@ describe('localFilterCommands', () => {
     expect(screen.keys[0]).toMatch(/^filter\.evil\d+\.smudge$/);
   });
 
+  // POSIX-only: the shim is a `#!/bin/sh` script found through `which`, and
+  // neither works on Windows. The ORDER it pins is platform-independent, so
+  // skipping there loses the witness, not the behaviour.
+  it.skipIf(process.platform === 'win32')(
+    'reads `<common>/config` LAST, after the amplifiable admin set',
+    () => {
+      // Order is load-bearing, not cosmetic. The checkout this screen authorises
+      // reads merged config at its start, so the gap between the screen's read
+      // of a file and that checkout is a window a plant can land in. The
+      // admin-dir set is attacker-amplifiable — filler entries cost spawns and
+      // stretch the walk — so reading the common config first would put the
+      // likeliest plant target at the START of a window the plant itself can
+      // widen. Pinned through a `git` shim on PATH, which is deterministic;
+      // racing it would not be.
+      const shimDir = mkdtempSync(join(tmpdir(), 'qwen-shim-'));
+      const log = join(shimDir, 'calls.log');
+      const realGit = execFileSync('which', ['git'], { encoding: 'utf8' }).trim();
+      writeFileSync(
+      join(shimDir, 'git'),
+      `#!/bin/sh\nprintf '%s\\n' "$*" >> ${log}\nexec ${realGit} "$@"\n`,
+      );
+      chmodSync(join(shimDir, 'git'), 0o755);
+      for (const e of ['w1', 'w2']) {
+      mkdirSync(join(dir, '.git', 'worktrees', e), { recursive: true });
+      writeFileSync(join(dir, '.git', 'worktrees', e, 'config.worktree'), '');
+      }
+      writeFileSync(join(dir, '.git', 'config.worktree'), '');
+      const savedPath = process.env['PATH'];
+      try {
+      process.env['PATH'] = `${shimDir}:${savedPath ?? ''}`;
+
+      localFilterCommands(dir);
+
+      const reads = readFileSync(log, 'utf8')
+        .split('\n')
+        .filter((l) => l.includes('--file'))
+        .map((l) => l.split(' --file ')[1]?.split(' ')[0] ?? '');
+      const commonAt = reads.indexOf(join(dir, '.git', 'config'));
+      expect(commonAt).toBeGreaterThanOrEqual(0);
+      // Every other candidate was read before it.
+      expect(commonAt).toBe(reads.length - 1);
+      } finally {
+        process.env['PATH'] = savedPath;
+        rmSync(shimDir, { recursive: true, force: true });
+      }
+    },
+  );
+
   it("refuses past the candidate cap instead of walking a plant's filler", () => {
     // Each entry costs one synchronous `git config` spawn, on every screen
     // call — once per mutant. Skipping past the bound would let filler hide a
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index 7708dbd5573..aef1a30c5f4 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -659,9 +659,9 @@ export interface LocalFilterScreen {
    * stopped it — otherwise null.
    *
    * Exit 1 is git's ordinary "no key matched" and is not a failure. Anything
-   * else is: a spawn error, a config git could not parse to the end (a
-   * malformed section, or an include it could not follow), or an ENOBUFS from
-   * more filter keys than the buffer holds. Skipping such a file reports the
+   * else is: a spawn error, a spawn killed at the timeout, a config git could
+   * not parse to the end (a malformed section header exits 128), or an ENOBUFS
+   * from more filter keys than the buffer holds. Skipping such a file reports the
    * repository clean on the one file that might define the filter, so the caller
    * refuses instead. One file, not a list: the refusal only has to name where
    * the screen stopped, and an unbounded join is its own denial-of-service.

From 0a410029b68a96079a0b8ae59615ab7794988eea Mon Sep 17 00:00:00 2001
From: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Date: Thu, 3 Sep 2026 08:23:39 +0000
Subject: [PATCH 15/15] fix(cli): stop the filter screen's second discovery
 spawn doubling its stall (#10421)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`localFilterCommands` ran `git rev-parse --git-common-dir` and `--git-dir`
unconditionally, but a null `commonDir` already decides the refusal below
whatever `gitDir` would have answered. Both read config at git startup, so a
planted include naming a FIFO blocks each for the whole 20s spawn timeout:
40s of synchronous event-loop block, measured (both rev-parse calls killed at
the timeout, ETIMEDOUT/SIGKILL).

That is most of vitest's fixed 60s worker->main `onTaskUpdate` RPC budget, and
on the Linux lane — where the unhandled-error exemption is deliberately off —
an RPC timeout exits an all-green suite non-zero. The FIFO test this PR adds is
the only such stall in packages/cli, and the shape is the one the ubuntu Test
lane and the verification gate both reported: 6955 tests passed, no FAIL line,
2 unhandled errors, exit 1.

Skip the second spawn once the first fails (40s -> 20s, refusal unchanged) and
pin it with a timing assertion in the FIFO test: removing the guard turns that
assertion red at 40042ms.
---
 .../src/commands/review/lib/worktree.test.ts  | 50 +++++++++++--------
 .../cli/src/commands/review/lib/worktree.ts   | 14 ++++--
 2 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts
index b59aa09f8c6..d05b19f8200 100644
--- a/packages/cli/src/commands/review/lib/worktree.test.ts
+++ b/packages/cli/src/commands/review/lib/worktree.test.ts
@@ -1927,7 +1927,7 @@ describe('localFilterCommands', () => {
     expect(screen.unreadable).toBeNull();
   });
 
-  it('never reports the user\'s global keys through an include', () => {
+  it("never reports the user's global keys through an include", () => {
     // The screen does not follow includes at all — a declared limit, see the
     // spawn's comment. This pins the half of it that MUST hold: an
     // `include.path` naming the user's own global config never drags their
@@ -2030,13 +2030,20 @@ describe('localFilterCommands', () => {
       const fifo = join(dir, '.git', 'evil.fifo');
       execFileSync('mkfifo', [fifo]);
       appendFileSync(
-      join(dir, '.git', 'config'),
-      `[include]\n\tpath = ${fifo}\n`,
+        join(dir, '.git', 'config'),
+        `[include]\n\tpath = ${fifo}\n`,
       );
 
+      const started = Date.now();
       const screen = localFilterCommands(dir);
+      const elapsed = Date.now() - started;
 
       expect(screen.stopped).toBe('unreadable');
+      // One spawn timeout, not two. A null `commonDir` already decides the
+      // refusal above, so re-running discovery for `gitDir` only doubles the
+      // synchronous block: 40s measured, most of vitest's fixed 60s worker RPC
+      // budget, which on the Linux lane exits an all-green suite red.
+      expect(elapsed).toBeLessThan(30_000);
     },
     60_000,
   );
@@ -2075,31 +2082,33 @@ describe('localFilterCommands', () => {
       // racing it would not be.
       const shimDir = mkdtempSync(join(tmpdir(), 'qwen-shim-'));
       const log = join(shimDir, 'calls.log');
-      const realGit = execFileSync('which', ['git'], { encoding: 'utf8' }).trim();
+      const realGit = execFileSync('which', ['git'], {
+        encoding: 'utf8',
+      }).trim();
       writeFileSync(
-      join(shimDir, 'git'),
-      `#!/bin/sh\nprintf '%s\\n' "$*" >> ${log}\nexec ${realGit} "$@"\n`,
+        join(shimDir, 'git'),
+        `#!/bin/sh\nprintf '%s\\n' "$*" >> ${log}\nexec ${realGit} "$@"\n`,
       );
       chmodSync(join(shimDir, 'git'), 0o755);
       for (const e of ['w1', 'w2']) {
-      mkdirSync(join(dir, '.git', 'worktrees', e), { recursive: true });
-      writeFileSync(join(dir, '.git', 'worktrees', e, 'config.worktree'), '');
+        mkdirSync(join(dir, '.git', 'worktrees', e), { recursive: true });
+        writeFileSync(join(dir, '.git', 'worktrees', e, 'config.worktree'), '');
       }
       writeFileSync(join(dir, '.git', 'config.worktree'), '');
       const savedPath = process.env['PATH'];
       try {
-      process.env['PATH'] = `${shimDir}:${savedPath ?? ''}`;
-
-      localFilterCommands(dir);
-
-      const reads = readFileSync(log, 'utf8')
-        .split('\n')
-        .filter((l) => l.includes('--file'))
-        .map((l) => l.split(' --file ')[1]?.split(' ')[0] ?? '');
-      const commonAt = reads.indexOf(join(dir, '.git', 'config'));
-      expect(commonAt).toBeGreaterThanOrEqual(0);
-      // Every other candidate was read before it.
-      expect(commonAt).toBe(reads.length - 1);
+        process.env['PATH'] = `${shimDir}:${savedPath ?? ''}`;
+
+        localFilterCommands(dir);
+
+        const reads = readFileSync(log, 'utf8')
+          .split('\n')
+          .filter((l) => l.includes('--file'))
+          .map((l) => l.split(' --file ')[1]?.split(' ')[0] ?? '');
+        const commonAt = reads.indexOf(join(dir, '.git', 'config'));
+        expect(commonAt).toBeGreaterThanOrEqual(0);
+        // Every other candidate was read before it.
+        expect(commonAt).toBe(reads.length - 1);
       } finally {
         process.env['PATH'] = savedPath;
         rmSync(shimDir, { recursive: true, force: true });
@@ -2259,7 +2268,6 @@ describe('localFilterCommands', () => {
     expect(screen.keys).toEqual([]);
     expect(screen.stopped).toBeNull();
   });
-
 });
 
 describe('screenStopDetail', () => {
diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts
index aef1a30c5f4..96a2f406c17 100644
--- a/packages/cli/src/commands/review/lib/worktree.ts
+++ b/packages/cli/src/commands/review/lib/worktree.ts
@@ -762,7 +762,15 @@ export function carriesReplacementChar(value: string): boolean {
 
 export function localFilterCommands(worktree: string): LocalFilterScreen {
   const commonDir = revParsePath(worktree, '--git-common-dir');
-  const gitDir = revParsePath(worktree, '--git-dir');
+  // The second discovery spawn is skipped once the first fails. Both read
+  // config at git startup, so a plant that blocks one blocks the other for the
+  // whole `SCREEN_SPAWN_TIMEOUT_MS`, and a null `commonDir` already decides the
+  // refusal below whatever `gitDir` would have answered. Running both anyway
+  // doubled that stall to a measured 40s of blocked event loop — most of
+  // vitest's fixed 60s worker RPC budget, which on the Linux lane (no
+  // unhandled-error exemption) exits an all-green suite red.
+  const gitDir =
+    commonDir === null ? null : revParsePath(worktree, '--git-dir');
   if (commonDir === null || gitDir === null) {
     // Two very different reasons land here, and only one is benign. A plain
     // "not a repository" answer is the caller's own problem — its checkout has
@@ -803,9 +811,7 @@ export function localFilterCommands(worktree: string): LocalFilterScreen {
   // between it and the checkout. This does not close the window — nothing
   // here can, since a checkout either reads merged config or does not run —
   // it removes the amplification.
-  const candidates = [
-    join(resolve(worktree, gitDir), 'config.worktree'),
-  ];
+  const candidates = [join(resolve(worktree, gitDir), 'config.worktree')];
   // Every OTHER worktree's per-worktree config too. This screen runs against
   // the review worktree, but the checkout it authorises runs in the SCRATCH
   // tree, whose own `<common>/worktrees/<label>/config.worktree` is honored