Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions packages/cli/src/serve/acp-http/transport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>((r) => (releaseClose = r));
Expand Down
45 changes: 35 additions & 10 deletions packages/cli/src/serve/routes/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ import {
} from '../server/session-export.js';
import { setDaemonTelemetryWorkspace } from '../server/telemetry.js';
import { createSessionOrganizationService } from '../session-organization-helpers.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 {
Expand Down Expand Up @@ -2100,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 });
}
Expand Down Expand Up @@ -2413,7 +2419,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));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Un-redacted browser egress: the virtual-subagent load path.

restoreSessionHandler has an earlier return for virtual subagent ids (packages/cli/src/serve/routes/session.ts:2104):

const session = await virtualSubagentSessions.load(runtime, sessionId, clientId);
...
res.status(200).json(session);   // <- not redacted

VirtualSubagentSessions.load() returns compactedReplay: snapshot.events (virtual-subagent-sessions.ts:645), i.e. the same BridgeEvent[] shape this line redacts, but it never passes through omitSkillDetailsFromReplayArrays. The Web Shell opens subagent panes through exactly this route.

Scenario: any available_commands_update frame that ends up on a virtual subagent's EventBus (the bus is generic and refreshLive/readStreamUpdates republish whatever the target produces) is delivered to the browser with every SKILL.md body inline — the payload this PR removes from every other browser load path. Today's transcript-derived updates happen not to include that frame, so this is a defense-in-depth gap rather than a live leak, but it is the one res.json of a replay array the PR leaves behind.

Suggest routing it through the helper too:

Suggested change
res.status(200).json(omitSkillDetailsFromReplayArrays(session));
res.status(200).json(omitSkillDetailsFromReplayArrays(session));

(applies at line 2104, not here)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2e7e414. The virtual-subagent load response now goes through omitSkillDetailsFromReplayArrays — same BridgeEvent[] shape as the load response, so the one remaining replay-array res.json is covered. Regression test spies VirtualSubagentSessions.prototype.load and asserts the stripped envelope.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flat-frame branch's real producer is still un-redacted: GET /session/:id/transcript.

Commit b5a4e74 added the flat (data.sessionUpdate, no update wrapper) branch to omitSkillDetailsForSdkSurface, citing "persisted-transcript" frames. The route that actually serves persisted-transcript frames to the browser builds them at packages/cli/src/serve/routes/session.ts:2963 and sends them verbatim:

events: replay.updates.map((update) => ({
  v: 1 as const,
  type: 'session_update' as const,
  data: update,          // flat shape, exactly what the new branch handles
})),

Neither GET /session/:id/transcript nor GET /workspaces/:workspace/session/:id/transcript applies the redaction, and the Web Shell paginates history through them. Scenario: if an available_commands_update ever reaches collectHistoryReplayUpdates (a persisted record kind that replays to it), the browser gets the full skill bodies on every history page while the load path strips them — inconsistent, and it reopens the memory-pressure path this PR closes. Consider mapping the array through omitSkillDetailsForSdkSurface here so the flat branch is exercised where flat frames are actually produced.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2e7e414. Both transcript routes now map their frames through omitSkillDetailsForSdkSurface — the workspace-qualified route at the flat-frame construction site and /session/:id/transcript over result.events — so the flat branch is exercised where flat frames are actually produced. Regression test asserts a flat available_commands_update in a transcript page is stripped.

} catch (err) {
sendBridgeError(res, err, {
route,
Expand Down Expand Up @@ -2596,7 +2604,16 @@ 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). 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(
omitSkillDetailsFromReplayArrays(result as BridgeBranchedSession),
);
},
),
);
Expand Down Expand Up @@ -2660,7 +2677,7 @@ export function registerSessionRoutes(
}
return;
}
res.status(201).json(result);
res.status(201).json(omitSkillDetailsFromReplayArrays(result));
},
),
);
Expand Down Expand Up @@ -2828,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),
});
Comment on lines +2851 to +2854

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R5-1: The new redaction mapping dereferences result.events unguarded, which turns two previously-green tests in session-telemetry.test.ts red — Failure scenario: that file's bridge mock returns { records: [] } (no events array); result.events.map(omitSkillDetailsForSdkSurface) throws TypeError: Cannot read properties of undefined (reading 'map'), the catch routes it to sendBridgeError, and the client gets 500 instead of 200 — publishes the live transcript owner in a multi-workspace daemon and publishes the sole active transcript runtime after storage lookup both fail with expected 500 to be 200. Measured net-new by A/B: this file fails 2/9 on this PR and passes 9/9 on merge base 3119d53e4 — this is also why CI's Test (ubuntu-latest, Node 22.x) is red at the reviewed commit. Production exposure is bounded (BridgeSessionTranscriptPage declares events required), but the diff breaks the project's own suite.

