fix(acp): price cache and thought tokens, and stop stacking derived cost

[onatozmenn]

HUMAN:

Before touching any of the math I wanted to be sure cache tokens weren't already inside the input count. Turns out ACP keeps them in separate buckets, so they were going through completely unpriced. The other two were more clear cut.

---

AGENT:

Why

Fixes #4382.

Three defects in ACP cost accounting, all reachable from _record_usage.

1. Cache and thought tokens are never priced. _extract_token_usage pulls five buckets out of the ACP Usage, but _estimate_cost_from_tokens(model, input_tokens, output_tokens) only ever sees two of them.

Worth being precise about the direction of this one, because it is the opposite of what it looks like. ACP reports the buckets separately: Usage.total_tokens is documented as "Sum of all token types across session", and metrics.py already says so out loud in cache_hit_rate ("litellm/OpenAI count cached reads inside prompt_tokens; ACP reports them separately"). So cached reads are not a discountable subset of the input total the way they are on the litellm path. They are additional tokens that were being billed at nothing, which means a cache-heavy gemini-cli session is under-billed, not discounted incorrectly.

2. The derived estimate can stack on top of provider cost. cost_recorded is only set when a UsageUpdate arrives with delta > 0. Provider cost is cumulative, so a second UsageUpdate carrying the same amount produces delta == 0, cost_recorded stays False, and the token-derived estimate is added on top of cost that was already recorded. accumulated_cost then exceeds the provider's own cumulative figure.

3. An unpriced model records $0 in silence. cost_map.get(model, {}) yields empty, both rates are 0, the function returns 0.0, and the caller's if cost > 0 drops it. Nothing is logged, so a whole session reads as free.

Summary

• _estimate_cost_from_tokens takes the cache and thought buckets and prices them; cache rates fall back to the plain input rate only when the model has no rate at all (an explicit rate of 0 means free), thought tokens price as output.
• The derived branch keys off whether the provider has ever reported a cost for the session, rather than off this call's delta.
• An unpriced model warns once (@cache on the warn helper keeps it to one line per model, not one per turn).

Issue Number

Fixes #4382

How to Test

Unit tests:

uv run pytest tests/sdk/agent/test_acp_agent.py -k "TestEstimateCostFromTokens or TestRecordUsageDerivedCost"

Since unit tests alone are not sufficient here, the behaviour is also demonstrated against the real ACPAgent._record_usage with real litellm pricing (nothing on the pricing path is mocked), on gemini-2.5-flash: input 3e-07, output 2.5e-06, cache_read 3e-08, cache_creation absent.

Same script, upstream/main vs this branch:

                                                     before        after
scenario 1  cache-heavy turn, no provider cost
  1000 input / 100 output / 900 cache_read
  / 50 cache_write / 30 thought            0.00055000   0.00066700

scenario 2  provider reports cumulative 10.0 twice
  accumulated_cost                        10.00055000  10.00000000
  overcharge vs provider figure            0.00055000   0.00000000

scenario 3  unpriced model
  accumulated_cost                         0.00000000   0.00000000
  warning emitted                                 no          yes

Scenario 1 going up is the point: the missing 0.000117 is the 900 cache-read, 50 cache-write and 30 thought tokens that were previously free.

Scenario 3's warning:

WARNING  No LiteLLM pricing for ACP model 'totally-unknown-model-xyz' - cost stays 0 for this session

Full file: 441 passed. Four failures in TestACPFileSecretMaterialisation and TestACPDataDirIsolation are pre-existing on this Windows host and fail identically on unmodified upstream/main.

Each of the four behaviours was checked by neutralising it in turn and confirming the matching test goes red: dropping the double-count guard, unpricing cache reads, unpricing thought tokens, and removing the warning. All four are caught.

ruff format --check, ruff check and scripts/check_import_rules.py are clean on both touched files.

Video/Screenshots

Console output above; this path has no UI surface.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

