Repository navigation
feat: server and client extensions ({ id, setup(ctx) }), request middleware (use), acceptResultType - #2820
feat: server and client extensions ({ id, setup(ctx) }), request middleware (use), acceptResultType#2820mattzcarey wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: ed2a77c 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/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
mattzcarey
changed the title
mattzcarey
changed the title
mattzcarey
changed the title
e056cf6 to
af8889b
Compare
claude
Bot
added
the
v2
af8889b to
e17337b
Compare
e17337b to
be40c1e
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 optional notes from this repository's REVIEW.md or CLAUDE.md checks were not posted as comments, over this review's limit for such notes; they are on this commit's check card.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/core-internal/src/shared/protocol.ts— Client extensions that wrapelicitation/create(orsampling/createMessage,roots/list) withusesilently get no middleware on a 2026-07-28 connection. On the modern era those requests arrive embedded ininput_requiredresults and are dispatched through_getRequestHandlerat protocol.ts:747, which reads_requestHandlersdirectly and skips the middleware stack. Fix: resolve every dispatch path through_resolveRequestHandlersousemiddleware composes around embedded input requests too, which also covers thegetRequestHandlerhost hook in client.ts:752. Only legacy_onrequesttraffic composes middleware today, andClient._shouldDropInbounddrops modern top-level requests, so on 2026-07-28 the middleware never runs. [also at: packages/core-internal/src/shared/protocol.ts:1896 - Client extension middleware onelicitation/createis skipped for embedded input requests on 2026-07-28 connections, so a gate or rewrite it installs silently does not run there.]Why this was flagged
A user installs a ClientExtension that calls
client.use('elicitation/create', mw)as the JSDoc in packages/client/src/client/extension.ts:15 describes, and connects on protocol revision 2026-07-28. The server answers tools/call with an input_required result carrying an embedded elicitation/create request; the Client's_resolveNonCompleteResult(packages/client/src/client/client.ts:749-752) runsrunInputRequiredFlow, which callshost.getRequestHandler(method)which isProtocol._getRequestHandler(protocol.ts:746-747) returningthis._requestHandlers.get(method). Only_onrequestat protocol.ts:1037 uses the new_resolveRequestHandler, so the middleware array is never consulted here andmwnever runs; the raw registered handler answers. On the modern eraClient._shouldDropInbound(client.ts:690-696) drops any top-level inbound request, so there is no path on which the middleware applies at all. The base branch has nouse, so the regression is that the new API advertises a hook that is a no-op on the default modern negotiation.Verification: The pre-existing
_getRequestHandlerat protocol.ts:746-748 is untouched and still doesreturn this._requestHandlers.get(method);.dispatchInputRequestin packages/core-internal/src/shared/inputRequiredEngine.ts:145-163 calls that handler directly, so middleware registered viausefor those methods is never invoked on the embedded path. On 2026-07-28 this is the only path for those methods.
| // registered middleware ends up outermost. | ||
| for (const { middleware } of stack.toReversed()) { | ||
| const next = composed; | ||
| composed = async (request, ctx) => middleware(request, ctx, next); |
There was a problem hiding this comment.
🔴 Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the requestState.verify hook resolved with, as the accessor's JSDoc promises. Middleware composed at protocol.ts:1893 runs outside the wrapped handler, and the verify hook only runs inside it at server.ts:634-645. A middleware that gates or branches on that state therefore acts on attacker-controlled input even when HMAC verification is configured. Fix: run the requestState type check and verify hook before the middleware stack for every method use() can wrap, or make the accessor return undefined until verified and state in the use() JSDoc that middleware sees pre-verification context.
Why this was flagged
A server configures ServerOptions.requestState.verify and installs an extension whose middleware on tools/call reads ctx.mcpReq.requestState() to decide whether to answer itself or call next. A client retries a multi-round-trip request with a tampered requestState string. In _onrequest the context is built with requestStateAccessor(lifted.requestState), the raw string, at protocol.ts:1099, and handler(request, ctx) at protocol.ts:1130 is the composed middleware from protocol.ts:1893. Only when the middleware calls next does Server._wrapHandler reach _invokeInputRequiredCapableHandler, which runs _verifyRequestState at server.ts:634-645 and hands the decoded payload to the handler via withRequestStateValue at server.ts:643. The middleware therefore sees the unverified string while the accessor's JSDoc at protocol.ts:404-420 says it returns 'the value the configured ServerOptions.requestState.verify hook resolved with'. On the base branch there is no middleware seam, so every reader of requestState sat behind the verify hook.
Verification: _onrequest builds the context with the raw lifted string at packages/core-internal/src/shared/protocol.ts:1099. _resolveRequestHandler (:1880-1895) composes middleware around the _wrapHandler-wrapped handler; the verify hook runs only inside that wrapped handler, in Server._invokeInputRequiredCapableHandler at packages/server/src/server/server.ts:633-645. The base branch has no middleware path.
| for (const extension of options?.extensions ?? []) { | ||
| this.registerCapabilities({ extensions: { [extension.id]: {} } }); |
There was a problem hiding this comment.
🔴 Operators who configure an extension's settings in capabilities.extensions[id] and also list that extension in extensions silently lose those settings. server.ts:364 calls registerCapabilities({ extensions: { [extension.id]: {} } }), and mergeCapabilities (protocol.ts:2041) merges one level deep, so {} replaces whatever the user put under that id. Fix: only advertise {} when this._capabilities.extensions?.[extension.id] is not already set (or deep-merge that key), at both sites: server.ts:364 and client.ts:671. The docs say the maps merge; for this id they are overwritten. [also at: packages/client/src/client/client.ts:671 - Client users who pass extension settings in options.capabilities.extensions[id] and also list that extension in options.extensions lose those settings: the client advertises {} instead. client.ts:671 registers { extensions: { [extension.id]: {} } } after the constructor copied the user's capabilities, and mergeCapabilities merges only one level deep, so the {} replaces the user's entry.]
Why this was flagged
A user constructs new Server(info, { capabilities: { extensions: { 'com.example/gate': { exampleData: true } } }, extensions: [gate] }) (or the same on Client at client.ts:671). The constructor loop at server.ts:363-366 calls registerCapabilities({ extensions: { 'com.example/gate': {} } }). mergeCapabilities at protocol.ts:2048-2051 spreads { ...baseValue, ...addValue } only at the extensions level, so the 'com.example/gate' entry becomes {} and exampleData is gone from the advertised capability. On the base branch extensions is not a ServerOptions key and capabilities.extensions passed by the user is advertised unchanged. docs/advanced/extensions.md:44 says settings 'merge', which holds only when install registers them after the {} write, not for settings given in capabilities.
Verification: When a user passes both capabilities.extensions[id]: {...settings} and extensions: [ext] with the same id at construction, the settings are silently dropped from the advertised capabilities. server.ts:363-366 calls this.registerCapabilities({ extensions: { [extension.id]: {} } }), and mergeCapabilities (packages/core-internal/src/shared/protocol.ts:2041-2054) merges exactly one level deep. No guard or warning exists.
| * Installs middleware on the request handler for `method`. The middleware | ||
| * receives the parsed request, the context, and `next` — the handler it |
There was a problem hiding this comment.
🟡 (optional) Extension authors who trust the use JSDoc get a middleware that receives an unparsed, unvalidated request, so reading request.params.name on a malformed tools/call throws and the client gets -32603 instead of the -32602 the base answers. protocol.ts:1811-1812 says the middleware 'receives the parsed request', but protocol.ts:1893 runs it outside the stored handler and the server.ts:549 validateRequest step, so params may be undefined or any shape. Fix: either validate the request against the method's params schema before the middleware stack runs, or make the JSDoc and docs say the request is raw and must be checked by the middleware. [also at: packages/core-internal/src/shared/protocol.ts:1893 - Clients of a server with a tools/call extension can receive results the base branch never puts on the wire: unvalidated bodies, and input_required results that skip the capability check and the 2025 legacy shim. protocol.ts:1884-1893 composes middleware outside the stored handler, so a middleware that answers or rewrites skips the whole server.ts:498 _wrapHandler seam.]
Why this was flagged
Trigger: any server with an extension middleware on tools/call whose body reads request fields, which the JSDoc invites by calling the request 'parsed'; a client sends tools/call with missing or malformed params. The middleware at protocol.ts:1893 gets the raw JSONRPCRequest straight from _onrequest; the only prior checks are the era gate (protocol.ts:1028) and the envelope check (protocol.ts:1053), neither of which validates params. Request-shape validation for tools/call happens inside the wrapped handler at server.ts:549, i.e. only once next is called. On the base branch a malformed tools/call reaches validateRequest first and answers -32602 'Invalid tools/call request'. After the change a middleware that dereferences request.params.name throws a TypeError, which the error half at protocol.ts:1172 maps to -32603 Internal error, and the throw never reaches the base handler. Remedy: run method params validation before composing middleware, or correct the JSDoc and docs to say the request is unvalidated.
Verification: The use JSDoc at protocol.ts:1811-1812 says the middleware "receives the parsed request". But _resolveRequestHandler composes the middleware OUTSIDE the stored handler (protocol.ts:1893), and all schema validation lives inside next, for tools/call at server.ts:549-556, which throws -32602. A TypeError thrown from middleware carries no code, so protocol.ts:1172 maps it to -32603, whereas the base answers -32602.
| let composed: (request: JSONRPCRequest, ctx: ContextT) => Promise<Result> = | ||
| base ?? | ||
| (async () => { | ||
| throw new ProtocolError(ProtocolErrorCode.MethodNotFound, 'Method not found'); | ||
| }); |
There was a problem hiding this comment.
🟡 (optional) Operators hosting with createMcpHandler now answer an unknown method with HTTP 200 instead of 404 once an extension installs middleware on it. protocol.ts:1884-1888 turns a method with middleware but no handler into a handler that throws MethodNotFound asynchronously, so the synchronous -32601 at protocol.ts:1040 never fires. The per-request transport maps -32601 to 404 only inside its dispatch window (perRequestTransport.ts:290), and the envelope check at protocol.ts:1053 now runs first, so a missing envelope answers -32602. Fix: keep -32601 for middleware-only methods on the synchronous pre-dispatch path, e.g. return undefined from _resolveRequestHandler when base is undefined, and say in the use() JSDoc that middleware needs an underlying handler. [also at: packages/core-internal/src/shared/protocol.ts:1037 - 2026-07-28 clients that call a method an extension wraps but no handler serves now get -32602 instead of the base's -32601 when their request lacks the _meta envelope. protocol.ts:1037 returns a composed handler whenever middleware exists, so the handler-existence check at protocol.ts:1039 no longer fires and the envelope check at protocol.ts:1053 answers first.]
Why this was flagged
A server is built with an extension whose install calls server.use('tools/call', mw) but no tool is registered and capabilities.tools is not declared, so no tools/call handler exists; it is hosted via createMcpHandler on 2026-07-28, which dispatches through PerRequestHTTPServerTransport (invoke.ts:61). A client POSTs tools/call. On the base branch _onrequest finds no handler and calls sendErrorResponse(MethodNotFound) synchronously at protocol.ts:1040, inside the transport's dispatch window (perRequestTransport.ts:216-220), so LADDER_ERROR_HTTP_STATUS maps -32601 to HTTP 404. After the change _resolveRequestHandler at protocol.ts:1881-1888 returns a composed function whose base is an async thrower, so handler is defined; the -32601 is thrown from inside the Promise chain at protocol.ts:1162-1182 after the window closed, ladderStatus at perRequestTransport.ts:290 is undefined, and the error is settled with status 200 at perRequestTransport.ts:311. The envelope check at protocol.ts:1053 also now precedes the not-found answer, so the same request without a _meta envelope gets -32602 instead of -32601.
Verification: Triggered when an extension installs middleware via use() on a method that has no registered handler and the server is hosted via createMcpHandler. _resolveRequestHandler (protocol.ts:1880-1894) returns a composed function, so the handler === undefined branch at protocol.ts:1039 is skipped and the -32601 is produced after the window closed, landing on HTTP 200 at perRequestTransport.ts:311.
| if ( | ||
| error instanceof ProtocolError && | ||
| (error.code === ProtocolErrorCode.UrlElicitationRequired || | ||
| error.code === ProtocolErrorCode.MissingRequiredClientCapability) | ||
| ) { | ||
| throw error; // Protocol-level: return the error to the caller without wrapping in CallToolResult |
There was a problem hiding this comment.
🟡 (optional) Gateway-style servers whose tools call a downstream MCP server now relay the downstream's -32021 to their own client as a JSON-RPC error instead of an isError result. mcp.ts:317-323 re-throws any ProtocolError with code MissingRequiredClientCapability, but a tool that awaits a downstream callTool receives that peer's -32021 rebuilt by ProtocolError.fromError (errors.ts:93-100), so the upstream client is told it lacks a capability the gateway's own client lacked, with HTTP 400 from the per-request transport. Fix: re-throw only a -32021 this server can vouch for, e.g. check error.requiredCapabilities against the request's declared client capabilities (ctx.mcpReq.envelope) and keep the createToolError wrap when they are already declared.
Why this was flagged
A server built on McpServer registers a tool that proxies to another MCP server through a Client and awaits client.callTool(...). The downstream server answers the proxy's call with a JSON-RPC error -32021. Client _onresponse (packages/core-internal/src/shared/protocol.ts:1244) turns that into a MissingRequiredClientCapabilityError via ProtocolError.fromError (packages/core-internal/src/types/errors.ts:93-100) and the tool callback rejects with it. On the base branch the catch at mcp.ts:316 only passes through UrlElicitationRequired, so the upstream client got an isError CallToolResult. After the change mcp.ts:317-323 re-throws it, the upstream client receives error -32021 with the downstream's requiredCapabilities in data, and on PerRequestHTTPServerTransport (perRequestTransport.ts:288-296) that code is answered with HTTP 400 even though the upstream client declared everything this server needs. No safeguard distinguishes a -32021 minted by this server's gates from one relayed from a peer: the check is on the bare code.
Verification: Client _onresponse rebuilds the wire error with ProtocolError.fromError (packages/core-internal/src/shared/protocol.ts:1244), yielding a MissingRequiredClientCapabilityError; the new branch at packages/server/src/server/mcp.ts:317-322 forwards it as a JSON-RPC error. On base the same error became an in-band isError tool result via createToolError.
| }); | ||
| // Wrap from the innermost (last registered) outwards so the first | ||
| // registered middleware ends up outermost. | ||
| for (const { middleware } of stack.toReversed()) { |
There was a problem hiding this comment.
🟡 (optional) If any consumer runs the client or server package on a runtime without ES2023 array methods, every inbound request on a method with middleware throws a TypeError at dispatch instead of being answered. protocol.ts:1891 calls stack.toReversed() synchronously inside _onrequest, before the Promise chain that maps errors to JSON-RPC responses, so the throw escapes into the transport's onmessage. The packages are built with lib: esnext (common/tsconfig/tsconfig.json:4) and ship a browser export (packages/client/package.json:74), and engines.node >= 20 says nothing about browsers. Fix: use [...stack].reverse() or iterate the array backwards so no ES2023 built-in is required on the request path.
Why this was flagged
When the client or server runs on an engine lacking Array.prototype.toReversed and an extension has installed middleware with use, the first request for that method reaches protocol.ts:1037 and _resolveRequestHandler evaluates stack.toReversed() at protocol.ts:1891. That call runs synchronously in _onrequest, outside the Promise.resolve().then(...) chain at protocol.ts:1129 that converts handler errors into error responses, so the TypeError propagates out of _onrequest into the transport's message callback and the request is never answered; the peer waits until its timeout. On the base branch no ES2023 built-in sat on the dispatch path. engines.node >= 20 does not constrain browser or React Native consumers; common/tsconfig/tsconfig.json:3-4 sets target/lib to esnext so nothing downlevels the call. Remedy: reverse with [...stack].reverse() or a descending index loop.
Verification: packages/core-internal/src/shared/protocol.ts:1891 calls stack.toReversed() inside _resolveRequestHandler, which _onrequest calls synchronously at protocol.ts:1037, before the Promise chain that maps handler errors to JSON-RPC error responses. _onrequest runs from the transport's onmessage wrapper at protocol.ts:832 with no try/catch, so the TypeError escapes and the request is never answered.
| // `resultType` the caller declared for this method bypasses | ||
| // the codec's closed vocabulary and reaches the caller's | ||
| // schema as-is, discriminator included. | ||
| if (this._isAcceptedResultType(request.method, response.result)) { |
There was a problem hiding this comment.
🟡 nit (optional): the JSDoc and docs say only the explicit-schema request() path consults acceptResultType, but the code applies it to typed spec calls too. The check at protocol.ts:1552 lives in _requestWithSchemaViaCodec, shared by request({method}) without a schema and callTool. After an extension declares acceptResultType('tools/call','task'), a typed callTool receiving resultType: 'task' skips the codec and fails with InvalidResult instead of the codec's typed unknown-kind error. Fix: either gate the bypass on the explicit-schema entry (pass a flag from protocol.ts:1305) or correct protocol.ts:1849-1851 and docs/advanced/extensions.md:84 to say typed calls are affected too. [also at: packages/core-internal/src/shared/protocol.ts:1561 - Typed spec callers also hit the acceptResultType bypass, so the JSDoc, docs and changeset saying only the explicit-schema path consults it are false.; packages/core-internal/src/shared/protocol.ts:1849 - nit: CLAUDE.md asks that comments say what the code does: the acceptResultType JSDoc says "Only the explicit-schema request(request, resultSchema) path consults this; typed spec calls keep the closed vocabulary", but both request() overloads reach _requestWithSchemaViaCodec, where _isAcceptedResultType runs before codec.decodeResult regardless of overload.]
Why this was flagged
The JSDoc at packages/core-internal/src/shared/protocol.ts:1849-1851 states 'Only the explicit-schema request(request, resultSchema) path consults this; typed spec calls keep the closed vocabulary', and docs/advanced/extensions.md:84 repeats it. The _isAcceptedResultType check at protocol.ts:1552 is inside the response handler of _requestWithSchemaViaCodec, which protocol.ts:1312 also reaches for typed calls with codecResultValidator. So once acceptResultType('tools/call', 'task') is declared, a typed client.callTool whose raw response carries resultType: 'task' bypasses codec.decodeResult and is validated raw against the spec schema, producing SdkErrorCode.InvalidResult 'Invalid result for tools/call' (the PR's own test at packages/client/test/client/extensions.test.ts:126-127 shows this). On the base branch the 2026 codec answers that response with its typed unknown-kind error. CLAUDE.md principle 6 ('The words are true') asks that comments and docs say what the code does.
Verification: The JSDoc at packages/core-internal/src/shared/protocol.ts:1849-1851 says only the explicit-schema request() path consults this, but the _isAcceptedResultType check at protocol.ts:1552 sits inside _requestWithSchemaViaCodec, which typed request() calls also reach. A typed callTool receiving resultType: 'task' thus skips codec.decodeResult and fails with InvalidResult instead of the base's UnsupportedResultType.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 7 findings from earlier reviews are still open above.
1 optional suggestion (a nit or a note on pre-existing code) was found and not posted.
Still open from earlier reviews (7):
- 🔴
packages/core-internal/src/shared/protocol.ts:1893—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - 🔴
packages/server/src/server/server.ts:364—Operators who configure an extension's settings in capabilities.extensions[id] and also list that extension in extensio… - Also unresolved: 5 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
be40c1e to
09c01f2
Compare
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 7 findings from earlier reviews are still open above.
1 optional suggestion (a nit or a note on pre-existing code) was found and not posted.
Still open from earlier reviews (7):
- 🔴
packages/core-internal/src/shared/protocol.ts:1901—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - 🔴
packages/server/src/server/server.ts:368—Operators who configure an extension's settings in capabilities.extensions[id] and also list that extension in extensio… - Also unresolved: 5 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
09c01f2 to
96868ae
Compare
mattzcarey
changed the title
There was a problem hiding this comment.
Beyond the inline finding, I also checked the new filesystem surface the Skills extension adds in packages/server/src/ext/skills/fsPath.ts and node.ts for path traversal: splitRelativePath rejects .. and empty segments, fsPathWithin re-checks containment with path.relative after the join, and node.ts opens with O_NOFOLLOW so a final-component symlink cannot escape the skills folder.
Extended reasoning...
Since the last review the PR grew from the generic extension hooks to also include the full Skills extension (SEP-2640), which serves files from a server-side folder over skill:// URIs and resources/directory/read; that path-containment guard was the new security-sensitive surface examined this run and it holds, so this note records only that ruled-out check.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (7):
- 🔴
packages/core-internal/src/shared/protocol.ts:1901—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - Also unresolved: 5 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| export { PerRequestHTTPServerTransport } from './server/perRequestTransport'; | ||
| // Opt-in HMAC sealing for the multi-round-trip requestState (SEP-2322): the | ||
| // convenience codec consumers drop into ServerOptions.requestState.verify. | ||
| export type { ServerExtension } from './server/extension'; |
There was a problem hiding this comment.
🟡 nit (optional): maintainers reading the server barrel now see the SEP-2322 HMAC-sealing comment attached to the unrelated ServerExtension export. The new export type { ServerExtension } at packages/server/src/index.ts:59 was inserted between the comment at lines 57-58 and the RequestStateCodec exports it describes. Fix: move the ServerExtension export above the comment (or give it its own one-line comment) so the comment sits on the requestStateCodec exports it is about.
Why this was flagged
The diff adds export type { ServerExtension } from './server/extension'; at packages/server/src/index.ts:59, directly after the two-line comment at index.ts:57-58 that reads 'Opt-in HMAC sealing for the multi-round-trip requestState (SEP-2322): the convenience codec consumers drop into ServerOptions.requestState.verify.' That comment describes RequestStateCodec, RequestStateCodecOptions and createRequestStateCodec at index.ts:60-61, which the insertion pushes away from it. On the base branch the comment is immediately followed by the exports it describes. No runtime behaviour changes; CLAUDE.md principle 6 asks that comments say what the code does, and this one now labels the wrong export.
Verification: nit. The only hunk in packages/server/src/index.ts inserts export type { ServerExtension } from './server/extension'; at line 59, directly beneath the comment at lines 57-58 and above the RequestStateCodec exports (lines 60-61) that comment describes. After the change the comment reads as describing ServerExtension, which has nothing to do with HMAC sealing.
Finding ID: f-56f8ec5d
96868ae to
55c03c8
Compare
mattzcarey
changed the title
There was a problem hiding this comment.
2 other optional suggestions (nits or notes on pre-existing code) were found and not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (8):
- 🔴
packages/core-internal/src/shared/protocol.ts:1901—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - Also unresolved: 6 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| if (codecForVersion(MODERN_WIRE_REVISION).hasRequestMethod(name)) { | ||
| throw new Error(`Extension "${id}" cannot register "${name}": it is a core method; wrap it with use() instead`); |
There was a problem hiding this comment.
🔴 Extension authors can register a handler for a core method such as elicitation/create through ctx.method, replacing the SDK's typed handler, although the JSDoc and docs say it throws. The core-method check at packages/client/src/client/extension.ts:161 only asks the 2026-07-28 codec, whose registry holds no client-served methods and omits legacy-only server methods. Fix: refuse every spec method of any era (an isSpecRequestMethod-style check across both codecs), while still allowing the SEP-2663 reused names tasks/get and tasks/cancel that the Tasks extension registers with explicit schemas, which covers the 2 sites listed. Same pattern at 2 sites (packages/client/src/client/extension.ts:161, packages/server/src/server/extension.ts:189). [also at: packages/server/src/server/extension.ts:188 - Extension authors who call ctx.method with a legacy-only core method name get it registered instead of the refusal the JSDoc and docs promise.]
Why this was flagged
A client built with new Client(info, { capabilities: { elicitation: {} }, extensions: [ext] }) whose setup calls ctx.method('elicitation/create', { params: z.looseObject({}) }, handler). The guard at packages/client/src/client/extension.ts:161 calls codecForVersion(MODERN_WIRE_REVISION).hasRequestMethod(name); the 2026 registry at packages/core-internal/src/wire/rev2026-07-28/registry.ts:45-56 lists only ten server-side methods, so elicitation/create passes. client.assertCanSetRequestHandler at extension.ts:164 only fails when a handler already exists, and Client.assertRequestHandlerCapability at packages/client/src/client/client.ts:1558 passes once the capability is declared, so client.setRequestHandler('elicitation/create', schemas, handler) installs a custom-schema handler for a spec method. The JSDoc at extension.ts:81-84 and docs/advanced/extensions.md:19 promise a throw for a core method. The PR's own test only exercises tools/call on the client (packages/client/test/client/extensions.test.ts:106), which the client never serves.
Verification: packages/client/src/client/extension.ts:160 guards only with codecForVersion(MODERN_WIRE_REVISION).hasRequestMethod(name); that registry (packages/core-internal/src/wire/rev2026-07-28/registry.ts:45-56) holds only ten server-served methods. So with capabilities: { elicitation: {} }, elicitation/create passes the core check and is installed, contrary to the JSDoc (extension.ts:80-83).
Finding ID: f-fc33ef97
| ctx.use('tools/call', (request, requestCtx, next) => { | ||
| const declared = requestCtx.mcpReq.envelope?.[CLIENT_CAPABILITIES_META_KEY]?.extensions ?? {}; | ||
| if (!(GATE in declared)) { | ||
| throw new MissingRequiredClientCapabilityError({ requiredCapabilities: { extensions: { [GATE]: {} } } }, 'declare the gate'); | ||
| } | ||
| return next(request, requestCtx); | ||
| }); |
There was a problem hiding this comment.
🟡 nit (optional): readers who copy the documented gate extension get a server that refuses every 2025-era client, even one that declared the extension in initialize. The example at docs/advanced/extensions.md:48 reads only requestCtx.mcpReq.envelope, which a legacy request never carries, so the -32021 throw at line 50 fires on every legacy tools/call. The same guide promises legacy clients advertise the extension in initialize (line 98). Fix: make the example era-aware, falling back to ctx.server.getClientCapabilities() when the envelope is absent, or state that the gate only serves 2026-07-28 connections.
Why this was flagged
A server built from the docs example installs gate and is connected on a 2025-era session. A 2025 client built with this SDK and extensions: [gateClient] sends capabilities.extensions['com.example/gate'] in initialize (packages/client/src/client/client.ts:1095), as docs/advanced/extensions.md:98 says it does. On a legacy request _onrequest sets mcpReq.envelope only when the request carried reserved _meta keys (packages/core-internal/src/shared/protocol.ts:1089), so requestCtx.mcpReq.envelope is undefined and declared is {} at docs/advanced/extensions.md:48. The middleware throws MissingRequiredClientCapabilityError at line 50 and the client gets -32021 for every tools/call. The server's own gates use the era-aware _inputRequestCapabilityView (packages/server/src/server/server.ts:742-746); that view is private, and the example neither uses the public getClientCapabilities() nor says it is 2026-only. On the base branch there is no extension guide, so no reader is steered into this.
Verification: nit — triggered whenever a server built from the documented gate example serves a 2025-era client. docs/advanced/extensions.md:48-50 reads only requestCtx.mcpReq.envelope?.[CLIENT_CAPABILITIES_META_KEY]?.extensions ?? {} and throws MissingRequiredClientCapabilityError when the id is absent. A legacy client never sends an envelope, so on every legacy tools/call declared is {} and the gate throws -32021.
Finding ID: f-26792aac
…leware (use), acceptResultType
55c03c8 to
ed2a77c
Compare
There was a problem hiding this comment.
2 other optional suggestions (nits or notes on pre-existing code) were found and not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (10):
- 🔴
packages/client/src/client/extension.ts:161—Extension authors can register a handler for a core method such as elicitation/create through ctx.method, replacing the… - 🔴
packages/core-internal/src/shared/protocol.ts:1901—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - Also unresolved: 7 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| * Registers a handler for a custom server-to-client request. Throws when | ||
| * `name` is a core method (wrap those with `use` instead) or when a | ||
| * handler already serves it, such as another extension's. | ||
| */ | ||
| method<P extends StandardSchemaV1, R extends StandardSchemaV1 | undefined = undefined>( |
There was a problem hiding this comment.
🟡 nit (optional): client extension authors are told ctx.method serves custom server-to-client requests, but on a 2026-07-28 connection such a handler never runs. Client._shouldDropInbound at client.ts:717-726 drops every inbound JSON-RPC request once a modern era is negotiated, so a handler registered through ctx.method only answers on legacy connections. The JSDoc at extension.ts:80-83 and docs/advanced/extensions.md:101 say nothing about this. Fix: state in both places that client custom methods are served on 2025-era connections only (the 2026-07-28 revision has no server-to-client request channel), or drop the client-side ctx.method from the documented surface. [also at: packages/client/src/client/extension.ts:83 - nit: extension authors who register a custom server-to-client request with ctx.method get a handler that never runs on a 2026-07-28 connection, and the server waits out its request timeout.]
Why this was flagged
A client built with an extension whose setup calls ctx.method('gate/ping', { params }, handler) connects with versionNegotiation: { mode: 'auto' } to a 2026-07-28 server. When the server sends { method: 'gate/ping' }, _onrequest at protocol.ts:971-974 consults _shouldDropInbound; the client override at client.ts:717-726 returns 'drop' for any inbound request on a modern era, so the message is reported to onerror and never reaches the registered handler, and the server's request waits for its timeout. The PR's own test for this path (packages/client/test/client/extensions.test.ts:189-198) runs only on a legacy connection. The JSDoc at packages/client/src/client/extension.ts:80-83 and docs/advanced/extensions.md:101 describe ctx.method as the hook for custom server-to-client requests without naming the era limit. The base branch has the same drop for setRequestHandler, but no extension API or guide inviting authors to rely on it.
Verification: ctx.method at extension.ts:159-169 just calls client.setRequestHandler(name, schemas, handler). Client._shouldDropInbound at client.ts:717-726 returns 'drop' for every isJSONRPCRequest(message) once isModernProtocolVersion(this._negotiatedProtocolVersion). The JSDoc at extension.ts:79-83 and docs/advanced/extensions.md:101 say nothing about the handler being reachable only on legacy connections.
Finding ID: f-09bf5991
This branch has not been deployed
1 participant
Servers and clients take MCP extensions as
{ id, setup(ctx) }objects:new McpServer(info, { extensions: [skills, tasks] }).setupruns at construction and adds everything through its context, so the SDK can check names, ids and dependencies for the extension. First of a stack: #2782 adds Tasks on top, and #2825 an example.Skills (#3047, merged) registers a resource template through
McpServer, and Tasks (#2782) needs middleware ontools/callplus its own result kind on the client. One context covers both, and it maps onto SEP-3371's registration, custom methods and middleware:ctx.methodrefuses a core method name, and a method another handler already serves, so two extensions can't silently replace each other.ctx.requiretakes a core capability ('tools','resources', …) or another extension id.connect()checks it, so a capabilityMcpServerturns on later, or an extension listed further down, still counts. BehindcreateMcpHandler, a missing dependency answers-32603and reports the cause toonerror.setupruns. Extension middleware runs outside middleware the application adds afterwards withserver.use.McpServeralso accepts{ id, attach(mcp) }, so the Skills extension goes inextensionsas it ships.extensionsis anMcpServeroption; the low-levelServerno longer takes it.ctx.client,ctx.methodfor server-to-client requests andctx.acceptResultType, which letstools/callcarryresultType: "task".Protocol.useandProtocol.acceptResultTypestay public for applications.McpServerre-throwsMissingRequiredClientCapabilityError(-32021) from a tool as a JSON-RPC error.@experimental. Docs are indocs/advanced/extensions.md.pnpm -r typecheck,pnpm -r lint,pnpm docs:checkand the core-internal, client and server suites ran, including the Skills extension served throughextensionsunchanged and arequirecheck throughcreateMcpHandler. #2782 adds an end-to-end test with Skills and Tasks on one server.