fix(acp): restore legacy permission compatibility and stabilize ACP#395
fix(acp): restore legacy permission compatibility and stabilize ACP#395sailist wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 43c0cd9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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: f529a0735a
ℹ️ 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 session = await this.harness.createSession({ | ||
| id: sessionId, | ||
| workDir: params.cwd, | ||
| ...(acpKaos !== undefined ? { kaos: acpKaos } : {}), |
There was a problem hiding this comment.
When a client advertises fs.readTextFile or fs.writeTextFile, this passes AcpKaos into the core session, so the session’s own persistence starts using the editor reverse-RPC instead of the local store. Session.writeMetadata()/readMetadata() call this.options.kaos.writeText/readText for $KIMI_HOME/.../state.json, so clients that only serve workspace/unsaved-buffer paths will reject session creation or resume, and even permissive clients are asked to edit Kimi’s internal state through ACP. The ACP bridge should be scoped to tool/workspace file access, not the session metadata Kaos.
Useful? React with 👍 / 👎.
| readBytes(path: string, n?: number): Promise<Buffer> { | ||
| return this.inner.readBytes(path, n); |
There was a problem hiding this comment.
With ACP-backed sessions, ordinary Read calls still invoke ReadTool's this.kaos.readBytes(..., MEDIA_SNIFF_BYTES) before reading text, so this delegation sniffs the saved local file instead of the editor buffer. When the client has unsaved text for a file whose on-disk header is binary/media or otherwise different, the tool can reject or classify the file from stale disk content before the ACP readTextFile path ever gets used. Keep the text-read sniff consistent with the ACP text surface, or restrict the local byte path to media-only callers.
Useful? React with 👍 / 👎.
| } | ||
| if (event.type === 'turn.ended') { | ||
| if (settled) return; | ||
| if (!isFromMainAgent(event)) return; |
There was a problem hiding this comment.
This only ignores child turn.ended events; the earlier assistant.delta and tool.* branches still forward every subagent event as parent ACP session/update notifications. In a foreground Agent tool call, the child has a generated agentId and returns its result through the parent tool result, so leaving these branches unfiltered streams the subagent's private assistant text/tool cards into the user's main prompt while the parent turn is still running. Apply the same main-agent guard to user-visible update branches, not just to settlement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a558b64756
ℹ️ 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 msg = cur.message.toLowerCase(); | ||
| if (msg.includes('not found') || msg.includes('no such file')) return true; |
There was a problem hiding this comment.
In append mode, readText() wraps every ACP failure with a message that includes the file path before isNotFoundError() sees it. If a non-ENOENT failure occurs for a path containing not found or no such file (or a transport/permission message uses those words), this check treats it as a missing file and proceeds to write existing = '' + data, which can mask the real failure and overwrite content instead of surfacing the error. Prefer checking the structured RequestError code or unwrapping the cause before applying any loose message fallback.
Useful? React with 👍 / 👎.
| if (event.type === 'error') { | ||
| if (settled) return; | ||
| if (!isFromMainAgent(event)) return; | ||
| if (event.code !== ErrorCodes.TURN_AGENT_BUSY) return; | ||
| settled = true; |
There was a problem hiding this comment.
When a second session/prompt arrives while a turn is already running, the SDK emits this turn.agent_busy error to every listener for the session, including the listener owned by the already-active prompt. Because this branch rejects any unsettled main-agent prompt, the original in-flight prompt is rejected and unsubscribed even though its turn should continue streaming; only the new overlapping request should fail. Distinguish the prompt that caused the busy event (for example using the active turnId in event.details) before rejecting.
Useful? React with 👍 / 👎.
| const { kaos, ...resumeInput } = input; | ||
| const summary = | ||
| kaos === undefined | ||
| ? await this.rpc.resumeSession({ ...resumeInput, id }) | ||
| : await this.rpc.resumeSessionWithKaos({ ...resumeInput, id }, kaos); |
There was a problem hiding this comment.
The new ACP Kaos is only forwarded after the active-session cache check, so session/load or session/resume with FS capabilities cannot retrofit the ACP file bridge onto a session that is already active in this harness (for example a session created earlier without ACP FS support or by another in-process surface). In that case this method returns the cached Session before resumeSessionWithKaos is called, and all subsequent file tools keep using the old local Kaos; the previous per-prompt wrapping path did not have this active-session gap.
Useful? React with 👍 / 👎.
…ntics - Replace per-prompt runWithKaos with boundary-injection at session creation\n- Fix readBytes to bypass ACP text RPC for binary-safe reads via inner\n- Fix readLines to preserve \n terminators matching LocalKaos\n- Fix writeText append mode to only swallow not-found errors\n- Filter subagent events so child agents don't settle the parent prompt\n- Update SDK createSession/resumeSession to accept optional id and kaos
sailist
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db73b6954a
ℹ️ 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".
| setTimeout(() => { | ||
| void this.emitAvailableCommandsUpdate(sessionId); | ||
| }, 0); |
There was a problem hiding this comment.
Because the skill command map is populated inside emitAvailableCommandsUpdate, deferring that call with setTimeout(0) leaves a window after session/new, session/load, or session/resume has returned where AcpSession.prompt() still has its default empty map. If an ACP client immediately sends a prompt such as /skill:foo ... in that window (before the scheduled notification runs), the slash detector treats it as unknown and forwards the literal text to Session.prompt instead of calling Session.activateSkill; the new tests have to wait for available_commands_update to avoid this race. Resolve and install the map before the session RPC completes, and only defer the client notification if needed.
Useful? React with 👍 / 👎.
| return { | ||
| commands: [...builtinCommands, ...skillCommands], | ||
| skillCommandMap: built.commandMap, |
There was a problem hiding this comment.
This publishes every TUI built-in command to ACP clients, but the ACP prompt path only intercepts entries present in skillCommandMap and explicitly passes unknown slash commands such as /clear through to Session.prompt. In an ACP client that renders available_commands_update, selecting built-ins like /new, /logout, /exit, or /clear will therefore send the literal slash text to the model instead of executing the command the palette advertised. Advertise only commands the ACP adapter can execute, or add ACP-side handlers for these built-ins before including them.
Useful? React with 👍 / 👎.
| settled = true; | ||
| argsByToolCall.clear(); | ||
| startedToolCalls.clear(); | ||
| this.currentTurnId = undefined; |
There was a problem hiding this comment.
When a second session/prompt arrives while a turn is already running, the busy listener for that second prompt reaches this branch and clears the shared currentTurnId even though the original turn is still active. Any subsequent approval or AskUserQuestion from the original turn then builds an unprefixed tool call id in handleApproval/handleQuestion, so the client cannot correlate the permission request with the already-rendered ${turnId}:${toolCallId} tool card. Reject the overlapping prompt without mutating the active turn state owned by the in-flight prompt.
Useful? React with 👍 / 👎.
Add ACP slash-command snapshots and skill activation routing.\nKeep tool Kaos separate from persistence Kaos for ACP session bootstrap.\nTighten append-mode not-found handling and legacy approval compatibility.\nFilter subagent events from parent prompt resolution and update changesets.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c0cd9da3
ℹ️ 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".
| } | ||
|
|
||
| setKaos(kaos: Kaos) { | ||
| this._kaos = kaos; |
There was a problem hiding this comment.
Fresh evidence in this version: the active-session resume path now reaches setKaos, but this setter only replaces Agent._kaos; already-initialized builtin tools still hold the old Kaos object they captured in ToolManager.initializeBuiltinTools() (e.g. new ReadTool(kaos, ...), WriteTool, EditTool). In the inspected active resumeSessionWithOverrides flow, resuming a cached local session with ACP FS capabilities will update agent.kaos but subsequent file tools continue reading/writing through the stale local Kaos instead of the ACP bridge, so the fix for cached ACP sessions is ineffective unless the builtin tools are rebuilt after the rebind.
Useful? React with 👍 / 👎.
1 participant
Related Issue
Fixes #426 — #426
Problem
ACP clients built against the legacy Python
kimi-clipermissionoptionIds (approve,approve_for_session) are currently rejected by the TypeScript ACP adapter, causing tools such as Bash and Write to be auto-denied.This PR also stabilizes adjacent ACP session paths that affect slash skill routing, session bootstrap, filesystem bridging, and subagent event handling.
What changed
Summary
Restores legacy ACP permission compatibility and hardens the ACP adapter around session creation, tool/persistence Kaos separation, slash-command snapshots, and turn event ownership.
1. Restore legacy ACP permission compatibility
Problem: Custom ACP clients using the old Python
kimi-cliopaqueoptionIds send valid permission responses, but the new adapter defensively maps them torejected.What was done:
approve→ allow once.approve_for_session→ allow for session.2. Stabilize ACP session bootstrap and filesystem access
Problem: ACP sessions could route session-internal bootstrap reads and persistence through the transient ACP tool bridge, making AGENTS.md/bootstrap context and metadata persistence fragile when the client-side
fs/readTextFilepath fails.What was done:
3. Add ACP slash command snapshots and skill routing
Problem: ACP clients advertised an empty slash palette or sent
/skill:<name>prompts as plain text, which could fall back to model-driven filesystem exploration.What was done:
/skill:<name> [args]prompts throughSession.activateSkill.available_commands_updateafter the session wire id is stable.4. Filter subagent events from parent prompt resolution
Problem: Subagent
turn.ended, assistant deltas, and tool events could interfere with the parent prompt turn lifecycle.What was done:
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.