vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-08-06 23:35:34 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 17

feat(review): measured failure attribution, round ledger, richer mutants, doc parity (#8218)

Browse source 
* feat(review): borrowed-verification trio — test-plan check, base-tree A/B, per-hunk probes

- 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.

* fix(review): survive real runner output — ANSI-laced and trimmed-away summaries

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.

* fix(review): address the eight findings from live review of this branch

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'.

* fix(cli): never score a hunk survived when its own test left the baseline (#8215)

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.

* fix(review): silence-bias hardening from four live review rounds of this branch

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.

* feat(review): four round-2 borrowings — measured failure attribution, round ledger, richer mutants, doc parity

Shaped by a live round-2 maintainer re-verification (QwenLM/qwen-code#7818):

- qwen review test-delta: rerun the PR side's failed test commands on the
  built merge base and diff the failing FILE SETS — netNew is the PR's own
  failure by measurement (whatever files the diff touches), shared is
  pre-existing by measurement. Counts are never compared: a flaky suite
  fails different test names between runs of the same tree. An unfinished
  base rerun attributes nothing.
- Round ledger: the incremental review cache persists confirmed findings
  under round-scoped ids (R1-2); a high-effort re-review rules on each
  (fixed / still stands / cannot tell) and opens its report with the table,
  the way a human round-2 comment opens with 'M1 is fixed'.
- Three replacement mutation operators in test-efficacy: drop '?? fallback',
  force a comparison-bearing guard condition to 'true', drop a '+ CONST'
  term. Each survivor maps to one crisp untested-protection sentence.
  A line whose raw text and literal-blanked code view disagree yields no
  candidate — an edit index computed across the two views once spliced
  'iftrue 0)' into a guard, and a mangled mutant reads as inconclusive
  while quietly spending a cap slot.
- Quality brief: documentation-parity lens — a new user-facing surface whose
  siblings are documented is a Suggestion naming the sibling precedent; no
  documented sibling, no finding.

* fix(review): address review feedback — false-positive hardening, binary diff guard, error convention (#8215)

* fix(review): address review feedback — base-tree availability gate, test-plan false positives, hunk-probe ranges (#8215)

- 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.

* fix(review): address review feedback — diff-header false positives, stale prompt enumeration, added-file hunk probes (#8215)

* fix(review): address review feedback — cd-base exclusion, Test Files count guard, base-tree error handling, probe delegation (#8215)

* fix(review): port the collocated-dropout test to the post-#8050 runner seam

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.

* fix(review): bound the summary rescue, apply the ATX heading rule, sweep stale build locks

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 <n>: …' 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.

* 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.

* fix(review): close the ten open findings on this PR

- guard-true tested for a comparison anywhere after `if (`, including the
  then-body, so `if (ready) emit(a !== b);` admitted a mutant on the
  comparison-less condition the gate exists to exclude. It now tests the
  condition span only.
- The `survived` detail said "when it changes" for legacy DELETION
  mutants too; it now matches the operator.
- test-delta's `unparsed` required both sides to parse zero files, so a
  PR-side failure whose FAIL lines the trim scattered was silently
  dropped whenever the base rerun happened to parse. netNew/shared come
  from the PR side, so the PR side alone decides.
- failingFilesOf now matches Windows path shapes (backslashes, C:) - a
  missed parse is an unattributed failure, not a loud error.
- The replacement branch of runOneMutant (write-file -> run-probe ->
  classify) had no end-to-end test; one now drives a coalesce operator
  through the real handler and asserts the mutated line, the verdict,
  the operator-specific wording, and that the shared tree is untouched.
- Two tests were vacuous with respect to what they promised: the
  baseline-dir test never asserted the cwd (its helper swallowed the
  argument - fixed at the helper), and the one-candidate-per-line test
  used an input that never triggered the replacement path, so the
  `continue` under test was not load-bearing.
- Reattached the orphaned selectMutants JSDoc; reworded the SKILL line.

* fix(review): the test helper's cwd parameter is required, not optional

CI's `tsc --build` failed on test-delta.test.ts: the exec seam always
passes a cwd, but the helper's signature marked it optional, so pushing
it into a string[] was `string | undefined`.

Missed locally because vitest runs through esbuild, which strips types
without checking them - the suite was green while the build was red. The
gate to run before pushing a type-level change is `npx tsc --build`, not
the test suite.

* fix(review): a base rerun that could not RUN attributes nothing

Two Criticals from the latest review, both reachable on the brief's own
happy path:

- baseUnusable covered only timeouts. Every other way the base side can
  fail to run - an unbuilt base tree, a missing install, a workspace the
  PR ADDED (npm test --workspace cannot resolve on base), an ENOBUFS
  truncation - exits non-zero with zero FAIL lines, which this code read
  as a green base. Every PR-side failure then became netNew: the
  strongest evidence the command emits, manufactured from a base that
  never ran a test. It now attributes nothing and says why.
- Timeout detection was the weaker substring form the sibling explicitly
  rejects; an external SIGTERM (container stop, cancelled job) set
  neither an ETIMEDOUT message nor an exit code and fed straight into
  the above. build-test now exports spawnTimedOut and test-delta asks
  the same question rather than re-deriving it.

Also: the base output is trimmed (it precedes the verdict fields in the
report the agent reads, so an untrimmed megabyte truncates exactly what
the command produces); the guard-true gate no longer reads an arrow
function's => as a comparison (every predicate guard was a candidate -
the if (ready) noise the gate exists to exclude); the term-drop message
no longer calls a string concatenation a reserve term; the unparsed note
describes its own PR-side-only condition; and the ledger's Step 6/Step 8
now agree that a still-standing finding keeps its id.

* fix(review): compare failing files by a normalised, project-keyed identity

Critical: the two sides run in DIFFERENT roots (the PR worktree and the
base tree), and netNew/shared compared the parsed paths verbatim - so an
absolute-path runner turned every pre-existing failure into a fabricated
Critical, with the authority of a measurement behind it. Paths are now
normalised against each run's own root (and backslashes to /, so a
Windows path compares with its POSIX-printed twin), which is why
test-delta gained --pr-worktree.

The identity also keeps the vitest project token: dropping it collapsed
same-named test 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 - the worse failure direction.

Also from the same review, all of them reachable on the brief's own path:

- The base rerun now inherits build-test's stdio: ['ignore','pipe','pipe']
  ("a build that asks a question is a build that hangs until the
  deadline") and its trimOutput, which matters because entries[].base
  precedes the verdict fields in the report the agent reads.
- The brief gates on base-tree's `available`, not just its `path`: a tree
  that was created but did not build populates path too, and measuring
  against it turns an infrastructure failure into Criticals.
- A programmatic caller omitting `timeout` no longer sends NaN into
  spawnSync.
- MutantCandidate is a discriminated union, so an operator without its
  replacement line - which would delete a line while reporting "with its
  ?? fallback dropped" - is unrepresentable.
- The comparison class no longer requires a trailing space (if (a<b) is
  the same guard, just unformatted) and matches a brace-less else if.
- DeltaEntry.unparsed's doc now describes the PR-side-only condition it
  actually implements.

* fix(review): restore the whole-command budget, keep generics out of guard-true

Round-1 findings from a fresh review of this PR:

- test-delta had no aggregate deadline: --timeout is PER command and
  defaults to 300s, so three failed commands is 900s against Agent 7's
  600s ceiling - killed with NO report at all, discarding the base-tree
  install and build just paid for. TOTAL_BUDGET_MS mirrors the one
  test-efficacy reserves; commands it cannot fit are disclosed.
- guard-true matched generic calls: `if (isRecord<string>(v))` produced a
  mutant, and a type-guard predicate is exactly the `if (ready)` shape
  whose survivors the gate calls noise. The trailing \s is required, not
  an accidental asymmetry with [!=]== - telling `a<b` from `fn<T>(x)`
  needs a parser, and the gate is silence-biased by design.
- --pr-worktree had no contract test, and its failure mode is the worst
  here: arriving undefined, root stripping silently stops and every
  pre-existing failure becomes a fabricated netNew. The new test feeds
  parseSync's output straight into runTestDelta and asserts an
  attribution only reachable when both roots were stripped (verified
  red against the snake_case field shape that shipped once already).
- Merged the two consecutive doc comments on prWorktree.

* fix(review): a budget-shortened deadline is not the same fact as a slow rerun

Round-2 finding on the budget just restored: `Math.min(perCommandMs,
remaining)` can hand a rerun far less than --timeout, and if it dies
there the note said only "timed out - infrastructure, not evidence".
True, but it sends the reader hunting a hang that is really an exhausted
budget - and unlike a real timeout, a rerun with budget to spare would
still measure it. The note now names those commands separately and says
so. Verified red against removing the tracking line.

* fix(review): restore the replacement sub-cap, stranded on a downstream branch

Round-3 finding, and the third instance of one class: an 8218-layer fix
committed on the 8261 branch, four PRs above the code it belongs to.

Measured over 40 real commits, the replacement operators produce ~24x
the deletion pool (215 vs 9 candidates; guard-true drives it). Every
mutant run drains the same window hunk probes draw from LAST, so
uncapped, most diffs with any replacement candidates leave hunk probing
zero runs - the hunk-survived finding class silently stops firing and
nothing says so. Three slots, and what the sub-cap drops is counted in
skippedForCap rather than lost.

Also swept the other direction: diffed every review file against its
8261 copy to confirm nothing else 8218-layer is stranded up there. The
remaining divergence is 8261's own (the positive control, its lenses).

* docs(review): complete the "delta cannot rule" enumeration in both places

Round-4 finding. The brief and SKILL.md each listed three cases where
test-delta attributes nothing - unparsed, timed-out base, no merge base -
but the code has five: the later rounds of this PR added "a base rerun
that failed without naming any failing file" (it did not measure the
base) and "a command the whole-command budget could not fit". Two
enumerations of the same set with different membership, in the two
places an agent reads.

That is the sibling-enumeration lesson this skill teaches, applied to
its own prose for the second time: the fix is not just adding the
missing members but saying that the report names each case with its own
reason rather than folding them into one.

* docs(review): the rationale named only one of the ways base goes unmeasured

Third and last copy of the enumeration the previous commit fixed. The
bullet's headline already generalised - "base attributes nothing it did
not finish" - but its body named only the timeout, so a reader learning
the contract from the rationale would conclude timeouts are the only
unusable case. Name the set, and say why the report keeps the reasons
apart: "we could not measure" and "we measured nothing" are different
facts to the author, and only one of them is about their PR.

* fix(review): a file-count label stops counting at the end of its line

`Test Files  45 passed` filing its 45 as a differing TEST count was
fixed once, with a lookbehind on the bare-count pattern. That only ever
rejected the all-green shape. The moment any file fails, the runner
prints `Test Files  1 failed | 44 passed (45)`, the label is no longer
adjacent to the number, and 44 comes through as a test count - so the
note reads "claimed 44, observed 1323" on exactly the runs whose summary
someone would paste. Adjacency was the wrong invariant; the line is.

Masking from the label to end-of-line is distance-independent and picks
up jest's `Test Suites: 1 failed, 44 passed, 45 total` at no cost. The
label keeps its `Test` word on purpose: a first cut matched a bare
`files` and blanked the line in "expect all four files and 471 tests to
pass", silencing a real claim. An existing test caught that, which is
the argument for the rule being as narrow as it is - anything that
suppresses claims is worth exactly its narrowness.

Also drops the now-dead lookbehind, which a reviewer had separately (and
wrongly) called a JS syntax error; variable-length lookbehind is legal
in V8, and the module parsed fine. It goes because the line rule
subsumes it, not because it was broken.

* fix(review): parse the base rerun before its output is trimmed

`trimOutput` rescues module errors and runner summaries out of the
omitted middle, not the per-file FAIL lines this command reads. A base
suite whose failure section overruns the tail budget therefore lost
failing files into the gap - and a SHORT base set is the dangerous
direction, because netNew is the PR side minus the base side. Every file
the trim hid came back as a Critical attributed to this PR by
"measurement". Parse the raw text, report the bounded one.

The PR side cannot be fixed here: it is read out of build-test's stored
output, trimmed before this command existed. That loss runs the other
way - it understates `shared`, never invents a netNew - so the entry
carries `prTruncated` and the note says the list may be partial. A
silence-biased gap is still a gap the author should hear about.

Also names both selection caps in the mutant-skip diagnostic. The count
accumulates replacement sub-cap drops, and with 2 deletions and 6
replacements the total is exactly MAX_MUTANTS: the main cap never fires,
yet 3 are dropped, and the message sent the reader looking for a pool of
11 candidates that never existed.

* fix(review): rerun only the command shapes build-test emits

This command reads a report off disk and then executes the strings in it
with `shell: true`, in the base worktree. Nothing else in the pipeline
re-executes a value it read back from a file, so nothing else has to
care where that value came from - this does, and the provenance is worse
than it looks: the command is `npm test --workspace="<dir>"`, the
workspace token is a directory, and a directory is a name a pull request
chooses. `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, since that
grammar is the two shapes build-test produces. A command outside it is
skipped and disclosed, the same treatment everything else this command
cannot do already gets, so a future shape degrades to "judge it by the
diff" rather than to arbitrary execution.

---------

Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com>
Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
This commit is contained in:
Shaojin Wen 2026-08-01 20:34:30 +08:00 • committed by GitHub
parent 36698c5669
commit 9bfe608be9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 1506 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

1
packages/cli/src/commands/review.test.ts
View file

@ -50,6 +50,7 @@ describe('reviewCommand', () => {
'agent-prompt',
'build-test',
'base-tree',
'test-delta',
'script-lint',
'resolve-anchors',
'check-coverage',

4
packages/cli/src/commands/review.ts
View file

@ -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: () => {

22
packages/cli/src/commands/review/build-test.ts
View file

@ -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,

4
packages/cli/src/commands/review/lib/agent-briefs.ts
View file

@ -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 <plan> --worktree <worktree> --out <plan dir>/qwen-review-pr-<n>-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 <the build-test report you wrote> --baseline <the base-tree report's path field> --pr-worktree <this worktree> --out <plan dir>/qwen-review-pr-<n>-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 <base>:<path>\`), 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.

467
packages/cli/src/commands/review/test-delta.test.ts Normal file
View file

@ -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>): 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<BuildTestReport>));
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']);
});
});

453
packages/cli/src/commands/review/test-delta.ts Normal file
View file

@ -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="<dir>"`, 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<string>();
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}`);
},
};

70
packages/cli/src/commands/review/test-efficacy.integration.test.ts
View file

@ -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)',
);
});

150
packages/cli/src/commands/review/test-efficacy.test.ts
View file

@ -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<string>(v))` is a type-guard predicate — the `if (ready)`
// shape whose survivors this gate calls noise. Telling `a<b` from
// `fn<T>(x)` needs a parser, so the gate stays silence-biased.
expect(same('if (isRecord<string>(v)) return;')).toBeNull();
expect(same('if (fn<Bar>(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');
});
});

228
packages/cli/src/commands/review/test-efficacy.ts
View file

@ -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<HunkCandidate, 'patch'> {
*/
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<string>(v))` — matches at `<string`, and
// a type-guard predicate is exactly the `if (ready)` shape whose
// survivors this gate calls noise. Telling `a<b` from `fn<T>(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<void> {
.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<void> {
}
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) {

28
packages/cli/src/commands/review/test-plan.test.ts
View file

@ -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',

50
packages/cli/src/commands/review/test-plan.ts
View file

@ -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(?<!Files\s{1,20})(\d+)\s+pass(?:ed|ing)\b/gi,
/\b(\d+)\s+pass(?:ed|ing)\b/gi,
];
/**
* Labels after which every number on the line counts FILES, not tests.
*
* `Test Files 45 passed (45)` filing its 45 as a differing TEST count was
* measured on this command's own PR body. The first fix was a lookbehind on
* the bare-count pattern, which only ever rejected the all-green shape: the
* moment any file fails the runner prints `Test Files 1 failed | 44 passed`,
* and the label is no longer adjacent to the number. That mixed shape is the
* COMMON one — a summary gets pasted into a Test Plan precisely when there is
* something to show — so the narrow fix left the false note in place for the
* case that produces it. Masking the rest of the line is label-distance
* independent, and covers jest's `Test Suites: 1 failed, 44 passed, 45 total`
* for free.
*
* Both runner labels carry the `Test` word, and the pattern requires it: a
* bare `files` is ordinary prose, and blanking its line ate the future-tense
* claim in "expect all four files and 471 tests to pass" — an existing test
* caught it. A rule that silences claims is worth exactly as much as its
* narrowness.
*/
const FILE_COUNT_LABEL_RE = /\btest\s+(?:files|suites)\b/gi;
/**
* Blank out file-count segments, preserving length so match offsets still
* index into the original section.
*/
function maskFileCounts(section: string): string {
let out = '';
let cursor = 0;
FILE_COUNT_LABEL_RE.lastIndex = 0;
let m: RegExpExecArray | null;
while ((m = FILE_COUNT_LABEL_RE.exec(section))) {
const eol = section.indexOf('\n', m.index);
const stop = eol === -1 ? section.length : eol;
out += section.slice(cursor, m.index) + ' '.repeat(stop - m.index);
cursor = stop;
// Resume past the blanked run: the label matches again inside it
// (`Test Files ... 3 files`), and stop > 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;

29
packages/core/src/skills/bundled/review/DESIGN.md
View file

@ -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.

29
packages/core/src/skills/bundled/review/SKILL.md
View file

File diff suppressed because one or more lines are too long