Repository navigation
[BREAKING] Python: scope declarative MCP sessions to invocations - #8662
Eduard van Valkenburg (eavanvalkenburg) merged 96 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ed91283-cb21-49a7-98f4-b1c088d0b6b6
Code coverage overviewLanguages: Python, C# Python / code-coverage/python
View coverage of the 2 most impacted files.
C# / code-coverage/dotnet
View coverage of the 3 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): bb9e92b189ed
Model: gpt-5.6-sol-fast
Overview
The change adds workflow identity to both default MCP cache keys while preserving the existing endpoint, label, connection, and header dimensions, and its regression tests cover normal isolation, continuation, and same-key creation coalescing. Provider-backed calls also retain their per-invocation lifecycle and shutdown guards. Two residual gaps remain: the .NET compatibility fallback is scoped to a reusable workflow object rather than an execution session, and Python's completed-entry LRU does not bound the now-per-workflow set of concurrent connection attempts.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 2 files. Details are attached to the affected lines below.
Affected areas: dotnet/src/Microsoft.Agents.AI.Workflows.Declarative/Interpreter/DeclarativeWorkflowContext.cs, python/packages/declarative/agent_framework_declarative/_workflows/_mcp_handler.py
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the latest review summary item about .NET global client lock contention in current branch changes: cache misses now reserve/coalesce an in-flight creation under |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 89548cb9-9583-47c1-a00f-675aebf810f8
There was a problem hiding this comment.
🟡 Changes recommended
Wrapped-reply routes are cached before checkpoint state is restored or earlier handlers update routing fields, so stale decisions can drop replies or deliver them to the wrong child.
2 open issues
1 resolved since last review
Review effort: Balanced
Reproduce routing decisions cached before earlier handlers run and before checkpointed executor fields are restored. These eight regression cases currently fail; the implementation correction is pending routing-contract guidance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06d37a04-f20d-46d4-ab56-1ef5adda7b2d
Select wrapped reply routes after preceding handlers and checkpoint restoration, validate all selected children before delivery, and retain selected parent scopes through the superstep. Remove message-bound route caches and preserve undelivered messages on rejection. Document the delivery-time validation boundary and checkpoint recovery after partial execution. Preserve ordinary response and structural checkpoint preflight. Validate routing regressions, child-policy staging and the revised rejection boundary without skipping tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06d37a04-f20d-46d4-ab56-1ef5adda7b2d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06d37a04-f20d-46d4-ab56-1ef5adda7b2d
Preserve per-edge delivery ordering while validating wrapped replies only across compatible target paths. Add waiter/child-edge regressions for broadcast, targeted, and independently queued replies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9f6c9cde-10e6-497b-9dd3-c708cf6f10b3
…ion-isolation # Conflicts: # python/packages/declarative/agent_framework_declarative/_workflows/_mcp_handler.py # python/packages/declarative/tests/test_default_mcp_tool_handler.py Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
Preserve per-edge ordering while coordinating only child workflow policies. Cover filtered ordinary consumers in streaming and non-streaming runs and document checkpoint migration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63b4d784-d6b7-44fd-b8c8-6373d546cc94
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because nested reply validation can deadlock, and incomplete descendant edge-state preflight can leave a rejected restore partially applied.
2 open issues
2 resolved since last review
Review effort: Balanced
Co-authored-by: baywet <7905502+baywet@users.noreply.github.com>
|
We're likely to replace this PR with another one. |
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues silently drop authorization and tenant-routing headers and cause all six client-isolation test parameterizations to fail before exercising the handler.
2 open issues
4 resolved since last review
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The test expects headers instead of the actual static_headers constructor argument, causing the assertion to fail and leaving a subsequent lookup that would raise KeyError.
2 open issues
1 resolved since last review
Review effort: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
MAF Automated Review — Iteration 3
Result: Findings reported
Scope: 250 net-new commit(s): 54b558b86eae, bb4a74331bcf, c01db29b7611, e5886f4fac6b, 2c52eeda221b, 809cc04f2d61, 8ab068b8b360, fa7746745606, f7c6928a93f0, 30bc5c1bd015, 08851e69d722, 6b48d20288fa, 02005995380d, 1fe5467d13ec, 4c191ff5099d, 20f07283ae47, dbca9d8f1fcf, 6ae0f4086d81, 621e8e39daff, c7394476bffb, 10f7f989fba2, 70ba23a3970c, 597342c2c688, a479349b2a7a, a9eff53bdcca, 1ba7425571fd, b7ce0d616fd0, 06050253e90e, 8be57da33007, 5da17697d62d, b7f8ad686b07, 74f3447d030c, 89457c5bcdd7, 8ff2ec937fe4, bba95967e36f, 074bc0b17e60, 1263843977f6, d0c3e6acfe4e, 33439bf8bcf3, fb57ceb6bb0d, c3ce250695f7, ce214c0b3f69, 07a83a7c369b, d33c44583992, a8e284ca8ce7, 3b725b3b0ffe, 9c53b5f44658, 9581926deec6, 8730cb197485, 678c6616aa4c, ea32b6db9e9b, 71cf48cb4019, c3b9f22d64fd, 9c8a3514a15a, edc2427539b3, 4982e4c61ec8, 67ba500ec1fd, 1ac5a55edae8, cca10b35b667, bacdc8210dc9, 93be013696ac, 9737244b425d, 5f85df31777c, bbe3efb4ac25, 264439f7f5c7, 6d4d2ee59f95, d8dcc1ae926e, 9e105555cf8e, bdf5256b4fea, 3cf2cd83ef01, 477c6dea1658, 5a974549cb81, 269f65ee3d73, df9b0450650b, da9457a3e678, eceab14b40c9, 0fd5002871ab, 9337c508e46d, 89c7018da7b8, 5b780766cefa, eeeb67a5ae75, b9c5905d38be, 92bed0b75dfd, d0eb8658b52b, 7db56c05f949, b79c03c7cb2b, 3f61bf5bdfc0, 3f2705269ab8, 1c19834d996b, 0c1e6eed959c, f86c0ebd86cd, 172ed9139441, 18adf808a57e, 1168c001dd22, b1a4c9494ee4, 6e9167e101f7, 79f848f3a322, 7de7026a32a5, c4de86d41e34, 45ea037d474b, a9f29d0e324b, 14ce83b5635b, 63d0998c9e3a, 10a94cf736d7, 3a70a81a260b, abaf33a3c8eb, 1b858e53e75a, 626b3fb50a36, cdedd6942d74, 61357e4950a2, 981f70e3c7bb, c7bfe991b0ae, be2ef000e309, 5a950ad5d3ea, a9fe090bb1e8, 0d767ce528bf, 7b957d19f0e8, 8639a7411478, e1b150d4c735, 6afcecd428dd, 16b16558019e, 7e45edea863e, 20458faef1e5, 4a272d2b8e96, 9b72ed4f96bc, 49b715debe37, 2e931679dcf6, c2a8f91b3070, 5f469d26d519, 8624e76997b9, b2d552e3c8e9, 581751842e86, fc3c3c078937, 569a39e7ad7c, 13c22b50e158, 30ce7e39ce04, b6d068d730a7, 908ed6fa908e, 268461487a02, 9ef867212c7b, f0f190693fa7, ce7d17463537, 9e971dba685e, 5009d68efe26, 517ebf12965d, 0577d313fb96, 7951f33f15f7, 1e4d973b4e90, 85f3fc6bec6f, 4d515ae7b834, 21c5ab124bc6, 36cf53c7c918, 4d28d0a4ba77, 1a6300e28765, 59b183b5bcb2, 5de71b8a7a2c, 34452c8ae32b, dc780f892e91, 43d767c55050, adb2f4ee366c, 74f9dbcbe9aa, 4d4e2168fe57, b2fd10b3f9ca, e3dd24f741ea, 83df9d6d8d8c, 4d1674b78951, 3aad44e54dca, b7884ad88eb8, cf040fce66fd, 92a4dcce6c5a, 64127c1bc152, 9798ad3115ab, ca65085e1025, c68fe3065688, e480189352d5, da172009ab37, 5b8c9c8a3491, 33637aa0e481, 96646c596aeb, 93c346f5a05a, d2fb432174e9, d1726fea434b, 6896fc77bec5, 97cc7f31a0f7, 9f52e715526f, f048cd35b339, 60eae452ec75, f1483343ae52, 03341b15e6cf, 6fdeb2d22994, f6548e727767, 9be4c6737825, 4bb5ffa7da18, 88371929a016, b5344024e145, fad6929ca8cd, 65b2ee61e8d9, 2551317ca2ff, 91e49cdc5452, f4bdd1dc98ae, b2c4de383e1c, a7c5abe3a880, d615566db7b9, 7998d7c9e79f, c120e390da5d, 4668b67ed966, d6e83735488c, d8fc1553779a, 0becdc4d8c34, e2936e2caa8e, 503a9ec155ec, dc6617aa96f2, f34fafb95469, b6ad777cfad2, 52230700d5c9, 84573b186a1e, e2dc4605b7c2, 39dc737178cf, 1c152ab2c43e, 4522e336a18f, cd45c5890ef5, 3f70b72c7f9f, 693b2f5c3140, 91b1d9aad800, 3c301645f447, 989bc9964ed3, c2b8286a60b5, b76bb7639365, f84f1a42efb3, fd1230844b89, f6a7335697bd, 8533256df5d0, 9fa9e9b80673, 3792f452324c, cf734bc4692c, 843cee55458f, ad15f10663f7, 85d73a9b6944, 08705f7451eb, ce097d1de381, faee0dddce41, 708c8f3a59bb, 725a0684cad0, 7f1f8f4fab0c, feb59e3e6470, 513cdea38d4d, d700d962bf96, 5ca6004409a0, 98a98439077c, c6b4574ec2ef
Model: gpt-5.6-sol
Overview
The handler now creates and closes a fresh MCP tool and protocol session for each invocation, with shielded cleanup, explicit borrowed-client ownership, and strong isolation coverage across concurrency and resume paths. The implementation uses Core’s supported static_headers contract and removes the previously problematic retained cache state. One changed constructor assertion still expects the obsolete keyword, however, so the added unit test fails against the final implementation.
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/declarative/tests/declarative/test_default_mcp_tool_handler.py
Align invalid tools/list arguments with the .NET handler and assert validation precedes client creation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0ba8a2b-1523-4b32-bb92-07cc5ce128e0
Align setup, discovery, and transport failures with .NET without converting them into tool results. Preserve remote tool error responses and exception causes, including after approval, and document the contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b0ba8a2b-1523-4b32-bb92-07cc5ce128e0


