Repository navigation
fix(gateway): route Bedrock models without a global profile - #3104
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThe change extracts default-region pinning into a reusable helper, adds concrete regional mapping preference in chat routing, and introduces Vitest coverage for Bedrock region-selection scenarios and pass-through conditions. ChangesRegional provider selection
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
AWS Bedrock's region routing pins the `global` cross-region inference profile by default. A handful of Bedrock models expose only a `us` profile (Claude Opus 4.1, Llama 3.1 70B, Llama 4 Scout/Maverick) and have no `global` one. For those, applyPinnedDefaultRegions kept only the synthetic root, which getProviderEndpoint resolves to `global.<externalId>` — an id Bedrock rejects with 400 "The provided model identifier is invalid.", so every default request failed. Verified directly against Bedrock: `global.anthropic.claude-opus-4-1-20250805-v1:0` and `global.meta.llama3-1-70b-instruct-v1:0` both 400, while the `us.` prefixed ids return 200. When a model has regional variants but none matching the provider's default region, pin to the concrete regional variants instead of the synthetic root. Extracted applyPinnedDefaultRegions into its own module with unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8d6a2dc to
cc51181
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/pin-default-regions.ts (1)
40-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptimize provider lookup and avoid duplicate find operations.
The
providers.findlookup is performed twice for every mapping (once in the setup loop and once in the filter loop). While the arrays are likely small, you can improve performance and readability by looking up the provider definition once and caching it.Additionally, if
providersis correctly typed from@llmgateway/models, the explicitas ProviderDefinition | undefinedtype assertions might be unnecessary.♻️ Proposed refactor
- const def = providers.find((p) => p.id === m.providerId) as - | ProviderDefinition - | undefined; - if (m.region === def?.regionConfig?.defaultRegion) { + const def = providers.find((p) => p.id === m.providerId); + if (def && m.region === def.regionConfig?.defaultRegion) { providerHasDefaultRegion.add(m.providerId); } } return mappings.filter((m) => { - const def = providers.find((p) => p.id === m.providerId) as - | ProviderDefinition - | undefined; + const def = providers.find((p) => p.id === m.providerId); if (!def?.regionConfig?.pinDefaultRegion) {🤖 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/pin-default-regions.ts` around lines 40 - 51, Refactor the provider lookup in the setup and filtering logic of the pin-default-regions flow to resolve each mapping’s provider definition once and reuse the cached result, eliminating both repeated providers.find calls. If the providers collection is already typed as ProviderDefinition, remove the redundant ProviderDefinition | undefined assertions while preserving the existing default-region and pinDefaultRegion behavior.
🤖 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 `@apps/gateway/src/chat/tools/pin-default-regions.ts`:
- Around line 40-51: Refactor the provider lookup in the setup and filtering
logic of the pin-default-regions flow to resolve each mapping’s provider
definition once and reuse the cached result, eliminating both repeated
providers.find calls. If the providers collection is already typed as
ProviderDefinition, remove the redundant ProviderDefinition | undefined
assertions while preserving the existing default-region and pinDefaultRegion
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18a31774-1cc9-4dd2-bde3-a6fb29e62f15
📒 Files selected for processing (3)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/pin-default-regions.spec.tsapps/gateway/src/chat/tools/pin-default-regions.ts
Problem
The AWS Bedrock dashboard shows
claude-opus-4-1-20250805:globalfailing 100% of the time with:Root cause
Bedrock's
regionConfigusesdefaultRegion: "global"+pinDefaultRegion: true, so by default all Bedrock traffic is pinned to theglobalcross-region inference profile. Most Claude models on Bedrock expose aglobalCRIS profile — but a handful expose only ausprofile and noglobalone:claude-opus-4-1-20250805llama-3.1-70b-instructllama-4-scout-17b-instructllama-4-maverick-17b-instructFor those, on the default routing path (managed key, no
:regionsuffix):applyPinnedDefaultRegionsdropped theusvariant (us !== global), leaving only the region-less synthetic root.getProviderEndpointapplied the defaultglobal.prefix → e.g.global.anthropic.claude-opus-4-1-20250805-v1:0.Verification (direct Bedrock calls)
us.anthropic.claude-opus-4-1-20250805-v1:0global.anthropic.claude-opus-4-1-20250805-v1:0us.meta.llama3-1-70b-instruct-v1:0global.meta.llama3-1-70b-instruct-v1:0Fix
applyPinnedDefaultRegionsnow detects when a model has regional variants but none matching the provider's default region. In that case the synthetic root cannot be used (it would resolve to the invalidglobal.prefix), so it pins to the model's concrete regional variants (us) instead. Models that do have aglobalvariant, and Bedrock mappings with no regional variants at all, are unchanged.The helper was extracted into
apps/gateway/src/chat/tools/pin-default-regions.tswith unit tests covering: global-capable models,us-only models (Opus 4.1 + Llama), region-less mappings, explicit:regionrequests, and provider-key region locks.This keeps Opus 4.1 (and the Llama models) working on Bedrock via their
usprofile instead of deactivating them.Summary by CodeRabbit
Bug Fixes
Tests