From baa04ce78b268cba0a2cb7c63145c111d5d37752 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 09:28:28 +0800 Subject: [PATCH 1/4] feat(review): mutation-test the tests in the test-coverage pass (Agent 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent 5 checked that tests exist and cover the diff's paths, but not that they would actually FAIL if the code were broken. A test that stays green no matter what the code does is worse than no test: it reads as coverage while blessing the exact regression it was written to catch. Add a mutation-testing discipline to Agent 5's brief (and its whole-diff test-matrix counterpart): for a test this diff adds or changes to pin a value or behaviour, name the one-line mutation to the code under it that should turn it red — if none does, the test is vacuous. Call out the recurring shapes: both sides of the assertion computed the same way so they move together (`expect(undefined).toBe(undefined)` from a loose/unanchored extraction); reading only the first of several sites the behaviour spans; an `expect(x).toBe(x)` tautology; a "does not throw" assertion for a bug that is a wrong value, not a throw. A vacuous test is a Suggestion on its own, but a Critical when it is the sole guard for a behaviour this diff changes. The agent briefs are CLI-generated (lib/agent-briefs.ts), so the behavioural change is there; the SKILL.md dimension table is synced for the human reader. --- .../cli/src/commands/review/lib/agent-briefs.ts | 13 +++++++++++-- packages/core/src/skills/bundled/review/SKILL.md | 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 25fd2ea546b..baf84c600ef 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -286,7 +286,15 @@ Expect the three ends to be far apart. The declaration, the pass-through, and th - Do the assertions actually verify *behaviour*, or only that the code ran without throwing? - Are integration boundaries tested, not just the unit-level happy path? -**Do not complain about "low coverage" abstractly.** Point to a specific code path in the diff that lacks a test and say what scenario is uncovered. And keep the severity honest: a missing test is a **Suggestion**. If a missing test would let a specific incorrect behaviour ship, report **that behaviour** as the Critical and cite the missing test as your evidence — naming the bug is the work, naming the gap is not.`, +**Do not complain about "low coverage" abstractly.** Point to a specific code path in the diff that lacks a test and say what scenario is uncovered. And keep the severity honest: a missing test is a **Suggestion**. If a missing test would let a specific incorrect behaviour ship, report **that behaviour** as the Critical and cite the missing test as your evidence — naming the bug is the work, naming the gap is not. + +**Mutation-test the tests that matter — a test that exists is not the same as a test that would catch the bug.** For any test this diff adds or changes to pin a specific value or behaviour, name the one-line mutation to the *code under test* (or to the test's own value extraction) that *should* make it fail; if no plausible mutation does, the test is **vacuous** — it passes whether the code is right or wrong. The recurring shapes to check for: + +- both sides of the assertion are computed the same way, so they move together — e.g. \`expect(extract(a)).toBe(extract(b))\` where a loose or unanchored \`extract\` returns \`undefined\` on both, i.e. \`expect(undefined).toBe(undefined)\`; **pin the literal** instead; +- 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.`, }, '6a': { @@ -378,7 +386,8 @@ 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.`, +- 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.`, }, 'invariant-a': { diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 3f6f6015f5d..94f6a927391 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -427,7 +427,7 @@ An agent that finds nothing must say so **and say what it walked** — `No issue | `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. | +| `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. | From ee8955063f461430b1d78f0e807d051c90b3de0e Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 10:19:16 +0800 Subject: [PATCH 2/4] feat(review): add contract-shape, migration, equivalent-mutant, and perf-repro lenses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Borrow four review disciplines a maintainer applies by hand, wiring them into the agent briefs (CLI-generated) so every review gets them: - Agent 1b (removed-behavior): a changed *literal* is a contract too — when the diff renames/reformats a value a distant consumer matches on its raw shape (a marker string, a key, a status code, regex text), grep the old shape, not the symbol; a rename that compiles can silently stop matching a filter in another file. And a rename/format/schema/default change must handle the data that already exists — an un-migrated population is a split-brain (orphaned records, double-writes), usually fixed by a two-line legacy fallback. - Agent 5 (test coverage): before calling a test vacuous, rule out the equivalent mutant — a mutation that leaves observable behaviour unchanged is not a coverage gap. Name the input that makes the mutation observable. - Agent 4 (performance): do not take the PR's own numbers on trust. Reproduce a cheap deterministic claim (bundle bytes, tree-shake) or report it doesn't; flag a runtime benchmark you can't re-run as unverified rather than endorsing it. SKILL.md dimension table synced for the human reader. Briefs only — no control flow changes. --- .../src/commands/review/lib/agent-briefs.ts | 13 +++++++-- .../core/src/skills/bundled/review/SKILL.md | 28 +++++++++---------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index baf84c600ef..1a87c66e6d4 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.`, @@ -268,7 +270,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 +301,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 94f6a927391..2814eb3f007 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -418,20 +418,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. | +| `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. **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). From 7728e6fcad5e4313c60e8bf052b39d4dbcc58a71 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 10:25:49 +0800 Subject: [PATCH 3/4] feat(review): flag subprocess option/argument injection in the security pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent 2 listed shell/command injection but not the sink `execFile`/`spawn` leaves open: a user-controlled positional argument to git/gh/tar/ffmpeg that is reinterpreted as an option or special token. A value starting with `-` becomes a flag (`git log --output=` overwrites an arbitrary file; `git checkout -f` discards the working tree) and `.`/`..` becomes a pathspec (`git checkout .` drops unstaged changes) — none of which shell-free spawning stops. Add it to the brief with the standard fix (validate + terminate the argv with `--`) and the call-site-asymmetry tell (one site validates, its sibling does not). SKILL.md security row synced. Briefs only. --- packages/cli/src/commands/review/lib/agent-briefs.ts | 1 + packages/core/src/skills/bundled/review/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 1a87c66e6d4..e6766adf0b9 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -233,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 diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 2814eb3f007..c12a95cc571 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -424,7 +424,7 @@ An agent that finds nothing must say so **and say what it walked** — `No issue | `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. | +| `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; 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. **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). | From facd10f01f2795f5cc3671e1083243a02817f353 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 10:30:54 +0800 Subject: [PATCH 4/4] feat(review): add the sibling-consistency lens to the code-quality pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When one member of a family of parallel paths carries a validation, guard, cleanup, or shape-check and its twin does not, the lone exception is usually accidental and the missing half is a latent asymmetric failure — harmless until the one input that path sees. Agent 3 now checks that siblings share the guard, names the divergent one, and escalates to the security pass when the missing guard is a validation on untrusted input (the `gitCheckout`-validates-but-its- sibling-does-not shape), rather than filing it as a style nit. SKILL.md quality row synced. Briefs only. --- packages/cli/src/commands/review/lib/agent-briefs.ts | 1 + packages/core/src/skills/bundled/review/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index e6766adf0b9..922cdfbf240 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -253,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`, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c12a95cc571..85c293d91fb 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -425,7 +425,7 @@ An agent that finds nothing must say so **and say what it walked** — `No issue | `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; and **altitude** — is the fix at the right depth, or a bandaid on shared infrastructure? | +| `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. |