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: e49a483420
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the unconditional from mcp.server ... imports at the top of src/mcp-client/mcp_client/client/_memory.py for a standalone-install break — ruled out, since that private module is not imported by any mcp_client public path (the Client(server) in-process branch in client.py imports mcp.server.mcpserver lazily, and only the full-SDK mcp.client shim re-exports _memory).
Extended reasoning...
The change extracts the client, transports, OAuth, and shared protocol code into a new mcp-client distribution under src/mcp-client, replaces the src/mcp/client and src/mcp/shared modules with re-export shims, and updates publishing workflows, docs, and lockstep version pins. It relocates auth code (OAuth2, client credentials, identity assertion) but as a move rather than a behavioral change. Five confirmed findings are posted inline and further verified findings were withheld, so approval is not appropriate; the note above records one additional standalone-install concern that was examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
.github/workflows/publish-pypi.yml— If themcp-clientPyPI project is not yet owned or trusted-publisher-configured when a release fires, users get anmcprelease on PyPI that cannot be installed. The publish step at .github/workflows/publish-pypi.yml:61 uploads all six files fromdist/in one twine call; glob order putsmcp-X.tar.gzandmcp-X-py3-none-any.whlbeforemcp_client-*, and a 403 on the firstmcp_clientfile aborts the rest aftermcpis already live pinned tomcp-client==X. Fix: publish in dependency order withmcplast (separate publish steps formcp_types-*, thenmcp_client-*, thenmcp-*, each gated on the previous), or fail the job before any upload when a target project rejects the OIDC token, so a partial run never leaves anmcpversion whose exact pin cannot resolve. …Why this was flagged
…The PR text calls the configuration a pre-release step; that step is outside the repository and the workflow does nothing to enforce it.
A GitHub release is published before the
mcp-clientPyPI project has this repository'spublish-pypi.yml/releaseenvironment registered as a trusted publisher (the PR description says ownership of the existingmcp-clientproject still has to be verified). Therelease-buildjob now builds three distributions (.github/workflows/publish-pypi.yml:31-33) andpypi-publishhands the wholedist/directory topypa/gh-action-pypi-publish(.github/workflows/publish-pypi.yml:61-65), which runs a singletwine upload dist/*. In both C and en_US collationmcp-X.tar.gzandmcp-X-py3-none-any.whlsort beforemcp_client-X*, somcpX is uploaded first; themcp_clientupload then gets a 403 thatskip-existingdoes not cover (it only skips 'already exists' responses) and twine stops. The publishedmcpX carriesRequires-Dist: mcp-client==X(pyproject.toml:135) that no index can satisfy:pip install mcp==Xfails to resolve, and unpinned…Verification: nit — acknowledged in diff: RELEASE.md (this PR) adds "Before the first
mcp-clientrelease, verify ownership of the existing PyPI project and configure that same trusted publisher for it too" and the workflow comment at .github/workflows/publish-pypi.yml:63-65 ("Lets a re-run after a partially failed upload publish the remaining files") — the re-run bound is accurate, but it does not…
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
41 issues found across 134 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="tests/client/test_package.py">
<violation number="1" location="tests/client/test_package.py:18">
P2: Both identity checks only compare names that exist on both sides, so a client export dropped by the split passes silently. `set(mcp_client.__all__) & set(mcp.__all__)` shrinks when a name is removed from either `__all__`, and the walk test iterates only `mcp_client` modules and only names present in the extracted package's `__all__` (packages like `mcp_client.os` and `mcp_client.shared` have empty `__all__` and are checked only for nested-module identity). If, say, `Client` were removed from `mcp_client.__all__`, neither test asserts anything about it, and the PR's central guarantee (client API fully preserved) is unenforced. Add a completeness assertion in the reverse direction — e.g. that every client-side name in `mcp_client.__all__` is also exported by `mcp` with identical objects, or that every legacy `mcp.client.*`/`mcp.shared.*` module has an extracted counterpart — so a dropped export fails the suite.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/session.py">
<violation number="1" location="src/mcp-client/mcp_client/client/session.py:591">
P1: `send_request` rejects modern extension results against the core `tools/call` surface before `_call_tool_adapter` can parse them. Skip this core validation when active claims are present and let the claim adapter validate the response.</violation>
</file>
<file name="docs/run/legacy-clients.md">
<violation number="1" location="docs/run/legacy-clients.md:136">
P2: Regenerate and commit the translated `run/legacy-clients.md` pages with this traceback update. All twelve locale pages still show `mcp.shared.exceptions.MCPError`, so those sites document a traceback that no longer matches the extracted client package.
(Based on your team's feedback about keeping translated docs aligned with the English source.) .</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/inbound.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/inbound.py:179">
P2: `decode_header_value` lets `ValueError` escape for a sentinel containing non-ASCII base64 text, so malformed `Mcp-Name` or `Mcp-Param-*` headers can produce an HTTP 500 instead of a `HEADER_MISMATCH` response. Catch `ValueError` with `binascii.Error` and return `None`.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/subscriptions.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/subscriptions.py:111">
P2: `ListenHandler` drops updates for sub-resources: a subscription to `note://todo` never receives `ResourceUpdated(uri="note://todo/draft")`. Match the subscribed URI and its sub-resources here so server delivery follows the protocol and client admission behavior.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/auth/oauth2.py">
<violation number="1" location="src/mcp-client/mcp_client/client/auth/oauth2.py:483">
P3: `_handle_token_response` decodes a non-200 error body with strict UTF-8. A token endpoint returning a non-UTF-8 error page (e.g. ISO-8859-1 or binary) makes this raise `UnicodeDecodeError`, which is caught by `_auth_flow`'s generic `except Exception` and re-raised, masking the intended `OAuthTokenError` and the server's actual error. The sibling provider in this batch already handles this: `IdentityAssertionOAuthProvider` decodes with `.decode(errors="replace")`. Use `errors="replace"` here too (or `decode('utf-8', errors='replace')`) so the token-exchange error is reported, not a byte-decoding crash.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/auth/oauth2.py:646">
P1: A new 401 discovery pass retains the previous PRM/AS metadata when every probe fails. Clear `protected_resource_metadata`, `auth_server_url`, and `oauth_metadata` before Step 1 so the flow falls back to the current origin or fails closed instead of reusing stale endpoints.
(Based on your team's feedback about discovery state reset on pass.)</violation>
</file>
<file name="src/mcp-client/mcp_client/client/auth/utils.py">
<violation number="1" location="src/mcp-client/mcp_client/client/auth/utils.py:33">
P2: `extract_field_from_www_auth` can extract values from quoted text or longer parameter names instead of actual `WWW-Authenticate` parameters. Parse auth-param names with proper boundaries before using the result for requested scopes or discovery URLs.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/auth/utils.py:335">
P2: This validation accepts `https:///client` because it never requires an authority. Require a non-empty, valid host before accepting the configured CIMD URL.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/__main__.py">
<violation number="1" location="src/mcp-client/mcp_client/client/__main__.py:77">
P2: `mcp-client` does not install Trio, but this entry point always selects the Trio backend, so standalone CLI invocation fails before connecting to a server. Use the guaranteed asyncio backend or declare Trio as a runtime dependency.</violation>
</file>
<file name="src/mcp/client/client.py">
<violation number="1" location="src/mcp/client/client.py:59">
P2: This module replacement drops the existing `mcp.client.client.Server` and `MCPServer` exports, breaking imports that worked before the split. Copy these compatibility attributes onto `_implementation` before replacing the module.</violation>
</file>
<file name="src/mcp-client/mcp_client/os/win32/utilities.py">
<violation number="1" location="src/mcp-client/mcp_client/os/win32/utilities.py:74">
P2: This resolver returns `.ps1` scripts as executables, but both spawn paths pass them directly to Windows process creation without PowerShell, so PowerShell MCP servers fail with an invalid-executable error. Remove `.ps1` from this resolver or change the spawn contract to invoke PowerShell with `-File`.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/_otel.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/_otel.py:56">
P2: Malformed `_meta.tracestate` can escape this helper and fail the server request instead of falling back to ambient parenting. Catch `AttributeError` alongside the existing exceptions (or validate carrier values before extraction).</violation>
</file>
<file name="src/mcp-client/mcp_client/client/auth/extensions/client_credentials.py">
<violation number="1" location="src/mcp-client/mcp_client/client/auth/extensions/client_credentials.py:58">
P1: This issuer check can reuse stale authorization-server metadata after a later discovery pass fails, causing the provider to build a token request from an endpoint that the current MCP server has not validated. Clear discovery state at the start of each pass and commit only metadata validated during that pass.
(Based on your team's feedback about discovery state reset on pass.)</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/auth/extensions/client_credentials.py:249">
P2: `additional_claims` can overwrite mandatory JWT claims such as `aud`, `iss`, `exp`, and `jti`, producing invalid or unexpectedly long-lived client assertions. Merge additional claims first and apply the required claims afterward so only non-reserved claims can be customized.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/context.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/context.py:9">
P3: This module claims `ClientContext` is an alias, but the standalone package neither defines nor exports it, so the documented import fails. Add and export the alias, or remove this claim.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/auth.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/auth.py:118">
P2: `redirect_uris` accepts fragments, so a registered `https://client.example/callback#fragment` reaches authorization and produces a callback URL whose fragment is not sent to the callback server. Reject fragment-bearing redirect URIs in both client metadata and registered-client metadata per RFC 6749 §3.1.2.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/auth_utils.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/auth_utils.py:65">
P2: `check_resource_allowed` accepts resources that differ by query or fragment. Because `get_resource_url()` sends the accepted PRM value verbatim, the client can request a token for a different resource and can emit an RFC 8707-invalid fragment; compare these URI components and reject fragments before accepting PRM metadata.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/caching.py">
<violation number="1" location="src/mcp-client/mcp_client/client/caching.py:256">
P2: `capture` returns the same generation for concurrent misses, so an older response can overwrite a newer response and be served for the full TTL. Advance a per-fetch generation or coalesce same-key misses before allowing a write.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/streamable_http.py">
<violation number="1" location="src/mcp-client/mcp_client/client/streamable_http.py:239">
P2: A resumed POST stream that is redirected outside the endpoint origin loses the redirect diagnostic and spends its retry budget before returning a generic connection-closed error. Apply `_unfollowed_redirect` here and resolve the request immediately with the same `INVALID_REQUEST` redirect message used by the other GET paths.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/streamable_http.py:239">
P1: A non-success response from the initial resumption GET tears down the shared transport instead of resolving this request. Catch the HTTP status error and send a synthesized JSON-RPC error to `ctx.read_stream_writer`, leaving other requests and the session usable.</violation>
<violation number="3" location="src/mcp-client/mcp_client/client/streamable_http.py:755">
P3: `terminate_session` runs in the context-manager `finally` without a cancellation shield. When the transport exits because the enclosing scope is cancelled, the awaited DELETE is cancelled mid-flight and the server-side session is left alive until its own idle timeout. `stdio_client` in this same batch shields its shutdown (`with anyio.CancelScope(shield=True)`); mirror that here so session termination always completes.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/stdio.py">
<violation number="1" location="src/mcp-client/mcp_client/client/stdio.py:151">
P2: Strict decoding lets malformed server bytes escape `stdout_reader` as an unhandled task-group exception. Catch `UnicodeDecodeError` deliberately and report or close the read stream so the client sees a transport failure instead.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/stdio.py:178">
P2: A strict ASCII encoding with a non-ASCII request raises `UnicodeEncodeError`, which this handler misses and leaves the read side open. Add `UnicodeError` to the exception tuple so the session observes clean connection closure.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/_probe.py">
<violation number="1" location="src/mcp-client/mcp_client/client/_probe.py:102">
P2: These direct fallback calls bypass the `-32022` handling used for probe errors. If the fallback handshake reports a mutual modern version, auto negotiation fails instead of re-probing once at that version; route both fallback paths through the same bounded handshake/reprobe logic.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/_probe.py:104">
P2: This treats every successful discover response without a known modern version as a legacy advertisement, including `supportedVersions=["2099-01-01"]`. Fall back only when the advertisement includes a handshake-era version; otherwise preserve the no-mutual-version incompatibility error.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/direct_dispatcher.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/direct_dispatcher.py:76">
P2: `_DirectDispatchContext` never closes when `_on_request` finishes, so a retained context can continue sending requests and notifications after the inbound response has completed. Close the context in `_dispatch_request`'s `finally` and reject or drop subsequent back-channel calls, matching `_JSONRPCDispatchContext`.</violation>
<violation number="2" location="src/mcp-client/mcp_client/shared/direct_dispatcher.py:300">
P1: `DirectDispatcher` forwards `notifications/cancelled` without setting the matching request context's `cancel_requested` event. Handlers waiting for cancellation therefore keep running on the in-process path; track in-flight contexts and apply cancellation before forwarding the notification.</violation>
<violation number="3" location="src/mcp-client/mcp_client/shared/direct_dispatcher.py:304">
P2: This direct notification path lets an `OnNotify` exception escape to the sender, unlike `JSONRPCDispatcher` and the documented fire-and-forget behavior. Catch `Exception`, log it, and return so one notification cannot fail its peer's send.</violation>
</file>
<file name="src/mcp-client/mcp_client/shared/uri_template.py">
<violation number="1" location="src/mcp-client/mcp_client/shared/uri_template.py:251">
P2: Exploded variables accept scalar strings during expansion but always come back as lists during matching, breaking the class's bidirectional contract and changing the value type. Reject scalar inputs for exploded variables or define and enforce a consistent scalar/list contract before exposing this as inverse matching.</violation>
<violation number="2" location="src/mcp-client/mcp_client/shared/uri_template.py:826">
P3: The explode rejection message says "is not supported for matching", but this `raise` sits in `_parse_expression`, so `UriTemplate.parse` rejects these templates for every use — `expand()` is blocked too. For `{var*}`, `{+var*}`, `{#var*}` the Level 4 explode expansion is RFC-defined (comma-join, identical to the non-explode form for sequence values), so a user doing pure expansion gets an error that misattributes the limitation to matching. Rephrase to cover both directions, e.g. "Explode modifier on {a*} is not supported".</violation>
</file>
<file name=".github/workflows/shared.yml">
<violation number="1" location=".github/workflows/shared.yml:68">
P2: The package checks run under only the runner's implicit Python version, so they do not validate the built wheel and sdist at the supported 3.10 and 3.14 endpoints. Add a Python matrix or explicit `--python` runs; the later source-tree matrix does not cover artifact installation.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/session_group.py">
<violation number="1" location="src/mcp-client/mcp_client/client/session_group.py:305">
P2: If component aggregation raises, such as on a duplicate name, this call propagates without rolling back the session registered by `_establish_session`. Each failed connection remains open and hidden until the group exits; close the just-established session on aggregation failure.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/session_group.py:369">
P2: Cancellation during transport entry or `session.initialize()` bypasses this `except Exception`, because AnyIO cancellation inherits `BaseException`; the manually owned stack is then never closed. Catch cancellation while unwinding the stack and remove any partial registration.</violation>
<violation number="3" location="src/mcp-client/mcp_client/client/session_group.py:392">
P2: `_aggregate_components` fetches each list only once and discards `next_cursor`, so paginated servers expose only their first page in `group.tools`, `group.resources`, and `group.prompts`. Loop through each cursor until `None` before aggregating.</violation>
<violation number="4" location="src/mcp-client/mcp_client/client/session_group.py:424">
P2: This deletion assumes every session has an entry in `_session_exit_stacks`, but `connect_with_session` accepts externally owned sessions and never creates one. An empty external server therefore raises `KeyError`; for internally opened servers, disconnect cannot close the stack after this deletion.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/auth/extensions/identity_assertion.py">
<violation number="1" location="src/mcp-client/mcp_client/client/auth/extensions/identity_assertion.py:177">
P2: A later `insufficient_scope` challenge drops scopes granted by earlier step-ups. For example, after requesting `mcp files:read`, a challenge for `files:write` asks for only `mcp files:write`, so the retried token can lose `files:read`; include the current token's granted scope when building each step-up union.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/sse.py">
<violation number="1" location="src/mcp-client/mcp_client/client/sse.py:27">
P2: `remove_request_params` preserves the query when `urlparse(url).path` is empty. The debug log then records root-URL credentials such as `?token=secret`; clear query and fragment with `ParseResult._replace`.</violation>
<violation number="2" location="src/mcp-client/mcp_client/client/sse.py:89">
P2: `urlparse().netloc` compares spelling, not URL origin, so `http://example.com` rejects endpoint `http://EXAMPLE.com:80/messages`. Normalize host and default ports like `_within_origin` before rejecting a valid same-origin endpoint.
(Based on your team's feedback about SSE endpoint resolution.)</violation>
<violation number="3" location="src/mcp-client/mcp_client/client/sse.py:152">
P1: `post_writer` swallows HTTP errors from `_send_message` and leaves the SSE read stream open. `ClientSession.send_request()` then waits forever; send the exception and close the read writer so EOF wakes pending requests.</violation>
</file>
<file name="src/mcp-client/mcp_client/client/client.py">
<violation number="1" location="src/mcp-client/mcp_client/client/client.py:461">
P2: A failed connection permanently marks this `Client` as entered. Reset `_entered` when `__aenter__` fails so callers can retry after a transient transport or handshake error.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # Literal fallback covers pre-handshake and stateless; matches runner.py. | ||
| version = self._negotiated_version or "2025-11-25" | ||
| try: | ||
| _methods.validate_server_result(method, version, raw) |
There was a problem hiding this comment.
P1: send_request rejects modern extension results against the core tools/call surface before _call_tool_adapter can parse them. Skip this core validation when active claims are present and let the claim adapter validate the response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/session.py, line 591:
<comment>`send_request` rejects modern extension results against the core `tools/call` surface before `_call_tool_adapter` can parse them. Skip this core validation when active claims are present and let the claim adapter validate the response.</comment>
<file context>
@@ -0,0 +1,1506 @@
+ # Literal fallback covers pre-handshake and stateless; matches runner.py.
+ version = self._negotiated_version or "2025-11-25"
+ try:
+ _methods.validate_server_result(method, version, raw)
+ except KeyError:
+ pass
</file context>
| _methods.validate_server_result(method, version, raw) | |
| if not (method == "tools/call" and self._active_claims): | |
| _methods.validate_server_result(method, version, raw) |
| www_auth_resource_metadata_url = extract_resource_metadata_from_www_auth(response) | ||
|
|
||
| # Step 1: Discover protected resource metadata (SEP-985 with fallback support) | ||
| prm_discovery_urls = build_protected_resource_metadata_discovery_urls( |
There was a problem hiding this comment.
P1: A new 401 discovery pass retains the previous PRM/AS metadata when every probe fails. Clear protected_resource_metadata, auth_server_url, and oauth_metadata before Step 1 so the flow falls back to the current origin or fails closed instead of reusing stale endpoints.
(Based on your team's feedback about discovery state reset on pass.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/auth/oauth2.py, line 646:
<comment>A new 401 discovery pass retains the previous PRM/AS metadata when every probe fails. Clear `protected_resource_metadata`, `auth_server_url`, and `oauth_metadata` before Step 1 so the flow falls back to the current origin or fails closed instead of reusing stale endpoints.
(Based on your team's feedback about discovery state reset on pass.) </comment>
<file context>
@@ -0,0 +1,793 @@
+ www_auth_resource_metadata_url = extract_resource_metadata_from_www_auth(response)
+
+ # Step 1: Discover protected resource metadata (SEP-985 with fallback support)
+ prm_discovery_urls = build_protected_resource_metadata_discovery_urls(
+ www_auth_resource_metadata_url, self.context.server_url
+ )
</file context>
| if issuer is None: | ||
| return | ||
| metadata = context.oauth_metadata | ||
| if metadata is not None and issuers_match(str(metadata.issuer), issuer): |
There was a problem hiding this comment.
P1: This issuer check can reuse stale authorization-server metadata after a later discovery pass fails, causing the provider to build a token request from an endpoint that the current MCP server has not validated. Clear discovery state at the start of each pass and commit only metadata validated during that pass.
(Based on your team's feedback about discovery state reset on pass.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/auth/extensions/client_credentials.py, line 58:
<comment>This issuer check can reuse stale authorization-server metadata after a later discovery pass fails, causing the provider to build a token request from an endpoint that the current MCP server has not validated. Clear discovery state at the start of each pass and commit only metadata validated during that pass.
(Based on your team's feedback about discovery state reset on pass.) </comment>
<file context>
@@ -0,0 +1,416 @@
+ if issuer is None:
+ return
+ metadata = context.oauth_metadata
+ if metadata is not None and issuers_match(str(metadata.issuer), issuer):
+ return
+ context.oauth_metadata = None
</file context>
| # The same GET would be redirected again, so retrying cannot help. | ||
| logger.warning(f"GET stream not opened: {redirect}") | ||
| return | ||
| event_source.response.raise_for_status() |
There was a problem hiding this comment.
P1: A non-success response from the initial resumption GET tears down the shared transport instead of resolving this request. Catch the HTTP status error and send a synthesized JSON-RPC error to ctx.read_stream_writer, leaving other requests and the session usable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/streamable_http.py, line 239:
<comment>A non-success response from the initial resumption GET tears down the shared transport instead of resolving this request. Catch the HTTP status error and send a synthesized JSON-RPC error to `ctx.read_stream_writer`, leaving other requests and the session usable.</comment>
<file context>
@@ -0,0 +1,758 @@
+ # The same GET would be redirected again, so retrying cannot help.
+ logger.warning(f"GET stream not opened: {redirect}")
+ return
+ event_source.response.raise_for_status()
+ logger.debug("GET SSE connection established")
+
</file context>
| # dropped, not raised back into the sender's call. | ||
| logger.debug("dropped notification %r to closed DirectDispatcher", method) | ||
| return | ||
| if run_notify_intercept(self._on_notify_intercept, method, params): |
There was a problem hiding this comment.
P1: DirectDispatcher forwards notifications/cancelled without setting the matching request context's cancel_requested event. Handlers waiting for cancellation therefore keep running on the in-process path; track in-flight contexts and apply cancellation before forwarding the notification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/shared/direct_dispatcher.py, line 300:
<comment>`DirectDispatcher` forwards `notifications/cancelled` without setting the matching request context's `cancel_requested` event. Handlers waiting for cancellation therefore keep running on the in-process path; track in-flight contexts and apply cancellation before forwarding the notification.</comment>
<file context>
@@ -0,0 +1,334 @@
+ # dropped, not raised back into the sender's call.
+ logger.debug("dropped notification %r to closed DirectDispatcher", method)
+ return
+ if run_notify_intercept(self._on_notify_intercept, method, params):
+ return
+ assert self._on_notify is not None
</file context>
|
|
||
| Satisfies `Outbound`, so `ClientPeer` can wrap it. Shared between client and | ||
| server: the server's `Context` extends this with `lifespan`/`connection`; | ||
| `ClientContext` is just an alias. |
There was a problem hiding this comment.
P3: This module claims ClientContext is an alias, but the standalone package neither defines nor exports it, so the documented import fails. Add and export the alias, or remove this claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/shared/context.py, line 9:
<comment>This module claims `ClientContext` is an alias, but the standalone package neither defines nor exports it, so the documented import fails. Add and export the alias, or remove this claim.</comment>
<file context>
@@ -0,0 +1,85 @@
+
+Satisfies `Outbound`, so `ClientPeer` can wrap it. Shared between client and
+server: the server's `Context` extends this with `lifespan`/`connection`;
+`ClientContext` is just an alias.
+"""
+
</file context>
| # Explode only makes sense for operators that repeat a separator. | ||
| # Simple/reserved/fragment have no per-item separator; query-explode | ||
| # needs order-agnostic dict matching which we don't support yet. | ||
| if explode and operator in ("", "+", "#", "?", "&"): |
There was a problem hiding this comment.
P3: The explode rejection message says "is not supported for matching", but this raise sits in _parse_expression, so UriTemplate.parse rejects these templates for every use — expand() is blocked too. For {var*}, {+var*}, {#var*} the Level 4 explode expansion is RFC-defined (comma-join, identical to the non-explode form for sequence values), so a user doing pure expansion gets an error that misattributes the limitation to matching. Rephrase to cover both directions, e.g. "Explode modifier on {a*} is not supported".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/shared/uri_template.py, line 826:
<comment>The explode rejection message says "is not supported for matching", but this `raise` sits in `_parse_expression`, so `UriTemplate.parse` rejects these templates for every use — `expand()` is blocked too. For `{var*}`, `{+var*}`, `{#var*}` the Level 4 explode expansion is RFC-defined (comma-join, identical to the non-explode form for sequence values), so a user doing pure expansion gets an error that misattributes the limitation to matching. Rephrase to cover both directions, e.g. "Explode modifier on {a*} is not supported".</comment>
<file context>
@@ -0,0 +1,1116 @@
+ # Explode only makes sense for operators that repeat a separator.
+ # Simple/reserved/fragment have no per-item separator; query-explode
+ # needs order-agnostic dict matching which we don't support yet.
+ if explode and operator in ("", "+", "#", "?", "&"):
+ raise InvalidUriTemplate(
+ f"Explode modifier on {{{operator}{name}*}} is not supported for matching",
</file context>
| try: | ||
| yield read_stream, write_stream | ||
| finally: | ||
| if transport.session_id and terminate_on_close: |
There was a problem hiding this comment.
P3: terminate_session runs in the context-manager finally without a cancellation shield. When the transport exits because the enclosing scope is cancelled, the awaited DELETE is cancelled mid-flight and the server-side session is left alive until its own idle timeout. stdio_client in this same batch shields its shutdown (with anyio.CancelScope(shield=True)); mirror that here so session termination always completes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/streamable_http.py, line 755:
<comment>`terminate_session` runs in the context-manager `finally` without a cancellation shield. When the transport exits because the enclosing scope is cancelled, the awaited DELETE is cancelled mid-flight and the server-side session is left alive until its own idle timeout. `stdio_client` in this same batch shields its shutdown (`with anyio.CancelScope(shield=True)`); mirror that here so session termination always completes.</comment>
<file context>
@@ -0,0 +1,758 @@
+ try:
+ yield read_stream, write_stream
+ finally:
+ if transport.session_id and terminate_on_close:
+ await transport.terminate_session(client)
+ tg.cancel_scope.cancel()
</file context>
| """Handle token exchange response.""" | ||
| if response.status_code not in {200, 201}: | ||
| body = await response.aread() | ||
| body_text = body.decode("utf-8") |
There was a problem hiding this comment.
P3: _handle_token_response decodes a non-200 error body with strict UTF-8. A token endpoint returning a non-UTF-8 error page (e.g. ISO-8859-1 or binary) makes this raise UnicodeDecodeError, which is caught by _auth_flow's generic except Exception and re-raised, masking the intended OAuthTokenError and the server's actual error. The sibling provider in this batch already handles this: IdentityAssertionOAuthProvider decodes with .decode(errors="replace"). Use errors="replace" here too (or decode('utf-8', errors='replace')) so the token-exchange error is reported, not a byte-decoding crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp-client/mcp_client/client/auth/oauth2.py, line 483:
<comment>`_handle_token_response` decodes a non-200 error body with strict UTF-8. A token endpoint returning a non-UTF-8 error page (e.g. ISO-8859-1 or binary) makes this raise `UnicodeDecodeError`, which is caught by `_auth_flow`'s generic `except Exception` and re-raised, masking the intended `OAuthTokenError` and the server's actual error. The sibling provider in this batch already handles this: `IdentityAssertionOAuthProvider` decodes with `.decode(errors="replace")`. Use `errors="replace"` here too (or `decode('utf-8', errors='replace')`) so the token-exchange error is reported, not a byte-decoding crash.</comment>
<file context>
@@ -0,0 +1,793 @@
+ """Handle token exchange response."""
+ if response.status_code not in {200, 201}:
+ body = await response.aread()
+ body_text = body.decode("utf-8")
+ raise OAuthTokenError(
+ f"Token exchange failed ({response.status_code}){redirect_note(response)}: {body_text}"
</file context>
| body_text = body.decode("utf-8") | |
| body_text = body.decode("utf-8", errors="replace") |
| uv run --isolated --no-project --with ./src/mcp-types python -c \ | ||
| "import mcp_types, mcp_types.jsonrpc, mcp_types.methods, mcp_types.version, mcp_types._v2025_11_25, mcp_types._v2026_07_28" | ||
|
|
||
| packages: |
There was a problem hiding this comment.
I re-reviewed after the follow-up commit (Decouple standalone client typing from the full SDK) and found no new bugs, but given this is a ~14k-line package split touching the OAuth client and every transport, a maintainer's read is still worthwhile.
What was reviewed in the follow-up:
- The new
__mcp_client_connect__hook onServer/MCPServerand the removal of_connect_inprocfrommcp_client— legacy vs. modern paths match the previous in-process behaviour. - Import graph for the moved
mcp.client._memory(it importsmcp.serverat module load;mcp.serveronly importsmcp.clientlazily inside the hook) — no cycle. scripts/check_client_types.pyand its CI step — the pyright assertion targets theClient(42)line and the isolated env excludesmcp.
Extended reasoning...
The PR moves the client, transports, OAuth (client_credentials, identity_assertion, oauth2, utils), and shared protocol machinery into a new mcp-client workspace package, leaving thin re-export shims under mcp/, and updates packaging, CI publishing, and docs. The auth code is relocated rather than rewritten, but it is a security-sensitive surface and the diff is 136 files. The follow-up commit replaced the runtime import of mcp.server inside Client.post_init with a protocol hook implemented on Server and MCPServer, which I checked for correctness and import cycles. No new findings this run; my earlier inline threads from the first push remain open and are not restated here. Size, auth surface, and the pending release/publisher setup are why a human look is still warranted rather than an approval.
Still open from earlier reviews (3):
- Unresolved: 3 minor or pre-existing.
This branch has not been deployed
1 participant
Summary
Extract the clients, transports, OAuth support, and shared protocol machinery into
mcp-client, imported asmcp_client, without the HTTP server dependencies. Keep existingmcpimports as permanent aliases and preserveClient(server)when the full SDK is installed.Release
mcp,mcp-client, andmcp-typesin lockstep with exact dependency pins. Update workspace configuration, publishing, documentation, and isolated wheel/sdist checks.Validation
strict-no-coverand all pre-commit checks pass.mcpor its server dependencies. All three wheels have matching versions and no overlapping files.Before release
Verify ownership of the existing
mcp-clientPyPI project and configure its trusted publisher for this repository'spublish-pypi.ymlworkflow andreleaseenvironment.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.