E2E Report — deflake review test-efficacy tests

Flaky mechanism

packages/cli/src/commands/review/test-efficacy.test.ts had two environment-dependent tests, both failing on CI with AssertionError: expected [Function] to throw an error (run 30909672420, ubuntu-latest job; same signature as latest main run 30893612230):

• findVitestBin > names the search root when vitest cannot be resolved — expects findVitestBin(<bare tmpdir>) to throw MODULE_NOT_FOUND.
• runControlMutant > leaves the probe file byte-identical when it cannot run — expects the vitest run to never start, so the function throws out of the suite helper.

Both assumed vitest/package.json is not resolvable up-tree from os.tmpdir(). On the project's self-hosted runner, a node_modules above TMPDIR provides vitest, so resolution succeeds: test 1 never throws, and test 2 runs the probe for real (no throw). On macOS dev machines (/var/folders/... has nothing above it) both pass — the classic environment-dependent flake.

Which fix was applied and why

Allowed-fix class 4 (isolate/serialize interference): remove the dependence on ambient filesystem state, preserving every assertion verbatim.

1. findVitestBin now accepts an optional injected resolver (default is the unchanged createRequire(join(worktree, 'noop.js')).resolve). The test injects a resolver that throws MODULE_NOT_FOUND directly. A vi.doMock('node:module') alternative was prototyped and empirically rejected — builtin imports inside user modules are externalized by this repo's vite-node setup and bypass the mock registry (verified with a throwaway probe test).
2. The runControlMutant test plants a shadow node_modules/vitest inside the probe dir whose exports map hides package.json. The innermost node_modules wins Node resolution on every host, so findVitestBin surfaces ERR_PACKAGE_PATH_NOT_EXPORTED, runControlMutant throws through the same finally restore path, and the probe file stays byte-identical — on machines with and without an ambient vitest alike.

No assertion was weakened, skipped, or deleted; the production call path is byte-for-byte the default.

Repeated-run evidence

A/B reproduction harness on macOS: TMPDIR=/tmp/qwen-fake-tmp with node_modules/vitest symlinked to the repo's real vitest — reproduces the CI condition locally.

• Old code + repro env: both tests FAIL (matches CI signature exactly).
• New code + repro env: full file 128/128 pass.
• New code + normal env: full file 128/128 pass, 4 consecutive runs (3 normal + 1 repro).
• npm run typecheck (packages/cli), ESLint, Prettier: all clean.

中文说明

失败机制

packages/cli/src/commands/review/test-efficacy.test.ts 有两个依赖环境的测试，在 CI 上以 AssertionError: expected [Function] to throw an error 失败（run 30909672420，ubuntu-latest；与最新 main run 30893612230 同签名）：两个用例都假设从 os.tmpdir() 向上解析不到 vitest/package.json。项目自托管 runner 上 TMPDIR 之上的 node_modules 提供了 vitest，解析成功 → 用例 1 不抛错，用例 2 真实执行了探针（也不抛错）。macOS 开发机上 /var/folders/... 上方无 vitest，所以通过——典型的环境依赖型 flaky。

修复方式

断言逐字保留，消除对环境文件系统状态的依赖：

1. findVitestBin 增加可选注入解析器（默认行为不变），测试直接注入抛 MODULE_NOT_FOUND 的解析器。vi.doMock('node:module') 备选方案经探针实验证明在此仓库 vite-node 配置下无效（builtin 导入被 externalize，绕过 mock 注册表）。
2. runControlMutant 测试在探针目录内种植一个 exports 不含 package.json 的 shadow vitest 包；最内层 node_modules 在任何宿主上都赢得解析，findVitestBin 抛出 ERR_PACKAGE_PATH_NOT_EXPORTED，runControlMutant 经同一 finally 恢复路径抛错，探针文件保持字节一致。

未弱化、跳过或删除任何断言；生产调用路径与默认值完全一致。

重复运行证据

macOS 本地 A/B 复现：以 TMPDIR=/tmp/qwen-fake-tmp 并在其中 symlink 仓库真实 vitest，复现 CI 条件。旧代码 + 复现环境：两个测试都失败（与 CI 签名完全一致）；新代码 + 复现环境：整文件 128/128 通过；新代码 + 常规环境：整文件 128/128 通过，连续 4 轮。packages/cli typecheck、ESLint、Prettier 全部通过。

Code Review — #8537

Verdict: LGTM, approve with two minor suggestions. I reproduced the CI condition and verified every claim in the PR body independently.

What it does

Two tests in test-efficacy.test.ts produced their failure condition by relying on os.tmpdir() having no vitest/package.json anywhere up-tree. That is true on macOS dev machines and false on the self-hosted ubuntu runner, where a node_modules above TMPDIR resolves one. The PR replaces the ambient dependency with two host-deterministic constructions: a MODULE_NOT_FOUND-throwing resolver injected via a new optional findVitestBin parameter, and a planted shadow vitest whose exports hides package.json.

Verification I ran

Built the fake-TMPDIR harness from the test plan (node_modules/vitest symlink above TMPDIR), then ran the file in a clean worktree of the PR head:

Mutation check — the reworked test still has teeth. I broke the production invariant it guards, moving the restore in runControlMutant out of finally onto the success path only:

const __v = perFile.some((r) => r.verdict === 'gated');
writeFileSync(abs, original, 'utf8');
return __v;
} finally { /* no longer survives the throw path */ }

→ × leaves the probe file byte-identical when it cannot run. The shadow-package rewrite did not weaken the assertion's ability to catch a restore that doesn't survive the throw. This was my main concern with the change and it is settled.

Production path is untouched. findVitestBin has exactly one production call site (test-efficacy.ts:1477), called with one argument, so the default resolver runs. createRequire(...).resolve extracted unbound is safe — Node's resolve closes over the module rather than using this; confirmed it still throws MODULE_NOT_FOUND correctly when detached. The default is also still exercised for real by the two sibling findVitestBin tests (declares no bin, hides its package.json), so the anchor itself remains covered.

Convention fit. The optional-parameter seam matches what this file already does — runControlMutant(…, now = Date.now), runOneMutant(…, dependencyRoot = probeTree). Consistent, not a new pattern.

Side benefit worth noting: the reworked test 2 now throws before any spawn, dropping from ~604ms (it was actually executing a real vitest run under the ambient-vitest condition) to ~10ms.

Suggestions

1. Pin test 2's assertion to the error the comment promises (test-efficacy.test.ts:349)

The comment states the point precisely — "findVitestBin surfaces the ERR_PACKAGE_PATH_NOT_EXPORTED" — but the assertion is a bare toThrow(). Since the entire change is about which throw the shadow package produces, a bare match doesn't hold that. Anything that throws earlier in a later refactor (probe-tree validation, a spawn failure, even a typo in the planted JSON) keeps this green while silently no longer testing the described path.

-      expect(() => runControlMutant(dir, 'a.test.ts')).toThrow();
+      expect(() => runControlMutant(dir, 'a.test.ts')).toThrow(
+        /not defined by "exports"/,
+      );

I applied this and it passes — the throw genuinely is that one. It also matches the sibling keeps the real error when vitest is present but hides its package.json test, which already asserts that exact regex.

2. mkdtempSync in test 1 is now dead weight (test-efficacy.test.ts:185)

With the resolver injected, worktree is only interpolated into the expected message — the directory is never touched. I confirmed the test passes with a path that does not exist at all. The real mkdtempSync now just leaks an unremoved no-vitest-* dir per run (verified against a scratch TMPDIR). Leaking is a pre-existing pattern in this file, but here it's newly pointless:

-    const worktree = mkdtempSync(join(tmpdir(), 'no-vitest-'));
+    // Never touched on disk — the injected resolver decides the outcome and
+    // the path only has to appear in the message.
+    const worktree = join(tmpdir(), 'no-vitest-unused');

(Alternatively keep mkdtempSync and wrap in try/finally { rmSync } like the runControlMutant tests do — but the dir buys nothing here.)

Note, not a defect

After this change nothing verifies the link "a genuine require.resolve failure carries code === 'MODULE_NOT_FOUND', therefore this branch fires." The branch body is covered via injection; its real-world trigger is now assumed. That is unavoidable — you cannot force a genuine not-found on a host with ambient vitest, and MODULE_NOT_FOUND is a stable Node contract. Worth one sentence in the findVitestBin JSDoc (which still describes only the createRequire anchor) so the next reader knows the second parameter is a test-only seam and why it exists.

Risk / security / performance

No production behavior change, no new dependency, no I/O outside per-test temp dirs, and the planted shadow package lives inside a mkdtemp tree removed in finally. Net performance is a small improvement. Nothing to flag.

@qwen-code /takeover

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.