fix(server): return -32602 for nonexistent resources (SEP-2164)#2267
fix(server): return -32602 for nonexistent resources (SEP-2164)#2267mattzcarey wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 5215acf The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
1f43d9c to
cda6637
Compare
|
Rebased onto current Follow-up fixes included in the pushed branch:
Verification run on the rebased branch:
Two broad-suite attempts were intentionally superseded by the direct Vitest commands above: the e2e package-script form ran the whole suite and hit the known |
|
Addressed the two new review threads in
Verification:
|
There was a problem hiding this comment.
Thanks for addressing both earlier threads in e264f39 — the verbatim data.uri echo (with the non-canonical-URI regression test) and the changeset supersession note both look right, and I found no new issues on this revision. I'm leaving the final call to a maintainer since this changes wire-level error-code behavior and reverses the deliberate #1389 direction.
Extended reasoning...
Overview
This PR implements SEP-2164: McpServer's resources/read not-found path now throws -32602 (InvalidParams) with the verbatim requested URI in data.uri, deprecates ProtocolErrorCode.ResourceNotFound (-32002) while keeping it exported for legacy-server matching, adds setSupportedProtocolVersions forwarding on NodeStreamableHTTPServerTransport so server-configured version lists are honored by HTTP header validation, opts the conformance fixture into the draft protocol version, and updates e2e/integration tests, the requirements manifest, and the conformance expected-failures baseline. The latest commit (e264f39) addressed both of my earlier inline comments: data.uri now echoes request.params.uri verbatim (with a non-canonical-URI regression test), and the changeset notes that it supersedes the #1389 / 2.0.0-alpha.1 change.
Security risks
None identified. The change affects which JSON-RPC error code is returned for unknown resources and protocol-version header validation plumbing; no auth, crypto, or input-trust boundaries are altered. Echoing the client-supplied URI back in error data is a standard correlation aid and does not expose server-side data.
Level of scrutiny
This warrants maintainer review rather than auto-approval: it changes observable wire behavior (error code emitted by every SDK-based server for unknown resources), reverses a recent deliberate decision (#1389), deprecates a public enum member, and the author's original draft explicitly raised a direction question (unconditional flip vs. version-gated) that a maintainer should sign off on even though the author resolved it in favor of the spec's unconditional MUST. The conformance expected-failures entry retained for sep-2164-resource-not-found (stateless-path limitation) is also a judgment call maintainers may want to confirm.
Other factors
The implementation is internally consistent: protocol.ts already calls transport.setSupportedProtocolVersions?.() during connect, so the new forwarding method on the Node wrapper closes a real gap, and there is a dedicated middleware test for it. Test coverage for the error-code change is good (integration, e2e, plus the new verbatim-URI regression). No bugs were found by the bug-hunting system on this revision, and both prior review threads are resolved.
e264f39 to
d615984
Compare
There was a problem hiding this comment.
Both of my earlier review threads are addressed in the latest commits (data.uri now echoes the request verbatim, and the changeset notes it supersedes the #1389 change) and I found no further issues — but since this changes protocol error-code behavior per SEP-2164 (resolved as an unconditional flip rather than version-gated) and bundles the protocol-version forwarding fix for the Node transport, it should get a maintainer's sign-off rather than an automated approval.
Extended reasoning...
Overview
This PR flips the resources/read not-found error from -32002 (ResourceNotFound) back to -32602 (InvalidParams) per SEP-2164, adding the requested URI verbatim in data.uri, deprecating (but keeping) ProtocolErrorCode.ResourceNotFound, and updating integration/e2e/conformance tests accordingly. It also adds setSupportedProtocolVersions() to NodeStreamableHTTPServerTransport so the existing optional hook in Protocol.connect() (packages/core/src/shared/protocol.ts:433) reaches the wrapped web-standard transport, with a new test, plus conformance fixture/expected-failures updates.
Security risks
None identified. The error-code change does not alter authorization or input handling; data.uri echoes only the client's own request parameter back to that client, so there is no new data-exposure surface. The protocol-version forwarding only widens accepted mcp-protocol-version headers to versions the server explicitly configured.
Level of scrutiny
This is wire-protocol behavior in the core server package — it changes what error code every SDK-based server emits for unknown resources, reversing the deliberate #1389 change. The author originally flagged the unconditional-vs-version-gated question as an open maintainer decision and then resolved it themselves on rebase; that direction call (and cross-SDK consistency with python-sdk) is exactly the kind of judgment a maintainer should confirm. The transport-forwarding fix is small and correct (it implements an already-defined optional Transport method, mirroring the web-standard transport), but bundling it here also slightly broadens the PR's scope.
Other factors
My two prior inline comments (URL-normalized data.uri and the contradictory pending #1389 changeset prose) were both addressed in the latest commits, and I verified the fixes in the diff — including a new integration regression test with a non-canonical URI. Test coverage is good: integration, e2e requirement manifest, and a Node middleware test for the configured-protocol-version path. The conformance expected-failures.yaml entry for sep-2164-resource-not-found remains with an updated rationale (stateless-path limitation), which the author explained in the PR thread. No bugs were found in this review run; the deferral is purely about the protocol-direction and scope judgment.
Per SEP-2164, the draft spec upgrades resource-not-found handling from SHOULD return -32002 (2025-11-25) to MUST return -32602 (Invalid params). The requested URI is included in the error's data.uri so clients can still distinguish not-found from other invalid-params errors. ProtocolErrorCode.ResourceNotFound (-32002) is deprecated but remains exported, since clients SHOULD still accept it from older servers. Closes #2194
d615984 to
b3c60d3
Compare
Summary
Implements SEP-2164: reads of nonexistent resources now fail with JSON-RPC
-32602(Invalid params) instead of the MCP-specific-32002(ProtocolErrorCode.ResourceNotFound). The requested URI is carried in the error'sdata.uriso clients can still distinguish not-found from other invalid-params failures.Spec citation
The draft spec (
server/resources.mdx, ~L395-398) says:Note the history: the 2025-11-25 revision said servers SHOULD return
-32002; the draft (SEP-2164) flips this to MUST return-32602, keeping-32002only as a legacy code clients should tolerate.This reverses a recent deliberate change
#1389 (shipped in
2.0.0-alpha.1) intentionally moved unknown-resource reads from-32602→-32002and introducedProtocolErrorCode.ResourceNotFound, matching the then-current SHOULD-strength spec text. SEP-2164 has since standardized the opposite direction, so this PR flips it back:McpServer'sresources/readhandler throwsProtocolErrorCode.InvalidParamswithdata: { uri }.ProtocolErrorCode.ResourceNotFoundis marked@deprecatedbut remains exported — clients may still need to match it from older servers, per the spec's "SHOULD also accept legacy-32002" language.-32002.Open design question for maintainers
Implemented as an unconditional flip — should this instead be gated on the negotiated protocol version (runtime still defaults to 2025-11-25, where
-32002was SHOULD-strength)? Unconditional seems defensible since the draft tells clients to accept both, but flagging for a direction call. (The 2026-07-28 reference types from #2252 already define onlyINVALID_PARAMSfor this case.)Draft until that's resolved.
Changes
packages/server/src/server/mcp.ts:resources/readnot-found path throwsInvalidParamswithdata: { uri }packages/core/src/types/enums.ts:ResourceNotFounddeprecated with JSDoc pointing atInvalidParamsper SEP-2164test/integration/test/server/mcp.test.ts: unknown-resource test now asserts-32602+data.uritest/e2e/scenarios/resources.test.ts+test/e2e/requirements.ts:resources:read:unknown-uriupdated to-32602with a manifest note on the SEP-2164 SHOULD→MUST history@modelcontextprotocol/server+@modelcontextprotocol/core)Validation
pnpm build:all,pnpm typecheck:all,pnpm lint:all: passprotocol:timeout:max-total [sse](unrelated, fails only under full-suite parallel load)bearerAuth"expired 0 seconds ago") failed once under the full run, passes 3/3 in isolation — unrelatedDownstream impact: cloudflare/agents MCPAgent servers will emit -32602 for unknown resources after upgrading; grep confirms no agents code branches on -32002, so no downstream changes needed.
Closes #2194