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 12 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d2e046d 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 |
|
@claude review |
|
@claude review |
|
@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.
1 participant
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.