Skip to content

fix(research): split compressor protected turns by role, not midpoint - #50411

Open
Vesna-9 wants to merge 1 commit into
NousResearch:mainfrom
Vesna-9:fix/compressor-protected-region-role-split
Open

fix(research): split compressor protected turns by role, not midpoint#50411
Vesna-9 wants to merge 1 commit into
NousResearch:mainfrom
Vesna-9:fix/compressor-protected-region-role-split

Conversation

@Vesna-9

@Vesna-9 Vesna-9 commented Jun 21, 2026

Copy link
Copy Markdown

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

  • 🐛 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

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.5.0)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

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
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 21, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #7248 — both replace the same n // 2 head/tail split in _find_protected_indices (trajectory_compressor.py) with an explicit head-protected set + tail_start = max(0, n - protect_last_n_turns), computing compressible_start = max(head_protected) + 1 and compressible_end = tail_start. The identical fix; #7248 frames the symptom as an inverted region on short trajectories and this PR frames it as a late-first-tool misclassification, but both stem from the positional midpoint split and are resolved by the same change. #7248 is the earlier, still-open version.

Related: #40495 / #40593 (clean tool_call/tool_response boundary snapping — a different facet of the same function).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still uses the positional midpoint split in trajectory_compressor.py:517-521, so a late first tool turn can be classified as tail protection. The PR's separate head_protected set and tail_start boundary in trajectory_compressor.py:491-536 directly address that defect, and tests/test_trajectory_compressor.py:405-456 covers both the late-first-tool case and the protected-window invariant.

The existing tool-pair boundary handling added in f10a330aee7dd1d664389d15b35ccfd47bf0fe8d remains present in the PR base and current main. The current-base diff does not modify this function, so the change is suitable for a clean salvage.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/compression Context compression and continuation sessions labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/compression Context compression and continuation sessions duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants