diff --git a/.changeset/fix-acp-execution-regressions.md b/.changeset/fix-acp-execution-regressions.md new file mode 100644 index 000000000..713501caa --- /dev/null +++ b/.changeset/fix-acp-execution-regressions.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix ACP session regressions: Bash, Grep, and Glob failing when the editor does not support terminal command execution, session creation failing with stdio MCP servers, and reopening a closed session failing with an internal error. diff --git a/packages/acp-server/src/acp-terminal/acpTerminalRunner.ts b/packages/acp-server/src/acp-terminal/acpTerminalRunner.ts index ec70df180..9016d48b6 100644 --- a/packages/acp-server/src/acp-terminal/acpTerminalRunner.ts +++ b/packages/acp-server/src/acp-terminal/acpTerminalRunner.ts @@ -47,6 +47,7 @@ class AcpProcessService implements IHostProcessService { private readonly sessionId: string, private readonly cwd: string, private readonly connection: IAcpConnection, + private readonly local: IHostProcessService, ) {} async spawn( @@ -54,11 +55,8 @@ class AcpProcessService implements IHostProcessService { args: readonly string[] = [], options?: HostProcessOptions, ): Promise { - if (!this.connection.terminalEnabled) { - throw new Error('ACP terminal capability is unavailable'); - } - if (!isBashToolInvocation(args, options)) { - throw new Error('ACP runtime only supports interactive Bash tool processes'); + if (!this.connection.terminalEnabled || !isBashToolInvocation(args, options)) { + return this.local.spawn(command, args, { ...options, cwd: options?.cwd ?? this.cwd }); } const handle = await this.connection.get().createTerminal({ @@ -178,6 +176,7 @@ class AcpSessionRuntime implements Runtime { cwd: string, connection: IAcpConnection, environment: IHostEnvironment, + local: IHostProcessService, ) { this.identity = { workspaceId, @@ -205,7 +204,7 @@ class AcpSessionRuntime implements Runtime { dirname: (p: string) => path.dirname(p), }; this.fs = new AcpHostFileSystem({ sessionId } as unknown as ISessionContext, connection); - this.process = new AcpProcessService(sessionId, cwd, connection); + this.process = new AcpProcessService(sessionId, cwd, connection, local); } dispose(): void {} @@ -219,13 +218,14 @@ class AcpWorkspaceRuntimeAttachment implements RuntimeProviderAttachment { private readonly host: RuntimeProviderHost, private readonly connection: IAcpConnection, private readonly environment: IHostEnvironment, + private readonly local: IHostProcessService, ) {} bindSession(sessionId: string, cwd: string): string { const runtimeId = AcpRuntimeProviderFactory.runtimeId(sessionId); if (this.sessions.has(sessionId)) return runtimeId; const registration = this.host.registerRuntime( - new AcpSessionRuntime(this.workspace.id, sessionId, cwd, this.connection, this.environment), + new AcpSessionRuntime(this.workspace.id, sessionId, cwd, this.connection, this.environment, this.local), ); this.sessions.set(sessionId, registration); return runtimeId; @@ -253,6 +253,7 @@ export class AcpRuntimeProviderFactory implements RuntimeProviderFactory { constructor( private readonly connection: IAcpConnection, private readonly environment: IHostEnvironment, + private readonly local: IHostProcessService, ) {} static runtimeId(sessionId: string): string { @@ -260,7 +261,7 @@ export class AcpRuntimeProviderFactory implements RuntimeProviderFactory { } async attach(workspace: RuntimeProviderContext, host: RuntimeProviderHost): Promise { - const attachment = new AcpWorkspaceRuntimeAttachment(workspace, host, this.connection, this.environment); + const attachment = new AcpWorkspaceRuntimeAttachment(workspace, host, this.connection, this.environment, this.local); this.attachments.set(workspace.id, attachment); return { dispose: async () => { diff --git a/packages/acp-server/src/convert.ts b/packages/acp-server/src/convert.ts index 308d400a0..48c34bff6 100644 --- a/packages/acp-server/src/convert.ts +++ b/packages/acp-server/src/convert.ts @@ -176,7 +176,14 @@ export function acpMcpServersToConfigRecord( const out: Record = {}; for (const server of servers) { if (!('type' in server)) { - throw new Error(`ACP stdio MCP server ${server.name} does not declare a runtime identity`); + out[server.name] = { + transport: 'stdio', + command: server.command, + args: server.args, + env: namedPairsToRecord(server.env), + runtime_id: 'local', + }; + continue; } if (server.type === 'http' || server.type === 'sse') { out[server.name] = { diff --git a/packages/acp-server/src/start.ts b/packages/acp-server/src/start.ts index 6cf189017..5b60cac57 100644 --- a/packages/acp-server/src/start.ts +++ b/packages/acp-server/src/start.ts @@ -26,6 +26,7 @@ import { IAgentRuntimeBindingService, IAppendLogStore, IHostEnvironment, + IHostProcessService, ISessionContext, ISessionIndexMirror, IWorkspaceInstanceManager, @@ -142,7 +143,7 @@ export async function runAcpServerWithStream( // `IAcpConnection.get()`. acpConnection.bind(client); const workspaceManager = core.accessor.get(IWorkspaceInstanceManager); - const acpRuntimeProvider = new AcpRuntimeProviderFactory(acpConnection, core.accessor.get(IHostEnvironment)); + const acpRuntimeProvider = new AcpRuntimeProviderFactory(acpConnection, core.accessor.get(IHostEnvironment), core.accessor.get(IHostProcessService)); const acpProviderRegistration = await workspaceManager.addProvider(acpRuntimeProvider); const sessionWorkspaces = new Map(); server = new AcpServer(client, klient, acpConnection, { diff --git a/packages/acp-server/test/acp-terminal.test.ts b/packages/acp-server/test/acp-terminal.test.ts index 675e58c1f..30b83d19f 100644 --- a/packages/acp-server/test/acp-terminal.test.ts +++ b/packages/acp-server/test/acp-terminal.test.ts @@ -1,24 +1,29 @@ import { describe, expect, it } from 'vitest'; import type { + HostProcessOptions, IHostEnvironment, + IHostProcess, + IHostProcessService, Runtime, RuntimeProviderHost, } from '@moonshot-ai/agent-core-v2'; -import type { IAcpConnection } from '../src/acp-fs/acpConnection'; +import type { IAcpConnection, IAcpTerminalHandle } from '../src/acp-fs/acpConnection'; import { AcpHostFileSystem } from '../src/acp-fs/acpFsService'; import { AcpRuntimeProviderFactory } from '../src/acp-terminal/acpTerminalRunner'; -function makeConnection(): IAcpConnection { +function makeConnection( + options: { terminalEnabled?: boolean; createTerminal?: () => IAcpTerminalHandle } = {}, +): IAcpConnection { return { _serviceBrand: undefined, bound: true, fsReadTextFile: true, fsWriteTextFile: true, - terminalEnabled: true, + terminalEnabled: options.terminalEnabled ?? true, bind: () => {}, - get: () => ({}) as never, + get: () => ({ createTerminal: async () => options.createTerminal?.() }) as never, bindFsCapabilities: () => {}, bindTerminalCapability: () => {}, notifyTerminalCreated: () => {}, @@ -26,6 +31,24 @@ function makeConnection(): IAcpConnection { }; } +interface LocalSpawnCall { + readonly command: string; + readonly args: readonly string[]; + readonly options: HostProcessOptions | undefined; +} + +function makeLocalProcessService(): { local: IHostProcessService; calls: LocalSpawnCall[] } { + const calls: LocalSpawnCall[] = []; + const local: IHostProcessService = { + _serviceBrand: undefined, + spawn: async (command, args = [], options) => { + calls.push({ command, args, options }); + return {} as IHostProcess; + }, + }; + return { local, calls }; +} + function makeEnvironment(overrides: Partial = {}): IHostEnvironment { return { _serviceBrand: undefined, @@ -41,7 +64,10 @@ function makeEnvironment(overrides: Partial = {}): IHostEnviro } as IHostEnvironment; } -async function bindRuntime(environment: IHostEnvironment): Promise { +async function bindRuntime( + environment: IHostEnvironment, + options: { connection?: IAcpConnection; local?: IHostProcessService } = {}, +): Promise { const runtimes: Runtime[] = []; const host = { registerRuntime: (runtime: Runtime) => { @@ -49,7 +75,11 @@ async function bindRuntime(environment: IHostEnvironment): Promise { return { remove: async () => {} }; }, } as unknown as RuntimeProviderHost; - const factory = new AcpRuntimeProviderFactory(makeConnection(), environment); + const factory = new AcpRuntimeProviderFactory( + options.connection ?? makeConnection(), + environment, + options.local ?? makeLocalProcessService().local, + ); await factory.attach({ id: 'w1' } as never, host); factory.bindSession('w1', 's1', '/repo'); const runtime = runtimes[0]; @@ -98,3 +128,69 @@ describe('AcpSessionRuntime', () => { expect(runtime.path.resolve('C:\\repo', 'src')).toBe('C:\\repo\\src'); }); }); + +describe('AcpProcessService local fallback', () => { + const bashEnv = { NO_COLOR: '1', TERM: 'dumb' }; + + function makeTerminalHandle(): IAcpTerminalHandle { + return { + id: 'term-1', + currentOutput: async () => ({ output: '', truncated: false }), + waitForExit: async () => ({ exitCode: 0 }), + kill: async () => ({}), + release: async () => ({}), + }; + } + + it('runs Bash-shaped spawns in the client terminal when the capability is advertised', async () => { + let created = 0; + const connection = makeConnection({ + terminalEnabled: true, + createTerminal: () => { + created += 1; + return makeTerminalHandle(); + }, + }); + const { local, calls } = makeLocalProcessService(); + const runtime = await bindRuntime(makeEnvironment(), { connection, local }); + + await runtime.process!.spawn('/bin/bash', ['-c', 'echo hi'], { env: { ...bashEnv } }); + + expect(created).toBe(1); + expect(calls).toHaveLength(0); + }); + + it('falls back to local execution for Bash-shaped spawns without the terminal capability', async () => { + const connection = makeConnection({ terminalEnabled: false }); + const { local, calls } = makeLocalProcessService(); + const runtime = await bindRuntime(makeEnvironment(), { connection, local }); + + await runtime.process!.spawn('/bin/bash', ['-c', 'echo hi'], { env: { ...bashEnv } }); + + expect(calls).toHaveLength(1); + expect(calls[0]).toMatchObject({ + command: '/bin/bash', + args: ['-c', 'echo hi'], + options: { env: bashEnv, cwd: '/repo' }, + }); + }); + + it('falls back to local execution for non-Bash spawns even with the terminal capability', async () => { + let created = 0; + const connection = makeConnection({ + terminalEnabled: true, + createTerminal: () => { + created += 1; + return makeTerminalHandle(); + }, + }); + const { local, calls } = makeLocalProcessService(); + const runtime = await bindRuntime(makeEnvironment(), { connection, local }); + + await runtime.process!.spawn('rg', ['--files', '--hidden']); + + expect(created).toBe(0); + expect(calls).toHaveLength(1); + expect(calls[0]).toMatchObject({ command: 'rg', args: ['--files', '--hidden'], options: { cwd: '/repo' } }); + }); +}); diff --git a/packages/acp-server/test/convert.test.ts b/packages/acp-server/test/convert.test.ts index c5a1c490c..5dd8482d2 100644 --- a/packages/acp-server/test/convert.test.ts +++ b/packages/acp-server/test/convert.test.ts @@ -19,7 +19,7 @@ describe('acpMcpServersToConfigRecord', () => { expect(acpMcpServersToConfigRecord([])).toBeUndefined(); }); - it('rejects stdio servers that cannot declare a runtime identity', () => { + it('maps stdio servers (no type field) to local stdio configs', () => { const servers: McpServer[] = [ { name: 'fs', @@ -31,9 +31,15 @@ describe('acpMcpServersToConfigRecord', () => { ], }, ]; - expect(() => acpMcpServersToConfigRecord(servers)).toThrow( - 'ACP stdio MCP server fs does not declare a runtime identity', - ); + expect(acpMcpServersToConfigRecord(servers)).toEqual({ + fs: { + transport: 'stdio', + command: '/usr/local/bin/mcp-fs', + args: ['--root', '/tmp'], + env: { API_KEY: 'secret', DEBUG: '1' }, + runtime_id: 'local', + }, + }); }); it('maps http and sse servers with header pairs as a record', () => { diff --git a/packages/acp-server/test/e2e-turn.test.ts b/packages/acp-server/test/e2e-turn.test.ts index 7202878dc..db4c1e937 100644 --- a/packages/acp-server/test/e2e-turn.test.ts +++ b/packages/acp-server/test/e2e-turn.test.ts @@ -991,7 +991,7 @@ describe('acp-server terminal reverse-RPC (clientCapabilities.terminal)', () => expect(JSON.stringify(secondCall)).toContain('hello_from_terminal'); }, 30_000); - it('rejects Bash without falling back when the client does not advertise terminal capability', async () => { + it('falls back to local execution when the client does not advertise the capability', async () => { const c = await boot({}); const terminals = fakeTerminalClient(c, 'should_not_be_used\n'); scriptBashTurn('echo hello_from_bash'); @@ -999,7 +999,7 @@ describe('acp-server terminal reverse-RPC (clientCapabilities.terminal)', () => const { stopReason } = await runPrompt(c); expect(stopReason).toBe('end_turn'); - // No terminal reverse-RPC at all — behavior identical to today. + // No terminal reverse-RPC at all — the command ran locally. expect(terminals).toHaveLength(0); const terminalRpcs = c.received.filter( (m) => typeof m.method === 'string' && m.method.startsWith('terminal/'), @@ -1009,7 +1009,6 @@ describe('acp-server terminal reverse-RPC (clientCapabilities.terminal)', () => // The tool card carries the textual output, exactly as before. const completed = toolCallUpdates(c).find((u) => u.status === 'completed'); const text = completed?.content?.map((entry) => entry.content?.text ?? '').join('\n') ?? ''; - expect(text).not.toContain('hello_from_bash'); - expect(JSON.stringify(scripted!.callHistory()[1])).toContain('ACP terminal capability is unavailable'); + expect(text).toContain('hello_from_bash'); }, 30_000); }); diff --git a/packages/acp-server/test/lifecycle.test.ts b/packages/acp-server/test/lifecycle.test.ts index fe124ca8d..5b505203f 100644 --- a/packages/acp-server/test/lifecycle.test.ts +++ b/packages/acp-server/test/lifecycle.test.ts @@ -291,10 +291,10 @@ describe('acp-server session lifecycle', () => { ); it( - 'session/new rejects stdio MCP servers without runtime identity', + 'session/new connects ACP mcpServers as ephemeral session servers', async () => { const c = await boot(); - await expect(c.send('session/new', { + const created = (await c.send('session/new', { cwd: homeDir, mcpServers: [ { @@ -304,16 +304,19 @@ describe('acp-server session lifecycle', () => { env: [{ name: 'KIMI_TEST_MCP_START_DELAY_MS', value: '0' }], }, ], - })).rejects.toThrow('ACP stdio MCP server mock does not declare a runtime identity'); + })) as { sessionId: string }; + expect(created.sessionId).toMatch(/^session_/); // Engine-side assertion: the session scope's MCP handle is the overlay // view and the converted server ended up connected under its ACP name. + const entries = await sessionMcpEntries(c, created.sessionId); + expect(entries.find((e) => e.name === 'mock')?.status).toBe('connected'); }, 30_000, ); it( - 'session/load rejects stdio MCP servers without runtime identity', + 'session/load forwards mcpServers to the re-materialized session', async () => { const c = await boot(); const created = (await c.send('session/new', { cwd: homeDir, mcpServers: [] })) as { @@ -321,13 +324,16 @@ describe('acp-server session lifecycle', () => { }; await c.send('session/close', { sessionId: created.sessionId }); - await expect(c.send('session/load', { + await c.send('session/load', { sessionId: created.sessionId, cwd: homeDir, mcpServers: [ { name: 'mock', command: process.execPath, args: [STDIO_MCP_FIXTURE], env: [] }, ], - })).rejects.toThrow('ACP stdio MCP server mock does not declare a runtime identity'); + }); + + const entries = await sessionMcpEntries(c, created.sessionId); + expect(entries.find((e) => e.name === 'mock')?.status).toBe('connected'); }, 30_000, ); diff --git a/packages/agent-core-v2/src/runtime/runtimeUnitHost.ts b/packages/agent-core-v2/src/runtime/runtimeUnitHost.ts index 9554fbff4..2d750f658 100644 --- a/packages/agent-core-v2/src/runtime/runtimeUnitHost.ts +++ b/packages/agent-core-v2/src/runtime/runtimeUnitHost.ts @@ -304,7 +304,14 @@ class SharedRuntimeUnitHost implements RuntimeUnitHost { const handle: RuntimeProviderRuntimeHandle = { runtimeId: runtime.identity.runtimeId, update: (replacement) => this.updateRuntime(staged, replacement), - remove: () => this.removeRuntime(staged), + remove: async () => { + try { + await this.removeRuntime(staged); + } finally { + const index = runtimes.indexOf(staged); + if (index >= 0) runtimes.splice(index, 1); + } + }, }; return handle; }, diff --git a/packages/agent-core-v2/test/runtime/runtimeUnitHost.test.ts b/packages/agent-core-v2/test/runtime/runtimeUnitHost.test.ts index 9b9eec0e4..9bcbbc542 100644 --- a/packages/agent-core-v2/test/runtime/runtimeUnitHost.test.ts +++ b/packages/agent-core-v2/test/runtime/runtimeUnitHost.test.ts @@ -225,6 +225,56 @@ describe('RuntimeUnitHost', () => { disposables.dispose(); }); + it('re-registers the same runtime id after its registration was removed', async () => { + const { disposables, host, registry } = setup(); + let providerHost!: RuntimeProviderHost; + const handle = await host.provide(emptyImports(), async (provider) => { + providerHost = provider; + return { dispose: () => {} }; + }); + + const first = runtime('one'); + const registration = providerHost.registerRuntime(first); + await registration.remove(); + expect(registry.current('local')).toBeUndefined(); + + const second = runtime('two'); + providerHost.registerRuntime(second); + expect(registry.current('local')).toBe(second); + + await handle.remove(); + expect(registry.current('local')).toBeUndefined(); + expect(second.disposed).toBe(true); + await host.dispose(); + disposables.dispose(); + }); + + it('re-registers the same runtime id even when removal teardown fails', async () => { + const { disposables, host, registry } = setup(); + let providerHost!: RuntimeProviderHost; + const handle = await host.provide(emptyImports(), async (provider) => { + providerHost = provider; + return { dispose: () => {} }; + }); + + const failing = runtime('one'); + failing.dispose = () => { + throw new Error('boom'); + }; + const registration = providerHost.registerRuntime(failing); + await expect(registration.remove()).rejects.toThrow('boom'); + expect(registry.current('local')).toBeUndefined(); + + const second = runtime('two'); + providerHost.registerRuntime(second); + expect(registry.current('local')).toBe(second); + + await handle.remove(); + expect(registry.current('local')).toBeUndefined(); + await host.dispose(); + disposables.dispose(); + }); + it('waits for in-flight prepare, rejects new transactions, and tears down in reverse order', async () => { const { disposables, host } = setup(); const order: string[] = [];