Conversation
WalkthroughFavor provider-root (no-region) mappings over regional ones, exclude fallbacks by providerId only, add an optional includeProviderScoreRegions flag to suppress regions in routing metadata, and omit :region in usedModel unless provider was direct-specified. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as Gateway (chat.ts)
participant Selector as ProviderSelector (get-cheapest...)
participant Provider as ProviderPool
Client->>Gateway: Send generation request
Gateway->>Selector: Build candidates & call getCheapest(..., includeProviderScoreRegions: false?)
Selector->>Provider: Request provider candidates and scores
Provider-->>Selector: Return provider entries (root + regional)
Selector-->>Gateway: Return chosen provider + routingMetadata (providerScores with/without region)
Gateway-->>Client: Route request to chosen provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: a5ff8d0ea5
ℹ️ 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".
apps/gateway/src/chat/chat.ts
Outdated
| } | ||
|
|
||
| if (routingMetadata) { | ||
| routingMetadata = normalizeRoutingMetadataForLogging(routingMetadata); |
There was a problem hiding this comment.
This mutates routingMetadata.providerScores before the retry loop, but selectNextProvider later matches scores to model mappings using both provider and region (p.providerId === score.providerId && p.region === score.region in apps/gateway/src/chat/tools/retry-with-fallback.ts). For price-only-no-metrics, normalizeRoutingMetadataForLogging drops region for single-row providers, so any provider whose mapping has a concrete region (e.g. alibaba/singapore) can no longer be selected on retry, causing fallback to stop early and potentially fail requests that previously would have retried successfully.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a small normalization step to routing metadata specifically for logging, so “price-only-no-metrics” score tables don’t misleadingly show a single implicit region (while still preserving region labels when multiple regional rows are compared).
Changes:
- Introduce
normalizeRoutingMetadataForLogging()to hideregionon providerScores when a provider only has a single score row underprice-only-no-metrics. - Apply the normalization to
routingMetadatainchat.tsbefore it’s used for log entry creation. - Add a focused Vitest unit regression suite covering the normalization behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/gateway/src/chat/tools/normalize-routing-metadata.ts | Adds routing-metadata normalization logic for log output under price-only-no-metrics. |
| apps/gateway/src/chat/tools/normalize-routing-metadata.spec.ts | Adds unit tests verifying collapsed-region hiding and multi-region preservation. |
| apps/gateway/src/chat/chat.ts | Normalizes routing metadata before it’s passed into log entry creation paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (score) => score.providerId === "alibaba", | ||
| ); | ||
|
|
||
| expect(alibabaScore?.region).toBeUndefined(); |
There was a problem hiding this comment.
This assertion can pass even if the provider score isn't found (e.g., if providerScores doesn’t contain an alibaba entry), because alibabaScore?.region will be undefined in that case. Add an explicit expectation that alibabaScore is defined (or use find(...)!) so the test actually validates the intended behavior.
| expect(alibabaScore?.region).toBeUndefined(); | |
| expect(alibabaScore).toBeDefined(); | |
| expect(alibabaScore!.region).toBeUndefined(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@apps/gateway/src/chat/chat.ts`:
- Around line 1849-1851: The code overwrites runtime routing state by
reassigning routingMetadata with normalizeRoutingMetadataForLogging, which can
strip providerScores[*].region and break retry/region-failure dedupe; instead,
preserve the original routingMetadata and assign the normalized result to a
separate variable (e.g., routingMetadataForLogging) or pass a shallow/deep clone
into normalizeRoutingMetadataForLogging so retry selection logic still sees the
full providerScores with region; update places that log to use the new
logging-only variable and leave routingMetadata untouched for
retry/provider-region logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d085e2e-264c-4c8d-b3b1-6538d206c335
📒 Files selected for processing (3)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/normalize-routing-metadata.spec.tsapps/gateway/src/chat/tools/normalize-routing-metadata.ts
apps/gateway/src/chat/chat.ts
Outdated
| if (routingMetadata) { | ||
| routingMetadata = normalizeRoutingMetadataForLogging(routingMetadata); | ||
| } |
There was a problem hiding this comment.
Do not overwrite runtime routing state with logging-only normalization.
Reassigning routingMetadata here can remove providerScores[*].region before retry selection runs, which may break provider-region failure deduping and cause fallback to re-pick failed targets.
💡 Safer direction
-if (routingMetadata) {
- routingMetadata = normalizeRoutingMetadataForLogging(routingMetadata);
-}
+// Keep `routingMetadata` intact for retry/fallback logic.
+// Normalize only at log emission boundaries (e.g. inside `createLogEntry`
+// or immediately before passing metadata to it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 1849 - 1851, The code overwrites
runtime routing state by reassigning routingMetadata with
normalizeRoutingMetadataForLogging, which can strip providerScores[*].region and
break retry/region-failure dedupe; instead, preserve the original
routingMetadata and assign the normalized result to a separate variable (e.g.,
routingMetadataForLogging) or pass a shallow/deep clone into
normalizeRoutingMetadataForLogging so retry selection logic still sees the full
providerScores with region; update places that log to use the new logging-only
variable and leave routingMetadata untouched for retry/provider-region logic.
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 `@apps/gateway/src/chat/chat.ts`:
- Around line 1827-1829: The current predicate sets includeProviderScoreRegions
whenever selectionReason === "direct-provider-specified", which leaks a single
implicit region; change the condition to only enable provider region emission
when there was an explicit region selection or there is an actual multi-region
comparison (e.g. selectionReason === "direct-provider-specified" &&
providerScoreRegions?.length > 1 OR an explicit-region flag is true). Update the
boolean expressions where includeProviderScoreRegions is computed (the instance
using selectionReason === "direct-provider-specified" and the other two
occurrences referenced in the comment) to check for explicit selection or
providerScoreRegions.length > 1 so single implicit rows no longer emit region
metadata.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 646f37a2-fc8a-4b6e-9bbc-0d778b5131ac
📒 Files selected for processing (4)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0711e8629e
ℹ️ 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".
| { | ||
| metricsMap, | ||
| isStreaming: stream, | ||
| includeProviderScoreRegions: false, |
There was a problem hiding this comment.
Passing includeProviderScoreRegions: false here strips region data from routingMetadata.providerScores before the retry loop runs. In selectNextProvider (apps/gateway/src/chat/tools/retry-with-fallback.ts), the next mapping is resolved with p.providerId === score.providerId && p.region === score.region; when the chosen mappings are regional (for example alibaba with cn-beijing), score.region becomes undefined, no mapping matches, and fallback retries stop after the first retryable failure even when alternative providers exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bc1bf05aa
ℹ️ 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".
| // Filter model providers to only those eligible for this request | ||
| const availableModelProviders = filterEligibleModelProviders( | ||
| preferConcreteRegionalMappings(expandedIamFilteredModelProviders), | ||
| preferProviderRootMappings(expandedIamFilteredModelProviders), |
There was a problem hiding this comment.
Passing preferProviderRootMappings(expandedIamFilteredModelProviders) into filterEligibleModelProviders removes concrete region mappings before routing in the API-key/hybrid path, which makes providerLockedRegions effectively inert because that lock check only applies when provider.region is present. As a result, scoring and eligibility checks can run against the synthetic root row instead of the user’s locked region (e.g., cn-beijing), so routing decisions and pricing/capability filtering are based on the wrong region until execution-time token resolution.
Useful? React with 👍 / 👎.
| const providerIds = modelInfo.providers | ||
| .filter( | ||
| (p) => !(p.providerId === usedProvider && p.region === usedRegion), | ||
| ) // Exclude the exact low-uptime provider+region pair | ||
| .filter((p) => p.providerId !== usedProvider) | ||
| .map((p) => p.providerId); |
There was a problem hiding this comment.
This change drops all mappings for usedProvider during low-uptime rerouting, rather than excluding only the failing provider+region pair. In a model that has multiple regions for one provider, a low-uptime region can no longer fail over to a healthier region of the same provider, and if no other providers exist this path now silently skips fallback even though a viable regional alternative is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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/chat.ts (1)
1627-1638:⚠️ Potential issue | 🟠 MajorDon't collapse away provider-key region locks.
This block just built
providerLockedRegions, butpreferProviderRootMappings(...)runs first and drops the locked regional row whenever that provider also has a root mapping. The router can then score the provider on its root/default mapping and later execute against the DB-locked region, so price/uptime/capability selection can be wrong. Apply the root-mapping collapse only to providers that are not present inproviderLockedRegions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 1627 - 1638, The call to preferProviderRootMappings(expandedIamFilteredModelProviders) collapses provider root mappings and unintentionally drops providerLockedRegions entries when a provider has a root mapping; update the logic so you only apply preferProviderRootMappings to providers that are not present in providerLockedRegions (i.e., compute a filteredProviders = expandedIamFilteredModelProviders.filter(p => !providerLockedRegions.has(p.providerKey) or equivalent), pass that filtered list into preferProviderRootMappings, and then merge its results with the original providerLockedRegions so locked regional rows are preserved during scoring and selection (ensure you reference preferProviderRootMappings, expandedIamFilteredModelProviders, and providerLockedRegions in the change).
♻️ Duplicate comments (1)
apps/gateway/src/chat/chat.ts (1)
1822-1823:⚠️ Potential issue | 🟠 MajorDon't treat every direct-provider request as an explicit region choice.
Using
selectionReason === "direct-provider-specified"as the gate still emitsproviderScores[*].regionand appends:regiontousedModelFormattedwhen the flow resolved to a single implicit/default row. That recreates the leak this PR is trying to remove. Base the emitted-region flag ondirectProviderRegionWasExplicit || routingMetadataProviders.length > 1(or equivalent “actual multi-region comparison” logic) and reuse that same flag for the score rows and formatted used model. If retries still need raw region data internally, keep it in the pre-normalized state and only apply this gate to the emitted/logged copy.Also applies to: 1847-1857, 1929-1935, 2389-2394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 1822 - 1823, The emitted-region gating currently uses selectionReason === "direct-provider-specified", which still leaks region when the choice was implicit; change the logic that sets includeProviderScoreRegions (and any places that append :region to usedModelFormatted or emit providerScores[*].region) to use a new boolean computed from the actual multi-region condition (e.g., directProviderRegionWasExplicit || routingMetadataProviders.length > 1) and reuse that same flag for both score rows and the formatted usedModel string; keep raw region info available internally (pre-normalized) for retries but only attach region to emitted/logged copies when the new flag is true, and apply this change consistently to the other occurrences around the includeProviderScoreRegions usage and usedModelFormatted construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1627-1638: The call to
preferProviderRootMappings(expandedIamFilteredModelProviders) collapses provider
root mappings and unintentionally drops providerLockedRegions entries when a
provider has a root mapping; update the logic so you only apply
preferProviderRootMappings to providers that are not present in
providerLockedRegions (i.e., compute a filteredProviders =
expandedIamFilteredModelProviders.filter(p =>
!providerLockedRegions.has(p.providerKey) or equivalent), pass that filtered
list into preferProviderRootMappings, and then merge its results with the
original providerLockedRegions so locked regional rows are preserved during
scoring and selection (ensure you reference preferProviderRootMappings,
expandedIamFilteredModelProviders, and providerLockedRegions in the change).
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1822-1823: The emitted-region gating currently uses
selectionReason === "direct-provider-specified", which still leaks region when
the choice was implicit; change the logic that sets includeProviderScoreRegions
(and any places that append :region to usedModelFormatted or emit
providerScores[*].region) to use a new boolean computed from the actual
multi-region condition (e.g., directProviderRegionWasExplicit ||
routingMetadataProviders.length > 1) and reuse that same flag for both score
rows and the formatted usedModel string; keep raw region info available
internally (pre-normalized) for retries but only attach region to emitted/logged
copies when the new flag is true, and apply this change consistently to the
other occurrences around the includeProviderScoreRegions usage and
usedModelFormatted construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65472ee1-bbc2-41c7-a961-c46aeeefee34
📒 Files selected for processing (2)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/fallback.spec.ts
steebchen
force-pushed
the
fix-alibaba-region-label
branch
from
2bc1bf0 to
6141a4e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6141a4ed4d
ℹ️ 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".
| return providers.filter( | ||
| (mapping) => | ||
| !providersWithRegions.has(mapping.providerId) || Boolean(mapping.region), | ||
| !providersWithRootMappings.has(mapping.providerId) || !mapping.region, | ||
| ); |
There was a problem hiding this comment.
This filter now drops concrete regional rows whenever a provider has a root row, so routing selects a root modelName (no :region) even for regional providers. In API-key/hybrid mode, usedRegion is later populated from the provider key, but downstream validation still matches mappings by exact (providerId, modelName, region) (e.g. finalModelInfo.providers.find(...) used for max token and supported-parameter checks), which no longer matches and silently skips those guards. For regional providers like Alibaba, this can let invalid max_tokens/unsupported params through to upstream instead of being rejected by the gateway.
Useful? React with 👍 / 👎.
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 `@apps/gateway/src/chat/chat.ts`:
- Around line 1477-1489: Root-mapping is currently applied before eligibility
filtering which lets providers with locked regions be returned with region
undefined; fix by ensuring region-locks are enforced before/while root-mapping:
either call filterEligibleModelProviders on expandedIamFilteredModelProviders
(and related calls at the other sites) before preferProviderRootMappings, or
modify preferProviderRootMappings to preserve/propagate any explicit region
property from provider keys so that filterEligibleModelProviders still rejects
mismatched regions; update the call sites using filterEligibleModelProviders and
preferProviderRootMappings (referencing expandedIamFilteredModelProviders,
modelInfo.providers, availableProviders, response_format, hasImages, max_tokens,
reasoning_effort) accordingly so region locks cannot be bypassed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5a05fbc-93f1-4b46-85c9-c9e54a4cc105
📒 Files selected for processing (4)
apps/gateway/src/chat/chat.tsapps/gateway/src/fallback.spec.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/actions/src/models.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/fallback.spec.ts
| const availableModelProviders = filterEligibleModelProviders( | ||
| preferProviderRootMappings(expandedIamFilteredModelProviders), | ||
| { | ||
| allProviderVariants: modelInfo.providers, | ||
| availableProviders, | ||
| webSearchTool, | ||
| responseFormatType: response_format?.type, | ||
| hasImages, | ||
| maxTokens: max_tokens, | ||
| reasoningEffort: reasoning_effort, | ||
| }, | ||
| ).filter((provider) => provider.providerId !== usedProvider); | ||
|
|
There was a problem hiding this comment.
Provider-key region locks can be bypassed after root-mapping preference.
At Line 1781 (and similarly at Line 1478 and Line 1624), root mappings are preferred before eligibility filtering. Since filterEligibleModelProviders only rejects mismatched non-empty regions, a locked provider can still pass with region: undefined, which weakens explicit region pinning from provider keys.
💡 Suggested fix
diff --git a/apps/gateway/src/chat/chat.ts b/apps/gateway/src/chat/chat.ts
@@
const lockedRegion = options.providerLockedRegions?.get(
provider.providerId,
);
- if (lockedRegion && provider.region && provider.region !== lockedRegion) {
- return false;
- }
+ if (lockedRegion) {
+ // Enforce explicit provider-key region pin.
+ if (!provider.region || provider.region !== lockedRegion) {
+ return false;
+ }
+ }Also applies to: 1624-1634, 1781-1791
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 1477 - 1489, Root-mapping is
currently applied before eligibility filtering which lets providers with locked
regions be returned with region undefined; fix by ensuring region-locks are
enforced before/while root-mapping: either call filterEligibleModelProviders on
expandedIamFilteredModelProviders (and related calls at the other sites) before
preferProviderRootMappings, or modify preferProviderRootMappings to
preserve/propagate any explicit region property from provider keys so that
filterEligibleModelProviders still rejects mismatched regions; update the call
sites using filterEligibleModelProviders and preferProviderRootMappings
(referencing expandedIamFilteredModelProviders, modelInfo.providers,
availableProviders, response_format, hasImages, max_tokens, reasoning_effort)
accordingly so region locks cannot be bypassed.
Summary
Verification
Notes
Summary by CodeRabbit
New Features
Behavior Changes
Tests