diff --git a/md/migration_v2.0.md b/md/migration_v2.0.md index 2cc52f0..d9b90c6 100644 --- a/md/migration_v2.0.md +++ b/md/migration_v2.0.md @@ -350,14 +350,9 @@ are included in the SDK 2.0 migration rather than treated as stable-v1 wire chan - Terminal state is represented by `Terminal`, `TerminalUpdate`, `TerminalOutput`, `TerminalOutputChunk`, and `TerminalExitStatus`. `SessionUpdate` also has terminal update and output-chunk variants, so exhaustive matches must handle the new variants. -- Conversion helpers are now generic and fallible. Replace `v2_to_v1(value)` and - `v1_to_v2(value)` with `try_v2_to_v1::<_, Target>(value)` and - `try_v1_to_v2::<_, Target>(value)`. Use `try_v2_to_v1_many::<_, Target>(value)` when one v2 - update may become several v1 updates. -- The bespoke `IntoV1`, `IntoV1Many`, and `IntoV2` conversion traits have been removed. Use the - standard `TryFrom`/`TryInto` traits for fallible conversions, `From`/`Into` for infallible - conversions, or the helper functions above. -- `v2::SessionCapabilities::into_v1()` is now `try_into_v1_parts()`. +- The experimental `v2::conversion` module and its cross-version helpers have been removed. + Implement v1 and v2 handlers separately, and translate only application-owned shared state + where the application's semantics define a faithful mapping. ## `SentRequest::map` accepts arbitrary output diff --git a/md/protocol-v2.md b/md/protocol-v2.md index 28e8545..66486c0 100644 --- a/md/protocol-v2.md +++ b/md/protocol-v2.md @@ -312,8 +312,9 @@ Other declarations and unrelated request fields remain unchanged. See [MCP-over-ACP Compatibility Bridge](./mcp-bridge.md) for placement and feature configuration. -This feature extends the concrete compatibility proxy only. Global MCP -attachment and proxy-session helpers in the core SDK remain v1-only. +This feature extends the concrete compatibility proxy only. The core SDK's +global MCP attachment and proxy-session helpers support v1 and v2 independently, +as described above. The SDK handles the `initialize` negotiation at the JSON-RPC boundary: @@ -383,9 +384,10 @@ The protocol router reads the initial `initialize` request, selects the highest configured protocol version that is compatible with the requested version, and then hands the connection to that implementation. If only v2 is configured, v1 clients are rejected without changing the fluent API. The router -does not convert messages between v1 and v2 after routing. For compatibility, -the initial frame may be a batch whose first call-shaped entry is `initialize`; -the router preserves the complete frame when handing it to the selected +normalizes a v2 initialize request when selecting a v1 implementation, but does +not convert messages between v1 and v2 after routing. For compatibility, the +initial frame may be a batch whose first call-shaped entry is `initialize`; the +router preserves the complete frame when handing it to the selected implementation. Response-only frames before initialization are ignored. Clients use a connector because fallback may require opening a new transport. @@ -417,6 +419,10 @@ agent: - If the agent rejects the v2 initialize request, the error is surfaced. A rejected initialize is not treated as permission to retry with v1. +The reuse probe is conservative: if parsing and serializing the raw v2 request +would change any parameter, reuse is disabled and fallback opens a fresh +connection. That does not turn an otherwise valid v2 request into an error. + ## Draft schema changes in schema 1.5 and 1.6 The `unstable_protocol_v2` API follows the moving draft schema. Schema 1.5 added diff --git a/src/agent-client-protocol/src/role/acp.rs b/src/agent-client-protocol/src/role/acp.rs index 2466c0c..c603a27 100644 --- a/src/agent-client-protocol/src/role/acp.rs +++ b/src/agent-client-protocol/src/role/acp.rs @@ -195,8 +195,10 @@ impl ClientProtocolConnector { let agent_connection = RunningProtocolPeer::new(agent()); let (client, initialize) = start_client_protocol(ClientProtocol::V2, client).await?; - let v2_initialize_as_v1 = - normalize_initialize_params_for_agent_protocol(&initialize, AgentProtocol::V1)?; + // This normalization is only a probe for the connection-reuse + // optimization. A request that cannot be represented in v1 + // can still be valid v2 traffic and must reach the agent. + let v2_initialize_as_v1 = normalize_v2_initialize_params_for_reuse(&initialize); let (client, agent_connection, initialize_response) = send_initialize_and_receive(client, agent_connection, initialize).await?; @@ -206,12 +208,13 @@ impl ClientProtocolConnector { let fallback_client = v1.create(); let (fallback_client, fallback_initialize) = start_client_protocol(ClientProtocol::V1, fallback_client).await?; - let v1_initialize = normalize_initialize_params_for_agent_protocol( - &fallback_initialize, - AgentProtocol::V1, - )?; + let v1_initialize = + validated_initialize_params::(&fallback_initialize)?; - if v1_initialize == v2_initialize_as_v1 { + if v2_initialize_as_v1 + .as_ref() + .is_ok_and(|v2_initialize| v2_initialize == &v1_initialize) + { let fallback_response = initialize_response.with_id( initialize_request_id(&fallback_initialize) .expect("validated initialize request has an id"), @@ -538,10 +541,9 @@ fn select_agent_protocol( } #[cfg(feature = "unstable_protocol_v2")] -fn normalize_initialize_params_for_agent_protocol( +fn initialize_request_params( message: &RawJsonRpcMessage, - selected: AgentProtocol, -) -> Result, crate::Error> { +) -> Result<&serde_json::Map, crate::Error> { let RawJsonRpcMessage::Request(request) = message else { return Err( crate::Error::invalid_request().data("first ACP message must be an initialize request") @@ -555,17 +557,41 @@ fn normalize_initialize_params_for_agent_protocol( let Some(RawJsonRpcParams::Object(params)) = &request.params else { return Err(invalid_initialize_protocol_version()); }; - let Some(protocol_version) = params.get("protocolVersion") else { + if !params.contains_key("protocolVersion") { return Err(invalid_initialize_protocol_version()); - }; - - let requested = serde_json::from_value::(protocol_version.clone()) - .map_err(|_| invalid_initialize_protocol_version())?; - let mut params = params.clone(); - rewrite_initialize_params(&mut params, requested, selected)?; + } Ok(params) } +#[cfg(feature = "unstable_protocol_v2")] +fn validated_initialize_params( + message: &RawJsonRpcMessage, +) -> Result, crate::Error> { + let params = initialize_request_params(message)?; + parse_initialize_params::(params)?; + Ok(params.clone()) +} + +#[cfg(feature = "unstable_protocol_v2")] +fn normalize_v2_initialize_params_for_reuse( + message: &RawJsonRpcMessage, +) -> Result, crate::Error> { + let params = initialize_request_params(message)?; + let requested = params + .get("protocolVersion") + .cloned() + .ok_or_else(invalid_initialize_protocol_version) + .and_then(|version| { + serde_json::from_value::(version) + .map_err(|_| invalid_initialize_protocol_version()) + })?; + if requested == ProtocolVersion::V1 { + parse_initialize_params::(params)?; + return Ok(params.clone()); + } + normalize_v2_initialize_params_for_v1(params, true) +} + #[cfg(feature = "unstable_protocol_v2")] fn rewrite_initialize_params( params: &mut serde_json::Map, @@ -589,27 +615,200 @@ fn rewrite_initialize_params( match selected { AgentProtocol::V1 => { - let mut initialize = if requested >= ProtocolVersion::V2 { - v2::conversion::try_v2_to_v1(parse_initialize_params::( - params, - )?) - .map_err(invalid_initialize_params)? - } else { - parse_initialize_params::(params)? - }; - initialize.protocol_version = ProtocolVersion::V1; - replace_initialize_params(params, initialize) + debug_assert!(requested >= ProtocolVersion::V2); + *params = normalize_v2_initialize_params_for_v1(params, false)?; + Ok(()) } AgentProtocol::V2 => { - let mut initialize = if requested >= ProtocolVersion::V2 { - parse_initialize_params::(params)? - } else { - v2::conversion::try_v1_to_v2(parse_initialize_params::(params)?) - .map_err(invalid_initialize_params)? - }; + let mut initialize = parse_initialize_params::(params)?; initialize.protocol_version = ProtocolVersion::V2; - replace_initialize_params(params, initialize) + *params = serialize_initialize_params(initialize)?; + Ok(()) + } + } +} + +#[cfg(feature = "unstable_protocol_v2")] +fn normalize_v2_initialize_params_for_v1( + params: &serde_json::Map, + require_lossless: bool, +) -> Result, crate::Error> { + // Canonicalize through v2 first so tolerant field semantics are applied. + // A lossless result is only required by the connection-reuse probe. Normal + // v1 routing may discard fields that have no meaning in the selected + // protocol version. + let initialize = parse_initialize_params::(params)?; + let mut target = serialize_initialize_params(initialize)?; + if require_lossless && target != *params { + return Err(invalid_initialize_params( + "v2 initialize parameters are not losslessly representable in v1", + )); + } + + target.insert( + "protocolVersion".into(), + serde_json::to_value(ProtocolVersion::V1).map_err(crate::Error::into_internal_error)?, + ); + let info = target + .remove("info") + .ok_or_else(|| invalid_initialize_params("v2 InitializeRequest.info is required"))?; + target.insert("clientInfo".into(), info); + let capabilities = target + .remove("capabilities") + .and_then(|capabilities| capabilities.as_object().cloned()) + .ok_or_else(|| { + crate::util::internal_error("v2 initialize capabilities did not serialize as an object") + })?; + let mut capabilities = capabilities; + if let Some(auth) = capabilities + .get_mut("auth") + .and_then(serde_json::Value::as_object_mut) + { + let terminal = auth.remove("terminal"); + if require_lossless + && terminal + .as_ref() + .and_then(serde_json::Value::as_object) + .is_some_and(|terminal| terminal.contains_key("_meta")) + { + return Err(invalid_initialize_params( + "v2 terminal authentication metadata is not representable in v1", + )); } + auth.insert("terminal".into(), terminal.is_some().into()); + } + capabilities.insert( + "session".into(), + serde_json::json!({ "configOptions": { "boolean": {} } }), + ); + target.insert("clientCapabilities".into(), capabilities.into()); + + let initialize = parse_initialize_params::(&target)?; + let normalized = serialize_initialize_params(initialize)?; + if require_lossless && !json_object_contains(&normalized, &target) { + return Err(invalid_initialize_params( + "v2 initialize parameters are not losslessly representable in v1", + )); + } + Ok(normalized) +} + +#[cfg(all(test, feature = "unstable_protocol_v2"))] +mod initialize_normalization_tests { + use super::*; + + fn v2_initialize_params() -> serde_json::Map { + let value = serde_json::to_value(v2::InitializeRequest::new( + ProtocolVersion::V2, + v2::Implementation::new("test-client", "1.0.0"), + )) + .expect("serialize v2 initialize request"); + value + .as_object() + .expect("initialize params serialize as an object") + .clone() + } + + #[test] + fn v2_tolerant_fields_are_canonicalized_before_v1_normalization() { + let mut params = v2_initialize_params(); + params.insert( + "capabilities".into(), + serde_json::Value::String("malformed".into()), + ); + params.insert( + "_meta".into(), + serde_json::Value::String("malformed".into()), + ); + + let normalized = normalize_v2_initialize_params_for_v1(¶ms, false) + .expect("tolerant v2 fields should normalize through their defaults"); + let normalized = serde_json::Value::Object(normalized); + + assert!(normalized.get("_meta").is_none()); + assert_eq!( + normalized.pointer("/clientCapabilities/session/configOptions/boolean"), + Some(&serde_json::json!({})) + ); + } + + #[test] + fn noncanonical_v2_fields_disable_reuse_but_not_v1_routing() { + let mut params = v2_initialize_params(); + params + .get_mut("info") + .and_then(serde_json::Value::as_object_mut) + .expect("v2 initialize info is an object") + .insert("buildCommit".into(), serde_json::json!("abc123")); + + normalize_v2_initialize_params_for_v1(¶ms, false) + .expect("v1 routing may ignore parameters unavailable in v1"); + normalize_v2_initialize_params_for_v1(¶ms, true) + .expect_err("connection reuse requires lossless normalization"); + } + + #[test] + fn v1_reuse_probe_preserves_raw_initialize_params() { + let mut params = v2_initialize_params(); + params.insert( + "protocolVersion".into(), + serde_json::json!(ProtocolVersion::V1), + ); + let message = RawJsonRpcMessage::request( + "initialize".into(), + serde_json::Value::Object(params.clone()), + RequestId::Number(1), + ) + .expect("build initialize request"); + + let normalized = normalize_v2_initialize_params_for_reuse(&message) + .expect("v1-shaped initialize request should be valid"); + + assert_eq!(normalized, params); + } + + #[cfg(feature = "unstable_auth_methods")] + #[test] + fn null_v2_terminal_marker_meta_is_omitted_before_v1_normalization() { + let mut params = v2_initialize_params(); + params.insert( + "capabilities".into(), + serde_json::json!({ + "auth": { + "terminal": { "_meta": null } + } + }), + ); + + let normalized = normalize_v2_initialize_params_for_v1(¶ms, false) + .expect("null marker metadata is equivalent to omission"); + let normalized = serde_json::Value::Object(normalized); + + assert_eq!( + normalized.pointer("/clientCapabilities/auth/terminal"), + Some(&serde_json::Value::Bool(true)) + ); + } + + #[cfg(feature = "unstable_auth_methods")] + #[test] + fn terminal_marker_metadata_disables_reuse_but_not_v1_routing() { + let mut params = v2_initialize_params(); + params.insert( + "capabilities".into(), + serde_json::json!({ + "auth": { + "terminal": { + "_meta": { "source": "test" } + } + } + }), + ); + + normalize_v2_initialize_params_for_v1(¶ms, false) + .expect("v1 routing may discard terminal marker metadata"); + normalize_v2_initialize_params_for_v1(¶ms, true) + .expect_err("connection reuse must preserve terminal marker metadata"); } } @@ -622,18 +821,35 @@ fn parse_initialize_params( } #[cfg(feature = "unstable_protocol_v2")] -fn replace_initialize_params( - params: &mut serde_json::Map, +fn serialize_initialize_params( initialize: impl Serialize, -) -> Result<(), crate::Error> { +) -> Result, crate::Error> { let value = serde_json::to_value(initialize).map_err(crate::Error::into_internal_error)?; let serde_json::Value::Object(object) = value else { return Err(crate::util::internal_error( "initialize params did not serialize to an object", )); }; - *params = object; - Ok(()) + Ok(object) +} + +#[cfg(feature = "unstable_protocol_v2")] +fn json_object_contains( + actual: &serde_json::Map, + expected: &serde_json::Map, +) -> bool { + fn contains(actual: &serde_json::Value, expected: &serde_json::Value) -> bool { + match (actual, expected) { + (serde_json::Value::Object(actual), serde_json::Value::Object(expected)) => expected + .iter() + .all(|(key, value)| actual.get(key).is_some_and(|item| contains(item, value))), + _ => actual == expected, + } + } + + expected + .iter() + .all(|(key, value)| actual.get(key).is_some_and(|item| contains(item, value))) } #[cfg(feature = "unstable_protocol_v2")] diff --git a/src/agent-client-protocol/tests/protocol_v2.rs b/src/agent-client-protocol/tests/protocol_v2.rs index fba614e..3dc93df 100644 --- a/src/agent-client-protocol/tests/protocol_v2.rs +++ b/src/agent-client-protocol/tests/protocol_v2.rs @@ -329,6 +329,8 @@ struct InitializingV2Client { expected_session_id: &'static str, } +struct FutureInitializeV2Client; + struct DropTrackedClient { client: C, dropped: Arc, @@ -387,6 +389,40 @@ impl ConnectTo for InitializingV2Client { } } +impl ConnectTo for FutureInitializeV2Client { + async fn connect_to(self, agent: impl ConnectTo) -> Result<(), Error> { + let (mut channel, agent_future) = ConnectTo::::into_channel_and_future(agent); + let agent_task = tokio::spawn(agent_future); + + channel + .tx + .unbounded_send(TransportFrame::Single(RawJsonRpcMessage::request( + "initialize".into(), + initialize_params_with_extensions(ProtocolVersion::V2)?, + v1::RequestId::Number(1), + )?)) + .map_err(Error::into_internal_error)?; + + while let Some(message) = channel.rx.next().await { + let TransportFrame::Single(message) = message else { + continue; + }; + let RawJsonRpcMessage::Response(v1::Response::Result { result, .. }) = message else { + continue; + }; + let initialize = v2::InitializeResponse::from_value("initialize", result)?; + assert_eq!(initialize.protocol_version, ProtocolVersion::V2); + agent_task.abort(); + return Ok(()); + } + + agent_task.abort(); + Err(agent_client_protocol::util::internal_error( + "v2 agent did not respond to initialize", + )) + } +} + fn v1_agent_with_session(session_id: &'static str) -> impl ConnectTo { Agent .builder() @@ -976,6 +1012,52 @@ async fn client_protocol_connector_routes_to_v2_client_for_v2_agent() -> Result< .await } +#[tokio::test(flavor = "current_thread")] +async fn client_protocol_connector_does_not_require_v1_representability_for_v2_agent() +-> Result<(), Error> { + Client + .protocol_connector() + .with_v2(|| FutureInitializeV2Client) + .connect_to(|| { + Agent.v2().on_receive_request( + async |initialize: v2::InitializeRequest, responder, _cx| { + assert_eq!(initialize.protocol_version, ProtocolVersion::V2); + responder.respond(v2_initialize_response_with_session( + initialize.protocol_version, + )) + }, + agent_client_protocol::on_receive_request!(), + ) + }) + .await +} + +#[tokio::test(flavor = "current_thread")] +async fn client_protocol_connector_reconnects_when_v2_initialize_has_future_fields() +-> Result<(), Error> { + let connections = Arc::new(AtomicUsize::new(0)); + let agent_connections = Arc::clone(&connections); + + Client + .protocol_connector() + .with_v1(|| InitializingV1Client::new("v1-reconnected-session")) + .with_v2(|| FutureInitializeV2Client) + .connect_to(move || { + agent_connections.fetch_add(1, Ordering::SeqCst); + Agent + .protocol_router() + .with_v1(v1_agent_with_session("v1-reconnected-session")) + }) + .await?; + + assert_eq!( + connections.load(Ordering::SeqCst), + 2, + "future v2 fields must disable reuse of the normalized v1 connection" + ); + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn client_protocol_connector_does_not_retry_after_v2_initialize_rejection() -> Result<(), Error> { @@ -1375,6 +1457,23 @@ async fn protocol_router_downgrades_v2_initialize_metadata_to_v1() -> Result<(), "{:?}", initialize.client_capabilities ); + assert_eq!( + initialize + .client_capabilities + .meta + .as_ref() + .and_then(|meta| meta.get("source")), + Some(&Value::String("v2".into())) + ); + assert_eq!( + initialize + .meta + .as_ref() + .and_then(|meta| meta.get("request")), + Some(&Value::Bool(true)) + ); + #[cfg(feature = "unstable_auth_methods")] + assert!(initialize.client_capabilities.auth.terminal); responder.respond(v1::InitializeResponse::new(initialize.protocol_version)) }, agent_client_protocol::on_receive_request!(), @@ -1383,10 +1482,22 @@ async fn protocol_router_downgrades_v2_initialize_metadata_to_v1() -> Result<(), Client .v2() .connect_with(agent, async |cx| { + let mut capabilities = v2::ClientCapabilities::new().meta(Map::from_iter([( + "source".into(), + Value::String("v2".into()), + )])); + #[cfg(feature = "unstable_auth_methods")] + { + capabilities = capabilities.auth( + v2::AuthCapabilities::new().terminal(v2::TerminalAuthCapabilities::new()), + ); + } let request = v2::InitializeRequest::new( ProtocolVersion::V2, v2::Implementation::new("v2-metadata-client", "9.9.9").title("V2 Metadata Client"), - ); + ) + .capabilities(capabilities) + .meta(Map::from_iter([("request".into(), Value::Bool(true))])); let error = cx .send_request(request) .block_task() diff --git a/src/agent-client-protocol/tests/schema_plan_operations.rs b/src/agent-client-protocol/tests/schema_plan_operations.rs index e272673..27f6327 100644 --- a/src/agent-client-protocol/tests/schema_plan_operations.rs +++ b/src/agent-client-protocol/tests/schema_plan_operations.rs @@ -79,11 +79,8 @@ fn v1_plan_operations_are_capability_gated_and_routed() { #[cfg(feature = "unstable_protocol_v2")] #[test] -fn draft_v2_file_and_removal_operations_are_routed_and_convert() { - use agent_client_protocol::schema::v2::{ - self, - conversion::{try_v1_to_v2, try_v2_to_v1_many}, - }; +fn draft_v2_file_and_removal_operations_are_routed() { + use agent_client_protocol::schema::v2; let notification = v2::UpdateSessionNotification::new( "session-1", @@ -101,11 +98,16 @@ fn draft_v2_file_and_removal_operations_are_routed_and_convert() { }; assert!(matches!(parsed.update, v2::SessionUpdate::PlanUpdate(_))); - let v1_update = SessionUpdate::PlanRemoved(PlanRemoved::new("plan-1")); - let v2_update: v2::SessionUpdate = try_v1_to_v2(v1_update.clone()).unwrap(); - assert!(matches!(v2_update, v2::SessionUpdate::PlanRemoved(_))); - assert_eq!( - try_v2_to_v1_many::<_, SessionUpdate>(v2_update).unwrap(), - vec![v1_update] + let notification = v2::UpdateSessionNotification::new( + "session-1", + v2::SessionUpdate::PlanRemoved(v2::PlanRemoved::new("plan-1")), ); + let untyped = notification.to_untyped_message().unwrap(); + assert_eq!(untyped.params["update"]["sessionUpdate"], "plan_removed"); + let v2::AgentNotification::UpdateSessionNotification(parsed) = + v2::AgentNotification::parse_message("session/update", &untyped.params).unwrap() + else { + panic!("expected a v2 session update notification"); + }; + assert!(matches!(parsed.update, v2::SessionUpdate::PlanRemoved(_))); } diff --git a/src/agent-client-protocol/tests/schema_tool_call_name.rs b/src/agent-client-protocol/tests/schema_tool_call_name.rs index b0179bb..8dcb16c 100644 --- a/src/agent-client-protocol/tests/schema_tool_call_name.rs +++ b/src/agent-client-protocol/tests/schema_tool_call_name.rs @@ -1,7 +1,5 @@ #![cfg(feature = "unstable_tool_call_name")] -#[cfg(feature = "unstable_protocol_v2")] -use agent_client_protocol::schema::v1::ToolCallUpdate as V1ToolCallUpdate; use agent_client_protocol::schema::v1::{ToolCall, ToolCallUpdateFields}; use serde_json::json; @@ -27,28 +25,15 @@ fn v1_tool_call_name_serializes_and_updates() { #[cfg(feature = "unstable_protocol_v2")] #[test] -fn tool_call_name_preserves_v2_patch_semantics() { - use agent_client_protocol::schema::{ - MaybeUndefined, - v2::{ - ToolCallUpdate, - conversion::{try_v1_to_v2, try_v2_to_v1}, - }, - }; - - let named: ToolCallUpdate = - try_v1_to_v2(ToolCall::new("call_1", "Read configuration").name("read_file")).unwrap(); +fn v2_tool_call_name_has_patch_semantics() { + use agent_client_protocol::schema::{MaybeUndefined, v2::ToolCallUpdate}; + + let named = ToolCallUpdate::new("call_1").name("read_file"); assert_eq!(named.name, MaybeUndefined::Value("read_file".to_string())); - let omitted: ToolCallUpdate = - try_v1_to_v2(V1ToolCallUpdate::new("call_1", ToolCallUpdateFields::new())).unwrap(); + let omitted = ToolCallUpdate::new("call_1"); assert_eq!(omitted.name, MaybeUndefined::Undefined); let cleared = ToolCallUpdate::new("call_1").name(None::); assert_eq!(cleared.name, MaybeUndefined::Null); - let error = try_v2_to_v1::<_, V1ToolCallUpdate>(cleared).unwrap_err(); - assert_eq!( - error.message(), - "v2 ToolCallUpdate.name with null value cannot be represented in v1" - ); }