docs: reframe migration guide as 'most servers: just bump' + 21 doc gaps#1910
docs: reframe migration guide as 'most servers: just bump' + 21 doc gaps#1910felixweinberger wants to merge 5 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2126a9d 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/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/v2-bc-migration-docs
branch
from
f21a840 to
147fc36
Compare
|
@claude review |
Reframe both migration.md and migration-SKILL.md to lead with the backwards-compatible upgrade path: bump @modelcontextprotocol/sdk to ^2 and let deprecation warnings guide further migration. New content: - Prerequisites section: zod ^4.2.0 runtime-crash callout, moduleResolution bundler/nodenext requirement, bun file: cache note - "core is bundled" wording (never appears in node_modules) - Zod schemas no longer exported → parseJSONRPCMessage / specTypeSchema / isSpecType mapping; /zod-schemas escape hatch - Custom-method handler registration + spec-method extension warning - Transitive-v1-dependency type-mix guidance - OAuthError.errorCode → .code rename Mapping-table rows added: ResourceTemplateType, ZodRawShapeCompat, RequestInfo, FetchLike, OAuthProtectedResourceMetadata, server/auth/errors.js, server/express.js, inMemory.js, roots/list, completion/complete, resources/subscribe|unsubscribe|templates/list, tasks/get|result, notifications/roots/list_changed, notifications/elicitation/complete, callToolStream. Absorbs the doc edits from #1834 (SSE before/after example, InMemoryTransport export location, validator import simplification, method-string and callToolStream additions).
|
@claude review |
felixweinberger
force-pushed
the
fweinberger/v2-bc-migration-docs
branch
from
147fc36 to
93ce57c
Compare
|
@claude review |
…ign :251 with removed-aliases table
| | `IsomorphicHeaders` | `Headers` (Web Standard) | | ||
| | `RequestInfo` (custom SDK type) | `Request` (Web Standard, via `ctx.http?.req`) | |
There was a problem hiding this comment.
🔴 Commit 2126a9d reverted migration.md:251 to say IsomorphicHeaders is removed (no @deprecated alias, since #1905 is closed), but didn't touch SKILL §5 — so this table still lists IsomorphicHeaders and RequestInfo under the preamble "still exported as @deprecated aliases. Migrate when convenient." The two docs now give opposite answers for these symbols, and an LLM following SKILL.md will leave the imports in place and break the build. Move IsomorphicHeaders/RequestInfo out of the §5 @deprecated table to a "removed (no alias)" line alongside WebSocketClientTransport.
Extended reasoning...
What the bug is
Commit 2126a9d's stated purpose was to fix the IsomorphicHeaders self-contradiction by establishing that the symbol is removed with no @deprecated alias (the commit message literally says "IsomorphicHeaders removed (no @deprecated alias)", and the resolution comment says "With #1905 closed the table is correct; reverted :251 to drop the @deprecated-alias claim"). It changed docs/migration.md:251 to read "The IsomorphicHeaders type alias has been removed; the codemod rewrites it to Headers", which now agrees with the migration.md "Removed type aliases" table.
But 2126a9d touched only docs/migration.md. docs/migration-SKILL.md §5 (lines 92–117) still has:
- Heading: "Deprecated Type Aliases and Symbols"
- Preamble: "The v1 names below are still exported as
@deprecatedaliases. Migrate to the v2 names when convenient." - Column header: "v1 (
@deprecated)" - Row 103:
IsomorphicHeaders→Headers(Web Standard) - Row 104:
RequestInfo(custom SDK type) →Request(Web Standard, viactx.http?.req)
So SKILL.md asserts these symbols still compile (just deprecated, leave-alone-for-now), while migration.md asserts they're gone. This is exactly the cross-doc contradiction category that 2126a9d was meant to close — the fix was one-sided and re-introduced the disagreement in the opposite direction.
How the contradiction was introduced
Commit 10164d3 reframed SKILL §5 from "Removed" to "@deprecated … still exported" and rewrote the IsomorphicHeaders/RequestInfo rows to show replacements (responding to review 3096485477). At that point, migration.md:251 also said "still exported as a @deprecated alias", so the two docs agreed. Then review 3099748905 pointed out migration.md's "Removed type aliases" table contradicted :251. The author resolved that in 2126a9d by deciding the table was correct (#1905 closed → no alias) and reverting :251 to "removed". That made migration.md internally consistent — but SKILL §5 was not re-touched, so it kept the now-rejected "@deprecated alias" framing.
Why the "integrated v2.0.0 surface" defense doesn't apply
The author's earlier defense for forward-references ("this guide documents the integrated surface; symbol lands in #19xx") doesn't cover this. #1905 is closed, so neither doc is forward-referencing a future shim. This is the same internal-self-contradiction category the author already accepted and fixed in 10164d3 and 2126a9d — no companion-PR outcome can make "IsomorphicHeaders is still exported as a @deprecated alias" (SKILL §5) and "IsomorphicHeaders has been removed" (migration.md:251 + the Removed-aliases table) simultaneously true. rg 'IsomorphicHeaders' packages/ returns zero matches, confirming no alias exists in code.
Step-by-step proof
- An LLM agent loads
migration-SKILL.md(its stated purpose: "dense mapping tables designed for tools like Claude Code to mechanically apply all the changes"). - It reads §5 preamble: "The v1 names below are still exported as
@deprecatedaliases. Migrate to the v2 names when convenient." - It sees
IsomorphicHeaders→HeadersandRequestInfo→Requestin that table, under column "v1 (@deprecated)". - Per the "shortest path" framing at the top of the file ("v1 import paths and APIs continue to work as
@deprecatedaliases … report any IDE-flagged@deprecatedusages as optional follow-ups"), the agent concludes these are non-blocking and leavesimport { IsomorphicHeaders } from '@modelcontextprotocol/sdk/types.js'in place. tscfails withTS2305: Module '...' has no exported member 'IsomorphicHeaders'— because per migration.md and the actual code, the symbol is gone with no shim.- A human cross-checking against migration.md sees :251 say "has been removed" and the Removed-aliases table list both symbols under "Removed" — directly contradicting what SKILL.md told the agent.
Impact
SKILL.md is explicitly designed for mechanical LLM-driven migration, where the deprecated-vs-removed distinction is the entire signal for whether the agent must rewrite a line or can defer it. Mislabelling a removed symbol as @deprecated causes the agent to produce a non-compiling result and undermines the PR's central "just bump" promise.
Fix
Move the IsomorphicHeaders and RequestInfo rows out of the SKILL §5 "@deprecated aliases" table and list them as removed (no alias) alongside the existing WebSocketClientTransport line — e.g.:
IsomorphicHeadersandRequestInfoare removed (no deprecated alias). Use Web-StandardHeaders/Request(viactx.http?.req); the codemod rewrites these.
That brings SKILL §5 back in line with migration.md:251 and the migration.md "Removed type aliases" table.
| ### `Headers` object instead of plain objects | ||
|
|
||
| Transport APIs and `RequestInfo.headers` now use the Web Standard `Headers` object instead of plain `Record<string, string | string[] | undefined>` (`IsomorphicHeaders` has been removed). | ||
| Transport APIs and `RequestInfo.headers` now use the Web Standard `Headers` object instead of plain `Record<string, string | string[] | undefined>`. The `IsomorphicHeaders` type alias has been removed; the codemod (`mcp-codemod v1-to-v2`) rewrites it to `Headers`. |
There was a problem hiding this comment.
🟡 nit: The new parenthetical (mcp-codemod v1-to-v2) added in 2126a9d has no implementation in the monorepo and no cited v2-bc companion PR (unlike the other forward-references resolved via #1887/#1903/#1906/#1907/#1908/#1909/#1913/#1916/#1917). A user running npx mcp-codemod v1-to-v2 will get command-not-found. Either cite the v2-bc PR that ships mcp-codemod, or drop the parenthetical — the sentence already stands on its own ("IsomorphicHeaders … has been removed").
Extended reasoning...
What changed
Commit 2126a9d (the most recent commit, pushed after all prior review rounds had concluded) rewrote docs/migration.md:251 from claiming IsomorphicHeaders is a @deprecated alias to:
The
IsomorphicHeaderstype alias has been removed; the codemod (mcp-codemod v1-to-v2) rewrites it toHeaders.
The author's resolution comment for the previous review round says: "With #1905 closed the table is correct; reverted :251 to drop the @deprecated-alias claim and point at the codemod." So the codemod reference is newly introduced by this commit and is presented as the substitute migration aid now that there is no @deprecated alias for IsomorphicHeaders.
What ships
rg -i 'mcp-codemod|codemod' across the entire repo returns exactly one hit: docs/migration.md:251 itself. There is no mcp-codemod package under packages/ (which contains only client, core, middleware, server), no bin entry in any package.json, no script, and no other doc reference.
Why this isn't covered by the existing forward-reference disclaimer
Throughout this PR's review, every other forward-reference to not-yet-shipped APIs was resolved by citing a specific v2-bc companion PR: isSpecType/specTypeSchema → #1887, /zod-schemas → #1906, OAuthError#errorCode → #1903, 3-arg setNotificationHandler → #1916, Protocol/ProtocolSpec → #1917, @modelcontextprotocol/sdk meta-package → #1913, /express RS-auth → #1907, /node/sse → #1909, server-auth-legacy → #1908. The mcp-codemod reference has no such citation, and because it was added in the final commit after the cite-or-drop exchange in comments 3096226729/3096386106 had concluded, it never went through that vetting.
The top-of-doc note ("compatibility shims … land across the v2-bc PR series") licenses forward-references to shims in that series, but a CLI codemod tool is a different category from a compat shim, and there is no evidence (no PR title, no label) that mcp-codemod is part of the v2-bc series. Even after that note was added, the reviewer continued to flag unmapped forward-references (e.g., InMemoryTransport in comment 3096432260) and the author accepted those — so the established standard for this PR is "cite or drop".
Step-by-step proof
- A user reads
docs/migration.md:251and sees thatIsomorphicHeadershas been removed (no@deprecatedalias to lean on). - The same sentence offers
mcp-codemod v1-to-v2as the migration aid for rewriting it toHeaders. - The user runs
npx mcp-codemod v1-to-v2(or searches npm formcp-codemod). - Result:
npm ERR! 404 Not Found/command not found— no such package exists in the monorepo or (per repo-wide grep) anywhere in the v2-bc series. - The user falls back to the manual find-and-replace the sentence already implies (
IsomorphicHeaders→Headers), so the parenthetical added friction without adding a working path.
This is the REVIEW.md "Documentation & Changesets" recurring catch: prose that promises behavior the code doesn't ship — flag any claim the diff doesn't back.
Impact
Low — this is a parenthetical aside, and the sentence is complete and actionable without it ("IsomorphicHeaders … has been removed" → user replaces with Headers). It does not cause a self-contradiction or regress working guidance. Hence nit, not blocking.
Fix
Either:
- Cite the v2-bc PR that ships
mcp-codemod(matching the pattern used for feat(core): add isSpecType / specTypeSchemas Records for runtime validation of spec types #1887/feat(compat): McpError/ErrorCode/JSONRPCError/StreamableHTTPError + OAuth subclass aliases #1903/feat(compat): add /server/zod-schemas subpath re-exporting *Schema constants #1906/etc.), or - Drop the parenthetical: "The
IsomorphicHeaderstype alias has been removed; replace it with the Web StandardHeaderstype."
| > **Proxy/bridge use case:** If you were using `SSEServerTransport` to bridge a stdio server to a browser (the inspector pattern), a deprecated copy is available at `@modelcontextprotocol/node/sse` for transitional use. New code should expose Streamable HTTP and let the browser | ||
| > client open the SSE stream itself. |
There was a problem hiding this comment.
🟡 Same incomplete 'removed → @deprecated/relocated' reframe fixed for StreamableHTTPError in 10164d3: this PR added the callout "a deprecated copy is available at @modelcontextprotocol/node/sse" here and retitled SKILL §8 to "Relocated Server Features" / "SSEServerTransport is @deprecated and now lives under @modelcontextprotocol/node/sse", but migration.md:183's H3 is still "### Server-side SSE transport removed" and the lead (:185) still says "has been removed from the server". Consider retitling to e.g. "### Server-side SSE transport moved to @modelcontextprotocol/node/sse (deprecated)" so the heading/TOC matches SKILL §8. (The same residual applies to migration.md "### Server auth removed" vs SKILL §8's "split: RS helpers in @modelcontextprotocol/express" — the cross-doc disagreement flagged in the earlier review thread that 2126a9d did not address.)
Extended reasoning...
What the issue is
This PR's central reframe is "v1 APIs are not removed; they continue to work as @deprecated aliases — migrate when convenient". Commit 10164d3 applied that reframe to three spots where a heading still said "removed" while the body said "still works" (variadic methods, StreamableHTTPError, SKILL §5 rows). The SSE-server-transport section was reframed in the body and in SKILL §8, but the migration.md heading and lead sentence were left on the old "removed" wording.
The specific mismatch
This PR made two coordinated edits about SSEServerTransport:
- migration.md:218-219 (new callout): "a deprecated copy is available at
@modelcontextprotocol/node/ssefor transitional use." - migration-SKILL.md §8: section retitled from "## 8. Removed Server Features" → "## 8. Relocated Server Features", and the SSE body rewritten from "
SSEServerTransportremoved entirely" → "SSEServerTransportis@deprecatedand now lives under@modelcontextprotocol/node/sse."
But it left untouched:
- migration.md:183 (H3): "### Server-side SSE transport removed"
- migration.md:185 (lead): "The SSE transport has been removed from the server."
So the migration.md heading/TOC say removed, while the same section's new callout and the parallel SKILL §8 section say @deprecated, relocated to /node/sse.
Why this is the same category the author already accepted
In 10164d3 the author accepted exactly this pattern as worth fixing: the StreamableHTTPError heading said "removed" while the rewritten body said "instanceof StreamableHTTPError still works", and the heading was retitled to "deprecated". Here, the SSE heading says "removed" while the rewritten SKILL §8 says it "is @deprecated and now lives under @modelcontextprotocol/node/sse" — the same heading-vs-body/sibling-doc disagreement, introduced by this PR (pre-PR, both docs consistently said "removed entirely" with no escape hatch).
Addressing the "both statements are literally true" objection
It's defensible to read the lead narrowly — SSEServerTransport is gone from the @modelcontextprotocol/server package, and the deprecated copy lives in a different package (@modelcontextprotocol/node) — so "removed from X" + "deprecated copy in Y" are not flatly contradictory the way the StreamableHTTPError case was. SKILL §3's import-mapping row even uses both words together ("REMOVED … deprecated copy at @modelcontextprotocol/node/sse"). That's why this is filed as a nit, not a hard contradiction.
But the H3 "Server-side SSE transport removed" is about the transport, not the package, and per #1909 / this PR's own SKILL §8 edit the transport is relocated/deprecated, not gone. The two docs now lead with opposite framings for the same symbol: a TOC-scanner of migration.md concludes SSEServerTransport no longer exists; a reader of SKILL §8 concludes it still works at the new path. Aligning the heading with SKILL §8's "Relocated" framing keeps the PR's "just bump" message coherent across both docs.
Step-by-step proof
- A user (or an LLM consuming
migration-SKILL.md) is migrating an SSE-based server. - They scan migration.md's TOC and see "Server-side SSE transport removed" → conclude
SSEServerTransportis gone, plan a Streamable-HTTP rewrite. - They open SKILL §8 "Relocated Server Features" and read "
SSEServerTransportis@deprecatedand now lives under@modelcontextprotocol/node/sse" → conclude they can keep using it transitionally at the new path. - The two docs (and migration.md's own heading vs. its new :218 callout) give opposite primary signals. The practical guidance converges (migrate to Streamable HTTP;
/node/sseexists as an escape hatch), but the headline framing disagrees.
Related residual
The same cross-doc split applies to server auth: SKILL §8 now says "Server OAuth has been split: RS helpers … are now first-class in @modelcontextprotocol/express; AS implementation moved to @modelcontextprotocol/server-auth-legacy", but migration.md "### Server auth removed" still says "removed entirely from the SDK" with no /express or /server-auth-legacy mention. This was flagged in the earlier review thread ("(4) self-contradiction still present") and 2126a9d did not address it.
Fix
- Retitle migration.md:183 to e.g. "### Server-side SSE transport moved to
@modelcontextprotocol/node/sse(deprecated)" and adjust the :185 lead to match SKILL §8's "@deprecated, relocated" framing. - Apply the same treatment to "### Server auth removed" so it matches SKILL §8's "split: RS helpers in
@modelcontextprotocol/express, AS inserver-auth-legacy".
1 participant
Part of the v2 backwards-compatibility series — see reviewer guide.
Migration guide currently reads as a full rewrite checklist. With the BC layer, most servers just bump and follow deprecation warnings. This reframes the top + fills 21 specific gaps found during real-repo testing.
Motivation and Context
Migration guide currently reads as a full rewrite checklist. With the BC layer, most servers just bump and follow deprecation warnings. This reframes the top + fills 21 specific gaps found during real-repo testing.
v1 vs v2 pattern & evidence
v1 pattern:
v2-native:
Evidence: 21 doc gaps catalogued from real-repo migration friction.
How Has This Been Tested?
v2-bc-integrationvalidation branchpnpm typecheck:all && pnpm lint:all && pnpm test:allgreenBreaking Changes
None — additive
@deprecatedshim. Removed in v3.Types of changes
Checklist
Additional context
Stacks on: none