From 7a30a5e3dac9670e1a088b33e1df95302bba0d0c Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 15 Jul 2026 13:59:29 -0300 Subject: [PATCH] fix(openai): scope reasoning-strip guard to /chat/completions only stripGpt5ReasoningWhenTools gated on provider+model-name alone, so once #7242 routes the public GPT-5.6 family to /v1/responses (targetFormat "openai-responses", which natively supports tools + reasoning), the two PRs would compose into the worst of both worlds: routed to the endpoint that supports reasoning, but reasoning stripped anyway. Pass the request's already-resolved targetFormat into the guard and skip the strip whenever it is not going out over /chat/completions, so the guard tracks the actual upstream surface instead of a model-name list that would need updating for every future GPT-5.x family. Reported-by: Tech Solution (@techsolutionmta) (https://github.com/decolua/9router/issues/2540) --- open-sse/handlers/chatCore.ts | 15 ++- open-sse/services/gpt5SamplingGuard.ts | 30 +++-- tests/unit/gpt5-tools-reasoning-guard.test.ts | 107 ++++++++++++++---- 3 files changed, 119 insertions(+), 33 deletions(-) diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index ec016439c7..87fa76fb38 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -2093,16 +2093,19 @@ export async function handleChatCore({ log ); - // GPT-5.x reasoning models (raw openai Chat Completions) also reject function `tools` - // combined with an active `reasoning_effort`: HTTP 400 "Function tools with - // reasoning_effort are not supported ... Please use /v1/responses instead." Unlike the - // openai-compatible-* MCP/tool_search shape (forceResponsesUpstream.ts), the plain - // `openai` provider always stays on /chat/completions, so strip the reasoning fields - // here instead of rerouting. Port of 9router#2540. + // GPT-5.x reasoning models on the raw openai Chat Completions surface reject function + // `tools` combined with an active `reasoning_effort`: HTTP 400 "Function tools with + // reasoning_effort are not supported ... Please use /v1/responses instead." This used to + // be true for every GPT-5.x model on the plain `openai` provider, but #7242 (targetFormat + // "openai-responses" on GPT_5_6_API_CAPABILITIES) now routes the GPT-5.6 family to + // /v1/responses instead, which accepts tools + reasoning natively — so the strip must not + // fire there. Pass the already-resolved `targetFormat` so the guard gates on the actual + // upstream surface for this request instead of a model-name list. Port of 9router#2540. translatedBody = stripGpt5ReasoningWhenTools( translatedBody, provider, finalModelToUpstream, + targetFormat, log ); diff --git a/open-sse/services/gpt5SamplingGuard.ts b/open-sse/services/gpt5SamplingGuard.ts index e96703ae2d..729c1ce450 100644 --- a/open-sse/services/gpt5SamplingGuard.ts +++ b/open-sse/services/gpt5SamplingGuard.ts @@ -19,6 +19,8 @@ * Azure Foundry reasoning matrix, openai-python#2072. */ +import { FORMATS } from "../translator/formats.ts"; + type JsonRecord = Record; const SAMPLING_PARAMS = ["temperature", "top_p"] as const; @@ -101,23 +103,35 @@ function hasFunctionTools(record: JsonRecord): boolean { * Raw api.openai.com Chat Completions rejects GPT-5.x reasoning models that * carry BOTH function `tools` and an active `reasoning_effort` with HTTP 400: * "Function tools with reasoning_effort are not supported for in - * /v1/chat/completions. Please use /v1/responses instead." OmniRoute's - * `shouldForceResponsesUpstream` guard only re-routes `openai-compatible-*` - * connections carrying MCP/tool_search tool shapes to `/responses` — the - * plain `openai` provider always stays on `/chat/completions`, so this - * combination still reaches the upstream 400 today. Strip the reasoning - * fields instead so the request succeeds on `/chat/completions` (the - * dashboard offers no `reasoning_effort:"none"` override, so this cannot be - * worked around client-side). Port of 9router#2540. + * /v1/chat/completions. Please use /v1/responses instead." Historically the + * plain `openai` provider always stayed on `/chat/completions` for every + * GPT-5.x model, so this combination reached the upstream 400 with no way to + * recover other than dropping the reasoning fields. + * + * That is no longer true for every GPT-5.x model: the public GPT-5.6 family + * is tagged with `targetFormat: "openai-responses"` (see + * `GPT_5_6_API_CAPABILITIES` in `config/providers/shared.ts`, closes #2540 / + * 9router#2547) and is routed to `/v1/responses` instead, which natively + * accepts tools + reasoning together — /v1/responses is literally the + * endpoint the 400 message tells callers to use. Gate on the resolved + * `targetFormat` (the fact chatCore already computed for this request) + * rather than a model-name list: only strip when the request is actually + * going out over `/chat/completions`. If a future GPT-5.x family also moves + * to `/responses`, this guard keeps working with no change needed here. + * Port of 9router#2540. */ export function stripGpt5ReasoningWhenTools>( body: T, provider: string | null | undefined, model: string | null | undefined, + targetFormat: string | null | undefined, log?: { warn?: (tag: string, message: string) => void } | null ): T { if (provider !== "openai") return body; if (typeof model !== "string" || !/^gpt-5/i.test(model)) return body; + // Already routed to /v1/responses (e.g. GPT-5.6, #7242) — that endpoint + // supports tools + reasoning natively, nothing to strip. + if (targetFormat === FORMATS.OPENAI_RESPONSES) return body; const record = asRecord(body); if (!record) return body; diff --git a/tests/unit/gpt5-tools-reasoning-guard.test.ts b/tests/unit/gpt5-tools-reasoning-guard.test.ts index 410b2d734c..137194daf1 100644 --- a/tests/unit/gpt5-tools-reasoning-guard.test.ts +++ b/tests/unit/gpt5-tools-reasoning-guard.test.ts @@ -10,61 +10,76 @@ * the plain `openai` provider has no equivalent guard, so this scenario still * reaches the upstream 400 today. Strip `reasoning_effort`/`reasoning` when * function tools are present so the request succeeds on /v1/chat/completions. + * + * The guard is passed the request's already-resolved `targetFormat` (chatCore + * resolves it once via `resolveChatCoreTargetFormat` before this guard runs) so it + * gates on the actual upstream surface for THIS request rather than a model-name + * list. This matters because #7242 (closes #2540 upstream / 9router#2547) tags the + * public GPT-5.6 family with `targetFormat: "openai-responses"` and routes it to + * `/v1/responses` instead — an endpoint that accepts tools + reasoning natively — + * so stripping must NOT fire for GPT-5.6 requests once that routing is in effect. + * Without this composition, #7101's strip and #7242's reroute would combine into + * the worst of both worlds: routed to the endpoint that supports reasoning, but + * with reasoning silently dropped anyway. */ import test from "node:test"; import assert from "node:assert/strict"; import { stripGpt5ReasoningWhenTools } from "../../open-sse/services/gpt5SamplingGuard.ts"; -test("strips reasoning_effort for openai gpt-5.x when function tools are present", () => { +// Chat Completions models in this suite use gpt-5.4/gpt-5.5 (targetFormat "openai"), +// which stay on /chat/completions and must keep being stripped. gpt-5.6-sol is reserved +// for the /v1/responses composition cases below, where stripping must NOT happen. + +test("strips reasoning_effort for openai gpt-5.x on /chat/completions when function tools are present", () => { const body = { - model: "gpt-5.6-sol", + model: "gpt-5.4-sol", reasoning_effort: "high", tools: [{ type: "function", function: { name: "read_file" } }], messages: [], }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning_effort, undefined); }); -test("strips nested reasoning.effort for openai gpt-5.x when function tools are present", () => { +test("strips nested reasoning.effort for openai gpt-5.x on /chat/completions when function tools are present", () => { const body = { - model: "gpt-5.6-sol", + model: "gpt-5.4-sol", reasoning: { effort: "medium" }, tools: [{ type: "function", function: { name: "read_file" } }], }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning, undefined); }); test("keeps reasoning_effort=none untouched (already non-reasoning mode)", () => { const body = { - model: "gpt-5.6-sol", + model: "gpt-5.4-sol", reasoning_effort: "none", tools: [{ type: "function", function: { name: "read_file" } }], }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning_effort, "none"); }); test("keeps reasoning_effort when there are no tools", () => { - const body = { model: "gpt-5.6-sol", reasoning_effort: "high", messages: [] }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const body = { model: "gpt-5.4-sol", reasoning_effort: "high", messages: [] }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning_effort, "high"); }); test("keeps reasoning_effort when tools array is empty", () => { - const body = { model: "gpt-5.6-sol", reasoning_effort: "high", tools: [] }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const body = { model: "gpt-5.4-sol", reasoning_effort: "high", tools: [] }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning_effort, "high"); }); test("non-openai provider is untouched", () => { const body = { - model: "gpt-5.6-sol", + model: "gpt-5.4-sol", reasoning_effort: "high", tools: [{ type: "function", function: { name: "x" } }], }; - const result = stripGpt5ReasoningWhenTools(body, "codex", "gpt-5.6-sol"); + const result = stripGpt5ReasoningWhenTools(body, "codex", "gpt-5.4-sol", "openai"); assert.equal(result.reasoning_effort, "high"); }); @@ -74,13 +89,13 @@ test("non-gpt-5 openai model is untouched", () => { reasoning_effort: "high", tools: [{ type: "function", function: { name: "x" } }], }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-4o"); + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-4o", "openai"); assert.equal(result.reasoning_effort, "high"); }); test("returns the same reference when nothing to strip", () => { - const body = { model: "gpt-5.6-sol", tools: [{ type: "function" }], messages: [] }; - const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol"); + const body = { model: "gpt-5.4-sol", tools: [{ type: "function" }], messages: [] }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.4-sol", "openai"); assert.equal(result, body); }); @@ -89,15 +104,69 @@ test("logs the stripped fields when a logger is provided", () => { const log = { warn: (tag: string, message: string) => calls.push([tag, message]) }; stripGpt5ReasoningWhenTools( { - model: "gpt-5.6-sol", + model: "gpt-5.4-sol", reasoning_effort: "high", tools: [{ type: "function", function: { name: "x" } }], }, "openai", - "gpt-5.6-sol", + "gpt-5.4-sol", + "openai", log ); assert.equal(calls.length, 1); assert.equal(calls[0][0], "PARAMS"); assert.match(calls[0][1], /reasoning_effort/); }); + +// --- Composition with #7242 (GPT-5.6 → /v1/responses) --- +// +// #7242 tags the public GPT-5.6 family with targetFormat "openai-responses" so it is +// routed to /v1/responses, which natively supports tools + reasoning together. If this +// guard ignored targetFormat and only looked at provider+model-name (the pre-#7242 +// shape), a GPT-5.6 request with tools + reasoning_effort would still get its reasoning +// silently stripped even though it is no longer going to /chat/completions — the worst +// of both worlds. These cases prove the composition holds. + +test("gpt-5.6 routed to /v1/responses (targetFormat openai-responses) keeps reasoning_effort", () => { + const body = { + model: "gpt-5.6-sol", + reasoning_effort: "high", + tools: [{ type: "function", function: { name: "read_file" } }], + messages: [], + }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol", "openai-responses"); + assert.equal(result.reasoning_effort, "high"); + assert.equal(result, body, "no-op path should return the same reference"); +}); + +test("gpt-5.6 routed to /v1/responses keeps nested reasoning.effort too", () => { + const body = { + model: "gpt-5.6-sol", + reasoning: { effort: "medium" }, + tools: [{ type: "function", function: { name: "read_file" } }], + }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol", "openai-responses"); + assert.deepEqual(result.reasoning, { effort: "medium" }); +}); + +test("gpt-5.4/gpt-5.5 stay on /chat/completions (targetFormat openai) and keep stripping", () => { + for (const model of ["gpt-5.4-sol", "gpt-5.5-pro"]) { + const body = { + model, + reasoning_effort: "high", + tools: [{ type: "function", function: { name: "read_file" } }], + }; + const result = stripGpt5ReasoningWhenTools(body, "openai", model, "openai"); + assert.equal(result.reasoning_effort, undefined, `${model} should still be stripped`); + } +}); + +test("if gpt-5.6 were ever NOT routed to /v1/responses, the strip would still apply (defense in depth)", () => { + const body = { + model: "gpt-5.6-sol", + reasoning_effort: "high", + tools: [{ type: "function", function: { name: "read_file" } }], + }; + const result = stripGpt5ReasoningWhenTools(body, "openai", "gpt-5.6-sol", "openai"); + assert.equal(result.reasoning_effort, undefined); +});