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
This commit is contained in:
roboomp
2026-08-16 21:16:35 +00:00
parent 37eee71978
commit 412d0e3b42
3 changed files with 148 additions and 1 deletions
+4
View File
@@ -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
@@ -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);
}
@@ -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<Api>): 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<Api>, 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<Extract<AgentSessionEvent, { type: "retry_fallback_applied" }>> = [];
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." }]);
});
});