diff --git a/changelog.d/features/13868-strip-thinking-on-model-rejection.md b/changelog.d/features/13868-strip-thinking-on-model-rejection.md new file mode 100644 index 00000000000..938cd3bd561 --- /dev/null +++ b/changelog.d/features/13868-strip-thinking-on-model-rejection.md @@ -0,0 +1 @@ +- **feat(sse):** a request no longer dies when the upstream rejects thinking by naming the model instead of the offending field. OmniRoute now drops whichever reasoning field the body actually carries (`reasoning_effort`, `reasoning`, `thinking`, `think`) and retries once. Ollama rejects this way for Instruct-only models such as Qwen3-Coder, where the 400 reads `"Qwen3-Coder:latest" does not support thinking`. Neither existing detector could see anything to strip there, so the error went straight back to the client and every Claude Code request carrying a thinking block failed against a self-hosted non-thinking model. ([#13868](https://github.com/diegosouzapw/OmniRoute/pull/13868)) diff --git a/open-sse/config/providerFieldStrips.ts b/open-sse/config/providerFieldStrips.ts index dfa4344358d..f2419058047 100644 --- a/open-sse/config/providerFieldStrips.ts +++ b/open-sse/config/providerFieldStrips.ts @@ -53,6 +53,33 @@ export function detectUnsupportedParam(bodyText: string): string | null { return match?.[1] ?? null; } +/** + * Reasoning fields a request can carry into an OpenAI-compatible upstream. Ordered + * most to least common so the retry drops the real one first when several are set. + */ +export const REASONING_REQUEST_FIELDS: readonly string[] = [ + "reasoning_effort", + "reasoning", + "thinking", + "think", +]; + +/** + * Some upstreams reject thinking by naming the MODEL, not the offending field, so + * neither `findOffendingField` nor `detectUnsupportedParam` can see anything to strip. + * Ollama does this for non-thinking models such as Qwen3-Coder (an Instruct-only + * build): `"Qwen3-Coder:latest" does not support thinking`. Matches: + * - `"" does not support thinking` + * - `model X does not support reasoning` + */ +export const UNSUPPORTED_THINKING_RE = /does\s+not\s+support\s+(?:thinking|reasoning)\b/i; + +/** True when a 400 body says the model has no thinking mode at all. */ +export function isUnsupportedThinkingError(bodyText: string): boolean { + if (typeof bodyText !== "string" || !bodyText) return false; + return UNSUPPORTED_THINKING_RE.test(bodyText); +} + /** Immutably drop request fields Groq rejects with a 400. */ export function stripGroqUnsupportedFields>(body: T): T { if (!body || typeof body !== "object") return body; diff --git a/open-sse/executors/base.ts b/open-sse/executors/base.ts index 7f07fa56a3a..9f034652260 100644 --- a/open-sse/executors/base.ts +++ b/open-sse/executors/base.ts @@ -17,6 +17,8 @@ import { createCopilotIdentityFallback } from "./copilotIdentityFallback.ts"; import { findOffendingField, detectUnsupportedParam, + isUnsupportedThinkingError, + REASONING_REQUEST_FIELDS, stripGroqUnsupportedFields, } from "../config/providerFieldStrips.ts"; import { @@ -1581,6 +1583,11 @@ export class BaseExecutor { .text() .catch(() => ""); const offending = findOffendingField(errText); + const thinkingFields = REASONING_REQUEST_FIELDS.filter( + (field) => + !strippedFields.has(field) && + (transformedBody as Record)[field] !== undefined + ); if ( offending && !strippedFields.has(offending) && @@ -1597,6 +1604,23 @@ export class BaseExecutor { `Upstream 400 rejected ${offending} on ${url} — retrying without it` ); response = await fetchWithStartTimeout(url, { ...fetchOptions, body: retryBody }); + } else if (isUnsupportedThinkingError(errText) && thinkingFields.length > 0) { + // Ollama names the MODEL, not the field, as thinking-unsupported (e.g. + // Qwen3-Coder). When no reasoning field is present (e.g. Gemini's nested + // generationConfig.thinkingConfig), this falls through to auto-learn below. + for (const field of thinkingFields) { + strippedFields.add(field); + delete (transformedBody as Record)[field]; + } + let retryBody = JSON.stringify(transformedBody); + if (usesClaudeCodeProtocol || this.provider === "claude") { + retryBody = await signRequestBody(retryBody); + } + log?.info?.( + "THINKING_UNSUPPORTED", + `Upstream 400: ${model} has no thinking mode on ${url} — retrying without ${thinkingFields.join(", ")}` + ); + response = await fetchWithStartTimeout(url, { ...fetchOptions, body: retryBody }); } else { // Auto-learn: detect "Unsupported parameter" errors and persist to DB // when the provider config has autoLearn enabled (#6625). diff --git a/tests/unit/base-thinking-unsupported-retry.test.ts b/tests/unit/base-thinking-unsupported-retry.test.ts new file mode 100644 index 00000000000..27a2855ebc1 --- /dev/null +++ b/tests/unit/base-thinking-unsupported-retry.test.ts @@ -0,0 +1,163 @@ +/** + * TDD for #13868 — BaseExecutor's 400-recovery chain must retry once, stripping the + * request's reasoning field, when an upstream rejects thinking by naming the MODEL + * rather than the offending field (Ollama does this for Instruct-only models such as + * Qwen3-Coder). Neither `findOffendingField` nor `detectUnsupportedParam` can see + * anything to strip in that error shape, so this exercises the dedicated + * `isUnsupportedThinkingError` branch end-to-end through the real retry chain. + * + * Mirrors the fetch-capture pattern in context-editing-relays.test.ts. + * + * Run: node --import tsx/esm --test tests/unit/base-thinking-unsupported-retry.test.ts + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { DefaultExecutor } from "../../open-sse/executors/default.ts"; +import { setGlobalAutoLearnEnabled } from "../../src/lib/db/paramFilters.ts"; + +/** First call returns `status` with `errorText`; subsequent calls return 200 OK. */ +function mockFetchErrorThenOk(status: number, errorText: string) { + const bodies: Array> = []; + const original = globalThis.fetch; + let calls = 0; + globalThis.fetch = (async (_url: unknown, init: { body?: unknown } = {}) => { + bodies.push(JSON.parse(String(init.body ?? "{}"))); + calls += 1; + if (calls === 1) { + return new Response(errorText, { + status, + headers: { "Content-Type": "application/json" }, + }); + } + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof globalThis.fetch; + return { bodies, callCount: () => calls, restore: () => void (globalThis.fetch = original) }; +} + +const baseCredentials = { + apiKey: "relay-key", + providerSpecificData: { baseUrl: "https://relay.example/v1" }, +}; + +test("#13868: upstream 400 naming the model (not the field) as thinking-unsupported strips reasoning_effort and retries once", async () => { + const { bodies, callCount, restore } = mockFetchErrorThenOk( + 400, + '"Qwen3-Coder:latest" does not support thinking' + ); + try { + await new DefaultExecutor("anthropic-compatible-cc-myrelay").execute({ + model: "Qwen3-Coder:latest", + body: { + model: "Qwen3-Coder:latest", + messages: [{ role: "user", content: "hi" }], + max_tokens: 1, + reasoning_effort: "high", + }, + stream: false, + credentials: baseCredentials, + }); + } finally { + restore(); + } + assert.equal(callCount(), 2, "must retry exactly once after the thinking-unsupported 400"); + assert.equal(bodies[0]?.reasoning_effort, "high", "first attempt carried reasoning_effort"); + assert.equal( + "reasoning_effort" in (bodies[1] ?? {}), + false, + "retry must drop reasoning_effort entirely" + ); +}); + +test("#13868: the retry strips only the reasoning field actually present, not every REASONING_REQUEST_FIELDS entry", async () => { + const { bodies, callCount, restore } = mockFetchErrorThenOk( + 400, + "model gemma3 does not support reasoning" + ); + try { + await new DefaultExecutor("anthropic-compatible-cc-myrelay").execute({ + model: "gemma3", + body: { + model: "gemma3", + messages: [{ role: "user", content: "hi" }], + max_tokens: 1, + thinking: { type: "enabled" }, + }, + stream: false, + credentials: baseCredentials, + }); + } finally { + restore(); + } + assert.equal(callCount(), 2, "must retry exactly once"); + assert.equal("thinking" in (bodies[1] ?? {}), false, "retry must drop the present field"); + assert.equal( + "reasoning_effort" in (bodies[1] ?? {}), + false, + "a field never sent must not appear on retry either" + ); +}); + +test("#13868: an UNRELATED 400 does NOT strip reasoning fields or retry", async () => { + const { bodies, callCount, restore } = mockFetchErrorThenOk(400, "max_tokens: must be >= 1"); + try { + await new DefaultExecutor("anthropic-compatible-cc-myrelay").execute({ + model: "Qwen3-Coder:latest", + body: { + model: "Qwen3-Coder:latest", + messages: [{ role: "user", content: "hi" }], + max_tokens: 1, + reasoning_effort: "high", + }, + stream: false, + credentials: baseCredentials, + }); + } finally { + restore(); + } + assert.equal(callCount(), 1, "an unrelated 400 must not trigger the thinking-unsupported retry"); + assert.equal(bodies[0]?.reasoning_effort, "high", "the single attempt still carried the field"); +}); + +test("#13868: a thinking-unsupported 400 with NO reasoning field on the body falls through to auto-learn instead of silently no-oping", async () => { + // Gemini-shaped case: the request carries no top-level REASONING_REQUEST_FIELDS entry + // (its thinking config is nested under generationConfig.thinkingConfig), so the + // isUnsupportedThinkingError branch has nothing to strip. Before this fix, that branch + // had no `else`, so an upstream 400 matching BOTH signals (thinking-unsupported wording + // AND a literal "Unsupported parameter" name) would still fall into the dead-end `if` + // and never reach the auto-learn detector below it, even though auto-learn is enabled. + setGlobalAutoLearnEnabled(true); + const { bodies, callCount, restore } = mockFetchErrorThenOk( + 400, + "model does not support thinking; Unsupported parameter: generationConfig" + ); + try { + await new DefaultExecutor("anthropic-compatible-cc-myrelay").execute({ + model: "gemini-shaped-model", + body: { + model: "gemini-shaped-model", + messages: [{ role: "user", content: "hi" }], + max_tokens: 1, + generationConfig: { thinkingConfig: { thinkingBudget: 100 } }, + }, + stream: false, + credentials: baseCredentials, + }); + } finally { + restore(); + setGlobalAutoLearnEnabled(false); + } + assert.equal( + callCount(), + 2, + "must fall through to auto-learn and retry once the thinking branch finds nothing to strip" + ); + assert.equal( + "generationConfig" in (bodies[1] ?? {}), + false, + "auto-learn must have stripped the field it detected" + ); +}); diff --git a/tests/unit/provider-field-strips.test.ts b/tests/unit/provider-field-strips.test.ts index ffcbb869327..4bbbee1379e 100644 --- a/tests/unit/provider-field-strips.test.ts +++ b/tests/unit/provider-field-strips.test.ts @@ -3,6 +3,8 @@ import assert from "node:assert/strict"; import { findOffendingField, + isUnsupportedThinkingError, + REASONING_REQUEST_FIELDS, stripGroqUnsupportedFields, } from "../../open-sse/config/providerFieldStrips.ts"; @@ -80,3 +82,31 @@ test("stripGroqUnsupportedFields drops unsupported messages[].model and other me assert.equal("messageId" in out.messages[1], false); assert.equal("sender" in out.messages[1], false); }); + +test("isUnsupportedThinkingError matches upstreams that name the model, not the field", () => { + // Ollama's exact wording for an Instruct-only model. + assert.equal(isUnsupportedThinkingError('"Qwen3-Coder:latest" does not support thinking'), true); + assert.equal(isUnsupportedThinkingError("model gemma3 does not support reasoning"), true); + assert.equal(isUnsupportedThinkingError("does not\n support\tthinking"), true); + assert.equal(isUnsupportedThinkingError("DOES NOT SUPPORT THINKING"), true); +}); + +test("isUnsupportedThinkingError ignores unrelated and near-miss 400 bodies", () => { + assert.equal(isUnsupportedThinkingError("does not support tools"), false); + assert.equal(isUnsupportedThinkingError("does not support thinkingly"), false); + assert.equal(isUnsupportedThinkingError("supports thinking"), false); + assert.equal(isUnsupportedThinkingError("rate limit exceeded"), false); + assert.equal(isUnsupportedThinkingError(""), false); + assert.equal(isUnsupportedThinkingError(undefined), false); +}); + +test("neither existing detector can see the model-named thinking rejection", () => { + // The reason this branch exists: findOffendingField needs a literal field name. + assert.equal(findOffendingField('"Qwen3-Coder:latest" does not support thinking'), null); +}); + +test("REASONING_REQUEST_FIELDS covers the fields a request can carry", () => { + for (const field of ["reasoning_effort", "reasoning", "thinking", "think"]) { + assert.ok(REASONING_REQUEST_FIELDS.includes(field), `missing ${field}`); + } +});