diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts new file mode 100644 index 0000000000..c978b95333 --- /dev/null +++ b/src/codex/catalog/display-labels.ts @@ -0,0 +1,131 @@ +/** + * 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 — the post-gather boundary. + * + * Called at exactly two places, one per gather entry point, so every surface that + * reads live routed models is covered: + * + * - `fetchAllModels` (src/server/management/shared.ts) — `/v1/models`, + * `/api/models` via `listManagementModelRows`, and client exports via + * `loadExportModels` all funnel through it; + * - `prepareCatalog` (src/codex/convergence.ts) — reaches the gather by the + * separate catalog-gather entry point, which never passes through the above. + * + * Anything reading models *without* going through one of those two is a surface + * that will emit the routed slug as its label. That is not hypothetical: labelling + * only the convergence call site is what left the live `/v1/models` route wrong + * while the on-disk catalog was right. + * + * Both calls sit after the gather rather than inside it, because the gather is + * TTL-cached on provider identity and a label baked into a cached entry would + * outlive an operator's edit to it. + * + * Idempotent, so the two boundaries overlapping is harmless: `resolveModelDisplayLabel` + * reads the config and the row's own kind, never a previously applied result. + * + * 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 b338aa9d3b..7640d2058c 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -77,6 +77,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"; @@ -241,7 +242,17 @@ 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. + // + // One of the two post-gather label boundaries; the other is `fetchAllModels`, + // which covers every server surface. This path needs its own because it reaches + // the gather through `gatherRoutedModelsForCatalogGather`, which never passes + // through `fetchAllModels`. See applyOperatorDisplayLabels for why both sit + // after the gather rather than inside it. + 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 1308b3a64b..42c9fa3c44 100644 --- a/src/config.ts +++ b/src/config.ts @@ -7,6 +7,8 @@ import { isValidProviderName, hasOwnProvider } from "./config/provider-name"; import { apiKeyTransportConfigError, booleanRecordConfigError, + displayLabelRecordConfigError, + MAX_MODEL_DISPLAY_NAMES, modelAdapterRecordConfigError, nonBlankStringArrayConfigError, normalizeNonBlankStringArray, @@ -40,6 +42,7 @@ import { MAIN_CODEX_ACCOUNT_NAMESPACE_TARGET, } from "./codex/account-namespace-match"; import { isCodexAccountPriorityKey } from "./codex/account-priority"; +import { isValidDisplayLabel } from "./codex/catalog/display-labels"; import { UPSTREAM_HOST_CIRCUIT_MAX_THRESHOLD } from "./codex/upstream-host-health"; import { adoptCustomModelCatalogMigration, @@ -503,6 +506,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(), @@ -536,6 +558,8 @@ export { isValidProviderName, hasOwnProvider } from "./config/provider-name"; export { apiKeyTransportConfigError, booleanRecordConfigError, + displayLabelRecordConfigError, + MAX_MODEL_DISPLAY_NAMES, modelAdapterRecordConfigError, nonBlankStringArrayConfigError, normalizeNonBlankStringArray, diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index 8a068d271b..e022363d28 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -1,3 +1,4 @@ +import { MAX_DISPLAY_LABEL_LENGTH, isValidDisplayLabel } from "../codex/catalog/display-labels"; import { isCanonicalOpenAiForwardProvider } from "../providers/openai-tiers"; import { modelRecordValue } from "../reasoning-effort"; import { @@ -123,6 +124,83 @@ 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 { + // `null` clears the whole map, the same way a per-key `null` clears one label and the same + // way the load schema treats it. Rejecting it here made the documented way to remove every + // label a 400 on POST while the loader accepted it — the two boundaries disagreed about + // what the operator had asked for. + if (value === undefined || value === null) 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`; + } + // Keys are stored trimmed, so two submitted keys can collapse into one stored entry. + // Counting before that happens means the cap is enforced against a number the store + // never sees, and the later of the two labels silently wins over the earlier — the + // operator gets a 200 for an instruction that was self-contradictory. + const seen = new Set(); + for (const [key, label] of entries) { + const id = key.trim(); + if (!id) return `${field} keys must be nonblank model ids`; + if (seen.has(id)) return `${field} must not set the same model id twice (${id})`; + seen.add(id); + 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; +} + +/** + * Normalize a submitted label map to the shape that is actually persisted: keys and labels + * trimmed, non-string values carried through as tombstones for the caller to apply. + * + * PATCH already stored `model.trim()` while POST stored the key verbatim, so the same id + * submitted through the two routes produced two different stored keys for one model. This + * is the single definition of "what does this entry become", so the cap and the label rules + * can be checked against the map that will exist rather than the one that was sent. + */ +export function normalizeDisplayLabelRecord(value: object): Record { + // Null prototype, matching `modelAutoCompactTokenLimits`: `JSON.parse` gives a + // `"__proto__"` key as an *own* property, but assigning it on a `{}` literal runs the + // setter and creates nothing — so the entry would be counted by the validation above and + // then vanish before the store, which is the precise mismatch this change exists to close. + const out = Object.create(null) as Record; + for (const [key, label] of Object.entries(value)) { + const id = key.trim(); + if (!id) continue; + out[id] = typeof label === "string" ? label.trim() : null; + } + return out; +} + export function reasoningSummaryDeliveryRecordConfigError( value: unknown, supportsReasoningSummaries: unknown, diff --git a/src/server/management/provider-capability-config.ts b/src/server/management/provider-capability-config.ts index a3b27a1602..5b9774682c 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/provider-validation"; +import { booleanRecordConfigError, displayLabelRecordConfigError } from "../../config/provider-validation"; 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 d542306885..c1dbf8e8f7 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -76,7 +76,9 @@ 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 { normalizeDisplayLabelRecord } from "../../config/provider-validation"; import { applySystemEnvToggle } from "../system-env"; import { LOCAL_PROVIDER_RELOAD_NAME_HEADER, @@ -290,6 +292,45 @@ 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 = Object.assign( + Object.create(null) as Record, + next.modelDisplayNames ?? {}, + ); + // Collisions have to be caught against the *submitted* fragment. By the time the + // merged map is validated the duplicate has already collapsed into one key, so the + // later label wins silently and the check downstream has nothing left to see. + const submittedIds = new Set(); + for (const [model, label] of Object.entries(value)) { + if (!model.trim()) return { error: "modelDisplayNames keys must be nonblank model ids" }; + if (submittedIds.has(model.trim())) { + return { error: `modelDisplayNames must not set the same model id twice (${model.trim()})` }; + } + submittedIds.add(model.trim()); + // 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) { @@ -547,6 +588,8 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise = Object.assign( + Object.create(null) as Record, + existing?.modelDisplayNames ?? {}, + ); + for (const [model, label] of Object.entries(submitted ?? {})) { + if (label === null) delete merged[model]; + else merged[model] = label; + } + if (Object.keys(merged).length > 0) prov.modelDisplayNames = merged; + else delete prov.modelDisplayNames; + } + // Validate what will actually be persisted, not what was sent. This is the only check + // that sees the merged map, and it is the one the loader's salvage would otherwise be + // left to clean up after the write already reported success. + const mergedDisplayNamesError = providerDisplayNamesConfigError(name, prov); + if (mergedDisplayNamesError) return jsonResponse({ error: mergedDisplayNamesError }, 400); config.providers[name] = stripRegistryOnlyStaticHeaders(name, prov); if (body.setDefault === true) config.defaultProvider = name; save(config); @@ -722,6 +804,11 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { * canonical, TTL-cached `gatherRoutedModels` (single source of truth) — so the GUI/codex endpoints * share the same fetch, the same per-provider cache (dedups Codex's frequent /v1/models polling), * and the same stale fallback when a provider blips, instead of a parallel uncached copy. + * + * This is also the post-gather display-label boundary for every server surface (#2201). + * Every consumer that reads live routed models goes through here — `/v1/models` in + * src/server/index.ts, `/api/models` via `listManagementModelRows`, and client exports + * via `loadExportModels` — so applying labels at this one point covers all of them and + * cannot be forgotten by a fourth caller added later. Labelling each call site instead + * is what left `/v1/models` emitting the routed slug as `display_name` while the + * convergence path was already correct. + * + * Deliberately applied *after* the gather rather than inside it: `gatherRoutedModels` is + * TTL-cached on a key derived from provider identity, so a label baked into a cached + * entry would outlive an operator's edit to it. Display-only and idempotent, so a caller + * that labels again (convergence does, reaching the gather by a different entry point) + * gets the same result. */ export async function fetchAllModels(config: OcxConfig): Promise { const { gatherRoutedModels } = await import("../../codex/catalog"); - return gatherRoutedModels(config); + const { applyOperatorDisplayLabels } = await import("../../codex/catalog/display-labels"); + return applyOperatorDisplayLabels(await gatherRoutedModels(config), config); } export interface GrokCandidateModel { diff --git a/src/types/provider.ts b/src/types/provider.ts index b7ba042506..a4d9ce6655 100644 --- a/src/types/provider.ts +++ b/src/types/provider.ts @@ -209,6 +209,20 @@ export interface OcxProviderConfig { * An explicit config value always wins over the registry default. */ supportsServiceTier?: boolean; + /** + * Display-only labels for live-discovered models, keyed by the upstream native + * model id — the same key space as `modelAdapters`. + * + * A discovered row otherwise shows its routed slug, so an NVIDIA NIM row reads + * `nvidia/deepseek-ai-deepseek-v4-flash-0731`. This relabels the row only: the + * provider id, native model id, and routed slug are untouched, exactly as with + * `customModels[].displayName`. Labels are single-line, at most 128 characters, + * and may not contain `/` — a label with a slash would read as a routed slug. + * + * A model label is deliberately not a provider label; naming the provider is a + * separate field so the two never end up concatenated into one string. + */ + modelDisplayNames?: Record; /** 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-routes.test.ts b/tests/catalog-operator-display-labels-routes.test.ts new file mode 100644 index 0000000000..bed37a55f1 --- /dev/null +++ b/tests/catalog-operator-display-labels-routes.test.ts @@ -0,0 +1,206 @@ +/** + * #2201: operator display labels must reach every surface that lists routed models, + * not just the on-disk catalog. + * + * `applyOperatorDisplayLabels` was originally called only from `prepareCatalog`, so + * the live `GET /v1/models` route, `/api/models`, and client exports all kept + * emitting the routed slug as the label while the on-disk catalog was correct. A + * unit test that calls the helper by hand cannot see that gap — it proves the + * helper works, not that anything uses it. So these drive the real routes. + * + * Each surface is exercised twice, with and without the operator map, and the two + * results are compared field by field. That is what pins "display-only": rather + * than listing the fields that must not move — slug, provider, native id, + * ordering, spawn-candidate identity — the whole payload is required to be + * identical once the label field is normalised away. A change that shifted + * ordering, or renamed an id, would fail without anyone having predicted it. + */ + +import { afterEach, beforeEach, expect, setDefaultTimeout, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { loadConfig, saveConfig } from "../src/config"; +import { startServer } from "../src/server"; +import type { OcxConfig } from "../src/types"; +import { installIsolatedCodexHome, type IsolatedCodexHome } from "./helpers/isolated-codex-home"; + +// startServer plus two discovery GETs exceeds the default 5s budget under full-suite +// Windows load, same flake class as claude-models-discovery. +setDefaultTimeout(30_000); + +const LABEL = "Fast Draft"; +const LABELLED = "test-model"; +const PLAIN = "other-model"; + +let testDir = ""; +let previousHome: string | undefined; +let isolatedCodexHome: IsolatedCodexHome | null = null; + +beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + isolatedCodexHome = installIsolatedCodexHome("ocx-label-routes-"); + testDir = mkdtempSync(join(tmpdir(), "ocx-label-routes-")); + process.env.OPENCODEX_HOME = testDir; +}); + +afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + isolatedCodexHome?.restore(); + isolatedCodexHome = null; + if (testDir) rmSync(testDir, { recursive: true, force: true }); +}); + +/** Static models, so the surfaces under test never depend on a live provider fetch. */ +function config(modelDisplayNames?: Record): OcxConfig { + return { + port: 0, + defaultProvider: "mock", + openaiProviderTierVersion: 2, + providers: { + mock: { + adapter: "openai-chat", + baseUrl: "http://127.0.0.1:1/v1", + apiKey: "k", + allowPrivateNetwork: true, + liveModels: false, + models: [PLAIN, LABELLED], + ...(modelDisplayNames ? { modelDisplayNames } : {}), + }, + }, + } as unknown as OcxConfig; +} + +/** Run a surface against a config, from a clean load each time. */ +async function withConfig( + modelDisplayNames: Record | undefined, + read: (config: OcxConfig) => Promise, +): Promise { + saveConfig(config(modelDisplayNames)); + return read(loadConfig()); +} + +/** + * Drop a field everywhere it appears, so everything *else* can be compared. + * + * Removed rather than blanked: on the management and export rows the label field + * is optional and only present once a label resolves, so its *presence* is part + * of what changes. Blanking left the labelled run with an extra key and the + * comparison failed for the one reason it was meant to ignore. + */ +function withoutField(value: unknown, field: string): unknown { + if (Array.isArray(value)) return value.map(entry => withoutField(entry, field)); + if (value === null || typeof value !== "object") return value; + const out: Record = {}; + for (const [key, inner] of Object.entries(value as Record)) { + if (key === field) continue; + out[key] = withoutField(inner, field); + } + return out; +} + +// -- GET /v1/models, the live Codex catalog route -------------------------- + +async function codexCatalog(modelDisplayNames?: Record) { + saveConfig(config(modelDisplayNames)); + const server = startServer(0); + try { + const response = await fetch(new URL("/v1/models?client_version=0.50.0", server.url), { + headers: { authorization: "Bearer placeholder" }, + }); + expect(response.status).toBe(200); + const body = await response.json() as { models: { slug: string; display_name?: string }[] }; + return body.models; + } finally { + await server.stop(true); + } +} + +test("GET /v1/models emits the operator label as display_name", async () => { + const models = await codexCatalog({ [LABELLED]: LABEL }); + const labelled = models.find(m => m.slug === `mock/${LABELLED}`); + const plain = models.find(m => m.slug === `mock/${PLAIN}`); + + expect(labelled?.display_name).toBe(LABEL); + // The unmapped sibling keeps today's behaviour: the routed slug as its label. + expect(plain?.display_name).toBe(`mock/${PLAIN}`); + // The routing identity is untouched — the slug is what the client sends back. + expect(labelled?.slug).toBe(`mock/${LABELLED}`); +}); + +test("GET /v1/models changes nothing but the label", async () => { + const before = await codexCatalog(); + const after = await codexCatalog({ [LABELLED]: LABEL }); + + expect(before.find(m => m.slug === `mock/${LABELLED}`)?.display_name).toBe(`mock/${LABELLED}`); + expect(after.find(m => m.slug === `mock/${LABELLED}`)?.display_name).toBe(LABEL); + // Ordering included: the arrays are compared in order, so a label that moved a + // row would fail here even though no field changed. + expect(withoutField(after, "display_name")).toEqual(withoutField(before, "display_name")); +}); + +test("removing the label restores the derived one on the live route", async () => { + expect((await codexCatalog({ [LABELLED]: LABEL })) + .find(m => m.slug === `mock/${LABELLED}`)?.display_name).toBe(LABEL); + expect((await codexCatalog({})) + .find(m => m.slug === `mock/${LABELLED}`)?.display_name).toBe(`mock/${LABELLED}`); +}); + +// -- management rows and client exports ------------------------------------ + +test("/api/models rows carry the operator label, and nothing else moves", async () => { + const read = async (cfg: OcxConfig) => { + const { listManagementModelRows } = await import("../src/server/management/model-rows"); + return listManagementModelRows(cfg); + }; + const before = await withConfig(undefined, read); + const after = await withConfig({ [LABELLED]: LABEL }, read); + + const row = (rows: Awaited>, id: string) => + rows.find(r => r.provider === "mock" && r.id === id) as Record | undefined; + + expect(row(after, LABELLED)?.displayName).toBe(LABEL); + expect(row(before, LABELLED)?.displayName).toBeUndefined(); + expect(row(after, PLAIN)?.displayName).toBeUndefined(); + // `namespaced` is the routing identity the GUI writes back into disabledModels. + expect(row(after, LABELLED)?.namespaced).toBe(`mock/${LABELLED}`); + expect(withoutField(after, "displayName")).toEqual(withoutField(before, "displayName")); +}); + +test("client exports carry the operator label, and nothing else moves", async () => { + const read = async (cfg: OcxConfig) => { + const { loadExportModels } = await import("../src/server/management/model-rows"); + return loadExportModels(cfg); + }; + const before = await withConfig(undefined, read); + const after = await withConfig({ [LABELLED]: LABEL }, read); + + const exported = (models: Awaited>, id: string) => + models.find(m => (m as Record).namespaced === `mock/${id}`) as + Record | undefined; + + expect(exported(after, LABELLED)?.displayName).toBe(LABEL); + expect(exported(before, LABELLED)?.displayName).toBeUndefined(); + expect(exported(after, LABELLED)?.id).toBe(LABELLED); + expect(withoutField(after, "displayName")).toEqual(withoutField(before, "displayName")); +}); + +// -- the boundary itself --------------------------------------------------- + +test("fetchAllModels is the labelling boundary every server surface shares", async () => { + // The three surfaces above all reach live models through this one call. Pinning + // it directly means a fourth consumer added later inherits the labels rather + // than quietly becoming a fifth surface that shows routed slugs. + const models = await withConfig({ [LABELLED]: LABEL }, async cfg => { + const { fetchAllModels } = await import("../src/server/management/shared"); + return fetchAllModels(cfg); + }); + + expect(models.find(m => m.id === LABELLED)?.displayName).toBe(LABEL); + expect(models.find(m => m.id === PLAIN)?.displayName).toBeUndefined(); + // Identity is untouched, which is what makes this safe to do for every caller. + expect(models.map(m => `${m.provider}/${m.id}`).sort()).toEqual( + [`mock/${PLAIN}`, `mock/${LABELLED}`].sort(), + ); +}); diff --git a/tests/catalog-operator-display-labels.test.ts b/tests/catalog-operator-display-labels.test.ts new file mode 100644 index 0000000000..6333475713 --- /dev/null +++ b/tests/catalog-operator-display-labels.test.ts @@ -0,0 +1,310 @@ +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, per key and for the whole map", () => { + expect(kept({ m: null })).toBeUndefined(); + expect(writeError({ m: null })).toBeNull(); + // The whole-map form has to agree with the loader too: it accepts null and clears, + // so rejecting it at the write boundary made the documented way to remove every + // label a 400 on one path and a no-op on the other. + expect(kept(null)).toBeUndefined(); + expect(writeError(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 c839c5b6fa..43b483bd74 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -13,7 +13,7 @@ import { getCodexUpstreamHealth, recordCodexUpstreamOutcome, } from "../src/codex/routing"; -import { loadConfig, saveConfig } from "../src/config"; +import { loadConfig, MAX_MODEL_DISPLAY_NAMES, saveConfig } from "../src/config"; import { deriveProviderPresets } from "../src/providers/derive"; import { MAIN_CODEX_ACCOUNT_ID } from "../src/codex/main-account"; import { @@ -822,6 +822,281 @@ 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("an explicit null on POST clears the map instead of being refused or merged back", async () => { + // The two boundaries have to agree on what null means. The loader treats it as + // "clear"; the write boundary used to call it "must be a plain object", so the + // documented way to remove every label was a 400. And once accepted, the + // preservation carry-over would happily merge the stored map back over the clear. + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { modelDisplayNames: LABELS })).status).toBe(200); + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(LABELS); + + const cleared = await seedProvider(server.url, { modelDisplayNames: null }); + expect(cleared.status).toBe(200); + expect(loadConfig().providers.labels?.modelDisplayNames).toBeUndefined(); + + // An omitted field still means "not carried", not "cleared" — the two must not + // collapse into the same behaviour. + 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); + } + }); + + // Ingwannu, exact-head review of b0af8ebe: both write paths validate the *submitted + // fragment* and only then merge it with the stored map, so the cap is enforced against + // a number that is not the number that gets persisted. Two individually-legal maps + // therefore add up to an illegal one, and the loader is left to truncate it. + test("a POST that merges under the cap on its own still cannot exceed it once merged", async () => { + freshHome(); + const server = startServer(0); + try { + const half = (prefix: string, count: number) => + Object.fromEntries(Array.from({ length: count }, (_, i) => [`${prefix}-${i}`, `Label ${prefix} ${i}`])); + + const first = half("a", MAX_MODEL_DISPLAY_NAMES); + expect((await seedProvider(server.url, { modelDisplayNames: first })).status).toBe(200); + expect(Object.keys(loadConfig().providers.labels?.modelDisplayNames ?? {}).length) + .toBe(MAX_MODEL_DISPLAY_NAMES); + + // Legal in isolation: exactly at the cap, every label valid. Illegal once merged + // with the stored map, which preservation is about to do. + const second = half("b", MAX_MODEL_DISPLAY_NAMES); + const response = await seedProvider(server.url, { modelDisplayNames: second }); + expect(response.status).toBe(400); + expect((await response.json()).error).toMatch(/at most/); + + // The refused write leaves the stored map exactly as it was. + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(first); + } finally { + await server.stop(true); + } + }); + + test("a PATCH that merges over the cap is refused rather than persisted and truncated later", async () => { + freshHome(); + const server = startServer(0); + try { + const first = Object.fromEntries( + Array.from({ length: MAX_MODEL_DISPLAY_NAMES }, (_, i) => [`a-${i}`, `Label A ${i}`]), + ); + expect((await seedProvider(server.url, { modelDisplayNames: first })).status).toBe(200); + + const patch = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelDisplayNames: { "b-0": "One More Label" } }), + }); + expect(patch.status).toBe(400); + expect((await patch.json()).error).toMatch(/at most/); + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(first); + } finally { + await server.stop(true); + } + }); + + test("whitespace-collapsing keys are counted as what they become, not as what was sent", async () => { + // The stored key is `model.trim()`, so `"m"` and `" m "` are one entry after + // normalization but two before it. Counting before normalizing lets a map pass the + // cap on a number the store never sees — in this direction it merely miscounts, but + // it also means a collision silently overwrites rather than being rejected. + freshHome(); + const server = startServer(0); + try { + expect((await seedProvider(server.url, { modelDisplayNames: LABELS })).status).toBe(200); + + const patch = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + modelDisplayNames: { "moonshotai/kimi-k3": "First Wins", " moonshotai/kimi-k3 ": "Second Wins" }, + }), + }); + expect(patch.status).toBe(400); + expect((await patch.json()).error).toMatch(/same model id|duplicate/i); + expect(loadConfig().providers.labels?.modelDisplayNames).toEqual(LABELS); + } finally { + await server.stop(true); + } + }); + + test("a model id of __proto__ round-trips instead of vanishing between check and store", async () => { + // `JSON.parse` puts `"__proto__"` on the object as an *own* property, so it reaches + // validation and is counted. Assigning it onto a `{}` literal runs the prototype + // setter and creates no own property, so the entry would be accepted and then + // silently absent from the store — the same accepted-then-discarded shape this + // change exists to remove. Not a pollution vector (the value is a string), but the + // two boundaries have to agree on what was saved. + freshHome(); + const server = startServer(0); + try { + const labels = JSON.parse('{"__proto__":"Proto Label","moonshotai/kimi-k3":"Kimi K3"}') as Record; + expect((await seedProvider(server.url, { modelDisplayNames: labels })).status).toBe(200); + + const storedPost = loadConfig().providers.labels?.modelDisplayNames ?? {}; + expect(Object.prototype.hasOwnProperty.call(storedPost, "__proto__")).toBe(true); + expect(Object.getOwnPropertyNames(storedPost).sort()).toEqual(["__proto__", "moonshotai/kimi-k3"]); + // The prototype chain is untouched — the key is data, not a mutation. + expect(({} as Record).polluted).toBeUndefined(); + + const patch = await fetch(new URL("/api/providers?name=labels", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ modelDisplayNames: JSON.parse('{"__proto__":"Patched Proto"}') }), + }); + expect(patch.status).toBe(200); + + const storedPatch = loadConfig().providers.labels?.modelDisplayNames ?? {}; + expect(Object.getOwnPropertyDescriptor(storedPatch, "__proto__")?.value).toBe("Patched Proto"); + // The sibling label survives the PATCH merge rather than being dropped with it. + expect(storedPatch["moonshotai/kimi-k3"]).toBe("Kimi K3"); + } 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 });