Repository navigation
fix(worker): treat empty LLM response after text output as completion - #1677
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the worker's agentic loop where jobs would erroneously be marked as failed. The core problem stemmed from the system's inability to differentiate between a genuine LLM failure and a natural conclusion of the LLM's output (e.g., an empty response after providing all necessary information). By introducing a state flag to track prior substantive text output, the system can now correctly interpret subsequent empty responses or errors as successful completion, thereby ensuring accurate job status reporting and preventing unnecessary retries. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a has_text_response flag to the JobDelegate to refine job completion logic. This flag ensures that once a substantive text response has been generated, subsequent empty responses or LLM errors are treated as successful job completion rather than fatal failures or retry signals. The error handling in select_tools and respond_with_tools methods, as well as the handle_text_response method, have been updated to incorporate this new logic. A new test case has been added to validate this behavior. The review suggests refactoring duplicated error handling logic into a helper function to improve maintainability.
22e5dbf to
be47024
Compare
zmanian
left a comment
There was a problem hiding this comment.
Review: fix(worker): treat empty LLM response after text output as completion
The intent is valid but the implementation is too aggressive.
High
-
try_complete_on_errorswallows ALL error variants:AuthFailed,ContextLengthExceeded,ModelNotAvailable,Io-- all treated as "job done" after any text output. Configuration and infrastructure errors should never be silently swallowed. Restrict to empty-response-like errors only. -
Premature termination of multi-step jobs: A job that produces intermediate text ("I'll fetch the data now") then hits a transient
Httperror on the next tool-selection call gets marked complete instead of retrying.
Medium
-
Test doesn't exercise actual code:
empty_response_after_text_signals_completionreimplements the decision logic with string comparisons rather than testingJobDelegate::handle_text_response. Would pass even if the real implementation were broken. -
mark_completederrors silently dropped:let _ = self.worker.mark_completed().await-- existing code at line 1315 logs the error; new code drops it.
Recommendations
- Restrict error swallowing to specific variants (e.g.,
InvalidResponsewith empty body) - Raise the completion threshold (e.g., text + no pending tool calls, or two consecutive empty responses)
- Test the actual
JobDelegatemethods, not a reimplemented copy
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
…tion 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) <noreply@anthropic.com>
aa99a06 to
67d941f
Compare
zmanian
left a comment
There was a problem hiding this comment.
Re-review: empty LLM response completion
Significant improvement. Core safety concern resolved.
| Finding | Previous | Current |
|---|---|---|
| Error swallowing scope | HIGH | Fixed -- only EmptyResponse, not all errors |
| Premature termination | HIGH | Medium -- edge case in interleaved text+tool jobs |
| Test coverage | MEDIUM | Fixed -- tests actual is_completion_eligible_error() |
The EmptyResponse-only gating is the right approach. The remaining edge case (premature completion during multi-step jobs) is acceptable for the routine/heartbeat use case. Approve.
…nearai#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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: j-bloggs <j-bloggs@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nearai#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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: j-bloggs <j-bloggs@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Change Type
Linked Issue
Addresses review feedback from zmanian and gemini-code-assist on this PR.
Validation
Security Impact
None - error handling change only.
Database Impact
None
Blast Radius
Rollback Plan
Revert to staging behavior where all LLM errors propagate (re-introduces spurious failure bug but eliminates silent error swallowing).
Review Track
Track C - runtime changes in src/worker/ and src/llm/
Feature Parity
No FEATURE_PARITY.md changes needed.