[Vincent Biret (baywet)]

Motivation & Context

The default declarative MCP handlers cached connected clients by endpoint, label, connection name, and headers. When one handler instance served multiple workflow sessions, those sessions could therefore reuse the same stateful MCP protocol session. This change fixes the MCP client session isolation/cache scoping bug while preserving session continuity within one workflow session and the existing endpoint, header, label, and connection isolation.

Description & Review Guide

• What are the major changes? Added framework-owned workflow session identity to the Python invocation contract and a workflow-scoped .NET handler contract, propagated trusted session IDs from workflow execution contexts, and included that identity in default MCP client cache keys. Added regression coverage for separate workflows, same-workflow continuation, cache-key discrimination, and concurrent cache creation.
• What is the impact of these changes? Separate workflow sessions no longer share a stateful MCP protocol session through a shared default handler. Calls within the same workflow session continue to reuse the cached MCP client when all existing cache identity dimensions also match. Custom handlers using the existing .NET contract remain compatible.
• What do you want reviewers to focus on? Please review the workflow-owned scope propagation and the compatibility behavior for custom handlers and direct unscoped handler calls.

Related Issue

No public issue is available for this MCP client session isolation/cache scoping bug.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

[github-code-quality]

Code coverage overview

Languages: Python

Python / code-coverage/python

main 9e971db	fix/mcp-workflow-ses... 4d515ae	+/-
92%	92%	0%

View coverage of the 10 most impacted files.

File	main 9e971db	fix/mcp-workflow-ses... 4d515ae	+/-
packages/declar..._mcp_handler.py	98%	96%	-2%
packages/core/a...flows/_state.py	100%	98%	-2%
packages/declar...cutors_tools.py	99%	98%	-1%
packages/declar...arative_base.py	93%	93%	0%
packages/declar...tive_builder.py	99%	99%	0%
packages/core/a...lows/_runner.py	98%	99%	+1%
packages/declar...utors_agents.py	91%	93%	+2%
packages/core/a...low_executor.py	87%	89%	+2%
packages/core/a...flow_builder.py	94%	97%	+3%
packages/core/a...uest_context.py	0%	89%	+89%

ℹ️ Calculations represent line coverage. Results may change as calculations are updated.

---

_{Updated October 29, 2026 18:11 UTC}

[github-actions]

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

[Vincent Biret (baywet)]

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 _clientLock, run CreateClientAsync outside the global lock, and publish/evict under the lock afterward. Added NoProvider_ConcurrentWorkflowSessionCreations_DoNotSerializeHandshakeAsync to cover concurrent workflow handshakes.

[Vincent Biret (baywet)]

Fixed the latest Pre-commit Hooks failure in 0d767ce. Adding postponed annotations made the existing quoted Workflow annotation redundant; the pinned pyupgrade hook required it to be unquoted. Applied exactly that one-line correction. pyupgrade 3.21.2 (--py310-plus), Ruff lint/format, strict source Pyright, and 18 focused nested-workflow tests pass. Merge-gatekeeper failed solely because of that hook failure; the new head will rerun normal CI.

[Copilot]

🟡 Changes recommended

Nested invocation and parent-entry replay paths still leave gaps in MCP session isolation and continuity.

1 open issue

• Preserve child MCP scope during parent checkpoint replay · New

1 previously missed issue

In code that hasn't changed since last review

Propagate parent scope to workflow-backed agents

python/​packages/​declarative/​agent_framework_declarative/​_workflows/​_mcp_handler.py:102

When InvokeAzureAgent invokes a registered declarative workflow.as_agent() without an explicit session, unrelated parent sessions reuse the same child sessionless scope and cached MCP connection. _invoke_agent_and_store_results() does not supply parent identity by default, and the new WorkflowExecutor propagation does not cover this path. Propagate the parent workflow scope to workflow-backed agents while preserving top-level sessionless continuation, and add a regression with two parent sessions invoking the same child agent.

_{Review effort: Balanced}

[Vincent Biret (baywet)]

Investigated the separate workflow-backed-agent finding in #8662 (review). The reported InvokeAzureAgent -> registered workflow.as_agent() path does not currently reach child execution: both streaming and non-streaming parent regressions fail with TypeError: WorkflowAgent.run() got an unexpected keyword argument 'options', wrapped as AgentInvalidResponseException. _invoke_agent_and_store_results always passes options, but WorkflowAgent.run does not accept it; no child MCP invocation occurs. Consequently I have not reproduced cross-parent MCP reuse through this path and am not claiming it fixed. Per the agreed scope, I left that integration unchanged rather than enabling a currently unsupported path as part of this fix. Please clarify the supported invocation/adapter path intended by this finding, or whether enabling that integration should be separate work. Leaving this summary item open for guidance. The independently reproduced parent-entry replay finding is addressed in 7b957d1.

