feat(core): restore v1 Zod-schema overloads on setRequestHandler/setNotificationHandler/request#1891
feat(core): restore v1 Zod-schema overloads on setRequestHandler/setNotificationHandler/request#1891felixweinberger wants to merge 14 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 274ca81 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/protocol-concrete
branch
from
db49955 to
070a9cd
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/protocol-concrete
branch
from
070a9cd to
54c7b21
Compare
felixweinberger
force-pushed
the
fweinberger/protocol-concrete
branch
3 times, most recently
from
31f4544 to
fec9cce
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/protocol-concrete
branch
7 times, most recently
from
f573eb9 to
f20ca3b
Compare
felixweinberger
changed the title
felixweinberger
force-pushed
the
fweinberger/protocol-concrete
branch
from
f20ca3b to
0daaa15
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
There was a problem hiding this comment.
All prior feedback is addressed and I didn't find anything new on this pass — leaving the final call to a human since this expands the public API surface (new overloads on request/setRequestHandler/setNotificationHandler/callTool/mcpReq.send, plus new ZodLikeRequestSchema/isZodLikeSchema/extractMethodLiteral exports) as part of the v2-BC series.
Extended reasoning...
Overview
Restores v1 calling forms on Protocol/Client/Server: Zod-schema overloads for setRequestHandler/setNotificationHandler, explicit-result-schema overload for request() and mcpReq.send(), and a v1-compat 3-arg overload for callTool(). Adds packages/core/src/util/compatSchema.ts (ZodLikeRequestSchema, isZodLikeSchema, extractMethodLiteral) and exports them from the internal barrel and public index. Widens removeRequestHandler/removeNotificationHandler/assertCanSetRequestHandler to accept any method string. Includes new tests (customMethods.test.ts, callTool.compat.test.ts, setRequestHandlerSchemaParity.test.ts), new examples, and migration-doc updates. Also a small drive-by: _requestWithSchema now rejects with SdkError(NotConnected) instead of plain Error('Not connected'), which aligns with what migration-SKILL.md already documents.
State of prior review threads
This PR has been through six review rounds with me. Everything I raised is now resolved at HEAD (296e179): the callTool(params, undefined, opts) arg-dropping bug is fixed and tested; the schema-form parity bug (bypassing tools/call/elicitation/create/sampling/createMessage wrapping) is fixed with a regression test; @deprecated was dropped from all schema-arg overloads; _registerCompatRequestHandler was inlined; migration.md/migration-SKILL.md §9/§11/Unchanged-APIs prose now agree with the implementation including mcpReq.send; the §9 intro was reworded to "in addition to" in both files; the client example now spawns the server via npx tsx; the server example now actually emits acme/searchProgress so the client handler fires.
Security risks
None identified. No auth/crypto/permission code paths touched. The new schema-sniffing logic (isZodLikeSchema, '~standard' in arg, 'parse' in arg) only affects overload dispatch on local function arguments, not wire data.
Level of scrutiny
This is a deliberate public-API expansion in core protocol code, part of a coordinated v2-BC series with a linked reviewer guide and stacked follow-ups (#1916/#1917). Per the repo's "burden of proof is on addition" / "every new export is intentional" principles, the decision to ship these overloads as first-class (vs. deprecated compat shims) and to export ZodLikeRequestSchema/isZodLikeSchema/extractMethodLiteral publicly is one a maintainer should sign off on, not a bot.
Other factors
Test coverage for the new behavior is solid (round-trip, invalid-params rejection, parity test, callTool arg-dispatch matrix). The two cfWorker.ts diffs are pure export-reorder noise. I'm not approving solely because of scope/API-surface, not because of any outstanding concern with the implementation.
…otificationHandler/request Adds to Protocol (inherited by Client/Server): - setRequestHandler/setNotificationHandler(ZodSchema, handler) — the v1 form, first-class - request(req, resultSchema, opts?) — the v1 explicit-schema form, first-class - callTool(params, resultSchema?, opts?) — accepts the v1 schema arg (ignored) for source compat - removeRequestHandler/removeNotificationHandler/assertCanSetRequestHandler accept any method string Custom (non-spec) methods work via the Zod-schema form, same as v1. schema.ts/standardSchema.ts unchanged from main.
…igration.md mcpReq.send claim; complete custom-method example
…ent/Server
Normalize schema-arg to method string + parse-wrapped handler, then fall through to
the per-method dispatch (tools/call task validation, elicitation/create capability checks).
Previously the schema form short-circuited via _registerCompatRequestHandler, so e.g.
setRequestHandler(CallToolRequestSchema, h) and setRequestHandler('tools/call', h) had
different runtime behavior.
Also reword @deprecated message: schema form is not deprecated for non-spec methods (the
method-string overload is constrained to RequestMethod), so the advice now reads
'For spec methods, pass the method string instead.'
…gistration The normalize-then-fall-through change routes custom methods through _setRequestHandlerByMethod, which previously assumed a spec method (unconditional getRequestSchema(method).parse). Guard with schema?. so custom methods registered via Server/Client overrides do not crash.
…th other schema-arg overloads
…c methods Mirrors Protocol.request()'s schema overload so handlers can send custom-method requests via ctx.mcpReq.send. Also corrects the _registerCompatRequestHandler JSDoc (called by Protocol's own overload dispatch, not Client/Server overrides).
…mpatRequestHandler The schema-arg forms of request()/setRequestHandler()/setNotificationHandler() are a parallel supported style, not superseded: request(req, schema) is the only typed form for non-spec method results, and the schema-first handler form gives full-envelope validation. Replaces the @deprecated tags with plain JSDoc guidance so the PR's own custom-method examples no longer render with strikethrough. Also inlines _registerCompatRequestHandler (single callsite) and fixes the stale 'schema parameter removed' note in migration.md.
…both forms supported
felixweinberger
force-pushed
the
fweinberger/protocol-concrete
branch
from
f4b5f42 to
ddd6776
Compare
felixweinberger
requested review from
KKonstantinov
and removed request for
KKonstantinov
…n stdio example The parity test sent params.task without declaring tasks capability, so assertTaskHandlerCapability threw before the tools/call wrapper ran and both forms produced the same capability error rather than the wrapper's task-result-validation error. Declare tasks.requests.tools.call and assert on the wrapper's 'Invalid task creation result' message so the test cannot pass for the wrong reason. Also: console.log over StdioServerTransport writes to the JSON-RPC channel (stdout); use console.error so debug output reaches the user.
|
@claude review |
There was a problem hiding this comment.
All my prior comments are addressed and I have nothing further to flag — but this adds meaningful public API surface to core Protocol (new overloads, ZodLikeRequestSchema export, compatSchema helpers), so a maintainer should sign off on the final API shape.
Extended reasoning...
Overview
This PR restores v1 calling conventions on Protocol (inherited by Client/Server): Zod-schema overloads for setRequestHandler/setNotificationHandler, explicit-result-schema overloads for request() and ctx.mcpReq.send(), a v1-compat callTool(params, schema?, opts?) overload, and widening removeRequestHandler/removeNotificationHandler/assertCanSetRequestHandler to accept any string. It adds a new util/compatSchema.ts module (ZodLikeRequestSchema, isZodLikeSchema, extractMethodLiteral), exports ZodLikeRequestSchema from the public API, updates both migration docs, and adds paired client/server examples plus three new test files. 18 files changed.
Review history
This PR has been through eight rounds of review with me. Issues raised and now resolved include: doc/code drift on mcpReq.send() schema support (twice, in opposite directions), callTool(params, undefined, opts) dropping options, schema-form setRequestHandler bypassing per-method wrapping for tools/call/elicitation/create/sampling/createMessage, contradictory @deprecated tags on the only typecheckable form for non-spec methods, stale §9 intro wording, the example spawning a non-existent dist/ build, console.log on a stdio server's stdout, and a hollow regression test that never reached the wrapper it guards. The latest commit (d2e046d) adds the tasks capability so the parity test reaches wrappedHandler and tightens the assertion to .toContain('Invalid task creation result'), and switches the stdio example to console.error. The bug-hunting system found nothing this round.
Security risks
None identified. No auth, crypto, or permission code is touched. The new overload-dispatch logic sniffs argument shape ('~standard' in arg, 'parse' in arg) but only to route between internal code paths; no user-controlled data influences anything beyond which validation schema is applied.
Level of scrutiny
High. This expands the public API of the core Protocol class and adds a new public type export. Per the repo's review principles ("burden of proof is on addition", "every new export is intentional"), API-surface additions warrant maintainer sign-off even when the implementation is correct. The PR is also the base of a stack (#1916, #1917), so the shape chosen here constrains follow-ups.
Other factors
Test coverage is solid (custom-method round-trip, callTool compat dispatch, schema-parity regression). Docs and examples are now internally consistent after several iterations. The two cfWorker.ts files have a no-op export-reorder that's cosmetic. I'm deferring rather than approving because the scope and API-design weight of this change is beyond what I should green-light without a human maintainer's review, not because I see remaining issues.
There was a problem hiding this comment.
Nothing further from me — d2e046d addresses the last two items (parity test now reaches the wrapper, stdio example uses stderr). Leaving final sign-off to a human since this lands new public API surface on Protocol (schema-arg overloads + ZodLikeRequestSchema export) as part of the v2-BC series with #1916/#1917 stacked on it.
Extended reasoning...
Overview
Restores v1 calling conventions on Protocol/Client/Server: Zod-schema overloads for setRequestHandler/setNotificationHandler, explicit-result-schema overload for request() and ctx.mcpReq.send(), v1-compat 3-arg callTool(), and widening removeRequestHandler/removeNotificationHandler/assertCanSetRequestHandler to string. Adds compatSchema.ts helpers, exports ZodLikeRequestSchema from the public surface, ships paired client/server examples, and rewrites migration.md / migration-SKILL.md §9/§11 to reflect both forms. 18 files, ~+550 net.
Security risks
None identified. No auth/crypto/transport-lifecycle changes; the new dispatch is type/shape sniffing on caller-supplied schemas with the same downstream validation paths as before.
Level of scrutiny
High — this is additive public API on the core Protocol class that intentionally introduces a parallel call form alongside the method-string form, which the repo's own REVIEW.md flags ("one way to do things", "burden of proof on addition"). It's a deliberate BC tradeoff that anchors #1916/#1917, so a maintainer should own the design sign-off rather than a bot.
Other factors
Seven prior review rounds surfaced real issues (schema-form bypassing per-method wrapping, dropped callTool options, doc/code drift on mcpReq.send, hollow regression test) — all now addressed with tests (customMethods.test.ts, callTool.compat.test.ts, setRequestHandlerSchemaParity.test.ts). The current revision looks internally consistent; I found nothing new to flag.
…a overloads @deprecated; address review - @deprecated on the Zod-schema overloads of setRequestHandler / setNotificationHandler (Protocol + Client + Server). The 3-arg (method, paramsSchema, handler) form is the non-deprecated path for custom methods. request()/mcpReq.send()/callTool schema overloads are NOT deprecated (only typed form for custom-method results). - Unify positional-schema detection via isResultSchemaLike() (~standard or .parse) used by request(), mcpReq.send() and callTool(). - request()/mcpReq.send() now reject with a clear TypeError when a non-spec method is called without a result schema (was: undefined schema crashed in parseSchema downstream). - Avoid redundant spec-schema parses: _setRequestHandlerByMethod() takes skipSpecParse; Server/Client overrides pass true for their per-method wrappers and for the Zod-schema form. Aligns Protocol with subclasses (both skip when user supplied a schema). - compatSchema.ts: drop @internal from file header (ZodLikeRequestSchema is publicly exported); add isResultSchemaLike helper. - Tests: client setRequestHandlerSchemaParity (elicitation+sampling), compatSchema helpers, runtime-guard cases for request()/mcpReq.send().
…edHandler
The skipSpecParse optimization correctly skipped the redundant parse in
the base helper, but wrappedHandler was forwarding the raw JSONRPCRequest
to the user's handler instead of the validatedRequest.data it had already
produced. For the method-string form this meant the user handler received
{jsonrpc, id, method, params} instead of the spec-parsed CallToolRequest.
Forward validatedRequest.data in all three wrappers (server tools/call,
client elicitation/create, client sampling/createMessage). Adds a
regression test asserting the handler receives the parsed shape.
| expect((stringRes.error as { message: string }).message).toContain('Invalid'); | ||
| expect(schemaRes.error).toEqual(stringRes.error); |
There was a problem hiding this comment.
🟡 This assertion only checks .toContain('Invalid'), which would also be satisfied by 'Invalid sampling request: …' (client.ts:442, the request-validation path) — so if a future change made {messages:[], maxTokens:1} fail CreateMessageRequestSchema, the test would pass without ever reaching the result-validation wrapper it's meant to guard. The sibling elicitation test (line 57) and the server-side parity test (tightened in d2e046d per the same review feedback) already use the specific result-string; tighten this one to .toContain('Invalid sampling result') for consistency.
Extended reasoning...
What's wrong
The sampling/createMessage parity test asserts:
expect((stringRes.error as { message: string }).message).toContain('Invalid');But the sampling/createMessage wrappedHandler in client.ts has two error paths whose messages both contain 'Invalid':
- Line 442:
'Invalid sampling request: …'— thrown whenparseSchema(CreateMessageRequestSchema, request)fails (request-validation, before the user handler runs). - Line 469:
'Invalid sampling result: …'— thrown when the result failsCreateMessageResult(WithTools)Schema(result-validation, after the user handler runs).
The test's purpose (per its file-level JSDoc and the author's reply 3147452342) is to prove that the result-validation wrapper fires for both registration forms. .toContain('Invalid') cannot distinguish those two paths, so the assertion is satisfiable by the wrong error.
Why this is the same hollow-test pattern already fixed once in this PR
Review comment 3100469350 caught the server-side parity test passing on a pre-handler capability error instead of the result-validation error. The author's fix in d2e046d explicitly says: "tightened assertion to .toContain('Invalid task creation result') so it can't pass on the wrong error." The sibling elicitation test in this same file (line 57) likewise asserts the specific 'Invalid elicitation result'. The sampling case is the only one of the three that was left with the loose 'Invalid' — a partial application of the established fix pattern.
Step-by-step proof
- Today:
{ messages: [], maxTokens: 1 }passesCreateMessageRequestSchema, the handler returns{ role: 'assistant' }, result-validation throws'Invalid sampling result: …', and.toContain('Invalid')passes — the test exercises the right path. - Suppose a future change tightens
CreateMessageRequestSchemato requiremessages.min(1)(or any pre-handler gate is added that throws an'Invalid …'message for this input). - Now both
viaStringandviaSchemaproduce{ error: { message: 'Invalid sampling request: …' } }before the user handler or result-validation ever runs. .toContain('Invalid')✓ on the request-error string.schemaRes.errordeep-equalsstringRes.error✓ (same pre-handler error in both forms). Test passes.- The result-validation wrapper — the thing this regression test exists to guard (so a future refactor can't let the schema form bypass it) — was never reached, and CI gives no signal.
Why nothing prevents it
The test currently does reach the right code path, so CI is green. The looseness only matters under a future change to CreateMessageRequestSchema or the pre-handler pipeline — at which point the regression guard would silently degrade to asserting parity of the wrong layer. Hence nit: this is test-robustness/consistency, not a current correctness bug.
Fix
One-word change to match the sibling tests:
expect((stringRes.error as { message: string }).message).toContain('Invalid sampling result');2 participants
Part of the v2 BC series — see reviewer guide.
Restores the v1 calling forms on
Protocol(inherited byClient/Server) so v1 code keeps working unchanged:setRequestHandler(ZodSchema, handler)/setNotificationHandler(ZodSchema, handler)— method name read from the schema'smethodliteralrequest(req, resultSchema, options?)— explicit-result-schema form, plus method-keyed return type for spec methodscallTool(params, resultSchema?, options?)— accepts the v1 schema arg (ignored) for source compatremoveRequestHandler/removeNotificationHandler/assertCanSetRequestHandleraccept any method stringCustom (non-spec) methods work via the Zod-schema form, same as v1.
schema.ts/standardSchema.tsare unchanged from main.The 3-arg validator-agnostic form is split out as #1916. Exporting
Protocol+ProtocolSpecis #1917.Motivation and Context
v1 → v2 backwards compatibility. The largest single source of v2 typecheck breaks across consumer audits was
setRequestHandlerno longer accepting the Zod-schema form; this restores it as a first-class overload.How Has This Been Tested?
packages/core/test/shared/customMethods.test.ts— Zod-schema round-trip, invalid-params rejection,removeRequestHandler('any/string'), two-arg spec form,notification()mock-assignability.Breaking Changes
None — additive.
Types of changes
Additional context
#1916 and #1917 stack on this.