fix(agent): prevent multi-tool group splitting at protection boundaries

[5queezer]

User description

Summary

Addresses CodeRabbit review feedback from PR #74 — four issues where multi-tool groups (assistant + N consecutive tool messages) could be split at protection/keep_recent boundaries, orphaning tool messages and causing provider API errors.

• Phase 1 collapse (history_pruner.rs): scan the full consecutive tool run before checking protection; only collapse when every message in the group is unprotected
• Phase 2 budget drop (history_pruner.rs): verify all tool messages in the group are unprotected before dropping; skip the entire group if any member is protected
• Orphan invariant assertions (history_pruner.rs tests): relax the check to allow tool preceded by tool (valid in multi-tool groups), not just preceded by assistant
• Emergency trim (history.rs): count the full tool run regardless of the keep_recent cutoff; only drop the group if it lies entirely before the protected suffix

Test plan

• All 11 existing history_pruner tests pass (including multi-tool-group and realistic-pressure scenarios)
• cargo fmt --all -- --check passes
• cargo clippy --lib -p hrafn -- -D warnings passes with zero warnings
• CI gate (checks-on-pr.yml) should pass

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved conversation history management to treat assistant-tool message exchanges as atomic units, ensuring complete groups are consistently retained or removed together during trimming and pruning operations.

---

PR Type

Bug fix, Enhancement

---

Description

• Phase 1 collapse checks full tool run protection

• Phase 2 budget drop skips partially protected groups

• Emergency trim handles atomic tool groups correctly

• Test assertions updated for multi-tool sequences

---

Diagram Walkthrough

flowchart LR
  Start["Assistant Message"] --> Tools["Consecutive Tool Messages"]
  Tools --> Check{"All Unprotected?"}
  Check -->|"Yes"| Action["Collapse or Drop Group"]
  Check -->|"No"| Skip["Skip Entire Group"]

Loading

File Walkthrough

Relevant files

Bug fix

history_pruner.rs Atomic handling for multi-tool pruning phases                        src/agent/history_pruner.rs Phase 1 collapse verifies full tool run protection Phase 2 budget drop skips partially protected groups Test assertions allow tool preceded by tool +28/-14  history.rs Atomic group awareness for emergency trim                                src/agent/history.rs emergency_history_trim counts full tool run regardless of cutoff Drops group only if entirely before protected suffix +14/-7

---

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated history trimming and pruning logic in the agent module to treat assistant messages and their consecutive tool message groups as atomic units. Removal or collapsing operations now process entire groups together, preventing partial removal when a group would overlap protection boundaries.

Changes

Cohort / File(s)	Summary
Emergency History Trimming src/agent/history.rs	Refined emergency_history_trim to count all consecutive tool messages following an assistant regardless of cutoff position, then remove the entire assistant+tool group only if it ends within the protected range; skips whole groups that cross protection boundaries.
History Pruning Logic src/agent/history_pruner.rs	Updated both phases to enforce atomic group semantics: phase 1 collapses tool results only if all messages in the consecutive tool run are unprotected; phase 2 removes assistant+tool groups only if all tools are unprotected, skipping entirely if the group crosses a protection boundary. Test assertions adjusted to reflect grouping invariants.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(agent): treat tool_use/tool_result as atomic groups in history pruning #74: Implements the same atomic group semantics for assistant+tool message handling in emergency history trimming and history pruning.

Suggested labels

agent

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the primary fix: preventing multi-tool group splitting at protection boundaries, which directly addresses the main issue across both modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The PR description is well-structured, comprehensive, and addresses all template sections with detailed explanations of changes, test results, and specific file modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/history-pruner-multi-tool-groups

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

src/agent/history.rs (1)

99-109: Add a direct straddling-boundary regression test.

