Repository navigation
fix(responses): keep the reserved functions group intact for codex-spark (#3217) #3224
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import type { SsePayloadRewrite } from "./sse-payload-rewrite"; | ||
|
|
||
| function isPlainObject(value: unknown): value is Record<string, unknown> { | ||
| return !!value && typeof value === "object" && !Array.isArray(value); | ||
| } | ||
|
|
||
| /** | ||
| * Drop a tool-call `namespace` that merely repeats the call's own `name` (#3217). | ||
| * | ||
| * codex-rs resolves a client tool call as `ToolName::new(namespace, name)` and treats only | ||
| * `None | "" | "functions"` as the default namespace; anything else is concatenated into a flat | ||
| * name before routing. A backend answer of `{ name: "exec", namespace: "exec" }` therefore | ||
| * becomes `execexec`, which no client tool matches, and Codex re-issues the same call forever. | ||
| * That shape is never a legitimate identity — an MCP namespace is a server name, not the tool — | ||
| * so it is safe to scrub without consulting the declared catalog. The adapter fix that stops | ||
| * provoking the answer lives in `stripSparkCompatibility`; this is the belt to that suspender. | ||
| */ | ||
| export function scrubSelfNamedToolCallNamespace(value: unknown): { value: unknown; changed: boolean } { | ||
| if (Array.isArray(value)) { | ||
| let changed = false; | ||
| const out = value.map(entry => { | ||
| const result = scrubSelfNamedToolCallNamespace(entry); | ||
| changed ||= result.changed; | ||
| return result.value; | ||
| }); | ||
| return changed ? { value: out, changed: true } : { value, changed: false }; | ||
| } | ||
| if (!isPlainObject(value)) return { value, changed: false }; | ||
| let changed = false; | ||
| const out: Record<string, unknown> = {}; | ||
| for (const [key, entry] of Object.entries(value)) { | ||
| const result = scrubSelfNamedToolCallNamespace(entry); | ||
| out[key] = result.value; | ||
| changed ||= result.changed; | ||
| } | ||
| if ( | ||
| (value.type === "custom_tool_call" || value.type === "function_call") | ||
| && typeof value.name === "string" | ||
| && value.name.length > 0 | ||
| && value.namespace === value.name | ||
|
Comment on lines
+36
to
+40
There was a problem hiding this comment.
When a caller legitimately declares a namespace and child with the same name (for example, namespace AGENTS.md reference: src/AGENTS.md:L19-L19 Useful? React with 👍 / 👎. |
||
| ) { | ||
| delete out.namespace; | ||
| changed = true; | ||
| } | ||
| return changed ? { value: out, changed: true } : { value, changed: false }; | ||
| } | ||
|
|
||
| export function scrubSelfNamedToolCallNamespaceInJson(text: string): string { | ||
| if (!text.includes("\"namespace\"")) return text; | ||
| let payload: unknown; | ||
| try { | ||
| payload = JSON.parse(text); | ||
| } catch { | ||
| return text; | ||
| } | ||
| const result = scrubSelfNamedToolCallNamespace(payload); | ||
| return result.changed ? JSON.stringify(result.value) : text; | ||
| } | ||
|
|
||
| export function createSelfNamedToolCallNamespaceScrubRewrite(): SsePayloadRewrite { | ||
| return payload => scrubSelfNamedToolCallNamespaceInJson(payload); | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| /** | ||
| * #3217 — a custom_tool_call whose `namespace` repeats its own `name` must not reach Codex. | ||
| * | ||
| * codex-rs resolves `ToolName::new(namespace, name)` and only treats None/""/"functions" as the | ||
| * default namespace; `{ name: "exec", namespace: "exec" }` becomes the flat name `execexec`, | ||
| * which no client tool matches, and Codex re-issues the call every turn. The adapter fix keeps | ||
| * the reserved `functions` group intact so the backend stops answering that way; this scrub is | ||
| * the belt to that suspender on the client-facing passthrough (SSE and bounded JSON). | ||
| */ | ||
| import { afterEach, expect, test } from "bun:test"; | ||
| import { handleResponses } from "../src/server/responses"; | ||
| import { scrubSelfNamedToolCallNamespace } from "../src/server/responses-self-named-namespace-scrub"; | ||
| import type { OcxConfig } from "../src/types"; | ||
|
|
||
| const originalFetch = globalThis.fetch; | ||
| afterEach(() => { globalThis.fetch = originalFetch; }); | ||
|
|
||
| function forwardConfig(): OcxConfig { | ||
| return { | ||
| port: 0, | ||
| defaultProvider: "openai", | ||
| providers: { | ||
| openai: { | ||
| adapter: "openai-responses", | ||
| baseUrl: "https://chatgpt.com/backend-api/codex", | ||
| authMode: "forward", | ||
| codexAccountMode: "direct", | ||
| }, | ||
| }, | ||
| } as unknown as OcxConfig; | ||
| } | ||
|
|
||
| const requestBody = { | ||
| model: "gpt-5.3-codex-spark", | ||
| stream: true, | ||
| store: false, | ||
| instructions: "x", | ||
| input: [ | ||
| { | ||
| type: "additional_tools", | ||
| role: "developer", | ||
| tools: [{ | ||
| type: "namespace", | ||
| name: "functions", | ||
| tools: [ | ||
| { type: "custom", name: "exec", description: "shell" }, | ||
| { type: "function", name: "wait", parameters: { type: "object", properties: {} } }, | ||
| ], | ||
| }], | ||
| }, | ||
| { type: "message", role: "user", content: [{ type: "input_text", text: "run pwd" }] }, | ||
| ], | ||
| }; | ||
|
|
||
| function sseFrom(items: Array<Record<string, unknown>>): string { | ||
| const events = [ | ||
| { type: "response.output_item.added", output_index: 0, item: { ...items[0], input: "", status: "in_progress" } }, | ||
| { type: "response.output_item.done", output_index: 0, item: items[0] }, | ||
| { type: "response.completed", response: { id: "r1", status: "completed", output: items } }, | ||
| ]; | ||
| return events.map(e => `event: ${e.type}\ndata: ${JSON.stringify(e)}\n\n`).join(""); | ||
| } | ||
|
|
||
| function request(): Request { | ||
| return new Request("http://localhost/v1/responses", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json", authorization: "Bearer test", "chatgpt-account-id": "acct" }, | ||
| body: JSON.stringify(requestBody), | ||
| }); | ||
| } | ||
|
|
||
| test("a self-named namespace on a passthrough custom_tool_call is scrubbed before the client (#3217)", async () => { | ||
| const call = { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "exec", namespace: "exec", input: "pwd", status: "completed" }; | ||
| globalThis.fetch = (async () => new Response(sseFrom([call]), { | ||
| status: 200, headers: { "content-type": "text/event-stream" }, | ||
| })) as typeof fetch; | ||
|
|
||
| const res = await handleResponses(request(), forwardConfig(), { model: "", provider: "" }); | ||
| expect(res.status).toBe(200); | ||
| const text = await res.text(); | ||
| const payloads = text.split("\n").filter(l => l.startsWith("data: ") && l !== "data: [DONE]").map(l => JSON.parse(l.slice(6)) as Record<string, unknown>); | ||
| const callItems = payloads.flatMap(p => { | ||
| const item = p.item as Record<string, unknown> | undefined; | ||
| const output = (p.response as { output?: Array<Record<string, unknown>> } | undefined)?.output ?? []; | ||
| return [...(item ? [item] : []), ...output]; | ||
| }).filter(i => i.type === "custom_tool_call"); | ||
| expect(callItems.length).toBeGreaterThanOrEqual(3); | ||
| for (const item of callItems) { | ||
| expect(item.name).toBe("exec"); | ||
| expect("namespace" in item).toBe(false); | ||
| } | ||
| }); | ||
|
|
||
| test("a genuine MCP namespace on a passthrough call is left alone", async () => { | ||
| const call = { type: "function_call", id: "fc_1", call_id: "call_1", name: "search", namespace: "mcp__docs", arguments: "{}", status: "completed" }; | ||
| globalThis.fetch = (async () => new Response(sseFrom([call]), { | ||
| status: 200, headers: { "content-type": "text/event-stream" }, | ||
| })) as typeof fetch; | ||
|
|
||
| const res = await handleResponses(request(), forwardConfig(), { model: "", provider: "" }); | ||
| const text = await res.text(); | ||
| expect(text).toContain('"namespace":"mcp__docs"'); | ||
| }); | ||
|
|
||
| test("the bounded JSON (stream:false) passthrough path scrubs the same shape (#3217)", async () => { | ||
| const call = { type: "custom_tool_call", id: "ctc_1", call_id: "call_1", name: "exec", namespace: "exec", input: "pwd", status: "completed" }; | ||
| globalThis.fetch = (async () => new Response(JSON.stringify({ id: "r1", object: "response", status: "completed", output: [call] }), { | ||
| status: 200, headers: { "content-type": "application/json" }, | ||
| })) as typeof fetch; | ||
|
|
||
| const req = new Request("http://localhost/v1/responses", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json", authorization: "Bearer test", "chatgpt-account-id": "acct" }, | ||
| body: JSON.stringify({ ...requestBody, stream: false }), | ||
| }); | ||
| const res = await handleResponses(req, forwardConfig(), { model: "", provider: "" }); | ||
| expect(res.status).toBe(200); | ||
| const body = await res.json() as { output: Array<Record<string, unknown>> }; | ||
| expect(body.output[0]).toMatchObject({ type: "custom_tool_call", name: "exec" }); | ||
| expect("namespace" in body.output[0]).toBe(false); | ||
| }); | ||
|
|
||
| test("scrub is recursive, shape-preserving, and a no-op on clean payloads", () => { | ||
| const clean = { type: "response.completed", response: { output: [{ type: "custom_tool_call", name: "exec", input: "" }] } }; | ||
| expect(scrubSelfNamedToolCallNamespace(clean)).toEqual({ value: clean, changed: false }); | ||
| const dirty = { response: { output: [{ type: "function_call", name: "wait", namespace: "wait", arguments: "{}" }, { type: "message" }] } }; | ||
| const result = scrubSelfNamedToolCallNamespace(dirty); | ||
| expect(result.changed).toBe(true); | ||
| expect(result.value).toEqual({ response: { output: [{ type: "function_call", name: "wait", arguments: "{}" }, { type: "message" }] } }); | ||
| // An empty name never matches: a namespace equal to "" is not the self-named shape. | ||
| expect(scrubSelfNamedToolCallNamespace({ type: "custom_tool_call", name: "", namespace: "" }).changed).toBe(false); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
When a
functionsgroup contains a function whoseparametersis absent or not an object schema, preserving the group here prevents the laternormalizeToolSchemaspass from repairing it because that pass only examines direct entries inbody.toolsandadditional_tools.tools. Before this change the group was flattened, so the child received the required{ type: "object" }normalization; now the malformed nested declaration reaches Spark and can make the upstream reject the entire request. Apply the same schema normalization recursively to retained group children.AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.