mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-08-23 15:46:26 +00:00
fix(acp-server): restore local execution fallback, stdio MCP, and session reload (#3183)
* fix(acp-server): restore local process execution fallback * fix(acp-server): accept stdio MCP servers in ACP sessions * fix(agent-core-v2): allow runtime re-registration after removal * chore: add acp regression fixes changeset * fix(agent-core-v2): prune staged runtime entries when removal teardown fails
This commit is contained in:
parent
368b4b7400
commit
2adc6a1c6e
10 changed files with 209 additions and 31 deletions
5
.changeset/fix-acp-execution-regressions.md
Normal file
5
.changeset/fix-acp-execution-regressions.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -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<IHostProcess> {
|
||||
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<RuntimeProviderAttachment> {
|
||||
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 () => {
|
||||
|
|
|
|||
|
|
@ -176,7 +176,14 @@ export function acpMcpServersToConfigRecord(
|
|||
const out: Record<string, McpServerConfig> = {};
|
||||
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] = {
|
||||
|
|
|
|||
|
|
@ -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<string, string>();
|
||||
server = new AcpServer(client, klient, acpConnection, {
|
||||
|
|
|
|||
|
|
@ -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> = {}): IHostEnvironment {
|
||||
return {
|
||||
_serviceBrand: undefined,
|
||||
|
|
@ -41,7 +64,10 @@ function makeEnvironment(overrides: Partial<IHostEnvironment> = {}): IHostEnviro
|
|||
} as IHostEnvironment;
|
||||
}
|
||||
|
||||
async function bindRuntime(environment: IHostEnvironment): Promise<Runtime> {
|
||||
async function bindRuntime(
|
||||
environment: IHostEnvironment,
|
||||
options: { connection?: IAcpConnection; local?: IHostProcessService } = {},
|
||||
): Promise<Runtime> {
|
||||
const runtimes: Runtime[] = [];
|
||||
const host = {
|
||||
registerRuntime: (runtime: Runtime) => {
|
||||
|
|
@ -49,7 +75,11 @@ async function bindRuntime(environment: IHostEnvironment): Promise<Runtime> {
|
|||
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' } });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
},
|
||||
|
|
|
|||
|
|
@ -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[] = [];
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue