Python: Add User-Agent header for Anthropic API calls#13579
Python: Add User-Agent header for Anthropic API calls#13579mikelambert wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Applies the existing Semantic Kernel user-agent infrastructure to the Anthropic connector, matching what's already done for OpenAI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@microsoft-github-policy-service agree [company="Anthropic"] |
|
@microsoft-github-policy-service agree company="Anthropic" |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 91%
✓ Correctness
This PR adds default_headers support to the Anthropic chat completion connector, following the identical pattern already established in open_ai_config_base.py. The header merging logic (copy default_headers, merge APP_INFO, prepend user agent) is correct and consistent with the OpenAI implementation. The three new tests adequately cover the cases: APP_INFO present, APP_INFO with custom headers, and APP_INFO absent. No bugs or correctness issues found.
✓ Security Reliability
This PR adds default_headers support and User-Agent telemetry to the Anthropic chat completion connector. The implementation follows the identical pattern already established in OpenAIConfigBase and AzureConfigBase (copy headers, merge APP_INFO, prepend User-Agent). The code correctly uses copy() to avoid mutating the caller's Mapping, safely handles the None cases for both default_headers and APP_INFO, and only applies headers when constructing a new client (consistent with existing connectors). Tests cover all three branches (APP_INFO present, merged with custom headers, APP_INFO absent). No security or reliability concerns found.
✓ Test Coverage
The three new tests cover the main scenarios for the header-merging logic (APP_INFO present, APP_INFO with custom headers, APP_INFO absent). However, there are notable gaps: no test verifies that passing
default_headerswhenasync_clientis also provided silently drops those headers; no test covers passing customdefault_headerswhenAPP_INFOisNone(they should still be forwarded); and no test asserts the originaldefault_headersmapping is not mutated (thecopy()exists but is unverified).
✗ Design Approach
This PR adds
default_headerssupport and APP_INFO telemetry injection to the Anthropic connector, mirroring the existing pattern inazure_config_base.py. The implementation is largely correct and consistent with the rest of the codebase. However, there is a silent no-op bug: when a caller supplies bothasync_clientanddefault_headers, thedefault_headersare computed (including the APP_INFO merge) but then discarded because the pre-built client is used directly. This parameter silently has no effect in that code path, with no warning and no documentation note. Additionally, the merge order means APP_INFO overwrites any user-supplied header with the same key (e.g., a customsemantic-kernel-version), which matches the existing pattern but is arguably inverted precedence.
Flagged Issues
-
default_headersis silently ignored whenasync_clientis provided. The merged headers are computed but never applied to an externally-supplied client, so passing bothasync_clientanddefault_headersis a silent no-op. At minimum a warning should be emitted; alternatively, the parameter should be documented as ignored whenasync_clientis provided, or an error should be raised when both are given together.
Suggestions
- The merge order (
merged_headers.update(APP_INFO)) means APP_INFO silently overwrites any user-supplied header with the same key (e.g.,semantic-kernel-version). This is consistent withazure_config_base.pybut the precedence is arguably backwards — internal telemetry should not trump user-provided values. Consider reversing the order so user values always win. - The
dict(copy(default_headers))pattern is redundant:dict(default_headers)already creates a new dict from anyMapping. Thecopy()call (and its import) can be removed. - Add tests for: (1) both
async_clientanddefault_headersprovided together (to document/guard the silent-ignore behavior), (2) customdefault_headerswithAPP_INFO=None(headers should still be forwarded), and (3) verifying the caller's originaldefault_headersmapping is not mutated by the merge logic.
Automated review by moonbox3's agents
|
|
||
| if not async_client: | ||
| async_client = AsyncAnthropic( | ||
| api_key=anthropic_settings.api_key.get_secret_value(), |
There was a problem hiding this comment.
merged_headers is computed (including APP_INFO injection) for every call, but is only consumed in the if not async_client branch. When a caller passes a pre-built async_client, default_headers is silently discarded. Either emit a logger.warning here when both are provided, or raise early.
| api_key=anthropic_settings.api_key.get_secret_value(), | |
| if async_client and default_headers: | |
| logger.warning( | |
| "The `default_headers` parameter is ignored when `async_client` is provided. " | |
| "Set headers directly on the supplied client instead." | |
| ) | |
| # Merge APP_INFO into the headers if it exists | |
| merged_headers = dict(default_headers) if default_headers else {} | |
| if APP_INFO: | |
| merged_headers.update(APP_INFO) | |
| merged_headers = prepend_semantic_kernel_to_user_agent(merged_headers) |
| if not anthropic_settings.chat_model_id: | ||
| raise ServiceInitializationError("The Anthropic chat model ID is required.") | ||
|
|
||
| # Merge APP_INFO into the headers if it exists |
There was a problem hiding this comment.
dict(copy(default_headers)) is redundant — dict(default_headers) already produces a new dict from any Mapping. The copy() call (and the import) can be removed.
| # Merge APP_INFO into the headers if it exists | |
| merged_headers = dict(default_headers) if default_headers else {} |
|
|
||
| call_kwargs = mock_async_anthropic.call_args.kwargs | ||
| headers = call_kwargs["default_headers"] | ||
| assert headers == {} |
There was a problem hiding this comment.
This test only covers APP_INFO=None without custom headers. Consider adding sibling tests for: (1) default_headers={"X-Custom": "value"} with APP_INFO=None to verify custom headers are still forwarded, and (2) both async_client and default_headers provided to document the expected behavior (warning, error, or documented no-op).
Motivation and Context / Description
Applies the existing Semantic Kernel user-agent infrastructure to the Anthropic connector, matching what's already done for OpenAI. This helps ensure the user-agents are plumbed more consistently across backends.
Contribution Checklist