fix(compression): don't recommend a threshold the small-context floor will ignore - #67431
fix(compression): don't recommend a threshold the small-context floor will ignore#67431Sora-bluesky wants to merge 3 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the stale recommendation loop; the underlying mismatch is present on current upstream main (agent/conversation_compression.py:341, agent/context_compressor.py:1261-1263).
Problems
agent/conversation_compression.py:353checks only the percentage floor against the raw context window. The actual built-in trigger also uses the output reservation, 64K floor, and effective-window guard inagent/context_compressor.py:1293-1305. That can suppress a threshold recommendation that would actually fit after the persisted setting is recomputed.- The new code unconditionally imports and applies built-in
ContextCompressorpolicy. Plugin engines can be active atagent/agent_init.py:1881-1908, and that code explicitly states that external engines own compaction policy.
Suggested changes
- Gate the floor-specific calculation to the built-in compressor and use
_compute_threshold_tokens(..., max_tokens)after applying the effective percent. - Add a reservation-based regression test and a non-built-in-engine test.
Automated hermes-sweeper review.
|
|
||
| threshold_suggestion_viable = ( | ||
| not main_ctx | ||
| or _CC._effective_threshold_percent(main_ctx, safe_pct / 100) * main_ctx |
There was a problem hiding this comment.
This is not the complete built-in trigger calculation: _compute_threshold_tokens() also applies max_tokens reservation, the 64K floor, and the degenerate effective-window branch (agent/context_compressor.py:1293-1305). It also should not run for a plugin context engine, which owns its own policy. Please gate this to ContextCompressor and compare aux_context to the fully recomputed token threshold.
|
Both points addressed in
The non-viable message now names the recomputed trigger instead of hardcoding the 75%/512K wording, so it stays accurate when the reservation — not the percentage floor — is what makes the value unreachable. |
75d5b2a to
da7780a
Compare
da7780a to
150bffe
Compare
… will ignore The auxiliary-compression feasibility warning computes its compression.threshold suggestion as aux_context / main_context, independently of ContextCompressor._effective_threshold_percent()'s raise-only small-context floor. For main windows under 512K the floor raises any configured value below 75% back up, so a suggestion like 'threshold: 0.40' is silently ignored and the same warning returns every session. Derive the suggestion's viability through the compressor's own floor logic: offer the 'lower the threshold' option only when the floored value still fits the auxiliary model's context; otherwise recommend only a larger compression model and explain the floor, so the guidance is always actionable. Tests: the updated auto-correct test pins the floored branch (no threshold suggestion, floor explained); two new tests pin the surviving suggestion at/above the floor on a small window and below 75% on a 512K+ window where no floor applies. The updated test fails against the previous code. Fixes NousResearch#67422 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion guard Review follow-up on NousResearch#67431 (hermes-sweeper): - The viability check compared the floored percentage against the raw context window, but the built-in trigger recomputation also applies the output-token reservation, the 64K floor, and the degenerate-window guard (_compute_threshold_tokens). Mirror that math exactly, so e.g. a 200K window with max_tokens=120K recomputes to max(0.75*80K, 64K)=64K and the suggestion is correctly KEPT for an 80K aux model instead of being suppressed by the raw-window percentage. - Gate the built-in policy behind isinstance(ContextCompressor): external context engines own compaction policy (NousResearch#44439), so plugin engines keep the plain suggestion untouched. - The non-viable explanation now names the recomputed trigger instead of hardcoding the 75%/512K wording, so it stays accurate when the reservation (not the percentage floor) is what makes the value unreachable. Tests: reservation-viability regression and plugin-engine passthrough, per the review; the floored-branch assertion updated to the recomputed number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
150bffe to
21a59ab
Compare
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion guard Review follow-up on #67431 (hermes-sweeper): - The viability check compared the floored percentage against the raw context window, but the built-in trigger recomputation also applies the output-token reservation, the 64K floor, and the degenerate-window guard (_compute_threshold_tokens). Mirror that math exactly, so e.g. a 200K window with max_tokens=120K recomputes to max(0.75*80K, 64K)=64K and the suggestion is correctly KEPT for an 80K aux model instead of being suppressed by the raw-window percentage. - Gate the built-in policy behind isinstance(ContextCompressor): external context engines own compaction policy (#44439), so plugin engines keep the plain suggestion untouched. - The non-viable explanation now names the recomputed trigger instead of hardcoding the 75%/512K wording, so it stays accurate when the reservation (not the percentage floor) is what makes the value unreachable. Tests: reservation-viability regression and plugin-engine passthrough, per the review; the floored-branch assertion updated to the recomputed number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Agreed the original suggestion skipped |
|
Thanks for the review. Both points were against commit 7073d1a8; the follow-up 21a59ab (six minutes later, and on the current head) already does what it asks:
Flagging in case the keep_open verdict was rendered before that follow-up landed. |
…tion guard Review follow-up on NousResearch#67431 (hermes-sweeper): - The viability check compared the floored percentage against the raw context window, but the built-in trigger recomputation also applies the output-token reservation, the 64K floor, and the degenerate-window guard (_compute_threshold_tokens). Mirror that math exactly, so e.g. a 200K window with max_tokens=120K recomputes to max(0.75*80K, 64K)=64K and the suggestion is correctly KEPT for an 80K aux model instead of being suppressed by the raw-window percentage. - Gate the built-in policy behind isinstance(ContextCompressor): external context engines own compaction policy (NousResearch#44439), so plugin engines keep the plain suggestion untouched. - The non-viable explanation now names the recomputed trigger instead of hardcoding the 75%/512K wording, so it stays accurate when the reservation (not the percentage floor) is what makes the value unreachable. Tests: reservation-viability regression and plugin-engine passthrough, per the review; the floored-branch assertion updated to the recomputed number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What does this PR do?
The auxiliary-compression feasibility warning computes its
compression.thresholdsuggestion asaux_context / main_context(agent/conversation_compression.py), independently ofContextCompressor._effective_threshold_percent()'s raise-only small-context floor (agent/context_compressor.py:1261-1262). For main windows under 512K the floor raises any configured value below 75% back up, so a suggestion likethreshold: 0.40is silently ignored and the identical warning returns every session — an advice loop the user cannot exit by following the advice.This PR derives the suggestion's viability through the compressor's own floor logic (the
_effective_threshold_percentstaticmethod, so the two can't drift): the "lower the threshold" option is offered only when the floored value still fits the auxiliary model's context; otherwise the message recommends only a larger compression model and explains the floor. This is the issue's first two suggested fixes — no new config surface.Related Issue
Fixes #67422
Type of Change
Changes Made
agent/conversation_compression.py— computethreshold_suggestion_viableviaContextCompressor._effective_threshold_percent(); branch the persistence-guidance section of the warning on it. Unknown main context keeps the legacy message (the floor can't be evaluated without a window size).tests/run_agent/test_compression_feasibility.py— the existing auto-correct test (200K main, 40% suggestion → floored) now pins the new branch: nothreshold:suggestion, floor explained. Two new tests pin the surviving suggestion at/above the floor on a small window (100K main, 80% ≥ 75%) and below 75% on a 512K+ window where no floor applies (1M main, 30%).How to Test
scripts/run_tests.sh tests/run_agent/test_compression_feasibility.py -qcompression.threshold; it explains the floor and recommends a larger aux model. Previously, applying the suggested value changed nothing and the warning repeated every session.Checklist
Code
tests/run_agent/test_compression_feasibility.py,tests/agent/test_context_compressor.py) — 201 passed, zero failures on my platformDocumentation & Housekeeping
cli-config.yaml.example— N/A (no config surface added)CONTRIBUTING.md/AGENTS.md— N/AScreenshots / Logs
🤖 Generated with Claude Code