specs: add product and tech spec for ACP client support (GH7326)#9380
specs: add product and tech spec for ACP client support (GH7326)#9380nigelpurvis wants to merge 3 commits intowarpdotdev:masterfrom
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @nigelpurvis on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this spec-only pull request. I completed the review and posted feedback on this pull request. Comment I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds product and tech specs for ACP-compatible CLI agents in Warp. The specs describe the user-facing goal and identify several relevant integration points, but the implementation design leaves the core ACP transport integration, configuration model, and security boundaries unresolved.
Concerns
- The tech spec does not define how a bidirectional JSON-RPC stdio ACP process integrates with the current
ThirdPartyHarnessterminal-command runner or how ACP events become Warp conversation, diff, and tool-call UI state. - The product and tech specs conflict on whether session resumption is required in v1.
- The configuration model for multiple user-defined ACP agents is not specified enough to implement settings, selector entries, persistence, or serialized selection state.
- The proposed
CLIAgent::Acp(String)shape does not fit currentCLIAgentmethod signatures and derived traits.
Security
- Arbitrary configured commands/args and automatic MCP passthrough need explicit trust-boundary, argv/shell-safety, consent, and scoping requirements before implementation.
Verdict
Found: 0 critical, 7 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 3. User clicks "Add ACP agent" and enters: | ||
| - **Name** (e.g. "opencode", "Kimi") | ||
| - **Command** (e.g. `opencode`, `kimi`) | ||
| - **Args** (optional, e.g. `--model gpt-4o`) | ||
| 4. Warp validates the command exists on PATH. If not, shows an error with a link |
There was a problem hiding this comment.
| - Text responses appear as agent message blocks. | ||
| - File edits surface in Warp's diff view for review. | ||
| - Tool calls (if passed via MCP) render as tool call blocks. | ||
| 4. The session persists in conversation history and is resumable. |
There was a problem hiding this comment.
tech.md line 199 make resumption an open/follow-up item; choose one requirement and align the success criteria.
|
|
||
| ```rust | ||
| // command_prefix | ||
| CLIAgent::Acp(cmd) => cmd.as_str(), |
There was a problem hiding this comment.
command_prefix() currently returns &'static str and CLIAgent derives Copy/Sequence; CLIAgent::Acp(String) => cmd.as_str() will not fit those contracts, so specify the enum/API redesign or store configured ACP metadata elsewhere.
| - `validate()` → call `validate_cli_installed(&self.command, self.install_docs_url())` | ||
| - `install_docs_url()` → `None` (unknown for arbitrary agents; follow-up can add a registry) | ||
| - `prepare_environment_config()` → write ACP session config if needed; for v1 this may be a no-op or minimal JSON config file with MCP server list | ||
| - `build_command()` → spawn `self.command` with `self.args`, plus the stdio transport flags required by ACP (`--acp` or similar, depending on agent) |
There was a problem hiding this comment.
--acp or similar is not an implementable generic launch contract; specify whether ACP mode flags are entirely user-provided, come from per-agent presets, or are guaranteed by ACP-compatible CLIs.
| The ACP protocol handshake (JSON-RPC `initialize` / `initialized` exchange over | ||
| stdio) is handled in a new `run_acp_session()` function within this file, called | ||
| from `run()`. The initialize request must include: |
There was a problem hiding this comment.
| **MCP passthrough:** After initialization, if Warp has configured MCP servers, | ||
| they are passed to the agent via the ACP session configuration per the protocol | ||
| spec before the first prompt is sent. |
There was a problem hiding this comment.
| ### 5. `app/src/terminal/view/ambient_agent/harness_selector.rs` | ||
|
|
||
| Add `Harness::Acp` to the harness selector dropdown. For user-configured ACP | ||
| agents, each appears as a separate entry using its display name (the command | ||
| string). This requires reading the list of configured ACP agents from settings | ||
| and generating one selector entry per agent. |
There was a problem hiding this comment.
Harness::Acp.
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR adds product and tech specs for supporting ACP-compatible agents in Warp through a generic third-party harness. The specs cover the intended UX, repository files likely to change, testing ideas, and follow-ups.
Concerns
- The proposed ACP handshake and message flow do not match the current ACP protocol, so an implementation following the spec would not interoperate with compliant agents.
- The configurable-agent model is internally inconsistent: the proposed
CLIAgent::Acp(String)conflicts with the currentCopy/static API, while the tradeoffs section recommends a different fieldless/settings-backed design. - The spec does not fully define how ACP conversation persistence maps to Warp's existing external conversation/block snapshot flow.
Security
- The design introduces arbitrary user-configured agent commands and forwards MCP server configuration to third-party processes, but does not define the trust boundary, consent model, shell/process launching constraints, or log redaction for secret-bearing MCP env/args.
- The ACP client surface can include agent-to-client file and terminal requests; the spec needs to map those to Warp permission gates and workspace boundaries before implementation.
Verdict
Found: 1 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 5. Configured ACP agents appear in the harness selector alongside Claude Code, | ||
| Codex, and Gemini. | ||
| 6. MCP servers configured in Warp are passed to the ACP agent session. | ||
| 7. Conversation history for ACP sessions persists and is viewable. |
There was a problem hiding this comment.
|
|
||
| ```rust | ||
| /// An ACP-compatible agent with a user-configured command. | ||
| Acp(String), |
There was a problem hiding this comment.
CLIAgent::Acp(String) conflicts with the current CLIAgent shape (Copy, Sequence, and &'static str command/display APIs), and lines 153-160 recommend a different fieldless/settings-backed design; choose one data model and update the proposed changes accordingly.
| - `validate()` → call `validate_cli_installed(&self.command, self.install_docs_url())` | ||
| - `install_docs_url()` → `None` (unknown for arbitrary agents; follow-up can add a registry) | ||
| - `prepare_environment_config()` → write ACP session config if needed; for v1 this may be a no-op or minimal JSON config file with MCP server list | ||
| - `build_command()` → spawn `self.command` with `self.args`, plus the stdio transport flags required by ACP (`--acp` or similar, depending on agent) |
There was a problem hiding this comment.
--acp flag, so appending a generic transport flag would break arbitrary ACP agents; the spec should require users or a registry to provide the exact command/args needed for each agent.
| ```json | ||
| { | ||
| "jsonrpc": "2.0", | ||
| "id": 1, | ||
| "method": "initialize", | ||
| "params": { | ||
| "clientInfo": { "name": "warp", "version": "<warp_version>" }, | ||
| "capabilities": {} | ||
| } |
There was a problem hiding this comment.
🚨 [CRITICAL] The initialize example is not ACP-compatible: the request must include protocolVersion and clientCapabilities, the response is an initialize response, and ACP does not have an LSP-style initialized notification before session setup; rewrite this flow around initialize followed by session/new/session/prompt.
| **MCP passthrough:** After initialization, if Warp has configured MCP servers, | ||
| they are passed to the agent via the ACP session configuration per the protocol | ||
| spec before the first prompt is sent. |
There was a problem hiding this comment.
| Warp sends prompt as ACP session/message | ||
| Agent streams response chunks | ||
| Warp renders chunks as conversation blocks | ||
| File edits → diff view | ||
| Tool calls → tool call blocks |
There was a problem hiding this comment.
1 participant
Description
Adds product and tech spec for ACP client support, addressing issue #7326. Proposes a new AcpHarness implementing the existing ThirdPartyHarness trait, modeled on GeminiHarness, with user-configurable commands and JSON-RPC stdio transport for v1.