Motivation & Context
Prevent independently constructed declarative workflows from sharing stateful MCP sessions when they use the same handler, endpoint, and headers. Keep the fix in the declarative MCP handler instead of adding workflow identity and checkpoint machinery to Core.
Description & Review Guide
MCPStreamableHTTPToolfor every invocation, includingtools/list. Remove the initialized-client cache and handler-wide lifecycle state. Retain the optional Pythonclient_providercallback for borrowed HTTP connection pools. This follows the invocation-ownership principle from .NET: [BREAKING] fix: scope workflow MCP clients to invocations #9722 without introducing an HTTP client factory or additional transport options. Update the two MCP samples, package documentation, consumer guidance, and a proposed decision record.cache_max_size,aclose(), and the handler async context manager; implicit server-session continuity no longer survives later calls, approval waits, or checkpoint resumes. Every invocation pays connection and discovery overhead. There are no Core changes and no changes to workflow checkpoint, request routing, or approval-verification contracts.Migration: replace
async with DefaultMCPToolHandler(...) as handler:with ordinaryhandler = DefaultMCPToolHandler(...)construction. Removecache_max_sizeand handler shutdown calls. Keep caller-owned HTTP clients in their own context manager, and cancel/await active invocations before closing them. Hosts own admission, deadlines, cancellation, and shutdown coordination. Applications needing server-session continuity must use explicit remote state handles or an explicitly scoped customMCPToolHandler. No checkpoint migration flag is introduced. See the package guidance and decision record.The follow-up commit
5ca600440replaces the earlier workflow-scope implementation rather than adding another layer to it. Core source, tests, and guidance are restored exactly to the PR base. Request-origin maps, nested reply barriers/preflight, and legacy request-scope migration are no longer part of this PR.Local verification on Python 3.13, from
python/:uv run --no-sync pytest packages/core/tests/workflow packages/declarative/tests -o addopts='' -q --disable-warnings --maxfail=3: passed; 3,131 passed, 1 skipped, 2 expected failures, 29 warnings. The in-process MCP tests deliberately force the legacy stateful handshake, which emits the SDK ping deprecation warning.uv run --no-sync poe syntax -P declarative: passed.uv run --no-sync poe pyright -P declarative: passed, zero errors or warnings.uv run --no-sync poe test-typing -P declarative: passed for Pyright, mypy, pyrefly, ty, and zuban.git diff 725a0684cad006ad3e941045840311c02862a958 --exit-code -- python/packages/core: passed, no net Core diff.git diff --check: passed.The real protocol regressions cover sequential and concurrent invocations, optional-provider fallback and borrowed pools,
tools/list, independently constructed workflows in streaming and non-streaming modes, and approval/checkpoint resume. Server-side session termination is checked before each invocation returns. An in-memory comparison using the unmodified base handler reproduced the original failure: one session history with two calls instead of two isolated histories. No production files were changed for that comparison. The full workspace build, full workspace test suite, and external-service integration suites were not run.Related Issue
No public issue is available for this maintainer-directed bugfix. This updates the existing pull request rather than opening a competing one. #9722 addresses the corresponding .NET handler lifetime; this PR addresses Python.
AI Assistance
AI assistance details: Copilot assisted with implementation, regression tests, documentation, validation, and this PR description.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.