Skip to content

Commit 6797729

Browse files
committed
Bring back strict behavior on RequestId for stored streams
1 parent 656f887 commit 6797729

File tree

2 files changed

+2
-196
lines changed

2 files changed

+2
-196
lines changed

src/mcp/shared/session.py

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -430,26 +430,6 @@ async def _receive_loop(self) -> None:
430430
pass
431431
self._response_streams.clear()
432432

433-
def _normalize_request_id(self, response_id: RequestId) -> RequestId:
434-
"""Normalize a response ID to match how request IDs are stored.
435-
436-
Since the client always sends integer IDs, we normalize string IDs
437-
to integers when possible. This matches the TypeScript SDK approach:
438-
https://github.com/modelcontextprotocol/typescript-sdk/blob/a606fb17909ea454e83aab14c73f14ea45c04448/src/shared/protocol.ts#L861
439-
440-
Args:
441-
response_id: The response ID from the incoming message.
442-
443-
Returns:
444-
The normalized ID (int if possible, otherwise original value).
445-
"""
446-
if isinstance(response_id, str):
447-
try:
448-
return int(response_id)
449-
except ValueError:
450-
logging.warning(f"Response ID {response_id!r} cannot be normalized to match pending requests")
451-
return response_id
452-
453433
async def _handle_response(self, message: SessionMessage) -> None:
454434
"""Handle an incoming response or error message.
455435
@@ -463,8 +443,7 @@ async def _handle_response(self, message: SessionMessage) -> None:
463443
if not isinstance(message.message, JSONRPCResponse | JSONRPCError):
464444
return # pragma: no cover
465445

466-
# Normalize response ID to handle type mismatches (e.g., "0" vs 0)
467-
response_id = self._normalize_request_id(message.message.id)
446+
response_id = message.message.id
468447

469448
# First, check response routers (e.g., TaskResultHandler)
470449
if isinstance(message.message, JSONRPCError):

tests/shared/test_session.py

Lines changed: 1 addition & 174 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,7 @@
99
from mcp.server.lowlevel.server import Server
1010
from mcp.shared.exceptions import McpError
1111
from mcp.shared.memory import create_client_server_memory_streams
12-
from mcp.shared.message import SessionMessage
13-
from mcp.types import (
14-
CancelledNotification,
15-
CancelledNotificationParams,
16-
EmptyResult,
17-
ErrorData,
18-
JSONRPCError,
19-
JSONRPCRequest,
20-
JSONRPCResponse,
21-
TextContent,
22-
)
12+
from mcp.types import CancelledNotification, CancelledNotificationParams, EmptyResult, TextContent
2313

2414

2515
@pytest.mark.anyio
@@ -102,169 +92,6 @@ async def make_request(client: Client):
10292
await ev_cancelled.wait()
10393

10494

105-
@pytest.mark.anyio
106-
async def test_response_id_type_mismatch_string_to_int():
107-
"""Test that responses with string IDs are correctly matched to requests sent with
108-
integer IDs.
109-
110-
This handles the case where a server returns "id": "0" (string) but the client
111-
sent "id": 0 (integer). Without ID type normalization, this would cause a timeout.
112-
"""
113-
ev_response_received = anyio.Event()
114-
result_holder: list[types.EmptyResult] = []
115-
116-
async with create_client_server_memory_streams() as (client_streams, server_streams):
117-
client_read, client_write = client_streams
118-
server_read, server_write = server_streams
119-
120-
async def mock_server():
121-
"""Receive a request and respond with a string ID instead of integer."""
122-
message = await server_read.receive()
123-
assert isinstance(message, SessionMessage)
124-
root = message.message
125-
assert isinstance(root, JSONRPCRequest)
126-
# Get the original request ID (which is an integer)
127-
request_id = root.id
128-
assert isinstance(request_id, int), f"Expected int, got {type(request_id)}"
129-
130-
# Respond with the ID as a string (simulating a buggy server)
131-
response = JSONRPCResponse(
132-
jsonrpc="2.0",
133-
id=str(request_id), # Convert to string to simulate mismatch
134-
result={},
135-
)
136-
await server_write.send(SessionMessage(message=response))
137-
138-
async def make_request(client_session: ClientSession):
139-
nonlocal result_holder
140-
# Send a ping request (uses integer ID internally)
141-
result = await client_session.send_ping()
142-
result_holder.append(result)
143-
ev_response_received.set()
144-
145-
async with (
146-
anyio.create_task_group() as tg,
147-
ClientSession(read_stream=client_read, write_stream=client_write) as client_session,
148-
):
149-
tg.start_soon(mock_server)
150-
tg.start_soon(make_request, client_session)
151-
152-
# TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed.
153-
with anyio.fail_after(2): # pragma: no cover
154-
await ev_response_received.wait()
155-
156-
assert len(result_holder) == 1
157-
assert isinstance(result_holder[0], EmptyResult)
158-
159-
160-
@pytest.mark.anyio
161-
async def test_error_response_id_type_mismatch_string_to_int():
162-
"""Test that error responses with string IDs are correctly matched to requests
163-
sent with integer IDs.
164-
165-
This handles the case where a server returns an error with "id": "0" (string)
166-
but the client sent "id": 0 (integer).
167-
"""
168-
ev_error_received = anyio.Event()
169-
error_holder: list[McpError] = []
170-
171-
async with create_client_server_memory_streams() as (client_streams, server_streams):
172-
client_read, client_write = client_streams
173-
server_read, server_write = server_streams
174-
175-
async def mock_server():
176-
"""Receive a request and respond with an error using a string ID."""
177-
message = await server_read.receive()
178-
assert isinstance(message, SessionMessage)
179-
root = message.message
180-
assert isinstance(root, JSONRPCRequest)
181-
request_id = root.id
182-
assert isinstance(request_id, int)
183-
184-
# Respond with an error, using the ID as a string
185-
error_response = JSONRPCError(
186-
jsonrpc="2.0",
187-
id=str(request_id), # Convert to string to simulate mismatch
188-
error=ErrorData(code=-32600, message="Test error"),
189-
)
190-
await server_write.send(SessionMessage(message=error_response))
191-
192-
async def make_request(client_session: ClientSession):
193-
nonlocal error_holder
194-
try:
195-
await client_session.send_ping()
196-
pytest.fail("Expected McpError to be raised") # pragma: no cover
197-
except McpError as e:
198-
error_holder.append(e)
199-
ev_error_received.set()
200-
201-
async with (
202-
anyio.create_task_group() as tg,
203-
ClientSession(read_stream=client_read, write_stream=client_write) as client_session,
204-
):
205-
tg.start_soon(mock_server)
206-
tg.start_soon(make_request, client_session)
207-
208-
# TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed.
209-
with anyio.fail_after(2): # pragma: no cover
210-
await ev_error_received.wait()
211-
212-
assert len(error_holder) == 1
213-
assert "Test error" in str(error_holder[0])
214-
215-
216-
@pytest.mark.anyio
217-
async def test_response_id_non_numeric_string_no_match():
218-
"""Test that responses with non-numeric string IDs don't incorrectly match
219-
integer request IDs.
220-
221-
If a server returns "id": "abc" (non-numeric string), it should not match
222-
a request sent with "id": 0 (integer).
223-
"""
224-
ev_timeout = anyio.Event()
225-
226-
async with create_client_server_memory_streams() as (client_streams, server_streams):
227-
client_read, client_write = client_streams
228-
server_read, server_write = server_streams
229-
230-
async def mock_server():
231-
"""Receive a request and respond with a non-numeric string ID."""
232-
message = await server_read.receive()
233-
assert isinstance(message, SessionMessage)
234-
235-
# Respond with a non-numeric string ID (should not match)
236-
response = JSONRPCResponse(
237-
jsonrpc="2.0",
238-
id="not_a_number", # Non-numeric string
239-
result={},
240-
)
241-
await server_write.send(SessionMessage(message=response))
242-
243-
async def make_request(client_session: ClientSession):
244-
try:
245-
# Use a short timeout since we expect this to fail
246-
await client_session.send_request(
247-
types.PingRequest(),
248-
types.EmptyResult,
249-
request_read_timeout_seconds=0.5,
250-
)
251-
pytest.fail("Expected timeout") # pragma: no cover
252-
except McpError as e:
253-
assert "Timed out" in str(e)
254-
ev_timeout.set()
255-
256-
async with (
257-
anyio.create_task_group() as tg,
258-
ClientSession(read_stream=client_read, write_stream=client_write) as client_session,
259-
):
260-
tg.start_soon(mock_server)
261-
tg.start_soon(make_request, client_session)
262-
263-
# TODO(Marcelo): Drop the pragma once https://github.com/coveragepy/coveragepy/issues/1987 is fixed.
264-
with anyio.fail_after(2): # pragma: no cover
265-
await ev_timeout.wait()
266-
267-
26895
@pytest.mark.anyio
26996
async def test_connection_closed():
27097
"""Test that pending requests are cancelled when the connection is closed remotely."""

0 commit comments

Comments
 (0)