feat(acp): add agent-core-v2 ACP server#1586
Conversation
- add ACP session lifecycle, configuration, permissions, and event bridging - expose the experimental kimi acp-v2 command with terminal authentication - add integration coverage and workspace build configuration
|
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dfa11e5da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async readText(path: string, _options?: ReadTextOptions): Promise<string> { | ||
| // ACP `fs.readTextFile` returns already-decoded UTF-8 text, so the | ||
| // `encoding`/`errors` decode options are a no-op here. | ||
| const { content } = await this.connection | ||
| .get() | ||
| .readTextFile({ sessionId: this.ctx.sessionId, path }); |
There was a problem hiding this comment.
When an ACP client initializes without clientCapabilities.fs.readTextFile / writeTextFile (or explicitly sets them false), this session-scoped filesystem still replaces local text IO and unconditionally sends fs/read_text_file and fs/write_text_file reverse-RPCs. That makes normal Read/Edit/Write operations fail with method-not-found for clients that do not advertise the FS extension; use the stored client capabilities to install this backend only when supported, or fall back to the inner local filesystem per operation.
Useful? React with 👍 / 👎.
| return { | ||
| protocolVersion: params.protocolVersion, |
There was a problem hiding this comment.
If a client sends a newer or otherwise unsupported protocolVersion (for example 99), this echoes that value back even though this server only implements the current ACP 0.23/negotiation version 1 surface. The client can then assume semantics this server does not support; mirror the existing ACP adapter behavior by returning the highest supported version instead of the raw request value.
Useful? React with 👍 / 👎.
| mcpCapabilities: { | ||
| http: true, | ||
| sse: true, | ||
| }, |
There was a problem hiding this comment.
Advertising mcpCapabilities tells ACP clients they can pass configured MCP servers in session/new, session/load, or session/resume, but this server never reads params.mcpServers and only creates/resumes the v2 session with its cwd/id. In clients that rely on ACP-provided MCP config, the UI will think MCP is supported while none of those tools are connected; either remove these capabilities for now or convert and forward the requested servers into the v2 session creation/resume path.
Useful? React with 👍 / 👎.
| const acpSession = await this.wireSession(handle, params.sessionId); | ||
| this.sessions.set(params.sessionId, acpSession); |
There was a problem hiding this comment.
When session/load is called for a session already present in this.sessions (and session/resume has the same pattern below), resumeHandle returns the live scope and wireSession creates a new AcpInteractionBridge, but the previous wrapper is overwritten without dispose(). Subsequent approval/question interactions are observed by both bridges, causing duplicate permission prompts and leaving the stale subscription alive until the underlying session closes.
Useful? React with 👍 / 👎.
- replace placeholder hostnames (evil.com, foo.com, internal.corp, real.corp) with example.test / example.com in agent-core-v2 and kap-server tests - replace fixture emails (x@y.com, a@x.com) with example addresses in minidb tests and README
…review - add missing appendText to AcpHostFileSystem (IHostFileSystem drift) - replace IAgentPromptService.prompt with inject - use Turn.cancel() instead of abortController - gate FS reverse-RPCs on client capabilities, fallback to local FS - return PROTOCOL_VERSION constant instead of echoing client version - remove misleading mcpCapabilities from initialize response - dispose old session wrapper before replacing on load/resume - fix object stringification lint error in convert.ts - add acp-v2 to expected CLI sub-command list in test
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bcb4e00a8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return this.driveTurn(() => | ||
| this.mainAgent.accessor.get(IAgentPromptService).inject(message), | ||
| ); |
There was a problem hiding this comment.
For a normal session/prompt, using IAgentPromptService.inject() takes the steering path instead of the public prompt submission path used by the other v2 surfaces. In configurations with UserPromptSubmit/external prompt hooks, those hooks are registered on onBeforeSubmitPrompt and run from enqueue(), so ACP v2 prompts can bypass prompt-blocking policy and can also steer into an active turn instead of launching/queuing a discrete user turn.
Useful? React with 👍 / 👎.
| registerScopedService( | ||
| LifecycleScope.Session, | ||
| IHostFileSystem, |
There was a problem hiding this comment.
This Session-scoped IHostFileSystem override does not cover the Edit tool: EditTool delegates the actual read/write to the App-scoped IFileEditService, whose IHostFileSystem dependency is resolved at App scope and therefore remains the local HostFileSystem. With an ACP client that advertises fs.readTextFile/fs.writeTextFile, Edit will still read and mutate local disk instead of the editor/remote buffer, so unsaved client contents are ignored and edits may land in the wrong place.
Useful? React with 👍 / 👎.
| appendText(path: string, data: string): Promise<void> { | ||
| return this.inner.appendText(path, data); | ||
| } |
There was a problem hiding this comment.
When the client supports ACP writes, overwrite mode goes through fs.writeTextFile, but append mode from the v2 Write tool calls appendText() and this always delegates to the local filesystem. In ACP sessions backed by editor buffers or remote workspaces, Write with mode: 'append' will not update the client-visible file and may instead mutate or fail against local disk; append needs to be emulated through ACP read/write or rejected when it cannot be represented.
Useful? React with 👍 / 👎.
| name: skill.name, | ||
| description: skill.description, | ||
| })); | ||
| return [...ACP_BUILTIN_SLASH_COMMANDS, ...skills]; |
There was a problem hiding this comment.
This advertises /help, /status, /compact, and the other ACP builtins to clients, but prompt() only handles skill slash commands and explicitly lets builtin commands fall through as a normal model prompt. Any client command-palette selection for those advertised commands will be sent to the LLM instead of producing the local command response, so either handle the builtin intents before launching a turn or omit them from available_commands_update until they are implemented.
Useful? React with 👍 / 👎.
…mented builtins - replace IAgentPromptService.inject with enqueue so onBeforeSubmitPrompt hooks (prompt-blocking policy) are not bypassed - stop advertising builtin slash commands (/help, /status, etc.) until builtin command execution is implemented - add comment explaining appendText stays local (ACP has no append RPC) - update skills test to match new availableCommands behavior
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5f40ae790
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case 'turn.ended': | ||
| settle(() => | ||
| resolve({ stopReason: turnEndReasonToStopReason(event.reason, event.error) }), | ||
| ); |
There was a problem hiding this comment.
When a second session/prompt arrives while another turn is already running, IAgentPromptService.enqueue() returns a handle whose launched promise waits in the queue, but this subscription is already active and settles on the currently-running turn's turn.ended. That makes the queued prompt return early (and duplicates the active turn's updates), then its actual turn later runs with no ACP subscriber. Track the turnId returned by launch() and ignore events from any other turn until the queued prompt has actually launched.
Useful? React with 👍 / 👎.
| case 'failed': | ||
| if (error?.code === 'provider.filtered') return 'refusal'; | ||
| return 'end_turn'; |
There was a problem hiding this comment.
For provider/OAuth failures after a session is created (for example a revoked token causing agent-core-v2 to emit turn.ended with reason: 'failed' and error.code such as provider.auth_error or auth.login_required), this path resolves the prompt with stopReason: 'end_turn'. ACP clients only trigger their re-auth flow when the request rejects with auth_required, so these expired-token prompts look like successful empty turns and the user is never prompted to log in again.
Useful? React with 👍 / 👎.
…_required - track turnId in driveTurn and ignore events from unrelated turns, preventing queued prompts from settling on the running turn - reject prompt requests with auth_required when turn fails with an auth-related error code, enabling ACP client re-auth flow
1 participant
Related Issue
No linked issue — the problem is explained below.
Problem
The existing
kimi acpcommand serves ACP clients through the legacy@moonshot-ai/acp-adapteron top of the SDK harness and the legacyagent-core. The new DI × Scope agent engine (agent-core-v2) had no ACP entry point, so ACP-compatible clients (editors, IDEs) could not drive a session on the new engine.What changed
This PR adds
@moonshot-ai/acp-server, an ACP host backed directly byagent-core-v2, plus an experimentalkimi acp-v2CLI command to launch it over stdio.1. ACP server on agent-core-v2 (
packages/acp-server)Problem: No ACP host existed for the v2 engine.
What was done:
session.ts,server.ts,lifecycle) mapping ACP session/prompt/close semantics onto v2 sessions, including replay support for session resumption.events-map.ts,interaction-bridge.ts) translating v2 engine events into ACP session updates (streaming, tool calls, questions).approval.ts) and question handling (question.ts) surfaced to ACP clients.config-options.ts,modes.ts,model-catalog.ts,slash.ts,auth-methods.ts,builtin-commands.ts).acp-fs/) so clients can read/write through the session workspace.2.
kimi acp-v2CLI commandProblem: The v2 ACP server needed an entry point that mirrors
kimi acp's host-independent wire-up without leaking the experimental engine into the default CLI module graph.What was done:
kimi acp-v2(apps/kimi-code/src/cli/sub/acp-v2.ts) starting the v2 ACP server over stdio.--loginpivots into the shared device-code login flow via the first-classAuthMethodTerminalpath;KIMI_CODE_HOMEis forwarded intoauthMethods[0].envso the login subprocess writes its token under the same data root.@moonshot-ai/acp-serveris loaded via lazy dynamic import, mirroring thekimi server runv2 routing.3. Tests and workspace wiring
Problem: The new package needed integration coverage and build configuration.
What was done:
packages/acp-server/test/), plus CLI tests foracp-v2(apps/kimi-code/test/cli/acp-v2.test.ts).flake.nix,pnpm-lock.yaml).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.