feat(review): mutation-test the tests in the test-coverage pass (Agent 5) - #7735
Merged
Conversation
…t 5) 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.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 26, 2026 01:29
Collaborator
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@qwen-code /triage |
yiliang114
approved these changes
Jul 26, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Approve ✅
Good addition to the review skill's test-coverage pass. The mutation-testing discipline catches a real gap: tests that exist but would pass whether the code is right or wrong (expect(undefined).toBe(undefined), round-trip tautologies, single-site assertions). The severity guidance is correct — vacuous test alone is Suggestion, vacuous test as sole guard for a changed behaviour is Critical.
qwen-code-ci-bot
requested changes
Jul 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Thanks @wenshao — the change itself reads well, and syncing the SKILL.md dimension table to match Agent 5's brief is exactly the right move.
Before I can take this further, the PR body needs to follow the PR template. Right now it uses free-form headings (Summary / What changed / Testing), but the template's required sections are missing:
- What this PR does and Why it's needed — the current
Summary/What changedcover the "what", but there's no separate "why" section. - Reviewer Test Plan —
How to verify,Evidence (Before & After), and theTested ontable. For a prompt-copy change this is largely N/A, but the section still needs to be present. - Risk & Scope
- Linked Issues
Could you restructure the body under those headings? The content you've written maps onto them cleanly, so this should be a quick reformat rather than new work. Once that's in place I'll run the full review. 🙏
中文说明
感谢 @wenshao —— 改动本身写得很好,把 SKILL.md 维度表同步成 Agent 5 brief 的描述也是完全正确的做法。
在继续深入之前,PR 正文需要先遵循 PR 模板。目前用的是自由标题(Summary / What changed / Testing),但模板要求的章节缺失:
- What this PR does 和 Why it's needed —— 现在的
Summary/What changed覆盖了"做了什么",但没有单独的"为什么"章节。 - Reviewer Test Plan ——
How to verify、Evidence (Before & After)以及Tested on表格。对纯 prompt 文案改动大多是 N/A,但章节仍需要存在。 - Risk & Scope
- Linked Issues
能否按这些标题重组正文?你写的内容可以很干净地对应上去,应该只是快速重排而非新写内容。完成后我会跑完整审查。🙏
— Qwen Code · qwen3.8-max-preview
Collaborator
Author
|
Reformatted the body under the template headings — What this PR does / Why it's needed / Reviewer Test Plan (How to verify · Evidence, N/A for a prompt change · Tested on) / Risk & Scope / Linked Issues. The content mapped over cleanly as you noted, no new work. Ready for the full review 🙏 |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR — and for the quick body reformat, the template is all there now ✓ Problem: Real, not theoretical. Agent 5 today checks that tests exist and cover the diff's paths, but not that they'd actually fail if the code broke. The PR cites an observed case — a parity test whose two sides both resolved to Direction: Aligned. This is a refinement to the Size: One core path is touched — Approach: The scope is exactly right — a mutation-testing paragraph for Agent 5, a mirrored bullet for the Moving on to code review. 🔍 中文说明感谢贡献，也感谢快速重排了 PR 正文——模板现在完整了 ✓ 问题： 真实存在，而非理论性的。Agent 5 目前只检查测试是否存在、是否覆盖 diff 的路径，但不检查"代码被改坏时测试会不会失败"。PR 引用了一个观测到的案例：一个一致性测试两边都解析成了 方向： 对齐。这是对 规模： 触及一个核心路径—— 方案： 范围恰到好处——给 Agent 5 一段变异测试说明、给负责"行为↔测试"配对的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewI proposed my own take first: add a paragraph telling Agent 5 to name the one-line mutation that should turn each new/changed test red, flag tests that stay green under any plausible mutation as vacuous, and escalate to Critical when such a test is the sole guard for a behaviour the diff changes — then mirror it in the The prompt text is well-formed (template-literal backticks escaped correctly), matches the surrounding voice, and the severity guidance is consistent with the brief's existing pattern. The SKILL.md row accurately summarizes the new behaviour within the table cell. No correctness, security, or convention concerns — and nothing beyond the minimal set the goal needs. No blockers. Test evidenceThis is review-skill prompt text with no runtime or TUI surface, so the oracle is the agent-prompt unit suite plus CI. I did not run the PR's code (static review only); the signal below is the PR's own CI on the reviewed commit.
The Linux unit suite (which includes 中文说明代码审查我先独立给出自己的方案：加一段说明，让 Agent 5 对每个新增/改动的测试说出"理应让它变红"的一行变异，把在任何合理变异下都保持绿色的测试判为空转，并在该测试是被改动行为的唯一守卫时升级为 Critical——然后在 prompt 文案格式正确（模板字符串里的反引号正确转义），与周边语气一致，严重度指引与 brief 既有模式协调。SKILL.md 那一行在表格单元内准确概括了新行为。无正确性、安全或规范问题——也没有超出目标所需的最小集合。 无阻塞项。 测试证据这是审查技能的 prompt 文案，无运行时或 TUI 界面，所以判据是 agent-prompt 单元套件加 CI。我没有运行 PR 的代码（仅静态审查）；下方信号来自 PR 自身在被审 commit 上的 CI。 Linux 单元套件（含 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; minimal, well-motivated, and already carrying a maintainer approval. This is exactly the kind of change the review skill should absorb: a real gap (Agent 5 blessed tests that exist but wouldn't catch the bug), a fix calibrated to avoid over-flagging, and nothing beyond the minimal set. The mutation-testing paragraph reads like the maintainers' existing manual discipline written down, the The only thing unit tests can't prove is the real-model effect, which rests on the author's validation claim — but the severity guardrails (Suggestion alone, Critical only as sole guard) are conservative, and there's no code path here to regress. If I were maintaining this in six months I'd thank the author, not curse them. LGTM, approving. ✅ 中文说明置信度：5/5 —— 每个阶段都干净；改动最小、动机充分，且已有一位维护者批准。 这正是审查技能应当吸收的那类改动：一个真实缺口（Agent 5 为"存在但抓不住 bug"的测试背书），一个校准过以避免误报的修复，且没有任何超出最小集合的东西。变异测试那段读起来像是把维护者既有的人工纪律写了下来， 单元测试唯一无法证明的是真实模型效果，这取决于作者的验证声明——但严重度护栏（单独看是 Suggestion，仅作为唯一守卫时才是 Critical）是保守的，这里也没有可回归的代码路径。若六个月后由我维护，我会感谢作者，而不是埋怨。 LGTM，批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
reviewed
Jul 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
One post-merge severity inconsistency found on the exact merged head.
doudouOUC
approved these changes
Jul 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
pull Bot
pushed a commit
to Stars1233/qwen-code
that referenced
this pull request
Jul 26, 2026
…ollow-up to QwenLM#7735/QwenLM#7736) (QwenLM#7746) * fix(review): correct and tighten the borrowed lenses per review Four findings on the lenses this PR adds: - **Agent 2 (subprocess injection), Critical.** The guidance said "terminate the argv with `--`", but `--` ends *option* parsing without neutralizing a *pathspec* — for an overloaded command it creates one (`git checkout -- release` restores a path instead of switching branch; `git checkout -- .` still discards changes). Reword: validate against the subcommand grammar; a `--` helps only where it keeps the operand's role, and the value allowlist is what closes it. - **Agent 1b (changed literal).** A default ripgrep skips hidden `.github/**`, so a marker consumed only by a workflow reads as "no consumer". Require a hidden-path search (`rg --hidden --glob '!.git/**' --fixed-strings`). - **Agent 4 (unreproducible benchmark).** "Flag as unverified" conflicts with the actionable-findings-only contract. Make it actionable when load-bearing (request the script/env/raw numbers) or no finding when incidental (record under not-verified), never a non-defect finding. - **Agent 5 (equivalent mutant).** Add a focused `buildRoleBrief(PLAN, '5')` test pinning the equivalent-mutant rule and its discriminating-input requirement, so a prompt-assembly regression cannot silently drop it. * fix(review): a vacuous test is a Suggestion, not a Critical for being the sole guard Agent 5's mutation lens graded a sole-guard vacuous test as Critical, but the shared severity ladder — and Agent 7's deterministic efficacy probe — grade an ineffective test as Suggestion. Step 4 keeps the higher severity, so the same inert guard arrived as Critical from Agent 5 and Suggestion from Agent 7, and the Critical won: a PR could be blocked solely for lacking an *effective* test, the exact inflation those shared rules exist to prevent. Align it with the dimension's own "name the bug, not the gap" rule: a vacuous test is a Suggestion, escalated to Critical only when it asserts the opposite of the intended behaviour, was weakened in-diff, or lets a specific incorrect behaviour ship (in which case that behaviour is the Critical, with the test as evidence). Mirrored in the test-matrix brief and the SKILL.md dimension table, and pinned by a buildRoleBrief(PLAN, '5') assertion so the semantic reversal cannot pass the generic word-presence check again. * fix(review): pin the Agent 2 and test-matrix brief corrections, sync the SKILL table --------- Co-authored-by: verify <verify@local>
Collaborator
|
Released in v0.21.1. |
Collaborator
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a mutation-testing discipline to Agent 5 — the
/reviewskill's test-coverage pass — and its whole-difftest-matrixcounterpart. The agent briefs are CLI-generated, so the behavioural change lives inpackages/cli/src/commands/review/lib/agent-briefs.ts; theSKILL.mddimension table's Agent 5 row is synced so the human-facing description matches the brief.Agent 5's brief now, for any test this diff adds or changes to pin a value or behaviour, asks it to name the one-line mutation to the code under that test which should turn it red — and if no plausible mutation does, the test is vacuous. It calls out the recurring shapes: both sides of the assertion computed the same way so they move together (
expect(undefined).toBe(undefined)from a loose or unanchored extraction — pin the literal instead); reading only the first of several sites the changed behaviour spans; anexpect(x).toBe(x)tautology; and a "does not throw" assertion for a bug whose symptom would be 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.Why it's needed
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. This is a discipline maintainers already apply by hand in review — a recent human review caught exactly this: a parity test whose two sides both resolved to
undefined, soexpect(undefined).toBe(undefined)passed green under a two-sided rename. Agent 5 should catch that class too.Reviewer Test Plan
How to verify
This is a prompt-copy change to CLI-generated agent briefs, so the oracle is the agent-prompt suite plus the printed brief itself. Run
npx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(142 tests, green) andnode scripts/lint.js --eslint+prettier. No test pins the brief prose, so the copy change lands without touching any assertion. To see the behaviour directly, runqwen review agent-prompt --role 5 --plan <plan>and confirm the printed brief now carries the mutation-testing paragraph and thetest-matrixbrief its counterpart.Evidence (Before & After)
N/A — no user-visible or TUI change; this is review-skill prompt text.
Tested on
Environment (optional)
Unit tests only (
vitest); the change is platform-independent prompt text, so no runtime or sandbox is involved.Risk & Scope
Linked Issues
None — this refines the review skill's own heuristics; there is no tracking issue.
中文说明
这个 PR 做了什么
给 Agent 5（
/review技能的测试覆盖维度）及其全 diff 的test-matrix对应体加上变异测试（mutation testing）纪律。agent 的 brief 是 CLI 生成的,所以行为改动在packages/cli/src/commands/review/lib/agent-briefs.ts;SKILL.md维度表里 Agent 5 那行同步,使人类可读的描述与 brief 一致。Agent 5 的 brief 现在要求:对本 diff 新增或改动、用于钉住某个值或行为的测试,说出一个应让它变红的、对被测代码的一行变异——若没有任何合理变异能让它红,该测试就是空转的。它列出要盯的常见形态:断言两边用同一种方式计算、因而一起移动(松散或未锚定的提取导致
expect(undefined).toBe(undefined)——应改为钉住字面值);只读了被改动行为跨越的多个位点中的第一个;expect(x).toBe(x)恒真式;以及对症状本应是"错误值"的 bug 却用"不抛异常"来断言。空转测试单独看是 Suggestion,但当它是本 diff 改动的某个行为的唯一守卫时,是 Critical。为什么需要
Agent 5 原本检查测试是否存在、是否覆盖 diff 的路径,但没有检查"代码被改坏时测试会不会失败"。一个无论代码怎样都保持绿色的测试,比没有测试更糟:它看起来像覆盖,却为它本该抓住的那个回归背书。这套纪律维护者在人工审查里已经在用——最近一次人工审查恰好抓到这类:一个一致性测试的两边都解析成了
undefined,于是在两边一起改名后expect(undefined).toBe(undefined)照样绿。Agent 5 也该抓到这一类。Reviewer 测试计划
如何验证
这是对 CLI 生成的 agent brief 的纯文案改动,所以判据是 agent-prompt 测试套件加上打印出来的 brief 本身。运行
npx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(142 个测试,全绿)以及node scripts/lint.js --eslint+prettier。没有任何测试固定 brief 文案,故文案改动无需改动任何断言即可落地。若想直接看行为,运行qwen review agent-prompt --role 5 --plan <plan>,确认打印出的 brief 现在带上了变异测试那一段,test-matrixbrief 也带上了对应内容。证据(修改前后）
N/A —— 无用户可见或 TUI 变化;这是审查技能的 prompt 文案。
已测试平台
环境（可选）
仅单元测试(
vitest);改动是平台无关的 prompt 文案,不涉及任何运行时或沙箱。风险与范围
关联 Issue
无 —— 本 PR 打磨的是审查技能自身的启发式规则,没有对应的跟踪 issue。