diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index acf84350c5..d533063fe4 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -50,6 +50,7 @@ describe('reviewCommand', () => { 'agent-prompt', 'build-test', 'base-tree', + 'test-delta', 'script-lint', 'resolve-anchors', 'check-coverage', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 18cea24c1f..f3311ca32d 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -23,6 +23,7 @@ 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 { testDeltaCommand } from './review/test-delta.js'; import { scriptLintCommand } from './review/script-lint.js'; import { submitCommand } from './review/submit.js'; import { testEfficacyCommand } from './review/test-efficacy.js'; @@ -47,6 +48,7 @@ export const reviewCommand: CommandModule = { .command(agentPromptCommand) .command(buildTestCommand) .command(baseTreeCommand) + .command(testDeltaCommand) .command(scriptLintCommand) .command(resolveAnchorsCommand) .command(checkCoverageCommand) @@ -58,7 +60,7 @@ export const reviewCommand: CommandModule = { .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, base-tree, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, test-plan, 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, test-delta, 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/build-test.ts b/packages/cli/src/commands/review/build-test.ts index 7a5f7711ef..c32da3f02c 100644 --- a/packages/cli/src/commands/review/build-test.ts +++ b/packages/cli/src/commands/review/build-test.ts @@ -109,6 +109,24 @@ export interface BuildTestReport { const KEEP_HEAD = 2_000; const KEEP_TAIL = 6_000; +/** + * Did this spawn die on its deadline? + * + * Exported so `test-delta`'s rerun asks the SAME question rather than + * re-deriving it — a copy there used `error.message.includes('ETIMEDOUT')`, + * which misses an external SIGTERM and fed a silent "base is green". + */ +export function spawnTimedOut(r: { + error?: Error; + signal?: NodeJS.Signals | null; + status?: number | null; +}): boolean { + return ( + (r.error as NodeJS.ErrnoException | undefined)?.code === 'ETIMEDOUT' || + (r.signal === 'SIGTERM' && r.status === null) + ); +} + /** The module-resolution errors the widening loop reads to grow the build set. */ const MODULE_ERROR_RE = /Cannot find module '[^']+'|Could not resolve "[^"]+"/; @@ -232,9 +250,7 @@ function run(command: string, cwd: string, timeoutMs: number): CommandResult { // the authoritative signal. The `SIGTERM`/null-status pair is only a fallback: it // also matches an external SIGTERM (a container stop), and it misses a non-default // `killSignal`. Check the authoritative one first. - const timedOut = - (r.error as NodeJS.ErrnoException | undefined)?.code === 'ETIMEDOUT' || - (r.signal === 'SIGTERM' && r.status === null); + const timedOut = spawnTimedOut(r); return { command, exitCode: r.status, diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index fd8556b8b0..0597e8ffc1 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -256,7 +256,8 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th - **Sibling consistency — a guard one path has and its twin lacks.** When one member of a family of parallel paths carries a validation, guard, cleanup, or shape-check — sibling loaders, the handlers of a route table, the pair of functions that build the same command — check that **every** sibling carries it too. A lone exception is usually accidental, and the missing half is a latent **asymmetric failure**: harmless until the one input that path sees. Name the divergent sibling and the guard it is missing. When the missing guard is a validation on **untrusted input** (one \`gitCheckout\` validates its ref, its sibling does not), that is not a style nit — hand it to the security pass as a likely bug, not a consistency note. - Over-engineering and unnecessary abstraction - **Altitude** — is each change implemented at the right depth, or is it a fragile bandaid? A special case layered onto shared infrastructure to make one caller work is a sign the fix is not deep enough: prefer generalizing the underlying mechanism. The mirror image — a new abstraction serving a single call site — is over-engineering. **Name the depth the change should live at.** -- Missing or misleading comments; dead code`, +- Missing or misleading comments; dead code +- **Documentation parity with siblings.** When the diff adds a user-facing surface — a CLI flag, a slash-command option, a settings key — check whether its SIBLINGS are documented, and where. Three of four sibling selectors having a docs entry is a house convention the fourth just broke; a surface whose behaviour can silently change (a fallback, an automatic swap, an emitted warning) undocumented is a user staring at a message with nowhere to look it up. This is deliberately a **parity** check, not a docs mandate: the finding names the sibling precedent and the file it lives in (\`--fast\` and \`--vision\` are in \`docs/…/commands.md\`; the new \`--compaction\` is not), so "add docs" arrives as the codebase's own standard, not this reviewer's. No documented sibling, no finding. Severity: Suggestion.`, }, '4': { @@ -383,6 +384,7 @@ You are undirected on purpose. Do not restrict yourself to the list.`, 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. +- **When any \`test[]\` command failed (exit non-zero, not a timeout), MEASURE which failures are the PR's before ruling by path.** The path rule above misclassifies in both directions — an environment-flaky test in a touched file gets filed as a Critical it did not cause, and a PR that breaks a test in an UNTOUCHED file gets waved through as pre-existing. The measurement is two commands: \`qwen review base-tree --plan --worktree --out /qwen-review-pr--base-tree.json\` (builds the merge base beside the worktree). **Read \`available\` before using \`path\`** — a tree that was created but did NOT build populates \`path\` too, and a base that failed to build says nothing whatsoever about the PR, so measuring against it turns an infrastructure failure into a list of Criticals. \`available: false\` (local/lightweight review, no merge base, a base that would not compile) means the path rule stands — say so and stop here, and \`qwen review test-delta --report --baseline --pr-worktree --out /qwen-review-pr--test-delta.json\`. Read its verdict: a file in \`netNew\` fails on the PR side only — **that is the Critical**, whatever file the diff touches; a file in \`shared\` fails on base too — **pre-existing by measurement**, never filed, whatever file the diff touches; an \`unparsed\` entry, a timed-out base rerun, a base rerun that FAILED without naming any failing file (it did not measure the base — an unbuilt tree, a missing install, a workspace absent at base), or a command the whole-command budget could not fit attributes nothing — the report names each with its own reason; fall back to the path rule for those and say the delta could not rule. Compare failing FILE SETS, never counts: a flaky suite fails different test NAMES on two runs of the same tree, so counts are noise and the set difference is the signal. - \`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 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. diff --git a/packages/cli/src/commands/review/test-delta.test.ts b/packages/cli/src/commands/review/test-delta.test.ts new file mode 100644 index 0000000000..462383aadc --- /dev/null +++ b/packages/cli/src/commands/review/test-delta.test.ts @@ -0,0 +1,467 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The parser and the attribution are the two halves that matter: a wrong +// failing-file parse invents or drops evidence, and a wrong delta turns a +// pre-existing flake into a public Critical (or the reverse). The base rerun +// itself is a seam — one command in one cwd — so the exec is injected. + +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 { + testDeltaCommand, + type TestDeltaArgs, + failingFilesOf, + runTestDelta, + type TestDeltaReport, +} from './test-delta.js'; +import type { BuildTestReport, CommandResult } from './build-test.js'; + +const cmd = (over: Partial): CommandResult => ({ + command: 'npm test --workspace="packages/core"', + exitCode: 1, + seconds: 10, + timedOut: false, + output: '', + ...over, +}); + +describe('failingFilesOf', () => { + it('reads FAIL lines (vitest and jest shape) once per file', () => { + const out = [ + ' FAIL src/a.test.ts > suite > first case', + ' FAIL src/a.test.ts > suite > second case', + 'FAIL src/b.spec.tsx', + ].join('\n'); + expect(failingFilesOf(out)).toEqual(['src/a.test.ts', 'src/b.spec.tsx']); + }); + + it('reads a vitest ❯ progress line only when it says failed', () => { + const out = [ + ' ❯ src/red.test.ts (12 tests | 3 failed) 220ms', + ' ❯ src/green.test.ts (12 tests) 90ms', + ].join('\n'); + expect(failingFilesOf(out)).toEqual(['src/red.test.ts']); + }); + + it('sees through ANSI color codes', () => { + const out = '\x1b[31m FAIL \x1b[39m src/x.test.ts > case'; + expect(failingFilesOf(out)).toEqual(['src/x.test.ts']); + }); + + it('keeps the workspace project token IN the identity', () => { + // Dropping it collapsed same-named files across workspaces, so a PR-caused + // failure in one package could read as pre-existing because another package + // has a file by the same name. + const out = ' FAIL |@qwen-code/qwen-code| src/commands/x.test.ts > case'; + expect(failingFilesOf(out)).toEqual([ + '@qwen-code/qwen-code::src/commands/x.test.ts', + ]); + }); + + it("normalises paths against each run's own root before comparing", () => { + // The two sides run in DIFFERENT roots; comparing absolute paths verbatim + // made every pre-existing failure a fabricated netNew. + expect( + failingFilesOf(' FAIL /wt/pr/src/a.test.ts > flaky', '/wt/pr'), + ).toEqual(['src/a.test.ts']); + expect( + failingFilesOf(' FAIL /wt/base/src/a.test.ts > flaky', '/wt/base'), + ).toEqual(['src/a.test.ts']); + }); + + it('reads a Windows path shape', () => { + // A missed parse is an unattributed failure, not a loud error. + // Backslashes normalise to `/` so a Windows path compares with its + // POSIX-printed twin on the other side. + expect(failingFilesOf(' FAIL C:\\repo\\src\\x.test.ts > case')).toEqual([ + 'C:/repo/src/x.test.ts', + ]); + }); + + it('names no file from output with no failure lines', () => { + expect(failingFilesOf('Tests 12 passed (12)')).toEqual([]); + }); +}); + +describe('runTestDelta', () => { + let dir: string; + let baseline: string; + + const writeReport = (test: CommandResult[]): string => { + const p = join(dir, 'bt.json'); + writeFileSync(p, JSON.stringify({ test } as Partial)); + return p; + }; + + const runWith = ( + test: CommandResult[], + baseOutput: string | ((command: string, cwd: string) => CommandResult), + ): TestDeltaReport => + runTestDelta({ + report: writeReport(test), + baseline, + timeout: 60, + exec: + typeof baseOutput === 'function' + ? // Pass cwd through: swallowing it made the baseline-dir assertion + // impossible to write, which is why the test that promised it + // never made it. + (command, cwd) => baseOutput(command, cwd) + : (command) => cmd({ command, output: baseOutput }), + }); + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'qwen-test-delta-')); + baseline = join(dir, 'base'); + mkdirSync(baseline); + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it('attributes a PR-only failure as netNew and a both-sides failure as shared', () => { + const r = runWith( + [ + cmd({ + output: + ' FAIL src/new.test.ts > broken by pr\n FAIL src/flaky.test.ts > env', + }), + ], + ' FAIL src/flaky.test.ts > env', + ); + expect(r.netNew).toEqual(['src/new.test.ts']); + expect(r.shared).toEqual(['src/flaky.test.ts']); + expect(r.note).toContain('do NOT fail on base'); + expect(r.note).toContain('pre-existing'); + }); + + it('reruns ONLY the failed commands, in the baseline dir', () => { + const calls: string[] = []; + const cwds: string[] = []; + runWith( + [ + cmd({ command: 'npm test --workspace="a"', exitCode: 0 }), + cmd({ + command: 'npm test --workspace="b"', + output: 'FAIL src/x.test.ts', + }), + cmd({ + command: 'npm test --workspace="c"', + timedOut: true, + exitCode: null, + }), + ], + (command, cwd) => { + calls.push(command); + cwds.push(cwd); + return cmd({ command, output: '' }); + }, + ); + // Green suites have nothing to attribute; a timeout is infrastructure. + expect(calls).toEqual(['npm test --workspace="b"']); + // …and the rerun happens in the BASE tree, which the name promises and + // nothing asserted: running it in the PR worktree would compare a tree + // with itself and report every failure pre-existing. + expect(cwds).toEqual([baseline]); + }); + + it('an empty netNew with everything shared is the pre-existing verdict', () => { + // The live-run case this exists for: 3 env-sensitive failures that the + // model previously had to JUDGE as pre-existing become a measurement. + const failing = + ' FAIL src/extensionManager.test.ts > a\n FAIL src/session-writer-lease.test.ts > b'; + const r = runWith([cmd({ output: failing })], failing); + expect(r.netNew).toEqual([]); + expect(r.shared).toEqual([ + 'src/extensionManager.test.ts', + 'src/session-writer-lease.test.ts', + ]); + }); + + it('discloses a failed command whose failing files could not be parsed', () => { + const r = runWith( + [cmd({ output: 'npm error code 1 — no FAIL lines here' })], + '', + ); + expect(r.entries[0].unparsed).toBe(true); + expect(r.netNew).toEqual([]); + expect(r.note).toContain('no parseable failing file'); + expect(r.note).toContain('judge them by the diff as before'); + }); + + it('does NOT read a base that failed to RUN as "nothing fails on base"', () => { + // An unbuilt base tree, a missing install, a workspace the PR added: each + // exits non-zero with zero FAIL lines. Reading that as green manufactures + // the strongest evidence this command emits from a base that never ran. + const r = runWith([cmd({ output: ' FAIL src/a.test.ts > case' })], () => + cmd({ + exitCode: 1, + output: + 'Error: Cannot find module vitest/dist/cli.js\nnpm error code 1', + }), + ); + expect(r.netNew).toEqual([]); + expect(r.entries[0].shared).toEqual([]); + expect(r.note).toContain('did not measure the base'); + }); + + it('reads an external SIGTERM kill as a timeout, like build-test does', () => { + // The substring form missed it: no ETIMEDOUT message, no exit code — so + // timedOut:false with empty output, feeding the base-green path above. + const r = runWith([cmd({ output: ' FAIL src/a.test.ts > case' })], () => + cmd({ timedOut: true, exitCode: null, output: '' }), + ); + expect(r.netNew).toEqual([]); + }); + + it('treats a timed-out base rerun as infrastructure, not as "nothing fails on base"', () => { + const r = runWith([cmd({ output: 'FAIL src/x.test.ts' })], () => + cmd({ timedOut: true, exitCode: null, output: '' }), + ); + // The PR-side failure is NOT promoted to netNew off a run that never + // finished — an unknowable base failing set attributes nothing. (First + // written asserting only the note, this test passed over an implementation + // that promoted everything; the two lines below are the actual claim.) + expect(r.entries[0].base.timedOut).toBe(true); + expect(r.netNew).toEqual([]); + expect(r.entries[0].shared).toEqual([]); + expect(r.note).toContain('timed out'); + }); + + it('has nothing to do when every PR-side test command passed', () => { + const r = runWith([cmd({ exitCode: 0 })], ''); + expect(r.entries).toEqual([]); + expect(r.note).toContain('nothing to attribute'); + }); + + it('stops at the whole-command budget and discloses what it skipped', () => { + // --timeout is PER COMMAND: three failures at the 300s default is 900s + // against a 600s tool ceiling, killed with NO report at all. + const real = Date.now; + let t = 0; + Date.now = () => (t += 300_000); + try { + const r = runWith( + [ + cmd({ + command: 'npm test --workspace="a"', + output: 'FAIL a/x.test.ts', + }), + cmd({ + command: 'npm test --workspace="b"', + output: 'FAIL b/y.test.ts', + }), + ], + ' FAIL a/x.test.ts', + ); + expect(r.entries).toHaveLength(1); + expect(r.note).toContain('budget was exhausted'); + expect(r.note).toContain('npm test --workspace="b"'); + } finally { + Date.now = real; + } + }); + + it('never hands a command outside the emitter grammar to a shell', () => { + // The report is a file this reads and then executes from with shell:true. + // A workspace token is a DIRECTORY, and a directory is a name a pull + // request chooses — so the grammar, not the reader, is the boundary. + const ran: string[] = []; + const r = runWith( + [ + cmd({ + command: 'npm test --workspace="packages/x";touch /tmp/pwned;"', + output: 'FAIL src/a.test.ts', + }), + ], + (command) => { + ran.push(command); + return cmd({ command, output: '' }); + }, + ); + expect(ran).toEqual([]); + expect(r.entries).toEqual([]); + expect(r.netNew).toEqual([]); + expect(r.note).toContain('not the shape'); + expect(r.note).toContain('judge them by the diff'); + }); + + it('reruns both shapes build-test actually emits', () => { + const ran: string[] = []; + runWith( + [ + cmd({ command: 'npm test', output: 'FAIL src/a.test.ts' }), + cmd({ + command: 'npm test --workspace="packages/core"', + output: 'FAIL src/b.test.ts', + }), + ], + (command) => { + ran.push(command); + return cmd({ command, output: '' }); + }, + ); + expect(ran).toEqual(['npm test', 'npm test --workspace="packages/core"']); + }); + + it('prefers the failing set the run measured over re-parsing trimmed output', () => { + // The base rerun parses its own raw text; `output` is only the bounded copy + // that lands in the report. Re-parsing it would lose whatever the trim's + // omitted middle swallowed, and a SHORT base set fabricates netNew. + const r = runWith( + [cmd({ output: 'FAIL src/a.test.ts\nFAIL src/b.test.ts' })], + () => ({ + ...cmd({}), + output: 'FAIL src/a.test.ts\n\n... [90000 characters omitted] ...\n', + failingFiles: ['src/a.test.ts', 'src/b.test.ts'], + }), + ); + expect(r.netNew).toEqual([]); + expect(r.shared).toEqual(['src/a.test.ts', 'src/b.test.ts']); + }); + + it('discloses a PR-side output that was already trimmed', () => { + // build-test trimmed it before this command ran, so the PR failing set may + // be short. That understates `shared`; it cannot invent a netNew. Say so. + const r = runWith( + [ + cmd({ + output: + 'FAIL src/a.test.ts\n\n... [120000 characters omitted] ...\nFAIL src/z.test.ts', + }), + ], + 'FAIL src/a.test.ts', + ); + expect(r.note).toContain('may be partial'); + expect(r.entries[0].prTruncated).toBe(true); + }); + + it('says nothing about trimming when the PR output was complete', () => { + const r = runWith( + [cmd({ output: 'FAIL src/a.test.ts' })], + 'FAIL src/a.test.ts', + ); + expect(r.note).not.toContain('may be partial'); + expect(r.entries[0].prTruncated).toBe(false); + }); + + it('says when a rerun died on a BUDGET-shortened deadline, not its own', () => { + // Otherwise "timed out — infrastructure" sends the reader hunting a hang + // that is really an exhausted budget, and a rerun with room to spare would + // have measured it. + const real = Date.now; + let t = 0; + // `runWith` passes timeout: 60, so `remaining` must fall below 60s while + // staying above the 5s hard skip. Each call advances 490s: startedAt lands + // at 490s, the first iteration sees remaining = 540 - 490 = 50s (clamped), + // and the second falls past the skip threshold. + Date.now = () => { + t += 490_000; + return t; + }; + try { + const r = runWith( + [ + cmd({ + command: 'npm test --workspace="a"', + output: 'FAIL a/x.test.ts', + }), + cmd({ + command: 'npm test --workspace="b"', + output: 'FAIL b/y.test.ts', + }), + ], + (command) => + cmd({ command, timedOut: true, exitCode: null, output: '' }), + ); + expect(r.note).toContain('the whole-command budget shortened'); + } finally { + Date.now = real; + } + }); + + it('refuses an unreadable report and a missing base tree without throwing', () => { + expect( + runTestDelta({ report: join(dir, 'nope.json'), baseline, timeout: 60 }) + .note, + ).toMatch(/cannot read/); + expect( + runTestDelta({ + report: writeReport([cmd({ output: 'FAIL src/x.test.ts' })]), + baseline: join(dir, 'no-such-base'), + timeout: 60, + }).note, + ).toMatch(/base-tree/); + }); +}); + +describe('the CLI option contract', () => { + // Every test above builds its args by hand. That is how the flag-name bug + // got into `test-plan`: yargs camel-cases `--build-test` to `buildTest`, a + // field named for the flag read `undefined` on every real invocation, and + // the suite stayed green because nothing went through yargs. + // + // `--pr-worktree` has the worst failure mode of any flag here: arriving + // undefined, root stripping silently stops and EVERY pre-existing failure + // becomes a fabricated netNew. So this test does not assert the parsed + // shape and stop — it feeds the parsed object straight into runTestDelta + // and asserts on an attribution only reachable when the root was stripped. + let dir: string; + let baseDir: string; + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'qwen-test-delta-cli-')); + baseDir = join(dir, 'base'); + mkdirSync(baseDir); + writeFileSync( + join(dir, 'bt.json'), + JSON.stringify({ + test: [ + { + command: 'npm test', + exitCode: 1, + seconds: 1, + timedOut: false, + // Absolute, under the PR worktree root. + output: ' FAIL /wt/pr/src/flaky.test.ts > env', + }, + ], + }), + ); + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it('parses --pr-worktree into the field runTestDelta actually reads', () => { + const parsed = (testDeltaCommand.builder as (y: Argv) => Argv)( + yargs([]), + ).parseSync([ + '--report', + join(dir, 'bt.json'), + '--baseline', + baseDir, + '--pr-worktree', + '/wt/pr', + ]) as unknown as TestDeltaArgs; + + const report = runTestDelta({ + ...parsed, + // The base side prints the SAME failure under its own root. + exec: (command) => ({ + command, + exitCode: 1, + seconds: 1, + timedOut: false, + output: ` FAIL ${baseDir}/src/flaky.test.ts > env`, + }), + }); + + // Reachable ONLY if both roots were stripped: unstripped, the two absolute + // paths differ and the pre-existing failure is reported as the PR's own. + expect(report.netNew).toEqual([]); + expect(report.shared).toEqual(['src/flaky.test.ts']); + }); +}); diff --git a/packages/cli/src/commands/review/test-delta.ts b/packages/cli/src/commands/review/test-delta.ts new file mode 100644 index 0000000000..d984db390b --- /dev/null +++ b/packages/cli/src/commands/review/test-delta.ts @@ -0,0 +1,453 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review test-delta`: rerun the PR side's FAILED test commands on the +// base tree, and report the failing-file sets' difference — so "pre-existing" +// becomes a measurement instead of a judgment. +// +// Agent 7's brief has always said: correlate each failure with the diff — a +// failure in a file the PR changed is a Critical, one in a file it did not +// touch is pre-existing. That is a judgment by PATH, and it is the weakest kind +// of evidence this pipeline still leans on. It misclassifies in both +// directions: an environment-sensitive test fails in a file the PR happens to +// touch (filed as a Critical it did not cause), and a PR breaks a test in a +// file it never touched (waved through as pre-existing — the exact failure +// shape `base-tree` exists to catch). +// +// With a built base tree the question is decidable: run the SAME command there. +// A failure that reproduces on base predates the PR, whatever file it lives in. +// A failure only the PR side shows is the PR's — same caveat. +// +// Two disciplines, both measured on live maintainer verification runs: +// +// - **Compare failing FILE SETS, not counts.** A flaky suite fails different +// TESTS on two runs of the same tree (observed live: the same branch's +// AuthDialog failures changed names between runs), so absolute counts are +// noise. The failing-file set is stable enough to diff, and an EMPTY +// net-new set is the strongest "pre-existing" statement available. +// - **Only failed commands are rerun.** A green PR-side suite has nothing to +// attribute, and base's suite was green before the PR existed — running it +// would measure nothing about the diff. The base run costs exactly one run +// per PR-side failure. +// +// The PR side's failing files are parsed from the build-test report's already +// captured output, not from a rerun — the report is the record of what +// actually failed, and `trimOutput` keeps the failure section (the tail) plus +// rescued summary lines. A file this cannot parse is disclosed, never guessed. + +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 { + buildRunEnv, + spawnTimedOut, + trimOutput, + type BuildTestReport, + type CommandResult, +} from './build-test.js'; + +// eslint-disable-next-line no-control-regex -- ESC is the character under test +const ANSI_SGR_RE = /\x1b\[[0-9;]*m/g; + +/** + * The exact shapes `build-test` emits for a test command — and the only ones + * this command will hand to a shell. + * + * The report is a FILE this reads and then executes from, with `shell: true`, + * in the base worktree. Nothing else in the pipeline re-executes a string it + * read back off disk, so nothing else has to care where that string came from; + * this does. The workspace token is a directory, and a directory is a name a + * pull request can choose: `packages/x";curl …|sh;"` is a legal path in git + * and on Linux, and it round-trips through the report into a shell. + * + * Restricting to the emitter's own grammar costs nothing real — `build-test` + * produces `npm test` and `npm test --workspace=""`, both matched here — + * and anything outside it is skipped and disclosed rather than run, which is + * the same treatment every other thing this command cannot do gets. + */ +const RERUNNABLE_COMMAND_RE = /^npm test(?: --workspace="[\w@./-]+")?$/; + +/** `trimOutput`'s own marker — the one signal that a stored output is partial. */ +const TRIM_MARKER_RE = /\.\.\. \[\d+ characters omitted/; + +/** + * Test files a runner named as failing, out of one command's output. + * + * Two shapes cover vitest and jest, the runners build-test drives: + * `FAIL src/x.test.ts > name` (both, in the failure section) and vitest's + * per-file `❯ src/x.test.ts (12 tests | 3 failed)` progress line. Matching is + * on the path token, so a `FAIL` line whose path was truncated mid-token by + * output trimming simply does not match — an unparsed failure surfaces as a + * count mismatch in the caller's disclosure, never as an invented path. + */ +export function failingFilesOf(output: string, root = ''): string[] { + const text = output.replace(ANSI_SGR_RE, ''); + const files = new Set(); + const re = + // `\\` and `:` in the path class: a Windows runner prints + // `FAIL C:\\repo\\src\\x.test.ts`, which the POSIX-only class missed — + // and a missed parse is an unattributed failure, not a loud error. + /(?:^|\s)(?:FAIL\s+|❯\s+)(?:\|([^|]+)\|\s+)?([\w@.:\\/-]+\.(?:test|spec)\.[cm]?[jt]sx?)\b([^\n]*)/gm; + let m: RegExpExecArray | null; + while ((m = re.exec(text))) { + // The `❯` progress line lists every file; only a failing one counts. + if (m[0].trimStart().startsWith('❯') && !/failed/.test(m[3] ?? '')) + continue; + // ROOT-RELATIVE, and keyed by project. The two sides run in DIFFERENT + // roots (the PR worktree and the base tree), so comparing absolute paths + // verbatim made every pre-existing failure a fabricated netNew. The vitest + // project token is part of the identity too: dropping it collapsed + // same-named files across workspaces, suppressing a real Critical as a + // "measurement" — the worse of the two failure directions. + files.add(`${m[1] ? `${m[1].trim()}::` : ''}${relativeToRoot(m[2], root)}`); + } + return [...files].sort(); +} + +/** Strip the run's own root (and any leading `./`) so the two sides compare. */ +export function relativeToRoot(file: string, root: string): string { + const norm = (v: string) => v.replace(/\\/g, '/').replace(/\/+$/, ''); + const f = norm(file); + const r = root ? norm(root) : ''; + const rel = r && f.startsWith(`${r}/`) ? f.slice(r.length + 1) : f; + return rel.replace(/^\.\//, ''); +} + +/** One rerun: the same command, in the base tree. */ +export interface DeltaEntry { + command: string; + /** Failing test files parsed from the PR-side report's captured output. */ + prFailingFiles: string[]; + /** Failing test files from the base-side rerun. */ + baseFailingFiles: string[]; + /** Failing on the PR side only — the PR's own, by measurement. */ + netNew: string[]; + /** Failing on BOTH sides — pre-existing, whatever file the diff touches. */ + shared: string[]; + base: CommandResult; + /** + * True when the PR side named no parseable failing file although the + * command failed — the delta for this command proves nothing, and the + * path-based judgment stays in force. Disclosed, never silently dropped. + */ + unparsed: boolean; + /** + * True when the PR-side output this read was already trimmed by `build-test`. + * The failing-file list may be short, which can only understate `shared`. + */ + prTruncated: boolean; +} + +export interface TestDeltaReport { + entries: DeltaEntry[]; + /** Union across entries, deduplicated. */ + netNew: string[]; + shared: string[]; + note: string; +} + +export interface TestDeltaArgs { + report: string; + baseline: string; + out?: string; + /** + * The PR worktree the report's failures were produced in — its root is + * stripped so both sides compare as repo-relative paths. Named for yargs' + * camel-cased `--pr-worktree`; a field named for the flag itself would read + * `undefined` on every real invocation. + */ + prWorktree?: string; + timeout: number; + /** Test seam — production spawns the real command. */ + exec?: (command: string, cwd: string, timeoutMs: number) => BaseRunResult; +} + +/** + * A base-side rerun, plus what it measured BEFORE its output was bounded. + * + * `output` is trimmed for the report, and `trimOutput` rescues only module + * errors and runner summaries out of the omitted middle — not the per-file + * `FAIL` lines this command parses. A base suite with a failure section over + * the tail budget would therefore lose failing files into the gap, and a + * SHORTER base set is the dangerous direction: `netNew` is the PR side minus + * the base side, so every file trimming hid becomes a fabricated Critical + * attributed to this PR by "measurement". Parse the raw text, report the + * bounded one. + */ +export interface BaseRunResult extends CommandResult { + /** Parsed from the untrimmed output. Absent from a seam that predates this. */ + failingFiles?: string[]; +} + +function run(command: string, cwd: string, timeoutMs: number): BaseRunResult { + const started = Date.now(); + const r = spawnSync(command, { + shell: true, + cwd, + encoding: 'utf8', + timeout: timeoutMs, + env: buildRunEnv(process.env), + maxBuffer: 64 * 1024 * 1024, + // build-test's, deliberately: "a build that asks a question is a build that + // hangs until the deadline" — and this reruns those same commands. + stdio: ['ignore', 'pipe', 'pipe'], + }); + // The sibling's predicate, not a weaker re-derivation: an external SIGTERM + // (container stop, cancelled CI job) sets neither an ETIMEDOUT message nor + // an exit code, so the substring form reported timedOut:false with empty + // output and fed straight into the base-green path. + const timedOut = spawnTimedOut(r); + const raw = `${r.stdout ?? ''}${r.stderr ?? ''}`; + return { + command, + exitCode: timedOut ? null : (r.status ?? null), + seconds: Math.round((Date.now() - started) / 1000), + timedOut, + failingFiles: timedOut ? [] : failingFilesOf(raw, cwd), + // Bounded like build-test's: this lands in `entries[].base.output`, which + // is JSON.stringify'd to --out, and the verdict fields sit AFTER it — an + // untrimmed megabyte pushes exactly what the command produces past any + // reader's truncation. + output: trimOutput(raw), + }; +} + +/** + * Whole-command budget, mirroring test-efficacy's. `--timeout` is PER COMMAND, + * so three failed commands at the 300s default is 900s against the 600s tool + * ceiling — killed with NO report written, discarding the base-tree install and + * build just paid for. Commands the budget cannot fit are disclosed, never + * silently dropped. + */ +const TOTAL_BUDGET_MS = 540_000; + +/** The CLI default, reused when a programmatic caller omits `--timeout`. */ +const DEFAULT_TIMEOUT_S = 300; + +export function runTestDelta(args: TestDeltaArgs): TestDeltaReport { + const exec = args.exec ?? run; + const baseline = resolve(args.baseline); + const empty = (note: string): TestDeltaReport => ({ + entries: [], + netNew: [], + shared: [], + note, + }); + + let report: BuildTestReport; + try { + report = JSON.parse(readFileSync(args.report, 'utf8')) as BuildTestReport; + } catch (err) { + return empty( + `cannot read the build-test report ${args.report}: ${(err as Error).message}`, + ); + } + if (!existsSync(baseline)) { + return empty( + `the base tree ${baseline} does not exist — run \`qwen review base-tree\` first`, + ); + } + + // Failed for real: a timeout is an infrastructure result and reruns as one. + const failed = (report.test ?? []).filter( + (t) => !t.timedOut && t.exitCode !== 0, + ); + if (failed.length === 0) { + return empty( + 'no PR-side test command failed — there is nothing to attribute, and the base run would measure nothing', + ); + } + + // A programmatic caller may omit `timeout`; `NaN * 1000` reaches spawnSync as + // an invalid deadline. Fall back to the CLI's own default. + const perCommandMs = + (Number.isFinite(args.timeout) ? args.timeout : DEFAULT_TIMEOUT_S) * 1000; + const startedAt = Date.now(); + const skippedForBudget: string[] = []; + /** Reruns killed by a deadline the BUDGET shortened, not by their own. */ + const budgetClamped: string[] = []; + const entries: DeltaEntry[] = []; + /** Commands that did not match the emitter's grammar, so were never run. */ + const skippedUnrecognised: string[] = []; + for (const t of failed) { + if (!RERUNNABLE_COMMAND_RE.test(t.command)) { + skippedUnrecognised.push(t.command); + continue; + } + const remaining = TOTAL_BUDGET_MS - (Date.now() - startedAt); + if (remaining < 5_000) { + skippedForBudget.push(t.command); + continue; + } + const prFailingFiles = failingFilesOf( + t.output ?? '', + args.prWorktree ?? '', + ); + // A clamped deadline is not the same fact as a slow command: if the + // budget cut this rerun short, "timed out — infrastructure" would send the + // reader hunting a hang that is really an exhausted budget. Record which. + const clamped = remaining < perCommandMs; + const base = exec(t.command, baseline, Math.min(perCommandMs, remaining)); + if (base.timedOut && clamped) budgetClamped.push(t.command); + // Prefer what the run itself measured off the untrimmed text; fall back to + // re-parsing the bounded output only for a seam that supplies neither. + const baseFailingFiles = base.timedOut + ? [] + : (base.failingFiles ?? failingFilesOf(base.output, baseline)); + // The PR side is what netNew/shared are derived from, so a PR side that + // parsed NOTHING attributes nothing — regardless of what the base rerun + // managed to parse. Requiring both sides to be empty silently dropped a + // failed command whose FAIL lines the trim had scattered. + const unparsed = prFailingFiles.length === 0; + // The PR side is read out of build-test's STORED output, which that command + // trimmed on the same rules. The base side is parsed raw (see BaseRunResult) + // so it can never be the short one, but nothing here can un-trim the report: + // a PR-side set missing files makes `shared` — not `netNew` — too small, so + // the loss is silence, and silence still gets said out loud. + const prTruncated = TRIM_MARKER_RE.test(t.output ?? ''); + // A base run that never finished attributes NOTHING: with its failing set + // unknowable, promoting the PR side's failures to net-new would + // manufacture the strongest evidence this command produces out of an + // infrastructure timeout. The files stay unattributed (neither list), and + // the note says why. + // ...and so does a base rerun that FAILED without naming a single failing + // file. An unbuilt base tree, a missing node_modules, a workspace the PR + // ADDED (so `npm test --workspace=…` cannot resolve on base), an ENOBUFS + // truncation: each exits non-zero with zero FAIL lines, indistinguishable + // here from a green base. Reading it as green promotes every PR-side + // failure to netNew — the strongest evidence this command emits, + // manufactured from a base that never ran a test. + const baseUnusable = + base.timedOut || (base.exitCode !== 0 && baseFailingFiles.length === 0); + entries.push({ + command: t.command, + prFailingFiles, + baseFailingFiles, + netNew: baseUnusable + ? [] + : prFailingFiles.filter((f) => !baseFailingFiles.includes(f)), + shared: baseUnusable + ? [] + : prFailingFiles.filter((f) => baseFailingFiles.includes(f)), + base, + unparsed, + prTruncated, + }); + } + + const netNew = [...new Set(entries.flatMap((e) => e.netNew))].sort(); + const shared = [...new Set(entries.flatMap((e) => e.shared))].sort(); + const unparsed = entries.filter((e) => e.unparsed).length; + const timedOut = entries.filter((e) => e.base.timedOut).length; + const truncated = entries.filter((e) => e.prTruncated).length; + + const parts: string[] = []; + if (netNew.length) { + parts.push( + `${netNew.length} failing file(s) do NOT fail on base — the PR's own by measurement: ${netNew.join(', ')}`, + ); + } + if (shared.length) { + parts.push( + `${shared.length} failing file(s) also fail on base — pre-existing, whatever files the diff touches: ${shared.join(', ')}`, + ); + } + if (unparsed) { + parts.push( + `${unparsed} command(s) failed but named no parseable failing file — no delta for those; judge them by the diff as before`, + ); + } + if (skippedUnrecognised.length) { + parts.push( + `${skippedUnrecognised.length} failed command(s) were not rerun because they are not the shape \`build-test\` emits (${skippedUnrecognised.join(', ')}) — this command executes what the report names, so it executes only that grammar; their failures stay unattributed, judge them by the diff`, + ); + } + if (truncated) { + parts.push( + `${truncated} command(s) had their PR-side output trimmed before this ran, so their failing-file list may be partial — a file missing there is one this delta could not call pre-existing, never one it invented`, + ); + } + const unusable = entries.filter( + (e) => + !e.unparsed && + e.prFailingFiles.length > 0 && + e.netNew.length === 0 && + e.shared.length === 0 && + !e.base.timedOut, + ); + if (unusable.length) { + parts.push( + `${unusable.length} command(s) could not be attributed — the base rerun ${unusable + .map( + (e) => + `\`${e.command}\` failed (exit ${e.base.exitCode}) without naming a failing file, so it did not measure the base (an unbuilt tree, a missing install, a workspace absent at base)`, + ) + .join('; ')}; judge those failures by the diff as before`, + ); + } + if (timedOut) { + parts.push( + `${timedOut} base-side rerun(s) timed out — infrastructure, not evidence` + + (budgetClamped.length + ? ` (${budgetClamped.length} of them on a deadline the whole-command budget shortened, not their own: ${budgetClamped.join(', ')} — a rerun with budget to spare may still measure them)` + : ''), + ); + } + if (skippedForBudget.length) { + parts.push( + `${skippedForBudget.length} failed command(s) not rerun — the whole-command budget was exhausted (${skippedForBudget.join(', ')}); their failures stay unattributed, judge them by the diff`, + ); + } + return { + entries, + netNew, + shared, + note: parts.join('. ') || 'nothing to report', + }; +} + +export const testDeltaCommand: CommandModule = { + command: 'test-delta', + describe: + "Rerun the PR side's failed test commands on the base tree and report which failing files are the PR's own (net-new) vs pre-existing (shared)", + builder: (yargs) => + yargs + .option('report', { + type: 'string', + demandOption: true, + describe: + "Agent 7's build-test report (its failed commands and outputs)", + }) + .option('baseline', { + type: 'string', + demandOption: true, + describe: 'The BUILT base tree from `qwen review base-tree`', + }) + .option('pr-worktree', { + type: 'string', + describe: + "The PR worktree the report's failures were produced in — its root " + + 'is stripped so both sides compare as repo-relative paths', + }) + .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', + }), + handler: (argv) => { + const args = argv as unknown as TestDeltaArgs; + const report = runTestDelta(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-delta: ${report.note}`); + }, +}; 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 d0c8b2f281..b369fd8f32 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -311,10 +311,12 @@ describe('test-efficacy probe isolation (#6832)', () => { // 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'); + // No safety verb, no `??`, no `+ CONST`, and the condition edit carries no + // comparison — zero candidates for EVERY operator, which is the premise. write( 'packages/lib/src/f.ts', 'export function price(n: number) {\n' + - ' if (n < 0) return 0;\n' + + ' if (valid(n)) return 0;\n' + ' return n * 2;\n' + '}\n' + '\n'.repeat(12) + @@ -326,7 +328,7 @@ describe('test-efficacy probe isolation (#6832)', () => { write( 'packages/lib/src/f.ts', 'export function price(n: number) {\n' + - ' if (n <= 0) return 0;\n' + + ' if (!valid(n)) return 0;\n' + ' return n * 3;\n' + '}\n' + '\n'.repeat(12) + @@ -454,6 +456,66 @@ process.stdout.write(JSON.stringify({ ).toBe(false); }); + it('runs a REPLACEMENT mutant end-to-end and reports the survivor', async () => { + // The three new operators take the `lines[line-1] = mutated` branch of + // runOneMutant, and nothing exercised write-file -> run-probe -> classify + // for it: the unit tests stop at candidate selection, and the other + // integration fixture was deliberately made operator-free. + write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); + write( + 'packages/lib/src/f.ts', + 'export function pick(a?: string) {\n return a;\n}\n', + ); + const base = commitAll('base'); + write( + 'packages/lib/src/f.ts', + 'export function pick(a?: string) {\n' + + ' return a ?? fallback.value;\n' + + '}\n', + ); + write( + 'packages/lib/src/f.test.ts', + 'import { pick } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof pick).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')); + const coalesce = out.mutants.probed.find( + (m: { operator?: string }) => m.operator === 'coalesce', + ); + expect(coalesce).toBeDefined(); + expect(coalesce.mutated).toBe(' return a;'); + expect(coalesce.verdict).toBe('survived'); + // The wording must match the operator: a replacement CHANGES the line. + expect(coalesce.detail).toContain('when it changes'); + expect( + out.findings.some((f: { message: string }) => + f.message.includes('?? fallback'), + ), + ).toBe(true); + // 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 @@ -927,8 +989,10 @@ process.stdout.write(JSON.stringify({ expect(out.mutants.skippedForCap).toBe(1); expect(out.mutants.skippedForBaseline).toBe(0); expect(out.mutants.probed.length + out.mutants.skippedForCap).toBe(9); + // Names BOTH caps: this count carries sub-cap drops too, and a message + // naming only the total sends the reader after candidates that never were. expect(stdoutChunks.join('')).toContain( - '1 mutant(s) skipped: more candidates than the cap of 8', + '1 mutant(s) skipped: more candidates than the selection caps (8 total, 3 of them replacements)', ); }); diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 241120e32f..47d334635e 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -6,6 +6,7 @@ import { describe, it, expect } from 'vitest'; import { + replacementMutantsOf, splitDiffIntoHunks, selectHunkProbes, MAX_HUNK_PROBES, @@ -1631,3 +1632,152 @@ describe('selectHunkProbes', () => { expect(skippedForCap).toBe(0); }); }); + +describe('selectMutants — replacement operators', () => { + const fileOf = (content: string, addedLines: number[]) => ({ + file: 'src/m.ts', + content, + addedLines, + hasNewTests: false, + }); + + it('selects a replacement candidate on a diff with no safety verbs', () => { + const { selected } = selectMutants([ + fileOf('const model = pick() ?? config.getModel();\n', [1]), + ]); + expect(selected).toEqual([ + { + file: 'src/m.ts', + line: 1, + statement: 'const model = pick() ?? config.getModel();', + operator: 'coalesce', + mutated: 'const model = pick();', + }, + ]); + }); + + it('spends the cap on deletion mutants BEFORE replacement ones', () => { + const { selected, skippedForCap } = selectMutants( + [fileOf('if (a !== b) go();\nstate.clear();\n', [1, 2])], + 1, + ); + expect(selected).toHaveLength(1); + expect(selected[0].statement).toBe('state.clear();'); + expect(selected[0].operator).toBeUndefined(); + expect(skippedForCap).toBe(1); + }); + + it('caps replacements at their sub-cap and counts what it drops', () => { + // 24x pool inflation measured on real commits: uncapped replacements would + // drain the time window hunk probes draw from last, silently un-shipping + // the hunk-survived finding class. + const many = Array.from({ length: 6 }, (_, i) => + fileOf(`if (a${i} !== b${i}) go();\n`, [1]), + ).map((f, i) => ({ ...f, file: `src/g${i}.ts` })); + const { selected, skippedForCap } = selectMutants(many); + expect(selected).toHaveLength(3); // REPLACEMENT_SUB_CAP + expect(skippedForCap).toBe(3); + }); + + it('emits one candidate per line — a safety-verb line is not also mutated by replacement', () => { + // The input must trigger BOTH paths or the `continue` under test is not + // load-bearing: this line carries a safety verb AND a `?? fallback`, so + // without the guard it would yield a replacement candidate too. + const { selected } = selectMutants([ + fileOf('cache.delete(key) ?? fallback.reset();\n', [1]), + ]); + expect(selected).toHaveLength(1); + expect(selected[0].operator).toBeUndefined(); // deletion won + }); +}); + +describe('replacementMutantsOf', () => { + const same = (line: string) => replacementMutantsOf(line, line.trim()); + + it('drops a simple `?? fallback`', () => { + expect(same(' const m = pick() ?? config.getModel();')).toEqual({ + operator: 'coalesce', + mutated: ' const m = pick();', + }); + }); + + it('leaves a `??` whose fallback is not a simple chain alone', () => { + // Dropping part of `a ?? b + c` would truncate a larger expression. + expect(same('const n = x ?? y + z;')).toBeNull(); + }); + + it('drops a `+ UPPER_CONST` reserve term', () => { + expect( + same(' if (estimate + COMPACT_MAX_OUTPUT_TOKENS > window) {'), + ).toEqual({ + operator: 'term-drop', + mutated: ' if (estimate > window) {', + }); + }); + + it('replaces a comparison-bearing if condition with true', () => { + expect(same(' if (effective !== config.getModel()) {')).toEqual({ + operator: 'guard-true', + mutated: ' if (true) {', + }); + }); + + it('PRESERVES leading whitespace when editing a trimmed code view', () => { + // The shipped-then-caught bug: codeLines are trimmed, so an index computed + // there and applied to the raw line spliced `iftrue 0)` into a guard. The + // edit is now computed on the code view and re-indented. + const raw = ' if (n <= 0) return 0;'; + expect(replacementMutantsOf(raw, 'if (n <= 0) return 0;')).toEqual({ + operator: 'guard-true', + mutated: ' if (true) return 0;', + }); + }); + + it('yields NOTHING when the raw line and the code view disagree', () => { + // A string or comment was blanked out of the code view — indices cannot be + // trusted across the two, so the line is conservatively skipped. + expect( + replacementMutantsOf( + " if (s === ')') { fire(); }", + "if (s === '') { fire(); }", + ), + ).toBeNull(); + }); + + it('handles nested parens in the condition', () => { + expect(same('if (a(b) !== c(d, e(f))) return;')).toEqual({ + operator: 'guard-true', + mutated: 'if (true) return;', + }); + }); + + it('does not read a GENERIC call as a comparison', () => { + // `if (isRecord(v))` is a type-guard predicate — the `if (ready)` + // shape whose survivors this gate calls noise. Telling `a(x)` needs a parser, so the gate stays silence-biased. + expect(same('if (isRecord(v)) return;')).toBeNull(); + expect(same('if (fn(x)) go();')).toBeNull(); + // A spaced comparison still qualifies. + expect(same('if (n <= 0) return 0;')?.operator).toBe('guard-true'); + }); + + it('does not read an arrow function as a comparison', () => { + // `=>` ends in `>` followed by a space, so the old class matched it and + // every predicate guard became a guard-true candidate — exactly the + // `if (ready)` noise the gate exists to exclude. + expect(same('if (items.some((x) => x.ok)) return;')).toBeNull(); + expect(same('if (fn(() => run())) go();')).toBeNull(); + // A real comparison still qualifies. + expect(same('if (a !== b) go();')?.operator).toBe('guard-true'); + }); + + it('skips an if with no comparison, and one whose condition spans lines', () => { + expect(same('if (ready) go();')).toBeNull(); + expect(same('if (a !== b &&')).toBeNull(); + }); + + it('emits at most one candidate per line, most-specific first', () => { + // Both a `??` and a comparison on one line: coalesce wins. + expect(same('if ((x ?? fallback) !== y) go();')?.operator).toBe('coalesce'); + }); +}); diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index bfd9ab6da0..2a36e0effa 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -145,7 +145,23 @@ export function planTestEfficacy( export type MutantVerdict = 'killed' | 'survived' | 'inconclusive'; -export interface MutantCandidate { +/** + * A candidate the probe will run. The two shapes are a union so an operator + * without its replacement line is UNREPRESENTABLE: `runOneMutant` takes the + * ACTION from `mutated` and the verdict WORDING from `operator`, so a + * half-populated candidate would delete a line while reporting "with its + * `?? fallback` dropped". + */ +export type MutantCandidate = DeletionMutant | ReplacementMutant; + +export interface ReplacementMutant extends MutantBase { + operator: 'coalesce' | 'guard-true' | 'term-drop'; + /** The full replacement LINE (untrimmed). Required by construction. */ + mutated: string; +} + +/** What both mutant shapes carry. */ +export interface MutantBase { file: string; /** 1-based line number in the post-change file. */ line: number; @@ -153,10 +169,21 @@ export interface MutantCandidate { statement: string; } -export interface MutantResult extends MutantCandidate { +/** + * The legacy shape: the line is DELETED. `operator` is absent (or `'delete'`) + * and there is no replacement line — see the union above for why that is + * enforced by the type rather than by a convention. + */ +export interface DeletionMutant extends MutantBase { + operator?: 'delete'; + mutated?: undefined; +} + +/** An intersection, not `extends`: the candidate is a union now. */ +export type MutantResult = MutantCandidate & { verdict: MutantVerdict; detail: string; -} +}; /** * At most this many deletion mutants per run. Every mutant is a full vitest run @@ -195,6 +222,12 @@ export interface HunkResult extends Omit { */ export const MAX_HUNK_PROBES = 6; +/** + * At most this many REPLACEMENT mutants per run, inside the shared cap — see + * the selection comment for the 24x pool measurement that forced this. + */ +export const REPLACEMENT_SUB_CAP = 3; + /** Deadline for one vitest run (baseline, mutant, or revert probe alike). */ const PROBE_RUN_TIMEOUT_MS = 300_000; @@ -558,12 +591,112 @@ export interface MutantSourceFile { * backtick derails it) has ALL its candidates dropped and is returned in * `derailed` — the caller must disclose that zero for the same reason. */ +/** + * (Below: the replacement operators. `selectMutants`' own contract doc sits + * directly above `selectMutants` — this block documents its helper.) + * + * Replacement mutants for one added line. High-precision by construction: each + * pattern is anchored to a shape whose survival maps to one crisp sentence, + * because a survivor becomes a public Suggestion and a fuzzy operator would + * flood the report with "so what" mutations. + * + * The edit is computed on `codeLine` — the scanner's literal-blanked, + * comment-stripped, TRIMMED view — and reattached to the raw line's leading + * whitespace. That is only sound when the two views agree, so a line whose + * trimmed raw text differs from its code view (it carries a string literal or + * a comment) yields NO candidate: an index computed on one view and applied to + * the other spliced `iftrue 0)` into a guard the first time this ran, and a + * mangled mutant is worse than a skipped one — its compile error reads as + * `inconclusive` and quietly eats a cap slot. Conservative silence, as with + * every other selector here. + * + * At most ONE candidate per line, first match wins (coalesce → term-drop → + * guard-true, most-specific first): two mutants of the same line would run the + * suite twice to say nearly the same thing. + */ +export function replacementMutantsOf( + raw: string, + codeLine: string, +): { + operator: 'coalesce' | 'guard-true' | 'term-drop'; + mutated: string; +} | null { + if (raw.trim() !== codeLine) return null; + const lead = /^\s*/.exec(raw)![0]; + const done = ( + operator: 'coalesce' | 'guard-true' | 'term-drop', + edited: string, + ) => ({ operator, mutated: lead + edited }); + + // `a ?? b` with a SIMPLE fallback — an identifier/member/call chain, no + // operators — so the drop cannot truncate a larger expression. + const coalesce = + /\s\?\?\s+[\w$.]+(?:\((?:[^()]|\([^()]*\))*\))?(?=\s*[;,)\]}]|\s*$)/.exec( + codeLine, + ); + if (coalesce) { + return done( + 'coalesce', + codeLine.slice(0, coalesce.index) + + codeLine.slice(coalesce.index + coalesce[0].length), + ); + } + // `+ UPPER_CONST` — a constant-looking reserve/limit term in arithmetic. + const term = /\s\+\s+[A-Z][A-Z0-9_]{2,}\b/.exec(codeLine); + if (term) { + return done( + 'term-drop', + codeLine.slice(0, term.index) + + codeLine.slice(term.index + term[0].length), + ); + } + // A single-line `if (…)` whose condition CONTAINS a comparison — guards, not + // every branch: `if (ready)` survivors are noise, `if (a !== b)` survivors + // mean nothing pins when the guard must not fire. The condition must close on + // this line (balanced parens), or `true` would splice mid-expression. + // `}` optional before `else`: a brace-less `else if (a !== b)` is the same + // guard shape and was silently skipped. + const ifm = /^((?:}?\s*else\s+)?if\s*\()(.*)$/.exec(codeLine); + if (ifm) { + let depth = 1; + let condEnd = -1; + for (let i = 0; i < ifm[2].length; i++) { + if (ifm[2][i] === '(') depth++; + else if (ifm[2][i] === ')' && --depth === 0) { + condEnd = i; + break; + } + } + // The comparison must be in the CONDITION, not anywhere after `if (`: + // testing the whole remainder admitted `if (ready) emit(a !== b);` — the + // comparison-less shape whose survivors are pure noise, which this gate + // exists to exclude. + if ( + condEnd > 0 && + // Two exclusions, both measured. `(?])…(?![=>])` keeps an arrow + // function's `=>` from reading as a comparison. And the trailing `\s` is + // REQUIRED, not an accidental asymmetry with `[!=]==`: without it a + // generic call — `if (isRecord(v))` — matches at `(x)` needs a + // parser; the gate is silence-biased by design, and Prettier makes the + // unformatted comparison near-nonexistent here. + /[!=]==|(?])[<>]=?(?![=>])\s/.test(ifm[2].slice(0, condEnd)) + ) { + return done('guard-true', ifm[1] + 'true' + ifm[2].slice(condEnd)); + } + } + return null; +} + export function selectMutants( files: MutantSourceFile[], cap: number = MAX_MUTANTS, ): { selected: MutantCandidate[]; skippedForCap: number; derailed: string[] } { const preferred: MutantCandidate[] = []; const rest: MutantCandidate[] = []; + const replPreferred: MutantCandidate[] = []; + const replRest: MutantCandidate[] = []; const derailed: string[] = []; for (const f of files) { const lines = f.content.split('\n'); @@ -576,20 +709,52 @@ export function selectMutants( const raw = lines[n - 1]; if (raw === undefined) continue; const t = raw.trim(); - if (!SAFETY_VERB_RE.test(codeLines[n - 1] ?? '')) continue; if (inLiteral[n - 1]) continue; - if (!isRemovableStatement(lines, codeLines, n - 1)) continue; - (f.hasNewTests ? preferred : rest).push({ - file: f.file, - line: n, - statement: t, - }); + const code = codeLines[n - 1] ?? ''; + if ( + SAFETY_VERB_RE.test(code) && + isRemovableStatement(lines, codeLines, n - 1) + ) { + // No `operator` field: deletion is the legacy shape, and stamping it + // would churn every existing report reader for zero information. + (f.hasNewTests ? preferred : rest).push({ + file: f.file, + line: n, + statement: t, + }); + continue; // one candidate per line — deletion is the sharper experiment + } + const repl = replacementMutantsOf(raw, code); + if (repl) { + (f.hasNewTests ? replPreferred : replRest).push({ + file: f.file, + line: n, + statement: t, + operator: repl.operator, + mutated: repl.mutated, + }); + } } } - const eligible = [...preferred, ...rest]; + // Deletions first, then replacements — and replacements carry their OWN + // sub-cap. Measured over 40 real commits, the replacement operators produce + // ~24x the deletion pool (215 vs 9 candidates; guard-true drives it), and + // every mutant run drains the same time window hunk probes draw from last: + // uncapped, most diffs with any replacement candidates would leave hunk + // probing zero runs and the hunk-survived finding class would silently stop + // firing. Three slots keeps the highest-value replacements without buying + // coarser answers at the hunk probes' expense; what the sub-cap drops is + // counted in skippedForCap, never silently lost. + const replacements = [...replPreferred, ...replRest]; + const eligible = [ + ...preferred, + ...rest, + ...replacements.slice(0, REPLACEMENT_SUB_CAP), + ]; + const subCapSkipped = Math.max(0, replacements.length - REPLACEMENT_SUB_CAP); return { selected: eligible.slice(0, cap), - skippedForCap: Math.max(0, eligible.length - cap), + skippedForCap: Math.max(0, eligible.length - cap) + subCapSkipped, derailed, }; } @@ -1398,7 +1563,20 @@ export function runOneMutant( 'the probe tree does not match the selected statement at this line — nothing was mutated', }; } - lines.splice(mutant.line - 1, 1); + // A replacement operator edits the line; the legacy shape deletes it. + const what = + mutant.operator === 'coalesce' + ? 'with its `?? fallback` dropped' + : mutant.operator === 'guard-true' + ? 'with its guard condition replaced by `true`' + : mutant.operator === 'term-drop' + ? 'with its `+ CONSTANT` term dropped' + : 'deleted'; + if (mutant.mutated !== undefined) { + lines[mutant.line - 1] = mutant.mutated; + } else { + lines.splice(mutant.line - 1, 1); + } try { writeFileSync(abs, lines.join('\n'), 'utf8'); const { perFile } = runProbeSuite( @@ -1411,9 +1589,13 @@ export function runOneMutant( const verdict = classifyMutantRun(perFile); const detail = verdict === 'killed' - ? 'the suite went red with this statement deleted — a test catches its removal' + ? `the suite went red with this statement ${what} — a test catches it` : verdict === 'survived' - ? 'every affected test still PASSED with this statement deleted — no test fails when it is removed' + ? `every affected test still PASSED with this statement ${what} — no test fails ${ + mutant.mutated === undefined + ? 'when it is removed' + : 'when it changes' + }` : 'the mutated tree produced no clean verdict (likely a compile or import error) — not evidence either way'; return { ...mutant, verdict, detail }; } finally { @@ -1873,7 +2055,14 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { .map((m) => ({ file: m.file, 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.`, + message: + m.operator === 'coalesce' + ? `\`${m.file}:${m.line}\`: dropping the \`?? fallback\` from \`${m.statement}\` leaves every affected test green — the fallback is untested, and it is frequently the only thing standing between a miss and a worse default. Add a test that exercises the miss path.` + : m.operator === 'guard-true' + ? `\`${m.file}:${m.line}\`: forcing this guard's condition to \`true\` leaves every affected test green — no test pins when the guard must NOT fire. Add a case just on the other side of the condition.` + : m.operator === 'term-drop' + ? `\`${m.file}:${m.line}\`: dropping the \`+ CONSTANT\` term from \`${m.statement}\` leaves every affected test green — nothing pins what that term contributes. Add a case where its presence decides the outcome (a boundary, if the expression is arithmetic).` + : `\`${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') @@ -1932,7 +2121,12 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { } if (mutantsSkippedForCap > 0) { writeStdoutLine( - ` ${mutantsSkippedForCap} mutant(s) skipped: more candidates than the cap of ${MAX_MUTANTS}`, + // BOTH caps, because this count carries drops from either: with 2 + // deletions and 6 replacements the total is exactly MAX_MUTANTS and the + // main cap never fires, yet the sub-cap drops 3. Naming only the main cap + // then sends the reader looking for a pool of 11 candidates that does not + // exist — the number is right, the reason was not. + ` ${mutantsSkippedForCap} mutant(s) skipped: more candidates than the selection caps (${MAX_MUTANTS} total, ${REPLACEMENT_SUB_CAP} of them replacements)`, ); } if (mutantsSkippedForBaseline > 0) { diff --git a/packages/cli/src/commands/review/test-plan.test.ts b/packages/cli/src/commands/review/test-plan.test.ts index 7e86f9f300..dc3b6d9412 100644 --- a/packages/cli/src/commands/review/test-plan.test.ts +++ b/packages/cli/src/commands/review/test-plan.test.ts @@ -163,6 +163,34 @@ describe('extractClaims', () => { expect(claims[0].text).toContain('157'); }); + it('does not extract the MIXED file-count shape either', () => { + // The shape a runner prints the moment any file fails — which is when a + // summary actually gets pasted into a Test Plan. The label is no longer + // adjacent to the number it qualifies, so an adjacency rule lets `44` + // through as a test count and the note reads `claimed 44, observed 1323`. + const claims = extractClaims( + 'Test Files 1 failed | 44 passed (45)\n Tests 2 failed | 1323 passed (1325)', + ).filter((c) => c.kind === 'count'); + expect(claims.map((c) => c.text)).toEqual(['1323 passed']); + }); + + it('does not extract jest Test Suites counts as test counts', () => { + // Same rule, jest's spelling: every number after the label counts suites. + const claims = extractClaims( + 'Test Suites: 1 failed, 44 passed, 45 total\nTests: 2 failed, 1323 passed, 1325 total', + ).filter((c) => c.kind === 'count'); + expect(claims.map((c) => c.text)).toEqual(['1323 passed']); + }); + + it('keeps a bare count on a line that never named files', () => { + // The mask is per-line: blanking to end of line must not swallow a + // legitimate count that follows on the NEXT one. + const claims = extractClaims('Test Files 3 passed\n471 passed').filter( + (c) => c.kind === 'count', + ); + expect(claims.map((c) => c.text)).toEqual(['471 passed']); + }); + it('emits one claim per distinct count', () => { const claims = extractClaims( 'core: 1135 passed, desktop: 41 passed', diff --git a/packages/cli/src/commands/review/test-plan.ts b/packages/cli/src/commands/review/test-plan.ts index 39ba638d63..7c875b623a 100644 --- a/packages/cli/src/commands/review/test-plan.ts +++ b/packages/cli/src/commands/review/test-plan.ts @@ -209,9 +209,52 @@ 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(? m.index keeps this terminating. + FILE_COUNT_LABEL_RE.lastIndex = stop; + } + return out + section.slice(cursor); +} + /** Extract every backticked span, including fenced-block bodies. */ function codeSpans(section: string): string[] { const spans: string[] = []; @@ -347,10 +390,13 @@ export function extractClaims(section: string): Array<{ // 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]> = []; + // Length-preserving and byte-identical outside the blanked spans, so a match + // found here carries the original text and indexes the original section. + const forCounts = maskFileCounts(section); for (const re of COUNT_RES) { re.lastIndex = 0; let m: RegExpExecArray | null; - while ((m = re.exec(section))) { + while ((m = re.exec(forCounts))) { const start = m.index; const end = start + m[0].length; if (taken.some(([s, e]) => start < e && end > s)) continue; diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 1306da35fd..7942776272 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -397,6 +397,35 @@ Three deliberate limits: - **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 test failures are attributed by measurement, and why the delta is over file sets + +Agent 7's brief has always carried a path rule: a failure in a file the diff changed is a Critical, one in a file it did not touch is pre-existing. It was the best rule available when the review had one tree, and it misclassifies in both directions — an environment-sensitive test failing in a touched file gets filed as a Critical the PR did not cause, and a PR that breaks a test in an untouched file (the exact shape the base-tree section above is about) gets waved through. The first live run of this pipeline hit the benign half: three env-sensitive core failures the model had to _reason_ into "pre-existing, not in diff", correctly but on judgment. + +With `base-tree` standing, attribution is decidable: `test-delta` reruns the same failed command in the built merge base and diffs the outcomes. The two design points that matter: + +- **File sets, not counts.** Measured on a live re-verification: the same branch's flaky suite failed _different test names_ on two consecutive runs, so counts (and names) are noise. The failing-file set is the stable unit, and an empty net-new set is the strongest "all pre-existing" statement obtainable. +- **Failures only, and base attributes nothing it did not finish.** A green PR-side suite has nothing to attribute, and base's suite was green before the PR existed — so the base run costs exactly one rerun per PR-side failure. A base rerun that times out attributes _nothing_: promoting PR-side failures to net-new off an unfinished run would manufacture the command's strongest evidence out of an infrastructure timeout (this shipped briefly in review of the command itself, caught because the test that "covered" it asserted only the note text). The same holds for every other way the base side can end up unmeasured — a rerun that failed without naming a file, a command the budget could not fit, a base tree that would not build — and the report names each with its own reason, because "we could not measure" and "we measured nothing" are different facts to the author. + +## Why the cache carries a findings ledger + +A human reviewer's round-2 comment opens with "M1 is fixed"; the pipeline's round-2 opened with a fresh list, because the incremental cache stored a _count_ and a _verdict_ — enough to scope the diff to `lastCommitSha..HEAD`, nothing with which to say what became of round 1's findings. The author was left to diff two reports by hand, which inverts who is doing the review. + +So the cache now carries the findings themselves, with round-scoped ids (`R1-2`), and an incremental re-review owes each entry a ruling under the same bar the open-Criticals re-check already enforces — _fixed_ requires tracing that the mechanism can no longer fire, not observing that the diff contains a fix. Two boundaries keep the ledger honest: only **confirmed high-confidence** findings enter (next round re-asserts each entry by id, so the ledger holds claims the review stands behind), and a finding ruled fixed _leaves_ (the cache is what the next round must check, not history — the report already told the story). The fail-closed rules are unchanged: a run that must not advance the cache does not advance the ledger either. + +## Why three more mutation operators, and why each is shaped the way it is + +Statement deletion with a safety-verb filter was the first operator because it has the cleanest survivor semantics. But a live maintainer re-verification produced a survivor list the deletion operator cannot express — and every entry mapped to one of three shapes, each with equally crisp semantics: + +- **`?? fallback` dropped.** The surviving case was the one line preventing a previously-fixed regression from returning through a different path — a coalesce to `getModel()` that nothing tested. A coalesce survivor means the miss path is unexercised, and the miss path is frequently the entire safety property. +- **Guard condition → `true`.** The surviving case was the round-2 fix _itself_ — a skip-condition shipped in response to review, tested by nothing. Restricted to comparison-bearing `if`s on purpose: forcing `if (ready)` to `true` survives trivially everywhere and means nothing; forcing `a !== b` to `true` surviving means no test pins when the guard must _not_ fire, which is precisely the untested half of any guard. +- **`+ UPPER_CONST` dropped.** The surviving case was a reserve term in a budget estimate. A term-drop survivor means the constant never decides any test's outcome — the boundary is unpinned. + +Mechanically they are **replacements**, not deletions, which bought one bug worth recording: the selector's per-line code view is trimmed and literal-blanked, and an edit index computed there and applied to the raw line spliced `iftrue 0)` into a guard. The fix is the conservative equivalence the selector now enforces — a line whose raw text and code view disagree (it carries a string or comment) yields no candidate at all, because a mangled mutant is worse than a skipped one: its compile error reads as `inconclusive` and quietly spends a cap slot. Deletion mutants keep cap priority (they have the track record); the operators queue behind them and every skip is counted. + +## Why the quality brief checks documentation parity, not documentation + +"This flag needs docs" is a reviewer's preference; "three of this flag's four siblings have a docs entry and it does not" is the codebase's own convention, broken. The lens is deliberately the second shape: no documented sibling, no finding, and the finding must name the precedent file — so the Suggestion arrives as the house standard rather than taste. The trigger that earns it a place at all is the compounding case from a live review: a surface whose behaviour can _silently change_ (an automatic model swap with a warning) shipping undocumented leaves the user staring at a message with nowhere to look it up. + ## 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. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 9badb41a03..e4ab691c3a 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -118,7 +118,7 @@ Based on the parsed `target.type`: Worktree isolation: all subsequent steps (agents, build/test) operate inside `worktreePath`, not the user's working tree. Cache and reports (Step 8) are written to the **main project directory**, not the worktree. - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-.json` exists, read `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): - - If SHAs differ → continue with the worktree just created. Compute the incremental diff (`git diff ..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. + - If SHAs differ → continue with the worktree just created. Compute the incremental diff (`git diff ..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. - If SHAs match **and** model matches **and** `--comment` was NOT specified → inform the user "No new changes since last review", run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-` to remove the worktree just created, and stop. - If SHAs match **and** model matches **but** `--comment` WAS specified → run the full review anyway. Inform the user: "No new code changes. Running review to post inline comments." - If SHAs match **but** model differs → continue. Inform: "Previous review used {cached_model}. Running full review with {{model}} for a second opinion." @@ -464,6 +464,8 @@ Rules: at most 2; launch none when no domain stands out (the common case — mos Build and test results are **deterministic facts**. A code-caused failure skips Step 4 verification — the `[build]` / `[test]` source tag is how it is recognised as pre-confirmed. An environment/setup failure (a missing dependency, a tool not installed) is informational only and must not affect the verdict. Test-efficacy findings are deterministic in the same way, and likewise pre-confirmed. +When the PR side's tests fail, Agent 7's brief has it **measure** the attribution rather than judge it by path: `base-tree` + `test-delta` rerun the same failed commands on the built merge base and diff the failing **file sets**. `netNew` (fails on the PR side only) is the PR's own failure by measurement — a Critical even in a file the diff never touched; `shared` (fails on base too) is pre-existing by measurement — never filed, even in a file the diff rewrote. Counts are deliberately not compared: a flaky suite fails different test names between two runs of the same tree, so the file-set difference is the signal and an empty `netNew` is the strongest "pre-existing" statement available. Where the delta cannot rule — no merge base, an unparsed failure, a timed-out base rerun, a base rerun that failed without naming any failing file (it did not measure the base), or a command the whole-command budget could not fit — the old path judgment stands, and the report names each case with its own reason rather than folding them into one. + If the probe reports `inconclusive`, that is **not a finding and must never be reported as one**: reverting the source often breaks the test's own compile, and a runner that collected nothing is not a test catching a regression. Note it in the terminal and move on. ## Step 3C: Inline pass (low effort) @@ -631,6 +633,16 @@ List every chunk that returned `Uncoverable` in Step 3, with the files it spans, If there are none of these, omit this section. +### Previous round's findings (incremental re-review only) + +When Step 1 loaded a findings ledger from the cache, this review is **round N+1 of the same PR**, and the single most useful thing it can tell the reader is what happened to round N's findings — a re-reviewer who only lists new findings leaves the author to diff two reports by hand. Rule on **every** ledger entry against the code at the reviewed commit, exactly the way the open-Criticals re-check below rules (trace the mechanism; the diff containing a fix is not the same claim as the defect no longer firing): + +- **fixed** — the mechanism can no longer fire. Say so, by id, in one line: `R1-2 fixed by `. Do not re-report it as a finding. +- **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. +- **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. + +Render the rulings as a short table at the top of the Findings section — id, one-line title, this round's status — so the report reads as a continuation, the way a human reviewer's round-2 comment opens with "M1 is fixed". The incremental scope rule does not conflict with this: the _diff_ reviewed is `lastCommitSha..HEAD`, but a ledger ruling reads the code at HEAD, which every agent already has. + ### Before an Approve or a zero-Critical verdict: re-check the open Criticals A `C=0` outcome — Approve, or a Comment with no Critical — is a claim that nothing blocks the merge. It is not the default you fall back to when your own agents surfaced nothing. **If Step 1 set the context-unavailable state** (`pr-context` failed — lightweight or same-repo), there is no context file to read: skip the walk below, record every existing Critical as `cannot tell` by construction, and carry that into the verdict — which the Step 7 invariant already caps at `COMMENT`. Otherwise, take **each live blocker already on the PR — from every comment-bearing section of the context file: "Open inline comments", "Blockers to re-check", "Review summaries", and "Already discussed" (both its inline threads and its issue-level comments)** — and check it against the code as it stands at the reviewed commit. Select **semantically, not by the literal marker**: a `**[Critical]**` prefix qualifies, but so does any body that asserts a blocking defect in other words — a "Critical findings could not be anchored" preamble, an explicit must-fix claim (legacy body-only blockers were emitted markerless, and one such review is exactly what a marker filter once discarded). When unsure whether a body asserts a blocker, re-check it — the cost is one ruling; the alternative is certifying a merge past it. ("Already discussed" stays in scope even though `pr-context` now promotes blocker-bearing bodies out of it: `carriesBlockerSignal` is a **fail-safe floor, not a ceiling** — it recognises the phrasings we have seen, not every phrasing that exists, and a blocker worded around all of them still settles there. That section's "do NOT re-report" header governs duplicate-_reporting_ by the finder agents; it does not exempt a body from this re-check. Read it with the same eyes you bring to the promoted section.) Review-level bodies matter because an unmappable or 422-relocated blocker lives **only** there — and the context file now carries them **in full**: `pr-context` renders every meaningful review body whole under "Review summaries" (no more 240-character snippets), and pulls every blocker-bearing body — replied inline thread or issue comment, marker or no marker — into the "Blockers to re-check" section, rendered in full, because a reply alone never settles a blocker. So the re-check usually needs no separate fetch: read those sections under the file's untrusted-data preamble, paging with `offset`/`limit` until `isTruncated` is false. **For the status half of each INLINE-thread ruling — is the anchor outdated, did the anchored file change since the blocker was filed, which commits touched it — read Step 1's `comment-status` report instead of fetching per-comment metadata**: its `code.touchedBy` list is the candidate "fixed by" commits to read, and `changedSinceComment: false` (with no head drift) tells you the anchored file is untouched since the blocker — so a claimed fix, if any, must live in some OTHER file, and the mechanism-read below is still owed either way. Two scope limits, both deliberate: the report exists only **when Step 1 wrote it** (worktree mode, fetch succeeded — a lightweight-mode run still walks this re-check and re-derives status facts the old way), and it indexes **inline threads only** — an issue-level or review-level blocker (the #6486 shape) has no entry there and keeps the context-file walk as its sole source. The report never substitutes for reading the code: it routes the read, it does not rule. Review summaries and blocker bodies are rendered in full; the Open and Already-discussed sections use one-line snippets, and **every snippet the renderer cut carries its own `_(truncated — fetch …)_` note naming the exact, already-filled-in command for the rest** — a candidate blocker whose snippet was cut is ruled on only after running that fetch; ruling on the visible prefix alone is the fail-closed violation. Run any such fetch **redirected to a file, never into the terminal** (Shell returns only an approximately 4 000-character model preview for output beyond its 30 000-character persistence trigger, which would re-truncate the very body being completed): append `--jq .body > .qwen/tmp/qwen-review-{target}-body-.md` to the command the note names, then `read_file` that file, paging until `isTruncated` is false, before ruling. **Fail closed either way:** a body you could not read whole — the capped tail unfetched, or the single-object fetch failing (auth, rate limit, network) — is `cannot tell`, not "no Critical in it": it goes to compose-review's `cannotTellCriticals` input, which serializes it and caps the event at `COMMENT`; a blocker you could not read is never approved past. A reply alone does not retire a blocker — "I disagree" or "wontfix" is a reply, which is exactly why `pr-context` quarantines blocker-bearing threads in their own section instead of letting them settle into "Already discussed". Only the code decides: a blocker counts as closed exactly when the re-check below lands on "fixed by this diff", never because the thread has an answer. Record one verdict per blocker: @@ -994,11 +1006,24 @@ If reviewing a PR **at high effort**, update the review cache for incremental re "lastCommitSha": "", "lastModelId": "{{model}}", "lastReviewDate": "", + "round": , "findingsCount": , - "verdict": "" + "verdict": "", + "findings": [ + { + "id": "R-", + "severity": "Critical | Suggestion", + "file": "", + "line": , + "title": "", + "status": "open" + } + ] } ``` + The `findings` ledger is what lets the **next** high-effort run open with "R1-2 is fixed" instead of a from-scratch list (see Step 6's previous-round section). Write every **newly confirmed high-confidence** finding under a fresh `R-` id, and carry a still-standing previous entry forward **under the id it already has** — the whole payoff is that `R1-2` names the same claim in every round, so a finding that survives is re-reported, never renumbered — a finding ruled `fixed` this round leaves the ledger (the report said so; the cache is for what the next round must check, not history). Low-confidence and terminal-only findings stay out: the ledger holds claims this review stands behind, because next round re-asserts each one by id. + 3. Ensure `.qwen/reviews/` and `.qwen/review-cache/` are ignored by `.gitignore` — a broader rule like `.qwen/*` also satisfies this. Only warn the user if those paths are not ignored at all. ## Step 9: Clean up