fix(review): count a fix-induced re-report as first-time work - #9744
Conversation
Closes #9674. A carried id has meant two different things since the fix-induced disposition shipped, and the volume trend's first-time count read both as re-posts. One is a re-post: a finding re-asserted under the id it already had. The other is a new defect wearing the id of the entry whose fix produced it, carried deliberately so the author reads one thread per churning site instead of a new one every round. Counting that as a re-post made the trend understate new work exactly where the loop was creating the most of it — measured on the pull request that introduced the disposition, a round that newly identified six defects and re-reported four of them under earlier ids recorded a first-time count of two. Neither count moves. They measure different things and both readings are correct, which is why the two reconciliations the issue rules out stay ruled out: excluding carried-id re-reports from the census would put the attributed count outside it and every such census would be refused as impossible, and counting them as first-time posts wholesale would tell the trend a re-assertion is new work. What was missing is the distinction itself, so the comment now carries it: a fix-induced re-report is marked, and the reader of drafted comments passes that through to the count. The marking sits after the id and its separator, never inside the id grammar. That grammar is shared with the ledger's own carry, so widening it to swallow a parenthetical would put a finding's identity on the same regex as a model-written adjective — a spacing the wider grammar failed to anticipate would stop matching the id and silently renumber the finding. Read after the id, nothing about the token can cost it, and the reading is correspondingly lenient about case and spacing because it governs only whether a comment counts as first-time work. An unrecognised marking leaves the draft counted as a re-post, which is what every round did before this existed; a marking wrongly added to a still-stands is the expensive direction, so the skill restricts the token rather than offering it as a way to flag any carried finding as interesting. The token is stripped from the claim before it reaches the work list. Left in, it would ride into the next round as part of the text Step 6 re-locates the claim by and the status table prints — machine vocabulary about how to count one round, outliving the round it described. Beside no id it is ordinary claim text and survives, because there is no entry there for it to qualify and editing a finding's own words on the strength of a word it opened with is not this token's business.
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed and measured, not theoretical. Issue #9674 documents the bias with a concrete probe — a round on #9596 at 51cd13f posting eight comments (two genuinely new, four carried-id fix-induced, two still-standing) stamped Direction: aligned. This is the repo's own review-loop bookkeeping — the posting-volume trend's first-time count — split out of #9596 (round 5) precisely to keep that PR from widening further. No CHANGELOG reference, nor should there be: nothing user-facing changes. No escalation surface touched (no auth/sandbox/model-selection/telemetry/release/public contract). Size: ~140 production logic lines ( Approach: the scope is exactly the issue's suggested direction — mark a fix-induced re-report beside (never inside) the carried id, read the marking in Risk: no elevated risk signals — none of the changed files matches the revert-correlated path list. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有实测数据，不是理论性问题。Issue #9674 记录了带具体探针的偏差——在 #9596 的 51cd13f 上，一轮张贴 8 条评论（2 条真正的新发现、4 条携带 id 的 fix-induced 重报、2 条仍然成立），却只盖出 方向：对齐。这是仓库自身 review 循环的记账——张贴量趋势的首次计数——从 #9596（第 5 轮）拆出，正是为了不让那个 PR 继续膨胀。CHANGELOG 没有也不需要有引用：没有任何用户可见的变化。不触及任何升级面（无 auth/沙箱/模型选择/遥测/发布/公共契约）。 规模：约 140 行生产逻辑（ 方案：范围恰为 issue 建议的方向——在携带 id 旁侧（而非其内部）标记 fix-induced 重报， 风险：无升级风险信号——改动文件均不匹配与 revert 相关的路径清单。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed against The implementation matches the issue's suggested direction and lands cleanly in the existing readback architecture:
One honest caveat: a marked draft also enters the recurrence signal's per-file fresh count, not just the volume trend. That is consistent — a fix-induced defect is by definition new work at a file that already had findings, which is the churn pattern the recurrence signal exists to name — but the PR description's "touches only the posting-volume trend's first-time count" is one clause short of the whole picture. Not a blocker. Testing evidenceUnattended CI run — PR code is never executed here. The evidence below is the PR's own CI on the reviewed commit, fetched via the API; the finalize job rewrites the table region once CI settles. The Linux unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The changed suites' fixtures are differential — the same round composed twice over the same work list and id, with the marking as the only difference — so a green run pins the count change in both directions rather than on a count that simply never moves. Not verified here: how often the marking is applied correctly in live rounds. That is measurable only after this lands, the PR says so plainly, and no sandboxed lane could settle it — 中文说明代码审查按
一个诚实的说明：被标记的草稿不只进入张贴量趋势，也进入复发信号按文件的新鲜计数。这是一致的——fix-induced 缺陷按定义就是已有发现的文件上的新工作，正是复发信号要指认的打转形态——但 PR 描述中"只触及张贴量趋势的首次计数"这句话少了半句。不构成阻塞。 测试证据无人值守 CI 运行——此处不执行 PR 代码。下方证据是 PR 自身 CI 在受审提交上的结果，经 API 获取；finalize 任务会在 CI 落定后重写表格区域。 Linux 单测套件（ 改动套件的夹具是差分式的——同一份工作清单、同一个 id 组装同一轮两次，唯一差别是标记——因此绿色结果双向钉住计数变化，而不是钉住一个从不变动的计数。此处未验证：标记在真实轮次中被正确使用的频率。这只能在本改动上线后测量，PR 也如实说明，且任何沙箱通道都无法解决—— — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review, no blockers found; approval deferred only because the unit suite is still in flight on the reviewed commit. Stepping back: this is a small, precisely-scoped fix for a measured bias in the review loop's own bookkeeping, filed by the maintainer who observed it, with the reproduction numbers in the issue. The approach matches the one I would have proposed before reading the diff — tell the counter what the id can no longer say, at the one point where the round knows the answer, rather than widening the id grammar or inferring a distinction the round already makes explicitly. The implementation goes further than the minimum in exactly the right places: the marking sits outside the shared id grammar so nothing about it can cost a finding its name, it is stripped before it can ride the work list into the next round, it cannot appear without the id it qualifies, and the one-round mixed-rule migration seam is documented instead of discovered later. The differential test style — same round composed twice, marking as the only difference — is the right way to pin a count that a one-sided assertion would let pass vacuously. The one reservation is procedural, not about the code: 中文说明置信度：4/5 —— 审查干净、无阻塞；仅因单测套件在受审提交上仍在运行而暂缓批准。 退一步看：这是针对 review 循环自身记账中实测偏差的小而精确的修复，由观察到该偏差的维护者提交，issue 中附有复现数字。方案与我读 diff 之前独立提出的思路一致——在轮次知道答案的唯一位点，把 id 已无法说明的东西告诉计数器，而不是放宽 id 文法、或去推断轮次本就显式作出的区分。实现在恰当之处比最低要求走得更远：标记位于共享 id 文法之外，因此不可能让任何 finding 失去名字；它在进入工作清单前被剥离；没有它所修饰的 id 就不会出现；单轮混合规则的迁移接缝被记录在案而非留待日后发现。差分式的测试写法——同一轮组装两次、唯一差别是标记——是钉住一个计数的正确方式，单侧断言会让它空转通过。 唯一保留是程序性的，与代码无关： — Qwen Code · qwen3.8-max Reviewed at |
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
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
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/commands/review/lib/run-ledger.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/commands/review/lib/run-ledger.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * Case-insensitive, and tolerant of inner spacing, because it governs only | ||
| * whether a comment counts as first-time work — never which finding it is. | ||
| */ | ||
| const FIX_INDUCED_READBACK = /^\(\s*fix-induced\s*\)[:.,-]?\s*/i; |
There was a problem hiding this comment.
[Suggestion] The diff inserted a new doc comment and the FIX_INDUCED_READBACK constant between readClaim's original doc block and the function, and gave readClaim a replacement doc — so two consecutive /** */ blocks now precede FIX_INDUCED_READBACK. The older block ("The first line of what follows the severity marker, minus any carried id… Module-level rather than a closure inside the ledger builder…") still describes readClaim's behavior — readClaim is the function shared by buildLedger and draftedFindingsOf — but it now dangles in front of a regex constant it does not explain, while readClaim got a replacement doc. A reader (or IDE hover) arriving at FIX_INDUCED_READBACK reads readClaim's description first, and the rationale is detached from the function it explains. Delete the orphaned block (its substance is covered by readClaim's new doc), or fold any still-wanted wording into that doc so the constant has exactly one doc comment above it.
中文说明
此 diff 在 readClaim 的原始文档块和函数之间插入了新的文档注释与 FIX_INDUCED_READBACK 常量，并为 readClaim 补了一份替代文档 —— 于是 FIX_INDUCED_READBACK 前面现在有两个连续的 /** */ 块。较旧的块（"The first line of what follows the severity marker, minus any carried id… Module-level rather than a closure inside the ledger builder…"）描述的仍是 readClaim 的行为 —— readClaim 正是 buildLedger 与 draftedFindingsOf 共用的函数 —— 但它现在悬在这个它并不解释的正则常量前面，而 readClaim 已有替代文档。读者（或 IDE 悬浮）到达 FIX_INDUCED_READBACK 时会先读到 readClaim 的描述，那段设计理由也与它真正解释的函数脱节。建议删除孤立块（其内容已被 readClaim 的新文档覆盖），或把仍想保留的措辞并入那份文档，使该常量上方恰好只有一个文档注释。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // expensive error, and that is why the skill restricts the token to a | ||
| // re-report that genuinely is fix-induced rather than offering it as a way | ||
| // to flag any carried finding as interesting. | ||
| if (d?.fixInduced === true) return true; |
There was a problem hiding this comment.
[Suggestion] This arm changes the semantics of the fresh number that is persisted in the ledger marker and compared across rounds, but nothing distinguishes old-semantics markers from new ones: v stays 1, and prevLedgerFacts applies only a fresh <= posted sanity cap. At the deployment boundary of an in-flight loop, round N's new-rule count (marked fix-induced re-reports included) is compared against prev.fresh recorded by round N−1 under the old rules. The old-rule prev.fresh undercounts (it excluded fix-induced re-reports), so the mixed comparison can only produce a spurious firing, never mask a genuine one: a loop whose round N−1 posted 2 new findings plus k uncounted fix-induced re-reports (prev.fresh = 2) and whose round N posts 0 brand-new findings but 2 marked re-reports yields fresh.length = 2 >= prev.fresh = 2, so volumeNotShrinking fires and the body publishes "round N produced 2 first-time finding(s); the previous round produced 2" plus the batch-fixes/stem-surface advisory paragraph — one round of wrong trend narrative per in-flight loop, where the consistent new-vs-new comparison (2 vs 2+k) would have shown the volume shrinking. Advisory only (no verdict cap reads the diagnosis), self-heals the next round, and foreign markers are unaffected (VOLUME_FIELDS strips their fresh). Verified by probe in a scratch tree at the reviewed commit: an old-rule marker {round: 3, posted: 4, fresh: 2} plus a round posting two marked re-reports fires volumeNotShrinking (codes root-cause-triage, batch-fixes, stem-surface); the same round against a consistent new-rule baseline (fresh: 3) does not; reverting this early return removes the firing. To make the seam explicit: bump the ledger v (or stamp a small flag beside the volume tier) with this semantics change and have prevLedgerFacts read fresh from pre-change markers as absent — the module already treats absent prev.fresh as "trend unevaluable rather than measured on the wrong number" — or document the one-round boundary distortion as accepted in the Ledger.fresh contract.
中文说明
这一分支改变了持久化在 ledger marker 中、并跨轮比较的 fresh 计数的语义，但没有任何东西区分旧语义与新语义写出的 marker：v 保持 1，prevLedgerFacts 只有 fresh <= posted 这一道健全性上限。在任一进行中循环的部署边界上，第 N 轮按新规则算出的计数（含带标记的 fix-induced 重报）会与第 N−1 轮按旧规则记下的 prev.fresh 比较。旧规则的 prev.fresh 是少算的（它不计 fix-induced 重报），因此这种混合比较只可能产生误触发，不可能掩盖真正的收敛：一个第 N−1 轮发出 2 条新 finding、另有 k 条未计入的 fix-induced 重报（prev.fresh = 2），第 N 轮 0 条全新 finding、只有 2 条带标记重报的循环，会得到 fresh.length = 2 >= prev.fresh = 2，于是 volumeNotShrinking 触发，正文会刊出 "round N produced 2 first-time finding(s); the previous round produced 2" 以及 batch-fixes/stem-surface 建议段落 —— 每个进行中的循环至多一轮的错误趋势叙述；而一致的新-新比较（2 对 2+k）本应显示数量在下降。仅建议性质（没有任何裁决上限读取该诊断），下一轮两点都用新规则后自愈，外部账号的 marker 也不受影响（VOLUME_FIELDS 会剥掉其 fresh）。已在被审提交的 scratch tree 中用探针验证：旧规则 marker {round: 3, posted: 4, fresh: 2} 加上一个发出 2 条带标记重报的轮次会触发 volumeNotShrinking（codes 为 root-cause-triage、batch-fixes、stem-surface）；同一轮对一致的新规则基线（fresh: 3）则不触发；回退这一早返回后触发消失。若要把这个接缝显式化：随本次语义变更提升 ledger 的 v（或在 volume 档旁打一个小标记），并让 prevLedgerFacts 把变更前 marker 里的 fresh 读作缺失 —— 模块本就把缺失的 prev.fresh 当作"趋势不可评估，而非在错误的数字上测过" —— 或在 Ledger.fresh 契约里把这一轮边界失真明确记为已接受。
— qwen3.8-max via Qwen Code /review (v0.22.0)
… seam (#9744) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix address-review round — PR #9744Commit: Feedback dispositions[rv:5000959212] Review body — COMMENTED, "no blockers" — no actionThe review carries no findings; its suggestions arrived as the two inline [rc:3836907800] Orphaned doc block before
|
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.
Test Plan (not a blocker): src/commands/review/lib/run-ledger.test.ts — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/convergence.ts:335 — [review] D2-1 nonConvergenceCritical doc still forbids the marked-re-report counting rule this PR ships — deferred (code-age rule: anchored on code unchanged since round 1, which rou…
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/commands/review/lib/run-ledger.test.ts — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| * each rule for exactly one round; the old rule UNDERCOUNTED (it dropped | ||
| * the marked re-reports), so the mixed comparison can only fire the volume | ||
| * advisory spuriously, never mask a genuine one — and the advisory decides |
There was a problem hiding this comment.
[Suggestion] The seam note added this round on Ledger.fresh guarantees that the mixed old-rule/new-rule comparison "can only fire the volume advisory spuriously, never mask a genuine one", but that guarantee is one-sided. If the last old-rule round's entire new output was fix-induced re-reports, the old rule stamps fresh: 0 — postedFresh writes the zero, not undefined, and zero survives the persistence chain by design (ledger.ts:357-361, 461-462, 736-739). In the first new-rule round the restart guard input.prev.fresh > 0 (convergence.ts:572-578) then suppresses the comparison outright, masking a genuine advisory for exactly that transition round. The "undercounted ⇒ cannot mask" inference holds for the inequality alone — a smaller prev.fresh only makes fresh.length >= prev.fresh easier to satisfy — but it overlooks that the undercount can land on exactly the value (0) where the guard refuses the comparison; the pre-existing test 'will not measure a trend against a settled predecessor' (convergence.test.ts:156-171) pins that zero-prev state as reachable. The advisory decides nothing today and heals the round after, but this note is the load-bearing rationale for not bumping the marker version, and a maintainer promoting the volume signal to a gate — or auditing that decision — inherits a false "never mask" guarantee about the one round where the two counting rules meet.
| * each rule for exactly one round; the old rule UNDERCOUNTED (it dropped | |
| * the marked re-reports), so the mixed comparison can only fire the volume | |
| * advisory spuriously, never mask a genuine one — and the advisory decides | |
| * each rule for exactly one round; the old rule UNDERCOUNTED (it dropped | |
| * the marked re-reports), so the mixed comparison can fire the volume | |
| * advisory spuriously — and, when the undercount reaches 0 because the | |
| * predecessor's whole new output was re-reports the old rule dropped, the | |
| * `prev.fresh > 0` restart guard suppresses the comparison, masking the | |
| * advisory for that one round. Either way the advisory decides |
中文说明
本轮新增在 Ledger.fresh 上的接缝注释保证:新旧规则混合比较「只可能误触发数量建议,绝无可能掩盖真正的信号」,但这个保证是单边的。如果旧规则最后一轮的全部新产出都是 fix-induced 重报,旧规则会记下 fresh: 0 —— postedFresh 写下的是 0 而不是 undefined,且 0 按设计会存活于整条持久化链(ledger.ts:357-361、461-462、736-739)。于是第一个新规则轮次的比较会被重启守卫 input.prev.fresh > 0(convergence.ts:572-578)整体压制,恰好在那个过渡轮掩盖一次真正的建议。「少算 ⇒ 不可能掩盖」的推理只对不等式本身成立 —— prev.fresh 更小只会让 fresh.length >= prev.fresh 更容易满足 —— 但它忽略了少算可以恰好落在守卫拒绝比较的那个值(0)上;既有测试 'will not measure a trend against a settled predecessor'(convergence.test.ts:156-171)已钉住该零值状态确实可达。该建议今天不决定任何事,下一轮即自愈,但这段注释是「不提升 marker 版本」这一决定的承重理由;将来若有人把数量信号提升为闸门、或审计该决定,会继承一个关于两种计数规则交汇那一轮的错误「绝不掩盖」保证。
— qwen3.8-max via Qwen Code /review (v0.22.0)
… note (#9744) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix address-review round — PR #9744Commit: Feedback dispositions[rc:3837178821] Seam note's "never mask a genuine one" guarantee is one-sided — RESOLVEDReproduced before acting, by code inspection plus the pinned existing test:
So the finding holds: the old rule undercounting means a smaller Fix: implemented the finding's own suggested wording verbatim (comment-only). That is the minimal root-cause change: the zero-prev guard behavior is intentional and pinned by the existing test, and the finding frames the defect as the note being the load-bearing rationale for not bumping the marker version. Changing behavior instead (a marker-version bump or a transition-round special case) is exactly what the seam note documents as deliberately not paid for — the advisory decides nothing, names itself an observation, and heals the round after, in the masking case as in the spurious-firing case. [rv:5001195194] Review body — COMMENTED, "no blockers" — no action
Changes
Mutation probes: not applicable — this round adds no guard, branch, or behavior (comment-only change), so there is nothing to witness. Integration tests: not applicable — no behavior change, and nothing touched is exercised only through the bundled CLI. Settings schema: no settings source changed. Verification
中文说明Autofix 评审反馈处理轮 — PR #9744提交： 反馈处理结论[rc:3837178821] 接缝注释"绝不会掩盖真正的建议"的保证是单边的 — 已解决先行复现，方式为代码核查加既有钉住测试：
因此该发现成立：旧规则少算意味着更小的 修复：原文采纳该发现自带的建议措辞（纯注释改动）。这是最小根因改动：零前驱守卫的行为是有意的且已被既有测试钉住，而该发现把缺陷定位为"这段注释是'不提升 marker 版本'这一决定的承重理由"。若改为修改行为（提升 marker 版本或为过渡轮开特例），恰恰是接缝注释已记录在案、刻意不付的代价——该建议不决定任何事、自述为观察，且在掩盖情形与误触发情形中一样，下一轮即自愈。 [rv:5001195194] 评审正文 — COMMENTED，"无阻断问题" — 无需处理
改动
变更探针（mutation probe）：不适用——本轮未新增任何守卫、分支或行为（纯注释改动），没有需要见证的对象。集成测试：不适用——无行为变化，所触内容也并非只经由打包后的 CLI 行使。设置 schema：未改动任何设置源。 验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 5": did not execute the test suites to confirm they pass — this review worktree has no node_modules installed ( vitest itself unresolvable), and installing/build….
Test Plan (not a blocker): src/commands/review/lib/run-ledger.test.ts — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/convergence.ts:335 — [review] nonConvergenceCritical docblock, SKILL.md:814 and a test comment still state the old counting rule (re-surfacing of round-2 D2-1)packages/cli/src/commands/review/lib/convergence.ts:335 — [review] isFreshDraft contract docblock and DraftedFinding preamble still state the old rulepackages/cli/src/commands/review/lib/convergence.ts:335 — [probe] truncation caveat and rescue comment still state the pre-marking rule
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 5"：did not execute the test suites to confirm they pass — this review worktree has no node_modules installed ( vitest itself unresolvable), and installing/build…。
Test Plan（非阻断）：src/commands/review/lib/run-ledger.test.ts — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Three passages written when a carried id could only mean one thing now forbid the rule this branch adds. The blocker's docblock rules out "counting them as first-time POSTS" as one of two reconciliations that must never be made; the skill's census paragraph says the volume trend is the count "where a carried id is a re-post"; and a test comment restates the same premise. Each was true before a fix-induced re-report could be marked, and each now tells the next reader to undo the code beside it. The distinction the passages were protecting is real and stays. What they ruled out was reading a carried id as first-time work BY INFERENCE, which would count every re-assertion of a standing finding as new work — still wrong, and still what `isFreshDraft` refuses. What this branch added is narrower and is not an inference: the round marks the re-report, and only a marked one counts. An unmarked carried id is a re-post to the trend exactly as before, so the two counts still diverge by design; what is gone is the premise that a carried id can mean only one thing. The census paragraph gains the consequence that follows for whoever writes the comment: a fix-induced finding counted in the census but left unmarked in the body is counted by neither number. Prose only — no logic, no test assertion changes. Reported twice by the review as a deferred finding and left standing under the code-age rule, which is correct as a posting decision and not a reason to leave a contradiction in the file.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 4856 passed · 0 failed · 4856 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4856 通过 · 0 失败 · 4856 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9744 — fix(review): count a fix-induced re-report as first-time workVerdict: 中文摘要
ScopeCentral claim: a draft comment carrying a previous round's id AND the Secondary claims: (1) the token is stripped from the claim before it reaches the cross-round work list, survives as ordinary text where there is no id, and is never emitted on a finding that lost its id; (2) an unmarked round behaves byte-identically to base (backward compat), and the census counts ( Out of scope (not covered): per-commit attribution (shallow checkout), live-round marking accuracy, TUI surfaces, repo-wide suites beyond the review subsystem + core skill test. Central claim — A/B load-bearing proofHarness Fixture: round 4, previous marker round 3 with work list R3-1..R3-4; drafts: d1 new (no id), d2
Witnesses: Secondary claims
Scripted adjudication 15/15 (
Reviewer Test Plan walkthrough
FindingsF1 (Low, docs) — SKILL.md census paragraph: "counted by neither" contradicts the mechanism it describes
The same paragraph's first sentence says the opposite of the census half: "Fix-induced findings count whether they took a previous id or a new one." Measured behavior (cell 3, both arms, Why it matters: this is model-facing instruction on the census, and the cross-check on the census is one-sided ("an under-count passes it"). A model that reads "counted by neither" as "the census shouldn't count it either" and reconciles its census downward would delay the non-convergence blocker on exactly the churning PRs the mechanism exists for — the expensive direction under this PR's own asymmetry framing. Probability is low (the explicit counting instruction contradicts the trailing sentence), so severity is Low; the mechanism is correct and every behavioral assertion passes. Reproduce: Suggested fix (measured)Replace the tail of the sentence with the mechanism's actual consequence: "…so a fix-induced finding you count here but leave unmarked in the body drops out of the trend's count alone — the census keeps it, the trend reads it as the re-post it looks like, and the two numbers diverge by exactly that finding." Measured: cell 3 is precisely this statement (census publishes 4, trend 0, both arms); no code change, no test pins the current sentence, so the reword breaks nothing (the SKILL.test.ts pin targets different sentences — verified by the vacuity mutation redding only the marking pin). Not covered
MethodologyEnvironment: CI verify container ( Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review at 0d8b5e18 — the three open Suggestions are addressed; semantics verified by probe; approving
Small, well-scoped follow-up to the churn mechanism. I checked the three standing Suggestions against the code and then exercised the new reading rules directly, because the whole change rests on a regex that must never cost a finding its identity.
The three open Suggestions — addressed at this head
Orphaned doc block (compose-review.ts:5307) — fixed. FIX_INDUCED_READBACK now carries exactly one doc block, the one that explains it, and readClaim below has its own. The dangling description is gone.
Undistinguished fresh semantics across the deployment seam (convergence.ts:335) — addressed as an accepted, documented seam on Ledger.fresh, and I agree with the reasoning for not bumping v: parseLedger refuses any unknown v, so a bump for a count no gate reads would cost an old reader the whole marker — work list, anchor, streak — which is a worse trade than one round of advisory noise. That is consistent with how every other field here degrades by absence.
The one-sided "never mask" guarantee (ledger.ts) — fixed properly rather than argued away. The note now names the zero-prev case explicitly: when the undercount reaches 0 because the predecessor's whole new output was re-reports the old rule dropped, the prev.fresh > 0 restart guard suppresses the comparison and masks the advisory for that round. That was the load-bearing rationale for skipping the version bump, so correcting it rather than leaving a false absolute is the right call.
What I verified myself
I ran readClaim's real regexes (LEDGER_ID_READBACK + FIX_INDUCED_READBACK) over the shapes that matter:
"R1-2: (fix-induced) the new claim" id=R1-2 marked=true title="the new claim"
"R1-2: the same claim" id=R1-2 marked=false title="the same claim"
"R1-2: (Fix-Induced) …" / "( fix-induced ) …" / "(fix-induced). …" all marked, id intact
"R1-2. …" / "R1-2) …" both id terminators carry the marking
"(fix-induced) something" id=— marked=false title="(fix-induced) something"
"R1-2: (fix-inducedX) claim" id=R1-2 marked=false title="(fix-inducedX) claim"
"R1-2: (fix induced) claim" id=R1-2 marked=false title="(fix induced) claim"
Three properties hold that the design depends on:
- The marking never costs the id — with and without it,
R1-2andR12-34read back identically. Keeping the token out of the id grammar rather than wideningLEDGER_ID_READBACKis the right instinct; a shared grammar swallowing a model-written adjective is exactly how a finding gets silently renumbered. - No marking without an id — enforced twice independently:
readClaimonly runs the token regex whencarriedmatched, anddraftedFindingsOfsets the field only undercarried !== undefined && fixInduced. Beside no id the token survives as ordinary claim text, as documented. - Every miss fails toward "re-post" — near-miss tokens (
(fix-inducedX),(fix induced)) are not markings and leave the count where every round had it before. That is the cheap direction; a marking wrongly added to a still-stands is the expensive one, which is why restricting the token in the skill rather than making the reader liberal is the correct asymmetry.
fixInduced is not a dead switch: declared at convergence.ts:96, set at compose-review.ts:5444, read at convergence.ts:335. The token is stripped from the title before it reaches the cross-round work list, so it cannot outlive its round. House style is clean.
One cosmetic artefact from the probe, not worth changing: R1-2: (fix-induced) - the new claim leaves the title as "- the new claim", because the optional separator class binds directly after ) and a space intervenes. It affects only a work-list title's leading character on a shape no instruction produces.
Worth calling out
The final commit — "stop the blocker's docblock forbidding what this PR ships" — removes three passages that had become instructions to undo the code beside them, and its message draws the distinction that actually matters: what those passages protected was reading a carried id as first-time work by inference, which isFreshDraft still refuses; what this PR adds is an assertion the round makes explicitly. That is the same stale-seam-documentation failure class I have been flagging on the sibling PRs in this area, found and closed here before it shipped rather than deferred. The census paragraph also gained the honest consequence for whoever writes the comment: a fix-induced finding counted in the census but left unmarked in the body is counted by neither number.
Where I land
Approving. The three Suggestions are closed, the reading rules behave as documented under direct exercise, and the failure directions all point at the pre-existing behaviour.
Two things I did not verify: CI was still running when I reviewed — Test (ubuntu-latest, Node 22.x) and review-pr were both pending on this head, so the suite result is not part of my evidence (merge is CI-gated regardless). And I did not run the suites locally (no node_modules in my worktree), so the eight-mutation probe result is the author's, not mine; my evidence is the regex behaviour and the read/write sites traced at this head.
|
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
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. ✅
|
Released in v0.22.2. |
4 participants
What this PR does
Makes the review's first-time count recognise a fix-induced re-report as new work, so a carried id stops answering a question it can no longer answer on its own.
Since the fix-induced disposition shipped, a comment carrying an earlier round's id means one of two things. It may re-assert a finding that still stands — a re-post, and the count is right to treat it as one. Or it may report a genuinely new defect under the id of the entry whose fix produced it, carried deliberately so the author reads one thread per churning site instead of a new one every round. Reading the id alone called both re-posts. The comment now carries the distinction: a fix-induced re-report is marked as such where the author can see it, and the reader of drafted comments passes that through to the count.
Neither of the two counts in the posted body moves. They measure different things — one counts defects the round newly identified, the other counts comments posted for the first time — and both readings are correct, which is why the two obvious reconciliations stay ruled out and are recorded as refusals in the code: excluding carried-id re-reports from the census would put the attributed count outside it and every such census would be refused as impossible, and counting them as first-time posts wholesale would tell the trend that a re-assertion is new work.
The marking sits after the id and its separator, deliberately outside the id grammar. That grammar is shared with the ledger's own carry, so widening it to swallow a parenthetical would put a finding's identity on the same expression as a model-written adjective: a spacing the wider grammar failed to anticipate would stop matching the id and the finding would be silently renumbered, which is the failure that "one finding, one name" exists to prevent. Read after the id is already in hand, nothing about the token can cost it, and the reading is correspondingly lenient about case and inner spacing because it governs only whether a comment counts as first-time work, never which finding it is. The token is stripped from the claim before it reaches the cross-round work list, so it cannot outlive the round it describes; beside no id it is ordinary claim text and survives untouched.
Why it's needed
The bias is not uniform, which is what makes it worth fixing rather than tolerating: fix-induced re-reports concentrate on pull requests that are churning, so the trend baseline reads as if new work were falling on exactly the pull requests where it is not. Measured on the pull request that introduced the disposition, a round posting eight comments — two genuinely new, four carried-id fix-induced, two still standing — recorded a first-time count of two.
The failure direction of the fix is the safe one. A marking the reader does not recognise leaves the draft counted as a re-post, which is what every round did before this existed. A marking wrongly added to a still-stands is the expensive error — it would narrate divergence at the steady state — so the skill restricts the token to a re-report that genuinely is fix-induced rather than offering it as a way to flag any carried finding as interesting.
Reviewer Test Plan
How to verify
Run the changed suites:
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts src/commands/review/lib/convergence.test.ts, andcd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts.The behaviour worth confirming by reading is the differential, because a count that never moves would satisfy a one-sided assertion. One fixture composes the same round twice over the same work list and the same id, differing only in the marking: unmarked it records no first-time work, marked it records one, and in both arms the id still carries and the author still gets one thread for the site. The claim's title in the cross-round work list is asserted to be the claim alone, with the token gone.
Two properties are worth checking deliberately. The marking must never cost the id — pinned as a differential rather than as a list of expected ids, because the first version of that test asserted an id for a shape that has never carried one (the shared grammar wants whitespace after the separator, with or without any marking) and reddened correctly. And the marking must never appear without the id it qualifies: a second draft under an id the round already spent has its id dropped, and the marking goes with it.
Each guard here was mutation-probed — eight mutations, all red. Two of them were green on the first pass, and for the same reason: the two gates enforcing "no marking without an id" masked each other, so neither could be reddened alone. They are now pinned separately, one on the shape it produces and one on the claim title it leaves alone.
One unrelated failure to expect:
src/commands/review/lib/run-ledger.test.ts > refuses to append over a ledger it could not readfails on this machine at a clean checkout as well.Evidence (Before & After)
N/A — a count, a comment-body convention and their tests; no user-visible interface change.
Tested on
Environment (optional)
Unit tests only, plus a typecheck and lint of the changed files.
Risk & Scope
fresh/induced) are unchanged; this PR touches only the posting-volume trend's first-time count.Linked Issues
Closes #9674.
中文说明
本 PR 做了什么
让 review 的「首次」计数能够识别 fix-induced 重报属于新工作,从而让「携带 id」不再需要独自回答一个它已经无法回答的问题。
自 fix-induced 处置上线以来,一条携带早前轮次 id 的评论有两种含义。它可能是对一条仍然成立的 finding 的再次主张——那是重贴,计数如此对待它是正确的;也可能是在「其修复引入了该缺陷的那条条目」的 id 之下,报告一个确实是新的缺陷,之所以刻意沿用该 id,是为了让作者在一个反复打转的位点上只读一条线程,而不是每轮新开一条。只看 id 会把两者都算作重贴。现在评论自身承载了这一区分:fix-induced 重报会在作者看得见的位置被标记出来,而草拟评论的读取方把它传递给计数。
正文里的两个计数都没有改动。它们度量的是不同的东西——一个数本轮新识别的缺陷,一个数首次张贴的评论——且两种读法都正确,这也正是那两种显而易见的「调和」方式仍被排除、并在代码中记录为拒绝的原因:把携带 id 的重报排除出普查,会使归因计数落到普查之外,于是每一个这样的普查都会被当作不可能而拒绝;而把它们整体算作首次张贴,则等于告诉趋势「再次主张就是新工作」。
标记位于 id 及其分隔符之后,刻意置于 id 文法之外。该文法与 ledger 自身的 carry 共用,因此把它拓宽到吞下一个括号内容,等于把「一条 finding 的身份」压在与「模型写的一个形容词」相同的表达式上:一旦出现拓宽后的文法未曾预料的空格形态,id 就不再匹配,该 finding 会被静默重新编号——而这正是「一条 finding,一个名字」所要防止的失效。放在 id 已经到手之后读取,该 token 的任何形态都无法波及 id;相应地,读取对大小写与内部空格是宽容的,因为它只决定一条评论是否计为首次工作,绝不决定它是哪条 finding。该 token 在进入跨轮工作清单之前会被剥离,因此不会比它所描述的那一轮活得更久;而在没有 id 的位置,它就是普通的正文文字,原样保留。
为什么需要
这个偏差不是均匀的,这正是它值得修而不是容忍的原因:fix-induced 重报集中于正在打转的 PR,因此趋势基线读起来像是「新工作在减少」,而这恰恰发生在事实并非如此的那些 PR 上。在引入该处置的那个 PR 上实测:一个张贴 8 条评论的轮次——2 条确实是新的、4 条是携带 id 的 fix-induced、2 条仍然成立——记录下来的首次计数是 2。
本修复的失效方向是安全的那一侧。读取方认不出的标记,会让该草稿仍按重贴计数,也就是本机制存在之前每一轮的行为。而错误地给 still-stands 加上标记才是代价高的一侧——它会在稳态上叙述出发散——因此 skill 对该 token 作了限制,而不是把它当作「给任何携带 finding 加个醒目标签」的手段提供出去。
评审者测试计划
如何验证
运行改动过的测试套件:
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts src/commands/review/lib/convergence.test.ts,以及cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts。值得逐条阅读确认的是那个差分,因为一个从不变动的计数同样能满足单侧断言。一个夹具在同一份工作清单、同一个 id 上组装同一轮两次,唯一差别就是标记:不带标记时记录 0 条首次工作,带标记时记录 1 条,而两侧的 id 都照常 carry、作者在该位点上仍然只得到一条线程。跨轮工作清单中该条目的标题被断言为只有主张本身,token 已被剥离。
有两条性质值得刻意检查。标记绝不能让 id 丢失——这一条写成差分断言,而非「期望 id 列表」,因为该测试的第一版为一个从来就不携带 id 的形态断言了 id(共享文法要求分隔符后有空白,与是否带标记无关),并如实变红。以及标记绝不能在没有它所修饰的 id 的情况下出现:同一轮内第二条使用同一 id 的草稿会被摘掉 id,标记也随之一并摘掉。
这里每个守卫都做过变异探测——八个变异,全部变红。其中两个在第一遍是绿的,原因相同:强制执行「无 id 则无标记」的两个闸门互相遮蔽,导致谁都无法被单独触红。现在它们被分别钉住,一个钉它产生的形状,一个钉它不去改动的标题。
有一个与本改动无关的失败需要预期:
src/commands/review/lib/run-ledger.test.ts > refuses to append over a ledger it could not read在本机干净检出时同样失败。证据（改动前后对比）
N/A —— 一个计数、一项评论正文约定及其测试,没有用户可见的界面改动。
测试环境
运行环境（可选）
仅单元测试,另加对改动文件的类型检查与 lint。
风险与范围
fresh/induced)未作改动;本 PR 只触及张贴量趋势的首次计数。关联 Issue
Closes #9674。