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 10 commits into
Conversation
🦋 Changeset detectedLatest commit: 9f97b9d 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.
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.
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 (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.
ed2a77c to
3cfe7ec
Compare
There was a problem hiding this comment.
4 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 (11):
- 🔴
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: 8 minor or pre-existing, 2 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.
3cfe7ec to
39b0911
Compare
There was a problem hiding this comment.
1 other optional suggestion (a nit or a note on pre-existing code) was found and not posted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (12):
- 🔴
packages/core-internal/src/shared/protocol.ts:1905—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - Also unresolved: 9 minor or pre-existing, 2 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.
…leware (use), acceptResultType
39b0911 to
92954d8
Compare
On a 2026-07-28 connection the client fulfils elicitation/create, sampling/createMessage and roots/list embedded in input_required results through _getRequestHandler, which read the stored handler directly. Middleware an extension installed with ctx.use on those methods never ran there, though it is the only path for them on that era. Compose the same middleware stack around that handler.
…ion path ctx.setRequestHandler only checked the 2026-07-28 method registry, which holds no client-served methods and drops several 2025-11-25 ones. A client extension could register elicitation/create, sampling/createMessage or roots/list, and a server extension logging/setLevel or resources/subscribe, though the JSDoc and docs say a core name throws. Check both eras, keeping the tasks/get and tasks/cancel names SEP-2663 reuses for the Tasks extension.
- use() middleware receives the request as it arrived, params not yet validated, not a parsed request. - Typed calls such as callTool are also routed past the codec by acceptResultType, and reject the kind as an invalid result. - Application middleware goes on mcp.server.use; McpServer has no use(). - Client custom request handlers are served on 2025-11-25 connections only. - The gate example refuses every 2025-11-25 request as written. - Move the acceptResultType comment off the codec decode comment, and the ServerExtension export off the requestStateCodec comment.
…path only The check sat in the request funnel every typed spec call shares, so once a client extension accepted resultType "task" on tools/call, a plain client.callTool also skipped the codec. McpServer adds content: [] to a task handle, so the handle then parsed as an empty CallToolResult and the task was lost to the caller. Gate the bypass on the explicit-schema overload (and its retry legs), as the JSDoc already said; typed calls answer UnsupportedResultType again. Restores that JSDoc and corrects the guide.
There was a problem hiding this comment.
5 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 (6):
- 🔴
packages/core-internal/src/shared/protocol.ts:1917—Extension middleware that reads ctx.mcpReq.requestState() gets the raw, unverified wire string, not the value the reque… - Also unresolved: 4 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.
The export sat under the requestStateCodec comment, then under the OAuth metadata comment once sorted; it now has its own.
…through middleware
- setup may return a promise. The setups after it wait for it, so they still
run in array order, and connect (Protocol, and Server before its
one-connection check) waits for all of them and rejects with the error if
one fails. Nothing is processed with an extension partly set up, and a
failure that nobody connects for is not an unhandled rejection.
- Extension middleware (ctx.use) is kept in its own tier, composed outside
middleware the application adds with use(), so an async setup that
registers after the application cannot end up inside it.
- An extension's capabilities.extensions entry belongs to it: an entry for an
installed id in the capabilities option fails construction, and
registerCapabilities refuses it later. It was silently replaced by {}.
- Middleware on a method with no handler: when the MethodNotFound that next
throws is passed through, the per-request HTTP transport answers 404, as
for a method with no handler and as Hono answers notFound through route
middleware. A middleware answer keeps its own status.
- Documented that middleware sees the request before params validation and
before the requestState verify hook, and that an answer without next skips
result validation. Both installers now share installExtensions.
…brief comments - With acceptResultType in force, a typed call such as callTool on a 2025-11-25 connection had the codec strip resultType and returned a task handle as an empty tool result. Typed calls now reject an accepted kind with UnsupportedResultType on both eras. - ctx.mcpReq.send with an explicit schema now accepts extension result kinds like request() with one. - The middleware docs say the request arrives with wire-only material lifted onto ctx.mcpReq, not as it was received. - Cut the new comments to one line where they are not public API.
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (2):
- 🔴
packages/server/src/server/server.ts:362—Servers hosted with createMcpHandler skip a tool's scopeChallenge and Mcp-Param-* check when an async extension setup r… - Also unresolved: 1 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.
…'s instance createMcpHandler's modern route and serveStdio read the instance the factory returned before connecting it: the tool input schema for the Mcp-Param-* check, the tool's scope challenge, and the capabilities that narrow subscriptions/listen filters. Only connect waited for async setups, so a tool an async setup registered skipped the scope preflight and was called with a token lacking the scope. Both now wait right after the factory returns; a failed setup fails the request or connection.
… with accepted kinds - A ctx.require or other throw from an async setup, or from a setup after one, rejects connect rather than failing the constructor. - Typed calls reject an acceptResultType kind with UnsupportedResultType on either protocol version; the JSDoc, an inline comment and the changeset still said they ignore the declaration.
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs. Because this adds new public API across three packages and changes request dispatch, a human look at the design is still worthwhile.
What was reviewed
- The two newest commits:
createMcpHandlerandserveStdionow awaitextensionsSettledright after the factory returns, before the capability,Mcp-Param-*and scope-challenge reads. - The docs, changeset and JSDoc wording on async
setupfailures rejectingconnect, and on typed calls rejecting an accepted result kind withUnsupportedResultType, now match the code inshared/extensions.tsandprotocol.ts. - Middleware composition in
Protocol(extension tier outermost, the no-handlerMethodNotFoundmapped to 404 inperRequestTransport.ts) and capability ownership inregisterCapabilitieson both sides.
Extended reasoning...
The change is 21 files, +1655/-17, adding experimental extensions, Protocol.use middleware and acceptResultType across core-internal, client and server, with docs, a changeset and unit tests. It touches security-relevant surface: middleware runs before params validation and the requestState.verify hook, and the HTTP entry's scope-challenge preflight now depends on async extension setups having settled. This run found no bugs, and the commits pushed after the last review cover the hosting-entry readiness wait and the wording items. It is deferred, not approved, because of the size, the new public API surface, and the McpServer tool-dispatch behaviour change (-32021 re-thrown as a JSON-RPC error) shipped as a minor, which are design decisions for a maintainer.
Servers and clients take MCP extensions as
{ id, setup(ctx) }objects:new McpServer(info, { extensions: [skills, tasks] }). What is required and declarative sits on the object, which today is just theid. Everything else happens insetupthroughctx, so the SDK can check ids and method names for each extension. First of a stack: #3066 ports Skills, and #2782 adds Tasks with 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, dependencies, custom methods and middleware:ctx.setRequestHandlertakes the same arguments assetRequestHandler. It refuses a core method name, and a method another handler already serves, so two extensions can't silently replace each other.ctx.require(extensionId)names another extension. The full list is known before anysetupruns, so order doesn't matter, and a missing one fails construction, naming both. Optional behaviour stays in the extension's own code.setupruns. Extension middleware runs outside middleware the application adds afterwards withserver.use.extensionstakes only{ id, setup(ctx) }; there are no adapters. It is anMcpServeroption, and the low-levelServeris unchanged.ctx.client,ctx.setRequestHandlerfor server-to-client requests andctx.acceptResultType, which letstools/callcarryresultType: "task".shared/extensions.ts), so server and client refuse the same things with the same messages.Protocol.useandProtocol.acceptResultTypestay public for applications.McpServerre-throwsMissingRequiredClientCapabilityError(-32021) from a tool as a JSON-RPC error.extensionsoptions, the contexts,use,acceptResultTypeandRequestMiddlewareare@experimental. Docs are indocs/advanced/extensions.md.pnpm -r typecheck,pnpm -r lint,pnpm docs:checkand the core-internal, client and server suites ran. #2782 adds an end-to-end test with Skills and Tasks on one server.