mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
fix(worker): prevent orphaned tool_results and fix parallel merging (#1069)
* fix(worker): prevent orphaned tool_results and fix parallel merging Two fixes for tool result handling in the Worker: 1. Preserve reasoning text from select_tools() in the RespondResult content field so it appears in the assistant_with_tool_calls message pushed by execute_tool_calls. Without this, the LLM's reasoning context was lost when using the select_tools path. 2. Merge consecutive tool_result messages into a single User message in rig_adapter's convert_messages(). When parallel tools execute, each produces a separate ChatMessage with role: Tool. Without merging, these become consecutive User messages which Anthropic rejects. Now consecutive tool results are merged into one User message with multiple ToolResult content items. Includes regression tests for both fixes. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(worker): use find_map for first non-empty reasoning extraction The previous code only checked the first ToolSelection's reasoning, missing cases where the first selection has empty reasoning but subsequent ones do not. Switch to find_map to get the first non-empty reasoning across all selections. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
f618166ad8
commit
58a3eb1366
+87
-7
@@ -357,15 +357,31 @@ fn convert_messages(messages: &[ChatMessage]) -> (Option<String>, Vec<RigMessage
|
||||
}
|
||||
}
|
||||
crate::llm::Role::Tool => {
|
||||
// Tool result message: wrap as User { ToolResult }
|
||||
// Tool result message: wrap as User { ToolResult }.
|
||||
// Merge consecutive tool results into a single User message
|
||||
// so the API sees one multi-result message instead of
|
||||
// multiple consecutive User messages (which Anthropic rejects).
|
||||
let tool_id = normalized_tool_call_id(msg.tool_call_id.as_deref(), history.len());
|
||||
history.push(RigMessage::User {
|
||||
content: OneOrMany::one(UserContent::ToolResult(RigToolResult {
|
||||
id: tool_id.clone(),
|
||||
call_id: Some(tool_id),
|
||||
content: OneOrMany::one(ToolResultContent::text(&msg.content)),
|
||||
})),
|
||||
let tool_result = UserContent::ToolResult(RigToolResult {
|
||||
id: tool_id.clone(),
|
||||
call_id: Some(tool_id),
|
||||
content: OneOrMany::one(ToolResultContent::text(&msg.content)),
|
||||
});
|
||||
|
||||
let should_merge = matches!(
|
||||
history.last(),
|
||||
Some(RigMessage::User { content }) if content.iter().all(|c| matches!(c, UserContent::ToolResult(_)))
|
||||
);
|
||||
|
||||
if should_merge {
|
||||
if let Some(RigMessage::User { content }) = history.last_mut() {
|
||||
content.push(tool_result);
|
||||
}
|
||||
} else {
|
||||
history.push(RigMessage::User {
|
||||
content: OneOrMany::one(tool_result),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1280,4 +1296,68 @@ mod tests {
|
||||
|
||||
assert!(adapter.unsupported_params.is_empty());
|
||||
}
|
||||
|
||||
/// Regression test: consecutive tool_result messages from parallel tool
|
||||
/// execution must be merged into a single User message with multiple
|
||||
/// ToolResult content items. Without merging, APIs like Anthropic reject
|
||||
/// the request due to consecutive User messages.
|
||||
#[test]
|
||||
fn test_consecutive_tool_results_merged_into_single_user_message() {
|
||||
let tc1 = IronToolCall {
|
||||
id: "call_a".to_string(),
|
||||
name: "search".to_string(),
|
||||
arguments: serde_json::json!({"q": "rust"}),
|
||||
};
|
||||
let tc2 = IronToolCall {
|
||||
id: "call_b".to_string(),
|
||||
name: "fetch".to_string(),
|
||||
arguments: serde_json::json!({"url": "https://example.com"}),
|
||||
};
|
||||
let assistant = ChatMessage::assistant_with_tool_calls(None, vec![tc1, tc2]);
|
||||
let result_a = ChatMessage::tool_result("call_a", "search", "search results");
|
||||
let result_b = ChatMessage::tool_result("call_b", "fetch", "fetch results");
|
||||
|
||||
let messages = vec![assistant, result_a, result_b];
|
||||
let (_preamble, history) = convert_messages(&messages);
|
||||
|
||||
// Should be: 1 assistant + 1 merged user (not 1 assistant + 2 users)
|
||||
assert_eq!(
|
||||
history.len(),
|
||||
2,
|
||||
"Expected 2 messages (assistant + merged user), got {}",
|
||||
history.len()
|
||||
);
|
||||
|
||||
// The second message should contain both tool results
|
||||
match &history[1] {
|
||||
RigMessage::User { content } => {
|
||||
assert_eq!(
|
||||
content.len(),
|
||||
2,
|
||||
"Expected 2 tool results in merged user message, got {}",
|
||||
content.len()
|
||||
);
|
||||
for item in content.iter() {
|
||||
assert!(
|
||||
matches!(item, UserContent::ToolResult(_)),
|
||||
"Expected ToolResult content"
|
||||
);
|
||||
}
|
||||
}
|
||||
other => panic!("Expected User message, got: {:?}", other),
|
||||
}
|
||||
}
|
||||
|
||||
/// Verify that a tool_result after a non-tool User message is NOT merged.
|
||||
#[test]
|
||||
fn test_tool_result_after_user_text_not_merged() {
|
||||
let user_msg = ChatMessage::user("hello");
|
||||
let tool_msg = ChatMessage::tool_result("call_1", "search", "results");
|
||||
|
||||
let messages = vec![user_msg, tool_msg];
|
||||
let (_preamble, history) = convert_messages(&messages);
|
||||
|
||||
// Should be 2 separate User messages (text user + tool result user)
|
||||
assert_eq!(history.len(), 2);
|
||||
}
|
||||
}
|
||||
|
||||
+130
-1
@@ -1170,11 +1170,16 @@ impl<'a> LoopDelegate for JobDelegate<'a> {
|
||||
// Reset counter after a successful LLM call
|
||||
self.consecutive_rate_limits
|
||||
.store(0, std::sync::atomic::Ordering::Relaxed);
|
||||
// Preserve the LLM's reasoning text so it appears in the
|
||||
// assistant_with_tool_calls message pushed by execute_tool_calls.
|
||||
let reasoning_text = s
|
||||
.iter()
|
||||
.find_map(|sel| (!sel.reasoning.is_empty()).then_some(sel.reasoning.clone()));
|
||||
let tool_calls: Vec<ToolCall> = selections_to_tool_calls(&s);
|
||||
return Ok(crate::llm::RespondOutput {
|
||||
result: RespondResult::ToolCalls {
|
||||
tool_calls,
|
||||
content: None,
|
||||
content: reasoning_text,
|
||||
},
|
||||
usage: crate::llm::TokenUsage::default(),
|
||||
});
|
||||
@@ -1849,4 +1854,128 @@ mod tests {
|
||||
"Iteration cap should transition to Failed, not Stuck"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression test: selections_to_tool_calls must preserve tool_call_id
|
||||
/// so that tool_result messages match the assistant_with_tool_calls message
|
||||
/// and are not treated as orphaned by sanitize_tool_messages.
|
||||
#[test]
|
||||
fn test_selections_to_tool_calls_preserves_ids() {
|
||||
let selections = vec![
|
||||
ToolSelection {
|
||||
tool_name: "search".into(),
|
||||
parameters: serde_json::json!({"q": "test"}),
|
||||
reasoning: "Need to search".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_abc".into(),
|
||||
},
|
||||
ToolSelection {
|
||||
tool_name: "fetch".into(),
|
||||
parameters: serde_json::json!({"url": "https://example.com"}),
|
||||
reasoning: "Need to fetch".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_def".into(),
|
||||
},
|
||||
];
|
||||
|
||||
let tool_calls = selections_to_tool_calls(&selections);
|
||||
|
||||
assert_eq!(tool_calls.len(), 2);
|
||||
assert_eq!(tool_calls[0].id, "call_abc");
|
||||
assert_eq!(tool_calls[0].name, "search");
|
||||
assert_eq!(tool_calls[1].id, "call_def");
|
||||
assert_eq!(tool_calls[1].name, "fetch");
|
||||
}
|
||||
|
||||
/// Regression test: when select_tools returns selections with reasoning,
|
||||
/// the reasoning text should be preserved as content in the RespondResult
|
||||
/// so it appears in the assistant_with_tool_calls message. Without this,
|
||||
/// the LLM's reasoning context is lost and subsequent turns lack context.
|
||||
#[test]
|
||||
fn test_reasoning_text_extraction_from_selections() {
|
||||
// Simulate what call_llm does: extract first non-empty reasoning
|
||||
let selections = [
|
||||
ToolSelection {
|
||||
tool_name: "search".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: "I need to search for relevant information".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_1".into(),
|
||||
},
|
||||
ToolSelection {
|
||||
tool_name: "fetch".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: "I need to search for relevant information".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_2".into(),
|
||||
},
|
||||
];
|
||||
|
||||
let reasoning_text = selections
|
||||
.iter()
|
||||
.find_map(|sel| (!sel.reasoning.is_empty()).then_some(sel.reasoning.clone()));
|
||||
|
||||
assert_eq!(
|
||||
reasoning_text.as_deref(),
|
||||
Some("I need to search for relevant information"),
|
||||
"Reasoning text should be extracted from first non-empty selection"
|
||||
);
|
||||
|
||||
// Empty reasoning should result in None
|
||||
let empty_selections = [ToolSelection {
|
||||
tool_name: "echo".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: String::new(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_3".into(),
|
||||
}];
|
||||
|
||||
let empty_reasoning = empty_selections
|
||||
.iter()
|
||||
.find_map(|sel| (!sel.reasoning.is_empty()).then_some(sel.reasoning.clone()));
|
||||
|
||||
assert!(
|
||||
empty_reasoning.is_none(),
|
||||
"Empty reasoning should not be included as content"
|
||||
);
|
||||
}
|
||||
|
||||
/// When the first selection has empty reasoning but a subsequent one has
|
||||
/// non-empty reasoning, find_map should skip the empty one and return the
|
||||
/// first non-empty reasoning.
|
||||
#[test]
|
||||
fn test_reasoning_text_skips_empty_first_selection() {
|
||||
let selections = [
|
||||
ToolSelection {
|
||||
tool_name: "echo".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: String::new(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_1".into(),
|
||||
},
|
||||
ToolSelection {
|
||||
tool_name: "search".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: "Found the answer in the second selection".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_2".into(),
|
||||
},
|
||||
ToolSelection {
|
||||
tool_name: "fetch".into(),
|
||||
parameters: serde_json::json!({}),
|
||||
reasoning: "Third selection reasoning".into(),
|
||||
alternatives: vec![],
|
||||
tool_call_id: "call_3".into(),
|
||||
},
|
||||
];
|
||||
|
||||
let reasoning_text = selections
|
||||
.iter()
|
||||
.find_map(|sel| (!sel.reasoning.is_empty()).then_some(sel.reasoning.clone()));
|
||||
|
||||
assert_eq!(
|
||||
reasoning_text.as_deref(),
|
||||
Some("Found the answer in the second selection"),
|
||||
"Should skip empty first reasoning and return the first non-empty one"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user