fix(core): export InMemoryTransport, tighten migration docs#1834
fix(core): export InMemoryTransport, tighten migration docs#1834felixweinberger wants to merge 2 commits intomainfrom
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/migration-doc-fixes
branch
from
32d24b0 to
5ecd8b9
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/migration-doc-fixes
branch
from
5ecd8b9 to
9a350f9
Compare
There was a problem hiding this comment.
Two of my prior inline comments were incorrect and should be disregarded: the InMemoryTransport comment at line 502 (the PR correctly adds it to the public barrel, making the docs claim valid) and the transport-scope comment at line 135 (the v1 Before example correctly uses let at module scope). The real issues to address are in the After (v2) SSE example — see the new inline comments for details.
Extended reasoning...
Overview
This PR touches three files: docs/migration-SKILL.md, docs/migration.md, and packages/core/src/exports/public/index.ts. The core code change is adding InMemoryTransport to the public barrel export. The doc changes fill gaps in the migration guide: fixing the core package description, adding an SSE to StreamableHTTP example, adding missing schema-to-method-string table rows, and noting callToolStream() drops the schema parameter.
Prior review corrections
Both of my previous inline comments were based on incorrect analysis:
- Line 502 (InMemoryTransport): I claimed the export did not exist, but the PR adds it to
packages/core/src/exports/public/index.ts, which is re-exported by both@modelcontextprotocol/serverand@modelcontextprotocol/client. The docs are correct after this PR. - Line 135 (transport scope): I claimed
transportwas declared asconstinside the GET callback, but the v1 Before example declareslet transport: SSEServerTransport;at module scope. The example is correct.
Real issues (new inline comments)
The After (v2) SSE example at lines 131-139 has two real problems: randomUUID is called without an import from node:crypto (runtime ReferenceError on first request), and creating a new NodeStreamableHTTPServerTransport per request with sessionIdGenerator: () => randomUUID() breaks session continuity because each fresh transport instance starts with sessionId === undefined, causing all follow-up requests to fail with HTTP 400/404. The new inline comments on this PR capture these issues correctly.
Security risks
None -- this is a documentation and public export change with no security-sensitive logic.
Level of scrutiny
The InMemoryTransport export addition is a small, appropriate API surface change. Most doc fixes are correct. The SSE example needs the missing import and a corrected transport pattern before merging.
| import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node'; | ||
|
|
||
| app.all('/mcp', async (req, res) => { | ||
| const transport = new NodeStreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID() }); | ||
| await server.connect(transport); | ||
| await transport.handleRequest(req, res); | ||
| }); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🔴 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:
- Client POSTs {"method": "initialize"} -> handler fires, creates transport1, calls server.connect(transport1), transport1 sets sessionId = "abc123", _initialized = true, responds with Mcp-Session-Id: abc123.
- transport1 goes out of scope (local const) and is eligible for garbage collection.
- Client POSTs {"method": "tools/list"} with header Mcp-Session-Id: abc123 -> handler fires again, creates transport2 with sessionId = undefined, _initialized = false. validateSession sees !this._initialized === true (since this is not an init request) and returns HTTP 400. The MCP session is permanently broken after the handshake.
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.
| | `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'` | | ||
|
|
||
| ### `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):** |
There was a problem hiding this comment.
🟣 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
- Developer has v1 code: server.setNotificationHandler(InitializedNotificationSchema, handler)
- They open migration.md and navigate to 'Common method string replacements'
- They scan the table for InitializedNotificationSchema — it is not listed
- The table ends at ElicitationCompleteNotificationSchema
- The developer is left without a documented mapping and must guess or search elsewhere
- Had they opened migration-SKILL.md, they would find the entry: InitializedNotificationSchema | notifications/initialized
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.
| | `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'` | | ||
|
|
||
| ### `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):** |
There was a problem hiding this comment.
🟡 The PR adds client.experimental.tasks.callToolStream() to the prose in migration.md as a method that no longer accepts a Zod schema argument, but neither the before/after call-signature table in migration.md nor migration-SKILL.md section 11 (heading, prose, or table) were updated to include it. Developers using either document as a quick reference will find no row mapping callToolStream(params, schema) → callToolStream(params), and the SKILL doc omits callToolStream from section 11 entirely.
Extended reasoning...
What the bug is
The PR correctly updates the prose in migration.md (line 420 of the modified file) to state that client.experimental.tasks.callToolStream() no longer accepts a Zod result schema argument. However, neither the accompanying call-signature migration table in migration.md nor the companion migration-SKILL.md section 11 were updated to reflect this change.
The specific code paths affected
migration.md — schema-removal table gap: The section titled "Protocol.request(), ctx.mcpReq.send(), and Client.callTool() no longer take a schema parameter" contains a before/after mapping table listing rows for client.request(req, ResultSchema), ctx.mcpReq.send(req, ResultSchema), and client.callTool(params, schema[, options]). The prose now mentions callToolStream() but no corresponding rows appear in the table. A developer scanning the table will find the pattern for callTool but not for callToolStream.
migration-SKILL.md — section 11 entirely missing callToolStream: The PR diff shows that only the schema-to-method-string mapping table (section 9) and the core package description were updated in migration-SKILL.md. Section 11 retains the old heading ("Schema parameter removed from request(), send(), and callTool()"), the old prose (listing only Protocol.request(), BaseContext.mcpReq.send(), and Client.callTool()), and a table with no row for callToolStream. A grep of the file confirms callToolStream appears zero times in migration-SKILL.md.
Why existing tooling does not prevent it
Both 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.
Impact
Any developer (or LLM agent using migration-SKILL.md) migrating code that calls client.experimental.tasks.callToolStream(params, schema) or callToolStream(params, schema, options) will:
- Read the prose in migration.md and learn a schema argument must be dropped (good)
- Consult either table to confirm the exact call-site transformation and find no row (bad)
- Using migration-SKILL.md: find callToolStream mentioned nowhere in section 11, possibly concluding no change is needed for that method (bad)
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
- Developer has v1 code:
const stream = await client.experimental.tasks.callToolStream(params, schema); - They open migration.md and navigate to the schema-removal section. Prose says callToolStream drops its schema argument.
- They look at the table for a concrete before/after mapping — no row for callToolStream exists.
- They switch to migration-SKILL.md section 11 for an LLM-friendly reference. The heading reads "request(), send(), and callTool()" — callToolStream absent. Table has no row for it.
- A mechanical migration tool (LLM) reading only migration-SKILL.md would not know to strip the schema argument from callToolStream calls.
How to fix
In migration.md, add two rows to the call-signature table:
| `client.experimental.tasks.callToolStream(params, schema)` | `client.experimental.tasks.callToolStream(params)` |
| `client.experimental.tasks.callToolStream(params, schema, options)` | `client.experimental.tasks.callToolStream(params, options)` |
In migration-SKILL.md section 11, update the heading to include callToolStream, add it to the prose list, and add the same two rows to the table.
|
|
||
| ### `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. | ||
|
|
||
| For **testing**, import it directly from the internal core package: | ||
| `InMemoryTransport` is intended 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'; | ||
| ``` |
There was a problem hiding this comment.
🟡 The section heading ### InMemoryTransport removed from public API was not updated when the body text was rewritten to say the transport is now re-exported from the public packages. A developer reading this section encounters a direct contradiction: the heading says it was removed, while the body says it is available via @modelcontextprotocol/server and @modelcontextprotocol/client. The heading should be updated to something like ### InMemoryTransport import path changed.
Extended reasoning...
What the bug is
The 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 path
At 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 this
Before 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.
Impact
A 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 fix
Update the heading to accurately reflect the migration story, for example: InMemoryTransport import path changed, or InMemoryTransport moved to @modelcontextprotocol/server.
Step-by-step proof
- Developer opens docs/migration.md and navigates to the InMemoryTransport section.
- They read the heading: InMemoryTransport removed from public API - this signals the transport is NOT available in the v2 public API.
- They read the first sentence of the body: It is re-exported from both @modelcontextprotocol/server and @modelcontextprotocol/client - this signals it IS available.
- The heading and body give opposite answers to the same question; the developer cannot determine the correct import path without reading outside this section.
felixweinberger
force-pushed
the
fweinberger/migration-doc-fixes
branch
from
9a350f9 to
26b1ddc
Compare
InMemoryTransport was unreachable: the migration doc said to import it from @modelcontextprotocol/core, but core is private (no exports field, no dist). Adding it to core/public makes it available via the server/client re-exports. migration.md: - Rewrite the InMemoryTransport section to match the new export reality (was: 'removed from public API, import from internal core' — neither half of that worked) - Add SSE-to-StreamableHTTP before/after example - Method-string table: add tasks/get, tasks/result, notifications/elicitation/complete - Mention callToolStream() in the schema-removal section header migration-SKILL.md: - Fix 'core is installed automatically as a dependency' (it's internal) - Add the same method-string rows
felixweinberger
force-pushed
the
fweinberger/migration-doc-fixes
branch
from
26b1ddc to
c9c03a9
Compare
2 participants
Export
InMemoryTransportand tighten the v1→v2 migration docs.Motivation and Context
The migration doc had a section on
InMemoryTransportthat was incoherent: the heading said "removed from public API," then told users to import it from@modelcontextprotocol/core— but core isprivate: truewith no exports field, so that import never worked. There was no working v2 import path.This adds
InMemoryTransportto the curated public barrel (core/exports/public/index.ts) so it surfaces in both@modelcontextprotocol/serverand@modelcontextprotocol/clientvia their existingexport *lines.InMemoryTaskStore/InMemoryTaskMessageQueueare already there — same pattern, same use case (testing/dev).While here, fixed a few other gaps in the migration docs:
tasks/get,tasks/result,notifications/elicitation/complete,notifications/initialized.callToolStream()schema removal — added before/after example in migration.md and updated section 11 in migration-SKILL.md.How Has This Been Tested?
CI green. Method strings verified against
packages/core/src/types/schemas.ts,handleRequestsignature againstpackages/middleware/node/src/streamableHttp.ts,callToolStreamsignature againstpackages/client/src/experimental/tasks/client.ts.Breaking Changes
None — adds an export, doesn't remove or change any.
Types of changes
Checklist
Additional context
Related: #1627 (testing guide) wants this export for the same reason — there's no clean way to unit-test MCP servers without it.