[Copilot]

🟡 Changes recommended

Resumed child requests leave ancestor scopes unchanged, allowing downstream MCP calls to reuse another run’s session.

1 open issue

• Restore originating scope before forwarding resumed child outputs · New

1 resolved since last review

• Preserve child MCP scope during parent checkpoint replay

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Cross-cutting workflow scope and checkpoint changes alter core continuation semantics, requiring maintainer validation of stored-checkpoint compatibility, nested restoration, and cancellation behavior beyond isolated verification.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Preserve parent request origins across deferred and replayed responses

python/​packages/​core/​agent_framework/​_workflows/​_workflow.py:473

Ordinary parent ctx.request_info() requests do not save or restore their run origin; only forwarded child requests populate the origin map. If parent runs A and B pause before a declarative child, answering B and then A makes both child invocations derive B's scope and reuse its MCP session. Capture origins for ordinary core requests too, include them in mixed-scope batch validation, and restore them before response handlers execute, including checkpoint replay.

Avoid deep-copying non-migrated request payloads

python/​packages/​core/​agent_framework/​_workflows/​_workflow.py:1205

This unconditionally deep-copies every original request payload, even in a core workflow with no declarative migration policy. An in-process request containing a threading.Lock can be emitted without checkpointing, but answering it now fails with TypeError: cannot pickle '_thread.lock' object before its response handler runs. Avoid copying payloads for read-only validation, and restrict migration snapshots to the metadata that migration actually changes.

1 resolved since last review

• Restore originating scope before forwarding resumed child outputs

_{Review effort: Balanced}

[Vincent Biret (baywet)]

Addressed both summary-only findings from #8662 (review) in e1b150d.

• Ordinary Core requests now capture their originating framework context, participate in mixed-origin validation, and restore it before response/cancellation handlers execute. Nested boundaries use the same bookkeeping. Regression cases cover B-then-A responses, streaming/non-streaming execution, rebuilt file checkpoints, and queued-response replay before invoking a declarative child.
• Response-tree validation no longer deep-copies live request payloads. Validators run read-only first; missing declarative scope metadata is updated only after every validator succeeds. Non-checkpointed requests containing a threading.Lock retain the original dictionary and lock identity through response handling. Existing checkpoint/migration atomicity coverage remains passing.

All eight new regression cases failed for the reported behaviors before the implementation and now pass. Container validation:

docker exec -e PYTHONPATH=/workspace/python/packages/core:/workspace/python/packages/declarative:/workspace/python/packages/openai -w /workspace/python af-8662-review-48ae /tmp/review-venv/bin/python -m pytest packages/core/tests packages/declarative/tests -q -o addopts= -o cache_dir=/tmp/af-8662-core-request-cache --tb=short --disable-warnings

Result: 10,681 passed, 193 skipped, 1 xfailed (exit 0). Ruff lint/format, pinned pyupgrade, source Pyright, and all five required test type checkers also passed. Request-origin documentation was updated alongside the implementation.

The existing maintainer-guidance discussions remain open; this batch does not change the dependency floor or enable the unsupported workflow-backed-agent call path.

[Copilot]

🟡 Changes recommended

Critical validation gaps in cancellation preflight and wrapped-reply batches can mix response origins and execute downstream work under the wrong restored scope.

2 open issues

• Validate wrapped child replies across response batches · New
• Validate queued reply origins during cancellation preflight · New

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Manually composed Core workflows retain a stale declarative scope, causing fresh runs to reuse cached stateful MCP clients and child workflows to ignore inherited parent-scope changes.

No open issues 🎉

2 resolved since last review

• Validate queued reply origins during cancellation preflight
• Validate wrapped child replies across response batches

_{Review effort: Balanced}

[Vincent Biret (baywet)]

Addressed the manual-composition concern in Copilot review 5675089233 in 7e45ede. Declarative executors manually composed with Core WorkflowBuilder now use the active Core scope, including inherited child scopes, rather than a stale declarative cache. Factory scope initialization and standalone state-helper behavior are preserved.

Test-first evidence: all ten new parameterized cases failed on unchanged 16b1655 because separate fresh runs reused a scope, then passed with the fix. Coverage includes direct/nested streaming and non-streaming runs, explicit/sessionless agent continuity, rebuilt entry-checkpoint replay, and interleaved approval continuations with live/checkpoint-restored state.

Validation passed: full containerized Core/declarative pytest suites (10,701 passed, 193 skipped, 1 xfailed), Ruff lint/format, pinned pyupgrade, strict source Pyright, and all five repository-configured test type checkers. The existing OpenTelemetry shutdown exporter warning remains non-fatal. The separate “statement has no effect” finding on await run was answered and resolved with execution evidence; that await is required to drive the lazy non-streaming continuation.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because same-ID request relays can break MCP session continuity, while repeated origin-map copying introduces quadratic event-loop work during large fan-outs.

