diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index baf84c600ef..922cdfbf240 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -189,6 +189,8 @@ 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 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. Each failure scenario must name what input or state now slips past the removed behaviour, and what wrong outcome results.`, @@ -231,6 +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. - XSS — stored, reflected, DOM-based - SSRF and path traversal - Authentication and authorization bypass @@ -250,6 +253,7 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th - Style consistency with the surrounding codebase; naming conventions - **Duplication and missed reuse.** When the diff re-implements something the codebase already has, grep the shared/utility modules and the files adjacent to the change, and **name the existing helper it should call instead**. A duplication finding that does not name the thing being duplicated is not a finding. +- **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`, @@ -268,7 +272,12 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th - Unnecessary re-renders, for UI code - Inefficient algorithms or data structures - Missing caching opportunities -- Bundle-size impact`, +- Bundle-size impact + +**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.`, }, '5': { @@ -294,7 +303,9 @@ 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** 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. + +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.`, }, '6a': { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index d5413a885e9..c47b5fbeb26 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**. | -| `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. | -| `3` | **Code quality.** Duplication that names the existing helper to call instead; over-engineering; and **altitude** — is the fix at the right depth, or a bandaid on shared infrastructure? | -| `4` | **Performance & efficiency.** N+1s, leaks, needless re-renders, bad data structures, bundle size. | -| `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 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. | 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).