From e078664dddbcbe3878e2ac9248c92efaf692fa14 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Sun, 12 Jul 2026 18:08:07 -0300 Subject: [PATCH] fix(sse): schema-aware optional tool-arg normalization for Codex routes (#6951) (#6992) stripEmptyOptionalToolArgs was allowlist-only (Read/Subagent) and only stripped empty-string/empty-array values, so Responses API strict mode (every property forced into `required`) could forward a forced non-empty value (e.g. Agent.isolation) or a schema-declared default value verbatim to the client. Add schema-aware drop-if-default and generalized drop-if-empty (any tool, gated on schema.required), and thread each tool's JSON Schema from the request's tools[] into the two streaming call sites (response.output_item.done handling). Closes #6951 --- .../fixes/6951-codex-optional-tool-params.md | 1 + .../translator/response/openai-responses.ts | 5 +- .../response/openai-responses/pureHelpers.ts | 90 ++++++++++-- .../response/openai-responses/toolSchemas.ts | 29 ++++ open-sse/utils/stream.ts | 4 + tests/unit/repro-6951.test.ts | 137 ++++++++++++++++++ 6 files changed, 252 insertions(+), 14 deletions(-) create mode 100644 changelog.d/fixes/6951-codex-optional-tool-params.md create mode 100644 open-sse/translator/response/openai-responses/toolSchemas.ts create mode 100644 tests/unit/repro-6951.test.ts diff --git a/changelog.d/fixes/6951-codex-optional-tool-params.md b/changelog.d/fixes/6951-codex-optional-tool-params.md new file mode 100644 index 0000000000..2d634fd24c --- /dev/null +++ b/changelog.d/fixes/6951-codex-optional-tool-params.md @@ -0,0 +1 @@ +- fix(sse): make Codex Responses tool-arg normalization schema-aware — drop values equal to the tool's declared JSON Schema `default`, generalize empty-optional stripping to any tool (not just `Read`/`Subagent`) via `schema.required`, and thread each tool's schema from the request's `tools[]` into the streaming response translator (#6951) diff --git a/open-sse/translator/response/openai-responses.ts b/open-sse/translator/response/openai-responses.ts index fe95ee7599..42fca56462 100644 --- a/open-sse/translator/response/openai-responses.ts +++ b/open-sse/translator/response/openai-responses.ts @@ -847,6 +847,7 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { const currentIndex = state.toolCallIndex; // capture before increment const callId = item.call_id || state.currentToolCallId || fallbackToolCallId(); const toolName = normalizeToolName(item.name); + const toolSchema = state.toolSchemas?.get(toolName); if (state.currentToolCallDeferred) { state.currentToolCallDeferred = false; @@ -859,7 +860,7 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { state.toolCallIndex++; - const argsToEmit = stripEmptyOptionalToolArgs(item.arguments, toolName); + const argsToEmit = stripEmptyOptionalToolArgs(item.arguments, toolName, toolSchema); const argsStr = argsToEmit != null @@ -901,7 +902,7 @@ function openaiResponsesToOpenAIResponseStream(chunk, state) { // Only emit if arguments exist in the done event AND they weren't already streamed via deltas if (item.arguments != null && !buffered) { - const argsToEmit = stripEmptyOptionalToolArgs(item.arguments, toolName); + const argsToEmit = stripEmptyOptionalToolArgs(item.arguments, toolName, toolSchema); const argsStr = typeof argsToEmit === "string" ? argsToEmit : JSON.stringify(argsToEmit); if (argsStr) { diff --git a/open-sse/translator/response/openai-responses/pureHelpers.ts b/open-sse/translator/response/openai-responses/pureHelpers.ts index 31f1abb73f..094d7dc58d 100644 --- a/open-sse/translator/response/openai-responses/pureHelpers.ts +++ b/open-sse/translator/response/openai-responses/pureHelpers.ts @@ -12,17 +12,89 @@ export function normalizeToolName(value) { // which Cursor rejects unless environment is cloud — ported from decolua/9router#2446. const STRIPPABLE_EMPTY_ARG_TOOLS = new Set(["Read", "Subagent"]); -export function stripEmptyOptionalToolArgs(value, toolName) { +// Deep-equal for JSON-shaped values (schema `default` comparison). Cheap and safe: +// tool args are always JSON-serializable, so a stringify comparison is exact. +function jsonValuesEqual(a, b) { + if (a === b) return true; + if (a == null || b == null) return false; + try { + return JSON.stringify(a) === JSON.stringify(b); + } catch { + return false; + } +} + +function hasUsableSchema(schema) { + return !!(schema && typeof schema === "object" && !Array.isArray(schema)); +} + +function schemaProperties(schema) { + return hasUsableSchema(schema) && schema.properties && typeof schema.properties === "object" + ? schema.properties + : null; +} + +function schemaRequiredSet(schema) { + return new Set(hasUsableSchema(schema) && Array.isArray(schema.required) ? schema.required : []); +} + +function isEmptyToolArgValue(entry) { + return entry === "" || (Array.isArray(entry) && entry.length === 0); +} + +// True when `entry` strictly equals the property's declared JSON Schema `default` — an +// emitted value indistinguishable from omission, safe to drop for any tool. +function matchesSchemaDefault(propSchema, entry) { + if (!propSchema || !Object.prototype.hasOwnProperty.call(propSchema, "default")) return false; + return jsonValuesEqual(entry, propSchema.default); +} + +// True when `entry` is empty and either the tool is on the legacy allowlist, or the +// schema declares this property but does not mark it `required` (generalized #6951 rule). +function isDroppableEmptyEntry(entry, propSchema, required, key, allowlisted) { + if (!isEmptyToolArgValue(entry)) return false; + return allowlisted || (propSchema != null && !required.has(key)); +} + +function stripEmptyOptionalToolArgsObject(value, toolName, schema) { + const properties = schemaProperties(schema); + const required = schemaRequiredSet(schema); + const allowlisted = STRIPPABLE_EMPTY_ARG_TOOLS.has(toolName); + + const cleaned = { ...value }; + for (const [key, entry] of Object.entries(cleaned)) { + const propSchema = properties ? properties[key] : null; + if ( + matchesSchemaDefault(propSchema, entry) || + isDroppableEmptyEntry(entry, propSchema, required, key, allowlisted) + ) { + delete cleaned[key]; + } + } + return cleaned; +} + +// #6951 — Responses API strict mode forces every tool property into `required`, so the +// model always emits *some* value for "optional" params (no first-class optional). +// When the tool's JSON Schema is available (`schema`, from the request's `tools[]`), +// normalization becomes schema-aware instead of allowlist-only: +// - drop-if-default: value strictly equals the property's declared `default`. +// - drop-if-empty (generalized): empty string/array for a property that is declared +// in `schema.properties` but absent from `schema.required` — any tool, not just the +// Read/Subagent allowlist above. +// Without a schema, behavior is unchanged (allowlist + empty-only), preserving existing +// callers that only pass (value, toolName). +export function stripEmptyOptionalToolArgs(value, toolName, schema) { if (value == null) return value; if (typeof value === "string") { - // JSON-string cleanup is intentionally scoped to the allowlisted tools above. - // For arbitrary tools, empty strings/arrays may be valid user payloads. - if (!STRIPPABLE_EMPTY_ARG_TOOLS.has(toolName)) return value; + // JSON-string cleanup runs for allowlisted tools, or for any tool once a schema is + // supplied (schema-aware normalization is not restricted to the allowlist). + if (!hasUsableSchema(schema) && !STRIPPABLE_EMPTY_ARG_TOOLS.has(toolName)) return value; try { const parsed = JSON.parse(value); if (Array.isArray(parsed) || typeof parsed !== "object" || parsed === null) return value; - const cleaned = stripEmptyOptionalToolArgs(parsed, toolName); + const cleaned = stripEmptyOptionalToolArgs(parsed, toolName, schema); return JSON.stringify(cleaned ?? {}); } catch { return value; @@ -31,13 +103,7 @@ export function stripEmptyOptionalToolArgs(value, toolName) { if (Array.isArray(value) || typeof value !== "object") return value; - const cleaned = { ...value }; - for (const [key, entry] of Object.entries(cleaned)) { - if (entry === "" || (Array.isArray(entry) && entry.length === 0)) { - delete cleaned[key]; - } - } - return cleaned; + return stripEmptyOptionalToolArgsObject(value, toolName, schema); } export function normalizeOutputIndex(outputIndex) { diff --git a/open-sse/translator/response/openai-responses/toolSchemas.ts b/open-sse/translator/response/openai-responses/toolSchemas.ts new file mode 100644 index 0000000000..8f5457ea72 --- /dev/null +++ b/open-sse/translator/response/openai-responses/toolSchemas.ts @@ -0,0 +1,29 @@ +// Pure, stateless helper: build a Map from a request body's +// `tools[]` (Chat Completions `{type:"function",function:{name,parameters}}` shape or +// Responses API `{type:"function",name,parameters}` shape). Used to thread each tool's +// JSON Schema into response-side normalization (#6951 — stripEmptyOptionalToolArgs) so +// it can be schema-aware instead of allowlist-only. No stream state, no host import. + +type JsonRecord = Record; + +function asRecord(value: unknown): JsonRecord | null { + return value && typeof value === "object" && !Array.isArray(value) ? (value as JsonRecord) : null; +} + +export function extractToolSchemaMap(body: unknown): Map | null { + const record = asRecord(body); + const tools = record?.tools; + if (!Array.isArray(tools)) return null; + + const map = new Map(); + for (const tool of tools) { + const item = asRecord(tool); + if (!item) continue; + const fn = asRecord(item.function); + const name = (typeof fn?.name === "string" ? fn.name : typeof item.name === "string" ? item.name : "").trim(); + if (!name) continue; + const schema = asRecord(fn?.parameters ?? item.parameters); + if (schema) map.set(name, schema); + } + return map.size > 0 ? map : null; +} diff --git a/open-sse/utils/stream.ts b/open-sse/utils/stream.ts index ded2ed700d..581f1f6342 100644 --- a/open-sse/utils/stream.ts +++ b/open-sse/utils/stream.ts @@ -38,6 +38,7 @@ import { import { buildErrorBody } from "./error.ts"; import { parseTextualToolCallCandidate, isValidToolCallHeaderPrefix } from "./textualToolCall.ts"; import { recordToolLatency } from "../services/toolLatencyTracker.ts"; +import { extractToolSchemaMap } from "../translator/response/openai-responses/toolSchemas.ts"; import { generateSessionId, markToolFinish, @@ -153,6 +154,8 @@ type TranslateState = ReturnType & { accumulatedContent?: string; /** Accumulated reasoning content (separate from content) */ accumulatedReasoning?: string; + /** #6951 — per-tool JSON Schema (from request `tools[]`), keyed by tool name. */ + toolSchemas?: Map> | null; upstreamError?: { status: number; type: string; @@ -687,6 +690,7 @@ export function createSSEStream(options: StreamOptions = {}) { suppressThinkClose, accumulatedContent: "", accumulatedReasoning: "", + toolSchemas: extractToolSchemaMap(body), } : null; diff --git a/tests/unit/repro-6951.test.ts b/tests/unit/repro-6951.test.ts new file mode 100644 index 0000000000..9de9bf3912 --- /dev/null +++ b/tests/unit/repro-6951.test.ts @@ -0,0 +1,137 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +// #6951 — Codex Responses API strict mode forces every tool property into +// `required`, so the model always emits *some* value for "optional" params. +// `stripEmptyOptionalToolArgs` (open-sse/translator/response/openai-responses/pureHelpers.ts) +// used to be an allowlist of 2 tool names that only stripped empty-string/empty-array +// values, so a forced non-empty value on a non-allowlisted tool (or a schema-declared +// default value) was always forwarded verbatim to the client. This test proves the +// schema-aware normalization (drop-if-default, generalized drop-if-empty) added for #6951, +// and the end-to-end schema threading from the request's `tools[]` into the streaming +// call sites in `openai-responses.ts` (response.output_item.done handling). + +const { stripEmptyOptionalToolArgs } = await import( + "../../open-sse/translator/response/openai-responses/pureHelpers.ts" +); +const { extractToolSchemaMap } = await import( + "../../open-sse/translator/response/openai-responses/toolSchemas.ts" +); +const { openaiResponsesToOpenAIResponse } = await import( + "../../open-sse/translator/response/openai-responses.ts" +); + +const AGENT_SCHEMA = { + type: "object", + properties: { + description: { type: "string" }, + prompt: { type: "string" }, + subagent_type: { type: "string" }, + model: { type: "string" }, + run_in_background: { type: "boolean" }, + isolation: { type: "string", enum: ["local", "remote"], default: "local" }, + cloud_base_branch: { type: "string" }, + }, + // Responses API strict mode: every property is required, even "optional" ones. + required: [ + "description", + "prompt", + "subagent_type", + "model", + "run_in_background", + "isolation", + "cloud_base_branch", + ], +}; + +test("6951: drop-if-default — forced value equal to the schema default is stripped", () => { + const raw = JSON.stringify({ + description: "x", + prompt: "y", + subagent_type: "claude", + model: "sonnet", + run_in_background: false, + isolation: "local", // matches schema default -> indistinguishable from omission + cloud_base_branch: "", + }); + const cleaned = JSON.parse(stripEmptyOptionalToolArgs(raw, "Agent", AGENT_SCHEMA)); + assert.equal(Object.prototype.hasOwnProperty.call(cleaned, "isolation"), false); + // cloud_base_branch is required by this schema -> must NOT be stripped just for being empty. + assert.equal(Object.prototype.hasOwnProperty.call(cleaned, "cloud_base_branch"), true); +}); + +test("6951: generalized drop-if-empty — empty optional prop stripped for ANY tool when not in schema.required", () => { + const schema = { + type: "object", + properties: { note: { type: "string" }, tags: { type: "array" } }, + required: [], // both optional + }; + const raw = JSON.stringify({ note: "", tags: [] }); + const cleaned = JSON.parse(stripEmptyOptionalToolArgs(raw, "SomeOtherTool", schema)); + assert.equal(Object.prototype.hasOwnProperty.call(cleaned, "note"), false); + assert.equal(Object.prototype.hasOwnProperty.call(cleaned, "tags"), false); +}); + +test("6951: required-by-schema empty prop is preserved (safety — not indistinguishable from omission)", () => { + const schema = { + type: "object", + properties: { note: { type: "string" } }, + required: ["note"], + }; + const raw = JSON.stringify({ note: "" }); + const cleaned = JSON.parse(stripEmptyOptionalToolArgs(raw, "SomeOtherTool", schema)); + assert.equal(Object.prototype.hasOwnProperty.call(cleaned, "note"), true); +}); + +test("6951: no schema supplied — behavior unchanged (allowlist + empty-only, backward compatible)", () => { + const raw = JSON.stringify({ query: "", tags: [] }); + assert.equal(stripEmptyOptionalToolArgs(raw, "SomeOtherTool"), raw); +}); + +test("6951: extractToolSchemaMap builds a name->schema map from Chat Completions tools[]", () => { + const body = { + tools: [ + { type: "function", function: { name: "Agent", parameters: AGENT_SCHEMA } }, + { type: "function", function: { name: "no_schema" } }, + ], + }; + const map = extractToolSchemaMap(body); + assert.equal(map?.get("Agent"), AGENT_SCHEMA); + assert.equal(map?.has("no_schema"), false); + assert.equal(extractToolSchemaMap({}), null); +}); + +test("6951: RED->GREEN — schema threaded end-to-end strips the default-valued isolation arg", () => { + // Simulates createSSEStream's TranslateState carrying `toolSchemas` extracted from the + // request body, as wired in open-sse/utils/stream.ts. + const state = { toolSchemas: new Map([["Agent", AGENT_SCHEMA]]) }; + + openaiResponsesToOpenAIResponse( + { type: "response.output_item.added", item: { type: "function_call", call_id: "call_1", name: "Agent" } }, + state + ); + const done = openaiResponsesToOpenAIResponse( + { + type: "response.output_item.done", + item: { + type: "function_call", + call_id: "call_1", + name: "Agent", + arguments: JSON.stringify({ + description: "d", + prompt: "p", + subagent_type: "claude", + model: "sonnet", + run_in_background: false, + isolation: "local", + cloud_base_branch: "", + }), + }, + }, + state + ); + + const args = JSON.parse(done.choices[0].delta.tool_calls[0].function.arguments); + assert.equal(Object.prototype.hasOwnProperty.call(args, "isolation"), false); + assert.equal(args.description, "d"); +});