diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts new file mode 100644 index 0000000000..84521d0a4b --- /dev/null +++ b/src/codex/catalog/display-labels.ts @@ -0,0 +1,110 @@ +/** + * Operator-supplied display labels for live-discovered provider models (#2201). + * + * A discovered row's label is its routed slug, so NVIDIA NIM surfaces as + * `nvidia/deepseek-ai-deepseek-v4-flash-0731` in the picker, the dashboard, + * `/v1/models`, and client exports. `customModels[].displayName` and combo + * display labels already relabel their rows display-only; live discovery is the + * one row source with no equivalent. + * + * The single invariant: a display label is never routing identity. Nothing here + * touches `provider`, `id`, or the routed slug — the resolved label lands on + * `CatalogModel.displayName`, which `applyCatalogModelMetadata` already treats + * as display-only, and which client exports already read. + */ + +import { COMBO_NAMESPACE } from "../../combos/types"; +import type { OcxConfig } from "../../types/config"; +import { CODEX_CUSTOM_MODEL_CATALOG_KIND } from "./parsing"; +import type { CatalogModel } from "./parsing"; + +/** Same bound as the combo display label (src/combos/types.ts) so every label surface agrees. */ +export const MAX_DISPLAY_LABEL_LENGTH = 128; + +// Control characters corrupt picker rendering, so a label carrying one is rejected. +// The range is C0, DEL, and C1 (U+0080-U+009F). C1 was originally missing, which let +// a label such as `LabelMore` through — U+0085 is NEL, a line break, and +// `trim()` does not touch a mid-string one. U+2028/U+2029 are added for the same +// reason: they are line and paragraph separators, so a label carrying one is not +// single-line whatever its width. +const CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/; + +/** + * Checked against the *untrimmed* value, unlike `CONTROL_CHARS`. + * + * `trim()` counts U+2028/U+2029 as whitespace and would strip an edge one, so a + * trailing line separator would otherwise be normalised away and reported as + * valid. A stray space or newline is plausible slop in a hand-edited config and + * is still forgiven; a Unicode line separator is not, so it is rejected wherever + * it appears rather than quietly removed. + */ +const CONTROLS_TRIM_WOULD_HIDE = /[\u0080-\u009f\u2028\u2029]/; + +/** + * A label is usable when it is a non-empty single-line string within the shared bound. + * + * Slashes are rejected, matching the `customModels[].displayName` rule: a label + * containing `/` reads as a routed slug, and this field must never be mistaken + * for one. + */ +export function isValidDisplayLabel(value: unknown): value is string { + if (typeof value !== "string") return false; + if (CONTROLS_TRIM_WOULD_HIDE.test(value)) return false; + const trimmed = value.trim(); + if (trimmed.length === 0 || trimmed.length > MAX_DISPLAY_LABEL_LENGTH) return false; + if (CONTROL_CHARS.test(trimmed)) return false; + return !trimmed.includes("/"); +} + +/** + * The precedence chain, in one place: + * + * 1. operator override — `providers[].modelDisplayNames[]` + * 2. trusted discovery metadata — a label discovery already attached + * 3. undefined — caller keeps its derived slug, i.e. today's behaviour + * + * Rows that already own an operator-supplied label keep it, and the provider map + * must not outrank them: + * - combo rows validate their own bounded label independently, so an entry under + * the combo namespace is never relabelled from provider config; + * - an explicit `customModels[]` row carries the label the operator typed there, + * and #2201 requires those to continue unchanged. Matching on `catalogKind` + * rather than on the provider name is what makes that hold, because a custom + * model shares its provider with the discovered rows this function exists for. + * + * Native OpenAI rows never reach here at all — they come from the pinned snapshot + * path with no `CatalogModel` — so upstream marketing names stay untouched. + */ +export function resolveModelDisplayLabel( + config: OcxConfig, + model: CatalogModel, +): string | undefined { + if (model.provider === COMBO_NAMESPACE || model.catalogKind === CODEX_CUSTOM_MODEL_CATALOG_KIND) { + return isValidDisplayLabel(model.displayName) ? model.displayName.trim() : undefined; + } + const override = config.providers?.[model.provider]?.modelDisplayNames?.[model.id]; + if (isValidDisplayLabel(override)) return override.trim(); + if (isValidDisplayLabel(model.displayName)) return model.displayName.trim(); + return undefined; +} + +/** + * Resolve labels across a discovered model list. + * + * Returns the input array unchanged when nothing resolves, and otherwise a new + * array of new objects — the input models are never mutated, so a caller holding + * the pre-label list keeps it intact. + */ +export function applyOperatorDisplayLabels( + models: CatalogModel[], + config: OcxConfig, +): CatalogModel[] { + let changed = false; + const labeled = models.map(model => { + const label = resolveModelDisplayLabel(config, model); + if (label === undefined || label === model.displayName) return model; + changed = true; + return { ...model, displayName: label }; + }); + return changed ? labeled : models; +} diff --git a/src/codex/convergence.ts b/src/codex/convergence.ts index d5aeb893b7..9129af2d75 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -74,6 +74,7 @@ import { resolveCodexModelEntitlements, type CodexModelEntitlementSnapshot, } from "./model-entitlements"; +import { applyOperatorDisplayLabels } from "./catalog/display-labels"; import { ACCOUNT_GATED_NATIVE_OPENAI_MODELS } from "./catalog/native-models"; import { providerCodexAccountMode } from "../providers/registry"; import { OPENAI_CODEX_PROVIDER_ID } from "../providers/openai-tiers"; @@ -237,7 +238,11 @@ function prepareCatalog( const template = findNativeTemplate(catalog); const enabled = filterCatalogVisibleModels(routedModels, config); const featured = config.subagentModels ?? []; - const ordered = orderForSubagents(enabled, featured); + // #2201: resolve operator display labels before ordering. Display-only — the + // routed slug, provider id and native model id are all unchanged, so ordering, + // featuring and spawn-candidate derivation below see the same identities. + const labeled = applyOperatorDisplayLabels(enabled, config); + const ordered = orderForSubagents(labeled, featured); const modelPickerOrder = config.modelPickerOrder ?? []; const multiAgentMode = config.multiAgentMode === "v1" || config.multiAgentMode === "v2" ? config.multiAgentMode : "default"; diff --git a/src/config.ts b/src/config.ts index dcf34313a4..046d34f132 100644 --- a/src/config.ts +++ b/src/config.ts @@ -29,6 +29,7 @@ import { MAIN_CODEX_ACCOUNT_NAMESPACE_TARGET, } from "./codex/account-namespace-match"; import { isCodexAccountPriorityKey } from "./codex/account-priority"; +import { MAX_DISPLAY_LABEL_LENGTH, isValidDisplayLabel } from "./codex/catalog/display-labels"; import { UPSTREAM_HOST_CIRCUIT_MAX_THRESHOLD } from "./codex/upstream-host-health"; import { adoptCustomModelCatalogMigration, @@ -714,6 +715,25 @@ const providerConfigSchema = z.object({ fastWire: fastWireSchema.nullable().optional(), supportsServiceTier: z.boolean().optional(), modelSupportsServiceTier: z.record(z.string().min(1), z.boolean()).optional(), + // Display-only labels for discovered models, keyed by native model id — the same + // key space as `modelAdapters`. Declared rather than left to `.passthrough()` + // below, for the reason the `codexToolMode` comment gives: an undeclared key is + // accepted, persisted, and then silently ignored (#2106). + // + // Salvaged entry by entry rather than validated strictly, following `apiKeys`: + // one hand-edited label must not send the whole config through the + // backup-and-defaults repair path, and must not take the operator's other + // labels down with it. Writes go through displayLabelRecordConfigError instead, + // so an invalid label is a 400 at the API and a dropped entry on load. + modelDisplayNames: z.unknown().optional().transform(value => { + if (value === undefined || value === null) return undefined; + if (typeof value !== "object" || Array.isArray(value)) return undefined; + const kept = Object.entries(value as Record) + .filter(([id, label]) => id.trim().length > 0 && isValidDisplayLabel(label)) + .slice(0, MAX_MODEL_DISPLAY_NAMES) + .map(([id, label]) => [id.trim(), (label as string).trim()] as const); + return kept.length > 0 ? Object.fromEntries(kept) : undefined; + }), preserveResponsesReasoningContent: z.boolean().optional(), decodesNativeCompactionBlobs: z.boolean().optional(), allowPrivateNetwork: z.boolean().optional(), @@ -947,6 +967,48 @@ export function booleanRecordConfigError(value: unknown, field: string): string return null; } +/** + * Bound on how many labels one provider may carry. A display map is a convenience, + * not a catalogue, so an unbounded hand-edited map is a mistake rather than a use + * case — and every entry is walked on each convergence. + */ +export const MAX_MODEL_DISPLAY_NAMES = 512; + +/** + * Strict diagnostic for `providers[].modelDisplayNames`, mirroring + * `booleanRecordConfigError`. + * + * This is the *write* rule, used by the provider editor so a bad label is a 400 + * rather than something that lands on disk. The load path is deliberately more + * forgiving — see the schema entry, which drops a bad entry instead of failing — + * because the two paths answer different questions: "is this a valid edit?" and + * "can this file still be served?". + * + * `null` is accepted as an explicit clear, matching `upstreamHttpVersion`: the + * management API says null means "remove this", so rejecting it here would refuse + * the documented way to take a label back off. + */ +export function displayLabelRecordConfigError(value: unknown, field = "modelDisplayNames"): string | null { + if (value === undefined) return null; + if (!value || typeof value !== "object" || Array.isArray(value)) return `${field} must be a plain object`; + const prototype = Object.getPrototypeOf(value); + if (prototype !== Object.prototype && prototype !== null) return `${field} must be a plain object with own properties`; + const entries = Object.entries(value); + if (entries.length > MAX_MODEL_DISPLAY_NAMES) { + return `${field} must hold at most ${MAX_MODEL_DISPLAY_NAMES} entries`; + } + for (const [key, label] of entries) { + if (!key.trim()) return `${field} keys must be nonblank model ids`; + if (label === null) continue; + if (typeof label !== "string") return `${field}.${key} must be a string`; + if (!isValidDisplayLabel(label)) { + return `${field}.${key} must be a nonblank single-line label of at most ` + + `${MAX_DISPLAY_LABEL_LENGTH} characters, and must not contain '/'`; + } + } + return null; +} + const REASONING_SUMMARY_DELIVERY_SET = new Set(REASONING_SUMMARY_DELIVERY_VALUES); export function reasoningSummaryDeliveryRecordConfigError( diff --git a/src/server/management/provider-capability-config.ts b/src/server/management/provider-capability-config.ts index ce58966df3..3031ff3407 100644 --- a/src/server/management/provider-capability-config.ts +++ b/src/server/management/provider-capability-config.ts @@ -1,4 +1,4 @@ -import { booleanRecordConfigError } from "../../config"; +import { booleanRecordConfigError, displayLabelRecordConfigError } from "../../config"; import type { OcxConfig } from "../../types"; /** @@ -17,6 +17,25 @@ export function providerServiceTierConfigError(name: unknown, provider: unknown) return error ? `provider ${name} ${error}` : null; } +/** + * Reject an invalid `modelDisplayNames` edit at the API instead of letting it land. + * + * The load path drops a bad entry and carries on, so without this an operator could + * PATCH a slash-bearing label, get 200, and then find the label silently absent — + * the config would be valid and the request would look accepted. Failing the write + * is what makes the two behaviours coherent. + */ +export function providerDisplayNamesConfigError(name: unknown, provider: unknown): string | null { + if (typeof name !== "string" || !provider || typeof provider !== "object" || Array.isArray(provider)) { + return null; + } + const error = displayLabelRecordConfigError( + (provider as { modelDisplayNames?: unknown }).modelDisplayNames, + "modelDisplayNames", + ); + return error ? `provider ${name} ${error}` : null; +} + function publicServiceTierRecord(value: unknown): Record | undefined { if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; const entries = Object.entries(value).filter(([model, supported]) => diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 47909e50f0..fa448cef1e 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -75,7 +75,8 @@ import { filterRequestLogs, getRequestLogEntries, type RequestLogEntry } from ". import { estimateComboCost, estimateRequestCost, normalizeCostTokens, tokensPerSecond } from "../../usage/cost"; import type { PersistedUsageAttempt } from "../../usage/log"; import { isAllowedRequestOrigin, jsonResponse, providerManagementConfigError, publicProviderBaseUrl, safeConfigDTO } from "../auth-cors"; -import { providerServiceTierConfigError } from "./provider-capability-config"; +import { MAX_DISPLAY_LABEL_LENGTH, isValidDisplayLabel } from "../../codex/catalog/display-labels"; +import { providerDisplayNamesConfigError, providerServiceTierConfigError } from "./provider-capability-config"; import { applySystemEnvToggle } from "../system-env"; import { LOCAL_PROVIDER_RELOAD_NAME_HEADER, @@ -266,6 +267,34 @@ function applyProviderPatchFields( } touched = true; } + if (Object.hasOwn(rawBody, "modelDisplayNames")) { + const value = rawBody.modelDisplayNames; + if (value === null) { + delete next.modelDisplayNames; + } else { + if (!isPlainRecord(value)) return { error: "modelDisplayNames must be a plain object or null" }; + const labels: Record = { ...(next.modelDisplayNames ?? {}) }; + for (const [model, label] of Object.entries(value)) { + if (!model.trim()) return { error: "modelDisplayNames keys must be nonblank model ids" }; + // Per-key null clears one label, matching `modelContextWindows`, so an operator can + // take a single label back off without resubmitting the rest of the map. + if (label === null) { + delete labels[model.trim()]; + continue; + } + if (!isValidDisplayLabel(label)) { + return { + error: "modelDisplayNames values must be a nonblank single-line label of at most " + + `${MAX_DISPLAY_LABEL_LENGTH} characters without '/', or null`, + }; + } + labels[model.trim()] = label.trim(); + } + if (Object.keys(labels).length > 0) next.modelDisplayNames = labels; + else delete next.modelDisplayNames; + } + touched = true; + } if (Object.hasOwn(rawBody, "modelSupportsServiceTier")) { const value = rawBody.modelSupportsServiceTier; if (value === null) { @@ -500,6 +529,8 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise; /** Exact upstream model ids that override the provider-level service-tier capability. */ modelSupportsServiceTier?: Record; /** diff --git a/tests/catalog-operator-display-labels-convergence.test.ts b/tests/catalog-operator-display-labels-convergence.test.ts new file mode 100644 index 0000000000..3ad8cb32cd --- /dev/null +++ b/tests/catalog-operator-display-labels-convergence.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, test } from "bun:test"; + +import { applyOperatorDisplayLabels } from "../src/codex/catalog/display-labels"; +import { CODEX_CUSTOM_MODEL_CATALOG_KIND } from "../src/codex/catalog/parsing"; +import { buildCatalogEntries } from "../src/codex/catalog/sync"; +import { validateConfigCandidate } from "../src/config"; +import type { CatalogModel } from "../src/codex/catalog/parsing"; +import type { OcxConfig } from "../src/types/config"; + +/** + * End-to-end cover for #2201: an operator label loaded through the *real* config + * validator must reach `entry.display_name`, and must reach nothing else. + * + * The unit file beside this one pins `resolveModelDisplayLabel` in isolation. This + * one exists because that is not the claim worth making — the claim is that the + * value survives `validateConfigCandidate`, the label pass, and catalog assembly, + * and that routing identity is byte-identical on the way through. Asserting the + * resolver alone would pass even if the label never reached the picker. + */ + +const NATIVE_SLUG = "gpt-5.5"; +const NVIDIA_ID = "deepseek-ai/deepseek-v4-flash-0731"; +/** What #2201 reports: the routed slug is what the operator sees today. */ +const ROUTED_SLUG = "nvidia/deepseek-ai-deepseek-v4-flash-0731"; + +function template(): Record { + return { + slug: NATIVE_SLUG, + display_name: NATIVE_SLUG, + description: "Native GPT model", + priority: 1, + visibility: "list", + base_instructions: "You are Codex, an agent based on GPT-5.", + tool_mode: "code", + supported_reasoning_levels: [{ effort: "low" }, { effort: "high" }], + }; +} + +/** Load through the real validator, so a test can never assert on a shape the loader would reject. */ +function loadConfig(providerConfig: Record): OcxConfig { + const result = validateConfigCandidate({ + defaultProvider: "nvidia", + providers: { nvidia: { adapter: "openai", baseUrl: "https://nim.example/v1", ...providerConfig } }, + }); + if (!result.ok) throw new Error(`fixture rejected by the config validator: ${result.error}`); + return result.config; +} + +function entriesFor(models: CatalogModel[], config: OcxConfig): Record> { + const labeled = applyOperatorDisplayLabels(models, config); + const built = buildCatalogEntries( + template() as unknown as Parameters[0], + [NATIVE_SLUG], + labeled as unknown as Parameters[2], + [], + false, + ) as unknown as Record[]; + return Object.fromEntries(built.map(entry => [String(entry.slug), entry])); +} + +const discovered = (): CatalogModel[] => [ + { provider: "nvidia", id: NVIDIA_ID, owned_by: "nvidia" } as CatalogModel, +]; + +describe("operator display labels through catalog assembly", () => { + test("today's behaviour, so the fix is measured against something", () => { + const entries = entriesFor(discovered(), loadConfig({})); + // This is the defect #2201 describes: the label IS the routed slug. + expect(entries[ROUTED_SLUG]?.display_name).toBe(ROUTED_SLUG); + }); + + test("an operator label reaches display_name and leaves routing identity alone", () => { + const config = loadConfig({ modelDisplayNames: { [NVIDIA_ID]: "DeepSeek V4 Flash" } }); + const entries = entriesFor(discovered(), config); + const row = entries[ROUTED_SLUG]; + + expect(row?.display_name).toBe("DeepSeek V4 Flash"); + // Routing identity, unchanged: the slug is still the routed slug and is still + // the key the entry is found under, so cost lookup, disabled-model lookup and a + // saved selection all continue to resolve against the same string. + expect(row?.slug).toBe(ROUTED_SLUG); + expect(Object.keys(entries).sort()).toEqual([NATIVE_SLUG, ROUTED_SLUG].sort()); + // The native row is not a CatalogModel, so an upstream marketing name is untouched. + expect(entries[NATIVE_SLUG]?.display_name).toBe(NATIVE_SLUG); + }); + + test("every field except display_name is byte-identical to the unlabelled build", () => { + const before = entriesFor(discovered(), loadConfig({}))[ROUTED_SLUG] ?? {}; + const after = entriesFor( + discovered(), + loadConfig({ modelDisplayNames: { [NVIDIA_ID]: "DeepSeek V4 Flash" } }), + )[ROUTED_SLUG] ?? {}; + + expect(Object.keys(after).sort()).toEqual(Object.keys(before).sort()); + const differing = Object.keys(after).filter( + key => JSON.stringify(after[key]) !== JSON.stringify(before[key]), + ); + expect(differing).toEqual(["display_name"]); + }); + + test("removing the label deterministically restores the derived label", () => { + const labelled = entriesFor( + discovered(), + loadConfig({ modelDisplayNames: { [NVIDIA_ID]: "DeepSeek V4 Flash" } }), + ); + expect(labelled[ROUTED_SLUG]?.display_name).toBe("DeepSeek V4 Flash"); + + // Both documented ways to take a label back off land on the same result. + for (const cleared of [{}, { modelDisplayNames: {} }, { modelDisplayNames: { [NVIDIA_ID]: null } }]) { + const entries = entriesFor(discovered(), loadConfig(cleared)); + expect(entries[ROUTED_SLUG]?.display_name).toBe(ROUTED_SLUG); + } + }); + + test("the key space is the native model id, not the routed slug", () => { + const config = loadConfig({ modelDisplayNames: { [ROUTED_SLUG]: "Wrong Key Space" } }); + const entries = entriesFor(discovered(), config); + // A miss must be inert, not a partial relabel. + expect(entries[ROUTED_SLUG]?.display_name).toBe(ROUTED_SLUG); + }); + + test("a label the loader drops cannot reach the catalog", () => { + // `bad/label` would read as a routed slug, so the schema drops it on load and + // the picker keeps the derived label rather than showing a second slug. + const config = loadConfig({ modelDisplayNames: { [NVIDIA_ID]: "bad/label" } }); + expect(config.providers.nvidia?.modelDisplayNames).toBeUndefined(); + expect(entriesFor(discovered(), config)[ROUTED_SLUG]?.display_name).toBe(ROUTED_SLUG); + }); + + test("one unusable label does not cost the operator their other labels", () => { + const other: CatalogModel = { provider: "nvidia", id: "moonshotai/kimi-k3", owned_by: "nvidia" } as CatalogModel; + const config = loadConfig({ + modelDisplayNames: { [NVIDIA_ID]: "bad/label", "moonshotai/kimi-k3": "Kimi K3" }, + }); + const entries = entriesFor([...discovered(), other], config); + + expect(entries["nvidia/moonshotai-kimi-k3"]?.display_name).toBe("Kimi K3"); + expect(entries[ROUTED_SLUG]?.display_name).toBe(ROUTED_SLUG); + }); + + test("an existing custom-model label survives, which is #2201's migration rule", () => { + const custom: CatalogModel = { + provider: "nvidia", + id: NVIDIA_ID, + owned_by: "nvidia", + displayName: "My Existing Custom Label", + catalogKind: CODEX_CUSTOM_MODEL_CATALOG_KIND, + } as CatalogModel; + const config = loadConfig({ modelDisplayNames: { [NVIDIA_ID]: "Provider Map Value" } }); + + const entries = entriesFor([custom], config); + expect(entries[ROUTED_SLUG]?.display_name).toBe("My Existing Custom Label"); + expect(entries[ROUTED_SLUG]?.opencodex_catalog_kind).toBe(CODEX_CUSTOM_MODEL_CATALOG_KIND); + }); +}); diff --git a/tests/catalog-operator-display-labels.test.ts b/tests/catalog-operator-display-labels.test.ts new file mode 100644 index 0000000000..02c4681d4f --- /dev/null +++ b/tests/catalog-operator-display-labels.test.ts @@ -0,0 +1,305 @@ +import { describe, expect, test } from "bun:test"; + +import { + applyOperatorDisplayLabels, + isValidDisplayLabel, + MAX_DISPLAY_LABEL_LENGTH, + resolveModelDisplayLabel, +} from "../src/codex/catalog/display-labels"; +import { COMBO_NAMESPACE } from "../src/combos/types"; +import { + CODEX_CUSTOM_MODEL_CATALOG_KIND, + CODEX_PROVIDER_MODEL_CATALOG_KIND, +} from "../src/codex/catalog/parsing"; +import type { CatalogModel } from "../src/codex/catalog/parsing"; +import { MAX_MODEL_DISPLAY_NAMES, validateConfigCandidate } from "../src/config"; +import { providerDisplayNamesConfigError } from "../src/server/management/provider-capability-config"; +import type { OcxConfig } from "../src/types/config"; + +/** The reported case: a discovered NVIDIA NIM row whose label is its routed slug. */ +const NVIDIA: CatalogModel = { + provider: "nvidia", + id: "deepseek-ai/deepseek-v4-flash-0731", + owned_by: "nvidia", +}; + +function configWith(providers: Record): OcxConfig { + return { providers } as unknown as OcxConfig; +} + +describe("isValidDisplayLabel", () => { + test("accepts a normal single-line label", () => { + expect(isValidDisplayLabel("DeepSeek V4 Flash")).toBe(true); + }); + + test("rejects a label containing a slash, which would read as a routed slug", () => { + expect(isValidDisplayLabel("nvidia/deepseek")).toBe(false); + }); + + test("rejects blank, non-string and over-long labels", () => { + expect(isValidDisplayLabel(" ")).toBe(false); + expect(isValidDisplayLabel(undefined)).toBe(false); + expect(isValidDisplayLabel(42)).toBe(false); + expect(isValidDisplayLabel("x".repeat(MAX_DISPLAY_LABEL_LENGTH + 1))).toBe(false); + }); + + test("accepts a label exactly at the bound", () => { + expect(isValidDisplayLabel("x".repeat(MAX_DISPLAY_LABEL_LENGTH))).toBe(true); + }); + + test("rejects a label carrying a control character", () => { + expect(isValidDisplayLabel("DeepSeek\u0007V4")).toBe(false); + }); + + test("ordinary ASCII whitespace at the edges is normalised, not rejected", () => { + // Deliberately forgiving, and only for this class: a stray space, tab, newline + // or CR in a hand-edited config is plausible slop, and on the load path a + // rejection means silently losing the operator's label. `"Label\n"` therefore + // stores as `"Label"` rather than disappearing. + for (const edge of ["\u0020", "\u0009", "\u000a", "\u000d"]) { + expect(isValidDisplayLabel(`Label${edge}`)).toBe(true); + expect(isValidDisplayLabel(`${edge}Label`)).toBe(true); + expect(resolveModelDisplayLabel( + configWith({ nvidia: { modelDisplayNames: { [NVIDIA.id]: `Label${edge}` } } }), + NVIDIA, + )).toBe("Label"); + } + // Mid-label, the same characters are rejected: a label is single-line. + expect(isValidDisplayLabel("La\u000abel")).toBe(false); + expect(isValidDisplayLabel("La\u0009bel")).toBe(false); + }); + + test("no control character reaches a stored label — C0, DEL, C1, and the line separators", () => { + // The class was originally C0 + DEL only. C1 (U+0080-U+009F) and U+2028/U+2029 + // leaked: `LabelMore` and `LabelMore` were reported valid and + // stored verbatim, and both are line breaks, so the "single-line" guarantee did + // not hold. + // + // The invariant is about what is STORED, not what is rejected — an edge TAB or + // newline is accepted and normalised away, which is deliberate. So this walks the + // ranges and, for every candidate the validator accepts, checks the value that + // actually lands on the row. Enumerated rather than sampled so a future narrowing + // of the regex cannot slip past this test. + const CONTROL = /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/; + const leaked: string[] = []; + for (const code of [ + ...Array.from({ length: 0x20 }, (_, i) => i), // C0 + 0x7f, // DEL + ...Array.from({ length: 0x20 }, (_, i) => 0x80 + i), // C1 + 0x2028, 0x2029, // LINE / PARAGRAPH SEPARATOR + ]) { + const ch = String.fromCharCode(code); + const hex = `U+${code.toString(16).padStart(4, "0").toUpperCase()}`; + for (const candidate of [`La${ch}bel`, `Label${ch}`, `${ch}Label`, ch]) { + if (!isValidDisplayLabel(candidate)) continue; + const stored = resolveModelDisplayLabel( + configWith({ nvidia: { modelDisplayNames: { [NVIDIA.id]: candidate } } }), + NVIDIA, + ); + if (stored !== undefined && CONTROL.test(stored)) { + leaked.push(`${hex} stored as ${JSON.stringify(stored)}`); + } + } + } + expect(leaked).toEqual([]); + }); + + test("a C1 control or line separator is rejected outright, not normalised", () => { + // The distinction from the whitespace class above: these are never plausible slop + // in a display label, and U+2028/U+2029 are in JS's whitespace set, so trimming + // first would have quietly accepted a trailing one. + for (const code of [0x85, 0x80, 0x9f, 0x2028, 0x2029]) { + const ch = String.fromCharCode(code); + expect(isValidDisplayLabel(`La${ch}bel`)).toBe(false); + expect(isValidDisplayLabel(`Label${ch}`)).toBe(false); + expect(isValidDisplayLabel(`${ch}Label`)).toBe(false); + } + }); + + test("the label characters that must keep working are not caught by that class", () => { + // The C1 range sits just above Latin-1 punctuation, so an over-wide regex would + // quietly break ordinary labels. These are the neighbours worth pinning. + for (const label of ["DeepSeek V4 Flash", "Qwen3-Max", "Llama_3.1", "GLM 4.6 (free)", + "Café Model", "モデル", "Ω-preview", "model@v2", "a^b", "x~y"]) { + expect(isValidDisplayLabel(label)).toBe(true); + } + }); +}); + +describe("resolveModelDisplayLabel precedence", () => { + test("an operator override wins and is trimmed", () => { + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": " DeepSeek V4 Flash " } }, + }); + expect(resolveModelDisplayLabel(config, NVIDIA)).toBe("DeepSeek V4 Flash"); + }); + + test("discovery metadata is used when no override exists", () => { + const config = configWith({ nvidia: {} }); + const discovered = { ...NVIDIA, displayName: "DeepSeek V4 Flash (upstream)" }; + expect(resolveModelDisplayLabel(config, discovered)).toBe("DeepSeek V4 Flash (upstream)"); + }); + + test("an operator override outranks discovery metadata", () => { + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "Operator Label" } }, + }); + const discovered = { ...NVIDIA, displayName: "Upstream Label" }; + expect(resolveModelDisplayLabel(config, discovered)).toBe("Operator Label"); + }); + + test("an invalid override falls through rather than taking effect", () => { + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "bad/label" } }, + }); + const discovered = { ...NVIDIA, displayName: "Upstream Label" }; + expect(resolveModelDisplayLabel(config, discovered)).toBe("Upstream Label"); + expect(resolveModelDisplayLabel(config, NVIDIA)).toBeUndefined(); + }); + + test("no override and no metadata leaves the caller on its derived slug", () => { + expect(resolveModelDisplayLabel(configWith({ nvidia: {} }), NVIDIA)).toBeUndefined(); + expect(resolveModelDisplayLabel(configWith({}), NVIDIA)).toBeUndefined(); + }); + + test("an override keyed on the routed slug rather than the native id does not apply", () => { + // The key space is the native model id, the same as `modelAdapters`. + const config = configWith({ + nvidia: { modelDisplayNames: { "nvidia/deepseek-ai-deepseek-v4-flash-0731": "Wrong Key" } }, + }); + expect(resolveModelDisplayLabel(config, NVIDIA)).toBeUndefined(); + }); + + test("an explicit custom-model row keeps the label the operator already typed", () => { + // #2201's migration rule. Matching on catalogKind rather than provider name is + // what makes this hold: a custom model shares its provider with the discovered + // rows this feature exists to relabel, so the provider name cannot separate them. + const custom = { + provider: "nvidia", + id: "deepseek-ai/deepseek-v4-flash-0731", + displayName: "My Existing Custom Label", + catalogKind: CODEX_CUSTOM_MODEL_CATALOG_KIND, + } as CatalogModel; + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "Provider Map Value" } }, + }); + expect(resolveModelDisplayLabel(config, custom)).toBe("My Existing Custom Label"); + }); + + test("a discovered row on the same provider is still relabelled", () => { + // The guard above must not be so broad that it disables the feature. + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "Provider Map Value" } }, + }); + expect(resolveModelDisplayLabel(config, NVIDIA)).toBe("Provider Map Value"); + expect(resolveModelDisplayLabel(config, { ...NVIDIA, catalogKind: CODEX_PROVIDER_MODEL_CATALOG_KIND })) + .toBe("Provider Map Value"); + }); + + test("a combo row keeps its own label and cannot be relabelled from provider config", () => { + const combo: CatalogModel = { + provider: COMBO_NAMESPACE, + id: "my-combo", + displayName: "My Combo", + }; + const config = configWith({ + [COMBO_NAMESPACE]: { modelDisplayNames: { "my-combo": "Hijacked" } }, + }); + expect(resolveModelDisplayLabel(config, combo)).toBe("My Combo"); + }); +}); + +describe("applyOperatorDisplayLabels", () => { + test("labels only the matching row and never mutates the input", () => { + const other: CatalogModel = { provider: "nvidia", id: "moonshotai/kimi-k3" }; + const models = [NVIDIA, other]; + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "DeepSeek V4 Flash" } }, + }); + + const labeled = applyOperatorDisplayLabels(models, config); + + expect(labeled[0]?.displayName).toBe("DeepSeek V4 Flash"); + expect(labeled[1]?.displayName).toBeUndefined(); + expect(NVIDIA.displayName).toBeUndefined(); + expect(models[0]).toBe(NVIDIA); + }); + + test("routing identity is untouched by relabelling", () => { + const config = configWith({ + nvidia: { modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "DeepSeek V4 Flash" } }, + }); + const [labeled] = applyOperatorDisplayLabels([NVIDIA], config); + expect(labeled?.provider).toBe("nvidia"); + expect(labeled?.id).toBe("deepseek-ai/deepseek-v4-flash-0731"); + expect(labeled?.owned_by).toBe("nvidia"); + }); + + test("returns the identical array when nothing resolves", () => { + const models = [NVIDIA]; + expect(applyOperatorDisplayLabels(models, configWith({ nvidia: {} }))).toBe(models); + }); +}); + +describe("modelDisplayNames config contract", () => { + const load = (modelDisplayNames: unknown) => + validateConfigCandidate({ + defaultProvider: "nvidia", + providers: { nvidia: { adapter: "openai", baseUrl: "https://nim.example/v1", modelDisplayNames } }, + }); + const kept = (modelDisplayNames: unknown) => { + const result = load(modelDisplayNames); + if (!result.ok) throw new Error(`unexpectedly rejected: ${result.error}`); + return result.config.providers.nvidia?.modelDisplayNames; + }; + const writeError = (modelDisplayNames: unknown) => + providerDisplayNamesConfigError("nvidia", { + adapter: "openai", baseUrl: "https://nim.example/v1", modelDisplayNames, + }); + + // The two paths answer different questions, so they are allowed to differ: + // "can this file still be served?" versus "is this a valid edit?". + test("load keeps a well-formed map, trimming the label", () => { + expect(kept({ m: " DeepSeek V4 Flash " })).toEqual({ m: "DeepSeek V4 Flash" }); + expect(writeError({ m: "DeepSeek V4 Flash" })).toBeNull(); + }); + + test("load drops an unusable entry instead of failing the whole config", () => { + for (const bad of [["array"], { m: "bad/label" }, { m: 42 }, { "": "blank key" }, "string"]) { + expect(load(bad).ok).toBe(true); + expect(kept(bad)).toBeUndefined(); + } + }); + + test("a write of the same values is refused, so a bad label never lands silently", () => { + expect(writeError(["array"])).toMatch(/must be a plain object/); + expect(writeError({ m: "bad/label" })).toMatch(/must not contain '\/'/); + expect(writeError({ m: 42 })).toMatch(/must be a string/); + expect(writeError({ "": "blank key" })).toMatch(/nonblank model ids/); + }); + + test("one bad neighbour does not evict the operator's other labels", () => { + expect(kept({ bad: "a/b", good: "Kimi K3" })).toEqual({ good: "Kimi K3" }); + }); + + test("null is an explicit clear on both paths", () => { + expect(kept({ m: null })).toBeUndefined(); + expect(writeError({ m: null })).toBeNull(); + }); + + test("the map is bounded, and the bound is a write error rather than silent truncation", () => { + const oversized = Object.fromEntries( + Array.from({ length: MAX_MODEL_DISPLAY_NAMES + 1 }, (_, i) => [`m${i}`, `L${i}`]), + ); + expect(Object.keys(kept(oversized) ?? {}).length).toBe(MAX_MODEL_DISPLAY_NAMES); + expect(writeError(oversized)).toMatch(/at most 512 entries/); + }); + + test("a prototype key is not a usable label source", () => { + // `{}.constructor` is a function, not a string, so the lookup in + // resolveModelDisplayLabel cannot promote it to a label. + const config = configWith({ nvidia: { modelDisplayNames: {} } }); + expect(resolveModelDisplayLabel(config, { provider: "nvidia", id: "constructor" } as CatalogModel)) + .toBeUndefined(); + }); +}); diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 7aaaba950a..9b3a3f359c 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -756,6 +756,144 @@ describe("provider management validation", () => { }); }); + // #2201: `ProviderPayload` has no member for modelDisplayNames either, and that PR leaves + // the dashboard editor to a follow-up, so the add/edit form structurally cannot round-trip + // the field. Absence in a POST therefore means "not carried", never "the operator deleted + // it" — without preservation, saving any unrelated provider setting wipes every label. + describe("provider POST overwrite preserves operator display labels (#2201)", () => { + const LABELS = { "deepseek-ai/deepseek-v4-flash-0731": "DeepSeek V4 Flash" }; + + async function seedProvider(url: URL, extra: Record): Promise { + return fetch(new URL("/api/providers", url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "labels", + provider: { adapter: "openai-chat", baseUrl: "https://nim.example.test/v1", apiKey: "k", ...extra }, + }), + }); + } + + function freshHome(): void { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + } + + test("an omitted modelDisplayNames keeps the operator's map", async () => { + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { modelDisplayNames: LABELS })).status).toBe(200); + expect((await seedProvider(server.url, {})).status).toBe(200); + + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(LABELS); + } finally { + await server.stop(true); + } + }); + + test("a submitted modelDisplayNames updates that key and keeps the others", async () => { + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { modelDisplayNames: LABELS })).status).toBe(200); + expect((await seedProvider(server.url, { modelDisplayNames: { "moonshotai/kimi-k3": "Kimi K3" } })).status).toBe(200); + + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual({ + ...LABELS, + "moonshotai/kimi-k3": "Kimi K3", + }); + } finally { + await server.stop(true); + } + }); + + test("a provider that never had labels does not gain the key", async () => { + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, {})).status).toBe(200); + expect(loadConfig().providers.labels?.modelDisplayNames).toBeUndefined(); + } finally { + await server.stop(true); + } + }); + + test("an invalid label is a 400 rather than a silent drop", async () => { + freshHome(); + const server = startServer(0); + try { + // A slash-bearing label reads as a routed slug. The load path would drop it, so + // accepting the write would return 200 for a label that is then simply absent. + const bad = await seedProvider(server.url, { + modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": "bad/label" }, + }); + expect(bad.status).toBe(400); + expect(loadConfig().providers.labels).toBeUndefined(); + } finally { + await server.stop(true); + } + }); + + test("PATCH clears one label with a per-key null, and the whole map with null", async () => { + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { + modelDisplayNames: { ...LABELS, "moonshotai/kimi-k3": "Kimi K3" }, + })).status).toBe(200); + + const one = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelDisplayNames: { "moonshotai/kimi-k3": null } }), + }); + expect(one.status).toBe(200); + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(LABELS); + + const all = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelDisplayNames: null }), + }); + expect(all.status).toBe(200); + expect(loadConfig().providers.labels?.modelDisplayNames).toBeUndefined(); + } finally { + await server.stop(true); + } + }); + + test("a control character PATCH is refused, including the ones trim would hide", async () => { + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { modelDisplayNames: LABELS })).status).toBe(200); + + const C = (code: number) => String.fromCharCode(code); + // U+0085 is NEL and U+2028 a line separator: both are line breaks that the + // original C0-only class let through into a stored picker label. + for (const label of [`Label${C(0x85)}More`, `Label${C(0x2028)}More`, `Label${C(0x2028)}`]) { + const patch = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelDisplayNames: { "deepseek-ai/deepseek-v4-flash-0731": label } }), + }); + expect(patch.status).toBe(400); + // Assert on the reason, not just the status. Before modelDisplayNames was a + // recognised PATCH field this same body returned 400 "no recognized fields to + // update", so a status-only assertion passed without the label rule running at all. + expect((await patch.json()).error).toMatch(/modelDisplayNames values must be/); + } + // The seeded map is untouched by the refused writes. + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(LABELS); + } finally { + await server.stop(true); + } + }); + }); + test("provider management accepts modelCosts on the canonical openai provider", async () => { if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true });