From fd41bdf4bed3c9b43cf12788b717ac4c0fa8b5b5 Mon Sep 17 00:00:00 2001 From: Joseph Bloggs <252831379+j-bloggs@users.noreply.github.com> Date: Sun, 29 Mar 2026 04:46:08 +1100 Subject: [PATCH] fix(worker): treat empty LLM response after text output as completion (#1677) * fix(worker): treat empty LLM response after text output as completion When a job's LLM produces a substantive text response (e.g., formatted results from a routine) and the next LLM call returns empty or errors, the worker now treats this as successful completion instead of continuing the loop until failure. Previously, empty responses always triggered TextAction::Continue, causing the loop to re-call the LLM. The LLM had nothing more to say, so the provider returned "Response contained no message or tool call (empty)". This made routine jobs that successfully produced results report as "failed". The fix adds a `has_text_response` flag to JobDelegate: - After any non-empty text response: flag is set - Empty text after flag is set: treated as completion - LLM errors (select_tools/respond_with_tools) after flag: treated as completion instead of propagating - Empty text before any output: still retries (rate-limit backoff) Co-Authored-By: Claude Opus 4.6 (1M context) * fix(worker): restrict error swallowing to EmptyResponse variant only - Add LlmError::EmptyResponse variant for when LLM returns no content - Update nearai_chat and github_copilot providers to emit EmptyResponse instead of InvalidResponse for empty/no-choice responses - try_complete_on_error now only swallows EmptyResponse (not AuthFailed, ContextLengthExceeded, Http, Io, etc.) - Extract is_completion_eligible_error as testable pure function - Log mark_completed errors at warn level instead of silently dropping - Add EmptyResponse to retry and circuit breaker transient classifications - Rewrite test to exercise real classification logic against all variants Addresses review feedback from zmanian and gemini-code-assist. Co-Authored-By: Claude Opus 4.6 (1M context) * refactor(worker): extract mark_completed_or_warn helper to DRY completion logic Extract shared mark-completed + warn-on-failure pattern into a single helper method used by both try_complete_on_error and handle_text_response. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: j-bloggs Co-authored-by: Claude Opus 4.6 (1M context) --- src/llm/circuit_breaker.rs | 1 + src/llm/error.rs | 3 + src/llm/github_copilot.rs | 6 +- src/llm/nearai_chat.rs | 6 +- src/llm/retry.rs | 1 + src/worker/job.rs | 152 ++++++++++++++++++++++++++++++++++++- 6 files changed, 157 insertions(+), 12 deletions(-) diff --git a/src/llm/circuit_breaker.rs b/src/llm/circuit_breaker.rs index 46f29ded..d0f74421 100644 --- a/src/llm/circuit_breaker.rs +++ b/src/llm/circuit_breaker.rs @@ -234,6 +234,7 @@ fn is_transient(err: &LlmError) -> bool { LlmError::RequestFailed { .. } | LlmError::RateLimited { .. } | LlmError::InvalidResponse { .. } + | LlmError::EmptyResponse { .. } | LlmError::SessionExpired { .. } | LlmError::SessionRenewalFailed { .. } | LlmError::Http(_) diff --git a/src/llm/error.rs b/src/llm/error.rs index 749e7820..ce516d72 100644 --- a/src/llm/error.rs +++ b/src/llm/error.rs @@ -17,6 +17,9 @@ pub enum LlmError { #[error("Invalid response from {provider}: {reason}")] InvalidResponse { provider: String, reason: String }, + #[error("Empty response from {provider}: no content returned")] + EmptyResponse { provider: String }, + #[error("Context length exceeded: {used} tokens used, {limit} allowed")] ContextLengthExceeded { used: usize, limit: usize }, diff --git a/src/llm/github_copilot.rs b/src/llm/github_copilot.rs index c7a24b1a..6fefe5af 100644 --- a/src/llm/github_copilot.rs +++ b/src/llm/github_copilot.rs @@ -231,9 +231,8 @@ impl LlmProvider for GithubCopilotProvider { .choices .into_iter() .next() - .ok_or_else(|| LlmError::InvalidResponse { + .ok_or_else(|| LlmError::EmptyResponse { provider: "github_copilot".to_string(), - reason: "No choices in response".to_string(), })?; let (content, _tool_calls) = extract_choice_content(&choice); @@ -309,9 +308,8 @@ impl LlmProvider for GithubCopilotProvider { .choices .into_iter() .next() - .ok_or_else(|| LlmError::InvalidResponse { + .ok_or_else(|| LlmError::EmptyResponse { provider: "github_copilot".to_string(), - reason: "No choices in response".to_string(), })?; let (content, tool_calls) = extract_choice_content(&choice); diff --git a/src/llm/nearai_chat.rs b/src/llm/nearai_chat.rs index 1f6dbb77..80335d86 100644 --- a/src/llm/nearai_chat.rs +++ b/src/llm/nearai_chat.rs @@ -490,9 +490,8 @@ impl LlmProvider for NearAiChatProvider { .choices .into_iter() .next() - .ok_or_else(|| LlmError::InvalidResponse { + .ok_or_else(|| LlmError::EmptyResponse { provider: "nearai_chat".to_string(), - reason: "No choices in response".to_string(), })?; // Fall back to reasoning_content when content is null (same as @@ -570,9 +569,8 @@ impl LlmProvider for NearAiChatProvider { .choices .into_iter() .next() - .ok_or_else(|| LlmError::InvalidResponse { + .ok_or_else(|| LlmError::EmptyResponse { provider: "nearai_chat".to_string(), - reason: "No choices in response".to_string(), })?; let tool_calls: Vec = choice diff --git a/src/llm/retry.rs b/src/llm/retry.rs index 78a26b27..db76ba8b 100644 --- a/src/llm/retry.rs +++ b/src/llm/retry.rs @@ -48,6 +48,7 @@ pub(crate) fn is_retryable(err: &LlmError) -> bool { LlmError::RequestFailed { .. } | LlmError::RateLimited { .. } | LlmError::InvalidResponse { .. } + | LlmError::EmptyResponse { .. } | LlmError::SessionRenewalFailed { .. } | LlmError::Http(_) | LlmError::Io(_) diff --git a/src/worker/job.rs b/src/worker/job.rs index f74d4ec8..94a04290 100644 --- a/src/worker/job.rs +++ b/src/worker/job.rs @@ -391,6 +391,7 @@ Report when the job is complete or if you encounter issues you cannot resolve."# worker: self, rx: tokio::sync::Mutex::new(rx), consecutive_rate_limits: std::sync::atomic::AtomicUsize::new(0), + has_text_response: std::sync::atomic::AtomicBool::new(false), }; let config = AgenticLoopConfig { @@ -1101,6 +1102,15 @@ fn store_fallback_in_metadata( } /// Job delegate: implements `LoopDelegate` for the background job context. +/// Whether an LLM error represents a completion-eligible empty response. +/// +/// Only `EmptyResponse` (provider returned no choices/content) qualifies. +/// Infrastructure errors (`AuthFailed`, `Http`, `Io`, etc.) never qualify — +/// they must propagate even if prior text output was produced. +fn is_completion_eligible_error(error: &crate::error::LlmError) -> bool { + matches!(error, crate::error::LlmError::EmptyResponse { .. }) +} + /// /// Handles: signal channel (stop/ping/user messages), cancellation checks, /// rate-limit retry, parallel tool execution, DB persistence, SSE broadcasting. @@ -1109,6 +1119,10 @@ struct JobDelegate<'a> { rx: tokio::sync::Mutex<&'a mut mpsc::Receiver>, /// Tracks consecutive rate-limit errors to fail fast instead of burning iterations. consecutive_rate_limits: std::sync::atomic::AtomicUsize, + /// Whether a substantive (non-empty) text response has been produced. + /// When true, an empty follow-up response is treated as job completion + /// rather than a retry signal (prevents spurious failures in routines). + has_text_response: std::sync::atomic::AtomicBool, } impl<'a> JobDelegate<'a> { @@ -1161,6 +1175,53 @@ impl<'a> JobDelegate<'a> { finish_reason: crate::llm::FinishReason::Stop, }) } + + /// Mark the job as completed, logging a warning on failure. + async fn mark_completed_or_warn(&self, context: &str) { + if let Err(e) = self.worker.mark_completed().await { + tracing::warn!( + job_id = %self.worker.job_id, + error = %e, + "Failed to mark job completed ({context})" + ); + } + } + + /// If a substantive text response was already produced and the error + /// indicates the LLM simply returned nothing, treat it as successful + /// completion rather than a fatal failure. + /// + /// Only swallows `EmptyResponse` — infrastructure errors (`AuthFailed`, + /// `ContextLengthExceeded`, `Http`, `Io`, etc.) always propagate. + /// + /// Returns `Some(empty RespondOutput)` when the error should be swallowed, + /// `None` when it should propagate normally. + async fn try_complete_on_error( + &self, + context: &str, + error: &crate::error::LlmError, + ) -> Option { + if !is_completion_eligible_error(error) { + return None; + } + if !self + .has_text_response + .load(std::sync::atomic::Ordering::Relaxed) + { + return None; + } + tracing::info!( + job_id = %self.worker.job_id, + error = %error, + "{context} empty response after text output — treating as completion" + ); + self.mark_completed_or_warn(context).await; + Some(crate::llm::RespondOutput { + result: RespondResult::Text(String::new()), + usage: crate::llm::TokenUsage::default(), + finish_reason: crate::llm::FinishReason::Stop, + }) + } } #[async_trait] @@ -1291,7 +1352,12 @@ impl<'a> LoopDelegate for JobDelegate<'a> { Err(crate::error::LlmError::RateLimited { retry_after, .. }) => { return self.handle_rate_limit(retry_after, "tool selection").await; } - Err(e) => return Err(e.into()), + Err(e) => { + if let Some(output) = self.try_complete_on_error("select_tools", &e).await { + return Ok(output); + } + return Err(e.into()); + } }; // Fall back to respond_with_tools @@ -1321,7 +1387,12 @@ impl<'a> LoopDelegate for JobDelegate<'a> { self.handle_rate_limit(retry_after, "respond_with_tools") .await } - Err(e) => Err(e.into()), + Err(e) => { + if let Some(output) = self.try_complete_on_error("respond_with_tools", &e).await { + return Ok(output); + } + Err(e.into()) + } } } @@ -1330,9 +1401,22 @@ impl<'a> LoopDelegate for JobDelegate<'a> { text: &str, reason_ctx: &mut ReasoningContext, ) -> TextAction { - // Empty text from rate-limit backoff retry — skip processing and let the - // loop proceed to the next iteration which will re-call the LLM. + // Empty text after a substantive response means the LLM has finished. + // Treat as successful completion rather than continuing the loop (which + // would produce "Response contained no message or tool call (empty)"). if text.is_empty() { + if self + .has_text_response + .load(std::sync::atomic::Ordering::Relaxed) + { + tracing::debug!( + job_id = %self.worker.job_id, + "Empty response after text output — treating as completion" + ); + self.mark_completed_or_warn("empty text response").await; + return TextAction::Return(LoopOutcome::Response(String::new())); + } + // No prior text response — this is likely a rate-limit backoff retry. return TextAction::Continue; } @@ -1348,6 +1432,10 @@ impl<'a> LoopDelegate for JobDelegate<'a> { return TextAction::Return(LoopOutcome::Response(text.to_string())); } + // Track that a substantive response has been produced. + self.has_text_response + .store(true, std::sync::atomic::Ordering::Relaxed); + // Add assistant response to context reason_ctx.messages.push(ChatMessage::assistant(text)); @@ -2285,4 +2373,60 @@ mod tests { assert_eq!(telegram[0].0, "owner-scope"); assert_eq!(telegram[0].1.content, "hello from routine"); } + + /// Regression test: only `EmptyResponse` errors are eligible for + /// completion-swallowing. Infrastructure errors must always propagate. + #[test] + fn is_completion_eligible_only_matches_empty_response() { + use crate::error::LlmError; + + // EmptyResponse is eligible + assert!(super::is_completion_eligible_error( + &LlmError::EmptyResponse { + provider: "test".to_string(), + } + )); + + // All other variants are NOT eligible + assert!(!super::is_completion_eligible_error( + &LlmError::InvalidResponse { + provider: "test".to_string(), + reason: "parse error".to_string(), + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::AuthFailed { + provider: "test".to_string(), + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::ContextLengthExceeded { + used: 100_000, + limit: 50_000, + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::ModelNotAvailable { + provider: "test".to_string(), + model: "gpt-4".to_string(), + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::RequestFailed { + provider: "test".to_string(), + reason: "timeout".to_string(), + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::SessionExpired { + provider: "test".to_string(), + } + )); + assert!(!super::is_completion_eligible_error( + &LlmError::SessionRenewalFailed { + provider: "test".to_string(), + reason: "timeout".to_string(), + } + )); + } }