From 8718f5bdd2eaf307e839969dbc906eeb1c4dce32 Mon Sep 17 00:00:00 2001 From: liuzhaochen03 Date: Fri, 3 Jul 2026 10:50:40 +0800 Subject: [PATCH 1/9] feat: mutlti root workspace support --- .changeset/support-acp-multi-root.md | 5 +++ packages/acp-adapter/src/server.ts | 6 +++ .../acp-adapter/test/e2e-happy-path.test.ts | 1 + packages/acp-adapter/test/server.test.ts | 1 + .../acp-adapter/test/session-load.test.ts | 28 +++++++++++++- packages/acp-adapter/test/session-new.test.ts | 37 +++++++++++++++++-- .../acp-adapter/test/session-resume.test.ts | 28 +++++++++++++- 7 files changed, 101 insertions(+), 5 deletions(-) create mode 100644 .changeset/support-acp-multi-root.md diff --git a/.changeset/support-acp-multi-root.md b/.changeset/support-acp-multi-root.md new file mode 100644 index 000000000..9b06d3856 --- /dev/null +++ b/.changeset/support-acp-multi-root.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Support ACP multi-root workspaces when creating, loading, and resuming sessions. diff --git a/packages/acp-adapter/src/server.ts b/packages/acp-adapter/src/server.ts index 607e7ef6e..f69ae23fe 100644 --- a/packages/acp-adapter/src/server.ts +++ b/packages/acp-adapter/src/server.ts @@ -227,6 +227,7 @@ export class AcpServer implements Agent { sse: true, }, sessionCapabilities: { + additionalDirectories: {}, list: {}, resume: {}, }, @@ -286,6 +287,7 @@ export class AcpServer implements Agent { const session = await this.harness.createSession({ id: sessionId, workDir: params.cwd, + additionalDirs: params.additionalDirectories, kaos: acpKaos, persistenceKaos, sessionStartedProperties: { mode: 'new' }, @@ -358,6 +360,7 @@ export class AcpServer implements Agent { const { session, acpSession, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, + additionalDirectories: params.additionalDirectories, mcpServers: params.mcpServers, mode: 'load', }); @@ -394,6 +397,7 @@ export class AcpServer implements Agent { const { session, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, + additionalDirectories: params.additionalDirectories, mcpServers: params.mcpServers, mode: 'resume', }); @@ -427,6 +431,7 @@ export class AcpServer implements Agent { private async setupSessionFromExisting(params: { cwd: string; sessionId: string; + additionalDirectories?: readonly string[]; mcpServers?: ReadonlyArray; mode: 'load' | 'resume'; }): Promise<{ @@ -456,6 +461,7 @@ export class AcpServer implements Agent { try { session = await this.harness.resumeSession({ id: params.sessionId, + additionalDirs: params.additionalDirectories, kaos: acpKaos, persistenceKaos, sessionStartedProperties: { mode: params.mode }, diff --git a/packages/acp-adapter/test/e2e-happy-path.test.ts b/packages/acp-adapter/test/e2e-happy-path.test.ts index 8ee7c56da..f62641eda 100644 --- a/packages/acp-adapter/test/e2e-happy-path.test.ts +++ b/packages/acp-adapter/test/e2e-happy-path.test.ts @@ -174,6 +174,7 @@ describe('AcpServer end-to-end happy path', () => { sse: true, }, sessionCapabilities: { + additionalDirectories: {}, list: {}, resume: {}, }, diff --git a/packages/acp-adapter/test/server.test.ts b/packages/acp-adapter/test/server.test.ts index 883fae3d3..d7e6e527f 100644 --- a/packages/acp-adapter/test/server.test.ts +++ b/packages/acp-adapter/test/server.test.ts @@ -79,6 +79,7 @@ describe('AcpServer + AgentSideConnection', () => { expect(response.agentCapabilities?.promptCapabilities?.embeddedContext).toBe(true); expect(response.agentCapabilities?.mcpCapabilities?.http).toBe(true); expect(response.agentCapabilities?.mcpCapabilities?.sse).toBe(true); + expect(response.agentCapabilities?.sessionCapabilities?.additionalDirectories).toEqual({}); expect(response.agentCapabilities?.sessionCapabilities?.list).toEqual({}); expect(response.agentCapabilities?.sessionCapabilities?.resume).toEqual({}); }); diff --git a/packages/acp-adapter/test/session-load.test.ts b/packages/acp-adapter/test/session-load.test.ts index fb3e62807..18dc4710a 100644 --- a/packages/acp-adapter/test/session-load.test.ts +++ b/packages/acp-adapter/test/session-load.test.ts @@ -82,6 +82,7 @@ function makeSessionWithHistory( function makeHarness( opts: { + capturedResumeInputs?: Array<{ additionalDirs?: readonly string[]; id: string }>; hasUsableToken?: boolean; session?: Session; resumeError?: Error; @@ -92,7 +93,8 @@ function makeHarness( auth: { status: async () => (authed ? AUTHED_STATUS : UNAUTHED_STATUS), }, - resumeSession: async (_input: { id: string }) => { + resumeSession: async (input: { additionalDirs?: readonly string[]; id: string }) => { + opts.capturedResumeInputs?.push(input); if (opts.resumeError) throw opts.resumeError; if (!opts.session) throw new Error('test harness has no session configured'); return opts.session; @@ -129,6 +131,30 @@ describe('AcpServer session/load auth gate', () => { }); describe('AcpServer session/load replay', () => { + it('passes ACP additionalDirectories through as SDK additionalDirs', async () => { + const sessionId = 'sess-load-multi-root'; + const session = makeSessionWithHistory(sessionId, []); + const capturedResumeInputs: Array<{ additionalDirs?: readonly string[]; id: string }> = []; + const harness = makeHarness({ hasUsableToken: true, session, capturedResumeInputs }); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const clientConn = new ClientSideConnection((_a) => new CapturingClient(), clientStream); + + await clientConn.loadSession({ + sessionId, + cwd: '/tmp/x', + additionalDirectories: ['/tmp/docs', '/tmp/plugin'], + mcpServers: [], + }); + + expect(capturedResumeInputs).toHaveLength(1); + expect(capturedResumeInputs[0]).toMatchObject({ + id: sessionId, + additionalDirs: ['/tmp/docs', '/tmp/plugin'], + }); + }); + it('replays a single assistant text-only turn as agent_message_chunk updates', async () => { const sessionId = 'sess-text-only'; const history = [ diff --git a/packages/acp-adapter/test/session-new.test.ts b/packages/acp-adapter/test/session-new.test.ts index ece351999..a82776e8f 100644 --- a/packages/acp-adapter/test/session-new.test.ts +++ b/packages/acp-adapter/test/session-new.test.ts @@ -46,7 +46,12 @@ function makeInMemoryStreamPair(): { } interface CapturedCall { - options: { id?: string; workDir: string; mcpServers?: Record }; + options: { + additionalDirs?: readonly string[]; + id?: string; + mcpServers?: Record; + workDir: string; + }; } function makeHarness(sessionId: string, captured: CapturedCall[]): { @@ -61,7 +66,11 @@ function makeHarness(sessionId: string, captured: CapturedCall[]): { } as unknown as Session; const harness = { auth: { status: async () => AUTHED_STATUS }, - createSession: async (options: { id?: string; workDir: string }) => { + createSession: async (options: { + additionalDirs?: readonly string[]; + id?: string; + workDir: string; + }) => { captured.push({ options }); return Object.assign({}, fakeSession, { id: options.id ?? sessionId }) as Session; }, @@ -114,7 +123,11 @@ describe('AcpServer session/new', () => { const captured: CapturedCall[] = []; const harness = { auth: { status: async () => AUTHED_STATUS }, - createSession: async (options: { id?: string; workDir: string }) => { + createSession: async (options: { + additionalDirs?: readonly string[]; + id?: string; + workDir: string; + }) => { captured.push({ options }); return { id: options.id ?? 'fallback', @@ -144,6 +157,24 @@ describe('AcpServer session/new', () => { expect(captured[1]?.options.id).toBe(second.sessionId); }); + it('passes ACP additionalDirectories through as SDK additionalDirs', async () => { + const captured: CapturedCall[] = []; + const { harness } = makeHarness('sess-multi-root', captured); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await client.newSession({ + cwd: '/tmp/work', + additionalDirectories: ['/tmp/docs', '/tmp/plugin'], + mcpServers: [], + }); + + expect(captured).toHaveLength(1); + expect(captured[0]?.options.additionalDirs).toEqual(['/tmp/docs', '/tmp/plugin']); + }); + it('advertises configOptions (PLAN D11 + Phase 15 thinking toggle) — model + thinking + mode under the unified SessionConfigOption surface', async () => { const captured: CapturedCall[] = []; const { harness } = makeHarness('sess-modes', captured); diff --git a/packages/acp-adapter/test/session-resume.test.ts b/packages/acp-adapter/test/session-resume.test.ts index 34b92783a..3d0a70194 100644 --- a/packages/acp-adapter/test/session-resume.test.ts +++ b/packages/acp-adapter/test/session-resume.test.ts @@ -98,6 +98,7 @@ function makeSessionWithMainConfig( } function makeHarness(opts: { + capturedResumeInputs?: Array<{ additionalDirs?: readonly string[]; id: string }>; hasUsableToken?: boolean; session?: Session; resumeError?: Error; @@ -107,7 +108,8 @@ function makeHarness(opts: { auth: { status: async () => (authed ? AUTHED_STATUS : UNAUTHED_STATUS), }, - resumeSession: async (_input: { id: string }) => { + resumeSession: async (input: { additionalDirs?: readonly string[]; id: string }) => { + opts.capturedResumeInputs?.push(input); if (opts.resumeError) throw opts.resumeError; if (!opts.session) throw new Error('test harness has no session configured'); return opts.session; @@ -139,6 +141,30 @@ describe('AcpServer.resumeSession', () => { ).rejects.toMatchObject({ code: -32000 }); }); + it('passes ACP additionalDirectories through as SDK additionalDirs', async () => { + const sessionId = 'sess-resume-multi-root'; + const session = makeSessionWithMainConfig(sessionId); + const capturedResumeInputs: Array<{ additionalDirs?: readonly string[]; id: string }> = []; + const harness = makeHarness({ hasUsableToken: true, session, capturedResumeInputs }); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const clientConn = new ClientSideConnection((_a) => new CapturingClient(), clientStream); + + await clientConn.resumeSession({ + sessionId, + cwd: '/tmp/x', + additionalDirectories: ['/tmp/docs', '/tmp/plugin'], + mcpServers: [], + }); + + expect(capturedResumeInputs).toHaveLength(1); + expect(capturedResumeInputs[0]).toMatchObject({ + id: sessionId, + additionalDirs: ['/tmp/docs', '/tmp/plugin'], + }); + }); + it('returns configOptions matching the resumed session model + mode + thinking', async () => { const sessionId = 'sess-resume-model'; // Resume state reports kimi-plain (thinking unsupported) so we can From 5b49c73cd73dd0dbcafb6a3debc8b0dcd91cb571 Mon Sep 17 00:00:00 2001 From: liuzhaochen03 Date: Fri, 3 Jul 2026 18:29:45 +0800 Subject: [PATCH 2/9] fix: active session resume when additionDirectories exist --- packages/node-sdk/src/kimi-harness.ts | 2 + .../test/create-session-transport.test.ts | 52 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/packages/node-sdk/src/kimi-harness.ts b/packages/node-sdk/src/kimi-harness.ts index d503143ea..328b302a9 100644 --- a/packages/node-sdk/src/kimi-harness.ts +++ b/packages/node-sdk/src/kimi-harness.ts @@ -120,6 +120,8 @@ export class KimiHarness { if (active !== undefined) { if (kaos !== undefined || persistenceKaos !== undefined) { await this.rpc.resumeSessionWithKaos({ ...resumeInput, id }, kaos ?? persistenceKaos as Kaos, persistenceKaos); + } else if (resumeInput.additionalDirs !== undefined) { + await this.rpc.resumeSession({ ...resumeInput, id }); } return active; } diff --git a/packages/node-sdk/test/create-session-transport.test.ts b/packages/node-sdk/test/create-session-transport.test.ts index 5bd3d4a64..3be850c25 100644 --- a/packages/node-sdk/test/create-session-transport.test.ts +++ b/packages/node-sdk/test/create-session-transport.test.ts @@ -52,6 +52,7 @@ max_context_size = 1000 } class StubRpc extends SDKRpcClientBase { + plainResumeCalls: ResumeSessionInput[] = []; resumeCalls: Array<{ input: ResumeSessionInput; kaos: Kaos; persistenceKaos?: Kaos }> = []; protected async getRpc(): Promise { @@ -68,6 +69,27 @@ class StubRpc extends SDKRpcClientBase { }; } + override async resumeSession(input: ResumeSessionInput): Promise { + this.plainResumeCalls.push(input); + return { + id: input.id, + workDir: '/tmp/work', + sessionDir: '/tmp/session', + createdAt: 1, + updatedAt: 1, + additionalDirs: input.additionalDirs ?? [], + sessionMetadata: { + createdAt: '', + updatedAt: '', + title: '', + isCustomTitle: false, + agents: {}, + custom: {}, + }, + agents: {}, + }; + } + override async resumeSessionWithKaos(input: ResumeSessionInput, kaos: Kaos, persistenceKaos?: Kaos): Promise { this.resumeCalls.push({ input, kaos, persistenceKaos }); return { @@ -76,6 +98,7 @@ class StubRpc extends SDKRpcClientBase { sessionDir: '/tmp/session', createdAt: 1, updatedAt: 1, + additionalDirs: input.additionalDirs ?? [], sessionMetadata: { createdAt: '', updatedAt: '', @@ -694,6 +717,35 @@ effort = "medium" persistenceKaos: undefined, }); }); + + it('refreshes an active session when resumeSession receives additionalDirs without a new Kaos', async () => { + const records: TelemetryRecord[] = []; + const rpc = new StubRpc(); + const harness = new KimiHarness(rpc, { + homeDir: '/tmp/home', + configPath: '/tmp/config.toml', + auth: { status: async () => ({ providers: [] }) } as never, + telemetry: recordingTelemetry(records), + ensureConfigFile: async () => undefined, + onClose: () => undefined, + }); + + const session = await harness.createSession({ id: 'ses_active_dirs', workDir: '/tmp/work' }); + + const resumed = await harness.resumeSession({ + id: session.id, + additionalDirs: ['/tmp/extra'], + }); + + expect(resumed).toBe(session); + expect(rpc.plainResumeCalls).toEqual([ + { + id: 'ses_active_dirs', + additionalDirs: ['/tmp/extra'], + }, + ]); + expect(rpc.resumeCalls).toHaveLength(0); + }); }); function coreSessionIds(harness: KimiHarness): readonly string[] { From 1437991ae20c395071a65d93de6b39a3c663b0d4 Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 3 Jul 2026 18:08:14 +0800 Subject: [PATCH 3/9] feat(acp-adapter): enforce path boundary for additionalDirectories - Add path-boundary module with resolveCanonicalPath, resolveCanonicalRoots and assertPathInRoots primitives - Inject effectiveRoots into AcpKaos to validate every file operation against canonical workspace roots - Add validateAdditionalDirectories to reject malformed additionalDirectories at session-init time - Wire canonical roots through newSession/loadSession/resumeSession lifecycle --- packages/acp-adapter/src/kaos-acp.ts | 64 ++++- packages/acp-adapter/src/path-boundary.ts | 150 +++++++++++ packages/acp-adapter/src/server.ts | 100 +++++++- .../acp-adapter/test/boundary-session.test.ts | 154 ++++++++++++ packages/acp-adapter/test/e2e-fs.test.ts | 20 +- packages/acp-adapter/test/kaos-acp.test.ts | 64 ++--- .../acp-adapter/test/kaos-activation.test.ts | 29 ++- .../acp-adapter/test/kaos-boundary.test.ts | 232 ++++++++++++++++++ .../acp-adapter/test/path-boundary.test.ts | 187 ++++++++++++++ packages/acp-adapter/test/server.test.ts | 48 +++- .../acp-adapter/test/session-load.test.ts | 35 +++ packages/acp-adapter/test/session-new.test.ts | 102 ++++++++ .../acp-adapter/test/session-resume.test.ts | 34 +++ 13 files changed, 1156 insertions(+), 63 deletions(-) create mode 100644 packages/acp-adapter/src/path-boundary.ts create mode 100644 packages/acp-adapter/test/boundary-session.test.ts create mode 100644 packages/acp-adapter/test/kaos-boundary.test.ts create mode 100644 packages/acp-adapter/test/path-boundary.test.ts diff --git a/packages/acp-adapter/src/kaos-acp.ts b/packages/acp-adapter/src/kaos-acp.ts index 40f9759ef..5a9c9b2e3 100644 --- a/packages/acp-adapter/src/kaos-acp.ts +++ b/packages/acp-adapter/src/kaos-acp.ts @@ -28,6 +28,8 @@ import { type StatResult, } from '@moonshot-ai/kaos'; +import { assertPathInRoots } from './path-boundary'; + /** * `Kaos` that routes `read*` / `write*` through the ACP reverse-RPC * channel and delegates everything else to `inner`. @@ -44,6 +46,19 @@ export class AcpKaos implements Kaos { private readonly conn: AgentSideConnection, private readonly sessionId: string, private readonly inner: Kaos, + /** + * Canonical (realpath-resolved) workspace roots this AcpKaos + * treats as authoritative: `[cwd, ...additionalDirectories]`. + * Set at construction time so the per-call hot path only + * resolves the target, not the roots. Must be the output of + * {@link resolveCanonicalRoots} — caller is responsible for + * realpath'ing the supplied cwd + additionalDirectories up front + * (see `maybeBuildAcpKaos` in `server.ts`). + * + * Carried unchanged through {@link withCwd} / {@link withEnv} — + * the effective root set is session-scoped, not cwd-scoped. + */ + private readonly effectiveRoots: readonly string[], ) {} // ── identity ──────────────────────────────────────────────────────── @@ -84,33 +99,47 @@ export class AcpKaos implements Kaos { * instance — so a `chdir` followed by `readText('relative.ts')` * continues to hit the ACP bridge rather than silently dropping back * to local filesystem reads. + * + * The effective root set (this adapter's `additionalDirectories` + * boundary) is **session-scoped**, not cwd-scoped, so it carries + * forward unchanged — changing `inner.withCwd()` does not retune + * which paths are allowed. */ withCwd(cwd: string): Kaos { - return new AcpKaos(this.conn, this.sessionId, this.inner.withCwd(cwd)); + return new AcpKaos(this.conn, this.sessionId, this.inner.withCwd(cwd), this.effectiveRoots); } withEnv(env: Record): Kaos { - return new AcpKaos(this.conn, this.sessionId, this.inner.withEnv(env)); + return new AcpKaos(this.conn, this.sessionId, this.inner.withEnv(env), this.effectiveRoots); } - stat(path: string, options?: { followSymlinks?: boolean }): Promise { + async stat(path: string, options?: { followSymlinks?: boolean }): Promise { + await assertPathInRoots(path, this.effectiveRoots, 'stat'); return this.inner.stat(path, options); } - iterdir(path: string): AsyncGenerator { - return this.inner.iterdir(path); + async *iterdir(path: string): AsyncGenerator { + // Anchor the check before yielding so callers that never consume + // the iterator still see the boundary violation. + await assertPathInRoots(path, this.effectiveRoots, 'iterdir'); + yield* this.inner.iterdir(path); } - glob( + async *glob( path: string, pattern: string, options?: { caseSensitive?: boolean }, ): AsyncGenerator { - return this.inner.glob(path, pattern, options); + await assertPathInRoots(path, this.effectiveRoots, 'glob'); + yield* this.inner.glob(path, pattern, options); } - mkdir(path: string, options?: { parents?: boolean; existOk?: boolean }): Promise { - return this.inner.mkdir(path, options); + async mkdir( + path: string, + options?: { parents?: boolean; existOk?: boolean }, + ): Promise { + await assertPathInRoots(path, this.effectiveRoots, 'mkdir'); + await this.inner.mkdir(path, options); } // ── reads: route through ACP `fs/readTextFile` ───────────────────── @@ -126,6 +155,7 @@ export class AcpKaos implements Kaos { path: string, _options?: { encoding?: BufferEncoding; errors?: 'strict' | 'replace' | 'ignore' }, ): Promise { + await assertPathInRoots(path, this.effectiveRoots, 'readText'); const rpcPath = this.toClientPath(path); try { const resp = await this.conn.readTextFile({ sessionId: this.sessionId, path: rpcPath }); @@ -141,8 +171,14 @@ export class AcpKaos implements Kaos { * payloads (images, video, archives — anything `ReadMediaFile` may * touch). The ACP bridge only owns the *text* surface; raw bytes * stay on the local filesystem via `inner`. + * + * Even though the bytes go to `inner`, the path is still subject + * to the effective-root check — without that, a model could read + * `/etc/passwd` bytes by routing through the binary surface. The + * `additionalDirectories` boundary is about scope, not surface. */ - readBytes(path: string, n?: number): Promise { + async readBytes(path: string, n?: number): Promise { + await assertPathInRoots(path, this.effectiveRoots, 'readBytes'); return this.inner.readBytes(path, n); } @@ -200,6 +236,13 @@ export class AcpKaos implements Kaos { data: string, options?: { mode?: 'w' | 'a'; encoding?: BufferEncoding }, ): Promise { + // Single boundary check for the outer path, regardless of mode. + // The append-mode's read-then-write fallback re-checks the same + // target via readText, which would otherwise double-resolve and + // race the same canonical path twice. The outer check is what + // gates the write; the inner read is a fallback for "file does + // not exist yet", not a separate authorization surface. + await assertPathInRoots(path, this.effectiveRoots, 'writeText'); if (options?.mode === 'a') { let existing = ''; try { @@ -221,6 +264,7 @@ export class AcpKaos implements Kaos { * (Read/Write/Edit tools), not binary streaming. */ async writeBytes(path: string, data: Buffer): Promise { + await assertPathInRoots(path, this.effectiveRoots, 'writeBytes'); await this.acpWrite(path, data.toString('utf8')); return data.byteLength; } diff --git a/packages/acp-adapter/src/path-boundary.ts b/packages/acp-adapter/src/path-boundary.ts new file mode 100644 index 000000000..bce512322 --- /dev/null +++ b/packages/acp-adapter/src/path-boundary.ts @@ -0,0 +1,150 @@ +/** + * Path-boundary primitives for the ACP `additionalDirectories` feature. + * + * The boundary check is the security-bearing layer behind + * `AcpKaos.readText` / `writeText` / etc. — every file operation goes + * through `assertPathInRoots` BEFORE the ACP reverse-RPC call, so a + * model-side `Read /etc/passwd` cannot bypass the client's trust + * boundary by hopping through the kernel's local filesystem. + * + * Three primitives: + * + * {@link resolveCanonicalPath} + * Symlink-resolving canonicalisation for a target path. Handles + * the "path doesn't yet exist" case (writes to new files) by + * anchoring at the deepest existing ancestor. + * + * {@link resolveCanonicalRoots} + * Realpath every supplied root at session-init time. Missing + * roots cause `KaosError` — this is the spec's fail-closed-at- + * creation guarantee. + * + * {@link assertPathInRoots} + * Pre-flight check: target must canonicalise to a location + * inside one of the canonicalised roots, OR be exactly a root. + * Failure raises `KaosError` carrying the operation name. + * + * All primitives use `fs.realpath` so symlinks/junctions/mount points + * are resolved before the prefix comparison — the prefix check would + * otherwise miss escapes planted under a parent symlink. + */ + +import { promises as fsp } from 'node:fs'; +import path from 'node:path'; + +import { KaosError } from '@moonshot-ai/kaos'; + +/** + * Resolve `p` to its canonical, symlink-resolved form. + * + * - If `p` exists, returns `fs.realpath(p)`. + * - If `p` (or any parent) doesn't exist, walks up until it finds an + * existing ancestor, realpaths that, and reappends the suffix. The + * realpath'd ancestor is the security anchor: any symlink planted + * between it and `p` is invisible to us, but the prefix check in + * {@link assertPathInRoots} still uses the canonical ancestor's + * prefix — and the canonical ancestor IS inside-or-outside the + * root set, which is what we care about. + * + * Throws {@link KaosError} if the walk exhausts before hitting any + * realpath-able anchor (e.g. a path whose root segment is missing). + */ +export async function resolveCanonicalPath(p: string): Promise { + try { + return await fsp.realpath(p); + } catch (firstErr) { + let cursor = path.dirname(p); + let suffix = path.basename(p); + let lastErr: unknown = firstErr; + while (true) { + try { + const realCursor = await fsp.realpath(cursor); + return path.join(realCursor, suffix); + } catch (e) { + lastErr = e; + const next = path.dirname(cursor); + if (next === cursor) break; + suffix = path.join(path.basename(cursor), suffix); + cursor = next; + } + } + throw new KaosError( + `path-boundary: no canonical ancestor for ${p}: ${describeErr(lastErr)}`, + ); + } +} + +/** + * Canonicalise every root up front. Missing roots fail-closed. + * + * Called once per session inside `maybeBuildAcpKaos`; passing the + * result into {@link AcpKaos}'s constructor means every subsequent + * `assertPathInRoots` call only handles the per-target resolve, not + * the root resolve. That keeps the per-call hot path to a single + * `realpath(target)` + O(N) prefix loop, instead of N `realpath` + * per call. + */ +export async function resolveCanonicalRoots( + roots: readonly string[], +): Promise { + const out: string[] = []; + for (const r of roots) { + try { + out.push(await fsp.realpath(r)); + } catch (err) { + throw new KaosError( + `path-boundary: cannot resolve root ${r}: ${describeErr(err)}`, + ); + } + } + return out; +} + +/** + * Throw {@link KaosError} if `target`, after canonicalisation, is not + * inside any of the canonical `roots`. + * + * Pass `roots` as the OUTPUT of {@link resolveCanonicalRoots} — + * passing raw root paths would silently miss escapes if a root + * itself is a symlink. + * + * The `operation` string is included in the error message so logs + * can attribute the failure to a read vs write vs stat without + * separate bookkeeping. + */ +export async function assertPathInRoots( + target: string, + roots: readonly string[], + operation: string, +): Promise { + if (roots.length === 0) { + // No roots allowed → fail closed. Spec: "If you cannot safely + // determine whether the path is inside allowed roots, fail + // closed." Equally, if the boundary literally is empty, every + // path is outside. + throw new KaosError( + `path-boundary: ${operation} refused for ${target}: no workspace roots configured`, + ); + } + const realTarget = await resolveCanonicalPath(target); + for (const root of roots) { + if (realTarget === root) return; + // Preserve trailing separator on root (e.g. '/' on POSIX) so the + // prefix check works for the filesystem root itself, where + // `root + path.sep` would expand to '//' and never match. + const prefix = root.endsWith(path.sep) ? root : root + path.sep; + if (realTarget.startsWith(prefix)) return; + } + throw new KaosError( + `path-boundary: ${operation} refused for ${target} — outside workspace roots`, + ); +} + +function describeErr(err: unknown): string { + if (err && typeof err === 'object' && 'code' in err) { + const code = String((err as { code: unknown }).code); + const msg = err instanceof Error ? err.message : typeof err === 'string' ? err : 'Unknown error'; + return `${code}: ${msg}`; + } + return err instanceof Error ? err.message : typeof err === 'string' ? err : 'Unknown error'; +} diff --git a/packages/acp-adapter/src/server.ts b/packages/acp-adapter/src/server.ts index cbbe1758a..334c68575 100644 --- a/packages/acp-adapter/src/server.ts +++ b/packages/acp-adapter/src/server.ts @@ -8,6 +8,7 @@ import { Readable, Writable } from 'node:stream'; import { randomUUID } from 'node:crypto'; +import path from 'node:path'; import { AgentSideConnection, @@ -51,6 +52,7 @@ import { LocalKaos, type Kaos } from '@moonshot-ai/kaos'; import { TERMINAL_AUTH_METHOD, buildTerminalAuthMethod } from './auth-methods'; import { redirectConsoleToStderr } from './log-guard'; import { AcpKaos } from './kaos-acp'; +import { resolveCanonicalRoots } from './path-boundary'; import { AcpSession, type TelemetryTrackFn } from './session'; import { buildSessionConfigOptions } from './config-options'; import { availableCommandsUpdateNotification } from './events-map'; @@ -282,14 +284,15 @@ export class AcpServer implements Agent { // by the kernel `SessionImpl` ctor and every tool downstream sees // the same reference, no AsyncLocalStorage needed. const sessionId = `session_${randomUUID()}`; - const acpKaos = await this.maybeBuildAcpKaos(sessionId); + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); + const acpKaos = await this.maybeBuildAcpKaos(sessionId, params.cwd, additionalDirs); const persistenceKaos = acpKaos === undefined ? undefined : await this.ensureInnerKaos(); const session = await this.harness.createSession({ id: sessionId, workDir: params.cwd, - additionalDirs: params.additionalDirectories, kaos: acpKaos, persistenceKaos, + additionalDirs, sessionStartedProperties: { mode: 'new' }, // @ts-expect-error — `mcpServers` is a kernel-side extension // (agent-core `CreateSessionPayload`) the SDK transparently @@ -357,11 +360,12 @@ export class AcpServer implements Agent { * deliberately skips it (per ACP spec G4 / plan gap-4.3). */ async loadSession(params: LoadSessionRequest): Promise { + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); const { session, acpSession, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, - additionalDirectories: params.additionalDirectories, mcpServers: params.mcpServers, + additionalDirs, mode: 'load', }); // Synchronously replay history — the response must not settle @@ -394,11 +398,12 @@ export class AcpServer implements Agent { * rationale, and gap-4.1 for the matching capability advertisement. */ async resumeSession(params: ResumeSessionRequest): Promise { + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); const { session, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, - additionalDirectories: params.additionalDirectories, mcpServers: params.mcpServers, + additionalDirs, mode: 'resume', }); this.scheduleAvailableCommandsUpdate(session.id); @@ -431,8 +436,8 @@ export class AcpServer implements Agent { private async setupSessionFromExisting(params: { cwd: string; sessionId: string; - additionalDirectories?: readonly string[]; mcpServers?: ReadonlyArray; + additionalDirs?: readonly string[]; mode: 'load' | 'resume'; }): Promise<{ session: Session; @@ -455,15 +460,19 @@ export class AcpServer implements Agent { // `resumeSession` spreads `input` so unknown fields ride to the // kernel. const mcpServers = acpMcpServersToConfigs(params.mcpServers); - const acpKaos = await this.maybeBuildAcpKaos(params.sessionId); + const acpKaos = await this.maybeBuildAcpKaos( + params.sessionId, + params.cwd, + params.additionalDirs, + ); const persistenceKaos = acpKaos === undefined ? undefined : await this.ensureInnerKaos(); let session: Session; try { session = await this.harness.resumeSession({ id: params.sessionId, - additionalDirs: params.additionalDirectories, kaos: acpKaos, persistenceKaos, + additionalDirs: params.additionalDirs, sessionStartedProperties: { mode: params.mode }, // @ts-expect-error — see block comment above; mcpServers is a // kernel-only field that the SDK forwards via spread. @@ -537,8 +546,24 @@ export class AcpServer implements Agent { * it. The resulting {@link AcpKaos} is captured by the kernel * `SessionImpl` ctor and every tool downstream sees the same * reference — no AsyncLocalStorage involved. + * + * @param sessionId ACP-side session id (also used as the + * reverse-RPC session binding). + * @param additionalDirs Optional additional workspace roots from + * `additionalDirectories`. Combined with + * `cwd` (supplied by the caller) to form + * the effective root set the {@link AcpKaos} + * enforces on every file operation. + * Missing / non-existent roots cause a + * structured `invalid_params` rejection — + * fail-closed at session-init time per the + * ACP `additionalDirectories` RFD. */ - private async maybeBuildAcpKaos(sessionId: string): Promise { + private async maybeBuildAcpKaos( + sessionId: string, + cwd: string, + additionalDirs?: readonly string[], + ): Promise { const fs = this.clientCapabilities?.fs; if (!fs?.readTextFile && !fs?.writeTextFile) { return undefined; @@ -547,7 +572,22 @@ export class AcpServer implements Agent { return undefined; } const innerKaos = await this.ensureInnerKaos(); - return new AcpKaos(this.conn, sessionId, innerKaos); + const roots = [cwd, ...(additionalDirs ?? [])]; + let canonical: string[]; + try { + canonical = await resolveCanonicalRoots(roots); + } catch (err) { + // Fail-closed: a missing/unresolved root is a misconfigured + // session, not a transient condition. Surface it as a + // structured JSON-RPC `invalid_params` so the client sees a + // clear failure rather than a tool-time boundary error many + // turns later. + throw RequestError.invalidParams( + { additionalDirectories: additionalDirs ?? [] }, + `cannot resolve additionalDirectories on disk: ${err instanceof Error ? err.message : String(err)}`, + ); + } + return new AcpKaos(this.conn, sessionId, innerKaos, canonical); } private async ensureInnerKaos(): Promise { @@ -1103,3 +1143,45 @@ function sessionSummaryToSessionInfo(summary: SessionSummary): SessionInfo { updatedAt, }; } + +/** + * Validate the ACP `additionalDirectories` field per protocol spec. + * + * Returns the validated string array when the field is present, or + * `undefined` when the field is absent (`null` or `undefined`). + * Throws {@link RequestError.invalidParams} for non-array values, + * non-string entries, empty strings, or non-absolute paths. + */ +export function validateAdditionalDirectories( + dirs: unknown, +): string[] | undefined { + if (dirs === undefined || dirs === null) return undefined; + if (!Array.isArray(dirs)) { + throw RequestError.invalidParams( + { additionalDirectories: dirs }, + 'additionalDirectories must be an array', + ); + } + for (let i = 0; i < dirs.length; i++) { + const entry = dirs[i]; + if (typeof entry !== 'string') { + throw RequestError.invalidParams( + { additionalDirectories: dirs }, + `additionalDirectories[${i}] must be a string`, + ); + } + if (entry === '') { + throw RequestError.invalidParams( + { additionalDirectories: dirs }, + `additionalDirectories[${i}] must not be empty`, + ); + } + if (!path.isAbsolute(entry)) { + throw RequestError.invalidParams( + { additionalDirectories: dirs }, + `additionalDirectories[${i}] must be an absolute path`, + ); + } + } + return dirs; +} diff --git a/packages/acp-adapter/test/boundary-session.test.ts b/packages/acp-adapter/test/boundary-session.test.ts new file mode 100644 index 000000000..91a70b277 --- /dev/null +++ b/packages/acp-adapter/test/boundary-session.test.ts @@ -0,0 +1,154 @@ +/** + * Session-level boundary tests: an `additionalDirectories` entry that + * does not exist on disk must be rejected at `session/new` time so a + * misconfigured client can't push the failure into a tool call many + * turns later. The effective root set is resolved when the boundary- + * aware {@link AcpKaos} is built (`maybeBuildAcpKaos`); if any root's + * `realpath` fails we surface a structured `invalid_params` error + * before the harness sees the request. + * + * These tests cover the wiring, not the boundary logic itself (that + * lives in `path-boundary.test.ts` / `kaos-boundary.test.ts`). Each + * test advertises the FS reverse-RPC capability on `initialize` so + * `maybeBuildAcpKaos` actually constructs an `AcpKaos` and runs the + * root-set resolution; without the capability, the boundary code is + * never reached and the test would be testing nothing. + */ + +import { + AgentSideConnection, + ClientSideConnection, + ndJsonStream, + type Client, + type ReadTextFileRequest, + type ReadTextFileResponse, + type RequestPermissionRequest, + type RequestPermissionResponse, + type SessionNotification, + type WriteTextFileRequest, + type WriteTextFileResponse, +} from '@agentclientprotocol/sdk'; +import type { KimiHarness } from '@moonshot-ai/kimi-code-sdk'; +import { promises as fsp } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { AcpServer } from '../src/server'; +import { AUTHED_STATUS } from './_helpers/harness-stubs'; + +function makeInMemoryStreamPair(): { + agentStream: ReturnType; + clientStream: ReturnType; +} { + const clientToAgent = new TransformStream(); + const agentToClient = new TransformStream(); + const agentStream = ndJsonStream(agentToClient.writable, clientToAgent.readable); + const clientStream = ndJsonStream(clientToAgent.writable, agentToClient.readable); + return { agentStream, clientStream }; +} + +class StubClient implements Client { + async readTextFile(_p: ReadTextFileRequest): Promise { + throw new Error('not exercised'); + } + async writeTextFile(_p: WriteTextFileRequest): Promise { + throw new Error('not exercised'); + } + async sessionUpdate(_n: SessionNotification): Promise { + /* no-op */ + } + async requestPermission(_p: RequestPermissionRequest): Promise { + throw new Error('not exercised'); + } +} + +function makeHarness(): { + harness: KimiHarness; + calls: { newSession: number; resumeSession: number }; +} { + const calls = { newSession: 0, resumeSession: 0 }; + const harness = { + auth: { status: async () => AUTHED_STATUS }, + createSession: vi.fn(async (options: Record) => { + calls.newSession += 1; + return { + id: String((options as { id?: string }).id ?? 'sess-x'), + prompt: async () => undefined, + cancel: async () => undefined, + onEvent: () => () => undefined, + }; + }), + resumeSession: vi.fn(async (options: Record) => { + calls.resumeSession += 1; + return { + id: String((options as { id?: string }).id ?? 'sess-r'), + prompt: async () => undefined, + cancel: async () => undefined, + onEvent: () => () => undefined, + }; + }), + getConfig: async () => ({ providers: {}, models: {} }), + } as unknown as KimiHarness; + return { harness, calls }; +} + +let cwdOnDisk: string; + +beforeEach(async () => { + cwdOnDisk = await fsp.mkdtemp(path.join(tmpdir(), 'acp-sess-cwd-')); +}); + +afterEach(async () => { + await fsp.rm(cwdOnDisk, { recursive: true, force: true }); +}); + +describe('session/new — boundary', () => { + it('rejects when an additionalDirectories entry does not exist on disk', async () => { + const { harness, calls } = makeHarness(); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await client.initialize({ + protocolVersion: 1, + clientCapabilities: { fs: { readTextFile: true, writeTextFile: true } }, + }); + + await expect( + client.newSession({ + cwd: cwdOnDisk, + mcpServers: [], + additionalDirectories: [path.join(cwdOnDisk, 'definitely-not-here')], + }), + ).rejects.toMatchObject({ code: -32602 }); + + // Strong invariant: harness.createSession must NOT have been + // called when boundary validation fails — that's the whole point + // of validating upfront. + expect(calls.newSession).toBe(0); + }); + + it('accepts when every additionalDirectory entry exists on disk', async () => { + const extra = path.join(cwdOnDisk, 'shared'); + await fsp.mkdir(extra); + const { harness, calls } = makeHarness(); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await client.initialize({ + protocolVersion: 1, + clientCapabilities: { fs: { readTextFile: true, writeTextFile: true } }, + }); + + const response = await client.newSession({ + cwd: cwdOnDisk, + mcpServers: [], + additionalDirectories: [extra], + }); + + expect(response.sessionId).toBeDefined(); + expect(calls.newSession).toBe(1); + }); +}); diff --git a/packages/acp-adapter/test/e2e-fs.test.ts b/packages/acp-adapter/test/e2e-fs.test.ts index f2ef77c11..f365f8317 100644 --- a/packages/acp-adapter/test/e2e-fs.test.ts +++ b/packages/acp-adapter/test/e2e-fs.test.ts @@ -37,7 +37,9 @@ import { } from '@agentclientprotocol/sdk'; import type { Kaos } from '@moonshot-ai/kaos'; import type { Event, KimiHarness, Session } from '@moonshot-ai/kimi-code-sdk'; -import { describe, expect, it } from 'vitest'; +import { promises as fsp } from 'node:fs'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { AcpServer } from '../src/server'; import { AUTHED_STATUS } from './_helpers/harness-stubs'; @@ -125,8 +127,18 @@ function makeReadingSession( const textBlock = (text: string): ContentBlock => ({ type: 'text', text }); describe('end-to-end FS reverse-RPC', () => { + // The boundary check on `cwd` requires a realpath-able path; these + // e2e tests use mkdtemp'd scratch rather than the synthetic `/tmp/x` + // they used to carry. Set up once for the suite and tear it down. + let scratchWork: string; + beforeAll(async () => { + scratchWork = await fsp.mkdtemp(path.join('/tmp', 'acp-e2efs-')); + }); + afterAll(async () => { + if (scratchWork) await fsp.rm(scratchWork, { recursive: true, force: true }); + }); it('routes a tool-time readText through the client when fs.readTextFile is advertised', async () => { - const targetPath = '/Users/test/x.ts'; + const targetPath = path.join(scratchWork, 'x.ts'); let createdSession: Session | undefined; let capturedSessionId: string | undefined; const harness = { @@ -153,7 +165,7 @@ describe('end-to-end FS reverse-RPC', () => { }, }); - const newSession = await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const newSession = await client.newSession({ cwd: scratchWork, mcpServers: [] }); const response = await client.prompt({ sessionId: newSession.sessionId, @@ -238,7 +250,7 @@ describe('end-to-end FS reverse-RPC', () => { }, }); - const newSession = await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); + const newSession = await client.newSession({ cwd: scratchWork, mcpServers: [] }); const response = await client.prompt({ sessionId: newSession.sessionId, diff --git a/packages/acp-adapter/test/kaos-acp.test.ts b/packages/acp-adapter/test/kaos-acp.test.ts index 5f8d8e69c..43894cb6f 100644 --- a/packages/acp-adapter/test/kaos-acp.test.ts +++ b/packages/acp-adapter/test/kaos-acp.test.ts @@ -206,7 +206,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'HELLO' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); const result = await kaos.readText('/a.ts'); @@ -223,7 +223,7 @@ describe('AcpKaos', () => { throw rpcErr; }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); await expect(kaos.readText('/x.ts')).rejects.toMatchObject({ name: 'KaosError', @@ -233,10 +233,10 @@ describe('AcpKaos', () => { try { await kaos.readText('/x.ts'); throw new Error('should have thrown'); - } catch (err) { - expect((err as Error & { cause?: unknown }).cause).toBe(rpcErr); - expect((err as Error).message).toContain('acp: readTextFile failed for /x.ts'); - expect((err as Error).message).toContain('rpc died'); + } catch (error) { + expect((error as Error & { cause?: unknown }).cause).toBe(rpcErr); + expect((error as Error).message).toContain('acp: readTextFile failed for /x.ts'); + expect((error as Error).message).toContain('rpc died'); } }); @@ -245,7 +245,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'HELLO' }), }); const inner = makeMockInner({ pathClass: 'win32' }); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); await kaos.readText('G:/python-code/render_with_mult_gpu/README.md'); await kaos.writeText('G:/python-code/render_with_mult_gpu/README.md', 'updated'); @@ -274,7 +274,7 @@ describe('AcpKaos', () => { }, }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); const buf = await kaos.readBytes('/img.png', 4); expect(buf).toBeInstanceOf(Buffer); @@ -288,7 +288,7 @@ describe('AcpKaos', () => { it('forwards omitted n to inner unchanged', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); const buf = await kaos.readBytes('/img.png'); expect(buf.byteLength).toBe(8); @@ -305,20 +305,20 @@ describe('AcpKaos', () => { it('yields each line of "a\\nb\\nc" with terminators preserved', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb\nc' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b\n', 'c']); }); it('drops the trailing empty token when the file ends with a newline', async () => { // "a\nb\n" → ['a\n', 'b\n'] (NOT ['a\n', 'b\n', '']) const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb\n' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b\n']); }); it('yields the final line without a trailing newline when missing', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b']); }); @@ -326,13 +326,13 @@ describe('AcpKaos', () => { // ReadTool depends on this — stripping \n would expose bare \r and // render visible carriage returns. const conn = makeMockConn({ readHandler: async () => ({ content: 'a\r\nb\r\n' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\r\n', 'b\r\n']); }); it('yields nothing for an empty file', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: '' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); expect(await collect(kaos.readLines('/a.ts'))).toEqual([]); }); }); @@ -340,7 +340,7 @@ describe('AcpKaos', () => { describe('writeText', () => { it('forwards content to conn.writeTextFile and returns char count', async () => { const conn = makeMockConn({}); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); const n = await kaos.writeText('/a.ts', 'hello'); expect(n).toBe(5); expect(conn.writeCalls).toEqual([{ sessionId: 's1', path: '/a.ts', content: 'hello' }]); @@ -350,7 +350,7 @@ describe('AcpKaos', () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'old:' }), }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); const n = await kaos.writeText('/a.ts', 'new', { mode: 'a' }); // Return value is the size of the appended data, not the merged size. expect(n).toBe(3); @@ -367,7 +367,7 @@ describe('AcpKaos', () => { throw RequestError.resourceNotFound('/missing.ts'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); const n = await kaos.writeText('/missing.ts', 'fresh', { mode: 'a' }); expect(n).toBe(5); expect(conn.writeCalls).toEqual([ @@ -384,7 +384,7 @@ describe('AcpKaos', () => { throw new Error('permission denied for /tmp/not found/file.txt'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); await expect(kaos.writeText('/tmp/not found/file.txt', 'fresh', { mode: 'a' })) .rejects.toBeInstanceOf(KaosError); @@ -400,7 +400,7 @@ describe('AcpKaos', () => { throw RequestError.internalError(undefined, 'transient transport blip'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); await expect(kaos.writeText('/a.ts', 'new', { mode: 'a' })).rejects.toBeInstanceOf( KaosError, ); @@ -415,15 +415,15 @@ describe('AcpKaos', () => { throw rpcErr; }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); await expect(kaos.writeText('/a.ts', 'hello')).rejects.toBeInstanceOf(KaosError); try { await kaos.writeText('/a.ts', 'hello'); - } catch (err) { - expect((err as Error & { cause?: unknown }).cause).toBe(rpcErr); - expect((err as Error).message).toContain('acp: writeTextFile failed for /a.ts'); - expect((err as Error).message).toContain('write rpc died'); + } catch (error) { + expect((error as Error & { cause?: unknown }).cause).toBe(rpcErr); + expect((error as Error).message).toContain('acp: writeTextFile failed for /a.ts'); + expect((error as Error).message).toContain('write rpc died'); } }); }); @@ -431,7 +431,7 @@ describe('AcpKaos', () => { describe('writeBytes', () => { it('forwards utf8-decoded content via conn.writeTextFile, returns byte count', async () => { const conn = makeMockConn({}); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); const n = await kaos.writeBytes('/a.ts', Buffer.from('hi')); expect(n).toBe(2); expect(conn.writeCalls).toEqual([{ sessionId: 's1', path: '/a.ts', content: 'hi' }]); @@ -444,7 +444,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'BRIDGED' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); const child = kaos.withCwd('/new/cwd'); expect(child).toBeInstanceOf(AcpKaos); @@ -463,7 +463,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'BRIDGED' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); const env = { FOO: 'bar' }; const child = kaos.withEnv(env); @@ -479,7 +479,7 @@ describe('AcpKaos', () => { it('delegates pathClass, normpath, gethome, getcwd to inner', () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); expect(kaos.pathClass()).toBe('posix'); expect(kaos.normpath('/foo')).toBe('/foo'); @@ -495,7 +495,7 @@ describe('AcpKaos', () => { it('delegates chdir, stat, mkdir to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); await kaos.chdir('/x'); await kaos.stat('/y', { followSymlinks: false }); @@ -509,7 +509,7 @@ describe('AcpKaos', () => { it('delegates iterdir and glob to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); // Just consume the generators — the inner spy records the call. for await (const _ of kaos.iterdir('/d')) { @@ -528,7 +528,7 @@ describe('AcpKaos', () => { it('delegates exec and execWithEnv to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); await kaos.exec('ls', '-la'); await kaos.execWithEnv(['env'], { FOO: 'bar' }); @@ -542,7 +542,7 @@ describe('AcpKaos', () => { it('exposes a wrapping name and the inner osEnv', () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner); + const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); expect(kaos.name).toBe('acp(mock-inner)'); expect(kaos.osEnv).toBe(inner.osEnv); }); diff --git a/packages/acp-adapter/test/kaos-activation.test.ts b/packages/acp-adapter/test/kaos-activation.test.ts index ad6b03c0d..eeb227cf5 100644 --- a/packages/acp-adapter/test/kaos-activation.test.ts +++ b/packages/acp-adapter/test/kaos-activation.test.ts @@ -26,7 +26,9 @@ import { } from '@agentclientprotocol/sdk'; import type { Kaos } from '@moonshot-ai/kaos'; import type { KimiHarness, Session } from '@moonshot-ai/kimi-code-sdk'; -import { describe, expect, it } from 'vitest'; +import { promises as fsp } from 'node:fs'; +import path from 'node:path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { AcpKaos } from '../src/kaos-acp'; import { AcpServer } from '../src/server'; @@ -81,6 +83,19 @@ function makeHarness(captured: CapturedCreate[]): KimiHarness { } describe('AcpServer FS-capability activation (boundary injection)', () => { + // The boundary check on `cwd` requires a realpath-able path; these + // tests predate that and exercise delegation only, but the server + // still has to construct an AcpKaos (with realpathed roots) before + // the call reaches the harness. Use a mkdtemp'd scratch dir and + // teardown so the activation surface is exercised end-to-end. + let scratchWork: string; + beforeAll(async () => { + scratchWork = await fsp.mkdtemp(path.join('/tmp', 'acp-activation-')); + }); + afterAll(async () => { + if (scratchWork) await fsp.rm(scratchWork, { recursive: true, force: true }); + }); + it('passes an AcpKaos to createSession when the client advertises fs.readTextFile', async () => { const captured: CapturedCreate[] = []; const harness = makeHarness(captured); @@ -93,7 +108,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: true, writeTextFile: false } }, }); - await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + await client.newSession({ cwd: scratchWork, mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeInstanceOf(AcpKaos); @@ -114,7 +129,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: true } }, }); - await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + await client.newSession({ cwd: scratchWork, mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeInstanceOf(AcpKaos); @@ -134,7 +149,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } }, }); - await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + await client.newSession({ cwd: scratchWork, mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeUndefined(); @@ -153,7 +168,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } }, }); - await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + await client.newSession({ cwd: scratchWork, mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeUndefined(); @@ -180,13 +195,13 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: true } }, }); - const response = await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + const response = await client.newSession({ cwd: scratchWork, mcpServers: [] }); const kaos = captured[0]?.options.kaos; expect(kaos).toBeInstanceOf(AcpKaos); // Drive a reverse-RPC read through the AcpKaos and verify the // sessionId on the wire matches the one returned by newSession. - await kaos!.readText('/abs/file.ts'); + await kaos!.readText(path.join(scratchWork, 'file.ts')); expect(observedSessionId).toBe(response.sessionId); }); }); diff --git a/packages/acp-adapter/test/kaos-boundary.test.ts b/packages/acp-adapter/test/kaos-boundary.test.ts new file mode 100644 index 000000000..301a439f0 --- /dev/null +++ b/packages/acp-adapter/test/kaos-boundary.test.ts @@ -0,0 +1,232 @@ +/** + * Integration tests: {@link AcpKaos} enforces `[cwd, ...additionalDirectories]` + * as the effective root set on every file operation. + * + * The boundary guarantee is the security-bearing layer of Phase 17. + * Without it, the `additionalDirectories` capability is purely + * advisory — an agent that wanted to `Read /etc/passwd` could bypass + * the client's trust boundary by calling `readText('/etc/passwd')` + * and the bridge would happily forward it. These tests use real + * filesystem roots (mkdtemp + symlinks) and a `mock conn` that fails + * the test if reached before the boundary check, so a regression to + * "no boundary check" surfaces as a clear failure rather than a + * silent leak. + */ + +import { promises as fsp } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import type { AgentSideConnection } from '@agentclientprotocol/sdk'; +import { KaosError, type Environment, type Kaos, type KaosProcess, type StatResult } from '@moonshot-ai/kaos'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { AcpKaos } from '../src/kaos-acp'; +import { resolveCanonicalRoots } from '../src/path-boundary'; + +let cwdRoot: string; +let extraRoot: string; + +beforeEach(async () => { + cwdRoot = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-cwd-')); + extraRoot = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-extra-')); +}); + +afterEach(async () => { + await fsp.rm(cwdRoot, { recursive: true, force: true }); + await fsp.rm(extraRoot, { recursive: true, force: true }); +}); + +/** + * Connection mock whose every RPC call is recorded AND returns dummy + * content (instead of throwing). Tests assert `rpcCalls.readTextFile` + * is empty after a boundary-violation attempt: a non-zero count means + * the boundary check fired AFTER the RPC was made, which is the + * regression we want to catch. + */ +function makeRecordingConn(): { + conn: AgentSideConnection; + rpcCalls: { readTextFile: number; writeTextFile: number }; +} { + const rpcCalls = { readTextFile: 0, writeTextFile: 0 }; + return { + rpcCalls, + conn: { + readTextFile: vi.fn(async () => { + rpcCalls.readTextFile += 1; + return { content: '' }; + }), + writeTextFile: vi.fn(async () => { + rpcCalls.writeTextFile += 1; + return {}; + }), + } as unknown as AgentSideConnection, + }; +} + +/** Minimal inner Kaos that never delegates (tests don't exercise it). */ +function makeInner(): Kaos { + return { + name: 'mock-inner', + osEnv: { os: 'linux', shell: 'bash' } as unknown as Environment, + pathClass: () => 'posix', + normpath: (p: string) => p, + gethome: () => '/home/mock', + getcwd: () => '/cwd', + chdir: async () => undefined, + withCwd: () => makeInner(), + withEnv: () => makeInner(), + stat: async () => ({}) as StatResult, + iterdir: async function* () { + yield* []; + }, + glob: async function* () { + yield* []; + }, + mkdir: async () => undefined, + exec: async () => ({}) as KaosProcess, + execWithEnv: async () => ({}) as KaosProcess, + readText: async () => 'INNER', + readBytes: async () => Buffer.alloc(0), + readLines: async function* () { + yield* []; + }, + writeText: async (_p: string, data: string) => data.length, + writeBytes: async () => 0, + } as unknown as Kaos; +} + +async function makeKaos(opts?: { + overrideConn?: { conn: AgentSideConnection; rpcCalls: { readTextFile: number; writeTextFile: number } }; +}): Promise { + const roots = await resolveCanonicalRoots([cwdRoot, extraRoot]); + const recording = opts?.overrideConn ?? makeRecordingConn(); + return new AcpKaos(recording.conn, 'session-test', makeInner(), roots); +} + +describe('AcpKaos — boundary enforcement', () => { + it('readText outside roots throws KaosError and never reaches the conn', async () => { + const recording = makeRecordingConn(); + const kaos = await makeKaos({ overrideConn: recording }); + await expect(kaos.readText('/etc/passwd')).rejects.toBeInstanceOf(KaosError); + expect(recording.rpcCalls.readTextFile).toBe(0); + }); + + it('readText via symlink escape inside cwd throws KaosError and never reaches the conn', async () => { + const outside = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-out-')); + try { + await fsp.writeFile(path.join(outside, 'secret.txt'), 'secret'); + await fsp.symlink(outside, path.join(cwdRoot, 'escape'), 'dir'); + const recording = makeRecordingConn(); + const kaos = await makeKaos({ overrideConn: recording }); + await expect(kaos.readText(path.join(cwdRoot, 'escape', 'secret.txt'))).rejects.toBeInstanceOf( + KaosError, + ); + expect(recording.rpcCalls.readTextFile).toBe(0); + } finally { + await fsp.rm(outside, { recursive: true, force: true }); + } + }); + + it('readText inside cwd delegates to the conn (no boundary violation)', async () => { + const roots = await resolveCanonicalRoots([cwdRoot]); + const target = path.join(cwdRoot, 'plain.txt'); + await fsp.writeFile(target, 'hi'); + const conn = { + readTextFile: vi.fn(async () => ({ content: 'from-rpc' })), + writeTextFile: vi.fn(), + } as unknown as AgentSideConnection; + const kaos = new AcpKaos(conn, 'session-test', makeInner(), roots); + const out = await kaos.readText(target); + expect(out).toBe('from-rpc'); + expect(conn.readTextFile).toHaveBeenCalledOnce(); + }); + + it('readText inside an additionalDirectories root delegates to the conn', async () => { + const target = path.join(extraRoot, 'lib.ts'); + await fsp.writeFile(target, 'export {};'); + const roots = await resolveCanonicalRoots([cwdRoot, extraRoot]); + const conn = { + readTextFile: vi.fn(async () => ({ content: 'from-rpc' })), + writeTextFile: vi.fn(), + } as unknown as AgentSideConnection; + const kaos = new AcpKaos(conn, 'session-test', makeInner(), roots); + const out = await kaos.readText(target); + expect(out).toBe('from-rpc'); + }); + + it('writeText outside roots throws KaosError', async () => { + const recording = makeRecordingConn(); + const kaos = await makeKaos({ overrideConn: recording }); + await expect(kaos.writeText('/tmp/acp-kaos-should-not-exist.txt', 'x')).rejects.toBeInstanceOf( + KaosError, + ); + expect(recording.rpcCalls.writeTextFile).toBe(0); + }); + + it('writeText to a non-existent path whose parent symlink-escapes throws', async () => { + const outside = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-out-')); + try { + await fsp.symlink(outside, path.join(cwdRoot, 'esc'), 'dir'); + const recording = makeRecordingConn(); + const kaos = await makeKaos({ overrideConn: recording }); + await expect( + kaos.writeText(path.join(cwdRoot, 'esc', 'new.txt'), 'x'), + ).rejects.toBeInstanceOf(KaosError); + // RPC non-reach proof: the boundary check fires BEFORE writeTextFile. + expect(recording.rpcCalls.writeTextFile).toBe(0); + // And: nothing got written to the escape target either. + const targetExists = await fsp + .stat(path.join(outside, 'new.txt')) + .then(() => true) + .catch(() => false); + expect(targetExists).toBe(false); + } finally { + await fsp.rm(outside, { recursive: true, force: true }); + } + }); + + it('readBytes outside roots throws KaosError (no longer bypasses via inner)', async () => { + const kaos = await makeKaos(); + await expect(kaos.readBytes('/etc/passwd')).rejects.toBeInstanceOf(KaosError); + }); + + it('stat outside roots throws KaosError', async () => { + const kaos = await makeKaos(); + await expect(kaos.stat('/etc/passwd')).rejects.toBeInstanceOf(KaosError); + }); + + it('iterdir outside roots throws KaosError', async () => { + const kaos = await makeKaos(); + // iterdir returns an async iterator; we have to start consuming + // before the throw surfaces. + await expect((async () => { + for await (const _ of kaos.iterdir('/etc')) { + // drain + } + })()).rejects.toBeInstanceOf(KaosError); + }); + + it('glob outside roots throws KaosError', async () => { + const kaos = await makeKaos(); + await expect((async () => { + for await (const _ of kaos.glob('/etc', '*')) { + // drain + } + })()).rejects.toBeInstanceOf(KaosError); + }); + + it('mkdir outside roots throws KaosError', async () => { + const kaos = await makeKaos(); + await expect(kaos.mkdir('/tmp/acp-kaos-mkdir')).rejects.toBeInstanceOf(KaosError); + }); + + it('withCwd preserves the effective roots across the wrapper', async () => { + const kaos = await makeKaos(); + const child = kaos.withCwd('/somewhere/else') as AcpKaos; + // The child shares roots — even though the bound cwd has moved, + // a path under `/etc` is still rejected because `/etc` isn't in + // the session's effective root set. + await expect(child.readText('/etc/passwd')).rejects.toBeInstanceOf(KaosError); + }); +}); diff --git a/packages/acp-adapter/test/path-boundary.test.ts b/packages/acp-adapter/test/path-boundary.test.ts new file mode 100644 index 000000000..5e9f7a41a --- /dev/null +++ b/packages/acp-adapter/test/path-boundary.test.ts @@ -0,0 +1,187 @@ +/** + * Unit tests for {@link resolveCanonicalPath} and {@link assertPathInRoots}. + * + * The boundary checker is the security-bearing primitive invoked by + * {@link AcpKaos} on every file operation: it MUST refuse escapes via + * symlinks (per ACP `additionalDirectories` RFD), and it MUST handle + * the "path doesn't yet exist" case for writes to new files. These + * tests use real files on a `mkdtemp` scratch tree plus real + * `symlinkSync` to exercise the symlink-escape vector end-to-end — + * mocking `realpath` would test the mock, not the property we care + * about. + */ + +import { promises as fsp } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { KaosError } from '@moonshot-ai/kaos'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { + assertPathInRoots, + resolveCanonicalPath, + resolveCanonicalRoots, +} from '../src/path-boundary'; + +let scratch: string; + +beforeEach(async () => { + scratch = await fsp.mkdtemp(path.join(tmpdir(), 'acp-boundary-')); +}); + +afterEach(async () => { + await fsp.rm(scratch, { recursive: true, force: true }); +}); + +describe('resolveCanonicalPath', () => { + it('returns realpath for an existing regular file', async () => { + const target = path.join(scratch, 'file.txt'); + await fsp.writeFile(target, 'hi'); + const canonical = await resolveCanonicalPath(target); + expect(canonical).toBe(await fsp.realpath(target)); + }); + + it('resolves symlinks in the middle of the path', async () => { + const real = path.join(scratch, 'real'); + const linkDir = path.join(scratch, 'links'); + await fsp.mkdir(real); + await fsp.writeFile(path.join(real, 'inside.txt'), 'hi'); + await fsp.mkdir(linkDir); + await fsp.symlink(real, path.join(linkDir, 'to-real'), 'dir'); + const canonical = await resolveCanonicalPath(path.join(linkDir, 'to-real', 'inside.txt')); + expect(canonical).toBe(path.join(real, 'inside.txt')); + }); + + it('handles non-existent leaf by anchoring at the deepest existing ancestor', async () => { + // `scratch` exists; `scratch/newdir/new.txt` does not. After this + // call we should be anchored at the canonical form of `scratch` + // with the suffix appended — so a symlink scramble inside + // `scratch` still gets resolved before the check fires. + const canonical = await resolveCanonicalPath(path.join(scratch, 'newdir', 'new.txt')); + expect(canonical.startsWith(await fsp.realpath(scratch) + path.sep)).toBe(true); + expect(canonical.endsWith(path.join('newdir', 'new.txt'))).toBe(true); + }); + + it('resolves a fully non-existent path by anchoring at the filesystem root', async () => { + // Walking up the parent chain always hits `/`, which is realpath- + // able on any unix host. The "no canonical ancestor exists" branch + // is only reachable on a path whose deepest mountpoint is itself + // missing — a degenerate case that's effectively impossible to + // contrive in unit tests. What we CAN verify is the canonical + // form comes out anchored at `/` with the suffix appended. + const canonical = await resolveCanonicalPath('/totally/made/up/path.txt'); + expect(canonical).toBe(`/totally/made/up/path.txt`); + }); +}); + +describe('resolveCanonicalRoots', () => { + it('realpaths every supplied root preserving order', async () => { + const a = path.join(scratch, 'a'); + const b = path.join(scratch, 'b'); + await fsp.mkdir(a); + await fsp.mkdir(b); + const out = await resolveCanonicalRoots([a, b]); + expect(out).toEqual([await fsp.realpath(a), await fsp.realpath(b)]); + }); + + it('throws when any root does not exist (fail-closed at session init)', async () => { + const good = path.join(scratch, 'good'); + const missing = path.join(scratch, 'gone'); + await fsp.mkdir(good); + await expect(resolveCanonicalRoots([good, missing])).rejects.toBeInstanceOf(KaosError); + }); + + it('returns empty array for empty input', async () => { + expect(await resolveCanonicalRoots([])).toEqual([]); + }); +}); + +describe('assertPathInRoots', () => { + it('accepts a path inside the primary cwd root', async () => { + const cwd = path.join(scratch, 'cwd'); + await fsp.mkdir(cwd); + const inner = path.join(cwd, 'inside.txt'); + await fsp.writeFile(inner, 'hi'); + const roots = await resolveCanonicalRoots([cwd]); + await expect(assertPathInRoots(inner, roots, 'read')).resolves.toBeUndefined(); + }); + + it('accepts a path inside an additional directory root', async () => { + const cwd = path.join(scratch, 'cwd'); + const extra = path.join(scratch, 'extra'); + await fsp.mkdir(cwd); + await fsp.mkdir(extra); + const inner = path.join(extra, 'lib', 'x.ts'); + await fsp.mkdir(path.dirname(inner), { recursive: true }); + await fsp.writeFile(inner, 'hi'); + const roots = await resolveCanonicalRoots([cwd, extra]); + await expect(assertPathInRoots(inner, roots, 'read')).resolves.toBeUndefined(); + }); + + it('rejects a path that escapes via a symlinked directory', async () => { + const cwd = path.join(scratch, 'cwd'); + const outside = path.join(scratch, 'outside'); + await fsp.mkdir(cwd); + await fsp.mkdir(outside); + await fsp.writeFile(path.join(outside, 'secret.txt'), 'secret'); + // Plant the trap: cwd/escape -> outside + await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); + const roots = await resolveCanonicalRoots([cwd]); + await expect( + assertPathInRoots(path.join(cwd, 'escape', 'secret.txt'), roots, 'read'), + ).rejects.toBeInstanceOf(KaosError); + }); + + it('rejects a sibling path that merely sounds similar', async () => { + const cwd = path.join(scratch, 'cwd'); + await fsp.mkdir(cwd); + const sibling = path.join(scratch, 'cwd-evil', 'leak.txt'); + await fsp.mkdir(path.dirname(sibling), { recursive: true }); + await fsp.writeFile(sibling, 'leak'); + const roots = await resolveCanonicalRoots([cwd]); + // `scratch/cwd-evil` is NOT inside `scratch/cwd` after canonical + // resolution (different segment), so the prefix-startsWith check + // must catch it. + await expect(assertPathInRoots(sibling, roots, 'read')).rejects.toBeInstanceOf(KaosError); + }); + + it('rejects a write target whose parent is a symlink escape', async () => { + const cwd = path.join(scratch, 'cwd'); + const outside = path.join(scratch, 'outside'); + await fsp.mkdir(cwd); + await fsp.mkdir(outside); + await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); + const roots = await resolveCanonicalRoots([cwd]); + // `cwd/escape/new.txt` doesn't exist yet; assertPathInRoots must + // anchor at the realpath'd parent (= scratch/outside) and reject. + await expect( + assertPathInRoots(path.join(cwd, 'escape', 'new.txt'), roots, 'write'), + ).rejects.toBeInstanceOf(KaosError); + }); + + it('errors include the operation name in the message', async () => { + const cwd = path.join(scratch, 'cwd'); + const outside = path.join(scratch, 'outside'); + await fsp.mkdir(cwd); + await fsp.mkdir(outside); + await fsp.writeFile(path.join(outside, 'secret.txt'), 's'); + await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); + const roots = await resolveCanonicalRoots([cwd]); + await expect( + assertPathInRoots(path.join(cwd, 'escape', 'secret.txt'), roots, 'write'), + ).rejects.toThrow(/write/i); + }); + + it('rejects a fully non-existent path that resolves outside the roots', async () => { + // The path `/totally/made/up/file.txt` doesn't exist on disk; the + // canonical-resolve walk anchors at `/` and the result still + // doesn't fall under `scratch/cwd` — boundary check fires. + const cwd = path.join(scratch, 'cwd'); + await fsp.mkdir(cwd); + const roots = await resolveCanonicalRoots([cwd]); + await expect( + assertPathInRoots('/totally/made/up/file.txt', roots, 'read'), + ).rejects.toBeInstanceOf(KaosError); + }); +}); diff --git a/packages/acp-adapter/test/server.test.ts b/packages/acp-adapter/test/server.test.ts index d7e6e527f..43efc5982 100644 --- a/packages/acp-adapter/test/server.test.ts +++ b/packages/acp-adapter/test/server.test.ts @@ -16,7 +16,7 @@ import { } from '@agentclientprotocol/sdk'; import type { KimiHarness } from '@moonshot-ai/kimi-code-sdk'; -import { AcpServer } from '../src/server'; +import { AcpServer, validateAdditionalDirectories } from '../src/server'; import { TERMINAL_AUTH_METHOD } from '../src'; /** Minimal Client that throws on every callback so tests fail loudly. */ @@ -203,3 +203,49 @@ describe('AcpServer + AgentSideConnection', () => { expect(method._meta?.['terminal-auth']).toBeUndefined(); }); }); + +describe('validateAdditionalDirectories', () => { + it('returns undefined when dirs is undefined', () => { + expect(validateAdditionalDirectories(undefined)).toBeUndefined(); + }); + + it('returns undefined when dirs is null', () => { + expect(validateAdditionalDirectories(null)).toBeUndefined(); + }); + + it('returns the array when dirs is a valid string array', () => { + const dirs = ['/home/user/projects', '/tmp/work']; + expect(validateAdditionalDirectories(dirs)).toEqual(dirs); + }); + + it('returns the empty array when dirs is an empty array', () => { + expect(validateAdditionalDirectories([])).toEqual([]); + }); + + it('throws when dirs is not an array', () => { + expect(() => validateAdditionalDirectories('not-an-array')).toThrow(); + expect(() => validateAdditionalDirectories(42)).toThrow(); + expect(() => validateAdditionalDirectories({})).toThrow(); + }); + + it('throws when a dirs entry is not a string', () => { + expect(() => validateAdditionalDirectories(['/valid', 42])).toThrow(); + expect(() => validateAdditionalDirectories(['/valid', null as unknown as string])).toThrow(); + expect(() => validateAdditionalDirectories(['/valid', undefined as unknown as string])).toThrow(); + }); + + it('throws when a dirs entry is an empty string', () => { + expect(() => validateAdditionalDirectories(['/valid', ''])).toThrow(); + }); + + it('throws when a dirs entry is not an absolute path', () => { + expect(() => validateAdditionalDirectories(['/valid', 'relative/path'])).toThrow(); + expect(() => validateAdditionalDirectories(['/valid', './relative'])).toThrow(); + }); + + it('throws with a message that includes the invalid dirs and index', () => { + expect(() => validateAdditionalDirectories(['/valid', 42])).toThrow(/additionalDirectories\[1\]/); + expect(() => validateAdditionalDirectories(['/valid', ''])).toThrow(/additionalDirectories\[1\]/); + expect(() => validateAdditionalDirectories(['/valid', 'relative'])).toThrow(/additionalDirectories\[1\]/); + }); +}); diff --git a/packages/acp-adapter/test/session-load.test.ts b/packages/acp-adapter/test/session-load.test.ts index 18dc4710a..8c1bdede2 100644 --- a/packages/acp-adapter/test/session-load.test.ts +++ b/packages/acp-adapter/test/session-load.test.ts @@ -155,6 +155,41 @@ describe('AcpServer session/load replay', () => { }); }); + it('omits additionalDirs when additionalDirectories is absent on load', async () => { + const sessionId = 'sess-load-no-adddir'; + const session = makeSessionWithHistory(sessionId, []); + const capturedResumeInputs: Array> = []; + const harness = { + auth: { status: async () => AUTHED_STATUS }, + resumeSession: async (input: Record) => { + capturedResumeInputs.push(input); + return session; + }, + getConfig: async () => ({ + providers: {}, + defaultModel: 'kimi-coder', + models: makeModelsMap([ + { id: 'kimi-coder', name: 'Kimi Coder', thinkingSupported: true }, + ]), + }), + } as unknown as KimiHarness; + const { agentStream, clientStream } = makeInMemoryStreamPair(); + + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const clientConn = new ClientSideConnection((_a) => new CapturingClient(), clientStream); + + await clientConn.loadSession({ + sessionId, + cwd: '/tmp/work', + mcpServers: [], + }); + + expect(capturedResumeInputs).toHaveLength(1); + expect( + (capturedResumeInputs[0] as { additionalDirs?: unknown }).additionalDirs, + ).toBeUndefined(); + }); + it('replays a single assistant text-only turn as agent_message_chunk updates', async () => { const sessionId = 'sess-text-only'; const history = [ diff --git a/packages/acp-adapter/test/session-new.test.ts b/packages/acp-adapter/test/session-new.test.ts index a82776e8f..0570b8c0b 100644 --- a/packages/acp-adapter/test/session-new.test.ts +++ b/packages/acp-adapter/test/session-new.test.ts @@ -175,6 +175,108 @@ describe('AcpServer session/new', () => { expect(captured[0]?.options.additionalDirs).toEqual(['/tmp/docs', '/tmp/plugin']); }); + it('omits additionalDirs from createSession when additionalDirectories is absent', async () => { + const captured: CapturedCall[] = []; + const { harness } = makeHarness('sess-omit', captured); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); + + expect(captured).toHaveLength(1); + expect((captured[0]!.options as { additionalDirs?: readonly string[] }).additionalDirs).toBeUndefined(); + }); + + it('forwards empty additionalDirectories as empty array', async () => { + const captured: CapturedCall[] = []; + const { harness } = makeHarness('sess-empty', captured); + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await client.newSession({ cwd: '/tmp/work', mcpServers: [], additionalDirectories: [] }); + + expect(captured).toHaveLength(1); + expect((captured[0]!.options as { additionalDirs?: readonly string[] }).additionalDirs).toEqual([]); + }); + + it('rejects non-array additionalDirectories with invalid_params', async () => { + const harness = { + auth: { status: async () => AUTHED_STATUS }, + getConfig: async () => ({ providers: {}, models: {} }), + } as unknown as KimiHarness; + + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await expect( + client.newSession({ + cwd: '/tmp/work', + mcpServers: [], + additionalDirectories: 'not-an-array' as never, + }), + ).rejects.toMatchObject({ code: -32602 }); + }); + + it('rejects non-string entry in additionalDirectories', async () => { + const harness = { + auth: { status: async () => AUTHED_STATUS }, + getConfig: async () => ({ providers: {}, models: {} }), + } as unknown as KimiHarness; + + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await expect( + client.newSession({ + cwd: '/tmp/work', + mcpServers: [], + additionalDirectories: [42] as never, + }), + ).rejects.toMatchObject({ code: -32602 }); + }); + + it('rejects empty string entry in additionalDirectories', async () => { + const harness = { + auth: { status: async () => AUTHED_STATUS }, + getConfig: async () => ({ providers: {}, models: {} }), + } as unknown as KimiHarness; + + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await expect( + client.newSession({ + cwd: '/tmp/work', + mcpServers: [], + additionalDirectories: [''], + }), + ).rejects.toMatchObject({ code: -32602 }); + }); + + it('rejects relative path in additionalDirectories', async () => { + const harness = { + auth: { status: async () => AUTHED_STATUS }, + getConfig: async () => ({ providers: {}, models: {} }), + } as unknown as KimiHarness; + + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const client = new ClientSideConnection((_a) => new StubClient(), clientStream); + + await expect( + client.newSession({ + cwd: '/tmp/work', + mcpServers: [], + additionalDirectories: ['relative/path'], + }), + ).rejects.toMatchObject({ code: -32602 }); + }); + it('advertises configOptions (PLAN D11 + Phase 15 thinking toggle) — model + thinking + mode under the unified SessionConfigOption surface', async () => { const captured: CapturedCall[] = []; const { harness } = makeHarness('sess-modes', captured); diff --git a/packages/acp-adapter/test/session-resume.test.ts b/packages/acp-adapter/test/session-resume.test.ts index b05eac723..3453d7879 100644 --- a/packages/acp-adapter/test/session-resume.test.ts +++ b/packages/acp-adapter/test/session-resume.test.ts @@ -165,6 +165,40 @@ describe('AcpServer.resumeSession', () => { }); }); + it('omits additionalDirs when additionalDirectories is absent on resume', async () => { + const sessionId = 'sess-resume-no-adddir'; + const session = makeSessionWithMainConfig(sessionId); + const capturedResumeInputs: Array> = []; + const harness = { + auth: { status: async () => AUTHED_STATUS }, + resumeSession: async (input: Record) => { + capturedResumeInputs.push(input); + return session; + }, + getConfig: async () => ({ + providers: {}, + defaultModel: 'kimi-coder', + models: makeModelsMap([ + { id: 'kimi-coder', name: 'Kimi Coder', thinkingSupported: true }, + ]), + }), + } as unknown as KimiHarness; + const { agentStream, clientStream } = makeInMemoryStreamPair(); + new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); + const clientConn = new ClientSideConnection((_a) => new CapturingClient(), clientStream); + + await clientConn.resumeSession({ + sessionId, + cwd: '/tmp/work', + mcpServers: [], + }); + + expect(capturedResumeInputs).toHaveLength(1); + expect( + (capturedResumeInputs[0] as { additionalDirs?: unknown }).additionalDirs, + ).toBeUndefined(); + }); + it('returns configOptions matching the resumed session model + mode + thinking', async () => { const sessionId = 'sess-resume-model'; // Resume state reports kimi-plain (thinking unsupported) so we can From 022e111e71ab563c285721d0850c61cfce502add Mon Sep 17 00:00:00 2001 From: yzhou Date: Sat, 4 Jul 2026 21:38:53 +0800 Subject: [PATCH 4/9] revert(acp-adapter): remove per-operation path boundary from AcpKaos Remove the assertPathInRoots boundary checks from every AcpKaos file operation (stat, readText, writeText, etc.) and the associated resolveCanonicalRoots machinery. Keep the validateAdditionalDirectories function for input-level validation of the additionalDirectories parameter. --- .changeset/remove-acp-path-boundary.md | 5 + packages/acp-adapter/src/kaos-acp.ts | 64 +---- packages/acp-adapter/src/path-boundary.ts | 150 ----------- packages/acp-adapter/src/server.ts | 45 +--- .../acp-adapter/test/boundary-session.test.ts | 154 ------------ packages/acp-adapter/test/e2e-fs.test.ts | 20 +- packages/acp-adapter/test/kaos-acp.test.ts | 64 ++--- .../acp-adapter/test/kaos-activation.test.ts | 29 +-- .../acp-adapter/test/kaos-boundary.test.ts | 232 ------------------ .../acp-adapter/test/path-boundary.test.ts | 187 -------------- 10 files changed, 63 insertions(+), 887 deletions(-) create mode 100644 .changeset/remove-acp-path-boundary.md delete mode 100644 packages/acp-adapter/src/path-boundary.ts delete mode 100644 packages/acp-adapter/test/boundary-session.test.ts delete mode 100644 packages/acp-adapter/test/kaos-boundary.test.ts delete mode 100644 packages/acp-adapter/test/path-boundary.test.ts diff --git a/.changeset/remove-acp-path-boundary.md b/.changeset/remove-acp-path-boundary.md new file mode 100644 index 000000000..ab7cef33d --- /dev/null +++ b/.changeset/remove-acp-path-boundary.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/acp-adapter": patch +--- + +Remove the per-operation path boundary enforcement from AcpKaos while retaining the additionalDirectories input validation. diff --git a/packages/acp-adapter/src/kaos-acp.ts b/packages/acp-adapter/src/kaos-acp.ts index 5a9c9b2e3..40f9759ef 100644 --- a/packages/acp-adapter/src/kaos-acp.ts +++ b/packages/acp-adapter/src/kaos-acp.ts @@ -28,8 +28,6 @@ import { type StatResult, } from '@moonshot-ai/kaos'; -import { assertPathInRoots } from './path-boundary'; - /** * `Kaos` that routes `read*` / `write*` through the ACP reverse-RPC * channel and delegates everything else to `inner`. @@ -46,19 +44,6 @@ export class AcpKaos implements Kaos { private readonly conn: AgentSideConnection, private readonly sessionId: string, private readonly inner: Kaos, - /** - * Canonical (realpath-resolved) workspace roots this AcpKaos - * treats as authoritative: `[cwd, ...additionalDirectories]`. - * Set at construction time so the per-call hot path only - * resolves the target, not the roots. Must be the output of - * {@link resolveCanonicalRoots} — caller is responsible for - * realpath'ing the supplied cwd + additionalDirectories up front - * (see `maybeBuildAcpKaos` in `server.ts`). - * - * Carried unchanged through {@link withCwd} / {@link withEnv} — - * the effective root set is session-scoped, not cwd-scoped. - */ - private readonly effectiveRoots: readonly string[], ) {} // ── identity ──────────────────────────────────────────────────────── @@ -99,47 +84,33 @@ export class AcpKaos implements Kaos { * instance — so a `chdir` followed by `readText('relative.ts')` * continues to hit the ACP bridge rather than silently dropping back * to local filesystem reads. - * - * The effective root set (this adapter's `additionalDirectories` - * boundary) is **session-scoped**, not cwd-scoped, so it carries - * forward unchanged — changing `inner.withCwd()` does not retune - * which paths are allowed. */ withCwd(cwd: string): Kaos { - return new AcpKaos(this.conn, this.sessionId, this.inner.withCwd(cwd), this.effectiveRoots); + return new AcpKaos(this.conn, this.sessionId, this.inner.withCwd(cwd)); } withEnv(env: Record): Kaos { - return new AcpKaos(this.conn, this.sessionId, this.inner.withEnv(env), this.effectiveRoots); + return new AcpKaos(this.conn, this.sessionId, this.inner.withEnv(env)); } - async stat(path: string, options?: { followSymlinks?: boolean }): Promise { - await assertPathInRoots(path, this.effectiveRoots, 'stat'); + stat(path: string, options?: { followSymlinks?: boolean }): Promise { return this.inner.stat(path, options); } - async *iterdir(path: string): AsyncGenerator { - // Anchor the check before yielding so callers that never consume - // the iterator still see the boundary violation. - await assertPathInRoots(path, this.effectiveRoots, 'iterdir'); - yield* this.inner.iterdir(path); + iterdir(path: string): AsyncGenerator { + return this.inner.iterdir(path); } - async *glob( + glob( path: string, pattern: string, options?: { caseSensitive?: boolean }, ): AsyncGenerator { - await assertPathInRoots(path, this.effectiveRoots, 'glob'); - yield* this.inner.glob(path, pattern, options); + return this.inner.glob(path, pattern, options); } - async mkdir( - path: string, - options?: { parents?: boolean; existOk?: boolean }, - ): Promise { - await assertPathInRoots(path, this.effectiveRoots, 'mkdir'); - await this.inner.mkdir(path, options); + mkdir(path: string, options?: { parents?: boolean; existOk?: boolean }): Promise { + return this.inner.mkdir(path, options); } // ── reads: route through ACP `fs/readTextFile` ───────────────────── @@ -155,7 +126,6 @@ export class AcpKaos implements Kaos { path: string, _options?: { encoding?: BufferEncoding; errors?: 'strict' | 'replace' | 'ignore' }, ): Promise { - await assertPathInRoots(path, this.effectiveRoots, 'readText'); const rpcPath = this.toClientPath(path); try { const resp = await this.conn.readTextFile({ sessionId: this.sessionId, path: rpcPath }); @@ -171,14 +141,8 @@ export class AcpKaos implements Kaos { * payloads (images, video, archives — anything `ReadMediaFile` may * touch). The ACP bridge only owns the *text* surface; raw bytes * stay on the local filesystem via `inner`. - * - * Even though the bytes go to `inner`, the path is still subject - * to the effective-root check — without that, a model could read - * `/etc/passwd` bytes by routing through the binary surface. The - * `additionalDirectories` boundary is about scope, not surface. */ - async readBytes(path: string, n?: number): Promise { - await assertPathInRoots(path, this.effectiveRoots, 'readBytes'); + readBytes(path: string, n?: number): Promise { return this.inner.readBytes(path, n); } @@ -236,13 +200,6 @@ export class AcpKaos implements Kaos { data: string, options?: { mode?: 'w' | 'a'; encoding?: BufferEncoding }, ): Promise { - // Single boundary check for the outer path, regardless of mode. - // The append-mode's read-then-write fallback re-checks the same - // target via readText, which would otherwise double-resolve and - // race the same canonical path twice. The outer check is what - // gates the write; the inner read is a fallback for "file does - // not exist yet", not a separate authorization surface. - await assertPathInRoots(path, this.effectiveRoots, 'writeText'); if (options?.mode === 'a') { let existing = ''; try { @@ -264,7 +221,6 @@ export class AcpKaos implements Kaos { * (Read/Write/Edit tools), not binary streaming. */ async writeBytes(path: string, data: Buffer): Promise { - await assertPathInRoots(path, this.effectiveRoots, 'writeBytes'); await this.acpWrite(path, data.toString('utf8')); return data.byteLength; } diff --git a/packages/acp-adapter/src/path-boundary.ts b/packages/acp-adapter/src/path-boundary.ts deleted file mode 100644 index bce512322..000000000 --- a/packages/acp-adapter/src/path-boundary.ts +++ /dev/null @@ -1,150 +0,0 @@ -/** - * Path-boundary primitives for the ACP `additionalDirectories` feature. - * - * The boundary check is the security-bearing layer behind - * `AcpKaos.readText` / `writeText` / etc. — every file operation goes - * through `assertPathInRoots` BEFORE the ACP reverse-RPC call, so a - * model-side `Read /etc/passwd` cannot bypass the client's trust - * boundary by hopping through the kernel's local filesystem. - * - * Three primitives: - * - * {@link resolveCanonicalPath} - * Symlink-resolving canonicalisation for a target path. Handles - * the "path doesn't yet exist" case (writes to new files) by - * anchoring at the deepest existing ancestor. - * - * {@link resolveCanonicalRoots} - * Realpath every supplied root at session-init time. Missing - * roots cause `KaosError` — this is the spec's fail-closed-at- - * creation guarantee. - * - * {@link assertPathInRoots} - * Pre-flight check: target must canonicalise to a location - * inside one of the canonicalised roots, OR be exactly a root. - * Failure raises `KaosError` carrying the operation name. - * - * All primitives use `fs.realpath` so symlinks/junctions/mount points - * are resolved before the prefix comparison — the prefix check would - * otherwise miss escapes planted under a parent symlink. - */ - -import { promises as fsp } from 'node:fs'; -import path from 'node:path'; - -import { KaosError } from '@moonshot-ai/kaos'; - -/** - * Resolve `p` to its canonical, symlink-resolved form. - * - * - If `p` exists, returns `fs.realpath(p)`. - * - If `p` (or any parent) doesn't exist, walks up until it finds an - * existing ancestor, realpaths that, and reappends the suffix. The - * realpath'd ancestor is the security anchor: any symlink planted - * between it and `p` is invisible to us, but the prefix check in - * {@link assertPathInRoots} still uses the canonical ancestor's - * prefix — and the canonical ancestor IS inside-or-outside the - * root set, which is what we care about. - * - * Throws {@link KaosError} if the walk exhausts before hitting any - * realpath-able anchor (e.g. a path whose root segment is missing). - */ -export async function resolveCanonicalPath(p: string): Promise { - try { - return await fsp.realpath(p); - } catch (firstErr) { - let cursor = path.dirname(p); - let suffix = path.basename(p); - let lastErr: unknown = firstErr; - while (true) { - try { - const realCursor = await fsp.realpath(cursor); - return path.join(realCursor, suffix); - } catch (e) { - lastErr = e; - const next = path.dirname(cursor); - if (next === cursor) break; - suffix = path.join(path.basename(cursor), suffix); - cursor = next; - } - } - throw new KaosError( - `path-boundary: no canonical ancestor for ${p}: ${describeErr(lastErr)}`, - ); - } -} - -/** - * Canonicalise every root up front. Missing roots fail-closed. - * - * Called once per session inside `maybeBuildAcpKaos`; passing the - * result into {@link AcpKaos}'s constructor means every subsequent - * `assertPathInRoots` call only handles the per-target resolve, not - * the root resolve. That keeps the per-call hot path to a single - * `realpath(target)` + O(N) prefix loop, instead of N `realpath` - * per call. - */ -export async function resolveCanonicalRoots( - roots: readonly string[], -): Promise { - const out: string[] = []; - for (const r of roots) { - try { - out.push(await fsp.realpath(r)); - } catch (err) { - throw new KaosError( - `path-boundary: cannot resolve root ${r}: ${describeErr(err)}`, - ); - } - } - return out; -} - -/** - * Throw {@link KaosError} if `target`, after canonicalisation, is not - * inside any of the canonical `roots`. - * - * Pass `roots` as the OUTPUT of {@link resolveCanonicalRoots} — - * passing raw root paths would silently miss escapes if a root - * itself is a symlink. - * - * The `operation` string is included in the error message so logs - * can attribute the failure to a read vs write vs stat without - * separate bookkeeping. - */ -export async function assertPathInRoots( - target: string, - roots: readonly string[], - operation: string, -): Promise { - if (roots.length === 0) { - // No roots allowed → fail closed. Spec: "If you cannot safely - // determine whether the path is inside allowed roots, fail - // closed." Equally, if the boundary literally is empty, every - // path is outside. - throw new KaosError( - `path-boundary: ${operation} refused for ${target}: no workspace roots configured`, - ); - } - const realTarget = await resolveCanonicalPath(target); - for (const root of roots) { - if (realTarget === root) return; - // Preserve trailing separator on root (e.g. '/' on POSIX) so the - // prefix check works for the filesystem root itself, where - // `root + path.sep` would expand to '//' and never match. - const prefix = root.endsWith(path.sep) ? root : root + path.sep; - if (realTarget.startsWith(prefix)) return; - } - throw new KaosError( - `path-boundary: ${operation} refused for ${target} — outside workspace roots`, - ); -} - -function describeErr(err: unknown): string { - if (err && typeof err === 'object' && 'code' in err) { - const code = String((err as { code: unknown }).code); - const msg = err instanceof Error ? err.message : typeof err === 'string' ? err : 'Unknown error'; - return `${code}: ${msg}`; - } - return err instanceof Error ? err.message : typeof err === 'string' ? err : 'Unknown error'; -} diff --git a/packages/acp-adapter/src/server.ts b/packages/acp-adapter/src/server.ts index 334c68575..b620bde72 100644 --- a/packages/acp-adapter/src/server.ts +++ b/packages/acp-adapter/src/server.ts @@ -52,7 +52,6 @@ import { LocalKaos, type Kaos } from '@moonshot-ai/kaos'; import { TERMINAL_AUTH_METHOD, buildTerminalAuthMethod } from './auth-methods'; import { redirectConsoleToStderr } from './log-guard'; import { AcpKaos } from './kaos-acp'; -import { resolveCanonicalRoots } from './path-boundary'; import { AcpSession, type TelemetryTrackFn } from './session'; import { buildSessionConfigOptions } from './config-options'; import { availableCommandsUpdateNotification } from './events-map'; @@ -285,7 +284,7 @@ export class AcpServer implements Agent { // the same reference, no AsyncLocalStorage needed. const sessionId = `session_${randomUUID()}`; const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); - const acpKaos = await this.maybeBuildAcpKaos(sessionId, params.cwd, additionalDirs); + const acpKaos = await this.maybeBuildAcpKaos(sessionId); const persistenceKaos = acpKaos === undefined ? undefined : await this.ensureInnerKaos(); const session = await this.harness.createSession({ id: sessionId, @@ -460,11 +459,7 @@ export class AcpServer implements Agent { // `resumeSession` spreads `input` so unknown fields ride to the // kernel. const mcpServers = acpMcpServersToConfigs(params.mcpServers); - const acpKaos = await this.maybeBuildAcpKaos( - params.sessionId, - params.cwd, - params.additionalDirs, - ); + const acpKaos = await this.maybeBuildAcpKaos(params.sessionId); const persistenceKaos = acpKaos === undefined ? undefined : await this.ensureInnerKaos(); let session: Session; try { @@ -546,24 +541,8 @@ export class AcpServer implements Agent { * it. The resulting {@link AcpKaos} is captured by the kernel * `SessionImpl` ctor and every tool downstream sees the same * reference — no AsyncLocalStorage involved. - * - * @param sessionId ACP-side session id (also used as the - * reverse-RPC session binding). - * @param additionalDirs Optional additional workspace roots from - * `additionalDirectories`. Combined with - * `cwd` (supplied by the caller) to form - * the effective root set the {@link AcpKaos} - * enforces on every file operation. - * Missing / non-existent roots cause a - * structured `invalid_params` rejection — - * fail-closed at session-init time per the - * ACP `additionalDirectories` RFD. */ - private async maybeBuildAcpKaos( - sessionId: string, - cwd: string, - additionalDirs?: readonly string[], - ): Promise { + private async maybeBuildAcpKaos(sessionId: string): Promise { const fs = this.clientCapabilities?.fs; if (!fs?.readTextFile && !fs?.writeTextFile) { return undefined; @@ -572,22 +551,7 @@ export class AcpServer implements Agent { return undefined; } const innerKaos = await this.ensureInnerKaos(); - const roots = [cwd, ...(additionalDirs ?? [])]; - let canonical: string[]; - try { - canonical = await resolveCanonicalRoots(roots); - } catch (err) { - // Fail-closed: a missing/unresolved root is a misconfigured - // session, not a transient condition. Surface it as a - // structured JSON-RPC `invalid_params` so the client sees a - // clear failure rather than a tool-time boundary error many - // turns later. - throw RequestError.invalidParams( - { additionalDirectories: additionalDirs ?? [] }, - `cannot resolve additionalDirectories on disk: ${err instanceof Error ? err.message : String(err)}`, - ); - } - return new AcpKaos(this.conn, sessionId, innerKaos, canonical); + return new AcpKaos(this.conn, sessionId, innerKaos); } private async ensureInnerKaos(): Promise { @@ -1185,3 +1149,4 @@ export function validateAdditionalDirectories( } return dirs; } + diff --git a/packages/acp-adapter/test/boundary-session.test.ts b/packages/acp-adapter/test/boundary-session.test.ts deleted file mode 100644 index 91a70b277..000000000 --- a/packages/acp-adapter/test/boundary-session.test.ts +++ /dev/null @@ -1,154 +0,0 @@ -/** - * Session-level boundary tests: an `additionalDirectories` entry that - * does not exist on disk must be rejected at `session/new` time so a - * misconfigured client can't push the failure into a tool call many - * turns later. The effective root set is resolved when the boundary- - * aware {@link AcpKaos} is built (`maybeBuildAcpKaos`); if any root's - * `realpath` fails we surface a structured `invalid_params` error - * before the harness sees the request. - * - * These tests cover the wiring, not the boundary logic itself (that - * lives in `path-boundary.test.ts` / `kaos-boundary.test.ts`). Each - * test advertises the FS reverse-RPC capability on `initialize` so - * `maybeBuildAcpKaos` actually constructs an `AcpKaos` and runs the - * root-set resolution; without the capability, the boundary code is - * never reached and the test would be testing nothing. - */ - -import { - AgentSideConnection, - ClientSideConnection, - ndJsonStream, - type Client, - type ReadTextFileRequest, - type ReadTextFileResponse, - type RequestPermissionRequest, - type RequestPermissionResponse, - type SessionNotification, - type WriteTextFileRequest, - type WriteTextFileResponse, -} from '@agentclientprotocol/sdk'; -import type { KimiHarness } from '@moonshot-ai/kimi-code-sdk'; -import { promises as fsp } from 'node:fs'; -import { tmpdir } from 'node:os'; -import path from 'node:path'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -import { AcpServer } from '../src/server'; -import { AUTHED_STATUS } from './_helpers/harness-stubs'; - -function makeInMemoryStreamPair(): { - agentStream: ReturnType; - clientStream: ReturnType; -} { - const clientToAgent = new TransformStream(); - const agentToClient = new TransformStream(); - const agentStream = ndJsonStream(agentToClient.writable, clientToAgent.readable); - const clientStream = ndJsonStream(clientToAgent.writable, agentToClient.readable); - return { agentStream, clientStream }; -} - -class StubClient implements Client { - async readTextFile(_p: ReadTextFileRequest): Promise { - throw new Error('not exercised'); - } - async writeTextFile(_p: WriteTextFileRequest): Promise { - throw new Error('not exercised'); - } - async sessionUpdate(_n: SessionNotification): Promise { - /* no-op */ - } - async requestPermission(_p: RequestPermissionRequest): Promise { - throw new Error('not exercised'); - } -} - -function makeHarness(): { - harness: KimiHarness; - calls: { newSession: number; resumeSession: number }; -} { - const calls = { newSession: 0, resumeSession: 0 }; - const harness = { - auth: { status: async () => AUTHED_STATUS }, - createSession: vi.fn(async (options: Record) => { - calls.newSession += 1; - return { - id: String((options as { id?: string }).id ?? 'sess-x'), - prompt: async () => undefined, - cancel: async () => undefined, - onEvent: () => () => undefined, - }; - }), - resumeSession: vi.fn(async (options: Record) => { - calls.resumeSession += 1; - return { - id: String((options as { id?: string }).id ?? 'sess-r'), - prompt: async () => undefined, - cancel: async () => undefined, - onEvent: () => () => undefined, - }; - }), - getConfig: async () => ({ providers: {}, models: {} }), - } as unknown as KimiHarness; - return { harness, calls }; -} - -let cwdOnDisk: string; - -beforeEach(async () => { - cwdOnDisk = await fsp.mkdtemp(path.join(tmpdir(), 'acp-sess-cwd-')); -}); - -afterEach(async () => { - await fsp.rm(cwdOnDisk, { recursive: true, force: true }); -}); - -describe('session/new — boundary', () => { - it('rejects when an additionalDirectories entry does not exist on disk', async () => { - const { harness, calls } = makeHarness(); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); - const client = new ClientSideConnection((_a) => new StubClient(), clientStream); - - await client.initialize({ - protocolVersion: 1, - clientCapabilities: { fs: { readTextFile: true, writeTextFile: true } }, - }); - - await expect( - client.newSession({ - cwd: cwdOnDisk, - mcpServers: [], - additionalDirectories: [path.join(cwdOnDisk, 'definitely-not-here')], - }), - ).rejects.toMatchObject({ code: -32602 }); - - // Strong invariant: harness.createSession must NOT have been - // called when boundary validation fails — that's the whole point - // of validating upfront. - expect(calls.newSession).toBe(0); - }); - - it('accepts when every additionalDirectory entry exists on disk', async () => { - const extra = path.join(cwdOnDisk, 'shared'); - await fsp.mkdir(extra); - const { harness, calls } = makeHarness(); - const { agentStream, clientStream } = makeInMemoryStreamPair(); - new AgentSideConnection((c) => new AcpServer(harness, c), agentStream); - const client = new ClientSideConnection((_a) => new StubClient(), clientStream); - - await client.initialize({ - protocolVersion: 1, - clientCapabilities: { fs: { readTextFile: true, writeTextFile: true } }, - }); - - const response = await client.newSession({ - cwd: cwdOnDisk, - mcpServers: [], - additionalDirectories: [extra], - }); - - expect(response.sessionId).toBeDefined(); - expect(calls.newSession).toBe(1); - }); -}); diff --git a/packages/acp-adapter/test/e2e-fs.test.ts b/packages/acp-adapter/test/e2e-fs.test.ts index f365f8317..f2ef77c11 100644 --- a/packages/acp-adapter/test/e2e-fs.test.ts +++ b/packages/acp-adapter/test/e2e-fs.test.ts @@ -37,9 +37,7 @@ import { } from '@agentclientprotocol/sdk'; import type { Kaos } from '@moonshot-ai/kaos'; import type { Event, KimiHarness, Session } from '@moonshot-ai/kimi-code-sdk'; -import { promises as fsp } from 'node:fs'; -import path from 'node:path'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { AcpServer } from '../src/server'; import { AUTHED_STATUS } from './_helpers/harness-stubs'; @@ -127,18 +125,8 @@ function makeReadingSession( const textBlock = (text: string): ContentBlock => ({ type: 'text', text }); describe('end-to-end FS reverse-RPC', () => { - // The boundary check on `cwd` requires a realpath-able path; these - // e2e tests use mkdtemp'd scratch rather than the synthetic `/tmp/x` - // they used to carry. Set up once for the suite and tear it down. - let scratchWork: string; - beforeAll(async () => { - scratchWork = await fsp.mkdtemp(path.join('/tmp', 'acp-e2efs-')); - }); - afterAll(async () => { - if (scratchWork) await fsp.rm(scratchWork, { recursive: true, force: true }); - }); it('routes a tool-time readText through the client when fs.readTextFile is advertised', async () => { - const targetPath = path.join(scratchWork, 'x.ts'); + const targetPath = '/Users/test/x.ts'; let createdSession: Session | undefined; let capturedSessionId: string | undefined; const harness = { @@ -165,7 +153,7 @@ describe('end-to-end FS reverse-RPC', () => { }, }); - const newSession = await client.newSession({ cwd: scratchWork, mcpServers: [] }); + const newSession = await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); const response = await client.prompt({ sessionId: newSession.sessionId, @@ -250,7 +238,7 @@ describe('end-to-end FS reverse-RPC', () => { }, }); - const newSession = await client.newSession({ cwd: scratchWork, mcpServers: [] }); + const newSession = await client.newSession({ cwd: '/tmp/x', mcpServers: [] }); const response = await client.prompt({ sessionId: newSession.sessionId, diff --git a/packages/acp-adapter/test/kaos-acp.test.ts b/packages/acp-adapter/test/kaos-acp.test.ts index 43894cb6f..5f8d8e69c 100644 --- a/packages/acp-adapter/test/kaos-acp.test.ts +++ b/packages/acp-adapter/test/kaos-acp.test.ts @@ -206,7 +206,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'HELLO' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); const result = await kaos.readText('/a.ts'); @@ -223,7 +223,7 @@ describe('AcpKaos', () => { throw rpcErr; }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); await expect(kaos.readText('/x.ts')).rejects.toMatchObject({ name: 'KaosError', @@ -233,10 +233,10 @@ describe('AcpKaos', () => { try { await kaos.readText('/x.ts'); throw new Error('should have thrown'); - } catch (error) { - expect((error as Error & { cause?: unknown }).cause).toBe(rpcErr); - expect((error as Error).message).toContain('acp: readTextFile failed for /x.ts'); - expect((error as Error).message).toContain('rpc died'); + } catch (err) { + expect((err as Error & { cause?: unknown }).cause).toBe(rpcErr); + expect((err as Error).message).toContain('acp: readTextFile failed for /x.ts'); + expect((err as Error).message).toContain('rpc died'); } }); @@ -245,7 +245,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'HELLO' }), }); const inner = makeMockInner({ pathClass: 'win32' }); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); await kaos.readText('G:/python-code/render_with_mult_gpu/README.md'); await kaos.writeText('G:/python-code/render_with_mult_gpu/README.md', 'updated'); @@ -274,7 +274,7 @@ describe('AcpKaos', () => { }, }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); const buf = await kaos.readBytes('/img.png', 4); expect(buf).toBeInstanceOf(Buffer); @@ -288,7 +288,7 @@ describe('AcpKaos', () => { it('forwards omitted n to inner unchanged', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); const buf = await kaos.readBytes('/img.png'); expect(buf.byteLength).toBe(8); @@ -305,20 +305,20 @@ describe('AcpKaos', () => { it('yields each line of "a\\nb\\nc" with terminators preserved', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb\nc' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b\n', 'c']); }); it('drops the trailing empty token when the file ends with a newline', async () => { // "a\nb\n" → ['a\n', 'b\n'] (NOT ['a\n', 'b\n', '']) const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb\n' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b\n']); }); it('yields the final line without a trailing newline when missing', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'a\nb' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\n', 'b']); }); @@ -326,13 +326,13 @@ describe('AcpKaos', () => { // ReadTool depends on this — stripping \n would expose bare \r and // render visible carriage returns. const conn = makeMockConn({ readHandler: async () => ({ content: 'a\r\nb\r\n' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); expect(await collect(kaos.readLines('/a.ts'))).toEqual(['a\r\n', 'b\r\n']); }); it('yields nothing for an empty file', async () => { const conn = makeMockConn({ readHandler: async () => ({ content: '' }) }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); expect(await collect(kaos.readLines('/a.ts'))).toEqual([]); }); }); @@ -340,7 +340,7 @@ describe('AcpKaos', () => { describe('writeText', () => { it('forwards content to conn.writeTextFile and returns char count', async () => { const conn = makeMockConn({}); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); const n = await kaos.writeText('/a.ts', 'hello'); expect(n).toBe(5); expect(conn.writeCalls).toEqual([{ sessionId: 's1', path: '/a.ts', content: 'hello' }]); @@ -350,7 +350,7 @@ describe('AcpKaos', () => { const conn = makeMockConn({ readHandler: async () => ({ content: 'old:' }), }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); const n = await kaos.writeText('/a.ts', 'new', { mode: 'a' }); // Return value is the size of the appended data, not the merged size. expect(n).toBe(3); @@ -367,7 +367,7 @@ describe('AcpKaos', () => { throw RequestError.resourceNotFound('/missing.ts'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); const n = await kaos.writeText('/missing.ts', 'fresh', { mode: 'a' }); expect(n).toBe(5); expect(conn.writeCalls).toEqual([ @@ -384,7 +384,7 @@ describe('AcpKaos', () => { throw new Error('permission denied for /tmp/not found/file.txt'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); await expect(kaos.writeText('/tmp/not found/file.txt', 'fresh', { mode: 'a' })) .rejects.toBeInstanceOf(KaosError); @@ -400,7 +400,7 @@ describe('AcpKaos', () => { throw RequestError.internalError(undefined, 'transient transport blip'); }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); await expect(kaos.writeText('/a.ts', 'new', { mode: 'a' })).rejects.toBeInstanceOf( KaosError, ); @@ -415,15 +415,15 @@ describe('AcpKaos', () => { throw rpcErr; }, }); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); await expect(kaos.writeText('/a.ts', 'hello')).rejects.toBeInstanceOf(KaosError); try { await kaos.writeText('/a.ts', 'hello'); - } catch (error) { - expect((error as Error & { cause?: unknown }).cause).toBe(rpcErr); - expect((error as Error).message).toContain('acp: writeTextFile failed for /a.ts'); - expect((error as Error).message).toContain('write rpc died'); + } catch (err) { + expect((err as Error & { cause?: unknown }).cause).toBe(rpcErr); + expect((err as Error).message).toContain('acp: writeTextFile failed for /a.ts'); + expect((err as Error).message).toContain('write rpc died'); } }); }); @@ -431,7 +431,7 @@ describe('AcpKaos', () => { describe('writeBytes', () => { it('forwards utf8-decoded content via conn.writeTextFile, returns byte count', async () => { const conn = makeMockConn({}); - const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner(), ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', makeMockInner()); const n = await kaos.writeBytes('/a.ts', Buffer.from('hi')); expect(n).toBe(2); expect(conn.writeCalls).toEqual([{ sessionId: 's1', path: '/a.ts', content: 'hi' }]); @@ -444,7 +444,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'BRIDGED' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); const child = kaos.withCwd('/new/cwd'); expect(child).toBeInstanceOf(AcpKaos); @@ -463,7 +463,7 @@ describe('AcpKaos', () => { readHandler: async () => ({ content: 'BRIDGED' }), }); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); const env = { FOO: 'bar' }; const child = kaos.withEnv(env); @@ -479,7 +479,7 @@ describe('AcpKaos', () => { it('delegates pathClass, normpath, gethome, getcwd to inner', () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); expect(kaos.pathClass()).toBe('posix'); expect(kaos.normpath('/foo')).toBe('/foo'); @@ -495,7 +495,7 @@ describe('AcpKaos', () => { it('delegates chdir, stat, mkdir to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); await kaos.chdir('/x'); await kaos.stat('/y', { followSymlinks: false }); @@ -509,7 +509,7 @@ describe('AcpKaos', () => { it('delegates iterdir and glob to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); // Just consume the generators — the inner spy records the call. for await (const _ of kaos.iterdir('/d')) { @@ -528,7 +528,7 @@ describe('AcpKaos', () => { it('delegates exec and execWithEnv to inner', async () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); await kaos.exec('ls', '-la'); await kaos.execWithEnv(['env'], { FOO: 'bar' }); @@ -542,7 +542,7 @@ describe('AcpKaos', () => { it('exposes a wrapping name and the inner osEnv', () => { const conn = makeMockConn({}); const inner = makeMockInner(); - const kaos = new AcpKaos(conn.asConn(), 's1', inner, ['/']); + const kaos = new AcpKaos(conn.asConn(), 's1', inner); expect(kaos.name).toBe('acp(mock-inner)'); expect(kaos.osEnv).toBe(inner.osEnv); }); diff --git a/packages/acp-adapter/test/kaos-activation.test.ts b/packages/acp-adapter/test/kaos-activation.test.ts index eeb227cf5..ad6b03c0d 100644 --- a/packages/acp-adapter/test/kaos-activation.test.ts +++ b/packages/acp-adapter/test/kaos-activation.test.ts @@ -26,9 +26,7 @@ import { } from '@agentclientprotocol/sdk'; import type { Kaos } from '@moonshot-ai/kaos'; import type { KimiHarness, Session } from '@moonshot-ai/kimi-code-sdk'; -import { promises as fsp } from 'node:fs'; -import path from 'node:path'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { AcpKaos } from '../src/kaos-acp'; import { AcpServer } from '../src/server'; @@ -83,19 +81,6 @@ function makeHarness(captured: CapturedCreate[]): KimiHarness { } describe('AcpServer FS-capability activation (boundary injection)', () => { - // The boundary check on `cwd` requires a realpath-able path; these - // tests predate that and exercise delegation only, but the server - // still has to construct an AcpKaos (with realpathed roots) before - // the call reaches the harness. Use a mkdtemp'd scratch dir and - // teardown so the activation surface is exercised end-to-end. - let scratchWork: string; - beforeAll(async () => { - scratchWork = await fsp.mkdtemp(path.join('/tmp', 'acp-activation-')); - }); - afterAll(async () => { - if (scratchWork) await fsp.rm(scratchWork, { recursive: true, force: true }); - }); - it('passes an AcpKaos to createSession when the client advertises fs.readTextFile', async () => { const captured: CapturedCreate[] = []; const harness = makeHarness(captured); @@ -108,7 +93,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: true, writeTextFile: false } }, }); - await client.newSession({ cwd: scratchWork, mcpServers: [] }); + await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeInstanceOf(AcpKaos); @@ -129,7 +114,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: true } }, }); - await client.newSession({ cwd: scratchWork, mcpServers: [] }); + await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeInstanceOf(AcpKaos); @@ -149,7 +134,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } }, }); - await client.newSession({ cwd: scratchWork, mcpServers: [] }); + await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeUndefined(); @@ -168,7 +153,7 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } }, }); - await client.newSession({ cwd: scratchWork, mcpServers: [] }); + await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); expect(captured).toHaveLength(1); expect(captured[0]?.options.kaos).toBeUndefined(); @@ -195,13 +180,13 @@ describe('AcpServer FS-capability activation (boundary injection)', () => { protocolVersion: 1, clientCapabilities: { fs: { readTextFile: true } }, }); - const response = await client.newSession({ cwd: scratchWork, mcpServers: [] }); + const response = await client.newSession({ cwd: '/tmp/work', mcpServers: [] }); const kaos = captured[0]?.options.kaos; expect(kaos).toBeInstanceOf(AcpKaos); // Drive a reverse-RPC read through the AcpKaos and verify the // sessionId on the wire matches the one returned by newSession. - await kaos!.readText(path.join(scratchWork, 'file.ts')); + await kaos!.readText('/abs/file.ts'); expect(observedSessionId).toBe(response.sessionId); }); }); diff --git a/packages/acp-adapter/test/kaos-boundary.test.ts b/packages/acp-adapter/test/kaos-boundary.test.ts deleted file mode 100644 index 301a439f0..000000000 --- a/packages/acp-adapter/test/kaos-boundary.test.ts +++ /dev/null @@ -1,232 +0,0 @@ -/** - * Integration tests: {@link AcpKaos} enforces `[cwd, ...additionalDirectories]` - * as the effective root set on every file operation. - * - * The boundary guarantee is the security-bearing layer of Phase 17. - * Without it, the `additionalDirectories` capability is purely - * advisory — an agent that wanted to `Read /etc/passwd` could bypass - * the client's trust boundary by calling `readText('/etc/passwd')` - * and the bridge would happily forward it. These tests use real - * filesystem roots (mkdtemp + symlinks) and a `mock conn` that fails - * the test if reached before the boundary check, so a regression to - * "no boundary check" surfaces as a clear failure rather than a - * silent leak. - */ - -import { promises as fsp } from 'node:fs'; -import { tmpdir } from 'node:os'; -import path from 'node:path'; - -import type { AgentSideConnection } from '@agentclientprotocol/sdk'; -import { KaosError, type Environment, type Kaos, type KaosProcess, type StatResult } from '@moonshot-ai/kaos'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; - -import { AcpKaos } from '../src/kaos-acp'; -import { resolveCanonicalRoots } from '../src/path-boundary'; - -let cwdRoot: string; -let extraRoot: string; - -beforeEach(async () => { - cwdRoot = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-cwd-')); - extraRoot = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-extra-')); -}); - -afterEach(async () => { - await fsp.rm(cwdRoot, { recursive: true, force: true }); - await fsp.rm(extraRoot, { recursive: true, force: true }); -}); - -/** - * Connection mock whose every RPC call is recorded AND returns dummy - * content (instead of throwing). Tests assert `rpcCalls.readTextFile` - * is empty after a boundary-violation attempt: a non-zero count means - * the boundary check fired AFTER the RPC was made, which is the - * regression we want to catch. - */ -function makeRecordingConn(): { - conn: AgentSideConnection; - rpcCalls: { readTextFile: number; writeTextFile: number }; -} { - const rpcCalls = { readTextFile: 0, writeTextFile: 0 }; - return { - rpcCalls, - conn: { - readTextFile: vi.fn(async () => { - rpcCalls.readTextFile += 1; - return { content: '' }; - }), - writeTextFile: vi.fn(async () => { - rpcCalls.writeTextFile += 1; - return {}; - }), - } as unknown as AgentSideConnection, - }; -} - -/** Minimal inner Kaos that never delegates (tests don't exercise it). */ -function makeInner(): Kaos { - return { - name: 'mock-inner', - osEnv: { os: 'linux', shell: 'bash' } as unknown as Environment, - pathClass: () => 'posix', - normpath: (p: string) => p, - gethome: () => '/home/mock', - getcwd: () => '/cwd', - chdir: async () => undefined, - withCwd: () => makeInner(), - withEnv: () => makeInner(), - stat: async () => ({}) as StatResult, - iterdir: async function* () { - yield* []; - }, - glob: async function* () { - yield* []; - }, - mkdir: async () => undefined, - exec: async () => ({}) as KaosProcess, - execWithEnv: async () => ({}) as KaosProcess, - readText: async () => 'INNER', - readBytes: async () => Buffer.alloc(0), - readLines: async function* () { - yield* []; - }, - writeText: async (_p: string, data: string) => data.length, - writeBytes: async () => 0, - } as unknown as Kaos; -} - -async function makeKaos(opts?: { - overrideConn?: { conn: AgentSideConnection; rpcCalls: { readTextFile: number; writeTextFile: number } }; -}): Promise { - const roots = await resolveCanonicalRoots([cwdRoot, extraRoot]); - const recording = opts?.overrideConn ?? makeRecordingConn(); - return new AcpKaos(recording.conn, 'session-test', makeInner(), roots); -} - -describe('AcpKaos — boundary enforcement', () => { - it('readText outside roots throws KaosError and never reaches the conn', async () => { - const recording = makeRecordingConn(); - const kaos = await makeKaos({ overrideConn: recording }); - await expect(kaos.readText('/etc/passwd')).rejects.toBeInstanceOf(KaosError); - expect(recording.rpcCalls.readTextFile).toBe(0); - }); - - it('readText via symlink escape inside cwd throws KaosError and never reaches the conn', async () => { - const outside = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-out-')); - try { - await fsp.writeFile(path.join(outside, 'secret.txt'), 'secret'); - await fsp.symlink(outside, path.join(cwdRoot, 'escape'), 'dir'); - const recording = makeRecordingConn(); - const kaos = await makeKaos({ overrideConn: recording }); - await expect(kaos.readText(path.join(cwdRoot, 'escape', 'secret.txt'))).rejects.toBeInstanceOf( - KaosError, - ); - expect(recording.rpcCalls.readTextFile).toBe(0); - } finally { - await fsp.rm(outside, { recursive: true, force: true }); - } - }); - - it('readText inside cwd delegates to the conn (no boundary violation)', async () => { - const roots = await resolveCanonicalRoots([cwdRoot]); - const target = path.join(cwdRoot, 'plain.txt'); - await fsp.writeFile(target, 'hi'); - const conn = { - readTextFile: vi.fn(async () => ({ content: 'from-rpc' })), - writeTextFile: vi.fn(), - } as unknown as AgentSideConnection; - const kaos = new AcpKaos(conn, 'session-test', makeInner(), roots); - const out = await kaos.readText(target); - expect(out).toBe('from-rpc'); - expect(conn.readTextFile).toHaveBeenCalledOnce(); - }); - - it('readText inside an additionalDirectories root delegates to the conn', async () => { - const target = path.join(extraRoot, 'lib.ts'); - await fsp.writeFile(target, 'export {};'); - const roots = await resolveCanonicalRoots([cwdRoot, extraRoot]); - const conn = { - readTextFile: vi.fn(async () => ({ content: 'from-rpc' })), - writeTextFile: vi.fn(), - } as unknown as AgentSideConnection; - const kaos = new AcpKaos(conn, 'session-test', makeInner(), roots); - const out = await kaos.readText(target); - expect(out).toBe('from-rpc'); - }); - - it('writeText outside roots throws KaosError', async () => { - const recording = makeRecordingConn(); - const kaos = await makeKaos({ overrideConn: recording }); - await expect(kaos.writeText('/tmp/acp-kaos-should-not-exist.txt', 'x')).rejects.toBeInstanceOf( - KaosError, - ); - expect(recording.rpcCalls.writeTextFile).toBe(0); - }); - - it('writeText to a non-existent path whose parent symlink-escapes throws', async () => { - const outside = await fsp.mkdtemp(path.join(tmpdir(), 'acp-kaos-out-')); - try { - await fsp.symlink(outside, path.join(cwdRoot, 'esc'), 'dir'); - const recording = makeRecordingConn(); - const kaos = await makeKaos({ overrideConn: recording }); - await expect( - kaos.writeText(path.join(cwdRoot, 'esc', 'new.txt'), 'x'), - ).rejects.toBeInstanceOf(KaosError); - // RPC non-reach proof: the boundary check fires BEFORE writeTextFile. - expect(recording.rpcCalls.writeTextFile).toBe(0); - // And: nothing got written to the escape target either. - const targetExists = await fsp - .stat(path.join(outside, 'new.txt')) - .then(() => true) - .catch(() => false); - expect(targetExists).toBe(false); - } finally { - await fsp.rm(outside, { recursive: true, force: true }); - } - }); - - it('readBytes outside roots throws KaosError (no longer bypasses via inner)', async () => { - const kaos = await makeKaos(); - await expect(kaos.readBytes('/etc/passwd')).rejects.toBeInstanceOf(KaosError); - }); - - it('stat outside roots throws KaosError', async () => { - const kaos = await makeKaos(); - await expect(kaos.stat('/etc/passwd')).rejects.toBeInstanceOf(KaosError); - }); - - it('iterdir outside roots throws KaosError', async () => { - const kaos = await makeKaos(); - // iterdir returns an async iterator; we have to start consuming - // before the throw surfaces. - await expect((async () => { - for await (const _ of kaos.iterdir('/etc')) { - // drain - } - })()).rejects.toBeInstanceOf(KaosError); - }); - - it('glob outside roots throws KaosError', async () => { - const kaos = await makeKaos(); - await expect((async () => { - for await (const _ of kaos.glob('/etc', '*')) { - // drain - } - })()).rejects.toBeInstanceOf(KaosError); - }); - - it('mkdir outside roots throws KaosError', async () => { - const kaos = await makeKaos(); - await expect(kaos.mkdir('/tmp/acp-kaos-mkdir')).rejects.toBeInstanceOf(KaosError); - }); - - it('withCwd preserves the effective roots across the wrapper', async () => { - const kaos = await makeKaos(); - const child = kaos.withCwd('/somewhere/else') as AcpKaos; - // The child shares roots — even though the bound cwd has moved, - // a path under `/etc` is still rejected because `/etc` isn't in - // the session's effective root set. - await expect(child.readText('/etc/passwd')).rejects.toBeInstanceOf(KaosError); - }); -}); diff --git a/packages/acp-adapter/test/path-boundary.test.ts b/packages/acp-adapter/test/path-boundary.test.ts deleted file mode 100644 index 5e9f7a41a..000000000 --- a/packages/acp-adapter/test/path-boundary.test.ts +++ /dev/null @@ -1,187 +0,0 @@ -/** - * Unit tests for {@link resolveCanonicalPath} and {@link assertPathInRoots}. - * - * The boundary checker is the security-bearing primitive invoked by - * {@link AcpKaos} on every file operation: it MUST refuse escapes via - * symlinks (per ACP `additionalDirectories` RFD), and it MUST handle - * the "path doesn't yet exist" case for writes to new files. These - * tests use real files on a `mkdtemp` scratch tree plus real - * `symlinkSync` to exercise the symlink-escape vector end-to-end — - * mocking `realpath` would test the mock, not the property we care - * about. - */ - -import { promises as fsp } from 'node:fs'; -import { tmpdir } from 'node:os'; -import path from 'node:path'; - -import { KaosError } from '@moonshot-ai/kaos'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; - -import { - assertPathInRoots, - resolveCanonicalPath, - resolveCanonicalRoots, -} from '../src/path-boundary'; - -let scratch: string; - -beforeEach(async () => { - scratch = await fsp.mkdtemp(path.join(tmpdir(), 'acp-boundary-')); -}); - -afterEach(async () => { - await fsp.rm(scratch, { recursive: true, force: true }); -}); - -describe('resolveCanonicalPath', () => { - it('returns realpath for an existing regular file', async () => { - const target = path.join(scratch, 'file.txt'); - await fsp.writeFile(target, 'hi'); - const canonical = await resolveCanonicalPath(target); - expect(canonical).toBe(await fsp.realpath(target)); - }); - - it('resolves symlinks in the middle of the path', async () => { - const real = path.join(scratch, 'real'); - const linkDir = path.join(scratch, 'links'); - await fsp.mkdir(real); - await fsp.writeFile(path.join(real, 'inside.txt'), 'hi'); - await fsp.mkdir(linkDir); - await fsp.symlink(real, path.join(linkDir, 'to-real'), 'dir'); - const canonical = await resolveCanonicalPath(path.join(linkDir, 'to-real', 'inside.txt')); - expect(canonical).toBe(path.join(real, 'inside.txt')); - }); - - it('handles non-existent leaf by anchoring at the deepest existing ancestor', async () => { - // `scratch` exists; `scratch/newdir/new.txt` does not. After this - // call we should be anchored at the canonical form of `scratch` - // with the suffix appended — so a symlink scramble inside - // `scratch` still gets resolved before the check fires. - const canonical = await resolveCanonicalPath(path.join(scratch, 'newdir', 'new.txt')); - expect(canonical.startsWith(await fsp.realpath(scratch) + path.sep)).toBe(true); - expect(canonical.endsWith(path.join('newdir', 'new.txt'))).toBe(true); - }); - - it('resolves a fully non-existent path by anchoring at the filesystem root', async () => { - // Walking up the parent chain always hits `/`, which is realpath- - // able on any unix host. The "no canonical ancestor exists" branch - // is only reachable on a path whose deepest mountpoint is itself - // missing — a degenerate case that's effectively impossible to - // contrive in unit tests. What we CAN verify is the canonical - // form comes out anchored at `/` with the suffix appended. - const canonical = await resolveCanonicalPath('/totally/made/up/path.txt'); - expect(canonical).toBe(`/totally/made/up/path.txt`); - }); -}); - -describe('resolveCanonicalRoots', () => { - it('realpaths every supplied root preserving order', async () => { - const a = path.join(scratch, 'a'); - const b = path.join(scratch, 'b'); - await fsp.mkdir(a); - await fsp.mkdir(b); - const out = await resolveCanonicalRoots([a, b]); - expect(out).toEqual([await fsp.realpath(a), await fsp.realpath(b)]); - }); - - it('throws when any root does not exist (fail-closed at session init)', async () => { - const good = path.join(scratch, 'good'); - const missing = path.join(scratch, 'gone'); - await fsp.mkdir(good); - await expect(resolveCanonicalRoots([good, missing])).rejects.toBeInstanceOf(KaosError); - }); - - it('returns empty array for empty input', async () => { - expect(await resolveCanonicalRoots([])).toEqual([]); - }); -}); - -describe('assertPathInRoots', () => { - it('accepts a path inside the primary cwd root', async () => { - const cwd = path.join(scratch, 'cwd'); - await fsp.mkdir(cwd); - const inner = path.join(cwd, 'inside.txt'); - await fsp.writeFile(inner, 'hi'); - const roots = await resolveCanonicalRoots([cwd]); - await expect(assertPathInRoots(inner, roots, 'read')).resolves.toBeUndefined(); - }); - - it('accepts a path inside an additional directory root', async () => { - const cwd = path.join(scratch, 'cwd'); - const extra = path.join(scratch, 'extra'); - await fsp.mkdir(cwd); - await fsp.mkdir(extra); - const inner = path.join(extra, 'lib', 'x.ts'); - await fsp.mkdir(path.dirname(inner), { recursive: true }); - await fsp.writeFile(inner, 'hi'); - const roots = await resolveCanonicalRoots([cwd, extra]); - await expect(assertPathInRoots(inner, roots, 'read')).resolves.toBeUndefined(); - }); - - it('rejects a path that escapes via a symlinked directory', async () => { - const cwd = path.join(scratch, 'cwd'); - const outside = path.join(scratch, 'outside'); - await fsp.mkdir(cwd); - await fsp.mkdir(outside); - await fsp.writeFile(path.join(outside, 'secret.txt'), 'secret'); - // Plant the trap: cwd/escape -> outside - await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); - const roots = await resolveCanonicalRoots([cwd]); - await expect( - assertPathInRoots(path.join(cwd, 'escape', 'secret.txt'), roots, 'read'), - ).rejects.toBeInstanceOf(KaosError); - }); - - it('rejects a sibling path that merely sounds similar', async () => { - const cwd = path.join(scratch, 'cwd'); - await fsp.mkdir(cwd); - const sibling = path.join(scratch, 'cwd-evil', 'leak.txt'); - await fsp.mkdir(path.dirname(sibling), { recursive: true }); - await fsp.writeFile(sibling, 'leak'); - const roots = await resolveCanonicalRoots([cwd]); - // `scratch/cwd-evil` is NOT inside `scratch/cwd` after canonical - // resolution (different segment), so the prefix-startsWith check - // must catch it. - await expect(assertPathInRoots(sibling, roots, 'read')).rejects.toBeInstanceOf(KaosError); - }); - - it('rejects a write target whose parent is a symlink escape', async () => { - const cwd = path.join(scratch, 'cwd'); - const outside = path.join(scratch, 'outside'); - await fsp.mkdir(cwd); - await fsp.mkdir(outside); - await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); - const roots = await resolveCanonicalRoots([cwd]); - // `cwd/escape/new.txt` doesn't exist yet; assertPathInRoots must - // anchor at the realpath'd parent (= scratch/outside) and reject. - await expect( - assertPathInRoots(path.join(cwd, 'escape', 'new.txt'), roots, 'write'), - ).rejects.toBeInstanceOf(KaosError); - }); - - it('errors include the operation name in the message', async () => { - const cwd = path.join(scratch, 'cwd'); - const outside = path.join(scratch, 'outside'); - await fsp.mkdir(cwd); - await fsp.mkdir(outside); - await fsp.writeFile(path.join(outside, 'secret.txt'), 's'); - await fsp.symlink(outside, path.join(cwd, 'escape'), 'dir'); - const roots = await resolveCanonicalRoots([cwd]); - await expect( - assertPathInRoots(path.join(cwd, 'escape', 'secret.txt'), roots, 'write'), - ).rejects.toThrow(/write/i); - }); - - it('rejects a fully non-existent path that resolves outside the roots', async () => { - // The path `/totally/made/up/file.txt` doesn't exist on disk; the - // canonical-resolve walk anchors at `/` and the result still - // doesn't fall under `scratch/cwd` — boundary check fires. - const cwd = path.join(scratch, 'cwd'); - await fsp.mkdir(cwd); - const roots = await resolveCanonicalRoots([cwd]); - await expect( - assertPathInRoots('/totally/made/up/file.txt', roots, 'read'), - ).rejects.toBeInstanceOf(KaosError); - }); -}); From 9df47bc9cef67cdf25e58ba7f8b9d3934a409077 Mon Sep 17 00:00:00 2001 From: yzhou Date: Sun, 5 Jul 2026 12:18:12 +0800 Subject: [PATCH 5/9] fix(acp-adapter): normalize omitted additionalDirectories to [] on load/resume, reject null --- packages/acp-adapter/src/server.ts | 8 ++++---- packages/acp-adapter/test/server.test.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/acp-adapter/src/server.ts b/packages/acp-adapter/src/server.ts index b620bde72..1ac033191 100644 --- a/packages/acp-adapter/src/server.ts +++ b/packages/acp-adapter/src/server.ts @@ -359,7 +359,7 @@ export class AcpServer implements Agent { * deliberately skips it (per ACP spec G4 / plan gap-4.3). */ async loadSession(params: LoadSessionRequest): Promise { - const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories) ?? []; const { session, acpSession, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, @@ -397,7 +397,7 @@ export class AcpServer implements Agent { * rationale, and gap-4.1 for the matching capability advertisement. */ async resumeSession(params: ResumeSessionRequest): Promise { - const additionalDirs = validateAdditionalDirectories(params.additionalDirectories); + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories) ?? []; const { session, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, @@ -1112,14 +1112,14 @@ function sessionSummaryToSessionInfo(summary: SessionSummary): SessionInfo { * Validate the ACP `additionalDirectories` field per protocol spec. * * Returns the validated string array when the field is present, or - * `undefined` when the field is absent (`null` or `undefined`). + * `undefined` when the field is absent (`undefined`). * Throws {@link RequestError.invalidParams} for non-array values, * non-string entries, empty strings, or non-absolute paths. */ export function validateAdditionalDirectories( dirs: unknown, ): string[] | undefined { - if (dirs === undefined || dirs === null) return undefined; + if (dirs === undefined) return undefined; if (!Array.isArray(dirs)) { throw RequestError.invalidParams( { additionalDirectories: dirs }, diff --git a/packages/acp-adapter/test/server.test.ts b/packages/acp-adapter/test/server.test.ts index 43efc5982..f5968fbef 100644 --- a/packages/acp-adapter/test/server.test.ts +++ b/packages/acp-adapter/test/server.test.ts @@ -209,8 +209,8 @@ describe('validateAdditionalDirectories', () => { expect(validateAdditionalDirectories(undefined)).toBeUndefined(); }); - it('returns undefined when dirs is null', () => { - expect(validateAdditionalDirectories(null)).toBeUndefined(); + it('throws when dirs is null (present non-array value)', () => { + expect(() => validateAdditionalDirectories(null)).toThrow(); }); it('returns the array when dirs is a valid string array', () => { From dcb7faa49bb72baf0dd7c5164d7c752e8c64e965 Mon Sep 17 00:00:00 2001 From: yzhou Date: Sun, 5 Jul 2026 12:31:35 +0800 Subject: [PATCH 6/9] test(acp-adapter): update tests to expect [] instead of undefined for omitted additionalDirs --- packages/acp-adapter/test/session-load.test.ts | 2 +- packages/acp-adapter/test/session-resume.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/acp-adapter/test/session-load.test.ts b/packages/acp-adapter/test/session-load.test.ts index 8c1bdede2..5d37dc221 100644 --- a/packages/acp-adapter/test/session-load.test.ts +++ b/packages/acp-adapter/test/session-load.test.ts @@ -187,7 +187,7 @@ describe('AcpServer session/load replay', () => { expect(capturedResumeInputs).toHaveLength(1); expect( (capturedResumeInputs[0] as { additionalDirs?: unknown }).additionalDirs, - ).toBeUndefined(); + ).toEqual([]); }); it('replays a single assistant text-only turn as agent_message_chunk updates', async () => { diff --git a/packages/acp-adapter/test/session-resume.test.ts b/packages/acp-adapter/test/session-resume.test.ts index 3453d7879..a2426fc4d 100644 --- a/packages/acp-adapter/test/session-resume.test.ts +++ b/packages/acp-adapter/test/session-resume.test.ts @@ -196,7 +196,7 @@ describe('AcpServer.resumeSession', () => { expect(capturedResumeInputs).toHaveLength(1); expect( (capturedResumeInputs[0] as { additionalDirs?: unknown }).additionalDirs, - ).toBeUndefined(); + ).toEqual([]); }); it('returns configOptions matching the resumed session model + mode + thinking', async () => { From ae1f8506f631779e7c2722248b9bbf2772cc0059 Mon Sep 17 00:00:00 2001 From: liuzhaochen03 Date: Mon, 6 Jul 2026 11:01:07 +0800 Subject: [PATCH 7/9] feat: update summary --- packages/node-sdk/src/kimi-harness.ts | 9 +++++++-- packages/node-sdk/src/session.ts | 9 +++++++-- .../node-sdk/test/create-session-transport.test.ts | 14 ++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/node-sdk/src/kimi-harness.ts b/packages/node-sdk/src/kimi-harness.ts index 328b302a9..4c5d6019c 100644 --- a/packages/node-sdk/src/kimi-harness.ts +++ b/packages/node-sdk/src/kimi-harness.ts @@ -22,6 +22,7 @@ import type { ListSessionsOptions, RenameSessionInput, ResumeSessionInput, + ResumedSessionSummary, ReloadSessionInput, SessionSummary, TelemetryClient, @@ -118,10 +119,14 @@ export class KimiHarness { const active = this.activeSessions.get(id); const { kaos, persistenceKaos, sessionStartedProperties, ...resumeInput } = input; if (active !== undefined) { + let summary: ResumedSessionSummary | undefined; if (kaos !== undefined || persistenceKaos !== undefined) { - await this.rpc.resumeSessionWithKaos({ ...resumeInput, id }, kaos ?? persistenceKaos as Kaos, persistenceKaos); + summary = await this.rpc.resumeSessionWithKaos({ ...resumeInput, id }, kaos ?? persistenceKaos as Kaos, persistenceKaos); } else if (resumeInput.additionalDirs !== undefined) { - await this.rpc.resumeSession({ ...resumeInput, id }); + summary = await this.rpc.resumeSession({ ...resumeInput, id }); + } + if (summary !== undefined) { + active.updateSummary(summary); } return active; } diff --git a/packages/node-sdk/src/session.ts b/packages/node-sdk/src/session.ts index 3e28ad394..b7b4e5440 100644 --- a/packages/node-sdk/src/session.ts +++ b/packages/node-sdk/src/session.ts @@ -71,14 +71,19 @@ export class Session { return this.resumeState; } + updateSummary(summary: ResumedSessionSummary): void { + this.ensureOpen(); + this.summary = summary; + this.resumeState = resumeStateFromSummary(summary); + } + async reloadSession(options?: ReloadSessionOptions): Promise { this.ensureOpen(); const summary = await this.rpc.reloadSession({ sessionId: this.id, forcePluginSessionStartReminder: options?.forcePluginSessionStartReminder, }); - this.summary = summary; - this.resumeState = resumeStateFromSummary(summary); + this.updateSummary(summary); return summary; } diff --git a/packages/node-sdk/test/create-session-transport.test.ts b/packages/node-sdk/test/create-session-transport.test.ts index 3be850c25..9a0921e12 100644 --- a/packages/node-sdk/test/create-session-transport.test.ts +++ b/packages/node-sdk/test/create-session-transport.test.ts @@ -710,6 +710,13 @@ effort = "medium" const resumed = await harness.resumeSession({ id: session.id, kaos }); expect(resumed).toBe(session); + expect(resumed.summary?.additionalDirs).toEqual([]); + expect(resumed.getResumeState()).toMatchObject({ + sessionMetadata: { + title: '', + }, + agents: {}, + }); expect(rpc.resumeCalls).toHaveLength(1); expect(rpc.resumeCalls[0]).toMatchObject({ input: { id: 'ses_active' }, @@ -738,6 +745,13 @@ effort = "medium" }); expect(resumed).toBe(session); + expect(resumed.summary?.additionalDirs).toEqual(['/tmp/extra']); + expect(resumed.getResumeState()).toMatchObject({ + sessionMetadata: { + title: '', + }, + agents: {}, + }); expect(rpc.plainResumeCalls).toEqual([ { id: 'ses_active_dirs', From b2d8516f13a2bddb6f9a1a1d5c45c8607e7a5522 Mon Sep 17 00:00:00 2001 From: liuzhaochen03 Date: Mon, 6 Jul 2026 11:32:21 +0800 Subject: [PATCH 8/9] chore: update changeset --- .changeset/refresh-sdk-resume-summary.md | 5 +++++ .changeset/remove-acp-path-boundary.md | 5 ----- .changeset/support-acp-additional-directories.md | 5 +++++ 3 files changed, 10 insertions(+), 5 deletions(-) create mode 100644 .changeset/refresh-sdk-resume-summary.md delete mode 100644 .changeset/remove-acp-path-boundary.md create mode 100644 .changeset/support-acp-additional-directories.md diff --git a/.changeset/refresh-sdk-resume-summary.md b/.changeset/refresh-sdk-resume-summary.md new file mode 100644 index 000000000..1494905c7 --- /dev/null +++ b/.changeset/refresh-sdk-resume-summary.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code-sdk": patch +--- + +Apply additional session root updates and refresh active resumed session summaries. diff --git a/.changeset/remove-acp-path-boundary.md b/.changeset/remove-acp-path-boundary.md deleted file mode 100644 index ab7cef33d..000000000 --- a/.changeset/remove-acp-path-boundary.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@moonshot-ai/acp-adapter": patch ---- - -Remove the per-operation path boundary enforcement from AcpKaos while retaining the additionalDirectories input validation. diff --git a/.changeset/support-acp-additional-directories.md b/.changeset/support-acp-additional-directories.md new file mode 100644 index 000000000..bb0bd07f3 --- /dev/null +++ b/.changeset/support-acp-additional-directories.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/acp-adapter": patch +--- + +Advertise and forward ACP additionalDirectories when creating, loading, and resuming sessions. From b09636f46c3408a4965017cd70e1470dc4d157e2 Mon Sep 17 00:00:00 2001 From: Kevin Date: Mon, 6 Jul 2026 12:27:00 +0800 Subject: [PATCH 9/9] fix(acp-adapter): preserve auth_required before validating extra roots When an unauthenticated client sends session/load or session/resume with a malformed additionalDirectories value, this validation previously threw invalid_params before setupSessionFromExisting could run the existing auth gate. This changes the unauthenticated error to an auth_required error for these inputs. --- .changeset/fix-acp-auth-order.md | 5 +++++ packages/acp-adapter/src/server.ts | 11 +++++------ 2 files changed, 10 insertions(+), 6 deletions(-) create mode 100644 .changeset/fix-acp-auth-order.md diff --git a/.changeset/fix-acp-auth-order.md b/.changeset/fix-acp-auth-order.md new file mode 100644 index 000000000..d2b59e55e --- /dev/null +++ b/.changeset/fix-acp-auth-order.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix ACP session load and resume returning parameter errors before authentication errors on malformed directories. diff --git a/packages/acp-adapter/src/server.ts b/packages/acp-adapter/src/server.ts index 1ac033191..4b850911b 100644 --- a/packages/acp-adapter/src/server.ts +++ b/packages/acp-adapter/src/server.ts @@ -359,12 +359,11 @@ export class AcpServer implements Agent { * deliberately skips it (per ACP spec G4 / plan gap-4.3). */ async loadSession(params: LoadSessionRequest): Promise { - const additionalDirs = validateAdditionalDirectories(params.additionalDirectories) ?? []; const { session, acpSession, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, mcpServers: params.mcpServers, - additionalDirs, + additionalDirectories: params.additionalDirectories, mode: 'load', }); // Synchronously replay history — the response must not settle @@ -397,12 +396,11 @@ export class AcpServer implements Agent { * rationale, and gap-4.1 for the matching capability advertisement. */ async resumeSession(params: ResumeSessionRequest): Promise { - const additionalDirs = validateAdditionalDirectories(params.additionalDirectories) ?? []; const { session, configOptions } = await this.setupSessionFromExisting({ cwd: params.cwd, sessionId: params.sessionId, mcpServers: params.mcpServers, - additionalDirs, + additionalDirectories: params.additionalDirectories, mode: 'resume', }); this.scheduleAvailableCommandsUpdate(session.id); @@ -436,7 +434,7 @@ export class AcpServer implements Agent { cwd: string; sessionId: string; mcpServers?: ReadonlyArray; - additionalDirs?: readonly string[]; + additionalDirectories?: unknown; mode: 'load' | 'resume'; }): Promise<{ session: Session; @@ -446,6 +444,7 @@ export class AcpServer implements Agent { if (!(await harnessIsAuthed(this.harness))) { throw RequestError.authRequired(); } + const additionalDirs = validateAdditionalDirectories(params.additionalDirectories) ?? []; if (!this.conn) { throw RequestError.internalError(undefined, 'AcpServer is missing its AgentSideConnection'); } @@ -467,7 +466,7 @@ export class AcpServer implements Agent { id: params.sessionId, kaos: acpKaos, persistenceKaos, - additionalDirs: params.additionalDirs, + additionalDirs, sessionStartedProperties: { mode: params.mode }, // @ts-expect-error — see block comment above; mcpServers is a // kernel-only field that the SDK forwards via spread.