Repository navigation
fix(model): read a request id written in float form as that integer - #1346
connor4312 merged 2 commits into
Conversation
github-actions
Bot
added
T-test
|
FYI, #1287 merged earlier today and seems to overlap with this. It checks which keys a message has before picking a variant, so I ran this PR's tests on current |
A JSON-RPC id of 1.0 or 1e2 is an integer written the way some encoders write every number. It used to be rejected, so the request got no response. NumberOrString now accepts a float that an f64 represents exactly as an integer (up to 2^53) and reads it as that integer. modelcontextprotocol#1287 already makes a message with an id and a method parse only as a request, so the earlier change to JsonRpcNotification is dropped; the tests for an unusable id stay as regression tests. Refs modelcontextprotocol#1345 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
66db2cb to
7022d1f
Compare
talhayme
changed the title
|
Thanks, you're right. I rebased on main: #1287 covers the unusable-id case, so I dropped the AI disclosure: rebased and drafted with Claude Code; I ran the tests and reviewed it. |
| /// `f64` represents exactly. | ||
| fn integral_f64_to_i64(f: f64) -> Option<i64> { | ||
| const MAX_EXACT: f64 = 9_007_199_254_740_992.0; // 2^53 | ||
| (f.fract() == 0.0 && f.abs() <= MAX_EXACT).then_some(f as i64) |
There was a problem hiding this comment.
Small one: I think <= here lets 2^53 + 1 slip through. 9007199254740993.0 rounds to 2^53 when it's parsed, so it passes the check and comes out as 9007199254740992. I tried it on this branch and that's what I got.
Probably won't come up much, since a client sending float ids likely can't hold that value anyway. But < seems safer, since 2^53 as an f64 could be either number:
| (f.fract() == 0.0 && f.abs() <= MAX_EXACT).then_some(f as i64) | |
| (f.fract() == 0.0 && f.abs() < MAX_EXACT).then_some(f as i64) |
Might be worth adding "9007199254740993.0" to request_with_unusable_id_is_rejected too.
There was a problem hiding this comment.
Good catch, thanks. Confirmed on the branch: 9007199254740993.0 came out as 9007199254740992. Switched to < and added that id to request_with_unusable_id_is_rejected (300f4b1).
AI disclosure: fixed and tested with Claude Code; I reviewed it.
…er was sent 2^53 + 1 written in float form parses to the same f64 as 2^53, so accepting 2^53 silently turned 9007199254740993.0 into 9007199254740992. The exact-integer range is now strictly below 2^53. Added that id to request_with_unusable_id_is_rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
connor4312
left a comment
connor4312
left a comment
There was a problem hiding this comment.
lgtm, thank you for the contribution!
3 participants
Fixes #1345. #1287 already makes a message with
idandmethodparse only as a request, which fixes the unusable-id case. This PR now covers the one part that still fails on main.Change:
NumberOrStringreads an integral number written in float form (1.0,1e2) as that integer, up to 2^53. Some JSON encoders write every number that way; such a request used to be rejected and got no response.Tests:
integral_id_in_float_form_is_that_integerfails on main and passes with the change. The unusable-id tests intests/test_deserialization.rsandasync_rw.rspass on main too; I kept them as regression tests for #1287's behaviour.No public API change.
cargo testforrmcpandcargo +nightly fmtpass.AI disclosure: written with Claude Code; I reviewed the change and ran the checks above.
🤖 Generated with Claude Code