From 412d0e3b42dfce308241a7e372d993d447bedfa0 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 16 Aug 2026 21:16:35 +0000 Subject: [PATCH] fix(coding-agent): keep thinking-loop retries on the same model A ThinkingLoop abort is the loop guard asking for a same-model resample (it injects a thinking-loop-redirect notice that only makes sense on the model that looped), not a provider failure. #handleRetryableError routed it through the generic retryable-error branch, so on attempt 1 it called noteRetryFallbackCooldown + #tryRetryModelFallback and could switch to another family from retry.fallbackChains while parking the original selector on a 5-minute cooldown. A healthy Grok 4.6 planning turn got replaced by whatever the chain listed next. Carve ThinkingLoop out of the model-fallback branch and out of the Fireworks Fast->base degrade so the loop guard always re-samples the same model; the retry budget still bounds a genuinely stuck stream. Fixes #8760 --- packages/coding-agent/CHANGELOG.md | 4 + .../coding-agent/src/session/turn-recovery.ts | 18 ++- .../test/agent-session-retry-fallback.test.ts | 127 ++++++++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4901d84f7..616f2657a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed thinking-loop aborts (`AIError.Flag.ThinkingLoop`) walking `retry.fallbackChains` and switching to another model family on attempt 1, so a healthy planning turn on Grok 4.6 (SuperGrok / Cursor OAuth) no longer gets replaced by whatever the chain lists next. The loop guard now re-samples the same model with its `thinking-loop-redirect` notice, and no longer parks the model selector on a fallback cooldown. ([#8760](https://github.com/can1357/oh-my-pi/issues/8760)) + ## [17.3.5] - 2026-08-16 ### Added diff --git a/packages/coding-agent/src/session/turn-recovery.ts b/packages/coding-agent/src/session/turn-recovery.ts index a07cb117e..ef729add2 100644 --- a/packages/coding-agent/src/session/turn-recovery.ts +++ b/packages/coding-agent/src/session/turn-recovery.ts @@ -1660,6 +1660,9 @@ export class TurnRecovery { if (AIError.isContextOverflow(message, model.contextWindow ?? 0)) return false; if (AIError.is(id, AIError.Flag.UsageLimit)) return false; if (AIError.is(id, AIError.Flag.AuthFailed)) return false; + // A thinking loop is a same-model resample signal, not a router fault, so a + // base-model swap would abandon the loop-guard redirect (issue #8760). + if (AIError.is(id, AIError.Flag.ThinkingLoop)) return false; return this.#host.modelRegistry.find("fireworks", toFireworksBaseModelId(model.id)) !== undefined; } @@ -1969,10 +1972,23 @@ export class TurnRecovery { ); if (switchedCredential) delayMs = 0; } + // A thinking-loop abort is not a provider failure — it is the loop guard + // asking for a same-model resample, paired with a hidden + // `thinking-loop-redirect` notice that only makes sense on the model that + // looped. Walking `fallbackChains` (or parking the selector on a cooldown) + // would swap a healthy planning turn to another family based on chain + // contents, not model health (issue #8760). Keep it on the same model; the + // retry budget still bounds a genuinely stuck stream. + const thinkingLoop = AIError.is(id, AIError.Flag.ThinkingLoop); if (!staleOpenAIResponsesReplayError && !switchedCredential && currentSelector) { // A refusal chain stops at the retry budget: the exhausted-attempt // last resort is for provider failures, not classifier decisions. - if (allowModelFallback && retrySettings.modelFallback && !(retryBudgetExhausted && classifierRefusal)) { + if ( + allowModelFallback && + retrySettings.modelFallback && + !thinkingLoop && + !(retryBudgetExhausted && classifierRefusal) + ) { if (!classifierRefusal) { this.noteRetryFallbackCooldown(currentSelector, parsedRetryAfterMs, errorMessage); } diff --git a/packages/coding-agent/test/agent-session-retry-fallback.test.ts b/packages/coding-agent/test/agent-session-retry-fallback.test.ts index 39e6662fe..577fd2a6a 100644 --- a/packages/coding-agent/test/agent-session-retry-fallback.test.ts +++ b/packages/coding-agent/test/agent-session-retry-fallback.test.ts @@ -4,6 +4,7 @@ import { scheduler } from "node:timers/promises"; import { type } from "@oh-my-pi/omptype"; import { Agent, type AgentTool } from "@oh-my-pi/pi-agent-core"; import { + type Api, type AssistantMessage, Effort, type Model, @@ -12,6 +13,7 @@ import { } from "@oh-my-pi/pi-ai"; import * as AIError from "@oh-my-pi/pi-ai/error"; import { createMockModel } from "@oh-my-pi/pi-ai/providers/mock"; +import { AssistantMessageEventStream } from "@oh-my-pi/pi-ai/utils/event-stream"; import { buildModel } from "@oh-my-pi/pi-catalog/build"; import { writeModelCache } from "@oh-my-pi/pi-catalog/model-cache"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; @@ -88,6 +90,63 @@ function createFallbackAgent( }); } +function emptyUsage(): AssistantMessage["usage"] { + return { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }; +} + +/** A stream that terminates with a `ThinkingLoop`-flagged error, exactly as the + * loop guard aborts a repetitive reasoning stream (issue #8760). */ +function thinkingLoopErrorStream(model: Model): AssistantMessageEventStream { + const stream = new AssistantMessageEventStream(); + queueMicrotask(() => { + const partial: AssistantMessage = { + role: "assistant", + content: [], + api: model.api, + provider: model.provider, + model: model.id, + usage: emptyUsage(), + stopReason: "error", + errorMessage: + "Thinking loop detected: the model repeated near-identical content (4 near-identical segments within the last 16). Treating as a stream stall and retrying.", + errorId: AIError.create(AIError.Flag.ThinkingLoop), + timestamp: Date.now(), + }; + stream.push({ type: "error", reason: "error", error: partial }); + }); + return stream; +} + +/** A stream that completes normally with a single text block. */ +function recoveredTextStream(model: Model, text: string): AssistantMessageEventStream { + const stream = new AssistantMessageEventStream(); + queueMicrotask(() => { + const partial: AssistantMessage = { + role: "assistant", + content: [{ type: "text", text }], + api: model.api, + provider: model.provider, + model: model.id, + usage: emptyUsage(), + stopReason: "stop", + timestamp: Date.now(), + }; + stream.push({ type: "start", partial }); + stream.push({ type: "text_start", contentIndex: 0, partial }); + stream.push({ type: "text_delta", contentIndex: 0, delta: text, partial }); + stream.push({ type: "text_end", contentIndex: 0, content: text, partial }); + stream.push({ type: "done", reason: "stop", message: partial }); + }); + return stream; +} + describe("AgentSession retry fallback", () => { let tempDir: TempDir; let authStorage: AuthStorage; @@ -4938,4 +4997,72 @@ describe("AgentSession retry fallback", () => { isFallback: true, }); }); + + // A thinking-loop abort is a same-model resample signal (the guard pairs it + // with a `thinking-loop-redirect` notice), not a provider failure. It must + // not walk `retry.fallbackChains` or park the current selector on a cooldown, + // or a healthy planning turn gets replaced by another family (issue #8760). + it("retries the same model on a thinking-loop error instead of switching via fallback", async () => { + const primaryModel = getBundledModel("anthropic", "claude-sonnet-4-5"); + const fallbackModel = getBundledModel("openai", "gpt-4o-mini"); + if (!primaryModel || !fallbackModel) { + throw new Error("Expected bundled test models to exist"); + } + const primarySelector = `${primaryModel.provider}/${primaryModel.id}`; + const fallbackSelector = `${fallbackModel.provider}/${fallbackModel.id}`; + + const requestedModels: string[] = []; + const agent = new Agent({ + getApiKey: model => `${model.provider}-test-key`, + initialState: { model: primaryModel, systemPrompt: ["Test"], tools: [], messages: [] }, + streamFn: model => { + requestedModels.push(`${model.provider}/${model.id}`); + return requestedModels.length === 1 + ? thinkingLoopErrorStream(model) + : recoveredTextStream(model, "Recovered on the same model."); + }, + }); + + const settings = Settings.isolated({ + "compaction.enabled": false, + "retry.baseDelayMs": 0, + "retry.maxRetries": 2, + "retry.modelFallback": true, + "retry.fallbackChains": { default: [fallbackSelector] }, + "model.loopGuard.enabled": true, + }); + settings.setModelRole("default", primarySelector); + + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings, + modelRegistry, + }); + + const fallbackAppliedEvents: Array> = []; + const retryStartEvents: AutoRetryStartEvent[] = []; + session.subscribe(event => { + if (event.type === "retry_fallback_applied") fallbackAppliedEvents.push(event); + if (event.type === "auto_retry_start") retryStartEvents.push(event); + }); + vi.spyOn(scheduler, "wait").mockResolvedValue(undefined); + + await session.prompt("Plan the ticket, then act"); + await session.waitForIdle(); + + // The fallback chain lists a different family, but the thinking-loop abort + // re-samples the SAME model: no chain consult, no model switch. + expect(requestedModels).toEqual([primarySelector, primarySelector]); + expect(session.model?.provider).toBe(primaryModel.provider); + expect(session.model?.id).toBe(primaryModel.id); + expect(fallbackAppliedEvents).toHaveLength(0); + // The abort is a thinking-loop, and the retry stayed on the same model. + expect(retryStartEvents).toHaveLength(1); + expect(AIError.is(retryStartEvents[0].errorId, AIError.Flag.ThinkingLoop)).toBe(true); + // The selector must not be parked on a fallback cooldown by the abort. + expect(modelRegistry.isSelectorSuppressed(primarySelector)).toBe(false); + const finalAssistant = getLastAssistantMessage(session); + expect(finalAssistant.content).toEqual([{ type: "text", text: "Recovered on the same model." }]); + }); });