Export Protocol and mergeCapabilities from client/server package roots#2501
Export Protocol and mergeCapabilities from client/server package roots#2501felixweinberger wants to merge 1 commit into
Conversation
Consumers that run their own JSON-RPC protocol on the Protocol engine (e.g. the MCP Apps SDK's ui/initialize handshake over postMessage) have no v2 extension point: subclassing Client/Server forces the MCP initialize handshake onto channels whose deployed peers do not speak it. Restore the v1 extension point by exporting Protocol and mergeCapabilities from the curated public barrel, which both package roots re-export. Protocol.connect() wires the transport without any handshake, and non-spec methods pass the wire layer without era gating, so the v1 wire posture is fully expressible. The codemod now rewrites Protocol/mergeCapabilities imports from shared/protocol.js to the package root like the module's other symbols; its removedSymbols machinery keeps coverage via a synthetic mapping. Root-level export pins prevent silent regression, and the migration guide documents the subclass shape changes and the cross-bundle class identity caveat (each root bundles its own copy).
🦋 Changeset detectedLatest commit: f60b401 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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/codemod
@modelcontextprotocol/core
@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}
There was a problem hiding this comment.
Beyond the inline findings, one candidate was examined and ruled out: the concern that the newly-public Protocol rewrites handler-thrown -32002 to -32602 on custom methods. The 2025 codec's encodeErrorCode does apply that mapping era-blind, but it matches what the deployed v1.x SDK already emitted, so a v1 Protocol subclass migrating to this export sees identical wire behavior — no regression.
Extended reasoning...
Bugs were found (three nit-level findings, posted inline), so no approval — and independently, making the Protocol engine class public API is a deliberate surface-expansion decision a maintainer should sign off on, even though the ext-apps justification and test coverage (barrel/root pins, no-initialize-on-the-wire behavior pin, codemod suite) are solid. This note records the one refuted candidate so a later review pass does not re-derive it: I confirmed in packages/core-internal/src/wire/rev2025-11-25/codec.ts:137-141 that the -32002→-32602 mapping is documented as matching deployed v1.x behavior, so it is not a behavior change introduced by this PR.
| * features like request/response linking, notifications, and progress. | ||
| * | ||
| * `Protocol` is abstract; `Client` and `Server` are the concrete role-specific | ||
| * implementations most code should use. | ||
| * implementations most code should use. Consumers that run their own JSON-RPC | ||
| * protocol on the engine (custom methods, own handshake — e.g. the MCP Apps SDK) | ||
| * extend `Protocol` directly instead: `connect()` wires the transport without | ||
| * performing the MCP `initialize` handshake. Subclasses implement `buildContext` | ||
| * (identity is fine) and the three capability assertion hooks. | ||
| */ | ||
| export abstract class Protocol<ContextT extends BaseContext> { | ||
| private _transport?: Transport; |
There was a problem hiding this comment.
🟡 The wire layer silently strips reserved MCP field names from custom-method traffic on the newly-public Protocol extension point, contradicting the changeset's claim that non-spec methods "pass through the wire layer without era gating": (1) _onrequest() unconditionally lifts top-level inputResponses/requestState out of every inbound request's params before the 3-arg handler's schema validation, and (2) the 2025 codec's decodeResult unconditionally deletes a top-level resultType key from any inbound result before the caller's schema validates it — a Protocol subclass never negotiates a version, so it is permanently on that codec. A custom protocol using any of these names gets a hard -32602/InvalidResult failure (required field) or silent data loss (optional field), where v1 passed both legs verbatim. Consider gating both strips on isSpecRequestMethod, or at minimum documenting these reserved top-level names in the new Protocol JSDoc and the migration guide's Protocol section.
Extended reasoning...
What happens
The PR frames the Protocol export as restoring the v1 extension point for consumers that run their own JSON-RPC protocol on the engine, and the changeset states that "non-spec methods pass through the wire layer without era gating." Two pre-existing wire-layer mechanisms break that contract for custom-method traffic — neither is gated on isSpecRequestMethod, unlike the era gates in the same dispatch funnel which explicitly treat non-spec methods as consumer-owned and era-blind.
Request leg — Protocol._onrequest() calls liftWireOnlyMaterial(rawRequest, 'request') unconditionally (packages/core-internal/src/shared/protocol.ts:934). RETRY_PARAMS_KEYS = ['inputResponses', 'requestState'] (line 198) is filtered purely by key in params (line 219) with no spec-method check, so those top-level params members are deleted from the params the handler sees on ANY request method. The 3-arg setRequestHandler path then validates the already-lifted {...request.params}. Additionally, a lifted inputResponses value is run through partitionInputResponses, which drops entries that don't look like bare MRTR response objects — real data loss for a custom protocol that reuses the name with its own shape. Notably, the lift's own comment carves out notifications for exactly this reason ("a vendor notification may legitimately use the same names") — custom-method requests get no equivalent carve-out.
Response leg — every Protocol.request() resolves through _requestWithSchemaViaCodec, whose response handler calls codec.decodeResult(request.method, response.result) before validating against the caller's schema. A direct Protocol subclass never negotiates a protocol version (skipping the MCP initialize handshake is the PR's entire premise), so codecForVersion(undefined) pins it to the 2025 codec (wire/codec.ts:298-300). That codec's decodeResult (wire/rev2025-11-25/codec.ts:110-121) ignores its _method parameter and unconditionally deletes a top-level resultType key from any plain-object result. The strip's tolerate-and-drop rationale (a 2025-era MCP peer sending 2026 vocabulary is misbehaving) is sound for MCP traffic but doesn't extend to custom protocols, where resultType is a perfectly natural result field name.
Step-by-step proof (response leg)
- Two
Protocolsubclasses A and B connect over any transport with no MCP handshake, per the newpublicProtocol.test.tspattern. - B registers
setRequestHandler('acme/search', { params: ..., result: ... }, handler)whose handler returns{ resultType: 'partial', hits: [...] }. The 2025 codec'sencodeResultis identity for non-tools/listmethods, so the result goes out intact. - A calls
a.request({ method: 'acme/search', params }, z.object({ resultType: z.string(), hits: z.array(z.string()) })). - On A,
decodeResulthits the'resultType' in rawbranch, copies the object, deletes the key, and hands the husk tovalidateStandardSchema. - A's schema requires
resultType, so validation fails and every call rejects withSdkError(SdkErrorCode.InvalidResult, 'Invalid result for acme/search: ...')— the method is permanently broken even though B answered correctly. If the schema marks the field optional, the value silently vanishes instead.
The request leg has the mirror-image proof: B registers a params schema requiring requestState; A sends { doc: 'x', requestState: 's1' }; the lift deletes params.requestState before validation; B answers -32602 Invalid params despite the client having sent the field.
Why nothing else catches it
The new pin test (publicProtocol.test.ts) exercises only a result field named echoed and a param named value, so neither strip is on its path. The new Protocol JSDoc (protocol.ts:552-562), the migration guide's Protocol section, and docs/advanced documentation all describe the extension point without mentioning any reserved top-level names. v1's Protocol handed request.params to handlers verbatim and validated response.result directly against the caller's schema, so a v1 subclass migrating along the documented path can regress silently.
Why nit rather than blocking
Both mechanisms pre-date this PR and are deliberate for MCP traffic; the PR doesn't touch them — it publicizes the consumer class for which the field names matter. The trigger requires a custom protocol to pick exactly inputResponses, requestState, or resultType as a top-level name, and the motivating consumer (ext-apps, 302 passing tests) doesn't. Nothing shipped concretely breaks on merge; the gap is between the prose contract and the wire behavior.
Fix
Either gate both strips on isSpecRequestMethod — the retry-key lift in liftWireOnlyMaterial (mirroring its existing notification carve-out), and the resultType strip in rev2025Codec.decodeResult (which already receives the method as its first parameter, currently ignored as _method) — or document the reserved top-level names (inputResponses, requestState, resultType, plus the reserved io.modelcontextprotocol/* _meta envelope keys) in the new Protocol JSDoc and the migration guide's Protocol section, and soften the changeset's "without era gating" phrasing accordingly.
| '@modelcontextprotocol/sdk/shared/protocol.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved', | ||
| removedSymbols: { | ||
| Protocol: | ||
| 'The Protocol base class is not exported by the v2 packages. To observe or handle inbound requests ' + | ||
| 'that have no registered handler, use client.fallbackRequestHandler / server.fallbackRequestHandler; ' + | ||
| 'build custom behavior on Client or Server instead of subclassing Protocol. ' + | ||
| 'See the migration guide: Behavioral changes > Client connection & dispatch.', | ||
| mergeCapabilities: | ||
| 'mergeCapabilities() is not exported by the v2 packages. Pass the complete capabilities object to the ' + | ||
| 'Client/Server constructor, or merge capability objects with a plain object spread.' | ||
| } | ||
| status: 'moved' | ||
| }, |
There was a problem hiding this comment.
🟡 Routing Protocol through RESOLVE_BY_CONTEXT resolves per file, so in a mixed client+server project one codemod run can silently rewrite file A's Protocol import to @modelcontextprotocol/client and file B's to @modelcontextprotocol/server — exactly the dual-copy configuration this PR's migration-guide caveat says to avoid (instanceof is false across the two compiled copies and the nominal types are incompatible). Consider emitting an info/action-required diagnostic when the Protocol value symbol is routed by context in a both project, or when one run resolves it to different packages across files.
Extended reasoning...
The bug. This PR removes the removedSymbols entry for Protocol/mergeCapabilities from the shared/protocol.js mapping (packages/codemod/src/migrations/v1-to-v2/mappings/importMap.ts:174-177), so Protocol now routes through the generic RESOLVE_BY_CONTEXT path like the module's structural types. But context resolution is per file: resolveTypesPackage (packages/codemod/src/utils/projectAnalyzer.ts:118-135) returns @modelcontextprotocol/client when a file has client-only sibling imports and @modelcontextprotocol/server when it has server-only siblings, before the project-level projectType is consulted. There is no run-level consistency mechanism for a symbol across files.
The code path. Consider a projectType: 'both' codebase — e.g. the exact shape of this PR's motivating consumer, ext-apps, where App (client-side, imports client SDK siblings) and AppBridge (host-side, imports server siblings) both extend Protocol. Step by step:
app.tsimports{ Protocol } from '@modelcontextprotocol/sdk/shared/protocol.js'alongside@modelcontextprotocol/sdk/client/...imports.fileHasClientImports=true, fileHasServerImports=false→ rewritten to@modelcontextprotocol/client.app-bridge.tsimports the same symbol alongside@modelcontextprotocol/sdk/server/...imports → rewritten to@modelcontextprotocol/server.- No diagnostic fires for either file — the info note in
resolveTypesPackageonly fires for no-signal files, and its text ("both client and server re-export them… importing from either compiles") was written for structural types; it is now misleading for the nominally-typedProtocolclass. - The two package roots each bundle their own compiled copy of the class (per this PR's own migration-guide caveat in
docs/migration/upgrade-to-v2.md), sox instanceof Protocolisfalseacross the copies, and sinceProtocolhas many private members, the two declarations are nominally incompatible — cross-file type usage produces confusingTS2322errors.
Why existing code doesn't prevent it. Pre-PR, Protocol imports were dropped with an @mcp-codemod-error marker, forcing a deliberate manual choice of one package — the split was impossible to produce mechanically. Post-PR, the codemod can mechanically produce the configuration the guide explicitly instructs users to avoid ("import it from one package consistently within a process"), with zero signal.
Impact. Bounded, which is why this is a nit rather than a blocker: (1) the trigger requires a mixed project importing Protocol in multiple files with divergent sibling signals and cross-file reliance on class identity; (2) the type-level mismatch surfaces at typecheck as TS2322 (confusing but discoverable), leaving only runtime instanceof across copies as a truly silent failure; (3) the same per-file routing already applies to other classes (InMemoryTransport, UriTemplate) without diagnostics, so this extends an existing accepted pattern rather than introducing a new one.
How to fix. Emit an info/action-required diagnostic when the Protocol value symbol specifically is routed by context in a both-signal project (or, more precisely, when one codemod run resolves it to different package roots across files), pointing at the migration-guide caveat. Also consider adjusting the no-signal info note so it doesn't claim "importing from either compiles" when the routed symbols include the nominal Protocol class.
| The `Protocol` base class and `mergeCapabilities` moved: import them from the | ||
| `@modelcontextprotocol/client` or `@modelcontextprotocol/server` package root instead | ||
| of `shared/protocol.js`. Extending `Protocol` directly remains the right shape for | ||
| consumers that run their own JSON-RPC protocol on the SDK engine (custom methods, own | ||
| handshake) — `Protocol.connect()` wires the transport without performing the MCP | ||
| `initialize` handshake. Subclasses adapt to the v2 shape: one `ContextT` type | ||
| parameter instead of the three message generics, a `buildContext` implementation | ||
| (identity is fine), and method-string-keyed handler registration (see | ||
| [`setRequestHandler` / `setNotificationHandler` use method strings](#setrequesthandler--setnotificationhandler-use-method-strings)). If you | ||
| were only reaching into protocol internals for | ||
| debugging, prefer `client.fallbackRequestHandler` / `server.fallbackRequestHandler`, | ||
| which receives every inbound request that no registered handler matches, before | ||
| capability gating. The codemod rewrites `Protocol` and `mergeCapabilities` imports to | ||
| the package root, like the module's other symbols. | ||
|
|
||
| One caveat: the client and server packages each bundle their own compiled copy of the | ||
| class, so the two roots' `Protocol` exports are distinct classes. Import it from one | ||
| package consistently within a process — `instanceof` returns `false` across the two | ||
| copies, and their type declarations are nominally incompatible (the class has private | ||
| members). |
There was a problem hiding this comment.
🟡 The newly-public Protocol extension point is only documented in prose inside the v1→v2 migration guide, which is addressed to migrating users — a new v2 consumer has no discoverable docs that Protocol is subclassable, how to implement it, or the dual-bundled-copy instanceof caveat (which lives only here). Consider adding a short section or pointer in docs/advanced/custom-methods.md (and/or an examples/ story) covering the custom-JSON-RPC-engine use case.
Extended reasoning...
What the finding is. This PR makes Protocol and mergeCapabilities public API on the @modelcontextprotocol/client and @modelcontextprotocol/server package roots, positioned as the supported extension point for consumers that build their own JSON-RPC protocols on the SDK engine (the ext-apps ui/initialize use case). However, the only prose documentation added is the section in docs/migration/upgrade-to-v2.md (~lines 1447–1466) — content framed for v1 migrators ("The Protocol base class and mergeCapabilities moved: import them from … instead of shared/protocol.js"). There is no guide page, section, or example addressed to a new v2 consumer.
Verification. Grepping docs/ for extends Protocol, Protocol base class, subclass Protocol, or custom JSON-RPC matches nothing outside the migration guide. docs/advanced/custom-methods.md — the natural home, since it already documents custom JSON-RPC methods on Client/Server via setRequestHandler — never mentions that Protocol is public and directly subclassable. mergeCapabilities appears in no non-migration doc. No examples/ story demonstrates extends Protocol. The only other documentation is the (good) JSDoc on the Protocol class in packages/core-internal/src/shared/protocol.ts.
Why this matters. Concrete walkthrough of the gap: a developer starting fresh on v2 wants to run a custom handshake protocol over an SDK transport (exactly the ext-apps shape this PR exists to support). They browse docs/advanced/ — custom-methods.md, custom-transports.md, low-level-server.md — and find only the Client/Server custom-method path, which still forces the MCP initialize handshake onto their channel. Nothing tells them (1) that Protocol is importable from the package root, (2) what a subclass must implement (buildContext — identity is fine — plus the three capability assertion hooks), or (3) the dual-bundled-copy caveat: the client and server packages each bundle their own compiled copy of the class, so instanceof returns false across the two copies and the nominal types don't mix. That last caveat is a real consumer-facing gotcha, and it is currently discoverable only via the migration guide. The repo review checklist also lists, for new features, "verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example" as a distinct item from the migration-guide item.
Why nit, not blocking. Prose documentation does exist (migration guide + changeset + JSDoc), the primary audience for this feature today is migrating v1 consumers like ext-apps — who are well served by the migration guide — and there is zero runtime impact. The export itself is well-tested (barrel + both package roots + a no-initialize-on-the-wire behavior pin).
Suggested fix. Add a short section (or pointer) in docs/advanced/custom-methods.md covering the "build your own protocol on the engine" case: import Protocol from one package root, implement buildContext + the three assertion hooks, note that connect() performs no MCP initialize, and include the single-package-consistently / instanceof caveat. Optionally add an examples/ story mirroring packages/core-internal/test/shared/publicProtocol.test.ts. This can equally land as a fast follow-up.
1 participant
Makes the
Protocolbase class andmergeCapabilitiesimportable from the@modelcontextprotocol/clientand@modelcontextprotocol/serverpackage roots.Motivation and Context
Some SDK consumers use
Protocolas a JSON-RPC engine for their own protocol rather than speaking MCP itself. ext-apps is the concrete case:App/AppBridgerun aui/initializehandshake over an iframe postMessage channel, and in v1 both extendProtocoldirectly. The v2 guidance (subclassClient/Server) forces the core MCPinitializehandshake onto that channel — see modelcontextprotocol/ext-apps#710, where views end up performing a double handshake (initialize+ui/initialize) that deployed hosts reject with -32601.v2's
Protocolis still handshake-free (connect()only wires the transport) and custom methods pass the wire layer without era gating, so exporting the class restores the v1 extension point without behavior changes.Also updates the codemod:
Protocol/mergeCapabilitiesimports fromshared/protocol.jsnow rewrite to the package root like the module's other symbols, instead of being dropped with an action-required marker.How Has This Been Tested?
Protocolsubclass pair exchanges a custom method with no MCPinitializeon the wire (both directions captured).App/AppBridgeextendProtocol, keep the v1 wire contract (ui/initializeis the first and only handshake), all 302 ext-apps tests pass, and the double-handshake workarounds from ext-apps#710 become deletable.import { Protocol, mergeCapabilities, type ProtocolOptions } from '.../shared/protocol.js'rewrites cleanly to@modelcontextprotocol/server; codemod suite green (629).Breaking Changes
None — additive export. One caveat documented in the migration guide: the client and server packages each bundle their own compiled copy of the class, so import it from one package consistently within a process (
instanceofand the nominal types don't mix across the two copies).Types of changes
Checklist
Additional context
The
removedSymbolscodemod machinery now has no live mapping users (these two symbols were the last); it stays in place for future removals, with its tests re-pinned against a synthetic mapping. Happy to remove it in a follow-up if we'd rather not keep unused machinery.