No open issues 🎉

_{Review effort: Balanced}

[Eduard van Valkenburg (eavanvalkenburg)]

Vincent Biret (@baywet) I independently reproduced two current-head blockers from the latest automated review:

1. In python/packages/core/agent_framework/_workflows/_workflow_executor.py:787-789, a coordinator can relay a child request with the original request ID after Core has already consumed that origin before the coordinator response handler. Restoring the same ID again then falls back to a new UUID. In both streaming and non-streaming reproductions, downstream calls through the same MCP binding changed scope across the pause/resume boundary; a distinct relay request ID preserved continuity. Please keep relay and child-boundary origins independently owned, or otherwise prevent the same origin from being consumed twice.

2. In python/packages/core/agent_framework/_workflows/_request_context.py:62-70, each capture and restore reads, validates, deep-copies, and rewrites the complete pending-origin map. A plain Core workflow issuing and resolving 2,000 requests took 16.81 seconds on this head versus 0.071 seconds on the base; 1,000 requests took 4.23 seconds versus 0.039 seconds. Please make origin insertion and removal incremental so request fan-out does not add quadratic synchronous event-loop work.

Please address both before approval.

[Vincent Biret (baywet)]

Eduard van Valkenburg (@eavanvalkenburg) Both blockers are addressed in commit f0f1906, now pushed to this PR branch.

1. Wrapped child origins are now owned separately by each binding, independently of the coordinator's external request origin. Reusing the child's request ID no longer consumes the boundary origin before the wrapped continuation. The regression exercises the same downstream MCP binding before and after the pause, interleaved runs, streaming/non-streaming execution, file-checkpoint restoration, and queued checkpoint responses. All six variants failed before the fix and pass afterward.
2. Capture/restore now read and stage individual mapping entries rather than validating and deep-copying the complete pending-origin map for every request. State commits apply the staged entry deltas; discard preserves the previous committed state. Full-map validation remains at response-batch/checkpoint boundaries. Added regressions for linear copying work, snapshot isolation, rollback, whole-value replacement/deletion, malformed entries, independently owned same-ID origins, and legacy boundary-origin restoration.

The same container benchmark for issuing and resolving plain Core requests measured:

Requests	Original capture/restore	Fixed
1,000	4.403120s	0.083152s
2,000	18.871806s	0.173432s

Validation used Python 3.12 in the existing .NET-enabled container, with the local Core/declarative source roots on PYTHONPATH:

• python -m pytest packages/core/tests/workflow/test_state.py packages/core/tests/workflow/test_sub_workflow.py packages/core/tests/workflow/test_workflow.py packages/core/tests/workflow/test_checkpoint.py packages/core/tests/workflow/test_workflow_agent.py packages/declarative/tests/test_invoke_mcp_tool_executor.py -m 'not integration' -q --disable-warnings -p no:cacheprovider --tb=short — passed.
• After extracting the shared relay fixture: python -m pytest packages/core/tests/workflow/test_state.py packages/declarative/tests/test_invoke_mcp_tool_executor.py -m 'not integration' -o addopts= -q -p no:cacheprovider --tb=short — 386 passed (one existing experimental-feature warning).
• After adding the legacy-origin compatibility case: python -m pytest packages/core/tests/workflow/test_state.py -o addopts= -q -p no:cacheprovider --tb=short — 45 passed.
• Ruff formatting/lint, strict source Pyright, and all five configured test type checkers (Pyright, mypy, pyrefly, ty, zuban) — passed on the changed files.

The unrelated reflection thread remains open for maintainer guidance.

[Copilot]

🟡 Changes recommended

Preflight ignores edge conditions and fan-out selection, so it can incorrectly reject a valid single-child reply as a mixed-scope batch when request IDs overlap across parent scopes.

1 open issue

• Validate only selected routes during reply dispatch · New

_{Review effort: Balanced}

[Copilot]

🟡 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

• Cached wrapped-reply routes ignore earlier same-edge state changes · New
• Preflight routes wrapped replies before restoring checkpointed executor state · New

1 resolved since last review

• Validate only selected routes during reply dispatch

_{Review effort: Balanced}

[Vincent Biret (baywet)]

Eduard van Valkenburg (@eavanvalkenburg) Reproduced on 517ebf1 with local regression tests: an earlier handler on the same edge changes the field read by the routing callback, then a targeted wrapped reply follows it. The field changes as expected, but the reply is dropped because preflight cached the earlier decision. All four variants fail for that behavior (condition/selection × streaming/non-streaming).

