feat(core): add custom request/notification handler API to Protocol#1846
feat(core): add custom request/notification handler API to Protocol#1846felixweinberger wants to merge 11 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: b52bc34 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: |
felixweinberger
force-pushed
the
fweinberger/custom-method-handlers
branch
from
a2558ec to
e4a5c5c
Compare
felixweinberger
changed the base branch from
main
to
fweinberger/migration-doc-fixes
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Adds setCustomRequestHandler, setCustomNotificationHandler, sendCustomRequest, sendCustomNotification (plus remove* variants) to the Protocol class. These allow registering handlers for vendor-specific methods outside the standard RequestMethod/NotificationMethod unions, with user-provided Zod schemas for param/result validation. Custom handlers share the existing _requestHandlers map and dispatch path, so they receive full context (cancellation, task support, send/notify) for free. Capability checks are skipped for custom methods. Also exports InMemoryTransport from core/public so examples and tests can use createLinkedPair() without depending on the internal core barrel, and adds examples/server/src/customMethodExample.ts demonstrating the API.
- Guard setCustom*/removeCustom* against standard MCP method names (throws directing users to setRequestHandler/setNotificationHandler) - Add isRequestMethod/isNotificationMethod runtime predicates - Add comprehensive unit tests (15 cases) for all 6 custom-method APIs - Add ext-apps style example demonstrating mcp-ui/* methods and DOM-style event listeners built on setCustomNotificationHandler - Add @modelcontextprotocol/client path mapping to examples/server tsconfig so the example resolves source instead of dist
…id prototype-chain false positives
…typed-params overloads; migration docs
- sendCustomNotification now delegates to notification() so debouncing and
task-queued delivery apply to custom methods
- sendCustomRequest/sendCustomNotification gain a {params, result}/{params}
schema-bundle overload that validates outbound params before sending
- clarify JSDoc: capability checks are a no-op for custom methods regardless
of enforceStrictCapabilities
- add migration.md / migration-SKILL.md sections for custom protocol methods
felixweinberger
force-pushed
the
fweinberger/custom-method-handlers
branch
from
6509f2f to
07c5491
Compare
felixweinberger
changed the base branch from
fweinberger/migration-doc-fixes
to
main
The {params, result} schema bundle in sendCustomRequest/sendCustomNotification
is a type guard, not a transformer — the caller-provided value is sent as-is,
matching request()/v1 behavior. Transforms/defaults on the params schema are
not applied outbound (parsed.data is intentionally unused on the send path).
Adds JSDoc and a test asserting params are sent verbatim.
felixweinberger
force-pushed
the
fweinberger/custom-method-handlers
branch
from
1f5fa16 to
98d7742
Compare
…ustom methods
Adds Client.extension(id, settings, {peerSchema?}) and Server.extension(...)
returning an ExtensionHandle that:
- merges settings into capabilities.extensions[id] (advertised in initialize)
- exposes getPeerSettings() with optional schema validation of the peer blob
- wraps setCustom*/sendCustom* with peer-capability gating under
enforceStrictCapabilities
Connects the SEP-2133 capabilities.extensions field to the custom-method API
from #1846. Declare-before-register is structural (you cannot get a handle
without declaring); peer-gating on send mirrors assertCapabilityForMethod.
Stacked on #1846.
…r, drop ext-apps demo
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
packages/core/src/exports/public/index.ts:133-135— The "InMemoryTransport removed from public API" section in docs/migration.md (lines ~514-528) actively contradicts this PR: it tells migrating users that InMemoryTransport was removed and that they must import it from the internal@modelcontextprotocol/corepackage, but this PR re-adds it to the public barrel viapackages/core/src/exports/public/index.ts. After this PR merges, users following the migration guide will unnecessarily import from an internal-only package path (which the guide itself marks as "not for production use") when they could simply use@modelcontextprotocol/serveror@modelcontextprotocol/client.Extended reasoning...
What the bug is and how it manifests
docs/migration.md contains a section titled "InMemoryTransport removed from public API" (roughly lines 514–528). It reads: "InMemoryTransport has been removed from the public API surface" and instructs users to import it from the internal
@modelcontextprotocol/corepackage with the caveat "(testing only — @modelcontextprotocol/core is internal, not for production use)". This section was accurate before this PR, but this PR intentionally re-adds InMemoryTransport to the public exports without updating the migration guide.The specific code path that triggers it
This PR adds
export { InMemoryTransport } from '../../util/inMemory.js';topackages/core/src/exports/public/index.ts. Since@modelcontextprotocol/serverand@modelcontextprotocol/clientboth doexport * from '@modelcontextprotocol/core/public', InMemoryTransport is now importable from either of the public packages. The PR's own example files confirm this: bothcustomMethodExample.tsandcustomMethodExtAppsExample.tsuseimport { InMemoryTransport, Server } from '@modelcontextprotocol/server'. The PR description also explicitly acknowledges the intent: "Also exports InMemoryTransport from @modelcontextprotocol/core/public (needed by the examples; one-line overlap with #1834)".Why existing content does not prevent it
The migration guide was written before InMemoryTransport was re-added. The PR only adds a new "Custom (non-standard) protocol methods" section to migration.md; the pre-existing "InMemoryTransport removed from public API" section was left unchanged. There is no mechanism in the PR review flow to automatically detect that a code change contradicts prose in documentation.
What the impact would be
Any developer migrating from v1 who reads migration.md will follow the incorrect guidance and write
import { InMemoryTransport } from '@modelcontextprotocol/core', importing from a package the guide itself labels as internal and not for production use. The correct, public import —import { InMemoryTransport } from '@modelcontextprotocol/server'(or@modelcontextprotocol/client) — goes undiscovered until they stumble on the PR examples or source code.How to fix it
The "InMemoryTransport removed from public API" section in migration.md should be updated (or removed) to reflect that InMemoryTransport is now part of the public API again. The corrected guidance should direct users to import from
@modelcontextprotocol/serveror@modelcontextprotocol/client, matching the pattern used in the PR's own examples.Step-by-step proof
- User reads migration.md to migrate a v1 codebase that used
InMemoryTransportfor in-process testing. - User finds the "InMemoryTransport removed from public API" section and follows its v2 guidance.
- User writes:
import { InMemoryTransport } from '@modelcontextprotocol/core'; - The guide notes this import is "testing only — @modelcontextprotocol/core is internal, not for production use".
- After this PR merges, the correct import is:
import { InMemoryTransport } from '@modelcontextprotocol/server';(a public, stable package). - The user has been actively misled into using the internal package path that the guide itself discourages.
- User reads migration.md to migrate a v1 codebase that used
| const result = await client.sendCustomRequest('acme/noargs', undefined, z.object({ ok: z.boolean() })); | ||
| expect(result.ok).toBe(true); | ||
| }); | ||
|
|
||
| test('result validated against resultSchema', async () => { | ||
| const [client, server] = await linkedPair(); |
There was a problem hiding this comment.
🔴 sendCustomRequest throws a plain Error('Not connected') when not connected, while sendCustomNotification correctly throws SdkError(SdkErrorCode.NotConnected) — making the two new symmetric-looking methods behave asymmetrically on a common error path. The test at line 169 uses .rejects.toThrow(/Not connected/) which matches any error type, masking the inconsistency; the sendCustomNotification test correctly uses .rejects.toSatisfy(e => e instanceof SdkError && e.code === SdkErrorCode.NotConnected). The migration guide explicitly documents not-connected as throwing SdkError with SdkErrorCode.NotConnected, so users following the guide and writing catch(e) { if (e instanceof SdkError && e.code === SdkErrorCode.NotConnected) } will silently miss sendCustomRequest errors. The fix is to change _requestWithSchema (protocol.ts line 814) to earlyReject(new SdkError(SdkErrorCode.NotConnected, 'Not connected')), or add a guard in sendCustomRequest, and tighten the test assertion to match sendCustomNotification's.
Extended reasoning...
What the bug is and how it manifests
This PR introduces two symmetric-looking methods, sendCustomRequest and sendCustomNotification, as the new API surface for custom protocol methods. However, these two methods behave differently on a common error path: when the transport is not connected.
- sendCustomRequest → _requestWithSchema (line 814): earlyReject(new Error('Not connected')) — a plain Error
- sendCustomNotification → notification() (line 952): throw new SdkError(SdkErrorCode.NotConnected, 'Not connected') — an SdkError
The test at line 169 uses .rejects.toThrow(/Not connected/) which matches any error whose message contains that string, regardless of whether it is a plain Error or SdkError. This masks the inconsistency at testing time.
The specific code path that triggers it
- User calls sendCustomRequest('acme/x', {}, schema) on a disconnected Protocol instance
- sendCustomRequest calls _requestWithSchema, which checks if (!this._transport) and calls earlyReject(new Error('Not connected'))
- The rejected promise contains a plain Error object (not SdkError)
- By contrast, sendCustomNotification calls notification(), which checks if (!this._transport) and throws new SdkError(SdkErrorCode.NotConnected, 'Not connected')
- The test passes because .rejects.toThrow(/Not connected/) matches both plain Error and SdkError
Why existing code does not prevent it
The test uses .rejects.toThrow(/Not connected/) — a weak regex-based assertion that only checks the error message string. The sendCustomNotification test uses .rejects.toSatisfy((e) => e instanceof SdkError && e.code === SdkErrorCode.NotConnected), which is the correct, strict assertion. The asymmetry in test quality means the sendCustomRequest type mismatch goes undetected.
What the impact would be
The migration guide (docs/migration.md) includes an error hierarchy table that explicitly documents the not-connected scenario as: v2 type = SdkError with SdkErrorCode.NotConnected. Users who read this guide and write unified error handling for both new methods will silently miss errors from sendCustomRequest:
This is not a theoretical concern: the PR itself documents sendCustomRequest and sendCustomNotification side-by-side as equivalent APIs, and the migration table explicitly says NotConnected → SdkError with SdkErrorCode.NotConnected.
How to fix it
Option A (preferred): Change _requestWithSchema line 814 to:
This makes all request-path not-connected errors consistent (custom and standard).
Option B: Add a guard in sendCustomRequest specifically:
before calling _requestWithSchema.
Also tighten the test at line 169 to match sendCustomNotification's assertion style:
Step-by-step proof
- Create a TestProtocol instance without connecting it: const proto = new TestProtocol()
- Call proto.sendCustomRequest('acme/x', {}, z.object({})) — routes to _requestWithSchema
- _requestWithSchema checks if (!this._transport) → true → earlyReject(new Error('Not connected'))
- Catch the error: error instanceof SdkError → false (it is a plain Error)
- error instanceof SdkError && error.code === SdkErrorCode.NotConnected → false
- Meanwhile, proto.sendCustomNotification('acme/x', {}) → notification() → throws SdkError(SdkErrorCode.NotConnected)
- e instanceof SdkError && e.code === SdkErrorCode.NotConnected → true
- Both methods appear equivalent in the docs; only one actually behaves as documented.
…ustom methods
Adds Client.extension(id, settings, {peerSchema?}) and Server.extension(...)
returning an ExtensionHandle that:
- merges settings into capabilities.extensions[id] (advertised in initialize)
- exposes getPeerSettings() with optional schema validation of the peer blob
- wraps setCustom*/sendCustom* with peer-capability gating under
enforceStrictCapabilities
Connects the SEP-2133 capabilities.extensions field to the custom-method API
from #1846. Declare-before-register is structural (you cannot get a handle
without declaring); peer-gating on send mirrors assertCapabilityForMethod.
Stacked on #1846.
… example notification to request stream; add changeset - setCustomRequestHandler/setCustomNotificationHandler now strip _meta from params before validating against the user schema, so .strict() schemas do not reject SDK-injected fields like progressToken. _meta remains available via ctx.mcpReq._meta. Adds regression test. - examples/server/src/customMethodExample.ts: pass relatedRequestId so the acme/statusUpdate notification routes to the request response stream as the comment claims (was going to the standalone SSE stream). - Add .changeset/custom-method-handlers.md (minor bump for client+server).
…ustom methods
Adds Client.extension(id, settings, {peerSchema?}) and Server.extension(...)
returning an ExtensionHandle that:
- merges settings into capabilities.extensions[id] (advertised in initialize)
- exposes getPeerSettings() with optional schema validation of the peer blob
- wraps setCustom*/sendCustom* with peer-capability gating under
enforceStrictCapabilities
Connects the SEP-2133 capabilities.extensions field to the custom-method API
from #1846. Declare-before-register is structural (you cannot get a handle
without declaring); peer-gating on send mirrors assertCapabilityForMethod.
Stacked on #1846.
|
@claude review |
1 similar comment
|
@claude review |
| const { _meta, ...userParams } = (request.params ?? {}) as Record<string, unknown>; | ||
| void _meta; | ||
| const parsed = parseSchema(paramsSchema, userParams); |
There was a problem hiding this comment.
🟡 In both setCustomRequestHandler and setCustomNotificationHandler, missing params are normalized to {} via (request.params ?? {}) before schema validation, causing z.undefined() to always fail with InvalidParams — parseSchema(z.undefined(), {}) rejects because {} is not undefined. The test at line 179 works around this by using z.undefined().or(z.object({})) rather than z.undefined() alone, but the test name "undefined params accepted" is misleading. Either document that undefined params are normalized to {} (so z.object({}) is the correct no-args schema), or special-case undefined params to skip the destructuring.
Extended reasoning...
What the bug is and how it manifests
In setCustomRequestHandler (protocol.ts lines 1080–1082) and setCustomNotificationHandler (lines ~1115–1117), the code does:
const { _meta, ...userParams } = (request.params ?? {}) as Record<string, unknown>;
const parsed = parseSchema(paramsSchema, userParams);When the sender provides no params (request.params is undefined), the ?? {} coalesces it to {}. After destructuring _meta, userParams becomes {} — not undefined. Calling parseSchema(z.undefined(), {}) then fails because {} does not satisfy z.undefined(), resulting in a ProtocolError(InvalidParams) thrown before the user handler is ever invoked.
The specific code path that triggers it
- Client calls
sendCustomRequest("acme/noargs", undefined, ResultSchema). - Wire message:
{ method: "acme/noargs" }— no params field. - Server closure evaluates
(request.params ?? {})→{}. - Destructuring:
userParams = {}. parseSchema(z.undefined(), {})→{ success: false }.- Handler throws
ProtocolError(InvalidParams)— user handler never runs.
Why existing code does not prevent it
The ?? {} normalization is necessary to safely destructure _meta from potentially-undefined params. However, its side effect of converting undefined to {} breaks z.undefined() as a params schema. No test exercises z.undefined() alone: the test at line 179 uses z.undefined().or(z.object({})) as an explicit workaround, and the debouncing test at line ~254 does the same. Both test names imply z.undefined() is an accepted schema, but neither validates it in isolation. The normalization is also undocumented in the JSDoc for either method.
Impact
Any user who writes setCustomRequestHandler("acme/noargs", z.undefined(), handler) gets a handler that always returns InvalidParams — even for requests explicitly sent with no params. The failure is not silent (an error is returned), but the error message describes the schema mismatch rather than the normalization root cause, making diagnosis non-obvious. The misleading test name "undefined params accepted" actively suggests this pattern should work.
Practical severity is low: z.object({}) is the correct JSON-RPC idiom for no-args handlers (MCP params are always objects), and the workaround z.undefined().or(z.object({})) is demonstrated in the test. But the API contract (accepts AnySchema) implies z.undefined() should work, and the test name reinforces that expectation.
How to fix it
Option A — special-case undefined: Before destructuring, check for null/undefined and pass it through as-is so z.undefined() can match:
const userParams = request.params == null
? request.params
: (({ _meta, ...rest }) => rest)(request.params as Record<string, unknown>);
const parsed = parseSchema(paramsSchema, userParams);Option B — document the normalization: Add a JSDoc note explaining that absent params are normalized to {} before schema validation, so z.object({}) (not z.undefined()) is the correct no-args schema. Rename the misleading test to reflect the workaround, or switch it to use z.object({}) directly.
Step-by-step proof
server.setCustomRequestHandler("acme/noargs", z.undefined(), handler)— registers withz.undefined()paramsSchema.- Client calls
client.sendCustomRequest("acme/noargs", undefined, z.object({ ok: z.boolean() })). - Wire message arrives:
request.paramsisundefined. - Handler closure:
(undefined ?? {})→{}; destructuring givesuserParams = {}. z.undefined().safeParse({})→{ success: false }.throw new ProtocolError(InvalidParams, ...)— handler is never invoked.- Client receives
InvalidParamsfor a request explicitly sent with no params.
1 participant
Adds an explicit API on
Protocolfor registering handlers and sending messages for non-standard / vendor-specific methods, without reintroducing the class-level generic type parameters removed in #1451.Motivation and Context
#1446 changed
setRequestHandlerto take string method names constrained to the closedRequestMethodunion, and #1451 removed the<SendRequestT, SendNotificationT, SendResultT>generics fromProtocol/Server/Client. Together these closed the door on custom protocol extensions: there is no longer a typed way to register a handler for e.g.mcp-ui/initializeoracme/search.This PR adds a small explicit surface instead:
_requestHandlers/_notificationHandlersmaps, so they get the full dispatch path (context, cancellation, tasks, error wrapping) for free'ping','tools/call') and points tosetRequestHandlerinsteadsendCustomNotificationroutes throughnotification()so debouncing and task-queued delivery applysendCustomRequest/sendCustomNotificationaccept an optional schema bundle ({params, result}) for typed outbound params with pre-send validation — closes the typing gap vs v1's class-level genericsenforceStrictCapabilities(theassertCapabilityForMethod/assertNotificationCapabilityswitches have no default case)The primary consumer is
ext-apps, which currently extends v1'sProtocol<SendRequestT, SendNotificationT, SendResultT>to register ~15mcp-ui/*methods. The includedcustomMethodExtAppsExample.tsdemonstrates that pattern is fully expressible on this API.How Has This Been Tested?
packages/core/test/shared/customMethods.test.ts(typed params/results, full ctx, validation errors, collision guard, removal, not-connected, last-wins, prototype-key regression, schema-bundle overloads, debouncing, strict-caps)pnpm build:all,pnpm typecheck:all,pnpm lint:all, core tests 510/510npx tsx examples/server/src/customMethod{,ExtApps}Example.tsBreaking Changes
None. Purely additive to
Protocol. Not exposed onMcpServer(usemcpServer.server.*per existing guidance).Types of changes
Checklist
Additional context
isRequestMethod/isNotificationMethodruntime predicates inschemas.ts(usingObject.hasOwn)@modelcontextprotocol/clientpath mapping toexamples/server/tsconfig.jsondocs/migration.mdanddocs/migration-SKILL.md(methodString, paramsSchema)separatelyInMemoryTransportfrom@modelcontextprotocol/core/public(needed by the examples; one-line overlap with fix(core): export InMemoryTransport, tighten migration docs #1834)