From 17efd01612096efd86257fcf36006b8d3f5f244c Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 08:19:06 -0700 Subject: [PATCH 1/4] feat(catalog): operator display labels for live-discovered models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A discovered row's label is its routed slug, so an NVIDIA NIM model reads `nvidia/deepseek-ai-deepseek-v4-flash-0731` in the picker, the dashboard, /v1/models and client exports. customModels[].displayName and combo labels already relabel their rows display-only; live discovery was the one row source with no equivalent. Adds providers[].modelDisplayNames, keyed by the upstream native model id — the same key space as modelAdapters — and one resolver holding the precedence chain: operator override, then trusted discovery metadata, then undefined so the caller keeps its derived slug and today's behaviour stands. Display metadata never becomes routing identity. The resolved label lands on CatalogModel.displayName, which applyCatalogModelMetadata already treats as display-only and which client exports already read, so nothing new touches provider id, native model id or the routed slug. Labels are bounded at 128 characters to match the combo label, reject control characters, and reject `/` so a label can never read as a slug. Combo rows are skipped: they validate their own bounded label independently. Native OpenAI rows come from the pinned snapshot path with no CatalogModel, so upstream marketing names stay untouched. Provider-level display labels are deliberately left out. Mixing a provider name and a model name in one field is the failure this issue warns against, so it belongs in its own field and its own change. --- src/codex/catalog/display-labels.ts | 85 +++++++++++ src/codex/convergence.ts | 7 +- src/types/provider.ts | 14 ++ tests/catalog-operator-display-labels.test.ts | 136 ++++++++++++++++++ 4 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 src/codex/catalog/display-labels.ts create mode 100644 tests/catalog-operator-display-labels.test.ts diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts new file mode 100644 index 0000000000..8e6ee4781a --- /dev/null +++ b/src/codex/catalog/display-labels.ts @@ -0,0 +1,85 @@ +/** + * 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 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. +const CONTROL_CHARS = /[\u0000-\u001f\u007f]/; + +/** + * 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; + 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 + * + * Combo rows are skipped: they validate their own bounded label independently, so + * an entry under the combo namespace must not be relabelled from provider config. + * 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) { + 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/types/provider.ts b/src/types/provider.ts index b4044050d5..321e89b026 100644 --- a/src/types/provider.ts +++ b/src/types/provider.ts @@ -194,6 +194,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.test.ts b/tests/catalog-operator-display-labels.test.ts new file mode 100644 index 0000000000..98115a279c --- /dev/null +++ b/tests/catalog-operator-display-labels.test.ts @@ -0,0 +1,136 @@ +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 type { CatalogModel } from "../src/codex/catalog/parsing"; +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); + }); +}); + +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("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); + }); +}); From a6f8167cc41c0df4f18de549ba0399bf2749070d Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 14:49:41 -0700 Subject: [PATCH 2/4] feat(catalog): declare modelDisplayNames, and keep custom-model labels Addresses both blockers from @Ingwannu's review. modelDisplayNames existed only in the TypeScript interface, so providerConfigSchema's .passthrough() accepted anything: an array, a number-valued entry, a blank key, a slash-bearing label and a 1000-entry map all validated and persisted. A misspelled key was silently dropped, which is the #2106 failure mode the codexToolMode comment beside it already warns about. Declared it, with the two paths deliberately differing: - load salvages entry by entry, following apiKeys. One hand-edited label must not send the whole config through backup-and-defaults, and must not take the operator's other labels with it. - writes go through displayLabelRecordConfigError at the three provider-route sites, so an invalid label is a 400 rather than a 200 followed by a label that silently isn't there. null is an explicit clear on both paths, matching upstreamHttpVersion. The map is bounded at 512 entries. resolveModelDisplayLabel also relabelled explicit customModels[] rows, overwriting a label the operator had already typed and breaking #2201's migration rule. Those now keep their own label, alongside combos. The guard matches on catalogKind rather than provider name, because a custom model shares its provider with the discovered rows this feature exists to relabel. Adds the end-to-end cover asked for: a label loaded through the real validator reaches entry.display_name while every other field on the entry stays byte-identical, and removing it restores the derived label. 32 unit + 8 convergence tests. Against the unfixed source the two fix-gating cases go red; the other six are regression guards. --- src/codex/catalog/display-labels.ts | 14 +- src/config.ts | 62 +++++++ .../management/provider-capability-config.ts | 21 ++- src/server/management/provider-routes.ts | 11 +- ...perator-display-labels-convergence.test.ts | 155 ++++++++++++++++++ tests/catalog-operator-display-labels.test.ts | 95 +++++++++++ 6 files changed, 353 insertions(+), 5 deletions(-) create mode 100644 tests/catalog-operator-display-labels-convergence.test.ts diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts index 8e6ee4781a..03ebdbc689 100644 --- a/src/codex/catalog/display-labels.ts +++ b/src/codex/catalog/display-labels.ts @@ -15,6 +15,7 @@ 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. */ @@ -45,8 +46,15 @@ export function isValidDisplayLabel(value: unknown): value is string { * 2. trusted discovery metadata — a label discovery already attached * 3. undefined — caller keeps its derived slug, i.e. today's behaviour * - * Combo rows are skipped: they validate their own bounded label independently, so - * an entry under the combo namespace must not be relabelled from provider config. + * 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. */ @@ -54,7 +62,7 @@ export function resolveModelDisplayLabel( config: OcxConfig, model: CatalogModel, ): string | undefined { - if (model.provider === COMBO_NAMESPACE) { + 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]; 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..c6011490e2 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -75,7 +75,7 @@ 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 { providerDisplayNamesConfigError, providerServiceTierConfigError } from "./provider-capability-config"; import { applySystemEnvToggle } from "../system-env"; import { LOCAL_PROVIDER_RELOAD_NAME_HEADER, @@ -500,6 +500,8 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { + 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 index 98115a279c..11db4f8b07 100644 --- a/tests/catalog-operator-display-labels.test.ts +++ b/tests/catalog-operator-display-labels.test.ts @@ -7,7 +7,13 @@ import { 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. */ @@ -90,6 +96,32 @@ describe("resolveModelDisplayLabel precedence", () => { 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, @@ -134,3 +166,66 @@ describe("applyOperatorDisplayLabels", () => { 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(); + }); +}); From 866dda2e94a5e71b31baa83655e8fce2d0c31dc5 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 14:53:20 -0700 Subject: [PATCH 3/4] test(catalog): pin that no control character reaches a stored display label Answers the CodeRabbit finding about checking control characters before trimming. The ordering is real: the class overlaps trim()'s whitespace, so an edge LF/TAB/CR is normalised away while every other control character is rejected wherever it sits, and a mid-label LF is rejected because a label is single-line by definition. The outcome is correct either way, but it depends on two lines interacting and was not asserted anywhere, so it was one refactor away from silently becoming untrue. --- tests/catalog-operator-display-labels.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/catalog-operator-display-labels.test.ts b/tests/catalog-operator-display-labels.test.ts index 11db4f8b07..b7a09f1ffe 100644 --- a/tests/catalog-operator-display-labels.test.ts +++ b/tests/catalog-operator-display-labels.test.ts @@ -50,6 +50,29 @@ describe("isValidDisplayLabel", () => { test("rejects a label carrying a control character", () => { expect(isValidDisplayLabel("DeepSeek\u0007V4")).toBe(false); }); + + test("no control character can reach the stored label, whichever side of trim it falls on", () => { + // The check runs on the trimmed value, so the whitespace-class controls are + // normalised away rather than rejected: `"Label\n"` stores as `"Label"`. Every + // other control character is rejected wherever it sits. Pinned explicitly + // because the outcome depends on trim() and the class overlapping, which is + // not obvious from either line on its own. + for (const edge of ["\u000a", "\u0009", "\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"); + } + for (const inner of ["\u0000", "\u0007", "\u001f", "\u007f"]) { + expect(isValidDisplayLabel(`Label${inner}`)).toBe(false); + expect(isValidDisplayLabel(`La${inner}bel`)).toBe(false); + } + // A control character mid-label is rejected even from the whitespace class, + // because a label is single-line by definition. + expect(isValidDisplayLabel("La\u000abel")).toBe(false); + }); }); describe("resolveModelDisplayLabel precedence", () => { From 48326fc534d5c03b56bbd20912bf8817b3151d24 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 15:44:23 -0700 Subject: [PATCH 4/4] fix(catalog): widen the label control class, and preserve labels across a POST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both blockers from @Ingwannu's second review. 1. isValidDisplayLabel only excluded C0 and DEL, so `LabelMore` and `LabelMore` were reported valid, produced no write error, and were stored verbatim. U+0085 is NEL and U+2028 a line separator, so the "single-line label" guarantee did not hold for either. The class is now C0 + DEL + C1 + U+2028/U+2029. C1 and the separators are additionally checked against the untrimmed value: trim() counts U+2028/U+2029 as whitespace, so an edge one would have been normalised away and reported valid. Ordinary ASCII whitespace is still forgiven at the edges, deliberately — that is plausible slop in a hand-edited config, and on the load path a rejection means silently losing the operator's label. The previous test claimed no control character could reach storage while only covering C0. It now walks the ranges and asserts on the value that actually lands on the row, which is the invariant that matters, rather than on a sample of rejections. 2. A provider POST that omitted modelDisplayNames deleted the stored map. ProviderPayload has no member for the field and this change leaves the dashboard editor to a follow-up, so the add/edit form cannot round-trip it at all: absence means "not carried", never "the operator deleted it". Ownership is now sampled before enrichProviderFromCatalog, matching the comment there about why a post-enrichment guard can never fire, and the existing map is preserved on omission and merged on submission — the same boundary as modelCosts, requestPacing and modelContextWindows. PATCH also becomes the deletion path rather than being left out of scope: modelDisplayNames was not a recognised PATCH field, so such a body returned 400 "no recognized fields to update". A per-key null now clears one label and an explicit null clears the map. 36 unit/convergence and 79 management-route tests pass. Against the unfixed source, 4 of the 6 new route cases and both new class cases go red. One of the new route tests initially passed for the wrong reason: it asserted only status 400, which the unrecognised-field path already returned. It now asserts the error message, so it can only pass when the label rule runs. --- src/codex/catalog/display-labels.ts | 19 ++- src/server/management/provider-routes.ts | 40 +++++ tests/catalog-operator-display-labels.test.ts | 77 ++++++++-- tests/management-provider-validation.test.ts | 138 ++++++++++++++++++ 4 files changed, 260 insertions(+), 14 deletions(-) diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts index 03ebdbc689..84521d0a4b 100644 --- a/src/codex/catalog/display-labels.ts +++ b/src/codex/catalog/display-labels.ts @@ -22,7 +22,23 @@ import type { CatalogModel } from "./parsing"; export const MAX_DISPLAY_LABEL_LENGTH = 128; // Control characters corrupt picker rendering, so a label carrying one is rejected. -const CONTROL_CHARS = /[\u0000-\u001f\u007f]/; +// 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. @@ -33,6 +49,7 @@ const CONTROL_CHARS = /[\u0000-\u001f\u007f]/; */ 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; diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index c6011490e2..fa448cef1e 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -75,6 +75,7 @@ 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 { MAX_DISPLAY_LABEL_LENGTH, isValidDisplayLabel } from "../../codex/catalog/display-labels"; import { providerDisplayNamesConfigError, providerServiceTierConfigError } from "./provider-capability-config"; import { applySystemEnvToggle } from "../system-env"; import { @@ -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) { @@ -539,6 +568,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { expect(isValidDisplayLabel("DeepSeek\u0007V4")).toBe(false); }); - test("no control character can reach the stored label, whichever side of trim it falls on", () => { - // The check runs on the trimmed value, so the whitespace-class controls are - // normalised away rather than rejected: `"Label\n"` stores as `"Label"`. Every - // other control character is rejected wherever it sits. Pinned explicitly - // because the outcome depends on trim() and the class overlapping, which is - // not obvious from either line on its own. - for (const edge of ["\u000a", "\u0009", "\u000d"]) { + 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( @@ -65,13 +64,65 @@ describe("isValidDisplayLabel", () => { NVIDIA, )).toBe("Label"); } - for (const inner of ["\u0000", "\u0007", "\u001f", "\u007f"]) { - expect(isValidDisplayLabel(`Label${inner}`)).toBe(false); - expect(isValidDisplayLabel(`La${inner}bel`)).toBe(false); - } - // A control character mid-label is rejected even from the whitespace class, - // because a label is single-line by definition. + // 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); + } }); }); 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 });