diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index d87e934..a6668e4 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -2453,6 +2453,57 @@ mod tests { // Verify no tool messages remain assert!(messages.iter().all(|m| m.role != "tool")); } + + #[test] + fn test_sanitize_strips_tool_calls_when_tool_results_not_immediately_following() { + // [assistant(tool_calls=[A]), user, tool(A)] + // Tool result exists but is NOT immediately after assistant → strip tool_calls + // This is the scenario that triggers DeepSeek 400 "insufficient tool messages + // following tool_calls message" — the reverse scan sees tool(A) as resolved, + // but the API requires it to be IMMEDIATELY after the assistant. + let mut messages = vec![ + ChatMessage::assistant_with_tool_calls( + "calling tool", + vec![ToolCall { + id: "call_A".to_string(), + name: "search".to_string(), + arguments: serde_json::json!({}), + }], + ), + ChatMessage::user("interrupting message"), + ChatMessage::tool("call_A", "search", "result"), + ]; + + let removed = crate::bus::message::sanitize_incomplete_tool_call_sequences(&mut messages); + // The assistant should be removed (tool_calls stripped via removal) + // and the orphaned tool(A) should also be removed + assert!(removed >= 2, "should remove both the assistant and orphaned tool message, got {}", removed); + assert_eq!(messages.len(), 1, "only the user message should remain"); + assert_eq!(messages[0].role, "user"); + assert!(messages.iter().all(|m| m.tool_calls.as_ref().map_or(true, |c| c.is_empty())), + "no assistant should have tool_calls remaining"); + } + + #[test] + fn test_sanitize_preserves_tool_calls_when_immediately_followed() { + // [assistant(tool_calls=[A]), tool(A), user] — valid, tool result is immediate + let mut messages = vec![ + ChatMessage::assistant_with_tool_calls( + "calling tool", + vec![ToolCall { + id: "call_A".to_string(), + name: "search".to_string(), + arguments: serde_json::json!({}), + }], + ), + ChatMessage::tool("call_A", "search", "result"), + ChatMessage::user("next message after tool result"), + ]; + + let removed = crate::bus::message::sanitize_incomplete_tool_call_sequences(&mut messages); + assert_eq!(removed, 0, "should not remove anything — tool result immediately follows"); + assert_eq!(messages.len(), 3); + } } #[derive(Debug)] diff --git a/src/bus/message.rs b/src/bus/message.rs index 08b611c..dbd0b38 100644 --- a/src/bus/message.rs +++ b/src/bus/message.rs @@ -316,6 +316,86 @@ pub(crate) fn sanitize_incomplete_tool_call_sequences(messages: &mut Vec = HashSet::new(); + let mut pending_assistant_idx: Option = None; + + for (i, m) in messages.iter().enumerate() { + // If we have pending tool_ids and encounter a non-tool message, + // the assistant's tool results were NOT immediately following. + if !pending_tool_ids.is_empty() && m.role != "tool" { + if let Some(idx) = pending_assistant_idx { + if !remove_indices.contains(&idx) { + tracing::warn!( + message_index = idx, + interrupted_by_index = i, + interrupted_by_role = %m.role, + pending_tool_call_count = pending_tool_ids.len(), + "Removing assistant with tool_calls — tool results \ + not immediately following (interrupted by non-tool message)" + ); + // Remove this assistant's tool_call_ids from with_parent + // so Phase 2 cleans up the now-orphaned tool messages + if let Some(calls) = messages[idx].tool_calls.as_ref() { + for tc in calls.iter() { + with_parent.remove(&tc.id); + } + } + remove_indices.push(idx); + } + } + pending_tool_ids.clear(); + pending_assistant_idx = None; + } + + if m.role == "assistant" + && m.tool_calls.as_ref().map_or(false, |calls| !calls.is_empty()) + { + let already_marked = remove_indices.contains(&i); + if !already_marked { + pending_tool_ids = m.tool_calls.as_ref().unwrap() + .iter().map(|tc| tc.id.clone()).collect(); + pending_assistant_idx = Some(i); + } + } else if m.role == "tool" { + if let Some(ref tc_id) = m.tool_call_id { + pending_tool_ids.remove(tc_id); + if pending_tool_ids.is_empty() { + pending_assistant_idx = None; + } + } + } + } + + // Handle trailing assistant with unresolved immediate tool results + if !pending_tool_ids.is_empty() { + if let Some(idx) = pending_assistant_idx { + if !remove_indices.contains(&idx) { + tracing::warn!( + message_index = idx, + "Removing trailing assistant with incomplete immediate tool results" + ); + if let Some(calls) = messages[idx].tool_calls.as_ref() { + for tc in calls.iter() { + with_parent.remove(&tc.id); + } + } + remove_indices.push(idx); + } + } + } + } + // Remove in descending index order to avoid shifting for &idx in &remove_indices { messages.remove(idx); diff --git a/src/gateway/execution.rs b/src/gateway/execution.rs index b812470..73bf432 100644 --- a/src/gateway/execution.rs +++ b/src/gateway/execution.rs @@ -219,8 +219,6 @@ impl AgentExecutionService { } let enriched_content = enrich_user_content_with_media_refs(request.content, &media_refs)?; - enrich_user_content_with_media_refs(request.content, &media_refs)?; - enrich_user_content_with_media_refs(request.content, &media_refs)?; // 先计算 user_message_count(在添加新消息之前) let history_before = session_guard.get_or_create_history(request.chat_id).clone(); diff --git a/src/providers/openai.rs b/src/providers/openai.rs index 01d62ad..aac4282 100644 --- a/src/providers/openai.rs +++ b/src/providers/openai.rs @@ -672,6 +672,82 @@ impl OpenAIProvider { } } + // Forward-order check: verify tool messages IMMEDIATELY follow the + // assistant(tool_calls). If any non-tool message appears between the + // assistant and its tool results, the API rejects with + // "insufficient tool messages following tool_calls message". + // + // The reverse scan above only checks existence (tool result appears + // somewhere after assistant), NOT immediacy. This forward pass catches: + // [assistant(tool_calls=[A]), user, tool(A)] + // ^ reverse scan sees tool(A) after assistant → "resolved" + // but API requires tool(A) to be IMMEDIATELY after assistant + { + let mut pending_tool_ids: std::collections::HashSet<&str> = std::collections::HashSet::new(); + let mut pending_assistant_idx: Option = None; + + for (i, m) in request.messages.iter().enumerate() { + // If we have pending tool_ids and encounter a non-tool message, + // the assistant's tool results were NOT immediately following. + if !pending_tool_ids.is_empty() && m.role != "tool" { + if let Some(idx) = pending_assistant_idx { + skip_assistant_indices.insert(idx); + tracing::warn!( + message_index = idx, + interrupted_by_index = i, + interrupted_by_role = %m.role, + pending_tool_call_count = pending_tool_ids.len(), + "build_request_body: assistant tool_calls not immediately \ + followed by tool results — stripping tool_calls" + ); + // Remove this assistant's tool_call_ids from with_parent + // so orphaned tool messages are dropped during serialization + if let Some(calls) = &request.messages[idx].tool_calls { + for tc in calls.iter() { + with_parent.remove(tc.id.as_str()); + } + } + } + pending_tool_ids.clear(); + pending_assistant_idx = None; + } + + if m.role == "assistant" { + if let Some(ref calls) = m.tool_calls { + if !calls.is_empty() && !skip_assistant_indices.contains(&i) { + pending_tool_ids = calls.iter().map(|tc| tc.id.as_str()).collect(); + pending_assistant_idx = Some(i); + } + } + } else if m.role == "tool" { + if let Some(ref tc_id) = m.tool_call_id { + pending_tool_ids.remove(tc_id.as_str()); + if pending_tool_ids.is_empty() { + pending_assistant_idx = None; + } + } + } + } + + // Handle trailing assistant with unresolved immediate tool results + if !pending_tool_ids.is_empty() { + if let Some(idx) = pending_assistant_idx { + skip_assistant_indices.insert(idx); + tracing::warn!( + message_index = idx, + pending_tool_call_count = pending_tool_ids.len(), + "build_request_body: trailing assistant tool_calls without \ + immediately following tool results — stripping tool_calls" + ); + if let Some(calls) = &request.messages[idx].tool_calls { + for tc in calls.iter() { + with_parent.remove(tc.id.as_str()); + } + } + } + } + } + // valid_tool_call_parent_ids = with_parent (assistant tool_call_ids // whose parent assistant has ALL results after it) let valid_tool_call_parent_ids = &with_parent; @@ -795,6 +871,48 @@ impl OpenAIProvider { body["tools"] = json!(tools); } + // Diagnostic: log the final message sequence when tool_calls are involved. + // This captures the exact sequence sent to the API, making 400 errors + // like "insufficient tool messages following tool_calls message" easy to + // diagnose. + let has_tool_calls = body["messages"].as_array() + .map(|msgs| msgs.iter().any(|m| m.get("tool_calls").is_some())) + .unwrap_or(false); + if has_tool_calls { + let sequence: Vec = body["messages"].as_array() + .map(|msgs| msgs.iter().enumerate().map(|(i, m)| { + let role = m.get("role").and_then(|r| r.as_str()).unwrap_or("?"); + match role { + "assistant" => { + let tc_count = m.get("tool_calls") + .and_then(|t| t.as_array()) + .map(|a| a.len()) + .unwrap_or(0); + if tc_count > 0 { + format!("[{}] assistant(tool_calls={})", i, tc_count) + } else { + format!("[{}] assistant", i) + } + } + "tool" => { + let tcid = m.get("tool_call_id") + .and_then(|t| t.as_str()) + .unwrap_or("??"); + format!("[{}] tool(id={})", i, tcid) + } + _ => format!("[{}] {}", i, role), + } + }).collect()) + .unwrap_or_default(); + tracing::info!( + provider = %self.name, + model = %self.model_id, + message_count = sequence.len(), + sequence = ?sequence, + "build_request_body: final message sequence with tool_calls" + ); + } + body } } @@ -1496,4 +1614,131 @@ mod tests { // custom_param 应该保留 assert_eq!(body["custom_param"], Value::String("value".to_string())); } + + #[test] + fn test_build_request_body_strips_tool_calls_when_not_immediately_followed() { + // [assistant(tool_calls=[A]), user, tool(A)] → should strip tool_calls + // The tool result exists but a user message interrupts between assistant + // and tool result. The API would reject this with + // "insufficient tool messages following tool_calls message". + let provider = OpenAIProvider::new( + "test".to_string(), + "key".to_string(), + "https://example.com/v1".to_string(), + HashMap::new(), + 120, + "gpt-test".to_string(), + None, + None, + HashMap::new(), + ); + + 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_A".to_string(), + name: "search".to_string(), + arguments: json!({}), + }]), + }, + Message { + role: "user".to_string(), + content: vec![ContentBlock::text("interrupting message")], + reasoning_content: None, + tool_call_id: None, + name: None, + tool_calls: None, + }, + Message { + role: "tool".to_string(), + content: vec![ContentBlock::text("result")], + reasoning_content: None, + tool_call_id: Some("call_A".to_string()), + name: Some("search".to_string()), + tool_calls: None, + }, + ], + temperature: None, + max_tokens: None, + tools: None, + }; + + let body = provider.build_request_body(&request); + let messages = body["messages"].as_array().unwrap(); + + // Assistant should NOT have tool_calls (stripped because not immediately followed) + assert!( + messages[0].get("tool_calls").is_none(), + "tool_calls should be stripped when tool results are not immediately following" + ); + // Tool message should be dropped (orphaned after stripping) + assert_eq!( + messages.len(), + 2, + "tool message should be dropped as orphaned, got {} messages", + messages.len() + ); + } + + #[test] + fn test_build_request_body_preserves_tool_calls_when_immediately_followed() { + // [assistant(tool_calls=[A]), tool(A)] → should keep tool_calls (valid sequence) + let provider = OpenAIProvider::new( + "test".to_string(), + "key".to_string(), + "https://example.com/v1".to_string(), + HashMap::new(), + 120, + "gpt-test".to_string(), + None, + None, + HashMap::new(), + ); + + 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_A".to_string(), + name: "search".to_string(), + arguments: json!({}), + }]), + }, + Message { + role: "tool".to_string(), + content: vec![ContentBlock::text("result")], + reasoning_content: None, + tool_call_id: Some("call_A".to_string()), + name: Some("search".to_string()), + tool_calls: None, + }, + ], + temperature: None, + max_tokens: None, + tools: None, + }; + + let body = provider.build_request_body(&request); + let messages = body["messages"].as_array().unwrap(); + + // Assistant should keep tool_calls (valid immediate sequence) + let tool_calls = messages[0].get("tool_calls") + .and_then(|t| t.as_array()) + .expect("tool_calls should be preserved when immediately followed"); + assert_eq!(tool_calls.len(), 1); + // Tool message should be present + assert_eq!(messages.len(), 2); + assert_eq!(messages[1]["role"], "tool"); + } }