Conversation
📝 WalkthroughWalkthroughThis pull request adds support for the Mistral provider, enhances provider-specific field handling (Anthropic system, Google AI model), improves base URL parsing for versioned endpoints across providers, introduces a comprehensive live model smoke test suite, and updates project configuration. ChangesMistral Provider Support
Anthropic System Field Enhancement
Google AI Model Field Handling
Provider Base URL Versioning
Live Model Smoke Testing
Project Configuration
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 1/5 review remaining, refill in 40 minutes and 43 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
detection.go (1)
87-88: ⚡ Quick winUnify provider-prefix source of truth.
DetectProviderFromModelhardcodes prefixes whilestripProviderPrefixusesknownProviderPrefixesinautorouter.go(lines 546-557). This will drift on future provider additions.♻️ Proposed refactor
- switch prefix { - case "openai", "anthropic", "googleai", "groq", "fireworks", "xai", "perplexity", "bedrock", "azure", "mistral": - return prefix - } + if knownProviderPrefixes[prefix] { + return prefix + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@detection.go` around lines 87 - 88, DetectProviderFromModel currently hardcodes provider prefixes; change it to use the single source of truth by consulting knownProviderPrefixes (used by stripProviderPrefix in autorouter.go) instead of the hardcoded list. Update DetectProviderFromModel to iterate over the keys/prefixes in knownProviderPrefixes and return the matching prefix when the model string starts with it; if knownProviderPrefixes is in another file/package, make it accessible (export or move) so DetectProviderFromModel can reference it. This ensures provider additions only need to be added to knownProviderPrefixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models_live_test.go`:
- Around line 49-52: The test currently calls liveProviderEnv(models) before
checking liveSkipReason, which causes missing provider credentials to fail the
whole suite even when all of that provider's models would be skipped; change the
flow to compute the set of runnable models first (by applying liveSkipReason or
whatever skip predicate used in liveSkipReason to filter the models slice) and
then call liveProviderEnv(filteredModels), or alternatively move the env
resolution call to after the skip-gate where liveSkipReason is applied;
references: liveProviderEnv(models), liveSkipReason, envByProvider, missing.
- Around line 39-47: The test currently applies the LLMPROXY_LIVE_MODEL_LIMIT
slice before the LLMPROXY_LIVE_MODEL_IDS allowlist, which can drop explicitly
requested models; swap the order so you call envSet("LLMPROXY_LIVE_MODEL_IDS")
and run models = filterLiveModels(models, allowlist) (and keep the len(models)
== 0 t.Fatalf check) before applying envInt("LLMPROXY_LIVE_MODEL_LIMIT") and
truncating models; keep the same symbols (models, envSet, filterLiveModels,
envInt, LLMPROXY_LIVE_MODEL_IDS, LLMPROXY_LIVE_MODEL_LIMIT) and behavior
otherwise.
- Around line 155-180: In liveProviderEnv, when you find a set env value and
assign envByProvider[model.ProviderName] = value, also remove any previously
recorded stale missing entry for that provider by deleting
missingByProvider[model.ProviderName]; this ensures that if an earlier model
added missingByProvider[provider] but a later model supplies credentials
(checked via os.Getenv over envNames returned by liveEnvNames), the provider is
no longer reported missing. Reference symbols: function liveProviderEnv, maps
envByProvider and missingByProvider, loop variable model.ProviderName and
envNames, and os.Getenv.
- Around line 283-285: The helper currently hardcodes the Perplexity path to
"sonar", causing any non‑sonar Perplexity model to hit the wrong endpoint;
update the switch case for "perplexity" to use the selected model identifier
instead of the literal "sonar" by passing the chosen model name/ID (e.g.,
model.Name) into joinLiveURLPath along with model.Provider.API and "v1" so the
URL becomes joinLiveURLPath(model.Provider.API, "v1", model.Name) (use the
actual field that holds the model identifier in the model struct).
- Around line 401-412: stripLiveProviderPrefix currently removes everything
before the first slash regardless of whether that segment is a known provider,
which can alter real upstream model IDs; update stripLiveProviderPrefix to first
extract the prefix (before the first "/") and only perform the stripping logic
if that prefix is in a curated list/set of recognized provider prefixes (e.g.,
knownProviders or providerPrefixSet); keep the existing handling for repeated
prefixes (the strings.HasPrefix(..., prefix+"/") then TrimPrefix(...)) but gate
all of it behind the membership check so unknown prefixes and real model IDs
with slashes are left unchanged.
- Around line 414-425: The function joinLiveURLPath currently only strips a
leading "v1" duplicate; change the logic to generically deduplicate any
versioned or identical path segment by comparing the first element in elems
(elems[0]) to the base URL's last path segment (use strings.TrimRight(u.Path,
"/") and path.Base or equivalent) and drop elems[0] when they match; update the
conditional that references u.Path and elems so the join no longer produces
duplicate segments like "/v1beta/v1beta".
In `@providers/googleai/resolver.go`:
- Around line 25-27: The current normalization in resolver.go uses
strings.CutPrefix once which only strips a single "googleai/" prefix (variable
model); update the logic to remove repeated "googleai/" prefixes (same behavior
as autorouter.stripProviderPrefix) by repeatedly trimming the prefix until none
remain (e.g., loop or use strings.TrimPrefix in a loop) before building the
Google AI path so models like "googleai/googleai/gemini-2.0-flash" normalize to
"gemini-2.0-flash".
---
Nitpick comments:
In `@detection.go`:
- Around line 87-88: DetectProviderFromModel currently hardcodes provider
prefixes; change it to use the single source of truth by consulting
knownProviderPrefixes (used by stripProviderPrefix in autorouter.go) instead of
the hardcoded list. Update DetectProviderFromModel to iterate over the
keys/prefixes in knownProviderPrefixes and return the matching prefix when the
model string starts with it; if knownProviderPrefixes is in another
file/package, make it accessible (export or move) so DetectProviderFromModel can
reference it. This ensures provider additions only need to be added to
knownProviderPrefixes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b25ed51-5d6e-43a8-97f3-19a69e9ff894
📒 Files selected for processing (15)
.gitignoreautorouter.goautorouter_test.godetection.godetection_test.gointerceptors/billing_test.gomodels_live_test.goproviders/anthropic/parser.goproviders/anthropic/parser_test.goproviders/googleai/parser.goproviders/googleai/parser_test.goproviders/googleai/resolver.goproviders/openai_compatible/parser_test.goproviders/perplexity/provider_test.goproviders/perplexity/resolver.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🧰 Additional context used
🪛 golangci-lint (2.11.4)
providers/perplexity/resolver.go
[error] 28-28: S1017: should replace this if statement with an unconditional strings.TrimSuffix
(staticcheck)
🔇 Additional comments (12)
.gitignore (1)
2-4: Good ignore coverage for local/integration artifacts.These additions reduce accidental commits of local config and generated files.
detection_test.go (1)
141-141: Nice coverage addition for mistral-prefixed routing.This test closes the gap for explicit
mistral/...model detection.autorouter.go (1)
556-570: Prefix stripping update looks good.Including
mistraland handling repeated provider prefixes improves normalization before upstream forwarding.interceptors/billing_test.go (1)
288-288: Good billing test extension for mistral-prefixed models.This keeps billing detection aligned with routing/provider detection changes.
autorouter_test.go (1)
295-298: Great table coverage for repeated and mistral prefixes.These cases directly validate the new normalization behavior.
providers/openai_compatible/parser_test.go (1)
395-411: Nice regression test for versioned custom base URLs.This is a strong guardrail for resolver normalization behavior.
providers/perplexity/resolver.go (1)
19-32: Base URL normalization change is solid.Trimming trailing slash and terminal
/v1makes resolver output stable for versioned inputs.providers/perplexity/provider_test.go (1)
52-67: Good regression coverage for versioned Perplexity bases.This pins the
/v1/trailing-slash case the resolver normalization is meant to support.providers/anthropic/parser.go (1)
58-60: Nice use of the existing content normalization forsystem.This keeps string and block-array prompts on the same path and avoids empty
meta.Custom["system"]entries.Also applies to: 90-90
providers/anthropic/parser_test.go (1)
48-59: Good coverage for array-form system prompts.This closes the gap introduced by switching
systemto flexible content parsing.providers/googleai/parser.go (1)
91-91: Parsingmodelas a first-class field here makes the routing metadata consistent.Also good call excluding it from
Customonce it becomes known input.Also applies to: 152-154
providers/googleai/parser_test.go (1)
35-49: Useful regression coverage for both parsed models and prefixed resolution.These two cases pin the parser/resolver contract that this PR relies on.
Also applies to: 150-165
| if limit := envInt("LLMPROXY_LIVE_MODEL_LIMIT", 0); limit > 0 && limit < len(models) { | ||
| models = models[:limit] | ||
| } | ||
| if allowlist := envSet("LLMPROXY_LIVE_MODEL_IDS"); len(allowlist) > 0 { | ||
| models = filterLiveModels(models, allowlist) | ||
| if len(models) == 0 { | ||
| t.Fatalf("no models matched LLMPROXY_LIVE_MODEL_IDS") | ||
| } | ||
| } |
There was a problem hiding this comment.
Apply LLMPROXY_LIVE_MODEL_IDS before LLMPROXY_LIVE_MODEL_LIMIT.
Right now the limit truncates the sorted model list before the allowlist runs. An explicitly requested model that falls after the first N entries will never be considered, and the suite can fail with no models matched even though the ID is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 39 - 47, The test currently applies the
LLMPROXY_LIVE_MODEL_LIMIT slice before the LLMPROXY_LIVE_MODEL_IDS allowlist,
which can drop explicitly requested models; swap the order so you call
envSet("LLMPROXY_LIVE_MODEL_IDS") and run models = filterLiveModels(models,
allowlist) (and keep the len(models) == 0 t.Fatalf check) before applying
envInt("LLMPROXY_LIVE_MODEL_LIMIT") and truncating models; keep the same symbols
(models, envSet, filterLiveModels, envInt, LLMPROXY_LIVE_MODEL_IDS,
LLMPROXY_LIVE_MODEL_LIMIT) and behavior otherwise.
| envByProvider, missing := liveProviderEnv(models) | ||
| if len(missing) > 0 { | ||
| t.Fatalf("missing provider env vars:\n%s", strings.Join(missing, "\n")) | ||
| } |
There was a problem hiding this comment.
Don't require provider creds for models that will be skipped anyway.
This preflight runs before liveSkipReason, so a provider whose selected models are all skipped can still abort the entire suite with a missing-env fatal. Filter runnable models first, or move env resolution after the skip gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 49 - 52, The test currently calls
liveProviderEnv(models) before checking liveSkipReason, which causes missing
provider credentials to fail the whole suite even when all of that provider's
models would be skipped; change the flow to compute the set of runnable models
first (by applying liveSkipReason or whatever skip predicate used in
liveSkipReason to filter the models slice) and then call
liveProviderEnv(filteredModels), or alternatively move the env resolution call
to after the skip-gate where liveSkipReason is applied; references:
liveProviderEnv(models), liveSkipReason, envByProvider, missing.
| func liveProviderEnv(models []liveModel) (map[string]string, []string) { | ||
| envByProvider := make(map[string]string) | ||
| missingByProvider := make(map[string][]string) | ||
|
|
||
| for _, model := range models { | ||
| if envByProvider[model.ProviderName] != "" { | ||
| continue | ||
| } | ||
| envNames := liveEnvNames(model) | ||
| for _, name := range envNames { | ||
| if value := os.Getenv(name); value != "" { | ||
| envByProvider[model.ProviderName] = value | ||
| break | ||
| } | ||
| } | ||
| if envByProvider[model.ProviderName] == "" { | ||
| missingByProvider[model.ProviderName] = envNames | ||
| } | ||
| } | ||
|
|
||
| var missing []string | ||
| for provider, envNames := range missingByProvider { | ||
| missing = append(missing, fmt.Sprintf("%s: set one of %s", provider, strings.Join(envNames, ", "))) | ||
| } | ||
| slices.Sort(missing) | ||
| return envByProvider, missing |
There was a problem hiding this comment.
Clear stale missingByProvider entries when a later model resolves credentials.
The function records a provider as missing the first time one model has no matching env var, but it never deletes that entry if a later model for the same provider exposes a different env name that is set. The final preflight can therefore fail purely because of model ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 155 - 180, In liveProviderEnv, when you
find a set env value and assign envByProvider[model.ProviderName] = value, also
remove any previously recorded stale missing entry for that provider by deleting
missingByProvider[model.ProviderName]; this ensures that if an earlier model
added missingByProvider[provider] but a later model supplies credentials
(checked via os.Getenv over envNames returned by liveEnvNames), the provider is
no longer reported missing. Reference symbols: function liveProviderEnv, maps
envByProvider and missingByProvider, loop variable model.ProviderName and
envNames, and os.Getenv.
| case "perplexity": | ||
| return joinLiveURLPath(model.Provider.API, "v1", "sonar") | ||
| default: |
There was a problem hiding this comment.
Use the selected Perplexity model in the path.
Perplexity resolution is model-path based (/v1/{model}), but this helper hardcodes /v1/sonar. Any smoke test for a different Perplexity model will hit the wrong endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 283 - 285, The helper currently hardcodes
the Perplexity path to "sonar", causing any non‑sonar Perplexity model to hit
the wrong endpoint; update the switch case for "perplexity" to use the selected
model identifier instead of the literal "sonar" by passing the chosen model
name/ID (e.g., model.Name) into joinLiveURLPath along with model.Provider.API
and "v1" so the URL becomes joinLiveURLPath(model.Provider.API, "v1",
model.Name) (use the actual field that holds the model identifier in the model
struct).
| func stripLiveProviderPrefix(model string) string { | ||
| idx := strings.Index(model, "/") | ||
| if idx < 0 { | ||
| return model | ||
| } | ||
| prefix := model[:idx] | ||
| stripped := model[idx+1:] | ||
| if strings.HasPrefix(stripped, prefix+"/") { | ||
| stripped = strings.TrimPrefix(stripped, prefix+"/") | ||
| } | ||
| return stripped | ||
| } |
There was a problem hiding this comment.
Only strip recognized provider prefixes.
This helper drops everything before the first slash, even when the slash is part of the real upstream model ID. That can make the smoke test send a different model than the models API advertised. Mirror the production prefix stripping and only normalize known provider prefixes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 401 - 412, stripLiveProviderPrefix
currently removes everything before the first slash regardless of whether that
segment is a known provider, which can alter real upstream model IDs; update
stripLiveProviderPrefix to first extract the prefix (before the first "/") and
only perform the stripping logic if that prefix is in a curated list/set of
recognized provider prefixes (e.g., knownProviders or providerPrefixSet); keep
the existing handling for repeated prefixes (the strings.HasPrefix(...,
prefix+"/") then TrimPrefix(...)) but gate all of it behind the membership check
so unknown prefixes and real model IDs with slashes are left unchanged.
| func joinLiveURLPath(base string, elems ...string) string { | ||
| u, err := url.Parse(base) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if len(elems) > 0 && elems[0] == "v1" && strings.HasSuffix(strings.TrimRight(u.Path, "/"), "/v1") { | ||
| elems = elems[1:] | ||
| } | ||
| all := []string{strings.TrimRight(u.Path, "/")} | ||
| all = append(all, elems...) | ||
| u.Path = pathJoin(all...) | ||
| u.RawQuery = "" |
There was a problem hiding this comment.
Deduplicate versioned base paths generically, not only /v1.
joinLiveURLPath handles bases that already end in /v1, but not /v1beta or other versioned prefixes. For Google AI, a base like .../v1beta would become .../v1beta/v1beta/models/... and fail even though the base URL is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models_live_test.go` around lines 414 - 425, The function joinLiveURLPath
currently only strips a leading "v1" duplicate; change the logic to generically
deduplicate any versioned or identical path segment by comparing the first
element in elems (elems[0]) to the base URL's last path segment (use
strings.TrimRight(u.Path, "/") and path.Base or equivalent) and drop elems[0]
when they match; update the conditional that references u.Path and elems so the
join no longer produces duplicate segments like "/v1beta/v1beta".
| if stripped, ok := strings.CutPrefix(model, "googleai/"); ok { | ||
| model = stripped | ||
| } |
There was a problem hiding this comment.
Strip repeated googleai/ prefixes, not just one.
strings.CutPrefix only removes the first occurrence. If meta.Model arrives as googleai/googleai/gemini-2.0-flash, this still resolves to /models/googleai/gemini-2.0-flash:generateContent, which is not a valid Google AI model path. Reuse the same normalization behavior as autorouter.stripProviderPrefix, or loop until the prefix is gone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/googleai/resolver.go` around lines 25 - 27, The current
normalization in resolver.go uses strings.CutPrefix once which only strips a
single "googleai/" prefix (variable model); update the logic to remove repeated
"googleai/" prefixes (same behavior as autorouter.stripProviderPrefix) by
repeatedly trimming the prefix until none remain (e.g., loop or use
strings.TrimPrefix in a loop) before building the Google AI path so models like
"googleai/googleai/gemini-2.0-flash" normalize to "gemini-2.0-flash".
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores