Skip to content

Python: preserve MCP request ownership on low-level sends - #8234

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

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

Conversation

@eavanvalkenburg

@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

Supported MCP dependencies can issue streamable HTTP requests through AsyncClient.send(). That path bypassed the transport wrapper's request ownership marker, so header_provider hooks ignored those requests.

Description & Review Guide

  • What are the major changes? Add a focused send() wrapper that applies the same private request marker as the existing stream and delete paths, plus a regression test for that invariant.
  • What is the impact of these changes? Request-scoped headers continue to work on the low-level send path. The rebased focused suite passes all 75 cases, together with core syntax, source typing, and test typing checks.
  • What do you want reviewers to focus on? Confirm that marking the request immediately before delegation preserves the existing client ownership and lifecycle behavior.

Related Issue

Related to #7841.

Follow-up to #8225, which supplies run kwargs during connection setup; this change preserves request ownership when the MCP dependency sends through AsyncClient.send().

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 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.

🟢 Approval recommended

The change is narrowly scoped, aligns with the existing request-tagging mechanism, and includes a focused regression test covering the fixed behavior.

Pull request overview

This PR fixes MCP request ownership tagging for the low-level httpx.AsyncClient.send() path so request-scoped header_provider hooks consistently apply across all transport request entry points.

Changes:

  • Add a _MCPHeaderScopedClient.send() wrapper that tags outbound httpx.Request objects with the private MCP ownership extension before delegating to the underlying client.
  • Add a regression test ensuring requests sent via .send() carry the expected ownership marker.
File summaries
File Description
python/packages/core/agent_framework/_mcp.py Adds a send() wrapper that marks request ownership via request extensions, aligning with existing wrapped send paths.
python/packages/core/tests/core/test_mcp.py Adds a unit test verifying the ownership tag is applied for requests sent through AsyncClient.send().
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

@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)): c150063a84a7
Model: gpt-5.6-sol-fast

Overview

The PR closes the low-level AsyncClient.send() ownership gap with a focused wrapper method, while the existing identity check, same-origin enforcement, and lifecycle cleanup continue to constrain header injection. The new regression test proves that the marker reaches the underlying transport. However, the wrapper leaves its ownership and injected-header state on caller-owned requests, so reusing one through the shared client can incorrectly apply tool credentials to a later non-tool send.

Reviewed the supplied pull-request 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/core/agent_framework/_mcp.py

return self._client.stream(*args, **self._tagged_kwargs(kwargs))

async def send(self, request: Request, **kwargs: Any) -> Response:
request.extensions[_MCP_HEADER_OWNER_EXTENSION] = self._owner

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 ownership marker remains on the caller-owned Request after send() returns or raises, along with any headers and bookkeeping added by the request hook. If that buffered request is later reused directly through the shared AsyncClient, the hook still classifies it as this tool's request and can attach the tool's credentials to a non-tool send, contrary to the request-scoping contract. Please restore the request's pre-send ownership and injected-header state in a finally block, or otherwise isolate the sent request, so wrapper state does not survive delegation.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Closing this independent PR to sequence the change after #8225. The branch and commit remain intact; after #8225 merges, the branch will be rebased onto updated main and opened as a focused follow-up.

This branch was previously deployed

1 inactive deployment
github-app-auth — c150063a Deployed Sep 10, 2026 by eavanvalkenburg via add_label #22443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants