feat(daemon): deliver web-shell mid-turn messages into the running turn - #5175
Conversation
Let the web-shell hand a message typed while a turn is running to that turn instead of holding it until the next turn. The daemon now answers the ACP child's `craft/drainMidTurnQueue` ext-method from a per-session queue the browser feeds; previously BridgeClient had no `extMethod`, so the child got -32601 and latched the drain off for the session. Server: `SessionEntry` gains a mid-turn queue; `bridge.enqueueMidTurnMessage` accepts only while a turn is active and the queue is emptied at the idle boundary; `BridgeClient.extMethod` drains it and publishes a `mid_turn_message_injected` SSE frame. A new `POST /session/:id/mid-turn-message` endpoint plus DaemonClient/DaemonSessionClient methods feed the queue. Browser: `enqueuePrompt` also pushes text-only messages to the daemon; a sidechannel hook drops the matching entries from the local queue when the injection frame arrives. A message is therefore delivered exactly once — mid-turn when a turn is live, or via the existing next-turn queue otherwise — and never both. Exactly-once rests on the injection frame arriving in order ahead of the turn-complete frame, plus the dedupe effect running before the next-turn drain. Out of scope: live "sent" rendering of an injected message (it shows on reload); ACP-transport (non-REST) ingestion parity.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
This PR implements mid-turn message delivery for the web-shell, bringing it to parity with the desktop app. The implementation is well-designed with careful attention to exactly-once delivery semantics across the browser/daemon split.
Strengths
- Solid
extMethodimplementation inBridgeClientwith defensive programming: sessionId validation, entry resolution, and try-catch for closed event bus - Proper accept-gate and idle-clear in
enqueueMidTurnMessage— rejects empty messages and idle-session messages to prevent double delivery, with idle-clear in the settle handler to prevent stale injection - Well-documented race condition handling — comments clearly explain the SSE frame ordering dependency and the defense-in-depth strategy
- Clean sidechannel implementation following the established
followupSidechannelpattern with proper message filtering and fresh-object publishing foruseSyncExternalStore - Comprehensive test coverage (464 bridge tests, 129 webui tests) covering the new functionality
- Backward compatibility — new event type and endpoint are additive; older clients gracefully degrade
Observations
The implementation correctly handles the key challenge: ensuring exactly-once delivery across the browser/daemon split. The accept-gate (rejecting idle messages) combined with the idle-clear (dropping undrained messages when the session goes idle) prevents the double-delivery scenario where both the daemon and browser would inject the same message.
The browser bundle budget bump from 118KB to 119KB is reasonable for the added surface area.
Verification
- CI: All checks passing (no failures)
- Presubmit: No downgrade required, no overlap with existing comments
Assessment
The implementation is solid and well-tested. The code is well-documented with clear explanations of the non-trivial delivery semantics. The implementation correctly handles the race conditions inherent in cross-process message delivery.
Verdict: COMMENT (downgraded from APPROVE due to pending CI)
The implementation looks ready to merge once CI passes.
— claude-opus-4-6 via Qwen Code /review
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
|
@qwen-code /triage |
Review follow-ups on the web-shell mid-turn drain.
[Critical] The injected-message sidechannel was single-slot (latest-wins), so two drain frames landing back-to-back — a multi-batch turn, or a backgrounded tab flushing buffered SSE — coalesced: the first batch's messages were never removed from the browser queue and got resent next turn = double delivery, the exact failure this feature prevents. The sidechannel now ACCUMULATES batches; the consumer reconciles every batch and then clears. The queue-dedup is extracted into a pure `removeInjectedFromQueue` helper and unit-tested (the App.tsx path had no test, so this regressed silently).
Hardening and tests:
- Cap mid-turn message length (server, 16 KB) and per-session queue depth (bridge, 20), matching the bounds on the sibling /btw and /prompt; over-cap returns `{accepted:false}` and the browser keeps the message for its next-turn queue.
- Drop the dead try/catch around `EventBus.publish` (never-throws contract — "don't wrap publish()"); check the return value and emit one diagnostic line per non-empty drain.
- Assert the settle-clear: a new test seam exposes the agent-side connection so a test can drive `extMethod('craft/drainMidTurnQueue')` after settle and assert the leftover was cleared (not re-drained next turn), plus the back-to-back FIFO survival case.
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for mid-turn user message injection in the web-shell flow: text-only messages submitted while a daemon session is actively running can be queued on the daemon, drained by the running ACP child between tool batches, and echoed back to the browser via an SSE event so the browser can dedupe its local “next-turn” queue.
Changes:
- Add a new daemon HTTP API (
POST /session/:id/mid-turn-message) and SDK/client helpers to enqueue mid-turn messages. - Implement the ACP client-side
extMethodhandler in the daemon bridge (craft/drainMidTurnQueue) and publish amid_turn_message_injectedSSE signal on successful drains. - Add webui/web-shell sidechannel + hook + queue reconciliation logic to ensure exactly-once delivery (mid-turn injection vs next-turn resend), plus unit tests.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/webui/src/daemon/useDaemonMidTurnInjected.ts | New hook to consume accumulated mid_turn_message_injected batches via useSyncExternalStore. |
| packages/webui/src/daemon/session/types.ts | Adds enqueueMidTurnMessage to the daemon session actions interface. |
| packages/webui/src/daemon/session/DaemonSessionProvider.tsx | Parses/publishes mid_turn_message_injected SSE frames into a sidechannel. |
| packages/webui/src/daemon/session/actions.ts | Adds best-effort enqueueMidTurnMessage action that resolves {accepted:false} on failure. |
| packages/webui/src/daemon/midTurnInjectedSidechannel.ts | New accumulating sidechannel store + parser for injected batches. |
| packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts | Tests for parsing and accumulating/clearing semantics. |
| packages/webui/src/daemon/index.ts | Re-exports useDaemonMidTurnInjected. |
| packages/webui/src/daemon-react-sdk.ts | Re-exports useDaemonMidTurnInjected from the React SDK entrypoint. |
| packages/web-shell/client/midTurnDedup.ts | New dedupe helper to remove injected messages from local queued prompts. |
| packages/web-shell/client/midTurnDedup.test.ts | Unit tests for queue reconciliation behavior (multi-batch, count-based, images). |
| packages/web-shell/client/App.tsx | Enqueues mid-turn messages best-effort and consumes injected batches to dedupe local queue. |
| packages/sdk-typescript/test/unit/daemonEvents.test.ts | Adds schema validation coverage for mid_turn_message_injected. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Adds contract tests for enqueueMidTurnMessage POST endpoint behavior. |
| packages/sdk-typescript/src/daemon/types.ts | Adds DaemonMidTurnMessageResult type for the new endpoint. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports new event/result types. |
| packages/sdk-typescript/src/daemon/events.ts | Registers new known event type + guard and reducer passthrough. |
| packages/sdk-typescript/src/daemon/DaemonSessionClient.ts | Adds session-scoped enqueueMidTurnMessage wrapper. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds REST transport implementation for POST /session/:id/mid-turn-message. |
| packages/sdk-typescript/scripts/build.js | Bumps browser bundle size budget for added daemon surface. |
| packages/cli/src/serve/server.ts | Adds the new REST endpoint for mid-turn message enqueueing. |
| packages/acp-bridge/src/internal/testUtils.ts | Keeps AgentSideConnection handle for driving ext-methods in tests. |
| packages/acp-bridge/src/bridgeTypes.ts | Extends bridge interface with enqueueMidTurnMessage. |
| packages/acp-bridge/src/bridgeClient.ts | Implements extMethod for craft/drainMidTurnQueue + SSE echo event. |
| packages/acp-bridge/src/bridgeClient.test.ts | Adds unit tests for drain behavior and methodNotFound handling. |
| packages/acp-bridge/src/bridge.ts | Adds per-session mid-turn queue, accept gating, depth cap, and settle-time clearing. |
| packages/acp-bridge/src/bridge.test.ts | End-to-end tests for accept gating, draining, and idle-boundary clearing semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewRead the full diff twice and cross-checked the concurrency-critical paths against the source on this branch ( Core correctness — exactly-once holds ✅This is the load-bearing claim, so I traced it specifically. The three assumptions all hold:
I worked through the "enqueue interleaves with settle-clear in the same tick" and "back-to-back FIFO across prompts" races — both Main finding — the new endpoint skips the per-session client-ownership check its siblings enforce
|
- POST /session/:id/mid-turn-message now length-checks and enqueues the TRIMMED message (it was checking the raw `message.length` while the bridge stores the trimmed value), so whitespace-padded input whose real content fits is no longer rejected. - Correct the `mid_turn_message_injected` docs (events.ts payload + bridgeClient.ts): it is a transient dedupe signal, not a transcript render — the message reaches the model mid-turn and the persisted transcript shows it on reload.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review Summary
整体设计扎实，exactly-once 不变量在服务端（accept-gate + settle-clear）和浏览器端（按 batch reconcile）双侧均显式表达，并被新增测试锁死。951e4a4c 已正确响应自评 4 条，f8e801d7 又消化了 Copilot 的 trim/length 顺序与 docstring 两条——目前没有阻断项。
仅留 3 条 hardening/讨论建议，均不阻断合入：
- 多客户端下 SSE 帧缺少
originatorClientId隔离（详见 inline）。Web-shell 主流是单 client，先合入 + follow-up issue 即可；最便宜的修法是把originatorClientId一并写进帧 data，与同文件prompt-suggestion等路径一致。 MAX_MID_TURN_QUEUE_DEPTH = 20硬编码（详见 inline），siblingmaxPendingPromptsPerSession是可配置项。当前默认值合理，仅建议在注释里点出后续可配化的口子。events.publish返回 falsy（teardown / closed bus）的降级分支无测试（详见 inline）。该路径会触发"child 已注入、浏览器仍重发"的双发降级，stderr 日志已经埋点，建议补一个 unit 用例锁定契约。
Strengths
MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'与 desktop / cli Session.ts 严格一致，避免 child latched-off。- accept-gate（
pendingPromptCount > 0） + settle-clear（pendingPromptCount === 0）的"真正 idle"语义被 back-to-back FIFO 测试锁死。 - 累积式 sidechannel +
useSyncExternalStore的 EMPTY 引用稳定 + clear 空操作短路，多 batch turn 已用回归测试覆盖。 - 16KB 单条 / 20 条深度上限，超限即 reject、浏览器走 next-turn 兜底，无丢消息风险。
enqueueMidTurnMessage在actions.ts内全部归约为{ accepted: false }，silent 不弹 user-facing notice，符合"优化路径而非用户动作"的语义。BridgeClient.extMethod对未知方法throw RequestError.methodNotFound(method)返回 -32601，让 child 的 drain caller 正确 latch off。
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Summary: 1 critical correctness issue (useEffect + consume() race can lose a batch between render and effect → double delivery) plus 3 suggestions (telemetry route missing, cross-package string duplication, no rejection logs). Test coverage gaps also noted (queue depth overflow, trimming, bus-closed path, error suppression wrapper — the HTTP endpoint test gap overlaps with the prior review's comment).
— qwen3.7-max via Qwen Code /review
…mid-turn dedupe Addresses the /review main finding and doudouOUC's inline comments on the web-shell mid-turn drain. - Authorize the mid-turn endpoint per session (review main finding): the route now forwards the client id via `parseClientIdHeader` and `enqueueMidTurnMessage` runs `resolveTrustedClientId` before queuing — mirrors `/prompt` and `/btw`, so a token-holding client bound to another session can no longer push into this turn (throws `InvalidClientIdError`). - Route the drain's SSE echo per originator (doudouOUC #3417739340): the trusted client id is recorded on each queue entry and the drain publishes one `mid_turn_message_injected` frame per originator carrying `originatorClientId`, so a peer on the same session can't dedupe a coincidentally-equal entry it never queued. - Wire the web-shell consumer to its own client id: the daemon now stamps every drained frame, and the web-shell always sends a client id, so `removeInjectedFromQueue` must filter on it. Plumbed `clientId` onto `DaemonConnectionState` (set from the bound session) and passed `connection.clientId` into the dedupe — without this the new filter would skip every batch, leaving our own messages to be resent next turn (the exact double-delivery this feature prevents). - Tests (doudouOUC #3417739347 + coverage for the above): per-originator publishing, the `published === false` (bus-closed) degradation, the endpoint ownership gate, end-to-end originator stamping, and the web-shell originator-filtering matrix (match / peer-skip / anonymous / mixed / missing-id regression guard). - Comment-only: point `MAX_MID_TURN_QUEUE_DEPTH` at `maxPendingPromptsPerSession` as the promotion model (doudouOUC #3417739352).
…servability
Addresses the qwen3.7-max /review pass on the web-shell mid-turn drain (3
criticals + 6 suggestions).
Criticals
- consume() race (sidechannel): the buffer is read during render but reconciled
in an async effect, so a frame appended in that window was wiped by an
unconditional clear → resent next turn (double delivery). `consume` now does a
compare-and-swap — it only clears if the buffer still holds the exact snapshot
it reconciled; a newly-arrived batch survives to the next reconcile.
- Late-arriving enqueue (web-shell): the fire-and-forget mid-turn POST is now
scoped to a per-turn AbortController, aborted when the turn settles, so a slow
push can't land during a SUBSEQUENT turn and be injected twice. An aborted
push resolves `{ accepted: false }`, so the message just follows its normal
next-turn path.
- HTTP route had zero tests: add a `POST /session/:id/mid-turn-message` suite
(accept, reject, missing/empty/oversized body, unknown session, malformed
client id) plus the fakeBridge wiring it needed.
Suggestions
- Telemetry: register `mid-turn-message` in the daemon route regex so the
endpoint gets a route label / spans / latency like its siblings.
- Single source of truth for `craft/drainMidTurnQueue`: export
`MID_TURN_QUEUE_DRAIN_METHOD` from acp-bridge and import it in both the
answerer (BridgeClient) and the caller (Session.ts), so a rename can't desync
them into a silent -32601 latch.
- Observability: `enqueueMidTurnMessage` now logs idle/empty/full rejects and
the drop-at-settle path (the drain already logged); rejects are low-volume
(the browser only pushes when it believes a turn is live).
- Buffer safety cap (sidechannel): bound the accumulating buffer and evict
oldest so an orphaned consumer can't grow it without limit.
- Tests: depth-cap overflow + trimming (bridge), compare-and-swap + cap
(sidechannel).
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
…iginator doc Addresses the follow-up /qreview pass. - Cross-session wipe (web-shell): the dedupe reconcile is session-scoped, but `consume()` cleared the whole accumulating buffer. The buffer is a cross-session singleton, so a late `mid_turn_message_injected` frame for the PREVIOUS session (e.g. after an in-place `/resume` switch) was wiped un-reconciled and lost on switch-back → resent next turn = double delivery. Replace the blanket clear with identity-removal: `consumeSidechannelMidTurnInjected(handled)` drops only the batches actually reconciled (the active session's). Batches for other sessions — and frames that arrived after the render snapshot (the render→effect race the prior compare-and-swap covered) — are not in `handled` and stay buffered for their own reconcile. So this subsumes the race fix and adds multi-session correctness. `consume` is now stable (no per-render churn). - originatorClientId doc (SDK): the field is declared on `DaemonMidTurnMessageInjectedData` (the `data` shape) but, unlike the sibling permission events, this event is not reduced and the daemon never merges the id into `data` — it rides the SSE envelope (`event.originatorClientId`) and is lifted into `data` only by the web-shell's own parser. Document that so an SDK consumer doesn't read an always-undefined `data.originatorClientId` and treat every batch as anonymous (dedupe-for-all foot-gun). - Tests: identity-removal across the race, the cross-session leave-behind, and the already-evicted no-op.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Summary: All Critical findings from the previous round have been properly addressed — compare-and-swap clear, AbortController scoping, originator routing, telemetry route, shared drain constant, rejection logging, and extensive test coverage. The incremental changes are well-structured. Three remaining Suggestions focus on test gaps and browser-side observability.
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
…rn-drain # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/acp-bridge/src/bridgeTypes.ts
…, tests Addresses the latest /review pass (DeepSeek + qwen3.7-max). Stale duplicates of already-shipped fixes (shared drain-method constant, depth-cap test, consume cross-session/race) are answered inline; the substantive new items: - DaemonClient.enqueueMidTurnMessage now routes through `fetchWithTimeout` like every other method, so a hung daemon can't wedge the void-ed caller in actions.ts forever. The helper composes the caller's signal with its timeout, so the turn-settle abort still propagates. (+ propagation and timeout tests.) - Browser-side observability (mirrors the server-side writeStderrLine added earlier): the actions catch logs non-abort failures at debug (an abort is the designed settle cancel, kept silent); the settle-abort and sidechannel buffer eviction each get a `console.debug`; and a debug warns when stamped batches arrive but `connection.clientId` is undefined (dedupe would skip them). - Docs: spell out the `originatorClientId` CONTRACT — a consumer that dedupes MUST compare it against its own client id (the daemon broadcasts, it does not route), or it drops another client's coincidentally-equal message. - Tests: `POST /session/:id/mid-turn-message` InvalidClientIdError → 400; the SSE event-pump routing of `mid_turn_message_injected` to the sidechannel (not the transcript); DaemonClient signal propagation + hung-daemon timeout.
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Follow-up Review — all 3 comments resolved
#1 originatorClientId isolation (bridgeClient.ts:577)
Resolved. The fix exceeds the suggestion:
MidTurnQueueEntrycarriesoriginatorClientId(fromresolveTrustedClientId).BridgeClient.extMethodgroups drained messages per originator and publishes one frame per group with the id on the envelope.- Client-side
removeInjectedFromQueuenow acceptsclientIdand skips batches from other clients. DaemonMidTurnMessageInjectedDatahas an explicit CONTRACT JSDoc warning consumers MUST compare the id.enqueueMidTurnMessagevalidates clientId against the session (throwsInvalidClientIdError), same as/promptand/btw.
#2 published === false branch untested (bridgeClient.ts:581)
Resolved. New test: "still returns the drained messages to the child when the echo frame is dropped (bus closed)" — asserts (a) messages returned, (b) queue emptied, (c) echo frame dropped logged to stderr.
The writeStderrLine vs daemonLog point: accepted the author's rationale that acp-bridge has no daemonLog/debugLogger (it uses writeStderrLine — 16 existing uses). Keeping it consistent within the package is the right call; future logger migration would be package-wide.
#3 MAX_MID_TURN_QUEUE_DEPTH hardcoded (bridge.ts:671)
Resolved. Comment now reads:
Intentionally a fixed const for now; if this ever needs tuning, promote it to a
BridgeOptionsknob the same waymaxPendingPromptsPerSession(the analogous bound/promptenforces, default 5) is wired.
Plus a boundary test (21st message rejected).
Other improvements since last review
The author also addressed the qwen3.7-max round-3/4 findings:
| Fix | Commit |
|---|---|
| Cross-turn double delivery (per-turn AbortController, abort on settle) | f794006 + ea0a87f |
| useEffect race / blanket consume → identity-removal consume (session-scoped) | 6b0a892 |
Server route tests (POST /session/:id/mid-turn-message, 7 cases) |
f794006 |
DaemonClient.enqueueMidTurnMessage routes through fetchWithTimeout |
ea0a87f |
DaemonSessionProvider integration test (parse → publish → continue) |
ea0a87f |
Sidechannel buffer eviction cap (MAX_PENDING_BATCHES = 64) |
6b0a892 |
Client-side observability (console.debug on abort/eviction/clientId-skip) |
ea0a87f |
MID_TURN_QUEUE_DRAIN_METHOD shared constant exported from bridgeTypes |
f794006 |
Remaining known gap
App-level AbortController lifecycle (create ref → share → abort on settle) has no test — author acknowledges this explicitly and proposes a follow-up RTL test. The signal contract is covered at the SDK layer. Acceptable as-is.
Verdict: Ship it. No blocking issues remain. The exactly-once contract is well-defended at every layer (accept-gate, settle-clear, per-originator SSE, identity-removal consume, per-turn abort), and each invariant has at least one test that fails when the relevant line is deleted.
|
Thanks for the PR! This is a well-scoped feature that brings web-shell to parity with the desktop app for mid-turn message injection. Template looks good ✓ Direction: Solid. The desktop app already injects mid-turn messages; the web-shell lagging behind is a genuine parity gap. Claude Code's CHANGELOG has multiple entries about mid-turn messages being lost or dropped ( Approach: The PR wires the drain end-to-end across five packages (acp-bridge, cli, sdk-typescript, webui, web-shell), which matches the scope of the feature. The exactly-once delivery design is thorough — accept-gate (busy only), idle-clear at settle, per-originator SSE echo frames, browser-side dedupe by text+session, abort controller for turn-settle race. Each race condition has a corresponding test. The 16KB per-message cap and queue depth limit (20) are reasonable DoS guards. One minor note: there's a small unrelated formatting change in Nothing in the diff is unnecessary for the stated goal. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向： 合理。桌面应用已经支持 mid-turn 消息注入，web-shell 在这方面落后是真实的对齐缺口。Claude Code 的 CHANGELOG 多次出现 mid-turn 消息丢失的修复（ 方案： PR 在五个包（acp-bridge、cli、sdk-typescript、webui、web-shell）之间端到端地接通了 drain，与功能范围匹配。exactly-once 交付设计很周全——接受门控（仅 busy 时）、idle 时清空、按 originator 分组的 SSE 回显帧、浏览器侧按文本+session 去重、turn-settle 竞态的 abort controller。每个竞态条件都有对应测试。16KB 单条消息上限和队列深度限制（20）是合理的 DoS 防护。 一个小问题： diff 中没有超出目标所需的改动。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
All 17 inline comments resolved — replies posted above.
The exactly-once delivery contract is now defended at every layer:
- Server: accept-gate (
pendingPromptCount > 0) + settle-clear + per-originator queue entry + clientId authorization + depth/length caps + logging - Child:
craft/drainMidTurnQueueext-method shared constant frombridgeTypes+-32601latch-off for unsupported clients - SSE: per-originator frame grouping with
originatorClientIdon envelope + never-throws publish with fallback logging - Browser: identity-removal consume (no blanket-clear) + per-turn
AbortControlleraborted at settle +removeInjectedFromQueuewith originator filtering +MAX_PENDING_BATCHESeviction cap - SDK:
fetchWithTimeoutintegration + signal propagation +DaemonSessionClientforwarding
Each invariant has at least one test that fails when the relevant line is deleted.
Only known gap: App-level AbortController lifecycle has no RTL test (acknowledged as follow-up). Non-blocking — SDK signal contract is covered.
Ship it.
Code ReviewRead the full diff across all 29 files. The implementation is clean — no critical blockers, no security holes, no AGENTS.md violations. The architecture follows the existing The concurrency handling is where this PR earns its complexity. Five distinct race conditions are each handled with a specific mechanism:
One observation: the Unit tests: All pass — bridge (282), bridgeClient (23), server (441), SDK (253), webui (84), sidechannel (new). Total: 1,083+ tests green across affected packages. Real-Scenario TestingDrove the PR's daemon with curl against the new All four HTTP contract tests behave as expected: 404 for unknown sessions, 401 for missing auth, 400 for empty/whitespace-only messages. The daemon starts cleanly, the route is registered, and error responses match the sibling Note: the full mid-turn drain (message → bridge queue → ACP child drain → model injection → SSE echo → browser dedupe) requires a live model session and was validated by the PR author against DeepSeek/deepseek-v4-pro with evidence in the PR description. The server-side HTTP contract verified here is the externally-facing half of that chain. 中文说明代码审查完整阅读了 29 个文件的 diff。实现干净——无关键阻断问题、无安全漏洞、无 AGENTS.md 违规。 架构遵循了现有的 并发处理是这个 PR 复杂度的价值所在。五种不同的竞态条件各有专门机制：
一个观察： 单元测试： 全部通过——bridge (282)、bridgeClient (23)、server (441)、SDK (253)、webui (84)、sidechannel（新增）。受影响包共计 1,083+ 测试绿色。 真实场景测试用 curl 对 PR 的 daemon 新端点 所有四个 HTTP 契约测试表现如预期：未知 session 返回 404、缺少 auth 返回 401、空消息/纯空白消息返回 400。daemon 正常启动、路由已注册、错误响应与 注意：完整的 mid-turn drain（消息 → bridge 队列 → ACP 子进程 drain → 模型注入 → SSE 回显 → 浏览器去重）需要真实模型会话，PR 作者已在 DeepSeek/deepseek-v4-pro 上验证并提供了 PR 描述中的证据。此处验证的服务端 HTTP 契约是该链路的外部接口部分。 — Qwen Code · qwen3.7-max |
ReflectionWhat this PR does well:
What could be improved (minor, non-blocking):
Risk assessment:
VerdictAPPROVE ✅ All three gate checks pass:
The implementation is well-designed for a genuinely complex concurrency problem. Test coverage is thorough and the architecture is consistent with the rest of the codebase. Approving. 中文说明反思PR 做得好的方面：
可以改进的方面（次要，不阻断）：
风险评估：
结论批准 ✅ 所有三项检查均通过：模板与方向有效、代码审查干净无阻断、1,083+ 单元测试全部通过、HTTP 端点真实场景测试正确、类型检查干净。 该实现针对一个真正复杂的并发问题设计良好。测试覆盖充分，架构与代码库的其余部分一致。批准合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Approve. Code review clean, 1083+ unit tests pass, HTTP contract verified via tmux real-scenario testing, typechecks clean. See Stage 1–3 comments for full analysis.
…rains (#5266) * fix(daemon): centralize mid-turn event constant + recover timed-out drains Follow-up to #5175 addressing two post-merge /review suggestions. Centralize the `mid_turn_message_injected` SSE event `type`: it was a bare literal in the daemon publisher (acp-bridge), the SDK validator/reducer, and the browser consumer (webui), so a rename in one could silently break browser-side dedup. It now lives once in acp-bridge's dependency-free `daemonEventTypes` module (lightweight like `mcpTimeouts`, so the SDK re-exports it via its build-time devDep without dragging acp-bridge's type graph into the SDK bundle), and bridgeClient / the SDK / webui all import the single binding. Close the drain-timeout message-loss window: the daemon splices + SSE-publishes (browser dedupes) before the ACP child's response lands, so if the child's 2s drain timeout fires first, the late response was discarded — losing the messages from both queues (silent, one-turn loss). The child now recovers that late response and injects it on the next batch instead of dropping it. Tests: drain-timeout recovery (Session), and a rename-safety assertion pinning the shared event constant to the wire literal. * fix(daemon): address review — log drain recovery + rename buildMidTurnParts - Emit a `debugLogger.debug` line when a timed-out drain is recovered (session id + count), guarded on a non-empty payload, so the recovery path is correlatable in production logs. - Rename `#formatMidTurnParts` → `#buildMidTurnParts`: the method records to the chat transcript, so a "format" verb understated its side effect.
What this PR does
Lets the web-shell hand a message the user types while a turn is still running to that turn, instead of holding it until the turn finishes. When a turn is in flight and the user submits, the message is offered to the running turn and drained into it between tool batches; if the turn has already ended (or the message carries images) it falls back to the existing next-turn queue. A message is delivered exactly once — never both mid-turn and as a next-turn prompt.
Mechanically: the spawned
qwen --acpchild already asks the client for queued messages between tool batches via thecraft/drainMidTurnQueueext-method (this is how the desktop app does mid-turn injection). Inqwen servethe client side is the daemon'sBridgeClient, which did not implementextMethod, so the child got JSON-RPC-32601and permanently latched the drain off for the session. This PR implements that path end-to-end.Why it's needed
On the desktop app a message typed mid-turn is injected into the running turn so the model sees it before the turn ends. On the web-shell the same keystrokes were always held until the next turn, because the daemon never answered the drain. This brings the web-shell to parity: follow-up context ("also check the tests", "actually use X") reaches the model during the turn instead of a turn late.
Reviewer Test Plan
How to verify
Server + SDK (automated):
cd packages/acp-bridge && npx vitest run→ 464 pass, incl. newBridgeClient.extMethoddrain (5) andenqueueMidTurnMessageaccept-gate / idle-clear (4).cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/daemonEvents.test.ts→ pass, incl. newPOST /session/:id/mid-turn-messagecontract (4) andmid_turn_message_injectedvalidation (1).cd packages/webui && npx vitest run src/daemon→ 129 pass, incl. new sidechannel parse / pub-sub (4).npx tsc --noEmitclean inacp-bridge/sdk-typescript/webui(web-shellhas 2 unrelated pre-existing fixture errors).Manual (browser): open the web-shell, start a tool-heavy turn, type a follow-up and submit while tools are running. Expected: the daemon drains it into the turn (the model's subsequent output reflects it) and it is not resent as a separate next-turn prompt. With an idle session, or a message containing an image, it is queued for the next turn as before.
Evidence (Before & After)
Before: a mid-turn submit was held in the browser's local queue and sent as a brand-new turn after the current one ended;
BridgeClientanswered the child's drain with-32601, latching it off.After: a mid-turn submit (text-only, turn live) goes to the daemon queue, is drained into the running turn, emits a
mid_turn_message_injectedSSE frame, and is removed from the browser queue.Live daemon run — verified end-to-end against a real
qwen serve(this PR's code) over the HTTP API, modelDeepSeek/deepseek-v4-pro, on a turn running a shell tool (sleep 10). The endpoint accepted the mid-turn message, the bridge drained it and published the injection frame, and the child recorded it once:Exactly-once: the transcript holds exactly 1
mid_turn_user_messageand 1 user-role record in total — no duplicate next-turn delivery (the injected text appears a second time only inside the model's own reasoning). The model also acted on it mid-turn: the prompt only asked tosleep 10then replyFINISHED, yet the model's reasoning reads "…and also check the tests" and it answered "5 test files. FINISHED", confirming the message entered the model's context during the running turn.Scope: this exercises the server/protocol path (the core drain). The browser-side dedupe of
queuedPromptsin the web-shell UI is covered by typecheck + the sidechannel/event unit tests, not a live browser session.Tested on
Environment (optional)
Unit tests +
tsc --noEmit+npm run build(sdk-typescript,webui) on macOS. No livenpm run devbrowser session.Risk & Scope
acpRouteTable+AcpDispatcher); the web-shell uses the REST transport, which is wired.mid_turn_message_injectedframe, and an older daemon returns 404 to the new endpoint so the browser keeps its next-turn fallback.Linked Issues
N/A
中文说明
这个 PR 做了什么
让 web-shell 把用户在一个 turn 还在运行时输入的消息交给那个 turn,而不是攒到 turn 结束才发。当一个 turn 正在进行、用户提交时,消息会被提供给运行中的 turn,并在两批 tool 调用之间被排空注入;如果该 turn 已经结束(或消息带图片),则退回现有的"下一轮"队列。每条消息恰好交付一次——绝不会既 mid-turn 注入又作为下一轮 prompt 重发。
机制上:被 spawn 的
qwen --acp子进程本来就会在每批 tool 之间通过craft/drainMidTurnQueueext-method 向 client 索取排队消息(desktop 应用就是这样做 mid-turn 注入的)。在qwen serve里 client 侧是 daemon 的BridgeClient,而它没有实现extMethod,所以子进程收到 JSON-RPC-32601,该 session 后续永久关闭 drain。本 PR 把这条链路端到端补齐。为什么需要
在 desktop 应用里,mid-turn 输入的消息会被注入运行中的 turn,模型在 turn 结束前就能看到。而在 web-shell,同样的输入一直被攒到下一轮,因为 daemon 从不应答 drain。本 PR 让 web-shell 对齐:后续上下文("也看下测试""其实用 X")能在 turn 进行中抵达模型,而不是晚一整轮。
审阅者测试计划
如何验证
服务端 + SDK(自动化):
cd packages/acp-bridge && npx vitest run→ 464 通过,含新增BridgeClient.extMethoddrain(5)与enqueueMidTurnMessage接受门控 / idle 清空(4)。cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/daemonEvents.test.ts→ 通过,含新增POST /session/:id/mid-turn-message契约(4)与mid_turn_message_injected校验(1)。cd packages/webui && npx vitest run src/daemon→ 129 通过,含新增 sidechannel parse / pub-sub(4)。npx tsc --noEmit在acp-bridge/sdk-typescript/webui干净(web-shell有 2 个与本次无关的预存 fixture 报错)。手动(浏览器):打开 web-shell,开一个 tool 密集的 turn,在 tool 运行时输入后续消息并提交。预期:daemon 把它排空注入该 turn(模型后续输出体现它),且不会再作为单独的下一轮 prompt 重发。若 session 空闲、或消息含图片,则像以前一样进入下一轮队列。
证据(前后对比)
之前:mid-turn 提交被攒在浏览器本地队列,等当前 turn 结束后作为全新一轮发出;
BridgeClient用-32601应答子进程的 drain,将其 latch 关闭。之后:mid-turn 提交(纯文本、turn 进行中)进入 daemon 队列,被排空注入运行中的 turn,发出
mid_turn_message_injectedSSE 帧,并从浏览器队列移除。真机 daemon 验证——对真实运行的
qwen serve(本 PR 代码)经 HTTP API 端到端验证,模型DeepSeek/deepseek-v4-pro,在一个跑 shell tool(sleep 10)的 turn 上注入。端点接受了 mid-turn 消息,bridge 排空并发布了注入帧,子进程只记录了一次:Exactly-once:转录中恰好 1 条
mid_turn_user_message、user 角色记录共 1 条——没有第二次的 next-turn 投递(注入文本第二次出现仅在模型自身的推理里)。模型也在 turn 中途据此行动:原 prompt 只要求sleep 10后回FINISHED,而模型推理写道"…and also check the tests"、最终答复"5 test files. FINISHED",证明该消息在 turn 进行中进入了模型上下文。范围:这验证的是服务端/协议路径(核心 drain)。web-shell UI 里
queuedPrompts的浏览器侧去重由类型检查 + sidechannel/事件单测覆盖,非 live 浏览器会话。测试平台
环境(可选)
macOS 上的单测 +
tsc --noEmit+npm run build(sdk-typescript、webui)。未做 livenpm run dev浏览器会话。风险与范围
acpRouteTable+AcpDispatcher)的 mid-turn 摄入也在范围外;web-shell 用 REST 传输,已接通。mid_turn_message_injected帧,旧 daemon 对新端点返回 404,浏览器据此保留下一轮兜底。关联 Issue
N/A