fix(agent): stop double-counting api_content in the token estimator

[israellot]

What changed and why

api_content is a substitute for content, not an addition to it. turn_context.substitute_api_content() pops the sidecar and overwrites content at every API-bound message-build site (the api_messages build in conversation_loop, the max-iterations summary in chat_completion_helpers, the chat-completions transport), so exactly one of the two is ever sent to the provider.

The preflight estimator counted both. Both _estimate_message_chars and _estimate_message_tokens_without_images walked every key of the persisted message dict behind a single-entry denylist (_anthropic_content_blocks), so any message carrying a sidecar that differs from its clean stored content was counted twice — exactly 2.00x on a 40KB sidecar:

upstream estimate, wire shape       (content only)          10,008
upstream estimate, persisted shape  (content + sidecar)     20,013
                                                            = 2.00x

Two reasons this is worth fixing rather than tolerating as "rough":

• The sidecar exists specifically to keep the provider prompt-cache prefix byte-stable across turns, so it is written on precisely the long, cache-pinned messages where doubling hurts most.
• estimate_messages_tokens_rough() is not display-only — it feeds the compaction threshold through context_compressor and conversation_loop. An inflated estimate makes compression fire on bytes that were never sent.

Approach

Substitute rather than sum, mirroring what substitute_api_content() does on the wire.

The two estimator helpers had drifted into near-identical copies of the same shadow-building loop, so rather than patch one site and leave the other subtly different, this factors the shared logic into _wire_message_shadow() and fixes the class once. Net effect on behaviour is limited to api_content; the refactor is otherwise mechanical.

Image accounting is deliberately untouched — base64 payloads are still replaced with a placeholder and charged at the flat _count_image_tokens rate, and the _multimodal text_summary path is preserved.

How to test

python -m pytest tests/agent/test_model_metadata.py -q
python -m pytest tests/agent -q -k "compress or context or token or estimate or prune"

Three new cases in TestEstimateMessagesTokensRough:

1. test_api_content_substitutes_for_content_not_added_to_it — sidecar equal to content is counted once (this is the regression guard; reverting the substitution fails it).
2. test_api_content_is_counted_when_it_differs_from_content — a lower bound, so it fails if the field were dropped rather than substituted. Dropping it would undercount the real request, which is the more dangerous direction: compaction would fire too late and the turn dies on a hard context error instead of compacting early.
3. test_api_content_does_not_defeat_image_stripping — a sidecar cannot smuggle raw base64 past the flat image rate.

Results on this branch:

• 53 passed — tests/agent/test_model_metadata.py
• 57 passed — with tests/agent/test_context_breakdown.py
• 656 passed, 3 skipped — compression/context/token/estimate/prune surface of tests/agent
• Mutation-tested: reverting the substitution fails case 1 above.

Platforms tested

Linux (x86_64, Python 3.11). scripts/check-windows-footguns.py is not applicable — this change touches no file I/O, process management, terminal handling, subprocesses, or signals. Pure dict/str logic, so cross-platform behaviour is identical.

Related

Same function, deliberately not the same bug as the reasoning-field over-count tracked in #73298 (with #72087 and #73306 in flight). Those concern the reasoning / reasoning_content / reasoning_details triple-count; this is the independent api_content double-count and touches a disjoint branch of the same loop. Kept separate to stay one-logical-change-per-PR and to avoid conflicting with that work.

One note for whoever consolidates #72087 / #73306, since it bit me: the exclusion at the top of these loops names _anthropic_content_blocks with a leading underscore, while the wire path writes anthropic_content_blocks (chat_completion_helpers → anthropic_adapter). The real replay channel is therefore counted only by accident today, and reshaping this loop into a field allowlist silently drops it — a ~1000x undercount on thinking models. Details in my comment on #72087. This PR keeps the denylist shape precisely to avoid that trap.

[alt-glitch]

This was generated by AI during triage.

Related to #72087 and #73298: this fixes api_content sidecar double-counting, while #72087 fixes a distinct Anthropic block-field overcount. Both edit the estimator helper surface; please consolidate or sequence the changes.

[teknium1]

Thanks for isolating this from the related reasoning/Anthropic estimator work. The premise is present on current main: agent/model_metadata.py:3005-3026 includes both persisted content and api_content, while agent/turn_context.py:100-106 and agent/conversation_loop.py:1477-1491 substitute the sidecar on the provider-bound shape.

Problems

• tests/agent/test_model_metadata.py's new test_api_content_does_not_defeat_image_stripping fixture does not include api_content; it verifies the existing content-list image behavior, not the test name's sidecar condition. Moreover, valid sidecars are strings under substitute_api_content() (agent/turn_context.py:100-106), so an image-list sidecar is not a provider-bound shape.

Suggested changes

• Rename that test as an image-stripping non-regression test for the helper extraction, or revise its description to avoid claiming it exercises api_content. The two direct substitution tests cover the reported defect.

This is an automated hermes-sweeper review.

[israellot]

Addressed in 9910a13. Thanks — the mis-named test was a real defect in the diff, and chasing it turned up a second fidelity gap in my own change.

1. The mis-named image test (sweeper finding) — fixed

You're right on both counts: the fixture never carried api_content, and a valid sidecar is a string, so an image-list sidecar isn't a provider-bound shape at all. There was nothing to test there. It is now test_image_stripping_survives_shadow_extraction, described as what it actually is — a non-regression pin on the flat per-image accounting that moved into the extracted _wire_message_shadow(). The two direct substitution tests carry the reported defect, as you noted.

2. The shadow didn't mirror substitute_api_content()'s guard — fixed

Writing that rename made me re-read my own predicate, and it was wrong in the dangerous direction. My shadow substituted whenever the api_content key was merely present:

if k == "content":
    if "api_content" in msg:   # too broad
        continue

But the wire substitutes only a non-empty string sidecar on a user/assistant row (agent/turn_context.py:100-106). For any other shape — None, "", a non-string, or a tool/system row — substitute_api_content() pops the sidecar and discards it, and the clean content is what gets sent. My shadow dropped content for those rows and counted nothing in its place, i.e. it undercounted.

That's the direction that actually hurts: an overcount makes compaction fire early, a undercount makes it fire too late and the turn dies on a hard context-length error. The shadow now computes sidecar_wins from the same three conditions and gates both branches on it. test_non_string_api_content_does_not_displace_content covers None, "", 42, a list, and a tool row; it fails against the unconditional version (verified by reverting the guard: AssertionError: None).

3. Sequencing with #72087 (re: the triage comment)

Sequencing, not consolidation — the two fixes are orthogonal (sidecar substitution vs. Anthropic block-field duplication) and each is independently reviewable. They do collide textually, so here's the concrete resolution rather than an assurance:

Both PRs touch _estimate_message_tokens_without_images. This PR extracts the shared shadow builder that both estimator helpers were duplicating; #72087 edits the duplicate in place. Merging in either order leaves one conflict hunk, and the naive resolution (take my extracted return estimate_tokens_rough(str(_wire_message_shadow(msg)))) silently discards #72087's dedup — it compiles and most tests pass, which is exactly what makes it a trap.

The correct resolution folds #72087's two conditions into the shared shadow: skip reasoning/reasoning_content/reasoning_details and the text-extracted content when anthropic_content_blocks is present. I ran that merge locally (my head 9910a13 + #72087 at 3522fb2) and applied it; the combined tests/agent/test_model_metadata.py is 58 passed, including all 4 of #72087's TestAnthropicInterleavedThinkingDedup tests and all 4 sidecar/image tests from this PR.

Whichever lands first, I'm happy to do the rebase and carry that resolution — just say which order you want. If you'd rather not sequence at all, I can equally close this and hand the two commits to @adurham to fold into #72087.

Note anthropic_content_blocks vs _anthropic_content_blocks: the pre-existing shadow skips the underscore-prefixed key, #72087 keys off the unprefixed one, and both exist in the tree (agent/transports/anthropic.py:183 and agent/anthropic_adapter.py:2262). I left that alone here since it's #72087's surface, but it looks worth confirming on that PR that the intended one is being read.

Verification

• tests/agent/test_model_metadata.py: 54 passed on this PR's head.
• New guard test confirmed RED before the fix, GREEN after.
• Regression diff against the PR base (cc4cab2) over tests/agent -k 'compress or context or token or estimate or prune': 48 failed / 607 passed on both, identical failure sets — no new failures introduced. (Those 48 are pre-existing on main in my environment, plus 4 collection errors from a missing optional openai dep.)

[kshitijk4poor]

Merged via #75892. Your commits cherry-picked with authorship preserved (rebase-merge). Thanks for the thorough fix and the excellent PR description!