feat(acp): implement Agent Communication Protocol adapter for IDE integration#368
Conversation
🦋 Changeset detectedLatest commit: 3e97dd0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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: 381446fd3a
ℹ️ 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".
| const onSigint = (): void => { | ||
| void cleanup('SIGINT'); | ||
| }; | ||
| const onSigterm = (): void => { | ||
| void cleanup('SIGTERM'); | ||
| }; |
There was a problem hiding this comment.
When SIGINT/SIGTERM is delivered while stdin is still open (for example Ctrl-C in a terminal), these handlers only start cleanup() and then return. They do not close/destroy the ndjson stream or exit the process, so the main path remains blocked on await runAcpServerWithStream(harness, stream) and the CLI can hang instead of terminating after the harness drain.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,48 @@ | |||
| { | |||
| "name": "@moonshot-ai/acp-adapter", | |||
There was a problem hiding this comment.
Adding this new workspace package makes pnpm include @moonshot-ai/acp-adapter, and apps/kimi-code now depends on it, but flake.nix still omits both ./packages/acp-adapter from workspacePaths and @moonshot-ai/acp-adapter from workspaceNames (checked the hardcoded lists). Under the repository's Nix build, the package files are excluded from the source fileset and pnpmConfigHook will not fetch this workspace dependency, so the CLI build cannot resolve the adapter there.
Useful? React with 👍 / 👎.
| "dependencies": { | ||
| "@earendil-works/pi-tui": "^0.74.0", | ||
| "@mariozechner/clipboard": "^0.3.2", | ||
| "@moonshot-ai/acp-adapter": "workspace:^", |
There was a problem hiding this comment.
Because @moonshot-ai/acp-adapter is marked private: true, adding it under the publishable CLI's runtime dependencies makes the npm package depend on an internal workspace package that will not be available to users installing @moonshot-ai/kimi-code. The CLI already bundles @moonshot-ai/* packages in apps/kimi-code/tsdown.config.ts, so this should not be a published runtime dependency; otherwise npm install -g @moonshot-ai/kimi-code can fail resolving the private adapter even though the code is bundled.
Useful? React with 👍 / 👎.
| // `coreInput` includes the optional `mcpServers` extension (Phase | ||
| // 10.1) — the in-process RPC layer round-trips arbitrary JSON | ||
| // fields, so the extra payload survives the kernel boundary even | ||
| // though agent-core's `CreateSessionPayload` does not yet declare | ||
| // it. The kernel-side consumer is a separate concern. | ||
| return rpc.createSession(coreInput); |
There was a problem hiding this comment.
In sessions where an IDE supplies mcpServers, this only forwards the extra field through the SDK RPC call, but agent-core's createSession/resumeSession payloads do not declare or read it and build mcpConfig solely from on-disk/plugin config. The advertised ACP MCP forwarding therefore drops client-provided servers, so tools from those servers never become available in the session.
Useful? React with 👍 / 👎.
| if (!(await this.harness.auth.hasUsableToken())) { | ||
| throw RequestError.authRequired(); |
There was a problem hiding this comment.
When the user configured Kimi Code via /provider or environment/API-key provider settings instead of OAuth login, this unconditional OAuth-token gate rejects session/new before the normal provider manager can use the configured API key. Since the rest of the app supports ProviderConfig.apiKey and /provider, ACP should either check whether the selected provider actually needs OAuth or let session creation/prompting surface auth failures, otherwise valid non-OAuth setups cannot start an ACP session.
Useful? React with 👍 / 👎.
| this.session.prompt(parts).catch((err) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| unsub(); | ||
| reject(mapPromptError(err, sessionId)); |
There was a problem hiding this comment.
If an ACP client sends a second session/prompt while the agent already has an active turn, core emits an error event with turn.agent_busy and returns from Session.prompt() successfully; it does not reject and no turn.ended event belongs to this request. This code only rejects on Session.prompt() rejection or resolves on turn.ended, so the JSON-RPC request remains pending forever in that scenario.
Useful? React with 👍 / 👎.
| stat(path: string, options?: { followSymlinks?: boolean }): Promise<StatResult> { | ||
| return this.inner.stat(path, options); |
There was a problem hiding this comment.
When the client advertises fs.readTextFile, the actual Read tool still calls kaos.stat() before readBytes/readText; this implementation delegates stat to the local filesystem, so a file that exists only as an IDE unsaved buffer fails with local ENOENT before the ACP readTextFile request is ever sent. That breaks the advertised unsaved-buffer read path for new files or virtual client files even though the client can provide their contents.
Useful? React with 👍 / 👎.
sailist
force-pushed
the
feat/acp
branch
2 times, most recently
from
32944f5 to
9ddcd74
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ddcd74fe4
ℹ️ 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".
| if (!acpKaos) { | ||
| return this.runPromptBody(parts, sessionId, conn); | ||
| } | ||
| return runWithKaos(acpKaos, () => this.runPromptBody(parts, sessionId, conn)); |
There was a problem hiding this comment.
When an IDE advertises fs.readTextFile, this only wraps runPromptBody in runWithKaos, but the real built-in Read/Write/Edit tools capture agent.kaos when they are initialized (packages/agent-core/src/agent/tool/index.ts:376-380) rather than calling getCurrentKaos(). In real ACP sessions those tools therefore keep using the LocalKaos created at session creation and never send readTextFile/writeTextFile to the client, so unsaved-buffer file I/O works only in the test fakes that explicitly call getCurrentKaos().
Useful? React with 👍 / 👎.
| */ | ||
| private async maybeBuildAcpKaos(): Promise<AcpKaos | undefined> { | ||
| const fs = this.clientCapabilities?.fs; | ||
| if (!fs?.readTextFile && !fs?.writeTextFile) { |
There was a problem hiding this comment.
When a client advertises only one filesystem capability, this condition still swaps in AcpKaos for every file operation; AcpKaos.readText unconditionally calls conn.readTextFile and writes similarly call writeTextFile. For a write-only or read-only client, ordinary tools on the unsupported side will send reverse-RPC requests the client did not advertise instead of falling back to local filesystem behavior, so partial capability negotiation breaks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13eae126c0
ℹ️ 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".
| let start = 0; | ||
| for (let i = 0; i < text.length; i++) { | ||
| if (text.charCodeAt(i) === 0x0a /* \n */) { | ||
| yield text.slice(start, i); |
There was a problem hiding this comment.
When an ACP client serves an unsaved buffer with CRLF line endings, this generator strips the \n before handing lines to the normal Read tool. ReadTool relies on Kaos.readLines() preserving newline terminators, like LocalKaos, to classify CRLF vs lone carriage returns; with this path it sees \r without \n and renders the file as mixed/lone-CR text with visible carriage returns. Preserve the terminator (or otherwise match LocalKaos.readLines) so ACP-backed reads display the same content as local reads.
Useful? React with 👍 / 👎.
| transport: 'stdio', | ||
| command: stdio.command, | ||
| args: stdio.args, | ||
| env: envArrayToRecord(stdio.env), |
There was a problem hiding this comment.
For stdio MCP servers, ACP only requires name, command, and args; env may be omitted. In that common case this calls envArrayToRecord(undefined) and throws before session/new/session/load can create the session, so an otherwise valid IDE stdio MCP configuration without environment variables breaks the whole ACP session setup. Treat a missing stdio.env as [] before converting it.
Useful? React with 👍 / 👎.
| }); | ||
| return; | ||
| } | ||
| if (event.type === 'turn.ended') { |
There was a problem hiding this comment.
When a prompt invokes a subagent, the child agent emits its own turn.ended event on the same session with a different agentId. This listener does not filter for the main agent or for the turn started by this prompt, so the ACP session/prompt request can resolve as soon as the subagent finishes while the parent turn is still processing the result and may continue streaming afterward. Check the event's agent/turn identity before settling the prompt response.
Useful? React with 👍 / 👎.
| if (event.display) { | ||
| const planNote = planFromDisplayBlock(sessionId, event.turnId, event.display); |
There was a problem hiding this comment.
This is the only path that emits ACP plan updates, but it depends on ToolCallStartedEvent.display carrying kind: 'todo_list'. The current TodoListTool.resolveExecution() in packages/agent-core/src/tools/builtin/state/todo-list.ts does not set any display, so normal TodoList updates only arrive later as textual tool results and this branch never sends sessionUpdate: 'plan'; IDE clients therefore do not see the advertised plan updates. Wire a structured display from the TodoList tool or derive the plan update from the result path.
Useful? React with 👍 / 👎.
| } catch { | ||
| // ENOENT-style failure → treat as empty (mirrors Python open('a')). | ||
| existing = ''; |
There was a problem hiding this comment.
When appending through an ACP-backed filesystem, any failure from the initial readTextFile is treated as if the target were empty. If the client returns a transient error, permission error, or other non-ENOENT failure for an existing unsaved buffer, this then sends writeTextFile with only the appended text and can replace the existing file contents. Only fall back to empty for a real not-found condition; otherwise propagate the read failure.
Useful? React with 👍 / 👎.
| * needs human authorization to proceed with a tool call. | ||
| */ | ||
| private async handleApproval(req: ApprovalRequest): Promise<ApprovalResponse> { | ||
| const toolCall = buildPermissionToolCallUpdate(this.currentTurnId, req); |
There was a problem hiding this comment.
ApprovalRequest already carries the exact turnId for the tool awaiting permission, but this uses the last turn id observed from the event stream. If the approval arrives before that cache is set, or after another agent in the same session has emitted an event, the permission request is sent with a raw or wrong-prefixed toolCallId and clients cannot attach it to the tool card they already rendered. Build the permission update from req.turnId instead of the mutable session-wide cache.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae4c7e4669
ℹ️ 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; | ||
| } | ||
|
|
||
| let turnId = 0; |
There was a problem hiding this comment.
When loading a session that already has turns, this replay counter starts at 0 and beginAssistantTurn increments before emitting, so the first historical assistant turn is replayed as turn 1. In agent-core, live turn IDs start at 0 (TurnFlow initializes to -1 and increments on launch/restore), so after one saved turn the next live prompt also uses turn 1; ACP tool IDs like 1:<toolCallId> can then collide with the replayed tool cards and clients may update the old card instead of creating/updating the new one. Start the synthetic counter at -1 or otherwise mirror the persisted/live turn numbering.
Useful? React with 👍 / 👎.
| this.harness, | ||
| currentModelId, | ||
| currentThinkingEnabled, | ||
| DEFAULT_MODE_ID, |
There was a problem hiding this comment.
When a user has defaultPlanMode: true or defaultPermissionMode: 'auto'/'yolo', KimiCore.createSession applies those defaults to the main agent, but the ACP response still hard-codes DEFAULT_MODE_ID for the mode config option. In those configurations the IDE shows “Default” while prompts actually run in plan/auto/yolo mode, so users can approve or edit under the wrong assumption until they manually change the picker. Initialize the ACP current mode from the same config defaults (or from the created session state) before building configOptions.
Useful? React with 👍 / 👎.
| if (typeof this.harness.getConfig !== 'function') return false; | ||
| try { | ||
| const config = await this.harness.getConfig(); | ||
| const declared = (config as { defaultThinking?: unknown }).defaultThinking; |
There was a problem hiding this comment.
For a normal config with no defaultThinking override, KimiCore.createSession calls resolveThinkingLevel(undefined, config), which defaults to a non-off effort (high) unless thinking.mode is explicitly off; this ACP resolver only reads defaultThinking and otherwise returns false. As a result, thinking-capable models are advertised to the IDE as “Thinking Off” while the session is actually using thinking by default, and configs that set only [thinking] mode/effort are also misrepresented. Use the same resolveThinkingLevel semantics or inspect config.thinking before building the toggle.
Useful? React with 👍 / 👎.
| if (part.type === 'text' && part.text) { | ||
| await conn.sessionUpdate({ |
There was a problem hiding this comment.
When a saved conversation contains a user image/video part, session/load drops it because replay only emits user_message_chunk updates for text parts. The adapter advertises image prompt support and ACP load is supposed to reconstruct the prior conversation in the client, so reopening an image-based session loses the user-provided media from the IDE transcript even though it remains in the SDK history. Convert persisted image_url/other supported user parts back to ACP content blocks instead of skipping every non-text part.
Useful? React with 👍 / 👎.
| return { | ||
| protocolVersion: this.negotiated.protocolVersion, | ||
| agentCapabilities, | ||
| authMethods: [], |
There was a problem hiding this comment.
For first-run ACP clients where harnessIsAuthed() is false, session/new and session/load return auth_required, but initialization advertises no authentication methods and authenticate() is implemented as method-not-found. In that state the client has no ACP path to recover (for example to run the existing login flow) and can only fail session creation indefinitely, so expose a real auth method or avoid returning auth_required when none is available.
Useful? React with 👍 / 👎.
sailist
force-pushed
the
feat/acp
branch
2 times, most recently
from
9dce8de to
60252f5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60252f506c
ℹ️ 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".
| promptCapabilities: { | ||
| image: true, | ||
| audio: false, | ||
| embeddedContext: false, |
There was a problem hiding this comment.
When an ACP client follows the negotiated prompt capabilities, this embeddedContext: false tells it not to send resource blocks; the ACP prompt docs require clients to restrict prompt content by initialization capabilities, and this commit's converter/docs explicitly accept resource and resource_link prompt content. In IDE flows that attach file/@-mention context as embedded resources, the client will suppress that context before acpBlocksToPromptParts() can convert it, so the model receives only the user's text. Either advertise embedded context support here or stop claiming/handling resource prompts.
Useful? React with 👍 / 👎.
| session = await this.harness.resumeSession({ | ||
| id: params.sessionId, | ||
| // @ts-expect-error — see block comment above; mcpServers is a | ||
| // kernel-only field that the SDK forwards via spread. | ||
| mcpServers, |
There was a problem hiding this comment.
When session/load is called with a valid sessionId from a different workspace than params.cwd (for example stale IDE state or a manually selected previous session), this resumes the stored session anyway and then replays it to the current client. The underlying SDK Session has its own workDir, so subsequent tools run against the old project while the ACP client believes it loaded the current cwd; reject or validate mismatched cwd before registering/replaying the session.
Useful? React with 👍 / 👎.
| // other string (including a stale `true` / `false` boolean | ||
| // sent by a pre-Phase-16 client) reads as "off" rather than | ||
| // silently flipping based on truthiness. | ||
| await acpSession.setThinking(value === 'on'); |
There was a problem hiding this comment.
For set_config_option calls with configId: 'thinking', any value other than the exact string 'on' is silently treated as off. A typo, stale client value such as 'true', or future thought-level value will therefore disable thinking even though the advertised select options are only 'on' and 'off'; validate the value and return invalid_params unless it is one of the advertised options.
Useful? React with 👍 / 👎.
| if ( | ||
| block.kind === 'file_io' && | ||
| block.before !== undefined && | ||
| block.after !== undefined | ||
| ) { |
There was a problem hiding this comment.
The Write tool's display payload is kind: 'file_io' with content but no before/after, so this branch returns null and buildPermissionToolCallUpdate() falls back to only the generic “Requesting approval...” text. In manual ACP sessions, users approving a file write cannot inspect the content being written, unlike edit diffs; render content as a text block or a new-file diff (oldText: null) before asking for approval.
Useful? React with 👍 / 👎.
| // Best-effort stringify for object/array outputs. | ||
| let text: string; | ||
| try { | ||
| text = JSON.stringify(out); |
There was a problem hiding this comment.
When a tool returns structured content parts, such as ReadMedia returning text plus an image_url/video_url part, this object/array fallback JSON-stringifies the whole payload into one text block. ACP tool-call content can carry regular content blocks, so image reads in an IDE surface as raw JSON/data URLs instead of renderable media (and the client may have to display a huge base64 string); convert known content parts to ACP content blocks before falling back to JSON.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8095d333d3
ℹ️ 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".
| // otherwise the client would race the load completion against | ||
| // its own UI bootstrap. This is the ONE difference vs. | ||
| // `resumeSession`, which intentionally omits this step. | ||
| await acpSession.replayHistory(); |
There was a problem hiding this comment.
When an ACP client calls session/load for a session that is already active in this server process (for example switching back to a session created earlier via session/new), setupSessionFromExisting() gets the active SDK Session from KimiHarness.resumeSession() rather than a freshly resumed one. Sessions created through createSession() do not carry a resume state, so this replayHistory() call logs and emits no transcript while loadSession still returns success, leaving the client with a blank or incomplete conversation. Load should either fetch/replay the persisted records for active sessions or reuse an existing replay-capable state instead of silently skipping it.
Useful? React with 👍 / 👎.
| // the absolute path to this very binary (`process.argv[1]`) so the | ||
| // client can spawn it with `args:['login']` for the top-level | ||
| // `kimi login` subcommand — matches kimi-cli `acp/server.py:77-96`. | ||
| const legacyCommand = process.argv[1]; |
There was a problem hiding this comment.
For Windows npm installs, process.argv[1] is the JavaScript entrypoint that Node is running, not the .cmd shim the user launched. When a legacy ACP client follows _meta['terminal-auth'] and spawns this command directly, Windows will try to execute the .mjs file instead of node/the shim, so terminal login fails for those clients even though kimi login works in a shell. Use an actually executable command for this path, or include the Node executable plus script in the advertised legacy command/args.
Useful? React with 👍 / 👎.
| const text = await this.readText(path); | ||
| const buf = Buffer.from(text, 'utf8'); |
There was a problem hiding this comment.
When a client advertises fs.readTextFile, this implementation also backs Kaos.readBytes() with the text-only ACP read. The built-in ReadMediaFile tool uses readBytes() for image/video headers and payloads, so binary media reads will be sent through fs/read_text_file, where clients can reject the request or return normalized text that corrupts the bytes. Delegate byte reads to the inner filesystem or otherwise avoid the text RPC for media/binary reads.
Useful? React with 👍 / 👎.
| await this.session.setModel(baseKey); | ||
| if (hasSuffix && typeof this.session.setThinking === 'function') { | ||
| await this.session.setThinking(THINKING_ON_LEVEL); | ||
| this.currentThinkingEnabledInternal = true; | ||
| } | ||
| this.currentModelIdInternal = baseKey; |
There was a problem hiding this comment.
If the current ACP session has Thinking On and the client switches the model selector to an alias that does not support thinking, setModel() preserves the existing SDK thinking level while buildSessionConfigOptions() hides the thinking option for the new model. The user then has no ACP control to turn it off, and subsequent prompts can still send provider-specific reasoning parameters to a model that does not support them. When changing to a non-thinking model, clear the SDK/adaptor thinking state or keep the toggle visible so the client can disable it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0855c34a0
ℹ️ 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".
| if (active !== undefined) return active; | ||
|
|
||
| const summary = await this.rpc.resumeSession({ id }); | ||
| const summary = await this.rpc.resumeSession({ ...input, id }); |
There was a problem hiding this comment.
When an ACP client calls session/load/session/resume with mcpServers for a session already cached in this process, the early activeSessions return above bypasses this new { ...input, id } forwarding, so the caller-provided MCP servers are never connected; I also checked KimiCore.resumeSession, which has the same active-session fast path before its new merge. This breaks IDE reconnect/switch-back flows where the session exists already but the client supplies its current MCP config, leaving those tools unavailable until the session is fully closed and reopened.
Useful? React with 👍 / 👎.
| // `null` (the schema-allowed sentinel for "no filter") the same | ||
| // as `undefined`. | ||
| const cwd = params.cwd ?? undefined; | ||
| const summaries = await this.harness.listSessions( |
There was a problem hiding this comment.
Unlike session/new, session/load, and session/resume, this handler reads and returns saved session metadata without first checking harnessIsAuthed(). In a first-run or logged-out ACP process, any connected client can call session/list and receive prior session IDs, workspace paths, titles, and timestamps even though it cannot load those sessions; apply the same auth gate before calling harness.listSessions() to avoid leaking local history metadata.
Useful? React with 👍 / 👎.
…g, and IDE integration This commit scaffolds the @moonshot-ai/acp-adapter package and introduces the full ACP (Agent Communication Protocol) server implementation for Kimi Code CLI, including: - Scaffold @moonshot-ai/acp-adapter workspace package with build skeleton - `kimi acp` CLI subcommand and stdout-safe logging - ACP version negotiation and AgentSideConnection wrapper - Auth gate for session creation - Session lifecycle: new, list, load with history replay - Prompt content conversion (text, image, embedded resources, resource links) - Assistant streaming with thinking support and end-turn handling - Tool call streaming (started, delta, progress) with result conversion (text / diff) - Approval handling with diff/text display blocks mapped to ACP options - Kaos read/write interface (AcpKaos) for unsaved buffer access - Session mode (yolo/auto) and model management - Config options builder with thinking toggle - MCP server forwarding from ACP to harness - Agent plan updates and available commands updates - AskUserQuestion bridged to session/request_permission - Plan review options surfaced through requestPermission - Error mapping, ext_method stubs, and graceful shutdown - IDE integration guide (Zed + JetBrains) - End-to-end tests against ACP TS SDK client - Changeset, telemetry, and release notes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e97dd05e6
ℹ️ 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".
| const onSigint = (): void => { | ||
| void cleanup('SIGINT'); | ||
| }; | ||
| const onSigterm = (): void => { | ||
| void cleanup('SIGTERM'); | ||
| }; |
There was a problem hiding this comment.
When kimi acp receives SIGINT/SIGTERM while stdin remains open (for example a user pressing Ctrl-C in a terminal or an editor sending SIGTERM without closing the pipe), installing these listeners suppresses Node's default process termination, but the handler only calls harness.close(). The main await runAcpServerWithStream(...) is still waiting for conn.closed, so the finally block that removes listeners never runs and the ACP process can hang instead of exiting after cleanup.
Useful? React with 👍 / 👎.
| "zod": "^4.3.6" | ||
| }, | ||
| "devDependencies": { | ||
| "@moonshot-ai/acp-adapter": "workspace:^", |
There was a problem hiding this comment.
Adding the ACP adapter as a devDependency is enough for the local build, but the non-native CLI bundle only force-bundles @moonshot-ai/* packages (apps/kimi-code/tsdown.config.ts), so the adapter's runtime import of @agentclientprotocol/sdk can remain external. Since @agentclientprotocol/sdk is not listed in this publishable package's dependencies, users installing the npm CLI can hit ERR_MODULE_NOT_FOUND as soon as kimi acp loads; either bundle that package too or declare it as a runtime dependency.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR implements the Agent Communication Protocol (ACP) adapter, enabling IDEs like Zed and JetBrains to drive a kimi-code session over stdio. It introduces the new
@moonshot-ai/acp-adapterpackage, wires thekimi acpCLI subcommand, and provides full two-way conversion between ACP messages and Kimi Code's internal session/harness/model abstractions — including prompt streaming, tool calls, approvals, plan review, session mode/model switching, image/embedded-resource content, and MCP server forwarding.1. ACP adapter package and CLI subcommand
Problem: Kimi Code was CLI-only with no standardized machine interface for external editors or IDEs to consume agent capabilities.
What was done:
@moonshot-ai/acp-adapterpackage with version negotiation,AgentSideConnectionwrapper, and stdio JSON-RPC server.kimi acpsubcommand in the CLI with stdout-safe logging to avoid corrupting the JSON-RPC transport.2. Session lifecycle and state management
Problem: ACP clients need to create, list, load, and reconfigure sessions remotely.
What was done:
session/new,session/load,session/list,session/set_model, andsession/set_modehandlers.session/loadso resumed sessions retain prior context.configOptionsinnewSession/loadSessionresponses.setSessionConfigOptionhandler withconfig_option_updatenotification, replacing the previouscurrent_mode_updatediscriminator.SessionConfigBooleantoggle (thinking), dynamically shown based on the selected model'sthinkingSupportedflag.3. Prompt and content conversion
Problem: ACP sends rich prompts (text, images, embedded resources) that must be mapped into Kimi Code's internal message format.
What was done:
resource_linksupport for embedded resources.4. Tool execution and result streaming
Problem: Agent tool calls and their results need to stream back to the ACP client in real time.
What was done:
ToolCallStarted,ToolCallDelta, andToolCallProgressevents to ACP tool call messages.hideOutputmarker for tools that own their own UI so the client doesn't double-render.5. Approval and permission flow
Problem: Agent approvals (file edits, shell commands, etc.) must be surfaced in the IDE's native permission UX.
What was done:
ApprovalHandlertosession/request_permissionso IDE users can approve or deny agent actions.requestPermissionso users can approve or tweak agent plans before execution.AskUserQuestiontosession/request_permissionfor interactive multi-choice queries.6. Plan updates, commands, and MCP bridging
Problem: ACP clients should see what the agent is planning and what commands are available, and should be able to reuse the user's MCP servers.
What was done:
AgentPlanUpdateandAvailableCommandsUpdateevent forwarding.7. Node SDK and documentation
Problem: The Node SDK lacked a way to enumerate available models, and there was no user-facing IDE setup guide.
What was done:
KimiHarness.listAvailableModels()in@moonshot-ai/kimi-code-sdk.kimi-acp.md).8. Testing
What was done:
Checklist
gen-changesetsand included the changeset (acp-initial.md).docs/en/guides/ides.md,docs/zh/guides/ides.md,docs/en/reference/kimi-acp.md,docs/zh/reference/kimi-acp.md).