From 572c91444166d92847cf19880e0f249a2f5824e9 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 15 Aug 2026 23:07:07 +0800 Subject: [PATCH 1/5] fix(serve): redact skill bodies from the Web Shell event surface (#9234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit available_commands_update snapshots embed every installed skill's full SKILL.md body for ACP clients (e.g. desktop), but no SSE/REST consumer reads them — with many skills installed each snapshot weighed ~640 KB that every browser tab parsed and discarded, pushing the renderer into sustained memory pressure until the tab crashed. Strip _meta.availableSkillDetails at the SDK/browser egress points (SSE frames and the session-load replay arrays); the /acp surface keeps delivering the full snapshot. --- packages/cli/src/serve/routes/session.ts | 5 +- packages/cli/src/serve/routes/sse-events.ts | 10 +- packages/cli/src/serve/server.test.ts | 198 ++++++++++++++++++ .../cli/src/serve/skill-details-redaction.ts | 79 +++++++ 4 files changed, 287 insertions(+), 5 deletions(-) create mode 100644 packages/cli/src/serve/skill-details-redaction.ts diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index f90de2ac94b..546ec90ed0c 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -93,6 +93,7 @@ import { } from '../server/session-export.js'; import { setDaemonTelemetryWorkspace } from '../server/telemetry.js'; import { createSessionOrganizationService } from '../session-organization-helpers.js'; +import { omitSkillDetailsFromReplayArrays } from '../skill-details-redaction.js'; import { replayTranscriptRecordPage } from '../../acp-integration/session/history-replay-page.js'; import { GENERATION_MAX_PROMPT_BYTES } from '../../acp-integration/generation.js'; import { @@ -2413,7 +2414,9 @@ export function registerSessionRoutes( } } } - res.status(200).json(session); + // The load response embeds the replay snapshot inline; redact the + // skill bodies there just like the SSE egress does (#9234). + res.status(200).json(omitSkillDetailsFromReplayArrays(session)); } catch (err) { sendBridgeError(res, err, { route, diff --git a/packages/cli/src/serve/routes/sse-events.ts b/packages/cli/src/serve/routes/sse-events.ts index 94695e3f183..3b4d2df77c2 100644 --- a/packages/cli/src/serve/routes/sse-events.ts +++ b/packages/cli/src/serve/routes/sse-events.ts @@ -33,6 +33,7 @@ import { parseMaxQueuedQuery, } from '../server/request-helpers.js'; import { parseEventEpochHeader } from '../sse-last-event-id.js'; +import { omitSkillDetailsForSdkSurface } from '../skill-details-redaction.js'; import type { WorkspaceRegistry } from '../workspace-registry.js'; import { requireSessionRuntime } from './session-runtime.js'; import { @@ -125,6 +126,7 @@ interface RegisterSseEventsRoutesDeps { type OmitId = Omit; function formatSseFrame(event: BridgeEvent | OmitId): string { + const shaped = omitSkillDetailsForSdkSurface(event); // SSE format: id (optional), event (optional), data, blank line. // The `id:` line is intentionally omitted when `event.id` is absent — // terminal/synthetic frames (e.g. daemon-side `stream_error`) must not @@ -142,7 +144,7 @@ function formatSseFrame(event: BridgeEvent | OmitId): string { // `_meta.serverTimestamp`: EventBus stamps normal session frames when they // are published so SSE and load/replay share the same event time. Keep this // fallback for synthetic frames that do not pass through EventBus. - const existingMeta = (event as { _meta?: Record })._meta; + const existingMeta = (shaped as { _meta?: Record })._meta; const existingServerTimestamp = existingMeta?.['serverTimestamp']; const serverTimestamp = typeof existingServerTimestamp === 'number' && @@ -150,13 +152,13 @@ function formatSseFrame(event: BridgeEvent | OmitId): string { ? existingServerTimestamp : Date.now(); const stamped = { - ...event, + ...shaped, _meta: { ...(existingMeta ?? {}), serverTimestamp }, }; const dataJson = JSON.stringify(stamped); const idLine = - 'id' in event && event.id !== undefined ? `id: ${event.id}\n` : ''; - return `${idLine}event: ${event.type}\ndata: ${dataJson}\n\n`; + 'id' in shaped && shaped.id !== undefined ? `id: ${shaped.id}\n` : ''; + return `${idLine}event: ${shaped.type}\ndata: ${dataJson}\n\n`; } export function registerSseEventsRoutes( diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index b4a76a9fc3e..981a79bd5e2 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -11881,6 +11881,77 @@ describe('createServeApp', () => { ]); }); + it('redacts skill bodies from the load response replay arrays (#9234)', async () => { + const commandsEvent = { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'persisted-replay', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'x'.repeat(600_000) }, + ], + }, + }, + }, + } satisfies BridgeEvent; + const textEvent = { + id: 2, + v: 1, + type: 'session_update', + data: { + sessionId: 'persisted-replay', + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'hi' }, + }, + }, + } satisfies BridgeEvent; + const bridge = fakeBridge({ + loadImpl: async (req) => ({ + sessionId: req.sessionId, + workspaceCwd: req.workspaceCwd, + attached: false, + clientId: 'client-load', + state: {}, + compactedReplay: [commandsEvent], + liveJournal: [textEvent], + }), + }); + const app = createServeApp(baseOpts, undefined, { bridge }); + const res = await request(app) + .post('/session/persisted-replay/load') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({}); + + expect(res.status).toBe(200); + const replay = res.body.compactedReplay as Array<{ + data: { update: Record }; + }>; + const update = replay[0]!.data.update; + expect(update['sessionUpdate']).toBe('available_commands_update'); + expect(update['availableCommands']).toEqual([ + { name: 'help', description: 'Help' }, + ]); + const meta = update['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(res.body.liveJournal).toEqual([textEvent]); + expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + // Bus events are shared with other subscribers (e.g. the /acp pump); + // the redaction must reshape immutably, never mutate the source. + expect( + (commandsEvent.data.update._meta as Record)[ + 'availableSkillDetails' + ], + ).toBeDefined(); + }); + it('passes client identity headers through to load/resume bridge calls', async () => { for (const action of ['load', 'resume'] as const) { const bridge = fakeBridge(); @@ -25549,6 +25620,133 @@ describe('GET /session/:id/events (SSE)', () => { expect(JSON.parse(frames[1]!.data!)).not.toHaveProperty('promptId'); }); + it('omits skill bodies from available_commands_update frames (#9234)', async () => { + // The daemon-side snapshot embeds every skill's full SKILL.md body for + // ACP clients; the SSE surface must strip it while keeping the command + // entries and the skill name list. + const sharedUpdate = { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { + name: 'bugfix', + description: 'Fix a bug', + body: 'x'.repeat(600_000), + filePath: '/skills/bugfix/SKILL.md', + level: 'project', + modelInvocable: true, + }, + ], + }, + }; + const bridge = fakeBridge({ + async *subscribeImpl() { + yield { + id: 1, + v: 1, + type: 'session_update', + data: { sessionId: 'sess-A', update: sharedUpdate }, + }; + yield { + id: 2, + v: 1, + type: 'session_update', + data: { + sessionId: 'sess-A', + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'hi' }, + }, + }, + }; + }, + }); + const app = createServeApp(baseOpts, undefined, { bridge }); + + const res = await request(app) + .get('/session/sess-A/events') + .set('Host', `127.0.0.1:${baseOpts.port}`); + + expect(res.status).toBe(200); + const payloads = res.text + .split('\n\n') + .map((raw) => { + const dataLine = raw + .split('\n') + .find((line) => line.startsWith('data: ')); + // Skip non-frame prelude lines such as `retry: 3000`. + if (!dataLine) return undefined; + return JSON.parse(dataLine.slice('data: '.length)) as { + id?: number; + data?: { update?: Record }; + }; + }) + .filter( + ( + payload, + ): payload is { + id?: number; + data?: { update?: Record }; + } => payload !== undefined, + ); + expect(payloads).toHaveLength(2); + const commandsUpdate = payloads[0]!.data!.update!; + expect(commandsUpdate['sessionUpdate']).toBe('available_commands_update'); + expect(commandsUpdate['availableCommands']).toEqual([ + { name: 'help', description: 'Help' }, + ]); + const meta = commandsUpdate['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(payloads[1]!.data!.update).toMatchObject({ + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'hi' }, + }); + expect(res.text).not.toContain('x'.repeat(64)); + // Bus events are shared with other subscribers (e.g. the /acp pump); + // the strip must reshape immutably, never mutate the source event. + expect(sharedUpdate._meta).toHaveProperty('availableSkillDetails'); + }); + + it('drops an available_commands_update _meta left empty by skill-detail stripping (#9234)', async () => { + const bridge = fakeBridge({ + async *subscribeImpl() { + yield { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'sess-A', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkillDetails: [{ name: 'bugfix', body: 'body' }], + }, + }, + }, + }; + }, + }); + const app = createServeApp(baseOpts, undefined, { bridge }); + + const res = await request(app) + .get('/session/sess-A/events') + .set('Host', `127.0.0.1:${baseOpts.port}`); + + expect(res.status).toBe(200); + const dataLine = res.text + .split('\n') + .find((line) => line.startsWith('data: ')); + expect(dataLine).toBeDefined(); + const payload = JSON.parse(dataLine!.slice('data: '.length)) as { + data?: { update?: Record }; + }; + expect(payload.data!.update).not.toHaveProperty('_meta'); + }); + it('correlates the SSE response, daemon lifecycle log, and request span', async () => { const predecessor = '019535d9-3df7-7a61-8f6d-6f37c39c5f19'; const setAttribute = vi.fn(); diff --git a/packages/cli/src/serve/skill-details-redaction.ts b/packages/cli/src/serve/skill-details-redaction.ts new file mode 100644 index 00000000000..33badcf04e2 --- /dev/null +++ b/packages/cli/src/serve/skill-details-redaction.ts @@ -0,0 +1,79 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { BridgeEvent } from '@qwen-code/acp-bridge/eventBus'; + +/** + * `available_commands_update` snapshots embed every installed skill's full + * SKILL.md body under `update._meta.availableSkillDetails` for ACP clients + * that display or edit skill files (e.g. desktop). The SDK/browser surface + * (SSE streams and REST responses) only reads the command entries and the + * `availableSkills` name list, so with many skills installed the bodies are + * hundreds of kilobytes of dead weight that every browser tab parses and + * discards on each snapshot (#9234). The `/acp` surface keeps delivering the + * full snapshot; apply this at every SDK/browser egress point. Frames are + * shared with other bus subscribers, so reshape immutably instead of + * mutating. + */ +export function omitSkillDetailsForSdkSurface< + T extends { type: string; data: unknown }, +>(event: T): T { + if (event.type !== 'session_update') return event; + const data = asRecord(event.data); + if (!data) return event; + const update = asRecord(data['update']); + if (!update || update['sessionUpdate'] !== 'available_commands_update') { + return event; + } + const meta = asRecord(update['_meta']); + if (!meta || !('availableSkillDetails' in meta)) return event; + const trimmedMeta: Record = {}; + for (const [key, value] of Object.entries(meta)) { + if (key !== 'availableSkillDetails') trimmedMeta[key] = value; + } + const nextUpdate: Record = { ...update }; + if (Object.keys(trimmedMeta).length > 0) { + nextUpdate['_meta'] = trimmedMeta; + } else { + delete nextUpdate['_meta']; + } + return { ...event, data: { ...data, update: nextUpdate } }; +} + +/** + * `POST /session/:id/load` embeds the replay snapshot (compacted turns plus + * the in-flight journal) directly in the response body; apply the same + * redaction to those frames as the SSE egress does. + */ +export function omitSkillDetailsFromReplayArrays< + T extends { + compactedReplay?: BridgeEvent[]; + liveJournal?: BridgeEvent[]; + }, +>(session: T): T { + if (!session.compactedReplay && !session.liveJournal) return session; + return { + ...session, + ...(session.compactedReplay + ? { + compactedReplay: session.compactedReplay.map( + omitSkillDetailsForSdkSurface, + ), + } + : {}), + ...(session.liveJournal + ? { + liveJournal: session.liveJournal.map(omitSkillDetailsForSdkSurface), + } + : {}), + }; +} + +function asRecord(value: unknown): Record | undefined { + return typeof value === 'object' && value !== null && !Array.isArray(value) + ? (value as Record) + : undefined; +} From 429253fe31bb9a95a6f0d1f30ed4163a947d9bb8 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 01:47:15 +0800 Subject: [PATCH 2/5] fix(serve): redact skill bodies from branch/side-task responses too (#9234) Review follow-up: POST /session/:id/branch and POST /session/:id/side-task serialize the same replay snapshot shape as load/resume but were missed by the redaction. Wrap both 201 responses the same way, harden the load test so a dropped liveJournal redaction can no longer pass silently, and pin the /acp surface's verbatim retention of availableSkillDetails with a mirror test. --- .../cli/src/serve/acp-http/transport.test.ts | 50 ++++++ packages/cli/src/serve/routes/session.ts | 16 +- packages/cli/src/serve/server.test.ts | 162 +++++++++++++++++- 3 files changed, 224 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/serve/acp-http/transport.test.ts b/packages/cli/src/serve/acp-http/transport.test.ts index d960085af7c..5cf79c5d1b4 100644 --- a/packages/cli/src/serve/acp-http/transport.test.ts +++ b/packages/cli/src/serve/acp-http/transport.test.ts @@ -6296,6 +6296,56 @@ describe('ACP Streamable HTTP transport (over the wire)', () => { // (takeFrames already locked + aborted `sess`; afterEach force-closes.) }); + it('keeps availableSkillDetails verbatim on pumped session_update frames (#9234)', async () => { + // Mirror of the SSE redaction tests: the SDK/browser surface strips + // `_meta.availableSkillDetails`, but the /acp surface must keep + // delivering it untouched — desktop clients parse the skill bodies + // for display/editing. + const connId = await initialize(); + await newSession(connId); + const sess = await openStream(connId, 'sess-1'); + const got = takeFrames(sess, 1); + await new Promise((r) => setTimeout(r, 50)); + const skillDetails = [ + { name: 'bugfix', body: 'skill body', filePath: '/skills/bugfix' }, + ]; + bridge.queues.get('sess-1')?.push({ + type: 'session_update', + data: { + sessionId: 'sess-1', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: skillDetails, + }, + }, + }, + }); + const frames = (await got) as Array<{ + method: string; + params: { + sessionId: string; + update: { _meta?: { availableSkillDetails?: unknown } }; + }; + }>; + expect(frames[0]).toMatchObject({ + method: 'session/update', + params: { + sessionId: 'sess-1', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: skillDetails, + }, + }, + }, + }); + }); + it('session/load while a session/close is in-flight → rejected (TOCTOU guard)', async () => { let releaseClose: () => void = () => {}; bridge.closeGate = new Promise((r) => (releaseClose = r)); diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 546ec90ed0c..96c5277fc55 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -2599,7 +2599,19 @@ export function registerSessionRoutes( } } if (!res.writable) return; - res.status(201).json(result); + // Branch/side-task responses carry the same replay snapshot shape as + // load; apply the same redaction (#9234). Checkpoint branches + // (`atRecordId` set) return no replay arrays, so only the restored + // variant needs shaping. + res + .status(201) + .json( + atRecordId === undefined + ? omitSkillDetailsFromReplayArrays( + result as BridgeBranchedSession, + ) + : result, + ); }, ), ); @@ -2663,7 +2675,7 @@ export function registerSessionRoutes( } return; } - res.status(201).json(result); + res.status(201).json(omitSkillDetailsFromReplayArrays(result)); }, ), ); diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 981a79bd5e2..30201e32659 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -11912,6 +11912,26 @@ describe('createServeApp', () => { }, }, } satisfies BridgeEvent; + // The in-flight journal can hold a fresher snapshot than the compacted + // turns (mid-turn load); it must be redacted too. + const journalCommandsEvent = { + id: 3, + v: 1, + type: 'session_update', + data: { + sessionId: 'persisted-replay', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'y'.repeat(600_000) }, + ], + }, + }, + }, + } satisfies BridgeEvent; const bridge = fakeBridge({ loadImpl: async (req) => ({ sessionId: req.sessionId, @@ -11920,7 +11940,7 @@ describe('createServeApp', () => { clientId: 'client-load', state: {}, compactedReplay: [commandsEvent], - liveJournal: [textEvent], + liveJournal: [journalCommandsEvent, textEvent], }), }); const app = createServeApp(baseOpts, undefined, { bridge }); @@ -11941,8 +11961,20 @@ describe('createServeApp', () => { const meta = update['_meta'] as Record; expect(meta['availableSkills']).toEqual(['bugfix']); expect(meta).not.toHaveProperty('availableSkillDetails'); - expect(res.body.liveJournal).toEqual([textEvent]); + const journal = res.body.liveJournal as Array<{ + data: { update: Record }; + }>; + const journalUpdate = journal[0]!.data.update; + expect(journalUpdate['sessionUpdate']).toBe('available_commands_update'); + expect(journalUpdate['availableCommands']).toEqual([ + { name: 'help', description: 'Help' }, + ]); + const journalMeta = journalUpdate['_meta'] as Record; + expect(journalMeta['availableSkills']).toEqual(['bugfix']); + expect(journalMeta).not.toHaveProperty('availableSkillDetails'); + expect(journal[1]).toEqual(textEvent); expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + expect(JSON.stringify(res.body)).not.toContain('y'.repeat(64)); // Bus events are shared with other subscribers (e.g. the /acp pump); // the redaction must reshape immutably, never mutate the source. expect( @@ -17442,6 +17474,132 @@ describe('createServeApp', () => { await fsp.rm(runtimeDir, { recursive: true, force: true }); } }); + + it('redacts skill bodies from the branch response replay arrays (#9234)', async () => { + const commandsEvent = { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'branched-session', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'x'.repeat(600_000) }, + ], + }, + }, + }, + } satisfies BridgeEvent; + const bridge = fakeBridge(); + bridge.branchSession = vi.fn(async (sessionId) => ({ + sessionId: 'branched-session', + workspaceCwd: WS_BOUND, + attached: false, + clientId: 'client-branch', + state: {}, + displayName: 'Branched', + forkedFrom: { sessionId, displayName: 'Source' }, + compactedReplay: [commandsEvent], + })); + const runtime = makeWorkspaceRuntimeForTest({ + workspaceId: 'branch-redaction', + workspaceCwd: WS_BOUND, + primary: true, + bridge, + generationGuard: createWorkspaceGenerationGuard(), + }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { workspaceRegistry: createWorkspaceRegistry([runtime]) }, + ); + + const res = await request(app) + .post('/session/source-session/branch') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({}); + + expect(res.status).toBe(201); + const replay = res.body.compactedReplay as Array<{ + data: { update: Record }; + }>; + const update = replay[0]!.data.update; + expect(update['sessionUpdate']).toBe('available_commands_update'); + expect(update['availableCommands']).toEqual([ + { name: 'help', description: 'Help' }, + ]); + const meta = update['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + }); + }); + + describe('POST /session/:id/side-task (skill-detail redaction, #9234)', () => { + it('redacts skill bodies from the side-task response replay arrays', async () => { + const commandsEvent = { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'side-task-session', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'x'.repeat(600_000) }, + ], + }, + }, + }, + } satisfies BridgeEvent; + const bridge = fakeBridge(); + bridge.createSideTaskSession = vi.fn(async () => ({ + sessionId: 'side-task-session', + workspaceCwd: WS_BOUND, + attached: false, + clientId: 'client-side-task', + state: {}, + liveJournal: [commandsEvent], + })); + const runtime = makeWorkspaceRuntimeForTest({ + workspaceId: 'side-task-redaction', + workspaceCwd: WS_BOUND, + primary: true, + bridge, + generationGuard: createWorkspaceGenerationGuard(), + }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { workspaceRegistry: createWorkspaceRegistry([runtime]) }, + ); + + const res = await request(app) + .post('/session/source-session/side-task') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ name: 'follow-up' }); + + expect(res.status).toBe(201); + const journal = res.body.liveJournal as Array<{ + data: { update: Record }; + }>; + const update = journal[0]!.data.update; + expect(update['sessionUpdate']).toBe('available_commands_update'); + expect(update['availableCommands']).toEqual([ + { name: 'help', description: 'Help' }, + ]); + const meta = update['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + }); }); describe('POST /session/:id/fork', () => { From b5a4e74ff6f3f7a747809201f17e5dfa15399b54 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 08:59:12 +0800 Subject: [PATCH 3/5] fix(serve): harden skill-detail redaction per review (flat frames, envelope pins) - R2-1: recognize the persisted-transcript flat frame shape (data.sessionUpdate) in addition to the eventBus-wrapped shape, with a regression test using a flat frame in compactedReplay. - R3-1: apply the replay-array redaction unconditionally in the branch route instead of re-deriving the bridge's variant discrimination. - R3-2: pin the full frame envelope and response sessionId in the redaction tests so envelope-level regressions cannot ship green. - R3-3: add a colocated skill-details-redaction.test.ts unit suite. --- packages/cli/src/serve/routes/session.ts | 13 +- packages/cli/src/serve/server.test.ts | 95 ++++++++-- .../src/serve/skill-details-redaction.test.ts | 163 ++++++++++++++++++ .../cli/src/serve/skill-details-redaction.ts | 23 ++- 4 files changed, 263 insertions(+), 31 deletions(-) create mode 100644 packages/cli/src/serve/skill-details-redaction.test.ts diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 96c5277fc55..032dd2dd26d 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -2600,17 +2600,14 @@ export function registerSessionRoutes( } if (!res.writable) return; // Branch/side-task responses carry the same replay snapshot shape as - // load; apply the same redaction (#9234). Checkpoint branches - // (`atRecordId` set) return no replay arrays, so only the restored - // variant needs shaping. + // load; apply the same redaction (#9234). The helper returns its + // input unchanged when no replay arrays are present (checkpoint + // branches), so apply it unconditionally rather than re-deriving the + // bridge's variant discrimination here. res .status(201) .json( - atRecordId === undefined - ? omitSkillDetailsFromReplayArrays( - result as BridgeBranchedSession, - ) - : result, + omitSkillDetailsFromReplayArrays(result as BridgeBranchedSession), ); }, ), diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 30201e32659..8fb9f62792d 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -11950,28 +11950,35 @@ describe('createServeApp', () => { .send({}); expect(res.status).toBe(200); + expect(res.body).toMatchObject({ sessionId: 'persisted-replay' }); const replay = res.body.compactedReplay as Array<{ data: { update: Record }; }>; - const update = replay[0]!.data.update; - expect(update['sessionUpdate']).toBe('available_commands_update'); - expect(update['availableCommands']).toEqual([ - { name: 'help', description: 'Help' }, - ]); - const meta = update['_meta'] as Record; - expect(meta['availableSkills']).toEqual(['bugfix']); - expect(meta).not.toHaveProperty('availableSkillDetails'); + // Pin the full envelope (id/v/type/data.sessionId) so envelope-level + // regressions in the reshape cannot ship green (review R3-2). + expect(replay[0]).toEqual({ + ...commandsEvent, + data: { + ...commandsEvent.data, + update: { + ...commandsEvent.data.update, + _meta: { availableSkills: ['bugfix'] }, + }, + }, + }); const journal = res.body.liveJournal as Array<{ data: { update: Record }; }>; - const journalUpdate = journal[0]!.data.update; - expect(journalUpdate['sessionUpdate']).toBe('available_commands_update'); - expect(journalUpdate['availableCommands']).toEqual([ - { name: 'help', description: 'Help' }, - ]); - const journalMeta = journalUpdate['_meta'] as Record; - expect(journalMeta['availableSkills']).toEqual(['bugfix']); - expect(journalMeta).not.toHaveProperty('availableSkillDetails'); + expect(journal[0]).toEqual({ + ...journalCommandsEvent, + data: { + ...journalCommandsEvent.data, + update: { + ...journalCommandsEvent.data.update, + _meta: { availableSkills: ['bugfix'] }, + }, + }, + }); expect(journal[1]).toEqual(textEvent); expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); expect(JSON.stringify(res.body)).not.toContain('y'.repeat(64)); @@ -11984,6 +11991,54 @@ describe('createServeApp', () => { ).toBeDefined(); }); + it('redacts flat persisted-transcript frames in replay arrays (#9234)', async () => { + // Persisted-transcript frames carry the ACP update flat under `data` + // (no `update` wrapper); the redactor must handle both shapes. + const flatCommandsEvent = { + id: 7, + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'LEAK-CANARY-SKILL-BODY'.repeat(100) }, + ], + }, + }, + } satisfies BridgeEvent; + const bridge = fakeBridge({ + loadImpl: async (req) => ({ + sessionId: req.sessionId, + workspaceCwd: req.workspaceCwd, + attached: false, + clientId: 'client-load', + state: {}, + compactedReplay: [flatCommandsEvent], + }), + }); + const app = createServeApp(baseOpts, undefined, { bridge }); + const res = await request(app) + .post('/session/persisted-flat/load') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({}); + + expect(res.status).toBe(200); + const replay = res.body.compactedReplay as Array<{ + data: Record; + }>; + expect(replay[0]).toEqual({ + ...flatCommandsEvent, + data: { + ...flatCommandsEvent.data, + _meta: { availableSkills: ['bugfix'] }, + }, + }); + expect(JSON.stringify(res.body)).not.toContain('LEAK-CANARY-SKILL-BODY'); + }); + it('passes client identity headers through to load/resume bridge calls', async () => { for (const action of ['load', 'resume'] as const) { const bridge = fakeBridge(); @@ -25850,6 +25905,14 @@ describe('GET /session/:id/events (SSE)', () => { } => payload !== undefined, ); expect(payloads).toHaveLength(2); + // Pin the envelope the reshape must preserve (SSE id line, schema + // version, session attribution) — review R3-2 mutant M1. + expect(payloads[0]).toMatchObject({ + id: 1, + v: 1, + type: 'session_update', + data: { sessionId: 'sess-A' }, + }); const commandsUpdate = payloads[0]!.data!.update!; expect(commandsUpdate['sessionUpdate']).toBe('available_commands_update'); expect(commandsUpdate['availableCommands']).toEqual([ diff --git a/packages/cli/src/serve/skill-details-redaction.test.ts b/packages/cli/src/serve/skill-details-redaction.test.ts new file mode 100644 index 00000000000..210ceaf9266 --- /dev/null +++ b/packages/cli/src/serve/skill-details-redaction.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from 'vitest'; +import type { BridgeEvent } from '@qwen-code/acp-bridge/eventBus'; +import { + omitSkillDetailsForSdkSurface, + omitSkillDetailsFromReplayArrays, +} from './skill-details-redaction.js'; + +interface CommandsData { + sessionUpdate: string; + availableCommands: Array<{ name: string; description: string }>; + _meta?: Record; +} +interface WrappedEvent extends BridgeEvent { + data: { sessionId: string; update: CommandsData }; +} +interface FlatEvent extends BridgeEvent { + data: CommandsData; +} + +function wrappedCommandsEvent(): WrappedEvent { + return { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'sess-1', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [{ name: 'bugfix', body: 'skill body' }], + other: 'kept', + }, + }, + }, + }; +} + +function flatCommandsEvent(): FlatEvent { + return { + id: 2, + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [{ name: 'bugfix', body: 'skill body' }], + }, + }, + }; +} + +describe('omitSkillDetailsForSdkSurface', () => { + it('strips availableSkillDetails from wrapped frames, keeping the rest', () => { + const shaped = omitSkillDetailsForSdkSurface(wrappedCommandsEvent()); + expect(shaped).toEqual({ + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'sess-1', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { availableSkills: ['bugfix'], other: 'kept' }, + }, + }, + }); + }); + + it('strips availableSkillDetails from flat persisted-transcript frames', () => { + const shaped = omitSkillDetailsForSdkSurface(flatCommandsEvent()); + expect(shaped).toEqual({ + id: 2, + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { availableSkills: ['bugfix'] }, + }, + }); + }); + + it('drops _meta entirely when stripping leaves it empty', () => { + const event = wrappedCommandsEvent(); + event.data.update._meta = { + availableSkillDetails: [{ name: 'bugfix', body: 'skill body' }], + }; + const shaped = omitSkillDetailsForSdkSurface(event); + expect(shaped.data.update).not.toHaveProperty('_meta'); + }); + + it('passes through non-available_commands_update events unchanged', () => { + const event: BridgeEvent = { + id: 3, + v: 1, + type: 'session_update', + data: { + sessionId: 'sess-1', + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'hi' }, + }, + }, + }; + expect(omitSkillDetailsForSdkSurface(event)).toBe(event); + }); + + it('passes through commands frames without availableSkillDetails unchanged', () => { + const event = wrappedCommandsEvent(); + event.data.update._meta = { availableSkills: ['bugfix'] }; + expect(omitSkillDetailsForSdkSurface(event)).toBe(event); + }); + + it('never mutates the source event', () => { + const event = wrappedCommandsEvent(); + omitSkillDetailsForSdkSurface(event); + expect(event.data.update._meta?.['availableSkillDetails']).toEqual([ + { name: 'bugfix', body: 'skill body' }, + ]); + }); +}); + +describe('omitSkillDetailsFromReplayArrays', () => { + it('redacts both arrays when both are present', () => { + const shaped = omitSkillDetailsFromReplayArrays({ + sessionId: 'sess-1', + compactedReplay: [wrappedCommandsEvent()], + liveJournal: [flatCommandsEvent()], + }); + expect(shaped.sessionId).toBe('sess-1'); + expect(shaped.compactedReplay[0].data.update._meta).not.toHaveProperty( + 'availableSkillDetails', + ); + expect(shaped.liveJournal[0].data._meta).not.toHaveProperty( + 'availableSkillDetails', + ); + }); + + it('redacts a single present array', () => { + const shaped = omitSkillDetailsFromReplayArrays({ + sessionId: 'sess-1', + liveJournal: [wrappedCommandsEvent()], + }); + expect(shaped.liveJournal[0].data.update._meta).toEqual({ + availableSkills: ['bugfix'], + other: 'kept', + }); + }); + + it('returns its input unchanged when no replay arrays are present', () => { + const session = { + sessionId: 'sess-1', + displayName: 'Branch', + compactedReplay: undefined, + }; + expect(omitSkillDetailsFromReplayArrays(session)).toBe(session); + }); +}); diff --git a/packages/cli/src/serve/skill-details-redaction.ts b/packages/cli/src/serve/skill-details-redaction.ts index 33badcf04e2..1786c3e8398 100644 --- a/packages/cli/src/serve/skill-details-redaction.ts +++ b/packages/cli/src/serve/skill-details-redaction.ts @@ -24,23 +24,32 @@ export function omitSkillDetailsForSdkSurface< if (event.type !== 'session_update') return event; const data = asRecord(event.data); if (!data) return event; - const update = asRecord(data['update']); - if (!update || update['sessionUpdate'] !== 'available_commands_update') { + // Two documented frame shapes: eventBus-wrapped (`data.update.*`) and + // persisted-transcript flat (`data.*`); the bridge's + // `transcriptEventRecordId` accepts both, so the redactor must too. + const wrapped = asRecord(data['update']); + const flat = !wrapped && data['sessionUpdate'] !== undefined; + const candidate = flat ? data : wrapped; + if ( + !candidate || + candidate['sessionUpdate'] !== 'available_commands_update' + ) { return event; } - const meta = asRecord(update['_meta']); + const meta = asRecord(candidate['_meta']); if (!meta || !('availableSkillDetails' in meta)) return event; const trimmedMeta: Record = {}; for (const [key, value] of Object.entries(meta)) { if (key !== 'availableSkillDetails') trimmedMeta[key] = value; } - const nextUpdate: Record = { ...update }; + const nextCandidate: Record = { ...candidate }; if (Object.keys(trimmedMeta).length > 0) { - nextUpdate['_meta'] = trimmedMeta; + nextCandidate['_meta'] = trimmedMeta; } else { - delete nextUpdate['_meta']; + delete nextCandidate['_meta']; } - return { ...event, data: { ...data, update: nextUpdate } }; + if (flat) return { ...event, data: nextCandidate }; + return { ...event, data: { ...data, update: nextCandidate } }; } /** From 2e7e414e1ae83cba1aacbe8d29fe57cc84776705 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 10:17:12 +0800 Subject: [PATCH 4/5] fix(serve): close remaining skill-body egresses per review (#9234) - Redact the virtual-subagent load replay (same BridgeEvent[] shape as the load response). - Redact flat persisted-transcript frames on both transcript routes, where the flat shape is actually produced. - Add regression tests for both paths and the Apache-2.0 header. --- packages/cli/src/serve/routes/session.ts | 29 +++++-- packages/cli/src/serve/server.test.ts | 101 +++++++++++++++++++++++ 2 files changed, 122 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 032dd2dd26d..60e7d161bc4 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -93,7 +93,10 @@ import { } from '../server/session-export.js'; import { setDaemonTelemetryWorkspace } from '../server/telemetry.js'; import { createSessionOrganizationService } from '../session-organization-helpers.js'; -import { omitSkillDetailsFromReplayArrays } from '../skill-details-redaction.js'; +import { + omitSkillDetailsForSdkSurface, + omitSkillDetailsFromReplayArrays, +} from '../skill-details-redaction.js'; import { replayTranscriptRecordPage } from '../../acp-integration/session/history-replay-page.js'; import { GENERATION_MAX_PROMPT_BYTES } from '../../acp-integration/generation.js'; import { @@ -2101,7 +2104,9 @@ export function registerSessionRoutes( }); return; } - res.status(200).json(session); + // Same replay-array shape as the load response; redact skill + // bodies for the browser surface (#9234). + res.status(200).json(omitSkillDetailsFromReplayArrays(session)); } catch (err) { sendBridgeError(res, err, { route, sessionId }); } @@ -2840,7 +2845,13 @@ export function registerSessionRoutes( }, ); if (result === undefined) return; - res.status(200).set('Cache-Control', 'no-store').json(result); + res + .status(200) + .set('Cache-Control', 'no-store') + .json({ + ...result, + events: result.events.map(omitSkillDetailsForSdkSurface), + }); } catch (err) { sendBridgeError(res, err, { route, @@ -2960,11 +2971,13 @@ export function registerSessionRoutes( return { v: 1 as const, sessionId, - events: replay.updates.map((update) => ({ - v: 1 as const, - type: 'session_update' as const, - data: update, - })), + events: replay.updates.map((update) => + omitSkillDetailsForSdkSurface({ + v: 1 as const, + type: 'session_update' as const, + data: update, + }), + ), ...(replay.nextCursor && !cursorTooLarge ? { nextCursor: replay.nextCursor } : {}), diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 8fb9f62792d..4c669037443 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -11617,6 +11617,63 @@ describe('createServeApp', () => { expect(bridge.resumeCalls).toEqual([]); }); + it('redacts skill bodies from virtual subagent load replay (#9234)', async () => { + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const sessionId = createVirtualSubagentSessionId('parent-1', 'agent-1'); + const commandsEvent = { + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'parent-1', + update: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'x'.repeat(600_000) }, + ], + }, + }, + }, + }; + const loadSpy = vi + .spyOn(VirtualSubagentSessions.prototype, 'load') + .mockResolvedValue({ + sessionId, + workspaceCwd: WS_BOUND, + attached: true, + clientId: 'client-v', + state: {}, + compactedReplay: [commandsEvent], + liveJournal: [], + }); + + try { + const res = await request(app) + .post(`/session/${sessionId}/load`) + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({}); + + expect(res.status).toBe(200); + const replay = res.body.compactedReplay as Array<{ + data: { update: Record }; + }>; + const meta = replay[0]!.data.update['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + } finally { + loadSpy.mockRestore(); + } + }); + it('passes the requested initial history page size to load', async () => { const bridge = fakeBridge(); const app = createServeApp( @@ -21292,6 +21349,50 @@ describe('createServeApp', () => { expect(bridge.resumeCalls).toHaveLength(0); }); + it('redacts skill bodies from flat transcript events (#9234)', async () => { + const sid = '55555555-bbbb-cccc-dddd-aaaaaaaaaaab'; + const bridge = fakeBridge({ + sessionTranscriptImpl: async (req) => ({ + v: 1, + sessionId: req.sessionId, + events: [ + { + v: 1, + type: 'session_update', + data: { + sessionUpdate: 'available_commands_update', + availableCommands: [{ name: 'help', description: 'Help' }], + _meta: { + availableSkills: ['bugfix'], + availableSkillDetails: [ + { name: 'bugfix', body: 'x'.repeat(600_000) }, + ], + }, + }, + }, + ], + hasMore: false, + }), + }); + await writeTranscriptSession(sid); + const app = createServeApp({ ...baseOpts, workspace: wsDir }, undefined, { + bridge, + boundWorkspace: wsDir, + }); + + const res = await request(app) + .get(`/session/${sid}/transcript`) + .set('Host', `127.0.0.1:${baseOpts.port}`); + + expect(res.status).toBe(200); + const event = res.body.events[0] as { data: Record }; + expect(event.data['sessionUpdate']).toBe('available_commands_update'); + const meta = event.data['_meta'] as Record; + expect(meta['availableSkills']).toEqual(['bugfix']); + expect(meta).not.toHaveProperty('availableSkillDetails'); + expect(JSON.stringify(res.body)).not.toContain('x'.repeat(64)); + }); + it('forwards an exclusive persisted-record boundary', async () => { const sid = '55555555-bbbb-cccc-dddd-bbbbbbbbbbbb'; const bridge = fakeBridge({ From 48a0760ab4fd385f59783aa44b2f45ad6b11e80e Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 11:23:38 +0800 Subject: [PATCH 5/5] fix(serve): tolerate transcript pages without events in redaction (#9234) Guard the transcript-route redaction with `?? []` so a page payload that omits `events` (as some test fakes produce) no longer throws a 500. --- packages/cli/src/serve/routes/session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/serve/routes/session.ts b/packages/cli/src/serve/routes/session.ts index 60e7d161bc4..fe1226988e3 100644 --- a/packages/cli/src/serve/routes/session.ts +++ b/packages/cli/src/serve/routes/session.ts @@ -2850,7 +2850,7 @@ export function registerSessionRoutes( .set('Cache-Control', 'no-store') .json({ ...result, - events: result.events.map(omitSkillDetailsForSdkSurface), + events: (result.events ?? []).map(omitSkillDetailsForSdkSurface), }); } catch (err) { sendBridgeError(res, err, {