docs: correct migration-guide errors found in a real-world v1-to-v2 migration#2246
docs: correct migration-guide errors found in a real-world v1-to-v2 migration#2246felixweinberger wants to merge 1 commit into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…-to-v2 migration Every item here caused incorrect code when the guides were followed literally while migrating a large production MCP application: - OAuth: v1 example imported removed classes from the v2 package name; document the .code property and the unknown-error-code retry semantics (v1 collapsed unknown codes into ServerError, v2 preserves them) - Context: note the restructure also applies to client-side handlers (ClientContext = BaseContext) with a client-side example - Custom methods: call out the v1->v2 handler signature change (parsed params instead of the request envelope) that breaks handler bodies and test fakes - Complete the schema-to-method-string tables (14/19 of 31 entries were missing, including tasks/get, tasks/result, and notifications/elicitation/complete) - Document that JSONRPCErrorResponse.id is now optional - Document that stdio transports now skip non-JSON lines instead of surfacing them via onerror - Document in-band tool argument-validation failures (isError result instead of a thrown InvalidParams protocol error) - Add a rule for choosing between client/server packages for shared types
felixweinberger
force-pushed
the
fweinberger/migration-doc-fixes
branch
from
6d76ce6 to
6309881
Compare
| Task-related schemas (`ListTasksRequestSchema`, `GetTaskRequestSchema`, `GetTaskPayloadRequestSchema`, `CancelTaskRequestSchema`, `TaskStatusNotificationSchema`) have no v2 handler-registration equivalent: the experimental task feature was removed and servers answer inbound | ||
| `tasks/*` with `-32601` (see the experimental-tasks section). |
There was a problem hiding this comment.
🔴 The new sentence claiming ListTasksRequestSchema/GetTaskRequestSchema/GetTaskPayloadRequestSchema/CancelTaskRequestSchema/TaskStatusNotificationSchema "have no v2 handler-registration equivalent" is contradicted by the SDK: after #2248 restored the task wire types, 'tasks/get', 'tasks/result', 'tasks/list', 'tasks/cancel', and 'notifications/tasks/status' are valid spec methods that setRequestHandler/setNotificationHandler accept and resolve schemas for — the -32601 answer only applies when no handler is registered. Either add the five mappings to the table (with a caveat that the high-level task feature/taskStore plumbing is removed and servers don't advertise the tasks capability) or reword the prose; the same sentence appears in docs/migration-SKILL.md (~lines 434-435).
Extended reasoning...
What the prose claims vs. what the SDK does. The new paragraph after the schema→method-string table states that the five task-related schemas "have no v2 handler-registration equivalent: the experimental task feature was removed and servers answer inbound tasks/* with -32601". But PR #2248 deliberately restored the task wire surface for 2025-11-25 interop, and that wire surface includes the request/notification union memberships. GetTaskRequestSchema, GetTaskPayloadRequestSchema, ListTasksRequestSchema, and CancelTaskRequestSchema are members of both ClientRequestSchema and ServerRequestSchema (packages/core/src/types/schemas.ts:2093-2096, 2124-2127), and TaskStatusNotificationSchema is in both notification unions (schemas.ts:2104, 2138).\n\nThe code path that makes the claim false. Because RequestMethod is derived from those unions (packages/core/src/types/types.ts:379) and ResultTypeMap explicitly maps 'tasks/get' → GetTaskResult, 'tasks/result' → Result, 'tasks/list' → ListTasksResult, 'tasks/cancel' → CancelTaskResult (types.ts:400-403), the typed 2-arg spec-method overload server.setRequestHandler('tasks/get', handler) compiles. At runtime the request-schema map is built from the union options (schemas.ts:2206+), so getRequestSchema('tasks/get') resolves GetTaskRequestSchema and Protocol.setRequestHandler registers the handler without hitting the "not a spec request method" error path. Server.assertRequestHandlerCapability (packages/server/src/server/server.ts:306) has no tasks/* case and no throwing default, and a grep of packages/server/src shows no other special-casing of tasks/ — nothing rejects the registration, and a registered handler will fire on inbound requests. The same holds for setNotificationHandler('notifications/tasks/status', ...) since TaskStatusNotificationSchema is in the notification unions.\n\nStep-by-step proof. (1) A v1 low-level Server user has server.setRequestHandler(GetTaskRequestSchema, handler). (2) Following the new table+prose, they conclude there is no method string to migrate to and delete the handler. (3) In reality, server.setRequestHandler('tasks/get', handler) in v2 type-checks (the method is in RequestMethod/ResultTypeMap), registers (the schema map built from ClientRequestSchema.options/ServerRequestSchema.options contains tasks/get), passes capability assertion (no tasks/* case in assertRequestHandlerCapability), and the handler is invoked when a 2025-11-25 peer sends tasks/get — the -32601 response is only the default when no handler is registered. So a direct equivalent exists for all five schemas: GetTaskRequestSchema → 'tasks/get', GetTaskPayloadRequestSchema → 'tasks/result', ListTasksRequestSchema → 'tasks/list', CancelTaskRequestSchema → 'tasks/cancel', TaskStatusNotificationSchema → 'notifications/tasks/status'.\n\nWhy this matters. This PR's whole purpose is making the migration guides literally followable, and this paragraph tells low-level Server/Client users that v1 task handlers cannot be ported when they can be — leading them to delete handlers that interop with 2025-11-25 peers depends on. It also sits in tension with the PR's own description, which cites 'tasks/result' as one of the method strings the real-world migration needed. The likely source of the confusion is the codemod's schemaToMethodMap, which omits the task schemas — but a codemod omission only means no automatic rewrite, not that the SDK has no equivalent. The surrounding statements (the high-level task feature and taskStore plumbing are gone, servers don't advertise the tasks capability, unhandled tasks/* gets -32601) all remain true; only the absolute "no handler-registration equivalent" claim is wrong.\n\nHow to fix. Either (a) add the five mappings to the table (GetTaskRequestSchema → 'tasks/get', GetTaskPayloadRequestSchema → 'tasks/result', ListTasksRequestSchema → 'tasks/list', CancelTaskRequestSchema → 'tasks/cancel', TaskStatusNotificationSchema → 'notifications/tasks/status') with a caveat that the McpServer-level task feature/taskStore plumbing is removed and servers built on this SDK don't advertise the tasks capability, or (b) reword the paragraph to say the high-level task feature was removed but the method strings remain registrable on the low-level Server/Client (and the codemod won't rewrite them automatically). Apply the same fix to the duplicated sentence in docs/migration-SKILL.md (~lines 434-435), per the repo convention that the two guides stay in sync.
| Note this is a signature change from v1, where custom-schema handlers received the full request/notification envelope. Handler bodies must switch from `request.params.x` to `params.x`, and test doubles that capture registration arguments must account for the handler now being the | ||
| _third_ argument (the second is the schemas object). |
There was a problem hiding this comment.
🟡 The PR adds the same "signature change from v1" guidance twice within the Custom (non-spec) methods subsection of docs/migration.md: once before the Before/After example (~line 397) and again here after the setNotificationHandler note (lines 435-436), with only cosmetic wording differences. Keep one of the two paragraphs (probably the first) and drop the other — the companion docs/migration-SKILL.md states this note only once.
Extended reasoning...
What the issue is. This PR inserts two near-verbatim copies of the same paragraph into the Custom (non-spec) methods subsection of docs/migration.md. The first, added right after the vendor-prefixed-methods intro (~line 397), reads: "This is a signature change from v1, where custom-schema handlers received the full request/notification envelope: handler bodies switch from request.params.x to params.x, and test doubles that capture registration arguments must account for the handler now being the third argument (the second is the schemas object)." The second, added ~40 lines later after the setNotificationHandler paragraph (lines 435-436), reads: "Note this is a signature change from v1, where custom-schema handlers received the full request/notification envelope. Handler bodies must switch from request.params.x to params.x, and test doubles that capture registration arguments must account for the handler now being the third argument (the second is the schemas object)."
Both additions come from this PR. The diff hunks @@ -380,6 +394,9 @@ and @@ -415,6 +432,9 @@ each add one of the paragraphs, and both land inside the same subsection — so this is duplication introduced by the PR, not pre-existing text being echoed.
Step-by-step proof that the two paragraphs are redundant:
- Paragraph 1 (line ~397): states (a) v1 custom-schema handlers received the full envelope, (b) handler bodies change
request.params.x→params.x, (c) the handler is now the third registration argument and the second is the schemas object. - Paragraph 2 (lines 435-436): states (a) v1 custom-schema handlers received the full envelope, (b) handler bodies change
request.params.x→params.x, (c) the handler is now the third registration argument and the second is the schemas object. - The only differences are the lead-in ("This is" vs. "Note this is") and "switch" vs. "must switch" — no new information is conveyed by the second occurrence.
- The companion
docs/migration-SKILL.md(kept in sync with this guide per repo convention, and updated in the same PR) carries this guidance exactly once, as the single "Signature trap:" paragraph in section 9 — confirming a single statement was the intent and the second copy is an authoring slip rather than deliberate reinforcement.
Why nothing catches it. Documentation prose isn't linted for repetition, and each hunk reads fine in isolation; only reading the whole subsection makes the duplication visible.
Impact. Purely editorial — no code or behavior is affected. The cost is reader friction: someone working through the Custom (non-spec) methods section reads the identical caveat twice within ~40 lines, which makes the guide feel unedited and slightly dilutes the (genuinely useful) warning.
How to fix. Drop one of the two paragraphs. The first placement (before the Before/After example) is probably the better one to keep, since the reader hits the caveat before seeing the code comparison it explains; the second copy after the setNotificationHandler note can simply be deleted.
1 participant
Corrects errors and fills gaps in
docs/migration.mdanddocs/migration-SKILL.md. Every item below produced incorrect code when the guides were followed literally during a large production v1→v2 migration (an MCP host application using the client, server, custom transports, and OAuth surfaces).Motivation and Context
Each fix maps to a concrete backwards-compatibility error a migrator hits:
@modelcontextprotocol/client) — v1 code never imported from that package; copying the example produces imports that cannot have existed. Fixed to the real v1 path (@modelcontextprotocol/sdk/server/auth/errors.js).OAuthErrorcode property is.code, and v1's unknown-code collapse is gone. v1 mapped unrecognized OAuth error codes ontoServerError, soinstanceof ServerErrorretry checks also matched nonstandard codes (e.g.invalid_refresh_token). A mechanical rewrite toerror.code === OAuthErrorCode.ServerErrorsilently stops retrying those — a behavior regression with no compile error. Documented with a known-codes-set pattern that preserves v1 semantics.request.params.xand test fakes capturing argument 2 as the handler break at runtime (handler is not a function) — observed exactly this way in real test suites.'notifications/elicitation/complete'(not'elicitation/complete') and'tasks/result'(not'tasks/get-result') — wrong guesses register handlers that never fire or throw. Both tables now carry all 31 mappings, matching the codemod's authoritative map.ClientContext=BaseContext); all existing examples were server-side, so client elicitation/sampling/roots handlers (and their test mocks) were migrated with the wrongctxshape. Added a client-side example.JSONRPCErrorResponse.idis now optional (v1 required it) — id-less error responses reachonmessageinstead of being rejected at the transport. Undocumented behavior change; code assumingidis always present should handle absence.onerror— consumers usingonerrorto detect noisy servers lose that signal. Undocumented until now.isError: true,Input validation error: ...) instead of v1's protocol-levelInvalidParams— client code catching protocol errors, and tests asserting a thrown error, behave differently with no compile-time hint.@modelcontextprotocol/clientvs/serverfor shared types — the single most frequent point of confusion in package-split migrations; both packages re-export identical declarations, so the guide now says to pick by file role and never mix.How Has This Been Tested?
Every claim was verified against the source in this repo before documenting (
OAuthErrorinpackages/core/src/auth/errors.ts,BaseContext/handler overloads inpackages/core/src/shared/protocol.ts,JSONRPCErrorResponseSchemainpackages/core/src/types/schemas.ts,ReadBufferinpackages/core/src/shared/stdio.ts, in-band tool errors inpackages/server/src/server/mcp.ts, method strings againstpackages/core/src/types/and the codemod'sschemaToMethodMap). The failure modes themselves were each observed during a real v1→v2 migration of a large MCP application.Breaking Changes
None — documentation only.
Types of changes
Checklist
Additional context
Both guides were updated together per the repo convention that
docs/migration.mdanddocs/migration-SKILL.mdstay in sync. No changeset included: the change is docs-only and releases no package. The new table rows follow the codemod'sschemaToMethodMap.ts, so docs, codemod, and spec types now agree on all 31 method strings.