diff --git a/CHANGELOG.md b/CHANGELOG.md index 8db034bd..17e99a5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * [BC Break] `ResourceDefinition::__construct()` signature changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. * [BC Break] `ResourceTemplate::__construct()` signature changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. * [BC Break] `McpResource` and `McpResourceTemplate` attribute signatures changed — `$title` parameter added between `$name` and `$description`. Callers using positional arguments must switch to named arguments. +* Fix JSON-RPC error responses fabricating an empty-string `id`. Unrecoverable parse errors (`-32700`) now return `id: null` per JSON-RPC 2.0, and invalid-but-parseable requests (`-32600`) preserve the original request `id`. `Error::$id` / `Error::getId()` and the `$id` parameter of `Error::forParseError()` / `Error::forInvalidRequest()` are widened to `string|int|null`; `InvalidInputMessageException` now carries the recoverable request id via `getRequestId()`/`setRequestId()`. 0.5.0 ----- 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 1bb82db8..1fdb9d14 100644 --- a/src/JsonRpc/MessageFactory.php +++ b/src/JsonRpc/MessageFactory.php @@ -111,6 +111,9 @@ public function create(string $input): array try { $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/Schema/JsonRpc/Error.php b/src/Schema/JsonRpc/Error.php index 532e406d..4bb71290 100644 --- a/src/Schema/JsonRpc/Error.php +++ b/src/Schema/JsonRpc/Error.php @@ -42,7 +42,7 @@ class Error implements MessageInterface * @param mixed|null $data additional information about the error */ public function __construct( - public readonly string|int $id, + public readonly string|int|null $id, public readonly int $code, public readonly string $message, public readonly mixed $data = null, @@ -76,12 +76,12 @@ final public static function fromArray(array $data): self return new self($data['id'], $data['error']['code'], $data['error']['message'], $data['error']['data'] ?? null); } - final public static function forParseError(string $message, string|int $id = ''): self + final public static function forParseError(string $message, string|int|null $id = null): self { return new self($id, self::PARSE_ERROR, $message); } - final public static function forInvalidRequest(string $message, string|int $id = ''): self + final public static function forInvalidRequest(string $message, string|int|null $id = null): self { return new self($id, self::INVALID_REQUEST, $message); } @@ -111,7 +111,7 @@ final public static function forResourceNotFound(string $message, string|int $id return new self($id, self::RESOURCE_NOT_FOUND, $message); } - public function getId(): string|int + public function getId(): string|int|null { return $this->id; } @@ -119,7 +119,7 @@ public function getId(): string|int /** * @return array{ * jsonrpc: string, - * id: string|int, + * id: string|int|null, * error: array{ * code: int, * message: string, diff --git a/src/Server/Protocol.php b/src/Server/Protocol.php index d9af4e4c..9d9f1655 100644 --- a/src/Server/Protocol.php +++ b/src/Server/Protocol.php @@ -151,7 +151,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 f1d1c834..a3d615bf 100644 --- a/tests/Unit/Server/ProtocolTest.php +++ b/tests/Unit/Server/ProtocolTest.php @@ -297,6 +297,45 @@ 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: the response + // must be null or omit the id, never a fabricated empty string. + $this->assertNotSame('', $decoded['id'] ?? null, 'Parse error must not fabricate an empty-string id'); + // isset() is false for both an absent key and a null value, so this asserts the id is + // null or omitted (never a fabricated empty string). + $this->assertFalse( + isset($decoded['id']), + 'Unrecoverable parse error id should be null or omitted', + ); + } + #[TestDox('Invalid message structure returns error')] public function testInvalidMessageStructureReturnsError(): void { @@ -349,6 +388,55 @@ public function testInvalidMessageStructureReturnsError(): void $this->assertEquals(Error::INVALID_REQUEST, $message['error']['code']); } + #[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 {