From a973aba1622d95117dc6748992de8bdbef2ce81d Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 08:19:06 -0700 Subject: [PATCH 1/8] 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 b338aa9d3b..075a5456ee 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,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 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.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 e9b4fe9b9f8833d4a3376f0af31a11313c255ad4 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 14:49:41 -0700 Subject: [PATCH 2/8] 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 | 24 +++ src/config/provider-validation.ts | 43 +++++ .../management/provider-capability-config.ts | 21 ++- src/server/management/provider-routes.ts | 14 +- ...perator-display-labels-convergence.test.ts | 155 ++++++++++++++++++ tests/catalog-operator-display-labels.test.ts | 95 +++++++++++ 7 files changed, 361 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 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..d64deff9de 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,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; +} + 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..babbc14339 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -76,7 +76,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, @@ -547,6 +547,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 eb7f1944923444576c4dc03407f08b8c3a8e0e4c Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 14:53:20 -0700 Subject: [PATCH 3/8] 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 b4909d1dcfb5a38ac4733a6fb233007053479160 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Fri, 21 Aug 2026 15:44:23 -0700 Subject: [PATCH 4/8] 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 | 41 ++++++ tests/catalog-operator-display-labels.test.ts | 77 ++++++++-- tests/management-provider-validation.test.ts | 138 ++++++++++++++++++ 4 files changed, 261 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 babbc14339..142b132778 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -76,6 +76,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 { @@ -290,6 +291,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) { @@ -587,6 +616,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 c839c5b6fa..c873ce2d6a 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -822,6 +822,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 }); From 79201cbfd6339a7fc747c3351c6d506e5da533f4 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Mon, 24 Aug 2026 09:18:07 -0700 Subject: [PATCH 5/8] fix(config): accept a null modelDisplayNames map at the write boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit finding, and it is a real disagreement between two boundaries I wrote. The load schema treats `modelDisplayNames: null` as "clear the map"; displayLabelRecordConfigError called it "must be a plain object". So the documented way to remove every label was a 400 on POST and a silent clear on load, and the function's own comment claimed null was accepted when only the per-key form was. Accepting it exposed a second half: with the value accepted, the preservation carry-over added for the previous review merged the stored map straight back over the clear, so a successful POST would have left every label in place. An explicit null is now sampled alongside request ownership and canonicalized to absent — the same treatment upstreamHttpVersion gets twenty lines above, for the same reason. An omitted field still means "not carried" rather than "cleared"; the test asserts both so the two cannot collapse into one behaviour. 80 management-route tests and 36 unit/convergence pass. Without the fix the new route case is the only one that fails. --- src/config/provider-validation.ts | 6 ++++- src/server/management/provider-routes.ts | 9 ++++++- tests/catalog-operator-display-labels.test.ts | 7 +++++- tests/management-provider-validation.test.ts | 25 +++++++++++++++++++ 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index d64deff9de..888881f60f 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -146,7 +146,11 @@ export const MAX_MODEL_DISPLAY_NAMES = 512; * the documented way to take a label back off. */ export function displayLabelRecordConfigError(value: unknown, field = "modelDisplayNames"): string | null { - if (value === undefined) return 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`; diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 142b132778..7d4a85eec3 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -617,6 +617,9 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { expect(kept({ bad: "a/b", good: "Kimi K3" })).toEqual({ good: "Kimi K3" }); }); - test("null is an explicit clear on both paths", () => { + 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", () => { diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index c873ce2d6a..39d5987a9a 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -931,6 +931,31 @@ describe("provider management validation", () => { } }); + 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); + } + }); + test("a control character PATCH is refused, including the ones trim would hide", async () => { freshHome(); const server = startServer(0); From 13eac063213f4272ebfcaa73bfbee4fe336aaf4d Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Wed, 26 Aug 2026 09:00:20 -0700 Subject: [PATCH 6/8] fix(catalog): validate the merged display-label map, not the submitted fragment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both write paths validated the map that arrived and only then merged it with the stored one, so the cap was enforced against a number the store never sees. Two individually legal 512-entry maps merged to 1024 on disk; the loader's salvage then kept the first 512 — all of them the *old* entries — so every label the operator had just submitted, and received HTTP 200 for, was silently discarded. POST now normalizes the submitted fragment, applies per-key tombstones, merges, and validates the result before saving. Keys are normalized through one shared helper so POST and PATCH cannot store two different keys for the same model id. Keys collapse on trim, so two submitted keys can become one stored entry: that is now rejected against the submitted fragment, since by the time the merged map is validated the duplicate has already collapsed and the later label has silently won. --- src/config/provider-validation.ts | 29 ++++++- src/server/management/provider-routes.ts | 34 ++++++++- tests/management-provider-validation.test.ts | 79 +++++++++++++++++++- 3 files changed, 136 insertions(+), 6 deletions(-) diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index 888881f60f..d989fc72b2 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -158,8 +158,16 @@ export function displayLabelRecordConfigError(value: unknown, field = "modelDisp 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) { - if (!key.trim()) return `${field} keys must be nonblank model ids`; + 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)) { @@ -170,6 +178,25 @@ export function displayLabelRecordConfigError(value: unknown, field = "modelDisp 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 { + const out: 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-routes.ts b/src/server/management/provider-routes.ts index 7d4a85eec3..a961df58b2 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -78,6 +78,7 @@ 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 { normalizeDisplayLabelRecord } from "../../config/provider-validation"; import { applySystemEnvToggle } from "../system-env"; import { LOCAL_PROVIDER_RELOAD_NAME_HEADER, @@ -298,8 +299,16 @@ function applyProviderPatchFields( } else { if (!isPlainRecord(value)) return { error: "modelDisplayNames must be a plain object or null" }; const labels: 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) { @@ -672,11 +681,28 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise = { ...(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); diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 39d5987a9a..6a48c2bf32 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 { @@ -956,6 +956,83 @@ describe("provider management validation", () => { } }); + // 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 control character PATCH is refused, including the ones trim would hide", async () => { freshHome(); const server = startServer(0); From 2b875f7d544b5cbc3f8603002885141b735ee6f0 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Wed, 26 Aug 2026 17:38:12 -0700 Subject: [PATCH 7/8] fix(catalog): keep a __proto__ model id from vanishing between check and store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit JSON.parse puts a "__proto__" key on the object as an *own* property, so it reaches displayLabelRecordConfigError and is counted against the cap. Assigning it onto a {} literal runs the prototype setter and creates no own property, so the entry was validated and then silently absent from what was saved. That is the same accepted-then-discarded shape this branch exists to remove, one key narrower: the write reports success for something the store never holds. Not a pollution vector — the value is a string and the prototype chain is unchanged — but the two boundaries have to agree on what was written. The normalizer and both merge sites now build on Object.create(null), matching modelAutoCompactTokenLimits directly above them, which already does this. Raised by CodeRabbit on the previous head; verified rather than taken on trust — JSON.parse yields an own property, a {} literal drops it, and Object.create(null) and the object spread both keep it, which is why the merge and the normalizer disagreed. --- src/config/provider-validation.ts | 6 +++- src/server/management/provider-routes.ts | 10 ++++-- tests/management-provider-validation.test.ts | 35 ++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index d989fc72b2..e022363d28 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -188,7 +188,11 @@ export function displayLabelRecordConfigError(value: unknown, field = "modelDisp * can be checked against the map that will exist rather than the one that was sent. */ export function normalizeDisplayLabelRecord(value: object): Record { - const out: 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; diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index a961df58b2..c1dbf8e8f7 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -298,7 +298,10 @@ function applyProviderPatchFields( delete next.modelDisplayNames; } else { if (!isPlainRecord(value)) return { error: "modelDisplayNames must be a plain object or null" }; - const labels: Record = { ...(next.modelDisplayNames ?? {}) }; + 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. @@ -690,7 +693,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise = { ...(existing?.modelDisplayNames ?? {}) }; + const merged: Record = 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; diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 6a48c2bf32..43b483bd74 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -1033,6 +1033,41 @@ describe("provider management validation", () => { } }); + 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); From dc77114399970bfcdaada5925c7804cca811abad Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Thu, 27 Aug 2026 12:24:11 -0700 Subject: [PATCH 8/8] fix(catalog): label models at one post-gather boundary, not just convergence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applyOperatorDisplayLabels was called only from prepareCatalog, so the live GET /v1/models route kept emitting the routed slug as display_name while the on-disk catalog was already correct. The management and export path had the same gap: listManagementModelRows and loadExportModels both read raw gather output. All three of those surfaces reach live models through fetchAllModels, so that is where the labels now resolve — one boundary covering /v1/models, /api/models and client exports, rather than three call sites that a fourth consumer would silently not join. Notably this needs no change to src/server/index.ts, which already calls fetchAllModels, so the route is fixed without touching an owner-only file. convergence keeps its own call because it reaches the gather through gatherRoutedModelsForCatalogGather, which never passes through fetchAllModels. Both boundaries are documented as the pair they are, and overlapping is harmless: resolveModelDisplayLabel reads the config and the row's own kind, never a previously applied result. Both sit after the gather rather than inside it on purpose. 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. The regressions drive the real routes, because a test that calls the helper by hand proves the helper works and not that anything uses it — which is exactly how the omission survived the previous round. Each surface is exercised twice, with and without the operator map, and the two payloads are compared with the label field removed. That pins display-only without enumerating what must not move: slug, provider, native id and ordering are all covered because the whole payload has to match, so a change nobody predicted still fails. Verified the tests detect the defect rather than only passing: with fetchAllModels reverted to the pre-fix `return gatherRoutedModels(config)`, all six fail. Restored and confirmed. Gates: 128/128 on this PR's four suites; `bun run privacy:scan` and `git diff --check` pass; typecheck reports 3 errors, all pre-existing on an unmodified tree in src/server/claude-messages.ts and src/server/responses/fetch-helpers.ts, none in the changed files. Four failures appear when eight catalog suites run in one process; they reproduce identically on an unmodified tree, so they are pre-existing cross-file interference rather than anything here. Part of #2201 --- src/codex/catalog/display-labels.ts | 23 +- src/codex/convergence.ts | 6 + src/server/management/shared.ts | 17 +- ...log-operator-display-labels-routes.test.ts | 206 ++++++++++++++++++ 4 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 tests/catalog-operator-display-labels-routes.test.ts diff --git a/src/codex/catalog/display-labels.ts b/src/codex/catalog/display-labels.ts index 84521d0a4b..c978b95333 100644 --- a/src/codex/catalog/display-labels.ts +++ b/src/codex/catalog/display-labels.ts @@ -89,7 +89,28 @@ export function resolveModelDisplayLabel( } /** - * Resolve labels across a discovered model list. + * 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 diff --git a/src/codex/convergence.ts b/src/codex/convergence.ts index 075a5456ee..7640d2058c 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -245,6 +245,12 @@ function prepareCatalog( // #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 ?? []; diff --git a/src/server/management/shared.ts b/src/server/management/shared.ts index 3ebea685e8..7090587810 100644 --- a/src/server/management/shared.ts +++ b/src/server/management/shared.ts @@ -175,10 +175,25 @@ export function requestLogDto(entry: RequestLogEntry): Record { * 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/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(), + ); +});