From 543ebf5f8ab496592d9086be468ecbb4974d0836 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 10:58:49 +0100 Subject: [PATCH 01/11] fix(acp)!: make root connection conversions version-checked Remove `Builder::with_v2_protocol_guard`, a v2-named method on the version-neutral builder with no v1 or proxy counterpart and no callers. Merge `ConnectionTo::spawn_connection_with_context` into `spawn_connection`, which now returns the connection type the child builder's callbacks receive, as the v1 and v2 connections already did. `ConnectionTo::into_protocol::

()` now returns a `Result` and fails when the builder's guard selected a different version, so a v1 connection can no longer be retyped to send v2 payloads. Add `ConnectionTo::protocol_version()`. The typed `send_cancel_request(_to)` on `v1::ConnectionTo` and `v2::ConnectionTo` take the version's own request ID, so v2 can no longer send `{"requestId": null}`. `v1::ConnectionTo` and `v2::ConnectionTo` convert into the root `ConnectionTo` with `From`, and `Protocol::Connection` requires it, so `P`-generic code can reach the raw connection. Findings F01, F15, F18, F04, G82. BREAKING CHANGE: `Builder::with_v2_protocol_guard` and `ConnectionTo::spawn_connection_with_context` are removed; `ConnectionTo::spawn_connection` returns `P::Connection` and takes two type parameters; `ConnectionTo::into_protocol` returns a `Result`; the typed `send_cancel_request(_to)` methods take typed request IDs. Co-Authored-By: Claude --- md/migration-v5.md | 38 ++- md/protocol-v2.md | 25 +- src/agent-client-protocol/src/jsonrpc.rs | 245 +++++------------- .../src/jsonrpc/protocol.rs | 39 ++- .../src/jsonrpc/protocol_compat.rs | 1 - .../src/jsonrpc/v1_connection.rs | 29 ++- .../src/jsonrpc/v2_connection.rs | 30 ++- .../tests/protocol_v2.rs | 25 +- src/agent-client-protocol/tests/session_v2.rs | 22 +- .../tests/v1_connection.rs | 15 +- 10 files changed, 248 insertions(+), 221 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index 895947df..7eea13dc 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -66,7 +66,7 @@ It no longer has the v1 session helpers. Convert explicitly between the two; use agent_client_protocol::{ConnectionTo, V1, v1}; // raw -> v1, for example in a custom runner that opens a v1 session -let v1_cx: v1::ConnectionTo = raw_cx.into_protocol::(); +let v1_cx: v1::ConnectionTo = raw_cx.into_protocol::()?; v1_cx.build_session_cwd()?.block_task().start_session().await?; // v1 -> raw, for example to drive a `RunWithConnectionTo` runner @@ -89,6 +89,33 @@ fn send(cx: &ConnectionTo) -> SentRequest { ... } Where code compared a v1 request's `id()` with IDs from the raw layer, use `raw_id()`. +### Converting, spawning and cancelling on connections + +- `ConnectionTo::into_protocol::

()` returns + `Result, Error>`. It fails with `-32600` when the + connection's builder guards another version, for example + `into_protocol::()` on a connection from `Client.v1()`, which used to + succeed and then send v2 payloads over the v1 wire. Connections from + unguarded builders such as `Builder::new` convert to every version. Add `?` + to existing calls. The new `ConnectionTo::protocol_version()` returns the + guarded version, or `None` for an unguarded connection. +- `ConnectionTo::spawn_connection` returns the connection type the child + builder's callbacks receive, like `v1::ConnectionTo::spawn_connection` and + `v2::ConnectionTo::spawn_connection` already did. It replaces + `ConnectionTo::spawn_connection_with_context`, which is removed. Spawning + `Client.v1()` now returns a `v1::ConnectionTo<_>`; call `.into_raw()` where + the raw connection is wanted. A raw builder still returns the raw + `ConnectionTo`. Turbofish calls name both parameters: + `spawn_connection::(...)`. +- `v1::ConnectionTo::send_cancel_request(_to)` takes + `impl Into` and `v2::ConnectionTo::send_cancel_request(_to)` + takes `impl Into`, the type their requests' `id()` + returns. The v2 methods can no longer send `{"requestId": null}`, which v2 + forbids. Only the raw `ConnectionTo` methods take any `RawJsonRpcId`. +- `Builder::with_v2_protocol_guard` (`unstable_protocol_v2`) is removed. A + builder that needs the v2 guard is `Client.v2()` or `Agent.v2()`; code that + needs raw handles in its callbacks converts them with `into_raw()`. + ### Every session setup method returns `SessionSetup` `v1::SessionSetup` and `v2::SessionSetup` replace `v1::OpenedSession` @@ -710,8 +737,7 @@ async |message: Dispatch, cx| { ... } `Client.v1()`, `Agent.v1()` and `Proxy.v1()` return `v1::Builder<_>` (`Builder<_, _, _, _, V1>`). -`Builder::new`, other roles' builders, and `with_v2_protocol_guard()` -(which returns a `Raw` builder) use `Raw`. The v1 session helpers that take +`Builder::new` and other roles' builders use `Raw`. The v1 session helpers that take a responder, such as `v1::SessionBuilder::on_proxy_session_start`, take a `v1::Responder<_>`. @@ -798,6 +824,12 @@ callbacks receive (`ConnectionTo` for `Raw`, `v1::ConnectionTo` for `V1`, `v2::ConnectionTo` for `V2`), so code generic over a builder's context changes `Context: ConnectionContext` to `P: Protocol`. +Generic code can turn a `P::Connection` back into the +version-neutral `ConnectionTo` with `.into()`: `v1::ConnectionTo` and +`v2::ConnectionTo` implement `From` for it, and `Protocol::Connection` +requires `Into>`. Their own methods, such as the v1 +session helpers, are only available on the concrete types. + ## Schema enums and SDK helpers ### v1 extension methods keep their `_` prefix diff --git a/md/protocol-v2.md b/md/protocol-v2.md index 181ae89e..3e5d6037 100644 --- a/md/protocol-v2.md +++ b/md/protocol-v2.md @@ -115,21 +115,16 @@ the same low-level `HandleDispatchFrom` interface and therefore also receive `ConnectionTo<_>`. Raw code converts explicitly: `connection.into_protocol::()` returns a `v1::ConnectionTo` (and `into_protocol::()` a `v2::ConnectionTo`), and either versioned connection's `into_raw()` returns the version-neutral one. - -Nested connections preserve the stable `ConnectionTo` API while still typing -the child implementation's callbacks. On a raw `ConnectionTo<_>`, -`spawn_connection(Client.v2(), transport)` returns a raw `ConnectionTo<_>`, and -callbacks installed on that v2 child builder still receive -`v2::ConnectionTo<_>`. Existing `spawn_connection::` calls therefore remain -source-compatible. - -When a raw parent also needs a typed handle to the child, -`ConnectionTo::spawn_connection_with_context` returns the context selected by -the child builder. `v1::ConnectionTo::spawn_connection` and -`v2::ConnectionTo::spawn_connection` likewise follow the child builder -naturally, so spawning `Client.v2()` through an already-typed v2 connection -returns another `v2::ConnectionTo<_>`, and spawning `Client.v1()` through any -typed connection returns a `v1::ConnectionTo<_>`. +`into_protocol` fails with `-32600` when the connection's builder guards a +different version, so a connection from `Client.v1()` never becomes a +`v2::ConnectionTo`. `ConnectionTo::protocol_version()` reports the guarded +version, or `None` for an unguarded `Builder::new` connection. + +`spawn_connection` returns the connection type the child builder's callbacks +receive, on the raw `ConnectionTo` and on both versioned connections: +spawning `Client.v2()` returns a `v2::ConnectionTo<_>`, spawning +`Client.v1()` a `v1::ConnectionTo<_>`, and spawning a raw `Builder::new` +builder a raw `ConnectionTo<_>`. A complete client installs update and interactive-request handlers before connecting. After `session/prompt` is accepted, it must keep the connection diff --git a/src/agent-client-protocol/src/jsonrpc.rs b/src/agent-client-protocol/src/jsonrpc.rs index c3b73400..9697b326 100644 --- a/src/agent-client-protocol/src/jsonrpc.rs +++ b/src/agent-client-protocol/src/jsonrpc.rs @@ -976,92 +976,6 @@ where } } -#[cfg(feature = "unstable_protocol_v2")] -impl< - Handler: HandleDispatchFrom, - Runner: RunWithConnectionTo, - Close: HandleConnectionClose, -> Builder -{ - /// Apply protocol-v2 wire validation while keeping version-neutral - /// callbacks. - /// - /// This is intended for protocol-routing infrastructure that has already - /// selected v2 but still needs protocol-neutral [`ConnectionTo`] values - /// and [`Raw`] request handles in its callbacks. The guarded child must still send and receive the `initialize` round trip. A router that - /// consumes initialization itself must use an unguarded - /// [`Builder::new`] for the selected child. Most - /// code should use [`Client::v2`](crate::Client::v2), which also exposes - /// the version-typed [`v2::ConnectionTo`](crate::v2::ConnectionTo) API. - /// - /// ```compile_fail - /// # use agent_client_protocol::Client; - /// let _ = Client.v2().with_v2_protocol_guard(); - /// ``` - pub fn with_v2_protocol_guard(mut self) -> Self { - self.protocol_mode = ProtocolMode::v2_client(); - self - } -} - -#[cfg(feature = "unstable_protocol_v2")] -impl< - Handler: HandleDispatchFrom, - Runner: RunWithConnectionTo, - Close: HandleConnectionClose, -> Builder -{ - /// Apply protocol-v2 wire validation and return a [`Raw`] builder. - /// - /// See [`Builder::with_v2_protocol_guard`] on [`Raw`] builders. - pub fn with_v2_protocol_guard(self) -> Builder { - self.into_protocol::().with_v2_protocol_guard() - } -} - -#[cfg(feature = "unstable_protocol_v2")] -impl< - Handler: HandleDispatchFrom, - Runner: RunWithConnectionTo, - Close: HandleConnectionClose, -> Builder -{ - /// Apply protocol-v2 wire validation while keeping version-neutral - /// callbacks. - /// - /// This is intended for protocol-routing infrastructure that has already - /// selected v2 but still needs protocol-neutral [`ConnectionTo`] values - /// and [`Raw`] request handles in its callbacks. The guarded child must still receive and answer the `initialize` request. A router that - /// consumes initialization itself must use an unguarded - /// [`Builder::new`] for the selected child. Most - /// code should use [`Agent::v2`](crate::Agent::v2), which also exposes - /// the version-typed [`v2::ConnectionTo`](crate::v2::ConnectionTo) API. - /// - /// ```compile_fail - /// # use agent_client_protocol::Agent; - /// let _ = Agent.v2().with_v2_protocol_guard(); - /// ``` - pub fn with_v2_protocol_guard(mut self) -> Self { - self.protocol_mode = ProtocolMode::v2_agent(); - self - } -} - -#[cfg(feature = "unstable_protocol_v2")] -impl< - Handler: HandleDispatchFrom, - Runner: RunWithConnectionTo, - Close: HandleConnectionClose, -> Builder -{ - /// Apply protocol-v2 wire validation and return a [`Raw`] builder. - /// - /// See [`Builder::with_v2_protocol_guard`] on [`Raw`] builders. - pub fn with_v2_protocol_guard(self) -> Builder { - self.into_protocol::().with_v2_protocol_guard() - } -} - impl< Host: Role, Handler: HandleDispatchFrom, @@ -3219,13 +3133,6 @@ pub struct ConnectionTo { dynamic_handler_tx: mpsc::UnboundedSender>, transport_completion: SharedTransportCompletion, pending_replies: PendingRepliesRegistrar, - #[cfg_attr( - not(feature = "unstable_protocol_v2"), - allow( - dead_code, - reason = "retained so ConnectionTo has one constructor shape" - ) - )] protocol_mode: ProtocolMode, incoming_closed: IncomingClosed, protected_operations: Arc>, @@ -3639,11 +3546,6 @@ impl ConnectionTo { self.incoming_closed.state.shutdown_rx.clone().await; } - #[cfg(feature = "unstable_protocol_v2")] - pub(crate) fn acp_protocol_version(&self) -> Option { - self.protocol_mode.api_protocol_version() - } - /// Whether this connection uses the ACP protocol v2 API. /// /// Shared code paths select v2 behavior with this runtime check rather than @@ -3654,7 +3556,7 @@ impl ConnectionTo { pub(crate) fn is_acp_v2(&self) -> bool { #[cfg(feature = "unstable_protocol_v2")] { - self.acp_protocol_version() == Some(crate::schema::ProtocolVersion::V2) + self.protocol_version() == Some(crate::schema::ProtocolVersion::V2) } #[cfg(not(feature = "unstable_protocol_v2"))] { @@ -3825,8 +3727,8 @@ impl ConnectionTo { Task::new(location, task).spawn(&self.task_tx) } - /// Spawn a JSON-RPC connection in the background and return a raw - /// [`ConnectionTo`] for it. + /// Spawn a JSON-RPC connection in the background and return the + /// connection context its builder selects. /// /// This is useful for creating multiple connections that communicate with each other, /// such as implementing proxy patterns or connecting to multiple backend services. @@ -3838,13 +3740,11 @@ impl ConnectionTo { /// /// # Returns /// - /// The child builder may use any [`Protocol`]. For example, this method - /// can spawn a `v2::Builder`, whose callbacks receive `v2::ConnectionTo`, - /// while preserving this method's existing raw return type and single - /// explicit role parameter. - /// - /// When a raw parent also needs the builder-selected child handle, use the - /// protocol-v2 `spawn_connection_with_context` method. + /// The connection type that `builder`'s callbacks receive: a raw + /// [`ConnectionTo`] for a [`Raw`] builder such as [`Builder::new`], a + /// [`v1::ConnectionTo`](crate::v1::ConnectionTo) for + /// [`Client.v1()`](crate::Client::v1), and a `v2::ConnectionTo` for + /// `Client.v2()`. Convert a typed connection back with `into_raw`. /// /// # Example: Proxying to a backend connection /// @@ -3860,56 +3760,15 @@ impl ConnectionTo { /// }, agent_client_protocol::on_receive_request!()); /// /// // Spawn it and get a context to send requests to it - /// let backend_connection = cx.spawn_connection::(backend, MockTransport)?; + /// let backend_connection = cx.spawn_connection(backend, MockTransport)?; /// /// // Now you can forward requests to the backend /// let response = backend_connection.send_request(MyRequest {}).block_task().await?; /// # Ok(()) /// # } /// ``` - #[track_caller] - pub fn spawn_connection( - &self, - builder: Builder< - R, - impl HandleDispatchFrom + 'static, - impl RunWithConnectionTo + 'static, - impl HandleConnectionClose + 'static, - impl Protocol, - >, - transport: impl ConnectTo + 'static, - ) -> Result, crate::Error> { - self.spawn_connection_raw(builder, transport) - } - - /// The same connection, typed for protocol `P`. - /// - /// `into_protocol::()` returns a [`v1::ConnectionTo`](crate::v1::ConnectionTo) - /// (and, with `unstable_protocol_v2`, `into_protocol::()` a - /// `v2::ConnectionTo`), whose sends return request handles typed for that - /// version and which carries that version's session helpers. Only the - /// Rust API changes: the connection's wire guard is the one its builder - /// selected. The versioned connections convert back with `into_raw`. - /// - /// ``` - /// # use agent_client_protocol::{Agent, ConnectionTo, V1, v1}; - /// fn sessions(connection: ConnectionTo) -> v1::ConnectionTo { - /// connection.into_protocol::() - /// } - /// ``` - #[must_use] - pub fn into_protocol(self) -> P::Connection { - protocol::connection_from_raw::(self) - } - - /// Spawn a JSON-RPC connection and return the connection context selected - /// by its builder. /// - /// This is the low-level counterpart to - /// [`v1::ConnectionTo::spawn_connection`](crate::v1::ConnectionTo::spawn_connection) for code that intentionally works - /// with a raw [`ConnectionTo`], such as custom [`HandleDispatchFrom`] or - /// [`RunWithConnectionTo`] implementations. Prefer [`Self::spawn_connection`] - /// when a raw child handle is sufficient. + /// # Example: Spawning a v1 client /// /// ```no_run /// # use agent_client_protocol::{ @@ -3919,14 +3778,13 @@ impl ConnectionTo { /// # connection: ConnectionTo, /// # transport: impl ConnectTo + 'static, /// # ) -> Result<(), Error> { - /// let child: v1::ConnectionTo = - /// connection.spawn_connection_with_context(Client.v1(), transport)?; + /// let child: v1::ConnectionTo = connection.spawn_connection(Client.v1(), transport)?; /// # drop(child); /// # Ok(()) /// # } /// ``` #[track_caller] - pub fn spawn_connection_with_context( + pub fn spawn_connection( &self, builder: Builder< R, @@ -3937,26 +3795,58 @@ impl ConnectionTo { >, transport: impl ConnectTo + 'static, ) -> Result, crate::Error> { - let connection = self.spawn_connection_raw(builder, transport)?; - Ok(protocol::connection_from_raw::(connection)) - } - - #[track_caller] - fn spawn_connection_raw( - &self, - builder: Builder< - R, - impl HandleDispatchFrom + 'static, - impl RunWithConnectionTo + 'static, - impl HandleConnectionClose + 'static, - P, - >, - transport: impl ConnectTo + 'static, - ) -> Result, crate::Error> { let (connection, future) = builder.into_connection_and_future(transport, false, |_| std::future::pending()); Task::new(std::panic::Location::caller(), future).spawn(&self.task_tx)?; - Ok(connection) + Ok(protocol::connection_from_raw::(connection)) + } + + /// The ACP protocol version of this connection's wire guard, or `None` + /// when its builder applies no guard, as with [`Builder::new`]. + /// + /// `Client.v1()`, `Agent.v1()` and `Proxy.v1()` connections report + /// [`ProtocolVersion::V1`](crate::schema::ProtocolVersion::V1), and the + /// `v2()` entry points report `ProtocolVersion::V2`. + #[must_use] + pub fn protocol_version(&self) -> Option { + self.protocol_mode.api_protocol_version() + } + + /// The same connection, typed for protocol `P`. + /// + /// `into_protocol::()` returns a [`v1::ConnectionTo`](crate::v1::ConnectionTo) + /// (and, with `unstable_protocol_v2`, `into_protocol::()` a + /// `v2::ConnectionTo`), whose sends return request handles typed for that + /// version and which carries that version's session helpers. Only the + /// Rust API changes: the connection's wire guard is the one its builder + /// selected. The versioned connections convert back with `into_raw`. + /// + /// # Errors + /// + /// Returns an Invalid Request error (`-32600`) when the builder's guard + /// selected a different version than `P`, for example + /// `into_protocol::()` on a connection from `Client.v1()`. A + /// connection whose builder applies no guard (see + /// [`protocol_version`](Self::protocol_version)) converts to every + /// protocol, and `into_protocol::()` never fails. + /// + /// ``` + /// # use agent_client_protocol::{Agent, ConnectionTo, Error, V1, v1}; + /// fn sessions(connection: ConnectionTo) -> Result, Error> { + /// connection.into_protocol::() + /// } + /// ``` + pub fn into_protocol(self) -> Result, crate::Error> { + if let (Some(guard), Some(requested)) = + (self.protocol_version(), protocol::protocol_version::

