diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 39a2d888f67a..bcc98814ef13 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -70,6 +70,9 @@ const DEFAULT_MAX_TURNS: u32 = 1000; const DEFAULT_STOP_HOOK_BLOCK_CAP: u32 = 8; const COMPACTION_PROGRESS_TEXT: &str = "goose is compacting the conversation..."; const MAX_TURNS_MESSAGE: &str = "I've reached the maximum number of actions I can do without user input. Would you like me to continue?"; +const MAX_EMPTY_TURN_RETRIES: u32 = 3; +const EMPTY_TURN_MESSAGE: &str = + "The model returned an empty response. Please resend your message to continue."; const DEFAULT_FRONTEND_INSTRUCTIONS: &str = "The following tools are provided directly by the frontend and will be executed by the frontend when called."; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -687,23 +690,15 @@ impl Agent { messages: &mut Conversation, session_config: &SessionConfig, initial_messages: &[Message], - ) -> Result { - let result = self - .retry_manager + ) -> Result { + self.retry_manager .handle_retry_logic( messages, session_config, initial_messages, &self.final_output_tool, ) - .await?; - - match result { - RetryResult::Retried => Ok(true), - RetryResult::Skipped - | RetryResult::MaxAttemptsReached - | RetryResult::SuccessChecksPassed => Ok(false), - } + .await } async fn load_project_instructions(&self, session: &Session) -> Option { let project_id = session.project_id.as_deref()?; @@ -1908,6 +1903,8 @@ impl Agent { .unwrap_or(DEFAULT_MAX_TURNS) }); let mut compaction_attempts = 0; + let mut empty_turn_retries = 0u32; + let mut retrying_after_empty_turn = false; let mut last_assistant_text = String::new(); let mut goal_check_pending = false; let mut tool_pair_summarization_done = false; @@ -1984,6 +1981,8 @@ impl Agent { if retrying_after_stop_hook_denial { retrying_after_stop_hook_denial = false; + } else if retrying_after_empty_turn { + retrying_after_empty_turn = false; } else { turns_taken += 1; } @@ -2036,6 +2035,7 @@ impl Agent { let mut tools_updated = false; let mut did_recovery_compact_this_iteration = false; let mut exit_chat = false; + let mut provider_errored = false; let mut pending_final_output: Option = None; let mut pending_turn_usage: Option = None; @@ -2411,6 +2411,7 @@ impl Agent { } #[allow(unused_variables)] Err(ref provider_err @ ProviderError::ContextLengthExceeded(_)) => { + provider_errored = true; #[cfg(feature = "telemetry")] crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); compaction_attempts += 1; @@ -2470,6 +2471,7 @@ impl Agent { } } Err(ref provider_err @ ProviderError::CreditsExhausted { details: _, ref top_up_url }) => { + provider_errored = true; #[cfg(feature = "telemetry")] crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); error!("Error: {}", provider_err); @@ -2494,6 +2496,7 @@ impl Agent { break; } Err(ref provider_err @ ProviderError::Refusal { ref details, ref category }) => { + provider_errored = true; #[cfg(feature = "telemetry")] crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); error!("Error: {}", provider_err); @@ -2509,6 +2512,7 @@ impl Agent { break; } Err(ref provider_err @ ProviderError::NetworkError(_)) => { + provider_errored = true; #[cfg(feature = "telemetry")] crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); error!("Error: {}", provider_err); @@ -2520,6 +2524,7 @@ impl Agent { break; } Err(ref provider_err) => { + provider_errored = true; #[cfg(feature = "telemetry")] crate::posthog::emit_error(provider_err.telemetry_type(), &provider_err.to_string()); error!("Error: {}", provider_err); @@ -2551,6 +2556,24 @@ impl Agent { } } + // An empty provider response — no tool calls, no text, and no error + // or recovery compaction that legitimately produces no assistant + // output — must never be persisted: strict providers reject a + // conversation that contains an empty assistant turn. Drop it here + // regardless of what the match below decides to do about the turn + // (final-output nudge, steer, goal/grind, retry, or fallback). + let empty_response = no_tools_called + && !exit_chat + && !provider_errored + && !did_recovery_compact_this_iteration + && last_assistant_text.is_empty(); + + if empty_response { + messages_to_add = Conversation::default(); + } else { + empty_turn_retries = 0; + } + if no_tools_called && !exit_chat { // Lock, extract state, drop guard before branching — handle_retry_logic // also locks final_output_tool and tokio::sync::Mutex is not reentrant. @@ -2614,17 +2637,52 @@ impl Agent { None => { self.set_goal(None).await; self.set_grind(None).await; + // Recipe retry logic owns the turn whenever a + // retry_config is present: it runs success checks, + // on_failure, and max_retries. Only when no recipe + // retry is configured (Skipped) does the empty-turn + // fallback apply. match self.handle_retry_logic(&mut conversation, &session_config, &initial_messages).await { - Ok(should_retry) => { - if should_retry { - info!("Retry logic triggered, restarting agent loop"); - messages_to_add = Conversation::default(); - session_manager.replace_conversation(&session_config.id, &conversation).await?; - yield AgentEvent::HistoryReplaced(conversation.clone()); + Ok(RetryResult::Retried) => { + info!("Retry logic triggered, restarting agent loop"); + messages_to_add = Conversation::default(); + session_manager.replace_conversation(&session_config.id, &conversation).await?; + yield AgentEvent::HistoryReplaced(conversation.clone()); + } + Ok(RetryResult::Skipped) if empty_response => { + // No recipe retry configured, and this empty + // turn would otherwise fall through to a + // silent exit. Retry a bounded number of + // times, then surface a visible message so + // the user is never left with no response. + if empty_turn_retries < MAX_EMPTY_TURN_RETRIES { + empty_turn_retries += 1; + retrying_after_empty_turn = true; + warn!( + "Provider returned an empty response; retrying ({}/{})", + empty_turn_retries, MAX_EMPTY_TURN_RETRIES + ); } else { + warn!("Provider returned an empty response after retries; ending turn"); + last_assistant_text = EMPTY_TURN_MESSAGE.to_string(); + let message = Message::assistant().with_text(EMPTY_TURN_MESSAGE); + messages_to_add.push(message.clone()); + yield AgentEvent::Message(message); exit_chat = true; } } + Ok(RetryResult::MaxAttemptsReached(message)) => { + // Surface and persist the failure message + // through the normal path so recipes don't + // exit silently when retries are exhausted. + last_assistant_text = message.as_concat_text(); + messages_to_add.push(message.clone()); + yield AgentEvent::Message(message); + exit_chat = true; + } + Ok(_) => { + exit_chat = true; + } Err(e) => { error!("Retry logic failed: {}", e); yield AgentEvent::Message( diff --git a/crates/goose/src/agents/retry.rs b/crates/goose/src/agents/retry.rs index 367c557ef515..574b2b7500cb 100644 --- a/crates/goose/src/agents/retry.rs +++ b/crates/goose/src/agents/retry.rs @@ -22,8 +22,9 @@ use crate::tool_monitor::RepetitionInspector; pub enum RetryResult { /// No retry configuration or session available, retry logic skipped Skipped, - /// Maximum retry attempts reached, cannot retry further - MaxAttemptsReached, + /// Maximum retry attempts reached, cannot retry further. Carries the + /// user-facing failure message so the caller can yield and persist it. + MaxAttemptsReached(Message), /// Success checks passed, no retry needed SuccessChecksPassed, /// Retry is needed and will be performed @@ -135,7 +136,6 @@ impl RetryManager { "Maximum retry attempts ({}) exceeded. Unable to complete the task successfully.", retry_config.max_retries )); - messages.push(error_msg); warn!( "Maximum retry attempts ({}) exceeded", retry_config.max_retries @@ -145,7 +145,7 @@ impl RetryManager { "retry_max_exceeded", &format!("Max retries ({}) exceeded", retry_config.max_retries), ); - return Ok(RetryResult::MaxAttemptsReached); + return Ok(RetryResult::MaxAttemptsReached(error_msg)); } if let Some(on_failure_cmd) = &retry_config.on_failure { @@ -335,21 +335,19 @@ mod tests { #[test] fn test_retry_result_enum() { - assert_ne!(RetryResult::Skipped, RetryResult::MaxAttemptsReached); + let max_attempts = RetryResult::MaxAttemptsReached(Message::assistant().with_text("done")); + assert_ne!(RetryResult::Skipped, max_attempts); assert_ne!(RetryResult::Skipped, RetryResult::SuccessChecksPassed); assert_ne!(RetryResult::Skipped, RetryResult::Retried); - assert_ne!( - RetryResult::MaxAttemptsReached, - RetryResult::SuccessChecksPassed - ); - assert_ne!(RetryResult::MaxAttemptsReached, RetryResult::Retried); + assert_ne!(max_attempts, RetryResult::SuccessChecksPassed); + assert_ne!(max_attempts, RetryResult::Retried); assert_ne!(RetryResult::SuccessChecksPassed, RetryResult::Retried); let result = RetryResult::Retried; let cloned = result.clone(); assert_eq!(result, cloned); - let debug_str = format!("{:?}", RetryResult::MaxAttemptsReached); + let debug_str = format!("{:?}", max_attempts); assert!(debug_str.contains("MaxAttemptsReached")); } diff --git a/crates/goose/tests/agent.rs b/crates/goose/tests/agent.rs index 175128640c1e..23cc7e0d134c 100644 --- a/crates/goose/tests/agent.rs +++ b/crates/goose/tests/agent.rs @@ -2782,4 +2782,489 @@ mod tests { } } } + + mod empty_turn_tests { + use super::*; + use async_trait::async_trait; + use goose::agents::{AgentEvent, SessionConfig}; + use goose::config::GooseMode; + use goose::conversation::message::{Message, MessageContent}; + use goose::providers::base::{ + stream_from_single_message, MessageStream, Provider, ProviderDef, ProviderMetadata, + }; + use goose::session::session_manager::SessionType; + use goose_providers::conversation::token_usage::{ProviderUsage, Usage}; + use goose_providers::errors::ProviderError; + use goose_providers::model::ModelConfig; + use rmcp::model::Tool; + use std::path::PathBuf; + use std::sync::atomic::{AtomicUsize, Ordering}; + + fn usage() -> ProviderUsage { + ProviderUsage::new( + "mock-model".to_string(), + Usage::new(Some(10), Some(5), Some(15)), + ) + } + + /// Yields empty responses (no text, no tool calls) for the first + /// `empty_count` provider calls, then a normal text response. + struct EmptyThenTextProvider { + call_count: AtomicUsize, + empty_count: usize, + } + + impl EmptyThenTextProvider { + fn new(empty_count: usize) -> Self { + Self { + call_count: AtomicUsize::new(0), + empty_count, + } + } + } + + impl goose::providers::base::ProviderDescriptor for EmptyThenTextProvider { + fn metadata() -> ProviderMetadata { + ProviderMetadata { + name: "empty-then-text-mock".to_string(), + display_name: "Empty Then Text Mock".to_string(), + description: "Mock provider for empty-turn tests".to_string(), + default_model: "mock-model".to_string(), + known_models: vec![], + model_doc_link: "".to_string(), + config_keys: vec![], + setup_steps: vec![], + model_selection_hint: None, + fast_model: None, + } + } + } + + impl ProviderDef for EmptyThenTextProvider { + type Provider = Self; + + fn from_env( + _extensions: Vec, + _tls_config: Option, + ) -> futures::future::BoxFuture<'static, anyhow::Result> { + unimplemented!() + } + } + + #[async_trait] + impl Provider for EmptyThenTextProvider { + async fn stream( + &self, + _model_config: &ModelConfig, + _system_prompt: &str, + _messages: &[Message], + _tools: &[Tool], + ) -> Result { + let call = self.call_count.fetch_add(1, Ordering::SeqCst); + if call < self.empty_count { + // Empty assistant turn: no text, no tool calls. + Ok(stream_from_single_message(Message::assistant(), usage())) + } else { + Ok(stream_from_single_message( + Message::assistant().with_text("All done."), + usage(), + )) + } + } + + fn get_name(&self) -> &str { + "empty-then-text-mock" + } + } + + /// Runs a reply to completion and returns the messages yielded to the + /// caller along with the conversation persisted to the session store. + async fn run_reply( + provider: Arc, + session_name: &str, + ) -> Result<(Vec, Vec)> { + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + session_name.to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + agent + .update_provider(provider, ModelConfig::new("mock-model"), &session.id) + .await?; + + let session_id = session.id.clone(); + let session_config = SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(50), + retry_config: None, + }; + + let reply_stream = agent + .reply(Message::user().with_text("Hi"), session_config, None) + .await?; + tokio::pin!(reply_stream); + + let mut messages = Vec::new(); + while let Some(event) = reply_stream.next().await { + if let AgentEvent::Message(m) = event? { + messages.push(m); + } + } + + let persisted = agent + .config + .session_manager + .get_session(&session_id, true) + .await? + .conversation + .map(|c| c.messages().to_vec()) + .unwrap_or_default(); + + Ok((messages, persisted)) + } + + fn concat_text(messages: &[Message]) -> String { + messages + .iter() + .flat_map(|m| m.content.iter()) + .filter_map(|c| match c { + MessageContent::Text(t) => Some(t.text.clone()), + _ => None, + }) + .collect::>() + .join("\n") + } + + fn is_empty_assistant(message: &Message) -> bool { + message.role == rmcp::model::Role::Assistant && message.content.is_empty() + } + + /// A transient empty response should be retried and recover, ultimately + /// delivering the real text response instead of stopping silently. + #[tokio::test] + async fn test_empty_turn_retries_then_recovers() -> Result<()> { + let provider = Arc::new(EmptyThenTextProvider::new(2)); + let (messages, persisted) = run_reply(provider, "empty-retry-recover").await?; + + let text = concat_text(&messages); + assert!( + text.contains("All done."), + "expected recovery to deliver the real response, got: {text:?}" + ); + assert!( + !text.contains("empty response"), + "should not surface the empty-turn fallback when recovery succeeds: {text:?}" + ); + assert!( + !persisted.iter().any(is_empty_assistant), + "retried empty turns must not be persisted: {persisted:?}" + ); + Ok(()) + } + + /// A provider that only ever returns empty responses must not hang + /// silently — after the retry budget it surfaces a visible message. + #[tokio::test] + async fn test_persistent_empty_turn_surfaces_message() -> Result<()> { + let provider = Arc::new(EmptyThenTextProvider::new(usize::MAX)); + let (messages, persisted) = run_reply(provider, "empty-persistent").await?; + + let text = concat_text(&messages); + assert!( + text.contains("empty response"), + "expected a visible empty-response message, got: {text:?}" + ); + + let last = messages.last().expect("expected at least one message"); + assert!( + matches!(last.content.first(), Some(MessageContent::Text(_))), + "expected the final message to be visible text, got: {:?}", + last.content + ); + assert!( + !persisted.iter().any(is_empty_assistant), + "empty assistant turn must not be persisted alongside the fallback: {persisted:?}" + ); + Ok(()) + } + + /// An empty response with a queued steer hands the turn to the steer + /// rather than the empty-turn fallback, but the empty assistant message + /// must still not be persisted ahead of the steer. + #[tokio::test] + async fn test_empty_response_with_steer_drops_empty_message() -> Result<()> { + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + "empty-steer".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + agent + .update_provider( + Arc::new(EmptyThenTextProvider::new(1)), + ModelConfig::new("mock-model"), + &session.id, + ) + .await?; + + // Queue the steer before reply so it stays pending through the first + // (empty) turn instead of being drained at the loop's start. + agent + .steer(&session.id, Message::user().with_text("keep going")) + .await; + + let session_id = session.id.clone(); + let session_config = SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(50), + retry_config: None, + }; + + let reply_stream = agent + .reply(Message::user().with_text("Hi"), session_config, None) + .await?; + tokio::pin!(reply_stream); + while let Some(event) = reply_stream.next().await { + event?; + } + + let persisted = agent + .config + .session_manager + .get_session(&session_id, true) + .await? + .conversation + .map(|c| c.messages().to_vec()) + .unwrap_or_default(); + + assert!( + !persisted.iter().any(is_empty_assistant), + "empty assistant turn must not be persisted before the steer: {persisted:?}" + ); + assert!( + persisted + .iter() + .any(|m| m.as_concat_text().contains("keep going")), + "the queued steer should have been consumed: {persisted:?}" + ); + Ok(()) + } + + /// When a final-output tool is installed and the model stops without + /// calling it, the empty turn must yield the mandatory final-output nudge + /// — not the generic empty-response fallback — so structured-output + /// recipes are not abandoned without producing a result. + #[tokio::test] + async fn test_empty_turn_with_final_output_tool_nudges() -> Result<()> { + use goose::agents::final_output_tool::FINAL_OUTPUT_CONTINUATION_MESSAGE; + use goose::recipe::Response; + + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + "empty-final-output".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + agent + .update_provider( + Arc::new(EmptyThenTextProvider::new(usize::MAX)), + ModelConfig::new("mock-model"), + &session.id, + ) + .await?; + agent + .add_final_output_tool(Response { + json_schema: Some(serde_json::json!({ + "type": "object", + "properties": { "result": { "type": "string" } } + })), + }) + .await; + + let session_config = SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(3), + retry_config: None, + }; + + let reply_stream = agent + .reply(Message::user().with_text("Hi"), session_config, None) + .await?; + tokio::pin!(reply_stream); + + let mut messages = Vec::new(); + while let Some(event) = reply_stream.next().await { + if let AgentEvent::Message(m) = event? { + messages.push(m); + } + } + + let text = concat_text(&messages); + assert!( + text.contains(FINAL_OUTPUT_CONTINUATION_MESSAGE), + "expected the final-output nudge, got: {text:?}" + ); + assert!( + !text.contains("empty response"), + "empty-turn fallback must not pre-empt the final-output nudge: {text:?}" + ); + Ok(()) + } + + /// A recipe with retry_config owns the turn: recipe retry logic runs + /// its success checks before the empty-turn fallback. When the check + /// already passes, an empty final turn is the successful end of the + /// recipe, not a generic empty-response error. + #[tokio::test] + async fn test_empty_turn_defers_to_recipe_retry() -> Result<()> { + use goose::agents::types::{RetryConfig, SuccessCheck}; + + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + "empty-recipe-retry".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + agent + .update_provider( + Arc::new(EmptyThenTextProvider::new(usize::MAX)), + ModelConfig::new("mock-model"), + &session.id, + ) + .await?; + + let session_config = SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(3), + retry_config: Some(RetryConfig { + max_retries: 2, + checks: vec![SuccessCheck::Shell { + command: "true".to_string(), + }], + on_failure: None, + timeout_seconds: Some(30), + on_failure_timeout_seconds: None, + }), + }; + + let reply_stream = agent + .reply(Message::user().with_text("Hi"), session_config, None) + .await?; + tokio::pin!(reply_stream); + + let mut messages = Vec::new(); + while let Some(event) = reply_stream.next().await { + if let AgentEvent::Message(m) = event? { + messages.push(m); + } + } + + let text = concat_text(&messages); + assert!( + !text.contains("empty response"), + "recipe retry (passing check) must own the empty turn, not the fallback: {text:?}" + ); + Ok(()) + } + + /// When a recipe exhausts its retries on empty turns, the max-attempts + /// failure message must be surfaced and persisted — not swallowed into a + /// silent stop. + #[tokio::test] + async fn test_recipe_max_retries_surfaces_failure() -> Result<()> { + use goose::agents::types::{RetryConfig, SuccessCheck}; + + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + "recipe-max-retries".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + let session_id = session.id.clone(); + agent + .update_provider( + Arc::new(EmptyThenTextProvider::new(usize::MAX)), + ModelConfig::new("mock-model"), + &session.id, + ) + .await?; + + let session_config = SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(5), + retry_config: Some(RetryConfig { + max_retries: 1, + checks: vec![SuccessCheck::Shell { + command: "false".to_string(), + }], + on_failure: None, + timeout_seconds: Some(30), + on_failure_timeout_seconds: None, + }), + }; + + let reply_stream = agent + .reply(Message::user().with_text("Hi"), session_config, None) + .await?; + tokio::pin!(reply_stream); + + let mut messages = Vec::new(); + while let Some(event) = reply_stream.next().await { + if let AgentEvent::Message(m) = event? { + messages.push(m); + } + } + + let text = concat_text(&messages); + assert!( + text.contains("Maximum retry attempts"), + "exhausted recipe retries must surface the failure message: {text:?}" + ); + + let persisted = agent + .config + .session_manager + .get_session(&session_id, true) + .await? + .conversation + .map(|c| c.messages().to_vec()) + .unwrap_or_default(); + assert!( + concat_text(&persisted).contains("Maximum retry attempts"), + "the max-retry failure message must be persisted: {persisted:?}" + ); + Ok(()) + } + } }