Fix/fast pricing#153
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a billing service tier concept that decouples cost calculation from the actual or requested service tier. A new resolver function normalizes tiers for billing, pricing rules now treat ChangesBilling Service Tier Separation
Sequence Diagram(s)sequenceDiagram
participant Handler as Request Handler
participant Translator as resolveBillingServiceTier
participant UsageLog as UsageLogInput
participant DB as InsertUsageLog
participant BillingCalc as calculateCost
Handler->>Translator: resolveBillingServiceTier(actualTier, requestedTier)
Translator-->>Handler: billingServiceTier
Handler->>UsageLog: set BillingServiceTier field
Handler->>DB: InsertUsageLog(UsageLogInput{..., BillingServiceTier})
DB->>BillingCalc: calculateCost(..., billingServiceTier)
BillingCalc-->>DB: accountBilled
DB-->>Handler: usage logged with computed cost
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR separates user-visible service tier normalization from billing tier calculation so usage logs can bill based on upstream-reported tiers while still presenting a consistent UI tier.
Changes:
- Adds
resolveBillingServiceTierand unit tests to compute billing tier independently from UI tier. - Propagates
BillingServiceTierthrough request handlers intoUsageLogInput. - Updates billing logic so
fastuses priority pricing when available, and removes the old “priority multiplier” fallback.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proxy/translator.go | Introduces resolveBillingServiceTier to decouple billing tier resolution from UI tier normalization. |
| proxy/translator_test.go | Adds table-driven tests validating billing-tier resolution rules. |
| proxy/handler.go | Records both resolved UI tier and billing tier in usage logs. |
| database/postgres.go | Bills using BillingServiceTier when provided; extends UsageLogInput accordingly. |
| database/billing.go | Treats fast as eligible for priority pricing; removes priority multiplier behavior. |
| database/billing_test.go | Adds coverage for fast priority pricing and “no invented priority multiplier” behavior. |
| database/sqlite_test.go | Adds regression test asserting billed amount depends on BillingServiceTier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1809
to
+1823
| actualTier = strings.ToLower(strings.TrimSpace(actualTier)) | ||
| switch actualTier { | ||
| case "priority", "fast": | ||
| return "priority" | ||
| case "default", "auto", "flex", "scale": | ||
| return actualTier | ||
| } | ||
|
|
||
| requestedTier = strings.ToLower(strings.TrimSpace(requestedTier)) | ||
| switch requestedTier { | ||
| case "priority", "fast": | ||
| return "priority" | ||
| default: | ||
| return requestedTier | ||
| } |
Comment on lines
+1806
to
+1807
| // report a concrete tier, or when it confirms priority. If upstream downgrades | ||
| // to default, billing must follow the actual default tier. |
Comment on lines
+788
to
+793
| if logs[0].ServiceTier != "fast" || logs[0].AccountBilled != wantPriority { | ||
| t.Fatalf("priority-billed fast log = tier %q billed %.12f, want tier fast billed %.12f", logs[0].ServiceTier, logs[0].AccountBilled, wantPriority) | ||
| } | ||
| if logs[1].ServiceTier != "fast" || logs[1].AccountBilled != wantDefault { | ||
| t.Fatalf("default-billed fast log = tier %q billed %.12f, want tier fast billed %.12f", logs[1].ServiceTier, logs[1].AccountBilled, wantDefault) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
database/billing_test.go (1)
103-120: ⚡ Quick winAdd a
fastfallback test when priority prices are unavailable.You already cover
priorityfallback ongpt-4o; adding the same assertion forfastcloses the exact new branch added in billing logic.Proposed test-case addition
{ name: "does not invent priority multiplier when priority price is unknown", model: "gpt-4o", serviceTier: "priority", inputTokens: 1000, outputTokens: 500, cachedTokens: 200, want: 0.0075, }, + { + name: "fast tier falls back to standard pricing when priority price is unknown", + model: "gpt-4o", + serviceTier: "fast", + inputTokens: 1000, + outputTokens: 500, + cachedTokens: 200, + want: 0.0075, + },🤖 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 `@database/billing_test.go` around lines 103 - 120, Add a test case in billing_test.go to cover the new branch where the "fast" serviceTier falls back to priority prices when priority prices are unavailable: insert a table entry mirroring the existing "does not invent priority multiplier when priority price is unknown" case but with serviceTier set to "fast", model "gpt-4o", inputTokens 1000, outputTokens 500, cachedTokens 200 and want 0.0075; place it alongside the other cases so the test that drives the billing calculation (the same test table used by the CalculateCost / billing logic) verifies the fast-tier fallback behavior.proxy/translator_test.go (1)
60-81: ⚡ Quick winAdd a regression case for unknown concrete upstream tiers.
Please add a case where
actualis non-empty unknown (e.g.,"burst") andrequestedis"fast"; expected billing tier should be"burst".Suggested test case
tests := []struct { name string actual string requested string want string }{ {name: "actual priority wins", actual: "priority", requested: "fast", want: "priority"}, {name: "actual default downgrade wins", actual: "default", requested: "fast", want: "default"}, + {name: "unknown concrete actual tier wins", actual: "burst", requested: "fast", want: "burst"}, {name: "requested fast fallback bills priority", actual: "", requested: "fast", want: "priority"}, {name: "requested priority fallback bills priority", actual: "", requested: "priority", want: "priority"}, {name: "default stays default", actual: "default", requested: "", want: "default"}, }🤖 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 `@proxy/translator_test.go` around lines 60 - 81, Add a regression test in TestResolveBillingServiceTier to cover unknown concrete upstream tiers: include a test case with name like "unknown actual stays", actual: "burst", requested: "fast", want: "burst" so resolveBillingServiceTier("burst","fast") returns "burst"; update the tests slice inside TestResolveBillingServiceTier (the table-driven tests) to include this case referencing the existing TestResolveBillingServiceTier and resolveBillingServiceTier identifiers.
🤖 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 `@proxy/translator.go`:
- Around line 1808-1823: The function resolveBillingServiceTier currently only
treats a small allowlist of actualTier values as concrete; change it so any
non-empty upstream actualTier is honored as the concrete billing tier: trim and
lowercase actualTier, if the result is non-empty then map known synonyms ("fast"
-> "priority") but otherwise return the normalized actualTier; only when
actualTier is empty fall back to normalizing and mapping requestedTier. Update
the logic in resolveBillingServiceTier to perform this check using the existing
actualTier and requestedTier variables and existing mapping rules.
---
Nitpick comments:
In `@database/billing_test.go`:
- Around line 103-120: Add a test case in billing_test.go to cover the new
branch where the "fast" serviceTier falls back to priority prices when priority
prices are unavailable: insert a table entry mirroring the existing "does not
invent priority multiplier when priority price is unknown" case but with
serviceTier set to "fast", model "gpt-4o", inputTokens 1000, outputTokens 500,
cachedTokens 200 and want 0.0075; place it alongside the other cases so the test
that drives the billing calculation (the same test table used by the
CalculateCost / billing logic) verifies the fast-tier fallback behavior.
In `@proxy/translator_test.go`:
- Around line 60-81: Add a regression test in TestResolveBillingServiceTier to
cover unknown concrete upstream tiers: include a test case with name like
"unknown actual stays", actual: "burst", requested: "fast", want: "burst" so
resolveBillingServiceTier("burst","fast") returns "burst"; update the tests
slice inside TestResolveBillingServiceTier (the table-driven tests) to include
this case referencing the existing TestResolveBillingServiceTier and
resolveBillingServiceTier identifiers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f303c129-ddeb-4eb8-90cf-bce5457f54c1
📒 Files selected for processing (7)
database/billing.godatabase/billing_test.godatabase/postgres.godatabase/sqlite_test.goproxy/handler.goproxy/translator.goproxy/translator_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
修复fast模式下计价错误的问题
Summary by CodeRabbit
New Features
Tests