Repository navigation
mcp: refuse a call on a legacy session before initialize - #1273
Merged
guglielmo-san merged 3 commits intoSep 17, 2026
Merged
guglielmo-san merged 3 commits into
guglielmo-san merged 3 commits into
Conversation
The lifecycle spec lets a client send nothing but pings before the server has answered initialize. ServerSession.handle enforced that in the default branch of its method switch, so every method with a case of its own was served before the handshake: logging/setLevel, resources/subscribe and resources/unsubscribe share a case that only refuses new-protocol requests. Subscribe is the one that reaches state. A session that never handshook was registered as a subscriber and delivered resource-updated notifications. The check now runs after the switch, for every call that carries no new-protocol _meta, with initialize and ping exempt. A new-protocol request stays exempt because a SEP-2575 session has no initialize to wait for, and server/discover on a legacy request keeps the method-not-found answer its own case already gave it. The error is the one the default branch produced, so a client sees one refusal whichever method it sent. The tests drive handle on a fresh session: each affected method is refused before initialize, the subscribe refusal runs no handler and leaves no subscriber behind, initialize and ping are served, and a new-protocol session is served tools/list with no handshake while the removed methods keep their method-not-found error rather than the initialization refusal.
Keep the rule and the exemption, drop the archaeology. How the check came to be missed belongs to the commit that moves it, not to a comment every future reader pays for. The exemption stays because nothing on the line below says why a new-protocol request skips the gate: a SEP-2575 session has no 'initialize' to wait for, and the request itself declares the version it speaks.
jmrplens
added a commit
to jmrplens/gitlab-mcp-server
that referenced
this pull request
Sep 15, 2026
… 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.
guglielmo-san
approved these changes
Sep 17, 2026
jmrplens
added a commit
to jmrplens/go-sdk
that referenced
this pull request
Nov 2, 2026
negotiatedLegacyProtocol now reads a session that has recorded no protocol version as legacy, so the no handshake rows of TestClientSupportsMultiRoundTrip and of TestNegotiatedLegacyProtocol_NoHandshakeIsLegacy, renamed after the predicate, expect that, and its doc comment says so. That is how the notification loops read a session with no InitializeParams before this change, and how assertServerInitiatedRequestAllowed reads it. No call reaches such a session: a new-protocol call records its version before it is dispatched, and since modelcontextprotocol#1273 a legacy one is refused before initialize.
guglielmo-san
added a commit
that referenced
this pull request
Nov 3, 2026
…ion (#1266) `initialize` is deprecated in `2026-07-28`, so `negotiatedVersion` caps that handshake below it on purpose: a client asking for `2026-07-28` there is answered `2025-11-25`. `InitializeParams.ProtocolVersion` keeps whatever the client asked for, and four places that decide how to talk to a session read that value instead of the version the handshake settled on. The result is that such a session gets none of the interaction the protocol offers it. `clientSupportsMultiRoundTrip` marks the result `input_required` and attaches an `inputRequests` map, which `2025-11-25` does not define, while `assertServerInitiatedRequestAllowed` refuses `elicitation/create`, `sampling/createMessage` and `roots/list`, which are the mechanism that session does have, with an error naming a version it never got. The second function's own doc comment already describes it as a check on a session "negotiated at protocol version >= 2026-07-28". `Server.notifySessions` and `Server.ResourceUpdated` lose the same session a second way: it is not counted a legacy subscriber, so it is not notified on the shared session channel, and it never opened the `subscriptions/listen` stream the other branch delivers on, because that method does not exist in the version it negotiated. `ResourceUpdated` goes further and stamps the per-session subscription id into the notification's `_meta`, which that version does not define either. This change adds `ServerSession.protocolVersion`, which answers with `ServerSessionState.NegotiatedProtocolVersion` and falls back to the declared value when the state recorded none, and `ServerSession.negotiatedLegacyProtocol` on top of it. All four places now read the version through `protocolVersion`, three of them through `negotiatedLegacyProtocol`, so the version that chooses an interaction pattern is read in one place rather than in four copies of a condition. ## Why all four and not only the first They are one decision and they have to agree. I tried fixing `clientSupportsMultiRoundTrip` alone, and the failure gets worse rather than better: the server-side shim then calls `ServerSession.Elicit`, the second check refuses it, and a tool call that used to return a quietly ignored result returns ``` multi-round-trip: fulfilling input request "confirm": "elicitation/create" cannot be sent while serving a request on protocol version 2026-07-28: return an InputRequests map instead (multi round-trip requests, SEP-2322) ``` The two notification paths are the same decision again. I first sent this without them and offered to take them separately; they are folded in here at review's request. ## Why the fallback Since #1274, every path in the SDK that records a version records the negotiated one too: `initialize`, `server/discover`, the first call of a SEP-2575 session and the state the stateless handler synthesizes for a request carrying `MCP-Protocol-Version`. The fallback stays for state the SDK did not write itself: `ServerSessionOptions.State` is exported, so a caller may supply `InitializeParams` with no negotiated version, and `NegotiatedProtocolVersion` was added in #1199, so state persisted before that carries only the declared version. ## What a session with no recorded version is The four places disagreed about this. `clientSupportsMultiRoundTrip` defaults to `latestProtocolVersion` and reads the absence as the new protocol; the two notification loops read `InitializeParams().isNil()` and read it as legacy, as `assertServerInitiatedRequestAllowed` does. For the three places that read it, `negotiatedLegacyProtocol` settles it the second way, at review's request. `ServerSession.handle` records the declared version on the first call a new-protocol client makes, before the call is dispatched, and `server/discover` records it in its own handler, while since #1273 a legacy call before `initialize` is refused, so no call reaches `clientSupportsMultiRoundTrip` on such a session. What the reading still covers is a session that has issued no call: one that has connected and not yet sent anything, which `notifySessions` iterates because it walks every session in `s.sessions`, and which keeps receiving the notification on the session channel, as on `main`. Subscribing before the handshake does not put such a session in front of `ResourceUpdated`: since #1273, a `resources/subscribe` sent before `initialize` is refused and leaves no subscriber. ## Tests `TestMultiRoundTrip_NegotiatedDownFromNewProtocol` is the test that pins this. It drives a raw JSON-RPC client that sends `initialize` declaring `2026-07-28`, asserts the server answers `2025-11-25`, calls a tool whose handler returns an `InputRequests` map, and asserts that what comes back is an `elicitation/create` request the client can answer, followed by the completed tool result. Before the change the next message is the tool result itself, carrying `resultType: "input_required"`, and the test fails there. `TestNotifySessions_NegotiatedDownFromNewProtocol` drives the same handshake, adds a tool, and asserts the list-changed notification arrives on the session channel. Before the change it blocks until the deadline. `TestResourceUpdated_NegotiatedDownFromNewProtocol` subscribes on that session and asserts both halves: the notification arrives, and it does not carry `io.modelcontextprotocol/subscriptionId`. Before the change it fails on the second. `TestClientSupportsMultiRoundTrip` is a table over session states: no handshake, a `server/discover` session, an `initialize` session at a legacy version, an `initialize` session negotiated down, and the synthesized state of a stateless legacy request. The first and fourth rows fail before the change; the others document that nothing else moves. `TestNegotiatedLegacyProtocol_NoHandshakeIsLegacy` is the same table read from the notification side. ## What an existing user sees change A session that ran `initialize` asking for `2026-07-28` and was answered an older version is now served the legacy interaction. A handler returning `InputRequests` no longer produces an `input_required` result for it: the server fulfils the requests itself and re-invokes the handler once, which is what the SDK already does for every other client below `2026-07-28`. `ServerSession.Elicit`, `CreateMessage`, `CreateMessageWithTools` and `ListRoots` stop returning an error on such a session and send the request. It is also counted a legacy subscriber, so list-changed and resource-updated notifications reach it on the session channel, without a subscription id in `_meta`. A session that has recorded no protocol version at all is still counted legacy in the two notification loops, as on `main`, and `clientSupportsMultiRoundTrip` now reads it as legacy too instead of as the latest version, which no call can observe, as above. No other session changes. A client that reaches `2026-07-28` through `server/discover`, which is what this SDK's own client does, has had that version recorded as negotiated since #1274, and is read exactly as before. ## What I left out There is one read of the declared version I deliberately did not touch: the exported `ServerRequest.ProtocolVersion` falls back to `InitializeParams.ProtocolVersion`, which its doc comment states, and its only caller in the SDK is `Server.discover`, on the new-protocol path where the two values agree. Fixes #1258. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1271.
Server.handlerefuses a call made beforeinitializeon a legacy session, and that refusal lives in thedefaultbranch of its method switch, so any method with acaseof its own skips it. Three are served on a session that never handshook:logging/setLevel,resources/subscribeandresources/unsubscribe.resources/subscribeis the one that reaches state rather than merely answering: it starts a watcher and registers a subscriber, so a client that subscribes beforeinitializeis deliverednotifications/resources/updatedfor the lifetime of a session the server never agreed to.The check now runs after the switch, for every call carrying no new-protocol
_meta, withinitializeandpingexempt. That is the same condition thedefaultbranch already applied and the same error it produced; the change is that the three methods above stop skipping it.pingstays allowed because the lifecycle page exempts it.server/discoveris deliberately left alone: a legacy request there already receives method-not-found, which tells the client more than an initialization error would. The two notification cases stay outside, since the gate covers calls and a notification carries no refusal.The SEP-2575 path is untouched. A session without an
initializehandshake is exactly what that proposal is for, so the exemption is the request's own_meta.protocolVersionrather than anything about the session, and a new-protocol client is served as it is today.Tests:
TestServerHandle_LegacyCallBeforeInitializedrives each affected method on a fresh session and asserts the refusal, thatresources/subscriberuns no handler and leaves no subscriber, and thatinitializeandpingare served.TestServerHandle_NewProtocolCallWithoutInitializeasserts a new-protocol session with no handshake still getstools/list, and that the three methods keep their existing method-not-found rather than the initialization refusal. Without the change, the setLevel, subscribe and unsubscribe rows fail.go test ./mcp/ -count=1is green,gofmt -lis empty andgo vet ./mcp/is clean. This does not depend on #1266 and rebases onto it cleanly, verified with the suite green on the combination.One decision worth your view: the refusal is a plain error, which on the wire carries code 0, exactly as the
defaultbranch produced before this change. The specification prescribes no code for it. If you would rather it were-32600or-32603, say so and I will change it here.