feat(mcp/py): support SEP-2663 tasks - #4125
poshinchen merged 8 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@strandly-the-agent review, please! |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Changes requested — the SEP-2663 slice matches the spec and the 1.x path is genuinely preserved, but the new poll loop leaks server-side tasks on every exit that isn't a timeout (a protection the legacy path had), several public docstrings now describe behavior the 2.x path doesn't have, and the surface is large enough for the API-review workflow.
- 1 🔴 + 7 🟡 inline, 11 appendix items, 3 blocking questions below. One pre-existing bug found along the way was filed separately as #4130 (not this PR's problem).
- API review: 8 new public methods + 12 exported types reads as substantial per
team/API_BAR_RAISING.md— it likely needsapi/needs-reviewand a reviewer session. Note the greencheck-api-review-labelcheck skips when no label is present, so it doesn't mean review happened. - Reachability caveat: with the runtime pin
mcp<2.0.0, no released-SDK user can reach the 2.x task path yet — that's why most findings are 🟡. The one 🔴 is a dropped protection with no test, live the moment the pin lifts.
What was verified
- ✅ Reviewed head
ebb3755d4(includes the modern→v2 rename). - ✅ 1.x (mcp 1.29.1):
tests/strands/tools/mcp→ 306 passed / 28 skipped — the 1.x-preservation claim holds against the existing suite. - ✅ 2.x (mcp 2.0.1, mirroring the CI job):
test__compat.py+test_mcp_client_tasks_v2.py→ 63 passed / 1 skipped. - ✅ 8 mutants run against the 2.x selection; 4 survived (input dedupe, server
pollIntervalMs, all four lifecycle guards,createdAtcheck) — each survivor maps to an inline finding. - ✅ ~10 adversarial repros against an in-process mcp 2.0.1 tasks server; key ones re-run at head. Robustness that held: status flapping with distinct keys,
pollIntervalMs=0floor,stop()mid-poll (no thread/loop leak), all four*_asyncmethods from a normal asyncio loop (no deadlock). - ✅
ruff format --check(638 files) +ruff checkpass. ✅ Exports in__init__.pyexactly matchmcp_tasks.__all__. - ✅ Verified on both mcp lines: the same task JSON parses on 2.0.1 and fails with 5 validation errors on 1.29.1 (inline finding on the exports).
⚠️ mypy not run locally (two sandbox attempts failed); one open check rides on CI:MCPError.from_error_datais 2.x-only, used without an ignore, andmcp_client.pyisn't in thewarn_unused_ignoresexemption.⚠️ CI at this head was still pending when reviewed.- Repro scripts/logs and the mutation-test log are uploaded to the review artifacts store (
strands-agents/harness-sdk/pr/4125/).
Questions
Blocking
tasks_config={}is the only way to reach the new lifecycle API, and it also routes everycall_tool_syncon a tasks-advertising server through the task path (tool-leveltaskSupportis no longer consulted on 2.x). Is that coupling intended, or should the manual API get its own opt-in?- What's the intended public timeout story?
read_timeout_secondsnow means per-round on a direct call but whole-task on a task call, with the per-round bound only reachable via the newrequest_timeoutconfig — whilepoll_timeoutalready means "whole task". If deliberate, it's a behavior change to existing public API on 2.x and worth a line in the API review. - Is dropping the experimental markers a decision or a casualty of the rewrite? Either is fine — it needs to be on the record (see inline).
Non-blocking
4. Are task status notifications (notifications/tasks via subscriptions) a planned follow-up? It's the one SEP-2663 piece this slice leaves out and the actual ask in #1812; #4112 already built the subscription plumbing. A line in the body would close the loop.
5. The manual API exposes create/get/update/cancel but keeps the waiting logic private, so "start now, join later" means re-implementing ~50 subtle lines of _complete_v2_task. Worth exposing a wait_for_task_* over the machinery that already exists?
6. The claim pins protocol_versions={"2026-07-28"} at session construction and the guard demands exact equality, so the next protocol revision fails closed at start()/first call. Intended posture? CI pins mcp==2.0.1 and 2.1.x is already on PyPI, so nothing would notice drift.
7. _fulfill_task_input sends one tasks/update per key while the MRTR path batches all responses into one round. Deliberate (a declining callback doesn't discard collected answers)? A comment would settle it.
8. Naming: call_tool_with_task_* is the odd one out in a <verb>_task_* set and doesn't convey "returns without waiting" — MCP's own vocabulary says "call tool as task". And update_task reads like "modify the task" rather than "answer its pending input requests".
9. Four new *_async methods with no tests, on a client that's otherwise sync-only for prompts/resources/list_tools. Commit to async pairs across the client, or trim to sync until asked?
Reading order
mcp_tasks.py— wire models andTasksConfig; everything else is written against these._compat.py(new block at the end) — how a server is judged task-capable on each mcp line, and how the 2.x extension claim is installed at session build.mcp_client.pygates —_should_use_task(where 2.x stops consulting tool-leveltaskSupport) and_require_v2_task_lifecycle(the four guards).mcp_client.pyengine —_complete_v2_task,_reconcile_v2_task_state,_call_tool_with_task_and_poll_async. Most findings live here; read against the untouched legacy_call_tool_as_task_and_poll_asyncdirectly below, which shows what the new path chose not to keep.- The 8 new public methods — thin wrappers over 3–4.
mcp/__init__.pyand thecall_tool_sync/call_tool_asyncdocstrings — the public surface and its stated contract.- Last: the new test file and the workflow change — what is actually pinned, and where it runs.
Appendix — non-blocking (11)
- ⚪ The new task path emits zero log lines; the legacy path logs 11, including the created task id — an operator's only handle for out-of-band
tasks/get/tasks/cancel. - ⚪ A failed task's JSON-RPC
code/dataare dropped; only the message survives, and a task failing with-32042misses the elicitation-required handling the direct path gives the same code. - ⚪
pollIntervalMshas a floor but no ceiling and no clamp to remaining budget:pollIntervalMs: 3600000sleeps past the wholepoll_timeoutwithout a singletasks/get(demonstrated; bounded by the deadline). - ⚪ Raw pydantic validation text reaches the tool result — and hence the model's context ("1 validation error for tagged-union[…]"). Fails closed, but the message is written for a pydantic user.
- ⚪
isinstance(input_responses, dict)→TypeErroris defensive code the annotation already covers, and it rejects a non-dictMappingthe wire path would accept. - ⚪ Test structure: one 118-line test covers four public methods and both automatic paths (failures localize poorly), and there's a load-bearing
assertinside the server handler — prefer asserting recorded params client-side. The real in-process server itself should stay: it's the only thing pinningallow_claimed=True(mutant-verified). - ⚪ CI pin
mcp==2.0.*→==2.0.1is fine per the job's own "bump deliberately" comment, but nothing in CI notices protocol drift (see Q6) — a scheduled unpinned run or tracking issue would. - ⚪ Capability detection ignores the per-extension settings dict; a server advertising the extension but implementing a different revision makes every tool call create-then-fail (demonstrated). Non-blocking because mcp 2.0.1 defines those values as opaque — there's nothing spec'd to check; the residual leak is the 🔴 inline.
- ⚪ No
tests_integ/addition, while the legacy path hastests_integ/mcp/test_mcp_client_tasks.py+task_echo_server.py. - ⚪ The CONTRIBUTING attestation ("I have reviewed and understand every line") is unchecked on a +1265-line PR.
- Pre-existing (not this PR's problem): per-call remote cancellation silently never fires on the 2.x direct path — filed as #4130.
Review run: triage → context pack → 5 independent specialist passes (correctness, issue-alignment, API bar-raise, adversarial+repro, test-quality) → aggregation. The API and adversarial passes timed out on the first attempt and were re-run successfully on the standard tier. Solid work overall — the survived-attacks list above is real robustness; the asks are focused on cleanup guarantees, the documented contract, and pinning the guards with tests.
|
Issue (API bar-raising): This PR adds a substantial new public surface — 8 methods (
Suggestion: Add the |
|
Assessment: Comment Solid, well-structured addition of SEP-2663 task support that preserves the MCP 1.x path and keeps the feature opt-in. The task result models have thorough validation and the malformed-shape/reconciliation tests are a highlight. Main themes below before merge. Review Categories
Nice work threading this through the existing compat layer without disturbing 1.x behavior. |
|
Review reconciliation / verdict correction Cross-referencing the more thorough review already on this PR (the "Changes requested" review with the 🔴 task-leak finding): that review is the authoritative one, and I'm aligning to it. Two corrections to my earlier pass:
Apologies for the noise from the duplicate pass. |
ebb3755 to
1ca5592
Compare
1e629ae to
f1f47b1
Compare
|
@strandly-the-agent please review oncemore |
…into agent-tasks/sep-2663-python # Conflicts: # .github/workflows/python-test-lint.yml
|
Assessment: Comment — code is in good shape; one process gate remains. Independent re-review at Verified locally against
|
1171b5a to
54da93c
Compare
54da93c to
5e0f7ec
Compare
|
Issue: The PR description's Public API Changes section is now stale after the Suggestion: Update the description to On the one remaining process gate (raised in earlier passes, still open): the PR carries no |
|
Assessment: Comment The implementation is in strong shape after several review passes — the substantive code concerns (task-leak on cancellation/timeout, 2.x timeout docstrings, patch coverage, timestamp dedup, Review notes
Nice work closing the cancellation guarantees and backing them with regression tests — the remaining items are the stale description and the API-review sign-off, not code correctness. |
|
Assessment: Request Changes The implementation is well-structured and thoroughly documented, with strong unit coverage of the SEP-2663 lifecycle, cancellation races, and model validation. The blocker is in the tests, not the production code: the one test that exercises the real MCP 2.x transport end to end has import typos ( Review themes
Nice work on the cancellation-during-create handling and the timestamp-reconciliation logic — those edge cases are easy to miss and are handled carefully here. |
|
Disregard the bot comments, |
Description
This is the SEP-2663 Tasks slice of the Python SDK's MCP 2026-07-28 adoption tracked in #1659. Earlier PRs prepared the SDK for MCP 2.x model and instrumentation changes (#3528, #3611), established compatibility and
server/discovernegotiation (#3708, #3821), and added pagination, tool-call, MRTR input, and list-changed subscription support (#3984, #4090, #4094, #4112).With those foundations in place, this PR adds opt-in support for finalized task handles and the
tasks/get,tasks/update, andtasks/cancellifecycle. Tasks now surface in two ways:call_tool_sync/call_tool_async— and tool calls anAgentmakes throughMCPAgentTool— keep returning a terminal tool result. Whentasks_configis set and the server advertises task support, the client routes under the hood by installedmcpline: on 2.x it drives the finalized SEP-2663 lifecycle for you (create → polltasks/get→ answerinputRequestsviatasks/update→ terminal result); on 1.x it keeps the legacy 2025-11-25 flow. Callers see no behavioral difference between the two lines.RuntimeErrorbefore sending anything.Public API Changes
New
MCPClientmethods, each as a sync/async pair:*_sync/*_async)call_tool_with_task_*MCPCallToolResult | MCPCreateTaskResultget_task_*MCPGetTaskResultupdate_task_*MCPUpdateTaskResultcancel_task_*MCPCancelTaskResultNew types exported from
strands.tools.mcp(experimental, like the existing tasks surface):MCPTask— base task state shared by task results (task_id,status, timestamps,ttl_ms,poll_interval_ms), withMCPTaskStatusas its status literalMCPCreateTaskResult— task handle returned instead of an immediate tool resultMCPGetTaskResult— status-specific state fromtasks/get(input_requests/result/error), validated against the task's statusMCPUpdateTaskResult,MCPCancelTaskResult— validated empty acknowledgementsMCPTaskError— JSON-RPC error stored by a failed taskMCPCallToolResult,MCPInputRequest,MCPInputRequests,MCPInputResponse,MCPInputResponsesNo existing signature changes; the 1.x path and the terminal-result behavior of
call_tool_sync/call_tool_asyncare preserved.Related Issues
Documentation PR
N/A — no site documentation changes in this PR.
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 preparemcp==2.0.1, including the public lifecycle over an in-process transportChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.