fix(a2a): accept the whole JSON-RPC id union the spec defines - #37704
Conversation
|
|
Greptile SummaryThe response model now accepts the complete JSON-RPC ID union while preventing boolean IDs from being interpreted as integers
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains; both previously reported boolean-ID paths are fixed at the normalization and direct-validation boundaries
|
| Filename | Overview |
|---|---|
| litellm/types/agents.py | Widens A2A response IDs to string, strict integer, or null and safely stringifies boolean IDs during normalization |
| tests/test_litellm/a2a_protocol/test_send_message_response.py | Adds focused regression tests covering integer, zero, null, boolean, echoed, backfilled, and direct-construction ID behavior |
Reviews (4): Last reviewed commit: "fix(a2a): accept the whole JSON-RPC id u..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
64debe2 to
77b78db
Compare
|
@greptileai please review the current head 77b78db. It now also fixes null ids (LIT-2818) plus your boolean finding; details in the description. |
77b78db to
e743498
Compare
|
@greptileai please review the current head e743498. Pure rebase onto staging for the new test-tree lint step; the code diff is unchanged. |
JSON-RPC 2.0 types `id` as string, integer or null, but
LiteLLMSendMessageResponse annotated it as a bare required `str`. Pydantic v2
dropped v1's int-to-str coercion, so an upstream agent echoing an integer id was
rejected outright, and a null id, which section 5 requires for an error that
cannot be correlated to a request, was rejected too. Both surfaced as -32603 with
a pydantic ValidationError in the message: five distinct 500s on
/a2a/{agent_id}, across message/send and tasks/get.
Everything around the model already handled the full union: the endpoint reads
the id off the body as Any, its helpers are typed `str | int | None`, the error
builder takes `object`, and the streaming path passes the id through untouched.
The response model was the only narrowing left.
Backfilling an id the agent omitted keeps the caller's type too, since JSON-RPC
requires the response id to equal the request id and a caller that sent 7 cannot
correlate a response carrying "7".
`bool` is excluded from the integer half even though it subclasses `int`, so a
boolean id is stringified rather than relayed as 1 or 0, where it would collide
with a real integer id another in-flight request may be using.
e743498 to
4affda4
Compare
|
@greptileai please review the current head 4affda4. The boolean P1 is fixed at the annotation via StrictInt, not just in normalization; my prior reply was wrong. |
tin-berri
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix (2 files) — widens the A2A response id field to the full JSON-RPC spec union (string | integer | null) instead of requiring str, which was 500ing on integer/null/zero ids despite the agent answering fine. Appreciate the transparency in the review notes about the bool-coercion trap (bool subclasses int, so a naive int widening would silently turn a boolean id into 1/0, colliding with a real in-flight integer id) — caught by Greptile, fixed with StrictInt so bool/float/list/dict are all rejected on every construction path, not just the normalization helper. Proof-of-fix covers all the JSON-RPC id shapes (string, int, 0, omitted/null, bool) against a real upstream stub with access-log confirmation the agent was reached on every case. CI green aside from the long-running benchmarks job. Already has Yucheng's approval too.
TLDR
Problem this solves:
string | integer | null; our A2A response model requiredstrmessage/sendandtasks/getHow it solves it:
idto the full spec unionbool, which subclassesint, sotruenever returns as1User Flow
Before: a developer whose A2A client numbers its JSON-RPC requests (
"id": 42) cannot call any agent through the proxy at all{"jsonrpc":"2.0","id":42,"method":"message/send","params":{...}}{"error":{"code":-32603,"message":"Internal error: 1 validation error for LiteLLMSendMessageResponse\nid\n Input should be a valid string ..."}}"id": "42"and it returns HTTP 200 with the agent's replytasks/getwith"id": 42fails the same way, as does"id": 0After: the same requests succeed, and the id comes back as the value and type they sent
"id": 42"id": 42as a numbertasks/getwith"id": 42and with"id": 0both return HTTP 200 with the id unchanged42rather than"42", so the client can still match it to its requestRelevant issues
Linear ticket
Resolves LIT-2818
Pre-Submission checklist
uv run pytest tests/test_litellm/<your_test_file>.py -vScreenshots / Proof of Fix
Live proxy against a stub upstream agent that echoes the caller's JSON-RPC id back verbatim and logs every inbound request. The two string-id cases are controls: they must return a real 200 with the agent's reply on both sides, or the failing cases mean nothing.
Shared setup, identical for both runs:
Every case below is the same curl with only the
idandmethodchanging:Before (fc3b160)
Code under test,
litellm/types/agents.py:334, isid: strmessage/send, string id (control)
{"jsonrpc":"2.0","id":"ctl-1","method":"message/send","params":{"message":{"role":"user","messageId":"m1","parts":[{"kind":"text","text":"hi"}]}}}message/send, integer id
{"jsonrpc":"2.0","id":42,"method":"message/send","params":{"message":{"role":"user","messageId":"m1","parts":[{"kind":"text","text":"hi"}]}}}tasks/get, string id (control)
{"jsonrpc":"2.0","id":"ctl-2","method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, integer id
{"jsonrpc":"2.0","id":42,"method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, falsy integer id
{"jsonrpc":"2.0","id":0,"method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, id omitted so the agent answers with a null id
{"jsonrpc":"2.0","method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, boolean id (not a legal JSON-RPC id)
{"jsonrpc":"2.0","id":true,"method":"tasks/get","params":{"id":"stub-task-1"}}upstream access log for the run
After (4affda4)
Code under test,
litellm/types/agents.py:347, isid: str | StrictInt | None = Nonemessage/send, string id (control)
{"jsonrpc":"2.0","id":"ctl-1","method":"message/send","params":{"message":{"role":"user","messageId":"m1","parts":[{"kind":"text","text":"hi"}]}}}message/send, integer id
{"jsonrpc":"2.0","id":42,"method":"message/send","params":{"message":{"role":"user","messageId":"m1","parts":[{"kind":"text","text":"hi"}]}}}tasks/get, string id (control)
{"jsonrpc":"2.0","id":"ctl-2","method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, integer id
{"jsonrpc":"2.0","id":42,"method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, falsy integer id
{"jsonrpc":"2.0","id":0,"method":"tasks/get","params":{"id":"stub-task-1"}}0is preserved rather than read as absenttasks/get, id omitted so the agent answers with a null id
{"jsonrpc":"2.0","method":"tasks/get","params":{"id":"stub-task-1"}}tasks/get, boolean id (not a legal JSON-RPC id)
{"jsonrpc":"2.0","id":true,"method":"tasks/get","params":{"id":"stub-task-1"}}1, which would collide with a real integer idupstream access log for the run
Review notes
Greptile filed the boolean trap twice and was right both times, including against my own reply.
boolsubclassesint, so widening to accept integers made pydantic coerce a boolean id to1or0, which is worse than the 500 it replaced because1collides with a real integer id another in-flight request may be using.My first correction guarded only
_normalize_a2a_jsonrpc_response, and I said in a review reply that the annotation was guarded too. That was wrong: direct construction bypasses normalization, and the follow-up P1 saying so is accurate. The field is nowstr | StrictInt | None, which accepts exactlystring | integer | nulland rejectsbool,float,listanddicton every path including direct construction. Only the integer half needs strictness, sinceboolsubclassesintand notstr.Getting there without raising a type ceiling took some measuring, which is worth recording. The ceilings in
basedpyright-code-budget.jsonare ratcheted to the exact current count, so any+1redslint.StrictStr | StrictIntcosts tworeportUnknownVariableType(basedpyright cannot resolve either pydantic symbol here) and afield_validatorcosts one of those plus areportUntypedFunctionDecoratoragainst a ceiling of 27.str | StrictIntcosts exactly one, paid for by givingresponse_dictinfrom_a2a_responsea real binding instead of an untyped rebind. Net per-rule delta against the merge base is zero on every rule.Rebased onto
fc3b160fb5to pick up #37671, which added theruff check --config ruff-tests.toml testsstep. That job checks out the PR head rather than the merge ref, so the step ran against a tree predating the config file it names, and no rerun could clear it. Both proof legs above were re-captured after the rebase.This supersedes #29196, which proposed the same
str | int | Nonewidening for LIT-2818 in May. That branch targetslitellm_oss_agent_shin_daily_branch, predates_normalize_a2a_jsonrpc_response, and would conflict.Type
🐛 Bug Fix
Caveats
"id": nullexclude_nonedump, unchanged hererequest_id: stracross six provider packagesFinal Attestation