perf(review): extend the convergence pair to 3B (chunked) reviews - #8903
Conversation
The reverse-audit loop is 77-80% of the wall clock on long CI reviews (measured on two v0.21.9 runs: 291min with 223min in the RA loop, 252min with ~205min), and on 3B its rounds ran serially because the convergence pair — rounds 1 and 2 launched together — was 3A-only. The pair's arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative findings list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit. Launching `--all-chunks --round 1` and `--all-chunks --round 2` in one response runs each chunk's two establishing audits concurrently, saving one round's wall (~30-56min) off every chunked review — at the same one-round suppression window the 3A pair and the pipelined loop already accept. Orchestration-only: the CLI already builds round 2 before round 1's transcripts exist (round 2 always fans out to every chunk; the retirement schedule only reads history from round 3), and the deadline gate prices the paired round-2 admission on its 600s floor exactly as the 3A pair relies on. A new agent-prompt test pins that mechanism; SKILL.md carries the per-chunk pair, and DESIGN.md the measurement.
|
Re-run on Template looks good ✓
Moving on to code review. 🔍 中文说明在 模板完整 ✓
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first, written before re-reading the diff: the underpricing Critical wants the pair's second member priced for both members' wall — detect "predecessor still in flight" from the freshness of its stamp, and scale the single-round estimate by the wave ratio ceil(2C/N)/ceil(C/N) on the runtime's tool-concurrency pool. That is exactly what landed in
Non-blocking notes: the body's "no change to the deadline gate or any CLI logic" line is stale and should be updated; the acp-integration.test.ts hunk is formatting-only; the DESIGN.md run-attribution thread is still open with the author; the deferred follow-ups (hoisting the pool constant's four copies, width-wiring tests at the 3A/ TestingEvidence carried: the PR's own CI checks on the reviewed commit — all completed, none failing.
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified: the wall-clock saving itself. The suite pins the builder mechanism, the gate's pair pricing, and the prose cross-references — but no test observes a real 3B run launching rounds 1+2 in one response; the saving is a prose-driven behavior of the orchestrating model. Sandboxed verification would settle this: Real-scenario testing: N/A — nothing user-visible; the behavioral surface is 中文说明代码审查（读 diff 前先独立写下方案：underpricing 的 Critical 需要把收敛对第二成员按两个成员的 wall 计价——用前任 stamp 的新鲜度判断"前任仍在飞行"，再按运行时 tool-concurrency 池上的波数比 ceil(2C/N)/ceil(C/N) 缩放单轮估值。落地的
不阻塞的提醒：正文"不改动 deadline 门或任何 CLI 逻辑"已过时，应更新；acp-integration.test.ts 是纯格式化改动；DESIGN.md 的 run 出处线索仍在作者处开放；延期跟进项（四处池常量上提、3A/ 测试：本评论携带的证据是审查提交上的 PR 自身 CI 检查——全部完成、无失败。 未验证：wall-clock 节省本身。套件钉住 builder 机制、门的配对计价与文案交叉引用——但没有测试观测到真实 3B 运行把 rounds 1+2 放在同一 response 里 launch；节省是编排模型的文案驱动行为。沙箱验证可以定论： 真实场景测试：N/A——无用户可见变化；行为面是 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — the pair and its gate pricing are correct and tested; what remains is an open ledger attribution, a stale body sentence, and the real-model replay the wall-clock claim still owes. Stepping back: this PR started as an asymmetry removal — the convergence pair applied where its arithmetic was already per-chunk — and the review process then forced it to become something more: the two Criticals on the original shape (the reporting transition was undefined; the gate underpriced the pair) are both answered, and answered at the right depth. The pricing fix is the piece I scrutinized hardest, because a wrong price near the deadline is exactly the killed-before-compose failure the gate exists to prevent: it detects the in-flight predecessor by stamp freshness, scales the measured round estimate by the wave ratio on the real pool, caps at two rounds, and degrades to the serial shape when only one member fits. My independent proposal written before reading the diff is what landed; I found no simpler path and no hole in the arithmetic. The failure mode stays benign — a model that never pairs the launches just runs today's serial shape. Reservations, named: the DESIGN.md note's provenance is the one genuinely open item — it names neither measured run, and "six serial rounds" contradicts the round cap that shipped before v0.21.9; the autofix thread is escalated to the author and only the measurement data can settle it. The wall-clock saving remains a real-model claim; the suite pins everything mechanical but cannot observe an orchestrator actually launching rounds 1+2 together, and the CI is settled green on the reviewed commit (no pending runs), so this pass approves, pinned to 中文说明置信度：4/5 —— 收敛对与其门计价正确且经过测试；剩下的是台账出处未决、正文一句话过时，以及 wall-clock 结论仍欠的真实模型回放。 退一步看：这个 PR 起初只是消除不对称——把收敛对应用到它的算术本就逐块成立的地方——评审过程随后要求它做得更多：原始形态上的两个 Critical（报告转移未定义；门低估配对）都得到了回应，且深度恰当。计价修复是我盯得最紧的一块，因为临 deadline 的错误计价正是门要防止的 compose 前被杀：它用 stamp 新鲜度判断在飞的前任，按真实池的波数比缩放实测轮估值，上界两轮，只容得下一个成员时降级为串行。读 diff 前我独立写下的方案就是落地的方案；没找到更简路径，算术也没有漏洞。失败模式仍然温和——模型若不配对 launch，就跑今天的串行形态。 保留意见，明说：DESIGN.md 记事的出处是唯一真正未决的项——既未点名实测 run，"六轮串行"又与 v0.21.9 之前已生效的轮数上限矛盾；autofix 线索已升级给作者，只有实测数据能定论。wall-clock 节省仍是真实模型结论；套件钉住了所有机械部分，却观测不到编排器真的把 rounds 1+2 一起 launch，Stage 2 评论里的 CI 已在审查提交上收敛为绿（无 pending run），本轮批准，钉在 — 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. |
…llet The k=0 launch-coupling note still described 3B's first reverse-audit launch as "round 1's fan-out"; with the convergence pair now applying to 3B it is rounds 1 and 2 per chunk, matching the same fix already made at Step 4's verifier-coupling paragraph.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the required Node 22 configuration did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the required Node 22 configuration did not run locally.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 18903, 19590, 1200 passed; 712 passed — this review observed 18903, 19590, 1200 passed; and 2 more.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the required Node 22 configuration did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the required Node 22 configuration did not run locally。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 18903, 19590, 1200 passed; 712 passed — this review observed 18903, 19590, 1200 passed; and 2 more。
— Qwen Code via Qwen Code /review (v0.21.9)
| - If the round-2 build is refused by the deadline gate (exit 4), launch round 1 alone and treat the refusal as the budget stop it is (the termination rules below). Defensive only: under the gate's pricing a paired round 2 admits strictly cheaper than the round 1 just admitted, so this cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. | ||
|
|
||
| On 3B the pair does not apply: rounds already fan out per chunk, rounds 1 and 2 are what establishes each chunk's record, and the retirement schedule is the convergence ledger there. What 3B shares is the launch coupling: its round 1 also rides with the Step 4 verifiers. | ||
| **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight); same deadline-gate degradation (if the round-2 `--all-chunks` build is refused, exit 4, launch round 1 alone and take the stop). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). |
There was a problem hiding this comment.
[Critical] The 3B convergence pair defines concurrent launch and convergence, but not the reporting-pair transition when either member returns findings. The later generic rule still says round k verifiers launch with round k+1 auditors, even though round 2 is already in flight. — Failure scenario: a round-1 chunk reports a finding → following the generic rule launches round 2 again; skipping it leaves no cross-member deduplicated union, verifier round label, or merge point before round 3, so findings can be verified twice or remain unverified and disappear from the submitted review. Please make the pair-result rules explicitly cover 3B: wait for both fan-outs, deduplicate and merge across rounds/chunks, build one verifier batch with the pair's later round label alongside round 3, and exempt convergence pairs from the generic k/k+1 rule.
中文说明
3B convergence pair 只定义了并发启动与收敛，却没有定义任一成员返回 finding 时的 reporting-pair 流程。后面的通用规则仍要求 round k 的 verifier 与 round k+1 auditor 同时启动，但此时 round 2 已经在运行。失败场景：round 1 的某个 chunk 报告 finding → 继续遵循通用规则会重复启动 round 2；跳过该规则则没有跨成员去重后的 union、verifier 的 round label，以及进入 round 3 前的 merge 点，导致重复验证或 finding 未验证并从最终 review 中丢失。请明确让 pair-result 规则适用于 3B：等待两轮 fan-out，跨 round/chunk 去重并合并，使用 pair 的 later-round label 生成一次 verifier batch 并与 round 3 同时启动，同时让通用 k/k+1 规则排除 convergence pair。
— Qwen Code via Qwen Code /review (v0.21.9)
| - If the round-2 build is refused by the deadline gate (exit 4), launch round 1 alone and treat the refusal as the budget stop it is (the termination rules below). Defensive only: under the gate's pricing a paired round 2 admits strictly cheaper than the round 1 just admitted, so this cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. | ||
|
|
||
| On 3B the pair does not apply: rounds already fan out per chunk, rounds 1 and 2 are what establishes each chunk's record, and the retirement schedule is the convergence ledger there. What 3B shares is the launch coupling: its round 1 also rides with the Step 4 verifiers. | ||
| **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight); same deadline-gate degradation (if the round-2 `--all-chunks` build is refused, exit 4, launch round 1 alone and take the stop). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). |
There was a problem hiding this comment.
[Critical] The deadline gate underprices the concurrent 3B pair because round 2 is built immediately after round 1 writes its admission stamp. expectedRoundSeconds then sees only seconds of observed time and clamps the estimate to the 600-second minimum, even when the two full per-chunk fan-outs exceed the agent pool and require multiple waves. — Failure scenario: the review is near its CI deadline → the pair is admitted at roughly one round's price, consumes the compose/submit reserve, and hits the outer timeout before posting. Admit the initial pair atomically and account for execution waves at the configured concurrency; fall back to round 1 alone when the pair plus tail reserve does not fit, rather than pricing round 2 from the just-written round-1 stamp.
中文说明
Deadline gate 会低估并发 3B pair 的成本，因为 round 1 写入 admission stamp 后立即构建 round 2，expectedRoundSeconds 只观察到几秒钟并把估算压到 600 秒下限，即使两个完整 per-chunk fan-out 超过 agent pool、需要多波执行。失败场景：review 接近 CI deadline → pair 按近似一个 round 的价格被放行，消耗 compose/submit reserve，并在发布前触发外层超时。请把初始 pair 作为原子 admission，并按配置的 concurrency 计算执行波数；当 pair 与 tail reserve 无法同时容纳时退化为只启动 round 1，不要用刚写入的 round-1 stamp 给 round 2 定价。
— Qwen Code via Qwen Code /review (v0.21.9)
| expect(keysOf(2)).toHaveLength(3); | ||
| }); | ||
|
|
||
| it('the 3B pair: round 2 builds every chunk with round 1 still in flight (no round-1 transcripts)', () => { |
There was a problem hiding this comment.
[Suggestion] This test proves that both prompt builders can run without round-1 transcripts, but it does not pin the effect-bearing SKILL.md instruction that both 3B rounds must launch in the same response. — Concrete cost: a later edit can serialize the skill while this test stays green because it calls runRound(1) and runRound(2) itself, restoring the extra reverse-audit round wall. Add a bounded assertion in packages/core/src/skills/bundled/review/SKILL.test.ts that the 3B section contains both --all-chunks --round commands and in the same response.
中文说明
这个测试证明了两个 prompt builder 可以在没有 round-1 transcript 时运行，但没有固定真正影响行为的 SKILL.md 约束：两个 3B rounds 必须在同一个 response 中启动。具体代价：后续可以把 skill 改回串行，而该测试仍然通过，因为它自己顺序调用 runRound(1) 和 runRound(2)，从而重新增加一个 reverse-audit round wall。建议在 packages/core/src/skills/bundled/review/SKILL.test.ts 中加入有界断言，要求 3B section 同时包含两个 --all-chunks --round 命令和 in the same response。
— Qwen Code via Qwen Code /review (v0.21.9)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): PR #8903 extends the reverse-audit convergence pair from ...: I did not run the full agent-prompt.test.ts suite — only the new 3B-pair test (which passes); a regression elsewhere in the 220-test file would not be visible t…; PR #8903 extends the reverse-audit convergence pair from ...: I did not re-run g1's deadline probe against a live budgeted run; I confirmed the mechanism by reading expectedRoundSeconds (a fresh stamp spans seconds and flo…; PR #8903 extends the reverse-audit convergence pair from ...: I did not verify the DESIGN.md note's two CI runs against real workflow logs (the runs are unnamed — f6/RA-2's domain — so unverifiable from the repo), and I di…; PR #8903 extends the reverse-audit convergence pair (roun...: could not execute the new test — npx vitest run ... -t "3B pair" fails at module transform ( Failed to resolve import "ajv/dist/2020.js" from "../core/src/uti…; PR #8903 extends the reverse-audit convergence pair (roun...: none.I've completed my walk of the diff. Here is my report., and 3 more.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19582, 1200, 1502, 486, 3069, 473 passed; 712 passed — this review observed 19582, 1200, 1502, 486, 3069, 473 passed; and 2 more.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：PR #8903 extends the reverse-audit convergence pair from ...：I did not run the full agent-prompt.test.ts suite — only the new 3B-pair test (which passes); a regression elsewhere in the 220-test file would not be visible t…；PR #8903 extends the reverse-audit convergence pair from ...：I did not re-run g1's deadline probe against a live budgeted run; I confirmed the mechanism by reading expectedRoundSeconds (a fresh stamp spans seconds and flo…；PR #8903 extends the reverse-audit convergence pair from ...：I did not verify the DESIGN.md note's two CI runs against real workflow logs (the runs are unnamed — f6/RA-2's domain — so unverifiable from the repo), and I di…；PR #8903 extends the reverse-audit convergence pair (roun...：could not execute the new test — npx vitest run ... -t "3B pair" fails at module transform ( Failed to resolve import "ajv/dist/2020.js" from "../core/src/uti…；PR #8903 extends the reverse-audit convergence pair (roun...：none.I've completed my walk of the diff. Here is my report.，另有 3 条。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19582, 1200, 1502, 486, 3069, 473 passed; 712 passed — this review observed 19582, 1200, 1502, 486, 3069, 473 passed; and 2 more。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.9)
|
|
||
| Measured on the CI reviews of #8619 and #8607: both audits converged at the minimum — round 1 dry, round 2 dry — and the rounds ran serially at 13–25 minutes each, although a dry round leaves the cumulative findings list unchanged, so round 2's launch input was substantively identical to round 1's — the same entries, at most with verification tags the unconditional merge had cleared in between: an independent rerun, paid for at the price of a dependent one. The #8501 round-5 review made the cost concrete: round 1 came back dry, the deadline gate then refused round 2 (`BUDGET:`, exit 4), and the verdict shipped capped by a budget stop — for want of a second dry audit the run had time to launch in parallel but not in series. | ||
|
|
||
| ### The serial 3B convergence rounds |
There was a problem hiding this comment.
[Suggestion] Name the two measured CI runs this note is based on — every sibling measurement note in this file names its runs (#8619, #8607, #8501, #8353), and this paragraph is the load-bearing justification for the PR's claimed saving. — Failure scenario: a future maintainer cannot locate the source runs behind the "~30–56 minutes measured" figure; if the attribution is wrong, the error enters the design record uncorrected and every later design decision re-quotes it.
中文说明
建议为本段实测数据标注来源的两次 CI 评审（PR 号或 workflow run ID）——本文件其他同类实测段落均标注了来源（#8619、#8607、#8501、#8353），而这一段正是本 PR 声称的节省量的核心依据。失败场景：后续维护者无法追溯 "~30–56 分钟（实测）" 数字背后的两次运行；若数据归属有误，错误将进入设计文档且被后续设计决策反复引用。— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
Escalated to the maintainer (author data needed — leaving this thread open).
The two measured runs behind the new "The serial 3B convergence rounds" note are not identifiable from the repository: no PR number, workflow run id, or other attribution for them exists anywhere in the tree, and guessing run ids is exactly the failure this finding warns about ("if the attribution is wrong, the error enters the design record uncorrected").
Options, in order of preference:
- Name the two runs in the note the way every sibling measurement does (fix(desktop): strip Windows verbatim prefix from workspace paths #8619, fix(core): include full filePath alongside fileName in edit/write-file diff results #8607, fix(core): report signal-terminated shell commands as errors #8501, fix(cli): let ESC cancel ongoing work before popping queued messages #8353 style).
- If the attribution cannot be recovered, soften the paragraph to unnamed measurements ("two large chunked-PR CI reviews measured on v0.21.9-era builds…").
Note the coupling with the sibling finding on this paragraph: the six-round run it describes cannot be a v0.21.9 run (the round cap #8773 is an ancestor of the v0.21.9 tag — verified), so if that run is one of these two, its version attribution needs correcting too.
中文说明
升级给维护者（需要作者数据——本线程保持打开）。
新增 "The serial 3B convergence rounds" 段落背后的两次实测运行无法从仓库中识别：树中任何地方都没有它们的 PR 号、workflow run ID 或其他归属信息，而猜测运行标识正是本发现所警告的失败形态（"若归属有误，错误将未经纠正地进入设计记录"）。
选项（按优先级）：
- 像其他同类实测段落一样（fix(desktop): strip Windows verbatim prefix from workspace paths #8619、fix(core): include full filePath alongside fileName in edit/write-file diff results #8607、fix(core): report signal-terminated shell commands as errors #8501、fix(cli): let ESC cancel ongoing work before popping queued messages #8353 的形式）在段落中标注这两次运行。
- 若无法找回归属，则将该段弱化为不点名的实测（"两次大尺寸 chunked PR 的 CI 评审，在 v0.21.9 时代构建上实测……"）。
注意与本段落另一条发现的关联：其中描述的六轮运行不可能是 v0.21.9 的运行（轮数上限 #8773 是 v0.21.9 tag 的祖先——已核实），因此若该运行是这两次之一，其版本归属也需要修正。
|
|
||
| ### The serial 3B convergence rounds | ||
|
|
||
| Two v0.21.9 CI reviews of large chunked PRs spent 77–80% of their wall clock inside the reverse-audit loop, not the fan-out. A 291-minute review ran its 28-agent fan-out in 63 minutes (22%) and then three serial reverse-audit rounds in 223 (round boundaries measured at +65, +134, +190 min); a 252-minute review ran six serial rounds of ~30–37 minutes each. On 3B the rounds ran one at a time because the convergence pair — rounds 1 and 2 launched together, which the 3A path already uses to collapse two serial rounds into one wall — was 3A-only. Its arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — the independent-rerun-paid-as-dependent shape the 3A pair removes, present on every chunk. Pairing rounds 1 and 2 on 3B launches each chunk's two establishing auditors together, saving one round's wall (~30–56 minutes measured) off every chunked review, at the same one-round suppression window the 3A pair and the pipelined loop already accept. The saving is bounded by the agent pool's concurrency: where the pool holds both rounds' auditors it is a full round, and where it does not the doubled launch still packs tighter than two serial rounds ever could. |
There was a problem hiding this comment.
[Suggestion] "a 252-minute review ran six serial rounds" contradicts v0.21.9's own reverse-audit round cap — MAX_REVERSE_AUDIT_ROUNDS = 5 (normal) / HUGE_REVERSE_AUDIT_ROUNDS = 3 (huge diff), introduced by #8773 which is an ancestor of the v0.21.9 release commit — and six exceeds both tiers. Either the version attribution is wrong (the run predates the cap) or the count is wrong (five, or the cap round). — Failure scenario: a maintainer sizes the pair's savings or reasons about the loop's ceiling from a run that v0.21.9's own admission gate could not have executed; the error is re-quoted by later design decisions.
中文说明
"252 分钟的评审跑了六轮串行" 与 v0.21.9 自身的反向审计轮数上限矛盾——`MAX_REVERSE_AUDIT_ROUNDS = 5`（常规）/ `HUGE_REVERSE_AUDIT_ROUNDS = 3`（超大 diff），由 #8773 引入（该提交是 v0.21.9 发布提交的祖先），六轮同时超出两个档位。要么版本归属有误（该运行早于上限引入），要么轮数有误（应为五轮或封顶轮）。失败场景：维护者依据一次 v0.21.9 准入门根本不可能执行出的运行来估算配对的节省量或推断循环上限；错误随后被后续设计决策引用。— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
Escalated to the maintainer (author data needed — leaving this thread open). The finding's premise is confirmed: #8773 (commit 9aec40f2d2, perf(review): cap the reverse audit and shed Agent 8 on a huge diff, 2026-08-10) IS an ancestor of tag v0.21.9 (verified via git merge-base --is-ancestor), and its cap — MAX_REVERSE_AUDIT_ROUNDS = 5 / HUGE_REVERSE_AUDIT_ROUNDS = 3, enforced by the builder's ROUND CAP: refusal — makes a six-round v0.21.9 run impossible.
What the fix needs is the author's measurement record, which is not in the repository:
- The 252-minute run predates the cap → correct the version attribution (name the build/version it actually ran on); or
- The count is wrong → it was five rounds (or the cap round), and the ~30–37-minute figure adjusts accordingly.
Which is it? The paragraph is left unchanged until then rather than picking between the two on a guess — the note is the load-bearing justification for the PR's claimed saving, so the correction should come from the source data.
中文说明
升级给维护者（需要作者数据——本线程保持打开）。该发现的前提已核实：#8773（提交 9aec40f2d2，perf(review): cap the reverse audit and shed Agent 8 on a huge diff，2026-08-10）是 tag v0.21.9 的祖先（已用 git merge-base --is-ancestor 验证），其上限——MAX_REVERSE_AUDIT_ROUNDS = 5 / HUGE_REVERSE_AUDIT_ROUNDS = 3，由 builder 的 ROUND CAP: 拒绝强制执行——使 v0.21.9 的六轮运行不可能发生。
修复需要作者的实测记录，而该记录不在仓库中：
- 252 分钟的运行早于上限引入 → 修正版本归属（写明它实际运行的构建/版本）；或
- 轮数有误 → 实为五轮（或封顶轮），~30–37 分钟的数字相应调整。
是哪一种？在得到答复前该段落保持原样，而不是靠猜测二选一——本段是本 PR 节省量主张的核心依据，修正应来自原始数据。
|
|
||
| ### The serial 3B convergence rounds | ||
|
|
||
| Two v0.21.9 CI reviews of large chunked PRs spent 77–80% of their wall clock inside the reverse-audit loop, not the fan-out. A 291-minute review ran its 28-agent fan-out in 63 minutes (22%) and then three serial reverse-audit rounds in 223 (round boundaries measured at +65, +134, +190 min); a 252-minute review ran six serial rounds of ~30–37 minutes each. On 3B the rounds ran one at a time because the convergence pair — rounds 1 and 2 launched together, which the 3A path already uses to collapse two serial rounds into one wall — was 3A-only. Its arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — the independent-rerun-paid-as-dependent shape the 3A pair removes, present on every chunk. Pairing rounds 1 and 2 on 3B launches each chunk's two establishing auditors together, saving one round's wall (~30–56 minutes measured) off every chunked review, at the same one-round suppression window the 3A pair and the pipelined loop already accept. The saving is bounded by the agent pool's concurrency: where the pool holds both rounds' auditors it is a full round, and where it does not the doubled launch still packs tighter than two serial rounds ever could. |
There was a problem hiding this comment.
[Suggestion] "where it does not [hold both rounds' auditors] the doubled launch still packs tighter than two serial rounds ever could" is false in the exact-division pool regime: with C chunks and an N-agent pool, serial costs 2·ceil(C/N) waves and the doubled launch costs ceil(2C/N) — equal (not tighter) whenever C is a multiple of N, including the common C = N case this sentence describes. — Failure scenario: a maintainer sizes the pair's saving for a pool where the chunk count divides the pool exactly (e.g. 6 chunks on a 6-slot pool): both shapes take the same two waves, so the design record overstates the benefit in exactly the regime its sentence addresses. Suggested fix: soften to the provable bound ("never worse than two serial rounds, and strictly tighter when the doubled load does not divide the pool evenly").
中文说明
"池子容不下两轮审计员时，加倍后的 launch 也比两轮串行更紧凑" 在整除池场景下不成立：设 C 个 chunk、N 个并发槽位，串行为 2·ceil(C/N) 波，加倍 launch 为 ceil(2C/N)——当 C 是 N 的整数倍时两者相等（例如 6 个 chunk 配 6 个槽位，两种形态都恰好两波）。失败场景：维护者按 chunk 数整除池容量的场景估算配对节省量，发现两种形态波数相同，设计文档在该句所描述的区间内高估了收益。建议改为可证明的界限（"绝不差于两轮串行，且当加倍负载不能整除池容量时严格更紧凑"）。— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.9)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR extends the /review skill's reverse-audit converg...: none — all planned checks completed within budget.; This PR extends the /review skill's reverse-audit converg...: none — all checks I started completed within budget..
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 18904, 19570, 1200, 1502, 481, 3069, 473 passed; 712 passed — this review observed 18904, 19570, 1200, 1502, 481, 3069, 473 passed; and 2 more.
[Critical] Re-check of open Critical at packages/core/src/skills/bundled/review/SKILL.md:635 (existing thread 3755398320): STILL STANDS at e3c274d — the 3B convergence pair defines concurrent launch and convergence, but not the reporting-pair transition when either member returns findings: the pair's reporting bullets (dedup the pair against itself; one reporting round; verify the deduped union with every shard passed as --round 2) sit under the heading renamed to "3A (whole-diff form)", and the generic pipelined rule ("round k's verifiers launch with round k+1's auditors") still instructs launching round 2's auditors when round 1 returns findings, even though round 2 is already in flight. No deduplicated union, verifier round label, or merge point is specified for a reporting 3B pair, so findings can be verified twice or remain unverified and drop out of the submitted review.
[Critical] Re-check of open Critical at packages/core/src/skills/bundled/review/SKILL.md:635 (existing thread 3755398324): STILL STANDS at e3c274d — the deadline gate underprices the concurrent 3B pair: when --all-chunks --round 2 is built seconds after round 1's admission stamp, expectedRoundSeconds (packages/cli/src/commands/review/lib/deadline.ts:238) observes a seconds-old span and clamps the round-2 price to the 600-second floor, and both admissions check against nearly the same remaining time, so the pair is committed at roughly one round's price even when the two per-chunk fan-outs exceed the agent pool and run in multiple waves — a regime this PR's own DESIGN.md entry acknowledges. Near the CI deadline the pair can then consume the tail reserve and hit the outer timeout before posting.
中文说明
未探索到全部深度（达到工具调用预算）：This PR extends the /review skill's reverse-audit converg...：none — all planned checks completed within budget.；This PR extends the /review skill's reverse-audit converg...：none — all checks I started completed within budget.。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 18904, 19570, 1200, 1502, 481, 3069, 473 passed; 712 passed — this review observed 18904, 19570, 1200, 1502, 481, 3069, 473 passed; and 2 more。
[Critical] Re-check of open Critical at packages/core/src/skills/bundled/review/SKILL.md:635 (existing thread 3755398320): STILL STANDS at e3c274d — the 3B convergence pair defines concurrent launch and convergence, but not the reporting-pair transition when either member returns findings: the pair's reporting bullets (dedup the pair against itself; one reporting round; verify the deduped union with every shard passed as --round 2) sit under the heading renamed to "3A (whole-diff form)", and the generic pipelined rule ("round k's verifiers launch with round k+1's auditors") still instructs launching round 2's auditors when round 1 returns findings, even though round 2 is already in flight. No deduplicated union, verifier round label, or merge point is specified for a reporting 3B pair, so findings can be verified twice or remain unverified and drop out of the submitted review.
[Critical] Re-check of open Critical at packages/core/src/skills/bundled/review/SKILL.md:635 (existing thread 3755398324): STILL STANDS at e3c274d — the deadline gate underprices the concurrent 3B pair: when --all-chunks --round 2 is built seconds after round 1's admission stamp, expectedRoundSeconds (packages/cli/src/commands/review/lib/deadline.ts:238) observes a seconds-old span and clamps the round-2 price to the 600-second floor, and both admissions check against nearly the same remaining time, so the pair is committed at roughly one round's price even when the two per-chunk fan-outs exceed the agent pool and run in multiple waves — a regime this PR's own DESIGN.md entry acknowledges. Near the CI deadline the pair can then consume the tail reserve and hit the outer timeout before posting.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // apart. Pins the mechanism the SKILL 3B-pair orchestration relies on. | ||
| const r1 = runRound(1); // built, but no transcripts written for it | ||
| expect(r1).toContain('3 auditors required this round — one per chunk.'); | ||
| const r2 = runRound(2); // round 1's records are empty at this point |
There was a problem hiding this comment.
[Suggestion] The comment states round 1's records are empty, but runRound(1) just wrote them — the test itself asserts expect(keysOf(1)).toHaveLength(3) eight lines below. What is absent at this point is round 1's transcripts, as the test title says. The two behave differently in scheduleReverseAuditRound: records are read from round 3 on, an absent transcript classifies unknown, an absent record erases the round from history — so a maintainer debugging the pair/retirement interaction from this pin test would look for tolerance of missing records instead of the actual safety (the k >= 3 guard plus absent transcripts). — Failure scenario: a future fix or regression test gets pointed at the wrong artifact.
| const r2 = runRound(2); // round 1's records are empty at this point | |
| const r2 = runRound(2); // round 1's transcripts don't exist yet at this point |
中文说明
注释声称 round 1 的 records 为空，但 runRound(1) 刚刚写入了它们——本测试在下面八行处断言 expect(keysOf(1)).toHaveLength(3)。此时缺失的是 round 1 的 transcripts（与测试标题一致）。两者在 scheduleReverseAuditRound 中行为不同：records 从 round 3 起才被读取，缺失的 transcript 归类为 unknown，缺失的 record 会把该轮从历史中抹去——因此从这条钉住测试调试 pair/retirement 交互的维护者会去寻找对缺失 records 的容忍，而实际的安全来自 k >= 3 守卫加上缺失的 transcripts。— 失败场景：未来的修复或回归测试被指向错误的对象。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] R0-2 (blocker 3755398324): The deadline gate underprices the concurrent 3B pair — the verifier traced through the code and found the pricing is identical for 3A and 3B (the 600s floor is a per-round minimum, not a per-agent cost; agents are launched in parallel). The blocker's claim about underpricing is not supported by the code, but the SKILL.md does not explicitly address the pricing concern for the 3B pair.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory.
[Critical] R1-1: The 3B convergence pair section defines concurrent launch and convergence, but does not define the reporting-pair transition when either member returns findings. The 3A section has explicit rules for this case; the 3B section says "The convergence contract is unchanged" which is ambiguous — it refers to convergence criteria, not the reporting-pair flow. The generic k/k+1 rule ("verification rides alongside the next round, not ahead of it") conflicts with the pair's concurrent launch — applying it verbatim to the convergence pair would attempt to launch round 2 again, since round 2 is already in flight. Failure scenario: A round-1 per-chunk fan-out returns findings. The orchestrator reads the generic rule and attempts to launch round 2's verifiers with round 2's auditors — but round 2's auditors are already in flight. If the orchestrator skips the generic rule, there is no explicit instruction for: waiting for both fan-outs, deduplicating across rounds 1 and 2 per chunk, building one verifier batch with the pair's later round label (--round 2), or launching verifiers alongside round 3. Findings can be verified twice or remain unverified and disappear from the submitted review.
中文说明
未决，请确认：[Critical] R0-2 (blocker 3755398324): The deadline gate underprices the concurrent 3B pair — the verifier traced through the code and found the pricing is identical for 3A and 3B (the 600s floor is a per-round minimum, not a per-agent cost; agents are launched in parallel). The blocker's claim about underpricing is not supported by the code, but the SKILL.md does not explicitly address the pricing concern for the 3B pair.
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory。
[Critical] R1-1: The 3B convergence pair section defines concurrent launch and convergence, but does not define the reporting-pair transition when either member returns findings. The 3A section has explicit rules for this case; the 3B section says "The convergence contract is unchanged" which is ambiguous — it refers to convergence criteria, not the reporting-pair flow. The generic k/k+1 rule ("verification rides alongside the next round, not ahead of it") conflicts with the pair's concurrent launch — applying it verbatim to the convergence pair would attempt to launch round 2 again, since round 2 is already in flight. Failure scenario: A round-1 per-chunk fan-out returns findings. The orchestrator reads the generic rule and attempts to launch round 2's verifiers with round 2's auditors — but round 2's auditors are already in flight. If the orchestrator skips the generic rule, there is no explicit instruction for: waiting for both fan-outs, deduplicating across rounds 1 and 2 per chunk, building one verifier batch with the pair's later round label (--round 2), or launching verifiers alongside round 3. Findings can be verified twice or remain unverified and disappear from the submitted review.
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
…rting transition The deadline gate priced the concurrent 3B pair's round-2 build off the seconds-old round-1 stamp — clamped to the observation floor — so both members were committed at roughly one round's price even though their two per-chunk fan-outs share the tool-concurrency pool and can take up to two rounds' wall. Admissions whose predecessor is still in flight now pay both members' wall in waves of the pool (expectedAdmissionSeconds): one round's price when the pool holds both fan-outs at once, up to the two-round bound when it serializes them, and the refusal degrades to round 1 alone as the skill's budget-stop rule says. SKILL.md's 3B pair also defined only the dry outcome; its reporting transition now spells out waiting for both fan-outs, deduping across rounds and chunks, one `--round 2` verifier batch riding round 3's build, and the pair's exemption from the pipelined k/k+1 launch rule. DESIGN.md's "packs tighter than two serial rounds ever could" claim is replaced with the provable bound, and the gate's wave pricing is recorded beside it. Tests pin the pair price (deadline.ts and the builder's refusal/admission shapes) and the skill's same-response pair launch.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8903 (round: findings newer than 2026-08-11T03:09:20Z)Commit: Critical findingsrc:3755398320 — the 3B pair defined launch and dry convergence, but not the reporting-pair transition. ADDRESSED.
rc:3755398324 — the deadline gate underpriced the concurrent 3B pair. ADDRESSED.
Suggestionsrc:3755398328 — pin the same-response 3B launch in SKILL.test.ts. ADDRESSED. Added a bounded assertion scoped to the 3B section: it must contain both rc:3755487394 — name the two measured CI runs in the new DESIGN.md note. ESCALATED to the maintainer. The run identities are the author's measurement data and are not recoverable from the repository; fabricating PR numbers is exactly the failure the finding warns about. The thread is left open with the explicit question (see the reply on the thread): name the runs as every sibling note does, or — if the attribution cannot be recovered — soften the paragraph to unnamed measurements. rc:3755487402 — "a 252-minute review ran six serial rounds" contradicts the round cap. ESCALATED to the maintainer, premise verified. Confirmed with git: #8773 (commit rc:3755487404 — "packs tighter than two serial rounds ever could" is false in the exact-division regime. ADDRESSED. Replaced with the provable bound: the doubled launch is never worse than the two serial rounds it replaces — rc:3755596649 — misleading "records are empty" comment in the 3B pair test. ADDRESSED. Applied the suggestion verbatim: Notes
VerificationCommands actually run (all on the final committed tree
中文说明处理评审总结 — PR #8903（本轮：2026-08-11T03:09:20Z 之后的新反馈）提交： Critical 发现rc:3755398320 — 3B pair 只定义了启动与 dry 收敛，未定义 reporting-pair 过渡。已处理。
rc:3755398324 — deadline gate 低估了并发 3B pair 的成本。已处理。
Suggestion 发现rc:3755398328 — 在 SKILL.test.ts 中钉住 3B 同响应启动。已处理。 新增限定在 3B 一节的断言：必须同时包含 rc:3755487404 — "比两轮串行更紧凑" 在整除池区间不成立。已处理。 改为可证明的界限：加倍 launch 绝不差于它所替代的两轮串行—— rc:3755596649 — 3B pair 测试中误导性的 "records are empty" 注释。已处理。 逐字采纳建议： rc:3755487394 — 为新 DESIGN.md 段落标注两次实测 CI 运行。已升级给维护者。 运行标识属于作者的实测数据，无法从仓库中恢复；凭空补写 PR 号正是该发现所警告的失败形态。线程保持打开并附明确问题（见线程回复）：像其他同类段落一样标注运行；若无法找回归属，则将该段弱化为不点名的实测。 rc:3755487402 — "252 分钟的评审跑了六轮串行" 与轮数上限矛盾。已升级给维护者，前提已核实。 git 确认：#8773（提交 备注
验证实际运行的命令（除注明外均在最终提交树
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/模型 |
…gence-pair # Conflicts: # packages/cli/src/commands/review/agent-prompt.ts # packages/cli/src/commands/review/lib/deadline.test.ts # packages/core/src/skills/bundled/review/SKILL.test.ts
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8903 (round 2)Feedback dispositions[Critical] R0-2 — the deadline gate underprices the concurrent 3B pair (blocker 3755398324). Resolved in code (commit [Critical] R1-1 — the 3B section does not define the reporting-pair transition. Resolved in code (same commit, re-verified this round). The 3B convergence-pair section now carries an explicit reporting bullet covering every step the finding named as missing: wait for BOTH fan-outs to return in full before dedup; dedup the pair against itself across rounds and chunks; merge the union into the cumulative list once; verify as one batch sharded per Test plan note (not a blocker): the three test paths in the review's test plan exist — under their package prefixes ( Conflict resolution (
|
qwen-code-ci-bot
left a comment
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): This PR extends the /review skill's reverse-audit converg...: none — all planned checks completed within budget..
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19573, 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 19573, 1221, 1505, 481, 3107, 478 passed; and 2 more.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：This PR extends the /review skill's reverse-audit converg...：none — all planned checks completed within budget.。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19573, 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 19573, 1221, 1505, 481, 3107, 478 passed; and 2 more。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const pairWaves = Math.ceil((2 * width) / pool); | ||
| const roundWaves = Math.ceil(width / pool); | ||
| return Math.ceil((single * pairWaves) / roundWaves); |
There was a problem hiding this comment.
[Suggestion] The pair's wave count prices only the two auditor fan-outs (2 * width), but the pair launches in the same response as the Step 4 verifier shards, which share the same tool-concurrency pool — the priced wall can be one wave shorter than the wall the batch actually takes. — Failure scenario: a 5-chunk 3B plan on the default 10-slot pool with ≥1 verifier shard: the batch is 10 auditors + V verifiers → 2 waves, but pairWaves = ceil(10/10) = 1, so the price is one round estimate for ~2 waves of wall; near the deadline the gate admits a commitment that does not fit, squeezing the reserve that covers the compose/submit tail. Bounded to ~1 extra wave (the reserve normally absorbs it).
Suggested fix: add the co-launched verifier shards to the wave count (pairWaves = ceil((2 * width + shards) / pool) — shards = ceil(findings / verifyShard), known at build time), or state in the docstring that the estimate deliberately prices auditors only and the reserve carries the verifier-contended wave.
中文说明
[Suggestion] 配对的波数计算只给两个审计员 fan-out（2 * width）计价，但配对与 Step 4 的验证员分片在同一个 response 中启动、共享同一个工具并发池——计价的墙钟可能比该批次实际的墙钟少一波。— 失败场景：默认 10 槽池上的 5 块 3B 计划且存在 ≥1 个验证员分片时：批次为 10 个审计员 + V 个验证员 → 2 波，但 pairWaves = ceil(10/10) = 1，于是价格只是一轮估价，对应的却是约 2 波的墙钟；接近截止时间时门会放行一个实际容纳不下的承诺，挤占本应保护 compose/submit 收尾的 reserve。缺口上界约为 1 波（reserve 通常可以吸收）。
建议修复：把同时启动的验证员分片计入波数（pairWaves = ceil((2 * width + shards) / pool)——shards = ceil(findings / verifyShard)，构建时已知），或在 docstring 中说明该估价刻意只对审计员计价、由 reserve 承担验证员争用产生的那波。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| export const TOOL_CONCURRENCY_ENV = 'QWEN_CODE_MAX_TOOL_CONCURRENCY'; | ||
| export const DEFAULT_TOOL_CONCURRENCY = 10; |
There was a problem hiding this comment.
[Suggestion] The env-name + default-10 pair is the fourth independent copy of the tool-concurrency pool read — identical literal reads exist in coreToolScheduler.ts (~L4114), Session.ts (~L7789), and nonInteractiveCli.ts (~L1922). No shared constant owns the pair; the comment this diff adds ("the gate and the launches it gates read the same pool") is enforced only by four copies agreeing. — Concrete cost: a future change to the scheduler's default pool or the env name that misses deadline.ts makes the gate price waves against a stale pool — underpricing admits pairs that do not fit (the killed-before-compose shape this PR removes), overpricing refuses pairs that would fit. Nothing is wrong at this commit; the cost is the unowned invariant.
| export const TOOL_CONCURRENCY_ENV = 'QWEN_CODE_MAX_TOOL_CONCURRENCY'; | |
| export const DEFAULT_TOOL_CONCURRENCY = 10; | |
| export const TOOL_CONCURRENCY_ENV = 'QWEN_CODE_MAX_TOOL_CONCURRENCY'; | |
| export const DEFAULT_TOOL_CONCURRENCY = 10; // keep in sync with coreToolScheduler's pool read — or hoist into a shared core export |
Better: hoist the name+default pair into one shared export beside parsePositiveIntegerEnv in packages/core/src/utils/env.ts and import it at all four sites.
中文说明
[Suggestion] 该 env 名 + 默认值 10 的组合是工具并发池读取的第四份独立拷贝——coreToolScheduler.ts（约 L4114）、Session.ts（约 L7789）、nonInteractiveCli.ts（约 L1922）存在完全相同的字面量读取。没有任何共享常量拥有这对值；本 diff 新增的注释（"门与它所门控的启动读取同一个池"）只能靠四份拷贝保持一致来维持。— 具体代价：未来若修改调度器的默认池或 env 名而漏掉 deadline.ts，门会按过期的池大小为波计价——定价过低会放行容纳不下的配对（即本 PR 要消除的 compose 前被杀形态），定价过高则会拒绝本可容纳的配对。本次提交时尚无错误；代价是这个无人拥有的不变量。
建议修复：把名称+默认值对提升为一个共享导出（放在 packages/core/src/utils/env.ts 的 parsePositiveIntegerEnv 旁边），并在全部四处读取点 import。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
Deferred this round (not dropped): the pair here is indeed the fourth copy of the pool read — the other sites are coreToolScheduler.ts in core and Session.ts / nonInteractiveCli.ts in cli — and the proposed hoist is the right long-term fix. But it is a cross-package refactor across four call sites in two packages, larger than this PR's scope in a round running under a budget warning, and the finding itself confirms nothing is wrong at this commit. Follow-up sketch, preserved verbatim: export the env name + default once beside parsePositiveIntegerEnv in packages/core/src/utils/env.ts and import it at all four read sites (deadline.ts already imports parsePositiveIntegerEnv from core, so the import path exists).
中文说明
本轮推迟（并非丢弃）：此处的名称+默认值对确实是工具并发池读取的第四份拷贝——其余读取点为 core 中的 coreToolScheduler.ts 与 cli 中的 Session.ts / nonInteractiveCli.ts——所建议的提升也是正确的长期修复。但这是一次横跨两个 package、四个读取点的跨包重构，在预算警告轮次中超出了本 PR 的范围，且发现本身确认本次提交并无错误。后续修复草案原样保留：在 packages/core/src/utils/env.ts 的 parsePositiveIntegerEnv 旁一次性导出该 env 名 + 默认值，并在全部四处读取点 import（deadline.ts 已从 core import parsePositiveIntegerEnv，import 路径现成）。
| if (!predecessorInFlight) { | ||
| return costliestSpanSeconds(stamps, nowMs) ?? DEFAULT_ROUND_SECONDS; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This non-in-flight branch inlines expectedRoundSeconds's body verbatim, and this diff removed its last production caller (agent-prompt.ts now imports expectedAdmissionSeconds), leaving it exported but reachable only from deadline.test.ts — round-cost policy now lives in two exported functions in one file. — Concrete cost: a future fix to the estimate (floor, span measurement, fallback) applied to one function silently skips the other; the gate prices from expectedAdmissionSeconds while the BudgetExhausted field name still says "expected round seconds", and a reader cannot tell which estimator is authoritative — the divergence stays green because each twin is pinned by its own tests.
Suggested fix: delegate the branch (return expectedRoundSeconds(planPath, round, nowMs);) restoring one production estimator, or delete expectedRoundSeconds and its test block in this same change if admission pricing fully supersedes it.
中文说明
[Suggestion] 这个非 in-flight 分支逐字内联了 expectedRoundSeconds 的函数体，而本 diff 移除了它最后一个生产调用方（agent-prompt.ts 现在 import 的是 expectedAdmissionSeconds），使其仍然导出、却只能从 deadline.test.ts 到达——轮成本策略如今住在同一文件的两个导出函数里。— 具体代价：未来对估价的修复（地板值、span 测量、回退值）若只应用于其中一个函数，会悄悄绕过另一个；门从 expectedAdmissionSeconds 取价，而 BudgetExhausted 字段名仍叫 "expected round seconds"，读者无法判断哪个估价器是权威——由于两个孪生函数各自被自己的测试钉住，这种分歧依然能让测试全绿。
建议修复：让该分支委托调用（return expectedRoundSeconds(planPath, round, nowMs);）恢复唯一的生产估价器；或者若准入计价已完全取代它，就在本次改动中直接删除 expectedRoundSeconds 及其测试块。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| args.plan, | ||
| args.round, | ||
| reverseAuditRoundCap(report.budget), | ||
| planChunkIds.length, | ||
| ) |
There was a problem hiding this comment.
[Suggestion] The fanOutWidth wiring is untested at two of the three admission call sites — the whole-diff 3A site (literal 1, ~L2433) and this --chunk first-build site (planChunkIds.length): every width-sensitive test in the diff drives runAllChunks (--all-chunks), and the existing --chunk gate tests use expired or far-future deadlines where any price gives the same outcome. — Failure scenario: a future refactor passing a narrower width here survives the whole suite — on the default 10-slot pool widths 1–3 price identically for the 3-chunk fixture plan (equivalent mutant). The mutant becomes observable only under QWEN_CODE_MAX_TOOL_CONCURRENCY=2 with a fresh round-1 stamp, where the first --chunk 13 --round 2 build of an unadmitted round would price 1800s instead of 2700s, admitting a pair that does not fit — the killed-before-compose failure this gate exists to prevent. (The untested wiring is correct as written — this is a missing guard, not a live defect.)
Suggested fix: add one handler-level test admitting round 1 via --all-chunks and building round 2's first auditor via { chunk: 13, round: 2 } at pool 2, asserting exit 4 at 3000s remaining and admission at 3400s; optionally a 3A twin ({ round: 1 } then { round: 2 }, width-1 price).
中文说明
[Suggestion] fanOutWidth 的接线在三个准入调用点中的两个没有测试覆盖——整 diff 的 3A 调用点（字面量 1，约 L2433）和这个 --chunk 首次构建调用点（planChunkIds.length）：diff 中所有对宽度敏感的测试都驱动 runAllChunks（--all-chunks），而既有的 --chunk 门测试使用的是已过期或远未来的截止时间，任何价格都得到相同结果。— 失败场景：未来一次在这里传入更窄宽度的重构能活着通过整个测试套件——在默认 10 槽池上，对 3 块 fixture 计划宽度 1–3 的定价完全相同（等价突变体）。该突变只有在 QWEN_CODE_MAX_TOOL_CONCURRENCY=2 且存在新鲜 round-1 戳记时才可观测：未准入轮次的首个 --chunk 13 --round 2 构建会定价 1800 秒而非 2700 秒，从而放行一个容纳不下的配对——正是这个门要防止的 compose 前被杀失败。（未测试的接线本身是正确的——这是缺失的防护，不是现存缺陷。）
建议修复：新增一个 handler 级测试——通过 --all-chunks 准入 round 1，再以 { chunk: 13, round: 2 } 构建 round 2 的首个审计员，池为 2，断言剩余 3000 秒时 exit 4、3400 秒时放行；可选再加一个 3A 孪生测试（{ round: 1 } 后 { round: 2 }，宽度 1 的价格）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
Deferred this round (not dropped): the wiring is correct as written — the finding itself classifies this as a missing guard, not a live defect — and this round landed the cheaper fixes under a budget warning. Test recipe preserved for the follow-up: one handler-level test admitting round 1 via --all-chunks, then building round 2's first auditor via { chunk: 13, round: 2 } under QWEN_CODE_MAX_TOOL_CONCURRENCY=2 with a fresh round-1 stamp, asserting exit 4 at 3000s remaining and admission at 3400s; optionally the 3A twin ({ round: 1 } then { round: 2 }, width-1 price).
中文说明
本轮推迟（并非丢弃）：接线按现状是正确的——发现本身将其归类为缺失的防护，而非现存缺陷——且本轮在预算警告下落地了成本较低的修复。为后续保留的测试配方：一个 handler 级测试，先通过 --all-chunks 准入 round 1，再在 QWEN_CODE_MAX_TOOL_CONCURRENCY=2 且存在新鲜 round-1 戳记的条件下以 { chunk: 13, round: 2 } 构建 round 2 的首个审计员，断言剩余 3000 秒时 exit 4、3400 秒时放行；可选再加 3A 孪生测试（{ round: 1 } 后 { round: 2 }，宽度 1 的价格）。
| const { | ||
| sendRequest, | ||
| cleanup, | ||
| stderr, | ||
| sessionUpdates, | ||
| permissionRequests, | ||
| agent, | ||
| } = setupAcpTest(rig, { |
There was a problem hiding this comment.
[Suggestion] This changed test file sits outside every npm workspace, so the workspace-scoped unit-test pass (npm test per package, as run by the unit CI jobs) never collects it — only the dedicated integration job does. — Concrete cost: if the Integration Tests (CLI, No Sandbox) job is skipped on this PR, this changed file ships without ever having executed in the pipeline. It was skipped in this PR's CI. Mitigated here: the hunk is formatting-only (destructuring re-wrap) and an explicit run against the PR bundle passed 11/11 — the residual risk is CI-job coverage of the run, not the code.
Suggested fix: N/A for the code; confirm the integration-tests CI job ran (not skipped) for this PR before merge.
中文说明
[Suggestion] 这个被修改的测试文件位于所有 npm workspace 之外，因此按 workspace 划分的单测通行流程（各 package 的 npm test，即单测 CI 作业所运行的）永远不会收集它——只有专门的集成作业会。— 具体代价：如果 Integration Tests (CLI, No Sandbox) 作业在本 PR 上被跳过，这个被修改的文件就会从未在流水线中执行过就合入。本 PR 的 CI 中该作业确实被跳过了。此处有缓解：该 hunk 仅是格式化（解构重排），且针对 PR bundle 的显式运行 11/11 通过——残余风险在于该运行的 CI 作业覆盖，而非代码本身。
建议修复：代码层面无需改动；合入前确认集成测试 CI 作业确实运行了（而非被跳过）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
Declined — no code change exists: the suggested fix is "N/A for the code" by its own text. The hunk in this file is formatting-only (a destructuring re-wrap), and the review's own explicit run against the PR bundle passed 11/11. The residual risk named — the Integration Tests (CLI, No Sandbox) job being skipped on this PR — is a merge-time check owned by the workflow/maintainer, not something a branch change can fix. Leaving this thread open so the check stays visible at merge time.
中文说明
拒绝——不存在可做的代码改动：建议修复按原文即为 "N/A for the code"。该文件中被改动的 hunk 仅是格式化（解构重排），且评审自己针对 PR bundle 的显式运行 11/11 通过。所指的残余风险——Integration Tests (CLI, No Sandbox) 作业在本 PR 上被跳过——是 workflow/维护者在合入时的检查项，不是分支改动能解决的。线程保持打开，使该检查在合入时保持可见。
| if (!predecessorInFlight) { | ||
| return costliestSpanSeconds(stamps, nowMs) ?? DEFAULT_ROUND_SECONDS; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Pair-shaped span ledger poisoning: the 3B pair stamps rounds 1 and 2 seconds apart at the pair's start (stamps record admission time), so after the pair returns the span ledger holds an r2→next span ≈ the pair's full wall (~2 round-walls on a pool that serializes the pair — ≥6 chunks on the default 10-slot pool). costliestSpanSeconds takes the max span with no cap, so every subsequent SOLO round (3–5) is priced at ~2× its true cost. — Failure scenario: probe-observed on the real gate — 6 chunks, default pool: round-3 price 3600s vs solo wall 1800s (ratio 2.00), and the gate REFUSES round 3 although reserve + one true round wall fits; the same gate at the true cost admits, and the serial (pre-PR) ledger prices round 3 at 1800s. Near the deadline the loop stops early ("stopped before round 3") with a capped verdict, clawing back the wall the pairing saves — in the 6–10-chunk band the pair saves zero waves, so a paired run stops strictly earlier than the pre-PR serial shape would have.
Suggested fix: normalize pair-shaped spans before taking the max — e.g. treat consecutive stamps closer than the observation floor as one admission epoch and split the pair's total span across its two members — or document the over-price as accepted conservatism in expectedAdmissionSeconds' doc comment and DESIGN.md.
中文说明
[Suggestion] 配对形状的 span 台账污染：3B 配对在配对开始时相隔数秒分别为 round 1、round 2 盖戳（戳记录的是准入时间），因此配对返回后台账里存在一个 r2→下一轮 的 span ≈ 配对的完整墙钟（在会使配对串行化的池上约为 2 轮墙钟——默认 10 槽池上 ≥6 块即是）。costliestSpanSeconds 取最大 span 且无上限，于是之后每一个 SOLO 轮（3–5）都被定价为约 2 倍真实成本。— 失败场景：已在真实门上用探针观测——6 块、默认池：round 3 定价 3600 秒而单轮墙钟 1800 秒（比值 2.00），门在 reserve + 一轮真实墙钟明明容纳得下时仍拒绝了 round 3；同一门在按真实成本时放行，而串行（PR 前）台账对 round 3 定价 1800 秒。接近截止时间时循环提前停止（"stopped before round 3"）并带着被封顶的裁决，把配对省下的墙钟又 claw 了回来——在 6–10 块区间配对本就省不下任何波，因此配对后的运行会比 PR 前的串行形态严格更早停止。
建议修复：在取最大值之前归一化配对形状的 span——例如把相距小于观察地板的连续戳记视为一个准入纪元，并把配对总 span 平摊给其两个成员——或在 expectedAdmissionSeconds 的 doc 注释与 DESIGN.md 中把这种高估作为可接受的保守性写明。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| - If the round-2 build is refused by the deadline gate (exit 4), launch round 1 alone and treat the refusal as the budget stop it is (the termination rules below). Defensive only: while the runtime's tool-concurrency pool holds both whole-diff members at once, the gate prices the paired round 2 at one round's wall, so it admits no dearer than the round 1 just admitted and this cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. | ||
|
|
||
| On 3B the pair does not apply: rounds already fan out per chunk, rounds 1 and 2 are what establishes each chunk's record, and the retirement schedule is the convergence ledger there. What 3B shares is the launch coupling: its round 1 also rides with the Step 4 verifiers. | ||
| **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). |
There was a problem hiding this comment.
[Suggestion] The Step 5 copyable 3B reverse-audit command block (~lines 653–659) was not updated for the convergence pair this section introduces: its 3A twin block carries an explicit pair comment ("The convergence pair is two of these builds — --round 1 and --round 2, same --findings — launched together"), while the 3B block still shows one call with a single redirect and no pair note; the launch-both instruction lives only in prose ~18 lines above the command the orchestrator copies. — Concrete cost: an orchestrator building 3B rounds from the copyable example serializes rounds 1 and 2 (the block's "ONE call builds them all" comment reinforces the one-call reading), silently losing the one-round-wall saving this PR exists to capture (~30–56 min measured per chunked review) — this skill's measured failure history (the head-sampled roster, the hand-written reverse-audit launches) is exactly "the executor followed the copyable artifact, not the prose". The redirect shape needs no change (<k> yields distinct files); only the annotation is missing.
Suggested fix: mirror the 3A comment in the 3B block, e.g. # Step 3B (large diff): one auditor PER CHUNK per round — ONE call builds them all. The convergence pair is two of these builds — --round 1 and --round 2, same --findings — launched together in one response, each redirected to its own round file.
中文说明
[Suggestion] Step 5 中可复制的 3B 反向审计命令块（约 653–659 行）没有为本节引入的收敛对做更新：它的 3A 孪生块带有明确的配对注释（"The convergence pair is two of these builds — --round 1 and --round 2, same --findings — launched together"），而 3B 块仍只显示单次调用、单个重定向、没有配对说明；"两个一起 launch"的指令只存在于编排器所复制的命令上方约 18 行的 prose 里。— 具体代价：从可复制示例构建 3B 轮次的编排器会把 rounds 1、2 串行化（块中 "ONE call builds them all" 的注释强化了一轮一调用的读法），悄悄丢掉本 PR 要争取的一轮墙钟节省（每个分块评审实测约 30–56 分钟）——本 skill 实测的失败史（head-sampled roster、手写反向审计启动）正是"执行器照可复制产物办事、而不是照 prose 办事"。重定向形状无需改动（<k> 会产生不同文件）；缺的只是注释。
建议修复：在 3B 块中镜像 3A 的注释，例如 # Step 3B (large diff): one auditor PER CHUNK per round — ONE call builds them all. The convergence pair is two of these builds — --round 1 and --round 2, same --findings — launched together in one response, each redirected to its own round file.
— qwen3.8-max via Qwen Code /review (v0.21.9)
| **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). | ||
|
|
||
| - **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. | ||
| - If the round-2 `--all-chunks` build is refused by the deadline gate (exit 4), launch round 1 alone and take the stop. The gate prices that build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so the refusal fires exactly when the pair plus the reserve does not fit, and round 1 alone keeps the serial shape. |
There was a problem hiding this comment.
[Suggestion] This refusal bullet models only the outcome "round 1 admitted, round 2 refused"; when the remaining budget cannot fit even one round (remaining < reserve + solo price, ≈ <40 min at defaults), BOTH pair builds refuse — and the bullet's instruction becomes impossible while the two refusals name contradictory stop entries. — Failure scenario: probe-observed on the real builders — remaining 2000s: round 1 refuses (first-refusal-wins marker "stopped before round 1", zero prompts built); round 2, seeing no stamps, prices solo and also refuses, its BUDGET message instructing the orchestrator to add a "stopped before round 2" entry. The bullet's precondition ("the round-2 build is refused") now holds, but "launch round 1 alone" has nothing to launch, and round 1's own message forbids rebuilding it — contradictory termination guidance (including an instruction to launch a nonexistent prompt) in exactly the time-critical runs the gate exists for. No race required: this fires on ordinary in-order builds. The posted body stays consistent (compose-review dedupes on the budget phrase and renders the marker's entry); the conflict lives in orchestrator guidance.
Suggested fix: extend the bullet — "If the round-1 build is also refused, nothing launches: the first refusal's marker (stopped before round 1) is the stop; proceed to Step 6 and relay that entry only." (The mirror single-refusal orientation — round 2 admitted first, round 1 refused second — is likewise uncovered; making the bullet orientation-symmetric would close both.)
中文说明
[Suggestion] 这条拒绝要点只建模了"round 1 准入、round 2 被拒"这一种结果；当剩余预算连一轮都容纳不下时（remaining < reserve + 单轮价格，默认约 <40 分钟），配对的两个构建都会被拒——此时该要点的指令变得不可执行，而两次拒绝还会给出互相矛盾的停止条目。— 失败场景：已在真实构建器上用探针观测——剩余 2000 秒：round 1 被拒（首次拒绝生效的标记 "stopped before round 1"，0 份 prompt 被构建）；round 2 见没有任何戳记、按单轮定价后同样被拒，其 BUDGET 消息指示编排器添加 "stopped before round 2" 条目。此时要点的前提（"round-2 构建被拒"）成立，但 "launch round 1 alone" 没有任何东西可以启动，且 round 1 自己的消息禁止重建它——在这个门正是为其存在的时间紧迫运行中，出现了互相矛盾的终止指引（包括一条启动不存在 prompt 的指令）。无需任何竞态：普通的顺序构建即可触发。发布的正文仍保持一致（compose-review 按预算短语去重并渲染标记条目）；冲突存在于编排器指引中。
建议修复：扩展该要点——"若 round-1 构建同样被拒，则什么都不启动：首次拒绝的标记（stopped before round 1）即为停止；进入 Step 6 且只转述该条目。"（镜像的单拒绝方向——round 2 先准入、round 1 后被拒——同样未被覆盖；让该要点方向对称可同时补上两者。）
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 3/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31484498320 🧠 Handled by Qwen Code · model/模型 |
…s pricing bounds - Delegate expectedAdmissionSeconds' solo branch to expectedRoundSeconds, restoring one production round-cost estimator (R2-3). - State that the pair price covers the auditor fan-outs only; the co-launched Step 4 verifier shards' extra wave is the reserve's to carry (R2-1). - Document the pair-shaped span ledger's solo over-price as accepted conservatism, in the estimator doc and DESIGN.md (R2-6). - Mirror the 3A pair annotation in the Step 5 3B copyable command block (R2-7). - Make both pair refusal bullets orientation-symmetric and cover the both-builds-refused shape, where nothing launches and the first refusal's marker is the stop (R2-8).
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review feedback round — PR #8903This round ran under a budget warning (the previous round exhausted its time budget), so it landed the smallest valuable subset: five of the eight Suggestions were verified and addressed in one commit ( Dispositions[Addressed] rc:3757227200 — [Addressed] rc:3757227173 — [Addressed] rc:3757227216 — [Addressed] rc:3757227222 — [Addressed] rc:3757227229 — [Deferred] rc:3757227176 — [Deferred] rc:3757227206 — [Declined] rc:3757227210 — Conflict notes
Verification
中文说明评审反馈轮次 — PR #8903本轮在预算警告下运行（上一轮耗尽了时间预算），因此只落地了最小的高价值子集：八条 Suggestion 中有五条经过验证后在一次提交（ 处置结果[已处理] rc:3757227200 — [已处理] rc:3757227173 — [已处理] rc:3757227216 — [已处理] rc:3757227222 — [已处理] rc:3757227229 — [推迟] rc:3757227176 — [推迟] rc:3757227206 — [拒绝] rc:3757227210 — 冲突说明
验证
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.
Reviewed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): PR #8903 extends the /review skill's reverse-audit conver...: none — no check was left unfinished.; PR #8903 extends the /review skill's reverse-audit conver...: none — all checks above completed within the tool budget.; PR #8903 extends the /review skill's reverse-audit conver...: none — all planned checks completed within budget..
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 1221, 1505, 481, 3107, 478 passed; and 2 more.
中文说明
已审查。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8903 extends the /review skill's reverse-audit conver...：none — no check was left unfinished.；PR #8903 extends the /review skill's reverse-audit conver...：none — all checks above completed within the tool budget.；PR #8903 extends the /review skill's reverse-audit conver...：none — all planned checks completed within budget.。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 1221, 1505, 481, 3107, 478 passed; and 2 more。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| expect(section).toContain('`--all-chunks --round 1`'); | ||
| expect(section).toContain('`--all-chunks --round 2`'); | ||
| expect(section).toContain('in the same response'); |
There was a problem hiding this comment.
[Suggestion] The 3B reporting-pair transition — the fix for the existing blocker (thread 3755398320): wait for BOTH fan-outs to return in full, dedup the pair across rounds and chunks, merge the union once, one verifier batch with every shard passed as --round 2 — is pure SKILL.md prose with no regression pin, while every sibling protocol bullet in this file has one; this pin asserts only the launch phrasing. — Concrete cost: a later edit dropping or contradicting the reporting bullet leaves every test in this suite green, restoring the shape where findings on a reporting 3B pair get verified twice or remain unverified and drop out of the submitted review.
| expect(section).toContain('`--all-chunks --round 1`'); | |
| expect(section).toContain('`--all-chunks --round 2`'); | |
| expect(section).toContain('in the same response'); | |
| expect(section).toContain('`--all-chunks --round 1`'); | |
| expect(section).toContain('`--all-chunks --round 2`'); | |
| expect(section).toContain('in the same response'); | |
| // The reporting transition is the fix for the round-0 blocker; a revert | |
| // dropping it must fail here, not slip through. | |
| expect(section).toContain('wait for BOTH fan-outs'); | |
| expect(section).toContain('every shard passed as `--round 2`'); |
中文说明
3B 报告配对转换（reporting-pair transition）——即对既有阻断项（thread 3755398320）的修复：等待两个 fan-out 全部返回、跨轮且跨块对配对自身去重、将并集一次性合并进累积列表、以一个验证批次处理且每个分片都以 --round 2 传入——目前只是 SKILL.md 中的纯文案，没有任何回归钉住测试；而同文件中每个同级协议要点都有对应的钉住测试，且当前这个测试只钉住了启动措辞。—— 具体代价：后续若有编辑删除或推翻该报告要点，本套件中的所有测试仍会保持绿色，从而恢复“报告型 3B 配对的发现被重复验证、或始终未验证而从最终提交的评审中丢失”的形态。建议按上方 suggestion 块扩展钉住测试，使其同时断言转换文案。
— qwen3.8-max via Qwen Code /review (v0.21.9)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round 3 address-review summary — PR #8903Feedback points and dispositions1. Inline [rc:3758703041] —
|
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. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19006, 19573, 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 19006, 19573, 1221, 1505, 481, 3107, 478 passed; and 2 more.
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory; 220 passed — this review observed 19006, 19573, 1221, 1505, 481, 3107, 478 passed; 712 passed — this review observed 19006, 19573, 1221, 1505, 481, 3107, 478 passed; and 2 more。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). | ||
|
|
||
| - **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. | ||
| - If the deadline gate refuses one member's `--all-chunks` build (exit 4) and admits the other's, launch the admitted member alone and take the stop. The gate prices the round-2 build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so this split fires exactly when the pair plus the reserve does not fit but one round still does, and the admitted round alone keeps the serial shape. If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). |
There was a problem hiding this comment.
[Suggestion] The new both-refused mini-protocol ("nothing launches … proceed to Step 6 and relay the MARKER's entry only") drops the bounded tail that the budget-stop termination rule mandates for exactly this shape — "Verify the last round's findings … only through agent-prompt --role verify". At the pair's k=0 the Step 4 verifier shards were already built in the same response, and the verify builder is gated only on the compose floor. Probe-measured at this commit: the band where both builds refuse while the verify gate still admits is (compose floor, reserve + round price) — ~90 minutes wide under default constants, and it survives every CI reserve value tested (600/1800/3600/4800). A budgeted run reaching the pair inside that band follows this bullet literally, launches none of the already-built shards, and every Step 3/4 finding keeps its — [unverified] tag — the verdict caps at Comment although the budget's own rules funded the verification. The adjacent single-refusal clause defers to "(the termination rules below)" while this clause substitutes its own tail-less protocol; the 3A twin bullet (~line 633) is verbatim-identical and affected the same way. — Failure scenario: both pair builds refuse with remaining ≈ 4000s → the built verifier shards (verify gate admits above 1200s) are never launched → all findings stay unverified → capped verdict despite ~2800s of affordable verification.
Suggested fix (apply identically to the 3A bullet):
If it refuses BOTH builds, no audit round launches — treat it as the budget stop it is (the termination rules below): the bounded tail still applies, so the Step 4 verifier shards already built in this same response (gated only on the compose floor) still launch under it before Step 6. Relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders).中文说明
新增的“两个构建均被拒绝”小协议（“nothing launches … proceed to Step 6 and relay the MARKER's entry only”）丢掉了预算停止终止规则针对这一形态强制要求的有界尾部——“Verify the last round's findings … only through agent-prompt --role verify”。在配对的 k=0 时刻，Step 4 的验证分片已经在同一 response 里构建完成，而 verify 构建器只受 compose floor 门控。在本提交上实测：两个构建均被拒绝而 verify 门仍放行的预算区间为 (compose floor, reserve + round price)——默认常量下约 90 分钟宽，且在测试过的每个 CI reserve 值（600/1800/3600/4800）下都存在。预算受限的 run 若在该区间内到达配对，按此要点字面执行将不启动任何已构建的分片，所有 Step 3/4 发现保持 — [unverified] 标签——尽管预算规则本身允许验证，裁决仍被封顶为 Comment。相邻的“单侧拒绝”分句写有“(the termination rules below)”转交，而本分句自带一个没有尾部的协议；3A 孪生要点（约 633 行）与本句逐字相同、受同样影响。—— 失败场景：两个配对构建在剩余约 4000s 时被拒 → 已构建的验证分片（verify 门在 >1200s 时放行）从未启动 → 所有发现保持未验证 → 尽管约 2800s 的验证可负担，裁决仍被封顶。
建议修复（对 3A 要点做相同修改，见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| Math.ceil( | ||
| (DEFAULT_ROUND_SECONDS * Math.ceil(12 / DEFAULT_TOOL_CONCURRENCY)) / | ||
| Math.ceil(6 / DEFAULT_TOOL_CONCURRENCY), |
There was a problem hiding this comment.
[Suggestion] Every default-pool oracle in this new suite recomputes its expected value from DEFAULT_TOOL_CONCURRENCY itself, so the one constant coupling the gate's pool model to the runtime's is unpinned. Mutation run at this commit: DEFAULT_TOOL_CONCURRENCY = 10 → 11 survives the whole diff's test suite (deadline.test.ts 49/49 and the pair tests 7/7 stay green) while the runtime pool stays the literal 10 in coreToolScheduler.ts. — Failure scenario: a future edit of the constant (or drift against the scheduler's literal) silently misprices pairs in production — a 16-chunk fan-out prices at ceil(32/11)/ceil(16/11) = 1.5× instead of the real ceil(32/10)/ceil(16/10) = 2× waves — admitting pairs near the deadline on less wall than they consume, with all tests green.
Suggested fix: pin one oracle to a literal at a discriminating width (width 16 diverges between pools 10 and 11), or assert DEFAULT_TOOL_CONCURRENCY equals the scheduler's literal 10:
expect(
expectedAdmissionSeconds(p, 2, 16, {}, NOW_MS),
).toBe(3600); // literal, not DEFAULT_TOOL_CONCURRENCY-derived: pool 10 ⇒ 2 waves中文说明
这个新套件中所有默认池的期望值都用 DEFAULT_TOOL_CONCURRENCY 本身重新计算，因此把门的池模型与运行时耦合起来的这个唯一常量没有被钉住。在本提交上执行变异测试：DEFAULT_TOOL_CONCURRENCY = 10 → 11 在整个 diff 的测试套件下存活（deadline.test.ts 49/49、配对测试 7/7 仍然全绿），而运行时池仍是 coreToolScheduler.ts 中的字面量 10。—— 失败场景：未来对该常量的修改（或与调度器字面量的漂移）会在生产中悄无声息地错误定价配对——16 个 chunk 的 fan-out 按 ceil(32/11)/ceil(16/11) = 1.5× 定价，而真实波数是 ceil(32/10)/ceil(16/10) = 2×——在临近 deadline 时以少于实际消耗的墙钟放行配对，且所有测试保持绿色。
建议修复：用一个可区分的宽度把某个期望值钉成字面量（宽度 16 在池 10 与池 11 之间结果不同），或断言 DEFAULT_TOOL_CONCURRENCY 等于调度器的字面量 10（见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it('prices the pair at one round when the pool holds both fan-outs at once', () => { | ||
| // The default 10-slot pool holds all six auditors of PLAN's 3-chunk | ||
| // pair in one wave, so round 2 pays one round estimate — a flat 2x |
There was a problem hiding this comment.
[Suggestion] This is the only one of the three new pair tests that never pins TOOL_CONCURRENCY_ENV — its two siblings set it explicitly, and repo convention (coreToolScheduler.test.ts: "Ensure tests are deterministic regardless of environment"; Session.test.ts) shields this exact knob because it is documented as operator-tunable. Probe-measured at this commit: with QWEN_CODE_MAX_TOOL_CONCURRENCY exported at 1, 2, or 5, an isolated run of this single test (vitest -t, IDE run-button, single-test rerun) deterministically fails with expected 4 to be undefined — the pair price doubles past the 3000 deadline although the code under test is correct. Full-suite runs self-heal (a preceding test's afterEach deletes the ambient key), which is how this escaped. — Failure scenario: a developer or runner exporting the documented pool knob at ≤ 5 gets a spurious failure indicting the budget gate instead of the environment.
Suggested fix (verified by probe to flip the failing arm green):
it('prices the pair at one round when the pool holds both fan-outs at once', () => {
// …
delete process.env[TOOL_CONCURRENCY_ENV]; // shield the ambient before the builds
process.env[RESERVE_ENV] = '600';中文说明
这是三个新配对测试中唯一没有钉住 TOOL_CONCURRENCY_ENV 的——它的两个兄弟测试都显式设置该值，且仓库惯例（coreToolScheduler.test.ts：“Ensure tests are deterministic regardless of environment”；Session.test.ts）会屏蔽这个确切的旋钮，因为它被文档标注为运维可调。在本提交上实测：当导出 QWEN_CODE_MAX_TOOL_CONCURRENCY 为 1、2 或 5 时，单独运行该测试（vitest -t、IDE 运行按钮、单测重跑）会确定性地以 expected 4 to be undefined 失败——配对价格翻倍越过 3000 deadline，尽管被测代码是正确的。全套件运行能自愈（前面测试的 afterEach 删除了环境键），这正是它未被发现的原因。—— 失败场景：导出该文档化池旋钮 ≤ 5 的开发者或运行器得到一个错误归因于预算门而非环境的假失败。
建议修复（探针已验证可使失败分支转绿，见上方英文代码块）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action was taken in this round. Critical-only mode is active for PR #8903 after five change-producing rounds. All actionable sections for this round were empty:
The No code changes were made and no commit was created. The PR head remains at its previous commit. 中文说明本轮未采取任何操作。 PR #8903 在经历 5 个产生改动的轮次后已进入仅处理 Critical 的模式。本轮所有可执行区域均为空：
未做任何代码改动，也未创建任何提交。PR 的 head 仍停留在之前的提交上。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@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: 51 passed · 0 failed · 51 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：51 通过 · 0 失败 · 51 总计 Verification reportPR #8903 deep verification —
|
| Scenario (oracle) | head (PR) | base control | Expectation |
|---|---|---|---|
pair-refuse-3b: 3 chunks, pool 2, reserve 600, +3000 s. Pair wall = ceil(1800·3/2)=2700; 600+2700=3300 > 3000 |
r2 refused (exit 4, 0 auditors, marker stopped before round 2) |
r2 admitted (exit 0, 3 auditors, priced at 600 s floor) | head refuses, base admits → the load-bearing flip |
pair-admit-3b: same but +3400 s (3300 ≤ 3400) |
both admitted, stamps [1,2] | both admitted | no over-refusal when the pair fits |
pair-pool10-3b: default 10-slot pool holds all 6 auditors → price = 1800 |
admitted | admitted | one-wave pair priced at one round |
mech-3b: no deadline, round 2 built with no round-1 transcripts |
3 auditors, stamps [1,2] | 3 auditors, stamps [1,2] | mechanism pre-exists on base (matches PR claim) |
both-refused-3b: budget can't cover even round 1 |
both exit 4, marker holds first refusal (round 1) |
identical | both-refused shape + first-refusal-wins marker |
3a-pool1: whole-diff pair on a 1-slot pool → serialized, 2× price (60 min) |
r2 refused (exit 4) | r2 admitted (600 s floor) | 3A orientation of the same fix |
3a-pool10: whole-diff pair on default pool |
admitted | admitted | 3A shape unaffected when it fits |
14/14 cells matched their scripted expectation (the base-arm "admits the under-priced pair" rows are expected control failures and are counted as passing assertions).
Price oracle (unit level). expectedAdmissionSeconds on head returns the wave formula across 14 targeted cells plus a 25×25 (width × pool) sweep of 625 cells, every one within [1×, 2×] of the single-round estimate and equal to ceil(single·ceil(2w/n)/ceil(w/n)); malformed/0 pool env falls back to the default 10 (never a zero divisor), and width 0 clamps to 1. On base the function is absent and expectedRoundSeconds prices an in-flight pair at the 600 s floor — quantifying the under-price the PR removes. Witness: 02-price-oracle-head-vs-base.png (18/18).
Corrections
The PR description understates the change; the deadline gate and CLI logic ARE modified. The body's Risk & Scope states "No change to the round cap, retirement schedule, deadline gate, or any CLI logic" and calls it a "prose/orchestration change"; the How to verify section describes the gate as pricing "the paired round-2 admission on its 600s floor". The delivered diff contradicts all three: deadline.ts gains expectedAdmissionSeconds (+ pool env constants, + costliestSpanSeconds refactor, 104 lines) and agent-prompt.ts threads a fanOutWidth through the admission gate (27 lines). This is labeled a correction to the description, not a request to change the code — the code is the better, intended state (commit fix(review): price the 3B pair's wall at the gate), and this verification proves it correct. But a reviewer reading the body would conclude the budget gate is untouched, when in fact its admission behaviour changes (it now refuses a paired round-2 build that the old 600 s-floor pricing would have admitted near the deadline). That behaviour change is the whole point of the fix and deserves conscious sign-off. The body's stale test counts (claimed 220/712/5/11 vs actual 227/729/8/11) are consistent with the body predating the gate-fix commit plus a merge of main.
Findings (non-blocking, ordered by severity)
1. Description/scope mismatch — the deadline gate is changed (worth reviewer attention). As detailed under Corrections: the PR body's "no CLI logic change / orchestration-only / 600 s floor" claims are false against head. Evidence: git diff HEAD^1..HEAD --stat shows packages/cli/src/commands/review/lib/deadline.ts | 104 + and agent-prompt.ts | 27 +; the A/B pair-refuse-3b and 3a-pool1 cells show the new refusal behaviour that the old gate did not have. No code action required; the description should be reconciled so the gate change is reviewed knowingly.
2. Unrelated formatting-only change in integration-tests/cli/acp-integration.test.ts (nit). This file is outside the PR's subject (ACP integration test, not the review skill). Proven a semantic no-op: the whitespace-stripped token streams of base vs head differ by exactly one inserted , (the prettier trailingComma:"all" destructuring comma), the rest is line re-wrapping; base failed prettier --check under the repo .prettierrc.json while head passes, so this is a stray prettier --write reformat. Repro: git diff HEAD^1..HEAD -w -- integration-tests/cli/acp-integration.test.ts (only the destructuring re-wrap) and prettier --check on base (exit 1) vs head (exit 0). Harmless, but it pollutes a focused diff — the author should confirm it is intentional or drop it. Note the repo's CI "Run Prettier" step is prettier --write . (scripts/lint.js runPrettier), which rewrites but never fails, so non-compliant files can live on main; that is pre-existing context, not introduced here.
Observed design property, not a defect: expectedAdmissionSeconds treats a round as "paired" when the previous stamp is < 600 s old. A non-paired serial round admitted within 600 s would be priced up to 2× (over-refusal near the deadline) — the code documents this as accepted conservatism (over-price ends the loop at most one round early with a disclosed budget stop, never the killed-before-compose shape). The 25×25 sweep confirms the price never exceeds 2× the single-round estimate. No action needed.
Not covered
- Real-model before/after CI-faithful replay — the PR itself names this the "honest proof" of the wall-clock saving (~30–56 min per chunked review). It requires live model runs and is out of scope for this sandboxed job; the wall-clock claim is therefore not independently verified here. What was verified is the mechanism it depends on (round 2 builds every chunk before round 1 returns, and the gate prices/admits the pair correctly).
- Per-commit attribution — the checkout is a merge-ref at depth 2 (
git rev-parse --is-shallow-repository= true; only the merge commit and its two parents are reachable). The PR's 6 commits could not be exercised individually; the aggregateHEAD^1..HEADdiff was verified instead. - Base side of the A/B is a two-module transplant, not a full base rebuild. A full
tscrebuild ofpackages/cliin aHEAD^1worktree failed in this container with 223 module-resolution errors (the worktree cannot see the hoisted/nestednode_moduleslayout), so the control was built by transplanting esbuild-transpiled baseagent-prompt.ts+deadline.tsinto the HEADdist. This is exact for the surface under test (those are the only two changed production modules inpackages/cli; all their imports are unchanged between base and head), and was corroborated byexpectedAdmissionSecondsbeing absent on base and base pricing at the 600 s floor. Stated as a control-construction caveat, not a result. - Whole-repo gates not run — scoped to the affected workspaces; no repo-wide test or lint pass was claimed or performed.
- The
acp-integration.test.tsintegration suite itself was not executed (the change was proven a no-op instead). - Windows / macOS not exercised (Linux container).
Methodology
Environment: node:22-bookworm-class container, working tree = refs/pull/8903/merge (depth 2), npm ci + npm run build already completed at HEAD. The load-bearing proof is a 14-cell A/B (run-ab.mjs → harness-cell.mjs) driving the compiled agentPromptCommand.handler through real temp plans under controlled QWEN_REVIEW_DEADLINE_* / QWEN_CODE_MAX_TOOL_CONCURRENCY envs, comparing head dist against a base control dist. The price oracle (harness-price.mjs) imports the compiled deadline.js directly and checks the wave formula plus a 625-cell sweep. Vacuity/mutation was done by mutation-matrix.mjs and skill-vacuity.mjs, which apply single-point mutants to the real source, run the affected vitest suite (parsed from junit.xml), and revert via git checkout HEAD --; a positive control (mutating the pre-existing MIN_OBSERVED_ROUND_SECONDS 600→60, caught by an old test) proves the suites can fail at all. The four Reviewer Test Plan suites were run verbatim (gates.mjs). @qwen-code/qwen-code-core resolves to /__w/qwen-code/qwen-code/packages/core (head tree) for both arms, and the PR makes no production change under packages/core/src (only SKILL.md/DESIGN.md prose + SKILL.test.ts), so the shared core introduces no A/B confound. Assertion total (51): 14 A/B cells + 18 price-oracle + 4 Reviewer-Test-Plan gates + 7 mutation-matrix (2 control-green + 5 killed) + 3 SKILL-vacuity + 2 typecheck (core, cli) + 2 eslint live-probe + 1 acp no-op; vitest suite pass counts (975 tests across the four suites) are gate evidence in the table above, not double-counted. Evidence images 01–05 were rendered by scripts/verify-capture.mjs; raw per-cell logs (ab-cells.log, price-oracle.log, mutation-matrix.log, skill-vacuity.log) and all harness .mjs files sit alongside this report for rerun.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action requiredThis round found no actionable feedback on PR #8903:
Critical-only mode is active because this PR has already completed five change-producing review rounds. The single remaining feedback item — a non-Critical PR comment from the automated reviewer (@qwen-code-ci-bot, issuecomment-5256848072) — was excluded from this round's actionable sections by that mode and remains open for human follow-up. Per the Critical-only rules, this round made no code change, resolved no thread, and posted no reply for that deferred item. Result: no code changes and no commit. The branch remains at 中文说明Autofix 轮次：无需处理本轮在 PR #8903 上未发现任何可执行的反馈：
由于本 PR 已完成五个产生改动的评审轮次，当前处于仅处理 Critical 的模式。唯一剩余的反馈条目——来自自动评审机器人（@qwen-code-ci-bot）的一条非 Critical PR 评论（issuecomment-5256848072）——已被该模式从本轮可执行区域中排除，保持开放状态留待人工跟进。按照仅处理 Critical 的规则，本轮未对该延后条目做任何代码改动、未解决任何评论线程、也未发布任何回复。 结果：无代码改动，无提交。分支保持在 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory.
中文说明
Test Plan（非阻断）：src/commands/review/agent-prompt.test.ts — no such file or directory; src/skills/bundled/review/SKILL.test.ts — no such file or directory; src/skills/bundled-skills.integration.test.ts — no such file or directory。
— glm-5.2 via Qwen Code /review (v0.21.10)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action takenFeedback triage. This round's feedback carries one automated-reviewer review with zero findings — an Approve downgraded to Comment solely by the self-PR policy — and no inline comments, failed checks, or still-red checks. Critical-only mode (after five change-producing rounds) applies, and there are no Critical items to address. The test-plan note (declined as a code change, with evidence). The review's "Test Plan (not a blocker)" reports three test files as
(Also run: Working reproduction commands: cd packages/cli && npx vitest run src/commands/review/agent-prompt.test.ts src/commands/review/lib/deadline.test.ts
cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts src/skills/bundled-skills.integration.test.tsNo repository file references the bare paths; if they originated in this PR's Reviewer Test Plan section, the fix is a PR-description edit, which this autofix mode does not perform (the workflow owns PR-body writes). Flagging for a maintainer. Conclusion. No code changes this round; the branch head is unchanged and the working tree is clean. Verification
中文说明Autofix 轮次：无需改动反馈分诊。 本轮反馈只包含一条自动评审器的 review，findings 为零 —— 该 Approve 仅因 self-PR 策略被降级为 Comment —— 且没有行内评论、失败检查或仍然失败（红）的检查。当前处于 Critical-only 模式（在 5 个产生改动的轮次之后），没有需要处理的 Critical 条目。 Test Plan 备注（作为代码改动予以拒绝，附证据）。 该 review 的 "Test Plan (not a blocker)" 报告三个测试文件
（另运行了 可正常运行的复现命令： cd packages/cli && npx vitest run src/commands/review/agent-prompt.test.ts src/commands/review/lib/deadline.test.ts
cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts src/skills/bundled-skills.integration.test.ts仓库中没有任何文件引用这些裸路径；如果它们来自本 PR 的 Reviewer Test Plan 部分，修复方式是编辑 PR 描述，而该操作不在本 autofix 模式的职责范围内（PR 正文的写入由工作流负责）。特此提请维护者注意。 结论。 本轮不做任何代码改动；分支头保持不变，工作树干净。 验证
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The 3B convergence pair is wired coherently end to end:
admitReverseAuditRoundnow prices admission viaexpectedAdmissionSeconds(planPath, round, fanOutWidth, env); all three call sites pass the right width (whole-diff=1, chunked=planChunkIds.length, runAllChunks=chunks.length).- Pair pricing math checks out: pairWaves/roundWaves ∈ [1,2], so the pair never prices below one round (no under-commit near the deadline) and never above two (bounded conservatism); pool read from QWEN_CODE_MAX_TOOL_CONCURRENCY with sane malformed fallback, matching the scheduler.
- SKILL.md 3B pair rules are consistent with the gate: single refusal degrades to the admitted round + stop, both refusals relay only the MARKER entry, dedup across rounds+chunks, verifiers ride round 3 as one batch with
--round 2, and the pipelined k/k+1 rule correctly does not relaunch round 2. - Tests pin the right things: wave pricing both directions, env pool reads, reserve-at-boundary refusal, exit-4 budget stop on over-reserve, and SKILL.test.ts guards the 3B same-response launch + reporting transition against silent reverts.
Nits (non-blocking): the acp-integration.test.ts hunk is unrelated prettier churn; the post-pair stamp span over-prices subsequent solo rounds by up to 2x, but that conservatism is documented and fails safe (capped verdict, never killed-before-compose).
|
Released in v0.21.11. |
5 participants
What this PR does
Extends the reverse-audit convergence pair — rounds 1 and 2 launched together in one response — from 3A (whole-diff) reviews to 3B (chunked) reviews. On a 3B plan the orchestrator now launches
agent-prompt --role reverse-audit --all-chunks --round 1and--all-chunks --round 2in the same response, so each chunk's two establishing audits run concurrently instead of one round-wall apart. This is an orchestration change inSKILL.md; the CLI already builds round 2 before round 1's transcripts exist. A newagent-prompttest pins that mechanism, andDESIGN.mdrecords the measurement that motivates it.Why it's needed
On long CI reviews the reverse-audit loop — not the fan-out — is the latency bottleneck: measured on two v0.21.9 runs, a 291-minute review spent 63 min (22%) in its 28-agent fan-out and 223 min (77%) in three serial reverse-audit rounds, and a 252-minute review spent ~205 min across six serial rounds. On 3B those rounds run one at a time because the convergence pair was 3A-only. But the pair's arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative findings list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — the independent-rerun-paid-as-dependent shape the 3A pair removes, present on every chunk. Pairing rounds 1 and 2 on 3B saves one round's wall (~30–56 min measured) off every chunked review, at the same one-round suppression window the 3A pair and the pipelined loop already accept.
Reviewer Test Plan
How to verify
The change is orchestration (SKILL.md) plus one CLI test that locks the mechanism the orchestration relies on — that
--all-chunks --round 2builds every chunk before round 1 has produced any transcript, and stamps its own admission:The mechanism holds because round 2 always fans out to every chunk (the retirement schedule only reads history from round 3) and the deadline gate prices the paired round-2 admission on its 600s floor — exactly what the 3A pair already relies on, so a budgeted run degrades to the serial shape (round 1 alone) if the round-2 build is refused.
Evidence (Before & After)
N/A — non–user-visible (review-tooling orchestration + docs; no TUI surface). The wall-clock before/after is a real-model measurement, not a unit-test artifact — see Risk & Scope.
Tested on
Environment (optional)
Unit tests only (vitest); no runtime app launch.
Risk & Scope
/reviewskill.Linked Issues
Follow-up in the /review latency line (the reverse-audit loop is the dominant cost identified while validating the v0.21.9 timeout fixes). No closing keyword.
中文说明
这个 PR 做了什么
把反向审计的收敛对(convergence pair)——rounds 1 和 2 在同一个 response 里一起 launch——从 3A(整 diff)评审扩展到 3B(分块)评审。在 3B plan 下,编排器现在把
agent-prompt --role reverse-audit --all-chunks --round 1和--all-chunks --round 2放在同一 response 里 launch,于是每个 chunk 的两次"建立记录"审计并发执行,而不是相隔一整轮 wall。这是SKILL.md里的编排改动;CLI 本就支持在 round 1 的 transcript 还没产生时构建 round 2。新增一个agent-prompt测试钉住该机制,DESIGN.md记录了支撑它的实测数据。为什么需要
长 CI 评审里,延迟瓶颈是反向审计循环、而非 fan-out:实测两个 v0.21.9 run——291 分钟的评审里,28-agent fan-out 占 63min(22%),三轮串行反向审计占 223min(77%);252 分钟的评审里六轮串行占 ~205min。3B 的这些轮次逐一串行,是因为收敛对此前仅用于 3A。但收敛对的算术是逐块的、而非整 diff:某个 chunk 在 round 1 干燥,就让它在累积发现列表里的那一片保持不变,于是该 chunk 的 round-2 审计员跑的是实质相同的审计——正是 3A 对所消除的"独立重跑却按依赖计价"的形态,在每个 chunk 上都存在。把 rounds 1、2 在 3B 上配对,可为每个分块评审省下一整轮 wall(实测 ~30–56min),代价与 3A 对和流水线循环已接受的"一轮抑制窗口"相同。
评审测试计划
如何验证
改动是编排(SKILL.md)加一个钉住其所依赖机制的 CLI 测试——即
--all-chunks --round 2会在 round 1 还没产生任何 transcript 时就构建每个 chunk,并 stamp 自己的准入:机制成立的原因:round 2 总是 fan out 到每个 chunk(退休表只从 round 3 才读历史),且截止门用其 600s 地板给配对的 round-2 准入定价——这正是 3A 对已经依赖的,所以预算受限的 run 若 round-2 构建被拒,会优雅降级为串行(仅 round 1)。
证据(前后对比)
N/A——非用户可见(review 工具的编排 + 文档;无 TUI 界面)。wall-clock 的前后对比是真实模型的测量、不是单测产物——见"风险与范围"。
测试环境
运行环境(可选)
仅单元测试(vitest);未启动运行时应用。
风险与范围
/reviewskill 内部。关联 Issue
/review 延迟线的后续(在验证 v0.21.9 超时修复时确认反向审计循环是主要成本)。无自动关闭关键字。