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 13 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: e4cb1a9 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
…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 |
|
Re the review-body finding on |
|
@claude review |
…st Zod) AnySchema is now StandardSchemaV1 (which Zod schemas implement), and parseSchema routes through validateStandardSchema. This lets specTypeSchema() output and other non-Zod Standard Schemas be passed directly to setCustomRequestHandler / setCustomNotificationHandler / sendCustomRequest / sendCustomNotification. parseSchema is now async; all 16 callers were already in async contexts except _setupListChangedHandler, which is now async too (called from connect()). validateStandardSchema constraint relaxed from StandardSchemaWithJSON to StandardSchemaV1. isSchemaBundle was already StandardSchema-aware.
felixweinberger
force-pushed
the
fweinberger/custom-method-handlers
branch
from
f77b393 to
e4cb1a9
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.
Calls peerSchema['~standard'].validate directly (sync path); falls back to raw blob with a warning if the schema's validation is async, since getPeerSettings() is a synchronous read of already-received capabilities.
|
@claude review |
| test('delivered to peer with no handler -> no error thrown on sender', async () => { | ||
| const [client, server] = await linkedPair(); | ||
| const errors: Error[] = []; | ||
| client.onerror = e => errors.push(e); | ||
| await expect(server.sendCustomNotification('acme/unhandled', { x: 1 })).resolves.toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🟡 This test sets up const errors: Error[] = []; client.onerror = e => errors.push(e); but never asserts on errors, so the capture is dead code — and it's attached to the receiver (client), not the sender the test name references. Either drop the two lines, or add expect(errors).toEqual([]) after a tick if you want to assert the receiver stays quiet too.
Extended reasoning...
What the issue is
The test 'delivered to peer with no handler -> no error thrown on sender' does:
const [client, server] = await linkedPair();
const errors: Error[] = [];
client.onerror = e => errors.push(e);
await expect(server.sendCustomNotification('acme/unhandled', { x: 1 })).resolves.toBeUndefined();The errors array and client.onerror hook are set up but never read. The only assertion is that server.sendCustomNotification(...) resolves to undefined. The error-capture lines contribute nothing to the test outcome.
Why it's slightly confusing rather than purely harmless
The test name says "no error thrown on sender". The sender here is server; the .resolves.toBeUndefined() already fully covers that claim. The onerror hook is attached to client — the receiver — so even if it were asserted on, it would be testing a different property ("receiver doesn't surface an error for an unhandled custom notification") than the one named. A reader skimming the test sees error-capture setup and reasonably assumes it's load-bearing, then has to trace through to discover it isn't.
Why existing code doesn't catch it
Vitest doesn't warn on unused locals, and the test passes because the only real assertion (resolves.toBeUndefined()) is correct. Lint won't flag errors since it's written to inside the closure.
Addressing the "too minor" objection
One reviewer noted this is harmless dead scaffolding and not worth flagging. That's fair on impact — it doesn't affect production code or test correctness. But this is brand-new test code being added in this PR (not pre-existing), and the fix is deleting two lines (or adding one assertion). Cleaning it up before merge is essentially free and avoids leaving a misleading setup in a file that will be the reference for future custom-method tests. Filed as a nit accordingly — not blocking.
How to fix
Either delete the two setup lines:
test('delivered to peer with no handler -> no error thrown on sender', async () => {
const [, server] = await linkedPair();
await expect(server.sendCustomNotification('acme/unhandled', { x: 1 })).resolves.toBeUndefined();
});or, if the receiver-side quiet is also intended to be covered, assert it and adjust the test name:
await new Promise(r => setTimeout(r, 0));
expect(errors).toEqual([]);Step-by-step proof
- Test creates linked pair;
serveris sender,clientis receiver. errorsarray allocated;client.onerrorset to push into it.server.sendCustomNotification('acme/unhandled', {x:1})called → routes throughnotification()→ transport send → resolvesundefined..resolves.toBeUndefined()passes — this is the only assertion.- On the client side,
_onnotificationfinds no handler for'acme/unhandled'and returns early (protocol.ts_onnotification: "Ignore notifications not being subscribed to"), soclient.onerrornever fires anderrorsstays[]. - Test ends.
errorsis never read. Deleting lines 234–235 produces an identical pass/fail outcome.
| import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node'; | ||
| import { isInitializeRequest, Server } from '@modelcontextprotocol/server'; | ||
| import type { Request, Response } from 'express'; | ||
| import { z } from 'zod'; |
There was a problem hiding this comment.
🟡 nit: these two new example files use import { z } from 'zod' while every other example in the repo (and the SDK internals) uses import * as z from 'zod/v4'. Consider switching to import * as z from 'zod/v4' here and in examples/client/src/customMethodExample.ts:13 for consistency with the codebase convention.
Extended reasoning...
What the issue is
The two new example files introduced in this PR — examples/server/src/customMethodExample.ts:19 and examples/client/src/customMethodExample.ts:13 — import zod via import { z } from 'zod'. Every other example file in the repository (9 files total: simpleStreamableHttp.ts, mcpServerOutputSchema.ts, etc.) and the SDK internals use import * as z from 'zod/v4'. CLAUDE.md §Zod Schemas explicitly states "The SDK uses zod/v4 internally", and docs/migration.md shows import * as z from 'zod/v4' in its v2 examples.
Why it matters (mildly)
The bare 'zod' specifier and the 'zod/v4' subpath are not guaranteed to resolve identically. Zod 4.x ships both entry points and they currently re-export the same module, so this works fine at runtime in this repo. However, in a downstream project that has both zod v3 and v4 installed (e.g. via transitive deps), the bare 'zod' specifier may resolve to v3 while 'zod/v4' pins to v4. Since these are example files that users copy-paste, propagating an inconsistent import style undermines the convention the rest of the codebase establishes.
Why this isn't a functional bug
This is purely a consistency/style issue in example code. The repo's own zod dependency is v4, so both import forms resolve to the same module here, and the examples build and run correctly. No SDK behavior is affected.
How to fix
Change line 19 of examples/server/src/customMethodExample.ts and line 13 of examples/client/src/customMethodExample.ts from:
import { z } from 'zod';to:
import * as z from 'zod/v4';Step-by-step proof
grep -rn "from 'zod" examples/shows 11 zod imports across the examples directory.- 9 of them (all pre-existing files) use
import * as z from 'zod/v4'. - The 2 new files in this PR are the only ones using
import { z } from 'zod'. grep -rn "from 'zod'" packages/shows zero matches in SDK source — internals exclusively use'zod/v4'(e.g.packages/core/src/types/schemas.ts:1,packages/core/src/util/schema.ts:6).- Therefore the two new files are the sole deviation from an otherwise repo-wide convention.
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)