()) + && guard != requested + { + return Err(crate::Error::invalid_request().data(format!( + "cannot use a connection guarded for ACP protocol version {guard} \ + as a protocol version {requested} connection", + ))); + } + Ok(protocol::connection_from_raw::(self)) } /// Send a request/notification and forward the response appropriately. @@ -8107,8 +7997,8 @@ mod tests { let (transport, _peer) = Channel::duplex(); let (callback_tx, callback_rx) = oneshot::channel(); - let child: ConnectionTo = parent - .spawn_connection::( + let child: crate::v2::ConnectionTo = parent + .spawn_connection( Client.v2().with_spawned( async move |_connection: crate::v2::ConnectionTo| { callback_tx.send(()).map_err(|()| { @@ -8140,13 +8030,14 @@ mod tests { #[cfg(feature = "unstable_protocol_v2")] #[test] - fn raw_connection_can_return_v2_context_for_spawned_builder() { + fn raw_connection_returns_raw_context_for_raw_builder() { let (parent, mut task_rx) = connection_with_task_receiver(); let (transport, _peer) = Channel::duplex(); - let child: crate::v2::ConnectionTo = parent - .spawn_connection_with_context(Client.v2(), transport) - .expect("v2 child connection should be spawned"); + let child: ConnectionTo = parent + .spawn_connection(Builder::new(Client), transport) + .expect("raw child connection should be spawned"); + assert_eq!(child.protocol_version(), None); let child_task = futures::FutureExt::now_or_never(futures::StreamExt::next(&mut task_rx)) .expect("child connection task should already be queued") diff --git a/src/agent-client-protocol/src/jsonrpc/protocol.rs b/src/agent-client-protocol/src/jsonrpc/protocol.rs index 8c020699..a0dfe88a 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol.rs @@ -88,13 +88,33 @@ pub trait Protocol: sealed::Sealed + Copy + Debug + Default + Send + Sync + 'sta + 'static; /// The connection type that builder callbacks receive for `Counterpart`. - type Connection: Clone + Send + Sync + 'static; + /// + /// Every connection type converts back to the version-neutral + /// [`ConnectionTo`] with `Into`, so code generic over `P` can reach the + /// raw connection: + /// + /// ``` + /// use agent_client_protocol::{Agent, ConnectionTo, Protocol}; + /// + /// fn raw(connection: P::Connection) -> ConnectionTo { + /// connection.into() + /// } + /// ``` + type Connection: Clone + + Send + + Sync + + Into> + + 'static; } pub(crate) mod sealed { use super::{ConnectionTo, Protocol, RawJsonRpcId, Role}; pub trait Sealed { + /// The ACP protocol version this marker types, or `None` for + /// [`Raw`](super::Raw). + fn protocol_version() -> Option; + /// Wraps the version-neutral connection in this version's connection /// type. fn connection_from_raw( @@ -118,6 +138,11 @@ pub(crate) mod sealed { } } +/// The ACP protocol version `P` types, or `None` for [`Raw`]. +pub(crate) fn protocol_version() -> Option { +

