Skip to content

Python: fix FoundryChatClient/FoundryAgent dropping default_headers#5433

Closed
he-yufeng wants to merge 1 commit intomicrosoft:mainfrom
he-yufeng:fix/foundry-default-headers
Closed

Python: fix FoundryChatClient/FoundryAgent dropping default_headers#5433
he-yufeng wants to merge 1 commit intomicrosoft:mainfrom
he-yufeng:fix/foundry-default-headers

Conversation

@he-yufeng
Copy link
Copy Markdown

Motivation and Context

Fixes #5416.

FoundryChatClient(default_headers=...) and FoundryAgent(default_headers=...) are documented to set additional HTTP headers on outbound requests. The constructor accepted the parameter, stored it on self.default_headers, and then forgot about it — the underlying AsyncOpenAI client was obtained via project_client.get_openai_client() without forwarding the headers, so custom headers never reached the wire.

This is a silent failure: the parameter looks wired up, existing unit test (test_init_with_default_header) even asserts the stored attribute is populated, but the reporter still spent hours tracking down why their header wasn't appearing in requests.

Description

AIProjectClient.get_openai_client(**kwargs) already forwards default_headers (and friends) to AsyncOpenAI, so the fix is just to pass them through from the two Foundry entry points:

  • agent_framework_foundry/_chat_client.py (RawFoundryChatClient.__init__)
  • agent_framework_foundry/_agent.py (FoundryAgent.__init__)

I deliberately didn't touch load_openai_service_settings in the shared openai package — while that is where the headers are dropped for the pre-built-client path, the reported bug is Foundry-specific and going through get_openai_client is the Azure-native way. The existing self.default_headers storage on the instance is kept intact for serialization/reflection.

Tests

  • test_default_headers_forwarded_to_openai_client — regression test: asserts the headers actually land on the get_openai_client call.
  • test_get_openai_client_not_called_with_headers_kwarg_when_unset — don't inject an empty dict when the user didn't ask for headers.
  • Existing test_init_with_default_header still passes (storage attribute behavior unchanged).

Full foundry test suite: 102 passed, 14 skipped (skips are the integration tests needing a real endpoint).

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? No.

default_headers was accepted by the constructor and stored on the
instance but never forwarded to the AsyncOpenAI client obtained from
AIProjectClient.get_openai_client(), so custom headers never reached
outbound requests.

Forward default_headers through get_openai_client(**kwargs) -- the
Azure SDK already supports this kwarg and passes it on to AsyncOpenAI.

Added two regression tests and updated the mock-based header test so
we cover both the stored-attribute path and the actually-forwarded
path.

Fixes microsoft#5416.
@moonbox3
Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/foundry/agent_framework_foundry
   _agent.py1493775%183–184, 188–190, 195–198, 203, 296, 326–327, 339–340, 352–354, 356–357, 359–365, 367–368, 370, 372, 376–377, 564–565, 568, 610
   _chat_client.py1541987%84, 86–88, 92–93, 97, 191, 226, 319, 380, 382, 480, 484–485, 487–490
TOTAL29036347588% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
5783 30 💤 0 ❌ 0 🔥 1m 34s ⏱️

@TaoChenOSU
Copy link
Copy Markdown
Contributor

I think we can also safely remove these
image

@he-yufeng
Copy link
Copy Markdown
Author

Thanks for the approval @TaoChenOSU! The screenshot doesn't render for me in the API — just to make sure I'm removing the right thing, are you suggesting we also drop the now-redundant default_headers=default_headers kwarg from the two super().__init__(...) calls (_chat_client.py:214 and _agent.py:208)? Once the headers are forwarded through AsyncOpenAI, the parent's self.default_headers is a dead store — nothing in packages/openai or packages/foundry reads it back (grepped), it's just recorded "for serialization" in _chat_client.py:454-459 with no consumer.

If that's the intent, I'll push a follow-up that also removes test_init_with_default_header in test_foundry_chat_client.py, since it currently asserts exactly that parent-side self.default_headers bookkeeping. Let me know and I'll turn it around.

@moonbox3
Copy link
Copy Markdown
Contributor

@he-yufeng please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@he-yufeng please accept the CLA

@eavanvalkenburg
Copy link
Copy Markdown
Member

Closing this, as it is already done, with additional setup for the new hosted agents in #5447

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: FoundryChatClient(default_headers=...) and FoundryAgent(default_headers=...) silently drop custom headers

4 participants