feat(mcp-py): relax mcp version to <2.2 - #4151
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Assessment: Comment (PR is marked WIP) Focused, well-documented change that relaxes the Review Themes
Nice work on the compatibility layer and the thorough migration doc. |
2c59727 to
b383544
Compare
|
Re-review after force-push (HEAD now The version-bound relaxation and CI flip still look sound, and my earlier notes on the What changed and what to fix
|
b383544 to
04dd4e0
Compare
|
@strandly-the-agent review the changes |
|
Re-review (HEAD Thanks for the quick turnaround. The blocking item is fully resolved and the PR is in good shape. Verification of prior feedback
One non-blocking pre-merge check (external to the code): confirm branch-protection required checks are updated to the renamed Nice, focused change — the compatibility layer and migration doc are clear and the docs now match the implementation. |
|
Status reconciliation (HEAD To avoid conflicting signals: my earlier "Approve" comment above was posted before the full fresh-context verification pipeline ran, and it only validated the
No new commits since the detailed review, so its inline comments and blocking questions (the |
|
Re-review (HEAD The three blocking items from the prior review are all resolved, and the tasks documentation is now accurate and code-backed:
Non-blocking follow-ups (not gating this PR)
Nice turnaround — rebasing onto #4125 and making the integration suite version-aware resolved the substantive concerns, and the docs now line up with the code. |
f8472b6 to
4fb6144
Compare
|
Re-review (HEAD Since the last approval the branch was rebased onto #4129 ("added mcp 2.x integ tests"). I checked that the conflict resolution integrates it cleanly rather than dropping its coverage — the earlier "whichever merges second, those tests silently skip" concern does not materialize:
One minor non-blocking nit (out of this PR's diff, inherited from #4129): No new blocking issues — the rebase is coherent and coverage is preserved on both mcp lines. |
4fb6144 to
177ffdd
Compare
|
@strandly-the-agent Review the changes. You will notice that there are many |
|
Re-review (HEAD The rewritten On the new Codecov report (80% patch, 3 missing lines) — this is expected and non-blocking. All three sit on version-divergent branches that only execute on the other mcp line:
These run in the forced-install MCP 1.x Compat job's separate coverage upload rather than the default matrix Codecov measures for patch %, so they surface as missing/partial. That's the known characteristic of the dual-version |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Verdict: approve, with cleanups — re-review of 177ffdd (rebased on #4125/#4129) plus the # type: ignore analysis you asked for. All my earlier 🔴s are resolved; nothing new blocks.
Type ignores, in one line: of the 45 in strands/tools/mcp, 25 exist only because 1.x-only code is type-checked against 2.x types, 18 only for the reverse (and CI never runs mypy against 1.x, so those 18 are never exercised), 1 is dead on both lines, 1 is over-broad. The 14 new ones in mcp_client.py/mcp_instrumentation.py can move behind ~4 _compat helpers, which restores the invariant main had (all version divergence lives in _compat).
- ⚪
_compat.py:609— delete: both names exist on 1.29.1 and 2.1.1, mypy reports the ignore unused on both lines. - ⚪
_compat.py:72—[misc, assignment]→[misc];assignmentnever fires on either line. - 🟡
pyproject.toml:263— the 1.x-only ignores are unchecked today; amypystep in the MCP 1.x Compat job closes that (inline). - 🟡
mcp_client.py— 12 new ignores collapse into_compat.experimental(), task-status constants, acancelled_notification()helper and a widerMCPTransport(inline). - 🟡
auth_provider(my open thread) — documented now, still no fail-fast; downgraded from 🔴, reply in-thread.
Ran this one myself (follow-up shape) rather than the full fan-out; details and the per-ignore matrix below.
Status of my previous findings
| Finding | Status |
|---|---|
🔴 tests_integ/mcp red on 2.x |
Fixed — tests_integ/mcp/conftest.py collect_ignore + mcp-1x integ scope + hatch STRANDS_TEST_MCP_V1 override. Verified collection: 2.1.1 → 13 tests (test_mcp_client_v2.py only), 1.29.1 → 48 tests across all 9 modules. Caveat: the mcp-1x scope does not run on this PR — pull_request_target executes main's workflow file — so its first real run is after merge. I could not exercise the hatch override locally (sandbox filesystem failed mid-review); the override syntax matches hatch's overrides.env.<VAR>.dependencies form. |
🔴 auth_provider=<httpx.Auth> breaks on 2.x |
Documented, not guarded → 🟡. MCP_VERSIONS.md now lists it and the README says "most code". The runtime failure is still Invalid "auth" argument with no mention of mcp versions. Fail-fast at construction remains my suggestion; not blocking. |
🟡 ignore_errors = true on mcp_client/mcp_instrumentation |
Fixed — replaced by per-line coded ignores + warn_unused_ignores = false, as suggested. Those ignores are what the analysis below is about. |
🟡 MCP_VERSIONS.md accuracy |
Fixed — raw-model field casing, auth_provider, and the ToolAnnotations wording are all corrected. Nit: the casing bullet names get_prompt/read_resource; list_prompts, list_resources, list_resource_templates return raw models too (nextCursor/resourceTemplates). |
🟡 tasks_config bare ImportError on 2.x |
Moot — #4125 landed; tasks run on both lines and the doc says so. |
Type-ignore analysis — method and matrix
Method: flipped the mcp-module override to warn_unused_ignores = true in a scratch copy of pyproject.toml and ran mypy ./src ./tests_typing in two venvs (mcp 1.29.1 and 2.1.1, mypy 2.3.1). An ignore reported unused on line A is one that exists only for line B.
| Class | Count | Where |
|---|---|---|
| Needed only when checking against 2.x (1.x-branch code seen through 2.x types) | 25 | _compat.py:65,68,160,205,212,217,548,621,628,630; mcp_client.py:1756,1760,1762,1957,1984,2316,2335,2378,2407,2419,2422,2609; mcp_instrumentation.py:220,222 |
| Needed only when checking against 1.x | 18 | _compat.py:62,72(misc),99,100,111,169,210,243,276,277,502,542,545,575; mcp_client.py:2093; mcp_tasks.py:198,211,223 |
| Needed on both (not version-related) | 1 | mcp_client.py:1678 [unreachable] |
| Dead on both | 1 | _compat.py:609 — streamable_http_client/create_mcp_http_client exist on 1.29.1 already; only guards 1.x releases before the rename, which nothing type-checks |
| Over-broad | — | _compat.py:72 [misc, assignment]: assignment unused on both lines |
Two consequences worth acting on:
- CI runs mypy once, against 2.x (
hatch fmt --linter --checkin the Lint job). The 18 1.x-side ignores are therefore never exercised — nothing checks that the 1.x-typed code actually type-checks on 1.x, and a dead ignore like:609is invisible. Addingpip install mypy && mypy ./src ./tests_typingto the MCP 1.x Compat job (it already has a real 1.x install) makes both sets load-bearing at the cost of ~1 minute. - Where the ignores live. On
main,mcp_client.pycarried 2 ignores (neither version-related) and_compat.pycarried 23 — the version seam was fully inside_compat. This PR adds 12 tomcp_client.pyand 2 tomcp_instrumentation.py. Every one of those is a 1.x-only expression that can be a_compathelper (see inline threads), leavingmcp_client.pywith its original 2 and_compatas the single place a reader has to know about the two lines.
Considered and not recommended: always_true/always_false = ["MCP_V2"]. It's the mypy-native way to type-check each branch against its own line (like sys.platform), and would need two mypy runs (Lint with always_true, 1.x job with --always-false). I tried it with all 45 ignores stripped: 26 errors remain on 2.x and 12 on 1.x, because most 1.x-only code is in unconditionally defined functions (_call_tool_as_task_and_poll_async, the cancel path, TransportContextExtractingReader, the client_credentials_auth fall-through), which branch pruning doesn't reach — and every if MCP_V2: return … fall-through trips warn_unreachable. It only pays off if the 1.x-only functions are moved into branches or a separate module; the _compat helper route gets most of the benefit for much less churn.
Runtime facts used above (verified): mcp 2.1.1 CallToolResult(isError=True) → is_error=True (populate_by_name=True), so mcp_client.py:1984 is typing-only; mcp 1.x TASK_OPTIONAL/TASK_REQUIRED/TASK_STATUS_* are the plain strings "optional"/"required"/"cancelled"/"completed"/"failed".
Verified
- Branch
docs/mcp-v2-migration@177ffdd, basemain@3f2ee2f(includes #4125, #4129). Diff: 12 files, +136/−46. - GitHub Actions on
177ffdd: CI success — Lint, MCP 1.x Compat, 11 unit legs; "Python: Integration Test" (allscope) still running at review time;mcp-1xscope absent from this run by construction (pull_request_target). pytest tests_integ/mcp --collect-only: 2.1.1 → 13 collected, 0 errors; 1.29.1 → 48 collected, 0 errors.- mypy matrix above (both lines,
warn_unused_ignores = true), plus thealways_true/always_falseexperiment on an ignore-stripped copy. - Not re-run at this head: the unit suite in my own venvs — the sandbox filesystem failed partway; CI's green Lint / 1.x Compat / matrix stand in for it.
- Evidence:
type-ignore-matrix.mduploaded to the artifact store for this PR.
Questions (non-blocking)
MCP_VERSIONS.md:30says integration tests "split the same way", but themcp-1xscope only takes effect after merge and the 1.x compat unit job runs no mypy. Do you want the 1.x line type-checked in CI, or is "1.x is best-effort, 2.x is the checked line" the intended posture? The answer decides whether the 18 1.x-side ignores are worth maintaining precisely.- Still open from the first review, no change: is 2.x-by-default recorded anywhere (
team/DECISIONS.md), and is there a trackedsite/follow-up under #1659 for the pages that teachstreamablehttp_client/FastMCP?
Description
Widens the range to
mcp>=1.23.0,<2.2, so a fresh install now resolves mcp 2.x.Projects that need to stay on 1.x pin
mcp<2themselves. The upper bound excludesmcpreleases we have not verified yet and moves up as we test new ones.Because the unit-test matrix now resolves 2.x, the force-install compat job flips direction:
unit-test-mcp-v2becomesunit-test-mcp-v1("MCP 1.x Compat"), force-installing mcp 1.x so the 1.x branches of_compatstill run against the real package. The legacy 2025-11-25 task tests are gated to 1.x, matching the workflow they exercise.A new developer doc,
strands-py/docs/MCP_VERSIONS.md, covers the support status, install options, and what behaves differently on 2.x. The README's MCP section links to it.Related Issues
Part of #1659
Documentation PR
Developer docs are included in this PR (
strands-py/docs/MCP_VERSIONS.mdand a README pointer). Nosite/changes.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 prepareCI exercises both
mcplines on this PR: the regular unit-test matrix resolves 2.x, and the renamed MCP 1.x Compat job force-installs 1.x and runs the full MCP client suite against it.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.