feat: add MCP protocol adapters - #6625
Conversation
🦋 Changeset detectedLatest commit: 0182ea3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMCP server configuration now requires protocol adapters. A registry selects and namespaces protocols, session state pins negotiated adapters, and inbound/outbound traffic uses protocol-specific schemas. Built-in support starts at ChangesMCP protocol support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant McpServer
participant protocolRegistry
participant RpcServer
Client->>McpServer: Send initialize or MCP request
McpServer->>protocolRegistry: Select protocol and route request
protocolRegistry-->>McpServer: Return version-specific request
McpServer->>RpcServer: Decode and dispatch selected schema
RpcServer-->>McpServer: Return response
McpServer-->>Client: Encode and send protocol-specific response
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
a19c276 to
b9e862f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
packages/effect/src/unstable/ai/McpServer.ts (4)
804-807: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the predicate parameter to avoid shadowing
protocol.The outer
protocol(Line 801) is the value returned at Line 813; shadowing it with the adapter inside thesomecallback is easy to misread.(candidate) => candidate.protocolVersion === protocolVersionreads clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/ai/McpServer.ts` around lines 804 - 807, Rename the some callback parameter in the protocol version validation predicate to candidate, and update its property access accordingly; preserve the comparison against protocolVersion and avoid shadowing the outer protocol value.
1612-1614: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDie with an
Error, not a bare string.
Effect.die("...")loses the stack trace for what is an internal invariant violation surfaced at layer-build time.Effect.die(new Error(...))keeps it diagnosable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/ai/McpServer.ts` around lines 1612 - 1614, Update the invariant failure in the handler registration flow around namespacedRpc, handler, and routed.tag to pass an Error instance to Effect.die instead of a bare string, preserving the existing message while retaining diagnostic stack information.
557-557: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently swallowing notification failures hurts diagnosability.
Effect.catchCause(() => Effect.void)discards both payload decode errors and handler defects for every inbound notification, so a malformednotifications/*frame or a broken handler produces no signal at all. Log at debug/warn before discarding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/ai/McpServer.ts` at line 557, Update the inbound notification handling around Effect.catchCause so failures are logged at an appropriate debug or warning level before being converted to Effect.void. Preserve the existing discard behavior after logging, and include the caught cause so malformed notifications and handler defects remain diagnosable.
591-594: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent fallback for a missing
clientProtocolsentry.Line 574 falls back to
registry.protocols[0]viagetProtocolForClient, while this loop silentlycontinues. Using the same helper here would keep notification delivery aligned with response routing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/ai/McpServer.ts` around lines 591 - 594, Update the notification-delivery loop around selectedProtocol to use getProtocolForClient with the current clientId instead of directly reading clientProtocols and continuing when absent. Preserve the existing notification routing while applying the same registry.protocols[0] fallback used by response routing.packages/effect/test/unstable/ai/McpServer.test.ts (1)
503-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the negative assertion in the pinned-adapter test.
assert.property(invalidBody, "error")passes for any JSON-RPC error, including a 404/session or internal failure, so it doesn't actually prove session A decoded with adapter A's schema. Assert on the error code (or that the message mentions the expected field) to make the pinning guarantee explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/test/unstable/ai/McpServer.test.ts` around lines 503 - 508, Strengthen the negative assertion in the pinned-adapter test by validating the specific schema-decoding error for session A, using its expected JSON-RPC error code or a message containing the expected field, instead of only checking that invalidBody has an error property. Keep the validABody and validBBody assertions unchanged.packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts (1)
10-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe namespace prefix is private to the registry, forcing recomputation and reverse-engineering.
prefix()is an unexported helper, so it is re-derived on everyrouteClientRequestcall and callers must fabricate a dummy request to read it back. Exposing it once onProtocolRegistryresolves both.
packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts#L10-L11: precompute aprotocol → prefixmap alongsidebyVersionand surface it (e.g.prefixFor(protocol)) on the returned registry.packages/effect/src/unstable/ai/McpServer.ts#L1685-L1702: replace the syntheticRequestEncodedinprotocolForInternalTagwithtag.startsWith(registry.prefixFor(protocol)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts` around lines 10 - 11, The ProtocolRegistry currently keeps the namespace prefix private and recomputes it indirectly. In packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts lines 10-11, precompute a protocol-to-prefix map alongside byVersion and expose a prefixFor(protocol) method on the returned registry. In packages/effect/src/unstable/ai/McpServer.ts lines 1685-1702, update protocolForInternalTag to use tag.startsWith(registry.prefixFor(protocol)) and remove the synthetic RequestEncoded request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/effect/MCP.md`:
- Around line 82-84: Update packages/effect/MCP.md lines 82-84 to state that
this release supports McpProtocol.v2025_06_18, without implying a starting point
or compatibility floor. Update .changeset/quiet-mice-negotiate.md line 5 to
replace “support floor” with “built-in support for MCP 2025-06-18”.
- Line 85: Update the prose near McpServer.layerHttp to hyphenate “HTTP-based”
wherever it modifies “implementation.”
In `@packages/effect/src/unstable/ai/McpServer.ts`:
- Around line 1455-1457: The protocol loop currently constructs handlers from
the fixed ClientRpcs schema instead of each adapter’s
selectedProtocol.clientRpcs, causing version-specific RPC additions or removals
to mismatch handlerContext. Update the adapter/handler construction around
selectedProtocol and addProtocolHandlers so handlers are derived from or
supplied by each adapter, preserving the correct RPC keys for every registered
protocol, including the 2025-11-25 adapter.
- Line 408: The notification reconciliation loop must evict disconnected clients
from both per-client maps. In packages/effect/src/unstable/ai/McpServer.ts lines
408-408, remove clientProtocols entries whose IDs are absent from
patchedProtocol.clientIds; in lines 345-357, remove the corresponding
clientSessions entry using the HTTP UUID key or non-HTTP clientId key. Ensure
both cleanups occur alongside the existing server.initializedClients
reconciliation.
- Around line 1665-1675: Prefix session-map keys by transport identity: update
both clientSessions.set call sites to store HTTP sessions as session:${uuid} and
non-HTTP sessions as client:${clientId}, then update getClientSession to apply
the matching prefixes for mcpSessionIdHeader lookups and clientId fallbacks.
- Around line 507-532: Reuse the optional HttpServerRequest value already
obtained earlier in the surrounding protocol flow instead of calling
Context.getUnsafe in the headers and pre-response-handler logic. Guard
HTTP-specific behavior on that existing request value, and reuse the existing
fiber/request references rather than re-reading Fiber.getCurrent(), while
preserving non-HTTP header handling.
- Around line 595-606: Update the outbound notification encoding in the server
notification handling flow around
selectedProtocol.serverNotificationRpcs.requests and rpc.payloadSchema to use
Schema.toCodecJson before encoding, matching the inbound JSON transport path.
Preserve the existing encoded payload assignment and Request message
construction.
- Around line 535-538: Update the notification handling around
Schema.decodeUnknownEffect to resolve the RPC payload schema from
clientNotificationRpcs when the notification tag is not present in clientRpcs,
ensuring notifications/cancelled still follows the special Interrupt path
instead of the generic request handler. Preserve the existing clientRpcs lookup
and decoding behavior for RPCs indexed there.
---
Nitpick comments:
In `@packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts`:
- Around line 10-11: The ProtocolRegistry currently keeps the namespace prefix
private and recomputes it indirectly. In
packages/effect/src/unstable/ai/internal/mcpProtocolRegistry.ts lines 10-11,
precompute a protocol-to-prefix map alongside byVersion and expose a
prefixFor(protocol) method on the returned registry. In
packages/effect/src/unstable/ai/McpServer.ts lines 1685-1702, update
protocolForInternalTag to use tag.startsWith(registry.prefixFor(protocol)) and
remove the synthetic RequestEncoded request.
In `@packages/effect/src/unstable/ai/McpServer.ts`:
- Around line 804-807: Rename the some callback parameter in the protocol
version validation predicate to candidate, and update its property access
accordingly; preserve the comparison against protocolVersion and avoid shadowing
the outer protocol value.
- Around line 1612-1614: Update the invariant failure in the handler
registration flow around namespacedRpc, handler, and routed.tag to pass an Error
instance to Effect.die instead of a bare string, preserving the existing message
while retaining diagnostic stack information.
- Line 557: Update the inbound notification handling around Effect.catchCause so
failures are logged at an appropriate debug or warning level before being
converted to Effect.void. Preserve the existing discard behavior after logging,
and include the caught cause so malformed notifications and handler defects
remain diagnosable.
- Around line 591-594: Update the notification-delivery loop around
selectedProtocol to use getProtocolForClient with the current clientId instead
of directly reading clientProtocols and continuing when absent. Preserve the
existing notification routing while applying the same registry.protocols[0]
fallback used by response routing.
In `@packages/effect/test/unstable/ai/McpServer.test.ts`:
- Around line 503-508: Strengthen the negative assertion in the pinned-adapter
test by validating the specific schema-decoding error for session A, using its
expected JSON-RPC error code or a message containing the expected field, instead
of only checking that invalidBody has an error property. Keep the validABody and
validBBody assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a276e0a2-f82b-4a28-9666-0e344c20ed4f
📒 Files selected for processing (18)
.changeset/quiet-mice-negotiate.mdpackages/effect/MCP.mdpackages/effect/src/unstable/ai/McpProtocol.tspackages/effect/src/unstable/ai/McpSchema.tspackages/effect/src/unstable/ai/McpServer.tspackages/effect/src/unstable/ai/index.tspackages/effect/src/unstable/ai/internal/mcpProtocol.tspackages/effect/src/unstable/ai/internal/mcpProtocolRegistry.tspackages/effect/test/unstable/ai/McpProtocol.test.tspackages/effect/test/unstable/ai/McpServer.test.tspackages/effect/test/unstable/ai/McpServer/BaseProtocol.test.tspackages/effect/test/unstable/ai/McpServer/Prompts.test.tspackages/effect/test/unstable/ai/McpServer/Resources.test.tspackages/effect/test/unstable/ai/McpServer/Tools.test.tspackages/effect/test/unstable/ai/McpServer/Transports.test.tspackages/effect/test/unstable/ai/McpServer/Utilities.test.tspackages/effect/test/unstable/ai/McpServer/utils.tspackages/effect/typetest/unstable/ai/McpServer.tst.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
c09763f to
499bbb1
Compare
499bbb1 to
ef76f26
Compare
| export const v2025_06_18 = Internal.make({ | ||
| protocolVersion: "2025-06-18", | ||
| clientRpcs: McpSchema.ClientRpcs, | ||
| clientNotificationRpcs: McpSchema.ClientNotificationRpcs, | ||
| serverRequestRpcs: McpSchema.ServerRequestRpcs, | ||
| serverNotificationRpcs: McpSchema.ServerNotificationRpcs | ||
| }) |
There was a problem hiding this comment.
I like the idea of having an abstracted McpProtocol - but we will need to come up with a solution for McpSchema as we introduce support for other protocol versions.
Also please make sure all public exports have their type annotated - this does not.
There was a problem hiding this comment.
gave it a more explicit type then just typeof:
export type ProtocolAdapter = Internal.ProtocolAdapter<
"2025-06-18",
RpcGroup.Rpcs<typeof McpSchema.ClientRpcs>,
RpcGroup.Rpcs<typeof McpSchema.ClientNotificationRpcs>,
RpcGroup.Rpcs<typeof McpSchema.ServerRequestRpcs>,
RpcGroup.Rpcs<typeof McpSchema.ServerNotificationRpcs>
>I was waiting to adding v2025-11-28 before having a hard think about the best type for this, but if you have ideas now i'm all ears
There was a problem hiding this comment.
I'm honestly not sure what the best strategy is going to be. The schemas themselves will likely be quite similar between versions, with some minor changes, but I don't know for sure.
Maybe we implement sets of Rpcs for each protocol in McpSchema? Not sure how laborious that will be, but if we share individual Rpcs that don't change between versions, it might not be too bad?
There was a problem hiding this comment.
ideally there is some "core" McpSchema that just needs to be extended with version specific shapes. I already codified it in the McpServer that if an rpc is missing from something like the .serverNotificationRpcs then the broadcast is skipped since this might be something that the client on that particular version is missing:
for (const clientId of server.initializedClients.keys()) {
if (!clientIds.has(clientId)) {
server.initializedClients.delete(clientId)
continue
}
const selectedProtocol = clientProtocols.get(clientId)
if (!selectedProtocol) {
continue
}
const rpc = selectedProtocol.serverNotificationRpcs.requests.get(request.tag)
if (!rpc) {
continue
}
const encoded = yield* Schema.encodeUnknownEffect(
Schema.toCodecJson(rpc.payloadSchema)
)(request.payload)
...
}This kind of assumes that the Rpc shape could end up being quite different between say v2025-06-18 and v2026-07-28
There was a problem hiding this comment.
Yea - I think we will need a robust solution for this as we introduce further protocol versions.
There was a problem hiding this comment.
I'm going to track this in #6639 so its easier to collect information and ideas. If you want this to be a blocker, then I can build another PR that stacks into this one so its easier to review
There was a problem hiding this comment.
I don't think one has to block the other. If this PR is ready for a final review, let me know and I can do a deep dive.
There was a problem hiding this comment.
i'll go over it again tonight to be sure but i think its just about ready. see what i can bring from the McpCore exploration
There was a problem hiding this comment.
I'm happy with it. Feel free to deep dive into this when you have time. I'll try figure out how the other protocols will fit together on top of this
ef76f26 to
30af973
Compare
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "effect": minor | |||
There was a problem hiding this comment.
| "effect": minor | |
| "effect": patch |
This can be a patch since we're still in beta.
| export const run: (options: { | ||
| readonly name: string | ||
| readonly version: string | ||
| readonly protocols: Arr.NonEmptyReadonlyArray<McpProtocol.ProtocolAdapter> |
There was a problem hiding this comment.
Are we going to force the user to pass in versions, or should this be optional and default to the latest version we support currently?
There was a problem hiding this comment.
I think making this required makes sense cause there needs to be a decision on what can be supported, especially with the upcoming v2026-06-28 stateless shift. Since each item in the array multiplies the number of RpcGroups it would be good to not hide it behind defaults.
In the McpCore version i'm working on now I've broken v2024-11-05 and v2025-03-26 from the McpSchema to test how the fallbacks work and this seems like an important decisions for the McpServer on what happens when an unexpected version'd client sends a request:
const server = McpServer.layerHttp({
name: "Acme Mcp",
version: "1.0.0",
path: "/mcp",
protocols: [
McpProtocol.v2025_06_18, // <- fallback
McpProtocol.v2025_03_26,
McpProtocol.v2024_11_05
],
})If you think this is too breaking of a change then we could have a default array, but it would need to be communicated well that this increases the size of the server by just updating versions
There was a problem hiding this comment.
No I think this argument is well thought out - we can keep the protocols array required for now.
IMax153
left a comment
IMax153
left a comment
There was a problem hiding this comment.
I think this looks good to me in its current form - we can merge and test it out 👍
Bundle Size Analysis
|
2 participants
Type
Goal/Scope
Prepare
McpServerto support multiple incompatible MCP protocol versions through one endpoint without merging their schemas into a permissive shared protocol.Adds the version-isolation seam and the
2025-06-18adapter, but does not implement the2025-11-25or2026-07-28protocols.Description
Servers now declare an ordered, non-empty list of implemented adapters:
An adapter contains its version and the four RPC groups used for client requests, client notifications, server requests, and server notifications. This prevents a server from advertising support using only a version string.
ProtocolRegistrysnapshots the declaration, rejects duplicate versions, and selects an exact match or falls back to the first declared adapter. Allowing incoming methods to be routed to versioned internal keys before payload decoding:The internal key selects exactly one adapter's schema and handler. The existing
RpcServerstill owns decoding, middleware, handler execution, and response encoding.Successful initialization pins the selected adapter to the session. Later requests, reverse client requests, and outbound notifications all reuse that adapter.
How to Review
Start with
McpProtocol.tsandinternal/mcpProtocol.ts. Confirm an adapter represents executable support rather than only a version identifier.Review
internal/mcpProtocolRegistry.ts. Check the declaration snapshot, duplicate rejection, first-adapter fallback, and versioned RPC-group merge.Follow one request through
runWithRegistryinMcpServer.ts:The important invariant is that adapter selection happens before schema decoding and is not renegotiated after initialization.
Review
layerHandlersandaddProtocolHandlers. Shared MCP domain handlers are prepared withRpcGroup.toHandlersand registered under each adapter's versioned key.Review the protocol and server tests. They cover registry validation and fallback, incompatible schema isolation, session pinning, the HTTP protocol header, missing sessions, and preservation of the June stdio transcript.
Comments
Only
McpProtocol.v2025_06_18is public in this phase. Later versions can introduce distinct schemas and projections without changing the selection boundary.Server-originated MCP notifications expose an existing mismatch with
RpcServer.Protocol.send, which only models RPC responses. The narrow cast is retained with a TODO: changing the RPC transport contract is outside this PR.Related
Summary by CodeRabbit
2025-06-18.protocolVersion.notifications/cancellednow behaves as an interrupt.protocolsexplicitly and reflect the new support floor.