Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📚 Documentation preview
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc21a0e6f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not complete: | ||
| call.cancel() |
There was a problem hiding this comment.
When the caller cancels or times out an incomplete initialize or server/discover request, the client deliberately supplies cancel_on_abandon=False because those negotiation methods must not be cancelled, but this finally block unconditionally calls call.cancel(). The gRPC adapter therefore aborts server-side negotiation despite the option; the receive path needs to avoid propagating or issuing native cancellation when that flag is false.
Useful? React with 👍 / 👎.
| if len(payload) > MAX_PAYLOAD_SIZE: | ||
| raise ValueError("Payload exceeds the gRPC binding's size limit") |
There was a problem hiding this comment.
With default gRPC message limits, a JSON payload just below 4 MiB passes this check but becomes larger than 4 MiB once wrapped in CallRequest or CallEvent, so an otherwise accepted request/result fails on the wire with RESOURCE_EXHAUSTED. The bound should apply to the serialized protobuf message or reserve enough envelope overhead rather than allowing the JSON field itself to consume the entire gRPC limit.
Useful? React with 👍 / 👎.
| request = CallRequest( | ||
| method=method, | ||
| params_json=encode_json(params), | ||
| request_id_json=encode_json(request_id), |
There was a problem hiding this comment.
The dispatcher contract accepts any Mapping[str, Any], but passing a valid non-dict mapping such as MappingProxyType or ChainMap reaches json.dumps unchanged and raises TypeError before issuing the RPC. The JSON-RPC dispatcher already normalizes with dict(params); this adapter should do the same anywhere its Mapping-typed APIs encode parameters.
Useful? React with 👍 / 👎.
| uv run --frozen --no-sync --package mcp-transport-examples --group dev | ||
| pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none | ||
| --junitxml=transport-results.xml |
There was a problem hiding this comment.
This new package is outside the root coverage sources, and the dedicated job invokes plain pytest; installing coverage and setting fail_under = 100 in the child configuration has no effect unless coverage is actually run and reported. Consequently, untested adapter branches can pass CI despite the repository's 100% coverage requirement, so this step should execute coverage run followed by a failing coverage report.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 35 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="examples/transports/pyproject.toml">
<violation number="1" location="examples/transports/pyproject.toml:6">
P2: The adapter imports `anyio` from four runtime modules, but this package does not declare it directly. Add `anyio>=4.5` so the wheel does not rely on `mcp`'s transitive dependency.</violation>
</file>
<file name="examples/transports/mcp_transport_examples/_grpc_codec.py">
<violation number="1" location="examples/transports/mcp_transport_examples/_grpc_codec.py:15">
P2: The JSON-field limit allows a payload whose enclosing `CallRequest` or `CallEvent` exceeds gRPC's default 4 MiB receive limit. Validate the serialized protobuf envelope, or reserve its envelope overhead before accepting the JSON payload.</violation>
<violation number="2" location="examples/transports/mcp_transport_examples/_grpc_codec.py:31">
P1: On supported Python 3.10, `json.loads` uses an unbounded `int`; a valid 4 MiB decimal number can consume seconds synchronously and block the asyncio loop. `MAX_PAYLOAD_SIZE` therefore does not bound decode cost; add a digit-bounded `parse_int` or another linear-time integer parser.</violation>
</file>
<file name="examples/transports/mcp_transport_examples/grpc_response.py">
<violation number="1" location="examples/transports/mcp_transport_examples/grpc_response.py:51">
P2: When a progress callback or message handler blocks, these awaits stop `receive_response` from reading the terminal event, so a successful RPC can time out. Run notification callbacks as managed child tasks and join them during dispatcher shutdown instead of awaiting them on the response-reading task.</violation>
</file>
<file name="examples/transports/tests/test_grpc_lifecycle.py">
<violation number="1" location="examples/transports/tests/test_grpc_lifecycle.py:68">
P2: In the `timeout` cause the 0.5s read timeout is also the gRPC RPC deadline. `entered.wait()` runs before that deadline expires; if the server starts the handler slower than 0.5s (contended CI loop, coverage) the deadline fires first, the handler never sets `entered`, and the test hangs on `entered.wait()` until the outer `fail_after(5)` fails it. Widen the margin, e.g. use `read_timeout_seconds=1.0`, so handler startup cannot be preempted by the deadline being tested.</violation>
</file>
<file name="examples/transports/README.md">
<violation number="1" location="examples/transports/README.md:12">
P2: The final Pyright command runs in the root environment instead of the adapter environment, so it can fail on missing optional imports such as `grpc`. Run it with the same `UV_PROJECT_ENVIRONMENT` (or select the package) as the preceding commands.</violation>
</file>
<file name="examples/transports/mcp_transport_examples/grpc_client.py">
<violation number="1" location="examples/transports/mcp_transport_examples/grpc_client.py:96">
P2: Valid non-`dict` mappings such as `MappingProxyType` and `ChainMap` fail in `encode_json` before the RPC starts. Convert non-null `params` to `dict` before encoding.</violation>
<violation number="2" location="examples/transports/mcp_transport_examples/grpc_client.py:133">
P2: When `cancel_on_abandon` is false, this cleanup still cancels the native RPC. Check that option before calling `call.cancel()` so abandoned negotiation requests can continue server-side.</violation>
</file>
<file name=".github/workflows/shared.yml">
<violation number="1" location=".github/workflows/shared.yml:122">
P2: Run this job under coverage and fail on `coverage report`; plain `pytest` never executes coverage, so the child `fail_under = 100` setting is ignored.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| return number | ||
|
|
||
| try: | ||
| return json.loads(payload, parse_constant=reject_constant, parse_float=finite_float) |
There was a problem hiding this comment.
P1: On supported Python 3.10, json.loads uses an unbounded int; a valid 4 MiB decimal number can consume seconds synchronously and block the asyncio loop. MAX_PAYLOAD_SIZE therefore does not bound decode cost; add a digit-bounded parse_int or another linear-time integer parser.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/mcp_transport_examples/_grpc_codec.py, line 31:
<comment>On supported Python 3.10, `json.loads` uses an unbounded `int`; a valid 4 MiB decimal number can consume seconds synchronously and block the asyncio loop. `MAX_PAYLOAD_SIZE` therefore does not bound decode cost; add a digit-bounded `parse_int` or another linear-time integer parser.</comment>
<file context>
@@ -0,0 +1,46 @@
+ return number
+
+ try:
+ return json.loads(payload, parse_constant=reject_constant, parse_float=finite_float)
+ except RecursionError as exc:
+ raise ValueError("JSON payload is too deeply nested") from exc
</file context>
| dependencies = [ | ||
| "grpcio>=1.71", | ||
| "mcp", | ||
| "protobuf>=6.33.5", | ||
| ] | ||
|
|
There was a problem hiding this comment.
P2: The adapter imports anyio from four runtime modules, but this package does not declare it directly. Add anyio>=4.5 so the wheel does not rely on mcp's transitive dependency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/pyproject.toml, line 6:
<comment>The adapter imports `anyio` from four runtime modules, but this package does not declare it directly. Add `anyio>=4.5` so the wheel does not rely on `mcp`'s transitive dependency.</comment>
<file context>
@@ -0,0 +1,66 @@
+version = "0.1.0"
+description = "Reference native gRPC adapter for the MCP transport API"
+requires-python = ">=3.10"
+dependencies = [
+ "grpcio>=1.71",
+ "mcp",
</file context>
| dependencies = [ | |
| "grpcio>=1.71", | |
| "mcp", | |
| "protobuf>=6.33.5", | |
| ] | |
| dependencies = [ | |
| "anyio>=4.5", | |
| "grpcio>=1.71", | |
| "mcp", | |
| "protobuf>=6.33.5", | |
| ] |
| if notification.method == "notifications/progress" and "on_progress" in opts: | ||
| progress = ProgressNotificationParams.model_validate(data, by_name=False, strict=True) | ||
| try: | ||
| await opts["on_progress"](progress.progress, progress.total, progress.message) |
There was a problem hiding this comment.
P2: When a progress callback or message handler blocks, these awaits stop receive_response from reading the terminal event, so a successful RPC can time out. Run notification callbacks as managed child tasks and join them during dispatcher shutdown instead of awaiting them on the response-reading task.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/mcp_transport_examples/grpc_response.py, line 51:
<comment>When a progress callback or message handler blocks, these awaits stop `receive_response` from reading the terminal event, so a successful RPC can time out. Run notification callbacks as managed child tasks and join them during dispatcher shutdown instead of awaiting them on the response-reading task.</comment>
<file context>
@@ -0,0 +1,61 @@
+ if notification.method == "notifications/progress" and "on_progress" in opts:
+ progress = ProgressNotificationParams.model_validate(data, by_name=False, strict=True)
+ try:
+ await opts["on_progress"](progress.progress, progress.total, progress.message)
+ except Exception:
+ logger.exception("Progress callback failed")
</file context>
| task_status.started(scope) | ||
| try: | ||
| # A real deadline is the behavior under test, not a synchronization delay. | ||
| await client.call_tool("wait", read_timeout_seconds=0.5 if cause == "timeout" else None) |
There was a problem hiding this comment.
P2: In the timeout cause the 0.5s read timeout is also the gRPC RPC deadline. entered.wait() runs before that deadline expires; if the server starts the handler slower than 0.5s (contended CI loop, coverage) the deadline fires first, the handler never sets entered, and the test hangs on entered.wait() until the outer fail_after(5) fails it. Widen the margin, e.g. use read_timeout_seconds=1.0, so handler startup cannot be preempted by the deadline being tested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/tests/test_grpc_lifecycle.py, line 68:
<comment>In the `timeout` cause the 0.5s read timeout is also the gRPC RPC deadline. `entered.wait()` runs before that deadline expires; if the server starts the handler slower than 0.5s (contended CI loop, coverage) the deadline fires first, the handler never sets `entered`, and the test hangs on `entered.wait()` until the outer `fail_after(5)` fails it. Widen the margin, e.g. use `read_timeout_seconds=1.0`, so handler startup cannot be preempted by the deadline being tested.</comment>
<file context>
@@ -0,0 +1,95 @@
+ task_status.started(scope)
+ try:
+ # A real deadline is the behavior under test, not a synchronization delay.
+ await client.call_tool("wait", read_timeout_seconds=0.5 if cause == "timeout" else None)
+ except MCPError as exc:
+ errors.append(exc.code)
</file context>
| await client.call_tool("wait", read_timeout_seconds=0.5 if cause == "timeout" else None) | |
| await client.call_tool("wait", read_timeout_seconds=1.0 if cause == "timeout" else None) |
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples python examples/transports/demo_grpc.py | ||
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples python examples/transports/demo_grpc_features.py | ||
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples --group dev pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none | ||
| uv run --frozen pyright --project examples/transports |
There was a problem hiding this comment.
P2: The final Pyright command runs in the root environment instead of the adapter environment, so it can fail on missing optional imports such as grpc. Run it with the same UV_PROJECT_ENVIRONMENT (or select the package) as the preceding commands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/README.md, line 12:
<comment>The final Pyright command runs in the root environment instead of the adapter environment, so it can fail on missing optional imports such as `grpc`. Run it with the same `UV_PROJECT_ENVIRONMENT` (or select the package) as the preceding commands.</comment>
<file context>
@@ -0,0 +1,65 @@
+UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples python examples/transports/demo_grpc.py
+UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples python examples/transports/demo_grpc_features.py
+UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples --group dev pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none
+uv run --frozen pyright --project examples/transports
+```
+
</file context>
| uv run --frozen pyright --project examples/transports | |
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples pyright --project examples/transports |
| raise ValueError(f"Request id {request_id!r} is already in flight") | ||
| request = CallRequest( | ||
| method=method, | ||
| params_json=encode_json(params), |
There was a problem hiding this comment.
P2: Valid non-dict mappings such as MappingProxyType and ChainMap fail in encode_json before the RPC starts. Convert non-null params to dict before encoding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/mcp_transport_examples/grpc_client.py, line 96:
<comment>Valid non-`dict` mappings such as `MappingProxyType` and `ChainMap` fail in `encode_json` before the RPC starts. Convert non-null `params` to `dict` before encoding.</comment>
<file context>
@@ -0,0 +1,139 @@
+ raise ValueError(f"Request id {request_id!r} is already in flight")
+ request = CallRequest(
+ method=method,
+ params_json=encode_json(params),
+ request_id_json=encode_json(request_id),
+ report_progress="on_progress" in opts,
</file context>
| finally: | ||
| self._calls.pop(key) | ||
| if not complete: | ||
| call.cancel() |
There was a problem hiding this comment.
P2: When cancel_on_abandon is false, this cleanup still cancels the native RPC. Check that option before calling call.cancel() so abandoned negotiation requests can continue server-side.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/mcp_transport_examples/grpc_client.py, line 133:
<comment>When `cancel_on_abandon` is false, this cleanup still cancels the native RPC. Check that option before calling `call.cancel()` so abandoned negotiation requests can continue server-side.</comment>
<file context>
@@ -0,0 +1,139 @@
+ finally:
+ self._calls.pop(key)
+ if not complete:
+ call.cancel()
+ pending.done.set()
+
</file context>
| - name: Run all adapter regressions | ||
| run: >- | ||
| uv run --frozen --no-sync --package mcp-transport-examples --group dev | ||
| pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none |
There was a problem hiding this comment.
P2: Run this job under coverage and fail on coverage report; plain pytest never executes coverage, so the child fail_under = 100 setting is ignored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/shared.yml, line 122:
<comment>Run this job under coverage and fail on `coverage report`; plain `pytest` never executes coverage, so the child `fail_under = 100` setting is ignored.</comment>
<file context>
@@ -96,6 +96,40 @@ jobs:
+ - name: Run all adapter regressions
+ run: >-
+ uv run --frozen --no-sync --package mcp-transport-examples --group dev
+ pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none
+ --junitxml=transport-results.xml
+ - name: Retain adapter test results
</file context>
|
|
||
| def encode_json(value: Any) -> bytes: | ||
| payload = json.dumps(value, allow_nan=False, ensure_ascii=True, separators=(",", ":")).encode("utf-8") | ||
| if len(payload) > MAX_PAYLOAD_SIZE: |
There was a problem hiding this comment.
P2: The JSON-field limit allows a payload whose enclosing CallRequest or CallEvent exceeds gRPC's default 4 MiB receive limit. Validate the serialized protobuf envelope, or reserve its envelope overhead before accepting the JSON payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/transports/mcp_transport_examples/_grpc_codec.py, line 15:
<comment>The JSON-field limit allows a payload whose enclosing `CallRequest` or `CallEvent` exceeds gRPC's default 4 MiB receive limit. Validate the serialized protobuf envelope, or reserve its envelope overhead before accepting the JSON payload.</comment>
<file context>
@@ -0,0 +1,46 @@
+
+def encode_json(value: Any) -> bytes:
+ payload = json.dumps(value, allow_nan=False, ensure_ascii=True, separators=(",", ":")).encode("utf-8")
+ if len(payload) > MAX_PAYLOAD_SIZE:
+ raise ValueError("Payload exceeds the gRPC binding's size limit")
+ return payload
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async def notify(self, method: str, params: Mapping[str, Any] | None, opts: CallOptions | None = None) -> None: | ||
| """Reject notifications without an originating RPC; use its DispatchContext instead.""" | ||
| raise NoBackChannelError(method) |
There was a problem hiding this comment.
🔴 Server handlers on this binding that send a connection-scoped notification fail their whole request with INVALID_REQUEST, where every other SDK entry delivers or drops it. GRPCServerDispatcher.notify at grpc_server.py:154 raises NoBackChannelError, and serve_modern_dispatcher hands that dispatcher to NotifyOnlyOutbound (runner.py:802). So ctx.session.send_progress_notification(...), send_elicit_complete(...), send_notification(n) with no related_request_id, and ctx.connection.notify(...) (documented "Never raises", connection.py:404) all raise inside the handler. Fix: drop with a debug log like _NoChannelOutbound.notify (connection.py:149-150), the SDK's own no-standalone-channel rule, while send_raw_request keeps raising.
Extended reasoning...
The README calls unsolicited notifications unsupported, but the SDK routes ordinary handler calls onto this channel. ServerSession._notify (session.py:132-135) picks self._connection.outbound whenever related_request_id is None, which is the default for send_progress_notification (session.py:436-455), send_elicit_complete and send_notification. On the modern dispatcher path, Connection.from_envelope gets outbound=NotifyOnlyOutbound(dispatcher) (runner.py:802, 816-817). NotifyOnlyOutbound.notify (connection.py:174-182) only drops LISTEN_STREAM_METHODS (list_changed, resources/updated) and forwards everything else to dispatcher.notify. GRPCServerDispatcher.notify raises NoBackChannelError, an MCPError with code INVALID_REQUEST. Connection.notify (connection.py:407-410) catches only BrokenResourceError and ClosedResourceError, so its "Never raises" contract is broken. The raise escapes the handler; serve_modern_dispatcher re-raises MCPError (runner.py:828-830); invoke() at grpc_server.py:120-121 turns it into error_json. The client gets INVALID_REQUEST "Cannot send…
Verification: normal; acknowledged in diff: README.md:23 says "Unsolicited notifications without a request channel are unsupported" and test_grpc_context.py:28-29 pins dispatcher.notify raising NoBackChannelError, but neither states the actual consequence (the whole inbound request fails with INVALID_REQUEST) and the SDK contract this breaks. Trigger: any server handler on the gRPC binding calls a…
| response = CallEvent(result_json=encode_json(result)) | ||
| except Exception as exc: | ||
| response = CallEvent(error_json=encode_json(modern_error_data(exc).model_dump(by_alias=True))) |
There was a problem hiding this comment.
🔴 Custom-method handlers whose dict result or MCPError.data holds values the SDK's JSON-RPC transports serialize (datetime, UUID, Enum, Decimal, set) get INTERNAL_ERROR or CONNECTION_CLOSED over gRPC. encode_json (_grpc_codec.py:14) is stdlib json.dumps, while stdio/HTTP use pydantic model_dump_json (stdio.py:203) and _dump_result copies dict results verbatim (runner.py:121-125). At grpc_server.py:121 a non-serializable data raises TypeError inside the except clause, escapes invoke() and crashes the servicer. Fix: encode with pydantic's serializer (pydantic_core.to_json inside encode_json, model_dump(mode="json") for ErrorData) so all 4 encode sites (grpc_server.py:87,119,121; grpc_client.py:96) accept what the JSON-RPC path accepts, keeping the size cap.
Extended reasoning...
Spec-method results are re-dumped in json mode by serialize_server_result (methods.py:653-654) and BaseModel results by _dump_result, so those are safe. Exposed inputs are custom-method handlers returning a dict (runner.py:121-125 copies it unchanged), extension resultType shapes, raw ctx.notify params, and ErrorData.data from a handler-raised MCPError, which ErrorData.model_dump(by_alias=True) at grpc_server.py:121 leaves in python mode. Result path: encode_json(result) at :119 raises TypeError, caught by except Exception at :120, modern_error_data logs it and the client receives INTERNAL_ERROR "Internal server error" instead of the result. Error path: encode_json at :121 raises TypeError inside the except clause; finally sets ready, the exception leaves invoke(), the task group cancels the host, tg.cancel_scope.cancel_called is True so cancel_requested is not set, and an ExceptionGroup propagates out of handle(); grpc.aio logs an unexpected servicer exception and returns status UNKNOWN; the client maps that AioRpcError to MCPError CONNECTION_CLOSED "gRPC connection failed"…
Verification: normal — triggers whenever a custom-method handler returns a dict (or raises an MCPError whose data) containing a value pydantic serializes but stdlib json does not (datetime, UUID, Enum, Decimal, set). Mechanism verified: examples/transports/mcp_transport_examples/_grpc_codec.py:14 is plain json.dumps(value, allow_nan=False, ...) with no default=; the JSON-RPC transports instead run…
| raise ValueError(f"Request id {request_id!r} is already in flight") | ||
| request = CallRequest( | ||
| method=method, | ||
| params_json=encode_json(params), |
There was a problem hiding this comment.
🟡 (optional) Callers passing tool arguments over 4 MiB or containing a NaN or infinite float get a bare ValueError from call_tool, which the session's documented MCPError contract does not cover and which stdio and HTTP accept. grpc_client.py:96 runs encode_json(params) before the try block that maps failures to MCPError, so the codec's ValueError escapes unmapped. Fix: any outgoing-payload rejection must surface as an MCPError (INVALID_PARAMS with the codec message) for both the size cap and the non-finite float case, and the size cap should be documented on the client API. [also at: examples/transports/mcp_transport_examples/grpc_server.py:121 - Servers whose handler returns a result over 4 MiB (a resources/read blob, a large image) fail the call with INTERNAL_ERROR and a misleading 'modern request handler raised' traceback, where stdio and HTTP deliver it. grpc_server.py:119 encodes the result inside the same try as the handler call,…]
Extended reasoning...
ClientSession.send_request at session.py:586 forwards the caller's params to send_raw_request; its Raises section (session.py:553-556) promises MCPError or a ValueError only for the name_param case. grpc_client.py:94-99 builds the CallRequest with encode_json(params); _grpc_codec.py:14 uses allow_nan=False, and _grpc_codec.py:15-16 raises ValueError when the JSON exceeds MAX_PAYLOAD_SIZE (4 MiB). This happens before the try at grpc_client.py:106, so the except ValueError at :122 never sees it and the caller receives a raw ValueError. On the base branch the JSON-RPC path serialises the same params through pydantic, which emits null for NaN and has no 4 MiB cap, so a tool that uploads a large blob argument or forwards a float computed as NaN works over stdio and HTTP and fails only over gRPC, and it fails with an exception type the caller's except MCPError does not catch. Four finders (L11 x2, L27, L37) let this go as…
Verification: nit. Trigger: a caller sends a request whose params JSON-encode to more than 4 MiB (e.g. session.call_tool with a large base64 argument), or a raw send_raw_request caller passes a NaN/inf float. Mechanism verified: grpc_client.py:94-99 builds CallRequest(... params_json=encode_json(params) ...) before the try: at :106, and _grpc_codec.py:14-16 raises ValueError (json.dumps… | nit —…
| if notification.method == "notifications/progress" and "on_progress" in opts: | ||
| progress = ProgressNotificationParams.model_validate(data, by_name=False, strict=True) | ||
| try: | ||
| await opts["on_progress"](progress.progress, progress.total, progress.message) | ||
| except Exception: | ||
| logger.exception("Progress callback failed") | ||
| if not run_notify_intercept(intercept, notification.method, data): | ||
| try: | ||
| await on_notify(context, notification.method, data) |
There was a problem hiding this comment.
🟡 (optional) gRPC clients whose logging or message handlers do real work get REQUEST_TIMEOUT on requests the server answered in time, and the handler is cancelled mid-work. grpc_response.py:56 awaits on_notify inline while grpc_client.py:107 holds the stream under anyio.fail_after(opts["timeout"]) and pending.scope; the JSON-RPC dispatcher spawns those callbacks (jsonrpc_dispatcher.py:661) outside any request timeout. Fix: deliver progress/notification callbacks outside the request's timeout scope (spawn them in the dispatcher task group like JSON-RPC, or exclude callback time from the deadline) so callback latency never fails or cancels the originating request.
Extended reasoning...
Every server notification on a gRPC call runs through receive_response before the stream is read further. ClientSession._on_notify (session.py:1446-1485) awaits logging_callback and message_handler in that same task. A user sets timeout=10 on call_tool and has a message_handler that awaits a database write or another MCP request. The server sends three notifications/message events then the result within one second. The client spends the deadline inside the callbacks; fail_after at grpc_client.py:107 raises TimeoutError, mapped at :124-125 to MCPError REQUEST_TIMEOUT, and the callback is cancelled mid-await. The result the server already produced is discarded and the RPC is cancelled at :133. On the base branch the JSON-RPC dispatcher spawns on_notify via _spawn (jsonrpc_dispatcher.py:661) and _shielded_progress (:642), so callback time never counts against the request and the result is delivered. The dismissing finder cited the PendingCall docstring; the docstring describes the task, not the timeout coupling. Rate: every notification-bearing request for clients with non-trivial…
Verification: nit. Trigger: a ClientSession over grpc_client sets a request timeout and its logging_callback/message_handler/progress_callback awaits slow work while the server streams notifications before the result. Mechanism verified: grpc_client.py:100 passes timeout as the gRPC deadline and :107 wraps the whole stream consumption in `with pending.scope,… | normal — triggers whenever a gRPC…
| try: | ||
| state = self._channel.get_state() | ||
| while state != grpc.ChannelConnectivity.SHUTDOWN: | ||
| await self._channel.wait_for_state_change(state) |
There was a problem hiding this comment.
🟡 (optional) Users of the client adapter in an ordinary short-lived script (connect, call, close, exit) get 'Event loop is closed' errors from gRPC's poller thread at interpreter exit, which no SDK transport produces. grpc_client.py:56-59 parks run() in wait_for_state_change; session exit cancels that coroutine but the native watch stays registered until the channel is destroyed, and its completion is posted to a loop that has since closed. Fix: the client dispatcher must not leave a native connectivity watch outstanding after the session exits; await a stop event instead of watching connectivity and detect a closed channel per call via get_state() and AioRpcError. [also at: examples/transports/mcp_transport_examples/grpc_client.py:59 - Operators who close the channel with a grace period get a raw grpc UsageError('Channel is closed.') out of the client session instead of a clean shutdown. grpc_client.py:56-59 loops on get_state() then wait_for_state_change; cygrpc reports SHUTDOWN from check_connectivity_state only once the…]
Extended reasoning...
Client(grpc_client(channel)) exits, cancelling the dispatcher task while it is suspended at grpc_client.py:58 await self._channel.wait_for_state_change(state). Cancelling the Python coroutine does not remove the grpc core watch operation. The user then calls channel.close(), which destroys the channel and completes the watch tag on grpc's process-wide completion queue. anyio.run() returns and closes the loop. grpc's poller thread delivers the late completion with loop.call_soon_threadsafe, which raises RuntimeError('Event loop is closed') or logs an unretrieved exception. The PR's own reproduce_grpc_loop_shutdown.py:22-33 shows exactly this sequence (cancel the watch task, close the channel, loop exit) producing errors on grpcio 1.84 with Python 3.14, and demo_grpc.py:78 uses the same anyio.run(main) pattern. The README labels it a…
Verification: nit — acknowledged in diff: examples/transports/README.md:53-55 ("gRPC's process-wide completion queue can deliver cancelled connectivity-watch callbacks after channel.close() returns. Joining MCP handlers does not drain those native callbacks... Repeated anyio.run() or asyncio.run() lifetimes are outside this adapter's current support") and the shipped reproduce_grpc_loop_shutdown.py…
| except grpc.aio.AioRpcError as exc: | ||
| code = REQUEST_TIMEOUT if exc.code() == grpc.StatusCode.DEADLINE_EXCEEDED else CONNECTION_CLOSED | ||
| raise MCPError( | ||
| code=code, message="gRPC request timed out" if code == REQUEST_TIMEOUT else "gRPC connection failed" | ||
| ) from exc |
There was a problem hiding this comment.
🟡 (optional) Clients rejected by the server's capacity cap or by payload validation are told the connection is closed, so retry and reconnect logic tears down a healthy channel and operators chase a connection fault that does not exist. grpc_client.py:117-121 maps every gRPC status except DEADLINE_EXCEEDED to CONNECTION_CLOSED with the message "gRPC connection failed". Fix: map each server abort to a distinguishable MCP error; RESOURCE_EXHAUSTED and INVALID_ARGUMENT must not become CONNECTION_CLOSED, and CANCELLED, UNAVAILABLE and UNKNOWN should keep the status in the message so the cause is visible.
Extended reasoning...
grpc_server.py:73-75 aborts with RESOURCE_EXHAUSTED whenever the 64-slot limiter is full, and grpc_server.py:96-97 aborts with INVALID_ARGUMENT for a bad payload. The client receives these as grpc.aio.AioRpcError from the stream iteration at grpc_response.py:39. grpc_client.py:117-118 checks only for DEADLINE_EXCEEDED, so both aborts fall into code = CONNECTION_CLOSED and grpc_client.py:119-121 raises MCPError with the message "gRPC connection failed". CONNECTION_CLOSED is the same code ClientSession uses for a genuinely lost session (session.py:1402), so caller code that keys on it treats the channel as dead. Rate: with the default max_requests=64 and long-lived listen RPCs each holding a slot (grpc_server.py:73 releases only at :144), capacity rejections are a routine event under load, not a fault. Consequence relative to the base branch: the JSON-RPC transports return the server's actual error code and message, so a caller can back off on capacity and fix its input on validation; over gRPC every such failure reads as a network problem. The dismissing finder accepted the README's…
Verification: nit, acknowledged in diff: README.md:23 states "other gRPC failures become CONNECTION_CLOSED with the original status exception as their cause", and test_grpc_server.py:90-95 pins RESOURCE_EXHAUSTED -> CONNECTION_CLOSED; the stated bound (status preserved as __cause__) is accurate. Triggering condition: a 65th concurrent request (default max_requests=64) hits the server's… | nit;…
| # Shutdown must wait for this callback, not abandon it after five seconds. | ||
| with anyio.move_on_after(5.1) as window: | ||
| await client_closed.wait() | ||
| assert window.cancelled_caught |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers running this suite get two deliberate 5.1 s stalls (about 10 s) that assert wall-clock time, which the repo's test bar forbids. At test_grpc_client_shutdown.py:74 the test waits a fixed 5.1 s in anyio.move_on_after(5.1) and asserts window.cancelled_caught, i.e. "nothing happened for 5.1 s"; the comment cites a "former five-second join deadline" that no code in this diff has, so the wait guards nothing. Fix: drop the timed window and the widened fail_after(10), and prove the join by ordering with events (release the callback, then await client_closed), which covers the 2 sites listed. Same instruction at 2 sites (test_grpc_client_shutdown.py:74, test_grpc_shutdown_order.py:79). [also at: examples/transports/tests/test_grpc_client_shutdown.py:74 - nit: AGENTS.md asks tests to be fast: test_client_shutdown_joins_blocked_callbacks[True] and test_runtime_keeps_lifespan_alive_through_shielded_handler_cleanup each sit idle in anyio.move_on_after(5.1) waiting for an event that by design never fires, so the two tests add a guaranteed 10+ s of…; examples/transports/tests/test_grpc_client_shutdown.py:93 - nit: AGENTS.md asks indefinite waits to be wrapped in anyio.fail_after(5): the two shutdown tests wrap their event waits in anyio.fail_after(10) instead, widened only because the same tests deliberately idle 5.1 s inside (see the comment on each).; +1 more]
Extended reasoning...
AGENTS.md delegates test rules to .claude/skills/test-quality/SKILL.md, which says "No sleeps, ever", "Never assert wall-clock time, even with huge margins" and "Bound every indefinite wait with anyio.fail_after(5). 5 is the standard". test_grpc_client_shutdown.py:74-76 opens anyio.move_on_after(5.1) around await client_closed.wait() and asserts window.cancelled_caught; the only way that assertion passes is by the wall clock advancing 5.1 s with no event, which is a timing…
Verification: nit. Triggering condition: every run of the example test suite (locally and in the new transport-examples CI job on two Python versions). The rule is real: /home/claude/python-sdk/AGENTS.md:68 says "When writing or reviewing tests, conform to .claude/skills/test-quality/SKILL.md", and that file at lines 65-75 states "No sleeps, ever." (sole exception: tests of time-based features, with a…
| - name: Run all adapter regressions | ||
| run: >- | ||
| uv run --frozen --no-sync --package mcp-transport-examples --group dev | ||
| pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none | ||
| --junitxml=transport-results.xml |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers get an adapter CI job whose pytest run does not carry the PEP 597 encoding guard the rest of CI relies on. The new transport-examples step at shared.yml:119-123 runs pytest without PYTHONWARNDEFAULTENCODING: "1", which the root test job sets at shared.yml:88 and AGENTS.md says CI runs with. Fix: give the adapter pytest step the same env so a text-mode open()/read_text() without encoding= added under examples/transports fails the job as it would under the root suite. Today no file in examples/transports does text I/O, so nothing fails yet.
Extended reasoning...
The convention in AGENTS.md is that CI runs pytest with PYTHONWARNDEFAULTENCODING=1 so that any text I/O omitting encoding= raises EncodingWarning and the error filter turns it into a failure. The root job sets that env at .github/workflows/shared.yml:86-88 and scripts/test:7 mirrors it. The new job at shared.yml:99-131 defines only UV_PROJECT_ENVIRONMENT and runs pytest -c examples/transports/pyproject.toml examples/transports/tests --record-mode=none at lines 119-123 with no PYTHONWARNDEFAULTENCODING. examples/transports/pyproject.toml:34 sets filterwarnings = ["error"], but without the env var CPython never emits EncodingWarning, so the filter has nothing to reject. A grep of examples/transports for open(, read_text, write_text, subprocess and tempfile finds no text I/O today, so this is a missing guard rather than a current failure: the next adapter change (for example a TLS test that writes PEM files with Path.write_text, or a demo that reads a config) can omit encoding= and the adapter job stays green while the same code under tests/ would fail.
Verification: nit. Triggering condition: any future text-mode open()/read_text()/write_text() without encoding= added under examples/transports (or emitted by a dependency such as cassetter while loading cassettes) will pass the only CI job that runs those tests. Mechanism verified: the new transport-examples job (.github/workflows/shared.yml:99-131, diff hunk after line 96) declares only `env:… | nit.…
| @@ -0,0 +1,139 @@ | |||
| """A native gRPC dispatcher that reuses the SDK's high-level client.""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
🟡 nit (optional): New adapter modules add from __future__ import annotations, which the repo's test-quality skill says not to use in this py310+ codebase. sweep:^from __future__ import annotations under examples/transports. Example: grpc_client.py:3, grpc_server.py:3. Fix: remove the future import from the five mcp_transport_examples modules; every annotation there is already valid py310 syntax, and the two string cast("...") calls stay as they are.
Extended reasoning...
The skill referenced by AGENTS.md (.claude/skills/test-quality/SKILL.md, Hygiene) says No from __future__ import annotations (py310+ repo). The five new modules _grpc_codec.py:3, grpc.py:3, grpc_client.py:3, grpc_context.py:3, grpc_response.py:3 and grpc_server.py:3 all carry it. Nothing breaks at runtime; the ruff config for the package selects FA and UP with target py310, so nothing there requires the import either. Some older src/ modules still carry it, so this is a convention nit for new work rather than a defect.
Verification: nit. Rule exists: /home/claude/python-sdk/AGENTS.md:68 says "When writing or reviewing tests, conform to .claude/skills/test-quality/SKILL.md", and that skill (self-described as "Test & code quality guide … What 'best practice for new work' means in this repo") states at SKILL.md:109-110 under Hygiene: "No from __future__ import annotations (py310+ repo)." The diff adds that import at…
| async def send_raw_request( | ||
| self, method: str, params: Mapping[str, Any] | None, opts: CallOptions | None = None | ||
| ) -> dict[str, Any]: | ||
| """Reject server-initiated requests in the modern binding.""" |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md asks public APIs to list catchable exceptions in a Raises: section: GRPCServerDispatcher.send_raw_request/notify, GRPCClientDispatcher.notify and GRPCDispatchContext.send_raw_request all raise NoBackChannelError (which the SDK's own ServerSession/connection docstrings list under Raises:) with only a one-line summary, and GRPCClientDispatcher.send_raw_request documents MCPError but not the ValueError the 4 MiB / non-finite payload limit raises (asserted by test_grpc_client.py). Fix: add a Raises: block naming NoBackChannelError (and ValueError for the payload limit), which covers the 5 sites listed. Same instruction at 5 sites (grpc_server.py:149, grpc_server.py:153, grpc_client.py:137, grpc_context.py:49, grpc_client.py:76).
Extended reasoning...
Nothing fails at runtime. A handler author reading the adapter's docstrings has no listed exception to catch when calling ctx.session.notify()/send_request() on this binding, unlike every SDK-side API that raises NoBackChannelError; and a raw-dispatcher user hitting the 4 MiB cap gets an undocumented ValueError rather than the documented MCPError. Documentation-only gap in an experimental example package.
Verification: AGENTS.md (base commit) "Code Quality": "Public APIs must have docstrings. When a public API raises exceptions a caller would reasonably catch, document them in a Raises: section." — present verbatim. The diff adds examples/transports/mcp_transport_examples/grpc_server.py:149 GRPCServerDispatcher.send_raw_request (docstring "Reject server-initiated requests in the modern binding.") and…
1 participant
Review scope
Stacked on #3517. This diff contains only the native gRPC reference adapter, protobuf binding, examples, regression tests, and optional-package/CI setup; SDK API changes belong to the base PR.
The adapter is experimental and requires one long-lived asyncio loop. It preserves the local review corrections but does not claim interoperability with another protobuf binding or completion of the final compatibility review.
Validation
58 gRPC tests and pre-commit pass after the split.
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.