-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(acp): Honor MCP servers from clients like Zed (stdio + http) #6230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
since this isn't approved yet anyway, i'll await a pending release from sacp that should be going out today and work any changes in. Also, I did the same MCP integration test in another agent and found it was serving the answer 1+1 from memory and not calling tools. So, I will change the fake MCP server to something an LLM can't guess. (regardless this isnt an issue here as we are pre-canning responses, but I want to set a good example) |
|
tested with nvim yetone/avante.nvim#2883 |
Use sacp (symposium-dev/symposium-acp) which provides struct literals over builders and direct notification dispatch via JrConnectionCx. This crate is slated to become the canonical agentclientprotocol/rust-sdk implementation. Signed-off-by: Adrian Cole <[email protected]>
- Convert McpServer from ACP schema to ExtensionConfig - Stdio: command, args, env vars - Http: url, headers (StreamableHttp transport) - SSE: explicitly not supported (panics) - Inline Calculator MCP server in integration tests - Implement permission flow using await_when_result_received pattern - Use provider tool call ID directly (matching other ACP agents) - Add serde snake_case rename to Permission enum for drift-free conversion Note: rmcp version (0.9.1) matches sacp from https://github.com/symposium-dev/symposium-acp Fixes #6111 Signed-off-by: Adrian Cole <[email protected]>
8f14d8d to
3218e6e
Compare
|
bringing this up for review. the next sacp upgrade will be mechanical and not need much review. better to not hold this change hostage to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR switches goose's ACP implementation from the agent-client-protocol library to the sacp (symposium-acp) library, adding support for MCP servers passed from ACP clients. The refactor enables bidirectional permission requests and adds support for both stdio and HTTP transports for MCP servers.
Key changes:
- Replaces
agent-client-protocolwithsacplibrary for handler-based API - Implements bidirectional permission flow for tool confirmations via
RequestPermissionRequest - Adds HTTP transport support for MCP servers (e.g., kiwi MCP server)
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/goose-cli/src/commands/acp.rs |
Complete rewrite to use sacp's handler-based API; adds MCP server conversion and bidirectional permission handling |
crates/goose-cli/Cargo.toml |
Replaces agent-client-protocol dependency with sacp 9.0.0 |
crates/goose/Cargo.toml |
Adds sacp dev-dependency and rmcp streamable-http-server feature for tests |
crates/goose-mcp/Cargo.toml |
Updates rmcp from 0.8.1 to 0.10.0 |
crates/goose/tests/acp_integration_test.rs |
Rewrites ACP tests to use sacp; adds HTTP MCP server integration test |
crates/goose/tests/common.rs |
Extracts shared test binary building logic; adds GOOSE_BINARY constant |
crates/goose/tests/mcp_integration_test.rs |
Refactors to use shared common module for binary paths |
crates/goose/src/permission/permission_confirmation.rs |
Adds PartialEq and Eq derives for test assertions |
crates/goose/src/agents/extension.rs |
Adds PartialEq derive for ExtensionConfig and Envs for test assertions |
crates/goose/tests/test_data/*.txt |
Adds new test data files for OpenAI mock responses |
Cargo.lock |
Updates dependencies for sacp, rmcp, tokio, and transitive dependencies |
Signed-off-by: Adrian Cole <[email protected]>
3218e6e to
9e3d6b5
Compare
|
|
||
| [dependencies] | ||
| rmcp = { version = "0.8.1", features = ["server", "client", "transport-io", "macros"] } | ||
| rmcp = { workspace = true, features = ["server", "client", "transport-io", "macros"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? was this one just inconsistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah just to have it match versions. copilot insisted ;)
|
Thanks for the look @alexhancock and happy holidays! |

Summary
Switches to the sacp library and adds support for MCP servers passed from ACP clients.
This means that any ACP editor can portably pass an MCP server to Goose the same way as another agent like Gemini, Codex or Claude.
Why sacp?
Notes:
RequestPermissionRequestto the client, which responds with approval/denial.Type of Change
AI Assistance
Testing
test_acp_with_mcp_http_server: Spins up real MCP HTTP server withsumtool, tests full flow (prompt → tool call → tool result → answer)Related Issues
Closes #6111