fix: cap the empty-assistant-message retry loop and surface diagnostics - #1112
fix: cap the empty-assistant-message retry loop and surface diagnostics#1112carmonium wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAnthropic stop reasons now flow into stream usage chunks. ChangesEmpty-response retry control
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/core/task/__tests__/grace-retry-errors.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Around line 3708-3725: In the consecutive empty-response terminal branch of
initiateTaskLoop, change the final return value from false to true after
restoring the user message, emitting the error, and persisting the assistant
failure message so the outer loop stops. Add a regression test covering five
empty responses and assert that a sixth request is not started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86d45c0e-03bc-4738-a943-de93320fa189
📒 Files selected for processing (4)
src/api/providers/anthropic.tssrc/api/transform/stream.tssrc/core/task/Task.tssrc/core/webview/ClineProvider.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/__tests__/grace-retry-errors.spec.ts`:
- Around line 365-409: Rewrite both tests to exercise the Task retry flow with
consecutive empty provider responses rather than directly invoking the mocked
task.say method or duplicating the retry condition. Configure the mock provider
and Task execution so five empty responses pass through the real loop, then
assert that the fifth attempt emits the terminal retry-cap error, execution
terminates, and the failure message is persisted. Add the corresponding
below-cap assertion using the real flow, confirming the generic
MODEL_NO_ASSISTANT_MESSAGES behavior remains in place before the cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 905f265f-7331-4c7b-a838-d2855904d233
📒 Files selected for processing (1)
src/core/task/__tests__/grace-retry-errors.spec.ts
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Simulate reaching the retry cap (MAX_EMPTY_RESPONSE_RETRIES = 5 consecutive | ||
| // empty responses). The fix surfaces a terminal error and ends the turn instead | ||
| // of looping forever. | ||
| task.consecutiveNoAssistantMessagesCount = 5 | ||
|
|
||
| // The retry-cap branch surfaces a terminal error (not the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker) once the cap is reached. | ||
| await task.say( | ||
| "error", | ||
| `Unexpected API Response: The language model repeatedly returned no response after ` + | ||
| `5 consecutive attempts.`, | ||
| ) | ||
|
|
||
| // Verify the terminal error was surfaced. | ||
| expect(saySpy).toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response after 5 consecutive attempts"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should not surface the terminal error before the cap is reached", async () => { | ||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Below the cap (e.g. 2 consecutive empty responses), the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker is used, not the terminal error. | ||
| task.consecutiveNoAssistantMessagesCount = 2 | ||
|
|
||
| if (task.consecutiveNoAssistantMessagesCount >= 2) { | ||
| await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| } | ||
|
|
||
| expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| expect(saySpy).not.toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response"), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/core/task/__tests__/grace-retry-errors.spec.ts --items all
rg -n -C 8 'MODEL_NO_ASSISTANT_MESSAGES|MAX_EMPTY_RESPONSE_RETRIES|consecutiveNoAssistantMessagesCount|repeatedly returned no response' \
src/core/task/Task.ts src/core/task/__tests__/grace-retry-errors.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 34804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant Task implementation and retry-path structure.
sed -n '3620,3750p' src/core/task/Task.ts
printf '\n--- surrounding loop/control flow ---\n'
rg -n -C 5 'for \\(const .*RetryItem|retryAttempt|currentItem|while\\(|for \\(|continue|break' src/core/task/Task.ts | sed -n '1,220p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 6283
Exercise the Task retry path instead of calling the spy directly.
Both tests set consecutiveNoAssistantMessagesCount, then invoke the mocked task.say() directly. The second test also implements a local retry condition. These pass even if Task stops enforcing the retry cap, return termination, or persisted failure message. Drive consecutive empty provider responses through the Task loop and assert the terminal error, loop termination, and persisted failure message after attempt 5.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/task/__tests__/grace-retry-errors.spec.ts` around lines 365 - 409,
Rewrite both tests to exercise the Task retry flow with consecutive empty
provider responses rather than directly invoking the mocked task.say method or
duplicating the retry condition. Configure the mock provider and Task execution
so five empty responses pass through the real loop, then assert that the fifth
attempt emits the terminal retry-cap error, execution terminates, and the
failure message is persisted. Add the corresponding below-cap assertion using
the real flow, confirming the generic MODEL_NO_ASSISTANT_MESSAGES behavior
remains in place before the cap.
|
Thanks @coderabbitai — good catch. I've addressed the critical finding:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
What's the format CLARITY PATCH: used for? That's a new comment format for this repo I think.
| it("should surface a terminal error once the retry cap is reached", async () => { | ||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Simulate reaching the retry cap (MAX_EMPTY_RESPONSE_RETRIES = 5 consecutive | ||
| // empty responses). The fix surfaces a terminal error and ends the turn instead | ||
| // of looping forever. | ||
| task.consecutiveNoAssistantMessagesCount = 5 | ||
|
|
||
| // The retry-cap branch surfaces a terminal error (not the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker) once the cap is reached. | ||
| await task.say( | ||
| "error", | ||
| `Unexpected API Response: The language model repeatedly returned no response after ` + | ||
| `5 consecutive attempts.`, | ||
| ) | ||
|
|
||
| // Verify the terminal error was surfaced. | ||
| expect(saySpy).toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response after 5 consecutive attempts"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should not surface the terminal error before the cap is reached", async () => { | ||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Below the cap (e.g. 2 consecutive empty responses), the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker is used, not the terminal error. | ||
| task.consecutiveNoAssistantMessagesCount = 2 | ||
|
|
||
| if (task.consecutiveNoAssistantMessagesCount >= 2) { | ||
| await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| } | ||
|
|
||
| expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| expect(saySpy).not.toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response"), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Both tests call task.say() themselves and then assert the spy saw the call the test just made, so the production cap branch (Task.ts:3708) is never executed. Would these tests still pass if the cap branch were deleted? It might be better to drive the request loop with a mocked stream that yields empty responses (like the attemptApiRequest harness in Task.spec.ts) and assert the terminal message, the Failure history entry, and the loop exit — that could also cover finishReason propagation and the timer in one go.
| // rehydrate) so a fresh task/context is required to continue. Normal Stops for tasks NOT | ||
| // in this retry loop (counter === 0) are completely unaffected. | ||
| // See docs/issues/issue-014-empty-response-infinite-retry-loop.md | ||
| if (task.consecutiveNoAssistantMessagesCount > 0) { |
There was a problem hiding this comment.
This gate has no test coverage anywhere — if the > 0 check were removed, would anything catch it? A cancel-path test may be worth adding (count > 0 → evict, count === 0 → normal graceful path).
| // formatResponse.noToolsUsed() — which, if also empty, repeats this branch | ||
| // without backoff and defeats the retry cap. Returning true exits the outer | ||
| // loop so the task ends cleanly after the terminal failure. | ||
| return true |
There was a problem hiding this comment.
Should consecutiveNoAssistantMessagesCount and emptyResponseRetryLoopStartTimeMs be reset here, like the success path does (~3446)? Left at 5, a later Stop on this task would hit the hard-abort gate in cancelTask even though the loop already ended.
| role: "user", | ||
| content: currentUserContent, | ||
| }) | ||
| await this.say( |
There was a problem hiding this comment.
If the user presses Stop between the history append above and this call, say() throws on abort and the Failure assistant append below never runs — could that leave the persisted history ending with a user message? An this.abort check after the first await might be worth it.
| // Error" toast is diagnosable without grepping sidecar logs afterward (issue-014). | ||
| const backoffDetailLines: string[] = [] | ||
| if (Array.isArray(error?.errorDetails) && error.errorDetails.length > 0) { | ||
| backoffDetailLines.push(`errorDetails: ${JSON.stringify(error.errorDetails)}`) |
There was a problem hiding this comment.
For a 429 with Google RPC details, would this stringify raw RetryInfo metadata into the toast? errorDetails is parsed as RetryInfo at line 4528 for delay extraction. Filtering those entries out before display might be cleaner.
| outputTokens: chunk.usage.output_tokens || 0, | ||
| // CLARITY PATCH: thread stop_reason through so Task.ts can surface it in | ||
| // empty-response diagnostics (issue-014). | ||
| finishReason: chunk.delta.stop_reason || undefined, |
There was a problem hiding this comment.
Only Anthropic populates finishReason, so the diagnostic will show finish_reason: unknown for every other provider. Should base-openai-compatible-provider.ts set it too (it already reads finish_reason), or should the JSDoc note this is Anthropic-only for now?
| if (Array.isArray(error?.errorDetails) && error.errorDetails.length > 0) { | ||
| backoffDetailLines.push(`errorDetails: ${JSON.stringify(error.errorDetails)}`) | ||
| } | ||
| if (error?.finishReason) { |
There was a problem hiding this comment.
Does anything attach finishReason to an error object? I could not find it — handleProviderError does not preserve it and all three call sites pass plain errors, so this branch never fires (the value already rides in emptyResponseDetail). Drop it?
| // same oversized request forever at the maximum backoff delay with no give-up condition. | ||
| // The 2026-08-03 FACE incident needed 4 retries (~7 min) before the provider recovered | ||
| // on its own; 5 gives one margin round without letting the loop run indefinitely. | ||
| // See docs/issues/issue-014-empty-response-infinite-retry-loop.md |
There was a problem hiding this comment.
docs/issues/issue-014-empty-response-infinite-retry-loop.md does not exist — can you just reference the issue and maybe include a comment on the issue.
github-actions
Bot
added
awaiting-author
2 participants
Fixes #1111
Problem statement
When a provider returns an empty assistant response — no text content and no tool calls — the model-response retry loop in Task.ts has no upper bound on retries. It resends the same unchanged request forever at the maximum exponential-backoff delay, with the only escape routes being manual user cancellation or the provider eventually returning valid content on its own.
We observed this live with Claude Sonnet during a large file-write task: the extension made 5 requests / 4 retries over ~7 minutes against a frozen ~160k-token payload, receiving empty end_turn responses each time and only recovering on the 5th attempt. Users also reported that pressing Stop did not reliably escape the loop, because the graceful cancel path rehydrated the same task with its still-oversized history and the loop resumed immediately.
Root cause
Fix summary
Three complementary changes:
Note: the trigger is a transient provider-side empty response, which is outside our control — this is a robustness improvement that bounds the loop and makes Stop reliable, not a cure for the provider issue.
Files changed
Summary by CodeRabbit