Repository navigation
fix(gateway): keep reasoning on unpinned routing - #2791
Conversation
When a Claude model is requested without a provider prefix (e.g. `claude-opus-4-6` rather than `aws-bedrock/claude-opus-4-6`), the gateway left `modelInfo.providers` un-expanded — only the region-agnostic root mapping (`region: undefined`) survives, because region expansion is applied only when a provider is explicitly requested. Routing still resolves `usedRegion` to a concrete value (AWS Bedrock's `global`), so the capability lookup `p.region === usedRegion` matched nothing (`'global' !== undefined`). `supportsReasoning` collapsed to false and `prepareRequestBody` skipped the entire thinking block, so Bedrock Claude returned no reasoning. Pinned routing was unaffected. Resolve the mapping with a region-agnostic fallback: prefer the exact `(providerId, region)` mapping, otherwise fall back to the provider-level mapping. Capability flags (reasoning, splitTaggedReasoning, healStreamingJsonOutput, reasoningMode) cannot be overridden per region (`ProviderRegion` only allows pricing/context/output/streaming overrides), so the fallback is lossless. Extracted into a pure `selectProviderMapping` helper with unit tests. Verified live against AWS Bedrock: unpinned `claude-opus-4-6` / `claude-opus-4-8` now emit reasoning tokens; pinned and native Anthropic routes unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new ChangesselectProviderMapping helper and gateway integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🤖 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 `@packages/actions/src/select-provider-mapping.ts`:
- Around line 26-28: The fallback logic in the provider find chain does not
explicitly target the region-agnostic root mapping as described in the function
contract. The current second find returns any first match with the providerId,
but it should specifically look for a provider with region set to undefined
before falling back to any generic provider lookup. Modify the fallback chain to
add an explicit intermediate check that searches for a provider with the
matching providerId and region explicitly set to undefined, ensuring the
fallback behavior follows the documented contract and is not order-dependent on
provider array ordering.
🪄 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: 54856e33-ac23-4774-9191-74c58bdf6742
📒 Files selected for processing (4)
apps/gateway/src/chat/chat.tspackages/actions/src/index.tspackages/actions/src/select-provider-mapping.spec.tspackages/actions/src/select-provider-mapping.ts
Address PR review: the region-agnostic fallback in selectProviderMapping returned the first provider entry by array order, which contradicted the documented "region-agnostic root" contract and was order-dependent. Resolve explicitly: exact (providerId, region) -> root (region: undefined) -> any provider mapping. No behavioral change for the reasoning fix (capability flags are region-invariant), but the lookup is now order-independent and matches its docstring. Adds an order-independence regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb836b53ff
ℹ️ 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".
| const selectedProviderMapping = selectProviderMapping( | ||
| modelInfo.providers, | ||
| usedProvider, | ||
| usedRegion, | ||
| ); |
There was a problem hiding this comment.
Reuse region fallback for retry context
This fixes the initial request, but retry/fallback requests still rebuild the provider context through resolveProviderContext, which exact-matches modelInfo.providers on (providerId, region) at apps/gateway/src/chat/tools/resolve-provider-context.ts:378 and derives supportsReasoning from that result at line 415. For unpinned AWS Bedrock routing, modelInfo.providers can still contain only the root mapping while usedRegion is global, so any retry after a transient upstream/key failure will send the retried request without reasoning even though the first attempt now includes it. Use the same fallback helper in the retry context path as well.
Useful? React with 👍 / 👎.
Address PR review (Codex): resolveProviderContext — used to rebuild the
provider context on retries and alternate-key/provider fallback — did the
same exact (providerId, region) lookup as the initial path and derived
supportsReasoning from it. For unpinned AWS Bedrock routing, modelInfo is
un-expanded (root mapping, region: undefined) while the retry candidate's
region is the concrete resolved usedRegion ('global'), so the lookup
missed and any retry (e.g. after a transient upstream/key failure) sent
the request without reasoning even though the first attempt included it.
Resolve via the shared selectProviderMapping fallback helper so the retry
path matches the initial-request fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
Reasoning was silently dropped for Claude models on AWS Bedrock when the model was requested without a provider prefix.
aws-bedrock/claude-opus-4-6(pinned)claude-opus-4-6(unpinned)reasoning_tokens: 0)Reported by users via RisuAI:
reasoning_effort: highonclaude-opus-4-6/claude-opus-4-8produced no<Thoughts>/delta.reasoning/ reasoning tokens, but the same request pinned toaws-bedrock/...worked.Root cause
useExpandedRoutingProvidersis true only when a provider is explicitly requested. For unpinned routing,modelInfo.providerskeeps only the region-agnostic root mapping (region: undefined) — region expansion (expandProviderRegions) is never applied to it. Meanwhile routing still resolvesusedRegionto a concrete value (AWS Bedrock'sglobal,pinDefaultRegion: true).The capability lookup in
chat.tsmatched on exact region equality:'global' !== undefined→ no match →supportsReasoning = false(andsplitTaggedReasoning/healStreamingJsonOutput) →prepareRequestBodyskipped the entire thinking block → Bedrock Claude returned no reasoning.Fix
Resolve the mapping with a region-agnostic fallback — prefer the exact
(providerId, region)mapping, otherwise fall back to the provider-level mapping. Capability flags (reasoning,reasoningMode,splitTaggedReasoning,healStreamingJsonOutput) cannot be overridden per region (ProviderRegiononly permits pricing/context/output/streaming overrides), so the base mapping carries identical flags to every regional variant and the fallback is lossless. Extracted into a pureselectProviderMappinghelper in@llmgateway/actionswith unit tests.Verification
Live against AWS Bedrock (gateway run locally with real Bedrock key):
claude-opus-4-6→ bedrockclaude-opus-4-8→ bedrockaws-bedrock/claude-opus-4-6:globalanthropic/claude-opus-4-6claude-haiku-4-5(no reasoning support)pnpm formatand fullpnpm buildpass; existingprepare-request-bodyreasoning specs unchanged.(providerId, region)lookups in the rate-limit/uptime fallback and dev-plan paths are gated byrequestedProvider(the pinned path, where providers are already region-expanded), so they are not affected.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests