fix: mcp conformance and add test suite - #6693
Conversation
🦋 Changeset detectedLatest commit: 255cdb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 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:
📝 WalkthroughWalkthroughThis change revises MCP schemas, server routing, session and transport handling, and RPC cancellation. It adds reusable HTTP/stdio conformance infrastructure and broad protocol-version ChangesMCP protocol and server behavior
Conformance test infrastructure
Existing validation updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (1 passed)
Comment |
effect-slopcop
Bot
added
4.0
enhancement
284be9d to
2d57078
Compare
lloydrichards
changed the title
2d57078 to
acf2d74
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: 9
🧹 Nitpick comments (21)
packages/effect/test/unstable/ai/McpServer/TestUtils/McpHttpHarness.ts (2)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSession-id injection is unconditional, unlike protocol version.
Line 23 overwrites any caller-supplied
Mcp-Session-Id, so a test that deliberately sends a stale/omitted session id throughfetch(e.g. the 400-on-missing-session case inMcpServer.test.ts) silently gets the harness value once an initialize has been observed. Mirror thehas()guard used forMcp-Protocol-Versionso callers can opt out.♻️ Proposed change
- if (sessionId !== null) { + if (sessionId !== null && !request.headers.has("Mcp-Session-Id")) { request.headers.set("Mcp-Session-Id", sessionId) }🤖 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/TestUtils/McpHttpHarness.ts` around lines 20 - 33, Update the fetch wrapper’s session header injection to set Mcp-Session-Id only when sessionId is non-null and the request does not already contain that header, matching the existing Mcp-Protocol-Version guard and preserving caller-supplied values.
35-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
post/postTextbypassfetch, soresponsesand header propagation don't apply to them.Two different code paths with different side effects is easy to trip over (
responsesstays empty for harnesspostcallers). RoutingpostTextthroughfetchwould unify tracking; keep bypassing only if the conformance suite intentionally needs full manual header control.♻️ Optional unification
const postText = (body: string, headers?: HeadersInit) => Effect.promise(() => - handler( + fetch( new Request(MCP_ENDPOINT, {🤖 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/TestUtils/McpHttpHarness.ts` around lines 35 - 58, Update postText and post in the MCP HTTP harness to route requests through the existing fetch helper instead of invoking handler directly, so responses tracking and header propagation are consistent; preserve postText’s request body and headers behavior, and keep post delegating to postText.packages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.ts (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHardcoded protocol version in
EnabledWhendefeats the multi-revision goal of these fixtures.
makeFeaturesServerLayeris parameterized byprotocol, butStructuredTool's gate compares against the literal"2025-06-18". Running the shared suites against another adapter will silently disable this tool, so structured-output tests fail for reasons unrelated to the revision under test. Move the tool into a factory closed overprotocol.protocolVersion.♻️ Sketch
-const StructuredTool = Tool.make("StructuredTool", { - parameters: Tool.EmptyParams, - success: Schema.Struct({ - value: Schema.String - }) -}).annotate( - McpSchema.EnabledWhen, - (client) => client.protocolVersion === "2025-06-18" -) +const makeStructuredTool = (protocolVersion: string) => + Tool.make("StructuredTool", { + parameters: Tool.EmptyParams, + success: Schema.Struct({ + value: Schema.String + }) + }).annotate( + McpSchema.EnabledWhen, + (client) => client.protocolVersion === protocolVersion + )🤖 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/McpConformance/McpConformanceFixtures.ts` around lines 28 - 36, Refactor StructuredTool into a factory that accepts or closes over protocol.protocolVersion, and have its McpSchema.EnabledWhen predicate compare against that value instead of the hardcoded "2025-06-18". Update makeFeaturesServerLayer to create/use the protocol-specific tool so structured-output tests remain enabled for every revision under test.packages/effect/typetest/unstable/ai/McpServer.tst.ts (1)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact
keyof typeof McpProtocolassertion is brittle.Any new export from
McpProtocol(a future revision adapter, or a helper) breaks this test even when nothing regressed. If the intent is "v2025_06_18 exists", preferexpect<"v2025_06_18">().type.toBeAssignableTo<keyof typeof McpProtocol>(); keep the exact form only if pinning the exported surface is deliberate.🤖 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/typetest/unstable/ai/McpServer.tst.ts` around lines 73 - 75, Update the type assertion in the “should expose the supported protocol adapter” test to verify that "v2025_06_18" is assignable to keyof typeof McpProtocol, rather than asserting the entire exported key set exactly. Keep the ProtocolVersion assertion unchanged.packages/effect/test/unstable/ai/McpServer/TestUtils/McpStdioHarness.ts (1)
113-132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
JSON.parsein the framing fiber turns malformed stdout into a test timeout.Line 127 runs inside a
forkScopedloop, so a non-JSON line (server crash text, partial write, or a deliberately malformed-output test) kills the reader fiber; every latertakeMessage/sendRequestthen hangs until the test times out with no indication of the real cause. Fail loudly with the offending line instead.🛡️ Proposed fix
if (line.length > 0) { - yield* routeFrame(JSON.parse(line)) + yield* routeFrame( + yield* Effect.try({ + try: () => JSON.parse(line), + catch: (cause) => new Error(`McpStdioHarness: invalid JSON frame: ${line}`, { cause }) + }).pipe(Effect.orDie) + ) }🤖 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/TestUtils/McpStdioHarness.ts` around lines 113 - 132, Update the framing loop in the forked Effect around routeFrame and JSON.parse to catch parse failures, then fail loudly with an error that includes the offending line. Ensure malformed stdout terminates or propagates the reader failure instead of silently killing the fiber and leaving later takeMessage/sendRequest calls blocked.packages/effect/test/unstable/ai/McpServer/McpServer.test.ts (1)
423-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNegative isolation assertions rely on timing.
Lines 518-519 use
Queue.pollright after reading each update; if a cross-session notification were delivered a tick later, the poll would pass and the leak would go undetected. Consider asserting after a short yield/TestClockadvance, or draining both queues and asserting the full set of received URIs.🤖 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/McpServer.test.ts` around lines 423 - 519, The negative isolation checks after nextResourceUpdate are timing-sensitive because immediate Queue.poll calls may miss delayed cross-session notifications. Update the resource subscription test’s assertions to allow pending effects to run, such as yielding or advancing TestClock, then drain or inspect both client outbound queues and assert that each session received only its subscribed URI.packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts (2)
97-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnbounded drain loop depends on the vitest timeout to fail.
If neither the cancelled response nor the ping response ever arrives,
takeMessageblocks and the failure surfaces only as a suite timeout with no diagnostic. Wrapping the loop inEffect.timeoutwould give a clearer failure.🤖 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/McpConformance/UtilitiesTest.ts` around lines 97 - 104, Bound the message-draining loop around fixture.takeMessage with Effect.timeout so it fails explicitly when neither the cancelled response nor the ping response arrives. Preserve the existing assertions and break condition for valid responses, and configure the timeout using the test’s established timing conventions.
139-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree progress smoke tests differ only in
params.Consider a table-driven variant (single
it.effectper case generated from an array) to drop the repeated body and the thrice-copied NOTE comment.🤖 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/McpConformance/UtilitiesTest.ts` around lines 139 - 201, Consolidate the three Progress smoke tests into a table-driven set generated from an array of case names and params, using one shared it.effect body for initialization, notification sending, and response assertions. Preserve coverage for string tokens, numeric tokens, and the optional total, and retain the NOTE only once near the shared test definition.packages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.ts (2)
13-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
callToolandcallToolWireduplicate the same setup.
callTooliscallToolWireplus adecodeCallToolstep; the initialize/notify/send block is copy-pasted. Deriving one from the other keeps the arguments/id handling in one place.♻️ Proposed consolidation
-const callTool = (name: string, arguments_: Record<string, unknown> = {}) => - Effect.gen(function*() { - const test = yield* McpConformance - const initialized = yield* test.initialize({ server: "features" }) - yield* test.notifyInitialized(initialized) - const response = yield* test.send(initialized, { - jsonrpc: "2.0", - id: 2, - method: "tools/call", - params: { name, arguments: arguments_ } - }) - return yield* test.decodeResult(response).pipe( - Effect.flatMap((message) => decodeCallTool(message.result)) - ) - }) - -const callToolWire = (name: string) => +const callToolWire = (name: string, arguments_: Record<string, unknown> = {}) => Effect.gen(function*() { const test = yield* McpConformance const initialized = yield* test.initialize({ server: "features" }) yield* test.notifyInitialized(initialized) const response = yield* test.send(initialized, { jsonrpc: "2.0", id: 2, method: "tools/call", - params: { name, arguments: {} } + params: { name, arguments: arguments_ } }) return yield* test.decodeResult(response) }) + +const callTool = (name: string, arguments_: Record<string, unknown> = {}) => + callToolWire(name, arguments_).pipe( + Effect.flatMap((message) => decodeCallTool(message.result)) + )🤖 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/McpConformance/ToolsTest.ts` around lines 13 - 41, Consolidate the duplicated initialization, notification, and tool-request setup in callTool and callToolWire by deriving one helper from the other. Preserve callTool’s arguments_ support and decodeCallTool processing, while keeping callToolWire’s raw decoded response behavior and the existing request id and method values.
227-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resetObservationsrelies on tests within this layer not running concurrently.The invocation counter is shared layer state; if this file is ever run with concurrent tests, the reset plus
toolInvocations === 0assertion becomes order-dependent. Consider scoping the observation to the specific call (e.g. capturing invocations before/after and asserting no delta).🤖 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/McpConformance/ToolsTest.ts` around lines 227 - 244, Update the test around McpConformance.resetObservations and the toolInvocations assertion to avoid relying on shared counter state: capture the invocation count immediately before sending the invalid tools/call request, then assert the count is unchanged afterward. Preserve the existing validation-failure scenario and zero-handler-invocation expectation without depending on other tests’ resets.packages/effect/test/unstable/ai/McpServer/v2025_06_18.test.ts (1)
34-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this progress case into
UtilitiesTest.ts.The other three
notifications/progresssmoke tests live inMcpConformance/UtilitiesTest.tsunder the sameUtilities > Progresspath. Keeping this one in the entry-point file splits the group across files for no apparent revision-specific reason (themessagefield exists in earlier revisions too).🤖 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/v2025_06_18.test.ts` around lines 34 - 59, The `notifications/progress` smoke test currently in the entry-point test file should be moved into `McpConformance/UtilitiesTest.ts`, alongside the other tests under the existing `Utilities > Progress` suite. Preserve the test’s behavior and assertions, and do not retain a duplicate in the original file.packages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.ts (2)
165-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests assert the same thing.
"MUST return exactly one error response for a failed request" (Lines 165-184) and "MUST not include both result and error in a response" (Lines 212-229) both only check
errorpresent /resultabsent; neither verifies "exactly one". Consider making the first assert single-delivery (as the stdio variant on Lines 147-163 does) and letting the second keep the mutual-exclusion check, or drop one.Also applies to: 212-229
🤖 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/McpConformance/BaseProtocolTest.ts` around lines 165 - 184, Update the “MUST return exactly one error response for a failed request” test around the existing test.initialize, test.notifyInitialized, and test.send flow to verify single delivery, matching the stdio variant’s assertion. Keep the separate “MUST not include both result and error in a response” test focused only on mutual exclusion, rather than duplicating error-present/result-absent checks.
46-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the exported
McpSchemaerror-code constants over raw numbers.
ToolsTest.tsalready usesMcpSchema.INVALID_PARAMS_ERROR_CODE/INTERNAL_ERROR_CODE; here the same codes are hard-coded (-32600,-32601,-32602,-32700). Using the constants (McpSchema.INVALID_REQUEST_ERROR_CODE,McpSchema.PARSE_ERROR_CODE, etc.) keeps the suite consistent and self-documenting.Also applies to: 128-143, 245-272
🤖 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/McpConformance/BaseProtocolTest.ts` around lines 46 - 95, Replace the hard-coded JSON-RPC error-code values in the conformance tests, including the cases around the shown tests and the additional referenced ranges, with the corresponding exported constants from McpSchema: INVALID_REQUEST_ERROR_CODE, METHOD_NOT_FOUND_ERROR_CODE, INVALID_PARAMS_ERROR_CODE, PARSE_ERROR_CODE, and any other applicable error-code constants. Preserve each test’s existing assertions and behavior.packages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.ts (1)
13-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
getPromptduplicatesgetPromptWireverbatim.The two helpers differ only in the trailing decode step. Define one in terms of the other.
♻️ Proposed fix
-const getPrompt = (name: string) => - Effect.gen(function*() { - const test = yield* McpConformance - const initialized = yield* test.initialize({ server: "features" }) - yield* test.notifyInitialized(initialized) - const response = yield* test.send(initialized, { - jsonrpc: "2.0", - id: 2, - method: "prompts/get", - params: { name } - }) - return yield* test.decodeResult(response).pipe( - Effect.flatMap((message) => decodeGetPrompt(message.result)) - ) - }) - const getPromptWire = (name: string) => Effect.gen(function*() { const test = yield* McpConformance const initialized = yield* test.initialize({ server: "features" }) yield* test.notifyInitialized(initialized) const response = yield* test.send(initialized, { jsonrpc: "2.0", id: 2, method: "prompts/get", params: { name } }) return yield* test.decodeResult(response) }) + +const getPrompt = (name: string) => + getPromptWire(name).pipe(Effect.flatMap((message) => decodeGetPrompt(message.result)))🤖 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/McpConformance/PromptsTest.ts` around lines 13 - 41, Refactor the duplicate initialization and request flow in getPrompt and getPromptWire by defining getPrompt in terms of getPromptWire, retaining only the additional decodeGetPrompt step in getPrompt and preserving the existing result behavior.packages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.ts (1)
41-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
"2025-06-18"defeats the suite's protocol parameterization.
suitetakes aprotocol: McpProtocol.ProtocolAdapterprecisely so these tests can be instantiated per revision, butrunElicitationpins bothprotocolVersionandinitializePayload.protocolVersionto"2025-06-18". When this suite is reused for another revision the injectedMcpServerClientwill report the wrong negotiated version, and any version-sensitive behaviour inMcpServer.elicitwould be tested against the wrong contract.Since
runElicitationis module-scoped it has no access toprotocol; thread the version through as a parameter (or move the helper insidesuite).♻️ Proposed fix
const runElicitation = <S extends Schema.ConstraintEncoder<Record<string, unknown>, unknown>>( client: McpTestPeer["client"], - schema: S + schema: S, + protocolVersion: string ) => McpServer.elicit({ message: request.message, schema }).pipe( Effect.provideService( McpSchema.McpServerClient, McpSchema.McpServerClient.of({ clientId: 1, - protocolVersion: "2025-06-18", + protocolVersion, initializePayload: { - protocolVersion: "2025-06-18", + protocolVersion, capabilities: { elicitation: {} },Then pass
protocol.protocolVersionat each call site (lines 137, 158, 176, 201).🤖 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/McpConformance/ElicitationTest.ts` around lines 41 - 65, Update the module-scoped runElicitation helper to accept the protocol version as a parameter and use it for both McpServerClient.protocolVersion and initializePayload.protocolVersion. Pass protocol.protocolVersion at every runElicitation call site in suite, including the calls around lines 137, 158, 176, and 201.packages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.ts (2)
136-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe ordering test can't fail independently of the earlier test.
The expected values
["alpha", "beta"]are already in lexicographic order, so an accidental.sort()in the completion path would still pass. Making the fixture return deliberately non-alphabetical values (e.g.["beta", "alpha"]for the empty prefix) would give this assertion real signal.🤖 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/McpConformance/CompletionTest.ts` around lines 136 - 144, Update the completion fixture used by “MUST return completion values in order” so the empty-prefix response returns deliberately non-alphabetical values such as beta before alpha, and update the assertion to expect that same order. Keep the test focused on preserving server-provided ordering rather than sorting.
10-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extending
completeto cover the context/error cases and remove the duplicated request blocks.Lines 67-97, 98-115 and 117-134 re-implement the exact same initialize → notify → send flow, differing only by an optional
contextparam and by decoding an error instead of a result. An optionalcontextargument plus a rawcompleteRawvariant returning the undecoded response would remove three copies.♻️ Sketch
const complete = ( ref: { readonly type: "ref/prompt"; readonly name: string } | { readonly type: "ref/resource" readonly uri: string }, - argument: { readonly name: string; readonly value: string } + argument: { readonly name: string; readonly value: string }, + context?: { readonly arguments: Record<string, string> } ) => + completeRaw(ref, argument, context).pipe( + Effect.flatMap((message) => decodeCompletion(message.result)) + )🤖 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/McpConformance/CompletionTest.ts` around lines 10 - 30, Refactor the completion test helpers around complete to accept an optional context value and add a completeRaw variant that performs the shared initialize → notifyInitialized → send flow while returning the raw response. Update the context and error test cases to reuse these helpers, decoding successful results through complete and error responses through completeRaw, and remove their duplicated request setup.packages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.ts (2)
112-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case for the roots refresh.
This confirms a
listChanged-capable client triggersroots/list, but nothing covers the inverse: a client that does not advertiseroots.listChanged(or norootscapability at all) sendingnotifications/roots/list_changedshould not cause the server to issueroots/list. Given this PR changes roots refresh behaviour, that's the direction most likely to regress unnoticed.Also worth noting the test asserts only that the request was issued — the
respondat lines 127-129 is unasserted cleanup, so "refresh" adoption itself is untested.🤖 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/McpConformance/RootsTest.ts` around lines 112 - 130, Add a negative conformance test alongside “Root List Changes” that initializes clients without roots.listChanged, including no roots capability, sends notifications/roots/list_changed, and asserts no roots/list request is emitted. Keep the existing positive test, but ensure the new assertions verify the server does not initiate a refresh rather than relying on response cleanup.
16-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two capability tests exercise an identical path.
The only difference is
roots: {}vsroots: { listChanged: true }, and neither test asserts anything about the advertised value — both just check the outbound method. ThelistChangedvariant only becomes meaningful if it also asserts the refresh behaviour, which the "Root List Changes" test at lines 113-130 already covers. Consider dropping the second case or asserting the negotiated capability.🤖 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/McpConformance/RootsTest.ts` around lines 16 - 44, The two capability tests cover the same outbound roots/list request without validating the listChanged capability. Remove the redundant “list changes” test in the McpConformance roots test block, or update it to assert the negotiated listChanged capability; retain the existing roots request coverage and the separate “Root List Changes” behavior test.packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts (1)
316-446: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated subscription-target fixture.
The same
addResource({ resource: new McpSchema.Resource({ uri: "file:///subscription-target", ... }), annotations: Context.empty(), handle: ... })block appears five times (lines 320-331, 346-357, 377-388, 408-419, 435-446). ThemakeResourcehelper already defined at lines 283-291 for the list-changed test could be hoisted to module scope and reused here.🤖 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/McpConformance/ResourcesTest.ts` around lines 316 - 446, Hoist the existing makeResource helper used by the list-changed test to module scope, then replace each repeated subscription-target fixture in the Subscriptions tests with that helper when calling fixture.server.addResource. Preserve the current resource URI, annotations, and read-result behavior while removing the duplicated inline construction.packages/effect/src/unstable/ai/McpServer.ts (1)
920-972: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConfirm
jsonRpc()decode/encode round-tripping throughJSON.stringify/JSON.parseis intended.Each frame is re-serialized (Line 959) and each encoded response re-parsed (Line 966) purely to bridge the two serializers. It works, but it adds two extra JSON passes per message on the stdio hot path; consider decoding frames directly into
RpcMessageshapes instead of delegating to the string-basedjsonRpccodec.🤖 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 920 - 972, Replace the JSON.stringify/JSON.parse bridge in makeUnsafe’s decode and encode handlers with direct conversion between framing values and RpcMessage shapes. Preserve the existing frame batching validation, protocol selection, and framing behavior while eliminating the extra JSON serialization passes on the stdio path.
🤖 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/src/unstable/ai/McpServer.ts`:
- Around line 951-957: Update the synthetic invalid-batch Request created in the
batch handling flow to use a null/absent request identifier rather than an empty
string, so the downstream Exit failure and JSON-RPC encoder emit id: null for
the unidentifiable request. Preserve the existing MCP_INVALID_BATCH_METHOD
payload and headers.
- Around line 2078-2087: Update the MCP log filtering logic near the level
comparison to compare the original LoggingLevel ordinals directly, rather than
comparing values from mcpLogLevels. Preserve mcpLogLevels for Effect-side
logging, while ensuring “this level and higher” filtering distinguishes notice
from info and alert/emergency from critical.
- Around line 1051-1067: Update the Accept parsing and validation in the
request-handling flow to support media-range wildcards: treat */* as accepting
both application/json and text/event-stream, and application/* as accepting
application/json while preserving exact matches and q-value filtering. Keep
returning 406 only when either required response type is not accepted.
- Around line 1095-1114: The array branch in the MCP request handling flow must
bypass single-message validation on the batch array itself. When batches are
accepted by the selected transport, validate each entry individually and apply
the relevant session and initialize checks per entry, while preserving batch
rejection when the transport does not support JSON-RPC batches.
- Line 737: Update the cleanup around the sessions.byClientId.delete(clientId)
operation to distinguish HTTP sessions from other clients, using bySessionId for
HTTP session state instead of evicting the entry read by the notification loop.
Preserve the existing byClientId cleanup behavior for non-HTTP sessions and
ensure subsequent HTTP notifications retain their session information and
resource updates.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.ts`:
- Around line 55-67: Update the malformed-initialize assertions in LifecycleTest
to compare error.error.code exactly with McpSchema.INVALID_PARAMS_ERROR_CODE,
importing McpSchema as needed. Iterate invalidParams via entries() so each
request uses the entry’s index and params without unchecked indexed access,
while preserving the existing response ID and session-header assertions.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.ts`:
- Around line 254-271: Anchor
packages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.ts lines
254-271: capture the test.send result, decode it with test.decodeError, and
assert error.error.code equals McpSchema.INVALID_PARAMS_ERROR_CODE before
asserting promptInvocations is zero. Apply the same change in
packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts lines
262-276, asserting the expected resource error code before
resourceTemplateInvocations is zero.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts`:
- Around line 448-467: Register file:///subscription-sentinel with
fixture.addResource before fixture.initialize() in the test setup, ensuring its
resources/subscribe request succeeds before validating notification behavior.
Keep the existing subscription and notification assertions unchanged.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.ts`:
- Around line 106-126: Update the revision-specific tests in
packages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.ts:106-126
and 337-345. In the stdio batch-policy test, derive the expected
error-versus-batched-results assertion from protocol instead of hardcoding an
error. In the protocol-version-header test, gate the 400-status assertion on
whether the selected revision requires the Mcp-Protocol-Version header.
---
Nitpick comments:
In `@packages/effect/src/unstable/ai/McpServer.ts`:
- Around line 920-972: Replace the JSON.stringify/JSON.parse bridge in
makeUnsafe’s decode and encode handlers with direct conversion between framing
values and RpcMessage shapes. Preserve the existing frame batching validation,
protocol selection, and framing behavior while eliminating the extra JSON
serialization passes on the stdio path.
In
`@packages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.ts`:
- Around line 165-184: Update the “MUST return exactly one error response for a
failed request” test around the existing test.initialize,
test.notifyInitialized, and test.send flow to verify single delivery, matching
the stdio variant’s assertion. Keep the separate “MUST not include both result
and error in a response” test focused only on mutual exclusion, rather than
duplicating error-present/result-absent checks.
- Around line 46-95: Replace the hard-coded JSON-RPC error-code values in the
conformance tests, including the cases around the shown tests and the additional
referenced ranges, with the corresponding exported constants from McpSchema:
INVALID_REQUEST_ERROR_CODE, METHOD_NOT_FOUND_ERROR_CODE,
INVALID_PARAMS_ERROR_CODE, PARSE_ERROR_CODE, and any other applicable error-code
constants. Preserve each test’s existing assertions and behavior.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.ts`:
- Around line 136-144: Update the completion fixture used by “MUST return
completion values in order” so the empty-prefix response returns deliberately
non-alphabetical values such as beta before alpha, and update the assertion to
expect that same order. Keep the test focused on preserving server-provided
ordering rather than sorting.
- Around line 10-30: Refactor the completion test helpers around complete to
accept an optional context value and add a completeRaw variant that performs the
shared initialize → notifyInitialized → send flow while returning the raw
response. Update the context and error test cases to reuse these helpers,
decoding successful results through complete and error responses through
completeRaw, and remove their duplicated request setup.
In
`@packages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.ts`:
- Around line 41-65: Update the module-scoped runElicitation helper to accept
the protocol version as a parameter and use it for both
McpServerClient.protocolVersion and initializePayload.protocolVersion. Pass
protocol.protocolVersion at every runElicitation call site in suite, including
the calls around lines 137, 158, 176, and 201.
In
`@packages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.ts`:
- Around line 28-36: Refactor StructuredTool into a factory that accepts or
closes over protocol.protocolVersion, and have its McpSchema.EnabledWhen
predicate compare against that value instead of the hardcoded "2025-06-18".
Update makeFeaturesServerLayer to create/use the protocol-specific tool so
structured-output tests remain enabled for every revision under test.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.ts`:
- Around line 13-41: Refactor the duplicate initialization and request flow in
getPrompt and getPromptWire by defining getPrompt in terms of getPromptWire,
retaining only the additional decodeGetPrompt step in getPrompt and preserving
the existing result behavior.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts`:
- Around line 316-446: Hoist the existing makeResource helper used by the
list-changed test to module scope, then replace each repeated
subscription-target fixture in the Subscriptions tests with that helper when
calling fixture.server.addResource. Preserve the current resource URI,
annotations, and read-result behavior while removing the duplicated inline
construction.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.ts`:
- Around line 112-130: Add a negative conformance test alongside “Root List
Changes” that initializes clients without roots.listChanged, including no roots
capability, sends notifications/roots/list_changed, and asserts no roots/list
request is emitted. Keep the existing positive test, but ensure the new
assertions verify the server does not initiate a refresh rather than relying on
response cleanup.
- Around line 16-44: The two capability tests cover the same outbound roots/list
request without validating the listChanged capability. Remove the redundant
“list changes” test in the McpConformance roots test block, or update it to
assert the negotiated listChanged capability; retain the existing roots request
coverage and the separate “Root List Changes” behavior test.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.ts`:
- Around line 13-41: Consolidate the duplicated initialization, notification,
and tool-request setup in callTool and callToolWire by deriving one helper from
the other. Preserve callTool’s arguments_ support and decodeCallTool processing,
while keeping callToolWire’s raw decoded response behavior and the existing
request id and method values.
- Around line 227-244: Update the test around McpConformance.resetObservations
and the toolInvocations assertion to avoid relying on shared counter state:
capture the invocation count immediately before sending the invalid tools/call
request, then assert the count is unchanged afterward. Preserve the existing
validation-failure scenario and zero-handler-invocation expectation without
depending on other tests’ resets.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts`:
- Around line 97-104: Bound the message-draining loop around fixture.takeMessage
with Effect.timeout so it fails explicitly when neither the cancelled response
nor the ping response arrives. Preserve the existing assertions and break
condition for valid responses, and configure the timeout using the test’s
established timing conventions.
- Around line 139-201: Consolidate the three Progress smoke tests into a
table-driven set generated from an array of case names and params, using one
shared it.effect body for initialization, notification sending, and response
assertions. Preserve coverage for string tokens, numeric tokens, and the
optional total, and retain the NOTE only once near the shared test definition.
In `@packages/effect/test/unstable/ai/McpServer/McpServer.test.ts`:
- Around line 423-519: The negative isolation checks after nextResourceUpdate
are timing-sensitive because immediate Queue.poll calls may miss delayed
cross-session notifications. Update the resource subscription test’s assertions
to allow pending effects to run, such as yielding or advancing TestClock, then
drain or inspect both client outbound queues and assert that each session
received only its subscribed URI.
In `@packages/effect/test/unstable/ai/McpServer/TestUtils/McpHttpHarness.ts`:
- Around line 20-33: Update the fetch wrapper’s session header injection to set
Mcp-Session-Id only when sessionId is non-null and the request does not already
contain that header, matching the existing Mcp-Protocol-Version guard and
preserving caller-supplied values.
- Around line 35-58: Update postText and post in the MCP HTTP harness to route
requests through the existing fetch helper instead of invoking handler directly,
so responses tracking and header propagation are consistent; preserve postText’s
request body and headers behavior, and keep post delegating to postText.
In `@packages/effect/test/unstable/ai/McpServer/TestUtils/McpStdioHarness.ts`:
- Around line 113-132: Update the framing loop in the forked Effect around
routeFrame and JSON.parse to catch parse failures, then fail loudly with an
error that includes the offending line. Ensure malformed stdout terminates or
propagates the reader failure instead of silently killing the fiber and leaving
later takeMessage/sendRequest calls blocked.
In `@packages/effect/test/unstable/ai/McpServer/v2025_06_18.test.ts`:
- Around line 34-59: The `notifications/progress` smoke test currently in the
entry-point test file should be moved into `McpConformance/UtilitiesTest.ts`,
alongside the other tests under the existing `Utilities > Progress` suite.
Preserve the test’s behavior and assertions, and do not retain a duplicate in
the original file.
In `@packages/effect/typetest/unstable/ai/McpServer.tst.ts`:
- Around line 73-75: Update the type assertion in the “should expose the
supported protocol adapter” test to verify that "v2025_06_18" is assignable to
keyof typeof McpProtocol, rather than asserting the entire exported key set
exactly. Keep the ProtocolVersion assertion 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: 2adfc1bb-6e3e-4d60-bd94-e7de9c7480fd
📒 Files selected for processing (44)
.changeset/brown-peas-enter.md.changeset/clean-lions-cancel.md.changeset/fair-logs-listen.md.changeset/fair-sampling-content.md.changeset/fix-mcp-completion-context.md.changeset/fix-mcp-request-errors.md.changeset/fruity-sloths-walk.md.changeset/fuzzy-batches-stop.md.changeset/green-ads-camp.md.changeset/quiet-owls-validate.md.changeset/refresh-mcp-roots.md.changeset/resource-subscriptions.md.changeset/tiny-lizards-correct.mdpackages/effect/src/unstable/ai/McpProtocol.tspackages/effect/src/unstable/ai/McpSchema.tspackages/effect/src/unstable/ai/McpServer.tspackages/effect/src/unstable/ai/internal/mcpProtocol.tspackages/effect/src/unstable/rpc/RpcMessage.tspackages/effect/src/unstable/rpc/RpcSerialization.tspackages/effect/src/unstable/rpc/RpcServer.tspackages/effect/test/unstable/ai/McpProtocol.test.tspackages/effect/test/unstable/ai/McpServer/Lifecycle.test.tspackages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LoggingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/McpConformance.tspackages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.tspackages/effect/test/unstable/ai/McpServer/McpConformance/McpTestPeer.tspackages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/SamplingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.tspackages/effect/test/unstable/ai/McpServer/McpServer.test.tspackages/effect/test/unstable/ai/McpServer/TestUtils/McpHttpHarness.tspackages/effect/test/unstable/ai/McpServer/TestUtils/McpServerLayer.tspackages/effect/test/unstable/ai/McpServer/TestUtils/McpStdioHarness.tspackages/effect/test/unstable/ai/McpServer/utils.tspackages/effect/test/unstable/ai/McpServer/v2025_06_18.test.tspackages/effect/typetest/unstable/ai/McpServer.tst.ts
💤 Files with no reviewable changes (2)
- packages/effect/test/unstable/ai/McpServer/utils.ts
- packages/effect/test/unstable/ai/McpServer/Lifecycle.test.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
acf2d74 to
9047eb2
Compare
|
@coderabbitai Pagination-specific traversal tests are intentionally omitted. |
|
Tip For best results, initiate chat on the files or code changes.
🐇💭 ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.ts (1)
165-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winVerify duplicate error responses on the stdio transcript.
Lines 165-184 inspect one transport response, so they cannot detect a second JSON-RPC error emitted over stdio. Reuse
makeMcpStdioHarnessas in the successful-response test and assert that a second message times out; otherwise duplicate error responses can pass this test.🤖 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/McpConformance/BaseProtocolTest.ts` around lines 165 - 184, Update the failed-request conformance test around “MUST return exactly one error response” to use makeMcpStdioHarness, matching the successful-response test’s stdio transcript flow. Consume and validate the first error response, then assert that attempting to read a second message times out, while preserving the existing error/result and request-id assertions.packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts (1)
139-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise
notifications/progressthrough a valid active request. These cases only prove the transport accepts arbitrary notifications; they don’t cover the spec path where progress is tied to an active request, so a server that ignores progress updates still passes. Either drive them from a real request withprogressTokenin_meta, or move them out of conformance coverage and label them as smoke tests.🤖 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/McpConformance/UtilitiesTest.ts` around lines 139 - 202, The notifications/progress cases in the Progress describe block must exercise progress updates associated with an active request rather than standalone notifications. Update the tests around McpConformance, initialize, and test.send to issue a real request carrying progressToken in _meta, then send and validate the corresponding progress notification; alternatively remove these cases from conformance coverage and explicitly retain them only as smoke tests.
🧹 Nitpick comments (1)
packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts (1)
97-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the post-cancellation receive loop.
The
while (true)loop has no local timeout; if the ping response is lost, this test hangs until the runner’s global timeout instead of producing a focused failure. Add a timeout aroundfixture.takeMessageor the whole loop.🤖 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/McpConformance/UtilitiesTest.ts` around lines 97 - 104, Bound the post-cancellation receive loop around fixture.takeMessage so it fails with a focused timeout if the ping response is not received. Preserve the existing assertions for cancelled requests and the expected empty ping result while ensuring the loop cannot run indefinitely.
🤖 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/src/unstable/ai/McpSchema.ts`:
- Around line 1476-1479: Update the outputSchema field in the MCP schema
definition to validate only object-shaped JSON Schema values instead of using
Schema.Any, while preserving its optional nature. Add a negative test covering
scalar or array outputSchema inputs and verify they are rejected.
---
Outside diff comments:
In
`@packages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.ts`:
- Around line 165-184: Update the failed-request conformance test around “MUST
return exactly one error response” to use makeMcpStdioHarness, matching the
successful-response test’s stdio transcript flow. Consume and validate the first
error response, then assert that attempting to read a second message times out,
while preserving the existing error/result and request-id assertions.
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts`:
- Around line 139-202: The notifications/progress cases in the Progress describe
block must exercise progress updates associated with an active request rather
than standalone notifications. Update the tests around McpConformance,
initialize, and test.send to issue a real request carrying progressToken in
_meta, then send and validate the corresponding progress notification;
alternatively remove these cases from conformance coverage and explicitly retain
them only as smoke tests.
---
Nitpick comments:
In `@packages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.ts`:
- Around line 97-104: Bound the post-cancellation receive loop around
fixture.takeMessage so it fails with a focused timeout if the ping response is
not received. Preserve the existing assertions for cancelled requests and the
expected empty ping result while ensuring the loop cannot run indefinitely.
🪄 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: c17c2cee-7b2d-429a-91c3-ff177b301da4
📒 Files selected for processing (30)
.changeset/clean-lions-cancel.md.changeset/fair-logs-listen.md.changeset/fair-sampling-content.md.changeset/fix-mcp-completion-context.md.changeset/fuzzy-batches-stop.md.changeset/green-ads-camp.md.changeset/refresh-mcp-roots.md.changeset/resource-subscriptions.mdpackages/effect/src/unstable/ai/McpProtocol.tspackages/effect/src/unstable/ai/McpSchema.tspackages/effect/src/unstable/ai/McpServer.tspackages/effect/src/unstable/ai/internal/mcpProtocol.tspackages/effect/src/unstable/rpc/RpcServer.tspackages/effect/test/unstable/ai/McpProtocol.test.tspackages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LoggingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.tspackages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/SamplingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.tspackages/effect/test/unstable/ai/McpServer/McpServer.test.tspackages/effect/test/unstable/ai/McpServer/v2025_06_18.test.tspackages/effect/typetest/unstable/ai/McpServer.tst.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- .changeset/green-ads-camp.md
- .changeset/fair-logs-listen.md
- .changeset/fair-sampling-content.md
- packages/effect/src/unstable/ai/McpProtocol.ts
- .changeset/refresh-mcp-roots.md
- packages/effect/src/unstable/rpc/RpcServer.ts
- .changeset/fuzzy-batches-stop.md
- .changeset/resource-subscriptions.md
- packages/effect/typetest/unstable/ai/McpServer.tst.ts
- packages/effect/test/unstable/ai/McpProtocol.test.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/LoggingTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.ts
- packages/effect/test/unstable/ai/McpServer/v2025_06_18.test.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.ts
- packages/effect/src/unstable/ai/internal/mcpProtocol.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/SamplingTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts
- packages/effect/test/unstable/ai/McpServer/McpServer.test.ts
- packages/effect/src/unstable/ai/McpServer.ts
20dc68c to
7657056
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/effect/test/unstable/ai/McpServer/McpServer.test.ts (1)
516-519: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNegative isolation assertion can pass before a cross-delivery would arrive.
Queue.pollruns immediately after each session's expected update is received, so a wrongly-routed notification that is still in flight would not be observed — the assertion could pass even if isolation regressed. Draining both queues after a short yield/TestClockadvance, or asserting exact received counts, would make the negative case reliable.🤖 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/McpServer.test.ts` around lines 516 - 519, The negative isolation checks after nextResourceUpdate can run before misrouted notifications arrive. Update the test around nextResourceUpdate and the client1Outbound/client2Outbound Queue.poll assertions to allow pending deliveries to settle, using the test’s existing yield or TestClock mechanism, then drain both queues and verify they remain empty (or assert exact received counts).packages/effect/src/unstable/ai/McpServer.ts (2)
764-779: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDecoding the notification payload per client is redundant.
Both
LoggingMessageNotificationandResourceUpdatedNotificationdecodes depend only onrequest, yet they run once per initialized client. Hoisting them above theforloop (or decoding lazily once) avoids repeated schema work on every broadcast.🤖 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 764 - 779, The notification payloads are decoded redundantly for each client during broadcast. In the broadcast flow surrounding the client iteration, decode `LoggingMessageNotification.payloadSchema` and `ResourceUpdatedNotification.payloadSchema` once per request before the loop, then reuse the decoded level and URI inside the `notifications/message` and `notifications/resources/updated` branches while preserving their existing filtering behavior.
327-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a named constant for the resource-not-found error code.
-32002is still inlined here while the other protocol error codes live inMcpSchema. Adding aRESOURCE_NOT_FOUND_ERROR_CODEexport there and using it here would keep the error taxonomy centralized.🤖 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 327, Replace the inline -32002 value in the resource lookup failure with a named RESOURCE_NOT_FOUND_ERROR_CODE exported from McpSchema. Update the corresponding McpErrorBase construction to reference that centralized constant while preserving the existing resource-not-found message and behavior.
🤖 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/src/unstable/ai/McpServer.ts`:
- Around line 1975-1984: Update the resources/subscribe handler near
getClientSession so it does not return success when the server does not support
subscriptions or no client session can be resolved. Validate the advertised
capability and session before adding the URI to resourceSubscriptions, and
return the established MethodNotFound or InvalidParams error for the
corresponding failure instead of silently ignoring it; preserve the successful
empty response only when the subscription is applied.
- Around line 659-674: Update the notification handling branch in the request
handler for "notifications/roots/list_changed" so the client["roots/list"]
refresh does not block the HTTP POST or notification processing. Fork the
existing Effect operation or apply an appropriate timeout while preserving the
current client lookup and scoped resource handling.
- Around line 1047-1053: Update the routes assembled in the Layer.mergeAll block
so OPTIONS requests to options.path are handled by the CORS preflight response
rather than methodNotAllowed. Ensure the response includes the expected CORS
headers for configured allowedOrigins, while preserving methodNotAllowed for the
other unsupported methods.
---
Nitpick comments:
In `@packages/effect/src/unstable/ai/McpServer.ts`:
- Around line 764-779: The notification payloads are decoded redundantly for
each client during broadcast. In the broadcast flow surrounding the client
iteration, decode `LoggingMessageNotification.payloadSchema` and
`ResourceUpdatedNotification.payloadSchema` once per request before the loop,
then reuse the decoded level and URI inside the `notifications/message` and
`notifications/resources/updated` branches while preserving their existing
filtering behavior.
- Line 327: Replace the inline -32002 value in the resource lookup failure with
a named RESOURCE_NOT_FOUND_ERROR_CODE exported from McpSchema. Update the
corresponding McpErrorBase construction to reference that centralized constant
while preserving the existing resource-not-found message and behavior.
In `@packages/effect/test/unstable/ai/McpServer/McpServer.test.ts`:
- Around line 516-519: The negative isolation checks after nextResourceUpdate
can run before misrouted notifications arrive. Update the test around
nextResourceUpdate and the client1Outbound/client2Outbound Queue.poll assertions
to allow pending deliveries to settle, using the test’s existing yield or
TestClock mechanism, then drain both queues and verify they remain empty (or
assert exact received counts).
🪄 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: 1fd39409-3c2b-447c-888c-27a1ef1c20ac
📒 Files selected for processing (38)
.changeset/brown-peas-enter.md.changeset/clean-lions-cancel.md.changeset/fair-logs-listen.md.changeset/fair-sampling-content.md.changeset/fix-mcp-completion-context.md.changeset/fix-mcp-request-errors.md.changeset/fruity-sloths-walk.md.changeset/fuzzy-batches-stop.md.changeset/green-ads-camp.md.changeset/quiet-owls-validate.md.changeset/refresh-mcp-roots.md.changeset/resource-subscriptions.md.changeset/tiny-lizards-correct.mdpackages/effect/src/unstable/ai/McpProtocol.tspackages/effect/src/unstable/ai/McpSchema.tspackages/effect/src/unstable/ai/McpServer.tspackages/effect/src/unstable/ai/internal/mcpProtocol.tspackages/effect/src/unstable/rpc/RpcMessage.tspackages/effect/src/unstable/rpc/RpcSerialization.tspackages/effect/src/unstable/rpc/RpcServer.tspackages/effect/test/unstable/ai/McpProtocol.test.tspackages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/LoggingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.tspackages/effect/test/unstable/ai/McpServer/McpConformance/PromptsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/RootsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/SamplingTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.tspackages/effect/test/unstable/ai/McpServer/McpConformance/UtilitiesTest.tspackages/effect/test/unstable/ai/McpServer/McpServer.test.tspackages/effect/test/unstable/ai/McpServer/TestUtils/McpServerLayer.tspackages/effect/test/unstable/ai/McpServer/v2025_06_18.test.tspackages/effect/typetest/unstable/ai/McpServer.tst.ts
🚧 Files skipped from review as they are similar to previous changes (30)
- .changeset/green-ads-camp.md
- .changeset/clean-lions-cancel.md
- .changeset/brown-peas-enter.md
- .changeset/fix-mcp-completion-context.md
- .changeset/quiet-owls-validate.md
- .changeset/resource-subscriptions.md
- .changeset/fuzzy-batches-stop.md
- .changeset/refresh-mcp-roots.md
- packages/effect/test/unstable/ai/McpServer/TestUtils/McpServerLayer.ts
- .changeset/fair-logs-listen.md
- .changeset/fair-sampling-content.md
- .changeset/fruity-sloths-walk.md
- packages/effect/test/unstable/ai/McpServer/McpConformance/CompletionTest.ts
- packages/effect/src/unstable/ai/McpProtocol.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/LoggingTest.ts
- packages/effect/test/unstable/ai/McpProtocol.test.ts
- packages/effect/src/unstable/ai/internal/mcpProtocol.ts
- .changeset/tiny-lizards-correct.md
- packages/effect/test/unstable/ai/McpServer/McpConformance/LifecycleTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ElicitationTest.ts
- packages/effect/test/unstable/ai/McpServer/v2025_06_18.test.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/SamplingTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/BaseProtocolTest.ts
- packages/effect/typetest/unstable/ai/McpServer.tst.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/TransportsTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ResourcesTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/ToolsTest.ts
- packages/effect/test/unstable/ai/McpServer/McpConformance/McpConformanceFixtures.ts
- packages/effect/src/unstable/ai/McpSchema.ts
- packages/effect/src/unstable/rpc/RpcSerialization.ts
19f17f1 to
431f0d1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
IMax153
left a comment
IMax153
left a comment
There was a problem hiding this comment.
These changes look great @lloydrichards - really fantastic work 👍
|
Looks like the RPC changes have effected Shard 😬 I'll fix it when I land 🛬 |
b5b3799 to
c763cbc
Compare
c763cbc to
255cdb7
Compare
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Type
Description
At the moment the tests for the
McpServerare a little thin, especially when it comes to spec compliance. Thankfully a spec is basically a pre-written unit test so it should be quite easy to translate all the requirements for the different protocol versions into a solid test suite that test any given protocol version:What I've done here is first convert the various spec versions into a collection of unit tests (20a9079), build a consistent
McpServerharness for Http and Stdio which could be used depending on the spec, and then setup a loop to generate out all the specifications (ceaed62), looking for gaps in the current implementation (and skipping). Lastly I reviewed what was missing and cleaned up the fixtures and test utilities so its clear what is being tested and what gaps we have based on a comprehensive conformance suite:Vitest Results
✓ effect test/unstable/ai/McpServer/v2025_06_18.test.ts (160 tests) 219ms ✓ Mcp Conformance (2025-06-18) (9) ✓ Lifecycle (9) ✓ Lifecycle Phases (9) ✓ Initialization (5) ✓ MUST reject non-ping requests before initialize 6ms ✓ MUST reject initialized notifications before initialize 1ms ✓ SCHEMA requires protocolVersion, capabilities, and clientInfo 6ms ✓ SCHEMA returns server capabilities and implementation information 3ms ✓ MUST accept initialized after a successful initialize response 2ms ✓ Version Negotiation (2) ✓ MUST echo a requested version supported by the server 1ms ✓ SHOULD negotiate an unsupported requested version to a supported version 1ms ✓ Capability Negotiation (1) ✓ SCHEMA advertises the registered prompt, resource, and tool capabilities 8ms ✓ Operation (1) ✓ MUST continue to use the version negotiated during initialization 2ms ✓ Mcp Conformance (2025-06-18) (17) ✓ Base Protocol (17) ✓ Messages (16) ✓ Requests (8) ✓ SCHEMA accepts JSON-RPC 2.0 requests with string identifiers 4ms ✓ SCHEMA accepts JSON-RPC 2.0 requests with numeric identifiers 1ms ✓ MUST reject requests with an invalid JSON-RPC version 1ms ✓ MUST return method not found for unknown request methods 1ms ✓ MUST return invalid params for request payloads that do not match the method schema 1ms ✓ MUST not reply to unknown notifications 1ms ✓ MUST not reply to notifications with invalid params 1ms ✓ MUST reject requests with invalid identifiers 1ms ✓ Responses (5) ✓ MUST return exactly one result response for a successful request 4ms ✓ MUST return exactly one error response for a failed request 2ms ✓ SCHEMA preserves the request identifier in result responses 1ms ✓ SCHEMA preserves the request identifier in error responses 1ms ✓ MUST not include both result and error in a response 1ms ✓ Notifications (1) ✓ MUST accept notifications without an identifier and send no response 1ms ✓ MUST return a parse error for malformed JSON 1ms ✓ MUST return an invalid request error for malformed JSON-RPC messages 1ms ✓ General fields (1) ✓ SCHEMA preserves additional result metadata fields 0ms ✓ Mcp Conformance (2025-06-18) (26) ✓ Transports (26) ✓ stdio (5) ✓ MUST exchange compact UTF-8 newline-delimited JSON-RPC records 2ms ✓ SCENARIO parses UTF-8 JSON-RPC records split across input chunks 2ms ✓ SCENARIO processes consecutive stdio messages independently 2ms ✓ SCENARIO applies the revision-specific stdio batch policy 2ms ✓ MUST shut down when the client closes stdin 1ms ✓ Streamable HTTP (21) ✓ Sending Messages to the Server (8) ✓ MUST accept JSON-RPC requests through POST on the MCP endpoint 2ms ✓ MUST accept JSON-RPC notifications through POST on the MCP endpoint 1ms ✓ MUST accept JSON-RPC responses through POST on the MCP endpoint 1ms ✓ MUST require the application/json content type for POST requests 1ms ✓ MUST require clients to accept application/json and text/event-stream 1ms ✓ MUST return application/json for a single JSON-RPC response 0ms ✓ MUST return an empty 202 response for accepted notifications and responses 1ms ✓ MUST reject unsupported HTTP methods with method not allowed 0ms ✓ Listening for Messages from the Server (1) ✓ MUST return method not allowed when GET SSE is not offered 0ms ✓ Session Management (7) ✓ SCENARIO returns an MCP session identifier during initialization 0ms ✓ SCENARIO uses distinct UUIDv4 session identifiers 1ms ✓ MUST require the returned session identifier on subsequent HTTP requests 1ms ✓ MUST reject an unknown session identifier with not found 0ms ✓ SCENARIO declines client session termination without invalidating the session 1ms ✓ MUST reject initialize requests carrying a session identifier 1ms ✓ SCENARIO keeps two distinct POST sessions live 1ms ✓ Protocol Version Header (4) ✓ MUST apply the revision-specific protocol header requirement 0ms ✓ MUST accept the negotiated protocol version 1ms ✓ MUST reject an unsupported protocol version with bad request 0ms ✓ SCENARIO replays the selected protocol version on HTTP responses 1ms ✓ Security (1) ✓ MUST validate the Origin header before every MCP route 1ms ✓ Mcp Conformance (2025-06-18) (8) ✓ Utilities (8) ✓ Ping (1) ✓ MUST respond to a client ping with an empty result 4ms ✓ Cancellation (4) ✓ MUST not send a response to a cancellation notification 1ms ✓ SHOULD stop work and suppress the response after cancellation 3ms ✓ SHOULD ignore cancellation for an unknown request identifier 1ms ✓ SHOULD ignore cancellation for an already completed request identifier 1ms ✓ Progress (3) ✓ MUST accept string progress tokens 1ms ✓ MUST accept numeric progress tokens 1ms ✓ SCHEMA accepts the optional total 1ms ✓ Mcp Conformance (2025-06-18) (22) ✓ Tools (22) ✓ Capabilities (3) ✓ MUST advertise the tools capability when tools are registered 3ms ✓ MUST NOT advertise the tools capability when tools are not supported 1ms ✓ MUST advertise listChanged when tool list change notifications are supported 1ms ✓ Listing Tools (4) ✓ MUST list every tool visible to the initialized client 2ms ✓ SCHEMA preserves tool names and descriptions 1ms ✓ MUST return each tool input schema 1ms ✓ MUST return each declared tool output schema 1ms ✓ Calling Tools (14) ✓ MUST call a registered tool with valid arguments 3ms ✓ MUST reject an unknown tool name with a protocol error 1ms ✓ MUST reject arguments that do not match the input schema with a protocol error 2ms ✓ MUST not invoke a tool handler when argument validation fails 1ms ✓ SCHEMA returns text content 1ms ✓ SCHEMA returns image content 1ms ✓ SCHEMA returns audio content 1ms ✓ SCHEMA returns resource links 1ms ✓ SCHEMA returns embedded resources 1ms ✓ MUST return multiple content items in order 1ms ✓ SCHEMA returns structured content 1ms ✓ MUST return tool execution failures with isError 1ms ✓ MUST keep tool execution errors distinct from protocol errors 2ms ✓ SHOULD not expose defects or internal error details 1ms ✓ List Changed Notification (1) ✓ SHOULD send a tool list changed notification when the advertised list changes 4ms ✓ Mcp Conformance (2025-06-18) (20) ✓ Resources (20) ✓ Capabilities (4) ✓ MUST advertise resources when resources are registered 2ms ✓ MUST NOT advertise resources when resources are not supported 1ms ✓ MUST NOT advertise resource subscriptions when they are unsupported 0ms ✓ MUST advertise listChanged when resource list change notifications are supported 1ms ✓ Listing Resources (2) ✓ MUST list every resource visible to the initialized client 1ms ✓ SCHEMA preserves resource URI, name, description, and MIME type 1ms ✓ Reading Resources (5) ✓ MUST read text resource contents 1ms ✓ MUST read binary resource contents as base64 1ms ✓ SCHEMA preserves the resource URI and MIME type in returned contents 1ms ✓ MUST return multiple resource contents in order 1ms ✓ SHOULD return resource not found for an unknown resource URI 1ms ✓ Resource Templates (3) ✓ MUST list every registered resource template 1ms ✓ MUST match and decode a concrete resource-template URI 1ms ✓ MUST not invoke the handler when template parameter decoding fails 1ms ✓ List Changed Notification (1) ✓ SHOULD send a resource list changed notification when the advertised list changes 4ms ✓ Subscriptions (5) ✓ MUST subscribe to a resource when subscriptions are advertised 2ms ✓ MUST send update notifications only for subscribed resources 2ms ✓ MUST include the updated resource URI in each notification 2ms ✓ MUST unsubscribe from resource updates 2ms ✓ MUST not send updates after a resource is unsubscribed 2ms ✓ Mcp Conformance (2025-06-18) (18) ✓ Prompts (18) ✓ Capabilities (3) ✓ MUST advertise prompts when prompts are registered 2ms ✓ MUST NOT advertise prompts when prompts are not supported 1ms ✓ MUST advertise listChanged when prompt list change notifications are supported 0ms ✓ Listing Prompts (3) ✓ MUST list every prompt visible to the initialized client 1ms ✓ SCHEMA preserves prompt names, descriptions, and arguments 1ms ✓ MUST mark required and optional prompt arguments correctly 1ms ✓ Getting Prompts (11) ✓ MUST get a registered prompt without arguments 2ms ✓ MUST get a registered prompt with valid arguments 1ms ✓ SHOULD reject an unknown prompt name with Invalid Params 1ms ✓ SHOULD reject missing required prompt arguments with Invalid Params 3ms ✓ SHOULD reject prompt arguments with invalid values 1ms ✓ MUST not invoke the prompt handler when argument validation fails 1ms ✓ SCHEMA preserves the prompt description and message order 1ms ✓ MUST return text message content 1ms ✓ MUST return image message content 1ms ✓ MUST return audio message content 1ms ✓ MUST return embedded resource message content 1ms ✓ List Changed Notification (1) ✓ SHOULD send a prompt list changed notification when the advertised list changes 3ms ✓ Mcp Conformance (2025-06-18) (9) ✓ Completion (9) ✓ Capabilities (1) ✓ MUST advertise completions when argument completion is supported 2ms ✓ Requesting Completions (8) ✓ MUST complete a prompt argument 1ms ✓ MUST complete a resource template argument 1ms ✓ MUST pass previously resolved argument context to the completion handler 1ms ✓ SHOULD reject an unknown prompt reference with Invalid Params 1ms ✓ MUST reject an unknown argument name 1ms ✓ MUST return completion values in order 1ms ✓ SCHEMA returns the total and additional-results indicator 1ms ✓ MUST return at most one hundred completion values 1ms ✓ Mcp Conformance (2025-06-18) (10) ✓ Logging (10) ✓ Capabilities (1) ✓ MUST advertise logging when log notifications are supported 2ms ✓ Setting Log Level (5) ✓ MUST accept every specified log level 5ms ✓ MUST reject an unknown log level 1ms ✓ SHOULD update the minimum level for subsequent operations 2ms ✓ SHOULD send notifications at the selected level and higher 4ms ✓ MUST not send notifications below the selected level 2ms ✓ Log Message Notifications (4) ✓ SCHEMA preserves the log level, logger name, and data 0ms ✓ MUST allow arbitrary JSON-compatible log data 0ms ✓ MUST emit log messages as notifications without an identifier 1ms ✓ SCENARIO does not corrupt the stdio protocol stream with log output 2ms ✓ Mcp Conformance (2025-06-18) (6) ✓ Roots (6) ✓ Capabilities (2) ✓ MUST send roots requests when the client advertises roots 1ms ✓ MUST accept roots requests when the client advertises list changes 0ms ✓ Listing Roots (3) ✓ MUST accept roots with file URIs and preserve optional names 1ms ✓ MAY accept an empty roots list 0ms ✓ MUST surface client errors returned by roots/list 1ms ✓ Root List Changes (1) ✓ SHOULD refresh roots after a capable client reports a list change 2ms ✓ Mcp Conformance (2025-06-18) (6) ✓ Sampling (6) ✓ Capabilities (1) ✓ MUST send sampling requests when the client advertises sampling 1ms ✓ Creating Messages (5) ✓ MUST preserve message order and sampling request options 0ms ✓ MUST accept and decode text sampling content 0ms ✓ MUST accept image sampling content 0ms ✓ MUST accept audio sampling content 0ms ✓ MUST surface sampling errors returned by the client 0ms ✓ Mcp Conformance (2025-06-18) (6) ✓ Elicitation (6) ✓ Capabilities (1) ✓ MUST send elicitation requests when the client advertises elicitation 0ms ✓ Form Mode (5) ✓ MUST send the message and requested primitive form schema 0ms ✓ MUST decode accepted content against the requested schema 0ms ✓ SCENARIO returns a typed failure when the user declines 0ms ✓ SCENARIO interrupts the operation when the user cancels 0ms ✓ MUST reject accepted content that does not match the requested schema 0ms ✓ Mcp Conformance (2025-06-18) (3) ✓ Utilities (1) ✓ Progress (1) ✓ SCHEMA accepts the optional progress message 1ms ✓ Transport-specific behavior (2) ✓ MUST reject JSON-RPC batches 0ms ✓ MUST require the negotiated protocol-version header after initialization 0ms Test Files 1 passed (1) Tests 160 passed (160) Start at 23:01:32 Duration 535msComformance Todos
Part of building out the initial suite was getting to discover the gaps in the current McpSchema to the v2025-06-18 spec. What I've done then is go through each issue and align them to the conformance so all tests pass now:
Gaps
Enhancements
How to Review
There are a few places that are important to review, specifically the harness and fixutres for the McpServer as well as the conformance suite as these are used extensively in the testing and need to be idiomatic effect implmentations.
After this I would recommend setting up an agent to loop over the active/skipped issues to verify that failure of tests is possible (never trust a test you havent seen fail). What I've been using is something like:
example prompt
Use something cheap with a low thinking as this will take a while.
260728_conformance-suite_audit.json
260729_conformance-suite_audit.json
Related
Summary by CodeRabbit
New Features
hasMore.logging/setLevel).Bug Fixes
Tests