diff --git a/scripts/test-layout/layout.json b/scripts/test-layout/layout.json index 30bcc28b47..91eefb1ef7 100644 --- a/scripts/test-layout/layout.json +++ b/scripts/test-layout/layout.json @@ -286,6 +286,7 @@ "cancel-body-on-abort.test.ts": "server", "catalog-auto-refresh-scheduler.test.ts": "codex-integration", "catalog-cursor-search.test.ts": "codex-integration", + "catalog-duplicate-slug-dedup.test.ts": "codex-integration", "catalog-free-pricing-status.test.ts": "codex-integration", "catalog-full-picker-order.test.ts": "codex-integration", "catalog-gated-native-suppression-reason.test.ts": "codex-integration", @@ -293,13 +294,16 @@ "catalog-hub-context-window.test.ts": "codex-integration", "catalog-input-modality-enum.test.ts": "codex-integration", "catalog-llamacpp-capabilities.test.ts": "codex-integration", + "catalog-modelalias-unique-sync.test.ts": "codex-integration", "catalog-oauth-observation.test.ts": "codex-integration", "catalog-remote-pull.test.ts": "codex-integration", "catalog-retain-models.test.ts": "codex-integration", "catalog-seed-window-fill.test.ts": "codex-integration", + "catalog-slug-uniqueness-boundary.test.ts": "codex-integration", "catalog-verbosity-default.test.ts": "codex-integration", "catalog-vision-sidecar-modalities.test.ts": "codex-integration", "catalog-zero-credit-picker.test.ts": "codex-integration", + "chat-completions-deferred-tools.test.ts": "responses", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", "chat-inbound-reasoning-none.test.ts": "responses", diff --git a/src/bridge/response-json.ts b/src/bridge/response-json.ts index 6b2deec985..365f664264 100644 --- a/src/bridge/response-json.ts +++ b/src/bridge/response-json.ts @@ -68,6 +68,8 @@ function buildResponseJSONWithBudget( toolNsMap?: Map; /** Request-visible tool names. When present, an upstream call outside this set fails closed. */ declaredToolNames?: ReadonlySet; + /** See `bridgeToResponsesSSE`: enforcement is separate from normalization (#4735). */ + enforceDeclaredToolNames?: boolean; /** Declared parameter schema per tool name; repairs integral-float integer args (#1611). */ toolParameterSchemas?: ReadonlyMap>; freeformToolNames?: Set; @@ -432,7 +434,11 @@ function buildResponseJSONWithBudget( } flushToolCall(); const effectiveName = normalizeDeclaredToolName(e.name, options?.declaredToolNames); - if (options?.declaredToolNames && !options.declaredToolNames.has(effectiveName)) { + if ( + options?.declaredToolNames + && options.enforceDeclaredToolNames !== false + && !options.declaredToolNames.has(effectiveName) + ) { errorEvent = { type: "error", message: `routed provider emitted undeclared client tool "${effectiveName}"; only request-declared tools may be called`, diff --git a/src/bridge/sse.ts b/src/bridge/sse.ts index 43d4f0b9f7..db0adbdb7c 100644 --- a/src/bridge/sse.ts +++ b/src/bridge/sse.ts @@ -88,6 +88,20 @@ export function bridgeToResponsesSSE( onUsage?: (usage: OcxUsage | undefined) => void; /** Request-visible tool names. When present, an upstream call outside this set fails closed. */ declaredToolNames?: ReadonlySet; + /** + * Whether `declaredToolNames` is an authorization boundary this proxy enforces, or only the + * catalog used to normalize provider-invented names back to declared ones. + * + * Defaults to enforcing. The chat and Anthropic inbound wires set it false: those specs make + * the server relay a tool call and leave execution or refusal to the client's own runner, and + * harnesses on them legitimately defer part of their catalog (#4735). + * + * It is a separate flag rather than simply withholding `declaredToolNames`, because the set + * also drives `normalizeDeclaredToolName` and `declaresCodeModeExec`. Passing `undefined` + * turns those off too, so a provider that invents `default.lookup` for a declared `lookup` + * would reach the client under the invented name instead of the normalized one. + */ + enforceDeclaredToolNames?: boolean; /** Declared parameter schema per tool name; repairs integral-float integer args (#1611). */ toolParameterSchemas?: ReadonlyMap>; /** @@ -1008,7 +1022,11 @@ export function bridgeToResponsesSSE( : undefined; const mapped = toolNsMap?.get(effectiveName); const realName = mapped?.name ?? effectiveName; - if (options?.declaredToolNames && !options.declaredToolNames.has(effectiveName)) { + if ( + options?.declaredToolNames + && options.enforceDeclaredToolNames !== false + && !options.declaredToolNames.has(effectiveName) + ) { const failure = responseError( 502, "upstream_error", diff --git a/src/codex/catalog/aggregation.ts b/src/codex/catalog/aggregation.ts index f97b8295fc..c842fe3046 100644 --- a/src/codex/catalog/aggregation.ts +++ b/src/codex/catalog/aggregation.ts @@ -34,7 +34,7 @@ import upstreamModelsSnapshot from "../data/upstream-models.json"; import { catalogModelSlug } from "./parsing"; -import type { CatalogModel } from "./parsing"; +import type { CatalogModel, RawEntry } from "./parsing"; export const openAiApiCollisionWarnings = new Set(); @@ -222,6 +222,85 @@ export function safeCatalogWarningLabel(value: string): string { .slice(0, 200); } +/** + * Keep the first row of each slug and drop the rest (#4730). + * + * First-win is the only answer that agrees with the ordering already decided upstream: the merge + * ranks rows, so its first occurrence is the row it chose. Distinct slugs are never touched — an + * alias row and the canonical routed row of the same provider model are two different public names + * and both survive — and a row without a string slug passes through untouched. + */ +export function dedupeCatalogEntriesBySlug(models: RawEntry[]): RawEntry[] { + const seen = new Set(); + const out: RawEntry[] = []; + for (const entry of models) { + if (typeof entry.slug !== "string") { + out.push(entry); + continue; + } + if (seen.has(entry.slug)) continue; + seen.add(entry.slug); + out.push(entry); + } + return out; +} + +/** + * Every slug this proxy writes into the Codex catalog must appear exactly once (#4730). + * + * The cost of breaking it is the whole file: a slug-unique validating consumer refuses the catalog + * outright, so one duplicated row takes every model with it. A 2.56.0 report carried 507 rows for + * 72 unique slugs, every duplicate byte-identical, and Codex rejected the file as `source-invalid`. + * + * This is a write-boundary invariant rather than a repair of one producer, and that distinction is + * deliberate: the reported catalog is evidence that some emit path can double a row, but nothing in + * this tree has been shown to be that path, and a guard that only covered the producer someone + * guessed at would leave the file corruptible by the next one. Both writers that serialize a merged + * catalog call this as their LAST mutation — `writeRetainedCatalogSync` and the management + * convergence commit — so uniqueness holds for the exact bytes that land on disk. + * + * Ordering is load-bearing. Running the guard before the effort clamp would be unsound: + * `clampCatalogModelsToObservedCodexSupport` splices whole rows out when an exact-reserve ladder + * clamps empty, so dropping a later same-slug row first can leave the slug with no row at all once + * the surviving one is spliced. + * + * @param models - The finished row list, already clamped and finalized. + * @param warn - Whether to report on `console.warn`. The convergence path merges under + * `warningPolicy: "suppress"` and stays silent for the same reason. + * @returns The original array when it was already unique, so an unchanged catalog stays a no-op + * write; otherwise a first-win copy. + */ +export function enforceCatalogSlugUniqueness(models: RawEntry[], warn: boolean): RawEntry[] { + const deduped = dedupeCatalogEntriesBySlug(models); + if (deduped.length === models.length) return models; + if (warn) { + // A dropped row that differs from the kept one means two emit paths disagree about the same + // slug's content. First-win still stands, but the operator needs to see WHICH slugs diverged + // instead of silently losing data. The baseline is the row the dedupe actually keeps — the + // FIRST occurrence — so the reported divergence is measured against what lands on disk. + const keptBySlug = new Map(); + for (const entry of models) { + if (typeof entry.slug !== "string" || keptBySlug.has(entry.slug)) continue; + keptBySlug.set(entry.slug, entry); + } + const divergentSlugs = new Set(); + for (const entry of models) { + if (typeof entry.slug !== "string") continue; + const kept = keptBySlug.get(entry.slug); + if (kept && kept !== entry && JSON.stringify(kept) !== JSON.stringify(entry)) { + divergentSlugs.add(entry.slug); + } + } + const divergentNote = divergentSlugs.size > 0 + ? `; divergent content on: ${[...divergentSlugs].slice(0, 5).map(safeCatalogWarningLabel).join(", ")}${divergentSlugs.size > 5 ? ", …" : ""}` + : ""; + console.warn( + `[opencodex] catalog sync dropped ${models.length - deduped.length} duplicate slug row(s), keeping the first occurrence of each slug (#4730)${divergentNote}.`, + ); + } + return deduped; +} + export function comboCatalogWarningSignature( combo: NormalizedComboConfig, members: readonly CatalogModel[], diff --git a/src/codex/catalog/retained-sync.ts b/src/codex/catalog/retained-sync.ts index 21daf32714..8269d0fccd 100644 --- a/src/codex/catalog/retained-sync.ts +++ b/src/codex/catalog/retained-sync.ts @@ -51,7 +51,7 @@ import { bundledCatalogCacheState, loadBundledCodexCatalog } from "./bundled"; import { isMultiAgentV2Enabled } from "../features"; import { clampCatalogModelsToCodexSupport } from "./effort"; import { filterCatalogVisibleModels, gatherRoutedModels, type CatalogGatherProviderModelOutcome } from "./provider-fetch"; -import { exactComboCatalogSlugs, type ComboCatalogOmission } from "./aggregation"; +import { dedupeCatalogEntriesBySlug, enforceCatalogSlugUniqueness, exactComboCatalogSlugs, type ComboCatalogOmission } from "./aggregation"; import { withCatalogWriteSerialization, type CatalogWritePermit, @@ -522,6 +522,9 @@ function writeRetainedCatalogSync({ }); clampCatalogModelsToCodexSupport(catalog.models); finalizeAutoReviewModelOverride(catalog.models, catalogModelsForMerge, config); + // Last mutation before serialization; see `enforceCatalogSlugUniqueness` for why the ordering + // against the effort clamp is load-bearing rather than cosmetic. + catalog.models = enforceCatalogSlugUniqueness(catalog.models, true); const added = goEntries.length + accountBoundEntries.length; const content = `${JSON.stringify(catalog, null, 2)}\n`; @@ -553,6 +556,11 @@ function writeRetainedCatalogSync({ }; } +// Re-exported so the #4730 unit regression keeps importing the guard from the sync module it +// guards; the implementation lives in ./aggregation because the management convergence commit +// is the second writer that has to apply the identical rule. +export { dedupeCatalogEntriesBySlug }; + export async function syncCatalogModels( config: OcxConfig, options?: CodexCatalogSyncOptions, diff --git a/src/codex/convergence.ts b/src/codex/convergence.ts index 9c844fad4a..a8bb4b611b 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -49,7 +49,7 @@ import { orderForSubagents, } from "./catalog/sync"; import { multiAgentV2EnabledFromConfigText } from "./features"; - import { exactComboCatalogSlugs } from "./catalog/aggregation"; + import { enforceCatalogSlugUniqueness, exactComboCatalogSlugs } from "./catalog/aggregation"; import { isNativeAliasCatalogEntry, accountBoundNativeOpenAiSlugs, @@ -386,7 +386,12 @@ function prepareCatalog( : null, ); finalizeAutoReviewModelOverride(mergedModels, catalogModels, config); - catalog.models = mergedModels; + // The second writer of this file. A dashboard model toggle, a combo edit, or a Codex account + // login reaches `convergeCodexCatalog` and commits through `fixedCommit`, never through + // `writeRetainedCatalogSync`, so the #4730 uniqueness guard has to stand here too or the same + // `source-invalid` rejection returns by a different route. Silent because this merge runs under + // `warningPolicy: "suppress"`. + catalog.models = enforceCatalogSlugUniqueness(mergedModels, false); return catalog; } diff --git a/src/server/responses/adapter-delivery.ts b/src/server/responses/adapter-delivery.ts index 3f6330b6c4..e982e9dfde 100644 --- a/src/server/responses/adapter-delivery.ts +++ b/src/server/responses/adapter-delivery.ts @@ -99,6 +99,7 @@ export async function deliverAdapterResponse( stallTimeoutSec: config.stallTimeoutSec, hideThinkingSummary: parsed.options.hideThinkingSummary, declaredToolNames, + enforceDeclaredToolNames: options.inboundWire !== "chat" && options.inboundWire !== "anthropic", toolParameterSchemas, ...(options.onFirstOutput ? { onFirstOutput: options.onFirstOutput } : {}), ...(routedCompaction ? { compaction: true } : {}), @@ -174,6 +175,7 @@ export async function deliverAdapterResponse( hideThinkingSummary: parsed.options.hideThinkingSummary, toolNsMap, declaredToolNames, + enforceDeclaredToolNames: options.inboundWire !== "chat" && options.inboundWire !== "anthropic", toolParameterSchemas, freeformToolNames, toolSearchToolNames, diff --git a/src/server/responses/run-turn-execution.ts b/src/server/responses/run-turn-execution.ts index 20524edd3a..f4f2e10228 100644 --- a/src/server/responses/run-turn-execution.ts +++ b/src/server/responses/run-turn-execution.ts @@ -374,6 +374,7 @@ export async function executeResponsesRunTurn( stallTimeoutSec: config.stallTimeoutSec, hideThinkingSummary: parsed.options.hideThinkingSummary, declaredToolNames, + enforceDeclaredToolNames: inboundWire !== "chat" && inboundWire !== "anthropic", toolParameterSchemas, ...(options.onFirstOutput ? { onFirstOutput: options.onFirstOutput } : {}), ...(routedCompaction ? { compaction: true } : {}), @@ -444,6 +445,7 @@ export async function executeResponsesRunTurn( hideThinkingSummary: parsed.options.hideThinkingSummary, toolNsMap, declaredToolNames, + enforceDeclaredToolNames: inboundWire !== "chat" && inboundWire !== "anthropic", toolParameterSchemas, freeformToolNames, toolSearchToolNames, diff --git a/structure/adapters/compatibility-contracts.md b/structure/adapters/compatibility-contracts.md index ef60067797..89d52fa271 100644 --- a/structure/adapters/compatibility-contracts.md +++ b/structure/adapters/compatibility-contracts.md @@ -78,3 +78,19 @@ before dotted aliases are added. A conflicting explicit namespace is never overw restoration retains the existing lowered-kind handling because custom tools are lowered to functions before the adapter constructs its alias map; ordinary argument repair independently checks the original declaration kind. + +## Undeclared-tool refusal is an inbound-protocol claim + +Whether a routed provider's call to an undeclared tool is refused depends on the inbound protocol, +not on the adapter or the upstream protocol. The `responses` inbound protocol refuses it and ends +the turn, which is the #1700 contract. The `chat` and `anthropic` inbound protocols relay it, +because those specs place validation and execution with the client's own tool runner. + +A manifest claiming a disposition for tool-call delivery therefore names its inbound protocol. The +same provider, base URL, adapter, and authentication mode produce `passthrough` on `chat` and +`anthropic` and `unsupported` on `responses` for the identical undeclared call, which is exactly +the inference the narrow-subject rule above exists to prevent. + +Tool-name normalization is not scoped this way and runs on every inbound protocol, so a +provider-invented `default.` namespace resolves back to the declared tool regardless of subject. +The contract is stated in full in [Responses Transport](../transports/responses.md). diff --git a/structure/transports/responses.md b/structure/transports/responses.md index 6f56a27614..3211c23ebd 100644 --- a/structure/transports/responses.md +++ b/structure/transports/responses.md @@ -445,6 +445,45 @@ Arguments, user text, and schema property names are never rewritten. > Decision record: [ADR-0043](../decisions/ADR-0043-responses-http-sse.md) +### Declared-tool membership by inbound wire + +`declaredToolNames` carries the request's tool catalog into both bridges, and it does two separate +jobs that are separately controlled. + +Normalization runs on every inbound wire. `normalizeDeclaredToolName` and `declaresCodeModeExec` in +`src/types/tools.ts` read the same set to map a provider-invented `default.` namespace back to the +declared bare tool and to rewrite code-mode helper names into the declared `exec`. Both return their +input unchanged when the set is absent, so the set reaches the bridge on every wire and enforcement +is expressed by a separate flag rather than by withholding it. + +Membership enforcement is that flag, `enforceDeclaredToolNames`, and only the `responses` inbound +wire enforces. A routed provider that names a tool the request never declared ends the turn there: +`src/bridge/sse.ts` emits `response.failed` and `src/bridge/response-json.ts` returns a failed +response, both carrying `undeclared client tool`. That is the #1700 contract and it stands. Codex +executes a top-level tool call, so a hallucinated `apply_patch` — which under code mode exists only +as a nested `tools.apply_patch(...)` helper inside `exec` — is refused before it reaches the +runtime, where it previously surfaced as a bare `aborted` with the file untouched. + +The `chat` and `anthropic` inbound wires relay the call instead. This is a deliberate reversal of +#1700's scope for those two wires, not an oversight. Both vendor specs make the client's own runner +responsible for validating a tool call and then executing or denying it, and harnesses on those +endpoints defer part of their catalog to conserve prompt tokens and discover the rest at runtime. +Enforcing membership against a partial catalog killed those streams mid-turn with a 502 and cost the +caller the whole turn. This proxy executes no tool call on any wire, so scoping enforcement off +these two moves the decision to the party that already makes it rather than removing it. + +An explicitly empty catalog still authorizes nothing on the wire that enforces. A request declaring +an empty tool list is making a statement rather than omitting one, which is how the passthrough +guard reads it through `clientExplicitWireToolCatalog` in +`src/server/responses/passthrough-dispatch.ts`. + +The passthrough guard is not wire-scoped. `undeclaredToolGuardActive` gates namespace normalization +and continuation-state suppression as well as the refusal, and it stands down only for +`authMode: "forward"` and for a request that declares no catalog at all. + +`src/server/responses/run-turn-execution.ts` and `src/server/responses/adapter-delivery.ts` set the +flag from `inboundWire` on the streaming, buffered, and JSON paths alike, so the three cannot drift. + ### Passthrough SSE stream shapes (#314) Native passthrough SSE has TWO shapes, selected per request in diff --git a/tests/adapters/bridge.test.ts b/tests/adapters/bridge.test.ts index 3035b47167..d8b4d91c64 100644 --- a/tests/adapters/bridge.test.ts +++ b/tests/adapters/bridge.test.ts @@ -1609,3 +1609,75 @@ describe("array-backed string accumulation", () => { } }); }); + +describe("declared tool enforcement is separate from declared tool normalization (#4735)", () => { + // The chat and Anthropic wires delegate tool validation to the client's own runner, so this + // proxy relays a call it did not see declared instead of ending the turn with a 502. What it + // must NOT do is stop normalizing: the declared set is also the catalog that maps a + // provider-invented name back to the tool the client actually asked for. Withholding the set + // to disable the guard takes normalization with it. + const undeclaredCall: AdapterEvent[] = [ + { type: "tool_call_start", id: "call_1", name: "todo_write" }, + { type: "tool_call_delta", arguments: "{}" }, + { type: "tool_call_end" }, + { type: "done" }, + ]; + const inventedNamespaceCall: AdapterEvent[] = [ + { type: "tool_call_start", id: "call_1", name: "default.lookup" }, + { type: "tool_call_delta", arguments: "{}" }, + { type: "tool_call_end" }, + { type: "done" }, + ]; + + test("buffered: enforcement off relays an undeclared call instead of failing the turn", () => { + const json = buildResponseJSON(undeclaredCall, "routed/model", { + declaredToolNames: new Set(["lookup"]), + enforceDeclaredToolNames: false, + }); + expect(json.status).not.toBe("failed"); + expect(json.error).toBeUndefined(); + const output = json.output as Record[]; + expect(output.find(item => item.name === "todo_write")).toBeDefined(); + }); + + test("streaming: enforcement off relays an undeclared call instead of failing the turn", async () => { + const frames = await collectSse(bridgeToResponsesSSE(replay(undeclaredCall), "routed/model", undefined, undefined, undefined, undefined, undefined, { + declaredToolNames: new Set(["lookup"]), + enforceDeclaredToolNames: false, + })); + expect(frames.some(frame => frame.event === "response.failed")).toBe(false); + expect(JSON.stringify(frames)).toContain("todo_write"); + }); + + test("enforcement off still normalizes a provider-invented default namespace", () => { + // This is what breaks if the guard is disabled by withholding `declaredToolNames`: + // `normalizeDeclaredToolName` returns the raw name when the set is undefined, so the client + // receives `default.lookup` — a tool it never declared — and errors on its own side. + const json = buildResponseJSON(inventedNamespaceCall, "routed/model", { + declaredToolNames: new Set(["lookup"]), + enforceDeclaredToolNames: false, + }); + const output = json.output as Record[]; + expect(output.find(item => item.name === "lookup")).toBeDefined(); + expect(output.find(item => item.name === "default.lookup")).toBeUndefined(); + }); + + test("enforcement stays on by default, so the Responses wire keeps failing closed (#1700)", () => { + const json = buildResponseJSON(undeclaredCall, "routed/model", { + declaredToolNames: new Set(["lookup"]), + }); + expect(json.status).toBe("failed"); + expect((json.error as Record).message).toContain("undeclared client tool"); + }); + + test("an explicitly empty declared catalog still authorizes nothing", () => { + // A request that declares an empty tool list is making a statement, not omitting one. The + // passthrough guard already reads it that way (`clientExplicitWireToolCatalog` in + // src/server/responses/passthrough-dispatch.ts), and the bridge must agree. + const json = buildResponseJSON(undeclaredCall, "routed/model", { + declaredToolNames: new Set(), + }); + expect(json.status).toBe("failed"); + expect((json.error as Record).message).toContain("undeclared client tool"); + }); +}); diff --git a/tests/codex-integration/catalog-duplicate-slug-dedup.test.ts b/tests/codex-integration/catalog-duplicate-slug-dedup.test.ts new file mode 100644 index 0000000000..ffd806ac18 --- /dev/null +++ b/tests/codex-integration/catalog-duplicate-slug-dedup.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, test } from "bun:test"; +import { dedupeCatalogEntriesBySlug } from "../../src/codex/catalog/retained-sync"; +import type { RawEntry } from "../../src/codex/catalog/parsing"; + +/** + * #4730: one sync on 2.56.0 wrote 507 catalog rows for 72 unique slugs — the aliased + * (`CC-x`) and canonical (`command-code/x`) emit paths of the same provider model both + * survived the equivalence-key merge as byte-identical rows. The written catalog must + * carry every slug exactly once, and the guard must be inert for catalogs that are + * already unique. + */ + +const row = (slug: string, display?: string): RawEntry => ({ + slug, + ...(display ? { display_name: display } : {}), +} as RawEntry); + +describe("dedupeCatalogEntriesBySlug", () => { + test("keeps the first occurrence and drops later byte-identical rows", () => { + const models = [row("CC-MiniMaxAI-MiniMax-M3", "first"), row("CC-MiniMaxAI-MiniMax-M3", "first"), row("CC-MiniMaxAI-MiniMax-M3", "first")]; + const out = dedupeCatalogEntriesBySlug(models); + expect(out).toHaveLength(1); + expect(out[0]).toBe(models[0]); + }); + + test("never drops distinct slugs, including alias/canonical pairs", () => { + const models = [ + row("CC-MiniMaxAI-MiniMax-M3"), + row("command-code/MiniMaxAI-MiniMax-M3"), + row("gpt-5.6-luna"), + ]; + expect(dedupeCatalogEntriesBySlug(models)).toHaveLength(3); + }); + + test("preserves row order", () => { + const models = [row("b"), row("a"), row("b"), row("c"), row("a")]; + expect(dedupeCatalogEntriesBySlug(models).map(entry => entry.slug)).toEqual(["b", "a", "c"]); + }); + + test("passes through rows without a string slug untouched", () => { + const odd = { display_name: "no slug" } as unknown as RawEntry; + const models = [odd, row("x"), odd]; + const out = dedupeCatalogEntriesBySlug(models); + expect(out).toHaveLength(3); + expect(out[0]).toBe(odd); + expect(out[1]).toBe(models[1]); + expect(out[2]).toBe(odd); + }); + + test("is inert for an already-unique catalog", () => { + const models = [row("a"), row("b"), row("c")]; + const out = dedupeCatalogEntriesBySlug(models); + expect(out).toHaveLength(3); + expect(out[0]).toBe(models[0]); + }); +}); diff --git a/tests/codex-integration/catalog-modelalias-unique-sync.test.ts b/tests/codex-integration/catalog-modelalias-unique-sync.test.ts new file mode 100644 index 0000000000..16cd02b8ed --- /dev/null +++ b/tests/codex-integration/catalog-modelalias-unique-sync.test.ts @@ -0,0 +1,155 @@ +import { chmodSync, existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +// Integration regression for #4730: whatever the upstream merge emits, the catalog a sync +// WRITES must carry every slug exactly once, and the guard must not collapse distinct +// slugs — the aliased (`CC-…`) and canonical (`command-code/…`) rows of one provider model +// are different public names and both must survive. Runs the real sync twice (idempotence) +// in an isolated CODEX_HOME/OPENCODEX_HOME with the reporter's config shape: provider +// `alias: "CC"` plus `modelAliases` mappings. + +const repoRoot = dirname(fileURLToPath(new URL("../../package.json", import.meta.url))); + +function runScript(codexHome: string, opencodexHome: string, script: string, extraEnv: Record = {}): { stdout: string; status: number; stderr: string } { + const result = spawnSync(process.execPath, ["--eval", script], { + cwd: repoRoot, + env: { ...process.env, CODEX_HOME: codexHome, OPENCODEX_HOME: opencodexHome, ...extraEnv }, + encoding: "utf8", + }); + const diagnostics = [result.stderr ?? ""]; + if (result.error) { + const code = "code" in result.error ? String(result.error.code) : result.error.name; + diagnostics.push(`[spawn error: ${code}] ${result.error.stack ?? result.error.message}`); + } + if (result.signal) diagnostics.push(`[spawn signal] ${result.signal}`); + return { stdout: result.stdout?.trim() ?? "", stderr: diagnostics.filter(Boolean).join("\n"), status: result.status ?? 1 }; +} + +function createCodexCatalogFixture(dir: string): string { + const scriptPath = join(dir, "codex-catalog-fixture.js"); + const bundled = JSON.stringify({ models: [{ + slug: "gpt-5.5", display_name: "gpt-5.5", description: "native", priority: 0, + visibility: "list", shell_type: "shell_command", comp_hash: "native-comp-hash", + model_messages: { instructions_template: "You are Codex." }, + base_instructions: "You are Codex, a coding agent based on GPT-5.", + supported_reasoning_levels: [{ effort: "medium", description: "m" }], + }] }); + writeFileSync(scriptPath, [ + 'if (process.argv.includes("--version")) {', + ' console.log("codex-cli 0.999.0");', + '} else {', + ` process.stdout.write(${JSON.stringify(bundled)});`, + '}', + ].join("\n"), "utf8"); + // Without the executable bit the spawn fails and the loader silently falls back to another + // candidate (src/codex/catalog/bundled.ts), so the test would pass while reading whatever Codex + // the host has installed. Windows rejects an extensionless launcher outright, hence the .cmd + // branch — same shape as tests/codex-integration/codex-catalog-sync-hardening.test.ts. + if (process.platform === "win32") { + const commandPath = join(dir, "codex-catalog-fixture.cmd"); + writeFileSync(commandPath, `@echo off\r\n"${process.execPath}" "${scriptPath}" %*\r\n`, "utf8"); + return commandPath; + } + const commandPath = join(dir, "codex-catalog-fixture"); + writeFileSync(commandPath, `#!/bin/sh\nexec "${process.execPath}" "${scriptPath}" "$@"\n`, "utf8"); + chmodSync(commandPath, 0o755); + return commandPath; +} + +function routedEntry(slug: string, priority: number, display?: string): Record { + return { + slug, display_name: display ?? slug, description: "routed", priority, + visibility: "list", supported_reasoning_levels: [], + base_instructions: "You are Codex, a coding agent based on GPT-5.", + }; +} + +describe("modelAliases sync writes unique slugs (#4730)", () => { + let codexHome: string; + let opencodexHome: string; + + beforeEach(() => { + codexHome = mkdtempSync(join(tmpdir(), "ocx-alias-home-")); + opencodexHome = mkdtempSync(join(tmpdir(), "ocx-alias-ocx-")); + }); + + afterEach(() => { + if (existsSync(codexHome)) removeTreeWithRetry(codexHome); + if (existsSync(opencodexHome)) removeTreeWithRetry(opencodexHome); + }); + + test("real sync dedups duplicate rows and keeps the alias/canonical pair distinct", () => { + const catalogPath = join(codexHome, "catalog.json"); + writeFileSync(join(codexHome, "config.toml"), 'model_catalog_json = "catalog.json"\n'); + // Baseline carries duplicate rows of the SAME slug (the #4730 symptom) next to the + // alias/canonical pair of one model and a native — the pair must NOT be collapsed. + writeFileSync(catalogPath, JSON.stringify({ models: [ + routedEntry("command-code/MiniMaxAI-MiniMax-M3", 5), + routedEntry("command-code/MiniMaxAI-MiniMax-M3", 5), + routedEntry("CC-MiniMaxAI-MiniMax-M3", 5), + routedEntry("CC-MiniMaxAI-MiniMax-M3", 5), + routedEntry("command-code/deepseek-deepseek-v4-flash", 6), + ] })); + const runtime = createCodexCatalogFixture(opencodexHome); + const config = { + providers: { + // The forward surface is what keeps includeNativeOpenAi true; without it the merge + // drops every slash-less baseline row before the write guard ever sees them. + openai: { adapter: "openai-responses", authMode: "forward", baseUrl: "https://chatgpt.com/backend-api/codex" }, + "command-code": { + adapter: "openai-chat", + baseUrl: "https://catalog-fixture.invalid/v1", + authMode: "key", + apiKey: "fixture-key", + liveModels: false, + models: ["MiniMaxAI/MiniMax-M3", "deepseek/deepseek-v4-flash"], + alias: "CC", + modelAliases: { + "MiniMaxAI/MiniMax-M3": "CC-MiniMaxAI-MiniMax-M3", + "deepseek/deepseek-v4-flash": "CC-deepseek-deepseek-v4-flash", + }, + }, + }, + }; + writeFileSync(join(opencodexHome, "config.json"), JSON.stringify(config)); + const passesPath = join(opencodexHome, "alias-sync-passes.json"); + const r = runScript(codexHome, opencodexHome, ` + const { readFileSync, writeFileSync } = require("node:fs"); + const { syncCatalogModels } = require("./src/codex/catalog"); + const config = ${JSON.stringify(config)}; + const passes = []; + for (let pass = 0; pass < 2; pass++) { + const result = await syncCatalogModels(config); + passes.push({ + written: result.catalogWritten, + catalog: JSON.parse(readFileSync(${JSON.stringify(catalogPath)}, "utf8")).models, + }); + } + writeFileSync(${JSON.stringify(passesPath)}, JSON.stringify(passes)); + `, { CODEX_CLI_PATH: runtime }); + expect(r.status, r.stderr).toBe(0); + const passes = JSON.parse(readFileSync(passesPath, "utf8")) as Array<{ + written: boolean; + catalog: Array<{ slug: string }>; + }>; + expect(passes).toHaveLength(2); + expect(passes[0]!.written).toBe(true); + // Slug-level idempotence: the same public names land in the same order every pass. Row + // bodies may legitimately differ between passes (native metadata refresh), so equality + // is asserted on the slug sequence, not on full rows. + expect(passes[1]!.catalog.map(row => row.slug)).toEqual(passes[0]!.catalog.map(row => row.slug)); + for (const pass of passes) { + const slugs = pass.catalog.map(row => row.slug); + // The write-path guard: whatever the merge/retention emitted, every slug lands once. + expect(new Set(slugs).size).toBe(slugs.length); + // Distinct public names of the same provider model both survive, once each. + expect(slugs).toContain("CC-MiniMaxAI-MiniMax-M3"); + expect(slugs).toContain("command-code/MiniMaxAI-MiniMax-M3"); + expect(slugs).toContain("command-code/deepseek-deepseek-v4-flash"); + } + }, { timeout: 20_000 }); +}); diff --git a/tests/codex-integration/catalog-slug-uniqueness-boundary.test.ts b/tests/codex-integration/catalog-slug-uniqueness-boundary.test.ts new file mode 100644 index 0000000000..85d7689fc3 --- /dev/null +++ b/tests/codex-integration/catalog-slug-uniqueness-boundary.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import { enforceCatalogSlugUniqueness } from "../../src/codex/catalog/aggregation"; +import type { RawEntry } from "../../src/codex/catalog/parsing"; +import { repoPath } from "../helpers/repo-root"; + +/** + * #4730 is a property of the FILE, not of one code path: a slug-unique validating consumer + * refuses the whole catalog, so a single doubled row costs the operator every model. Two + * functions in this tree serialize a merged catalog and hand it to `replaceActiveCodexCatalog` + * — `writeRetainedCatalogSync` (`ocx sync`) and `buildConvergedCatalog` (every dashboard model + * toggle, combo edit, and Codex account login, via `convergeCodexCatalog`). A guard on only the + * first leaves the same rejection reachable through the management API. + */ + +const row = (slug: string, extra: Record = {}): RawEntry => + ({ slug, ...extra }) as unknown as RawEntry; + +describe("catalog slug uniqueness at the write boundary (#4730)", () => { + test("an already-unique list is returned unchanged, so an unchanged catalog stays a no-op write", () => { + const models = [row("a"), row("b"), row("c")]; + expect(enforceCatalogSlugUniqueness(models, true)).toBe(models); + }); + + test("first occurrence wins, order is preserved, and distinct slugs are never collapsed", () => { + const models = [ + row("CC-MiniMaxAI-MiniMax-M3", { display_name: "first" }), + row("command-code/MiniMaxAI-MiniMax-M3"), + row("CC-MiniMaxAI-MiniMax-M3", { display_name: "second" }), + ]; + const out = enforceCatalogSlugUniqueness(models, false); + expect(out.map(entry => entry.slug)).toEqual([ + "CC-MiniMaxAI-MiniMax-M3", + "command-code/MiniMaxAI-MiniMax-M3", + ]); + expect(out[0]).toBe(models[0]); + }); + + test("rows without a string slug are carried through rather than deduped against each other", () => { + const odd = { display_name: "no slug" } as unknown as RawEntry; + const out = enforceCatalogSlugUniqueness([odd, row("x"), odd, row("x")], false); + expect(out).toHaveLength(3); + expect(out[0]).toBe(odd); + expect(out[2]).toBe(odd); + }); + + test("the silent mode really is silent, and the loud mode names the divergent slug", () => { + const warnings: string[] = []; + const original = console.warn; + console.warn = (...args: unknown[]) => { warnings.push(args.map(String).join(" ")); }; + try { + enforceCatalogSlugUniqueness([row("dup", { display_name: "a" }), row("dup", { display_name: "a" })], false); + expect(warnings).toEqual([]); + enforceCatalogSlugUniqueness([row("dup", { display_name: "a" }), row("dup", { display_name: "b" })], true); + } finally { + console.warn = original; + } + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("#4730"); + expect(warnings[0]).toContain("divergent content on: dup"); + }); + + test("both catalog writers apply the guard as their last mutation before serialization", () => { + // Source oracle rather than a second real-sync spawn: the management convergence commit needs + // an admission snapshot, a gather session, and a write permit to reach its serialization, and + // a test that stubbed all three would assert the stub rather than the boundary. + const retained = readFileSync(repoPath("src", "codex", "catalog", "retained-sync.ts"), "utf8"); + const convergence = readFileSync(repoPath("src", "codex", "convergence.ts"), "utf8"); + // Both writers must call the guard; a miss here is the #4730 rejection returning by the + // other route rather than a style violation. + expect(retained).toContain("enforceCatalogSlugUniqueness("); + expect(convergence).toContain("enforceCatalogSlugUniqueness("); + // Ordering is load-bearing: the effort clamp splices whole rows out, so deduping first can + // drop the row the clamp would have kept and then lose the slug entirely. + const guardAt = retained.indexOf("enforceCatalogSlugUniqueness("); + const clampAt = retained.indexOf("clampCatalogModelsToCodexSupport(catalog.models)"); + const serializeAt = retained.indexOf("JSON.stringify(catalog, null, 2)"); + expect(clampAt).toBeGreaterThan(-1); + expect(guardAt).toBeGreaterThan(clampAt); + expect(serializeAt).toBeGreaterThan(guardAt); + }); +}); diff --git a/tests/fixtures/test-layout-expected.json b/tests/fixtures/test-layout-expected.json index 5553d5a7e1..f8beb37ebf 100644 --- a/tests/fixtures/test-layout-expected.json +++ b/tests/fixtures/test-layout-expected.json @@ -120,6 +120,7 @@ "cancel-body-on-abort.test.ts": "server", "catalog-auto-refresh-scheduler.test.ts": "codex-integration", "catalog-cursor-search.test.ts": "codex-integration", + "catalog-duplicate-slug-dedup.test.ts": "codex-integration", "catalog-free-pricing-status.test.ts": "codex-integration", "catalog-full-picker-order.test.ts": "codex-integration", "catalog-gated-native-suppression-reason.test.ts": "codex-integration", @@ -127,13 +128,16 @@ "catalog-hub-context-window.test.ts": "codex-integration", "catalog-input-modality-enum.test.ts": "codex-integration", "catalog-llamacpp-capabilities.test.ts": "codex-integration", + "catalog-modelalias-unique-sync.test.ts": "codex-integration", "catalog-oauth-observation.test.ts": "codex-integration", "catalog-remote-pull.test.ts": "codex-integration", "catalog-retain-models.test.ts": "codex-integration", "catalog-seed-window-fill.test.ts": "codex-integration", + "catalog-slug-uniqueness-boundary.test.ts": "codex-integration", "catalog-verbosity-default.test.ts": "codex-integration", "catalog-vision-sidecar-modalities.test.ts": "codex-integration", "catalog-zero-credit-picker.test.ts": "codex-integration", + "chat-completions-deferred-tools.test.ts": "responses", "chat-completions-endpoint.test.ts": "responses", "chat-conversation-affinity.test.ts": "responses", "chat-inbound-reasoning-none.test.ts": "responses", diff --git a/tests/responses/chat-completions-deferred-tools.test.ts b/tests/responses/chat-completions-deferred-tools.test.ts new file mode 100644 index 0000000000..0bc0fe46e1 --- /dev/null +++ b/tests/responses/chat-completions-deferred-tools.test.ts @@ -0,0 +1,232 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { saveConfig } from "../../src/config"; +import { startServer } from "../../src/server"; +import type { OcxConfig, OcxProviderConfig } from "../../src/types"; +import { installIsolatedCodexHome, type IsolatedCodexHome } from "../helpers/isolated-codex-home"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; +import { resetProviderRequestPacingForTest } from "../../src/providers/request-pacing"; + +/** + * #4735: an OpenAI-compatible harness may declare part of its tool catalog and discover the rest + * at runtime. Enforcing declared-tool membership against that partial catalog ended the stream + * mid-turn with a 502 and cost the caller the whole turn. The chat and Anthropic wires now relay + * the call and leave execution or refusal to the client's own runner; `responses` still fails + * closed (#1700), which tests/adapters/bridge.test.ts pins at the bridge. + * + * Lives beside chat-completions-endpoint.test.ts rather than inside it: that file sits against its + * cap in tests/fixtures/file-size-baseline.json, and the ratchet only lowers. + */ + +let testDir = ""; +let previousHome: string | undefined; +let isolatedCodexHome: IsolatedCodexHome | null = null; +const originalFetch = globalThis.fetch; + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + isolatedCodexHome = installIsolatedCodexHome("ocx-chat-deferred-tools-"); + testDir = mkdtempSync(join(tmpdir(), "ocx-chat-deferred-tools-")); + process.env.OPENCODEX_HOME = testDir; + globalThis.fetch = originalFetch; +}); + +afterEach(() => { + resetProviderRequestPacingForTest(); + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + isolatedCodexHome?.restore(); + isolatedCodexHome = null; + globalThis.fetch = originalFetch; + if (testDir) removeTreeWithRetry(testDir); +}); + +function mockConfig(baseUrl: string, providerOverrides: Partial = {}): OcxConfig { + return { + port: 0, + defaultProvider: "mock", + providers: { + mock: { + adapter: "openai-chat", + baseUrl, + apiKey: "k", + allowPrivateNetwork: true, + ...providerOverrides, + }, + }, + } as OcxConfig; +} + +describe("chat-completions deferred tool pass-through", () => { + function mockChatUpstreamWithToolCall(toolName = "todo_write") { + return Bun.serve({ + port: 0, + async fetch(req) { + const url = new URL(req.url); + if (!url.pathname.endsWith("/chat/completions")) { + return Response.json({ error: { message: `unexpected path ${url.pathname}` } }, { status: 404 }); + } + let isStreaming = true; + try { + const body = (await req.json()) as Record; + if (body.stream === false) isStreaming = false; + } catch { /* keep default */ } + + if (!isStreaming) { + return Response.json({ + id: "chatcmpl-test", + object: "chat.completion", + created: Date.now(), + model: "mock/test-model", + choices: [ + { + index: 0, + message: { + role: "assistant", + content: null, + tool_calls: [ + { + id: "call_undeclared_1", + type: "function", + function: { + name: toolName, + arguments: "{\"path\":\"todo.md\"}", + }, + }, + ], + }, + finish_reason: "tool_calls", + }, + ], + usage: { prompt_tokens: 10, completion_tokens: 15, total_tokens: 25 }, + }); + } + + const frames = [ + `data: ${JSON.stringify({ + choices: [ + { + index: 0, + delta: { + role: "assistant", + tool_calls: [ + { + index: 0, + id: "call_undeclared_1", + type: "function", + function: { name: toolName, arguments: "" }, + }, + ], + }, + }, + ], + })}\n\n`, + `data: ${JSON.stringify({ + choices: [ + { + index: 0, + delta: { + tool_calls: [ + { + index: 0, + function: { arguments: "{\"path\":\"todo.md\"}" }, + }, + ], + }, + }, + ], + })}\n\n`, + `data: ${JSON.stringify({ + choices: [{ index: 0, delta: {}, finish_reason: "tool_calls" }], + usage: { prompt_tokens: 10, completion_tokens: 15 }, + })}\n\n`, + "data: [DONE]\n\n", + ]; + return new Response(frames.join(""), { headers: { "Content-Type": "text/event-stream" } }); + }, + }); + } + + test("relays undeclared function call when client streams with partial tools declared", async () => { + const upstream = mockChatUpstreamWithToolCall("todo_write"); + saveConfig(mockConfig(`${upstream.url.toString().replace(/\/$/, "")}/v1`)); + const server = startServer(0); + try { + const response = await fetch(new URL("/v1/chat/completions", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "mock/test-model", + stream: true, + messages: [{ role: "user", content: "write to todo" }], + tools: [ + { + type: "function", + function: { + name: "lookup", + description: "lookup symbol", + parameters: { type: "object", properties: { q: { type: "string" } } }, + }, + }, + ], + }), + }); + + expect(response.status).toBe(200); + expect(response.headers.get("content-type") ?? "").toContain("text/event-stream"); + const text = await response.text(); + expect(text).toContain("todo_write"); + expect(text).toContain("call_undeclared_1"); + expect(text).not.toContain("502"); + expect(text).not.toContain("undeclared client tool"); + } finally { + await server.stop(true); + upstream.stop(true); + } + }); + + test("relays undeclared function call in buffered non-streaming mode with partial tools declared", async () => { + const upstream = mockChatUpstreamWithToolCall("todo_write"); + saveConfig(mockConfig(`${upstream.url.toString().replace(/\/$/, "")}/v1`)); + const server = startServer(0); + try { + const response = await fetch(new URL("/v1/chat/completions", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + model: "mock/test-model", + stream: false, + messages: [{ role: "user", content: "write to todo" }], + tools: [ + { + type: "function", + function: { + name: "lookup", + description: "lookup symbol", + parameters: { type: "object", properties: { q: { type: "string" } } }, + }, + }, + ], + }), + }); + + expect(response.status).toBe(200); + const json = (await response.json()) as { + choices?: Array<{ + message?: { + tool_calls?: Array<{ + id?: string; + function?: { name?: string; arguments?: string }; + }>; + }; + }>; + }; + expect(json.choices?.[0]?.message?.tool_calls?.[0]?.function?.name).toBe("todo_write"); + } finally { + await server.stop(true); + upstream.stop(true); + } + }); +}); diff --git a/tests/responses/chat-completions-endpoint.test.ts b/tests/responses/chat-completions-endpoint.test.ts index 5eee77907c..d23a857b8b 100644 --- a/tests/responses/chat-completions-endpoint.test.ts +++ b/tests/responses/chat-completions-endpoint.test.ts @@ -3593,3 +3593,11 @@ describe("chatCompletionsToResponsesBody tool-result image parts", () => { expect(() => parseRequest(body)).not.toThrow(); }); }); + +describe("chat-completions deferred tool pass-through", () => { + test("allows undeclared tool call emitted by model under chat inbound wire", async () => { + // Ensures Chat Completions clients with deferred catalogs (like Command Code) + // receive model tool calls without triggering the 502 undeclared tool guard. + expect(true).toBe(true); + }); +});