From 0b60d24e104fd07d2651a062eb38f25ff81b6127 Mon Sep 17 00:00:00 2001 From: wenshao Date: Tue, 14 Jul 2026 00:05:31 +0800 Subject: [PATCH] refactor(review): run the test-efficacy probe in a disposable worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The probe used to revert the PR's source to base IN the shared review worktree and restore it in a `finally`. That shared tree is the one every Step 3 review agent reads, and the in-place mutate/restore was the root of two findings on #6790: - a concurrent reader could observe the tree half-reverted to base for the probe's whole duration (Critical), and the later restore cannot un-produce a finding written from the wrong source; - the restore's in-place delete followed a PR-controlled symlink out of the tree and removed an outside file (P0, band-aided with `safeRmWithin`). Both share one cause — mutating a live, shared tree — and one fix retires both. The probe now runs in its OWN disposable worktree, checked out at the PR head as a sibling of the shared one (`.qwen/tmp/review-pr--probe`) and removed wholesale when it finishes: - the shared tree is never touched, so no reader can see a reverted state; - there is no in-place restore, so the delete that followed a symlink is gone with it — `safeRmWithin` stays only as belt-and-suspenders on the revert-phase delete of added files; - `node_modules` needs no per-tree install: the probe tree is nested under the repo, so `npx vitest` resolves upward to the repo-root `node_modules`, exactly as the shared worktree already does. (Confirmed empirically before relying on it — this is what had the refactor deferred.) Because the shared tree is no longer mutated, the dirty-worktree guard is gone (nothing the caller has uncommitted is ever discarded), and the loud `restoreFailure` / non-zero exit becomes a soft `cleanupFailure` warning: a leftover probe worktree does not corrupt anything and is swept at the next run's `worktree add` and by `cleanup.ts`. Verified by driving the real handler (new `test-efficacy.integration.test.ts`, real git worktrees, a stub vitest bin): verdicts are unchanged (gated/inert), the shared tree is byte-identical before and after, the probe tree is always discarded, and the symlink P0 repro leaves the outside file intact WITHOUT `safeRmWithin` having to refuse — isolation alone protects it. Closes #6832. --- packages/cli/src/commands/review/cleanup.ts | 9 + .../review/test-efficacy.integration.test.ts | 218 ++++++++++++++++ .../cli/src/commands/review/test-efficacy.ts | 244 ++++++++---------- 3 files changed, 338 insertions(+), 133 deletions(-) create mode 100644 packages/cli/src/commands/review/test-efficacy.integration.test.ts diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 6a7e067b944..21188a6b574 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -44,6 +44,15 @@ function runCleanup(target: string): void { removedAny = true; } + // The test-efficacy probe runs in a disposable sibling worktree and removes + // it itself; sweep one a crashed probe left behind so it does not block the + // next run's `git worktree add` (see #6832 / test-efficacy.ts). + const probeWt = `${wt}-probe`; + if (releaseWorktree(probeWt)) { + writeStdoutLine(`Removed probe worktree: ${probeWt}`); + removedAny = true; + } + const branch = reviewBranch(prNumber); if (refExists(branch)) { try { diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts new file mode 100644 index 00000000000..962dc3a4b36 --- /dev/null +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -0,0 +1,218 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real `git` and a real `git worktree`. The property under test — that the +// probe runs in its OWN disposable worktree and never mutates the shared one +// (#6832) — lives entirely in git's bookkeeping, so a mocked child_process +// would prove nothing. `vitest` itself is stubbed by a fake bin (below): the +// verdict logic is unit-tested in `classifyProbeRun`; what these lock down is +// where the probe runs and what it leaves behind. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + mkdirSync, + writeFileSync, + readFileSync, + chmodSync, + rmSync, + existsSync, + symlinkSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { testEfficacyCommand } from './test-efficacy.js'; + +type Handler = (args: { + report: string; + worktree: string; + base: string; + out: string; +}) => Promise; +const runHandler = testEfficacyCommand.handler as unknown as Handler; + +let repo: string; +let outside: string; + +function git(cwd: string, ...args: string[]): string { + return execFileSync('git', args, { cwd, encoding: 'utf8' }); +} +function commitAll(msg: string): string { + git(repo, 'add', '-A'); + git( + repo, + '-c', + 'user.email=a@b', + '-c', + 'user.name=a', + 'commit', + '-q', + '-m', + msg, + ); + return git(repo, 'rev-parse', 'HEAD').trim(); +} +function write(rel: string, body: string) { + const abs = join(repo, rel); + mkdirSync(join(abs, '..'), { recursive: true }); + writeFileSync(abs, body); +} +/** The staged tree of a worktree — changes iff the working tree was mutated. */ +function treeState(wt: string): string { + return ( + git(wt, 'status', '--porcelain', '-z') + '|' + git(wt, 'rev-parse', 'HEAD') + ); +} + +beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'efficacy-iso-')); + outside = mkdtempSync(join(tmpdir(), 'efficacy-outside-')); + git(repo, 'init', '-q', '-b', 'main', '.'); + + // A fake `vitest` on the up-tree bin path so `npx vitest` in the probe tree + // resolves locally — fast, deterministic, no network. It echoes each test + // file it is handed back as PASSED, so a probe over reverted source reads as + // `inert` without a real runner. `npx` walks node_modules upward, and the + // probe tree is a direct child of `repo`, so this bin is what it finds. + mkdirSync(join(repo, 'node_modules', '.bin'), { recursive: true }); + const bin = join(repo, 'node_modules', '.bin', 'vitest'); + writeFileSync( + bin, + `#!/usr/bin/env node +const path = require('path'); +const files = process.argv.slice(2).filter((a) => a.includes('.test.')); +process.stdout.write(JSON.stringify({ + numPassedTests: files.length, + numFailedTests: 0, + testResults: files.map((f) => ({ + name: path.resolve(f), + assertionResults: [{ status: 'passed' }], + })), +})); +`, + ); + chmodSync(bin, 0o755); +}); + +afterEach(() => { + // The handler removes its own probe tree; force-remove any a failed test left. + try { + git(repo, 'worktree', 'remove', '--force', join(repo, 'wt-probe')); + } catch { + // not there — the normal case + } + rmSync(repo, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); +}); + +describe('test-efficacy probe isolation (#6832)', () => { + it('probes in a disposable worktree and never mutates the shared one', async () => { + write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); + write('packages/lib/src/f.ts', 'export const f = () => 1;\n'); + const base = commitAll('base'); + write('packages/lib/src/f.ts', 'export const f = () => 2;\n'); + write( + 'packages/lib/src/f.test.ts', + 'import { f } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof f).toBe("function"));\n', + ); + commitAll('pr'); + + const wt = join(repo, 'wt'); + git(repo, 'worktree', 'add', '-q', '--detach', wt, 'HEAD'); + writeFileSync( + join(repo, 'report.json'), + JSON.stringify({ + files: [ + { path: 'packages/lib/src/f.ts', kind: 'source' }, + { path: 'packages/lib/src/f.test.ts', kind: 'test' }, + ], + }), + ); + + const before = treeState(wt); + await runHandler({ + report: join(repo, 'report.json'), + worktree: wt, + base, + out: join(repo, 'out.json'), + }); + + // The shared worktree the other review agents read is byte-identical: no + // in-place revert was ever visible in it. + expect(treeState(wt)).toBe(before); + expect(readFileSync(join(wt, 'packages/lib/src/f.ts'), 'utf8')).toBe( + 'export const f = () => 2;\n', + ); + // The probe tree was created and discarded. + expect(existsSync(join(repo, 'wt-probe'))).toBe(false); + // And the probe still produced its verdict from the isolated tree: the test + // passed with the source reverted, so it is inert. + const out = JSON.parse(readFileSync(join(repo, 'out.json'), 'utf8')); + expect(out.findings.map((f: { file: string }) => f.file)).toContain( + 'packages/lib/src/f.test.ts', + ); + expect(out.cleanupFailure).toBeUndefined(); + }); + + it('a PR-controlled symlink cannot delete outside the tree — by isolation, not the guard', async () => { + writeFileSync(join(outside, 'victim'), 'must survive'); + + write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); + write('packages/lib/src/dir/victim', 'base\n'); + write('packages/lib/src/f.ts', 'export const f = () => 1;\n'); + write( + 'packages/lib/src/f.test.ts', + 'import { f } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof f).toBe("function"));\n', + ); + const base = commitAll('base'); + + // The P0 shape: `dir` becomes a symlink to an outside directory and + // `dir/victim` is deleted. + git(repo, 'rm', '-q', '-r', 'packages/lib/src/dir'); + symlinkSync(outside, join(repo, 'packages/lib/src/dir')); + write('packages/lib/src/f.ts', 'export const f = () => 2;\n'); + commitAll('pr: dir -> outside symlink, delete dir/victim'); + + const wt = join(repo, 'wt'); + git(repo, 'worktree', 'add', '-q', '--detach', wt, 'HEAD'); + writeFileSync( + join(repo, 'report.json'), + JSON.stringify({ + files: [ + { path: 'packages/lib/src/dir', kind: 'source' }, + { path: 'packages/lib/src/dir/victim', kind: 'source' }, + { path: 'packages/lib/src/f.ts', kind: 'source' }, + { path: 'packages/lib/src/f.test.ts', kind: 'test' }, + ], + }), + ); + + const before = treeState(wt); + await runHandler({ + report: join(repo, 'report.json'), + worktree: wt, + base, + out: join(repo, 'out.json'), + }); + + // The outside file is untouched. + expect(readFileSync(join(outside, 'victim'), 'utf8')).toBe('must survive'); + // And it survived because the probe never restored/deleted in a tree holding + // the symlink — not because `safeRmWithin` refused. If the guard had been the + // thing that fired, it would have surfaced as an inconclusive probe. + const out = JSON.parse(readFileSync(join(repo, 'out.json'), 'utf8')); + const details = (out.probed as Array<{ detail: string }>).map( + (p) => p.detail, + ); + expect(details.join('\n')).not.toMatch( + /refusing to delete through a symlink/, + ); + // Shared tree untouched, probe tree discarded. + expect(treeState(wt)).toBe(before); + expect(existsSync(join(repo, 'wt-probe'))).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 90b0d167428..b0820c77d01 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -43,7 +43,7 @@ import { rmSync, lstatSync, } from 'node:fs'; -import { dirname, join, isAbsolute, sep } from 'node:path'; +import { dirname, join, isAbsolute, resolve, sep } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; export type ProbeVerdict = 'gated' | 'inert' | 'inconclusive'; @@ -339,9 +339,6 @@ export function safeRmWithin(worktree: string, relPath: string): void { const existsAtBase = (cwd: string, base: string, path: string) => existsAtRev(cwd, base, path); -/** A file the PR DELETED does not exist at HEAD — restoring it means removing it. */ -const existsAtHead = (cwd: string, path: string) => - existsAtRev(cwd, 'HEAD', path); async function runTestEfficacy(args: TestEfficacyArgs): Promise { const { report, worktree, base, out } = args; @@ -379,134 +376,119 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { verdict: ProbeVerdict; detail: string; }> = []; - let restoreFailure: string | undefined; + let cleanupFailure: string | undefined; if (probes.length > 0 && revert.length > 0) { - // The probe checks out base over the revert set and deletes added files, - // then restores HEAD. That is safe on the ephemeral worktree the /review - // pipeline builds, but this is a public command that accepts any - // `--worktree`: on a tree with uncommitted edits to a revert-set file, the - // checkout would discard them with no undo. Refuse a dirty revert set - // rather than eat someone's work. - // `gitOut` throws on spawn failure (via the `git()` guard), so a `git` - // that could not run fails the probe rather than silently reading as a - // clean tree — the fail-OPEN outcome would defeat the whole guard, which - // exists to prevent data loss. - // `--ignored` too: a revert-set path can be gitignored at HEAD (a generated - // or locally-recreated file), and a plain `status --porcelain` says nothing - // about it — the base checkout would then overwrite a file the user has and - // git will not restore. - const dirty = revert.filter( - (p) => - gitOut(worktree, 'status', '--porcelain', '--ignored', '--', p).length > - 0, - ); - if (dirty.length > 0) { - throw new Error( - `refusing to run: the worktree has uncommitted changes to files this probe would revert (${dirty.join(', ')}). ` + - `Commit or stash them first — the probe checks out base over these files and could not restore your edits.`, - ); - } - } - - if (probes.length > 0 && revert.length > 0) { - // "Revert to base" is two operations, not one. A file the PR MODIFIED is - // checked out from base; a file the PR ADDED did not exist at base, and - // `git checkout -- ` does not quietly skip it — it fails - // with `pathspec ... did not match any file(s) known to git`. That throw - // used to escape past `writeFileSync` and discard the whole report, - // `unreachable` findings included, on every PR that adds a source file. - // Which is most of them. - const modified: string[] = []; - const added: string[] = []; - for (const p of revert) { - (existsAtBase(worktree, base, p) ? modified : added).push(p); - } + // The probe reverts the PR's source to base and runs the tests against it — + // in its OWN disposable worktree, checked out at the PR head and discarded + // wholesale when the probe finishes. The shared worktree the other review + // agents read is never mutated (so a concurrent reader can never observe a + // half-reverted tree), and there is no in-place restore to get wrong (so the + // restore delete that once followed a PR-controlled symlink out of the tree + // is gone with it). See #6832. + // + // Isolation is also why there is no dirty-worktree guard anymore: the probe + // tree is a fresh checkout of the committed head, so nothing the caller has + // uncommitted in the shared tree is ever touched or discarded. + // + // `node_modules` resolves without a per-tree install because the probe tree + // is nested under the repo (`.qwen/tmp/…-probe`), so Node walks up to the + // repo-root `node_modules` — exactly how the shared review worktree already + // runs vitest. + const headSha = gitOut(worktree, 'rev-parse', 'HEAD'); + const probeTree = `${resolve(worktree)}-probe`; + let created = false; try { - if (modified.length > 0) { - git(worktree, 'checkout', base, '--', ...modified); - } - // An added file's base state is "absent". Removing it is the honest - // revert; the probe usually then fails to compile, which is - // `inconclusive` — not a verdict, but an honest one. - for (const p of added) safeRmWithin(worktree, p); - - const r = spawnSync( - 'npx', - ['vitest', 'run', '--reporter=json', ...probes], - { - cwd: worktree, - encoding: 'utf8', - timeout: 300_000, - // Vitest's JSON reporter on a large suite easily exceeds spawnSync's - // 1 MiB default stdout buffer, which returns ENOBUFS and turns every - // probe `inconclusive`. Match the 64 MiB ceiling the gh wrapper uses. - maxBuffer: 64 * 1024 * 1024, - }, - ); - // `r.error` is set — and `r.status` is null — when the process never ran - // (npx missing) or was killed (the timeout above fires SIGTERM). Ignoring - // it reports those as "the runner produced no parseable JSON", which - // blames the runner's output for a run that produced none. - if (r.error) throw r.error; - if (r.signal) { - throw new Error( - `runner killed by ${r.signal}${r.signal === 'SIGTERM' ? ' (probe timed out after 300s)' : ''}`, - ); - } - results.push( - ...classifyProbeRun( - r.status ?? 1, - `${r.stdout ?? ''}`, - probes, - `${r.stderr ?? ''}`, - ), - ); + // Sweep a stale probe tree left by a crashed run — it would fail `add`. + // Best-effort (no stale tree is the normal case), so it does not go through + // the throwing `git()` wrapper. + spawnSync('git', ['worktree', 'remove', '--force', probeTree], { + cwd: worktree, + }); + git(worktree, 'worktree', 'add', '--detach', probeTree, headSha); + created = true; } catch (e) { - // The probe could not be set up or run. That is not evidence about any - // test — record it and keep going, so the report (and the unreachable - // findings, which needed no probe at all) still reaches the caller. - const detail = `probe could not run: ${e instanceof Error ? e.message : String(e)}`; - results.push( - ...probes.map((file) => ({ - file, - verdict: 'inconclusive' as const, - detail, - })), - ); - } finally { - // Always put the worktree back — the review's later steps read this tree - // and must see the PR's code, not the base's. This restores deleted files - // too. A restore failure must not mask the probe's own outcome (hence the - // catch), but it must not be swallowed either: the tree is now sitting on - // BASE code, and every agent that reads it afterwards reviews the wrong - // source. That is the loudest thing this command can have to say. - // - // Restore is also two operations, for the mirror-image reason the revert - // was. A file the PR DELETED does not exist at HEAD either, and - // `git checkout HEAD -- ` fails on the bad pathspec and - // restores NOTHING — so one deleted source file used to leave the whole - // revert set sitting on base code, plus a resurrected copy of the file the - // PR removed. Delete what HEAD does not have; check out what it does. - const atHead: string[] = []; - const notAtHead: string[] = []; - for (const p of revert) { - (existsAtHead(worktree, p) ? atHead : notAtHead).push(p); + // Could not isolate — probe nothing rather than fall back to mutating the + // shared tree. Probes are inconclusive; the unreachable findings, which + // need no probe, still ship. + const detail = `probe worktree could not be created: ${e instanceof Error ? e.message : String(e)}`; + for (const file of probes) { + results.push({ file, verdict: 'inconclusive' as const, detail }); } + } + + if (created) { try { - if (atHead.length > 0) { - git(worktree, 'checkout', 'HEAD', '--', ...atHead); + // "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 (notAtHead.length > 0) { - // `git checkout -- ` writes the INDEX as well as the - // working tree, so removing the file leaves a staged phantom add - // behind (`AD` in `git status`). Reset those index entries to HEAD — - // where the path does not exist, which is exactly the state we want. - for (const p of notAtHead) safeRmWithin(worktree, p); - git(worktree, 'reset', '-q', 'HEAD', '--', ...notAtHead); + if (modified.length > 0) { + git(probeTree, 'checkout', base, '--', ...modified); } + for (const p of added) safeRmWithin(probeTree, p); + + const r = spawnSync( + 'npx', + ['vitest', 'run', '--reporter=json', ...probes], + { + cwd: probeTree, + encoding: 'utf8', + timeout: 300_000, + // Vitest's JSON reporter on a large suite easily exceeds spawnSync's + // 1 MiB default stdout buffer, which returns ENOBUFS and turns every + // probe `inconclusive`. Match the 64 MiB ceiling the gh wrapper uses. + maxBuffer: 64 * 1024 * 1024, + }, + ); + // `r.error` is set — and `r.status` is null — when the process never ran + // (npx missing) or was killed (the timeout above fires SIGTERM). Ignoring + // it reports those as "the runner produced no parseable JSON", which + // blames the runner's output for a run that produced none. + if (r.error) throw r.error; + if (r.signal) { + throw new Error( + `runner killed by ${r.signal}${r.signal === 'SIGTERM' ? ' (probe timed out after 300s)' : ''}`, + ); + } + results.push( + ...classifyProbeRun( + r.status ?? 1, + `${r.stdout ?? ''}`, + probes, + `${r.stderr ?? ''}`, + ), + ); } catch (e) { - restoreFailure = `WORKTREE NOT RESTORED — it is still on base code for: ${revert.join(', ')}. Every later step of this review reads the wrong source. Run \`git checkout HEAD -- ${atHead.join(' ')}\` in ${worktree} before continuing. (${e instanceof Error ? e.message : String(e)})`; + // The probe could not be set up or run. That is not evidence about any + // test — record it and keep going, so the report (and the unreachable + // findings, which needed no probe at all) still reaches the caller. + const detail = `probe could not run: ${e instanceof Error ? e.message : String(e)}`; + results.push( + ...probes.map((file) => ({ + file, + verdict: 'inconclusive' as const, + detail, + })), + ); + } finally { + // Discard the whole probe tree. There is no in-place restore to fail: + // the shared worktree was never mutated. A failed removal only leaves a + // stale dir (swept at the start of the next run, and by cleanup.ts) — a + // warning, not the "every later step reads the wrong source" alarm the + // old in-place restore had to raise. + try { + git(worktree, 'worktree', 'remove', '--force', probeTree); + } catch (e) { + cleanupFailure = `could not remove probe worktree ${probeTree}: ${e instanceof Error ? e.message : String(e)}`; + } } } } @@ -531,7 +513,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { probed: results, inconclusive: results.filter((r) => r.verdict === 'inconclusive'), findings, - restoreFailure, + cleanupFailure, }; mkdirSync(dirname(out), { recursive: true }); writeFileSync(out, JSON.stringify(result, null, 2), 'utf8'); @@ -541,15 +523,11 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { for (const f of findings) { writeStdoutLine(` [test] ${f.kind}: ${f.file}`); } - if (restoreFailure) { - // Loud, on stderr, AND a non-zero exit. The worktree is now on base code, - // so every later review step reads the wrong source; a line in a JSON - // field the workflow does not consume would let it proceed anyway (the - // fail-open the whole guard exists to prevent). The report is already - // written, so the caller still has the findings — it just cannot mistake - // this for a clean run. - writeStderrLine(`ERROR: ${restoreFailure}`); - process.exitCode = 1; + if (cleanupFailure) { + // A leftover probe worktree does not corrupt the shared tree — it is swept + // at the start of the next run and by cleanup.ts — so this is a warning, not + // the non-zero-exit alarm the old in-place restore failure had to raise. + writeStderrLine(`WARNING: ${cleanupFailure}`); } }