fix(agent): tolerate empty final response after completed tool rounds#63
fix(agent): tolerate empty final response after completed tool rounds#63LukasParke wants to merge 3 commits into
Conversation
Mini-class models intermittently return an empty final turn when the tool call was the answer; the SDK threw away successful runs. Now: retry the follow-up once; if still empty, resolve with empty text. strictFinalResponse: true restores the old throw. Runs with no completed tool rounds still throw.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 New client-only option leaks into API requests when no async parameter functions are used
The new strictFinalResponse option is sent to the API as an unknown field (resolveRequestForContext at packages/agent/src/lib/model-result.ts:1367-1375) because the non-async code path only strips six client-only fields, while the async path (packages/agent/src/lib/async-params.ts:197-208) correctly strips all ten.
Impact: An unrecognized field is included in every API request when strictFinalResponse is set without any async parameter functions, which could cause unexpected API errors.
Mismatch between async and non-async field stripping
The async path in resolveAsyncFunctions (packages/agent/src/lib/async-params.ts:197-208) uses a clientOnlyFields set that includes strictFinalResponse, allowFinalResponse, onTurnStart, onTurnEnd, and sharedContextSchema. The non-async path in resolveRequestForContext (packages/agent/src/lib/model-result.ts:1367-1375) only destructures out stopWhen, state, requireApproval, approveToolCalls, rejectToolCalls, and context. The remaining client-only fields (including the newly added strictFinalResponse) pass through into this.resolvedRequest and are sent to the API via betaResponsesSend.
(Refers to lines 1367-1375)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (canTolerateEmptyFinal && isEmptyOutput) { | ||
| const turnNumber = this.allToolExecutionRounds.length + 1; | ||
| currentResponse = await this.retryCurrentRequest(turnNumber); | ||
| } |
There was a problem hiding this comment.
🔴 Retry response after an empty final output is not persisted to conversation state
The retried response is not saved to state (retryCurrentRequest at packages/agent/src/lib/model-result.ts:2192) unlike every other API response in the tool loop, so stateful conversations lose the final turn's content.
Impact: When using state persistence, the retry response is silently dropped from the conversation history, causing data loss on resume.
Missing saveResponseToState call after retry
Every other API response in executeToolsIfNeeded is persisted via saveResponseToState — see packages/agent/src/lib/model-result.ts:1945 (initial response), packages/agent/src/lib/model-result.ts:2091 (follow-up responses), and packages/agent/src/lib/model-result.ts:2178 (final response after stopWhen). However, after the retry at line 2192, saveResponseToState is never called before markStateComplete at line 2202. This means the retry response (which becomes this.finalResponse) is not recorded in the conversation state.
| if (canTolerateEmptyFinal && isEmptyOutput) { | |
| const turnNumber = this.allToolExecutionRounds.length + 1; | |
| currentResponse = await this.retryCurrentRequest(turnNumber); | |
| } | |
| if (canTolerateEmptyFinal && isEmptyOutput) { | |
| const turnNumber = this.allToolExecutionRounds.length + 1; | |
| currentResponse = await this.retryCurrentRequest(turnNumber); | |
| await this.saveResponseToState(currentResponse); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
…rip all client-only fields in non-async path - retryCurrentRequest result now goes through saveResponseToState like every other response in the loop (stateful conversations were silently losing the final turn on resume) - resolveRequestForContext now strips the same client-only field set as async-params.ts clientOnlyFields (strictFinalResponse, allowFinalResponse, sharedContextSchema, onTurnStart, onTurnEnd were leaking into API requests on the non-async path) - regression tests for both
|
Both findings addressed in 5e4a324:
Suite: 27 files / 303 tests, typecheck + lint green. |
|
Reviewed One pre-existing edge for a possible follow-up (not this PR): in the plain path, an empty-final retry still carries tools — a fresh Suite verified: 27 files / 304 tests green. |
![perry-the-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/3222961?s=60&v=4){kind=small}
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions
Details
Solid, well-tested layered fix for the mini-model "tool call was the answer" failure mode. The tolerance is correctly gated on allToolExecutionRounds.length > 0, the retry-once-then-accept flow is clean, and the retried response is persisted to state so resumes don't lose the final turn. Also a nice catch adding validateFinalResponse to the two early no-tool exits, which previously skipped validation entirely. Opt-out via strictFinalResponse and the client-only-field stripping are both correct and tested (7/7 green locally).
One real question about the retry path — see the inline comment on retryCurrentRequest. Not a blocker, but worth confirming before merge.
| throw new Error('Request not initialized'); | ||
| } | ||
|
|
||
| const newRequest: models.ResponsesRequest = { |
There was a problem hiding this comment.
retryCurrentRequest re-sends this.resolvedRequest verbatim, which on the natural-loop-completion path still carries tools/toolChoice/parallelToolCalls (only makeFinalResponseRequest, used on the stopWhen path, strips them). So a retry after an empty final can legitimately come back with a fresh function_call rather than text. That output is non-empty, so validateFinalResponse passes, getText() returns '', and the newly-proposed tool call is silently dropped (never executed, no matching output persisted). Two questions: (1) is dropping a proposed tool call on the retry acceptable, or should the retry strip tools like makeFinalResponseRequest does to force a text turn? (2) worth a test where the empty-final retry returns a function_call to pin the intended behavior.
▶ Prompt for agents: In retryCurrentRequest, strip tools/toolChoice/parallelToolCalls from the re-sent request (mirroring makeFinalResponseRequest) so the retry coerces a text turn instead of possibly emitting an unexecuted tool call, OR document why carrying tools through is intended. Add a unit test covering an empty-final retry that returns a function_call.
1 participant
Problem
When a run's terminal action is a tool call (post a comment, file an entry), mini-class models intermittently return an empty final turn —
validateFinalResponsethrowsInvalid final response: empty or invalid output, reporting a successful run (side effect executed, cost incurred) as a failure. Reproduced 2/3 runs on gpt-5.4-mini (agent-monorepo pr-review FRICTION #8).Fix (layered)
getText()→''(response, usage, tool results, state intact)strictFinalResponse: trueopt-in restores old throwTests
4 new cases in
tool-terminal-empty-final.test.ts; suite 301 green. Changeset: patch.