fix(s08): snip_compact respects max_messages and handles tool-result boundaries#470
Open
qiugu wants to merge 1 commit into
Open
fix(s08): snip_compact respects max_messages and handles tool-result boundaries#470qiugu wants to merge 1 commit into
qiugu wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
snip_compact is the L1 layer of the s08 context-compaction pipeline. It drops the middle of the message list (replacing it with a placeholder) once the count exceeds max_messages. The original implementation had three issues.
Problems
Off-by-one (result exceeds the limit): The kept region keep_head(3) + keep_tail(max-3) = max, plus the inserted placeholder, yields a result of length max + 1 — so max_messages was never actually honored. When len(messages) == max + 1, it "removed 1, added 1 placeholder" and effectively did nothing.
Out-of-bounds for small max_messages: keep_tail = max_messages - 3 becomes <= 0 when max_messages <= 3, producing an empty/negative tail window, and the early-return then handed back the still-over-limit list.
Tail only handled a single tool_result: The original tail_start -= 1 left orphan tool_results at the tail start when a single tool call produced multiple results, and was asymmetric with the while loop used on the head side.
Fix
Reserve one slot for the placeholder: keep_tail = max_messages - keep_head - 1, so the compacted result never exceeds max_messages.
Add a max_messages < 2 guard to avoid empty/negative tail windows.
The tail boundary now walks back with while over consecutive tool_result messages, keeping multi-result tool calls intact (symmetric with the head side).
keep_head is capped with min(3, max_messages - 1) to stay safe for small max.
Verification
All 6 cases in tests/test_compaction_tool_pairs.py pass.
Additional checks: off-by-one (7 → ≤6), large scale (100 → ≤50), tail with multiple tool_results leaves no orphans, multi-result tool-call boundary, and max_messages=1 is a no-op — all as expected.
Scope
Only snip_compact in s08_context_compact/code.py changed. The contract is unchanged: head/tail are preserved, tool_use/tool_result pairs are protected, and the [snipped N messages] placeholder is used.