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
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
2 participants
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.