Skip to content

fix(compressor): strip orphaned tool messages at compression boundary - #20886

Closed
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:fix/context-compressor-tool-boundary
Closed

fix(compressor): strip orphaned tool messages at compression boundary#20886
liuhao1024 wants to merge 3 commits into
NousResearch:mainfrom
liuhao1024:fix/context-compressor-tool-boundary

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented May 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a crash where context compression produces invalid tool/assistant message ordering, causing HTTP 400 errors from providers like DeepSeek.

Root Cause

After compression, the first message in the tail section may be a tool message whose parent assistant (which contained the tool_calls) was in the summarised region. The existing _sanitize_tool_pairs() handles orphaned tool_call_id matching (removes tool results whose call_id has no matching assistant tool_call), but does not catch the ordering issue: some providers require every tool message to be immediately preceded by the assistant message that issued the corresponding tool_calls.

When the boundary lands on a tool message:

  • The preceding message is the inserted summary (user or assistant without tool_calls)
  • The API rejects this with: Messages with role 'tool' must be a response to a preceding message with 'tool_calls'

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

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 26.4.1

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 and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

…INSTALL_TIMEOUT

Increase the default npm install timeout for WhatsApp bridge from 60s
to 300s (5 minutes) to accommodate slower systems like Unraid NAS.
Make it configurable via WHATSAPP_NPM_INSTALL_TIMEOUT environment variable
for users who need even longer timeouts.

Closes NousResearch#14980
- Add 'path', 'old_string', 'new_string', and 'patch' to required list
- Update description to clarify mode-specific parameter requirements
- This addresses issue where LLMs would omit these parameters because
  they were not marked as required in the schema, even though they
  are required depending on the mode

Fixes NousResearch#15524
After context compression, the first message in the tail may be a tool
message whose parent assistant was summarised away. Providers like
DeepSeek require every tool message to be immediately preceded by the
assistant that issued the corresponding tool_calls, causing HTTP 400.

The existing _sanitize_tool_pairs handles orphaned call_id matching,
but does not catch tool messages at the boundary where the parent
assistant no longer has tool_calls (it was replaced by a summary).

Add a third sanitization pass that removes tool messages not preceded
by an assistant with tool_calls or a sibling tool message.

Fixes NousResearch#20883
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/file File tools (read, write, patch, search) platform/whatsapp WhatsApp Business adapter P1 High — major feature broken, no workaround labels May 6, 2026
@Bartok9

Bartok9 commented May 7, 2026

Copy link
Copy Markdown
Contributor

Reviewed the _sanitize_tool_pairs addition. The step-3 loop logic is correct:

  • last_assistant_had_calls starts False and is only True immediately after an assistant message with non-empty tool_calls
  • Consecutive tool messages from the same call group are kept via the filtered[-1].get('role') == 'tool' check
  • After the first tool in a group, last_assistant_had_calls flips back to False, but siblings are allowed through the role-chain check
  • A user or assistant-without-tool_calls in between resets the state, so an orphaned tool after such a boundary is correctly removed

One edge case worth noting: the existing step 1 (orphan removal by tool_call_id) runs before step 3. That means a tool message at the boundary also has its tool_call_id checked against surviving assistant tool_calls. If step 1 removes it, step 3 never sees it; if step 1 doesn't (because no call_ids are tracked), step 3 catches the ordering violation. No double-removal risk. ✓

@teknium1

Copy link
Copy Markdown
Contributor

This looks implemented on current main.

Automated hermes-sweeper review found that the compressor boundary failure described here is already fixed by the current boundary-alignment path:

  • agent/context_compressor.py:1708 defines _align_boundary_backward(), which walks backward across consecutive role: "tool" messages to the parent assistant with tool_calls and moves the boundary before the whole group.
  • agent/context_compressor.py:1881 applies that alignment before returning the tail cut used by compress(), so the compressed tail does not begin with a tool result whose parent assistant was summarized away.
  • tests/run_agent/test_compression_boundary.py:82 covers the boundary-in-the-middle-of-tool-results case, and the end-to-end test in the same file verifies no orphaned tool results remain after compression.
  • The implementation is present in d132e344d7b3710047aa8a0e82f7177510d5847d (fix(agent): prevent silent tool result loss during context compression (#1993)), included in v2026.3.23 and later.

I also checked the bundled non-compressor commits: the WhatsApp npm timeout change is already on main (gateway/platforms/whatsapp.py:601), and the patch-schema issue is handled on main via provider-safe per-mode descriptions plus tests (tests/tools/test_file_tools.py:468). Thanks for the detailed fix and the review discussion from @Bartok9; the specific compressor crash this PR targets is now covered on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround platform/whatsapp WhatsApp Business adapter sweeper:implemented-on-main Sweeper: behavior already present on current main tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants