-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[agent] fix: scope the Responses control strip to the ChatGPT backend and carry Chat reasoning and penalties #4535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Comment on lines
310
to
312
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a replayed assistant message has Useful? React with 👍 / 👎. |
||
|
|
@@ -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; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This commit changes the mapped AGENTS.md reference: structure/AGENTS.md:L49-L50 Useful? React with 👍 / 👎. |
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
Comment on lines
+276
to
+279
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Correct the These lines state that output-cap restrictions are gated by Proposed fix- 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
+ sampling 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.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| 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. | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>; | ||
|
|
||
| function body(messages: unknown[], extra: Record<string, unknown> = {}): Record<string, unknown> { | ||
| return chatCompletionsToResponsesBody({ model: "m", messages, ...extra }); | ||
| } | ||
|
|
||
| function items(out: Record<string, unknown>): 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); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>): Record<string, unknown> { | ||
| 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<string, unknown>; | ||
|
|
||
| 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"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The new control-scope tests call the sanitizer directly but do not exercise
buildRequest's canonical-provider branch. Add canonical and non-canonical forward request cases so the suite detects a missing or incorrectly scoped wiring of this helper.🤖 Prompt for AI Agents