fix(review): correct the borrowed lenses and vacuous-test severity (follow-up to #7735/#7736)

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

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 -- '<old-shape>' .\`): 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=<path>\` 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=<path>\` 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': {