Conversation
…uning Prevents orphaned tool_result messages from causing Anthropic 400 errors during tool-heavy conversations with low context budgets. Ported from zeroclaw-labs/zeroclaw#4825 by @singlerider. Original PR was closed without review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
History Management src/agent/history.rs |
Modified emergency_history_trim to drop assistant messages together with immediately following consecutive tool messages as atomic groups, preserving tool_use/tool_result adjacency. |
History Pruning Logic src/agent/history_pruner.rs |
Updated prune_history to treat assistant+consecutive tool messages as atomic groups in both tool collapsing (phase 1) and budget enforcement (phase 2) phases; adjusted tests to cover multi-tool collapsing and atomic group removal with invariants ensuring no orphaned tool messages. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description covers the key changes and test plan, but lacks the required 'What' section with issue closure, 'How to test' section with specific test commands, and the checklist items from the template. | Add a 'What' section linking the issue being closed, provide explicit 'How to test' instructions with cargo test commands, complete the checklist items, and indicate whether there are breaking changes or security implications. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately and concisely summarizes the main change: treating tool_use/tool_result as atomic groups in history pruning, which is the core objective of the PR. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ 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
port/zc-4825-atomic-tool-pruning
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/history_pruner.rs`:
- Around line 367-374: The loop enforcing "tool" messages must be immediately
preceded by an "assistant" is too strict for multi-tool groups; update the check
in the iterations over messages (the for (i, m) in messages.iter().enumerate()
loop and the analogous checks at the other spots noted) so that a "tool" is
allowed if the previous message is also a "tool" (i.e., only assert failure if i
== 0 or the previous message exists and is neither "assistant" nor "tool"); in
practice change the assertion to: if m.role == "tool" then ensure i > 0 and
(messages[i-1].role == "assistant" || messages[i-1].role == "tool"), otherwise
panic — apply the same logic to the other two loops referenced (lines ~401-409
and ~468-476) so multi-tool consecutive groups are accepted.
- Around line 153-167: The current assistant-group drop logic in the block
checking if messages[i].role == "assistant" removes the assistant plus all
following "tool" messages without checking the protected/keep_recent boundary;
change it so you only remove the group if every message in that assistant+tool
run has index >= protected (i.e., is unprotected). Concretely, compute the tool
run as you do with tool_count, then verify that i and i+1..i+tool_count are all
>= protected before removing any entries; if any message in that group is
protected, skip dropping the entire group (do not remove partial messages),
leaving messages, dropped_messages, and dropped_any unchanged for that group.
Reference symbols: messages, protected, tool_count, dropped_messages,
dropped_any in the assistant-role removal branch.
- Around line 113-133: The collapse logic currently counts only an unprotected
prefix of consecutive tool messages and then replaces the assistant with a
summary while leaving protected tool messages after that summary; change the
algorithm so you first scan the full consecutive tool run (starting at i+1) to
determine its length, then check whether every message in that run is
unprotected before performing the collapse; if any tool in the run is protected,
do not replace messages[i] or remove any messages in that run. Update the code
around variables/messages: messages[i], tool_count, protected, collapsed_pairs
and the ChatMessage replacement so collapsing happens only when the entire tool
run is unprotected.
In `@src/agent/history.rs`:
- Around line 92-103: The removal loop for assistant+tool groups can split a
tool-result group at the keep_recent cutoff; to fix, compute the cutoff as
history.len().saturating_sub(keep_recent) and only count/remove the assistant
plus trailing tool messages if the entire group lies strictly before that
cutoff: in the block using variables history, i, tool_count, keep_recent and
dropped, advance tool_count as you do but then check if (i + 1 + tool_count) <=
cutoff (or that the last tool index < cutoff) before performing the for-loop of
history.remove; if the group crosses into the protected suffix, skip removal
(and advance i appropriately) so you never orphan the protected suffix or
increment dropped for a partial group.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a802f5bc-7e1b-4e24-b445-343ffc9f1036
📒 Files selected for processing (2)
src/agent/history.rssrc/agent/history_pruner.rs
Comment on lines
+113
to
+133
| if messages[i].role == "assistant" && !protected[i] { | ||
| // Count consecutive tool messages following this assistant | ||
| let mut tool_count = 0; | ||
| while i + 1 + tool_count < messages.len() | ||
| && messages[i + 1 + tool_count].role == "tool" | ||
| && !protected[i + 1 + tool_count] | ||
| { | ||
| tool_count += 1; | ||
| } | ||
| if tool_count > 0 { | ||
| let summary = | ||
| format!("[Tool exchange: {tool_count} tool call(s) — results collapsed]"); | ||
| messages[i] = ChatMessage { | ||
| role: "assistant".to_string(), | ||
| content: summary, | ||
| }; | ||
| for _ in 0..tool_count { | ||
| messages.remove(i + 1); | ||
| } | ||
| collapsed_pairs += tool_count; | ||
| continue; |
Contributor
There was a problem hiding this comment.
Avoid partial collapse when a multi-tool group straddles keep_recent.
Line 116 only counts unprotected tools. If the cutoff lands inside a multi-tool run, Lines 123-131 replace the original assistant with a summary and remove only the unprotected prefix. The remaining protected tool message(s) now follow a synthetic summary instead of the original tool-call payload, which can still violate provider pairing rules.
🛠️ Suggested fix
if messages[i].role == "assistant" && !protected[i] {
- // Count consecutive tool messages following this assistant
- let mut tool_count = 0;
- while i + 1 + tool_count < messages.len()
- && messages[i + 1 + tool_count].role == "tool"
- && !protected[i + 1 + tool_count]
- {
- tool_count += 1;
- }
- if tool_count > 0 {
+ let mut group_end = i + 1;
+ while group_end < messages.len() && messages[group_end].role == "tool" {
+ group_end += 1;
+ }
+
+ if group_end > i + 1 {
+ if protected[i + 1..group_end].iter().any(|p| *p) {
+ i = group_end;
+ continue;
+ }
+
+ let tool_count = group_end - i - 1;
let summary =
format!("[Tool exchange: {tool_count} tool call(s) — results collapsed]");
messages[i] = ChatMessage {
role: "assistant".to_string(),
content: summary,
};
- for _ in 0..tool_count {
- messages.remove(i + 1);
- }
+ drop(messages.drain(i + 1..group_end));
collapsed_pairs += tool_count;
continue;
}
}🤖 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 113 - 133, The collapse logic
currently counts only an unprotected prefix of consecutive tool messages and
then replaces the assistant with a summary while leaving protected tool messages
after that summary; change the algorithm so you first scan the full consecutive
tool run (starting at i+1) to determine its length, then check whether every
message in that run is unprotected before performing the collapse; if any tool
in the run is protected, do not replace messages[i] or remove any messages in
that run. Update the code around variables/messages: messages[i], tool_count,
protected, collapsed_pairs and the ChatMessage replacement so collapsing happens
only when the entire tool run is unprotected.
Comment on lines
+153
to
+167
| if messages[i].role == "assistant" { | ||
| // Count following tool messages — drop as atomic group | ||
| let mut tool_count = 0; | ||
| while i + 1 + tool_count < messages.len() | ||
| && messages[i + 1 + tool_count].role == "tool" | ||
| { | ||
| tool_count += 1; | ||
| } | ||
| if tool_count > 0 { | ||
| for _ in 0..=tool_count { | ||
| messages.remove(i); | ||
| } | ||
| dropped_messages += 1 + tool_count; | ||
| dropped_any = true; | ||
| break; |
Contributor
There was a problem hiding this comment.
Don't let group dropping override keep_recent.
Lines 156-164 remove every consecutive tool after an unprotected assistant without consulting protected. If the recent window starts inside that run, Phase 2 deletes protected tool results just to keep the group atomic, which breaks the keep_recent contract.
🛠️ Suggested fix
if messages[i].role == "assistant" {
- // Count following tool messages — drop as atomic group
- let mut tool_count = 0;
- while i + 1 + tool_count < messages.len()
- && messages[i + 1 + tool_count].role == "tool"
- {
- tool_count += 1;
- }
- if tool_count > 0 {
- for _ in 0..=tool_count {
- messages.remove(i);
- }
- dropped_messages += 1 + tool_count;
+ let mut group_end = i + 1;
+ while group_end < messages.len() && messages[group_end].role == "tool" {
+ group_end += 1;
+ }
+ if group_end > i + 1 {
+ if protected[i + 1..group_end].iter().any(|p| *p) {
+ i = group_end;
+ continue;
+ }
+
+ dropped_messages += group_end - i;
+ drop(messages.drain(i..group_end));
dropped_any = true;
break;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if messages[i].role == "assistant" { | |
| // Count following tool messages — drop as atomic group | |
| let mut tool_count = 0; | |
| while i + 1 + tool_count < messages.len() | |
| && messages[i + 1 + tool_count].role == "tool" | |
| { | |
| tool_count += 1; | |
| } | |
| if tool_count > 0 { | |
| for _ in 0..=tool_count { | |
| messages.remove(i); | |
| } | |
| dropped_messages += 1 + tool_count; | |
| dropped_any = true; | |
| break; | |
| if messages[i].role == "assistant" { | |
| let mut group_end = i + 1; | |
| while group_end < messages.len() && messages[group_end].role == "tool" { | |
| group_end += 1; | |
| } | |
| if group_end > i + 1 { | |
| if protected[i + 1..group_end].iter().any(|p| *p) { | |
| i = group_end; | |
| continue; | |
| } | |
| dropped_messages += group_end - i; | |
| drop(messages.drain(i..group_end)); | |
| dropped_any = true; | |
| break; | |
| } | |
| } |
🤖 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 153 - 167, The current
assistant-group drop logic in the block checking if messages[i].role ==
"assistant" removes the assistant plus all following "tool" messages without
checking the protected/keep_recent boundary; change it so you only remove the
group if every message in that assistant+tool run has index >= protected (i.e.,
is unprotected). Concretely, compute the tool run as you do with tool_count,
then verify that i and i+1..i+tool_count are all >= protected before removing
any entries; if any message in that group is protected, skip dropping the entire
group (do not remove partial messages), leaving messages, dropped_messages, and
dropped_any unchanged for that group. Reference symbols: messages, protected,
tool_count, dropped_messages, dropped_any in the assistant-role removal branch.
Comment on lines
+367
to
+374
| for (i, m) in messages.iter().enumerate() { | ||
| if m.role == "tool" { | ||
| assert!( | ||
| i > 0 && messages[i - 1].role == "assistant", | ||
| "tool message at index {i} has no preceding assistant" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The orphan invariant is too strict for multi-tool groups.
This PR explicitly supports assistant + N consecutive tool exchanges, but these loops require every remaining tool message to be immediately preceded by assistant. That will fail on a valid protected multi-tool group and makes it impossible to add the boundary-straddle regression that would catch the current keep_recent bug.
Also applies to: 401-409, 468-476
🤖 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 367 - 374, The loop enforcing
"tool" messages must be immediately preceded by an "assistant" is too strict for
multi-tool groups; update the check in the iterations over messages (the for (i,
m) in messages.iter().enumerate() loop and the analogous checks at the other
spots noted) so that a "tool" is allowed if the previous message is also a
"tool" (i.e., only assert failure if i == 0 or the previous message exists and
is neither "assistant" nor "tool"); in practice change the assertion to: if
m.role == "tool" then ensure i > 0 and (messages[i-1].role == "assistant" ||
messages[i-1].role == "tool"), otherwise panic — apply the same logic to the
other two loops referenced (lines ~401-409 and ~468-476) so multi-tool
consecutive groups are accepted.
Comment on lines
+92
to
+103
| } else if history[i].role == "assistant" { | ||
| // Count following tool messages — drop as atomic group | ||
| let mut tool_count = 0; | ||
| while i + 1 + tool_count < history.len().saturating_sub(keep_recent) | ||
| && history[i + 1 + tool_count].role == "tool" | ||
| { | ||
| tool_count += 1; | ||
| } | ||
| for _ in 0..=tool_count { | ||
| history.remove(i); | ||
| dropped += 1; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't split a tool group at the keep_recent boundary.
When keep_recent starts between two tool results, Line 95 stops counting at the cutoff but Lines 100-103 still remove the assistant and the unprotected prefix. That leaves the protected suffix orphaned, and src/agent/loop_.rs:2800-2840 retries immediately on dropped > 0, so the overflow-recovery path can still resend invalid history.
🛠️ Suggested fix
} else if history[i].role == "assistant" {
- // Count following tool messages — drop as atomic group
- let mut tool_count = 0;
- while i + 1 + tool_count < history.len().saturating_sub(keep_recent)
- && history[i + 1 + tool_count].role == "tool"
- {
- tool_count += 1;
- }
- for _ in 0..=tool_count {
- history.remove(i);
- dropped += 1;
- }
+ let mut group_end = i + 1;
+ while group_end < history.len() && history[group_end].role == "tool" {
+ group_end += 1;
+ }
+
+ let recent_start = history.len().saturating_sub(keep_recent);
+ if group_end > recent_start {
+ i = group_end;
+ continue;
+ }
+
+ dropped += group_end - i;
+ drop(history.drain(i..group_end));
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/history.rs` around lines 92 - 103, The removal loop for
assistant+tool groups can split a tool-result group at the keep_recent cutoff;
to fix, compute the cutoff as history.len().saturating_sub(keep_recent) and only
count/remove the assistant plus trailing tool messages if the entire group lies
strictly before that cutoff: in the block using variables history, i,
tool_count, keep_recent and dropped, advance tool_count as you do but then check
if (i + 1 + tool_count) <= cutoff (or that the last tool index < cutoff) before
performing the for-loop of history.remove; if the group crosses into the
protected suffix, skip removal (and advance i appropriately) so you never orphan
the protected suffix or increment dropped for a partial group.
4 tasks
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
1 participant
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.
Summary
emergency_history_triminhistory.rsgains the same atomic-group awarenessUpstream reference
history.rsandhistory_pruner.rsshare the same codebase structure as zeroclaw. Only the test assertion content format string was adapted (uses em-dash in summary format).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit