diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 35d79c4f151..88bfb3423f2 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1790,6 +1790,48 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { expect(p).not.toMatch(/If you find no issues, say/i); }); + it('carries the mutation-testing lens into Agent 5, equivalent-mutant escape hatch included', () => { + // The all-role test above proves every brief gets the diff and the format; it + // cannot see whether a *specific* lens reached its role. If prompt assembly + // ever drops or misassigns the equivalent-mutant paragraph, that test stays + // green while Agent 5 again flags unobservable mutations as coverage gaps. + const p = buildRoleBrief(PLAN, '5'); + expect(p).toContain('Mutation-test the tests that matter'); + expect(p).toContain('equivalent mutant'); + // The discriminating-input requirement is what keeps the escape hatch from + // waving through a genuinely vacuous test. + expect(p).toContain('the input that makes it observable'); + // And the lens must not bleed into a sibling dimension's brief. + expect(buildRoleBrief(PLAN, '2')).not.toContain('equivalent mutant'); + // Severity must align with the shared ladder: a vacuous test is a Suggestion, + // escalated only by naming the concrete incorrect behaviour it lets ship — + // never Critical merely for being the sole guard, which would grade the same + // inert test above Agent 7's efficacy probe (a Suggestion) and inflate the + // verdict. This pins the semantic the "words Critical and Suggestion exist" + // check could not see. + expect(p).toContain('A vacuous test is a **Suggestion**'); + expect(p).toContain('report **that behaviour** as the Critical'); + expect(p).not.toContain('is a **Critical**: a green-no-matter-what'); + // The test-matrix agent applies Agent 5's rules to the behaviour/test pairing + // it owns, so its severity must move in lockstep — a revert of just this bullet + // would let the two agents grade the same inert test differently on one PR. + expect(buildRoleBrief(PLAN, 'test-matrix')).toContain( + 'a **Suggestion** on its own, Critical only when', + ); + }); + + it('carries the command-aware subprocess-injection correction into Agent 2', () => { + // The all-role test sees only that Agent 2 gets the diff and the format; it + // cannot see whether the `--` correction reached it. If a revert restores the + // old "terminate the argv with `--`" guidance, that test stays green while + // Agent 2 again advises that `--` alone closes the injection — but + // `git checkout -- .` still discards unstaged changes. Pin the corrected wording. + const p = buildRoleBrief(PLAN, '2'); + expect(p).toContain('does **not** neutralize a *pathspec*'); + expect(p).toContain('the value allowlist is what closes the injection'); + expect(p).not.toContain('terminate the argv with'); + }); + it('gives Agent 7 no diff — its evidence is the commands it ran', () => { // It runs the build. Requiring it to open the diff would be requiring a thing // its job does not involve, and reporting it "blind" for not doing so would diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 922cdfbf240..064c09ccf92 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -189,7 +189,7 @@ For every line the diff deletes or replaces: - **Search the new code for where that behaviour is re-established** — in the replacement lines, in a callee, in a helper. If you cannot find it, that is a candidate finding: a removed guard, a dropped error path, a narrowed validation, a lost cleanup, a deleted test that covered a real case. - **Treat a replacement as a deletion plus an insertion.** Check the new form preserves the old behaviour for **all** inputs, not just the common case: a rewritten condition that quietly drops one operand, a broadened \`catch\` that used to rethrow specific codes. - **Removed or renamed _exported_ symbols get the same treatment, one level up.** Enumerate every export the diff deletes or renames. Find what replaced it — often in another file — and compare the two as **behaviour, not as names**: did a default flip (\`includeSubdirs: true\` → an exact-match override)? did a scope narrow? did an error that used to propagate become a log line? Then look at **the call sites the diff never touches**: they still call the new thing and now mean something different by it. A replacement that compiles is not a replacement that behaves, nothing in the build will tell you, and the callers live outside the diff where no other agent will look. -- **A changed _literal_ is a contract too, not just a symbol.** When the diff renames or reformats a *value* that other code matches on its raw shape — a marker or sentinel string, a serialization key, a status/enum code, a path prefix, the exact text a regex or \`includes\`/\`startsWith\`/\`contains\` keys on — grep for consumers of the **old shape**, not the symbol. A rename that compiles can silently stop matching a filter three files (or three CI workflows) away, and nothing in the build will flag it: the consumer just quietly changes what it lets through or drops. Name the consumer and the concrete regression (a bot comment that now bypasses a filter, an event that no longer routes, a record that no longer dedupes). +- **A changed _literal_ is a contract too, not just a symbol.** When the diff renames or reformats a *value* that other code matches on its raw shape — a marker or sentinel string, a serialization key, a status/enum code, a path prefix, the exact text a regex or \`includes\`/\`startsWith\`/\`contains\` keys on — grep for consumers of the **old shape**, not the symbol, and **include hidden paths** (\`rg --hidden --glob '!.git/**' --fixed-strings -- '' .\`): a consumer in \`.github/workflows/*.yml\` or a sibling CI script is invisible to a default root search, and that is exactly the automation this rule exists to catch. A rename that compiles can silently stop matching a filter three files (or three CI workflows) away, and nothing in the build will flag it: the consumer just quietly changes what it lets through or drops. Name the consumer and the concrete regression (a bot comment that now bypasses a filter, an event that no longer routes, a record that no longer dedupes). - **A rename / format / schema / default change must handle the data that already exists.** If the change reads, matches, or upserts against persisted state — rows in a store, comments on a thread, entries in a cache, a config or lockfile on disk — check it handles the **pre-change population**, not just new writes. A marker renamed with no legacy fallback leaves every existing record unmatched (orphaned, or double-written on the next upsert); a widened schema with no migration splits state into old- and new-shaped halves. Flag the un-migrated population and the split-brain it causes, and note that the fix is usually a two-line fallback (accept the old shape while writing the new). - **For moved or renamed code, check the move is faithful.** A branch dropped during a move looks like clean refactoring in each hunk separately, and is invisible unless the two hunks are compared. @@ -233,7 +233,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th brief: `You are **Agent 2: Security**. Review the diff for: - Injection — SQL, command, prototype pollution, code injection -- **Argument / option injection into a subprocess — the sink \`execFile\`/\`spawn\` (no shell) does NOT close.** When user-controlled input reaches a subprocess as a **positional argument** — a \`git\`, \`gh\`, \`tar\`, \`ffmpeg\` … invocation built from request/config data — check whether that value can be reinterpreted as an **option or special token**. A value that starts with \`-\` becomes a flag (\`git log --output=\` overwrites an arbitrary file as the daemon user; \`git checkout -f\` discards the working tree), and tokens like \`.\` / \`..\` become a pathspec (\`git checkout .\` silently drops unstaged changes). \`execFile\` stops *shell* injection but passes these straight through, so a no-shell spawn is not a clean bill of health. Flag every such positional; the fix is to validate the value (a ref/name allowlist, reject a leading \`-\`) **and** terminate the argv with \`--\` so nothing after it is read as an option or pathspec. Watch for the asymmetry too — one call site validates, its sibling does not. +- **Argument / option injection into a subprocess — the sink \`execFile\`/\`spawn\` (no shell) does NOT close.** When user-controlled input reaches a subprocess as a **positional argument** — a \`git\`, \`gh\`, \`tar\`, \`ffmpeg\` … invocation built from request/config data — check whether that value can be reinterpreted as an **option or special token**. A value that starts with \`-\` becomes a flag (\`git log --output=\` overwrites an arbitrary file as the daemon user; \`git checkout -f\` discards the working tree), and tokens like \`.\` / \`..\` become a pathspec (\`git checkout .\` silently drops unstaged changes). \`execFile\` stops *shell* injection but passes these straight through, so a no-shell spawn is not a clean bill of health. Flag every such positional; the fix is to validate the value against the subcommand's own grammar (a ref/name allowlist, reject a leading \`-\`). A \`--\` separator ends *option* parsing but does **not** neutralize a *pathspec* — for an overloaded command it can create one (\`git checkout -- release\` restores the path \`release\` instead of switching to that branch; \`git checkout -- .\` still discards unstaged changes), so a \`--\` helps only where it keeps the operand's role, and is never a blanket fix — the value allowlist is what closes the injection. Watch for the asymmetry too — one call site validates, its sibling does not. - XSS — stored, reflected, DOM-based - SSRF and path traversal - Authentication and authorization bypass @@ -277,7 +277,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th **Do not take the PR's own performance numbers on trust — separate the claims you can reproduce from the ones you cannot.** - **Reproducible by inspection or a cheap deterministic check** — bundle bytes from the esbuild metafile, whether an import is actually tree-shaken out of the shipped chunk, a loop's iteration count, whether a cache is really consulted on the hot path: reproduce it and confirm the magnitude, or report that it does **not** reproduce. A claimed win whose mechanism cannot produce it (the "optimized" path still does the work, the lazy import is still statically reachable) is a finding, even when the PR shows a number. -- **A runtime benchmark you cannot re-run in review** — a wall-clock latency, a throughput figure tied to specific hardware: do not endorse it, and do not treat its absence as a defect. Flag the number as **unverified** and say what would verify it, so a green review never launders an unreproduced benchmark into a merge.`, +- **A runtime benchmark you cannot re-run in review** — a wall-clock latency, a throughput figure tied to specific hardware: do not endorse it, and do not treat its absence as a defect. Where the number is **load-bearing** for the PR, the actionable finding is to **request the benchmark script, its environment, and the raw results** so a reader can reproduce it; where it is incidental, it produces **no finding** — record it under what the review did not verify, never as a defect. Either way a green review must not launder an unreproduced benchmark into a merge.`, }, '5': { @@ -303,7 +303,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th - the assertion reads only the *first* of several sites the changed behaviour spans, so drift in a later site passes green; - an \`expect(x).toBe(x)\` / round-trip tautology, or a "does not throw" assertion for code whose bug would be a *wrong value*, not a throw. -A vacuous test is a **Suggestion** on its own. But when it is the **only** test guarding a behaviour this diff changes, it is a **Critical**: a green-no-matter-what test blesses the exact regression it was written to catch, and that is worse than no test, because it reads as coverage. +A vacuous test is a **Suggestion** — an ineffective guard is a gap, not a defect in the code, and grading it Critical merely for being the sole guard is the severity inflation the shared ladder is built to avoid (Agent 7's efficacy probe reports the very same inert test as a Suggestion, and Step 4 keeps the higher of the two). Escalate only the way this dimension always does: if the vacuous test lets a **specific incorrect behaviour** ship, report **that behaviour** as the Critical with the test as your evidence — naming the bug is the work, naming the gap is not; and a test that asserts the **opposite** of the intended behaviour, or was **weakened/disabled in this diff**, is already Critical under the existing rule. Before you call a test vacuous, rule out the **equivalent mutant** — a mutation that leaves observable behaviour unchanged is not a coverage gap, because *nothing* could discriminate it. If the branch you would flip is unreachable given an invariant the code already holds, or two paths are provably identical for every input the code can actually reach, the test is not weak; the mutation is unobservable by construction. Name the mutation you tried and the input that makes it observable — a surviving mutation is a finding only when a real input tells the mutant apart from the original.`, }, @@ -398,7 +398,7 @@ Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`, - **Map each behavioural change in the production code to the test that exercises it**, wherever that test lives. - **Flag behaviour/test pairs split across territories** — the change in one place, its only test weakened or deleted in another. That pairing is invisible to both of the agents who own those halves, which is the entire reason you exist. - Otherwise apply Agent 5's rules: name the specific untested scenario, never "coverage is low". A missing test is a **Suggestion**. **A test weakened, disabled, or deleted _in this diff_ so that new behaviour passes is Critical** — as is a test that asserts the opposite of the intended behaviour, because it will bless the very regression it was written to catch. -- **Mutation-test the pairing, do not just confirm it exists:** for the test you paired to a change, name the mutation that should turn it red; a test that stays green under that mutation — both sides of its assertion move together (\`expect(undefined).toBe(undefined)\`), or it reads only the first of the sites the change spans — is **vacuous**, and a vacuous test that is the sole guard for a behaviour this diff changes is a **Critical**, not coverage.`, +- **Mutation-test the pairing, do not just confirm it exists:** for the test you paired to a change, name the mutation that should turn it red; a test that stays green under that mutation — both sides of its assertion move together (\`expect(undefined).toBe(undefined)\`), or it reads only the first of the sites the change spans — is **vacuous** — a **Suggestion** on its own, Critical only when it asserts the opposite of the intended behaviour, was weakened in this diff, or lets a **specific incorrect behaviour** ship (report that behaviour, not the gap).`, }, 'invariant-a': { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c47b5fbeb26..47e3f7e6c67 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -433,20 +433,20 @@ An agent that finds nothing must say so **and say what it walked** — `No issue **`qwen review agent-prompt --role ` builds every one of these.** What follows is what each agent is _for_ — so you can read a finding and know which lens produced it, and so you can tell when a run is missing one. It is **not** what the agent is _sent_: that is in the command, and the command's copy is the one that arrives. When the two disagree, the command is right. -| Role | What it owns | -| ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `0` | **Issue fidelity & root-cause ownership** (PR reviews only). Does the change fix the thing it claims to fix — the _observed_ behaviour in the linked issue, not just the author's theory of it? Is the root cause the client's, or the upstream service's? A client-side workaround for malformed upstream data is a Critical unless a maintainer asked for it. An empty scope (feature PR, no linked issue) is a complete answer, with its evidence. | -| `1a` | **Line-by-line correctness.** Walks every hunk, reading the _enclosing function_ so the change is judged in its real context. Off-by-ones, inverted conditions, missing `await`, falsy-zero, swallowed errors, the language's own pitfalls, and wrapper/proxy routing. | -| `1b` | **Removed-behavior audit.** Owns the `-` lines, which exist only in the diff — the post-change tree carries no trace of what was deleted. For each removal: what invariant did it enforce, and where is that re-established? Includes removed or renamed _exports_ (compared to their replacement as **behaviour, not names**), changed _literals_ a distant consumer matches on by shape (marker strings, keys, codes, regex text), and whether a rename/format/schema change handles the data that **already exists** (migration / split-brain). | -| `1c` | **Cross-file tracer** (needs a local tree). Owns the whole cross-file walk. _Consumer direction_: grep every caller of every changed export and check it against the new contract. _Producer direction_: for every field the diff **adds**, grep its **read sites** — a live path reading a field the diff never populates is Critical, and nothing in the build will tell you. | -| `2` | **Security.** Injection, XSS, SSRF, path traversal, authn/authz bypass, secrets in logs, weak crypto, hardcoded credentials. Includes **option/argument injection into subprocess calls** — a user-controlled positional that starts with `-` or is `.`/`..` becomes a git/gh flag or pathspec (`--output=`, `-f`, `checkout .`); `execFile` does not stop it — validate and terminate the argv with `--`. | -| `3` | **Code quality.** Duplication that names the existing helper to call instead; over-engineering; **altitude** — is the fix at the right depth, or a bandaid on shared infrastructure?; and **sibling consistency** — a guard/validation one member of a parallel family has but its twin lacks (asymmetric failure; if the missing guard is on untrusted input, a security bug, not a nit). | -| `4` | **Performance & efficiency.** N+1s, leaks, needless re-renders, bad data structures, bundle size. **Reproduces the PR's claimed numbers** rather than trusting them — confirms a cheap deterministic claim (bundle bytes, tree-shake) or flags an unreproducible/unsubstantiated benchmark as unverified. | -| `5` | **Test coverage.** Specific untested paths in the diff, never "coverage is low"; a missing test is a Suggestion. **Mutation-tests the tests the diff adds/changes** — a test that stays green when the code under it is broken is vacuous, and a vacuous test that is the sole guard for a changed behaviour is a Critical (it blesses the regression). | -| `6a` `6b` `6c` | **Undirected audit, three personas** — attacker, 3 AM oncall, six-months-later maintainer. The framings force diverse paths; the union of what they find is the point, so all three run. | -| `7` | **Build & test verification** (needs a local tree). Runs _one_ build and _one_ test command, and the **test-efficacy probe** — which reverts the diff's source, keeps its tests, and reports the ones that pass anyway. Its evidence is the commands it ran. `Source: [build]` / `[test]`, never `[review]`. | -| `test-matrix` | **Test coverage matrix** (Step 3B). Maps each behavioural change to the test that exercises it — the pairing a territory agent cannot see, because it holds either the implementation or the test, rarely both. | -| `invariant-a` `invariant-b` `invariant-c` | **Whole-file invariants** on a `heavy` file, one checklist slice each: (a) mutable fields, timers, collections; (b) retry counters, ignored return values, error taxonomies; (c) config fields, early returns. | +| Role | What it owns | +| ----------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `0` | **Issue fidelity & root-cause ownership** (PR reviews only). Does the change fix the thing it claims to fix — the _observed_ behaviour in the linked issue, not just the author's theory of it? Is the root cause the client's, or the upstream service's? A client-side workaround for malformed upstream data is a Critical unless a maintainer asked for it. An empty scope (feature PR, no linked issue) is a complete answer, with its evidence. | +| `1a` | **Line-by-line correctness.** Walks every hunk, reading the _enclosing function_ so the change is judged in its real context. Off-by-ones, inverted conditions, missing `await`, falsy-zero, swallowed errors, the language's own pitfalls, and wrapper/proxy routing. | +| `1b` | **Removed-behavior audit.** Owns the `-` lines, which exist only in the diff — the post-change tree carries no trace of what was deleted. For each removal: what invariant did it enforce, and where is that re-established? Includes removed or renamed _exports_ (compared to their replacement as **behaviour, not names**), changed _literals_ a distant consumer matches on by shape (marker strings, keys, codes, regex text), and whether a rename/format/schema change handles the data that **already exists** (migration / split-brain). | +| `1c` | **Cross-file tracer** (needs a local tree). Owns the whole cross-file walk. _Consumer direction_: grep every caller of every changed export and check it against the new contract. _Producer direction_: for every field the diff **adds**, grep its **read sites** — a live path reading a field the diff never populates is Critical, and nothing in the build will tell you. | +| `2` | **Security.** Injection, XSS, SSRF, path traversal, authn/authz bypass, secrets in logs, weak crypto, hardcoded credentials. Includes **option/argument injection into subprocess calls** — a user-controlled positional that starts with `-` or is `.`/`..` becomes a git/gh flag or pathspec (`--output=`, `-f`, `checkout .`); `execFile` does not stop it — validate the value against the subcommand grammar (a ref/name allowlist, reject a leading `-`); a `--` separator ends option parsing but does **not** neutralize a pathspec (`checkout -- .` still discards changes), so the value allowlist is the fix. | +| `3` | **Code quality.** Duplication that names the existing helper to call instead; over-engineering; **altitude** — is the fix at the right depth, or a bandaid on shared infrastructure?; and **sibling consistency** — a guard/validation one member of a parallel family has but its twin lacks (asymmetric failure; if the missing guard is on untrusted input, a security bug, not a nit). | +| `4` | **Performance & efficiency.** N+1s, leaks, needless re-renders, bad data structures, bundle size. **Reproduces the PR's claimed numbers** rather than trusting them — confirms a cheap deterministic claim (bundle bytes, tree-shake) or flags an unreproducible/unsubstantiated benchmark as unverified. | +| `5` | **Test coverage.** Specific untested paths in the diff, never "coverage is low"; a missing test is a Suggestion. **Mutation-tests the tests the diff adds/changes** — a test that stays green when the code under it is broken is vacuous — a Suggestion, Critical only when it asserts the opposite, was weakened in-diff, or lets a named incorrect behaviour ship (report the behaviour, not the gap). | +| `6a` `6b` `6c` | **Undirected audit, three personas** — attacker, 3 AM oncall, six-months-later maintainer. The framings force diverse paths; the union of what they find is the point, so all three run. | +| `7` | **Build & test verification** (needs a local tree). Runs _one_ build and _one_ test command, and the **test-efficacy probe** — which reverts the diff's source, keeps its tests, and reports the ones that pass anyway. Its evidence is the commands it ran. `Source: [build]` / `[test]`, never `[review]`. | +| `test-matrix` | **Test coverage matrix** (Step 3B). Maps each behavioural change to the test that exercises it — the pairing a territory agent cannot see, because it holds either the implementation or the test, rarely both. | +| `invariant-a` `invariant-b` `invariant-c` | **Whole-file invariants** on a `heavy` file, one checklist slice each: (a) mutable fields, timers, collections; (b) retry counters, ignored return values, error taxonomies; (c) config fields, early returns. | Two things the command's briefs carry that no orchestrator should be relaying by hand, and that a hand-written prompt has never once included: the **Exclusion Criteria** (what is not a finding — the whole precision control), and the rules that make an **anchor** resolvable (prefer added lines; a removed line cannot be anchored; a bare `}` matches everywhere).