fix(agent): retry empty provider turns instead of stopping silently - #10360
Conversation
An empty provider turn — no tool calls, no text, and no error — fell through the reply loop to a silent break: no message yielded, no retry, no telemetry. The session simply stopped as if finished, leaving the user with no response. This was observed across multiple providers, confirming it is goose-side rather than provider-specific. Guard the empty-turn case: retry the provider call a bounded number of times (MAX_EMPTY_TURN_RETRIES) without consuming the turn budget or polluting history, then surface a visible message so the user is never left staring at a stopped session. The guard excludes turns that legitimately produce no assistant output (provider errors, recovery compaction, pending steers). Fixes aaif-goose#10353 Assisted-by: Claude Code
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32ef1a1621
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
When the empty-turn retry budget is exhausted, the visible EMPTY_TURN_MESSAGE was emitted but last_assistant_text was left empty, so the stop hook received an empty last_assistant_message and trace output lost the final text. Set last_assistant_text to the fallback message, matching the MAX_TURNS_MESSAGE path. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31869baef9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@kojiromike really interesting and seems like something did change. I think if you address the codex stuff could do with this change. I little weird that we have to resort to this and I really wonder how it gets into the empty response state, but I assume you can easily repro and this fixes? |
The empty assistant response is appended to messages_to_add in the no-tool/no-text path before the post-stream empty-turn logic runs. The retry branch cleared it, but the exhausted-retry branch appended the fallback on top, so the session persisted an empty assistant turn immediately before the resend message — exactly the empty message strict providers reject. Clear messages_to_add before pushing the fallback, and assert in both empty-turn tests that no empty assistant message is persisted. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bde9787f0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@michaelneale hi, yes, I can repro and this fixes. I'll address codex. I am curious what changed recently that could cause this. If I figure it out, I'll LYK. |
The empty-turn predicate folded in the pending-steer check, so an empty provider response with a queued steer made empty_turn false and skipped the cleanup entirely — the pending-steer arm then persisted the empty assistant message ahead of the steer, the exact history strict providers reject. Split the concern: empty_response (drop the empty message, always) from empty_turn (retry/fallback, only when no steer is queued). A steer still takes over the turn, but the empty message is no longer persisted. Adds a regression test that queues a steer during an empty turn and asserts no empty assistant message is persisted. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33b518d2a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@kojiromike excellent - fascinated to know if you report back, nice work |
|
@kojiromike Thanks for finding and suggesting a fix. A question: can you reproduce it on a previous version of goose the same way as the one you’re using? If not, I’d love to see the results of a git bisect to find the commit that introduced the issue and think about a root cause fix If you can get all the way there it would be very good to know, but even if you just isolate a version that doesn’t reproduce the bug I could do a bisect tomorrow. |
The empty-turn retry/fallback was hoisted above the `match final_output` block, so it shadowed every arm below it: an empty turn pre-empted the final-output nudge, the steer arm, and the goal/grind nudges. This fixed each collision one predicate at a time and kept regressing. Restructure instead of patching symptoms: keep only the empty-message cleanup above the match (it must run regardless), and move the retry/ fallback decision into a `None if empty_response =>` arm. Match ordering now does the prioritization — final-output, recovery-compact, steer, and goal/grind all naturally win, and the empty-turn fallback fires only when nothing else claims the turn. Net simpler than the guard-per-branch form. Adds a regression test asserting a final-output recipe that stops without calling the tool gets the mandatory nudge, not the empty-turn fallback; verified it fails without the fix. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30b2ef169f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Idk if I'll get around to a full bisect, but I did externalize the repro by writing a python "openai server" that always returns an empty response. This is reliable in 1.41.0: https://gist.github.com/kojiromike/8c6f5b9b47c053e5b2a04af2f6676eda |
|
I'm not going to test this tonight, because it's late, but Claude suggests that a bisect won't work. It thinks that the current state was introduced in #4648, and prior to that an empty response would have caused a retry loop, effectively a hang. Basically #4648 traded an infinite-loop failure mode for a silent-exit failure mode. |
The empty-turn fallback arm shadowed the recipe retry arm, so a session with retry_config skipped its success checks, on_failure, and max_retries whenever the model returned an empty turn. A recipe whose check already passed was reported as an empty model response, and a failing recipe never performed its configured retry. Route the empty turn through handle_retry_logic first and return the granular RetryResult: recipe retry owns the turn whenever it is configured, and the empty-turn fallback fires only on RetryResult::Skipped (no recipe retry). Add test_empty_turn_defers_to_recipe_retry covering a passing-check recipe; verified it fails against the old shadowing order. Assisted-by: Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b786c57d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ntly RetryManager appended the "Maximum retry attempts exceeded" message directly onto the conversation, but the agent only yields and persists messages_to_add — so on RetryResult::MaxAttemptsReached the failure was neither shown to the user nor saved, and (with messages_to_add cleared for an empty turn) the recipe stopped silently. Carry the failure message on the MaxAttemptsReached variant and route it through the normal messages_to_add yield/persist path, matching every other completion branch. Add test_recipe_max_retries_surfaces_failure (failing check + exhausted retries must surface and persist the message); verified it fails against the silent-stop behavior. Assisted-by: Claude Code
|
@kojiromike @alexhancock that sqlite one is too old to be cause of this, 9 months ago so that wouldn't be it (sorry claude! claude in particular is very very bad at understanding time and then rat-holing on a solution). Would be curious if previous release does this or not basically, I wouldn't bisect very far. The only thing in recent memory that burned me with stalling was when some models had their response token limit set too low - it would just die (but I don't think it would show up as empty... probably?). but it would show like this, just an FYI. I think this is probably a good fix, but would be really nice to know if the immediate release or not had this. |
|
I haven't been using goose for 9 months, but I actually saw stalling heavily when I first started using it, maybe 4 months ago. I actually didn't use goose much for a while because of it, but I could easily believe this bug has been around for a while. |
|
Yeah I think this is ok with me. Not a huge change and may solve some things. Really interested why. @alexhancock you want to merge in? |
3 participants
Problem
goose sessions sometimes stop with no response — the agent does some work (or nothing) and then halts as if finished, without emitting any message. An empty provider turn (no tool calls, no text, and no error) falls through the reply loop to a silent
break: nothing is yielded to the user, no retry happens, and no telemetry is recorded.Investigating a local session store, this signature appeared across sessions from four different providers (gcp_vertex_ai, lmstudio, ollama, direct anthropic), confirming the defect is goose-side rather than provider-specific.
Fixes #10353. Related: #6293, #10351, #9082.
Fix
Guard the empty-turn case in the reply loop:
MAX_EMPTY_TURN_RETRIES = 3) without consuming the turn budget or persisting the empty assistant message into history (strict providers reject empty turns).EMPTY_TURN_MESSAGEso the user is never left staring at a stopped session.Tests
Added
mod empty_turn_testsincrates/goose/tests/agent.rswith a mock provider that returns empty turns before recovering:test_empty_turn_retries_then_recovers— a transient empty response is retried and the real response is ultimately delivered.test_persistent_empty_turn_surfaces_message— a provider that only ever returns empty responses surfaces a visible message instead of hanging silently.Verified:
cargo build,cargo fmt,cargo clippy -p goose --all-targets -- -D warnings, and the fullcargo test -p goose --test agentsuite all pass.