Skip to content

fix(client): replace authorization headers case-insensitively - #584

Open
Yusef Syed (YusefSyed) wants to merge 1 commit into
microsoft:mainfrom
YusefSyed:codex/client-bearer-header-case
Open

fix(client): replace authorization headers case-insensitively#584
Yusef Syed (YusefSyed) wants to merge 1 commit into
microsoft:mainfrom
YusefSyed:codex/client-bearer-header-case

Conversation

@YusefSyed

Copy link
Copy Markdown
Contributor

Passing an explicit bearer key alongside an existing lowercase or mixed-case authorization header currently sends both the old and new values. This also occurs with httpx.Headers, which normalizes keys to lowercase. Build the merged headers with httpx.Headers so the explicit key replaces any existing authorization value case-insensitively in both clients.

Tests exercise actual requests through MockTransport, preserve unrelated headers and the caller's header object, and retain existing authorization when no key is supplied. Six cases fail on the unchanged implementation. The client, controller, server, and package test selection passes all 46 cases; scoped Ruff, formatting, Pyright, and diff checks pass.

AI assistance: Codex helped investigate, implement, and validate this change.

@YusefSyed
Yusef Syed (YusefSyed) marked this pull request as ready for review September 4, 2026 20:13
Copilot AI balanced review requested due to automatic review settings September 4, 2026 20:13

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 implementation is focused, correct, and adequately covered by regression tests.

Pull request overview

Fixes duplicate authorization headers when an explicit bearer key is provided.

Changes:

  • Uses httpx.Headers for case-insensitive replacement.
  • Adds sync and async regression coverage, including immutability checks.
File summaries
File Description
agentlightning/client.py Correctly merges authorization headers.
tests/test_client.py Tests replacement and preservation behavior.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using httpx.Headers at the merge boundary is the right fix because header names are case-insensitive while a plain dict preserves duplicate spellings. The regression is stronger than a unit helper check because it verifies the actual emitted request contains exactly one Authorization value and that caller-owned headers are not mutated. The no-key path also preserves the existing credential contract. I don’t see a blocker here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants