From 7a28fa552cd9385029cb74ed9da8b2f7270c5ad7 Mon Sep 17 00:00:00 2001 From: olddonkey Date: Wed, 26 Aug 2026 09:58:37 -0700 Subject: [PATCH 1/5] fix(xai): normalize Responses root tool schemas --- src/adapters/openai-chat.ts | 274 +-------------------- src/adapters/openai-responses.ts | 56 +++-- src/adapters/xai-tool-schema.ts | 274 +++++++++++++++++++++ structure/04_transports-and-sidecars.md | 7 + tests/openai-responses-passthrough.test.ts | 115 +++++++++ 5 files changed, 442 insertions(+), 284 deletions(-) create mode 100644 src/adapters/xai-tool-schema.ts diff --git a/src/adapters/openai-chat.ts b/src/adapters/openai-chat.ts index eed42a525f..9add6b8a8c 100644 --- a/src/adapters/openai-chat.ts +++ b/src/adapters/openai-chat.ts @@ -26,6 +26,11 @@ import { } from "../providers/fastwire"; import { openaiChatCompletionsUrl } from "./openai-chat-url"; import { stripResponsesOnlyEncryptedMarker } from "./responses-tool-schema"; +import { + isXaiSchemaTarget, + lookupLocalJsonPointer, + normalizeXaiToolParameters, +} from "./xai-tool-schema"; import { isTranslatorBudgetExceededError, retainTranslatedEventBatch, @@ -953,16 +958,6 @@ function sanitizeAzureChatToolParameters(parameters: unknown): Record typeof item === "string") - : []; -} - -/** Variant keys the merger can keep. Anything else is refused, not silently dropped. */ -const XAI_VARIANT_MERGE_KEYS = new Set([ - "type", - "properties", - "required", - "additionalProperties", - "description", - "title", - "$comment", - "$defs", - "definitions", -]); - -function decodeJsonPointerToken(token: string): string { - return token.replace(/~1/g, "/").replace(/~0/g, "~"); -} - -function lookupLocalJsonPointer(root: unknown, ref: string): unknown { - if (ref === "#" || ref === "#/") return root; - if (!ref.startsWith("#/")) return undefined; - let current: unknown = root; - for (const token of ref.slice(2).split("/").map(decodeJsonPointerToken)) { - if (!isXaiObjectSchema(current) || !Object.hasOwn(current, token)) return undefined; - current = current[token]; - } - return current; -} - -/** Resolve local `#/` `$ref`s. Unresolvable or cyclic refs return undefined. */ -function resolveXaiSchemaRefs( - schema: unknown, - root: Record, - stack: Set = new Set(), -): unknown | undefined { - if (!isXaiObjectSchema(schema)) return schema; - if (typeof schema.$ref === "string") { - const ref = schema.$ref; - if (stack.has(ref)) return undefined; - const target = lookupLocalJsonPointer(root, ref); - if (target === undefined) return undefined; - stack.add(ref); - const resolvedTarget = resolveXaiSchemaRefs(target, root, stack); - stack.delete(ref); - if (resolvedTarget === undefined) return undefined; - const rest: Record = { ...schema }; - delete rest.$ref; - if (Object.keys(rest).length === 0) return resolvedTarget; - const resolvedRest = resolveXaiSchemaRefs(rest, root, stack); - if (resolvedRest === undefined || !isXaiObjectSchema(resolvedTarget) || !isXaiObjectSchema(resolvedRest)) { - return undefined; - } - return composeXaiObjectSchemas(resolvedTarget, resolvedRest); - } - - const resolved: Record = {}; - for (const [key, value] of Object.entries(schema)) { - if ((key === "oneOf" || key === "anyOf") && Array.isArray(value)) { - const items: unknown[] = []; - for (const item of value) { - const next = resolveXaiSchemaRefs(item, root, stack); - if (next === undefined) return undefined; - items.push(next); - } - resolved[key] = items; - continue; - } - if (key === "properties" && isXaiObjectSchema(value)) { - const properties: Record = {}; - for (const [name, property] of Object.entries(value)) { - const next = resolveXaiSchemaRefs(property, root, stack); - if (next === undefined) return undefined; - properties[name] = next; - } - resolved[key] = properties; - continue; - } - resolved[key] = value; - } - return resolved; -} - -function xaiVariantIsConcreteObject(variant: Record): boolean { - if (variant.type !== undefined && variant.type !== "object") return false; - return Object.keys(variant).every(key => XAI_VARIANT_MERGE_KEYS.has(key)); -} - -function variantProperties(variant: Record): Record { - return isXaiObjectSchema(variant.properties) ? variant.properties : {}; -} - -/** - * Independent per-property anyOf is lossless only when every property name exists - * on every variant (absence is meaningful under xAI's default additionalProperties: - * false, and promoting a branch-local key also tightens explicit-true variants) - * and at most one of those shared properties has a conflicting schema. - */ -function xaiPropertyMergeIsLossless(variants: Record[]): boolean { - const names = new Set(); - const props = variants.map(variant => { - const properties = variantProperties(variant); - for (const name of Object.keys(properties)) names.add(name); - return properties; - }); - let schemaConflicts = 0; - for (const name of names) { - const values = props.map(property => property[name]); - if (values.some(value => value === undefined)) return false; - if (values.some(value => JSON.stringify(value) !== JSON.stringify(values[0]))) schemaConflicts += 1; - } - return schemaConflicts <= 1; -} - -function xaiRequiredSetsMatch(variants: Record[]): boolean { - const serialized = variants.map(variant => [...stringRequiredFields(variant.required)].sort().join("\0")); - return serialized.every(value => value === serialized[0]); -} - -function mergeXaiAdditionalProperties( - variants: Record[], -): { ok: true; value?: unknown } | { ok: false } { - const values = variants.map(variant => variant.additionalProperties); - const explicit = values.filter(value => value !== undefined); - if (explicit.length === 0) return { ok: true }; - if (explicit.length !== values.length) return { ok: false }; - const hasFalse = explicit.some(value => value === false); - const permissive = explicit.filter(value => value !== false); - if (hasFalse && permissive.length > 0) return { ok: false }; - if (hasFalse) return { ok: true, value: false }; - const unique: unknown[] = []; - const seen = new Set(); - for (const value of permissive) { - const key = JSON.stringify(value); - if (seen.has(key)) continue; - seen.add(key); - unique.push(value); - } - if (unique.length !== 1) return { ok: false }; - return { ok: true, value: unique[0] }; -} - -/** Compose root siblings into a branch so properties/required are not overwritten. */ -function composeXaiObjectSchemas( - inherited: Record, - branch: Record, -): Record { - const composed: Record = { ...inherited, ...branch }; - const inheritedProps = isXaiObjectSchema(inherited.properties) ? inherited.properties : undefined; - const branchProps = isXaiObjectSchema(branch.properties) ? branch.properties : undefined; - if (inheritedProps || branchProps) { - const properties: Record = { ...(inheritedProps ?? {}) }; - for (const [name, value] of Object.entries(branchProps ?? {})) { - const inheritedValue = inheritedProps?.[name]; - properties[name] = inheritedValue !== undefined && JSON.stringify(inheritedValue) !== JSON.stringify(value) - ? { allOf: [inheritedValue, value] } - : value; - } - composed.properties = properties; - } - const required = [...new Set([ - ...stringRequiredFields(inherited.required), - ...stringRequiredFields(branch.required), - ])]; - if (required.length > 0) composed.required = required; - else delete composed.required; - return composed; -} - -function expandXaiRootObjectSchemas(schema: unknown): Record[] | undefined { - if (!isXaiObjectSchema(schema)) return undefined; - const compositionKey = ["oneOf", "anyOf"].find(key => Array.isArray(schema[key])); - if (!compositionKey) { - if (schema.type !== undefined && schema.type !== "object") return undefined; - return [{ ...schema, type: "object" }]; - } - - const siblings = Object.fromEntries(Object.entries(schema).filter(([key]) => key !== compositionKey)); - const branches = schema[compositionKey]; - if (!Array.isArray(branches)) return undefined; - const expanded: Record[] = []; - for (const branch of branches) { - const variants = expandXaiRootObjectSchemas(branch); - if (!variants) return undefined; - for (const variant of variants) expanded.push(composeXaiObjectSchemas(siblings, variant)); - } - return expanded.length > 0 ? expanded : undefined; -} - -function mergeXaiPropertySchemas(values: unknown[]): unknown { - const unique: unknown[] = []; - const serialized = new Set(); - for (const value of values) { - const key = JSON.stringify(value); - if (serialized.has(key)) continue; - serialized.add(key); - unique.push(value); - } - return unique.length === 1 ? unique[0] : { anyOf: unique }; -} - -/** - * The Grok CLI proxy rejects a function parameter schema whose root remains oneOf/anyOf. - * Flatten only when the merge is lossless: local $refs resolve, every variant is a concrete - * object whose keys we can preserve, required sets match, additionalProperties does not change - * meaning, every property name exists on every variant, and at most one property schema - * differs. Otherwise omit the tool rather than emit a weaker schema. - */ -function normalizeXaiToolParameters(parameters: unknown): Record | undefined { - if (!isXaiObjectSchema(parameters)) return undefined; - const resolved = resolveXaiSchemaRefs(parameters, parameters); - if (!isXaiObjectSchema(resolved)) return undefined; - - const normalizedRoot = { ...resolved }; - delete normalizedRoot.$schema; - - const variants = expandXaiRootObjectSchemas(normalizedRoot); - if (!variants) return undefined; - if (variants.length === 1) { - return xaiVariantIsConcreteObject(variants[0]) ? variants[0] : undefined; - } - if (!variants.every(xaiVariantIsConcreteObject) || !xaiRequiredSetsMatch(variants)) return undefined; - const additionalProperties = mergeXaiAdditionalProperties(variants); - if (!additionalProperties.ok) return undefined; - if (!xaiPropertyMergeIsLossless(variants)) return undefined; - - const metadata = Object.fromEntries(Object.entries(normalizedRoot).filter(([key]) => key !== "oneOf" && key !== "anyOf" && key !== "type")); - delete metadata.properties; - delete metadata.required; - delete metadata.additionalProperties; - - const propertyValues = new Map(); - for (const variant of variants) { - if (!variant.properties || typeof variant.properties !== "object" || Array.isArray(variant.properties)) continue; - for (const [name, value] of Object.entries(variant.properties as Record)) { - const values = propertyValues.get(name) ?? []; - values.push(value); - propertyValues.set(name, values); - } - } - - const properties = Object.fromEntries( - [...propertyValues].map(([name, values]) => [name, mergeXaiPropertySchemas(values)]), - ); - const required = stringRequiredFields(variants[0]?.required); - - return { - ...metadata, - type: "object", - properties, - ...(required.length > 0 ? { required } : {}), - ...("value" in additionalProperties ? { additionalProperties: additionalProperties.value } : {}), - }; -} - function toolsToChatFormat(parsed: OcxParsedRequest, provider: OcxProviderConfig): unknown[] | undefined { if (!parsed.context.tools || parsed.context.tools.length === 0) return undefined; const tools = parsed.context.tools.filter(toolChoiceToolPredicate(parsed.options.toolChoice, parsed.context.tools)); diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index 305cc5d755..6ac5176c97 100644 --- a/src/adapters/openai-responses.ts +++ b/src/adapters/openai-responses.ts @@ -20,6 +20,7 @@ import { rewriteRoutedToolSearchForUpstream } from "../responses/tool-search-com import { rewriteRoutedNamespaceToolsForUpstream } from "../responses/namespace-tool-compat"; import { openaiResponsesUrl } from "./openai-responses-url"; import { normalizeXaiResponsesWebSearch } from "./xai-web-search"; +import { isXaiSchemaTarget, normalizeXaiToolParameters } from "./xai-tool-schema"; import { createAdapterTierMetadata, } from "../providers/fastwire"; @@ -531,8 +532,12 @@ function isPlainObject(v: unknown): v is Record { return !!v && typeof v === "object" && !Array.isArray(v); } -function normalizeFunctionToolSchema(tool: unknown): unknown { +function normalizeFunctionToolSchema(tool: unknown, xaiTarget: boolean): unknown | undefined { if (!isPlainObject(tool) || tool.type !== "function") return tool; + if (xaiTarget) { + const parameters = normalizeXaiToolParameters(isPlainObject(tool.parameters) ? tool.parameters : {}); + return parameters === undefined ? undefined : { ...tool, parameters }; + } if (isPlainObject(tool.parameters) && tool.parameters.type === "object") return tool; return { ...tool, @@ -540,16 +545,21 @@ function normalizeFunctionToolSchema(tool: unknown): unknown { }; } -function normalizeToolSchemas(body: unknown): unknown { +function normalizeToolSchemas(body: unknown, xaiTarget: boolean): unknown { if (!isPlainObject(body)) return body; const normalizeTools = (tools: unknown[]): unknown[] => { let changed = false; - const normalized = tools.map((tool) => { - const fixed = normalizeFunctionToolSchema(tool); + const normalized: unknown[] = []; + for (const tool of tools) { + const fixed = normalizeFunctionToolSchema(tool, xaiTarget); + if (fixed === undefined) { + changed = true; + continue; + } if (fixed !== tool) changed = true; - return fixed; - }); + normalized.push(fixed); + } return changed ? normalized : tools; }; @@ -1998,15 +2008,31 @@ export function createResponsesPassthroughAdapter(provider: OcxProviderConfig): outBody = buildRoutedCompactionBody(outBody); } const threadServingIdentityChanged = parsed._stripReasoningEncryptedContent === true; - const sanitizedBody = normalizeToolSchemas(stripSparkCompatibility(stripUnsupportedReasoningParams(stripItemIdsWhenUnstored(stripInvalidItemIds(stripUnsupportedHostedTools(sanitizeReasoningInputContent(scrubOcxCompactionItems( - outBody, - destinationDecodesNativeCompactionBlob(provider), - threadServingIdentityChanged, - ), { - preserveRawReasoningContent: provider.preserveResponsesReasoningContent === true, - dropNullContentChannel: !isOpenAiOperatedResponsesDestination(provider), - stripEncryptedContent: threadServingIdentityChanged, - }))))))); + const sanitizedBody = normalizeToolSchemas( + stripSparkCompatibility( + stripUnsupportedReasoningParams( + stripItemIdsWhenUnstored( + stripInvalidItemIds( + stripUnsupportedHostedTools( + sanitizeReasoningInputContent( + scrubOcxCompactionItems( + outBody, + destinationDecodesNativeCompactionBlob(provider), + threadServingIdentityChanged, + ), + { + preserveRawReasoningContent: provider.preserveResponsesReasoningContent === true, + dropNullContentChannel: !isOpenAiOperatedResponsesDestination(provider), + stripEncryptedContent: threadServingIdentityChanged, + }, + ), + ), + ), + ), + ), + ), + isXaiSchemaTarget(provider), + ); const finalBody = stripDisabledVerbosity( stripDisabledReasoningSummaries( normalizeConfiguredReasoningSummaryDelivery(sanitizedBody, provider, parsed.modelId), diff --git a/src/adapters/xai-tool-schema.ts b/src/adapters/xai-tool-schema.ts new file mode 100644 index 0000000000..544f72cae5 --- /dev/null +++ b/src/adapters/xai-tool-schema.ts @@ -0,0 +1,274 @@ +import type { OcxProviderConfig } from "../types"; + +function isSchemaObject(value: unknown): value is Record { + return Boolean(value) && typeof value === "object" && !Array.isArray(value); +} + +export function isXaiSchemaTarget(provider: Pick): boolean { + try { + // Public api.x.ai accepts native root object unions. Only the Grok CLI proxy + // 400s on a root oneOf/anyOf, so flattening/omitting is scoped to that host. + return new URL(provider.baseUrl).hostname === "cli-chat-proxy.grok.com"; + } catch { + return false; + } +} + +function stringRequiredFields(value: unknown): string[] { + return Array.isArray(value) + ? value.filter((item): item is string => typeof item === "string") + : []; +} + +/** Variant keys the merger can keep. Anything else is refused, not silently dropped. */ +const XAI_VARIANT_MERGE_KEYS = new Set([ + "type", + "properties", + "required", + "additionalProperties", + "description", + "title", + "$comment", + "$defs", + "definitions", +]); + +function decodeJsonPointerToken(token: string): string { + return token.replace(/~1/g, "/").replace(/~0/g, "~"); +} + +export function lookupLocalJsonPointer(root: unknown, ref: string): unknown { + if (ref === "#" || ref === "#/") return root; + if (!ref.startsWith("#/")) return undefined; + let current: unknown = root; + for (const token of ref.slice(2).split("/").map(decodeJsonPointerToken)) { + if (!isSchemaObject(current) || !Object.hasOwn(current, token)) return undefined; + current = current[token]; + } + return current; +} + +/** Resolve local `#/` `$ref`s. Unresolvable or cyclic refs return undefined. */ +function resolveXaiSchemaRefs( + schema: unknown, + root: Record, + stack: Set = new Set(), +): unknown | undefined { + if (!isSchemaObject(schema)) return schema; + if (typeof schema.$ref === "string") { + const ref = schema.$ref; + if (stack.has(ref)) return undefined; + const target = lookupLocalJsonPointer(root, ref); + if (target === undefined) return undefined; + stack.add(ref); + const resolvedTarget = resolveXaiSchemaRefs(target, root, stack); + stack.delete(ref); + if (resolvedTarget === undefined) return undefined; + const rest: Record = { ...schema }; + delete rest.$ref; + if (Object.keys(rest).length === 0) return resolvedTarget; + const resolvedRest = resolveXaiSchemaRefs(rest, root, stack); + if (resolvedRest === undefined || !isSchemaObject(resolvedTarget) || !isSchemaObject(resolvedRest)) { + return undefined; + } + return composeXaiObjectSchemas(resolvedTarget, resolvedRest); + } + + const resolved: Record = {}; + for (const [key, value] of Object.entries(schema)) { + if ((key === "oneOf" || key === "anyOf") && Array.isArray(value)) { + const items: unknown[] = []; + for (const item of value) { + const next = resolveXaiSchemaRefs(item, root, stack); + if (next === undefined) return undefined; + items.push(next); + } + resolved[key] = items; + continue; + } + if (key === "properties" && isSchemaObject(value)) { + const properties: Record = {}; + for (const [name, property] of Object.entries(value)) { + const next = resolveXaiSchemaRefs(property, root, stack); + if (next === undefined) return undefined; + properties[name] = next; + } + resolved[key] = properties; + continue; + } + resolved[key] = value; + } + return resolved; +} + +function xaiVariantIsConcreteObject(variant: Record): boolean { + if (variant.type !== undefined && variant.type !== "object") return false; + return Object.keys(variant).every(key => XAI_VARIANT_MERGE_KEYS.has(key)); +} + +function variantProperties(variant: Record): Record { + return isSchemaObject(variant.properties) ? variant.properties : {}; +} + +/** + * Independent per-property anyOf is lossless only when every property name exists + * on every variant (absence is meaningful under xAI's default additionalProperties: + * false, and promoting a branch-local key also tightens explicit-true variants) + * and at most one property schema differs. + */ +function xaiPropertyMergeIsLossless(variants: Record[]): boolean { + const names = new Set(); + const props = variants.map(variant => { + const properties = variantProperties(variant); + for (const name of Object.keys(properties)) names.add(name); + return properties; + }); + let schemaConflicts = 0; + for (const name of names) { + const values = props.map(property => property[name]); + if (values.some(value => value === undefined)) return false; + if (values.some(value => JSON.stringify(value) !== JSON.stringify(values[0]))) schemaConflicts += 1; + } + return schemaConflicts <= 1; +} + +function xaiRequiredSetsMatch(variants: Record[]): boolean { + const serialized = variants.map(variant => [...stringRequiredFields(variant.required)].sort().join("\0")); + return serialized.every(value => value === serialized[0]); +} + +function mergeXaiAdditionalProperties( + variants: Record[], +): { ok: true; value?: unknown } | { ok: false } { + const values = variants.map(variant => variant.additionalProperties); + const explicit = values.filter(value => value !== undefined); + if (explicit.length === 0) return { ok: true }; + if (explicit.length !== values.length) return { ok: false }; + const hasFalse = explicit.some(value => value === false); + const permissive = explicit.filter(value => value !== false); + if (hasFalse && permissive.length > 0) return { ok: false }; + if (hasFalse) return { ok: true, value: false }; + const unique: unknown[] = []; + const seen = new Set(); + for (const value of permissive) { + const key = JSON.stringify(value); + if (seen.has(key)) continue; + seen.add(key); + unique.push(value); + } + if (unique.length !== 1) return { ok: false }; + return { ok: true, value: unique[0] }; +} + +/** Compose root siblings into a branch so properties/required are not overwritten. */ +function composeXaiObjectSchemas( + inherited: Record, + branch: Record, +): Record { + const composed: Record = { ...inherited, ...branch }; + const inheritedProps = isSchemaObject(inherited.properties) ? inherited.properties : undefined; + const branchProps = isSchemaObject(branch.properties) ? branch.properties : undefined; + if (inheritedProps || branchProps) { + const properties: Record = { ...(inheritedProps ?? {}) }; + for (const [name, value] of Object.entries(branchProps ?? {})) { + const inheritedValue = inheritedProps?.[name]; + properties[name] = inheritedValue !== undefined && JSON.stringify(inheritedValue) !== JSON.stringify(value) + ? { allOf: [inheritedValue, value] } + : value; + } + composed.properties = properties; + } + const required = [...new Set([ + ...stringRequiredFields(inherited.required), + ...stringRequiredFields(branch.required), + ])]; + if (required.length > 0) composed.required = required; + else delete composed.required; + return composed; +} + +function expandXaiRootObjectSchemas(schema: unknown): Record[] | undefined { + if (!isSchemaObject(schema)) return undefined; + const compositionKey = ["oneOf", "anyOf"].find(key => Array.isArray(schema[key])); + if (!compositionKey) { + if (schema.type !== undefined && schema.type !== "object") return undefined; + return [{ ...schema, type: "object" }]; + } + + const siblings = Object.fromEntries(Object.entries(schema).filter(([key]) => key !== compositionKey)); + const branches = schema[compositionKey]; + if (!Array.isArray(branches)) return undefined; + const expanded: Record[] = []; + for (const branch of branches) { + const variants = expandXaiRootObjectSchemas(branch); + if (!variants) return undefined; + for (const variant of variants) expanded.push(composeXaiObjectSchemas(siblings, variant)); + } + return expanded.length > 0 ? expanded : undefined; +} + +function mergeXaiPropertySchemas(values: unknown[]): unknown { + const unique: unknown[] = []; + const serialized = new Set(); + for (const value of values) { + const key = JSON.stringify(value); + if (serialized.has(key)) continue; + serialized.add(key); + unique.push(value); + } + return unique.length === 1 ? unique[0] : { anyOf: unique }; +} + +/** + * The Grok CLI proxy rejects a function parameter schema whose root remains oneOf/anyOf. + * Flatten only when the merge is lossless: local $refs resolve, every variant is a concrete + * object whose keys we can preserve, required sets match, additionalProperties does not change + * meaning, every property name exists on every variant, and at most one property schema + * differs. Otherwise omit the tool rather than emit a weaker schema. + */ +export function normalizeXaiToolParameters(parameters: unknown): Record | undefined { + if (!isSchemaObject(parameters)) return undefined; + const resolved = resolveXaiSchemaRefs(parameters, parameters); + if (!isSchemaObject(resolved)) return undefined; + + const normalizedRoot = { ...resolved }; + delete normalizedRoot.$schema; + + const variants = expandXaiRootObjectSchemas(normalizedRoot); + if (!variants) return undefined; + if (variants.length === 1) { + return xaiVariantIsConcreteObject(variants[0]) ? variants[0] : undefined; + } + if (!variants.every(xaiVariantIsConcreteObject) || !xaiRequiredSetsMatch(variants)) return undefined; + const additionalProperties = mergeXaiAdditionalProperties(variants); + if (!additionalProperties.ok) return undefined; + if (!xaiPropertyMergeIsLossless(variants)) return undefined; + + const metadata = Object.fromEntries(Object.entries(normalizedRoot).filter(([key]) => key !== "oneOf" && key !== "anyOf" && key !== "type")); + delete metadata.properties; + delete metadata.required; + delete metadata.additionalProperties; + + const propertyValues = new Map(); + for (const variant of variants) { + if (!isSchemaObject(variant.properties)) continue; + for (const [name, value] of Object.entries(variant.properties)) { + const values = propertyValues.get(name) ?? []; + values.push(value); + propertyValues.set(name, values); + } + } + + const properties = Object.fromEntries( + [...propertyValues].map(([name, values]) => [name, mergeXaiPropertySchemas(values)]), + ); + const required = stringRequiredFields(variants[0]?.required); + + return { + ...metadata, + type: "object", + properties, + ...(required.length > 0 ? { required } : {}), + ...("value" in additionalProperties ? { additionalProperties: additionalProperties.value } : {}), + }; +} diff --git a/structure/04_transports-and-sidecars.md b/structure/04_transports-and-sidecars.md index 53f69dd7aa..8c1fa4822d 100644 --- a/structure/04_transports-and-sidecars.md +++ b/structure/04_transports-and-sidecars.md @@ -124,6 +124,13 @@ Codex-private tool fields are removed at the same boundary from one table web-search variant, and `defer_loading` on any declaration, which `activateDeferredTool` clears only for tools a `tool_search_output` already loaded. A new private bit is a row there. +After that namespace boundary has produced public function tools, the Grok CLI Responses transport +applies the same root-schema policy as its Chat transport. A root `oneOf`/`anyOf` is flattened only +when the shared xAI normalizer can preserve its meaning; an unsafe function is omitted instead of +letting one incompatible declaration reject the entire request before inference. This is scoped to +`cli-chat-proxy.grok.com`: public `api.x.ai` keeps native root unions, as do unrelated Responses +gateways. Both top-level `tools` and Responses Lite `additional_tools` pass through this policy. + The same noncanonical boundary strips ChatGPT's private `external_web_access` bit from routed `web_search` declarations. The public tool remains enabled and all other options remain intact; canonical OpenAI forwarding preserves the bit. xAI's public Responses schema enables browsing by diff --git a/tests/openai-responses-passthrough.test.ts b/tests/openai-responses-passthrough.test.ts index 98db0c2d8c..7c1df08d9b 100644 --- a/tests/openai-responses-passthrough.test.ts +++ b/tests/openai-responses-passthrough.test.ts @@ -3,6 +3,7 @@ import { createResponsesPassthroughAdapter as createResponsesPassthroughAdapterP import { openaiResponsesUrl } from "../src/adapters/openai-responses-url"; import { enrichProviderFromRegistry, providerConfigSeed } from "../src/providers/derive"; import { getProviderRegistryEntry } from "../src/providers/registry"; +import { XAI_GROK_CLI_BASE_URL } from "../src/providers/xai-transport"; import { routeModel } from "../src/router"; import { resolveWireProtocolOverride } from "../src/server/adapter-resolve"; import { handleResponses, sanitizeEncryptedContentInPlace } from "../src/server/responses"; @@ -874,6 +875,120 @@ describe("OpenAI Responses passthrough sanitization", () => { ]); }); + test("normalizes or omits xAI CLI root unions after namespace lowering", () => { + const unsafeAutomationParameters = { + oneOf: [ + { + type: "object", + properties: { mode: { type: "string", enum: ["view"] } }, + required: ["mode"], + }, + { + oneOf: [ + { + type: "object", + properties: { id: { type: "string" }, mode: { const: "update" } }, + required: ["id", "mode"], + }, + { + type: "object", + properties: { name: { type: "string" }, mode: { const: "create" } }, + required: ["name", "mode"], + }, + ], + }, + ], + }; + const safeUnionParameters = { + type: "object", + properties: { token: { type: "string" } }, + required: ["token"], + oneOf: [ + { properties: { mode: { const: "view" } } }, + { properties: { mode: { const: "delete" } } }, + ], + }; + const namespace = { + type: "namespace", + name: "mcp__codex_app", + tools: [ + { type: "function", name: "automation_update", parameters: unsafeAutomationParameters }, + { type: "function", name: "safe_union", parameters: safeUnionParameters }, + { type: "function", name: "plain", parameters: {} }, + ], + }; + const adapter = createResponsesPassthroughAdapter({ + adapter: "openai-responses", + baseUrl: XAI_GROK_CLI_BASE_URL, + authMode: "oauth", + apiKey: "xai-test", + }); + const build = (lite: boolean) => JSON.parse(adapter.buildRequest({ + modelId: "grok-4.6", + context: { messages: [] }, + stream: true, + options: {}, + _rawBody: { + model: "grok-4.6", + input: lite ? [{ type: "additional_tools", tools: [namespace] }] : [], + ...(lite ? {} : { tools: [namespace] }), + }, + }, { headers: new Headers() }).body) as { + tools?: Array<{ name?: string; parameters?: Record }>; + input: Array<{ type: string; tools?: Array<{ name?: string; parameters?: Record }> }>; + }; + + for (const lite of [false, true]) { + const body = build(lite); + const tools = lite ? body.input[0]?.tools : body.tools; + expect(tools?.map(tool => tool.name)).toEqual([ + "mcp__codex_app__safe_union", + "mcp__codex_app__plain", + ]); + const safe = tools?.find(tool => tool.name === "mcp__codex_app__safe_union")?.parameters; + expect(safe).toEqual({ + type: "object", + properties: { + token: { type: "string" }, + mode: { anyOf: [{ const: "view" }, { const: "delete" }] }, + }, + required: ["token"], + }); + expect(tools?.find(tool => tool.name === "mcp__codex_app__plain")?.parameters) + .toEqual({ type: "object" }); + } + }); + + test("keeps native root unions on public xAI Responses", () => { + const parameters = { + oneOf: [ + { type: "object", properties: { mode: { const: "view" } } }, + { oneOf: [{ type: "object", properties: {} }, { type: "object", properties: {} }] }, + ], + }; + const request = createResponsesPassthroughAdapter({ + adapter: "openai-responses", + baseUrl: "https://api.x.ai/v1", + authMode: "key", + apiKey: "xai-test", + }).buildRequest({ + modelId: "grok-4.6", + context: { messages: [] }, + stream: true, + options: {}, + _rawBody: { + model: "grok-4.6", + input: [], + tools: [{ type: "function", name: "automation_update", parameters }], + }, + }, { headers: new Headers() }); + const body = JSON.parse(request.body) as { + tools: Array<{ parameters: Record }>; + }; + + expect(body.tools[0]?.parameters).toEqual({ ...parameters, type: "object" }); + }); + test("model reasoning-summary opt-out strips unsupported delivery fields (#323)", () => { const adapter = createResponsesPassthroughAdapter({ adapter: "openai-responses", From e2ca660272dbba9fd4f0d98d9cb30fc14abc301a Mon Sep 17 00:00:00 2001 From: olddonkey Date: Wed, 26 Aug 2026 13:26:32 -0700 Subject: [PATCH 2/5] fix(xai): preserve oneOf exclusivity and bound schema flattening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the Responses root-schema normalizer. A root `oneOf` rejects an instance matching more than one branch. Flattening the differing property into `anyOf` dropped that: branches like `{type: "string"}` and `{const: "view"}` overlap, so the merged schema accepted `"view"` where the original rejected it. Only the destination's ROOT rejects a union, so exclusivity now survives by moving the union DOWN onto that one property rather than widening it — a property-level `oneOf` accepts exactly what the root `oneOf` accepted once every other property, the required set, and additionalProperties already match. Provably disjoint branches keep emitting `anyOf`, where the two keywords describe the same set and the keyword is already proven on this wire. An optional discriminator was the second half of the same hole: absent, it matched every branch, which the root `oneOf` rejects and a per-property union would accept. That property is promoted into `required`, which is exactly equivalent. A `oneOf` nested among other unions binds exclusivity to its own branch group, which a flat variant list cannot express in either direction, so those omit the tool instead — promoting a discriminator there would have NARROWED the schema. Expansion was unbounded. Nested binary unions are 2^n variants and a `$ref` diamond amplifies node count the same way without ever cycling, so a deep MCP schema could exhaust memory before the tool was ever judged unflattenable. Depth, node, and variant budgets bound the walk; exceeding one omits that single function, which is the fallback an unflattenable schema already takes. A 2^40 union now resolves in under a millisecond. Omitting a function left `tool_choice` dangling. Namespace lowering rewrites the declarations and the selector together, so a selector could still name a tool this proxy had just dropped — reaching Grok as a reference it rejects. Relaxing it to `auto` would be worse, quietly running the turn without the tool the caller required. An `allowed_tools` list now drops the omitted entries while any remain, and a selection with nothing left to point at fails locally with the same 400 a tool catalog this proxy cannot lower already returns. Dropped tools are also named in a provider diagnostic, since the only other trace was a turn that never called. Duplicate branches stay collapsed rather than omitted. A `oneOf` listing the same branch twice strictly accepts nothing, which no author intends and no root object schema can express, so the tool stays usable instead of vanishing over a source-schema bug. Co-Authored-By: Claude Opus 5 --- src/adapters/openai-responses.ts | 64 +++++- src/adapters/xai-tool-schema.ts | 242 +++++++++++++++++---- src/server/responses/core.ts | 5 +- structure/04_transports-and-sidecars.md | 15 ++ tests/openai-responses-passthrough.test.ts | 66 +++++- tests/xai-tool-schema.test.ts | 183 +++++++++++++++- tests/xai-transport.test.ts | 4 +- 7 files changed, 529 insertions(+), 50 deletions(-) diff --git a/src/adapters/openai-responses.ts b/src/adapters/openai-responses.ts index 6ac5176c97..d69d2909b1 100644 --- a/src/adapters/openai-responses.ts +++ b/src/adapters/openai-responses.ts @@ -6,6 +6,7 @@ import { COMPACT_PROMPT, compactionItemToText, decodeCompactionSummary, isCompac import { collectResponsesToolGroups } from "../responses/tool-groups"; import { isHostedToolUnsupportedForModel } from "../responses/hosted-tool-policy"; import { decodeServerSentEvents } from "../lib/sse-decoder"; +import { debugProviderDiagnostic } from "../lib/debug"; import { CODEX_FORWARD_BASE_URL, destinationDecodesNativeCompactionBlob, @@ -20,7 +21,11 @@ import { rewriteRoutedToolSearchForUpstream } from "../responses/tool-search-com import { rewriteRoutedNamespaceToolsForUpstream } from "../responses/namespace-tool-compat"; import { openaiResponsesUrl } from "./openai-responses-url"; import { normalizeXaiResponsesWebSearch } from "./xai-web-search"; -import { isXaiSchemaTarget, normalizeXaiToolParameters } from "./xai-tool-schema"; +import { + isXaiSchemaTarget, + normalizeXaiToolParameters, + XaiToolSchemaCompatibilityError, +} from "./xai-tool-schema"; import { createAdapterTierMetadata, } from "../providers/fastwire"; @@ -545,9 +550,56 @@ function normalizeFunctionToolSchema(tool: unknown, xaiTarget: boolean): unknown }; } +/** + * Re-point `tool_choice` after an incompatible function was dropped from the catalog. Names here + * are already wire names, because namespace lowering rewrote the declarations and the selector + * together before this runs. A selector left naming an omitted tool reaches Grok as a dangling + * reference it rejects, and silently relaxing it to `auto` is worse: the turn would quietly + * proceed without the tool the caller required. So an `allowed_tools` list drops the omitted + * entries while any remain, and a selection with nothing left to point at fails locally with the + * same 400 the caller gets for a tool catalog this proxy cannot lower. + */ +function reconcileToolChoiceForOmittedTools( + body: Record, + omittedFunctionNames: ReadonlySet, +): Record { + if (omittedFunctionNames.size === 0) return body; + const toolChoice = body.tool_choice; + if (!isPlainObject(toolChoice)) return body; + + const refuse = (name: string): never => { + throw new XaiToolSchemaCompatibilityError( + `tool_choice requires function "${name}", but its parameter schema cannot be represented for this destination; ` + + "relax tool_choice or simplify the tool's parameter schema", + ); + }; + + if (toolChoice.type === "function" && typeof toolChoice.name === "string") { + return omittedFunctionNames.has(toolChoice.name) ? refuse(toolChoice.name) : body; + } + + if (toolChoice.type === "allowed_tools" && Array.isArray(toolChoice.tools)) { + const omitted = toolChoice.tools.filter(tool => + isPlainObject(tool) + && tool.type === "function" + && typeof tool.name === "string" + && omittedFunctionNames.has(tool.name)); + if (omitted.length === 0) return body; + const kept = toolChoice.tools.filter(tool => !omitted.includes(tool)); + if (kept.length === 0) { + const first = omitted[0]; + return refuse(isPlainObject(first) && typeof first.name === "string" ? first.name : "unknown"); + } + return { ...body, tool_choice: { ...toolChoice, tools: kept } }; + } + + return body; +} + function normalizeToolSchemas(body: unknown, xaiTarget: boolean): unknown { if (!isPlainObject(body)) return body; + const omittedFunctionNames = new Set(); const normalizeTools = (tools: unknown[]): unknown[] => { let changed = false; const normalized: unknown[] = []; @@ -555,6 +607,7 @@ function normalizeToolSchemas(body: unknown, xaiTarget: boolean): unknown { const fixed = normalizeFunctionToolSchema(tool, xaiTarget); if (fixed === undefined) { changed = true; + if (isPlainObject(tool) && typeof tool.name === "string") omittedFunctionNames.add(tool.name); continue; } if (fixed !== tool) changed = true; @@ -579,7 +632,14 @@ function normalizeToolSchemas(body: unknown, xaiTarget: boolean): unknown { }); if (inputChanged) normalizedBody = { ...normalizedBody, input }; } - return normalizedBody; + if (omittedFunctionNames.size > 0) { + // A dropped tool is a capability the caller declared and will not get, and the only other + // trace of it is a turn that never makes the call. Name them so the cause is recoverable. + debugProviderDiagnostic("openai-responses", "tool-schema-omitted", { + omitted: [...omittedFunctionNames], + }); + } + return reconcileToolChoiceForOmittedTools(normalizedBody, omittedFunctionNames); } function activateDeferredTool(tool: Record): Record { diff --git a/src/adapters/xai-tool-schema.ts b/src/adapters/xai-tool-schema.ts index 544f72cae5..86197f92e3 100644 --- a/src/adapters/xai-tool-schema.ts +++ b/src/adapters/xai-tool-schema.ts @@ -14,6 +14,9 @@ export function isXaiSchemaTarget(provider: Pick): } } +/** A tool this proxy had to omit is still named by `tool_choice`; the caller maps this to a 400. */ +export class XaiToolSchemaCompatibilityError extends Error {} + function stringRequiredFields(value: unknown): string[] { return Array.isArray(value) ? value.filter((item): item is string => typeof item === "string") @@ -33,10 +36,33 @@ const XAI_VARIANT_MERGE_KEYS = new Set([ "definitions", ]); +/** + * A root union expands combinatorially: n nested binary unions yield 2^n variants, and a + * diamond of `$ref`s amplifies node count the same way without ever cycling. Either one can + * exhaust memory before the schema is judged unflattenable, so depth, node, and variant + * budgets bound the walk. Exceeding any of them omits that one function — the same fallback + * an unflattenable schema already takes. Mirrors the node budgets in openai-chat.ts. + */ +const XAI_MAX_SCHEMA_DEPTH = 64; +const XAI_MAX_SCHEMA_NODES = 4_096; +const XAI_MAX_ROOT_VARIANTS = 256; + +/** Mutable walk budget, shared across ref resolution and root-union expansion for one tool. */ +interface XaiSchemaBudget { + remainingNodes: number; + remainingVariants: number; +} + +/** One budget per tool: a large catalog must not let one schema spend another's allowance. */ +function createXaiSchemaBudget(): XaiSchemaBudget { + return { remainingNodes: XAI_MAX_SCHEMA_NODES, remainingVariants: XAI_MAX_ROOT_VARIANTS }; +} + function decodeJsonPointerToken(token: string): string { return token.replace(/~1/g, "/").replace(/~0/g, "~"); } +/** Resolve a local `#/`-rooted JSON Pointer against `root`; undefined when it does not resolve. */ export function lookupLocalJsonPointer(root: unknown, ref: string): unknown { if (ref === "#" || ref === "#/") return root; if (!ref.startsWith("#/")) return undefined; @@ -48,26 +74,30 @@ export function lookupLocalJsonPointer(root: unknown, ref: string): unknown { return current; } -/** Resolve local `#/` `$ref`s. Unresolvable or cyclic refs return undefined. */ +/** Resolve local `#/` `$ref`s. Unresolvable, cyclic, or over-budget refs return undefined. */ function resolveXaiSchemaRefs( schema: unknown, root: Record, + budget: XaiSchemaBudget, stack: Set = new Set(), + depth = 0, ): unknown | undefined { if (!isSchemaObject(schema)) return schema; + if (depth >= XAI_MAX_SCHEMA_DEPTH || budget.remainingNodes <= 0) return undefined; + budget.remainingNodes -= 1; if (typeof schema.$ref === "string") { const ref = schema.$ref; if (stack.has(ref)) return undefined; const target = lookupLocalJsonPointer(root, ref); if (target === undefined) return undefined; stack.add(ref); - const resolvedTarget = resolveXaiSchemaRefs(target, root, stack); + const resolvedTarget = resolveXaiSchemaRefs(target, root, budget, stack, depth + 1); stack.delete(ref); if (resolvedTarget === undefined) return undefined; const rest: Record = { ...schema }; delete rest.$ref; if (Object.keys(rest).length === 0) return resolvedTarget; - const resolvedRest = resolveXaiSchemaRefs(rest, root, stack); + const resolvedRest = resolveXaiSchemaRefs(rest, root, budget, stack, depth + 1); if (resolvedRest === undefined || !isSchemaObject(resolvedTarget) || !isSchemaObject(resolvedRest)) { return undefined; } @@ -79,7 +109,7 @@ function resolveXaiSchemaRefs( if ((key === "oneOf" || key === "anyOf") && Array.isArray(value)) { const items: unknown[] = []; for (const item of value) { - const next = resolveXaiSchemaRefs(item, root, stack); + const next = resolveXaiSchemaRefs(item, root, budget, stack, depth + 1); if (next === undefined) return undefined; items.push(next); } @@ -89,7 +119,7 @@ function resolveXaiSchemaRefs( if (key === "properties" && isSchemaObject(value)) { const properties: Record = {}; for (const [name, property] of Object.entries(value)) { - const next = resolveXaiSchemaRefs(property, root, stack); + const next = resolveXaiSchemaRefs(property, root, budget, stack, depth + 1); if (next === undefined) return undefined; properties[name] = next; } @@ -101,6 +131,7 @@ function resolveXaiSchemaRefs( return resolved; } +/** A variant is mergeable only when it is an object whose every key the merger preserves. */ function xaiVariantIsConcreteObject(variant: Record): boolean { if (variant.type !== undefined && variant.type !== "object") return false; return Object.keys(variant).every(key => XAI_VARIANT_MERGE_KEYS.has(key)); @@ -137,6 +168,86 @@ function xaiRequiredSetsMatch(variants: Record[]): boolean { return serialized.every(value => value === serialized[0]); } +/** Values a schema pins through `const`/`enum`, or undefined when it pins none. */ +function xaiLiteralValues(schema: unknown): unknown[] | undefined { + if (!isSchemaObject(schema)) return undefined; + if (Object.hasOwn(schema, "const")) return [schema.const]; + if (Array.isArray(schema.enum)) return schema.enum; + return undefined; +} + +/** JSON type name for a literal, so it can be compared against a `type` keyword. */ +function xaiJsonTypeOf(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + if (typeof value === "string") return "string"; + if (typeof value === "boolean") return "boolean"; + if (typeof value === "number") return Number.isInteger(value) ? "integer" : "number"; + return "object"; +} + +/** Types a schema declares, or undefined when it constrains none. */ +function xaiDeclaredTypes(schema: unknown): Set | undefined { + if (!isSchemaObject(schema)) return undefined; + const type = schema.type; + if (typeof type === "string") return new Set([type]); + if (Array.isArray(type) && type.every(item => typeof item === "string")) return new Set(type as string[]); + return undefined; +} + +/** `integer` is a subset of `number`, so those two names overlap rather than exclude. */ +function xaiTypesOverlap(left: string, right: string): boolean { + if (left === right) return true; + return (left === "integer" && right === "number") || (left === "number" && right === "integer"); +} + +/** + * Conservative mutual-exclusion test: true only when no instance can satisfy both schemas. + * Proof comes from disjoint literal sets or disjoint declared types; anything it cannot prove + * is reported as overlapping so the caller refuses the merge instead of widening the schema. + */ +function xaiSchemasAreProvablyDisjoint(left: unknown, right: unknown): boolean { + const leftValues = xaiLiteralValues(left); + const rightValues = xaiLiteralValues(right); + if (leftValues && rightValues) { + const seen = new Set(rightValues.map(value => JSON.stringify(value))); + return leftValues.every(value => !seen.has(JSON.stringify(value))); + } + const leftTypes = xaiDeclaredTypes(left); + const rightTypes = xaiDeclaredTypes(right); + const literalsExcludedByTypes = (values: unknown[], types: Set): boolean => + values.every(value => ![...types].some(type => xaiTypesOverlap(xaiJsonTypeOf(value), type))); + if (leftValues && rightTypes) return literalsExcludedByTypes(leftValues, rightTypes); + if (rightValues && leftTypes) return literalsExcludedByTypes(rightValues, leftTypes); + if (leftTypes && rightTypes) { + return ![...leftTypes].some(leftType => [...rightTypes].some(rightType => xaiTypesOverlap(leftType, rightType))); + } + return false; +} + +/** Every pair provably disjoint, so a union over them accepts each instance exactly once. */ +function xaiSchemasArePairwiseDisjoint(schemas: unknown[]): boolean { + for (let i = 0; i < schemas.length; i += 1) { + for (let j = i + 1; j < schemas.length; j += 1) { + if (!xaiSchemasAreProvablyDisjoint(schemas[i], schemas[j])) return false; + } + } + return true; +} + +/** Deduplicate schemas by serialized shape, preserving first-seen order. */ +function uniqueXaiSchemas(values: unknown[]): unknown[] { + const unique: unknown[] = []; + const serialized = new Set(); + for (const value of values) { + const key = JSON.stringify(value); + if (serialized.has(key)) continue; + serialized.add(key); + unique.push(value); + } + return unique; +} + function mergeXaiAdditionalProperties( variants: Record[], ): { ok: true; value?: unknown } | { ok: false } { @@ -148,14 +259,7 @@ function mergeXaiAdditionalProperties( const permissive = explicit.filter(value => value !== false); if (hasFalse && permissive.length > 0) return { ok: false }; if (hasFalse) return { ok: true, value: false }; - const unique: unknown[] = []; - const seen = new Set(); - for (const value of permissive) { - const key = JSON.stringify(value); - if (seen.has(key)) continue; - seen.add(key); - unique.push(value); - } + const unique = uniqueXaiSchemas(permissive); if (unique.length !== 1) return { ok: false }; return { ok: true, value: unique[0] }; } @@ -187,55 +291,88 @@ function composeXaiObjectSchemas( return composed; } -function expandXaiRootObjectSchemas(schema: unknown): Record[] | undefined { - if (!isSchemaObject(schema)) return undefined; +/** Flattened root variants, plus the shape of the union tree they came from. */ +interface XaiRootExpansion { + variants: Record[]; + /** This node was itself a union, as opposed to a plain object leaf. */ + isUnion: boolean; + /** + * True when a `oneOf` was expanded anywhere in the tree. `oneOf` rejects an instance that + * matches more than one branch, so its variants must stay mutually exclusive after merging; + * `anyOf` carries no such obligation. + */ + exclusive: boolean; + /** + * True when a branch was itself a union. Nested `anyOf` flattens associatively and stays + * exact, but a `oneOf` mixed into a nest no longer maps onto one flat variant list: its + * exclusivity binds only its own branch group, which a merged root cannot express. + */ + nestedUnion: boolean; +} + +function expandXaiRootObjectSchemas( + schema: unknown, + budget: XaiSchemaBudget, + depth = 0, +): XaiRootExpansion | undefined { + if (!isSchemaObject(schema) || depth >= XAI_MAX_SCHEMA_DEPTH) return undefined; const compositionKey = ["oneOf", "anyOf"].find(key => Array.isArray(schema[key])); if (!compositionKey) { if (schema.type !== undefined && schema.type !== "object") return undefined; - return [{ ...schema, type: "object" }]; + if (budget.remainingVariants <= 0) return undefined; + budget.remainingVariants -= 1; + return { variants: [{ ...schema, type: "object" }], isUnion: false, exclusive: false, nestedUnion: false }; } const siblings = Object.fromEntries(Object.entries(schema).filter(([key]) => key !== compositionKey)); const branches = schema[compositionKey]; if (!Array.isArray(branches)) return undefined; const expanded: Record[] = []; + let exclusive = compositionKey === "oneOf"; + let nestedUnion = false; for (const branch of branches) { - const variants = expandXaiRootObjectSchemas(branch); - if (!variants) return undefined; - for (const variant of variants) expanded.push(composeXaiObjectSchemas(siblings, variant)); + const nested = expandXaiRootObjectSchemas(branch, budget, depth + 1); + if (!nested) return undefined; + exclusive ||= nested.exclusive; + nestedUnion ||= nested.isUnion || nested.nestedUnion; + for (const variant of nested.variants) expanded.push(composeXaiObjectSchemas(siblings, variant)); } - return expanded.length > 0 ? expanded : undefined; -} - -function mergeXaiPropertySchemas(values: unknown[]): unknown { - const unique: unknown[] = []; - const serialized = new Set(); - for (const value of values) { - const key = JSON.stringify(value); - if (serialized.has(key)) continue; - serialized.add(key); - unique.push(value); - } - return unique.length === 1 ? unique[0] : { anyOf: unique }; + return expanded.length > 0 ? { variants: expanded, isUnion: true, exclusive, nestedUnion } : undefined; } /** * The Grok CLI proxy rejects a function parameter schema whose root remains oneOf/anyOf. - * Flatten only when the merge is lossless: local $refs resolve, every variant is a concrete - * object whose keys we can preserve, required sets match, additionalProperties does not change - * meaning, every property name exists on every variant, and at most one property schema - * differs. Otherwise omit the tool rather than emit a weaker schema. + * Flatten only when the merge is lossless: local $refs resolve inside the walk budget, every + * variant is a concrete object whose keys we can preserve, required sets match, + * additionalProperties does not change meaning, every property name exists on every variant, + * and at most one property schema differs. + * + * A `oneOf` carries one more obligation than `anyOf`: it rejects an instance matching several + * branches. Only the destination's ROOT rejects a union, so exclusivity survives by moving the + * union down onto the one differing property rather than by widening it — with every other + * property, the required set, and additionalProperties already identical across branches, a + * property-level `oneOf` accepts exactly what the root `oneOf` accepted. Branches that are + * provably disjoint keep emitting `anyOf`, since there the two keywords describe the same set. + * The remaining hole is an optional discriminator: absent, it matches every branch, which the + * root `oneOf` rejects and a per-property union would accept, so that property is promoted into + * `required`. + * + * Duplicate branches are the one deliberate widening. A `oneOf` listing the same branch twice + * accepts nothing at all, which no author intends and no root-object schema can express, so the + * duplicates collapse and the tool stays usable instead of disappearing over a source-schema bug. */ export function normalizeXaiToolParameters(parameters: unknown): Record | undefined { if (!isSchemaObject(parameters)) return undefined; - const resolved = resolveXaiSchemaRefs(parameters, parameters); + const budget = createXaiSchemaBudget(); + const resolved = resolveXaiSchemaRefs(parameters, parameters, budget); if (!isSchemaObject(resolved)) return undefined; const normalizedRoot = { ...resolved }; delete normalizedRoot.$schema; - const variants = expandXaiRootObjectSchemas(normalizedRoot); - if (!variants) return undefined; + const expansion = expandXaiRootObjectSchemas(normalizedRoot, budget); + if (!expansion) return undefined; + const { variants, exclusive, nestedUnion } = expansion; if (variants.length === 1) { return xaiVariantIsConcreteObject(variants[0]) ? variants[0] : undefined; } @@ -259,10 +396,29 @@ export function normalizeXaiToolParameters(parameters: unknown): Record [name, mergeXaiPropertySchemas(values)]), - ); - const required = stringRequiredFields(variants[0]?.required); + const properties: Record = {}; + const differingNames: string[] = []; + for (const [name, values] of propertyValues) { + const unique = uniqueXaiSchemas(values); + if (unique.length === 1) { + properties[name] = unique[0]; + continue; + } + // A `oneOf` nested among other unions binds exclusivity to its own branch group only, so a + // single flat variant list cannot say what the original said — in either direction. Refuse. + if (exclusive && nestedUnion) return undefined; + differingNames.push(name); + // Disjoint branches make `anyOf` and `oneOf` describe the same set, so prefer the keyword + // already proven on this wire; overlapping branches need the exclusivity kept verbatim. + properties[name] = exclusive && !xaiSchemasArePairwiseDisjoint(unique) + ? { oneOf: unique } + : { anyOf: unique }; + } + + let required = stringRequiredFields(variants[0]?.required); + if (exclusive && differingNames.length > 0) { + required = [...new Set([...required, ...differingNames])]; + } return { ...metadata, diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 08a679894d..606123ae52 100644 --- a/src/server/responses/core.ts +++ b/src/server/responses/core.ts @@ -30,6 +30,7 @@ import { import { awaitThoughtSignatureDurability, thoughtSignatureReplaySalt } from "../../responses/thought-signature-replay"; import { buildCompactV1Output, COMPACT_PROMPT, decodeCompactionSummary, extractCompactUserMessages } from "../../responses/compaction"; import { FORWARD_HEADERS, sanitizeReasoningInputContent } from "../../adapters/openai-responses"; +import { XaiToolSchemaCompatibilityError } from "../../adapters/xai-tool-schema"; import { copyPreviousResponseReplayProvenance, expandPreviousResponseInput, @@ -3165,7 +3166,9 @@ async function handleResponsesInner( // the rotation-rebuild and bridged paths already answer 400 for the identical throw. Rethrowing // it here escaped every catch up to the Bun handler, so the same request produced an // unstructured 500 — and no request log — depending only on whether a rotation ran first. - if (error instanceof NamespaceToolCollisionError) { + // Same shape for a tool_choice this proxy cannot honor: the destination rejects a schema the + // catalog had to drop, so the selector naming it is a client input error, not a 500. + if (error instanceof NamespaceToolCollisionError || error instanceof XaiToolSchemaCompatibilityError) { return formatErrorResponse(400, "invalid_request_error", redactSecretString(error.message)); } throw error; diff --git a/structure/04_transports-and-sidecars.md b/structure/04_transports-and-sidecars.md index 8c1fa4822d..1fc289df60 100644 --- a/structure/04_transports-and-sidecars.md +++ b/structure/04_transports-and-sidecars.md @@ -131,6 +131,21 @@ letting one incompatible declaration reject the entire request before inference. `cli-chat-proxy.grok.com`: public `api.x.ai` keeps native root unions, as do unrelated Responses gateways. Both top-level `tools` and Responses Lite `additional_tools` pass through this policy. +Only the ROOT rejects a union, so exclusivity is preserved by moving it down rather than widening +it: a root `oneOf` whose branches differ in one property becomes that property's `oneOf`, or its +`anyOf` when the branches are provably disjoint and the two keywords describe the same set. That +property is also promoted into `required`, because absent it matched every branch — which the root +`oneOf` rejects. Branches that are wholly identical validate nothing and have no faithful +flattening, so they omit the tool. The walk carries depth, node, and variant budgets, since nested +unions are combinatorial and a `$ref` diamond amplifies the same way without ever cycling; +exceeding a budget omits that one function rather than expanding until memory is gone. + +Omitting a function makes `tool_choice` the loose end. A selector naming a dropped tool would reach +Grok as a dangling reference, and relaxing it to `auto` is worse — the turn would quietly run +without the tool the caller required. So an `allowed_tools` list drops the omitted entries while any +remain, and a selection with nothing left to point at fails locally with the same 400 a tool catalog +this proxy cannot lower already returns. + The same noncanonical boundary strips ChatGPT's private `external_web_access` bit from routed `web_search` declarations. The public tool remains enabled and all other options remain intact; canonical OpenAI forwarding preserves the bit. xAI's public Responses schema enables browsing by diff --git a/tests/openai-responses-passthrough.test.ts b/tests/openai-responses-passthrough.test.ts index 7c1df08d9b..a7ef5e57f2 100644 --- a/tests/openai-responses-passthrough.test.ts +++ b/tests/openai-responses-passthrough.test.ts @@ -950,15 +950,79 @@ describe("OpenAI Responses passthrough sanitization", () => { type: "object", properties: { token: { type: "string" }, + // Disjoint consts, so `anyOf` describes the same set the root `oneOf` did. `mode` is + // promoted into `required`: absent, it matched BOTH branches, which `oneOf` rejects. mode: { anyOf: [{ const: "view" }, { const: "delete" }] }, }, - required: ["token"], + required: ["token", "mode"], }); expect(tools?.find(tool => tool.name === "mcp__codex_app__plain")?.parameters) .toEqual({ type: "object" }); } }); + test("reconciles tool_choice against tools the xAI CLI schema policy omitted", () => { + // `oneOf` branches that disagree on which property names exist cannot be flattened, so this + // function is dropped. Namespace lowering already rewrote both the declaration and the + // selector to wire names, so the selector is left pointing at a tool that no longer ships. + const unsafe = { + oneOf: [ + { type: "object", properties: { mode: { const: "view" } }, required: ["mode"] }, + { type: "object", properties: { id: { type: "string" } }, required: ["id"] }, + ], + }; + const namespace = { + type: "namespace", + name: "mcp__codex_app", + tools: [ + { type: "function", name: "automation_update", parameters: unsafe }, + { type: "function", name: "plain", parameters: {} }, + ], + }; + const adapter = createResponsesPassthroughAdapter({ + adapter: "openai-responses", + baseUrl: XAI_GROK_CLI_BASE_URL, + authMode: "oauth", + apiKey: "xai-test", + }); + const build = (toolChoice: unknown) => adapter.buildRequest({ + modelId: "grok-4.6", + context: { messages: [] }, + stream: true, + options: {}, + _rawBody: { model: "grok-4.6", input: [], tools: [namespace], tool_choice: toolChoice }, + }, { headers: new Headers() }); + + // A forced selection has no safe replacement: relaxing it to auto would quietly run the turn + // without the tool the caller required, so this fails locally instead of reaching Grok. + expect(() => build({ type: "function", name: "mcp__codex_app__automation_update" })) + .toThrow(/tool_choice requires function "mcp__codex_app__automation_update"/); + + // An allowed_tools list still has a usable entry, so it simply loses the omitted one. + const narrowed = JSON.parse(build({ + type: "allowed_tools", + mode: "auto", + tools: [ + { type: "function", name: "mcp__codex_app__automation_update" }, + { type: "function", name: "mcp__codex_app__plain" }, + ], + }).body) as { tool_choice: { tools: Array<{ name: string }> } }; + expect(narrowed.tool_choice.tools).toEqual([{ type: "function", name: "mcp__codex_app__plain" }]); + + // Nothing left to point at is the forced case again. + expect(() => build({ + type: "allowed_tools", + mode: "auto", + tools: [{ type: "function", name: "mcp__codex_app__automation_update" }], + })).toThrow(/tool_choice requires function/); + + // A selector naming a surviving tool is untouched. + const kept = JSON.parse(build({ type: "function", name: "mcp__codex_app__plain" }).body) as { + tool_choice: unknown; + }; + expect(kept.tool_choice).toEqual({ type: "function", name: "mcp__codex_app__plain" }); + }); + test("keeps native root unions on public xAI Responses", () => { const parameters = { oneOf: [ diff --git a/tests/xai-tool-schema.test.ts b/tests/xai-tool-schema.test.ts index 26bf63e56e..2a660475d4 100644 --- a/tests/xai-tool-schema.test.ts +++ b/tests/xai-tool-schema.test.ts @@ -141,7 +141,9 @@ describe("xAI Grok CLI tool schema normalization", () => { type: "object", properties: { command: { - anyOf: [ + // The branches overlap, so `oneOf` exclusivity is load-bearing and is kept verbatim on + // the property. Only the ROOT union is what this destination rejects. + oneOf: [ { type: "string" }, { type: "string", minLength: 1 }, ], @@ -151,4 +153,181 @@ describe("xAI Grok CLI tool schema normalization", () => { additionalProperties: false, }); }); -}); \ No newline at end of file + + test("keeps an overlapping root oneOf exclusive instead of widening it to anyOf", async () => { + // A root `oneOf` rejects an instance matching several branches. Flattening the differing + // property to `anyOf` would accept `{ mode: "view" }`, which matches BOTH branches here and + // the original therefore rejects. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + oneOf: [ + { type: "object", properties: { mode: { type: "string" } }, required: ["mode"] }, + { type: "object", properties: { mode: { const: "view" } }, required: ["mode"] }, + ], + }, + })); + const body = JSON.parse(request.body) as { + tools?: Array<{ function: { parameters: Record } }>; + }; + + expect(body.tools?.[0]?.function.parameters).toEqual({ + type: "object", + properties: { mode: { oneOf: [{ type: "string" }, { const: "view" }] } }, + required: ["mode"], + }); + }); + + test("promotes an optional discriminator into required when flattening a root oneOf", async () => { + // `mode` absent matches both branches, so the root `oneOf` rejects `{}`. A per-property union + // alone would accept it; requiring the discriminator restores the original's accepted set. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + type: "object", + oneOf: [ + { properties: { mode: { const: "view" } } }, + { properties: { mode: { const: "edit" } } }, + ], + }, + })); + const body = JSON.parse(request.body) as { + tools?: Array<{ function: { parameters: Record } }>; + }; + + expect(body.tools?.[0]?.function.parameters).toEqual({ + type: "object", + properties: { mode: { anyOf: [{ const: "view" }, { const: "edit" }] } }, + required: ["mode"], + }); + }); + + test("collapses a root oneOf whose branches are identical", async () => { + // Duplicated branches always match together, so the union strictly accepts nothing. No author + // means that and no root object schema can express it, so the duplicates collapse and the tool + // stays usable rather than vanishing over a source-schema bug. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + oneOf: [ + { type: "object", properties: { mode: { const: "view" } }, required: ["mode"] }, + { type: "object", properties: { mode: { const: "view" } }, required: ["mode"] }, + ], + }, + })); + const body = JSON.parse(request.body) as { + tools?: Array<{ function: { parameters: Record } }>; + }; + + expect(body.tools?.[0]?.function.parameters).toEqual({ + type: "object", + properties: { mode: { const: "view" } }, + required: ["mode"], + }); + }); + + test("omits a oneOf nested inside another union rather than guessing its meaning", async () => { + // `anyOf: [oneOf[V1, V2], V3]` accepts `{}` — it fails the inner `oneOf` but matches V3. A flat + // variant list cannot say that, and promoting the discriminator into `required` would NARROW + // the schema by rejecting it. Neither direction is faithful, so the tool is omitted. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + type: "object", + anyOf: [ + { + oneOf: [ + { properties: { mode: { const: "view" } } }, + { properties: { mode: { const: "edit" } } }, + ], + }, + { properties: { mode: { const: "list" } } }, + ], + }, + })); + const body = JSON.parse(request.body) as { tools?: unknown[] }; + + expect(body.tools).toBeUndefined(); + }); + + test("bounds a nested root union instead of expanding 2^n variants", async () => { + // 30 nested binary unions, every branch distinct, is 2^30 variants. Reaching the assertion at + // all is the point: an unbounded walk never returns from this. + let parameters: Record = { + type: "object", + properties: { leaf: { type: "string" } }, + required: ["leaf"], + }; + for (let depth = 0; depth < 30; depth += 1) { + parameters = { + oneOf: [ + { ...parameters, properties: { [`k${depth}`]: { const: `a${depth}` } } }, + { ...parameters, properties: { [`k${depth}`]: { const: `b${depth}` } } }, + ], + }; + } + + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters, + })); + const body = JSON.parse(request.body) as { tools?: unknown[] }; + + expect(body.tools).toBeUndefined(); + }); + + test("omits a root union wider than the variant budget but keeps one within it", async () => { + const flatUnion = (branches: number) => ({ + oneOf: Array.from({ length: branches }, (_, index) => ({ + type: "object", + properties: { mode: { const: `m${index}` } }, + required: ["mode"], + })), + }); + const toolsFor = async (branches: number) => { + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: flatUnion(branches), + })); + return (JSON.parse(request.body) as { tools?: unknown[] }).tools; + }; + + expect(await toolsFor(200)).toHaveLength(1); + expect(await toolsFor(300)).toBeUndefined(); + }); + + test("omits a tool whose $ref graph fans out past the node budget", async () => { + // Each level references the one below it twice. No cycle is ever formed, so the ref stack + // alone does not stop it; only the node budget does. + const defs: Record = { level0: { type: "string" } }; + for (let level = 1; level <= 32; level += 1) { + defs[`level${level}`] = { + type: "object", + properties: { + left: { $ref: `#/$defs/level${level - 1}` }, + right: { $ref: `#/$defs/level${level - 1}` }, + }, + }; + } + + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + type: "object", + $defs: defs, + properties: { root: { $ref: "#/$defs/level32" } }, + required: ["root"], + }, + })); + const body = JSON.parse(request.body) as { tools?: unknown[] }; + + expect(body.tools).toBeUndefined(); + }); +}); diff --git a/tests/xai-transport.test.ts b/tests/xai-transport.test.ts index 38cb29142f..f197ed690b 100644 --- a/tests/xai-transport.test.ts +++ b/tests/xai-transport.test.ts @@ -207,7 +207,9 @@ describe("xAI auth-mode transport selection", () => { token: { type: "string" }, mode: { anyOf: [{ const: "path" }, { const: "url" }] }, }); - expect(xaiParameters.required).toEqual(["token"]); + // `mode` absent matched BOTH branches, which the root `oneOf` rejects, so flattening has to + // require the discriminator to keep accepting exactly what the original accepted. + expect(xaiParameters.required).toEqual(["token", "mode"]); }); test("omits an xAI union whose branch required fields cannot be flattened", () => { From 669efd568e081ee774d898146d55b61f7c00c4c5 Mon Sep 17 00:00:00 2001 From: olddonkey Date: Wed, 26 Aug 2026 22:56:46 -0700 Subject: [PATCH 3/5] test(xai): pin the additionalProperties composition boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Hoisting branch properties beside a root `additionalProperties: false` does move them across an applicator boundary, so the restriction that could not see into `oneOf` now sees them. Checked with ajv rather than by argument: the source schema in that report validates NOTHING — not `{}`, not `{mode:"view"}`, not `{mode:"edit"}` — because the root forbids the very key its branches require. The `$ref` target/sibling shape behaves the same. So the widening is real but its floor is the empty set, and the emitted schema still carries `additionalProperties: false`: `{other: 1}` and `{mode: "other"}` stay refused. That is the same call already documented for duplicate branches — an unsatisfiable schema is a source bug no author intends, and omitting the tool serves nobody. Refusing every composition that carries an explicit `additionalProperties` would also drop the satisfiable shape, where the branch property IS declared on the root. ajv confirms the original and the emitted schema accept exactly the same instances there, so that one is lossless and worth keeping. Both are now pinned by tests, and the module docstring records the boundary and why the presence of `additionalProperties` alone is not grounds to refuse. Co-Authored-By: Claude Opus 5 --- src/adapters/xai-tool-schema.ts | 12 ++++-- tests/xai-tool-schema.test.ts | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/src/adapters/xai-tool-schema.ts b/src/adapters/xai-tool-schema.ts index 86197f92e3..b805d767ea 100644 --- a/src/adapters/xai-tool-schema.ts +++ b/src/adapters/xai-tool-schema.ts @@ -357,9 +357,15 @@ function expandXaiRootObjectSchemas( * root `oneOf` rejects and a per-property union would accept, so that property is promoted into * `required`. * - * Duplicate branches are the one deliberate widening. A `oneOf` listing the same branch twice - * accepts nothing at all, which no author intends and no root-object schema can express, so the - * duplicates collapse and the tool stays usable instead of disappearing over a source-schema bug. + * Two deliberate widenings remain, and both widen only from the EMPTY set. A `oneOf` listing the + * same branch twice accepts nothing at all. Neither does a root `additionalProperties: false` + * placed over branches that declare properties the root itself does not: the restriction cannot + * see into an applicator, so it forbids the very keys those branches require (same for a `$ref` + * target's restriction against a sibling's properties). No author intends either, no root object + * schema can express "accepts nothing", and the emitted schema still carries the restriction — so + * the tool stays usable instead of disappearing over a source-schema bug. A union whose branch + * properties ARE declared on the root is satisfiable and is normalized losslessly, which is why + * the presence of `additionalProperties` alone is not grounds to refuse. */ export function normalizeXaiToolParameters(parameters: unknown): Record | undefined { if (!isSchemaObject(parameters)) return undefined; diff --git a/tests/xai-tool-schema.test.ts b/tests/xai-tool-schema.test.ts index 2a660475d4..0937137b75 100644 --- a/tests/xai-tool-schema.test.ts +++ b/tests/xai-tool-schema.test.ts @@ -154,6 +154,75 @@ describe("xAI Grok CLI tool schema normalization", () => { }); }); + test("hoists branch properties past a root additionalProperties:false, and keeps the restriction", async () => { + // A root `additionalProperties: false` cannot see into `oneOf` branches, so the source schema + // below forbids the very `mode` its branches require: verified with ajv, it validates NOTHING + // — not `{}`, not `{mode:"view"}`. Flattening hoists `mode` beside the restriction, which is a + // widening in the strict reading but only from the empty set, and the emitted schema still + // carries `additionalProperties: false`, so `{other: 1}` and `{mode: "other"}` stay refused. + // Same call as the duplicate-branch case: an unsatisfiable schema is a source bug no author + // intends, and omitting the tool serves nobody. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + additionalProperties: false, + oneOf: [ + { properties: { mode: { const: "view" } }, required: ["mode"] }, + { properties: { mode: { const: "edit" } }, required: ["mode"] }, + ], + }, + })); + const body = JSON.parse(request.body) as { + tools?: Array<{ function: { parameters: Record } }>; + }; + + expect(body.tools?.[0]?.function.parameters).toEqual({ + type: "object", + properties: { mode: { anyOf: [{ const: "view" }, { const: "edit" }] } }, + required: ["mode"], + additionalProperties: false, + }); + }); + + test("a satisfiable additionalProperties:false union keeps its exact accepted set", async () => { + // The distinguishing case: `mode` is declared on the ROOT too, so the restriction never + // forbade it and the source schema really does accept `view`/`edit`. Verified with ajv, the + // original and the emitted schema accept exactly the same instances. Refusing every + // composition that carries an explicit `additionalProperties` would drop this one for nothing. + const request = await xaiAdapter().buildRequest(parsedRequest({ + name: "Bash", + description: "Execute a shell command", + parameters: { + type: "object", + additionalProperties: false, + properties: { mode: { type: "string" } }, + required: ["mode"], + oneOf: [ + { properties: { mode: { const: "view" } } }, + { properties: { mode: { const: "edit" } } }, + ], + }, + })); + const body = JSON.parse(request.body) as { + tools?: Array<{ function: { parameters: Record } }>; + }; + + expect(body.tools?.[0]?.function.parameters).toEqual({ + type: "object", + properties: { + mode: { + oneOf: [ + { allOf: [{ type: "string" }, { const: "view" }] }, + { allOf: [{ type: "string" }, { const: "edit" }] }, + ], + }, + }, + required: ["mode"], + additionalProperties: false, + }); + }); + test("keeps an overlapping root oneOf exclusive instead of widening it to anyOf", async () => { // A root `oneOf` rejects an instance matching several branches. Flattening the differing // property to `anyOf` would accept `{ mode: "view" }`, which matches BOTH branches here and From 6769db6e832872f8c69bb25e99d55df8f8c5a027 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 27 Aug 2026 14:49:40 +0900 Subject: [PATCH 4/5] docs(devlog): record the L2 lane outcome and audit Squash fidelity proven by identical diffs plus empty file-level diff against the PR head; the guard-laundering question answered structurally and measured; 13 hostile helper-name probes all refused. Also records a core.bare flip that made the main checkout report as bare mid-lane. --- .../031_wp4_l2_outcome.md | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 devlog/_plan/260827_bug_pr_merge_round/031_wp4_l2_outcome.md diff --git a/devlog/_plan/260827_bug_pr_merge_round/031_wp4_l2_outcome.md b/devlog/_plan/260827_bug_pr_merge_round/031_wp4_l2_outcome.md new file mode 100644 index 0000000000..2d6c533ab4 --- /dev/null +++ b/devlog/_plan/260827_bug_pr_merge_round/031_wp4_l2_outcome.md @@ -0,0 +1,83 @@ +# wp4 — L2 lane outcome + +#2663 landed on `dev` as `cebe005db` (PR #2724), squashed into `cb9bb9b76`. +The PR is CLOSED with a comment naming the sha. + +## Squash fidelity was proven, not assumed + +``` +git diff 58f5a294e 71e182ae6 -> pr.diff (1087 lines) +git diff cb9bb9b76~1 cb9bb9b76 -> squash.diff (1087 lines) IDENTICAL +git diff --stat 71e182ae6 cb9bb9b76 -- <12 files> -> empty +git diff --stat 58f5a294e 64c6d642b -- <12 files> -> empty +``` + +The last line is the one that mattered and would have been easy to skip: `dev` +moved 96 commits between the PR's merge base and the squash base, but touched none +of these 12 files. That is WHY a whole-take squash was safe here. Had any of those +files moved, the same procedure would have silently produced a different result. + +## The guard question, answered properly + +The reviewer's main line of attack was the right one: `rememberPassthroughResponseChecked` +now runs the undeclared-tool guard on a RESTORED response +(`src/server/responses/core.ts:3286-3299`), so restoration could in principle launder +an undeclared name into a declared one. + +It cannot, and the reason is structural rather than incidental: restoration is strictly +NARROWER than the guard's own normalization. Both call `normalizeDeclaredToolName` +(`src/types/tools.ts:47`), and `routedCustomToolTargetName` +(`src/responses/custom-tool-compat.ts:70`) additionally requires the normalized target +to be in the routed set. Any name restoration rewrites was already declared-equivalent +to the guard. + +Measured, guard verdict before vs after restoration: + +| item | before | after | resulting name | +|---|---|---|---| +| `other_tool` | refused | refused | `other_tool` | +| `rm` | refused | refused | `rm` | +| `apply_patch` (helper) | accepted | accepted | `exec` | +| `namespace:"evil"` + `apply_patch` | refused | refused | `apply_patch` | +| `apply_patch_evil` | refused | refused | `apply_patch_evil` | + +No row flips from refused to accepted. + +## Helper matching is exact membership, and that was tested adversarially + +13 hostile variants probed — `apply_patch_evil`, `evil__apply_patch`, `Apply_Patch`, +`apply_patch ` (trailing space), `apply-patch`, `tools.apply_patch`, `exec_commandX` +— all returned `target = undefined`. A substring predicate would have captured three +of them. Injection is closed the same way: `compileCodeModeHelperInput` serializes +every value with `JSON.stringify`, and hostile payloads (quote-escape, backtick/\${}, +U+2028/2029, patch-body breakout) each compiled to exactly one `await tools.` call +with zero escapes out of the string literal. + +## Test changes were repointed, not weakened + +Every removed `apply_patch` refusal assertion is replaced by an equivalent +`other_tool` refusal — still-undeclared, so refusal coverage is preserved while +`apply_patch` becomes a recognized helper. The suite also gains bridged-turn +continuation, streamed bridging, and escape-resistance coverage. + +Incidental find: `tests/bridge-legacy-shell-normalization.test.ts` fixes a latent bug +in its own helper (`delta:` -> `arguments:`). The old field was never read, so those +assertions were partly vacuous before. + +## Evidence + +- `bun run test` full suite: exit 0 +- reviewer's independent run over 7 relevant files: 189 pass / 0 fail +- `bun x tsc --noEmit`: clean on the merged tree +- PR #2724 CI: 23/23 green after one rerun of the `update-stop-first` launcher flake + (`git diff --name-only` vs dev shows NO_OVERLAP with any update/launcher file) +- VERDICT: PASS + +## Incident: `core.bare` flipped mid-lane + +After the merge, `git status` in the main checkout began failing with "this operation +must be run in a work tree" and `git worktree list` reported the repository as +`(bare)`. `git config --local --get core.bare` returned `true` while every tracked +file and `.git/index` were intact. Restored with +`git config --local core.bare false`; no data was lost and no commit was affected. +Cause not established — worth watching if it recurs while many worktrees are live. From 4c5adb2b2eba214260738a3ba34076740e5add65 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 27 Aug 2026 15:00:09 +0900 Subject: [PATCH 5/5] docs(devlog): record the L4 lane outcome 2694 closed NOOP (the landed 2663 bridge covers it), 2690 landed whole after the author rebased, 2693 left open on an upstream question, 2638 and 2497 reported as NEEDS_HUMAN for the MAINTAINERS.md security review their auth surface requires. --- .../041_wp5_l4_outcome.md | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 devlog/_plan/260827_bug_pr_merge_round/041_wp5_l4_outcome.md diff --git a/devlog/_plan/260827_bug_pr_merge_round/041_wp5_l4_outcome.md b/devlog/_plan/260827_bug_pr_merge_round/041_wp5_l4_outcome.md new file mode 100644 index 0000000000..75180189fe --- /dev/null +++ b/devlog/_plan/260827_bug_pr_merge_round/041_wp5_l4_outcome.md @@ -0,0 +1,101 @@ +# wp5 — L4 lane outcome + +## #2694 — NOOP, closed + +The landed #2663 (`cebe005db`) does provider-agnostically what #2694 hand-built for +one provider. Measured on current dev: + +``` +exec_command -> exec +shell_command -> exec +apply_patch -> exec +compiled: const result = await tools.exec_command({"cmd":"pwd"}); + text(result); +``` + +That is the same wrapper `codeModeExecCommandInput` produced. Closed with the five +`tsc` errors and the nonexistent `sensenova` provider id spelled out, so the author +knows why it could not have landed as written. + +## #2690 — landed whole, NOT reimplemented + +003 reclassified this L3 -> L4 on the reviewer's finding that the fix imports the +module the refactor creates, so "take the fix, leave the refactor" was incoherent. +The plan then said: rebase and land whole, or reimplement against the existing helper. + +The author (olddonkey) rebased it themselves onto post-#2684 dev, twice, ending at +`669efd568`. So the reimplement never became necessary — the branch merges clean and +the merged tree passes: + +``` +git merge-tree origin/dev pr2690-fresh -> exit 0 (no conflict) +bun x tsc --noEmit (merged tree) -> clean +bun test xai-tool-schema, xai-transport, + openai-responses-passthrough, + azure-model-router-tool-schema -> 165 pass / 0 fail +``` + +The Azure suite is the one that matters there: it is #2684's, in the region #2690 +conflicted with. Both now coexist. Merged as `3d986ef9c`. + +Lesson worth keeping: ordering #2684 first (005, correction 2) was what made this +cheap. Had #2690 landed first, #2684 would have been the one needing a rewrite. + +## #2693 — BLOCKED on an upstream fact, left open + +Test-only diff whose test fails on its own branch; +`skip_thought_signature_validator` exists nowhere in `src/`. The blocking question is +posted on the PR: does Gemini 3 on Antigravity actually honor that sentinel as a +functionCall `thoughtSignature`? Dev deliberately refuses to forward non-genuine +signatures, so implementing the fallback without that fact risks trading a clean 400 +for a silently degraded turn. Left OPEN rather than closed — it is a real question, +not a rejected patch. + +## #2638 and #2497 — NEEDS_HUMAN, security boundary + +Both are hygiene-blocked for the same reason, and it is the correct reason. + +``` +unsponsored_surface — This changes an authentication, workflow, release-automation, +or dependency surface. MAINTAINERS.md requires security review for these. +Paths: src/codex/auth-context.ts +``` + +### #2638 is in better shape than its label suggests + +Its auth surface is 14 lines in `src/codex/auth-context.ts`: hoisting +`nativeMainSelectionOnly` to a const, and widening ONE condition from +`nativeMainTrafficBlocked` to `nativeMainReadsForbidden` so a turn drain reports the +temporary fence instead of a permanent model-entitlement denial. No other auth, +credential, OAuth, token, workflow or release path is touched +(`git diff --name-only` over auth-ish patterns returns only that file and its test). + +Verified at the merged tree: + +``` +git merge (dev + pr2638) -> MERGE_OK +bun x tsc --noEmit -> clean +codex-auth-context, codex-routing, subagent-fallback-handle-responses, +core-lab-boundary -> 267 pass / 0 fail +src/server/index.ts, lifecycle.ts, router.ts -> NOT TOUCHED +``` + +That last line matters: `AGENTS.md` warns that an `await` added to the synchronous +activation chain silently reroutes subagents to a different model than the operator +configured. This PR does not touch those files, and `core-lab-boundary` passes. + +### #2497 is the heavier one + +2622/76 across 20 files, CONFLICTING on five including `src/server/responses/core.ts`, +and it touches `src/oauth/chatgpt.ts` (+88/-12) and `src/codex/main-account.ts` +(+551/-12) — OAuth refresh and credential storage. + +### Why this round stops here + +`MAINTAINERS.md` requires explicit security review for these surfaces, and the gate +asks for a maintainer to apply `maintainer-sponsored` after reviewing. An agent +applying that label to clear its own PR would defeat the check entirely — the label +IS the human judgment. So both are reported with evidence and left for the maintainer. + +This is `NEEDS_HUMAN`, not `BLOCKED`: nothing external is missing, a person's +decision is.