[Schema][Server] Preserve request id in JSON-RPC error responses instead of fabricating id:"" (#379)
* 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 * 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. * Document intentional id recovery guard in MessageFactory * Cover id: 0 and string ids in recoverable-id preservation test * Fixed client handling of null-id error responses
This commit is contained in:
parent
5041caf77a
commit
1961414432
7 changed files with 130 additions and 2 deletions
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -16,4 +16,17 @@ namespace Mcp\Exception;
|
|||
*/
|
||||
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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -148,6 +148,11 @@ final class MessageFactory
|
|||
|
||||
$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']);
|
||||
}
|
||||
$messages[] = $e;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ use Mcp\Schema\Enum\ProtocolVersion;
|
|||
*
|
||||
* @phpstan-type ErrorData array{
|
||||
* jsonrpc: string,
|
||||
* id: string|int,
|
||||
* id: string|int|null,
|
||||
* code: int,
|
||||
* message: string,
|
||||
* data?: mixed,
|
||||
|
|
|
|||
|
|
@ -228,7 +228,7 @@ class Protocol
|
|||
{
|
||||
$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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -112,6 +112,20 @@ final class ProtocolTest extends TestCase
|
|||
$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(
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ use Mcp\Server\Session\SessionInterface;
|
|||
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;
|
||||
|
|
@ -323,6 +324,40 @@ final class ProtocolTest extends TestCase
|
|||
);
|
||||
}
|
||||
|
||||
#[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 +565,66 @@ final class ProtocolTest extends TestCase
|
|||
$this->assertSame([], $protocol->consumeOutgoingMessages($sessionId));
|
||||
}
|
||||
|
||||
/**
|
||||
* @return iterable<string, array{string, string|int}>
|
||||
*/
|
||||
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(string $input, string|int $expectedId): 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 id but missing method/result/error: the message is
|
||||
// structurally invalid, yet its id IS recoverable from the decoded payload.
|
||||
$protocol->processInput(
|
||||
$this->transport,
|
||||
$input,
|
||||
$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($expectedId, $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
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in a new issue