[Schema] Omit the id of an error response that never had one - #442
Conversation
An error response whose id could not be read sent `"id": ""`, which claims the peer issued a request with an empty-string id - a different statement from "the id could not be read". JSON-RPC spells the latter as a null id, and a receiver that cannot read the id is exactly the case the code covers. [BC Break] Error::$id accepts null and getId() may return it. All the for*() factories default to null, fromArray() accepts a missing or explicitly-null id, and the member is omitted from the serialized form when absent.
There was a problem hiding this comment.
Pull request overview
This PR updates the JSON-RPC error schema so that an “unknown/unreadable request id” is represented as an absent/null id (instead of the fabricated empty string ""), and ensures the serialized error omits the id member when it’s unknown. This aligns error behavior with JSON-RPC conventions and the MCP spec’s allowance for id-less error responses when correlation is impossible.
Changes:
- Allow
Mcp\Schema\JsonRpc\Errorto carry anullid, accept missing/nullids infromArray(), and default allfor*()factories tonull. - Omit
idduringErrorserialization when the id is unknown. - Update
MessageFactoryTestexpectations for missing/nullerror ids and document the BC break inCHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Unit/JsonRpc/MessageFactoryTest.php | Updates parsing expectations so id-less / id:null error responses decode to Error with null id. |
| src/Schema/JsonRpc/Error.php | Changes the Error schema to support null ids and omits id from JSON output when unknown. |
| CHANGELOG.md | Documents the BC break and the behavioral changes around error ids. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public function getId(): string|int|null | ||
| { | ||
| return $this->id; | ||
| } |
There was a problem hiding this comment.
Fixed in e3ff482 — both Client and Server Protocol now guard the null-id case instead of passing it into storeResponse()/the session key.
| * jsonrpc: string, | ||
| * id: string|int, | ||
| * id?: string|int, | ||
| * error: array{ | ||
| * code: int, | ||
| * message: string, |
There was a problem hiding this comment.
Fixed in e3ff482 — moved data back inside error in the phpdoc shape.
Also fix Error::jsonSerialize()'s phpdoc: data belongs inside error, not beside it.
An error response whose id could not be read sent
"id": "", which claims the peer issued a request with an empty-string id — a different statement from "the id could not be read". JSON-RPC spells the latter as a null id, and a receiver that cannot read the id off a malformed request is exactly the case the code covers.[BC Break]
Mcp\Schema\JsonRpc\Erroracceptsnullas its$id, andgetId()may return it:for*()factories default tonullinstead of''fromArray()accepts a missing or explicitly-null idMessageFactorydecodes both as an id-less error rather than rejecting themidmember is omitted from the serialized form when absentCloses #333 and the id half of #381.