From f8434b441579ff61ad63774dfd04bdab0c2f97df Mon Sep 17 00:00:00 2001 From: David Katz Date: Thu, 2 Oct 2025 13:02:34 -0400 Subject: [PATCH] pending fix --- crates/goose/src/conversation/mod.rs | 319 ++++++++++++++++++++++++++- ui/desktop/openapi.json | 2 +- 2 files changed, 316 insertions(+), 5 deletions(-) diff --git a/crates/goose/src/conversation/mod.rs b/crates/goose/src/conversation/mod.rs index 4005d1322e..96b42a729b 100644 --- a/crates/goose/src/conversation/mod.rs +++ b/crates/goose/src/conversation/mod.rs @@ -144,9 +144,245 @@ impl Default for Conversation { /// Fix a conversation that we're about to send to an LLM. So the last and first /// messages should always be from the user. pub fn fix_conversation(conversation: Conversation) -> (Conversation, Vec) { - let messages = conversation.messages().clone(); - let (messages, issues) = fix_messages(messages); - (Conversation::new_unvalidated(messages), issues) + let all_messages = conversation.messages().clone(); + + // If no messages, return empty + if all_messages.is_empty() { + return (conversation, vec![]); + } + + // Check if there are any agent-visible messages + let has_agent_visible = all_messages.iter().any(|m| m.is_agent_visible()); + if !has_agent_visible { + return ( + conversation, + vec!["No agent-visible messages to fix".to_string()], + ); + } + + // Apply fixes with visibility awareness + let (fixed_messages, issues) = fix_messages_with_visibility(all_messages); + + (Conversation::new_unvalidated(fixed_messages), issues) +} + +fn fix_messages_with_visibility(messages: Vec) -> (Vec, Vec) { + // First pass: remove empty agent-visible messages + let (messages_1, empty_removed) = remove_empty_agent_visible_messages(messages); + + // Second pass: fix tool calling for agent-visible messages + let (messages_2, tool_calling_fixed) = fix_tool_calling_with_visibility(messages_1); + + // Third pass: merge consecutive agent-visible messages (but not if separated by non-agent-visible) + let (messages_3, messages_merged) = merge_consecutive_agent_visible_messages(messages_2); + + // Fourth pass: fix leading/trailing for agent-visible messages + let (messages_4, lead_trail_fixed) = fix_lead_trail_agent_visible(messages_3); + + // Fifth pass: populate if no agent-visible messages + let (messages_5, populated_if_empty) = populate_if_no_agent_visible(messages_4); + + let mut issues = Vec::new(); + issues.extend(empty_removed); + issues.extend(tool_calling_fixed); + issues.extend(messages_merged); + issues.extend(lead_trail_fixed); + issues.extend(populated_if_empty); + + (messages_5, issues) +} + +fn remove_empty_agent_visible_messages(messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + let filtered_messages = messages + .into_iter() + .filter(|msg| { + if msg.is_agent_visible() && msg.content.is_empty() { + issues.push("Removed empty agent-visible message".to_string()); + false + } else { + true + } + }) + .collect(); + (filtered_messages, issues) +} + +fn fix_tool_calling_with_visibility(mut messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + let mut pending_tool_requests: HashSet = HashSet::new(); + + for message in &mut messages { + // Only process agent-visible messages + if !message.is_agent_visible() { + continue; + } + + let mut content_to_remove = Vec::new(); + + match message.role { + Role::User => { + for (idx, content) in message.content.iter().enumerate() { + match content { + MessageContent::ToolRequest(req) => { + content_to_remove.push(idx); + issues.push(format!( + "Removed tool request '{}' from user message", + req.id + )); + } + MessageContent::ToolConfirmationRequest(req) => { + content_to_remove.push(idx); + issues.push(format!( + "Removed tool confirmation request '{}' from user message", + req.id + )); + } + MessageContent::Thinking(_) | MessageContent::RedactedThinking(_) => { + content_to_remove.push(idx); + issues.push("Removed thinking content from user message".to_string()); + } + MessageContent::ToolResponse(resp) => { + if pending_tool_requests.contains(&resp.id) { + pending_tool_requests.remove(&resp.id); + } else { + content_to_remove.push(idx); + issues + .push(format!("Removed orphaned tool response '{}'", resp.id)); + } + } + _ => {} + } + } + } + Role::Assistant => { + for (idx, content) in message.content.iter().enumerate() { + match content { + MessageContent::ToolResponse(resp) => { + content_to_remove.push(idx); + issues.push(format!( + "Removed tool response '{}' from assistant message", + resp.id + )); + } + MessageContent::FrontendToolRequest(req) => { + content_to_remove.push(idx); + issues.push(format!( + "Removed frontend tool request '{}' from assistant message", + req.id + )); + } + MessageContent::ToolRequest(req) => { + pending_tool_requests.insert(req.id.clone()); + } + _ => {} + } + } + } + } + + for &idx in content_to_remove.iter().rev() { + message.content.remove(idx); + } + } + + // Remove orphaned tool requests from agent-visible assistant messages + for message in &mut messages { + if message.is_agent_visible() && message.role == Role::Assistant { + let mut content_to_remove = Vec::new(); + for (idx, content) in message.content.iter().enumerate() { + if let MessageContent::ToolRequest(req) = content { + if pending_tool_requests.contains(&req.id) { + content_to_remove.push(idx); + issues.push(format!("Removed orphaned tool request '{}'", req.id)); + } + } + } + for &idx in content_to_remove.iter().rev() { + message.content.remove(idx); + } + } + } + + let (messages, empty_removed) = remove_empty_agent_visible_messages(messages); + issues.extend(empty_removed); + (messages, issues) +} + +fn merge_consecutive_agent_visible_messages(messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + let mut merged_messages: Vec = Vec::new(); + + for message in messages { + // If message is not agent-visible, just add it + if !message.is_agent_visible() { + merged_messages.push(message); + continue; + } + + // Check if we can merge with the last agent-visible message + // Find the last agent-visible message + let last_agent_visible_idx = merged_messages.iter().rposition(|m| m.is_agent_visible()); + + if let Some(idx) = last_agent_visible_idx { + // Check if they are truly consecutive from agent's perspective + // (no other agent-visible messages between them) + let has_other_agent_visible_between = merged_messages[idx + 1..] + .iter() + .any(|m| m.is_agent_visible()); + + if !has_other_agent_visible_between { + // Check if they have the same effective role + let effective = effective_role(&message); + if effective_role(&merged_messages[idx]) == effective { + // Merge the content + merged_messages[idx].content.extend(message.content); + issues.push(format!("Merged consecutive {} messages", effective)); + continue; + } + } + } + + merged_messages.push(message); + } + + (merged_messages, issues) +} + +fn fix_lead_trail_agent_visible(mut messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + + // Find first agent-visible message + if let Some(first_idx) = messages.iter().position(|m| m.is_agent_visible()) { + if messages[first_idx].role == Role::Assistant { + messages.remove(first_idx); + issues.push("Removed leading agent-visible assistant message".to_string()); + } + } + + // Find last agent-visible message + if let Some(last_idx) = messages.iter().rposition(|m| m.is_agent_visible()) { + if last_idx < messages.len() && messages[last_idx].role == Role::Assistant { + messages.remove(last_idx); + issues.push("Removed trailing agent-visible assistant message".to_string()); + } + } + + (messages, issues) +} + +fn populate_if_no_agent_visible(mut messages: Vec) -> (Vec, Vec) { + let mut issues = Vec::new(); + + // Check if there are any agent-visible messages + let has_agent_visible = messages.iter().any(|m| m.is_agent_visible()); + + if !has_agent_visible { + issues.push("Added placeholder user message as no agent-visible messages".to_string()); + messages.push(Message::user().with_text(PLACEHOLDER_USER_MESSAGE)); + } + + (messages, issues) } fn fix_messages(messages: Vec) -> (Vec, Vec) { @@ -502,7 +738,9 @@ mod tests { assert_eq!(fixed.len(), 1); - assert!(issues.iter().any(|i| i.contains("Removed empty message"))); + assert!(issues + .iter() + .any(|i| i.contains("Removed empty agent-visible message"))); assert!(issues .iter() .any(|i| i.contains("Removed orphaned tool response 'wrong_id'"))); @@ -550,4 +788,77 @@ mod tests { let (_fixed, issues) = run_verify(messages); assert_eq!(issues.len(), 0); } + + #[test] + fn test_mixed_visibility_messages_preserved() { + use crate::conversation::message::MessageMetadata; + + let messages = vec![ + Message::user().with_text("Visible to all"), + Message::assistant() + .with_text("Agent-only response") + .with_metadata(MessageMetadata::agent_only()), + Message::user() + .with_text("User-only comment") + .with_metadata(MessageMetadata::user_only()), + Message::assistant().with_text("Visible to all again"), + Message::user().with_text("Final visible message"), + ]; + + let conversation = Conversation::new_unvalidated(messages.clone()); + let (fixed, issues) = fix_conversation(conversation); + + // The two assistant messages get merged from agent's perspective + // because the user-only message between them is not agent-visible + assert_eq!( + fixed.len(), + 4, + "Messages after merging consecutive assistant messages" + ); + + // Check visibility is preserved + assert!(fixed.messages()[0].is_user_visible() && fixed.messages()[0].is_agent_visible()); + // The merged assistant message + assert!(!fixed.messages()[1].is_user_visible() && fixed.messages()[1].is_agent_visible()); + // User-only message preserved + assert!(fixed.messages()[2].is_user_visible() && !fixed.messages()[2].is_agent_visible()); + // Final user message + assert!(fixed.messages()[3].is_user_visible() && fixed.messages()[3].is_agent_visible()); + + // Should have merge issue + assert_eq!(issues.len(), 1); + assert!(issues[0].contains("Merged consecutive assistant messages")); + } + + #[test] + fn test_fixing_with_non_agent_visible_preserved() { + use crate::conversation::message::MessageMetadata; + + let messages = vec![ + Message::user().with_text("Start"), + Message::user().with_text("Duplicate user message"), // This should be merged for agent + Message::assistant() + .with_text("User-only status") + .with_metadata(MessageMetadata::user_only()), // Not visible to agent + Message::assistant().with_text("Response 1"), + Message::assistant().with_text("Response 2"), // Should be merged with Response 1 + Message::user().with_text("End"), + ]; + + let conversation = Conversation::new_unvalidated(messages); + let (fixed, issues) = fix_conversation(conversation); + + // The user-only assistant message should be preserved + // Agent-visible messages get fixed (consecutive messages merged) + assert_eq!(fixed.len(), 4); // Start + Duplicate merged, user-only preserved, Response 1+2 merged, End + + // Check that the user-only message is still there + let user_only_messages = fixed.user_visible_messages(); + assert!(user_only_messages + .iter() + .any(|m| m.as_concat_text().contains("User-only status"))); + + // Should have merge issues + assert!(issues.iter().any(|i| i.contains("Merged consecutive"))); + } } diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index 4366315912..b4dde77fd3 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -10,7 +10,7 @@ "license": { "name": "Apache-2.0" }, - "version": "1.9.0" + "version": "1.9.1" }, "paths": { "/agent/add_sub_recipes": {