mcp: record the negotiated protocol version on every path that records one - #1274
Conversation
… entry (#784) ## What this does Records three upstream contributions this record did not have, and corrects one it had wrong. ## The correction Entry 46, the auto-merge cancellation, said "!255239, open". That merge request is closed unmerged and the work is now two others, so the record was stating something false about the state of an upstream contribution, which is the one thing this file exists to get right. `!255239` changed the handler so its response matched the documentation. @phikai answered that this is a breaking change whichever way it is argued, since it alters the response of a stable endpoint, and @marc_shaw proposed the shape actually taken: leave the old endpoint behaving exactly as it does, deprecate it in the documentation, and add `cancel_auto_merge` under current naming. That is !255702 and !255704. His reason is recorded with it, because it applies to every future contribution of this shape and is written nowhere in the code: "we basically can't deprecate our API, by introducing another endpoint, we are now maintaining the old and the new". It is why the deprecation is a documentation notice rather than an entry in `doc/api/rest/deprecations.md`, which promises removals, and why a symmetric `add_to_auto_merge` was declined in the same message. ## The two new entries Reviewing the MRTR pull request turned up two go-sdk defects that are not about MRTR: - **49** — three methods served on a legacy session before the handshake ([#1271](modelcontextprotocol/go-sdk#1271), fixed by [#1273](modelcontextprotocol/go-sdk#1273)). The gate sat in the `default` branch of the method switch, so every method with a `case` of its own escaped it. `resources/subscribe` is the one that matters: it starts a watcher and registers a subscriber, so a client that subscribes before `initialize` is delivered updates for the lifetime of a session the server never agreed to. It reaches our surface, since ADR-0015 makes the first read the authorization check. - **50** — the negotiated protocol version recorded on one code path of four ([#1272](modelcontextprotocol/go-sdk#1272), fixed by [#1274](modelcontextprotocol/go-sdk#1274)). This one names the transport we lead with: `ioConn.sessionUpdated` reads only `NegotiatedProtocolVersion`, so a SEP-2575 session over **stdio** is read as `2025-03-26` and accepts JSON-RPC batches, which `2025-06-18` removed and which the streamable handler already refuses. Two transports disagreeing about the same session shape is worth writing down whether or not the fix lands upstream. ## Three findings kept because they are rules, not details Each entry keeps the reasoning that outlives the change it came from. On the GitLab side: the old endpoint's own request spec never arms an auto-merge and cannot notice, because its fixture is already mergeable with no pipeline in progress, so `availability_details` errors and `auto_merge_enabled` stays false while the endpoint answers `201` unconditionally and the spec asserts only `:created`. That spec would pass with an empty handler, which is the second reason the defect survived. The false contract was also published in the CI-gated `doc/api/openapi/openapi_v3.yaml`, and the `406` in the `desc` failure list can never fire, since `not_acceptable!` is called in exactly one place in the codebase. On the go-sdk side: why widening the initialization gate from calls to notifications was declined, since `notifications/cancelled` has no case of its own and a client giving up on a slow `initialize` would have its cancellation refused; and why validating against the session's version list refuses `server/discover` on a stateful session, which a test catches and which the obvious reading of the code does not. ## Verification ``` npx markdownlint-cli2 docs/development/upstream-bugs.md # clean go run ./cmd/format_md_tables/ --check # up to date go run ./cmd/audit_doc_tool_names/ --check # clean ``` Both new table rows were checked to resolve against the headings they link to.
| if slices.ContainsFunc(versions, func(v string) bool { return v >= protocolVersion20260728 }) { | ||
| req.Session.updateState(func(state *ServerSessionState) { | ||
| state.InitializeParams = init | ||
| // A client keeps speaking the version it asked about when the | ||
| // answer lists it, so that is the version this session negotiated. | ||
| // One whose version is not listed picks another from the list on | ||
| // its next call, which nothing here can predict, so only the | ||
| // declared version is recorded for it. | ||
| if slices.Contains(versions, init.ProtocolVersion) { | ||
| state.NegotiatedProtocolVersion = init.ProtocolVersion | ||
| } |
There was a problem hiding this comment.
what do you think about
| if slices.ContainsFunc(versions, func(v string) bool { return v >= protocolVersion20260728 }) { | |
| req.Session.updateState(func(state *ServerSessionState) { | |
| state.InitializeParams = init | |
| // A client keeps speaking the version it asked about when the | |
| // answer lists it, so that is the version this session negotiated. | |
| // One whose version is not listed picks another from the list on | |
| // its next call, which nothing here can predict, so only the | |
| // declared version is recorded for it. | |
| if slices.Contains(versions, init.ProtocolVersion) { | |
| state.NegotiatedProtocolVersion = init.ProtocolVersion | |
| } | |
| if slices.Contains(versions, init.ProtocolVersion) { | |
| req.Session.updateState(func(state *ServerSessionState) { | |
| state.InitializeParams = init | |
| // A client keeps speaking the version it asked about when the | |
| // answer lists it, so that is the version this session negotiated. | |
| // One whose version is not listed picks another from the list on | |
| // its next call, which nothing here can predict, so only the | |
| // declared version is recorded for it. | |
| state.NegotiatedProtocolVersion = init.ProtocolVersion | |
| } | |
| } |
If a request reaches this point, for sure the protocol version requested is >=20260728 due to the check in the handle() func
case methodDiscover:
// In case of methodDiscover call the state.initializeParams is populated
// within the discover handle function to make sure the method is supported
// when the user is probing a pre-2026-07-28 server.
if !validatedMeta.usesNewProtocol {
return nil, &jsonrpc.Error{
Code: jsonrpc.CodeMethodNotFound,
Message: fmt.Sprintf("%q is only supported in protocol version >= %s", req.Method, protocolVersion20260728),
}
}
There was a problem hiding this comment.
I went to check this properly and came back agreeing with you more than when I started, so let me separate what holds from what does not.
The handle() check does guarantee the client asked for a version >= 20260728. But that is a different quantity from the one the guard tests: the guard asks ContainsFunc(versions, v >= protocolVersion20260728), which is whether this transport serves any new version, not whether it serves the client's. So the premise is true and does not by itself license replacing the guard.
What does license it is that your form implies the guard. If the client's version is in versions, then versions contains a version >= 20260728, so the leak the guard exists for stays prevented: the stateful StreamableHTTPHandler case where a discover request creates a session never surfaced through Mcp-Session-Id.
The behaviour it changes is narrower than I first thought, and it may well be an improvement. A client declaring a version >= 20260728 that this transport does not serve keeps its InitializeParams today and would keep nothing under your form. Following that through:
- The MRTR predicate is unaffected.
protocolVersion()falls back toInitializeParams.ProtocolVersionand returns""when there is none, and the predicate already answers""as "not legacy", so such a session is served the same pattern either way. - The elicitation capability check reads
iparams.Capabilities.Elicitationand would refuse for a nil, which is the right answer for a session this transport cannot serve. serveStatefulPOST's safety net keys onInitializeParams() == nil, so under your form that session gets closed rather than kept. For a client whose version we do not serve, closing looks more correct than holding a session it can never use.
So the honest summary is that your version is simpler, keeps the property the guard was written for, and turns an unservable session into a closed one instead of a half-recorded one. I am happy to take it.
One request, and it is only about shape: I would rather it came as its own commit with a test naming the case, a client declaring a supported-by-spec but unserved version, than folded into this one, because this pull request is about recording the negotiated version on the paths that record one and that is a change to what gets recorded at all. If you would rather see it here, say so and I will add it here instead.
There was a problem hiding this comment.
yes add it here with a comment explaining the logic behind
There was a problem hiding this comment.
Done in 7930405, folded into the single commit and rebased onto main (the conflict was only the tests #1273 added beside these).
discover now records both fields together, and only when its answer lists the version the client declared, with the comment saying why: a client whose version is not listed picks another on its next call, and a nil InitializeParams is what lets serveStatefulPOST close a stateful session the client was never told about. TestServerDiscover_UnservedVersionRecordsNothing names that case with a transport that filters 2026-07-28 out, the way a stateful StreamableHTTPHandler does: the answer omits the version and nothing is recorded. The listed case stays covered by TestServerDiscover_RecordsNegotiatedVersion, and the commit body carries the reasoning.
…s one Only the initialize handshake wrote NegotiatedProtocolVersion. The three other paths that record a session's version, the first new-protocol call in ServerSession.handle, server/discover, and the state the streamable handler synthesizes from MCP-Protocol-Version, wrote InitializeParams alone, so every reader of the negotiated field saw a SEP-2575 session as having no version. The stdio connection reads nothing else: it took such a session for 2025-03-26 and accepted the JSON-RPC batches that 2025-06-18 and later forbid, which the streamable handler already refuses from the header. Each path now records the version it serves under. handle records the declared version once the check against the server's supported list has accepted it, which is the only negotiation SEP-2575 has: a version the server does not speak is refused with the list it does. It is not put through negotiatedVersion, which serves the deprecated handshake and caps its answer below 2026-07-28; that would downgrade every new-protocol session with no handshake response to tell the client. discover records InitializeParams and the negotiated version together, and only when its answer lists the version the client declared, since such a client keeps speaking it; one whose version is not listed picks another on its next call, so nothing is recorded for it. That replaces a guard which asked whether the transport served any new version rather than the client's, and keeps what it was for: a stateful streamable session a discover request created is never surfaced through Mcp-Session-Id, and a nil InitializeParams is what lets serveStatefulPOST close it. The synthesized state records the header, which the transport spec defines as the version negotiated earlier, or the 2025-03-26 it has a server assume without one. The field stays empty in state a caller supplies with InitializeParams alone and in state persisted before it existed, so a reader that needs the version a session speaks still has the declared one to fall back on.
5b7ebed to
7930405
Compare
2 participants
Closes #1272. Implements what you asked in #1266: "what do you think about setting the
ss.state.NegotiatedProtocolVersionalso in case of new protocol version?"Four places record a protocol version and only
ServerSession.initializerecorded it as negotiated. The other three recordInitializeParamsalone:Server.handlefrom a new-protocol client's_meta,server/discoverfrom the version it was asked about, and the streamable handler from theMCP-Protocol-Versionheader. All three now record the negotiated field too.The concrete failure this closes is on stdio.
ioConn.sessionUpdatedreads onlyNegotiatedProtocolVersion, so a SEP-2575 session over stdio was treated as2025-03-26and accepted JSON-RPC batches, which2025-06-18removed. The streamable handler already refuses them by reading the header, so the two transports disagreed about the same session shape. Recording the version on the_metapath settles it.Why recording it as negotiated is accurate on those paths rather than a rename of a declared value: the support check each already applies is all the negotiation SEP-2575 has. There is no handshake response in which a downgrade could be told to the client, so a version those paths accept is one the server supports.
handlerefuses a_metaversion outsidess.server.protocolVersionswithCodeUnsupportedProtocolVersionbefore recording anything,discoverrecords only a version its own answer lists, and the streamable header is what the transport page defines as the version negotiated earlier.initializeremains the one path that downgrades, as the lifecycle page requires, with the result visible inInitializeResult.ProtocolVersion.One deliberate non-refusal: a request with no header at all, on a server that narrowed out
2025-03-26, is still served as2025-03-26. The transport page says a server SHOULD assume that version when the header is absent, and nothing was declared to refuse.ServerSessionState.NegotiatedProtocolVersionis exported, so its doc comment is rewritten to say what it now means on each path. The change is additive in JSON and is not breaking for any client that works today, with one exception worth stating plainly: a SEP-2575 client that sends JSON-RPC batches over stdio is now refused, which is the defect being fixed.Tests:
TestServerHandle_RecordsNegotiatedVersionOnNewProtocolCall,TestServerDiscover_RecordsNegotiatedVersion, and the old-protocol row ofTestEphemeralConnectOpts. All three fail without the change.go test ./mcp/ -count=1is green,gofmt -lempty,go vet ./...clean. Based on main, so it does not depend on #1266, and it rebases onto it cleanly with the suite green on the combination.If you would rather have this inside #1266, since it answers a question raised there, say so and I will fold it in and close this. It carries one small follow-up either way: #1266's
protocolVersion()doc says a SEP-2575 session carries its version inInitializeParamsalone, which stops being true once this lands. The fallback itself stays, for a caller-suppliedServerSessionOptions.Stateand for state persisted before #1199.