• @EvolveAegis reported this with a dynamic reproduction, which is what the two scenarios above are built on. The one thing I would flag against that report is the direction of the cache fix: it describes the cache-read gap as a lost discount, and under ACP's bucket semantics it is a lost charge.
• Follow-up commit ed804ff after review: the cache-rate fallback used info.get(key) or input_cost, which cannot tell a missing rate from a rate of 0. 17 entries in litellm's cost map set cache_read_input_token_cost to 0 and 31 set cache_creation_input_token_cost to 0 (deepseek/deepseek-chat among them), so those were charged at the input rate instead of being free. Now a dict.get default, with a test.
• _estimate_cost_from_tokens keeps its positional (model, input, output) signature, so existing callers and tests are unaffected.
• Thought tokens are priced at the output rate. That matches how Gemini and Anthropic bill reasoning, but it is a judgement call, and worth a second opinion from someone who owns the pricing side.
• Not addressed here: the issue's item 5, that failed or cancelled turns never reach _record_usage at all and a pending UsageUpdate can be dropped on the next prepare_usage_sync. That is a lifecycle question rather than a pricing one, and it deserves its own change.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 015b2fe67f99204913fbe56d88f97f3b4b67e69b
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/05933f00-8f62-4776-9e4e-926feb390646

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Three cost-accounting fixes in ACPAgent._record_usage, all correct in direction and well-tested:

1. Cache + thought tokens priced — ACP reports these in separate buckets (not folded into input_tokens like litellm/OpenAI), so pricing them additively is the right call. The metrics.py cache_hit_rate doc already documents this bucket separation, which corroborates the approach.
2. Derived cost no longer stacks on provider cost — Keying the derived-estimate guard on session_id in _last_cost_by_session ("provider ever reported") instead of cost_recorded ("this call's delta > 0") correctly handles the repeated-cumulative-amount case where delta == 0. I verified the guard works: _last_cost_by_session[session_id] is set unconditionally inside the usage_update.cost is not None block (line 1907), so any provider cost report — even a delta == 0 one — marks the session.
3. Unpriced-model warning — @cache on _warn_unknown_acp_pricing keeps it to one line per model per process, which is appropriate for a warning.

Tests are solid: 8 new unit tests cover all four behaviors, and the stacking-guard test correctly fails against the old code (derived cost would be ~0.002 on the second call). All 12 new tests pass.

Risk Assessment

Low risk. The changes are isolated to ACP cost derivation, backward compatible (the _estimate_cost_from_tokens signature keeps its positional args with new optional kwargs), and don't touch agent behavior, prompts, or tool execution. No security concerns.

Findings

One inline finding (medium): or fallback over-charges models with explicitly free cache rates

See inline comment on line 846. info.get(key) or input_cost treats 0.0 as falsy, so a model whose litellm cost map explicitly sets cache_read_input_token_cost=0 or cache_creation_input_token_cost=0 (free cache reads/writes) gets priced at the input rate instead of $0. I confirmed this against real litellm data: 17 models have cache_read_input_token_cost=0 and 31 have cache_creation_input_token_cost=0. For example, deepseek/deepseek-chat (free cache writes) charges 100 cache-write tokens at $2.8e-05 instead of $0. The fix is info.get(key, input_cost) — returns input_cost only when the key is absent, and respects an explicit 0.0.

Minor: first-call $0 provider cost scenario untested

The stacking-guard test uses amount=10.0 (delta > 0 on first call). The edge case where the provider's first UsageUpdate has cost.amount=0.0 is handled correctly by the new guard (provider_cost_seen becomes True while cost_recorded stays False), but there's no test asserting it. Not blocking — the core delta==0 scenario is covered — but a one-line test would lock in the behavior.

Note: reasoning tokens priced at output rate

The PR description flags this as a judgement call. Pricing reasoning/thinking tokens at output_cost matches how Anthropic and Gemini bill them, so this is reasonable. Some litellm cost-map entries have model-specific reasoning rates, but those are uncommon and the output-rate default is a sound choice for now.

[all-hands-bot]

🚦 CI is currently failing on this PR's latest commit.

Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[onatozmenn]

The red sdk-tests on ed804ff is tests/sdk/conversation/test_fifo_lock.py::test_fifo_lock_fairness, which is unrelated to this change. It got [0, 1, 2, 3, 5, 4, 6, 7, 8, 9], so threads 4 and 5 swapped.

It's a pre-existing flake on main, not something this branch introduced. I went through the last 20 failed tests.yml runs on main and this same test took the job down twice on its own, run 16599 (2026-07-08) and run 16301 (2026-06-30). The other ten were all different timing-sensitive tests (test_cache_performance_repeated_reads, test_utility_methods_declare_operation_metadata, a couple of MCP stdio ones).

The mechanism is visible in the test: it starts ten threads, then sets their events 2ms apart and expects that to fix the queue order. On a busy runner a thread can miss its 2ms window, and the next one enqueues first. I could not reproduce it locally in 50 runs, including 20 under full CPU load, which fits, this box has 20 cores and 2ms is a lot of room here.

I can't re-run the job from a fork, so I pushed 03b2180 instead, which is the one-line test the review asked for on the first-report cost.amount=0.0 case. It fails against the old delta-based guard. That gives sdk-tests a fresh run as a side effect.

Happy to send the fairness test as its own PR if you want it made deterministic rather than left to timing.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 03b2180fcb8a1bbd0bf6f45f71a4fc00a23d0003
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/2d6b21c0-833e-4108-bda7-4710373e4194

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR fixes three cost-accounting defects in ACPAgent._record_usage, all correctly addressed since the prior review (commit 015b2fe):

1. Cache and thought tokens are now priced. _estimate_cost_from_tokens accepts cache_read_tokens, cache_write_tokens, and reasoning_tokens and prices them additively. This is the right call for ACP'''s bucket semantics — Usage.total_tokens is the sum of all types (confirmed by cache_hit_rate'''s docstring in metrics.py), so cached reads are a separate billable bucket, not a discountable subset of input_tokens.
2. Derived cost no longer stacks on provider cost. The guard now keys on session_id in self._client._last_cost_by_session ("provider ever reported") instead of cost_recorded ("this call'''s delta > 0"). Since _last_cost_by_session[session_id] is set unconditionally inside the usage_update.cost is not None block (line 1908), a repeated cumulative amount (delta == 0) correctly blocks the derived path.
3. Unpriced models warn once. @cache on _warn_unknown_acp_pricing keeps it to one warning per model per process, consistent with the existing @cache pattern in deprecation.py and model_features.py.

Previous review findings — all resolved

• or fallback over-charging free cache rates (medium, line 846 in 015b2fe): Fixed. Lines 847-848 now use info.get(key, input_cost) instead of info.get(key) or input_cost, correctly distinguishing a missing rate (fall back to input) from an explicit 0.0 (free). The test_explicit_zero_cache_rate_is_free test locks this in.
• First-call $0 provider cost untested (minor): Fixed. test_provider_reporting_zero_cost_blocks_the_derived_estimate covers the case where the first UsageUpdate has cost.amount=0.0 — provider_cost_seen becomes True via the unconditional _last_cost_by_session[session_id] = 0.0 assignment, and the derived estimate is correctly blocked.
• Reasoning tokens at output rate (note): Confirmed reasonable — matches Anthropic/Gemini billing.

Risk Assessment

Low risk. Changes are isolated to ACP cost derivation. The _estimate_cost_from_tokens signature keeps its positional (model, input, output) args with new optional kwargs, so existing callers are unaffected. No changes to agent behavior, prompts, tool execution, or event schemas. No security concerns.

Findings

No material findings. The implementation is correct, well-tested (12 new tests covering all four behaviors), and the previous review'''s inline finding has been resolved with the dict.get default pattern.

One minor observation (not actionable): the unpriced-model guard at line 843 (if not input_cost and not output_cost) would warn for a hypothetical model with both rates explicitly set to 0.0 in litellm'''s cost map, framing it as "no pricing" when it'''s actually "free." No real-world litellm entry has both rates at 0, so this is theoretical and the current behavior (warn + $0) is safe either way.