feat(acp): support permission mode switching#2364
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| case ThinkPart(think=think): | ||
| await self._send_thinking(think) | ||
| replayed_updates += 1 | ||
| case TextPart(text=text): | ||
| await self._send_text(text) | ||
| replayed_updates += 1 | ||
| case ContentPart(): | ||
| logger.warning("Unsupported replay content part: {part}", part=msg) | ||
| await self._send_text(f"[{msg.__class__.__name__}]") | ||
| replayed_updates += 1 | ||
| case ToolCall(): | ||
| if turn_token is None: | ||
| turn_token = self._begin_replay_turn() | ||
| await self._send_tool_call(msg) | ||
| replayed_updates += 1 | ||
| case ToolCallPart(): | ||
| if self._turn_state is not None: | ||
| await self._send_tool_call_part(msg) | ||
| replayed_updates += 1 | ||
| case ToolResult(): | ||
| if self._turn_state is not None: | ||
| await self._send_tool_result(msg) | ||
| replayed_updates += 1 | ||
| case Notification(): | ||
| await self._send_notification(msg) | ||
| replayed_updates += 1 |
There was a problem hiding this comment.
🔴 AssertionError in replay_history when Notification/content appears outside a turn
In replay_history, after a TurnEnd or StepInterrupted message, self._turn_state is set to None at src/kimi_cli/acp/session.py:329. If a Notification record follows in the wire file, the call chain _send_notification → _send_text → _content_run_id (line 438) hits assert self._turn_state is not None and crashes with AssertionError.
Notifications from background tasks are pumped into the wire file asynchronously via _pump_notifications_to_wire (src/kimi_cli/soul/__init__.py:279) and can legitimately appear at any position — including after TurnEnd or before the first TurnBegin. The same crash applies to ThinkPart, TextPart, and ContentPart records that appear outside a turn boundary during replay. Unlike ToolCall and StepBegin (which have if turn_token is None: turn_token = self._begin_replay_turn() guards), these message types have no such protection.
Prompt for agents
In replay_history(), the ThinkPart, TextPart, ContentPart, and Notification handlers all call _send_thinking/_send_text which invoke _content_run_id, which asserts self._turn_state is not None. But self._turn_state can be None after TurnEnd/StepInterrupted sets it to None, or before the first TurnBegin.
The fix should mirror what ToolCall and StepBegin already do: check if turn_token is None and call self._begin_replay_turn() to create a new _TurnState before attempting to send. For example, add a guard like:
if turn_token is None:
turn_token = self._begin_replay_turn()
before each of the ThinkPart, TextPart, ContentPart, and Notification cases (lines 333-358 in session.py). This ensures _turn_state is always set when _content_run_id is called during replay.
Affected file: src/kimi_cli/acp/session.py, method replay_history(), lines 333-358.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 73f6534022
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for task in pending: | ||
| task.cancel() | ||
| if pending: | ||
| await asyncio.gather(*pending, return_exceptions=True) |
There was a problem hiding this comment.
Cancelling every pending task here also cancels request_task, which is awaiting ApprovalRequest.wait() on the same future consumed by the approval bridge in KimiCLI.run. In the normal path where request_permission returns first, this cancellation marks the shared request future as cancelled, so the later request.resolve(...) calls in this function become no-ops and the bridge never resolves approval_runtime; tool calls that requested approval can then hang indefinitely waiting for a response. This regression is triggered whenever an ACP client responds to a permission dialog before any external mode switch resolves the request.
Useful? React with 👍 / 👎.
huntharo
force-pushed
the
fix/acp-permission-mode-switching
branch
from
73f6534 to
a80adf5
Compare
Kimi surfaced TWO overlapping access-mode dropdowns: the hardcoded Default/Full Access execution modes (kimi+grok only) AND its own advertised runtime modes (Default/Plan/Auto/Yolo). They mirrored each other (picking Yolo flipped the other to Full Access), and the legacy /yolo slash command the hardcoded modes drive is rejected by current kimi. Fix: when an agent advertises its OWN runtime mode selector, defer to it as the single source of truth and suppress the hardcoded execution modes — matching how Gemini already works (one dropdown). - New acpAdvertisesRuntimeModeSelector() mirrors the renderer's getAcpRuntimeModeControl gating EXACTLY (configOptions category 'mode' with >=2 values, OR ACP SessionModeState modes.availableModes with >=2). Kimi's modes arrive via the latter, so a configOptions-only check would have missed them and left the duplicate. - buildAcpExecutionModes() returns [] when a runtime mode selector exists. - usesKimiSlashExecutionModes() returns false in that case, so kimi never sends the rejected /yolo; approval policy is the runtime 'yolo' mode set over the standard session/set_mode protocol method. - grok (no runtime mode selector) keeps its /always-approve execution modes. Tests: execution modes suppressed for kimi advertising runtime modes via either configOptions or SessionModeState; kept for single-mode/no-mode kimi and for grok; registry no longer sends /yolo once kimi advertises modes. Note on Kimi yolo support: mainline kimi-cli advertises only the 'default' ACP mode and its set_session_mode asserts mode_id=='default' (rejects yolo). MoonshotAI/kimi-cli#2364 (open) adds yolo advertisement + a real session/set_mode — which is the mechanism this fix routes through. So a kimi build containing that PR is required for yolo-via-ACP; PwrAgent is now ready for it with no further changes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Kimi surfaced TWO overlapping access-mode dropdowns: the hardcoded Default/Full Access execution modes (kimi+grok only) AND its own advertised runtime modes (Default/Plan/Auto/Yolo). They mirrored each other (picking Yolo flipped the other to Full Access), and the legacy /yolo slash command the hardcoded modes drive is rejected by current kimi. Fix: when an agent advertises its OWN runtime mode selector, defer to it as the single source of truth and suppress the hardcoded execution modes — matching how Gemini already works (one dropdown). - New acpAdvertisesRuntimeModeSelector() mirrors the renderer's getAcpRuntimeModeControl gating EXACTLY (configOptions category 'mode' with >=2 values, OR ACP SessionModeState modes.availableModes with >=2). Kimi's modes arrive via the latter, so a configOptions-only check would have missed them and left the duplicate. - buildAcpExecutionModes() returns [] when a runtime mode selector exists. - usesKimiSlashExecutionModes() returns false in that case, so kimi never sends the rejected /yolo; approval policy is the runtime 'yolo' mode set over the standard session/set_mode protocol method. - grok (no runtime mode selector) keeps its /always-approve execution modes. Tests: execution modes suppressed for kimi advertising runtime modes via either configOptions or SessionModeState; kept for single-mode/no-mode kimi and for grok; registry no longer sends /yolo once kimi advertises modes. Note on Kimi yolo support: mainline kimi-cli advertises only the 'default' ACP mode and its set_session_mode asserts mode_id=='default' (rejects yolo). MoonshotAI/kimi-cli#2364 (open) adds yolo advertisement + a real session/set_mode — which is the mechanism this fix routes through. So a kimi build containing that PR is required for yolo-via-ACP; PwrAgent is now ready for it with no further changes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Related Issue
Resolve #1414
Note that this stacks on top of #2363 - Each PR should be reviewed / merged in order. This is not a 700 line PR trying to change 3 things 😂
Description
I added protocol-level ACP permission mode switching for Kimi sessions.
This PR advertises
defaultandyoloACP session modes fromsession/new,session/load, andsession/resume, and implementssession/set_modeso ACP clients can switch Kimi's persisted yolo approval state without restarting the session. When switching intoyolo, Kimi also resolves currently pending approval requests so a permission dialog does not stay stuck after the mode change. The ACP approval bridge now races the client permission request with local request resolution so external mode changes can unblock an in-flight approval.I checked the prior open context before implementing this: issue #1414 requested switching directly into yolo mode from permission handling, and PR #1525 attempted that by adding an extra approval-dialog option. This implementation keeps the behavior at the ACP session-mode protocol layer instead.
This branch is stacked on the existing ACP message-id and session-load history commits, with this PR still targeting
main.Verification
uv run ruff check src/kimi_cli/acp/server.py src/kimi_cli/acp/session.py tests/acp/test_protocol_v1.py tests/acp/test_server_permission_modes.pyuv run pytest tests/acpuv run pytest tests/acp/test_protocol_v1.py tests/acp/test_server_permission_modes.pykimi acpthrough the ACP SDK, creating a session, switchingdefault -> yolo -> default, checking resumed mode state, and confirming an invalid mode is rejected:SMOKE_OK root=/tmp/kimi-acp-mode-smoke-qcJqmyChecklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.