perf(review): remove serial wall-clock the audit loop and tail never needed - #8642
Conversation
|
🔄 Qwen Triage is running — watch live progress. Stage results will post in this thread as they complete. 🔄 Qwen Triage 正在运行 —— 查看实时进度。各阶段结果完成后会更新在本线程。 |
|
Thanks for the PR! Template looks good ✓ — all sections present, with an unusually thorough evidence base. Problem: observed, not theoretical. Six real CI review runs (with run IDs), stage-level timings extracted from the Direction: aligned. A median-141-minute review run that barely correlates with PR size is a real merge-throughput bottleneck, and this complements rather than overlaps #8602 (hangs) and #8631 (fan-out serialization). CHANGELOG: no direct reference — this is our own internal review pipeline, so that's expected. Size: touches Approach: the scope feels right. Each of the four cuts is anchored to a measured incident, the quality-relevant invariants are explicitly preserved (two independent dry audits before convergence, every finding verified, whiff handling, 3B retirement), and the out-of-scope list is honest (effort-tier policy, per-agent tool budgets, 3B pairing). I looked for a simpler path and didn't find one — the convergence pair is the minimal change that removes the serial second round, and dropping the env-var preference entirely (rather than reordering it) is right given it was wrong 100% of the time it was consulted. No unrelated edits in the diff. Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ —— 各节齐全,证据基础扎实。 问题:已观测而非理论。6 个真实 CI review run(附 run ID)、从 方向:对齐。中位 141 分钟且与 PR 大小几乎无关的 review 耗时是真实的合入吞吐瓶颈,本 PR 与 #8602(挂死)、#8631(扇出串行化)互补而非重叠。CHANGELOG:无直接引用——这是我们自己的内部 review 流水线,符合预期。 规模:触及 方案:范围合理。四处削减各自锚定一个实测事故,质量相关的不变量被显式保留(收敛前两次独立 dry 审计、每个 finding 都过验证、whiff 处理、3B retirement),范围外清单也诚实(effort 档位政策、单 agent 工具预算、3B 配对)。我找过更简路径,没找到——收敛对是移除串行第二轮的最小改动;鉴于该 env 偏好被咨询时 100% 是错的,彻底弃用(而非调整优先级)是对的。diff 无无关改动。 风险:无升级风险信号——改动文件均未命中 revert 历史分析的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blockers found. I verified the load-bearing mechanics claims against the base code rather than taking the description at face value:
Independent proposal matched: my pre-diff plan was the same four cuts, and the diff handles the edge cases I'd have asked about (pair dedup, convergence-count restart after a reporting pair, retroactive-dry timing one round late under pipelining) explicitly in the text. Nothing simpler occurred to me that the PR missed. Test evidence — the PR's own CI, read via APIFetched once at the reviewed commit; no polling. At fetch time: 0 failures, Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 What the suite pins (per the diff, not re-run here): the save-artifact suite pins explicit Sandboxed verification would settle this: 中文说明代码审查:未发现阻塞项。关键机制声明均对照 base 代码核实,而非仅凭描述采信:
独立提案吻合:我看 diff 前的方案就是这四处削减;diff 对我会追问的边界情形(配对去重、报告对后收敛计数重启、流水线下的追认 dry 晚一轮到达)都在文本中显式处理。没有更简路径被遗漏。 测试证据:在被审 commit 上经 API 一次性读取,不轮询。读取时:0 失败, 沙箱验证可落定此事: — Qwen Code · qwen3.8-max Reviewed at |
Code Review —
|
|
Confidence: 4/5 — clean review across every stage; the one open item is inherent to the change: the wall-clock win is counterfactual until live review runs execute the new skill text. Stepping back: this is what a good perf PR looks like. Every cut is anchored to a named, measured incident — not "this looks parallelizable", but "run X spent 22 minutes waiting on verdicts its launch never consumed", "run Y shipped a capped verdict for want of a second dry audit it had time to launch in parallel but not in series". I independently proposed the same four cuts before reading the diff, and the diff goes further than my proposal in exactly the right places: the edge cases (reporting-pair dedup, convergence-count restart, retroactive-dry arriving one round late under pipelining) are handled in the text instead of being left to orchestrator discretion — which is precisely the failure mode fix 1 removes. The quality invariants hold up under scrutiny: two independent substantively-dry audits before convergence, every finding verified, whiffed scopes staying not-audited, 3B untouched. The tag backstop plus Reservations, named so they aren't forgotten: the "after" numbers are counterfactual (the PR says so itself), so the −5% to −30% range is a promise until the next CI reviews land — the Approving — deferred until CI lands green on 中文说明退一步看整体:这是一个 perf PR 该有的样子。每处削减都锚定一个有名有姓的实测事故——不是"这里看起来可以并行",而是"run X 等了 22 分钟一个它的发射根本不消费的验证结果"、"run Y 为一个来得及并行、来不及串行的第二次 dry 审计背上了 capped verdict"。我在读 diff 前独立提出了同样的四处削减,而 diff 在正确的方向上超出了我的提案:边界情形(报告对去重、收敛计数重启、流水线下追认 dry 晚一轮到达)被写进文本,而不是留给编排器自由裁量——后者正是修复点 1 要消灭的失败模式。 质量不变量经得起推敲:收敛前两次独立、有实质收据的 dry 审计、每个 finding 都过验证、whiff 范围保持未审计、3B 未触碰。标签兜底加 保留意见,点名以免被遗忘:"后"的数字是反事实的(PR 自己也这么说),所以 −5% ~ −30% 区间是一个承诺,要等下一批 CI review 落地才兑现——上面点名的 批准——延迟至 CI 在被审 commit 上落绿后执行。 — 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. |
|
All five suggestions addressed in 71a2ba3:
Also in flight: a live qwen3.8-max run of the updated skill against PR 8619 (same CI invocation shape) to convert the counterfactual savings into a measured before/after; will post the stage table when it completes. — Claude Code · Opus 5 |
71a2ba3 to
e0e4d50
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
…needed Four latency cuts, each backed by stage-level timings extracted from six real CI review runs (2026-08-05/06); none changes what a review covers or what evidence a verdict requires. - Step 4's initial verification rides with the first reverse-audit launch (the k=0 case of the existing pipelined-loop rule). One measured run held its round-1 auditor 22 minutes behind verdicts the launch never consumed, while a sibling run the same day launched the two together. - Reverse-audit rounds 1 and 2 become the convergence pair on 3A plans: two independent auditors launched together, both-dry = the two dry audits the criterion already demands. A dry round leaves the findings list unchanged, so serial round 2's input was byte-identical to round 1's; the pair removes the wall clock between them, not either audit. Also spares the budget-stop cap a run took for want of a second dry round it had time to run in parallel but not in series. - A reporting round whose every finding the verifier rejected is retroactively dry: rejections already remove the entries, but not the reset they applied to the dry counter, and one measured run spent two extra rounds re-proving what round 2 plus a rejection had already answered. - save-artifact resolves its workspace root from cwd (or an explicit --workspace-root), not QWEN_CODE_PROJECT_DIR - the harness exports that variable as the session-storage dir under the runtime base, never the main checkout, and all six runs burned minutes rediscovering it before improvising workarounds. Steps 8/9 also batch the tail bookkeeping into four responses instead of one command per model turn.
- Pin the cwd default with the trap armed: the regression test now chdirs into the root, passes no workspace root, and points QWEN_CODE_PROJECT_DIR at a decoy - reintroducing the removed env preference turns it red (verified by negative control). - Have the skill pass --workspace-root explicitly in the Step 8 block, so the containment anchor never depends on where the command happened to run. - Pin the retroactive-dry pairing direction: forward only, consulted on a round's return; the upgrade never ends the loop by itself and never discards an in-flight round. - Make the Step 4 batch self-contained (findings files written before either prompt set is built), and gate tail group 4 on group 3's success so cleanup cannot destroy save-artifact's only inputs after a failure. - Add the defensive rule for a deadline-refused pair member: launch round 1 alone and treat the refusal as the budget stop.
…t review Ten findings from a 9-agent review round plus one reverse-audit round over the PR's own diff; all verified against the cited lines before fixing. - Step 4 k=0 batch: pre-confirmed [build]/[test] entries enter the cumulative list untagged (the every-entry-tagged wording would have sent deterministic findings into the tag backstop and capped the verdict); the no-shard clause covers the all-pre-confirmed case, not just zero findings. - Convergence pair x retroactive-dry: the two rules now compose instead of conflicting - a pair retired in full counts as the dry predecessor at round 3's return; round k-1 counts launches, not labels; verdicts are applied at a round's return before the dry consultation; the pair's verification is one build over the deduped union passed as --round 2. - Tail batching: a group (3) that cannot be resolved no longer gates cleanup forever - disclose, skip record_artifact, preserve the inputs beside the report, and still run cleanup so the bypass audit and the completion line always happen; the old before-cleanup ordering sentence now names the group (3)/(4) split instead of contradicting it. - Accuracy: "byte-identical" weakened to what the pipelined merge rule actually guarantees, and the pair's round-2 suppression window named as the already-accepted trade; the #8353 incident recast to the forward pairing the rule licenses (round 5 was the waste, not rounds 4-5) with the ledger-vs-audit round numbering disambiguated. - save-artifact: comment block and test comments no longer describe the flag as embedder-only - it is the containment anchor the skill passes on every run; new CLI option contract test drives --workspace-root through real yargs into saveReviewArtifact (mirrors the test-plan/test-delta precedent for camel-case flag regressions).
Round-2 reverse audit: the pair bullet's "one verify build over the deduped union" contradicted Step 4's verifyShard sharding (quality collapses past 8 findings per verifier) and its own plural "the pair's verifiers". The rule now shards the deduped union exactly as any reporting round's findings are, every shard passed as --round 2; the per-member ban is unchanged.
e06ef40 to
2f1b763
Compare
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. Code: removing the QWEN_CODE_PROJECT_DIR preference is correct (measured wrong 100% of the time — it names the session-storage dir, not the checkout); the explicit --workspace-root + cwd fallback is a trust improvement, and the decoy test (env never consulted) plus the yargs-contract test (parsed object fed straight in, catching camelCase flag bugs) are exactly the right pins. Orchestration: pipelining the initial verification (k=0), the 3A convergence pair, the retroactively-dry rule, and Step 8/9 batching all remove serial wall-clock while preserving the safety invariants (last-round verification before Step 6, tag backstop, budget gate, unreviewedDimensions cap); each is backed by measured DESIGN.md data and the retroactively-dry rule is carefully constrained (forward-only, never ends the loop alone, a launched auditor's return is always taken). No P0/P1.
|
@qwen-code /triage |
|
🔬 Sandboxed verification is running — watch live progress. The report will be posted here when the run completes. 🔬 沙箱验证正在运行 —— 查看实时进度。运行结束后验证报告会发布在这里。 |
|
Closing the loop on the promised validation run (live qwen3.8-max, CI-faithful headless replay against PR 8619, updated skill bundled): Confirmed executing as specified — the launch shapes the counterfactual savings depend on:
Caveat, disclosed: the run's subcommands were contaminated by a local-only version skew (launching via The real before/after lands with the next few CI reviews now that this is on main — happy to compare stage tables against the six-run baseline once a couple complete. — Claude Code · Opus 5 |
|
Released in v0.21.7. |
What this PR does
Four latency cuts to the
/reviewpipeline, each derived from stage-level timings extracted from six real GitHub-triggered review runs (2026-08-05/06). None of them changes what a review covers, which agents run, or what evidence a verdict requires — they remove wall-clock that was serial by accident, not by design.— [unverified]tags for exactly this state), while a sibling run the same day launched the two together. Now the initial verification is the k=0 case of the same rule, on both topologies.--round 1and--round 2, same--findingsfile; the CLI already keys the two records apart by round, so this needs zero CLI change. Both dry = the two independent, substantively-dry audits the two-consecutive-dry criterion already demands: a dry round leaves the cumulative list unchanged, so serial round 2's launch input was substantively identical to round 1's — the same entries, at most with verification tags cleared in between — an independent rerun bought at the price of a full round of wall clock. If either member reports, the pair is one reporting round and the loop continues serially from round 3 with convergence counting restarted, so the non-dry path loses nothing. 3B is untouched (rounds already fan out per chunk; the retirement schedule is the convergence ledger there).save-artifactresolves its workspace root from cwd (or an explicit--workspace-root), never fromQWEN_CODE_PROJECT_DIR— plus Steps 8/9 batch the tail bookkeeping into four responses instead of one command per model turn. The env preference was wrong 100% of the time it was consulted: the harness exports that variable as the session-storage directory under the runtime base (Storage.getProjectDir(), where the harness's transcripts live), never the main checkout, in every environment. All six measured runs refused their own inputs at Step 8 and burned 1.5–3 minutes improvising workarounds (override the env var; copy inputs into the qwen-home mirror; retry path shapes). The skill runs every subcommand from the main checkout, so cwd is right by construction.Five new measured-incident entries in DESIGN.md anchor the new rules, enforced by the existing SKILL.test.ts pointer↔heading consistency gate.
Why it's needed
/reviewhas become the bottleneck on merge throughput: of the last 81 executed review runs, the 33 that ran a real review (≥30 min, all successful) took median 141 / p75 236 / p90 277 minutes, and duration barely correlates with PR size — a 29-line PR took 65 minutes, a 72-line PR 138 minutes, a 1-line docs PR 57 minutes.Stage decomposition of two small-PR runs (timestamps from the CI job logs):
The audit-loop tail and bookkeeping stages above are serial waste: #8619 and #8607 both converged at the minimum two rounds, run serially with byte-identical inputs; the #8501 round-5 review had its round 2 refused by the deadline gate and shipped a capped verdict for want of a second dry audit it had time to run in parallel but not in series (fix 2 also removes that cap scenario); the round-15 review of #8353 ran a fifth audit round (~15–20 min) after round 4's dry return had already completed the two-dry evidence alongside a verifier rejection (fix 3). Counterfactual totals for the six runs: −5% to −30% wall clock, with the quality-relevant loop structure — two independent dry audits before convergence, every finding verified, whiff handling, 3B retirement — unchanged.
Complements #8602 (stream lifetime cap + slim launch blocks) and #8631 (daemon fan-out concurrency): those fix hangs and serialization; this removes work-shaped waiting that remains after them.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/— 56 files, 1991 passed. The save-artifact suite now pins: explicit--workspace-rootresolution, cwd default, and a regression test thatQWEN_CODE_PROJECT_DIRis ignored entirely (decoy-root test).cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts— the pointer↔heading gate covers the five new DESIGN.md incidents.agent-promptkeys findings-role records asreverse-audit--round-<k>--<digest>(agent-prompt.ts), so--round 1/--round 2with the same findings file are two records, two briefs, two receipts; the deadline gate prices a paired round-2 admission at the 600-second observed floor (deadline.tsexpectedRoundSecondstakes the max span), so a pair admits whenever round 1 does and round 3's admission measures the pair's real cost.Evidence (Before & After)
The six dissected runs above are the before. After is counterfactual until the next CI reviews land: fix 1 saves 8–22 min on the ~half of runs that serialized the initial verify; fix 2 saves 13–25 min on min-convergence runs (2 of 6) and converts #8501-shaped budget caps into clean convergences; fix 3 saves one round (~15–20 min) on rejected-finding shapes; fix 4 saves 5–8 min on every run.
Tested on
Environment (optional)
Vitest suites from
packages/cli/packages/core; stage timings extracted from thereview-prjob logs of runs 31089304225 (#8619), 31080454889 (#8628), 31082388591 (#8607), 31080978821 (#8353), 31089156499 (#8594), 31085374670 (#8501).Risk & Scope
save-artifactbehavior change: an embedder that deliberately exportedQWEN_CODE_PROJECT_DIRas a workaround (what the measured CI runs improvised) now gets cwd resolution instead — correct in every observed case, since those runs ran from the main checkout;--workspace-rootis the explicit escape hatch.--comment, a policy change), per-agent tool budgets (the 11-vs-41-minute fan-out wave variance — named by fix(cli): Run ACP agent fan-outs concurrently and past the tool-call cap #8631 as the next target), and 3B pairing of rounds 1+2 (would interact with retirement/deadline pricing; not measured as a bottleneck).Linked Issues
Related: #8602, #8631 (complementary
/reviewperformance work).中文说明
这个 PR 做了什么
对
/review流水线的四处延迟削减,全部来自对 6 个真实 GitHub 触发 review 运行(2026-08-05/06)的日志逐阶段计时。它们不改变 review 的覆盖范围、agent 名单或裁决所需的证据——移除的是"偶然串行"而非"设计串行"的墙钟。— [unverified]标签支持这个状态),同一天的另一个 run 则把两者一起发。现在首次验证是同一规则的 k=0 情形,两种拓扑都适用。--round 1与--round 2、同一--findings文件;CLI 的记录键本就按轮号区分,零 CLI 改动。双双 dry = 两连 dry 判据要求的"两次独立、有实质收据的 dry 审计":dry 轮不改变累计清单,串行第 2 轮的输入与第 1 轮实质相同——同一组条目,至多验证标签在间隙被清除——是一次用整轮墙钟买来的独立重跑。若任一成员报告 finding,收敛对算一个报告轮,从第 3 轮起恢复串行、收敛计数重新起算,非 dry 路径无任何损失。3B 不动(轮次本就按 chunk 扇出,retirement 调度就是那里的收敛台账)。save-artifact的 workspace 根从 cwd(或显式--workspace-root)解析,不再读QWEN_CODE_PROJECT_DIR;同时 Step 8/9 把尾部记账合批为 4 个响应(原来一命令一模型轮)。旧的 env 偏好 100% 是错的:harness 导出的该变量是 runtime base 下的会话存储目录(Storage.getProjectDir(),transcripts 所在),任何环境下都不是主 checkout。6 个实测 run 全部在 Step 8 被拒收自己的输入、烧 1.5–3 分钟各自摸索绕法。skill 本就要求所有子命令从主 checkout 运行,cwd 天然正确。DESIGN.md 新增 5 条 measured-incident 条目锚定新规则,由既有 SKILL.test.ts 指针↔标题一致性测试强制。
为什么需要
/review已成为合入吞吐率的瓶颈:最近 81 个实际执行的 review run 中,33 个真正跑完整流程的(≥30 分钟,全部成功)耗时中位 141 / p75 236 / p90 277 分钟,且与 PR 大小几乎无关——29 行的 PR 65 分钟,72 行的 138 分钟,1 行的 docs PR 57 分钟。两个小 PR run 的逐阶段分解(时间戳取自 CI job 日志):#8619(29 行,59 分钟)setup 3.5 / 扇出 11.7 / verify 与 RA1 并行 15 / RA2 12.8(修复点 2)/ 提交 7.8 / 尾部 5.3(修复点 4);#8628(72 行,130 分钟)setup 3.2 / 扇出 41 / verify 串行 22.4(修复点 1)/ RA1 22 / RA2 18 / 提交 12 / 尾部 4.6。
其中的串行浪费:#8619 与 #8607 都以最小两轮收敛,却用逐字节相同的输入串行跑了两轮;#8501 的 round-5 review 第 2 轮被 deadline gate 拒绝,为一个"来得及并行、来不及串行"的第二次 dry 审计背上了 capped verdict(修复点 2 同时消掉这个场景);#8353 的 round-15 review 在第 4 轮 dry 返回加一次驳回已凑齐两连 dry 证据后,仍多跑了第 5 轮(约 15–20 分钟)(修复点 3)。6 个 run 的反事实合计:墙钟 −5% ~ −30%,而质量相关的循环结构——收敛前两次独立 dry 审计、每个 finding 都过验证、whiff 处理、3B retirement——原封不动。
与 #8602(流总时长上限 + 发射块瘦身)、#8631(daemon 扇出并发)互补:那两个修挂死与串行化,本 PR 移除其后仍剩的"长得像工作的等待"。
审查者验证计划
如何验证:
cd packages/cli && npx vitest run src/commands/review/——56 个文件、1991 通过;save-artifact 套件钉住显式--workspace-root解析、cwd 默认值、以及"完全忽略QWEN_CODE_PROJECT_DIR"的诱饵根回归测试。cd packages/core && npx vitest run src/skills/bundled/review/SKILL.test.ts——指针↔标题一致性测试覆盖 5 条新 DESIGN.md 条目。收敛对的机制核查(零 CLI 改动,可自行确认成立):agent-prompt对 findings 角色的记录键为reverse-audit--round-<k>--<digest>,同一 findings 文件的--round 1/--round 2是两条记录、两份 brief、两张收据;deadline gate 对成对第 2 轮的准入按 600 秒观测下限计价(expectedRoundSeconds取最大跨度),第 1 轮能准入则第 2 轮必然能,第 3 轮准入实测收敛对的真实成本。SKILL 语义:读 Step 4 "the verifiers do not launch alone"、Step 5 "The convergence pair (3A only)" 与两条新终止规则,确认两次独立 dry 审计要求、whiff 规则、3B retirement/CONVERGED 路径均未被削弱。前后证据:前=上述 6 个解剖 run;后=反事实,待后续 CI review 落地实测。修复点 1 在约一半 run 上省 8–22 分钟;修复点 2 在最小收敛 run(6 中 2)省 13–25 分钟、并把 #8501 型 budget cap 变为干净收敛;修复点 3 在驳回型 run 省一轮(约 15–20 分钟);修复点 4 每个 run 省 5–8 分钟。
测试平台:Linux ✅(macOS/Windows:CI)。测试环境:
packages/cli/packages/core下 vitest;阶段计时提取自 run 31089304225(#8619)、31080454889(#8628)、31082388591(#8607)、31080978821(#8353)、31089156499(#8594)、31085374670(#8501)的review-prjob 日志。风险与范围
save-artifact行为变化:曾把导出QWEN_CODE_PROJECT_DIR当绕法的接入方(即实测 CI run 的即兴做法)现在得到 cwd 解析——在所有观测场景下均正确(它们本就从主 checkout 运行);--workspace-root是显式逃生口。--comment,属政策变更)、单 agent 工具预算(扇出波 11 vs 41 分钟的方差,fix(cli): Run ACP agent fan-outs concurrently and past the tool-call cap #8631 已点名为下一目标)、3B 的 1+2 轮配对(与 retirement/deadline 计价有交互,且未实测为瓶颈)。关联 Issue
相关:#8602、#8631(互补的
/review性能工作)。