refactor(acp-adapter): boundary-inject AcpKaos and fix FS bridge semantics#395
refactor(acp-adapter): boundary-inject AcpKaos and fix FS bridge semantics#395sailist wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: a78c954 The changes in this PR will be included in the next version bump. 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
1 participant
Summary
This PR refactors the ACP adapter to inject
AcpKaosat session boundaries instead of wrapping every prompt execution withrunWithKaos. It also fixes several FS bridge semantics issues to align binary/text behavior withLocalKaos, and updates the SDK session APIs to support optionalidandkaosparameters.1. Boundary-inject AcpKaos instead of per-prompt wrapping
Problem:
Previously,
AcpKaoswas instantiated and injected viarunWithKaoson every prompt turn. This added unnecessary overhead and complexity to the prompt lifecycle.What was done:
runWithKaoswith boundary-injection at session creation.acp-adapter.2. Fix FS bridge semantics for binary-safe reads and text consistency
Problem:
Several FS operations in the ACP bridge diverged from
LocalKaosbehavior:readByteswent through ACP text RPC, which is unsafe for binary content.readLinesdid not preserve\nterminators, causing mismatches withLocalKaos.writeTextin append mode swallowed errors too broadly.What was done:
readBytesto bypass ACP text RPC and read directly via the inner implementation for binary safety.readLinesto preserve\nterminators, matchingLocalKaosoutput.writeTextappend mode to only swallow not-found errors.3. Filter subagent events to prevent parent prompt settlement
Problem:
Child agent events (e.g., subagent completion) could incorrectly settle the parent agent's prompt, causing premature or incorrect state transitions.
What was done:
4. Update SDK session APIs
Problem:
The SDK's
createSessionandresumeSessiondid not allow callers to pass an existingidor a customkaosinstance.What was done:
createSession/resumeSessionsignatures to accept optionalidandkaos.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.