Witness (A/B, same command in both trees):

PR:   npx vitest run src/serve/routes/session-telemetry.test.ts → 2 failed | 7 passed
base: npx vitest run src/serve/routes/session-telemetry.test.ts → 9 passed
Suggested change
.json({
...result,
events: result.events.map(omitSkillDetailsForSdkSurface),
});
.json({
...result,
events: (result.events ?? []).map(omitSkillDetailsForSdkSurface),
});

The ?? [] guard keeps the pre-PR tolerance for pages without events (the old code passed whatever the bridge returned straight through). Alternatively, bring the telemetry mock to the full BridgeSessionTranscriptPage shape — then re-run npx vitest run src/serve/routes/session-telemetry.test.ts (9/9 expected).

中文说明

[Critical] 新增的脱敏映射未经防护地解引用 result.events,导致 session-telemetry.test.ts 中两个原本通过的测试变红——失败场景:该文件的 bridge mock 返回 { records: [] }(没有 events 数组);result.events.map(omitSkillDetailsForSdkSurface) 抛出 TypeError: Cannot read properties of undefined (reading 'map'),catch 将其路由到 sendBridgeError,客户端收到 500 而非 200——publishes the live transcript owner in a multi-workspace daemonpublishes the sole active transcript runtime after storage lookup 均以 expected 500 to be 200 失败。经 A/B 测量为净新增失败:该文件在本 PR 上 2/9 失败,在合并基 3119d53e4 上 9/9 通过——这也是受审 commit 上 CI 的 Test (ubuntu-latest, Node 22.x) 变红的原因。生产暴露面有限(BridgeSessionTranscriptPage 声明 events 为必需字段),但本 diff 破坏了项目自身的测试套件。证据(两条树上执行同一命令的 A/B):PR 侧 2 failed | 7 passed;基线侧 9 passed。建议修复:?? [] 防护可保留修复前对缺省 events 的容忍(旧代码会将 bridge 返回的内容原样透传);或者将 telemetry mock 补全为完整的 BridgeSessionTranscriptPage 形状——修复后重跑 npx vitest run src/serve/routes/session-telemetry.test.ts(预期 9/9 通过)。

