-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(core): export InMemoryTransport, tighten migration docs #1834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,36 @@ const transport = new NodeStreamableHTTPServerTransport({ sessionIdGenerator: () | |
|
|
||
| The SSE transport has been removed from the server. Servers should migrate to Streamable HTTP. The client-side SSE transport remains available for connecting to legacy SSE servers. | ||
|
|
||
| **Before (v1):** | ||
|
|
||
| ```typescript | ||
| import { SSEServerTransport } from '@modelcontextprotocol/sdk/server/sse.js'; | ||
|
|
||
| let transport: SSEServerTransport; | ||
|
|
||
| app.get('/sse', async (req, res) => { | ||
| transport = new SSEServerTransport('/messages', res); | ||
| await server.connect(transport); | ||
| }); | ||
| app.post('/messages', async (req, res) => { | ||
| await transport.handlePostMessage(req, res); | ||
| }); | ||
| ``` | ||
|
|
||
| **After (v2, stateless):** | ||
|
|
||
| ```typescript | ||
| import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node'; | ||
|
|
||
| app.all('/mcp', async (req, res) => { | ||
| const transport = new NodeStreamableHTTPServerTransport({ sessionIdGenerator: undefined }); | ||
| await server.connect(transport); | ||
| await transport.handleRequest(req, res); | ||
| }); | ||
| ``` | ||
|
|
||
| With `sessionIdGenerator: undefined` the transport runs in stateless mode, so creating a fresh instance per request is correct. For stateful sessions, the transport must be created once per session and stored in a `Map<string, NodeStreamableHTTPServerTransport>` keyed by session ID — see `examples/server/src/simpleStreamableHttp.ts` for the full pattern. | ||
|
|
||
| ### `WebSocketClientTransport` removed | ||
|
|
||
| `WebSocketClientTransport` has been removed. WebSocket is not a spec-defined MCP transport, and keeping it in the SDK encouraged transport proliferation without a conformance baseline. | ||
|
|
@@ -381,10 +411,14 @@ Common method string replacements: | |
| | `ToolListChangedNotificationSchema` | `'notifications/tools/list_changed'` | | ||
| | `ResourceListChangedNotificationSchema` | `'notifications/resources/list_changed'` | | ||
| | `PromptListChangedNotificationSchema` | `'notifications/prompts/list_changed'` | | ||
| | `GetTaskRequestSchema` | `'tasks/get'` | | ||
| | `GetTaskPayloadRequestSchema` | `'tasks/result'` | | ||
| | `ElicitationCompleteNotificationSchema` | `'notifications/elicitation/complete'` | | ||
| | `InitializedNotificationSchema` | `'notifications/initialized'` | | ||
|
|
||
| ### `Protocol.request()`, `ctx.mcpReq.send()`, and `Client.callTool()` no longer take a schema parameter | ||
|
|
||
| The public `Protocol.request()`, `BaseContext.mcpReq.send()`, and `Client.callTool()` methods no longer accept a Zod result schema argument. The SDK now resolves the correct result schema internally based on the method name. This means you no longer need to import result schemas | ||
| The public `Protocol.request()`, `BaseContext.mcpReq.send()`, `Client.callTool()`, and `client.experimental.tasks.callToolStream()` methods no longer accept a Zod result schema argument. The SDK now resolves the correct result schema internally based on the method name. This means you no longer need to import result schemas | ||
| like `CallToolResultSchema` or `ElicitResultSchema` when making requests. | ||
|
|
||
| **`client.request()` — Before (v1):** | ||
|
Comment on lines
411
to
424
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 The setRequestHandler method-string table in migration.md is missing the InitializedNotificationSchema -> notifications/initialized row that already exists in migration-SKILL.md. This is a pre-existing omission, but the PR edits the rows immediately before where this entry should appear, making it a natural opportunity to add it. Extended reasoning...What the bug is The method-string mapping table in migration.md (the human-facing migration guide) ends at ElicitationCompleteNotificationSchema without including InitializedNotificationSchema -> notifications/initialized. The companion migration-SKILL.md (the LLM-optimized reference) includes this row at the end of the same table. This inconsistency means developers relying solely on migration.md have no documented mapping for InitializedNotificationSchema. The specific code path The PR adds three new rows to migration.md's table: GetTaskRequestSchema -> tasks/get, GetTaskPayloadRequestSchema -> tasks/result, and ElicitationCompleteNotificationSchema -> notifications/elicitation/complete. In migration-SKILL.md, these same three rows are inserted before the existing InitializedNotificationSchema -> notifications/initialized row. In migration.md, the table stops after ElicitationCompleteNotificationSchema with no InitializedNotificationSchema entry, leaving the table incomplete relative to the SKILL doc. Why existing code does not prevent it Both files are maintained manually. There is no automated test or lint rule that cross-checks the two tables for completeness. The omission in migration.md predates this PR — migration.md intentionally has a smaller table while migration-SKILL.md is a comprehensive LLM-optimized reference — but the omission of InitializedNotificationSchema is a gap rather than an intentional exclusion, since the schema is a core part of the MCP protocol initialization handshake. Impact A developer using migration.md as their reference when migrating setNotificationHandler(InitializedNotificationSchema, ...) calls will find no mapping and may guess the wrong method string, or fall back to grepping the codebase. The correct string (notifications/initialized) is confirmed in packages/core/src/types/schemas.ts and is present in migration-SKILL.md line 379. Step-by-step proof
How to fix Add the missing row to migration.md's table after ElicitationCompleteNotificationSchema: | InitializedNotificationSchema | 'notifications/initialized' | Pre-existing note This omission predates the PR. However, since the PR directly edits the rows immediately before where InitializedNotificationSchema should appear, this is a natural and easy fix to include.
Comment on lines
411
to
424
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The PR adds Extended reasoning...What the bug isThe PR correctly updates the prose in The specific code paths affected
Why existing tooling does not prevent itBoth documents are maintained manually. There is no automated test, lint rule, or CI check that cross-validates prose mentions against table entries within a section, or that verifies migration-SKILL.md and migration.md stay in sync. The authoring error therefore goes undetected. ImpactAny developer (or LLM agent using migration-SKILL.md) migrating code that calls
Because callToolStream is an experimental API the blast radius is limited, and the prose does cover the intent, making this a nit rather than a blocking issue. Step-by-step proof
How to fixIn In |
||
|
|
@@ -440,6 +474,24 @@ const result = await client.callTool({ name: 'my-tool', arguments: {} }, Compati | |
| const result = await client.callTool({ name: 'my-tool', arguments: {} }); | ||
| ``` | ||
|
|
||
| **`client.experimental.tasks.callToolStream()` — Before (v1):** | ||
|
|
||
| ```typescript | ||
| import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; | ||
|
|
||
| for await (const event of client.experimental.tasks.callToolStream({ name: 'my-tool', arguments: {} }, CallToolResultSchema)) { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| **After (v2):** | ||
|
|
||
| ```typescript | ||
| for await (const event of client.experimental.tasks.callToolStream({ name: 'my-tool', arguments: {} })) { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| The return type is now inferred from the method name via `ResultTypeMap`. For example, `client.request({ method: 'tools/call', ... })` returns `Promise<CallToolResult | CreateTaskResult>`. | ||
|
|
||
| ### Client list methods return empty results for missing capabilities | ||
|
|
@@ -457,21 +509,19 @@ const client = new Client( | |
| ); | ||
| ``` | ||
|
|
||
| ### `InMemoryTransport` removed from public API | ||
|
|
||
| `InMemoryTransport` has been removed from the public API surface. It was previously used for in-process client-server connections and testing. | ||
| ### `InMemoryTransport` import path | ||
|
|
||
| For **testing**, import it directly from the internal core package: | ||
| `InMemoryTransport` is used for testing client/server interactions within a single process. It is re-exported from both `@modelcontextprotocol/server` and `@modelcontextprotocol/client`. | ||
|
|
||
| ```typescript | ||
| // v1 | ||
| import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'; | ||
|
|
||
| // v2 (testing only — @modelcontextprotocol/core is internal, not for production use) | ||
| import { InMemoryTransport } from '@modelcontextprotocol/core'; | ||
| // v2 | ||
| import { InMemoryTransport } from '@modelcontextprotocol/server'; | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ``` | ||
|
Comment on lines
511
to
522
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The section heading Extended reasoning...What the bug isThe PR rewrites the body of the InMemoryTransport migration section to reflect that the transport is now part of the public v2 API surface, but leaves the section heading unchanged as InMemoryTransport removed from public API. This creates a direct, visible contradiction within the same section. The specific code pathAt docs/migration.md line 492, the heading reads InMemoryTransport removed from public API. The very next paragraph after the PR now reads: InMemoryTransport is intended for testing client/server interactions within a single process. It is re-exported from both @modelcontextprotocol/server and @modelcontextprotocol/client. The code block that follows shows import from @modelcontextprotocol/server - a public package - directly contradicting the heading claim of removal. Why existing content does not prevent thisBefore the PR, the section was internally consistent: the heading said removed from public API and the body explained that developers must import from the internal @modelcontextprotocol/core package for testing only. The PR updated the body text to reflect the new reality (the export was added to packages/core/src/exports/public/index.ts), but the heading was simply left unchanged - a straightforward editing oversight that no automated tooling would catch in markdown files. ImpactA developer reading the migration guide to determine whether they can use InMemoryTransport from a public package in their v2 test code encounters conflicting information. The heading suggests they cannot; the body says they can. This is especially confusing in a migration guide where the reader is actively trying to understand API availability changes. How to fixUpdate the heading to accurately reflect the migration story, for example: InMemoryTransport import path changed, or InMemoryTransport moved to @modelcontextprotocol/server. Step-by-step proof
|
||
|
|
||
| For **production in-process connections**, use `StreamableHTTPClientTransport` with a local server URL, or connect client and server via paired streams. | ||
| For **production in-process connections**, prefer `StreamableHTTPClientTransport` with a local server URL. | ||
|
|
||
| ### Removed type aliases and deprecated exports | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The new "After (v2)" example in the SSE migration section creates a fresh NodeStreamableHTTPServerTransport on every request while passing sessionIdGenerator: () => randomUUID() (stateful mode) -- but each new transport instance starts with sessionId === undefined, so every follow-up MCP request after the initial initialize POST fails with HTTP 400 "Server not initialized" or HTTP 404 "Session not found". Additionally, randomUUID is called without being imported from node:crypto, causing a ReferenceError at runtime. The correct stateless pattern uses sessionIdGenerator: undefined; for stateful multi-client use, see examples/server/src/simpleStreamableHttp.ts which maintains a session Map.
Extended reasoning...
Bug 1: Per-request transport creation breaks stateful sessions
The example passes sessionIdGenerator: () => randomUUID() to NodeStreamableHTTPServerTransport. This puts the transport into stateful mode: the first initialize POST causes the transport to invoke this.sessionIdGenerator(), storing e.g. "abc123" in this.sessionId, and returns that value in the Mcp-Session-Id response header. The client then echoes Mcp-Session-Id: abc123 on every subsequent request.
However, the example creates a brand-new transport instance on every request. When the second request arrives with header Mcp-Session-Id: abc123, a fresh transport is constructed whose sessionId field is still undefined and _initialized is false. Inside WebStandardStreamableHTTPServerTransport.handleRequest (packages/server/src/server/streamableHttp.ts ~line 846), validateSession checks !this._initialized first and returns HTTP 400 "Bad Request: Server not initialized" (or checks sessionId !== this.sessionId giving "abc123" !== undefined -> HTTP 404 "Session not found"). Either way, every follow-up request fails.
Step-by-step proof:
Bug 2: Missing randomUUID import
randomUUID is called as a bare identifier but it is NOT a global in Node.js. It must be imported:
The actual working example file examples/server/src/simpleStreamableHttp.ts (line 1) has exactly this import. Without it, the very first request to /mcp throws ReferenceError: randomUUID is not defined -- before the transport architecture issue even has a chance to manifest.
Why existing tooling does not catch this
Markdown code blocks are not type-checked or linted by the repository CI. There is no compilation step applied to documentation snippets, so both the architectural error and the missing import go undetected automatically.
Impact
This is the migration guide's primary SSE->StreamableHTTP example, referenced at the exact point where developers are actively porting their v1 servers. A developer copying this snippet verbatim will: (a) immediately hit ReferenceError: randomUUID is not defined, and (b) once they add the import, find that MCP initialization appears to succeed but every subsequent request (tools/list, tools/call, etc.) fails with HTTP 400/404 -- a difficult-to-diagnose failure that makes it appear the transport itself is broken.
How to fix
For a stateless server (simplest correct pattern):
For a stateful multi-client server, follow the pattern in examples/server/src/simpleStreamableHttp.ts: create the transport once per session during the initialize request, store it in a Map keyed by session ID, and route subsequent requests to the existing transport -- never create a new transport instance for a non-init request.