Uh oh!
There was an error while loading. Please reload this page.
IsTerminal must not kill the turn on recoverable failures - #9
Open
timothyerwin wants to merge 1 commit into
Open
IsTerminal must not kill the turn on recoverable failures#9timothyerwin wants to merge 1 commit into
timothyerwin wants to merge 1 commit into
Conversation
IsTerminal was an alias for terminalForFallback, but the two answer different questions. terminalForFallback asks "should the fallback walk stop here?", and several of its cases stop the walk precisely BECAUSE something else recovers them: a context overflow compacts and retries, an incomplete stream retries the same call, an exhausted lane raises the user's fallback-choice card. IsTerminal decides whether a delegated worker's failure ENDS THE TURN, which is strictly narrower. As written, a worker that merely overflowed its context would kill the parent turn instead of compacting — replacing a working recovery with a hard failure, and looking very much like the hang this classification was added to prevent. Shipped in v0.31.0; the new table test fails against that build and passes here.
There was a problem hiding this comment.
Walkthrough
Separates IsTerminal from terminalForFallback so recoverable errors do not abort the parent agent turn. Worker failures caused by context overflow, incomplete streams, or exhausted lanes now remain recoverable rather than triggering turn-fatal errors.
- Explicitly filters out wire.ErrContextOverflow, wire.ErrStreamIncomplete, and provider.ErrLaneExhausted before delegating to terminalForFallback.
- Adds TestIsTerminalExcludesRecoverableCauses covering malformed requests, auth issues, context overflows, rate limits, transport failures, and lane exhaustion.
The change cleanly isolates turn-killing terminal errors from fallback-stopping conditions and is well-covered by targeted tests.
No findings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A real bug in v0.31.0, found while setting up a test PR to verify the reviewer.
The bug
v0.31.0 added
IsTerminalso a delegated worker's terminal failure ends the turn instead of coming back as a retryable tool error the model retries forever. It was written as a one-line alias:But those two functions answer different questions.
terminalForFallbackasks "should the fallback walk stop here?" Several of its cases stop the walk precisely because something else recovers them:ErrContextOverflowrunLoopErrStreamIncompleteErrLaneExhaustedIsTerminalasks the narrower question: "does this end the turn?" Aliasing them means a worker that merely overflowed its context now kills the parent turn instead of compacting — replacing a working recovery with a hard failure.The irony is that the failure would look like a sub-agent dying abruptly on a long task, which is close to the hang this classification was added to prevent.
The fix
IsTerminalexcludes the recoverable causes explicitly, then defers toterminalForFallbackfor the rest. Request-shape 4xx, auth, and billing states stay terminal; overflow, stream cuts, rate limits, 5xx and transport failures do not.Verification
The new table test fails against the shipped v0.31.0 build (
context overflowandincomplete streamboth reportIsTerminal = true) and passes here. Full gate clean:gofmt,go vet,go test -race ./....Why this PR exists
Opened partly as a live end-to-end check that PR review works again after the Gemini thought-signature fix chain (#8, released as v0.31.0, reviewer image rebuilt at that tag). It turned into a real fix, which is a better test than a dummy edit.