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. 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. 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..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,6 +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, + XaiToolSchemaCompatibilityError, +} from "./xai-tool-schema"; import { createAdapterTierMetadata, } from "../providers/fastwire"; @@ -531,8 +537,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 +550,69 @@ function normalizeFunctionToolSchema(tool: unknown): unknown { }; } -function normalizeToolSchemas(body: unknown): 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 = 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; + if (isPlainObject(tool) && typeof tool.name === "string") omittedFunctionNames.add(tool.name); + continue; + } if (fixed !== tool) changed = true; - return fixed; - }); + normalized.push(fixed); + } return changed ? normalized : tools; }; @@ -569,7 +632,14 @@ function normalizeToolSchemas(body: unknown): 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 { @@ -1998,15 +2068,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..b805d767ea --- /dev/null +++ b/src/adapters/xai-tool-schema.ts @@ -0,0 +1,436 @@ +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; + } +} + +/** 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") + : []; +} + +/** 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", +]); + +/** + * 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; + 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, 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, 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, budget, stack, depth + 1); + 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, budget, stack, depth + 1); + 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, budget, stack, depth + 1); + if (next === undefined) return undefined; + properties[name] = next; + } + resolved[key] = properties; + continue; + } + resolved[key] = value; + } + 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)); +} + +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]); +} + +/** 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 } { + 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 = uniqueXaiSchemas(permissive); + 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; +} + +/** 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; + 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 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 ? { 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 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`. + * + * 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; + const budget = createXaiSchemaBudget(); + const resolved = resolveXaiSchemaRefs(parameters, parameters, budget); + if (!isSchemaObject(resolved)) return undefined; + + const normalizedRoot = { ...resolved }; + delete normalizedRoot.$schema; + + 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; + } + 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: 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, + type: "object", + properties, + ...(required.length > 0 ? { required } : {}), + ...("value" in additionalProperties ? { additionalProperties: additionalProperties.value } : {}), + }; +} diff --git a/src/server/responses/core.ts b/src/server/responses/core.ts index 295415144e..01663e5b19 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 53f69dd7aa..1fc289df60 100644 --- a/structure/04_transports-and-sidecars.md +++ b/structure/04_transports-and-sidecars.md @@ -124,6 +124,28 @@ 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. + +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 98db0c2d8c..a7ef5e57f2 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,184 @@ 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" }, + // 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", "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: [ + { 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", diff --git a/tests/xai-tool-schema.test.ts b/tests/xai-tool-schema.test.ts index 26bf63e56e..0937137b75 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,250 @@ describe("xAI Grok CLI tool schema normalization", () => { additionalProperties: false, }); }); -}); \ No newline at end of file + + 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 + // 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", () => {