fix(ai): agent lane drops OpenRouter cache-write tokens and under-counts context#111435
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 3:34 PM ET / 19:34 UTC. Summary PR surface: Source +14, Tests +32. Total +46 across 4 files. Reproducibility: yes. at source level: current main drops the separate cache-write field in the agent transport, and the PR supplies a concrete real-SSE reproduction that makes the overflow decision differ before versus after the paired fix. Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the focused transport and overflow-accounting fix after the exact-head required merge gates complete, while leaving provider-billed cost-breakdown semantics to the distinct product discussion in #9016. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main drops the separate cache-write field in the agent transport, and the PR supplies a concrete real-SSE reproduction that makes the overflow decision differ before versus after the paired fix. Is this the best way to solve the issue? Yes. Aligning the agent transport with the existing OpenAI-completions sibling contract and correcting the dependent overflow fallback is narrower and safer than adding a separate usage or cost API. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e2bb04328f5c. Label changesLabel justifications:
Evidence reviewedPR surface: Source +14, Tests +32. Total +46 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (8 earlier review cycles)
|
|
Narrowed the scope to take the review's second option. The blocking P1 was right, and measuring it made the case stronger than the review did. That leaves two defects with independent runtime evidence and no exported type or protocol change: 16 files → 4, I re-ran the mock-free proof for the narrowed scope — real
The middle column answers "why is Local after merging current The breakdown-desync question is real, but it is an owner-level call about what @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…che writes `applyProviderReportedUsageCost` overwrote only `cost.total`, leaving the per-component estimates in place, so `input + output + cacheRead + cacheWrite` stopped summing to the total and the replaced estimate was unrecoverable. The repo rebuilds that sum in four other places; this helper was the only writer breaking it. Retain the replaced estimate in a new optional `cost.estimatedTotal` instead of inverting the override: free OpenRouter models report `cost: 0`, and `shouldPreserveRecordedZeroCost` depends on a provider-billed zero staying authoritative, so holding the estimate in `total` would write phantom spend. `parseTransportChunkUsage` also hard-coded `cacheWrite: 0` and never read `prompt_tokens_details.cache_write_tokens`, while its plugin-sdk sibling reads it and documents the contract. Writes were dropped from `cacheWrite` and wrongly counted inside `input`. OpenRouter routes through this lane. Fixing that shifts tokens out of `input`, which `resolveContextInputTokens` used as a proxy for prompt size, so context-overflow and length-stop detection are aligned with the Anthropic lane's `input + cacheRead + cacheWrite`. Related: openclaw#9016
refactor(ai): drop the retained estimatedTotal cost surface ClawSweeper blocked the branch three review cycles running on the same P1: the new `Usage.cost.estimatedTotal` field crossed both the plugin SDK and the closed worker transcript schema, making it an upgrade contract that a bug-fix PR cannot decide on its own. Measurement settles it: the field was written in one place and forwarded through four transport layers, but nothing ever read it. It also did not repair the invariant it was introduced for -- `input + output + cacheRead + cacheWrite` still disagrees with a provider-billed `total`; the field only parked the discarded estimate beside it. Removing it drops eight files, retires the compatibility merge risk, and leaves the two fixes that carry real runtime evidence: OpenRouter `cache_write_tokens` mapping and the context-overflow accounting that depends on it. @
e135185 to
b1176ac
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(ai): agent lane drops OpenRouter cache-write tokens and under-counts context This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
|
Merged via squash.
|
…nts context (openclaw#111435) * fix(ai): provider-billed totals desync the cost breakdown and drop cache writes `applyProviderReportedUsageCost` overwrote only `cost.total`, leaving the per-component estimates in place, so `input + output + cacheRead + cacheWrite` stopped summing to the total and the replaced estimate was unrecoverable. The repo rebuilds that sum in four other places; this helper was the only writer breaking it. Retain the replaced estimate in a new optional `cost.estimatedTotal` instead of inverting the override: free OpenRouter models report `cost: 0`, and `shouldPreserveRecordedZeroCost` depends on a provider-billed zero staying authoritative, so holding the estimate in `total` would write phantom spend. `parseTransportChunkUsage` also hard-coded `cacheWrite: 0` and never read `prompt_tokens_details.cache_write_tokens`, while its plugin-sdk sibling reads it and documents the contract. Writes were dropped from `cacheWrite` and wrongly counted inside `input`. OpenRouter routes through this lane. Fixing that shifts tokens out of `input`, which `resolveContextInputTokens` used as a proxy for prompt size, so context-overflow and length-stop detection are aligned with the Anthropic lane's `input + cacheRead + cacheWrite`. Related: openclaw#9016 * @ refactor(ai): drop the retained estimatedTotal cost surface ClawSweeper blocked the branch three review cycles running on the same P1: the new `Usage.cost.estimatedTotal` field crossed both the plugin SDK and the closed worker transcript schema, making it an upgrade contract that a bug-fix PR cannot decide on its own. Measurement settles it: the field was written in one place and forwarded through four transport layers, but nothing ever read it. It also did not repair the invariant it was introduced for -- `input + output + cacheRead + cacheWrite` still disagrees with a provider-billed `total`; the field only parked the discarded estimate beside it. Removing it drops eight files, retires the compatibility merge risk, and leaves the two fixes that carry real runtime evidence: OpenRouter `cache_write_tokens` mapping and the context-overflow accounting that depends on it. @ * refactor(agents): tighten OpenRouter usage contract --------- Co-authored-by: Peter Steinberger <[email protected]>
Related: #9016
AI-assisted (Claude Code). Every claim below is backed by a command I ran; base-vs-head comparisons were made by stashing the branch.
What Problem This Solves
Two defects in OpenAI-compatible usage accounting on the lane that serves live OpenRouter agent turns.
1. The agent lane drops OpenRouter cache-write tokens
parseTransportChunkUsagehard-codedcacheWrite: 0and never readprompt_tokens_details.cache_write_tokens. Its siblingparseChunkUsage(packages/ai/src/providers/openai-completions.ts:1354) reads it correctly, and that sibling's own comment states the contract:So the correct behavior was already specified in-repo; the agent lane never adopted it. Write tokens were dropped from
cacheWriteand wrongly counted insideinput.This is the lane OpenRouter uses.
extensions/openrouter/src/provider-catalog.ts:58declaresapi: "openai-completions";src/agents/provider-transport-stream.ts:89-90routes that tocreateOpenAICompletionsTransportStreamFn(), i.e.src/agents/openai-completions-transport.ts.2. Context-overflow detection under-counts the same tokens
resolveContextInputTokens(packages/ai/src/utils/overflow.ts:91) fell back toinput + cacheRead. That was accidentally exact only whileinputstill absorbed cache-write tokens. Fixing #1 without this would have shipped a regression, so it lands here.The completions lane never populates
contextUsage(zero occurrences in that file), so it always takes this fallback. The consumers are control decisions, not display: silent-overflow (overflow.ts:158) and the Xiaomi-MiMo length-stop check (overflow.ts:168), reached fromsrc/agents/sessions/agent-session-compaction.ts:279andagent-session-execution.ts:28. The length-stop test uses a 1% band, which is often narrower than a single cache-write block.The Anthropic lane already gets this right —
packages/ai/src/providers/anthropic-usage.ts:96computesinput + cacheRead + cacheWrite. It populatescontextUsageand so bypasses the fallback, which is why only the completions lane was exposed. The fix aligns the fallback with that sibling and is a strict improvement for every lane.Why the retained-estimate field was removed
The earlier revision added
estimatedTotalto hold the catalog estimate that a provider-billedtotalreplaced, becauseapplyProviderReportedUsageCostoverwrites onlytotaland leaves the four components disagreeing with it.Two findings retired that approach:
model-utils.ts) and forwarded through four transport layers — SDK type, worker protocol schema, transcript commit, replay normalization — with no consumer anywhere in the tree. It was a write-only surface, and a permanent one, since it crossed a published SDK and a persisted wire schema.input + output + cacheRead + cacheWritestill disagrees with a provider-billedtotalafter the change; the field only parked the discarded estimate beside the disagreement. The desync is real, but reconciling it means deciding whatcost.totalmeans when a provider bills a number the components cannot reproduce — an owner-level decision about the public usage contract, which is exactly the call ClawSweeper said a mechanical repair should not make.Both fixes that remain carry independent runtime evidence and touch no exported type, so they stand on their own. The desync is left for #9016 follow-up, where the field can arrive with a consumer and an owner decision instead of ahead of both.
User Impact
input.No config, schema migration, or operator action. No public API or protocol change. Free-model zero-cost handling is untouched.
Evidence
Mock-free runtime proof — real socket, real transport, no provider account
A real
node:httpserver on127.0.0.1serves an OpenRouter-shaped SSE stream whose final chunk carriesprompt_tokens_details.cache_write_tokens. The realcreateOpenAICompletionsTransportStreamFn()consumes it over a real socket, and the real publicisContextOverflowscores the resulting message. Novi.mock, no fake timers, no stubbed transport, no provider account.Usage
prompt_tokens: 100, completion_tokens: 50, cached_tokens: 20, cache_write_tokens: 10. The context window is set to95so the control decision is observable rather than an internal number: the correct prompt count is100, and a cache-write mapping without the overflow fix would count90.I ran three configurations by checking out each file individually:
origin/main)input80— inflated by the writes7070cacheRead202020cacheWrite0— 10 tokens dropped1010totalTokens150150150isContextOverflow(msg, 95)truefalsetrueThe middle column is why
overflow.tsbelongs in this PR rather than a follow-up. Landing the cache-write mapping alone flips a real overflow to undetected: base only reaches the correct count becauseinputwrongly absorbed the writes, so correcting the mapping without correcting the fallback silently shrinks every overflow and length-stop decision on this lane. The two changes are one fix.Focused tests
Full local run after merging current
main: both suites green,node scripts/run-tsgo.mjsclean.Negative controls
Each line reverted individually; exactly the named test reddens.
cacheWrite: cacheWriteTokens→0maps cache_write_tokens as a separate write count- cacheWriteTokensfrominput+ message.usage.cacheWriteinoverflow.tscounts cache-write tokens toward the context…Static checks
oxfmt --checkclean ·oxlintclean ·git diff --checkclean · max-lines ratchet OK · both extension-import-boundary checks return[]· repoautoreviewclean.The generated plugin-SDK API baseline is no longer touched by this branch — with
estimatedTotalgone,docs/.generated/plugin-sdk-api-baseline.sha256is byte-identical tomain. That also removed the merge conflict this PR had picked up.Scope
Two defects, four files,
+52/-6. The remaining #9016 work — carrying provider-reported cost andcost_details.upstream_inference_costinto session state, agent runtime metadata, andsession_status, plus the breakdown-reconciliation question above — is follow-up I'd rather bring with its consuming surface than land as write-only fields.