From c72ba0764e2fa23cc805ec884486a4bbaaabe98d Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 31 Jul 2026 17:02:20 +0800 Subject: [PATCH 01/12] =?UTF-8?q?feat(review):=20borrowed-verification=20t?= =?UTF-8?q?rio=20=E2=80=94=20test-plan=20check,=20base-tree=20A/B,=20per-h?= =?UTF-8?q?unk=20probes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - qwen review test-plan: rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree; contradictions and differing counts are disclosed via compose-review, never capping. - qwen review base-tree: build the merge base in a sibling worktree so the verifier can A/B a comparative claim instead of reading it; swept by cleanup. - test-efficacy: third probe kind — reverse-apply one hunk at a time and re-run the affected tests, attributing a still-green suite to the specific change nothing gates; shares the mutants' budget window, runs last. --- packages/cli/src/commands/review.test.ts | 2 + packages/cli/src/commands/review.ts | 6 +- .../cli/src/commands/review/base-tree.test.ts | 176 +++++ packages/cli/src/commands/review/base-tree.ts | 227 +++++++ .../src/commands/review/build-test.test.ts | 40 ++ .../cli/src/commands/review/build-test.ts | 27 +- .../cli/src/commands/review/cleanup.test.ts | 22 +- packages/cli/src/commands/review/cleanup.ts | 8 + .../commands/review/compose-review.test.ts | 101 +++ .../cli/src/commands/review/compose-review.ts | 90 ++- .../src/commands/review/lib/agent-briefs.ts | 15 + packages/cli/src/commands/review/lib/paths.ts | 14 + .../cli/src/commands/review/lib/worktree.ts | 69 ++ .../review/test-efficacy.integration.test.ts | 231 ++++++- .../src/commands/review/test-efficacy.test.ts | 136 ++++ .../cli/src/commands/review/test-efficacy.ts | 382 ++++++++++- .../cli/src/commands/review/test-plan.test.ts | 554 +++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 630 ++++++++++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 59 ++ .../core/src/skills/bundled/review/SKILL.md | 29 +- 20 files changed, 2774 insertions(+), 44 deletions(-) create mode 100644 packages/cli/src/commands/review/base-tree.test.ts create mode 100644 packages/cli/src/commands/review/base-tree.ts create mode 100644 packages/cli/src/commands/review/lib/worktree.ts create mode 100644 packages/cli/src/commands/review/test-plan.test.ts create mode 100644 packages/cli/src/commands/review/test-plan.ts diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 677dcad60df..acf84350c55 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -49,11 +49,13 @@ describe('reviewCommand', () => { 'load-rules', 'agent-prompt', 'build-test', + 'base-tree', 'script-lint', 'resolve-anchors', 'check-coverage', 'presubmit', 'test-efficacy', + 'test-plan', 'compose-review', 'submit', 'cleanup', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 7f5f61c7d78..18cea24c1f3 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -22,9 +22,11 @@ import { resolveAnchorsCommand } from './review/resolve-anchors.js'; import { checkCoverageCommand } from './review/check-coverage.js'; import { agentPromptCommand } from './review/agent-prompt.js'; import { buildTestCommand } from './review/build-test.js'; +import { baseTreeCommand } from './review/base-tree.js'; import { scriptLintCommand } from './review/script-lint.js'; import { submitCommand } from './review/submit.js'; import { testEfficacyCommand } from './review/test-efficacy.js'; +import { testPlanCommand } from './review/test-plan.js'; import { cleanupCommand } from './review/cleanup.js'; import { runCommand } from './review/run.js'; @@ -44,17 +46,19 @@ export const reviewCommand: CommandModule = { .command(loadRulesCommand) .command(agentPromptCommand) .command(buildTestCommand) + .command(baseTreeCommand) .command(scriptLintCommand) .command(resolveAnchorsCommand) .command(checkCoverageCommand) .command(presubmitCommand) .command(testEfficacyCommand) + .command(testPlanCommand) .command(composeReviewCommand) .command(submitCommand) .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, test-plan, compose-review, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts new file mode 100644 index 00000000000..5448297492a --- /dev/null +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -0,0 +1,176 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Against a REAL git repo, because the part that breaks is the worktree +// lifecycle — a detached add at a specific SHA, a stale sibling from a crashed +// run, a path that must sit beside the review worktree rather than inside it. +// None of that is exercised by mocking `spawnSync`, and all of it is what makes +// the command fail on a real review. +// +// The build is the seam. It is the slow half and it has its own suite; what +// matters here is that a base tree only counts as `available` when the build +// actually succeeded, since an A/B against a half-built tree measures the build, +// not the diff. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + mkdirSync, + rmSync, + writeFileSync, + existsSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { runBaseTree, type BaseTreeReport } from './base-tree.js'; +import { baseWorktreePath } from './lib/paths.js'; +import type { BuildTestReport } from './build-test.js'; + +const okBuild = { ok: true, note: 'built' } as BuildTestReport; +const failedBuild = { + ok: false, + note: 'TS2307', + build: [{ command: 'npm run build', exitCode: 2 }], +} as unknown as BuildTestReport; + +describe('runBaseTree', () => { + let repo: string; + let worktree: string; + let baseSha: string; + let headSha: string; + + const git = (cwd: string, ...args: string[]) => + execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); + + const writePlan = (over: Record = {}): string => { + const p = join(repo, 'plan.json'); + writeFileSync( + p, + JSON.stringify({ mergeBaseSha: baseSha, files: [], ...over }), + ); + return p; + }; + + const run = ( + over: { plan?: Record; worktree?: string } = {}, + build: (w: string) => BuildTestReport = () => okBuild, + ): BaseTreeReport => { + const { plan: planOver, ...rest } = over; + return runBaseTree({ + plan: writePlan(planOver), + worktree, + timeout: 60, + install: false, + build, + ...rest, + }); + }; + + beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'qwen-base-tree-')); + git(repo, 'init', '-q', '-b', 'main'); + git(repo, 'config', 'user.email', 't@t.t'); + git(repo, 'config', 'user.name', 't'); + writeFileSync(join(repo, 'a.txt'), 'before\n'); + git(repo, 'add', '-A'); + git(repo, 'commit', '-qm', 'base'); + baseSha = git(repo, 'rev-parse', 'HEAD'); + writeFileSync(join(repo, 'a.txt'), 'after\n'); + git(repo, 'commit', '-qam', 'head'); + headSha = git(repo, 'rev-parse', 'HEAD'); + // The review worktree the base tree is created beside. + worktree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(join(repo, '.qwen', 'tmp'), { recursive: true }); + git(repo, 'worktree', 'add', '--detach', '-q', worktree, headSha); + }); + + afterEach(() => rmSync(repo, { recursive: true, force: true })); + + it('creates a sibling worktree holding the BASE commit, not the head', () => { + const r = run(); + expect(r.available).toBe(true); + expect(r.path).toBe(baseWorktreePath(worktree)); + expect(r.baseSha).toBe(baseSha); + // The whole point: this tree is the code as it stood before the PR. + expect(git(r.path!, 'rev-parse', 'HEAD')).toBe(baseSha); + expect(existsSync(join(r.path!, 'a.txt'))).toBe(true); + }); + + it('places the base tree BESIDE the review worktree, never inside it', () => { + // Nested, it would land in the PR's own diff and be swept with it. + const r = run(); + expect(r.path!.startsWith(`${worktree}/`)).toBe(false); + expect(r.path).toBe(`${worktree}-base`); + }); + + it('builds in the base tree, and only there', () => { + const seen: string[] = []; + const r = run({}, (w) => { + seen.push(w); + return okBuild; + }); + expect(seen).toEqual([baseWorktreePath(worktree)]); + expect(r.build).toBe(okBuild); + }); + + it('recovers from a stale base tree left by a crashed run', () => { + const stale = baseWorktreePath(worktree); + mkdirSync(stale, { recursive: true }); + writeFileSync(join(stale, 'junk'), 'x'); + // A non-empty directory makes `git worktree add` fail `already exists`. + expect(run().available).toBe(true); + }); + + it('is NOT available when the base tree does not build', () => { + const r = run({}, () => failedBuild); + expect(r.available).toBe(false); + // The tree is kept: a base that will not compile is worth looking at, and + // the note must not read as a defect in the PR. + expect(existsSync(r.path!)).toBe(true); + expect(r.build).toBe(failedBuild); + expect(r.note).toMatch(/did not build/); + expect(r.note).toMatch(/never a finding against the PR/); + }); + + it('refuses when the plan carries no mergeBaseSha', () => { + const r = run({ plan: { mergeBaseSha: undefined } }); + expect(r.available).toBe(false); + expect(r.build).toBeNull(); + expect(r.note).toMatch(/no mergeBaseSha/); + expect(existsSync(baseWorktreePath(worktree))).toBe(false); + }); + + it('refuses when the base branch could not be fetched — the SHA may be stale', () => { + // An A/B against a stale base attributes the base branch's own commits to + // this PR: the two-dot-diff error, in another shape. + const r = run({ plan: { baseFetchFailed: true } }); + expect(r.available).toBe(false); + expect(r.note).toMatch(/stale/); + expect(existsSync(baseWorktreePath(worktree))).toBe(false); + }); + + it('refuses an unreadable plan and a missing worktree without throwing', () => { + expect( + runBaseTree({ + plan: join(repo, 'nope.json'), + worktree, + timeout: 60, + install: false, + build: () => okBuild, + }).note, + ).toMatch(/cannot read the plan/); + expect(run({ worktree: join(repo, 'no-such-tree') }).note).toMatch( + /does not exist/, + ); + }); + + it('refuses a mergeBaseSha that is not a commit in this repo', () => { + const r = run({ plan: { mergeBaseSha: '0'.repeat(40) } }); + expect(r.available).toBe(false); + expect(r.note).toMatch(/base worktree could not be created/); + }); +}); diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts new file mode 100644 index 00000000000..ebab5b3f76c --- /dev/null +++ b/packages/cli/src/commands/review/base-tree.ts @@ -0,0 +1,227 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review base-tree`: stand up a BUILT tree at the merge base, so a claim +// about changed behaviour can be measured instead of read. +// +// Every other step in this pipeline looks at one tree. The agents read the PR's +// code, the verifier traces a failure scenario through the PR's code, and even +// the probe capability — which does run something — runs it against the PR's +// code alone. The merge base is known (`fetch-pr` resolves `mergeBaseSha`) and +// is used for exactly one thing: choosing the diff range. Nothing has ever built +// it. +// +// That leaves a whole class of claim decided by reading. "This preserves the +// existing output." "This only adds a field." "Cancelled calls looked the same +// as failures before." Each is a statement about the DIFFERENCE between two +// programs, and the review has only ever had one of them in front of it. Reading +// a diff and concluding what the old behaviour was is exactly the step that goes +// wrong quietly: the new lines are always right there and always look correct, +// and whether they change what a user observes routinely turns on code the diff +// never touches. +// +// With a built base tree the same input can be fed to both and the two outputs +// compared. That is a different kind of evidence from anything else here — not a +// stronger argument, but an observation — and it is the only kind that settles a +// disagreement about what a program used to do. +// +// **What this command deliberately does NOT do: run anything.** It creates the +// tree and builds it, which is the expensive, fiddly, failure-prone half (a +// detached worktree at the right SHA, a stale sibling from a crashed run, the +// minimal build set, the widening loop, deadlines that a real build can meet). +// WHAT to run is the reviewer's question, not this command's — it depends +// entirely on the claim under test, and a fixed scenario would fit almost none +// of them. So the report hands back a path and gets out of the way. +// +// Cost is why this is on demand rather than part of every review: a second build +// is a second build. It is worth it for one claim that turns on it and wasted on +// a review with none, which is why the verifier's brief offers it per finding +// instead of the pipeline spending it up front. + +import type { CommandModule } from 'yargs'; +import { spawnSync } from 'node:child_process'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { baseWorktreePath } from './lib/paths.js'; +import { + discardWorktree, + worktreeCreateFailureDetail, + type SweepResult, +} from './lib/worktree.js'; +import { runBuildTest, type BuildTestReport } from './build-test.js'; + +export interface BaseTreeReport { + /** + * True when a tree stands at `path` and its build succeeded — the only state + * in which an A/B comparison means anything. A tree that would not compile + * cannot be run, and a difference measured against one that half-built is not + * a difference between the two programs. + */ + available: boolean; + /** Absolute path to the base worktree, when one was created. */ + path?: string; + /** The commit it holds — the merge base of the PR and its target branch. */ + baseSha?: string; + /** The build that ran there; null when the tree could not be created. */ + build: BuildTestReport | null; + /** What happened, in one line. Rendered to the reviewer verbatim. */ + note: string; +} + +export interface BaseTreeArgs { + plan: string; + worktree: string; + out?: string; + timeout: number; + install: boolean; + /** Test seam: the build step. Production runs the real `runBuildTest`. */ + build?: (worktree: string) => BuildTestReport; +} + +function git(cwd: string, ...args: string[]): void { + const r = spawnSync('git', args, { cwd, encoding: 'utf8' }); + if (r.error) throw r.error; + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${r.stderr ?? ''}`); + } +} + +export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { + const unavailable = (note: string): BaseTreeReport => ({ + available: false, + build: null, + note, + }); + + let plan: { mergeBaseSha?: unknown; baseFetchFailed?: unknown }; + try { + plan = JSON.parse(readFileSync(args.plan, 'utf8')); + } catch (err) { + return unavailable( + `cannot read the plan ${args.plan}: ${(err as Error).message}`, + ); + } + + const baseSha = plan.mergeBaseSha; + if (typeof baseSha !== 'string' || !baseSha) { + // A local review, a bare `plan-diff`, or a PR whose merge base could not be + // resolved. There is no "before" to build, and saying so plainly beats + // guessing at one — an A/B against the wrong base is worse than no A/B. + return unavailable( + 'the plan carries no mergeBaseSha, so there is no base commit to build ' + + '(a local review, or a merge base that could not be resolved)', + ); + } + // `fetch-pr` sets this when it could not fetch the base branch, which leaves + // `mergeBaseSha` possibly stale — pointing at whatever the local ref happened + // to be. A comparison against a stale base attributes the base branch's own + // movement to this PR, which is the same class of error two-dot diffs made. + if (plan.baseFetchFailed === true) { + return unavailable( + 'the base branch could not be fetched, so mergeBaseSha may be stale; an ' + + "A/B against it would attribute the base branch's own commits to this PR", + ); + } + + const worktree = resolve(args.worktree); + if (!existsSync(worktree)) { + return unavailable(`the review worktree ${worktree} does not exist`); + } + + const tree = baseWorktreePath(worktree); + let sweep: SweepResult | undefined; + try { + // Clear a stale base tree left by a crashed run — it would fail `add`. Its + // stderr is kept, because it is usually what explains that failure. + sweep = discardWorktree(worktree, tree); + git(worktree, 'worktree', 'add', '--detach', tree, baseSha); + } catch (e) { + return unavailable( + worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')), + ); + } + + const build = args.build + ? args.build(tree) + : runBuildTest({ + plan: args.plan, + worktree: tree, + timeout: args.timeout, + install: args.install, + // The base tree's own suite says nothing about this PR — it was green + // before the PR existed. What the A/B needs from here is a compiled + // tree to run against. + buildOnly: true, + }); + + if (!build.ok) { + // Leave the tree standing. A base that does not build is a fact worth + // looking at by hand, and deleting the evidence to save a directory is a + // bad trade — `cleanup` sweeps it at the end of the review either way. + return { + available: false, + path: tree, + baseSha, + build, + note: + `the base tree at ${baseSha.slice(0, 9)} did not build, so nothing can be run ` + + 'against it; an A/B is not available for this review (this is an ' + + 'infrastructure result, never a finding against the PR)', + }; + } + + return { + available: true, + path: tree, + baseSha, + build, + note: + `base tree built at ${baseSha.slice(0, 9)} in ${tree}. Run the same input here and in the ` + + 'PR worktree and compare the observed output; a difference is evidence, a ' + + 'reading is not.', + }; +} + +export const baseTreeCommand: CommandModule = { + command: 'base-tree', + describe: + "Build the PR's merge base in a sibling worktree, so a claim about changed " + + 'behaviour can be measured against the code as it stood before', + builder: (yargs) => + yargs + .option('plan', { + type: 'string', + demandOption: true, + describe: 'The plan report from fetch-pr (it carries `mergeBaseSha`)', + }) + .option('worktree', { + type: 'string', + demandOption: true, + describe: "The PR's worktree — the base tree is created beside it", + }) + .option('out', { type: 'string', describe: 'Write the JSON report here' }) + .option('timeout', { + type: 'number', + default: 300, + describe: "Per-command deadline in seconds, as `build-test`'s", + }) + .option('install', { + type: 'boolean', + default: true, + describe: 'Run `npm ci` first when node_modules is absent', + }), + handler: (argv) => { + const args = argv as unknown as BaseTreeArgs; + const report = runBaseTree(args); + if (args.out) { + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + } + writeStdoutLine(JSON.stringify(report, null, 2)); + writeStderrLine(`base-tree: ${report.note}`); + }, +}; diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index 362af15f1db..bedc6426a99 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/packages/cli/src/commands/review/build-test.test.ts @@ -453,6 +453,46 @@ describe('runBuildTest', () => { command, ) => ({ command, exitCode: 0, seconds: 1, timedOut: false, output: '' }); + it('buildOnly builds the same set but runs NO tests', () => { + // For the merge-base tree an A/B probe compares against: base's suite was + // green before this PR existed, so running it measures nothing about the + // diff and doubles the cost of the one thing the probe does need — a + // compiled tree to run against. + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ name: 'r', workspaces: ['packages/*'] }), + ); + pkg('packages/core', { + name: '@x/core', + scripts: { build: 'exit 0', test: 'exit 0' }, + }); + writePlan(['packages/core/src/a.ts']); + + const args = { + plan: planPath, + worktree: root, + timeout: 60, + install: false, + exec: okExec, + }; + const withTests = runBuildTest(args); + const buildOnly = runBuildTest({ ...args, buildOnly: true }); + + expect(withTests.test.map((t) => t.command)).toEqual([ + 'npm test --workspace="packages/core"', + ]); + expect(buildOnly.test).toEqual([]); + // The build itself is untouched — same set, same commands, same verdict. + expect(buildOnly.buildSet).toEqual(withTests.buildSet); + expect(buildOnly.build.map((b) => b.command)).toEqual( + withTests.build.map((b) => b.command), + ); + expect(buildOnly.ok).toBe(true); + // And the note must not claim tests it did not run. + expect(buildOnly.note).toContain('build-only'); + expect(buildOnly.note).not.toContain('ran the tests'); + }); + it('scopes the build to the changed workspace and its dependents', () => { writeFileSync( join(root, 'package.json'), diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index f0118d6e76f..13f7eef9b4a 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -249,6 +249,17 @@ interface BuildTestArgs { out?: string; timeout: number; install: boolean; + /** + * Build, then stop — do not run the changed workspaces' tests. + * + * For the merge-base tree an A/B probe compares against. Base's tests were + * green before this PR existed and running them measures nothing about it; + * what the probe needs from that tree is a compiled `dist/` to run against, + * and paying for the suite twice is the difference between an A/B a reviewer + * will use and one they will skip. Defaults false, so the PR-side call is + * unchanged. + */ + buildOnly?: boolean; /** * How to run a command. Injectable so the tests can build the states that are * hard to force out of real npm — chiefly the one that cost a live review: an @@ -626,7 +637,7 @@ export function runBuildTest(args: BuildTestArgs): BuildTestReport { // parallel and does not finish; the packages the diff did not touch cannot have // been broken by it, and their tests were green before this PR and will be green // after it. - for (const dir of affected) { + for (const dir of args.buildOnly ? [] : affected) { const pkg = byDir.get(dir); if (!pkg?.scripts.includes('test')) continue; const r = exec(testCommand(dir), root, perCommandMs); @@ -653,7 +664,11 @@ export function runBuildTest(args: BuildTestArgs): BuildTestReport { widened.size ? `, plus ${[...widened].join(', ')} the compiler asked for` : '' - }) and ran the tests of the changed ones. Everything passed.`; + })${ + args.buildOnly + ? '. Tests were not run (build-only).' + : ' and ran the tests of the changed ones. Everything passed.' + }`; } else if (realFailures.length === 0) { results.note = `${failed.length} command(s) ran out of time (${args.timeout}s). A timeout is an ` + @@ -721,6 +736,14 @@ export const buildTestCommand: CommandModule = { type: 'boolean', default: true, describe: 'Run `npm ci` first when node_modules is absent', + }) + .option('build-only', { + type: 'boolean', + default: false, + describe: + "Build, then stop — skip the changed workspaces' tests. For the " + + 'merge-base tree an A/B probe compares against, whose suite says ' + + 'nothing about this PR.', }), handler: (argv) => { const args = argv as unknown as BuildTestArgs; diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 25a29f4e57a..2d75e47b50e 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -15,7 +15,9 @@ const mocks = vi.hoisted(() => ({ writeStderrLine: vi.fn(), clearReviewWorktreeLease: vi.fn(), refExists: vi.fn(() => true), - releaseWorktree: vi.fn(() => ({ + // The parameter is declared so `mock.calls` is typed `[string][]` rather than + // `[][]` — the paths it was asked to free are the assertion in the sweep test. + releaseWorktree: vi.fn((_path: string) => ({ existed: false, freed: false, reason: undefined, @@ -77,6 +79,7 @@ vi.mock('./lib/gh.js', () => ({ vi.mock('./lib/paths.js', () => ({ worktreePath: (prNumber: string) => `/repo/.qwen/tmp/review-pr-${prNumber}`, probeWorktreePath: (path: string) => `${path}-probe`, + baseWorktreePath: (path: string) => `${path}-base`, reviewBranch: (prNumber: string) => `qwen-review/pr-${prNumber}`, REVIEW_TMP_DIR: '/repo/.qwen/tmp', tmpFile: (target: string, suffix: string) => @@ -132,6 +135,23 @@ describe('runCleanup', () => { 'pr-123', ); }); + + it('releases the review worktree AND both disposable siblings', () => { + // `base-tree` deliberately leaves its tree standing for the whole review + // (a later verifier may need it, and a base that failed to build is kept as + // evidence), so this is its ONLY removal — not a crash sweep like the + // probe's. A missing entry here leaks a full built checkout per review and + // blocks the next run's `git worktree add`. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + + runCleanup('pr-123'); + + expect(mocks.releaseWorktree.mock.calls.map((c) => c[0])).toEqual([ + '/repo/.qwen/tmp/review-pr-123', + '/repo/.qwen/tmp/review-pr-123-probe', + '/repo/.qwen/tmp/review-pr-123-base', + ]); + }); }); describe('findUnsanctionedIssueComments', () => { diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 230b17d0fd0..57441926c55 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -24,6 +24,7 @@ import { refExists, releaseWorktree } from './lib/git.js'; import { worktreePath, probeWorktreePath, + baseWorktreePath, reviewBranch, REVIEW_TMP_DIR, tmpFile, @@ -414,6 +415,13 @@ export function runCleanup(target: string): void { // path helper with the probe so the suffix cannot drift between the two. report('probe worktree', probeWorktreePath(wt)); + // The A/B base tree is the same story: `base-tree` leaves it standing for + // the rest of the review (a verifier may run against it at any point, and a + // base that failed to build is kept deliberately, as evidence), so this is + // its only removal — not just a crash sweep. Same shared path helper, same + // reason: the suffix must not drift between creator and sweeper. + report('base worktree', baseWorktreePath(wt)); + const branch = reviewBranch(prNumber); if (refExists(branch)) { try { diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index c65d7610a39..e508222e07f 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -21,6 +21,7 @@ import { getGhHost, setGhHost } from './lib/gh.js'; import { composeReview, scriptLintGate, + testPlanGate, composeReviewCommand, describeChunkGap, verdictLine, @@ -2990,3 +2991,103 @@ describe('composeReview — the script-lint gate wired to the verdict', () => { expect(r.body).toContain('LGTM'); }); }); + +describe('testPlanGate — Test Plan rulings, disclosed but never capping', () => { + // The gate's whole contract is that it produces NOTES and nothing else: no + // critical, no cap, no unreviewed scope. Every test here is really the same + // assertion from a different angle — a Test Plan defect must never be able to + // change what the review does to the pull request. + let diffPath: string; + let diffHash: string; + + beforeEach(() => { + diffPath = join(dir, 'pr.diff'); + writeFileSync(diffPath, 'diff --git a/x b/x\n@@ -0,0 +1 @@\n+added\n'); + diffHash = createHash('sha256') + .update(readFileSync(diffPath)) + .digest('hex'); + }); + + const writePlan = (over: Record = {}): string => { + const p = join(dir, 'plan.json'); + writeFileSync( + p, + JSON.stringify({ prNumber: 1, diffPathAbsolute: diffPath, ...over }), + ); + return p; + }; + const writeReport = ( + claims: Array>, + over: Record = {}, + name = 'qwen-review-pr-1-test-plan.json', + ) => + writeFileSync( + join(dir, name), + JSON.stringify({ found: true, claims, diffHash, note: '', ...over }), + ); + + it('renders a contradicted claim with what was observed', () => { + const p = writePlan(); + writeReport([ + { + kind: 'path', + text: 'src/ghost.test.ts', + verdict: 'contradicted', + observed: 'no such file or directory', + }, + ]); + // Both halves go through `mdField`: the claim is the author's text and the + // observation is read back off disk, so neither is trusted to be inert + // markdown. + expect(testPlanGate(p).notes).toEqual([ + '`src/ghost.test.ts` — `no such file or directory`', + ]); + }); + + it('renders a differing count as an observation, not a contradiction', () => { + const p = writePlan(); + writeReport([ + { + kind: 'count', + text: '471 tests passed', + verdict: 'differs', + observed: '472 passed', + }, + ]); + expect(testPlanGate(p).notes).toEqual([ + '`471 tests passed` — this review observed `472 passed`', + ]); + }); + + it('says nothing about claims that reproduced or could not be checked', () => { + const p = writePlan(); + writeReport([ + { kind: 'command', text: 'npm run build', verdict: 'reproduces' }, + { kind: 'count', text: '9 tests passed', verdict: 'unchecked' }, + ]); + expect(testPlanGate(p).notes).toEqual([]); + }); + + it('stays silent on a local review — there is no PR body to have checked', () => { + const p = writePlan({ prNumber: undefined }); + writeReport([ + { kind: 'path', text: 'src/ghost.ts', verdict: 'contradicted' }, + ]); + expect(testPlanGate(p).notes).toEqual([]); + }); + + it('drops a STALE report rather than quoting a previous commit Test Plan', () => { + const p = writePlan(); + writeReport([{ kind: 'path', text: 'src/g.ts', verdict: 'contradicted' }], { + diffHash: 'a-different-hash', + }); + expect(testPlanGate(p).notes).toEqual([]); + }); + + it('does not cap or block when the report is missing or the plan is unreadable', () => { + // The `deferred`-checker precedent: a limitation the author cannot fix must + // never make a PR un-Approvable. Both paths return notes only. + expect(testPlanGate(writePlan()).notes).toEqual([]); + expect(testPlanGate(join(dir, 'nope.json')).notes).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 6999e933844..381701c7833 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -39,6 +39,7 @@ import { type RosterPlan, } from './lib/roster.js'; import { diffHashOf, type ScriptLintReport } from './script-lint.js'; +import type { TestPlanReport } from './test-plan.js'; import { CRITICAL_PREFIX, SUGGESTION_PREFIX, @@ -321,11 +322,15 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // Disclosed-but-non-capping notes from the gate (a deferred checker). Rendered // in the body on every verdict, but never fed into the cap. const gateDisclosed: string[] = []; + // Test Plan rulings. Disclosed on every verdict and counted toward nothing — + // see `testPlanGate` for why this one neither blocks nor caps. + const testPlanNotes: string[] = []; if (input.planPath) { const gate = scriptLintGate(input.planPath); bodyCriticals.push(...gate.criticals); // render + count toward `c`, deterministic unreviewed.push(...gate.unreviewed); gateDisclosed.push(...gate.disclosed); + testPlanNotes.push(...testPlanGate(input.planPath).notes); } // The Criticals a verifier must have ruled on before this review may post them as @@ -932,6 +937,18 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { ] : []; + // The Test Plan's own claims, ruled against the reviewed tree. Rendered on + // every verdict — including Approve, which is where most of them land — and + // worded so the author can see it is about the description, not the code. + const testPlanBlock: Bi[] = testPlanNotes.length + ? [ + { + en: `Test Plan (not a blocker): ${testPlanNotes.join('; ')}.`, + zh: `Test Plan（非阻断）：${testPlanNotes.join('; ')}。`, + }, + ] + : []; + if (event === 'REQUEST_CHANGES') { // Empty body, except the disclosures: every clause whose state holds // appears on every event — a confirmed blocker must not squeeze out the @@ -942,6 +959,7 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { ...cannotTellBlock, ...notReviewedParts, ...deferredBlock, + ...testPlanBlock, ...bodyCriticalBlock, ]; return { @@ -963,8 +981,9 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { [ { en: 'No issues found. LGTM! ✅', zh: '未发现问题。LGTM！✅' }, ...deferredBlock, + ...testPlanBlock, ], - deferredBlock.length ? '\n\n' : ' ', + deferredBlock.length || testPlanBlock.length ? '\n\n' : ' ', ), baseEvent, cappedBy, @@ -1077,6 +1096,10 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { // shell actionlint would lint but we do not yet trust. clauses.push(...deferredBlock); + // 6c. Test Plan rulings (non-capping) — a claim in the PR description that + // the reviewed tree does not bear out. + clauses.push(...testPlanBlock); + // 7. Body Criticals — on a COMMENT that stands where a REQUEST_CHANGES // would have been: the presubmit carve-out, and the unverified-blockers // cap. Either way the body copy is the ONLY copy of an unanchorable @@ -1344,6 +1367,71 @@ function scriptLintReportName(pr: unknown): string { : 'qwen-review-script-lint.json'; } +/** + * Read the test-plan report and turn its rulings into body notes. + * + * Unlike `scriptLintGate`, this one **never caps and never blocks**, and every + * early return is therefore a plain "nothing to say" rather than a fail-closed + * disclosure. That asymmetry is deliberate on both halves: + * + * - A Test Plan defect is not a code defect. The author claimed a path that + * is not there, or a count from a different suite; the diff is unaffected. + * Blocking a merge on it would spend the review's one irreversible action + * on a documentation nit, and the skill's design philosophy is that a + * comment not worth the reader's time costs more than it returns. + * - Capping on a MISSING report would cap essentially every PR, because most + * PRs produce no notes at all and a run has no way to prove the difference + * between "checked, nothing to say" and "never checked" that is worth the + * un-Approvability. This is the `deferred`-checker precedent above: a + * limitation the author cannot fix must not become a permanent cap. + * + * A stale report is dropped in silence for the same reason a stale one is + * refused elsewhere — a note about a previous commit's Test Plan is worse than + * no note, and here there is no cap to fall back to. + */ +export function testPlanGate(planPath: string): { notes: string[] } { + const notes: string[] = []; + let plan: { prNumber?: unknown; diffPathAbsolute?: unknown }; + try { + plan = JSON.parse(readFileSync(planPath, 'utf8')); + } catch { + return { notes }; + } + // A local review has no PR body, so there is no Test Plan to have checked. + const pr = plan.prNumber; + const isPr = + (typeof pr === 'number' && Number.isInteger(pr) && pr > 0) || + (typeof pr === 'string' && /^\d+$/.test(pr) && Number(pr) > 0); + if (!isPr) return { notes }; + + let report: TestPlanReport; + try { + report = JSON.parse( + readFileSync( + join(dirname(planPath), `qwen-review-pr-${pr}-test-plan.json`), + 'utf8', + ), + ) as TestPlanReport; + } catch { + return { notes }; + } + const planDiffHash = diffHashOf(plan.diffPathAbsolute); + if (!planDiffHash || report.diffHash !== planDiffHash) return { notes }; + + for (const claim of report.claims ?? []) { + if (claim.verdict === 'contradicted') { + notes.push( + `${mdField(claim.text)} — ${mdField(claim.observed ?? 'not reproduced')}`, + ); + } else if (claim.verdict === 'differs') { + notes.push( + `${mdField(claim.text)} — this review observed ${mdField(claim.observed ?? 'a different result')}`, + ); + } + } + return { notes }; +} + /** * Whether the posted body carries the collapsed Chinese version: the plan * (fetch-pr's report) recorded Han characters in the PR description. The diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 895370f5f2f..d883f12a1e1 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -483,6 +483,21 @@ For each finding you were given: **Leave the tree as you found it** — delete any probe file and revert any fix you applied for the self-check, so nothing you wrote reaches the diff or the build. A finding you actually probed carries \`Source: [probe]\` with the observed evidence; never tag one you only reasoned about — that source means "a run produced this", and downstream treats it as deterministic. +**When the claim is about a CHANGE in behaviour, one tree cannot settle it — build the other one.** A probe runs the PR's code, which answers "what does it do now". It cannot answer "and what did it do before", and a whole class of finding is exactly that difference: "this changes the output format", "this only adds a field", "this silently drops the error message", "cancelled and failed used to be indistinguishable". Reading the diff to recover the old behaviour is the step that goes wrong quietly — the new lines are always there and always look right, and whether they change what anyone observes routinely turns on code the diff never touches. So when a finding's claim is comparative, get the *before* and measure it: + +\`\`\`bash +"\${QWEN_CODE_CLI:-qwen}" review base-tree --plan --worktree \\ + --out /qwen-review-pr--base-tree.json +\`\`\` + +It builds the merge base in a sibling worktree and reports \`available\` and \`path\`. Then run **the same input** in both trees — the same command, the same fixture, the same script — and compare the observed output byte for byte. The three rules that make this evidence: + +- **Same input, same procedure, both sides.** A difference produced by running two different things is not a difference between the two programs. If you had to build or install differently on one side, say so and treat the result as inconclusive. +- **Quote both outputs.** \`BASE: \` / \`PR: \`. The observation is the verdict; a summary of it is a reading again. +- **A/B is expensive — spend it on a claim that turns on it.** One extra build per review, at most. A finding you can settle by tracing does not need this, and \`available: false\` (no merge base, a stale one, or a base that does not build) is a fact about the harness, never a finding against the PR. + +A finding an A/B settled carries \`Source: [probe]\` like any other run-produced evidence, with both sides' output quoted. **Do not remove the base tree** — \`cleanup\` sweeps it at the end of the review, and a later finding may need it. + Return, for each finding, one verdict: - **confirmed (high confidence)** — the trace works: you can restate the failure scenario against the real code, naming the triggering input/state and quoting the line(s) that produce the wrong outcome. Carry the severity (Critical | Suggestion | Nice to have). diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 9b79b707ed2..11e6014aa1a 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -48,6 +48,20 @@ export function probeWorktreePath(worktree: string): string { return `${resolve(worktree)}-probe`; } +/** + * The merge-base tree an A/B probe compares against — a second sibling of the + * review worktree, holding the code as it stood *before* the PR. + * + * Absolute for the same reason as `probeWorktreePath`: `git worktree add` runs + * with the review worktree as cwd, so a relative path would land the base tree + * nested inside the tree it is meant to sit beside. Kept here beside its sibling + * so `base-tree` and `cleanup.ts`'s sweep cannot drift apart on the suffix — + * the failure mode that made the probe tree's helper shared in the first place. + */ +export function baseWorktreePath(worktree: string): string { + return `${resolve(worktree)}-base`; +} + /** Local branch ref name for a fetched PR head. */ export function reviewBranch(prNumber: string | number): string { return `qwen-review/pr-${prNumber}`; diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts new file mode 100644 index 00000000000..14c97d9eb42 --- /dev/null +++ b/packages/cli/src/commands/review/lib/worktree.ts @@ -0,0 +1,69 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Disposable sibling worktrees, and the one step both users of them need. +// +// `test-efficacy` runs its mutants in one; `base-tree` builds the merge-base in +// another. Both add a tree beside the review worktree, both must survive a +// leftover from a crashed run, and both need the sweep's stderr to explain a +// subsequent `add` failure — so the step lives here rather than twice. + +import { spawnSync } from 'node:child_process'; +import { rmSync } from 'node:fs'; + +export type SweepResult = ReturnType; + +/** + * Free a disposable worktree's path: unregister it, then remove what is left. + * + * `git worktree remove --force` only clears a tree git still tracks. A directory + * left at the path after metadata loss or a partial cleanup is reported "not a + * working tree" and left in place — and a *non-empty* one then makes + * `git worktree add` fail `already exists`, wedging every later run until + * someone clears it by hand. So the unregister is followed by a plain remove of + * whatever dir remains. `rmSync` unlinks a symlink rather than following it, so + * a tampered leftover cannot redirect the delete outside `tree`. + * + * This is `releaseWorktree`'s two-step, and deliberately NOT a call to it: + * `releaseWorktree` runs git from the process cwd, which need not be this + * worktree's repo, and it discards the sweep's stderr — which is usually the + * only thing that explains a subsequent `add` failure. Every caller here needs + * `cwd` and that stderr. + * + * Best-effort by design: a clean path is the normal case, so the unregister does + * not throw on a non-zero status. `rmSync` still can (`force` suppresses ENOENT + * but not EPERM/EBUSY) — callers decide what that means. + */ +export function discardWorktree(cwd: string, tree: string): SweepResult { + const sweep = spawnSync('git', ['worktree', 'remove', '--force', tree], { + cwd, + encoding: 'utf8', + }); + rmSync(tree, { recursive: true, force: true }); + return sweep; +} + +/** + * The reason a disposable worktree could not be created. + * + * The stale-sweep's stderr is folded in because it is usually the explanation: + * when `add` fails on a leftover the sweep could not clear, the sweep is what + * says why. Pure, and extracted for that reason — the branch it lives on fires + * only when `git worktree add` fails, and there is no portable way to force that + * in a real-git test (the one lever, making `.git/worktrees` unwritable, is + * bypassed by root and behaves differently under CI's unprivileged user). + */ +export function worktreeCreateFailureDetail( + label: string, + err: unknown, + sweepStderr: string, +): string { + const sweepErr = sweepStderr.trim(); + return ( + `${label} worktree could not be created: ${err instanceof Error ? err.message : String(err)}` + + (sweepErr ? ` (stale-tree sweep also reported: ${sweepErr})` : '') + ); +} 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 1927d6acd37..99e848186ca 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -25,7 +25,12 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { runOneMutant, testEfficacyCommand } from './test-efficacy.js'; +import { + runOneMutant, + runOneHunkProbe, + splitDiffIntoHunks, + testEfficacyCommand, +} from './test-efficacy.js'; type Handler = (args: { report: string; @@ -278,6 +283,75 @@ describe('test-efficacy probe isolation (#6832)', () => { expect(existsSync(join(repo, 'wt-probe'))).toBe(false); }); + it('probes hunks end-to-end on a diff with NO mutant candidates', async () => { + // The gating bug this pins: hunk probes once lived inside the mutant + // branch, so they ran only on a diff that already had a safety-verb + // candidate — exactly inverting their purpose. This diff changes a return + // value and a condition. `SAFETY_VERB_RE` matches neither, so there are + // zero mutants, and before the fix there were zero hunk probes too: the + // one class of diff per-hunk probing exists for got nothing at all. + write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); + write( + 'packages/lib/src/f.ts', + 'export function price(n: number) {\n' + + ' if (n < 0) return 0;\n' + + ' return n * 2;\n' + + '}\n' + + '\n'.repeat(12) + + 'export function label() {\n' + + ' return "old";\n' + + '}\n', + ); + const base = commitAll('base'); + write( + 'packages/lib/src/f.ts', + 'export function price(n: number) {\n' + + ' if (n <= 0) return 0;\n' + + ' return n * 3;\n' + + '}\n' + + '\n'.repeat(12) + + 'export function label() {\n' + + ' return "new";\n' + + '}\n', + ); + write( + 'packages/lib/src/f.test.ts', + 'import { price } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof price).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'), + }); + + const out = JSON.parse(readFileSync(join(repo, 'out.json'), 'utf8')); + expect(out.mutants.probed).toEqual([]); + // Two well-separated changes, so two hunks — and the fake vitest is green + // whatever the tree holds, so both changes ship with nothing gating them. + expect(out.hunks.probed).toHaveLength(2); + expect(out.hunks.survived).toBe(2); + expect( + out.findings.filter((f: { kind: string }) => f.kind === 'hunk-survived'), + ).toHaveLength(2); + // The mutation happened only in the disposable tree. + expect(treeState(wt)).toEqual(before); + }); + it('runs a deletion mutant end-to-end and reports the survivor', async () => { // The dogfood shape at full scale: the PR adds a reset function whose one // safety statement (`state.clear()`) nothing gates. The fake vitest is @@ -1107,3 +1181,158 @@ process.stdout.write(JSON.stringify({ expect(existsSync(join(repo, 'wt-probe'))).toBe(false); }); }); + +describe('per-hunk probes against real git', () => { + // The risky half is the patch, not the verdict: a single-hunk patch has to be + // something `git apply --reverse` accepts, and reverse-applying hunk N has to + // change hunk N's lines and nothing else. A wrong patch here does not fail + // loudly — it neutralises the wrong change and attributes the run's verdict + // to code it never touched, which is the exact failure the mutants' own + // line-mismatch guard exists to prevent. + // + // `runProbeSuite` is not the subject. With an empty probe list it collects + // nothing and the verdict is `inconclusive` by the third-outcome rule, which + // leaves the patch application and the restore as what these assert. + const FILE = 'src/x.ts'; + const BEFORE = + Array.from({ length: 30 }, (_, i) => `line${i + 1};`).join('\n') + '\n'; + let base: string; + + const contents = () => readFileSync(join(repo, FILE), 'utf8'); + + const hunkPatches = () => { + const diff = git( + repo, + 'diff', + '--no-color', + '--src-prefix=a/', + '--dst-prefix=b/', + base, + 'HEAD', + '--', + FILE, + ); + return splitDiffIntoHunks(diff); + }; + + beforeEach(() => { + write(FILE, BEFORE); + base = commitAll('base'); + // Two well-separated changes, so they land in two distinct hunks. + const after = BEFORE.split('\n'); + after[1] = 'line2_CHANGED;'; + after[24] = 'line25_CHANGED;'; + write(FILE, after.join('\n')); + commitAll('head'); + }); + + it('produces two patches git accepts, one per change', () => { + const hunks = hunkPatches(); + expect(hunks).toHaveLength(2); + for (const h of hunks) { + // `--check` applies nothing; it asks git whether the patch is well-formed + // and would apply. A patch this rejects would be `inconclusive` forever. + expect(() => + execFileSync('git', ['apply', '--reverse', '--check', '-'], { + cwd: repo, + input: h.patch, + encoding: 'utf8', + }), + ).not.toThrow(); + } + }); + + it('reverting ONE hunk restores only that change', () => { + const [first, second] = hunkPatches(); + + runOneHunkProbe( + repo, + { + file: FILE, + index: 0, + header: first.header, + startLine: first.startLine, + patch: first.patch, + }, + [], + ); + // Restored afterwards — the probe must leave the tree as it found it. + expect(contents()).toContain('line2_CHANGED;'); + + // Apply by hand to observe the mid-probe state the probe itself hides. + execFileSync('git', ['apply', '--reverse', '-'], { + cwd: repo, + input: second.patch, + encoding: 'utf8', + }); + const reverted = contents(); + // The second change is undone… + expect(reverted).toContain('line25;'); + expect(reverted).not.toContain('line25_CHANGED;'); + // …and the first is untouched. This is what `git checkout base -- ` + // cannot do, and the whole reason the probe is per-hunk. + expect(reverted).toContain('line2_CHANGED;'); + }); + + it('restores the file after the run, verdict notwithstanding', () => { + const [first] = hunkPatches(); + const before = contents(); + const got = runOneHunkProbe( + repo, + { + file: FILE, + index: 0, + header: first.header, + startLine: first.startLine, + patch: first.patch, + }, + [], + ); + expect(contents()).toBe(before); + // No probe file collected anything, so the honest verdict is the + // third outcome — never `killed`. + expect(got.verdict).toBe('inconclusive'); + expect(got.header).toBe(first.header); + // …and it is inconclusive because nothing was COLLECTED, not because the + // patch bounced. Without this the assertion above passes just as well on a + // probe that never applied anything, which is the state it exists to rule + // out: a silent no-op reads exactly like a clean restore. + expect(got.detail).toContain('no clean verdict'); + expect(got.detail).not.toContain('could not be reverse-applied'); + }); + + it('is inconclusive and leaves the tree ALONE when the patch will not apply', () => { + const before = contents(); + const got = runOneHunkProbe( + repo, + { + file: FILE, + index: 0, + header: '@@ -1,3 +1,3 @@', + startLine: 1, + patch: + 'diff --git a/src/x.ts b/src/x.ts\n--- a/src/x.ts\n+++ b/src/x.ts\n@@ -1,3 +1,3 @@\n-nope;\n+also nope;\n context;\n', + }, + [], + ); + expect(got.verdict).toBe('inconclusive'); + expect(got.detail).toContain('could not be reverse-applied'); + expect(contents()).toBe(before); + }); + + it('is inconclusive when the probe tree does not hold the file at all', () => { + const got = runOneHunkProbe( + repo, + { + file: 'src/gone.ts', + index: 0, + header: '@@ -1 +1 @@', + startLine: 1, + patch: 'x', + }, + [], + ); + expect(got.verdict).toBe('inconclusive'); + expect(got.detail).toContain('does not hold'); + }); +}); diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 1d79f799be1..93c93ddea52 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -6,6 +6,9 @@ import { describe, it, expect } from 'vitest'; import { + splitDiffIntoHunks, + selectHunkProbes, + MAX_HUNK_PROBES, isWorkspaceMember, planTestEfficacy, classifyProbeRun, @@ -1148,3 +1151,136 @@ describe('fitsAnotherMutantRun', () => { expect(fitsAnotherMutantRun(0, 60_000)).toBe(false); }); }); + +describe('splitDiffIntoHunks', () => { + const DIFF = [ + 'diff --git a/src/x.ts b/src/x.ts', + 'index 111..222 100644', + '--- a/src/x.ts', + '+++ b/src/x.ts', + '@@ -1,3 +1,4 @@', + ' const a = 1;', + '+const added = 2;', + ' const b = 3;', + ' const c = 4;', + '@@ -20,3 +21,3 @@', + ' const p = 1;', + '-const q = 2;', + '+const q = 99;', + ' const r = 3;', + '', + ].join('\n'); + + it('returns one self-contained patch per hunk', () => { + const hunks = splitDiffIntoHunks(DIFF); + expect(hunks.map((h) => h.header)).toEqual([ + '@@ -1,3 +1,4 @@', + '@@ -20,3 +21,3 @@', + ]); + // Each patch carries the file header, so `git apply` can place it alone. + for (const h of hunks) { + expect(h.patch).toContain('diff --git a/src/x.ts b/src/x.ts'); + expect(h.patch).toContain('--- a/src/x.ts'); + expect(h.patch).toContain('+++ b/src/x.ts'); + expect(h.patch.endsWith('\n')).toBe(true); + } + // And ONLY its own hunk — the whole point of splitting. + expect(hunks[0].patch).toContain('+const added = 2;'); + expect(hunks[0].patch).not.toContain('const q = 99;'); + expect(hunks[1].patch).toContain('+const q = 99;'); + expect(hunks[1].patch).not.toContain('const added = 2;'); + }); + + it('reads the new-side start line from each header', () => { + expect(splitDiffIntoHunks(DIFF).map((h) => h.startLine)).toEqual([1, 21]); + }); + + it('does not mistake a removed line whose text begins `@@` for a header', () => { + // In a unified diff every body line is prefixed, so `-@@ x` is content. + const d = [ + 'diff --git a/a.md b/a.md', + '--- a/a.md', + '+++ b/a.md', + '@@ -1,2 +1,2 @@', + '-@@ old marker', + '+@@ new marker', + '', + ].join('\n'); + const hunks = splitDiffIntoHunks(d); + expect(hunks).toHaveLength(1); + expect(hunks[0].patch).toContain('-@@ old marker'); + }); + + it('returns nothing for a diff with no hunks (a binary file)', () => { + expect( + splitDiffIntoHunks( + 'diff --git a/i.png b/i.png\nBinary files a/i.png and b/i.png differ\n', + ), + ).toEqual([]); + expect(splitDiffIntoHunks('')).toEqual([]); + }); +}); + +describe('selectHunkProbes', () => { + const diffOf = (...hunks: Array<[number, number]>) => + [ + 'diff --git a/f b/f', + '--- a/f', + '+++ b/f', + ...hunks.map( + ([start, len]) => `@@ -${start},${len} +${start},${len} @@\n x`, + ), + '', + ].join('\n'); + + const file = (over: Record = {}) => ({ + file: 'src/a.ts', + diff: diffOf([1, 3], [20, 3]), + hasNewTests: false, + mutantLines: [] as number[], + ...over, + }); + + it('produces one candidate per hunk', () => { + const { selected } = selectHunkProbes([file()]); + expect(selected.map((c) => c.startLine)).toEqual([1, 20]); + expect(selected.map((c) => c.index)).toEqual([0, 1]); + }); + + it('skips a hunk a mutant already covers, and keeps the others', () => { + // The mutant ran the finer-grained experiment on those lines; a second run + // over the whole hunk buys a coarser answer at the same price. + const { selected } = selectHunkProbes([file({ mutantLines: [2] })]); + expect(selected.map((c) => c.startLine)).toEqual([20]); + }); + + it('puts files whose collocated tests the diff also touches first', () => { + const { selected } = selectHunkProbes([ + file({ file: 'src/plain.ts', diff: diffOf([1, 1]) }), + file({ file: 'src/tested.ts', diff: diffOf([1, 1]), hasNewTests: true }), + ]); + expect(selected.map((c) => c.file)).toEqual([ + 'src/tested.ts', + 'src/plain.ts', + ]); + }); + + it('COUNTS what the cap drops rather than losing it', () => { + // A capped `survived: 0` that read as "every change is covered" is exactly + // the false assurance the mutant cap already guards against. + const many = Array.from({ length: MAX_HUNK_PROBES + 3 }, (_, i) => + file({ file: `src/f${i}.ts`, diff: diffOf([1, 1]) }), + ); + const { selected, skippedForCap } = selectHunkProbes(many); + expect(selected).toHaveLength(MAX_HUNK_PROBES); + expect(skippedForCap).toBe(3); + }); + + it('has nothing to probe when every hunk is mutant-covered', () => { + const { selected, skippedForCap } = selectHunkProbes([ + file({ mutantLines: [2, 21] }), + ]); + expect(selected).toEqual([]); + expect(skippedForCap).toBe(0); + }); +}); diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index df00d1d9ff4..90bd49a01e8 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -60,6 +60,9 @@ import { import { dirname, join, isAbsolute, sep } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.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). +import { discardWorktree, type SweepResult } from './lib/worktree.js'; import { isWorkspaceMember } from './lib/workspaces.js'; export type ProbeVerdict = 'gated' | 'inert' | 'inconclusive'; @@ -153,6 +156,36 @@ export interface MutantResult extends MutantCandidate { */ export const MAX_MUTANTS = 8; +export type HunkVerdict = MutantVerdict; + +export interface HunkCandidate { + file: string; + /** 0-based index of this hunk within the file's diff against base. */ + index: number; + /** The `@@ … @@` line, quoted back in the report. */ + header: string; + /** New-side first line the hunk occupies — where the finding points. */ + startLine: number; + /** A complete patch: the file header plus this ONE hunk. */ + patch: string; +} + +export interface HunkResult extends Omit { + verdict: HunkVerdict; + detail: string; +} + +/** + * At most this many per-hunk probes per run. + * + * Lower than {@link MAX_MUTANTS} on purpose: a hunk probe costs the same full + * suite run, but it is the THIRD claim on a budget the mutants and the revert + * probe already share, and it runs last. The cap is what keeps a 40-hunk diff + * from starving the revert probe rather than a statement about how many hunks + * are worth probing. + */ +export const MAX_HUNK_PROBES = 6; + /** Deadline for one vitest run (baseline, mutant, or revert probe alike). */ const PROBE_RUN_TIMEOUT_MS = 300_000; @@ -853,40 +886,6 @@ export function safeRmWithin(worktree: string, relPath: string): void { rmSync(cur, { force: true }); } -type SweepResult = ReturnType; - -/** - * Free the probe worktree's path: unregister it, then remove whatever is left. - * - * `git worktree remove --force` only clears a tree git still tracks. A directory - * left at the path after metadata loss or a partial cleanup is reported "not a - * working tree" and left in place — and a *non-empty* one then makes - * `git worktree add` fail `already exists`, wedging every probe as - * `inconclusive` until someone clears it by hand. So the unregister is followed - * by a plain remove of whatever dir remains. `rmSync` unlinks a symlink rather - * than following it, so a tampered leftover cannot redirect the delete outside - * `tree`. - * - * This is `releaseWorktree`'s two-step, and deliberately NOT a call to it: - * `releaseWorktree` runs git from the process cwd, which need not be this - * worktree's repo, and it discards the sweep's stderr — which is usually the - * only thing that explains a subsequent `add` failure. Both callers here need - * `cwd: worktree` and that stderr, so the step lives here and is shared between - * them. - * - * Best-effort by design: a clean path is the normal case, so the unregister does - * not go through the throwing `git()` wrapper. `rmSync` can still throw (`force` - * suppresses ENOENT but not EPERM/EBUSY) — callers decide what that means. - */ -function discardWorktree(cwd: string, tree: string): SweepResult { - const sweep = spawnSync('git', ['worktree', 'remove', '--force', tree], { - cwd, - encoding: 'utf8', - }); - rmSync(tree, { recursive: true, force: true }); - return sweep; -} - const existsAtBase = (cwd: string, base: string, path: string) => existsAtRev(cwd, base, path); @@ -1008,6 +1007,182 @@ function runProbeSuite( * reached through the command (selection and the probe tree derive from the * same commit), so the test pins it directly rather than not at all. */ +/** + * Split one file's diff into one self-contained patch per hunk. + * + * The statement mutants answer "is this one safety statement protected?" for a + * deliberately narrow class of statement; the revert probe answers "is ANY of + * this protected?" all at once. Between them sits the question neither can + * reach: **which particular change in this diff has no test behind it.** A hunk + * is the natural unit for that — it is the granularity the author wrote and the + * granularity a reviewer reads — and reverting one at a time is the only way to + * attribute a green suite to a specific change rather than to the diff at large. + * + * A column-0 `@@` is unambiguously a hunk header: every body line of a unified + * diff starts with ' ', '+', '-' or '\', so a removed line whose own text begins + * `@@` arrives as `-@@` and cannot be mistaken for one. + */ +export function splitDiffIntoHunks( + diffText: string, +): Array<{ header: string; patch: string; startLine: number }> { + const lines = diffText.split('\n'); + const first = lines.findIndex((l) => l.startsWith('@@')); + if (first < 0) return []; + const fileHeader = lines.slice(0, first); + const out: Array<{ header: string; patch: string; startLine: number }> = []; + let i = first; + while (i < lines.length) { + if (!lines[i].startsWith('@@')) { + i++; + continue; + } + const header = lines[i]; + let j = i + 1; + while ( + j < lines.length && + !lines[j].startsWith('@@') && + !lines[j].startsWith('diff --git ') + ) { + j++; + } + const startLine = Number( + /^@@ -\d+(?:,\d+)? \+(\d+)/.exec(header)?.[1] ?? '0', + ); + out.push({ + header: header.trim(), + startLine, + // `git apply` requires the trailing newline; a patch that ends mid-line is + // rejected with `corrupt patch`. + patch: `${[...fileHeader, ...lines.slice(i, j)].join('\n').replace(/\n+$/, '')}\n`, + }); + i = j; + } + return out; +} + +/** + * The hunks worth probing, in the order they should be spent. + * + * Two selection rules, both about not paying twice for the same answer: + * + * - **A hunk that already contains a mutant is skipped.** The mutant ran a + * finer-grained version of the same experiment on that hunk's own lines; a + * second run over the whole hunk buys a coarser answer at the same price, + * and the budget is better spent on ground nothing has covered. + * - **Files whose collocated tests the diff also touches go first**, as with + * mutants: a survivor is most informative exactly where the PR claims its + * new tests cover the new code. + * + * Candidates the cap cannot fit are counted, never dropped in silence — a + * capped `survived: 0` that read as "every change is covered" is the same false + * assurance the mutant cap already guards against. + */ +export function selectHunkProbes( + files: Array<{ + file: string; + diff: string; + hasNewTests: boolean; + mutantLines: number[]; + }>, + cap: number = MAX_HUNK_PROBES, +): { selected: HunkCandidate[]; skippedForCap: number } { + const preferred: HunkCandidate[] = []; + const rest: HunkCandidate[] = []; + for (const f of files) { + const hunks = splitDiffIntoHunks(f.diff); + hunks.forEach((h, index) => { + const end = h.startLine + newSideLength(h.header); + if (f.mutantLines.some((n) => n >= h.startLine && n < end)) return; + (f.hasNewTests ? preferred : rest).push({ + file: f.file, + index, + header: h.header, + startLine: h.startLine, + patch: h.patch, + }); + }); + } + const eligible = [...preferred, ...rest]; + return { + selected: eligible.slice(0, cap), + skippedForCap: Math.max(0, eligible.length - cap), + }; +} + +/** New-side line count from an `@@ -a,b +c,d @@` header (`d` defaults to 1). */ +function newSideLength(header: string): number { + const m = /^@@ -\d+(?:,\d+)? \+\d+(?:,(\d+))? @@/.exec(header); + return m ? Number(m[1] ?? '1') : 1; +} + +/** + * Neutralise ONE hunk in the probe tree, run the affected tests, restore. + * + * Reverse-applying the hunk's own patch is what makes this attributable: `git + * checkout base -- ` would revert the whole file and the verdict would + * belong to no particular change, which is precisely the all-or-nothing limit + * of the revert probe. `git` does the line-offset arithmetic, so a hunk later in + * the file is neutralised at the right place without this code tracking offsets. + * + * The asymmetry the mutants established holds here too, and for the same + * reason: a patch that will not apply, or a tree that will not compile without + * the hunk, is `inconclusive` — NEVER `killed`. A compile error says nothing + * about whether a test would have caught a behavioural regression, and scoring + * it as "a test caught it" is exactly the false assurance this command exists + * to remove. + */ +export function runOneHunkProbe( + probeTree: string, + hunk: HunkCandidate, + probes: string[], + deadlineAt?: number, + now: () => number = Date.now, +): HunkResult { + const { patch: _patch, ...meta } = hunk; + const abs = join(probeTree, hunk.file); + let original: string; + try { + original = readFileSync(abs, 'utf8'); + } catch (e) { + return { + ...meta, + verdict: 'inconclusive', + detail: `the probe tree does not hold ${hunk.file}: ${e instanceof Error ? e.message : String(e)}`, + }; + } + const applied = spawnSync('git', ['apply', '--reverse', '-'], { + cwd: probeTree, + input: hunk.patch, + encoding: 'utf8', + }); + if (applied.error || applied.status !== 0) { + // Nothing was changed (git applies a patch atomically), so there is nothing + // to restore — and nothing was learned. + return { + ...meta, + verdict: 'inconclusive', + detail: `the hunk could not be reverse-applied, so nothing was neutralised: ${(applied.stderr ?? applied.error?.message ?? '').toString().trim()}`, + }; + } + try { + const { perFile } = runProbeSuite(probeTree, probes, deadlineAt, now); + const verdict = classifyMutantRun(perFile); + const detail = + verdict === 'killed' + ? 'the suite went red with this hunk reverted — a test covers this change' + : verdict === 'survived' + ? 'every affected test still PASSED with this hunk reverted — no test in this diff fails when the change is undone' + : 'the tree with this hunk reverted produced no clean verdict (likely a compile or import error) — not evidence either way'; + return { ...meta, verdict, detail }; + } finally { + // Restore by content, not by re-applying the patch forward: a forward apply + // can fail on its own and would leave the tree neutralised for every later + // probe, turning one bad restore into a run of false survivors. Writing the + // saved bytes back also recreates a file the reverse patch deleted. + writeFileSync(abs, original, 'utf8'); + } +} + export function runOneMutant( probeTree: string, mutant: MutantCandidate, @@ -1046,6 +1221,62 @@ export function runOneMutant( } } +/** + * The per-file inputs `selectHunkProbes` needs, read from the COMMITTED head. + * + * Same discipline as mutant selection: the diff comes from `base..HEAD` and so + * describes exactly the tree the probe worktree checks out, never whatever + * uncommitted state the shared worktree happens to hold. Default context (three + * lines) rather than `--unified=0`, because these patches are handed to + * `git apply`, which needs context to place a hunk. + * + * Per-file failures are swallowed to an empty diff: a blob that will not read + * says nothing about any test, and it must not take down the probing of the + * other files. + */ +function hunkProbeInputs( + worktree: string, + base: string, + headSha: string, + sources: string[], + probes: string[], + mutants: MutantCandidate[], +): Array<{ + file: string; + diff: string; + hasNewTests: boolean; + mutantLines: number[]; +}> { + return sources.map((file) => { + let diff = ''; + try { + diff = gitCapture( + worktree, + '-c', + 'core.quotePath=false', + 'diff', + '--no-color', + '--src-prefix=a/', + '--dst-prefix=b/', + '--no-ext-diff', + '--no-textconv', + base, + headSha, + '--', + file, + ); + } catch { + diff = ''; + } + return { + file, + diff, + hasNewTests: hasCollocatedNewTest(file, probes), + mutantLines: mutants.filter((m) => m.file === file).map((m) => m.line), + }; + }); +} + async function runTestEfficacy(args: TestEfficacyArgs): Promise { const now = args.now ?? Date.now; const startedAt = now(); @@ -1088,6 +1319,9 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { const mutantResults: MutantResult[] = []; let mutantsSkippedForBudget = 0; let mutantsSkippedForCap = 0; + const hunkResults: HunkResult[] = []; + let hunksSkippedForBudget = 0; + let hunksSkippedForCap = 0; let mutantsSkippedForBaseline = 0; let mutantsNote: string | undefined; // Notes can stack (a derailed file AND a red baseline); never clobber one @@ -1171,6 +1405,25 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { candidates = []; } + // Hunk candidates are chosen HERE, beside the mutants, and not inside the + // mutant branch below. Gating them on `candidates.length > 0` would run + // per-hunk probes only on diffs that already have a safety-verb mutant — + // exactly inverting their purpose, which is to cover the changes the + // safety-verb filter cannot see. A diff of pure condition and return-value + // edits has no mutants at all and is precisely the diff this reaches. + let hunkCandidates: HunkCandidate[] = []; + try { + const selection = selectHunkProbes( + hunkProbeInputs(worktree, base, headSha, revert, probes, candidates), + ); + hunkCandidates = selection.selected; + hunksSkippedForCap = selection.skippedForCap; + } catch { + // Selection is bookkeeping, like the mutants': a diff that will not read + // says nothing about any test. Probe nothing rather than guess. + hunkCandidates = []; + } + const probeTree = probeWorktreePath(worktree); let created = false; let sweep: SweepResult | undefined; @@ -1191,9 +1444,12 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { for (const c of candidates) { mutantResults.push({ ...c, verdict: 'inconclusive' as const, detail }); } + for (const { patch: _p, ...h } of hunkCandidates) { + hunkResults.push({ ...h, verdict: 'inconclusive' as const, detail }); + } } - if (created && candidates.length > 0) { + if (created && (candidates.length > 0 || hunkCandidates.length > 0)) { // The mutation phase runs BEFORE the revert: it needs the probe tree at // the unmodified PR head, and the revert below rewrites that tree to // base. The two cannot contaminate each other — every mutated file is in @@ -1224,6 +1480,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { .map((r) => r.file); if (greenProbes.length === 0) { mutantsSkippedForBaseline = candidates.length; + hunksSkippedForBudget = hunkCandidates.length; noteMutants( 'mutants not run: no probe file was green in the unmutated baseline (every file was red or collected nothing), so a red mutant run would prove nothing', ); @@ -1240,6 +1497,24 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { runOneMutant(probeTree, c, greenProbes, mutantDeadline, now), ); } + + // Per-hunk probes run LAST and out of the same window, on whatever + // the mutants left. That ordering is the priority statement: a + // safety-verb mutant is the higher-precision experiment, so it gets + // the budget first, and a hunk probe is what the leftovers buy. It + // also means a diff whose mutants consumed the window reports zero + // hunk probes with `skippedForBudget` set — never a silent zero. + for (const h of hunkCandidates) { + const remaining = mutantDeadline - now(); + if (!fitsAnotherMutantRun(remaining, estimatedRunMs)) { + hunksSkippedForBudget = + hunkCandidates.length - hunkResults.length; + break; + } + hunkResults.push( + runOneHunkProbe(probeTree, h, greenProbes, mutantDeadline, now), + ); + } } } catch (e) { // The baseline, a mutant run, or a restore failed. Not evidence about @@ -1253,6 +1528,11 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { detail, }); } + for (const { patch: _p, ...h } of hunkCandidates.slice( + hunkResults.length, + )) { + hunkResults.push({ ...h, verdict: 'inconclusive' as const, detail }); + } } } @@ -1339,6 +1619,13 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { kind: 'mutant-survived' as const, message: `\`${m.file}:${m.line}\`: deleting the added safety statement \`${m.statement}\` leaves every affected test green. No test in this diff fails when it is removed — confirm an existing test covers it, or add one, so a regression that drops or skips this statement is caught.`, })), + ...hunkResults + .filter((h) => h.verdict === 'survived') + .map((h) => ({ + file: h.file, + kind: 'hunk-survived' as const, + message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.)`, + })), ]; const count = (v: MutantVerdict) => @@ -1357,13 +1644,22 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { skippedForBaseline: mutantsSkippedForBaseline, ...(mutantsNote ? { note: mutantsNote } : {}), }, + hunks: { + probed: hunkResults, + killed: hunkResults.filter((h) => h.verdict === 'killed').length, + survived: hunkResults.filter((h) => h.verdict === 'survived').length, + inconclusive: hunkResults.filter((h) => h.verdict === 'inconclusive') + .length, + skippedForBudget: hunksSkippedForBudget, + skippedForCap: hunksSkippedForCap, + }, findings, cleanupFailure, }; mkdirSync(dirname(out), { recursive: true }); writeFileSync(out, JSON.stringify(result, null, 2), 'utf8'); writeStdoutLine( - `Wrote test-efficacy report to ${out} (${unreachable.length} unreachable, ${results.length} probed, ${mutantResults.length} mutant(s), ${findings.length} finding(s))`, + `Wrote test-efficacy report to ${out} (${unreachable.length} unreachable, ${results.length} probed, ${mutantResults.length} mutant(s), ${hunkResults.length} hunk probe(s), ${findings.length} finding(s))`, ); for (const f of findings) { writeStdoutLine(` [test] ${f.kind}: ${f.file}`); @@ -1383,6 +1679,18 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { ` ${mutantsSkippedForBudget} mutant(s) skipped: the remaining budget cannot fit another suite run`, ); } + // Both skip counts are printed for the same reason the mutants' are: a hunk + // probe that never ran must not be readable as a hunk that came back clean. + if (hunksSkippedForCap > 0) { + writeStdoutLine( + ` ${hunksSkippedForCap} hunk probe(s) skipped: more hunks than the cap of ${MAX_HUNK_PROBES}`, + ); + } + if (hunksSkippedForBudget > 0) { + writeStdoutLine( + ` ${hunksSkippedForBudget} hunk probe(s) skipped: the mutants used the window`, + ); + } if (mutantsNote) { writeStdoutLine(` ${mutantsNote}`); } @@ -1397,7 +1705,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { export const testEfficacyCommand: CommandModule = { command: 'test-efficacy ', describe: - "Check whether the diff's new tests actually gate its new behaviour (unreachable + revert probe + statement-deletion mutants)", + "Check whether the diff's new tests actually gate its new behaviour (unreachable + revert probe + statement-deletion mutants + per-hunk probes)", builder: (yargs) => yargs .positional('report', { diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts new file mode 100644 index 00000000000..2f0e6f02560 --- /dev/null +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -0,0 +1,554 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The two halves this command is made of, tested separately: the parsers (what +// counts as a Test Plan, what counts as a claim) and the rulings (what the tree +// says about each claim). The rulings are where a regression is expensive — +// every `contradicted` verdict becomes a note on someone's pull request, so the +// negative cases (a path that legitimately exists untouched, a count from a +// suite this review did not run) carry as much weight here as the positives. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import yargs, { type Argv } from 'yargs'; +import { + testPlanCommand, + extractTestPlanSection, + extractClaims, + observedTestCounts, + npmScriptOf, + runTestPlan, + type TestPlanClaim, + type TestPlanArgs, +} from './test-plan.js'; +import type { BuildTestReport } from './build-test.js'; + +describe('extractTestPlanSection', () => { + it('finds a `## Test Plan` heading and stops at the next same-level heading', () => { + const s = extractTestPlanSection( + '## Summary\n\nfixes it\n\n## Test Plan\n\n- ran `npm test`\n\n## Risk\n\nlow\n', + ); + expect(s?.heading).toBe('## Test Plan'); + expect(s?.content).toBe('- ran `npm test`'); + }); + + it('keeps sub-headings deeper than its own level', () => { + const s = extractTestPlanSection( + '## Test Plan\n\n### Unit\n\n`npm test`\n\n### E2E\n\nmanual\n\n## Risk\n\nlow', + ); + expect(s?.content).toContain('### Unit'); + expect(s?.content).toContain('### E2E'); + expect(s?.content).not.toContain('## Risk'); + }); + + it('stops at a HIGHER-level heading too', () => { + const s = extractTestPlanSection( + '### Test Plan\n\nran it\n\n## Risk\n\nlow', + ); + expect(s?.content).toBe('ran it'); + }); + + it('accepts the bold form and the Chinese headings', () => { + expect( + extractTestPlanSection('**Test Plan**\n\nran it\n\n**Risk**\n\nlow') + ?.content, + ).toBe('ran it'); + expect( + extractTestPlanSection('## 测试计划\n\n跑了 `npm test`')?.content, + ).toBe('跑了 `npm test`'); + }); + + it('does not end the section on a `#` inside a fenced block', () => { + // The regression this guards: a repro script's shebang read as a heading, + // truncating the Test Plan to its first line. + const s = extractTestPlanSection( + '## Test Plan\n\n```bash\n#!/usr/bin/env bash\n# build first\nnpm run build\n```\n\ndone\n\n## Risk\n\nlow', + ); + expect(s?.content).toContain('npm run build'); + expect(s?.content).toContain('done'); + expect(s?.content).not.toContain('low'); + }); + + it('finds a heading whose name is PREFIXED, not anchored', () => { + // This repo's own PR template writes `## Reviewer Test Plan`. An anchored + // pattern found neither it nor `## Reviewer 测试计划`, so the command + // reported "no Test Plan section" on exactly the PRs it was built for. + expect( + extractTestPlanSection('## Reviewer Test Plan\n\nran it')?.content, + ).toBe('ran it'); + expect( + extractTestPlanSection('## Reviewer 测试计划\n\n跑了它')?.content, + ).toBe('跑了它'); + expect( + extractTestPlanSection('## Manual QA / Testing\n\nran it')?.content, + ).toBe('ran it'); + }); + + it('returns null when there is no Test Plan section', () => { + expect(extractTestPlanSection('## Summary\n\njust a change')).toBeNull(); + }); +}); + +describe('extractClaims', () => { + it('picks commands and paths out of code spans', () => { + const claims = extractClaims( + 'Ran `npm run build` and `npm test --workspace=packages/cli`.\nAdded `packages/cli/src/a.test.ts`.', + ); + expect(claims).toContainEqual({ kind: 'command', text: 'npm run build' }); + expect(claims).toContainEqual({ + kind: 'command', + text: 'npm test --workspace=packages/cli', + }); + expect(claims).toContainEqual({ + kind: 'path', + text: 'packages/cli/src/a.test.ts', + }); + }); + + it('reads fenced blocks, stripping prompts and trailing comments', () => { + const claims = extractClaims( + '```bash\n$ npm run lint # should be clean\n```', + ); + expect(claims).toContainEqual({ kind: 'command', text: 'npm run lint' }); + }); + + it('emits ONE count claim for one statement, not one per overlapping pattern', () => { + const claims = extractClaims('All 471 tests passed.').filter( + (c) => c.kind === 'count', + ); + expect(claims).toHaveLength(1); + expect(claims[0].text).toBe('471 tests passed'); + }); + + it('emits one claim per distinct count', () => { + const claims = extractClaims( + 'core: 1135 passed, desktop: 41 passed', + ).filter((c) => c.kind === 'count'); + expect(claims.map((c) => c.text)).toEqual(['1135 passed', '41 passed']); + }); + + it('reads a count stated in the future tense', () => { + // `expect all four files and 471 tests to pass` — the shape PR #8176 used, + // and the claim this command exists to check. + const claims = extractClaims( + 'expect all four files and 471 tests to pass', + ).filter((c) => c.kind === 'count'); + expect(claims.map((c) => c.text)).toEqual(['471 tests to pass']); + }); + + it('pulls path arguments out of a repro command', () => { + const claims = extractClaims( + '```bash\nnpx vitest run src/a.test.ts src/b.test.ts\n```', + ); + expect(claims).toContainEqual({ kind: 'path', text: 'src/a.test.ts' }); + expect(claims).toContainEqual({ kind: 'path', text: 'src/b.test.ts' }); + }); + + it('resolves a repro command path against its leading `cd`', () => { + // Unresolved, `src/telemetry/loggers.test.ts` does not exist at the repo + // root and every one of these becomes a false `contradicted` note. + const claims = extractClaims( + '`cd packages/core && npx vitest run src/telemetry/loggers.test.ts`', + ); + expect(claims).toContainEqual({ kind: 'path', text: 'packages/core' }); + expect(claims).toContainEqual({ + kind: 'path', + text: 'packages/core/src/telemetry/loggers.test.ts', + }); + expect(claims).not.toContainEqual({ + kind: 'path', + text: 'src/telemetry/loggers.test.ts', + }); + }); + + it('extracts no path from a `cd` shape it cannot resolve', () => { + // Rather than guess a base and file a wrong note. + expect( + extractClaims( + '`for d in a b; do cd $d && npx vitest run src/x.test.ts; done`', + ), + ).toEqual([]); + }); + + it('does not read a bare parenthesised number as a count', () => { + expect( + extractClaims('Follows up on (#8176).').filter((c) => c.kind === 'count'), + ).toEqual([]); + }); + + it('does not treat prose or a bare word in backticks as a path or command', () => { + expect(extractClaims('The `status` field is now authoritative.')).toEqual( + [], + ); + }); +}); + +describe('observedTestCounts', () => { + const report = (outputs: string[]): BuildTestReport => + ({ + test: outputs.map((output) => ({ + command: 'npm test', + exitCode: 0, + seconds: 1, + timedOut: false, + output, + })), + }) as BuildTestReport; + + it('reads the vitest summary', () => { + expect( + observedTestCounts(report(['\n Tests 472 passed (472)\n'])), + ).toEqual([472]); + }); + + it('reads the jest summary', () => { + expect( + observedTestCounts(report(['Tests: 12 passed, 12 total'])), + ).toEqual([12]); + }); + + it('reads a summary that also reports failures', () => { + expect( + observedTestCounts(report(['Tests 1 failed | 40 passed (41)'])), + ).toEqual([40]); + }); + + it('sums the summaries within one command and keeps commands separate', () => { + expect( + observedTestCounts( + report([ + 'Tests 10 passed (10)\nTests 5 passed (5)', + 'Tests 41 passed (41)', + ]), + ), + ).toEqual([15, 41]); + }); + + it('returns nothing when there is no report and when no count was printed', () => { + expect(observedTestCounts(null)).toEqual([]); + expect(observedTestCounts(report(['no summary here']))).toEqual([]); + }); +}); + +describe('npmScriptOf', () => { + it('reads the script name past `run` and past a workspace flag', () => { + expect(npmScriptOf('npm run build')).toBe('build'); + expect(npmScriptOf('npm test --workspace=packages/cli')).toBe('test'); + expect(npmScriptOf('npm run test:unit')).toBe('test:unit'); + }); + + it('is null for npm verbs that are not scripts, and for non-npm runners', () => { + expect(npmScriptOf('npm ci')).toBeNull(); + expect(npmScriptOf('npm install')).toBeNull(); + expect(npmScriptOf('make build')).toBeNull(); + }); +}); + +describe('runTestPlan', () => { + let dir: string; + + const plan = (files: string[]) => { + const p = join(dir, 'plan.json'); + writeFileSync( + p, + JSON.stringify({ + files: files.map((path) => ({ path, kind: 'source' })), + diffPathAbsolute: join(dir, 'diff.txt'), + }), + ); + return p; + }; + + const run = ( + body: string, + files: string[] = [], + buildTest?: BuildTestReport, + ) => { + let btPath: string | undefined; + if (buildTest) { + btPath = join(dir, 'bt.json'); + writeFileSync(btPath, JSON.stringify(buildTest)); + } + return runTestPlan( + { + plan: plan(files), + pr: '1', + repo: 'o/r', + worktree: dir, + buildTest: btPath, + }, + () => body, + ); + }; + + const verdictOf = (claims: TestPlanClaim[], text: string) => + claims.find((c) => c.text === text)?.verdict; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'qwen-test-plan-')); + writeFileSync(join(dir, 'diff.txt'), 'diff'); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ + workspaces: ['packages/*'], + scripts: { build: 'tsc', test: 'vitest' }, + }), + ); + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it('reports a missing Test Plan as absent, not as a finding', () => { + const r = run('## Summary\n\nno plan here'); + expect(r.found).toBe(false); + expect(r.claims).toEqual([]); + expect(r.note).toMatch(/no Test Plan section/); + }); + + it('distinguishes a failed body fetch from an absent Test Plan', () => { + const r = runTestPlan( + { plan: plan([]), pr: '1', repo: 'o/r', worktree: dir }, + () => { + throw new Error('gh: not authenticated'); + }, + ); + expect(r.found).toBe(false); + expect(r.note).toMatch(/could not be fetched/); + expect(r.note).toMatch(/not authenticated/); + }); + + it('binds the report to the diff it ran against', () => { + expect(run('## Test Plan\n\nran it').diffHash).toMatch(/^[0-9a-f]{64}$/); + }); + + describe('path claims', () => { + it('reproduces a path the diff changes', () => { + const r = run('## Test Plan\n\nAdded `packages/cli/src/a.test.ts`', [ + 'packages/cli/src/a.test.ts', + ]); + expect(verdictOf(r.claims, 'packages/cli/src/a.test.ts')).toBe( + 'reproduces', + ); + }); + + it('reproduces a path that exists but the diff does not touch', () => { + // A Test Plan may legitimately say "ran the existing suite at X". + mkdirSync(join(dir, 'packages/core/src'), { recursive: true }); + writeFileSync(join(dir, 'packages/core/src/old.test.ts'), ''); + const r = run('## Test Plan\n\nRan `packages/core/src/old.test.ts`'); + expect(verdictOf(r.claims, 'packages/core/src/old.test.ts')).toBe( + 'reproduces', + ); + }); + + it('contradicts a path that is in neither the diff nor the tree', () => { + const r = run('## Test Plan\n\nAdded `packages/cli/src/ghost.test.ts`'); + const claim = r.claims.find( + (c) => c.text === 'packages/cli/src/ghost.test.ts', + ); + expect(claim?.verdict).toBe('contradicted'); + expect(claim?.observed).toBe('no such file or directory'); + }); + + it('ignores a line suffix and a trailing slash when resolving a path', () => { + mkdirSync(join(dir, 'packages/cli/src'), { recursive: true }); + writeFileSync(join(dir, 'packages/cli/src/a.ts'), ''); + const r = run( + '## Test Plan\n\nSee `packages/cli/src/a.ts:42` and `packages/cli/`', + ); + expect(verdictOf(r.claims, 'packages/cli/src/a.ts:42')).toBe( + 'reproduces', + ); + expect(verdictOf(r.claims, 'packages/cli/')).toBe('reproduces'); + }); + + it('does not rule on a path that escapes the repo root', () => { + const r = run('## Test Plan\n\nWrote `../other/x.ts`'); + expect(verdictOf(r.claims, '../other/x.ts')).toBe('unchecked'); + }); + + it('never extracts an absolute path as a repo claim', () => { + // `/tmp/out/log.json` is a real thing to write in a Test Plan and is not + // a statement about the repository — it must produce no claim at all. + expect(extractClaims('Wrote `/tmp/out/log.json`')).toEqual([]); + }); + }); + + describe('command claims', () => { + it('reproduces a script the manifests define', () => { + const r = run('## Test Plan\n\nRan `npm run build`'); + expect(verdictOf(r.claims, 'npm run build')).toBe('reproduces'); + }); + + it('finds a script defined by a workspace package, not just the root', () => { + mkdirSync(join(dir, 'packages/cli'), { recursive: true }); + writeFileSync( + join(dir, 'packages/cli/package.json'), + JSON.stringify({ name: '@x/cli', scripts: { 'test:e2e': 'vitest' } }), + ); + const r = run('## Test Plan\n\nRan `npm run test:e2e`'); + expect(verdictOf(r.claims, 'npm run test:e2e')).toBe('reproduces'); + }); + + it('contradicts a script no package defines', () => { + const r = run('## Test Plan\n\nRan `npm run test:ghost`'); + const claim = r.claims.find((c) => c.text === 'npm run test:ghost'); + expect(claim?.verdict).toBe('contradicted'); + expect(claim?.observed).toBe('no package defines this script'); + }); + + it("prefers this review's own exit code over the manifest lookup", () => { + const bt = { + build: [ + { + command: 'npm run build', + exitCode: 1, + seconds: 3, + timedOut: false, + output: 'TS2307', + }, + ], + test: [], + } as unknown as BuildTestReport; + const r = run('## Test Plan\n\nRan `npm run build`', [], bt); + const claim = r.claims.find((c) => c.text === 'npm run build'); + expect(claim?.verdict).toBe('contradicted'); + expect(claim?.observed).toBe('exit 1'); + }); + + it('does not rule on a command killed by the deadline', () => { + // A timeout is an infrastructure result, never a defect in the PR. + const bt = { + build: [ + { + command: 'npm run build', + exitCode: null, + seconds: 120, + timedOut: true, + output: '', + }, + ], + test: [], + } as unknown as BuildTestReport; + const r = run('## Test Plan\n\nRan `npm run build`', [], bt); + // Falls through to the manifest lookup, which defines `build`. + expect(verdictOf(r.claims, 'npm run build')).toBe('reproduces'); + }); + + it('does not rule on a non-npm runner', () => { + const r = run('## Test Plan\n\nRan `make check`'); + expect(verdictOf(r.claims, 'make check')).toBe('unchecked'); + }); + }); + + describe('count claims', () => { + const withCounts = (...counts: number[]) => + ({ + build: [], + test: counts.map((n) => ({ + command: 'npm test', + exitCode: 0, + seconds: 1, + timedOut: false, + output: `Tests ${n} passed (${n})`, + })), + }) as unknown as BuildTestReport; + + it('reproduces a count a suite in this review reported', () => { + const r = run('## Test Plan\n\n471 tests passed', [], withCounts(471)); + expect(verdictOf(r.claims, '471 tests passed')).toBe('reproduces'); + }); + + it('reports a mismatch as `differs`, NOT as a contradiction', () => { + // The whole point: 471 vs 472 may be two different suites, and this + // command cannot tell. It must never become a blocker. + const r = run('## Test Plan\n\n471 tests passed', [], withCounts(472)); + const claim = r.claims.find((c) => c.text === '471 tests passed'); + expect(claim?.verdict).toBe('differs'); + expect(claim?.observed).toBe('472 passed'); + expect(r.claims.some((c) => c.verdict === 'contradicted')).toBe(false); + }); + + it('is unchecked when no suite reported a count', () => { + const r = run('## Test Plan\n\n471 tests passed'); + expect(verdictOf(r.claims, '471 tests passed')).toBe('unchecked'); + }); + }); + + it('summarises the verdicts in its note', () => { + const r = run( + '## Test Plan\n\nAdded `src/ghost.ts`, ran `npm run build`, 9 tests passed', + ); + expect(r.note).toMatch(/1 contradicted/); + expect(r.note).toMatch(/1 reproduced/); + expect(r.note).toMatch(/1 unchecked/); + }); +}); + +describe('the CLI option contract', () => { + // Every test above calls `runTestPlan` with a hand-built args object, which is + // exactly how the flag-name bug got in: yargs' camel-case expansion turns + // `--build-test` into `buildTest`, and a field named `build_test` reads + // `undefined` on every real invocation — silently downgrading every count + // claim to `unchecked` while the whole suite stayed green. + // + // So this test does not assert the parsed shape and stop; asserting yargs + // produces `buildTest` would still pass if `runTestPlan` read some other name. + // It feeds the PARSED object straight into `runTestPlan` and asserts on a + // verdict only reachable when the build-test report was actually loaded. + let dir: string; + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'qwen-test-plan-cli-')); + writeFileSync(join(dir, 'diff.txt'), 'diff'); + writeFileSync( + join(dir, 'plan.json'), + JSON.stringify({ files: [], diffPathAbsolute: join(dir, 'diff.txt') }), + ); + writeFileSync( + join(dir, 'bt.json'), + JSON.stringify({ + build: [], + test: [ + { + command: 'npm test', + exitCode: 0, + seconds: 1, + timedOut: false, + output: 'Tests 472 passed (472)', + }, + ], + }), + ); + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it('parses --build-test into the field runTestPlan actually reads', () => { + const parsed = (testPlanCommand.builder as (y: Argv) => Argv)( + yargs([]), + ).parseSync([ + '--plan', + join(dir, 'plan.json'), + '--pr', + '8176', + '--repo', + 'o/r', + '--worktree', + dir, + '--build-test', + join(dir, 'bt.json'), + ]) as unknown as TestPlanArgs; + + const report = runTestPlan( + parsed, + () => '## Test Plan\n\n471 tests passed', + ); + // `differs` is reachable ONLY if the build-test report was loaded and its + // 472 compared against the claimed 471. A dropped flag yields `unchecked`. + expect(report.claims.map((c) => c.verdict)).toEqual(['differs']); + expect(report.claims[0].observed).toBe('472 passed'); + }); +}); diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts new file mode 100644 index 00000000000..c0896123b96 --- /dev/null +++ b/packages/cli/src/commands/review/test-plan.ts @@ -0,0 +1,630 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review test-plan`: rule on the claims the PR author already wrote down. +// +// A Test Plan is the one place in a pull request where the author states, in +// their own words, what they ran and what they saw — a list of falsifiable +// assertions, handed to the reviewer for free. Nothing in this pipeline read it. +// `pr-context` renders the PR body, but its consumer is Agent 0, whose question +// is root-cause fidelity ("is this the right fix for the linked issue?"), not +// "the author says 471 tests pass — do they?". So a Test Plan could name a file +// the diff never adds, invoke an npm script that does not exist, or report a +// count from three commits ago, and the review would approve around it. +// +// The split is this file's whole design, and it is the one this skill keeps +// arriving at: **determinism owns the evidence, judgment owns the ruling** — but +// only for the claims where determinism can actually own it. Two kinds can be +// settled here with no model and no false positives: +// +// - **A path that is not there.** "Added `packages/core/src/foo.test.ts`" is +// checkable against the reviewed tree. Absent from the diff AND absent from +// the worktree means the sentence describes a commit that is not this one. +// - **An npm script that does not exist.** "Run `npm run test:unit`" is +// checkable against the workspace manifests. If no package defines it, the +// reviewer cannot reproduce the Test Plan by following it. +// +// A third kind — **a test count** — is the one that motivated this command and +// is deliberately NOT ruled as a contradiction. A count is only falsifiable +// against the suite the author meant, and a Test Plan almost never says which +// one; `build-test` runs the subset of workspaces the diff touched, which is +// frequently a different set. Ruling "471 ≠ 472, contradiction" off that +// mismatch would file a defect on arithmetic the command cannot do, and this +// skill's one design philosophy is that a wrong comment costs more than a +// missing one. So a count claim is reported as `differs`: both numbers, side by +// side, framed as claimed-vs-observed. That is what the finding was worth in the +// first place — a note to the author, never a blocker. +// +// Everything else is `unchecked` and says so. An unchecked claim does not cap +// the verdict: capping every PR whose Test Plan contains a prose sentence would +// make them un-Approvable forever, and "write fewer sentences" is not a fix the +// author can apply. It is the same disclosed-but-not-capping treatment +// `script-lint` gives a deferred checker, for the same reason. + +import type { CommandModule } from 'yargs'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { dirname, join, normalize, resolve } from 'node:path'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { gh, setGhHost } from './lib/gh.js'; +import { diffHashOf } from './script-lint.js'; +import { readRootPackage, readWorkspacePackages } from './lib/workspaces.js'; +import type { BuildTestReport } from './build-test.js'; +import type { FileMetric } from './lib/report.js'; + +/** What kind of assertion a claim is, which decides how it can be ruled. */ +export type ClaimKind = 'path' | 'command' | 'count'; + +export type ClaimVerdict = + /** Checked, and the tree agrees. */ + | 'reproduces' + /** Checked, and the tree disagrees. Sound: a real defect in the Test Plan. */ + | 'contradicted' + /** + * Checked against something adjacent, and the numbers are not equal. NOT a + * contradiction — see the header: the claim and the observation may be about + * different suites, and this command cannot tell. Reported, never blocking. + */ + | 'differs' + /** Nothing here can settle it. Disclosed as scope, never capping. */ + | 'unchecked'; + +export interface TestPlanClaim { + kind: ClaimKind; + /** The claim as the author wrote it, for quoting back. */ + text: string; + verdict: ClaimVerdict; + /** What this command observed, when it observed anything. */ + observed?: string; + /** One line: why the verdict is what it is. Rendered to the reader verbatim. */ + note?: string; +} + +export interface TestPlanReport { + /** False when the PR body has no Test Plan section — not a finding. */ + found: boolean; + /** The heading the section was found under, verbatim. */ + heading?: string; + claims: TestPlanClaim[]; + /** + * Hash of the diff this ran against. `compose-review` re-hashes the plan's + * current diff and refuses a report that does not match, exactly as it does + * for `script-lint` — a report from an earlier commit is not this review's. + */ + diffHash?: string; + /** Why the run did what it did, in one line. */ + note: string; +} + +/** + * What a Test Plan calls itself. English and Chinese both, because this repo's + * PRs use either, and a section this command cannot find is a section it + * silently declines to check. + * + * Matched anywhere in the heading TEXT, not anchored to its start. This repo's + * own PR template writes `## Reviewer Test Plan` / `## Reviewer 测试计划`, and an + * anchored pattern found neither — the command returned "no Test Plan section" + * on the very PRs it was built for, which is indistinguishable from an author + * who wrote none. Other templates prefix with `Manual`, `QA`, `How I`. + */ +const PLAN_NAME_RE = + /(test\s*plan|\btesting\b|how\s+(?:has\s+this|to)\s+(?:been\s+)?test(?:ed)?|测试计划|测试方案|测试步骤)/i; + +/** A `#`-style heading: the level, and the text to match the name against. */ +const HEADING_LINE_RE = /^(#{1,6})\s*(\S.*?)\s*$/; + +/** A standalone bold line: `**Test Plan**`, the same heading in another shape. */ +const BOLD_LINE_RE = /^\*\*\s*([^*\n]+?)\s*\*\*:?\s*$/; + +/** + * Pull the Test Plan section out of a PR body. + * + * Ends at the next heading of the SAME OR HIGHER level (`###` closes on `###` + * and on `##`, not on `####`), so a Test Plan with sub-headings keeps them. The + * bold form ends at the next heading of any level or the next standalone bold + * line, which is as much structure as that form carries. + */ +export function extractTestPlanSection( + body: string, +): { heading: string; content: string } | null { + const lines = body.split(/\r?\n/); + // A `#` inside a fenced block is not a heading — it is a shell comment or a + // shebang, and a Test Plan's repro steps are full of both. Scanning without + // this ends the section at the first `#!/usr/bin/env bash` and reports a Test + // Plan that stops one line into its own repro. + const fenced = new Array(lines.length).fill(false); + let inFence = false; + for (let i = 0; i < lines.length; i++) { + if (/^\s*(```|~~~)/.test(lines[i])) { + fenced[i] = true; + inFence = !inFence; + continue; + } + fenced[i] = inFence; + } + + for (let i = 0; i < lines.length; i++) { + if (fenced[i]) continue; + const hash = HEADING_LINE_RE.exec(lines[i]); + const bold = BOLD_LINE_RE.exec(lines[i]); + const name = hash?.[2] ?? bold?.[1]; + if (!name || !PLAN_NAME_RE.test(name)) continue; + // The bold form has no level, so nothing deeper can nest under it; `Infinity` + // makes every `#` heading close it, which is the only sound reading. + const level = hash ? hash[1].length : Infinity; + const out: string[] = []; + for (let j = i + 1; j < lines.length; j++) { + if (!fenced[j]) { + const next = HEADING_LINE_RE.exec(lines[j]); + if (next && next[1].length <= level) break; + if (!hash && (next || BOLD_LINE_RE.test(lines[j]))) break; + } + out.push(lines[j]); + } + return { heading: lines[i].trim(), content: out.join('\n').trim() }; + } + return null; +} + +/** Runners whose presence makes a backticked span a command, not prose. */ +const RUNNER_RE = + /^(npm|npx|yarn|pnpm|bun|make|node|go|cargo|python3?|pytest)\b/; + +/** `foo/bar.ts`, `packages/cli/src/x.tsx:42` — a path, not a sentence. */ +const PATH_RE = /^[\w.@-]+(?:\/[\w.@-]+)+\/?(?::\d+(?::\d+)?)?$/; + +/** + * Counts, in the shapes test runners and humans actually print them. + * + * Deliberately anchored on a test word next to the number. A bare `(42)` in a + * Test Plan is far more often a PR reference or a line number than a count, and + * a wrong count claim produces a `differs` note nobody asked for. + */ +const COUNT_RES = [ + // A Test Plan states its count in the future tense as often as the past: + // "expect all four files and 471 tests **to pass**". Dropping the modal was + // measured against this repo's own PR #8176, where the exact claim this + // command exists to check went unextracted. + /\b(\d+)\s+(?:tests?|specs?|assertions?)\s+(?:(?:to|should|will|would|must)\s+)?(?:pass(?:ed|ing|es)?|green|ok)\b/gi, + /\btests?:?\s+(\d+)\s+pass(?:ed|ing)?\b/gi, + /\b(\d+)\s+pass(?:ed|ing)\b/gi, +]; + +/** Extract every backticked span, including fenced-block bodies. */ +function codeSpans(section: string): string[] { + const spans: string[] = []; + const add = (line: string) => { + // Strip a prompt marker, then anything after a `#` comment: a repro line is + // written `npm test # 471 pass`, and the comment is not part of the command. + const t = line + .trim() + .replace(/^[$>]\s+/, '') + .replace(/\s+#.*$/, '') + .trim(); + if (t) spans.push(t); + }; + const fence = /(?:```|~~~)[^\n]*\n([\s\S]*?)(?:```|~~~)/g; + let m: RegExpExecArray | null; + while ((m = fence.exec(section))) m[1].split('\n').forEach(add); + + const inline = /`([^`\n]+)`/g; + const outsideFences = section.replace(fence, ' '); + while ((m = inline.exec(outsideFences))) add(m[1]); + return spans; +} + +/** + * Turn a Test Plan section into the claims this command can rule on. + * + * Only three kinds are extracted, and prose is not one of them: a sentence has + * no deterministic ruling, so lifting it into the report as an `unchecked` + * entry would produce a list the length of the Test Plan and tell the reader + * nothing they could not get by reading it. The `unchecked` verdict is for a + * claim of a checkable KIND that this run could not settle — a count with no + * observed count to compare against — which is a fact about the run. + */ +export function extractClaims(section: string): Array<{ + kind: ClaimKind; + text: string; +}> { + const claims: Array<{ kind: ClaimKind; text: string }> = []; + const seen = new Set(); + const push = (kind: ClaimKind, text: string) => { + const key = `${kind}:${text}`; + if (seen.has(key)) return; + seen.add(key); + claims.push({ kind, text }); + }; + + for (const span of codeSpans(section)) { + if (RUNNER_RE.test(span)) push('command', span); + if (PATH_RE.test(span)) { + push('path', span); + continue; + } + // Paths named as ARGUMENTS of a command line. A Test Plan's most checkable + // sentence is usually its repro command — "run vitest on these four files" — + // and every one of those files is an existence claim about the tree. + // + // The `cd` prefix is load-bearing, not a nicety. `cd packages/core && npx + // vitest run src/telemetry/loggers.test.ts` names a path that is relative to + // `packages/core`, not to the repo root; resolving it against the root finds + // nothing and files four `contradicted` notes on a PR whose Test Plan was + // correct. Anything more exotic than the leading-`cd` shape keeps its tokens + // unresolved, which is why they are only extracted when there is no `cd` to + // misread. + const cd = /^cd\s+([^\s&;|]+)\s*(?:&&|;)\s*(.*)$/.exec(span); + if (!cd && /(^|\s)cd\s/.test(span)) continue; + const base = cd?.[1] ?? ''; + if (base && PATH_RE.test(base)) push('path', base); + for (const token of (cd?.[2] ?? span).split(/\s+/)) { + const t = token.replace(/[.,;:)'"]+$/, ''); + if (PATH_RE.test(t)) push('path', base ? `${base}/${t}` : t); + } + } + + // The count patterns overlap by construction — `Tests 471 passed` matches + // both the runner-summary shape and the bare ` passed` shape. Matched + // spans are claimed so the more specific pattern (listed first) wins, and one + // statement produces one claim instead of two near-identical ones. + const taken: Array<[number, number]> = []; + for (const re of COUNT_RES) { + re.lastIndex = 0; + let m: RegExpExecArray | null; + while ((m = re.exec(section))) { + const start = m.index; + const end = start + m[0].length; + if (taken.some(([s, e]) => start < e && end > s)) continue; + taken.push([start, end]); + push('count', m[0].trim()); + } + } + return claims; +} + +/** Every test count the runners actually printed, summed per command. */ +export function observedTestCounts(report: BuildTestReport | null): number[] { + if (!report) return []; + const counts: number[] = []; + for (const cmd of report.test ?? []) { + // vitest: `Tests 472 passed (472)`. jest: `Tests: 12 passed, 12 total`. + let total = 0; + let saw = false; + // `Tests 472 passed (472)`, `Tests: 12 passed, 12 total`, and the mixed + // form `Tests 1 failed | 40 passed (41)` — vitest separates with ` | `, + // jest with `, `, so the separator carries its own surrounding whitespace. + const re = /^\s*Tests:?\s+(?:\d+\s+failed\s*[,|]\s*)?(\d+)\s+passed/gim; + let m: RegExpExecArray | null; + while ((m = re.exec(cmd.output ?? ''))) { + total += Number(m[1]); + saw = true; + } + if (saw) counts.push(total); + } + return counts; +} + +/** A path claim's own text reduced to a repo-relative path. */ +function normalizeClaimPath(text: string): string { + return normalize(text.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, '')); +} + +function rulePath( + text: string, + worktree: string, + changed: Set, +): TestPlanClaim { + const path = normalizeClaimPath(text); + if (changed.has(path)) { + return { + kind: 'path', + text, + verdict: 'reproduces', + note: 'the diff changes this file', + }; + } + // A path that escapes the repo root is not a claim about this tree, so it is + // ruled `unchecked`, never "missing" — calling `../scratch/out.json` a + // contradiction would be a finding about the reviewer's filesystem. (Absolute + // paths never reach here: `PATH_RE` does not admit a leading `/`, precisely + // because `/tmp/x.json` is never a claim about the repository.) + if (path.startsWith('..')) { + return { + kind: 'path', + text, + verdict: 'unchecked', + note: 'not a repo-relative path', + }; + } + if (existsSync(join(worktree, path))) { + return { + kind: 'path', + text, + verdict: 'reproduces', + note: 'exists at the reviewed commit (the diff does not change it)', + }; + } + return { + kind: 'path', + text, + verdict: 'contradicted', + observed: 'no such file or directory', + note: 'the Test Plan names a path that is neither in the diff nor in the tree at the reviewed commit', + }; +} + +/** `npm run build` / `npm test` / `npm run x --workspace=y` → the script name. */ +export function npmScriptOf(command: string): string | null { + const m = /^(?:npm|pnpm|yarn|bun)\s+(?:run\s+)?([\w:.-]+)/.exec(command); + if (!m) return null; + const name = m[1]; + // `npm test` / `npm start` are npm's own aliases and need no `run`. + if ( + name === 'run' || + name === 'exec' || + name === 'ci' || + name === 'install' + ) { + return null; + } + return name; +} + +function ruleCommand( + text: string, + worktree: string, + buildTest: BuildTestReport | null, +): TestPlanClaim { + // A command this review actually ran is settled by its exit code — the + // strongest evidence available, and it needs no manifest lookup. + const ran = [...(buildTest?.build ?? []), ...(buildTest?.test ?? [])].find( + (c) => c.command.trim() === text.trim(), + ); + if (ran && !ran.timedOut) { + return ran.exitCode === 0 + ? { + kind: 'command', + text, + verdict: 'reproduces', + observed: 'exit 0', + note: 'this review ran it', + } + : { + kind: 'command', + text, + verdict: 'contradicted', + observed: `exit ${ran.exitCode}`, + note: 'this review ran it and it failed', + }; + } + + const script = npmScriptOf(text); + if (!script) { + return { + kind: 'command', + text, + verdict: 'unchecked', + note: 'not an npm script', + }; + } + const root = readRootPackage(worktree); + const defined = new Set(root?.scripts ?? []); + for (const pkg of readWorkspacePackages(worktree)) { + for (const s of pkg.scripts) defined.add(s); + } + // No manifest could be read at all (a tree this command cannot inspect): + // absent evidence, not evidence of absence. + if (defined.size === 0) { + return { + kind: 'command', + text, + verdict: 'unchecked', + note: 'no package manifest could be read', + }; + } + return defined.has(script) + ? { + kind: 'command', + text, + verdict: 'reproduces', + note: `\`${script}\` is a defined script`, + } + : { + kind: 'command', + text, + verdict: 'contradicted', + observed: 'no package defines this script', + note: 'the Test Plan tells the reviewer to run a script that does not exist at the reviewed commit', + }; +} + +function ruleCount(text: string, observed: number[]): TestPlanClaim { + const claimed = Number(/(\d+)/.exec(text)?.[1]); + if (!observed.length || !Number.isFinite(claimed)) { + return { + kind: 'count', + text, + verdict: 'unchecked', + note: 'no suite in this review reported a pass count to compare against', + }; + } + if (observed.includes(claimed)) { + return { + kind: 'count', + text, + verdict: 'reproduces', + observed: `${claimed} passed`, + note: 'a suite this review ran reported the same count', + }; + } + return { + kind: 'count', + text, + verdict: 'differs', + observed: `${observed.join(', ')} passed`, + // The header's reason, restated where the reader meets the verdict: this is + // not a contradiction, because the two numbers may be about different suites. + note: 'the suites this review ran reported a different count — they may not be the suite the Test Plan means', + }; +} + +export interface TestPlanArgs { + plan: string; + pr: string; + repo: string; + worktree: string; + out?: string; + /** + * yargs' camel-case expansion turns `--build-test` into `buildTest`; naming + * the field for the flag would read `undefined` on every real invocation and + * silently downgrade every count claim to `unchecked`. + */ + buildTest?: string; + host?: string; +} + +/** Production reader: one `gh pr view` for the description body. */ +function fetchPrBody(ownerRepo: string, prNumber: string): string { + return gh( + 'pr', + 'view', + prNumber, + '--repo', + ownerRepo, + '--json', + 'body', + '--jq', + '.body', + ); +} + +export function runTestPlan( + args: TestPlanArgs, + fetchBody: (ownerRepo: string, pr: string) => string = fetchPrBody, +): TestPlanReport { + let plan: { files?: FileMetric[]; diffPathAbsolute?: unknown }; + try { + plan = JSON.parse(readFileSync(args.plan, 'utf8')); + } catch (err) { + throw new Error( + `test-plan: cannot read the plan ${args.plan}: ${(err as Error).message}`, + ); + } + const diffHash = diffHashOf(plan.diffPathAbsolute); + + let body: string; + try { + body = fetchBody(args.repo, args.pr); + } catch (err) { + // A body we could not fetch is not a body with no Test Plan. Say which one + // happened — `found: false` on a failed fetch would read as "the author + // wrote no Test Plan", which is a different (and unearned) statement. + return { + found: false, + claims: [], + diffHash, + note: `the PR description could not be fetched (${(err as Error).message.split('\n')[0]}); no Test Plan was checked`, + }; + } + + const section = extractTestPlanSection(body ?? ''); + if (!section) { + return { + found: false, + claims: [], + diffHash, + note: 'the PR description has no Test Plan section', + }; + } + + const worktree = resolve(args.worktree); + const changed = new Set( + (plan.files ?? []).map((f) => normalize(String(f.path))), + ); + let buildTest: BuildTestReport | null = null; + if (args.buildTest) { + try { + buildTest = JSON.parse( + readFileSync(args.buildTest, 'utf8'), + ) as BuildTestReport; + } catch { + // Absent build/test evidence downgrades count and command claims to + // `unchecked` on their own paths; it is not an error here. + buildTest = null; + } + } + const counts = observedTestCounts(buildTest); + + const claims = extractClaims(section.content).map((c) => { + if (c.kind === 'path') return rulePath(c.text, worktree, changed); + if (c.kind === 'command') return ruleCommand(c.text, worktree, buildTest); + return ruleCount(c.text, counts); + }); + + const contradicted = claims.filter( + (c) => c.verdict === 'contradicted', + ).length; + const differs = claims.filter((c) => c.verdict === 'differs').length; + return { + found: true, + heading: section.heading, + claims, + diffHash, + note: claims.length + ? `checked ${claims.length} claim(s): ${contradicted} contradicted, ${differs} differing, ` + + `${claims.filter((c) => c.verdict === 'reproduces').length} reproduced, ` + + `${claims.filter((c) => c.verdict === 'unchecked').length} unchecked` + : 'the Test Plan states no path, command, or count this command can check', + }; +} + +export const testPlanCommand: CommandModule = { + command: 'test-plan', + describe: + "Rule on the PR Test Plan's checkable claims (paths, npm scripts, test counts) against the reviewed tree", + builder: (yargs) => + yargs + .option('plan', { + type: 'string', + demandOption: true, + describe: 'The plan report from Step 1', + }) + .option('pr', { + type: 'string', + demandOption: true, + describe: 'PR number', + }) + .option('repo', { + type: 'string', + demandOption: true, + describe: 'owner/repo the PR belongs to', + }) + .option('worktree', { + type: 'string', + demandOption: true, + describe: "The PR's worktree — the tree claims are checked against", + }) + .option('build-test', { + type: 'string', + describe: + "Agent 7's build-test report; supplies the observed test counts and exit codes", + }) + .option('out', { type: 'string', describe: 'Write the JSON report here' }) + .option('host', { + type: 'string', + describe: 'GitHub host for GitHub Enterprise (routes every gh call)', + }), + handler: (argv) => { + const args = argv as unknown as TestPlanArgs; + setGhHost(args.host); + const report = runTestPlan(args); + if (args.out) { + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + } + writeStdoutLine(JSON.stringify(report, null, 2)); + writeStderrLine(`test-plan: ${report.note}`); + }, +}; diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 8fb5017d602..583169adf44 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -376,6 +376,65 @@ Two other deliberate limits: - **A test-only diff is never probed.** A new test for old code is _supposed_ to pass with nothing reverted. Probing it would flag every such PR as inert — a false blocker on exactly the PRs we want people to write. - **Findings are Suggestions, not Criticals.** A test that does not gate is not itself wrong code; nothing is broken today. What the finding must say concretely is which behaviour is now shipping unprotected. +## Why a review that only ever had one tree needed the other one + +Every step in this pipeline looks at a single tree. The agents read the PR's code. The verifier traces a failure scenario through the PR's code. Even the probe capability — the one thing here that _runs_ rather than reads — runs against the PR's code alone. The merge base has been known since the first `fetch-pr` (`mergeBaseSha`, resolved and recorded), and it was used for exactly one thing: choosing the diff range. Nothing ever built it. + +That is fine for most findings, because most findings are claims about the code in front of you: this branch is unreachable, this variable is undefined here, this lock is never released. But it leaves a class the review can only ever guess at, and it is a large one, because it is the class the diff itself is _about_: + +- "This changes the output format." +- "This only adds a field; existing consumers are unaffected." +- "This silently drops the error message." +- "Before this, a cancelled call and a failed call were indistinguishable." + +Every one is a statement about the **difference between two programs**, and the review has had one of them. So the difference gets recovered by reading the diff — and that is precisely the reading that fails, for the reason this document keeps rediscovering in other contexts: a diff's new lines are always present and always look correct, and whether they change what anyone observes routinely turns on code the diff never touches. It is the same shape as the `fixed by this diff` trap ("the diff adds a fix" is not "the defect can no longer fire") and the same shape as the documented-intent trap. Both were closed by making the verifier go read something outside the diff. This one cannot be closed that way, because what is outside the diff here is not a _file_ — it is a _build_. + +**With a built base tree the question stops being an argument and becomes an observation.** Feed the same input to both, compare the two outputs. That is a different kind of evidence from anything else in this pipeline: not a better-traced claim, but a measurement, and the only kind that settles a disagreement about what a program used to do. + +Three deliberate limits: + +- **The command builds; it does not run.** Standing up the tree is the expensive, failure-prone half — a detached worktree at the right SHA, a stale sibling from a crashed run, the minimal build set, the widening loop, deadlines a real build can meet — and all of it is decidable, so it is code. _What_ to run is not: it depends entirely on the claim under test, and a fixed scenario would fit almost none of them. The report hands back a path and stops. +- **It is per-finding, not per-review.** A second build is a second build. Paid on a review with a comparative claim it is cheap for what it settles; paid on every review it is a tax most of them get nothing for. So it lives in the verifier's brief as an option, next to the probe, on the same terms. +- **Unavailable is never a finding.** No merge base, a merge base that may be stale (`baseFetchFailed` — an A/B against the wrong base attributes the base branch's own commits to this PR, the two-dot-diff error in another shape), or a base tree that will not compile: each is a fact about the harness. The base failing to build says nothing whatsoever about the PR, and a review that filed it as one would be reporting on its own infrastructure. + +## Why the Test Plan is checked — and why a count mismatch is never a contradiction + +Every other input this pipeline reads is something the review has to derive: the diff, the linked issue, the existing threads, the build's exit code. A Test Plan is different. It is a list of falsifiable assertions the author **already wrote down** and handed over, and until `test-plan` existed the review read none of them. + +Not for want of the text — `pr-context` renders the PR body in full. But its consumer is Agent 0, and Agent 0's question is root-cause fidelity: is this the right fix for the linked issue? "The author says 471 tests pass; do they?" is a different question, nobody owned it, and the answer is frequently no in a way that costs the next reader real time — a path from a commit that got amended away, an `npm run test:unit` that was renamed, a count copied from the first push. + +The split follows this document's recurring line — determinism owns the evidence, judgment owns the ruling — but the interesting part is where it says determinism owns **nothing**. Two claim kinds are decidable here with no model and no false positives: + +- **A path that is not there.** Checkable against the reviewed tree. Absent from the diff _and_ absent from the worktree means the sentence describes some other commit. (Present-but-untouched is not a defect: "ran the existing suite at X" is a normal thing to write, and the ruling says so.) +- **An npm script that does not exist.** Checkable against the workspace manifests. If no package defines it, a reviewer who follows the Test Plan cannot run it. + +**A test count is the third kind, it is the one that motivated the command, and it is deliberately not ruled a contradiction.** The temptation is obvious — the count is right there, `build-test` observed a count, compare them. It is wrong, because a count is only falsifiable against the suite the author meant, and a Test Plan almost never names one. `build-test` runs the subset of workspaces the diff touched; the author ran whatever they ran. `471 ≠ 472` is then a fact about two different measurements, and filing it as a defect is filing arithmetic the command cannot do. So the verdict is `differs`: both numbers, side by side, framed as claimed-versus-observed, and the reader decides. That is what the observation was worth in the first place — a note to the author, never a blocker. The real 471-vs-472 case that prompted this was the mildest item in a four-item review, and the fix was "bump the number". + +**Nothing here blocks and nothing caps**, which makes `testPlanGate` the first gate in this file that is pure disclosure. Both halves are deliberate. A Test Plan defect is not a code defect — the diff is unaffected, and the verdict is about the code; spending the review's one irreversible public action on a documentation nit is exactly the "cry wolf" cost the design philosophy exists to avoid. And capping on a **missing** report would cap essentially every PR, because most produce no notes at all. That is the deferred-checker precedent from `script-lint`, for the identical reason: a limitation the author cannot fix must not make their PR un-Approvable forever. A stale report is dropped in silence rather than failed closed, since there is no cap to fall back to and a note about a previous commit's Test Plan is worse than no note. + +## Why the probe is also per-hunk, when there are already mutants + +The efficacy command asks "does anything gate this change?" three ways, and the third exists because the first two leave a gap that is easy to miss: + +| probe | neutralises | answers | +| ------ | -------------------------------------------------------- | ------------------------ | +| revert | **all** the diff's source, at once | is ANY of this gated? | +| mutant | **one statement**, from a high-precision safety-verb set | is THIS statement gated? | +| hunk | **one hunk** | is THIS change gated? | + +The revert probe is all-or-nothing, and the live dogfood that motivated the mutants showed exactly what that costs: a file with six well-tested behaviours and one untested safety statement reverts red on the six, reports `gated`, and the seventh — the PR's headline invariant — is invisible. The mutants close that, but only for statements the safety-verb set recognises: calls that discard, detach or reset state, and reassignment to an empty collection. That set is deliberately narrow, because a wide one produces mutants nobody should act on. + +So a diff made of **condition changes, return-value changes, format changes, off-by-one fixes** — which is most diffs — generates **zero mutants**, and its only signal is the all-or-nothing revert. A hunk is the natural unit for the missing question: it is the granularity the author wrote and the granularity a reviewer reads, and reverting one at a time is the only way to attribute a still-green suite to a **particular** change rather than to the diff at large. + +Four things this gets right by construction, three of them borrowed from the mutants: + +- **The patch, not a checkout.** `git checkout base -- ` reverts the whole file and the verdict belongs to no particular change — the revert probe's limitation, one level down. Reverse-applying the hunk's own patch keeps the attribution, and `git` does the line-offset arithmetic so a later hunk lands in the right place without this code tracking offsets. +- **The third outcome is still asymmetric.** A patch that will not apply, or a tree that will not compile without the hunk, is `inconclusive` and **never** `killed`. A compile error says nothing about whether a test would have caught a behavioural regression, and scoring it as "a test caught it" is the precise false assurance this whole command exists to remove. +- **Restore by content, never by re-applying forward.** A forward re-apply can fail on its own and would leave the tree neutralised for every probe after it, turning one bad restore into a run of false survivors. +- **Hunks a mutant already covers are skipped**, and the probes run **last**, out of the mutants' leftover budget. The ordering is the priority statement: the safety-verb mutant is the higher-precision experiment, so it is bought first; a hunk probe is what the remainder buys. Both skip counts — cap and budget — are reported, because a hunk probe that never ran must never be readable as a hunk that came back clean. + +The gating mistake worth recording, because it inverted the feature while every test stayed green: the hunk loop first lived **inside** the mutant branch, so it ran only when the diff already had a safety-verb candidate. The one class of diff per-hunk probing exists for — no mutants at all — got nothing. Selection now happens beside the mutants' and the phase runs whenever **either** kind has candidates. + ## Why "fixed by this diff" is the verdict that needed a bar The re-check has three verdicts, and until PR #6486 only two of them cost anything: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c479a0821ca..70d19eb14a7 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -444,7 +444,7 @@ An agent that finds nothing must say so **and say what it walked** — `No issue | `4` | **Performance & efficiency.** N+1s, leaks, needless re-renders, bad data structures, bundle size. **Reproduces the PR's claimed numbers** rather than trusting them — confirms a cheap deterministic claim (bundle bytes, tree-shake) or flags an unreproducible/unsubstantiated benchmark as unverified. | | `5` | **Test coverage.** Specific untested paths in the diff, never "coverage is low"; a missing test is a Suggestion. **Mutation-tests the tests the diff adds/changes** — a test that stays green when the code under it is broken is vacuous — a Suggestion, Critical only when it asserts the opposite, was weakened in-diff, or lets a named incorrect behaviour ship (report the behaviour, not the gap). | | `6a` `6b` `6c` | **Undirected audit, three personas** — attacker, 3 AM oncall, six-months-later maintainer. The framings force diverse paths; the union of what they find is the point, so all three run. | -| `7` | **Build & test verification** (needs a local tree). Runs _one_ build and _one_ test command, and the **test-efficacy probe** — which reverts the diff's source, keeps its tests, and reports the ones that pass anyway, and deletes individual added safety statements (mutants) to find the ones no test notices. Its evidence is the commands it ran. `Source: [build]` / `[test]`, never `[review]`. | +| `7` | **Build & test verification** (needs a local tree). Runs _one_ build and _one_ test command, and the **test-efficacy probe** — which reverts the diff's source, keeps its tests, and reports the ones that pass anyway, deletes individual added safety statements (mutants) to find the ones no test notices, and reverts individual **hunks** one at a time to find the changes no test turns on. Its evidence is the commands it ran. `Source: [build]` / `[test]`, never `[review]`. | | `test-matrix` | **Test coverage matrix** (Step 3B). Maps each behavioural change to the test that exercises it — the pairing a territory agent cannot see, because it holds either the implementation or the test, rarely both. | | `invariant-a` `invariant-b` `invariant-c` | **Whole-file invariants** on a `heavy` file, one checklist slice each: (a) mutable fields, timers, collections; (b) retry counters, ignored return values, error taxonomies; (c) config fields, early returns. | @@ -509,6 +509,8 @@ Write this shard's findings to a file — each with its file, line, issue and fa The brief holds the method the orchestrator used to spell out here and that a paraphrase kept dropping: trace the failure scenario through the real code rather than voting on the finding's prose; engage the diff's own documented intent before calling a documented change a regression (the rule a run skipped when it auto-posted a false "leaks tokens" Critical); the one-way, quote-the-contradiction bar on **rejecting a Critical**; and — when a finding's claim is **runnable** and the repo has a fast unit harness (`vitest`/`jest`/`pytest`) — the option to **write and run a probe** and let the observed behaviour, not a re-reading, settle the verdict. That last one earns its place: measured on this repo, the strongest model traced a real double-execute (`!git push` firing twice) and called it correct; a probe that runs the path reports `sendShellCommand called twice` and the guessing stops. The brief makes the probe evidence rather than theatre with two hard rules — a mandatory self-check that the probe **flips** between buggy and correct, and leaving the tree exactly as found (no probe file, no fix edit, reaches the diff or build). A finding a probe confirmed carries `Source: [probe]`, which `compose-review` treats as deterministic (a run produced it), exactly like `[build]`/`[test]`. Read the brief to know what a verdict means; do not re-derive it here. +The brief also carries the **A/B capability**, which is the probe's counterpart for a claim that a probe structurally cannot settle. A probe runs the PR's code and answers "what does it do now"; it cannot answer "and what did it do before". A whole class of finding is exactly that difference — "this changes the output format", "this only adds a field", "cancelled and failed used to be indistinguishable" — and recovering the old behaviour by reading the diff is the step that goes wrong quietly, because the new lines are always present and always look right. So a verifier facing a comparative claim can run `qwen review base-tree`, which builds the merge base in a sibling worktree, and then run the same input on both sides and quote both outputs. Until this existed, `mergeBaseSha` was used for exactly one thing — choosing the diff range — and no step in this pipeline had ever built the code the PR is a change _to_. It costs one extra build, so it is spent per finding rather than per review, and an unavailable base (no merge base, a stale one, a base that will not compile) is a fact about the harness that never becomes a finding against the PR. + **After verification:** remove all rejected findings. Separate confirmed findings into two groups: high-confidence and low-confidence. Low-confidence findings appear **only in terminal output** (under "Needs Human Review") and are **never posted as PR inline comments** — this preserves the "Silence is better than noise" principle for PR interactions. ### Pattern aggregation @@ -661,6 +663,30 @@ Two failure modes this closes, both observed in this repo's own dogfood: reporti **You do not read its output or decide anything from it — `compose-review` does.** It derives the report's path from the plan (the pr-numbered name above, next to the plan; `qwen-review-script-lint.json` for a local review), reads it as the sole authority, and turns it into the verdict itself: a finding on a **changed line** above cosmetic `style` becomes a **pre-confirmed `[lint]` Critical** that needs no verifier (the tool already ran); an **uninstalled or crashed** checker becomes **unreviewed scope** that caps a would-be Approve; a **deferred** checker — a workflow's embedded `run:` shell, which `actionlint` would lint but whose output this env cannot trust — is **disclosed in the body on every verdict (including Approve) but does not cap**, because it is a tool limitation, not a gap the author can close; and — the proof it ran — a diff that carries an executable script but produced **no readable report** is itself unreviewed (fail closed). That is the whole reason it runs here rather than inside an agent: neither the blocker nor its severity depends on a model, and skipping the command cannot slip an Approve past the fail-closed gate. It is harmless when the diff has no scripts (it reports "nothing to lint"), and it must write to the derived path or `compose-review` will not find it. +### The Test Plan check (deterministic — you run it, not an agent) + +**For a PR review, rule on the claims the author already wrote down.** A Test Plan is the one place in a pull request where the author states, in their own words, what they ran and what they saw — a list of falsifiable assertions, handed to the reviewer for free. Nothing in this pipeline read it. `pr-context` renders the PR body, but its consumer is Agent 0, whose question is root-cause fidelity ("is this the right fix for the linked issue?"), not "the author says 471 tests pass — do they?". So a Test Plan could name a file the diff never adds, invoke an npm script that does not exist, or report a count from three commits ago, and the review would approve around it. + +```bash +"${QWEN_CODE_CLI:-qwen}" review test-plan \ + --plan \ + --pr --repo / \ + --worktree \ + --build-test \ + --out /qwen-review-pr--test-plan.json +# GitHub Enterprise: add --host — it fetches the PR description. +``` + +Run it on a same-repo **PR** review only. A **local** or **file** review has no PR body, and a cross-repo **lightweight** review has no worktree to resolve paths against; the command is skipped in both, and `compose-review` expects nothing from it there. + +**You do not read its output or decide anything from it — `compose-review` does**, from the path derived off the plan, exactly as it does for `script-lint`. What it rules on, and what it deliberately refuses to: + +- A **path** the Test Plan names that is in neither the diff nor the tree at the reviewed commit is `contradicted` — the sentence describes a commit that is not this one. A path that exists but the diff does not touch is fine: "ran the existing suite at X" is a legitimate thing to write. +- An **npm script** the Test Plan tells the reviewer to run that no workspace manifest defines is `contradicted` — the Test Plan cannot be followed. A command this review actually ran is settled by its exit code instead, which outranks the manifest lookup. +- A **test count** that differs from what this review's suites reported is `differs`, and **never** `contradicted`. A count is only falsifiable against the suite the author meant, and a Test Plan almost never says which one; `build-test` runs the subset of workspaces the diff touched, which is frequently a different set. Ruling "471 ≠ 472, contradiction" off that mismatch would file a defect on arithmetic the command cannot do. Both numbers are reported side by side, and the reader decides. + +**None of it blocks, and none of it caps.** A Test Plan defect is not a code defect — the diff is unaffected — and the verdict is about the code. The notes are disclosed in the body on every event including Approve, the same disclosed-but-not-capping treatment a deferred checker gets, and for the same reason: an author cannot fix "you wrote a sentence I could not check", so it must never become a permanent cap. + ### Verdict **You do not decide the verdict, and you do not write it. Ask for it:** @@ -943,6 +969,7 @@ Create the `.qwen/reviews/` directory if it doesn't exist. **For PR worktree mod Report content should include: - Review timestamp and target description +- **Provenance — the commits and the toolchain.** The head SHA reviewed (`fetchedSha` from the fetch report) and the base it was diffed against (`mergeBaseSha`), plus the platform and the Node/npm versions the gates ran on, and one line per gate with its result (`build`, `test`, `script-lint`, `test-efficacy`, `test-plan` — ran / clean / failed / skipped, and why). A saved report is read by someone who cannot re-derive what it was about: without the SHA pair a "Verdict: Approve" names no commit, so it can be neither checked against the PR nor distinguished from an approval of a different head; and without the gate line a reader cannot tell a gate that passed from one that never ran. Both facts are already in reports this run has open — copy them, do not re-measure. - Effort level the review ran at (low / medium / high; **low** findings are marked unverified — medium and high verify them in Step 4) - Diff statistics (files changed, lines added/removed) — omit if reviewing a file with no diff - Build & test results (Agent 7 output summary) — high and medium effort From c0481dd07e81d8ace13b735b5b6bc642b864efdf Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 31 Jul 2026 17:25:56 +0800 Subject: [PATCH 02/12] =?UTF-8?q?fix(review):=20survive=20real=20runner=20?= =?UTF-8?q?output=20=E2=80=94=20ANSI-laced=20and=20trimmed-away=20summarie?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both measured on a live /review of QwenLM/qwen-code#8176 with the built CLI: - test-plan's observedTestCounts strips SGR sequences before matching; a color-enabled pipe interleaves them BETWEEN tokens, and the count claim fell to 'unchecked' with the summary right there in the report. - build-test's trimOutput rescues runner summary lines from the omitted middle (like module-resolution errors): a failing suite's tail is all failure details and npm epilogue, which pushed the one-line summary out of the kept text entirely. --- .../src/commands/review/build-test.test.ts | 23 ++++++++++++++ .../cli/src/commands/review/build-test.ts | 30 +++++++++++++++++-- .../cli/src/commands/review/test-plan.test.ts | 13 ++++++++ packages/cli/src/commands/review/test-plan.ts | 10 ++++++- 4 files changed, 72 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index bedc6426a99..e05c470a3df 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/packages/cli/src/commands/review/build-test.test.ts @@ -10,6 +10,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runBuildTest, + trimOutput, unresolvedWorkspaceDeps, buildRunEnv, } from './build-test.js'; @@ -453,6 +454,28 @@ describe('runBuildTest', () => { command, ) => ({ command, exitCode: 0, seconds: 1, timedOut: false, output: '' }); + it('rescues the runner summary from a trimmed middle', () => { + // A failing suite's tail is all failure details and npm epilogue, which + // pushes the one-line `Tests 3 failed | 1132 passed` summary into the + // omitted middle — measured live on PR #8176, where the count check then + // found no summary anywhere in the kept report. Tested against trimOutput + // directly: the injected exec seam used elsewhere bypasses the trim, which + // is exactly how the gap shipped. + const summary = 'Tests 3 failed | 1132 passed (1135)'; + const trimmed = trimOutput( + 'head\n' + 'x'.repeat(3000) + `\n${summary}\n` + 'y'.repeat(9000), + ); + expect(trimmed).toContain(summary); + expect(trimmed).toContain('runner summaries kept'); + // The colored form a real pipe delivers is rescued too. + const colored = `Tests\x1b[2m \x1b[22m\x1b[31m3 failed\x1b[39m | 1132 passed`; + expect( + trimOutput( + 'h\n' + 'x'.repeat(3000) + `\n${colored}\n` + 'y'.repeat(9000), + ), + ).toContain(colored); + }); + it('buildOnly builds the same set but runs NO tests', () => { // For the merge-base tree an A/B probe compares against: base's suite was // green before this PR existed, so running it measures nothing about the diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 13f7eef9b4a..032f9ce532c 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -112,7 +112,25 @@ const KEEP_TAIL = 6_000; /** The module-resolution errors the widening loop reads to grow the build set. */ const MODULE_ERROR_RE = /Cannot find module '[^']+'|Could not resolve "[^"]+"/; -function trimOutput(s: string): string { +/** + * Runner summary lines, rescued from a trimmed middle like module errors are. + * + * On a FAILING suite the failure details land in the tail and push the + * `Tests 3 failed | 1132 passed` summary into the omitted middle — measured on + * a live review of PR #8176, where `test-plan`'s count check found no summary + * anywhere in an 8 000-char report of a 3-failure run. The summary is the one + * line that says what the whole run amounted to; keep it. + */ +const RUNNER_SUMMARY_RE = /^\s*(?:Tests?|Test Files):?\s/; + +/** SGR color sequences — stripped per line before the summary test, because a + * real runner interleaves them BETWEEN tokens (`Tests\x1b[2m \x1b[22m3 failed`), + * where no anchored pattern can step over them. The rescued line itself keeps + * its original bytes. */ +// eslint-disable-next-line no-control-regex -- ESC is the character under test +const ANSI_SGR_RE = /\x1b\[[0-9;]*m/g; + +export function trimOutput(s: string): string { if (s.length <= KEEP_HEAD + KEEP_TAIL) return s; const middle = s.slice(KEEP_HEAD, s.length - KEEP_TAIL); // Rescue module-resolution errors from the omitted middle. The widening loop @@ -120,10 +138,16 @@ function trimOutput(s: string): string { // find module` line lost to trimming (a long TypeScript log can push one past the // head and before the tail) would end the widening early and surface a real // graph gap as a false build error. Report stays bounded; the signal survives. - const rescued = middle.split('\n').filter((l) => MODULE_ERROR_RE.test(l)); + const rescued = middle + .split('\n') + .filter( + (l) => + MODULE_ERROR_RE.test(l) || + RUNNER_SUMMARY_RE.test(l.replace(ANSI_SGR_RE, '')), + ); const omitted = s.length - KEEP_HEAD - KEEP_TAIL; const marker = rescued.length - ? `\n\n... [${omitted} characters omitted; module-resolution errors kept] ...\n${rescued.join('\n')}\n\n` + ? `\n\n... [${omitted} characters omitted; module-resolution errors and runner summaries kept] ...\n${rescued.join('\n')}\n\n` : `\n\n... [${omitted} characters omitted] ...\n\n`; return s.slice(0, KEEP_HEAD) + marker + s.slice(-KEEP_TAIL); } diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 2f0e6f02560..f19cf7f2a6e 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -212,6 +212,19 @@ describe('observedTestCounts', () => { ).toEqual([12]); }); + it('reads a summary interleaved with ANSI color codes', () => { + // What a real color-enabled pipe delivers — the codes sit BETWEEN tokens, + // so a token-level regex without the strip finds nothing. From a live + // review of PR #8176. + expect( + observedTestCounts( + report([ + 'Tests\x1b[2m \x1b[22m\x1b[1m\x1b[31m3 failed\x1b[39m\x1b[22m\x1b[2m | \x1b[22m\x1b[1m\x1b[32m1132 passed\x1b[39m\x1b[22m (1135)', + ]), + ), + ).toEqual([1132]); + }); + it('reads a summary that also reports failures', () => { expect( observedTestCounts(report(['Tests 1 failed | 40 passed (41)'])), diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index c0896123b96..2110f373503 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -296,8 +296,16 @@ export function observedTestCounts(report: BuildTestReport | null): number[] { // form `Tests 1 failed | 40 passed (41)` — vitest separates with ` | `, // jest with `, `, so the separator carries its own surrounding whitespace. const re = /^\s*Tests:?\s+(?:\d+\s+failed\s*[,|]\s*)?(\d+)\s+passed/gim; + // Strip ANSI SGR sequences first. A real runner writes its summary through + // a color-enabled pipe, so the kept text reads + // `Tests\x1b[2m \x1b[22m\x1b[1m3 failed\x1b[22m…` — the codes sit BETWEEN + // the tokens, and no token-level regex survives that. Measured on a live + // review of PR #8176: the count claim fell to `unchecked` with the summary + // line right there in the report. + // eslint-disable-next-line no-control-regex -- ESC is the character under test + const text = (cmd.output ?? '').replace(/\x1b\[[0-9;]*m/g, ''); let m: RegExpExecArray | null; - while ((m = re.exec(cmd.output ?? ''))) { + while ((m = re.exec(text))) { total += Number(m[1]); saw = true; } From 5054121b08511859493eb7b9d1b020e1e05ebced Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 31 Jul 2026 20:04:52 +0800 Subject: [PATCH 03/12] fix(review): address the eight findings from live review of this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All measured in the review (QwenLM/qwen-code#8215 review comment): - test-plan: linear-time bold-heading scan (the old pattern backtracked catastrophically on an unclosed ** line an untrusted PR body controls); a flag preceding the npm script yields no claim instead of a false 'no package defines this script'. - test-efficacy: a hunk probe's restore recreates the parent directory a reverse-applied 'new file' hunk removed (the ENOENT from finally lost the verdict and marked every remaining hunk inconclusive); hunks get their own skippedForBaseline instead of mislabeling a red baseline as a budget skip; splitDiffIntoHunks re-captures the file header at every diff --git boundary; a hunk-survived finding notes when it restates an inert file-level revert at hunk granularity. - base-tree: idempotent fast path keyed on a build marker + HEAD check — concurrent verifier shards reuse one built tree instead of sweeping it out from under each other mid-A/B (a fabricated base-side difference with a deterministic source tag was the worst case); cost wording is now 'an install and a build' everywhere it was 'one extra build'. --- .../cli/src/commands/review/base-tree.test.ts | 22 +++++++++ packages/cli/src/commands/review/base-tree.ts | 47 +++++++++++++++++-- .../src/commands/review/lib/agent-briefs.ts | 2 +- .../review/test-efficacy.integration.test.ts | 35 ++++++++++++++ .../src/commands/review/test-efficacy.test.ts | 24 ++++++++++ .../cli/src/commands/review/test-efficacy.ts | 35 ++++++++++++-- .../cli/src/commands/review/test-plan.test.ts | 17 +++++++ packages/cli/src/commands/review/test-plan.ts | 14 +++++- .../core/src/skills/bundled/review/DESIGN.md | 2 +- .../core/src/skills/bundled/review/SKILL.md | 2 +- 10 files changed, 188 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index 5448297492a..c6fa0d27fc7 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -117,6 +117,28 @@ describe('runBaseTree', () => { expect(r.build).toBe(okBuild); }); + it('REUSES an already-built base tree instead of sweeping it (concurrent shards)', () => { + // Reviewed live on this PR: N verifier shards run in parallel and all + // resolve the same path; without the fast path, shard B's opening sweep + // destroys the tree shard A is mid-A/B in, and A's base side reads as + // empty output — a fabricated difference with a deterministic source tag. + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + return okBuild; + }; + const first = run({}, build); + expect(first.available).toBe(true); + const second = run({}, build); + expect(second.available).toBe(true); + expect(second.path).toBe(first.path); + expect(second.note).toContain('reusing'); + expect(builds).toHaveLength(1); // one install+build, not two + // A marker for a DIFFERENT sha (rebase between runs) does not shortcut. + writeFileSync(join(first.path!, '.qwen-review-base-ok'), 'f'.repeat(40)); + expect(run({}, build).note).not.toContain('reusing'); + }); + it('recovers from a stale base tree left by a crashed run', () => { const stale = baseWorktreePath(worktree); mkdirSync(stale, { recursive: true }); diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index ebab5b3f76c..bdaa53c8d00 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -36,15 +36,15 @@ // entirely on the claim under test, and a fixed scenario would fit almost none // of them. So the report hands back a path and gets out of the way. // -// Cost is why this is on demand rather than part of every review: a second build -// is a second build. It is worth it for one claim that turns on it and wasted on +// Cost is why this is on demand rather than part of every review: the base +// worktree is a cold checkout, so this is an install AND a build. It is worth it for one claim that turns on it and wasted on // a review with none, which is why the verifier's brief offers it per finding // instead of the pipeline spending it up front. import type { CommandModule } from 'yargs'; import { spawnSync } from 'node:child_process'; import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; -import { dirname, resolve } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { baseWorktreePath } from './lib/paths.js'; import { @@ -82,6 +82,15 @@ export interface BaseTreeArgs { build?: (worktree: string) => BuildTestReport; } +function gitOut(cwd: string, ...args: string[]): string { + const r = spawnSync('git', args, { cwd, encoding: 'utf8' }); + if (r.error) throw r.error; + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${r.stderr ?? ''}`); + } + return (r.stdout ?? '').trim(); +} + function git(cwd: string, ...args: string[]): void { const r = spawnSync('git', args, { cwd, encoding: 'utf8' }); if (r.error) throw r.error; @@ -133,6 +142,35 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { } const tree = baseWorktreePath(worktree); + // Idempotent fast path — and the CONCURRENCY guard. Step 4 launches its + // verifier shards together, the brief offers every one of them this command, + // and they all resolve the same path; without this, shard B's opening sweep + // destroys the tree shard A is mid-A/B in, and A's base side silently reads + // as empty output — a fabricated difference with a deterministic source tag. + // A tree that exists, holds the right commit, and carries the marker a + // successful build wrote is returned as-is: same answer, no clobber, and the + // duplicate install+build cost gone with it. (Not a lock: two shards racing + // the FIRST build can still collide — the window is narrow and the failure + // is a build error, not a wrong verdict. A marker of the wrong SHA — a + // rebase between runs — falls through to the rebuild below.) + const marker = () => join(tree, '.qwen-review-base-ok'); + try { + if ( + existsSync(tree) && + readFileSync(marker(), 'utf8').trim() === baseSha && + gitOut(tree, 'rev-parse', 'HEAD') === baseSha + ) { + return { + available: true, + path: tree, + baseSha, + build: null, + note: `base tree already built at ${baseSha.slice(0, 9)} in ${tree} (reusing it — a concurrent or earlier probe built it)`, + }; + } + } catch { + // No marker, unreadable marker, or a tree git cannot answer for: rebuild. + } let sweep: SweepResult | undefined; try { // Clear a stale base tree left by a crashed run — it would fail `add`. Its @@ -174,6 +212,9 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { }; } + // The marker is what the fast path above trusts, so it is written only after + // a build that succeeded, and it records the SHA it vouches for. + writeFileSync(marker(), `${baseSha}\n`); return { available: true, path: tree, diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index d883f12a1e1..526b3faef89 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -494,7 +494,7 @@ It builds the merge base in a sibling worktree and reports \`available\` and \`p - **Same input, same procedure, both sides.** A difference produced by running two different things is not a difference between the two programs. If you had to build or install differently on one side, say so and treat the result as inconclusive. - **Quote both outputs.** \`BASE: \` / \`PR: \`. The observation is the verdict; a summary of it is a reading again. -- **A/B is expensive — spend it on a claim that turns on it.** One extra build per review, at most. A finding you can settle by tracing does not need this, and \`available: false\` (no merge base, a stale one, or a base that does not build) is a fact about the harness, never a finding against the PR. +- **A/B is expensive — spend it on a claim that turns on it.** An install and a build, once per review at most (the command reuses an already-built base tree, so concurrent verifiers pay once). A finding you can settle by tracing does not need this, and \`available: false\` (no merge base, a stale one, or a base that does not build) is a fact about the harness, never a finding against the PR. A finding an A/B settled carries \`Source: [probe]\` like any other run-produced evidence, with both sides' output quoted. **Do not remove the base tree** — \`cleanup\` sweeps it at the end of the review, and a later finding may need 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 99e848186ca..728ddee2344 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -1274,6 +1274,41 @@ describe('per-hunk probes against real git', () => { expect(reverted).toContain('line2_CHANGED;'); }); + it('restores a hunk-ADDED file whose parent directory the reverse apply removed', () => { + // Reviewed live on this PR: reverse-applying a `new file` hunk deletes the + // directories it emptied, and the old restore threw ENOENT from finally — + // losing the verdict and marking every remaining hunk inconclusive. + write('src/newdir/added.ts', 'export const fresh = 1;\n'); + commitAll('adds a file in a new dir'); + const diff = git( + repo, + 'diff', + '--no-color', + '--src-prefix=a/', + '--dst-prefix=b/', + 'HEAD~1', + 'HEAD', + '--', + 'src/newdir/added.ts', + ); + const [h] = splitDiffIntoHunks(diff); + const got = runOneHunkProbe( + repo, + { + file: 'src/newdir/added.ts', + index: 0, + header: h.header, + startLine: h.startLine, + patch: h.patch, + }, + [], + ); + expect(got.verdict).toBe('inconclusive'); // no probe files collected — honest + expect(readFileSync(join(repo, 'src/newdir/added.ts'), 'utf8')).toBe( + 'export const fresh = 1;\n', + ); + }); + it('restores the file after the run, verdict notwithstanding', () => { const [first] = hunkPatches(); const before = contents(); diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 93c93ddea52..c6e757fe574 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -1211,6 +1211,30 @@ describe('splitDiffIntoHunks', () => { expect(hunks[0].patch).toContain('-@@ old marker'); }); + it('gives each hunk ITS OWN file header on a multi-file diff', () => { + const d = [ + 'diff --git a/one.ts b/one.ts', + '--- a/one.ts', + '+++ b/one.ts', + '@@ -1,1 +1,1 @@', + '-const a = 1;', + '+const a = 2;', + 'diff --git a/two.ts b/two.ts', + '--- a/two.ts', + '+++ b/two.ts', + '@@ -5,1 +5,1 @@', + '-const b = 1;', + '+const b = 2;', + '', + ].join('\n'); + const hunks = splitDiffIntoHunks(d); + expect(hunks).toHaveLength(2); + // The second patch must name the second file, or git applies it to the wrong one. + expect(hunks[1].patch).toContain('diff --git a/two.ts b/two.ts'); + expect(hunks[1].patch).not.toContain('one.ts'); + expect(hunks[0].patch).not.toContain('two.ts'); + }); + it('returns nothing for a diff with no hunks (a binary file)', () => { expect( splitDiffIntoHunks( diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 90bd49a01e8..83420bcdfae 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -1028,10 +1028,20 @@ export function splitDiffIntoHunks( const lines = diffText.split('\n'); const first = lines.findIndex((l) => l.startsWith('@@')); if (first < 0) return []; - const fileHeader = lines.slice(0, first); + // Re-captured at every `diff --git` boundary: a hunk's patch must carry ITS + // file's header, or a multi-file diff hands git a patch naming the wrong + // file. (Latent while hunkProbeInputs diffs one path at a time — but this is + // exported and reads as general-purpose, so it behaves as one.) + let fileHeader = lines.slice(0, first); const out: Array<{ header: string; patch: string; startLine: number }> = []; let i = first; while (i < lines.length) { + if (lines[i].startsWith('diff --git ')) { + const start = i; + while (i < lines.length && !lines[i].startsWith('@@')) i++; + fileHeader = lines.slice(start, i); + continue; + } if (!lines[i].startsWith('@@')) { i++; continue; @@ -1178,7 +1188,11 @@ export function runOneHunkProbe( // Restore by content, not by re-applying the patch forward: a forward apply // can fail on its own and would leave the tree neutralised for every later // probe, turning one bad restore into a run of false survivors. Writing the - // saved bytes back also recreates a file the reverse patch deleted. + // saved bytes back also recreates a file the reverse patch deleted — and + // the parent directory first: reverse-applying a `new file` hunk removes + // the directories it emptied, and a restore that throws ENOENT from this + // finally loses the verdict AND marks every remaining hunk inconclusive. + mkdirSync(dirname(abs), { recursive: true }); writeFileSync(abs, original, 'utf8'); } } @@ -1322,6 +1336,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { const hunkResults: HunkResult[] = []; let hunksSkippedForBudget = 0; let hunksSkippedForCap = 0; + let hunksSkippedForBaseline = 0; let mutantsSkippedForBaseline = 0; let mutantsNote: string | undefined; // Notes can stack (a derailed file AND a red baseline); never clobber one @@ -1480,7 +1495,9 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { .map((r) => r.file); if (greenProbes.length === 0) { mutantsSkippedForBaseline = candidates.length; - hunksSkippedForBudget = hunkCandidates.length; + // Their own reason, not the budget's: the mutants ran zero suites in + // this branch, and "the mutants used the window" would be a false note. + hunksSkippedForBaseline = hunkCandidates.length; noteMutants( 'mutants not run: no probe file was green in the unmutated baseline (every file was red or collected nothing), so a red mutant run would prove nothing', ); @@ -1624,7 +1641,11 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { .map((h) => ({ file: h.file, kind: 'hunk-survived' as const, - message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.)`, + message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.${ + results.some((r) => r.verdict === 'inert') + ? ' A file-level revert probe also came back inert, so this restates that gap at hunk granularity — read the two as one finding, not two.' + : '' + })`, })), ]; @@ -1652,6 +1673,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { .length, skippedForBudget: hunksSkippedForBudget, skippedForCap: hunksSkippedForCap, + skippedForBaseline: hunksSkippedForBaseline, }, findings, cleanupFailure, @@ -1691,6 +1713,11 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { ` ${hunksSkippedForBudget} hunk probe(s) skipped: the mutants used the window`, ); } + if (hunksSkippedForBaseline > 0) { + writeStdoutLine( + ` ${hunksSkippedForBaseline} hunk probe(s) skipped: no probe file was green in the unmutated baseline`, + ); + } if (mutantsNote) { writeStdoutLine(` ${mutantsNote}`); } diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index f19cf7f2a6e..9fd5f918555 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -89,6 +89,15 @@ describe('extractTestPlanSection', () => { ).toBe('ran it'); }); + it('scans a hostile unclosed-bold line in linear time', () => { + // Reviewed live on this PR: the old bold pattern backtracked + // catastrophically (3.2s at 3,000 spaces) on `**` + whitespace with no + // closer — a line an untrusted PR body controls. A regression here does + // not fail an assertion; it hangs the test into vitest's timeout. + const hostile = `## Summary\n\n**${' '.repeat(50_000)}\nplain text`; + expect(extractTestPlanSection(hostile)).toBeNull(); + }); + it('returns null when there is no Test Plan section', () => { expect(extractTestPlanSection('## Summary\n\njust a change')).toBeNull(); }); @@ -255,6 +264,14 @@ describe('npmScriptOf', () => { expect(npmScriptOf('npm run test:unit')).toBe('test:unit'); }); + it('is null when a FLAG precedes the script — never a false script name', () => { + // `--workspace` used to be the capture, and end-to-end that posted + // `no package defines this script` on a correct Test Plan. + expect(npmScriptOf('npm --workspace=packages/cli run build')).toBeNull(); + expect(npmScriptOf('npm -w packages/cli run test')).toBeNull(); + expect(npmScriptOf('yarn --cwd packages/cli build')).toBeNull(); + }); + it('is null for npm verbs that are not scripts, and for non-npm runners', () => { expect(npmScriptOf('npm ci')).toBeNull(); expect(npmScriptOf('npm install')).toBeNull(); diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index 2110f373503..cf6c39dfad9 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -116,7 +116,12 @@ const PLAN_NAME_RE = const HEADING_LINE_RE = /^(#{1,6})\s*(\S.*?)\s*$/; /** A standalone bold line: `**Test Plan**`, the same heading in another shape. */ -const BOLD_LINE_RE = /^\*\*\s*([^*\n]+?)\s*\*\*:?\s*$/; +// No `\s*` on either side of the capture and no lazy quantifier: with all +// three able to match a space, a line opening `**` that never closes made the +// engine walk every split of a whitespace run — measured 3.2s at 3,000 spaces, +// unbounded at GitHub's 65,536-char body cap, on a line an untrusted PR body +// controls. The capture is trimmed at the use site instead. +const BOLD_LINE_RE = /^\*\*([^*\n]+)\*\*:?\s*$/; /** * Pull the Test Plan section out of a PR body. @@ -149,7 +154,7 @@ export function extractTestPlanSection( if (fenced[i]) continue; const hash = HEADING_LINE_RE.exec(lines[i]); const bold = BOLD_LINE_RE.exec(lines[i]); - const name = hash?.[2] ?? bold?.[1]; + const name = hash?.[2] ?? bold?.[1]?.trim(); if (!name || !PLAN_NAME_RE.test(name)) continue; // The bold form has no level, so nothing deeper can nest under it; `Infinity` // makes every `#` heading close it, which is the only sound reading. @@ -368,6 +373,11 @@ export function npmScriptOf(command: string): string | null { const m = /^(?:npm|pnpm|yarn|bun)\s+(?:run\s+)?([\w:.-]+)/.exec(command); if (!m) return null; const name = m[1]; + // A leading flag (`npm --workspace=x run build`, `npm -w x run test`) lands + // in the capture because `-` is word-adjacent in the class. Biased toward + // silence: a claim this cannot parse is `unchecked`, never a false + // `no package defines this script` on a correct Test Plan. + if (name.startsWith('-')) return null; // `npm test` / `npm start` are npm's own aliases and need no `run`. if ( name === 'run' || diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 583169adf44..1306da35fd6 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -394,7 +394,7 @@ Every one is a statement about the **difference between two programs**, and the Three deliberate limits: - **The command builds; it does not run.** Standing up the tree is the expensive, failure-prone half — a detached worktree at the right SHA, a stale sibling from a crashed run, the minimal build set, the widening loop, deadlines a real build can meet — and all of it is decidable, so it is code. _What_ to run is not: it depends entirely on the claim under test, and a fixed scenario would fit almost none of them. The report hands back a path and stops. -- **It is per-finding, not per-review.** A second build is a second build. Paid on a review with a comparative claim it is cheap for what it settles; paid on every review it is a tax most of them get nothing for. So it lives in the verifier's brief as an option, next to the probe, on the same terms. +- **It is per-finding, not per-review.** A cold checkout means an install and a build — the honest price, and why the command's idempotent fast path reuses an already-built tree instead of letting concurrent verifiers each pay it (or worse, sweep it out from under each other mid-A/B). Paid on a review with a comparative claim it is cheap for what it settles; paid on every review it is a tax most of them get nothing for. So it lives in the verifier's brief as an option, next to the probe, on the same terms. - **Unavailable is never a finding.** No merge base, a merge base that may be stale (`baseFetchFailed` — an A/B against the wrong base attributes the base branch's own commits to this PR, the two-dot-diff error in another shape), or a base tree that will not compile: each is a fact about the harness. The base failing to build says nothing whatsoever about the PR, and a review that filed it as one would be reporting on its own infrastructure. ## Why the Test Plan is checked — and why a count mismatch is never a contradiction diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 70d19eb14a7..427884f89f2 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -509,7 +509,7 @@ Write this shard's findings to a file — each with its file, line, issue and fa The brief holds the method the orchestrator used to spell out here and that a paraphrase kept dropping: trace the failure scenario through the real code rather than voting on the finding's prose; engage the diff's own documented intent before calling a documented change a regression (the rule a run skipped when it auto-posted a false "leaks tokens" Critical); the one-way, quote-the-contradiction bar on **rejecting a Critical**; and — when a finding's claim is **runnable** and the repo has a fast unit harness (`vitest`/`jest`/`pytest`) — the option to **write and run a probe** and let the observed behaviour, not a re-reading, settle the verdict. That last one earns its place: measured on this repo, the strongest model traced a real double-execute (`!git push` firing twice) and called it correct; a probe that runs the path reports `sendShellCommand called twice` and the guessing stops. The brief makes the probe evidence rather than theatre with two hard rules — a mandatory self-check that the probe **flips** between buggy and correct, and leaving the tree exactly as found (no probe file, no fix edit, reaches the diff or build). A finding a probe confirmed carries `Source: [probe]`, which `compose-review` treats as deterministic (a run produced it), exactly like `[build]`/`[test]`. Read the brief to know what a verdict means; do not re-derive it here. -The brief also carries the **A/B capability**, which is the probe's counterpart for a claim that a probe structurally cannot settle. A probe runs the PR's code and answers "what does it do now"; it cannot answer "and what did it do before". A whole class of finding is exactly that difference — "this changes the output format", "this only adds a field", "cancelled and failed used to be indistinguishable" — and recovering the old behaviour by reading the diff is the step that goes wrong quietly, because the new lines are always present and always look right. So a verifier facing a comparative claim can run `qwen review base-tree`, which builds the merge base in a sibling worktree, and then run the same input on both sides and quote both outputs. Until this existed, `mergeBaseSha` was used for exactly one thing — choosing the diff range — and no step in this pipeline had ever built the code the PR is a change _to_. It costs one extra build, so it is spent per finding rather than per review, and an unavailable base (no merge base, a stale one, a base that will not compile) is a fact about the harness that never becomes a finding against the PR. +The brief also carries the **A/B capability**, which is the probe's counterpart for a claim that a probe structurally cannot settle. A probe runs the PR's code and answers "what does it do now"; it cannot answer "and what did it do before". A whole class of finding is exactly that difference — "this changes the output format", "this only adds a field", "cancelled and failed used to be indistinguishable" — and recovering the old behaviour by reading the diff is the step that goes wrong quietly, because the new lines are always present and always look right. So a verifier facing a comparative claim can run `qwen review base-tree`, which builds the merge base in a sibling worktree, and then run the same input on both sides and quote both outputs. Until this existed, `mergeBaseSha` was used for exactly one thing — choosing the diff range — and no step in this pipeline had ever built the code the PR is a change _to_. It costs an install and a build (reused across the review once built), so it is spent per finding rather than per review, and an unavailable base (no merge base, a stale one, a base that will not compile) is a fact about the harness that never becomes a finding against the PR. **After verification:** remove all rejected findings. Separate confirmed findings into two groups: high-confidence and low-confidence. Low-confidence findings appear **only in terminal output** (under "Needs Human Review") and are **never posted as PR inline comments** — this preserves the "Silence is better than noise" principle for PR interactions. From 7a3354577df8d0dda2effe1ae2bbb8e8315f90aa Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 31 Jul 2026 16:14:31 +0000 Subject: [PATCH 04/12] fix(cli): never score a hunk survived when its own test left the baseline (#8215) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A per-hunk probe reported `survived` whenever the green baseline probes still passed with the hunk reverted. When the hunk's own collocated test dropped out of the baseline (a probe-tree import error collects nothing), the remaining green probes prove only that THEY do not cover the hunk, so the verdict is now `inconclusive` — the same dropped-test asymmetry the mutants already hold. Also scope the hunk-survived cross-reference note to the hunk's own collocated test, and let test-plan match a workspace-scoped run of the plan's bare command instead of falling through to the manifest on an exact-string miss. --- .../review/test-efficacy.integration.test.ts | 80 +++++++++++++++++++ .../src/commands/review/test-efficacy.test.ts | 25 ++++++ .../cli/src/commands/review/test-efficacy.ts | 61 ++++++++++---- .../cli/src/commands/review/test-plan.test.ts | 43 ++++++++++ packages/cli/src/commands/review/test-plan.ts | 12 ++- 5 files changed, 206 insertions(+), 15 deletions(-) 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 728ddee2344..a17c1df2e63 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -352,6 +352,86 @@ describe('test-efficacy probe isolation (#6832)', () => { expect(treeState(wt)).toEqual(before); }); + it('scores a hunk inconclusive when its OWN collocated test dropped out of the baseline', async () => { + // The false survivor this exists to remove. The probe tree resolves + // `node_modules` by walking up to the repo root, so a probe file that + // transitively imports a workspace-NESTED dependency collects nothing in the + // probe tree and is dropped from the green baseline set. Before the fix the + // hunk probe then ran the OTHER (green) probes, they passed, and the hunk + // was scored `survived` — a false finding, since the one test that covers + // the hunk never ran. Here `price.test.ts` (collocated with the changed + // `price.ts`) collects nothing while an unrelated `other.test.ts` is green. + write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); + write( + 'packages/lib/src/price.ts', + 'export function price(n: number) {\n return n * 2;\n}\n', + ); + const base = commitAll('base'); + write( + 'packages/lib/src/price.ts', + 'export function price(n: number) {\n return n * 3;\n}\n', + ); + write( + 'packages/lib/src/price.test.ts', + 'import { price } from "./price.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof price).toBe("function"));\n', + ); + write( + 'packages/lib/src/other.test.ts', + 'import { it, expect } from "vitest"; it("t", () => expect(1).toBe(1));\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/price.ts', kind: 'source' }, + { path: 'packages/lib/src/price.test.ts', kind: 'test' }, + { path: 'packages/lib/src/other.test.ts', kind: 'test' }, + ], + }), + ); + // The baseline drops the collocated test: `price.test.ts` collects nothing + // (the probe-tree import-error shape); every other file passes. + 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({ + testResults: files.map((f) => ({ + name: path.resolve(f), + assertionResults: + path.basename(f) === 'price.test.ts' ? [] : [{ status: 'passed' }], + })), +})); +`, + ); + chmodSync(bin, 0o755); + + await runHandler({ + report: join(repo, 'report.json'), + worktree: wt, + base, + out: join(repo, 'out.json'), + }); + + const out = JSON.parse(readFileSync(join(repo, 'out.json'), 'utf8')); + // The hunk in price.ts is NOT scored survived: its collocated test never ran + // green, so the green run of the other probe proves nothing about it. + expect(out.hunks.survived).toBe(0); + expect(out.hunks.inconclusive).toBe(1); + expect(out.hunks.probed[0].verdict).toBe('inconclusive'); + expect(out.hunks.probed[0].detail).toContain('collocated test'); + expect( + (out.findings as Array<{ kind: string }>).some( + (f) => f.kind === 'hunk-survived', + ), + ).toBe(false); + }); + it('runs a deletion mutant end-to-end and reports the survivor', async () => { // The dogfood shape at full scale: the PR adds a reset function whose one // safety statement (`state.clear()`) nothing gates. The fake vitest is diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index c6e757fe574..983b9b2b7e0 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -17,6 +17,7 @@ import { selectMutants, parseAddedLines, hasCollocatedNewTest, + collocatedProbe, fitsAnotherMutantRun, probeCreateFailureDetail, probeCleanupFailureDetail, @@ -1060,6 +1061,30 @@ describe('hasCollocatedNewTest', () => { }); }); +describe('collocatedProbe', () => { + it('returns the collocated test path when one is in the probe set', () => { + expect( + collocatedProbe('packages/cli/src/x.ts', [ + 'packages/cli/src/other.test.ts', + 'packages/cli/src/x.test.ts', + ]), + ).toBe('packages/cli/src/x.test.ts'); + expect( + collocatedProbe('packages/cli/src/x.ts', ['packages/cli/src/x.spec.ts']), + ).toBe('packages/cli/src/x.spec.ts'); + }); + + it('returns undefined when no collocated test is probed', () => { + // A different directory, or a basename-suffix collision, is not collocated. + expect( + collocatedProbe('packages/cli/src/x.ts', [ + 'packages/core/src/x.test.ts', + 'packages/cli/src/xy.test.ts', + ]), + ).toBeUndefined(); + }); +}); + describe('classifyMutantRun', () => { // Verdicts flow through the SAME per-file classifier the revert probe uses, // so these fixtures are the vitest-JSON shapes classifyProbeRun already diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 83420bcdfae..046d2061d89 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -628,6 +628,21 @@ export function parseAddedLines(diffText: string): Map { return added; } +/** + * The probe file that is the collocated test of `file` (the repo convention + * `file.test.ts` / `file.spec.ts` beside it), if one is in `testPaths`. + */ +export function collocatedProbe( + file: string, + testPaths: string[], +): string | undefined { + const stem = file.replace(/\.[^./]+$/, ''); + return testPaths.find((t) => { + const tstem = t.replace(/\.[^./]+$/, ''); + return tstem === `${stem}.test` || tstem === `${stem}.spec`; + }); +} + /** * Does the diff add or change a test collocated with this production file? * The repo convention is `file.test.ts` beside `file.ts`. Used only to ORDER @@ -637,11 +652,7 @@ export function hasCollocatedNewTest( file: string, testPaths: string[], ): boolean { - const stem = file.replace(/\.[^./]+$/, ''); - return testPaths.some((t) => { - const tstem = t.replace(/\.[^./]+$/, ''); - return tstem === `${stem}.test` || tstem === `${stem}.spec`; - }); + return collocatedProbe(file, testPaths) !== undefined; } /** @@ -1522,6 +1533,23 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // also means a diff whose mutants consumed the window reports zero // hunk probes with `skippedForBudget` set — never a silent zero. for (const h of hunkCandidates) { + // A hunk whose OWN collocated test the baseline dropped (red, or the + // case this exists for: a probe-tree import error that collected + // nothing) cannot be scored `survived`: the other probes passing + // shows only that THEY do not cover it, not that nothing does, since + // the one test that would catch it never ran. Same asymmetry the + // mutants hold, pointed the other way: there an inconclusive run is + // never `killed`, here an absent covering test is never `survived`. + const own = collocatedProbe(h.file, probes); + if (own && !greenProbes.includes(own)) { + const { patch: _patch, ...meta } = h; + hunkResults.push({ + ...meta, + verdict: 'inconclusive' as const, + detail: `this hunk's collocated test ${own} did not run green in the unmutated baseline (likely a compile or import error in the probe tree), so the remaining probes passing cannot show the hunk is uncovered`, + }); + continue; + } const remaining = mutantDeadline - now(); if (!fitsAnotherMutantRun(remaining, estimatedRunMs)) { hunksSkippedForBudget = @@ -1638,15 +1666,20 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { })), ...hunkResults .filter((h) => h.verdict === 'survived') - .map((h) => ({ - file: h.file, - kind: 'hunk-survived' as const, - message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.${ - results.some((r) => r.verdict === 'inert') - ? ' A file-level revert probe also came back inert, so this restates that gap at hunk granularity — read the two as one finding, not two.' - : '' - })`, - })), + .map((h) => { + const own = collocatedProbe(h.file, probes); + const restatesInert = + !!own && results.some((r) => r.verdict === 'inert' && r.file === own); + return { + file: h.file, + kind: 'hunk-survived' as const, + message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.${ + restatesInert + ? ' A file-level revert probe also came back inert, so this restates that gap at hunk granularity — read the two as one finding, not two.' + : '' + })`, + }; + }), ]; const count = (v: MutantVerdict) => diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 9fd5f918555..bdd45416792 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -450,6 +450,49 @@ describe('runTestPlan', () => { expect(claim?.observed).toBe('exit 1'); }); + it("rules a clean exit from this review's own run as reproduces", () => { + // The exit-1 case above pins one ternary arm; this pins the other, so a + // swap of the two arms cannot pass both tests. + const bt = { + build: [ + { + command: 'npm run build', + exitCode: 0, + seconds: 3, + timedOut: false, + output: '', + }, + ], + test: [], + } as unknown as BuildTestReport; + const r = run('## Test Plan\n\nRan `npm run build`', [], bt); + const claim = r.claims.find((c) => c.text === 'npm run build'); + expect(claim?.verdict).toBe('reproduces'); + expect(claim?.observed).toBe('exit 0'); + }); + + it("matches this review's workspace-scoped run of the plan's bare command", () => { + // build-test runs `npm run build --workspace=...`; the plan names the bare + // `npm run build`. An exact-string match misses it and falls through to the + // manifest, which would report `reproduces` even though the build failed. + const bt = { + build: [ + { + command: 'npm run build --workspace="packages/cli"', + exitCode: 1, + seconds: 3, + timedOut: false, + output: 'TS2307', + }, + ], + test: [], + } as unknown as BuildTestReport; + const r = run('## Test Plan\n\nRan `npm run build`', [], bt); + const claim = r.claims.find((c) => c.text === 'npm run build'); + expect(claim?.verdict).toBe('contradicted'); + expect(claim?.observed).toBe('exit 1'); + }); + it('does not rule on a command killed by the deadline', () => { // A timeout is an infrastructure result, never a defect in the PR. const bt = { diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index cf6c39dfad9..c7ab976c2fd 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -398,7 +398,17 @@ function ruleCommand( // A command this review actually ran is settled by its exit code — the // strongest evidence available, and it needs no manifest lookup. const ran = [...(buildTest?.build ?? []), ...(buildTest?.test ?? [])].find( - (c) => c.command.trim() === text.trim(), + (c) => { + const command = c.command.trim(); + const claimed = text.trim(); + // A workspace-scoped run (`npm run build --workspace=...`) still settles + // the plan's bare command, so match it plus any extra flags — not only an + // exact string. The space guard keeps `build` from matching `build:all`. + return ( + command === claimed || + (command.startsWith(claimed) && command[claimed.length] === ' ') + ); + }, ); if (ran && !ran.timedOut) { return ran.exitCode === 0 From 436809bcab007fdd1d31b95d1dd45f89037f95e5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 1 Aug 2026 01:36:58 +0800 Subject: [PATCH 05/12] fix(review): silence-bias hardening from four live review rounds of this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two blocking findings, reproduced on this PR's own Test Plan: - test-plan files no false contradicted notes: npm rulings move from a four-verb denylist to an allowlist (the run form + npm's script aliases — the ~fifty other builtins each used to become 'no package defines this script'); a slash token is claimed as a repo path only with evidence (an extension or ./ prefix), never when it is a flag's value (--repo owner/repo) or under the review's own temp root; HEADING_LINE_RE drops the same quadratic shape its bold sibling was rewritten to remove. - base-tree gets a real mutual-exclusion lock around sweep+add+build (mkdirSync test-and-set; the loser returns busy instead of deleting the tree the winner is mid-install in), and a failed build writes a settled marker so later shards stop re-paying the install to relearn 'unavailable'. Also: Agent 7's brief now names hunk-survived and the hunks.* counters (it is the report's only consumer, and the finding class was invisible); hunk findings anchor at the first ADDED line instead of up to three context lines above the change. --- .../cli/src/commands/review/base-tree.test.ts | 32 ++++ packages/cli/src/commands/review/base-tree.ts | 145 +++++++++++++----- .../src/commands/review/lib/agent-briefs.ts | 2 + .../src/commands/review/test-efficacy.test.ts | 7 +- .../cli/src/commands/review/test-efficacy.ts | 13 +- .../cli/src/commands/review/test-plan.test.ts | 52 ++++++- packages/cli/src/commands/review/test-plan.ts | 74 ++++++--- 7 files changed, 250 insertions(+), 75 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index c6fa0d27fc7..9787d4f5096 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -139,6 +139,38 @@ describe('runBaseTree', () => { expect(run({}, build).note).not.toContain('reusing'); }); + it('returns BUSY instead of sweeping while another probe holds the build lock', () => { + // Reviewed live: shard B's opening sweep deleted the tree shard A was + // mid-`npm ci` in, and whichever finished stamped the marker for a tree + // the other was still mutating. + mkdirSync(`${baseWorktreePath(worktree)}.lock`, { recursive: true }); + const builds: string[] = []; + const r = run({}, (w) => { + builds.push(w); + return okBuild; + }); + expect(r.available).toBe(false); + expect(r.note).toContain('another probe is building'); + expect(builds).toEqual([]); // no sweep, no build under the lock holder + rmSync(`${baseWorktreePath(worktree)}.lock`, { + recursive: true, + force: true, + }); + }); + + it('a FAILED build is a settled answer — later shards do not re-pay it', () => { + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + return failedBuild; + }; + expect(run({}, build).available).toBe(false); + const second = run({}, build); + expect(second.available).toBe(false); + expect(second.note).toContain('already failed'); + expect(builds).toHaveLength(1); + }); + it('recovers from a stale base tree left by a crashed run', () => { const stale = baseWorktreePath(worktree); mkdirSync(stale, { recursive: true }); diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index bdaa53c8d00..9a60f2f880e 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -43,7 +43,13 @@ import type { CommandModule } from 'yargs'; import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { + existsSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { baseWorktreePath } from './lib/paths.js'; @@ -154,6 +160,7 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // is a build error, not a wrong verdict. A marker of the wrong SHA — a // rebase between runs — falls through to the rebuild below.) const marker = () => join(tree, '.qwen-review-base-ok'); + const failedMarker = () => join(tree, '.qwen-review-base-failed'); try { if ( existsSync(tree) && @@ -171,60 +178,114 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { } catch { // No marker, unreadable marker, or a tree git cannot answer for: rebuild. } - let sweep: SweepResult | undefined; + // A base that FAILED to build is a settled answer too. Without this marker, + // every shard that asks re-sweeps and re-pays the install+build to relearn + // the same "unavailable" — and the sweep destroys the evidence tree the + // failure deliberately leaves standing. + try { + if ( + existsSync(tree) && + readFileSync(failedMarker(), 'utf8').trim() === baseSha + ) { + return { + available: false, + path: tree, + baseSha, + build: null, + note: + `the base tree at ${baseSha.slice(0, 9)} already failed to build (an earlier probe measured it); ` + + 'an A/B is not available for this review (infrastructure, never a finding against the PR)', + }; + } + } catch { + // No failed-marker: proceed to build. + } + // A real mutual-exclusion lock around sweep+add+build, not just the marker. + // The reuse fast path covers the AFTER-build window; this covers the build + // itself: measured in review, shard B's opening sweep deleted the tree shard + // A was mid-`npm ci` in, both installed into the same directory, and + // whichever finished stamped the marker for a tree the other was still + // mutating. `mkdirSync` without `recursive` is the atomic test-and-set; the + // loser returns busy rather than waiting out a multi-minute build. + const lock = `${tree}.lock`; try { - // Clear a stale base tree left by a crashed run — it would fail `add`. Its - // stderr is kept, because it is usually what explains that failure. - sweep = discardWorktree(worktree, tree); - git(worktree, 'worktree', 'add', '--detach', tree, baseSha); - } catch (e) { + mkdirSync(lock); + } catch { return unavailable( - worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')), + 'another probe is building the base tree right now — retry when its ' + + 'marker appears (the fast path will then reuse it), or settle the ' + + 'claim by reading; do not sweep the tree out from under the builder', ); } + try { + return buildBaseTree(baseSha); + } finally { + rmSync(lock, { recursive: true, force: true }); + } + + // The parameter re-narrows: TS narrowing does not cross function scopes. + function buildBaseTree(baseSha: string): BaseTreeReport { + let sweep: SweepResult | undefined; + try { + // Clear a stale base tree left by a crashed run — it would fail `add`. Its + // stderr is kept, because it is usually what explains that failure. + sweep = discardWorktree(worktree, tree); + git(worktree, 'worktree', 'add', '--detach', tree, baseSha); + } catch (e) { + return unavailable( + worktreeCreateFailureDetail('base', e, String(sweep?.stderr ?? '')), + ); + } + + const build = args.build + ? args.build(tree) + : runBuildTest({ + plan: args.plan, + worktree: tree, + timeout: args.timeout, + install: args.install, + // The base tree's own suite says nothing about this PR — it was green + // before the PR existed. What the A/B needs from here is a compiled + // tree to run against. + buildOnly: true, + }); - const build = args.build - ? args.build(tree) - : runBuildTest({ - plan: args.plan, - worktree: tree, - timeout: args.timeout, - install: args.install, - // The base tree's own suite says nothing about this PR — it was green - // before the PR existed. What the A/B needs from here is a compiled - // tree to run against. - buildOnly: true, - }); + if (!build.ok) { + // Leave the tree standing. A base that does not build is a fact worth + // looking at by hand, and deleting the evidence to save a directory is a + // bad trade — `cleanup` sweeps it at the end of the review either way. + // The marker makes the failure a SETTLED answer for every later shard. + try { + writeFileSync(failedMarker(), `${baseSha}\n`); + } catch { + // The tree may be too broken to hold a marker; the next shard repays. + } + return { + available: false, + path: tree, + baseSha, + build, + note: + `the base tree at ${baseSha.slice(0, 9)} did not build, so nothing can be run ` + + 'against it; an A/B is not available for this review (this is an ' + + 'infrastructure result, never a finding against the PR)', + }; + } - if (!build.ok) { - // Leave the tree standing. A base that does not build is a fact worth - // looking at by hand, and deleting the evidence to save a directory is a - // bad trade — `cleanup` sweeps it at the end of the review either way. + // The marker is what the fast path above trusts, so it is written only after + // a build that succeeded, and it records the SHA it vouches for. + writeFileSync(marker(), `${baseSha}\n`); return { - available: false, + available: true, path: tree, baseSha, build, note: - `the base tree at ${baseSha.slice(0, 9)} did not build, so nothing can be run ` + - 'against it; an A/B is not available for this review (this is an ' + - 'infrastructure result, never a finding against the PR)', + `base tree built at ${baseSha.slice(0, 9)} in ${tree}. Run the same input here and in the ` + + 'PR worktree and compare the observed output; a difference is evidence, a ' + + 'reading is not.', }; } - - // The marker is what the fast path above trusts, so it is written only after - // a build that succeeded, and it records the SHA it vouches for. - writeFileSync(marker(), `${baseSha}\n`); - return { - available: true, - path: tree, - baseSha, - build, - note: - `base tree built at ${baseSha.slice(0, 9)} in ${tree}. Run the same input here and in the ` + - 'PR worktree and compare the observed output; a difference is evidence, a ' + - 'reading is not.', - }; } export const baseTreeCommand: CommandModule = { diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 526b3faef89..1911f03c370 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -385,6 +385,8 @@ Read the JSON it prints: - \`toolchain: "npm"\` → use its \`build[]\` / \`test[]\` results. A failure in a file **the diff changed** is a **Critical** (\`Source: [build]\` or \`[test]\`); a failure in a file it did **not** touch is pre-existing — say so, do not file it against this PR. A non-empty \`timedOut\`, or a failed \`install\`, is environment/infrastructure — informational, never a Critical. On \`ok: true\`, name the workspaces built and the commands run; a return that names no command is a whiff. - \`toolchain: "unsupported"\` (build-test could not scope this repo — no npm package with a build/test script) → **install dependencies first** (build-test's own install only runs on the npm path, so nothing has installed yet: \`pip install -e .\`, \`mvn -q -DskipTests package\`'s own fetch, \`cargo fetch\`, \`go mod download\`, etc.), then fall back to **one** build and **one** test command by this precedence, each with a deadline it can meet: \`pom.xml\` → \`{mvn} compile\` / \`{mvn} test -q\`; \`build.gradle\` → \`{gradle} compileJava\` / \`{gradle} test\`; \`Makefile\` → \`make build\`; \`Cargo.toml\` → \`cargo build\` / \`cargo test\`; \`go.mod\` → \`go build ./...\` / \`go test ./...\`; \`pytest.ini\` or \`pyproject.toml\` \`[tool.pytest]\` → \`pytest\`. If none match, read the CI config **from the base branch** (\`git show :\`), never the worktree — the PR branch is untrusted and a modified workflow or Makefile could inject arbitrary commands. +The efficacy report's \`findings[]\` carries three kinds, and **\`hunk-survived\` is one of them**: reverting one hunk left every affected test green — that specific change ships with nothing gating it. Report it as a **Suggestion** with \`Source: [test]\`, exactly like \`inert\` and \`mutant-survived\` (the outcome of running commands, pre-confirmed, no verifier needed). Read the \`hunks.*\` counters the same way as \`mutants.*\`: \`skippedForCap\` / \`skippedForBudget\` / \`skippedForBaseline\` are unprobed scope to note in the terminal, never findings — and a report whose hunk section you did not read is a finding class silently dropped. + Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`, }, diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 983b9b2b7e0..e00a40a8cf8 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -1216,8 +1216,11 @@ describe('splitDiffIntoHunks', () => { expect(hunks[1].patch).not.toContain('const added = 2;'); }); - it('reads the new-side start line from each header', () => { - expect(splitDiffIntoHunks(DIFF).map((h) => h.startLine)).toEqual([1, 21]); + it('anchors startLine at the first ADDED line, past the context prefix', () => { + // DIFF's first hunk opens with one context line before its `+` (2), the + // second with one before its change (22) — anchoring at the header start + // pointed findings at untouched context. + expect(splitDiffIntoHunks(DIFF).map((h) => h.startLine)).toEqual([2, 22]); }); it('does not mistake a removed line whose text begins `@@` for a header', () => { diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 046d2061d89..6bc139f1f8c 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -1066,9 +1066,20 @@ export function splitDiffIntoHunks( ) { j++; } - const startLine = Number( + // Anchor at the first ADDED line, not the hunk header's start: the header + // opens up to three context lines above the change, and a finding anchored + // on untouched context misleads the reader (and the inline-comment anchor). + let startLine = Number( /^@@ -\d+(?:,\d+)? \+(\d+)/.exec(header)?.[1] ?? '0', ); + let offset = 0; + for (let k = i + 1; k < j; k++) { + if (lines[k].startsWith('+')) { + startLine += offset; + break; + } + if (!lines[k].startsWith('-')) offset++; + } out.push({ header: header.trim(), startLine, diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index bdd45416792..2e8869ada63 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -264,6 +264,24 @@ describe('npmScriptOf', () => { expect(npmScriptOf('npm run test:unit')).toBe('test:unit'); }); + it('is null for every npm builtin outside the run form and script aliases', () => { + // The denylist knew four verbs; npm has ~fifty. Each of the rest became a + // false `no package defines this script` on a correct Test Plan. + for (const c of [ + 'npm audit', + 'npm ls --workspaces', + 'npm pack', + 'npm publish --dry-run', + 'npm view qwen-code version', + 'npm outdated', + 'yarn add left-pad', + ]) { + expect(npmScriptOf(c)).toBeNull(); + } + expect(npmScriptOf('npm test')).toBe('test'); // the aliases still rule + expect(npmScriptOf('npm run test:unit')).toBe('test:unit'); + }); + it('is null when a FLAG precedes the script — never a false script name', () => { // `--workspace` used to be the capture, and end-to-end that posted // `no package defines this script` on a correct Test Plan. @@ -384,16 +402,40 @@ describe('runTestPlan', () => { expect(claim?.observed).toBe('no such file or directory'); }); - it('ignores a line suffix and a trailing slash when resolving a path', () => { + it('ignores a line suffix when resolving a path', () => { mkdirSync(join(dir, 'packages/cli/src'), { recursive: true }); writeFileSync(join(dir, 'packages/cli/src/a.ts'), ''); - const r = run( - '## Test Plan\n\nSee `packages/cli/src/a.ts:42` and `packages/cli/`', - ); + const r = run('## Test Plan\n\nSee `packages/cli/src/a.ts:42`'); expect(verdictOf(r.claims, 'packages/cli/src/a.ts:42')).toBe( 'reproduces', ); - expect(verdictOf(r.claims, 'packages/cli/')).toBe('reproduces'); + }); + + it('claims a slash token as a path only with EVIDENCE it is one', () => { + // This PR's own Test Plan produced two false `contradicted` notes before + // this bar: `QwenLM/qwen-code` (a --repo slug) and `.qwen/tmp/review-…` + // (a path the reader is told to CREATE). A bare two-segment token with + // no extension is a slug or a ref far more often than a directory. + const r = run( + '## Test Plan\n\nRun `gh pr view 1 --repo QwenLM/qwen-code`, ' + + 'check `origin/main`, create `.qwen/tmp/review-pr-1/x.json`, ' + + 'then read `packages/cli/` and `./run.sh`', + ); + const texts = r.claims.map((c) => c.text); + expect(texts).not.toContain('QwenLM/qwen-code'); // flag value AND slug + expect(texts).not.toContain('origin/main'); // ref, no extension + expect(texts.some((t) => t.startsWith('.qwen/'))).toBe(false); // temp root + expect(texts).not.toContain('packages/cli/'); // bare dir, no evidence + expect(texts).toContain('./run.sh'); // explicit ./ prefix qualifies + }); + + it('does not claim the VALUE of a flag inside a repro command', () => { + const r = run( + '## Test Plan\n\n`docker compose -f infra/docker-compose.yml up`', + ); + expect(r.claims.map((c) => c.text)).not.toContain( + 'infra/docker-compose.yml', + ); }); it('does not rule on a path that escapes the repo root', () => { diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index c7ab976c2fd..88edd63ef02 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -112,8 +112,13 @@ export interface TestPlanReport { const PLAN_NAME_RE = /(test\s*plan|\btesting\b|how\s+(?:has\s+this|to)\s+(?:been\s+)?test(?:ed)?|测试计划|测试方案|测试步骤)/i; -/** A `#`-style heading: the level, and the text to match the name against. */ -const HEADING_LINE_RE = /^(#{1,6})\s*(\S.*?)\s*$/; +/** + * A `#`-style heading: the level, and everything after it (trimmed at the use + * sites). Zero backtracking by construction — `\s*(\S.*?)\s*$` here was the + * same quadratic shape the bold pattern below was rewritten to remove, on the + * same untrusted line. + */ +const HEADING_LINE_RE = /^(#{1,6})(.*)$/; /** A standalone bold line: `**Test Plan**`, the same heading in another shape. */ // No `\s*` on either side of the capture and no lazy quantifier: with all @@ -154,7 +159,7 @@ export function extractTestPlanSection( if (fenced[i]) continue; const hash = HEADING_LINE_RE.exec(lines[i]); const bold = BOLD_LINE_RE.exec(lines[i]); - const name = hash?.[2] ?? bold?.[1]?.trim(); + const name = (hash?.[2] ?? bold?.[1])?.trim(); if (!name || !PLAN_NAME_RE.test(name)) continue; // The bold form has no level, so nothing deeper can nest under it; `Infinity` // makes every `#` heading close it, which is the only sound reading. @@ -163,6 +168,11 @@ export function extractTestPlanSection( for (let j = i + 1; j < lines.length; j++) { if (!fenced[j]) { const next = HEADING_LINE_RE.exec(lines[j]); + // A bare `#` run with no text is not a heading (the old `\s*\S` bar). + if (next && !next[2].trim()) { + out.push(lines[j]); + continue; + } if (next && next[1].length <= level) break; if (!hash && (next || BOLD_LINE_RE.test(lines[j]))) break; } @@ -243,10 +253,23 @@ export function extractClaims(section: string): Array<{ claims.push({ kind, text }); }; + // A slash token is claimed as a repo path only with EVIDENCE it is one: a + // file extension on its last segment, or an explicit ./ prefix. A bare + // `owner/repo` is far more often a slug (`--repo QwenLM/qwen-code`), and + // `origin/main` a ref — this PR's own Test Plan produced two false + // `contradicted` notes before this bar existed. The review's temp root is + // excluded outright: `.qwen/tmp/...` paths are things a Test Plan tells the + // reader to CREATE, absent at the reviewed commit by construction. + const isPathClaim = (t: string): boolean => { + const bare = t.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, ''); + if (bare.startsWith('.qwen/')) return false; + return /\.\w+$/.test(bare) || t.startsWith('./'); + }; + for (const span of codeSpans(section)) { if (RUNNER_RE.test(span)) push('command', span); if (PATH_RE.test(span)) { - push('path', span); + if (isPathClaim(span)) push('path', span); continue; } // Paths named as ARGUMENTS of a command line. A Test Plan's most checkable @@ -264,9 +287,16 @@ export function extractClaims(section: string): Array<{ if (!cd && /(^|\s)cd\s/.test(span)) continue; const base = cd?.[1] ?? ''; if (base && PATH_RE.test(base)) push('path', base); - for (const token of (cd?.[2] ?? span).split(/\s+/)) { - const t = token.replace(/[.,;:)'"]+$/, ''); - if (PATH_RE.test(t)) push('path', base ? `${base}/${t}` : t); + const tokens = (cd?.[2] ?? span).split(/\s+/); + for (let i = 0; i < tokens.length; i++) { + // A token following a flag is that flag's VALUE (`--repo owner/repo`, + // `-f infra/compose.yml`) — a claim about the tool's argument space, + // not about this tree. + if (i > 0 && tokens[i - 1].startsWith('-')) continue; + const t = tokens[i].replace(/[.,;:)'"]+$/, ''); + if (PATH_RE.test(t) && isPathClaim(t)) { + push('path', base ? `${base}/${t}` : t); + } } } @@ -370,24 +400,18 @@ function rulePath( /** `npm run build` / `npm test` / `npm run x --workspace=y` → the script name. */ export function npmScriptOf(command: string): string | null { - const m = /^(?:npm|pnpm|yarn|bun)\s+(?:run\s+)?([\w:.-]+)/.exec(command); - if (!m) return null; - const name = m[1]; - // A leading flag (`npm --workspace=x run build`, `npm -w x run test`) lands - // in the capture because `-` is word-adjacent in the class. Biased toward - // silence: a claim this cannot parse is `unchecked`, never a false - // `no package defines this script` on a correct Test Plan. - if (name.startsWith('-')) return null; - // `npm test` / `npm start` are npm's own aliases and need no `run`. - if ( - name === 'run' || - name === 'exec' || - name === 'ci' || - name === 'install' - ) { - return null; - } - return name; + // ALLOWLIST, not denylist: only the ` run ` form and npm's own + // script aliases are ruled. A denylist of four verbs read every OTHER npm + // builtin (`npm audit`, `npm pack`, `npm ls`, ~fifty of them) as a script + // name and filed `no package defines this script` on correct Test Plans — + // measured on real PR bodies. The true positive this exists for + // ("`npm run test:unit` was renamed") lives entirely in the allowed forms. + const m = /^(?:npm|pnpm|yarn|bun)\s+run\s+([\w:.-]+)/.exec(command); + if (m && !m[1].startsWith('-')) return m[1]; + const alias = /^(?:npm|pnpm|yarn|bun)\s+(test|start|stop|restart)\b/.exec( + command, + ); + return alias ? alias[1] : null; } function ruleCommand( From 6e0d50c76d6418bcf0e9e6047f0e5fb783826e57 Mon Sep 17 00:00:00 2001 From: Qwen Code Date: Fri, 31 Jul 2026 19:18:08 +0000 Subject: [PATCH 06/12] =?UTF-8?q?fix(review):=20address=20review=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20false-positive=20hardening,=20binary=20diff=20g?= =?UTF-8?q?uard,=20error=20convention=20(#8215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/cli/src/commands/review/base-tree.ts | 2 +- .../cli/src/commands/review/build-test.ts | 2 +- .../src/commands/review/lib/agent-briefs.ts | 2 +- .../src/commands/review/test-efficacy.test.ts | 44 ++++++++++++++++ .../cli/src/commands/review/test-efficacy.ts | 25 +++++++-- .../cli/src/commands/review/test-plan.test.ts | 42 +++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 51 +++++++++++++------ 7 files changed, 147 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index 9a60f2f880e..2a4cae1349d 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -72,7 +72,7 @@ export interface BaseTreeReport { path?: string; /** The commit it holds — the merge base of the PR and its target branch. */ baseSha?: string; - /** The build that ran there; null when the tree could not be created. */ + /** The build that ran there; null when the tree could not be created or a fast-path reuse found it already built. */ build: BuildTestReport | null; /** What happened, in one line. Rendered to the reviewer verbatim. */ note: string; diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 032f9ce532c..857dd514901 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -121,7 +121,7 @@ const MODULE_ERROR_RE = /Cannot find module '[^']+'|Could not resolve "[^"]+"/; * anywhere in an 8 000-char report of a 3-failure run. The summary is the one * line that says what the whole run amounted to; keep it. */ -const RUNNER_SUMMARY_RE = /^\s*(?:Tests?|Test Files):?\s/; +const RUNNER_SUMMARY_RE = /^\s*(?:Tests?|Test Files):?\s+\d/; /** SGR color sequences — stripped per line before the summary test, because a * real runner interleaves them BETWEEN tokens (`Tests\x1b[2m \x1b[22m3 failed`), diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 1911f03c370..81bf6c5ab6c 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -496,7 +496,7 @@ It builds the merge base in a sibling worktree and reports \`available\` and \`p - **Same input, same procedure, both sides.** A difference produced by running two different things is not a difference between the two programs. If you had to build or install differently on one side, say so and treat the result as inconclusive. - **Quote both outputs.** \`BASE: \` / \`PR: \`. The observation is the verdict; a summary of it is a reading again. -- **A/B is expensive — spend it on a claim that turns on it.** An install and a build, once per review at most (the command reuses an already-built base tree, so concurrent verifiers pay once). A finding you can settle by tracing does not need this, and \`available: false\` (no merge base, a stale one, or a base that does not build) is a fact about the harness, never a finding against the PR. +- **A/B is expensive — spend it on a claim that turns on it.** An install and a build (the command reuses an already-built base tree; shards that race the first build may both pay). A finding you can settle by tracing does not need this, and \`available: false\` (no merge base, a stale one, or a base that does not build) is a fact about the harness, never a finding against the PR. A finding an A/B settled carries \`Source: [probe]\` like any other run-produced evidence, with both sides' output quoted. **Do not remove the base tree** — \`cleanup\` sweeps it at the end of the review, and a later finding may need it. diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index e00a40a8cf8..237a053cbc2 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -1271,6 +1271,26 @@ describe('splitDiffIntoHunks', () => { ).toEqual([]); expect(splitDiffIntoHunks('')).toEqual([]); }); + + it("does not swallow a binary file into the next file's header", () => { + // A binary entry has no `@@` hunks; the boundary scan must stop at the + // next `diff --git` rather than running past it into the next file. + const d = [ + 'diff --git a/img.png b/img.png', + 'Binary files a/img.png and b/img.png differ', + 'diff --git a/src/x.ts b/src/x.ts', + '--- a/src/x.ts', + '+++ b/src/x.ts', + '@@ -1,1 +1,1 @@', + '-const a = 1;', + '+const a = 2;', + '', + ].join('\n'); + const hunks = splitDiffIntoHunks(d); + expect(hunks).toHaveLength(1); + expect(hunks[0].patch).toContain('diff --git a/src/x.ts b/src/x.ts'); + expect(hunks[0].patch).not.toContain('img.png'); + }); }); describe('selectHunkProbes', () => { @@ -1328,6 +1348,30 @@ describe('selectHunkProbes', () => { expect(skippedForCap).toBe(3); }); + it('skips deleted files rather than spending cap slots on them', () => { + // A deleted file's hunks are all removals; runOneHunkProbe reads the file + // first and returns `inconclusive` every time. + const deleted = { + file: 'src/gone.ts', + diff: [ + 'diff --git a/src/gone.ts b/src/gone.ts', + 'deleted file mode 100644', + '--- a/src/gone.ts', + '+++ /dev/null', + '@@ -1,3 +0,0 @@', + '-const a = 1;', + '-const b = 2;', + '-const c = 3;', + '', + ].join('\n'), + hasNewTests: false, + mutantLines: [] as number[], + }; + const { selected } = selectHunkProbes([deleted, file()]); + expect(selected.every((c) => c.file !== 'src/gone.ts')).toBe(true); + expect(selected.length).toBeGreaterThan(0); + }); + it('has nothing to probe when every hunk is mutant-covered', () => { const { selected, skippedForCap } = selectHunkProbes([ file({ mutantLines: [2, 21] }), diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 6bc139f1f8c..b3309f2efb2 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -1043,13 +1043,28 @@ export function splitDiffIntoHunks( // file's header, or a multi-file diff hands git a patch naming the wrong // file. (Latent while hunkProbeInputs diffs one path at a time — but this is // exported and reads as general-purpose, so it behaves as one.) - let fileHeader = lines.slice(0, first); + // Start from the LAST `diff --git` before the first `@@`: a binary entry + // before it has no hunks, and including its header in the initial slice + // would hand the first real hunk a patch naming two files. + let headerStart = 0; + for (let k = first - 1; k >= 0; k--) { + if (lines[k].startsWith('diff --git ')) { + headerStart = k; + break; + } + } + let fileHeader = lines.slice(headerStart, first); const out: Array<{ header: string; patch: string; startLine: number }> = []; let i = first; while (i < lines.length) { if (lines[i].startsWith('diff --git ')) { const start = i; - while (i < lines.length && !lines[i].startsWith('@@')) i++; + while ( + i < lines.length && + !lines[i].startsWith('@@') && + !(i > start && lines[i].startsWith('diff --git ')) + ) + i++; fileHeader = lines.slice(start, i); continue; } @@ -1121,6 +1136,10 @@ export function selectHunkProbes( const preferred: HunkCandidate[] = []; const rest: HunkCandidate[] = []; for (const f of files) { + // A deleted file's hunks are all removals; `runOneHunkProbe` reads the + // file first and returns `inconclusive` every time. Spending cap slots on + // guaranteed inconclusives wastes the budget on a delete-heavy diff. + if (f.diff.includes('\n+++ /dev/null')) continue; const hunks = splitDiffIntoHunks(f.diff); hunks.forEach((h, index) => { const end = h.startLine + newSideLength(h.header); @@ -1684,7 +1703,7 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { return { file: h.file, kind: 'hunk-survived' as const, - message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. Nothing in this diff's tests fails when this particular change is undone, so it ships unprotected — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.${ + message: `\`${h.file}:${h.startLine}\` (\`${h.header}\`): reverting this hunk on its own leaves every affected test green. No test in this diff fails when this particular change is undone — confirm an existing test covers it, or add one. (The suite as a whole may still be gated: this says only that THIS change is not what any of it turns on.${ restatesInert ? ' A file-level revert probe also came back inert, so this restates that gap at hunk granularity — read the two as one finding, not two.' : '' diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 2e8869ada63..abe24f52623 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -74,6 +74,14 @@ describe('extractTestPlanSection', () => { expect(s?.content).not.toContain('low'); }); + it('tracks fence markers: a ``` line inside a ~~~ fence does not close it', () => { + const s = extractTestPlanSection( + '## Test Plan\n\n~~~\n```\n# still fenced\nnpm test\n```\n~~~\n\nafter\n\n## Risk\n\nlow', + ); + expect(s?.content).toContain('after'); + expect(s?.content).not.toContain('low'); + }); + it('finds a heading whose name is PREFIXED, not anchored', () => { // This repo's own PR template writes `## Reviewer Test Plan`. An anchored // pattern found neither it nor `## Reviewer 测试计划`, so the command @@ -184,6 +192,32 @@ describe('extractClaims', () => { ).toEqual([]); }); + it('does not read prose inside a quoted argument as a path', () => { + // `-t 'covers write/edit tools'` is a test-name filter, not a path claim. + const claims = extractClaims( + "`npx vitest run src/a.test.ts -t 'covers write/edit tools'`", + ); + expect(claims).toContainEqual({ kind: 'path', text: 'src/a.test.ts' }); + expect( + claims + .filter((c) => c.kind === 'path') + .some((c) => c.text.includes('write/edit')), + ).toBe(false); + }); + + it('bails on path-rebasing flags like --root, as it does on cd', () => { + // `--root ./integration-tests` rebases relative paths like `cd` does; + // resolving them against the repo root files false `contradicted` notes. + const claims = extractClaims( + '`npx vitest run --root ./integration-tests sdk-typescript/perm.test.ts`', + ); + expect( + claims + .filter((c) => c.kind === 'path') + .some((c) => c.text.includes('sdk-typescript')), + ).toBe(false); + }); + it('does not read a bare parenthesised number as a count', () => { expect( extractClaims('Follows up on (#8176).').filter((c) => c.kind === 'count'), @@ -240,6 +274,14 @@ describe('observedTestCounts', () => { ).toEqual([40]); }); + it('reads a three-segment summary (failed | skipped | passed)', () => { + expect( + observedTestCounts( + report(['Tests 2 failed | 3 skipped | 40 passed (45)']), + ), + ).toEqual([40]); + }); + it('sums the summaries within one command and keeps commands separate', () => { expect( observedTestCounts( diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index 88edd63ef02..eaaeed6d5bc 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -145,14 +145,16 @@ export function extractTestPlanSection( // this ends the section at the first `#!/usr/bin/env bash` and reports a Test // Plan that stops one line into its own repro. const fenced = new Array(lines.length).fill(false); - let inFence = false; + let fenceMarker: string | null = null; for (let i = 0; i < lines.length; i++) { - if (/^\s*(```|~~~)/.test(lines[i])) { + const m = /^\s*(```|~~~)/.exec(lines[i]); + if (m) { fenced[i] = true; - inFence = !inFence; + if (!fenceMarker) fenceMarker = m[1]; + else if (m[1] === fenceMarker) fenceMarker = null; continue; } - fenced[i] = inFence; + fenced[i] = fenceMarker !== null; } for (let i = 0; i < lines.length; i++) { @@ -287,7 +289,18 @@ export function extractClaims(section: string): Array<{ if (!cd && /(^|\s)cd\s/.test(span)) continue; const base = cd?.[1] ?? ''; if (base && PATH_RE.test(base)) push('path', base); - const tokens = (cd?.[2] ?? span).split(/\s+/); + // Flags that rebase relative paths (`--root ./integration-tests`) are + // `cd`'s twin: a path token after one is relative to the flag's value, + // not the repo root. Bail like the exotic-`cd` case — the `cd` directory + // above was already pushed. + const rest = cd?.[2] ?? span; + if (/(?:^|\s)(?:--root|--prefix|--cwd|--project|-C)\s/.test(rest)) continue; + // Strip quoted arguments before tokenizing: `-t 'covers write/edit tools'` + // is prose inside a flag value, not a path claim about the tree. + const tokens = rest + .replace(/'[^']*'/g, '') + .replace(/"[^"]*"/g, '') + .split(/\s+/); for (let i = 0; i < tokens.length; i++) { // A token following a flag is that flag's VALUE (`--repo owner/repo`, // `-f infra/compose.yml`) — a claim about the tool's argument space, @@ -327,10 +340,10 @@ export function observedTestCounts(report: BuildTestReport | null): number[] { // vitest: `Tests 472 passed (472)`. jest: `Tests: 12 passed, 12 total`. let total = 0; let saw = false; - // `Tests 472 passed (472)`, `Tests: 12 passed, 12 total`, and the mixed - // form `Tests 1 failed | 40 passed (41)` — vitest separates with ` | `, - // jest with `, `, so the separator carries its own surrounding whitespace. - const re = /^\s*Tests:?\s+(?:\d+\s+failed\s*[,|]\s*)?(\d+)\s+passed/gim; + // `Tests 472 passed (472)`, `Tests: 12 passed, 12 total`, and multi- + // segment forms like `Tests 2 failed | 3 skipped | 40 passed (45)` — + // vitest separates with ` | `, jest with `, `. + const re = /^\s*Tests:?\s+(?:\d+\s+\w+\s*[,|]\s*)*(\d+)\s+passed/gim; // Strip ANSI SGR sequences first. A real runner writes its summary through // a color-enabled pipe, so the kept text reads // `Tests\x1b[2m \x1b[22m\x1b[1m3 failed\x1b[22m…` — the codes sit BETWEEN @@ -671,12 +684,20 @@ export const testPlanCommand: CommandModule = { handler: (argv) => { const args = argv as unknown as TestPlanArgs; setGhHost(args.host); - const report = runTestPlan(args); - if (args.out) { - mkdirSync(dirname(resolve(args.out)), { recursive: true }); - writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + try { + const report = runTestPlan(args); + if (args.out) { + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + } + writeStdoutLine(JSON.stringify(report, null, 2)); + writeStderrLine(`test-plan: ${report.note}`); + } catch (err) { + // A missing/invalid plan makes `runTestPlan` throw. Emit the one-line + // message and a non-zero exit (matching build-test and script-lint), not + // yargs' stack trace — the orchestrator reads a clean error. + writeStderrLine((err as Error).message); + process.exitCode = 1; } - writeStdoutLine(JSON.stringify(report, null, 2)); - writeStderrLine(`test-plan: ${report.note}`); }, }; From ec3fda8ae885626a43be34a1fa69570ae52ecbcb Mon Sep 17 00:00:00 2001 From: Qwen Code Date: Fri, 31 Jul 2026 22:45:52 +0000 Subject: [PATCH 07/12] =?UTF-8?q?fix(review):=20address=20review=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20base-tree=20availability=20gate,=20test-plan=20?= =?UTF-8?q?false=20positives,=20hunk-probe=20ranges=20(#8215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - base-tree: only stamp a base tree available when runBuildTest actually compiled something (ok AND npm toolchain AND a non-empty build). An `unsupported` handoff or an empty npm scope returns ok:true having built nothing; marking that tree available let an A/B read the absence of a build as a behavioural difference. - cleanup: sweep the stale base-tree build lock a killed builder leaves behind. - test-plan: read the root manifest's scripts directly so a root-only script survives when the root defines no build/test; bail on the inline --root=./dir rebasing form; stop treating a positional after an inline --flag=value as the flag's value; prefer a failed scoped run when ruling a bare command; anchor the npm script alias to a full token so `yarn test:unit` is not truncated. - test-efficacy: exclude `\ No newline at end of file` from the startLine offset count; compute the mutant-overlap range from the header's new-side span so it no longer overshoots into a closely following hunk. --- .../cli/src/commands/review/base-tree.test.ts | 36 ++++++++- packages/cli/src/commands/review/base-tree.ts | 8 +- .../cli/src/commands/review/cleanup.test.ts | 15 ++++ packages/cli/src/commands/review/cleanup.ts | 15 ++++ .../src/commands/review/test-efficacy.test.ts | 42 ++++++++++ .../cli/src/commands/review/test-efficacy.ts | 16 +++- .../cli/src/commands/review/test-plan.test.ts | 81 +++++++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 74 +++++++++++------ 8 files changed, 259 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index 9787d4f5096..f424d35878c 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -30,7 +30,12 @@ import { runBaseTree, type BaseTreeReport } from './base-tree.js'; import { baseWorktreePath } from './lib/paths.js'; import type { BuildTestReport } from './build-test.js'; -const okBuild = { ok: true, note: 'built' } as BuildTestReport; +const okBuild = { + ok: true, + toolchain: 'npm', + build: [{ command: 'npm run build', exitCode: 0 }], + note: 'built', +} as unknown as BuildTestReport; const failedBuild = { ok: false, note: 'TS2307', @@ -190,6 +195,35 @@ describe('runBaseTree', () => { expect(r.note).toMatch(/never a finding against the PR/); }); + it('is NOT available when the build handed off without building anything', () => { + // A PR that adds a workspace package maps to no package at the merge base, + // so runBuildTest hands off `unsupported` (ok: true, build: []). Stamping that + // tree available would let an A/B read the missing build as a behavioural diff. + const handoff = { + ok: true, + toolchain: 'unsupported', + build: [], + note: 'handoff', + } as unknown as BuildTestReport; + const r = run({}, () => handoff); + expect(r.available).toBe(false); + expect( + existsSync(join(baseWorktreePath(worktree), '.qwen-review-base-ok')), + ).toBe(false); + }); + + it('is NOT available when npm scoped nothing to compile', () => { + // A docs-only diff (or a package with no build script) runs zero build commands + // and returns ok: true with an empty build[]; that is not a built tree. + const empty = { + ok: true, + toolchain: 'npm', + build: [], + note: 'nothing to build', + } as unknown as BuildTestReport; + expect(run({}, () => empty).available).toBe(false); + }); + it('refuses when the plan carries no mergeBaseSha', () => { const r = run({ plan: { mergeBaseSha: undefined } }); expect(r.available).toBe(false); diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index 2a4cae1349d..07d30ac3976 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -250,7 +250,13 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { buildOnly: true, }); - if (!build.ok) { + // `ok: true` is not enough: `runBuildTest` returns `ok: true` for a handoff + // that built nothing — an `unsupported` toolchain (a changed dir the merge + // base maps to no package, e.g. a package this PR adds), or an npm scope with + // nothing to compile. Such a tree was never built, so it cannot be run against; + // stamping it `available` would let an A/B read the absence of a build as a + // behavioural difference. + if (!build.ok || build.toolchain !== 'npm' || build.build.length === 0) { // Leave the tree standing. A base that does not build is a fact worth // looking at by hand, and deleting the evidence to save a directory is a // bad trade — `cleanup` sweeps it at the end of the review either way. diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 2d75e47b50e..ce819ef0383 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -152,6 +152,21 @@ describe('runCleanup', () => { '/repo/.qwen/tmp/review-pr-123-base', ]); }); + + it('sweeps a stale base-tree build lock left by a killed builder', () => { + // The lock is a plain directory (`mkdirSync` test-and-set), not a worktree, + // so `releaseWorktree` never touches it; a builder killed mid-build leaves it + // behind and every later base-tree probe reports "another probe is building" + // until a manual rm. Cleanup sweeps it at the end of the review. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + + runCleanup('pr-123'); + + expect(mocks.rmSync).toHaveBeenCalledWith( + '/repo/.qwen/tmp/review-pr-123-base.lock', + { recursive: true, force: true }, + ); + }); }); describe('findUnsanctionedIssueComments', () => { diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 57441926c55..5bb010d2b90 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -422,6 +422,21 @@ export function runCleanup(target: string): void { // reason: the suffix must not drift between creator and sweeper. report('base worktree', baseWorktreePath(wt)); + // The base-tree build lock is a plain directory (`mkdirSync` test-and-set), + // not a git worktree, so `releaseWorktree` above does not touch it. A builder + // killed mid-build leaves it behind (its `finally` rmSync never runs), and every + // later base-tree probe for this PR then hits EEXIST and reports "another probe + // is building" until a manual rm. Sweep it here, at the end of the review when no + // builder is active. Best effort only — a lock that will not delete is an + // operational paper-cut, never a wrong verdict, so it does not fail the cleanup. + try { + rmSync(`${baseWorktreePath(wt)}.lock`, { recursive: true, force: true }); + } catch (err) { + writeStderrLine( + `note: could not remove base lock ${baseWorktreePath(wt)}.lock: ${(err as Error).message}`, + ); + } + const branch = reviewBranch(prNumber); if (refExists(branch)) { try { diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 237a053cbc2..83e3f32165b 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -1223,6 +1223,24 @@ describe('splitDiffIntoHunks', () => { expect(splitDiffIntoHunks(DIFF).map((h) => h.startLine)).toEqual([2, 22]); }); + it('does not let a "\\ No newline" marker inflate startLine', () => { + // The marker corresponds to no file line; counting it as context shifts + // startLine off by one per marker, while parseAddedLines already excludes it. + const d = [ + 'diff --git a/f b/f', + '--- a/f', + '+++ b/f', + '@@ -1,2 +1,2 @@', + ' const same = 1;', + '-const old = 2;', + '\\ No newline at end of file', + '+const new = 2;', + '\\ No newline at end of file', + '', + ].join('\n'); + expect(splitDiffIntoHunks(d).map((h) => h.startLine)).toEqual([2]); + }); + it('does not mistake a removed line whose text begins `@@` for a header', () => { // In a unified diff every body line is prefixed, so `-@@ x` is content. const d = [ @@ -1326,6 +1344,30 @@ describe('selectHunkProbes', () => { expect(selected.map((c) => c.startLine)).toEqual([20]); }); + it('does not overshoot the hunk end into a later, unrelated mutant', () => { + // The overlap range is the header's new-side span [start, start+len). The + // old code anchored it at the first ADDED line (past leading context) and added + // the full new-side length, overshooting the real end by the context-line count + // — so a mutant just past the hunk wrongly skipped it and lost probe coverage. + const diff = [ + 'diff --git a/f b/f', + '--- a/f', + '+++ b/f', + '@@ -1,3 +1,4 @@', + ' const a = 1;', + '+const added = 2;', + ' const b = 3;', + ' const c = 4;', + '', + ].join('\n'); + // Hunk covers new-side lines 1-4; a mutant at 5 is outside it and must NOT + // cause the hunk to be skipped. + const { selected } = selectHunkProbes([ + { file: 'f', diff, hasNewTests: false, mutantLines: [5] }, + ]); + expect(selected).toHaveLength(1); + }); + it('puts files whose collocated tests the diff also touches first', () => { const { selected } = selectHunkProbes([ file({ file: 'src/plain.ts', diff: diffOf([1, 1]) }), diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index b3309f2efb2..4d7782ba677 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -1093,7 +1093,10 @@ export function splitDiffIntoHunks( startLine += offset; break; } - if (!lines[k].startsWith('-')) offset++; + // `\ No newline at end of file` marks no file line — exclude it like a + // removal, or startLine drifts off by one per marker (parseAddedLines + // already excludes it the same way). + if (!lines[k].startsWith('-') && !lines[k].startsWith('\\')) offset++; } out.push({ header: header.trim(), @@ -1142,8 +1145,15 @@ export function selectHunkProbes( if (f.diff.includes('\n+++ /dev/null')) continue; const hunks = splitDiffIntoHunks(f.diff); hunks.forEach((h, index) => { - const end = h.startLine + newSideLength(h.header); - if (f.mutantLines.some((n) => n >= h.startLine && n < end)) return; + // Range from the header's new-side start, not `startLine`: that anchor + // sits at the first ADDED line, past leading context, so adding the hunk's + // full new-side length to it overshoots the hunk's end by the context-line + // count and silently skips a mutant in a closely following hunk. + const hunkStart = Number( + /^@@ -\d+(?:,\d+)? \+(\d+)/.exec(h.header)?.[1] ?? '0', + ); + const end = hunkStart + newSideLength(h.header); + if (f.mutantLines.some((n) => n >= hunkStart && n < end)) return; (f.hasNewTests ? preferred : rest).push({ file: f.file, index, diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index abe24f52623..1d25fbbd37b 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -218,6 +218,29 @@ describe('extractClaims', () => { ).toBe(false); }); + it('bails on the inline --root=./dir form too, not only --root ./dir', () => { + // The inline `=` form rebases paths exactly like the spaced one; before this + // bail it extracted them as repo-root-relative and filed false `contradicted`. + const claims = extractClaims( + '`npx vitest run --root=./integration-tests src/b.test.ts`', + ); + expect( + claims + .filter((c) => c.kind === 'path') + .some((c) => c.text.includes('src/b.test.ts')), + ).toBe(false); + }); + + it('still reads a positional path after an inline --flag=value', () => { + // `--reporter=verbose` carries its value in the same token and does NOT + // consume the next one, so the file after it is still a path claim. The old + // skip treated it as the flag's value and silently dropped the path. + const claims = extractClaims( + '`npx vitest run --reporter=verbose src/a.test.ts`', + ); + expect(claims).toContainEqual({ kind: 'path', text: 'src/a.test.ts' }); + }); + it('does not read a bare parenthesised number as a count', () => { expect( extractClaims('Follows up on (#8176).').filter((c) => c.kind === 'count'), @@ -337,6 +360,16 @@ describe('npmScriptOf', () => { expect(npmScriptOf('npm install')).toBeNull(); expect(npmScriptOf('make build')).toBeNull(); }); + + it('does not truncate a run-less `yarn test:unit` to `test`', () => { + // `\b` matched at the `:`, so a correct `test:unit` claim was ruled against + // the wrong script; anchored to a full token, it falls through to unchecked. + expect(npmScriptOf('yarn test:unit')).toBeNull(); + expect(npmScriptOf('pnpm test:e2e')).toBeNull(); + // The bare alias and the `run` form are unchanged. + expect(npmScriptOf('yarn test')).toBe('test'); + expect(npmScriptOf('yarn run test:unit')).toBe('test:unit'); + }); }); describe('runTestPlan', () => { @@ -600,6 +633,54 @@ describe('runTestPlan', () => { const r = run('## Test Plan\n\nRan `make check`'); expect(verdictOf(r.claims, 'make check')).toBe('unchecked'); }); + + it('rules a bare command contradicted when ANY scoped run failed', () => { + // build-test records one scoped command per package and does not stop on + // failure; the first match could be the green package that sorted first, + // stating the opposite of the authoritative `ok: false`. + const bt = { + build: [], + test: [ + { + command: 'npm test --workspace="packages/a"', + exitCode: 0, + seconds: 1, + timedOut: false, + output: '', + }, + { + command: 'npm test --workspace="packages/b"', + exitCode: 1, + seconds: 1, + timedOut: false, + output: 'fail', + }, + ], + } as unknown as BuildTestReport; + const r = run('## Test Plan\n\nRan `npm test`', [], bt); + const claim = r.claims.find((c) => c.text === 'npm test'); + expect(claim?.verdict).toBe('contradicted'); + expect(claim?.observed).toBe('exit 1'); + }); + + it('finds a root-only script when the root defines no build/test', () => { + // `readRootPackage` returns null when the root has neither build nor test, + // which used to drop a root-only `lint` and rule a correct claim contradicted. + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ + workspaces: ['packages/*'], + scripts: { lint: 'eslint .' }, + }), + ); + mkdirSync(join(dir, 'packages/cli'), { recursive: true }); + writeFileSync( + join(dir, 'packages/cli/package.json'), + JSON.stringify({ name: '@x/cli', scripts: { build: 'tsc' } }), + ); + const r = run('## Test Plan\n\nRan `npm run lint`'); + expect(verdictOf(r.claims, 'npm run lint')).toBe('reproduces'); + }); }); describe('count claims', () => { diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index eaaeed6d5bc..a324120b2fc 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -50,7 +50,7 @@ import { dirname, join, normalize, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { gh, setGhHost } from './lib/gh.js'; import { diffHashOf } from './script-lint.js'; -import { readRootPackage, readWorkspacePackages } from './lib/workspaces.js'; +import { readWorkspacePackages } from './lib/workspaces.js'; import type { BuildTestReport } from './build-test.js'; import type { FileMetric } from './lib/report.js'; @@ -294,7 +294,8 @@ export function extractClaims(section: string): Array<{ // not the repo root. Bail like the exotic-`cd` case — the `cd` directory // above was already pushed. const rest = cd?.[2] ?? span; - if (/(?:^|\s)(?:--root|--prefix|--cwd|--project|-C)\s/.test(rest)) continue; + if (/(?:^|\s)(?:--root|--prefix|--cwd|--project|-C)(?:\s|=)/.test(rest)) + continue; // Strip quoted arguments before tokenizing: `-t 'covers write/edit tools'` // is prose inside a flag value, not a path claim about the tree. const tokens = rest @@ -304,8 +305,15 @@ export function extractClaims(section: string): Array<{ for (let i = 0; i < tokens.length; i++) { // A token following a flag is that flag's VALUE (`--repo owner/repo`, // `-f infra/compose.yml`) — a claim about the tool's argument space, - // not about this tree. - if (i > 0 && tokens[i - 1].startsWith('-')) continue; + // not about this tree. The inline `--flag=value` form is the exception: + // it carries its value in the same token and does NOT consume the next + // one, so a positional path after it is still a claim about the tree. + if ( + i > 0 && + tokens[i - 1].startsWith('-') && + !tokens[i - 1].includes('=') + ) + continue; const t = tokens[i].replace(/[.,;:)'"]+$/, ''); if (PATH_RE.test(t) && isPathClaim(t)) { push('path', base ? `${base}/${t}` : t); @@ -421,9 +429,8 @@ export function npmScriptOf(command: string): string | null { // ("`npm run test:unit` was renamed") lives entirely in the allowed forms. const m = /^(?:npm|pnpm|yarn|bun)\s+run\s+([\w:.-]+)/.exec(command); if (m && !m[1].startsWith('-')) return m[1]; - const alias = /^(?:npm|pnpm|yarn|bun)\s+(test|start|stop|restart)\b/.exec( - command, - ); + const alias = + /^(?:npm|pnpm|yarn|bun)\s+(test|start|stop|restart)(?=\s|$)/.exec(command); return alias ? alias[1] : null; } @@ -434,20 +441,29 @@ function ruleCommand( ): TestPlanClaim { // A command this review actually ran is settled by its exit code — the // strongest evidence available, and it needs no manifest lookup. - const ran = [...(buildTest?.build ?? []), ...(buildTest?.test ?? [])].find( - (c) => { - const command = c.command.trim(); - const claimed = text.trim(); - // A workspace-scoped run (`npm run build --workspace=...`) still settles - // the plan's bare command, so match it plus any extra flags — not only an - // exact string. The space guard keeps `build` from matching `build:all`. - return ( - command === claimed || - (command.startsWith(claimed) && command[claimed.length] === ' ') - ); - }, - ); - if (ran && !ran.timedOut) { + const matches = [ + ...(buildTest?.build ?? []), + ...(buildTest?.test ?? []), + ].filter((c) => { + const command = c.command.trim(); + const claimed = text.trim(); + // A workspace-scoped run (`npm run build --workspace=...`) still settles + // the plan's bare command, so match it plus any extra flags — not only an + // exact string. The space guard keeps `build` from matching `build:all`. + return ( + command === claimed || + (command.startsWith(claimed) && command[claimed.length] === ' ') + ); + }); + // build-test records one scoped command per package and does not stop on + // failure, so a bare claim can match several runs. Prefer a failure: if ANY + // scoped run failed, the phase failed, and the bare claim must read + // `contradicted` — the first match could be a green package that merely + // sorted first, stating the opposite of the authoritative `ok: false`. + const ran = + matches.find((c) => !c.timedOut && c.exitCode !== 0) ?? + matches.find((c) => !c.timedOut); + if (ran) { return ran.exitCode === 0 ? { kind: 'command', @@ -474,8 +490,20 @@ function ruleCommand( note: 'not an npm script', }; } - const root = readRootPackage(worktree); - const defined = new Set(root?.scripts ?? []); + // The root manifest's scripts read directly: `readRootPackage` returns null + // when the root defines neither `build` nor `test` (it is scoped to those), + // which would drop a root-only `lint`/`format` from `defined` and rule a + // correct `npm run lint` claim `contradicted`. + let rootScripts: string[] = []; + try { + const rootPkg = JSON.parse( + readFileSync(join(worktree, 'package.json'), 'utf8'), + ) as { scripts?: Record }; + rootScripts = Object.keys(rootPkg.scripts ?? {}); + } catch { + // No readable root manifest; workspace packages may still define scripts. + } + const defined = new Set(rootScripts); for (const pkg of readWorkspacePackages(worktree)) { for (const s of pkg.scripts) defined.add(s); } From eae6485a44cb27881a368684bf7bf77e263b1d57 Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Fri, 31 Jul 2026 23:51:39 +0000 Subject: [PATCH 08/12] =?UTF-8?q?fix(review):=20address=20review=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20diff-header=20false=20positives,=20stale=20prom?= =?UTF-8?q?pt=20enumeration,=20added-file=20hunk=20probes=20(#8215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../cli/src/commands/review/agent-prompt.ts | 15 ++++++---- .../src/commands/review/lib/agent-briefs.ts | 2 +- .../src/commands/review/test-efficacy.test.ts | 25 ++++++++++++++++ .../cli/src/commands/review/test-efficacy.ts | 4 +++ .../cli/src/commands/review/test-plan.test.ts | 30 +++++++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 7 +++++ 6 files changed, 77 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 4096b16eb99..4875c00562a 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -861,8 +861,9 @@ export function buildRoleBrief( '**Then run the test-efficacy probe.** A green suite says the tests pass. It does ' + 'not say they would have failed had the change been wrong, and those are ' + 'different claims. Give this call `timeout: 600000` too — besides the revert ' + - 'probe it runs up to 8 single-statement deletion mutants, each a suite run, and ' + - 'it budgets itself to finish inside that ceiling:', + 'probe it runs up to 8 single-statement deletion mutants and up to 6 per-hunk ' + + 'reverse-apply probes, each a suite run, and it budgets itself to finish inside ' + + 'that ceiling:', '', '```bash', `"\${QWEN_CODE_CLI:-qwen}" review test-efficacy ${resolve(opts.planPath)} \\`, @@ -878,14 +879,18 @@ export function buildRoleBrief( 'is a single safety statement the diff added (a `.clear()`, an `.abort(…)`, a ' + 'reset-to-empty) that was **deleted and every affected test stayed green** — no ' + 'test in the diff fails when it is removed, which the whole-file ' + - "revert cannot see when the file's other, tested behaviours mask it. Report each as a " + + "revert cannot see when the file's other, tested behaviours mask it. " + + '`kind: "hunk-survived"` is a hunk the diff added whose reverse-apply left ' + + '**every affected test green** — that specific change ships with nothing gating it. ' + + 'Report each as a ' + '**Suggestion** with `Source: [test]`, saying plainly which behaviour has no ' + 'test in this diff that would catch its removal. **`inconclusive` is not a ' + - 'finding** — for probes and mutants alike, ' + + 'finding** — for probes, mutants, and hunks alike, ' + "reverting or mutating the source often breaks the test's own compile, and that is " + 'not the test catching anything. Mutants counted in `mutants.skippedForBudget`, ' + '`mutants.skippedForCap`, or `mutants.skippedForBaseline` never ran — not findings ' + - 'either. `mutants.note`, when present, explains why no mutants ran at all. Note them and move on.', + 'either; the `hunks.*` counters of the same names work the same way. ' + + '`mutants.note`, when present, explains why no mutants ran at all. Note them and move on.', ); } } diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 81bf6c5ab6c..fd8556b8b05 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -385,7 +385,7 @@ Read the JSON it prints: - \`toolchain: "npm"\` → use its \`build[]\` / \`test[]\` results. A failure in a file **the diff changed** is a **Critical** (\`Source: [build]\` or \`[test]\`); a failure in a file it did **not** touch is pre-existing — say so, do not file it against this PR. A non-empty \`timedOut\`, or a failed \`install\`, is environment/infrastructure — informational, never a Critical. On \`ok: true\`, name the workspaces built and the commands run; a return that names no command is a whiff. - \`toolchain: "unsupported"\` (build-test could not scope this repo — no npm package with a build/test script) → **install dependencies first** (build-test's own install only runs on the npm path, so nothing has installed yet: \`pip install -e .\`, \`mvn -q -DskipTests package\`'s own fetch, \`cargo fetch\`, \`go mod download\`, etc.), then fall back to **one** build and **one** test command by this precedence, each with a deadline it can meet: \`pom.xml\` → \`{mvn} compile\` / \`{mvn} test -q\`; \`build.gradle\` → \`{gradle} compileJava\` / \`{gradle} test\`; \`Makefile\` → \`make build\`; \`Cargo.toml\` → \`cargo build\` / \`cargo test\`; \`go.mod\` → \`go build ./...\` / \`go test ./...\`; \`pytest.ini\` or \`pyproject.toml\` \`[tool.pytest]\` → \`pytest\`. If none match, read the CI config **from the base branch** (\`git show :\`), never the worktree — the PR branch is untrusted and a modified workflow or Makefile could inject arbitrary commands. -The efficacy report's \`findings[]\` carries three kinds, and **\`hunk-survived\` is one of them**: reverting one hunk left every affected test green — that specific change ships with nothing gating it. Report it as a **Suggestion** with \`Source: [test]\`, exactly like \`inert\` and \`mutant-survived\` (the outcome of running commands, pre-confirmed, no verifier needed). Read the \`hunks.*\` counters the same way as \`mutants.*\`: \`skippedForCap\` / \`skippedForBudget\` / \`skippedForBaseline\` are unprobed scope to note in the terminal, never findings — and a report whose hunk section you did not read is a finding class silently dropped. +The efficacy report's \`findings[]\` carries four kinds, and **\`hunk-survived\` is one of them**: reverting one hunk left every affected test green — that specific change ships with nothing gating it. Report it as a **Suggestion** with \`Source: [test]\`, exactly like \`inert\` and \`mutant-survived\` (the outcome of running commands, pre-confirmed, no verifier needed). Read the \`hunks.*\` counters the same way as \`mutants.*\`: \`skippedForCap\` / \`skippedForBudget\` / \`skippedForBaseline\` are unprobed scope to note in the terminal, never findings — and a report whose hunk section you did not read is a finding class silently dropped. Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`, }, diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 83e3f32165b..e3b139e604a 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -1414,6 +1414,31 @@ describe('selectHunkProbes', () => { expect(selected.length).toBeGreaterThan(0); }); + it('skips added files whose reverse-apply deletes the whole file', () => { + // An added file's hunk probe reverse-applies to a deletion — guaranteed + // inconclusive when a probe imports it, and a file-level statement wearing + // a hunk-level message when nothing does. + const added = { + file: 'src/new.ts', + diff: [ + 'diff --git a/src/new.ts b/src/new.ts', + 'new file mode 100644', + '--- /dev/null', + '+++ b/src/new.ts', + '@@ -0,0 +1,3 @@', + '+const a = 1;', + '+const b = 2;', + '+const c = 3;', + '', + ].join('\n'), + hasNewTests: false, + mutantLines: [] as number[], + }; + const { selected } = selectHunkProbes([added, file()]); + expect(selected.every((c) => c.file !== 'src/new.ts')).toBe(true); + expect(selected.length).toBeGreaterThan(0); + }); + it('has nothing to probe when every hunk is mutant-covered', () => { const { selected, skippedForCap } = selectHunkProbes([ file({ mutantLines: [2, 21] }), diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 4d7782ba677..86f6cb71e3f 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -1142,7 +1142,11 @@ export function selectHunkProbes( // A deleted file's hunks are all removals; `runOneHunkProbe` reads the // file first and returns `inconclusive` every time. Spending cap slots on // guaranteed inconclusives wastes the budget on a delete-heavy diff. + // An added file's reverse-apply deletes the whole file — the same waste + // when a probe imports it, and a file-level statement wearing a hunk-level + // message when nothing does. if (f.diff.includes('\n+++ /dev/null')) continue; + if (f.diff.includes('\n--- /dev/null')) continue; const hunks = splitDiffIntoHunks(f.diff); hunks.forEach((h, index) => { // Range from the header's new-side start, not `startLine`: that anchor diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 1d25fbbd37b..f18881c6b26 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -252,6 +252,36 @@ describe('extractClaims', () => { [], ); }); + + it('skips unified-diff headers pasted into the Test Plan', () => { + // The template's Evidence section invites pasting diffs; their a/ b/ + // prefixes are not path claims about the reviewed tree. + const claims = extractClaims( + [ + '```diff', + 'diff --git a/packages/cli/src/foo.ts b/packages/cli/src/foo.ts', + '--- a/packages/cli/src/foo.ts', + '+++ b/packages/cli/src/foo.ts', + '@@ -1,3 +1,4 @@', + '+added line', + '```', + ].join('\n'), + ); + expect(claims.filter((c) => c.kind === 'path')).toEqual([]); + }); + + it('does not extract a gitignored build-output path as a claim', () => { + // dist/ is absent at the reviewed commit by construction — a Test Plan + // naming it is telling the reader to build, not claiming the commit ships it. + const claims = extractClaims( + 'Run `node packages/cli/dist/index.js --yolo`', + ); + expect( + claims + .filter((c) => c.kind === 'path') + .some((c) => c.text.includes('dist/')), + ).toBe(false); + }); }); describe('observedTestCounts', () => { diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index a324120b2fc..ffb089e19ae 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -265,10 +265,17 @@ export function extractClaims(section: string): Array<{ const isPathClaim = (t: string): boolean => { const bare = t.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, ''); if (bare.startsWith('.qwen/')) return false; + // Build output is gitignored — absent at the reviewed commit by + // construction, the same category as .qwen/ above. + if (/(?:^|\/)(?:dist|build|out|bundle|coverage|node_modules)\//.test(bare)) + return false; return /\.\w+$/.test(bare) || t.startsWith('./'); }; for (const span of codeSpans(section)) { + // A unified diff pasted into the Test Plan (the template's Evidence + // section invites it) is not a set of path claims about the tree. + if (/^(?:diff --git|---|\+\+\+|@@)\s/.test(span)) continue; if (RUNNER_RE.test(span)) push('command', span); if (PATH_RE.test(span)) { if (isPathClaim(span)) push('path', span); From 416622c2dcb70129e7788f1984b6552051675805 Mon Sep 17 00:00:00 2001 From: Qwen Code Autofix Date: Sat, 1 Aug 2026 01:56:26 +0000 Subject: [PATCH 09/12] =?UTF-8?q?fix(review):=20address=20review=20feedbac?= =?UTF-8?q?k=20=E2=80=94=20cd-base=20exclusion,=20Test=20Files=20count=20g?= =?UTF-8?q?uard,=20base-tree=20error=20handling,=20probe=20delegation=20(#?= =?UTF-8?q?8215)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/cli/src/commands/review/base-tree.ts | 23 +++++++++----- .../commands/review/compose-review.test.ts | 15 +++++++++ .../cli/src/commands/review/compose-review.ts | 8 +++++ .../cli/src/commands/review/test-efficacy.ts | 31 +++++++------------ .../cli/src/commands/review/test-plan.test.ts | 29 +++++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 25 +++++++++------ 6 files changed, 94 insertions(+), 37 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index 07d30ac3976..55fe4f80635 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -280,7 +280,11 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // The marker is what the fast path above trusts, so it is written only after // a build that succeeded, and it records the SHA it vouches for. - writeFileSync(marker(), `${baseSha}\n`); + try { + writeFileSync(marker(), `${baseSha}\n`); + } catch { + // The tree may be too broken to hold a marker; the next shard rebuilds. + } return { available: true, path: tree, @@ -324,12 +328,17 @@ export const baseTreeCommand: CommandModule = { }), handler: (argv) => { const args = argv as unknown as BaseTreeArgs; - const report = runBaseTree(args); - if (args.out) { - mkdirSync(dirname(resolve(args.out)), { recursive: true }); - writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + try { + const report = runBaseTree(args); + if (args.out) { + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + writeFileSync(resolve(args.out), JSON.stringify(report, null, 2)); + } + writeStdoutLine(JSON.stringify(report, null, 2)); + writeStderrLine(`base-tree: ${report.note}`); + } catch (err) { + writeStderrLine((err as Error).message); + process.exitCode = 1; } - writeStdoutLine(JSON.stringify(report, null, 2)); - writeStderrLine(`base-tree: ${report.note}`); }, }; diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index e508222e07f..9ee9119ef07 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -3090,4 +3090,19 @@ describe('testPlanGate — Test Plan rulings, disclosed but never capping', () = expect(testPlanGate(writePlan()).notes).toEqual([]); expect(testPlanGate(join(dir, 'nope.json')).notes).toEqual([]); }); + + it('caps notes at five plus a summary line', () => { + const p = writePlan(); + writeReport( + Array.from({ length: 8 }, (_, i) => ({ + kind: 'count', + text: `${i + 1} passed`, + verdict: 'differs', + observed: '999 passed', + })), + ); + const notes = testPlanGate(p).notes; + expect(notes).toHaveLength(6); + expect(notes[5]).toBe('and 3 more'); + }); }); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 381701c7833..166f69a273b 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1429,6 +1429,14 @@ export function testPlanGate(planPath: string): { notes: string[] } { ); } } + // The same cap discipline the mutant/hunk probes apply: unbounded notes + // joined into one line drown the verdict they ride on. + const MAX_NOTES = 5; + if (notes.length > MAX_NOTES) { + const extra = notes.length - MAX_NOTES; + notes.length = MAX_NOTES; + notes.push(`and ${extra} more`); + } return { notes }; } diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 86f6cb71e3f..f437f52e9ab 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -62,7 +62,11 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.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). -import { discardWorktree, type SweepResult } from './lib/worktree.js'; +import { + discardWorktree, + worktreeCreateFailureDetail, + type SweepResult, +} from './lib/worktree.js'; import { isWorkspaceMember } from './lib/workspaces.js'; export type ProbeVerdict = 'gated' | 'inert' | 'inconclusive'; @@ -900,29 +904,11 @@ export function safeRmWithin(worktree: string, relPath: string): void { const existsAtBase = (cwd: string, base: string, path: string) => existsAtRev(cwd, base, path); -/** - * The `inconclusive` detail for a probe worktree that could not be created. - * - * Pure, and extracted for that reason: the branch it lives on fires only when - * `git worktree add` fails, and there is no portable way to force that in a - * real-git test — the one lever (making `.git/worktrees` unwritable) is bypassed - * by root and behaves differently under CI's unprivileged user, so a test built - * on it would assert one thing locally and another in CI. The composition is the - * part with logic in it, so it is testable here on its own. - * - * The stale-sweep's stderr is folded in because it is usually the explanation: - * when `add` fails on a leftover the sweep could not clear, the sweep is what - * says why. - */ export function probeCreateFailureDetail( err: unknown, sweepStderr: string, ): string { - const sweepErr = sweepStderr.trim(); - return ( - `probe worktree could not be created: ${err instanceof Error ? err.message : String(err)}` + - (sweepErr ? ` (stale-tree sweep also reported: ${sweepErr})` : '') - ); + return worktreeCreateFailureDetail('probe', err, sweepStderr); } /** @@ -1032,6 +1018,11 @@ function runProbeSuite( * A column-0 `@@` is unambiguously a hunk header: every body line of a unified * diff starts with ' ', '+', '-' or '\', so a removed line whose own text begins * `@@` arrives as `-@@` and cannot be mistaken for one. + * + * Rename patches are not guarded here: `hunkProbeInputs` diffs one pathspec at + * a time, so a rename renders as a pure add (`--- /dev/null`) and + * `selectHunkProbes` skips it. A rename reaching `runOneHunkProbe` directly + * would reverse-apply the rename and leave both paths present. */ export function splitDiffIntoHunks( diffText: string, diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index f18881c6b26..03b26dc273a 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -142,6 +142,17 @@ describe('extractClaims', () => { expect(claims[0].text).toBe('471 tests passed'); }); + it('does not extract a Test Files file-count line as a test-count claim', () => { + // A pasted vitest summary nests 'Test Files 3 passed (3)' above + // 'Tests 157 passed (157)'. The file-count line must not produce a + // count claim — it would always 'differs' against the real test count. + const claims = extractClaims( + 'Test Files 3 passed (3)\n Tests 157 passed (157)', + ).filter((c) => c.kind === 'count'); + expect(claims).toHaveLength(1); + expect(claims[0].text).toContain('157'); + }); + it('emits one claim per distinct count', () => { const claims = extractClaims( 'core: 1135 passed, desktop: 41 passed', @@ -183,6 +194,24 @@ describe('extractClaims', () => { }); }); + it('excludes a cd base under .qwen/ or build output from path claims', () => { + // The cd base is a directory the Test Plan tells the reader to CREATE + // (.qwen/) or gitignored build output (dist/) — absent at the reviewed + // commit by construction, the same exclusion isPathClaim applies to tokens. + const qwen = extractClaims('`cd .qwen/tmp/review-pr-9 && npm test`'); + expect(qwen.filter((c) => c.kind === 'path')).toEqual([]); + + const dist = extractClaims('`cd dist/foo && npm test`'); + expect(dist.filter((c) => c.kind === 'path')).toEqual([]); + }); + + it('still extracts a normal cd base as a path claim', () => { + const claims = extractClaims( + '`cd packages/core && npx vitest run src/a.test.ts`', + ); + expect(claims).toContainEqual({ kind: 'path', text: 'packages/core' }); + }); + it('extracts no path from a `cd` shape it cannot resolve', () => { // Rather than guess a base and file a wrong note. expect( diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index ffb089e19ae..1b6a1557fe1 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -206,7 +206,7 @@ const COUNT_RES = [ // command exists to check went unextracted. /\b(\d+)\s+(?:tests?|specs?|assertions?)\s+(?:(?:to|should|will|would|must)\s+)?(?:pass(?:ed|ing|es)?|green|ok)\b/gi, /\btests?:?\s+(\d+)\s+pass(?:ed|ing)?\b/gi, - /\b(\d+)\s+pass(?:ed|ing)\b/gi, + /\b(? + bare.startsWith('.qwen/') || + /(?:^|\/)(?:dist|build|out|bundle|coverage|node_modules)\//.test(bare); + // A slash token is claimed as a repo path only with EVIDENCE it is one: a // file extension on its last segment, or an explicit ./ prefix. A bare // `owner/repo` is far more often a slug (`--repo QwenLM/qwen-code`), and // `origin/main` a ref — this PR's own Test Plan produced two false - // `contradicted` notes before this bar existed. The review's temp root is - // excluded outright: `.qwen/tmp/...` paths are things a Test Plan tells the - // reader to CREATE, absent at the reviewed commit by construction. + // `contradicted` notes before this bar existed. const isPathClaim = (t: string): boolean => { const bare = t.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, ''); - if (bare.startsWith('.qwen/')) return false; - // Build output is gitignored — absent at the reviewed commit by - // construction, the same category as .qwen/ above. - if (/(?:^|\/)(?:dist|build|out|bundle|coverage|node_modules)\//.test(bare)) - return false; + if (isExcludedPath(bare)) return false; return /\.\w+$/.test(bare) || t.startsWith('./'); }; @@ -295,7 +297,10 @@ export function extractClaims(section: string): Array<{ const cd = /^cd\s+([^\s&;|]+)\s*(?:&&|;)\s*(.*)$/.exec(span); if (!cd && /(^|\s)cd\s/.test(span)) continue; const base = cd?.[1] ?? ''; - if (base && PATH_RE.test(base)) push('path', base); + if (base && PATH_RE.test(base)) { + const bareBase = base.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, ''); + if (!isExcludedPath(bareBase)) push('path', base); + } // Flags that rebase relative paths (`--root ./integration-tests`) are // `cd`'s twin: a path token after one is relative to the flag's value, // not the repo root. Bail like the exotic-`cd` case — the `cd` directory From b1a38ad0b30397bf4754e290f7456048eb31df95 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 1 Aug 2026 13:41:14 +0800 Subject: [PATCH 10/12] fix(review): port the collocated-dropout test to the post-#8050 runner seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging main brought #8050's Windows-portability refactor, which resolves the probe runner through vitest/package.json's bin — a node_modules/.bin fake is dead weight it never reads. The 8215-only collocated-dropout test still installed the old .bin fake, so the REAL vitest ran its fixtures, price.test.ts genuinely passed, and the hunk scored survived. The test now overrides the fake package's vitest.mjs like every post-refactor test. --- .../commands/review/test-efficacy.integration.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 5373d1f4627..d0c8b2f2811 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -414,11 +414,14 @@ describe('test-efficacy probe isolation (#6832)', () => { ); // The baseline drops the collocated test: `price.test.ts` collects nothing // (the probe-tree import-error shape); every other file passes. - const bin = join(repo, 'node_modules', '.bin', 'vitest'); + // Override the fake PACKAGE entry — post-#8050 the probe resolves the + // runner through vitest/package.json's bin, so a node_modules/.bin file + // is dead weight it never reads. price.test.ts collects nothing; every + // other file passes. writeFileSync( - bin, + join(repo, 'node_modules', 'vitest', 'vitest.mjs'), `#!/usr/bin/env node -const path = require('path'); +import path from 'node:path'; const files = process.argv.slice(2).filter((a) => a.includes('.test.')); process.stdout.write(JSON.stringify({ testResults: files.map((f) => ({ @@ -429,7 +432,6 @@ process.stdout.write(JSON.stringify({ })); `, ); - chmodSync(bin, 0o755); await runHandler({ report: join(repo, 'report.json'), From cdff5693b79e2ddb64305fed7cc359933bcc526a Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 1 Aug 2026 14:49:12 +0800 Subject: [PATCH 11/12] fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three 8215-layer findings from the latest review, fixed at this layer (they were first patched further up the stack, where the reviewer of THIS PR cannot see them): - trimOutput's summary rescue is capped at 40 lines — uncapped, 40k lines of 'Test : …' prose voided the trim entirely (measured 1.6MB in, 1.6MB out) and the bounded-output contract is the whole point. - A '#' with no following whitespace is prose, not a heading (the ATX rule GitHub applies): '#8176', '#tag', an unfenced '#!/bin/bash' no longer end the Test Plan section mid-body; the bare-#-run crash on the closing scan is guarded. - A base-tree build lock older than 30 minutes is a corpse left by a killed builder — swept and rebuilt instead of reporting busy for the rest of the review. --- packages/cli/src/commands/review/base-tree.test.ts | 12 ++++++++++++ packages/cli/src/commands/review/base-tree.ts | 14 ++++++++++++++ .../cli/src/commands/review/build-test.test.ts | 14 ++++++++++++++ packages/cli/src/commands/review/build-test.ts | 8 +++++++- packages/cli/src/commands/review/test-plan.test.ts | 10 ++++++++++ packages/cli/src/commands/review/test-plan.ts | 7 +++++-- 6 files changed, 62 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index f424d35878c..e5870e7e0db 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -18,6 +18,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync } from 'node:child_process'; import { + utimesSync, mkdtempSync, mkdirSync, rmSync, @@ -163,6 +164,17 @@ describe('runBaseTree', () => { }); }); + it('sweeps a STALE lock instead of reporting busy for the whole review', () => { + // A builder killed without its finally leaves the lock forever; 30+ min + // old is a corpse, not a live install+build. + const lock = `${baseWorktreePath(worktree)}.lock`; + mkdirSync(lock, { recursive: true }); + const old = Date.now() / 1000 - 45 * 60; + utimesSync(lock, old, old); + const r = run(); + expect(r.available).toBe(true); // built through the corpse + }); + it('a FAILED build is a settled answer — later shards do not re-pay it', () => { const builds: string[] = []; const build = (w: string) => { diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index 55fe4f80635..a9dfd71967c 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -48,6 +48,7 @@ import { mkdirSync, readFileSync, rmSync, + statSync, writeFileSync, } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; @@ -208,6 +209,19 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // mutating. `mkdirSync` without `recursive` is the atomic test-and-set; the // loser returns busy rather than waiting out a multi-minute build. const lock = `${tree}.lock`; + // Staleness: a builder killed without its finally leaves the lock forever, + // and within the same review every later probe reports busy until cleanup. + // A lock older than any plausible install+build (30 min) is a corpse — sweep + // it and take the build. mtime is the lock dir's creation time (nothing + // touches it after mkdir), so this cannot fire on a live build. + try { + const age = Date.now() - statSync(lock).mtimeMs; + if (age > 30 * 60 * 1000) { + rmSync(lock, { recursive: true, force: true }); + } + } catch { + // No lock — the normal case. + } try { mkdirSync(lock); } catch { diff --git a/packages/cli/src/commands/review/build-test.test.ts b/packages/cli/src/commands/review/build-test.test.ts index e05c470a3df..d97bf25058a 100644 --- a/packages/cli/src/commands/review/build-test.test.ts +++ b/packages/cli/src/commands/review/build-test.test.ts @@ -476,6 +476,20 @@ describe('runBuildTest', () => { ).toContain(colored); }); + it('caps the rescue so hostile prose cannot void the trim', () => { + // 40k lines matching the summary shape made the trim a no-op (1.6MB in, + // 1.6MB out) — the rescue saves a handful of lines, never the middle. + const hostile = + 'head\n' + + Array.from({ length: 5000 }, (_, i) => `Test ${i} passed thing`).join( + '\n', + ) + + '\n' + + 'y'.repeat(9000); + const trimmed = trimOutput(hostile); + expect(trimmed.length).toBeLessThan(hostile.length / 4); + }); + it('buildOnly builds the same set but runs NO tests', () => { // For the merge-base tree an A/B probe compares against: base's suite was // green before this PR existed, so running it measures nothing about the diff --git a/packages/cli/src/commands/review/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 857dd514901..7a5f7711ef8 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -138,13 +138,19 @@ export function trimOutput(s: string): string { // find module` line lost to trimming (a long TypeScript log can push one past the // head and before the tail) would end the widening early and surface a real // graph gap as a false build error. Report stays bounded; the signal survives. + // CAPPED: the rescue exists to save a handful of summary/module-error lines, + // and an uncapped predicate made the whole trim a no-op on 40k lines of + // `Test : …` prose (measured in review — 1.6 MB in, 1.6 MB out). Past the + // cap the trim's bounded-output contract wins and the rest stays omitted. + const RESCUE_MAX = 40; const rescued = middle .split('\n') .filter( (l) => MODULE_ERROR_RE.test(l) || RUNNER_SUMMARY_RE.test(l.replace(ANSI_SGR_RE, '')), - ); + ) + .slice(0, RESCUE_MAX); const omitted = s.length - KEEP_HEAD - KEEP_TAIL; const marker = rescued.length ? `\n\n... [${omitted} characters omitted; module-resolution errors and runner summaries kept] ...\n${rescued.join('\n')}\n\n` diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 03b26dc273a..11f69ef50f2 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -106,6 +106,16 @@ describe('extractTestPlanSection', () => { expect(extractTestPlanSection(hostile)).toBeNull(); }); + it('does not end the section on a spaceless # line (ATX rule)', () => { + // `#8176`, `#tag`, an unfenced `#!/bin/bash` are prose, not headings. + const s = extractTestPlanSection( + '## Test Plan\n\nsee #8176\n#!/usr/bin/env bash\nmore\n\n## Risk\n\nx', + ); + expect(s?.content).toContain('#!/usr/bin/env bash'); + expect(s?.content).toContain('more'); + expect(s?.content).not.toContain('## Risk'); + }); + it('returns null when there is no Test Plan section', () => { expect(extractTestPlanSection('## Summary\n\njust a change')).toBeNull(); }); diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index 1b6a1557fe1..72abf1a1b79 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -118,7 +118,10 @@ const PLAN_NAME_RE = * same quadratic shape the bold pattern below was rewritten to remove, on the * same untrusted line. */ -const HEADING_LINE_RE = /^(#{1,6})(.*)$/; +// `#` must be followed by whitespace or end-of-line (the ATX rule GitHub +// applies): `#tag`, `#!/bin/bash` outside a fence, `#8176` are prose, and a +// spaceless line once ended the Test Plan section mid-body. +const HEADING_LINE_RE = /^(#{1,6})(?:[ \t](.*))?$/; /** A standalone bold line: `**Test Plan**`, the same heading in another shape. */ // No `\s*` on either side of the capture and no lazy quantifier: with all @@ -171,7 +174,7 @@ export function extractTestPlanSection( if (!fenced[j]) { const next = HEADING_LINE_RE.exec(lines[j]); // A bare `#` run with no text is not a heading (the old `\s*\S` bar). - if (next && !next[2].trim()) { + if (next && !next[2]?.trim()) { out.push(lines[j]); continue; } From 7c6b678e10dceac709867e0772bc23879b428d39 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 1 Aug 2026 15:27:32 +0800 Subject: [PATCH 12/12] fix(review): EEXIST-only lock busy, bun test alias, chained cd bail, fence backreference Four live findings from the latest inline review round (the rest of the round was already fixed upstream by the takeover bot - verified by probing head behavior rather than re-reading the threads): - base-tree's lock catch distinguishes EEXIST (a concurrent builder, busy) from EPERM/EROFS/ENOSPC (this run's own failure, reported as such, not as a busy that will never clear). - "bun test" is bun's built-in runner, not a package-script alias: it runs whether or not any manifest defines test, so ruling it against the scripts table filed a false contradicted. - A chained cd matches the leading-cd shape but the single-hop resolver joined file tokens against the FIRST directory; it now bails like the exotic-cd case. - codeSpans' fence regex closes on its own marker via backreference; a tilde fence line inside a backtick block ended the span early and lines after it were lost to extraction. --- packages/cli/src/commands/review/base-tree.ts | 10 +++++++- .../cli/src/commands/review/test-plan.test.ts | 23 +++++++++++++++++++ packages/cli/src/commands/review/test-plan.ts | 18 +++++++++++---- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index a9dfd71967c..cc85fa59ca4 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -224,7 +224,15 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { } try { mkdirSync(lock); - } catch { + } catch (err) { + // Only EEXIST means "another builder holds it". EPERM/EROFS/ENOSPC is a + // real failure this run owns — reporting it as busy sends the caller into + // a retry loop against a lock that will never appear. + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') { + return unavailable( + `could not take the base-tree build lock: ${(err as Error).message}`, + ); + } return unavailable( 'another probe is building the base tree right now — retry when its ' + 'marker appears (the fast path will then reuse it), or settle the ' + diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 11f69ef50f2..7e86f9f3004 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -398,6 +398,29 @@ describe('npmScriptOf', () => { expect(npmScriptOf('npm run test:unit')).toBe('test:unit'); }); + it("never rules bun test against the scripts table — it is bun's built-in runner", () => { + expect(npmScriptOf('bun test')).toBeNull(); + expect(npmScriptOf('bun run lint')).toBe('lint'); // the run form still rules + }); + + it('bails on a CHAINED cd instead of joining against the first hop', () => { + // `cd a && cd b && vitest run x.test.ts` once produced `a/x.test.ts`. + expect( + extractClaims('`cd a && cd packages/b && npx vitest run src/x.test.ts`'), + ).toEqual([]); + }); + + it('closes a fence only on its own marker inside codeSpans', () => { + // A ~~~ line inside a ``` block ended the span early — lines after it + // fell OUT of the fence and were lost to span extraction entirely. + const claims = extractClaims( + '```bash\nnpx vitest run src/real.test.ts\n~~~\nnpx vitest run src/inside.test.ts\n```', + ); + expect(claims).toContainEqual({ kind: 'path', text: 'src/real.test.ts' }); + // Still inside the ``` block, so still extracted: + expect(claims).toContainEqual({ kind: 'path', text: 'src/inside.test.ts' }); + }); + it('is null for every npm builtin outside the run form and script aliases', () => { // The denylist knew four verbs; npm has ~fifty. Each of the rest became a // false `no package defines this script` on a correct Test Plan. diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index 72abf1a1b79..39ba638d637 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -225,9 +225,11 @@ function codeSpans(section: string): string[] { .trim(); if (t) spans.push(t); }; - const fence = /(?:```|~~~)[^\n]*\n([\s\S]*?)(?:```|~~~)/g; + // Backreference: a ``` fence closes only on ``` and ~~~ only on ~~~ — the + // alternation form let a ~~~ line inside a ``` block end the span early. + const fence = /(```|~~~)[^\n]*\n([\s\S]*?)\1/g; let m: RegExpExecArray | null; - while ((m = fence.exec(section))) m[1].split('\n').forEach(add); + while ((m = fence.exec(section))) m[2].split('\n').forEach(add); const inline = /`([^`\n]+)`/g; const outsideFences = section.replace(fence, ' '); @@ -299,6 +301,10 @@ export function extractClaims(section: string): Array<{ // misread. const cd = /^cd\s+([^\s&;|]+)\s*(?:&&|;)\s*(.*)$/.exec(span); if (!cd && /(^|\s)cd\s/.test(span)) continue; + // A CHAINED cd (`cd a && cd b && …`) matches the leading-cd shape but the + // single-hop resolver would join file tokens against the FIRST directory + // only — a wrong base is worse than none. Bail like the exotic case. + if (cd && /(^|\s)cd\s/.test(cd[2])) continue; const base = cd?.[1] ?? ''; if (base && PATH_RE.test(base)) { const bareBase = base.replace(/:\d+(?::\d+)?$/, '').replace(/\/$/, ''); @@ -444,8 +450,12 @@ export function npmScriptOf(command: string): string | null { // ("`npm run test:unit` was renamed") lives entirely in the allowed forms. const m = /^(?:npm|pnpm|yarn|bun)\s+run\s+([\w:.-]+)/.exec(command); if (m && !m[1].startsWith('-')) return m[1]; - const alias = - /^(?:npm|pnpm|yarn|bun)\s+(test|start|stop|restart)(?=\s|$)/.exec(command); + // `bun test` is bun's own built-in runner, not a package-script alias — it + // runs whether or not any manifest defines `test`, so ruling it against the + // scripts table filed a false contradicted. + const alias = /^(?:npm|pnpm|yarn)\s+(test|start|stop|restart)(?=\s|$)/.exec( + command, + ); return alias ? alias[1] : null; }