Uh oh!
There was an error while loading. Please reload this page.
refactor(core): extract Dispatcher; Protocol composes it - #2130
refactor(core): extract Dispatcher; Protocol composes it#2130felixweinberger wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 628f0e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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/nodecommit: |
5b5c538 to
09fc142Comparefelixweinberger
commented
May 21, 2026
@claude review |
c67522f to
ce732aeCompare09fc142 to
315684dCompare| * `Protocol._onrequest` prior to extraction. | ||
| */ | ||
| async dispatch(request: JSONRPCRequest, ctx: ContextT): Promise<JSONRPCResponse | JSONRPCErrorResponse> { |
There was a problem hiding this comment.
🟡 Type-precision nit: Dispatcher.dispatch() declares Promise<JSONRPCResponse | JSONRPCErrorResponse>, but JSONRPCResponse is already the union JSONRPCResultResponse | JSONRPCErrorResponse (schemas.ts:170), so the trailing | JSONRPCErrorResponse is redundant. Likewise okResponse() always builds a success response but is typed as returning the wider JSONRPCResponse union instead of JSONRPCResultResponse. No runtime impact — purely a clarity/precision improvement.
Extended reasoning...
The redundant union.Dispatcher.dispatch() (packages/core/src/shared/dispatcher.ts:159) declares its return type as Promise<JSONRPCResponse | JSONRPCErrorResponse>. But JSONRPCResponse is itself already a union: packages/core/src/types/schemas.ts:170 defines JSONRPCResponseSchema = z.union([JSONRPCResultResponseSchema, JSONRPCErrorResponseSchema]), and spec.types.ts:257 confirms type JSONRPCResponse = JSONRPCResultResponse | JSONRPCErrorResponse. TypeScript collapses A | B where B ⊆ A to just A, so the declared type is exactly equivalent to Promise<JSONRPCResponse> — the | JSONRPCErrorResponse adds nothing.
The over-wide okResponse() type.okResponse() (packages/core/src/shared/dispatcher.ts:195) unconditionally builds { jsonrpc, id, result } — always a success-shaped response. Yet it's typed as returning JSONRPCResponse (the success-or-error union) when the precise type is JSONRPCResultResponse. A caller that wanted to access .result directly would be forced to narrow with a type guard even though the function can never produce an error variant.
Why this isn't a behavior bug. Both signatures are correct in the sense that the actual return value is assignable to the declared type — they're just wider than necessary. Every current caller (Protocol._onrequest in protocol.ts) immediately passes the result to transport.send(), which accepts the full JSONRPCMessage union, so nothing observable changes. The pre-PR _onrequest code also typed its locally-built success object as const response: JSONRPCResponse, so this carries over a pre-existing widening — but since the PR is introducing these new exported helpers in a brand-new file, it's a good moment to get the types exactly right.
Why fix it.okResponse and errorResponse are now exported from packages/core (export * from './shared/dispatcher.js' in index.ts), so they become public-ish API for sibling packages and the upcoming _dispatchStateless path. Tighter return types give downstream callers better narrowing for free and document the contract ("this always succeeds") without needing a comment.
Concrete walk-through.
- A future consumer calls
const r = okResponse(1, { tools: [] }). - They want to read
r.result. With the current signaturer: JSONRPCResponse, TypeScript reportsProperty 'result' does not exist on type 'JSONRPCErrorResponse'and forces anif ('result' in r)guard. - With
r: JSONRPCResultResponse,r.resulttype-checks directly — matching the function's actual guaranteed behavior.
Suggested fix.
asyncdispatch(request: JSONRPCRequest,ctx: ContextT): Promise<JSONRPCResponse>{…}exportfunctionokResponse(id: JSONRPCRequest['id'],result: Result): JSONRPCResultResponse{…}errorResponse is already correctly narrowed to JSONRPCErrorResponse, so only these two annotations need to change. Filed as a nit since there's zero runtime impact.
There was a problem hiding this comment.
Leaving as-is for this PR. The wider types match the pre-existing convention and every caller passes the result straight to transport.send(); will tighten if a downstream caller needs the narrowing.
Uh oh!
There was an error while loading. Please reload this page.
…ences) Removes the 2025-11 experimental tasks side-channel through Protocol: TaskManager, processInbound*/processOutbound*, task interception, the experimental.tasks.* client/server accessors, and all task-augmented request handling. Also extends beyond the implementation deletion to scrub remaining references in examples, docs, and comments. The only task-related symbol remaining in packages/*.ts is `taskSupport` in ToolExecutionSchema, kept solely to match spec.types.ts (which still declares it); both are removed together in the next commit (spec regen). CHANGELOG entries are preserved (historical record). Migration docs retain a brief removal note. `microtask`/`platformBackgroundTask` are JS/platform terminology, not MCP tasks. Satisfies: SEP-2663 (core-removal half; tasks are now Extensions Track).
pnpm fetch:spec-types. Spec moved 44 commits since last regen. Brings: - RequestMetaObject with namespaced io.modelcontextprotocol/* keys (required protocolVersion/clientInfo/clientCapabilities) - Result.resultType: "complete" | "input_required" (required) - DiscoverRequest/Result, InputRequiredResult, InputRequest/Response/s, InputResponseRequestParams - SubscriptionsListenRequest, SubscriptionFilter, SubscriptionsAcknowledgedNotification - UnsupportedProtocolVersionError, MissingRequiredClientCapabilityError (-32003) Removes (moved to extension or removed from spec): - InitializeRequest/Result, InitializedNotification, PingRequest, SetLevelRequest - SubscribeRequest, UnsubscribeRequest, RootsListChangedNotification - All Task* types, taskSupport, TaskAugmentedRequestParams Typecheck broken pending next commit (schemas.ts/guards.ts reference removed types). Isolated for diff clarity per plan §10.
…emas
schemas.ts:
- Result/Notification _meta now use plain MetaObject (not RequestMetaSchema)
- ResultTypeSchema, resultType optional on ResultSchema (BC: pre-2026 omits it)
- RequestMetaSchema kept simple {progressToken?}; namespaced io.modelcontextprotocol/*
keys validated by parseClientMeta in P2, not here (TS2589 depth budget)
- DiscoverRequest/Result, UnsupportedProtocolVersionErrorData,
MissingRequiredClientCapabilityErrorData
- SubscriptionFilter, SubscriptionsListenRequest/Params,
SubscriptionsAcknowledgedNotification/Params
- InputRequest/Response/s, InputRequiredResult, InputResponseRequestParams
- CacheableResult/CacheScope (SEP-2243), PaginatedResult extends CacheableResult,
ReadResourceResult extends CacheableResult
- structuredContent: unknown (was Record<string,unknown>)
- Tool inputSchema/outputSchema loosened to match spec (open record + type:object on input)
- ToolExecution removed (only had taskSupport)
- ClientRequest/ServerNotification/etc unions = 2026 spec methods only
- registerLegacySchemas() lets legacyWireSchemas merge into runtime lookup maps
- Dropped Initialize*/Ping/SetLevel/Subscribe/Unsubscribe/RootsListChanged
NEW legacyWireSchemas.ts:
- LegacyRequestParams/Result/RequestMetaObject base types (no namespaced keys, no resultType)
- TS interfaces + zod for Initialize/Ping/SetLevel/Subscribe/Unsubscribe/RootsListChanged
- legacyRequest/Result/NotificationSchemas maps; registerLegacySchemas() side-effect on import
types.ts: re-exports legacy types; RequestTypeMap/NotificationTypeMap/ResultTypeMap
merge legacy methods. Adds Discover*/InputRequired*/Subscriptions*/Cache* types.
guards.ts: isInitializeRequest/isInitializedNotification import from legacyWireSchemas.
specTypeSchema.ts: allowlist updated (drop legacy, add 2026).
spec.types.test.ts: Relax<T> helper makes spec-required PermissiveKeys
(_meta/resultType/ttlMs/cacheScope/inputResponses/requestState/io.modelcontextprotocol/*)
optional so mutual assignability holds for everything else. Dropped checks for
removed types; added 17 checks for 2026 types. Count 176→150.
types.capabilities.test.ts: import InitializeRequestParamsSchema from legacyWireSchemas.
types.test.ts: outputSchema no longer requires type:object.
mcp.ts: drop ToolExecution/execution (removed from spec).
client.ts: assertCapabilityForMethod switches on RequestMethod (includes legacy).…or change) Introduce `Dispatcher<ContextT>` (`packages/core/src/shared/dispatcher.ts`) holding the request-handler registry, middleware chain, and `dispatch()` entry point. `Protocol` now composes a `protected readonly dispatcher` and delegates: - `setRequestHandler` / `removeRequestHandler` / `assertCanSetRequestHandler` - `fallbackRequestHandler` (now `dispatcher.fallbackHandler`) - `_onrequest` invokes `dispatcher.dispatch(request, ctx)` for lookup, middleware onion, and JSON-RPC response wrapping The previous `_wrapHandler` override hook is replaced by `dispatcher.use()`: - `Server` registers `_callToolResultMiddleware` (was `_wrapHandler` for tools/call) - `Client` registers `_validationMiddleware` (was `_wrapHandler` for elicitation/create + sampling/createMessage) Same validation runs on the same paths; the observation point moves from per-registration wrapping to per-dispatch middleware. `dispatch()`'s catch block preserves `_onrequest`'s error behavior verbatim: thrown errors surface their `code` (if a safe integer), `message`, and `data`. No sanitization is introduced. Tests: - `dispatcher.test.ts` added (11 tests for setHandler/dispatch/middleware/error) - `wrapHandler.test.ts` deleted (covered by rewritten customMethods test) - `customMethods.test.ts` adapts one test from spying on `_wrapHandler` to spying via middleware; same assertion (both 2-arg and 3-arg routes through) No existing test assertions changed.
ce732ae to
29c4991Compare315684d to
628f0e1Comparefelixweinberger
commented
May 21, 2026
@claude review |
| /** The handler registry. Both `_onrequest` and `_dispatchStateless` route through it. */ | ||
| protected readonly dispatcher = new Dispatcher<ContextT>(); |
There was a problem hiding this comment.
🟡 Several new comments and JSDoc blocks reference _dispatchStateless and "the 2026-06 dispatch path" — neither of which exists in this PR or the codebase yet (they are planned for #2131). A reader who greps for _dispatchStateless after merge will find nothing. Consider qualifying the references (e.g. "and the forthcoming _dispatchStateless path (#2131)") or deferring them to the PR that introduces the second path. Affects protocol.ts:279, dispatcher.ts:55, client.ts:288, and server.ts:182.
Extended reasoning...
What the issue is. Four new comments added by this PR reference code that does not exist yet:
packages/core/src/shared/protocol.ts:279—/** The handler registry. Both \_onrequest` and `_dispatchStateless` route through it. */`packages/core/src/shared/dispatcher.ts(class JSDoc, ~line 55) — "Both the legacy connect/_onrequest path and the 2026-06 stateless dispatch path route through dispatch."packages/client/src/client/client.ts:288— "…so it applies to both the legacy `_onrequest` path and the 2026-06 dispatch path."packages/server/src/server/server.ts:182— "…so it applies to both the legacy `_onrequest` path and the 2026-06 dispatch path."
_dispatchStateless appears nowhere in the codebase except inside the comment at protocol.ts:279, and there is no "2026-06 dispatch path" implemented anywhere. These are forward references to PR #2131.
Why it matters. The repo's own review guidance flags "prose that promises behavior the code no longer ships" and "any claim the diff doesn't back." Once this PR merges, a contributor reading Protocol who sees "Both `_onrequest` and `_dispatchStateless` route through it" and greps for _dispatchStateless will find zero hits. They have no way to know whether the symbol was renamed, removed, or never landed. _onrequest already calls _onrequest legacy — there is no "legacy" vs. "new" distinction observable in the merged code, so the dichotomy the comments draw doesn't exist for a reader of the codebase at this commit.
Step-by-step proof.
- Merge refactor(core): extract Dispatcher; Protocol composes it #2130 in isolation.
- Open
packages/core/src/shared/protocol.tsat line 279:/** The handler registry. Both \_onrequest` and `_dispatchStateless` route through it. */` - Run
grep -rn _dispatchStateless packages/→ only the comment itself matches. There is no method, no export, no test. - Open
packages/server/src/server/server.ts:182: "…applies to both the legacy `_onrequest` path and the 2026-06 dispatch path." → there is exactly one path,_onrequest. Nothing distinguishes it as "legacy".
Why this is a nit and not a blocker. The PR description openly states this is PR 3 of a 7-PR v2-stateless stack, and that #2131 ("HTTP-stateless") introduces _dispatchStateless. The author and reviewers have full stack context, the comments are accurate as a description of intent, and they will become literally true once #2131 merges. There is zero runtime impact. But comments live in the merged tree, not in the PR description — and PRs in a stack don't always all land, or land in order.
How to fix. Either qualify the references so they are true at merge time, e.g.:
/** The handler registry. \`_onrequest\` routes through it (and the forthcoming \`_dispatchStateless\` path, #2131). */or describe only what exists today and let #2131 update the comments when it adds the second path:
/** The handler registry. \`_onrequest\` routes through it. */The first option preserves the useful forward signal for reviewers of this stack; the second is the most conservative.
29c4991 to
751ffcbCompare
Extracts
Dispatcher<Ctx>(handler map + middleware chain +dispatch()) fromProtocol.Protocolnow composes one;setRequestHandler/_onrequest/fallbackRequestHandlerdelegate. The_wrapHandleroverride hook becomesdispatcher.use(middleware).Zero behavior change.
dispatch()'s catch block preserves_onrequest's error mapping verbatim. No existing test assertions changed.Motivation and Context
Enabling refactor for SEP-2575:
_dispatchStateless(next PR) needs to invoke the same handler map + middleware that legacy_onrequestuses, without going through Protocol's session machinery.How Has This Been Tested?
pnpm test:all— count delta isdispatcher.test.ts(+11) andwrapHandler.test.ts(−1, redundant). No existing assertion changed.Breaking Changes
Protocol._wrapHandleroverride hook removed (was protected). Subclasses usethis.dispatcher.use(mw)instead.Types of changes
Checklist