Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 6d9e4be 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d9e4be81f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!supported.includes(options.protocolVersion)) { | ||
| throw new UnsupportedProtocolVersionError({ supported, requested: options.protocolVersion }); | ||
| } | ||
| this._negotiatedProtocolVersion = options.protocolVersion; |
There was a problem hiding this comment.
When options is used with a custom transport that implements setProtocolVersion (for example, one that uses the selected revision to configure framing or headers), this branch updates only the Server and never calls that transport hook. The normal server initialize path calls this.transport?.setProtocolVersion(...), and modern client negotiation likewise propagates its choice, so an explicitly connected server and its transport can retain different version state for the whole connection; propagate options.protocolVersion to the transport as part of this path.
Useful? React with 👍 / 👎.
| throw new UnsupportedProtocolVersionError({ supported, requested: options.protocolVersion }); | ||
| } | ||
| this._negotiatedProtocolVersion = options.protocolVersion; | ||
| this._explicitProtocolVersion = true; |
There was a problem hiding this comment.
When a Server that completed a legacy initialize is closed and then reconnected through this explicit-modern path, _clientCapabilities and _clientVersion still belong to the old client because this path changes only the protocol version and modern connections never run _oninitialize to overwrite them. getClientCapabilities() and getClientVersion() explicitly remain functional for 2026 requests via envelope backfilling, but the manual connection path never performs the equivalent of seedClientIdentityFromEnvelope, so modern handlers can observe the previous client's identity (or undefined on a fresh instance); clear connection-scoped identity and backfill it from the validated envelope.
Useful? React with 👍 / 👎.
claude
Bot
added
the
v2
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether forwarded params could leak an upstream subscription ID or URI through to the listen stream: stampSubscriptionId in packages/server/src/server/listenRouter.ts spreads the forwarded _meta first and then overwrites io.modelcontextprotocol/subscriptionId, and serverEventToNotification overrides uri with event.uri after the spread, so the gateway's own values win in both cases.
Extended reasoning...
The change adds forwarded params to ServerEvent and a ServerConnectOptions argument on Server.connect/McpServer.connect in the server package, plus docs, a changeset and an integration test; it touches no auth or crypto surface. Several verified findings are posted inline (and some go unposted), so this is not an approval; the body only records a concrete concern checked and ruled out from reading the listen router's stamping order.
One optional note from this repository's REVIEW.md or CLAUDE.md checks was verified and left out, over this review's limit for such notes.
| if (options !== undefined) { | ||
| const supported = modernProtocolVersions(this._supportedProtocolVersions); | ||
| if (!supported.includes(options.protocolVersion)) { | ||
| throw new UnsupportedProtocolVersionError({ supported, requested: options.protocolVersion }); | ||
| } | ||
| this._negotiatedProtocolVersion = options.protocolVersion; | ||
| this._explicitProtocolVersion = true; | ||
| } |
There was a problem hiding this comment.
🟡 nit (optional): Gateway authors who bind a modern revision via the new connect option get undefined from getClientVersion() and getClientCapabilities() in handlers, unlike instances served by createMcpHandler. The explicit-binding branch at packages/server/src/server/server.ts:369-376 only writes the negotiated version; nothing seeds client identity from the request's _meta envelope, so the JSDoc promise at server.ts:992 that modern-era instances are "backfilled per request from the validated envelope" is false on this path. Fix: either backfill _clientCapabilities/_clientVersion from lifted.envelope whenever _servedModernEra() holds, or state in the ServerConnectOptions JSDoc and docs/advanced/low-level-server.md:123 that the deprecated accessors stay empty on an explicitly bound connection. [also at: docs/advanced/low-level-server.md:123 - nit: Handlers on a gateway wired per this recipe see getClientVersion() and getClientCapabilities() return undefined, unlike the same handlers under createMcpHandler.]
Why this was flagged
A custom HTTP entry follows docs/advanced/low-level-server.md:123, constructs a Server with a modern supportedProtocolVersions entry, and calls connect(transport, { protocolVersion }) at packages/server/src/server/server.ts:369-376. The request's _meta envelope carries clientInfo and clientCapabilities, but on this path only _negotiatedProtocolVersion is written; createMcpHandler additionally calls seedClientIdentityFromEnvelope at packages/server/src/server/createMcpHandler.ts:873 to fill _clientCapabilities and _clientVersion. A handler on the hand-wired instance that calls server.getClientVersion() or getClientCapabilities() therefore receives undefined, while the same handler under createMcpHandler receives the envelope values. The JSDoc at server.ts:992 and server.ts:1005 says instances serving the 2026-07-28 era are backfilled per request from the validated envelope, which no longer holds for every modern-serving instance. On the base branch this path does not exist (the classified request was rejected with -32022), so the gap is introduced with the new option.
Verification: The new branch at packages/server/src/server/server.ts:369-376 writes only this._negotiatedProtocolVersion = options.protocolVersion; this._explicitProtocolVersion = true;; nothing touches _clientCapabilities / _clientVersion (lines 270-271). getClientVersion()/getClientCapabilities() (server.ts:994-1008) return undefined on this path, while the JSDoc at server.ts:991-992 and 1004-1005 is false for it.
Finding ID: f-6d0038f6
| protected override _onclose(): void { | ||
| if (this._explicitProtocolVersion) { | ||
| this._explicitProtocolVersion = false; | ||
| this._negotiatedProtocolVersion = undefined; | ||
| } | ||
| super._onclose(); |
There was a problem hiding this comment.
🟡 (optional) Gateway authors who bind a modern revision via the new connect option see getNegotiatedProtocolVersion() return undefined inside their onclose callback and in handlers reacting to the close-time abort, unlike instances served by createMcpHandler, where the version survives close. The override at packages/server/src/server/server.ts:381-383 clears _negotiatedProtocolVersion before super._onclose() at server.ts:385 runs this.onclose?.() and aborts in-flight request handlers (protocol.ts:855-863). Fix: clear the explicit binding after super._onclose() returns, or keep the version readable until base teardown has fired callbacks, so close-time observers on both bound and entry-served instances see the same value.
Why this was flagged
Trigger: any close of a Server or McpServer connected with { protocolVersion } per docs/advanced/low-level-server.md:123, which is every per-request exchange on a custom gateway entry. Server._onclose at packages/server/src/server/server.ts:381-383 sets _negotiatedProtocolVersion = undefined first, then calls super._onclose() at server.ts:385. Protocol._onclose (packages/core-internal/src/shared/protocol.ts:855-863) only then invokes the user's onclose and aborts every in-flight request handler via controller.abort(error). A handler's abort listener or the onclose callback that reads getNegotiatedProtocolVersion() (server.ts:1022) now observes an un-negotiated instance. On the base, createMcpHandler's setNegotiatedProtocolVersion (createMcpHandler.ts:869) is never reset on close, so the same callbacks see the modern revision. Remedy: reset the binding after super._onclose() returns.
Verification: The override at packages/server/src/server/server.ts:380-386 runs this._negotiatedProtocolVersion = undefined (line 383) and only then super._onclose() (line 385). Protocol._onclose (packages/core-internal/src/shared/protocol.ts:836-865) invokes this.onclose?.() at line 855 and aborts in-flight request handlers at lines 861-863, so getNegotiatedProtocolVersion() (server.ts:1022-1023) returns undefined there.
Finding ID: f-c8e7368d
|
|
||
| Every serving recipe — [stdio](../serving/stdio.md), [HTTP](../serving/http.md) — applies to this server unchanged. | ||
|
|
||
| For a custom HTTP entry built from `classifyInboundRequest` and `PerRequestHTTPServerTransport`, bind the selected modern revision with `await server.connect(transport, { protocolVersion: route.classification.revision })` before delivering the classified message. The revision must be a modern entry in the server's `supportedProtocolVersions`; an unsupported revision throws before the transport starts. Omitting the option keeps the legacy initialization flow. Use a fresh instance for each per-request exchange, as the built-in HTTP entry does. |
There was a problem hiding this comment.
🟡 nit (optional): Readers who paste the documented call get a TypeScript compile error, because the snippet passes an optional field into a required one. docs/advanced/low-level-server.md:123 shows server.connect(transport, { protocolVersion: route.classification.revision }), but revision is typed string | undefined while ServerConnectOptions.protocolVersion is string. Fix: make the doc match what compiles, either narrow revision first (throw when undefined, as the built-in entry does) in the snippet, or accept string | undefined in ServerConnectOptions and treat undefined as unsupported in Server.connect.
Why this was flagged
A maintainer or user follows docs/advanced/low-level-server.md:123 and writes await server.connect(transport, { protocolVersion: route.classification.revision }) in a strict TypeScript project (common/tsconfig/tsconfig.json:19 sets strict). MessageClassification.revision is declared optional at packages/core-internal/src/types/types.ts:897, so its type is string | undefined. ServerConnectOptions.protocolVersion at packages/server/src/server/server.ts:80 is string, so the assignment fails type-checking. The new integration test avoids this by passing the constant revision instead of route.classification.revision, so nothing in the repo exercises the documented form. The base branch has no such doc paragraph. CLAUDE.md principle 6 asks that docs say what the code does.
Verification: A reader pastes the documented snippet into a strict TypeScript project. docs/advanced/low-level-server.md:123 tells readers to call server.connect(transport, { protocolVersion: route.classification.revision }). revision is string | undefined (packages/core-internal/src/types/types.ts:897), while ServerConnectOptions.protocolVersion: string at packages/server/src/server/server.ts:80 is required, so the call does not compile as written.
Finding ID: f-43a325c8
| @@ -12,11 +12,15 @@ import type { ServerCapabilities, SubscriptionFilter } from '@modelcontextprotoc | |||
| * The bus carries the EVENT, not the wire shape — the entry's listen router | |||
| * owns subscription-id stamping and per-stream filtering. | |||
There was a problem hiding this comment.
🟡 nit (optional): maintainers reading the ServerEvent doc comment are told the bus carries "the EVENT, not the wire shape", but the type now carries wire params. The new params?: JSONRPCNotification['params'] member at packages/server/src/server/serverEventBus.ts:22 is the notification's wire params, spread onto the outgoing notification at serverEventBus.ts:186-195. Fix: reword lines 12-13 so the comment matches the code, e.g. say the bus carries the event plus optional forwarded params while the router still owns subscription-id stamping and filtering.
Why this was flagged
The diff adds params?: JSONRPCNotification['params'] to ServerEvent (packages/server/src/server/serverEventBus.ts:20-23) and serverEventToNotification spreads it onto the wire {method, params} (serverEventBus.ts:186-195). The unchanged comment directly above at serverEventBus.ts:12-13 still says "The bus carries the EVENT, not the wire shape". On the base branch that sentence was accurate; after the change a maintainer reading the comment is told the opposite of what the type does. CLAUDE.md principle 6 requires comments to say what the code does; nothing fails at runtime.
Verification: nit. Triggering condition: any maintainer reading the ServerEvent doc comment after this diff. The unchanged JSDoc at packages/server/src/server/serverEventBus.ts:12-13 still reads "The bus carries the EVENT, not the wire shape". The diff adds to every ServerEvent variant params?: JSONRPCNotification['params'] (lines 20-23). On the base the sentence was accurate. No runtime effect.
Finding ID: f-58eb0dfd
| export type ServerConnectOptions = { | ||
| /** A supported modern revision selected by the serving entry; omit for a legacy initialize handshake. */ | ||
| protocolVersion: string; | ||
| }; | ||
|
|
||
| export type ServerOptions = ProtocolOptions & { | ||
| /** | ||
| * Capabilities to advertise as being supported by this server. |
There was a problem hiding this comment.
🟡 nit (optional): CLAUDE.md Minimalism prefers changes that add no API or option unless justified: this adds a public ServerConnectOptions type and a new options parameter on both Server.connect and McpServer.connect to hand the server a revision that the PerRequestHTTPServerTransport it is being connected to already carries in classification.revision (the test passes the same revision to both). Fix: before adding a public option, consider whether Server.connect can bind the era from the transport's existing classification (or an internal seam like setNegotiatedProtocolVersion), and keep the option only if the maintainers agree the duplication is justified.
Why this was flagged
Nothing fails at runtime. The instruction guards the SDK's public surface: every exported option is API the maintainers must support and keep compatible. Here the revision is supplied twice for one exchange (once to the transport's classification, once to connect), so the two can disagree and a custom entry has one more thing to get right. The PR description justifies the option by the gateway use case in #3055; the maintainers may accept that justification, in which case this is a non-issue.
Verification: Base CLAUDE.md line 5 reads exactly: "1. Minimalism — The SDK should do less, not more: prefer changes that add no API or option unless justified." The diff adds a new public API surface: export type ServerConnectOptions = { protocolVersion: string } (packages/server/src/server/server.ts:78-81), re-exported from packages/server/src/index.ts:61, plus a new options?: ServerConnectOptions parameter on Server.connect (server.ts:362) and McpServer.connect (mcp.ts:184).
Finding ID: f-19469de8
| /** Options for connecting a server to a transport that serves an already-selected modern revision. */ | ||
| export type ServerConnectOptions = { | ||
| /** A supported modern revision selected by the serving entry; omit for a legacy initialize handshake. */ | ||
| protocolVersion: string; |
There was a problem hiding this comment.
🟡 nit (optional): CLAUDE.md says comments must say what the code does: the JSDoc on ServerConnectOptions.protocolVersion reads "omit for a legacy initialize handshake", but the property is required (protocolVersion: string), so it cannot be omitted — connect(transport, {}) is a type error. What may be omitted is the whole options argument. Fix: move that sentence to the type-level or connect JSDoc ("omit the options to keep the legacy initialize handshake") or make the property optional if that is the intended shape.
Why this was flagged
Nothing fails at runtime. A reader of the field's hover text may try { } or { protocolVersion: undefined } to get the legacy flow and be stopped by the compiler; the docs in low-level-server.md state it correctly ("Omitting the option keeps the legacy initialization flow"). Small wording inaccuracy on new public API.
Verification: CLAUDE.md (base 46d9c99) item 6 reads verbatim "The words are true — Changesets, docs and comments say what the code does." The diff adds in packages/server/src/server/server.ts:78-81 a ServerConnectOptions type whose required property protocolVersion: string carries the JSDoc "A supported modern revision selected by the serving entry; omit for a legacy initialize handshake."
Finding ID: f-fe317a3b
This branch has not been deployed
2 participants
Fixes #3055.
A gateway cannot forward a change notification faithfully through
ServerEventBus: the event carries only a kind/URI, so the listen router drops custom_metaand extension params. A manually connectedServeralso rejects a correctly classified modern request because advertising supported versions does not bind the instance's serving revision.Keep optional notification params on bus events. The listen router retains those fields while stamping its own subscription ID, and resource events use their authoritative URI for filtering and delivery. Add an optional
ServerConnectOptionsargument toServer.connectandMcpServer.connectso a custom serving entry can bind a supported modern revision before transport startup. Clear that explicit binding on close; existing legacy connections keep their initialization flow.Ten public API regressions cover all four change notification kinds over real HTTP, both server classes, unsupported revisions, and a later legacy connection. All ten fail on the unmodified source and pass with this change. Includes documentation and a server patch changeset.
Validation on macOS with Node 24:
pnpm lint:allpasses from a fresh checkout.pnpm build:allandpnpm typecheck:allpass.node scripts/smoke-dist-types.mjspasses for ESM and CommonJS withoutskipLibCheck.pnpm test:all: all 271 test files report passed, with 7,207 tests passed and 141 existing expected failures. It exits nonzero on an unhandled 200 msREQUEST_TIMEOUTrejection fromscenarios/protocol.test.ts.The protocol timeout rejection also occurs without this change. An earlier full run reported two unhandled rejections; replaying only
scenarios/protocol.test.tsagainst unmodified upstream commit9bc40e903fde15534ec06ff4c1347d47000dbe16reproduced both (358 passed, 50 expected failures, two unhandled errors). The latest full run reports one of those rejections. No timeout, assertion, expected failure, or lint rule was changed.