feat(web-shell): expose assistant turn settlement - #10398
Conversation
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR! Template looks good ✓ Problem: a feature PR implementing the linked issue #10389 (maintainer-labeled Direction: aligned. Web Shell is a newly established opt-in embedding surface (#9984), and exposing the daemon's prompt terminals as a host lifecycle callback is squarely within that surface's lane — the daemon already emits these events, this just stops hosts from guessing. No daemon routes or event payloads change. No direct CHANGELOG reference in the comparison agent (nothing comparable exists there); the area is relevant on its own merits. Size: no core paths touched — changes span Approach: scope feels right. Terminal observation, duplicate suppression, and live-journal-repair deferral live in the session provider that already owns those orderings; Web Shell only projects the final visible message (reusing the existing Risk: no elevated revert-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：功能 PR，实现关联 issue #10389（维护者已标记 方向：对齐。Web Shell 是新确立的可选嵌入面（#9984），把 daemon 的 prompt 终态暴露为宿主生命周期回调完全在该表面的职责范围内——daemon 本来就会发出这些事件，此举只是让宿主不必靠猜。不修改 daemon 路由或事件载荷。对比产品的 CHANGELOG 无直接参考（其无可比物），但该领域本身成立。 规模：未触及核心路径——改动横跨 方案：范围合理。终态观察、去重和 live-journal-repair 延迟发布放在本就拥有这些时序的 session provider 里；Web Shell 只负责投影最终可见消息（复用现有 风险：未命中高回滚风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewNo blockers found. I went in expecting the usual concurrency traps in a terminal-delivery feature and checked them one by one: The ordering invariant holds. Terminals trigger the existing terminal-guarded History suppression and reconnect catch-up ride on machinery that already existed rather than adding a parallel mechanism: the replay loop now captures On the Web Shell side, One non-blocking follow-up for later: the cross-session attribution guard (design TC-09) is tested at the App level with a mocked provider hook; a provider-level test for a terminal arriving across a session switch would close that edge from the other side too. Not merge-blocking. sequenceDiagram
participant P1 as Daemon SSE stream
participant P2 as DaemonSessionProvider
participant P3 as Transcript store
participant P4 as Web Shell App
participant P5 as Embedding host
P1->>P2: turn_complete or turn_error (promptId)
P2->>P3: flush buffered deltas, commit terminal projection
alt live-journal repair pending for this prompt
P2->>P2: park settlement on the repair episode
P3->>P2: repair committed (or failed)
end
P2->>P4: settlement (sessionId, promptId, outcome) once per key
P4->>P4: project final assistant message if same session
P4->>P5: onAssistantTurnSettled (event with message)
Files changed (12 of 12 shown)
Test evidence (this PR's own CI, via API)The one red check is not this PR's doing — and I want to be specific about why, because it also means something about coverage. Everything else on the commit landed green or is still running: visuals capture passed, Desktop Shell (Tauri compile + release-config tests, both OSes) passed, Dependency CVE audit and Secret scan passed.
Sandboxed verification would settle the remaining gap: This is an unattended CI run, so no live real-scenario drive was performed here; the comment triggers above are the sanctioned path for that signal. 中文说明代码审查未发现阻塞问题。这类终态投递功能常见的并发陷阱我逐一核对过： 时序不变式成立。终态事件触发既有的"仅终态" 历史抑制与重连补收复用了既有机制而非另起炉灶：回放循环现在捕获 Web Shell 侧， 一个非阻塞的后续建议：跨会话归属守卫（设计文档 TC-09）目前只在 App 层（mock provider hook）有测试；在 provider 层补一个"跨会话切换到达的终态"测试可以从另一侧封闭该边界。不影响合并。 测试证据（本 PR 自己的 CI，经 API 获取）唯一的红色检查不是本 PR 造成的——我想说清楚原因，因为它同时影响覆盖结论。 该 commit 上其余检查或为绿色或在运行中：视觉捕获通过，Desktop Shell（Tauri 编译 + 发布配置测试，双平台）通过，依赖 CVE 审计与密钥扫描通过。 沙箱验证可以补齐剩余缺口： 这是无人值守 CI 运行，此处不做真实场景驱动；上述评论触发器是该信号的正当通道。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — a clean, tightly scoped review; the only thing standing between this PR and approval is the CI state on this commit, not the code. Stepping back: I wrote down my own design before reading the diff — hook the daemon terminals in the session provider, defer until the transcript projection commits, dedup on The reason I'm not approving in this run is procedural, and I want it on the record explicitly. The verdict on the code is approve — but the unit suite that would attest to it never got to run on this commit: @dreamWB — the path forward is short: re-run the failed 中文说明信心：4/5 —— 一次干净、范围收敛的审查；挡在本 PR 与批准之间的只有该 commit 的 CI 状态，而不是代码本身。 退一步看：我在读 diff 之前先写下了自己的设计——在 session provider 里挂接 daemon 终态、延迟到 transcript 投影提交后、以 本轮不批准的原因是程序性的，我要明确记录在案。对代码的结论是批准——但能为它背书的单测套件在本 commit 上根本没跑起来： @dreamWB —— 后续路径很短：重跑失败的 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the web-shell and webui package suites beyond the two changed test files did not run: the package build fails on a pre-existing tsc error measured at the merge base (base-tree), so the harness never reached its test phase; the two changed test files pass locally (248/248, 560/560).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — the web-shell and webui package suites beyond the two changed test files did not run: the package build fails on a pre-existing tsc error measured at the merge base (base-tree), so the harness never reached its test phase; the two changed test files pass locally (248/248, 560/560)。
— qwen3.8-max via Qwen Code /review (v0.22.2)
aaa96d6 to
5bcf741
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）为单个提交。 |
|
Review fixes are available in 5bcf741. Addressed:
Validation:
The disposable root preflight passed clean, format, lint, build, and typecheck. Root test:ci then hit unrelated packages/cli timing failures: 13 AuthDialog interaction timeouts, 1 revert-hunk timeout, and 5 Vitest worker RPC timeouts. No CLI files are changed by this PR. All six review discussions have individual replies and are resolved. CI for the new head is running. |
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. |
| if (settlement && !settlementDelayedForRepair) { | ||
| publishPromptSettlement(settlement); | ||
| } |
There was a problem hiding this comment.
[Critical] R2-1: [certifies-falsely] [new-surface] The live terminal publish branch emits transcriptComplete: true (the promptSettledFromTurnEvent default) even when the current session epoch was rebuilt from a degraded replay — the live-path twin of the R1-8 fix. A mid-turn reconnect or refresh reload can return a degraded snapshot (any compaction error sets compactionDegraded, snapshotReplay() returns degraded: true, /load maps it to replayDegraded: true), and findLiveJournalRepairTarget returns undefined for degraded replays, so no repair episode exists to defer into. When the turn later completes, its live turn_complete reaches this branch and publishes transcriptComplete: true although the committed transcript is missing the pre-disconnect chunks — a host persisting under (sessionId, promptId) treats a provably partial answer as the complete final answer. The catch-up branch derives completeness from replay integrity (~2321-2323); the live branch skips the same check for the same degraded state. The only difference from the TC-13 degraded case (which correctly publishes false) is whether the terminal sits inside the replay snapshot or arrives one event later on the live stream.
Witness (probe on the unmodified PR, scratch tree at the reviewed commit):
PR: PROBE-F1 settlement {"promptId":"prompt-live","outcome":"completed","eventId":10,"transcriptComplete":true}
FIX: same probe receives "transcriptComplete":false (all 255 tests pass with the fix)
Capture at load time whether the prompt being settled was active at a degraded /load — before settleRestoredActivePrompt() consumes it — and gate the live publish on it, e.g. transcriptComplete: settlement.transcriptComplete && !(wasRestoredAtDegradedLoad && activeSession.replayDegraded). Note the constraint: restoredActivePrompt is consumed at ~2806/~2818, before this publish site (~2979), so the state must be captured earlier; and do not apply a blanket !activeSession.replayDegraded — a turn submitted live AFTER a degraded load is fully observed live and must keep transcriptComplete: true.
Fix witness: please add a provider test modelled on 'repairs and publishes a live terminal after a reconnect restores an active prompt' with replayDegraded: true, no marker and no terminal in the replay, then a live turn_complete, asserting exactly one settlement with transcriptComplete: false — and confirm the mutant (removing the guard) turns it red.
中文说明
live 终态发布分支在当前会话 epoch 由降级重放重建时仍发布默认的 transcriptComplete: true——是 R1-8 修复在 live 路径上的孪生缺口。中途重连/刷新重载可能返回降级快照（任何 compaction 错误都会置位 compactionDegraded，/load 映射为 replayDegraded: true），而降级重放下不存在修复 episode。turn 随后经 live turn_complete 完成时，该分支会对缺失断连前内容的 transcript 发布 transcriptComplete: true——按 (sessionId, promptId) 持久化的宿主会把确知不完整的答案当作完整最终答案。catch-up 分支（~2321-2323）已按重放完整性推导，live 分支却跳过了同样的检查。证据：未修改 PR 上探针得到 transcriptComplete: true，应用修复后翻转为 false（255 条测试全绿）。修复请在加载时（settleRestoredActivePrompt() 消费之前）捕获"该 prompt 是否在降级 /load 时处于活动"，并据此门控 live 发布；不要一刀切 !replayDegraded（降级加载之后才提交的 turn 全程 live 可见，必须保持 true）。修复见证：新增降级重放 + live 终态的 provider 测试并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const promptSettlementListenersRef = useRef<Set<DaemonPromptSettledListener>>( | ||
| new Set(), | ||
| ); | ||
| const publishedPromptSettlementsRef = useRef<Set<string>>(new Set()); |
There was a problem hiding this comment.
[Critical] R2-2: [new-surface] publishedPromptSettlementsRef grows without bound: every published settlement adds getPromptSettledKey(sessionId, promptId) to this Set, and nothing in the file ever removes entries — the only other references are the has/add in publishPromptSettlement (~1019-1021). The provider instance outlives session switches (it is mounted with a static key "main-session" in WorkspaceSessionProvider.tsx:229, so React keeps the same instance while props change), so in a long-lived embedded Web Shell tab a user running prompts across many sessions/workspaces accumulates one JSON key per settled prompt forever — monotonic memory growth until the tab closes. Contrast the sibling settledPromptsRef, whose entries ARE deleted on consumption (actions.ts:2279), and the file's own retention comment (~2185) documenting a past unbounded-retention incident of exactly this shape. Growth is one short string per settled prompt, so this is a slow leak, not a burst — but the embeddable is designed to live in host pages for hours.
Witness (probe, one mounted provider, sessions A→B→A via ring-eviction reloads):
PR: settlements [{"sessionId":"session-f2-a","promptId":"prompt-a"},{"sessionId":"session-f2-b","promptId":"prompt-b"}] loadCalls=3
— the re-delivered live prompt-a terminal after returning to session A was suppressed by the entry added at first settlement (entries persist across switches)
MUTANT (dedup retention removed): 3 settlements — prompt-a re-published (probe flips)
Suggested fix: bound the record — drop keys that do not belong to the session epoch currently being served (keeping the current session's keys so SSE catch-up redelivery still dedups), or cap the Set with oldest-entry eviction. Constraint: keys are JSON.stringify([sessionId, promptId]) (getPromptSettledKey, actions.ts:2334), and an epoch reset must not drop the current session's keys before its terminal commit, or a redelivered terminal after reconnect republishes.
Fix witness: 'publishes authoritative prompt settlements once after terminal transcript commit' yields the same turn_complete twice and asserts exactly one delivery — any pruning fix must keep that green for duplicate terminals within the same session; please confirm the mutant (removing the dedup) turns it red.
中文说明
publishedPromptSettlementsRef 无界增长：每次发布结算都会向该 Set 加入 getPromptSettledKey(sessionId, promptId)，而整个文件没有任何删除点（唯一的其他引用是 publishPromptSettlement 里的 has/add）。Provider 实例在会话切换间存活（WorkspaceSessionProvider.tsx:229 以静态 key "main-session" 挂载），因此长驻嵌入式 Web Shell 页签中，用户跨多个会话/工作区的每个已结算 prompt 都会永久累积一条 JSON 键——单调内存增长直到页签关闭。对照：姊妹结构 settledPromptsRef 在消费时删除条目（actions.ts:2279），且本文件 ~2185 行的保留期注释正是对同形态无界保留事故的记录。证据（探针）：单 Provider 挂载、A→B→A 会话切换后，首次结算写入的键仍抑制了重投的 prompt-a 终态（键跨切换存活）；移除保留的突变体翻转为重复发布。修复建议：按当前会话 epoch 修剪（保留当前会话键以维持 SSE 补收去重），或设上限按最旧条目驱逐；约束：epoch 重置不得在终态提交前丢弃当前会话键，否则重连重投会重复发布。修复见证：双终态精确一次测试必须保持绿色，并请做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| settleRestoredActivePrompt(); | ||
| } | ||
| if (!activePromptSettled) continue; |
There was a problem hiding this comment.
[Critical] R2-3: [fails-closed] [new-surface] Catch-up settlement publication is gated on activePromptSettled, which only becomes true for locally submitted prompts (entries in activePromptsRef with a bound promptId) — restored-active and observer terminals delivered via catch-up replay never publish, asymmetric with the live path, which publishes them. Scenario: the user refreshes the page mid-turn (the prompt is restored via hasActivePrompt, with no entry in activePromptsRef); the transport drops; the turn completes while disconnected; the reconnect /load returns hasActivePrompt: false with the turn_complete inside the replay. In this loop settleActivePromptFromTurnEvent returns false (no activePrompts entry), so continue skips publication — onAssistantTurnSettled never fires for a turn the host watched streaming, and the host never persists the result. The same gate also means repair.terminalSeen is never set for a marker targeting that prompt, so TC-14's catch-up repair half never starts; and the added if (activePromptSettled && restoredActivePrompt) settleRestoredActivePrompt() above is unreachable for restored-only prompts, since activePromptSettled is false there.
Witness (probe on the unmodified PR, scratch tree at the reviewed commit):
PR: PROBE-F3 settlements: [] — a turn the host watched streaming never settles
MUTANT (gate removed): PROBE-F3 settlements: [{"promptId":"prompt-1","outcome":"completed","transcriptComplete":true}]
Suggested fix: mirror the live path's restored branch here — when a replay terminal closes a previously-active (restored) turn, consume the restored flag and build/defer the settlement instead of skipping, keeping ordinary first-load history excluded per the design doc ("Ordinary session load, branch/split transcript replay, and older-history pagination never publish settlements"). Note the probe mutant is deliberately over-broad; the correct fix must scope to restored/watched prompts and keep ordinary history replay silent.
Fix witness: please add a provider test — restored prompt via hasActivePrompt: true, reconnect replay carrying the terminal with hasActivePrompt: false; assert exactly one settlement — and confirm removing the new branch turns it red.
中文说明
catch-up 结算发布受 activePromptSettled 门控，而该值只对本地提交的 prompt（activePromptsRef 中有绑定 promptId 的条目）为真——经 catch-up 重放投递的 restored-active / observer 终态从不发布，与 live 路径（无条件发布）不对称。场景：turn 进行中刷新页面（prompt 经 hasActivePrompt 恢复，但无 activePromptsRef 条目）；传输断开；turn 在断连期间完成；重连 /load 返回 hasActivePrompt: false 且 turn_complete 在重放中——此循环中 settleActivePromptFromTurnEvent 返回 false，continue 跳过发布：宿主看着流式输出的 turn 永远收不到 onAssistantTurnSettled，结果永不持久化。同一门禁还导致针对该 prompt 的 repair.terminalSeen 永不置位（TC-14 的 catch-up 修复半边无法启动）；上方新增的 if (activePromptSettled && restoredActivePrompt) 对仅 restored 的 prompt 不可达。证据：未修改 PR 探针 settlements: []；移除门禁的突变体发布一条结算。修复建议：在 catch-up 循环中镜像 live 路径的 restored 分支（消费 restored 标志并构建/延迟结算），同时保持普通首次加载历史不发布（设计文档排除项）。修复见证：新增 restored prompt + 重放终态测试，断言恰好一条结算，并做变异验证。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (existingRepair && !reuseExistingRepair) { | ||
| publishPendingRepairSettlement(existingRepair, false); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-7: The repair-episode replacement release branch has no test. TC-12 promises that "clearing or replacing a repair episode releases its held settlement once with transcriptComplete: false", but only the clearing half is pinned (publishes an incomplete settlement before clearing an in-flight repair); no test holds a pendingSettlement on an episode that is then replaced by a differently-signed repair target. If this call were removed, a held settlement would be silently discarded on episode replacement and the host's onAssistantTurnSettled would never fire for that turn — shipping green.
Witness (mutation run on the provider suite):
MUTANT: removed publishPendingRepairSettlement(existingRepair, false) from the replacement branch
→ Test Files 1 passed (1) / Tests 253 passed (253) — suite stays green with the release removed
Suggested fix: add a DaemonSessionProvider.test.tsx case — gate a repair load while a terminal is held (as in the existing clear test), then resolve the reload with a replay snapshot whose history_truncated marker produces a different repair-target signature; assert exactly one settlement with transcriptComplete: false and no second settlement afterwards. Constraint: release must stay exactly-once per (sessionId, promptId) — publishPromptSettlement dedupes at ~1019-1021 and publishPendingRepairSettlement clears pendingSettlement before publishing (~1043), so the new test must expect exactly one settlement.
Fix witness: the new test itself must go red if the replacement-branch call is removed; the existing clear-path test would not catch this mutation.
中文说明
修复 episode 的"替换"释放分支没有测试。TC-12 承诺"清除或替换修复 episode 时以 transcriptComplete: false 一次性释放其持有的结算"，但只有清除半边被钉住（in-flight clear 测试）；没有测试在持有 pendingSettlement 的 episode 被不同签名的修复目标替换时验证释放。若移除该调用，被持有的结算会在替换时静默丢弃，宿主的 onAssistantTurnSettled 对该 turn 永不触发——且会绿着上线。证据（突变体）：移除替换分支调用后整套 provider 测试仍 253/253 全绿。建议新增测试：挂起修复 reload 并持有终态，随后以产生不同修复目标签名的重放快照完成 reload，断言恰好一条 transcriptComplete: false 结算。约束：释放必须保持每 (sessionId, promptId) 精确一次（~1019-1021 去重；~1043 先清 pendingSettlement 后发布）。修复见证：新测试在移除替换分支调用后必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| block.parentToolCallId !== undefined || | ||
| block.text.trim().length === 0 | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R2-8: The empty/whitespace-text skip in getSettledAssistantMessage has no test witness — none of the four new settlement fixtures contains an empty-text assistant block. If this predicate were removed, all existing tests would stay green, and the regression that would then ship: for a turn whose LAST prompt-matching top-level assistant block has whitespace-only text (e.g. a trailing empty assistant chunk after substantive earlier content), the callback would project message.content as that empty string instead of falling back to the earlier substantive message — a host persisting event.message?.content stores an empty turn result. Test 4 covers the adjacent-but-different "no assistant block at all" case.
Witness (mutation run):
MUTANT: removed block.text.trim().length === 0 from getSettledAssistantMessage
→ settlement tests: 4 passed | 558 skipped (562); full App.test.tsx: 562 passed (562) — predicate unguarded
Suggested fix: extend the App.test settlement fixtures — give the prompt a final assistant block with whitespace-only text plus an earlier non-empty assistant block, and assert the callback's message.id/content point at the earlier block.
Fix witness: the new assertion must go red if the block.text.trim().length === 0 predicate is removed (it would then select the empty final block).
中文说明
getSettledAssistantMessage 的空白文本跳过分支没有测试见证——四个新结算 fixture 均不含空文本 assistant block。若移除该判断，现有测试依然全绿；会随之上线的回归：当某 turn 匹配 promptId 的最后一个顶层 assistant block 仅含空白（例如实内容之后追加的空 assistant 块）时，回调会把 message.content 投影为该空字符串，而不是回退到更早的实内容消息——持久化 event.message?.content 的宿主会存入空结果。测试 4 覆盖的是相邻但不同的"完全没有 assistant block"场景。证据（突变体）：移除该判断后结算测试 4 通过、全文件 562/562 通过。建议扩展 fixture：最后一个 assistant block 仅空白 + 更早的非空 block，断言 message 指向更早的 block。修复见证：移除该判断后新断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| ] as DaemonEvent[], | ||
| }, | ||
| ])( | ||
| 'marks catch-up settlement incomplete when the replay is $label', |
There was a problem hiding this comment.
[Suggestion] R2-12: No test pins the catch-up settlement published for a replayed turn_error terminal (outcome 'failed'). This it.each replaced the old turn_error ring-eviction test with turn_complete-only variants, and the re-added turn_error test (~12141) never subscribes to useDaemonPromptSettled; the only outcome: 'failed' assertion in the suite (~15902) covers a LIVE SSE terminal. The catch-up loop does publish failed settlements today (settleActivePromptFromTurnEvent returns true on turn_error, and there is no terminal-type filter before publication) — but a future refactor that skips non-turn_complete terminals in that loop ships green, and a turn that errors while the client is mid-reconnect/replay would silently lose its failure record under this PR's persistence semantics.
Witness (mutation run):
MUTANT: inserted `if (replayEvent.type !== 'turn_complete') continue;` after the settle call in the catch-up loop
→ Test Files 1 passed (1) / Tests 253 passed (253) — the mutant is silent
Suggested fix: in the re-added 'rejects active prompts from replay turn_error after ring eviction' test, collect settlements via useDaemonPromptSettled((event) => settlements.push(event)) and assert the failed settlement shape — outcome: 'failed', the derived transcriptComplete, and error: { message: 'model overloaded', code: 'overloaded' }.
Fix witness: gating or skipping the catch-up publication for turn_error in the loop (~2304/~2319 of DaemonSessionProvider.tsx) must turn the new settlements assertion red while today's suite stays green.
中文说明
catch-up 重放投递的 turn_error 终态（outcome 'failed'）发布的结算没有任何测试钉住：该 it.each 用仅含 turn_complete 的变体替换了原 turn_error 环形驱逐测试，而重新加回的 turn_error 测试（~12141）从未订阅 useDaemonPromptSettled；套件中唯一的 outcome: 'failed' 断言（~15902）覆盖的是 LIVE 终态。当前实现确实会发布 failed 结算（settleActivePromptFromTurnEvent 对 turn_error 返回 true，发布前无终态类型过滤）——但未来在 catch-up 循环中跳过非 turn_complete 终态的重构可以绿着上线：客户端重连/重放期间出错的 turn 会在本 PR 的持久化语义下静默丢失失败记录。证据（突变体）：在 settle 调用后插入 if (replayEvent.type !== 'turn_complete') continue;，253/253 全绿，突变体静默。建议：在重加回的 turn_error 测试中订阅结算并断言 failed 形态（outcome: 'failed'、推导出的 transcriptComplete、error 载荷）。修复见证：在循环中门控/跳过 turn_error 发布后新断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
|
||
| - `sessionId` and daemon-assigned `promptId`; their tuple is the stable idempotency key; | ||
| - `outcome`: `completed`, `cancelled`, or `failed`; | ||
| - the daemon `stopReason` when a turn completed; |
There was a problem hiding this comment.
[Suggestion] R2-16: The Contract enumeration in this design doc omits two fields the implementation delivers — error?: { message: string; code?: string } (every failed turn) and eventId?: number (whenever the terminal event has an id) — and describes stopReason as present only "when a turn completed", although cancelled turns carry it too (the type's own JSDoc at customization.tsx:229 says "Present for completed and cancelled turns"). The README section added by this PR also never mentions error. A host integrating against a section literally titled "Contract" that implements failure handling for outcome: 'failed' turns would miss that for a failed turn with no assistant content, error.message is the ONLY failure diagnostic the settlement carries — the daemon's failure reason ('model unavailable', 'model_error', …) gets silently dropped instead of persisted or surfaced. The PR's own tests pin these fields being delivered, so code and contract disagree from day one.
Witness (run of the PR's own test at the reviewed commit):
App.test.tsx 'omits the message when the settled prompt has no assistant content' — Tests 1 passed | 561 skipped
asserts the public callback payload includes eventId: 12 and error: { message: 'model unavailable', code: 'model_error' }
— the Contract section enumerates only sessionId/promptId, outcome, stopReason, transcriptComplete, and the final assistant message
Suggested fix:
- `stopReason` for completed and cancelled turns;
- `error` (`{ message, code? }`) for failed turns — the daemon's failure reason; for turns without assistant content this is the only failure diagnostic;
- `eventId`, the terminal SSE event cursor (not part of the idempotency key);and mention error once in the README section's example prose. The documented shapes must match DaemonPromptSettledEvent at packages/webui/src/daemon/session/types.ts:204-218 and WebShellAssistantTurnSettledEvent at packages/web-shell/client/customization.tsx:224-241, both declaring eventId?: number and error?: { message: string; code?: string }.
中文说明
设计文档的 Contract 枚举遗漏了实现实际交付的两个字段——error?: { message: string; code?: string }（每个失败 turn）与 eventId?: number（终态事件带 id 时）——且把 stopReason 描述为仅"当 turn 完成时"存在，而取消的 turn 同样携带它（类型 JSDoc customization.tsx:229 写明"Present for completed and cancelled turns"）。本 PR 新增的 README 章节也从未提及 error。对着标题即为 "Contract" 的章节实现失败处理的主机会错过：对没有 assistant 内容的失败 turn，error.message 是结算携带的唯一失败诊断——daemon 的失败原因会被静默丢弃而不是被持久化/呈现。本 PR 自己的测试已钉住这些字段的交付，代码与契约从第一天起就不一致。证据：在评审提交上运行 App.test.tsx 的相应用例（1 通过），其断言回调载荷包含 eventId: 12 与 error: { message: 'model unavailable', code: 'model_error' }。建议：补充 error/eventId 契约条目，把 stopReason 措辞改为"完成与取消的 turn 均携带"，并在 README 示例说明中提及 error；文档形状须与 types.ts:204-218、customization.tsx:224-241 一致。
— qwen3.8-max via Qwen Code /review (v0.22.3)
2 participants
What this PR does
This PR adds an optional Web Shell callback for the semantic settlement of an assistant turn. The callback is driven by authoritative daemon prompt terminals, runs only after the terminal transcript projection is committed, reports completed, cancelled, and failed outcomes, and includes the final visible assistant message when that message is still available in the committed current-session transcript.
The contract uses the daemon-assigned
(sessionId, promptId)tuple as its durable idempotency key, suppresses duplicate terminal delivery for the lifetime of a mounted provider, distinguishes incomplete live-journal recovery withtranscriptComplete, and keeps artifact and workspace projection as independent lifecycles. It does not change daemon routes or event payloads.Why it's needed
Prompt status becoming idle is a UI state transition, not proof that a specific assistant turn reached an authoritative terminal. It can also occur around waits, cancellation requests, reconnects, and history restoration. Hosts that need to consume a final answer should not have to infer completion from busy-to-idle transitions or reverse-scan a mutable transcript.
Reviewer Test Plan
How to verify
cancelled; confirm a terminal error reportsfailedand may include partial assistant content.(sessionId, promptId)to verify host-side durable idempotency.Evidence (Before & After)
N/A — this is a non-visual host lifecycle API.
Tested on
Environment (optional)
macOS with Node.js 24.19.0. Web Shell passed 4,421 tests, WebUI's daemon session provider passed 248 tests, the Web Shell application test file passed 560 tests, and both affected packages passed lint, typecheck, and build. Repository preflight completed clean, install, format, lint, build, and typecheck; its parallel CLI test phase reported four unrelated server/scheduled-task failures with worker RPC timeouts and socket resets, and all four passed in an isolated rerun (7 matching assertions passed).
Risk & Scope
Linked Issues
Fixes #10389
中文说明
本 PR 做了什么
本 PR 为 Web Shell 新增一个可选回调，用于表达 Assistant Turn 的语义终态。回调由 daemon 权威 prompt terminal 驱动，只在终态 transcript projection 已提交后执行，区分 completed、cancelled 和 failed，并在最终可见 Assistant Message 仍存在于当前会话已提交 transcript 中时携带该消息。
契约使用 daemon 分配的
(sessionId, promptId)二元组作为持久幂等键，在单次 Provider 挂载期间抑制重复 terminal，通过transcriptComplete区分 live journal 恢复不完整的情况，并保持 Artifact 与 workspace projection 为独立生命周期。本 PR 不修改 daemon 路由或事件载荷。为什么需要
Prompt 状态变为 idle 只是 UI 状态变化，不能证明某个 Assistant Turn 已到达权威终态；等待、取消请求、重连和历史恢复过程中也可能出现 idle。需要消费最终回答的宿主不应依赖 busy 到 idle 的推断，也不应反向扫描仍可能变化的 transcript。
Reviewer 测试计划
如何验证
cancelled；确认 terminal error 会报告failed，且可以携带部分 Assistant 内容。(sessionId, promptId)验证宿主持久幂等。证据（修改前与修改后）
不适用——这是非可视化的宿主生命周期 API。
测试平台
环境（可选）
macOS，Node.js 24.19.0。Web Shell 4,421 条测试通过，WebUI daemon session provider 248 条测试通过，Web Shell application 测试文件 560 条测试通过，两个受影响包的 lint、typecheck 和 build 均通过。仓库 preflight 的 clean、安装、format、lint、build 和 typecheck 均完成；并行 CLI 测试阶段有 4 个无关的 server/scheduled-task case 因 worker RPC timeout 和 socket reset 失败，这 4 个 case 在隔离复跑中全部通过（共 7 条匹配断言通过）。
风险与范围
关联 Issue
Fixes #10389