feat(cli): fold completed read/search tool batches into the thought line - #9503
feat(cli): fold completed read/search tool batches into the thought line#9503DragonnZhang wants to merge 28 commits into
Conversation
When the model thinks and then runs an all-read/search/list tool batch,
the completed batch summary now folds into the collapsed thought line
("Thought for 9s, searched 'alpha', 'beta'") instead of rendering as a
separate line below it — removing the inconsistent gap under thought
lines and keeping scrollback dense, Claude Code style.
The thought commit moves from "tool call requested" to "tool batch
completed" so the merge decision lands before Ink's <Static> freezes the
line; every early exit (local cancel, error, retry, fallback, nothing
scheduled, concurrent new prompt) commits the thought as-is. Non-mergeable
batches (edit/command tools, failures, images, memory ops) keep today's
rendering, and folded groups stay in history — full detail (Ctrl+O) shows
them again.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
E2E test report / E2E 测试报告Deterministic tmux E2E against a mocked OpenAI endpoint (mock streams Baseline (Before, global qwen 0.21.13) — thought line and tool summary render as two separate lines: After (this PR's bundle) — merged into one line: Scenarios verified:
Rendering path: default virtual viewport ( Unit coverage for the deferral state machine: merge on completion, non-collapsible batch, cancelled tool, and local-cancel-during-deferral (regression test added after code review) — |
|
Thanks for the PR! Template: looks good ✓ — all sections present, bilingual, with a concrete reviewer test plan and before/after captures. Problem: this is a UX enhancement rather than a bug fix, and the motivation is observable rather than theoretical: the collapsed thought line currently sits above a second summary line for read/search batches, and the gap below the thought line varies with the item type that follows it. The PR body includes before/after captures, and the full E2E plan with baseline is referenced ( Direction: aligned. Folding disposable read/search summaries into the thought line matches the Claude Code interaction style this project actively tracks; Claude Code's CHANGELOG shows the same area is live there ( Size: no core-infrastructure paths touched — everything is Approach: the scope feels right. Deferring the thought commit from "tool call requested" to "batch completed" is the mechanism the merge decision needs (Ink's Risk: no elevated risk signals — none of the changed files match the repo's revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献! **模板:**完整 ✓ —— 各节齐全、中英双语,附有具体的审阅者测试计划和 before/after 截图。 **问题:**这是 UX 增强而非 bug 修复,动机是可观察的而非理论性的:当前折叠的思考行下方会再渲染一行 read/search 批次摘要,且思考行下方的间距随后继条目类型变化。PR 正文附上了 before/after 对比,完整 E2E 计划与基线见 **方向:**对齐。把一次性 read/search 摘要折叠进思考行符合本项目持续对标的 Claude Code 交互风格;Claude Code 的 CHANGELOG 显示同一领域在其侧也在演进( **规模:**未触及核心基础设施路径——全部位于 **方案:**范围合理。把思考提交从"收到工具调用请求时"延迟到"批次完成时"是合并决策所需的机制(Ink 的 **风险:**无升级风险信号——改动文件均未命中仓库与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach lands exactly where an independent design would land: Ink's What I verified by reading the code:
Tests cover the merge path, a non-collapsible (edit) batch, a cancelled tool, and the local-cancel stranding case, plus render tests for the merged label ("Thought for 9s, searched 2 patterns", "Thought briefly, read a.ts") and the finalized-but-pending style. The rarer fallbacks (post-loop nothing-scheduled, concurrent No critical blockers found. sequenceDiagram
participant P1 as useGeminiStream
participant P2 as Tool scheduler
participant P3 as History and Ink Static
P1->>P1: thought streams, ToolCallRequest arrives
Note over P1: defer commit, freeze duration, mark finalized
P1->>P2: batch executes, rendering unchanged
P2-->>P1: onComplete with finished batch
alt every call successful and collapsible
P1->>P3: commit merged thought line plus suppressed group
else edit, command, error, cancel, or images present
P1->>P3: commit thought as-is, group renders normally
end
Note over P1: cancel, error, retry, fallback, nothing scheduled all abort the deferral and commit as-is
Files changed (9)
Testing evidenceThis is an unattended CI run — I did not build or execute the PR's code. Evidence below is the PR's own CI on the reviewed commit, read through the API, plus static review. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The main unit suite ( Not verified here: the live TUI behavior. The unit suite pins the commit-timing and merge-predicate logic, but whether the fold looks right in a real session (spacing, Ctrl+O transcript, click-to-expand) is a visual claim — the before/after in the PR body is the author's own tmux run on macOS, attributed as their claim rather than as independent evidence. Sandboxed verification would settle this: 中文说明代码审查实现方案与独立推演的设计完全一致:Ink 的 通过阅读代码验证的点:
测试覆盖合并路径、含不可折叠工具(edit)的批次、被取消的工具、本地取消悬置场景,以及合并标签("Thought for 9s, searched 2 patterns"、"Thought briefly, read a.ts")与 finalized 样式的渲染测试。较少的兜底路径(循环后未调度、批次中途并发 未发现关键阻塞问题。时序图见英文部分(思考提交延迟 → 批次完成时判定合并或原样提交;所有提前退出路径中止延迟并原样提交)。 测试证据本次为无人值守 CI 运行——未构建或执行 PR 代码。证据为通过 API 读取的该提交自身 CI 结果与静态审查。主单测 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean static review with the deferral lifecycle fully audited; holding back the last point because the main unit suite hasn't landed green yet and the visual behavior rests on the author's tmux captures until someone runs the TUI lane. Stepping back: this is a well-built PR. The problem is observable (inconsistent spacing below thought lines, a redundant second summary line), the direction matches the Claude Code interaction style this project tracks, and the implementation takes the only path the rendering model allows — Ink's Reservations, all non-blocking: the post-loop "nothing scheduled" fallback and the concurrent-new-prompt path have no dedicated unit tests; and the live rendering itself is verified only by the author's macOS tmux run so far — Verdict: approve. CI is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度:4/5 —— 静态审查干净、延迟窗口所有出口均已核查;保留一分是因为主单测尚未落绿,且在有人运行 TUI 沙箱通道之前,视觉效果仍依赖作者的 tmux 捕获。 整体看:这是一个构建良好的 PR。问题是可观察的(思考行下方间距不一致、多出一行摘要),方向与本项目对标的 Claude Code 交互风格一致,实现选择了渲染模型唯一允许的路径——Ink 的 保留意见(均不阻塞):循环后"未调度"兜底与并发新提示路径没有专门单测;实际渲染目前仅有作者在 macOS 上的 tmux 验证——如需在合并前做视觉确认,维护者可运行 结论:批准。由于该提交的 CI 仍在运行,批准将推迟到 CI 在该提交上全部落绿后执行——finalize 任务会在检查落定后代发绑定该提交的批准;若有检查变红则不会批准。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) and tmux-testing were skipped in CI and their suites did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": none — the full chunk and all mandated source traces completed within budget..
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) and tmux-testing were skipped in CI and their suites did not run locally。
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 3)":none — the full chunk and all mandated source traces completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.14)
- Forward isPending && !finalized to ThinkBody so a finalized thought body is no longer sliced as streaming content while the deferred batch runs (the tail was hidden without a truncation cue). - Abort the merge deferral on ABNORMAL finish reasons so the deferred thought commits above the warning info item, keeping the thought-first ordering every other abort site maintains; a normal STOP keeps the deferral alive so the batch can still merge. - Resolve an active deferral BEFORE prepareQueryForGemini appends the new user item so a concurrent prompt (?btw) lands below the previous turn's thought instead of above it. - useHistoryManager.compactOldItems: un-suppress a merged tool_group (clear display.mergedIntoThought) when its paired thought is dropped, so the batch falls back to its collapsed summary instead of vanishing. - MainContent (virtual viewport): capture the anchored item before a Ctrl+O fullDetail flip and scrollToItem back to it after the filter re-runs, preserving the reading position when merged groups are inserted/removed mid-list.
Deferral-active coverage for every deferral-resolution site (with no deferral active, abortThoughtMergeDeferral is indistinguishable from the old commitPendingThought, so reverting any site stayed green): - UserCancelled, Error, non-continuation Retry, ModelFallback and Content handlers, the sendMessage catch block, the post-loop "nothing scheduled" fallback (loop-detection halt) and the repeated duplicate-provider-id batch drop: Thought -> ToolCallRequest first, then fire the event and assert the thought commits as-is exactly once (no toolSummary). - Abnormal finish (MAX_TOKENS): thought commits ABOVE the warning info item and the aborted deferral is not resurrected by the later batch. - Concurrent ?btw mid deferral: the deferred thought commits between the two user items (above the new prompt), pending area cleared. - Merge test: pin the frozen durationMs across completion (a refactor charging tool-execution time to the thought now fails), and assert pendingHistoryItems clears after resolution in the merge and both no-merge tests. - Merge-predicate clauses never exercised before: inline images, omitted-image overflow, and memory-op reads block the merge. - Cancelled-tool test: wait for deferral establishment before firing onComplete (the thought-null wait was vacuous), and assert the tool_group exists before checking mergedIntoThought absence. - Local-cancel test: fire onComplete after the cancel (the scheduler still completes cancelled batches) — thought commits exactly once, group unmerged below the info item. - ThinkMessage: expanded finalized-but-pending variant (completed label, collapse hint, no "Thinking") and a body-slicing regression test. - MainContent: merged tool_group hidden outside full detail and re-admitted in full detail; virtual-viewport anchor restored across the fullDetail toggle (and not fought when bottom-stuck). - useHistoryManager: un-suppress merged groups whose paired thought is compacted away; keep the flag when the thought survives.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/components/HistoryItemDisplay.tsx:346 — [probe] finalized forward through HistoryItemDisplay has no test — deleting it keeps 93/93 green (deferred: anchored on code unchanged since round 1)packages/cli/src/ui/hooks/useGeminiStream.test.tsx:12139 — [probe] onComplete-driven deferral tests never assert exactly-once thought commit — double-commit mutant ships 243/243 green (deferred: code-age rule)packages/cli/src/ui/hooks/useGeminiStream.ts:1212 — [probe] oversized-thought tail branch of the deferral guard untested — guard mutant ships 243/243 green (deferred: code-age rule)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…batch ownership R2-1: a Thought arriving after the ToolCallRequest boundary hit handleThoughtEvent with an empty buffer and replaced the finalized deferred thought in the pending slot — the first thought was never committed. Resolve an active deferral (committing the finalized thought as-is) before starting a new thought. R2-2: the deferral was an identity-less flag shared across concurrent turn chains, so whichever batch's onComplete fired first consumed it and could merge another turn's thought into its own group. The deferral now records the arming batch's callIds, and mergeDeferredThoughtWithToolGroup resolves only when the completing group's callIds match; a non-owning batch leaves the deferral armed and commits its group unmerged. Adds regression tests for the Thought->ToolCallRequest->Thought ordering and for the non-owning-batch ownership guard.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 4: running useGeminiStream.test.tsx to confirm the new tests actually pass — the worktree has no node_modules (vitest absent) and install+build exceeds the too…; chunk 1: run npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx in packages/cli (blocked: no node_modules in worktre….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.test.tsx:11499 — [review] multi-tool batches never exercise the deferral tests (re-arm accumulation, multi-id ownership)packages/cli/src/ui/hooks/useGeminiStream.ts:1214 — [review] oversized-thought tail branch at a ToolCallRequest has no testpackages/cli/src/ui/hooks/useGeminiStream.ts:3600 — [review] Cron/Teammate disjuncts of the new-prompt deferral abort are untestedpackages/cli/src/ui/components/HistoryItemDisplay.tsx:195 — [review] the clickable && isActive gate has no testpackages/cli/src/ui/components/HistoryItemDisplay.tsx:346 — [probe] finalized passthrough through HistoryItemDisplay has no testpackages/cli/src/ui/hooks/useGeminiStream.test.tsx:12226 — [review] non-collapsible test lacks the deferral-established sync point its sibling haspackages/cli/src/ui/components/MainContent.test.tsx:850 — [probe] wholesale ScrollableList mock leaves the render-phase capture timing invariant untestedpackages/cli/src/ui/components/messages/ConversationMessages.tsx:442 — [probe] expanded finalized-pending thought renders unbounded in the clamped pending area (non-VP path)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 4:running useGeminiStream.test.tsx to confirm the new tests actually pass — the worktree has no node_modules (vitest absent) and install+build exceeds the too…;chunk 1:run npx vitest run src/ui/components/HistoryItemDisplay.test.tsx src/ui/components/MainContent.test.tsx in packages/cli (blocked: no node_modules in worktre…。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…up-merge # Conflicts: # packages/cli/src/ui/hooks/useGeminiStream.ts
…atches and concurrent streams R3-1: the ownership guard required exact set equality between the completing batch's callIds and the armed set, but the armed set grows per streamed ToolCallRequest while only a subset may be scheduled (replay-suppressed calls) or a mid-turn thought may re-arm only the trailing calls. When the sets diverged, no batch could satisfy the guard — the group committed unmerged and the deferral stranded until a later abort committed the thought BELOW the group. Relax the guard to 'armed set is a subset of the completing group' and prune each replay-suppressed callId from the armed set at scheduling (committing as-is if the set empties). R3-2: the in-stream abort sites resolved the deferral with no ownership check, so a concurrent stream (?btw during Responding) whose first Content/end-of-loop ran after the main turn armed the deferral committed the main turn's finalized thought below the concurrent user item. Record the arming invocation's promptId and gate the Content-case recovery and the end-of-loop stranded fallback on it, so only the owning invocation resolves its own deferral. Adds regression tests for both entrances (subset merge, replay pruning) and the arms-after-submit concurrent-window grouping.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5 (the cap round reported findings, verified after the loop).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/components/HistoryItemDisplay.tsx:345 — [review] the pending-phase render state of a deferred thought (finalized passthrough + clickable&&isActive gate) has no test at the HistoryItemDisplay layer (deferred: anchored on …packages/cli/src/ui/hooks/useGeminiStream.ts:1274 — [review] the oversized-thought tail's immediate-commit at a ToolCallRequest has no test (deferred: anchored on code unchanged since round 3)packages/cli/src/ui/hooks/useGeminiStream.ts:3701 — [review] the Cron and Teammate clauses of the submit-time deferral resolution are untested (deferred: anchored on code unchanged since round 3)packages/cli/src/ui/components/messages/ConversationMessages.tsx:442 — [probe] expanded finalized-pending thought renders unbounded in the clamped pending area, non-VP path (deferred: rounds 2-3, probe-verified then, code unchanged since ro…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:12554 — [review] the non-collapsible deferral test lacks the deferral-established sync point its siblings have (deferred: round 3, code unchanged since round 3)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:reverse audit — did not converge within the reverse-audit round cap of 5 (the cap round reported findings, verified after the loop)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Review: feat(cli): fold completed read/search tool batches into the thought lineThis is a well-structured, well-documented PR. The design doc is thorough, the tests are comprehensive, and the edge-case handling (concurrent streams, cancellation, error, retry, model fallback, duplicate suppression, history compaction) is carefully thought through. High-level assessmentThe core insight — deferring the thought commit from ToolCallRequest time to batch completion time so the merge decision can be made before Ink's The batch-identity collapse (#9420, Specific findingsMedium:
|
Key the merge deferral's cross-stream ownership on the prompt identity stamped into every tool-call request instead of callIds (which are not a stable identity: native Gemini ids are re-minted deterministically per stream and some providers reuse wire ids), guard every in-stream resolution site (error, retry, model fallback, abnormal finish, duplicate-batch drop, replay prune, content, end-of-loop fallback, submit catch) so a concurrent stream can only settle its own deferral, snapshot the armed thought at arm time so a shared pending slot overwritten mid-deferral cannot corrupt the merge, and only arm on the arming invocation's own thought. Also restore the scroll anchor's within-item offset across the full-detail toggle, retarget an anchor on a removed merged group to its paired thought, suppress the transient duplicate pending copy of a merged batch, correct the design doc's expansion model, and harden/expand the associated unit tests (commit-order adjacency, icon/compaction/overflow assertions, shrink-direction and reference-identity anchor tests, abort-site pinning on Retry).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — reached the round cap (5) without converging: rounds 3, 4 and 5 each reported new findings, so audit depth beyond the cap went unexamined.
Not explored to full depth (tool budget reached): "agent test-matrix": none — all planned checks completed (diff fully read; MainContent/HistoryItemDisplay/useGeminiStream/useReactToolScheduler/agentHistoryAdapter/CompactToolGroupD….
Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.ts:1359 — [review] oversized-thought tail branch of the deferral guard untested (deferred rounds 3-5: anchored on code unchanged since round 4)packages/cli/src/ui/hooks/useGeminiStream.ts:1576 — [probe] local cancel consumes a surviving detached continuation's armed deferral — fold loss (deferred: code-age rule)packages/cli/src/ui/hooks/useGeminiStream.ts:1384 — [probe] arming zeroes a continuing concurrent thought's duration — renders 'Thought briefly' (deferred: code-age rule)packages/cli/src/ui/components/HistoryItemDisplay.tsx:195 — [review] finalized thought is mouse-unexpandable during the batch window (deferred: file unchanged since round 4)packages/cli/src/ui/components/MainContent.test.tsx:872 — [probe] one-shot anchor-restore property unpinned (deferred: code-age rule)
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:reverse audit — reached the round cap (5) without converging: rounds 3, 4 and 5 each reported new findings, so audit depth beyond the cap went unexamined。
未探索到全部深度(达到工具调用预算):"agent test-matrix":none — all planned checks completed (diff fully read; MainContent/HistoryItemDisplay/useGeminiStream/useReactToolScheduler/agentHistoryAdapter/CompactToolGroupD…。
未审查:验证——有验证 agent 运行并打开了自己的 brief,但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的,发布的发现不能算作经它验证。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…eign thoughts A non-owning concurrent stream that settles (Content transition, Retry, ModelFallback, abnormal/normal Finished, stream finally) while another invocation's merge deferral is armed previously skipped committing the shared pending slot even when it held that stream's OWN owner-stamped thought, so the thought stranded pending and was committed later out of order (or dropped by a new-prompt reset). Add commitOwnPendingThought: owner-aware, payload-protected commit used by settleThoughtMergeDeferral's non-owner branch, normal Finished, and the stream finally. Also stop the tool-first ToolCallRequest boundary from committing a foreign stream's still-streaming thought: the foreign stream's local buffer rebuilds the slot on its next chunk and its own settlement re-commits the full text, so the early partial commit duplicated the reasoning. Leave it pending for its owner. Tests: owner-aware settlement regression, foreign-thought dedup regression, ModelFallback concurrent-settlement row, and continuation-Retry must-not-settle companion.
…ests Suppress a merged batch's live pending tool_group copy on the legacy Static render path (useTerminalBuffer=false) too, not just the VP allVirtualItems memo — the committed merged group is filtered out of visibleHistory when fullDetail is off, but the scheduler keeps its live display copy until onComplete's await returns, so the default renderer otherwise showed the merged thought line PLUS a duplicate group row. Hoist the mergedBatchIds identity set into a shared memo used by both paths. Anchor restore (Ctrl+O flip): preserve the within-item pixel offset only when the anchored item does not shrink across the flip. Toggle-OFF collapses thought bodies to a 1-line label, so re-applying the pre-toggle offset overshoots the now-short item; restore thoughts to their top on toggle-OFF. Non-thought items keep their height and their offset. Tests: Static-path suppression counterpart, fullDetail=true symmetric suppression pin, toggle-OFF thought anchor restore, and direct VirtualizedList/ScrollableList getScrollAnchor coverage.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — reached the round cap (5) without converging: rounds 3 and 4 each reported confirmed findings and round 5's sole finding was rejected after verification, so no two consecutive dry rounds were reached and audit depth beyond the cap went unexamined.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useGeminiStream.test.tsx:11852 — [probe] TS2352 type error in the successfulReadToolCall helper's single cast (double-cast is the file pattern; hidden only by the TODO(5691) typecheck exclusion)packages/cli/src/ui/components/HistoryItemDisplay.tsx:346 — [probe] finalized prop plumbing into ThinkMessage has no paired test — deleting the line ships green (109/109) and regresses the deferred thought's completed stylingpackages/cli/src/ui/hooks/useHistoryManager.test.ts:343 — [probe] un-suppression composed with same-pass output clearing untested — the ...item-for-group mutant ships green (30/30) and re-suppresses the oldest merged groupspackages/cli/src/ui/hooks/useGeminiStream.test.tsx:13777 — [probe] colliding-callId test never asserts the foreign group's existence — splicing the commit out keeps it greenpackages/cli/src/ui/components/MainContent.test.tsx:981 — [probe] remap test never asserts viewOffset — pre-remap-offset mutant ships green and restores 2 rows past the collapsed thought's toppackages/cli/src/ui/components/MainContent.test.tsx:1020 — [probe] toggle-OFF zero-offset arm for gemini_thought_content has no test — deleting the disjunct ships green (42/42)packages/cli/src/ui/hooks/useGeminiStream.ts:1409 — [review] oversized-thought tail branch of the deferral guard has no paired test (deferred rounds 3-5)packages/cli/src/ui/hooks/useGeminiStream.ts:3899 — [review] submit-time deferral resolution's Cron/Teammate arms are untested (deferred rounds 3-4)packages/cli/src/ui/hooks/useGeminiStream.ts:3367 — [review] the owner guard on replay-pruning is untested — a concurrent redelivery of the owner's in-flight call is its stated purposepackages/cli/src/ui/components/HistoryItemDisplay.tsx:195 — [review] clickable={clickable && isActive} change has no paired test (deferred rounds 3-5)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:reverse audit — reached the round cap (5) without converging: rounds 3 and 4 each reported confirmed findings and round 5's sole finding was rejected after verification, so no two consecutive dry rounds were reached and audit depth beyond the cap went unexamined。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 10 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…nt, Static/toggle render fixes - Mint prompt_ids via an atomic ref-backed sequence (never below the render-scoped stats counter): concurrent ?btw submissions previously read the same getPromptCount() and minted identical ids, making the streams indistinguishable to the ownership layer. - Local cancel (cancelOngoingRequest) and UserCancelled-event settlement are now owner-aware: a surviving concurrent stream's armed deferral stays armed for its own batch instead of being re-homed under the cancelled turn. - Static path suppresses a merged batch's live pending copy under fullDetail too (the #9420 collapse is VP-only). - Toggle-OFF anchor restore zeroes the offset for tool_group rows as well (they shrink when fullDetail collapses them).
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the macOS/Windows unit-matrix jobs were also skipped; the same suite ran green on Linux in CI and locally).
Not explored to full depth (tool budget reached): chunk 4: executing the four new tests ( npx vitest run src/ui/components/MainContent.test.tsx ) — worktree has no node_modules/core dist and installing into the shared r….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/components/HistoryItemDisplay.tsx:195 — [probe] clickable && isActive gate has no paired test; the mutant…packages/cli/src/ui/components/HistoryItemDisplay.tsx:346 — [probe] finalized pass-through untested at the HistoryItemDisplay…packages/cli/src/ui/components/MainContent.test.tsx:814 — [probe] toggle-OFF zero-offset arm for gemini_thought_content…packages/cli/src/ui/components/MainContent.test.tsx:844 — [probe] one-shot anchor-restore disarm mutation-invisible; no…packages/cli/src/ui/components/MainContent.test.tsx:891 — [probe] bottom-stuck skip pinned for toggle-ON only; toggle-OFF…packages/cli/src/ui/components/MainContent.tsx:348 — [probe] batchId membership check mutation-invisible;…packages/cli/src/ui/components/MainContent.tsx:469 — [probe] toggle-OFF remap recognizes only the committed group copy;…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:13325 — [probe] prune guard's owner clause untested concurrently; mutant…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:13325 — [probe] re-arm accumulation (armedCallIds.add) mutation-invisible;…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:13433 — [probe] replay-prune size-0 abort branch unexercised; mutant…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:13946 — [probe] abort-path slot-protection clause mutation-invisible;…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:14564 — [probe] commitOwnPendingThought's owner-rejection clause unpinned;…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:15341 — [probe] duplicate-drop settle owner-awareness mutation-invisible;…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:15594 — [probe] merge predicate's every() quantifier unpinned; mixed…packages/cli/src/ui/hooks/useGeminiStream.test.tsx:15594 — [probe] merge-predicate Success clause with an errored tool…packages/cli/src/ui/hooks/useGeminiStream.ts:3126 — [probe] Finished-normal commitOwnPendingThought masked by the…
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the macOS/Windows unit-matrix jobs were also skipped; the same suite ran green on Linux in CI and locally)。
未探索到全部深度(达到工具调用预算):chunk 4:executing the four new tests ( npx vitest run src/ui/components/MainContent.test.tsx ) — worktree has no node_modules/core dist and installing into the shared r…。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 16 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
R10-1: settleThoughtMergeDeferral's no-deferral fallback was the owner-agnostic commitPendingThought, so the local-cancel and stream- catch settlement sites committed whatever occupied the shared slot — including a CONCURRENT stream's in-flight thought — under the settling turn's timestamp, clearing the owner stamp so the other stream's next chunk rebuilt the slot and its settlement committed the same reasoning again (duplicated, truncated, misplaced). The fallback is now commitOwnPendingThought(userMessageTimestamp, streamPromptId): identical when the slot holds this invocation's own thought or is empty; a no-op for a foreign occupant. R10-2: the new-prompt reset guarded the pending-slot clear on deferral presence, so once the deferral was disarmed it wiped a surviving concurrent stream's thought out of the shared slot — its settlement then found an empty slot and the reasoning was permanently dropped. The clear is now keyed on slot ownership: the outgoing interaction's prompt id is captured before activeInteractionPromptIdRef is overwritten, and the reset clears only unowned occupants or the outgoing turn's own unsettled thought. Tests (both red when the respective fix is reverted): - 'does not commit a concurrent in-flight thought when settling without a deferral': ?btw mid-thought before its first TCR occupies the slot; foreground cancel commits nothing and leaves the slot. - 'keeps a surviving stream thought in the slot across the next prompt reset': armed main deferral, ?btw overwrites the slot, cancel settles main's snapshot, the next UserQuery reset keeps the ?btw occupant, and ?btw's settlement commits it exactly once. All 273 useGeminiStream tests green.
Dismissing stale bot review state. Round 10's two fresh Criticals are fixed in 29b2d40 with pinned regression tests (both verified red when the respective fix is reverted; all 273 useGeminiStream tests green): R10-1 (owner-agnostic settlement fallback now commitOwnPendingThought) and R10-2 (new-prompt reset now keys the slot clear on ownership). The remaining R5-3 finding is the long-escalated arms-after-submit design issue — author-reproduced in round 5, intentionally open pending a maintainer decision, tracked in its original thread; its duplicate refiling was resolved as a duplicate. No other outstanding requested changes remain.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
10 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R11-4 clickable && isActive gate has no paired test — already recorded in the rounds 3-10 deferral lists (HistoryItemDisplay.tsx:195)
- R11-5 oversized-thought tail commit at the TCR boundary has no paired test — already recorded in the rounds 3-10 deferral lists (useGeminiStream.ts:1430)
- R11-6 toggle-OFF offset-zeroing disjunct for gemini_thought_content unpinned — already recorded in the rounds 8-10 deferral lists (MainContent.test.tsx:1030)
- R11-7 one-shot anchor-restore null-clear unpinned — already recorded in the rounds 5/7/10 deferral lists (MainContent.test.tsx:816)
- R11-8 UserCancelled missing from the concurrent-stream settlement table — already recorded in the round-7 deferral list (useGeminiStream.test.tsx:13946)
- R11-9 finalized wire-through has no render-level test — already recorded in the rounds 2-10 deferral lists (HistoryItemDisplay.tsx:346)
- R11-10 colliding-callId test never asserts the foreign group's existence — already recorded in the round-6 deferral list (useGeminiStream.test.tsx:13777)
- R11-11 merged-batch pending-suppression selectivity unpinned — already recorded in the round-9 deferral list (MainContent.tsx:341)
- R11-12 replay-prune owner gate has no foreign-stream test — already recorded in the round-9 deferral list (useGeminiStream.test.tsx:13878)
- R11-13 repeated-duplicate batch-drop settle site has only a single-stream test — already recorded in the round-7 deferral list (useGeminiStream.test.tsx:13946)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped after round 4: rounds 3-4 re-derived only findings already recorded in earlier rounds; audit depth beyond round 4 unexamined.
Not explored to full depth (tool budget reached): chunk 3: running MainContent.test.tsx under vitest to confirm green — the shared worktree has no node_modules , and a full monorepo npm ci plus workspace builds exc….
Convergence: round 11 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 3 (2 new). Findings keep coming back to the same files: packages/cli/src/ui/hooks/useGeminiStream.ts (findings in rounds 5, 10; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 10 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:reverse audit — stopped after round 4: rounds 3-4 re-derived only findings already recorded in earlier rounds; audit depth beyond round 4 unexamined。
未探索到全部深度(达到工具调用预算):chunk 3:running MainContent.test.tsx under vitest to confirm green — the shared worktree has no node_modules , and a full monorepo npm ci plus workspace builds exc…。
收敛情况:第 11 轮发布了 2 条行内评论,其中 1 条是首次提出;上一轮发布了 3 条(其中 2 条首次提出)。发现反复回到同一批文件:packages/cli/src/ui/hooks/useGeminiStream.ts(第 5、10 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
…type (R11-1) The UserQuery branch of the pre-query deferral-resolution block aborted an armed merge deferral unconditionally. After a foreground cancel settles owner-aware and leaves an admitted concurrent stream's deferral armed (the hook drops to Idle, the surviving batch keeps running on its detached signal), the next user query entered the block and aborted the SURVIVOR's deferral — committing its frozen thought under the new turn's timestamp and stranding its batch without the fold: the exact re-home outcome the sibling settlement sites spent rounds 4-10 closing. All three submit types (UserQuery/Cron/Teammate) now settle through settleThoughtMergeDeferral keyed on activeInteractionPromptIdRef, which still holds the OUTGOING turn's prompt id at this point (the overwrite happens further down): the outgoing turn's own armed deferral matches and aborts exactly as before — the pinned mid-deferral second-query contract is unchanged — while a surviving concurrent stream's deferral is left armed for its own batch completion. Pinned by 'keeps a surviving concurrent deferral armed across the next user query' (deferral armed by an admitted ?btw stream, Esc-cancel, then a next UserQuery: nothing commits, the frozen thought stays pending, and the surviving batch folds it); verified red when the unconditional UserQuery abort is restored. All 274 useGeminiStream tests green.
…ge (#9503) Main renamed useGeminiStream.ts → use-llm-stream.ts and the Gemini* identifiers to Llm*. Git rename detection carried this PR's pre-rename additions into the renamed files, leaving stale identifiers that broke the CI build: - use-llm-stream.ts: ServerGeminiFinishedEvent → ServerLlmFinishedEvent (import already present; fixes TS2552) - useHistoryManager.test.ts: HistoryItemGeminiThought → HistoryItemLlmThought (fixes TS2724) - use-llm-stream.test.tsx: ServerGeminiEventType → ServerLlmEventType, useGeminiStream → useLlmStream, MockedGeminiClientClass → MockedLlmClientClass, responseSubmittedToGemini → responseSubmittedToLlm (file is excluded from tsc --build but vitest runs it, so the undefined value references would fail the Test job at runtime) Intentionally preserved: 'gemini'/'gemini_thought' history-item string literals (types.ts keeps them), core's deprecated Gemini* aliases, and prose/comments.
…uery (#9503) submitQuery's body never references abortThoughtMergeDeferral directly — the abort path runs inside processLlmStreamEvents, which tracks the dependency itself. react-hooks/exhaustive-deps flagged the extra entry and CI lints with --max-warnings 0, failing the Run ESLint step.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R12-2 clickable && isActive gate has no paired test (HistoryItemDisplay.tsx:195) — already reported as R11-4, recorded in the rounds 3-10 deferral lists
- R12-3 finalized wire-through has no render-level test (HistoryItemDisplay.tsx:346) — already reported as R11-9, recorded in the rounds 2-10 deferral lists
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.test.tsx:12734 — [probe] non-collapsible deferral test's only sync is a vacuous thought-null wait
Convergence: round 12 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 12 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 12 轮发布了 2 条行内评论,其中 1 条是首次提出;上一轮发布了 2 条(其中 1 条首次提出)。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 2 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 1),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Teammate clause of the submit-time deferral settlement untested — already recorded in the rounds 3-9 deferral lists (useGeminiStream.ts:3899/3935/3954)
- colliding-callId test never asserts the foreign group's existence — already recorded in the round-6 deferral list and re-recorded as R11-10 (useGeminiStream.test.tsx:13777)
- non-collapsible deferral test's only sync is a vacuous thought-null wait — already recorded in the round-10 and round-12 deferral lists (use-llm-stream.test.tsx:12734)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.test.tsx:13422 — [review] R13-2 the R12-1 test's disarm proof asserts only absences — never that the batch's tool_group was committed unmerged
Convergence: round 13 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/cli/src/ui/hooks/use-llm-stream.ts (findings in rounds 5, 12; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后(第 13 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 13 轮发布了 2 条行内评论,其中 1 条是首次提出;上一轮发布了 2 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/cli/src/ui/hooks/use-llm-stream.ts(第 5、12 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 2 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 1),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R11-10 colliding-callId test never asserts the foreign group's existence — already reported (round-6 deferral list, re-recorded as R11-10)
- R11-12 replay-prune owner gate has no foreign-stream test — already reported (round-9 deferral list, re-recorded as R11-12)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
1 fails-closed, new-surface Critical(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 14, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/ui/hooks/use-llm-stream.ts:1687 — [review] Critical [fails-closed] [new-surface] R14-2: An armed merge deferral is stranded permanently when its owning stream is killed by the repeat-cancel branch of cancelOngoingRequest …packages/cli/src/ui/AppContainer.tsx:1107 — [probe] R13-1 seed test leaves the userTurnCount term of the max unwitnessedpackages/cli/src/ui/hooks/use-llm-stream.ts:3439 — [probe] replay-prune-to-empty abort branch has no testpackages/cli/src/ui/hooks/use-llm-stream.test.tsx:13655 — [probe] R10-2 test comments misattribute the settlement mechanismpackages/cli/src/ui/hooks/use-llm-stream.test.tsx:15306 — [probe] UserCancelled settlement site has no concurrent-stream witnesspackages/cli/src/ui/hooks/use-llm-stream.test.tsx:16672 — [probe] duplicate-drop settle has no foreign-stream witness
Convergence: round 14 posted 2 inline comment(s), 1 of them reported for the first time; the previous round posted 2 (1 new). Findings keep coming back to the same files: packages/cli/src/ui/hooks/use-llm-stream.ts (findings in rounds 5, 13; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
1 条 fails-closed 且 new-surface 的 Critical 在已解析的 critical 发布下限之外被起草为行内评论;CLI 已将其移入下方延后清单(下限强制执行)。
收敛姿态下延后(第 14 轮,非阻断)——已记录,本轮不要求修改;其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface:未认证任何错误结果,且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中:共 6 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 14 轮发布了 2 条行内评论,其中 1 条是首次提出;上一轮发布了 2 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/cli/src/ui/hooks/use-llm-stream.ts(第 5、13 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,可以避免循环反复推导同一组发现;本 PR 的评审已解析为 critical 发布下限。(仅为观察——本轮评审未因此扣留任何内容。)
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 2 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 1),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…under a foreign armed deferral (R14-1) (#9503)
|
@/tmp/9503_note2.md |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5: rounds 1-5 each reported findings (two later rejected in verification), so audit depth beyond the cap went unexamined.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
10 Suggestion(s) were drafted inline past the resolved critical posting floor; the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 15, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.ts:1338 — [review] R15-7: the slot-retention branch of the owner-side abort has no test — making the clear unconditional ships green and silently drops a concurrent stream's thought. Main turn arms a…packages/cli/src/ui/hooks/use-llm-stream.ts:1457 — [review] R15-11: the R14-1 else branch — this round's fix — closes the two-stream case but leaves a reachable three-stream interleaving that loses a thought from history, the exact loss cla…packages/cli/src/ui/hooks/use-llm-stream.ts:1477 — [review] R15-2: the owning stream's oversized gemini_thought_content tail committing as-is at its own ToolCallRequest boundary has no test — only the foreign-tail variant is covered, and …packages/cli/src/ui/hooks/use-llm-stream.ts:3450 — [review] R15-6: the owner check added to the replay-prune branch is unwitnessed — the only replay-prune test prunes from the owning stream, so removing the owner equality ships green. Finge…packages/cli/src/ui/hooks/use-llm-stream.ts:3453 — [review] R15-1: the prune-to-empty abort branch added here has no test — a future refactor can delete it with every existing test green and strand the armed deferral. The general prune case…packages/cli/src/ui/hooks/use-llm-stream.ts:3993 — [review] R15-5: the Teammate branch of the submit-time settle added in this diff has no test — UserQuery and Cron each have one, but nothing submits SendMessageType.Teammate against an …packages/cli/src/ui/hooks/use-llm-stream.test.tsx:12743 — [review] R15-10: the non-collapsible no-merge test completes the batch after only the vacuous thought-null wait, missing the deferral-established sync point its sibling cancelled-too…packages/cli/src/ui/hooks/use-llm-stream.test.tsx:15289 — [review] R15-9: the foreign Error matrix case witnesses the new catch-block settle (use-llm-stream.ts:4608) only because this fixture's Error event value omits .error , making cl…packages/cli/src/ui/hooks/use-llm-stream.test.tsx:15322 — [review] R15-3: the it.each foreign-ownership matrix covers Error, non-continuation Retry, abnormal Finished and ModelFallback but omits UserCancelled — the one remaining in-stream…packages/cli/src/ui/hooks/use-llm-stream.test.tsx:16744 — [review] R15-8: the owner-aware settle at the repeated-duplicate batch-drop site ( settleThoughtMergeDeferral(userMessageTimestamp, promptId) , use-llm-stream.ts:3422) is witnessed o…
中文说明
仅完成部分审查,审查缺口已披露。
未审查:reverse audit — did not converge within the reverse-audit round cap of 5: rounds 1-5 each reported findings (two later rejected in verification), so audit depth beyond the cap went unexamined。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
10 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论;CLI 已将其移入下方延后清单(下限强制执行)。
收敛姿态下延后(第 15 轮,非阻断)——已记录,本轮不要求修改:共 10 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…up-merge # Conflicts: # packages/cli/src/ui/hooks/use-llm-stream.ts
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D16-3 oversized own-stream thought tail untested — already reported (round-15 deferral list, R15-2)
- D16-4 prune-to-empty abort branch untested — already reported (round-14 and round-15 deferral lists, R15-1)
- D16-8 it.each matrix omits the UserCancelled settle site — already reported (round-14 and round-15 deferral lists, R15-3)
- D16-12 colliding-callId test never asserts the foreign group's existence — already reported (round-6 deferral list, re-recorded as R11-10 in round 14)
- D16-14 submit-time settlement Teammate branch untested — already reported (round-15 deferral list, R15-5)
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): chunk 3: did not execute MainContent.test.tsx — the review worktree has no node_modules / dist and a full npm ci + npm run build prerequisite exceeded the remain….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Deferred under the convergence posture (round 16, not a blocker) — recorded, not requested in this round; 1 Critical(s) among them are deferred by their axes — fails-closed on new surface, where no wrong result is certified and the merge base had neither the surface nor the defect — and remain follow-up work recorded in the findings artifact:
packages/cli/src/ui/hooks/use-llm-stream.ts:2611 — [probe] Critical [fails-closed] [new-surface] armed deferral of a surviving concurrent (?btw) stream strands permanently when that stream is cancelled after a local foreground cancel — merg…packages/cli/src/ui/components/HistoryItemDisplay.tsx:194 — [probe] new clickable={clickable && isActive} gating has no test — a regression ships green and advertises a dead click for finalized-pending thoughts in VP modedocs/design/thought-tool-group-merge.md:143 — [review] design doc and MainContent comment reference nonexistent useGeminiStream.ts — the feature's durable map points at wrong paths from day onepackages/cli/src/ui/components/MainContent.test.tsx:872 — [probe] one-shot semantics of the fullDetail-flip anchor restore are unpinned — mutant deleting the anchorRestoreItemRef clear ships green and re-issues scrollToItem on every commitpackages/cli/src/ui/components/MainContent.tsx:502 — [probe] toggle-OFF zero-offset shrink set's gemini_thought_content clause is unexercised — mutant removing it ships green (depth overshoot on toggle-OFF)packages/cli/src/ui/hooks/use-llm-stream.ts:4429 — [review] diff-added comments cite nonexistent prepareQueryForGemini / processGeminiStreamEvents — the order-sensitive invariants point at phantomspackages/cli/src/ui/components/MainContent.test.tsx:1473 — [probe] merged-batch suppression tests never pin that an unrelated pending tool_group keeps rendering — a widened suppression predicate ships greenpackages/cli/src/ui/hooks/use-llm-stream.test.tsx:15483 — [probe] R12-1 disarm proof never asserts the old batch's group was committed — a conditional group commit ships greenpackages/cli/src/ui/components/HistoryItemDisplay.tsx:346 — [probe] finalized={itemForDisplay.finalized} forwarding has no test witness — deleting the line ships 111 tests greenpackages/cli/src/ui/components/MainContent.test.tsx:891 — [probe] bottom-stuck skip is only pinned for toggle-ON — mutant disabling it for toggle-OFF ships green, jumps the viewport and kills auto-followpackages/cli/src/ui/components/MainContent.test.tsx:814 — [probe] anchor-restore tests never render with pendingHistoryItems — mutant gating capture on pending-zero ships green and disables restore during streamingpackages/cli/src/ui/hooks/use-llm-stream.ts:1580 — [probe] arm-time duration freeze reads the un-owner-scoped thoughtStartTimeRef — a concurrent thought-start freezes an understated duration (probe: 48ms frozen over a 411ms window)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (2 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 0), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 3:did not execute MainContent.test.tsx — the review worktree has no node_modules / dist and a full npm ci + npm run build prerequisite exceeded the remain…。
未审查:反向审计——评审时间预算不足,未能开始第 5 轮。
收敛姿态下延后(第 16 轮,非阻断)——已记录,本轮不要求修改;其中 1 条 Critical 按其失败方向与对照基线延后——fails-closed 且 new-surface:未认证任何错误结果,且 merge base 既无该功能面也无该缺陷——作为后续工作记录在 findings 工件中:共 12 条(原文未翻译,列表见上方英文部分)。
残余风险:本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在(本轮 2 条 Critical),首次发现的速率没有下降(本轮 1,上一轮 0),且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议:land-with-residual-risk——出口是 maintainer 的风险接受决定(合入并承担残余风险),而非再开一轮评审。供该决定使用的残余风险清单(maintainer 填写):按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
CI note: the red checks on this PR in the last ~6h are runner-fleet degradation, not PR content. Evidence:
Response is mechanical rerun until the fleet recovers; no code change is indicated. If a maintainer sees this note going stale (fleet recovered), disregard. 中文说明CI 说明:本 PR 最近约 6 小时的红灯是 runner 集群降级所致,不是 PR 内容问题。 证据:
对策是机械性重跑直至集群恢复,无需任何代码改动。若维护者看到本帖时集群已恢复,请忽略。 |
…9503) A batch whose deferred thought resolved as a merge already renders its summary as the folded thought line's toolSummary suffix, but the continuation loop still fired generateToolUseSummary for the same batch: the emission gate filtered only status === 'success' and goal tools, and the staleness check passes because merged groups deliberately stay in history — so every merged batch showed two summaries (the folded suffix plus a redundant ● line) whenever a fast model was configured (emitToolUseSummaries defaults to true). Record the batch's callIds when the deferral resolves as a merge and empty the emission gate's successfulTools for that batch, so no fast-model call fires and no tool_use_summary item is added. Adds a regression test: a mergeable batch with emitToolUseSummaries enabled asserts zero generateText calls and zero tool_use_summary items; removing the exclusion turns it red (verified). Full file 298/298 green locally.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- the replay-prune owner clause has no foreign-stream test (use-llm-stream.ts:3566) — already reported (rounds 14-15 deferral lists, R15-6)
- the R12-1 disarm proof asserts only absences (use-llm-stream.test.tsx:15571) — already reported (round-16 deferral list, use-llm-stream.test.tsx:15483)
- the non-collapsible deferral test lacks the deferral-established sync point (use-llm-stream.test.tsx:14906) — already reported (rounds 3/10/12/15 deferral lists, R15-10)
Not reviewed: build-and-test — the packages/cli suite did not run: the harness's per-call build budget (570s) was spent before the prerequisite workspace builds reached the changed workspace (7 of 16 built cleanly, none in diff scope); the suite is unrun, not failing.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/use-llm-stream.ts:6056 — [probe] D17-1 the R16-1 exclusion reconstructs the merged-batch skip verdict from callId membership at gate time — a co-drained batch loses its summary, stranded markers suppress later reus…packages/cli/src/ui/hooks/use-llm-stream.test.tsx:14832 — [probe] D17-2 the R16-1 test completes the batch without the deferral-established sync point — flakes red on correct code under a stream-loop timing shift
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — the packages/cli suite did not run: the harness's per-call build budget (570s) was spent before the prerequisite workspace builds reached the changed workspace (7 of 16 built cleanly, none in diff scope); the suite is unrun, not failing。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 17 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
What this PR does / 这个 PR 做了什么
When the model thinks and then immediately runs information-gathering tools, the TUI used to render two separate lines: a collapsed thought line ("Thought for 9s (ctrl+o to expand)") and, below it, the completed tool batch summary ("Searched 'alpha', 'beta'"). Because the gap below the thought line is owned by whichever item follows it, the spacing under the same thought line was inconsistent from turn to turn (blank line before a fresh assistant block, none before a tool group).
This change folds a completed all-read/search/list tool batch into the thought line itself, Claude Code style:
While the tools are still executing everything renders as before; the fold happens when the batch finishes. The fold only applies when every call in the batch succeeded and is a collapsible read/search/list tool without inline images or managed-memory side effects — edit/command/agent batches, errors, and cancellations keep today's rendering. The folded tool group stays in history (turn mapping,
/export, SDK surface) and is shown again in the full-detail transcript (Ctrl+O), where the thought is expanded and each tool call renders with its result.The thought commit is deferred from "tool call requested" to "tool batch completed" so the merge decision can be made before Ink's
<Static>freezes the line; every path that ends the window early (cancel, error, retry, model fallback, nothing scheduled) commits the thought as-is.Why this is needed / 为什么需要这个改动
The collapsed thought line is a dense scrollback summary; pairing it with a second summary line for disposable read/search calls wastes a line and, because of the item-type-dependent margin, made the vertical rhythm around thoughts visibly inconsistent. Folding matches the mental model: thinking plus looking things up is one unit of work.
Reviewer Test Plan / 审阅者测试计划
How to verify (TUI):
∴ Thought for …s, searched … (click or ctrl+o to expand). Before: two separate lines with an inconsistent gap.OS tested: macOS (Darwin, tmux E2E with a mocked OpenAI endpoint; see
.qwen/e2e-tests/thought-tool-merge.mdfor the full plan, baseline and after captures).Before (baseline, 0.21.13):
After:
Risks, scope, and follow-ups / 风险、范围与后续
Linked issues / 关联 Issue
N/A — direct UX enhancement request (no existing issue).
🇨🇳 中文完整翻译
这个 PR 做了什么
当模型先思考、随后立即执行信息收集类工具时,TUI 之前会渲染两行:折叠的思考行("Thought for 9s (ctrl+o to expand)")和它下方已完成的工具批次摘要("Searched 'alpha', 'beta'")。由于思考行下方的间距由紧随其后的条目类型决定,同一思考行下方的留白在不同轮次之间并不一致(新回复块前有空行,工具组前没有)。
本改动把"全部为 read/search/list"的已完成工具批次折叠进思考行本身,对齐 Claude Code 的样式:
工具执行期间一切照旧渲染;折叠发生在批次完成时。仅当批次中每个调用都成功、且都是不含内联图片和记忆读写的可折叠 read/search/list 工具时才折叠——edit/命令/子代理批次、出错和取消保持现有渲染。被折叠的工具组仍保留在历史中(轮次映射、
/export、SDK 接口),并在全细节视图(Ctrl+O)中重新显示:思考正文展开,每个工具调用连同结果一起列出。为了在 Ink 的
<Static>冻结该行之前做出合并决策,思考的提交从"收到工具调用请求时"延迟到"工具批次完成时";所有提前结束该窗口的路径(取消、报错、重试、模型降级、没有可调度工具)都会按原样提交思考。为什么需要这个改动
折叠的思考行是高密度的回滚浏览摘要;为一次性的 read/search 调用再配一行摘要既浪费空间,又因为条目类型相关的 margin 导致思考行周围的垂直节奏明显不一致。折叠符合心智模型:思考加查资料是一个完整的工作单元。
审阅者测试计划
验证方式(TUI):
∴ Thought for …s, searched … (click or ctrl+o to expand)。改动前是两行且间距不一致。测试系统:macOS(Darwin,tmux E2E + mock OpenAI 端点;完整计划、基线与改动后捕获见
.qwen/e2e-tests/thought-tool-merge.md)。改动前(基线,0.21.13):
改动后:
风险、范围与后续
关联 Issue
无——直接的 UX 增强请求(没有现成 issue)。