fix(review): clamp the posting volume at its origin, not only where it is written - #9460
Conversation
…t is written Follow-up to #9413, from a maintainer's independent pass. The terminal `VOLUME:` line was the one read site of the round's count that skipped the shared volume reader: the serializer, the parser and the side-file recovery all clamp, but `result.postedInline` carried the raw drafted- comment length, so in the defensive over-cap case the line would print an uncapped number beside a marker recording the capped one — the two- outputs-disagree failure that reader's own docstring exists to prevent. Clamping once where the count is derived puts every surface on the same value. Not reachable with any real producer (no round drafts 100k inline comments), so this closes a consistency gap rather than a live bug. Also corrects a fixture comment that survived the round-4 semantics reversal: `postedInline`'s absence is preserved, not defaulted to zero, and the comment beside it still claimed the opposite of what the validator, the type and two tests all say.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template: complete, bilingual summary included ✓ Problem: real and recorded, not speculative. Both changes trace to named follow-ups from the review of #9413: the maintainer approval there explicitly lists "the terminal Direction: aligned. This is internal telemetry of the review pipeline's convergence design (#9278), and the direction was confirmed when #9413 merged with maintainer approval. CHANGELOG: no direct reference, but the area is the repo's own review tooling. Size: no core paths touched — not applicable. For context, 37 lines total: 10 production (one expression in Approach: scope is exactly right. Clamping at the single derivation point puts all four surfaces on the same value by construction instead of relying on each boundary to clamp, and the fixture comment now matches what the validator, the type, and the tests assert. Nothing to cut. Risk: no high-risk path matches. No elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! **模板:**完整,含中文总结 ✓ **问题:**真实且有记录,不是推测性加固。两处改动都可以追溯到 #9413 评审中点名的后续项:该 PR 的维护者 approve 评论明确把「终端 **方向:**对齐。这是 review 流水线收敛设计(#9278)的内部遥测,且方向在 #9413 经维护者批准合入时已确认。CHANGELOG:无直接引用,但该领域是仓库自身的 review 工具。 **规模:**未触及核心路径——不适用。供参考,共 37 行:10 行生产代码( **方案:**范围恰好。在唯一推导点钳制,使四个界面在构造上取同一个值,而不是依赖每个边界各自钳制;夹具注释现在与校验器、类型和测试的断言一致。无可裁剪。 **风险:**高风险路径无匹配。无升级风险信号。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review. Before reading the diff, my independent take on "clamp the posting volume at its origin" was: find the one derivation site that bypasses the shared
No blockers, no convention violations. The production change is one expression and its comment. Testing. CI on the reviewed commit as of this pass — Desktop Shell (both OSes) and the secret scan are green; the Linux unit suite (the one that runs the new test) is still in flight, and the macOS/Windows unit + CLI-integration skips match the repo baseline. No failing checks, so no log excerpt to quote. The table below updates in place once CI settles. This is a unit-test-only change with no user-visible surface — the divergence it closes needs a 100,005-comment review round that no real producer generates — so the unit suite is the right evidence and no real-scenario run applies. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明**代码审查。**在读 diff 之前,我对「在源头钳制发布音量」的独立方案是:找到唯一绕过共享
无阻断问题,无规范违规。生产改动只有一处表达式及其注释。 **测试。**本次审查时被审提交的 CI——Desktop Shell(双平台)与密钥扫描为绿;承载新测试的 Linux 单元测试套件仍在运行;macOS/Windows 单测与 CLI 集成的跳过与仓库基线一致。无失败检查,故无日志摘录。下表在 CI 结束后原地更新。这是无用户可见界面的纯单测改动——它闭合的分歧需要一轮 100,005 条评论的评审,而真实生产者不会产生这种输入——因此单测套件即正确证据,不适用真实场景运行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — a one-line invariant completion I would have written the same way, with a test that fails if you remove it. Stepping back: this PR closes the last gap in the agreement the #9413 shared reader was built to guarantee. My independent proposal before reading the diff — clamp at the one derivation site that bypasses the reader, pin it with an over-cap test, fix the stale comment — is exactly what landed. Nothing was added that did not need to be: one production expression, one test, one comment. The clamp is a no-op for every count a real round produces, it cannot lower a value below what the marker already recorded, and the test makes the two-surfaces-agree property fail loudly if anyone undoes it. If I had to maintain this in six months, the comment tells me exactly why the line is shaped that way. The Linux unit suite is still in flight on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度:5/5 —— 一行不变量补全,我会用同样的方式写,且附带的测试在改动被移除时会失败。 退一步看:这个 PR 闭合了 #9413 共享读取器所要保证的「各界面一致」的最后一个缺口。我在读 diff 之前的独立方案——在唯一绕过读取器的推导点钳制、用超上限测试钉住、修正过期注释——与落地的完全一致。没有添加任何不必要的东西:一处生产表达式、一个测试、一条注释。对任何真实轮次产生的计数该钳制都是空操作,它不会把值降到标记已记录值之下,且一旦有人撤销它,测试会让「两个界面一致」这一性质大声失败。六个月后维护这段代码时,注释会准确告诉我这一行为什么是这个形状。 被审提交上的 Linux 单元测试套件仍在运行,因此批准推迟到 CI 在该提交上变绿之后。 — Qwen Code · qwen3.8-max Reviewed at |
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.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 5": execute the new over-cap test to confirm it passes and its runtime (worktree and parent checkout have no node_modules or built dist; a full npm ci + build excee….
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 5":execute the new over-cap test to confirm it passes and its runtime (worktree and parent checkout have no node_modules or built dist; a full npm ci + build excee…。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…tics The rewritten `postedInline` comment broke what the line below it refers to. The fixture's comments used to read as one chain — every field non-default because absence defaults to something — so `bodyTrim`'s "for the same reason" resolved correctly. With `postedInline` now documenting the OPPOSITE reason (its absence is preserved, not defaulted), the nearest antecedent teaches a maintainer that `bodyTrim` is preserved too, while the validator defaults it and always emits it. The comment now states its own side of the split instead of pointing at whichever comment happens to precede it.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
Released in v0.21.15. |
What this PR does
Applies the shared volume reader at the one site that was still missing it, and corrects a fixture comment that outlived the semantics it described.
The round's posting count reaches four surfaces: the ledger marker, the persisted artifact, the terminal
VOLUME:line, and the next round through the side file. Three of them read it through the shared reader that validates and clamps in one place; the fourth — the count as the body composer derives it — carried the raw drafted-comment length. In the defensive over-cap case the terminal line would therefore print an unclamped number beside a marker recording the clamped one, which is precisely the two-outputs-disagree failure that reader's own docstring cites as its reason for existing. Clamping once where the count is derived puts every surface on the same value by construction rather than by four boundaries agreeing to do the same thing.The fixture comment is smaller but the same species:
postedInline's absence is preserved rather than defaulted, decided a few rounds into the previous PR, and the comment beside the fixture still told the reader that absence defaults to zero — the opposite of what the validator, the type and two tests all assert.Why it's needed
Neither is reachable with a real producer: no review round drafts a hundred thousand inline comments, and a comment misleads only the next person to read it. They are worth closing because the value they protect is agreement between surfaces — a convergence trend is only as good as its points, and a count that reads differently depending on which output you look at is exactly the kind of quiet disagreement the shared reader was introduced to make impossible. Leaving one site outside it also leaves the invariant resting on nobody adding a fifth.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts— the convergence-telemetry block gains a case driving the over-cap shape and asserting the result field and the marker report the same clamped value. The existing cases continue to pin the ordinary path, so a regression that clamps only at the write side still fails.save-artifact.test.ts: its comment now states the preserved-absence contract the surrounding tests assert, rather than the defaulting one they refute.src/commands/reviewsuite: 3901 pass.tsc --noEmitclean after a workspace build (a stalecore/acp-bridgedistreports unrelated errors inserve/; rebuilding clears them).Evidence (Before & After)
N/A — one clamp and one comment; the observable difference is asserted directly in the new test.
Tested on
Environment (optional)
Unit tests only;
tsc --noEmit, eslint and prettier clean.Risk & Scope
Linked Issues
Follow-up to #9413 (raised in a maintainer's independent pass over that PR's head, marked non-blocking there). Refs #9278.
中文说明
本 PR 做了什么
在唯一遗漏的读取点应用共享音量读取器,并修正一处比它所描述的语义活得更久的夹具注释。
本轮的发布计数会到达四个界面:台账标记、持久化产物、终端
VOLUME:行,以及经由侧文件到达的下一轮。其中三个通过共享读取器读取——该读取器在一处完成校验与钳制;第四个(正文组合器推导出该计数的地方)携带的是未经处理的草稿评论数。于是在防御性的超上限场景下,终端行会打印未钳制的数字,而同一轮的标记记录的是钳制后的值——这恰恰是该读取器自身 docstring 用以论证其存在理由的「同一轮两个输出互相矛盾」的失败形态。在计数被推导处钳制一次,使所有界面在构造上取得一致,而不是依赖四个边界各自记得做同一件事。夹具注释这条更小,但属于同一类:
postedInline的缺失是被保留而非默认填零的(这是上一个 PR 中途确定的语义),而夹具旁的注释仍然告诉读者「缺失默认为 0」——与校验器、类型以及两个测试所断言的正好相反。为什么需要
两者对真实生产者都不可达:没有哪一轮评审会起草十万条行内评论,而注释只会误导下一个阅读它的人。值得关闭它们,是因为它们保护的是界面之间的一致:收敛趋势的价值不高于其数据点的可信度,而一个「取决于你看哪个输出」的计数,正是引入共享读取器所要杜绝的那种静默分歧。把一个站点留在读取器之外,也就把不变量寄托在「没人添加第五个站点」上。
审查者测试计划
验证方式:
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts——收敛遥测测试块新增一个用例,驱动超上限形态并断言结果字段与标记报告同一个钳制后的值;既有用例继续钉住常规路径,因此「只在写入侧钳制」的回归仍会失败。另请阅读save-artifact.test.ts中的夹具:其注释现在陈述的是周围测试所断言的「保留缺失」契约,而非它们所否定的「默认填零」。完整src/commands/review套件 3901 通过;工作区构建后tsc --noEmit干净(陈旧的core/acp-bridgedist会在serve/报出无关错误,重建即可消除)。证据:N/A——一处钳制与一条注释,可观测差异已在新测试中直接断言。已在 macOS 验证;Windows/Linux 未本地验证(CI 覆盖)。环境:仅单元测试;
tsc --noEmit、eslint、prettier 干净。风险与范围
关联 Issue
#9413 的后续(由维护者对该 PR head 的独立评审提出,在那里标记为非阻断)。参考 #9278。