Python: Fix FoundryChatClient and FoundryAgent silently dropping default_headers#5466
Python: Fix FoundryChatClient and FoundryAgent silently dropping default_headers#5466moonbox3 wants to merge 5 commits intomicrosoft:mainfrom
FoundryChatClient and FoundryAgent silently dropping default_headers#5466Conversation
…t and pre-built client path - _chat_client.py: pass default_headers into project_client.get_openai_client() so custom headers reach the underlying AsyncOpenAI instance instead of being stored only on self.default_headers (dead field). - _agent.py: same fix for RawFoundryAgentChatClient. - _shared.py: when a pre-built async_client is provided, apply merged_headers (custom headers + APP_INFO) to client._custom_headers so they are not silently dropped in the early-return path. - Add regression tests covering all three bug sites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ng suites Move regression coverage for default_headers forwarding from the standalone repro file into test_foundry_chat_client.py and test_foundry_agent.py, where similar init tests already live. - test_foundry_chat_client: add two tests verifying get_openai_client receives the headers dict (or None) on construction - test_foundry_agent: same coverage for RawFoundryAgentChatClient - remove tests/foundry/test_5416_default_headers_dropped.py (standalone repro file created during reproduction phase) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tring - Delete REPRODUCTION_REPORT.md (dev artifact, not source code) - Remove 'microsoft#5416' issue-number reference from test docstring in test_openai_shared.py to match codebase conventions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 96% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Design Approach
Automated review by moonbox3's agents
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Fixes a regression/bug in the Python Foundry + OpenAI integrations where default_headers provided to FoundryChatClient, FoundryAgent (and pre-built OpenAI clients via load_openai_service_settings) were accepted but not applied to outbound OpenAI SDK requests.
Changes:
- Forward
default_headersintoAIProjectClient.get_openai_client(...)in Foundry chat client and agent client initialization. - Ensure
load_openai_service_settings(...)applies merged headers to a pre-built OpenAI client viaclient.with_options(default_headers=...)before early-returning. - Add unit tests covering the three regression points (Foundry chat client, Foundry agent chat client, OpenAI shared settings helper).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/packages/openai/agent_framework_openai/_shared.py | Applies merged default headers to pre-built clients before returning from load_openai_service_settings. |
| python/packages/openai/tests/openai/test_openai_shared.py | Adds tests validating header application behavior for pre-built clients. |
| python/packages/foundry/agent_framework_foundry/_chat_client.py | Forwards default_headers into project_client.get_openai_client(...). |
| python/packages/foundry/agent_framework_foundry/_agent.py | Forwards default_headers into project_client.get_openai_client(...) for agent client setup. |
| python/packages/foundry/tests/foundry/test_foundry_chat_client.py | Adds tests asserting forwarding behavior to get_openai_client. |
| python/packages/foundry/tests/foundry/test_foundry_agent.py | Adds tests asserting forwarding behavior to get_openai_client for agent chat client. |
…ness (microsoft#5416) The test_load_openai_service_settings_no_headers_still_applies_app_info test was implicitly relying on APP_INFO being non-None. When a developer runs with AGENT_FRAMEWORK_USER_AGENT_DISABLED=true, APP_INFO is None, merged_headers stays empty, with_options is never called, and the assert_called_once() assertion fails. Fix: wrap the load_openai_service_settings call in so the test is deterministic regardless of environment configuration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 88%
✓ Correctness
This PR fixes a bug where default_headers were silently dropped when a pre-built client was passed to load_openai_service_settings, and forwards default_headers to get_openai_client() in the Foundry package. The production code changes are correct. One test is fragile: test_load_openai_service_settings_no_headers_still_applies_app_info relies on APP_INFO being truthy (telemetry enabled), but APP_INFO is None when the AGENT_FRAMEWORK_USER_AGENT_DISABLED env var is set to 'true', which would cause the test to fail.
✓ Security Reliability
This PR fixes a bug where custom default_headers were silently dropped when a pre-built OpenAI client was provided. The foundry package now forwards default_headers into get_openai_client(), and load_openai_service_settings() now applies merged headers (including APP_INFO telemetry) to pre-built clients via with_options(). The changes are sound from a security and reliability perspective: headers are defensively copied via dict(), the with_options() call correctly gates on merged_headers being non-empty, and tests cover both the headers-present and headers-absent paths.
✓ Test Coverage
The tests for forwarding default_headers in the foundry agent and chat client are well-structured and cover both the 'with headers' and 'without headers' cases. The two new tests for
load_openai_service_settingscover the OpenAI code path (non-Azure branch), but neither test exercises the identical fix applied to the Azure code path (right-side lines 265-266 of_shared.py). Since both branches independently handle the pre-built client + merged_headers logic, a test that routes through the Azure branch (e.g., by providing anAsyncAzureOpenAI-spec'd mock asclient) would close this gap.
✗ Design Approach
The shared
load_openai_service_settings()change addresses the real gap: prebuilt OpenAI clients were bypassing the common header/telemetry decoration path. The extra Foundry changes go beyond that fix and push header handling down intoAIProjectClient.get_openai_client(...), even though the Foundry wrappers already pass bothasync_clientanddefault_headersintoRawOpenAIChatClient, which centralizes that work through the shared helper. That duplicates responsibility and couples the Foundry layer to an Azure SDK-specific transport detail instead of keeping header application in one place.
Flagged Issues
- Foundry now applies
default_headersin two layers: once viaAIProjectClient.get_openai_client()and again in the sharedload_openai_service_settings(). SinceRawFoundryChatClient/RawFoundryAgentChatAgentalready forward bothasync_clientanddefault_headersintoRawOpenAIChatClient(which routes through the shared helper), the Foundry-specific forwarding duplicates responsibility and deepens coupling to an Azure SDK transport detail instead of relying on the shared prebuilt-client path.
Suggestions
- Keep
project_client.get_openai_client()as a plain client factory in Foundry, and letload_openai_service_settings()remain the single place that mergesdefault_headersand telemetry headers onto prebuilt clients.
Automated review by moonbox3's agents
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 94%
✓ Correctness
The change wraps the test body in a patch context that sets APP_INFO to a non-empty dict. This is required for correctness: _shared.py lines 201-203 only call client.with_options when merged_headers is truthy, and merged_headers is only non-empty when either default_headers or APP_INFO is non-empty. Without the patch the assertion pre_built.with_options.assert_called_once() would fail non-deterministically depending on whether APP_INFO was populated at runtime. The patch makes the precondition explicit and the test reliably exercises the intended code path. No bugs found.
✓ Security Reliability
The change correctly scopes a
unittest.mock.patcharound the test to ensureAPP_INFOis non-empty during the call, making the assertion onpre_built.with_optionsdeterministic. No security or reliability issues are introduced.
✓ Test Coverage
The patch correctly makes the test deterministic by forcing APP_INFO to a known non-empty value so that the
if merged_headers:guard in _shared.py (line 202) is satisfied andwith_optionsis reliably invoked. Without the patch the test was either non-deterministic (passing only when APP_INFO happened to be truthy in the test environment) or always failing. The fix is sound. The only gap is that the test never checks that the APP_INFO content was actually forwarded towith_options, which is the core behaviour the docstring advertises — the sibling test (lines 109–110) does verify header content but this one stops atassert_called_once().
✗ Design Approach
The test change makes the scenario deterministic, but it does so by patching in a value that does not match the production contract for
APP_INFO. In production,APP_INFOis just telemetry metadata andload_openai_service_settingsadds theUser-Agentseparately, so injecting{"User-Agent": ...}here weakens the test and can let regressions in that composition logic slip through.
Flagged Issues
- In
python/packages/openai/tests/openai/test_openai_shared.py,APP_INFOis patched with{"User-Agent": "test-agent"}, which is the composed output rather than the realAPP_INFOshape. ProductionAPP_INFO(defined inagent_framework/_telemetry.py:18-24) contains keys likeagent-framework-version; it isagent_framework_openai/_shared.py:173-177that builds theUser-Agentheader from it. Patching the already-composed value bypasses that logic and allows regressions in the composition step to go undetected.
Suggestions
- After
pre_built.with_options.assert_called_once(), add an assertion that the composed header was actually forwarded, e.g.assert pre_built.with_options.call_args.kwargs.get('default_headers', {}).get('User-Agent') == 'test-agent'. This fully validates the behaviour described in the test name and docstring, consistent with the pattern used in the sibling test at lines 109–110. (Note: this assertion becomes meaningful only once the patch value is corrected to a realistic telemetry-shaped dict as described in the blocking issue.)
Automated review by moonbox3's agents
…tClient(default_headers=...) and FoundryAgent(default_headers=...) silently drop custom headers
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 90%
✓ Correctness
The test change correctly updates the patch targets to match the real APP_INFO structure (a dict with a non-User-Agent key) and separately patches AGENT_FRAMEWORK_USER_AGENT to control the User-Agent value. The new assertion verifying User-Agent propagation through with_options is logically consistent with the _shared.py implementation: merged_headers.update(APP_INFO) populates a non-User-Agent key, then prepend_agent_framework_to_user_agent sets User-Agent from the patched AGENT_FRAMEWORK_USER_AGENT. No bugs found.
✓ Security Reliability
The diff updates a test to patch APP_INFO with the correct key ('agent-framework-version' instead of 'User-Agent') and adds a second patch for AGENT_FRAMEWORK_USER_AGENT so that prepend_agent_framework_to_user_agent uses a known value, then asserts the resulting User-Agent header. The patch targets are correct: APP_INFO is patched at its import site (agent_framework_openai._shared.APP_INFO) and AGENT_FRAMEWORK_USER_AGENT is patched at its definition site (agent_framework._telemetry.AGENT_FRAMEWORK_USER_AGENT), which is where prepend_agent_framework_to_user_agent reads it. No security or reliability issues were identified.
✓ Test Coverage
The diff improves the test
test_load_openai_service_settings_no_headers_still_applies_app_infoby: (1) updating theAPP_INFOpatch to match the real structure (agent-framework-versionkey rather thanUser-Agent), (2) adding a patch forAGENT_FRAMEWORK_USER_AGENTinagent_framework._telemetry, and (3) adding a meaningful assertion thatUser-Agentis correctly set in the headers passed towith_options. The patch targets are correct —get_user_agent()in_telemetry.pyreadsAGENT_FRAMEWORK_USER_AGENTas a module-level global, sopatch('agent_framework._telemetry.AGENT_FRAMEWORK_USER_AGENT', ...)will affect its return value. One fragility worth noting: the test does not patchIS_TELEMETRY_ENABLEDinagent_framework._telemetry. If theAGENT_FRAMEWORK_USER_AGENT_DISABLEDenv var is set totruein the test environment,prepend_agent_framework_to_user_agentreturns headers unchanged (withoutUser-Agent), causing the new assertion to fail. Similarly,get_user_agent()calls_detect_hosted_environment()which may prepend hosted-env prefixes from_user_agent_prefixesif theFOUNDRY_HOSTING_ENVIRONMENTenv var is set, making the User-Agent string not match the exact expected value. These are not correctness bugs in normal CI but could cause flaky failures in specialised environments.
Suggestions
- Consider patching
agent_framework._telemetry.IS_TELEMETRY_ENABLEDtoTrueinside thewithblock so the User-Agent assertion cannot silently fail when theAGENT_FRAMEWORK_USER_AGENT_DISABLEDenv var is set in the test environment. - Consider patching
agent_framework._telemetry._user_agent_prefixesto an empty set inside thewithblock — if_detect_hosted_environment()was previously invoked (e.g. viaFOUNDRY_HOSTING_ENVIRONMENT), the prepended prefix causesget_user_agent()to return a value likefoundry-hosting/agent-framework-python/test-versionthat will not match the exact asserted string.
Automated review by moonbox3's agents
|
Handled in #5447 |
Motivation and Context
default_headerspassed toFoundryChatClientandFoundryAgentwas accepted and stored on the instance but never forwarded to the underlyingAsyncOpenAIclient, so custom headers were silently dropped and never sent on outbound HTTP requests. Additionally, theload_openai_service_settingshelper inagent_framework_openaiwould early-return a pre-built client without applying any merged headers, compounding the problem.Fixes #5416
Description
In
_chat_client.pyand_agent.py,project_client.get_openai_client()was called with no arguments; the fix forwardsdefault_headersinto that call so theAsyncOpenAIclient is constructed with the correct headers from the start. In_shared.py, both early-return paths for a pre-builtclientnow callclient.with_options(default_headers=merged_headers)before returning, ensuring custom headers are applied to any externally-supplied client as well. Tests were added for all three fix sites to prevent regression.Contribution Checklist