Uh oh!
There was an error while loading. Please reload this page.
refactor!: extract LegacyServer/LegacyClient; NEW Server/Client compose them - #2134
refactor!: extract LegacyServer/LegacyClient; NEW Server/Client compose them#2134felixweinberger wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 65b0624 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client@modelcontextprotocol/server@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
felixweinberger
commented
May 21, 2026
@claude review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
c73c457 to
fecd8fdComparedfae0de to
f70313fComparefecd8fd to
4e002f6Comparegit mv server.ts legacyServer.ts + class rename. Zero body changes. Importers updated to `LegacyServer as Server` so behavior is identical and the rename shows as a true file rename in history.
…rver `Server` no longer extends `Protocol`. It owns the 2026 stateless dispatch path (subscriptions, statelessHandlers, _dispatchStateless, _buildDispatchServerContext, _ondiscover) and the dual-mode surface (connect/close/transport/onclose/onerror, _fanoutNotify, send*ListChanged), lifted verbatim from the sectioned `LegacyServer` with field reads re-pointed at `this.config.*` / `this._legacy.*`. `LegacyServer` keeps the session-dependent block. Both share one handler registry via `_legacy._dispatch`/`_legacy.setRequestHandler`. `server.legacy` is the explicit escape hatch. LegacyServer body changes are limited to: - `_assertSession()` guard inserted at top of createMessage / elicitInput / listRoots / sendLoggingMessage / ping - ctor: dropped `subscriptions` init + `server/discover` registration (lifted to NEW Server) Adds `SdkErrorCode.SessionRequired`.
git mv client.ts legacyClient.ts + class rename. Zero body changes. Importers updated to `LegacyClient as Client` so behavior is identical and the rename shows as a true file rename in history.
…ient `Client` no longer extends `Protocol`. It owns the 2026 stateless send path (_isStateless/_buildMeta/_withMeta/_collect/_send/_negotiate/ subscribe/_listChangedLoop) and the typed request methods (callTool/listTools/getPrompt/listPrompts/readResource/listResources/ listResourceTemplates/complete/setLoggingLevel), lifted verbatim from the sectioned `LegacyClient` with field reads re-pointed at `this.config.*` / `this._legacy.*`. `LegacyClient` keeps the session-dependent block. Both share one handler registry via `_legacy._dispatch`/`_legacy.setRequestHandler`; negotiated server state lives on `_legacy` (single source of truth) and is written by both `_negotiate` (via `_setNegotiated`) and `_initialize`. `client.legacy` is the explicit escape hatch. LegacyClient body changes are limited to: - `_assertSession()` guard inserted at top of ping / subscribeResource / unsubscribeResource / sendRootsListChanged - ctor: dropped now-unused `_enforceStrictCapabilities` / `_pendingListChangedConfig` / `_cachedToolOutputValidators` fields (lifted to NEW Client) - `_initialize` / `_setupListChangedHandler` visibility widened to @internal
Integration tests that exercise the pre-2026 session-dependent surface (server.createMessage / elicitInput / listRoots / sendLoggingMessage / ping / getClientCapabilities / getClientVersion / oninitialized / createElicitationCompletionNotifier / notification / request / setNotificationHandler; client.sendRootsListChanged / ping / subscribeResource / unsubscribeResource / request / notification) now go through `.legacy`. LegacyTestClient unchanged: it extends NEW Client and pins versions, so `connect()` skips the discover probe exactly as before. clientSend statelessClient() helper updated to set `_transport` / negotiated state on `c.legacy` (private state moved there).
f70313f to
65b0624Comparefelixweinberger
commented
May 21, 2026
@claude review |
| _setNegotiated(r: { | ||
| serverCapabilities: ServerCapabilities; | ||
| serverVersion: Implementation; | ||
| instructions: string | undefined; | ||
| protocolVersion: string; | ||
| }): void { | ||
| this._serverCapabilities = r.serverCapabilities; | ||
| this._serverVersion = r.serverVersion; | ||
| this._instructions = r.instructions; | ||
| this._negotiatedProtocolVersion = r.protocolVersion; | ||
| this._isStatelessConnection = true; | ||
| } |
There was a problem hiding this comment.
🔴 LegacyClient._isStatelessConnection is set to true in _setNegotiated() but is never reset to false — not in Protocol.close(), not in a LegacyClient.close() override, and not in _initialize(). After connect(2026 server) → close() → connect(pre-2026 server), the legacy session is fully established but _assertSession() still sees the stale flag and client.legacy.ping() / subscribeResource() / unsubscribeResource() / sendRootsListChanged() incorrectly throw SdkError(SessionRequired). Reset _isStatelessConnection = false in _initialize() (where a legacy session is established) or have Client.close() reach in and reset it, mirroring how Client.close() already resets its own _isStateless.
Extended reasoning...
What the bug is
_isStatelessConnection was added in commit 8e502fd (in response to earlier review feedback) so that _assertSession() would fire on a 2026-stateless connection, not just when no transport is connected. It is set to true in LegacyClient._setNegotiated() (legacyClient.ts:252) when Client._negotiate() succeeds against a 2026 server, but there is no code path that ever resets it back to false. The only references to the field are: the declaration = false at line 200, the = true assignment at line 252, and the read at line 271 in _assertSession().
The code path that triggers it
Client is explicitly designed to be reusable after close() — Client.close() resets this._isStateless = false and tears down _listChangedAbort precisely so a subsequent connect() starts from a clean slate. But that reset only covers the composing Client's own flag; the symmetric reset on LegacyClient._isStatelessConnection is missing. Protocol._onclose() only nulls _transport; it does not touch subclass fields. LegacyClient._initialize() (the legacy handshake that runs on a non-stateless reconnect) sets _serverCapabilities, _serverVersion, _negotiatedProtocolVersion, and _instructions, but does not reset _isStatelessConnection.
Step-by-step proof
client.connect(transport1)against a 2026-stateless server →_negotiate()succeeds →_setNegotiated()setsLegacyClient._isStatelessConnection = true.await client.close()→Client._isStateless = false;Protocol._onclose()clears_transport;_isStatelessConnectionstaystrue.client.connect(transport2)against a pre-2026 server →_negotiate()falls through (no mutual stateless version) →_legacy._initialize()runs the legacyinitializehandshake successfully and sets capabilities/version, but does not reset the flag.await client.legacy.ping()→_assertSession('ping')evaluates(!this.transport || this._isStatelessConnection)→(false || true)→true→ throwsSdkError(SdkErrorCode.SessionRequired, 'LegacyClient.ping requires a connected pre-2026 session…')even though a valid pre-2026 session is connected. The same applies tosubscribeResource(),unsubscribeResource(), andsendRootsListChanged().
Why nothing else catches it
The Protocol.request() path's own NotConnected check would pass (transport is set), so _assertSession() is the only guard — and it's now reporting a false positive. The composed pair is left in an inconsistent state: Client._isStateless says "not stateless", LegacyClient._isStatelessConnection says "stateless", and both are reachable from public surface.
Impact
Any application that reconnects a single Client instance across protocol eras — e.g. a tool that probes a list of servers sequentially, or one that retries against an older fallback endpoint after closing — will get a misleading SessionRequired error pointing at the migration docs when the user is in fact correctly connected to a pre-2026 server. The error message tells them to migrate when no migration is needed.
How to fix
Reset _isStatelessConnection = false in LegacyClient._initialize() (where a legacy session is established and the flag should be cleared regardless of how it got set), or override close() on LegacyClient, or have Client.close() reach into _legacy to reset it alongside Client._isStateless. The first option is the most robust since it makes the legacy handshake self-correcting.
| | Removed from `Server`/`Client` | Moved to | | ||
| | --- | --- | | ||
| | `server.createMessage(...)` / `elicitInput` / `listRoots` / `sendLoggingMessage` / `ping` | `server.legacy.*` | | ||
| | `client.ping()` / `subscribeResource` / `unsubscribeResource` / `sendRootsListChanged` | `client.legacy.*` | | ||
| | `request(...)` / `notification(...)` (raw `Protocol` methods) | `server.legacy.*` / `client.legacy.*` | |
There was a problem hiding this comment.
🔴 The new "Removed from Server/Client / Moved to" migration table omits several public Server members that this PR also removes (the PR's own tests/examples had to migrate them to server.legacy.*): oninitialized, getClientCapabilities(), getClientVersion(), createElicitationCompletionNotifier(), and setNotificationHandler/fallbackNotificationHandler. Add the missing rows, fix the now-stale server.setNotificationHandler('notifications/message', …) v2 example at docs/migration.md:~363, and update the Server.legacy escape-hatch JSDoc (which currently lists only createMessage/elicitInput/listRoots/ping/sendLoggingMessage/oninitialized).
Extended reasoning...
What is incomplete
The migration table added by this PR at docs/migration.md:906-910 is the structured "what moved where" reference users land on when their build breaks. It currently lists:
server.createMessage / elicitInput / listRoots / sendLoggingMessage / ping→server.legacy.*client.ping / subscribeResource / unsubscribeResource / sendRootsListChanged→client.legacy.*request() / notification()→*.legacy.*
But the new Server class (packages/server/src/server/server.ts) also no longer exposes the following public members that existed on the old Server (declared on the class or inherited from Protocol), all of which now live only on LegacyServer:
oninitialized(legacyServer.ts:259)getClientCapabilities()(legacyServer.ts:598)getClientVersion()(legacyServer.ts:607)createElicitationCompletionNotifier()(legacyServer.ts:711)setNotificationHandlerandfallbackNotificationHandler(inherited fromProtocol; the newServerdelegates onlysetRequestHandler/removeRequestHandler/assertCanSetRequestHandler/fallbackRequestHandler/registerCapabilities/getCapabilities)
None of these appear in the migration table.
Proof from this PR's own diff
The PR's tests and examples were forced to migrate every one of these to server.legacy.*:
test/integration/test/server.test.ts:54—server.setNotificationHandler('notifications/initialized', …)→server.legacy.setNotificationHandler(...)test/integration/test/server.test.ts:312andstatelessAcceptance.test.ts:341-344—server.getClientCapabilities()/server.getClientVersion()→server.legacy.*test/integration/test/server.test.ts:843/893/930andexamples/server/src/elicitationUrlExample.ts:56/92/586-587—server.createElicitationCompletionNotifier(...)→server.legacy.createElicitationCompletionNotifier(...)
Each of those is a compile error on a real call site that a user will hit and then turn to the migration table for guidance — and find none.
A second, contradicting doc-vs-code spot
docs/migration.md:~363 (the "setRequestHandler and setNotificationHandler use method strings" section) still shows the v2 example as:
server.setNotificationHandler('notifications/message',notification=>{console.log(notification.params.data);});After this PR that does not compile — Server has no setNotificationHandler (only Client retains a delegating wrapper). The section header at line ~330 still reads "…on Client, Server, and Protocol", which is also no longer accurate for Server.
On the Client/Server asymmetry (addressing the refutation)
A refutation argued the asymmetry itself is intentional: under the 2026 stateless model the server never receives client notifications, so server-side notification handlers are inherently a pre-2026 concept and correctly live only on LegacyServer, whereas Client receives server notifications under both eras and therefore keeps a top-level setNotificationHandler wrapper. That is a fair design call and this comment does not dispute it. The remaining issue is purely documentation: an intentional removal still has to be documented, and right now the migration guide both omits the move and shows a code sample that no longer compiles.
Why nothing else surfaces it
These are tsc errors, not runtime failures, so users hit them immediately on upgrade — but the migration guide is the only structured reference for "X moved to Y," and docs/migration-SKILL.md (consumed by automated migration tooling) mirrors the same incomplete table. The Server.legacy JSDoc (server.ts, the get legacy() accessor) lists only createMessage/elicitInput/listRoots/ping/sendLoggingMessage/oninitialized, so a user reading the in-editor hover also won't see getClientCapabilities, getClientVersion, createElicitationCompletionNotifier, or setNotificationHandler.
How to fix
- Add rows to the "Removed from Server/Client / Moved to" table at
docs/migration.md:906-910for:server.oninitialized,server.getClientCapabilities(),server.getClientVersion(),server.createElicitationCompletionNotifier(), andserver.setNotificationHandler/server.fallbackNotificationHandler(server-side only — note thatClientretains delegating wrappers for the latter two). Mirror the same additions indocs/migration-SKILL.md. - Update the v2 example at
docs/migration.md:~363toserver.legacy.setNotificationHandler('notifications/message', ...)(or note the server/client asymmetry inline) and adjust the line ~330 prose about "onClient,Server, andProtocol". - Update the
Server.legacyJSDoc to list the full set of session-dependent members it gates.
Extracts
LegacyServer/LegacyClient(theProtocol-derived, session-stateful classes) and introduces NEWServer/Clientthat own the 2026 stateless path and compose the legacy class via.legacy.Motivation and Context
After #2131 the sectioned
Server/Clientcarry both the 2026 stateless surface and the pre-2026 session-dependent surface side by side. That works, but nothing in the type system tells a caller which methods are session-only. This PR makes the boundary structural:Server/Clientno longer extendProtocol. They expose the 2026 stateless surface plus the version-agnostic typed request methods.createMessage/elicitInput/listRoots/sendLoggingMessage/pingon the server;subscribeResource/unsubscribeResource/sendRootsListChanged/pingon the client; rawrequest/notification/setNotificationHandleron both) live only onLegacyServer/LegacyClient, reachable via the explicit.legacyescape hatch._assertSession()guards the legacy methods at runtime and throwsSdkErrorCode.SessionRequiredwith a migration URL when called on a 2026 connection.Both classes share one handler registry and negotiated state (single source of truth on the legacy instance), so behavior is identical to the sectioned base; this PR just moves the seam into the type system.
How Has This Been Tested?
pnpm typecheck:all && lint:fix:all && build:all && test:all && docs:checkall green. Integration tests updated to call session-dependent methods through.legacy.Breaking Changes
Server/Clientno longer extendProtocol. Session-dependent methods move to.legacy.*. Seedocs/migration.md.Types of changes
Checklist
Additional context
Reviewable per-commit:
refactor(server): rename Server -> LegacyServer (verbatim)—git mv+ class rename, zero body changesrefactor(server)!: NEW Server class (Protocol-free) composes LegacyServerrefactor(client): rename Client -> LegacyClient (verbatim)—git mv+ class rename, zero body changesrefactor(client)!: NEW Client class (Protocol-free) composes LegacyClienttest: adapt tests to .legacy getter for session-dependent methods