From 244141ce0cc3806d1375c4182f6345837ccc746c Mon Sep 17 00:00:00 2001 From: JUN Date: Mon, 14 Sep 2026 09:14:22 +0900 Subject: [PATCH] [agent] fix: scope the Responses control strip to the ChatGPT backend and carry Chat reasoning and penalties Restacked onto the squashed #4534 landing; tree identical to pre-restack head e35995ce0b. --- scripts/test-layout/layout.json | 2 + src/adapters/openai-responses.ts | 26 ++++ src/chat/inbound.ts | 41 +++++++ src/server/chat-completions.ts | 22 +++- structure/data-planes/inbound-compat.md | 26 ++++ tests/fixtures/test-layout-expected.json | 2 + .../chat-inbound-reasoning-replay.test.ts | 115 ++++++++++++++++++ .../chat-responses-control-scope.test.ts | 83 +++++++++++++ 8 files changed, 311 insertions(+), 6 deletions(-) create mode 100644 tests/responses/chat-inbound-reasoning-replay.test.ts create mode 100644 tests/responses/chat-responses-control-scope.test.ts diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 8e8be1563a..04641cae4a 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -290,10 +290,12 @@ "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", "chat-inbound-reasoning-none.test.ts": "responses", + "chat-inbound-reasoning-replay.test.ts": "responses", "chat-json-sse-fallback.test.ts": "responses", "chat-native-image-normalization.test.ts": "responses", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", + "chat-responses-control-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", "chatgpt-oauth.test.ts": "oauth", "chatgpt-token-expiry.test.ts": "oauth", diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index e2c7cf2a14..6a07b84851 100644 --- a/src/adapters/openai-responses.ts +++ b/src/adapters/openai-responses.ts @@ -1285,6 +1285,31 @@ function stripUnsupportedForwardParams(body: unknown): unknown { return rest; } +/** Sampling controls the canonical ChatGPT backend rejects; other forward gateways accept them. */ +const CANONICAL_FORWARD_UNSUPPORTED_SAMPLING = ["temperature", "top_p", "stop", "user"] as const; + +/** + * Remove sampling controls only the canonical ChatGPT backend rejects. + * + * A translated Chat turn used to lose these at the Chat ingress for every provider on + * the `openai-responses` adapter, which silently discarded caller intent on generic + * key gateways that accept them. Deciding at the ingress was also unsound for combo + * and policy routes, whose concrete child is chosen later — so the decision belongs + * here, on the provider that actually receives the body. + * + * Returns a copy and never mutates, so `parsed._rawBody` stays caller-owned, and + * no-ops when the body carries none of these keys. + */ +export function stripCanonicalForwardSamplingParams(body: unknown): unknown { + if (!isPlainObject(body)) return body; + if (!CANONICAL_FORWARD_UNSUPPORTED_SAMPLING.some(key => Object.prototype.hasOwnProperty.call(body, key))) { + return body; + } + const next: Record = { ...body }; + for (const key of CANONICAL_FORWARD_UNSUPPORTED_SAMPLING) delete next[key]; + return next; +} + /** Return the lossless text represented by one system message, or null when it is multimodal. */ function canonicalForwardSystemText(item: Record): string | null { const content = item.content; @@ -2254,6 +2279,7 @@ export function createResponsesPassthroughAdapter(provider: OcxProviderConfig): // Only the canonical ChatGPT backend rejects the retired field; a self-hosted or // third-party forward gateway may still accept it, so this must not be widened. if (isCanonicalOpenAiForwardProvider(provider)) { + outBody = stripCanonicalForwardSamplingParams(outBody); outBody = stripDeprecatedPromptCacheRetention(outBody, parsed.modelId); outBody = stripCanonicalForwardPromptCacheOptions(outBody); outBody = normalizeCanonicalForwardPromptEnvelope(outBody); diff --git a/src/chat/inbound.ts b/src/chat/inbound.ts index 597724fb7a..6e5538fbe3 100644 --- a/src/chat/inbound.ts +++ b/src/chat/inbound.ts @@ -96,6 +96,31 @@ function userContentToBlocks(content: unknown): Rec[] { return blocks; } +/** + * The assistant's prior thinking, as plaintext, from either Chat spelling. + * + * The outbound direction already reconstructs these for providers listed in + * `preserveReasoningContentModels` (src/adapters/openai-chat.ts), so a client + * replaying a turn sends them back. Dropping them here made the round trip lossy and + * left interleaved-thinking providers seeing a bare continuation. + * + * Only representable plaintext is read. No signature, encrypted payload or + * provider-issued item id is reconstructed — see the reasoning item built below. + */ +function assistantReasoningText(msg: Rec): string | undefined { + if (typeof msg.reasoning_content === "string" && msg.reasoning_content.length > 0) { + return msg.reasoning_content; + } + if (Array.isArray(msg.reasoning_details)) { + const segments: string[] = []; + for (const raw of msg.reasoning_details) { + if (isRec(raw) && typeof raw.text === "string" && raw.text.length > 0) segments.push(raw.text); + } + if (segments.length > 0) return segments.join(""); + } + return undefined; +} + function assistantContentToBlocks(content: unknown): Rec[] { if (typeof content === "string") { return content.length > 0 ? [{ type: "output_text", text: content }] : []; @@ -273,6 +298,15 @@ export function chatCompletionsToResponsesBody(raw: unknown): Rec { break; } case "assistant": { + // A reasoning item precedes the assistant message it belongs to: the + // Responses assistant item schema admits only output content blocks, so there + // is no attachment point on the message itself, and the parser buffers a + // reasoning item and prepends it to the NEXT assistant message. Emitting it + // here keeps that adjacency intact. + const reasoningText = assistantReasoningText(msg); + if (reasoningText !== undefined) { + input.push({ type: "reasoning", content: [{ type: "reasoning_text", text: reasoningText }] }); + } const blocks = assistantContentToBlocks(msg.content); if (blocks.length > 0) input.push({ type: "message", role: "assistant", content: blocks }); if (msg.tool_calls !== undefined) toolCallsToItems(msg.tool_calls, input, knownNameByCallId); @@ -320,6 +354,13 @@ export function chatCompletionsToResponsesBody(raw: unknown): Rec { if (typeof maxTokens === "number") body.max_output_tokens = maxTokens; if (typeof raw.temperature === "number") body.temperature = raw.temperature; if (typeof raw.top_p === "number") body.top_p = raw.top_p; + // responsesRequestSchema accepts both, parser.ts reads them into + // options.presencePenalty/frequencyPenalty, and the openai-chat adapter writes them + // back to the wire. Only this first link was missing, so a Chat caller's penalties + // never reached a provider that supports them. Per-model noPenaltyModels opt-outs + // still apply at the adapter. + if (typeof raw.presence_penalty === "number") body.presence_penalty = raw.presence_penalty; + if (typeof raw.frequency_penalty === "number") body.frequency_penalty = raw.frequency_penalty; if (raw.stop !== undefined) body.stop = raw.stop; if (typeof raw.user === "string") body.user = raw.user; if (typeof raw.parallel_tool_calls === "boolean") body.parallel_tool_calls = raw.parallel_tool_calls; diff --git a/src/server/chat-completions.ts b/src/server/chat-completions.ts index 947f71f240..ee190e4620 100644 --- a/src/server/chat-completions.ts +++ b/src/server/chat-completions.ts @@ -226,13 +226,23 @@ async function handleChatCompletionsWithBudget( // for non-streaming clients. Native Chat uses the caller's original stream bit. internalBody.stream = true; if (settledRoute?.provider.adapter === "openai-responses") { - // ChatGPT backend rejects store:true and unsupported sampling knobs. + // The proxy never wants upstream-side retention for a translated Chat turn, so + // store stays pinned for every Responses route. + // + // The sampling and output-cap restrictions used to be applied here too, keyed on + // the adapter string. That was wrong twice over. Seven providers share this + // adapter (openai, openai-apikey, meta-model, meta-muse, zai, + // zhipu-bigmodel-responses, volcengine-agent-plan), so a generic key gateway lost + // controls it accepts. And settledRoute is the route settled at INGRESS: a combo + // or policy route resolves its concrete child later in the Responses pipeline, so + // deciding here mutates shared intent before the real target is known — a + // canonical-first combo that falls back to a key gateway had already lost the + // caller's controls, while a non-canonical-first combo that falls back to + // canonical still shipped them. + // + // Canonical-backend sanitization now happens at the final outgoing body in + // src/adapters/openai-responses.ts, where the concrete provider is known. internalBody.store = false; - delete internalBody.max_output_tokens; - delete internalBody.temperature; - delete internalBody.top_p; - delete internalBody.stop; - delete internalBody.user; } else if (internalBody.store === undefined) { internalBody.store = false; } diff --git a/structure/data-planes/inbound-compat.md b/structure/data-planes/inbound-compat.md index 49a24b39aa..8344033723 100644 --- a/structure/data-planes/inbound-compat.md +++ b/structure/data-planes/inbound-compat.md @@ -269,6 +269,32 @@ are preserved; the native path is a whitelist passthrough, so an incidental deep would itself be a behavior change. A remote reference is recognized and rewritten, never fetched. +## Translated Chat control fidelity + +A translated Chat turn keeps the controls the caller sent. The Chat ingress pins +`store:false` for every `openai-responses` route and strips nothing else: the +sampling and output-cap restrictions that the canonical ChatGPT backend requires are +applied at the final outgoing body in `src/adapters/openai-responses.ts`, gated on +`isCanonicalOpenAiForwardProvider`, which additionally requires `authMode: "forward"` +and the canonical base URL. + +Deciding at the ingress was wrong on two axes. Seven providers share the +`openai-responses` adapter string, so a generic key gateway lost controls it +accepts; and `settledRoute` is the ingress-time route, while a combo or policy route +resolves its concrete child later, so the decision preceded knowledge of the real +target in both directions. `stripCanonicalForwardSamplingParams` returns a copy and +no-ops when none of its keys are present, so `_rawBody` stays caller-owned. The +separate forward-wide `max_output_tokens`/`metadata` sanitizer is unchanged. + +An assistant turn's `reasoning_content` or `reasoning_details` is carried into the +projection as a `reasoning` input item emitted immediately before its assistant +message, matching the parser's buffer-and-prepend adjacency. Only representable +plaintext crosses: no signature, encrypted payload or provider item id is +reconstructed, because those attest to content this proxy never received. Opaque +reasoning replay across a Chat boundary remains unimplemented by design. +`presence_penalty` and `frequency_penalty` are carried too; per-model +`noPenaltyModels` opt-outs still apply at the adapter. + ## Explicit reasoning disable on the Chat ingress The Chat inbound effort allowlist accepts `none` alongside the ladder values. diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 3ef2cc0cf8..34baef2a15 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -123,10 +123,12 @@ "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", "chat-inbound-reasoning-none.test.ts": "responses", + "chat-inbound-reasoning-replay.test.ts": "responses", "chat-json-sse-fallback.test.ts": "responses", "chat-native-image-normalization.test.ts": "responses", "chat-refusal.test.ts": "responses", "chat-refusal-scope.test.ts": "responses", + "chat-responses-control-scope.test.ts": "responses", "chatgpt-device-auth.test.ts": "oauth", "chatgpt-oauth.test.ts": "oauth", "chatgpt-token-expiry.test.ts": "oauth", diff --git a/tests/responses/chat-inbound-reasoning-replay.test.ts b/tests/responses/chat-inbound-reasoning-replay.test.ts new file mode 100644 index 0000000000..de78b3eaaa --- /dev/null +++ b/tests/responses/chat-inbound-reasoning-replay.test.ts @@ -0,0 +1,115 @@ +/** + * Audit F6 (2026-09-14): the translated Chat path dropped an assistant turn's + * `reasoning_content`/`reasoning_details` and never carried the sampling penalties. + * + * Both are asymmetries rather than missing features. The outbound direction already + * reconstructs reasoning for `preserveReasoningContentModels` + * (src/adapters/openai-chat.ts), so a client replaying a turn sends it back and the + * proxy threw it away. And `presence_penalty`/`frequency_penalty` are accepted by + * responsesRequestSchema, parsed into options, and written back to the wire by the + * openai-chat adapter — only this first link was missing. + * + * Safety boundary asserted here: a synthesized reasoning item carries representable + * plaintext only. No signature, encrypted payload or provider item id is forged, and + * the Anthropic adapter's signature filter rejects anything this path could produce. + */ +import { describe, expect, test } from "bun:test"; +import { chatCompletionsToResponsesBody } from "../../src/chat/inbound"; +import { responsesRequestSchema } from "../../src/responses/schema"; + +type Item = Record; + +function body(messages: unknown[], extra: Record = {}): Record { + return chatCompletionsToResponsesBody({ model: "m", messages, ...extra }); +} + +function items(out: Record): Item[] { + return out.input as Item[]; +} + +const USER = { role: "user", content: "question" }; + +describe("F6 assistant reasoning survives translation", () => { + test("a reasoning_content string becomes a reasoning item before its assistant message", () => { + const out = items(body([USER, { role: "assistant", content: "answer", reasoning_content: "prior analysis" }])); + const idx = out.findIndex(i => i.type === "reasoning"); + + expect(idx).toBeGreaterThanOrEqual(0); + expect(out[idx]!.content).toEqual([{ type: "reasoning_text", text: "prior analysis" }]); + // Adjacency matters: the parser prepends a buffered reasoning item to the NEXT + // assistant message, so it must sit immediately before it. + expect(out[idx + 1]).toMatchObject({ type: "message", role: "assistant" }); + }); + + test("reasoning_details segments are joined in order", () => { + const out = items(body([USER, { + role: "assistant", + content: "answer", + reasoning_details: [ + { type: "reasoning.text", text: "first " }, + { type: "reasoning.text", text: "second" }, + ], + }])); + + expect(out.find(i => i.type === "reasoning")!.content).toEqual([{ type: "reasoning_text", text: "first second" }]); + }); + + test("no signature, encrypted payload or item id is forged", () => { + const item = items(body([USER, { role: "assistant", content: "a", reasoning_content: "t" }])).find(i => i.type === "reasoning")!; + + expect(item.signature).toBeUndefined(); + expect(item.encrypted_content).toBeUndefined(); + expect(item.id).toBeUndefined(); + }); + + test("reasoning is carried for a tool-calling assistant turn too", () => { + const out = items(body([USER, { + role: "assistant", + reasoning_content: "deciding", + tool_calls: [{ id: "call1", type: "function", function: { name: "lookup", arguments: "{}" } }], + }])); + + expect(out.some(i => i.type === "reasoning")).toBe(true); + expect(out.some(i => i.type === "function_call")).toBe(true); + }); + + test("an assistant turn with no reasoning produces no reasoning item", () => { + expect(items(body([USER, { role: "assistant", content: "answer" }])).some(i => i.type === "reasoning")).toBe(false); + }); + + test("empty reasoning is treated as absent rather than an empty item", () => { + expect(items(body([USER, { role: "assistant", content: "a", reasoning_content: "" }])).some(i => i.type === "reasoning")).toBe(false); + expect(items(body([USER, { role: "assistant", content: "a", reasoning_details: [] }])).some(i => i.type === "reasoning")).toBe(false); + }); + + test("the produced body still validates against responsesRequestSchema", () => { + const out = body([USER, { role: "assistant", content: "a", reasoning_content: "t" }]); + expect(responsesRequestSchema.safeParse(out).success).toBe(true); + }); +}); + +describe("F6 sampling penalties reach the Responses body", () => { + test("both penalties are carried", () => { + const out = body([USER], { presence_penalty: 0.4, frequency_penalty: -0.2 }); + + expect(out.presence_penalty).toBe(0.4); + expect(out.frequency_penalty).toBe(-0.2); + }); + + test("omitted penalties stay absent", () => { + const out = body([USER]); + + expect(out.presence_penalty).toBeUndefined(); + expect(out.frequency_penalty).toBeUndefined(); + }); + + test("a non-numeric penalty is ignored rather than forwarded", () => { + const out = body([USER], { presence_penalty: "high" }); + expect(out.presence_penalty).toBeUndefined(); + }); + + test("a penalty-carrying body still validates", () => { + const out = body([USER], { presence_penalty: 0.4, frequency_penalty: 0.1 }); + expect(responsesRequestSchema.safeParse(out).success).toBe(true); + }); +}); diff --git a/tests/responses/chat-responses-control-scope.test.ts b/tests/responses/chat-responses-control-scope.test.ts new file mode 100644 index 0000000000..46cb7afe1e --- /dev/null +++ b/tests/responses/chat-responses-control-scope.test.ts @@ -0,0 +1,83 @@ +/** + * Audit F2 (2026-09-14): a translated Chat turn lost `max_output_tokens`, + * `temperature`, `top_p`, `stop` and `user` for EVERY provider on the + * `openai-responses` adapter, keyed on the adapter string at the Chat ingress. + * + * The restriction is real for the canonical ChatGPT backend and wrong as a blanket + * rule: seven providers share that adapter, and a generic key gateway accepts these + * controls. Deciding at the ingress was also unsound for combo and policy routes, + * whose concrete child is chosen later in the Responses pipeline — so an + * ingress-time strip mutated shared intent before the real target was known. + * + * Sanitization now happens on the final outgoing body, gated on + * isCanonicalOpenAiForwardProvider, which requires adapter openai-responses AND + * authMode "forward" AND the canonical base URL. + */ +import { describe, expect, test } from "bun:test"; +import { stripCanonicalForwardSamplingParams } from "../../src/adapters/openai-responses"; +import { chatCompletionsToResponsesBody } from "../../src/chat/inbound"; + +function chat(extra: Record): Record { + return { model: "m", messages: [{ role: "user", content: "hi" }], ...extra }; +} + +describe("F2 the ingress no longer strips caller controls", () => { + test("the translated body carries every control the caller sent", () => { + const body = chatCompletionsToResponsesBody(chat({ + max_tokens: 123, + temperature: 0.2, + top_p: 0.8, + stop: ["END"], + user: "u-1", + })); + + expect(body.max_output_tokens).toBe(123); + expect(body.temperature).toBe(0.2); + expect(body.top_p).toBe(0.8); + expect(body.stop).toEqual(["END"]); + expect(body.user).toBe("u-1"); + }); + + test("store stays pinned false for a translated turn", () => { + expect(chatCompletionsToResponsesBody(chat({})).store).toBe(false); + }); +}); + +describe("F2 canonical-backend sanitization at the final target", () => { + test("removes exactly the four controls the canonical backend rejects", () => { + const out = stripCanonicalForwardSamplingParams({ + model: "gpt-5.6", + temperature: 0.2, + top_p: 0.8, + stop: ["END"], + user: "u-1", + max_output_tokens: 123, + }) as Record; + + expect(out.temperature).toBeUndefined(); + expect(out.top_p).toBeUndefined(); + expect(out.stop).toBeUndefined(); + expect(out.user).toBeUndefined(); + // max_output_tokens is owned by the separate forward-wide sanitizer, not this one. + expect(out.max_output_tokens).toBe(123); + expect(out.model).toBe("gpt-5.6"); + }); + + test("never mutates its input, so _rawBody stays caller-owned", () => { + const input = { temperature: 0.2, model: "gpt-5.6" }; + const out = stripCanonicalForwardSamplingParams(input); + + expect(out).not.toBe(input); + expect(input.temperature).toBe(0.2); + }); + + test("returns the identical reference when no such control is present", () => { + const input = { model: "gpt-5.6", input: [] }; + expect(stripCanonicalForwardSamplingParams(input)).toBe(input); + }); + + test("passes a non-object through untouched", () => { + expect(stripCanonicalForwardSamplingParams(undefined)).toBeUndefined(); + expect(stripCanonicalForwardSamplingParams("x")).toBe("x"); + }); +});