feat(review): record each round's posting volume in the ledger marker - #9413
feat(review): record each round's posting volume in the ledger marker#9413wenshao wants to merge 6 commits into
Conversation
The convergence advisory design (#9278) needs one thing before it can diagnose anything: a per-round measurement of how much this account actually posts. That number exists nowhere durable today — a comment count on the PR cannot separate this pipeline's rounds from anyone else's, and asking the model for it would put a policy input in the one place the design refuses to trust. So the marker carries it. `posted` is the size of the posting set AFTER floor enforcement — what `submit` sends, and therefore what the next round will find on the pull request — and `prevPosted` carries the predecessor's forward, so a single marker holds the shortest window in which 'still shrinking' is a statement rather than one step. Both come out of the same side-file read the round number does, because a marker pairing one round's number with another's count is a trend nobody can read back. These are the only fields here that decide nothing. Every other one gates something and therefore fails closed; these are read by no gate, so they fail open (absent) and — unlike the anchor pair — survive a truncated work list and a fail-closed round, which is exactly when a volume trend is worth having. Zero is recorded rather than dropped: a converged round is the observation, and a predecessor that recorded nothing is not one that posted nothing. The compose report states the round's volume beside the previous one. Facts only — no threshold, no advice, no judgement about whether the number is large. The operator owns that reading; the diagnosis that consumes the trend is the next work item.
|
Re-run after four review-pipeline rounds and a merge of main — gate re-checked against the current head. Template: complete, with the bilingual summary ✓ Problem: real and tracked. There is no durable record anywhere of how many inline comments a review round posted, and this is the second work item of the convergence-advisory design (#9278), building on the floor enforcement that already merged (#9279). The description correctly rules out the two obvious alternatives — counting PR comments can't separate this account's rounds from anyone else's, and reading the count back from the model puts a policy input where the design refuses to trust it. Direction: aligned. Shipping the measurement alone — without the diagnosis that consumes it — is the right split: the telemetry starts accumulating history instead of waiting on its consumer. Size: 286 production lines across four files (compose-review.ts, ledger.ts, pr-context.ts, save-artifact.ts) and 479 test lines across four test files. Approach: minimal. The fields ride the existing ledger marker — the round's only durable record — one shared volume reader ( Risk: no elevated risk signals — no high-risk path matches. Moving on to code review. 🔍 中文说明四轮 review 流水线修订并合并 main 之后的重新运行——对当前头提交重新过门。 模板:完整,含双语说明 ✓ 问题:真实且有据可查。目前没有任何持久化记录记载一轮评审发布了多少条行内评论,本 PR 是收敛咨询设计(#9278)的第二项工作,建立在已合并的底线强制(#9279)之上。PR 描述也正确排除了两个显而易见的替代方案——统计 PR 评论数无法把本账号的轮次与其他评审者区分开,而从模型回读计数则把策略输入放进了该设计明确不信任的地方。 方向:对齐。只交付度量、不带消费它的诊断,是正确的切分——遥测可以立即开始积累历史,而不必等待其消费方。 规模:生产代码 286 行,分布在四个文件(compose-review.ts、ledger.ts、pr-context.ts、save-artifact.ts);测试 479 行,分布在四个测试文件。 方案:最小改动。字段搭载在既有的台账标记上——轮次唯一的持久记录——一个共享的音量读取器( 风险:无升级风险信号——未命中任何高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review. Before reading the diff, my independent take on "record a trustworthy per-round posting volume" was: extend the ledger marker (the round's only durable record) with a post-enforcement count, carry the predecessor's figure forward from the persisted side file, validate on both write and read through one shared reader, fail open since nothing gates on it yet, and keep zero distinct from absence. The PR lands exactly that shape, and then answers one question I hadn't thought to ask — what happens when the ~27 bytes of volume push a marker that fit with its anchor over the byte budget — by making volume the first thing the shed cascade gives up. Verified in this pass, including everything the earlier rounds flagged:
No blockers, and nothing I'd simplify. The one residual is round 4's own deferred probe (the artifact validator's clamp branch isn't pinned by a >cap test); Testing — the PR's own CI on
Sandboxed verification: the central claim — the marker carries the post-enforcement posting volume through every boundary — is substantiated by the unit suite itself, which exercises the real compose/serialize/parse/persist paths with real file I/O, not mocks. The
中文说明代码审查。 在读 diff 之前,我对"记录可信的每轮发布音量"的独立设想是:在台账标记(轮次唯一的持久记录)上扩展一个强制执行后的计数,从持久化侧文件携带前一轮的数字向前传递,写入与读取两侧通过同一个共享读取器校验,因尚无任何门消费而 fail open,并让零与缺失保持区分。PR 落地的正是这个形状,并且还回答了一个我没想到的问题——当约 27 字节的音量把一个带着锚点恰好放下的标记推过字节预算时怎么办——答案是让音量成为让位级联中第一个被放弃的东西。 本次核实的内容(含此前各轮提出的所有问题):
无阻断项,也没有我会简化的地方。唯一遗留是第 4 轮自己延后的探针(artifact 校验器的 clamp 分支没有 >上限的测试钉住); 测试—— 上方表格为头提交上的真实 check 结果;CI 结束后 finalize 任务会就地更新该区域。 沙箱验证:核心主张——标记携带强制执行后的发布音量穿过所有边界——已由单元套件本身证实,测试走的是真实的 compose/serialize/parse/persist 路径与真实文件 I/O,而非 mock。搭载本次
— Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — four review rounds drove this to a place where I have nothing left to ask for. Stepping back: the PR does one thing and does it the way I would have done it — the count lives in the marker because the marker is the round's only durable record, it measures what Housekeeping note for the maintainer: the standing CHANGES_REQUESTED state on this PR is the round-2 review artifact against CI on the reviewed head is green (both 中文说明置信度:5/5 —— 四轮评审之后,我没有任何进一步的要求。 退一步看:这个 PR 只做一件事,而且用的正是我会采用的做法——计数放在标记里,因为标记是轮次唯一的持久记录;它度量 给维护者的事务性说明:本 PR 当前挂着的 CHANGES_REQUESTED 状态是第 2 轮评审在 被审头提交上的 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 — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…s (review round 1) Round-1 findings, all seven addressed; three shared a root. The volume shape check had been restated at three boundaries and only two of them applied the cap, so a side file carrying an over-cap count made one compose report an uncapped number while its own marker recorded the capped one. There is now a single exported reader that validates AND clamps, and the serializer, the parser, the side-file recovery and the persisted-verdict validator all call it. The round's volume likewise has one origin: the body composer already returns it, so the marker takes that value instead of re-deriving the same expression a few lines later — an invariant that held only by two expressions being textually identical is not an invariant. The volume now travels with its round or not at all. A side file with a usable count but no usable round degraded to round 0 and still handed the count forward, so a round-1 marker asserted a `prevPosted` for a round that never ran; and `pr-context`'s anonymous-recovery branch advanced the round past a foreign marker while keeping this account's stale count, fabricating a trend point. Both now drop the volume exactly where they already drop the anchor and the age reference, for the same reason: each is a fact about a specific round. The rest is the coverage those fields were missing. `lib/ledger.test.ts` — the marker's untrusted-input boundary — now pins the round trip, the parse-side refusal of hand-crafted volumes (its real input domain is a body any account can write), the cap on both sides, and survival across a truncated work list. save-artifact gains the sibling pair its three other validators have, and the `VOLUME:` line is asserted in both of its branches.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:4131 — [review] VOLUME-line comment claims a report-only round's volume is the next round's trend reference, but prevPosted only comes from the side file written from posted reviews
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…inds (review round 2) Round-2 findings, both addressed. The Critical: the volume fields were written unconditionally and sat in no stage of the marker's byte-budget cascade, which sheds the anchor pair and then trims the work list. A ledger that fitted WITH its anchor could therefore cross the 8 KiB cap on the ~27 bytes of telemetry alone, and the re-render paid with the anchor — trading the next round's incremental scope (measured in this module's own docstring at ~119 minutes and ~34M tokens) for a trend line that, by this field's documented contract, decides nothing. The cascade now sheds the volume FIRST, before the anchor and long before a finding: absence is this field's free, documented reading, while everything below it buys something the next round spends. Pinned by a sweep over the reported pressure region that asserts a marker keeping its anchor without volume still keeps it with volume — and it fails under the old order. The Suggestion: the anonymous round-advance branch's volume drop was inert under mutation because the only fixture exercising that branch carried no volumes. It carries them now, and deleting the drop turns the test red.
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: verify, reverse-audit — the agent returned no evidence of its walk twice.
Not explored to full depth (tool budget reached): "agent 5": NO Budget gap: line — all checks completed within budget.; "agent 4": NO — nothing was cut short.; "agent 1b": NO Budget gap: line — no checks were cut short.; "agent 3a": NO budget gap — all checks completed within the budget..
Not reviewed: verification and reverse audit — neither the verifier nor the reverse auditor was launched with a prompt this skill builds — the posted findings were ruled on, and the misses the rest of the review left were hunted, if at all, without the briefs this skill certifies against.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:verify、reverse-audit——该 agent 连续两次未返回任何检查过程的证据。
未探索到全部深度(达到工具调用预算):"agent 5":NO Budget gap: line — all checks completed within budget.;"agent 4":NO — nothing was cut short.;"agent 1b":NO Budget gap: line — no checks were cut short.;"agent 3a":NO budget gap — all checks completed within the budget.。
未审查:验证与反向审计——验证 agent 与反向审计 agent 都没有用本 skill 构建的 prompt 启动——发布的发现即便被裁定过、评审其余部分遗漏的问题即便被搜寻过,也都缺失了本 skill 用以认证的 brief。
— qwen3.8-max via Qwen Code /review (v0.21.13)
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-1 anonymous-merge volume strip has no regression test — already reported (comment 3806345586 by qwen-code-ci-bot at pr-context.ts:1066, comment 3806602126 by doudouOUC at pr-context.ts:1068); author replied fixed in 214e7d9 (fixture now…
Unresolved, please confirm:
- [Critical] lib/ledger.ts (qwen-code-ci-bot comment 3806345583, round-2 Critical): volume key bytes can evict the anchor pair at the LEDGER_MAX_BYTES boundary — author claims fixed in 214e7d9 (volume becomes the first shed stage, sweep-pinned), but th…
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未决,请确认:共 1 条(原文未翻译,列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.11)
…eview round 4) Round-4 findings, both addressed, and the second is this PR marking its own inconsistency. The persisted verdict defaulted an absent `postedInline` to zero the way its siblings default theirs — but their defaults are TRUE of a round that predates them (it deferred nothing, enforced nothing, trimmed nothing), while a pre-telemetry round DID post comments, so zero asserted a count nobody observed and made that round indistinguishable from a genuinely converged one. That is precisely the zero-versus-absent conflation this field refuses at every other boundary it touches, and `lowSignal` in the same function already persists null rather than inventing a default. Absence is now preserved; the artifact type makes the field optional at that boundary alone, since a live compose always knows the number. The other half of the same distinction was unpinned at the display boundary: the terminal line's ternary survived a falsy-check mutation, so a converged predecessor's recorded zero would silently print as no predecessor at all. Both arms are pinned now — a recorded zero persists and displays, an unrecorded one stays absent — and each fix fails under its own mutation.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/ledger.ts:91 — [review] posted docstring's "ONLY field here that decides nothing" uniqueness claim is contradicted by prevPosted's same decides-nothing contract in the same diff (unchanged since the prev…
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…uards that were vacuous (review round 5) Round-5 findings, all six addressed. Two changed behaviour. The byte cascade shed both volumes as one unit, so in the band where this round's own count still fits it went out with the carried one — and `posted` is the next link in the chain, the value the next compose reads back and stamps as its own `prevPosted`, so the chain broke a round earlier than the budget required. The carried value now sheds first and `posted` survives a rung longer. And `PersistedVerdict` inherited `prevPostedInline` through an `Omit` that stripped only its sibling, advertising a field the validator neither reads nor writes; the type now omits both, and says why. The rest is coverage that was not covering. The anti-vacuity guard added last round counted the wrong population — every window whose anchor survived without volume, which a sweep satisfies a hundred times while executing the shed cascade zero times; it now counts windows actually under byte pressure, and narrowing the sweep off that band fails the test. The write-side clamps were unobserved because the clamp test round-tripped through a parser that clamps independently, so the raw serialized text is asserted now. And volume survival on the recovery-threw path — where the round counter stands, so the count still describes it — is pinned, since generalising the anonymous branch's drop to there would have shipped green.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/save-artifact.test.ts:400 — [probe] Artifact validator's volumeOf clamp branch has no test — an unclamped shape check survives the whole suite
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /resolve |
…ce-telemetry # Conflicts: # packages/cli/src/commands/review/compose-review.ts
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
Each posted review round now records how many inline comments it put on the pull request, carried in the machine-readable marker the review body already embeds.
postedis the size of the posting set after the severity floor has moved anything it moves — the setsubmitactually sends, and therefore what the next round finds on the pull request — andprevPostedcarries the previous round's figure forward, so a single marker holds the two-round window in which "still shrinking" is a statement rather than a single step. Both come out of the same side-file read the round number does, because a marker that paired one round's number with another's count would describe a trend nobody could read back. The composed result carries the same two numbers for the terminal report, which states this round's volume beside the previous one.These are the only fields in the marker that decide nothing. Every other one gates something — the finding list is the next round's work list, the anchor and its model qualifier scope that round's diff, the dropped count withholds that scoping — and each therefore fails closed. These are read by no gate, so they fail open: an absent or malformed value costs a point on a trend line and nothing else. For the same reason they behave differently from the anchor in two places where that difference matters: they survive a truncated work list, and they survive a round that could not certify its scope. A volume trend that went blank exactly when a pull request started capping would be blind on the rounds it exists to describe. Zero is recorded rather than dropped, because a round that posted nothing is precisely the observation a convergence trend is looking for, and a predecessor that recorded no volume at all is not the same claim.
Why it's needed
The convergence advisory design tracked in #9278 rests on measuring whether a review loop is converging, and the first thing it needs is a per-round posting volume that can be trusted. That number exists nowhere durable today. Counting comments on the pull request cannot separate this pipeline's rounds from any other reviewer's, and reading the count back from the model would put a policy input in the one place the design refuses to trust it. Recording it where the round already records its work list makes it recoverable from the pull request itself, in any environment, by any later round — which is the same property that made the ledger the right home for the finding list and the anchor.
This ships the measurement alone, deliberately. The diagnosis that consumes the trend is the next work item, and keeping the two apart means the telemetry can start accumulating history immediately rather than waiting on the surface that reads it.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts src/commands/review/save-artifact.test.ts src/commands/review/submit.test.ts— the new block ("convergence telemetry — volume, carried in the marker") pins the properties above: the recorded number is the post-enforcement posting set rather than the drafts (a round whose floor moves two of three records one), the predecessor's figure rides forward into the next marker, zero survives as a value, a predecessor that recorded nothing stays distinguishable from one that posted nothing, a malformed side-file volume costs the trend point without costing the round, and a round whose anchor is withheld still carries its volume."posted": N, and from the second round"prevPosted": M; the terminal report gains aVOLUME:line stating both.src/commands/reviewsuite: 3622 pass, 1 fail —script-lint-isolationfails under concurrency and passes alone, and it fails identically on a clean checkout ofmain(verified by stashing this branch's changes and re-running), so it is pre-existing and unrelated.Evidence (Before & After)
N/A — the change is a machine-readable field inside an HTML comment plus one terminal line; both are asserted directly in the tests.
Tested on
Environment (optional)
Unit tests only;
tsc --noEmit, eslint and prettier clean.Risk & Scope
Linked Issues
Refs #9278 (second work item of the convergence-advisory design). Builds on #9279 (the floor enforcement whose post-enforcement set defines the recorded number).
中文说明
本 PR 做了什么
每一轮已发布的评审现在会记录它在 PR 上新增了多少行内评论,写在评审正文已有的机器可读标记里。
posted是严重性下限完成改道之后的发布集大小——即submit真正发出、下一轮会在 PR 上看到的那一份——而prevPosted把上一轮的数字带上,于是单个标记就持有"仍在收缩"所需的两轮窗口(否则那只是单步)。两者取自与轮次编号同一次侧文件读取:若标记把某轮的编号配上另一轮的计数,趋势就无法回读。组合结果同样携带这两个数字供终端报告使用,终端会把本轮音量与上一轮并排陈述。这是标记中唯一不决定任何事的两个字段。其余每个字段都在把关——发现清单是下一轮的工作清单,锚点及其模型限定界定该轮的增量范围,截断计数会撤销该界定——因此它们全部 fail closed。这两个不被任何门读取,所以 fail open:缺失或畸形只损失趋势上的一个点。同样的理由让它们在两个关键处与锚点行为不同:工作清单被截断时它们保留,轮次无法认证其范围时它们也保留。一条恰好在 PR 开始触发上限时变空白的音量趋势,会在它最该描述的那些轮次上失明。零被记录而非丢弃:发布为零的轮次正是收敛趋势要找的观测,而"完全没有记录音量的前一轮"并不是同一个断言。
为什么需要
#9278 跟踪的收敛建议设计立足于测量回路是否收敛,而它首先需要一个可信的每轮发布量。这个数字目前在任何持久位置都不存在:数 PR 上的评论无法把本管线的轮次与其他评审者区分开,而从模型读回这个数会把政策输入放回设计明确拒绝信任的地方。把它记录在轮次已经记录工作清单的地方,意味着它可从 PR 自身、在任何环境、由任何后续轮次恢复——这正是当初让台账成为发现清单与锚点归宿的同一性质。
本 PR 刻意只交付测量本身。消费该趋势的诊断是下一个工作项;两者分开意味着遥测可以立即开始积累历史,而不必等待读取它的界面。
审查者测试计划
验证方式:
cd packages/cli && npx vitest run src/commands/review/compose-review.test.ts src/commands/review/save-artifact.test.ts src/commands/review/submit.test.ts——新增用例块(convergence telemetry)钉住上述性质:记录的数字是强制执行后的发布集而非草稿集(下限移走三条中的两条时记为一)、上一轮数字随标记前传、零作为值存活、"未记录"与"发布为零"保持可区分、侧文件中畸形的音量只损失趋势点而不损失轮次、锚点被扣留的轮次仍携带音量。预期:任何已组合的 PR 评审,其标记新增"posted": N,从第二轮起新增"prevPosted": M;终端报告新增陈述两者的VOLUME:行。撰写期间做了变异检查:把零当作缺失会使收敛轮用例变红,空操作对照变异保持绿。完整src/commands/review套件 3622 通过、1 失败——script-lint-isolation在并发下失败、单独运行通过,且在main的干净检出上同样失败(已通过暂存本分支改动并重跑验证),属既有问题且与本改动无关。证据:N/A——改动是 HTML 注释内的机器可读字段加一行终端输出,两者均在测试中直接断言。已在 macOS 验证;Windows/Linux 未本地验证(CI 覆盖)。环境:仅单元测试;
tsc --noEmit、eslint、prettier 全部干净。风险与范围
关联 Issue
参考 #9278(收敛建议设计的第二个工作项)。基于 #9279(其强制执行后的发布集定义了本 PR 记录的数字)。