From cc6444cd16525aadabd465a8ea96289ac2969e95 Mon Sep 17 00:00:00 2001
From: Tim Erwin <tim.erwin@gmail.com>
Date: Thu, 3 Sep 2026 16:06:56 +0700
Subject: [PATCH] llm: IsTerminal must not kill the turn on recoverable
 failures
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 internal/llm/policy_test.go | 41 +++++++++++++++++++++++++++++++++++++
 internal/llm/recover.go     | 25 +++++++++++++++++++++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/internal/llm/policy_test.go b/internal/llm/policy_test.go
index eca74d3..7f0bf17 100644
--- a/internal/llm/policy_test.go
+++ b/internal/llm/policy_test.go
@@ -368,3 +368,44 @@ func TestRequestShapeErrorsAreTerminalButTimingErrorsWalk(t *testing.T) {
 		})
 	}
 }
+
+// TestIsTerminalExcludesRecoverableCauses: IsTerminal decides whether a failure
+// KILLS THE TURN, which is a stricter question than "should the fallback walk
+// stop here?" — the question terminalForFallback answers.
+//
+// Several causes stop the walk precisely because something else recovers them.
+// Aliasing the two would make a delegated worker's context overflow kill the
+// parent turn instead of compacting and retrying: the recovery would be
+// replaced by a hard failure, and the bug would look like the very hang this
+// classification exists to prevent.
+func TestIsTerminalExcludesRecoverableCauses(t *testing.T) {
+	shape := fmt.Errorf("gemini stream: %w", &genai.APIError{Code: 400, Message: "malformed"})
+
+	for _, tc := range []struct {
+		name string
+		err  error
+		want bool
+		why  string
+	}{
+		{"malformed request", shape, true, "fails identically on every model"},
+		{"unauthorized", wire.ErrUnauthorized, true, "needs the user to sign in"},
+		{"insufficient credit", wire.ErrInsufficientCredit, true, "needs the user to top up"},
+		{"context overflow", wire.ErrContextOverflow, false, "compact-and-retry recovers it"},
+		{"incomplete stream", wire.ErrStreamIncomplete, false, "the same call is retried"},
+		{"rate limited", fmt.Errorf("x: %w", &genai.APIError{Code: 429}), false, "timing, not shape"},
+		{"provider down", fmt.Errorf("x: %w", &genai.APIError{Code: 503}), false, "infrastructure"},
+		{"transport failure", errors.New("connection reset"), false, "infrastructure"},
+		{"nil", nil, false, "no failure at all"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := IsTerminal(tc.err); got != tc.want {
+				t.Errorf("IsTerminal = %v, want %v — %s", got, tc.want, tc.why)
+			}
+		})
+	}
+
+	// The lane-exhausted card is the user's decision, not a turn kill.
+	if IsTerminal(&provider.ErrLaneExhausted{}) {
+		t.Error("an exhausted lane raises the fallback-choice card; it must not kill the turn")
+	}
+}
diff --git a/internal/llm/recover.go b/internal/llm/recover.go
index ab1520a..7d79a68 100644
--- a/internal/llm/recover.go
+++ b/internal/llm/recover.go
@@ -41,7 +41,30 @@ const maxModelFallbacks = 2
 // thought-signature 400 was retried roughly 150 times over 12 minutes, because
 // nothing in the loop could tell "this tool had a bad day" from "this will
 // never work".
-func IsTerminal(err error) bool { return err != nil && terminalForFallback(err) }
+func IsTerminal(err error) bool {
+	if err == nil {
+		return false
+	}
+	// NOT an alias for terminalForFallback. That answers "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. Killing the turn on those would break the
+	// recovery instead of surfacing a cause.
+	for _, recoverable := range []error{
+		wire.ErrContextOverflow,  // compact-and-retry, inside the worker
+		wire.ErrStreamIncomplete, // same-call transport retry
+	} {
+		if errors.Is(err, recoverable) {
+			return false
+		}
+	}
+	var exh *provider.ErrLaneExhausted
+	if errors.As(err, &exh) {
+		return false // raises the fallback-choice card; the user decides
+	}
+	return terminalForFallback(err)
+}
 
 func terminalForFallback(err error) bool {
 	for _, sentinel := range []error{