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: 27803333e2
ℹ️ 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".
|
|
||
| [tool.pytest.ini_options] | ||
| # Live broker checks belong to the optional transport example package. | ||
| testpaths = ["tests"] |
There was a problem hiding this comment.
Restricting discovery here means the existing CI command in .github/workflows/shared.yml (coverage run -m pytest -n auto) skips every test under examples/transports/tests, including the broker-independent gRPC lifecycle and cancellation tests. A repo-wide search of .github, scripts, and .pre-commit-config.yaml finds no dedicated command using examples/transports/pyproject.toml, so all 12 newly added adapter test files can regress while the required full check and coverage report remain green; add a dedicated transport-package CI job or include these tests and dependencies in an existing job.
AGENTS.md reference: AGENTS.md:L98-L105
Useful? React with 👍 / 👎.
Return explicitly after successful transport cleanup and check peer request IDs after closing the inner client context. This preserves behavior while avoiding unreported async-exit branch arcs on older interpreters.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 70 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="docs_src/client_transports/tutorial006.py">
<violation number="1" location="docs_src/client_transports/tutorial006.py:27">
P2: The example fails on its first tool call because `create_direct_dispatcher_pair()` defaults `can_send_request` to `True`, but `greet` asserts the server transport cannot send requests. Create the pair with `can_send_request=False` to match the handler contract.</violation>
</file>
<file name="src/mcp/server/runtime.py">
<violation number="1" location="src/mcp/server/runtime.py:142">
P2: When `server.serve()` exits, task-group cancellation reaches `await run()` before this runtime enables its shield, so the runner's asynchronous stream cleanup can be cancelled before it closes the peer. Shield and bound the driver's cancellation cleanup itself, or close the driver-owned streams under a shielded scope before leaving `serve`.
(Based on your team's feedback about shielded transport termination.)</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Re-trigger cubic
| def direct_client(runtime: ServerRuntime[Any]) -> DispatcherTransport: | ||
| @asynccontextmanager | ||
| async def connection() -> AsyncIterator[Dispatcher[TransportContext]]: | ||
| client_dispatcher, server_dispatcher = create_direct_dispatcher_pair() |
There was a problem hiding this comment.
P2: The example fails on its first tool call because create_direct_dispatcher_pair() defaults can_send_request to True, but greet asserts the server transport cannot send requests. Create the pair with can_send_request=False to match the handler contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs_src/client_transports/tutorial006.py, line 27:
<comment>The example fails on its first tool call because `create_direct_dispatcher_pair()` defaults `can_send_request` to `True`, but `greet` asserts the server transport cannot send requests. Create the pair with `can_send_request=False` to match the handler contract.</comment>
<file context>
@@ -0,0 +1,54 @@
+def direct_client(runtime: ServerRuntime[Any]) -> DispatcherTransport:
+ @asynccontextmanager
+ async def connection() -> AsyncIterator[Dispatcher[TransportContext]]:
+ client_dispatcher, server_dispatcher = create_direct_dispatcher_pair()
+
+ @asynccontextmanager
</file context>
| client_dispatcher, server_dispatcher = create_direct_dispatcher_pair() | |
| client_dispatcher, server_dispatcher = create_direct_dispatcher_pair(can_send_request=False) |
| try: | ||
| if not isinstance(transport, DispatcherTransport): | ||
| status.started() | ||
| await run() |
There was a problem hiding this comment.
P2: When server.serve() exits, task-group cancellation reaches await run() before this runtime enables its shield, so the runner's asynchronous stream cleanup can be cancelled before it closes the peer. Shield and bound the driver's cancellation cleanup itself, or close the driver-owned streams under a shielded scope before leaving serve.
(Based on your team's feedback about shielded transport termination.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/runtime.py, line 142:
<comment>When `server.serve()` exits, task-group cancellation reaches `await run()` before this runtime enables its shield, so the runner's asynchronous stream cleanup can be cancelled before it closes the peer. Shield and bound the driver's cancellation cleanup itself, or close the driver-owned streams under a shielded scope before leaving `serve`.
(Based on your team's feedback about shielded transport termination.) </comment>
<file context>
@@ -0,0 +1,159 @@
+ try:
+ if not isinstance(transport, DispatcherTransport):
+ status.started()
+ await run()
+ except BaseException as exc:
+ run_error = exc
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| raise | ||
| finally: | ||
| lifespan_scope.shield = True | ||
| lifespan_scope.deadline = anyio.current_time() + 5 |
There was a problem hiding this comment.
🟡 (optional) Every CI run now idles about 25 s longer: five new runtime tests sleep out the full real-time 5-second cleanup grace. The grace is a literal anyio.current_time() + 5 at runtime.py:69 and runtime.py:148 with no way to inject a shorter value, so the tests at tests/server/test_runtime.py:339 (2 params), :376, :403 and :515 each wrap the wait in fail_after(10) and burn the grace for real. Fix: expose the grace as one overridable value (a module constant or cleanup_timeout argument on serve()/ServerRuntime.open) that covers both sites, and have those tests set it to a fraction of a second, keeping 5 s as the documented default. [also at: src/mcp/server/runtime.py:148 - nit: maintainers running the core suite now wait about 25 real seconds on five new tests that block on the cleanup grace period.]
Extended reasoning...
AGENTS.md requires fast, deterministic tests and says ./scripts/test takes about 23 s; CI runs that suite across 3.10-3.14 x {ubuntu, windows} x {locked, lowest-direct}. runtime.py:69 sets lifespan_scope.deadline = anyio.current_time() + 5 and runtime.py:148 sets cleanup_scope.deadline = anyio.current_time() + 5; both are literals, not parameters, and nothing lets a caller shorten them. tests/server/test_runtime.py:339 test_host_abandons_unresponsive_cleanup is parametrized over transport and lifespan and each run awaits anyio.sleep_forever() in cleanup until the 5 s deadline fires. tests/server/test_runtime.py:376, :403 and :515 do the same, each commented "includes the documented five-second cleanup grace" and wrapped in anyio.fail_after(10). That is five test invocations of roughly 5 s wall time each, on the asyncio backend where anyio has no mock clock, so about 25 s of pure sleeping is added to a suite that previously finished in about 23 s; with -n auto the cost is spread across workers but each worker that draws one of these tests stalls for the…
Verification: nit. Trigger: every run of tests/server/test_runtime.py (locally and in each CI matrix cell). Mechanism verified: /home/claude/python-sdk/src/mcp/server/runtime.py:69 lifespan_scope.deadline = anyio.current_time() + 5 and :148 cleanup_scope.deadline = anyio.current_time() + 5 are literals with no parameter on ServerRuntime.open()/Server.serve() to shorten them. The suite runs on the real…
| try: | ||
| yield runtime | ||
| finally: |
There was a problem hiding this comment.
🟡 (optional) Callers of the new server.serve() get any exception raised in their async with body re-raised as an ExceptionGroup, so an ordinary except OSError: (or except KeyboardInterrupt:) around the block no longer matches. The body runs inside async with anyio.create_task_group() as tg: at runtime.py:59-61 and anyio wraps host-body exceptions from a task group; tests/server/test_runtime.py:415 already has to use pytest.RaisesGroup(RuntimeError) for a plain raise failure. …
Extended reasoning...
…Fix: capture the body exception and re-raise it after the task group exits, as the repo's own harness does at tests/server/test_runner.py:169-176, so serve() propagates the caller's exception unwrapped like other SDK context managers; connection-task failures are already logged, not raised, so nothing else needs the group.
An operator writes the documented pattern from docs/run/index.md: async with mcp.serve() as runtime: then starts a broker listener or loops on await runtime.connect(...); the listener raising OSError (port in use, broker unreachable) or the user pressing Ctrl-C raises inside the body. runtime.py:59 opens async with anyio.create_task_group() as tg: and runtime.py:61 yields inside it. When the body raises, the exception passes through the task group's __aexit__, which under anyio 4 collects the host exception and raises BaseExceptionGroup("unhandled errors in a TaskGroup", [exc]); runtime.py:64-66 stores body_error and re-raises the group unchanged. tests/server/test_runtime.py:403-418 shows exactly this: raise failure inside the block must be…
Verification: nit. Trigger: any exception raised by the caller's own code inside async with server.serve() as runtime: (e.g. an OSError from a listener bind, or any error from a runtime.connect() loop) leaves the block wrapped in an ExceptionGroup, so a plain except OSError: placed around the block does not match. Mechanism verified: /home/claude/python-sdk/src/mcp/server/runtime.py:57-66 opens…
| async with self._limiter: | ||
| with anyio.CancelScope() as cleanup_scope: | ||
| async with AsyncExitStack() as stack: | ||
| if isinstance(transport, DispatcherTransport): | ||
| dispatcher = await stack.enter_async_context(transport.connection) | ||
| run = partial( | ||
| serve_modern_dispatcher, | ||
| self._server, | ||
| dispatcher, | ||
| lifespan_state=self._lifespan_state, | ||
| task_status=status, | ||
| ) | ||
| else: | ||
| read, write = await stack.enter_async_context(transport) | ||
| run = partial( | ||
| serve_dual_era_loop, | ||
| self._server, | ||
| read, |
There was a problem hiding this comment.
🟡 (optional) Callers waiting in connect() for a free slot when the runtime exits get a foreign CancelledError instead of RuntimeError, and it cancels their enclosing task group too. The _active check at runtime.py:116 runs only before the wait. The serve child parked at runtime.py:133 (or before a dispatcher calls started()) is cancelled by tg.cancel_scope.cancel() at runtime.py:79, and anyio's start() re-raises that in the caller's uncancelled task. Fix: closure while a connect is still waiting must raise RuntimeError("Server runtime is closed") on both backends, e.g. acquire the limiter in the caller's task with acquire_on_behalf_of, re-check _active after it, and release from the serve task.
Extended reasoning...
Trigger: a server at max_connections whose acceptor task (a broker callback loop holding the runtime) awaits runtime.connect(transport) while the async with server.serve() body exits, or a connect() for a DispatcherTransport whose run() has not yet called started() when the body exits. Docstring at runtime.py:112 promises RuntimeError if the runtime has closed; the guard at runtime.py:116 only runs at entry. connect() then does self._task_group.start(serve) at runtime.py:176. The serve task blocks in async with self._limiter at runtime.py:133 (or in await run() at runtime.py:159 before ReadyStatus.started fires). Body exit runs runtime._active = False and tg.cancel_scope.cancel() at runtime.py:78-79. The serve task receives CancelledError there; it is a BaseException so except Exception at runtime.py:171 does not see it and the task ends cancelled before started(). anyio asyncio task_done sees the start future not done and calls task_status_future.set_exception(CancelledError); TaskGroup.start awaits that future, catches CancelledError, cancels the child, and re-raises it in the…
Verification: nit — triggered when a task other than the serve() body is awaiting runtime.connect() (parked on the limiter at capacity, or on a DispatcherTransport that has not yet called started()) at the moment the async with server.serve() body exits. Mechanism verified in /home/claude/python-sdk/src/mcp/server/runtime.py (candidate's line numbers are offset by ~17; the real lines are given here):…
There was a problem hiding this comment.
1 optional suggestion (a nit or a note on pre-existing code) was found and not posted.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
Still open from earlier reviews (3):
- Unresolved: 3 minor or pre-existing.
| transport_context: TransportContext | None = field(default=None, kw_only=True, repr=False) | ||
| """Context supplied by the framing transport; omitted from repr to avoid logging request headers.""" |
There was a problem hiding this comment.
🔴 Operators who log or capture a handler ctx (a %r in middleware, Sentry or rich locals) now see every request header, Authorization and Cookie included; the base prints only <Request object at ...>. The repr=False at message.py:55 only hides the SDK's own SessionMessage logs. ServerRequestContext.transport (context.py:50) and TransportContext.headers keep default reprs, so the Headers stamped at sse.py:285 and streamable_http.py:256 print in full. Fix: keep headers out of every context repr: field(default=None, repr=False) on ServerRequestContext.transport, and durably on TransportContext.headers, which also covers dispatcher contexts and user subclasses.
Extended reasoning...
This push is what puts real headers into the default HTTP deployments: sse.py:283-286 and streamable_http.py:250-259 now build TransportContext(..., headers=request.headers) and _default_transport_builder (jsonrpc_dispatcher.py:197-198) forwards that object into dctx.transport. runner.py:339 copies dctx.transport into ServerRequestContext.transport. ServerRequestContext is @ dataclass(kw_only=True) (context.py:30-50) with the default generated __repr__, and TransportContext is @ dataclass(kw_only=True, frozen=True) (transport_context.py:15) whose headers field is also in its repr. Starlette Headers.__repr__ prints Headers({'authorization': 'Bearer ...', 'cookie': ...}). So repr(ctx) for any lowlevel handler or ServerMiddleware.__call__(ctx, call_next) (context.py:158-160 invites logging middleware to record it) now contains the bearer token. On base ServerRequestContext had no transport field and request=<starlette.requests.Request object at 0x...> shows nothing. The author added repr=False plus tests/shared/test_message.py to stop exactly this…
Verification: normal — triggers whenever operator code reprs a ServerRequestContext (a %r in a ServerMiddleware/lowlevel handler, Sentry's default include_local_variables, rich show_locals) on an SSE or streamable-HTTP deployment. Mechanism verified: src/mcp/server/sse.py:283-286 and src/mcp/server/streamable_http.py:256-258 now build TransportContext(..., headers=request.headers) (Starlette…
There was a problem hiding this comment.
2 issues found across 62 files (changes from recent commits).
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="src/mcp/shared/jsonrpc_dispatcher.py">
<violation number="1" location="src/mcp/shared/jsonrpc_dispatcher.py:198">
P2: When metadata leaves `can_send_request` at its default but its `transport_context` denies the back-channel, this replacement changes the context to `True`. Combine both capability flags so an explicit transport-context denial cannot expose a nonexistent back-channel.</violation>
</file>
<file name="TRANSPORT_API_PLAN.md">
<violation number="1" location="TRANSPORT_API_PLAN.md:265">
P2: `pyright` runs outside the adapter environment. Give this invocation the same environment, package, and dev-group selectors as the adapter test command.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| """ | ||
| can_send_request = metadata.can_send_request if isinstance(metadata, ServerMessageMetadata) else True | ||
| if isinstance(metadata, ServerMessageMetadata) and metadata.transport_context is not None: | ||
| return replace(metadata.transport_context, can_send_request=can_send_request) |
There was a problem hiding this comment.
P2: When metadata leaves can_send_request at its default but its transport_context denies the back-channel, this replacement changes the context to True. Combine both capability flags so an explicit transport-context denial cannot expose a nonexistent back-channel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/shared/jsonrpc_dispatcher.py, line 198:
<comment>When metadata leaves `can_send_request` at its default but its `transport_context` denies the back-channel, this replacement changes the context to `True`. Combine both capability flags so an explicit transport-context denial cannot expose a nonexistent back-channel.</comment>
<file context>
@@ -194,6 +194,8 @@ def _default_transport_builder(metadata: MessageMetadata) -> TransportContext:
"""
can_send_request = metadata.can_send_request if isinstance(metadata, ServerMessageMetadata) else True
+ if isinstance(metadata, ServerMessageMetadata) and metadata.transport_context is not None:
+ return replace(metadata.transport_context, can_send_request=can_send_request)
return TransportContext(kind="jsonrpc", can_send_request=can_send_request)
</file context>
| return replace(metadata.transport_context, can_send_request=can_send_request) | |
| return replace( | |
| metadata.transport_context, | |
| can_send_request=can_send_request and metadata.transport_context.can_send_request, | |
| ) |
| ./scripts/test | ||
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv sync --frozen --package mcp-transport-examples --group dev | ||
| 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: pyright runs outside the adapter environment. Give this invocation the same environment, package, and dev-group selectors as the adapter test command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At TRANSPORT_API_PLAN.md, line 265:
<comment>`pyright` runs outside the adapter environment. Give this invocation the same environment, package, and dev-group selectors as the adapter test command.</comment>
<file context>
@@ -252,7 +256,27 @@ The native adapter now exposes gRPC's verified `peer_identity_key` and immutable
+./scripts/test
+UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv sync --frozen --package mcp-transport-examples --group dev
+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
+DOCS_LANGUAGES=en-only bash scripts/docs/build.sh
+```
</file context>
| uv run --frozen pyright --project examples/transports | |
| UV_PROJECT_ENVIRONMENT=examples/transports/.venv uv run --frozen --package mcp-transport-examples --group dev pyright --project examples/transports |
1 participant
Review scope
This PR now contains only the SDK transport APIs, transport metadata, lifecycle handling, core tests, and documentation. Network adapters and their dependencies have moved to separate PRs; the previous local review corrections are preserved without adding further feature work.
Review order
DispatcherTransport,ServerRuntime, metadata, and cleanupEach adapter PR targets the preceding branch, so its diff shows only its own layer.
TRANSPORT_API_PLAN.mdtracks the whole initiative rather than this PR alone; final API decisions and adapter validation gates remain open.Validation
Pre-commit passes after the split; the latest full core run passed 6,033 tests with 100% branch coverage and
strict-no-cover.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.