diff --git a/CHANGELOG.md b/CHANGELOG.md index d560e5971f..ad46477413 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,9 @@ ## [3.8.20] — Unreleased -_Development cycle in progress._ +### 🔧 Bug Fixes + +- **fix(routing):** combo model substitution no longer forwards a client `thinking:{type:"disabled"}` to a target model that rejects it — when a combo/route swaps the upstream model (e.g. `claude-opus-4-8` → `claude-fable-5`), OmniRoute now strips the now-invalid `thinking.type:"disabled"` for models flagged `rejectsThinkingDisabled` (Fable 5 defaults to adaptive and rejects it), preventing the upstream 400 that silently broke Claude Code's internal title/name-generation calls. Models that accept `disabled` (opus/sonnet) are untouched. ([#3554](https://github.com/diegosouzapw/OmniRoute/issues/3554)) --- diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 83d69993b5..2f911bfb6c 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -34,6 +34,7 @@ import { import { resolveModelAlias } from "../services/modelDeprecation.ts"; import { getUnsupportedParams } from "../config/providerRegistry.ts"; import { supportsMaxTokens } from "@/lib/modelCapabilities.ts"; +import { normalizeThinkingForModel } from "@/shared/constants/modelSpecs.ts"; import { buildErrorBody, createErrorResult, @@ -3553,6 +3554,14 @@ export async function handleChatCore({ } translatedBody.model = finalModelToUpstream; + // #3554: a combo/route may substitute the upstream model AFTER the client chose its + // `thinking` value. Claude Code sends `thinking:{type:"disabled"}` for internal calls, + // which claude-fable-5 (adaptive-only) rejects with a 400. Drop the now-invalid value + // when the resolved target model rejects it; models that accept `disabled` are untouched. + if (typeof finalModelToUpstream === "string") { + translatedBody = normalizeThinkingForModel(translatedBody, finalModelToUpstream); + } + const previousResponseIdPolicy = applyResponsesPreviousResponseIdPolicy(translatedBody, { mode: settings.responsesPreviousResponseIdMode, sourceFormat, diff --git a/src/shared/constants/modelSpecs.ts b/src/shared/constants/modelSpecs.ts index 0e966f842c..3520747314 100644 --- a/src/shared/constants/modelSpecs.ts +++ b/src/shared/constants/modelSpecs.ts @@ -15,6 +15,10 @@ export interface ModelSpec { supportsThinking?: boolean; supportsTools?: boolean; supportsVision?: boolean; + // Model defaults to adaptive thinking and REJECTS an explicit `thinking.type:"disabled"` + // (upstream returns 400). Used to normalize the request when a combo/route substitutes + // this model after the client already chose `disabled`. See issue #3554. + rejectsThinkingDisabled?: boolean; } const BEDROCK_CLAUDE_ALIASES = (...modelIds: string[]) => [ @@ -221,6 +225,8 @@ export const MODEL_SPECS: Record = { supportsThinking: true, supportsTools: true, supportsVision: true, + // Fable 5 defaults to adaptive thinking and rejects `thinking.type:"disabled"` (#3554). + rejectsThinkingDisabled: true, aliases: BEDROCK_CLAUDE_ALIASES("claude-fable-5"), }, @@ -427,6 +433,34 @@ export function getModelSpec(modelId: string): ModelSpec | undefined { return undefined; } +/** + * Normalize a request's `thinking` field against the (possibly combo-substituted) target model. + * + * A combo/route can swap the upstream model AFTER the client already chose its `thinking` + * value. Claude Code sends `thinking:{type:"disabled"}` for internal title/name-generation + * calls — valid for opus/sonnet, but claude-fable-5 defaults to adaptive thinking and rejects + * `type:"disabled"` with an upstream 400. When the resolved target model is flagged + * `rejectsThinkingDisabled`, drop the now-invalid `thinking` so the model uses its adaptive + * default instead of hard-failing. Models that accept `disabled` are left untouched, and any + * non-`disabled` thinking (enabled/adaptive) is always preserved. See issue #3554. + */ +export function normalizeThinkingForModel>( + body: T, + modelId: string +): T { + const thinking = body?.thinking as Record | undefined; + if ( + thinking && + typeof thinking === "object" && + thinking.type === "disabled" && + getModelSpec(modelId)?.rejectsThinkingDisabled + ) { + const { thinking: _omitted, ...rest } = body as Record; + return rest as T; + } + return body; +} + export function capMaxOutputTokens(modelId: string, requested?: number): number { const spec = getModelSpec(modelId); const cap = spec?.maxOutputTokens ?? MODEL_SPECS.__default__.maxOutputTokens; diff --git a/tests/unit/combo-thinking-disabled-fable5-3554.test.ts b/tests/unit/combo-thinking-disabled-fable5-3554.test.ts new file mode 100644 index 0000000000..3db0f142fe --- /dev/null +++ b/tests/unit/combo-thinking-disabled-fable5-3554.test.ts @@ -0,0 +1,61 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + normalizeThinkingForModel, + getModelSpec, +} from "../../src/shared/constants/modelSpecs.ts"; + +// Regression for #3554: a combo can substitute the upstream model AFTER the client +// already chose its `thinking` value. Claude Code sends `thinking:{type:"disabled"}` for +// internal title/name-generation calls. That value is valid for claude-opus-4-8 and +// claude-sonnet-4-6, but claude-fable-5 defaults to adaptive thinking and REJECTS +// `thinking.type:"disabled"` with an upstream 400. When the substituted target rejects +// `disabled`, OmniRoute must strip the now-invalid value instead of forwarding it. + +test("#3554 claude-fable-5 is flagged as rejecting thinking.type:disabled", () => { + assert.equal(getModelSpec("claude-fable-5")?.rejectsThinkingDisabled, true); +}); + +test("#3554 models that accept disabled are NOT flagged (opus-4-8, sonnet-4-6)", () => { + assert.notEqual(getModelSpec("claude-opus-4-8")?.rejectsThinkingDisabled, true); + assert.notEqual(getModelSpec("claude-sonnet-4-6")?.rejectsThinkingDisabled, true); +}); + +test("#3554 normalizeThinkingForModel strips thinking.type:disabled for fable-5", () => { + const body = { model: "claude-opus-4-8", thinking: { type: "disabled" }, max_tokens: 64000 }; + const out = normalizeThinkingForModel(body, "claude-fable-5"); + assert.equal("thinking" in out, false, "thinking must be stripped for fable-5"); + assert.equal(out.max_tokens, 64000, "other fields untouched"); + assert.equal(out.model, "claude-opus-4-8", "model field untouched by this helper"); +}); + +test("#3554 normalizeThinkingForModel preserves disabled for opus-4-8 and sonnet-4-6", () => { + for (const m of ["claude-opus-4-8", "claude-sonnet-4-6"]) { + const out = normalizeThinkingForModel({ model: m, thinking: { type: "disabled" } }, m); + assert.deepEqual(out.thinking, { type: "disabled" }, `disabled preserved for ${m}`); + } +}); + +test("#3554 normalizeThinkingForModel preserves enabled/adaptive thinking for fable-5", () => { + const enabled = normalizeThinkingForModel( + { thinking: { type: "enabled", budget_tokens: 4000 } }, + "claude-fable-5" + ); + assert.deepEqual(enabled.thinking, { type: "enabled", budget_tokens: 4000 }); + const adaptive = normalizeThinkingForModel({ thinking: { type: "adaptive" } }, "claude-fable-5"); + assert.deepEqual(adaptive.thinking, { type: "adaptive" }); +}); + +test("#3554 normalizeThinkingForModel is a no-op when there is no thinking field", () => { + const body = { model: "claude-fable-5", messages: [] }; + const out = normalizeThinkingForModel(body, "claude-fable-5"); + assert.deepEqual(out, body); +}); + +test("#3554 normalizeThinkingForModel tolerates unknown models (no spec → preserve)", () => { + const out = normalizeThinkingForModel( + { thinking: { type: "disabled" } }, + "some-unknown-model-xyz" + ); + assert.deepEqual(out.thinking, { type: "disabled" }); +});