[Schema][Server] Preserve request id in JSON-RPC error responses instead of fabricating id:""#379
Open
valeriudev wants to merge 1 commit into
Open
Conversation
…g id:"" When the server received input it could not turn into a valid message, the error response carried a fabricated empty-string id that was never in the request, breaking JSON-RPC client correlation. The id default for the error factories was '' and that default reached the wire unchanged: unrecoverable parse errors emitted id:"", and for invalid- but-parseable messages MessageFactory discarded the decoded id so it also fell back to "". Now unrecoverable parse errors (-32700) return id: null per JSON-RPC 2.0, and invalid-but-parseable requests (-32600) preserve the original request id. Error::$id / getId() and the $id parameter of forParseError() / forInvalidRequest() are widened to string|int|null, and the recoverable id is threaded through InvalidInputMessageException. Fixes modelcontextprotocol#333
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.
The bug
When the server receives input it cannot turn into a valid message, the JSON-RPC error response carries a fabricated empty-string
idthat was never in the request:{"jsonrpc":"2.0","id":"","error":{"code":-32700,...}}The reporter sent an
initializerequest with a real numeric id (900512) nested past PHP'sjson_decode()depth limit; the server replied withid:"". An empty string is not a valid JSON-RPC / MCPRequestId, so this breaks client correlation.Root cause
Two paths both collapsed to
"":Error::forParseError()andError::forInvalidRequest()defaulted$idto'', andError::$idwas typedstring|int(nonull). On an unrecoverable parse failure (\JsonExceptioninProtocol::processInput()) the factory was called with no id, so""reached the wire.MessageFactorythrewInvalidInputMessageExceptionwithout carrying the decodedid, soProtocol::handleInvalidMessage()also fell back to""— even though the real id was recoverable from the decoded payload.The fix
-32700) now returnid: null, per JSON-RPC 2.0 (id isnullwhen it cannot be determined).-32600) now preserve the original requestid.Error::$id/Error::getId()and the$idparameter ofError::forParseError()/Error::forInvalidRequest()are widened tostring|int|null.jsonSerialize()is unchanged — it now emits"id":nullinstead of"id":"".MessageFactorythroughInvalidInputMessageExceptionvia newgetRequestId()/setRequestId(). The id is only attached when the decoded value is a validstring|int(mirroringRequest/Error::fromArrayvalidation);id:0is preserved (type-checked, not truthiness), whilenull/true/array/missing staynull.No
[BC Break]: the type is widened, not narrowed, and the only behavioral change is replacing an invalid fabricated id with a spec-compliant one.Tests
Two regression tests in
tests/Unit/Server/ProtocolTest.php:testParseErrorDoesNotFabricateEmptyStringId— feeds JSON nested 600 deep sojson_decode()throws, assertserror.code === -32700,id !== '', and thatidis null or omitted.testInvalidMessagePreservesRecoverableId— feeds valid JSON with id42but nomethod/result/error, assertserror.code === -32600andid === 42.Both were red for the right reason (got
"") before the fix and are green after. Full unit suite (766 tests), phpstan (level 6), and php-cs-fixer all pass.Fixes #333