From 3c7677a58b869adbd7408c2e3683cc2d497c22c9 Mon Sep 17 00:00:00 2001 From: Valery Gutu Date: Sat, 6 Jun 2026 21:50:05 +0300 Subject: [PATCH 1/5] Preserve request id in JSON-RPC error responses instead of fabricating 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 #333 --- CHANGELOG.md | 1 + .../InvalidInputMessageException.php | 13 +++ src/JsonRpc/MessageFactory.php | 3 + src/Server/Protocol.php | 2 +- tests/Unit/Server/ProtocolTest.php | 83 +++++++++++++++++++ 5 files changed, 101 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c2b0584..fb7f8bdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to `mcp/sdk` will be documented in this file. ----- * [BC Break] `Mcp\Schema\JsonRpc\Error` accepts `null` as its `$id`, and `getId()` may return it. An error response whose id could not be read now omits the member instead of sending `"id": ""` — which claimed the peer had issued a request with an empty-string id. All the `for*()` factories default to `null`, `fromArray()` accepts a missing or explicitly-null id, and `MessageFactory` decodes both as an id-less error rather than rejecting them. +* Preserve the original request `id` on an invalid-but-parseable message (`-32600`) instead of answering it id-less: `InvalidInputMessageException` now carries the recoverable id via `getRequestId()`/`setRequestId()`, threaded from `MessageFactory` through to the error response. * [BC Break] Drop the SDK-only name pattern on `ResourceDefinition`/`ResourceTemplate` `$name` — the spec allows any string (its own examples use `main.rs` and `Project Files`). URI/URI-template validation is unchanged. * Add `ClientGateway::supportsExtension()`, `Client\Builder::enableExtension()`, and `ClientCapabilities::withExtensions()` so clients can negotiate and check protocol extensions (e.g. MCP Apps) the same way servers already do. [BC Break] `ServerExtensionInterface` is replaced by the side-agnostic `Mcp\Schema\Extension\ExtensionInterface`. * Deprecate Roots, Sampling and Logging per SEP-2577 (protocol revision `2026-07-28`, earliest removal `2027-07-28`). They keep working but using them now triggers a deprecation notice — migrate to tool arguments/resource URIs, a direct LLM provider API, and stderr/OpenTelemetry respectively. diff --git a/src/Exception/InvalidInputMessageException.php b/src/Exception/InvalidInputMessageException.php index 4ab485a9..0f961224 100644 --- a/src/Exception/InvalidInputMessageException.php +++ b/src/Exception/InvalidInputMessageException.php @@ -16,4 +16,17 @@ */ class InvalidInputMessageException extends \InvalidArgumentException implements ExceptionInterface { + private string|int|null $requestId = null; + + public function getRequestId(): string|int|null + { + return $this->requestId; + } + + public function setRequestId(string|int|null $requestId): self + { + $this->requestId = $requestId; + + return $this; + } } diff --git a/src/JsonRpc/MessageFactory.php b/src/JsonRpc/MessageFactory.php index d9a895ec..cfc13d10 100644 --- a/src/JsonRpc/MessageFactory.php +++ b/src/JsonRpc/MessageFactory.php @@ -148,6 +148,9 @@ public function create(string $input): array $messages[] = $this->createMessage($message); } catch (InvalidInputMessageException $e) { + if (\is_array($message) && isset($message['id']) && (\is_string($message['id']) || \is_int($message['id']))) { + $e->setRequestId($message['id']); + } $messages[] = $e; } } diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index f9f79c35..a6e9f1ab 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -228,7 +228,7 @@ private function handleInvalidMessage(TransportInterface $transport, InvalidInpu { $this->logger->warning('Failed to create message.', ['exception' => $exception]); - $error = Error::forInvalidRequest($exception->getMessage()); + $error = Error::forInvalidRequest($exception->getMessage(), $exception->getRequestId()); $this->sendResponse($transport, $error, $session); } diff --git a/tests/Unit/Server/ProtocolTest.php b/tests/Unit/Server/ProtocolTest.php index 4b64b4a5..2b25afba 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -323,6 +323,40 @@ public function testInvalidJsonReturnsParseError(): void ); } + #[TestDox('Unrecoverable parse error does not fabricate an empty-string id')] + public function testParseErrorDoesNotFabricateEmptyStringId(): void + { + $sentPayload = null; + $this->transport->expects($this->once()) + ->method('send') + ->willReturnCallback(static function ($data) use (&$sentPayload) { + $sentPayload = $data; + }); + + $protocol = new Protocol( + requestHandlers: [], + notificationHandlers: [], + messageFactory: MessageFactory::make(), + sessionManager: $this->sessionManager, + ); + + // Well-formed JSON nested past PHP's json_decode() depth limit (512), mirroring + // issue #333: json_decode() throws "Maximum stack depth exceeded" so the request + // carries a real numeric id (900512) that cannot be recovered once decoding fails. + $deeplyNested = str_repeat('[', 600).str_repeat(']', 600); + $input = '{"jsonrpc":"2.0","id":900512,"method":"initialize","params":'.$deeplyNested.'}'; + + $protocol->processInput($this->transport, $input, null); + + $this->assertNotNull($sentPayload); + $decoded = json_decode($sentPayload, true); + $this->assertSame(Error::PARSE_ERROR, $decoded['error']['code']); + // The original id is genuinely unrecoverable after a parse failure: it must never be + // fabricated as an empty string, and — per the MCP `RequestId` schema, which never + // allows `null` — the key must be omitted rather than sent as `id: null`. + $this->assertArrayNotHasKey('id', $decoded, 'Unrecoverable parse error must omit id, not fabricate one'); + } + #[TestDox('Invalid message structure returns error')] public function testInvalidMessageStructureReturnsError(): void { @@ -530,6 +564,55 @@ public function testFailingNotificationListenerDoesNotProduceResponse(): void $this->assertSame([], $protocol->consumeOutgoingMessages($sessionId)); } + #[TestDox('Invalid but parseable message preserves its recoverable id')] + public function testInvalidMessagePreservesRecoverableId(): void + { + $session = $this->createMock(SessionInterface::class); + + $this->sessionManager->method('createWithId')->willReturn($session); + $this->sessionManager->method('exists')->willReturn(true); + + // Configure session mock for queue operations (mirrors testInvalidMessageStructureReturnsError). + $queue = []; + $session->method('get')->willReturnCallback(static function ($key, $default = null) use (&$queue) { + if ('_mcp.outgoing_queue' === $key) { + return $queue; + } + + return $default; + }); + + $session->method('set')->willReturnCallback(static function ($key, $value) use (&$queue) { + if ('_mcp.outgoing_queue' === $key) { + $queue = $value; + } + }); + + $protocol = new Protocol( + requestHandlers: [], + notificationHandlers: [], + messageFactory: MessageFactory::make(), + sessionManager: $this->sessionManager, + ); + + $sessionId = Uuid::v4(); + // Valid JSON carrying a real numeric id but missing method/result/error: the message + // is structurally invalid, yet its id (42) IS recoverable from the decoded payload. + $protocol->processInput( + $this->transport, + '{"jsonrpc": "2.0", "id": 42, "params": {}}', + $sessionId + ); + + $outgoing = $protocol->consumeOutgoingMessages($sessionId); + $this->assertCount(1, $outgoing); + + $message = json_decode($outgoing[0]['message'], true); + $this->assertArrayHasKey('error', $message); + $this->assertEquals(Error::INVALID_REQUEST, $message['error']['code']); + $this->assertSame(42, $message['id'], 'Invalid-but-parseable message must preserve its recoverable id, not return ""'); + } + #[TestDox('Request without handler returns method not found error')] public function testRequestWithoutHandlerReturnsMethodNotFoundError(): void { From 656807e71d23fb89d3b63e8b067683816f9ad2ff Mon Sep 17 00:00:00 2001 From: Valery Gutu Date: Mon, 8 Jun 2026 14:40:50 +0300 Subject: [PATCH 2/5] Preserve nullable request id consistently across Error factories and fromArray - Widen the remaining for*() factories (forMethodNotFound, forInvalidParams, forInternalError, forServerError, forResourceNotFound) to string|int|null defaulting to null, so no error path can fabricate id:"" again. The only defaulting call site (ProtocolVersionMiddleware) now emits id:null instead of id:"" for a pre-parse rejection, which is the spec-correct value. - Accept a null id in Error::fromArray(): require the id key to be present (JSON-RPC responses must carry it) but allow its value to be null, so a spec-compliant id:null error response round-trips and incoming error responses with id:null are accepted instead of rejected. - Widen the @phpstan-type ErrorData id to string|int|null to match the ctor, getId(), and jsonSerialize() shape. --- src/Schema/JsonRpc/Error.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Schema/JsonRpc/Error.php b/src/Schema/JsonRpc/Error.php index 4f95f72b..7749fa8f 100644 --- a/src/Schema/JsonRpc/Error.php +++ b/src/Schema/JsonRpc/Error.php @@ -20,7 +20,7 @@ * * @phpstan-type ErrorData array{ * jsonrpc: string, - * id: string|int, + * id: string|int|null, * code: int, * message: string, * data?: mixed, From cc4577ec4a5fc652f8e07172f8629001d4d5a9bc Mon Sep 17 00:00:00 2001 From: Valery Gutu Date: Mon, 8 Jun 2026 16:00:50 +0300 Subject: [PATCH 3/5] Document intentional id recovery guard in MessageFactory --- src/JsonRpc/MessageFactory.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/JsonRpc/MessageFactory.php b/src/JsonRpc/MessageFactory.php index cfc13d10..806eea54 100644 --- a/src/JsonRpc/MessageFactory.php +++ b/src/JsonRpc/MessageFactory.php @@ -148,6 +148,8 @@ public function create(string $input): array $messages[] = $this->createMessage($message); } catch (InvalidInputMessageException $e) { + // Recover the id only when it's a valid JSON-RPC scalar; + // a null or malformed id is left at the exception's null default. if (\is_array($message) && isset($message['id']) && (\is_string($message['id']) || \is_int($message['id']))) { $e->setRequestId($message['id']); } From 3a8224b05a135ac98361db750775e11264da0290 Mon Sep 17 00:00:00 2001 From: Valery Gutu Date: Mon, 8 Jun 2026 16:43:51 +0300 Subject: [PATCH 4/5] Cover id: 0 and string ids in recoverable-id preservation test --- tests/Unit/Server/ProtocolTest.php | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/Unit/Server/ProtocolTest.php b/tests/Unit/Server/ProtocolTest.php index 2b25afba..997c39d0 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -29,6 +29,7 @@ use Mcp\Server\Session\SessionManagerInterface; use Mcp\Server\Transport\TransportInterface; use Mcp\Tests\Unit\Fixtures\ThrowingRequest; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\TestDox; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -564,8 +565,19 @@ public function testFailingNotificationListenerDoesNotProduceResponse(): void $this->assertSame([], $protocol->consumeOutgoingMessages($sessionId)); } + /** + * @return iterable + */ + public static function recoverableIdProvider(): iterable + { + yield 'positive int' => ['{"jsonrpc": "2.0", "id": 42, "params": {}}', 42]; + yield 'zero int (truthiness trap)' => ['{"jsonrpc": "2.0", "id": 0, "params": {}}', 0]; + yield 'string id' => ['{"jsonrpc": "2.0", "id": "req-1", "params": {}}', 'req-1']; + } + + #[DataProvider('recoverableIdProvider')] #[TestDox('Invalid but parseable message preserves its recoverable id')] - public function testInvalidMessagePreservesRecoverableId(): void + public function testInvalidMessagePreservesRecoverableId(string $input, string|int $expectedId): void { $session = $this->createMock(SessionInterface::class); @@ -596,11 +608,11 @@ public function testInvalidMessagePreservesRecoverableId(): void ); $sessionId = Uuid::v4(); - // Valid JSON carrying a real numeric id but missing method/result/error: the message - // is structurally invalid, yet its id (42) IS recoverable from the decoded payload. + // Valid JSON carrying a real id but missing method/result/error: the message is + // structurally invalid, yet its id IS recoverable from the decoded payload. $protocol->processInput( $this->transport, - '{"jsonrpc": "2.0", "id": 42, "params": {}}', + $input, $sessionId ); @@ -610,7 +622,7 @@ public function testInvalidMessagePreservesRecoverableId(): void $message = json_decode($outgoing[0]['message'], true); $this->assertArrayHasKey('error', $message); $this->assertEquals(Error::INVALID_REQUEST, $message['error']['code']); - $this->assertSame(42, $message['id'], 'Invalid-but-parseable message must preserve its recoverable id, not return ""'); + $this->assertSame($expectedId, $message['id'], 'Invalid-but-parseable message must preserve its recoverable id, not return ""'); } #[TestDox('Request without handler returns method not found error')] From a2d01f2555b4970646a226f3756aa430521198c2 Mon Sep 17 00:00:00 2001 From: Valeriu Gutu Date: Wed, 15 Jul 2026 13:56:28 +0300 Subject: [PATCH 5/5] Fixed client handling of null-id error responses --- tests/Unit/Client/ProtocolTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Unit/Client/ProtocolTest.php b/tests/Unit/Client/ProtocolTest.php index 722df9f9..4ac40fdb 100644 --- a/tests/Unit/Client/ProtocolTest.php +++ b/tests/Unit/Client/ProtocolTest.php @@ -112,6 +112,20 @@ public function testIgnoresIdLessErrorResponse(): void $this->assertCount(1, $logger->warnings); } + #[TestDox('stores an error response under its id so the pending request can be correlated')] + public function testErrorResponseWithIdIsStoredForItsPendingRequest(): void + { + $protocol = new Protocol(); + + $protocol->processMessage('{"jsonrpc": "2.0", "id": 7, "error": {"code": -32601, "message": "Method not found"}}'); + + $response = $protocol->getState()->consumeResponse(7); + + $this->assertInstanceOf(Error::class, $response); + $this->assertSame(7, $response->getId()); + $this->assertSame(Error::METHOD_NOT_FOUND, $response->code); + } + private function createConfiguration(ProtocolVersion $protocolVersion): Configuration { return new Configuration(