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
1 participant
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