feat(review): bake the round label into a findings role's identity line and key - #7171
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: Observed bug with evidence. Dogfooded on a real 3A review right after #7150 merged — two same-findings reverse-audit rounds shared one record key, the orchestrator hand-appended Direction: Aligned. The core insight — "what the caller will reach for, the CLI should print" — is consistent with how this review skill has evolved across multiple PRs. When the CLI leaves something out of the printed prompt, the orchestrator writes it in by hand, and hand-writing is exactly what the delivery check exists to refuse. Adding Size: Core path touched via Approach: Scope feels right. One flag, validated at the same boundary as Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:有复现的实际 bug。 在 #7150 合入后真实 3A review 上 dogfood 发现——两轮 findings 相同的反审计共享一条 record key,编排器手工追加 方向:对齐。核心洞察——"调用者会去够的东西,应该由 CLI 打印"——与本 review skill 跨多个 PR 的演进方向一致。CLI 没打进 prompt 的东西,编排器就会手写进去,而手写正是交付检查要拒绝的。新增 规模:核心路径仅通过 方案:范围合理。一个 flag,在与 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewClean. I read the diff against my own mental proposal for the problem — I would have done the same thing: add No correctness bugs, no security holes, no regressions found. The key construction is consistent — The SKILL.md changes are documentation only — updating the Step 4/5 command examples to include No critical findings. Real-Scenario TestingBuilt the bundled CLI from the PR branch and drove it with a synthetic plan report and findings file. Three runs, three separate records with different keys but the same findings digest — exactly the fix. Without Unit TestsAll 735 tests green across all 31 test files in the review command suite, including the new 中文说明代码审查干净。我对照自己对该问题的独立方案读了 diff——我会做同样的事:在 args 接口加 没有发现正确性 bug、安全漏洞或回归。key 构造一致——单构建为 SKILL.md 的改动仅限文档——更新 Step 4/5 命令示例加入 无 Critical 发现。 真实场景测试从 PR 分支构建 bundle,用合成的 plan report 和 findings 文件驱动。三轮运行产生三条独立 record(不同 key、相同 findings digest)——正是修复所在。不传 单元测试31 个测试文件 735 测试全绿,包括新增的 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — Clean across every stage; would merge without hesitation. This is a focused, well-motivated fix for a real failure the author hit in production: two same-findings rounds sharing one record key, the orchestrator compensating by hand-writing a round label into the identity line, and the delivery check rightfully rejecting the hand-written prompt. The fix is exactly what the skill's philosophy demands — "what the caller will reach for, the CLI should print." The implementation is minimal and correct. One flag, threaded through the right places (identity line, key construction, boundary validation, SKILL.md examples, repair templates). Every edit earns its place. The boundary guards are thorough — every combination that would silently drop 735 tests green, real-scenario testing confirms three separate records coexist for three rounds on the same findings, boundary guards fire correctly. No concerns, no nits. Ships it. ✅ 中文说明信心:5/5 — 每个阶段都干净,毫不犹豫合入。 这是一个聚焦、动机明确的修复,解决了作者在真实 review 中遇到的故障:两轮 findings 相同的 round 共享一个 record key,编排器被迫在身份行手写轮次标签,交付检查合理地拒绝了手写的 prompt。修复正是 skill 哲学所要求的——"调用者会去够的东西,应该由 CLI 打印。" 实现最小且正确。一个 flag,贯通正确的位置(身份行、key 构造、边界校验、SKILL.md 示例、修复模板)。每处编辑都有存在的理由。边界守卫充分——每种会静默丢弃 735 测试全绿,真实场景测试确认三轮在相同 findings 上产生三条独立 record,边界守卫正确触发。无顾虑,无瑕疵。合入。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Review — LGTM at b7fb02b ✅Two minor, non-blocking suggestions below. Correctness, boundary behavior, backward compatibility, and the "no delivery-side changes needed" claim were all verified against the head commit — statically and by live probe. Overview
What I verifiedCross-module consistency (the load-bearing claim) — all confirmed by reading the head tree:
Tests — Live probes (real yargs parse via
Findings
Neither blocks merge: (1) is stderr remediation prose, and (2)'s behavior is already exercised transitively by the chunk-shaped cousin. Quality notes
中文版评审 — LGTM at b7fb02b ✅两条不阻塞合并的小建议,见下文。正确性、边界行为、向后兼容以及"交付侧无需改动"的论断均已在 head commit 上静态阅读 + 实测验证。 验证内容跨模块一致性: 测试:隔离 worktree 中 实测探针(真实 yargs 解析,覆盖单测绕过的字符串强转路径): 发现
两条均不阻塞:(1) 是 stderr 修复提示文案;(2) 的行为已被 chunk 形态的同族测试间接覆盖。 |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Re-review (round 2) — LGTM at 37131de ✅One fast-forward commit since b7fb02b (no force-push), touching only Both round-1 suggestions are resolved
Verification at 37131de
Merge-ready from my side. 中文版复审(第二轮)— LGTM at 37131de ✅自 b7fb02b 起一个 fast-forward commit(无 force-push),只动了 两条建议均已解决
37131de 上的验证
我这边可以合并。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
Re: the This is the third sighting of the false alarm (route at #7150, seven names here), and the root cause is in our own presubmit classifier — it judges failure per RUN while judging "did it run" per NAME. Fixed in #7183: judge by the name's latest verdict, ordered by run timestamps. |
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v92 via Qwen Code /review
…ne and key Dogfooded on a 3A review: the reverse-audit loop ran two rounds on the same findings list, so both builds shared one digest, one key, one record. The orchestrator -- wanting to tell its own launches apart -- appended `(round N)` to the identity line, the exact line the delivery check anchors on. Both rounds were flagged rewritten, the verdict was capped, and the review paid a repair round for a label. What the caller will reach for, the CLI now prints: `--round <k>` puts `(round k)` inside the identity line and `--round-k` inside the record key, so two rounds are two launches, two briefs, two receipts -- even when the findings have not changed between them. Valid only for a findings role (declared via acceptsFindings, not hardcoded), 1-based integer, and ruled on at the primary-mode boundary like --all-chunks: a --round that reached --roster, --whole-diff, a bare --chunk or a roleless call used to be worth silently dropping, and a dropped round label is a record keyed as a launch the caller never made. The Step 5 skill text now passes --round per round instead of telling the orchestrator to track rounds "in your own notes", and the repair templates in coverage.ts carry [--round <k>] so a repair round rebuilt from the FIX line keeps its key shape.
…claiming --round bakes shards Review follow-through on the two round-1 suggestions: - The single-chunk rebuild (--role reverse-audit --chunk 14 --round 1 -- the exact call the FIX line prescribes after a gap) had no happy-path test: its key is concatenated at a different site than the batch's, so a swapped segment order there would break repair-round matching with every batch test green. Pinned key shape, identity label, and the chunk's own range; mutation-checked with exactly that swap. - rebuildFix appended "(--round bakes it in)" to both nouns, so a verify gap read "no hand-added shard number (--round bakes it in)" -- but --round bakes in a round number, and shards are told apart by their findings digest. The parenthetical is now reverse-audit-only, and the verify text is pinned against the conflation coming back.
37131de to
7f7b3e4
Compare
|
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)为单个提交。 |
…ftover
classifyCi already asks "did this name run" per NAME -- the routing
workflows re-dispatch a name several times per commit -- but it judged
failure per RUN: any single failing conclusion pushed its name into
failedCheckNames, including the cancelled runs those very re-dispatches
displace and the failed attempt a flaky job leaves behind when re-run to
green. Two real reviews were downgraded from Approve over exactly that
("CI failing: route" at #7150; route, review-pr, review-config and four
more at #7171), each on a commit whose every live check was green on the
PR page.
Failure is now judged by the name's latest verdict, ordered by the runs'
own timestamps -- the same evidence GitHub's PR page shows a human.
Skipped/neutral/stale runs stay non-verdicts, so a re-dispatch that
skipped cannot erase a real failure beside it; a name whose only run was
cancelled still fails, because nothing superseded it; and a re-run that
fails after a success fails, because latest wins in both directions. On
equal or absent timestamps the first-listed run keeps the name (the API
lists newest first).
Re-review (round 3) — LGTM at 7f7b3e4, rebase-only ✅This update is a rebase onto current main (picking up #7161/#7176/#7053/#7065/#7142/#7149/#7074/#7127/#7157) — no new PR content. Verified, not assumed:
Still merge-ready from my side; CI was mid-run at review time, with the classify/authorize jobs already green. 中文版复审(第三轮)— LGTM at 7f7b3e4,纯 rebase ✅本次更新是 rebase 到最新 main(带上 #7161/#7176/#7053/#7065/#7142/#7149/#7074/#7127/#7157),无新增 PR 内容。逐项验证:
我这边维持可合并;评审时 CI 尚在运行,classify/authorize 已绿。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
ytahdn
left a comment
There was a problem hiding this comment.
LGTM ✅
--round 将轮次标签烙入身份行和 record key,关掉编排器手写标签触发交付检查拒绝的失效族。双路径 key 一致、边界拒绝全面、acceptsFindings 守卫精确。测试覆盖充分(9/9 突变击杀),向后兼容。
…ftover (QwenLM#7183) * fix(review): judge a CI check by its name's latest run, not by any leftover classifyCi already asks "did this name run" per NAME -- the routing workflows re-dispatch a name several times per commit -- but it judged failure per RUN: any single failing conclusion pushed its name into failedCheckNames, including the cancelled runs those very re-dispatches displace and the failed attempt a flaky job leaves behind when re-run to green. Two real reviews were downgraded from Approve over exactly that ("CI failing: route" at QwenLM#7150; route, review-pr, review-config and four more at QwenLM#7171), each on a commit whose every live check was green on the PR page. Failure is now judged by the name's latest verdict, ordered by the runs' own timestamps -- the same evidence GitHub's PR page shows a human. Skipped/neutral/stale runs stay non-verdicts, so a re-dispatch that skipped cannot erase a real failure beside it; a name whose only run was cancelled still fails, because nothing superseded it; and a re-run that fails after a success fails, because latest wins in both directions. On equal or absent timestamps the first-listed run keeps the name (the API lists newest first). * test(review): pin the started_at fallback in verdictStamp The fallback branch had no test: with it dropped, two same-name runs whose completed_at is null collapse to insertion order. The winning run is listed second on purpose -- listed first, the fixture passes with or without the fallback (first-seen keeps the name on equal stamps) and pins nothing; listed second, the success can win only through its started_at.
What this PR does
Adds
--round <k>toqwen review agent-promptfor findings roles (verify,reverse-audit). The CLI bakes the round label into the launch prompt's identity line (You are review agent \reverse-audit` — Reverse audit agent (round 2).) and into the record key (reverse-audit--round-2--,reverse-audit--chunk-5--round-2--for--all-chunks`), so every round of the Step 5 loop is its own launch, its own brief, its own delivery receipt — even when the cumulative findings list has not changed between rounds.The flag is validated at the primary-mode boundary like
--all-chunks: it is refused with--roster,--whole-diff, a bare--chunk, a roleless call, a role that runs once, and any value that is not a 1-based integer — a silently dropped--roundwould be a record keyed as a launch the caller never made. The Step 5 skill text now passes--round <k>per round instead of telling the orchestrator to track rounds "in your own notes", and the Step 6 repair templates carry[--round <k>]so a repair rebuilt from the FIX line keeps its key shape.Why it's needed
Dogfooded on a real 3A review right after #7150 merged: the reverse-audit loop ran two rounds on the same findings list, so both builds shared one digest, one key, one record. The orchestrator — wanting to tell its own launches apart — appended
(round N)to the identity line, which is exactly the line the delivery check anchors on. Both rounds were flagged rewritten, the verdict was capped from Approve to Comment, and the review paid a full repair round for a label.This is the same failure family this skill keeps fixing the same way: when the CLI leaves something out of the printed prompt, the orchestrator writes it in by hand, and hand-writing is what the delivery check exists to refuse. What the caller will reach for, the CLI should print.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review— 31 files, 734 tests green. New coverage: round-in-key and round-in-identity for single and--all-chunksbuilds, two same-findings rounds producing two records,verify --round, boundary refusals (--roster/--whole-diff/bare--chunk/roleless/once-only role), and 1-based-integer validation.qwen review agent-prompt --plan <plan> --role reverse-audit --findings <file> --round 2— the first printed line ends with(round 2).and the record lands at<plan>-prompts/reverse-audit--round-2--<hex>.txt. Repeat with--round 3and both records coexist.Evidence (Before & After)
Before (dogfood transcript, 3A review): round 1 and round 2 launches both keyed
reverse-audit--23d5c771c4e9; the delivered identity lines read… Reverse audit agent (round 1)./(round 2).— hand-added, sowasDeliveredVerbatimfailed on the identity line andcompose-reviewcapped the verdict withcappedBy: ["unreviewed-dimension"].After: the same two builds with
--round 1/--round 2producereverse-audit--round-1--<digest>andreverse-audit--round-2--<digest>, each printing the label inside the identity line, and pasting the printed block verbatim passes delivery for both rounds.Tested on
Risk & Scope
--roundomitted → byte-identical behavior to today (label empty, key shape unchanged); no existing caller changes.role--*prefix family (reverse-audit,reverse-audit--*), so round-carrying keys are collected by the existing check-coverage/compose logic without changes there.--rosterand Step 3 flows untouched.Linked Issues
Follow-up from the #7150 post-merge dogfood (friction point: hand-labelled rounds).
中文说明
本 PR 做了什么
为
qwen review agent-prompt的 findings 角色(verify、reverse-audit)新增--round <k>:由 CLI 把轮次标签烙进 launch prompt 的身份行(You are review agent \reverse-audit` — Reverse audit agent (round 2).)和 record key(reverse-audit--round-2--;--all-chunks下为reverse-audit--chunk-5--round-2--`),使 Step 5 循环的每一轮都是独立的 launch、独立的 brief、独立的交付收据——即使两轮之间累积 findings 未变。该 flag 与
--all-chunks一样在 primary-mode 边界裁决:与--roster、--whole-diff、裸--chunk、无角色调用、单次运行角色组合,或传入非 1 起始整数,一律显式拒绝——被静默丢弃的--round意味着记录被键成一个调用者从未发起的 launch。Step 5 的 SKILL 文本改为每轮传--round <k>(不再让编排器"在自己的笔记里记轮次"),Step 6 修复模板带上[--round <k>],保证按 FIX 行重建的修复轮 key 形态一致。为什么需要
#7150 合入当天在真实 3A review 上 dogfood 发现:反审计循环对同一 findings 列表跑了两轮,两次构建共享同一 digest、同一 key、同一 record。编排器为了区分自己的两次 launch,在身份行末尾手工追加了
(round N)——而身份行恰是交付检查锚定的那一行。两轮全部被判 rewritten,verdict 从 Approve 被 cap 成 Comment,为一个标签付了一整轮修复。这与本 skill 反复修复的失效族同源:CLI 没打进 prompt 的东西,编排器就会手写进去,而手写正是交付检查要拒绝的。调用者会去够的东西,应该由 CLI 打印。
验证方式
cd packages/cli && npx vitest run src/commands/review—— 31 文件 734 测试全绿。新覆盖:单路径与--all-chunks的 key/身份行带轮次、同 findings 两轮产生两条记录、verify --round、边界拒绝、1 起始整数校验。--role reverse-audit --findings <file> --round 2首行以(round 2).结尾,记录落在<plan>-prompts/reverse-audit--round-2--<hex>.txt;换--round 3两条记录并存。证据(前后对照)
修复前(dogfood transcript,3A review):两轮 launch 同键
reverse-audit--23d5c771c4e9,交付的身份行为手工追加的(round 1)./(round 2).,wasDeliveredVerbatim在身份行失配,compose-reviewcap 掉 verdict(cappedBy: ["unreviewed-dimension"])。修复后:同样两次构建以
--round 1/--round 2生成reverse-audit--round-1--<digest>与reverse-audit--round-2--<digest>,标签由 CLI 打印在身份行内,逐字粘贴即通过两轮交付检查。风险与范围
--round行为与现状逐字节一致(标签为空、key 形态不变),现有调用零改动。role--*前缀族收集 key,带轮次的 key 无需改动 check-coverage/compose 即被覆盖。--roster与 Step 3 流程未触碰。关联
#7150 合入后 dogfood 的 follow-up(摩擦点:手工标注轮次)。