extract mcp-over-acp proxy, align with RFD#146
Draft
nikomatsakis wants to merge 13 commits intoagentclientprotocol:mainfrom
Draft
extract mcp-over-acp proxy, align with RFD#146nikomatsakis wants to merge 13 commits intoagentclientprotocol:mainfrom
nikomatsakis wants to merge 13 commits intoagentclientprotocol:mainfrom
Conversation
- Remove McpAcpTransport struct and MetaCapability impl from capabilities.rs (keep MetaCapability/MetaCapabilityExt traits for proxy capability) - Rename McpConnectRequest.acp_url to acp_id across all source and tests - Update snapshot tests for schema changes (mcpCapabilities.acp, auth removal) - Switch agent-client-protocol-schema to path dependency
New crate src/agent-client-protocol-polyfill with mcp_over_acp module: - McpOverAcpPolyfill proxy (ConnectTo<Conductor>) - BridgeConnectionActor, BridgeListeners, BridgeConnection - HTTP and stdio bridge transports - Uses own BridgeMessage enum instead of ConductorMessage Conductor bridge code still present (removed in Step 3).
- Remove McpBridgeMode enum and mcp_bridge_mode parameter - Delete conductor/mcp_bridge module (actor, http, stdio) - Remove bridge fields from ConductorResponder - Remove bridge ConductorMessage variants and handling - Remove NewSessionRequest MCP server transformation - Update all 16 test files to remove McpBridgeMode - Mark 13 bridge-dependent tests as #[ignore]
…Capabilities.acp) Replace all _meta.symposium.mcp_acp_transport references with mcpCapabilities.acp in: - md/proxying-acp.md (8 references) - md/protocol.md (3 references) - md/conductor.md (2 references) - md/mcp-bridge.md (1 reference)
- Add McpConnectionTo::acp_id(), deprecate acp_url() - Remove orphaned 'conductor mcp $port' CLI subcommand and mcp_bridge.rs - Fix all 13 previously-ignored tests to use McpOverAcpPolyfill::http() - Fix BridgeResponder bug: route through BridgeMessage::ConnectionEstablished instead of creating new HashMap in on_receiving_result callback - Auto-update 2 snapshot tests for new proxy in chain
- agent-client-protocol: removed McpAcpTransport, renamed acp_url→acp_id, added acp_id(), deprecated acp_url() - agent-client-protocol-conductor: removed McpBridgeMode, removed conductor mcp CLI subcommand
Instead of unconditionally enabling unstable_mcp_over_acp on the schema dependency, declare it as a feature on agent-client-protocol that forwards to agent-client-protocol-schema/unstable_mcp_over_acp. Conductor and polyfill crates opt in explicitly.
The conductor and polyfill don't reference McpServer::Acp or McpCapabilities.acp in their source — only snapshot tests see the field in serialized output. Move the feature to dev-dependencies.
nikomatsakis
commented
Apr 30, 2026
Contributor
Author
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
(Review for agent)
Comment on lines
+78
to
+80
| ProxiesAndAgent::new(Testy::new()) | ||
| .proxy(create_echo_proxy()) | ||
| .proxy(McpOverAcpPolyfill::http()), |
Contributor
Author
There was a problem hiding this comment.
This API is a bit wacky. I'd prefer to see...
ProxiesAndAgent::proxies()
.proxy(create_echo_proxy())
.proxy(McpOverAcpPolyfill::http())
.agent(Testy::new())...which would reflect the "execution ordering".
| // 1. Client -> Agent: initialize, session/new, session/prompt (left-to-right ACP) | ||
| // 2. Agent -> Client: MCP initialize, tools/call (right-to-left MCP - all the way back!) | ||
| // 3. Client -> Agent: MCP response | ||
| // 4. Agent -> Client: session/update notification, prompt response |
Contributor
Author
There was a problem hiding this comment.
Dear agent, can you compare the trace below? I'm having a bit of trouble reading it actually.
I expect we now see another element, a proxy.
| "agentCapabilities": Object { | ||
| "loadSession": Bool(false), | ||
| "mcpCapabilities": Object { | ||
| "acp": Bool(false), |
Contributor
Author
There was a problem hiding this comment.
I expect to see "acp" as true when coming out from the mcp-over-acp proxy
| /// Sender for messages to the conductor | ||
| conductor_tx: mpsc::Sender<ConductorMessage>, | ||
|
|
||
| /// Receiver for messages from the conductor to the MCP client |
Contributor
Author
There was a problem hiding this comment.
why did we lose all the comments :)
| conductor_tx: mpsc::Sender<ConductorMessage>, | ||
|
|
||
| /// Receiver for messages from the conductor to the MCP client | ||
| bridge_tx: mpsc::Sender<BridgeMessage>, |
Contributor
Author
There was a problem hiding this comment.
We should have comments on fields to tell us what they are for
| // exactly one. | ||
| // | ||
| // There are provisions for "resuming" from a blocked point by tagging each message in the SSE stream | ||
| // with an id, but we are not implementing that because I am lazy. |
Contributor
Author
There was a problem hiding this comment.
again we lost all the comments. please review the diff and evaluate whether comments were removed "just because" or because they no longer apply
- Restore doc comments in polyfill actor.rs and http.rs that were stripped during extraction from conductor - Polyfill now intercepts InitializeProxyRequest and sets mcpCapabilities.acp = true in the response - Re-enable unstable_mcp_over_acp feature for polyfill crate (it needs to set the acp field) - Update snapshot tests to reflect acp: true ProxiesAndAgent API redesign noted for separate PR.
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
2 participants
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.
This PR makes two important changes
Also, removes some outdated text about
html_paneletc.