feat(pricing): add admin-managed model pricing overrides#306
Merged
SantiagoDePolonia merged 7 commits intomainfrom May 7, 2026
Merged
feat(pricing): add admin-managed model pricing overrides#306SantiagoDePolonia merged 7 commits intomainfrom
SantiagoDePolonia merged 7 commits intomainfrom
Conversation
Adds dashboard + API for operator-supplied USD pricing that shadows model registry and config.yaml pricing in usage cost calculations. The most-specific selector wins (provider+model > model > provider > global). Also fixes pricing lookup in usage paths to use the canonical model id (e.g. gpt-4o-mini) instead of the provider's versioned response model (e.g. gpt-4o-mini-2024-07-18), which previously could miss base prices. Selector grammar shared with model access overrides was extracted to a new internal/modelselectors package. ModelMetadata now carries a PricingSources map so the UI can show where each price came from. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a persisted USD model pricing overrides subsystem: selector normalization, stores (SQLite/Postgres/Mongo), an in-memory snapshot service with validation/rollback and background refresh, admin REST API and OpenAPI schemas, dashboard editor (UI/CSS/JS), app wiring for a pricingResolver, and tests. ChangesModel Pricing Overrides System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–90 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryAdds a complete admin-managed model pricing override subsystem: an in-memory-cached, store-backed service (SQLite/PostgreSQL/MongoDB) that resolves effective pricing at four scopes (exact → model-wide → provider-wide → global) and applies DB overrides on top of the base registry during usage cost calculation. The PR also threads a specific
Confidence Score: 5/5The change is safe to merge; the new subsystem is well-isolated and the gateway modifications are narrowly scoped to improving pricing-model lookup accuracy. The rollback-with-reconciliation path, atomic snapshot swaps, and tier validation are all handled correctly. All error paths in app.go include the new pricingOverrides.Close() call. The pricing resolution chain is correct across gateway, stream observer, and cache-hit paths. internal/modelselectors/selectors.go — the trailing-slash input for an unregistered provider is silently folded into a model-only override rather than rejected. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
REQ["Incoming Request\n(model, provider)"] --> GW["Gateway / StreamObserver\nusagePricingModel()"]
GW --> |"pricingModel, providerName"| PR["pricingoverrides.Service\nResolvePricing()"]
PR --> SNAP["Snapshot.matchingOverride()"]
SNAP --> |"exact: provider+model"| E["exact[provider/model]"]
SNAP --> |"model-wide"| M["modelWide[model]"]
SNAP --> |"provider-wide"| P["providerWide[provider]"]
SNAP --> |"global"| G["global override"]
SNAP --> |"no match"| BASE["Base Registry Pricing"]
E --> MERGE["mergePricing(base, override)"]
M --> MERGE
P --> MERGE
G --> MERGE
MERGE --> COST["Usage Cost Calculation"]
BASE --> COST
ADMIN["Admin API\nPUT /model-pricing-overrides/{selector}"] --> SVC["Service.Upsert()"]
SVC --> NORM["NormalizeInput\n(catalog + selector)"]
NORM --> STORE["Store.Upsert\n(SQLite/PG/Mongo)"]
STORE --> REFRESH["refreshLocked()\natomic snapshot swap"]
REFRESH --> SNAP
Reviews (3): Last reviewed commit: "fix(pricing): align tier schema validati..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR introduces an admin-managed model pricing override system with four selector scopes (exact provider+model, model-wide, provider-wide, global), backed by SQLite/PostgreSQL/MongoDB stores, an atomic in-memory snapshot, and a usage cost resolver that applies DB overrides at inference time for both streaming and non-streaming paths.
Confidence Score: 3/5The change is mostly safe to merge; the pricing resolution and snapshot logic are well-tested, but the nil-interface default in handler.go can produce a panic in the usage recalculation endpoint if wiring is ever incomplete. The admin usage recalculation endpoint now calls h.pricingResolver which defaults to registry — a nil registry produces a non-nil interface wrapping a nil pointer. Any call to RecalculateUsagePricing without WithPricingResolver would panic rather than return a graceful error. This is a present risk on the changed path. internal/admin/handler.go (nil-interface default for pricingResolver) and internal/pricingoverrides/validation.go (mutual-exclusivity of UpToTokens/UpToMtok per tier) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as InferenceOrchestrator
participant PricingSvc as PricingOverrides.Service
participant Snapshot as In-Memory Snapshot
participant Store as DB Store
participant UsageLog as UsageLogger
Client->>Gateway: POST /v1/chat/completions
Gateway->>Gateway: usagePricingModel resolves gpt-4o-mini
Gateway->>PricingSvc: ResolvePricing(gpt-4o-mini, openai)
PricingSvc->>Snapshot: matchingOverride(openai, gpt-4o-mini)
Note right of Snapshot: Priority: exact > model-wide > provider-wide > global
Snapshot-->>PricingSvc: override or nil
PricingSvc-->>Gateway: merged ModelPricing
Gateway->>UsageLog: Write entry with effective pricing
Note over Store, Snapshot: Background refresh every 60s
Store-->>Snapshot: store.List atomic snapshot swap
participant AdminAPI as Admin API
AdminAPI->>PricingSvc: Upsert Override
PricingSvc->>Store: store.Upsert normalized
PricingSvc->>Store: store.List refreshLocked
Store-->>Snapshot: new snapshot stored atomically
PricingSvc-->>AdminAPI: View with ScopeKind
Reviews (2): Last reviewed commit: "fix(pricing): preserve slash-shaped over..." | Re-trigger Greptile |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/admin/handler_usage.go (1)
237-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
h.registrynil-guard doesn't protect the actual argument used in the callLine 260 passes
h.pricingResolver, but the nil guard on Line 241 checksh.registry. If wiring ever produces a non-nilh.registryalongside a nilh.pricingResolver(e.g., a custom pricing resolver path omits the default), the guard passes and line 260 panics. The guard should protect what it actually uses.🛡️ Proposed fix
- if h.registry == nil { - return handleError(c, featureUnavailableError("model pricing metadata is unavailable")) - } + if h.pricingResolver == nil { + return handleError(c, featureUnavailableError("model pricing metadata is unavailable")) + }🤖 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 `@internal/admin/handler_usage.go` around lines 237 - 242, The nil-check currently validates h.registry but the subsequent call uses h.pricingResolver, so replace or add the guard to check h.pricingResolver (and keep the existing h.registry check if both are required) before proceeding; specifically, ensure that before the call that references h.pricingResolver you validate h.pricingResolver != nil and return handleError(c, featureUnavailableError("model pricing metadata is unavailable" or a clearer message for missing pricing resolver)) if it's nil so the code cannot panic when h.pricingResolver is absent.internal/providers/registry_metadata.go (1)
321-329:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PricingSourcesis fully replaced instead of merged, silently dropping registry sources for non-overridden fieldsWhen
override.Pricingis set but only contains some fields (e.g., onlyInputPerMtok), callingFieldSources(...)returns entries only for those non-nil fields. Assigning that directly tomerged.PricingSourcesreplaces the entire map, discarding any registry-sourced entries that were already present for fields the override doesn't touch.Example: base model from registry has
{input_per_mtok: ModelRegistry, output_per_mtok: ModelRegistry}. A config YAML override sets onlyInputPerMtok. After line 323,PricingSourcesbecomes{input_per_mtok: ConfigYAML}andoutput_per_mtok's source is lost—despite the merged price value still coming from the registry.Merge into the existing map instead of replacing it:
Proposed fix
if override.Pricing != nil { - merged.PricingSources = override.Pricing.FieldSources(core.ModelPricingSourceConfigYAML) + for field, src := range override.Pricing.FieldSources(core.ModelPricingSourceConfigYAML) { + if merged.PricingSources == nil { + merged.PricingSources = make(map[string]core.ModelPricingSource) + } + merged.PricingSources[field] = src + } }🤖 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 `@internal/providers/registry_metadata.go` around lines 321 - 329, The current code replaces merged.PricingSources outright when override.Pricing is non-nil, which discards registry-origin entries for fields the override doesn't touch; instead, initialize merged.PricingSources if nil and copy/merge the entries from override.Pricing.FieldSources(core.ModelPricingSourceConfigYAML) into the existing merged.PricingSources map (overwriting only the keys present in the override), so registry sources for untouched fields are preserved; update the block around merged := modeldata.MergeMetadata(...) / if override.Pricing != nil { ... } to perform a map-merge rather than direct assignment.
🤖 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 `@cmd/gomodel/docs/docs.go`:
- Around line 5421-5438: The OpenAPI enum for pricingoverrides.ScopeKind
contains duplicated entries; update the "pricingoverrides.ScopeKind" enum array
to contain only the four unique values in the same order as x-enum-varnames
("global", "model", "provider", "provider_model") so the enum length matches
x-enum-varnames and client generation won't break.
In `@docs/openapi.json`:
- Around line 830-835: The array response schema for the modeloverrides list
endpoints is unbounded; update the array schema (the object with "type": "array"
and "items": {"$ref":"#/components/schemas/modeloverrides.View"}) to include a
reasonable "maxItems" property (and optionally "minItems") to cap the response
size so clients have clear bounds; apply the same change to both occurrences
(the one referencing modeloverrides.View at the shown location and the other
occurrence around lines 1034-1038).
- Around line 5362-5369: The schema admin.upsertModelPricingOverrideRequest
currently defines a pricing property but doesn't mark it required; update this
schema to include "required": ["pricing"] so the pricing field (referencing
components.schemas.pricingoverrides.Pricing) is documented as mandatory for the
request payload, ensuring the OpenAPI spec reflects the API requirement.
- Around line 7023-7034: The pricingoverrides.ScopeKind enum in the OpenAPI spec
contains duplicated literals; update the pricingoverrides.ScopeKind schema to
list each enum value only once (keep "global", "model", "provider",
"provider_model" as unique entries) so the enum array has no duplicates, then
save the modified openapi.json (and regenerate any generated clients/schemas if
applicable) to ensure validators and generators accept the cleaned enum.
In `@internal/admin/dashboard/templates/page-models.html`:
- Around line 228-239: The labels lack for attributes and the select/input lack
unique ids, so add bound ids using the row identifier and reference them from
the labels; for the select in the pricing-override-type-field create an id bound
to the row (e.g. "pricing-type-" + row.id) and set the label's for to the same
bound id, and likewise for the pricing-override-value-field create a bound id
(e.g. "pricing-value-" + row.id) and set that label's for to match so the
elements tied to x-model="row.field" and x-model="row.value" are properly
associated and unique per row.
In `@internal/admin/handler_model_pricing_overrides_test.go`:
- Around line 95-103: The test only checks HTTP status for
ListModelPricingOverrides; extend it to assert the response body contains the
expected items by decoding listRec.Body into the expected response struct or
slice and comparing key fields (e.g., model ID, price, or override entries)
against the expected fixtures set up in the test; update the test in
internal/admin/handler_model_pricing_overrides_test.go to unmarshal listRec.Body
(after calling h.ListModelPricingOverrides) and use assertions to validate the
returned list length and at least one representative record matches the expected
values for the created overrides.
In `@internal/admin/handler.go`:
- Around line 253-257: NewHandler assigns pricingResolver: registry which
creates a non-nil interface wrapping a nil concrete when registry is nil; change
NewHandler so it explicitly sets the pricingResolver interface to a true nil
when registry is nil (e.g., declare a local var pr PricingResolver = nil and if
registry != nil set pr = registry) and then assign pricingResolver: pr, ensuring
RecalculateUsagePricing can safely check the interface for nil; update
references to the Handler fields (pricingResolver, registry) as needed.
In `@internal/core/types.go`:
- Around line 262-301: Add brief godoc comments above the exported constants
ModelPricingSourceModelRegistry and ModelPricingSourceConfigYAML describing they
are string identifiers for where pricing data originated (registry vs config
YAML), and add a godoc comment for the exported method
(*ModelPricing).FieldSources explaining it returns a map of non-empty pricing
field names to the provided source string, that callers should pass a non-empty
source, and that pricing tiers (Tiers) are reported only as a single
coarse-grained "tiers" key rather than per-tier entries; place these comments
directly above the constants and the FieldSources method declarations.
In `@internal/modeldata/merger_test.go`:
- Around line 154-156: TestResolve_ProviderModelOverride currently only asserts
meta.PricingSources["input_per_mtok"]; add a parallel assertion that
meta.PricingSources["output_per_mtok"] equals
core.ModelPricingSourceModelRegistry so both InputPerMtok and OutputPerMtok from
the provider override are covered; update the test to check
meta.PricingSources["output_per_mtok"] with the same t.Errorf pattern used for
"input_per_mtok" to maintain consistency.
In `@internal/pricingoverrides/resolver.go`:
- Around line 10-23: Normalize providerName and model (call strings.TrimSpace on
providerName and modelIDFromSelector(model, providerName)) before invoking
base.ResolvePricing so the base lookup uses the same normalized identifiers;
move the providerName/model normalization above the base pricing lookup and pass
the normalized model and providerName into s.base.ResolvePricing (references:
ResolvePricing, modelIDFromSelector, providerName, base.ResolvePricing,
cloneBasePricing, mergePricing, snapshot().matchingOverride) while preserving
the nil checks and existing clone/merge behavior.
In `@internal/pricingoverrides/validation.go`:
- Around line 14-23: The loop validating p.Tiers must also reject tiers that set
both UpToTokens and UpToMtok to avoid silent precedence of UpToTokens; inside
the same validation logic that checks tier.UpToTokens/UpToMtok and the threshold
presence (the loop iterating over p.Tiers, e.g., in the Validate method where
you reference tier.UpToTokens and tier.UpToMtok), add a guard that if
tier.UpToTokens != nil && tier.UpToMtok != nil return
newValidationError("pricing.tiers only one of up_to_tokens/up_to_mtok may be
set", nil); apply the same check in the other validation block mentioned (the
one at the ~67-75 range) so both entry points reject dual-specified limits.
---
Outside diff comments:
In `@internal/admin/handler_usage.go`:
- Around line 237-242: The nil-check currently validates h.registry but the
subsequent call uses h.pricingResolver, so replace or add the guard to check
h.pricingResolver (and keep the existing h.registry check if both are required)
before proceeding; specifically, ensure that before the call that references
h.pricingResolver you validate h.pricingResolver != nil and return
handleError(c, featureUnavailableError("model pricing metadata is unavailable"
or a clearer message for missing pricing resolver)) if it's nil so the code
cannot panic when h.pricingResolver is absent.
In `@internal/providers/registry_metadata.go`:
- Around line 321-329: The current code replaces merged.PricingSources outright
when override.Pricing is non-nil, which discards registry-origin entries for
fields the override doesn't touch; instead, initialize merged.PricingSources if
nil and copy/merge the entries from
override.Pricing.FieldSources(core.ModelPricingSourceConfigYAML) into the
existing merged.PricingSources map (overwriting only the keys present in the
override), so registry sources for untouched fields are preserved; update the
block around merged := modeldata.MergeMetadata(...) / if override.Pricing != nil
{ ... } to perform a map-merge rather than direct assignment.
🪄 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
Run ID: ec057796-5fff-41b6-b920-cd38533afb7f
📒 Files selected for processing (53)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/model-pricing-overrides.jsinternal/admin/dashboard/static/js/modules/model-pricing-overrides.test.cjsinternal/admin/dashboard/templates/dollar-icon.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-budgets.htmlinternal/admin/dashboard/templates/page-guardrails.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/dashboard/templates/page-workflows.htmlinternal/admin/errors.gointernal/admin/handler.gointernal/admin/handler_model_overrides.gointernal/admin/handler_model_pricing_overrides.gointernal/admin/handler_model_pricing_overrides_test.gointernal/admin/handler_usage.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/core/types.gointernal/gateway/inference_execute.gointernal/gateway/inference_orchestrator_test.gointernal/gateway/usage.gointernal/modeldata/merge.gointernal/modeldata/merger.gointernal/modeldata/merger_test.gointernal/modeloverrides/service.gointernal/modeloverrides/store.gointernal/modeloverrides/types.gointernal/modelselectors/selectors.gointernal/pricingoverrides/factory.gointernal/pricingoverrides/pricing.gointernal/pricingoverrides/resolver.gointernal/pricingoverrides/service.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/snapshot.gointernal/pricingoverrides/store.gointernal/pricingoverrides/store_mongodb.gointernal/pricingoverrides/store_postgresql.gointernal/pricingoverrides/store_sqlite.gointernal/pricingoverrides/store_sqlite_test.gointernal/pricingoverrides/types.gointernal/pricingoverrides/validation.gointernal/providers/registry_metadata.gointernal/providers/registry_metadata_override_test.gointernal/responsecache/usage_hit.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.go
Comment on lines
+230
to
+248
| var pricingOverrideResult *pricingoverrides.Result | ||
| sharedPricingOverrideStorage := firstSharedStorage(auditResult.Storage, usageResult.Storage, batchResult.Storage, aliasResult.Storage, modelOverrideResult.Storage) | ||
| if sharedPricingOverrideStorage != nil { | ||
| pricingOverrideResult, err = pricingoverrides.NewWithSharedStorage(ctx, appCfg, sharedPricingOverrideStorage, providerResult.Registry, providerResult.Registry) | ||
| } else { | ||
| pricingOverrideResult, err = pricingoverrides.New(ctx, appCfg, providerResult.Registry, providerResult.Registry) | ||
| } | ||
| if err != nil { | ||
| closeErr := errors.Join(app.modelOverrides.Close(), app.aliases.Close(), app.batch.Close(), app.budgets.Close(), app.usage.Close(), app.audit.Close(), app.providers.Close()) | ||
| if closeErr != nil { | ||
| return nil, fmt.Errorf("failed to initialize model pricing overrides: %w (also: close error: %v)", err, closeErr) | ||
| } | ||
| return nil, fmt.Errorf("failed to initialize model pricing overrides: %w", err) | ||
| } | ||
| app.pricingOverrides = pricingOverrideResult | ||
| pricingResolver := usage.PricingResolver(providerResult.Registry) | ||
| if app.pricingOverrides != nil && app.pricingOverrides.Service != nil { | ||
| pricingResolver = app.pricingOverrides.Service | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the pricingoverrides package constructors to confirm the two trailing
# arguments are intentionally the same registry, and check for any feature flag.
fd -t f 'pricingoverrides' -e go | head -50
echo "---- New / NewWithSharedStorage signatures ----"
rg -nP -C2 '\bfunc\s+(New|NewWithSharedStorage)\s*\(' --type=go -g 'internal/pricingoverrides/**'
echo "---- config flags referencing pricing overrides ----"
rg -nP -C2 'PricingOverride' --type=go -g 'config/**'Repository: ENTERPILOT/GoModel
Length of output: 1152
🏁 Script executed:
# Check what types Catalog and usage.PricingResolver are, and what providerResult.Registry is
rg -nP 'type\s+(Catalog|PricingResolver)' --type=go
echo "---- providerResult.Registry type context ----"
rg -nP 'providerResult\.Registry' --type=go -B2 -A2 | head -60Repository: ENTERPILOT/GoModel
Length of output: 3694
🏁 Script executed:
# Search for feature flags or config related to pricing overrides with broader patterns
rg -nP 'PricingOverride|pricing.*[Ee]nable|pricing.*[Cc]onfig' --type=go internal/config/ | head -40Repository: ENTERPILOT/GoModel
Length of output: 119
🏁 Script executed:
# Check the shutdown/close comments around the lines mentioned in the scratchpad
sed -n '700,770p' internal/app/app.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 2649
🏁 Script executed:
# Check the Catalog interface definition from modelselectors
rg -nP 'type Catalog interface' --type=go -A10 internal/modelselectors/Repository: ENTERPILOT/GoModel
Length of output: 701
🏁 Script executed:
# Find Registry type definition and see what it implements
rg -nP 'type.*Registry|func.*NewRegistry|providerResult.*Registry\s*=' --type=go -B2 -A5 | head -100Repository: ENTERPILOT/GoModel
Length of output: 3978
🏁 Script executed:
# Check if Registry has methods that satisfy Catalog interface
rg -nP 'func.*\(.*\*Registry\)|func.*\(.*Registry\)' --type=go | grep -i 'catalog\|model\|select' | head -20Repository: ENTERPILOT/GoModel
Length of output: 2217
🏁 Script executed:
# Check if ModelRegistry has ProviderNames method
rg -nP 'ProviderNames' --type=goRepository: ENTERPILOT/GoModel
Length of output: 2654
🏁 Script executed:
# Check the PricingResolver interface definition
rg -nP 'type PricingResolver interface' --type=go -A10Repository: ENTERPILOT/GoModel
Length of output: 245
🏁 Script executed:
# Look for any config structures related to pricing to confirm there's no feature flag
fd -t f -e go -path '*/config/*' | head -20Repository: ENTERPILOT/GoModel
Length of output: 231
🏁 Script executed:
# Check if ModelRegistry implements ResolvePricing method
rg -nP 'ResolvePricing|func.*\(.*\*ModelRegistry\).*ResolvePricing' --type=goRepository: ENTERPILOT/GoModel
Length of output: 4943
🏁 Script executed:
# Check the pricingoverrides initialization again - verify if there's any conditional logic
sed -n '228,250p' internal/app/app.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 1528
🏁 Script executed:
# Check if there are any admin handlers or feature checks that gate pricing overrides access
rg -nP 'pricingOverrides|pricing.*[Oo]verride' --type=go internal/admin/ -B2 -A2Repository: ENTERPILOT/GoModel
Length of output: 19436
Pricing overrides initialization is unconditional — confirm this matches your design intent.
Unlike modelOverrideResult, which is gated by appCfg.Models.OverridesEnabled (line 210) and stubbed via &modeloverrides.Result{} otherwise, pricingoverrides.New[WithSharedStorage] runs unconditionally. Since admin handlers check if h.pricingOverrides == nil to return featureUnavailableError, and the initialization never produces a nil result, this nil check will never trigger. If pricing overrides should be a toggleable feature, add a config flag to conditionally initialize it like model overrides do.
Additionally, fix duplicate step numbering in shutdown sequence: lines 742–750 both label their comments as "10." Should be 10 and 11 respectively.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/pricingoverrides/resolver.go (1)
14-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
model/providerNamebefore invokingbase.ResolvePricing.
s.base.ResolvePricing(model, providerName)at line 16 receives the raw selector form. Whenmodelarrives as"openai/gpt-4o", the base resolver (which keys by canonical model ID) will miss, and a partial DB override below ends up merged onto a nil base — silently dropping base pricing fields. Move the trim/modelIDFromSelectornormalization above the base lookup so both paths see consistent identifiers.🛠 Proposed fix
func (s *Service) ResolvePricing(model, providerName string) *core.ModelPricing { if s == nil { return nil } + providerName = strings.TrimSpace(providerName) + model = modelIDFromSelector(model, providerName) + var basePricing *core.ModelPricing if s.base != nil { basePricing = s.base.ResolvePricing(model, providerName) } - - providerName = strings.TrimSpace(providerName) - model = modelIDFromSelector(model, providerName) if rule, ok := s.snapshot().matchingOverride(providerName, model); ok { return mergePricing(basePricing, rule.override.Pricing) } return cloneBasePricing(basePricing) }🤖 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 `@internal/pricingoverrides/resolver.go` around lines 14 - 23, The base resolve is called with raw selector values causing mismatches; normalize providerName and model first by trimming providerName and calling modelIDFromSelector(model, providerName) before invoking s.base.ResolvePricing so s.base.ResolvePricing(model, providerName) and the later matchingOverride lookup use the same canonical identifiers; update the code flow in resolver.go to perform providerName = strings.TrimSpace(providerName) and model = modelIDFromSelector(model, providerName) prior to calling s.base.ResolvePricing, keeping the subsequent snapshot().matchingOverride and mergePricing logic unchanged.
🤖 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 `@internal/gateway/inference_execute.go`:
- Around line 352-361: The function usagePricingModel should defensively trim
requestedModel before using it; modify usagePricingModel to call
strings.TrimSpace on requestedModel at the top (similar to
failoverModel/responseModel) and use the trimmed value when calling
ResolvedModelFromWorkflow and returning—this ensures callers that don't go
through requestModel won't pass untrimmed values into ResolvedModelFromWorkflow.
In `@internal/modelselectors/selectors.go`:
- Around line 119-132: The cached boolean globalSelector (computed via
IsGlobal(selector) at the top) should be reused instead of calling
IsGlobal(selector) again later; replace the second IsGlobal(selector) check with
the existing globalSelector variable so the checks are consistent and avoid
redundant trimming/compare—update the conditional that currently reads "if
providerName == "" && model == "" && !IsGlobal(selector) { return Selector{},
NewValidationError(...)" to use globalSelector, keeping the surrounding logic
that may call String(providerName, model) and ParseStoredParts unchanged and
preserving the same error return (Selector{}, NewValidationError).
In `@internal/pricingoverrides/service.go`:
- Around line 196-230: StartBackgroundRefresh can be given an interval shorter
than the per-tick refresh timeout (30s) which can cause ticker ticks to pile up
and drift; update StartBackgroundRefresh to enforce or clamp the interval to a
sensible minimum (e.g. at least the refresh timeout) instead of silently
accepting smaller values: introduce a named constant for the per-refresh timeout
(e.g. refreshTimeout := 30*time.Second), ensure the passed interval is set to
max(interval, refreshTimeout) (or refreshTimeout + small buffer) before creating
the ticker, and update the function comment to document the required minimum
interval and that the per-refresh timeout is refreshTimeout; reference
StartBackgroundRefresh, the refresh timeout constant, and s.Refresh in your
change.
---
Duplicate comments:
In `@internal/pricingoverrides/resolver.go`:
- Around line 14-23: The base resolve is called with raw selector values causing
mismatches; normalize providerName and model first by trimming providerName and
calling modelIDFromSelector(model, providerName) before invoking
s.base.ResolvePricing so s.base.ResolvePricing(model, providerName) and the
later matchingOverride lookup use the same canonical identifiers; update the
code flow in resolver.go to perform providerName =
strings.TrimSpace(providerName) and model = modelIDFromSelector(model,
providerName) prior to calling s.base.ResolvePricing, keeping the subsequent
snapshot().matchingOverride and mergePricing logic unchanged.
🪄 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
Run ID: 599bc572-6385-4c44-b5c3-d8d76ee2c316
📒 Files selected for processing (9)
internal/admin/handler.gointernal/admin/handler_pricing_test.gointernal/gateway/inference_execute.gointernal/modeloverrides/types.gointernal/modelselectors/selectors.gointernal/pricingoverrides/resolver.gointernal/pricingoverrides/service.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/validation.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/modeloverrides/service.go (1)
136-145: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd selector-match regression tests for the shared exact-key path.
Both snapshot population and lookup now depend on
modelselectors.ExactMatchKey(). Please add a table-driven test that exercises exact, model-wide, provider-wide, and global matches, plus a raw slash-containing model ID whose prefix is not a configured provider. That will catch any silent drift between normalization and key generation before exact overrides become unresolvable.As per coding guidelines, "Add or update tests for behavior changes."
Also applies to: 423-425
🤖 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 `@internal/modeloverrides/service.go` around lines 136 - 145, Add a table-driven unit test that verifies selector matching for all scopes using the same ExactMatchKey path: construct test cases for exact (provider+model), model-wide, provider-wide, global (matching modelselectors.ScopeProviderModel, ScopeProvider, ScopeGlobal, default model-wide), and a raw model ID containing a slash whose prefix is not a configured provider; for each case assert that the snapshot population uses modelselectors.ExactMatchKey(normalized.ProviderName, normalized.Model) to populate next.exact and that lookup resolves the same key (check next.exact, next.modelWide, next.providerWide, next.global) so any drift between normalization and ExactMatchKey is detected.
♻️ Duplicate comments (1)
internal/providers/registry_metadata_override_test.go (1)
74-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
output_per_mtokprovenance in this init-path test as well.This fixture overrides both
InputPerMtokandOutputPerMtok, but the test only verifies the input source. Add the paralleloutput_per_mtok == core.ModelPricingSourceConfigYAMLcheck so a one-sided provenance regression inInitializefails here too.Proposed addition
if got := overridden.Model.Metadata.PricingSources["input_per_mtok"]; got != core.ModelPricingSourceConfigYAML { t.Errorf("PricingSources[input_per_mtok] = %q, want %q", got, core.ModelPricingSourceConfigYAML) } +if got := overridden.Model.Metadata.PricingSources["output_per_mtok"]; got != core.ModelPricingSourceConfigYAML { + t.Errorf("PricingSources[output_per_mtok] = %q, want %q", got, core.ModelPricingSourceConfigYAML) +}As per coding guidelines, "Add or update tests for behavior changes."
🤖 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 `@internal/providers/registry_metadata_override_test.go` around lines 74 - 76, The test only asserts overridden.Model.Metadata.PricingSources["input_per_mtok"] equals core.ModelPricingSourceConfigYAML but misses the parallel check for "output_per_mtok"; update the test in registry_metadata_override_test.go to also assert that overridden.Model.Metadata.PricingSources["output_per_mtok"] == core.ModelPricingSourceConfigYAML so that the Initialize-path override for OutputPerMtok is verified (use the same t.Errorf pattern as the existing input_per_mtok assertion and reference the same overridden variable and core.ModelPricingSourceConfigYAML constant).
🤖 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 `@docs/openapi.json`:
- Around line 818-868: Update the GET operation description for the
"/admin/api/v1/model-overrides" endpoint to explicitly document the four
selector formats (global as "/", provider as "provider/", model as "model", and
exact as "provider/model") mirroring the phrasing used in GET
/admin/api/v1/model-pricing-overrides; edit the "description" value under the
GET for "/admin/api/v1/model-overrides" so it lists those selector examples and
matches the parameter wording used by the PUT counterparts to ensure
consistency.
- Around line 5355-5365: The admin.upsertModelOverrideRequest schema's
user_paths array is unbounded; add a sensible maxItems (e.g., 100) to the
user_paths property and optionally add a maxLength (e.g., 1024) to the string
items to match bounding used elsewhere; update the
"admin.upsertModelOverrideRequest" schema's "user_paths" definition to include
"maxItems" and add "maxLength" under the items object so clients and linters
have an explicit cap.
- Around line 6918-6923: The duplicate x-enum-varnames for
modeloverrides.ScopeKind and pricingoverrides.ScopeKind can collide in some
generators; either (A) change the x-enum-varnames in each schema to unique,
schema-prefixed identifiers (e.g., ModelScopeGlobal/ModelScopeModel/... for
modeloverrides.ScopeKind and PricingScopeGlobal/... for
pricingoverrides.ScopeKind) by updating the x-enum-varnames arrays, or (B)
refactor both schema definitions to reference a single shared ScopeKind
component (remove duplicate local definitions and point modeloverrides.ScopeKind
and pricingoverrides.ScopeKind to a shared components/schemas/ScopeKind) so only
one x-enum-varnames set exists.
- Around line 897-906: Update the OpenAPI response for PUT
/admin/api/v1/model-overrides/{selector} to return modeloverrides.View (not
modeloverrides.Override) so it includes scope_kind (matching PUT
/admin/api/v1/model-pricing-overrides which returns pricingoverrides.View); then
update the corresponding handler that serves PUT
/admin/api/v1/model-overrides/{selector} to construct and return a
modeloverrides.View object (with scope_kind computed/populated) instead of
returning the Override shape. Ensure the Swagger annotation/schema in
docs/openapi.json is changed from "$ref":
"#/components/schemas/modeloverrides.Override" to "$ref":
"#/components/schemas/modeloverrides.View" and the handler that handles that
route returns the View type.
In `@internal/admin/handler_model_pricing_overrides_test.go`:
- Around line 64-129: TestModelPricingOverrideLifecycle only exercises a
selector without embedded slashes; add a second lifecycle sub-case that uses a
selector whose model ID contains slashes (e.g. selector
"openrouter/meta-llama/llama-3.1-8b-instruct") to exercise URL-decoding and
round-tripping. For the new case, create HTTP requests with the selector
URL-encoded in the path (e.g. openrouter%2Fmeta-llama%2Fllama-3.1-8b-instruct)
but set the Echo path value for "selector" to the raw slash-containing string,
then call UpsertModelPricingOverride, ListModelPricingOverrides and
DeleteModelPricingOverride and assert the returned View fields (Selector,
ProviderName, Model, ScopeKind, Pricing.InputPerMtok) match the expected parsed
parts to ensure the handler correctly preserves and parses slash-containing
model IDs.
In `@internal/pricingoverrides/snapshot.go`:
- Around line 39-60: The snapshot build loop currently appends every
normalized.Selector into next.order and blindly writes into next.bySelector and
the scope maps, so two stored overrides that normalize to the same selector will
silently overwrite one another; after calling normalizeStoredOverride (in the
loop that builds next.order/next.bySelector/compiledOverride) check if
normalized.Selector already exists in next.bySelector and reject by returning an
error (include both the normalized selector and the original override.Selector
in the message) rather than overwriting or appending a duplicate; this prevents
writing into the scope maps (next.global, next.exact, next.providerWide,
next.modelWide) for a colliding selector and avoids duplicate entries in
next.order and snapshotOverrides output.
---
Outside diff comments:
In `@internal/modeloverrides/service.go`:
- Around line 136-145: Add a table-driven unit test that verifies selector
matching for all scopes using the same ExactMatchKey path: construct test cases
for exact (provider+model), model-wide, provider-wide, global (matching
modelselectors.ScopeProviderModel, ScopeProvider, ScopeGlobal, default
model-wide), and a raw model ID containing a slash whose prefix is not a
configured provider; for each case assert that the snapshot population uses
modelselectors.ExactMatchKey(normalized.ProviderName, normalized.Model) to
populate next.exact and that lookup resolves the same key (check next.exact,
next.modelWide, next.providerWide, next.global) so any drift between
normalization and ExactMatchKey is detected.
---
Duplicate comments:
In `@internal/providers/registry_metadata_override_test.go`:
- Around line 74-76: The test only asserts
overridden.Model.Metadata.PricingSources["input_per_mtok"] equals
core.ModelPricingSourceConfigYAML but misses the parallel check for
"output_per_mtok"; update the test in registry_metadata_override_test.go to also
assert that overridden.Model.Metadata.PricingSources["output_per_mtok"] ==
core.ModelPricingSourceConfigYAML so that the Initialize-path override for
OutputPerMtok is verified (use the same t.Errorf pattern as the existing
input_per_mtok assertion and reference the same overridden variable and
core.ModelPricingSourceConfigYAML constant).
🪄 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
Run ID: 31f83de6-f2c3-46c5-b4e1-feb545c076e2
📒 Files selected for processing (18)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/templates/page-models.htmlinternal/admin/handler_model_pricing_overrides.gointernal/admin/handler_model_pricing_overrides_test.gointernal/admin/handler_usage.gointernal/core/types.gointernal/modeldata/merger_test.gointernal/modeloverrides/service.gointernal/modeloverrides/types.gointernal/pricingoverrides/resolver.gointernal/pricingoverrides/snapshot.gointernal/pricingoverrides/types.gointernal/providers/registry_metadata.gointernal/providers/registry_metadata_override_test.gointernal/providers/registry_pricing_merge.gotools/openapi-postprocess.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/gateway/inference_execute.go (1)
100-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
ExecuteEmbeddingsbypassesusagePricingModel, missing the requested-model pricing resolution.Chat and responses now use
usagePricingModel(workflow, requestedModel, ...)so that admin pricing overrides keyed on the canonical requested model are found even when the provider echoes a versioned model in the response.ExecuteEmbeddingsstill passesresp.Modeldirectly tologUsage, so any pricing override on the canonical embedding model name would be silently skipped if the response model differs.Since embeddings have no failover, the
failoverModelbranch ofusagePricingModelwould never fire — the only risk is the requested-vs-response model mismatch. A minimal fix aligns all three paths:⚡ Proposed fix
func (o *InferenceOrchestrator) ExecuteEmbeddings(ctx context.Context, workflow *core.Workflow, req *core.EmbeddingRequest, requestID, endpoint string) (*EmbeddingResult, error) { if err := o.validateProviderAndRequest(req != nil, "embeddings request is required"); err != nil { return nil, err } resp, providerType, providerName, err := o.executeEmbeddings(ctx, workflow, req) if err != nil { return nil, err } - o.logUsage(ctx, workflow, resp.Model, providerType, providerName, func(pricing *core.ModelPricing) *usage.UsageEntry { + pricingModel := usagePricingModel(workflow, req.Model, "", resp.Model) + o.logUsage(ctx, workflow, pricingModel, providerType, providerName, func(pricing *core.ModelPricing) *usage.UsageEntry { return usage.ExtractFromEmbeddingResponse(resp, requestID, providerType, endpoint, pricing) })🤖 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 `@internal/gateway/inference_execute.go` around lines 100 - 120, ExecuteEmbeddings currently passes resp.Model into logUsage which skips admin pricing overrides keyed on the canonical requested model; instead call usagePricingModel with the requested model and the response model to resolve the correct pricing model (e.g., pricingModel := usagePricingModel(workflow, req.Model, resp.Model)) and pass that resolved pricingModel into the logUsage callback used by usage.ExtractFromEmbeddingResponse; keep EmbeddingResult.Model as resp.Model but ensure logUsage uses the resolved pricing model.
🤖 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 `@docs/openapi.json`:
- Around line 6371-6372: The tier schemas currently declare up_to_tokens with
"type": "number", allowing fractional token counts; change the schema property
up_to_tokens to use "type": "integer" (and optionally add "minimum": 0) in both
schema definitions referenced (the up_to_tokens property occurrences in the tier
schemas), so token counts are validated as whole non-negative integers.
- Around line 6956-7014: The pricingoverrides.Pricing schema currently allows an
empty object which permits no-op upserts; update the schema
(pricingoverrides.Pricing) to disallow empty payloads by adding a JSON Schema
constraint such as "minProperties": 1 (or explicitly list required fields if
specific properties must always be present) so that
admin.upsertModelPricingOverrideRequest cannot pass an empty pricing object.
In `@internal/admin/handler_model_overrides.go`:
- Around line 51-54: The Swagger docs are missing the 502 failure annotation for
write operations; update the handler annotations for UpsertModelOverride and
DeleteModelOverride to include "@Failure 502 {object} core.GatewayError" so the
OpenAPI contract reflects provider/store write errors (also add the same
annotation near the other similar block around the second annotation group
referenced). Locate the comment blocks above the UpsertModelOverride and
DeleteModelOverride handlers in internal/admin/handler_model_overrides.go and
insert the 502 failure line in each relevant `@Failure` list.
In `@internal/admin/handler_model_pricing_overrides_test.go`:
- Around line 116-120: The test bypasses URL-decoding by injecting the
already-decoded selector via putCtx.SetPathValues; change the setup so the
handler actually decodes tt.encodedPath: remove the SetPathValues call and
either (A) register the route on the test Echo instance and create the request
with the encoded path (use e.Router()/e.ServeHTTP so Echo extracts and decodes
the "selector" param), or (B) if you keep using the context directly, pass the
encoded path into the request URL and use Echo's route matcher to populate
params instead of injecting tt.selector; apply the same change to the other
block at lines 153-156.
In `@internal/modelselectors/selectors.go`:
- Around line 197-213: The branch in ParseStoredParts that checks
splitFirst(selector) with rest == "" is redundant; simplify by removing that
early-return and rely on the existing splitFirst(selector) branch that returns
(providerName, model, ok) — when model is empty it already yields (prefix,""),
so delete the first if-block (the one inspecting rest == "") and keep the final
splitFirst branch to return providerName, model when ok is true, otherwise fall
back to returning "", selector.
In `@internal/pricingoverrides/service_test.go`:
- Around line 340-357: Test currently asserts on exact error substrings from
service.buildSnapshot which is brittle; change the test
(TestServiceBuildSnapshotRejectsDuplicateNormalizedSelectors) to assert the
error category/type instead of message text: use errors.Is or errors.As to check
for a sentinel or typed duplicate-selector error returned by buildSnapshot (or
assert IsValidationError(err) as appropriate), and if you need to verify
selector details, cast to the typed error and assert its Normalized/Original
fields rather than relying on strings.Contains.
In `@internal/pricingoverrides/service.go`:
- Around line 243-245: The function rollbackContext currently calls
context.WithTimeout(context.Background(), 30*time.Second) which intentionally
ignores the caller's context and potential cancellation; add a one-line
clarifying comment immediately above rollbackContext (referencing
rollbackContext, context.WithTimeout, context.Background) stating that using
context.Background is deliberate so cleanup/rollback can continue for up to 30s
even if the request context is cancelled and to warn future readers not to
replace it with a caller ctx.
- Around line 243-245: The rollbackContext function currently hard-codes
30*time.Second; change it to reuse the existing refreshTimeout constant (or if
you prefer, introduce a dedicated rollbackTimeout constant) to avoid
drift—update rollbackContext to call context.WithTimeout(context.Background(),
refreshTimeout) (or use the new rollbackTimeout) so the timeout is consistent
with the rest of the codebase.
- Around line 184-194: The refresh error path in refreshLocked handling returns
immediately when !existed, which can silently leave a persisted delete without
rollback because previous is zero-valued; add a short comment above the if
!existed branch explaining this is intentional (we have no prior snapshot to
restore) and that we choose not to attempt rollback when previous is unknown,
and then enhance the delete/refresh flow to treat a store-side ErrNotFound from
the Delete call as an explicit signal to also skip rollback (i.e., if
store.Delete returned ErrNotFound, log/annotate and return the refresh error
without attempting Upsert), keeping the existing rollback logic that uses
s.store.Upsert(rollbackCtx, previous) and
s.reconcileSnapshotAfterRollbackFailureLocked unchanged.
In `@internal/pricingoverrides/snapshot.go`:
- Around line 53-63: The switch on normalized.ScopeKind() in buildSnapshot
currently uses default to populate next.modelWide, which hides unknown/new
ScopeKind values; add an explicit case for modelselectors.ScopeModel that sets
next.modelWide[normalized.Model] = compiled, and change the default branch to
treat unknown scopes as an error by returning a descriptive error from
buildSnapshot (use the same return path as other errors). Reference the switch
handling in buildSnapshot and the variables next, normalized, and compiled when
making this change.
---
Outside diff comments:
In `@internal/gateway/inference_execute.go`:
- Around line 100-120: ExecuteEmbeddings currently passes resp.Model into
logUsage which skips admin pricing overrides keyed on the canonical requested
model; instead call usagePricingModel with the requested model and the response
model to resolve the correct pricing model (e.g., pricingModel :=
usagePricingModel(workflow, req.Model, resp.Model)) and pass that resolved
pricingModel into the logUsage callback used by
usage.ExtractFromEmbeddingResponse; keep EmbeddingResult.Model as resp.Model but
ensure logUsage uses the resolved pricing model.
🪄 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
Run ID: 8e74e597-d232-4bfe-bfac-c09126db99c0
📒 Files selected for processing (13)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/handler_model_overrides.gointernal/admin/handler_model_overrides_test.gointernal/admin/handler_model_pricing_overrides_test.gointernal/gateway/inference_execute.gointernal/modeloverrides/service_test.gointernal/modelselectors/selectors.gointernal/pricingoverrides/service.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/snapshot.gointernal/providers/registry_metadata_override_test.gotools/openapi-postprocess.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/modelselectors/selectors.go`:
- Around line 63-220: Add table-driven unit tests for selector
parsing/normalization and scope classification to cover NormalizeInput,
NormalizeInputWithProviderNames, NormalizeStored, ParseStoredParts,
ScopeKindFor, String, IsGlobal, ExactMatchKey and splitFirst: create a new
_test.go file with multiple test cases (including empty input, global "/",
provider-only "prov/", model-only "model", provider/model "prov/model",
slash-shaped model "org/model" with no provider in catalog, unknown provider,
and stored rows missing provider/model), assert expected Selector fields,
normalized Selector string, ExactMatchKey, and ScopeKind for each case, and
include provider catalog variants via ProviderNames/ProviderNames(catalog) to
exercise provider detection and fallback parsing. Ensure tests are table-driven,
cover error cases (validation errors), and include ParseStoredParts behavior for
legacy stored selectors.
In `@tools/openapi-postprocess.mjs`:
- Around line 162-170: The loop sets up_to_tokens.minimum = 0 which contradicts
tier validation that forbids zero; update the adjustment for
schema("pricingoverrides.Pricing") and the loop over
["core.ModelPricingTier","pricingoverrides.PricingTier"] so that
up_to_tokens.type remains "integer" but up_to_tokens.minimum = 1 (not 0) to
match the tier validation and prevent generating invalid payloads for the
up_to_tokens property.
🪄 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
Run ID: 1b6d98b8-35bb-4222-af4f-e399e82cbbc9
📒 Files selected for processing (11)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/handler_model_overrides.gointernal/admin/handler_model_pricing_overrides_test.gointernal/gateway/inference_execute.gointernal/modelselectors/selectors.gointernal/pricingoverrides/service.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/snapshot.gointernal/server/handlers_test.gotools/openapi-postprocess.mjs
👮 Files not reviewed due to content moderation or server errors (2)
- internal/pricingoverrides/service.go
- docs/openapi.json
Comment on lines
+63
to
+220
| // NormalizeInput validates and normalizes one user-supplied selector. | ||
| func NormalizeInput(catalog Catalog, raw string) (Selector, error) { | ||
| return NormalizeInputWithProviderNames(ProviderNames(catalog), raw) | ||
| } | ||
|
|
||
| // NormalizeInputWithProviderNames validates and normalizes one user-supplied selector. | ||
| func NormalizeInputWithProviderNames(providerNames []string, raw string) (Selector, error) { | ||
| raw = strings.TrimSpace(raw) | ||
| if raw == "" { | ||
| return Selector{}, NewValidationError("selector is required", nil) | ||
| } | ||
| if IsGlobal(raw) { | ||
| return Selector{Selector: "/"}, nil | ||
| } | ||
|
|
||
| providerNameSet := make(map[string]struct{}, len(providerNames)) | ||
| for _, name := range providerNames { | ||
| name = strings.TrimSpace(name) | ||
| if name == "" { | ||
| continue | ||
| } | ||
| providerNameSet[name] = struct{}{} | ||
| } | ||
|
|
||
| var providerName, model string | ||
| if prefix, rest, ok := splitFirst(raw); ok { | ||
| if _, exists := providerNameSet[prefix]; exists { | ||
| providerName = prefix | ||
| model = rest | ||
| } else { | ||
| model = raw | ||
| } | ||
| } else { | ||
| model = raw | ||
| } | ||
|
|
||
| if providerName == "" && model == "" { | ||
| return Selector{}, NewValidationError("selector is required", nil) | ||
| } | ||
| if providerName != "" { | ||
| if _, exists := providerNameSet[providerName]; !exists { | ||
| return Selector{}, NewValidationError("unknown provider_name: "+providerName, nil) | ||
| } | ||
| } | ||
| return Selector{ | ||
| Selector: String(providerName, model), | ||
| ProviderName: providerName, | ||
| Model: model, | ||
| }, nil | ||
| } | ||
|
|
||
| // NormalizeStored normalizes selector columns loaded from storage. | ||
| func NormalizeStored(selector, providerName, model string) (Selector, error) { | ||
| selector = strings.TrimSpace(selector) | ||
| providerName = strings.TrimSpace(providerName) | ||
| model = strings.TrimSpace(model) | ||
| globalSelector := IsGlobal(selector) | ||
|
|
||
| if selector == "" { | ||
| selector = String(providerName, model) | ||
| } | ||
| if selector == "" { | ||
| return Selector{}, NewValidationError("selector is required", nil) | ||
| } | ||
| if providerName == "" && model == "" && !globalSelector { | ||
| providerName, model = ParseStoredParts(selector) | ||
| } | ||
| if providerName == "" && model == "" && !globalSelector { | ||
| return Selector{}, NewValidationError("selector is required", nil) | ||
| } | ||
| if normalized := String(providerName, model); normalized != "" { | ||
| selector = normalized | ||
| } | ||
| return Selector{ | ||
| Selector: selector, | ||
| ProviderName: providerName, | ||
| Model: model, | ||
| }, nil | ||
| } | ||
|
|
||
| // ProviderNames returns a copy of configured provider names from catalog. | ||
| func ProviderNames(catalog Catalog) []string { | ||
| if catalog == nil { | ||
| return nil | ||
| } | ||
| return append([]string(nil), catalog.ProviderNames()...) | ||
| } | ||
|
|
||
| // ScopeKindFor reports the normalized selector scope. | ||
| func ScopeKindFor(selector, providerName, model string) ScopeKind { | ||
| switch { | ||
| case IsGlobal(selector): | ||
| return ScopeGlobal | ||
| case strings.TrimSpace(providerName) != "" && strings.TrimSpace(model) != "": | ||
| return ScopeProviderModel | ||
| case strings.TrimSpace(providerName) != "": | ||
| return ScopeProvider | ||
| default: | ||
| return ScopeModel | ||
| } | ||
| } | ||
|
|
||
| // String returns the canonical selector string for normalized parts. | ||
| func String(providerName, model string) string { | ||
| providerName = strings.TrimSpace(providerName) | ||
| model = strings.TrimSpace(model) | ||
| switch { | ||
| case providerName != "" && model != "": | ||
| return providerName + "/" + model | ||
| case providerName != "": | ||
| return providerName + "/" | ||
| case model != "": | ||
| return model | ||
| default: | ||
| return "" | ||
| } | ||
| } | ||
|
|
||
| // IsGlobal reports whether raw selects every provider and model. | ||
| func IsGlobal(raw string) bool { | ||
| return strings.TrimSpace(raw) == "/" | ||
| } | ||
|
|
||
| // ExactMatchKey returns the lookup key used to index exact provider+model overrides. | ||
| // Returns "" when either side is missing — exact matching only applies to fully-qualified pairs. | ||
| func ExactMatchKey(providerName, model string) string { | ||
| providerName = strings.TrimSpace(providerName) | ||
| model = strings.TrimSpace(model) | ||
| if providerName == "" || model == "" { | ||
| return "" | ||
| } | ||
| return providerName + "/" + model | ||
| } | ||
|
|
||
| // ParseStoredParts splits a stored selector without a configured-provider catalog. | ||
| // This is a best-effort fallback for old rows. Stores should persist | ||
| // provider_name and model columns because slash-shaped model IDs cannot be | ||
| // distinguished from provider/model selectors without a provider catalog. | ||
| func ParseStoredParts(selector string) (providerName, model string) { | ||
| selector = strings.TrimSpace(selector) | ||
| if IsGlobal(selector) { | ||
| return "", "" | ||
| } | ||
| if providerName, model, ok := splitFirst(selector); ok { | ||
| return providerName, model | ||
| } | ||
| return "", selector | ||
| } | ||
|
|
||
| func splitFirst(value string) (prefix, rest string, ok bool) { | ||
| prefix, rest, ok = strings.Cut(value, "/") | ||
| if !ok { | ||
| return "", "", false | ||
| } | ||
| prefix = strings.TrimSpace(prefix) | ||
| rest = strings.TrimSpace(rest) | ||
| return prefix, rest, true | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add direct table-driven tests for selector normalization and scope classification.
This file is the core of selector parsing/normalization and currently has 0% patch coverage, so regressions in global/provider/model/exact behavior (especially slash-shaped IDs) can slip through.
As per coding guidelines, **/*_test.go: “Add or update tests for behavior changes.”
🤖 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 `@internal/modelselectors/selectors.go` around lines 63 - 220, Add table-driven
unit tests for selector parsing/normalization and scope classification to cover
NormalizeInput, NormalizeInputWithProviderNames, NormalizeStored,
ParseStoredParts, ScopeKindFor, String, IsGlobal, ExactMatchKey and splitFirst:
create a new _test.go file with multiple test cases (including empty input,
global "/", provider-only "prov/", model-only "model", provider/model
"prov/model", slash-shaped model "org/model" with no provider in catalog,
unknown provider, and stored rows missing provider/model), assert expected
Selector fields, normalized Selector string, ExactMatchKey, and ScopeKind for
each case, and include provider catalog variants via
ProviderNames/ProviderNames(catalog) to exercise provider detection and fallback
parsing. Ensure tests are table-driven, cover error cases (validation errors),
and include ParseStoredParts behavior for legacy stored selectors.
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 `@tools/openapi-postprocess.mjs`:
- Around line 161-170: The OpenAPI schema modification in
applyPricingSchemaConstraints sets a minimum for up_to_tokens but not for
up_to_mtok, allowing zero thresholds the backend rejects; update
applyPricingSchemaConstraints to locate the up_to_mtok property on the same
schemas checked (e.g., in the loop over "core.ModelPricingTier" and
"pricingoverrides.PricingTier") and set up_to_mtok.type = "integer" and
up_to_mtok.minimum = 1 (mirror the changes applied to up_to_tokens) so both
fields are constrained to >=1.
🪄 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
Run ID: 9df8c7b6-b9bb-482e-9bf8-a6e4236bca86
📒 Files selected for processing (12)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/handler_model_overrides.gointernal/admin/handler_model_pricing_overrides_test.gointernal/gateway/inference_execute.gointernal/modelselectors/selectors.gointernal/modelselectors/selectors_test.gointernal/pricingoverrides/service.gointernal/pricingoverrides/service_test.gointernal/pricingoverrides/snapshot.gointernal/server/handlers_test.gotools/openapi-postprocess.mjs
👮 Files not reviewed due to content moderation or server errors (4)
- cmd/gomodel/docs/docs.go
- internal/pricingoverrides/snapshot.go
- internal/admin/handler_model_pricing_overrides_test.go
- internal/pricingoverrides/service.go
Comment on lines
+161
to
+170
| function applyPricingSchemaConstraints() { | ||
| schema("pricingoverrides.Pricing").minProperties = 1; | ||
| for (const name of ["core.ModelPricingTier", "pricingoverrides.PricingTier"]) { | ||
| const upToTokens = schema(name).properties?.up_to_tokens; | ||
| if (!upToTokens) { | ||
| throw new Error(`missing up_to_tokens property on schema: ${name}`); | ||
| } | ||
| upToTokens.type = "integer"; | ||
| upToTokens.minimum = 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Align up_to_mtok schema bounds with tier validation
applyPricingSchemaConstraints() enforces up_to_tokens.minimum = 1, but leaves up_to_mtok unconstrained. The service rejects zero thresholds, so current OpenAPI still permits payloads the backend will reject.
♻️ Proposed fix
function applyPricingSchemaConstraints() {
schema("pricingoverrides.Pricing").minProperties = 1;
for (const name of ["core.ModelPricingTier", "pricingoverrides.PricingTier"]) {
- const upToTokens = schema(name).properties?.up_to_tokens;
+ const tierProps = schema(name).properties || {};
+ const upToTokens = tierProps.up_to_tokens;
if (!upToTokens) {
throw new Error(`missing up_to_tokens property on schema: ${name}`);
}
upToTokens.type = "integer";
upToTokens.minimum = 1;
+ if (tierProps.up_to_mtok) {
+ tierProps.up_to_mtok.minimum = 1;
+ }
}
}🤖 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 `@tools/openapi-postprocess.mjs` around lines 161 - 170, The OpenAPI schema
modification in applyPricingSchemaConstraints sets a minimum for up_to_tokens
but not for up_to_mtok, allowing zero thresholds the backend rejects; update
applyPricingSchemaConstraints to locate the up_to_mtok property on the same
schemas checked (e.g., in the loop over "core.ModelPricingTier" and
"pricingoverrides.PricingTier") and set up_to_mtok.type = "integer" and
up_to_mtok.minimum = 1 (mirror the changes applied to up_to_tokens) so both
fields are constrained to >=1.
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
2 participants
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
Tests
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests