Repository navigation
Conversation
Output costs came out high on Anthropic reasoning requests. In costs.ts the completionIncludesReasoning allow-list covered Google and the OpenAI-style Responses providers but not anthropic/vertex-anthropic, so those fell through to completionTokens + reasoningTokens — adding reasoning that is already inside completionTokens. extract-token-usage reads Anthropic's output_tokens straight into completionTokens and builds totalTokens as prompt + output without re-adding reasoning; its spec pins this (51 in, 136 out incl. 31 reasoning = 187). The extract layer and the cost layer disagreed; this lines them up. Verified against real upstream (Vertex Anthropic, claude-sonnet-4-5): raw usage returned output_tokens 72 with thinking_tokens 65 for a one-token text answer, confirming output_tokens includes thinking. Before the fix a live request billed 179 output tokens for 93 completion tokens; after, output cost is exactly completionTokens * price. aws-bedrock bundles reasoning into outputTokens the same way and has no separate reasoning source, so it is already correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G6eFuuNRtA7bogrRtAHAYV
The reported total_tokens added reasoning on top of a completion count that, for several providers, already contained it. A live Vertex Anthropic request came back as prompt 64 + completion 135 + reasoning 78 = 277, when completion already included the reasoning and the total should have been 199. This contradicted extract-token-usage, whose spec pins the Anthropic total at 51 + 136 = 187 rather than 218. The extract layer was right and every downstream total-tokens site re-added reasoning behind its back. Extract the provider predicate the cost path already used into an exported completionIncludesReasoning(), add sumTotalTokens() alongside it, and route every total-tokens site through it: the five in chat.ts (cached streaming log, streaming fallback, streaming usage chunk, non-streaming prompt-delta recompute, non-streaming response), the cached-response log entry, the three parse-provider-response fallbacks, and the transform-response-to-openai fallback. This affects every provider whose completion count already includes reasoning — Anthropic, Vertex Anthropic, Google, OpenAI, Azure, RanoAI, Sakana, Meta, aws-mantle — not just Anthropic. Billing was already correct; this only fixes the totals reported back to callers. The chat-basic e2e asserted the old formula, so it encoded the bug; it now asserts the same invariant via sumTotalTokens. Verified against real upstream (Vertex Anthropic, claude-sonnet-4-5): non-streaming 66 + 101 (72 reasoning) now totals 167 instead of 239, and streaming 65 + 50 (43 reasoning) totals 115 instead of 158, with output cost still exactly completionTokens * price. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G6eFuuNRtA7bogrRtAHAYV
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 1 remains after this review. WalkthroughThe change centralizes provider-specific reasoning-token handling and applies it to cost calculation, provider response parsing, response transformation, cached usage, streaming usage, non-streaming usage, and end-to-end validation. ChangesProvider-aware token accounting
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change corrects token totals and billing for affected providers, with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ProviderResponse
participant ResponseParser
participant sumTotalTokens
participant OpenAIUsage
ProviderResponse->>ResponseParser: provide usage fields
ResponseParser->>sumTotalTokens: calculate fallback total
sumTotalTokens-->>ResponseParser: return provider-aware total
ResponseParser->>OpenAIUsage: construct normalized usage
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Probed all 25 providers with credentials against live upstream: a completion count that already contains reasoning is the near-universal convention, not the exception. Adds the 14 confirmed providers to completionIncludesReasoning. Canopywave is the one actively mis-billed: it reports reasoning_tokens at the top level of usage, which is the only shape extract-token-usage reads for OpenAI-format providers, so its reasoning reached the cost engine and was billed twice (92% over) and inflated streaming total_tokens. xAI stays off the list — grok-4 answers "9" with completion_tokens 1, reasoning_tokens 118 and total_tokens 324, so its completion genuinely excludes reasoning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 `@apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts`:
- Around line 624-626: Update the xAI test in transformResponseToOpenai to pass
a non-empty data array, ensuring the image-response rebuild branch executes
before asserting usage.total_tokens. Keep the existing expected token count and
test intent unchanged.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99bc4ef1-879f-4591-b466-e4a70614dccb
📒 Files selected for processing (8)
apps/gateway/src/chat-basic.e2e.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/parse-provider-response.spec.tsapps/gateway/src/chat/tools/parse-provider-response.tsapps/gateway/src/chat/tools/transform-response-to-openai.spec.tsapps/gateway/src/chat/tools/transform-response-to-openai.tsapps/gateway/src/lib/costs.spec.tsapps/gateway/src/lib/costs.ts
| test("adds reasoning for a provider that reports it separately", () => { | ||
| // xai rebuilds the response when the payload carries a `data` array. | ||
| expect(call("xai", { data: [] }).usage.total_tokens).toBe(192); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a non-empty data array for the xAI branch.
transformResponseToOpenai rebuilds xAI image responses only when json.data.length > 0. With data: [], call() returns the input object without usage, so this test throws before it checks total_tokens.
Proposed fix
- expect(call("xai", { data: [] }).usage.total_tokens).toBe(192);
+ expect(
+ call("xai", { data: [{ url: "https://example.test/image.png" }] }).usage
+ .total_tokens,
+ ).toBe(192);📝 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.
| test("adds reasoning for a provider that reports it separately", () => { | |
| // xai rebuilds the response when the payload carries a `data` array. | |
| expect(call("xai", { data: [] }).usage.total_tokens).toBe(192); | |
| test("adds reasoning for a provider that reports it separately", () => { | |
| // xai rebuilds the response when the payload carries a `data` array. | |
| expect( | |
| call("xai", { data: [{ url: "https://example.test/image.png" }] }).usage | |
| .total_tokens, | |
| ).toBe(192); |
🤖 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 `@apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts` around
lines 624 - 626, Update the xAI test in transformResponseToOpenai to pass a
non-empty data array, ensuring the image-response rebuild branch executes before
asserting usage.total_tokens. Keep the existing expected token count and test
intent unchanged.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
calculateDataStorageCost summed prompt + completion + reasoning unconditionally, but most providers (OpenAI, Azure, Google, aws-mantle, Sakana, Meta, RanoAI) already count reasoning inside the completion count, so retention storage billed those tokens twice whenever a reasoning count reached a call site. Thread the used provider into the storage calculation and skip the reasoning add-on when completionIncludesReasoning() says the completion count already contains it. Providers that report reasoning outside the completion count (e.g. xAI) keep the additive behavior. Stacked on #3486, which provides completionIncludesReasoning() and fixes the equivalent double-count in usage.total_tokens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Synced with main (merge commit, conflicts resolved):
Re-verified after the merge:
#3574 is now restacked on top of this branch and shrunk to the one thing this PR doesn't cover: provider-aware |
## Summary
`parseProviderResponse` (Google branch) caches Gemini thought signatures
in
Redis under the id it emits — `${name}_${shortid(24)}`, as
`thought_signature:<id>` — so the signature can be re-injected when the
client
replays the call next turn. The `n > 1` branch of
`transformResponseToOpenai`
(`apps/gateway/src/chat/tools/transform-response-to-openai.ts:454-456`)
regenerated tool calls from the raw parts with
`${name}_${candidateIndex}_${fcIndex}`
ids and dropped `extra_content`, so the id the client echoes back never
matches
the cached key. The next-turn lookup in `chat.ts`
(`redisClient.get("thought_signature:" + toolCall.id)`) misses, the
signature
is not re-injected, and Gemini rejects the replay with *"Corrupted
thought
signature"* — the exact failure the shortid fix (#1448) eliminated on
the
streaming path. The `n > 1` branch is the one path left behind: the
single-candidate path reuses parse's `toolResults` and is consistent.
This is the same "forgotten sibling branch" pattern as #3492 (our
previous
fix) and #3504 (the maintainer's own follow-up).
## Fix
Two changes in the multi-candidate branch:
- **Candidate 0** reuses the tool calls from `toolResults` (what
`parseProviderResponse` already emitted and cached), so the id the
client
receives is exactly the one the signature is cached under.
- **Candidates 1+** (which parse does not process) get the same
treatment
parse applies to candidate 0: a unique `${name}_${shortid(24)}` id, the
inline `extra_content.google.thought_signature`, and the
`setex("thought_signature:<id>", 86400, sig)` Redis entry under the
emitted
id.
Fallback keeps the branch safe: if `toolResults` is absent for candidate
0
(e.g. no tool calls parsed), the shortid path applies.
## Tests
`transform-response-to-openai.spec.ts`:
- The existing multi-candidate test no longer pins the broken
`${name}_${candidateIndex}_${fcIndex}` ids; it asserts the id scheme and
that candidates are distinct.
- New test: `keeps multi-candidate Google tool_call ids in the
thought_signature
cache` — mocks `@llmgateway/cache` (`setex`) and asserts:
- choice 0 reuses the parsed tool calls verbatim (id + inline
signature);
- choice 1 gets a unique id, `extra_content` with its own signature, and
a
`setex("thought_signature:<id>", 86400, sig)` call under the emitted id;
- no `setex` is written under choice 0's id from the transform (parse
already did that).
## Verification
Every command below was run and its output captured. The record is
reproducible — commands included so you can re-run them.
**red — probe against main (c1cd97b), real functions via esbuild,
before the
fix** (harness preserved at the hub):
```
parse toolResults[0].id : get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
+ extra_content.google.thought_signature
Redis writes made by parse : [{"key":"thought_signature:get_weather_xxxxxxxxxxxxxxxxxxxxxxxx", …}]
transform choice 0 tool_calls: id get_weather_0_0, extra_content undefined
RESULT: MISMATCH — next-turn GET thought_signature:get_weather_0_0 misses;
signature lost, Gemini 3 rejects replay
=== control (n=1) ===
CONTROL RESULT: MATCH — single-candidate path is consistent
```
**green — same probe after the fix**:
```
choice 0 tool_calls: id get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
extra_content.google.thought_signature sig-candidate-0
choice 1 tool_calls: id get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
extra_content.google.thought_signature sig-candidate-1
Redis writes: thought_signature:get_weather_xxx = sig-candidate-0
thought_signature:get_weather_xxx = sig-candidate-1
RESULT: MATCH (no bug)
CONTROL RESULT: MATCH — single-candidate path is consistent
```
(The deterministic shortid stub makes both candidate ids equal in the
probe;
the real `shortid` yields unique ids per candidate, asserted in the
spec.)
**green — vitest, full spec**:
```
$ pnpm exec vitest run apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts --no-file-parallelism
✓ apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts (14 tests) 24ms
Test Files 1 passed (1)
Tests 14 passed (14)
[exit code 0]
```
**Area suite** — `vitest run apps/gateway/src/chat/tools`: **593/603
tests,
37/38 files pass**. The only failing file is
`openai-content-filter.spec.ts` (10 tests), a service-harness spec whose
fetch/Redis mocks time out in this environment; it imports only
`openai-content-filter.ts`, which this change does not touch (same class
of
harness failures documented in previous PRs here).
<details><summary>Environment</summary>
```
so: Windows 11 (AMD64)
node --version: v24.14.1
pnpm --version: 9.15.9 (via npx; repo packageManager is pnpm@10.30.3)
vitest: 4.1.8
base: c1cd97b (origin/main @ 08-09)
fix commit: 7e76bf9 (2 files, +177/−13)
```
</details>
**Gates:** eslint ✅ on both files (after `import/order` fix) · prettier
✅ ·
`git diff --check` clean · gateway typecheck ✅ (builds in `build:core`,
turbo 12/12) · `pnpm-lock.yaml` untouched.
**Collisions:** no open PR touches the multi-candidate Google tool_call
ids /
thought_signature path (checked live 2026-08-11). #3486 (reasoning
tokens)
and #3233 (Claude on Azure) also touch `transform-response-to-openai.ts`
but
in unrelated areas (usage tokens / new provider branch); no semantic
overlap.
Not verified: a live multi-turn round-trip against Gemini (no provider
keys on
this machine). The id↔Redis-key chain is covered end-to-end by the
spec's
`setex` assertions and the probe's recorded Redis writes.
---
Disclosure: an AI coding assistant helped locate the drift and draft the
test
scaffolding. The reproduction, the red→green cycle and this write-up
were
verified by running the code; I own the change and will follow up on
review
comments.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved handling of Google responses containing multiple candidates
and tool calls.
* Preserved candidate-specific thought signatures for reliable follow-up
processing.
* Prevented tool-call identifiers and cached data from being overwritten
across candidates.
* Added graceful logging when caching metadata cannot be completed.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Codebuff <noreply@codebuff.com>
# Conflicts: # apps/gateway/src/chat/tools/parse-provider-response.spec.ts # apps/gateway/src/chat/tools/parse-provider-response.ts # apps/gateway/src/chat/tools/transform-response-to-openai.ts # apps/gateway/src/lib/costs.spec.ts # apps/gateway/src/lib/costs.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
# Conflicts: # apps/gateway/src/lib/costs.spec.ts # apps/gateway/src/lib/costs.ts
# Conflicts: # apps/gateway/src/lib/costs.ts
Supersedes #3435 by @serhiizghama, which diagnosed the cost half of this correctly. That PR had gone stale against
main(which has since addedranoaito the same allow-list), and verifying it end-to-end turned up two more double-counts. All three are fixed here.1. Anthropic reasoning tokens were billed twice
In
costs.ts, thecompletionIncludesReasoningallow-list covered Google and the OpenAI-style Responses providers but notanthropic/vertex-anthropic, so those fell through tocompletionTokens + reasoningTokens— adding reasoning that is already insidecompletionTokens.extract-token-usagereads Anthropic'soutput_tokensstraight intocompletionTokensand buildstotalTokensas prompt + output without re-adding reasoning; its spec pins this (51 in, 136 out incl. 31 reasoning = 187, not 218). The extract layer and the cost layer disagreed.aws-bedrockbundles reasoning intooutputTokensthe same way but exposes no reasoning count at all, so it was already correct — unchanged.2.
total_tokenswas inflated for every affected providerVerifying the above against real upstream surfaced a second, independent bug. A live Vertex Anthropic request came back as:
when
completionalready contained the reasoning and the total should have been 199. Ten separate sites added reasoning on top of the completion count, behind the extract layer's back. Billing was unaffected by this half; only the totals reported back to callers were wrong.The fix extracts the provider predicate the cost path already used into an exported
completionIncludesReasoning(), addssumTotalTokens()alongside it, and routes every total-tokens site through it:chat.ts— cached streaming log entry, streaming fallback, streaming usage chunk, non-streaming prompt-delta recompute, non-streaming response, cached-response log entryparse-provider-response.ts— the aws-bedrock, alibaba and default (OpenAI-format) fallbackstransform-response-to-openai.ts— thebuildUsageObjectfallback3. The allow-list itself was incomplete — Canopywave was overbilled ~92%
Review asked whether Alibaba belonged on the list. Rather than guess, every provider in the catalogue with credentials in
.envwas probed directly against live upstream (25 providers), asking a question whose visible answer is one token so thatcompletion_tokens ≈ reasoning_tokens + 1proves inclusivity.The result is that inclusive is the near-universal convention, not the exception — OpenAI documents
completion_tokensas containingcompletion_tokens_details.reasoning_tokens, and every provider that reports a reasoning count follows it. Confirmed inclusive, all withtotal_tokens = prompt_tokens + completion_tokens:xAI is the one genuine counter-example and stays off the list:
grok-4answering "9" reportscompletion_tokens: 1alongsidereasoning_tokens: 118andtotal_tokens: 324, i.e. its completion count really does exclude reasoning and its own total adds it back. The additive branch exists for xAI.Most of these are latent rather than live today:
extract-token-usagereads the raw upstream chunk, and for OpenAI-format providers it only reads a top-levelusage.reasoning_tokens, so a count nested incompletion_tokens_detailsnever reaches the cost engine. Canopywave is the exception — it reportsreasoning_tokensat the top level, so its reasoning did reach the cost engine and was billed twice.Live before/after through the gateway,
canopywave/kimi-k2.6, prices 0.5e-6 in / 2.8e-6 out:total_tokensVerification against real upstream
Anthropic (the original cost bug)
A direct call to Vertex, bypassing the gateway, settles the premise:
A one-token answer reported as 72 output tokens only makes sense if
output_tokensincludes the 65 thinking tokens.Cost, through the gateway (same prompt, cache flushed between runs):
output_costcompletion × 15e-6)Totals, through the gateway:
total_tokensbeforeCoverage of the ten changed sites
Every site was driven against live upstream except where noted, and the resulting rows were read back out of Postgres with the worker running, not just off the API response:
total_tokens200 → 98)google-ai-studio/gemini-3.6-flashwith an image — Google excludes image tokens from upstream usage, socosts.promptTokens(1101) exceeds the text-only count and the recompute firescached=true,cost=0, log rowtotal_tokens 1435 = 1101 + 334total_tokens 1371 = 1101 + 270parse-provider-responsefallbackstotal_tokenscannot be forced from a real provider)transform-response-to-openaifallbackfinalTotalTokensfallbackPersisted log rows were read back out of
logfor all of these. One clarification on the table above: itstotal_tokenscolumn is what the client receives, and for the streaming rows that is the SSE usage chunk, which is a different site from the log entry. Thebefore, streamingrow is the clearest case — the client got200(25 + 89 + 86) while the log row already stored114. Costs match everywhere. The Google cached pairs land ascached=true, cost=0withtotal_tokens 1435 = 1101 + 334(non-streaming) and1371 = 1101 + 270(streaming).Providers not exercised live
google-vertexauthenticates with an API key rather than a service account here;sakana403s from this network;aws-mantlerejects the bearer token,vertex-openaineeds an OAuth token andazure/azure-ai-foundryreturnDeploymentNotFound. Those six are covered by unit tests only — all were already on the list before this PR, so none is newly classified on unverified evidence.openaiis now confirmed live (a puzzle prompt givescompletion_tokens: 306/reasoning_tokens: 296/total_tokens: 353).Seven providers (atlascloud, avalanche, cloudrift, general-compute, nvidia, obsidian, openrouter) are BYOK-only — they have no base URL outside a user's provider key, so they cannot be probed from here at all.
Two providers report
reasoning_tokens: 0while clearly reasoning —xiaomiandnanogpt(gpt-oss-120b at high effort: 945 completion tokens,reasoning_tokens: 0). Their totals are consistent with inclusive semantics, but a zero count is not proof, so both were deliberately left off. Worth noting nanogpt reports its zero at the top level, the same shape that made canopywave overbill — it is inert only for as long as the value stays 0.Tests
sumTotalTokens/completionIncludesReasoningunit tests covering both sides of the predicate (all 27 inclusive providers, plus xai and aws-bedrock on the additive side) and null handling.anthropic,vertex-anthropic,canopywave(the live-overbilled one) andxai(asserting the additive branch is preserved). Confirmed the canopywave test fails without the fix:expected 0.0004508 to be close to 0.0002296.claude-3-5-sonnet-20241022, a mapping that isdeactivatedAtand has noreasoning: true— a model that cannot emit thinking tokens at all. Swapped toclaude-sonnet-4-5(identical 3.0e-6/15.0e-6 pricing, so the arithmetic carries over) and tightened totoBeCloseTo(…, 10), matching the neighbouring RanoAI test — the default 2-decimal tolerance does not catch this bug.parse-provider-response.spec.tsandtransform-response-to-openai.spec.tscovering thetotal_tokens-omitted branches on both sides of the predicate.chat-basice2e asserted the oldprompt + completion + reasoningformula, i.e. it encoded the bug. It now asserts the same invariant viasumTotalTokens.Full unit suite: 4312 passed, 5 pre-existing failures in
videos.spec.ts/logs.spec.ts/admin-devpass-payg.spec.tsthat reproduce identically with this branch's changes reverted (local GCS/Stripe env, unrelated).pnpm build: 17/17. Scoped e2e against real upstream,TEST_MODELS="canopywave/kimi-k2.6" FULL_MODE=true: 101 passed across 28 files (the one failure is a/v1/ocr401 onmistral-ocr-latest, an unrelated credential issue) — includingbasic,reasoning + streamingandreasoning + tool callsfor the newly-classified provider.🤖 Generated with Claude Code
Summary by CodeRabbit