-
Notifications
You must be signed in to change notification settings - Fork 143
[Schema][Server] Preserve request id in JSON-RPC error responses instead of fabricating id:"" #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
79e6632
40b4c6b
e78be29
262db1a
84fb7b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 🟡 🔵 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 nit: assertion intent and actual behavior don't quite line up. $this->assertArrayHasKey('id', $decoded);
$this->assertNull($decoded['id'], 'Unrecoverable parse error must emit id: null');That also catches a future regression where someone drops the key entirely (which would break clients that strictly validate the JSON-RPC shape). |
||
| } | ||
|
|
||
| #[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 ""'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 nit: PR description specifically calls out that |
||
| } | ||
|
|
||
| #[TestDox('Request without handler returns method not found error')] | ||
| public function testRequestWithoutHandlerReturnsMethodNotFoundError(): void | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ JSON-RPC 2.0 allows
id: nullin requests. Hereisset()drops it, so a null-id invalid request falls back to thenulldefault inforInvalidRequest()— same outcome, fine. Intentional? If so, worth a one-line comment so the next reader doesn't "fix" it by switching toarray_key_exists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional - isset + the type guard. We only recover valid
string|intids; a null or non-scalar id stays at the exception's null default.array_key_existswould converge to the same via the type guard. Leaving a comment in the code.