From baa04ce78b268cba0a2cb7c63145c111d5d37752 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 09:28:28 +0800 Subject: [PATCH] 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. |