From fa16535140058bd1a7ee1980f39dab5e28d3aa73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20Manuel=20Mart=C3=ADn?= Date: Fri, 17 Apr 2026 11:07:16 +0000 Subject: [PATCH] fix(loop): prevent compaction from splitting tool_call/tool_result pairs The compactor split messages purely by count, so an assistant message with tool_calls could end up in the summary while its matching Role::Tool responses stayed in recent (or vice versa). This produces histories with dangling tool_call_ids that providers reject. Walk the proposed split backward until no unanswered tool_call straddles the boundary. If no safe split exists above `start`, the caller's existing guard skips compaction entirely. Co-Authored-By: Claude Opus 4.6 (1M context) --- loop/src/compaction.rs | 176 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 174 insertions(+), 2 deletions(-) diff --git a/loop/src/compaction.rs b/loop/src/compaction.rs index 7651fce..26227a7 100644 --- a/loop/src/compaction.rs +++ b/loop/src/compaction.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use anyhow::Result; use tracing::info; @@ -26,7 +28,8 @@ pub async fn compact_messages( let system_msg = messages.first().filter(|m| m.role == Role::System).cloned(); let start = if system_msg.is_some() { 1 } else { 0 }; - let split_point = messages.len().saturating_sub(keep_recent); + let proposed = messages.len().saturating_sub(keep_recent); + let split_point = find_safe_split(messages, start, proposed); if split_point <= start { return Ok(messages.to_vec()); @@ -70,6 +73,43 @@ pub async fn compact_messages( Ok(result) } +/// Walk the proposed split backward until no tool_call in +/// `messages[start..split]` has its matching `Role::Tool` response +/// outside that range. Keeps assistant-with-tool_calls and their +/// responses together across the compaction boundary — providers +/// reject histories with dangling `tool_call_id`s. +fn find_safe_split(messages: &[Message], start: usize, proposed: usize) -> usize { + let mut split = proposed.min(messages.len()); + while split > start && has_straddling_pair(messages, start, split) { + split -= 1; + } + split +} + +/// True if any tool call issued by an assistant in `messages[start..split]` +/// has no matching `Role::Tool` response within the same slice. +fn has_straddling_pair(messages: &[Message], start: usize, split: usize) -> bool { + let mut unanswered: HashSet<&str> = HashSet::new(); + for msg in &messages[start..split] { + match msg.role { + Role::Assistant => { + if let Some(calls) = msg.tool_calls.as_ref() { + for c in calls { + unanswered.insert(c.id.as_str()); + } + } + } + Role::Tool => { + if let Some(id) = msg.tool_call_id.as_deref() { + unanswered.remove(id); + } + } + _ => {} + } + } + !unanswered.is_empty() +} + async fn summarize(llm: &(impl LlmService + ?Sized), messages: &[Message]) -> Result { let conversation_text = messages .iter() @@ -131,9 +171,56 @@ pub fn estimate_tokens(messages: &[Message]) -> usize { #[cfg(test)] mod tests { use super::*; - use crate::llm::{ContentBlock, TypedBlock}; + use crate::llm::{ContentBlock, FunctionCall, ToolCall, TypedBlock}; use serde_json::json; + fn user(text: &str) -> Message { + Message { + role: Role::User, + content: Some(Content::Text(text.into())), + tool_calls: None, + tool_call_id: None, + } + } + + fn assistant_text(text: &str) -> Message { + Message { + role: Role::Assistant, + content: Some(Content::Text(text.into())), + tool_calls: None, + tool_call_id: None, + } + } + + fn assistant_tools(ids: &[&str]) -> Message { + Message { + role: Role::Assistant, + content: None, + tool_calls: Some( + ids.iter() + .map(|id| ToolCall { + id: (*id).into(), + call_type: "function".into(), + function: FunctionCall { + name: "f".into(), + arguments: "{}".into(), + }, + }) + .collect(), + ), + tool_call_id: None, + } + } + + fn tool_result(id: &str) -> Message { + Message { + role: Role::Tool, + content: Some(Content::Text(format!("result-{id}"))), + tool_calls: None, + tool_call_id: Some(id.into()), + } + } + #[test] fn test_estimate_tokens_empty() { assert_eq!(estimate_tokens(&[]), 0); @@ -161,6 +248,91 @@ mod tests { assert_eq!(estimate_tokens(&[msg]), 0); } + #[test] + fn test_safe_split_no_tool_calls() { + let msgs = vec![ + user("q1"), + assistant_text("a1"), + user("q2"), + assistant_text("a2"), + user("q3"), + assistant_text("a3"), + ]; + // Proposed split anywhere is safe — no tool pairs to straddle. + assert_eq!(find_safe_split(&msgs, 0, 3), 3); + assert_eq!(find_safe_split(&msgs, 0, 5), 5); + } + + #[test] + fn test_safe_split_walks_past_tool_pair() { + // [U, A(tc=a), T(a), done] len=4 + let msgs = vec![ + user("q"), + assistant_tools(&["a"]), + tool_result("a"), + assistant_text("done"), + ]; + // proposed=2: to_compact=[U, A(tc=a)], recent=[T(a), done]. + // A(tc=a)'s response is in recent → unsafe. Walk back to 1: + // to_compact=[U], no tool calls → safe. + assert_eq!(find_safe_split(&msgs, 0, 2), 1); + // proposed=3 is already safe: both A(tc=a) and T(a) in to_compact. + assert_eq!(find_safe_split(&msgs, 0, 3), 3); + } + + #[test] + fn test_safe_split_multi_tool_chain() { + // [U, A(tc=[a,b]), T(a), T(b), done] len=5 + let msgs = vec![ + user("q"), + assistant_tools(&["a", "b"]), + tool_result("a"), + tool_result("b"), + assistant_text("done"), + ]; + // proposed=3: A(tc=[a,b]) in compact, T(b) in recent → unsafe. + // Walk back past A(tc=...) until to_compact has no unanswered + // tool calls. Lands at 1 ([U] only). + assert_eq!(find_safe_split(&msgs, 0, 3), 1); + // proposed=4: to_compact includes both tool responses → safe. + assert_eq!(find_safe_split(&msgs, 0, 4), 4); + } + + #[test] + fn test_safe_split_respects_start() { + // System prompt at index 0; tool pair straddles the only + // viable split — walk-back stops at `start` and returns it. + let msgs = vec![ + Message { + role: Role::System, + content: Some(Content::Text("sys".into())), + tool_calls: None, + tool_call_id: None, + }, + assistant_tools(&["a"]), + tool_result("a"), + ]; + // proposed=2 unsafe; walk-back to 1 unsafe (A still in compact); + // walk-back stops at start=1 and returns 1. + // (Caller guards `split <= start` and skips compaction.) + assert_eq!(find_safe_split(&msgs, 1, 2), 1); + } + + #[test] + fn test_safe_split_earlier_pair_unaffected() { + // An earlier completed tool pair shouldn't force walk-back if + // the proposed split lands cleanly. + // [U, A(tc=a), T(a), U', A''] split at 4 → recent=[A''] + let msgs = vec![ + user("q1"), + assistant_tools(&["a"]), + tool_result("a"), + user("q2"), + assistant_text("a2"), + ]; + assert_eq!(find_safe_split(&msgs, 0, 4), 4); + } + #[test] fn test_estimate_tokens_with_image_blocks() { let msg = Message {