fix(openai): unify responses upstream header handling - #1029
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors header handling by centralizing auth header extraction and request header application through an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ce229cfcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request refactors header application logic by introducing apply_upstream_auth_headers to replace the more generic apply_request_headers across the OpenAI response and MCP tool loop paths. This change centralizes authentication normalization for external providers. Feedback indicates that the new helper should be updated to support provider-specific authentication headers and to maintain the forwarding of tracing and correlation headers for observability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/header_utils.rs`:
- Around line 350-389: Add a new unit test in header_utils.rs that exercises
apply_upstream_auth_headers with Some(&headers) where headers contains
non-authorization headers but no Authorization header, ensuring the function
does not forward arbitrary headers and instead uses the worker fallback; create
a test named like test_apply_upstream_auth_headers_some_headers_no_caller_auth
(or similar), construct a HeaderMap with e.g. "openai-project" and
"x-custom-header" but no "authorization", call apply_upstream_auth_headers with
that headers Some reference and a worker secret, build the request, and assert
that the resulting request has the worker "authorization" header and does not
contain the other custom headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e9c0e8f-e850-4344-b8d0-bed7ba1f1ee2
📒 Files selected for processing (4)
model_gateway/src/routers/header_utils.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rs
3f8b192 to
228d84a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 228d84a524
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ac4a887 to
3ca031f
Compare
Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
3ca031f to
afdfd0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afdfd0cb2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02d60c8b7f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Description
Problem
/v1/responsesused inconsistent upstream header behavior across code paths.The non-streaming path applied auth explicitly, while the streaming and MCP-assisted paths forwarded incoming request headers more broadly. That caused behavior differences based on
stream: true/falseor whether MCP tool looping was triggered. In particular, headers likeOpenAI-Projectcould reach the OpenAI upstream on only some/responsespaths, and the streaming / MCP paths also missed worker API key fallback when the caller did not provideAuthorization.Solution
Align the streaming and MCP-assisted
/responsespaths with the existing non-streaming behavior by applying provider-aware auth handling directly at each upstream call site.The updated behavior is:
This is now applied consistently in:
/responses/responses/responsesProvider-aware extraction is preserved, so if these paths hit a non-OpenAI upstream in a single-provider setup, headers like
x-api-key/x-goog-api-keystill work correctly.Changes
/responsesto use provider-aware auth extraction inline/responsespassthrough to use the same auth behavior inline/responsespathsapply_request_headershelperx-api-keyTest Plan
Before:
Expected before this change:
/responsescould forwardOpenAI-Projectupstream{ "error": { "message": "OpenAI-Project header should match project for API key", "type": "invalid_request_error", "code": "mismatched_project", "param": null }, "status": 401 }After:
Expected after this change:
/responsesmatches the non-streaming pathOpenAI-Projectis not forwarded upstream on this pathmismatched_projectAdditional verification:
cargo check -p smg cargo test -p smg header_utilsChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit