Skip to content

Commit 44574ad

Browse files
committed
fix: align JSONRPCDispatcher error shapes with existing server
Three pinned wire shapes the interaction suite locks in: - Peer-cancelled requests are answered with ErrorData(code=0, message="Request cancelled"). Spec says SHOULD NOT respond, but the existing server always has. - Unhandled handler exceptions become code=0 (not INTERNAL_ERROR), message=str(e). Matches Server._handle_request. - ValidationError becomes the fixed "Invalid request parameters" / data="" shape rather than leaking the pydantic error text. All three carry TODO(maxisbey) markers; they're compat with current behavior, not the intended end state.
1 parent 013d406 commit 44574ad

2 files changed

Lines changed: 32 additions & 20 deletions

File tree

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@
4343
from mcp.shared.transport_context import TransportContext
4444
from mcp.types import (
4545
CONNECTION_CLOSED,
46-
INTERNAL_ERROR,
4746
INVALID_PARAMS,
4847
REQUEST_CANCELLED,
4948
REQUEST_TIMEOUT,
@@ -524,11 +523,15 @@ async def _handle_request(
524523
# `dctx.send_raw_request()` should see `NoBackChannelError`.
525524
dctx.close()
526525
await self._write_result(req.id, result)
527-
# Peer-cancel: `_dispatch_notification` cancelled this scope. anyio
528-
# swallows a scope's *own* cancel at __exit__, so the result write
529-
# (or the handler) is interrupted and execution lands here without
530-
# reaching the `except cancelled` arm below. Spec SHOULD: send no
531-
# response - fall through to `finally`.
526+
if scope.cancel_called:
527+
# Peer-cancel: `_dispatch_notification` cancelled this scope.
528+
# anyio swallows a scope's *own* cancel at __exit__, so the
529+
# result write (or the handler) is interrupted and execution
530+
# lands here rather than the `except cancelled` arm below.
531+
# TODO(maxisbey): spec says SHOULD NOT respond after cancel.
532+
# The existing server always has and the interaction suite pins
533+
# that; revisit once the suite's divergence entry is resolved.
534+
await self._write_error(req.id, ErrorData(code=0, message="Request cancelled"))
532535
except anyio.get_cancelled_exc_class():
533536
# Outer-cancel: run()'s task group is shutting down. Any bare
534537
# `await` here re-raises immediately, so shield the courtesy write.
@@ -537,11 +540,20 @@ async def _handle_request(
537540
raise
538541
except MCPError as e:
539542
await self._write_error(req.id, e.error)
540-
except ValidationError as e:
541-
await self._write_error(req.id, ErrorData(code=INVALID_PARAMS, message=str(e)))
543+
except ValidationError:
544+
# TODO(maxisbey): data="" is pinned compat with the existing
545+
# server (which never leaked pydantic error text onto the wire).
546+
# Consider putting the validation detail in `data` once the
547+
# interaction suite's divergence entry is resolved.
548+
await self._write_error(
549+
req.id, ErrorData(code=INVALID_PARAMS, message="Invalid request parameters", data="")
550+
)
542551
except Exception as e:
543552
logger.exception("handler for %r raised", req.method)
544-
await self._write_error(req.id, ErrorData(code=INTERNAL_ERROR, message=str(e)))
553+
# TODO(maxisbey): code=0 is pinned compat with the existing
554+
# server's `_handle_request`. JSON-RPC says INTERNAL_ERROR
555+
# (-32603); revisit once the suite's divergence entry is resolved.
556+
await self._write_error(req.id, ErrorData(code=0, message=str(e)))
545557
if self._raise_handler_exceptions:
546558
raise
547559
finally:

tests/shared/test_jsonrpc_dispatcher.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -70,22 +70,23 @@ async def call(method: str) -> None:
7070

7171

7272
@pytest.mark.anyio
73-
async def test_handler_raising_exception_sends_internal_error_with_str_message():
74-
"""Per design: INTERNAL_ERROR carries str(e), not a scrubbed message."""
73+
async def test_handler_raising_exception_sends_code_zero_with_str_message():
74+
"""Matches the existing server's `_handle_request`: code=0, message=str(e)."""
7575

7676
async def server_on_request(ctx: DCtx, method: str, params: Mapping[str, Any] | None) -> dict[str, Any]:
7777
raise RuntimeError("kaboom")
7878

7979
async with running_pair(jsonrpc_pair, server_on_request=server_on_request) as (client, *_):
8080
with anyio.fail_after(5), pytest.raises(MCPError) as exc:
8181
await client.send_raw_request("tools/list", None)
82-
assert exc.value.error.code == INTERNAL_ERROR
82+
assert exc.value.error.code == 0
8383
assert exc.value.error.message == "kaboom"
8484
assert exc.value.__cause__ is None # cause does not survive the wire
8585

8686

8787
@pytest.mark.anyio
88-
async def test_peer_cancel_interrupt_mode_sets_cancel_requested_and_sends_no_response():
88+
async def test_peer_cancel_interrupt_mode_writes_cancelled_error_response():
89+
"""Matches the existing server: a peer-cancelled request is answered with code=0."""
8990
handler_started = anyio.Event()
9091
handler_exited = anyio.Event()
9192
seen_ctx: list[DCtx] = []
@@ -99,23 +100,22 @@ async def server_on_request(ctx: DCtx, method: str, params: Mapping[str, Any] |
99100
handler_exited.set()
100101
raise NotImplementedError
101102

103+
seen_error: list[ErrorData] = []
102104
async with running_pair(jsonrpc_pair, server_on_request=server_on_request) as (client, *_):
103105
with anyio.fail_after(5):
104106
async with anyio.create_task_group() as tg: # pragma: no branch
105107

106108
async def call_then_record() -> None:
107-
with pytest.raises(MCPError): # we'll cancel via tg below
109+
with pytest.raises(MCPError) as exc:
108110
await client.send_raw_request("slow", None)
111+
seen_error.append(exc.value.error)
109112

110113
tg.start_soon(call_then_record)
111114
await handler_started.wait()
112-
# cancel just the handler (peer-cancel), not our caller
113115
await client.notify("notifications/cancelled", {"requestId": 1})
114116
await handler_exited.wait()
115-
# Handler torn down, no response was written; caller is still parked.
116-
# Cancel the caller's task to end the test.
117-
tg.cancel_scope.cancel()
118117
assert seen_ctx[0].cancel_requested.is_set()
118+
assert seen_error == [ErrorData(code=0, message="Request cancelled")]
119119

120120

121121
@pytest.mark.anyio
@@ -266,7 +266,7 @@ async def on_notify(ctx: DCtx, method: str, params: Mapping[str, Any] | None) ->
266266
sent = s2c_recv.receive_nowait()
267267
assert isinstance(sent, SessionMessage)
268268
assert isinstance(sent.message, JSONRPCError)
269-
assert sent.message.error.code == INTERNAL_ERROR
269+
assert sent.message.error.code == 0
270270
finally:
271271
for s in (c2s_send, c2s_recv, s2c_send, s2c_recv):
272272
s.close()
@@ -408,7 +408,7 @@ async def server_on_request(ctx: DCtx, method: str, params: Mapping[str, Any] |
408408
async with running_pair(jsonrpc_pair, server_on_request=server_on_request) as (client, *_):
409409
with anyio.fail_after(5), pytest.raises(MCPError) as exc:
410410
await client.send_raw_request("t", None)
411-
assert exc.value.error.code == INVALID_PARAMS
411+
assert exc.value.error == ErrorData(code=INVALID_PARAMS, message="Invalid request parameters", data="")
412412

413413

414414
@pytest.mark.anyio

0 commit comments

Comments
 (0)