feat(tests): 增加工具调用序列清理的单元测试,确保孤立工具结果的正确处理
This commit is contained in:
parent
416ac047e3
commit
97c0e46348
@ -2259,6 +2259,69 @@ mod tests {
|
|||||||
assert_eq!(messages[4].content, "task 2");
|
assert_eq!(messages[4].content, "task 2");
|
||||||
assert_eq!(messages[5].content, "task 3");
|
assert_eq!(messages[5].content, "task 3");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_sanitize_removes_orphaned_tool_results_without_any_assistant_tool_calls() {
|
||||||
|
// After heavy compaction, all assistant tool_calls messages may be
|
||||||
|
// summarized into plain text, leaving only orphaned tool results.
|
||||||
|
// Phase 2 must still clean them even when no assistant has tool_calls.
|
||||||
|
let mut messages = vec![
|
||||||
|
ChatMessage::user("do something"),
|
||||||
|
ChatMessage::assistant("I used a tool to help you"), // plain text, no tool_calls
|
||||||
|
ChatMessage::tool("orphan_call", "search", "some result"), // orphaned!
|
||||||
|
ChatMessage::user("thanks"),
|
||||||
|
ChatMessage::assistant("you're welcome"),
|
||||||
|
];
|
||||||
|
|
||||||
|
let removed = crate::bus::message::sanitize_incomplete_tool_call_sequences(&mut messages);
|
||||||
|
assert_eq!(removed, 1, "should remove the orphaned tool result");
|
||||||
|
assert_eq!(messages.len(), 4);
|
||||||
|
assert_eq!(messages[0].role, "user");
|
||||||
|
assert_eq!(messages[1].role, "assistant");
|
||||||
|
assert_eq!(messages[2].role, "user");
|
||||||
|
assert_eq!(messages[2].content, "thanks");
|
||||||
|
assert_eq!(messages[3].role, "assistant");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_sanitize_handles_empty_tool_calls_vector() {
|
||||||
|
// An assistant message with tool_calls: Some(vec![]) — edge case
|
||||||
|
// that shouldn't crash or cause API errors.
|
||||||
|
let mut msg = ChatMessage::assistant("thinking...");
|
||||||
|
msg.tool_calls = Some(vec![]); // Some but empty
|
||||||
|
|
||||||
|
let mut messages = vec![
|
||||||
|
ChatMessage::user("hello"),
|
||||||
|
msg,
|
||||||
|
ChatMessage::assistant("here is my answer"),
|
||||||
|
];
|
||||||
|
|
||||||
|
let removed = crate::bus::message::sanitize_incomplete_tool_call_sequences(&mut messages);
|
||||||
|
assert_eq!(removed, 0, "empty tool_calls vec should not be removed");
|
||||||
|
assert_eq!(messages.len(), 3);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_sanitize_removes_multiple_orphaned_tool_results_after_compaction() {
|
||||||
|
// Simulates a scenario where compaction summarized all tool call
|
||||||
|
// sequences, leaving multiple orphaned tool results scattered.
|
||||||
|
let mut messages = vec![
|
||||||
|
ChatMessage::user("task 1"),
|
||||||
|
ChatMessage::assistant("completed task 1"), // summary text, no tool_calls
|
||||||
|
ChatMessage::tool("old_call_1", "read", "file contents"), // orphaned
|
||||||
|
ChatMessage::user("task 2"),
|
||||||
|
ChatMessage::assistant("completed task 2"), // summary text
|
||||||
|
ChatMessage::tool("old_call_2", "bash", "command output"), // orphaned
|
||||||
|
ChatMessage::user("task 3"),
|
||||||
|
ChatMessage::assistant("final answer"),
|
||||||
|
];
|
||||||
|
|
||||||
|
let removed = crate::bus::message::sanitize_incomplete_tool_call_sequences(&mut messages);
|
||||||
|
assert_eq!(removed, 2, "should remove both orphaned tool results");
|
||||||
|
assert_eq!(messages.len(), 6);
|
||||||
|
// Verify no tool messages remain
|
||||||
|
assert!(messages.iter().all(|m| m.role != "tool"));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
|
|||||||
@ -325,7 +325,12 @@ pub(crate) fn sanitize_incomplete_tool_call_sequences(messages: &mut Vec<ChatMes
|
|||||||
// Phase 2: Forward pass to remove ALL orphaned tool messages (not just
|
// Phase 2: Forward pass to remove ALL orphaned tool messages (not just
|
||||||
// trailing ones). A tool message is orphaned if its tool_call_id has no
|
// trailing ones). A tool message is orphaned if its tool_call_id has no
|
||||||
// matching parent assistant remaining in the history.
|
// matching parent assistant remaining in the history.
|
||||||
if !with_parent.is_empty() || !resolved_ids.is_empty() {
|
//
|
||||||
|
// Always execute this pass unconditionally — even when Phase 1 found no
|
||||||
|
// assistant messages with tool_calls (e.g., after heavy compaction that
|
||||||
|
// summarized all tool_call sequences into text), there may still be
|
||||||
|
// orphaned tool result messages in the history that must be cleaned up.
|
||||||
|
{
|
||||||
let mut i = 0;
|
let mut i = 0;
|
||||||
while i < messages.len() {
|
while i < messages.len() {
|
||||||
let msg = &messages[i];
|
let msg = &messages[i];
|
||||||
|
|||||||
@ -219,7 +219,6 @@ impl Default for InChatCommandRouter {
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
use uuid::Uuid;
|
|
||||||
|
|
||||||
struct TestHandler;
|
struct TestHandler;
|
||||||
|
|
||||||
|
|||||||
@ -112,12 +112,6 @@ impl SaveSessionCommandHandler {
|
|||||||
system_prompt_provider,
|
system_prompt_provider,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// 从会话记录获取存储(用于测试)
|
|
||||||
#[cfg(test)]
|
|
||||||
fn store(&self) -> &Arc<SessionStore> {
|
|
||||||
&self.store
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[async_trait]
|
#[async_trait]
|
||||||
|
|||||||
@ -103,7 +103,6 @@ impl SystemPromptProvider for SimpleAgentPromptProvider {
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
use crate::storage::SessionStore;
|
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
|
||||||
fn test_config() -> LLMProviderConfig {
|
fn test_config() -> LLMProviderConfig {
|
||||||
|
|||||||
@ -13,6 +13,7 @@ impl CliSessionService {
|
|||||||
Self { store }
|
Self { store }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[allow(dead_code)]
|
||||||
pub(crate) fn create(&self, title: Option<&str>) -> Result<SessionRecord, AgentError> {
|
pub(crate) fn create(&self, title: Option<&str>) -> Result<SessionRecord, AgentError> {
|
||||||
self.store
|
self.store
|
||||||
.create_cli_session(title)
|
.create_cli_session(title)
|
||||||
|
|||||||
@ -270,7 +270,7 @@ impl MemoryMaintenanceService {
|
|||||||
)))
|
)))
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg_attr(not(test), allow(dead_code))]
|
#[allow(dead_code)]
|
||||||
pub(crate) async fn run_for_scope(
|
pub(crate) async fn run_for_scope(
|
||||||
&self,
|
&self,
|
||||||
scope_key: &str,
|
scope_key: &str,
|
||||||
|
|||||||
@ -93,6 +93,7 @@ impl ToolRegistryFactory {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Get a reference to the shell session manager for lifecycle control.
|
/// Get a reference to the shell session manager for lifecycle control.
|
||||||
|
#[allow(dead_code)]
|
||||||
pub(crate) fn shell_session_manager(&self) -> Arc<ShellSessionManager> {
|
pub(crate) fn shell_session_manager(&self) -> Arc<ShellSessionManager> {
|
||||||
self.shell_session_manager.clone()
|
self.shell_session_manager.clone()
|
||||||
}
|
}
|
||||||
|
|||||||
@ -631,22 +631,89 @@ impl OpenAIProvider {
|
|||||||
|
|
||||||
fn build_request_body(&self, request: &ChatCompletionRequest) -> Value {
|
fn build_request_body(&self, request: &ChatCompletionRequest) -> Value {
|
||||||
let supports_images = self.supports_images();
|
let supports_images = self.supports_images();
|
||||||
|
|
||||||
|
// --- Final defense: validate tool_call / tool result pairing ---
|
||||||
|
// Collect all tool_call_ids that have a corresponding tool result message.
|
||||||
|
// Any assistant tool_call NOT in this set is orphaned and must be stripped
|
||||||
|
// to avoid API 400 errors ("insufficient tool messages following tool_calls").
|
||||||
|
let mut resolved_tool_ids: std::collections::HashSet<&str> = std::collections::HashSet::new();
|
||||||
|
for m in &request.messages {
|
||||||
|
if m.role == "tool" {
|
||||||
|
if let Some(ref tc_id) = m.tool_call_id {
|
||||||
|
resolved_tool_ids.insert(tc_id.as_str());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Build the set of assistant tool_call_ids that are fully valid
|
||||||
|
// (ALL tool_calls in the message have corresponding results).
|
||||||
|
// If an assistant has partial or no valid tool_calls, we strip the
|
||||||
|
// tool_calls field and serialize it as a plain assistant message.
|
||||||
|
let mut valid_tool_call_parent_ids: std::collections::HashSet<&str> = std::collections::HashSet::new();
|
||||||
|
for m in &request.messages {
|
||||||
|
if m.role == "assistant" {
|
||||||
|
if let Some(ref calls) = m.tool_calls {
|
||||||
|
if !calls.is_empty()
|
||||||
|
&& calls.iter().all(|tc| resolved_tool_ids.contains(tc.id.as_str()))
|
||||||
|
{
|
||||||
|
for tc in calls {
|
||||||
|
valid_tool_call_parent_ids.insert(tc.id.as_str());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let mut body = json!({
|
let mut body = json!({
|
||||||
"model": self.model_id,
|
"model": self.model_id,
|
||||||
"messages": request.messages.iter().enumerate().map(|(i, m)| {
|
"messages": request.messages.iter().enumerate().filter_map(|(i, m)| {
|
||||||
if m.role == "tool" {
|
if m.role == "tool" {
|
||||||
json!({
|
// Skip orphaned tool results (no matching assistant tool_call)
|
||||||
|
let is_orphaned = match &m.tool_call_id {
|
||||||
|
Some(tc_id) => !valid_tool_call_parent_ids.contains(tc_id.as_str()),
|
||||||
|
None => true,
|
||||||
|
};
|
||||||
|
if is_orphaned {
|
||||||
|
tracing::warn!(
|
||||||
|
tool_call_id = ?m.tool_call_id,
|
||||||
|
message_index = i,
|
||||||
|
"build_request_body: skipping orphaned tool result message"
|
||||||
|
);
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
Some(json!({
|
||||||
"role": m.role,
|
"role": m.role,
|
||||||
"content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i),
|
"content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i),
|
||||||
"tool_call_id": m.tool_call_id,
|
"tool_call_id": m.tool_call_id,
|
||||||
"name": m.name,
|
"name": m.name,
|
||||||
})
|
}))
|
||||||
} else if m.role == "assistant" && m.tool_calls.is_some() {
|
} else if m.role == "assistant" && m.tool_calls.is_some() {
|
||||||
let mut message = json!({
|
let calls = m.tool_calls.as_ref().unwrap();
|
||||||
"role": m.role,
|
// Filter to only valid tool_calls (all have results)
|
||||||
"content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i),
|
let valid_calls: Vec<&ToolCall> = calls.iter()
|
||||||
"tool_calls": m.tool_calls.as_ref().map(|calls| {
|
.filter(|tc| valid_tool_call_parent_ids.contains(tc.id.as_str()))
|
||||||
calls.iter().map(|call| json!({
|
.collect();
|
||||||
|
|
||||||
|
if valid_calls.is_empty() {
|
||||||
|
// All tool_calls are orphaned — serialize as plain assistant message
|
||||||
|
tracing::warn!(
|
||||||
|
orphaned_tool_call_count = calls.len(),
|
||||||
|
message_index = i,
|
||||||
|
"build_request_body: stripping all orphaned tool_calls from assistant message"
|
||||||
|
);
|
||||||
|
let mut message = json!({
|
||||||
|
"role": m.role,
|
||||||
|
"content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i)
|
||||||
|
});
|
||||||
|
if let Some(reasoning_content) = &m.reasoning_content {
|
||||||
|
message["reasoning_content"] = Value::String(reasoning_content.clone());
|
||||||
|
}
|
||||||
|
Some(message)
|
||||||
|
} else {
|
||||||
|
let mut message = json!({
|
||||||
|
"role": m.role,
|
||||||
|
"content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i),
|
||||||
|
"tool_calls": valid_calls.iter().map(|call| json!({
|
||||||
"id": call.id,
|
"id": call.id,
|
||||||
"type": "function",
|
"type": "function",
|
||||||
"function": {
|
"function": {
|
||||||
@ -654,14 +721,14 @@ impl OpenAIProvider {
|
|||||||
"arguments": self.serialize_tool_arguments(&call.arguments)
|
"arguments": self.serialize_tool_arguments(&call.arguments)
|
||||||
}
|
}
|
||||||
})).collect::<Vec<_>>()
|
})).collect::<Vec<_>>()
|
||||||
})
|
});
|
||||||
});
|
|
||||||
|
|
||||||
if let Some(reasoning_content) = &m.reasoning_content {
|
if let Some(reasoning_content) = &m.reasoning_content {
|
||||||
message["reasoning_content"] = Value::String(reasoning_content.clone());
|
message["reasoning_content"] = Value::String(reasoning_content.clone());
|
||||||
|
}
|
||||||
|
|
||||||
|
Some(message)
|
||||||
}
|
}
|
||||||
|
|
||||||
message
|
|
||||||
} else {
|
} else {
|
||||||
let mut message = json!({
|
let mut message = json!({
|
||||||
"role": m.role,
|
"role": m.role,
|
||||||
@ -674,7 +741,7 @@ impl OpenAIProvider {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
message
|
Some(message)
|
||||||
}
|
}
|
||||||
}).collect::<Vec<_>>(),
|
}).collect::<Vec<_>>(),
|
||||||
});
|
});
|
||||||
@ -970,18 +1037,28 @@ mod tests {
|
|||||||
);
|
);
|
||||||
|
|
||||||
let request = ChatCompletionRequest {
|
let request = ChatCompletionRequest {
|
||||||
messages: vec![Message {
|
messages: vec![
|
||||||
role: "assistant".to_string(),
|
Message {
|
||||||
content: vec![ContentBlock::text("calling tool")],
|
role: "assistant".to_string(),
|
||||||
reasoning_content: None,
|
content: vec![ContentBlock::text("calling tool")],
|
||||||
tool_call_id: None,
|
reasoning_content: None,
|
||||||
name: None,
|
tool_call_id: None,
|
||||||
tool_calls: Some(vec![ToolCall {
|
name: None,
|
||||||
id: "call_1".to_string(),
|
tool_calls: Some(vec![ToolCall {
|
||||||
name: "calculator".to_string(),
|
id: "call_1".to_string(),
|
||||||
arguments: json!({"expression": "1+1"}),
|
name: "calculator".to_string(),
|
||||||
}]),
|
arguments: json!({"expression": "1+1"}),
|
||||||
}],
|
}]),
|
||||||
|
},
|
||||||
|
Message {
|
||||||
|
role: "tool".to_string(),
|
||||||
|
content: vec![ContentBlock::text("2")],
|
||||||
|
reasoning_content: None,
|
||||||
|
tool_call_id: Some("call_1".to_string()),
|
||||||
|
name: Some("calculator".to_string()),
|
||||||
|
tool_calls: None,
|
||||||
|
},
|
||||||
|
],
|
||||||
temperature: None,
|
temperature: None,
|
||||||
max_tokens: None,
|
max_tokens: None,
|
||||||
tools: None,
|
tools: None,
|
||||||
@ -1016,18 +1093,28 @@ mod tests {
|
|||||||
);
|
);
|
||||||
|
|
||||||
let request = ChatCompletionRequest {
|
let request = ChatCompletionRequest {
|
||||||
messages: vec![Message {
|
messages: vec![
|
||||||
role: "assistant".to_string(),
|
Message {
|
||||||
content: vec![ContentBlock::text("calling tool")],
|
role: "assistant".to_string(),
|
||||||
reasoning_content: None,
|
content: vec![ContentBlock::text("calling tool")],
|
||||||
tool_call_id: None,
|
reasoning_content: None,
|
||||||
name: None,
|
tool_call_id: None,
|
||||||
tool_calls: Some(vec![ToolCall {
|
name: None,
|
||||||
id: "call_1".to_string(),
|
tool_calls: Some(vec![ToolCall {
|
||||||
name: "calculator".to_string(),
|
id: "call_1".to_string(),
|
||||||
arguments: json!({"expression": "1+1"}),
|
name: "calculator".to_string(),
|
||||||
}]),
|
arguments: json!({"expression": "1+1"}),
|
||||||
}],
|
}]),
|
||||||
|
},
|
||||||
|
Message {
|
||||||
|
role: "tool".to_string(),
|
||||||
|
content: vec![ContentBlock::text("2")],
|
||||||
|
reasoning_content: None,
|
||||||
|
tool_call_id: Some("call_1".to_string()),
|
||||||
|
name: Some("calculator".to_string()),
|
||||||
|
tool_calls: None,
|
||||||
|
},
|
||||||
|
],
|
||||||
temperature: None,
|
temperature: None,
|
||||||
max_tokens: None,
|
max_tokens: None,
|
||||||
tools: None,
|
tools: None,
|
||||||
@ -1059,18 +1146,28 @@ mod tests {
|
|||||||
);
|
);
|
||||||
|
|
||||||
let request = ChatCompletionRequest {
|
let request = ChatCompletionRequest {
|
||||||
messages: vec![Message {
|
messages: vec![
|
||||||
role: "assistant".to_string(),
|
Message {
|
||||||
content: vec![ContentBlock::text("calling tool")],
|
role: "assistant".to_string(),
|
||||||
reasoning_content: None,
|
content: vec![ContentBlock::text("calling tool")],
|
||||||
tool_call_id: None,
|
reasoning_content: None,
|
||||||
name: None,
|
tool_call_id: None,
|
||||||
tool_calls: Some(vec![ToolCall {
|
name: None,
|
||||||
id: "call_1".to_string(),
|
tool_calls: Some(vec![ToolCall {
|
||||||
name: "calculator".to_string(),
|
id: "call_1".to_string(),
|
||||||
arguments: Value::String("{\"expression\":\"1+1\"}".to_string()),
|
name: "calculator".to_string(),
|
||||||
}]),
|
arguments: Value::String("{\"expression\":\"1+1\"}".to_string()),
|
||||||
}],
|
}]),
|
||||||
|
},
|
||||||
|
Message {
|
||||||
|
role: "tool".to_string(),
|
||||||
|
content: vec![ContentBlock::text("2")],
|
||||||
|
reasoning_content: None,
|
||||||
|
tool_call_id: Some("call_1".to_string()),
|
||||||
|
name: Some("calculator".to_string()),
|
||||||
|
tool_calls: None,
|
||||||
|
},
|
||||||
|
],
|
||||||
temperature: None,
|
temperature: None,
|
||||||
max_tokens: None,
|
max_tokens: None,
|
||||||
tools: None,
|
tools: None,
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user