— qwen3.8-max via Qwen Code /review (v0.21.12)

} catch (err) {
sendBridgeError(res, err, {
route,
Expand Down Expand Up @@ -2948,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({
Comment on lines +2974 to +2975

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R5-2: The workspace-qualified transcript route (GET /workspaces/:workspace/session/:id/transcript) is still the only redaction call site with no route-level regression test — Failure scenario: the redaction tests in server.test.ts hit only /session/:id/transcript, /load, /branch, /side-task, and the SSE stream; multi-workspace-sessions.test.ts (the only file exercising this route) contains no available_commands_update record and no availableSkillDetails, so removing the omitSkillDetailsForSdkSurface wrapper here would regress full SKILL.md bodies onto the browser-facing workspace transcript response with every test staying green. Mutation-verified at the reviewed commit: removing the wrapper keeps all 1071 tests green across the four relevant suites. The wrapper is currently defense-in-depth (the persisted replay never emits available_commands_update), so this is a coverage gap, not an active leak. Suggested fix: add a workspace-transcript test that persists an available_commands_update record carrying availableSkillDetails and asserts the response events lack the key (plus a body canary), mirroring the sibling test for /session/:id/transcript.

Witness (mutation probe at 48a0760ab4):

wrapper removed at session.ts:2974-2975 →
  server.test.ts + multi-workspace-sessions.test.ts + session-telemetry.test.ts + skill-details-redaction.test.ts:
  1071 passed, 0 failed
中文说明

[Suggestion] R5-2:工作区限定的 transcript 路由(GET /workspaces/:workspace/session/:id/transcript)仍然是唯一一个没有路由级回归测试的脱敏调用点——失败场景:server.test.ts 中的脱敏测试只覆盖 /session/:id/transcript/load/branch/side-task 与 SSE 流;multi-workspace-sessions.test.ts(唯一触达该路由的文件)既无 available_commands_update 记录也无 availableSkillDetails,因此移除这里的 omitSkillDetailsForSdkSurface 包裹会让浏览器面的工作区 transcript 响应重新携带完整 SKILL.md 正文,而全部测试仍然通过。已在受审 commit 上做突变验证:移除该包裹后四个相关套件共 1071 个测试全部通过。该包裹目前属于纵深防御(持久化回放从不下发 available_commands_update),所以这是覆盖缺口,而非活动中的泄漏。建议修复:新增一个 workspace-transcript 测试,持久化一条携带 availableSkillDetailsavailable_commands_update 记录,并断言响应事件中不含该键(外加正文金丝雀),与 /session/:id/transcript 的姊妹测试保持同构。

证据(在 48a0760ab4 上执行的突变探针):移除 session.ts:2974-2975 处的包裹后,四个套件 1071 通过、0 失败。

— qwen3.8-max via Qwen Code /review (v0.21.12)

v: 1 as const,
Comment on lines +2974 to +2976

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R5-2: The workspace-qualified transcript route (GET /workspaces/:workspace/session/:id/transcript, registered at session.ts:2863) is the only redaction call site this diff adds with no route-level regression test — every other redacted egress this PR touches (virtual-subagent load, load/resume, branch, side-task, primary transcript, SSE strip/empty-_meta drop, /acp retention) has one, and this route builds its frames differently (via SessionTranscriptReader + replayTranscriptRecordPage, wrapping each update inline before redacting). — Concrete cost: a future refactor of this route (extracting a shared handler with the primary transcript route, or rebuilding the events array) that drops or mis-scopes the omitSkillDetailsForSdkSurface wrap ships with every test green, and workspace-scoped transcript responses would again embed full SKILL.md bodies (~600 KB per snapshot in this PR's own fixtures — the #9234 freeze payload) the moment replayed history carries a command snapshot. Witness (mutation probe at the reviewed commit):

MUTATED (wrap removed): multi-workspace-sessions.test.ts → Tests 101 passed (101)
probe on MUTATED tree:  AssertionError: expected { availableSkills: ['bugfix'], …(1) } to not have property "availableSkillDetails"
probe on PR tree:       Tests 1 passed (1)

(The leak is not reachable today — the replay machinery feeding this route currently emits only message/tool/plan updates, never available_commands_update — so this is defense-in-depth consistent with its siblings, not a live bug.) Suggested fix: add a mirror of redacts skill bodies from flat transcript events (#9234) aimed at this route — seed a persisted available_commands_update record carrying _meta.availableSkillDetails, and assert the response keeps availableSkills, lacks availableSkillDetails, and a body canary string is absent.

中文说明

[Suggestion] 工作区限定的 transcript 路由(GET /workspaces/:workspace/session/:id/transcript,注册于 session.ts:2863)是本 diff 新增的脱敏调用点中唯一没有路由级回归测试的一个——本 PR 触及的其他所有脱敏出口(虚拟 subagent load、load/resume、branch、side-task、主 transcript、SSE 剥离/空 _meta 丢弃、/acp 保留)都有对应测试,且该路由构造帧的方式不同(经 SessionTranscriptReader + replayTranscriptRecordPage,在脱敏前将每个 update 内联包装)。具体代价:未来对该路由的重构(与主 transcript 路由抽取共享处理器、或重建 events 数组)若丢掉或错置 omitSkillDetailsForSdkSurface 包装,将在所有测试保持绿色的情况下上线——一旦回放历史携带命令快照,工作区限定 transcript 响应会重新嵌入完整 SKILL.md 正文(按本 PR 自身夹具约每快照 600 KB——即 #9234 的卡死负载)。证据(在受审 commit 上的变异探针):移除包装后 multi-workspace-sessions.test.ts 仍 101/101 全绿;金丝雀探针测试在变异树上失败、在 PR 树上通过。(该泄漏目前不可达——喂给该路由的回放机制当前只产生 message/tool/plan 更新,从不产生 available_commands_update——因此这是与其兄弟路由一致的纵深防御,而非现行缺陷。)建议修复:仿照 redacts skill bodies from flat transcript events (#9234) 为该路由新增镜像测试——注入一条携带 _meta.availableSkillDetails 的持久化 available_commands_update 记录,断言响应保留 availableSkills、不含 availableSkillDetails、且正文金丝雀字符串缺失。

— qwen3.8-max via Qwen Code /review (v0.21.12)

type: 'session_update' as const,
data: update,
}),
),
...(replay.nextCursor && !cursorTooLarge
? { nextCursor: replay.nextCursor }
: {}),
Expand Down
10 changes: 6 additions & 4 deletions packages/cli/src/serve/routes/sse-events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -125,6 +126,7 @@ interface RegisterSseEventsRoutesDeps {
type OmitId<T> = Omit<T, 'id'>;

function formatSseFrame(event: BridgeEvent | OmitId<BridgeEvent>): 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
Expand All @@ -142,21 +144,21 @@ function formatSseFrame(event: BridgeEvent | OmitId<BridgeEvent>): 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<string, unknown> })._meta;
const existingMeta = (shaped as { _meta?: Record<string, unknown> })._meta;
const existingServerTimestamp = existingMeta?.['serverTimestamp'];
const serverTimestamp =
typeof existingServerTimestamp === 'number' &&
Number.isFinite(existingServerTimestamp)
? 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(
Expand Down
Loading
Loading