Repository navigation
refactor(gateway): centralize service tier checks - #3467
Merged
Merged
Conversation
A Flex/Priority request can change provider or credential mid-flight — cross-provider fallback, or key rotation after a 429/auth failure — and a resolved attempt that cannot carry the tier would previously be sent at standard, succeeding (and billing) without the caller knowing. Every dispatch path now re-resolves the tier against the provider, region and credential it actually resolved to, and rejects the attempt instead of downgrading it. In resolveProviderContext that rejection is what the retry loop already treats as "try the next provider/key", so a fallback candidate that cannot serve the tier is skipped rather than silently serving standard. Also folds the three copies of service-tier credential eligibility (BYOK filter, managed-credential filter, env round-robin) into one predicate, so the Vertex global-endpoint rule that only the managed and env paths enforced now applies everywhere. The org-level coding-plan default tier stays soft: it is a cost preference, not a requirement, so an attempt that cannot serve it still runs at standard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
WalkthroughService-tier routing now evaluates credential and regional support, preserves explicit Flex/Priority requests across fallback and rotation, and rejects unsupported candidates. Tests and documentation cover forwarding, retry behavior, Vertex constraints, and optional defaults. ChangesService-tier routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/gateway/src/chat/chat.ts (1)
6229-6243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "resolve tier, then assert" pair into one helper.
Four sites call
getForwardedServiceTierwith the same six arguments and immediately callassertServiceTierHonoredwith the same five fields. The shared root cause is thatapps/gateway/src/chat/tools/service-tier.tsexports the two steps separately but no combined helper. A single exported function that resolves the tier, asserts it, and returns the forwarded value would remove the duplication and guarantee that a future call site cannot resolve without asserting.
apps/gateway/src/chat/chat.ts#L6229-L6243: replace the initial-path pair with the combined helper and keep the returned value ininitialForwardedServiceTier.apps/gateway/src/chat/chat.ts#L7099-L7113: replace the streaming retry pair with the combined helper, keeping the call outside thetryblock.apps/gateway/src/chat/chat.ts#L11376-L11390: replace the non-streaming retry pair with the combined helper, keeping the call outside thetryblock.apps/gateway/src/chat/tools/resolve-provider-context.ts#L645-L659: replace the resolver pair with the combined helper and passoptions.requestedServiceTier ?? null.As per coding guidelines: "Apply DRY principles for code reuse".
♻️ Sketch of the shared helper
+/** + * Resolves the tier to forward for a resolved attempt and rejects the attempt + * when an explicitly requested tier cannot be honored. Use this instead of + * calling `getForwardedServiceTier` and `assertServiceTierHonored` separately, + * so no dispatch path can resolve a tier without asserting it. + */ +export function resolveAndAssertServiceTier(options: { + model: string; + provider: Provider; + region: string | undefined; + serviceTier: "auto" | "default" | "flex" | "priority" | undefined; + requestedServiceTier: "flex" | "priority" | null; + configIndex?: number; + variant?: EnvVarVariant; +}): "flex" | "priority" | undefined { + const forwardedServiceTier = getForwardedServiceTier( + options.model, + options.provider, + options.region, + options.serviceTier, + options.configIndex, + options.variant, + ); + assertServiceTierHonored({ + requestedServiceTier: options.requestedServiceTier, + forwardedServiceTier, + provider: options.provider, + model: options.model, + region: options.region, + }); + return forwardedServiceTier; +}🤖 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 `@apps/gateway/src/chat/chat.ts` around lines 6229 - 6243, In apps/gateway/src/chat/tools/service-tier.ts, add one exported helper that accepts the six getForwardedServiceTier inputs plus the requested tier, resolves the forwarded tier, asserts it with assertServiceTierHonored, and returns the value. Replace the duplicated pairs at apps/gateway/src/chat/chat.ts lines 6229-6243, 7099-7113, and 11376-11390 with this helper, preserving initialForwardedServiceTier and keeping retry calls outside their try blocks. Replace the pair at apps/gateway/src/chat/tools/resolve-provider-context.ts lines 645-659 likewise, passing options.requestedServiceTier ?? null.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.
Inline comments:
In `@apps/gateway/src/api.spec.ts`:
- Around line 2414-2424: Update the response assertion in the flex-tier routing
test to expect the exact status code returned by the TRIGGER_ERROR upstream
path, replacing the broad not-200 check. Keep the existing waitForLogs and
provider/service-tier assertions unchanged.
---
Nitpick comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 6229-6243: In apps/gateway/src/chat/tools/service-tier.ts, add one
exported helper that accepts the six getForwardedServiceTier inputs plus the
requested tier, resolves the forwarded tier, asserts it with
assertServiceTierHonored, and returns the value. Replace the duplicated pairs at
apps/gateway/src/chat/chat.ts lines 6229-6243, 7099-7113, and 11376-11390 with
this helper, preserving initialForwardedServiceTier and keeping retry calls
outside their try blocks. Replace the pair at
apps/gateway/src/chat/tools/resolve-provider-context.ts lines 645-659 likewise,
passing options.requestedServiceTier ?? null.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95d9e06b-a933-42b6-b15d-926ccf584617
📒 Files selected for processing (11)
apps/docs/content/features/service-tiers.mdxapps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-provider-env.tsapps/gateway/src/chat/tools/resolve-platform-credential.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/chat/tools/service-tier.spec.tsapps/gateway/src/chat/tools/service-tier.tsapps/gateway/src/test-utils/mock-openai-server.tspackages/actions/src/apply-google-service-tier.spec.tspackages/actions/src/apply-google-service-tier.ts
Comment on lines
+2414
to
+2424
| // openai is the only flex-capable mapping, so the upstream failure is | ||
| // returned instead of being retried on azure at the standard tier. | ||
| expect(res.status).not.toBe(200); | ||
|
|
||
| const logs = await waitForLogs(1); | ||
| expect(logs.length).toBeGreaterThanOrEqual(1); | ||
| for (const log of logs) { | ||
| expect(log.usedProvider).toBe("openai"); | ||
| expect(log.requestedServiceTier).toBe("flex"); | ||
| expect(log.usedServiceTier).toBeNull(); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact status code.
expect(res.status).not.toBe(200) passes for any failure, including a setup error unrelated to service-tier routing. Assert the specific upstream status that the TRIGGER_ERROR path returns, so the test proves the request stayed on openai and returned the upstream error rather than falling back.
💚 Proposed assertion tightening
- expect(res.status).not.toBe(200);
+ expect(res.status).toBe(500);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // openai is the only flex-capable mapping, so the upstream failure is | |
| // returned instead of being retried on azure at the standard tier. | |
| expect(res.status).not.toBe(200); | |
| const logs = await waitForLogs(1); | |
| expect(logs.length).toBeGreaterThanOrEqual(1); | |
| for (const log of logs) { | |
| expect(log.usedProvider).toBe("openai"); | |
| expect(log.requestedServiceTier).toBe("flex"); | |
| expect(log.usedServiceTier).toBeNull(); | |
| } | |
| // openai is the only flex-capable mapping, so the upstream failure is | |
| // returned instead of being retried on azure at the standard tier. | |
| expect(res.status).toBe(500); | |
| const logs = await waitForLogs(1); | |
| expect(logs.length).toBeGreaterThanOrEqual(1); | |
| for (const log of logs) { | |
| expect(log.usedProvider).toBe("openai"); | |
| expect(log.requestedServiceTier).toBe("flex"); | |
| expect(log.usedServiceTier).toBeNull(); | |
| } |
🤖 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 `@apps/gateway/src/api.spec.ts` around lines 2414 - 2424, Update the response
assertion in the flex-tier routing test to expect the exact status code returned
by the TRIGGER_ERROR upstream path, replacing the broad not-200 check. Keep the
existing waitForLogs and provider/service-tier assertions unchanged.
Main made `requestedServiceTier` mutable and now stamps it for the coding-plan default tier (#3459), so the strict-enforcement assert keys off the new `serviceTierSource === "request"` instead — the coding-plan default must stay soft, or DevPass flex requests would start failing where they used to run at standard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/tools/resolve-provider-context.ts (1)
489-501: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate strict credential eligibility on
clientRequestedServiceTier.
serviceTierKeyFilterandrequiresServiceTieruseoptions.service_tier. That value can contain an organization default. When the default isflexorpriorityandclientRequestedServiceTierisnull, this code still excludes credentials that cannot provide the premium tier. The request can fail with"No API key set for provider: ..."instead of using the credential at standard tier.Use
clientRequestedServiceTierfor strict filtering. Keep organization defaults as best-effort preferences.Proposed fix
- const serviceTierKeyFilter = isPremiumServiceTier(options.service_tier) + const hasStrictClientTier = + options.clientRequestedServiceTier !== undefined && + options.clientRequestedServiceTier !== null; + const serviceTierKeyFilter = hasStrictClientTier ? providerKeySupportsServiceTier : undefined; - requiresServiceTier: serviceTierKeyFilter !== undefined, + requiresServiceTier: hasStrictClientTier,Also applies to: 537-540, 575-578
🤖 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 `@apps/gateway/src/chat/tools/resolve-provider-context.ts` around lines 489 - 501, Update the service-tier eligibility logic around serviceTierKeyFilter, requiresServiceTier, and the related provider-key selection paths to use clientRequestedServiceTier rather than options.service_tier for strict filtering. Organization defaults such as flex or priority must remain best-effort preferences when no client tier was requested, allowing standard-tier credentials to remain eligible.
🤖 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.
Inline comments:
In `@apps/gateway/src/chat/tools/resolve-provider-context.ts`:
- Line 171: Update the initial chat resolveProviderContext(...) call to pass
clientRequestedServiceTier from the request, matching
resolveProviderContextForRetry(...). Alternatively, make
clientRequestedServiceTier required in ProviderContextOptions, ensuring the
initial chat path cannot omit the client-selected service tier.
---
Outside diff comments:
In `@apps/gateway/src/chat/tools/resolve-provider-context.ts`:
- Around line 489-501: Update the service-tier eligibility logic around
serviceTierKeyFilter, requiresServiceTier, and the related provider-key
selection paths to use clientRequestedServiceTier rather than
options.service_tier for strict filtering. Organization defaults such as flex or
priority must remain best-effort preferences when no client tier was requested,
allowing standard-tier credentials to remain eligible.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e15189-7566-4bdc-8bea-2e94d9b3f3f3
📒 Files selected for processing (5)
apps/gateway/src/api.spec.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/resolve-provider-context.tsapps/gateway/src/chat/tools/service-tier.spec.tsapps/gateway/src/chat/tools/service-tier.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/gateway/src/chat/tools/service-tier.ts
- apps/gateway/src/chat/tools/service-tier.spec.ts
- apps/gateway/src/api.spec.ts
- apps/gateway/src/chat/chat.ts
| * candidate that cannot serve it is rejected rather than downgraded, so the | ||
| * retry loop moves on instead of quietly serving standard. | ||
| */ | ||
| clientRequestedServiceTier?: "flex" | "priority" | null; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 14 '\bresolveProviderContext\s*\(' apps/gateway/src --glob '*.ts'Repository: theopenco/llmgateway
Length of output: 14263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## resolved ProviderContextOptions/resolveProviderContext definition"
sed -n '1,190p' apps/gateway/src/chat/tools/resolve-provider-context.ts
echo
echo "## call sites in chat.ts around resolveProviderContextForRetry and original usage"
sed -n '6520,6635p' apps/gateway/src/chat/chat.ts
echo
echo "## all references to resolveProviderContext outside videos.ts"
rg -n 'resolveProviderContext\s*\(' apps/gateway/src/chat --glob '*.ts'Repository: theopenco/llmgateway
Length of output: 9492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## line number ranges for resolveProviderContext uses"
rg -n 'resolveProviderContext\s*\(|function resolveProviderContext|export async function resolveProviderContext' apps/gateway/src --glob '*.ts'
echo
echo "## focused call-site contexts"
sed -n '470,700p' apps/gateway/src/chat/chat.ts
sed -n '2150,2175p' apps/gateway/src/videos/videos.ts
sed -n '4720,4795p' apps/gateway/src/videos/videos.ts
sed -n '4870,4895p' apps/gateway/src/videos/videos.tsRepository: theopenco/llmgateway
Length of output: 11127
Require clientRequestedServiceTier on the chat request path.
resolveProviderContextForRetry(...) already passes clientRequestedServiceTier, but the initial chat resolveProviderContext(...) call does not. Omitting it makes the function convert the client-selected tier to loose validation; either pass the tier there too, or make clientRequestedServiceTier required on ProviderContextOptions.
🤖 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 `@apps/gateway/src/chat/tools/resolve-provider-context.ts` at line 171, Update
the initial chat resolveProviderContext(...) call to pass
clientRequestedServiceTier from the request, matching
resolveProviderContextForRetry(...). Alternatively, make
clientRequestedServiceTier required in ProviderContextOptions, ensuring the
initial chat path cannot omit the client-selected service tier.
steebchen
changed the title
steebchen
enabled auto-merge
August 7, 2026 20:21
steebchen
added this pull request to the merge queue
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
A user on a coding plan reported that a
service_tier: "flex"request to GPT-5.6 Sol got a 429 on one OpenAI key, was retried on another key, succeeded — and came back billed at standard rates. They asked for a way to strictly enforce the requested tier, or to disable alternate-key retry so they get the original 429 back.The investigation did not find a bug. The tier survives both alternate-key rotation and cross-provider fallback today: routing is narrowed to tier-capable mappings before a provider is picked, and the retry context rebuilds the request body with the tier intact. The two integration tests added here pass on
mainunchanged. So the reported request was either served at standard by OpenAI itself, or never carried the tier upstream to begin with.This PR is therefore hardening and regression coverage, not a fix. It is worth reviewing on that basis — if you'd rather not carry the guard, the tests and the filter dedup stand on their own.
What's here
Roughly 800 added lines break down as ~420 lines of tests, ~170 lines of code moved out of
chat.ts, ~30 lines of docs, and ~60 lines of genuinely new logic.Regression tests for the reported scenario. Three integration tests in
api.spec.ts: key rotation preserves the tier through a 429/500 retry, a tier request never routes to a provider that lacks the tier, and a control proving the same request does fall back to that provider when no tier is requested. Plus unit coverage in a newservice-tier.spec.ts.One credential-eligibility predicate. The rule for "can this credential carry a premium tier" existed in three hand-synced copies — the BYOK key filter, the managed-credential filter, and the env round-robin — and had already drifted: the BYOK copy was missing the Vertex global-endpoint rule the other two enforced. (Not reachable today, since BYOK Vertex keys take their region from the environment rather than key options, but it is one config option away from being a silent billing bug.) All three now share
providerCredentialSupportsServiceTier.A dispatch-time assert.
getForwardedServiceTiersilently returnsundefinedwhen a resolved mapping or credential can't carry the tier, so "send it at standard anyway" is always one stray candidate away, and the resulting failure is invisible — the request succeeds and is billed at up to 2x what was asked for.assertServiceTierHonoredturns that into a 400unsupported_service_tier. InresolveProviderContext— the choke point every retry passes through, streaming and non-streaming — the throw is what both retry loops already treat as "try the next provider/key", so an ineligible candidate is skipped rather than downgraded.Tier helpers extracted from
chat.tsintoapps/gateway/src/chat/tools/service-tier.ts(getForwardedServiceTier,mappingSupportsRequestedServiceTier, and the Vertex region resolution each of them re-implemented inline).Strictness applies only to a tier the client asked for itself. The coding-plan org default is a cost preference rather than a requirement, so an attempt that can't serve it still runs at standard — keyed off
serviceTierSource === "request"from #3459.A caveat worth knowing about the guard
Syncing this branch with main hit exactly the failure mode the guard is meant to prevent, from the other direction. #3459 made
requestedServiceTiermutable and now stamps it when the coding-plan default fires; the assert had been keying off that variable, so taking the merge naively would have made every DevPass flex request hard-fail where it used to run at standard. Caught in the merge and rewired, but it's a fair argument that the guard is itself a footgun if wired carelessly, where a plain regression test would not be.Not included
Two things the reporter asked for, out of scope here and worth deciding separately:
no_key_fallback—x-no-fallbackdisables cross-provider fallback but deliberately still rotates keys within a provider (there is an existing test pinning that). Returning the original 429 to the client needs a new opt-out.Verification
origin/main: 255 tests pass acrossapi.spec,fallback.spec,service-tier.spec,resolve-provider-context.specand the actions spec; before the merge the fullapps/gateway+packages/actionsrun was 2663 passing.pnpm format,pnpm lint,pnpm buildclean.No UI surface changed. The only user-visible change is a new "Retries and fallback never downgrade the tier" section on the service-tiers docs page.
🤖 Generated with Claude Code