fix(gateway): preserve HTTPX event hooks#6327
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
github-actions
Bot
added
size: S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe gateway provider now preserves caller-provided Sequence Diagram(s)Included in the hidden review stack artifact above. Related Issues: Suggested labels: bug, providers Suggested reviewers: DouweM, samuelcolvin 🐰 A hook that once would sweep hooks clean, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/providers/test_gateway.py (1)
75-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a note on why these aren't VCR tests.
Both new tests pin exact internal hook ordering/dedup behavior by manually invoking
event_hooks['request']on a request object, without any real HTTP call — cassette playback wouldn't exercise or catch regressions here. As per coding guidelines, "each unit test should explain why it is not (or cannot be) a VCR test, especially when it pins internal behavior... that cassettes would not reliably catch."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/providers/test_gateway.py` around lines 75 - 119, Add a brief note in both gateway provider tests explaining why they are not VCR tests: they validate internal httpx event hook preservation/replacement behavior by directly inspecting and manually invoking http_client.event_hooks rather than making a real HTTP request. Place the note near test_init_with_http_client_preserves_existing_event_hooks and test_init_with_http_client_replaces_existing_gateway_hook so it is clear that cassette playback would not exercise these hook-ordering and dedup semantics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/providers/test_gateway.py`:
- Around line 75-119: Add a brief note in both gateway provider tests explaining
why they are not VCR tests: they validate internal httpx event hook
preservation/replacement behavior by directly inspecting and manually invoking
http_client.event_hooks rather than making a real HTTP request. Place the note
near test_init_with_http_client_preserves_existing_event_hooks and
test_init_with_http_client_replaces_existing_gateway_hook so it is clear that
cassette playback would not exercise these hook-ordering and dedup semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c210e4f-5202-4c39-a637-1703a82b3b3d
📒 Files selected for processing (2)
pydantic_ai_slim/pydantic_ai/providers/gateway.pytests/providers/test_gateway.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)pydantic/pydantic(auto-detected)
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
David's AICA here: 🔍 Local review suite (automated, pre-merge)
Verdict: ✅ PASS — all 3 waves passed, 0 blocking, 0 open. The core fix (merge gateway hook into ✅ 3 required — RESOLVED1. String-marker A follow-up commit ( 2. Bare 3. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pydantic_ai_slim/pydantic_ai/providers/gateway.py (1)
225-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten
headerstodict[str, str].inject()only writes string header values here, soAnyis unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pydantic_ai_slim/pydantic_ai/providers/gateway.py` at line 225, The `headers` variable in `inject()` is typed too loosely as `dict[str, Any]` even though only string header values are written. Update the `headers` annotation in `gateway.py` to `dict[str, str]` and keep the rest of the injection logic unchanged, using the `inject()` function as the location to make the type more precise.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pydantic_ai_slim/pydantic_ai/providers/gateway.py`:
- Line 225: The `headers` variable in `inject()` is typed too loosely as
`dict[str, Any]` even though only string header values are written. Update the
`headers` annotation in `gateway.py` to `dict[str, str]` and keep the rest of
the injection logic unchanged, using the `inject()` function as the location to
make the type more precise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 602c5ecc-0aa9-4261-85f4-07197dac609d
📒 Files selected for processing (1)
pydantic_ai_slim/pydantic_ai/providers/gateway.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)pydantic/pydantic(auto-detected)
2 participants
Fixes #6326
Fixes a Gateway provider edge case where passing a pre-configured
httpx.AsyncClientcould silently drop the caller's existing event hooks.What Problem This Solves
gateway_provider(..., http_client=client)currently replaces the client's fullevent_hooksmapping with the Gateway request hook.That means a caller-provided client loses any existing
requesthooks and allresponsehooks. These hooks are commonly used for tracing, metrics, request capture, auditing, custom diagnostics, or auth-adjacent behavior. The request can still succeed, so the failure mode is easy to miss: surrounding HTTP client behavior disappears after constructing the Gateway provider.A minimal local reproduction before this change showed:
Change
The Gateway provider now adds its request hook without replacing caller-provided HTTPX hooks.
For repeated
gateway_provider(..., http_client=same_client, ...)calls, the Gateway-owned hook is replaced rather than duplicated. This keeps user hooks intact while ensuring the latest Gateway provider configuration owns the Gateway authorization hook.Bedrock remains unchanged because that Gateway path does not use HTTPX.
Evidence
Focused local validation:
Broader non-live Gateway subset:
Static checks:
Possible call chain / impact
This affects the shared HTTPX Gateway setup used by OpenAI, Groq, Anthropic, and Google Cloud providers. It does not affect Bedrock, which returns before HTTPX client setup.
Checklist