fix(estimator): stop quadruple-counting Anthropic interleaved-thinking blocks - #72087
fix(estimator): stop quadruple-counting Anthropic interleaved-thinking blocks#72087adurham wants to merge 1 commit into
Conversation
…ousResearch#72087) Documents the outcome of the 5th Bucket A upstream candidate: submitted as PR NousResearch#72087 against NousResearch/hermes-agent, no competing PR found. This one required more investigation than expected. Verification found the fork's own commit message overstated its baseline -- the "existing content-dedup" it claimed to extend was itself a separate, earlier fork-only commit (7eee5ef) that never went upstream, so the portable fix needed to cover both content-dedup and reasoning-field-dedup, not just layer the latter on an existing former. Also found a genuine root cause upstream didn't know about: the estimator checks for a field named "_anthropic_content_blocks" (leading underscore) that's never actually written anywhere -- the real field (chat_completion_helpers.py writes it, anthropic_adapter.py's replay path reads it) is "anthropic_content_blocks", no underscore. The exclusion never fired. Reproduced the bug live against a real upstream worktree before writing anything: ~4x token overcounting on a message with one thinking block duplicated across content/reasoning/reasoning_content/reasoning_details/ anthropic_content_blocks. Confirmed the affected function (_estimate_message_tokens_without_images) is the live path feeding real compaction-trigger decisions (context_compressor, conversation_loop, turn_context, context_breakdown), not the dead-code sibling function (_estimate_message_chars, zero call sites upstream). Searched issues/PRs first per CONTRIBUTING.md -- genuinely nothing existing, unlike the two prior candidates this session. Wrote and tested the combined fix: 136/136 in the estimator's own test file, 302/302 across every real caller. Zero fork-only symbols in the isolated diff. No production code changes to the fork itself -- FORK.md + a saved reference patch only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ran all 7 upstream PRs filed this session back through external review with the real diffs. 2 came back with genuine, actionable findings, both fixed and pushed as follow-up commits to the existing PRs: - NousResearch#72054 (MCP orphan reap): silent exception swallow in the cleanup path now logs; added a 4th test exercising the real shutdown()/park machinery end-to-end rather than only faked versions. - NousResearch#72152 (profile deletion): tightened a false-positive-prone script-name match to the actual known console-script entry points. 2 more findings were checked against the real code and resolved as non-issues (not accepted at face value, not dismissed either) -- one on NousResearch#72087 (content/blocks divergence risk -- verified architecturally safe since blocks are what's actually replayed regardless of content's state; image-strip gap -- verified blocks can never contain images given how they're populated) and one on NousResearch#72151 (ref-update ordering -- verified correct by reading the real flush function). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the focused reproduction and narrow estimator fix. The current-main premise is verified: Problems
Suggested changes
Automated hermes-sweeper review. |
|
Independently hit this on a fork carrying the same estimator, and I can confirm both your premise and the sweeper's review ask — plus add one datum that I think constrains the final shape here and in #73306. The underscore is load-bearing. As the review notes, the exclusion names I did exactly that and measured the result. A ~11K-char interleaved-thinking payload went from 9,026 estimated tokens to 9: That trades a bounded overcount for a ~1000x undercount, which is the more dangerous direction: compaction fires too late and the turn dies on a hard context error instead of compacting early. Worth flagging that On the sweeper's lower-bound ask: it catches this exactly, and I'd suggest making it payload-proportional rather than a simple assert result >= baseline + (len(LARGE_PAYLOAD) // 4) * 0.9Parametrised over all three channel keys, this fails if any one is dropped or stubbed, while still allowing the duplicate-reasoning exclusion to be asserted separately. Emptying my replay-key set fails 5 of 6 new tests; the upper-bound-only version passed all of them. One trap if you add base64 stripping alongside this. Image Happy to push my parametrised replay-channel tests as a PR against this branch if that's useful, or leave them here if you'd rather keep the diff yours. Not filing a competing PR — the reasoning-dedup design belongs in this thread and #73298. |
|
Fixed per the sweeper's review and @israellot's detailed report -- thank you both, the exact ~9-token measurement was invaluable for confirming the fix has real teeth. Added Verified empirically before pushing: simulated the exact allowlist regression locally (reshaped the function to a bare field allowlist) and confirmed 5 of 7 tests in the class correctly fail against it -- reproduced your ~9-token estimate for the stripped payload almost exactly. Restored the real fix and all 7 pass. @israellot -- appreciate the offer to push parametrized tests directly; I went ahead and added them myself since I had the local repro running, but flagging your name in the commit message for the credit either way. Let me know if you want to open a follow-up for the base64-stripping trap you found in the replay blocks (separate concern from this PR's scope, but a real one) -- happy to look at that too if useful. |
…s on all 7 PRs Documents the real external engagement on the 7 upstream PRs filed 2026-07-26, and the 3 substantive follow-up fixes pushed in response: - NousResearch#72054 closed as superseded, but merged anyway via NousResearch#74139 (contributor CrowLoki's reconciliation with NousResearch#62026, credited via Co-authored-by). - NousResearch#72087, NousResearch#72151, NousResearch#72152, NousResearch#72153, NousResearch#72155, NousResearch#72164 all reviewed by the repo's automated sweeper -- keep_open/high on all 6. - Fixed NousResearch#72087 (payload-proportional test assertions, catching a future allowlist-regression risk flagged by both the sweeper and an independent contributor who measured it precisely on their own fork). - Fixed NousResearch#72152 (extracted ProfileRail's focus/visibilitychange wiring into a tested hook, matching the directory's own established use-profile-prewarm.ts pattern). - Rebased NousResearch#72155 past a real merge conflict (an unrelated upstream test-pruning pass removed 3 tests my diff's context touched). All fixes verified by simulating the exact regression each review was warning about and confirming the new tests catch it, then restoring the real fix. Also noted a real environment issue found this session: the `upstream` remote's SSH URL intermittently fails to connect from this network; a one-off HTTPS fetch into a separate ref works around it without touching the configured remote. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g blocks Rebased/retargeted after upstream's own NousResearch#73298 refactor extracted the shared `_wire_message_shadow()` helper out from under `_estimate_message_tokens_without_images()` (this fix's original target). That refactor introduced the exact bug this PR describes, independently: `_wire_message_shadow()` excludes the legacy back-compat key `_anthropic_content_blocks` (leading underscore) but the live wire field `chat_completion_helpers.py` actually writes and `anthropic_adapter.py` actually replays is `anthropic_content_blocks` (no underscore) -- so the exclusion silently never fires for the field that matters. Verified empirically against current `main` before fixing: a message with the same thinking text duplicated across `content`/`reasoning`/ `reasoning_content`/`anthropic_content_blocks` is estimated at 4.04x the correct token count. `_estimate_message_tokens_without_images()` now delegates to `_wire_message_shadow()`, so this bug affects BOTH the char count (`_estimate_message_chars`) and the token estimate feeding compaction-trigger decisions -- worse than the original single-function bug this PR was written against. Fixed by adding `anthropic_content_blocks` dedup directly into `_wire_message_shadow()`: when present, skip `content`/`reasoning`/ `reasoning_content` as pure duplicates of the same thinking text already inside the blocks (the replay path reads blocks alone for these turns). `reasoning_details` was already unconditionally excluded by an intervening fix (NousResearch#73298) and needed no change here. Guarded against a real regression risk raised in review (by the hermes-sweeper and independently by @israellot on NousResearch#72087, who measured a ~1000x undercount from an allowlist-shaped version of this exact fix): this is an EXCLUSION of specific known-duplicate keys, not a field allowlist. Added `_assert_payload_proportional()` lower-bound assertions to every test carrying a replay channel, plus explicit coverage for Codex's `codex_reasoning_items`/`codex_message_items` (a sibling verbatim-replay channel that must never be silently dropped) and a meta-test proving the assertion has real teeth. Verified: reverted the `anthropic_content_blocks` dedup locally and re-ran -- 5 of 7 new tests correctly fail, reproducing the same ~4x overcount pattern. Restored the fix; all 7 pass. Tests: tests/agent/test_model_metadata.py -- 66 passed (was already- passing baseline before this PR's tests + 7 new). Broader regression sweep (context_compressor/turn_context/context_breakdown, since _wire_message_shadow is now shared infra): 236 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a12cfa1 to
6511cc3
Compare
|
Important update: while rebasing this onto current Verified empirically before fixing: a message with duplicated thinking text across Retargeted the fix into Verified the fix has teeth: reverted just the |
…Research#72153 both hit real upstream refactors Documents rebasing both PRs onto main a few days after the first fix round, finding both had real merge conflicts caused by unrelated upstream work independently touching the exact code they target: - NousResearch#72087: upstream's own NousResearch#73298 refactor extracted a shared _wire_message_shadow() helper and independently reintroduced the same underscore-mismatch bug this PR describes (verified 4.04x overcount on unpatched main). Retargeted the fix into the new shared helper. - NousResearch#72153: upstream landed a user-configurable terminal font feature that replaced the two hardcoded strings this PR touched. The underlying bug was still real against the new default constant; fix collapsed to a single-constant change. All 6 open PRs confirmed mergeable as of today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SummarySeven PRs address or reference three compression defects: #72087, #73306, and #75884 change thinking-token estimation; #74251, #74417, and #75884 persist hygiene failure cooldowns; #75604, #75635, and #75884 bound the short-tool-suffix compression path. #75884 contains the landed cross-issue implementation, while #72087 retains a distinct Anthropic replay-channel deduplication beyond #75884's reasoning_details exclusion. Related pull requests
Duplicates#74417 is a duplicate of #74251; #75635 substantially duplicates #75604. #73306's estimator fix and the best-fix portions of #74251 and #75604 were incorporated into #75884; #72087 overlaps the estimator area but is not an exact duplicate because it deduplicates the live anthropic_content_blocks replay channel while preserving that channel's payload accounting. Suggested consolidationKeep #72087 open with a salvage path: retain its payload-proportional replay-channel tests and rebase its Anthropic content/reasoning deduplication on top of #75884's landed reasoning_details handling, explicitly verifying that anthropic_content_blocks and both Codex replay channels still count once. The automated keep-open verdict on #72087 supports this path; #73306, #74251, #74417, #75604, and #75635 should remain closed under the explicit salvage/duplicate chains to #75884, with no additional duplicate closure needed. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I73298(["issue #73298 (closed)"])
P72087["PR #72087 (open)"]
P72087 -.->|partial| I73298
class I73298 closed
class P72087 open
class P72087 target
click I73298 "https://github.com/NousResearch/hermes-agent/issues/73298"
click P72087 "https://github.com/NousResearch/hermes-agent/pull/72087"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 7 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 94 kB of PR diffs, 33 kB of issue/PR text, 27 kB of discussion (27 comments), 20 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
What does this PR do?
Fixes a token-estimation bug in
agent/model_metadata.py's_estimate_message_tokens_without_images()— the live preflight/compaction-trigger estimator (reached via
estimate_messages_tokens_rough(), calledfrom
context_compressor.py,conversation_loop.py,turn_context.py, andcontext_breakdown.py).When Anthropic's interleaved-thinking feature is active, the same thinking
text ends up duplicated across up to 5 places on a stored assistant message:
content,reasoning,reasoning_content,reasoning_details, andanthropic_content_blocks(the raw provider content array stashed onto themessage so
_convert_assistant_messageinagent/anthropic_adapter.pycanreplay it verbatim on the next API call — that replay path reads
anthropic_content_blocksalone and never touches the other four).The estimator had a stale exclusion check for a field named
_anthropic_content_blocks(leading underscore) that doesn't match the realfield name actually written (
chat_completion_helpers.py) and read(
anthropic_adapter.py's replay path) —anthropic_content_blocks, nounderscore. So the exclusion never fired, and
content/reasoning/reasoning_content/reasoning_details/anthropic_content_blockswere allwalked into the estimate in full.
Reproduced live (not just theoretically): a message with a single
thinking block replicated across all 5 fields estimated at ~4x the token
cost it should have (~4053 vs ~1000 for a 4000-char thinking block). This is
the dominant driver of a preflight/compaction estimate running far ahead of
the real provider-reported
prompt_tokenson any session using interleavedthinking + reasoning effort — triggering compaction well before the actual
context window is anywhere near full, with no indication to the user that
it was imminent.
Related Issue
No existing issue found for this (searched via
gh search issues/gh search prsfor "estimate_message_chars", "anthropic_content_blocks double count","preflight estimator quadruple", "premature compaction interleaved
thinking" — no hits). Filing directly with a live-reproduced repro instead.
Type of Change
Changes Made
agent/model_metadata.py:_estimate_message_tokens_without_images()—corrected the field-name check from
_anthropic_content_blockstoanthropic_content_blocks, and extended the same skip toreasoning/reasoning_content/reasoning_detailswhenanthropic_content_blocksispresent. Non-Anthropic providers (no
anthropic_content_blockson themessage) are unaffected — those fields are their only copy and are still
counted in full.
tests/agent/test_model_metadata.py: addedTestAnthropicInterleavedThinkingDedup(4 tests) — blocks-present dedup counts the thinking text once, the
non-Anthropic fallback path still counts reasoning fields normally, the
content-vs-blocks-only path (no reasoning fields) still dedupes correctly,
and an end-to-end regression guard comparing the fixed estimate against
what the old unstripped behavior would have produced.
How to Test
content,anthropic_content_blocks(as athinkingblock),reasoning,reasoning_content, andreasoning_details.estimate_messages_tokens_rough([msg])returnsroughly 4x the token count of the actual unique text.
scripts/run_tests.sh tests/agent/test_model_metadata.py -q— 136passed (132 pre-existing + 4 new).
tests/agent/test_context_compressor*.py,test_context_breakdown.py,test_turn_context*.py— 302 passed, 0 failed.Checklist
pytest tests/ -q(targeted:tests/agent/test_model_metadata.py+ all real callers of the estimator) and all tests pass