Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions src/codex/catalog/display-labels.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
/**
* Operator-supplied display labels for live-discovered provider models (#2201).
*
* A discovered row's label is its routed slug, so NVIDIA NIM surfaces as
* `nvidia/deepseek-ai-deepseek-v4-flash-0731` in the picker, the dashboard,
* `/v1/models`, and client exports. `customModels[].displayName` and combo
* display labels already relabel their rows display-only; live discovery is the
* one row source with no equivalent.
*
* The single invariant: a display label is never routing identity. Nothing here
* touches `provider`, `id`, or the routed slug — the resolved label lands on
* `CatalogModel.displayName`, which `applyCatalogModelMetadata` already treats
* as display-only, and which client exports already read.
*/

import { COMBO_NAMESPACE } from "../../combos/types";
import type { OcxConfig } from "../../types/config";
import { CODEX_CUSTOM_MODEL_CATALOG_KIND } from "./parsing";
import type { CatalogModel } from "./parsing";

/** Same bound as the combo display label (src/combos/types.ts) so every label surface agrees. */
export const MAX_DISPLAY_LABEL_LENGTH = 128;

// Control characters corrupt picker rendering, so a label carrying one is rejected.
// The range is C0, DEL, and C1 (U+0080-U+009F). C1 was originally missing, which let
// a label such as `Label<U+0085>More` through — U+0085 is NEL, a line break, and
// `trim()` does not touch a mid-string one. U+2028/U+2029 are added for the same
// reason: they are line and paragraph separators, so a label carrying one is not
// single-line whatever its width.
const CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/;

/**
* Checked against the *untrimmed* value, unlike `CONTROL_CHARS`.
*
* `trim()` counts U+2028/U+2029 as whitespace and would strip an edge one, so a
* trailing line separator would otherwise be normalised away and reported as
* valid. A stray space or newline is plausible slop in a hand-edited config and
* is still forgiven; a Unicode line separator is not, so it is rejected wherever
* it appears rather than quietly removed.
*/
const CONTROLS_TRIM_WOULD_HIDE = /[\u0080-\u009f\u2028\u2029]/;

