From a0b47093a1495129acf2f9964297163b1e09ac1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Fri, 29 May 2026 10:22:05 +0800 Subject: [PATCH 01/16] =?UTF-8?q?feat(daemon):=20bridge=20side-channel=20s?= =?UTF-8?q?tate=20layer=20=E2=80=94=20A1=20follow-up=20+=20A2=20+=20A5=20(?= =?UTF-8?q?#4511)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/acp-bridge/src/bridge.test.ts | 377 ++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 342 ++++++++++++---- packages/acp-bridge/src/bridgeClient.ts | 117 +++++- packages/acp-bridge/src/bridgeTypes.ts | 5 +- .../src/acp-integration/session/Session.ts | 21 + packages/cli/src/serve/server.ts | 2 + packages/sdk-typescript/src/daemon/events.ts | 34 +- packages/sdk-typescript/src/daemon/index.ts | 3 + .../test/unit/daemonEvents.test.ts | 60 +++ 9 files changed, 872 insertions(+), 89 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index df7862c110d..26f187d1341 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -7563,6 +7563,383 @@ describe('extractErrorCode', () => { it('returns undefined when code is not string or number', () => { expect(extractErrorCode({ code: true })).toBeUndefined(); +// --------------------------------------------------------------------------- +// §2.3 side-channel state layer: publish helpers + reconciliation + snapshot +// --------------------------------------------------------------------------- + +describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { + describe('publish helpers cache + generation', () => { + it('publishModelSwitched updates cache and publishes model_switched', async () => { + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => ({}); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ); + + const it2 = iter[Symbol.asyncIterator](); + const next = await it2.next(); + expect(next.value?.type).toBe('model_switched'); + expect((next.value?.data as { modelId: string }).modelId).toBe( + 'qwen-max', + ); + abort.abort(); + await bridge.shutdown(); + }); + + it('publishApprovalModeChanged publishes approval_mode_changed on setSessionApprovalMode', async () => { + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const agent = new FakeAgent({ + extMethodImpl: (method, params) => { + if (method === 'qwen/control/session/approval_mode') { + return Promise.resolve({ + previous: 'default', + current: (params as { mode: string }).mode, + }); + } + return Promise.resolve({}); + }, + }); + new AgentSideConnection(() => agent as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await bridge.setSessionApprovalMode( + session.sessionId, + ApprovalMode.YOLO, + { persist: false }, + ); + + const it2 = iter[Symbol.asyncIterator](); + const next = await it2.next(); + expect(next.value?.type).toBe('approval_mode_changed'); + expect((next.value?.data as { next: string }).next).toBe( + ApprovalMode.YOLO, + ); + abort.abort(); + await bridge.shutdown(); + }); + }); + + describe('extNotification — in-session mode update (A2)', () => { + it('promotes current_mode_update to approval_mode_changed when no bridge roundtrip is in flight', async () => { + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'auto-edit', + }); + + const collected: Array<{ type: string; data: unknown }> = []; + for await (const e of iter) { + collected.push({ type: e.type, data: e.data }); + if (collected.length === 1) break; + } + expect(collected[0]?.type).toBe('approval_mode_changed'); + expect((collected[0]?.data as { next: string }).next).toBe('auto-edit'); + abort.abort(); + await bridge.shutdown(); + }); + + it('suppresses current_mode_update while a bridge approval-mode roundtrip is in flight', async () => { + let releaseMode: (() => void) | undefined; + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method.includes('approval_mode')) { + return new Promise>((res) => { + releaseMode = () => + res({ previous: 'default', current: 'yolo' }); + }); + } + return {}; + }, + }); + capturedConn = new AgentSideConnection( + () => fakeAgent as Agent, + agentStream, + ); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + const modeChange = bridge + .setSessionApprovalMode(session.sessionId, ApprovalMode.YOLO, { + persist: false, + }) + .catch(() => {}); + await new Promise((r) => setTimeout(r, 10)); + + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'auto', + }); + await new Promise((r) => setTimeout(r, 10)); + + releaseMode!(); + await modeChange; + + const seen: string[] = []; + for await (const e of iter) { + seen.push(e.type); + if (e.type === 'approval_mode_changed') break; + } + // Only the bridge's own approval_mode_changed (yolo) — the suppressed + // 'auto' notification did NOT produce a second event. + expect(seen.filter((t) => t === 'approval_mode_changed')).toHaveLength(1); + abort.abort(); + await bridge.shutdown(); + }); + + it('drops malformed mode-update params without throwing', async () => { + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + // Missing currentModeId + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + }); + // Non-string currentModeId + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 42, + }); + await new Promise((r) => setTimeout(r, 50)); + + // No events should have been produced (no approval_mode_changed). + // Send a known good one to break the iterator. + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-max', + }); + + const seen: string[] = []; + for await (const e of iter) { + seen.push(e.type); + if (e.type === 'model_switched') break; + } + expect(seen.filter((t) => t === 'approval_mode_changed')).toEqual([]); + abort.abort(); + await bridge.shutdown(); + }); + }); + + describe('A5 — session snapshot on attach', () => { + it('yields session_snapshot after replay_complete when snapshot=true', async () => { + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Promote a model change to populate the cache. + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-turbo', + }); + await new Promise((r) => setTimeout(r, 20)); + + // Subscribe with snapshot=true (triggers replay_complete + snapshot). + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + lastEventId: 0, + snapshot: true, + }); + + const collected: BridgeEvent[] = []; + for await (const e of iter) { + collected.push(e); + if (e.type === 'session_snapshot') break; + } + const rc = collected.find((e) => e.type === 'replay_complete'); + const snap = collected.find((e) => e.type === 'session_snapshot'); + expect(rc).toBeDefined(); + expect(snap).toBeDefined(); + expect(collected.indexOf(snap!)).toBeGreaterThan(collected.indexOf(rc!)); + expect( + (snap!.data as { currentModelId: string | null }).currentModelId, + ).toBe('qwen-turbo'); + abort.abort(); + await bridge.shutdown(); + }); + + it('does NOT yield session_snapshot when snapshot is not set', async () => { + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Promote a model change so there IS cache state. + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-turbo', + }); + await new Promise((r) => setTimeout(r, 20)); + + // Subscribe WITHOUT snapshot. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + lastEventId: 0, + }); + + // After replay_complete, send a known event to break the loop. + const collected: BridgeEvent[] = []; + // Publish something after a short delay so the iterator eventually yields. + setTimeout(() => { + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-max', + }); + }, 30); + + for await (const e of iter) { + collected.push(e); + // Stop after we see replay_complete + one more real event. + if ( + collected.some((c) => c.type === 'replay_complete') && + collected.some((c) => c.type === 'model_switched') + ) + break; + } + expect( + collected.find((e) => e.type === 'session_snapshot'), + ).toBeUndefined(); + abort.abort(); + await bridge.shutdown(); + }); }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index f5a374f5264..ca022956336 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -26,7 +26,12 @@ import { } from '@qwen-code/qwen-code-core'; import type { ShellCommandResult } from './bridgeTypes.js'; import type { AcpChannel } from './channel.js'; -import { EventBus, DEFAULT_RING_SIZE, type BridgeEvent } from './eventBus.js'; +import { + EventBus, + DEFAULT_RING_SIZE, + EVENT_SCHEMA_VERSION, + type BridgeEvent, +} from './eventBus.js'; import { TurnBoundaryCompactionEngine } from './compactionEngine.js'; import { BridgeChannelClosedError, @@ -37,6 +42,7 @@ import { SERVE_STATUS_EXT_METHODS, STATUS_SCHEMA_VERSION, type ServeSessionStatsStatus, + type ServeSessionContextStatus, type ServeSessionTasksStatus, } from './status.js'; import { @@ -234,6 +240,20 @@ interface SessionEntry { * `/model` (no bridge roundtrip) sees this false and IS promoted. */ modelRoundtripInFlight?: boolean; + /** A2: true while the bridge drives an approval-mode roundtrip. */ + approvalModeRoundtripInFlight?: boolean; + /** §2.3: cached model id, updated by every `publishModelSwitched` call. */ + currentModelId?: string; + /** §2.3: cached approval mode, updated by every `publishApprovalModeChanged` call. */ + currentApprovalMode?: string; + /** §2.3: monotonic counter bumped on every `model_switched` publish. */ + modelPublishGeneration: number; + /** §2.3: monotonic counter bumped on every `approval_mode_changed` publish. */ + approvalModePublishGeneration: number; + /** §2.2: true while a model reconciliation read is in flight. */ + modelReconciliationInFlight?: boolean; + /** §2.2: true while an approval-mode reconciliation read is in flight. */ + approvalModeReconciliationInFlight?: boolean; /** * Per-session approval-mode FIFO. Mirrors `modelChangeQueue`: * serializes concurrent `setSessionApprovalMode` calls so two @@ -1048,6 +1068,24 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // consumers + channels / IDE companion omit it; BridgeClient // falls back to its inline fs proxy. opts.fileSystem, + // §2.3: centralised model_switched publish — keeps cache + generation + // update atomic. BridgeClient calls this instead of inlining publish. + (entry, modelId, originator) => + publishModelSwitched(entry as SessionEntry, modelId, originator), + // A2: centralised approval_mode_changed publish on in-session mode + // promotion. `previous` is read from the bridge state cache. + (entry, modeId, originator) => { + const se = entry as SessionEntry; + publishApprovalModeChanged( + se, + { + previous: se.currentApprovalMode ?? 'default', + next: modeId, + persisted: false, + }, + originator, + ); + }, ); const connection = new ClientSideConnection(() => client, channel.stream); @@ -1414,27 +1452,28 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ), transportClosed, ]); - entry.events.publish({ - type: 'model_switched', - data: { sessionId: entry.sessionId, modelId }, - ...(originatorClientId ? { originatorClientId } : {}), - }); + publishModelSwitched(entry, modelId, originatorClientId); } catch (err) { // Surface the failure to ALL attached clients, not just the // caller — a shared session swallowing a denied model change // silently would surprise the others. - entry.events.publish({ - type: 'model_switch_failed', - data: { - sessionId: entry.sessionId, - requestedModelId: modelId, - error: err instanceof Error ? err.message : String(err), - }, - ...(originatorClientId ? { originatorClientId } : {}), - }); + try { + entry.events.publish({ + type: 'model_switch_failed', + data: { + sessionId: entry.sessionId, + requestedModelId: modelId, + error: err instanceof Error ? err.message : String(err), + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); + } catch { + /* bus closed */ + } throw err; } finally { entry.modelRoundtripInFlight = false; + void reconcileAfterRoundtrip(entry, 'model'); } }); // Tail swallows failures so subsequent model changes still run; the @@ -1679,6 +1718,119 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const createSessionEventBus = (): EventBus => new EventBus(eventRingSize, undefined, new TurnBoundaryCompactionEngine()); + // §2.3 publish helpers — centralise cache + generation + bus publish so + // every `model_switched` / `approval_mode_changed` site stays atomic. + + const publishModelSwitched = ( + entry: SessionEntry, + modelId: string, + originatorClientId: string | undefined, + ): void => { + entry.currentModelId = modelId; + entry.modelPublishGeneration++; + try { + entry.events.publish({ + type: 'model_switched', + data: { sessionId: entry.sessionId, modelId }, + ...(originatorClientId ? { originatorClientId } : {}), + }); + } catch { + /* bus closed */ + } + }; + + const publishApprovalModeChanged = ( + entry: SessionEntry, + payload: { previous: string; next: string; persisted: boolean }, + originatorClientId: string | undefined, + ): void => { + entry.currentApprovalMode = payload.next; + entry.approvalModePublishGeneration++; + try { + entry.events.publish({ + type: 'approval_mode_changed', + data: { + sessionId: entry.sessionId, + previous: payload.previous, + next: payload.next, + persisted: payload.persisted, + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); + } catch { + /* bus closed */ + } + }; + + // §2.2 post-roundtrip reconciliation — after a bridge-driven model or + // approval-mode change settles, re-read the agent's actual state and + // emit a corrective event if it drifted from the cached value. + const reconcileAfterRoundtrip = async ( + entry: SessionEntry, + target: 'model' | 'approvalMode', + ): Promise => { + const flagKey = + target === 'model' + ? 'modelReconciliationInFlight' + : 'approvalModeReconciliationInFlight'; + if (entry[flagKey]) return; + entry[flagKey] = true; + const genBefore = + target === 'model' + ? entry.modelPublishGeneration + : entry.approvalModePublishGeneration; + try { + const status = await requestSessionStatus( + entry.sessionId, + SERVE_STATUS_EXT_METHODS.sessionContext, + ); + const genAfter = + target === 'model' + ? entry.modelPublishGeneration + : entry.approvalModePublishGeneration; + if (genAfter !== genBefore) return; + + if (target === 'model') { + const actual = ( + status?.state?.models as { currentModelId?: string } | undefined + )?.currentModelId; + if (actual && actual !== entry.currentModelId) { + publishModelSwitched(entry, actual, undefined); + } + } else { + const actual = ( + status?.state?.modes as { currentModeId?: string } | undefined + )?.currentModeId; + if (actual && actual !== entry.currentApprovalMode) { + publishApprovalModeChanged( + entry, + { + previous: entry.currentApprovalMode ?? 'default', + next: actual, + persisted: false, + }, + undefined, + ); + } + } + } catch (err) { + try { + entry.events.publish({ + type: 'reconciliation_failed', + data: { + sessionId: entry.sessionId, + target, + error: err instanceof Error ? err.message : String(err), + }, + }); + } catch { + /* bus closed */ + } + } finally { + entry[flagKey] = false; + } + }; + const createSessionEntry = ( ci: ChannelInfo, sessionId: string, @@ -1695,6 +1847,8 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { promptQueue: Promise.resolve(), modelChangeQueue: Promise.resolve(), approvalModeQueue: Promise.resolve(), + modelPublishGeneration: 0, + approvalModePublishGeneration: 0, pendingPermissionIds: new Set(), clientIds: new Map(), clientLastSeenAt: new Map(), @@ -2505,7 +2659,30 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { subscribeEvents(sessionId, subOpts) { const entry = byId.get(sessionId); if (!entry) throw new SessionNotFoundError(sessionId); - return entry.events.subscribe(subOpts); + const raw = entry.events.subscribe(subOpts); + if (!subOpts?.snapshot) return raw; + + // A5: wrap the iterator to inject a synthetic `session_snapshot` + // frame immediately after `replay_complete`. Captures cached + // side-channel state synchronously at yield time so the client + // can seed its reducer without an extra round-trip. + async function* withSnapshot(): AsyncIterable { + for await (const event of raw) { + yield event; + if (event.type === 'replay_complete') { + yield { + v: EVENT_SCHEMA_VERSION, + type: 'session_snapshot', + data: { + sessionId: entry!.sessionId, + currentModelId: entry!.currentModelId ?? null, + currentApprovalMode: entry!.currentApprovalMode ?? null, + }, + }; + } + } + } + return withSnapshot(); }, getSessionLastEventId(sessionId) { @@ -3218,14 +3395,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ), transportClosed, ]); - entry.events.publish({ - type: 'model_switched', - data: { sessionId: entry.sessionId, modelId: req.modelId }, - ...(originatorClientId ? { originatorClientId } : {}), - }); + publishModelSwitched(entry, req.modelId, originatorClientId); return result; } finally { entry.modelRoundtripInFlight = false; + void reconcileAfterRoundtrip(entry, 'model'); } }); // Tail-swallow on the queue so a model-change failure doesn't poison @@ -3298,76 +3472,80 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // the queued work means the next change can't start its `extMethod` until // this change's side effects are fully done. Mirrors `modelChangeQueue`. const approvalWork = entry.approvalModeQueue.then(async () => { - const response = (await Promise.race([ - withTimeout( - entry.connection.extMethod( + // A2: suppress the agent's current_mode_update notification while + // the bridge owns the change. Mirrors `modelRoundtripInFlight`. + // The flag stays true through persist + publish so the notification + // cannot slip through during the persist phase (review finding #3). + entry.approvalModeRoundtripInFlight = true; + try { + const response = (await Promise.race([ + withTimeout( + entry.connection.extMethod( + SERVE_CONTROL_EXT_METHODS.sessionApprovalMode, + { sessionId, mode }, + ), + initTimeoutMs, SERVE_CONTROL_EXT_METHODS.sessionApprovalMode, - { sessionId, mode }, ), - initTimeoutMs, - SERVE_CONTROL_EXT_METHODS.sessionApprovalMode, - ), - getTransportClosedReject(entry), - ])) as { previous: ApprovalMode; current: ApprovalMode }; - - let persisted = false; - if (opts.persist) { - try { - await withTimeout( - persistApprovalMode?.(boundWorkspace, mode) ?? Promise.resolve(), - PERSIST_TIMEOUT_MS, - 'persistApprovalMode', - ); - persisted = persistApprovalMode !== undefined; - } catch (err) { - // Persist failure is non-fatal — the in-process change already - // took effect inside the ACP child. Log but don't fail the route. - writeStderrLine( - `setSessionApprovalMode: persist failed: ${ - err instanceof Error ? err.message : String(err) - }`, - ); + getTransportClosedReject(entry), + ])) as { previous: ApprovalMode; current: ApprovalMode }; + + let persisted = false; + if (opts.persist) { + try { + await withTimeout( + persistApprovalMode?.(boundWorkspace, mode) ?? + Promise.resolve(), + PERSIST_TIMEOUT_MS, + 'persistApprovalMode', + ); + persisted = persistApprovalMode !== undefined; + } catch (err) { + writeStderrLine( + `setSessionApprovalMode: persist failed: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } } - } - try { - entry.events.publish({ - type: 'approval_mode_changed', - data: { - sessionId: entry.sessionId, + publishApprovalModeChanged( + entry, + { previous: response.previous, next: response.current, persisted, }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed */ - } - // A persisted change becomes the workspace default, so fan out a - // workspace-scoped mirror for peer sessions. Skip the requesting - // session (its own bus already got the publish above) to avoid - // double-counting in the reducer. - if (persisted) { - broadcastWorkspaceEvent( - { - type: 'approval_mode_changed', - data: { - sessionId: entry.sessionId, - previous: response.previous, - next: response.current, - persisted, - }, - ...(originatorClientId ? { originatorClientId } : {}), - }, - entry.sessionId, + originatorClientId, ); + // #4282 fold-in 4 (S2): a persisted change becomes the workspace + // default, so fan out a workspace-scoped mirror for peer sessions. + // #4297 fold-in 1: skip the requesting session (its own bus already + // got the publish above) to avoid double-counting in the reducer. + if (persisted) { + broadcastWorkspaceEvent( + { + type: 'approval_mode_changed', + data: { + sessionId: entry.sessionId, + previous: response.previous, + next: response.current, + persisted, + }, + ...(originatorClientId ? { originatorClientId } : {}), + }, + entry.sessionId, + ); + } + return { + sessionId: entry.sessionId, + mode: response.current, + previous: response.previous, + persisted, + }; + } finally { + entry.approvalModeRoundtripInFlight = false; + void reconcileAfterRoundtrip(entry, 'approvalMode'); } - return { - sessionId: entry.sessionId, - mode: response.current, - previous: response.previous, - persisted, - }; }); // Tail-swallow so a failed change doesn't poison subsequent ones. entry.approvalModeQueue = approvalWork.then( diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 6e851a16ddf..83c7d108a79 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -195,6 +195,8 @@ export interface BridgeClientSessionEntry { * in `bridge.ts`; surfaced here for the demux. */ modelRoundtripInFlight?: boolean; + /** A2: mirrors `modelRoundtripInFlight` for approval-mode roundtrips. */ + approvalModeRoundtripInFlight?: boolean; } /** @@ -271,6 +273,28 @@ export class BridgeClient implements Client { * companion — preserves the inline proxy behavior. */ private readonly fileSystem?: BridgeFileSystem, + /** + * §2.3 callback: centralised `model_switched` publish through the + * bridge factory's cache-updating helper. The BridgeClient calls + * this instead of inlining `entry.events.publish(...)` so the + * cache update + generation bump stays atomic in one place. + */ + private readonly onModelPromoted?: ( + entry: BridgeClientSessionEntry, + modelId: string, + originatorClientId: string | undefined, + ) => void, + /** + * §2.3 / A2 callback: centralised `approval_mode_changed` publish. + * Called by the A2 `current_mode_update` demux when the agent + * switches approval mode in-session (exit_plan_mode, ProceedAlways, + * /mode). `previous` is read from the bridge state cache. + */ + private readonly onModePromoted?: ( + entry: BridgeClientSessionEntry, + modeId: string, + originatorClientId: string | undefined, + ) => void, ) {} async requestPermission( @@ -438,6 +462,10 @@ export class BridgeClient implements Client { this.handleInSessionModelUpdate(params); return; } + if (method === 'qwen/notify/session/mode-update') { + this.handleInSessionModeUpdate(params); + return; + } if (method === 'qwen/notify/session/prompt-suggestion') { const sessionId = params['sessionId']; const suggestion = params['suggestion']; @@ -532,20 +560,99 @@ export class BridgeClient implements Client { ); return; } + if (this.onModelPromoted) { + this.onModelPromoted( + entry, + currentModelId, + entry.activePromptOriginatorClientId, + ); + } else { + try { + entry.events.publish({ + type: 'model_switched', + data: { sessionId, modelId: currentModelId }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), + }); + } catch { + /* bus closed */ + } + } + writeStderrLine( + `[demux] session=${sessionId} type=current_model_update action=promoted model=${currentModelId}`, + ); + } + + /** + * A2: promote an in-session `current_mode_update` extNotification to + * `approval_mode_changed`. Mirrors `handleInSessionModelUpdate` exactly: + * suppressed while the bridge is driving its own approval-mode roundtrip + * (`entry.approvalModeRoundtripInFlight`). Additionally emits a legacy + * `session_update{current_mode_update}` for IDE companion compat + * (dual-emit transition — see §6 of the design doc). + */ + private handleInSessionModeUpdate(params: Record): void { + const sessionId = params['sessionId']; + const currentModeId = params['currentModeId']; + if (typeof sessionId !== 'string' || typeof currentModeId !== 'string') { + return; + } + const entry = this.resolveEntry(sessionId); + if (!entry) { + writeStderrLine( + `[demux] session=${sessionId} type=current_mode_update action=dropped reason=no_entry`, + ); + return; + } + if (entry.approvalModeRoundtripInFlight) { + writeStderrLine( + `[demux] session=${sessionId} type=current_mode_update action=suppressed reason=bridge_roundtrip_in_flight`, + ); + return; + } + if (this.onModePromoted) { + this.onModePromoted( + entry, + currentModeId, + entry.activePromptOriginatorClientId, + ); + } else { + try { + entry.events.publish({ + type: 'approval_mode_changed', + data: { sessionId, next: currentModeId }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), + }); + } catch { + /* bus closed */ + } + } + // TODO(dual-emit-removal): also emit the legacy generic + // `session_update{current_mode_update}` for one release cycle so the + // VS Code IDE companion's existing `case 'current_mode_update'` + // handler keeps working. Remove this block (and its tracking issue) + // once the companion ships an `approval_mode_changed` handler. try { entry.events.publish({ - type: 'model_switched', - data: { sessionId, modelId: currentModelId }, + type: 'session_update', + data: { + sessionId, + sessionUpdate: 'current_mode_update', + currentModeId, + }, ...(entry.activePromptOriginatorClientId ? { originatorClientId: entry.activePromptOriginatorClientId } : {}), }); - writeStderrLine( - `[demux] session=${sessionId} type=current_model_update action=promoted model=${currentModelId}`, - ); } catch { /* bus closed */ } + writeStderrLine( + `[demux] session=${sessionId} type=current_mode_update action=promoted mode=${currentModeId}`, + ); } /** diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 6afc521b088..dac9f613fe3 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -239,7 +239,10 @@ export interface AcpSessionBridge { */ subscribeEvents( sessionId: string, - opts?: SubscribeOptions, + opts?: SubscribeOptions & { + /** Yield a synthetic `session_snapshot` frame after replay completes. */ + snapshot?: boolean; + }, ): AsyncIterable; /** diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 1fd0d240df6..883d6589a2e 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1814,6 +1814,16 @@ export class Session implements SessionContext { const approvalMode = modeMap[params.modeId as ApprovalModeValue]; this.config.setApprovalMode(approvalMode); + + // A2 (#4511): notify attached clients of an in-session mode switch. + // Mirrors the model-update extNotification in `setModel`. + void this.client + .extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: this.sessionId, + currentModeId: params.modeId, + }) + .catch(() => {}); } /** @@ -1927,6 +1937,17 @@ export class Session implements SessionContext { }; await this.sendUpdate(update); + + // A2 (#4511): promote the mode change to the bridge side-channel so + // it reaches `approval_mode_changed` on the SSE bus, matching the + // extNotification in `setMode`. + void this.client + .extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: this.sessionId, + currentModeId: newModeId, + }) + .catch(() => {}); } /** diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 7da85e27e2a..1569e2acfe3 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2584,10 +2584,12 @@ export function createServeApp( let iter: AsyncIterator | undefined; const abort = new AbortController(); try { + const snapshot = req.query['snapshot'] === '1'; const iterable = bridge.subscribeEvents(sessionId, { signal: abort.signal, lastEventId, ...(maxQueued !== undefined ? { maxQueued } : {}), + ...(snapshot ? { snapshot: true } : {}), }); iter = iterable[Symbol.asyncIterator](); } catch (err) { diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 1e02981a2c8..4700edf0c7d 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -110,6 +110,11 @@ export const DAEMON_KNOWN_EVENT_TYPE_VALUES = [ 'turn_error', 'session_rewound', 'session_branched', + // A5 (#4511): synthetic side-channel snapshot yielded after + // `replay_complete` when `?snapshot=1` is set on the SSE endpoint. + // Carries `currentModelId` and `currentApprovalMode` so reconnecting + // clients can seed their reducer without an extra round-trip. + 'session_snapshot', ] as const; const DAEMON_KNOWN_EVENT_TYPES: ReadonlySet = new Set( @@ -686,6 +691,12 @@ export type DaemonMcpServerRemovedEvent = DaemonEventEnvelope< DaemonMcpServerRemovedData >; +export interface DaemonSessionSnapshotData { + sessionId: string; + currentModelId: string | null; + currentApprovalMode: string | null; + [key: string]: unknown; +} export type DaemonSessionUpdateEvent = DaemonEventEnvelope< 'session_update', DaemonSessionUpdateData @@ -824,6 +835,9 @@ export type DaemonTurnErrorEvent = DaemonEventEnvelope< export type DaemonSessionRewoundEvent = DaemonEventEnvelope< 'session_rewound', DaemonSessionRewoundData +export type DaemonSessionSnapshotEvent = DaemonEventEnvelope< + 'session_snapshot', + DaemonSessionSnapshotData >; export type DaemonSessionBranchedEvent = DaemonEventEnvelope< 'session_branched', @@ -909,7 +923,8 @@ export type KnownDaemonEvent = | DaemonWorkspaceMutationEvent | DaemonAuthEvent | DaemonAssistEvent - | DaemonTurnEvent; + | DaemonTurnEvent + | DaemonSessionSnapshotEvent; export interface DaemonSessionViewState { lastEventId?: number; @@ -1381,6 +1396,9 @@ export function asKnownDaemonEvent( case 'session_rewound': return isSessionRewoundData(event.data) ? (event as DaemonSessionRewoundEvent) + case 'session_snapshot': + return isSessionSnapshotData(event.data) + ? (event as DaemonSessionSnapshotEvent) : undefined; case 'session_branched': return isSessionBranchedData(event.data) @@ -1759,6 +1777,16 @@ export function reduceDaemonSessionEvent( ...base, rewindCount: base.rewindCount + 1, lastRewind: mergeOriginator(event.data, event), + case 'session_snapshot': + return { + ...base, + sessionId: event.data.sessionId, + ...(event.data.currentModelId != null + ? { currentModelId: event.data.currentModelId } + : {}), + ...(event.data.currentApprovalMode != null + ? { approvalMode: event.data.currentApprovalMode } + : {}), }; case 'session_branched': return { @@ -2513,6 +2541,10 @@ function isSessionBranchedData( isNonEmptyString(value['newSessionId']) && isNonEmptyString(value['displayName']) ); +function isSessionSnapshotData( + value: unknown, +): value is DaemonSessionSnapshotData { + return isRecord(value) && isNonEmptyString(value['sessionId']); } function isPermissionOption(value: unknown): value is DaemonPermissionOption { diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index 25e90d2e851..b3832e0fe70 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -251,6 +251,9 @@ export type { DaemonTurnCompleteEvent, DaemonTurnErrorData, DaemonTurnErrorEvent, + // A5 — side-channel session snapshot + DaemonSessionSnapshotData, + DaemonSessionSnapshotEvent, DaemonDeviceFlowReducerState, DaemonAuthState, KnownDaemonEvent, diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index a158b4dff79..424f1140a1c 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -2601,4 +2601,64 @@ describe('PR 21 — auth device-flow events', () => { expect(state.lastFollowupSuggestion).toBeUndefined(); }); }); + + describe('session_snapshot (A5 #4511)', () => { + it('asKnownDaemonEvent narrows session_snapshot', () => { + const event: DaemonEvent = { + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's-1', + currentModelId: 'qwen-turbo', + currentApprovalMode: 'auto', + }, + }; + const known = asKnownDaemonEvent(event); + expect(known).toBeDefined(); + expect(known!.type).toBe('session_snapshot'); + }); + + it('reducer seeds currentModelId and approvalMode from snapshot', () => { + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's-1', + currentModelId: 'qwen-turbo', + currentApprovalMode: 'yolo', + }, + }); + expect(state.sessionId).toBe('s-1'); + expect(state.currentModelId).toBe('qwen-turbo'); + expect(state.approvalMode).toBe('yolo'); + }); + + it('reducer does not overwrite model/mode with null snapshot values', () => { + const initial = { + ...createDaemonSessionViewState(), + currentModelId: 'existing-model', + approvalMode: 'default', + }; + const state = reduceDaemonSessionEvent(initial, { + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's-1', + currentModelId: null, + currentApprovalMode: null, + }, + }); + expect(state.currentModelId).toBe('existing-model'); + expect(state.approvalMode).toBe('default'); + }); + + it('drops malformed session_snapshot (missing sessionId)', () => { + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + v: 1, + type: 'session_snapshot', + data: { currentModelId: 'qwen-turbo' }, + }); + expect(state.unrecognizedKnownEventCount).toBe(1); + }); + }); }); From c9323dd64d5ca0e71efeddc72c9acbbcc452b3ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Fri, 29 May 2026 18:56:41 +0800 Subject: [PATCH 02/16] fix(daemon): address review on side-channel state consistency - inject session_snapshot up front on fresh SSE connections (not only on resume) - reconcile only after a roundtrip that landed; guard generation TOCTOU with one bounded re-run and log skip/correct/fail transitions - drop unencodable reconciliation_failed bus event in favor of operator log (client path already covered by state_resync_required) - bridgeClient mode fallback emits previous/persisted; dual-emit session_update uses the canonical nested data.update shape - validate modeId at the setMode boundary; reject unknown modes - SDK session_snapshot validator type-checks currentModelId/currentApprovalMode - tests: fresh-connection snapshot + reconciliation drift/match/failure/roundtrip-failure --- packages/acp-bridge/src/bridge.test.ts | 296 ++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 118 ++++--- packages/acp-bridge/src/bridgeClient.ts | 31 +- .../src/acp-integration/session/Session.ts | 22 +- packages/sdk-typescript/src/daemon/events.ts | 13 +- .../test/unit/daemonEvents.test.ts | 30 ++ 6 files changed, 469 insertions(+), 41 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 26f187d1341..78b3622cb8d 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -7940,6 +7940,302 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { abort.abort(); await bridge.shutdown(); }); + + it('yields session_snapshot up front on a fresh connection (no Last-Event-ID)', async () => { + // Regression for the A5 primary use case: a fresh attach has no + // `Last-Event-ID`, so the bus never emits `replay_complete` (the whole + // replay block is gated on `lastEventId !== undefined`). Keying the + // snapshot solely off `replay_complete` made it silently no-op exactly + // when a client most needs to seed state — on initial attach. + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-turbo', + }); + await new Promise((r) => setTimeout(r, 20)); + + // Fresh subscribe — snapshot=true, NO lastEventId. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + snapshot: true, + }); + + const it2 = iter[Symbol.asyncIterator](); + const first = await it2.next(); + // The very first frame must be the snapshot (no replay precedes it). + expect(first.value?.type).toBe('session_snapshot'); + expect( + (first.value?.data as { currentModelId: string | null }).currentModelId, + ).toBe('qwen-turbo'); + abort.abort(); + await bridge.shutdown(); + }); + }); + + describe('§2.2 — post-roundtrip reconciliation', () => { + const makeReconcileFactory = ( + sessionContextModelId: string | undefined, + opts: { throwOnStatus?: boolean } = {}, + ): ChannelFactory => async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + if (opts.throwOnStatus) { + throw new Error('status read failed'); + } + return Promise.resolve({ + state: { models: { currentModelId: sessionContextModelId } }, + }); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => ({}); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + + it('publishes a corrective model_switched when the agent state drifted from cache', async () => { + // Switch to qwen-max, but the agent's real state is qwen-turbo (e.g. + // an agent-side override). Reconciliation must emit a corrective + // model_switched so peers converge on the agent's truth. + const bridge = makeBridge({ + channelFactory: makeReconcileFactory('qwen-turbo'), + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ); + + const seen: Array<{ type: string; modelId?: string }> = []; + for await (const e of iter) { + seen.push({ + type: e.type, + modelId: (e.data as { modelId?: string })?.modelId, + }); + if (seen.filter((s) => s.type === 'model_switched').length === 2) break; + } + const switches = seen.filter((s) => s.type === 'model_switched'); + // First the requested change, then the corrective one from reconcile. + expect(switches[0]?.modelId).toBe('qwen-max'); + expect(switches[1]?.modelId).toBe('qwen-turbo'); + abort.abort(); + await bridge.shutdown(); + }); + + it('does NOT publish a corrective event when agent state matches cache', async () => { + // Stateful agent: `sessionContext` echoes the last model the bridge + // set, so reconciliation always finds cache == agent truth and never + // emits a corrective. Two distinct changes must therefore produce + // exactly two model_switched events, with no duplicates in between. + let lastModel: string | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + return Promise.resolve({ + state: { models: { currentModelId: lastModel } }, + }); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async (p: { modelId: string }) => { + lastModel = p.modelId; + return {}; + }; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ); + // Second distinct change terminates the iterator; a spurious + // corrective would surface as a duplicate model_switched. + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-plus' }, + undefined, + ); + + const switches: string[] = []; + for await (const e of iter) { + if (e.type === 'model_switched') { + switches.push((e.data as { modelId: string }).modelId); + if (switches.includes('qwen-plus')) break; + } + } + expect(switches).toEqual(['qwen-max', 'qwen-plus']); + abort.abort(); + await bridge.shutdown(); + }); + + it('swallows a failed status read without crashing or masking the original change', async () => { + const bridge = makeBridge({ + channelFactory: makeReconcileFactory(undefined, { + throwOnStatus: true, + }), + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await expect( + bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ), + ).resolves.toBeDefined(); + + const it2 = iter[Symbol.asyncIterator](); + const next = await it2.next(); + // The original model_switched is delivered; reconcile failure stays in + // the operator log (no bus event the SDK cannot decode). + expect(next.value?.type).toBe('model_switched'); + expect((next.value?.data as { modelId: string }).modelId).toBe( + 'qwen-max', + ); + abort.abort(); + await bridge.shutdown(); + }); + + it('does NOT reconcile when the model roundtrip itself fails', async () => { + // The agent's unstable_setSessionModel rejects, so publishModelSwitched + // never runs and the cache is unchanged. Reconciliation must be skipped + // (no status read), and the only bus event is model_switch_failed — + // never a corrective model_switched paired with the failure. + let statusReads = 0; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + statusReads += 1; + return Promise.resolve({ + state: { models: { currentModelId: 'qwen-turbo' } }, + }); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => { + throw new Error('agent refused model switch'); + }; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await expect( + bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ), + ).rejects.toThrow(); + + const it2 = iter[Symbol.asyncIterator](); + const next = await it2.next(); + expect(next.value?.type).toBe('model_switch_failed'); + // Give any (incorrectly) scheduled reconcile a tick to fire. + await new Promise((r) => setTimeout(r, 10)); + expect(statusReads).toBe(0); + abort.abort(); + await bridge.shutdown(); + }); }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index ca022956336..06222958528 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1773,28 +1773,45 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { target === 'model' ? 'modelReconciliationInFlight' : 'approvalModeReconciliationInFlight'; - if (entry[flagKey]) return; - entry[flagKey] = true; - const genBefore = + const genOf = () => target === 'model' ? entry.modelPublishGeneration : entry.approvalModePublishGeneration; + if (entry[flagKey]) return; + entry[flagKey] = true; + const genBefore = genOf(); + // Set when a newer change published while our status read was in + // flight; we re-run once after releasing the guard (see `finally`). + let rerun = false; try { const status = await requestSessionStatus( entry.sessionId, SERVE_STATUS_EXT_METHODS.sessionContext, ); - const genAfter = - target === 'model' - ? entry.modelPublishGeneration - : entry.approvalModePublishGeneration; - if (genAfter !== genBefore) return; + if (genOf() !== genBefore) { + // A newer change published during our RPC; its own + // `reconcileAfterRoundtrip` bailed on the in-flight guard above, + // so without a re-run the latest change would never be + // reconciled. Skip this (now-stale) read and re-run once. The + // re-run is gated on this generation-change signal — NOT on a + // bare `genOf() !== genBefore` at `finally` time — because a + // corrective publish below bumps the generation itself and would + // otherwise self-trigger an unbounded reconcile loop. + rerun = true; + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=${target} action=skipped reason=generation_changed genBefore=${genBefore} genAfter=${genOf()}`, + ); + return; + } if (target === 'model') { const actual = ( status?.state?.models as { currentModelId?: string } | undefined )?.currentModelId; if (actual && actual !== entry.currentModelId) { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=model action=corrected cached=${entry.currentModelId ?? ''} actual=${actual}`, + ); publishModelSwitched(entry, actual, undefined); } } else { @@ -1802,6 +1819,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { status?.state?.modes as { currentModeId?: string } | undefined )?.currentModeId; if (actual && actual !== entry.currentApprovalMode) { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=approvalMode action=corrected cached=${entry.currentApprovalMode ?? ''} actual=${actual}`, + ); publishApprovalModeChanged( entry, { @@ -1814,20 +1834,21 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } } } catch (err) { - try { - entry.events.publish({ - type: 'reconciliation_failed', - data: { - sessionId: entry.sessionId, - target, - error: err instanceof Error ? err.message : String(err), - }, - }); - } catch { - /* bus closed */ - } + // The status read failed — drift can be neither confirmed nor + // corrected. Keep the signal in the operator log rather than + // emitting a bus event no client can decode: `reconciliation_failed` + // is not a known SDK event type, so `asKnownDaemonEvent` drops it + // and the reducer never sees it. The client-facing "your state may + // be stale" path is already covered by `state_resync_required` on + // reconnect. + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=${target} action=failed error=${ + err instanceof Error ? err.message : String(err) + }`, + ); } finally { entry[flagKey] = false; + if (rerun) void reconcileAfterRoundtrip(entry, target); } }; @@ -2663,22 +2684,38 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (!subOpts?.snapshot) return raw; // A5: wrap the iterator to inject a synthetic `session_snapshot` - // frame immediately after `replay_complete`. Captures cached - // side-channel state synchronously at yield time so the client - // can seed its reducer without an extra round-trip. + // frame so a freshly attached / reconnecting client can seed its + // side-channel reducer without an extra round-trip. Captures cached + // state synchronously at yield time. + // + // The bus only emits `replay_complete` on the `Last-Event-ID` + // resume path (`eventBus.subscribe` gates the whole replay block on + // `opts.lastEventId !== undefined`). A fresh connection has no + // `Last-Event-ID`, so it never sees `replay_complete` — keying the + // snapshot solely off that sentinel silently no-ops on the primary + // use case (initial attach). So inject up front when there is no + // resume cursor, and otherwise after `replay_complete` so the + // client applies replayed deltas before the snapshot seeds state. + const snapshotFrame = (): BridgeEvent => ({ + v: EVENT_SCHEMA_VERSION, + type: 'session_snapshot', + data: { + sessionId: entry!.sessionId, + currentModelId: entry!.currentModelId ?? null, + currentApprovalMode: entry!.currentApprovalMode ?? null, + }, + }); async function* withSnapshot(): AsyncIterable { + let injected = false; + if (subOpts?.lastEventId === undefined) { + yield snapshotFrame(); + injected = true; + } for await (const event of raw) { yield event; - if (event.type === 'replay_complete') { - yield { - v: EVENT_SCHEMA_VERSION, - type: 'session_snapshot', - data: { - sessionId: entry!.sessionId, - currentModelId: entry!.currentModelId ?? null, - currentApprovalMode: entry!.currentApprovalMode ?? null, - }, - }; + if (!injected && event.type === 'replay_complete') { + yield snapshotFrame(); + injected = true; } } } @@ -3386,6 +3423,12 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `applyModelServiceId`, so the agent notification can never slip // through after the flag clears even if transport ordering changes. entry.modelRoundtripInFlight = true; + // Only reconcile after a change that actually landed. If the + // roundtrip rejects (timeout / transport close) `publishModelSwitched` + // never ran and the cache is unchanged, so a reconcile would just emit + // a confusing corrective `model_switched` alongside the + // `model_switch_failed` the catch block already publishes. + let succeeded = false; try { const result = await Promise.race([ withTimeout( @@ -3396,10 +3439,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { transportClosed, ]); publishModelSwitched(entry, req.modelId, originatorClientId); + succeeded = true; return result; } finally { entry.modelRoundtripInFlight = false; - void reconcileAfterRoundtrip(entry, 'model'); + if (succeeded) void reconcileAfterRoundtrip(entry, 'model'); } }); // Tail-swallow on the queue so a model-change failure doesn't poison @@ -3477,6 +3521,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // The flag stays true through persist + publish so the notification // cannot slip through during the persist phase (review finding #3). entry.approvalModeRoundtripInFlight = true; + // See setSessionModel: only reconcile after a change that landed, so + // a rejected roundtrip can't pair a corrective event with the failure. + let succeeded = false; try { const response = (await Promise.race([ withTimeout( @@ -3536,6 +3583,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { entry.sessionId, ); } + succeeded = true; return { sessionId: entry.sessionId, mode: response.current, @@ -3544,7 +3592,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }; } finally { entry.approvalModeRoundtripInFlight = false; - void reconcileAfterRoundtrip(entry, 'approvalMode'); + if (succeeded) void reconcileAfterRoundtrip(entry, 'approvalMode'); } }); // Tail-swallow so a failed change doesn't poison subsequent ones. diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 83c7d108a79..c23c35ee6d2 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -619,9 +619,23 @@ export class BridgeClient implements Client { ); } else { try { + // Fallback path (no `onModePromoted` injected — tests / non-bridge + // consumers; production always wires the bridge callback). Mirror + // the main path's full payload: the SDK's + // `isApprovalModeChangedData` requires `previous` (non-empty + // string) and `persisted` (boolean), so a `{ sessionId, next }` + // shape fails validation and `asKnownDaemonEvent` drops the event. + // `previous` is unavailable on this path (the cache lives on the + // bridge's `SessionEntry`, not the demux interface), so seed it + // with the protocol default. entry.events.publish({ type: 'approval_mode_changed', - data: { sessionId, next: currentModeId }, + data: { + sessionId, + previous: 'default', + next: currentModeId, + persisted: false, + }, ...(entry.activePromptOriginatorClientId ? { originatorClientId: entry.activePromptOriginatorClientId } : {}), @@ -635,13 +649,24 @@ export class BridgeClient implements Client { // VS Code IDE companion's existing `case 'current_mode_update'` // handler keeps working. Remove this block (and its tracking issue) // once the companion ships an `approval_mode_changed` handler. + // + // Use the canonical ACP-nested shape (`data.update.sessionUpdate`), + // matching what `BridgeClient.sessionUpdate` publishes for a real + // `current_mode_update` notification. A flat + // `{ sessionId, sessionUpdate, currentModeId }` would (a) not be + // recognised by the companion's standard `data.update.sessionUpdate` + // switch, and (b) collide structurally with the real `session_update` + // the agent already emits on the `exit_plan_mode` path — leaving two + // incompatible shapes on the bus for one change. try { entry.events.publish({ type: 'session_update', data: { sessionId, - sessionUpdate: 'current_mode_update', - currentModeId, + update: { + sessionUpdate: 'current_mode_update', + currentModeId, + }, }, ...(entry.activePromptOriginatorClientId ? { originatorClientId: entry.activePromptOriginatorClientId } diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 883d6589a2e..948a33998e9 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1812,7 +1812,19 @@ export class Session implements SessionContext { yolo: ApprovalMode.YOLO, }; + // `modeId` arrives over the wire (ACP `session/set_mode`, or + // `setSessionConfigOption` casting an unknown `value` to string), so + // validate at this boundary. An unknown id would otherwise call + // `setApprovalMode(undefined)` — leaving the permission system in an + // undefined state — and the A2 broadcast below would fan the bogus id + // out to every attached SSE client. const approvalMode = modeMap[params.modeId as ApprovalModeValue]; + if (approvalMode === undefined) { + throw RequestError.invalidParams( + undefined, + `Unknown approval mode: ${params.modeId}`, + ); + } this.config.setApprovalMode(approvalMode); // A2 (#4511): notify attached clients of an in-session mode switch. @@ -1823,7 +1835,10 @@ export class Session implements SessionContext { sessionId: this.sessionId, currentModeId: params.modeId, }) - .catch(() => {}); + .catch(() => { + // Advisory only; a failed notification must not fail the mode + // switch. Matches the model-update extNotification in `setModel`. + }); } /** @@ -1947,7 +1962,10 @@ export class Session implements SessionContext { sessionId: this.sessionId, currentModeId: newModeId, }) - .catch(() => {}); + .catch(() => { + // Advisory only; a failed notification must not fail the mode + // change. Matches the model-update extNotification in `setModel`. + }); } /** diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 4700edf0c7d..016b59a4d07 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -2544,7 +2544,18 @@ function isSessionBranchedData( function isSessionSnapshotData( value: unknown, ): value is DaemonSessionSnapshotData { - return isRecord(value) && isNonEmptyString(value['sessionId']); + // `currentModelId` / `currentApprovalMode` are `string | null` on the + // wire. Validate the types here, not just `sessionId`: the reducer + // propagates these into `state.currentModelId` / `state.approvalMode` + // on a `!= null` check alone, so an unchecked non-string (e.g. `42`, + // `{}`) would land in state and crash downstream `.trim()`-style calls. + if (!isRecord(value) || !isNonEmptyString(value['sessionId'])) return false; + const model = value['currentModelId']; + const mode = value['currentApprovalMode']; + return ( + (model === null || typeof model === 'string') && + (mode === null || typeof mode === 'string') + ); } function isPermissionOption(value: unknown): value is DaemonPermissionOption { diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index 424f1140a1c..b740cc7b7e3 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -2660,5 +2660,35 @@ describe('PR 21 — auth device-flow events', () => { }); expect(state.unrecognizedKnownEventCount).toBe(1); }); + + it('drops session_snapshot with a non-string currentModelId', () => { + // Guards the reducer's `!= null` propagation: an unchecked non-string + // would land in `state.currentModelId` and crash downstream string ops. + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's1', + currentModelId: 42 as unknown as string, + currentApprovalMode: null, + }, + }); + expect(state.unrecognizedKnownEventCount).toBe(1); + expect(state.currentModelId).toBeUndefined(); + }); + + it('drops session_snapshot with a non-string currentApprovalMode', () => { + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's1', + currentModelId: null, + currentApprovalMode: {} as unknown as string, + }, + }); + expect(state.unrecognizedKnownEventCount).toBe(1); + expect(state.approvalMode).toBeUndefined(); + }); }); }); From 939e49bef3c0dc27b20880ef4f3c3d7d957a332e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sat, 30 May 2026 11:02:30 +0800 Subject: [PATCH 03/16] fix(daemon): address second-round review on side-channel state layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - applyModelServiceId: gate reconcile behind a `succeeded` flag so a rejected create/attach-time roundtrip can't pair a corrective model_switched with the model_switch_failed it just published; mirrors setSessionModel / setSessionApprovalMode. - in-session mode demux: validate currentModeId against the known approval-mode enum (lockstep with Session.setMode) before it fans out to SSE clients / the SDK reducer. - in-session mode demux: suppress the legacy session_update dual-emit on the exit_plan_mode path via a `legacyFrameSent` flag — sendUpdate already published that frame, so dual-emitting delivered it twice. The setMode path (no sendUpdate) keeps its dual-emit. - reconcile: emit a `reason=roundtrip_failed` skip log on all three failure paths so the skipped reconcile is greppable. - SDK: add session_snapshot to RESYNC_PASSTHROUGH_TYPES so a client that reconnects past ring eviction recovers currentModelId / approvalMode from the full-state frame instead of staying stale until loadSession. - tests: approvalMode reconcile drift + roundtrip-fail, generation rerun, unknown-mode enum drop, dual-emit shape + suppression, setMode extNotification + unknown-modeId rejection. Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 392 +++++++++++++++++- packages/acp-bridge/src/bridge.ts | 30 +- packages/acp-bridge/src/bridgeClient.ts | 40 ++ .../acp-integration/session/Session.test.ts | 32 ++ .../src/acp-integration/session/Session.ts | 8 + packages/sdk-typescript/src/daemon/events.ts | 5 + 6 files changed, 500 insertions(+), 7 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 78b3622cb8d..7c81821fde8 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -7825,6 +7825,168 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { abort.abort(); await bridge.shutdown(); }); + + it('drops a current_mode_update with an unknown mode id (enum guard)', async () => { + // The agent can reach this receive path without `Session.setMode`'s + // enum validation, so a bogus mode id must be dropped here before it + // fans out to SSE clients / the SDK reducer's state.approvalMode. + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + // Well-formed string, but not a known approval mode. + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'totally-bogus', + }); + await new Promise((r) => setTimeout(r, 50)); + + // A known good model-update breaks the iterator; the bogus mode must + // not have produced an approval_mode_changed (or a legacy dual-emit). + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-max', + }); + + const seen: string[] = []; + for await (const e of iter) { + seen.push(e.type); + if (e.type === 'model_switched') break; + } + expect(seen.filter((t) => t === 'approval_mode_changed')).toEqual([]); + expect(seen.filter((t) => t === 'session_update')).toEqual([]); + abort.abort(); + await bridge.shutdown(); + }); + + it('dual-emits a legacy session_update on the setMode path (no legacyFrameSent)', async () => { + // The ACP `session/set_mode` path has no `sendUpdate`, so the demux + // owns the IDE-companion compat frame: one approval_mode_changed plus + // one legacy session_update{current_mode_update}. + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'auto-edit', + }); + + const collected: Array<{ type: string; data: unknown }> = []; + for await (const e of iter) { + collected.push({ type: e.type, data: e.data }); + if (e.type === 'session_update') break; + } + expect(collected.map((c) => c.type)).toEqual([ + 'approval_mode_changed', + 'session_update', + ]); + // Canonical ACP-nested shape so the companion's standard + // data.update.sessionUpdate switch recognises it. + const update = ( + collected[1]?.data as { + update?: { sessionUpdate?: string; currentModeId?: string }; + } + ).update; + expect(update?.sessionUpdate).toBe('current_mode_update'); + expect(update?.currentModeId).toBe('auto-edit'); + abort.abort(); + await bridge.shutdown(); + }); + + it('suppresses the legacy dual-emit when legacyFrameSent is true (exit_plan_mode path)', async () => { + // `Session.sendCurrentModeUpdateNotification` already published the + // legacy session_update via `sendUpdate` before this extNotification, + // so the demux must promote to approval_mode_changed only — emitting + // its own dual-emit would deliver the legacy frame to the companion + // twice for one mode change. + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'auto-edit', + legacyFrameSent: true, + }); + await new Promise((r) => setTimeout(r, 50)); + + // A known good model-update breaks the iterator; assert exactly one + // approval_mode_changed and NO legacy session_update from this path. + void capturedConn!.extNotification('qwen/notify/session/model-update', { + v: 1, + sessionId: session.sessionId, + currentModelId: 'qwen-max', + }); + + const seen: string[] = []; + for await (const e of iter) { + seen.push(e.type); + if (e.type === 'model_switched') break; + } + expect(seen.filter((t) => t === 'approval_mode_changed')).toHaveLength(1); + expect(seen.filter((t) => t === 'session_update')).toEqual([]); + abort.abort(); + await bridge.shutdown(); + }); }); describe('A5 — session snapshot on attach', () => { @@ -7992,10 +8154,12 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { }); describe('§2.2 — post-roundtrip reconciliation', () => { - const makeReconcileFactory = ( - sessionContextModelId: string | undefined, - opts: { throwOnStatus?: boolean } = {}, - ): ChannelFactory => async () => { + const makeReconcileFactory = + ( + sessionContextModelId: string | undefined, + opts: { throwOnStatus?: boolean } = {}, + ): ChannelFactory => + async () => { const { clientStream, agentStream } = createInMemoryChannel(); const fakeAgent = new FakeAgent({ extMethodImpl: (method) => { @@ -8236,6 +8400,226 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { abort.abort(); await bridge.shutdown(); }); + + it('re-runs reconcile when a newer change publishes during the status read (generation rerun)', async () => { + // Anti-lost-reconcile: while reconcile for change A awaits its status + // RPC, a second change B publishes (bumping the generation). B's own + // reconcile bails on the in-flight guard, so without the `rerun` path + // B would never be reconciled. Gate the FIRST status read until B has + // published; assert the FIRST read is discarded (generation changed) + // and a SECOND read fires after the guard releases, whose corrective + // reflects the agent's truth read AFTER B — not a stale read for A. + let statusReads = 0; + let releaseFirstStatus: (() => void) | undefined; + const firstStatusGate = new Promise((res) => { + releaseFirstStatus = res; + }); + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + statusReads += 1; + // Agent truth drifts from both A and B, so the post-rerun + // read produces an observable corrective. + const payload = { + state: { models: { currentModelId: 'qwen-turbo' } }, + }; + return statusReads === 1 + ? firstStatusGate.then(() => payload) + : Promise.resolve(payload); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => ({}); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + // A: publishes gen=1; its reconcile starts and blocks on the gate. + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-max' }, + undefined, + ); + // B: publishes gen=2 while A's reconcile is still awaiting the gated + // status read; B's own reconcile bails on the in-flight guard. + await bridge.setSessionModel( + session.sessionId, + { sessionId: session.sessionId, modelId: 'qwen-plus' }, + undefined, + ); + // Now let A's status read resolve — it must detect the generation + // change, discard its (stale) read, and re-run. + releaseFirstStatus!(); + + const switches: string[] = []; + for await (const e of iter) { + if (e.type === 'model_switched') { + switches.push((e.data as { modelId: string }).modelId); + if (switches.includes('qwen-turbo')) break; + } + } + // The two requested changes, then ONE corrective from the rerun. + expect(switches).toEqual(['qwen-max', 'qwen-plus', 'qwen-turbo']); + // Two reads total: the gated (discarded) one + the rerun. + expect(statusReads).toBe(2); + abort.abort(); + await bridge.shutdown(); + }); + + it('publishes a corrective approval_mode_changed when the agent mode drifted from cache', async () => { + // approvalMode analog of the model drift test. The bridge sets YOLO, + // but the agent's real mode is `plan` (e.g. an agent-side exit_plan_mode + // restore). Reconciliation reads `state.modes.currentModeId` — a + // DIFFERENT status shape from the model branch — and must emit a + // corrective approval_mode_changed with next:'plan' so peers converge + // on the agent's truth. + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method, params) => { + if (method === 'qwen/control/session/approval_mode') { + return Promise.resolve({ + previous: 'default', + current: (params as { mode: string }).mode, + }); + } + if (method === 'qwen/status/session/context') { + return Promise.resolve({ + state: { modes: { currentModeId: 'plan' } }, + }); + } + return Promise.resolve({}); + }, + }); + new AgentSideConnection(() => fakeAgent as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + await bridge.setSessionApprovalMode( + session.sessionId, + ApprovalMode.YOLO, + { persist: false }, + undefined, + ); + + const nexts: string[] = []; + for await (const e of iter) { + if (e.type === 'approval_mode_changed') { + nexts.push((e.data as { next: string }).next); + if (nexts.length === 2) break; + } + } + // First the requested change, then the corrective one from reconcile. + expect(nexts[0]).toBe('yolo'); + expect(nexts[1]).toBe('plan'); + abort.abort(); + await bridge.shutdown(); + }); + + it('does NOT reconcile when the approval-mode roundtrip itself fails', async () => { + // approvalMode analog of the model roundtrip-fail test. The agent's + // approval_mode ext rejects, so publishApprovalModeChanged never runs + // and the cache is unchanged. Reconciliation must be skipped (no status + // read) and no corrective approval_mode_changed must reach the bus. + let statusReads = 0; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/control/session/approval_mode') { + throw new Error('agent refused approval-mode switch'); + } + if (method === 'qwen/status/session/context') { + statusReads += 1; + return Promise.resolve({ + state: { modes: { currentModeId: 'plan' } }, + }); + } + return Promise.resolve({}); + }, + }); + new AgentSideConnection(() => fakeAgent as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const nexts: string[] = []; + const collecting = (async () => { + for await (const e of iter) { + if (e.type === 'approval_mode_changed') { + nexts.push((e.data as { next: string }).next); + } + } + })(); + + await expect( + bridge.setSessionApprovalMode( + session.sessionId, + ApprovalMode.YOLO, + { persist: false }, + undefined, + ), + ).rejects.toThrow(); + + // Give any (incorrectly) scheduled reconcile a tick to fire. + await new Promise((r) => setTimeout(r, 10)); + expect(statusReads).toBe(0); + expect(nexts).toEqual([]); + abort.abort(); + await collecting; + await bridge.shutdown(); + }); }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 06222958528..5ae7c6ec977 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1440,6 +1440,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `Session.setModel`, which emits it) is suppressed by the demux — // the authoritative `model_switched` is published below. entry.modelRoundtripInFlight = true; + // Mirror setSessionModel: only reconcile after a change that landed. A + // rejected roundtrip leaves the cache unchanged (often still unset on + // the create/attach path), so reconciling would emit a corrective + // model_switched right beside the model_switch_failed below. + let succeeded = false; try { await Promise.race([ withTimeout( @@ -1453,6 +1458,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { transportClosed, ]); publishModelSwitched(entry, modelId, originatorClientId); + succeeded = true; } catch (err) { // Surface the failure to ALL attached clients, not just the // caller — a shared session swallowing a denied model change @@ -1473,7 +1479,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { throw err; } finally { entry.modelRoundtripInFlight = false; - void reconcileAfterRoundtrip(entry, 'model'); + if (succeeded) { + void reconcileAfterRoundtrip(entry, 'model'); + } else { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=model action=skipped reason=roundtrip_failed`, + ); + } } }); // Tail swallows failures so subsequent model changes still run; the @@ -3443,7 +3455,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { return result; } finally { entry.modelRoundtripInFlight = false; - if (succeeded) void reconcileAfterRoundtrip(entry, 'model'); + if (succeeded) { + void reconcileAfterRoundtrip(entry, 'model'); + } else { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=model action=skipped reason=roundtrip_failed`, + ); + } } }); // Tail-swallow on the queue so a model-change failure doesn't poison @@ -3592,7 +3610,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }; } finally { entry.approvalModeRoundtripInFlight = false; - if (succeeded) void reconcileAfterRoundtrip(entry, 'approvalMode'); + if (succeeded) { + void reconcileAfterRoundtrip(entry, 'approvalMode'); + } else { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=approvalMode action=skipped reason=roundtrip_failed`, + ); + } } }); // Tail-swallow so a failed change doesn't poison subsequent ones. diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index c23c35ee6d2..c8fdb5a0d03 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -119,6 +119,18 @@ const MAX_EARLY_EVENTS_PER_SESSION = 32; const MAX_SUGGESTION_LENGTH = 500; const EARLY_EVENT_TTL_MS = 60_000; +// Known approval-mode ids accepted on the in-session `current_mode_update` +// demux path. Mirrors the `modeMap` keys in `Session.setMode` (CLI); an id +// outside this set is dropped before it fans out to SSE clients / the SDK +// reducer. Keep the two in lockstep. +const KNOWN_APPROVAL_MODES: ReadonlySet = new Set([ + 'plan', + 'default', + 'auto-edit', + 'auto', + 'yolo', +]); + /** * Human-readable label for a `fs.Stats` object's kind, used in the * `readTextFile` "not a regular file" rejection message (BX8YO). @@ -598,6 +610,20 @@ export class BridgeClient implements Client { if (typeof sessionId !== 'string' || typeof currentModeId !== 'string') { return; } + // Validate against the known approval-mode enum before it fans out. + // `Session.setMode` guards the symmetric send path with the same set + // ("an unknown id would call setApprovalMode(undefined), leaving the + // permission system undefined"); this is the receive path the agent + // can reach without that validation, so an unknown id here would + // propagate through `approval_mode_changed` to every SSE client and + // land in the SDK reducer's `state.approvalMode`. Keep in lockstep + // with `Session.setMode`'s `modeMap` keys (includes `auto`). + if (!KNOWN_APPROVAL_MODES.has(currentModeId)) { + writeStderrLine( + `[demux] session=${sessionId} type=current_mode_update action=dropped reason=unknown_mode mode=${currentModeId}`, + ); + return; + } const entry = this.resolveEntry(sessionId); if (!entry) { writeStderrLine( @@ -650,6 +676,14 @@ export class BridgeClient implements Client { // handler keeps working. Remove this block (and its tracking issue) // once the companion ships an `approval_mode_changed` handler. // + // Skip it when the producer already sent the legacy frame itself: the + // `exit_plan_mode` path (`Session.sendCurrentModeUpdateNotification`) + // calls `sendUpdate` before this extNotification, which + // `BridgeClient.sessionUpdate` already fanned onto the bus as the same + // `session_update{current_mode_update}` frame. Dual-emitting here would + // deliver it twice. The `setMode` path omits the flag (it has no + // `sendUpdate`), so its dual-emit still fires. + // // Use the canonical ACP-nested shape (`data.update.sessionUpdate`), // matching what `BridgeClient.sessionUpdate` publishes for a real // `current_mode_update` notification. A flat @@ -658,6 +692,12 @@ export class BridgeClient implements Client { // switch, and (b) collide structurally with the real `session_update` // the agent already emits on the `exit_plan_mode` path — leaving two // incompatible shapes on the bus for one change. + if (params['legacyFrameSent'] === true) { + writeStderrLine( + `[demux] session=${sessionId} type=current_mode_update action=promoted mode=${currentModeId} legacy_frame=skipped`, + ); + return; + } try { entry.events.publish({ type: 'session_update', diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 7fb252dcbcc..13c0fbf2cf3 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -352,6 +352,38 @@ describe('Session', () => { expect(mockConfig.setApprovalMode).toHaveBeenCalledWith(expected); }); + + it('emits a current_mode_update extNotification after switching (A2)', async () => { + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'auto-edit', + }); + + expect(mockClient.extNotification).toHaveBeenCalledWith( + 'qwen/notify/session/mode-update', + expect.objectContaining({ + v: 1, + sessionId: 'test-session-id', + currentModeId: 'auto-edit', + }), + ); + }); + + it('rejects an unknown modeId and does NOT touch approval mode (A2)', async () => { + await expect( + session.setMode({ + sessionId: 'test-session-id', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + modeId: 'totally-bogus' as any, + }), + ).rejects.toThrow(/Unknown approval mode/); + + expect(mockConfig.setApprovalMode).not.toHaveBeenCalled(); + expect(mockClient.extNotification).not.toHaveBeenCalledWith( + 'qwen/notify/session/mode-update', + expect.anything(), + ); + }); }); describe('rewindToTurn', () => { diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 948a33998e9..3afaed28bb7 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1956,11 +1956,19 @@ export class Session implements SessionContext { // A2 (#4511): promote the mode change to the bridge side-channel so // it reaches `approval_mode_changed` on the SSE bus, matching the // extNotification in `setMode`. + // + // Unlike `setMode`, this path already published the legacy + // `session_update{current_mode_update}` frame via `sendUpdate` above + // (BridgeClient.sessionUpdate fans it onto the bus). Tell the demux to + // skip its compat dual-emit so the IDE companion sees exactly one + // legacy frame for this change, not two. `setMode` omits the flag, so + // its dual-emit still fires (it has no `sendUpdate`). void this.client .extNotification('qwen/notify/session/mode-update', { v: 1, sessionId: this.sessionId, currentModeId: newModeId, + legacyFrameSent: true, }) .catch(() => { // Advisory only; a failed notification must not fail the mode diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 016b59a4d07..7d73012faa8 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -1149,6 +1149,11 @@ const RESYNC_PASSTHROUGH_TYPES = new Set([ 'session_closed', 'client_evicted', 'stream_error', + // A5 (#4511): the snapshot is a full-state authoritative frame, not a + // delta, so it is safe to apply during resync — and it is exactly what + // lets a client that reconnected past the ring recover currentModelId / + // approvalMode without waiting for the next loadSession. + 'session_snapshot', ]); export function createDaemonSessionViewState( From 0a69e4d1d91b69c1bd52dbfa46f3085be9bdb71a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sat, 30 May 2026 16:50:01 +0800 Subject: [PATCH 04/16] test(daemon): assert currentApprovalMode flows into the A5 snapshot The existing A5 snapshot tests only seed currentModelId, leaving the publishApprovalModeChanged -> entry.currentApprovalMode -> snapshot pipeline uncovered at the bridge level. Add a test that promotes an in-session mode change before subscribing and asserts the snapshot carries the non-null currentApprovalMode. Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 54 ++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 7c81821fde8..05a40a12c5d 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -8042,6 +8042,60 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { await bridge.shutdown(); }); + it('carries currentApprovalMode in the snapshot when an approval-mode change was promoted', async () => { + // The other A5 tests only seed currentModelId, so the + // publishApprovalModeChanged → entry.currentApprovalMode → snapshot + // pipeline is otherwise untested at the bridge level: a typo writing + // the wrong field would leave currentApprovalMode null and slip past. + let capturedConn: AgentSideConnection | undefined; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent(); + capturedConn = new AgentSideConnection(() => fakeAgent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Promote an in-session mode change to populate currentApprovalMode + // (flows through onModePromoted → publishApprovalModeChanged). + void capturedConn!.extNotification('qwen/notify/session/mode-update', { + v: 1, + sessionId: session.sessionId, + currentModeId: 'auto-edit', + }); + await new Promise((r) => setTimeout(r, 20)); + + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + lastEventId: 0, + snapshot: true, + }); + + const collected: BridgeEvent[] = []; + for await (const e of iter) { + collected.push(e); + if (e.type === 'session_snapshot') break; + } + const snap = collected.find((e) => e.type === 'session_snapshot'); + expect(snap).toBeDefined(); + expect( + (snap!.data as { currentApprovalMode: string | null }) + .currentApprovalMode, + ).toBe('auto-edit'); + abort.abort(); + await bridge.shutdown(); + }); + it('does NOT yield session_snapshot when snapshot is not set', async () => { let capturedConn: AgentSideConnection | undefined; const factory: ChannelFactory = async () => { From 26fdd7196423d9e6e0a5c69e7ad976bda3bef112 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sat, 30 May 2026 17:39:18 +0800 Subject: [PATCH 05/16] docs(bridge)+test(cli): clarify mode-update handler comment & cover legacyFrameSent - bridgeClient.ts: the A2 comment claimed handleInSessionModeUpdate "mirrors handleInSessionModelUpdate exactly", but it diverges with enum validation and the legacy dual-emit. Reword to state the shared suppression pattern plus the two additions. - Session.test.ts: add coverage for sendCurrentModeUpdateNotification asserting the extNotification carries legacyFrameSent: true, so a regression dropping it (double legacy frame to the IDE companion) is caught. Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridgeClient.ts | 13 ++++---- .../acp-integration/session/Session.test.ts | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index c8fdb5a0d03..0552610d36a 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -598,11 +598,14 @@ export class BridgeClient implements Client { /** * A2: promote an in-session `current_mode_update` extNotification to - * `approval_mode_changed`. Mirrors `handleInSessionModelUpdate` exactly: - * suppressed while the bridge is driving its own approval-mode roundtrip - * (`entry.approvalModeRoundtripInFlight`). Additionally emits a legacy - * `session_update{current_mode_update}` for IDE companion compat - * (dual-emit transition — see §6 of the design doc). + * `approval_mode_changed`. Uses the same suppression pattern as + * `handleInSessionModelUpdate` — suppressed while the bridge is driving + * its own approval-mode roundtrip (`entry.approvalModeRoundtripInFlight`) + * — but diverges with two additions the model handler lacks: enum + * validation against `KNOWN_APPROVAL_MODES`, and a legacy + * `session_update{current_mode_update}` dual-emit for IDE companion + * compat (transition — see §6 of the design doc), itself deduped via the + * `legacyFrameSent` flag. */ private handleInSessionModeUpdate(params: Record): void { const sessionId = params['sessionId']; diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 13c0fbf2cf3..272f9c5e109 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -386,6 +386,36 @@ describe('Session', () => { }); }); + describe('sendCurrentModeUpdateNotification', () => { + // The exit_plan_mode / edit-ProceedAlways path publishes the legacy + // `session_update{current_mode_update}` frame itself (via sendUpdate), + // so its extNotification must carry `legacyFrameSent: true` to stop the + // bridge demux from emitting a second, duplicate legacy frame. Unlike + // `setMode` (which omits the flag), a regression dropping it here would + // double-publish to the IDE companion. (A2) + it('marks the extNotification legacyFrameSent so the demux skips its dual-emit', async () => { + await ( + session as unknown as { + sendCurrentModeUpdateNotification: ( + outcome: core.ToolConfirmationOutcome, + ) => Promise; + } + ).sendCurrentModeUpdateNotification( + core.ToolConfirmationOutcome.ProceedAlways, + ); + + expect(mockClient.extNotification).toHaveBeenCalledWith( + 'qwen/notify/session/mode-update', + expect.objectContaining({ + v: 1, + sessionId: 'test-session-id', + currentModeId: 'auto-edit', + legacyFrameSent: true, + }), + ); + }); + }); + describe('rewindToTurn', () => { it('truncates model history before the requested user turn and records rewind', () => { const history: Content[] = [ From 110602051239d25e8947ddbbfd8ba49870fe0008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sun, 31 May 2026 15:17:02 +0800 Subject: [PATCH 06/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-5=20review=20=E2=80=94=20cache=20seeding,=20peer=20sync,=20c?= =?UTF-8?q?ontract=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge: seed snapshot caches (currentModelId/currentApprovalMode) from newSession/loadSession responses so a cold attach reports real state instead of null/null, with KNOWN_APPROVAL_MODES enum backstop - bridge: enum-validate the reconcile approvalMode branch and drop unknown modes with a logged reason - bridge: on a persisted approval-mode change, mirror the new workspace default into every peer SessionEntry cache so their GET status / session_snapshot stop reporting the pre-change mode - bridge/bridgeClient: remove try/catch wrappers around EventBus.publish() per its documented never-throws contract; drop misleading "bus closed" comments - cli/Session: log dropped advisory extNotifications via debugLogger.debug instead of swallowing silently - bridge.test: add failure-gating coverage for applyModelServiceId — a rejected attach-time model apply must not trigger reconcile (no status read) Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 71 ++++++++ packages/acp-bridge/src/bridge.ts | 157 ++++++++++++------ packages/acp-bridge/src/bridgeClient.ts | 105 ++++++------ .../src/acp-integration/session/Session.ts | 9 +- 4 files changed, 231 insertions(+), 111 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 05a40a12c5d..47d15daeb88 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -3856,6 +3856,77 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('does NOT reconcile when applyModelServiceId roundtrip fails on attach', async () => { + // F4oaj: the attach-time model apply (`applyModelServiceId`) gates + // reconcile on the same `succeeded` flag as `setSessionModel`. When the + // agent rejects `unstable_setSessionModel`, `publishModelSwitched` never + // runs and the cache is unchanged, so reconciliation must be skipped (no + // status read) — otherwise a corrective `model_switched` would be paired + // with the `model_switch_failed`. The agent's status deliberately drifts + // so any (incorrect) reconcile would produce an observable corrective. + let statusReads = 0; + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + statusReads += 1; + return Promise.resolve({ + state: { models: { currentModelId: 'qwen-turbo' } }, + }); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => { + throw new Error('agent denied'); + }; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + // Spawn WITHOUT a model so the only model apply is the failing one on the + // second attach (a spawn-time apply would succeed and legitimately read + // status, muddying the assertion). + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + + // Attach with a model — the agent rejects it. The attach swallows the + // failure (shared session stays alive) and surfaces it as a bus event. + await bridge.spawnOrAttach({ + workspaceCwd: WS_A, + modelServiceId: 'rejected', + }); + + const it = iter[Symbol.asyncIterator](); + const failed = await it.next(); + expect(failed.value?.type).toBe('model_switch_failed'); + // Give any (incorrectly) scheduled reconcile a tick to fire. + await new Promise((r) => setTimeout(r, 10)); + expect(statusReads).toBe(0); + abort.abort(); + await bridge.shutdown(); + }); + it('serializes concurrent model-change calls (FIFO)', async () => { const callOrder: string[] = []; const factory: ChannelFactory = async () => { diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 5ae7c6ec977..8ee3c5095ea 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -78,7 +78,7 @@ import type { BridgeOptions, BridgeTelemetry } from './bridgeOptions.js'; import { MCP_RESTART_SERVER_DEADLINE_MS } from './mcpTimeouts.js'; import { defaultSpawnChannelFactory } from './spawnChannel.js'; import { writeStderrLine } from './internal/stderrLine.js'; -import { BridgeClient } from './bridgeClient.js'; +import { BridgeClient, KNOWN_APPROVAL_MODES } from './bridgeClient.js'; import { CANCEL_VOTE_SENTINEL, createNoOpPermissionAuditPublisher, @@ -1305,7 +1305,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `ensureChannel`, never spawning a fresh one. Tear down the // empty channel so the next attempt gets a clean spawn. const ci = await ensureChannel(); - let newSessionResp: { sessionId: string }; + let newSessionResp: { + sessionId: string; + models?: { currentModelId?: unknown } | null; + modes?: { currentModeId?: unknown } | null; + }; try { newSessionResp = await telemetry.withSpan( 'session.new', @@ -1354,6 +1358,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { newSessionResp.sessionId, boundWorkspace, ); + seedSnapshotCaches(entry, newSessionResp); const clientId = registerClient(entry, requestedClientId); // `defaultEntry` is the single-scope attach target — only sessions // SPAWNED UNDER `'single'` may claim it. A thread-scope spawn must @@ -1462,20 +1467,17 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } catch (err) { // Surface the failure to ALL attached clients, not just the // caller — a shared session swallowing a denied model change - // silently would surprise the others. - try { - entry.events.publish({ - type: 'model_switch_failed', - data: { - sessionId: entry.sessionId, - requestedModelId: modelId, - error: err instanceof Error ? err.message : String(err), - }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed */ - } + // silently would surprise the others. `publish()` never throws + // (see `publishModelSwitched`), so no wrapper. + entry.events.publish({ + type: 'model_switch_failed', + data: { + sessionId: entry.sessionId, + requestedModelId: modelId, + error: err instanceof Error ? err.message : String(err), + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); throw err; } finally { entry.modelRoundtripInFlight = false; @@ -1740,15 +1742,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ): void => { entry.currentModelId = modelId; entry.modelPublishGeneration++; - try { - entry.events.publish({ - type: 'model_switched', - data: { sessionId: entry.sessionId, modelId }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed */ - } + // `EventBus.publish` never throws (a closed bus is a return-undefined + // no-op); per its documented contract we don't wrap it — a try/catch + // here would be dead code for "bus closed" and would mislabel a real + // programming error (e.g. a `TypeError`) as a benign bus-closed swallow. + entry.events.publish({ + type: 'model_switched', + data: { sessionId: entry.sessionId, modelId }, + ...(originatorClientId ? { originatorClientId } : {}), + }); }; const publishApprovalModeChanged = ( @@ -1758,20 +1760,17 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ): void => { entry.currentApprovalMode = payload.next; entry.approvalModePublishGeneration++; - try { - entry.events.publish({ - type: 'approval_mode_changed', - data: { - sessionId: entry.sessionId, - previous: payload.previous, - next: payload.next, - persisted: payload.persisted, - }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed */ - } + // See `publishModelSwitched`: `publish()` never throws, so no wrapper. + entry.events.publish({ + type: 'approval_mode_changed', + data: { + sessionId: entry.sessionId, + previous: payload.previous, + next: payload.next, + persisted: payload.persisted, + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); }; // §2.2 post-roundtrip reconciliation — after a bridge-driven model or @@ -1830,7 +1829,17 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const actual = ( status?.state?.modes as { currentModeId?: string } | undefined )?.currentModeId; - if (actual && actual !== entry.currentApprovalMode) { + // Same enum backstop as the demux path (`handleInSessionModeUpdate`): + // `actual` is an agent-supplied id typed `unknown`, and the SDK's + // `isApprovalModeChangedData` is a structural check (deliberately + // forward-compatible with a future 5th mode), NOT an enum gate. An + // unknown id here would fan out to every SSE client and land in the + // reducer's `state.approvalMode`, so drop it before publishing. + if (actual && !KNOWN_APPROVAL_MODES.has(actual)) { + writeStderrLine( + `[reconcile] session=${entry.sessionId} target=approvalMode action=dropped reason=unknown_mode mode=${actual}`, + ); + } else if (actual && actual !== entry.currentApprovalMode) { writeStderrLine( `[reconcile] session=${entry.sessionId} target=approvalMode action=corrected cached=${entry.currentApprovalMode ?? ''} actual=${actual}`, ); @@ -1899,6 +1908,34 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { return entry; }; + // A5: seed the snapshot caches from the agent's session-create response + // (`newSession` / `loadSession` / `resumeSession` all return `models` + + // `modes`). Without this the caches stay unset until the first change, so a + // cold `?snapshot=1` attach to a session that never switched would return + // `{ currentModelId: null, currentApprovalMode: null }` and the SDK reducer's + // `!= null` guard would leave the client unseeded — defeating A5's primary + // (initial-attach) use case. The agent's `currentModelId` is already the + // canonical `model(authType)` form (acpAgent `formatAcpModelId`), matching + // what `reconcileAfterRoundtrip` reads back, so seeding it keeps the model + // comparison format-stable. Mode ids pass the same `KNOWN_APPROVAL_MODES` + // backstop the demux/reconcile paths use. + const seedSnapshotCaches = ( + entry: SessionEntry, + resp: { + models?: { currentModelId?: unknown } | null; + modes?: { currentModeId?: unknown } | null; + }, + ): void => { + const model = resp.models?.currentModelId; + if (typeof model === 'string' && model.length > 0) { + entry.currentModelId = model; + } + const mode = resp.modes?.currentModeId; + if (typeof mode === 'string' && KNOWN_APPROVAL_MODES.has(mode)) { + entry.currentApprovalMode = mode; + } + }; + const isAcpSessionResourceNotFound = ( err: unknown, sessionId: string, @@ -2162,6 +2199,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { restoreEvents, ); entry.restoreState = state; + seedSnapshotCaches(entry, state); const clientId = registerClient(entry, req.clientId); // Fold synchronous coalesce reservations into the new entry's // `attachCount`. By this point all coalescers that beat us must @@ -3477,20 +3515,17 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // Mirror `applyModelServiceId`'s observability contract: surface // failed model changes on the SSE bus so subscribers can update // their UI / retry. Without this the only signal is the HTTP - // 5xx, which doesn't reach passive viewers. - try { - entry.events.publish({ - type: 'model_switch_failed', - data: { - sessionId: entry.sessionId, - requestedModelId: req.modelId, - error: err instanceof Error ? err.message : String(err), - }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed */ - } + // 5xx, which doesn't reach passive viewers. `publish()` never + // throws (see `publishModelSwitched`), so no wrapper. + entry.events.publish({ + type: 'model_switch_failed', + data: { + sessionId: entry.sessionId, + requestedModelId: req.modelId, + error: err instanceof Error ? err.message : String(err), + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); throw err; } // model_switched is published inside the work callback above (while the @@ -3600,6 +3635,20 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }, entry.sessionId, ); + // F3Qgp: a persisted change rewrites the workspace default, so the + // peers we just notified now hold a stale `currentApprovalMode` in + // their SessionEntry cache. Their GET status / session_snapshot + // would report the pre-change mode until their own next roundtrip. + // `byId` is the per-workspace session map (the bridge is bound per + // workspace), so mirror the new default into every peer's cache; + // skip the originator, whose cache `publishApprovalModeChanged` + // already updated. + for (const peer of byId.values()) { + if (peer.sessionId === entry.sessionId) { + continue; + } + peer.currentApprovalMode = response.current; + } } succeeded = true; return { diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 0552610d36a..cafc1d03e5f 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -122,8 +122,9 @@ const EARLY_EVENT_TTL_MS = 60_000; // Known approval-mode ids accepted on the in-session `current_mode_update` // demux path. Mirrors the `modeMap` keys in `Session.setMode` (CLI); an id // outside this set is dropped before it fans out to SSE clients / the SDK -// reducer. Keep the two in lockstep. -const KNOWN_APPROVAL_MODES: ReadonlySet = new Set([ +// reducer. Keep the two in lockstep. Exported so the bridge's reconcile and +// snapshot-seed paths apply the same enum backstop to agent-supplied mode ids. +export const KNOWN_APPROVAL_MODES: ReadonlySet = new Set([ 'plan', 'default', 'auto-edit', @@ -579,17 +580,15 @@ export class BridgeClient implements Client { entry.activePromptOriginatorClientId, ); } else { - try { - entry.events.publish({ - type: 'model_switched', - data: { sessionId, modelId: currentModelId }, - ...(entry.activePromptOriginatorClientId - ? { originatorClientId: entry.activePromptOriginatorClientId } - : {}), - }); - } catch { - /* bus closed */ - } + // `EventBus.publish` never throws (closed bus → undefined no-op); per + // its documented contract we don't wrap it. + entry.events.publish({ + type: 'model_switched', + data: { sessionId, modelId: currentModelId }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), + }); } writeStderrLine( `[demux] session=${sessionId} type=current_model_update action=promoted model=${currentModelId}`, @@ -647,31 +646,31 @@ export class BridgeClient implements Client { entry.activePromptOriginatorClientId, ); } else { - try { - // Fallback path (no `onModePromoted` injected — tests / non-bridge - // consumers; production always wires the bridge callback). Mirror - // the main path's full payload: the SDK's - // `isApprovalModeChangedData` requires `previous` (non-empty - // string) and `persisted` (boolean), so a `{ sessionId, next }` - // shape fails validation and `asKnownDaemonEvent` drops the event. - // `previous` is unavailable on this path (the cache lives on the - // bridge's `SessionEntry`, not the demux interface), so seed it - // with the protocol default. - entry.events.publish({ - type: 'approval_mode_changed', - data: { - sessionId, - previous: 'default', - next: currentModeId, - persisted: false, - }, - ...(entry.activePromptOriginatorClientId - ? { originatorClientId: entry.activePromptOriginatorClientId } - : {}), - }); - } catch { - /* bus closed */ - } + // Fallback path (no `onModePromoted` injected — tests / non-bridge + // consumers; production always wires the bridge callback). Mirror + // the main path's full payload: the SDK's + // `isApprovalModeChangedData` requires `previous` (non-empty + // string) and `persisted` (boolean), so a `{ sessionId, next }` + // shape fails validation and `asKnownDaemonEvent` drops the event. + // `previous` is unavailable on this path (the cache lives on the + // bridge's `SessionEntry`, not the demux interface), so seed it + // with the protocol default. + // + // `EventBus.publish` never throws (a closed bus is a return-undefined + // no-op and subscriber-enqueue failures are caught internally), so + // per its documented contract we don't wrap it in try/catch. + entry.events.publish({ + type: 'approval_mode_changed', + data: { + sessionId, + previous: 'default', + next: currentModeId, + persisted: false, + }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), + }); } // TODO(dual-emit-removal): also emit the legacy generic // `session_update{current_mode_update}` for one release cycle so the @@ -701,23 +700,21 @@ export class BridgeClient implements Client { ); return; } - try { - entry.events.publish({ - type: 'session_update', - data: { - sessionId, - update: { - sessionUpdate: 'current_mode_update', - currentModeId, - }, + // `EventBus.publish` never throws (closed bus → undefined no-op); per its + // documented contract we don't wrap it in try/catch. + entry.events.publish({ + type: 'session_update', + data: { + sessionId, + update: { + sessionUpdate: 'current_mode_update', + currentModeId, }, - ...(entry.activePromptOriginatorClientId - ? { originatorClientId: entry.activePromptOriginatorClientId } - : {}), - }); - } catch { - /* bus closed */ - } + }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), + }); writeStderrLine( `[demux] session=${sessionId} type=current_mode_update action=promoted mode=${currentModeId}`, ); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 3afaed28bb7..5d43a12df14 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1835,9 +1835,10 @@ export class Session implements SessionContext { sessionId: this.sessionId, currentModeId: params.modeId, }) - .catch(() => { + .catch((error) => { // Advisory only; a failed notification must not fail the mode // switch. Matches the model-update extNotification in `setModel`. + debugLogger.debug('mode-update extNotification failed', error); }); } @@ -1895,8 +1896,9 @@ export class Session implements SessionContext { sessionId: this.sessionId, currentModelId: effectiveModelId, }) - .catch(() => { + .catch((error) => { // Advisory only; a failed notification must not fail the model switch. + debugLogger.debug('model-update extNotification failed', error); }); if (options.persistDefault ?? true) { @@ -1970,9 +1972,10 @@ export class Session implements SessionContext { currentModeId: newModeId, legacyFrameSent: true, }) - .catch(() => { + .catch((error) => { // Advisory only; a failed notification must not fail the mode // change. Matches the model-update extNotification in `setModel`. + debugLogger.debug('mode-update extNotification failed', error); }); } From 9e1c99d23094a5ab09777d521b8f7a6a66f5efed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sun, 31 May 2026 18:08:46 +0800 Subject: [PATCH 07/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-5=20review=20nits=20=E2=80=94=20stale=20comments=20and=20cac?= =?UTF-8?q?he=20doc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge: document that setSessionModel caches the raw model id and relies on the immediately-following reconcileAfterRoundtrip to correct any raw-vs-canonical drift (the bridge layer lacks access to the CLI's formatAcpModelId which requires authType) - bridge: fix stale reconcile-catch comment that referenced state_resync_required (long-lived SSE connections don't reconnect) - bridgeClient.test: update stale "7-arg constructor" comment to reflect the current 8-arg constructor Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.ts | 15 ++++++++++++--- packages/acp-bridge/src/bridgeClient.test.ts | 8 ++++---- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 8ee3c5095ea..d2afb37903c 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1859,9 +1859,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // corrected. Keep the signal in the operator log rather than // emitting a bus event no client can decode: `reconciliation_failed` // is not a known SDK event type, so `asKnownDaemonEvent` drops it - // and the reducer never sees it. The client-facing "your state may - // be stale" path is already covered by `state_resync_required` on - // reconnect. + // and the reducer never sees it. Long-lived SSE connections that + // never disconnect will hold their last-seen state until the next + // successful roundtrip triggers another reconcile; reconnecting + // clients get a fresh `session_snapshot` on attach. writeStderrLine( `[reconcile] session=${entry.sessionId} target=${target} action=failed error=${ err instanceof Error ? err.message : String(err) @@ -3488,6 +3489,14 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ), transportClosed, ]); + // Cache the model id as received from the caller. The bridge + // layer does not have access to the CLI's `formatAcpModelId` + // (which requires `authType`), so it cannot canonicalize here. + // In practice callers always send canonical ids (from + // `buildAvailableModels`); any residual raw→canonical drift is + // corrected by the `reconcileAfterRoundtrip` below, which reads + // the agent's authoritative canonical id and re-publishes if it + // differs. publishModelSwitched(entry, req.modelId, originatorClientId); succeeded = true; return result; diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts index 57978a9bc00..3f77dda4c3b 100644 --- a/packages/acp-bridge/src/bridgeClient.test.ts +++ b/packages/acp-bridge/src/bridgeClient.test.ts @@ -17,12 +17,12 @@ * proxy is fully bypassed — no `fs.writeFile` syscall); * 2. when `fileSystem` is omitted, the inline proxy runs and * reads / writes real disk (sanity check that the fallback - * path the 7-arg constructor's positional slot opt-outs to + * path the 8-arg constructor's positional slot opt-outs to * still works). * - * Regression guard: the constructor takes 7 positional args; the - * 7th (`fileSystem`) is optional and at the tail. A subtle re- - * ordering (or dropping the arg from `bridge.ts:773` factory's + * Regression guard: the constructor takes 8 positional args; the + * 6th (`fileSystem`) is optional. A subtle re-ordering (or + * dropping the arg from `bridge.ts`'s factory * `new BridgeClient(..., opts.fileSystem)` call) would silently * bypass the adapter in production. Test #1 + #2 catch that * because the mock fileSystem would never be called. From 38377e545b44d3e8f4941d19ac12d6c9de0ddb16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sun, 31 May 2026 19:38:38 +0800 Subject: [PATCH 08/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-6=20review=20=E2=80=94=20bundle=20cap,=20test=20gaps,=20asse?= =?UTF-8?q?rtions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - sdk: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES from 100 KiB to 105 KiB to accommodate session_snapshot type/validator/reducer additions (+1.2 KiB) - bridge: remove redundant entry! non-null assertions (already narrowed by if-guard at line 2708) - bridge: document setSessionModel raw-id cache + reconcile correction - bridge.test: add seedSnapshotCaches cold-attach test (newSession response seeds model+mode without intermediate notifications) - bridge.test: add peer cache sync test (persisted mode change updates peer snapshot) - bridge.test: add unknown-mode-drop test (reconcile drops agent- returned modes not in KNOWN_APPROVAL_MODES) Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 189 +++++++++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 6 +- 2 files changed, 192 insertions(+), 3 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 47d15daeb88..4a925887848 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -8276,6 +8276,60 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { abort.abort(); await bridge.shutdown(); }); + + it('seeds snapshot from newSession response without any intermediate notification (cold attach)', async () => { + // F7qEJ: seedSnapshotCaches fills the cache from the newSession + // response alone — no extNotification or setSessionModel needed. + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + newSessionImpl: (p) => + Promise.resolve({ + sessionId: `sess:${p.cwd}`, + models: { + currentModelId: 'qwen-plus', + availableModels: [{ modelId: 'qwen-plus', name: 'Qwen Plus' }], + }, + modes: { + currentModeId: 'auto-edit', + availableModes: [ + { modeId: 'auto-edit', id: 'auto-edit', name: 'Auto Edit' }, + ], + }, + }), + }); + new AgentSideConnection(() => fakeAgent as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Subscribe with snapshot=true, no lastEventId — pure cold attach. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + snapshot: true, + }); + const it2 = iter[Symbol.asyncIterator](); + const first = await it2.next(); + expect(first.value?.type).toBe('session_snapshot'); + const data = first.value?.data as { + currentModelId: string | null; + currentApprovalMode: string | null; + }; + expect(data.currentModelId).toBe('qwen-plus'); + expect(data.currentApprovalMode).toBe('auto-edit'); + abort.abort(); + await bridge.shutdown(); + }); }); describe('§2.2 — post-roundtrip reconciliation', () => { @@ -8745,6 +8799,141 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { await collecting; await bridge.shutdown(); }); + + it('drops unknown agent-returned approval mode without publishing a corrective event', async () => { + // F7qEL: when the agent returns a mode not in KNOWN_APPROVAL_MODES, + // reconcile should drop it (action=dropped reason=unknown_mode) + // instead of broadcasting an invalid approval_mode_changed. + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method) => { + if (method === 'qwen/status/session/context') { + return Promise.resolve({ + state: { modes: { currentModeId: 'super-yolo' } }, + }); + } + return Promise.resolve({}); + }, + }); + const augmented = new Proxy(fakeAgent, { + get(target, prop) { + if (prop === 'unstable_setSessionModel') { + return async () => ({}); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (target as any)[prop]; + }, + }); + new AgentSideConnection(() => augmented as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ + workspaceCwd: WS_A, + modelServiceId: 'qwen-turbo', + }); + const abort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: abort.signal, + }); + const events: string[] = []; + const collecting = (async () => { + for await (const e of iter) { + events.push(e.type); + } + })(); + + // The model switch triggers reconcile. Agent reports 'super-yolo' + // which is not a known mode — the corrective should be dropped. + await new Promise((r) => setTimeout(r, 50)); + expect(events.filter((e) => e === 'approval_mode_changed')).toEqual([]); + abort.abort(); + await collecting; + await bridge.shutdown(); + }); + + it('syncs peer session cache on persisted approval-mode change (snapshot reflects new mode)', async () => { + // F7qEK: when session A persists a mode change, peer session B's + // cache should be updated so a subsequent snapshot on B reports + // the new workspace default — not the stale pre-change value. + const factory: ChannelFactory = async () => { + const { clientStream, agentStream } = createInMemoryChannel(); + const fakeAgent = new FakeAgent({ + extMethodImpl: (method, params) => { + if (method === 'qwen/control/session/approval_mode') { + return Promise.resolve({ + previous: 'default', + current: (params as { mode: string }).mode, + }); + } + if (method === 'qwen/status/session/context') { + return Promise.resolve({ + state: { + modes: { + currentModeId: + (params as { mode?: string }).mode ?? 'default', + }, + }, + }); + } + return Promise.resolve({}); + }, + }); + new AgentSideConnection(() => fakeAgent as Agent, agentStream); + return { + stream: clientStream, + exited: new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >(() => {}), + kill: async () => {}, + killSync: () => {}, + }; + }; + const bridge = makeBridge({ + channelFactory: factory, + sessionScope: 'thread', + persistApprovalMode: async () => {}, + }); + // Two sessions in the same workspace (thread scope → each attach + // creates a new session). + const sessionA = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const sessionB = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(sessionA.sessionId).not.toBe(sessionB.sessionId); + + // Persist a mode change on A. + await bridge.setSessionApprovalMode( + sessionA.sessionId, + ApprovalMode.YOLO, + { persist: true }, + undefined, + ); + + // Subscribe on B with snapshot — should reflect the persisted mode. + const abort = new AbortController(); + const iter = bridge.subscribeEvents(sessionB.sessionId, { + signal: abort.signal, + snapshot: true, + }); + const it2 = iter[Symbol.asyncIterator](); + const first = await it2.next(); + expect(first.value?.type).toBe('session_snapshot'); + expect( + (first.value?.data as { currentApprovalMode: string | null }) + .currentApprovalMode, + ).toBe('yolo'); + abort.abort(); + await bridge.shutdown(); + }); }); }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index d2afb37903c..b9d27d12dba 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -2751,9 +2751,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { v: EVENT_SCHEMA_VERSION, type: 'session_snapshot', data: { - sessionId: entry!.sessionId, - currentModelId: entry!.currentModelId ?? null, - currentApprovalMode: entry!.currentApprovalMode ?? null, + sessionId: entry.sessionId, + currentModelId: entry.currentModelId ?? null, + currentApprovalMode: entry.currentApprovalMode ?? null, }, }); async function* withSnapshot(): AsyncIterable { From fe0517b712f45aba0dab07d65475cef72e624301 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sun, 31 May 2026 23:28:43 +0800 Subject: [PATCH 09/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-6=20follow-up=20=E2=80=94=20fix=20false-positive=20test,=20a?= =?UTF-8?q?dd=20resync=20passthrough=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge.test: rewrite unknown-mode-drop test to trigger approvalMode reconcile (via setSessionApprovalMode) instead of model reconcile (via modelServiceId), which never entered the approvalMode branch — the original was a false positive (F8E2h) - bridge.test: fix misleading params.mode cast in peer-cache-sync test; status RPC sends {sessionId} not {mode} — return fixed 'yolo' (F8E2o) - sdk daemonEvents.test: add session_snapshot passthrough-during-resync test (RESYNC_PASSTHROUGH_TYPES membership regression guard) (F8SOq) Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 67 +++++++++++-------- .../test/unit/daemonEvents.test.ts | 36 ++++++++++ 2 files changed, 74 insertions(+), 29 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 4a925887848..64a2337b01f 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -8801,14 +8801,24 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { }); it('drops unknown agent-returned approval mode without publishing a corrective event', async () => { - // F7qEL: when the agent returns a mode not in KNOWN_APPROVAL_MODES, - // reconcile should drop it (action=dropped reason=unknown_mode) - // instead of broadcasting an invalid approval_mode_changed. + // F7qEL / F8E2h: when the agent returns a mode not in + // KNOWN_APPROVAL_MODES, the approvalMode reconcile branch should + // drop it (action=dropped reason=unknown_mode) instead of + // broadcasting an invalid approval_mode_changed. We trigger the + // approvalMode reconcile via setSessionApprovalMode (not via + // modelServiceId, which only reconciles the model branch). const factory: ChannelFactory = async () => { const { clientStream, agentStream } = createInMemoryChannel(); const fakeAgent = new FakeAgent({ - extMethodImpl: (method) => { + extMethodImpl: (method, params) => { + if (method === 'qwen/control/session/approval_mode') { + return Promise.resolve({ + previous: 'default', + current: (params as { mode: string }).mode, + }); + } if (method === 'qwen/status/session/context') { + // Agent claims a mode that's NOT in KNOWN_APPROVAL_MODES. return Promise.resolve({ state: { modes: { currentModeId: 'super-yolo' } }, }); @@ -8816,16 +8826,7 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { return Promise.resolve({}); }, }); - const augmented = new Proxy(fakeAgent, { - get(target, prop) { - if (prop === 'unstable_setSessionModel') { - return async () => ({}); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (target as any)[prop]; - }, - }); - new AgentSideConnection(() => augmented as Agent, agentStream); + new AgentSideConnection(() => fakeAgent as Agent, agentStream); return { stream: clientStream, exited: new Promise< @@ -8837,25 +8838,35 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { }; }; const bridge = makeBridge({ channelFactory: factory }); - const session = await bridge.spawnOrAttach({ - workspaceCwd: WS_A, - modelServiceId: 'qwen-turbo', - }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const abort = new AbortController(); const iter = bridge.subscribeEvents(session.sessionId, { signal: abort.signal, }); - const events: string[] = []; + const modeEvents: string[] = []; const collecting = (async () => { for await (const e of iter) { - events.push(e.type); + if (e.type === 'approval_mode_changed') { + modeEvents.push((e.data as { next: string }).next); + } } })(); - // The model switch triggers reconcile. Agent reports 'super-yolo' - // which is not a known mode — the corrective should be dropped. + // setSessionApprovalMode triggers reconcileAfterRoundtrip(entry, + // 'approvalMode'). The status read returns 'super-yolo' which + // isn't in KNOWN_APPROVAL_MODES — reconcile must DROP it. + await bridge.setSessionApprovalMode( + session.sessionId, + ApprovalMode.YOLO, + { persist: false }, + undefined, + ); + + // Wait for reconcile to fire (async microtask chain). await new Promise((r) => setTimeout(r, 50)); - expect(events.filter((e) => e === 'approval_mode_changed')).toEqual([]); + // Only the original mode change should appear — no corrective + // for the unknown 'super-yolo' value from the agent. + expect(modeEvents).toEqual(['yolo']); abort.abort(); await collecting; await bridge.shutdown(); @@ -8876,13 +8887,11 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { }); } if (method === 'qwen/status/session/context') { + // Status RPC returns agent's authoritative mode. After the + // persist, the agent is on 'yolo' — return it so reconcile + // sees no drift and does not emit a corrective. return Promise.resolve({ - state: { - modes: { - currentModeId: - (params as { mode?: string }).mode ?? 'default', - }, - }, + state: { modes: { currentModeId: 'yolo' } }, }); } return Promise.resolve({}); diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index b740cc7b7e3..b0327b92153 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -2485,6 +2485,42 @@ describe('PR 21 — auth device-flow events', () => { expect(state.unrecognizedKnownEventCount).toBe(1); expect(state.awaitingResync).toBe(false); }); + + it('still applies session_snapshot while awaitingResync (RESYNC_PASSTHROUGH_TYPES)', () => { + // session_snapshot is in RESYNC_PASSTHROUGH_TYPES — a reconnecting + // client that missed ring events still needs to seed its side-channel + // model/mode state. Without passthrough, the auto-skip gate would + // drop the snapshot and the client would remain on stale null/null. + const afterResync = reduceDaemonSessionEvent( + createDaemonSessionViewState(), + { + v: 1, + type: 'state_resync_required', + data: { + reason: 'ring_evicted', + lastDeliveredId: 5, + earliestAvailableId: 12, + }, + }, + ); + expect(afterResync.awaitingResync).toBe(true); + + const afterSnapshot = reduceDaemonSessionEvent(afterResync, { + id: 13, + v: 1, + type: 'session_snapshot', + data: { + sessionId: 's-1', + currentModelId: 'qwen-max', + currentApprovalMode: 'auto-edit', + }, + }); + // The snapshot must have applied — model/mode state is populated. + expect(afterSnapshot.currentModelId).toBe('qwen-max'); + expect(afterSnapshot.approvalMode).toBe('auto-edit'); + // awaitingResync stays true (consumer hasn't explicitly recovered). + expect(afterSnapshot.awaitingResync).toBe(true); + }); }); describe('followup_suggestion (daemon assist push)', () => { From 6661565808f1e796ef7f659bec8d8486b532aa68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 1 Jun 2026 20:09:05 +0800 Subject: [PATCH 10/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-6=20follow-up=20=E2=80=94=20positive=20reconcile=20assertion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add statusReads counter to the unknown-mode-drop test so it positively asserts that reconcile actually executed (status RPC was called), not just that no corrective event appeared. Without this, a future refactor disabling reconcileAfterRoundtrip would make the test pass vacuously. Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 64a2337b01f..caf26188634 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -8807,6 +8807,7 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { // broadcasting an invalid approval_mode_changed. We trigger the // approvalMode reconcile via setSessionApprovalMode (not via // modelServiceId, which only reconciles the model branch). + let statusReads = 0; const factory: ChannelFactory = async () => { const { clientStream, agentStream } = createInMemoryChannel(); const fakeAgent = new FakeAgent({ @@ -8818,6 +8819,7 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { }); } if (method === 'qwen/status/session/context') { + statusReads += 1; // Agent claims a mode that's NOT in KNOWN_APPROVAL_MODES. return Promise.resolve({ state: { modes: { currentModeId: 'super-yolo' } }, @@ -8864,6 +8866,10 @@ describe('createHttpAcpBridge — side-channel state layer (#4511)', () => { // Wait for reconcile to fire (async microtask chain). await new Promise((r) => setTimeout(r, 50)); + // Positive assertion: reconcile DID execute (status was read). + // Without this, a future refactor that disables reconcile would + // make the modeEvents assertion pass vacuously. + expect(statusReads).toBe(1); // Only the original mode change should appear — no corrective // for the unknown 'super-yolo' value from the agent. expect(modeEvents).toEqual(['yolo']); From 20b60ae0a8c171390cef23df2bc1f2f26b90f7ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Sun, 7 Jun 2026 23:25:50 +0800 Subject: [PATCH 11/16] =?UTF-8?q?fix(daemon):=20address=20PR=20#4613=20rou?= =?UTF-8?q?nd-6=20follow-up=20=E2=80=94=20fix=20false-positive=20test,=20a?= =?UTF-8?q?dd=20resync=20passthrough=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge.test: restore missing closing braces for extractErrorCode describe/it blocks (lost during rebase conflict resolution) - sdk build.js: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES from 106 to 107 KiB (actual bundle is 108595 bytes = ~106.1 KiB) Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index caf26188634..8c1f24d42f6 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -7634,6 +7634,9 @@ describe('extractErrorCode', () => { it('returns undefined when code is not string or number', () => { expect(extractErrorCode({ code: true })).toBeUndefined(); + }); +}); + // --------------------------------------------------------------------------- // §2.3 side-channel state layer: publish helpers + reconciliation + snapshot // --------------------------------------------------------------------------- From 99f1996488083ddf9d3f8f6b2aede4dbd119de1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 8 Jun 2026 09:33:19 +0800 Subject: [PATCH 12/16] fix(daemon): validate agent approval-mode response + typeof guard on model reconcile - bridge: validate setSessionApprovalMode extMethod response against KNOWN_APPROVAL_MODES before publishing/broadcasting; drop with log if agent returns unknown mode (closes trust-boundary gap where handleInSessionModeUpdate and reconcile had guards but this path did not) - bridge: add typeof === 'string' guard to model reconcile branch so a non-string agent response (e.g. number) cannot pollute the cache and break downstream session_snapshot validation - bridge: add writeStderrLine to seedSnapshotCaches drop branches for operator observability parity with reconcile's drop logging Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.ts | 171 +++++++++++++++++++++++++++++- 1 file changed, 170 insertions(+), 1 deletion(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index b9d27d12dba..de4c77ad444 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -72,6 +72,8 @@ import type { BridgeSessionState, BridgeRestoredSession, BridgeSessionSummary, + BridgeClientRequestContext, + CloseSessionOpts, AcpSessionBridge, } from './bridgeTypes.js'; import type { BridgeOptions, BridgeTelemetry } from './bridgeOptions.js'; @@ -660,6 +662,8 @@ const DEFAULT_PERMISSION_TIMEOUT_MS = 5 * 60 * 1000; // the limit being hit. Configurable via // `BridgeOptions.maxPendingPermissionsPerSession`. const DEFAULT_MAX_PENDING_PER_SESSION = 64; +const DEFAULT_SESSION_REAP_INTERVAL_MS = 60_000; +const DEFAULT_SESSION_IDLE_TIMEOUT_MS = 30 * 60_000; export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const defaultSessionScope = opts.sessionScope ?? 'single'; @@ -793,6 +797,28 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let channelInfo: ChannelInfo | undefined; let idleTimer: ReturnType | undefined; + // Session reaper: periodically scans `byId` and closes sessions + // with no SSE subscribers, no registered clients, no active prompt, + // and whose last heartbeat exceeds the idle TTL. Disabled when + // either value resolves to 0 / non-finite. + const sessionReapIntervalMs = resolvePositiveFiniteMs( + opts.sessionReapIntervalMs, + DEFAULT_SESSION_REAP_INTERVAL_MS, + ); + const sessionIdleTimeoutMs = resolvePositiveFiniteMs( + opts.sessionIdleTimeoutMs, + DEFAULT_SESSION_IDLE_TIMEOUT_MS, + ); + let sessionReaper: ReturnType | undefined; + + function resolvePositiveFiniteMs( + raw: number | undefined, + fallback: number, + ): number { + if (raw === undefined) return fallback; + return raw > 0 && Number.isFinite(raw) ? raw : 0; + } + function cancelIdleTimer(): void { if (idleTimer !== undefined) { clearTimeout(idleTimer); @@ -840,6 +866,45 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }, timeoutMs); idleTimer.unref(); } + + function startSessionReaper(): void { + if (sessionReapIntervalMs <= 0 || sessionIdleTimeoutMs <= 0) return; + sessionReaper = setInterval(() => { + if (shuttingDown) return; + const now = Date.now(); + for (const [id, entry] of byId) { + if (entry.activePromptOriginatorClientId !== undefined) continue; + if (entry.events.subscriberCount > 0) continue; + if (entry.clientIds.size > 0) continue; + const lastActive = + entry.sessionLastSeenAt ?? Date.parse(entry.createdAt); + const idle = now - lastActive; + if (idle < sessionIdleTimeoutMs) continue; + writeStderrLine( + `qwen serve: reaping idle session ${JSON.stringify(id)} ` + + `(idle for ${Math.round(idle / 1000)}s, ` + + `threshold ${Math.round(sessionIdleTimeoutMs / 1000)}s)`, + ); + void closeSessionImpl(id, undefined, { reason: 'idle_timeout' }).catch( + (err) => { + writeStderrLine( + `qwen serve: session reaper failed to close ` + + `${JSON.stringify(id)}: ${String(err)}`, + ); + }, + ); + } + }, sessionReapIntervalMs); + sessionReaper.unref(); + } + + function stopSessionReaper(): void { + if (sessionReaper !== undefined) { + clearInterval(sessionReaper); + sessionReaper = undefined; + } + } + // BkUyD: superset of `channelInfo` covering channels // that are dying but not yet OS-reaped. `killSession` / // `doSpawn`-newSession-failure / `shutdown` mark a channel as @@ -1819,7 +1884,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const actual = ( status?.state?.models as { currentModelId?: string } | undefined )?.currentModelId; - if (actual && actual !== entry.currentModelId) { + if ( + typeof actual === 'string' && + actual && + actual !== entry.currentModelId + ) { writeStderrLine( `[reconcile] session=${entry.sessionId} target=model action=corrected cached=${entry.currentModelId ?? ''} actual=${actual}`, ); @@ -1930,10 +1999,18 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const model = resp.models?.currentModelId; if (typeof model === 'string' && model.length > 0) { entry.currentModelId = model; + } else if (model != null) { + writeStderrLine( + `[seed] session=${entry.sessionId} target=model action=dropped value=${JSON.stringify(model)} reason=invalid_type`, + ); } const mode = resp.modes?.currentModeId; if (typeof mode === 'string' && KNOWN_APPROVAL_MODES.has(mode)) { entry.currentApprovalMode = mode; + } else if (mode != null) { + writeStderrLine( + `[seed] session=${entry.sessionId} target=approvalMode action=dropped value=${JSON.stringify(mode)} reason=${typeof mode !== 'string' ? 'invalid_type' : 'unknown_mode'}`, + ); } }; @@ -2253,6 +2330,79 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } } + async function closeSessionImpl( + sessionId: string, + context?: BridgeClientRequestContext, + closeOpts?: CloseSessionOpts, + ): Promise { + const entry = byId.get(sessionId); + if (!entry) throw new SessionNotFoundError(sessionId); + let originatorClientId: string | undefined; + if (context?.clientId !== undefined) { + originatorClientId = resolveTrustedClientId(entry, context.clientId); + } + writeStderrLine( + `qwen serve: closing session ${JSON.stringify(sessionId)}` + + (originatorClientId + ? ` by client ${JSON.stringify(originatorClientId)}` + : ''), + ); + telemetry.event('session.close', { + 'qwen-code.daemon.bridge.operation': 'session.close', + 'session.id': sessionId, + }); + if (defaultEntry === entry) defaultEntry = undefined; + const ci = channelInfoForEntry(entry); + if (!ci) { + writeStderrLine( + `qwen serve: closeSession channelInfoForEntry returned undefined ` + + `for session ${JSON.stringify(sessionId)} — channel cleanup skipped (entry's channel already torn down)`, + ); + } + if (ci && ci.channel === entry.channel) { + ci.sessionIds.delete(sessionId); + } + await notifyAgentSessionClose(entry, ci, 'closeSession'); + permissionMediator.forgetSession(sessionId); + entry.pendingPermissionIds.clear(); + byId.delete(sessionId); + telemetry.metrics?.sessionLifecycle('close'); + ci?.client.markSessionClosed(sessionId); + const reason = closeOpts?.reason ?? 'client_close'; + try { + entry.events.publish({ + type: 'session_closed', + data: { + sessionId, + reason, + ...(originatorClientId ? { closedBy: originatorClientId } : {}), + }, + ...(originatorClientId ? { originatorClientId } : {}), + }); + } catch { + /* bus already closed */ + } + entry.events.close(); + try { + await telemetry.withSpan( + 'session.close.cancel_active_prompt', + { + 'qwen-code.daemon.bridge.operation': + 'session.close.cancel_active_prompt', + 'session.id': sessionId, + }, + async () => await entry.connection.cancel({ sessionId }), + ); + } catch { + /* no active prompt or session already torn down */ + } + if (ci && ci.sessionIds.size === 0 && ci.pendingRestoreIds.size === 0) { + await startIdleTimer(ci, `closeSession "${sessionId}"`); + } + } + + startSessionReaper(); + return { get sessionCount() { return byId.size; @@ -2908,6 +3058,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } }, +<<<<<<< HEAD async branchSession(sessionId, req, context) { if (shuttingDown) throw new Error('AcpSessionBridge is shutting down'); @@ -3599,6 +3750,22 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { getTransportClosedReject(entry), ])) as { previous: ApprovalMode; current: ApprovalMode }; + if ( + typeof response.current !== 'string' || + !KNOWN_APPROVAL_MODES.has(response.current) + ) { + writeStderrLine( + `setSessionApprovalMode: agent returned unknown mode=${JSON.stringify(response.current)}, dropping`, + ); + succeeded = true; + return { + sessionId: entry.sessionId, + mode: mode as ApprovalMode, + previous: response.previous ?? 'default', + persisted: false, + }; + } + let persisted = false; if (opts.persist) { try { @@ -4292,6 +4459,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `channel.exited` hasn't fired yet. shuttingDown = true; cancelIdleTimer(); + stopSessionReaper(); const channels = Array.from(aliveChannels); defaultEntry = undefined; byId.clear(); @@ -4311,6 +4479,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // spawning a child this teardown won't see. shuttingDown = true; cancelIdleTimer(); + stopSessionReaper(); const entries = Array.from(byId.values()); // Snapshot every alive channel (typically 1; up to 2 during a // `killSession`-then-`spawnOrAttach` overlap) — entries are From 017fdc430f4635a3e43f4267bdd3c45dedba69d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 8 Jun 2026 14:59:12 +0800 Subject: [PATCH 13/16] fix(daemon): fix unknown-mode succeeded flag + restore HAZARD comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge: leave succeeded=false when agent returns unknown approval mode — skips pointless reconcile that would re-drop the same value - bridge: restore channel-overlap HAZARD comment on closeSession's channelInfoForEntry call (lost during reaper code removal) Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index de4c77ad444..915434e39c9 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -3058,7 +3058,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } }, -<<<<<<< HEAD async branchSession(sessionId, req, context) { if (shuttingDown) throw new Error('AcpSessionBridge is shutting down'); @@ -3757,7 +3756,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { writeStderrLine( `setSessionApprovalMode: agent returned unknown mode=${JSON.stringify(response.current)}, dropping`, ); - succeeded = true; + // Leave succeeded=false so reconcile is skipped — the cache + // was not updated, so a reconcile would compare stale cache + // against the same unknown value and re-drop it pointlessly. return { sessionId: entry.sessionId, mode: mode as ApprovalMode, From 3413b767fb84d6db77362f3247645ae2f52cfc04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 8 Jun 2026 16:11:52 +0800 Subject: [PATCH 14/16] fix(daemon): restore missing delimiters in events.ts (rebase artifact) Three sites where session_snapshot was inserted immediately after session_rewound lost the preceding block's closing delimiter during rebase conflict resolution: type alias (missing >;), asKnownDaemonEvent case (missing : undefined;), and reducer case (missing };). Generated with AI Co-authored-by: Qwen-Coder --- packages/sdk-typescript/src/daemon/events.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 7d73012faa8..b617fd02367 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -835,6 +835,7 @@ export type DaemonTurnErrorEvent = DaemonEventEnvelope< export type DaemonSessionRewoundEvent = DaemonEventEnvelope< 'session_rewound', DaemonSessionRewoundData +>; export type DaemonSessionSnapshotEvent = DaemonEventEnvelope< 'session_snapshot', DaemonSessionSnapshotData @@ -1401,6 +1402,7 @@ export function asKnownDaemonEvent( case 'session_rewound': return isSessionRewoundData(event.data) ? (event as DaemonSessionRewoundEvent) + : undefined; case 'session_snapshot': return isSessionSnapshotData(event.data) ? (event as DaemonSessionSnapshotEvent) @@ -1782,6 +1784,7 @@ export function reduceDaemonSessionEvent( ...base, rewindCount: base.rewindCount + 1, lastRewind: mergeOriginator(event.data, event), + }; case 'session_snapshot': return { ...base, From af76d3be66a5d440527ff2171b8495b8ea7001fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 8 Jun 2026 17:43:14 +0800 Subject: [PATCH 15/16] fix(daemon): remove reaper scope creep + fix events.ts delimiters (rebase artifacts) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - bridge: remove session-reaper code (closeSessionImpl, startSession- Reaper, stopSessionReaper, constants) inadvertently included during rebase conflict resolution — not part of this PR's scope - events.ts: restore 2 missing delimiters (isSessionBranchedData closing brace, session_rewound type/case closers) lost when session_snapshot was inserted adjacent to session_branched blocks Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.ts | 139 ------------------- packages/sdk-typescript/src/daemon/events.ts | 7 +- 2 files changed, 6 insertions(+), 140 deletions(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 915434e39c9..65fca4e478c 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -72,8 +72,6 @@ import type { BridgeSessionState, BridgeRestoredSession, BridgeSessionSummary, - BridgeClientRequestContext, - CloseSessionOpts, AcpSessionBridge, } from './bridgeTypes.js'; import type { BridgeOptions, BridgeTelemetry } from './bridgeOptions.js'; @@ -662,8 +660,6 @@ const DEFAULT_PERMISSION_TIMEOUT_MS = 5 * 60 * 1000; // the limit being hit. Configurable via // `BridgeOptions.maxPendingPermissionsPerSession`. const DEFAULT_MAX_PENDING_PER_SESSION = 64; -const DEFAULT_SESSION_REAP_INTERVAL_MS = 60_000; -const DEFAULT_SESSION_IDLE_TIMEOUT_MS = 30 * 60_000; export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const defaultSessionScope = opts.sessionScope ?? 'single'; @@ -797,28 +793,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let channelInfo: ChannelInfo | undefined; let idleTimer: ReturnType | undefined; - // Session reaper: periodically scans `byId` and closes sessions - // with no SSE subscribers, no registered clients, no active prompt, - // and whose last heartbeat exceeds the idle TTL. Disabled when - // either value resolves to 0 / non-finite. - const sessionReapIntervalMs = resolvePositiveFiniteMs( - opts.sessionReapIntervalMs, - DEFAULT_SESSION_REAP_INTERVAL_MS, - ); - const sessionIdleTimeoutMs = resolvePositiveFiniteMs( - opts.sessionIdleTimeoutMs, - DEFAULT_SESSION_IDLE_TIMEOUT_MS, - ); - let sessionReaper: ReturnType | undefined; - - function resolvePositiveFiniteMs( - raw: number | undefined, - fallback: number, - ): number { - if (raw === undefined) return fallback; - return raw > 0 && Number.isFinite(raw) ? raw : 0; - } - function cancelIdleTimer(): void { if (idleTimer !== undefined) { clearTimeout(idleTimer); @@ -867,44 +841,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { idleTimer.unref(); } - function startSessionReaper(): void { - if (sessionReapIntervalMs <= 0 || sessionIdleTimeoutMs <= 0) return; - sessionReaper = setInterval(() => { - if (shuttingDown) return; - const now = Date.now(); - for (const [id, entry] of byId) { - if (entry.activePromptOriginatorClientId !== undefined) continue; - if (entry.events.subscriberCount > 0) continue; - if (entry.clientIds.size > 0) continue; - const lastActive = - entry.sessionLastSeenAt ?? Date.parse(entry.createdAt); - const idle = now - lastActive; - if (idle < sessionIdleTimeoutMs) continue; - writeStderrLine( - `qwen serve: reaping idle session ${JSON.stringify(id)} ` + - `(idle for ${Math.round(idle / 1000)}s, ` + - `threshold ${Math.round(sessionIdleTimeoutMs / 1000)}s)`, - ); - void closeSessionImpl(id, undefined, { reason: 'idle_timeout' }).catch( - (err) => { - writeStderrLine( - `qwen serve: session reaper failed to close ` + - `${JSON.stringify(id)}: ${String(err)}`, - ); - }, - ); - } - }, sessionReapIntervalMs); - sessionReaper.unref(); - } - - function stopSessionReaper(): void { - if (sessionReaper !== undefined) { - clearInterval(sessionReaper); - sessionReaper = undefined; - } - } - // BkUyD: superset of `channelInfo` covering channels // that are dying but not yet OS-reaped. `killSession` / // `doSpawn`-newSession-failure / `shutdown` mark a channel as @@ -2330,79 +2266,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } } - async function closeSessionImpl( - sessionId: string, - context?: BridgeClientRequestContext, - closeOpts?: CloseSessionOpts, - ): Promise { - const entry = byId.get(sessionId); - if (!entry) throw new SessionNotFoundError(sessionId); - let originatorClientId: string | undefined; - if (context?.clientId !== undefined) { - originatorClientId = resolveTrustedClientId(entry, context.clientId); - } - writeStderrLine( - `qwen serve: closing session ${JSON.stringify(sessionId)}` + - (originatorClientId - ? ` by client ${JSON.stringify(originatorClientId)}` - : ''), - ); - telemetry.event('session.close', { - 'qwen-code.daemon.bridge.operation': 'session.close', - 'session.id': sessionId, - }); - if (defaultEntry === entry) defaultEntry = undefined; - const ci = channelInfoForEntry(entry); - if (!ci) { - writeStderrLine( - `qwen serve: closeSession channelInfoForEntry returned undefined ` + - `for session ${JSON.stringify(sessionId)} — channel cleanup skipped (entry's channel already torn down)`, - ); - } - if (ci && ci.channel === entry.channel) { - ci.sessionIds.delete(sessionId); - } - await notifyAgentSessionClose(entry, ci, 'closeSession'); - permissionMediator.forgetSession(sessionId); - entry.pendingPermissionIds.clear(); - byId.delete(sessionId); - telemetry.metrics?.sessionLifecycle('close'); - ci?.client.markSessionClosed(sessionId); - const reason = closeOpts?.reason ?? 'client_close'; - try { - entry.events.publish({ - type: 'session_closed', - data: { - sessionId, - reason, - ...(originatorClientId ? { closedBy: originatorClientId } : {}), - }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus already closed */ - } - entry.events.close(); - try { - await telemetry.withSpan( - 'session.close.cancel_active_prompt', - { - 'qwen-code.daemon.bridge.operation': - 'session.close.cancel_active_prompt', - 'session.id': sessionId, - }, - async () => await entry.connection.cancel({ sessionId }), - ); - } catch { - /* no active prompt or session already torn down */ - } - if (ci && ci.sessionIds.size === 0 && ci.pendingRestoreIds.size === 0) { - await startIdleTimer(ci, `closeSession "${sessionId}"`); - } - } - - startSessionReaper(); - return { get sessionCount() { return byId.size; @@ -4460,7 +4323,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `channel.exited` hasn't fired yet. shuttingDown = true; cancelIdleTimer(); - stopSessionReaper(); const channels = Array.from(aliveChannels); defaultEntry = undefined; byId.clear(); @@ -4480,7 +4342,6 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // spawning a child this teardown won't see. shuttingDown = true; cancelIdleTimer(); - stopSessionReaper(); const entries = Array.from(byId.values()); // Snapshot every alive channel (typically 1; up to 2 during a // `killSession`-then-`spawnOrAttach` overlap) — entries are diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index b617fd02367..0559e9916b8 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -1365,7 +1365,10 @@ export function asKnownDaemonEvent( : undefined; case 'settings_changed': return event.data != null && typeof event.data === 'object' - ? (event as DaemonEventEnvelope<'settings_changed', Record>) + ? (event as DaemonEventEnvelope< + 'settings_changed', + Record + >) : undefined; case 'workspace_initialized': return isWorkspaceInitializedData(event.data) @@ -2549,6 +2552,8 @@ function isSessionBranchedData( isNonEmptyString(value['newSessionId']) && isNonEmptyString(value['displayName']) ); +} + function isSessionSnapshotData( value: unknown, ): value is DaemonSessionSnapshotData { From fff4657f2a3466a3800ef4e8657d5d0119ea9599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A7=A6=E5=A5=87?= Date: Mon, 8 Jun 2026 17:45:05 +0800 Subject: [PATCH 16/16] fix(daemon): throw on unknown agent approval-mode response instead of silent success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the agent returns a mode not in KNOWN_APPROVAL_MODES, throw instead of returning a misleading success response. The previous behavior sent 200 OK echoing the requested mode while the cache and SSE bus still showed the old value — a three-way state divergence. Generated with AI Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 65fca4e478c..dc122e9d897 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -3616,18 +3616,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { typeof response.current !== 'string' || !KNOWN_APPROVAL_MODES.has(response.current) ) { - writeStderrLine( - `setSessionApprovalMode: agent returned unknown mode=${JSON.stringify(response.current)}, dropping`, + // Throw so the HTTP caller sees a 500 instead of a misleading + // 200 OK with the requested mode echoed back. Without this, + // the HTTP client thinks the mode changed while the cache and + // SSE bus still show the old value. + throw new Error( + `Agent returned unknown approval mode: ${JSON.stringify(response.current)}`, ); - // Leave succeeded=false so reconcile is skipped — the cache - // was not updated, so a reconcile would compare stale cache - // against the same unknown value and re-drop it pointlessly. - return { - sessionId: entry.sessionId, - mode: mode as ApprovalMode, - previous: response.previous ?? 'default', - persisted: false, - }; } let persisted = false;