fix(review): correct the borrowed lenses and vacuous-test severity (follow-up to #7735/#7736) - #7746
Conversation
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.
… 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.
|
Thanks for the follow-up! Template looks good ✓ Problem: observed and verifiable. The five corrections were made and marked fixed on #7735/#7736, but both PRs squash-merged before the fix commits landed — main still carries the un-corrected lenses (confirmed by reading the current source). This isn't theoretical; the old Agent 2 text literally advises "terminate the argv with Direction: aligned. These are internal review-skill prompt corrections — no user-facing runtime change, no product direction question. The severity de-inflation for vacuous tests matches the shared ladder and Agent 7's efficacy probe, which is the right call. Size: Approach: scope is tight — five targeted text corrections plus two focused tests that pin the corrected semantics. Every edit maps to a stated finding. No drive-by changes, nothing to split. Moving on to code review. 🔍 中文说明感谢跟进！ 模板完整 ✓ 问题：已观测且可验证。五个修正在 #7735/#7736 上已做并标记为已修复，但两个 PR 在修复 commit 之前 squash 合并了——main 上仍是未修正的 lens（已通过阅读当前源码确认）。这不是理论问题：旧的 Agent 2 文案确实写着"用 方向：对齐。这些是内部审查技能的 prompt 修正——无用户可见的运行时变化，无产品方向问题。空转测试的严重度去抬高与共享阶梯及 Agent 7 的效力探针一致，是正确的做法。 规模： 方案：范围紧凑——五个定向文案修正加两个聚焦测试，钉住修正后的语义。每处改动都对应一个已陈述的 finding。无顺手改动，无需拆分。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given that #7735/#7736 merged before the fix commits, I'd apply the five specific text corrections to Findings: none. Walked all three files:
No correctness bugs, no convention violations, no scope creep. TestingN/A — prompt-text and test-only changes with no user-visible or TUI behaviour.
All completed checks green; no failures. macOS/Windows tests and integration tests skipped (fork PR). 中文说明代码审查独立方案： 鉴于 #7735/#7736 在修复 commit 之前合并，我会对 发现：无。 逐文件审查：
无正确性问题，无规范违反，无范围蔓延。 测试N/A——纯 prompt 文案和测试改动，无用户可见或 TUI 行为变化。 CI 全部完成且通过，无失败。macOS/Windows 测试和集成测试已跳过（fork PR）。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. The problem is real and verifiable on main — the old Agent 2 text still advises 中文说明置信度：5/5——每个阶段都干净，毫不犹豫可以合并。 问题真实且可在 main 上验证——旧的 Agent 2 文案仍把 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Independent local verification report (Linux, real bundled CLI)Verdict: all five corrections verified in the briefs the real binary actually renders. One Suggestion: the SKILL.md dimension table's row MethodBuilt this branch ( 1. Baseline
2. Rendered-brief assertions (patched binary, per role)
The severity chain the fix reasons about checks out: Agent 5 and Agent 7's efficacy probe now grade the same inert test identically (Suggestion), so Step 4's max-severity can no longer inflate it into a blocker; the two real escalation triggers (opposite assertion, weakened in-diff) were already Critical under the existing rule and remain so. 3. Suggestion (non-blocking): stale
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 2 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #7746All three feedback points were implemented. One commit: Feedback triage1. [Suggestion] Pin the Agent 2 subprocess-injection correction ( Valid. This PR corrects Agent 2's brief so that 2. [Suggestion] Pin the test-matrix severity update ( Valid. The same vacuous-test severity regrade was applied to the test-matrix agent ( 3. [Suggestion] Stale Valid. The PR updated the table's row Changes
Conflict notes
Verification
中文说明Autofix 审查轮次 — PR #7746三条反馈全部已实现。一次提交： 反馈分类1. [Suggestion] 钉住 Agent 2 子进程注入修正（ 有效。本 PR 修正了 Agent 2 的 brief，将 2. [Suggestion] 钉住 test-matrix 严重度更新（ 有效。同样的空转测试严重度调整也应用到了 test-matrix agent（ 3. [Suggestion] SKILL.md 维度表第 有效。本 PR 更新了表格第 改动
冲突说明
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Follow-up verification (new head
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action requiredAll feedback on this round is informational or confirmatory:
No inline comments, no failed checks, and no still-red checks. No changes made. 中文说明无需操作本轮所有反馈均为信息性或确认性的：
无行内评论、无失败检查、无持续红灯检查。未做任何更改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
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
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review Summary
This PR correctly addresses 5 review findings that were missed when #7735 and #7736 were squash-merged before the fix commits could land. The changes are well-justified and the test additions provide strong regression protection.
✅ Correctness
Agent 2 (Subprocess Injection Fix) — CORRECT
The original guidance "terminate the argv with --" was indeed incomplete:
--ends option parsing but does not neutralize a pathspecgit checkout -- .still discards unstaged changes (pathspec behavior)git checkout -- releaserestores a file path instead of switching branches
The fix correctly changes the guidance to be command-aware: validate against the subcommand grammar, use -- only where it preserves operand role, and emphasize that the value allowlist is what actually closes the injection. This is a substantive security correctness fix.
Agent 5 (Vacuous Test Severity Fix) — CORRECT
The severity alignment is correct:
- Original: "sole-guard vacuous test = Critical" — this inflates severity
- Agent 7's efficacy probe grades ineffective tests as Suggestion
- Step 4 takes max severity → inflation to merge-blocker
The fix correctly aligns with the shared severity ladder:
- Vacuous test = Suggestion (baseline)
- Escalate to Critical only when: asserts opposite behavior, weakened in-diff, or lets specific incorrect behavior ship
This prevents the inflation the shared ladder was designed to avoid.
Agent 1b (Hidden Path Search) — CORRECT
Adding rg --hidden --glob '!.git/**' is the right fix. Default ripgrep skips .github/**, so workflow-only consumers (markers, sentinel strings) would appear as "no consumer" without this. Good catch.
Agent 4 (Unreproducible Benchmark) — CORRECT
The change from "flag as unverified" (which violates actionable-findings-only contract) to:
- Load-bearing numbers: request script/env/raw data
- Incidental numbers: no finding, record under not-verified
This maintains contract integrity while still providing useful guidance.
✅ Test Coverage
The two new tests are excellent:
-
mutation-testing lens into Agent 5test:- Verifies equivalent-mutant paragraph reaches Agent 5
- Checks discriminating-input requirement
- Confirms no bleed into sibling (Agent 2)
- Pins severity alignment (Suggestion baseline, Critical only with named behavior)
- Also verifies test-matrix agent stays in sync
-
command-aware subprocess-injection correction into Agent 2test:- Verifies the
--correction is present - Confirms old incorrect guidance is removed
- Protects against semantic reverts
- Verifies the
These tests go beyond "word presence" checks and pin semantic meaning, preventing prompt-assembly regressions or semantic reversions from passing silently.
✅ Code Quality
- Changes are minimal and targeted (3 files, +61/-19)
- The briefs are well-written with concrete examples
- SKILL.md table stays synchronized with agent-briefs.ts
- Test comments clearly explain the "why" behind each assertion
⚠️ Minor Observations (Non-blocking)
-
Test file location: The test is in
agent-prompt.test.tswhich is appropriate for prompt assembly testing. -
No runtime change: This is purely prompt text, so the risk profile is very low. The severity change makes Agent 5 less aggressive on vacuous tests, which is the intended behavior.
Recommendation
Approve ✓
This PR fixes genuine correctness issues in the review agent lenses:
- A security guidance bug that gave incomplete injection mitigation advice
- A severity inflation that could block PRs for lacking effective tests
- Hidden path search omission that missed workflow consumers
- Contract conflict in benchmark verification
The tests are well-designed and will prevent regressions. The changes align with the stated severity ladder and improve the accuracy of the review skill.
Good follow-up to the original review comments!
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.1. |
|
|
6 participants
What this PR does
Applies five review findings on the Agent-5/1b/2/4 lenses that #7735 and #7736 merged before the fixes could land:
--", but--ends option parsing without neutralizing a pathspec — for an overloaded command it creates one (git checkout -- releaserestores a path instead of switching branch;git checkout -- .still discards changes). Reworded to be command-aware: validate against the subcommand grammar, and use--only where it keeps the operand's role — the value allowlist is what closes the injection.test-matrixbrief and the SKILL dimension table..github/**, so a marker consumed only by a workflow reads as "no consumer". The lens now requires a hidden-path search (rg --hidden --glob '!.git/**' --fixed-strings).buildRoleBrief(PLAN, '5')test pins the equivalent-mutant rule and the corrected severity, so a prompt-assembly regression or a semantic reversal reddens instead of passing the generic word-presence check.Why it's needed
These are confirmed review findings (one Critical each from #7735 and #7736) that were replied to and marked fixed, but the two PRs squash-merged at a commit before the fix commits, so main carries the un-corrected lenses. Without this, Agent 2 gives injection guidance that
--does not actually satisfy, and Agent 5 inflates an ineffective-test finding into a Critical that can block a PR for lacking an effective test — the exact inflation the shared severity rules exist to prevent.Reviewer Test Plan
How to verify
Prompt-copy changes to CLI-generated agent briefs plus one focused test. Run
npx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(144, green) andnode scripts/lint.js --eslint. To see the behaviour,qwen review agent-prompt --role 5 --plan <plan>— the printed brief now says a vacuous test is a Suggestion and carries the equivalent-mutant rule;--role 2carries the command-aware--/pathspec caveat.Evidence (Before & After)
N/A — no user-visible or TUI change; review-skill prompt text.
Tested on
Environment (optional)
Unit tests only (
vitest); platform-independent prompt text.Risk & Scope
Linked Issues
Follow-up to #7735 and #7736 (review comments addressed there; the fixes missed those merges).
中文说明
这个 PR 做了什么
补上 #7735、#7736 合并时未能一起落地的五个针对 Agent 5/1b/2/4 lens 的评审修复:
--终止 argv",但--只结束选项解析,并不中和pathspec——对重载命令反而会制造一个(git checkout -- release是恢复路径而非切分支;git checkout -- .仍丢弃改动)。改为命令感知:按子命令语法校验值,--仅在保持操作数角色时才用——真正堵住注入的是值白名单。test-matrixbrief 与 SKILL 维度表同步。.github/**,于是只被某工作流消费的标记会显示为"无消费者"。lens 现在要求隐藏路径搜索(rg --hidden --glob '!.git/**' --fixed-strings)。buildRoleBrief(PLAN, '5')测试,钉住等价变异规则与修正后的严重度,使 prompt 组装回归或语义反转会变红,而不是通过泛化的"词是否出现"检查。为什么需要
这些是已确认的评审 finding(#7735、#7736 各一个 Critical),已回复并标记为已修复,但那两个 PR 在修复 commit 之前 squash 合并了,因此 main 上是未修正的 lens。不修的话,Agent 2 给出的注入指引其实并不能靠
--满足,Agent 5 会把无效测试 finding 抬成 Critical、仅因缺少有效测试就阻断 PR——正是共享严重度规则要防止的抬高。Reviewer 测试计划
如何验证
对 CLI 生成的 agent brief 的纯文案改动,外加一个聚焦测试。运行
npx vitest run packages/cli/src/commands/review/agent-prompt.test.ts(144,全绿)与node scripts/lint.js --eslint。想看行为:qwen review agent-prompt --role 5 --plan <plan>—— 打印的 brief 现在说空转测试是 Suggestion,并带等价变异规则;--role 2带命令感知的--/pathspec 说明。证据(修改前后)
N/A —— 无用户可见或 TUI 变化;审查技能 prompt 文案。
已测试平台
环境(可选)
仅单元测试(
vitest);平台无关的 prompt 文案。风险与范围
关联 Issue
#7735、#7736 的后续(评审意见在那两处,但修复没赶上它们的合并)。