refactor: drop virtual model IDs and routing logic - #2199
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
WalkthroughProvider metrics keys were simplified to (modelId, providerId, region); code and tests across DB, actions, gateway/chat, and videos now build and read metrics using the reduced key shape and removed modelName-dependent helpers and entries. ChangesProvider Metrics Key Simplification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Refactors model routing/metrics to remove support for “virtual” Grok Fast model IDs, requiring callers to use the explicit reasoning/non-reasoning model IDs and simplifying metrics lookups accordingly.
Changes:
- Removes
grok-4-fast/grok-4-1-fastvirtual catalog entries and related routing behavior. - Drops
resolveMetricsModelIdand removesmodelNamefrommetricsKey/ provider-metrics APIs and call sites. - Simplifies
parseModelInputand deletes tests that existed only for virtual-model disambiguation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/models/src/models/xai.ts | Removes virtual Grok Fast catalog entries that previously fanned out to concrete variants. |
| packages/db/src/provider-metrics.ts | Simplifies metrics keying and metrics query payload by removing modelName from the metrics identity. |
| packages/db/src/provider-metrics.spec.ts | Updates tests to the new metricsKey(modelId, providerId, region) behavior and removes virtual-variant disambiguation coverage. |
| packages/actions/src/models.spec.ts | Removes resolveMetricsModelId tests and updates metrics-keyed fixtures to the new key shape. |
| packages/actions/src/get-cheapest-from-available-providers.ts | Removes resolveMetricsModelId and switches metrics lookups to use the catalog modelId directly. |
| apps/gateway/src/videos/videos.ts | Updates metrics combination construction / lookups to no longer include modelName/variant model IDs. |
| apps/gateway/src/chat/tools/parse-model-input.ts | Removes the multi-mapping “routing model” branch and always resolves to the provider mapping’s modelName when available. |
| apps/gateway/src/chat/chat.ts | Updates all metrics combination construction / lookups to use base model IDs and the simplified metricsKey. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
18
to
27
| /** | ||
| * Build a metrics map key from modelId, providerId, optional region, and | ||
| * optional provider modelName. Including modelName disambiguates virtual | ||
| * model variants (e.g. reasoning vs non-reasoning) that share the same | ||
| * (modelId, providerId, region) tuple in the routing tables. | ||
| * Build a metrics map key from modelId, providerId, and optional region. | ||
| */ | ||
| export function metricsKey( | ||
| modelId: string, | ||
| providerId: string, | ||
| region?: string | null, | ||
| modelName?: string | null, | ||
| ): string { | ||
| return `${modelId}:${providerId}:${region ?? ""}:${modelName ?? ""}`; | ||
| return `${modelId}:${providerId}:${region ?? ""}`; | ||
| } |
steebchen
force-pushed
the
remove-virtual-models
branch
from
May 13, 2026 11:12
a98bbef to
bc90077
Compare
Removes the grok-4-fast and grok-4-1-fast virtual catalog entries that
fanned out to concrete reasoning/non-reasoning siblings, along with the
helpers that existed solely to disambiguate them:
- resolveMetricsModelId (and call sites in chat.ts, videos.ts)
- modelName param on metricsKey / ProviderMetrics / combinations
- the multi-provider-mapping branch in parseModelInput
- the test fixtures and "virtual model variant routing" suite
Concrete grok-4-fast-{reasoning,non-reasoning} entries remain — callers
now request those directly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
steebchen
force-pushed
the
remove-virtual-models
branch
from
May 15, 2026 14:13
bc90077 to
f79e179
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@apps/gateway/src/chat/tools/parse-model-input.ts`:
- Around line 97-100: The current use of modelDef.providers.find(...) only picks
the first provider mapping and misses other mappings (e.g., multiple moonshot
entries); update the logic that sets requestedModel so it considers all mappings
in modelDef.providers for the requestedProvider: use filter(...) to collect all
entries where p.providerId === requestedProvider, then pick the correct mapping
either by matching an explicit version indicator in the incoming request (if
available) or by choosing the most recent/active mapping (compare version
strings/dates) and assign requestedModel from that chosen mapping (fall back to
modelName if none match); update the code around providerMapping,
modelDef.providers, requestedProvider, requestedModel and modelName accordingly.
🪄 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: f8b24359-701e-4d58-abca-5d13844040ff
📒 Files selected for processing (8)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/parse-model-input.tsapps/gateway/src/videos/videos.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.tspackages/db/src/provider-metrics.spec.tspackages/db/src/provider-metrics.tspackages/models/src/models/xai.ts
💤 Files with no reviewable changes (1)
- packages/models/src/models/xai.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/gateway/src/videos/videos.ts
- packages/db/src/provider-metrics.spec.ts
- packages/actions/src/get-cheapest-from-available-providers.ts
- packages/actions/src/models.spec.ts
- apps/gateway/src/chat/chat.ts
- packages/db/src/provider-metrics.ts
Comment on lines
+97
to
+100
| const providerMapping = modelDef.providers.find( | ||
| (p) => p.providerId === requestedProvider, | ||
| ); | ||
| if (providerMappings.length > 1) { | ||
| requestedModel = modelDef.id as Model; | ||
| } else if (providerMappings.length === 1) { | ||
| requestedModel = providerMappings[0].modelName; | ||
| } else { | ||
| requestedModel = modelName as Model; | ||
| } | ||
| requestedModel = (providerMapping?.modelName ?? modelName) as Model; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find model definition files (likely TS in models package)
fd -i 'xai.ts|models.ts|model.*\.ts|.*models.*\.ts' packages/models src apps 2>/dev/null || true
# Surface provider arrays where the same providerId appears more than once within one model object.
python - <<'PY'
import re, pathlib, json
roots = [pathlib.Path("packages/models"), pathlib.Path("apps"), pathlib.Path("src")]
files = []
for r in roots:
if r.exists():
files.extend([p for p in r.rglob("*.ts") if "node_modules" not in p.parts])
model_block_re = re.compile(r'\{[^{}]*?\bid\s*:\s*"([^"]+)"[^{}]*?\bproviders\s*:\s*\[', re.S)
provider_id_re = re.compile(r'providerId\s*:\s*"([^"]+)"')
found_any = False
for f in files:
text = f.read_text(encoding="utf-8", errors="ignore")
idx = 0
while True:
m = model_block_re.search(text, idx)
if not m:
break
model_id = m.group(1)
start = m.end() - 1 # at '['
depth = 0
end = start
for i in range(start, len(text)):
c = text[i]
if c == '[':
depth += 1
elif c == ']':
depth -= 1
if depth == 0:
end = i
break
providers_chunk = text[start:end+1]
provider_ids = provider_id_re.findall(providers_chunk)
dups = sorted({p for p in provider_ids if provider_ids.count(p) > 1})
if dups:
found_any = True
print(f"{f}: model '{model_id}' has duplicate providerId entries: {', '.join(dups)}")
idx = end + 1
if not found_any:
print("No duplicate providerId entries detected within model.providers arrays.")
PYRepository: theopenco/llmgateway
Length of output: 2052
🏁 Script executed:
# First, let's understand the repository structure and find model-related files
git ls-files | head -20Repository: theopenco/llmgateway
Length of output: 599
🏁 Script executed:
# Find files that likely contain model definitions
find . -path ./node_modules -prune -o -type f -name "*.ts" -print | grep -i model | head -30Repository: theopenco/llmgateway
Length of output: 1378
🏁 Script executed:
# Read the file under review to understand context
cat -n ./apps/gateway/src/chat/tools/parse-model-input.tsRepository: theopenco/llmgateway
Length of output: 5381
🏁 Script executed:
# Look at the model definitions structure
head -100 ./packages/models/src/models.tsRepository: theopenco/llmgateway
Length of output: 3381
🏁 Script executed:
# Continue reading to see model structure
sed -n '80,200p' ./packages/models/src/models.tsRepository: theopenco/llmgateway
Length of output: 3844
🏁 Script executed:
# Look at a specific model definition file to see structure
head -150 ./packages/models/src/models/openai.tsRepository: theopenco/llmgateway
Length of output: 3694
🏁 Script executed:
# Search all model definition files for duplicate providerId within the same model
python3 << 'EOF'
import re
import pathlib
# Find all model definition files in packages/models/src/models/
model_files = list(pathlib.Path("./packages/models/src/models").glob("*.ts"))
duplicate_found = False
for model_file in sorted(model_files):
content = model_file.read_text()
# Match model definitions: find id and its providers array
model_pattern = r'\{\s*id:\s*"([^"]+)"[^}]*?providers:\s*\['
for m in re.finditer(model_pattern, content, re.DOTALL):
model_id = m.group(1)
start = m.end() - 1 # Start at '['
# Find matching closing bracket
depth = 0
end = start
for i in range(start, len(content)):
if content[i] == '[':
depth += 1
elif content[i] == ']':
depth -= 1
if depth == 0:
end = i
break
providers_chunk = content[start:end+1]
# Find all providerId values
provider_ids = re.findall(r'providerId:\s*"([^"]+)"', providers_chunk)
# Check for duplicates
seen = {}
for pid in provider_ids:
if pid in seen:
duplicate_found = True
print(f"{model_file.name}: model '{model_id}' has duplicate providerId '{pid}'")
seen[pid] += 1
else:
seen[pid] = 1
if not duplicate_found:
print("✓ No duplicate providerId entries found within any model definition.")
EOFRepository: theopenco/llmgateway
Length of output: 129
🏁 Script executed:
# Find and display the kimi-k2 model definition
grep -A 100 'id: "kimi-k2"' ./packages/models/src/models/moonshot.ts | head -120Repository: theopenco/llmgateway
Length of output: 2488
Fix .find() logic to handle multiple provider mappings per model.
The code assumes only one providerId mapping exists per model, but kimi-k2 in moonshot.ts has two entries with providerId: "moonshot" (for versions kimi-k2-0711-preview and kimi-k2-0905-preview). When a user requests "moonshot/kimi-k2", .find() silently returns the first mapping, ignoring other versions. Either match both providerId and a version indicator, or select based on which version is most recent/active.
🤖 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/parse-model-input.ts` around lines 97 - 100, The
current use of modelDef.providers.find(...) only picks the first provider
mapping and misses other mappings (e.g., multiple moonshot entries); update the
logic that sets requestedModel so it considers all mappings in
modelDef.providers for the requestedProvider: use filter(...) to collect all
entries where p.providerId === requestedProvider, then pick the correct mapping
either by matching an explicit version indicator in the incoming request (if
available) or by choosing the most recent/active mapping (compare version
strings/dates) and assign requestedModel from that chosen mapping (fall back to
modelName if none match); update the code around providerMapping,
modelDef.providers, requestedProvider, requestedModel and modelName accordingly.
3 of 4 tasks
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
grok-4-fastandgrok-4-1-fastvirtual catalog entries that fanned out to concrete reasoning/non-reasoning siblings. Concretegrok-4-fast-{reasoning,non-reasoning}andgrok-4-1-fast-{reasoning,non-reasoning}entries remain — callers now request those directly.resolveMetricsModelIdand themodelNameparameter onmetricsKey/ProviderMetrics/getProviderMetricsForCombinations. Both existed only to disambiguate variants of a virtual model id.parseModelInput(no more multi-mapping branch) and removes the "virtual model variant routing" /resolveMetricsModelIdtest suites.No backwards compatibility shim — clients calling
model: "grok-4-1-fast"will now get a 400 and must switch to the explicit variant.Net diff: -749 lines.
Test plan
pnpm build— 17/17 taskspnpm exec vitest run packages/actions packages/db apps/gateway/src/chat— 427/427 testsgrok-4-fast-{reasoning,non-reasoning}andgrok-4-1-fast-{reasoning,non-reasoning}resolve correctly when called explicitly🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests