fix(web-shell): release final footer after background agent completion - #10503
Conversation
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献!
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — before reading the diff, my independent proposal for this problem was exactly what the PR does: treat a background launch as finished in the final-turn gate once a terminal notification carrying its
No critical findings, no AGENTS.md violations; the diff is exactly the minimal set (19 production lines) with nothing drive-by. Testing evidence — unattended CI run: I did not build or execute any PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via the API. The unit suite ( CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查 —— 在读 diff 之前,我独立想到的方案正是本 PR 的做法:当携带匹配
无关键问题,无 AGENTS.md 违规;diff 恰为最小集合(生产代码 19 行),无夹带改动。 测试证据 —— 无人值守 CI 运行:未构建或执行任何 PR 代码;以下证据为被审提交上 PR 自身 CI 经 API 拉取的结果。审查时单元测试套件( (CI 表格见上方标记区域) 沙箱验证可以补上最后一环: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean review across every stage; the only open items are the unit suite still in flight and the browser-level visual confirmation being blocked by a pre-existing visuals-job failure, neither of which reflects on the code. Stepping back: this is what a good small fix looks like. The problem is real and observable in the base code — the final-turn gate trusted stale The 4 rather than 5 is honesty about evidence, not doubt about the code: the unit suite and no-AK integration run hadn't landed at review time, and the author's manual verification was macOS-only. The Stage 2 comment names the sandboxed lane that would close the live-render gap. Approval deferred until CI lands green on 中文说明回顾整体:这是一个标准的小型优质修复。问题真实存在,且可以在基线代码中直接观察到——最终 Turn 门禁信任了陈旧的 给 4 分而不是 5 分,是对证据状态的如实描述,而非对代码的怀疑:审查时单元测试套件与 no-AK 集成测试尚未完成,作者的手工验证也仅在 macOS 上进行。Stage 2 评论中已指明可以补上实时渲染侧验证的沙箱通道。 批准将推迟到 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.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6a": executing the new DOM tests in MessageList.dom.test.tsx to confirm green at HEAD — the run fails transforming the unbuilt workspace dep @qwen-code/webui/daem…; "agent 1c": executing the three new DOM tests ( MessageList.dom.test.tsx , the PR's fix witnesses) — blocked by the review worktree lacking built workspace deps; building …; "agent 1c": mutation/red-test verification of the new tests' assertions (consequence of the same build timeout); the tests were verified by tracing their fixtures against p….
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 6a":executing the new DOM tests in MessageList.dom.test.tsx to confirm green at HEAD — the run fails transforming the unbuilt workspace dep @qwen-code/webui/daem…;"agent 1c":executing the three new DOM tests ( MessageList.dom.test.tsx , the PR's fix witnesses) — blocked by the review worktree lacking built workspace deps; building …;"agent 1c":mutation/red-test verification of the new tests' assertions (consequence of the same build timeout); the tests were verified by tracing their fixtures against p…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
448ec0d to
ecc42e8
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)为单个提交。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6c": full web-shell package test suite run (only the changed file components/MessageList.dom.test.tsx was executed; a root npm run build to satisfy deeper prereq…; "agent 3b": locate the daemon producer of background-agent notifications to confirm whether non-terminal kind:'agent' notifications with toolUseId are ever emitted (Finding…; "agent 6b": run the new tests in packages/web-shell/client/components/MessageList.dom.test.tsx to green — the review worktree lacks built workspace dists ( @qwen-code/web….
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 6c":full web-shell package test suite run (only the changed file components/MessageList.dom.test.tsx was executed; a root npm run build to satisfy deeper prereq…;"agent 3b":locate the daemon producer of background-agent notifications to confirm whether non-terminal kind:'agent' notifications with toolUseId are ever emitted (Finding…;"agent 6b":run the new tests in packages/web-shell/client/components/MessageList.dom.test.tsx to green — the review worktree lacks built workspace dists ( @qwen-code/web…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/web-shell/client/components/MessageList.tsx (findings in round 2; 3 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, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 3 条(其中 3 条首次提出)。发现反复回到同一批文件:packages/web-shell/client/components/MessageList.tsx(第 2 轮已出过发现,本轮又有 3 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.3)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 4": (none — no check was cut short); "agent 3c": none — no checks were cut short..
中文说明
未探索到全部深度(达到工具调用预算):"agent 4":(none — no check was cut short);"agent 3c":none — no checks were cut short.。
— qwen3.8-max via Qwen Code /review (v0.22.3)
yiliang114
left a comment
There was a problem hiding this comment.
LGTM at 2a05dae. Verified the production diff end to end:
- The three-way mapping duplication (transcriptToMessages applyBackgroundAgentTaskUpdate, useMessages inline mapping) is consolidated into projectTerminalBackgroundAgentTool with identical semantics — failed stays failed, cancelled/canceled map to completed plus rawOutput.status='cancelled', endTime is passed through (with the notification timestamp as fallback) — and both surviving call sites preserve their previous endTime derivation.
- normalizeTerminalBackgroundAgentTools is safely gated: it only applies terminal statuses (isTerminalBackgroundAgentStatus rejects anything else), only touches background sub-agent tools still in an active status, never mutates in place, and returns the original array when nothing changed. Last notification wins per callId, which is the right authority.
- backgroundAgentCompletionForMessage's status===undefined → 'completed' keeps legacy notification shapes working, and non-terminal explicit statuses are ignored rather than projected.
- Regression coverage lands for the exact reported shapes: ask_user_question waiting, two delayed terminal notifications, one stable final footer, readonly replay.
CI note: Test (ubuntu) and the bot review are still running at approval time; the merge queue will gate on them.
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approved.
Reviewed with AI assistance.
What I checked
Diff scope: 6 files, +366/-73. Four production files (toolClassification.ts, transcriptToMessages.ts, MessageList.tsx, useMessages.ts) plus two test files.
Core fix — normalizeTerminalBackgroundAgentTools:
Called as a pre-pass inside groupParallelAgents, it scans all messages for terminal background-agent completion notifications, builds a callId→completion map, then projects each still-active (isActiveToolStatus) background tool to its terminal state before the grouping pass runs. This is why the footer was not releasing: the grouped display was seeing pending tool status even after the completion notification had arrived.
Refactoring correctness:
- isTerminalBackgroundAgentStatus and projectTerminalBackgroundAgentTool were moved/created in toolClassification.ts and are now the single source of truth for status classification and tool projection. useMessages.ts correctly deletes its local copy and imports the shared one.
- transcriptToMessages.ts switches from the mutable applyBackgroundAgentTaskUpdate to the pure projectTerminalBackgroundAgentTool. Behaviour-preserving.
- backgroundAgentCompletionForMessage now returns null for non-terminal statuses (e.g. in_progress). Previously it would have returned { callId } for in-progress notifications, which would have caused premature completion. The fix is correct; the new test "does not normalize an explicitly non-terminal agent notification" pins it.
Double-normalization safety: Messages normalized by transcriptToMessages.ts or reconcileBackgroundAgentResolutions will have a non-active status; normalizeTerminalBackgroundAgentTools's isActiveToolStatus guard skips them. No double-application possible.
Test validity:
- "normalizes a %s agent notification before grouping" (failed/cancelled/canceled) verifies the status and rawOutput mapping through groupParallelAgents end-to-end
- "does not normalize an explicitly non-terminal agent notification" pins the null gate
- "releases the latest turn after matched delayed agent notifications" exercises the timer path
- All new assertions on assistantActions use .toBe("false") / .toBe("true") — no weakened .not.toBe("true") forms in the current head
Checked callers: groupParallelAgents is called at MessageList.tsx:514; the pre-pass is invisible to callers. backgroundAgentCompletionForMessage is called from normalizeTerminalBackgroundAgentTools (new) and backgroundAgentCompletion (existing); both paths now benefit from the richer return type.
Cross-check against existing review comments
R1-2 (missing callId-specificity mutation test): Confirmed as a valid observation. The existing "normalizes matched terminal agent notifications before grouping" test uses agents that all match their notifications, so a mutant replacing updates.get(tool.callId) with any-first-completion would pass green. Not a blocker for this fix, but a real coverage gap.
R2-1 (normalization only reaches groupParallelAgents consumers): Plausible. Consumers that operate on the original message array outside groupParallelAgents still read the raw tool.status. Scoped to the footer-release codepath which was the stated fix target; not a defect introduced by this PR.
R3-2 (no tests for failed/cancelled/canceled in normalization): Refuted. MessageList.test.ts adds it.each([['failed', ...], ['cancelled', ...], ['canceled', ...]])('normalizes a %s agent notification before grouping', ...) which drives the projection through groupParallelAgents for all three non-completed terminal statuses.
R3-3 (assertion relaxed to .not.toBe("true")): Cannot confirm at this head. No .not.toBe("true") assertion appears anywhere in the diff for either test file; all assistantActions checks in the new tests use strict .toBe(...).
R2-2/R2-3 (new optional default parameter): Cannot confirm. No new optional default parameter is visible in the diff.
CI
- Integration Tests (no-AK, No Sandbox): PASS
- Capture web-shell visuals: PASS
- Desktop Shell (ubuntu + windows): PASS
- Test (ubuntu-latest, Node 22.x): PENDING at review time
- Test (macOS, Windows): SKIPPED — no platform-specific logic in this change
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Gate APPROVE at head 2a05dae2 on the review conclusion (Test ubuntu still finishing at submission; per maintainer policy CI does not gate this decision). What I verified in the diff itself: (1) the mutation-to-projection refactor in toolClassification.projectTerminalBackgroundAgentTool is status-for-status identical to both deleted copies (failed→failed, cancelled→completed+rawOutput marker, endTime optional) and now has one owner; (2) the new normalizeTerminalBackgroundAgentTools pass only re-projects background sub-agent launch rows that are still in an active status when a terminal background_notification with the matching toolUseId exists — unknown-status notifications are ignored, non-agent and settled rows are untouched, so #8787's gates stay strict (the ask_user_question and still-active cases are pinned in the new tests); (3) it runs at the single display-item entry point, is pure and idempotent, and readonly replay goes through the same projection. 8/8 threads resolved, yiliang114 approved this exact head.
中文:在 head 2a05dae2 按 Review 结论通过。三点已逐行核过:投影函数与被删的两处旧逻辑状态映射完全一致且单一归属;新归一只处理「终端通知 + 匹配 toolUseId + 行仍 active」的组合,未知状态忽略、非 agent 行不动,#8787 的门禁未被放宽(ask_user_question 与仍活跃用例均有新测试钉住);归一挂在唯一显示入口、纯函数幂等,只读回放走同一投影。8 条线程全关闭,yiliang114 同 head 已批。
What this PR does
This PR makes terminal background-agent notifications authoritative for the matching background launch row when Web Shell classifies the final Assistant message of a turn. If transcript projection still leaves that launch tool at
pending, a terminalbackground_notificationcarrying the sametoolUseIdnow prevents the stale row from holding back final actions andrenderAssistantTurnFooter.The change stays inside Web Shell final-answer projection. It adds regression coverage for
ask_user_questionwaiting, two background agents with delayed terminal notifications, one stable final footer, and readonly history replay.Why it's needed
PR #8787 correctly added background-agent gates so intermediate narration cannot receive copy, branch, timestamp, or custom footer actions while work is active or awaiting summary. In some completed transcripts, however, the terminal agent notifications and final Assistant supplement were already committed while the original launch rows remained
pending.latestTurnAwaitsAgentSummarywas false, butturnHasActiveBackgroundAgentstill read those stale rows as active, socollectFinalAssistantTurnIdsproduced no entry and all final Assistant actions disappeared.The failure is broader than one host footer: copy, branch, and every integration using
renderAssistantTurnFootershare the same final-message identity.Reviewer Test Plan
How to verify
ask_user_questiontool and confirm no final actions/footer are rendered; answer it and complete the turn.toolUseId, leave the launch rows stale atpending, then append the final summary; confirm only the last Assistant message receives copy/branch/custom-footer identity.Evidence (Before & After)
Before: the deterministic DOM regression reproduced a completed two-agent transcript where both terminal notifications and the final supplement existed, but
data-assistant-actionsstayedfalseandrenderAssistantTurnFooterwas never mounted.After: the same transcript marks only
final-supplementas final, calls the footer with turnask-user-answer, and renders exactly one footer. No browser screenshot is attached because IDE integration was intentionally kept out of scope for this WebShell-only PR.Tested on
Environment
macOS, Node.js 22.12.0.
git diff --check: passed.Risk & Scope
callId; anonymous notifications do not override active state.ask_user_question/ streaming behavior is unchanged.Linked Issues
Related to #10398, which provides a separate live turn-settled lifecycle. This PR fixes render-only final-message projection for completed and historical transcripts.
中文说明
本 PR 做了什么
当 Web Shell 判定某个 Turn 的最终 Assistant 消息时,本 PR 会把带有匹配
toolUseId的后台 Agent 终态通知视为对应启动行已经结束。即使 transcript 投影仍把原启动工具保留为pending,该陈旧状态也不会继续阻塞最终操作和renderAssistantTurnFooter。改动只位于 Web Shell 的最终回答投影逻辑,并新增了
ask_user_question等待、两个后台 Agent 延迟完成通知、单一稳定 Footer、历史只读回放等回归覆盖。为什么需要
PR #8787 正确增加了后台 Agent 门禁,避免工作仍在执行或等待汇总时给中间叙述挂载复制、分叉、时间和自定义 Footer。但在部分已完成 transcript 中,后台 Agent 终态通知和最终补充回答都已落盘,原启动行却仍保持
pending。latestTurnAwaitsAgentSummary此时已经为 false,但turnHasActiveBackgroundAgent仍将陈旧启动行判定为 active,导致collectFinalAssistantTurnIds不生成映射,最终 Assistant 的所有操作一起缺失。这不是某一个宿主 Footer 的局部问题:复制、分叉和所有依赖
renderAssistantTurnFooter的能力共用同一个最终消息身份。Reviewer 测试计划
如何验证
ask_user_question工具等待态,确认不展示最终操作/Footer;用户回答并完成后再展示。toolUseId的多个 Agent 终态通知,同时让启动行仍保持pending,再追加最终汇总;确认只有最后一条 Assistant 消息获得复制、分叉和自定义 Footer 身份。前后证据
修改前:确定性 DOM 回归复现了双 Agent 已完成 transcript;两个终态通知和最终补充回答都存在,但
data-assistant-actions仍为false,renderAssistantTurnFooter没有挂载。修改后:同一 transcript 只把
final-supplement标记为最终回答,Footer 收到 Turnask-user-answer,页面只渲染一个 Footer。本 PR 明确只改 WebShell,未做 IDE 集成,因此未附 IDE 浏览器截图。测试平台
环境
macOS,Node.js 22.12.0。
git diff --check均通过。风险与范围
toolUseId匹配启动callId;匿名通知不会覆盖 active 状态。ask_user_question和 streaming 行为不变。关联 Issue
与 #10398 相关;后者提供独立的实时 Turn settled 生命周期,本 PR 修复已完成和历史 transcript 的纯渲染最终消息投影。