mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-07-26 09:52:11 +03:00
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
This commit is contained in:
committed by
GitHub
parent
9b43a00b60
commit
e078664ddd
1
changelog.d/fixes/6951-codex-optional-tool-params.md
Normal file
1
changelog.d/fixes/6951-codex-optional-tool-params.md
Normal file
@@ -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)
|
||||
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
29
open-sse/translator/response/openai-responses/toolSchemas.ts
Normal file
29
open-sse/translator/response/openai-responses/toolSchemas.ts
Normal file
@@ -0,0 +1,29 @@
|
||||
// Pure, stateless helper: build a Map<toolName, parametersSchema> 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<string, unknown>;
|
||||
|
||||
function asRecord(value: unknown): JsonRecord | null {
|
||||
return value && typeof value === "object" && !Array.isArray(value) ? (value as JsonRecord) : null;
|
||||
}
|
||||
|
||||
export function extractToolSchemaMap(body: unknown): Map<string, JsonRecord> | null {
|
||||
const record = asRecord(body);
|
||||
const tools = record?.tools;
|
||||
if (!Array.isArray(tools)) return null;
|
||||
|
||||
const map = new Map<string, JsonRecord>();
|
||||
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;
|
||||
}
|
||||
@@ -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<typeof initState> & {
|
||||
accumulatedContent?: string;
|
||||
/** Accumulated reasoning content (separate from content) */
|
||||
accumulatedReasoning?: string;
|
||||
/** #6951 — per-tool JSON Schema (from request `tools[]`), keyed by tool name. */
|
||||
toolSchemas?: Map<string, Record<string, unknown>> | null;
|
||||
upstreamError?: {
|
||||
status: number;
|
||||
type: string;
|
||||
@@ -687,6 +690,7 @@ export function createSSEStream(options: StreamOptions = {}) {
|
||||
suppressThinkClose,
|
||||
accumulatedContent: "",
|
||||
accumulatedReasoning: "",
|
||||
toolSchemas: extractToolSchemaMap(body),
|
||||
}
|
||||
: null;
|
||||
|
||||
|
||||
137
tests/unit/repro-6951.test.ts
Normal file
137
tests/unit/repro-6951.test.ts
Normal file
@@ -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");
|
||||
});
|
||||
Reference in New Issue
Block a user