From 97c0e4634885b1d45767907fae735303e9281f9d Mon Sep 17 00:00:00 2001 From: oudecheng <13802883547@139.com> Date: Tue, 16 Jun 2026 16:52:00 +0800 Subject: [PATCH] =?UTF-8?q?feat(tests):=20=E5=A2=9E=E5=8A=A0=E5=B7=A5?= =?UTF-8?q?=E5=85=B7=E8=B0=83=E7=94=A8=E5=BA=8F=E5=88=97=E6=B8=85=E7=90=86?= =?UTF-8?q?=E7=9A=84=E5=8D=95=E5=85=83=E6=B5=8B=E8=AF=95=EF=BC=8C=E7=A1=AE?= =?UTF-8?q?=E4=BF=9D=E5=AD=A4=E7=AB=8B=E5=B7=A5=E5=85=B7=E7=BB=93=E6=9E=9C?= =?UTF-8?q?=E7=9A=84=E6=AD=A3=E7=A1=AE=E5=A4=84=E7=90=86?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/agent/agent_loop.rs | 63 +++++++++ src/bus/message.rs | 7 +- src/command/handler.rs | 1 - src/command/handlers/save_session.rs | 6 - src/gateway/agent_prompt_provider.rs | 1 - src/gateway/cli_session.rs | 1 + src/gateway/memory_maintenance.rs | 2 +- src/gateway/tool_registry_factory.rs | 1 + src/providers/openai.rs | 199 ++++++++++++++++++++------- 9 files changed, 220 insertions(+), 61 deletions(-) diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 8f2069a..21b866c 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -2259,6 +2259,69 @@ mod tests { assert_eq!(messages[4].content, "task 2"); 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)] diff --git a/src/bus/message.rs b/src/bus/message.rs index 8fc1300..b1e5518 100644 --- a/src/bus/message.rs +++ b/src/bus/message.rs @@ -325,7 +325,12 @@ pub(crate) fn sanitize_incomplete_tool_call_sequences(messages: &mut Vec &Arc { - &self.store - } } #[async_trait] diff --git a/src/gateway/agent_prompt_provider.rs b/src/gateway/agent_prompt_provider.rs index 1497803..e825904 100644 --- a/src/gateway/agent_prompt_provider.rs +++ b/src/gateway/agent_prompt_provider.rs @@ -103,7 +103,6 @@ impl SystemPromptProvider for SimpleAgentPromptProvider { #[cfg(test)] mod tests { use super::*; - use crate::storage::SessionStore; use std::collections::HashMap; fn test_config() -> LLMProviderConfig { diff --git a/src/gateway/cli_session.rs b/src/gateway/cli_session.rs index 9fda499..238ef23 100644 --- a/src/gateway/cli_session.rs +++ b/src/gateway/cli_session.rs @@ -13,6 +13,7 @@ impl CliSessionService { Self { store } } + #[allow(dead_code)] pub(crate) fn create(&self, title: Option<&str>) -> Result { self.store .create_cli_session(title) diff --git a/src/gateway/memory_maintenance.rs b/src/gateway/memory_maintenance.rs index 4eaa223..1a9cdd9 100644 --- a/src/gateway/memory_maintenance.rs +++ b/src/gateway/memory_maintenance.rs @@ -270,7 +270,7 @@ impl MemoryMaintenanceService { ))) } - #[cfg_attr(not(test), allow(dead_code))] + #[allow(dead_code)] pub(crate) async fn run_for_scope( &self, scope_key: &str, diff --git a/src/gateway/tool_registry_factory.rs b/src/gateway/tool_registry_factory.rs index 759881d..0d307db 100644 --- a/src/gateway/tool_registry_factory.rs +++ b/src/gateway/tool_registry_factory.rs @@ -93,6 +93,7 @@ impl ToolRegistryFactory { } /// Get a reference to the shell session manager for lifecycle control. + #[allow(dead_code)] pub(crate) fn shell_session_manager(&self) -> Arc { self.shell_session_manager.clone() } diff --git a/src/providers/openai.rs b/src/providers/openai.rs index 435269c..ee80827 100644 --- a/src/providers/openai.rs +++ b/src/providers/openai.rs @@ -631,22 +631,89 @@ impl OpenAIProvider { fn build_request_body(&self, request: &ChatCompletionRequest) -> Value { 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!({ "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" { - 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, "content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i), "tool_call_id": m.tool_call_id, "name": m.name, - }) + })) } else if m.role == "assistant" && m.tool_calls.is_some() { - let mut message = json!({ - "role": m.role, - "content": convert_content_blocks(supports_images, &self.name, &self.model_id, &m.content, i), - "tool_calls": m.tool_calls.as_ref().map(|calls| { - calls.iter().map(|call| json!({ + let calls = m.tool_calls.as_ref().unwrap(); + // Filter to only valid tool_calls (all have results) + let valid_calls: Vec<&ToolCall> = calls.iter() + .filter(|tc| valid_tool_call_parent_ids.contains(tc.id.as_str())) + .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, "type": "function", "function": { @@ -654,14 +721,14 @@ impl OpenAIProvider { "arguments": self.serialize_tool_arguments(&call.arguments) } })).collect::>() - }) - }); + }); - if let Some(reasoning_content) = &m.reasoning_content { - message["reasoning_content"] = Value::String(reasoning_content.clone()); + if let Some(reasoning_content) = &m.reasoning_content { + message["reasoning_content"] = Value::String(reasoning_content.clone()); + } + + Some(message) } - - message } else { let mut message = json!({ "role": m.role, @@ -674,7 +741,7 @@ impl OpenAIProvider { } } - message + Some(message) } }).collect::>(), }); @@ -970,18 +1037,28 @@ mod tests { ); let request = ChatCompletionRequest { - messages: vec![Message { - role: "assistant".to_string(), - content: vec![ContentBlock::text("calling tool")], - reasoning_content: None, - tool_call_id: None, - name: None, - tool_calls: Some(vec![ToolCall { - id: "call_1".to_string(), - name: "calculator".to_string(), - arguments: json!({"expression": "1+1"}), - }]), - }], + messages: vec![ + Message { + role: "assistant".to_string(), + content: vec![ContentBlock::text("calling tool")], + reasoning_content: None, + tool_call_id: None, + name: None, + tool_calls: Some(vec![ToolCall { + id: "call_1".to_string(), + 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, max_tokens: None, tools: None, @@ -1016,18 +1093,28 @@ mod tests { ); let request = ChatCompletionRequest { - messages: vec![Message { - role: "assistant".to_string(), - content: vec![ContentBlock::text("calling tool")], - reasoning_content: None, - tool_call_id: None, - name: None, - tool_calls: Some(vec![ToolCall { - id: "call_1".to_string(), - name: "calculator".to_string(), - arguments: json!({"expression": "1+1"}), - }]), - }], + messages: vec![ + Message { + role: "assistant".to_string(), + content: vec![ContentBlock::text("calling tool")], + reasoning_content: None, + tool_call_id: None, + name: None, + tool_calls: Some(vec![ToolCall { + id: "call_1".to_string(), + 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, max_tokens: None, tools: None, @@ -1059,18 +1146,28 @@ mod tests { ); let request = ChatCompletionRequest { - messages: vec![Message { - role: "assistant".to_string(), - content: vec![ContentBlock::text("calling tool")], - reasoning_content: None, - tool_call_id: None, - name: None, - tool_calls: Some(vec![ToolCall { - id: "call_1".to_string(), - name: "calculator".to_string(), - arguments: Value::String("{\"expression\":\"1+1\"}".to_string()), - }]), - }], + messages: vec![ + Message { + role: "assistant".to_string(), + content: vec![ContentBlock::text("calling tool")], + reasoning_content: None, + tool_call_id: None, + name: None, + tool_calls: Some(vec![ToolCall { + id: "call_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, max_tokens: None, tools: None,