feat: price cached input tokens in standalone cost calculation - #356
Conversation
Standalone mode previously computed cost from prompt_tokens and completion_tokens only, ignoring the cache_read_tokens / cache_write_tokens that PR #196 already captures. This caused cached tokens to be mispriced: overcharged for OpenAI/Gemini (cached subset billed at full input rate) and undercharged for Anthropic (cache read/write billed at /bin/bash). Adds nullable cache_read_price_per_million and cache_write_price_per_million columns to ModelPricing (with Alembic migration), surfaces them in the PricingConfig schema, pricing init service, and /v1/pricing API, and populates them from genai-prices defaults. The cost calculation in log_usage now prices cache tokens following the provider inclusion convention: OpenAI/Gemini cache reads are discounted (re-priced at the cache rate), while Anthropic cache reads/writes are billed as additive charges. Fixes #198 Co-Authored-By: Laguna S 2.1 <noreply@poolside.ai>
GitHub Actions
Inactive
WalkthroughThis change adds nullable cache read/write pricing, provider-aware standalone cost calculation, API and model response fields, configuration support, dashboard pricing editors, cache usage metrics, tests, documentation, and rebuilt dashboard assets. ChangesCache pricing and usage accounting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
The cache-pricing heuristic detected the provider convention with `cache_write > 0`, but Anthropic reports a cache write only on the turn that creates the cache. Warm-cache read turns (the common multi-turn case) have cache_read > 0 and cache_write == 0, so they fell into the OpenAI discount branch and subtracted cache_read * input_rate from a prompt that (for Anthropic) never included it, producing negative cost that credits the user's budget. Carry the convention explicitly via a new GatewayUsage.cache_tokens_in_prompt flag (default True = OpenAI-shaped; the Anthropic messages path sets it False; streaming propagates it) instead of inferring it. Rewrite _compute_cost onto the single genai-prices model: the input total includes cache reads/writes, uncached input is billed at the input rate, cache buckets are priced at their own rate, and an unset cache rate falls back to the input rate rather than $0 or a double count. Add the missing warm-cache Anthropic regression test and correct the fallback-behavior tests and docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR updates Otari’s standalone-mode billing to correctly price cached input tokens by introducing cache-rate fields on pricing objects and using them in the request pipeline’s cost computation, aligning standalone behavior with the cached-token accounting introduced earlier.
Changes:
- Add optional cache token rates (
cache_read_price_per_million,cache_write_price_per_million) across config, DB model + migration, pricing init, and pricing API. - Extract and use a dedicated
_compute_cost()helper to price cache tokens according to provider conventions (subset-discount vs additive). - Add unit tests to pin cost calculation behavior and update OpenAPI/docs accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_shared_helpers_pure.py | Extends pricing response helper test coverage to include cache-rate fields. |
| tests/unit/test_compute_cost.py | Adds focused unit tests for _compute_cost() and PricingConfig cache-rate validation. |
| src/gateway/streaming.py | Preserves cache_tokens_in_prompt convention across merged streaming usage chunks. |
| src/gateway/services/pricing_service.py | Populates default cache rates from genai-prices dataset fields. |
| src/gateway/services/pricing_init_service.py | Loads cache-rate fields from config into ModelPricing rows during init. |
| src/gateway/models/entities.py | Adds nullable cache-rate columns to ModelPricing and includes them in serialization. |
| src/gateway/core/usage.py | Adds cache_tokens_in_prompt flag and accessor to disambiguate provider cache conventions. |
| src/gateway/core/config.py | Adds optional cache-rate fields to PricingConfig with validation. |
| src/gateway/api/routes/pricing.py | Extends pricing request/response models and persistence to include cache-rate fields. |
| src/gateway/api/routes/messages.py | Marks Anthropic usage as cache_tokens_in_prompt=False when extracting cache buckets. |
| src/gateway/api/routes/_pipeline.py | Introduces _compute_cost() and updates log_usage to use it. |
| docs/public/openapi.json | Regenerates OpenAPI schema to reflect new pricing fields. |
| docs/configuration.md | Documents cache token pricing configuration and behavior. |
| alembic/versions/c4d6e8f0a2b4_add_cache_pricing_columns.py | Adds migration for new cache-rate columns on model_pricing. |
| prompt_tokens = usage_data.prompt_tokens or 0 | ||
| completion_tokens = usage_data.completion_tokens or 0 | ||
| cache_read = cache_read_tokens_of(usage_data) | ||
| cache_write = cache_write_tokens_of(usage_data) | ||
|
|
||
| if cache_tokens_in_prompt_of(usage_data): | ||
| total_input_tokens = prompt_tokens | ||
| else: | ||
| total_input_tokens = prompt_tokens + cache_read + cache_write | ||
|
|
||
| read_rate = pricing.cache_read_price_per_million | ||
| write_rate = pricing.cache_write_price_per_million | ||
|
|
||
| # Only pull a cache bucket out of the uncached-input total when it has its | ||
| # own price; otherwise it stays billed at the input rate. | ||
| priced_cache_read = cache_read if read_rate is not None else 0 | ||
| priced_cache_write = cache_write if write_rate is not None else 0 | ||
| uncached_input_tokens = total_input_tokens - priced_cache_read - priced_cache_write |
|
|
||
| #### Cache token pricing | ||
|
|
||
| Providers that support prompt caching (OpenAI, Gemini, Anthropic) report cached-token counts that are captured in `cache_read_tokens` and `cache_write_tokens` on the usage log. By default these are not priced — the cost is computed from `prompt_tokens` and `completion_tokens` only. To bill cache tokens, add the optional `cache_read_price_per_million` and `cache_write_price_per_million` rates (USD per million tokens): |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Review by Claude Code (driven by @khaled).
Core money math is correct: _compute_cost normalizes both provider conventions via the explicit cache_tokens_in_prompt flag (cleaner than the cache_write_tokens > 0 heuristic the PR description claims, please fix the description to match). Migration chains cleanly onto the current head, single head, reversible. Unit coverage of _compute_cost is strong.
One finding worth acting on before merge (set_pricing null-wipe), one defensive minor, plus:
- Testing gap: no integration coverage of the migration up/down, ORM round-trip,
set_pricingpersistence, ordefault_model_pricingseeding end to end.test_pricing_config.pywas not run locally per the checklist. Suggest extending the existingtest_log_usage_finds_pricing_with_colon_formatpattern to price a cached request through the real DB. - Em dashes were added in prose/doc-comments (
docs/configuration.md, several docstrings intest_compute_cost.py); AGENTS.md forbids them as separators andmake lintwill not catch it.
Verdict: approve-with-nits.
| pricing.input_price_per_million = request.input_price_per_million | ||
| pricing.output_price_per_million = request.output_price_per_million | ||
| pricing.cache_read_price_per_million = request.cache_read_price_per_million | ||
| pricing.cache_write_price_per_million = request.cache_write_price_per_million |
There was a problem hiding this comment.
set_pricing overwrites both cache rates unconditionally, and both fields default to None on SetPricingRequest. Any client that updates a model's input/output price without re-sending cache fields writes NULL cache rates. Because an explicit DB row fully beats the (transient, unpersisted) genai-prices default, this silently disables cache pricing for that model: OpenAI/Gemini cached tokens revert to the full input rate (overcharge), Anthropic cache tokens go to $0 (undercharge). This is a live path, not theoretical: the dashboard's own ModelsPage.tsx setPricing calls send only input/output. Fix: use exclude_unset / a sentinel so omitted cache fields preserve the stored value, or make full-replace explicit and update the dashboard form to always send cache fields.
| # own price; otherwise it stays billed at the input rate. | ||
| priced_cache_read = cache_read if read_rate is not None else 0 | ||
| priced_cache_write = cache_write if write_rate is not None else 0 | ||
| uncached_input_tokens = total_input_tokens - priced_cache_read - priced_cache_write |
There was a problem hiding this comment.
No lower-bound clamp: on the cache_tokens_in_prompt=True (OpenAI/Gemini) branch, total_input_tokens == prompt_tokens, so if cache_read ever exceeds prompt_tokens this goes negative and subtracts from cost (undercharge). Today the invariant holds, but the streaming merge combines counts from different chunks with or, so a provider quirk could break it. This is money math, a max(0, ...) clamp is cheap defense.
Defensive guard on _compute_cost: clamp cache_read to the input total and cache_write to the remainder, so a malformed provider payload (e.g. a cached count larger than prompt_tokens on the OpenAI shape) cannot drive the uncached remainder negative and produce a negative cost that would credit the user's budget. No-op for well-formed payloads and for the Anthropic shape (whose total is built from these counts). Mirrors the clamp both LiteLLM and Bifrost apply at the same seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
Bring cache tokens to parity with input/output in the admin dashboard. Pricing: - /v1/models ModelPricingInfo now returns cache_read_price_per_million / cache_write_price_per_million (were only on /v1/pricing before), so default- priced models show cache rates too. - Dashboard types (ModelPricingInfo, PricingResponse, SetPricingRequest) carry the cache fields; the Models page pricing editors (quick form, detail panel, inline row) accept optional cache read/write rates, the detail Specs and two new table columns display them. Monitoring: - Usage analytics page gains Cache read / Cache write total tiles. - Overview home page gains a "Cache reads, last 30 days" tile linking to Usage. - (Per-request cache tokens were already shown in the Activity detail drawer.) Regenerated OpenAPI. Rebuilt the committed dashboard bundle. Adds Vitest coverage for the editors, table/detail display, and the new tiles, plus an integration test for the /v1/models cache-pricing round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Failure
Two follow-ups to the cache-pricing dashboard work: - Table width: the four separate price columns squashed the model name (e.g. "openai:gpt-5-search-api-..."). Combine input/output and cache read/write into two columns, each showing the pair as two clickable numbers that open the row's inline editor; drop the separate action column. Table goes from 8 columns to 5. - Cache defaults were blank: two of the five ModelPricingInfo build sites in the /v1/models endpoint (the discovered-model and single-model/alias paths) were missed, so a discovered model with a configured price reported no cache rates. Populate them so genai-prices cache defaults and stored cache rates both surface in the catalogue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Failure
The detail card previously always occupied a 360px column with a "Select a model…" placeholder, narrowing the table even when nothing was selected. Render it only when a model is selected (the table spans the full width otherwise) and add a close button so it can be dismissed back to the wide layout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Failure
The /v1/models pricing payload now carries cache_read_price_per_million / cache_write_price_per_million (null when unset). Two alias tests asserted the exact pricing dict and broke; add the two null cache keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/integration/test_models_endpoint.py (1)
92-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that both pricing setup requests succeed.
The test ignores both
POST /v1/pricingresponses, so setup failures can surface later as misleading missing-model or field errors. Capture each response and assert the expected success status before querying/v1/models.🤖 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 `@tests/integration/test_models_endpoint.py` around lines 92 - 124, Update test_list_models_exposes_cache_pricing to capture both POST /v1/pricing responses and assert each returns the expected successful status before calling /v1/models. Keep the existing model pricing assertions unchanged.web/src/pages/ModelsPage.tsx (1)
188-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame 4-clause price-validity check, copy-pasted three times.
PriceModelForm.canSubmit(here),PanelPriceEditor.canSave(line 375), andInlinePriceForm.canSave(line 758) all compute the identicalisValidPrice(input) && isValidPrice(output) && isValidOptionalPrice(cacheRead) && isValidOptionalPrice(cacheWrite). Nothing wrong today, but three copies means three places to remember if the rule ever changes.A tiny shared helper would keep them in sync for free:
♻️ Suggested helper
+function isValidPriceForm(input: string, output: string, cacheRead: string, cacheWrite: string): boolean { + return isValidPrice(input) && isValidPrice(output) && isValidOptionalPrice(cacheRead) && isValidOptionalPrice(cacheWrite); +} + function PriceModelForm({ onClose }: { onClose: () => void }) { ... - const canSubmit = - modelKey.trim() !== "" && - isValidPrice(input) && - isValidPrice(output) && - isValidOptionalPrice(cacheRead) && - isValidOptionalPrice(cacheWrite); + const canSubmit = modelKey.trim() !== "" && isValidPriceForm(input, output, cacheRead, cacheWrite);🤖 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 `@web/src/pages/ModelsPage.tsx` around lines 188 - 193, Extract the repeated input/output/cache price-validation expression into a shared helper near the model pricing form utilities, then update PriceModelForm.canSubmit, PanelPriceEditor.canSave, and InlinePriceForm.canSave to call it while preserving their existing modelKey/input checks.
🤖 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/configuration.md`:
- Line 272: Update the “Cache token pricing” heading in the configuration
documentation from level four to level three so it correctly nests beneath the
existing “Pricing” level-two heading.
- Line 274: Update the cache-pricing description in the provider prompt-caching
documentation to state that unset cache rates are not separately discounted and
fall back to input_price_per_million, including that Anthropic cache buckets are
added to input before fallback pricing. Replace the claim that cache tokens are
unpriced and that cost uses only prompt_tokens and completion_tokens, while
preserving the optional cache_read_price_per_million and
cache_write_price_per_million guidance.
In `@src/gateway/api/routes/_pipeline.py`:
- Around line 988-1022: Normalize prompt_tokens, completion_tokens, cache_read,
and cache_write to non-negative values before calculating totals and applying
cache clamps in the pricing flow. Update the assignments around usage_data and
cache_tokens_of so malformed negative counts become zero, while preserving the
existing cache-rate pricing behavior and cost calculation.
---
Nitpick comments:
In `@tests/integration/test_models_endpoint.py`:
- Around line 92-124: Update test_list_models_exposes_cache_pricing to capture
both POST /v1/pricing responses and assert each returns the expected successful
status before calling /v1/models. Keep the existing model pricing assertions
unchanged.
In `@web/src/pages/ModelsPage.tsx`:
- Around line 188-193: Extract the repeated input/output/cache price-validation
expression into a shared helper near the model pricing form utilities, then
update PriceModelForm.canSubmit, PanelPriceEditor.canSave, and
InlinePriceForm.canSave to call it while preserving their existing
modelKey/input checks.
🪄 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: CHILL
Plan: Pro
Run ID: 553af987-c133-4c5b-b666-7ebc21166e4e
⛔ Files ignored due to path filters (1)
docs/public/openapi.jsonis excluded by!docs/public/openapi.json
📒 Files selected for processing (29)
alembic/versions/c4d6e8f0a2b4_add_cache_pricing_columns.pydocs/configuration.mdsrc/gateway/api/routes/_pipeline.pysrc/gateway/api/routes/messages.pysrc/gateway/api/routes/models.pysrc/gateway/api/routes/pricing.pysrc/gateway/core/config.pysrc/gateway/core/usage.pysrc/gateway/models/entities.pysrc/gateway/services/pricing_init_service.pysrc/gateway/services/pricing_service.pysrc/gateway/static/dashboard/assets/index-BnmiwDLj.jssrc/gateway/static/dashboard/assets/index-GFwMr04s.csssrc/gateway/static/dashboard/assets/index-N48TjYIg.jssrc/gateway/static/dashboard/index.htmlsrc/gateway/streaming.pytests/integration/test_alias_api.pytests/integration/test_model_aliases.pytests/integration/test_models_endpoint.pytests/unit/test_compute_cost.pytests/unit/test_shared_helpers_pure.pyweb/src/api/types.tsweb/src/lib/pricing.test.tsweb/src/pages/ModelsPage.test.tsxweb/src/pages/ModelsPage.tsxweb/src/pages/OverviewPage.test.tsxweb/src/pages/OverviewPage.tsxweb/src/pages/UsagePage.test.tsxweb/src/pages/UsagePage.tsx
|
|
||
| A pricing entry whose provider is not listed in the `providers` section is skipped at startup with a warning, not treated as a fatal error: the provider may still be reachable through environment credentials (any-llm reads keys like `OPENAI_API_KEY`), so a pricing/provider mismatch should not abort the gateway. To have such an entry take effect, add the provider to the `providers` section. | ||
|
|
||
| #### Cache token pricing |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a level-three heading here.
#### Cache token pricing skips from ## Pricing; change it to ### to satisfy Markdown heading hierarchy.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 272-272: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 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 `@docs/configuration.md` at line 272, Update the “Cache token pricing” heading
in the configuration documentation from level four to level three so it
correctly nests beneath the existing “Pricing” level-two heading.
Source: Linters/SAST tools
| prompt_tokens = usage_data.prompt_tokens or 0 | ||
| completion_tokens = usage_data.completion_tokens or 0 | ||
| cache_read = cache_read_tokens_of(usage_data) | ||
| cache_write = cache_write_tokens_of(usage_data) | ||
|
|
||
| if cache_tokens_in_prompt_of(usage_data): | ||
| total_input_tokens = prompt_tokens | ||
| else: | ||
| total_input_tokens = prompt_tokens + cache_read + cache_write | ||
|
|
||
| # Defensive clamp: keep the cache buckets inside the input total so the | ||
| # uncached remainder stays >= 0 even if a provider/adapter reports an | ||
| # inconsistent payload. For the Anthropic shape the total is built from | ||
| # these counts, so this is a no-op there; it only bites a broken OpenAI-shape | ||
| # payload where cached_tokens exceeds prompt_tokens. | ||
| cache_read = min(cache_read, total_input_tokens) | ||
| cache_write = min(cache_write, total_input_tokens - cache_read) | ||
|
|
||
| read_rate = pricing.cache_read_price_per_million | ||
| write_rate = pricing.cache_write_price_per_million | ||
|
|
||
| # Only pull a cache bucket out of the uncached-input total when it has its | ||
| # own price; otherwise it stays billed at the input rate. | ||
| priced_cache_read = cache_read if read_rate is not None else 0 | ||
| priced_cache_write = cache_write if write_rate is not None else 0 | ||
| uncached_input_tokens = total_input_tokens - priced_cache_read - priced_cache_write | ||
|
|
||
| cost = (uncached_input_tokens / 1_000_000) * pricing.input_price_per_million | ||
| cost += (completion_tokens / 1_000_000) * pricing.output_price_per_million | ||
| if read_rate is not None and cache_read > 0: | ||
| cost += (cache_read / 1_000_000) * read_rate | ||
| if write_rate is not None and cache_write > 0: | ||
| cost += (cache_write / 1_000_000) * write_rate | ||
|
|
||
| return cost |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clamp malformed negative token counts too.
The upper-bound clamps retain negative prompt_tokens, completion_tokens, or cache counts. With a configured cache rate, a negative cache count can still produce a negative cost and credit budget. Normalize all token counts to >= 0 before pricing.
Proposed fix
- prompt_tokens = usage_data.prompt_tokens or 0
- completion_tokens = usage_data.completion_tokens or 0
- cache_read = cache_read_tokens_of(usage_data)
- cache_write = cache_write_tokens_of(usage_data)
+ prompt_tokens = max(0, usage_data.prompt_tokens or 0)
+ completion_tokens = max(0, usage_data.completion_tokens or 0)
+ cache_read = max(0, cache_read_tokens_of(usage_data))
+ cache_write = max(0, cache_write_tokens_of(usage_data))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prompt_tokens = usage_data.prompt_tokens or 0 | |
| completion_tokens = usage_data.completion_tokens or 0 | |
| cache_read = cache_read_tokens_of(usage_data) | |
| cache_write = cache_write_tokens_of(usage_data) | |
| if cache_tokens_in_prompt_of(usage_data): | |
| total_input_tokens = prompt_tokens | |
| else: | |
| total_input_tokens = prompt_tokens + cache_read + cache_write | |
| # Defensive clamp: keep the cache buckets inside the input total so the | |
| # uncached remainder stays >= 0 even if a provider/adapter reports an | |
| # inconsistent payload. For the Anthropic shape the total is built from | |
| # these counts, so this is a no-op there; it only bites a broken OpenAI-shape | |
| # payload where cached_tokens exceeds prompt_tokens. | |
| cache_read = min(cache_read, total_input_tokens) | |
| cache_write = min(cache_write, total_input_tokens - cache_read) | |
| read_rate = pricing.cache_read_price_per_million | |
| write_rate = pricing.cache_write_price_per_million | |
| # Only pull a cache bucket out of the uncached-input total when it has its | |
| # own price; otherwise it stays billed at the input rate. | |
| priced_cache_read = cache_read if read_rate is not None else 0 | |
| priced_cache_write = cache_write if write_rate is not None else 0 | |
| uncached_input_tokens = total_input_tokens - priced_cache_read - priced_cache_write | |
| cost = (uncached_input_tokens / 1_000_000) * pricing.input_price_per_million | |
| cost += (completion_tokens / 1_000_000) * pricing.output_price_per_million | |
| if read_rate is not None and cache_read > 0: | |
| cost += (cache_read / 1_000_000) * read_rate | |
| if write_rate is not None and cache_write > 0: | |
| cost += (cache_write / 1_000_000) * write_rate | |
| return cost | |
| prompt_tokens = max(0, usage_data.prompt_tokens or 0) | |
| completion_tokens = max(0, usage_data.completion_tokens or 0) | |
| cache_read = max(0, cache_read_tokens_of(usage_data)) | |
| cache_write = max(0, cache_write_tokens_of(usage_data)) |
🤖 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 `@src/gateway/api/routes/_pipeline.py` around lines 988 - 1022, Normalize
prompt_tokens, completion_tokens, cache_read, and cache_write to non-negative
values before calculating totals and applying cache clamps in the pricing flow.
Update the assignments around usage_data and cache_tokens_of so malformed
negative counts become zero, while preserving the existing cache-rate pricing
behavior and cost calculation.
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Follow-up by Claude Code (driven by @khaled). Re-reviewed after the fixes; approving.
Resolved from the earlier review:
- Negative-cost guard:
_compute_costnow clamps the cache buckets into the input total, so the uncached remainder can't go negative (and the cache charge is capped too). Correct fix. - The
set_pricingcache-null-wipe: the dashboard now sends cache fields at all threesetPricingsites, so the live regression path is closed.
Non-blocking notes below (see inline). The only one worth considering before merge is the backend set_pricing contract, which still full-replaces on a partial payload for non-UI clients.
PR hygiene: this branch bundles the entire #354 overview feature (the branches aren't rebased and main has neither yet). Worth rebasing once #354 lands so this PR's diff is just the pricing work; it keeps the squash-merge clean.
New dashboard cache-pricing UI (ModelsPage.tsx) is solid and consistent with the file's conventions: tokenized colors, aria-labels, stopPropagation, optional-price validation, and tests updated. LGTM.
| if pricing: | ||
| pricing.input_price_per_million = request.input_price_per_million | ||
| pricing.output_price_per_million = request.output_price_per_million | ||
| pricing.cache_read_price_per_million = request.cache_read_price_per_million |
There was a problem hiding this comment.
Residual from the first review, now lower severity since the dashboard sends cache fields: the API still assigns both cache rates unconditionally and they default to None on the request model, so a non-UI client (curl, the Postman collection) that updates input/output without re-sending cache fields silently nulls the stored cache rates. Since an explicit DB row beats the genai-prices default, that reverts cache tokens to the input rate. Optional: use exclude_unset/a sentinel to preserve omitted cache fields, or document set_pricing as an intentional full-replace. Non-blocking.
| <button | ||
| type="button" | ||
| aria-label={`Edit ${label} price for ${rowKey}`} | ||
| className="tabular-nums hover:text-[var(--otari-brand-dark)] hover:underline" |
There was a problem hiding this comment.
Minor: the price numbers reveal their editability only via hover:underline, so on touch and for keyboard-only users the fact that these are editable isn't discoverable (clicking still works). A persistent subtle affordance (e.g. always-underlined or a small edit glyph) would surface it without hover. Non-blocking.
| </div> | ||
| <button | ||
| type="button" | ||
| aria-label="Close model details" |
There was a problem hiding this comment.
Minor: this close control (px-1.5 py-0.5 text-lg) and the paired price-number buttons are small tap targets on mobile. otari's frontend standards don't specify a minimum size, so this is a usability nit rather than a violation, but a slightly larger hit area would help on touch. Non-blocking.
Address review feedback from Copilot and @khaledosman: - set_pricing: a client that updates input/output without re-sending cache fields no longer wipes the model's cache rates. Omitted cache fields inherit the model's most recent stored value (each POST without an explicit effective_at creates a new version, so the value must carry forward); an explicit null still clears the rate. - docs: the "cache tokens are not priced by default" sentence was inaccurate, they are billed at the input rate when no cache rate is set. Reword. - Scrub em dashes from the cache docs section and test_compute_cost docstrings (AGENTS.md forbids them in prose). - Add integration coverage for set_pricing cache persistence and the omitted-field preservation behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
Description
Standalone mode previously computed cost from
prompt_tokensandcompletion_tokensonly, ignoring thecache_read_tokens/cache_write_tokensthat PR #196 already captures on theUsageLogrow. This caused cached tokens to be mispriced:prompt_tokens) were billed at the full input rate (overcharge).cache_read_tokensandcache_write_tokenswere billed at $0 (undercharge), sinceprompt_tokensexcludes cache for Anthropic.This PR prices cache tokens using a single convention borrowed from the genai-prices dataset this project already uses: the input/prompt token count is the grand total that includes cache reads and writes, and each physical token is charged once. Providers report cache tokens two different ways, so usage carries an explicit
GatewayUsage.cache_tokens_in_promptflag and_compute_costnormalizes both shapes onto that one model:cache_tokens_in_prompt=True): cache reads are already a subset ofprompt_tokens, so the cached portion is discounted (re-priced atcache_read_price_per_millioninstead of the full input rate) rather than double counted.cache_tokens_in_prompt=False, native/v1/messagespath): cache reads/writes are separate frominput_tokens, so they are folded into the total and billed additively atcache_read_price_per_million/cache_write_price_per_million. This holds on every turn, including warm-cache reads that create no new cache (cache_write_tokensis 0).The flag is set at the usage-capture boundary (the Anthropic messages adapter sets it False; all OpenAI-shaped paths default True), so the cost function never has to guess the provider. When a cache rate is unset, those tokens stay in the input total and are billed at
input_price_per_million. Cache buckets are clamped to the input total so cost can never go negative.Changes
Backend
ModelPricingentity + migration: nullablecache_read_price_per_million/cache_write_price_per_millioncolumns.GatewayUsage.cache_tokens_in_prompt(core/usage.py): explicit per-usage flag for the cache-accounting convention, propagated through the streaming accumulator._compute_cost()(_pipeline.py): testable helper that normalizes both conventions, discounts or adds cache tokens per configured rates, falls back to the input rate when a rate is unset, and clamps so cost is never negative.PricingConfig(config.py), pricing init service, and default pricing (pricing_service.py, from the genai-pricescache_read_mtok/cache_write_mtokfields).pricing.py): cache fields onSetPricingRequest/PricingResponse;set_pricinginherits a model's stored cache rates when the client omits the fields, so a partial input/output update never silently wipes cache pricing (an explicit null still clears it).models.py):/v1/modelsnow surfaces cache rates for configured, alias, and default-priced models.Dashboard (
web/)Docs / specs
configuration.md; regenerated OpenAPI + Postman; rebuilt and committed the dashboard bundle.Test plan
uv run pytest tests/unit: passing (test_compute_cost.pycovers no-cache, OpenAI/Gemini discount, Anthropic additive, warm-cache-read regression, partial-rate fallback, clamp, and the convention flag).npm --prefix web test/typecheck: passing (editors, table/detail display, Usage/Overview tiles, hide/close panel).make lint,make typecheck: passing./v1/modelscache round-trip andset_pricingcache persistence / omitted-field preservation added totest_models_endpoint.pyandtest_pricing_config.py.PR Type
Relevant issues
Fixes #198. Follow-up for TTL-aware Anthropic cache-write pricing tracked in #357.
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).AI Usage
AI Model/Tool used: Laguna S 2.1 (Poolside AI) for the initial draft; Claude Code (via @njbrake) for the corrected cost model, dashboard work, and review follow-ups.
Any additional AI details you'd like to share: The cost calculation normalizes both provider cache conventions through an explicit
cache_tokens_in_promptflag set at usage capture, not a token-count heuristic. Decisions were made with maintainer clarification on provider cache accounting (genai-prices' inclusive-total model).