Preserve the Request ID in Invalid Request Error Responses#400
Open
koic wants to merge 1 commit into
Open
Conversation
b2032c3 to
580016f
Compare
## Motivation and Context Resolves modelcontextprotocol#398. `JsonRpcHandler` returned `"id": null` for JSON-RPC envelope errors (wrong or missing `jsonrpc` version, non-string `method`) even when the incoming payload carried a concrete, valid request id. From the client's perspective the original request stayed pending forever, since no response with the matching id ever arrived: ``` {"jsonrpc":"1.0","id":3,"method":"ping","params":{}} => {"jsonrpc":"2.0","id":null,"error":{"code":-32600,...}} ``` Per JSON-RPC 2.0 (Response object, `id`), the response id MUST be the same as the request's id; null is reserved for requests whose id could not be detected (e.g. Parse error). The envelope validation in `process_request` runs after JSON parsing with the request hash in hand, so the id is detectable - it was simply discarded by passing the `:unknown_id` sentinel unconditionally. The fix passes the request's id to `error_response` whenever the request carries one. The existing guards keep every other case intact: `error_response` already nils out ids that fail validation (so a type-invalid or pattern-violating id still yields `"id": null`, preserving the XSS-prevention policy on echoed ids), and the `:unknown_id` sentinel is kept for requests without an id so an Invalid Request error still produces a null-id response, matching the spec's own example. Parse errors and non-object requests are unchanged (null is correct there), and batch entries pick up the fix automatically since each entry goes through `process_request`. For reference, the TypeScript and Python SDKs currently respond with `"id": null` in these cases as well, because both validate the whole message against a schema (zod / pydantic) before extracting the id. This change is driven by the JSON-RPC 2.0 requirement rather than parity; the Ruby transport layer already echoes the request id where recoverable (`invalid_request_response(request_id:)` in `StreamableHTTPTransport`), and this closes the remaining gap in `JsonRpcHandler`. ## How Has This Been Tested? - Reproduced the three payloads from modelcontextprotocol#398 before and after the fix; they now return `"id":3`, `"id":4`, and `"id":8` respectively - New and updated unit tests covering: id preserved for wrong version, missing `jsonrpc`, numeric `method`, and `rpc.`-prefixed `method`; id preserved for an invalid entry inside a batch; null id kept for requests without an id, with an explicit null id, and with an id that fails validation ## Breaking Changes None in the protocol sense - this aligns the error responses with JSON-RPC 2.0. Clients that matched `"id": null` on these envelope errors will now see the request's own id, which is what allows them to correlate the error with the pending request.
580016f to
18761cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Resolves #398.
JsonRpcHandlerreturned"id": nullfor JSON-RPC envelope errors (wrong or missingjsonrpcversion, non-stringmethod) even when the incoming payload carried a concrete, valid request id. From the client's perspective the original request stayed pending forever, since no response with the matching id ever arrived:Per JSON-RPC 2.0 (Response object,
id), the response id MUST be the same as the request's id; null is reserved for requests whose id could not be detected (e.g. Parse error). The envelope validation inprocess_requestruns after JSON parsing with the request hash in hand, so the id is detectable - it was simply discarded by passing the:unknown_idsentinel unconditionally.The fix passes the request's id to
error_responsewhenever the request carries one. The existing guards keep every other case intact:error_responsealready nils out ids that fail validation (so a type-invalid or pattern-violating id still yields"id": null, preserving the XSS-prevention policy on echoed ids), and the:unknown_idsentinel is kept for requests without an id so an Invalid Request error still produces a null-id response, matching the spec's own example. Parse errors and non-object requests are unchanged (null is correct there), and batch entries pick up the fix automatically since each entry goes throughprocess_request.For reference, the TypeScript and Python SDKs currently respond with
"id": nullin these cases as well, because both validate the whole message against a schema (zod / pydantic) before extracting the id. This change is driven by the JSON-RPC 2.0 requirement rather than parity; the Ruby transport layer already echoes the request id where recoverable (invalid_request_response(request_id:)inStreamableHTTPTransport), and this closes the remaining gap inJsonRpcHandler.How Has This Been Tested?
id:nulleven when the request id is present #398 before and after the fix; they now return"id":3,"id":4, and"id":8respectivelyjsonrpc, numericmethod, andrpc.-prefixedmethod; id preserved for an invalid entry inside a batch; null id kept for requests without an id, with an explicit null id, and with an id that fails validationBreaking Changes
None in the protocol sense - this aligns the error responses with JSON-RPC 2.0. Clients that matched
"id": nullon these envelope errors will now see the request's own id, which is what allows them to correlate the error with the pending request.Types of changes
Checklist