Skip to content

fix: exclude reasoning_details from preflight token estimate (#73298) - #73306

Closed
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/reasoning-details-token-estimate
Closed

fix: exclude reasoning_details from preflight token estimate (#73298)#73306
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/reasoning-details-token-estimate

Conversation

@JonthanaHanh

Copy link
Copy Markdown
Contributor

Summary

The reasoning_details field (OpenRouter/Anthropic thinking blocks + opaque cryptographic signature blobs) inflates the rough preflight token estimate by ~4x, causing premature compression on thinking models.

Providers do not bill these envelope bytes as prompt tokens — in a measured Kimi K3 session, reasoning_details held 2,124K chars vs 281K chars of actual thinking text. The estimator reported ~533K tokens when real prompt_tokens was ~140K, triggering compression at ~27% of the configured threshold.

Fix

Skip reasoning_details in both _estimate_message_chars and _estimate_message_tokens_without_images, alongside the existing _anthropic_content_blocks exclusion. This is the same treatment already applied to Anthropic content blocks (which also contain opaque metadata not billed as tokens).

Changes

  • agent/model_metadata.py: Add reasoning_details to the skip set in both _estimate_message_chars() (L2788) and _estimate_message_tokens_without_images() (L2817)

Test Plan

  • All 132 tests in tests/agent/test_model_metadata.py pass
  • Syntax check passes

Fixes #73298

The reasoning_details field (OpenRouter/Anthropic thinking blocks +
opaque cryptographic signature blobs) inflates the rough token estimate
by ~4x. Providers do not bill these envelope bytes as prompt tokens.

In a measured Kimi K3 session, reasoning_details held 2,124K chars
vs 281K chars of actual thinking text. The estimator reported ~533K
tokens when real prompt_tokens was ~140K — triggering compression at
~27% of the configured threshold.

Fix: skip reasoning_details in both _estimate_message_chars and
_estimate_message_tokens_without_images, alongside the existing
_anthropic_content_blocks exclusion.

Fixes NousResearch#73298
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint provider/kimi Kimi / Moonshot P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation labels Jul 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #72087 already changes the same estimator hunk with a broader, regression-tested Anthropic replay-field deduplication. This PR is the narrower reasoning_details-only repair for #73298; a maintainer should choose or consolidate the overlapping approaches.

@PRATHAMESH75

Copy link
Copy Markdown
Contributor

Reviewed against #73298 — this correctly fixes the root cause. Excluding reasoning_details from the shadow in both _estimate_message_chars and _estimate_message_tokens_without_images (agent/model_metadata.py) drops exactly the ~2.1M-char signed envelope the issue identified as phantom weight, while reasoning_content (the ~281K of actual thinking text) is still counted — so the estimate lands near the ~188K reconstruction instead of the phantom ~533K. This is the issue's suggested direction #1, and updating _estimate_message_chars too is right since it feeds other budget paths.

Two notes for a reviewer:

  • No regression test. Worth adding one: a message dict carrying a large reasoning_details blob should no longer inflate estimate_messages_tokens_rough / _estimate_message_tokens_without_images (assert the estimate with vs. without the blob is ~equal). That locks in the fix and guards the exact regression [Bug]: Preflight token estimate counts reasoning_details envelope at chars/4 — compression fires at ~27% of real usage on thinking models #73298 describes.
  • Undercount direction is safe here. Fully excluding the envelope means the rough estimate no longer sees those bytes, but production evidence in the issue (real prompt_tokens ~140K while the envelope alone was 2.1M chars) shows the provider does not bill it at chars/4, and should_defer_preflight_to_real_usage prefers the real reading once one exists — so this won't cause the opposite (late-compression) failure.

LGTM once a test is added.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused preflight repair. The current-main premise is verified: agent/model_metadata.py:3005-3027 places reasoning_details into the estimator shadow, and agent/conversation_loop.py:1764 uses that estimate for request pressure.

Problems

  • The PR changes both estimator helpers but adds no regression coverage. A future refactor could reintroduce the signed-envelope inflation without failing tests/agent/test_model_metadata.py.

Suggested changes

  • Add a behavioral test comparing otherwise identical messages with and without a large reasoning_details payload. Assert that estimate_messages_tokens_rough is unchanged; directly test _estimate_message_chars too, since this diff changes it.
  • The member comment correctly notes overlap with #72087, which edits the same estimator hunk with an Anthropic-specific deduplication strategy. Consolidate the intended behavior before landing either patch.

Automated hermes-sweeper review.

Comment thread agent/model_metadata.py
@@ -2785,7 +2785,7 @@ def _estimate_message_chars(msg: Dict[str, Any]) -> int:
return len(str(msg))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a regression test showing that a large reasoning_details envelope leaves estimate_messages_tokens_rough unchanged while ordinary counted reasoning content still affects the result. This line changes the production preflight path but currently has no direct coverage.

@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Merged via salvage PR #75884 (#75884) — your preflight estimator fix was cherry-picked with authorship preserved (composed into main's newer _wire_message_shadow() helper during rebase), plus a companion commit covering the second site @israellot documented (_REPLAY_BUDGET_KEYS tail-budget walk), counting thinking text but excluding signature envelopes per the #51800 discussion. Fixes #73298. Thanks!

@teknium1 teknium1 closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists provider/kimi Kimi / Moonshot sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Preflight token estimate counts reasoning_details envelope at chars/4 — compression fires at ~27% of real usage on thinking models

4 participants