refactor: replace resolvePricing positional model args with RoutingInfo and add backward-compat fallback for legacy ExtraFields - #4021
Conversation
|
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:
📝 WalkthroughWalkthroughPricing cost resolution is refactored to use ChangesRoutingInfo pricing refactoring
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 5/5Safe to merge. The refactor is internally contained to the modelcatalog pricing package, the backward-compat fallback is correctly gated on a fully-empty RoutingInfo, and the new test matrix covers the key edge cases. All three call sites of resolvePricing are updated, the backward-compat condition is precise (all three RoutingInfo fields must be zero/nil to trigger the fallback), the lookup precedence change is tested end-to-end, and the new tests confirm no regression for legacy callers replaying pre-RoutingInfo logs. No files require special attention. Important Files Changed
Reviews (13): Last reviewed commit: "feat: adds routing info wiring for prici..." | Re-trigger Greptile |
993c4ad to
afd8acd
Compare
7f83e5f to
7795c41
Compare
live model cache store and port keyconfig regression tests for alias/model isolation
#4034
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/pricing_overrides_test.go (1)
65-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd skip reasons for disabled pricing override tests
Seven tests in
framework/modelcatalog/pricing_overrides_test.goare unconditionally skipped viat.Skip()with no message (lines 66, 104, 205, 234, 268, 303, 381), and there are no nearby TODO/FIXME/issue references explaining why they’re disabled or when to re-enable them.✨ Suggested improvement
Add an explanatory message to each
t.Skip():func TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric(t *testing.T) { - t.Skip() + t.Skip("TODO: re-enable after request-type matching is fully wired (PR `#XXXX`)")🤖 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 `@framework/modelcatalog/pricing_overrides_test.go` around lines 65 - 66, Several tests in pricing_overrides_test.go are unconditionally skipped with bare t.Skip() which lacks context; update each t.Skip() call (e.g., in TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric and the other tests in framework/modelcatalog/pricing_overrides_test.go that call t.Skip()) to include a concise skip reason string (briefly stating why the test is disabled) and, if applicable, a TODO or issue reference and the condition required to re-enable the test; ensure the message is short and actionable so future maintainers can understand and track when to re-enable the test.
🤖 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 `@framework/modelcatalog/pricing.go`:
- Around line 275-280: Two consecutive comment blocks redundantly state that
cache-debug pricing has a single model identifier; remove the duplicate and
replace with a single concise comment describing the behavior (e.g.,
"Cache-debug pricing has a single model identifier recorded by the cache; it
maps to aliasKey and RoutingInfo.Model with no alias resolution context, so
modelName/modelID remain empty"). Edit the comment adjacent to the cache-debug
pricing logic in pricing.go (the block referencing aliasKey and
RoutingInfo.Model) to keep only this single, clear sentence and delete the
repeated lines.
---
Outside diff comments:
In `@framework/modelcatalog/pricing_overrides_test.go`:
- Around line 65-66: Several tests in pricing_overrides_test.go are
unconditionally skipped with bare t.Skip() which lacks context; update each
t.Skip() call (e.g., in TestGetPricing_RequestTypeSpecificOverrideBeatsGeneric
and the other tests in framework/modelcatalog/pricing_overrides_test.go that
call t.Skip()) to include a concise skip reason string (briefly stating why the
test is disabled) and, if applicable, a TODO or issue reference and the
condition required to re-enable the test; ensure the message is short and
actionable so future maintainers can understand and track when to re-enable the
test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67dded0a-2c9a-44a2-a9cf-996d09a37630
📒 Files selected for processing (3)
framework/modelcatalog/pricing.goframework/modelcatalog/pricing_overrides_test.goframework/modelcatalog/pricing_test.go
afd8acd to
86ffbe6
Compare
7795c41 to
53ed5e5
Compare
53ed5e5 to
6fe1303
Compare
53745d2 to
61f1e36
Compare
6fe1303 to
3d70a3f
Compare
61f1e36 to
1d1469c
Compare
139c90d to
decab23
Compare
5f44a42 to
ff3bf4c
Compare
0f1ec91 to
dd0fa43
Compare
69e5dd3 to
cf9c97e
Compare
48605b5 to
d355ea9
Compare
cf9c97e to
b90517b
Compare
d355ea9 to
bdb85c8
Compare
b90517b to
8011af6
Compare
Merge activity
|

Summary
Refactors the pricing resolution path to consume a
schemas.RoutingInfostruct directly instead of a loose(provider, originalModelRequested, resolvedModelUsed string)triplet. This aligns pricing lookups with the routing context thatcore.bifrostalready populates at request time, and introduces a structured lookup precedence:AliasModelName → AliasModelID → ModelName.Changes
resolvePricingnow accepts aschemas.RoutingInfoinstead of three separate string parameters. The lookup order triesResolvedKeyAlias.ModelName, thenResolvedKeyAlias.ModelID, thenRoutingInfo.Model, stopping at the first catalog hit. Override keys are always derived from the wire model (AliasModelIDwhen an alias matched, otherwiseModel).calculateBaseCostreadsRoutingInfofromExtraFieldsdirectly. A backward-compatibility fallback synthesises aRoutingInfofrom the deprecatedProvider/OriginalModelRequested/ResolvedModelUsedtriplet whenRoutingInfois fully unset — coveringLoggerPlugin.RecalculateCostsreplaying pre-RoutingInfologs and third-party plugins on the legacy shape. PartialRoutingInfopopulation is trusted as-is and does not trigger the fallback.computeCacheEmbeddingCostsimilarly constructs aRoutingInfofrom the cache-debug fields when callingresolvePricing.RoutingInfothat pins the model fields to the container identifier while preserving the provider, replacing the previous duallookupModel/lookupResolvedoverride.schemas.RoutingInfostructs. New backward-compat tests cover: legacy-fields-only (no alias), legacy-fields-only (with alias/deployment),RoutingInfowinning over legacy fields when both are set, both empty returning zero cost, and partialRoutingInfonot triggering the fallback.Type of change
Affected areas
How to test
go test ./framework/modelcatalog/...Key scenarios to verify:
RoutingInfo.Model.ResolvedKeyAlias.ModelIDwhen an alias is matched and the deployment ID differs from the caller-facing model name.AliasModelNametakes precedence overAliasModelIDwhen both are present.ExtraFieldstriplet (noRoutingInfo) still produces correct costs.RoutingInfoand the legacy triplet are populated,RoutingInfowins.RoutingInfo(e.g.Modelset butProviderempty) does not fall back to legacy fields.Breaking changes
resolvePricingis an internal method, but any code calling it directly with the old(provider, originalModelRequested, resolvedModelUsed string, ...)signature must be updated to pass aschemas.RoutingInfo. External callers populating only the deprecatedProvider/OriginalModelRequested/ResolvedModelUsedfields onBifrostResponseExtraFieldscontinue to work via the backward-compat fallback, but should migrate toRoutingInfo.Related issues
N/A
Security considerations
No auth, secrets, PII, or sandboxing implications. Pricing resolution is internal cost accounting only.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Refactor
Tests