Skip to content

fix(model): reject a request whose id is not an integer or a string - #1346

Draft
talhayme wants to merge 1 commit into
modelcontextprotocol:mainfrom
talhayme:fix/1345-request-with-invalid-id
Draft

talhayme wants to merge 1 commit into
modelcontextprotocol:mainfrom
talhayme:fix/1345-request-with-invalid-id

Conversation

@talhayme

@talhayme talhayme commented Oct 12, 2026 •

Copy link
Copy Markdown

Opened as a draft only because creating a regular PR was refused for my account (CreatePullRequest permission error). The change is complete and ready for review; please mark it ready if that suits you.

Closes #1345.

JsonRpcMessage is untagged, so a message with id and method whose id did not parse as RequestId fell through to Notification, which ignored the extra field. The server never answered and the client waited for its own timeout.

Change

  • JsonRpcNotification no longer deserializes a message that carries an id. The message then matches no variant, and the transports already answer that with -32600 Invalid Request.
  • NumberOrString accepts an integral number written in float form (1.0, 1e2) up to 2^53.

Left as is: "id": null with a method still parses as a notification. Some peers spell "no id" that way, and I didn't want to break them in this PR. Say if you'd rather reject it too.

Tests: message parsing in tests/test_deserialization.rs and the transport reply in async_rw.rs. Three of them fail without the change. I also ran the cases in a separate crate built without serde_json/arbitrary_precision, since the SDK's own tests enable it; same results.

No public API change. cargo test --all-features, the non-local feature set, clippy and cargo +nightly fmt pass.

AI disclosure: written with Claude Code; I reviewed the change and ran the checks above.

🤖 Generated with Claude Code

`JsonRpcMessage` is untagged, so a message with `id` and `method` whose `id`
did not parse as `RequestId` fell through to `Notification`, which ignored
the extra field. The server never answered and the client waited for its
own timeout.

`JsonRpcNotification` no longer deserializes a message that carries an `id`,
so such a message matches no variant and the transports answer it with
-32600 Invalid Request. `NumberOrString` also accepts an integral number
written in float form (`1.0`, `1e2`) up to 2^53.

`"id": null` with a `method` still parses as a notification.

Closes modelcontextprotocol#1345

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-transport Transport layer changes labels Oct 12, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A request whose id is not an i64 or a string is parsed as a notification

1 participant