fix(review): stop shedding the convergence observation first - #9715
Conversation
The convergence paragraph is the only thing a posted review says about the SHAPE of the loop rather than about the diff, and it was rank 0 — the second block the body-budget ladder sheds, right after the mechanism-health note. The reasoning was that an advisory paragraph decides nothing, so it should yield before the deferral list and the not-reviewed disclosures. The arithmetic does not support that ordering. Rendered bilingually the paragraph is 603 characters when only the volume signal fired, 1,510 with three recurrence clusters, and 2,372 with the clusters, the evidence caveats and the land reading together — against a body budget of 56,830. Shed second it can pay for at most 4% of an overflow, so any overflow larger than the paragraph itself spent it and then went on to spend the deferral list and the disclosures anyway. On the rounds this fires on — the high-volume ones, which is the whole point — that is the normal case: the author lost the reminder AND the disclosures. It is rank 3 now, the last rank the ladder sheds. Still ranked, not untagged: a body that genuinely cannot hold its blockers must drop an advisory, and being ranked is what makes the trim notice name it when that happens. Ranked last because it is the cheapest block to keep and the only one whose reader is the pull request's author alone — the deferral list has a second durable copy in the findings artifact, the disclosures are restated in the terminal report, and the mechanism-health note above it is written for the operator, who has the `HEALTH:` line. The test that pinned the old order is replaced by two that pin the new one: one sized to the window where the ladder sheds the disclosures and stops, asserting the paragraph survives and the notice names what actually went; one sized a rung further, asserting the paragraph goes last and is named when it does, before the hard cut. Mutation-verified: restoring `trim: 0` reddens both; making the block untagged reddens the last-and-named test (and the existing terminal-copy test); removing the rank's name from RANK_NAMES reddens only the naming assertion.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, and quantified. The trim ladder exists because production review rounds overflow GitHub's body limit — #9278 documents those rounds (PRs accumulating 1,000+ inline threads, ledgers reaching round 14). On exactly those high-volume rounds the convergence paragraph was shed second, and at 603–2,372 characters against a 56,830 budget it could absorb at most ~4% of an overflow — so any larger overflow spent it and then spent the disclosures anyway. The before/after fixture sweep shows the window where the old order lost both. This is a measured ordering defect, not a theoretical one. Direction: aligned — this is the repo's own review pipeline, tracked under #9278 (the publish-time convergence advisory design), and changes nothing about what the paragraph says or when it fires. No CHANGELOG signal needed for internal pipeline tooling. Size: not core paths ( Approach: scope feels right. Retagging the block from rank 0 to rank 3 is the minimal move — the ladder iterates ranks generically, so no mechanism change is needed. The obvious alternative (making the paragraph untrimmable) is deliberately rejected and for the right reason: when even the blockers don't fit, an advisory must yield, and staying ranked is what makes the trim notice name it. Every edit in the diff serves the reorder — the RANK_NAMES entry move, the tag change, the comment updates, and the two re-tuned tests. Nothing to split out. Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测且有量化。裁剪阶梯存在的原因就是生产环境的评审轮次会超出 GitHub 正文上限——#9278 记录了这些轮次(单个 PR 累积 1,000+ inline 线程、台账高达第 14 轮)。恰恰在这些高音量轮次上,收敛情况观察段曾是第二个被丢弃的块:它只有 603–2,372 字符,而正文预算是 56,830——最多抵消约 4% 的溢出,因此任何更大的溢出都会先丢掉它、然后照样丢掉那些披露。before/after 夹具扫描展示了旧顺序"两者皆失"的窗口。这是测算出来的排序缺陷,不是理论问题。 方向:对齐——这是仓库自有的评审流水线,归属 #9278(发布时收敛建议设计)跟踪,不改变该段落的内容与触发时机。内部流水线工具无需 CHANGELOG 信号。 规模:非核心路径( 方案:范围合理。把该块从 rank 0 改到 rank 3 是最小改动——阶梯按 rank 泛化迭代,无需改机制。显而易见的替代方案(让该段不可裁剪)被有意否决,理由也正确:当正文连阻断项都放不下时,建议性内容必须让位,而保留档位才能让裁剪通告点名。diff 中每一处改动都服务于这次重排——RANK_NAMES 条目迁移、标签修改、注释更新、两条重新调参的测试。没有可拆分的内容。 风险:无升级风险信号——未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first: for "an advisory paragraph is shed second and can absorb at most ~4% of an overflow, so the rounds it fires on lose it AND the disclosures", I would move its trim rank above the disclosure ranks while keeping it ranked — the ladder iterates ranks generically, so no mechanism change is needed — and re-pin the policy in the two sized tests. That is exactly what this PR does; I didn't find a simpler path, and the rejected alternative (making the paragraph untrimmable) is rejected for the right reason: when even the blockers don't fit, an advisory must yield, and staying ranked is what lets the trim notice name it. What I verified against the code (not just the diff):
Two non-blocking nits, both comment hygiene:
No correctness, security, or regression concerns. The mutation table and the before/after sweep in the PR body are the author's measurements; the load-bearing evidence is the two new tests in the diff, which assert the new order against sized overflow and cannot pass under the old Test evidenceThe PR's own CI on the reviewed commit, fetched via API (per policy I do not run PR code): the ubuntu unit suite — the one that carries these tests — is still in progress at the time of this pass. The macOS/Windows Test jobs and Integration Tests are merge_group-only by design in this repo's CI (ubuntu is the PR signal), so their skips are expected, not a gap. Desktop Shell (both OSes), the CVE audit, and the secret scan are green. The finalize workflow will update the table below in place once CI settles. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Real-scenario (tmux) testing: N/A — this is an unattended CI run, and the change is a deterministic string-composition reorder fully pinned by the unit tests above; there is no TUI or runtime surface for a live lane to exercise. 中文说明代码审查先看独立方案:对于"一段建议性文字第二个被丢、最多只能吸收约 4% 的溢出,导致它触发的轮次把它和披露一起丢掉"这个问题,我会把它的 trim 档位移到披露档位之上、同时保留档位——阶梯按档位泛化迭代,不需要改机制——并在两条定容测试里重新钉住策略。这正是本 PR 的做法;我没有找到更简的路径。被否决的替代方案(让该段不可裁剪)否决得也对:当正文连阻断项都放不下时,建议性内容必须让位,而保留档位才能让裁剪通告点名。 对照代码(不只是 diff)核实的内容:
两条非阻断的小问题,都是注释卫生:
无正确性、安全或回归问题。PR 正文里的变异表与 before/after 扫描是作者自己的测量;承重证据是 diff 中的两条新测试——它们按定容溢出断言新顺序,在旧的 测试证据通过 API 获取被审提交上的 CI 结果(按策略不运行 PR 代码):承载这些测试的 ubuntu 单元测试在本次审查时仍在运行中。macOS/Windows 测试与集成测试在本仓库 CI 中按设计只在 merge_group 触发(PR 信号来自 ubuntu),其跳过是预期行为,不是缺口。Desktop Shell(双平台)、CVE 审计、密钥扫描均为绿。CI 结束后,finalize 工作流会就地更新下表。 真实场景(tmux)测试:N/A——这是无人值守 CI 运行,且本改动是完全由上述单元测试钉住的确定性字符串组合重排,没有可供真实通道演练的 TUI 或运行时表面。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, test-pinned policy fix; the two nits are comment hygiene only. Stepping back: this PR matches the independent proposal I wrote before the code review — re-rank the convergence paragraph last among the trimmable blocks, keep it ranked so the trim notice can still name it, and re-pin the ordering in sized tests. I didn't find a simpler path it missed, and the one alternative it rejects (an untrimmable advisory) is rejected correctly. On the harder questions: the problem is real — the overflow case is the documented production reality of this pipeline (#9278 records rounds with 1,000+ threads and ledgers into double-digit rounds), and the arithmetic of the old ordering is deterministic, not speculative. Every hunk in the diff serves the reorder; there is no drive-by. The tests carry the policy directly — the two new ones assert the new order against sized overflow and cannot pass under the old The only thing keeping this from a 5 is the stale "sheds first" title/comment on the neighboring terminal-copy test — cosmetic, but this file records its reasoning carefully and one spot now records it wrongly. CI: the ubuntu unit suite that carries these tests is still in progress on the reviewed commit, so approval is deferred until CI lands green on 中文说明退一步整体看:本 PR 与我在代码审查前独立写下的方案一致——把收敛段重排为可裁剪块中的最后一档、保留档位以便裁剪通告仍能点名、并用定容测试重新钉住顺序。我没有找到它遗漏的更简路径;它否决的替代方案(不可裁剪的建议段)否决得正确。 更难的问题上:问题是真实的——溢出正是本流水线有据可查的生产现实(#9278 记录了单 PR 千余线程、台账轮次达两位数的回合),旧顺序的算术是确定性的,不是推测。diff 中每个 hunk 都服务于这次重排,没有夹带。测试直接承载策略——两条新测试按定容溢出断言新顺序,在旧的 没给到 5 分的唯一原因,是相邻那条终端副本测试过时的"sheds first"标题/注释——纯属表面问题,但这个文件一向认真记录其推理,有一处现在记录错了。 CI:承载这些测试的 ubuntu 单元测试在被审提交上仍在运行,因此批准推迟到 CI 在该提交上变绿为止;finalize 工作流将执行推迟的批准,若有检查变红或头部移动,会在状态评论中标明。 — 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. ✅
…onstant Two hygiene items the review caught on the trim-rank change. A test title and its comment still said the convergence paragraph is the first rank the ladder sheds — the sentence the change makes false. Three other copies of that wording were updated with the move; this one was missed. And `keeps the terminal copy on the round that actually sheds the paragraph` was sized at 55,850. Reaching a body that dropped the paragraph now means sizing past every other rank, and the window that does so without also truncating runs 55,825–56,350 — so the constant sat twenty-five characters above its own floor, with no note saying it was tuned at all. It is 56,100 now, near the middle, and carries the same retuning instruction as the two order tests. It also asserts the body was not truncated, so a future retune that overshoots reads as a failure rather than as a pass for the wrong reason. Measured, not guessed: sweeping the blocker size in 25-character steps puts the shed boundary between 55,800 and 55,825 and the truncation boundary between 56,350 and 56,375. Dropping the constant back below the window reddens the test, so it still reaches the case the terminal copy exists for.
|
Both nits fixed in 1. The stale title and comment. Fixed. 2. The 55,850 constant. Fixed, and it needed re-centring, not just a note. Sweeping the blocker size in 25-character steps puts the boundaries at: So that constant sat twenty-five characters above its own floor. A test that reaches its case by a 25-character margin is one body-copy edit from silently asserting the opposite of what it names. It is 56,100 now — near the middle of the window, the same constant as the order test above it, since they measure the same window — and it carries the same retuning instruction. It also asserts Mutation: dropping the constant back below the window reddens Re-verified after the change: 中文说明两条都已在 1. 过时的标题与注释。 已修。 2. 55,850 这个常量。 已修,而且需要的是重新居中而不只是补注释。以 25 字符步长扫描阻塞项体积,边界是:≤ 55,800 收敛观察保留、未审查披露被丢;55,825–56,350 两块都丢且未截断;≥ 56,375 连同截断。 也就是说这个常量只比它自己的下界高 25 个字符。一条靠 25 字符余量才够到目标场景的测试,离"正文文案一改就悄悄断言相反的东西"只差一步。现在是 56,100——窗口中部,与上面那条顺序测试同一个常量(它们量的是同一个窗口),并带上了同样的重新调参说明。另外它现在还断言 变异验证:把常量降回窗口以下, 改后重跑: |
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 3b": running the retuned convergence test block in compose-review.test.ts — three attempts were lost to concurrent agents' npm run build cycles racing through th…; "agent 1a": full-file run of compose-review.test.ts (the CLI-glue test at :2693 and everything outside the convergence describe block) — repeatedly blocked by a concurren….
Test Plan (not a blocker): 403 passing — this review observed 22984 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 3b":running the retuned convergence test block in compose-review.test.ts — three attempts were lost to concurrent agents' npm run build cycles racing through th…;"agent 1a":full-file run of compose-review.test.ts (the CLI-glue test at :2693 and everything outside the convergence describe block) — repeatedly blocked by a concurren…。
Test Plan(非阻断):403 passing — this review observed 22984 passed。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Six comments still asserted that the convergence paragraph is the first thing the overflow ladder sheds. The reviewer named five; a sweep found a sixth, and two of the six were already inaccurate before this branch touched anything. Corrected: - the `convergence` result-field doc — "sheds this paragraph first" - the deferral block's own rank comment — "the first thing to yield", which ranks -1 and 0 had both preceded since before this branch - the not-reviewed disclosures — "and before nothing else" - the `CONVERGENCE:` stderr line's rationale — "the first thing the overflow ladder sheds" - `save-artifact.ts`'s allow-list — "the ONE clause the overflow ladder sheds first" - the body-budget suite's own statement of the policy under test, which listed two of the four ranks Left alone, and verified correct: every "sheds first" attached to the mechanism-health note, which is rank -1 and genuinely first (`save-artifact.ts`, `save-artifact.test.ts`, the health-note terminal-copy test), and the `keep`-ordering comment about the tail cut, which is not about `trim` at all. Each corrected site keeps its own reasoning — durability, the artifact, the terminal copy — and states the ordering only as far as that reasoning needs, rather than restating the whole argument a sixth time. Six copies of one fact is what let it drift; the argument for the ordering lives at the convergence block and the others point at it. The durability rationale gets stronger, not weaker, and the comments now say so: a body that sheds rank 3 has already shed every other rank, so the terminal and artifact copies are the only ones left exactly when they fire.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): 403 passing — this review observed 22984 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:3518 — [review] unchanged "rank 3, spent first" cut comment at :3518 collides with the new trim rank 3 (convergence, shed last)
中文说明
无阻断问题。LGTM!✅
Test Plan(非阻断):403 passing — this review observed 22984 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. I rechecked the current head: the trim ladder now removes present ranks in the intended -1 → 1 → 2 → 3 order, the trim notices and persisted convergence copy stay aligned, and required blocker/cap content still reaches the hard-cut path only after every trimmable rank is gone. The exact-head checks are green, with no unresolved actionable threads.
|
Released in v0.22.0. |
|
Taking the deferred item now rather than leaving it — It is real: Every mention of the number on either axis now says which axis it is on. The cannot-tell block states the collision outright rather than just qualifying itself. Four trim-side mentions and one keep-side test comment gained the word. The one I took it despite the posture deferring it because it is the same class of drift the previous commit closed — a fact restated in more than one place, drifting when one of them moves — and shipping the ambiguity would have seeded the next round of it. Comments only, no behaviour: 98 files / 4,403 passing, Also corrected in the PR body, not the code: the Test Plan's 中文说明延后的那条我现在就取—— 问题成立: 现在两条轴上出现的这个数字,每一处都标明了它属于哪条轴。cannot-tell 块直接把这次撞名写出来,而不只是给自己加限定。四处 trim 侧与一处 keep 侧的测试注释补上了轴名。已经自行消歧的那处 尽管姿态把它延后了我仍然取它,原因是:它与上一个提交关掉的是同一类漂移——同一个事实被复述在多处,其中一处移动时就会走样——而把这个歧义发出去,等于给下一轮埋下种子。纯注释、无行为变化:98 文件 / 4,403 通过, 另外在 PR 正文(不是代码)里也订正了:Test Plan 里的 |
What this PR does
The posted review body carries one paragraph about the shape of the review loop rather than about the diff: the convergence observation — what recurred, whether the rate of first-time findings is falling, and the handling recommendations derived from those measurements. It was the second block the body-budget ladder shed, right behind the mechanism-health note. This moves it to the last rank the ladder sheds, so it survives every disclosure the budget can afford to drop instead, and goes only when dropping all of them was still not enough.
It stays ranked rather than becoming untrimmable. A body that genuinely cannot hold its blockers must drop an advisory, and being ranked is what makes the trim notice name it when that happens.
Why it's needed
The old ordering rested on "an advisory paragraph decides nothing, so it should yield first". The arithmetic does not support it. Rendered bilingually the paragraph is 603 characters when only the volume signal fired, 1,510 with three recurrence clusters, and 2,372 with the clusters, the evidence caveats and the land reading together — against a body budget of 56,830 characters. Shed second it can pay for at most 4% of an overflow, so any overflow larger than the paragraph itself spent it and then went on to spend the disclosures anyway.
That is not an edge case. The rounds this paragraph fires on are precisely the high-volume rounds most likely to overflow — that is what it is for. So on its own target population the old order lost the reminder AND the disclosures.
Which one to keep, when the budget affords exactly one, is the substance of the change. The convergence observation is the cheapest block to keep, and the only one whose reader is the pull request's author alone: the deferred-findings list has a second durable copy in the run's findings artifact, the not-reviewed disclosures are restated in the terminal report, and the mechanism-health note ranked above it is written for the operator, who has the
HEALTH:terminal line. The convergence paragraph is the whole of what this pipeline tells a PR author about a loop that is not settling.Reviewer Test Plan
How to verify
npx vitest run packages/cli/src/commands/review— 98 files, 4,403 passing. That count is this directory only, which is the whole of what this change can reach; a full-repo run reports ~23,000 and is the same signal at a wider scope. Two tests carry the policy directly, both inconvergence diagnosis reaches the POSTED body:outlives every disclosure the ladder can shed— sized to the window where the ladder sheds the not-reviewed disclosures and stops; asserts the paragraph survives and the trim notice names the disclosures rather than the paragraph.still yields — last, and named — when shedding the rest was not enough— sized one rung further; asserts the paragraph goes and the notice names it, before the hard cut.Both constants are tuned rather than round, and each test says how to retune it after a body-copy change.
Mutation-verified, one named test each:
trim: 0(the old ordering)still yields — last, and named …, plus the existingkeeps the terminal copy on the round that actually sheds the paragraphRANK_NAMESstill yields — last, and named …Evidence (Before & After)
Same fixture through both trees, sweeping the blocker size across the overflow region.
convis the convergence observation,notrevthe not-reviewed disclosures.Read honestly: below the region both bodies are whole; inside it the two orders swap which block the author keeps; above it both orders shed both blocks — which is the "spent and bought nothing" case the size measurement predicts, and it is visible from 55,900 up, where the old order shed the paragraph and still had to shed the disclosures.
The width of the swap window is the size of the ranks below the paragraph. This fixture's only lower rank is a not-reviewed block of comparable size, so the window is narrow here; a body also carrying a deferred-findings list — often several KB — widens it by that much.
Tested on
Environment (optional)
Unit tests only (
vitest), plustsc --noEmitonpackages/cli.Risk & Scope
RANK_NAMESgains a rank-3 entry and loses its rank-0 one;bodyTrim.deferralListand the artifact pointer key on rank 1 and are untouched.Linked Issues
Refs #9278
中文说明
本 PR 做了什么
发布的评审正文里有且只有一段是在讲评审回路的形态而非 diff 本身:收敛情况观察——什么在反复出现、首次提出的产出速度是否下降,以及由这些测量推导出的处置建议。它原本是正文预算阶梯第二个被丢弃的块,紧跟在机制健康说明之后。本 PR 把它移到阶梯最后一档:预算能丢的其它披露都丢完之后,它才会走。
它仍然是"有档位"的,而不是不可裁剪。如果正文连阻塞项都放不下,建议性内容本就该让位;而保留档位正是让裁剪通告在它真被丢弃时能点名说出来的原因。
为什么需要
旧定级依据是"一段不决定任何事的建议应当先让位"。算术不支持这个排序。双语渲染下,这段在只有 volume 信号时是 603 字符,带三个复发簇时 1,510,簇 + 证据说明 + land 读数齐全时 2,372——而正文预算是 56,830 字符。放在第二位丢,它最多只能抵消 4% 的溢出;因此任何比它自身更大的溢出,都会先花掉它、然后照样继续花掉那些披露。
这不是边缘情况。这段文字触发的正是最容易溢出的高产出轮次——它本来就是为这种轮次存在的。所以在它自己的目标人群上,旧顺序把提醒和披露一起丢了。
当预算只够留一个时该留哪个,是本 PR 的实质。收敛观察是最便宜的块,也是唯一读者只有 PR 作者的块:延后发现清单在本次运行的 findings 工件里另有一份持久副本,未审查披露会在终端报告中重述,而排在它上面的机制健康说明是写给操作者看的,操作者有
HEALTH:终端行。这段文字是本流水线告知 PR 作者"回路没有收敛"的全部。复核方式
npx vitest run packages/cli/src/commands/review——98 个文件、4,403 条通过。该计数仅限本目录(也就是本改动能触及的全部范围);全仓运行约 2.3 万条,是同一个信号的更大口径。直接承载该策略的是两条测试(见英文正文),两个常量都是调出来的而非取整,每条测试都写明了正文文案变动后如何重新调参。变异验证见上表,每条变异只让点名的测试变红。证据(前后对比)
同一夹具在两棵树上跑,扫描阻塞项体积穿过溢出区(表格见上)。如实解读:区间以下两版正文都完整;区间之内,两种顺序交换了作者能留住的那一块;区间以上两种顺序都会把两块一起丢——这正是体积测算所预言的"花了却没买到任何东西",从 55,900 起可见:旧顺序丢了这段文字,仍然不得不再丢披露。
交换窗口的宽度等于排在它下面的那些档的体积。本夹具唯一的下位档是体积相近的未审查披露,所以这里窗口很窄;如果正文同时带着延后发现清单(常有数 KB),窗口会相应变宽。
风险与范围
RANK_NAMES新增 rank 3 条目、移除 rank 0 条目;bodyTrim.deferralList与工件指针以 rank 1 为键,未受影响。关联 issue
Refs #9278