[Schema] Omit the id of an error response that never had one (#442)
* [Schema] Omit the id of an error response that never had one An error response whose id could not be read sent `"id": ""`, which claims the peer issued a request with an empty-string id - a different statement from "the id could not be read". JSON-RPC spells the latter as a null id, and a receiver that cannot read the id is exactly the case the code covers. [BC Break] Error::$id accepts null and getId() may return it. All the for*() factories default to null, fromArray() accepts a missing or explicitly-null id, and the member is omitted from the serialized form when absent. * Guard null response id in Client/Server Protocol handlers Also fix Error::jsonSerialize()'s phpdoc: data belongs inside error, not beside it.
This commit is contained in:
Christopher Hertel
GitHub
parent
cfe5e8f570
commit
5041caf77a
7 changed files with 103 additions and 30 deletions
|
|
@ -5,6 +5,7 @@ All notable changes to `mcp/sdk` will be documented in this file.
|
|||
0.8.0
|
||||
-----
|
||||
|
||||
* [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.
|
||||
* [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.
|
||||
|
|
|
|||
|
|
@ -281,6 +281,12 @@ class Protocol
|
|||
{
|
||||
$requestId = $response->getId();
|
||||
|
||||
if (null === $requestId) {
|
||||
$this->logger->warning('Received an id-less error response; cannot correlate it to a request.', ['response' => $response->jsonSerialize()]);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
$this->logger->debug('Handling response', ['id' => $requestId]);
|
||||
|
||||
$this->state->storeResponse($requestId, $response->jsonSerialize());
|
||||
|
|
|
|||
|
|
@ -57,12 +57,15 @@ class Error implements MessageInterface
|
|||
public const UNSUPPORTED_PROTOCOL_VERSION = -32022;
|
||||
|
||||
/**
|
||||
* @param string|int|null $id The id of the request this answers. `null` only when it could not be
|
||||
* read — a malformed body, or a notification that was refused — in which
|
||||
* case the member is omitted rather than sent as an id nobody issued.
|
||||
* @param int $code the error type that occurred
|
||||
* @param string $message a short description of the error
|
||||
* @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,
|
||||
|
|
@ -77,10 +80,9 @@ class Error implements MessageInterface
|
|||
if (!isset($data['jsonrpc']) || MessageInterface::JSONRPC_VERSION !== $data['jsonrpc']) {
|
||||
throw new InvalidArgumentException('Invalid or missing "jsonrpc" in Error data.');
|
||||
}
|
||||
if (!isset($data['id'])) {
|
||||
throw new InvalidArgumentException('Invalid or missing "id" in Error data.');
|
||||
}
|
||||
if (!\is_string($data['id']) && !\is_int($data['id'])) {
|
||||
// An error response carrying no id is well-formed: it is what a
|
||||
// receiver sends when the id could not be read off the request.
|
||||
if (isset($data['id']) && !\is_string($data['id']) && !\is_int($data['id'])) {
|
||||
throw new InvalidArgumentException('Invalid "id" type in Error data.');
|
||||
}
|
||||
if (!isset($data['error']) || !\is_array($data['error'])) {
|
||||
|
|
@ -93,45 +95,45 @@ class Error implements MessageInterface
|
|||
throw new InvalidArgumentException('Invalid or missing "message" in Error data.');
|
||||
}
|
||||
|
||||
return new self($data['id'], $data['error']['code'], $data['error']['message'], $data['error']['data'] ?? null);
|
||||
return new self($data['id'] ?? null, $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);
|
||||
}
|
||||
|
||||
final public static function forMethodNotFound(string $message, string|int $id = ''): self
|
||||
final public static function forMethodNotFound(string $message, string|int|null $id = null): self
|
||||
{
|
||||
return new self($id, self::METHOD_NOT_FOUND, $message);
|
||||
}
|
||||
|
||||
final public static function forInvalidParams(string $message, string|int $id = '', mixed $data = null): self
|
||||
final public static function forInvalidParams(string $message, string|int|null $id = null, mixed $data = null): self
|
||||
{
|
||||
return new self($id, self::INVALID_PARAMS, $message, $data);
|
||||
}
|
||||
|
||||
final public static function forInternalError(string $message, string|int $id = ''): self
|
||||
final public static function forInternalError(string $message, string|int|null $id = null): self
|
||||
{
|
||||
return new self($id, self::INTERNAL_ERROR, $message);
|
||||
}
|
||||
|
||||
final public static function forServerError(string $message, string|int $id = ''): self
|
||||
final public static function forServerError(string $message, string|int|null $id = null): self
|
||||
{
|
||||
return new self($id, self::SERVER_ERROR, $message);
|
||||
}
|
||||
|
||||
final public static function forResourceNotFound(string $message, string|int $id = ''): self
|
||||
final public static function forResourceNotFound(string $message, string|int|null $id = null): self
|
||||
{
|
||||
return new self($id, self::RESOURCE_NOT_FOUND, $message);
|
||||
}
|
||||
|
||||
final public static function forHeaderMismatch(string $message, string|int $id = ''): self
|
||||
final public static function forHeaderMismatch(string $message, string|int|null $id = null): self
|
||||
{
|
||||
return new self($id, self::HEADER_MISMATCH, $message);
|
||||
}
|
||||
|
|
@ -142,7 +144,7 @@ class Error implements MessageInterface
|
|||
final public static function forMissingRequiredClientCapability(
|
||||
string $message,
|
||||
ClientCapabilities $requiredCapabilities,
|
||||
string|int $id = '',
|
||||
string|int|null $id = null,
|
||||
): self {
|
||||
return new self($id, self::MISSING_REQUIRED_CLIENT_CAPABILITY, $message, [
|
||||
'requiredCapabilities' => $requiredCapabilities,
|
||||
|
|
@ -159,7 +161,7 @@ class Error implements MessageInterface
|
|||
final public static function forUnsupportedProtocolVersion(
|
||||
string $requested,
|
||||
array $supported,
|
||||
string|int $id = '',
|
||||
string|int|null $id = null,
|
||||
): self {
|
||||
return new self($id, self::UNSUPPORTED_PROTOCOL_VERSION, 'Unsupported protocol version', [
|
||||
'requested' => $requested,
|
||||
|
|
@ -167,7 +169,7 @@ class Error implements MessageInterface
|
|||
]);
|
||||
}
|
||||
|
||||
public function getId(): string|int
|
||||
public function getId(): string|int|null
|
||||
{
|
||||
return $this->id;
|
||||
}
|
||||
|
|
@ -175,12 +177,12 @@ class Error implements MessageInterface
|
|||
/**
|
||||
* @return array{
|
||||
* jsonrpc: string,
|
||||
* id: string|int,
|
||||
* id?: string|int,
|
||||
* error: array{
|
||||
* code: int,
|
||||
* message: string,
|
||||
* },
|
||||
* data?: mixed,
|
||||
* },
|
||||
* }
|
||||
*/
|
||||
public function jsonSerialize(): array
|
||||
|
|
@ -194,10 +196,17 @@ class Error implements MessageInterface
|
|||
$error['data'] = $this->data;
|
||||
}
|
||||
|
||||
return [
|
||||
'jsonrpc' => MessageInterface::JSONRPC_VERSION,
|
||||
'id' => $this->id,
|
||||
'error' => $error,
|
||||
];
|
||||
$data = ['jsonrpc' => MessageInterface::JSONRPC_VERSION];
|
||||
|
||||
// Omitted, not empty: `"id": ""` claims the sender issued a request
|
||||
// with an empty-string id, which is a different statement from "the
|
||||
// id could not be read".
|
||||
if (null !== $this->id) {
|
||||
$data['id'] = $this->id;
|
||||
}
|
||||
|
||||
$data['error'] = $error;
|
||||
|
||||
return $data;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -341,6 +341,12 @@ class Protocol
|
|||
|
||||
$messageId = $response->getId();
|
||||
|
||||
if (null === $messageId) {
|
||||
$this->logger->warning('Received an id-less error response from client; cannot correlate it to a pending request.', ['response' => $response->jsonSerialize()]);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
$session->set(self::SESSION_RESPONSES.".{$messageId}", $response->jsonSerialize());
|
||||
$session->forget(self::SESSION_ACTIVE_REQUEST_META);
|
||||
|
||||
|
|
|
|||
|
|
@ -99,6 +99,19 @@ final class ProtocolTest extends TestCase
|
|||
yield 'modern revision' => [ProtocolVersion::V2026_07_28->value];
|
||||
}
|
||||
|
||||
#[TestDox('logs and ignores an id-less error response instead of crashing on it')]
|
||||
public function testIgnoresIdLessErrorResponse(): void
|
||||
{
|
||||
$protocol = new Protocol(logger: $logger = new CollectingLogger());
|
||||
|
||||
$protocol->processMessage(json_encode([
|
||||
'jsonrpc' => MessageInterface::JSONRPC_VERSION,
|
||||
'error' => ['code' => -32700, 'message' => 'Parse error'],
|
||||
], \JSON_THROW_ON_ERROR));
|
||||
|
||||
$this->assertCount(1, $logger->warnings);
|
||||
}
|
||||
|
||||
private function createConfiguration(ProtocolVersion $protocolVersion): Configuration
|
||||
{
|
||||
return new Configuration(
|
||||
|
|
|
|||
|
|
@ -278,13 +278,15 @@ final class MessageFactoryTest extends TestCase
|
|||
|
||||
public function testErrorMissingId(): void
|
||||
{
|
||||
// Well-formed: an error response leaves the member out when the id
|
||||
// could not be read off the request it answers.
|
||||
$json = '{"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid"}}';
|
||||
|
||||
$results = $this->factory->create($json);
|
||||
|
||||
$this->assertCount(1, $results);
|
||||
$this->assertInstanceOf(InvalidInputMessageException::class, $results[0]);
|
||||
$this->assertStringContainsString('id', $results[0]->getMessage());
|
||||
$this->assertInstanceOf(Error::class, $results[0]);
|
||||
$this->assertNull($results[0]->getId());
|
||||
}
|
||||
|
||||
public function testErrorMissingCode(): void
|
||||
|
|
@ -365,9 +367,21 @@ final class MessageFactoryTest extends TestCase
|
|||
$this->assertStringContainsString('id', $results[0]->getMessage());
|
||||
}
|
||||
|
||||
public function testErrorWithNullId(): void
|
||||
{
|
||||
// JSON-RPC 2.0 spells the same thing as an explicit null.
|
||||
$json = '{"jsonrpc": "2.0", "id": null, "error": {"code": -32600, "message": "Invalid"}}';
|
||||
|
||||
$results = $this->factory->create($json);
|
||||
|
||||
$this->assertCount(1, $results);
|
||||
$this->assertInstanceOf(Error::class, $results[0]);
|
||||
$this->assertNull($results[0]->getId());
|
||||
}
|
||||
|
||||
public function testErrorWithInvalidIdType(): void
|
||||
{
|
||||
$json = '{"jsonrpc": "2.0", "id": null, "error": {"code": -32600, "message": "Invalid"}}';
|
||||
$json = '{"jsonrpc": "2.0", "id": {"not": "an id"}, "error": {"code": -32600, "message": "Invalid"}}';
|
||||
|
||||
$results = $this->factory->create($json);
|
||||
|
||||
|
|
|
|||
|
|
@ -82,6 +82,30 @@ final class ProtocolTest extends TestCase
|
|||
);
|
||||
}
|
||||
|
||||
#[TestDox('An id-less error response from the client is logged and ignored, not stored under a collapsed session key')]
|
||||
public function testIdLessErrorResponseIsIgnored(): void
|
||||
{
|
||||
$session = $this->createMock(SessionInterface::class);
|
||||
$session->expects($this->never())->method('set');
|
||||
|
||||
$this->sessionManager->method('exists')->willReturn(true);
|
||||
$this->sessionManager->method('createWithId')->willReturn($session);
|
||||
|
||||
$protocol = new Protocol(
|
||||
requestHandlers: [],
|
||||
notificationHandlers: [],
|
||||
messageFactory: MessageFactory::make(),
|
||||
sessionManager: $this->sessionManager,
|
||||
);
|
||||
|
||||
$sessionId = Uuid::v4();
|
||||
$protocol->processInput(
|
||||
$this->transport,
|
||||
'{"jsonrpc": "2.0", "error": {"code": -32700, "message": "Parse error"}}',
|
||||
$sessionId
|
||||
);
|
||||
}
|
||||
|
||||
#[TestDox('A single request is handled only by the first matching handler')]
|
||||
public function testRequestHandledByFirstMatchingHandler(): void
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in a new issue