/**
* A label is usable when it is a non-empty single-line string within the shared bound.
*
* Slashes are rejected, matching the `customModels[].displayName` rule: a label
* containing `/` reads as a routed slug, and this field must never be mistaken
* for one.
*/
export function isValidDisplayLabel(value: unknown): value is string {
if (typeof value !== "string") return false;
if (CONTROLS_TRIM_WOULD_HIDE.test(value)) return false;
const trimmed = value.trim();
if (trimmed.length === 0 || trimmed.length > MAX_DISPLAY_LABEL_LENGTH) return false;
if (CONTROL_CHARS.test(trimmed)) return false;
return !trimmed.includes("/");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

/**
* The precedence chain, in one place:
*
* 1. operator override — `providers[<provider>].modelDisplayNames[<native id>]`
* 2. trusted discovery metadata — a label discovery already attached
* 3. undefined — caller keeps its derived slug, i.e. today's behaviour
*
* Rows that already own an operator-supplied label keep it, and the provider map
* must not outrank them:
* - combo rows validate their own bounded label independently, so an entry under
* the combo namespace is never relabelled from provider config;
* - an explicit `customModels[]` row carries the label the operator typed there,
* and #2201 requires those to continue unchanged. Matching on `catalogKind`
* rather than on the provider name is what makes that hold, because a custom
* model shares its provider with the discovered rows this function exists for.
*
* Native OpenAI rows never reach here at all — they come from the pinned snapshot
* path with no `CatalogModel` — so upstream marketing names stay untouched.
*/
export function resolveModelDisplayLabel(
config: OcxConfig,
model: CatalogModel,
): string | undefined {
if (model.provider === COMBO_NAMESPACE || model.catalogKind === CODEX_CUSTOM_MODEL_CATALOG_KIND) {
return isValidDisplayLabel(model.displayName) ? model.displayName.trim() : undefined;
}
const override = config.providers?.[model.provider]?.modelDisplayNames?.[model.id];
if (isValidDisplayLabel(override)) return override.trim();
if (isValidDisplayLabel(model.displayName)) return model.displayName.trim();
return undefined;
}

/**
* Resolve labels across a discovered model list — the post-gather boundary.
*
* Called at exactly two places, one per gather entry point, so every surface that
* reads live routed models is covered:
*
* - `fetchAllModels` (src/server/management/shared.ts) — `/v1/models`,
* `/api/models` via `listManagementModelRows`, and client exports via
* `loadExportModels` all funnel through it;
* - `prepareCatalog` (src/codex/convergence.ts) — reaches the gather by the
* separate catalog-gather entry point, which never passes through the above.
*
* Anything reading models *without* going through one of those two is a surface
* that will emit the routed slug as its label. That is not hypothetical: labelling
* only the convergence call site is what left the live `/v1/models` route wrong
* while the on-disk catalog was right.
*
* Both calls sit after the gather rather than inside it, because the gather is
* TTL-cached on provider identity and a label baked into a cached entry would
* outlive an operator's edit to it.
*
* Idempotent, so the two boundaries overlapping is harmless: `resolveModelDisplayLabel`
* reads the config and the row's own kind, never a previously applied result.
*
* Returns the input array unchanged when nothing resolves, and otherwise a new
* array of new objects — the input models are never mutated, so a caller holding
* the pre-label list keeps it intact.
*/
export function applyOperatorDisplayLabels(
models: CatalogModel[],
config: OcxConfig,
): CatalogModel[] {
let changed = false;
const labeled = models.map(model => {
const label = resolveModelDisplayLabel(config, model);
if (label === undefined || label === model.displayName) return model;
changed = true;
return { ...model, displayName: label };
});
return changed ? labeled : models;
}
13 changes: 12 additions & 1 deletion src/codex/convergence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -241,7 +242,17 @@ function prepareCatalog(
const template = findNativeTemplate(catalog);
const enabled = filterCatalogVisibleModels(routedModels, config);
const featured = config.subagentModels ?? [];
const ordered = orderForSubagents(enabled, featured);
// #2201: resolve operator display labels before ordering. Display-only — the
// routed slug, provider id and native model id are all unchanged, so ordering,
// featuring and spawn-candidate derivation below see the same identities.
//
// One of the two post-gather label boundaries; the other is `fetchAllModels`,
// which covers every server surface. This path needs its own because it reaches
// the gather through `gatherRoutedModelsForCatalogGather`, which never passes
// through `fetchAllModels`. See applyOperatorDisplayLabels for why both sit
// after the gather rather than inside it.
const labeled = applyOperatorDisplayLabels(enabled, config);
const ordered = orderForSubagents(labeled, featured);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const modelPickerOrder = config.modelPickerOrder ?? [];
const multiAgentMode = config.multiAgentMode === "v1" || config.multiAgentMode === "v2"
? config.multiAgentMode : "default";
Expand Down
24 changes: 24 additions & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { isValidProviderName, hasOwnProvider } from "./config/provider-name";
import {
apiKeyTransportConfigError,
booleanRecordConfigError,
displayLabelRecordConfigError,
MAX_MODEL_DISPLAY_NAMES,
modelAdapterRecordConfigError,
nonBlankStringArrayConfigError,
normalizeNonBlankStringArray,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<string, unknown>)
.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(),
Expand Down Expand Up @@ -536,6 +558,8 @@ export { isValidProviderName, hasOwnProvider } from "./config/provider-name";
export {
apiKeyTransportConfigError,
booleanRecordConfigError,
displayLabelRecordConfigError,
MAX_MODEL_DISPLAY_NAMES,
modelAdapterRecordConfigError,
nonBlankStringArrayConfigError,
normalizeNonBlankStringArray,
Expand Down
78 changes: 78 additions & 0 deletions src/config/provider-validation.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -123,6 +124,83 @@ export function booleanRecordConfigError(value: unknown, field: string): string
return null;
}

/**
* Bound on how many labels one provider may carry. A display map is a convenience,
* not a catalogue, so an unbounded hand-edited map is a mistake rather than a use
* case — and every entry is walked on each convergence.
*/
export const MAX_MODEL_DISPLAY_NAMES = 512;

/**
* Strict diagnostic for `providers[<name>].modelDisplayNames`, mirroring
* `booleanRecordConfigError`.
*
* This is the *write* rule, used by the provider editor so a bad label is a 400
* rather than something that lands on disk. The load path is deliberately more
* forgiving — see the schema entry, which drops a bad entry instead of failing —
* because the two paths answer different questions: "is this a valid edit?" and
* "can this file still be served?".
*
* `null` is accepted as an explicit clear, matching `upstreamHttpVersion`: the
* management API says null means "remove this", so rejecting it here would refuse
* the documented way to take a label back off.
*/
export function displayLabelRecordConfigError(value: unknown, field = "modelDisplayNames"): string | null {
// `null` clears the whole map, the same way a per-key `null` clears one label and the same
// way the load schema treats it. Rejecting it here made the documented way to remove every
// label a 400 on POST while the loader accepted it — the two boundaries disagreed about
// what the operator had asked for.
if (value === undefined || value === null) return null;
if (!value || typeof value !== "object" || Array.isArray(value)) return `${field} must be a plain object`;
const prototype = Object.getPrototypeOf(value);
if (prototype !== Object.prototype && prototype !== null) return `${field} must be a plain object with own properties`;
const entries = Object.entries(value);
if (entries.length > MAX_MODEL_DISPLAY_NAMES) {
return `${field} must hold at most ${MAX_MODEL_DISPLAY_NAMES} entries`;
}
// Keys are stored trimmed, so two submitted keys can collapse into one stored entry.
// Counting before that happens means the cap is enforced against a number the store
// never sees, and the later of the two labels silently wins over the earlier — the
// operator gets a 200 for an instruction that was self-contradictory.
const seen = new Set<string>();
for (const [key, label] of entries) {
const id = key.trim();
if (!id) return `${field} keys must be nonblank model ids`;
if (seen.has(id)) return `${field} must not set the same model id twice (${id})`;
seen.add(id);
if (label === null) continue;
if (typeof label !== "string") return `${field}.${key} must be a string`;
if (!isValidDisplayLabel(label)) {
return `${field}.${key} must be a nonblank single-line label of at most `
+ `${MAX_DISPLAY_LABEL_LENGTH} characters, and must not contain '/'`;
}
}
return null;
}

/**
* Normalize a submitted label map to the shape that is actually persisted: keys and labels
* trimmed, non-string values carried through as tombstones for the caller to apply.
*
* PATCH already stored `model.trim()` while POST stored the key verbatim, so the same id
* submitted through the two routes produced two different stored keys for one model. This
* is the single definition of "what does this entry become", so the cap and the label rules
* can be checked against the map that will exist rather than the one that was sent.
*/
export function normalizeDisplayLabelRecord(value: object): Record<string, string | null> {
// 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<string, string | null>;
for (const [key, label] of Object.entries(value)) {
const id = key.trim();
if (!id) continue;
out[id] = typeof label === "string" ? label.trim() : null;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return out;
}

export function reasoningSummaryDeliveryRecordConfigError(
value: unknown,
supportsReasoningSummaries: unknown,
Expand Down
21 changes: 20 additions & 1 deletion src/server/management/provider-capability-config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { booleanRecordConfigError } from "../../config/provider-validation";
import { booleanRecordConfigError, displayLabelRecordConfigError } from "../../config/provider-validation";
import type { OcxConfig } from "../../types";

/**
Expand All @@ -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<string, boolean> | undefined {
if (!value || typeof value !== "object" || Array.isArray(value)) return undefined;
const entries = Object.entries(value).filter(([model, supported]) =>
Expand Down
Loading
Loading