::protocol_version() +} + pub(crate) fn connection_from_raw( connection: ConnectionTo, ) -> P::Connection { @@ -133,6 +158,10 @@ pub(crate) fn connection_from_raw( pub struct Raw; impl sealed::Sealed for Raw { + fn protocol_version() -> Option { + None + } + fn connection_from_raw( connection: ConnectionTo, ) -> ConnectionTo { @@ -160,6 +189,10 @@ impl Protocol for Raw { pub struct V1; impl sealed::Sealed for V1 { + fn protocol_version() -> Option { + Some(crate::schema::ProtocolVersion::V1) + } + fn connection_from_raw( connection: ConnectionTo, ) -> super::v1_connection::ConnectionTo { @@ -191,6 +224,10 @@ pub struct V2; #[cfg(feature = "unstable_protocol_v2")] impl sealed::Sealed for V2 { + fn protocol_version() -> Option { + Some(crate::schema::ProtocolVersion::V2) + } + fn connection_from_raw( connection: ConnectionTo, ) -> super::v2_connection::ConnectionTo { diff --git a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs index 22aad495..0dd7da62 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs @@ -503,7 +503,6 @@ mod imp { } } - #[cfg(feature = "unstable_protocol_v2")] pub(crate) fn api_protocol_version(self) -> Option { match self { Self::Disabled => None, diff --git a/src/agent-client-protocol/src/jsonrpc/v1_connection.rs b/src/agent-client-protocol/src/jsonrpc/v1_connection.rs index a7dbbf05..334a01ac 100644 --- a/src/agent-client-protocol/src/jsonrpc/v1_connection.rs +++ b/src/agent-client-protocol/src/jsonrpc/v1_connection.rs @@ -4,7 +4,7 @@ use super::ConnectionTo as RawConnectionTo; use super::{ Builder, ConnectTo, ConnectionTermination, Dispatch, DynamicHandlerGuard, HandleConnectionClose, HandleDispatchFrom, JsonRpcNotification, JsonRpcRequest, - PreparedRequest, Protocol, RawJsonRpcId, SentRequest, V1, run::RunWithConnectionTo, + PreparedRequest, Protocol, SentRequest, V1, run::RunWithConnectionTo, }; use crate::role::{HasPeer, Role}; @@ -29,6 +29,15 @@ pub struct ConnectionTo { pub(crate) inner: RawConnectionTo, } +impl From> for RawConnectionTo { + /// The same as [`ConnectionTo::into_raw`]. Generic code over + /// [`Protocol`] uses it to turn a `P::Connection` back into + /// the version-neutral connection. + fn from(connection: ConnectionTo) -> Self { + connection.into_raw() + } +} + impl ConnectionTo { /// Access the underlying version-neutral connection inside the SDK. pub(crate) fn raw_connection(&self) -> &RawConnectionTo { @@ -115,7 +124,7 @@ impl ConnectionTo { >, transport: impl ConnectTo + 'static, ) -> Result, crate::Error> { - self.inner.spawn_connection_with_context(builder, transport) + self.inner.spawn_connection(builder, transport) } /// Send a request or notification and forward its response appropriately. @@ -224,26 +233,34 @@ impl ConnectionTo { } /// Send a `$/cancel_request` notification to the default counterpart peer. + /// + /// Prefer [`SentRequest::cancel`](crate::SentRequest::cancel) when you + /// have the request handle. `request_id` is a v1 request ID, such as the + /// [`id`](crate::SentRequest::id) of a request sent through this + /// connection. The raw [`ConnectionTo::send_cancel_request`](crate::ConnectionTo::send_cancel_request) + /// takes any [`RawJsonRpcId`](crate::RawJsonRpcId). pub fn send_cancel_request( &self, - request_id: impl Into, + request_id: impl Into, ) -> Result<(), crate::Error> where Counterpart: HasPeer, { - self.inner.send_cancel_request(request_id) + self.inner.send_cancel_request(request_id.into()) } /// Send a `$/cancel_request` notification to a specific peer. + /// + /// See [`send_cancel_request`](Self::send_cancel_request). pub fn send_cancel_request_to( &self, peer: Peer, - request_id: impl Into, + request_id: impl Into, ) -> Result<(), crate::Error> where Counterpart: HasPeer, { - self.inner.send_cancel_request_to(peer, request_id) + self.inner.send_cancel_request_to(peer, request_id.into()) } /// Register a low-level dynamic message handler. diff --git a/src/agent-client-protocol/src/jsonrpc/v2_connection.rs b/src/agent-client-protocol/src/jsonrpc/v2_connection.rs index 2e25f14b..c41de043 100644 --- a/src/agent-client-protocol/src/jsonrpc/v2_connection.rs +++ b/src/agent-client-protocol/src/jsonrpc/v2_connection.rs @@ -4,8 +4,7 @@ use super::ConnectionTo as RawConnectionTo; use super::{ Builder, ConnectTo, ConnectionTermination, Dispatch, DynamicHandlerGuard, HandleConnectionClose, HandleDispatchFrom, JsonRpcNotification, JsonRpcRequest, - PreparedRequest, Protocol, RawJsonRpcId, SentRequest, V2, protocol_compat, - run::RunWithConnectionTo, + PreparedRequest, Protocol, SentRequest, V2, protocol_compat, run::RunWithConnectionTo, }; use crate::Agent; use crate::role::{HasPeer, Role}; @@ -96,6 +95,15 @@ impl ConnectionTo { } } +impl From> for RawConnectionTo { + /// The same as [`ConnectionTo::into_raw`]. Generic code over + /// [`Protocol`] uses it to turn a `P::Connection` back into + /// the version-neutral connection. + fn from(connection: ConnectionTo) -> Self { + connection.into_raw() + } +} + impl ConnectionTo { /// Access the underlying version-neutral connection inside the SDK. pub(crate) fn raw_connection(&self) -> &RawConnectionTo { @@ -182,7 +190,7 @@ impl ConnectionTo { >, transport: impl ConnectTo + 'static, ) -> Result, crate::Error> { - self.inner.spawn_connection_with_context(builder, transport) + self.inner.spawn_connection(builder, transport) } /// Send a request or notification and forward its response appropriately. @@ -291,26 +299,34 @@ impl ConnectionTo { } /// Send a `$/cancel_request` notification to the default counterpart peer. + /// + /// Prefer [`SentRequest::cancel`](crate::SentRequest::cancel) when you + /// have the request handle. `request_id` is a v2 request ID, such as the + /// [`id`](crate::SentRequest::id) of a request sent through this + /// connection. The raw [`ConnectionTo::send_cancel_request`](crate::ConnectionTo::send_cancel_request) + /// takes any [`RawJsonRpcId`](crate::RawJsonRpcId). pub fn send_cancel_request( &self, - request_id: impl Into, + request_id: impl Into, ) -> Result<(), crate::Error> where Counterpart: HasPeer, { - self.inner.send_cancel_request(request_id) + self.inner.send_cancel_request(request_id.into()) } /// Send a `$/cancel_request` notification to a specific peer. + /// + /// See [`send_cancel_request`](Self::send_cancel_request). pub fn send_cancel_request_to( &self, peer: Peer, - request_id: impl Into, + request_id: impl Into, ) -> Result<(), crate::Error> where Counterpart: HasPeer, { - self.inner.send_cancel_request_to(peer, request_id) + self.inner.send_cancel_request_to(peer, request_id.into()) } /// Register a low-level dynamic message handler. diff --git a/src/agent-client-protocol/tests/protocol_v2.rs b/src/agent-client-protocol/tests/protocol_v2.rs index 8fcbc918..47c58be6 100644 --- a/src/agent-client-protocol/tests/protocol_v2.rs +++ b/src/agent-client-protocol/tests/protocol_v2.rs @@ -95,7 +95,7 @@ where R: Role, P: agent_client_protocol::Protocol, { - parent.spawn_connection_with_context(builder, transport) + parent.spawn_connection(builder, transport) } fn v2_implementation() -> v2::Implementation { @@ -2905,6 +2905,29 @@ async fn v2_client_can_cancel_request_to_v2_agent() -> Result<(), Error> { .await } +#[tokio::test(flavor = "current_thread")] +async fn v2_client_can_cancel_request_by_its_v2_id() -> Result<(), Error> { + Client + .v2() + .connect_with(v2_agent_with_cancellable_new_session(), async |cx| { + cx.send_request(v2_initialize_request(ProtocolVersion::V2)) + .block_task() + .await?; + + let request = cx.send_request(v2::NewSessionRequest::new(cwd()?)); + // The typed connection takes the v2 ID that `id()` returns. + let id: v2::RequestId = request.id().clone(); + cx.send_cancel_request(id)?; + let error = request + .block_task() + .await + .expect_err("request should be cancelled"); + assert_eq!(i32::from(error.code), -32800); + Ok(()) + }) + .await +} + #[tokio::test(flavor = "current_thread")] async fn send_initialize_reports_v1_selection_as_outcome() -> Result<(), Error> { // The outcome leaves `connect_with`, where a caller decides to start v1. diff --git a/src/agent-client-protocol/tests/session_v2.rs b/src/agent-client-protocol/tests/session_v2.rs index 656a9d9f..be355fc1 100644 --- a/src/agent-client-protocol/tests/session_v2.rs +++ b/src/agent-client-protocol/tests/session_v2.rs @@ -34,19 +34,17 @@ fn initialize_response(protocol_version: ProtocolVersion) -> v2::InitializeRespo } struct V1SessionGuardRunner { - cwd: PathBuf, completed: oneshot::Sender<()>, } impl RunWithConnectionTo for V1SessionGuardRunner { async fn run_with_connection_to(self, connection: ConnectionTo) -> Result<(), Error> { + // A v2 connection does not convert to the v1 connection type, so the + // v1 session helpers cannot run over it. let error = connection + .clone() .into_protocol::() - .build_session(self.cwd) - .block_task() - .start_session() - .await - .expect_err("the v1 helper must reject the raw protocol v2 connection"); + .expect_err("a protocol v2 connection must not convert to v1"); assert_eq!(error.code, ErrorCode::INVALID_REQUEST); let data = error @@ -54,7 +52,14 @@ impl RunWithConnectionTo for V1SessionGuardRunner { .as_ref() .and_then(serde_json::Value::as_str) .unwrap_or_default(); - assert!(data.contains("`v2::ConnectionTo`"), "{error:?}"); + assert!(data.contains("protocol version 2"), "{error:?}"); + + assert_eq!(connection.protocol_version(), Some(ProtocolVersion::V2)); + connection + .clone() + .into_protocol::()?; + let raw: ConnectionTo = connection.into_protocol::()?; + drop(raw); self.completed.send(()).map_err(|()| { Error::internal_error().data("v1 session guard test receiver was dropped") @@ -1211,12 +1216,11 @@ async fn v2_session_new_error_is_preserved_without_closing_connection() { } #[tokio::test(flavor = "current_thread")] -async fn low_level_v2_runner_rejects_the_v1_session_helper() { +async fn low_level_v2_runner_cannot_use_the_v1_connection_type() { let (completed_tx, completed_rx) = oneshot::channel(); let client = Client .v2() .with_runner(V1SessionGuardRunner { - cwd: cwd().expect("current directory should be available"), completed: completed_tx, }) .connect_with(Agent.v2(), async move |_connection| { diff --git a/src/agent-client-protocol/tests/v1_connection.rs b/src/agent-client-protocol/tests/v1_connection.rs index fbc6b391..1ee6d330 100644 --- a/src/agent-client-protocol/tests/v1_connection.rs +++ b/src/agent-client-protocol/tests/v1_connection.rs @@ -49,7 +49,20 @@ async fn v1_and_raw_connections_convert_explicitly() -> Result<(), Error> { let _: &RawJsonRpcId = raw_sent.id(); raw_sent.block_task().await?; - let v1_cx: agent_client_protocol::v1::ConnectionTo = raw.into_protocol::(); + assert_eq!(raw.protocol_version(), Some(ProtocolVersion::V1)); + #[cfg(feature = "unstable_protocol_v2")] + { + let error = raw + .clone() + .into_protocol::() + .expect_err("a v1 connection must not convert to v2"); + assert_eq!( + error.code, + agent_client_protocol::ErrorCode::INVALID_REQUEST + ); + } + let v1_cx: agent_client_protocol::v1::ConnectionTo = + raw.into_protocol::()?; let response = v1_cx .send_request(v1::InitializeRequest::new(ProtocolVersion::V1)) .block_task() From eb5e10ecbfd6fd8ac58db3920745fc535ffb3f2f Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:25:39 +0100 Subject: [PATCH 02/11] fix(acp)!: move the lower-version initialize types to the crate root `InitializationVersionMismatch` and `SelectedInitializeResponse` are not tied to protocol v2: every later version's negotiation reports a lower selection with them. They now live at the crate root (still behind `unstable_protocol_v2`, the first feature under which a lower version can be selected). `v2::InitializeOutcome` stays in `v2`. Drop the redundant `InitializationVersionMismatch::selected` field in favor of a `selected()` method that reads the response's version, so the two can no longer disagree. Findings F20, F75. BREAKING CHANGE: `v2::InitializationVersionMismatch` and `v2::SelectedInitializeResponse` move to the crate root, and `InitializationVersionMismatch::selected` is now a method. Co-Authored-By: Claude --- md/migration-v5.md | 12 ++++-- md/protocol-v2.md | 4 +- .../src/jsonrpc/protocol_compat.rs | 38 +++++++++++++------ .../src/jsonrpc/v2_connection.rs | 3 +- src/agent-client-protocol/src/lib.rs | 4 +- src/agent-client-protocol/src/v2.rs | 4 +- .../tests/protocol_v2.rs | 5 ++- 7 files changed, 46 insertions(+), 24 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index 7eea13dc..f3237806 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -26,7 +26,10 @@ derives, `UntypedMessage`, `Error`, `ErrorCode`, `Result`, `Protocol` with `SentRequest`, `PreparedRequest`, `Dispatch` and `Builder`, defaulting to `Raw`), the version-neutral `ConnectionTo`, and the routers that serve both versions (`AgentProtocolRouter`, `ProxyProtocolRouter`, -`ClientProtocolConnector`). +`ClientProtocolConnector`). With `unstable_protocol_v2` it also has +`InitializationVersionMismatch` and `SelectedInitializeResponse`, which +`v2::InitializeOutcome::LowerVersion` uses to report a lower version the +peer selected. | Name | `agent_client_protocol::v1` | `agent_client_protocol::v2` | | --- | --- | --- | @@ -42,7 +45,7 @@ versions (`AgentProtocolRouter`, `ProxyProtocolRouter`, | `ActiveSession` | the session handle with its update stream | the command-only session handle | | `ForkSessionBuilder` | - | `fork_session*` (`unstable_session_fork`) | | `LoadSessionBuilder`, `SessionMessage`, `Blocking`, `NonBlocking`, `SessionBlockState` | v1 session helpers | - | -| `InitializeOutcome`, `InitializationVersionMismatch`, `SelectedInitializeResponse` | - | the result of `send_initialize` | +| `InitializeOutcome` | - | the result of `send_initialize` | | `schema` | `schema::v1` | `schema::v2` | ### `v1::ConnectionTo` is its own type @@ -240,8 +243,9 @@ their path. | `OpenedV2Session` | `v2::SessionSetup` | | `V2Session` | `v2::ActiveSession` | | `InitializeOutcome` | `v2::InitializeOutcome` | -| `InitializationVersionMismatch` | `v2::InitializationVersionMismatch` | -| `SelectedInitializeResponse` | `v2::SelectedInitializeResponse` | +| `InitializationVersionMismatch` | unchanged, at the crate root | +| `InitializationVersionMismatch::selected` (field) | `InitializationVersionMismatch::selected()`, the version of `response` | +| `SelectedInitializeResponse` | unchanged, at the crate root | | `SessionBuilder` | `v1::SessionBuilder` | | `ActiveSession` | `v1::ActiveSession` | | `RestoredSession` | `v1::SessionSetup` | diff --git a/md/protocol-v2.md b/md/protocol-v2.md index 3e5d6037..2c6aea81 100644 --- a/md/protocol-v2.md +++ b/md/protocol-v2.md @@ -438,7 +438,9 @@ The SDK handles the `initialize` negotiation at the JSON-RPC boundary: `send_initialize` to learn how the agent negotiated: it resolves with `v2::InitializeOutcome::Initialized` for a v2 selection, or `v2::InitializeOutcome::LowerVersion` with the agent's validated response for - the selected version (`v2::SelectedInitializeResponse::V1`). After a lower + the selected version, an `InitializationVersionMismatch` whose `response` is a +`SelectedInitializeResponse::V1`. Both types are at the crate root because +every version's negotiation reports a lower selection with them. After a lower selection the connection stays uninitialized, and the caller can explicitly fall back to a v1 client implementation if that is acceptable. Errors returned by the agent, including `-32022`, malformed responses, and transport diff --git a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs index 0dd7da62..e25f43a7 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs @@ -154,6 +154,9 @@ pub(crate) fn normalize_v1_initialize_request( /// The outcome of a v2 client's [`send_initialize`](crate::v2::ConnectionTo::send_initialize). /// +/// A lower selection is reported with the crate-root +/// [`InitializationVersionMismatch`](crate::InitializationVersionMismatch). +/// /// A v1-only agent may legitimately answer a v2 initialization attempt with a /// v1 response. That is a successful reply, not an error, so it is reported as /// [`InitializeOutcome::LowerVersion`]. Errors from the agent, malformed @@ -175,18 +178,21 @@ pub enum InitializeOutcome { /// A successful `initialize` response that selected a lower protocol version /// than the local API requires. /// -/// Reported by [`InitializeOutcome::LowerVersion`]. This is not the `-32022` -/// (`Unsupported protocol version`) error an agent returns when it supports no -/// version at or below the requested one. +/// Reported by [`v2::InitializeOutcome::LowerVersion`](crate::v2::InitializeOutcome::LowerVersion). +/// This is not the `-32022` (`Unsupported protocol version`) error an agent +/// returns when it supports no version at or below the requested one. +/// +/// It lives at the crate root because it is not tied to one protocol +/// version: every version's `send_initialize` reports a lower selection with +/// it. #[cfg(feature = "unstable_protocol_v2")] #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub struct InitializationVersionMismatch { /// The protocol version the local connection requested. pub requested: crate::schema::ProtocolVersion, - /// The lower protocol version the peer selected. - pub selected: crate::schema::ProtocolVersion, - /// The peer's validated initialization response for the selected version. + /// The peer's validated initialization response for the version it + /// selected. [`selected`](Self::selected) returns that version. pub response: SelectedInitializeResponse, } @@ -203,10 +209,16 @@ impl InitializationVersionMismatch { ) -> Self { Self { requested, - selected: response.protocol_version(), response, } } + + /// The lower protocol version the peer selected: the version of + /// [`response`](Self::response). + #[must_use] + pub fn selected(&self) -> crate::schema::ProtocolVersion { + self.response.protocol_version() + } } #[cfg(feature = "unstable_protocol_v2")] @@ -215,15 +227,19 @@ impl std::fmt::Display for InitializationVersionMismatch { write!( f, "requested ACP protocol version {} but peer selected {}", - self.requested, self.selected, + self.requested, + self.selected(), ) } } /// A peer's initialization response, typed by the protocol version it selected. /// -/// New variants are added as later protocol versions make more versions -/// selectable below the one a client requests. +/// It lives at the crate root because it is not tied to one protocol +/// version. A version is only ever selected below a higher requested one, so +/// each variant is available with the feature of the next higher version: +/// `V1` with `unstable_protocol_v2`. New variants are added as later protocol +/// versions make more versions selectable below the one a client requests. #[cfg(feature = "unstable_protocol_v2")] #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] @@ -1624,7 +1640,7 @@ mod imp { unreachable!(); }; assert_eq!(mismatch.requested, ProtocolVersion::V2); - assert_eq!(mismatch.selected, ProtocolVersion::V1); + assert_eq!(mismatch.selected(), ProtocolVersion::V1); for value in [ serde_json::json!({}), diff --git a/src/agent-client-protocol/src/jsonrpc/v2_connection.rs b/src/agent-client-protocol/src/jsonrpc/v2_connection.rs index c41de043..16ec89d7 100644 --- a/src/agent-client-protocol/src/jsonrpc/v2_connection.rs +++ b/src/agent-client-protocol/src/jsonrpc/v2_connection.rs @@ -50,7 +50,8 @@ impl ConnectionTo { /// ```no_run /// # use agent_client_protocol::schema::{ProtocolVersion, v2}; /// # use agent_client_protocol::{Client, ConnectTo}; - /// use agent_client_protocol::v2::{InitializeOutcome, SelectedInitializeResponse}; + /// use agent_client_protocol::SelectedInitializeResponse; + /// use agent_client_protocol::v2::InitializeOutcome; /// # async fn example(agent: impl ConnectTo) -> agent_client_protocol::Result<()> { /// let outcome = Client /// .v2() diff --git a/src/agent-client-protocol/src/lib.rs b/src/agent-client-protocol/src/lib.rs index 10d576ea..53d21407 100644 --- a/src/agent-client-protocol/src/lib.rs +++ b/src/agent-client-protocol/src/lib.rs @@ -188,8 +188,6 @@ pub mod v1; #[cfg_attr(docsrs, doc(cfg(feature = "unstable_protocol_v2")))] pub mod v2; -#[cfg(feature = "unstable_protocol_v2")] -pub use jsonrpc::V2; pub use jsonrpc::{ Builder, ByteStreams, CONNECTION_TERMINATED_REASON, Channel, ConnectionTermination, ConnectionTo, Dispatch, DynamicHandlerGuard, HandleConnectionClose, HandleDispatchFrom, @@ -200,6 +198,8 @@ pub use jsonrpc::{ UntypedMessage, V1, is_connection_terminated, is_incoming_transport_closed, run::{ChainRun, NullRun, RunWithConnectionTo}, }; +#[cfg(feature = "unstable_protocol_v2")] +pub use jsonrpc::{InitializationVersionMismatch, SelectedInitializeResponse, V2}; pub use jsonrpc::{RequestCancellation, RequestCancellationHandle, is_cancel_request_notification}; #[cfg(feature = "unstable_protocol_v2")] diff --git a/src/agent-client-protocol/src/v2.rs b/src/agent-client-protocol/src/v2.rs index 0c73e705..b9a81839 100644 --- a/src/agent-client-protocol/src/v2.rs +++ b/src/agent-client-protocol/src/v2.rs @@ -50,10 +50,8 @@ use crate::{NullClose, NullHandler, UntypedMessage, V2, jsonrpc::run::NullRun}; /// SDK's `v2` module. pub use crate::schema::v2 as schema; +pub use crate::jsonrpc::InitializeOutcome; pub use crate::jsonrpc::v2_connection::ConnectionTo; -pub use crate::jsonrpc::{ - InitializationVersionMismatch, InitializeOutcome, SelectedInitializeResponse, -}; #[cfg(feature = "unstable_session_fork")] pub use crate::session::v2::ForkSessionBuilder; pub use crate::session::v2::{ActiveSession, ResumeSessionBuilder, SessionBuilder, SessionSetup}; diff --git a/src/agent-client-protocol/tests/protocol_v2.rs b/src/agent-client-protocol/tests/protocol_v2.rs index 47c58be6..b335ed5b 100644 --- a/src/agent-client-protocol/tests/protocol_v2.rs +++ b/src/agent-client-protocol/tests/protocol_v2.rs @@ -10,8 +10,9 @@ use std::{ }, }; +use agent_client_protocol::SelectedInitializeResponse; use agent_client_protocol::schema::{ProtocolVersion, SuccessorMessage, v1, v2}; -use agent_client_protocol::v2::{InitializeOutcome, SelectedInitializeResponse}; +use agent_client_protocol::v2::InitializeOutcome; use agent_client_protocol::{ Agent, AgentProtocolRouter, Builder, ByteStreams, Client, ClientProtocolConnector, Conductor, ConnectTo, ConnectionTo, DynamicHandlerGuard, Error, HandleConnectionClose, HandleDispatchFrom, @@ -2951,7 +2952,7 @@ async fn send_initialize_reports_v1_selection_as_outcome() -> Result<(), Error> panic!("expected a lower-version outcome: {outcome:?}"); }; assert_eq!(mismatch.requested, ProtocolVersion::V2); - assert_eq!(mismatch.selected, ProtocolVersion::V1); + assert_eq!(mismatch.selected(), ProtocolVersion::V1); assert!(matches!( mismatch.response, SelectedInitializeResponse::V1(v1::InitializeResponse { protocol_version, .. }) From 350386ac3cbd375d2a5b6e706b3ae55f2edead68 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:26:47 +0100 Subject: [PATCH 03/11] fix(acp): keep ProxyProtocolRouter open after a rejected proxy/initialize `ProxyProtocolRouter` closed the connection on the first rejected `proxy/initialize`, unsupported version, or malformed frame, while its rustdoc said it selected "exactly as `Agent::protocol_router` does". Both routers now share one selection loop that answers each rejected frame and keeps reading, so a conductor can retry `proxy/initialize` the same way a client can retry `initialize`. Finding F27. Co-Authored-By: Claude --- src/agent-client-protocol/src/role/acp.rs | 191 +++++++----------- .../tests/proxy_protocol_router_v2.rs | 71 +++++++ 2 files changed, 142 insertions(+), 120 deletions(-) diff --git a/src/agent-client-protocol/src/role/acp.rs b/src/agent-client-protocol/src/role/acp.rs index 1255491b..f4bff726 100644 --- a/src/agent-client-protocol/src/role/acp.rs +++ b/src/agent-client-protocol/src/role/acp.rs @@ -423,46 +423,11 @@ impl ConnectTo for AgentProtocolRouter { v1: self.v1.is_some(), v2: self.v2.is_some(), }; - let mut client = RunningProtocolPeer::new(client); - // A failed initialize leaves the connection uninitialized, and the - // client may send `initialize` again. Answer each rejected frame and - // keep reading until an initialize selects a configured version. - let (first_frame, client, selected) = loop { - let Some((mut frame, next_client)) = client.next_frame().await? else { - return Ok(()); - }; - client = next_client; - let message = match initialize_message_mut(&mut frame) { - Ok(Some(message)) => message, - Ok(None) => continue, - // A malformed frame, such as invalid JSON or a batch, gets the - // frame's own error with `id: null`, exactly as a v1 - // connection answers it. It selects no version, so the client - // can still initialize either one. - Err(error) => { - send_initialize_error(&client.tx, &frame, error)?; - continue; - } - }; - let selected = match select_initialize_protocol(message, "initialize", supported) { - Ok(selected) => selected, - // A v1 implementation answers any other initialize that does - // not select v2 itself, exactly as it would without the router. - Err(_) - if supported.v1 && !requests_protocol_v2(message, "initialize", supported) => - { - SelectedProtocol::V1 - } - Err(error) => { - send_initialize_error(&client.tx, &frame, error)?; - continue; - } - }; - if !selected.is_configured(supported) { - send_initialize_error(&client.tx, &frame, selected.unsupported_error(supported))?; - continue; - } - break (frame, client, selected); + let client = RunningProtocolPeer::new(client); + let Some((first_frame, client, selected)) = + select_protocol_peer(client, "initialize", supported).await? + else { + return Ok(()); }; let agent = selected .take_agent(self) @@ -474,6 +439,59 @@ impl ConnectTo for AgentProtocolRouter { } } +/// Read frames from `peer` until an `initialize_method` request selects a +/// configured protocol version, and return that request's frame, the peer and +/// the selection. Returns `None` if the peer closes first. +/// +/// A failed initialize leaves the connection uninitialized, and the peer may +/// send `initialize_method` again. Each rejected frame is answered and the +/// loop keeps reading, so [`AgentProtocolRouter`] and [`ProxyProtocolRouter`] +/// treat rejections the same way. +#[cfg(feature = "unstable_protocol_v2")] +async fn select_protocol_peer( + mut peer: RunningProtocolPeer, + initialize_method: &str, + supported: SupportedProtocols, +) -> Result, crate::Error> { + loop { + let Some((mut frame, next_peer)) = peer.next_frame().await? else { + return Ok(None); + }; + peer = next_peer; + let message = match initialize_message_mut(&mut frame) { + Ok(Some(message)) => message, + Ok(None) => continue, + // A malformed frame, such as invalid JSON or a batch, gets the + // frame's own error with `id: null`, exactly as a v1 connection + // answers it. It selects no version, so the peer can still + // initialize either one. + Err(error) => { + send_initialize_error(&peer.tx, &frame, error)?; + continue; + } + }; + let selected = match select_initialize_protocol(message, initialize_method, supported) { + Ok(selected) => selected, + // A v1 implementation answers any other message that does not + // select v2 itself, exactly as it would without the router. + Err(_) + if supported.v1 && !requests_protocol_v2(message, initialize_method, supported) => + { + SelectedProtocol::V1 + } + Err(error) => { + send_initialize_error(&peer.tx, &frame, error)?; + continue; + } + }; + if !selected.is_configured(supported) { + send_initialize_error(&peer.tx, &frame, selected.unsupported_error(supported))?; + continue; + } + return Ok(Some((frame, peer, selected))); + } +} + #[cfg(feature = "unstable_protocol_v2")] #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum SelectedProtocol { @@ -1018,44 +1036,6 @@ fn send_initialize_error( .map_err(crate::util::internal_error) } -#[cfg(feature = "unstable_protocol_v2")] -async fn reject_initialize( - client: RunningProtocolPeer, - frame: &TransportFrame, - error: crate::Error, -) -> Result<(), crate::Error> { - let RunningProtocolPeer { mut rx, tx, driver } = client; - send_initialize_error(&tx, frame, error)?; - drop(tx); - - let Some(mut driver) = driver.into_driver() else { - // The rejection has already been handed to the raw channel. There is - // no owned transport work or physical drain to await. - return Ok(()); - }; - if !driver.request_finish() { - // An opaque driver has no finite physical-finish contract. Preserve a - // ready error before cancelling it rather than wait for remote EOF. - return crate::util::run_until(driver, future::ready(Ok(()))).await; - } - - let drain_incoming = async move { - // Later input has no protocol meaning once initialization is rejected. - // Keep draining it only so the transport can flush the queued rejection. - // Treating a malformed trailing frame as fatal would cancel that flush. - while rx.next().await.is_some() {} - Ok::<_, crate::Error>(()) - }; - - match future::select(driver, Box::pin(drain_incoming)).await { - future::Either::Left((result, _)) => result, - future::Either::Right((result, driver)) => { - result?; - driver.await - } - } -} - #[cfg(feature = "unstable_protocol_v2")] struct RunningProtocolPeer { rx: futures::channel::mpsc::UnboundedReceiver, @@ -1548,9 +1528,11 @@ impl Proxy { /// a newer request. With a v1 implementation configured, everything else /// is handed to the v1 implementation, which negotiates it as it would /// without the router. A v2-only router rejects a request below v2 with - /// `-32022`. A request for exactly the selected version is handed over in - /// the complete, unchanged initial transport frame. The router does not - /// convert later messages. + /// `-32022`. Like the agent router, it answers each rejected request or + /// malformed frame with an error and keeps reading, so the conductor can + /// send `proxy/initialize` again. A request for exactly the selected + /// version is handed over in the complete, unchanged initial transport + /// frame. The router does not convert later messages. /// /// Requires the `unstable_protocol_v2` crate feature while protocol v2 /// stabilizes. @@ -1604,46 +1586,15 @@ impl ConnectTo for ProxyProtocolRouter { v1: self.v1.is_some(), v2: self.v2.is_some(), }; - let mut conductor = RunningProtocolPeer::new(conductor); - let (first_frame, conductor, selected) = loop { - let Some((mut frame, next_conductor)) = conductor.next_frame().await? else { - return Ok(()); - }; - let message = match initialize_message_mut(&mut frame) { - Ok(Some(message)) => message, - Ok(None) => { - conductor = next_conductor; - continue; - } - // A v1 implementation answers anything that does not select - // v2 itself, exactly as it would without the router. - Err(_) if supported.v1 => break (frame, next_conductor, SelectedProtocol::V1), - Err(error) => return reject_initialize(next_conductor, &frame, error).await, - }; - let selected = - match select_initialize_protocol(message, METHOD_INITIALIZE_PROXY, supported) { - Ok(selected) => selected, - // A v1 implementation answers any other `proxy/initialize` - // that does not select v2 itself, exactly as it would without - // the router. - Err(_) - if supported.v1 - && !requests_protocol_v2( - message, - METHOD_INITIALIZE_PROXY, - supported, - ) => - { - SelectedProtocol::V1 - } - Err(error) => return reject_initialize(next_conductor, &frame, error).await, - }; - break (frame, next_conductor, selected); - }; - let Some(proxy) = selected.take_proxy(self) else { - let error = selected.unsupported_error(supported); - return reject_initialize(conductor, &first_frame, error).await; + let conductor = RunningProtocolPeer::new(conductor); + let Some((first_frame, conductor, selected)) = + select_protocol_peer(conductor, METHOD_INITIALIZE_PROXY, supported).await? + else { + return Ok(()); }; + let proxy = selected + .take_proxy(self) + .expect("the router only selects a configured implementation"); let proxy = RunningProtocolPeer::new(proxy); proxy.send_frame(first_frame)?; diff --git a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs index 6ed7b30e..df4a9fa1 100644 --- a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs +++ b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs @@ -173,6 +173,77 @@ async fn v2_only_router_rejects_v1_like_a_v2_agent() -> Result<(), Error> { Ok(()) } +/// Wait for the response to `id` on `rx`, skipping other frames. +async fn response_to( + rx: &mut futures::channel::mpsc::UnboundedReceiver, + id: &RawJsonRpcId, +) -> Result, Error> { + loop { + let frame = rx.next().await.ok_or_else(|| { + Error::internal_error().data("proxy router closed before the response") + })?; + let TransportFrame::Single(RawJsonRpcMessage::Response(response)) = frame else { + continue; + }; + match response { + RawJsonRpcResponse::Result { id: got, result } if got == *id => return Ok(Ok(result)), + RawJsonRpcResponse::Error { id: got, error } if got == *id => { + return Ok(Err(error.into_acp_error())); + } + _ => {} + } + } +} + +#[tokio::test(flavor = "current_thread")] +async fn accepts_proxy_initialize_again_after_a_rejection() -> Result<(), Error> { + let v2_hits = Arc::new(AtomicUsize::new(0)); + let router = Proxy + .protocol_router() + .with_v2(v2_proxy(Arc::clone(&v2_hits))); + let (Channel { mut rx, tx }, future) = ConnectTo::::into_channel_and_future(router); + let task = tokio::spawn(future.expect("proxy router owns a connection driver")); + let send = |message: RawJsonRpcMessage| { + tx.unbounded_send(TransportFrame::Single(message)) + .map_err(Error::into_internal_error) + }; + + // A notification before initialization is ignored, and a rejected + // `proxy/initialize` leaves the connection open for another attempt, + // exactly as with `Agent::protocol_router`. + send(RawJsonRpcMessage::notification( + "_com.example/early".into(), + serde_json::json!({}), + )?)?; + let rejected_id = RawJsonRpcId::Number(1); + send(RawJsonRpcMessage::request( + "proxy/initialize".into(), + serde_json::to_value(v1_proxy_initialize(ProtocolVersion::V1)) + .map_err(Error::into_internal_error)?, + rejected_id.clone(), + )?)?; + let error = response_to(&mut rx, &rejected_id) + .await? + .expect_err("a v2-only router must reject v1"); + assert_eq!(i32::from(error.code), -32022); + + let accepted_id = RawJsonRpcId::Number(2); + send(RawJsonRpcMessage::request( + "proxy/initialize".into(), + serde_json::to_value(v2_proxy_initialize(ProtocolVersion::V2)) + .map_err(Error::into_internal_error)?, + accepted_id.clone(), + )?)?; + let response = response_to(&mut rx, &accepted_id).await??; + assert_eq!(response["protocolVersion"], 2); + assert_eq!(v2_hits.load(Ordering::SeqCst), 1); + + drop(tx); + drop(rx); + task.await.map_err(Error::into_internal_error)??; + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn requires_proxy_initialize_as_the_first_call() -> Result<(), Error> { let error = request( From 95e45654b4f0f7133d19e4daed1b47bceda2879e Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:33:36 +0100 Subject: [PATCH 04/11] fix(acp)!: harden Channel, parse_message and request handle conversions - `Channel` is `#[non_exhaustive]` with `Channel::new` and `Channel::into_parts`, and documents that the unbounded mpsc pair is the intended contract, so fields can be added later. - `JsonRpcMessage::parse_message` takes `params: &serde_json::Value` instead of `&impl Serialize`. Every SDK caller already passes a JSON value, and a concrete type lets implementations avoid a serialize round trip later without another break. The derive macros generate the new signature. - `SentRequest` and `PreparedRequest` gain `into_raw` and, on `Raw` handles, `into_protocol`, like `Responder` and `ResponseRouter`. Findings F38, F39, F50. BREAKING CHANGE: `Channel` can no longer be built or destructured exhaustively outside the crate; hand-written `JsonRpcMessage` implementations must change the `parse_message` signature. Co-Authored-By: Claude --- md/migration-v5.md | 27 ++++- src/agent-client-protocol-derive/src/lib.rs | 4 +- src/agent-client-protocol-http/src/client.rs | 4 + .../src/connection.rs | 1 + .../src/http_server.rs | 2 + .../src/websocket_server.rs | 1 + src/agent-client-protocol-test/src/lib.rs | 5 +- src/agent-client-protocol/src/jsonrpc.rs | 98 ++++++++++++++++++- .../src/jsonrpc/prepared_request_tests.rs | 36 ++++++- .../src/jsonrpc/protocol_compat.rs | 2 +- src/agent-client-protocol/src/schema/mod.rs | 10 +- .../src/schema/proxy_protocol.rs | 2 +- .../src/schema/v2_impls.rs | 8 +- .../tests/connection_driver_normalization.rs | 9 +- .../tests/cooperative_connection_driver.rs | 3 +- .../tests/jsonrpc_advanced.rs | 6 +- .../tests/jsonrpc_connection_builder.rs | 12 +-- .../tests/jsonrpc_deep_chain_stack.rs | 2 +- .../tests/jsonrpc_edge_cases.rs | 4 +- .../tests/jsonrpc_error_handling.rs | 8 +- .../tests/jsonrpc_hello.rs | 4 +- .../tests/jsonrpc_request_cancellation.rs | 2 +- .../tests/jsonrpc_transport_close.rs | 6 +- .../tests/jsonrpc_unhandled_messages.rs | 2 +- .../tests/match_dispatch.rs | 4 +- .../tests/protocol_driver_finish.rs | 4 +- .../tests/proxy_protocol_router_v2.rs | 6 +- .../tests/session_ordering.rs | 21 ++-- .../tests/session_restore.rs | 22 ++++- 29 files changed, 249 insertions(+), 66 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index f3237806..bafb311c 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -777,8 +777,9 @@ fn send(session: &ActiveSession) -> SentRequest { ... ### Getting a typed handle from the raw layer -`Responder`, `ResponseRouter` and `Dispatch` with the `Raw` marker convert -to a typed marker with `into_protocol`. Only the ID's type changes. It fails +`Responder`, `ResponseRouter`, `SentRequest`, `PreparedRequest` and +`Dispatch` with the `Raw` marker convert to a typed marker with +`into_protocol`. Only the ID's type changes. It fails only for a `null` ID on `V2`, with the Invalid Request error (`-32600`) a v2 connection sends for such a request; a request received on a v2 connection never has one. Any handle converts back with the infallible `into_raw`: @@ -834,6 +835,28 @@ version-neutral `ConnectionTo` with `.into()`: `v1::ConnectionTo` and requires `Into>`. Their own methods, such as the v1 session helpers, are only available on the concrete types. +## `Channel` and `JsonRpcMessage::parse_message` + +- `Channel` is `#[non_exhaustive]`. Build one with `Channel::new(rx, tx)` + (or `Channel::duplex()`) and take it apart with `into_parts()` or a pattern + with `..`: + + ```rust,ignore + // Before + let channel = Channel { rx, tx }; + let Channel { rx, tx } = channel; + + // After + let channel = Channel::new(rx, tx); + let Channel { rx, tx, .. } = channel; + ``` + +- `JsonRpcMessage::parse_message` takes `params: &serde_json::Value` instead + of `&impl Serialize`. Update hand-written implementations; derived ones + change with the derive macros. Callers holding raw + `Option` pass + `¶ms.map_or(serde_json::Value::Null, RawJsonRpcParams::into_value)`. + ## Schema enums and SDK helpers ### v1 extension methods keep their `_` prefix diff --git a/src/agent-client-protocol-derive/src/lib.rs b/src/agent-client-protocol-derive/src/lib.rs index c0c8c2aa..1435df21 100644 --- a/src/agent-client-protocol-derive/src/lib.rs +++ b/src/agent-client-protocol-derive/src/lib.rs @@ -93,7 +93,7 @@ pub fn derive_json_rpc_request(input: TokenStream) -> TokenStream { fn parse_message( #method_arg: &str, - #params_arg: &impl #krate::__private::serde::Serialize, + #params_arg: &#krate::__private::serde_json::Value, ) -> ::core::result::Result { if #method_arg != #method { return ::core::result::Result::Err(#krate::Error::method_not_found()); @@ -163,7 +163,7 @@ pub fn derive_json_rpc_notification(input: TokenStream) -> TokenStream { fn parse_message( #method_arg: &str, - #params_arg: &impl #krate::__private::serde::Serialize, + #params_arg: &#krate::__private::serde_json::Value, ) -> ::core::result::Result { if #method_arg != #method { return ::core::result::Result::Err(#krate::Error::method_not_found()); diff --git a/src/agent-client-protocol-http/src/client.rs b/src/agent-client-protocol-http/src/client.rs index e17826a4..af76b6fd 100644 --- a/src/agent-client-protocol-http/src/client.rs +++ b/src/agent-client-protocol-http/src/client.rs @@ -361,6 +361,7 @@ async fn run_with_finish( let Channel { rx: outgoing, tx: incoming, + .. } = channel; let mut outgoing = finishable_outgoing(outgoing, finish); let (sse_event_tx, mut sse_event_rx) = mpsc::unbounded::(); @@ -1530,6 +1531,7 @@ where let Channel { rx: outgoing, tx: incoming, + .. } = channel; let mut outgoing = finishable_outgoing(outgoing, finish); let writer = async move { @@ -3700,6 +3702,7 @@ mod tests { let Channel { tx: outgoing, rx: incoming, + .. } = caller; drop(incoming); @@ -3746,6 +3749,7 @@ mod tests { let Channel { tx: outgoing, rx: incoming, + .. } = caller; drop(incoming); outgoing diff --git a/src/agent-client-protocol-http/src/connection.rs b/src/agent-client-protocol-http/src/connection.rs index 2fdc45d5..6610b126 100644 --- a/src/agent-client-protocol-http/src/connection.rs +++ b/src/agent-client-protocol-http/src/connection.rs @@ -427,6 +427,7 @@ impl ConnectionRegistry { let Channel { rx: mut agent_rx, tx: mut agent_tx, + .. } = channel; let inbound = async move { while let Some(msg) = inbound_rx.recv().await { diff --git a/src/agent-client-protocol-http/src/http_server.rs b/src/agent-client-protocol-http/src/http_server.rs index 4285b765..ac150586 100644 --- a/src/agent-client-protocol-http/src/http_server.rs +++ b/src/agent-client-protocol-http/src/http_server.rs @@ -472,6 +472,7 @@ mod tests { let Channel { rx: mut incoming, tx: _, + .. } = agent; while let Some(frame) = incoming.next().await { let TransportFrame::Single(message) = frame else { @@ -522,6 +523,7 @@ mod tests { let Channel { rx: mut incoming, tx: _outgoing, + .. } = agent; drop(incoming.next().await); std::future::pending::>().await diff --git a/src/agent-client-protocol-http/src/websocket_server.rs b/src/agent-client-protocol-http/src/websocket_server.rs index 9c11b7ac..f5043f38 100644 --- a/src/agent-client-protocol-http/src/websocket_server.rs +++ b/src/agent-client-protocol-http/src/websocket_server.rs @@ -264,6 +264,7 @@ mod tests { let Channel { rx: mut incoming, tx: outgoing, + .. } = agent; while let Some(frame) = incoming.next().await { match frame { diff --git a/src/agent-client-protocol-test/src/lib.rs b/src/agent-client-protocol-test/src/lib.rs index 55f72fac..059df304 100644 --- a/src/agent-client-protocol-test/src/lib.rs +++ b/src/agent-client-protocol-test/src/lib.rs @@ -132,7 +132,10 @@ macro_rules! impl_jr_message { fn to_untyped_message(&self) -> Result { UntypedMessage::new($method, self) } - fn parse_message(method: &str, params: &impl Serialize) -> Result { + fn parse_message( + method: &str, + params: &serde_json::Value, + ) -> Result { if !Self::matches_method(method) { return Err(crate::Error::method_not_found()); } diff --git a/src/agent-client-protocol/src/jsonrpc.rs b/src/agent-client-protocol/src/jsonrpc.rs index 9697b326..ec3967e1 100644 --- a/src/agent-client-protocol/src/jsonrpc.rs +++ b/src/agent-client-protocol/src/jsonrpc.rs @@ -661,7 +661,7 @@ where /// # fn matches_method(_method: &str) -> bool { false } /// # fn method(&self) -> &str { "myRequests" } /// # fn to_untyped_message(&self) -> Result { todo!() } -/// # fn parse_message(_method: &str, _params: &impl serde::Serialize) -> Result { Err(agent_client_protocol::Error::method_not_found()) } +/// # fn parse_message(_method: &str, _params: &serde_json::Value) -> Result { Err(agent_client_protocol::Error::method_not_found()) } /// # } /// impl JsonRpcRequest for MyRequests { type Response = serde_json::Value; } /// @@ -5142,11 +5142,12 @@ pub trait JsonRpcMessage: 'static + Debug + Sized + Send + Clone { /// Convert this message into an untyped message. fn to_untyped_message(&self) -> Result; - /// Parse this type from a method name and parameters. + /// Parse this type from a method name and the message's JSON `params`. /// /// Returns an error if the method doesn't match or deserialization fails. /// Callers should use `matches_method` first to check if this type handles the method. - fn parse_message(method: &str, params: &impl Serialize) -> Result; + /// A message without `params` is parsed from [`serde_json::Value::Null`]. + fn parse_message(method: &str, params: &serde_json::Value) -> Result; } /// Defines the "payload" of a successful response to a JSON-RPC request. @@ -5668,7 +5669,7 @@ impl JsonRpcMessage for UntypedMessage { Ok(self.clone()) } - fn parse_message(method: &str, params: &impl Serialize) -> Result { + fn parse_message(method: &str, params: &serde_json::Value) -> Result { UntypedMessage::new(method, params) } } @@ -5906,6 +5907,32 @@ impl Debug for PreparedRequest { } } +impl PreparedRequest { + /// The same prepared request with a version-neutral [`Raw`] request ID, + /// for code that is not tied to one protocol version. This never fails + /// and does not publish the request. + pub fn into_raw(self) -> PreparedRequest { + PreparedRequest { + sent: self.sent.into_raw(), + publication: self.publication, + } + } +} + +impl PreparedRequest { + /// Convert this version-neutral prepared request to protocol `Q`'s + /// handle without publishing it. + /// + /// Only the request ID's type changes. The SDK never assigns `null` to + /// requests it sends, so this succeeds for every marker. + pub fn into_protocol(self) -> Result, crate::Error> { + Ok(PreparedRequest { + sent: self.sent.into_protocol()?, + publication: self.publication, + }) + } +} + impl PreparedRequest { /// The ID reserved for this request, which has not been sent yet, as /// this request's protocol version types it. @@ -6473,6 +6500,41 @@ impl SentRequest { } } +impl SentRequest { + /// The same request handle with a version-neutral [`Raw`] request ID, for + /// code that is not tied to one protocol version. This never fails. + pub fn into_raw(self) -> SentRequest { + self.retag_id(HandleId::into_raw) + } + + fn retag_id( + self, + retag: impl FnOnce(HandleId

) -> HandleId, + ) -> SentRequest { + SentRequest { + id: retag(self.id), + method: self.method, + task_tx: self.task_tx, + response_rx: self.response_rx, + to_result: self.to_result, + cancellation: self.cancellation, + response_ordering: self.response_ordering, + cancellation_sources: self.cancellation_sources, + } + } +} + +impl SentRequest { + /// Convert this version-neutral request handle to protocol `Q`'s handle. + /// + /// Only the request ID's type changes. The SDK never assigns `null` to + /// requests it sends, so this succeeds for every marker. + pub fn into_protocol(self) -> Result, crate::Error> { + let id = self.id.clone().into_protocol()?; + Ok(self.retag_id(|_| id)) + } +} + impl SentRequest { /// The id of the outgoing request, as this request's protocol version /// types it. It is the ID written to the wire; the SDK never assigns @@ -7243,6 +7305,12 @@ where /// explicit frame; failures while driving a physical transport are returned by that /// transport's future. /// +/// The unbounded [`futures::channel::mpsc`] pair is the intended contract: +/// frames are never dropped and sends never wait, so the transport or +/// component on each side applies its own backpressure. Build a channel from +/// an existing pair with [`Channel::new`] and take it apart with +/// [`Channel::into_parts`] or by destructuring with `..`. +/// /// # Example /// /// ```no_run @@ -7259,6 +7327,7 @@ where /// # } /// ``` #[derive(Debug)] +#[non_exhaustive] pub struct Channel { /// Receives frames from the counterpart. pub rx: mpsc::UnboundedReceiver, @@ -7267,6 +7336,27 @@ pub struct Channel { } impl Channel { + /// Create a channel endpoint that receives frames on `rx` and sends them + /// on `tx`. + #[must_use] + pub fn new( + rx: mpsc::UnboundedReceiver, + tx: mpsc::UnboundedSender, + ) -> Self { + Self { rx, tx } + } + + /// Split this endpoint into its receiving and sending halves, `(rx, tx)`. + #[must_use] + pub fn into_parts( + self, + ) -> ( + mpsc::UnboundedReceiver, + mpsc::UnboundedSender, + ) { + (self.rx, self.tx) + } + /// Create a pair of connected channel endpoints. /// /// Frames sent through either endpoint are received by the other endpoint. diff --git a/src/agent-client-protocol/src/jsonrpc/prepared_request_tests.rs b/src/agent-client-protocol/src/jsonrpc/prepared_request_tests.rs index 7388aa2e..bd5c9d8c 100644 --- a/src/agent-client-protocol/src/jsonrpc/prepared_request_tests.rs +++ b/src/agent-client-protocol/src/jsonrpc/prepared_request_tests.rs @@ -58,6 +58,40 @@ fn blocking_publishes_before_polling_and_drop_cancels() { ); } +#[test] +fn protocol_conversions_keep_the_request() { + let mut fixture = Fixture::new(); + let prepared = fixture + .connection + .prepare_request(request()) + .into_protocol::() + .unwrap(); + let id = prepared.raw_id().clone(); + assert_eq!(RawJsonRpcId::from(prepared.id().clone()), id); + // Conversion does not publish. + assert!(fixture.message_rx.next().now_or_never().is_none()); + + let prepared: PreparedRequest = prepared.into_raw(); + assert_eq!(prepared.id(), &id); + drop(prepared); + + let sent = fixture + .connection + .send_request(request()) + .into_protocol::() + .unwrap(); + let id = sent.raw_id().clone(); + let sent: SentRequest = sent.into_raw(); + assert_eq!(sent.id(), &id); + let response = sent.block_task(); + fixture.next_message(); + assert!(fixture.route(id, Ok(json!({"value": 42}))).is_none()); + assert_eq!( + futures::executor::block_on(response).unwrap(), + json!({"value": 42}) + ); +} + #[test] fn blocking_maps_the_response_without_holding_dispatch() { let mut fixture = Fixture::new(); @@ -644,7 +678,7 @@ impl JsonRpcMessage for FailingRequest { fn to_untyped_message(&self) -> Result { Err(crate::Error::invalid_params()) } - fn parse_message(_method: &str, _params: &impl Serialize) -> Result { + fn parse_message(_method: &str, _params: &serde_json::Value) -> Result { Err(crate::Error::invalid_params()) } } diff --git a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs index e25f43a7..a66a3e7f 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs @@ -335,7 +335,7 @@ impl crate::JsonRpcMessage for InitializeNegotiation { self.0.to_untyped_message() } - fn parse_message(method: &str, params: &impl serde::Serialize) -> Result { + fn parse_message(method: &str, params: &serde_json::Value) -> Result { crate::schema::v2::InitializeRequest::parse_message(method, params).map(Self) } } diff --git a/src/agent-client-protocol/src/schema/mod.rs b/src/agent-client-protocol/src/schema/mod.rs index a88c2f7a..eff63657 100644 --- a/src/agent-client-protocol/src/schema/mod.rs +++ b/src/agent-client-protocol/src/schema/mod.rs @@ -31,7 +31,7 @@ macro_rules! impl_jsonrpc_request { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if method != $method { return Err($crate::Error::method_not_found()); @@ -79,7 +79,7 @@ macro_rules! impl_jsonrpc_notification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if method != $method { return Err($crate::Error::method_not_found()); @@ -127,7 +127,7 @@ macro_rules! impl_jsonrpc_request_enum { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { match method { $( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )* @@ -189,7 +189,7 @@ macro_rules! impl_jsonrpc_notification_enum { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { match method { $( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )* @@ -256,7 +256,7 @@ macro_rules! impl_jsonrpc_protocol_level_notification_enum { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { match method { $( $method => $crate::util::json_cast_params(params).map(Self::$variant), )* diff --git a/src/agent-client-protocol/src/schema/proxy_protocol.rs b/src/agent-client-protocol/src/schema/proxy_protocol.rs index 6094c0c1..8cd51e2f 100644 --- a/src/agent-client-protocol/src/schema/proxy_protocol.rs +++ b/src/agent-client-protocol/src/schema/proxy_protocol.rs @@ -90,7 +90,7 @@ impl JsonRpcMessage for SuccessorMessage { ) } - fn parse_message(method: &str, params: &impl Serialize) -> Result { + fn parse_message(method: &str, params: &serde_json::Value) -> Result { if method != METHOD_SUCCESSOR_MESSAGE { return Err(crate::Error::method_not_found()); } diff --git a/src/agent-client-protocol/src/schema/v2_impls.rs b/src/agent-client-protocol/src/schema/v2_impls.rs index 94ef0ed6..5a5ab05b 100644 --- a/src/agent-client-protocol/src/schema/v2_impls.rs +++ b/src/agent-client-protocol/src/schema/v2_impls.rs @@ -20,7 +20,7 @@ macro_rules! impl_v2_jsonrpc_request { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if method != $method { return Err(crate::Error::method_not_found()); @@ -62,7 +62,7 @@ macro_rules! impl_v2_jsonrpc_notification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if method != $method { return Err(crate::Error::method_not_found()); @@ -98,7 +98,7 @@ macro_rules! impl_v2_jsonrpc_request_enum { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { match method { $( $(#[$meta])* $method => crate::util::json_cast_params(params).map(Self::$variant), )* @@ -149,7 +149,7 @@ macro_rules! impl_v2_jsonrpc_notification_enum { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { match method { $( $(#[$meta])* $method => crate::util::json_cast_params(params).map(Self::$variant), )* diff --git a/src/agent-client-protocol/tests/connection_driver_normalization.rs b/src/agent-client-protocol/tests/connection_driver_normalization.rs index c7d7b88b..b6af0f4c 100644 --- a/src/agent-client-protocol/tests/connection_driver_normalization.rs +++ b/src/agent-client-protocol/tests/connection_driver_normalization.rs @@ -43,7 +43,7 @@ impl JsonRpcMessage for ProbeNotification { UntypedMessage::new(self.method(), self) } - fn parse_message(method: &str, params: &impl Serialize) -> Result { + fn parse_message(method: &str, params: &serde_json::Value) -> Result { if !Self::matches_method(method) { return Err(Error::method_not_found()); } @@ -350,7 +350,7 @@ async fn finite_builder_stops_buffered_input_delivery_without_closing_raw_receiv #[tokio::test] async fn underway_close_keeps_callback_phase_task_progress_before_sealing_output() { let (channel, peer) = Channel::duplex(); - let Channel { mut rx, tx } = peer; + let Channel { mut rx, tx, .. } = peer; drop(tx); let (entered_tx, entered_rx) = futures::channel::oneshot::channel(); let (release_tx, release_rx) = futures::channel::oneshot::channel(); @@ -409,7 +409,7 @@ async fn underway_close_keeps_callback_phase_task_progress_before_sealing_output #[tokio::test] async fn success_does_not_resume_message_delivery_or_start_queued_tasks_for_drain() { let (channel, peer) = Channel::duplex(); - let Channel { mut rx, tx } = peer; + let Channel { mut rx, tx, .. } = peer; for message in (0..2).map(|sequence| { let TransportFrame::Single(message) = frame(sequence) else { unreachable!() @@ -499,6 +499,7 @@ async fn reactive_completion_probe(owned: bool) { let Channel { rx: remote_receive_guard, tx: escaped_producer, + .. } = peer; for sequence in 0..3 { escaped_producer.unbounded_send(frame(sequence)).unwrap(); @@ -598,7 +599,7 @@ async fn normalized_split_duplex_half_close_allows_final_reverse_response() { let transport = ByteStreams::new(sdk_outgoing.compat_write(), sdk_incoming.compat()); let (channel, driver) = ConnectTo::::into_channel_and_future(transport); let mut driver = Box::pin(driver.expect("ByteStreams must own transport work")); - let Channel { mut rx, tx } = channel; + let Channel { mut rx, tx, .. } = channel; tx.unbounded_send(TransportFrame::Single( RawJsonRpcMessage::request( "normalization-request".into(), diff --git a/src/agent-client-protocol/tests/cooperative_connection_driver.rs b/src/agent-client-protocol/tests/cooperative_connection_driver.rs index f1eb7469..034b1c50 100644 --- a/src/agent-client-protocol/tests/cooperative_connection_driver.rs +++ b/src/agent-client-protocol/tests/cooperative_connection_driver.rs @@ -47,7 +47,7 @@ impl JsonRpcMessage for ProbeNotification { UntypedMessage::new(self.method(), self) } - fn parse_message(method: &str, params: &impl Serialize) -> Result { + fn parse_message(method: &str, params: &serde_json::Value) -> Result { if !Self::matches_method(method) { return Err(Error::method_not_found()); } @@ -233,6 +233,7 @@ fn harness(mode: ProbeMode, fail_flush: bool) -> Harness { let Channel { rx: mut output, tx: input, + .. } = physical; let (sdk_output, peer_output) = tokio::io::duplex(1); let (peer_input, sdk_input) = tokio::io::duplex(4096); diff --git a/src/agent-client-protocol/tests/jsonrpc_advanced.rs b/src/agent-client-protocol/tests/jsonrpc_advanced.rs index 2790276b..04d688a0 100644 --- a/src/agent-client-protocol/tests/jsonrpc_advanced.rs +++ b/src/agent-client-protocol/tests/jsonrpc_advanced.rs @@ -77,7 +77,7 @@ impl JsonRpcMessage for PingRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -150,7 +150,7 @@ impl JsonRpcMessage for SlowRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -203,7 +203,7 @@ impl JsonRpcMessage for AfterResponseNotification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs b/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs index fb3ca1e3..8d09ef1d 100644 --- a/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs +++ b/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs @@ -55,7 +55,7 @@ impl JsonRpcMessage for FooRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -108,7 +108,7 @@ impl JsonRpcMessage for BarRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -256,7 +256,7 @@ impl JsonRpcMessage for TrackRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -383,7 +383,7 @@ impl JsonRpcMessage for Method1Request { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -418,7 +418,7 @@ impl JsonRpcMessage for Method2Request { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -611,7 +611,7 @@ impl JsonRpcMessage for EventNotification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs b/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs index 6cc5616d..c27f8540 100644 --- a/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs +++ b/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs @@ -60,7 +60,7 @@ macro_rules! test_message { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs b/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs index be4ac430..60541b9c 100644 --- a/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs +++ b/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs @@ -142,7 +142,7 @@ impl JsonRpcMessage for EmptyRequest { fn parse_message( method: &str, - _params: &impl serde::Serialize, + _params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -178,7 +178,7 @@ impl JsonRpcMessage for OptionalParamsRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_error_handling.rs b/src/agent-client-protocol/tests/jsonrpc_error_handling.rs index 0552909e..5a81c87c 100644 --- a/src/agent-client-protocol/tests/jsonrpc_error_handling.rs +++ b/src/agent-client-protocol/tests/jsonrpc_error_handling.rs @@ -198,7 +198,7 @@ impl JsonRpcMessage for SimpleRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -251,7 +251,7 @@ impl JsonRpcMessage for SimpleNotification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -516,7 +516,7 @@ impl JsonRpcMessage for ErrorRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -612,7 +612,7 @@ impl JsonRpcMessage for EmptyRequest { fn parse_message( method: &str, - _params: &impl serde::Serialize, + _params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_hello.rs b/src/agent-client-protocol/tests/jsonrpc_hello.rs index 90233d94..47212bce 100644 --- a/src/agent-client-protocol/tests/jsonrpc_hello.rs +++ b/src/agent-client-protocol/tests/jsonrpc_hello.rs @@ -69,7 +69,7 @@ impl JsonRpcMessage for PingRequest { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -185,7 +185,7 @@ impl JsonRpcMessage for LogNotification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs b/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs index 29e38824..6a02f22c 100644 --- a/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs +++ b/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs @@ -104,7 +104,7 @@ impl JsonRpcMessage for SimpleRequest { fn parse_message( method: &str, - params: &impl Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/jsonrpc_transport_close.rs b/src/agent-client-protocol/tests/jsonrpc_transport_close.rs index 60922a11..b46acad6 100644 --- a/src/agent-client-protocol/tests/jsonrpc_transport_close.rs +++ b/src/agent-client-protocol/tests/jsonrpc_transport_close.rs @@ -144,7 +144,7 @@ impl JsonRpcMessage for BlockingRequest { UntypedMessage::new(self.method(), serde_json::json!({})) } - fn parse_message(_method: &str, _params: &impl serde::Serialize) -> Result { + fn parse_message(_method: &str, _params: &serde_json::Value) -> Result { Err(Error::method_not_found().data("test-only outgoing request")) } } @@ -629,7 +629,7 @@ async fn channel_peer_receives_final_response_after_write_half_closes() { .connect_to(transport); let peer = async move { - let Channel { mut rx, tx } = peer; + let Channel { mut rx, tx, .. } = peer; tx.unbounded_send(TransportFrame::Single( RawJsonRpcMessage::request( "myRequest".into(), @@ -683,7 +683,7 @@ async fn transport_channel_keeps_read_half_open_after_write_half_closes() { let transport = ByteStreams::new(sdk_outgoing.compat_write(), sdk_incoming.compat()); let (channel, transport_future) = ConnectTo::::into_channel_and_future(transport); let transport_future = transport_future.expect("byte streams own a transport driver"); - let Channel { mut rx, tx } = channel; + let Channel { mut rx, tx, .. } = channel; tx.unbounded_send(TransportFrame::Single( RawJsonRpcMessage::request( diff --git a/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs b/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs index 8b62976b..1eef1258 100644 --- a/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs +++ b/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs @@ -79,7 +79,7 @@ impl JsonRpcMessage for SimpleRequest { fn parse_message( method: &str, - params: &impl Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/match_dispatch.rs b/src/agent-client-protocol/tests/match_dispatch.rs index 3ad0f0ea..8a6aaca7 100644 --- a/src/agent-client-protocol/tests/match_dispatch.rs +++ b/src/agent-client-protocol/tests/match_dispatch.rs @@ -33,7 +33,7 @@ impl JsonRpcMessage for EchoRequestResponse { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); @@ -79,7 +79,7 @@ impl JsonRpcMessage for RetryNotification { fn parse_message( method: &str, - params: &impl serde::Serialize, + params: &serde_json::Value, ) -> Result { if !Self::matches_method(method) { return Err(agent_client_protocol::Error::method_not_found()); diff --git a/src/agent-client-protocol/tests/protocol_driver_finish.rs b/src/agent-client-protocol/tests/protocol_driver_finish.rs index 78e19002..6aa8d146 100644 --- a/src/agent-client-protocol/tests/protocol_driver_finish.rs +++ b/src/agent-client-protocol/tests/protocol_driver_finish.rs @@ -193,7 +193,7 @@ async fn run_over_lines( #[tokio::test(flavor = "current_thread")] async fn client_connector_cancels_opaque_driver_after_final_frame() -> Result<(), Error> { - let (channel, Channel { mut rx, tx }) = Channel::duplex(); + let (channel, Channel { mut rx, tx, .. }) = Channel::duplex(); let input_guard = tx.clone(); let dropped = Arc::new(AtomicBool::new(false)); let drop_signal = DropSignal(dropped.clone()); @@ -273,7 +273,7 @@ async fn rejected_initialize_flushes_lines_without_input_eof() -> Result<(), Err #[tokio::test(flavor = "current_thread")] async fn rejected_initialize_requests_custom_finish_once_after_handoff() -> Result<(), Error> { for fail_flush in [false, true] { - let (channel, Channel { mut rx, tx }) = Channel::duplex(); + let (channel, Channel { mut rx, tx, .. }) = Channel::duplex(); let input_guard = tx; input_guard .unbounded_send(TransportFrame::Single(RawJsonRpcMessage::request( diff --git a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs index df4a9fa1..77133d60 100644 --- a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs +++ b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs @@ -75,7 +75,8 @@ async fn request( method: &str, params: Value, ) -> Result, Error> { - let (Channel { mut rx, tx }, future) = ConnectTo::::into_channel_and_future(router); + let (Channel { mut rx, tx, .. }, future) = + ConnectTo::::into_channel_and_future(router); let task = tokio::spawn(future.expect("proxy router owns a connection driver")); let request_id = RawJsonRpcId::Number(1); @@ -201,7 +202,8 @@ async fn accepts_proxy_initialize_again_after_a_rejection() -> Result<(), Error> let router = Proxy .protocol_router() .with_v2(v2_proxy(Arc::clone(&v2_hits))); - let (Channel { mut rx, tx }, future) = ConnectTo::::into_channel_and_future(router); + let (Channel { mut rx, tx, .. }, future) = + ConnectTo::::into_channel_and_future(router); let task = tokio::spawn(future.expect("proxy router owns a connection driver")); let send = |message: RawJsonRpcMessage| { tx.unbounded_send(TransportFrame::Single(message)) diff --git a/src/agent-client-protocol/tests/session_ordering.rs b/src/agent-client-protocol/tests/session_ordering.rs index 0381e6c0..0cc5e4d0 100644 --- a/src/agent-client-protocol/tests/session_ordering.rs +++ b/src/agent-client-protocol/tests/session_ordering.rs @@ -25,6 +25,15 @@ use agent_client_protocol::{ const TIMEOUT: Duration = Duration::from_secs(10); +/// The JSON value `parse_message` takes for a raw message's `params`. +#[cfg(feature = "unstable_protocol_v2")] +fn params_value(params: &Option) -> serde_json::Value { + params.clone().map_or( + serde_json::Value::Null, + agent_client_protocol::RawJsonRpcParams::into_value, + ) +} + #[cfg(feature = "unstable_protocol_v2")] async fn initialize_raw_v2_proxy( peer: &mut Channel, @@ -636,7 +645,7 @@ async fn v2_proxy_session_start_installs_routing_before_later_message() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - &forwarded.params, + ¶ms_value(&forwarded.params), )?; assert_eq!( successor.message.cwd, @@ -678,7 +687,7 @@ async fn v2_proxy_session_start_installs_routing_before_later_message() { RawJsonRpcMessage::Notification(notification) => { let update = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¬ification.params, + ¶ms_value(¬ification.params), )?; assert_eq!(update.session_id, session_id); assert!(matches!( @@ -785,7 +794,7 @@ async fn v2_proxy_fork_installs_response_id_routing_before_later_message() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - &forwarded.params, + ¶ms_value(&forwarded.params), )?; assert_eq!(successor.message.session_id, expected_source_session_id); assert_eq!( @@ -829,7 +838,7 @@ async fn v2_proxy_fork_installs_response_id_routing_before_later_message() { RawJsonRpcMessage::Notification(notification) => { let update = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¬ification.params, + ¶ms_value(¬ification.params), )?; assert_eq!(update.session_id, forked_session_id); assert!(matches!( @@ -936,7 +945,7 @@ async fn v2_proxy_resume_forwards_replay_before_its_response() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - &forwarded.params, + ¶ms_value(&forwarded.params), )?; assert_eq!(successor.message.session_id, session_id); @@ -964,7 +973,7 @@ async fn v2_proxy_resume_forwards_replay_before_its_response() { }; let replay = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¬ification.params, + ¶ms_value(¬ification.params), )?; assert_eq!(replay.session_id, session_id); assert!(matches!( diff --git a/src/agent-client-protocol/tests/session_restore.rs b/src/agent-client-protocol/tests/session_restore.rs index 6300b998..cf8d92e6 100644 --- a/src/agent-client-protocol/tests/session_restore.rs +++ b/src/agent-client-protocol/tests/session_restore.rs @@ -30,6 +30,14 @@ const DISPATCH_BARRIER_METHOD: &str = "_test/session-restore-dispatch-barrier"; #[notification(method = "_test/session-restore-dispatch-barrier")] struct DispatchBarrierNotification {} +/// The JSON value `parse_message` takes for a raw message's `params`. +fn params_value(params: &Option) -> serde_json::Value { + params.clone().map_or( + serde_json::Value::Null, + agent_client_protocol::RawJsonRpcParams::into_value, + ) +} + fn modes_state() -> SessionModeState { SessionModeState::new( SessionModeId::new("default"), @@ -138,7 +146,8 @@ async fn load_session_preserves_pre_response_replay_and_exact_response() { else { panic!("expected session/load") }; - let parsed = LoadSessionRequest::parse_message(&request.method, &request.params)?; + let parsed = + LoadSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?; assert_eq!(parsed.session_id, peer_session_id); assert_eq!(parsed.cwd, PathBuf::from("/restore/load")); @@ -200,7 +209,8 @@ async fn resume_session_returns_exact_response_and_an_active_session() { else { panic!("expected session/resume") }; - let parsed = ResumeSessionRequest::parse_message(&request.method, &request.params)?; + let parsed = + ResumeSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?; assert_eq!(parsed.session_id, session_id); for message in [ @@ -251,7 +261,7 @@ async fn resume_session_from_preserves_the_existing_request() { panic!("expected session/resume") }; assert_eq!( - ResumeSessionRequest::parse_message(&request.method, &request.params)?, + ResumeSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?, peer_request ); peer.tx @@ -484,8 +494,10 @@ async fn cancelling_restore_cancels_request_and_removes_routing() { else { panic!("dropping the restore future should send $/cancel_request") }; - let cancellation = - CancelRequestNotification::parse_message(¬ification.method, ¬ification.params)?; + let cancellation = CancelRequestNotification::parse_message( + ¬ification.method, + ¶ms_value(¬ification.params), + )?; assert_eq!(cancellation.request_id, restore_request_id.into()); for message in [ From 9663bfa1596df9a7166d384b815e452041a48bcb Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 11:54:42 +0100 Subject: [PATCH 05/11] test(acp): satisfy clippy in the new conversion tests Co-Authored-By: Claude --- .../src/http_server.rs | 4 +--- .../tests/session_ordering.rs | 16 ++++++------- .../tests/session_restore.rs | 23 ++++++++++++------- src/agent-client-protocol/tests/session_v2.rs | 4 ++++ 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/agent-client-protocol-http/src/http_server.rs b/src/agent-client-protocol-http/src/http_server.rs index ac150586..c45f01b4 100644 --- a/src/agent-client-protocol-http/src/http_server.rs +++ b/src/agent-client-protocol-http/src/http_server.rs @@ -470,9 +470,7 @@ mod tests { let forwarded = self.forwarded.clone(); let future = ConnectionDriver::new(async move { let Channel { - rx: mut incoming, - tx: _, - .. + rx: mut incoming, .. } = agent; while let Some(frame) = incoming.next().await { let TransportFrame::Single(message) = frame else { diff --git a/src/agent-client-protocol/tests/session_ordering.rs b/src/agent-client-protocol/tests/session_ordering.rs index 0cc5e4d0..114ec9e6 100644 --- a/src/agent-client-protocol/tests/session_ordering.rs +++ b/src/agent-client-protocol/tests/session_ordering.rs @@ -27,8 +27,8 @@ const TIMEOUT: Duration = Duration::from_secs(10); /// The JSON value `parse_message` takes for a raw message's `params`. #[cfg(feature = "unstable_protocol_v2")] -fn params_value(params: &Option) -> serde_json::Value { - params.clone().map_or( +fn params_value(params: Option<&agent_client_protocol::RawJsonRpcParams>) -> serde_json::Value { + params.cloned().map_or( serde_json::Value::Null, agent_client_protocol::RawJsonRpcParams::into_value, ) @@ -645,7 +645,7 @@ async fn v2_proxy_session_start_installs_routing_before_later_message() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - ¶ms_value(&forwarded.params), + ¶ms_value(forwarded.params.as_ref()), )?; assert_eq!( successor.message.cwd, @@ -687,7 +687,7 @@ async fn v2_proxy_session_start_installs_routing_before_later_message() { RawJsonRpcMessage::Notification(notification) => { let update = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¶ms_value(¬ification.params), + ¶ms_value(notification.params.as_ref()), )?; assert_eq!(update.session_id, session_id); assert!(matches!( @@ -794,7 +794,7 @@ async fn v2_proxy_fork_installs_response_id_routing_before_later_message() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - ¶ms_value(&forwarded.params), + ¶ms_value(forwarded.params.as_ref()), )?; assert_eq!(successor.message.session_id, expected_source_session_id); assert_eq!( @@ -838,7 +838,7 @@ async fn v2_proxy_fork_installs_response_id_routing_before_later_message() { RawJsonRpcMessage::Notification(notification) => { let update = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¶ms_value(¬ification.params), + ¶ms_value(notification.params.as_ref()), )?; assert_eq!(update.session_id, forked_session_id); assert!(matches!( @@ -945,7 +945,7 @@ async fn v2_proxy_resume_forwards_replay_before_its_response() { }; let successor = SuccessorMessage::::parse_message( forwarded.method.as_ref(), - ¶ms_value(&forwarded.params), + ¶ms_value(forwarded.params.as_ref()), )?; assert_eq!(successor.message.session_id, session_id); @@ -973,7 +973,7 @@ async fn v2_proxy_resume_forwards_replay_before_its_response() { }; let replay = v2::UpdateSessionNotification::parse_message( notification.method.as_ref(), - ¶ms_value(¬ification.params), + ¶ms_value(notification.params.as_ref()), )?; assert_eq!(replay.session_id, session_id); assert!(matches!( diff --git a/src/agent-client-protocol/tests/session_restore.rs b/src/agent-client-protocol/tests/session_restore.rs index cf8d92e6..8af27f50 100644 --- a/src/agent-client-protocol/tests/session_restore.rs +++ b/src/agent-client-protocol/tests/session_restore.rs @@ -31,8 +31,8 @@ const DISPATCH_BARRIER_METHOD: &str = "_test/session-restore-dispatch-barrier"; struct DispatchBarrierNotification {} /// The JSON value `parse_message` takes for a raw message's `params`. -fn params_value(params: &Option) -> serde_json::Value { - params.clone().map_or( +fn params_value(params: Option<&agent_client_protocol::RawJsonRpcParams>) -> serde_json::Value { + params.cloned().map_or( serde_json::Value::Null, agent_client_protocol::RawJsonRpcParams::into_value, ) @@ -146,8 +146,10 @@ async fn load_session_preserves_pre_response_replay_and_exact_response() { else { panic!("expected session/load") }; - let parsed = - LoadSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?; + let parsed = LoadSessionRequest::parse_message( + &request.method, + ¶ms_value(request.params.as_ref()), + )?; assert_eq!(parsed.session_id, peer_session_id); assert_eq!(parsed.cwd, PathBuf::from("/restore/load")); @@ -209,8 +211,10 @@ async fn resume_session_returns_exact_response_and_an_active_session() { else { panic!("expected session/resume") }; - let parsed = - ResumeSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?; + let parsed = ResumeSessionRequest::parse_message( + &request.method, + ¶ms_value(request.params.as_ref()), + )?; assert_eq!(parsed.session_id, session_id); for message in [ @@ -261,7 +265,10 @@ async fn resume_session_from_preserves_the_existing_request() { panic!("expected session/resume") }; assert_eq!( - ResumeSessionRequest::parse_message(&request.method, ¶ms_value(&request.params))?, + ResumeSessionRequest::parse_message( + &request.method, + ¶ms_value(request.params.as_ref()) + )?, peer_request ); peer.tx @@ -496,7 +503,7 @@ async fn cancelling_restore_cancels_request_and_removes_routing() { }; let cancellation = CancelRequestNotification::parse_message( ¬ification.method, - ¶ms_value(¬ification.params), + ¶ms_value(notification.params.as_ref()), )?; assert_eq!(cancellation.request_id, restore_request_id.into()); diff --git a/src/agent-client-protocol/tests/session_v2.rs b/src/agent-client-protocol/tests/session_v2.rs index be355fc1..698b816e 100644 --- a/src/agent-client-protocol/tests/session_v2.rs +++ b/src/agent-client-protocol/tests/session_v2.rs @@ -38,6 +38,10 @@ struct V1SessionGuardRunner { } impl RunWithConnectionTo for V1SessionGuardRunner { + #[expect( + clippy::unused_async_trait_impl, + reason = "the check is synchronous, the trait method is async" + )] async fn run_with_connection_to(self, connection: ConnectionTo) -> Result<(), Error> { // A v2 connection does not convert to the v1 connection type, so the // v1 session helpers cannot run over it. From 6be16a4535d25c1d8cfeeeab9cddc0d7751a0441 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 17:00:38 +0100 Subject: [PATCH 06/11] fix(acp): gate the Client import on unstable_mcp_over_acp only Removing with_v2_protocol_guard left the import unused when only unstable_protocol_v2 is enabled. Co-Authored-By: Claude --- src/agent-client-protocol/src/jsonrpc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent-client-protocol/src/jsonrpc.rs b/src/agent-client-protocol/src/jsonrpc.rs index ec3967e1..fd647c09 100644 --- a/src/agent-client-protocol/src/jsonrpc.rs +++ b/src/agent-client-protocol/src/jsonrpc.rs @@ -51,7 +51,7 @@ use crate::jsonrpc::protocol::HandleId; #[cfg(feature = "unstable_protocol_v2")] pub use crate::jsonrpc::protocol::V2; -#[cfg(any(feature = "unstable_protocol_v2", feature = "unstable_mcp_over_acp"))] +#[cfg(feature = "unstable_mcp_over_acp")] use crate::Client; pub use crate::jsonrpc::protocol::{Protocol, Raw, V1}; #[cfg(feature = "unstable_protocol_v2")] From fd67416ac9d172a10901e8a998851cc5548eb5d6 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 17:32:04 +0100 Subject: [PATCH 07/11] test(acp): cover the proxy router's rejection flush under retry semantics ProxyProtocolRouter no longer finishes after a rejected proxy/initialize, so the rejection-drain tests no longer applied. Check instead that the rejection reaches the wire while input stays open and that the router finishes on input EOF. Co-Authored-By: Claude --- .../tests/protocol_driver_finish.rs | 137 +++++------------- 1 file changed, 40 insertions(+), 97 deletions(-) diff --git a/src/agent-client-protocol/tests/protocol_driver_finish.rs b/src/agent-client-protocol/tests/protocol_driver_finish.rs index 6aa8d146..f7abc0c9 100644 --- a/src/agent-client-protocol/tests/protocol_driver_finish.rs +++ b/src/agent-client-protocol/tests/protocol_driver_finish.rs @@ -6,7 +6,7 @@ use std::{ future, io, sync::{ Arc, - atomic::{AtomicBool, AtomicUsize, Ordering}, + atomic::{AtomicBool, Ordering}, }, time::Duration, }; @@ -16,10 +16,7 @@ use agent_client_protocol::{ Lines, Proxy, RawJsonRpcMessage, RawJsonRpcResponse, Role, TransportFrame, schema::{ProtocolVersion, v1}, }; -use futures::{ - FutureExt as _, SinkExt as _, StreamExt as _, - channel::{mpsc, oneshot}, -}; +use futures::{FutureExt as _, SinkExt as _, StreamExt as _, channel::mpsc}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; @@ -252,102 +249,48 @@ async fn client_connector_cancels_opaque_driver_after_final_frame() -> Result<() Ok(()) } -// The agent router keeps a connection open after a rejected initialize so the -// client can retry, and a router with v1 configured hands anything that does -// not select v2 to the v1 implementation. A v2-only proxy router still -// finishes after rejecting the conductor's `proxy/initialize`, so it -// exercises the rejection drain. +// Both routers keep the connection open after a rejected initialize so the +// peer can retry. The rejection must still reach the wire while remote input +// stays open, and the router finishes once input reaches EOF. #[tokio::test(flavor = "current_thread")] async fn rejected_initialize_flushes_lines_without_input_eof() -> Result<(), Error> { - let frames = run_over_lines( - Proxy.protocol_router().with_v2(Proxy.v2()), - "proxy/initialize", - json!({}), - ) - .await?; - assert_eq!(frames.len(), 1, "{frames:?}"); - assert_rejection(&frames[0]); - Ok(()) -} - -#[tokio::test(flavor = "current_thread")] -async fn rejected_initialize_requests_custom_finish_once_after_handoff() -> Result<(), Error> { - for fail_flush in [false, true] { - let (channel, Channel { mut rx, tx, .. }) = Channel::duplex(); - let input_guard = tx; - input_guard - .unbounded_send(TransportFrame::Single(RawJsonRpcMessage::request( - "proxy/initialize".into(), - json!({}), - agent_client_protocol::RawJsonRpcId::Number(7), - )?)) - .map_err(Error::into_internal_error)?; - - let (finish_tx, finish_rx) = oneshot::channel(); - let (flush_started_tx, flush_started_rx) = oneshot::channel(); - let (flush_release_tx, flush_release_rx) = oneshot::channel(); - let finish_calls = Arc::new(AtomicUsize::new(0)); - let callback_calls = finish_calls.clone(); - let drained = Arc::new(AtomicBool::new(false)); - let driver_drained = drained.clone(); - let flush_error = Error::internal_error().data("rejection flush failed"); - let driver_error = flush_error.clone(); - let adapter = NormalizedAdapter { - channel, - driver: ConnectionDriver::with_finish( - async move { - // Consume no output until finish. Sealing then draining - // proves the rejection was handed off before the hook. - finish_rx - .await - .expect("explicit cooperative finish request"); - rx.close(); - let mut frames = Vec::new(); - while let Some(frame) = rx.next().await { - frames.push(frame_value(frame)?); - } - assert_eq!(frames.len(), 1, "{frames:?}"); - assert_rejection(&frames[0]); - flush_started_tx.send(()).unwrap(); - flush_release_rx.await.unwrap(); - driver_drained.store(true, Ordering::SeqCst); - if fail_flush { - Err(driver_error) - } else { - Ok(()) - } - }, - move || { - callback_calls.fetch_add(1, Ordering::SeqCst); - finish_tx.send(()).expect("owned driver is still alive"); - }, - ), - }; - let mut router = Box::pin( - Proxy - .protocol_router() - .with_v2(Proxy.v2()) - .connect_to(adapter), - ); - - assert!(router.as_mut().now_or_never().is_none()); - assert_eq!(finish_calls.load(Ordering::SeqCst), 1); - assert!(matches!(flush_started_rx.now_or_never(), Some(Ok(())))); - assert!(!drained.load(Ordering::SeqCst), "flush is still gated"); + let (input_guard, incoming) = mpsc::unbounded::>(); + let (outgoing, mut output) = mpsc::unbounded::(); + let lines = Lines::new(outgoing.sink_map_err(io::Error::other), incoming); + input_guard + .unbounded_send(Ok(json!({ + "jsonrpc": "2.0", + "id": 7, + "method": "proxy/initialize", + "params": {}, + }) + .to_string())) + .map_err(Error::into_internal_error)?; - flush_release_tx.send(()).unwrap(); - let result = tokio::time::timeout(DEADLINE, router) - .await - .expect("rejection drain must not require remote input EOF"); - if fail_flush { - assert_eq!(result, Err(flush_error)); - } else { - result?; + let mut router = Box::pin( + Proxy + .protocol_router() + .with_v2(Proxy.v2()) + .connect_to(lines), + ); + let line = tokio::time::timeout(DEADLINE, async { + futures::select! { + line = output.next() => line, + result = router.as_mut().fuse() => { + panic!("router finished before input EOF: {result:?}") + } } - assert_eq!(finish_calls.load(Ordering::SeqCst), 1); - assert!(drained.load(Ordering::SeqCst), "owned drain was awaited"); - drop(input_guard); - } + }) + .await + .expect("rejection must be flushed without input EOF") + .expect("router writes the rejection"); + assert_rejection(&serde_json::from_str(&line).map_err(Error::into_internal_error)?); + + drop(input_guard); + tokio::time::timeout(DEADLINE, router) + .await + .expect("router finishes after input EOF")?; + assert!(output.next().await.is_none(), "no further frames"); Ok(()) } From 6e8a409bd97f9cfde8d3c2f5084cad6336595a4c Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 17:42:50 +0100 Subject: [PATCH 08/11] fix(acp): report an into_protocol guard mismatch as an internal error A version mismatch in `ConnectionTo::into_protocol` is a local programming error. Reporting it as Invalid Request (-32600) told the remote peer its request was malformed when a handler propagated it with `?`. It is now an Internal error (-32603). Also tidy the migration and protocol-v2 notes: the v1 `send_cancel_request` change is source-compatible for `RawJsonRpcId` arguments, and the crate-root initialize mismatch types are described without claiming every version uses them. Co-Authored-By: Claude --- md/migration-v5.md | 15 +++++++++------ md/protocol-v2.md | 6 +++--- src/agent-client-protocol/src/jsonrpc.rs | 9 +++++---- .../src/jsonrpc/protocol_compat.rs | 4 ++-- src/agent-client-protocol/tests/session_v2.rs | 2 +- src/agent-client-protocol/tests/v1_connection.rs | 5 +---- 6 files changed, 21 insertions(+), 20 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index bafb311c..5585d2b2 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -95,10 +95,10 @@ Where code compared a v1 request's `id()` with IDs from the raw layer, use ### Converting, spawning and cancelling on connections - `ConnectionTo::into_protocol::

()` returns - `Result, Error>`. It fails with `-32600` when the - connection's builder guards another version, for example - `into_protocol::()` on a connection from `Client.v1()`, which used to - succeed and then send v2 payloads over the v1 wire. Connections from + `Result, Error>`. It fails with an Internal error + (`-32603`) when the connection's builder guards another version, for + example `into_protocol::()` on a connection from `Client.v1()`, which + used to succeed and then send v2 payloads over the v1 wire. Connections from unguarded builders such as `Builder::new` convert to every version. Add `?` to existing calls. The new `ConnectionTo::protocol_version()` returns the guarded version, or `None` for an unguarded connection. @@ -113,8 +113,11 @@ Where code compared a v1 request's `id()` with IDs from the raw layer, use - `v1::ConnectionTo::send_cancel_request(_to)` takes `impl Into` and `v2::ConnectionTo::send_cancel_request(_to)` takes `impl Into`, the type their requests' `id()` - returns. The v2 methods can no longer send `{"requestId": null}`, which v2 - forbids. Only the raw `ConnectionTo` methods take any `RawJsonRpcId`. + returns. The v1 change is source-compatible: `RawJsonRpcId` converts into + `schema::v1::RequestId`, so v1 calls that pass one still compile. V2 calls + that pass a `RawJsonRpcId` must pass a `schema::v2::RequestId` instead, so + the v2 methods can no longer send `{"requestId": null}`, which v2 forbids. + The raw `ConnectionTo` methods still take any `RawJsonRpcId`. - `Builder::with_v2_protocol_guard` (`unstable_protocol_v2`) is removed. A builder that needs the v2 guard is `Client.v2()` or `Agent.v2()`; code that needs raw handles in its callbacks converts them with `into_raw()`. diff --git a/md/protocol-v2.md b/md/protocol-v2.md index 2c6aea81..06672ab7 100644 --- a/md/protocol-v2.md +++ b/md/protocol-v2.md @@ -115,7 +115,7 @@ the same low-level `HandleDispatchFrom` interface and therefore also receive `ConnectionTo<_>`. Raw code converts explicitly: `connection.into_protocol::()` returns a `v1::ConnectionTo` (and `into_protocol::()` a `v2::ConnectionTo`), and either versioned connection's `into_raw()` returns the version-neutral one. -`into_protocol` fails with `-32600` when the connection's builder guards a +`into_protocol` fails with `-32603` when the connection's builder guards a different version, so a connection from `Client.v1()` never becomes a `v2::ConnectionTo`. `ConnectionTo::protocol_version()` reports the guarded version, or `None` for an unguarded `Builder::new` connection. @@ -439,8 +439,8 @@ The SDK handles the `initialize` negotiation at the JSON-RPC boundary: `v2::InitializeOutcome::Initialized` for a v2 selection, or `v2::InitializeOutcome::LowerVersion` with the agent's validated response for the selected version, an `InitializationVersionMismatch` whose `response` is a -`SelectedInitializeResponse::V1`. Both types are at the crate root because -every version's negotiation reports a lower selection with them. After a lower + `SelectedInitializeResponse::V1`. Both types are at the crate root because + they are not tied to one protocol version. After a lower selection the connection stays uninitialized, and the caller can explicitly fall back to a v1 client implementation if that is acceptable. Errors returned by the agent, including `-32022`, malformed responses, and transport diff --git a/src/agent-client-protocol/src/jsonrpc.rs b/src/agent-client-protocol/src/jsonrpc.rs index fd647c09..d9e4cb83 100644 --- a/src/agent-client-protocol/src/jsonrpc.rs +++ b/src/agent-client-protocol/src/jsonrpc.rs @@ -3823,9 +3823,10 @@ impl ConnectionTo { /// /// # Errors /// - /// Returns an Invalid Request error (`-32600`) when the builder's guard - /// selected a different version than `P`, for example - /// `into_protocol::()` on a connection from `Client.v1()`. A + /// Returns an Internal error (`-32603`) when the builder's guard selected + /// a different version than `P`, for example `into_protocol::()` on a + /// connection from `Client.v1()`. The mismatch is a local programming + /// error, not a malformed message from the peer. A /// connection whose builder applies no guard (see /// [`protocol_version`](Self::protocol_version)) converts to every /// protocol, and `into_protocol::()` never fails. @@ -3841,7 +3842,7 @@ impl ConnectionTo { (self.protocol_version(), protocol::protocol_version::

()) && guard != requested { - return Err(crate::Error::invalid_request().data(format!( + return Err(crate::Error::internal_error().data(format!( "cannot use a connection guarded for ACP protocol version {guard} \ as a protocol version {requested} connection", ))); diff --git a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs index a66a3e7f..861dd479 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs @@ -183,8 +183,8 @@ pub enum InitializeOutcome { /// returns when it supports no version at or below the requested one. /// /// It lives at the crate root because it is not tied to one protocol -/// version: every version's `send_initialize` reports a lower selection with -/// it. +/// version. `v2::ConnectionTo::send_initialize` reports a lower selection +/// with it. #[cfg(feature = "unstable_protocol_v2")] #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] diff --git a/src/agent-client-protocol/tests/session_v2.rs b/src/agent-client-protocol/tests/session_v2.rs index 698b816e..5a0adda6 100644 --- a/src/agent-client-protocol/tests/session_v2.rs +++ b/src/agent-client-protocol/tests/session_v2.rs @@ -49,7 +49,7 @@ impl RunWithConnectionTo for V1SessionGuardRunner { .clone() .into_protocol::() .expect_err("a protocol v2 connection must not convert to v1"); - assert_eq!(error.code, ErrorCode::INVALID_REQUEST); + assert_eq!(error.code, ErrorCode::INTERNAL_ERROR); let data = error .data diff --git a/src/agent-client-protocol/tests/v1_connection.rs b/src/agent-client-protocol/tests/v1_connection.rs index 1ee6d330..123f3d14 100644 --- a/src/agent-client-protocol/tests/v1_connection.rs +++ b/src/agent-client-protocol/tests/v1_connection.rs @@ -56,10 +56,7 @@ async fn v1_and_raw_connections_convert_explicitly() -> Result<(), Error> { .clone() .into_protocol::() .expect_err("a v1 connection must not convert to v2"); - assert_eq!( - error.code, - agent_client_protocol::ErrorCode::INVALID_REQUEST - ); + assert_eq!(error.code, agent_client_protocol::ErrorCode::INTERNAL_ERROR); } let v1_cx: agent_client_protocol::v1::ConnectionTo = raw.into_protocol::()?; From 09df66c823f4d8cc4bfbe8e8259fde918a3a1b5b Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sat, 31 Oct 2026 17:42:50 +0100 Subject: [PATCH 09/11] test(acp): cover malformed frames before proxy/initialize on a v1+v2 router Co-Authored-By: Claude --- .../tests/proxy_protocol_router_v2.rs | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs index 77133d60..14a57de3 100644 --- a/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs +++ b/src/agent-client-protocol/tests/proxy_protocol_router_v2.rs @@ -396,3 +396,72 @@ async fn preserves_the_complete_initial_transport_frame() -> Result<(), Error> { Ok(()) } + +#[tokio::test(flavor = "current_thread")] +async fn answers_malformed_frames_before_proxy_initialize_and_keeps_reading() -> Result<(), Error> { + use tokio::io::{AsyncWriteExt as _, BufReader}; + + let (mut conductor_writer, router_reader) = tokio::io::duplex(4096); + let (router_writer, conductor_reader) = tokio::io::duplex(4096); + let conductor_transport = + ByteStreams::new(router_writer.compat_write(), router_reader.compat()); + + let (router_to_v1_writer, v1_reader) = tokio::io::duplex(4096); + let (mut v1_writer, v1_to_router_reader) = tokio::io::duplex(4096); + let v1_transport = ByteStreams::new( + router_to_v1_writer.compat_write(), + v1_to_router_reader.compat(), + ); + let v2_hits = Arc::new(AtomicUsize::new(0)); + let router = Proxy + .protocol_router() + .with_v1(v1_transport) + .with_v2(v2_proxy(Arc::clone(&v2_hits))); + let router_task = tokio::spawn(router.connect_to(conductor_transport)); + let mut conductor_reader = BufReader::new(conductor_reader); + let mut v1_reader = BufReader::new(v1_reader); + + // Invalid JSON gets its own parse error with a null id. + conductor_writer + .write_all(b"{not json\n") + .await + .map_err(Error::into_internal_error)?; + let response = read_wire_json(&mut conductor_reader).await?; + assert_eq!(response["id"], Value::Null, "{response}"); + assert_eq!(response["error"]["code"], -32700, "{response}"); + + // A batch gets one invalid request error with a null id. + let batch = serde_json::json!([{ + "jsonrpc": "2.0", + "id": 7, + "method": "proxy/initialize", + "params": initialize_params_with_extensions(ProtocolVersion::V1)?, + }]); + write_wire_json(&mut conductor_writer, &batch).await?; + let response = read_wire_json(&mut conductor_reader).await?; + assert_eq!(response["id"], Value::Null, "{response}"); + assert_eq!(response["error"]["code"], -32600, "{response}"); + + // The router keeps reading, and the selected implementation receives the + // first valid `proxy/initialize` unchanged. + let initialize = serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "proxy/initialize", + "params": initialize_params_with_extensions(ProtocolVersion::V1)?, + }); + write_wire_json(&mut conductor_writer, &initialize).await?; + assert_eq!(read_wire_json(&mut v1_reader).await?, initialize); + assert_eq!(v2_hits.load(Ordering::SeqCst), 0); + + conductor_writer + .shutdown() + .await + .map_err(Error::into_internal_error)?; + v1_writer + .shutdown() + .await + .map_err(Error::into_internal_error)?; + router_task.await.map_err(Error::into_internal_error)??; + Ok(()) +} From b1f57689613a7b0e67333bb6e9b6ea1a1f9520d7 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sun, 1 Nov 2026 15:33:16 +0100 Subject: [PATCH 10/11] refactor(acp): drop the v1 session builders' redundant v2 check ensure_v1_session_protocol rejected v1 session builders on a v2-guarded connection. A v1::ConnectionTo now only comes from a .v1() builder or from ConnectionTo::into_protocol::(), which fails for a v2-guarded connection, so the check can no longer fire. Co-Authored-By: Claude --- src/agent-client-protocol/src/session.rs | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/agent-client-protocol/src/session.rs b/src/agent-client-protocol/src/session.rs index 0e539075..74029124 100644 --- a/src/agent-client-protocol/src/session.rs +++ b/src/agent-client-protocol/src/session.rs @@ -446,8 +446,6 @@ where F: FnOnce(SessionSetup<'static, Counterpart, Request::Response>) -> Fut + Send + 'static, Fut: Future> + Send, { - ensure_v1_session_protocol(&builder.connection)?; - let RestoreSessionBuilder { connection, request, @@ -482,8 +480,6 @@ where Counterpart: HasPeer, Request: RestoreRequest, { - ensure_v1_session_protocol(&builder.connection)?; - let RestoreSessionBuilder { connection, request, @@ -742,8 +738,6 @@ where F: FnOnce(SessionSetup<'static, Counterpart, NewSessionResponse>) -> Fut + Send + 'static, Fut: Future> + Send, { - ensure_v1_session_protocol(&self.connection)?; - let Self { connection, request, @@ -842,8 +836,6 @@ where Counterpart: HasPeer, R: 'static, { - ensure_v1_session_protocol(&self.connection)?; - let Self { connection, request, @@ -951,7 +943,6 @@ where run_session_scope( run.run_with_connection_to(runner_connection), async move { - ensure_v1_session_protocol(&connection)?; let response = connection .send_request_to(Agent, request) .block_task() @@ -987,8 +978,6 @@ where where R: 'static, { - ensure_v1_session_protocol(&self.connection)?; - let Self { connection, request, @@ -1368,16 +1357,3 @@ where format!("ActiveSessionHandler({})", self.session_id) } } - -fn ensure_v1_session_protocol( - connection: &ConnectionTo, -) -> Result<(), crate::Error> { - if !connection.raw_connection().is_acp_v2() { - return Ok(()); - } - - Err(crate::Error::invalid_request().data( - "stable session builders use ACP protocol v1 types, but this is a protocol v2 connection; \ - use the `v2::ConnectionTo` supplied to `Client.v2()` callbacks", - )) -} From 78d28e50d4490c59be81c13512afe4f71a815d07 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Sun, 1 Nov 2026 21:21:37 +0100 Subject: [PATCH 11/11] feat(acp)!: rename JsonRpcResponse::from_value to from_json JsonRpcResponse paired into_json with from_value. Rename the parser to from_json so the two read as a pair, before 5.0 makes the trait's method names hard to change. BREAKING CHANGE: JsonRpcResponse::from_value is renamed to from_json. Hand-written implementations and direct calls must use the new name; derived implementations change with the derive macros. Co-Authored-By: Claude --- md/migration-v5.md | 6 +++++- src/agent-client-protocol-derive/src/lib.rs | 2 +- .../src/mcp_over_acp/protocol.rs | 8 ++++---- src/agent-client-protocol-test/src/lib.rs | 2 +- src/agent-client-protocol/src/jsonrpc.rs | 16 +++++++-------- .../src/jsonrpc/protocol_compat.rs | 20 +++++++++---------- src/agent-client-protocol/src/schema/mod.rs | 4 ++-- .../src/schema/v2_impls.rs | 4 ++-- src/agent-client-protocol/src/util/typed.rs | 2 +- .../tests/derive_macros.rs | 2 +- .../tests/jsonrpc_advanced.rs | 4 ++-- .../tests/jsonrpc_connection_builder.rs | 4 ++-- .../tests/jsonrpc_deep_chain_stack.rs | 2 +- .../tests/jsonrpc_edge_cases.rs | 2 +- .../tests/jsonrpc_error_handling.rs | 2 +- .../tests/jsonrpc_hello.rs | 2 +- .../tests/jsonrpc_request_cancellation.rs | 2 +- .../tests/jsonrpc_unhandled_messages.rs | 2 +- .../tests/match_dispatch.rs | 2 +- .../tests/protocol_v2.rs | 18 ++++++++--------- .../tests/schema_elicitation.rs | 6 +++--- .../tests/schema_llm_providers.rs | 12 +++++------ .../tests/schema_session_delete.rs | 2 +- .../tests/session_ordering.rs | 8 ++++---- 24 files changed, 69 insertions(+), 65 deletions(-) diff --git a/md/migration-v5.md b/md/migration-v5.md index 5585d2b2..9a09443f 100644 --- a/md/migration-v5.md +++ b/md/migration-v5.md @@ -838,7 +838,7 @@ version-neutral `ConnectionTo` with `.into()`: `v1::ConnectionTo` and requires `Into>`. Their own methods, such as the v1 session helpers, are only available on the concrete types. -## `Channel` and `JsonRpcMessage::parse_message` +## `Channel`, `JsonRpcMessage::parse_message` and `JsonRpcResponse::from_json` - `Channel` is `#[non_exhaustive]`. Build one with `Channel::new(rx, tx)` (or `Channel::duplex()`) and take it apart with `into_parts()` or a pattern @@ -859,6 +859,10 @@ session helpers, are only available on the concrete types. change with the derive macros. Callers holding raw `Option` pass `¶ms.map_or(serde_json::Value::Null, RawJsonRpcParams::into_value)`. +- `JsonRpcResponse::from_value` is renamed to `from_json`, to pair with + `into_json`. Rename it in hand-written implementations and calls such as + `PromptResponse::from_value(method, value)`; derived implementations change + with the derive macros. ## Schema enums and SDK helpers diff --git a/src/agent-client-protocol-derive/src/lib.rs b/src/agent-client-protocol-derive/src/lib.rs index 1435df21..8198edd9 100644 --- a/src/agent-client-protocol-derive/src/lib.rs +++ b/src/agent-client-protocol-derive/src/lib.rs @@ -215,7 +215,7 @@ pub fn derive_json_rpc_response_payload(input: TokenStream) -> TokenStream { #krate::__private::serde_json::to_value(self).map_err(#krate::Error::into_internal_error) } - fn from_value(#method_arg: &str, #value_arg: #krate::__private::serde_json::Value) -> ::core::result::Result { + fn from_json(#method_arg: &str, #value_arg: #krate::__private::serde_json::Value) -> ::core::result::Result { #krate::__private::json_cast(#value_arg) } } diff --git a/src/agent-client-protocol-polyfill/src/mcp_over_acp/protocol.rs b/src/agent-client-protocol-polyfill/src/mcp_over_acp/protocol.rs index a10d0a53..3d203250 100644 --- a/src/agent-client-protocol-polyfill/src/mcp_over_acp/protocol.rs +++ b/src/agent-client-protocol-polyfill/src/mcp_over_acp/protocol.rs @@ -28,7 +28,7 @@ impl PolyfillProtocol { /// Validate against the negotiated ACP version before projecting onto HTTP. pub(super) fn message_response(self, value: Value) -> Result { match self { - Self::V1 => match v1::MessageMcpResponse::from_value("mcp/message", value)? { + Self::V1 => match v1::MessageMcpResponse::from_json("mcp/message", value)? { v1::MessageMcpResponse::Result { result, .. } => { Ok(NativeMcpOutcome::Result(result)) } @@ -38,7 +38,7 @@ impl PolyfillProtocol { _ => Err(Error::invalid_request().data("unsupported MCP outcome")), }, #[cfg(feature = "unstable_protocol_v2")] - Self::V2 => match v2::MessageMcpResponse::from_value("mcp/message", value)? { + Self::V2 => match v2::MessageMcpResponse::from_json("mcp/message", value)? { v2::MessageMcpResponse::Result { result, .. } => { Ok(NativeMcpOutcome::Result(result)) } @@ -100,7 +100,7 @@ impl PolyfillProtocol { ) -> Result { let mode = match self { Self::V1 => { - let parsed = agent_client_protocol::schema::v1::InitializeResponse::from_value( + let parsed = agent_client_protocol::schema::v1::InitializeResponse::from_json( "initialize", response.clone(), )?; @@ -111,7 +111,7 @@ impl PolyfillProtocol { } #[cfg(feature = "unstable_protocol_v2")] Self::V2 => { - let parsed = v2::InitializeResponse::from_value("initialize", response.clone())?; + let parsed = v2::InitializeResponse::from_json("initialize", response.clone())?; let mcp = parsed .capabilities .session diff --git a/src/agent-client-protocol-test/src/lib.rs b/src/agent-client-protocol-test/src/lib.rs index 059df304..379e9524 100644 --- a/src/agent-client-protocol-test/src/lib.rs +++ b/src/agent-client-protocol-test/src/lib.rs @@ -186,7 +186,7 @@ macro_rules! impl_jr_response_payload { fn into_json(self, _method: &str) -> Result { Ok(serde_json::to_value(self)?) } - fn from_value(_method: &str, value: serde_json::Value) -> Result { + fn from_json(_method: &str, value: serde_json::Value) -> Result { Ok(serde_json::from_value(value)?) } } diff --git a/src/agent-client-protocol/src/jsonrpc.rs b/src/agent-client-protocol/src/jsonrpc.rs index d9e4cb83..403f9a93 100644 --- a/src/agent-client-protocol/src/jsonrpc.rs +++ b/src/agent-client-protocol/src/jsonrpc.rs @@ -4069,7 +4069,7 @@ impl ConnectionTo { Counterpart: HasPeer, { let hook: ResponseRouteHook = Box::new(move |method, value| { - let response = Req::Response::from_value(method, value.clone())?; + let response = Req::Response::from_json(method, value.clone())?; response_hook(&response) }); self.send_request_to_with_options( @@ -4099,7 +4099,7 @@ impl ConnectionTo { Counterpart: HasPeer, { let hook: ResponseRouteHook = Box::new(move |method, value| { - let response = Req::Response::from_value(method, value.clone())?; + let response = Req::Response::from_json(method, value.clone())?; response_hook(&response) }); self.send_request_to_with_options( @@ -4226,7 +4226,7 @@ impl ConnectionTo { cancellation, response_ordering, ) - .map(move |json| ::from_value(&method, json)); + .map(move |json| ::from_json(&method, json)); PreparedRequest { sent, publication: RequestPublication { @@ -4772,7 +4772,7 @@ impl Responder { /// and which checks (dynamically) that the JSON value it receives /// can be converted to `T`. pub fn erase_to_json(self) -> Responder { - self.wrap_params(|method, value| T::from_value(method, value?)) + self.wrap_params(|method, value| T::from_json(method, value?)) } /// Return a new Responder with a different method name. @@ -5044,7 +5044,7 @@ impl ResponseRouter { /// and which checks (dynamically) that the JSON value it receives /// can be converted to `T`. pub fn erase_to_json(self) -> ResponseRouter { - self.wrap_params(|method, value| T::from_value(method, value?)) + self.wrap_params(|method, value| T::from_json(method, value?)) } /// The same router for protocol `Q`. Fails only if the request ID is not @@ -5171,11 +5171,11 @@ pub trait JsonRpcResponse: 'static + Debug + Sized + Send + Clone { fn into_json(self, method: &str) -> Result; /// Parse a JSON value into the response type. - fn from_value(method: &str, value: serde_json::Value) -> Result; + fn from_json(method: &str, value: serde_json::Value) -> Result; } impl JsonRpcResponse for serde_json::Value { - fn from_value(_method: &str, value: serde_json::Value) -> Result { + fn from_json(_method: &str, value: serde_json::Value) -> Result { Ok(value) } @@ -5473,7 +5473,7 @@ impl Dispatch { // Parse the response result let typed_result = match result { Ok(value) => { - match ::from_value(method, value) { + match ::from_json(method, value) { Ok(parsed) => { tracing::trace!("parse ok"); Ok(parsed) diff --git a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs index 861dd479..b959103c 100644 --- a/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs +++ b/src/agent-client-protocol/src/jsonrpc/protocol_compat.rs @@ -260,12 +260,12 @@ impl SelectedInitializeResponse { /// Parse `value` as the initialization response for `version`, or return /// `None` if the SDK has no response type for that version. - fn from_value( + fn from_json( version: crate::schema::ProtocolVersion, value: serde_json::Value, ) -> Option> { (version == crate::schema::ProtocolVersion::V1).then(|| { - ::from_value( + ::from_json( "initialize", value, ) @@ -285,7 +285,7 @@ impl crate::JsonRpcResponse for InitializeOutcome { } } - fn from_value(method: &str, value: serde_json::Value) -> Result { + fn from_json(method: &str, value: serde_json::Value) -> Result { let selected: crate::schema::ProtocolVersion = value .get("protocolVersion") .cloned() @@ -299,10 +299,10 @@ impl crate::JsonRpcResponse for InitializeOutcome { })?; let requested = crate::schema::ProtocolVersion::V2; if selected == requested { - return crate::schema::v2::InitializeResponse::from_value(method, value) + return crate::schema::v2::InitializeResponse::from_json(method, value) .map(Self::Initialized); } - match SelectedInitializeResponse::from_value(selected, value) { + match SelectedInitializeResponse::from_json(selected, value) { Some(response) if selected < requested => Ok(Self::LowerVersion( InitializationVersionMismatch::new(requested, response?), )), @@ -893,7 +893,7 @@ mod imp { if accepts_lower_version && mode.initialization_role == InitializationRole::Initiator && wire_version < mode.api - && let Some(response) = super::SelectedInitializeResponse::from_value( + && let Some(response) = super::SelectedInitializeResponse::from_json( wire_version.as_protocol_version(), value.clone(), ) @@ -1270,7 +1270,7 @@ mod imp { ) -> Result<(), crate::Error> { #[cfg(feature = "unstable_protocol_v2")] if mode.initialization_role != InitializationRole::Unchecked { - ::from_value( + ::from_json( "initialize", value.clone(), )?; @@ -1615,7 +1615,7 @@ mod imp { let v2_response = v2_initialize_response(ProtocolVersion::V2); assert_eq!( - super::super::InitializeOutcome::from_value( + super::super::InitializeOutcome::from_json( "initialize", serde_json::to_value(&v2_response)? )?, @@ -1623,7 +1623,7 @@ mod imp { ); let v1_response = v1_initialize_response(); - let outcome = super::super::InitializeOutcome::from_value( + let outcome = super::super::InitializeOutcome::from_json( "initialize", serde_json::to_value(&v1_response)?, )?; @@ -1646,7 +1646,7 @@ mod imp { serde_json::json!({}), serde_json::json!({ "protocolVersion": 3 }), ] { - super::super::InitializeOutcome::from_value("initialize", value) + super::super::InitializeOutcome::from_json("initialize", value) .expect_err("only v2 and lower known versions are outcomes"); } Ok(()) diff --git a/src/agent-client-protocol/src/schema/mod.rs b/src/agent-client-protocol/src/schema/mod.rs index eff63657..3de53c5d 100644 --- a/src/agent-client-protocol/src/schema/mod.rs +++ b/src/agent-client-protocol/src/schema/mod.rs @@ -49,7 +49,7 @@ macro_rules! impl_jsonrpc_request { serde_json::to_value(self).map_err($crate::Error::into_internal_error) } - fn from_value(_method: &str, value: serde_json::Value) -> Result { + fn from_json(_method: &str, value: serde_json::Value) -> Result { $crate::util::json_cast(&value) } } @@ -284,7 +284,7 @@ macro_rules! impl_jsonrpc_response_enum { serde_json::to_value(self).map_err($crate::Error::into_internal_error) } - fn from_value( + fn from_json( method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/src/schema/v2_impls.rs b/src/agent-client-protocol/src/schema/v2_impls.rs index 5a5ab05b..665bf570 100644 --- a/src/agent-client-protocol/src/schema/v2_impls.rs +++ b/src/agent-client-protocol/src/schema/v2_impls.rs @@ -38,7 +38,7 @@ macro_rules! impl_v2_jsonrpc_request { serde_json::to_value(self).map_err(crate::Error::into_internal_error) } - fn from_value(_method: &str, value: serde_json::Value) -> Result { + fn from_json(_method: &str, value: serde_json::Value) -> Result { crate::util::json_cast(value) } } @@ -188,7 +188,7 @@ macro_rules! impl_v2_jsonrpc_response_enum { serde_json::to_value(self).map_err(crate::Error::into_internal_error) } - fn from_value( + fn from_json( method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/src/util/typed.rs b/src/agent-client-protocol/src/util/typed.rs index 4072f39e..cbbbdf8f 100644 --- a/src/agent-client-protocol/src/util/typed.rs +++ b/src/agent-client-protocol/src/util/typed.rs @@ -312,7 +312,7 @@ impl MatchDispatch { // Method matches, parse the response let typed_router: ResponseRouter = router.cast(); let typed_result = match result { - Ok(value) => Req::Response::from_value(typed_router.method(), value), + Ok(value) => Req::Response::from_json(typed_router.method(), value), Err(err) => Err(err), }; diff --git a/src/agent-client-protocol/tests/derive_macros.rs b/src/agent-client-protocol/tests/derive_macros.rs index eecd1b02..c266f5b7 100644 --- a/src/agent-client-protocol/tests/derive_macros.rs +++ b/src/agent-client-protocol/tests/derive_macros.rs @@ -156,7 +156,7 @@ fn test_jr_response_payload_from_value() { let json = serde_json::json!({ "greeting": "Hi there!" }); - let response = HelloResponse::from_value("_test/hello", json).unwrap(); + let response = HelloResponse::from_json("_test/hello", json).unwrap(); assert_eq!(response.greeting, "Hi there!"); } diff --git a/src/agent-client-protocol/tests/jsonrpc_advanced.rs b/src/agent-client-protocol/tests/jsonrpc_advanced.rs index 04d688a0..a36467c9 100644 --- a/src/agent-client-protocol/tests/jsonrpc_advanced.rs +++ b/src/agent-client-protocol/tests/jsonrpc_advanced.rs @@ -100,7 +100,7 @@ impl JsonRpcResponse for PongResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { @@ -173,7 +173,7 @@ impl JsonRpcResponse for SlowResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs b/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs index 8d09ef1d..1328df7d 100644 --- a/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs +++ b/src/agent-client-protocol/tests/jsonrpc_connection_builder.rs @@ -78,7 +78,7 @@ impl JsonRpcResponse for FooResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { @@ -131,7 +131,7 @@ impl JsonRpcResponse for BarResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs b/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs index c27f8540..253bb5ec 100644 --- a/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs +++ b/src/agent-client-protocol/tests/jsonrpc_deep_chain_stack.rs @@ -95,7 +95,7 @@ impl JsonRpcResponse for PongResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs b/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs index 60541b9c..f0693d9f 100644 --- a/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs +++ b/src/agent-client-protocol/tests/jsonrpc_edge_cases.rs @@ -201,7 +201,7 @@ impl JsonRpcResponse for SimpleResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_error_handling.rs b/src/agent-client-protocol/tests/jsonrpc_error_handling.rs index 5a81c87c..cb97b440 100644 --- a/src/agent-client-protocol/tests/jsonrpc_error_handling.rs +++ b/src/agent-client-protocol/tests/jsonrpc_error_handling.rs @@ -221,7 +221,7 @@ impl JsonRpcResponse for SimpleResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_hello.rs b/src/agent-client-protocol/tests/jsonrpc_hello.rs index 47212bce..f8c2ff31 100644 --- a/src/agent-client-protocol/tests/jsonrpc_hello.rs +++ b/src/agent-client-protocol/tests/jsonrpc_hello.rs @@ -93,7 +93,7 @@ impl JsonRpcResponse for PongResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs b/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs index 6a02f22c..b7fbead7 100644 --- a/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs +++ b/src/agent-client-protocol/tests/jsonrpc_request_cancellation.rs @@ -127,7 +127,7 @@ impl JsonRpcResponse for SimpleResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs b/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs index 1eef1258..c841fe27 100644 --- a/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs +++ b/src/agent-client-protocol/tests/jsonrpc_unhandled_messages.rs @@ -102,7 +102,7 @@ impl JsonRpcResponse for SimpleResponse { serde_json::to_value(self).map_err(agent_client_protocol::Error::into_internal_error) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/match_dispatch.rs b/src/agent-client-protocol/tests/match_dispatch.rs index 8a6aaca7..b0ac8d22 100644 --- a/src/agent-client-protocol/tests/match_dispatch.rs +++ b/src/agent-client-protocol/tests/match_dispatch.rs @@ -47,7 +47,7 @@ impl JsonRpcResponse for EchoRequestResponse { agent_client_protocol::util::json_cast(self) } - fn from_value( + fn from_json( _method: &str, value: serde_json::Value, ) -> Result { diff --git a/src/agent-client-protocol/tests/protocol_v2.rs b/src/agent-client-protocol/tests/protocol_v2.rs index b335ed5b..5e2c16c6 100644 --- a/src/agent-client-protocol/tests/protocol_v2.rs +++ b/src/agent-client-protocol/tests/protocol_v2.rs @@ -436,7 +436,7 @@ impl ConnectTo for FutureInitializeV2Client { else { continue; }; - let initialize = v2::InitializeResponse::from_value("initialize", result)?; + let initialize = v2::InitializeResponse::from_json("initialize", result)?; assert_eq!(initialize.protocol_version, ProtocolVersion::V2); if let Some(task) = agent_task { task.abort(); @@ -731,8 +731,8 @@ where assert_eq!(request.to_untyped_message()?.method(), method); assert!(request_variant(request)); - ::from_value(method, response.clone())?; - assert!(response_variant(v2::AgentResponse::from_value( + ::from_json(method, response.clone())?; + assert!(response_variant(v2::AgentResponse::from_json( method, response )?)); Ok(()) @@ -758,8 +758,8 @@ where assert_eq!(request.to_untyped_message()?.method(), method); assert!(request_variant(request)); - ::from_value(method, response.clone())?; - assert!(response_variant(v2::ClientResponse::from_value( + ::from_json(method, response.clone())?; + assert!(response_variant(v2::ClientResponse::from_json( method, response )?)); Ok(()) @@ -1047,7 +1047,7 @@ fn mcp_over_acp_v1_variants_are_jsonrpc_mapped() -> Result<(), Error> { macro_rules! assert_response_mapping { ($ty:ty, $method:literal, $value:expr, $pattern:pat) => {{ - let response = <$ty as JsonRpcResponse>::from_value($method, $value)?; + let response = <$ty as JsonRpcResponse>::from_json($method, $value)?; assert!(matches!(response, $pattern)); }}; } @@ -1108,7 +1108,7 @@ fn mcp_carriers_preserve_inner_outcomes_in_both_protocol_versions() -> Result<() let carrier = $schema::MessageMcpResponse::success(result.clone()).meta(meta.clone()); let value = json_value(carrier)?; assert_eq!(value, serde_json::json!({"result": result, "_meta": meta})); - let response = $schema::ClientResponse::from_value("mcp/message", value.clone())?; + let response = $schema::ClientResponse::from_json("mcp/message", value.clone())?; let $schema::ClientResponse::MessageMcpResponse(carrier) = response else { panic!("expected MCP response"); }; @@ -1132,7 +1132,7 @@ fn mcp_carriers_preserve_inner_outcomes_in_both_protocol_versions() -> Result<() let value = json_value(carrier)?; assert_eq!(value, serde_json::json!({"error": inner_error, "_meta": meta})); // This is a successful *outer* ACP response, not an ACP Error. - let response = $schema::ClientResponse::from_value("mcp/message", value)?; + let response = $schema::ClientResponse::from_json("mcp/message", value)?; let $schema::ClientResponse::MessageMcpResponse(carrier) = response else { panic!("expected MCP response"); }; @@ -2777,7 +2777,7 @@ async fn protocol_router_routes_future_protocol_version_to_v2() -> Result<(), Er let RawJsonRpcMessage::Response(RawJsonRpcResponse::Result { result, .. }) = message else { continue; }; - let initialize = v2::InitializeResponse::from_value("initialize", result)?; + let initialize = v2::InitializeResponse::from_json("initialize", result)?; assert_eq!(initialize.protocol_version, ProtocolVersion::V2); agent_task.abort(); return Ok(()); diff --git a/src/agent-client-protocol/tests/schema_elicitation.rs b/src/agent-client-protocol/tests/schema_elicitation.rs index 3ac58d62..90d911c8 100644 --- a/src/agent-client-protocol/tests/schema_elicitation.rs +++ b/src/agent-client-protocol/tests/schema_elicitation.rs @@ -72,11 +72,11 @@ fn create_elicitation_response_round_trips_json() { .unwrap(); assert_eq!(value, json!({ "action": "decline" })); - let parsed = CreateElicitationResponse::from_value("elicitation/create", value).unwrap(); + let parsed = CreateElicitationResponse::from_json("elicitation/create", value).unwrap(); assert!(matches!(parsed.action, ElicitationAction::Decline)); let enum_response = - ClientResponse::from_value("elicitation/create", json!({ "action": "cancel" })).unwrap(); + ClientResponse::from_json("elicitation/create", json!({ "action": "cancel" })).unwrap(); assert!(matches!( enum_response, ClientResponse::CreateElicitationResponse(_) @@ -143,7 +143,7 @@ fn protocol_v2_elicitation_variants_are_jsonrpc_mapped() -> Result<(), Error> { )); let parsed_response = - v2::ClientResponse::from_value("elicitation/create", json!({ "action": "decline" }))?; + v2::ClientResponse::from_json("elicitation/create", json!({ "action": "decline" }))?; assert!(matches!( parsed_response, v2::ClientResponse::CreateElicitationResponse(_) diff --git a/src/agent-client-protocol/tests/schema_llm_providers.rs b/src/agent-client-protocol/tests/schema_llm_providers.rs index 30c64357..63b735f7 100644 --- a/src/agent-client-protocol/tests/schema_llm_providers.rs +++ b/src/agent-client-protocol/tests/schema_llm_providers.rs @@ -81,15 +81,15 @@ fn provider_requests_have_typed_v1_jsonrpc_routes() { )); assert!(matches!( - AgentResponse::from_value("providers/list", json!({ "providers": [] })).unwrap(), + AgentResponse::from_json("providers/list", json!({ "providers": [] })).unwrap(), AgentResponse::ListProvidersResponse(_) )); assert!(matches!( - AgentResponse::from_value("providers/set", json!({})).unwrap(), + AgentResponse::from_json("providers/set", json!({})).unwrap(), AgentResponse::SetProviderResponse(_) )); assert!(matches!( - AgentResponse::from_value("providers/disable", json!({})).unwrap(), + AgentResponse::from_json("providers/disable", json!({})).unwrap(), AgentResponse::DisableProviderResponse(_) )); } @@ -218,15 +218,15 @@ fn provider_requests_have_typed_draft_v2_aggregate_routes() -> Result<(), Error> )); assert!(matches!( - v2::AgentResponse::from_value("providers/list", json!({ "providers": [] }))?, + v2::AgentResponse::from_json("providers/list", json!({ "providers": [] }))?, v2::AgentResponse::ListProvidersResponse(_) )); assert!(matches!( - v2::AgentResponse::from_value("providers/set", json!({}))?, + v2::AgentResponse::from_json("providers/set", json!({}))?, v2::AgentResponse::SetProviderResponse(_) )); assert!(matches!( - v2::AgentResponse::from_value("providers/disable", json!({}))?, + v2::AgentResponse::from_json("providers/disable", json!({}))?, v2::AgentResponse::DisableProviderResponse(_) )); Ok(()) diff --git a/src/agent-client-protocol/tests/schema_session_delete.rs b/src/agent-client-protocol/tests/schema_session_delete.rs index 1aa50529..20ad290f 100644 --- a/src/agent-client-protocol/tests/schema_session_delete.rs +++ b/src/agent-client-protocol/tests/schema_session_delete.rs @@ -46,7 +46,7 @@ fn delete_session_response_round_trips_json() { .unwrap(); assert_eq!(value, json!({})); - let parsed = DeleteSessionResponse::from_value("session/delete", value).unwrap(); + let parsed = DeleteSessionResponse::from_json("session/delete", value).unwrap(); assert_eq!(parsed, DeleteSessionResponse::new()); assert_request_response_pair::(); diff --git a/src/agent-client-protocol/tests/session_ordering.rs b/src/agent-client-protocol/tests/session_ordering.rs index 114ec9e6..831eed3e 100644 --- a/src/agent-client-protocol/tests/session_ordering.rs +++ b/src/agent-client-protocol/tests/session_ordering.rs @@ -59,7 +59,7 @@ async fn initialize_raw_v2_proxy( panic!("expected the proxy initialize response"); }; assert_eq!(id, initialize_id); - let response = v2::InitializeResponse::from_value("proxy/initialize", result)?; + let response = v2::InitializeResponse::from_json("proxy/initialize", result)?; assert_eq!(response.protocol_version, ProtocolVersion::V2); Ok(()) } @@ -680,7 +680,7 @@ async fn v2_proxy_session_start_installs_routing_before_later_message() { agent_client_protocol::RawJsonRpcResponse::Result { id, result }, ) => { assert_eq!(id, upstream_id); - let response = v2::NewSessionResponse::from_value("session/new", result)?; + let response = v2::NewSessionResponse::from_json("session/new", result)?; assert_eq!(response.session_id, session_id); saw_response = true; } @@ -830,7 +830,7 @@ async fn v2_proxy_fork_installs_response_id_routing_before_later_message() { agent_client_protocol::RawJsonRpcResponse::Result { id, result }, ) => { assert_eq!(id, upstream_id); - let response = v2::ForkSessionResponse::from_value("session/fork", result)?; + let response = v2::ForkSessionResponse::from_json("session/fork", result)?; assert_eq!(response.session_id, forked_session_id); assert_eq!(response.config_options.len(), 1); saw_response = true; @@ -988,7 +988,7 @@ async fn v2_proxy_resume_forwards_replay_before_its_response() { panic!("expected the resume response after replay"); }; assert_eq!(id, upstream_id); - let response = v2::ResumeSessionResponse::from_value("session/resume", result)?; + let response = v2::ResumeSessionResponse::from_json("session/resume", result)?; assert_eq!(response, upstream_response); peer_done_tx