tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 5 Packages Projects Releases Wiki Activity

fix(agent): retry empty provider turns instead of stopping silently (#10360)

Browse Source
This commit is contained in:
Michael A. Smith
2026-07-10 21:16:00 -04:00
committed by GitHub
parent 1774b3a3a9
commit 3c1fdd692c
3 changed files with 569 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/agents/agent.rs
+75 -17
View File
@@ -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<bool> {
let result = self
.retry_manager
) -> Result<RetryResult> {
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<String> {
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<String> = None;
let mut pending_turn_usage: Option<ProviderUsage> = 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(
crates/goose/src/agents/retry.rs
+9 -11
View File
@@ -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"));
}
crates/goose/tests/agent.rs
+485
View File
@@ -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<goose::config::ExtensionConfig>,
_tls_config: Option<goose::providers::api_client::TlsConfig>,
) -> futures::future::BoxFuture<'static, anyhow::Result<Self>> {
unimplemented!()
}
}
#[async_trait]
impl Provider for EmptyThenTextProvider {
async fn stream(
&self,
_model_config: &ModelConfig,
_system_prompt: &str,
_messages: &[Message],
_tools: &[Tool],
) -> Result<MessageStream, ProviderError> {
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<dyn Provider>,
session_name: &str,
) -> Result<(Vec<Message>, Vec<Message>)> {
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::<Vec<_>>()
.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(())
}
}
}