Skip to content

Python: Bind MCP HTTP sessions to request identity - #8303

Open
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
microsoft:mainfrom
eavanvalkenburg:cuddly-spork
Open

Python: Bind MCP HTTP sessions to request identity#8303
Eduard van Valkenburg (eavanvalkenburg) wants to merge 1 commit into
microsoft:mainfrom
eavanvalkenburg:cuddly-spork

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

Keep each streamable HTTP connection aligned with the request headers that established its session.

Description & Review Guide

  • What are the major changes? Bind framework-created sessions to a normalized provider header set, reconnect when that set changes, and refresh session-derived discovery state.
  • What is the impact of these changes? Calls and ambient connection requests use one consistent header identity; changed headers establish a fresh session before the call is sent.
  • What do you want reviewers to focus on? Reconnect cleanup, cancellation behavior, and discovery-state replacement.
  • Validation: Core MCP suite and all 5,043 core unit tests pass; changed MCP files pass Pyright and core syntax checks.

Related Issue

None.

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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: efc43d46-4f87-4702-9f55-9f5f596c33b7

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Cross-identity discovery can leak stale tools, while cancellation and caller-owned sessions are not handled safely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Binds MCP streamable HTTP sessions to normalized request-header identities.

Changes:

  • Reconnects when provider headers change.
  • Refreshes discovery state and cancellation cleanup.
  • Adds identity, concurrency, failure, and redirect tests.
File summaries
File Description
python/packages/core/agent_framework/_mcp.py Implements session identity binding and reconnect logic.
python/packages/core/tests/core/test_mcp.py Updates redirect and test subclass behavior.
python/packages/core/tests/core/test_mcp_http_auth.py Adds HTTP identity and lifecycle tests.
python/packages/core/AGENTS.md Documents header-bound sessions.
Review details

Suppressed comments (2)

python/packages/core/agent_framework/_mcp.py:3907

  • A cancelled reconnect can leave the original session connected but erase all of its discovery state. For example, the new cancellation test makes connect(reset=True) raise before teardown; this line has already removed functions, metadata, and progressive selections, while the exception path only discards pending headers. Preserve and restore the old discovery state when cancellation leaves the existing session connected, or clear it only after reset teardown is committed; the cancellation test should also assert that the original functions remain available.
                    self._clear_session_discovery_state()

python/packages/core/agent_framework/_mcp.py:3907

  • Rebinding only when a tool is invoked is too late to isolate discovery between request identities. Agent setup copies tool.functions into its run tool list before any call (_agents.py:1464-1488), so a run for token B on a tool still connected as token A is shown token A's tool names/schemas (and cannot see B-only tools). This reconnect recreates the internal functions, but it cannot replace the already-copied run list. Resolve/rebind the provider identity during run tool preparation, before functions are exposed to the model, and update the live run list when discovery changes.
                elif self.is_connected and identity != self._session_header_identity:
                    await self._cancel_pending_reload_tasks()
                    self._clear_session_discovery_state()
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +3905 to +3910
elif self.is_connected and identity != self._session_header_identity:
await self._cancel_pending_reload_tasks()
self._clear_session_discovery_state()
self._stage_session_headers(headers, kwargs)
try:
await self.connect(reset=True)

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): ec99cf9c9873
Model: gpt-5.6-sol-fast

Overview

The change normalizes provider headers into a session identity, serializes identity-sensitive calls, stages replacement credentials before transport initialization, and drains notification reloads during teardown. The shared-client origin guard and new integration tests provide strong coverage for direct tool calls and failed reconnects. However, identity reconciliation occurs after an Agent has already exposed discovery from the prior principal, and cancellation can leave a live session with its discovery state erased.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_mcp.py

async with self._call_headers_lock:
if self.is_connected and self._session_header_identity is None:
self._bind_session_headers(headers)
elif self.is_connected and identity != self._session_header_identity:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this connected tool is reused by a run with different headers, Agent preparation has already copied tool.functions from the previous principal before this check runs. The new principal therefore sees stale names and schemas, cannot select its own principal-specific tools, and a stale captured remote name can still be sent after reconnection. Please bind/reconnect with the run's headers and refresh discovery before the Agent snapshots the model-facing function list.

self._bind_session_headers(headers)
elif self.is_connected and identity != self._session_header_identity:
await self._cancel_pending_reload_tasks()
self._clear_session_discovery_state()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clears functions and metadata before connect(reset=True) acquires lifecycle ownership. If the caller is cancelled while waiting for that reconnect, is_connected remains true and the exception path only discards the staged headers, leaving the old live session with empty discovery and parameter/task maps. Please defer the clear until the old session is committed to closing, or restore the prior discovery state whenever the old connection survives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants