Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive Agent Client Protocol (ACP) support to enable IDE integrations (Zed, Cursor) through a new ACP agent implementation, bridging layer to convert message streams, permission handling, utilities, configuration updates, and CLI entry point. Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as IDE Client<br/>(Zed/Cursor)
participant Entry as CLI Entry<br/>(--acp)
participant Agent as AcpAgent<br/>(Session Manager)
participant QE as QueryEngine<br/>(Message Stream)
participant Bridge as Bridge Layer<br/>(Conversion)
participant Perms as Permissions<br/>(Tool Allow/Deny)
IDE->>Entry: Connect stdin/stdout<br/>(NDJSON stream)
Entry->>Agent: AgentSideConnection<br/>instantiate
IDE->>Agent: initialize()
Agent-->>IDE: protocol version +<br/>capabilities
IDE->>Agent: newSession()
Agent->>Agent: Create AcpSession +<br/>QueryEngine
Agent-->>IDE: sessionId +<br/>modes/models
IDE->>Agent: prompt(text)
Agent->>QE: setModel() +<br/>submitMessage()
QE->>QE: Stream SDKMessages
loop For each SDKMessage
QE->>Bridge: Forward via<br/>forwardSessionUpdates()
Bridge->>Bridge: Translate tool_use<br/>to ToolInfo
alt Tool Execution
Bridge->>Perms: requestPermission()
Perms-->>IDE: Permission prompt
IDE-->>Perms: allow/deny response
end
Bridge->>Bridge: Convert results +<br/>track usage
Bridge->>Agent: sessionUpdate()<br/>(agent_message_chunk)
Agent-->>IDE: StreamUpdate
end
QE-->>Agent: Final result +<br/>usage
Agent-->>IDE: PromptResponse<br/>(stopReason + usage)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/entrypoints/cli.tsx (1)
135-139: Usesrc/*alias for the new ACP dynamic import.The new import should follow the repository alias convention for source imports.
Proposed fix
- const { runAcpAgent } = await import('../services/acp/entry.js') + const { runAcpAgent } = await import('src/services/acp/entry.js')As per coding guidelines: Use tsconfig path alias
src/*for imports from source directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.tsx` around lines 135 - 139, Update the dynamic import to use the tsconfig path alias by replacing the relative import '../services/acp/entry.js' with the aliased import 'src/services/acp/entry.js' where runAcpAgent is imported; in the ACP fast-path block (the feature('ACP') && process.argv[2] === '--acp' branch that calls profileCheckpoint('cli_acp_path') and awaits runAcpAgent()), change the import specifier to the src/* alias so the code imports from 'src/services/acp/entry.js' instead of a relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/acp-zed.md`:
- Around line 24-37: Two fenced code blocks in docs/features/acp-zed.md (the
ASCII diagram block beginning "┌──────────────┐ NDJSON/stdio
┌──────────────────┐" and the command/params block starting "命令: ccb --acp")
lack language identifiers; add a language tag (e.g., ```text) to both fenced
blocks so markdownlint MD040 is satisfied, updating the opening triple-backtick
lines for those two blocks accordingly.
In `@src/services/acp/__tests__/agent.test.ts`:
- Around line 87-89: The test's mock.module specifier doesn't match the import
used by agent.ts, so the mock doesn't take effect; update the mock.module call
to use the exact import specifier agent.ts uses (including the .js extension and
same relative path) so it intercepts the import. Specifically, change the
mock.module(...) specifier to the same string agent.ts imports for
../../utils/model/modelOptions.js and keep the mocked getModelOptions(() => [])
implementation so agent.ts receives the mocked function.
In `@src/services/acp/__tests__/bridge.test.ts`:
- Around line 670-672: The async generator errorStream is declared as an async
generator but never yields (causing lint/CI failures); update errorStream to
produce at least one yield before throwing (e.g., yield a minimal
SDKMessage-compatible value or placeholder) and then throw the Error('stream
exploded') to preserve the original error behavior while ensuring it's a valid
async generator.
In `@src/services/acp/__tests__/permissions.test.ts`:
- Around line 5-64: The test currently contains an inline re-implementation of
createAcpCanUseTool which risks drifting from the production
src/services/acp/permissions.ts logic; instead, remove the duplicated function
and load the real implementation under controlled mocks by using mock.module()
to mock the bridge/AgentSideConnection behavior (the
connection.requestPermission responses) and then use await import() to import
createAcpCanUseTool from src/services/acp/permissions.ts; ensure your mocked
module exposes the same AgentSideConnection requestPermission contract used in
the tests so the imported createAcpCanUseTool exercises the real code paths
(success, selected/cancelled, errors).
In `@src/services/acp/agent.ts`:
- Around line 450-456: The code currently calls process.chdir(cwd) in AcpAgent
which mutates global process state and breaks multi-session isolation; instead
remove the process.chdir call and keep per-session working directory state (via
setOriginalCwd(cwd) or a session.cwd field), then update any file/exec
operations to use absolute paths or a session-specific resolver (e.g.,
path.resolve(session.cwd, ...) or pass {cwd: session.cwd} to child_process APIs)
so each session operates in its own directory without changing process-wide CWD.
- Around line 513-524: availableModes is missing the 'bypassPermissions' entry
so clients may not be informed of a valid server mode; add an object for the
bypassPermissions mode to the availableModes array (the same place where 'auto',
'default', 'acceptEdits', 'plan', and 'dontAsk' are defined) so
modes.currentModeId can legitimately reference it, and ensure the id exactly
matches the mode name used in applySessionMode() ('bypassPermissions') and the
modes variable that wraps currentModeId.
- Around line 736-739: The getSetting<T>(key: string) stub currently always
returns undefined which causes createSession() to never pick up
permissions.defaultMode; implement getSetting so it actually reads the
configured settings (e.g., load and parse the settings.json or a settings object
/ process.env) and returns the value for the provided key (support nested keys
like "permissions.defaultMode" if used), preserving type T and returning
undefined only if the key is missing; update references in getSetting and ensure
createSession() will receive the configured defaultMode instead of always
falling back to "default".
In `@src/services/acp/bridge.ts`:
- Around line 1076-1079: The session update is hardcoding terminal capability
flags to false when calling toolInfoFromToolUse(...) and
toolUpdateFromToolResult(...), so clients with
clientCapabilities._meta.terminal_output still receive plain text; change those
calls to pass the actual terminal capability (e.g.,
clientCapabilities._meta.terminal_output) instead of false (update every
occurrence, including the calls around lines 1076 and 1111-1115) so terminal
streaming and _meta.terminal_* updates are propagated through the live bridge.
- Around line 576-585: The abort listener added inside the Promise.race for
sdkMessages.next() is never removed when sdkMessages.next() wins, causing leak;
modify the race so you register the handler (const handler = () => ...) and keep
a reference to the returned listener, then after the race resolves remove it via
abortSignal.removeEventListener('abort', handler) (or use a finally block around
awaiting Promise.race) so the listener is cleaned up regardless of which promise
wins; target the Promise.race block around sdkMessages.next(), the
abortSignal.addEventListener call and the handler/nextResult logic.
In `@src/services/acp/permissions.ts`:
- Around line 197-203: The current call to conn.sessionUpdate only notifies the
ACP client with sessionUpdate: 'current_mode_update' but does not change the
AcpAgent server-side session state; after ExitPlanMode succeeds, update the
actual session state on the server (AcpAgent) to the new mode so permission
checks use the updated mode. Modify the logic around the existing
conn.sessionUpdate call (the block that sets sessionUpdate:
'current_mode_update' and currentModeId: selectedOption) to also set the
session's mode/state in the AcpAgent session (the same sessionId) to the chosen
mode (auto/acceptEdits/bypassPermissions), ensuring the server-side session
object is mutated or updated via the appropriate AcpAgent session API so later
permission decisions reflect the UI change.
- Around line 171-203: The code currently calls conn.requestPermission(...) and
conn.sessionUpdate(...) (via handleExitPlanMode path) without catching transport
errors; wrap the call(s) to requestPermission and the subsequent sessionUpdate
branch (including handleExitPlanMode() if invoked) in a try/catch and, on any
exception from the ACP transport, return the same deny decision used by the
standard permission path (behavior: 'deny', message like 'Tool use aborted' or
'ACP transport error', and decisionReason: { type: 'mode', mode: 'default' }) so
client disconnects degrade to a deny decision instead of throwing.
---
Nitpick comments:
In `@src/entrypoints/cli.tsx`:
- Around line 135-139: Update the dynamic import to use the tsconfig path alias
by replacing the relative import '../services/acp/entry.js' with the aliased
import 'src/services/acp/entry.js' where runAcpAgent is imported; in the ACP
fast-path block (the feature('ACP') && process.argv[2] === '--acp' branch that
calls profileCheckpoint('cli_acp_path') and awaits runAcpAgent()), change the
import specifier to the src/* alias so the code imports from
'src/services/acp/entry.js' instead of a relative path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22929c2f-6a68-4306-99de-fcd52cf7b1df
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
README.mdbuild.tsdocs/features/acp-zed.mdpackage.jsonscripts/dev.tssrc/QueryEngine.tssrc/entrypoints/cli.tsxsrc/services/acp/__tests__/agent.test.tssrc/services/acp/__tests__/bridge.test.tssrc/services/acp/__tests__/permissions.test.tssrc/services/acp/agent.tssrc/services/acp/bridge.tssrc/services/acp/entry.tssrc/services/acp/permissions.tssrc/services/acp/utils.ts
Comment on lines
+24
to
+37
| ``` | ||
| ┌──────────────┐ NDJSON/stdio ┌──────────────────┐ | ||
| │ Zed / IDE │ ◄────────────────► │ CCB ACP Agent │ | ||
| │ (Client) │ stdin / stdout │ (Agent) │ | ||
| └──────────────┘ │ │ | ||
| │ entry.ts │ ← stdio → NDJSON stream | ||
| │ agent.ts │ ← ACP protocol handler | ||
| │ bridge.ts │ ← SDKMessage → ACP SessionUpdate | ||
| │ permissions.ts │ ← 权限桥接 | ||
| │ utils.ts │ ← 通用工具 | ||
| │ │ | ||
| │ QueryEngine │ ← 内部查询引擎 | ||
| └──────────────────┘ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
Two fenced blocks are missing a language tag, which will keep markdownlint warnings active.
Proposed fix
-```
+```text
┌──────────────┐ NDJSON/stdio ┌──────────────────┐
│ Zed / IDE │ ◄────────────────► │ CCB ACP Agent │
│ (Client) │ stdin / stdout │ (Agent) │
└──────────────┘ │ │
│ entry.ts │ ← stdio → NDJSON stream
│ agent.ts │ ← ACP protocol handler
│ bridge.ts │ ← SDKMessage → ACP SessionUpdate
│ permissions.ts │ ← 权限桥接
│ utils.ts │ ← 通用工具
│ │
│ QueryEngine │ ← 内部查询引擎
└──────────────────┘- +text
命令: ccb --acp
参数: ["--acp"]
通信: stdin/stdout NDJSON
协议版本: ACP v1
Also applies to: 111-116
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 24-24: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/features/acp-zed.md` around lines 24 - 37, Two fenced code blocks in
docs/features/acp-zed.md (the ASCII diagram block beginning "┌──────────────┐
NDJSON/stdio ┌──────────────────┐" and the command/params block starting "命令:
ccb --acp") lack language identifiers; add a language tag (e.g., ```text) to
both fenced blocks so markdownlint MD040 is satisfied, updating the opening
triple-backtick lines for those two blocks accordingly.
Comment on lines
+87
to
+89
| mock.module('../../../utils/model/modelOptions.ts', () => ({ | ||
| getModelOptions: mock(() => []), | ||
| })) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
rg -n "modelOptions\.(js|ts)'" src/services/acp/agent.ts src/services/acp/__tests__/agent.test.tsRepository: claude-code-best/claude-code
Length of output: 273
🏁 Script executed:
fd -i 'modeloptions' src/utils/model/Repository: claude-code-best/claude-code
Length of output: 103
🏁 Script executed:
cat -n src/utils/model/modelOptions.ts | head -20Repository: claude-code-best/claude-code
Length of output: 931
🏁 Script executed:
rg -A 3 'mock.module' src/services/acp/__tests__/agent.test.ts | head -30Repository: claude-code-best/claude-code
Length of output: 863
Mock the same specifier that agent.ts imports.
agent.ts imports ../../utils/model/modelOptions.js, but this test mocks ../../../utils/model/modelOptions.ts. Bun's mock.module() matches by import specifier string, so this mock will not intercept the production import. All other mocks in this file correctly use .js extensions.
Suggested fix
-mock.module('../../../utils/model/modelOptions.ts', () => ({
+mock.module('../../../utils/model/modelOptions.js', () => ({
getModelOptions: mock(() => []),
}))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mock.module('../../../utils/model/modelOptions.ts', () => ({ | |
| getModelOptions: mock(() => []), | |
| })) | |
| mock.module('../../../utils/model/modelOptions.js', () => ({ | |
| getModelOptions: mock(() => []), | |
| })) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/__tests__/agent.test.ts` around lines 87 - 89, The test's
mock.module specifier doesn't match the import used by agent.ts, so the mock
doesn't take effect; update the mock.module call to use the exact import
specifier agent.ts uses (including the .js extension and same relative path) so
it intercepts the import. Specifically, change the mock.module(...) specifier to
the same string agent.ts imports for ../../utils/model/modelOptions.js and keep
the mocked getModelOptions(() => []) implementation so agent.ts receives the
mocked function.
Comment on lines
+670
to
+672
| async function* errorStream(): AsyncGenerator<SDKMessage, void, unknown> { | ||
| throw new Error('stream exploded') | ||
| } |
Contributor
There was a problem hiding this comment.
Replace the non-yielding async generator.
Biome is right here: this is declared as an async generator but never yields. If lint runs in CI, the suite fails before the behavior is tested.
🧰 Tools
🪛 Biome (2.4.11)
[error] 670-672: This generator function doesn't contain yield.
(lint/correctness/useYield)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/__tests__/bridge.test.ts` around lines 670 - 672, The async
generator errorStream is declared as an async generator but never yields
(causing lint/CI failures); update errorStream to produce at least one yield
before throwing (e.g., yield a minimal SDKMessage-compatible value or
placeholder) and then throw the Error('stream exploded') to preserve the
original error behavior while ensuring it's a valid async generator.
Comment on lines
+5
to
+64
| // ── Inline re-implementation of createAcpCanUseTool for isolated testing ── | ||
| // We cannot import the real permissions.js because agent.test.ts mocks it globally. | ||
| // Instead we re-implement the core logic here, using our own mocked bridge.js. | ||
|
|
||
| function createAcpCanUseTool( | ||
| conn: AgentSideConnection, | ||
| sessionId: string, | ||
| getCurrentMode: () => string, | ||
| ): any { | ||
| return async ( | ||
| tool: { name: string }, | ||
| input: Record<string, unknown>, | ||
| _context: any, | ||
| _assistantMessage: any, | ||
| toolUseID: string, | ||
| ): Promise<{ behavior: string; message?: string; updatedInput?: Record<string, unknown> }> => { | ||
| if (getCurrentMode() === 'bypassPermissions') { | ||
| return { behavior: 'allow', updatedInput: input } | ||
| } | ||
|
|
||
| const TOOL_KIND_MAP: Record<string, string> = { | ||
| Read: 'read', Edit: 'edit', Write: 'edit', | ||
| Bash: 'execute', Glob: 'search', Grep: 'search', | ||
| WebFetch: 'fetch', WebSearch: 'fetch', | ||
| } | ||
|
|
||
| const toolCall = { | ||
| toolCallId: toolUseID, | ||
| title: tool.name, | ||
| kind: TOOL_KIND_MAP[tool.name] ?? 'other', | ||
| status: 'pending', | ||
| rawInput: input, | ||
| } | ||
|
|
||
| const options = [ | ||
| { kind: 'allow_always', name: 'Always Allow', optionId: 'allow_always' }, | ||
| { kind: 'allow_once', name: 'Allow', optionId: 'allow' }, | ||
| { kind: 'reject_once', name: 'Reject', optionId: 'reject' }, | ||
| ] | ||
|
|
||
| try { | ||
| const response = await (conn as any).requestPermission({ sessionId, toolCall, options }) | ||
|
|
||
| if (response.outcome.outcome === 'cancelled') { | ||
| return { behavior: 'deny', message: 'Permission request cancelled by client' } | ||
| } | ||
|
|
||
| if (response.outcome.outcome === 'selected' && response.outcome.optionId !== undefined) { | ||
| const optionId = response.outcome.optionId | ||
| if (optionId === 'allow' || optionId === 'allow_always') { | ||
| return { behavior: 'allow', updatedInput: input } | ||
| } | ||
| } | ||
|
|
||
| return { behavior: 'deny', message: 'Permission denied by client' } | ||
| } catch { | ||
| return { behavior: 'deny', message: 'Permission request failed' } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== verify inline re-implementation in test =="
rg -n -C2 "function createAcpCanUseTool|requestPermission\\(|mock\\.module\\(" src/services/acp/__tests__/permissions.test.ts
echo
echo "== verify production symbol exists =="
rg -n -C2 "createAcpCanUseTool" src/services/acp/permissions.tsRepository: claude-code-best/claude-code
Length of output: 653
Test the production createAcpCanUseTool using mock.module() and await import() instead of duplicating implementation.
The inline re-implementation violates the established test pattern: "Use mock.module() with inline await import() for mocking dependencies." This duplicated logic can drift from src/services/acp/permissions.ts without failing tests. Instead, use mock.module() to mock any bridge dependencies scoped to the test, then await import() the real function from the production module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/__tests__/permissions.test.ts` around lines 5 - 64, The test
currently contains an inline re-implementation of createAcpCanUseTool which
risks drifting from the production src/services/acp/permissions.ts logic;
instead, remove the duplicated function and load the real implementation under
controlled mocks by using mock.module() to mock the bridge/AgentSideConnection
behavior (the connection.requestPermission responses) and then use await
import() to import createAcpCanUseTool from src/services/acp/permissions.ts;
ensure your mocked module exposes the same AgentSideConnection requestPermission
contract used in the tests so the imported createAcpCanUseTool exercises the
real code paths (success, selected/cancelled, errors).
Comment on lines
+450
to
+456
| // Set CWD for the session | ||
| setOriginalCwd(cwd) | ||
| try { | ||
| process.chdir(cwd) | ||
| } catch { | ||
| // CWD may not exist yet; best-effort | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid process.chdir() in a multi-session agent.
AcpAgent keeps several sessions alive in one process, but process.chdir(cwd) changes global state for all of them. Creating or resuming another session can redirect relative-path work in an already-running session into the wrong project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/agent.ts` around lines 450 - 456, The code currently calls
process.chdir(cwd) in AcpAgent which mutates global process state and breaks
multi-session isolation; instead remove the process.chdir call and keep
per-session working directory state (via setOriginalCwd(cwd) or a session.cwd
field), then update any file/exec operations to use absolute paths or a
session-specific resolver (e.g., path.resolve(session.cwd, ...) or pass {cwd:
session.cwd} to child_process APIs) so each session operates in its own
directory without changing process-wide CWD.
Comment on lines
+736
to
+739
| private getSetting<T>(key: string): T | undefined { | ||
| // Simplified: read from environment or return undefined | ||
| // In a full implementation, this would read from settings.json | ||
| return undefined as T | undefined |
Contributor
There was a problem hiding this comment.
This stub makes ACP ignore config-backed session defaults.
createSession() resolves permissions.defaultMode through getSetting(), but this implementation always returns undefined. Every ACP session will therefore start in default, regardless of the user's configured default mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/agent.ts` around lines 736 - 739, The getSetting<T>(key:
string) stub currently always returns undefined which causes createSession() to
never pick up permissions.defaultMode; implement getSetting so it actually reads
the configured settings (e.g., load and parse the settings.json or a settings
object / process.env) and returns the value for the provided key (support nested
keys like "permissions.defaultMode" if used), preserving type T and returning
undefined only if the key is missing; update references in getSetting and ensure
createSession() will receive the configured defaultMode instead of always
falling back to "default".
Comment on lines
+576
to
+585
| const nextResult = await Promise.race([ | ||
| sdkMessages.next(), | ||
| new Promise<IteratorResult<SDKMessage, void>>((resolve) => { | ||
| if (abortSignal.aborted) { | ||
| resolve({ done: true, value: undefined }) | ||
| return | ||
| } | ||
| const handler = () => resolve({ done: true, value: undefined }) | ||
| abortSignal.addEventListener('abort', handler, { once: true }) | ||
| }), |
Contributor
There was a problem hiding this comment.
Clean up the per-iteration abort listener.
When sdkMessages.next() resolves first, the abort listener added for this race stays attached. Long streams will accumulate one extra closure per chunk until the signal fires.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/bridge.ts` around lines 576 - 585, The abort listener added
inside the Promise.race for sdkMessages.next() is never removed when
sdkMessages.next() wins, causing leak; modify the race so you register the
handler (const handler = () => ...) and keep a reference to the returned
listener, then after the race resolves remove it via
abortSignal.removeEventListener('abort', handler) (or use a finally block around
awaiting Promise.race) so the listener is cleaned up regardless of which promise
wins; target the Promise.race block around sdkMessages.next(), the
abortSignal.addEventListener call and the handler/nextResult logic.
Comment on lines
+1076
to
+1079
| ...toolInfoFromToolUse( | ||
| { name: toolName, id: toolUseId, input: toolInput ?? {} }, | ||
| false, | ||
| options?.cwd, |
Contributor
There was a problem hiding this comment.
Propagate terminal-output capability through the live bridge.
The permission bridge checks clientCapabilities._meta.terminal_output, but the actual session update path hardcodes false for both toolInfoFromToolUse() and toolUpdateFromToolResult(). ACP clients that support terminal streaming will still get plain text instead of terminal blocks / _meta.terminal_* updates.
Also applies to: 1111-1115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/bridge.ts` around lines 1076 - 1079, The session update is
hardcoding terminal capability flags to false when calling
toolInfoFromToolUse(...) and toolUpdateFromToolResult(...), so clients with
clientCapabilities._meta.terminal_output still receive plain text; change those
calls to pass the actual terminal capability (e.g.,
clientCapabilities._meta.terminal_output) instead of false (update every
occurrence, including the calls around lines 1076 and 1111-1115) so terminal
streaming and _meta.terminal_* updates are propagated through the live bridge.
Comment on lines
+171
to
+203
| const response = await conn.requestPermission({ | ||
| sessionId, | ||
| toolCall, | ||
| options, | ||
| }) | ||
|
|
||
| if (response.outcome.outcome === 'cancelled') { | ||
| return { | ||
| behavior: 'deny', | ||
| message: 'Tool use aborted', | ||
| decisionReason: { type: 'mode', mode: 'default' }, | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| response.outcome.outcome === 'selected' && | ||
| 'optionId' in response.outcome && | ||
| response.outcome.optionId !== undefined | ||
| ) { | ||
| const selectedOption = response.outcome.optionId | ||
| if ( | ||
| selectedOption === 'default' || | ||
| selectedOption === 'acceptEdits' || | ||
| selectedOption === 'auto' || | ||
| selectedOption === 'bypassPermissions' | ||
| ) { | ||
| await conn.sessionUpdate({ | ||
| sessionId, | ||
| update: { | ||
| sessionUpdate: 'current_mode_update', | ||
| currentModeId: selectedOption, | ||
| }, | ||
| }) |
Contributor
There was a problem hiding this comment.
Handle ACP transport failures here the same way as the standard permission path.
handleExitPlanMode() lets requestPermission() / sessionUpdate() exceptions escape. A client disconnect during this prompt will fail the tool call instead of degrading to a deny decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/permissions.ts` around lines 171 - 203, The code currently
calls conn.requestPermission(...) and conn.sessionUpdate(...) (via
handleExitPlanMode path) without catching transport errors; wrap the call(s) to
requestPermission and the subsequent sessionUpdate branch (including
handleExitPlanMode() if invoked) in a try/catch and, on any exception from the
ACP transport, return the same deny decision used by the standard permission
path (behavior: 'deny', message like 'Tool use aborted' or 'ACP transport
error', and decisionReason: { type: 'mode', mode: 'default' }) so client
disconnects degrade to a deny decision instead of throwing.
Comment on lines
+197
to
+203
| await conn.sessionUpdate({ | ||
| sessionId, | ||
| update: { | ||
| sessionUpdate: 'current_mode_update', | ||
| currentModeId: selectedOption, | ||
| }, | ||
| }) |
Contributor
There was a problem hiding this comment.
Update the server-side mode when ExitPlanMode succeeds.
This only sends current_mode_update to the ACP client. The actual session state in AcpAgent stays unchanged, so later permission decisions can continue using the previous mode even though the UI now shows auto/acceptEdits/bypassPermissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/acp/permissions.ts` around lines 197 - 203, The current call to
conn.sessionUpdate only notifies the ACP client with sessionUpdate:
'current_mode_update' but does not change the AcpAgent server-side session
state; after ExitPlanMode succeeds, update the actual session state on the
server (AcpAgent) to the new mode so permission checks use the updated mode.
Modify the logic around the existing conn.sessionUpdate call (the block that
sets sessionUpdate: 'current_mode_update' and currentModeId: selectedOption) to
also set the session's mode/state in the AcpAgent session (the same sessionId)
to the chosen mode (auto/acceptEdits/bypassPermissions), ensuring the
server-side session object is mutated or updated via the appropriate AcpAgent
session API so later permission decisions reflect the UI change.
claude-code-best
added a commit
that referenced
this pull request
Apr 16, 2026
* feat: 适配 zed acp 协议 * docs: 完善 acp 文档
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
--acpCLI option to launch the agent in ACP mode.