revert(feat): improve auto routing - #1969
Conversation
This reverts commit 9cf3d62.
WalkthroughThis PR refactors the auto-routing logic by removing the Claude-preference-based Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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.
Pull request overview
This PR reverts prior “improve auto routing” work by removing the default auto-model helper + unit test, and adjusting the gateway’s auto routing logic and related e2e expectations.
Changes:
- Removes
getDefaultAutoModelId(and its spec) used for Claude-based default auto selection. - Reworks
autorouting selection inchat.tsto use a hardcoded allowlist of candidate models and a simplified inlined selection loop. - Updates individual e2e tests for
autorouting behavior and skip conditions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/gateway/src/chat/tools/get-default-auto-model-id.ts | Deletes the helper for picking a default auto model based on context size. |
| apps/gateway/src/chat/tools/get-default-auto-model-id.spec.ts | Removes unit tests for the deleted helper. |
| apps/gateway/src/chat/chat.ts | Updates auto routing candidate selection and fallback behavior. |
| apps/gateway/src/api-individual.e2e.ts | Updates e2e tests/skip logic and assertions for auto routing. |
Comments suppressed due to low confidence (1)
apps/gateway/src/api-individual.e2e.ts:337
- This test inserts an API key with id
token-credits-auto, butcreateTestData("credits-auto")already inserts an API key with the same id (token-${testId}). This will violate the unique constraint and fail the test. Use a different id for the credits token (or update the existing row instead of inserting a second one).
const { userId, orgId, projectId } = await createTestData("credits-auto");
await db
.update(tables.organization)
.set({ credits: "1000" })
.where(eq(tables.organization.id, orgId));
await db
.update(tables.project)
.set({ mode: "credits" })
.where(eq(tables.project.id, projectId));
const creditsToken = "credits-token-auto";
await db.insert(tables.apiKey).values({
id: "token-credits-auto",
token: creditsToken,
projectId: projectId,
description: "Test API Key for Credits",
createdBy: userId,
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Default fallback if no suitable model is found - use cheapest allowed model | ||
| usedModel = "gpt-5-nano"; | ||
| usedProvider = "openai"; |
There was a problem hiding this comment.
The auto-routing fallback hardcodes usedProvider = "openai". This can select a provider that is not in availableProviders (no env/db key) or is disallowed by IAM allow/deny provider rules, and it also bypasses the normal provider-selection path that would choose an eligible provider-region mapping. Prefer leaving usedProvider undefined (or selecting from candidateIam.allowedProviders ∩ availableProviders) so the generic routing logic can pick a permitted provider for the fallback model.
| // Default fallback if no suitable model is found - use cheapest allowed model | |
| usedModel = "gpt-5-nano"; | |
| usedProvider = "openai"; | |
| // Default fallback if no suitable model is found - use cheapest allowed model. | |
| // Leave provider selection unset so the normal routing logic can choose | |
| // an eligible provider/region mapping for the fallback model. | |
| usedModel = "gpt-5-nano"; |
| const allowedAutoModels = ["gpt-oss-120b", "gpt-5-nano", "gpt-4.1-nano"]; | ||
|
|
||
| let selectedModel: ModelDefinition | undefined; | ||
| let selectedProviders: any[] = []; |
There was a problem hiding this comment.
selectedProviders is typed as any[], which defeats type-checking for the provider mapping fields used later (e.g., providerId, contextSize, deprecatedAt, inputPrice). Use the concrete mapping type (e.g., ProviderModelMapping[]) to keep the auto-routing logic safe during future refactors.
| let selectedProviders: any[] = []; | |
| let selectedProviders: ProviderModelMapping[] = []; |
| // require all provider keys to be set | ||
| for (const provider of providers) { | ||
| const envVarName = getProviderEnvVar(provider.id); | ||
| const envVarValue = envVarName ? process.env[envVarName] : undefined; | ||
| if (!envVarValue) { | ||
| console.log( | ||
| `Skipping llmgateway/auto in credits mode test - no API key provided for ${provider.id}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The credits auto-routing test now requires all provider env vars to be set in order to run. This makes the test extremely likely to be skipped in most environments and reduces coverage of the auto-routing path. Consider only requiring the providers that can actually be selected by the auto-routing candidates under test (e.g., the providers backing allowedAutoModels), or at minimum the provider you expect the test to route to.
| // require all provider keys to be set | |
| for (const provider of providers) { | |
| const envVarName = getProviderEnvVar(provider.id); | |
| const envVarValue = envVarName ? process.env[envVarName] : undefined; | |
| if (!envVarValue) { | |
| console.log( | |
| `Skipping llmgateway/auto in credits mode test - no API key provided for ${provider.id}`, | |
| ); | |
| return; | |
| } | |
| // Require at least one provider key so the auto-routing path can run. | |
| // Requiring every provider key makes this test skip in most environments. | |
| const availableProviders = providers.filter((provider) => { | |
| const envVarName = getProviderEnvVar(provider.id); | |
| return envVarName ? Boolean(process.env[envVarName]) : false; | |
| }); | |
| if (availableProviders.length === 0) { | |
| console.log( | |
| "Skipping llmgateway/auto in credits mode test - no provider API keys configured", | |
| ); | |
| return; |
| const { userId, orgId, projectId } = await createTestData( | ||
| `credits-auto-${Date.now()}`, | ||
| ); | ||
| const { userId, orgId, projectId } = await createTestData("credits-auto"); |
There was a problem hiding this comment.
createTestData does not clean up the DB between runs (only clearCache() runs in beforeEach), so using fixed ids like credits-auto can cause primary-key collisions when the suite is rerun against the same database. Reintroduce a per-run unique suffix (e.g., Date.now() / generateTestRequestId()) or add DB cleanup for this file to keep these e2e tests repeatable.
![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: a4f91bdcd1
ℹ️ 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".
| usedModel = "gpt-5-nano"; | ||
| usedProvider = "openai"; |
There was a problem hiding this comment.
When auto cannot find a suitable candidate, this fallback pins routing to openai/gpt-5-nano instead of leaving provider selection dynamic. In credits/hybrid environments that do not configure an OpenAI key (for example, orgs using only Anthropic or other providers), getProviderEnv("openai") later throws a 500, so auto requests fail even though other providers are available. The previous behavior (usedProvider = undefined) allowed downstream routing to pick an allowed configured provider or return a proper availability error.
Useful? React with 👍 / 👎.
| const creditsToken = "credits-token-auto"; | ||
| await db.insert(tables.apiKey).values({ | ||
| id: `token-credits-auto-${creditsSuffix}`, | ||
| id: "token-credits-auto", |
There was a problem hiding this comment.
This insert reuses id: "token-credits-auto", but createTestData("credits-auto") already creates an API key with the same id (token-${testId}), so the second insert hits a primary-key/unique violation when the test reaches this path. As written, the test will fail in environments where its provider-key preconditions are satisfied.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api-individual.e2e.ts`:
- Around line 306-315: The test currently requires every provider in providers
to have an API key, causing the auto-routing credits test to be skipped in most
environments; change the gate to require at least one provider with a valid API
key instead of all. Locate the provider check using providers,
getProviderEnvVar(provider.id) and envVarValue, replace the all-provider loop
with a check like providers.some(...) to detect any provider that has a
configured env var and only skip (with the existing log) when none are present;
ensure subsequent test code uses the available provider(s) accordingly.
In `@apps/gateway/src/chat/chat.ts`:
- Around line 1406-1408: The fallback sets usedModel="gpt-5-nano" and
usedProvider="openai" unconditionally which can bypass provider-level IAM
checks; update the fallback logic in the selection flow (references: usedModel,
usedProvider, allowedProviders and the IAM revalidation block) to choose a
provider from allowedProviders that supports the chosen model (or choose an
allowed model/provider pair), and if no allowed provider supports the fallback
model, pick the cheapest model-provider pair from allowedProviders or return a
clear authorization/error path instead of hardcoding "openai".
- Around line 1242-1252: The hybrid auto-routing path is expanding all regions
without applying the same key-availability filtering used for credits, which can
select regions lacking valid keys and cause routing failures; update the
construction of candidateProviders (the call to preferConcreteRegionalMappings)
so that when project.mode === "hybrid" you also run
expandAllProviderRegions(modelDef.providers as ProviderModelMapping[]) through
filterRegionsByAvailableKeys (the same filter used for "credits") before passing
to preferConcreteRegionalMappings, ensuring both "credits" and "hybrid" modes
only consider regions with available keys.
🪄 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: eb35ef56-9ff3-4c9a-832e-4f8b339c2366
📒 Files selected for processing (4)
apps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-default-auto-model-id.spec.tsapps/gateway/src/chat/tools/get-default-auto-model-id.ts
💤 Files with no reviewable changes (2)
- apps/gateway/src/chat/tools/get-default-auto-model-id.spec.ts
- apps/gateway/src/chat/tools/get-default-auto-model-id.ts
| // require all provider keys to be set | ||
| for (const provider of providers) { | ||
| const envVarName = getProviderEnvVar(provider.id); | ||
| const envVarValue = envVarName ? process.env[envVarName] : undefined; | ||
| if (!envVarValue) { | ||
| console.log( | ||
| `Skipping llmgateway/auto in credits mode test - no API key provided for ${provider.id}`, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This gate will skip the auto-routing credits test in most environments.
Requiring keys for every provider (providers) makes this test effectively non-running unless the environment is fully populated. That removes meaningful coverage for the auto-routing path.
🔧 Proposed fix
- // require all provider keys to be set
- for (const provider of providers) {
- const envVarName = getProviderEnvVar(provider.id);
- const envVarValue = envVarName ? process.env[envVarName] : undefined;
- if (!envVarValue) {
- console.log(
- `Skipping llmgateway/auto in credits mode test - no API key provided for ${provider.id}`,
- );
- return;
- }
- }
+ // require at least one provider key eligible for auto-routing
+ const autoModelIds = new Set(["gpt-oss-120b", "gpt-5-nano", "gpt-4.1-nano"]);
+ const autoProviderIds = new Set(
+ models
+ .filter((m) => autoModelIds.has(m.id))
+ .flatMap((m) => m.providers.map((p) => p.providerId)),
+ );
+ const hasAnyAutoProviderKey = Array.from(autoProviderIds).some((providerId) => {
+ const envVarName = getProviderEnvVar(providerId);
+ return Boolean(envVarName && process.env[envVarName]);
+ });
+ if (!hasAnyAutoProviderKey) {
+ console.log(
+ "Skipping llmgateway/auto in credits mode test - no eligible auto-routing provider key configured",
+ );
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/api-individual.e2e.ts` around lines 306 - 315, The test
currently requires every provider in providers to have an API key, causing the
auto-routing credits test to be skipped in most environments; change the gate to
require at least one provider with a valid API key instead of all. Locate the
provider check using providers, getProviderEnvVar(provider.id) and envVarValue,
replace the all-provider loop with a check like providers.some(...) to detect
any provider that has a configured env var and only skip (with the existing log)
when none are present; ensure subsequent test code uses the available
provider(s) accordingly.
| const candidateProviders = preferConcreteRegionalMappings( | ||
| project.mode === "credits" | ||
| ? filterRegionsByAvailableKeys( | ||
| expandAllProviderRegions( | ||
| modelDef.providers as ProviderModelMapping[], | ||
| ), | ||
| ); | ||
| // Check if any of the model's providers are available | ||
| const availableModelProviders = candidateProviders.filter( | ||
| (provider) => | ||
| availableProviders.includes(provider.providerId) && | ||
| (!candidateAllowedProviders || | ||
| candidateAllowedProviders.includes(provider.providerId)), | ||
| ); | ||
|
|
||
| // Filter by context size requirement, reasoning capability, and deprecation status | ||
| const suitableProviders = availableModelProviders.filter((provider) => { | ||
| // Skip deprecated provider mappings | ||
| if (provider.deprecatedAt && now > provider.deprecatedAt!) { | ||
| return false; | ||
| } | ||
|
|
||
| // Use the provider's context size, defaulting to a reasonable value if not specified | ||
| const modelContextSize = provider.contextSize ?? 8192; | ||
| const contextSizeMet = modelContextSize >= requiredContextSize; | ||
| ) | ||
| : expandAllProviderRegions( | ||
| modelDef.providers as ProviderModelMapping[], | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Hybrid auto-routing can choose regions without valid key coverage.
Line 1243 applies region-key filtering only for credits. In hybrid, this path expands all regions, which can pick non-default regions that were intentionally filtered elsewhere (see the project-mode filtering contract above this block). That can cause avoidable routing failures.
🔧 Proposed fix
- const candidateProviders = preferConcreteRegionalMappings(
- project.mode === "credits"
- ? filterRegionsByAvailableKeys(
- expandAllProviderRegions(
- modelDef.providers as ProviderModelMapping[],
- ),
- )
- : expandAllProviderRegions(
- modelDef.providers as ProviderModelMapping[],
- ),
- );
+ const expandedCandidates = expandAllProviderRegions(
+ modelDef.providers as ProviderModelMapping[],
+ );
+ const candidateProviders = preferConcreteRegionalMappings(
+ project.mode === "credits"
+ ? filterRegionsByAvailableKeys(expandedCandidates)
+ : project.mode === "hybrid"
+ ? filterHybridRegions(expandedCandidates)
+ : expandedCandidates,
+ );🤖 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 1242 - 1252, The hybrid
auto-routing path is expanding all regions without applying the same
key-availability filtering used for credits, which can select regions lacking
valid keys and cause routing failures; update the construction of
candidateProviders (the call to preferConcreteRegionalMappings) so that when
project.mode === "hybrid" you also run
expandAllProviderRegions(modelDef.providers as ProviderModelMapping[]) through
filterRegionsByAvailableKeys (the same filter used for "credits") before passing
to preferConcreteRegionalMappings, ensuring both "credits" and "hybrid" modes
only consider regions with available keys.
| // Default fallback if no suitable model is found - use cheapest allowed model | ||
| usedModel = "gpt-5-nano"; | ||
| usedProvider = "openai"; |
There was a problem hiding this comment.
Hardcoded auto fallback can bypass provider-level IAM restrictions.
At Line 1407-Line 1408, fallback pins openai, but after IAM revalidation (Line 1436+), usedProvider is not reconciled with allowedProviders. If IAM denies openai for gpt-5-nano but allows another provider for that model, this can still proceed with the denied provider.
🔧 Proposed fix
- usedModel = "gpt-5-nano";
- usedProvider = "openai";
+ usedModel = "gpt-5-nano";
+ usedProvider = "openai";
...
const allowedProviders = resolvedIamValidation.allowedProviders;
+ if (
+ usedProvider &&
+ allowedProviders &&
+ !allowedProviders.includes(usedProvider)
+ ) {
+ const allowedMapping = modelInfo.providers.find((p) =>
+ allowedProviders.includes(p.providerId),
+ );
+ if (!allowedMapping) {
+ throwIamException(
+ `No providers are allowed for model ${modelInfo.id} after applying IAM rules`,
+ );
+ }
+ usedProvider = allowedMapping.providerId;
+ usedModel = allowedMapping.modelName;
+ usedRegion = allowedMapping.region;
+ }
iamFilteredModelProviders = allowedProvidersAlso applies to: 1436-1450
🤖 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 1406 - 1408, The fallback sets
usedModel="gpt-5-nano" and usedProvider="openai" unconditionally which can
bypass provider-level IAM checks; update the fallback logic in the selection
flow (references: usedModel, usedProvider, allowedProviders and the IAM
revalidation block) to choose a provider from allowedProviders that supports the
chosen model (or choose an allowed model/provider pair), and if no allowed
provider supports the fallback model, pick the cheapest model-provider pair from
allowedProviders or return a clear authorization/error path instead of
hardcoding "openai".
Reverts #1962
Summary by CodeRabbit