feat(strategies/cost): add CapabilityAggregator to prevent opaque-cost misrouting#241
feat(strategies/cost): add CapabilityAggregator to prevent opaque-cost misrouting#241gr3enarr0w wants to merge 6 commits into
Conversation
Adds providers/nanogpt — an aggregator integration with nano-gpt.com that proxies requests to 20+ upstream models (Anthropic, OpenAI, xAI, DeepSeek, Moonshot, Google, Mistral, etc.). Implements core.Provider, core.StreamProvider, core.ProxiableProvider, and core.DiscoveryProvider via the existing internal/openaicompat helpers. Splits out the NanoGPT portion of PR ferro-labs#230 as requested by @MitulShah1. Parked alongside ferro-labs#226 pending aggregator design decision. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… name - Change defaultBaseURL from https://ollama.com (website) to https://api.ollama.com (actual Ollama Cloud API endpoint) - Rename env var gate from OLLAMA_API_KEY to OLLAMA_CLOUD_API_KEY to avoid ambiguity with the local Ollama provider (OLLAMA_HOST) - Update docker-compose.yml comment to reflect both corrections The Ollama Cloud provider was introduced without these fixes, which would cause default-configured instances to point at the marketing website rather than the inference API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gators from cost-optimized routing Aggregator providers (OpenRouter, NanoGPT) have SupportsModel()=true for all models and opaque upstream costs that include platform markups. When CostOptimized uses catalog prices for these providers, it silently misroutes — the catalog entry doesn't reflect the aggregator's fee. Adds CapabilityAggregator = "aggregator" to the capability constants and tags OpenRouter and NanoGPT. In CostOptimized.Execute(), any provider tagged as an aggregator is forced to hasPrice=false regardless of catalog entries, making it fall through to unpricedStrategy (default: "fallback" — only used when no direct priced provider is available). Wired via WithAggregatorPredicate in gateway.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd NanoGPT model list Three bugs found in code review: 1. allow-mode bypass: isAggregator forced costUSD=0.0, but under unpricedStrategy=allow the hasPrice guard is skipped, making the aggregator win every cost comparison at $0.00. Fix: add isAggregator field to priced struct; skip aggregators in ranking loop unconditionally (mode-independent). Also fix positional fallback (candidates[0]) to prefer the first non-aggregator target. 2. Streaming gap: streamingCostOrderLocked built candidates from raw catalog prices with no aggregator check. All three switch cases now filter candidates where isAggregator=true. Uses ProviderHasCapability directly (mirrors the strategy predicate). 3. NanoGPT hallucinated models: SupportedModels() contained fabricated version numbers (gpt-5.5, claude-opus-4.7, grok-4.20, gemini-3.5-flash, etc.). Replaced with real current model names. Discovery via DiscoverModels() remains the authoritative source. Adds tests: - TestCostOptimized_AggregatorExcludedUnderAllowMode - TestCostOptimized_AggregatorPositionalFallbackSkippedWhenDirectExists Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
MitulShah1
left a comment
MitulShah1
left a comment
There was a problem hiding this comment.
Nice iteration on this @gr3enarr0w — the fix is in good shape now. Skipping aggregators before the unpriced/allow check (costoptimized.go:161) closes the bypass in all modes, both the sync and streaming cost paths are covered, and tagging both OpenRouter and NanoGPT with CapabilityAggregator is the right structural move. TestCostOptimized_AggregatorExcludedWhenPricedDirectProviderExists is a genuinely good test.
A few things before I'd merge:
- The allow-mode test doesn't actually exercise the bug it names —
TestCostOptimized_AggregatorExcludedUnderAllowModeusesbuildCatalog(), which only hascheap/+expensive/entries (noagg//direct/), so both providers resolve unpriced and the headline scenario — a priced aggregator that looks cheapest underallow— isn't covered. Detail inline. - Scope creep — this is a cost-routing change, but it also adds a brand-new NanoGPT provider and the
OLLAMA_API_KEY→OLLAMA_CLOUD_API_KEYrename (which also lives in #243 and will conflict). Please split those out so this PR is just the aggregator fix + tagging. - Rationale wording — the "aggregator's own platform markup (OpenRouter +5.5%)" justification isn't accurate: OpenRouter passes token pricing through with no per-token markup; the 5.5% is a one-time credit-purchase fee. The behavior is still right (a single static catalog price can't be trusted for an aggregator that routes dynamically), but let's reword the reason.
- Please get Go CI running (only Snyk reported) — this adds a package and touches the stability matrix.
One design question I'll also raise on the issue: since OpenRouter really does pass prices through, should aggregator exclusion be a configurable policy (like unpricedStrategy) rather than a hard default? Not a blocker, just worth deciding deliberately.
| agg := &mockProvider{name: "agg", models: []string{"gpt-4o"}, resp: &providers.Response{ID: "agg"}} | ||
| direct := &mockProvider{name: "direct", models: []string{"gpt-4o"}, resp: &providers.Response{ID: "direct"}} | ||
|
|
||
| catalog := buildCatalog() // direct has pricing, agg has pricing (aggregator markup ignored) |
There was a problem hiding this comment.
This comment isn't accurate, and it hides a coverage gap: buildCatalog() only contains cheap/gpt-4o and expensive/gpt-4o — there's no agg/ or direct/ entry. So both agg and direct resolve to ModelFound=false (unpriced), and the test passes via the unpriced-at-$0 path + target ordering, not the scenario it claims. The actual bug from the issue — a priced aggregator that appears cheapest and would win under allow — still isn't exercised anywhere (the WhenPricedDirectProviderExists test covers the priced case but in default mode, not allow).
Suggest an inline catalog here where agg/gpt-4o is priced cheapest (e.g. 0.01) and direct/gpt-4o higher (e.g. 5.0), in allow mode, asserting direct wins. That version would fail if the isAggregator skip on line 161 were removed — which is what makes it a real regression test. Same false-fixture note applies to the comment on line 465.
| }) | ||
| } | ||
| // Aggregator providers are never selected by cost ranking: their catalog | ||
| // prices do not include the aggregator's own platform markup. They are |
There was a problem hiding this comment.
Small accuracy fix on the rationale: OpenRouter (and most aggregators) pass underlying token pricing through with no per-token markup — the 5.5% people cite is a one-time credit-purchase fee, not a per-request surcharge. So "prices do not include the aggregator's own platform markup" isn't quite right. The real justification for excluding them is sound though: an aggregator routes dynamically across sub-providers, so a single static catalog price doesn't reliably reflect the effective per-request cost. Suggest rewording to that.
| @@ -0,0 +1,113 @@ | |||
| // Package nanogpt provides a client for the NanoGPT API. | |||
There was a problem hiding this comment.
This net-new NanoGPT provider (and its test) is unrelated to the cost-routing fix — can it go in its own PR? It'd make this one a clean, reviewable "aggregator exclusion" change. (The provider itself looks fine and is correctly tagged CapabilityAggregator in providers_list.go, which is good — just want it decoupled.)
| Capabilities: []string{CapabilityChat, CapabilityStream, CapabilityDiscovery}, | ||
| EnvMappings: []EnvMapping{ | ||
| {CfgKeyAPIKey, "OLLAMA_API_KEY", true}, | ||
| {CfgKeyAPIKey, "OLLAMA_CLOUD_API_KEY", true}, |
There was a problem hiding this comment.
This OLLAMA_API_KEY → OLLAMA_CLOUD_API_KEY rename is the same breaking change that's also in #243 — having it in two open PRs will conflict, and either way it's a breaking config change for existing deployments (OLLAMA_API_KEY is shipped and is Ollama's own convention). Please drop it from this PR; let's settle the env-var question on #243.
…ord rationale - Split the net-new NanoGPT provider and the OLLAMA_API_KEY->OLLAMA_CLOUD_API_KEY rename back out of this PR: NanoGPT already ships separately in ferro-labs#240, and the rename duplicates/conflicts with ferro-labs#243's already-merged (as ferro-labs#328) direction. This PR is now scoped to just the aggregator cost-ranking exclusion + tagging OpenRouter with CapabilityAggregator. - Fix TestCostOptimized_AggregatorExcludedUnderAllowMode: it used buildCatalog(), which has no agg/ or direct/ entries, so both candidates resolved unpriced and the test never exercised its own premise (a *priced* aggregator that looks cheapest under allow mode). Gave it a catalog where the aggregator carries a real, low price so the exclusion path is actually exercised. - Reworded the aggregator-exclusion rationale throughout: the reason isn't a hidden "platform markup" (OpenRouter passes token pricing through with no per-token markup - the often-cited 5.5% is a one-time credit-purchase fee, not a per-request charge). The real reason is that an aggregator routes each request to whichever underlying provider/model it picks at call time, so a single static catalog price can't represent the actual per-request cost. Note: CI has never run on this branch at all (no Actions runs recorded, not even a pending one) - this needs a maintainer to approve the first workflow run for this fork branch before Go tests/lint will execute.
… nanogpt deletion)
📝 WalkthroughWalkthroughCost-optimized routing now recognizes aggregator providers, excludes them from price ranking, and uses them as fallback candidates. OpenRouter is marked as an aggregator, while Ollama Cloud defaults use updated API endpoint and authentication configuration. ChangesAggregator-aware routing
Ollama Cloud endpoint configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Gateway
participant CostOptimized
participant ProviderRegistry
Gateway->>CostOptimized: Execute cost-optimized request
CostOptimized->>ProviderRegistry: Check aggregator capability
CostOptimized-->>Gateway: Return direct provider or aggregator fallback
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway.go (1)
1714-1732: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix streaming fallback order to deprioritize aggregators.
Currently, when
rankedis empty or when unranked candidates are appended tokeys, the loop preserves the original configuredtargetsorder. This means an aggregator could precede an unpriced direct provider if placed higher in the YAML list, violating the PR objective which states aggregators should only be used when no direct provider supports the model.Reorder the unranked concatenation loops to append direct unpriced providers before aggregators, maintaining parity with the non-streaming positional fallback logic.
♻️ Proposed fix
- if len(ranked) == 0 { - if g.config.Strategy.UnpricedStrategy == unpricedStrategySkip { - return nil, fmt.Errorf("no priced provider supports model %s", req.Model) - } - return targetKeys(targets), nil - } + if len(ranked) == 0 && g.config.Strategy.UnpricedStrategy == unpricedStrategySkip { + return nil, fmt.Errorf("no priced provider supports model %s", req.Model) + } sort.SliceStable(ranked, func(i, j int) bool { return ranked[i].costUSD < ranked[j].costUSD }) keys := make([]string, 0, len(targets)) for _, candidate := range ranked { keys = appendUniqueKey(keys, candidate.key) } - for _, candidate := range candidates { - keys = appendUniqueKey(keys, candidate.key) - } + // Positional fallback for unranked candidates: prefer direct providers over aggregators. + for _, candidate := range candidates { + if !candidate.isAggregator { + keys = appendUniqueKey(keys, candidate.key) + } + } + for _, candidate := range candidates { + if candidate.isAggregator { + keys = appendUniqueKey(keys, candidate.key) + } + } return appendRemainingTargetKeys(keys, targets), nil🤖 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 `@gateway.go` around lines 1714 - 1732, Update the fallback ordering around the ranked-provider handling so direct unpriced providers are appended before aggregators, matching the non-streaming positional fallback behavior. Apply this ordering both when ranked is empty and when appending unranked candidates, while preserving the existing skip error, deduplication, and remaining-target handling.
🤖 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 `@docker-compose.yml`:
- Around line 67-69: Update the Ollama provider mapping in the provider-list
definition to read the documented OLLAMA_CLOUD_API_KEY variable, while retaining
OLLAMA_API_KEY as a compatibility fallback if supported by the existing mapping
structure. Keep the surrounding Ollama provider configuration unchanged.
In `@gateway.go`:
- Around line 964-967: Snapshot g.catalog before constructing the cost-optimized
strategy, matching the existing providerSnap pattern, and pass the snapshot to
strategies.NewCostOptimized instead of g.catalog. Ensure the snapshot is
independent of the live catalog so Strategy.Execute and models.Calculate cannot
observe concurrent catalog mutations.
---
Outside diff comments:
In `@gateway.go`:
- Around line 1714-1732: Update the fallback ordering around the ranked-provider
handling so direct unpriced providers are appended before aggregators, matching
the non-streaming positional fallback behavior. Apply this ordering both when
ranked is empty and when appending unranked candidates, while preserving the
existing skip error, deduplication, and remaining-target handling.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 49996fc3-dfdb-44cc-9241-ecc569463f10
📒 Files selected for processing (7)
docker-compose.ymlgateway.gointernal/strategies/costoptimized.gointernal/strategies/costoptimized_test.goproviders/factory.goproviders/ollama_cloud/ollama_cloud.goproviders/providers_list.go
| # - OLLAMA_CLOUD_API_KEY=... | ||
| # - OLLAMA_CLOUD_MODELS=gpt-oss:20b,gpt-oss:120b | ||
| # - OLLAMA_CLOUD_BASE_URL=https://ollama.com | ||
| # - OLLAMA_CLOUD_BASE_URL=https://api.ollama.com |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the documented API-key variable with the provider mapping.
providers/providers_list.go:294-314 still reads the required key from OLLAMA_API_KEY, so configuring only the newly documented OLLAMA_CLOUD_API_KEY leaves the provider without credentials and causes initialization to fail. Update the provider mapping to use OLLAMA_CLOUD_API_KEY (or retain OLLAMA_API_KEY here for compatibility).
Suggested provider mapping
- {CfgKeyAPIKey, "OLLAMA_API_KEY", true},
+ {CfgKeyAPIKey, "OLLAMA_CLOUD_API_KEY", true},This follows the provider wiring shown in providers/providers_list.go:294-314.
🤖 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 `@docker-compose.yml` around lines 67 - 69, Update the Ollama provider mapping
in the provider-list definition to read the documented OLLAMA_CLOUD_API_KEY
variable, while retaining OLLAMA_API_KEY as a compatibility fallback if
supported by the existing mapping structure. Keep the surrounding Ollama
provider configuration unchanged.
| s = strategies.NewCostOptimized(targets, lookup, g.catalog, g.config.Strategy.UnpricedStrategy). | ||
| WithAggregatorPredicate(func(vk string) bool { | ||
| return providers.ProviderHasCapability(vk, providers.CapabilityAggregator) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent data race on g.catalog.
Passing g.catalog directly by reference creates a data race. The logic executed inside Strategy.Execute (which invokes the models.Calculate method against this map) runs without g.mu held, while the gateway concurrently mutates or replaces the catalog map in place (e.g., during config reloads, as implied by g.Catalog() relying on maps.Copy). You should snapshot it similarly to how providerSnap is handled above.
🐛 Proposed fix
- s = strategies.NewCostOptimized(targets, lookup, g.catalog, g.config.Strategy.UnpricedStrategy).
+ s = strategies.NewCostOptimized(targets, lookup, maps.Clone(g.catalog), g.config.Strategy.UnpricedStrategy).
WithAggregatorPredicate(func(vk string) bool {
return providers.ProviderHasCapability(vk, providers.CapabilityAggregator)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s = strategies.NewCostOptimized(targets, lookup, g.catalog, g.config.Strategy.UnpricedStrategy). | |
| WithAggregatorPredicate(func(vk string) bool { | |
| return providers.ProviderHasCapability(vk, providers.CapabilityAggregator) | |
| }) | |
| s = strategies.NewCostOptimized(targets, lookup, maps.Clone(g.catalog), g.config.Strategy.UnpricedStrategy). | |
| WithAggregatorPredicate(func(vk string) bool { | |
| return providers.ProviderHasCapability(vk, providers.CapabilityAggregator) | |
| }) |
🤖 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 `@gateway.go` around lines 964 - 967, Snapshot g.catalog before constructing
the cost-optimized strategy, matching the existing providerSnap pattern, and
pass the snapshot to strategies.NewCostOptimized instead of g.catalog. Ensure
the snapshot is independent of the live catalog so Strategy.Execute and
models.Calculate cannot observe concurrent catalog mutations.
|
Addressed all four points:
Full local test suite (build, vet, race, short) passes. |
2 participants
Closes #244
Problem
CostOptimizedroutes to the cheapest provider by looking up prices in the model catalog. For aggregator providers like OpenRouter (already in the codebase) and NanoGPT (#240),SupportsModel()returnstruefor every model. If their catalog price is lower than a direct provider's,CostOptimizedpicks the aggregator — but the catalog price does not include the aggregator's own platform markup. The gateway silently routes through an aggregator with unknown real cost.There's also a bug under
unpricedStrategy: allow: forcinghasPrice=falseon an aggregator doesn't help becauseallowmode bypasses the!hasPriceguard, so the aggregator wins atcostUSD=0.0.Solution
Add
CapabilityAggregator = "aggregator"to the existingCapabilityXxxconstants and exclude aggregator-tagged providers from cost ranking in both the strategy and the streaming path.Changes
providers/factory.goCapabilityAggregator = "aggregator"constantproviders/providers_list.goCapabilityAggregator(retroactive fix)CapabilityAggregator(see feat(providers): add NanoGPT aggregator provider #240)internal/strategies/costoptimized.goisAggregator boolfield topricedcandidate structWithAggregatorPredicate(fn func(virtualKey string) bool) *CostOptimizedbuilderselectCostOptimizedCandidateskips aggregators unconditionally — the critical fix for the allow-mode bypassgateway.gostreamingCostOrderLocked(parallel streaming cost path) now also excludes aggregatorsWithAggregatorPredicatewhen constructingCostOptimizedproviders/nanogpt/nanogpt.goSupportedModels()with real onesinternal/strategies/costoptimized_test.goTestCostOptimized_AggregatorExcludedUnderAllowModeTestCostOptimized_AggregatorPositionalFallbackSkippedWhenDirectExistsBehavior change
unpricedStrategyfallback(default)skipallowNo breaking changes for users without aggregators configured.
Unblocks
Test plan
go test ./internal/strategies/... -run TestCostOptimizedgo test ./providers/...go test ./...🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes