Pluggable Transport Abstractions for Client and Shared sessions#2117
Pluggable Transport Abstractions for Client and Shared sessions#2117sreenithi wants to merge 19 commits intomodelcontextprotocol:mainfrom
Conversation
- Remove __init__ and WireMessageT from AbstractBaseSession, making it a pure abstract interface without state management. BaseSession now owns all state. - Convert BaseClientSession from ABC to @runtime_checkable Protocol with structural subtyping. This eliminates the diamond inheritance in ClientSession and removes the need for inheritance to satisfy the interface. - Add missing methods to BaseClientSession protocol: complete(), set_logging_level(), and explicitly declare send_request(), send_notification(), send_progress_notification(). - Remove commented-out import in context.py. - Add comprehensive tests for BaseClientSession protocol satisfaction and E2E usage. - Document all breaking changes in docs/migration.md including: * ClientRequestContext type change to BaseClientSession * Generic callback protocols (SamplingFnT[ClientSession], etc.) * SessionT renamed to SessionT_co * AbstractBaseSession simplification * BaseClientSession Protocol conversion Fixes structural issues identified in PR review: - AbstractBaseSession no longer manages state it doesn't own - Eliminates double initialization of _response_streams and _task_group - Removes Any leakage for WireMessageT - BaseClientSession now earns its place as a pure interface
…com/kziemski/mcp-python-sdk-sreenithi into kziemski-transport-abstractions-review-fixes
…ures - Added type hints to methods in StubClientSession for better clarity and type safety. - Updated the test for BaseClientSession to assert the type of content returned. - Improved method signatures to include specific types for parameters and return values, enhancing code readability and maintainability.
|
How do we even use this abstraction? On the client side, it needs to be able to be used by the |
The idea was to introduce the abstraction in the Sessions ( And for the existing flow for clients using read/write streams, while the If we want to make the |
|
@Kludex I looked into this, and because However, because the new callback protocols ( I've added two new examples showing this:
Please let me know if this makes sense, or if you had a different expectation for how |
There was a problem hiding this comment.
~~Hi @sreenithi thanks for submitting this! Just to clarify, is your goal here to:
1. Allow creation of new "pipes" through which JSON-RPC can be passed? E.g. we have Stdio and SHTTP currently, but you want to be able to build a Websocket pipe or gRPC pipe (while still sending JSON-RPC bytes through the wire).
2. Allow custom transports to use some message format other than JSON-RPC, say gRPC or Thrift directly?
My read of the blog post is that we want to allow 1, but not 2 (I could be wrong though). This PR seems to attempt implementation of 2, but I'm not sure hence double checking. The "Custom Transports" section of the spec also suggests JSON-RPC should be preserved even in custom-transports:
> Implementers who choose to support custom transports MUST ensure they preserve the JSON-RPC message format and lifecycle requirements defined by MCP.
Source: https://modelcontextprotocol.io/specification/draft/basic/transports#custom-transports
Edit: having looked through Discord it does look like 2 is what we're trying to achieve here. That does raise the question though of how this can be reflected in the spec, because how do we ensure that say gRPC conforms to the same MCP message types?
|
After taking a look through this PR I think there are better options for adding pluggable transports than the approach here. Currently this just turns The layers I see in the SDK:
Currently, Session Logic, Dispatching, and Wire Format are all inside the same Now, if the goal of pluggable transports as currently written in the SDK was to keep JSONRPC and only change the transport, then there's no need to modify any of the But, that's obviously pretty jank, you'd then have to smuggle JSONRPC through gRPC or w/e. So we're wanting to do both, we're wanting to be able to swap out both the Wire Format and the Transport. To do that I think the best path forward is to actually separate out those three layers (Session logic, Dispatching, and Wire Format), allowing you to replace the dispatching/wire format easily, without having to reimplement the entire SDK to do so. I got Claude to draft up a PR here if you want to take a look at what I mean: #2320 (note this is definitely not ready to merge, more of a PoC to demonstrate the idea) |
| "@overload", | ||
| "raise NotImplementedError", | ||
| "^\\s*\\.\\.\\.\\s*$", | ||
| "\\.\\.\\.", |
There was a problem hiding this comment.
🔴 This unanchored pattern matches ... anywhere in a line — coverage.py uses re.search() for exclude_lines, so it will silently exclude executable lines like fn: Callable[..., Any] = Field(exclude=True) (tools/base.py:26), tuple[int, ...] annotations, and any string literal containing an ellipsis. It also makes the anchored pattern on the line above completely redundant.
To match the inline Protocol stubs in base_client_session.py (e.g. ) -> Any: ...), use an end-anchored pattern instead: ":\\s*\\.\\.\\.\\s*(#.*)?$".
Extended reasoning...
What the bug is
The TOML string "\\.\\.\\." decodes to the regex \.\.\., which matches three literal consecutive periods. Because coverage.py applies exclude_lines patterns using re.search() (not re.fullmatch()), this pattern matches anywhere in a line — not just lines that consist solely of ....
This is a strict superset of the existing pattern one line above ("^\\s*\\.\\.\\.\\s*$" → ^\s*\.\.\.\s*$), which correctly anchors to match only bare-ellipsis lines. The fact that the new pattern makes the old one dead code is itself a signal that the new pattern is broader than intended.
How it would be triggered
Any source line under src/ or tests/ (per [tool.coverage.run].source) that contains three consecutive dots — in any context — will be marked as excluded from coverage. No action is required to trigger it; the exclusion applies automatically on every coverage run.
Concretely, these patterns all match:
Callable[..., Any]— variadic callable type hintstuple[int, ...]— variadic tuple type hintsarr[..., 0]— NumPy-style ellipsis indexingraise ValueError("expected foo... got bar")— string literalslogger.info("Processing...")— log messages# see the docs for more...— comments
Impact and blast radius
Verified real impact: src/mcp/server/mcpserver/tools/base.py:26 contains:
fn: Callable[..., Any] = Field(exclude=True)This is executable code — a class-body assignment with a Field() call — and it will be silently dropped from the coverage report.
This repo enforces fail_under = 100 with branch = true and uses strict-no-cover to police pragma usage (per pyproject.toml and CLAUDE.md). This pattern is effectively a permanent, invisible # pragma: no cover applied to every line containing ..., which defeats that gate. Future code that should be covered can silently slip through, and the escape hatch won't be visible to reviewers because there's no pragma comment in the source.
Why it was added (and why it's too broad)
The new BaseClientSession Protocol in src/mcp/client/base_client_session.py uses inline stub bodies:
async def send_request(
self,
...
) -> Any: ...The existing ^\s*\.\.\.\s*$ pattern only matches standalone ... lines, so it misses ) -> Any: .... Commit 609778f ("resolve coverage failures") added this pattern to make those lines pass — but the fix didn't anchor.
Step-by-step proof
- TOML decodes
"\\.\\.\\."→ Python string\.\.\.(each\\→\). - As a regex,
\.\.\.means "three literal dots" (each\.escapes a single.). re.search(r"\.\.\.", "fn: Callable[..., Any] = Field(exclude=True)")→<re.Match object; span=(16, 19), match='...'>— matches.- Coverage.py's
exclude_linesmechanism usesre.search, so any match anywhere excludes the whole line. - The line is dropped from coverage reporting despite being executable and unrelated to Protocol stubs.
Suggested fix
Replace the new pattern with one anchored to : ... at end-of-line (optionally followed by a comment):
exclude_lines = [
"pragma: no cover",
"pragma: lax no cover",
"if TYPE_CHECKING:",
"@overload",
"raise NotImplementedError",
"^\\s*\\.\\.\\.\\s*$",
- "\\.\\.\\.",
+ ":\\s*\\.\\.\\.\\s*(#.*)?$",
]This matches ) -> Any: ..., ) -> None: ... # comment, and def foo(): ... while leaving Callable[..., Any], tuple[int, ...], and string literals covered.
On the refutation
One verifier noted bug_002 is a duplicate of bug_001. That's correct — they describe the same line and the same mechanism, which is why synthesis merged them. The refutation is procedural (deduplication), not a claim that the bug is invalid.
|
|
||
| # 1. Initialize | ||
| init_result = await session.initialize() | ||
| print(f"Connected to: {init_result.serverInfo.name}@{init_result.serverInfo.version}") |
There was a problem hiding this comment.
🔴 This crashes at runtime with AttributeError: 'InitializeResult' object has no attribute 'serverInfo' — must be init_result.server_info. MCPModel's populate_by_name=True allows construction via the camelCase alias (so line 27's serverInfo=... works), but Pydantic never creates attributes for aliases; attribute access is always via the Python field name.
CI didn't catch this because examples/pluggable_transport/ isn't in pyproject.toml's [tool.pyright].include list (only examples/servers, examples/snippets, examples/clients are). Adding it would also flag two smaller issues in client_example.py: result.content.text at L30 needs isinstance(..., TextContent) narrowing (content is a union — the same file does it correctly at L60), and the import from private mcp.shared._context at L10 should be from mcp.client import ClientRequestContext (which is literally RequestContext[BaseClientSession], exactly what L38 uses).
Extended reasoning...
The bug
examples/pluggable_transport/custom_transport_client.py:117 accesses init_result.serverInfo, but InitializeResult has no such attribute — the Python field name is server_info (defined at src/mcp/types/_types.py:569). Running this example raises AttributeError on the very first line after initialize() returns.
This is one of only two examples demonstrating the PR's headline feature (pluggable transport abstractions), and it crashes on first run.
Why the confusion exists
MCPModel is configured at src/mcp/types/_types.py:42 with:
model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)This creates an asymmetry that's easy to miss:
- Construction accepts both the Python name (
server_info=...) and the camelCase alias (serverInfo=...). That's whatpopulate_by_name=Truedoes. - Attribute access is only via the Python name. Pydantic v2 has no mechanism to expose aliases as attributes — aliases exist solely for serialization and parsing.
So when line 27 writes serverInfo=types.Implementation(...) inside the InitializeResult(...) constructor, Pydantic happily accepts it, looks up the field by alias, and stores it under server_info. But when line 117 reads init_result.serverInfo, Python's normal attribute lookup runs — there's no serverInfo on the instance or class, so it raises.
For confirmation, this PR's own src/mcp/client/session_group.py correctly uses result.server_info when handling an InitializeResult.
Why CI passed
The PR description says "pyright passes" — and it does, but only because examples/pluggable_transport/ isn't in the type-checked surface. pyproject.toml's [tool.pyright].include list contains examples/servers, examples/snippets, and examples/clients, but not the new directory. Pyright would have flagged serverInfo immediately as an unknown attribute under strict mode.
Step-by-step proof
- User runs
python examples/pluggable_transport/custom_transport_client.py main()callsinteract_with_mcp(custom_session)(line 137)- Line 116:
init_result = await session.initialize()→ returnsInitializeResult(protocol_version='2024-11-05', capabilities=..., server_info=Implementation(name='CustomDirectServer', version='1.0.0')). Note the stored field isserver_info, even though it was constructed withserverInfo=at line 27. - Line 117:
init_result.serverInfo→ Python looks forserverInfoin__dict__, then on the class, finds nothing →AttributeError: 'InitializeResult' object has no attribute 'serverInfo' - Example crashes before listing tools or calling anything.
Two smaller issues in client_example.py (would also be caught by pyright)
Once the directory is added to pyright's include list, it will also flag:
L30 — result.content.text without narrowing. CreateMessageResult.content is TextContent | ImageContent | AudioContent; only TextContent has .text. This won't crash in this self-contained example (the callback at L45 always returns TextContent), but the same file correctly does isinstance(content, TextContent) at L60 — the internal inconsistency is what makes it a bad teaching example.
L10 — from mcp.shared._context import RequestContext imports from a private module (underscore prefix). The example then uses RequestContext[BaseClientSession] at L38 — but that is exactly what ClientRequestContext is (defined in this PR at src/mcp/client/context.py:6 and exported from mcp.client.__all__). The PR's own migration guide tells users to use ClientRequestContext; the example should too.
Suggested fix
--- a/examples/pluggable_transport/custom_transport_client.py
+++ b/examples/pluggable_transport/custom_transport_client.py
@@ -114,7 +114,7 @@ async def interact_with_mcp(session: BaseClientSession) -> None:
# 1. Initialize
init_result = await session.initialize()
- print(f"Connected to: {init_result.serverInfo.name}@{init_result.serverInfo.version}")
+ print(f"Connected to: {init_result.server_info.name}@{init_result.server_info.version}")
--- a/examples/pluggable_transport/client_example.py
+++ b/examples/pluggable_transport/client_example.py
@@ -7,9 +7,8 @@ import asyncio
from mcp import Client
-from mcp.client.base_client_session import BaseClientSession
+from mcp.client import ClientRequestContext
from mcp.server.mcpserver import MCPServer
-from mcp.shared._context import RequestContext
from mcp.types import (
CreateMessageRequestParams,
CreateMessageResult,
@@ -27,7 +26,9 @@ async def main():
result = await server.get_context().session.create_message(
messages=[{"role": "user", "content": {"type": "text", "text": message}}],
max_tokens=100,
)
- return f"Assistant replied: {result.content.text}"
+ if isinstance(result.content, TextContent):
+ return f"Assistant replied: {result.content.text}"
+ return "Assistant replied with non-text content"
@@ -35,7 +36,7 @@ async def main():
async def abstract_sampling_callback(
- context: RequestContext[BaseClientSession], params: CreateMessageRequestParams
+ context: ClientRequestContext, params: CreateMessageRequestParams
) -> CreateMessageResult:
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -106,6 +106,7 @@ include = [
"examples/servers",
"examples/snippets",
"examples/clients",
+ "examples/pluggable_transport",
]Alternatively, move both files under examples/clients/ which is already type-checked.
| **Before:** | ||
|
|
||
| ```python | ||
| SessionT_co = TypeVar("SessionT_co", bound="AbstractBaseSession[Any, Any]", covariant=True) |
There was a problem hiding this comment.
🟡 The first line of this "Before" block shows the new code — it's identical to line 939 in the "After" block. AbstractBaseSession didn't exist before this PR, so it can't appear in a "Before" example.
The actual pre-PR definition (from src/mcp/shared/_context.py) was:
SessionT = TypeVar("SessionT", bound=BaseSession[Any, Any, Any, Any, Any])Lines 931-933 are correct; only this line needs fixing.
Extended reasoning...
What the bug is
In docs/migration.md, the "Internal TypeVars Renamed and Variance Updated" section provides a Before/After comparison to help users understand the TypeVar renames. However, the first line of the "Before" code block (line 930) is identical to the first line of the "After" code block (line 939):
# Line 930 (labeled "Before:")
SessionT_co = TypeVar("SessionT_co", bound="AbstractBaseSession[Any, Any]", covariant=True)
# Line 939 (labeled "After:")
SessionT_co = TypeVar("SessionT_co", bound="AbstractBaseSession[Any, Any]", covariant=True)This is a copy-paste error. The other three lines in the "Before" block (SendRequestT, SendNotificationT, ReceiveResultT) correctly show the old definitions — only the SessionT line is wrong.
Why it's definitely wrong
The "Before" block references AbstractBaseSession, but that class is introduced by this very PR — it does not exist in the pre-PR codebase. The diff of src/mcp/shared/_context.py shows the actual change:
-from mcp.shared.session import BaseSession
+if TYPE_CHECKING:
+ from mcp.shared.session import AbstractBaseSession
...
-SessionT = TypeVar("SessionT", bound=BaseSession[Any, Any, Any, Any, Any])
+SessionT_co = TypeVar("SessionT_co", bound="AbstractBaseSession[Any, Any]", covariant=True)Additionally, the table directly above the code block (line 922) correctly states SessionT → SessionT_co, which contradicts the code example showing SessionT_co on both sides.
Step-by-step proof
- A user reads the table at line 922: "Original Name:
SessionT→ Updated Name:SessionT_co" - They scroll down to the "Before:" code block to see the old definition
- Line 930 shows
SessionT_cowithcovariant=Trueandbound="AbstractBaseSession[Any, Any]" - They scroll to "After:" — line 939 is identical
- The user is now confused: the table says the name changed, but the code shows the same thing on both sides. They also have no way to see that the bound changed from
BaseSession[Any, Any, Any, Any, Any]toAbstractBaseSession[Any, Any], or that variance was added.
Impact
This is a documentation error in a migration guide for a breaking change. Users migrating code that referenced SessionT cannot use this section to understand:
- What the old name was (
SessionT, notSessionT_co) - What the old bound was (
BaseSession[Any, Any, Any, Any, Any], notAbstractBaseSession[Any, Any]) - That variance was added (there was no
covariant=Truebefore)
The impact is partially mitigated because the table at line 922 correctly documents the rename, so careful readers won't be completely lost — but the code example directly contradicts it, which is confusing.
Suggested fix
**Before:**
```python
-SessionT_co = TypeVar("SessionT_co", bound="AbstractBaseSession[Any, Any]", covariant=True)
+SessionT = TypeVar("SessionT", bound=BaseSession[Any, Any, Any, Any, Any])
SendRequestT = TypeVar("SendRequestT", ClientRequest, ServerRequest)
SendNotificationT = TypeVar("SendNotificationT", ClientNotification, ServerNotification)
ReceiveResultT = TypeVar("ReceiveResultT", bound=BaseModel)
</details>
<!-- bughunter:bug_ids=bug_005 -->
| ) | ||
| ReceiveRequestT = TypeVar("ReceiveRequestT", ClientRequest, ServerRequest) | ||
| ReceiveResultT = TypeVar("ReceiveResultT", bound=BaseModel) | ||
| ReceiveResultT_co = TypeVar("ReceiveResultT_co", bound=BaseModel, covariant=True) |
There was a problem hiding this comment.
🟡 covariant=True is meaningless here — ReceiveResultT_co is not in any class's Generic[...] list (neither AbstractBaseSession nor BaseSession includes it). It's only used method-scoped in send_request(result_type: type[T]) -> T, where each call binds independently and there's no subtyping relation for variance to govern.
The other renames (SendRequestT_contra, SendNotificationT_contra) are in AbstractBaseSession's Generic[...] so their variance is meaningful — this one was likely renamed by pattern-matching. Suggest reverting to ReceiveResultT (no variance, no _co suffix) and dropping the migration.md table row that documents "Covariant" semantics that don't exist.
Extended reasoning...
What the bug is
The PR renames ReceiveResultT → ReceiveResultT_co and adds covariant=True:
# src/mcp/shared/session.py:44
ReceiveResultT_co = TypeVar("ReceiveResultT_co", bound=BaseModel, covariant=True)However, ReceiveResultT_co never appears in any class's Generic[...] parameter list. Checking both Generic declarations in the file:
AbstractBaseSession(Protocol, Generic[SendRequestT_contra, SendNotificationT_contra])— 2 params,ReceiveResultT_coabsentBaseSession(..., Generic[SendRequestT_contra, SendNotificationT_contra, SendResultT, ReceiveRequestT, ReceiveNotificationT])— 5 params,ReceiveResultT_coabsent
It only appears inside the send_request method signature (in both AbstractBaseSession and BaseSession):
async def send_request(
self,
request: SendRequestT_contra,
result_type: type[ReceiveResultT_co], # method-local binding
...
) -> ReceiveResultT_co:Per PEP 484, variance annotations (covariant=True / contravariant=True) only have meaning on TypeVars that parameterize a generic class. Variance governs how Foo[Subtype] relates to Foo[Supertype] — it's a property of the class under subtyping. A TypeVar that's only used inside a single method signature is function-scoped: each call to send_request binds it independently, and there is no inter-call subtyping relation for variance to govern. The covariant=True here is a semantic no-op.
Why this happened — contrast with the correct renames
The same diff hunk renames three TypeVars together, but they are not equivalent:
| TypeVar | In a Generic[...] list? |
Variance meaningful? |
|---|---|---|
SendRequestT_contra |
✅ AbstractBaseSession[SendRequestT_contra, ...] |
✅ Yes — governs Protocol subtyping |
SendNotificationT_contra |
✅ AbstractBaseSession[..., SendNotificationT_contra] |
✅ Yes — governs Protocol subtyping |
ReceiveResultT_co |
❌ Method-scoped only | ❌ No — no subtyping relation to govern |
The first two renames are correct and necessary — contravariance on input-position TypeVars is what makes the Protocol's structural subtyping sound. ReceiveResultT was renamed by pattern-matching without recognizing that it's fundamentally different: it's a per-method generic (like the T in list.index(x: T)), not a per-class generic.
Step-by-step proof
Consider what covariance would mean if it applied here:
ReceiveResultT_cois covariant → for some genericG,G[Child]should be a subtype ofG[Parent]- But there is no such
G—ReceiveResultT_codoesn't parameterize any class - The only binding site is
send_request. Call 1:send_request(req, CallToolResult)bindsT = CallToolResult. Call 2:send_request(req, ListToolsResult)bindsT = ListToolsResult. - These two calls don't have a subtyping relationship with each other — they're independent invocations. There's no "
send_request[Child]is-asend_request[Parent]" relation for covariance to describe. - Type checkers (pyright, mypy) recognize this and silently ignore the variance annotation.
Concretely: if you delete covariant=True from line 44, zero observable behavior changes — not at runtime, not in pyright, not in mypy. The annotation is dead code.
Impact
- No runtime impact —
TypeVarvariance is erased at runtime regardless. - No CI failure — pyright silently ignores variance on function-scoped TypeVars (which is why CI passes).
- No mypy hard error (correcting the original finding's claim) — mypy's
Cannot use a covariant type variable as a parametererror fires for class-scoped covariant TypeVars used in input positions. For purely function-scoped TypeVars, mypy also ignores the variance as a no-op. This is a latent correctness issue, not a build break. - Misleading documentation — the migration.md table row ("
ReceiveResultT→ReceiveResultT_co| Covariant | Handles successful response models safely") documents semantics that don't exist. In a PR whose explicit purpose is to get Protocol variance right, shipping a meaningless variance annotation plus a docs row claiming it "handles response models safely" is the kind of thing that misleads future contributors. - Misleading naming — the
_cosuffix (per PEP 484 convention) promises covariance to anyone reading the code.
Suggested fix
Revert the rename and drop the annotation (three-line logical change, ~6 physical lines):
--- a/src/mcp/shared/session.py
+++ b/src/mcp/shared/session.py
@@ -44 +44 @@
-ReceiveResultT_co = TypeVar("ReceiveResultT_co", bound=BaseModel, covariant=True)
+ReceiveResultT = TypeVar("ReceiveResultT", bound=BaseModel)Then s/ReceiveResultT_co/ReceiveResultT/ at the four usage sites in send_request signatures (two in AbstractBaseSession, two in BaseSession), and remove the ReceiveResultT row from the migration.md TypeVar table.
| """Request context for MCP client handlers.""" | ||
|
|
||
| from mcp.client.session import ClientSession | ||
| from mcp.client import BaseClientSession |
There was a problem hiding this comment.
🟡 This imports BaseClientSession from the mcp.client package rather than directly from the module, creating a circular dependency on __init__.py import ordering — it only works because base_client_session is imported on line 4 before context on line 6.
Ruff's isort keeps this safe alphabetically so it won't break accidentally, but it's inconsistent with session.py and task_handlers.py in this same PR, which both use from mcp.client.base_client_session import BaseClientSession. The pre-PR code also used a direct module import here. Suggest matching the other files.
Extended reasoning...
What the bug is
src/mcp/client/context.py:3 imports BaseClientSession from the mcp.client package namespace rather than directly from the mcp.client.base_client_session submodule:
from mcp.client import BaseClientSessionThis creates a circular import dependency on the statement ordering inside mcp/client/__init__.py. Both session.py and task_handlers.py in this same PR use the direct-module pattern instead — only context.py deviates. The pre-PR code also used a direct module import (from mcp.client.session import ClientSession).
The code path that triggers it
Walking through what happens when mcp.client is first imported:
- Python begins executing
mcp/client/__init__.py. Themcp.clientmodule object is created and added tosys.modules, but its namespace is empty. - Line 3 —
from mcp.client._transport import Transportexecutes;Transportis bound in the partialmcp.clientnamespace. - Line 4 —
from mcp.client.base_client_session import BaseClientSessionexecutes;BaseClientSessionis now bound in the partialmcp.clientnamespace. - Line 5 —
from mcp.client.client import Clientexecutes. - Line 6 —
from mcp.client.context import ClientRequestContextbegins. Python loadscontext.py. - Inside
context.py, line 3 runs:from mcp.client import BaseClientSession. Python findsmcp.clientalready insys.modules(partially initialized) and looks upBaseClientSessionin its namespace. This succeeds only because step 3 already bound it. context.pyfinishes loading,__init__.pyresumes at line 7.
If line 6 of __init__.py were ever moved above line 4, step 6 would fail with ImportError: cannot import name 'BaseClientSession' from partially initialized module 'mcp.client' (most likely due to a circular import).
Impact and blast radius
Low practical risk. The original finding's threat model ("a linter reorders them") is somewhat overstated: pyproject.toml enables ruff's I (isort) rule, which enforces alphabetical ordering within first-party import groups. Since base_client_session sorts before context alphabetically, the linter actively protects the current ordering rather than threatening it. The failure mode would require a manual edit that bypasses pre-commit hooks.
The real cost is inconsistency. This is the only file in the PR that imports BaseClientSession via the package. Compare:
| File | Import pattern |
|---|---|
session.py:11 |
from mcp.client.base_client_session import BaseClientSession, ClientSessionT_contra |
task_handlers.py:22 |
from mcp.client.base_client_session import BaseClientSession, ClientSessionT_contra |
session_group.py:23 |
from mcp.client.base_client_session import ClientSessionT_contra |
context.py:3 |
from mcp.client import BaseClientSession ← deviates |
The pre-PR context.py used from mcp.client.session import ClientSession (direct module import), so the package-level import is a new pattern introduced by this change, not pre-existing.
Suggested fix
-from mcp.client import BaseClientSession
+from mcp.client.base_client_session import BaseClientSessionThis matches every other file in the PR, removes the ordering dependency entirely, and restores the direct-module-import pattern the pre-PR code used. Zero behavioral change.
Addressing verifier objections
The single refutation noted that bug_009 duplicates bug_004 (same file, line, issue, fix). This is correct — the synthesis agent merged them into this single report. It is not a refutation of the finding itself.
| async def send_request( | ||
| self, | ||
| request: types.ClientRequest, | ||
| result_type: type, | ||
| request_read_timeout_seconds: float | None = None, | ||
| metadata: Any = None, | ||
| progress_callback: ProgressFnT | None = None, | ||
| ) -> Any: ... | ||
|
|
||
| async def send_notification( | ||
| self, | ||
| notification: types.ClientNotification, | ||
| related_request_id: Any = None, | ||
| ) -> None: ... |
There was a problem hiding this comment.
🟡 The comment on line 21 says these methods are "from AbstractBaseSession", but the redeclarations drop type information that AbstractBaseSession correctly carries:
send_requestusesresult_type: type→Anyinstead oftype[ReceiveResultT_co]→ReceiveResultT_co, so code typed againstBaseClientSessiongetsAnyback and loses all result type safetysend_notificationusesrelated_request_id: Anyinstead ofRequestId | None(whereRequestId = str | int)
AbstractBaseSession proves the method-scoped-TypeVar pattern works in a @runtime_checkable Protocol (ReceiveResultT_co isn't in its Generic[...] params), and RequestId is already importable from the same module as ProgressFnT on line 6. Also — the comment on line 88 (# Missing methods added per review) is a PR-review-cycle artifact that should be deleted.
Extended reasoning...
What the bug is
BaseClientSession is a new @runtime_checkable Protocol introduced so users can write code typed against an abstract client session rather than the concrete ClientSession. The comment at base_client_session.py:21 states these methods are redeclared from AbstractBaseSession — but the redeclarations are weaker than the source they're mirroring:
| Parameter | BaseClientSession (this file) |
AbstractBaseSession (session.py:178–198) |
|---|---|---|
send_request → result_type |
type |
type[ReceiveResultT_co] |
send_request → return |
Any |
ReceiveResultT_co |
send_request → metadata |
Any |
MessageMetadata |
send_notification → related_request_id |
Any |
RequestId | None |
There is no technical barrier here. AbstractBaseSession is itself a @runtime_checkable Protocol, and ReceiveResultT_co is not in its Generic[SendRequestT_contra, SendNotificationT_contra] params — it functions as a method-scoped TypeVar. So this same PR has already demonstrated the pattern works.
Separately, line 88 contains # Missing methods added per review, a leftover from commit d0b11ad's review cycle. Contrast with the other section headers in this file (# Methods from AbstractBaseSession..., # Client-specific methods) which describe what the methods are — this one only describes when they were added during development.
How this gets triggered
Any code that accepts a BaseClientSession (which is the entire point of introducing this Protocol) and calls send_request directly hits the Any hole. The PR's own example, custom_transport_client.py, encourages writing functions typed as async def interact_with_mcp(session: BaseClientSession). The moment someone calls session.send_request(...) inside such a function, the contagion starts.
Custom implementers are the second audience: when someone writes a BaseClientSession implementation, pyright will accept any return type for send_request and any argument type for related_request_id, because Any matches everything.
Impact and blast radius
This causes silent type erasure, not runtime failures. Two concrete harms:
- Callers lose result typing.
await session.send_request(req, types.CallToolResult)returnsAny, so every downstream.content,.is_error, etc. is unchecked.Anyis contagious — it propagates through the caller's code silently. - Implementers get no guidance. Someone implementing
BaseClientSessionin their own transport won't be told by pyright thatsend_requestmust return an instance ofresult_type, or thatrelated_request_idmust bestr | int | None.
The blast radius is bounded by the fact that most users will call the properly-typed high-level methods (call_tool() -> CallToolResult, list_tools() -> ListToolsResult, etc.), which are correct in this Protocol. send_request is the low-level escape hatch. But since this is a brand-new public API, tightening it later would technically be a breaking change — better to ship it right.
Step-by-step proof
Walk through what pyright sees when code is typed against BaseClientSession:
from mcp.client import BaseClientSession
from mcp import types
async def fetch_tools(session: BaseClientSession) -> list[types.Tool]:
# session is BaseClientSession, so pyright uses the Protocol's signature:
# async def send_request(self, request, result_type: type, ...) -> Any
result = await session.send_request(
types.ListToolsRequest(params=None),
types.ListToolsResult, # accepted: `type` matches anything
)
# reveal_type(result) → Any ← relationship to ListToolsResult LOST
return result.tooools # typo! pyright: no error, Any.anything is AnyNow the same code against AbstractBaseSession (same PR, session.py:178–185):
# signature: result_type: type[ReceiveResultT_co] -> ReceiveResultT_co
# pyright binds ReceiveResultT_co = ListToolsResult
# reveal_type(result) → ListToolsResult
# result.tooools → error: "tooools" is not a known attribute of "ListToolsResult"And for related_request_id:
async def notify(session: BaseClientSession) -> None:
await session.send_notification(
types.InitializedNotification(),
related_request_id={"oops": "a dict"}, # Any accepts this silently
)
# With RequestId | None: error — dict is not assignable to str | int | NoneSuggested fix
No import barriers exist: line 6 already imports ProgressFnT from mcp.shared.session, and RequestId lives in the same module.
--- a/src/mcp/client/base_client_session.py
+++ b/src/mcp/client/base_client_session.py
@@ -1,11 +1,14 @@
from typing import Any, TypeVar
+from pydantic import BaseModel
from typing_extensions import Protocol, runtime_checkable
from mcp import types
-from mcp.shared.session import ProgressFnT
+from mcp.shared.session import ProgressFnT, RequestId
from mcp.types._types import RequestParamsMeta
+_ResultT = TypeVar("_ResultT", bound=BaseModel)
+
ClientSessionT_contra = TypeVar("ClientSessionT_contra", bound="BaseClientSession", contravariant=True)
@@ -22,16 +25,16 @@ class BaseClientSession(Protocol):
async def send_request(
self,
request: types.ClientRequest,
- result_type: type,
+ result_type: type[_ResultT],
request_read_timeout_seconds: float | None = None,
metadata: Any = None,
progress_callback: ProgressFnT | None = None,
- ) -> Any: ...
+ ) -> _ResultT: ...
async def send_notification(
self,
notification: types.ClientNotification,
- related_request_id: Any = None,
+ related_request_id: RequestId | None = None,
) -> None: ...
@@ -85,7 +88,6 @@ class BaseClientSession(Protocol):
async def list_tools(self, *, params: types.PaginatedRequestParams | None = None) -> types.ListToolsResult: ...
- # Missing methods added per review
async def complete(Note on metadata: Any (line 27): AbstractBaseSession uses MessageMetadata here, but MessageMetadata is JSON-RPC-coupled (from mcp.shared.message), so keeping it Any may be an intentional transport-agnostic choice. related_request_id: Any has no such excuse — RequestId = str | int is a protocol-level MCP concept independent of transport. I've left metadata unchanged in the diff above; adjust if the Any was not intentional.
This PR introduces abstract Protocol classes above the existing classes to help with pluggable transport. Two new Protocol classes are introduced -
AbstractBaseSessionandBaseClientSession.AbstractBaseSession
AbstractBaseSessionis a class above the existingBaseSessionclass in hierarchy. It is a structural protocol with no implementation state or__init__method and just defines the contract that all session implementations must satisfy, irrespective of the transport used. State management and response routing now becomes the responsibility of the concrete implementations of this class, such as the Base session.Hence the Generic types required for this Protocol are also limited to the ones needed by the methods in the Protocol -
SendRequestTandSendNotificationTBaseClientSession
BaseClientSessionis a new Protocol defining the interface for MCP client sessions specifiying all methods that a client session must implement irrespective of the transport used.To support these 2 new Protocols, the following types have been changed/introduced.
SessionThas changed to be a covariant type bound to theAbstractBaseSessionProtocol so that it can support both the existingBaseSessionand any newer classes that inherit fromAbstractBaseSessionClientSessionT_contrabound to theBaseClientSessionis introduced.SendRequestTandSendNotificationTare changed to contravariant and renamed with*_contrato support its use in theAbstractBaseSessionProtocolReceiveResultTis also changed to be a covariant type and renamed toReceiveResultT_coFinally class definitions that used
RequestContext[ClientSession]specifically such asSamplingFnT,ElicitationFnT,GetTaskHandlerFnT, etc. are now made to use theGeneric[ClientSessionT_contra]type to allow other pluggable transport classes to comply with theBaseClientSessionProtocol.Motivation and Context
This PR introduces abstract classes and types to make it easier for users writing their own pluggable transports. This PR is the result of a discussion with the MCP core maintainers in December, where it was agreed that we would add a pluggable transport abstraction to the MCP SDKs. The current implementations assumes the use of JSON-RPC, and this PR aims to introduce abstractions to keep it transport and message type agnostic.
How Has This Been Tested?
All checks mentioned in the contributing guide (
pytest,pyrightandruff) passes.Breaking Changes
Classes like
SamplingFnT,ElicitationFnT,GetTaskHandlerFnT, etc. have now been modified to use Generic types. So users, using these classes for type hints will need to mention the specific type likeElicitationFnT[ClientSession]Types of changes
Checklist