From 8daf03063dae8a78ac056cdf64247417863bbe78 Mon Sep 17 00:00:00 2001 From: TRON Date: Mon, 20 Jul 2026 21:02:33 -0400 Subject: [PATCH] fix(compression): add pre-LLM feasibility check to skip costly no-op summaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the middle section is < 10% of threshold tokens AND at least one prior real-usage ineffectiveness strike has been recorded, skip the expensive LLM summarization call and fall through to the deterministic message-dropping path. Without this guard, a tool-heavy session where the protected tail already holds most of the tokens can burn 500+ seconds on a summary call that replaces a few lightweight messages with negligible token savings. Key design decisions per GottZ review on PR #60451: 1. Separate _prellm_skip_count counter — never increments _ineffective_compression_count (the strike counter that latches at >=2 to disable compression entirely). One real strike + one skip must NOT permanently lock out compression until /new. 2. feasibility_skip sentinel flag — exempts skips from the abort branch (abort_on_summary_failure / _last_summary_auth_failure / _last_summary_network_failure). A stale failure flag from a prior cycle must not turn a deliberate skip into a full abort. 3. reason=None for feasibility-skip fallbacks — a stale _last_summary_error from an earlier real failure must not be embedded into the skip's deterministic fallback marker. 4. info-level logging for feasibility-skip fallbacks (not warning) — this is an intentional optimization, not a failure. Skipped when force=True (manual /compress) so auth/error handling paths are always exercised on explicit user request. Adds 6 regression tests (TestPreLlmFeasibilityCheck) covering: - Strike counter isolation - Stale auth/network failure flag immunity - force=True bypass - No-skip when no prior strikes - Counter reset on session reset Co-authored-by: Cursor Co-authored-by: TRON --- agent/context_compressor.py | 119 ++++++++++++-- agent/conversation_compression.py | 4 + tests/agent/test_context_compressor.py | 215 +++++++++++++++++++++++++ 3 files changed, 327 insertions(+), 11 deletions(-) diff --git a/agent/context_compressor.py b/agent/context_compressor.py index bcb9e01cef341..7e878119a135b 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -636,6 +636,12 @@ _ACTIVE_TASK_MAX_CHARS = 1400 # high for small/light tails, but using all 20 as a hard floor here would bring # back the old large-tool-output case where nothing can be compacted. _MAX_TAIL_MESSAGE_FLOOR = 8 + +# Pre-LLM feasibility skip (#60451): when the compressible middle is below +# this fraction of threshold_tokens (and a prior real-usage ineffectiveness +# strike exists), skip the LLM summary call — deterministic dropping alone +# recovers the negligible savings such a summary could deliver. +_FEASIBILITY_SKIP_MIDDLE_FRACTION = 0.10 # Under context pressure (protected-tail tool bodies alone exceed the soft # tail budget), demote large completed tool/file outputs even inside the # protected region — but always keep this many trailing messages verbatim so @@ -1329,11 +1335,13 @@ class ContextCompressor(ContextEngine): self._consecutive_timeout_failures = 0 self._last_summary_dropped_count = 0 self._last_summary_fallback_used = False + self._last_feasibility_skip = False self._last_aux_model_failure_error = None self._last_aux_model_failure_model = None self._last_compression_savings_pct = 100.0 self._ineffective_compression_count = 0 self._anti_thrash_recovery_deadline = 0.0 + self._prellm_skip_count = 0 self._fallback_compression_streak = 0 self._verify_compaction_cleared_threshold = False self._last_compression_made_progress = False @@ -1595,11 +1603,13 @@ class ContextCompressor(ContextEngine): self._consecutive_timeout_failures = 0 self._last_summary_dropped_count = 0 self._last_summary_fallback_used = False + self._last_feasibility_skip = False self._last_aux_model_failure_error = None self._last_aux_model_failure_model = None self._last_compression_savings_pct = 100.0 self._ineffective_compression_count = 0 self._anti_thrash_recovery_deadline = 0.0 + self._prellm_skip_count = 0 self._fallback_compression_streak = 0 self._verify_compaction_cleared_threshold = False self._last_compression_made_progress = False @@ -1626,6 +1636,7 @@ class ContextCompressor(ContextEngine): self._consecutive_timeout_failures = 0 self._fallback_compression_streak = 0 self._ineffective_compression_count = 0 + self._prellm_skip_count = 0 self._anti_thrash_recovery_deadline = 0.0 self.get_active_compression_failure_cooldown() self._load_fallback_compression_streak() @@ -1770,9 +1781,34 @@ class ContextCompressor(ContextEngine): self._ineffective_compression_count = count self._persist_ineffective_compression_count() - def record_completed_compaction(self, *, used_fallback: bool = False) -> None: - """Record one completed boundary and its summary quality.""" + def record_completed_compaction( + self, *, used_fallback: bool = False, feasibility_skip: bool = False, + ) -> None: + """Record one completed boundary and its summary quality. + + ``feasibility_skip=True`` marks a deliberate pre-LLM skip (#60451): + the boundary is streak-NEUTRAL for ``_fallback_compression_streak`` + (neither incremented nor reset). It still arms the real-usage + effectiveness verdict (``_verify_compaction_cleared_threshold``) on + purpose — a skipped-summary drop that fails to clear the threshold is + exactly the incompressible-transcript case the ineffective-strike + breaker exists for, and its recovery probe bounds the block. + """ self._verify_compaction_cleared_threshold = True + if feasibility_skip: + # A deliberate pre-LLM feasibility skip (#60451) is not a + # summary-quality verdict: it must neither extend a fallback + # streak (two skips would otherwise latch the >= 2 breaker and + # disable compression entirely — including the cheap deterministic + # dropping the skip exists to reach) nor reset one (a skip proves + # nothing about the summary model's health). + if not self.quiet_mode: + logger.info( + "Compaction completed via pre-LLM feasibility skip; " + "fallback_compression_streak unchanged (%d)", + self._fallback_compression_streak, + ) + return if used_fallback: self._fallback_compression_streak += 1 if not self.quiet_mode: @@ -1982,6 +2018,7 @@ class ContextCompressor(ContextEngine): # trigger invalidates them. Keep the durable copy in sync so a # restart doesn't resurrect strikes this recalibration just voided. self._record_ineffective_compression_verdict(0) + self._prellm_skip_count = 0 if runtime_changed: self._fallback_compression_streak = 0 self._persist_fallback_compression_streak() @@ -2279,6 +2316,9 @@ class ContextCompressor(ContextEngine): # restart with a persisted tripped counter (#69872) waits a full fresh # window before probing (#54923: restart must never disarm a guard). self._anti_thrash_recovery_deadline: float = 0.0 + # Pre-LLM feasibility skips (#60451). Observability only; NEVER feeds + # the ineffectiveness strike latch or the fallback streak breaker. + self._prellm_skip_count: int = 0 # Consecutive completed deterministic-fallback boundaries. Unlike the # real-usage effectiveness counter, ordinary fitting responses must not # reset this breaker; only a healthy completed summary does. @@ -2300,6 +2340,7 @@ class ContextCompressor(ContextEngine): # (gateway hygiene, /compress) can surface a visible warning. self._last_summary_dropped_count: int = 0 self._last_summary_fallback_used: bool = False + self._last_feasibility_skip: bool = False # When summary generation fails we now ABORT compression entirely # and return the original messages unchanged instead of dropping # the middle window with a static placeholder. Callers inspect @@ -5879,6 +5920,7 @@ This compaction should PRIORITISE preserving all information related to the focu # after compress() returns to decide whether to surface a warning. self._last_summary_dropped_count = 0 self._last_summary_fallback_used = False + self._last_feasibility_skip = False self._last_summary_error = None self._last_aux_model_failure_error = None self._last_aux_model_failure_model = None @@ -6162,11 +6204,55 @@ This compaction should PRIORITISE preserving all information related to the focu # Phase 3: Generate structured summary summary_focus_topic = focus_topic or self._derive_auto_focus_topic(messages) - summary = self._generate_summary( - turns_to_summarize, - focus_topic=summary_focus_topic, - memory_context=memory_context, - ) + + # Pre-LLM feasibility check: if the middle section is too small to + # yield meaningful token savings, skip the expensive LLM summarization + # call and fall through to the deterministic message-dropping path + # (which is cheap and always applicable). Without this guard a + # tool-heavy session where the protected tail already holds most of + # the tokens can burn 500+ seconds on a summary call that replaces a + # few lightweight messages, leaving the total token count essentially + # unchanged. + # + # Only fires after at least one prior real-usage ineffectiveness + # strike. The check READS ``_ineffective_compression_count`` but + # never writes it: that strike counter is fed exclusively by real + # provider token counts (see the anti-thrashing verdict in + # _update_token_usage), and consumers latch at >= 2 to disable + # compression entirely. Feasibility skips are tracked separately + # in ``_prellm_skip_count`` for observability. + # + # Skipped when ``force=True`` (manual /compress) so auth/error + # handling paths are always exercised on explicit user request. + feasibility_skip = False + if not force and self._ineffective_compression_count >= 1: + middle_tokens = estimate_messages_tokens_rough(turns_to_summarize) + if middle_tokens < int( + self.threshold_tokens * _FEASIBILITY_SKIP_MIDDLE_FRACTION + ): + feasibility_skip = True + self._last_feasibility_skip = True + self._prellm_skip_count += 1 + telemetry["prellm_skip_count"] = self._prellm_skip_count + if not self.quiet_mode: + logger.warning( + "Compression: middle section (%d tokens at indices " + "%d-%d) is below %.0f%% of threshold (%d tokens) — " + "skipping LLM summarization, proceeding with " + "deterministic message dropping. prellm_skip_count=%d", + middle_tokens, compress_start, compress_end, + _FEASIBILITY_SKIP_MIDDLE_FRACTION * 100, + self.threshold_tokens, self._prellm_skip_count, + ) + + if feasibility_skip: + summary = None # No LLM call; Phase 4 inserts the deterministic fallback + else: + summary = self._generate_summary( + turns_to_summarize, + focus_topic=summary_focus_topic, + memory_context=memory_context, + ) # If summary generation failed, behavior splits on # ``abort_on_summary_failure`` (config: compression.abort_on_summary_failure): @@ -6188,7 +6274,7 @@ This compaction should PRIORITISE preserving all information related to the focu # of these cases, rotating into a child session with a placeholder # summary degrades the conversation for zero benefit. Preserve it # unchanged until access is restored or connectivity recovers. - if not summary and ( + if not summary and not feasibility_skip and ( self.abort_on_summary_failure or self._last_summary_auth_failure or self._last_summary_network_failure @@ -6268,15 +6354,26 @@ This compaction should PRIORITISE preserving all information related to the focu # content-free "N messages were removed" marker. if not summary: if not self.quiet_mode: - logger.warning("Summary generation failed — inserting deterministic fallback context summary") + if feasibility_skip: + logger.info("Feasibility skip — inserting deterministic fallback context summary") + else: + logger.warning("Summary generation failed — inserting deterministic fallback context summary") n_dropped = compress_end - compress_start self._last_summary_dropped_count = n_dropped self._last_summary_fallback_used = True telemetry["fallback_used"] = True - telemetry["failure_class"] = telemetry.get("failure_class") or "summary_generation_failed" + if feasibility_skip: + # Deliberate optimization, not a summary failure — keep the + # telemetry class distinct so dashboards don't count skips + # as aux-model breakage. + telemetry["failure_class"] = telemetry.get("failure_class") or "feasibility_skip" + else: + telemetry["failure_class"] = telemetry.get("failure_class") or "summary_generation_failed" summary = self._build_static_fallback_summary( turns_to_summarize, - reason=self._last_summary_error, + # A stale error from an earlier real failure must not be + # embedded into a deliberate feasibility skip's fallback. + reason=None if feasibility_skip else self._last_summary_error, ) tail_messages: List[Dict[str, Any]] = [] diff --git a/agent/conversation_compression.py b/agent/conversation_compression.py index 8308b618da76b..81b8ca71ac165 100644 --- a/agent/conversation_compression.py +++ b/agent/conversation_compression.py @@ -1838,6 +1838,9 @@ def compress_context( _compression_used_fallback = bool( getattr(agent.context_compressor, "_last_summary_fallback_used", False) ) + _compression_feasibility_skip = bool( + getattr(agent.context_compressor, "_last_feasibility_skip", False) + ) # If compression aborted (aux LLM failed to produce a usable summary) # the compressor returns the input messages unchanged. Surface the @@ -2348,6 +2351,7 @@ def compress_context( record_boundary( agent.context_compressor, used_fallback=_compression_used_fallback, + feasibility_skip=_compression_feasibility_skip, ) else: agent.context_compressor._verify_compaction_cleared_threshold = True diff --git a/tests/agent/test_context_compressor.py b/tests/agent/test_context_compressor.py index 3b51c6482f539..ba54b997765d4 100644 --- a/tests/agent/test_context_compressor.py +++ b/tests/agent/test_context_compressor.py @@ -2694,3 +2694,218 @@ class TestContextLengthSetterCoherence: # ...and budgets recompute from the same window+percent. assert c.threshold_tokens == 150_000 + + +class TestPreLlmFeasibilityCheck: + """Tests for the pre-LLM feasibility skip in compress(). + + When the middle section is < 10% of threshold tokens AND at least one + prior real-usage ineffectiveness strike has been recorded, the expensive + LLM summarization call is skipped and the deterministic fallback is used + instead. The skip must NOT increment _ineffective_compression_count (the + strike counter that latches at >=2) and must NOT trip the abort branch + via stale _last_summary_auth_failure / _last_summary_network_failure flags. + """ + + def _make_messages(self, n_pairs=10, content="short reply"): + """Build a message list large enough to survive compress()'s early exits. + + compress() bails if n_messages <= protect_head_size + 4, and again + if compress_start >= compress_end (tail budget covers everything). + With protect_first_n=2 → head=2, protect_last_n=2, we need enough + middle turns to produce compress_start < compress_end. 10 pairs + (21 messages including system) is comfortably past both gates. + """ + msgs = [{"role": "system", "content": "system prompt"}] + for i in range(n_pairs): + msgs.append({"role": "user", "content": f"question {i}"}) + msgs.append({"role": "assistant", "content": content}) + return msgs + + def test_skip_does_not_increment_strike_counter(self, compressor): + """Feasibility skip must use _prellm_skip_count, not _ineffective_compression_count.""" + compressor._ineffective_compression_count = 1 # one prior real strike + msgs = self._make_messages() + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary") as mock_gen: + mock_llm.return_value = (None, None) + # Middle section is tiny → feasibility skip fires + compressor.compress(msgs, force=False) + + # The strike counter must NOT have been incremented by the skip + assert compressor._ineffective_compression_count == 1 + # The pre-LLM skip counter should have been incremented + assert compressor._prellm_skip_count >= 1 + # _generate_summary must NOT have been called + mock_gen.assert_not_called() + + def test_skip_does_not_trip_abort_on_stale_auth_failure(self, compressor): + """A feasibility skip must not trip the abort branch even if + _last_summary_auth_failure is stale True from a prior cycle.""" + compressor._ineffective_compression_count = 1 + compressor._last_summary_auth_failure = True # stale from prior failure + msgs = self._make_messages() + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary") as mock_gen: + mock_llm.return_value = (None, None) + result = compressor.compress(msgs, force=False) + + # Should NOT abort — should produce compressed output with fallback + assert result is not None + assert len(result) > 0 + mock_gen.assert_not_called() + + def test_skip_does_not_trip_abort_on_stale_network_failure(self, compressor): + """Same as above but for _last_summary_network_failure.""" + compressor._ineffective_compression_count = 1 + compressor._last_summary_network_failure = True # stale + msgs = self._make_messages() + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary") as mock_gen: + mock_llm.return_value = (None, None) + result = compressor.compress(msgs, force=False) + + assert result is not None + assert len(result) > 0 + mock_gen.assert_not_called() + + def test_no_skip_when_force_true(self, compressor): + """force=True (manual /compress) must bypass the feasibility check.""" + compressor._ineffective_compression_count = 1 + msgs = self._make_messages() + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary", return_value="LLM summary") as mock_gen: + mock_llm.return_value = (None, None) + compressor.compress(msgs, force=True) + + # _generate_summary MUST have been called despite tiny middle section + mock_gen.assert_called_once() + assert compressor._prellm_skip_count == 0 + + def test_no_skip_when_no_prior_strikes(self, compressor): + """No prior ineffectiveness strikes → feasibility check doesn't fire.""" + compressor._ineffective_compression_count = 0 + msgs = self._make_messages() + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary", return_value="LLM summary") as mock_gen: + mock_llm.return_value = (None, None) + compressor.compress(msgs, force=False) + + mock_gen.assert_called_once() + assert compressor._prellm_skip_count == 0 + + def test_skip_count_resets_on_session_reset(self, compressor): + """_prellm_skip_count must reset alongside _ineffective_compression_count.""" + compressor._prellm_skip_count = 5 + compressor._ineffective_compression_count = 2 + + # on_session_reset() resets all per-session counters + compressor.on_session_reset() + + assert compressor._prellm_skip_count == 0 + assert compressor._ineffective_compression_count == 0 + + def test_skip_count_resets_on_bind_session_state(self, compressor): + """Rebinding to a session row must clear the per-session skip counter + like every other per-session guard (stale carry-over from a previous + binding must not inflate the new session's observability count).""" + compressor._prellm_skip_count = 5 + + compressor.bind_session_state(None, "s-new") + + assert compressor._prellm_skip_count == 0 + + def test_skip_fires_on_fat_tail_small_middle(self, compressor): + """The target scenario from #60451: a tool-heavy transcript whose + protected tail already holds most of the tokens, leaving a tiny + middle window. The skip must fire and _generate_summary must not + be called.""" + compressor._ineffective_compression_count = 1 + msgs = [{"role": "system", "content": "system prompt"}] + # Small middle: a few lightweight early exchanges. + for i in range(6): + msgs.append({"role": "user", "content": f"early question {i}"}) + msgs.append({"role": "assistant", "content": "brief answer"}) + # Fat tail: recent turns carrying big tool-style payloads. + for i in range(4): + msgs.append({"role": "user", "content": f"recent request {i}"}) + msgs.append({"role": "assistant", "content": "big result " + "x" * 20000}) + + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary") as mock_gen: + mock_llm.return_value = (None, None) + result = compressor.compress(msgs, force=False) + + mock_gen.assert_not_called() + assert compressor._prellm_skip_count == 1 + assert compressor._last_feasibility_skip is True + # Deterministic dropping still made progress. + assert len(result) < len(msgs) + + def test_boundary_accounting_skip_does_not_feed_fallback_streak(self, compressor): + """The interaction teknium's sweeper review flagged on #68334: the + skip path sets _last_summary_fallback_used, which the boundary + wrapper (conversation_compression.py) records via + record_completed_compaction(used_fallback=True) — incrementing + _fallback_compression_streak, whose second occurrence blocks + automatic compression. Two deliberate skips must NOT trip that + breaker.""" + compressor._ineffective_compression_count = 1 + msgs = self._make_messages() + + for _ in range(2): + with patch("agent.context_compressor.call_llm") as mock_llm, \ + patch.object(compressor, "_generate_summary") as mock_gen: + mock_llm.return_value = (None, None) + compressor.compress(list(msgs), force=False) + mock_gen.assert_not_called() + # Mirror the boundary wrapper's bookkeeping + # (agent/conversation_compression.py: record_completed_compaction + # call after a made-progress boundary). + assert compressor._last_compression_made_progress is True + compressor.record_completed_compaction( + used_fallback=compressor._last_summary_fallback_used, + feasibility_skip=compressor._last_feasibility_skip, + ) + + assert compressor._prellm_skip_count == 2 + assert compressor._fallback_compression_streak == 0 + assert not compressor._automatic_compression_blocked_locally(), ( + "two deliberate feasibility skips must not disable automatic " + "compression via the fallback-streak breaker" + ) + + def test_boundary_accounting_skip_does_not_reset_fallback_streak(self, compressor): + """A skip proves nothing about the summary model's health: an + existing real-fallback streak must survive a skip boundary (neither + incremented nor reset).""" + compressor.record_completed_compaction(used_fallback=True) + assert compressor._fallback_compression_streak == 1 + + compressor.record_completed_compaction( + used_fallback=True, feasibility_skip=True, + ) + + assert compressor._fallback_compression_streak == 1 + + def test_real_fallback_still_feeds_streak(self, compressor): + """Negative control: a genuine summary-failure fallback boundary + (no feasibility skip) must keep incrementing the streak breaker.""" + compressor._ineffective_compression_count = 1 + msgs = self._make_messages(content="filler " * 3000) # fat middle → no skip + + with patch.object(compressor, "_generate_summary", return_value=None): + compressor.compress(list(msgs), force=False) + + assert compressor._last_feasibility_skip is False + assert compressor._last_summary_fallback_used is True + compressor.record_completed_compaction( + used_fallback=compressor._last_summary_fallback_used, + feasibility_skip=compressor._last_feasibility_skip, + ) + assert compressor._fallback_compression_streak == 1