diff --git a/src/codex/catalog/retained-sync.ts b/src/codex/catalog/retained-sync.ts index 21daf32714..abad32769a 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 { exactComboCatalogSlugs, safeCatalogWarningLabel, type ComboCatalogOmission } from "./aggregation"; import { withCatalogWriteSerialization, type CatalogWritePermit, @@ -520,6 +520,30 @@ function writeRetainedCatalogSync({ warningPolicy: "emit", }, }); + const dedupedCatalogModels = dedupeCatalogEntriesBySlug(catalog.models); + if (dedupedCatalogModels.length !== catalog.models.length) { + // A dropped row that differs from the kept one means two emit paths disagree about + // the same slug's content. First-win still stands (the merge ranked the kept row), + // but the operator needs to see WHICH slugs diverged instead of silently losing data. + const keptBySlug = new Map(catalog.models.flatMap(entry => ( + typeof entry.slug === "string" ? [[entry.slug, entry] as const] : [] + ))); + const divergentSlugs = new Set(); + for (const entry of catalog.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 ${catalog.models.length - dedupedCatalogModels.length} duplicate slug row(s), keeping the first occurrence of each slug (#4730)${divergentNote}.`, + ); + catalog.models = dedupedCatalogModels; + } clampCatalogModelsToCodexSupport(catalog.models); finalizeAutoReviewModelOverride(catalog.models, catalogModelsForMerge, config); @@ -553,6 +577,30 @@ function writeRetainedCatalogSync({ }; } +/** + * Final guard for the written catalog: every slug must appear exactly once (#4730). + * + * Two emit paths can hand the merge the same model under the same Codex-facing slug and the + * equivalence-key merge keeps both (a slash-less `model.alias` slug is an "exact" key, so the + * aliased and canonical rows of one provider model never collapse). Observed on 2.56.0: a sync + * produced 507 rows for 72 unique slugs, every duplicate byte-identical. Keep the FIRST + * occurrence — the merge already ranked it — and never touch distinct slugs. + */ +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; +} + export async function syncCatalogModels( config: OcxConfig, options?: CodexCatalogSyncOptions, 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..38ddcbeff8 --- /dev/null +++ b/tests/codex-integration/catalog-modelalias-unique-sync.test.ts @@ -0,0 +1,145 @@ +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, 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"); + const commandPath = join(dir, "codex-catalog-fixture"); + writeFileSync(commandPath, `#!/bin/sh\nexec "${process.execPath}" "${scriptPath}" "$@"\n`, "utf8"); + 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 }); +});