Repository navigation
fix: reject discovery for legacy-only servers - #1269
JulienCote wants to merge 1 commit into
Conversation
github-actions
Bot
added
T-test
60b94ea to
61e1d60
Compare
| extensions: std::mem::take(request.extensions_mut()), | ||
| peer: peer.clone(), | ||
| }; | ||
| let response = match service.handle_request(request, context).await { |
There was a problem hiding this comment.
In serve_server_with_ct_inner, which manages the connection lifecycle, the discovery handler now waits forever on context.peer.notify_progress(...). Sending the notification waits for an acknowledgment from the service loop, but serve_inner does not start until the handler returns.
There was a problem hiding this comment.
Addressed by rebasing onto current main. #1248 now handles bootstrap discovery with an isolated peer, so a handler notification cannot wait on the not-yet-started service loop. This PR no longer changes that lifecycle path and is focused only on rejecting discovery for legacy-only servers.
Claude Code probes a stdio server with `server/discover` before `initialize` and falls back to `initialize` when the probe is refused. This server refuses it (2026-07-28 is deliberately not offered, see 542c2e1), but rmcp 3.1.3 then marks the session as needing per-request `_meta` and never clears the mark, so the legacy `tools/list` that follows is refused with -32602 and the client sees no tools at all. Wrap the stdio transport in a gate that answers the probe itself with -32601 Method not found, which is what a legacy-only server would say and what makes the client fall back; rmcp never sees the probe and starts from `initialize` as before. The gate turns itself off once the session has opened and whenever a 2026-07-28-or-newer revision is offered, so it is inert on every path except the one it exists for. This is a workaround for a defect rmcp fixes upstream in modelcontextprotocol/rust-sdk#1269. A canary test pins the defect against bare rmcp; when it fails after a dependency bump, the gate is to be deleted and `stdio()` handed to `serve` directly again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T8Fm7fDybwrRtpABNjdXmJ
|
This has merge conflicts with |
10d27a2 to
1b6cbb0
Compare
JulienCote
changed the title
|
Rebased onto the latest |
Servers that advertise only initialize-based protocol versions cannot establish the discovery lifecycle. Return MethodNotFound before dispatching `server/discover` so Auto clients fall back to `initialize` without invoking the discovery handler. Update discovery HTTP fixtures to advertise a discovery-capable version and cover the legacy-only fallback path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1b6cbb0 to
13e4798
Compare
|
|
||
| fn supported_protocol_versions(&self) -> Cow<'static, [ProtocolVersion]> { | ||
| Cow::Borrowed(&[ProtocolVersion::V_2025_11_25]) | ||
| Cow::Borrowed(&[ProtocolVersion::V_2025_11_25, ProtocolVersion::V_2026_07_28]) |
There was a problem hiding this comment.
With streamable HTTP, MethodNotFound becomes an HTTP 404. The client fallback then depends on streamable_http_client converting that 404 back into a JSON-RPC error. Now that this fixture supports discovery and the new test only covers the duplex transport, we don’t have anything that tests the full HTTP path. Would it be worth adding an HTTP test for a legacy-only server to check the 404 status and the -32601 response body, or the Auto fallback?
| _context: RequestContext<RoleServer>, | ||
| ) -> Result<DiscoverResult, ErrorData> { | ||
| self.discover_called.store(true, Ordering::Relaxed); | ||
| Err(ErrorData::method_not_found::<DiscoverRequestMethod>()) |
There was a problem hiding this comment.
Since this stub also returns MethodNotFound, the client would still fall back if we removed the new guard. That means only the discover_called flag would catch the regression.
| Err(ErrorData::method_not_found::<DiscoverRequestMethod>()) | |
| Ok(DiscoverResult::from_server_info( | |
| self.supported_protocol_versions().into_owned(), | |
| self.get_info(), | |
| )) |
| // Legacy-only servers do not implement discovery. MethodNotFound tells | ||
| // dual-lifecycle clients to fall back to initialize. |
There was a problem hiding this comment.
A custom ServerHandler::discover override is no longer called when supported_protocol_versions returns only versions from the initialize era. Implementers can't tell that from the trait, so could we add a short note to the discover doc comment in server_handler_methods!?
| .await | ||
| .expect("client should fall back to initialize"); | ||
|
|
||
| assert!(!discover_called.load(Ordering::Relaxed)); |
There was a problem hiding this comment.
This confirms the client started, but not that it actually used the legacy_version it was given. One more assertion would verify that.
| assert!(!discover_called.load(Ordering::Relaxed)); | |
| assert!(!discover_called.load(Ordering::Relaxed)); | |
| assert_eq!( | |
| client.peer_info().expect("peer info").protocol_version, | |
| ProtocolVersion::V_2025_11_25 | |
| ); |
| // Legacy-only servers do not implement discovery. MethodNotFound tells | ||
| // dual-lifecycle clients to fall back to initialize. | ||
| if matches!(&request, ClientRequest::DiscoverRequest(_)) | ||
| && self | ||
| .supported_protocol_versions() | ||
| .iter() | ||
| .all(ProtocolVersion::has_initialize) | ||
| { | ||
| return Err(McpError::method_not_found::<DiscoverRequestMethod>()); | ||
| } | ||
| if uses_inline_negotiation && let Some(requested_version) = requested_version.as_ref() { | ||
| let supported_versions = self.supported_protocol_versions(); |
There was a problem hiding this comment.
The guard and the version check below both call supported_protocol_versions(), so a custom implementation that returns Cow::Owned allocates twice per request. Reading it once lets both checks use the same list.
| // Legacy-only servers do not implement discovery. MethodNotFound tells | |
| // dual-lifecycle clients to fall back to initialize. | |
| if matches!(&request, ClientRequest::DiscoverRequest(_)) | |
| && self | |
| .supported_protocol_versions() | |
| .iter() | |
| .all(ProtocolVersion::has_initialize) | |
| { | |
| return Err(McpError::method_not_found::<DiscoverRequestMethod>()); | |
| } | |
| if uses_inline_negotiation && let Some(requested_version) = requested_version.as_ref() { | |
| let supported_versions = self.supported_protocol_versions(); | |
| let supported_versions = self.supported_protocol_versions(); | |
| // Legacy-only servers do not implement discovery. MethodNotFound tells | |
| // dual-lifecycle clients to fall back to initialize. | |
| if matches!(&request, ClientRequest::DiscoverRequest(_)) | |
| && supported_versions | |
| .iter() | |
| .all(ProtocolVersion::has_initialize) | |
| { | |
| return Err(McpError::method_not_found::<DiscoverRequestMethod>()); | |
| } | |
| if uses_inline_negotiation && let Some(requested_version) = requested_version.as_ref() { |
This branch has not been deployed
2 participants
Summary
server/discoverbefore handler dispatch when every advertised protocol version uses the classicinitializelifecycleAutoclients recognize legacy-only servers and fall back toinitializeon the same connectionMotivation
The default discovery handler advertises
supported_protocol_versions. For a server that advertises only initialize-era versions, a modernAutoclient otherwise receives a successful discovery response with no compatible discovery version and fails withNoCompatibleProtocolVersioninstead of falling back.Returning
MethodNotFoundidentifies the server as legacy without invoking a custom discovery handler. The bootstrap-neutral handling merged in #1248 then permits the client to continue withinitializesafely.Testing
cargo fmt --all -- --checkcargo test -p rmcp --features "client reqwest transport-io transport-streamable-http-server" --test test_stateless_server_requests --test test_cancelled_response --test test_server_discover_http