From c41106589b8aadfb5526358b339792eb59f74250 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 10:55:31 +0100 Subject: [PATCH 1/8] fix(http)!: narrow HttpClientError and CorsOptions and document the crate - HttpClientError is #[non_exhaustive] with rustdoc on every variant. Its url and reqwest causes are #[source] instead of #[from], and Display no longer repeats the source. - CorsOptions is an opaque struct built through its constructors, so the Vec payload is no longer public. - Re-export axum, reqwest, and url, which appear in public signatures. - Add crate docs and AcpHttpServer rustdoc, including a v1+v2 AgentProtocolRouter example and a # Panics section on into_router, covered by tests. BREAKING CHANGE: HttpClientError is non-exhaustive and no longer implements From or From. CorsOptions variants are no longer public; use CorsOptions::disabled(), allow_origins(..), and allow_any_origin(). Co-Authored-By: Claude --- md/http-transport.md | 6 +- md/migration-config-api.md | 11 +- md/migration-v5.md | 17 ++ src/agent-client-protocol-http/Cargo.toml | 2 + src/agent-client-protocol-http/src/client.rs | 36 +++- src/agent-client-protocol-http/src/lib.rs | 45 +++++ src/agent-client-protocol-http/src/server.rs | 184 ++++++++++++++----- 7 files changed, 247 insertions(+), 54 deletions(-) diff --git a/md/http-transport.md b/md/http-transport.md index 4db0af73..f4e28673 100644 --- a/md/http-transport.md +++ b/md/http-transport.md @@ -80,8 +80,10 @@ agent-client-protocol-http = { version = "3", features = ["client"] } # agent-client-protocol-http = { version = "3", features = ["client", "server"] } ``` -The `client` feature exposes `HttpClient`. The `server` feature exposes -`AcpHttpServer`, `ServerOptions`, and `CorsOptions`. +The `client` feature exposes `HttpClient` and re-exports `reqwest` and `url`. +The `server` feature exposes `AcpHttpServer`, `ServerOptions`, and +`CorsOptions`, and re-exports `axum`. Use the re-exports to name the types in +this crate's signatures without depending on matching versions yourself. ## Request Cancellation diff --git a/md/migration-config-api.md b/md/migration-config-api.md index 547b78eb..63236946 100644 --- a/md/migration-config-api.md +++ b/md/migration-config-api.md @@ -43,16 +43,17 @@ value and preserve the other options. ## CORS policies Existing variants and constructors remain available. **Breaking:** exhaustive -matches on `CorsOptions` outside the HTTP crate must add a wildcard arm: +matches on `CorsOptions` outside the HTTP crate must add a wildcard arm. + +In v5, `CorsOptions` is an opaque struct and its variants are no longer public. +Build policies with `CorsOptions::disabled()`, `CorsOptions::allow_origins(...)`, +or `CorsOptions::allow_any_origin()`, and compare them with `==`: ```rust use agent_client_protocol_http::CorsOptions; fn is_disabled(policy: &CorsOptions) -> bool { - match policy { - CorsOptions::Disabled => true, - _ => false, - } + *policy == CorsOptions::disabled() } ``` diff --git a/md/migration-v5.md b/md/migration-v5.md index a6702e37..fc483844 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -802,3 +802,20 @@ Future rmcp major versions arrive as new features and modules (such as `rmcp_4`) in minor releases, so you can enable both while moving your services to the new rmcp major version. Removing support for an rmcp major version is a breaking change and only happens in an ACP major release. + +## HTTP transport + +`agent-client-protocol-http` narrows two public types: + +- `HttpClientError` is `#[non_exhaustive]`, so matches need a wildcard arm. + `InvalidUrl` and `Reqwest` keep their payloads but no longer implement + `From`, and their `Display` text no longer repeats the underlying error. Walk + `std::error::Error::source` to see it. +- `CorsOptions` is an opaque struct. The `Disabled`, `AllowOrigins`, and + `AllowAnyOrigin` variants are gone. Use `CorsOptions::disabled()`, + `CorsOptions::allow_origins(...)`, and `CorsOptions::allow_any_origin()`, and + compare policies with `==`. + +The crate re-exports `axum` (with `server`) and `reqwest` and `url` (with +`client`), so callers can name `axum::Router`, `reqwest::ClientBuilder`, and +`url::ParseError` without adding matching dependencies. diff --git a/src/agent-client-protocol-http/Cargo.toml b/src/agent-client-protocol-http/Cargo.toml index 500438b5..eceef1bc 100644 --- a/src/agent-client-protocol-http/Cargo.toml +++ b/src/agent-client-protocol-http/Cargo.toml @@ -65,6 +65,8 @@ reqwest = { workspace = true, features = ["stream"], optional = true } url = { workspace = true, optional = true } [dev-dependencies] +# The `AcpHttpServer` example serves v1 and v2 through `AgentProtocolRouter`. +agent-client-protocol = { workspace = true, features = ["unstable_protocol_v2"] } async-stream.workspace = true axum = { workspace = true, features = ["ws", "json"] } tokio = { workspace = true, features = ["macros", "net", "rt", "sync", "time"] } diff --git a/src/agent-client-protocol-http/src/client.rs b/src/agent-client-protocol-http/src/client.rs index f63a9444..77a8c5a6 100644 --- a/src/agent-client-protocol-http/src/client.rs +++ b/src/agent-client-protocol-http/src/client.rs @@ -26,18 +26,30 @@ use crate::protocol::{ method_for_message, method_requires_session_header, session_id_from_message, }; +/// Errors from building an [`HttpClient`]. +/// +/// Returned by [`HttpClient::new`], [`HttpClient::with_endpoint`], +/// [`HttpClient::from_http_client`], and [`HttpClientBuilder::build`]. Errors +/// during a connection are reported as ACP errors by [`ConnectTo`] instead. +/// +/// New variants may be added in minor releases, so matches need a wildcard arm. #[derive(Debug, Error)] +#[non_exhaustive] pub enum HttpClientError { - #[error("invalid URL: {0}")] - InvalidUrl(#[from] url::ParseError), + /// The endpoint is not a valid URL. + #[error("invalid URL")] + InvalidUrl(#[source] url::ParseError), + /// The endpoint uses a scheme other than `http`, `https`, `ws`, or `wss`. #[error("unsupported URL scheme: {0}; expected http, https, ws, or wss")] UnsupportedScheme(String), + /// A WebSocket endpoint was passed to [`HttpClient::from_http_client`]. #[error( "WebSocket URLs require HttpClient::builder or builder_with_endpoint; a prebuilt reqwest client cannot enforce WebSocket connection policies" )] WebSocketRequiresBuilder, - #[error("failed to build HTTP client: {0}")] - Reqwest(#[from] reqwest::Error), + /// reqwest failed to build the underlying HTTP client. + #[error("failed to build HTTP client")] + Reqwest(#[source] reqwest::Error), } /// An endpoint-bound ACP transport using HTTP/SSE or WebSocket. @@ -233,7 +245,8 @@ impl HttpClientBuilder { } else { self.http } - .build()?; + .build() + .map_err(HttpClientError::Reqwest)?; Ok(HttpClient { endpoint, http }) } } @@ -253,7 +266,7 @@ fn parse_base_url(base_url: &str) -> Result { } fn parse_endpoint(endpoint: &str) -> Result { - let endpoint = url::Url::parse(endpoint)?; + let endpoint = url::Url::parse(endpoint).map_err(HttpClientError::InvalidUrl)?; match endpoint.scheme() { "http" | "https" | "ws" | "wss" => Ok(endpoint), scheme => Err(HttpClientError::UnsupportedScheme(scheme.to_string())), @@ -2017,6 +2030,17 @@ mod tests { .build() .unwrap_err(); assert!(matches!(error, HttpClientError::Reqwest(_))); + assert_eq!(error.to_string(), "failed to build HTTP client"); + assert!(std::error::Error::source(&error).is_some()); + } + + #[test] + fn invalid_url_error_reports_the_parse_error_as_its_source() { + let error = HttpClient::new("not a URL").unwrap_err(); + assert!(matches!(error, HttpClientError::InvalidUrl(_))); + assert_eq!(error.to_string(), "invalid URL"); + let source = std::error::Error::source(&error).expect("parse error source"); + assert!(source.downcast_ref::().is_some()); } #[test] diff --git a/src/agent-client-protocol-http/src/lib.rs b/src/agent-client-protocol-http/src/lib.rs index 6e0fbcb3..1ec4da3e 100644 --- a/src/agent-client-protocol-http/src/lib.rs +++ b/src/agent-client-protocol-http/src/lib.rs @@ -1,3 +1,33 @@ +//! HTTP/SSE and WebSocket transports for the Agent Client Protocol (ACP). +//! +//! Both sides serve ACP over a single endpoint, `/acp` by default: +//! +//! - `POST` carries client-to-agent JSON-RPC messages. An `initialize` request +//! creates a connection and returns its `Acp-Connection-Id`. +//! - `GET` with `Accept: text/event-stream` streams agent-to-client messages +//! over SSE, one stream per connection plus one per session. +//! - `GET` with a WebSocket upgrade carries both directions as text frames. +//! - `DELETE` closes the connection. +//! +//! Neither side is enabled by default. Enable the one you need: +//! +//! - `client`: [`HttpClient`] connects to a server over HTTP/SSE or WebSocket, +//! chosen by the URL scheme. It implements +//! [`ConnectTo`](agent_client_protocol::ConnectTo), so it takes the +//! place of an agent component. [`reqwest`] and [`url`] are re-exported for +//! [`HttpClientBuilder::configure_http`], [`HttpClient::from_http_client`], +//! and [`HttpClientError`]. +//! - `server`: [`AcpHttpServer`] turns an agent factory into an +//! [`axum::Router`]. [`ServerOptions`] and [`CorsOptions`] configure the +//! endpoint path, the `/health` route, and browser origins. [`axum`] is +//! re-exported for [`AcpHttpServer::into_router`] and +//! [`CorsOptions::allow_origins`]. +//! +//! The transport carries JSON-RPC messages unchanged, so it serves ACP v1, v2, +//! or both through `AgentProtocolRouter`. +//! JSON-RPC batches are rejected. See the +//! [HTTP transport guide](https://agentclientprotocol.github.io/rust-sdk/http-transport.html) +//! for routing, cancellation, and browser policy details. #![cfg_attr(docsrs, feature(doc_cfg))] #[cfg(feature = "client")] @@ -19,3 +49,18 @@ mod websocket_server; pub use client::{HttpClient, HttpClientBuilder, HttpClientError}; #[cfg(feature = "server")] pub use server::{AcpHttpServer, CorsOptions, ServerOptions}; + +/// The axum version used by [`AcpHttpServer::into_router`] and +/// [`CorsOptions::allow_origins`]. +#[cfg(feature = "server")] +#[doc(no_inline)] +pub use axum; +/// The reqwest version used by [`HttpClientBuilder::configure_http`] and +/// [`HttpClient::from_http_client`]. +#[cfg(feature = "client")] +#[doc(no_inline)] +pub use reqwest; +/// The url version used by [`HttpClientError::InvalidUrl`]. +#[cfg(feature = "client")] +#[doc(no_inline)] +pub use url; diff --git a/src/agent-client-protocol-http/src/server.rs b/src/agent-client-protocol-http/src/server.rs index 1bbef8e3..d0507a72 100644 --- a/src/agent-client-protocol-http/src/server.rs +++ b/src/agent-client-protocol-http/src/server.rs @@ -47,7 +47,7 @@ use crate::connection::ConnectionRegistry; pub struct ServerOptions { /// ACP endpoint path. Defaults to `/acp`. pub path: String, - /// Browser origin policy. Defaults to [`CorsOptions::Disabled`]. + /// Browser origin policy. Defaults to [`CorsOptions::disabled`]. pub cors: CorsOptions, /// Whether to expose `GET /health`. Defaults to `true`. pub health_endpoint: bool, @@ -95,73 +95,71 @@ impl Default for ServerOptions { /// This is not authentication: requests without an `Origin` header are accepted /// by the origin check. Enforce authentication in the host router. /// -/// Prefer the constructors; matches outside this crate must include a wildcard -/// arm to accommodate future policies: +/// Build a policy with [`Self::disabled`] (the default), +/// [`Self::allow_origins`], or [`Self::allow_any_origin`]. Policies compare +/// with `==`: /// /// ``` /// use agent_client_protocol_http::CorsOptions; /// -/// let policy = CorsOptions::disabled(); -/// let disabled = match policy { -/// CorsOptions::Disabled => true, -/// _ => false, -/// }; -/// assert!(disabled); -/// ``` -/// -/// ```compile_fail -/// use agent_client_protocol_http::CorsOptions; -/// -/// let policy = CorsOptions::disabled(); -/// match policy { -/// CorsOptions::Disabled => {}, -/// CorsOptions::AllowOrigins(_) => {}, -/// CorsOptions::AllowAnyOrigin => {}, -/// } +/// assert_eq!(CorsOptions::default(), CorsOptions::disabled()); +/// assert_ne!(CorsOptions::allow_any_origin(), CorsOptions::disabled()); /// ``` #[derive(Debug, Clone, Default, PartialEq, Eq)] -#[non_exhaustive] -pub enum CorsOptions { - /// Disable cross-origin HTTP access and reject WebSocket browser origins. +pub struct CorsOptions { + policy: CorsPolicy, +} + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +enum CorsPolicy { #[default] Disabled, - /// Allow only the listed browser origins. AllowOrigins(Vec), - /// Allow all browser origins. Does not enable credentialed CORS. AllowAnyOrigin, } impl CorsOptions { - /// Disable cross-origin browser access. + /// Disable cross-origin HTTP access and reject WebSocket browser origins. #[must_use] pub fn disabled() -> Self { - Self::Disabled + Self { + policy: CorsPolicy::Disabled, + } } /// Allow any browser origin, without enabling credentialed CORS. #[must_use] pub fn allow_any_origin() -> Self { - Self::AllowAnyOrigin + Self { + policy: CorsPolicy::AllowAnyOrigin, + } } - /// Allow the given browser origins. + /// Allow only the given browser origins. + /// + /// # Errors + /// + /// Returns [`InvalidHeaderValue`] if an origin is not a valid HTTP header + /// value. pub fn allow_origins(origins: I) -> Result where I: IntoIterator, S: AsRef, { - origins + let origins = origins .into_iter() .map(|origin| HeaderValue::from_str(origin.as_ref())) - .collect::, _>>() - .map(Self::AllowOrigins) + .collect::, _>>()?; + Ok(Self { + policy: CorsPolicy::AllowOrigins(origins), + }) } fn allow_origin_layer(&self) -> Option { - match self { - Self::Disabled => None, - Self::AllowOrigins(origins) => Some(AllowOrigin::list(origins.clone())), - Self::AllowAnyOrigin => Some(AllowOrigin::any()), + match &self.policy { + CorsPolicy::Disabled => None, + CorsPolicy::AllowOrigins(origins) => Some(AllowOrigin::list(origins.clone())), + CorsPolicy::AllowAnyOrigin => Some(AllowOrigin::any()), } } @@ -169,10 +167,10 @@ impl CorsOptions { let Some(origin) = origin else { return true; }; - match self { - Self::Disabled => false, - Self::AllowOrigins(origins) => origins.iter().any(|allowed| allowed == origin), - Self::AllowAnyOrigin => true, + match &self.policy { + CorsPolicy::Disabled => false, + CorsPolicy::AllowOrigins(origins) => origins.iter().any(|allowed| allowed == origin), + CorsPolicy::AllowAnyOrigin => true, } } } @@ -183,6 +181,60 @@ struct ServerState { cors: CorsOptions, } +/// Serves ACP agents over HTTP/SSE and WebSocket as an [`axum::Router`]. +/// +/// [`Self::new`] takes a factory that builds one agent component per +/// connection. Each `initialize` POST or WebSocket upgrade calls it once, and +/// the component runs until the client sends `DELETE` or disconnects. +/// +/// The transport forwards JSON-RPC messages unchanged, so the component decides +/// which protocol versions it speaks. To serve ACP v1 and v2 on one endpoint, +/// return an `AgentProtocolRouter` +/// from the factory (requires the core `unstable_protocol_v2` feature): +/// +/// ```no_run +/// use agent_client_protocol::schema::{v1, v2}; +/// use agent_client_protocol::{Agent, Client, ConnectTo, on_receive_request}; +/// use agent_client_protocol_http::AcpHttpServer; +/// +/// fn v1_agent() -> impl ConnectTo { +/// Agent.v1().on_receive_request( +/// async |init: v1::InitializeRequest, responder, _cx| { +/// responder.respond(v1::InitializeResponse::new(init.protocol_version)) +/// }, +/// on_receive_request!(), +/// ) +/// } +/// +/// fn v2_agent() -> impl ConnectTo { +/// Agent.v2().on_receive_request( +/// async |init: v2::InitializeRequest, responder, _cx| { +/// responder.respond(v2::InitializeResponse::new( +/// init.protocol_version, +/// v2::Implementation::new("example", "0.1.0"), +/// )) +/// }, +/// on_receive_request!(), +/// ) +/// } +/// +/// # async fn serve() -> Result<(), Box> { +/// let app = AcpHttpServer::new(|| { +/// Agent +/// .protocol_router() +/// .with_v1(v1_agent()) +/// .with_v2(v2_agent()) +/// }) +/// .into_router(); +/// +/// let listener = tokio::net::TcpListener::bind("127.0.0.1:8080").await?; +/// axum::serve(listener, app).await?; +/// # Ok(()) +/// # } +/// ``` +/// +/// Use [`Self::with_options`] to change the endpoint path, the browser origin +/// policy, or the `/health` route. pub struct AcpHttpServer { registry: Arc, options: ServerOptions, @@ -197,6 +249,9 @@ impl std::fmt::Debug for AcpHttpServer { } impl AcpHttpServer { + /// Create a server that calls `factory` once per ACP connection. + /// + /// The server starts with [`ServerOptions::default`]. pub fn new(factory: F) -> Self where F: Fn() -> C + Send + Sync + 'static, @@ -208,12 +263,24 @@ impl AcpHttpServer { } } + /// Replace the server options. #[must_use] pub fn with_options(mut self, options: ServerOptions) -> Self { self.options = options; self } + /// Build the axum router. + /// + /// The router serves `POST`, `GET`, and `DELETE` on + /// [`ServerOptions::path`] and, unless disabled, `GET /health`. Nest or + /// merge it into a larger router to add authentication or other routes. + /// + /// # Panics + /// + /// Panics if [`ServerOptions::path`] is empty or does not start with `/`, + /// or if it is `/health` while [`ServerOptions::health_endpoint`] is + /// enabled. These are the conditions under which [`Router::route`] panics. pub fn into_router(self) -> Router { let registry = self.registry.clone(); let path = self.options.path.clone(); @@ -295,9 +362,44 @@ mod tests { use axum::body::Body; use tower::{Layer as _, ServiceExt as _, service_fn}; + fn build_router(options: ServerOptions) { + drop( + AcpHttpServer::new(|| agent_client_protocol::Agent.v1()) + .with_options(options) + .into_router(), + ); + } + + #[test] + #[should_panic(expected = "Paths must start with a `/`")] + fn into_router_panics_on_a_path_without_a_leading_slash() { + build_router(ServerOptions::default().with_path("acp")); + } + + #[test] + #[should_panic(expected = "Paths must start with a `/`")] + fn into_router_panics_on_an_empty_path() { + build_router(ServerOptions::default().with_path("")); + } + + #[test] + #[should_panic(expected = "Overlapping method route")] + fn into_router_panics_when_the_path_is_the_health_route() { + build_router(ServerOptions::default().with_path("/health")); + } + + #[test] + fn into_router_accepts_the_health_path_without_the_health_route() { + build_router( + ServerOptions::default() + .with_path("/health") + .with_health_endpoint(false), + ); + } + #[test] fn cors_is_disabled_by_default() { - assert_eq!(ServerOptions::default().cors, CorsOptions::Disabled); + assert_eq!(ServerOptions::default().cors, CorsOptions::disabled()); } #[test] From 4c04d2ce033b4a25534382314542f22808808ee5 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:56:42 +0100 Subject: [PATCH 2/8] fix(rmcp)!: report rmcp tool errors as results and narrow the MCP API - Builder-built MCP servers report the builder name and a new McpServerBuilder::version (default 0.0.0) as serverInfo instead of rmcp. - Tool errors, including arguments that do not match the input schema, become tools/call results with isError: true. Unknown or disabled tools still get -32602. RegisteredMcpTool::call_tool reports bad arguments as -32602 instead of -32603. - The native MCP-over-ACP path follows the core crate's unstable_mcp_over_acp feature through a hidden __if_mcp_over_acp! macro, so enabling it only on agent-client-protocol no longer makes each request bridge a fresh rmcp service. - EnabledTools is crate-private. enable_tool/disable_tool return UnknownToolError, a local error, instead of a -32600 Error. - McpServerExt is sealed and McpConnectionContext::Acp is non_exhaustive. - Drop rmcp's no-op unstable_protocol_v2 passthrough feature. - Make the tool_fn/tool_fn_mut builder examples compile with their macro argument. BREAKING CHANGE: EnabledTools is no longer exported. enable_tool and disable_tool return UnknownToolError. McpServerExt is sealed. McpConnectionContext::Acp is non_exhaustive. agent-client-protocol-rmcp no longer has an unstable_protocol_v2 feature. Builder tool errors are now isError results rather than JSON-RPC errors. Co-Authored-By: Claude --- md/features.md | 2 +- md/mcp-over-acp.md | 2 +- md/migration-v5.md | 32 ++- .../tests/trace_mcp_tool_call.rs | 4 +- src/agent-client-protocol-cookbook/src/lib.rs | 11 +- src/agent-client-protocol-rmcp/Cargo.toml | 9 +- src/agent-client-protocol-rmcp/README.md | 9 +- .../examples/stateless_native_mcp.rs | 2 +- src/agent-client-protocol-rmcp/src/lib.rs | 6 +- .../src/rmcp_3/builder.rs | 204 +++++++++++++----- .../src/rmcp_3/mod.rs | 130 ++++++----- .../tests/modern_mcp.rs | 98 +++++++++ .../src/mcp_server/context.rs | 1 + .../src/mcp_server/mod.rs | 34 ++- .../src/mcp_server/registry.rs | 72 ++++++- 15 files changed, 467 insertions(+), 149 deletions(-) diff --git a/md/features.md b/md/features.md index 23b77aca..5fafa93c 100644 --- a/md/features.md +++ b/md/features.md @@ -56,7 +56,7 @@ serialization and deserialization. Within `mcp_server`, these typed APIs require `schemars`: - `McpTool` -- `McpToolRegistry`, `RegisteredMcpTool`, and `EnabledTools` +- `McpToolRegistry`, `RegisteredMcpTool`, and `UnknownToolError` - `McpToolMetadata` and `McpToolSchema` - The `tool_fn` and `tool_fn_mut` functions diff --git a/md/mcp-over-acp.md b/md/mcp-over-acp.md index cc7df9c8..71eb84dc 100644 --- a/md/mcp-over-acp.md +++ b/md/mcp-over-acp.md @@ -99,7 +99,7 @@ roll back arbitrary detached application work. ```sh cargo run -p agent-client-protocol-rmcp \ --example stateless_native_mcp \ - --features rmcp_3,unstable_mcp_over_acp,unstable_protocol_v2 + --features rmcp_3,unstable_mcp_over_acp ``` See the [wire reference](./protocol.md#native-mcp-over-acp), diff --git a/md/migration-v5.md b/md/migration-v5.md index fc483844..823b829f 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -780,8 +780,10 @@ agent-client-protocol-rmcp = { version = "5", features = ["rmcp_3", "unstable_mc rmcp = "3" ``` -The `unstable_protocol_v2` and `unstable_mcp_over_acp` passthrough features are -unchanged and apply to every rmcp module. +The `unstable_mcp_over_acp` passthrough feature is unchanged and applies to +every rmcp module. The `unstable_protocol_v2` passthrough feature is removed +because the crate has no v2-specific code. Enable `unstable_protocol_v2` on +`agent-client-protocol` instead. Then add `rmcp_3::` to every import from the crate: @@ -790,11 +792,10 @@ Then add `rmcp_3::` to every import from the crate: | `agent_client_protocol_rmcp::McpServerExt` | `agent_client_protocol_rmcp::rmcp_3::McpServerExt` | | `agent_client_protocol_rmcp::McpServerBuilder` | `agent_client_protocol_rmcp::rmcp_3::McpServerBuilder` | | `agent_client_protocol_rmcp::McpTool` | `agent_client_protocol_rmcp::rmcp_3::McpTool` | -| `agent_client_protocol_rmcp::EnabledTools` | `agent_client_protocol_rmcp::rmcp_3::EnabledTools` | | `agent_client_protocol_rmcp::tool_fn!` | `agent_client_protocol_rmcp::rmcp_3::tool_fn!` | | `agent_client_protocol_rmcp::tool_fn_mut!` | `agent_client_protocol_rmcp::rmcp_3::tool_fn_mut!` | -`McpTool`, `EnabledTools`, `tool_fn!` and `tool_fn_mut!` are re-exports of the +`McpTool`, `tool_fn!` and `tool_fn_mut!` are re-exports of the core crate's items, which you can also import from `agent_client_protocol::mcp_server` and `agent_client_protocol` directly. @@ -803,6 +804,29 @@ Future rmcp major versions arrive as new features and modules (such as to the new rmcp major version. Removing support for an rmcp major version is a breaking change and only happens in an ACP major release. +### Other rmcp and `mcp_server` changes + +- `EnabledTools` is no longer public. No public API took or returned it. Use + `enable_tool`, `disable_tool`, `enable_all_tools`, and `disable_all_tools`. +- `McpServerBuilder::enable_tool` / `disable_tool` and + `McpToolRegistry::enable_tool` / `disable_tool` return `UnknownToolError` + instead of a `-32600` `Error`. `?` still converts it into an `Error`. +- `McpServerExt` is sealed. It was only meant to be implemented for + `McpServer`. +- `McpConnectionContext::Acp` is `#[non_exhaustive]`. Match it as + `McpConnectionContext::Acp { server_id, .. }`. +- Builder-built servers report the builder name and `McpServerBuilder::version` + (default `0.0.0`) as their MCP `serverInfo`, not `rmcp`. +- An error from a builder tool, including arguments that do not match its input + schema, is a `tools/call` result with `isError: true` instead of a JSON-RPC + error. A call to an unknown or disabled tool still gets `-32602`. + `RegisteredMcpTool::call_tool` reports bad arguments as `-32602` instead of + `-32603`. +- The native MCP-over-ACP path for `McpServer::from_rmcp` and builder servers + follows the core crate's `unstable_mcp_over_acp` feature. Before, enabling + the feature only on `agent-client-protocol` made each request bridge a new + rmcp service. + ## HTTP transport `agent-client-protocol-http` narrows two public types: diff --git a/src/agent-client-protocol-conductor/tests/trace_mcp_tool_call.rs b/src/agent-client-protocol-conductor/tests/trace_mcp_tool_call.rs index 8c83c622..424f3b41 100644 --- a/src/agent-client-protocol-conductor/tests/trace_mcp_tool_call.rs +++ b/src/agent-client-protocol-conductor/tests/trace_mcp_tool_call.rs @@ -708,8 +708,8 @@ async fn test_trace_mcp_tool_call() -> Result<(), agent_client_protocol::Error> "cacheScope": String("private"), "_meta": Object { "io.modelcontextprotocol/serverInfo": Object { - "name": String("rmcp"), - "version": String(""), + "name": String("test"), + "version": String("0.0.0"), }, }, }, diff --git a/src/agent-client-protocol-cookbook/src/lib.rs b/src/agent-client-protocol-cookbook/src/lib.rs index ced77392..e194d308 100644 --- a/src/agent-client-protocol-cookbook/src/lib.rs +++ b/src/agent-client-protocol-cookbook/src/lib.rs @@ -1059,13 +1059,16 @@ pub mod filtering_tools { //! // Only enable the safe tool //! .enable_tool("safe") //! .map(|b| b.build()) + //! .map_err(agent_client_protocol::Error::from) //! } //! ``` //! //! # Error handling //! - //! Both [`enable_tool`] and [`disable_tool`] return `Result` and will error - //! if the tool name doesn't match any registered tool. This helps catch typos: + //! Both [`enable_tool`] and [`disable_tool`] return an [`UnknownToolError`] + //! if the tool name doesn't match any registered tool. This helps catch typos. + //! It is a local error, and `?` converts it into an + //! [`agent_client_protocol::Error`]: //! //! ``` //! use agent_client_protocol::mcp_server::McpServer; @@ -1076,7 +1079,8 @@ pub mod filtering_tools { //! let result = McpServer::::builder("server") //! .disable_tool("ech"); // Typo! Should be "echo" //! - //! assert!(result.is_err()); + //! let Err(error) = result else { panic!("expected an unknown tool") }; + //! assert_eq!(error.name(), "ech"); //! ``` //! //! Calling enable/disable on an already enabled/disabled tool is not an error - @@ -1085,6 +1089,7 @@ pub mod filtering_tools { //! [`disable_tool`]: agent_client_protocol_rmcp::rmcp_3::McpServerBuilder::disable_tool //! [`enable_tool`]: agent_client_protocol_rmcp::rmcp_3::McpServerBuilder::enable_tool //! [`disable_all_tools`]: agent_client_protocol_rmcp::rmcp_3::McpServerBuilder::disable_all_tools + //! [`UnknownToolError`]: agent_client_protocol_rmcp::rmcp_3::UnknownToolError } pub mod running_proxies_with_conductor { diff --git a/src/agent-client-protocol-rmcp/Cargo.toml b/src/agent-client-protocol-rmcp/Cargo.toml index bc7da789..d834b20c 100644 --- a/src/agent-client-protocol-rmcp/Cargo.toml +++ b/src/agent-client-protocol-rmcp/Cargo.toml @@ -20,9 +20,9 @@ rustdoc-args = ["--cfg", "docsrs"] default = [] # rmcp 3.x integration in the `rmcp_3` module. rmcp_3 = ["dep:rmcp_3"] -# Passthrough features; they apply to every rmcp module. +# Passthrough feature for the core crate's `unstable_mcp_over_acp`. The native +# MCP-over-ACP path follows the core feature however it is enabled. unstable_mcp_over_acp = ["agent-client-protocol/unstable_mcp_over_acp"] -unstable_protocol_v2 = ["agent-client-protocol/unstable_protocol_v2"] [[example]] name = "with_mcp_server" @@ -30,7 +30,8 @@ required-features = ["rmcp_3", "unstable_mcp_over_acp"] [[example]] name = "stateless_native_mcp" -required-features = ["rmcp_3", "unstable_protocol_v2", "unstable_mcp_over_acp"] +# Also needs the core `unstable_protocol_v2` feature, enabled by the dev-dependency below. +required-features = ["rmcp_3", "unstable_mcp_over_acp"] [dependencies] agent-client-protocol = { workspace = true, features = ["schemars"] } @@ -45,6 +46,8 @@ tokio = { workspace = true, features = ["io-util", "macros", "rt"] } tokio-util = { workspace = true, features = ["rt"] } [dev-dependencies] +# The `stateless_native_mcp` example attaches a server to a v2 proxy. +agent-client-protocol = { workspace = true, features = ["schemars", "unstable_protocol_v2"] } # Cargo requires one name per package, so tests and examples import `rmcp_3` # under its usual name with `extern crate rmcp_3 as rmcp;`. rmcp_3 = { package = "rmcp", version = "3.4.0", default-features = false, features = ["macros", "server", "schemars"] } diff --git a/src/agent-client-protocol-rmcp/README.md b/src/agent-client-protocol-rmcp/README.md index 3f33658b..54c89d67 100644 --- a/src/agent-client-protocol-rmcp/README.md +++ b/src/agent-client-protocol-rmcp/README.md @@ -31,9 +31,10 @@ build or directly serve a server. Enable this crate's matching `agent-client-protocol-polyfill` when the final agent accepts HTTP but not ACP-transport MCP servers. -`unstable_protocol_v2` and `unstable_mcp_over_acp` are independent passthrough -features. They apply to every rmcp module. Enable both when attaching a native -MCP server to draft ACP v2. +`unstable_mcp_over_acp` is a passthrough for the core SDK feature and applies to +every rmcp module. The native path follows the core feature however it is +enabled. To attach a native MCP server to draft ACP v2, also enable +`unstable_protocol_v2` on `agent-client-protocol` itself. Native attachments lazily initialize one shared rmcp application service. Standalone connections still invoke the factory separately. Each native @@ -42,7 +43,7 @@ Run the direct ACP example with: ```sh cargo run -p agent-client-protocol-rmcp --example stateless_native_mcp \ - --features rmcp_3,unstable_protocol_v2,unstable_mcp_over_acp + --features rmcp_3,unstable_mcp_over_acp ``` ## Usage diff --git a/src/agent-client-protocol-rmcp/examples/stateless_native_mcp.rs b/src/agent-client-protocol-rmcp/examples/stateless_native_mcp.rs index 033f1958..be04fd36 100644 --- a/src/agent-client-protocol-rmcp/examples/stateless_native_mcp.rs +++ b/src/agent-client-protocol-rmcp/examples/stateless_native_mcp.rs @@ -1,7 +1,7 @@ //! Direct native ACP call into a real rmcp service, without an MCP handshake. //! //! cargo run -p agent-client-protocol-rmcp --example stateless_native_mcp \ -//! --features rmcp_3,unstable_protocol_v2,unstable_mcp_over_acp +//! --features rmcp_3,unstable_mcp_over_acp use agent_client_protocol::v2::{ConnectionTo, Responder, schema}; use agent_client_protocol::{Agent, Client, Error, mcp_server::McpServer, schema::ProtocolVersion}; diff --git a/src/agent-client-protocol-rmcp/src/lib.rs b/src/agent-client-protocol-rmcp/src/lib.rs index e4795717..c3268abd 100644 --- a/src/agent-client-protocol-rmcp/src/lib.rs +++ b/src/agent-client-protocol-rmcp/src/lib.rs @@ -25,8 +25,10 @@ //! between rmcp major versions. Support for a new rmcp major version arrives //! as a new feature in a minor release; dropping one is a breaking change. //! -//! The `unstable_protocol_v2` and `unstable_mcp_over_acp` passthrough features -//! apply to every rmcp module. +//! The `unstable_mcp_over_acp` feature passes through to the core crate and +//! applies to every rmcp module. The native MCP-over-ACP path follows the core +//! crate's feature, however it is enabled. For draft ACP v2, enable +//! `unstable_protocol_v2` on `agent-client-protocol` directly. #![cfg_attr(docsrs, feature(doc_cfg))] #[cfg(feature = "rmcp_3")] diff --git a/src/agent-client-protocol-rmcp/src/rmcp_3/builder.rs b/src/agent-client-protocol-rmcp/src/rmcp_3/builder.rs index b86f3564..0533f27b 100644 --- a/src/agent-client-protocol-rmcp/src/rmcp_3/builder.rs +++ b/src/agent-client-protocol-rmcp/src/rmcp_3/builder.rs @@ -16,31 +16,59 @@ use schemars::JsonSchema; use serde::{Serialize, de::DeserializeOwned}; use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; -#[cfg(feature = "unstable_mcp_over_acp")] -use acp::mcp_server::{McpOutcome, McpRequest, McpRequestContext, McpService}; use agent_client_protocol as acp; use agent_client_protocol::{ ByteStreams, ChainRun, ConnectTo, DynConnectTo, NullRun, RunWithConnectionTo, mcp_server::{ McpConnectionTo, McpServer, McpServerConnect, McpTool, McpToolMetadata, McpToolRegistry, + UnknownToolError, }, role::{self, Role}, }; +acp::__if_mcp_over_acp! { + { + use acp::mcp_server::{McpOutcome, McpRequest, McpRequestContext, McpService}; + } else {} +} + +/// The `serverInfo.version` reported until [`McpServerBuilder::version`] is called. +const DEFAULT_SERVER_VERSION: &str = "0.0.0"; + /// Builder for creating MCP servers with tools. /// /// Use [`McpServerExt::builder`](super::McpServerExt::builder) to create a new builder, then chain methods to /// configure the server and call [`build`](Self::build) to create the server. /// +/// The server reports the builder's name and [`version`](Self::version) as its +/// MCP `serverInfo`. +/// +/// # Tool errors +/// +/// - A call to a tool that is not registered or is disabled gets an invalid +/// params JSON-RPC error (`-32602`). +/// - Arguments that do not deserialize into the tool's input type, and errors +/// returned by the tool itself, become a `tools/call` result with +/// `isError: true` and the error text as its content, so the model can see +/// and correct them. +/// - A cancelled call gets an internal JSON-RPC error (`-32603`). +/// /// # Example /// -/// ```rust,ignore -/// use agent_client_protocol::mcp_server::McpServer; +/// ``` +/// use agent_client_protocol::{mcp_server::McpServer, role::mcp}; /// use agent_client_protocol_rmcp::rmcp_3::McpServerExt; +/// use schemars::JsonSchema; +/// use serde::Deserialize; +/// +/// #[derive(Deserialize, JsonSchema)] +/// struct GreetInput { +/// name: String, +/// } /// -/// let server = McpServer::builder("my-server".to_string()) +/// let server = McpServer::::builder("my-server") +/// .version("1.0.0") /// .instructions("A helpful assistant") -/// .tool(EchoTool) /// .tool_fn( /// "greet", /// "Greet someone by name", @@ -48,6 +76,7 @@ use agent_client_protocol::{ /// agent_client_protocol_rmcp::rmcp_3::tool_fn!(), /// ) /// .build(); +/// # drop(server); /// ``` #[derive(Debug)] pub struct McpServerBuilder @@ -56,6 +85,7 @@ where { phantom: PhantomData, name: String, + version: String, data: McpToolRegistry, runner: Runner, } @@ -64,6 +94,7 @@ impl McpServerBuilder { pub(super) fn new(name: String) -> Self { Self { name, + version: DEFAULT_SERVER_VERSION.to_string(), phantom: PhantomData, data: McpToolRegistry::default(), runner: NullRun, @@ -75,6 +106,15 @@ impl McpServerBuilder where Runner: RunWithConnectionTo, { + /// Set the version reported in the MCP `serverInfo`. + /// + /// Defaults to `0.0.0`. + #[must_use] + pub fn version(mut self, version: impl ToString) -> Self { + self.version = version.to_string(); + self + } + /// Set the server instructions that are provided to the client. #[must_use] pub fn instructions(mut self, instructions: impl ToString) -> Self { @@ -107,16 +147,20 @@ where /// Disable a specific tool by name. /// - /// Returns an error if the tool is not registered. - pub fn disable_tool(mut self, name: &str) -> Result { + /// # Errors + /// + /// Returns [`UnknownToolError`] if no tool with this name has been added. + pub fn disable_tool(mut self, name: &str) -> Result { self.data.disable_tool(name)?; Ok(self) } /// Enable a specific tool by name. /// - /// Returns an error if the tool is not registered. - pub fn enable_tool(mut self, name: &str) -> Result { + /// # Errors + /// + /// Returns [`UnknownToolError`] if no tool with this name has been added. + pub fn enable_tool(mut self, name: &str) -> Result { self.data.enable_tool(name)?; Ok(self) } @@ -132,6 +176,7 @@ where McpServerBuilder { phantom: PhantomData, name: this.name, + version: this.version, data: this.data, runner: ChainRun::new(this.runner, tool_runner), } @@ -151,13 +196,30 @@ where /// /// # Examples /// - /// ```rust,ignore - /// McpServer::builder("my-server") + /// ``` + /// use agent_client_protocol::{mcp_server::McpServer, role::mcp}; + /// use agent_client_protocol_rmcp::rmcp_3::McpServerExt; + /// use schemars::JsonSchema; + /// use serde::Deserialize; + /// + /// #[derive(Deserialize, JsonSchema)] + /// struct CountInput { + /// amount: u32, + /// } + /// + /// let mut total = 0; + /// let server = McpServer::::builder("my-server") /// .tool_fn_mut( - /// "greet", - /// "Greet someone by name", - /// async |input: GreetInput, _cx| Ok(format!("Hello, {}!", input.name)), + /// "count", + /// "Add to a running total", + /// async move |input: CountInput, _cx| { + /// total += input.amount; + /// Ok(total) + /// }, + /// agent_client_protocol_rmcp::rmcp_3::tool_fn_mut!(), /// ) + /// .build(); + /// # drop(server); /// ``` pub fn tool_fn_mut( self, @@ -194,13 +256,26 @@ where /// /// # Examples /// - /// ```rust,ignore - /// McpServer::builder("my-server") + /// ``` + /// use agent_client_protocol::{mcp_server::McpServer, role::mcp}; + /// use agent_client_protocol_rmcp::rmcp_3::McpServerExt; + /// use schemars::JsonSchema; + /// use serde::Deserialize; + /// + /// #[derive(Deserialize, JsonSchema)] + /// struct GreetInput { + /// name: String, + /// } + /// + /// let server = McpServer::::builder("my-server") /// .tool_fn( /// "greet", /// "Greet someone by name", /// async |input: GreetInput, _cx| Ok(format!("Hello, {}!", input.name)), + /// agent_client_protocol_rmcp::rmcp_3::tool_fn!(), /// ) + /// .build(); + /// # drop(server); /// ``` pub fn tool_fn( self, @@ -236,46 +311,62 @@ where pub fn build(self) -> McpServer { let built = McpServerBuilt { name: self.name, + version: self.version, data: Arc::new(self.data), }; - #[cfg(feature = "unstable_mcp_over_acp")] - { - let standalone = McpServerBuilt { - name: built.name.clone(), - data: built.data.clone(), - }; - McpServer::new_service_with_standalone(built, standalone, self.runner) - } - #[cfg(not(feature = "unstable_mcp_over_acp"))] - { - McpServer::new(built, self.runner) + acp::__if_mcp_over_acp! { + { + let standalone = McpServerBuilt { + name: built.name.clone(), + version: built.version.clone(), + data: built.data.clone(), + }; + McpServer::new_service_with_standalone(built, standalone, self.runner) + } else { + McpServer::new(built, self.runner) + } } } } struct McpServerBuilt { name: String, + version: String, data: Arc>, } -#[cfg(feature = "unstable_mcp_over_acp")] -impl McpService for McpServerBuilt { - fn execute( +impl McpServerBuilt { + fn connection( &self, - request: McpRequest, - context: McpRequestContext, - ) -> BoxFuture<'static, Result> { - super::native::execute( - Arc::new(McpServerConnection { - data: self.data.clone(), - mcp_connection: context.connection().clone(), - }), - request, - context, - ) + mcp_connection: McpConnectionTo, + ) -> McpServerConnection { + McpServerConnection { + name: self.name.clone(), + version: self.version.clone(), + data: self.data.clone(), + mcp_connection, + } } } +acp::__if_mcp_over_acp! { + { + impl McpService for McpServerBuilt { + fn execute( + &self, + request: McpRequest, + context: McpRequestContext, + ) -> BoxFuture<'static, Result> { + super::native::execute( + Arc::new(self.connection(context.connection().clone())), + request, + context, + ) + } + } + } else {} +} + impl McpServerConnect for McpServerBuilt { fn name(&self) -> String { self.name.clone() @@ -285,15 +376,14 @@ impl McpServerConnect for McpServerBuilt, ) -> DynConnectTo { - DynConnectTo::new(McpServerConnection { - data: self.data.clone(), - mcp_connection, - }) + DynConnectTo::new(self.connection(mcp_connection)) } } /// A connected MCP server instance. pub(crate) struct McpServerConnection { + name: String, + version: String, data: Arc>, mcp_connection: McpConnectionTo, } @@ -367,7 +457,13 @@ impl ServerHandler for McpServerConnection { Ok(CallToolResult::success(vec![content]).into()) } } - Err(error) => Err(to_rmcp_error(error)), + // Tool failures, including arguments that do not match the + // input schema, are tool results the model can act on, not + // protocol errors. + Err(error) => Ok(CallToolResult::error(vec![rmcp::model::ContentBlock::text( + error.to_string(), + )]) + .into()), }, // If cancelled @@ -407,7 +503,10 @@ impl ServerHandler for McpServerConnection { .enable_tools() .build(), ) - .with_server_info(rmcp::model::Implementation::default()) + .with_server_info(rmcp::model::Implementation::new( + self.name.clone(), + self.version.clone(), + )) .with_protocol_version(rmcp::model::ProtocolVersion::default()); if let Some(instructions) = self.data.instructions() { @@ -436,12 +535,3 @@ fn make_tool_model(metadata: &McpToolMetadata) -> Tool { tool } - -/// Convert an [`agent_client_protocol::Error`] into an [`rmcp::ErrorData`]. -fn to_rmcp_error(error: acp::Error) -> rmcp::ErrorData { - rmcp::ErrorData { - code: rmcp::model::ErrorCode(error.code.into()), - message: error.message.into(), - data: error.data, - } -} diff --git a/src/agent-client-protocol-rmcp/src/rmcp_3/mod.rs b/src/agent-client-protocol-rmcp/src/rmcp_3/mod.rs index 43f9596c..b4db9628 100644 --- a/src/agent-client-protocol-rmcp/src/rmcp_3/mod.rs +++ b/src/agent-client-protocol-rmcp/src/rmcp_3/mod.rs @@ -42,28 +42,43 @@ use ::rmcp_3 as rmcp; use agent_client_protocol::mcp_server::{McpConnectionTo, McpServer, McpServerConnect}; -#[cfg(feature = "unstable_mcp_over_acp")] -use agent_client_protocol::mcp_server::{McpOutcome, McpRequest, McpRequestContext, McpService}; use agent_client_protocol::role; use agent_client_protocol::{ByteStreams, ConnectTo, DynConnectTo, NullRun, Role}; -#[cfg(feature = "unstable_mcp_over_acp")] -use futures::future::BoxFuture; use futures_concurrency::future::TryJoin as _; use rmcp::ServiceExt; -#[cfg(feature = "unstable_mcp_over_acp")] -use std::sync::{Arc, OnceLock}; use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; mod builder; -#[cfg(feature = "unstable_mcp_over_acp")] -mod native; -pub use agent_client_protocol::mcp_server::{EnabledTools, McpTool}; +// The native MCP-over-ACP path follows the core crate's +// `unstable_mcp_over_acp` feature, not this crate's passthrough feature, so a +// feature-unified build never falls back to bridging a fresh rmcp service for +// every request. +agent_client_protocol::__if_mcp_over_acp! { + { + mod native; + use agent_client_protocol::mcp_server::{McpOutcome, McpRequest, McpRequestContext, McpService}; + use futures::future::BoxFuture; + use std::sync::{Arc, OnceLock}; + } else {} +} + +pub use agent_client_protocol::mcp_server::{McpTool, UnknownToolError}; pub use agent_client_protocol::{tool_fn, tool_fn_mut}; pub use builder::McpServerBuilder; +mod sealed { + pub trait Sealed {} +} + +impl sealed::Sealed for McpServer {} + /// Extension constructors for MCP servers backed by `rmcp`. -pub trait McpServerExt { +/// +/// This trait is sealed: it is implemented for +/// [`McpServer`] and cannot be +/// implemented outside this crate. +pub trait McpServerExt: sealed::Sealed { /// Create an MCP server builder for defining tools in Rust code. fn builder(name: impl ToString) -> McpServerBuilder { McpServerBuilder::new(name.to_string()) @@ -90,29 +105,6 @@ pub trait McpServerExt { new_fn: F, } - #[cfg(feature = "unstable_mcp_over_acp")] - struct SharedRmcp { - new_fn: Arc, - service: OnceLock>, - } - - #[cfg(feature = "unstable_mcp_over_acp")] - impl McpService for SharedRmcp - where - Counterpart: Role, - F: Fn() -> S + Send + Sync + 'static, - S: rmcp::Service, - { - fn execute( - &self, - request: McpRequest, - context: McpRequestContext, - ) -> BoxFuture<'static, Result> { - let service = self.service.get_or_init(|| Arc::new((self.new_fn)())); - native::execute(service.clone(), request, context) - } - } - impl McpServerConnect for RmcpServer where Counterpart: Role, @@ -132,32 +124,52 @@ pub trait McpServerExt { } } - #[cfg(feature = "unstable_mcp_over_acp")] - { - // Standalone use under feature unification must not initialize an - // unused native service. The factory remains per-connection there. - let new_fn = Arc::new(new_fn); - McpServer::new_service_with_standalone( - SharedRmcp { - new_fn: new_fn.clone(), - service: OnceLock::new(), - }, - RmcpServer { - name: name.to_string(), - new_fn: move || new_fn(), - }, - NullRun, - ) - } - #[cfg(not(feature = "unstable_mcp_over_acp"))] - { - McpServer::new( - RmcpServer { - name: name.to_string(), - new_fn, - }, - NullRun, - ) + agent_client_protocol::__if_mcp_over_acp! { + { + struct SharedRmcp { + new_fn: Arc, + service: OnceLock>, + } + + impl McpService for SharedRmcp + where + Counterpart: Role, + F: Fn() -> S + Send + Sync + 'static, + S: rmcp::Service, + { + fn execute( + &self, + request: McpRequest, + context: McpRequestContext, + ) -> BoxFuture<'static, Result> { + let service = self.service.get_or_init(|| Arc::new((self.new_fn)())); + native::execute(service.clone(), request, context) + } + } + + // Standalone use under feature unification must not initialize an + // unused native service. The factory remains per-connection there. + let new_fn = Arc::new(new_fn); + McpServer::new_service_with_standalone( + SharedRmcp { + new_fn: new_fn.clone(), + service: OnceLock::new(), + }, + RmcpServer { + name: name.to_string(), + new_fn: move || new_fn(), + }, + NullRun, + ) + } else { + McpServer::new( + RmcpServer { + name: name.to_string(), + new_fn, + }, + NullRun, + ) + } } } } diff --git a/src/agent-client-protocol-rmcp/tests/modern_mcp.rs b/src/agent-client-protocol-rmcp/tests/modern_mcp.rs index 03bf22d1..7c07294e 100644 --- a/src/agent-client-protocol-rmcp/tests/modern_mcp.rs +++ b/src/agent-client-protocol-rmcp/tests/modern_mcp.rs @@ -127,6 +127,104 @@ async fn built_server_handles_modern_calls_before_discovery() -> Result<(), Erro .expect("modern adapter test timed out") } +#[test] +fn enabling_or_disabling_an_unknown_tool_is_a_local_error() { + let builder = McpServer::::builder("tools").tool_fn( + "echo", + "Echo a message", + async |input: Echo, _cx| Ok(input), + agent_client_protocol_rmcp::rmcp_3::tool_fn!(), + ); + let Err(error) = builder.disable_tool("ech") else { + panic!("a typo must be reported"); + }; + assert_eq!(error.name(), "ech"); + assert_eq!(error.to_string(), "unknown MCP tool `ech`"); +} + +#[tokio::test] +async fn built_server_reports_its_name_and_tool_errors_as_results() -> Result<(), Error> { + tokio::time::timeout(TIMEOUT, async { + let server = McpServer::::builder("named-tools") + .version("1.2.3") + .tool_fn( + "echo", + "Echo a message", + async |input: Echo, _cx| Ok(input), + agent_client_protocol_rmcp::rmcp_3::tool_fn!(), + ) + .tool_fn( + "fail", + "Always fails", + async |_input: Echo, _cx| -> Result { + Err(Error::internal_error().data("tool exploded")) + }, + agent_client_protocol_rmcp::rmcp_3::tool_fn!(), + ) + .build(); + + mcp::Client + .builder() + .connect_with(server, async |connection| { + let discovery = request( + &connection, + "server/discover", + params(json!({}), "discover"), + ) + .await?; + assert_eq!( + discovery["_meta"]["io.modelcontextprotocol/serverInfo"], + json!({"name": "named-tools", "version": "1.2.3"}) + ); + + let failed = request( + &connection, + "tools/call", + params( + json!({"name": "fail", "arguments": {"message": "x"}}), + "fail", + ), + ) + .await?; + assert_eq!(failed["isError"], true); + assert!( + failed["content"][0]["text"] + .as_str() + .is_some_and(|text| text.contains("tool exploded")) + ); + + let bad_arguments = request( + &connection, + "tools/call", + params( + json!({"name": "echo", "arguments": {"wrong": 1}}), + "bad-arguments", + ), + ) + .await?; + assert_eq!(bad_arguments["isError"], true); + assert!( + bad_arguments["content"][0]["text"] + .as_str() + .is_some_and(|text| text.contains("message")) + ); + + let unknown = request( + &connection, + "tools/call", + params(json!({"name": "missing", "arguments": {}}), "unknown"), + ) + .await + .expect_err("an unknown tool is a protocol error"); + assert_eq!(serde_json::to_value(unknown)?["code"], -32602); + Ok(()) + }) + .await + }) + .await + .expect("named server test timed out") +} + struct RetryServer; impl ServerHandler for RetryServer { diff --git a/src/agent-client-protocol/src/mcp_server/context.rs b/src/agent-client-protocol/src/mcp_server/context.rs index 007714fd..f84930a6 100644 --- a/src/agent-client-protocol/src/mcp_server/context.rs +++ b/src/agent-client-protocol/src/mcp_server/context.rs @@ -16,6 +16,7 @@ pub enum McpConnectionContext { /// The MCP server was attached to an ACP session. #[cfg(feature = "unstable_mcp_over_acp")] + #[non_exhaustive] Acp { /// The identifier advertised in the session's `McpServer::Acp` declaration. server_id: McpServerAcpId, diff --git a/src/agent-client-protocol/src/mcp_server/mod.rs b/src/agent-client-protocol/src/mcp_server/mod.rs index 44dccff7..68e551dd 100644 --- a/src/agent-client-protocol/src/mcp_server/mod.rs +++ b/src/agent-client-protocol/src/mcp_server/mod.rs @@ -70,7 +70,7 @@ pub use context::{McpConnectionContext, McpConnectionTo}; #[cfg(feature = "schemars")] #[cfg_attr(docsrs, doc(cfg(feature = "schemars")))] pub use registry::{ - EnabledTools, McpToolMetadata, McpToolRegistry, McpToolSchema, RegisteredMcpTool, + McpToolMetadata, McpToolRegistry, McpToolSchema, RegisteredMcpTool, UnknownToolError, }; pub use server::McpServer; #[cfg(feature = "unstable_mcp_over_acp")] @@ -90,3 +90,35 @@ pub use tool::McpTool; #[cfg(feature = "schemars")] #[cfg_attr(docsrs, doc(cfg(feature = "schemars")))] pub use tool_fn::{tool_fn, tool_fn_mut}; + +/// Expands to the first block when this crate was built with +/// `unstable_mcp_over_acp`, and to the `else` block otherwise. +/// +/// MCP integrations in other crates use this to pick the native +/// MCP-over-ACP path from this crate's features rather than their own, so +/// feature unification cannot leave them on the per-request fallback while +/// ACP attachment is available. Not part of the public API. +#[cfg(feature = "unstable_mcp_over_acp")] +#[doc(hidden)] +#[macro_export] +macro_rules! __if_mcp_over_acp { + ({ $($then:tt)* } else { $($else:tt)* }) => { + $($then)* + }; +} + +/// Expands to the first block when this crate was built with +/// `unstable_mcp_over_acp`, and to the `else` block otherwise. +/// +/// MCP integrations in other crates use this to pick the native +/// MCP-over-ACP path from this crate's features rather than their own, so +/// feature unification cannot leave them on the per-request fallback while +/// ACP attachment is available. Not part of the public API. +#[cfg(not(feature = "unstable_mcp_over_acp"))] +#[doc(hidden)] +#[macro_export] +macro_rules! __if_mcp_over_acp { + ({ $($then:tt)* } else { $($else:tt)* }) => { + $($else)* + }; +} diff --git a/src/agent-client-protocol/src/mcp_server/registry.rs b/src/agent-client-protocol/src/mcp_server/registry.rs index c012edcc..74d9478f 100644 --- a/src/agent-client-protocol/src/mcp_server/registry.rs +++ b/src/agent-client-protocol/src/mcp_server/registry.rs @@ -19,7 +19,7 @@ pub type McpToolSchema = Map; /// - `DenyList`: All tools enabled except those in the set (default) /// - `AllowList`: Only tools in the set are enabled #[derive(Clone, Debug)] -pub enum EnabledTools { +enum EnabledTools { /// All tools enabled except those in the deny set. DenyList(HashSet), /// Only tools in the allow set are enabled. @@ -34,8 +34,7 @@ impl Default for EnabledTools { impl EnabledTools { /// Check if a tool is enabled. - #[must_use] - pub fn is_enabled(&self, name: &str) -> bool { + fn is_enabled(&self, name: &str) -> bool { match self { EnabledTools::DenyList(deny) => !deny.contains(name), EnabledTools::AllowList(allow) => allow.contains(name), @@ -43,6 +42,46 @@ impl EnabledTools { } } +/// [`McpToolRegistry::enable_tool`] or [`McpToolRegistry::disable_tool`] named +/// a tool that is not registered. +/// +/// This is a local configuration error, usually a typo, not a JSON-RPC error. +/// Converting it into an [`Error`] with `?` produces an internal error +/// (`-32603`) whose data names the tool. +#[derive(Clone, Debug, PartialEq, Eq)] +#[non_exhaustive] +pub struct UnknownToolError { + name: String, +} + +impl UnknownToolError { + fn new(name: &str) -> Self { + Self { + name: name.to_string(), + } + } + + /// The tool name that is not registered. + #[must_use] + pub fn name(&self) -> &str { + &self.name + } +} + +impl std::fmt::Display for UnknownToolError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "unknown MCP tool `{}`", self.name) + } +} + +impl std::error::Error for UnknownToolError {} + +impl From for Error { + fn from(error: UnknownToolError) -> Self { + Error::internal_error().data(error.to_string()) + } +} + /// Runtime-neutral metadata for an MCP tool. #[derive(Clone, Debug)] pub struct McpToolMetadata { @@ -138,6 +177,12 @@ impl RegisteredMcpTool { } /// Invoke the registered tool using JSON input and output values. + /// + /// # Errors + /// + /// Returns an invalid params error (`-32602`) if `input` does not + /// deserialize into the tool's input type, an internal error (`-32603`) if + /// the output does not serialize, and otherwise the tool's own error. pub fn call_tool( &self, input: Value, @@ -239,10 +284,12 @@ impl McpToolRegistry { /// Disable a specific tool by name. /// - /// Returns an error if the tool is not registered. - pub fn disable_tool(&mut self, name: &str) -> Result<(), Error> { + /// # Errors + /// + /// Returns [`UnknownToolError`] if no tool with this name is registered. + pub fn disable_tool(&mut self, name: &str) -> Result<(), UnknownToolError> { if !self.contains_tool(name) { - return Err(Error::invalid_request().data(format!("unknown tool: {name}"))); + return Err(UnknownToolError::new(name)); } match &mut self.enabled_tools { EnabledTools::DenyList(deny) => { @@ -257,10 +304,12 @@ impl McpToolRegistry { /// Enable a specific tool by name. /// - /// Returns an error if the tool is not registered. - pub fn enable_tool(&mut self, name: &str) -> Result<(), Error> { + /// # Errors + /// + /// Returns [`UnknownToolError`] if no tool with this name is registered. + pub fn enable_tool(&mut self, name: &str) -> Result<(), UnknownToolError> { if !self.contains_tool(name) { - return Err(Error::invalid_request().data(format!("unknown tool: {name}"))); + return Err(UnknownToolError::new(name)); } match &mut self.enabled_tools { EnabledTools::DenyList(deny) => { @@ -303,9 +352,10 @@ where context: McpConnectionTo, ) -> BoxFuture<'_, Result> { Box::pin(async move { - let input = serde_json::from_value(input).map_err(crate::util::internal_error)?; + let input = serde_json::from_value(input) + .map_err(|error| Error::invalid_params().data(error.to_string()))?; serde_json::to_value(self.tool.call_tool(input, context).await?) - .map_err(crate::util::internal_error) + .map_err(|error| Error::internal_error().data(error.to_string())) }) } } From 70020fd16ffeadead64ba3e82510d12d2f91530d Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:59:02 +0100 Subject: [PATCH 3/8] fix(acp)!: narrow AcpAgent and deflake its SIGKILL shutdown test - AcpAgent::spawn_process is pub(crate). It exposed async_process types from a private optional dependency and skipped the process-group guard. - Remove AcpAgent::claude_agent() and AcpAgent::codex(), which ran unpinned npx @latest adapters. Use AcpAgent::from_str with a version. - Shutdown test scripts install their TERM trap before writing the PID file that starts shutdown, and the SIGKILL test gives the trap a 1s terminate window, so a loaded machine still records SIGTERM. BREAKING CHANGE: AcpAgent::spawn_process, AcpAgent::claude_agent, and AcpAgent::codex are no longer public. Co-Authored-By: Claude --- md/migration-v5.md | 17 +++++++++ src/agent-client-protocol/src/acp_agent.rs | 40 ++++++++-------------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index 823b829f..e08e882e 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -843,3 +843,20 @@ breaking change and only happens in an ACP major release. The crate re-exports `axum` (with `server`) and `reqwest` and `url` (with `client`), so callers can name `axum::Router`, `reqwest::ClientBuilder`, and `url::ParseError` without adding matching dependencies. + +## `AcpAgent` + +- `AcpAgent::spawn_process` is no longer public. It returned `async_process` + types and skipped the process-group teardown that `ConnectTo` installs. + Connect the `AcpAgent` itself instead. +- `AcpAgent::claude_agent()` and `AcpAgent::codex()` are removed. They ran + `npx -y ...@latest`, which installs whatever version was published last. + Name the adapter version yourself: + +```rust +use std::str::FromStr; +use agent_client_protocol::AcpAgent; + +let agent = AcpAgent::from_str("npx -y @agentclientprotocol/claude-agent-acp@")?; +# Ok::<(), agent_client_protocol::Error>(()) +``` diff --git a/src/agent-client-protocol/src/acp_agent.rs b/src/agent-client-protocol/src/acp_agent.rs index b076c9b8..65feeb4a 100644 --- a/src/agent-client-protocol/src/acp_agent.rs +++ b/src/agent-client-protocol/src/acp_agent.rs @@ -239,21 +239,6 @@ impl AcpAgent { } } - /// Create an ACP agent for the Claude Agent adapter. - /// Just runs `npx -y @agentclientprotocol/claude-agent-acp@latest`. - #[must_use] - pub fn claude_agent() -> Self { - Self::from_str("npx -y @agentclientprotocol/claude-agent-acp@latest") - .expect("valid bash command") - } - - /// Create an ACP agent for the Codex adapter. - /// Just runs `npx -y @agentclientprotocol/codex-acp@latest`. - #[must_use] - pub fn codex() -> Self { - Self::from_str("npx -y @agentclientprotocol/codex-acp@latest").expect("valid bash command") - } - /// Get the process-launch configuration. #[must_use] pub fn config(&self) -> &AcpAgentConfig { @@ -294,10 +279,9 @@ impl AcpAgent { /// Spawn the configured process and return its stdio streams and raw child handle. /// - /// This is a low-level escape hatch. The caller owns the returned child process and is - /// responsible for terminating it. Connections created through [`crate::ConnectTo`] instead - /// install a guard that tears down the spawned process group on Unix. - pub fn spawn_process( + /// The caller owns the returned child process and is responsible for terminating it. + /// [`crate::ConnectTo`] wraps it in a guard that tears down the process group on Unix. + pub(crate) fn spawn_process( &self, ) -> Result< ( @@ -1725,6 +1709,9 @@ mod tests { } /// An agent running `script` as `sh -c script sh `. + /// + /// Scripts install their `TERM` trap before writing the PID file, since + /// [`shut_down_once_started`] starts shutdown as soon as that file exists. fn agent(&self, script: &str, timeouts: ShutdownTimeouts) -> AcpAgent { let mut agent = AcpAgent::from_args([ "/bin/sh", @@ -1785,7 +1772,7 @@ mod tests { async fn graceful_shutdown_child_exiting_on_stdin_eof_never_receives_sigterm() { let markers = ShutdownMarkers::new("eof"); let agent = markers.agent( - r#"echo $$ > "$1"; trap 'echo term > "$2"' TERM; cat >/dev/null; echo eof > "$1.eof"; exit 0"#, + r#"trap 'echo term > "$2"' TERM; echo $$ > "$1"; cat >/dev/null; echo eof > "$1.eof"; exit 0"#, ShutdownTimeouts { exit: Duration::from_secs(5), terminate: Duration::from_secs(5), @@ -1812,7 +1799,7 @@ mod tests { async fn graceful_shutdown_sigterms_child_ignoring_stdin_eof() { let markers = ShutdownMarkers::new("term"); let agent = markers.agent( - r#"echo $$ > "$1"; trap 'echo term > "$2"; exit 0' TERM; while :; do sleep 1; done"#, + r#"trap 'echo term > "$2"; exit 0' TERM; echo $$ > "$1"; while :; do sleep 1; done"#, ShutdownTimeouts { exit: Duration::from_millis(200), terminate: Duration::from_secs(5), @@ -1838,12 +1825,15 @@ mod tests { async fn graceful_shutdown_sigkills_child_ignoring_sigterm() { let markers = ShutdownMarkers::new("kill"); // The trap records SIGTERM but keeps running; `sleep` itself is - // killed by the group SIGTERM, and the loop starts another one. + // killed by the group SIGTERM, and the loop starts another one. The + // trap is installed before the PID is written, so shutdown cannot + // start before it. The terminate window leaves a loaded machine time + // to run the trap before SIGKILL. let agent = markers.agent( - r#"echo $$ > "$1"; trap 'echo term > "$2"' TERM; while :; do sleep 1; done"#, + r#"trap 'echo term > "$2"' TERM; echo $$ > "$1"; while :; do sleep 1; done"#, ShutdownTimeouts { exit: Duration::from_millis(100), - terminate: Duration::from_millis(300), + terminate: Duration::from_secs(1), }, ); @@ -1854,7 +1844,7 @@ mod tests { result.expect("an agent killed during shutdown is not an error"); assert!(markers.received_sigterm(), "SIGTERM should precede SIGKILL"); assert!( - elapsed >= Duration::from_millis(400), + elapsed >= Duration::from_millis(1100), "SIGKILL should wait for the SIGTERM grace period ({elapsed:?})" ); assert_process_exits(pid).await; From bc03fe2e75787b0d9771ff95d47ca8f55ca3c9db Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 12:00:41 +0100 Subject: [PATCH 4/8] fix(acp): answer unmatched requests in MatchDispatch with -32601 MatchDispatch::otherwise_ignore dropped an unmatched request, which now answers -32603. It answers -32601 instead, and still ignores unmatched notifications and responses. The MatchDispatch and MatchDispatchFrom examples use Error::method_not_found(), and the if_request / if_notification docs describe what happens when params fail to parse instead of saying the call has no effect or naming handle_if. Co-Authored-By: Claude --- src/agent-client-protocol/src/util/typed.rs | 51 +++++++++++------ .../tests/match_dispatch.rs | 55 +++++++++++++++++++ 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/src/agent-client-protocol/src/util/typed.rs b/src/agent-client-protocol/src/util/typed.rs index 86ab013c..244326e9 100644 --- a/src/agent-client-protocol/src/util/typed.rs +++ b/src/agent-client-protocol/src/util/typed.rs @@ -61,7 +61,7 @@ fn preserve_retry( /// .otherwise(|message| async move { /// match message { /// Dispatch::Request(_, responder) => { -/// responder.respond_with_error(agent_client_protocol::util::internal_error("unknown method")) +/// responder.respond_with_error(agent_client_protocol::Error::method_not_found()) /// } /// Dispatch::Notification(_) | Dispatch::Response(_, _) => Ok(()), /// } @@ -88,9 +88,11 @@ impl MatchDispatch { /// Try to handle the message as a request of type `Req`. /// - /// If the message can be parsed as `Req`, the handler `op` is called with the parsed - /// request and a typed request context. If parsing fails or the message was already - /// handled by a previous call, this has no effect. + /// If the message is a request for one of `Req`'s methods, it is parsed and + /// `op` is called with the request and a typed responder. If the method + /// matches but the params do not parse, matching stops with the parse + /// error. Other messages, and messages already handled by a previous call, + /// pass through unchanged. pub async fn if_request( mut self, op: impl AsyncFnOnce(Req, Responder) -> Result, @@ -150,8 +152,11 @@ impl MatchDispatch { /// Try to handle the message as a notification of type `N`. /// - /// If the message can be parsed as `N`, the handler `op` is called with the parsed - /// notification. If parsing fails or the message was already handled, this has no effect. + /// If the message is a notification for one of `N`'s methods, it is parsed + /// and `op` is called with the notification. If the method matches but the + /// params do not parse, matching stops with the parse error. Other + /// messages, and messages already handled by a previous call, pass through + /// unchanged. pub async fn if_notification( mut self, op: impl AsyncFnOnce(N) -> Result, @@ -404,9 +409,15 @@ impl MatchDispatch { } } - /// Handle messages that didn't match any previous handler. + /// Finish matching, answering an unmatched request with a method not + /// found error (`-32601`) and ignoring an unmatched notification or + /// response. pub fn otherwise_ignore(self) -> Result<(), crate::Error> { match self.state { + Ok(Handled::No { + message: Dispatch::Request(_, responder), + retry: _, + }) => responder.respond_with_error(crate::Error::method_not_found()), Ok(_) => Ok(()), Err(err) => Err(err), } @@ -451,7 +462,7 @@ impl MatchDispatch { /// .otherwise(|message| async move { /// // Fallback for unrecognized messages /// match message { -/// Dispatch::Request(_, responder) => responder.respond_with_error(agent_client_protocol::util::internal_error("unknown method")), +/// Dispatch::Request(_, responder) => responder.respond_with_error(agent_client_protocol::Error::method_not_found()), /// Dispatch::Notification(_) | Dispatch::Response(_, _) => Ok(()), /// } /// }) @@ -479,14 +490,16 @@ impl MatchDispatchFrom { /// Try to handle the message as a request of type `Req`. /// - /// If the message can be parsed as `Req`, the handler `op` is called with the parsed - /// request and a typed request context. If parsing fails or the message was already - /// handled by a previous `handle_if`, this call has no effect. + /// If the message is a request for one of `Req`'s methods, it is parsed and + /// `op` is called with the request and a typed responder. If the method + /// matches but the params do not parse, matching stops with the parse + /// error. Other messages, and messages already handled by a previous + /// `if_*` call, pass through unchanged. /// /// The handler can return either `()` (which becomes `Handled::Yes`) or an explicit /// `Handled` value to control whether the message should be passed to the next handler. /// - /// Returns `self` to allow chaining multiple `handle_if` calls. + /// Returns `self` to allow chaining multiple `if_*` calls. pub async fn if_request( self, op: impl AsyncFnOnce(Req, Responder) -> Result, @@ -537,14 +550,16 @@ impl MatchDispatchFrom { /// Try to handle the message as a notification of type `N`. /// - /// If the message can be parsed as `N`, the handler `op` is called with the parsed - /// notification and connection context. If parsing fails or the message was already - /// handled by a previous `handle_if`, this call has no effect. + /// If the message is a notification for one of `N`'s methods, it is parsed + /// and `op` is called with the notification. If the method matches but the + /// params do not parse, matching stops with the parse error. Other + /// messages, and messages already handled by a previous `if_*` call, pass + /// through unchanged. /// /// The handler can return either `()` (which becomes `Handled::Yes`) or an explicit /// `Handled` value to control whether the message should be passed to the next handler. /// - /// Returns `self` to allow chaining multiple `handle_if` calls. + /// Returns `self` to allow chaining multiple `if_*` calls. pub async fn if_notification( self, op: impl AsyncFnOnce(N) -> Result, @@ -768,7 +783,7 @@ impl MatchDispatchFrom { } } - /// Handle messages that didn't match any previous `handle_if` call. + /// Handle messages that didn't match any previous `if_*` call. /// /// This is the fallback handler that receives the original untyped message if none /// of the typed handlers matched. You must call this method to complete the pattern @@ -784,7 +799,7 @@ impl MatchDispatchFrom { } } - /// Handle messages that didn't match any previous `handle_if` call. + /// Handle messages that didn't match any previous `if_*` call. /// /// This is the fallback handler that receives the original untyped message if none /// of the typed handlers matched. You must call this method to complete the pattern diff --git a/src/agent-client-protocol/tests/match_dispatch.rs b/src/agent-client-protocol/tests/match_dispatch.rs index 3669a895..3ad0f0ea 100644 --- a/src/agent-client-protocol/tests/match_dispatch.rs +++ b/src/agent-client-protocol/tests/match_dispatch.rs @@ -402,3 +402,58 @@ async fn modify_message_and_stop() -> Result<(), agent_client_protocol::Error> { }) .await } + +#[tokio::test] +async fn otherwise_ignore_answers_unmatched_requests_with_method_not_found() +-> Result<(), agent_client_protocol::Error> { + struct IgnoreHandler; + + impl HandleDispatchFrom for IgnoreHandler { + async fn handle_dispatch_from( + &mut self, + message: Dispatch, + _connection: ConnectionTo, + ) -> Result, agent_client_protocol::Error> { + MatchDispatch::new(message) + .if_notification(async |_: RetryNotification| Ok(())) + .await + .otherwise_ignore()?; + Ok(Handled::Yes) + } + + fn describe_chain(&self) -> impl std::fmt::Debug { + "IgnoreHandler" + } + } + + struct TestComponent; + + impl ConnectTo for TestComponent { + async fn connect_to( + self, + client: impl ConnectTo, + ) -> Result<(), agent_client_protocol::Error> { + UntypedRole + .builder() + .with_handler(IgnoreHandler) + .connect_to(client) + .await + } + } + + UntypedRole + .builder() + .connect_with(TestComponent, async |cx| { + let error = cx + .send_request(EchoRequestResponse { text: vec![] }) + .block_task() + .await + .expect_err("an unmatched request must get an error"); + assert_eq!( + error.code, + agent_client_protocol::Error::method_not_found().code + ); + Ok(()) + }) + .await +} From 72fcd26bb627762706dbfa93973489880307e9d6 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 12:01:43 +0100 Subject: [PATCH 5/8] docs: fix cookbook permission and dispatch recipes and document wasm Send bounds - The v1 permission recipe sends session/cancel before answering cancelled, like the v2 recipe, and now compiles as a doctest. - The custom handler recipe says that it parses a raw Dispatch as v1 types and only belongs on a v1 connection. - The WebAssembly docs say that handlers, transports, and connection futures must be Send on every target and how to keep !Send host values outside the connection. Co-Authored-By: Claude --- md/transport-architecture.md | 6 ++ src/agent-client-protocol-cookbook/src/lib.rs | 59 ++++++++++++------- src/agent-client-protocol/README.md | 5 ++ src/agent-client-protocol/src/lib.rs | 7 +++ 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/md/transport-architecture.md b/md/transport-architecture.md index 01032179..4b3a0dfc 100644 --- a/md/transport-architecture.md +++ b/md/transport-architecture.md @@ -331,6 +331,12 @@ the target does not imply a JavaScript host, other OS-less WebAssembly hosts must arrange a compatible UUID randomness backend instead. This crate does not provide a WebAssembly executor or host I/O adapter. +Handlers, transports, and connection futures must be `Send` on every target, +including WebAssembly. JavaScript host values such as `wasm_bindgen::JsValue` +are `!Send`, so keep them outside the connection and exchange `Send` data with +it through `Channel` or `ByteStreams`. A later release may relax these bounds +on WebAssembly. That change would not affect native code. + The rmcp integration library also compiles for WASI with its `rmcp_3` feature, alone or with its MCP-over-ACP features. It uses Tokio's async I/O utilities and single-thread-capable runtime without requiring native stdio or a multithreaded diff --git a/src/agent-client-protocol-cookbook/src/lib.rs b/src/agent-client-protocol-cookbook/src/lib.rs index e194d308..7f12ebf1 100644 --- a/src/agent-client-protocol-cookbook/src/lib.rs +++ b/src/agent-client-protocol-cookbook/src/lib.rs @@ -362,30 +362,44 @@ pub mod connecting_as_client { //! # Handling Permission Requests //! //! Agents may send [`RequestPermissionRequest`] to ask for user approval - //! before taking actions. Handle these with [`on_receive_request`]: + //! before taking actions. Handle these with [`on_receive_request`]. Answer + //! `cancelled` only after cancelling the turn with `session/cancel`: //! - //! ```ignore - //! Client.v1() - //! .on_receive_request(async |req: RequestPermissionRequest, responder, _connection| { - //! // Auto-approve (YOLO mode) by option `kind`, never by position - //! // or name: the agent decides which options exist and in what order. - //! let allow = req.options.iter().find(|option| { - //! matches!( - //! option.kind, - //! PermissionOptionKind::AllowOnce | PermissionOptionKind::AllowAlways - //! ) - //! }); - //! responder.respond(RequestPermissionResponse::new( - //! match allow { + //! ``` + //! use agent_client_protocol::schema::v1::{ + //! CancelNotification, PermissionOptionKind, RequestPermissionOutcome, + //! RequestPermissionRequest, RequestPermissionResponse, SelectedPermissionOutcome, + //! }; + //! use agent_client_protocol::{Client, ConnectTo}; + //! + //! async fn run(transport: impl ConnectTo) -> Result<(), agent_client_protocol::Error> { + //! Client.v1() + //! .on_receive_request(async |req: RequestPermissionRequest, responder, connection| { + //! // Auto-approve (YOLO mode) by option `kind`, never by position + //! // or name: the agent decides which options exist and in what order. + //! let allow = req.options.iter().find(|option| { + //! matches!( + //! option.kind, + //! PermissionOptionKind::AllowOnce | PermissionOptionKind::AllowAlways + //! ) + //! }); + //! let outcome = match allow { //! Some(option) => RequestPermissionOutcome::Selected( //! SelectedPermissionOutcome::new(option.option_id.clone()), //! ), - //! None => RequestPermissionOutcome::Cancelled, - //! } - //! )) - //! }, agent_client_protocol::on_receive_request!()) - //! .connect_with(transport, async |connection| { /* ... */ }) - //! .await + //! // `cancelled` is only valid after cancelling the turn. + //! None => { + //! connection.send_notification(CancelNotification::new( + //! req.session_id.clone(), + //! ))?; + //! RequestPermissionOutcome::Cancelled + //! } + //! }; + //! responder.respond(RequestPermissionResponse::new(outcome)) + //! }, agent_client_protocol::on_receive_request!()) + //! .connect_with(transport, async |_connection| Ok(())) + //! .await + //! } //! ``` //! //! # Note on `block_task` @@ -623,6 +637,11 @@ pub mod custom_message_handlers { //! } //! ``` //! + //! `Dispatch` is the raw, version-neutral message, so a handler decides + //! which protocol version's types to parse it as. This one parses v1 + //! `InitializeRequest`s, so install it only on a connection that speaks + //! ACP v1. A v2 handler would parse `schema::v2` types instead. + //! //! # When to use `MatchDispatch` vs `MatchDispatchFrom` //! //! - [`MatchDispatch`] - Use when you don't need peer-aware handling diff --git a/src/agent-client-protocol/README.md b/src/agent-client-protocol/README.md index 61a068cc..d3a0e267 100644 --- a/src/agent-client-protocol/README.md +++ b/src/agent-client-protocol/README.md @@ -160,6 +160,11 @@ Crypto through `wasm-bindgen` as the UUID randomness backend. The target does not imply a JavaScript host, so this feature is not enabled by default. Other OS-less WebAssembly hosts must arrange a compatible UUID randomness backend. +Handlers, transports, and connection futures must be `Send` on every target, +including WebAssembly. JavaScript host values such as `wasm_bindgen::JsValue` +are `!Send`, so keep them outside the connection and exchange `Send` data with +it through `Channel` or `ByteStreams`. + The native `AcpAgent` and `Stdio` implementations are not available on WebAssembly targets. See the [transport architecture](https://agentclientprotocol.github.io/rust-sdk/transport-architecture.html) for the runtime-neutral embedding options. diff --git a/src/agent-client-protocol/src/lib.rs b/src/agent-client-protocol/src/lib.rs index ea4321c2..ba8e1f99 100644 --- a/src/agent-client-protocol/src/lib.rs +++ b/src/agent-client-protocol/src/lib.rs @@ -151,6 +151,13 @@ //! `AsyncWrite`. The embedding runtime must drive the resulting connection //! future. //! +//! Handlers, transports, and connection futures must be `Send` on every +//! target, including WebAssembly. JavaScript host values such as +//! `wasm_bindgen::JsValue` are `!Send`, so keep them outside the connection and +//! exchange `Send` data with it through `Channel` or `ByteStreams`. A later +//! release may relax these bounds on WebAssembly. That change would not affect +//! native code. +//! //! ## Core Concepts //! //! The [`concepts`] module provides detailed explanations of how agent-client-protocol works, From ac780635992a78321fab9aeefa0c5b86497c798a Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 12:33:47 +0100 Subject: [PATCH 6/8] docs: document otherwise_ignore change and fix CORS migration text Add a migration-v5 entry for MatchDispatch::otherwise_ignore answering unmatched requests with -32601 and its effect on v1::ActiveSession::read_to_string. Rewrite the CORS section of the config migration guide so it no longer says the variants are still available. Document __if_mcp_over_acp! as a stable cross-crate contract for 5.x, and test that RegisteredMcpTool::call_tool reports bad arguments as -32602. Co-Authored-By: Claude --- md/migration-config-api.md | 7 +- md/migration-v5.md | 12 ++++ .../src/mcp_server/mod.rs | 10 ++- .../src/mcp_server/registry.rs | 68 +++++++++++++++++++ 4 files changed, 92 insertions(+), 5 deletions(-) diff --git a/md/migration-config-api.md b/md/migration-config-api.md index 63236946..d4c3e547 100644 --- a/md/migration-config-api.md +++ b/md/migration-config-api.md @@ -42,10 +42,11 @@ value and preserve the other options. ## CORS policies -Existing variants and constructors remain available. **Breaking:** exhaustive -matches on `CorsOptions` outside the HTTP crate must add a wildcard arm. +In 3.x, `CorsOptions` was a `#[non_exhaustive]` enum, so exhaustive matches +outside the HTTP crate needed a wildcard arm. -In v5, `CorsOptions` is an opaque struct and its variants are no longer public. +**Breaking:** in v5, `CorsOptions` is an opaque struct and its variants are no +longer public. Code that matched on the variants no longer compiles. Build policies with `CorsOptions::disabled()`, `CorsOptions::allow_origins(...)`, or `CorsOptions::allow_any_origin()`, and compare them with `==`: diff --git a/md/migration-v5.md b/md/migration-v5.md index e08e882e..afe89d93 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -718,6 +718,18 @@ fn respond(responder: Responder