Skip to content

Commit a6dd2a7

Browse files
fix(review): ban unexecuted mutation claims in coverage briefs (#9923)
* fix(review): ban unexecuted mutation claims in coverage briefs Reader agents (Agent 5 and the test-matrix) described their reading-based mutation analysis in execution-grade language, so test-coverage findings could claim "mutant verified N/N green" while their own gap notice admitted the suite could not run in the review environment. Add witness discipline to both briefs: an unrung mutation must be phrased as a reasoned hypothesis, never an executed result, and a claim whose weight depends on an unperformed run carries `witness: not run — <why>`. * fix(review): align mutation-witness discipline across briefs (#9923) Name both banned execution-grade phrasings in the test-matrix bullet (Agent 5 already banned both), switch the nonstandard 'unrung' to the codebase's established 'unrun' vocabulary, and pin the banned-phrase list plus the false-capability guard for both briefs in the lockstep test so a one-sided edit to either copy fails the suite. * test(review): pin the hypothesis clause in the test-matrix lockstep The lockstep test pinned the witness tag and both banned phrasings for the test-matrix brief, but not its hypothesis-phrasing clause, while Agent 5's block pins its half — deleting only that clause from the test-matrix bullet shipped green. Pin the clause so both halves of the discipline stay graded identically. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
1 parent 22bb5e8 commit a6dd2a7

2 files changed

Lines changed: 29 additions & 2 deletions

File tree

packages/cli/src/commands/review/agent-prompt.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2960,12 +2960,37 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
29602960
expect(p).toContain('A vacuous test is a **Suggestion**');
29612961
expect(p).toContain('report **that behaviour** as the Critical');
29622962
expect(p).not.toContain('is a **Critical**: a green-no-matter-what');
2963+
// The brief's mutation analysis is reading-based — executed verdicts
2964+
// belong to Agent 7's efficacy probe — so its mutation claims must be
2965+
// phrased as hypotheses or carry an explicit not-run witness, never the
2966+
// execution-grade "verified N/N green" (issue #9901). The rule anchors on
2967+
// ownership, not on a capability claim: the review-agent tool table is
2968+
// role-neutral and includes the shell, so "you have no runner" would be
2969+
// false and must never come back.
2970+
expect(p).toContain('An unrun mutation is a hypothesis');
2971+
expect(p).toContain('ships N/N green');
2972+
expect(p).toContain('verified N/N green');
2973+
expect(p).toContain('witness: not run —');
2974+
expect(p).toContain('Executed mutation verdicts belong to Agent 7');
2975+
expect(p).not.toContain('you have no runner');
29632976
// The test-matrix agent applies Agent 5's rules to the behaviour/test pairing
29642977
// it owns, so its severity must move in lockstep — a revert of just this bullet
29652978
// would let the two agents grade the same inert test differently on one PR.
29662979
expect(buildRoleBrief(PLAN, 'test-matrix')).toContain(
29672980
'a **Suggestion** on its own, Critical only when',
29682981
);
2982+
// And the witness discipline must move in lockstep too — test-matrix is the
2983+
// same reading-based mutation analysis, so it carries the same bar on
2984+
// execution-grade phrasing.
2985+
expect(buildRoleBrief(PLAN, 'test-matrix')).toContain('witness: not run —');
2986+
expect(buildRoleBrief(PLAN, 'test-matrix')).toContain('ships N/N green');
2987+
expect(buildRoleBrief(PLAN, 'test-matrix')).toContain('verified N/N green');
2988+
expect(buildRoleBrief(PLAN, 'test-matrix')).toContain(
2989+
'phrase an unrun mutation as a reasoned hypothesis',
2990+
);
2991+
expect(buildRoleBrief(PLAN, 'test-matrix')).not.toContain(
2992+
'you have no runner',
2993+
);
29692994
});
29702995

29712996
it('gives the verifier the probe capability — run a claim, self-check the probe, tag [probe]', () => {

packages/cli/src/commands/review/lib/agent-briefs.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,9 @@ Not your dimension: whether the code already exists elsewhere (3a) or whether th
492492
493493
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.
494494
495-
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.`,
495+
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 considered 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.
496+
497+
**An unrun mutation is a hypothesis — never write it as an executed result.** Executed mutation verdicts belong to Agent 7's efficacy probe; your analysis is reading-based. A mutation you only reasoned about reads "would likely survive, because <the assertion moves with the code>" — never "the mutation ships N/N green" or "mutant verified N/N green", which assert a run you did not do and a maintainer reads as observed. When a finding's weight depends on a run you did not do, carry \`witness: not run — <why>\` in the body, exactly as the verifier does for an unreachable claim.`,
496498
},
497499

498500
'6a': {
@@ -593,7 +595,7 @@ Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`,
593595
- **Map each behavioural change in the production code to the test that exercises it**, wherever that test lives.
594596
- **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.
595597
- 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.
596-
- **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).`,
598+
- **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). Like Agent 5, your mutant analysis is reading-based: phrase an unrun mutation as a reasoned hypothesis ("would likely stay green because …"), never as an executed result ("the mutation ships N/N green" or "mutant verified N/N green"), and carry \`witness: not run — <why>\` when the claim needs a run you did not do.`,
597599
},
598600

599601
'invariant-a': {

0 commit comments

Comments
 (0)