Before changing the implementation, could you confirm the intended validation boundary? Preserving normal routing order requires evaluating the callback after earlier handlers have executed. Those handlers can have arbitrary side effects, so speculative execution is not generally safe. Validating each selected delivery immediately before consuming its replies preserves dynamic routing, but would narrow the current whole-batch guarantee that mixed parent scopes reject before any reply is consumed. Alternatively, retaining whole-batch preflight would require an explicit stable/snapshot-aware routing contract rather than assuming arbitrary callbacks are message-pure.

The regression tests are local and uncommitted; implementation is unchanged. The existing 16 routing/control cases still pass. Leaving this thread open pending guidance rather than silently changing either contract. The checkpoint-state finding raises the same contract question.

[Vincent Biret (baywet)]

Implemented in 1e4d973. Wrapped replies now wait for preceding messages from their source, select routes once at delivery time, and validate every selected child before dispatch. Ordinary messages retain concurrent edge delivery. Selected parent scopes are retained through the superstep so later replies cannot evade mixed-scope rejection after earlier origins are released.

This intentionally uses a narrower rejection boundary: earlier handlers/child continuations are not rolled back; the rejected and subsequent undelivered messages remain queued. Recovery after partial execution requires a known-good checkpoint. The ADR, package guidance, consumer guidance and rejection-boundary tests document that behavior.

Validation: the original eight routing regressions pass without changing their assertions; 391 affected subworkflow/declarative MCP tests and 305 neighboring runner/edge/checkpoint/State/workflow tests pass. Ruff, strict source Pyright and all five test-checker profiles pass, including Zuban across 105 files with the explicit interpreter. Leaving this thread open for the remaining contract discussion.

[Vincent Biret (baywet)]

Eduard van Valkenburg (@eavanvalkenburg) Reproduced on 517ebf1 with four local cases (condition/selection × streaming/non-streaming). The candidate checkpoint selects the second child while the live executor selects the first. Restore correctly restores the field to the second child, but dispatch still resumes the first child using the preflight-cached decision.

The callbacks in this reproduction close over the live coordinator. Copying the checkpoint or executor does not rebind those closures, and invoking arbitrary restore hooks speculatively can have side effects. We therefore need agreement on a supported snapshot-aware routing contract, or on deferring state-dependent route planning/validation until restored state is live; the latter changes the current restore-time batch-rejection guarantee.

The related validation-boundary question and reproduction details are in #8662 (comment). These tests remain local and uncommitted; no implementation change or thread resolution is being claimed. Could you advise which contract the fix should preserve?

[Vincent Biret (baywet)]

Implemented in 1e4d973. Detached checkpoint validation no longer evaluates or caches wrapped-reply routes using live executor fields. Wrapped routing and child-policy validation run at delivery after checkpointed executor fields are restored; each predicate/selector is evaluated once. All four checkpoint-state regression variants now pass with their original assertions.

Graph/structural and ordinary-response checkpoint preflight remain. A wrapped-reply policy rejection now happens after restore, so it does not undo restored state or earlier continuations. Tests explicitly verify that boundary, pending-request preservation for the rejected child, unchanged input checkpoint contents, and checkpoint-based recovery. The ADR and package/consumer guidance describe the trade-off. Full local validation: 696 tests passed, Ruff and strict source Pyright passed, and all five test-checker profiles passed. Leaving this thread open for the remaining contract discussion.

[Copilot]

🔵 Needs a closer look

Checkpoint preflight caches wrapped-reply routing decisions before restoring executor state, causing dispatch to use stale targets that can drop queued replies or resume the wrong child.

2 open issues

• Preflight routes wrapped replies before restoring checkpointed executor state
• Cached wrapped-reply routes ignore earlier same-edge state changes

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The scope guard resets each superstep, allowing staggered cross-scope replies to make delayed actions use another scope’s MCP session.

3 open issues

• Preserve reply scope across supersteps to prevent session mix-up · New
• Preflight routes wrapped replies before restoring checkpointed executor state
• Cached wrapped-reply routes ignore earlier same-edge state changes

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate legacy-checkpoint session-continuity risks and quadratic validation costs when processing wrapped replies.

2 open issues

• Preflight routes wrapped replies before restoring checkpointed executor state
• Cached wrapped-reply routes ignore earlier same-edge state changes

1 resolved since last review

• Preserve reply scope across supersteps to prevent session mix-up

_{Review effort: Balanced}

[Eduard van Valkenburg (eavanvalkenburg)]

Vincent Biret (@baywet), this await couples all outgoing edges before the wrapped child reply is delivered. An unrelated edge can be waiting for a signal produced by that child reply, so the barrier withholds the reply that would release the waiter and the workflow deadlocks. I reproduced that cycle on 4d515ae7b83418208cb96e90a1b4c6ac29277f45; the same scenario completes on the base. Please preserve ordering only for the affected edge or selected target path, without awaiting unrelated outgoing handlers, and add a regression with separate waiter and child edges.