From 2af076ebfc85ffa6e7e6214b65e859c99c8bcb5b Mon Sep 17 00:00:00 2001 From: ip2a Date: Wed, 5 Aug 2026 15:39:56 +0800 Subject: [PATCH] fix(client): classify discover failures before Auto legacy fallback `ClientLifecycleMode::Auto` only fell back from `server/discover` on `-32601`, so legacy servers that reject the probe with other codes (`-32600`, `-32602`, session-middleware errors) failed to connect even though `initialize` would have succeeded. The 2026-07-28 backward-compatibility guidance is explicit that the fallback MUST NOT be keyed to one specific error code, but it also only applies to failures that signal a legacy server. Add `ClientInitializeError::indicates_legacy_server`, which classifies a discover failure by what it says about the peer: - a JSON-RPC error is legacy unless it is a modern-era rejection (`MISSING_REQUIRED_CLIENT_CAPABILITY` or `HEADER_MISMATCH`; version negotiation is already handled inside `discover_startup`, so `UNSUPPORTED_PROTOCOL_VERSION` never reaches here); - a closed connection, an unexpected response shape, or a non-authorization transport failure is treated as a legacy server that did not engage the probe; - an authorization or scope gate (HTTP 401/403, missing local OAuth), a modern version mismatch, or a client-side condition (`NoPreferredProtocolVersion`, `Cancelled`) is surfaced, because an `initialize` retry cannot resolve it and would mask the actionable error. The fallback is keyed on this classification, and the new `is_authorization_failure` helper fills the 403 gap left by the existing 401-only `is_authorization_required`. The classification match is exhaustive on `ClientInitializeError`, so adding a variant forces a decision instead of being silently swept into the fallback. A silently legacy server that ignores the probe entirely still stalls `expect_response`; that requires a discover timeout and is out of scope here. Fixes #1040. --- crates/rmcp/src/service/client.rs | 76 +++++++++- .../rmcp/tests/test_client_lifecycle_modes.rs | 131 ++++++++++++++++++ .../test_legacy_server_classification.rs | 118 ++++++++++++++++ 3 files changed, 322 insertions(+), 3 deletions(-) create mode 100644 crates/rmcp/tests/test_legacy_server_classification.rs diff --git a/crates/rmcp/src/service/client.rs b/crates/rmcp/src/service/client.rs index 2e25b13a3..5a93475f2 100644 --- a/crates/rmcp/src/service/client.rs +++ b/crates/rmcp/src/service/client.rs @@ -122,6 +122,58 @@ impl ClientInitializeError { Self::TransportError { error, .. } if error.is_authorization_required() ) } + + /// Whether initialization failed at an authorization or scope gate — an HTTP + /// `401`, an HTTP `403`, or missing local OAuth credentials. + /// + /// A legacy `initialize` retry cannot resolve any of these, so the caller + /// must surface the error instead of masking it with a fallback. This + /// complements [`is_authorization_required`](Self::is_authorization_required), + /// which is deliberately limited to the 401/credentials case. + pub(crate) fn is_authorization_failure(&self) -> bool { + #[cfg(feature = "transport-streamable-http-client")] + if self.auth_challenge().is_some() { + return true; + } + self.is_authorization_required() + } + + /// Whether a failed `server/discover` probe indicates that the peer is a + /// legacy server a fallback to `initialize` could still reach. + /// + /// The 2026-07-28 backward-compatibility guidance frames the fallback as a + /// property of the *server's* response: a legacy server returns an + /// implementation-defined JSON-RPC error, drops the connection, or fails to + /// respond. Failures that are not server signals — an authorization or + /// scope gate, a client-side misconfiguration, a modern-era rejection, or a + /// cancellation — are surfaced, because an `initialize` retry cannot + /// address them and would mask the actionable error. + /// + /// The match is exhaustive on purpose: adding a `ClientInitializeError` + /// variant forces the author to decide whether it indicates a legacy server. + pub fn indicates_legacy_server(&self) -> bool { + match self { + // The server returned an MCP-level error. It is legacy unless the + // error is a modern-era rejection; version negotiation is already + // handled inside discover_startup, so UNSUPPORTED_PROTOCOL_VERSION + // never reaches here. + Self::JsonRpcError(data) => !is_modern_rejection_code(data.code), + // The server closed the connection without an MCP response. + Self::ConnectionClosed(_) => true, + // A transport failure: surface an authorization/scope gate, since + // initialize would hit the same gate; otherwise the server did not + // produce a valid response, so attempt the fallback. + Self::TransportError { .. } => !self.is_authorization_failure(), + // The server answered discover but no version overlaps — modern. + Self::NoCompatibleProtocolVersion { .. } => false, + // Client-side state, not a signal about the server. + Self::NoPreferredProtocolVersion | Self::Cancelled => false, + // The server answered with an unexpected shape or request id. + Self::ExpectedInitResponse(_) + | Self::ExpectedInitResult(_) + | Self::ConflictInitResponseId(_, _) => true, + } + } } /// Helper function to get the next message from the stream @@ -739,9 +791,9 @@ where .await; match discover_result { Ok(()) => {} - Err(ClientInitializeError::JsonRpcError(error)) - if error.code == crate::model::ErrorCode::METHOD_NOT_FOUND => - { + // The failure identifies a legacy server the `initialize` + // handshake could still reach; fall back to it. + Err(error) if error.indicates_legacy_server() => { let mut legacy_info = client_info; if let Some(version) = legacy_version { legacy_info.protocol_version = version; @@ -749,6 +801,9 @@ where legacy_startup(&service, &mut transport, &id_provider, &peer, legacy_info) .await?; } + // Everything else (modern-server rejection, authorization gate, + // client-side state) cannot be resolved by `initialize` and is + // surfaced so the actionable error is not masked. Err(error) => return Err(error), } } @@ -756,6 +811,21 @@ where Ok(serve_inner(service, transport, peer, peer_rx, ct)) } +/// Modern-era JSON-RPC error codes a server can return from `server/discover` +/// without being legacy. Version negotiation (`UNSUPPORTED_PROTOCOL_VERSION`) +/// is handled inside `discover_startup` and never reaches the Auto fallback. +/// +/// `ErrorCode` is an open integer type, so this cannot be an exhaustive match: +/// if a future revision adds another modern-era rejection code, add it here and +/// to the corresponding classification test. +fn is_modern_rejection_code(code: crate::model::ErrorCode) -> bool { + matches!( + code, + crate::model::ErrorCode::MISSING_REQUIRED_CLIENT_CAPABILITY + | crate::model::ErrorCode::HEADER_MISMATCH + ) +} + async fn legacy_startup( service: &S, transport: &mut T, diff --git a/crates/rmcp/tests/test_client_lifecycle_modes.rs b/crates/rmcp/tests/test_client_lifecycle_modes.rs index 4b75c3550..ac0c9a456 100644 --- a/crates/rmcp/tests/test_client_lifecycle_modes.rs +++ b/crates/rmcp/tests/test_client_lifecycle_modes.rs @@ -337,6 +337,137 @@ async fn auto_startup_falls_back_after_discover_method_not_found() { server_task.await.expect("server task"); } +/// Drives an `Auto` client through a single `server/discover` probe and asserts +/// the legacy fallback decision against the response the server sends back. +/// +/// When `expect_fallback` is set, the server also accepts the subsequent +/// `initialize` request and the client is expected to connect. Otherwise the +/// client must surface the discover error without sending `initialize`, and the +/// server's next receive must not be an initialize request. +async fn run_auto_discover_response_scenario(error: ErrorData, expect_fallback: bool) { + let (server_transport, client_transport) = tokio::io::duplex(4096); + let mut server = IntoTransport::::into_transport(server_transport); + let server_task = tokio::spawn(async move { + let ClientJsonRpcMessage::Request(discover) = + server.receive().await.expect("expected discover request") + else { + panic!("expected request"); + }; + assert!(matches!( + discover.request, + ClientRequest::DiscoverRequest(_) + )); + server + .send(ServerJsonRpcMessage::error(error, Some(discover.id))) + .await + .expect("send discover error response"); + + if expect_fallback { + let ClientJsonRpcMessage::Request(initialize) = + server.receive().await.expect("expected initialize request") + else { + panic!("expected request"); + }; + assert!(matches!( + initialize.request, + ClientRequest::InitializeRequest(_) + )); + server + .send(ServerJsonRpcMessage::response( + ServerResult::InitializeResult(InitializeResult::new( + ServerCapabilities::default(), + )), + initialize.id, + )) + .await + .expect("send initialize response"); + assert!(matches!( + server.receive().await, + Some(ClientJsonRpcMessage::Notification(_)) + )); + } else { + // The client must surface the error without falling back, so no + // initialize request should follow. The transport closes when the + // failed client is dropped. + if let Some(ClientJsonRpcMessage::Request(request)) = server.receive().await { + panic!( + "client fell back to {:?} but should have surfaced the modern error", + request.request + ); + } + } + }); + + let client_result = DiscoverClient + .serve_with_lifecycle( + client_transport, + ClientLifecycleMode::Auto { + preferred_versions: vec![ProtocolVersion::V_2026_07_28], + legacy_version: Some(ProtocolVersion::V_2025_11_25), + }, + ) + .await; + + if expect_fallback { + let client = client_result.expect("auto client should fall back to initialize"); + client.cancel().await.expect("cancel client"); + } else { + assert!( + client_result.is_err(), + "modern error should surface without legacy fallback" + ); + } + server_task.await.expect("server task"); +} + +#[tokio::test] +async fn auto_startup_falls_back_after_discover_invalid_request() { + // Legacy servers commonly reject an unknown pre-initialize request with + // `-32600` (e.g. a session middleware that requires `initialize` first). + run_auto_discover_response_scenario( + ErrorData::new(ErrorCode::INVALID_REQUEST, "Bad Request", None), + true, + ) + .await; +} + +#[tokio::test] +async fn auto_startup_falls_back_after_discover_invalid_params() { + // `-32602` is explicitly called out by the specification as an + // implementation-defined response legacy servers use for unknown requests. + run_auto_discover_response_scenario( + ErrorData::new(ErrorCode::INVALID_PARAMS, "Invalid params", None), + true, + ) + .await; +} + +#[tokio::test] +async fn auto_startup_does_not_fall_back_for_missing_required_capability() { + // A `MISSING_REQUIRED_CLIENT_CAPABILITY` response identifies a modern + // server; falling back to `initialize` would not address it. + run_auto_discover_response_scenario( + ErrorData::new( + ErrorCode::MISSING_REQUIRED_CLIENT_CAPABILITY, + "Missing required client capability", + None, + ), + false, + ) + .await; +} + +#[tokio::test] +async fn auto_startup_does_not_fall_back_for_header_mismatch() { + // A `HEADER_MISMATCH` response identifies a modern server performing + // header validation; falling back to `initialize` would not address it. + run_auto_discover_response_scenario( + ErrorData::new(ErrorCode::HEADER_MISMATCH, "Header mismatch", None), + false, + ) + .await; +} + #[tokio::test] async fn discover_startup_retries_a_mutually_supported_version() { let unsupported: ProtocolVersion = diff --git a/crates/rmcp/tests/test_legacy_server_classification.rs b/crates/rmcp/tests/test_legacy_server_classification.rs new file mode 100644 index 000000000..159cef08a --- /dev/null +++ b/crates/rmcp/tests/test_legacy_server_classification.rs @@ -0,0 +1,118 @@ +//! Classification of `ClientInitializeError` for the `Auto` lifecycle fallback +//! decision: which failures indicate a legacy server worth an `initialize` +//! retry, and which must be surfaced because `initialize` cannot help. + +use std::{any::TypeId, error::Error}; + +use rmcp::{ + model::ErrorCode, + service::ClientInitializeError, + transport::{ + AuthError, DynamicTransportError, + streamable_http_client::{AuthRequiredError, InsufficientScopeError, StreamableHttpError}, + }, +}; +use thiserror::Error; + +type TestHttpError = StreamableHttpError; + +#[derive(Debug, Error)] +#[error("outer transport wrapper")] +struct OuterError(#[source] TestHttpError); + +fn transport_error(error: impl Error + Send + Sync + 'static) -> ClientInitializeError { + ClientInitializeError::TransportError { + error: DynamicTransportError::from_parts( + "test transport", + TypeId::of::<()>(), + Box::new(error), + ), + context: "discover".into(), + } +} + +fn json_rpc_error(code: ErrorCode) -> ClientInitializeError { + ClientInitializeError::JsonRpcError(rmcp::model::ErrorData::new(code, "test", None)) +} + +#[test] +fn legacy_server_json_rpc_errors_indicate_legacy() { + // Legacy servers reject an unknown pre-initialize request with + // implementation-defined errors; these are exactly the case the spec says + // to fall back from. + assert!(json_rpc_error(ErrorCode::METHOD_NOT_FOUND).indicates_legacy_server()); + assert!(json_rpc_error(ErrorCode::INVALID_REQUEST).indicates_legacy_server()); + assert!(json_rpc_error(ErrorCode::INVALID_PARAMS).indicates_legacy_server()); +} + +#[test] +fn modern_rejection_json_rpc_errors_do_not_indicate_legacy() { + // A modern server rejects discover for a capability or header reason; + // initialize would not address either. + assert!( + !json_rpc_error(ErrorCode::MISSING_REQUIRED_CLIENT_CAPABILITY).indicates_legacy_server() + ); + assert!(!json_rpc_error(ErrorCode::HEADER_MISMATCH).indicates_legacy_server()); +} + +#[test] +fn closed_connection_indicates_legacy() { + assert!( + ClientInitializeError::ConnectionClosed("server closed".to_owned()) + .indicates_legacy_server() + ); +} + +#[test] +fn authorization_gate_does_not_indicate_legacy() { + // HTTP 401 and 403, plus missing local OAuth, all block initialize too; + // the fallback must not mask them. + let http_401 = transport_error(TestHttpError::AuthRequired(AuthRequiredError::new( + "Bearer realm=\"mcp\"".to_owned(), + ))); + let http_403 = transport_error(TestHttpError::InsufficientScope( + InsufficientScopeError::new( + "Bearer error=\"insufficient_scope\"".to_owned(), + Some("admin".to_owned()), + ), + )); + let local_oauth = transport_error(TestHttpError::Auth(AuthError::AuthorizationRequired)); + let nested = transport_error(OuterError(TestHttpError::Auth( + AuthError::AuthorizationRequired, + ))); + + assert!(!http_401.indicates_legacy_server()); + assert!(!http_403.indicates_legacy_server()); + assert!(!local_oauth.indicates_legacy_server()); + assert!(!nested.indicates_legacy_server()); +} + +#[test] +fn other_transport_failures_indicate_legacy() { + // A transport failure that is not an auth/scope gate (e.g. an IO error or + // a non-JSON HTTP response) looks like a legacy server that did not engage + // the probe; the fallback is attempted. + let channel_closed = transport_error(TestHttpError::TransportChannelClosed); + assert!(channel_closed.indicates_legacy_server()); +} + +#[test] +fn client_side_errors_do_not_indicate_legacy() { + assert!(!ClientInitializeError::NoPreferredProtocolVersion.indicates_legacy_server()); + assert!(!ClientInitializeError::Cancelled.indicates_legacy_server()); +} + +#[test] +fn modern_version_mismatch_does_not_indicate_legacy() { + let error = ClientInitializeError::NoCompatibleProtocolVersion { + client_supported: vec![rmcp::model::ProtocolVersion::V_2026_07_28], + server_supported: vec![], + }; + assert!(!error.indicates_legacy_server()); +} + +#[test] +fn unexpected_response_shape_indicates_legacy() { + assert!(ClientInitializeError::ExpectedInitResult(None).indicates_legacy_server()); + assert!(ClientInitializeError::ExpectedInitResponse(None).indicates_legacy_server()); +}