Python: fix(core): authenticate MCP connection-lifetime requests with the run's kwargs - #8225
Conversation
Constructor-supplied MCP tools now seed the run's function_invocation_kwargs before connecting, matching the run-supplied path. Adds coverage for the connect-time credential matrix and fixes the API-key sample to close over a construction-time credential.
… provider's key Ambient header resolution previously swallowed every KeyError from header_provider, so a run whose kwargs did not carry the key the provider reads would silently send the initialize handshake unauthenticated and surface as a 401. Only tolerate the KeyError when no connection kwargs were seeded at all, which is the case a caller cannot avoid; a key missing from seeded kwargs is a misconfiguration and now propagates.
There was a problem hiding this comment.
🟡 Changes recommended
Empty kwargs and concurrent runs can still cause unauthenticated or incorrectly authenticated handshakes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes MCP connection-lifetime authentication using run-scoped kwargs.
Changes:
- Caches connecting run kwargs for handshake, discovery, and pings.
- Updates the authentication sample.
- Adds transport and Agent integration tests.
File summaries
| File | Description |
|---|---|
python/samples/02-agents/mcp/mcp_api_key_auth.py |
Updates credential guidance. |
python/packages/core/tests/core/test_mcp.py |
Adds Agent integration coverage. |
python/packages/core/tests/core/test_mcp_http_auth.py |
Tests authentication lifecycle behavior. |
python/packages/core/agent_framework/_mcp.py |
Adds connection-scoped kwargs resolution. |
python/packages/core/agent_framework/_agents.py |
Seeds kwargs before MCP connection. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (6 commit(s)): 05728b7e6f56, a1e05bc02c4c, 2280b1aabe71, b41e4ac626b9, e14c4b253a5a, 5ba7b147942c
Model: gpt-5.6-sol-fast
Overview
The PR correctly makes merged run kwargs available before lazy MCP connection and re-invokes the provider for ambient requests, preserving refresh behavior; request ownership, origin scoping, call serialization, and teardown tests provide strong guardrails. However, a connected tool can reuse one tenant's initialized session for later tenants, and a failed setup can retain seeded credentials for a subsequent unseeded connection.
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
…wargs on failed connect Addresses review on microsoft#8225. An empty mapping could not express the difference between a run that seeded no kwargs and a connection no run ever seeded, so a lazy run with empty kwargs still swallowed the provider's KeyError and sent an unauthenticated handshake. _connection_kwargs is now None until a run seeds it. The seeded kwargs were also only released through close(); a rejected handshake unwinds via _close_and_check_cancelled instead, leaving the failed run's credential for a later unseeded reconnect, so the release now happens there. Documents the connection-lifetime semantics on the public header_provider parameter.
… claim Addresses review on microsoft#8225. is_connected only turns true after initialize returns, so two concurrent runs could both pass the guard and the second would swap the credential out from under the first run's in-flight handshake, authenticating a shared tool as the wrong caller. The seed is now a first-writer claim, released when the connection closes or its setup fails.
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
small nit in the sample, the rest looks good
Co-authored-by: Eduard van Valkenburg <eavanvalkenburg@users.noreply.github.com>
Motivation & Context
header_providerwas only invoked with a run's kwargs fortools/call. Everyconnection-lifetime request — the
initializehandshake, tool and promptdiscovery, and background pings — resolved headers against an empty mapping.
A provider that derives credentials from the run could therefore authenticate
tool calls but never the handshake itself. Against a server that authenticates
initialize, the connection is rejected with a 401 before any tool runs, so theprovider's tool-call path is unreachable.
Description & Review Guide
What are the major changes?
MCPStreamableHTTPToolgains an internal_connection_kwargscache. Both MCPconnect paths in
_prepare_run_contextseed it with the run'sfunction_invocation_kwargsbefore connecting, and ambient header resolutioninvokes the provider with those kwargs.
The design caches the kwargs, not the resolved headers, so the provider is
still re-invoked per request and token-refresh providers keep working. The
kwargs are fixed for a connection's lifetime — the run that establishes the
connection wins — and are released on close.
Ambient resolution also previously swallowed every
KeyErrorfrom theprovider. That tolerance now applies only when nothing was seeded, which is the
one case a caller cannot avoid, since a connection-lifetime request genuinely
has no per-call values. A key missing from seeded kwargs is a
misconfiguration and now propagates instead of silently sending the handshake
unauthenticated and surfacing as an opaque 401.
No public API change.
What is the impact of these changes?
The supported credential/timing combinations, each covered by a test:
async with Agent(...))run())run(tools=[...]))async with Agent(...))One behavior change beyond the fix: a provider indexing a key that the seeded
kwargs do not carry now raises rather than degrading to a 401. This surfaces a
misconfiguration that was previously silent, and is why
samples/02-agents/mcp/mcp_api_key_auth.pyis updated in this PR — it indexedkwargs under
async with Agent(...), where connect precedes any run, so thecredential could never be read. It now closes over the construction-time key,
with guidance on when per-run kwargs apply instead.
What do you want reviewers to focus on?
Whether fixing connection headers to the first run is the right contract for a
shared, long-lived tool, and whether the narrowed
KeyErrorhandling should betreated as a breaking change rather than a bug fix.
Two deliberate non-changes worth confirming:
Agent.__aenter__is left as is. AG-UI'scollect_server_tools()gates onis_connectedand runs before the agent'srun, so deferring the connect would silently drop MCP tools from approval
flows.
_seed_connection_kwargswith areportPrivateUsagesuppression, matching existing precedent in_agent_hooks.pyand_skills.py.Unrelated to the fix: one blank line is removed from
tests/workflow/test_agent_executor.py.mainis out of sync with its ownimport formatting there and the pre-commit hook re-applies the fix, so the
change cannot be dropped from the branch.
Related Issue
Fixes #7841
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.