From 6797729aed15c72e53ecf2fd74bc25b59d75d870 Mon Sep 17 00:00:00 2001 From: Marcelo Trylesinski Date: Thu, 22 Jan 2026 09:56:59 +0100 Subject: [PATCH] Bring back strict behavior on RequestId for stored streams --- src/mcp/shared/session.py | 23 +---- tests/shared/test_session.py | 175 +---------------------------------- 2 files changed, 2 insertions(+), 196 deletions(-) diff --git a/src/mcp/shared/session.py b/src/mcp/shared/session.py index d00fd764c..867f90807 100644 --- a/src/mcp/shared/session.py +++ b/src/mcp/shared/session.py @@ -430,26 +430,6 @@ async def _receive_loop(self) -> None: pass self._response_streams.clear() - def _normalize_request_id(self, response_id: RequestId) -> RequestId: - """Normalize a response ID to match how request IDs are stored. - - Since the client always sends integer IDs, we normalize string IDs - to integers when possible. This matches the TypeScript SDK approach: - https://github.com/modelcontextprotocol/typescript-sdk/blob/a606fb17909ea454e83aab14c73f14ea45c04448/src/shared/protocol.ts#L861 - - Args: - response_id: The response ID from the incoming message. - - Returns: - The normalized ID (int if possible, otherwise original value). - """ - if isinstance(response_id, str): - try: - return int(response_id) - except ValueError: - logging.warning(f"Response ID {response_id!r} cannot be normalized to match pending requests") - return response_id - async def _handle_response(self, message: SessionMessage) -> None: """Handle an incoming response or error message. @@ -463,8 +443,7 @@ async def _handle_response(self, message: SessionMessage) -> None: if not isinstance(message.message, JSONRPCResponse | JSONRPCError): return # pragma: no cover - # Normalize response ID to handle type mismatches (e.g., "0" vs 0) - response_id = self._normalize_request_id(message.message.id) + response_id = message.message.id # First, check response routers (e.g., TaskResultHandler) if isinstance(message.message, JSONRPCError): diff --git a/tests/shared/test_session.py b/tests/shared/test_session.py index 89fe18ebb..ca1885a14 100644 --- a/tests/shared/test_session.py +++ b/tests/shared/test_session.py @@ -9,17 +9,7 @@ from mcp.server.lowlevel.server import Server from mcp.shared.exceptions import McpError from mcp.shared.memory import create_client_server_memory_streams -from mcp.shared.message import SessionMessage -from mcp.types import ( - CancelledNotification, - CancelledNotificationParams, - EmptyResult, - ErrorData, - JSONRPCError, - JSONRPCRequest, - JSONRPCResponse, - TextContent, -) +from mcp.types import CancelledNotification, CancelledNotificationParams, EmptyResult, TextContent @pytest.mark.anyio @@ -102,169 +92,6 @@ async def make_request(client: Client): await ev_cancelled.wait() -@pytest.mark.anyio -async def test_response_id_type_mismatch_string_to_int(): - """Test that responses with string IDs are correctly matched to requests sent with - integer IDs. - - This handles the case where a server returns "id": "0" (string) but the client - sent "id": 0 (integer). Without ID type normalization, this would cause a timeout. - """ - ev_response_received = anyio.Event() - result_holder: list[types.EmptyResult] = [] - - 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(): - """Receive a request and respond with a string ID instead of integer.""" - message = await server_read.receive() - assert isinstance(message, SessionMessage) - root = message.message - assert isinstance(root, JSONRPCRequest) - # Get the original request ID (which is an integer) - request_id = root.id - assert isinstance(request_id, int), f"Expected int, got {type(request_id)}" - - # Respond with the ID as a string (simulating a buggy server) - response = JSONRPCResponse( - jsonrpc="2.0", - id=str(request_id), # Convert to string to simulate mismatch - result={}, - ) - await server_write.send(SessionMessage(message=response)) - - async def make_request(client_session: ClientSession): - nonlocal result_holder - # Send a ping request (uses integer ID internally) - 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) as client_session, - ): - tg.start_soon(mock_server) - tg.start_soon(make_request, client_session) - - # TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed. - with anyio.fail_after(2): # pragma: no cover - await ev_response_received.wait() - - assert len(result_holder) == 1 - assert isinstance(result_holder[0], EmptyResult) - - -@pytest.mark.anyio -async def test_error_response_id_type_mismatch_string_to_int(): - """Test that error responses with string IDs are correctly matched to requests - sent with integer IDs. - - This handles the case where a server returns an error with "id": "0" (string) - but the client sent "id": 0 (integer). - """ - ev_error_received = anyio.Event() - error_holder: list[McpError] = [] - - 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(): - """Receive a request and respond with an error using a string ID.""" - message = await server_read.receive() - assert isinstance(message, SessionMessage) - root = message.message - assert isinstance(root, JSONRPCRequest) - request_id = root.id - assert isinstance(request_id, int) - - # Respond with an error, using the ID as a string - error_response = JSONRPCError( - jsonrpc="2.0", - id=str(request_id), # Convert to string to simulate mismatch - error=ErrorData(code=-32600, message="Test error"), - ) - await server_write.send(SessionMessage(message=error_response)) - - async def make_request(client_session: ClientSession): - nonlocal error_holder - try: - await client_session.send_ping() - pytest.fail("Expected McpError to be raised") # pragma: no cover - except McpError as e: - error_holder.append(e) - ev_error_received.set() - - async with ( - anyio.create_task_group() as tg, - ClientSession(read_stream=client_read, write_stream=client_write) as client_session, - ): - tg.start_soon(mock_server) - tg.start_soon(make_request, client_session) - - # TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed. - with anyio.fail_after(2): # pragma: no cover - await ev_error_received.wait() - - assert len(error_holder) == 1 - assert "Test error" in str(error_holder[0]) - - -@pytest.mark.anyio -async def test_response_id_non_numeric_string_no_match(): - """Test that responses with non-numeric string IDs don't incorrectly match - integer request IDs. - - If a server returns "id": "abc" (non-numeric string), it should not match - a request sent with "id": 0 (integer). - """ - ev_timeout = anyio.Event() - - 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(): - """Receive a request and respond with a non-numeric string ID.""" - message = await server_read.receive() - assert isinstance(message, SessionMessage) - - # Respond with a non-numeric string ID (should not match) - response = JSONRPCResponse( - jsonrpc="2.0", - id="not_a_number", # Non-numeric string - result={}, - ) - await server_write.send(SessionMessage(message=response)) - - async def make_request(client_session: ClientSession): - try: - # Use a short timeout since we expect this to fail - await client_session.send_request( - types.PingRequest(), - types.EmptyResult, - request_read_timeout_seconds=0.5, - ) - pytest.fail("Expected timeout") # pragma: no cover - except McpError as e: - assert "Timed out" in str(e) - ev_timeout.set() - - async with ( - anyio.create_task_group() as tg, - ClientSession(read_stream=client_read, write_stream=client_write) as client_session, - ): - tg.start_soon(mock_server) - tg.start_soon(make_request, client_session) - - # TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed. - with anyio.fail_after(2): # pragma: no cover - await ev_timeout.wait() - - @pytest.mark.anyio async def test_connection_closed(): """Test that pending requests are cancelled when the connection is closed remotely."""