From 7f70f3426f6ef2122a4bde2e7c37805609da81ed Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 07:04:42 +0800 Subject: [PATCH 1/7] feat(daemon): deliver web-shell mid-turn messages into the running turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/acp-bridge/src/bridge.test.ts | 104 ++++++++++++++++ packages/acp-bridge/src/bridge.ts | 54 ++++++++- packages/acp-bridge/src/bridgeClient.test.ts | 112 ++++++++++++++++++ packages/acp-bridge/src/bridgeClient.ts | 68 +++++++++++ packages/acp-bridge/src/bridgeTypes.ts | 13 ++ packages/cli/src/serve/server.ts | 28 +++++ packages/sdk-typescript/scripts/build.js | 4 +- .../sdk-typescript/src/daemon/DaemonClient.ts | 30 +++++ .../src/daemon/DaemonSessionClient.ts | 17 +++ packages/sdk-typescript/src/daemon/events.ts | 37 ++++++ packages/sdk-typescript/src/daemon/index.ts | 3 + packages/sdk-typescript/src/daemon/types.ts | 10 ++ .../test/unit/DaemonClient.test.ts | 53 +++++++++ .../test/unit/daemonEvents.test.ts | 47 ++++++++ packages/web-shell/client/App.tsx | 41 ++++++- packages/webui/src/daemon-react-sdk.ts | 3 + packages/webui/src/daemon/index.ts | 2 + .../daemon/midTurnInjectedSidechannel.test.ts | 89 ++++++++++++++ .../src/daemon/midTurnInjectedSidechannel.ts | 84 +++++++++++++ .../daemon/session/DaemonSessionProvider.tsx | 11 ++ packages/webui/src/daemon/session/actions.ts | 17 +++ packages/webui/src/daemon/session/types.ts | 8 ++ .../src/daemon/useDaemonMidTurnInjected.ts | 29 +++++ 23 files changed, 861 insertions(+), 3 deletions(-) create mode 100644 packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts create mode 100644 packages/webui/src/daemon/midTurnInjectedSidechannel.ts create mode 100644 packages/webui/src/daemon/useDaemonMidTurnInjected.ts diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 3d78697886f..59c3f28bc11 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -10301,3 +10301,107 @@ describe('close on last client detach', () => { await bridge.shutdown(); }); }); + +/** + * `enqueueMidTurnMessage` backs the web-shell mid-turn drain: the browser + * pushes a message typed during a turn, the ACP child drains it via + * `craft/drainMidTurnQueue` (answered by BridgeClient.extMethod). The accept + * gate is the exactly-once linchpin — it must reject when the session is idle + * so the browser's own next-turn queue stays the single delivery path in the + * settle-window race. + */ +describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)', () => { + function hangingPromptFactory(): { + factory: ChannelFactory; + release: () => void; + } { + let release: (() => void) | undefined; + const factory: ChannelFactory = async () => + makeChannel({ + promptImpl: async () => { + await new Promise((res) => { + release = res; + }); + return { stopReason: 'end_turn' }; + }, + }).channel; + return { factory, release: () => release?.() }; + } + + it('rejects (accepted:false) when the session is idle', async () => { + const bridge = makeBridge({ + channelFactory: async () => makeChannel().channel, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(bridge.enqueueMidTurnMessage(session.sessionId, 'later')).toEqual({ + accepted: false, + }); + await bridge.shutdown(); + }); + + it('accepts while a turn is in flight, then rejects again once it settles', async () => { + const { factory, release } = hangingPromptFactory(); + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const promptPromise = bridge + .sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'run tools' }], + }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + + // Let the FIFO worker pick up the (hanging) prompt before asserting. + await new Promise((r) => setTimeout(r, 10)); + expect( + bridge.enqueueMidTurnMessage(session.sessionId, 'also check tests'), + ).toEqual({ accepted: true }); + + // Settle the turn → the queue flips back to idle and the undrained copy is + // dropped server-side (the browser resends it as the next turn). + release(); + await promptPromise; + expect( + bridge.enqueueMidTurnMessage(session.sessionId, 'next time'), + ).toEqual({ accepted: false }); + await bridge.shutdown(); + }); + + it('rejects a whitespace-only message even while busy', async () => { + const { factory, release } = hangingPromptFactory(); + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const promptPromise = bridge + .sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'x' }], + }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + expect(bridge.enqueueMidTurnMessage(session.sessionId, ' ')).toEqual({ + accepted: false, + }); + release(); + await promptPromise; + await bridge.shutdown(); + }); + + it('throws SessionNotFoundError for an unknown session', async () => { + const bridge = makeBridge({ + channelFactory: async () => makeChannel().channel, + }); + expect(() => bridge.enqueueMidTurnMessage('nope', 'hi')).toThrow( + SessionNotFoundError, + ); + await bridge.shutdown(); + }); +}); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 20bce325dd4..9c119b0de92 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -229,6 +229,19 @@ interface SessionEntry { promptQueue: Promise; /** Accepted prompts that have not settled yet (queued + active). */ pendingPromptCount: number; + /** + * Mid-turn user messages pushed by the browser (`POST + * /session/:id/mid-turn-message`) while a turn is running. The ACP child + * drains these between tool batches via the `craft/drainMidTurnQueue` + * ext-method so the model sees them before the turn ends. The queue is + * accepted into only while the session is busy (`pendingPromptCount > 0`) + * and emptied when the session next goes idle — see the settle handler in + * `sendPrompt`. The browser keeps its own copy as the next-turn fallback, + * so a message left undrained here is NOT lost: it is dropped server-side + * (preventing a stale next-turn re-injection) and resent by the browser as + * a fresh prompt. + */ + midTurnMessageQueue: string[]; /** * Per-session model-change FIFO. Prevents two concurrent * `applyModelServiceId` calls (e.g. simultaneous attach-with-different- @@ -2002,6 +2015,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { events, promptQueue: Promise.resolve(), pendingPromptCount: 0, + midTurnMessageQueue: [], modelChangeQueue: Promise.resolve(), approvalModeQueue: Promise.resolve(), modelPublishGeneration: 0, @@ -2941,7 +2955,26 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { () => undefined, () => undefined, ); - result.finally(releasePromptSlot).catch(() => {}); + result + .finally(() => { + releasePromptSlot(); + // Mid-turn messages are scoped to the turn the user typed them + // during. Once the session goes fully idle with some still + // undrained, drop the server-side copy: the browser still holds + // them in its own queue and resends them as the next turn. Leaving + // them here would let the NEXT turn's first tool batch inject a + // stale message the browser ALSO resends — double delivery. The + // `pendingPromptCount === 0` guard keeps queued messages intact + // across a back-to-back FIFO of prompts (still "one turn" to the + // user) and only clears at the true idle boundary. + if ( + entry.pendingPromptCount === 0 && + entry.midTurnMessageQueue.length > 0 + ) { + entry.midTurnMessageQueue.length = 0; + } + }) + .catch(() => {}); return result; }, @@ -4032,6 +4065,25 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }; }, + enqueueMidTurnMessage(sessionId, message) { + const entry = byId.get(sessionId); + if (!entry) throw new SessionNotFoundError(sessionId); + const trimmed = message.trim(); + // Reject empty messages and — critically — messages that arrive while + // the session is idle. The browser only pushes here when it believes a + // turn is running, but the turn may have settled in the small window + // before its turn-complete frame landed. Accepting an idle message + // would strand it until the NEXT turn's first tool batch drained it, + // by which point the browser has already resent it as a fresh prompt — + // double delivery. Rejecting keeps the browser's next-turn fallback the + // single delivery path in that race. + if (trimmed.length === 0 || entry.pendingPromptCount === 0) { + return { accepted: false }; + } + entry.midTurnMessageQueue.push(trimmed); + return { accepted: true }; + }, + async generateSessionBtw(sessionId, question, signal, _context) { const entry = byId.get(sessionId); if (!entry) throw new SessionNotFoundError(sessionId); diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts index 53f526e1f44..ad3238b2050 100644 --- a/packages/acp-bridge/src/bridgeClient.test.ts +++ b/packages/acp-bridge/src/bridgeClient.test.ts @@ -38,6 +38,7 @@ import type { WriteTextFileRequest, WriteTextFileResponse, } from '@agentclientprotocol/sdk'; +import { RequestError } from '@agentclientprotocol/sdk'; import { BridgeClient } from './bridgeClient.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; import { CancelSentinelCollisionError } from './bridgeErrors.js'; @@ -596,3 +597,114 @@ describe('BridgeClient — requestPermission pre-publish collision guard', () => expect(fakeEntry.pendingPermissionIds.size).toBe(0); }); }); + +/** + * `extMethod` is the daemon's answer to the ACP child's + * `craft/drainMidTurnQueue` call (web-shell mid-turn drain). Desktop answers + * the same method from its own in-memory queue; in `qwen serve` the BridgeClient + * answers it from `SessionEntry.midTurnMessageQueue`. Without this the SDK's + * ClientSideConnection would reject the call with -32601 and the child would + * latch the drain as unavailable for the whole session. + */ +describe('BridgeClient — mid-turn queue drain (craft/drainMidTurnQueue)', () => { + const thrower = () => { + throw new Error('test: permission flow should not run'); + }; + + function makeClientWithEntry( + sessionId: string, + entry: + | { + sessionId: string; + midTurnMessageQueue: string[]; + events: { publish: ReturnType }; + } + | undefined, + ): BridgeClient { + return new BridgeClient( + ((sid: string) => (sid === sessionId ? entry : undefined)) as never, + thrower as never, + { request: thrower } as never, + 0, + Infinity, + ); + } + + it('drains the queue, returns the messages, and publishes one injected frame', async () => { + const publish = vi.fn().mockReturnValue(true); + const entry = { + sessionId: 'sess:drain', + midTurnMessageQueue: ['first', 'second'], + events: { publish }, + }; + const client = makeClientWithEntry('sess:drain', entry); + + const result = await client.extMethod('craft/drainMidTurnQueue', { + sessionId: 'sess:drain', + }); + + expect(result).toEqual({ messages: ['first', 'second'] }); + // Queue emptied so the same messages can't be re-injected on the next batch. + expect(entry.midTurnMessageQueue).toEqual([]); + // Exactly one SSE frame carrying the drained text for the browser to dedupe. + expect(publish).toHaveBeenCalledTimes(1); + expect(publish.mock.calls[0][0]).toMatchObject({ + type: 'mid_turn_message_injected', + data: { sessionId: 'sess:drain', messages: ['first', 'second'] }, + }); + }); + + it('returns an empty drain and publishes nothing when the queue is empty', async () => { + const publish = vi.fn().mockReturnValue(true); + const entry = { + sessionId: 'sess:empty', + midTurnMessageQueue: [] as string[], + events: { publish }, + }; + const client = makeClientWithEntry('sess:empty', entry); + + const result = await client.extMethod('craft/drainMidTurnQueue', { + sessionId: 'sess:empty', + }); + + expect(result).toEqual({ messages: [] }); + expect(publish).not.toHaveBeenCalled(); + }); + + it('returns an empty drain for an unknown session without throwing', async () => { + const client = makeClientWithEntry('sess:known', undefined); + const result = await client.extMethod('craft/drainMidTurnQueue', { + sessionId: 'sess:absent', + }); + expect(result).toEqual({ messages: [] }); + }); + + it('short-circuits to an empty drain when no sessionId is supplied', async () => { + // resolveEntry(undefined) throws on a multi-session channel, so extMethod + // must answer before ever calling it when the sessionId is missing. + const resolveThatThrowsOnUndefined = (sid?: string) => { + if (!sid) { + throw new Error('resolveEntry must not run without a sessionId'); + } + return undefined; + }; + const client = new BridgeClient( + resolveThatThrowsOnUndefined as never, + thrower as never, + { request: thrower } as never, + 0, + Infinity, + ); + const result = await client.extMethod('craft/drainMidTurnQueue', {}); + expect(result).toEqual({ messages: [] }); + }); + + it('rejects an unknown ext-method with JSON-RPC methodNotFound (-32601)', async () => { + const client = makeClientWithEntry('sess:x', undefined); + const err = await client + .extMethod('craft/somethingElse', { sessionId: 'sess:x' }) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(RequestError); + expect((err as RequestError).code).toBe(-32601); + }); +}); diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 258ea213d91..95b5350d7b8 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -196,10 +196,34 @@ function sliceLineRange( * `SessionEntry` is required to provide them — TS enforces the * structural match at the callback signature). */ +/** + * ACP ext-method the spawned `qwen --acp` child calls between tool batches to + * pull user messages the browser queued mid-turn. Must match the constant in + * `cli/src/acp-integration/session/Session.ts` (the caller). The desktop ACP + * client answers the same method from its own in-memory queue; in `qwen serve` + * the daemon answers it here, from `SessionEntry.midTurnMessageQueue`. + */ +const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'; + +/** + * SSE frame published when mid-turn messages are actually drained into the + * running turn. The browser consumes it to move those messages out of its + * pending queue (so they aren't resent as the next turn) and render them as + * sent. `data: { sessionId, messages: string[] }`. + */ +const MID_TURN_MESSAGE_INJECTED_EVENT = 'mid_turn_message_injected'; + export interface BridgeClientSessionEntry { sessionId: string; events: EventBus; pendingPermissionIds: Set; + /** + * Mid-turn user messages queued by the browser, drained here when the ACP + * child calls the `craft/drainMidTurnQueue` ext-method. Owned by the full + * `SessionEntry` in `bridge.ts`; surfaced on this narrowed view so + * `extMethod` can splice it. See `SessionEntry.midTurnMessageQueue`. + */ + midTurnMessageQueue: string[]; activePromptOriginatorClientId?: string; /** * True while the bridge drives a model roundtrip; the @@ -507,6 +531,50 @@ export class BridgeClient implements Client { */ private readonly inFlightRestoreIds = new Set(); + /** + * Handle child->bridge ACP `extMethod` requests (calls that expect a + * response, unlike `extNotification`). The only method served today is + * `craft/drainMidTurnQueue`: the ACP child calls it between tool batches to + * pull any messages the browser queued mid-turn. We splice the per-session + * queue, return them to the child as the response, and — when non-empty — + * publish a `mid_turn_message_injected` SSE frame so the browser can move + * those messages out of its pending queue and render them as sent. Unknown + * methods reject with ACP `methodNotFound` (-32601), matching the SDK's + * default for an unimplemented client surface; the child's drain caller + * treats that as "drain unsupported" and stops asking. + */ + async extMethod( + method: string, + params: Record, + ): Promise> { + if (method !== MID_TURN_QUEUE_DRAIN_METHOD) { + throw RequestError.methodNotFound(method); + } + const sessionId = + typeof params['sessionId'] === 'string' + ? (params['sessionId'] as string) + : undefined; + // The drain always carries a sessionId; without one we can't route it on a + // multi-session channel (and `resolveEntry(undefined)` would throw there), + // so answer with an empty drain rather than poisoning the turn. + if (!sessionId) return { messages: [] }; + const entry = this.resolveEntry(sessionId); + if (!entry) return { messages: [] }; + const messages = entry.midTurnMessageQueue.splice(0); + if (messages.length > 0) { + try { + entry.events.publish({ + type: MID_TURN_MESSAGE_INJECTED_EVENT, + data: { sessionId: entry.sessionId, messages }, + }); + } catch { + // Bus already closed (session tearing down). The child still receives + // the messages below; only the browser-side echo is lost. + } + } + return { messages }; + } + /** * Handle child->bridge ACP `extNotification` calls. Six methods are * recognized — `qwen/notify/session/model-update`, diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 43e1baddb16..d6efb87a602 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -483,6 +483,19 @@ export interface AcpSessionBridge { context?: BridgeClientRequestContext, ): Promise<{ sessionId: string; answer: string | null }>; + /** + * Queue a mid-turn user message for the running turn. The ACP child drains + * it between tool batches via the `craft/drainMidTurnQueue` ext-method so + * the model sees it before the turn ends. Accepted only while the session + * is busy (a prompt is queued or active); an idle session returns + * `{ accepted: false }` so the caller falls back to a normal next-turn + * prompt. Throws `SessionNotFoundError` for unknown ids. + */ + enqueueMidTurnMessage( + sessionId: string, + message: string, + ): { accepted: boolean }; + /** * Execute a shell command directly on the daemon (no LLM involvement). * Streams output through the session's SSE bus and injects the diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 4be51e46231..bd91c703085 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2699,6 +2699,34 @@ export function createServeApp( } }); + // Queue a user message typed while the session's turn is still running. The + // ACP child drains it between tool batches (`craft/drainMidTurnQueue`) so the + // model sees it before the turn ends, instead of waiting for the next turn. + // Returns `{ accepted }`: `false` when the session is idle, so the browser + // keeps the message in its own queue and sends it as a normal next-turn + // prompt. Synchronous — the bridge only pushes onto an in-memory queue. + app.post('/session/:id/mid-turn-message', mutate(), (req, res) => { + const sessionId = requireSessionId(req, res); + if (sessionId === null) return; + const body = safeBody(req); + const message = body['message']; + if (typeof message !== 'string' || message.trim().length === 0) { + res.status(400).json({ + error: '`message` is required and must be a non-empty string', + }); + return; + } + try { + const result = bridge.enqueueMidTurnMessage(sessionId, message); + res.status(200).json(result); + } catch (err) { + sendBridgeError(res, err, { + route: 'POST /session/:id/mid-turn-message', + sessionId, + }); + } + }); + app.post('/session/:id/shell', mutate({ strict: true }), async (req, res) => { const sessionId = req.params['id']; if (!sessionShellCommandEnabled) { diff --git a/packages/sdk-typescript/scripts/build.js b/packages/sdk-typescript/scripts/build.js index 7e109ba3c04..ecca56c30e6 100755 --- a/packages/sdk-typescript/scripts/build.js +++ b/packages/sdk-typescript/scripts/build.js @@ -23,7 +23,9 @@ const rootDir = join(__dirname, '..'); // Budget includes the DaemonTransport interface + DaemonTransportClosedError + // RestSseTransport (default transport, constructed by DaemonClient). // Bumped from 116KB to 118KB for the transport abstraction layer (~1.5KB). -const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 118 * 1024; +// Bumped from 118KB to 119KB for the mid-turn drain surface (enqueue methods + +// `mid_turn_message_injected` event type/guard/registration, ~150 bytes). +const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 119 * 1024; rmSync(join(rootDir, 'dist'), { recursive: true, force: true }); mkdirSync(join(rootDir, 'dist'), { recursive: true }); diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 0768cd5d631..8b041c887ab 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -70,6 +70,7 @@ import type { DaemonMcpManageAction, DaemonMcpManageResult, DaemonSessionBtwResult, + DaemonMidTurnMessageResult, DaemonSessionRecapResult, DaemonShellCommandResult, DaemonRuntimeMcpAddRequest, @@ -1477,6 +1478,35 @@ export class DaemonClient { return (await res.json()) as DaemonSessionBtwResult; } + /** + * Queue a user message typed while the session's turn is still running. The + * ACP child drains it between tool batches so the model sees it before the + * turn ends. Resolves `{ accepted: false }` when the session is idle — the + * caller should then send the message as a normal next-turn prompt. + */ + async enqueueMidTurnMessage( + sessionId: string, + message: string, + opts?: { signal?: AbortSignal; clientId?: string }, + ): Promise { + const res = await this.transport.fetch( + `${this.baseUrl}/session/${encodeURIComponent(sessionId)}/mid-turn-message`, + { + method: 'POST', + headers: this.headers( + { 'Content-Type': 'application/json' }, + opts?.clientId, + ), + body: JSON.stringify({ message }), + signal: opts?.signal, + }, + ); + if (!res.ok) { + throw await this.failOnError(res, 'POST /session/:id/mid-turn-message'); + } + return (await res.json()) as DaemonMidTurnMessageResult; + } + /** * Execute a direct daemon-side shell command for a session. The daemon must * be started with direct session shell enabled and bearer auth configured; diff --git a/packages/sdk-typescript/src/daemon/DaemonSessionClient.ts b/packages/sdk-typescript/src/daemon/DaemonSessionClient.ts index a3674a2cd63..5948113018d 100644 --- a/packages/sdk-typescript/src/daemon/DaemonSessionClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonSessionClient.ts @@ -20,6 +20,7 @@ import type { DaemonRewindResult, DaemonRewindSnapshotInfo, DaemonSessionBtwResult, + DaemonMidTurnMessageResult, DaemonSessionContextStatus, DaemonSessionContextUsageStatus, DaemonSessionRecapResult, @@ -367,6 +368,22 @@ export class DaemonSessionClient { }); } + /** + * Queue a user message typed while this session's turn is still running so + * the ACP child can drain it mid-turn. Forwards the client id bound at + * create/attach. Resolves `{ accepted: false }` when the session is idle — + * the caller should then send the message as a normal next-turn prompt. + */ + async enqueueMidTurnMessage( + message: string, + opts?: { signal?: AbortSignal }, + ): Promise { + return await this.client.enqueueMidTurnMessage(this.sessionId, message, { + ...(opts?.signal ? { signal: opts.signal } : {}), + ...(this.clientId ? { clientId: this.clientId } : {}), + }); + } + /** * Execute a direct daemon-side shell command for this session. Requires the * daemon to opt in to direct session shell and bearer auth; this wrapper diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 05b7e7c3eef..c5f3ff13bd6 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -21,6 +21,7 @@ export const DAEMON_KNOWN_EVENT_TYPE_VALUES = [ 'session_died', 'session_closed', 'session_metadata_updated', + 'mid_turn_message_injected', 'client_evicted', 'slow_client_warning', 'stream_error', @@ -250,6 +251,18 @@ export interface DaemonSessionMetadataUpdatedData { [key: string]: unknown; } +/** + * `mid_turn_message_injected` payload. Emitted when the daemon drains + * browser-queued mid-turn messages into the running turn (web-shell mid-turn + * drain). Consumers move these messages out of their pending queue — so they + * aren't resent as the next turn — and render them as sent. + */ +export interface DaemonMidTurnMessageInjectedData { + sessionId: string; + messages: string[]; + [key: string]: unknown; +} + export interface DaemonClientEvictedData { reason: string; droppedAfter?: number; @@ -742,6 +755,10 @@ export type DaemonSessionMetadataUpdatedEvent = DaemonEventEnvelope< 'session_metadata_updated', DaemonSessionMetadataUpdatedData >; +export type DaemonMidTurnMessageInjectedEvent = DaemonEventEnvelope< + 'mid_turn_message_injected', + DaemonMidTurnMessageInjectedData +>; export type DaemonClientEvictedEvent = DaemonEventEnvelope< 'client_evicted', DaemonClientEvictedData @@ -874,6 +891,7 @@ export type DaemonSessionEvent = | DaemonSessionDiedEvent | DaemonSessionClosedEvent | DaemonSessionMetadataUpdatedEvent + | DaemonMidTurnMessageInjectedEvent | DaemonSessionBranchedEvent; export type DaemonControlEvent = @@ -1319,6 +1337,10 @@ export function asKnownDaemonEvent( return isSessionMetadataUpdatedData(event.data) ? (event as DaemonSessionMetadataUpdatedEvent) : undefined; + case 'mid_turn_message_injected': + return isMidTurnMessageInjectedData(event.data) + ? (event as DaemonMidTurnMessageInjectedEvent) + : undefined; case 'client_evicted': return isClientEvictedData(event.data) ? (event as DaemonClientEvictedEvent) @@ -1799,9 +1821,13 @@ export function reduceDaemonSessionEvent( ...base, lastTurnError: event.data, }; + // `mid_turn_message_injected` is a transient UX signal (the browser dedupes + // its own pending queue); like these mcp/settings notices it carries no + // reduced session-view state. case 'mcp_server_added': case 'mcp_server_removed': case 'settings_reloaded': + case 'mid_turn_message_injected': return base; case 'session_rewound': return { @@ -2233,6 +2259,17 @@ function isSessionMetadataUpdatedData( ); } +function isMidTurnMessageInjectedData( + value: unknown, +): value is DaemonMidTurnMessageInjectedData { + return ( + isRecord(value) && + isNonEmptyString(value['sessionId']) && + Array.isArray(value['messages']) && + value['messages'].every((message) => typeof message === 'string') + ); +} + function isClientEvictedData(value: unknown): value is DaemonClientEvictedData { return ( isRecord(value) && diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index 769e9445907..77a6139b4bf 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -246,6 +246,8 @@ export type { DaemonSessionEvent, DaemonSessionMetadataUpdatedData, DaemonSessionMetadataUpdatedEvent, + DaemonMidTurnMessageInjectedData, + DaemonMidTurnMessageInjectedEvent, DaemonSessionUpdateData, DaemonSessionUpdateEvent, DaemonSessionViewState, @@ -300,6 +302,7 @@ export type { DaemonRewindResult, DaemonRewindSnapshotInfo, DaemonSessionBtwResult, + DaemonMidTurnMessageResult, DaemonSessionRecapResult, DaemonShellCommandResult, DaemonRuntimeMcpAddRequest, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index e98f2d1e752..993f5bdbe3d 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -1113,6 +1113,16 @@ export interface DaemonSessionBtwResult { answer: string | null; } +/** + * Result body of `POST /session/:id/mid-turn-message`. `accepted` is `true` + * when the message was queued for the running turn (the ACP child drains it + * between tool batches); `false` when the session was idle, in which case the + * caller should send the message as a normal next-turn prompt instead. + */ +export interface DaemonMidTurnMessageResult { + accepted: boolean; +} + export interface DaemonShellCommandResult { exitCode: number | null; output: string; diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 7a10a189dfa..c1bf1091105 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -1873,6 +1873,59 @@ describe('DaemonClient', () => { }); }); + describe('enqueueMidTurnMessage (web-shell mid-turn drain)', () => { + it('POSTs the message and returns accepted:true', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, { accepted: true }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + const result = await client.enqueueMidTurnMessage('s-1', 'also check tests'); + expect(result).toEqual({ accepted: true }); + expect(calls[0]?.url).toBe('http://daemon/session/s-1/mid-turn-message'); + expect(calls[0]?.method).toBe('POST'); + expect(JSON.parse(calls[0]?.body as string)).toEqual({ + message: 'also check tests', + }); + }); + + it('returns accepted:false verbatim when the session is idle', async () => { + const { fetch } = recordingFetch(() => + jsonResponse(200, { accepted: false }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + const result = await client.enqueueMidTurnMessage('s-1', 'late'); + expect(result.accepted).toBe(false); + }); + + it('URL-encodes the session id and forwards client id + abort signal', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, { accepted: true }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + const ctrl = new AbortController(); + await client.enqueueMidTurnMessage('s/1', 'hi', { + clientId: 'client-1', + signal: ctrl.signal, + }); + expect(calls[0]?.url).toBe('http://daemon/session/s%2F1/mid-turn-message'); + expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-1'); + expect(calls[0]?.signal).toBe(ctrl.signal); + }); + + it('throws on 404 when the session is unknown', async () => { + const { fetch } = recordingFetch(() => + jsonResponse(404, { + error: 'session not found', + code: 'session_not_found', + }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + await expect( + client.enqueueMidTurnMessage('s-1', 'hi'), + ).rejects.toMatchObject({ status: 404 }); + }); + }); + describe('setWorkspaceToolEnabled (#4175 Wave 4 PR 17)', () => { it('POSTs the enabled flag and URL-encodes the tool name', async () => { const { fetch, calls } = recordingFetch(() => diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index b0327b92153..48d4d142bf5 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -588,6 +588,53 @@ describe('daemon event schema', () => { ).toBeUndefined(); }); + it('validates mid_turn_message_injected events', () => { + expect( + asKnownDaemonEvent({ + id: 1, + v: 1, + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: ['check the tests too'] }, + }), + ).toBeDefined(); + + // Empty array is structurally valid (the guard only requires a string[]). + expect( + asKnownDaemonEvent({ + id: 2, + v: 1, + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: [] }, + }), + ).toBeDefined(); + + // Missing messages, non-string entries, and missing sessionId are rejected. + expect( + asKnownDaemonEvent({ + id: 3, + v: 1, + type: 'mid_turn_message_injected', + data: { sessionId: 's-1' }, + }), + ).toBeUndefined(); + expect( + asKnownDaemonEvent({ + id: 4, + v: 1, + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: ['ok', 42] }, + }), + ).toBeUndefined(); + expect( + asKnownDaemonEvent({ + id: 5, + v: 1, + type: 'mid_turn_message_injected', + data: { messages: ['x'] }, + }), + ).toBeUndefined(); + }); + it('reduces session_closed as terminal and clears pending permissions', () => { const state = reduceDaemonSessionEvents([ { diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 8d5a618c9ed..4b60f1011cf 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -11,6 +11,7 @@ import { useActions, useConnection, useDaemonFollowupSuggestion, + useDaemonMidTurnInjected, useSettings, useSessionNotices, useStreamingState, @@ -1250,6 +1251,7 @@ export function App({ (text: string, images?: PromptImage[], onComplete?: () => void) => { const trimmed = text.trim(); if (!trimmed) return true; + const hasImages = !!images && images.length > 0; const nextPrompt: QueuedPrompt = { id: nextQueuedPromptIdRef.current++, text: trimmed, @@ -1258,9 +1260,18 @@ export function App({ }; queuedPromptsRef.current = [...queuedPromptsRef.current, nextPrompt]; setQueuedPrompts(queuedPromptsRef.current); + // Best-effort: also offer text-only messages to the running turn so the + // daemon can drain them mid-turn (text-only because the drain channel + // carries plain strings — messages with images stay queued for the next + // turn). On success the daemon emits `mid_turn_message_injected`, which + // the effect below removes from the queue so it isn't resent. If idle / + // unsupported / failed, the message harmlessly stays queued. + if (!hasImages) { + void sessionActions.enqueueMidTurnMessage(trimmed); + } return true; }, - [], + [sessionActions], ); const popNextQueuedPrompt = useCallback((): QueuedPrompt | null => { @@ -1287,6 +1298,34 @@ export function App({ return true; }, [store, t]); + // When the daemon drains queued messages into the running turn it emits + // `mid_turn_message_injected`. Drop the matching (text-only) entries from the + // local queue so the idle-time drain doesn't ALSO resend them as the next + // turn — the SSE frame is delivered in-order ahead of the turn-complete frame + // that flips streamingState to idle, so this runs before that resend fires. + const midTurnInjected = useDaemonMidTurnInjected(); + useEffect(() => { + if (!midTurnInjected) return; + if (midTurnInjected.sessionId !== connection.sessionId) return; + const remaining = [...queuedPromptsRef.current]; + let changed = false; + for (const message of midTurnInjected.messages) { + const index = remaining.findIndex( + (prompt) => + prompt.text === message && + (!prompt.images || prompt.images.length === 0), + ); + if (index >= 0) { + remaining.splice(index, 1); + changed = true; + } + } + if (changed) { + queuedPromptsRef.current = remaining; + setQueuedPrompts(remaining); + } + }, [midTurnInjected, connection.sessionId]); + const handleThemeChange = useCallback( (nextTheme: WebShellTheme) => { setSelectedTheme(nextTheme); diff --git a/packages/webui/src/daemon-react-sdk.ts b/packages/webui/src/daemon-react-sdk.ts index ce107842956..def486f5a51 100644 --- a/packages/webui/src/daemon-react-sdk.ts +++ b/packages/webui/src/daemon-react-sdk.ts @@ -134,6 +134,9 @@ export { useDaemonPromptStatus as usePromptStatus } from './daemon/session/index /** Server-pushed prompt follow-up suggestions for daemon-backed UIs. */ export { useDaemonFollowupSuggestion } from './daemon/index.js'; +/** Notifies when the daemon drains browser-queued messages into the running turn. */ +export { useDaemonMidTurnInjected } from './daemon/index.js'; + // ── Constants ───────────────────────────────────────────────────── /** Ordered list of approval modes for cycling: `['auto', 'suggest', 'ask']`. */ diff --git a/packages/webui/src/daemon/index.ts b/packages/webui/src/daemon/index.ts index af2ead4cb95..62a26ba0606 100644 --- a/packages/webui/src/daemon/index.ts +++ b/packages/webui/src/daemon/index.ts @@ -95,6 +95,8 @@ export { type UseDaemonFollowupSuggestionReturn, } from './useDaemonFollowupSuggestion.js'; +export { useDaemonMidTurnInjected } from './useDaemonMidTurnInjected.js'; + // ── Re-exported SDK types/constants for UI consumers ────────────── // These allow web-shell and other UI packages to depend only on // @qwen-code/webui without importing @qwen-code/sdk/daemon directly. diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts new file mode 100644 index 00000000000..dbf4013a4fd --- /dev/null +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts @@ -0,0 +1,89 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + clearSidechannelMidTurnInjected, + getSidechannelMidTurnInjected, + parseSidechannelMidTurnInjected, + publishSidechannelMidTurnInjected, + subscribeSidechannelMidTurnInjected, +} from './midTurnInjectedSidechannel.js'; + +afterEach(() => { + clearSidechannelMidTurnInjected(); +}); + +describe('parseSidechannelMidTurnInjected', () => { + it('parses a well-formed frame', () => { + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: ['hi', 'there'] }, + }), + ).toEqual({ sessionId: 's-1', messages: ['hi', 'there'] }); + }); + + it('filters non-string and empty entries', () => { + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: ['keep', '', 42, 'also'] }, + }), + ).toEqual({ sessionId: 's-1', messages: ['keep', 'also'] }); + }); + + it('returns undefined for wrong type, missing data, or no usable messages', () => { + expect( + parseSidechannelMidTurnInjected({ type: 'other', data: {} }), + ).toBeUndefined(); + expect( + parseSidechannelMidTurnInjected({ type: 'mid_turn_message_injected' }), + ).toBeUndefined(); + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + data: { sessionId: 's-1', messages: [''] }, + }), + ).toBeUndefined(); + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + data: { messages: ['x'] }, + }), + ).toBeUndefined(); + expect(parseSidechannelMidTurnInjected(null)).toBeUndefined(); + }); +}); + +describe('mid-turn injected sidechannel pub/sub', () => { + it('notifies listeners, exposes the latest batch, and clear resets', () => { + const listener = vi.fn(); + const unsubscribe = subscribeSidechannelMidTurnInjected(listener); + + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); + expect(listener).toHaveBeenCalledTimes(1); + expect(getSidechannelMidTurnInjected()).toEqual({ + sessionId: 's-1', + messages: ['a'], + }); + + // A fresh reference every publish so `useSyncExternalStore` re-fires even + // for identical text. + const first = getSidechannelMidTurnInjected(); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); + expect(listener).toHaveBeenCalledTimes(2); + expect(getSidechannelMidTurnInjected()).not.toBe(first); + + clearSidechannelMidTurnInjected(); + expect(listener).toHaveBeenCalledTimes(3); + expect(getSidechannelMidTurnInjected()).toBeUndefined(); + + unsubscribe(); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b'] }); + expect(listener).toHaveBeenCalledTimes(3); + }); +}); diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts new file mode 100644 index 00000000000..b5d0dc29ebe --- /dev/null +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -0,0 +1,84 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; + +/** + * Side channel for `mid_turn_message_injected` daemon events. Mirrors + * {@link ./followupSidechannel.ts}: the session event pump parses the raw + * frame and publishes here, and a consumer (`useDaemonMidTurnInjected`) reads + * the latest batch via `useSyncExternalStore`. Kept out of the transcript + * reducer because it is a transient UX signal — the consumer moves the matching + * messages out of its own pending queue (so they are not resent as the next + * turn) rather than rendering anything from it. + */ + +const listeners = new Set<() => void>(); +let lastInjected: DaemonMidTurnMessageInjectedData | undefined; + +export function getSidechannelMidTurnInjected(): + | DaemonMidTurnMessageInjectedData + | undefined { + return lastInjected; +} + +export function subscribeSidechannelMidTurnInjected( + listener: () => void, +): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +export function publishSidechannelMidTurnInjected( + data: DaemonMidTurnMessageInjectedData, +): void { + // Fresh object every publish so `useSyncExternalStore` sees a new snapshot + // and re-fires consumers even when two batches carry identical text. + lastInjected = { ...data, messages: [...data.messages] }; + notifyMidTurnInjectedListeners(); +} + +export function clearSidechannelMidTurnInjected(): void { + if (lastInjected === undefined) return; + lastInjected = undefined; + notifyMidTurnInjectedListeners(); +} + +function notifyMidTurnInjectedListeners(): void { + for (const listener of listeners) { + listener(); + } +} + +/** + * Parse a raw daemon SSE frame into the injected-messages payload, or + * `undefined` if the frame is not a well-formed `mid_turn_message_injected` + * event. Filters out non-string / empty entries; returns `undefined` when + * nothing usable remains. + */ +export function parseSidechannelMidTurnInjected( + event: unknown, +): DaemonMidTurnMessageInjectedData | undefined { + if (!event || typeof event !== 'object') return undefined; + const record = event as Record; + if (record['type'] !== 'mid_turn_message_injected') return undefined; + const data = record['data']; + if (!data || typeof data !== 'object') return undefined; + const dataRecord = data as Record; + const sessionId = dataRecord['sessionId']; + const messages = dataRecord['messages']; + if (typeof sessionId !== 'string' || !Array.isArray(messages)) { + return undefined; + } + const stringMessages = messages.filter( + (message): message is string => + typeof message === 'string' && message.length > 0, + ); + if (stringMessages.length === 0) return undefined; + return { sessionId, messages: stringMessages }; +} diff --git a/packages/webui/src/daemon/session/DaemonSessionProvider.tsx b/packages/webui/src/daemon/session/DaemonSessionProvider.tsx index 1d31485468d..c2ed96b6d4c 100644 --- a/packages/webui/src/daemon/session/DaemonSessionProvider.tsx +++ b/packages/webui/src/daemon/session/DaemonSessionProvider.tsx @@ -62,6 +62,10 @@ import { parseSidechannelFollowupSuggestion, publishSidechannelFollowupSuggestion, } from '../followupSidechannel.js'; +import { + parseSidechannelMidTurnInjected, + publishSidechannelMidTurnInjected, +} from '../midTurnInjectedSidechannel.js'; import type { ActivePrompt, AddDaemonSessionNotice, @@ -664,6 +668,13 @@ export function DaemonSessionProvider({ publishSidechannelFollowupSuggestion(followupSuggestion); continue; } + const midTurnInjected = parseSidechannelMidTurnInjected(event); + if (midTurnInjected) { + // Transient UX signal — consumers drop these from their pending + // queue. Not a transcript item, so skip normalization. + publishSidechannelMidTurnInjected(midTurnInjected); + continue; + } const normalizedUiEvents = normalizeAndFilterEvent( event, activeSession.clientId, diff --git a/packages/webui/src/daemon/session/actions.ts b/packages/webui/src/daemon/session/actions.ts index 656fa2f6d59..ec1771c9af8 100644 --- a/packages/webui/src/daemon/session/actions.ts +++ b/packages/webui/src/daemon/session/actions.ts @@ -9,6 +9,7 @@ import type { DaemonSessionContextStatus, DaemonSessionClient, DaemonSessionBtwResult, + DaemonMidTurnMessageResult, DaemonSessionRecapResult, DaemonSessionTaskStatus, DaemonTranscriptStore, @@ -599,6 +600,22 @@ export function createDaemonSessionActions({ } }, + async enqueueMidTurnMessage( + message: string, + ): Promise { + // Best-effort and silent: no session / idle session / transport failure + // all resolve `{ accepted: false }` so the caller falls back to its own + // next-turn queue. Never raises a user-facing notice — a queued message + // typed mid-turn is an optimization, not a user-initiated action. + const session = sessionRef.current; + if (!session) return { accepted: false }; + try { + return await session.enqueueMidTurnMessage(message); + } catch { + return { accepted: false }; + } + }, + async sendShellCommand(command: string) { const session = requireSessionForAction( addNotice, diff --git a/packages/webui/src/daemon/session/types.ts b/packages/webui/src/daemon/session/types.ts index ed9398058ed..8622f8fc61d 100644 --- a/packages/webui/src/daemon/session/types.ts +++ b/packages/webui/src/daemon/session/types.ts @@ -12,6 +12,7 @@ import type { DaemonApprovalModeResult, DaemonAvailableCommand, DaemonSessionBtwResult, + DaemonMidTurnMessageResult, DaemonSessionContextStatus, DaemonSessionContextUsageStatus, DaemonSessionRecapResult, @@ -280,6 +281,13 @@ export interface DaemonSessionActions { question: string, opts?: { signal?: AbortSignal }, ): Promise; + /** + * Best-effort: queue a message typed while a turn is running so the daemon + * can drain it mid-turn. Resolves `{ accepted: false }` (never throws/raises + * a notice) when there is no session, the session is idle, or the push + * fails — the caller then keeps the message in its own next-turn queue. + */ + enqueueMidTurnMessage(message: string): Promise; sendShellCommand(command: string): Promise; getTasks(): Promise; cancelTask( diff --git a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts new file mode 100644 index 00000000000..36d3149b1d9 --- /dev/null +++ b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts @@ -0,0 +1,29 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useSyncExternalStore } from 'react'; +import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; +import { + getSidechannelMidTurnInjected, + subscribeSidechannelMidTurnInjected, +} from './midTurnInjectedSidechannel.js'; + +/** + * Returns the most recently injected mid-turn batch, or `undefined` if none. + * The snapshot reference changes on every publish (even for identical text), so + * a consumer can run an effect keyed on the return value to process each batch + * exactly once — typically to drop the matching messages from its own pending + * queue so they are not resent as the next turn. + */ +export function useDaemonMidTurnInjected(): + | DaemonMidTurnMessageInjectedData + | undefined { + return useSyncExternalStore( + subscribeSidechannelMidTurnInjected, + getSidechannelMidTurnInjected, + getSidechannelMidTurnInjected, + ); +} From 951e4a4c9c090733c95ec3e9224e6fdbf2c3cb2f Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 09:39:42 +0800 Subject: [PATCH 2/7] =?UTF-8?q?fix(daemon):=20address=20mid-turn=20drain?= =?UTF-8?q?=20review=20=E2=80=94=20exactly-once=20+=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/acp-bridge/src/bridge.test.ts | 134 ++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 12 ++ packages/acp-bridge/src/bridgeClient.ts | 25 ++-- packages/acp-bridge/src/internal/testUtils.ts | 13 +- packages/cli/src/serve/server.ts | 20 ++- packages/web-shell/client/App.tsx | 46 +++--- .../web-shell/client/midTurnDedup.test.ts | 91 ++++++++++++ packages/web-shell/client/midTurnDedup.ts | 53 +++++++ .../daemon/midTurnInjectedSidechannel.test.ts | 31 ++-- .../src/daemon/midTurnInjectedSidechannel.ts | 48 ++++--- .../src/daemon/useDaemonMidTurnInjected.ts | 32 +++-- 11 files changed, 429 insertions(+), 76 deletions(-) create mode 100644 packages/web-shell/client/midTurnDedup.test.ts create mode 100644 packages/web-shell/client/midTurnDedup.ts diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 59c3f28bc11..ad092f8094a 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -10404,4 +10404,138 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa ); await bridge.shutdown(); }); + + it('drains the queue through the child connection; a second drain is empty', async () => { + let release: (() => void) | undefined; + const handle = makeChannel({ + promptImpl: async () => { + await new Promise((r) => { + release = r; + }); + return { stopReason: 'end_turn' }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const prompt = bridge + .sendPrompt( + session.sessionId, + { sessionId: session.sessionId, prompt: [{ type: 'text', text: 'go' }] }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + + bridge.enqueueMidTurnMessage(session.sessionId, 'm1'); + bridge.enqueueMidTurnMessage(session.sessionId, 'm2'); + + // The child pulls them via the ext-method the real Session calls between + // tool batches — exercising bridge queue ⇄ BridgeClient.extMethod end to end. + const drained = await handle.agentConnection.extMethod( + 'craft/drainMidTurnQueue', + { sessionId: session.sessionId }, + ); + expect(drained).toEqual({ messages: ['m1', 'm2'] }); + // Spliced out, so the next batch's drain is empty. + expect( + await handle.agentConnection.extMethod('craft/drainMidTurnQueue', { + sessionId: session.sessionId, + }), + ).toEqual({ messages: [] }); + + release?.(); + await prompt; + await bridge.shutdown(); + }); + + it('clears undrained messages at settle — not re-drained on the next turn', async () => { + const releases: Array<() => void> = []; + const handle = makeChannel({ + promptImpl: async () => { + await new Promise((r) => { + releases.push(r); + }); + return { stopReason: 'end_turn' }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const send = (text: string) => + bridge + .sendPrompt( + session.sessionId, + { sessionId: session.sessionId, prompt: [{ type: 'text', text }] }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + const drain = () => + handle.agentConnection.extMethod('craft/drainMidTurnQueue', { + sessionId: session.sessionId, + }); + + // Turn 1: enqueue, do NOT drain, then settle. + const t1 = send('t1'); + await new Promise((r) => setTimeout(r, 10)); + expect(bridge.enqueueMidTurnMessage(session.sessionId, 'leftover')).toEqual({ + accepted: true, + }); + releases[0]!(); + await t1; + + // Turn 2: the drain must be empty — the leftover was dropped at turn-1 + // settle, NOT carried into turn 2's first batch. (Deleting the settle-clear + // line makes this return ['leftover'].) + const t2 = send('t2'); + await new Promise((r) => setTimeout(r, 10)); + expect(await drain()).toEqual({ messages: [] }); + + releases[1]!(); + await t2; + await bridge.shutdown(); + }); + + it('keeps the queue across a back-to-back prompt FIFO, clearing only at true idle', async () => { + const releases: Array<() => void> = []; + const handle = makeChannel({ + promptImpl: async () => { + await new Promise((r) => { + releases.push(r); + }); + return { stopReason: 'end_turn' }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const send = (text: string) => + bridge + .sendPrompt( + session.sessionId, + { sessionId: session.sessionId, prompt: [{ type: 'text', text }] }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + + const p1 = send('p1'); + await new Promise((r) => setTimeout(r, 10)); + const p2 = send('p2'); // queued behind p1 ⇒ pendingPromptCount = 2 + expect(bridge.enqueueMidTurnMessage(session.sessionId, 'x')).toEqual({ + accepted: true, + }); + + releases[0]!(); // settle p1 — session still busy (p2 pending), so NOT idle + await new Promise((r) => setTimeout(r, 10)); + // Survived the p1→p2 boundary: the clear only fires at true idle. + expect( + await handle.agentConnection.extMethod('craft/drainMidTurnQueue', { + sessionId: session.sessionId, + }), + ).toEqual({ messages: ['x'] }); + + releases[1]!(); + await Promise.all([p1, p2]); + await bridge.shutdown(); + }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 9c119b0de92..1ad519e8dbf 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -663,6 +663,12 @@ const SESSION_RECAP_TIMEOUT_MS = 60_000; const SESSION_BTW_TIMEOUT_MS = 60_000; const SHELL_COMMAND_TIMEOUT_MS = 120_000; const MAX_SHELL_OUTPUT_FOR_HISTORY = 10_000; +// Per-session cap on undrained mid-turn messages. Mirrors the bound `/prompt` +// gets from `maxPendingPromptsPerSession`: a busy turn with no drain point (a +// long tool-free generation) must not let a client pin unbounded messages in +// the in-memory queue. Past the cap, `enqueueMidTurnMessage` returns +// `{ accepted: false }` and the browser keeps the message for the next turn. +const MAX_MID_TURN_QUEUE_DEPTH = 20; const DEFAULT_MAX_SESSIONS = 20; // Keep in sync with CLI serve/server.ts and SDK DaemonClient.ts. const DEFAULT_MAX_PENDING_PROMPTS_PER_SESSION = 5; @@ -4080,6 +4086,12 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (trimmed.length === 0 || entry.pendingPromptCount === 0) { return { accepted: false }; } + // Bound queue depth (see MAX_MID_TURN_QUEUE_DEPTH). Full → reject so the + // browser keeps the message in its own queue for the next turn rather than + // pinning it here unboundedly when the turn has no drain point. + if (entry.midTurnMessageQueue.length >= MAX_MID_TURN_QUEUE_DEPTH) { + return { accepted: false }; + } entry.midTurnMessageQueue.push(trimmed); return { accepted: true }; }, diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 95b5350d7b8..7e419548f15 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -562,15 +562,22 @@ export class BridgeClient implements Client { if (!entry) return { messages: [] }; const messages = entry.midTurnMessageQueue.splice(0); if (messages.length > 0) { - try { - entry.events.publish({ - type: MID_TURN_MESSAGE_INJECTED_EVENT, - data: { sessionId: entry.sessionId, messages }, - }); - } catch { - // Bus already closed (session tearing down). The child still receives - // the messages below; only the browser-side echo is lost. - } + // `publish()` never throws — it returns `undefined` on a closed bus (see + // EventBus.publish's never-throws contract: "Don't add try/catch wrappers + // around publish()"). Capture the result instead. A dropped frame is + // teardown-only: the child still gets the spliced messages below, but the + // browser won't receive the echo and would resend them next turn — so log + // it. One line per non-empty drain also makes the (rare) drain path and + // any duplicate-delivery mode diagnosable from the daemon stderr. + const published = entry.events.publish({ + type: MID_TURN_MESSAGE_INJECTED_EVENT, + data: { sessionId: entry.sessionId, messages }, + }); + writeStderrLine( + published + ? `[mid-turn] session=${entry.sessionId} drained=${messages.length} injected into running turn` + : `[mid-turn] session=${entry.sessionId} drained=${messages.length} echo frame dropped (bus closed); browser may resend next turn`, + ); } return { messages }; } diff --git a/packages/acp-bridge/src/internal/testUtils.ts b/packages/acp-bridge/src/internal/testUtils.ts index 03ba2dd6cd4..ad0c1dee668 100644 --- a/packages/acp-bridge/src/internal/testUtils.ts +++ b/packages/acp-bridge/src/internal/testUtils.ts @@ -235,6 +235,13 @@ export class FakeAgent implements Agent { export interface ChannelHandle { channel: AcpChannel; agent: FakeAgent; + /** + * The agent-side ACP connection. Test seam for driving the client-bound calls + * a real `qwen --acp` child makes — e.g. the mid-turn drain + * `agentConnection.extMethod('craft/drainMidTurnQueue', { sessionId })`, + * answered by the bridge's `BridgeClient.extMethod`. + */ + agentConnection: AgentSideConnection; killed: boolean; /** * Resolve `channel.exited` without going through `kill()`. Optionally @@ -277,6 +284,7 @@ export function makeChannel(opts: FakeAgentOpts = {}): ChannelHandle { const handle: ChannelHandle = { channel: undefined as unknown as AcpChannel, agent: new FakeAgent(opts), + agentConnection: undefined as unknown as AgentSideConnection, killed: false, /** Test hook: simulate an unexpected child crash. */ crash: (info?: { @@ -284,8 +292,9 @@ export function makeChannel(opts: FakeAgentOpts = {}): ChannelHandle { signalCode: NodeJS.Signals | null; }) => resolveExited!(info), }; - // Spin up the fake agent on the agent side. - new AgentSideConnection(() => handle.agent, agentStream); + // Spin up the fake agent on the agent side; keep the connection so tests can + // drive client-bound ext-methods (e.g. the mid-turn drain). + handle.agentConnection = new AgentSideConnection(() => handle.agent, agentStream); handle.channel = { stream: clientStream, exited, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index bd91c703085..043f1421bf9 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2702,9 +2702,17 @@ export function createServeApp( // Queue a user message typed while the session's turn is still running. The // ACP child drains it between tool batches (`craft/drainMidTurnQueue`) so the // model sees it before the turn ends, instead of waiting for the next turn. - // Returns `{ accepted }`: `false` when the session is idle, so the browser - // keeps the message in its own queue and sends it as a normal next-turn - // prompt. Synchronous — the bridge only pushes onto an in-memory queue. + // Returns `{ accepted }`: `false` when the session is idle (or the per-session + // queue is full), so the browser keeps the message in its own queue and sends + // it as a normal next-turn prompt. Synchronous — the bridge only pushes onto + // an in-memory queue. + // + // Per-message abuse guard. The sibling `/btw` caps its field; without this + // only the global 10 MB body limit applies. Not a UX limit — a rejected + // message stays in the browser's own queue and is sent as the (uncapped) + // next-turn prompt — it only bounds how much a single mid-turn push can pin in + // the in-memory queue (the queue DEPTH is bounded in `enqueueMidTurnMessage`). + const MID_TURN_MESSAGE_MAX_LENGTH = 16 * 1024; app.post('/session/:id/mid-turn-message', mutate(), (req, res) => { const sessionId = requireSessionId(req, res); if (sessionId === null) return; @@ -2716,6 +2724,12 @@ export function createServeApp( }); return; } + if (message.length > MID_TURN_MESSAGE_MAX_LENGTH) { + res.status(400).json({ + error: `\`message\` must be at most ${MID_TURN_MESSAGE_MAX_LENGTH} characters`, + }); + return; + } try { const result = bridge.enqueueMidTurnMessage(sessionId, message); res.status(200).json(result); diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 4b60f1011cf..d4eab293309 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -27,6 +27,7 @@ import type { DaemonSessionTaskStatus, } from '@qwen-code/sdk/daemon'; import { extractPendingPermission } from './adapters/transcriptAdapter'; +import { removeInjectedFromQueue } from './midTurnDedup'; import { MessageList, type MessageListHandle } from './components/MessageList'; import { Editor, type EditorHandle } from './components/Editor'; import type { PromptImage } from './adapters/promptTypes'; @@ -1299,32 +1300,29 @@ export function App({ }, [store, t]); // When the daemon drains queued messages into the running turn it emits - // `mid_turn_message_injected`. Drop the matching (text-only) entries from the - // local queue so the idle-time drain doesn't ALSO resend them as the next - // turn — the SSE frame is delivered in-order ahead of the turn-complete frame - // that flips streamingState to idle, so this runs before that resend fires. - const midTurnInjected = useDaemonMidTurnInjected(); + // `mid_turn_message_injected` (one frame per tool batch). Drop the matching + // (text-only) entries from the local queue so the idle-time drain doesn't ALSO + // resend them as the next turn. Reconcile EVERY accumulated batch, not just the + // newest — a multi-batch turn can publish several frames back-to-back, and the + // first must not be lost before this runs. The frames arrive in-order ahead of + // the turn-complete frame that flips streamingState to idle, so this runs + // before that resend fires; `consume()` then clears the reconciled batches. + const { batches: midTurnInjectedBatches, consume: consumeMidTurnInjected } = + useDaemonMidTurnInjected(); useEffect(() => { - if (!midTurnInjected) return; - if (midTurnInjected.sessionId !== connection.sessionId) return; - const remaining = [...queuedPromptsRef.current]; - let changed = false; - for (const message of midTurnInjected.messages) { - const index = remaining.findIndex( - (prompt) => - prompt.text === message && - (!prompt.images || prompt.images.length === 0), - ); - if (index >= 0) { - remaining.splice(index, 1); - changed = true; - } - } - if (changed) { - queuedPromptsRef.current = remaining; - setQueuedPrompts(remaining); + const sessionId = connection.sessionId; + if (!sessionId || midTurnInjectedBatches.length === 0) return; + const next = removeInjectedFromQueue( + queuedPromptsRef.current, + midTurnInjectedBatches, + sessionId, + ); + if (next) { + queuedPromptsRef.current = next; + setQueuedPrompts(next); } - }, [midTurnInjected, connection.sessionId]); + consumeMidTurnInjected(); + }, [midTurnInjectedBatches, connection.sessionId, consumeMidTurnInjected]); const handleThemeChange = useCallback( (nextTheme: WebShellTheme) => { diff --git a/packages/web-shell/client/midTurnDedup.test.ts b/packages/web-shell/client/midTurnDedup.test.ts new file mode 100644 index 00000000000..9f90859aa2b --- /dev/null +++ b/packages/web-shell/client/midTurnDedup.test.ts @@ -0,0 +1,91 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { + removeInjectedFromQueue, + type MidTurnInjectedBatch, +} from './midTurnDedup'; + +interface Item { + id: number; + text: string; + images?: unknown[]; +} + +let nextId = 1; +const q = (text: string, images?: unknown[]): Item => ({ + id: nextId++, + text, + ...(images ? { images } : {}), +}); +const batch = (sessionId: string, ...messages: string[]): MidTurnInjectedBatch => ({ + sessionId, + messages, +}); + +describe('removeInjectedFromQueue', () => { + it('removes the matching text-only entry for a single batch', () => { + const prompts = [q('keep'), q('also check tests'), q('keep2')]; + const next = removeInjectedFromQueue(prompts, [batch('s', 'also check tests')], 's'); + expect(next?.map((p) => p.text)).toEqual(['keep', 'keep2']); + }); + + it('reconciles ACROSS multiple accumulated batches (the #439 regression)', () => { + // A multi-batch turn publishes one frame per batch; both must be removed. + const prompts = [q('first'), q('second'), q('stay')]; + const next = removeInjectedFromQueue( + prompts, + [batch('s', 'first'), batch('s', 'second')], + 's', + ); + expect(next?.map((p) => p.text)).toEqual(['stay']); + }); + + it('is count-based: removes one queued entry per injected occurrence', () => { + const prompts = [q('dup'), q('dup'), q('other')]; + // one injection -> one removal + expect( + removeInjectedFromQueue(prompts, [batch('s', 'dup')], 's')?.map((p) => p.text), + ).toEqual(['dup', 'other']); + // two injections (across batches) -> both removed + expect( + removeInjectedFromQueue( + prompts, + [batch('s', 'dup'), batch('s', 'dup')], + 's', + )?.map((p) => p.text), + ).toEqual(['other']); + }); + + it('never matches an image-bearing entry (images are not pushed mid-turn)', () => { + const prompts = [q('with image', [{ data: 'x' }]), q('with image')]; + const next = removeInjectedFromQueue(prompts, [batch('s', 'with image')], 's'); + // The text-only one is removed; the image-bearing one stays. + expect(next).not.toBeNull(); + expect(next).toHaveLength(1); + expect(next?.[0].images).toEqual([{ data: 'x' }]); + }); + + it('skips batches for a different session', () => { + const prompts = [q('x')]; + expect(removeInjectedFromQueue(prompts, [batch('other', 'x')], 's')).toBeNull(); + }); + + it('returns null (no new array) when nothing matched', () => { + const prompts = [q('a'), q('b')]; + expect(removeInjectedFromQueue(prompts, [batch('s', 'missing')], 's')).toBeNull(); + expect(removeInjectedFromQueue(prompts, [], 's')).toBeNull(); + }); + + it('returns a new array, leaving the input untouched, when changed', () => { + const prompts = [q('drop'), q('keep')]; + const next = removeInjectedFromQueue(prompts, [batch('s', 'drop')], 's'); + expect(next).not.toBe(prompts); + expect(prompts).toHaveLength(2); // input not mutated + expect(next).toHaveLength(1); + }); +}); diff --git a/packages/web-shell/client/midTurnDedup.ts b/packages/web-shell/client/midTurnDedup.ts new file mode 100644 index 00000000000..005cacf1d17 --- /dev/null +++ b/packages/web-shell/client/midTurnDedup.ts @@ -0,0 +1,53 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +export interface MidTurnQueueItem { + text: string; + images?: unknown[]; +} + +export interface MidTurnInjectedBatch { + sessionId: string; + messages: readonly string[]; +} + +/** + * Reconcile injected mid-turn messages against the local pending queue: remove + * the first text-only entry matching each injected message for `sessionId`, + * across ALL `batches` (a multi-batch turn drains once per tool batch, so the + * consumer must process every accumulated batch, not just the latest). + * + * Matching is count-based — one removal per injected message — so a queue that + * holds the same text twice loses one entry per matching injection. Entries + * carrying images are never matched: image messages aren't pushed mid-turn (the + * drain channel carries plain strings), so they stay queued for the next turn. + * + * Returns a NEW array when something was removed, or `null` when nothing matched + * (so the caller can skip a redundant state update). + */ +export function removeInjectedFromQueue( + prompts: readonly T[], + batches: readonly MidTurnInjectedBatch[], + sessionId: string, +): T[] | null { + const remaining = [...prompts]; + let changed = false; + for (const batch of batches) { + if (batch.sessionId !== sessionId) continue; + for (const message of batch.messages) { + const index = remaining.findIndex( + (prompt) => + prompt.text === message && + (!prompt.images || prompt.images.length === 0), + ); + if (index >= 0) { + remaining.splice(index, 1); + changed = true; + } + } + } + return changed ? remaining : null; +} diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts index dbf4013a4fd..d8cefd51570 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts @@ -60,30 +60,37 @@ describe('parseSidechannelMidTurnInjected', () => { }); describe('mid-turn injected sidechannel pub/sub', () => { - it('notifies listeners, exposes the latest batch, and clear resets', () => { + it('ACCUMULATES batches across publishes (does not coalesce) and clear resets', () => { const listener = vi.fn(); const unsubscribe = subscribeSidechannelMidTurnInjected(listener); + expect(getSidechannelMidTurnInjected()).toEqual([]); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); expect(listener).toHaveBeenCalledTimes(1); - expect(getSidechannelMidTurnInjected()).toEqual({ - sessionId: 's-1', - messages: ['a'], - }); - // A fresh reference every publish so `useSyncExternalStore` re-fires even - // for identical text. - const first = getSidechannelMidTurnInjected(); - publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); + // Critical: a second batch published before the consumer clears must NOT + // overwrite the first — both are retained so multi-batch turns reconcile in + // full (a single-slot store would drop 'a' → 'a' resent next turn). + const afterFirst = getSidechannelMidTurnInjected(); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b', 'c'] }); expect(listener).toHaveBeenCalledTimes(2); - expect(getSidechannelMidTurnInjected()).not.toBe(first); + expect(getSidechannelMidTurnInjected()).not.toBe(afterFirst); // fresh ref + expect(getSidechannelMidTurnInjected()).toEqual([ + { sessionId: 's-1', messages: ['a'] }, + { sessionId: 's-1', messages: ['b', 'c'] }, + ]); + + clearSidechannelMidTurnInjected(); + expect(listener).toHaveBeenCalledTimes(3); + expect(getSidechannelMidTurnInjected()).toEqual([]); + // Clearing an already-empty buffer is a no-op (no spurious notify). clearSidechannelMidTurnInjected(); expect(listener).toHaveBeenCalledTimes(3); - expect(getSidechannelMidTurnInjected()).toBeUndefined(); unsubscribe(); - publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b'] }); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['d'] }); expect(listener).toHaveBeenCalledTimes(3); }); }); diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts index b5d0dc29ebe..c775cf20a17 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -7,22 +7,33 @@ import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; /** - * Side channel for `mid_turn_message_injected` daemon events. Mirrors - * {@link ./followupSidechannel.ts}: the session event pump parses the raw - * frame and publishes here, and a consumer (`useDaemonMidTurnInjected`) reads - * the latest batch via `useSyncExternalStore`. Kept out of the transcript - * reducer because it is a transient UX signal — the consumer moves the matching - * messages out of its own pending queue (so they are not resent as the next - * turn) rather than rendering anything from it. + * Side channel for `mid_turn_message_injected` daemon events. Patterned on + * {@link ./followupSidechannel.ts}, but ACCUMULATING rather than latest-wins: + * the session event pump parses each raw frame and publishes here, appending to + * a buffer that a consumer (`useDaemonMidTurnInjected`) drains via + * `useSyncExternalStore` and then clears. Kept out of the transcript reducer + * because it is a transient UX signal — the consumer moves the matching messages + * out of its own pending queue (so they are not resent as the next turn) rather + * than rendering anything from it. (Followup can be latest-wins because only the + * newest suggestion matters; mid-turn drains are cumulative — every batch must + * be reconciled or its messages get double-delivered.) */ const listeners = new Set<() => void>(); -let lastInjected: DaemonMidTurnMessageInjectedData | undefined; +// Accumulating buffer — NOT latest-wins. A turn can drain in more than one +// tool batch, so the daemon publishes one frame per non-empty drain, and the +// event pump delivers buffered frames back-to-back with no await between them. +// Two frames can therefore land before the consumer's effect runs; a single- +// slot store would drop the first, and its messages would never be removed from +// the browser's pending queue (⇒ resent next turn = the double delivery this +// feature prevents). Every batch is retained until the consumer reconciles it +// and calls `clearSidechannelMidTurnInjected`. `EMPTY` is a shared frozen ref so +// the empty snapshot is reference-stable for `useSyncExternalStore`. +const EMPTY: readonly DaemonMidTurnMessageInjectedData[] = Object.freeze([]); +let pending: readonly DaemonMidTurnMessageInjectedData[] = EMPTY; -export function getSidechannelMidTurnInjected(): - | DaemonMidTurnMessageInjectedData - | undefined { - return lastInjected; +export function getSidechannelMidTurnInjected(): readonly DaemonMidTurnMessageInjectedData[] { + return pending; } export function subscribeSidechannelMidTurnInjected( @@ -37,15 +48,18 @@ export function subscribeSidechannelMidTurnInjected( export function publishSidechannelMidTurnInjected( data: DaemonMidTurnMessageInjectedData, ): void { - // Fresh object every publish so `useSyncExternalStore` sees a new snapshot - // and re-fires consumers even when two batches carry identical text. - lastInjected = { ...data, messages: [...data.messages] }; + // Append (new array ref so `useSyncExternalStore` re-fires). Copy the batch so + // a later mutation of the source can't change what the consumer reconciles. + pending = [ + ...pending, + { sessionId: data.sessionId, messages: [...data.messages] }, + ]; notifyMidTurnInjectedListeners(); } export function clearSidechannelMidTurnInjected(): void { - if (lastInjected === undefined) return; - lastInjected = undefined; + if (pending.length === 0) return; + pending = EMPTY; notifyMidTurnInjectedListeners(); } diff --git a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts index 36d3149b1d9..11e4b58505f 100644 --- a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts +++ b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts @@ -7,23 +7,37 @@ import { useSyncExternalStore } from 'react'; import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; import { + clearSidechannelMidTurnInjected, getSidechannelMidTurnInjected, subscribeSidechannelMidTurnInjected, } from './midTurnInjectedSidechannel.js'; +export interface UseDaemonMidTurnInjectedResult { + /** + * All injected mid-turn batches accumulated since the last `consume()`, in + * arrival order. The array reference changes on every publish/consume, so a + * consumer can run an effect keyed on it to reconcile every batch (not just + * the newest) against its pending queue. + */ + batches: readonly DaemonMidTurnMessageInjectedData[]; + /** + * Clear the buffer once the batches have been reconciled. Safe to call from + * the effect body: it is synchronous, so no new frame can append between the + * read and the clear. Stable reference. + */ + consume: () => void; +} + /** - * Returns the most recently injected mid-turn batch, or `undefined` if none. - * The snapshot reference changes on every publish (even for identical text), so - * a consumer can run an effect keyed on the return value to process each batch - * exactly once — typically to drop the matching messages from its own pending - * queue so they are not resent as the next turn. + * Subscribe to injected mid-turn batches. Unlike a latest-wins signal, this + * accumulates every batch so multi-batch turns (one frame per tool batch) are + * all reconciled; the consumer calls `consume()` after processing. */ -export function useDaemonMidTurnInjected(): - | DaemonMidTurnMessageInjectedData - | undefined { - return useSyncExternalStore( +export function useDaemonMidTurnInjected(): UseDaemonMidTurnInjectedResult { + const batches = useSyncExternalStore( subscribeSidechannelMidTurnInjected, getSidechannelMidTurnInjected, getSidechannelMidTurnInjected, ); + return { batches, consume: clearSidechannelMidTurnInjected }; } From f8e801d702882ea369472221802c0b58989d2ef3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 10:04:20 +0800 Subject: [PATCH 3/7] =?UTF-8?q?fix(daemon):=20address=20Copilot=20review?= =?UTF-8?q?=20=E2=80=94=20trim=20consistency=20+=20doc=20accuracy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- packages/acp-bridge/src/bridgeClient.ts | 10 ++++++---- packages/cli/src/serve/server.ts | 10 +++++++--- packages/sdk-typescript/src/daemon/events.ts | 6 ++++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 7e419548f15..4406a4cee2f 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -208,8 +208,9 @@ const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'; /** * SSE frame published when mid-turn messages are actually drained into the * running turn. The browser consumes it to move those messages out of its - * pending queue (so they aren't resent as the next turn) and render them as - * sent. `data: { sessionId, messages: string[] }`. + * pending queue so they aren't resent as the next turn (a transient dedupe + * signal — it isn't rendered as a transcript item). + * `data: { sessionId, messages: string[] }`. */ const MID_TURN_MESSAGE_INJECTED_EVENT = 'mid_turn_message_injected'; @@ -538,8 +539,9 @@ export class BridgeClient implements Client { * pull any messages the browser queued mid-turn. We splice the per-session * queue, return them to the child as the response, and — when non-empty — * publish a `mid_turn_message_injected` SSE frame so the browser can move - * those messages out of its pending queue and render them as sent. Unknown - * methods reject with ACP `methodNotFound` (-32601), matching the SDK's + * those messages out of its pending queue (a dedupe signal, not a transcript + * render). Unknown methods reject with ACP `methodNotFound` (-32601), matching + * the SDK's * default for an unimplemented client surface; the child's drain caller * treats that as "drain unsupported" and stops asking. */ diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 043f1421bf9..7b419c9ef5d 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2718,20 +2718,24 @@ export function createServeApp( if (sessionId === null) return; const body = safeBody(req); const message = body['message']; - if (typeof message !== 'string' || message.trim().length === 0) { + // Validate (and length-check, and enqueue) the TRIMMED value — the bridge + // stores the trimmed string, so checking the raw length would reject input + // whose real content fits but is padded with whitespace. + const trimmed = typeof message === 'string' ? message.trim() : ''; + if (trimmed.length === 0) { res.status(400).json({ error: '`message` is required and must be a non-empty string', }); return; } - if (message.length > MID_TURN_MESSAGE_MAX_LENGTH) { + if (trimmed.length > MID_TURN_MESSAGE_MAX_LENGTH) { res.status(400).json({ error: `\`message\` must be at most ${MID_TURN_MESSAGE_MAX_LENGTH} characters`, }); return; } try { - const result = bridge.enqueueMidTurnMessage(sessionId, message); + const result = bridge.enqueueMidTurnMessage(sessionId, trimmed); res.status(200).json(result); } catch (err) { sendBridgeError(res, err, { diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index c5f3ff13bd6..f82752261c8 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -254,8 +254,10 @@ export interface DaemonSessionMetadataUpdatedData { /** * `mid_turn_message_injected` payload. Emitted when the daemon drains * browser-queued mid-turn messages into the running turn (web-shell mid-turn - * drain). Consumers move these messages out of their pending queue — so they - * aren't resent as the next turn — and render them as sent. + * drain). It is a transient dedupe signal, not a transcript item: consumers + * move these messages out of their pending queue so they aren't resent as the + * next turn. They are not rendered from this event — the message already reached + * the model mid-turn, and the persisted transcript shows it on reload. */ export interface DaemonMidTurnMessageInjectedData { sessionId: string; From 231ef5936820038b508bce37816ec0f732f591bc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 11:40:04 +0800 Subject: [PATCH 4/7] =?UTF-8?q?fix(daemon):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20client-ownership=20gate=20+=20per-originator=20mid-turn=20de?= =?UTF-8?q?dupe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/acp-bridge/src/bridge.test.ts | 88 ++++++++++++++++- packages/acp-bridge/src/bridge.ts | 26 +++-- packages/acp-bridge/src/bridgeClient.test.ts | 82 +++++++++++++++- packages/acp-bridge/src/bridgeClient.ts | 44 ++++++--- packages/acp-bridge/src/bridgeTypes.ts | 22 ++++- packages/cli/src/serve/server.ts | 12 ++- packages/sdk-typescript/src/daemon/events.ts | 7 ++ packages/web-shell/client/App.tsx | 14 ++- .../web-shell/client/midTurnDedup.test.ts | 97 +++++++++++++++++-- packages/web-shell/client/midTurnDedup.ts | 15 +++ .../daemon/midTurnInjectedSidechannel.test.ts | 44 ++++++++- .../src/daemon/midTurnInjectedSidechannel.ts | 18 +++- .../daemon/session/DaemonSessionProvider.tsx | 5 + packages/webui/src/daemon/session/types.ts | 8 ++ 14 files changed, 439 insertions(+), 43 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index ad092f8094a..350cf32d698 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -10420,7 +10420,10 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa const prompt = bridge .sendPrompt( session.sessionId, - { sessionId: session.sessionId, prompt: [{ type: 'text', text: 'go' }] }, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'go' }], + }, undefined, { clientId: session.clientId }, ) @@ -10478,9 +10481,11 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa // Turn 1: enqueue, do NOT drain, then settle. const t1 = send('t1'); await new Promise((r) => setTimeout(r, 10)); - expect(bridge.enqueueMidTurnMessage(session.sessionId, 'leftover')).toEqual({ - accepted: true, - }); + expect(bridge.enqueueMidTurnMessage(session.sessionId, 'leftover')).toEqual( + { + accepted: true, + }, + ); releases[0]!(); await t1; @@ -10538,4 +10543,79 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa await Promise.all([p1, p2]); await bridge.shutdown(); }); + + it('rejects a non-member client id (mirrors /prompt and /btw authorization)', async () => { + // The route forwards the client-declared id; the bridge must authorize it + // against THIS session before queuing — a token-holding client bound to + // another session must not push into this turn. The check runs before the + // idle/empty gates, so it throws even on an idle session. + const bridge = makeBridge({ + channelFactory: async () => makeChannel().channel, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(() => + bridge.enqueueMidTurnMessage(session.sessionId, 'sneaky', { + clientId: 'client-not-issued', + }), + ).toThrow(InvalidClientIdError); + await bridge.shutdown(); + }); + + it('stamps the drained injection frame with the originator client id', async () => { + // End-to-end: the trusted client id passed to enqueue is recorded on the + // queue entry and surfaces as the published frame's `originatorClientId`, so + // only that client dedupes its own pending queue (a peer must keep its copy). + let release: (() => void) | undefined; + const handle = makeChannel({ + promptImpl: async () => { + await new Promise((r) => { + release = r; + }); + return { stopReason: 'end_turn' }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const prompt = bridge + .sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'go' }], + }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + + expect( + bridge.enqueueMidTurnMessage(session.sessionId, 'hi', { + clientId: session.clientId, + }), + ).toEqual({ accepted: true }); + + // Subscribe before the drain so the live injection frame is captured. The + // hanging prompt publishes nothing in between, so it is the first frame. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const drained = await handle.agentConnection.extMethod( + 'craft/drainMidTurnQueue', + { sessionId: session.sessionId }, + ); + expect(drained).toEqual({ messages: ['hi'] }); + + const it = iter[Symbol.asyncIterator](); + const next = await it.next(); + expect(next.value?.type).toBe('mid_turn_message_injected'); + expect(next.value?.originatorClientId).toBe(session.clientId); + expect(next.value?.data).toMatchObject({ messages: ['hi'] }); + + abort.abort(); + release?.(); + await prompt; + await bridge.shutdown(); + }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 1ad519e8dbf..a7a3ff6216f 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -78,6 +78,7 @@ import type { BridgeClientRequestContext, CloseSessionOpts, AcpSessionBridge, + MidTurnQueueEntry, } from './bridgeTypes.js'; import type { BridgeOptions, BridgeTelemetry } from './bridgeOptions.js'; import { MCP_RESTART_SERVER_DEADLINE_MS } from './mcpTimeouts.js'; @@ -241,7 +242,7 @@ interface SessionEntry { * (preventing a stale next-turn re-injection) and resent by the browser as * a fresh prompt. */ - midTurnMessageQueue: string[]; + midTurnMessageQueue: MidTurnQueueEntry[]; /** * Per-session model-change FIFO. Prevents two concurrent * `applyModelServiceId` calls (e.g. simultaneous attach-with-different- @@ -663,11 +664,13 @@ const SESSION_RECAP_TIMEOUT_MS = 60_000; const SESSION_BTW_TIMEOUT_MS = 60_000; const SHELL_COMMAND_TIMEOUT_MS = 120_000; const MAX_SHELL_OUTPUT_FOR_HISTORY = 10_000; -// Per-session cap on undrained mid-turn messages. Mirrors the bound `/prompt` -// gets from `maxPendingPromptsPerSession`: a busy turn with no drain point (a -// long tool-free generation) must not let a client pin unbounded messages in -// the in-memory queue. Past the cap, `enqueueMidTurnMessage` returns -// `{ accepted: false }` and the browser keeps the message for the next turn. +// Per-session cap on undrained mid-turn messages: a busy turn with no drain +// point (a long tool-free generation) must not let a client pin unbounded +// messages in the in-memory queue. Past the cap, `enqueueMidTurnMessage` +// returns `{ accepted: false }` and the browser keeps the message for the next +// turn. Intentionally a fixed const for now; if this ever needs tuning, promote +// it to a `BridgeOptions` knob the same way `maxPendingPromptsPerSession` +// (the analogous bound `/prompt` enforces, default 5) is wired. const MAX_MID_TURN_QUEUE_DEPTH = 20; const DEFAULT_MAX_SESSIONS = 20; // Keep in sync with CLI serve/server.ts and SDK DaemonClient.ts. @@ -4071,9 +4074,16 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }; }, - enqueueMidTurnMessage(sessionId, message) { + enqueueMidTurnMessage(sessionId, message, context) { const entry = byId.get(sessionId); if (!entry) throw new SessionNotFoundError(sessionId); + // Authorize the caller against THIS session before doing anything — + // mirrors `/prompt` and `/btw`. Throws `InvalidClientIdError` when the + // client-declared id isn't bound to the session, so a token-holding + // client attached to another session can't push into this turn. Returns + // the trusted id (or undefined for anonymous callers) — recorded as the + // message's originator so the drain's SSE echo only dedupes that client. + const originatorClientId = resolveTrustedClientId(entry, context?.clientId); const trimmed = message.trim(); // Reject empty messages and — critically — messages that arrive while // the session is idle. The browser only pushes here when it believes a @@ -4092,7 +4102,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (entry.midTurnMessageQueue.length >= MAX_MID_TURN_QUEUE_DEPTH) { return { accepted: false }; } - entry.midTurnMessageQueue.push(trimmed); + entry.midTurnMessageQueue.push({ text: trimmed, originatorClientId }); return { accepted: true }; }, diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts index ad3238b2050..5ed89b089c8 100644 --- a/packages/acp-bridge/src/bridgeClient.test.ts +++ b/packages/acp-bridge/src/bridgeClient.test.ts @@ -41,6 +41,7 @@ import type { import { RequestError } from '@agentclientprotocol/sdk'; import { BridgeClient } from './bridgeClient.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; +import type { MidTurnQueueEntry } from './bridgeTypes.js'; import { CancelSentinelCollisionError } from './bridgeErrors.js'; import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; @@ -616,7 +617,7 @@ describe('BridgeClient — mid-turn queue drain (craft/drainMidTurnQueue)', () = entry: | { sessionId: string; - midTurnMessageQueue: string[]; + midTurnMessageQueue: MidTurnQueueEntry[]; events: { publish: ReturnType }; } | undefined, @@ -634,7 +635,7 @@ describe('BridgeClient — mid-turn queue drain (craft/drainMidTurnQueue)', () = const publish = vi.fn().mockReturnValue(true); const entry = { sessionId: 'sess:drain', - midTurnMessageQueue: ['first', 'second'], + midTurnMessageQueue: [{ text: 'first' }, { text: 'second' }], events: { publish }, }; const client = makeClientWithEntry('sess:drain', entry); @@ -652,13 +653,88 @@ describe('BridgeClient — mid-turn queue drain (craft/drainMidTurnQueue)', () = type: 'mid_turn_message_injected', data: { sessionId: 'sess:drain', messages: ['first', 'second'] }, }); + // Anonymous queue entries (no originator) ⇒ no `originatorClientId` on the + // frame, so every consumer reconciles it. + expect(publish.mock.calls[0][0].originatorClientId).toBeUndefined(); + }); + + it('publishes ONE frame per originator, each carrying its own originatorClientId', async () => { + // A mixed-originator drain (two clients pushed into the same window) must + // route each client its own echo so a peer can't dedupe a message it did + // not queue. Order within an originator is preserved. + const publish = vi.fn().mockReturnValue(true); + const entry = { + sessionId: 'sess:multi', + midTurnMessageQueue: [ + { text: 'a', originatorClientId: 'client-1' }, + { text: 'b', originatorClientId: 'client-2' }, + { text: 'c', originatorClientId: 'client-1' }, + ], + events: { publish }, + }; + const client = makeClientWithEntry('sess:multi', entry); + + // The child still receives the full drained set, in queue order. + const result = await client.extMethod('craft/drainMidTurnQueue', { + sessionId: 'sess:multi', + }); + expect(result).toEqual({ messages: ['a', 'b', 'c'] }); + expect(entry.midTurnMessageQueue).toEqual([]); + + // One frame per originator: client-1 gets ['a','c'], client-2 gets ['b']. + expect(publish).toHaveBeenCalledTimes(2); + const frames = publish.mock.calls.map((c) => c[0]); + const c1 = frames.find((f) => f.originatorClientId === 'client-1'); + const c2 = frames.find((f) => f.originatorClientId === 'client-2'); + expect(c1).toMatchObject({ + type: 'mid_turn_message_injected', + data: { sessionId: 'sess:multi', messages: ['a', 'c'] }, + originatorClientId: 'client-1', + }); + expect(c2).toMatchObject({ + type: 'mid_turn_message_injected', + data: { sessionId: 'sess:multi', messages: ['b'] }, + originatorClientId: 'client-2', + }); + }); + + it('still returns the drained messages to the child when the echo frame is dropped (bus closed)', async () => { + // Teardown-only degradation: `publish()` returns falsy on a closed bus. The + // child has already been handed the messages (the model sees them), but the + // browser never gets the echo — log it so the resend-next-turn window is + // diagnosable. The drain itself must NOT fail. + const publish = vi.fn().mockReturnValue(undefined); + const stderr = vi + .spyOn(process.stderr, 'write') + .mockReturnValue(true as never); + try { + const entry = { + sessionId: 'sess:closed', + midTurnMessageQueue: [{ text: 'still-delivered' }], + events: { publish }, + }; + const client = makeClientWithEntry('sess:closed', entry); + + const result = await client.extMethod('craft/drainMidTurnQueue', { + sessionId: 'sess:closed', + }); + + // (a) the child still receives the message despite the dropped echo. + expect(result).toEqual({ messages: ['still-delivered'] }); + expect(entry.midTurnMessageQueue).toEqual([]); + // (b) the dropped-echo degradation is logged. + const logged = stderr.mock.calls.map((c) => String(c[0])).join(''); + expect(logged).toContain('echo frame dropped (bus closed)'); + } finally { + stderr.mockRestore(); + } }); it('returns an empty drain and publishes nothing when the queue is empty', async () => { const publish = vi.fn().mockReturnValue(true); const entry = { sessionId: 'sess:empty', - midTurnMessageQueue: [] as string[], + midTurnMessageQueue: [] as MidTurnQueueEntry[], events: { publish }, }; const client = makeClientWithEntry('sess:empty', entry); diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 4406a4cee2f..f93f2b3d769 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -19,6 +19,7 @@ import type { } from '@agentclientprotocol/sdk'; import { RequestError } from '@agentclientprotocol/sdk'; import type { BridgeEvent, EventBus } from './eventBus.js'; +import type { MidTurnQueueEntry } from './bridgeTypes.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; // Narrowed from the concrete `MultiClientPermissionMediator` to the @@ -224,7 +225,7 @@ export interface BridgeClientSessionEntry { * `SessionEntry` in `bridge.ts`; surfaced on this narrowed view so * `extMethod` can splice it. See `SessionEntry.midTurnMessageQueue`. */ - midTurnMessageQueue: string[]; + midTurnMessageQueue: MidTurnQueueEntry[]; activePromptOriginatorClientId?: string; /** * True while the bridge drives a model roundtrip; the @@ -562,24 +563,39 @@ export class BridgeClient implements Client { if (!sessionId) return { messages: [] }; const entry = this.resolveEntry(sessionId); if (!entry) return { messages: [] }; - const messages = entry.midTurnMessageQueue.splice(0); - if (messages.length > 0) { + const drained = entry.midTurnMessageQueue.splice(0); + const messages = drained.map((item) => item.text); + if (drained.length > 0) { // `publish()` never throws — it returns `undefined` on a closed bus (see // EventBus.publish's never-throws contract: "Don't add try/catch wrappers // around publish()"). Capture the result instead. A dropped frame is // teardown-only: the child still gets the spliced messages below, but the // browser won't receive the echo and would resend them next turn — so log - // it. One line per non-empty drain also makes the (rare) drain path and - // any duplicate-delivery mode diagnosable from the daemon stderr. - const published = entry.events.publish({ - type: MID_TURN_MESSAGE_INJECTED_EVENT, - data: { sessionId: entry.sessionId, messages }, - }); - writeStderrLine( - published - ? `[mid-turn] session=${entry.sessionId} drained=${messages.length} injected into running turn` - : `[mid-turn] session=${entry.sessionId} drained=${messages.length} echo frame dropped (bus closed); browser may resend next turn`, - ); + // it. + // + // Publish ONE frame per originator client. The frame is broadcast to every + // SSE subscriber on the session, so it carries `originatorClientId` for + // clients to filter on — a peer that didn't queue the message must not + // dedupe its own coincidentally-equal entry. A mixed-originator batch (two + // clients pushing in the same window) is rare but still routed correctly. + const byOriginator = new Map(); + for (const item of drained) { + const group = byOriginator.get(item.originatorClientId); + if (group) group.push(item.text); + else byOriginator.set(item.originatorClientId, [item.text]); + } + for (const [originatorClientId, texts] of byOriginator) { + const published = entry.events.publish({ + type: MID_TURN_MESSAGE_INJECTED_EVENT, + data: { sessionId: entry.sessionId, messages: texts }, + ...(originatorClientId ? { originatorClientId } : {}), + }); + writeStderrLine( + published + ? `[mid-turn] session=${entry.sessionId} drained=${texts.length}${originatorClientId ? ` originator=${originatorClientId}` : ''} injected into running turn` + : `[mid-turn] session=${entry.sessionId} drained=${texts.length} echo frame dropped (bus closed); browser may resend next turn`, + ); + } } return { messages }; } diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index d6efb87a602..f2bf4769d5b 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -183,6 +183,17 @@ export interface BridgeHeartbeatState { clientLastSeenAt: ReadonlyMap; } +/** + * One queued mid-turn message. `originatorClientId` is the trusted client id + * that pushed it (from `resolveTrustedClientId`), carried so the drain's SSE + * echo can be routed/filtered to that client only — a peer attached to the + * same session must not dedupe a message it did not queue. + */ +export interface MidTurnQueueEntry { + text: string; + originatorClientId?: string; +} + export interface AcpSessionBridge { /** * Create a new session, or — under `sessionScope: 'single'` — attach to an @@ -487,13 +498,18 @@ export interface AcpSessionBridge { * Queue a mid-turn user message for the running turn. The ACP child drains * it between tool batches via the `craft/drainMidTurnQueue` ext-method so * the model sees it before the turn ends. Accepted only while the session - * is busy (a prompt is queued or active); an idle session returns - * `{ accepted: false }` so the caller falls back to a normal next-turn - * prompt. Throws `SessionNotFoundError` for unknown ids. + * is busy (a prompt is queued or active); an idle (or full-queue) session + * returns `{ accepted: false }` so the caller falls back to a normal + * next-turn prompt. `context.clientId` is authorized against the session + * like `/prompt` and `/btw` — throws `InvalidClientIdError` when the id is + * not bound to the session, and `SessionNotFoundError` for unknown ids. The + * trusted client id is recorded as the message's originator so the drain's + * SSE echo only dedupes that client's pending queue. */ enqueueMidTurnMessage( sessionId: string, message: string, + context?: BridgeClientRequestContext, ): { accepted: boolean }; /** diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 7b419c9ef5d..2295bb05b3e 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2734,8 +2734,18 @@ export function createServeApp( }); return; } + // Forward the client id so the bridge authorizes it against the session + // (like `/prompt` and `/btw`) — a token-holding client bound to another + // session must not push into this one — and records it as the message's + // originator for SSE echo routing. `null` = malformed id (already answered). + const clientId = parseClientIdHeader(req, res); + if (clientId === null) return; try { - const result = bridge.enqueueMidTurnMessage(sessionId, trimmed); + const result = bridge.enqueueMidTurnMessage( + sessionId, + trimmed, + clientId !== undefined ? { clientId } : undefined, + ); res.status(200).json(result); } catch (err) { sendBridgeError(res, err, { diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index f82752261c8..2b21f746d91 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -262,6 +262,13 @@ export interface DaemonSessionMetadataUpdatedData { export interface DaemonMidTurnMessageInjectedData { sessionId: string; messages: string[]; + /** + * Trusted client id that queued these messages, surfaced from the SSE + * envelope's `originatorClientId` so a consumer dedupes only its OWN pending + * queue — a peer attached to the same session must not drop a + * coincidentally-equal entry it didn't queue. Absent for anonymous pushes. + */ + originatorClientId?: string; [key: string]: unknown; } diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index d4eab293309..1d211681805 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -1312,17 +1312,29 @@ export function App({ useEffect(() => { const sessionId = connection.sessionId; if (!sessionId || midTurnInjectedBatches.length === 0) return; + // Pass OUR client id so only batches the daemon stamped with it (or + // anonymous ones) are deduped. The daemon stamps every drained frame with + // the originator's client id, and the web-shell always sends one, so + // omitting this would skip every batch — leaving our own messages in the + // queue to be resent next turn (double delivery). A peer on the same + // session keeps its own coincidentally-equal entry. const next = removeInjectedFromQueue( queuedPromptsRef.current, midTurnInjectedBatches, sessionId, + connection.clientId, ); if (next) { queuedPromptsRef.current = next; setQueuedPrompts(next); } consumeMidTurnInjected(); - }, [midTurnInjectedBatches, connection.sessionId, consumeMidTurnInjected]); + }, [ + midTurnInjectedBatches, + connection.sessionId, + connection.clientId, + consumeMidTurnInjected, + ]); const handleThemeChange = useCallback( (nextTheme: WebShellTheme) => { diff --git a/packages/web-shell/client/midTurnDedup.test.ts b/packages/web-shell/client/midTurnDedup.test.ts index 9f90859aa2b..08a9b7daffd 100644 --- a/packages/web-shell/client/midTurnDedup.test.ts +++ b/packages/web-shell/client/midTurnDedup.test.ts @@ -22,15 +22,27 @@ const q = (text: string, images?: unknown[]): Item => ({ text, ...(images ? { images } : {}), }); -const batch = (sessionId: string, ...messages: string[]): MidTurnInjectedBatch => ({ +const batch = ( + sessionId: string, + ...messages: string[] +): MidTurnInjectedBatch => ({ sessionId, messages, }); +const batchFrom = ( + sessionId: string, + originatorClientId: string, + ...messages: string[] +): MidTurnInjectedBatch => ({ sessionId, originatorClientId, messages }); describe('removeInjectedFromQueue', () => { it('removes the matching text-only entry for a single batch', () => { const prompts = [q('keep'), q('also check tests'), q('keep2')]; - const next = removeInjectedFromQueue(prompts, [batch('s', 'also check tests')], 's'); + const next = removeInjectedFromQueue( + prompts, + [batch('s', 'also check tests')], + 's', + ); expect(next?.map((p) => p.text)).toEqual(['keep', 'keep2']); }); @@ -49,7 +61,9 @@ describe('removeInjectedFromQueue', () => { const prompts = [q('dup'), q('dup'), q('other')]; // one injection -> one removal expect( - removeInjectedFromQueue(prompts, [batch('s', 'dup')], 's')?.map((p) => p.text), + removeInjectedFromQueue(prompts, [batch('s', 'dup')], 's')?.map( + (p) => p.text, + ), ).toEqual(['dup', 'other']); // two injections (across batches) -> both removed expect( @@ -63,7 +77,11 @@ describe('removeInjectedFromQueue', () => { it('never matches an image-bearing entry (images are not pushed mid-turn)', () => { const prompts = [q('with image', [{ data: 'x' }]), q('with image')]; - const next = removeInjectedFromQueue(prompts, [batch('s', 'with image')], 's'); + const next = removeInjectedFromQueue( + prompts, + [batch('s', 'with image')], + 's', + ); // The text-only one is removed; the image-bearing one stays. expect(next).not.toBeNull(); expect(next).toHaveLength(1); @@ -72,12 +90,16 @@ describe('removeInjectedFromQueue', () => { it('skips batches for a different session', () => { const prompts = [q('x')]; - expect(removeInjectedFromQueue(prompts, [batch('other', 'x')], 's')).toBeNull(); + expect( + removeInjectedFromQueue(prompts, [batch('other', 'x')], 's'), + ).toBeNull(); }); it('returns null (no new array) when nothing matched', () => { const prompts = [q('a'), q('b')]; - expect(removeInjectedFromQueue(prompts, [batch('s', 'missing')], 's')).toBeNull(); + expect( + removeInjectedFromQueue(prompts, [batch('s', 'missing')], 's'), + ).toBeNull(); expect(removeInjectedFromQueue(prompts, [], 's')).toBeNull(); }); @@ -88,4 +110,67 @@ describe('removeInjectedFromQueue', () => { expect(prompts).toHaveLength(2); // input not mutated expect(next).toHaveLength(1); }); + + // The daemon stamps each drained frame with the originator's client id and + // broadcasts it to every client on the session. Only the originator should + // dedupe its own queue; a peer with a coincidentally-equal entry must keep it. + describe('originator (clientId) filtering', () => { + it('dedupes a batch whose originator matches our client id', () => { + const prompts = [q('mine'), q('keep')]; + const next = removeInjectedFromQueue( + prompts, + [batchFrom('s', 'me', 'mine')], + 's', + 'me', + ); + expect(next?.map((p) => p.text)).toEqual(['keep']); + }); + + it('skips a batch originated by a DIFFERENT client (no spurious dedupe)', () => { + // A peer pushed 'shared'; our identical queue entry was never injected on + // our side, so it must survive to be sent as our own next turn. + const prompts = [q('shared')]; + expect( + removeInjectedFromQueue( + prompts, + [batchFrom('s', 'peer', 'shared')], + 's', + 'me', + ), + ).toBeNull(); + }); + + it('dedupes an anonymous batch (no originator) regardless of our client id', () => { + const prompts = [q('anon'), q('keep')]; + const next = removeInjectedFromQueue( + prompts, + [batch('s', 'anon')], + 's', + 'me', + ); + expect(next?.map((p) => p.text)).toEqual(['keep']); + }); + + it('routes a mixed-originator set: ours dedupes, the peer’s is skipped', () => { + const prompts = [q('mine'), q('theirs'), q('keep')]; + const next = removeInjectedFromQueue( + prompts, + [batchFrom('s', 'me', 'mine'), batchFrom('s', 'peer', 'theirs')], + 's', + 'me', + ); + expect(next?.map((p) => p.text)).toEqual(['theirs', 'keep']); + }); + + it('skips our OWN-tagged batch when no client id is supplied (regression guard)', () => { + // If the caller forgets to pass its client id, an originator-tagged batch + // must NOT be force-deduped — but it also won't be reconciled, surfacing + // the wiring gap rather than silently double-delivering. (The web-shell + // always passes connection.clientId; this pins the helper's contract.) + const prompts = [q('mine')]; + expect( + removeInjectedFromQueue(prompts, [batchFrom('s', 'me', 'mine')], 's'), + ).toBeNull(); + }); + }); }); diff --git a/packages/web-shell/client/midTurnDedup.ts b/packages/web-shell/client/midTurnDedup.ts index 005cacf1d17..9816697774a 100644 --- a/packages/web-shell/client/midTurnDedup.ts +++ b/packages/web-shell/client/midTurnDedup.ts @@ -12,6 +12,8 @@ export interface MidTurnQueueItem { export interface MidTurnInjectedBatch { sessionId: string; messages: readonly string[]; + /** Trusted client id that queued the messages (from the SSE envelope). */ + originatorClientId?: string; } /** @@ -25,6 +27,12 @@ export interface MidTurnInjectedBatch { * carrying images are never matched: image messages aren't pushed mid-turn (the * drain channel carries plain strings), so they stay queued for the next turn. * + * Skips a batch whose `originatorClientId` is some OTHER client: the daemon + * broadcasts the injection frame to every client on the session, but only the + * client that queued the message should drop it — a peer with a coincidentally + * equal text must keep its own entry. Batches with no originator (anonymous + * push) are reconciled regardless. + * * Returns a NEW array when something was removed, or `null` when nothing matched * (so the caller can skip a redundant state update). */ @@ -32,11 +40,18 @@ export function removeInjectedFromQueue( prompts: readonly T[], batches: readonly MidTurnInjectedBatch[], sessionId: string, + clientId?: string, ): T[] | null { const remaining = [...prompts]; let changed = false; for (const batch of batches) { if (batch.sessionId !== sessionId) continue; + if ( + batch.originatorClientId !== undefined && + batch.originatorClientId !== clientId + ) { + continue; + } for (const message of batch.messages) { const index = remaining.findIndex( (prompt) => diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts index d8cefd51570..b870c1f47fc 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts @@ -27,6 +27,32 @@ describe('parseSidechannelMidTurnInjected', () => { ).toEqual({ sessionId: 's-1', messages: ['hi', 'there'] }); }); + it('lifts originatorClientId off the envelope top-level (not data)', () => { + // The daemon publishes one frame per originator and carries the id on the + // SSE envelope, not inside `data`, so consumers dedupe only their own queue. + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + originatorClientId: 'client-7', + data: { sessionId: 's-1', messages: ['hi'] }, + }), + ).toEqual({ + sessionId: 's-1', + messages: ['hi'], + originatorClientId: 'client-7', + }); + }); + + it('omits originatorClientId when absent or non-string (anonymous push)', () => { + expect( + parseSidechannelMidTurnInjected({ + type: 'mid_turn_message_injected', + originatorClientId: 42, + data: { sessionId: 's-1', messages: ['hi'] }, + }), + ).toEqual({ sessionId: 's-1', messages: ['hi'] }); + }); + it('filters non-string and empty entries', () => { expect( parseSidechannelMidTurnInjected({ @@ -73,7 +99,10 @@ describe('mid-turn injected sidechannel pub/sub', () => { // overwrite the first — both are retained so multi-batch turns reconcile in // full (a single-slot store would drop 'a' → 'a' resent next turn). const afterFirst = getSidechannelMidTurnInjected(); - publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b', 'c'] }); + publishSidechannelMidTurnInjected({ + sessionId: 's-1', + messages: ['b', 'c'], + }); expect(listener).toHaveBeenCalledTimes(2); expect(getSidechannelMidTurnInjected()).not.toBe(afterFirst); // fresh ref expect(getSidechannelMidTurnInjected()).toEqual([ @@ -93,4 +122,17 @@ describe('mid-turn injected sidechannel pub/sub', () => { publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['d'] }); expect(listener).toHaveBeenCalledTimes(3); }); + + it('retains per-batch originatorClientId through publish/accumulate', () => { + publishSidechannelMidTurnInjected({ + sessionId: 's-1', + messages: ['a'], + originatorClientId: 'client-1', + }); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b'] }); + expect(getSidechannelMidTurnInjected()).toEqual([ + { sessionId: 's-1', messages: ['a'], originatorClientId: 'client-1' }, + { sessionId: 's-1', messages: ['b'] }, + ]); + }); }); diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts index c775cf20a17..d24228007d5 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -52,7 +52,13 @@ export function publishSidechannelMidTurnInjected( // a later mutation of the source can't change what the consumer reconciles. pending = [ ...pending, - { sessionId: data.sessionId, messages: [...data.messages] }, + { + sessionId: data.sessionId, + messages: [...data.messages], + ...(data.originatorClientId + ? { originatorClientId: data.originatorClientId } + : {}), + }, ]; notifyMidTurnInjectedListeners(); } @@ -94,5 +100,13 @@ export function parseSidechannelMidTurnInjected( typeof message === 'string' && message.length > 0, ); if (stringMessages.length === 0) return undefined; - return { sessionId, messages: stringMessages }; + // `originatorClientId` lives on the SSE envelope (top-level), not in `data` — + // the daemon publishes one frame per originator so consumers dedupe only + // their own queue. + const originatorClientId = record['originatorClientId']; + return { + sessionId, + messages: stringMessages, + ...(typeof originatorClientId === 'string' ? { originatorClientId } : {}), + }; } diff --git a/packages/webui/src/daemon/session/DaemonSessionProvider.tsx b/packages/webui/src/daemon/session/DaemonSessionProvider.tsx index c2ed96b6d4c..6342ad44578 100644 --- a/packages/webui/src/daemon/session/DaemonSessionProvider.tsx +++ b/packages/webui/src/daemon/session/DaemonSessionProvider.tsx @@ -482,6 +482,11 @@ export function DaemonSessionProvider({ setConnection((current) => ({ status: 'connected', sessionId: activeSession.sessionId, + // Surface the bound client id so consumers can recognize their own + // originator-stamped frames (e.g. the web-shell's mid-turn dedupe). + ...(activeSession.clientId + ? { clientId: activeSession.clientId } + : {}), workspaceCwd: activeSession.workspaceCwd, commands, skills, diff --git a/packages/webui/src/daemon/session/types.ts b/packages/webui/src/daemon/session/types.ts index 8622f8fc61d..48b1322de91 100644 --- a/packages/webui/src/daemon/session/types.ts +++ b/packages/webui/src/daemon/session/types.ts @@ -42,6 +42,14 @@ export type DaemonConnectionStatus = export interface DaemonConnectionState { status: DaemonConnectionStatus; sessionId?: string; + /** + * Daemon-confirmed client identity bound to this session (the value sent as + * `X-Qwen-Client-Id`). Consumers use it to recognize their OWN + * originator-stamped frames — e.g. the web-shell dedupes a + * `mid_turn_message_injected` batch only when its `originatorClientId` + * matches this id (a peer on the same session must keep its own entry). + */ + clientId?: string; workspaceCwd?: string; commands?: DaemonCommandInfo[]; skills?: string[]; From f7940065ad1bfa18b712ea877be87deeb3373619 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 12:11:43 +0800 Subject: [PATCH 5/7] =?UTF-8?q?fix(daemon):=20address=20review=20round=202?= =?UTF-8?q?=20=E2=80=94=20mid-turn=20races,=20route=20tests,=20observabili?= =?UTF-8?q?ty?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- packages/acp-bridge/src/bridge.test.ts | 71 ++++++++++ packages/acp-bridge/src/bridge.ts | 22 ++- packages/acp-bridge/src/bridgeClient.ts | 11 +- packages/acp-bridge/src/bridgeTypes.ts | 12 ++ .../src/acp-integration/session/Session.ts | 4 +- packages/cli/src/serve/server.test.ts | 130 ++++++++++++++++++ packages/cli/src/serve/server.ts | 2 +- packages/web-shell/client/App.tsx | 29 +++- .../daemon/midTurnInjectedSidechannel.test.ts | 33 +++++ .../src/daemon/midTurnInjectedSidechannel.ts | 27 +++- packages/webui/src/daemon/session/actions.ts | 13 +- packages/webui/src/daemon/session/types.ts | 5 +- .../src/daemon/useDaemonMidTurnInjected.ts | 21 ++- 13 files changed, 355 insertions(+), 25 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 350cf32d698..3025bdc7ad1 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -10618,4 +10618,75 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa await prompt; await bridge.shutdown(); }); + + it('rejects past MAX_MID_TURN_QUEUE_DEPTH (20) — the DoS bound', async () => { + const { factory, release } = hangingPromptFactory(); + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const promptPromise = bridge + .sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'go' }], + }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + + // First 20 accepted, 21st rejected (browser keeps it for the next turn). + for (let i = 0; i < 20; i++) { + expect(bridge.enqueueMidTurnMessage(session.sessionId, `m${i}`)).toEqual({ + accepted: true, + }); + } + expect(bridge.enqueueMidTurnMessage(session.sessionId, 'overflow')).toEqual( + { accepted: false }, + ); + + release(); + await promptPromise; + await bridge.shutdown(); + }); + + it('trims the message before queuing (drain returns the trimmed text)', async () => { + let release: (() => void) | undefined; + const handle = makeChannel({ + promptImpl: async () => { + await new Promise((r) => { + release = r; + }); + return { stopReason: 'end_turn' }; + }, + }); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const prompt = bridge + .sendPrompt( + session.sessionId, + { + sessionId: session.sessionId, + prompt: [{ type: 'text', text: 'go' }], + }, + undefined, + { clientId: session.clientId }, + ) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + + expect( + bridge.enqueueMidTurnMessage(session.sessionId, ' hello '), + ).toEqual({ accepted: true }); + const drained = await handle.agentConnection.extMethod( + 'craft/drainMidTurnQueue', + { sessionId: session.sessionId }, + ); + expect(drained).toEqual({ messages: ['hello'] }); + + release?.(); + await prompt; + await bridge.shutdown(); + }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index a7a3ff6216f..35243533b48 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -2980,6 +2980,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { entry.pendingPromptCount === 0 && entry.midTurnMessageQueue.length > 0 ) { + // One line when we actually drop something — makes the + // "queued-but-never-drained, browser will resend" path visible. + writeStderrLine( + `[mid-turn] session=${entry.sessionId} dropped ${entry.midTurnMessageQueue.length} undrained message(s) at idle; browser resends next turn`, + ); entry.midTurnMessageQueue.length = 0; } }) @@ -4083,7 +4088,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // client attached to another session can't push into this turn. Returns // the trusted id (or undefined for anonymous callers) — recorded as the // message's originator so the drain's SSE echo only dedupes that client. - const originatorClientId = resolveTrustedClientId(entry, context?.clientId); + const originatorClientId = resolveTrustedClientId( + entry, + context?.clientId, + ); const trimmed = message.trim(); // Reject empty messages and — critically — messages that arrive while // the session is idle. The browser only pushes here when it believes a @@ -4094,12 +4102,24 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // double delivery. Rejecting keeps the browser's next-turn fallback the // single delivery path in that race. if (trimmed.length === 0 || entry.pendingPromptCount === 0) { + // Rejects are low-volume (the browser only pushes when it believes a + // turn is running) but the silent path made "why wasn't my mid-turn + // message injected?" undiagnosable. Empty is a client bug; idle is the + // settle-window race the browser recovers from via its next-turn queue. + writeStderrLine( + `[mid-turn] session=${entry.sessionId} rejected: ${ + trimmed.length === 0 ? 'empty message' : 'session idle' + }; browser keeps it for next turn`, + ); return { accepted: false }; } // Bound queue depth (see MAX_MID_TURN_QUEUE_DEPTH). Full → reject so the // browser keeps the message in its own queue for the next turn rather than // pinning it here unboundedly when the turn has no drain point. if (entry.midTurnMessageQueue.length >= MAX_MID_TURN_QUEUE_DEPTH) { + writeStderrLine( + `[mid-turn] session=${entry.sessionId} rejected: queue full (depth ${entry.midTurnMessageQueue.length} >= ${MAX_MID_TURN_QUEUE_DEPTH}); browser keeps it for next turn`, + ); return { accepted: false }; } entry.midTurnMessageQueue.push({ text: trimmed, originatorClientId }); diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index f93f2b3d769..a64a8826aec 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -19,6 +19,7 @@ import type { } from '@agentclientprotocol/sdk'; import { RequestError } from '@agentclientprotocol/sdk'; import type { BridgeEvent, EventBus } from './eventBus.js'; +import { MID_TURN_QUEUE_DRAIN_METHOD } from './bridgeTypes.js'; import type { MidTurnQueueEntry } from './bridgeTypes.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; @@ -197,14 +198,8 @@ function sliceLineRange( * `SessionEntry` is required to provide them — TS enforces the * structural match at the callback signature). */ -/** - * ACP ext-method the spawned `qwen --acp` child calls between tool batches to - * pull user messages the browser queued mid-turn. Must match the constant in - * `cli/src/acp-integration/session/Session.ts` (the caller). The desktop ACP - * client answers the same method from its own in-memory queue; in `qwen serve` - * the daemon answers it here, from `SessionEntry.midTurnMessageQueue`. - */ -const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'; +// `MID_TURN_QUEUE_DRAIN_METHOD` is imported from `./bridgeTypes.js` (the single +// source of truth shared with the child-side caller in `Session.ts`). /** * SSE frame published when mid-turn messages are actually drained into the diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index f2bf4769d5b..e001f966d9f 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -183,6 +183,18 @@ export interface BridgeHeartbeatState { clientLastSeenAt: ReadonlyMap; } +/** + * ACP ext-method the spawned `qwen --acp` child calls between tool batches to + * pull user messages the browser queued mid-turn. The child-side caller + * (`cli/src/acp-integration/session/Session.ts`) and the daemon-side answerer + * (`bridgeClient.ts`) both import THIS single definition, so a rename can't + * silently desync them into a runtime `-32601 methodNotFound` (which would + * latch the drain off for the session). The desktop ACP client answers the same + * method from its own in-memory queue; in `qwen serve` the daemon answers it + * from `SessionEntry.midTurnMessageQueue`. + */ +export const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'; + /** * One queued mid-turn message. `originatorClientId` is the trusted client id * that pushed it (from `resolveTrustedClientId`), carried so the drain's SSE diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index f53f87c67cc..12d791d8c8f 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -97,6 +97,9 @@ import { dedupeToolCallsById, } from '@qwen-code/qwen-code-core'; import { NOT_CURRENTLY_GENERATING_CANCEL_MESSAGE } from '@qwen-code/acp-bridge/bridgeErrors'; +// Single source of truth shared with the daemon-side answerer (BridgeClient), +// so a rename can't desync caller and answerer into a silent -32601 latch. +import { MID_TURN_QUEUE_DRAIN_METHOD } from '@qwen-code/acp-bridge/bridgeTypes'; import { getCommandSubcommandNames } from '../../services/commandMetadata.js'; import { getEffectiveSupportedModes } from '../../services/commandUtils.js'; @@ -168,7 +171,6 @@ type AutoCompressionSendResult = | { responseStream: AsyncGenerator; stopReason?: never } | { responseStream: null; stopReason: PromptResponse['stopReason'] }; -const MID_TURN_QUEUE_DRAIN_METHOD = 'craft/drainMidTurnQueue'; // The drain is served from an in-memory queue, so a conforming client answers // near-instantly (or rejects with -32601). No response within this window // means the client silently drops unknown methods; without a deadline the diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index a3c25ca00e1..45bc2960a5e 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -272,6 +272,15 @@ interface FakeBridgeOpts { * validator returns 400. Defaults to an empty set. */ knownClientIds?: Iterable; + /** + * Drives the `POST /session/:id/mid-turn-message` route. Default accepts. + * Throw (e.g. `SessionNotFoundError`) to exercise the error branch. + */ + enqueueMidTurnImpl?: ( + sessionId: string, + message: string, + context?: BridgeClientRequestContext, + ) => { accepted: boolean }; spawnImpl?: (req: BridgeSpawnRequest) => Promise; loadImpl?: ( req: BridgeRestoreSessionRequest, @@ -458,6 +467,11 @@ interface FakeBridge extends AcpSessionBridge { opts?: { requireZeroAttaches?: boolean }; }>; detachCalls: Array<{ sessionId: string; clientId?: string }>; + enqueueMidTurnCalls: Array<{ + sessionId: string; + message: string; + context?: BridgeClientRequestContext; + }>; permissionVotes: Array<{ requestId: string; response: RequestPermissionResponse; @@ -567,6 +581,9 @@ function fakeBridge(opts: FakeBridgeOpts = {}): FakeBridge { opts?: { requireZeroAttaches?: boolean }; }> = []; const detachCalls: FakeBridge['detachCalls'] = []; + const enqueueMidTurnCalls: FakeBridge['enqueueMidTurnCalls'] = []; + const enqueueMidTurnImpl = + opts.enqueueMidTurnImpl ?? (() => ({ accepted: true })); const permissionVotes: FakeBridge['permissionVotes'] = []; const sessionPermissionVotes: FakeBridge['sessionPermissionVotes'] = []; const listCalls: string[] = []; @@ -889,6 +906,7 @@ function fakeBridge(opts: FakeBridgeOpts = {}): FakeBridge { cancelCalls, killCalls, detachCalls, + enqueueMidTurnCalls, permissionVotes, sessionPermissionVotes, listCalls, @@ -1111,6 +1129,14 @@ function fakeBridge(opts: FakeBridgeOpts = {}): FakeBridge { async generateSessionBtw(sessionId, _question, _signal, _context) { return { sessionId, answer: 'mock btw answer' }; }, + enqueueMidTurnMessage(sessionId, message, context) { + enqueueMidTurnCalls.push({ + sessionId, + message, + ...(context ? { context } : {}), + }); + return enqueueMidTurnImpl(sessionId, message, context); + }, async executeShellCommand(sessionId, command, signal, context) { shellCalls.push({ sessionId, @@ -2460,6 +2486,110 @@ describe('createServeApp', () => { }); }); + describe('POST /session/:id/mid-turn-message', () => { + const midTurnPost = ( + app: ReturnType, + sessionId: string, + body: Record, + clientId?: string, + ) => { + const r = request(app) + .post(`/session/${sessionId}/mid-turn-message`) + .set('Host', `127.0.0.1:${baseOpts.port}`) + .set('Authorization', 'Bearer secret'); + if (clientId !== undefined) r.set('X-Qwen-Client-Id', clientId); + return r.send(body); + }; + const midTurnApp = (bridge: FakeBridge) => + createServeApp( + { ...baseOpts, token: 'secret', workspace: WS_BOUND }, + undefined, + { bridge }, + ); + + it('200 { accepted: true } and forwards the trimmed message + client id', async () => { + const bridge = fakeBridge(); + const res = await midTurnPost( + midTurnApp(bridge), + 's-1', + { message: ' hello ' }, + 'client-9', + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ accepted: true }); + // Trimmed before enqueue, and the client id is forwarded for the bridge's + // ownership check + originator stamping. + expect(bridge.enqueueMidTurnCalls).toEqual([ + { + sessionId: 's-1', + message: 'hello', + context: { clientId: 'client-9' }, + }, + ]); + }); + + it('200 { accepted: false } when the bridge rejects (idle session)', async () => { + const bridge = fakeBridge({ + enqueueMidTurnImpl: () => ({ accepted: false }), + }); + const res = await midTurnPost(midTurnApp(bridge), 's-1', { + message: 'later', + }); + expect(res.status).toBe(200); + expect(res.body).toEqual({ accepted: false }); + }); + + it('400 when `message` is missing', async () => { + const bridge = fakeBridge(); + const res = await midTurnPost(midTurnApp(bridge), 's-1', {}); + expect(res.status).toBe(400); + expect(bridge.enqueueMidTurnCalls).toEqual([]); + }); + + it('400 when `message` is whitespace-only', async () => { + const bridge = fakeBridge(); + const res = await midTurnPost(midTurnApp(bridge), 's-1', { + message: ' ', + }); + expect(res.status).toBe(400); + expect(bridge.enqueueMidTurnCalls).toEqual([]); + }); + + it('400 when the trimmed message exceeds the 16 KB cap', async () => { + const bridge = fakeBridge(); + const res = await midTurnPost(midTurnApp(bridge), 's-1', { + message: 'x'.repeat(16 * 1024 + 1), + }); + expect(res.status).toBe(400); + expect(bridge.enqueueMidTurnCalls).toEqual([]); + }); + + it('maps a bridge SessionNotFoundError to 404', async () => { + const bridge = fakeBridge({ + enqueueMidTurnImpl: (sessionId) => { + throw new SessionNotFoundError(sessionId); + }, + }); + const res = await midTurnPost(midTurnApp(bridge), 'missing', { + message: 'hi', + }); + expect(res.status).toBe(404); + expect(res.body.sessionId).toBe('missing'); + }); + + it('400 on a malformed X-Qwen-Client-Id (never reaches the bridge)', async () => { + const bridge = fakeBridge(); + const res = await midTurnPost( + midTurnApp(bridge), + 's-1', + { message: 'hi' }, + 'bad client id with spaces', + ); + expect(res.status).toBe(400); + expect(bridge.enqueueMidTurnCalls).toEqual([]); + }); + }); + describe('host allowlist (loopback bind)', () => { it('rejects requests with an unrelated Host header', async () => { const app = createServeApp(baseOpts); diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 2295bb05b3e..2a226f9928f 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -675,7 +675,7 @@ function resolveDaemonTelemetryRoute( return { route: 'POST /sessions/delete' }; } const sessionAction = path.match( - /^\/session\/([^/]+)\/(load|resume|prompt|cancel|recap|btw|model|shell|detach|rewind|approval-mode|language|a2ui-action)$/, + /^\/session\/([^/]+)\/(load|resume|prompt|cancel|recap|btw|mid-turn-message|model|shell|detach|rewind|approval-mode|language|a2ui-action)$/, ); const sessionActionId = sessionAction?.[1]; const sessionActionName = sessionAction?.[2]; diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 1d211681805..c164798450b 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -775,6 +775,11 @@ export function App({ const nextRecapMessageIdRef = useRef(1); const nextBtwMessageIdRef = useRef(1); const btwAbortControllerRef = useRef(null); + // Scopes the best-effort mid-turn enqueue POST(s) to the turn they were typed + // in. Aborted when the turn settles so a slow/late push can't arrive during a + // SUBSEQUENT turn (e.g. the browser's own next-turn resend of the same text) + // and get injected there — cross-turn double delivery. + const midTurnEnqueueAbortRef = useRef(null); const activeSessionIdRef = useRef(connection.sessionId); const displayMessages = useMemo(() => { const localMessages = [recapMessage].filter( @@ -1268,13 +1273,35 @@ export function App({ // the effect below removes from the queue so it isn't resent. If idle / // unsupported / failed, the message harmlessly stays queued. if (!hasImages) { - void sessionActions.enqueueMidTurnMessage(trimmed); + // One controller per turn: all mid-turn pushes typed during the same + // turn share it, and the settle effect aborts it so a late arrival + // can't land in the next turn. An aborted push resolves + // `{ accepted: false }`, so the message simply stays queued and is + // resent next turn — exactly-once preserved. + let abort = midTurnEnqueueAbortRef.current; + if (!abort) { + abort = new AbortController(); + midTurnEnqueueAbortRef.current = abort; + } + void sessionActions.enqueueMidTurnMessage(trimmed, { + signal: abort.signal, + }); } return true; }, [sessionActions], ); + // When the turn settles, abort any still-in-flight mid-turn push so it can't + // arrive during the next turn and be injected twice (see midTurnEnqueueAbortRef). + // The aborted push resolves `{ accepted: false }`; the message is already in + // queuedPrompts and follows the normal next-turn path. + useEffect(() => { + if (streamingState !== 'idle') return; + midTurnEnqueueAbortRef.current?.abort(); + midTurnEnqueueAbortRef.current = null; + }, [streamingState]); + const popNextQueuedPrompt = useCallback((): QueuedPrompt | null => { const [nextPrompt, ...rest] = queuedPromptsRef.current; if (!nextPrompt) return null; diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts index b870c1f47fc..9fe9af3e445 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts @@ -135,4 +135,37 @@ describe('mid-turn injected sidechannel pub/sub', () => { { sessionId: 's-1', messages: ['b'] }, ]); }); + + it('compare-and-swap clear preserves a batch appended after the snapshot', () => { + // Models the render→effect race: the consumer reconciles `snapshot`, but a + // new frame appends before its clear runs. A snapshot-scoped clear must NOT + // wipe the new batch (else it is resent next turn = double delivery). + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); + const snapshot = getSidechannelMidTurnInjected(); + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b'] }); // races in + + clearSidechannelMidTurnInjected(snapshot); // stale ref ⇒ no-op + expect(getSidechannelMidTurnInjected()).toEqual([ + { sessionId: 's-1', messages: ['a'] }, + { sessionId: 's-1', messages: ['b'] }, + ]); + + // Clearing with the CURRENT ref succeeds. + clearSidechannelMidTurnInjected(getSidechannelMidTurnInjected()); + expect(getSidechannelMidTurnInjected()).toEqual([]); + }); + + it('caps the buffer, evicting oldest batches past the limit', () => { + for (let i = 0; i < 70; i++) { + publishSidechannelMidTurnInjected({ + sessionId: 's-1', + messages: [`m${i}`], + }); + } + const buf = getSidechannelMidTurnInjected(); + expect(buf).toHaveLength(64); // MAX_PENDING_BATCHES + // Oldest (m0..m5) evicted; newest retained. + expect(buf[0].messages).toEqual(['m6']); + expect(buf[buf.length - 1].messages).toEqual(['m69']); + }); }); diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts index d24228007d5..b2f92d7dae8 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -30,6 +30,11 @@ const listeners = new Set<() => void>(); // and calls `clearSidechannelMidTurnInjected`. `EMPTY` is a shared frozen ref so // the empty snapshot is reference-stable for `useSyncExternalStore`. const EMPTY: readonly DaemonMidTurnMessageInjectedData[] = Object.freeze([]); +// Safety cap so an orphaned buffer (consumer unmounted or session switched +// without ever consuming) can't grow without bound. Past the cap the OLDEST +// batch is evicted: under that much un-reconciled backlog the consumer is gone, +// so there is no pending queue left to double-deliver into. +const MAX_PENDING_BATCHES = 64; let pending: readonly DaemonMidTurnMessageInjectedData[] = EMPTY; export function getSidechannelMidTurnInjected(): readonly DaemonMidTurnMessageInjectedData[] { @@ -50,7 +55,7 @@ export function publishSidechannelMidTurnInjected( ): void { // Append (new array ref so `useSyncExternalStore` re-fires). Copy the batch so // a later mutation of the source can't change what the consumer reconciles. - pending = [ + const appended = [ ...pending, { sessionId: data.sessionId, @@ -60,10 +65,28 @@ export function publishSidechannelMidTurnInjected( : {}), }, ]; + pending = + appended.length > MAX_PENDING_BATCHES + ? appended.slice(appended.length - MAX_PENDING_BATCHES) + : appended; notifyMidTurnInjectedListeners(); } -export function clearSidechannelMidTurnInjected(): void { +/** + * Clear the buffer. Pass `expected` (the exact snapshot the consumer just + * reconciled) for a compare-and-swap clear: `useDaemonMidTurnInjected` reads + * `pending` during render and reconciles it in an async effect, so a new frame + * can append in the window between the read and the effect. Clearing + * unconditionally would wipe that newly-arrived batch before it is reconciled — + * its messages stay in the browser's pending queue and are resent next turn (the + * double delivery this feature exists to prevent). When a frame arrived, + * `pending !== expected`, so we no-op and leave it for the next reconcile. + * Omitting `expected` clears unconditionally (e.g. test teardown). + */ +export function clearSidechannelMidTurnInjected( + expected?: readonly DaemonMidTurnMessageInjectedData[], +): void { + if (expected !== undefined && pending !== expected) return; if (pending.length === 0) return; pending = EMPTY; notifyMidTurnInjectedListeners(); diff --git a/packages/webui/src/daemon/session/actions.ts b/packages/webui/src/daemon/session/actions.ts index ec1771c9af8..49db5085ff5 100644 --- a/packages/webui/src/daemon/session/actions.ts +++ b/packages/webui/src/daemon/session/actions.ts @@ -602,15 +602,18 @@ export function createDaemonSessionActions({ async enqueueMidTurnMessage( message: string, + opts?: { signal?: AbortSignal }, ): Promise { - // Best-effort and silent: no session / idle session / transport failure - // all resolve `{ accepted: false }` so the caller falls back to its own - // next-turn queue. Never raises a user-facing notice — a queued message - // typed mid-turn is an optimization, not a user-initiated action. + // Best-effort and silent: no session / idle session / transport failure / + // abort all resolve `{ accepted: false }` so the caller falls back to its + // own next-turn queue. Never raises a user-facing notice — a queued + // message typed mid-turn is an optimization, not a user-initiated action. + // `opts.signal` lets the caller abort a still-in-flight push when the turn + // it was meant for settles, so a late arrival can't land in the next turn. const session = sessionRef.current; if (!session) return { accepted: false }; try { - return await session.enqueueMidTurnMessage(message); + return await session.enqueueMidTurnMessage(message, opts); } catch { return { accepted: false }; } diff --git a/packages/webui/src/daemon/session/types.ts b/packages/webui/src/daemon/session/types.ts index 48b1322de91..fdb61c30550 100644 --- a/packages/webui/src/daemon/session/types.ts +++ b/packages/webui/src/daemon/session/types.ts @@ -295,7 +295,10 @@ export interface DaemonSessionActions { * a notice) when there is no session, the session is idle, or the push * fails — the caller then keeps the message in its own next-turn queue. */ - enqueueMidTurnMessage(message: string): Promise; + enqueueMidTurnMessage( + message: string, + opts?: { signal?: AbortSignal }, + ): Promise; sendShellCommand(command: string): Promise; getTasks(): Promise; cancelTask( diff --git a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts index 11e4b58505f..357cc285ae0 100644 --- a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts +++ b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { useSyncExternalStore } from 'react'; +import { useCallback, useSyncExternalStore } from 'react'; import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; import { clearSidechannelMidTurnInjected, @@ -21,9 +21,11 @@ export interface UseDaemonMidTurnInjectedResult { */ batches: readonly DaemonMidTurnMessageInjectedData[]; /** - * Clear the buffer once the batches have been reconciled. Safe to call from - * the effect body: it is synchronous, so no new frame can append between the - * read and the clear. Stable reference. + * Clear the batches that were reconciled. Compare-and-swap: it only clears if + * the buffer still holds exactly the `batches` snapshot returned above. A + * frame that appended between the render snapshot and the (async) effect that + * calls this is therefore preserved, not wiped — it is reconciled on the next + * effect run instead of being lost (which would resend it next turn). */ consume: () => void; } @@ -39,5 +41,14 @@ export function useDaemonMidTurnInjected(): UseDaemonMidTurnInjectedResult { getSidechannelMidTurnInjected, getSidechannelMidTurnInjected, ); - return { batches, consume: clearSidechannelMidTurnInjected }; + // Compare-and-swap on the exact snapshot we reconciled, so a frame that + // appended between this render and the consuming effect survives. `batches` is + // reference-stable between store changes (useSyncExternalStore), so the + // `[batches]` dep keeps `consume` stable too — a new snapshot is what makes a + // new closure, which is exactly when the consuming effect should re-run. + const consume = useCallback( + () => clearSidechannelMidTurnInjected(batches), + [batches], + ); + return { batches, consume }; } From 6b0a892ad92603893f709881e9eacbe04200603c Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 13:02:07 +0800 Subject: [PATCH 6/7] fix(daemon): scope mid-turn consume to reconciled batches; correct originator doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/sdk-typescript/src/daemon/events.ts | 14 +++++-- packages/web-shell/client/App.tsx | 10 ++++- .../daemon/midTurnInjectedSidechannel.test.ts | 40 +++++++++++++++---- .../src/daemon/midTurnInjectedSidechannel.ts | 40 +++++++++++++------ .../src/daemon/useDaemonMidTurnInjected.ts | 29 +++++++------- 5 files changed, 94 insertions(+), 39 deletions(-) diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 2b21f746d91..771a38e5311 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -263,10 +263,18 @@ export interface DaemonMidTurnMessageInjectedData { sessionId: string; messages: string[]; /** - * Trusted client id that queued these messages, surfaced from the SSE - * envelope's `originatorClientId` so a consumer dedupes only its OWN pending - * queue — a peer attached to the same session must not drop a + * Trusted client id that queued these messages, so a consumer dedupes only its + * OWN pending queue — a peer attached to the same session must not drop a * coincidentally-equal entry it didn't queue. Absent for anonymous pushes. + * + * IMPORTANT — wire location: unlike the permission/settings events (which the + * session reducer's `mergeOriginator` step copies from the envelope INTO + * `data`), this event is NOT reduced, so the daemon leaves the id ONLY on the + * SSE envelope (`event.originatorClientId`) and never populates it here. A raw + * SDK consumer must read `event.originatorClientId`; `data.originatorClientId` + * is filled in only by a consumer that lifts it off the envelope itself (the + * web-shell's `parseSidechannelMidTurnInjected` does this). The field lives on + * this shape so that lifted representation is well-typed. */ originatorClientId?: string; [key: string]: unknown; diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index c164798450b..68837250979 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -1355,7 +1355,15 @@ export function App({ queuedPromptsRef.current = next; setQueuedPrompts(next); } - consumeMidTurnInjected(); + // Consume ONLY this session's batches. The reconcile above is session- + // scoped, so a batch for another session (a late frame after an in-place + // session switch) must NOT be cleared here — it was never reconciled and + // would otherwise be lost on switch-back (resent next turn = double + // delivery). Identity-removal also leaves any frame that arrived after this + // render's snapshot for the next effect run. + consumeMidTurnInjected( + midTurnInjectedBatches.filter((b) => b.sessionId === sessionId), + ); }, [ midTurnInjectedBatches, connection.sessionId, diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts index 9fe9af3e445..36f26e69869 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.test.ts @@ -7,6 +7,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { clearSidechannelMidTurnInjected, + consumeSidechannelMidTurnInjected, getSidechannelMidTurnInjected, parseSidechannelMidTurnInjected, publishSidechannelMidTurnInjected, @@ -136,22 +137,45 @@ describe('mid-turn injected sidechannel pub/sub', () => { ]); }); - it('compare-and-swap clear preserves a batch appended after the snapshot', () => { - // Models the render→effect race: the consumer reconciles `snapshot`, but a - // new frame appends before its clear runs. A snapshot-scoped clear must NOT - // wipe the new batch (else it is resent next turn = double delivery). + it('consume removes only the handled batches, leaving a later-arrived frame (race)', () => { + // Models the render→effect race: the consumer reconciles `snapshot`, then a + // new frame appends before its consume runs. Identity-removal must drop only + // the snapshot and leave the new batch (else it is resent next turn). publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); const snapshot = getSidechannelMidTurnInjected(); publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['b'] }); // races in - clearSidechannelMidTurnInjected(snapshot); // stale ref ⇒ no-op + consumeSidechannelMidTurnInjected(snapshot); // drops only ['a'] expect(getSidechannelMidTurnInjected()).toEqual([ - { sessionId: 's-1', messages: ['a'] }, { sessionId: 's-1', messages: ['b'] }, ]); - // Clearing with the CURRENT ref succeeds. - clearSidechannelMidTurnInjected(getSidechannelMidTurnInjected()); + consumeSidechannelMidTurnInjected(getSidechannelMidTurnInjected()); + expect(getSidechannelMidTurnInjected()).toEqual([]); + }); + + it('consume leaves OTHER-session batches buffered (no cross-session wipe)', () => { + // A late frame for a previous session must survive an active-session consume, + // or it is lost on switch-back (resent next turn = double delivery). + publishSidechannelMidTurnInjected({ sessionId: 's-old', messages: ['x'] }); + publishSidechannelMidTurnInjected({ sessionId: 's-new', messages: ['y'] }); + const all = getSidechannelMidTurnInjected(); + const activeForNew = all.filter((b) => b.sessionId === 's-new'); + + consumeSidechannelMidTurnInjected(activeForNew); // only the s-new batch + expect(getSidechannelMidTurnInjected()).toEqual([ + { sessionId: 's-old', messages: ['x'] }, + ]); + }); + + it('consume is a no-op for batches not in the buffer (already evicted/cleared)', () => { + publishSidechannelMidTurnInjected({ sessionId: 's-1', messages: ['a'] }); + const snapshot = getSidechannelMidTurnInjected(); + clearSidechannelMidTurnInjected(); // wipe out from under it + const listener = vi.fn(); + subscribeSidechannelMidTurnInjected(listener); + consumeSidechannelMidTurnInjected(snapshot); // nothing matches ⇒ no notify + expect(listener).not.toHaveBeenCalled(); expect(getSidechannelMidTurnInjected()).toEqual([]); }); diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts index b2f92d7dae8..7c192475e6f 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -73,20 +73,36 @@ export function publishSidechannelMidTurnInjected( } /** - * Clear the buffer. Pass `expected` (the exact snapshot the consumer just - * reconciled) for a compare-and-swap clear: `useDaemonMidTurnInjected` reads - * `pending` during render and reconciles it in an async effect, so a new frame - * can append in the window between the read and the effect. Clearing - * unconditionally would wipe that newly-arrived batch before it is reconciled — - * its messages stay in the browser's pending queue and are resent next turn (the - * double delivery this feature exists to prevent). When a frame arrived, - * `pending !== expected`, so we no-op and leave it for the next reconcile. - * Omitting `expected` clears unconditionally (e.g. test teardown). + * Remove exactly the `handled` batches (by object identity) from the buffer. + * + * The consumer reconciles a SESSION-SCOPED subset of the buffer (the batches for + * the active session) and passes that same subset here. Two classes of batch are + * therefore deliberately NOT removed and survive for their own reconcile: + * + * - Batches for a DIFFERENT session — the buffer is a cross-session singleton, so + * on an in-place session switch a frame for the previous session can still be + * buffered. It was never reconciled against this session's queue, so wiping it + * would lose it on switch-back (resent next turn = double delivery). + * - Batches that arrived AFTER the consumer's render snapshot (the render→effect + * window) — they aren't in `handled`, so a blanket clear would drop them + * unreconciled. Identity-removal leaves them for the next effect run. + * + * Clearing only what was reconciled is what makes the dedupe exactly-once across + * both the multi-session and the late-frame races. */ -export function clearSidechannelMidTurnInjected( - expected?: readonly DaemonMidTurnMessageInjectedData[], +export function consumeSidechannelMidTurnInjected( + handled: readonly DaemonMidTurnMessageInjectedData[], ): void { - if (expected !== undefined && pending !== expected) return; + if (handled.length === 0 || pending.length === 0) return; + const handledSet = new Set(handled); + const next = pending.filter((batch) => !handledSet.has(batch)); + if (next.length === pending.length) return; // nothing matched — no notify + pending = next.length === 0 ? EMPTY : next; + notifyMidTurnInjectedListeners(); +} + +/** Drop the entire buffer (e.g. test teardown). */ +export function clearSidechannelMidTurnInjected(): void { if (pending.length === 0) return; pending = EMPTY; notifyMidTurnInjectedListeners(); diff --git a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts index 357cc285ae0..387ae57ab69 100644 --- a/packages/webui/src/daemon/useDaemonMidTurnInjected.ts +++ b/packages/webui/src/daemon/useDaemonMidTurnInjected.ts @@ -7,7 +7,7 @@ import { useCallback, useSyncExternalStore } from 'react'; import type { DaemonMidTurnMessageInjectedData } from '@qwen-code/sdk/daemon'; import { - clearSidechannelMidTurnInjected, + consumeSidechannelMidTurnInjected, getSidechannelMidTurnInjected, subscribeSidechannelMidTurnInjected, } from './midTurnInjectedSidechannel.js'; @@ -21,19 +21,20 @@ export interface UseDaemonMidTurnInjectedResult { */ batches: readonly DaemonMidTurnMessageInjectedData[]; /** - * Clear the batches that were reconciled. Compare-and-swap: it only clears if - * the buffer still holds exactly the `batches` snapshot returned above. A - * frame that appended between the render snapshot and the (async) effect that - * calls this is therefore preserved, not wiped — it is reconciled on the next - * effect run instead of being lost (which would resend it next turn). + * Drop exactly the batches passed in (by identity) — the consumer passes the + * subset it actually reconciled (its active session's batches). Batches for + * OTHER sessions, and frames that arrived after the snapshot, are not in that + * subset and stay buffered for their own reconcile, so neither a session + * switch nor a late frame can wipe an un-reconciled batch (= double delivery). */ - consume: () => void; + consume: (handled: readonly DaemonMidTurnMessageInjectedData[]) => void; } /** * Subscribe to injected mid-turn batches. Unlike a latest-wins signal, this * accumulates every batch so multi-batch turns (one frame per tool batch) are - * all reconciled; the consumer calls `consume()` after processing. + * all reconciled; the consumer calls `consume(handled)` with the batches it + * processed. */ export function useDaemonMidTurnInjected(): UseDaemonMidTurnInjectedResult { const batches = useSyncExternalStore( @@ -41,14 +42,12 @@ export function useDaemonMidTurnInjected(): UseDaemonMidTurnInjectedResult { getSidechannelMidTurnInjected, getSidechannelMidTurnInjected, ); - // Compare-and-swap on the exact snapshot we reconciled, so a frame that - // appended between this render and the consuming effect survives. `batches` is - // reference-stable between store changes (useSyncExternalStore), so the - // `[batches]` dep keeps `consume` stable too — a new snapshot is what makes a - // new closure, which is exactly when the consuming effect should re-run. + // Stable identity (empty deps): `consume` removes exactly the batches it is + // handed, so it needs no closure over the current snapshot. const consume = useCallback( - () => clearSidechannelMidTurnInjected(batches), - [batches], + (handled: readonly DaemonMidTurnMessageInjectedData[]) => + consumeSidechannelMidTurnInjected(handled), + [], ); return { batches, consume }; } From ea0a87fb567380d2ce5a5026e75157643063d6c0 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 16 Jun 2026 15:21:29 +0800 Subject: [PATCH 7/7] =?UTF-8?q?fix(daemon):=20mid-turn=20robustness=20?= =?UTF-8?q?=E2=80=94=20fetch=20timeout,=20observability,=20docs,=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cli/src/serve/server.test.ts | 28 ++++++++--- .../sdk-typescript/src/daemon/DaemonClient.ts | 19 +++++-- packages/sdk-typescript/src/daemon/events.ts | 6 +++ .../test/unit/DaemonClient.test.ts | 42 ++++++++++++++-- packages/web-shell/client/App.tsx | 22 ++++++++- .../src/daemon/midTurnInjectedSidechannel.ts | 15 ++++-- .../session/DaemonSessionProvider.test.tsx | 49 +++++++++++++++++++ packages/webui/src/daemon/session/actions.ts | 13 ++++- 8 files changed, 172 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 3a812d5992c..533307379ba 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -2608,6 +2608,24 @@ describe('createServeApp', () => { expect(res.status).toBe(400); expect(bridge.enqueueMidTurnCalls).toEqual([]); }); + + it('maps a bridge InvalidClientIdError to 400 invalid_client_id', async () => { + // Well-formed but unbound client id: the bridge's ownership check throws, + // and `sendBridgeError` maps it like the sibling routes. + const bridge = fakeBridge({ + enqueueMidTurnImpl: (sid) => { + throw new InvalidClientIdError(sid, 'rogue'); + }, + }); + const res = await midTurnPost( + midTurnApp(bridge), + 's-1', + { message: 'hi' }, + 'rogue', + ); + expect(res.status).toBe(400); + expect(res.body.code).toBe('invalid_client_id'); + }); }); describe('host allowlist (loopback bind)', () => { @@ -6386,13 +6404,9 @@ describe('createServeApp', () => { describe('GET /daemon/status', () => { it('requires bearer auth when a token is configured', async () => { - const app = createServeApp( - { ...baseOpts, token: 'secret' }, - undefined, - { - bridge: fakeBridge(), - }, - ); + const app = createServeApp({ ...baseOpts, token: 'secret' }, undefined, { + bridge: fakeBridge(), + }); const noAuth = await request(app) .get('/daemon/status') diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 8b041c887ab..a7b4f4decfd 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -1489,7 +1489,11 @@ export class DaemonClient { message: string, opts?: { signal?: AbortSignal; clientId?: string }, ): Promise { - const res = await this.transport.fetch( + // Route through `fetchWithTimeout` like every other method so a hung daemon + // can't wedge this promise forever (the caller in `actions.ts` awaits it). + // The helper composes any caller `signal` (the turn-scoped abort) WITH its + // timeout controller, so the mid-turn-settle abort still propagates. + return await this.fetchWithTimeout( `${this.baseUrl}/session/${encodeURIComponent(sessionId)}/mid-turn-message`, { method: 'POST', @@ -1500,11 +1504,16 @@ export class DaemonClient { body: JSON.stringify({ message }), signal: opts?.signal, }, + async (res) => { + if (!res.ok) { + throw await this.failOnError( + res, + 'POST /session/:id/mid-turn-message', + ); + } + return (await res.json()) as DaemonMidTurnMessageResult; + }, ); - if (!res.ok) { - throw await this.failOnError(res, 'POST /session/:id/mid-turn-message'); - } - return (await res.json()) as DaemonMidTurnMessageResult; } /** diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 771a38e5311..ea6a9cd6b41 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -267,6 +267,12 @@ export interface DaemonMidTurnMessageInjectedData { * OWN pending queue — a peer attached to the same session must not drop a * coincidentally-equal entry it didn't queue. Absent for anonymous pushes. * + * CONTRACT: a consumer that dedupes on this event MUST compare this id against + * its own client id and skip frames originated by a different client. The + * daemon broadcasts the frame to every SSE subscriber on the session and does + * NOT route by originator, so a consumer that dedupes unconditionally will drop + * another client's coincidentally-equal pending message (double delivery). + * * IMPORTANT — wire location: unlike the permission/settings events (which the * session reducer's `mergeOriginator` step copies from the envelope INTO * `data`), this event is NOT reduced, so the daemon leaves the id ONLY on the diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index c1bf1091105..4fd8a2c0bcf 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -1879,7 +1879,10 @@ describe('DaemonClient', () => { jsonResponse(200, { accepted: true }), ); const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); - const result = await client.enqueueMidTurnMessage('s-1', 'also check tests'); + const result = await client.enqueueMidTurnMessage( + 's-1', + 'also check tests', + ); expect(result).toEqual({ accepted: true }); expect(calls[0]?.url).toBe('http://daemon/session/s-1/mid-turn-message'); expect(calls[0]?.method).toBe('POST'); @@ -1897,7 +1900,7 @@ describe('DaemonClient', () => { expect(result.accepted).toBe(false); }); - it('URL-encodes the session id and forwards client id + abort signal', async () => { + it('URL-encodes the session id, forwards client id, and propagates the abort signal', async () => { const { fetch, calls } = recordingFetch(() => jsonResponse(200, { accepted: true }), ); @@ -1907,9 +1910,40 @@ describe('DaemonClient', () => { clientId: 'client-1', signal: ctrl.signal, }); - expect(calls[0]?.url).toBe('http://daemon/session/s%2F1/mid-turn-message'); + expect(calls[0]?.url).toBe( + 'http://daemon/session/s%2F1/mid-turn-message', + ); expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-1'); - expect(calls[0]?.signal).toBe(ctrl.signal); + // `fetchWithTimeout` composes the caller signal with its timeout + // controller, so the forwarded signal is not identical to `ctrl.signal`, + // but aborting the caller's signal must still propagate to the request + // (this is what cancels a late mid-turn push at turn settle). + const forwarded = calls[0]?.signal; + expect(forwarded).toBeDefined(); + expect(forwarded?.aborted).toBe(false); + ctrl.abort(); + expect(forwarded?.aborted).toBe(true); + }); + + it('times out a hung daemon instead of hanging forever', async () => { + // Regression guard for the `fetchWithTimeout` switch: a daemon that never + // responds must reject (not wedge the void-ed caller in actions.ts). The + // mock honours the abort signal like a real fetch, so the timeout's abort + // settles it; before the switch this would hang on `transport.fetch`. + const fetch = ((_url: RequestInfo | URL, init?: RequestInit) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => + reject(new DOMException('aborted', 'AbortError')), + ); + })) as unknown as typeof globalThis.fetch; + const client = new DaemonClient({ + baseUrl: 'http://daemon', + fetch, + fetchTimeoutMs: 20, + }); + await expect( + client.enqueueMidTurnMessage('s-1', 'hi'), + ).rejects.toBeDefined(); }); it('throws on 404 when the session is unknown', async () => { diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 68837250979..c24789af63c 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -1298,7 +1298,14 @@ export function App({ // queuedPrompts and follows the normal next-turn path. useEffect(() => { if (streamingState !== 'idle') return; - midTurnEnqueueAbortRef.current?.abort(); + const ctrl = midTurnEnqueueAbortRef.current; + if (!ctrl) return; + // A controller exists ⇒ at least one mid-turn push was issued this turn. + // Cancel it so a still-in-flight push can't land in the next turn (a + // completed one makes this a no-op). Debug-only, mirrors the server-side + // mid-turn observability. + console.debug('[mid-turn] turn settled; cancelling any in-flight push'); + ctrl.abort(); midTurnEnqueueAbortRef.current = null; }, [streamingState]); @@ -1345,6 +1352,19 @@ export function App({ // omitting this would skip every batch — leaving our own messages in the // queue to be resent next turn (double delivery). A peer on the same // session keeps its own coincidentally-equal entry. + if ( + connection.clientId === undefined && + midTurnInjectedBatches.some( + (b) => b.sessionId === sessionId && b.originatorClientId !== undefined, + ) + ) { + // Edge: stamped batches but no client id yet (older daemon / reconnect + // timing). Dedupe skips them, so they may be resent next turn — make it + // diagnosable rather than a silent double-delivery. + console.debug( + '[mid-turn] originator-stamped batches but no client id; dedupe skipped (may resend next turn)', + ); + } const next = removeInjectedFromQueue( queuedPromptsRef.current, midTurnInjectedBatches, diff --git a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts index 7c192475e6f..ea6b956fcd9 100644 --- a/packages/webui/src/daemon/midTurnInjectedSidechannel.ts +++ b/packages/webui/src/daemon/midTurnInjectedSidechannel.ts @@ -65,10 +65,17 @@ export function publishSidechannelMidTurnInjected( : {}), }, ]; - pending = - appended.length > MAX_PENDING_BATCHES - ? appended.slice(appended.length - MAX_PENDING_BATCHES) - : appended; + if (appended.length > MAX_PENDING_BATCHES) { + const dropped = appended.length - MAX_PENDING_BATCHES; + // Eviction means an orphaned consumer (unmounted / never reconciled) — make + // the silent drop visible, mirroring the server-side mid-turn observability. + console.debug( + `[mid-turn] sidechannel buffer over ${MAX_PENDING_BATCHES}; evicting ${dropped} oldest batch(es)`, + ); + pending = appended.slice(dropped); + } else { + pending = appended; + } notifyMidTurnInjectedListeners(); } diff --git a/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx b/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx index 30b2c419a75..f4eb3b93825 100644 --- a/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx +++ b/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx @@ -33,6 +33,10 @@ import { type DaemonWorkspaceEventSignals, } from './DaemonSessionProvider.js'; import { DaemonWorkspaceProvider } from '../workspace/DaemonWorkspaceProvider.js'; +import { + clearSidechannelMidTurnInjected, + getSidechannelMidTurnInjected, +} from '../midTurnInjectedSidechannel.js'; interface MockSession { sessionId: string; @@ -422,6 +426,51 @@ describe('DaemonSessionProvider', () => { ]); }); + it('routes mid_turn_message_injected frames to the sidechannel, not the transcript', async () => { + // Locks the event-pump branch (parse → publishSidechannel → continue): the + // frame must seed the dedupe sidechannel and NOT normalize into a transcript + // block. Removing the `continue` (or the parse) would misroute it. + clearSidechannelMidTurnInjected(); + const session = createMockSession({ + events: async function* midTurnEvents() { + yield { + id: 21, + v: 1, + type: 'mid_turn_message_injected', + originatorClientId: 'client-mt', + data: { sessionId: 'mt-session', messages: ['also check the tests'] }, + }; + }, + }); + sdkMocks.sessions.push(session); + let blocks: readonly DaemonTranscriptBlock[] = []; + + function Harness() { + blocks = useDaemonTranscriptBlocks(); + return null; + } + + await renderWithProvider(, { + autoConnect: true, + autoReconnect: false, + }); + await act(async () => { + await flushPromises(); + }); + + // Seeded the dedupe sidechannel (with the envelope-level originatorClientId). + expect(getSidechannelMidTurnInjected()).toEqual([ + { + sessionId: 'mt-session', + messages: ['also check the tests'], + originatorClientId: 'client-mt', + }, + ]); + // …and was NOT rendered as a transcript block. + expect(blocks).toEqual([]); + clearSidechannelMidTurnInjected(); + }); + it('publishes action error notices when no session is connected', async () => { let actions: DaemonUiSessionActions | undefined; let blocks: readonly DaemonTranscriptBlock[] = []; diff --git a/packages/webui/src/daemon/session/actions.ts b/packages/webui/src/daemon/session/actions.ts index 49db5085ff5..7019c00dac0 100644 --- a/packages/webui/src/daemon/session/actions.ts +++ b/packages/webui/src/daemon/session/actions.ts @@ -614,7 +614,18 @@ export function createDaemonSessionActions({ if (!session) return { accepted: false }; try { return await session.enqueueMidTurnMessage(message, opts); - } catch { + } catch (err) { + // An abort is the designed settle-time cancel (the message stays in the + // browser queue for the next turn), not a failure — stay silent. Any + // OTHER error (daemon down, 4xx/5xx, network, timeout) silently disables + // mid-turn drain for every client, so surface it at debug for DevTools + // without raising a user-facing notice. + if (!(err instanceof DOMException && err.name === 'AbortError')) { + console.debug( + '[enqueueMidTurnMessage] push failed; kept for next turn', + err, + ); + } return { accepted: false }; } },