[codex] Fix channel test cache billing - #5340
Conversation
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughsettleTestQuota now separates prompt cached tokens and applies CacheRatio and CompletionRatio to compute quotaTokens, subtracts cached tokens for non-Claude semantics, then multiplies by ModelRatio and rounds once. Two tests validate cache-ratio application and Claude semantic cached-token treatment. ChangesCached Token Quota Calculation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 |
![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 (1)
controller/channel_test_internal_test.go (1)
44-59: ⚡ Quick winAdd a Claude-semantic regression case next to this one.
This only exercises the OpenAI-style
prompt-cache+cache*ratiopath. The production quota code has a separate Claude/message branch where cached tokens are not subtracted from the prompt base, so this test suite still won’t catch that mismatch insettleTestQuota. A second case withUsageSemantic: "anthropic"or ClaudeRelayInfowould pin that contract.Based on learnings, Claude/OpenRouter billing paths intentionally treat cached prompt tokens differently from the generic OpenAI-style path.
🤖 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 `@controller/channel_test_internal_test.go` around lines 44 - 59, Add a second test in controller/channel_test_internal_test.go alongside TestSettleTestQuotaAppliesCacheRatio that exercises the Claude/semantic billing branch by calling settleTestQuota with a dto.Usage configured for the Claude path (e.g., set UsageSemantic to "anthropic" or populate RelayInfo to indicate Claude) and appropriate dto.Usage.PromptTokensDetails.CachedTokens; assert the expected quota differs per the Claude rule (cached tokens not subtracted from prompt base). Reference settleTestQuota, TestSettleTestQuotaAppliesCacheRatio, dto.Usage, UsageSemantic, RelayInfo and types.PriceData when adding the new case so the Claude/message branch is covered.Source: Learnings
🤖 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 `@controller/channel-test.go`:
- Around line 549-553: The current quota calculation unconditionally subtracts
CachedTokens which is incorrect for Claude semantics; change the quotaTokens
computation to branch on the semantics: for non-Claude semantics compute
quotaTokens = promptTokens - cacheTokens + cacheTokens*priceData.CacheRatio +
completionTokens*priceData.CompletionRatio, but for Claude semantics do not
subtract cacheTokens (use quotaTokens = promptTokens +
cacheTokens*priceData.CacheRatio + completionTokens*priceData.CompletionRatio).
Locate the variables promptTokens, cacheTokens
(usage.PromptTokensDetails.CachedTokens), completionTokens, quotaTokens and
priceData in the function and implement the conditional using the existing
semantics indicator (e.g., usage.Semantic or an isClaudeSemantic() check) so
Claude/OpenRouter paths match the runtime quota logic.
---
Nitpick comments:
In `@controller/channel_test_internal_test.go`:
- Around line 44-59: Add a second test in
controller/channel_test_internal_test.go alongside
TestSettleTestQuotaAppliesCacheRatio that exercises the Claude/semantic billing
branch by calling settleTestQuota with a dto.Usage configured for the Claude
path (e.g., set UsageSemantic to "anthropic" or populate RelayInfo to indicate
Claude) and appropriate dto.Usage.PromptTokensDetails.CachedTokens; assert the
expected quota differs per the Claude rule (cached tokens not subtracted from
prompt base). Reference settleTestQuota, TestSettleTestQuotaAppliesCacheRatio,
dto.Usage, UsageSemantic, RelayInfo and types.PriceData when adding the new case
so the Claude/message branch is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 733900d0-1a97-4b84-bdc0-8c1af9d92430
📒 Files selected for processing (2)
controller/channel-test.gocontroller/channel_test_internal_test.go
a26c974 to
3b9a836
Compare
1 participant
Summary
settleTestQuota.Root cause
Channel/model tests write their own consume log through
settleTestQuota. The non-tiered fallback formula counted all prompt tokens at the full model ratio, while the log metadata already includedcache_tokensandcache_ratio. This made the log list quota disagree with the expanded billing details for cached requests.Validation
go test ./controller -run TestSettleTestQuotaAppliesCacheRatio -count=1initially failed with actual quota11295instead of expected2655.go test ./controller -run 'TestSettleTestQuota|TestBuildTestLogOther|TestResolveChannelTestUserID' -count=1\n\nNote:go test ./controller -count=1still fails locally in an unrelated existing test,TestListModelsTokenLimitIncludesTieredBillingModel, with the local SQLite test database state.\nSummary by CodeRabbit
Bug Fixes
Tests