Uh oh!
There was an error while loading. Please reload this page.
fix: remove server_info from DiscoverResult - #1065
Conversation
7c53e17 to
6b9c7b5Compare
tsarlandie-oai
left a comment
There was a problem hiding this comment.
Moving serverInfo into _meta for serialization makes sense, but clients must remain able to consume legacy top-level serverInfo, and must not fail when server identity is absent: the specification makes this metadata optional. The current MissingServerInfo path already causes 15 client conformance failures and bypasses the conservative fallback in #1045. We should preserve the legacy deserialization test, emit only the modern representation, and accept both representations on input.
| #[test] | ||
| fn discover_result_prefers_top_level_server_info_over_namespaced_metadata() { | ||
| fn discover_result_ignores_legacy_top_level_server_info() { |
There was a problem hiding this comment.
We should not change this test. I think it's a useful test to make sure we remain compatible with legacy servers.
6b9c7b5 to
b68d366CompareDaleSeo
commented
Jul 28, 2026
@howardjohn Can you take a look at the failing checks? |
b68d366 to
475c8c9Compare18d2c58 to
9e1c5e2Comparehowardjohn
commented
Jul 28, 2026
sorry should be good to go now! |
| if let Some(server_info) = result.server_info() { | ||
| peer.set_peer_info(ServerInfo { | ||
| protocol_version: selected.clone(), | ||
| capabilities: result.capabilities, | ||
| server_info, | ||
| instructions: result.instructions, | ||
| meta: result.meta, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Since serverInfo is optional, this branch leaves peer_info unset even for a valid discovery response. That means the version, capabilities, and instructions were negotiated, but RoleClient::enforce_peer_request_association still treats the connection as legacy and skips the 2026-07-28 guard against unsolicited sampling, roots, or elicitation requests.
There was a problem hiding this comment.
Good call.. working on it
There was a problem hiding this comment.
I think we have 3 options:
- Make PeerInfo a new type with server_info optional (breaking type change)
- Make an Implementation with just version, a empty/fake
name, and the restNone. No breaking change but pretty wonky - Just pass the version through for enforce_peer_request_association but do not give any peer info (they lose instructions etc)
(1) seems the best option but wanted to check we are ok with the break?
There was a problem hiding this comment.
@howardjohn I agree that 1 is the cleanest option, and I'm okay with the break since this is already a breaking change.
There was a problem hiding this comment.
👍 sent a change with (1)
DaleSeo
commented
Jul 28, 2026
For context, I introduced the top-level field in #973 following the accepted SEP-2575 schema. About 15 hours before it merged, modelcontextprotocol/modelcontextprotocol/pull/3002 moved server identity into the optional result |
9e1c5e2 to
0f4d2a2CompareFixesmodelcontextprotocol#1064 Replaces modelcontextprotocol#1044 This removes the `server_info` field which is not part of the spec: https://modelcontextprotocol.io/specification/draft/schema#discoverresult.
0f4d2a2 to
43fcf67CompareUh oh!
There was an error while loading. Please reload this page.
Rebase onto 3.0.0: RoleClient::PeerInfo is now ServerPeerInfo (modelcontextprotocol#1065) and its constructor takes the protocol version directly.
…t association (#1055) * refactor(service): express SEP-2260 receive-side association as an enum Replaces the has_pending_outbound_request bool on enforce_peer_request_association with PeerRequestAssociation, so a stream-separating transport can report per-request association (#1033). Behavior-preserving: the event loop still passes the coarse signal as Unknown. * feat(transport): record inbound stream origin on streamable HTTP client (#1033) The worker attaches an InboundStreamOrigin extension to each inbound server request: Unassociated for the standalone GET stream, OutboundRequest(id) for a POST's SSE stream. Mirror of the OriginatingRequestId marker used by the server side. * feat(service): enforce SEP-2260 client receive-side check per stream origin (#1033) The event loop maps InboundStreamOrigin plus the in-flight responder pool to PeerRequestAssociation: restricted requests arriving on the standalone GET stream are now rejected with -32602 even while unrelated outbound requests are in flight. * test: end-to-end SEP-2260 stream-based enforcement over streamable HTTP (#1033) * docs: cross-link SEP-2260 stream markers * test: origin marker survives SSE resumption per SEP-2260 (#1033) A POST SSE stream resumed via GET + Last-Event-ID (SEP-1699) reconnects beneath execute_sse_stream, so requests replayed after a resume keep their OutboundRequest origin. Pins the layering invariant: hoisting reconnection above the marker attach point would wrongly reject associated requests with -32602. * docs: trim SEP-2260 comments to spec rationale * docs: align SEP-2577 expect reason with sibling suppressions * test: harden SEP-2260 stream-enforcement e2e (#1033) Detect handler invocation via a channel asserted empty instead of a panic in a spawned task (swallowed, cannot fail the test); surface scripted-server misuse as transport errors rather than panics in the transport task; bound the tail awaits with 5s timeouts. Correct the header comment: a 2026-07-28 server minting a session id is not spec-legal (SEP-2567 removes sessions and the GET endpoint) — the scripted server is deliberately non-conforming, which is the point of receive-side enforcement. * test: adapt SEP-2260 association tests to ServerPeerInfo Rebase onto 3.0.0: RoleClient::PeerInfo is now ServerPeerInfo (#1065) and its constructor takes the protocol version directly. * chore: add 'Copy' macro to new PeerRequestAssociation Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com> --------- Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com>
Motivation and Context
Fixes#1064
Replaces #1044
This removes the
server_infofield which is not part of the spec:https://modelcontextprotocol.io/specification/draft/schema#discoverresult.
1044 add a synthetic server_info field; this PR removes it. This aligns with other types in this repo (using _meta as the source of truth, not a projection) and with other SDKS (Go and TypeScript do not have a
server_info) field. Instead, users can use the helper accessors.How Has This Been Tested?
Tested proxying to copilot MCP
Breaking Changes
Breaking from beta3 - the server_info field is removed
Types of changes
Checklist
Additional context