fix(review): count Step 6's inline findings from the drafted comments, never from typed numbers - #7173
Merged
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: Observed bug with clear evidence — a dogfooded report-only run moved its one Critical from Direction: Aligned. The two verdict boundaries ( Size: Not applicable — no core production code touched. The core-path changes are 4 lines of docs (SKILL.md and DESIGN.md). Production logic is all in Approach: The scope feels right. New Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有明确证据——dogfood 的 report-only 运行将唯一的 Critical 从 方向：对齐。两个 verdict 边界（Step 6 的 规模：不适用——未触及核心生产代码。core 路径的变更仅为 4 行文档（SKILL.md 和 DESIGN.md）。生产逻辑全部在 方案：范围合理。新增 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal (before reading the diff): I would add a Comparison: The PR's approach matches this proposal and goes one better — the unmarked-comment refusal is well-justified. An unmarked blocker weighs zero in the count, and for a verdict computation that means it approves the review it should block. Refusing at both boundaries (Step 6 for cheap draft fixes, Step 7 for post-churn drift) is the right call, not over-engineering. No correctness bugs, no security holes, no regressions found. The shared Reuse check: The PR correctly extracts previously duplicated counting logic. No critical findings. Real-Scenario TestingThree behavior probes driven through Test 1: Critical in drafted comments → Test 2: State JSON carrying Test 3: Unmarked comment at index 1 → refused, names the index Unit TestsAll three scenarios behave as documented. The fix works as described. 中文说明代码审查独立方案（读 diff 前）： 我会给 对比： PR 的方案与此一致且更优——无标记评论拒绝有充分理由。无标记的 blocker 在计数中权重为零，对 verdict 计算来说意味着它会放行本该阻止的审查。在两个边界拒绝（Step 6 处理廉价草稿修正，Step 7 处理变更后的漂移）是正确的选择，而非过度工程。 未发现正确性 bug、安全漏洞或回归。共享的 真实场景测试三个行为探针通过
单元测试30 个测试文件、726 个测试全部通过。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; focused fix for a dogfooded regression with comprehensive test coverage and no scope creep. The gap this closes is real and specific: on report-only runs, Step 6's verdict line was the final word, and it trusted typed numbers that could disagree with the comments they counted. The dogfood transcript shows exactly that — The independent proposal I wrote before reading the diff matched the PR's approach but stopped short of the unmarked-comment refusal. After seeing the code, the refusal earns its place: a blocker without a severity marker weighs zero in the count, so it approves the review it should block. Refusing at both boundaries (Step 6 cheaply, Step 7 after churn) is the right depth, not over-engineering. All three behavior probes confirm the fix works as described. 726 unit tests green. No correctness bugs, no regressions, no drive-by changes. Approving. ✅ 中文说明信心：5/5 — 各阶段均通过；针对 dogfood 回归的聚焦修复，测试覆盖全面，无范围蔓延。 该 PR 关闭的缺口真实且具体：在 report-only 模式下，Step 6 的 verdict 行是终局，而它信任的手填数字可能与它们所计数的评论不一致。dogfood transcript 准确展示了这一点—— 读 diff 前写的独立方案与 PR 方案一致但未包含无标记评论拒绝。看完代码后，该拒绝是合理的：无严重级标记的 blocker 在计数中权重为零，因此会放行本该阻止的审查。在两个边界拒绝（Step 6 廉价地，Step 7 在变更后）是正确的深度，而非过度工程。 三个行为探针确认修复按描述工作。726 个单元测试全绿。无正确性 bug、无回归、无顺手改动。 批准合并 ✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 18, 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. ✅
Collaborator
Author
Review — PR #7173, round 1 (head
|
Collaborator
Author
|
Re: round-1 review — both Suggestions and the nit taken, in
Suite: 31 files / 731 tests green. Thanks for the |
Collaborator
Author
Review — PR #7173, round 2 (head
|
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. |
…, never from typed numbers compose-review took criticalsInline/suggestionsInline as numbers in the model-written state JSON, with absence meaning zero. submit stopped trusting those numbers long ago -- it counts the comments in the payload and refuses the fields -- but Step 6 kept them, and the skill even told the orchestrator compose does not take inline counts at all. So on a report-only run, where nothing downstream recounts, the two computations the skill promises are the same one structurally diverged: a dogfooded review moved its one Critical from bodyCriticals to an inline comment, the count fell out on the way, compose printed `Verdict: Approve` over a blocker the same report listed -- and the model, seeing the wrong line, overrode it by hand, which is the exact move the verdict pipeline exists to forbid. Same cure as submit's, from one shared source: the severity prefixes and the counting move to lib/inline-counts.ts, used by both boundaries. compose-review now requires --comments -- the drafted inline comments file, the same array the Step 7 payload carries (bare array or full payload shape), [] when nothing anchors inline -- counts the markers itself, refuses a state JSON that still carries the numbers, and refuses a drafted comment that opens with no severity marker (an unmarked blocker weighs nothing, and nothing approves). The skill's Step 6 now says to draft the comments file before composing and to re-compose whenever the comment set changes, so the verdict is always computed from the set actually posted.
…y too Review follow-through on the round-1 suggestions: - submit now refuses a comment that opens with no severity marker, the same rule compose-review applies at Step 6. The gap between them was real: the skill's own re-compose instruction expects the comment set to churn after Step 6, and a marker lost in that churn reached exactly the boundary that posts -- where the verdict is counted from the markers, so the unmarked blocker weighed nothing and a clean state composed an APPROVE that posted the very comment it never weighed. - The opens-with-which-prefix predicate is stated once (severityOf in lib/inline-counts.ts); the counter, the unmarked-scan and submit's check all classify through it, so no two of them can drift -- drift between restatements being the bug-class the file's own header names. - Two docs stopped teaching the removed interface: the compose-review header and DESIGN.md both said 422 recovery is "the same call with updated counts"; the counts are counted now, so recovery is the same call with the updated --comments file.
wenshao
force-pushed
the
fix/compose-counts-from-comments
branch
from
July 18, 2026 16:18
3646dce to
35f1c25
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）为单个提交。 |
Collaborator
Author
Review — PR #7173, round 3 (head
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
Review — PR #7173, round 4 (head unchanged at
|
Collaborator
Author
|
@qwen-code /triage |
1 similar comment
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 18, 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. ✅
ytahdn
approved these changes
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅
compose-review 改为从 --comments 文件数 inline findings，关掉最后一个「计数与被计之物分离」的边界。共享 lib/inline-counts.ts 使 Step 6 verdict 行与 Step 7 posted verdict 成为同一来源上的同一计算。severityOf() 单一判定源消除 drift 风险。compose + submit 双边界都拒绝 unmarked comments，防御纵深到位。9 个新测试 + 突变测试 6/6 killed，覆盖充分。
ytahdn
approved these changes
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅
compose-review 改为从 --comments 文件数 inline findings，关掉最后一个「计数与被计之物分离」的边界。共享 lib/inline-counts.ts 使 Step 6 与 Step 7 成为同一来源上的同一计算，severityOf() 单一判定源消除 drift。双边界拒绝 unmarked comments，防御纵深到位。9 个新测试覆盖全面。
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.
What this PR does
Makes Step 6's
compose-reviewcount the inline findings from the drafted comments instead of acceptingcriticalsInline/suggestionsInlineas numbers in the model-written state JSON. The command now requires--comments <file>— the drafted inline comments, the same[{path, line, body, …}]array the Step 7 payload carries (bare array or full payload shape;[]when nothing anchors inline) — counts the**[Critical]**/**[Suggestion]**markers itself, refuses a state JSON that still carries the numeric fields, and refuses a drafted comment that opens with no severity marker (an unmarked blocker would weigh nothing, and nothing approves).The severity prefixes and the counting move to a shared
lib/inline-counts.ts, used by bothcompose-reviewandsubmit—submitalready counted from its payload and refused the typed numbers; it now does so through the same function, so the Step 6 verdict line and the posted verdict are the same computation on the same source. The skill's Step 6 tells the orchestrator to draft the comments file before composing and to re-runcompose-reviewwhenever the comment set changes (a failed anchor, a relocation to the body, a dropped comment).Why it's needed
Dogfooded on a report-only run (
/reviewwithout--comment, so nothing downstream ever recounts): the review's one Critical was anchorable, so the orchestrator moved it frombodyCriticalsto an inline comment — exactly as the skill directs — and the count fell out on the way, becausecompose-reviewtreats absent counts as zero and the skill even says Step 6 "does not take the inline counts".compose-reviewprintedVerdict: Approveover a blocker the same report listed, and the model, seeing a line it knew was wrong, overrode it by hand — the exact move the verdict pipeline exists to forbid, and this time the pipeline was the one forcing it.The root cause is a designed gap:
submitwas hardened to count-from-the-payload long ago, but Step 6 kept trusting typed numbers, so the two computations the skill promises are "the same computation on the same input" structurally diverged whenever inline findings existed — visibly so on report-only runs, where Step 6's line is the final word. A count handed over beside the thing it counts is a count that can disagree with it; this closes the last boundary where that was still possible.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review— 31 files, 730 tests green. New coverage: the report-only regression itself (a drafted inline Critical reaches the verdict line asRequest changes), counts derived from bare-array and payload-shaped files, refusal of typed counts in the state JSON, refusal of an unmarked comment (named by index), refusal of a missing/unreadable/non-array comments file;submit's behavior is unchanged and its suite still passes untouched except for the shared import.{"modelId":"m"}toin.jsonand[{"path":"a.ts","line":1,"body":"**[Critical]** x"}]toc.json, runqwen review compose-review --input in.json --comments c.json— the stderr verdict line readsRequest changes. Add"criticalsInline": 0toin.jsonand the command refuses with the pointer to--comments.--commentsguard, and coercing a non-array file to[]each turn exactly their tests red (6/6 killed).Evidence (Before & After)
Before (dogfood transcript, PR review in report-only mode): compose input carried
bodyCriticals: []and no counts after the finding moved inline;composed.jsonread"verdictLine": "Verdict: Approve"while the drafted comment carried**[Critical]**— and the archived report's verdict had to be hand-corrected toRequest changes, off-pipeline.After: the same state JSON with the same drafted comment file composes to
event: REQUEST_CHANGES,verdictLine: "Verdict: Request changes …"— pinned by the regression test named for the hole.Tested on
Risk & Scope
--commentsis a new required flag forcompose-review; the bundled skill (updated in this PR) is its only caller, and the change is a hard refusal rather than a silent behavior shift — an un-updated call fails with the sentence that names the fix.submitsemantics are unchanged (same counting, now via the shared function); its structural refusal of typed counts already existed.submitkeeps its permissive counting, and comment bodies are never rewritten by either.Linked Issues
Follow-up from the #7150 post-merge dogfood (friction point: the Step 6 verdict line was blind to inline findings on report-only runs).
中文说明
本 PR 做了什么
让 Step 6 的
compose-review从已起草的 inline 评论文件里数 inline findings,不再接受 state JSON 里手填的criticalsInline/suggestionsInline数字。命令新增必填--comments <file>——即 Step 7 payload 将携带的同一[{path, line, body, …}]数组(裸数组或完整 payload 形状均可;无 inline 锚定时传[])——由 CLI 自己数**[Critical]**/**[Suggestion]**标记;state JSON 若仍带数字字段则拒绝;起草评论若无严重级标记开头也拒绝(无标记的 blocker 权重为零,而零会放行 Approve)。严重级前缀与计数逻辑下沉到共享的
lib/inline-counts.ts,compose-review与submit共用——submit早已从 payload 数评论并拒绝手填数字,现在走同一个函数,使 Step 6 的 verdict 行与最终提交的 verdict 成为同一来源上的同一计算。SKILL 的 Step 6 相应更新:先起草评论文件再 compose;评论集合发生变化(锚定失败、降级到 body、丢弃)后必须重跑 compose。为什么需要
在 report-only 模式(
/review不带--comment,下游没有任何重算)的 dogfood 中:唯一的 Critical 可锚定,编排器按 SKILL 指引把它从bodyCriticals挪到 inline 评论——计数在途中丢失,因为compose-review把缺席当零,SKILL 甚至写着 Step 6"不收 inline 计数"。于是 compose 在同一报告列出的 blocker 之上打印Verdict: Approve;模型明知该行有错,只能口头改判——这恰是 verdict 流水线要禁止的动作,而这次是流水线自己逼出来的。根因是一个"设计好的缺口":
submit早就改为从 payload 计数,但 Step 6 仍信手填数字,导致 SKILL 承诺的"同一输入上的同一计算"在存在 inline findings 时结构性分叉——report-only 模式下 Step 6 的行就是终局,分叉直接见报。计数与被计之物分离,就一定会有不一致的一天;本 PR 关掉了最后一个这样的边界。验证方式
cd packages/cli && npx vitest run src/commands/review—— 31 文件 730 测试全绿。新覆盖:report-only 回归本身(起草的 inline Critical 抵达 verdict 行为Request changes)、裸数组/payload 两种形状、拒绝 state 带数字、拒绝无标记评论(按下标点名)、拒绝缺失/不可读/非数组评论文件;submit行为不变。in.json写{"modelId":"m"},c.json写一条**[Critical]**评论,跑compose-review --input in.json --comments c.json→ stderr verdict 行为Request changes;给in.json加"criticalsInline": 0→ 拒绝并指向--comments。--comments守卫、非数组文件被吞成[]——各自打红对应测试(6/6 击杀)。证据(前后对照)
修复前(dogfood transcript,report-only):finding 挪到 inline 后 compose 输入只剩
bodyCriticals: []且无计数;composed.json为"verdictLine": "Verdict: Approve",而起草评论带着**[Critical]**——报告的 verdict 只能在流水线之外手工纠正为Request changes。修复后:同样的 state JSON + 同样的评论文件 compose 出
event: REQUEST_CHANGES、verdictLine: "Verdict: Request changes …"——由以这个洞命名的回归测试钉死。风险与范围
--comments是compose-review的新必填项;唯一调用方是本 PR 同步更新的 bundled skill,且行为是硬拒绝而非静默变化——未更新的调用会得到点名修法的报错。submit语义不变(同一计数,改走共享函数);其对手填计数的拒绝早已存在。submit保持宽容计数,两端都不改写评论正文。关联
#7150 合入后 dogfood 的 follow-up(摩擦点:report-only 模式下 Step 6 verdict 行对 inline findings 全盲)。