fix(gateway): enforce IAM provider fallback rules - #1819
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces usage of Changes
Sequence Diagram(s)mermaid Client->>Gateway: request(model, options) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 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.
Pull request overview
This PR tightens IAM enforcement in the Gateway so provider allow/deny rules are applied consistently even when a specific provider is requested, and ensures provider fallback/selection logic only considers IAM-permitted providers.
Changes:
- Update IAM rule evaluation so
allow_providers/deny_providersalways produce an IAM-filtered provider set (including whenrequestedProvideris specified). - Route provider selection and low-uptime fallback filtering through the IAM-filtered provider mappings in
chat.ts. - Add regression assertions ensuring
allowedProvidersreflects IAM filtering for requested-provider scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/gateway/src/lib/iam.ts | Ensures provider allow/deny rules update the allowed-provider set even when a provider was explicitly requested. |
| apps/gateway/src/lib/iam.spec.ts | Adds regression expectations that allowedProviders is correctly filtered for requested-provider cases. |
| apps/gateway/src/chat/chat.ts | Uses IAM-filtered provider mappings for available-provider filtering, selection, and fallback routing paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/gateway/src/lib/iam.ts (1)
135-150:⚠️ Potential issue | 🟠 MajorKeep price-limit checks scoped to the IAM-filtered provider set.
These new intersections make
currentAllowedProvidersthe source of truth, butallow_pricingstill walks everymodelDef.providersentry. That means a provider already removed bydeny_providers/allow_providerscan still block access later. Example: ifopenaiis denied first,anthropicremains allowed, and a latermaxInputPriceonly excludesopenai, the model is still rejected even though the surviving provider is within policy.Possible fix
case "allow_pricing": if ( ruleValue.maxInputPrice !== undefined || ruleValue.maxOutputPrice !== undefined ) { for (const provider of modelDef.providers) { + if (!currentAllowedProviders.has(provider.providerId)) { + continue; + } + if (requestedProvider && provider.providerId !== requestedProvider) { continue; }Also applies to: 165-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/lib/iam.ts` around lines 135 - 150, The pricing checks in allow_pricing (and the similar block later) still iterate modelDef.providers instead of the IAM-filtered set, allowing providers previously removed by deny_providers/allow_providers to veto access; update allow_pricing to iterate only over the IAM-filtered provider set (use currentAllowedProviders/newAllowedProviders) when computing price limits and deciding allowedProviders, and when requestedProvider is present ensure you validate it against and return it from that same filtered set (use the same newAllowedProviders/currentAllowedProviders intersection rather than modelDef.providers).apps/gateway/src/chat/chat.ts (1)
1010-1050:⚠️ Potential issue | 🟠 MajorApply the full request-capability filter during low-uptime fallback.
This path now honors IAM, but it still only screens on web search / JSON / vision. A request with
reasoning_effortcan be rerouted here to a provider that the normal!usedProviderpath would reject, and that later path never runs onceusedProviderhas been reassigned.🤖 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 1010 - 1050, The low-uptime fallback filter (building availableModelProviders from iamFilteredModelProviders) currently only checks webSearchTool, response_format, and hasImages but misses other request capabilities like reasoning_effort, causing misrouted providers when usedProvider changes; update the filter to apply the full request-capability checks used elsewhere (e.g., include a check for reasoning_effort and any other capability flags tested in the normal provider-selection path) or refactor to reuse the same capability-checking helper (create/use a function like filterProvidersByRequestCapabilities(provider, request) and call it inside the availableModelProviders filter) so availableModelProviders and the regular selection path enforce identical capability constraints.
🤖 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 1217-1237: The current hasNonReasoningAlternative check can
wrongly prefer a non-reasoning sibling that is actually unusable for this
request; change the predicate so the "non-reasoning alternative" must also
satisfy the same eligibility checks you apply to mappings elsewhere (i.e., the
same capability/requirement filters such as vision, json_schema, etc.) before
deciding to exclude the reasoning mapping. Concretely, when computing
hasNonReasoningAlternative from modelInfo.providers, only count mappings with
matching providerId and reasoning !== true that also pass the same
usable/matching logic you apply to provider mappings in this function (or call a
shared helper like isMappingEligible(mapping, currentRequestRequirements) if one
exists) so you only prefer a non-reasoning sibling if it is actually usable for
this request.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1010-1050: The low-uptime fallback filter (building
availableModelProviders from iamFilteredModelProviders) currently only checks
webSearchTool, response_format, and hasImages but misses other request
capabilities like reasoning_effort, causing misrouted providers when
usedProvider changes; update the filter to apply the full request-capability
checks used elsewhere (e.g., include a check for reasoning_effort and any other
capability flags tested in the normal provider-selection path) or refactor to
reuse the same capability-checking helper (create/use a function like
filterProvidersByRequestCapabilities(provider, request) and call it inside the
availableModelProviders filter) so availableModelProviders and the regular
selection path enforce identical capability constraints.
In `@apps/gateway/src/lib/iam.ts`:
- Around line 135-150: The pricing checks in allow_pricing (and the similar
block later) still iterate modelDef.providers instead of the IAM-filtered set,
allowing providers previously removed by deny_providers/allow_providers to veto
access; update allow_pricing to iterate only over the IAM-filtered provider set
(use currentAllowedProviders/newAllowedProviders) when computing price limits
and deciding allowedProviders, and when requestedProvider is present ensure you
validate it against and return it from that same filtered set (use the same
newAllowedProviders/currentAllowedProviders intersection rather than
modelDef.providers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce2c311a-a6ac-40df-bf8a-b1b14b8ade84
📒 Files selected for processing (3)
apps/gateway/src/chat/chat.tsapps/gateway/src/lib/iam.spec.tsapps/gateway/src/lib/iam.ts
| // If reasoning_effort is specified, only include providers with reasoning support | ||
| if (reasoning_effort !== undefined) { | ||
| if ((provider as ProviderModelMapping).reasoning !== true) { | ||
| return false; | ||
| } | ||
| } | ||
| // If reasoning_effort is NOT specified, prefer non-reasoning providers | ||
| // by excluding reasoning providers when a non-reasoning alternative exists for same provider | ||
| if (reasoning_effort === undefined) { | ||
| const hasNonReasoningAlternative = modelInfo.providers.some( | ||
| (p) => | ||
| p.providerId === provider.providerId && | ||
| (p as ProviderModelMapping).reasoning !== true, | ||
| ); | ||
| if ( | ||
| hasNonReasoningAlternative && | ||
| (provider as ProviderModelMapping).reasoning === true | ||
| ) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Only prefer a non-reasoning sibling if it is actually usable.
hasNonReasoningAlternative scans every modelInfo.providers mapping for the same provider ID. Because capability flags are per mapping, this can discard the reasoning mapping even when the non-reasoning sibling fails the current request's other requirements (for example vision or JSON-schema support), leaving no selectable mapping for a provider that actually could serve the request.
🤖 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 1217 - 1237, The current
hasNonReasoningAlternative check can wrongly prefer a non-reasoning sibling that
is actually unusable for this request; change the predicate so the
"non-reasoning alternative" must also satisfy the same eligibility checks you
apply to mappings elsewhere (i.e., the same capability/requirement filters such
as vision, json_schema, etc.) before deciding to exclude the reasoning mapping.
Concretely, when computing hasNonReasoningAlternative from modelInfo.providers,
only count mappings with matching providerId and reasoning !== true that also
pass the same usable/matching logic you apply to provider mappings in this
function (or call a shared helper like isMappingEligible(mapping,
currentRequestRequirements) if one exists) so you only prefer a non-reasoning
sibling if it is actually usable for this request.
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/fallback.spec.ts (1)
32-54:⚠️ Potential issue | 🟠 MajorReset
modelProviderMappingbetween tests.
setRoutingMetricsmutates seeded routing data, but this cleanup only clears request-scoped tables. That makes later suites inherit the last low-uptime values and turns this file into order-dependent test state. Please reset or restore the touchedmodelProviderMappingrows here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/fallback.spec.ts` around lines 32 - 54, The test suite mutates routing state via setRoutingMetrics which alters the modelProviderMapping rows, so update the beforeEach to reset modelProviderMapping to its seeded/default state: after clearing tables call the same seeding logic or explicitly restore modelProviderMapping rows (e.g., using db.insert/update against modelProviderMapping) so tests do not inherit low-uptime values; reference the beforeEach hook, setRoutingMetrics, and modelProviderMapping when locating where to add the reset.
🧹 Nitpick comments (1)
apps/gateway/src/fallback.spec.ts (1)
611-727: Add one positive low-uptime reroute case.These new cases only assert “no fallback”. The suite would still go green if low-uptime rerouting were broken entirely. A control case where
together.aiis unhealthy andcerebrasis IAM-allowed would prove the reroute path still works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/fallback.spec.ts` around lines 611 - 727, Add a positive control test that verifies low-uptime rerouting works: create a new test (e.g., "reroutes to IAM-allowed healthy provider when requested provider is 0% uptime") inside the same describe block, use setupMultiProviderKeys() and setRoutingMetrics(modelId, "together.ai", 0) / setRoutingMetrics(modelId, "cerebras", 100), insertIamRules to allow only "cerebras" (via insertIamRules with ruleType "allow_providers" and providers ["cerebras"]), send the same app.request payload with model "together.ai/llama-3.1-8b-instruct", then assert the response metadata.used_provider is "cerebras", metadata.requested_provider is "together.ai", and that waitForLogs(1) shows logs[0].usedProvider === "cerebras" and logs[0].routingMetadata.selectedProvider === "cerebras" and logs[0].routingMetadata.selectionReason === "low-uptime-fallback".
🤖 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/fallback.spec.ts`:
- Around line 151-170: The update in setRoutingMetrics silently does nothing if
the modelId/providerId pair is missing; capture the result of db.update(...)
against tables.modelProviderMapping and assert that at least one row was
affected (e.g., throw or fail the test if the affected-row count is 0) so the
test will fail fast when the mapping is not present rather than letting
low-uptime tests pass without exercising the fallback branch.
---
Outside diff comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 32-54: The test suite mutates routing state via setRoutingMetrics
which alters the modelProviderMapping rows, so update the beforeEach to reset
modelProviderMapping to its seeded/default state: after clearing tables call the
same seeding logic or explicitly restore modelProviderMapping rows (e.g., using
db.insert/update against modelProviderMapping) so tests do not inherit
low-uptime values; reference the beforeEach hook, setRoutingMetrics, and
modelProviderMapping when locating where to add the reset.
---
Nitpick comments:
In `@apps/gateway/src/fallback.spec.ts`:
- Around line 611-727: Add a positive control test that verifies low-uptime
rerouting works: create a new test (e.g., "reroutes to IAM-allowed healthy
provider when requested provider is 0% uptime") inside the same describe block,
use setupMultiProviderKeys() and setRoutingMetrics(modelId, "together.ai", 0) /
setRoutingMetrics(modelId, "cerebras", 100), insertIamRules to allow only
"cerebras" (via insertIamRules with ruleType "allow_providers" and providers
["cerebras"]), send the same app.request payload with model
"together.ai/llama-3.1-8b-instruct", then assert the response
metadata.used_provider is "cerebras", metadata.requested_provider is
"together.ai", and that waitForLogs(1) shows logs[0].usedProvider === "cerebras"
and logs[0].routingMetadata.selectedProvider === "cerebras" and
logs[0].routingMetadata.selectionReason === "low-uptime-fallback".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f16fc3f2-3037-4994-82bf-9331f9c8fe82
📒 Files selected for processing (1)
apps/gateway/src/fallback.spec.ts
| async function setRoutingMetrics( | ||
| modelId: string, | ||
| providerId: string, | ||
| routingUptime: number, | ||
| ) { | ||
| await db | ||
| .update(tables.modelProviderMapping) | ||
| .set({ | ||
| routingUptime, | ||
| routingLatency: 100, | ||
| routingThroughput: 100, | ||
| routingTotalRequests: 100, | ||
| }) | ||
| .where( | ||
| and( | ||
| eq(tables.modelProviderMapping.modelId, modelId), | ||
| eq(tables.modelProviderMapping.providerId, providerId), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fail fast when the routing-metrics setup is a no-op.
If the modelId/providerId pair stops existing in seeded data, this update silently affects zero rows and the low-uptime tests below can still pass without ever exercising the fallback branch. Add an assertion that the target mapping was actually updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/fallback.spec.ts` around lines 151 - 170, The update in
setRoutingMetrics silently does nothing if the modelId/providerId pair is
missing; capture the result of db.update(...) against
tables.modelProviderMapping and assert that at least one row was affected (e.g.,
throw or fail the test if the affected-row count is 0) so the test will fail
fast when the mapping is not present rather than letting low-uptime tests pass
without exercising the fallback branch.
Summary
Testing
Summary by CodeRabbit