fix(openai-chat): forward caller-selected service tier - #1512
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds provider-level ChangesChat service tier forwarding
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
070eca7 to
a353594
Compare
|
Rebased the fork branch onto current |
a353594 to
ab72f54
Compare
|
Rebased and force-with-lease pushed onto current dev@cbbfdd8773e68a5dc2391ddeb32f33a225373c1a; new head is ab72f54. Fresh local proof: Chat/service-tier focused suite 50/50, 84 assertions; typecheck, privacy scan, and diff check passed. The PR description is updated; keeping Draft pending maintainer choice/review, with #1521 carrying the same serializer fix. |
|
The narrow serializer change is valid and appropriately scoped: However, #1521 contains the same serializer change together with the provider/model capability resolver and catalog/runtime gating. Keeping both open creates two competing landing paths for the same line. I recommend treating #1512 as the minimal fallback only: leave it draft while #1521 is reviewed, and close it as superseded if #1521 is accepted. If the broader capability PR is rejected or split, this two-file patch is a reasonable standalone candidate after exact-head CI and readiness completion. |
Follow-up on top of @Yuxin-Qiao's forwarding commit. The forwarding itself is right: the Responses parser records a caller-supplied service_tier and this adapter dropped it, so the value never reached the wire. But 66 registry providers share the openai-chat adapter, and applyServiceTierGate only strips for openai-responses, so forwarding unconditionally would put an OpenAI-specific field on every one of them. Strict gateways reject unknown body fields, which turns a caller-supplied tier into an upstream 400 on routes that work today. prompt_cache_key directly below already solves this exact problem with an opt-in flag, and its comment names the same hazard. service_tier now follows that precedent: chatServiceTier on the registry entry, threaded through derive and router the way promptCacheKey is. supportsServiceTier is deliberately not reused — it governs the Responses wire through applyServiceTierGate and means something different. No registry entry opts in yet; that wants per-provider evidence that the gateway documents the parameter, which is a separate change.
ab72f54 to
197c82a
Compare
|
Reviewed against current I pushed one commit on top of yours rather than changing it, because the forwarding needs a gate. Why. 66 registry providers share the The precedent is directly below your line: I deliberately did not reuse Your test is kept and now asserts the opted-in case; two more assert that the field is dropped without the opt-in and that an opted-in provider with no caller tier still sends nothing. No registry entry opts in yet — that wants per-provider evidence that the gateway documents the parameter, which belongs in its own change. Verification at the merged head ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197c82ae99
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // unknown body fields. Forwarding unconditionally would turn a caller-supplied | ||
| // `service_tier` into an upstream 400 on those routes. `supportsServiceTier` is the | ||
| // Responses-wire flag (applyServiceTierGate) and deliberately does not gate this path. | ||
| if (provider.chatServiceTier && parsed.options.serviceTier !== undefined) { |
There was a problem hiding this comment.
When an operator configures an openai-chat custom gateway according to docs-site/src/content/docs/reference/configuration/providers.md:70, setting the documented supportsServiceTier: true still causes this condition to drop every caller-supplied tier because the new, distinct chatServiceTier option is undocumented and absent from the GUI payload. Add chatServiceTier to the provider documentation and translated references (and expose or preserve it in provider configuration flows), or make this adapter honor the existing documented capability for the chat wire.
AGENTS.md reference: AGENTS.md:L234-L235
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197c82ae99
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (provider.chatServiceTier && parsed.options.serviceTier !== undefined) { | ||
| body.service_tier = parsed.options.serviceTier; |
There was a problem hiding this comment.
When an OpenAI-compatible client sends service_tier to POST /v1/chat/completions, chatCompletionsToResponsesBody forwards neighboring fields such as parallel_tool_calls and prompt_cache_key but omits service_tier (src/chat/inbound.ts:274-286). Consequently the Responses parser leaves parsed.options.serviceTier undefined and this new serializer branch never runs, even for a provider with chatServiceTier: true. Copy the validated string into the translated body and add an endpoint-level regression test.
Useful? React with 👍 / 👎.
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
3 participants
Summary
parsed.options.serviceTiervalue when serializing OpenAI-compatible Chat Completions requests.service_tier: "priority"on the outbound/chat/completionsbody.dev@cbbfdd8773e68a5dc2391ddeb32f33a225373c1a; current head:ab72f5441e1b.Refs #1504
Refs #1436
Verification
bun test --isolate --max-concurrency 1 tests/openai-chat-hardening.test.ts tests/service-tier-capability.test.ts— 50 passed, 0 failed, 84 assertions.bun run typecheck— passed on the rebased head.bun run privacy:scan— passed.git diff --check— passed.bun run testresult is not claimed; the suite contains current baseline/environment failures unrelated to this narrow serializer change.Checklist
devcommit.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes
Maintainer note (checklist reset by the follow-up push). Re-ticked against fresh evidence at
197c82ae9, not carried over:bun x tsc --noEmitexit 0 andtests/openai-chat-hardening.test.ts39 pass / 0 fail on a Linux runner (Bun 1.3.14); branch rebased ontodev@d03755ee9; the one review finding (unconditionalservice_tierforwarding across 66 openai-chat providers) is fixed by thechatServiceTieropt-in in the follow-up commit.