This branch is the new behavior in emergency_history_trim, but the shown emergency_history_trim tests in src/agent/loop_.rs only cover fully droppable histories. A case where an assistant + tool... run overlaps keep_recent would lock in the new “skip, don’t split” rule.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/agent/history.rs` around lines 99 - 109, Add a regression test for
emergency_history_trim that covers the “straddling boundary” case: construct a
history with an assistant message followed by tool_count tool messages such that
the group partially overlaps the protected suffix (using history.len() and
keep_recent to compute cutoff), call emergency_history_trim, and assert the
group is left intact (no partial removal) and dropped counter reflects only
fully-droppable groups; locate and extend the existing emergency_history_trim
tests in loop_.rs and use the same helpers/setup, targeting the code paths
around emergency_history_trim, i, tool_count, and dropped to ensure the
skip-not-split behavior is enforced.

src/agent/history_pruner.rs (1)

132-134: Prefer drain for contiguous group removal.

Both sites are deleting a contiguous range from a fixed index, so repeated remove just shifts the tail N times and obscures the atomic-group intent.

♻️ Proposed cleanup

-                    for _ in 0..tool_count {
-                        messages.remove(i + 1);
-                    }
+                    drop(messages.drain(i + 1..=i + tool_count));
...
-                        for _ in 0..=tool_count {
-                            messages.remove(i);
-                        }
+                        drop(messages.drain(i..=i + tool_count));

Also applies to: 169-171

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/agent/history_pruner.rs` around lines 132 - 134, The loop that calls
messages.remove(i + 1) tool_count times should be replaced with a single
contiguous removal using messages.drain(start..end) to express atomic group
deletion; compute start = i + 1 and end = start + tool_count and call
messages.drain(start..end) (or drain_exact if available) instead of the repeated
removes. Apply the same change at the second site that uses the same pattern
(the block around the other occurrence referencing messages, tool_count, and i)
so both contiguous-group deletions use drain for clarity and efficiency.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/agent/history_pruner.rs`:
- Around line 132-134: The loop that calls messages.remove(i + 1) tool_count
times should be replaced with a single contiguous removal using
messages.drain(start..end) to express atomic group deletion; compute start = i +
1 and end = start + tool_count and call messages.drain(start..end) (or
drain_exact if available) instead of the repeated removes. Apply the same change
at the second site that uses the same pattern (the block around the other
occurrence referencing messages, tool_count, and i) so both contiguous-group
deletions use drain for clarity and efficiency.

In `@src/agent/history.rs`:
- Around line 99-109: Add a regression test for emergency_history_trim that
covers the “straddling boundary” case: construct a history with an assistant
message followed by tool_count tool messages such that the group partially
overlaps the protected suffix (using history.len() and keep_recent to compute
cutoff), call emergency_history_trim, and assert the group is left intact (no
partial removal) and dropped counter reflects only fully-droppable groups;
locate and extend the existing emergency_history_trim tests in loop_.rs and use
the same helpers/setup, targeting the code paths around emergency_history_trim,
i, tool_count, and dropped to ensure the skip-not-split behavior is enforced.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7d68f20b-61bb-45b7-b863-f51cd47b649f

📥 Commits

Reviewing files that changed from the base of the PR and between 86c3aef and 7a4ae8f.

📒 Files selected for processing (2)

• src/agent/history.rs
• src/agent/history_pruner.rs

[5queezer]

@claude fix the nitpicks

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7a4ae8f)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 74 - PR Code Verified Compliant requirements: emergency_history_trim correctly counts full tool runs and drops/skips groups atomically based on protection boundaries Logic prevents splitting multi-tool groups at keep_recent cutoff Requires further human verification: Execution of cargo test to confirm all 11 history_pruner tests pass Verification of history_pruner.rs changes (not fully visible in diff)

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[5queezer]

/review

[github-actions]

Persistent review updated to latest commit 7a4ae8f

[5queezer]

/review

[github-actions]

Persistent review updated to latest commit 7a4ae8f

[5queezer]

/review

[github-actions]

Persistent review updated to latest commit 7a4ae8f

[hrafn-pr-agent]

Persistent review updated to latest commit 7a4ae8f