feat(review): borrow maintainer review lenses into the agent briefs - #7736
Merged
Conversation
added 2 commits
July 26, 2026 09:28
…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.
…erf-repro lenses 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.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 26, 2026 02:20
Collaborator
Collaborator
|
Thanks for the PR! Re-run on new commits (head moved from Template: still uses Problem: observed, not theoretical. The contract-shape lens cites a real Critical on #7723 (a triage marker rename that broke an autofix bot-comment filter). The migration/split-brain, equivalent-mutant, and perf-repro lenses address review gaps maintainers already handle by hand. The two new additions since the last review — subprocess argument injection (Agent 2) and sibling consistency (Agent 3) — are the same class: concrete failure patterns a maintainer would catch but the briefs didn't ask for. Direction: aligned. Review-quality improvements to the agent brief system, squarely within the review tooling's mission. No auth/sandbox/model/telemetry/public-contract surface touched. Size: Approach: the scope is tight — bullet-point additions to six existing briefs (1b ×2, 2, 3, 4, 5), plus the matching SKILL.md dimension-table update. No new code, no control-flow change, no new abstractions. One note: the PR body describes four lenses but the diff now carries six — the Agent 2 (subprocess argument injection) and Agent 3 (sibling consistency) additions aren't mentioned in the body. Worth updating the description so a reviewer doesn't have to diff-hunt for the extra two. Moving on to code review. 🔍 中文说明感谢贡献！在新提交上重新运行（head 从 模板： 仍使用 问题： 已观测到的，非理论性的。契约形状检查引用了 #7723 的真实 Critical。迁移/脑裂、等价变异和性能复现检查解决了维护者已在手工处理的审查盲区。上次审查后新增的两条——子进程参数注入（Agent 2）和兄弟一致性（Agent 3）——属于同一类别：维护者能发现但 brief 未要求的具体失败模式。 方向： 对齐。对 agent brief 系统的审查质量改进，完全在审查工具职责范围内。 规模： 核心路径生产行数共 43 行（全部为提示文本/文档表格，无逻辑）。远低于任何阈值。 方案： 范围紧凑——向六个现有 brief 添加要点（1b ×2、2、3、4、5），加上对应的 SKILL.md 维度表更新。无新代码、无控制流变更、无新抽象。一个备注：PR 正文描述了四条检查，但 diff 现在包含六条——Agent 2（子进程参数注入）和 Agent 3（兄弟一致性）未在正文中提及。建议更新描述，以免审查者需要自行在 diff 中寻找额外的两条。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code ReviewIndependent proposal: given the goal (wire maintainer-applied review disciplines into the agent briefs), I would add bullet-point paragraphs to the relevant briefs in Comparison with the diff: the PR does exactly this. The approach matches my independent proposal — no simpler path missed. Findings: none blocking. All six additions are well-written, specific, and actionable. Each names the failure mode, the check to perform, and the concrete regression it prevents. A few highlights:
The SKILL.md table updates for roles 1b, 2, 3, and 4 accurately summarize the new brief content. Role 5's table text is unchanged — the equivalent-mutant guard refines the existing mutation-testing dimension rather than adding a new one, so the table staying as-is is reasonable. The column-width reformatting across all rows is expected markdown table behavior. One note (non-blocking): the PR body describes four lenses but the diff carries six. The Agent 2 and Agent 3 additions are the same kind of change as the other four, so this is a description gap rather than a scope concern — but a reviewer reading the body first would miss two of the six additions. TestingThis is an unattended CI run (
Ubuntu unit tests pass. macOS/Windows tests and integration tests are skipped (likely matrix configuration). The web-shell E2E smoke test is still running — unrelated to this PR's prompt-text changes. Real-scenario testing: N/A — prompt-text-only change with no user-visible behavioral surface. 中文说明代码审查独立方案： 给定目标（将维护者手工审查纪律接入 agent brief），我会向 与 diff 对比： PR 完全这样做了。方案与独立提案一致——没有遗漏更简路径。 发现：无阻断项。 六条新增内容均写得具体且可操作。每条都命名了失败模式、检查步骤、以及防止的具体回归。几个亮点：契约形状要点给出了超越现有导出重命名检查的两个具体搜索模式；子进程参数注入是真正的安全视角—— SKILL.md 表格更新准确概括了新 brief 内容。角色 5 表格未变——等价变异守卫是对现有维度的细化。列宽重排是预期的 markdown 行为。 一个备注（非阻断）：PR 正文描述了四条检查但 diff 包含六条。Agent 2 和 Agent 3 与其他四条属于同类变更，因此这是描述缺口而非范围问题。 测试这是无人值守的 CI 运行（ 真实场景测试：不适用——仅提示文本变更，无用户可见行为面。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal, well-motivated brief additions; nits are the PR body lagging the diff (4 lenses described, 6 shipped) and non-standard template headings. This PR does one thing and does it well: six review disciplines that maintainers apply by hand, written into the agent briefs so every review gets them automatically. The brief text is specific and actionable — each lens names the failure mode, the check, and the concrete regression it prevents. The #7723 incident (marker rename breaking a bot-comment filter) is real evidence for the contract-shape lens, and the subprocess argument injection lens names a real asymmetry ( The diff is 43 lines of prompt text across two files, with no logic, no imports, no exports, and no control-flow change. My independent proposal matched the PR's approach exactly — I didn't find a simpler path it missed. The SKILL.md table updates accurately track the brief changes. Non-blocking nits: (1) the PR body describes four lenses but the diff carries six — the Agent 2 (subprocess argument injection) and Agent 3 (sibling consistency) additions aren't mentioned; worth updating the description; (2) the PR body uses non-standard headings vs the repo template. Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、精简、动机充分的 brief 补充；瑕疵为 PR 正文落后于 diff（描述四条、实际六条）和非标准模板标题。 这个 PR 只做一件事且做得好：将维护者手工应用的六条审查纪律写入 agent brief，让每次审查都自动带上。brief 文本具体且可操作——每条都命名了失败模式、检查步骤、以及防止的具体回归。#7723 事件是契约形状检查的真实证据，子进程参数注入检查命名了审查者经常忽略的真实不对称性。 diff 是两个文件共 43 行提示文本，无逻辑、无导入、无导出、无控制流变更。独立方案与 PR 方案完全一致。SKILL.md 表格更新准确跟踪了 brief 变更。 非阻断瑕疵：(1) PR 正文描述四条但 diff 包含六条——Agent 2 和 Agent 3 未提及，建议更新描述；(2) PR 正文使用非标准标题。 批准延迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
previously 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. ✅
…ty pass 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=<path>` 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.
wenshao
changed the title
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.
wenshao
changed the title
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
doudouOUC
reviewed
Jul 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
# Conflicts: # packages/cli/src/commands/review/lib/agent-briefs.ts # packages/core/src/skills/bundled/review/SKILL.md
wenshao
force-pushed
the
feat/review-brief-lenses
branch
from
July 26, 2026 08:42
f4a6460 to
b49c772
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
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 |
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
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.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
yiliang114
approved these changes
Jul 26, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Approve ✅
Four well-chosen maintainer review lenses wired into the agent briefs:
- Agent 1b — changed literals (markers, keys, codes) are contracts too; rename/schema changes must handle existing data (migration/split-brain)
- Agent 2 — argument/option injection into subprocess calls (
execFiledoes NOT prevent-flaginjection,--terminator needed) - Agent 3 — sibling consistency: guard one parallel path has that its twin lacks (asymmetric failure → security bug if on untrusted input)
- Agent 4 — don't trust PR's performance claims; reproduce deterministic checks, flag benchmarks as unverified
- Agent 5 — equivalent mutant note: don't call a test vacuous if the mutation is unobservable by construction
Brief-only changes, no control-flow impact. SKILL.md dimension table updated to match.
Collaborator
Author
|
Heads-up: this PR squash-merged at |
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>
wenshao
pushed a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 26, 2026
Resolve compose-review.test.ts by keeping both the deterministic script-lint gate tests and main's bilingual-recovery block. compose-review.ts / roster.ts / SKILL.md auto-merged (main's QwenLM#7733/QwenLM#7736/QwenLM#7739 work + this branch's gate).
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
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.
Summary
Four review disciplines a maintainer already applies by hand, wired into the agent briefs (which are CLI-generated) so every review gets them. Briefs only — no control-flow change.
What changed (
lib/agent-briefs.ts+ SKILL.md dimension table)includes/startsWithkeys on), grep the old shape, not the symbol — a rename that compiles can silently stop matching a filter in another file (or CI workflow), and nothing in the build flags it. (This exact class was a real Critical on fix(ci): rename triage status marker to avoid duplicate-guard collision #7723: a triage marker rename that broke an autofix bot-comment filter.)Testing
agent-promptsuite green (142);bundled-skills.integration.test.tsgreen;eslint --max-warnings 0+prettierclean. No test pins brief prose.中文说明
概述
四条维护者已在人工审查里使用的技法,接进 agent 的 brief（CLI 生成）,让每次审查都带上。仅 brief,无控制流改动。
改动内容（
lib/agent-briefs.ts+ SKILL 维度表）测试
agent-prompt套件全绿（142）；bundled-skills.integration.test.ts全绿；eslint --max-warnings 0+prettier干净。无测试固定 brief 文案。