From 25855180aaf253e757bdc8d5d83b9f2cb1c3a0b6 Mon Sep 17 00:00:00 2001 From: Vesna-9 <290871358+Vesna-9@users.noreply.github.com> Date: Sun, 21 Jun 2026 23:31:29 +0300 Subject: [PATCH] fix(research): split compressor protected turns by role, not midpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trajectory compressor decided where the compressible middle started and ended by partitioning the protected turn indices around the trajectory midpoint (n // 2): indices below it were treated as the protected head and indices at or above it as the protected tail. That positional split breaks whenever the first tool call lands in the second half of the trajectory: the first-tool turn gets misclassified as a tail turn, so it becomes the compressible region's end. The region then spans from just after the first gpt turn up to the first tool turn, which means the compressor summarizes the pre-tool conversation and leaves the entire tool-interaction middle untouched. Concretely, for a 24-turn trajectory whose first tool turn is at index 13 (protect_last_n_turns=4), the region was [3, 13): it compressed the early human/gpt setup turns 3..12 and skipped the tool middle 14..19. That both contradicts the documented strategy ("compress MIDDLE turns only, starting from 2nd tool response") and frequently leaves the trajectory over the token budget, since the heavy tool outputs in the middle are never touched. The fix tracks the first-occurrence head turns in their own set and ends the compressible region at the start of the protected last-N tail, instead of splitting by n // 2. After the change the same trajectory yields the region [14, 20): it compresses the tool middle and preserves the early setup. The returned protected set is unchanged, so head/tail protection and existing behavior for the common early-first-tool case are identical. ## What does this PR do? Fixes a bug in `_find_protected_indices` in the trajectory compressor that caused the wrong region of a trajectory to be summarized when the first tool call occurred in the trajectory's second half. The compressible region is now computed from the role-of-origin of each protected turn (first system/human/gpt/tool turns form the head; the last N turns form the tail) rather than from each turn's position relative to the midpoint. This keeps a late first-tool turn in the protected head, so the region that actually gets summarized is the tool-interaction middle, as documented, and the compressor stops summarizing pre-tool setup turns it was meant to leave alone. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) - [ ] ✨ New feature (non-breaking change that adds functionality) - [ ] 🔒 Security fix - [ ] 📝 Documentation update - [ ] ✅ Tests (adding or improving test coverage) - [ ] ♻️ Refactor (no behavior change) - [ ] 🎯 New skill (bundled or hub) ## Changes Made - `trajectory_compressor.py`: in `TrajectoryCompressor._find_protected_indices`, collect the first-occurrence head turns into a dedicated `head_protected` set, derive `tail_start` from `protect_last_n_turns`, and set `compressible_start = max(head_protected) + 1` and `compressible_end = tail_start`. Removed the `n // 2` head/tail split. The returned `protected` set is unchanged. - `tests/test_trajectory_compressor.py`: added `test_late_first_tool_compresses_tool_middle_not_pre_tool_chatter` (24-turn trajectory with the first tool at index 13; asserts the region is [14, 20)) and `test_compressible_region_excludes_all_protected_turns` (invariant that no protected index falls inside the compressible region). ## How to Test 1. Reproduce the bug on the old code: for a 24-turn trajectory whose first `tool` turn is at index 13 with `protect_last_n_turns=4`, `_find_protected_indices` returned the region `[3, 13)` — compressing the pre-tool turns and skipping the tool middle. 2. With this change the same input returns `[14, 20)`, so the tool-interaction middle is compressed and the early setup is preserved. 3. Run `pytest tests/test_trajectory_compressor.py tests/test_trajectory_compressor_async.py -q` — all tests pass, including the two new ones. ## Checklist ### Code - [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md) - [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.) - [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate - [x] My PR contains **only** changes related to this fix/feature (no unrelated commits) - [x] I've run `pytest tests/ -q` and all tests pass - [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features) - [x] I've tested on my platform: macOS 15 (Darwin 25.5.0) ### Documentation & Housekeeping - [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A - [x] I've considered cross-platform impact (Windows, macOS) per the [compatibility guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — or N/A - [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A --- tests/test_trajectory_compressor.py | 53 +++++++++++++++++++++++++++++ trajectory_compressor.py | 44 +++++++++++++++--------- 2 files changed, 80 insertions(+), 17 deletions(-) diff --git a/tests/test_trajectory_compressor.py b/tests/test_trajectory_compressor.py index 8fcbfc38cfef..c2b6995c5002 100644 --- a/tests/test_trajectory_compressor.py +++ b/tests/test_trajectory_compressor.py @@ -402,6 +402,59 @@ def test_disable_protect_first_system(self): protected, _, _ = tc._find_protected_indices(trajectory) assert 0 not in protected # system not protected + def test_late_first_tool_compresses_tool_middle_not_pre_tool_chatter(self): + """The compressible region must follow role-of-origin, not n // 2. + + When the first tool call lands in the second half of the trajectory, + the old positional split classified that head turn as a "tail" turn, + so the compressible region became [3, first_tool) — squeezing the + pre-tool conversation and leaving the tool-interaction middle intact, + and starting *before* the first tool response. The region must instead + begin right after the first tool turn and end at the protected tail. + """ + tc = _make_compressor() # protect_last_n_turns defaults to 4 + # 24 turns; the first "tool" turn is at index 13 (second half). + trajectory = [ + {"from": "system", "value": "sys"}, + {"from": "human", "value": "q"}, + ] + for i in range(2, 13): # indices 2..12: no tool turns yet + trajectory.append({"from": "gpt" if i % 2 == 0 else "human", "value": "x"}) + trajectory.append({"from": "tool", "value": "first tool result"}) # index 13 + for i in range(14, 24): # indices 14..23: the tool-interaction middle + tail + trajectory.append({"from": "gpt" if i % 2 == 0 else "tool", "value": "y"}) + + protected, start, end = tc._find_protected_indices(trajectory) + + first_tool = 13 + tail_start = len(trajectory) - 4 # 20 + # Region starts right after the first tool turn (not before it). + assert start == first_tool + 1 == 14 + # Region ends where the protected last-4 turns begin. + assert end == tail_start == 20 + # No protected turn is ever inside the compressible region. + assert not any(start <= idx < end for idx in protected) + # The first tool turn itself is protected as a head turn. + assert first_tool in protected + + def test_compressible_region_excludes_all_protected_turns(self): + """Invariant: protected turns are never inside the compressible region.""" + tc = _make_compressor() + trajectory = [ + {"from": "system", "value": "sys"}, + {"from": "human", "value": "q"}, + {"from": "gpt", "value": "a"}, + {"from": "tool", "value": "r"}, + {"from": "gpt", "value": "b"}, + {"from": "tool", "value": "r2"}, + {"from": "gpt", "value": "c"}, + {"from": "tool", "value": "r3"}, + {"from": "gpt", "value": "d"}, + {"from": "human", "value": "thanks"}, + ] + protected, start, end = tc._find_protected_indices(trajectory) + assert not any(start <= idx < end for idx in protected) + # --------------------------------------------------------------------------- # TrajectoryCompressor._extract_turn_content_for_summary diff --git a/trajectory_compressor.py b/trajectory_compressor.py index 9dc3826a854d..91a411a690f0 100644 --- a/trajectory_compressor.py +++ b/trajectory_compressor.py @@ -488,10 +488,14 @@ def _find_protected_indices(self, trajectory: List[Dict[str, str]]) -> Tuple[set """ n = len(trajectory) protected = set() - + # Head turns are the first occurrence of each role; the compressible + # region begins right after the last of them. Tracked separately from + # the tail so the boundary is decided by role-of-origin, not position. + head_protected = set() + # Track first occurrences first_system = first_human = first_gpt = first_tool = None - + for i, turn in enumerate(trajectory): role = turn.get("from", "") if role == "system" and first_system is None: @@ -502,28 +506,34 @@ def _find_protected_indices(self, trajectory: List[Dict[str, str]]) -> Tuple[set first_gpt = i elif role == "tool" and first_tool is None: first_tool = i - + # Protect first turns if self.config.protect_first_system and first_system is not None: - protected.add(first_system) + head_protected.add(first_system) if self.config.protect_first_human and first_human is not None: - protected.add(first_human) + head_protected.add(first_human) if self.config.protect_first_gpt and first_gpt is not None: - protected.add(first_gpt) + head_protected.add(first_gpt) if self.config.protect_first_tool and first_tool is not None: - protected.add(first_tool) - - # Protect last N turns - for i in range(max(0, n - self.config.protect_last_n_turns), n): + head_protected.add(first_tool) + protected.update(head_protected) + + # Protect last N turns (the tail group) + tail_start = max(0, n - self.config.protect_last_n_turns) if self.config.protect_last_n_turns > 0 else n + for i in range(tail_start, n): protected.add(i) - - # Determine compressible region - # Start after the last protected head turn - head_protected = [i for i in protected if i < n // 2] - tail_protected = [i for i in protected if i >= n // 2] - + + # Determine compressible region: everything strictly between the last + # protected head turn and the start of the protected tail. We must NOT + # split protected indices by their position relative to the trajectory + # midpoint (n // 2): when the first tool call lands in the second half + # of the trajectory, a positional split misclassifies that head turn as + # a tail turn, which makes the compressor squeeze the pre-tool + # conversation and leave the tool-interaction middle untouched — + # contrary to the documented "compress MIDDLE turns only, starting from + # 2nd tool response" strategy. compressible_start = max(head_protected) + 1 if head_protected else 0 - compressible_end = min(tail_protected) if tail_protected else n + compressible_end = tail_start return protected, compressible_start, compressible_end