fix: allow null id in JSONRPCError per JSON-RPC 2.0 spec (#2056)
This commit is contained in:
parent
3b53fb9a00
commit
705497a593
15 changed files with 151 additions and 24 deletions
|
|
@ -138,7 +138,7 @@ async def sse_client(
|
||||||
json=session_message.message.model_dump(
|
json=session_message.message.model_dump(
|
||||||
by_alias=True,
|
by_alias=True,
|
||||||
mode="json",
|
mode="json",
|
||||||
exclude_none=True,
|
exclude_unset=True,
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
response.raise_for_status()
|
response.raise_for_status()
|
||||||
|
|
|
||||||
|
|
@ -167,7 +167,7 @@ async def stdio_client(server: StdioServerParameters, errlog: TextIO = sys.stder
|
||||||
try:
|
try:
|
||||||
async with write_stream_reader:
|
async with write_stream_reader:
|
||||||
async for session_message in write_stream_reader:
|
async for session_message in write_stream_reader:
|
||||||
json = session_message.message.model_dump_json(by_alias=True, exclude_none=True)
|
json = session_message.message.model_dump_json(by_alias=True, exclude_unset=True)
|
||||||
await process.stdin.send(
|
await process.stdin.send(
|
||||||
(json + "\n").encode(
|
(json + "\n").encode(
|
||||||
encoding=server.encoding,
|
encoding=server.encoding,
|
||||||
|
|
|
||||||
|
|
@ -260,7 +260,7 @@ class StreamableHTTPTransport:
|
||||||
async with ctx.client.stream(
|
async with ctx.client.stream(
|
||||||
"POST",
|
"POST",
|
||||||
self.url,
|
self.url,
|
||||||
json=message.model_dump(by_alias=True, mode="json", exclude_none=True),
|
json=message.model_dump(by_alias=True, mode="json", exclude_unset=True),
|
||||||
headers=headers,
|
headers=headers,
|
||||||
) as response:
|
) as response:
|
||||||
if response.status_code == 202:
|
if response.status_code == 202:
|
||||||
|
|
|
||||||
|
|
@ -65,7 +65,7 @@ async def websocket_client(
|
||||||
async with write_stream_reader:
|
async with write_stream_reader:
|
||||||
async for session_message in write_stream_reader:
|
async for session_message in write_stream_reader:
|
||||||
# Convert to a dict, then to JSON
|
# Convert to a dict, then to JSON
|
||||||
msg_dict = session_message.message.model_dump(by_alias=True, mode="json", exclude_none=True)
|
msg_dict = session_message.message.model_dump(by_alias=True, mode="json", exclude_unset=True)
|
||||||
await ws.send(json.dumps(msg_dict))
|
await ws.send(json.dumps(msg_dict))
|
||||||
|
|
||||||
async with anyio.create_task_group() as tg:
|
async with anyio.create_task_group() as tg:
|
||||||
|
|
|
||||||
|
|
@ -490,7 +490,7 @@ class Server(Generic[LifespanResultT]):
|
||||||
except Exception as err:
|
except Exception as err:
|
||||||
if raise_exceptions: # pragma: no cover
|
if raise_exceptions: # pragma: no cover
|
||||||
raise err
|
raise err
|
||||||
response = types.ErrorData(code=0, message=str(err), data=None)
|
response = types.ErrorData(code=0, message=str(err))
|
||||||
|
|
||||||
await message.respond(response)
|
await message.respond(response)
|
||||||
else: # pragma: no cover
|
else: # pragma: no cover
|
||||||
|
|
|
||||||
|
|
@ -170,7 +170,7 @@ class SseServerTransport:
|
||||||
await sse_stream_writer.send(
|
await sse_stream_writer.send(
|
||||||
{
|
{
|
||||||
"event": "message",
|
"event": "message",
|
||||||
"data": session_message.message.model_dump_json(by_alias=True, exclude_none=True),
|
"data": session_message.message.model_dump_json(by_alias=True, exclude_unset=True),
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -71,7 +71,7 @@ async def stdio_server(stdin: anyio.AsyncFile[str] | None = None, stdout: anyio.
|
||||||
try:
|
try:
|
||||||
async with write_stream_reader:
|
async with write_stream_reader:
|
||||||
async for session_message in write_stream_reader:
|
async for session_message in write_stream_reader:
|
||||||
json = session_message.message.model_dump_json(by_alias=True, exclude_none=True)
|
json = session_message.message.model_dump_json(by_alias=True, exclude_unset=True)
|
||||||
await stdout.write(json + "\n")
|
await stdout.write(json + "\n")
|
||||||
await stdout.flush()
|
await stdout.flush()
|
||||||
except anyio.ClosedResourceError: # pragma: no cover
|
except anyio.ClosedResourceError: # pragma: no cover
|
||||||
|
|
|
||||||
|
|
@ -298,12 +298,12 @@ class StreamableHTTPServerTransport:
|
||||||
# Return a properly formatted JSON error response
|
# Return a properly formatted JSON error response
|
||||||
error_response = JSONRPCError(
|
error_response = JSONRPCError(
|
||||||
jsonrpc="2.0",
|
jsonrpc="2.0",
|
||||||
id="server-error", # We don't have a request ID for general errors
|
id=None,
|
||||||
error=ErrorData(code=error_code, message=error_message),
|
error=ErrorData(code=error_code, message=error_message),
|
||||||
)
|
)
|
||||||
|
|
||||||
return Response(
|
return Response(
|
||||||
error_response.model_dump_json(by_alias=True, exclude_none=True),
|
error_response.model_dump_json(by_alias=True, exclude_unset=True),
|
||||||
status_code=status_code,
|
status_code=status_code,
|
||||||
headers=response_headers,
|
headers=response_headers,
|
||||||
)
|
)
|
||||||
|
|
@ -323,7 +323,7 @@ class StreamableHTTPServerTransport:
|
||||||
response_headers[MCP_SESSION_ID_HEADER] = self.mcp_session_id
|
response_headers[MCP_SESSION_ID_HEADER] = self.mcp_session_id
|
||||||
|
|
||||||
return Response(
|
return Response(
|
||||||
response_message.model_dump_json(by_alias=True, exclude_none=True) if response_message else None,
|
response_message.model_dump_json(by_alias=True, exclude_unset=True) if response_message else None,
|
||||||
status_code=status_code,
|
status_code=status_code,
|
||||||
headers=response_headers,
|
headers=response_headers,
|
||||||
)
|
)
|
||||||
|
|
@ -336,7 +336,7 @@ class StreamableHTTPServerTransport:
|
||||||
"""Create event data dictionary from an EventMessage."""
|
"""Create event data dictionary from an EventMessage."""
|
||||||
event_data = {
|
event_data = {
|
||||||
"event": "message",
|
"event": "message",
|
||||||
"data": event_message.message.model_dump_json(by_alias=True, exclude_none=True),
|
"data": event_message.message.model_dump_json(by_alias=True, exclude_unset=True),
|
||||||
}
|
}
|
||||||
|
|
||||||
# If an event ID was provided, include it
|
# If an event ID was provided, include it
|
||||||
|
|
@ -975,12 +975,11 @@ class StreamableHTTPServerTransport:
|
||||||
# Determine which request stream(s) should receive this message
|
# Determine which request stream(s) should receive this message
|
||||||
message = session_message.message
|
message = session_message.message
|
||||||
target_request_id = None
|
target_request_id = None
|
||||||
# Check if this is a response
|
# Check if this is a response with a known request id.
|
||||||
if isinstance(message, JSONRPCResponse | JSONRPCError):
|
# Null-id errors (e.g., parse errors) fall through to
|
||||||
response_id = str(message.id)
|
# the GET stream since they can't be correlated.
|
||||||
# If this response is for an existing request stream,
|
if isinstance(message, JSONRPCResponse | JSONRPCError) and message.id is not None:
|
||||||
# send it there
|
target_request_id = str(message.id)
|
||||||
target_request_id = response_id
|
|
||||||
# Extract related_request_id from meta if it exists
|
# Extract related_request_id from meta if it exists
|
||||||
elif ( # pragma: no cover
|
elif ( # pragma: no cover
|
||||||
session_message.metadata is not None
|
session_message.metadata is not None
|
||||||
|
|
|
||||||
|
|
@ -244,11 +244,11 @@ class StreamableHTTPSessionManager:
|
||||||
# See: https://github.com/modelcontextprotocol/python-sdk/issues/1821
|
# See: https://github.com/modelcontextprotocol/python-sdk/issues/1821
|
||||||
error_response = JSONRPCError(
|
error_response = JSONRPCError(
|
||||||
jsonrpc="2.0",
|
jsonrpc="2.0",
|
||||||
id="server-error",
|
id=None,
|
||||||
error=ErrorData(code=INVALID_REQUEST, message="Session not found"),
|
error=ErrorData(code=INVALID_REQUEST, message="Session not found"),
|
||||||
)
|
)
|
||||||
response = Response(
|
response = Response(
|
||||||
content=error_response.model_dump_json(by_alias=True, exclude_none=True),
|
content=error_response.model_dump_json(by_alias=True, exclude_unset=True),
|
||||||
status_code=HTTPStatus.NOT_FOUND,
|
status_code=HTTPStatus.NOT_FOUND,
|
||||||
media_type="application/json",
|
media_type="application/json",
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -47,7 +47,7 @@ async def websocket_server(scope: Scope, receive: Receive, send: Send):
|
||||||
try:
|
try:
|
||||||
async with write_stream_reader:
|
async with write_stream_reader:
|
||||||
async for session_message in write_stream_reader:
|
async for session_message in write_stream_reader:
|
||||||
obj = session_message.message.model_dump_json(by_alias=True, exclude_none=True)
|
obj = session_message.message.model_dump_json(by_alias=True, exclude_unset=True)
|
||||||
await websocket.send_text(obj)
|
await websocket.send_text(obj)
|
||||||
except anyio.ClosedResourceError:
|
except anyio.ClosedResourceError:
|
||||||
await websocket.close()
|
await websocket.close()
|
||||||
|
|
|
||||||
|
|
@ -12,7 +12,10 @@ class MCPError(Exception):
|
||||||
|
|
||||||
def __init__(self, code: int, message: str, data: Any = None):
|
def __init__(self, code: int, message: str, data: Any = None):
|
||||||
super().__init__(code, message, data)
|
super().__init__(code, message, data)
|
||||||
self.error = ErrorData(code=code, message=message, data=data)
|
if data is not None:
|
||||||
|
self.error = ErrorData(code=code, message=message, data=data)
|
||||||
|
else:
|
||||||
|
self.error = ErrorData(code=code, message=message)
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def code(self) -> int:
|
def code(self) -> int:
|
||||||
|
|
|
||||||
|
|
@ -142,7 +142,7 @@ class RequestResponder(Generic[ReceiveRequestT, SendResultT]):
|
||||||
# Send an error response to indicate cancellation
|
# Send an error response to indicate cancellation
|
||||||
await self._session._send_response( # type: ignore[reportPrivateUsage]
|
await self._session._send_response( # type: ignore[reportPrivateUsage]
|
||||||
request_id=self.request_id,
|
request_id=self.request_id,
|
||||||
response=ErrorData(code=0, message="Request cancelled", data=None),
|
response=ErrorData(code=0, message="Request cancelled"),
|
||||||
)
|
)
|
||||||
|
|
||||||
@property
|
@property
|
||||||
|
|
@ -458,6 +458,12 @@ class BaseSession(
|
||||||
if not isinstance(message.message, JSONRPCResponse | JSONRPCError):
|
if not isinstance(message.message, JSONRPCResponse | JSONRPCError):
|
||||||
return # pragma: no cover
|
return # pragma: no cover
|
||||||
|
|
||||||
|
if message.message.id is None:
|
||||||
|
# Narrows to JSONRPCError since JSONRPCResponse.id is always RequestId
|
||||||
|
error = message.message.error
|
||||||
|
logging.warning(f"Received error with null ID: {error.message}")
|
||||||
|
await self._handle_incoming(MCPError(error.code, error.message, error.data))
|
||||||
|
return
|
||||||
# Normalize response ID to handle type mismatches (e.g., "0" vs 0)
|
# Normalize response ID to handle type mismatches (e.g., "0" vs 0)
|
||||||
response_id = self._normalize_request_id(message.message.id)
|
response_id = self._normalize_request_id(message.message.id)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -75,7 +75,7 @@ class JSONRPCError(BaseModel):
|
||||||
"""A response to a request that indicates an error occurred."""
|
"""A response to a request that indicates an error occurred."""
|
||||||
|
|
||||||
jsonrpc: Literal["2.0"]
|
jsonrpc: Literal["2.0"]
|
||||||
id: RequestId
|
id: RequestId | None
|
||||||
error: ErrorData
|
error: ErrorData
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -312,7 +312,7 @@ async def test_unknown_session_id_returns_404():
|
||||||
# Verify JSON-RPC error format
|
# Verify JSON-RPC error format
|
||||||
error_data = json.loads(response_body)
|
error_data = json.loads(response_body)
|
||||||
assert error_data["jsonrpc"] == "2.0"
|
assert error_data["jsonrpc"] == "2.0"
|
||||||
assert error_data["id"] == "server-error"
|
assert error_data["id"] is None
|
||||||
assert error_data["error"]["code"] == INVALID_REQUEST
|
assert error_data["error"]["code"] == INVALID_REQUEST
|
||||||
assert error_data["error"]["message"] == "Session not found"
|
assert error_data["error"]["message"] == "Session not found"
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -7,14 +7,19 @@ from mcp.server import Server, ServerRequestContext
|
||||||
from mcp.shared.exceptions import MCPError
|
from mcp.shared.exceptions import MCPError
|
||||||
from mcp.shared.memory import create_client_server_memory_streams
|
from mcp.shared.memory import create_client_server_memory_streams
|
||||||
from mcp.shared.message import SessionMessage
|
from mcp.shared.message import SessionMessage
|
||||||
|
from mcp.shared.session import RequestResponder
|
||||||
from mcp.types import (
|
from mcp.types import (
|
||||||
|
PARSE_ERROR,
|
||||||
CancelledNotification,
|
CancelledNotification,
|
||||||
CancelledNotificationParams,
|
CancelledNotificationParams,
|
||||||
|
ClientResult,
|
||||||
EmptyResult,
|
EmptyResult,
|
||||||
ErrorData,
|
ErrorData,
|
||||||
JSONRPCError,
|
JSONRPCError,
|
||||||
JSONRPCRequest,
|
JSONRPCRequest,
|
||||||
JSONRPCResponse,
|
JSONRPCResponse,
|
||||||
|
ServerNotification,
|
||||||
|
ServerRequest,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -297,3 +302,117 @@ async def test_connection_closed():
|
||||||
await ev_closed.wait()
|
await ev_closed.wait()
|
||||||
with anyio.fail_after(1): # pragma: no branch
|
with anyio.fail_after(1): # pragma: no branch
|
||||||
await ev_response.wait()
|
await ev_response.wait()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_null_id_error_surfaced_via_message_handler():
|
||||||
|
"""Test that a JSONRPCError with id=None is surfaced to the message handler.
|
||||||
|
|
||||||
|
Per JSON-RPC 2.0, error responses use id=null when the request id could not
|
||||||
|
be determined (e.g., parse errors). These cannot be correlated to any pending
|
||||||
|
request, so they are forwarded to the message handler as MCPError.
|
||||||
|
"""
|
||||||
|
ev_error_received = anyio.Event()
|
||||||
|
error_holder: list[MCPError] = []
|
||||||
|
|
||||||
|
async def capture_errors(
|
||||||
|
message: RequestResponder[ServerRequest, ClientResult] | ServerNotification | Exception,
|
||||||
|
) -> None:
|
||||||
|
assert isinstance(message, MCPError)
|
||||||
|
error_holder.append(message)
|
||||||
|
ev_error_received.set()
|
||||||
|
|
||||||
|
sent_error = ErrorData(code=PARSE_ERROR, message="Parse error")
|
||||||
|
|
||||||
|
async with create_client_server_memory_streams() as (client_streams, server_streams):
|
||||||
|
client_read, client_write = client_streams
|
||||||
|
_server_read, server_write = server_streams
|
||||||
|
|
||||||
|
async def mock_server():
|
||||||
|
"""Send a null-id error (simulating a parse error)."""
|
||||||
|
error_response = JSONRPCError(jsonrpc="2.0", id=None, error=sent_error)
|
||||||
|
await server_write.send(SessionMessage(message=error_response))
|
||||||
|
|
||||||
|
async with (
|
||||||
|
anyio.create_task_group() as tg,
|
||||||
|
ClientSession(
|
||||||
|
read_stream=client_read,
|
||||||
|
write_stream=client_write,
|
||||||
|
message_handler=capture_errors,
|
||||||
|
) as _client_session,
|
||||||
|
):
|
||||||
|
tg.start_soon(mock_server)
|
||||||
|
|
||||||
|
with anyio.fail_after(2): # pragma: no branch
|
||||||
|
await ev_error_received.wait()
|
||||||
|
|
||||||
|
assert len(error_holder) == 1
|
||||||
|
assert error_holder[0].error == sent_error
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_null_id_error_does_not_affect_pending_request():
|
||||||
|
"""Test that a null-id error doesn't interfere with an in-flight request.
|
||||||
|
|
||||||
|
When a null-id error arrives while a request is pending, the error should
|
||||||
|
go to the message handler and the pending request should still complete
|
||||||
|
normally with its own response.
|
||||||
|
"""
|
||||||
|
ev_error_received = anyio.Event()
|
||||||
|
ev_response_received = anyio.Event()
|
||||||
|
error_holder: list[MCPError] = []
|
||||||
|
result_holder: list[EmptyResult] = []
|
||||||
|
|
||||||
|
async def capture_errors(
|
||||||
|
message: RequestResponder[ServerRequest, ClientResult] | ServerNotification | Exception,
|
||||||
|
) -> None:
|
||||||
|
assert isinstance(message, MCPError)
|
||||||
|
error_holder.append(message)
|
||||||
|
ev_error_received.set()
|
||||||
|
|
||||||
|
sent_error = ErrorData(code=PARSE_ERROR, message="Parse error")
|
||||||
|
|
||||||
|
async with create_client_server_memory_streams() as (client_streams, server_streams):
|
||||||
|
client_read, client_write = client_streams
|
||||||
|
server_read, server_write = server_streams
|
||||||
|
|
||||||
|
async def mock_server():
|
||||||
|
"""Read a request, inject a null-id error, then respond normally."""
|
||||||
|
message = await server_read.receive()
|
||||||
|
assert isinstance(message, SessionMessage)
|
||||||
|
assert isinstance(message.message, JSONRPCRequest)
|
||||||
|
request_id = message.message.id
|
||||||
|
|
||||||
|
# First, send a null-id error (should go to message handler)
|
||||||
|
await server_write.send(SessionMessage(message=JSONRPCError(jsonrpc="2.0", id=None, error=sent_error)))
|
||||||
|
|
||||||
|
# Then, respond normally to the pending request
|
||||||
|
await server_write.send(SessionMessage(message=JSONRPCResponse(jsonrpc="2.0", id=request_id, result={})))
|
||||||
|
|
||||||
|
async def make_request(client_session: ClientSession):
|
||||||
|
result = await client_session.send_ping()
|
||||||
|
result_holder.append(result)
|
||||||
|
ev_response_received.set()
|
||||||
|
|
||||||
|
async with (
|
||||||
|
anyio.create_task_group() as tg,
|
||||||
|
ClientSession(
|
||||||
|
read_stream=client_read,
|
||||||
|
write_stream=client_write,
|
||||||
|
message_handler=capture_errors,
|
||||||
|
) as client_session,
|
||||||
|
):
|
||||||
|
tg.start_soon(mock_server)
|
||||||
|
tg.start_soon(make_request, client_session)
|
||||||
|
|
||||||
|
with anyio.fail_after(2): # pragma: no branch
|
||||||
|
await ev_error_received.wait()
|
||||||
|
await ev_response_received.wait()
|
||||||
|
|
||||||
|
# Null-id error reached the message handler
|
||||||
|
assert len(error_holder) == 1
|
||||||
|
assert error_holder[0].error == sent_error
|
||||||
|
|
||||||
|
# Pending request completed successfully
|
||||||
|
assert len(result_holder) == 1
|
||||||
|
assert isinstance(result_holder[0], EmptyResult)
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue