refactor(core): _wrapHandler hook so subclasses don't redeclare setRequestHandler#1976
Conversation
🦋 Changeset detectedLatest commit: e69c6a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…claring setRequestHandler
felixweinberger
force-pushed
the
fweinberger/1974a-wraphandler
branch
from
8f12176 to
e69c6a7
Compare
There was a problem hiding this comment.
LGTM — both earlier points are addressed (base return type tightened to ResultTypeMap[M], out-of-scope string widenings reverted), and the remaining diff is a behavior-preserving move of the wrapping logic into the new hook.
Extended reasoning...
Overview
This PR introduces a protected _wrapHandler(method, handler) hook on Protocol (identity by default) and has setRequestHandler route the stored closure through it. Client and Server now override _wrapHandler instead of redeclaring the public setRequestHandler signature; the validation bodies for elicitation/create, sampling/createMessage, and tools/call are moved verbatim (modulo reindent and await handler(...) replacing await Promise.resolve(handler(...)), which is equivalent since stored already returns a Promise). A new wrapHandler.test.ts asserts the hook is invoked per registration, and a changeset is included.
Follow-up on prior review
My two earlier comments are resolved in e69c6a7:
- The base
setRequestHandlerhandler return type is nowResultTypeMap[M] | Promise<ResultTypeMap[M]>, restoring the compile-time contract that the removed subclass overrides previously provided. - The four
assertCapabilityForMethod/assertNotificationCapabilityparameter widenings tostringwere reverted; both subclasses again match the abstract declarations inProtocol.
I traced the one ordering difference: previously the subclass wrapper received the already-schema.parsed request and re-parsed it; now _wrapHandler receives the raw JSONRPCRequest and parses it, then stored parses again before calling the user handler. Since getRequestSchema(method) and the explicit CallToolRequestSchema/ElicitRequestSchema/CreateMessageRequestSchema are the same loose schemas, the user handler still receives an identically-shaped parsed object, and the internal params.mode defaulting was never visible to the user handler before either (it mutated a separate parse result). Net runtime behavior is unchanged.
Security risks
None. No transport, auth, or I/O paths are touched; this only reorganizes where in-process handler wrapping happens. The validation that was applied before is still applied, on the same inputs, with the same error codes.
Level of scrutiny
Moderate — Protocol.setRequestHandler is public API on the core package, but the only public-facing change is the tightening of the handler return type on the base class to match what Client/Server already enforced, which is strictly safer for consumers. _wrapHandler is protected, so it's not new public surface. The rest is mechanical code movement covered by existing client/server validation tests plus the new hook test.
Other factors
The Client class JSDoc absorbs the example that used to live on the removed override, so doc coverage is preserved. No bugs from the bug-hunting pass. This is now a tightly-scoped base PR for #1974 as described.
There was a problem hiding this comment.
LGTM — both earlier comments addressed (base return type tightened to ResultTypeMap[M], capability-assert widenings reverted), and the remaining diff is a scoped hook refactor with identical runtime behavior.
Extended reasoning...
Overview
Introduces protected Protocol._wrapHandler(method, handler) (default: identity) and routes setRequestHandler through it before storing. Client and Server override _wrapHandler instead of redeclaring setRequestHandler; their validation bodies for elicitation/create / sampling/createMessage / tools/call are moved verbatim into the hook. The base setRequestHandler handler return type is tightened from Result to ResultTypeMap[M] to preserve the type contract previously enforced by the deleted overrides. A small vitest asserts the hook is invoked, plus a changeset.
Follow-up on prior review
I raised two issues on the first revision; both are fixed in the current diff and the threads are resolved:
- 🔴 Handler return-type loosening → base signature now uses
ResultTypeMap[M] | Promise<ResultTypeMap[M]>(protocol.ts), restoring the prior compile-time contract forClient/Servercallers. - 🟡 Out-of-scope widening of
assertCapabilityForMethod/assertNotificationCapabilitytostring→ reverted; both subclasses again takeRequestMethod/NotificationMethod, matching the base abstract declarations.
Security risks
None. No transport, auth, or I/O changes; this only restructures where the existing validation closures are assembled. The wrapped handler still runs the same Zod request/result parses with the same ProtocolError codes.
Level of scrutiny
Medium — it touches the core request-handler registration path used by every server/client, but the change is mechanical (override → hook) and the validation bodies are byte-for-byte moves apart from await handler(...) replacing await Promise.resolve(handler(...)) (equivalent, since stored already returns a Promise). The execution order of base-schema parse vs. role-specific parse is inverted relative to before, but both still run on the same raw request and the user handler still receives the base-parsed payload, so observable behavior is unchanged.
Other factors
_wrapHandlerisprotectedand underscore-prefixed, so no new public export surface.- Constructor-time
setRequestHandler('ping' | 'initialize' | 'logging/setLevel')calls now flow through the subclass_wrapHandler, which correctly returns identity for those methods. - Tightening the base return type is a strict improvement (direct
Protocolcallers now get the same checking subclasses had) and existing internal callsites (ping,initialize,logging/setLevel, the new test) all satisfy it. - Bug-hunting pass found nothing on this revision;
pnpm test:allreported green by the author.
2 participants
Replace
Client/ServersetRequestHandleroverrides with a protected_wrapHandlerhook.Motivation and Context
On main,
ClientandServereach overridesetRequestHandlerto wrap registered handlers with validation (sampling/createMessage,elicitation/create,tools/call). That means redeclaring the public method's signature and JSDoc in two subclasses, which goes stale when the base signature changes.This PR adds
protected _wrapHandler(method, handler): handlertoProtocol(default: identity), called insidesetRequestHandlerbefore storing. Subclasses override_wrapHandlerinstead of the public method. Same behavior, no public API change. Base PR for #1974.How Has This Been Tested?
pnpm test:allgreen. NewwrapHandler.test.tsasserts the hook is called for registered handlers. Existing client/server validation tests pass unchanged.Breaking Changes
None.
Types of changes
Checklist
Additional context
Pure refactor extracted from #1974 so the custom-method overload PR can stack on it without subclasses redeclaring the overload set.