fix(compaction): skip compression when it cannot reduce the token count - #56415
fix(compaction): skip compression when it cannot reduce the token count#56415golldyck wants to merge 1 commit into
Conversation
compress_trajectory (and _async) replaced the compressible middle region with a [CONTEXT SUMMARY] turn without checking that the region is actually larger than the summary. When a large protected system prompt dominates the budget, the compressible middle can be tiny; replacing e.g. a 2-token middle with a ~60-token summary GROWS the trajectory (tokens_saved negative), marks it was_compressed, and still spends a summarization call — the opposite of the intent, on exactly the hard over-budget cases. Add a net-savings guard mirroring the code's own comment (net_savings = region_tokens - summary_target_tokens): if the safely-compressible region is no larger than summary_target_tokens, return the trajectory unchanged. Applied to both the sync and async paths. Add sync+async regression tests.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Adds a guard to skip compression when the compressible region is smaller than the summary that would replace it. Prevents token count from growing instead of shrinking.
Looks Good
- Clean guard in both sync and async paths
- Well-tested with both sync and async test cases
- Correctly returns trajectory unchanged when compression would not help
Reviewed by Hermes Agent
|
Rechecked against current |
|
Merged via salvage PR #70243 as a clean cherry-pick, authorship preserved — thanks @golldyck! Both sync and async net-savings guards plus your regression tests are on main (merge commit 69339aa). The counterfactual run against pre-fix main confirmed the tests target the real 406→465-token growth bug. |
What does this PR do?
Skips trajectory compaction when it can't actually reduce the token count.
compress_trajectory(and its async twin) replaced a compressible middle region with a summary without checking that the summary is smaller than what it replaces. When the region was already small, the "summary" came out larger than the original. So the trajectory grew while still being markedwas_compressed=True(observed: 406 tokens went up to 465,tokens_saved = -59).The fix adds a net-savings guard: when the compressible region's token count is
<=the summary target, return the trajectory unchanged instead of expanding it. This applies to both the sync and async paths (sibling call path included).Related Issue
No separate issue filed; reproduces on current
main.Type of Change
Changes Made
trajectory_compressor.py: guard bothcompress_trajectoryandcompress_trajectory_asyncso they skip compression whenregion_tokens <= summary_target_tokens, returning the trajectory unchanged.tests/test_trajectory_compressor.py: sync and async tests asserting a tiny compressible region is left unchanged (no token growth,was_compressednot set).How to Test
main: compressing a trajectory with a small compressible middle grows the token count yet reportswas_compressed=True.scripts/run_tests.sh tests/test_trajectory_compressor.py -q. The added net-savings tests pass (34 passed). Note: 3 tests (*_omits_temperature, kimi/moonshot) fail identically on unmodifiedmain. They're unrelated to this change (temperature-omission for kimi models), not a regression from this PR.Checklist
Code
fix(compaction): ...)scripts/run_tests.sh tests/test_trajectory_compressor.py; the net-savings tests pass, and the only failures are the 3 pre-existing*_omits_temperaturecases that also fail onmainDocumentation & Housekeeping
cli-config.yaml.example: N/ACONTRIBUTING.md/AGENTS.md: N/A