mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-16 14:32:34 +00:00
fix(compaction): check the threshold against real tokens, not an estimated floor
Follow-up to the previous commit, whose futility check was unsound:
incompressible_floor = max(0, display_tokens - pre_estimate)
`display_tokens` is the provider's real prompt count; `pre_estimate` is
`estimate_messages_tokens_rough(messages)`. Subtracting an estimate from a real
count folds the tokenizer skew into "floor" and misreads it as incompressible
overhead. With a 1.6x skew on a 200K window (threshold 150K, true floor 30K):
rough_msgs=253,804 real_prompt=436,086
computed floor = 182,282 <-- mostly skew; exceeds the threshold
after compaction: 401 -> 77 msgs, real prompt = 106,361 (CLEARS 150,000)
verdict: ineffective_count = 1 <-- false positive
Two such passes would permanently disable compaction on a healthy session --
worse than the loop this PR set out to fix.
Move the check into should_compress(), where both sides of the comparison are
the caller's own token count:
* prompt under the threshold -> not thrashing; reset the counter
* a compaction just ran and we are STILL over -> one strike
Real-vs-real, so tokenizer skew can never be mistaken for a floor, and nothing
subtracts an estimate from a real count. compress() now only ever increments the
counter; the reset lives with the one measure the trigger uses.
Adds `test_no_false_positive_under_tokenizer_skew` (the case above) and
`test_counter_resets_once_the_prompt_fits_again` (one failed pass must not
disable compaction forever). Against upstream, 5 of the 7 cases fail; the 2 that
pass are the regression guards, which is the intended shape.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
32f30d2a4f
commit
d172445629
2 changed files with 97 additions and 27 deletions
|
|
@ -736,6 +736,7 @@ class ContextCompressor(ContextEngine):
|
|||
self._last_aux_model_failure_model = None
|
||||
self._last_compression_savings_pct = 100.0
|
||||
self._ineffective_compression_count = 0
|
||||
self._verify_compaction_cleared_threshold = False
|
||||
self._summary_failure_cooldown_until = 0.0 # transient errors must not block a fresh session
|
||||
self._last_summary_error = None
|
||||
self._last_compress_aborted = False
|
||||
|
|
@ -771,6 +772,7 @@ class ContextCompressor(ContextEngine):
|
|||
self._last_aux_model_failure_model = None
|
||||
self._last_compression_savings_pct = 100.0
|
||||
self._ineffective_compression_count = 0
|
||||
self._verify_compaction_cleared_threshold = False
|
||||
self._summary_failure_cooldown_until = 0.0
|
||||
self._last_compress_aborted = False
|
||||
self._context_probed = False
|
||||
|
|
@ -944,6 +946,7 @@ class ContextCompressor(ContextEngine):
|
|||
self.last_compression_rough_tokens = 0
|
||||
self.awaiting_real_usage_after_compression = False
|
||||
self._ineffective_compression_count = 0
|
||||
self._verify_compaction_cleared_threshold = False
|
||||
|
||||
# When the MINIMUM_CONTEXT_LENGTH floor meets/exceeds a small context
|
||||
# window, compacting at the percentage (50% → 32K of a 64K window) wastes
|
||||
|
|
@ -1131,6 +1134,9 @@ class ContextCompressor(ContextEngine):
|
|||
# Anti-thrashing: track whether last compression was effective
|
||||
self._last_compression_savings_pct: float = 100.0
|
||||
self._ineffective_compression_count: int = 0
|
||||
# Set by compress(); consumed by should_compress() to check the next
|
||||
# real reading against the threshold. See should_compress().
|
||||
self._verify_compaction_cleared_threshold: bool = False
|
||||
self._summary_failure_cooldown_until: float = 0.0
|
||||
self._last_summary_error: Optional[str] = None
|
||||
# When summary generation fails and a static fallback is inserted,
|
||||
|
|
@ -1231,8 +1237,41 @@ class ContextCompressor(ContextEngine):
|
|||
where each pass removes only 1-2 messages.
|
||||
"""
|
||||
tokens = prompt_tokens if prompt_tokens is not None else self.last_prompt_tokens
|
||||
|
||||
# Verify the previous compaction against the SAME measure this method
|
||||
# gates on. Effectiveness is "did the prompt get under the threshold?",
|
||||
# not "did the message list shrink?": compaction can only shrink
|
||||
# messages, while the system prompt and tool schemas are an
|
||||
# incompressible floor (with 50+ tools, 20-30K tokens — see #14695).
|
||||
# When that floor alone meets the threshold, every pass can shrink
|
||||
# messages by a healthy margin and still leave the prompt over the line,
|
||||
# so the next turn compacts again, forever.
|
||||
#
|
||||
# Both sides of this comparison are the caller's own token count, so it
|
||||
# is immune to the skew between a rough estimate and the provider's real
|
||||
# tokenizer — an analytic "floor = real_prompt - rough_estimate" is not,
|
||||
# and would misread that skew as an incompressible floor.
|
||||
if tokens < self.threshold_tokens:
|
||||
# The prompt fits. Whatever happened before, we are not thrashing.
|
||||
self._ineffective_compression_count = 0
|
||||
self._verify_compaction_cleared_threshold = False
|
||||
return False
|
||||
|
||||
# Past this point the prompt is over the threshold. If a compaction just
|
||||
# ran, it failed to clear it — one strike. Two strikes and we stop and
|
||||
# tell the user, instead of compacting on every turn forever.
|
||||
if self._verify_compaction_cleared_threshold:
|
||||
self._verify_compaction_cleared_threshold = False
|
||||
self._ineffective_compression_count += 1
|
||||
if not self.quiet_mode:
|
||||
logger.warning(
|
||||
"Compaction did not clear the threshold: %d tokens still "
|
||||
">= %d. The incompressible prompt (system prompt + tool "
|
||||
"schemas) may already exceed it, in which case shrinking "
|
||||
"messages cannot help. ineffective_compression_count=%d",
|
||||
tokens, self.threshold_tokens,
|
||||
self._ineffective_compression_count,
|
||||
)
|
||||
# Do not trigger compression while the summary LLM is in cooldown.
|
||||
# On a 429/transient failure _generate_summary() sets a cooldown and
|
||||
# returns None; compress() then inserts a static fallback marker and
|
||||
|
|
@ -2812,6 +2851,12 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
running so a manual ``/compress`` can retry immediately after
|
||||
an auto-compression abort. Auto-compress callers pass False.
|
||||
"""
|
||||
# A compaction attempt is running: have should_compress() verify, against
|
||||
# the next real reading, that it actually cleared the threshold. Set on
|
||||
# every path (including the early no-op returns below) so a pass that
|
||||
# changed nothing is still held to account.
|
||||
self._verify_compaction_cleared_threshold = True
|
||||
|
||||
# Reset per-call summary failure state — callers inspect these fields
|
||||
# after compress() returns to decide whether to surface a warning.
|
||||
self._last_summary_dropped_count = 0
|
||||
|
|
@ -3165,31 +3210,11 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
savings_pct = (saved_estimate / pre_estimate * 100) if pre_estimate > 0 else 0
|
||||
self._last_compression_savings_pct = savings_pct
|
||||
|
||||
# Effectiveness is "did we get the prompt under the threshold?", not
|
||||
# "did the message list shrink?". ``should_compress()`` trips on the
|
||||
# FULL prompt, but compaction can only shrink messages: the system
|
||||
# prompt and tool schemas are an incompressible floor. When that floor
|
||||
# alone meets the threshold — a too-small context_length makes this
|
||||
# trivially true — every pass can shrink messages by a healthy margin,
|
||||
# reset the counter here, and still leave the prompt over the line, so
|
||||
# the next turn compacts again, forever. Score against the goal.
|
||||
incompressible_floor = max(0, display_tokens - pre_estimate)
|
||||
projected_prompt = incompressible_floor + new_estimate
|
||||
if projected_prompt >= self.threshold_tokens:
|
||||
# Only ever INCREMENT here. The reset lives in should_compress(), which
|
||||
# is the one place that sees the same measure the trigger uses; see
|
||||
# ``_verify_compaction_cleared_threshold``.
|
||||
if savings_pct < 10:
|
||||
self._ineffective_compression_count += 1
|
||||
if not self.quiet_mode:
|
||||
logger.warning(
|
||||
"Compaction cannot clear the threshold: ~%d incompressible "
|
||||
"tokens (system prompt + tool schemas) + ~%d compressed "
|
||||
"messages >= %d threshold. Shrinking messages further "
|
||||
"cannot help. ineffective_compression_count=%d",
|
||||
incompressible_floor, new_estimate, self.threshold_tokens,
|
||||
self._ineffective_compression_count,
|
||||
)
|
||||
elif savings_pct < 10:
|
||||
self._ineffective_compression_count += 1
|
||||
else:
|
||||
self._ineffective_compression_count = 0
|
||||
|
||||
if not self.quiet_mode:
|
||||
logger.info(
|
||||
|
|
|
|||
|
|
@ -14,8 +14,15 @@ Two defects made it dead code:
|
|||
next turn compacts again, forever.
|
||||
|
||||
Together these made a mis-sized context window present as a hung CLI rather than a
|
||||
warning. Effectiveness is now scored against the goal: did the prompt get under the
|
||||
threshold?
|
||||
warning. Effectiveness is now scored against the goal -- did the prompt get under the
|
||||
threshold? -- and the check is made in ``should_compress()`` against the caller's own
|
||||
token count on both sides.
|
||||
|
||||
That last detail matters. An analytic ``floor = current_tokens - rough_estimate(messages)``
|
||||
looks equivalent and is not: it silently absorbs the skew between the rough estimator and
|
||||
the provider's real tokenizer, reports it as incompressible overhead, and disables
|
||||
compaction on a perfectly healthy session. ``test_no_false_positive_under_tokenizer_skew``
|
||||
pins that.
|
||||
"""
|
||||
import pytest
|
||||
|
||||
|
|
@ -80,7 +87,8 @@ class TestFutilityGuard:
|
|||
"""Incompressible floor >= threshold -> shrinking messages cannot help."""
|
||||
cc = _compressor(threshold_tokens=24_576)
|
||||
msgs = _messages(14)
|
||||
# Full prompt is far above threshold and dominated by system + tools.
|
||||
# Full prompt is far above threshold and dominated by system + tools,
|
||||
# so it never drops no matter how much the message list shrinks.
|
||||
real_prompt_tokens = 33_564
|
||||
|
||||
fired = 0
|
||||
|
|
@ -107,6 +115,43 @@ class TestFutilityGuard:
|
|||
assert cc._last_compression_savings_pct > 10
|
||||
assert cc._ineffective_compression_count == 0
|
||||
|
||||
def test_no_false_positive_under_tokenizer_skew(self):
|
||||
"""The provider's real count exceeds the rough estimate; that is not a floor.
|
||||
|
||||
An analytic `floor = real_prompt - rough_estimate` misreads tokenizer skew
|
||||
as incompressible overhead and disables compaction on a healthy session.
|
||||
The check must compare the caller's own measure on both sides.
|
||||
"""
|
||||
from agent.context_compressor import estimate_messages_tokens_rough
|
||||
|
||||
skew, floor = 1.6, 30_000
|
||||
cc = _compressor(threshold_tokens=150_000)
|
||||
msgs = _messages(160, size=2500)
|
||||
|
||||
real_prompt = floor + int(skew * estimate_messages_tokens_rough(msgs))
|
||||
assert cc.should_compress(real_prompt)
|
||||
msgs = cc.compress(msgs, current_tokens=real_prompt)
|
||||
real_prompt = floor + int(skew * estimate_messages_tokens_rough(msgs))
|
||||
|
||||
assert real_prompt < cc.threshold_tokens, "compaction really did clear it"
|
||||
assert not cc.should_compress(real_prompt)
|
||||
assert cc._ineffective_compression_count == 0, (
|
||||
"tokenizer skew must not be mistaken for an incompressible floor"
|
||||
)
|
||||
|
||||
def test_counter_resets_once_the_prompt_fits_again(self):
|
||||
"""One failed pass must not permanently disable compaction."""
|
||||
cc = _compressor(threshold_tokens=24_576)
|
||||
msgs = _messages(14)
|
||||
|
||||
cc.should_compress(33_564)
|
||||
cc.compress(msgs, current_tokens=33_564)
|
||||
cc.should_compress(33_564) # still over -> 1 strike
|
||||
assert cc._ineffective_compression_count == 1
|
||||
|
||||
cc.should_compress(1_000) # now under -> forgiven
|
||||
assert cc._ineffective_compression_count == 0
|
||||
|
||||
|
||||
class TestMinimumMessagesBranch:
|
||||
def test_too_few_messages_records_an_ineffective_pass(self):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue