feat: mcp v2 compatible changes under a flag - #3708
Conversation
github-actions
Bot
added
area-mcp
|
@strandly-the-agent review the PR and tell me what's missing or to be improved. |
github-actions
Bot
added
complexity/low
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Issue (Important): The whole point of this layer is 2.x compatibility, but nothing in CI verifies it. With the pin at Suggestion: Add a lightweight CI job / nox session that installs |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if MCP_V2: | ||
| from mcp.client.streamable_http import create_mcp_http_client, streamable_http_client | ||
|
|
||
| return streamable_http_client(url=url, http_client=create_mcp_http_client(headers=headers)) |
There was a problem hiding this comment.
Issue (Suggestion / question): The 2.x branch builds an HTTPX client via create_mcp_http_client(headers=headers) and hands it to streamable_http_client(http_client=...). It's worth confirming who owns that client's lifecycle — if streamable_http_client doesn't enter/close the client as part of its own context manager, the pool could leak once this path is actually exercised.
Suggestion: A short note in the docstring (or a comment) stating that the transport takes ownership of and closes the client would document the assumption. Also worth double-checking create_mcp_http_client tolerates headers=None. Non-blocking since the pin stays <2 and this branch isn't reached yet.
There was a problem hiding this comment.
Closing the loop on this: @strandly-the-agent answered it definitively from the mcp==2.0.0 source — streamable_http_client sets client_provided = http_client is not None and only enters/closes the client in its AsyncExitStack when it created it, so the client built here is never aclose()d (one leaked connection pool per connect). And headers=None is the real 2.0.0 default, so no bug there. So the docstring "takes ownership" note I suggested would actually be documenting the wrong contract — the fix is for the adapter to own the client's lifecycle itself (e.g. wrap it so it's closed with the transport). Deferring to that thread.
There was a problem hiding this comment.
Resolved by 4666078. The adapter now wraps create_mcp_http_client(...) and streamable_http_client(...) in a single @asynccontextmanager (_owned_client_transport) that enters both together, so the caller-provided client's lifetime is bound to the transport's and it's aclose()d on exit — no more leaked pool on the 2.x path. The new test_streamable_http_transport_v2_owns_client_lifecycle guards it by asserting the full ordered lifecycle (client_enter → transport_enter → transport_exit → client_exit) in one equality check. Thanks for turning this around.
|
Assessment: Comment (approve-worthy with minor follow-ups) Clean, well-scoped groundwork PR. The single internal Review Categories
Nicely documented change — the PR description alone made the review straightforward. |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
Reviewed 8aed1e85 end to end. Short version: the code that ships is correct — but CI is red because of this PR, and the description claims more than the code delivers.
The useful thing I could do here that a normal review can't: mcp==2.0.0 is a real release and installable, so I built venvs on both lines and actually ran the 2.x branch instead of reasoning about it.
| Check | Result |
|---|---|
pytest tests/strands/tools/mcp/ on mcp 1.23.0 (pin floor) |
✅ 217 passed |
| Same suite on mcp 2.0.0 | 🔴 64 failed / 153 passed |
Python / Lint + CI Gate on this head |
🔴 failure (main is green) |
ruff check locally |
✅ All checks passed |
mypy ./src locally, mcp 1.23.0 |
🔴 5 errors, all in _compat.py — reproduces the CI failure |
| HTTPX leak on the 2.x branch | 🔴 confirmed by execution (aclose() never called) |
MCP_V2 probe, MCPError alias, transport names vs real 2.0.0 |
✅ all correct |
| OTel 1.x-only gating | ✅ real (connect wrapped on 1.23.0, untouched on 2.0.0) |
The one blocker is mundane: mypy runs inside hatch fmt --linter --check (pyproject.toml:139-149 → lint-check = ["ruff check", "mypy ./src"]), which hatch run prepare and the gating lint job both call. It's failing on _compat.py. That also means the checklist's "I ran hatch run prepare" can't be accurate for this commit.
The framing question is the one that matters (in Questions below): is this PR meant to make 2.x work, or only import so the pin can move later? The body says "imports and runs cleanly on both major lines" and "the pin can later widen to <3 with no code changes" — I measured both and neither holds today. If the answer is "import-only groundwork," the code is basically right and only the wording needs fixing. I've filed nothing; the 2.x functional gaps are pre-existing and listed below for you to decide on.
3 inline comments. Nothing here re-litigates the resolved threads on #3611.
Questions (the framing one is blocking)
Blocking
-
Is the goal "2.x works" or "2.x imports"? This decides how everything else is tiered. What ships makes
strands.tools.mcpimportable on 2.x and consolidates the version probe — both real and both correct. ButMCPClientis not functional on 2.x:start()raises unconditionally, ~15 camelCase field accesses break, andtasks_config=raisesImportErrorat construction. If that's intentional staging, could the description say so? As written the next reader will believe the pin can move. -
Merge order — this is stacked on #3611, which is still open, and includes its commit. GitHub's Files-changed tab merges both into one diff, so anyone approving from that tab is implicitly approving #3611's content without it clearing its own gate. Is bottom-up (land #3611 first, let this shrink to its own commit) the intent?
Non-blocking, but worth settling before more code branches on the pattern
-
One boolean is standing in for at least six independently-probeable axes (property-vs-method, tasks surface, transport takes-a-client, transport yields-session-id, field casing, task types importable) — I verified each discriminates on its own.
_compat.pyalready uses per-nametry/exceptforMCPError/GetSessionIdCallbackbut the global flag for the transport. Since the body says lifecycle/tasks/native-OTel will all branch on this same flag, would per-capability constants age better than one flag a future 3.x can't name correctly? (hasattr(ClientSession, "discover")reads as "≥2" whileMCP_V2reads as "==2".) -
MCPTransportis public (mcp_types.py:47, exported in__all__) and quietly gets a second meaning here — on 2.xGetSessionIdCallbackdegrades to a structuralCallable[[], str | None]and the 3-tuple arm is simply dead. Worth deciding whatMCPTransportshould mean on 2.x before more code depends on the current shape? Relatedly, the PR carries noapi/needs-review/api/review-completelabel —check-api-review-labelis green only because it skips when no label is present. -
Should HTTPX ownership be a written rule for future adapters here — the adapter owns what it creates, never what it's given — rather than a per-call-site decision?
Appendix — non-blocking (6)
- ⚪
_compat.py:17-19— the comment callsClientSession.discover"the 2.x replacement for the removed initialize handshake," buthasattr(ClientSession, "initialize")isTrueon both installed lines. The spec removed the handshake; the Python client hasn't. Wording only. - ⚪
_compat.py:28-35catches onlyImportErrorand itsexceptbranch doesn't re-import frommcp, so an unrelated import failure inmcp.client.streamable_httpgets swallowed and you get a fake alias plus a worse stack trace later. TheMCPErrorsibling at:22-26doesn't have this gap because itsexceptre-imports from the same module. No caller triggers it today. - ⚪ Anything that adds a
discoverattribute toClientSessionon a real 1.x (subclass, backport, test double) flipsMCP_V2and sends the transport down the 2.x path. Contrived — same root cause as Question 3. - ⚪
test_mcp_instrumentation.py:426assertsregister_post_import_hookis called, which the new gating correctly skips on 2.x, so the suite can't be green there. Inherited from #3611's commit, not this one. - ⚪ No CI leg installs
mcp>=2, so every 2.x branch is mock-only. Smallest thing that would work today and passes right now: a@pytest.mark.skipif(not MCP_V2, …)import-smoke test plus one single-Python job doingpip install "mcp>=2.0.0,<3.0.0" --no-depsand running justtest__compat.py. That alone would have caught the vacuous-mock problem in comment 3. - ⚪ Codecov reports 3 uncovered lines in
_compat.py— exactly the 2.x/except ImportErrorbranches that need a realmcp>=2install to execute.
Pre-existing — needs an issue, not a fix here (4)
I verified with git diff 898cc0ef 8aed1e85 that this PR touches none of these lines. They're unchanged from main and only become reachable if the pin widens — so they are not regressions here, and I'm not treating them as blockers. Flagging because they're what stands between this PR and its stated goal. Happy to file them if you want.
MCPClient.start()raises on 2.x —mcp_client.py:1063callssession.get_server_capabilities(); 2.x made it aserver_capabilitiesproperty. Unconditional bootstrap, every transport. Repro against a real in-process server:AttributeError: 'ClientSession' object has no attribute 'get_server_capabilities'→MCPClientInitializationError.- ~15 camelCase→snake_case field sites — worst is
mcp_client.py:1007.isError, where theAttributeErroris swallowed into an errorToolResult, so every successful 2.x tool call reports as failed with the rawAttributeErroras the tool's output to the model. Wrong-answer shaped, not crash shaped. Alsomcp_agent_tool.py:77,82,83andmcp_client.py:622,1015,1149,1179-1199. tasks_config=raises a bareImportErrorat construction on 2.x (mcp_client.py:299) and isn't aliasable —ClientSession.experimentaldoesn't exist on 2.0.0, so:1259,1541,1584,1625have no 2.x surface. Needs a design call (clear "unsupported on 2.x" error vs. port to the extension), and #1659 already defers tasks pending the tasks-extension GA.tests_integ/mcp/*and the separatestrands-mcppackage are 2.x-broken (FastMCP,mcp.server.experimental.task_context;strands-mcphas its own<2.0.0pin). Out of scope, worth tracking so it isn't rediscovered piecemeal.
Automated review by strandly-the-agent — ran on both mcp lines with real installs. Solid work for a human to approve, not a gate; push back where I'm wrong.
| MCP_V2: bool = hasattr(ClientSession, "discover") | ||
|
|
||
| try: | ||
| from mcp.shared.exceptions import MCPError |
There was a problem hiding this comment.
🔴 This is what's turning CI red, and it blocks merge regardless of the mcp pin.
Python / Lint and CI Gate are both failing on 8aed1e85 while main's equivalent job is green. mypy is wired into that job — pyproject.toml:203-207 prepare → test-lint → hatch fmt --linter --check → pyproject.toml:139-149 lint-check = ["ruff check", "mypy ./src"]. I reproduced it locally against mcp==1.23.0 (the floor, so what CI's env sees); ruff check passes, and the only mcp-related errors are these 5, all in this file:
_compat.py:23: error: Module "mcp.shared.exceptions" has no attribute "MCPError" [attr-defined]
_compat.py:26: error: Unused "type: ignore[attr-defined, no-redef]" comment [unused-ignore]
_compat.py:35: error: Unused "type: ignore[assignment]" comment [unused-ignore]
_compat.py:57: error: Module "mcp.client.streamable_http" has no attribute "streamable_http_client" [attr-defined]
_compat.py:59: error: Returning Any from function declared to return "AbstractAsyncContextManager[Any, bool | None]" [no-any-return]
The merge-base is clean, so all 5 are net-new. Root cause is structural rather than a typo: mypy only ever sees one installed mcp, so the non-matching branch of each try/except is a hard attr-defined with no covering ignore (:23 and :29 have none; the deferred imports at :57/:61 have none either), and warn_unused_ignores = true (pyproject.toml:220) then makes the ignores that do exist "unused" on the other line. I checked both directions — there is no single mypy invocation, on either supported major, where this file passes clean as written.
No suggestion block, because the two halves need different fixes and I'd rather not hand you a guess: the attr-defined/unused-ignore half wants per-branch # type: ignore[attr-defined] plus a per-module warn_unused_ignores = false override (there's precedent at pyproject.toml:227-233), while the no-any-return at :59 is fixed for free by giving the function a real return type — see my comment on the 2.x branch below.
| if MCP_V2: | ||
| from mcp.client.streamable_http import create_mcp_http_client, streamable_http_client | ||
|
|
||
| return streamable_http_client(url=url, http_client=create_mcp_http_client(headers=headers)) |
There was a problem hiding this comment.
🟡 The HTTPX client built here is never closed — one leaked connection pool per connect. This is a definitive answer to the open bot thread on this line, and its other question (headers=None) is fine: that's the real 2.0.0 default, no bug.
streamable_http_client deliberately does not take ownership of a client you hand it — from the installed mcp==2.0.0:
# mcp/client/streamable_http.py:664
client_provided = http_client is not None
...
async with contextlib.AsyncExitStack() as stack:
# Only manage client lifecycle if we created it
if not client_provided: # :677-678
await stack.enter_async_context(client)Since :59 always passes http_client=, client_provided is always True, so the client is never entered or closed — and _compat.py keeps no reference to close it itself. Verified by monkeypatching the lifecycle methods and driving a full async with: {'aenter': 0, 'aexit': 0, 'aclose': 0}, is_closed=False after exit, 50/50 clients still open across a run. There's no __del__ on httpx2.AsyncClient, so nothing reclaims it. create_mcp_http_client's own docstring says it must be used as a context manager. Note this is a genuine 1.x/2.x asymmetry — 1.x owns its client internally, so the adapter inherits a responsibility that didn't exist before.
Reachability: needs the pin relaxed and a "streamable-http" server entry (mcp_client.py:1726, via MCPClient.load_servers). 🟡 rather than 🔴 only because pyproject.toml:37 still pins <2.0.0 — it becomes 🔴 the day the pin widens, which is this series' stated goal.
What I verified works (plain block, not a suggestion, deliberately — see below):
if MCP_V2:
from mcp.client.streamable_http import create_mcp_http_client, streamable_http_client
@asynccontextmanager
async def _owned_transport() -> AsyncIterator[Any]:
async with (
create_mcp_http_client(headers=headers) as http_client,
streamable_http_client(url=url, http_client=http_client) as streams,
):
yield streams
return _owned_transport()with from collections.abc import AsyncIterator and asynccontextmanager added at the top. Confirmed: client now closes (is_closed=True), ruff clean, and it also clears the no-any-return mypy error from the comment above (5 errors → 4), since the function stops forwarding an untyped Any.
The reason it isn't a committable suggestion: any ownership fix necessarily wraps the transport, and test__compat.py:52 asserts result is mock_client.return_value — the raw transport object. I tried both a lazy and an eager variant and each fails that test. So the test as written would reject any correct fix here, which is why this needs a paired change rather than a one-click commit.
| with ( | ||
| patch.object(_compat, "MCP_V2", True), | ||
| patch("mcp.client.streamable_http.streamable_http_client", create=True) as mock_client, | ||
| patch("mcp.client.streamable_http.create_mcp_http_client", create=True, return_value=http_client) as mock_http, | ||
| ): | ||
| result = streamable_http_transport("https://example.com/mcp", headers=headers) | ||
|
|
||
| mock_http.assert_called_once_with(headers=headers) | ||
| mock_client.assert_called_once_with(url="https://example.com/mcp", http_client=http_client) | ||
| assert result is mock_client.return_value |
There was a problem hiding this comment.
🟡 These two branch tests verify the mock rather than the contract, so "4 passed" doesn't mean the 2.x branch works.
create=True on both patches (:45, :46) tells mock.patch to invent the attribute when it's missing, so the test passes identically whether or not the real name exists on the installed line. Mutation-proven: I repointed _compat.py's 2.x branch at totally_bogus_transport / totally_bogus_factory — names that exist on neither mcp line — and still got 4 passed on both venvs. A rename or a moved symbol in a future mcp release is exactly what this file exists to catch, and it can't.
Worth pairing with the previous comment: :52's assert result is mock_client.return_value pins the adapter to returning the raw transport, which is precisely what makes closing the HTTPX client impossible. So this test doesn't just miss the leak — it would fail any fix for it.
Something that would actually bite, since it's checked against the real installed module rather than a mock:
import mcp.client.streamable_http as sh
assert hasattr(sh, "streamable_http_client") is _compat.MCP_V2
assert hasattr(sh, "streamablehttp_client") is not _compat.MCP_V2For the same reason, two nearby items are worth knowing about: test_mcp_v2_flag_matches_discover_capability (:9-13) restates the implementation and only discriminates when a real mcp>=2 is installed (which no CI leg does — I confirmed it fails there if MCP_V2 is hardcoded False, and is silent on 1.x); of the four tests here, only test_mcp_error_resolves_to_installed_exception has teeth under the current pin. Separately, this PR's own edit to test_mcp_client.py:1417,1449,1476,1491,1504,1528 swapped McpError→MCPError but kept the 1.x-only constructor — 2.x is MCPError(code, message, data=None), not MCPError(error=…), so those 6 sites raise TypeError on 2.0.0. (.error.code/.error.data do resolve on both lines, so mcp_client.py:963 itself is fine — only construction differs.) Not proposing a mechanical rewrite; the right fix spans six call shapes plus how these branch tests get version-gated.
Resolve every mcp name that was renamed or relocated in mcp 2.0 through a single _compat module so the package imports cleanly on both major lines: - MCPError (spelled McpError in 1.x) - streamable_http_transport adapter (2.x streamable_http_client takes a pre-configured HTTPX client instead of loose header kwargs) - GetSessionIdCallback (removed in 2.x along with protocol sessions) - ProgressFnT now imported from mcp.client.session (available on both lines) The MCP_V2 flag is feature-probed via ClientSession.discover rather than version-parsed, and replaces mcp_instrumentation's _is_mcp_v1 probe so the whole codebase branches on one source of truth. Related to strands-agents#1659
|
Assessment update: Request Changes (correcting my earlier "Comment / approve-worthy" summary — that was wrong; CI is red because of this PR.) I independently reproduced the mypy failure @strandly-the-agent flagged, and it's a hard merge blocker regardless of the
The detailed dual-line review already on this PR covers everything else well (HTTPX leak, framing question, pre-existing 2.x gaps) — I won't re-litigate it. My one net-new emphasis: green CI is the gate here, and it's currently red on the supported floor. |
576da59 to
884973a
Compare
|
Re-review of the force-pushed head The rebase cleaned up the commit history and description, but the mypy blocker is unchanged — I re-ran it against the current head: (3 on Nothing else changed materially in |
… mcp lines Address review findings on the compat layer: - mcp 2.x's streamable_http_client only closes an HTTPX client it created itself, so the adapter now binds the client's lifetime to the transport's via a wrapping context manager. The 1.x branch is unchanged. - mypy sees only the installed mcp line, so each try/except branch carries its own ignore and a per-module warn_unused_ignores override silences the branch the installed line doesn't take. - Branch tests no longer patch with create=True (which invents missing attributes and passes against any spelling); each branch test patches real attributes and skips on the other line, and a hasattr test checks the installed module directly. - Test error construction goes through a make_mcp_error helper because the two lines have different constructors: 2.x MCPError(code, message, data), 1.x McpError(ErrorData). The .error accessor the production code reads is identical on both. Related to strands-agents#1659
|
Assessment update: Approve (was Request Changes) Re-reviewed the new head Verification
Clean, well-scoped compat layer with tests that pin the exact contracts (lifecycle ordering, per-line name resolution) rather than incidental details — nice work. |
2 participants
Description
The
mcppackage renamed and relocated several public names in its 2.0 release, andstrandscurrently fails at import whenmcp>=2is installed. This PR adds a compatibility layer so the package imports and runs cleanly on both major lines, which is the groundwork for adopting the MCP 2026-07-28 specification without forcing an immediate pin cutover.All version-dependent names now resolve through a single internal module,
strands.tools.mcp._compat:MCPErroraliases the 1.xMcpErrorspelling.streamable_http_transportadapts the transport constructor. This rename is not a pure alias: the 2.xstreamable_http_clientdrops theheaderskwarg in favor of a pre-configured HTTPX client, so the adapter builds one viacreate_mcp_http_clientand keeps the 1.x call shape at theload_serverscall site.GetSessionIdCallbackfalls back to a plain callable alias on 2.x, where protocol sessions no longer exist.ProgressFnTis now imported frommcp.client.session, which exists on both lines down to the 1.23 pin floor, so it needs no compat entry.The module also exposes an
MCP_V2flag, feature-probed viahasattr(ClientSession, "discover")rather than version-parsed, so pre-releases and backports resolve by capability. It replaces the_is_mcp_v1version probe inmcp_instrumentation.py, giving the codebase one source of truth for which line is installed. Follow-up work (lifecycle, tasks, native OTel) will branch on this same flag, and the pin can later widen to<3with no code changes.The dependency pin stays
<2; nothing changes for current installs.Related Issues
#1659
Documentation PR
No documentation changes needed; the compat module is internal.
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run preparemcp1.x (217 tests).mcp==2.0.0:import strandssucceeds (fails at import today) and the_compattests pass, covering the flag, the error alias, and both transport call shapes.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.