From ea0f0efe224b7412f7683040f207a5aeba4b2a66 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Wed, 9 Sep 2026 15:56:36 +0800 Subject: [PATCH 01/14] feat(catalog): provider-level auto-review model overrides --- .../docs/reference/configuration/providers.md | 41 +++ src/codex/catalog/sync.ts | 254 +++++++++++++++++- src/codex/convergence.ts | 2 +- src/config.ts | 62 +++++ src/config/provider-validation.ts | 78 ++++++ src/server/auth-cors.ts | 11 + src/server/management/provider-routes.ts | 76 +++++- src/types/provider.ts | 22 ++ tests/codex-integration/codex-catalog.test.ts | 142 ++++++++++ ...odex-convergence-account-selectors.test.ts | 52 ++++ tests/config/config-load-degrade.test.ts | 36 +++ .../management-provider-validation.test.ts | 151 ++++++++++- 12 files changed, 910 insertions(+), 17 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 23b7e5e92d..e136b09d58 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -260,6 +260,46 @@ use `null` to clear a scalar or the whole map. A map entry set to `null` or `""` entry while preserving other entries. Malformed writes are rejected before saving. A malformed optional pin in a hand-edited file is ignored on load without discarding the rest of the config. +### Auto-review (approval) model selection + +Codex reads `auto_review_model_override` from the catalog row of the current turn's model to +choose the model that reviews approval requests. The root `auto_review_model` setting in +`$CODEX_HOME/config.toml` applies one reviewer to every catalog row. When different routed +providers should use different (usually cheaper) reviewers, add provider-scoped selectors to the +provider row in `config.json` instead: + +```json +{ + "providers": { + "blsc": { + "adapter": "openai-chat", + "baseUrl": "https://llmapi.blsc.cn", + "autoReviewModel": "opencode-go/deepseek-v4-flash", + "autoReviewModelOverrides": { + "kimi-k3": "gpt-5.6-terra" + } + } + } +} +``` + +`autoReviewModel` is the provider-wide target. A value can be a bare model id of that same +provider (the catalog row is normalized to the `provider/model` slug) or a full public catalog +slug such as `opencode-go/deepseek-v4-flash`. `autoReviewModelOverrides` keys are exact upstream +model ids of that provider; an entry wins over the provider-wide value for its model. A provider +stamp wins over the root selector on its own routed rows, and the root selector remains the +fallback for native rows and routed rows without a provider stamp. Removing a provider selector +clears only that provider's stamps; removing the root selector never clears provider stamps. +Model ids that contain a slash may be written raw or in their encoded catalog form; both +spellings resolve to the same routed row. +Selectors are resolved against the final catalog on each sync: an unknown target fails closed +for the override only, emits a diagnostic, and leaves normal upstream auto-review behavior in +place. The canonical `openai` provider does not accept these fields. + +`PATCH /api/providers?name=` accepts both fields. Use `null` to clear the scalar or +the whole map; use a map entry of `null` or `""` to remove that model while preserving other +entries. Unrelated provider saves preserve previously configured values. + ### Discovered model display names Use `modelDisplayNames` when a provider returns machine friendly ids but the Codex model picker @@ -328,6 +368,7 @@ projection, and merge precedence, so only a selector present in the catalog prod that sync can become an override. Native upstream values are preserved when the setting is cleared or unresolved. The persisted catalog field is read by Codex for the current turn's model, which is why a valid configured selector is copied to each applicable entry. +Provider-scoped selectors (above) are applied before this root fallback and win on routed rows. ### FastWire B1 capability migration diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 2f756663ad..eb3a6ff343 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -16,7 +16,8 @@ import { CODEX_REASONING_LEVELS, codexEffortRank, configuredReasoningEfforts, mo import { getModelMetadata, getModelMetadataCaseInsensitive, listModelMetadata, resolveMetadataProvider } from "../../generated/model-metadata"; import { enrichProviderFromRegistry, shouldCaseFoldMetadataModelId } from "../../providers/derive"; import { applyProviderContextCap, providerContextCap } from "../../providers/context-cap"; -import { routedSlug, slugEquals, slugEquivalenceKey, slugsEquivalent } from "../../providers/slug-codec"; +import { encodeRoutedModelId, routedSlug, slugEquals, slugEquivalenceKey, slugsEquivalent } from "../../providers/slug-codec"; +import { canonicalAutoReviewModelKey, isValidAutoReviewModel as isValidAutoReviewTarget } from "../../config/provider-validation"; import { identifyRoutedModel } from "../../adapters/identity"; import { filterCursorConfiguredModelsByLiveDiscovery } from "../../adapters/cursor/discovery"; import { fetchCursorUsableModels } from "../../adapters/cursor/live-models"; @@ -1586,14 +1587,11 @@ function catalogModelsForMergeWithNativeRecovery( ]); } -const AUTO_REVIEW_MODEL_CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f\u2028\u2029\s]/; +const AUTO_REVIEW_ROOT_MARKER = "opencodex_auto_review_root"; +/** True when the value is a valid Codex catalog auto-review selector. */ export function isValidAutoReviewModel(value: unknown): value is string { - if (typeof value !== "string") return false; - const trimmed = value.trim(); - return Boolean(trimmed) - && trimmed.length <= 1024 - && !AUTO_REVIEW_MODEL_CONTROL_CHARS.test(trimmed); + return isValidAutoReviewTarget(value); } export type AutoReviewModelOverrideResult = "absent" | "applied" | "invalid" | "unresolved"; @@ -1604,6 +1602,12 @@ function isRoutedCatalogEntry(entry: RawEntry): boolean { || (typeof entry.description === "string" && entry.description.startsWith("Routed via opencodex → ")); } +/** Remove an override and its root-derived provenance marker from one catalog row. */ +function clearAutoReviewOverrideValue(entry: RawEntry): void { + entry.auto_review_model_override = null; + delete entry[AUTO_REVIEW_ROOT_MARKER]; +} + function clearAutoReviewModelOverride( models: readonly RawEntry[], sourceModels: readonly RawEntry[] = [], @@ -1631,12 +1635,14 @@ function clearAutoReviewModelOverride( if (!entry || typeof entry !== "object") continue; const current = entry.auto_review_model_override; if (isRoutedCatalogEntry(entry) + || entry[AUTO_REVIEW_ROOT_MARKER] === true || (globalStamp && typeof current === "string" && configuredValues.has(current))) { - entry.auto_review_model_override = null; + clearAutoReviewOverrideValue(entry); } } } +/** Warn once about a malformed or unresolvable root auto-review selector. */ function warnAutoReviewModelDiagnostic( reason: "invalid" | "unresolved", configured: string, @@ -1650,24 +1656,58 @@ function warnAutoReviewModelDiagnostic( ); } +/** Warn once about a malformed or unresolvable provider-scoped auto-review selector. */ +function warnProviderAutoReviewModelDiagnostic( + reason: "invalid" | "unresolved", + provider: string, + configured: string, +): void { + const safeProvider = JSON.stringify(redactSecretString(provider)); + const safeConfigured = JSON.stringify(redactSecretString(configured)); + const detail = reason === "unresolved" + ? "the selector was not found in the final catalog" + : "the selector format is invalid"; + console.warn( + `[opencodex] auto_review_model for provider ${safeProvider} ${detail} (${safeConfigured}); preserving normal upstream auto-review behavior.`, + ); +} + +/** Preserve native upstream overrides and the root-derived provenance marker from source rows. */ function preserveNativeAutoReviewModelOverrides( models: readonly RawEntry[], sourceModels: readonly RawEntry[], ): void { - const existing = new Map(); + const existing = new Map(); for (const entry of sourceModels) { const slug = typeof entry.slug === "string" ? entry.slug : undefined; const value = entry.auto_review_model_override; if (!slug || isRoutedCatalogEntry(entry)) continue; - if (typeof value === "string" || value === null) existing.set(slug, value); + if (typeof value === "string" || value === null) { + existing.set(slug, { value, root: entry[AUTO_REVIEW_ROOT_MARKER] === true }); + } } for (const entry of models) { const slug = typeof entry.slug === "string" ? entry.slug : undefined; if (!slug || isRoutedCatalogEntry(entry) || !existing.has(slug)) continue; - entry.auto_review_model_override = existing.get(slug) ?? null; + const saved = existing.get(slug)!; + entry.auto_review_model_override = saved.value; + if (saved.root) entry[AUTO_REVIEW_ROOT_MARKER] = true; + else delete entry[AUTO_REVIEW_ROOT_MARKER]; } } +/** Stamp a root-derived override and mark native rows so later root removal is durable. */ +function stampRootAutoReviewOverride(entry: RawEntry, target: string): void { + entry.auto_review_model_override = target; + if (!isRoutedCatalogEntry(entry)) entry[AUTO_REVIEW_ROOT_MARKER] = true; +} + +/** Stamp a provider-derived override; provider stamps never fall under root removal. */ +function stampProviderAutoReviewOverride(entry: RawEntry, target: string): void { + entry.auto_review_model_override = target; + delete entry[AUTO_REVIEW_ROOT_MARKER]; +} + export function applyAutoReviewModelOverride( models: RawEntry[] | undefined, autoReviewModel: string | null | undefined, @@ -1695,21 +1735,207 @@ export function applyAutoReviewModelOverride( } for (const entry of models) { if (entry && typeof entry === "object") { - entry.auto_review_model_override = trimmed; + stampRootAutoReviewOverride(entry, trimmed); } } return "applied"; } +/** Validated provider-scoped target with both the configured spelling and catalog slug. */ +interface ValidProviderReviewTarget { + configured: string; + target: string; +} + +/** One provider's resolved provider-wide and per-model auto-review targets. */ +interface ProviderReviewPlan { + wide?: ValidProviderReviewTarget; + perModel: Map; +} + +/** Public provider namespace of a routed catalog row, when it has one. */ +function catalogEntryProviderName(entry: RawEntry): string | undefined { + const slug = typeof entry.slug === "string" ? entry.slug : ""; + const slash = slug.indexOf("/"); + return slash > 0 && isRoutedCatalogEntry(entry) ? slug.slice(0, slash) : undefined; +} + +/** Encoded model-id segment of a routed catalog row, when it has one. */ +function catalogEntryModelSegment(entry: RawEntry): string | undefined { + const slug = typeof entry.slug === "string" ? entry.slug : ""; + const slash = slug.indexOf("/"); + return slash > 0 ? slug.slice(slash + 1) : undefined; +} + +/** Case-insensitive encoded key used to match per-model override maps. */ +function providerModelKey(modelId: string): string { + return canonicalAutoReviewModelKey(modelId); +} + +/** Resolve one configured target against the assembled catalog; bare values name a model of the same provider. */ +function resolveProviderReviewTarget( + models: readonly RawEntry[], + provider: string, + configuredRaw: unknown, +): { kind: "valid"; value: ValidProviderReviewTarget } | { kind: "invalid"; configured: string } | { kind: "unresolved"; configured: string } | { kind: "absent" } { + if (typeof configuredRaw !== "string") return { kind: "absent" }; + const configured = configuredRaw.trim(); + if (!configured) return { kind: "absent" }; + if (!isValidAutoReviewModel(configured)) return { kind: "invalid", configured }; + const prefix = `${provider}/`; + let match: RawEntry | undefined; + const sameProviderCandidate = (rawModelId: string): RawEntry | undefined => models.find(entry => { + if (!isRoutedCatalogEntry(entry) || typeof entry.slug !== "string" || !entry.slug.startsWith(prefix)) return false; + const segment = catalogEntryModelSegment(entry); + return segment !== undefined && segment === encodeRoutedModelId(rawModelId); + }); + // A bare selector names a model of this provider. A full selector that resolves in the + // assembled catalog already names the exact row, including a same-provider encoded slug. + if (!configured.includes("/")) { + match = sameProviderCandidate(configured); + } + match ??= configuredCatalogEntry(models, configured); + if (!match && configured.startsWith(prefix)) { + match = sameProviderCandidate(configured.slice(prefix.length)); + } + if (!match) { + // A raw model id may itself contain "/" (for example zenmux moonshotai/kimi-k3). + // After the full-selector lookup misses, try that spelling as a same-provider id. + match = sameProviderCandidate(configured); + } + if (!match) return { kind: "unresolved", configured }; + const target = typeof match.slug === "string" ? match.slug : configured; + return { kind: "valid", value: { configured, target } }; +} + +/** Build resolved per-provider plans and emit one diagnostic per bad selector. */ +function buildProviderReviewPlans( + models: readonly RawEntry[], + config: Pick, +): { plans: Map; failure?: "invalid" | "unresolved" } { + const plans = new Map(); + let failure: "invalid" | "unresolved" | undefined; + const warned = new Set(); + const recordFailure = (kind: "invalid" | "unresolved", provider: string, configured: string): void => { + const signature = `${provider}\u0000${configured}`; + if (warned.has(signature)) return; + warned.add(signature); + warnProviderAutoReviewModelDiagnostic(kind, provider, configured); + failure ??= kind; + }; + for (const [name, provider] of Object.entries(config.providers ?? {})) { + if (provider.autoReviewModel === undefined && provider.autoReviewModelOverrides === undefined) continue; + const plan: ProviderReviewPlan = { perModel: new Map() }; + if (provider.autoReviewModel !== undefined) { + const resolved = resolveProviderReviewTarget(models, name, provider.autoReviewModel); + if (resolved.kind === "valid") plan.wide = resolved.value; + else if (resolved.kind !== "absent") recordFailure(resolved.kind, name, resolved.configured); + } + if (provider.autoReviewModelOverrides !== undefined) { + for (const [modelId, rawTarget] of Object.entries(provider.autoReviewModelOverrides)) { + const resolved = resolveProviderReviewTarget(models, name, rawTarget); + if (resolved.kind === "valid") { + plan.perModel.set(providerModelKey(modelId), resolved.value); + } else if (resolved.kind !== "absent") { + recordFailure(resolved.kind, name, resolved.configured); + } + } + } + if (plan.wide !== undefined || plan.perModel.size > 0) plans.set(name, plan); + } + return { plans, failure }; +} + +/** Apply or clear the root selector only on rows without a provider stamp. */ +function applyRootSelectorToRemaining( + models: readonly RawEntry[], + rootValue: string | null | undefined, + providerStamped: ReadonlySet, +): AutoReviewModelOverrideResult { + const clearRemaining = (): void => { + for (const entry of models) { + if (!entry || providerStamped.has(entry)) continue; + // Native rows written by releases before the root marker cannot be told apart from + // upstream values once provider stamps diverge. The no-provider path keeps the legacy + // whole-catalog heuristic; provider-scoped configurations restamp native rows whenever a + // root selector is present, so only a simultaneous upgrade-plus-removal needs a manual sync. + if (isRoutedCatalogEntry(entry) || entry[AUTO_REVIEW_ROOT_MARKER] === true) clearAutoReviewOverrideValue(entry); + } + }; + if (rootValue === null || rootValue === undefined) { + clearRemaining(); + return "absent"; + } + const trimmed = rootValue.trim(); + if (!trimmed) { + clearRemaining(); + return "absent"; + } + if (!isValidAutoReviewModel(trimmed)) { + clearRemaining(); + warnAutoReviewModelDiagnostic("invalid", trimmed); + return "invalid"; + } + if (!configuredCatalogEntry(models, trimmed)) { + clearRemaining(); + warnAutoReviewModelDiagnostic("unresolved", trimmed); + return "unresolved"; + } + for (const entry of models) { + if (!entry || providerStamped.has(entry)) continue; + stampRootAutoReviewOverride(entry, trimmed); + } + return "applied"; +} + +/** Provider-aware variant: provider rows win and the root selector is the fallback. */ +export function applyConfiguredAutoReviewModelOverride( + models: RawEntry[] | undefined, + rootAutoReviewModel: string | null | undefined, + config: Pick, +): AutoReviewModelOverrideResult { + if (!models || !Array.isArray(models)) return "absent"; + const { plans, failure } = buildProviderReviewPlans(models, config); + const providerStamped = new Set(); + for (const entry of models) { + if (!entry || typeof entry !== "object") continue; + const provider = catalogEntryProviderName(entry); + if (!provider) continue; + const plan = plans.get(provider); + if (!plan) continue; + const modelSegment = catalogEntryModelSegment(entry); + const perModel = modelSegment === undefined ? undefined : plan.perModel.get(providerModelKey(modelSegment)); + const selected = perModel ?? plan.wide; + if (selected) stampProviderAutoReviewOverride(entry, selected.target); + if (selected || (perModel !== undefined)) providerStamped.add(entry); + } + const rootResult = applyRootSelectorToRemaining(models, rootAutoReviewModel, providerStamped); + const providerApplied = [...providerStamped].some(entry => typeof entry.auto_review_model_override === "string"); + if (providerApplied) { + if (rootResult === "invalid" || rootResult === "unresolved") return rootResult; + return failure ?? "applied"; + } + return failure ?? rootResult; +} + +/** True when any provider row configures a provider-scoped auto-review selector. */ +function configHasProviderAutoReview(config: Pick): boolean { + return Object.values(config.providers ?? {}).some(provider => + provider.autoReviewModel !== undefined || provider.autoReviewModelOverrides !== undefined); +} + /** Apply the root Codex auto-review selector after the final catalog merge. */ export function finalizeAutoReviewModelOverride( models: RawEntry[] | undefined, sourceModels: readonly RawEntry[] = [], + config?: Pick, ): AutoReviewModelOverrideResult { if (models && sourceModels.length > 0) preserveNativeAutoReviewModelOverrides(models, sourceModels); + if (config && configHasProviderAutoReview(config)) { + return applyConfiguredAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), config); + } return applyAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), sourceModels); } - /** * Why an account-gated native model stopped being offered, but only when the answer is one the * operator can act on. @@ -2028,7 +2254,7 @@ function writeRetainedCatalogSync({ }, }); clampCatalogModelsToCodexSupport(catalog.models); - finalizeAutoReviewModelOverride(catalog.models, catalogModelsForMerge); + finalizeAutoReviewModelOverride(catalog.models, catalogModelsForMerge, config); const added = goEntries.length + accountBoundEntries.length; const content = `${JSON.stringify(catalog, null, 2)}\n`; diff --git a/src/codex/convergence.ts b/src/codex/convergence.ts index 2b8a8512c6..9c844fad4a 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -385,7 +385,7 @@ function prepareCatalog( ? supportedCodexReasoningEffortsFromObservedCatalog(source.runtimeSupport.catalog) : null, ); - finalizeAutoReviewModelOverride(mergedModels, catalogModels); + finalizeAutoReviewModelOverride(mergedModels, catalogModels, config); catalog.models = mergedModels; return catalog; } diff --git a/src/config.ts b/src/config.ts index 5e81a7e5f1..8c658b0845 100644 --- a/src/config.ts +++ b/src/config.ts @@ -14,8 +14,11 @@ import { pinnedReasoningEffortConfigError, modelAdapterRecordConfigError, modelDisplayNamesConfigError, + autoReviewModelOverridesConfigError, + autoReviewModelTargetConfigError, nonBlankStringArrayConfigError, normalizeNonBlankStringArray, + normalizeAutoReviewModelOverrides, positiveIntegerConfigError, positiveIntegerRecordConfigError, providerBaseUrlConfigError, @@ -573,6 +576,20 @@ const modelPinnedEffortsSchema = z.unknown().superRefine((value, ctx) => { Object.entries(value as Record).map(([key, effort]) => [key.trim(), effort]), )); +const autoReviewModelSchema = z.unknown().superRefine((value, ctx) => { + const error = autoReviewModelTargetConfigError(value, "autoReviewModel", true); + if (error) ctx.addIssue({ code: "custom", message: error }); +}).transform(value => { + if (typeof value !== "string") return undefined; + const trimmed = value.trim(); + return trimmed ? trimmed : undefined; +}); + +const autoReviewModelOverridesSchema = z.unknown().superRefine((value, ctx) => { + const error = autoReviewModelOverridesConfigError(value, "autoReviewModelOverrides", true); + if (error) ctx.addIssue({ code: "custom", message: error }); +}).transform(value => normalizeAutoReviewModelOverrides(value)); + /** * Zod schema for one provider entry: known fields are validated strictly while unknown * fields pass through (preserved for runtime extensions). @@ -584,6 +601,8 @@ const providerConfigSchema = z.object({ // load silently and then be ignored at selection time, which reads as a broken feature // rather than a rejected setting. apiKeyPoolStrategy: z.enum(["round-robin", "fill-first", "quota"]).optional(), + autoReviewModel: autoReviewModelSchema.optional(), + autoReviewModelOverrides: autoReviewModelOverridesSchema.optional(), adapter: z.string().min(1), baseUrl: z.string().min(1), alias: z.string().optional(), @@ -665,9 +684,12 @@ export { apiKeyTransportConfigError, booleanRecordConfigError, modelAdapterRecordConfigError, + autoReviewModelOverridesConfigError, + autoReviewModelTargetConfigError, modelDisplayNamesConfigError, nonBlankStringArrayConfigError, normalizeNonBlankStringArray, + normalizeAutoReviewModelOverrides, positiveIntegerConfigError, positiveIntegerRecordConfigError, providerBaseUrlConfigError, @@ -1946,6 +1968,44 @@ function sanitizeModelCostsForLoad(parsed: unknown): void { } } +/** + * Load-time degradation for provider-scoped auto-review selectors. A malformed + * hand edit must not fail the whole config parse; the management boundary stays + * strict and rejects the same shapes before they can be written. + */ +function sanitizeAutoReviewForLoad(parsed: unknown): void { + if (!parsed || typeof parsed !== "object") return; + const root = parsed as Record; + const providers = root.providers; + if (!providers || typeof providers !== "object" || Array.isArray(providers)) return; + for (const [name, providerValue] of Object.entries(providers as Record)) { + if (!providerValue || typeof providerValue !== "object" || Array.isArray(providerValue)) continue; + const provider = providerValue as Record; + const safeProviderName = JSON.stringify(redactSecretString(name)); + if (name === "openai") { + delete provider.autoReviewModel; + delete provider.autoReviewModelOverrides; + continue; + } + if (provider.autoReviewModel !== undefined + && autoReviewModelTargetConfigError(provider.autoReviewModel, "autoReviewModel", true) !== null) { + console.warn(`⚠️ config.json providers.${safeProviderName}.autoReviewModel is invalid — ignoring the selector`); + delete provider.autoReviewModel; + } + if (provider.autoReviewModelOverrides !== undefined) { + const overridesError = autoReviewModelOverridesConfigError( + provider.autoReviewModelOverrides, + "autoReviewModelOverrides", + true, + ); + if (overridesError) { + console.warn(`⚠️ config.json providers.${safeProviderName}.autoReviewModelOverrides is invalid — ignoring the map`); + delete provider.autoReviewModelOverrides; + } + } + } +} + /** * Companion to {@link warnDegradedStreamMode} for a blank persisted `hostname`. The bind * falls back to loopback, which is the safe direction but not what the file asked for — @@ -2401,6 +2461,7 @@ export function loadConfig(): OcxConfig { sanitizeAliasesForLoad(parsed); sanitizeReasoningPinsForLoad(parsed); sanitizeModelDisplayNamesForLoad(parsed); + sanitizeAutoReviewForLoad(parsed); sanitizeRetryOn429ForLoad(parsed); sanitizeModelCostsForLoad(parsed); const result = configSchema.safeParse(parsed); @@ -3025,6 +3086,7 @@ function configDiagnosticsFromRaw(raw: string): ConfigDiagnostics { // schema and send the caller a default-config fallback (the config command could then // persist that fallback over the user's providers/keys). sanitizeModelDisplayNamesForLoad(parsed); + sanitizeAutoReviewForLoad(parsed); sanitizeRetryOn429ForLoad(parsed); sanitizeModelCostsForLoad(parsed); const result = configSchema.safeParse(parsed); diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index c1e60033e8..30ee2a4a44 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -5,6 +5,7 @@ import { MODEL_DISCOVERY_MAX_MODELS, } from "../providers/model-discovery-limits"; import { isDeclaredReasoningEffort, modelRecordValue } from "../reasoning-effort"; +import { encodeRoutedModelId } from "../providers/slug-codec"; import { isWirePinnedModel, MODEL_ADAPTER_OVERRIDE_ALLOWED, @@ -233,6 +234,83 @@ export function modelDisplayNamesConfigError( return null; } +/** Characters that make a Codex catalog selector ambiguous or unrepresentable. */ +export const AUTO_REVIEW_MODEL_CONTROL_CHARS = /[\u0000-\u001f\u007f-\u009f\u2028\u2029\s]/; + +/** Validate one auto-review target (provider-wide value or map value). */ +export function autoReviewModelTargetConfigError( + value: unknown, + field = "autoReviewModel", + allowClear = false, +): string | null { + if (value === undefined || (allowClear && (value === null || value === ""))) return null; + if (typeof value !== "string") return `${field} must be a string`; + const trimmed = value.trim(); + if (!trimmed) return `${field} must be nonblank`; + if (trimmed.length > 1024 || AUTO_REVIEW_MODEL_CONTROL_CHARS.test(trimmed)) { + return `${field} must be a catalog selector without whitespace or control characters`; + } + return null; +} + +/** True when the value is a valid Codex catalog auto-review selector. */ +export function isValidAutoReviewModel(value: unknown): value is string { + return typeof value === "string" && autoReviewModelTargetConfigError(value) === null; +} + +/** Canonical model key used for map matching, duplicate detection, and route tombstones. */ +export function canonicalAutoReviewModelKey(modelId: string): string { + return encodeRoutedModelId(modelId.trim()).toLowerCase(); +} + +/** Validate a per-model auto-review override map. */ +export function autoReviewModelOverridesConfigError( + value: unknown, + field = "autoReviewModelOverrides", + allowTombstones = false, +): string | null { + if (value === undefined) return null; + if (value === null && allowTombstones) 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 > MODEL_DISCOVERY_MAX_MODELS) { + return `${field} must contain at most ${MODEL_DISCOVERY_MAX_MODELS} entries`; + } + const canonicalKeys = new Set(); + for (const [modelId, target] of entries) { + if (!isValidModelDiscoveryModelId(modelId)) return `${field} keys must be valid model ids`; + const safeModelId = JSON.stringify(redactSecretString(modelId)); + const canonicalKey = canonicalAutoReviewModelKey(modelId); + if (canonicalKeys.has(canonicalKey)) { + return `${field} keys must be unique after trimming and slash normalization`; + } + canonicalKeys.add(canonicalKey); + if (allowTombstones && (target === null || target === "")) continue; + const targetError = autoReviewModelTargetConfigError(target, `${field}.${safeModelId}`); + if (targetError) return targetError; + } + return null; +} + +/** Normalize a persisted auto-review override map (trim, drop blanks, keep insertion order). */ +export function normalizeAutoReviewModelOverrides(value: unknown): Record | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) return undefined; + const out = Object.create(null) as Record; + for (const [modelId, target] of Object.entries(value)) { + const key = modelId.trim(); + if (!key) continue; + if (target === null || typeof target !== "string") continue; + const trimmed = target.trim(); + if (!trimmed) continue; + out[key] = trimmed; + } + return Object.keys(out).length > 0 ? out : undefined; +} + /** Validate the management DTO boundary for the opt-in empty-tool-output annotation. */ export function providerEmptyToolOutputConfigError(name: string, provider: unknown): string | null { const raw = provider as Record | null | undefined; diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index 0910698a0c..d9cd3c57b1 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -13,6 +13,8 @@ import { } from "../config"; import { apiKeyTransportConfigError, + autoReviewModelOverridesConfigError, + autoReviewModelTargetConfigError, booleanRecordConfigError, providerReasoningPinsConfigError, modelAdapterRecordConfigError, @@ -590,6 +592,13 @@ export function providerManagementConfigError(name: unknown, provider: unknown): const raw = provider as Record; const pinsError = providerReasoningPinsConfigError(raw); if (pinsError) return pinsError; + if (name === "openai" && (Object.hasOwn(raw, "autoReviewModel") || Object.hasOwn(raw, "autoReviewModelOverrides"))) { + return "provider openai must not include autoReviewModel or autoReviewModelOverrides"; + } + const autoReviewTargetError = autoReviewModelTargetConfigError(raw.autoReviewModel, "autoReviewModel", true); + if (autoReviewTargetError) return autoReviewTargetError; + const autoReviewMapError = autoReviewModelOverridesConfigError(raw.autoReviewModelOverrides, "autoReviewModelOverrides", true); + if (autoReviewMapError) return autoReviewMapError; for (const field of FORBIDDEN_PROVIDER_RUNTIME_FIELDS) { if (Object.hasOwn(raw, field)) return `provider ${name} must not include runtime field "${field}"`; } @@ -855,6 +864,8 @@ const PROVIDER_CONFIG_FIELD_POLICY = { modelDefaultReasoningEfforts: "editor", pinnedReasoningEffort: "editor", modelPinnedReasoningEfforts: "editor", + autoReviewModel: "editor", + autoReviewModelOverrides: "editor", modelSupportsReasoningSummaries: "editor", modelSupportsVerbosity: "editor", supportsVerbosity: "editor", diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 6977a17783..a1fba73a49 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -15,6 +15,7 @@ import { mutatePersistedConfig, nonBlankStringArrayConfigError, normalizeNonBlankStringArray, + normalizeAutoReviewModelOverrides, providerBaseUrlConfigError, providerHeadersConfigError, requestPacingConfigError, @@ -34,7 +35,7 @@ import { upsertOAuthProvider, } from "../../oauth"; import { captureConfigTopLevelRollback } from "../../config/rebase-provenance"; -import { mergeModelPinnedEfforts, modelPinnedEffortsConfigError, pinnedReasoningEffortConfigError } from "../../config/provider-validation"; +import { canonicalAutoReviewModelKey, mergeModelPinnedEfforts, modelPinnedEffortsConfigError, pinnedReasoningEffortConfigError } from "../../config/provider-validation"; import { replaceProviderAccountSet } from "../../oauth/store"; import { providerDestinationResolvedError } from "../../lib/destination-policy"; import { reconcileLiveStateStores } from "../../lib/state-store-registrations"; @@ -518,6 +519,56 @@ function applyProviderPatchFields( if (error) return { error }; touched = true; } + if (Object.hasOwn(rawBody, "autoReviewModel")) { + const value = rawBody.autoReviewModel; + if (value === null || value === "") { + delete next.autoReviewModel; + } else if (typeof value === "string" && value.trim()) { + next.autoReviewModel = value.trim(); + } else { + return { error: "autoReviewModel must be a catalog selector string or null" }; + } + touched = true; + } + if (Object.hasOwn(rawBody, "autoReviewModelOverrides")) { + const value = rawBody.autoReviewModelOverrides; + if (value === null) { + delete next.autoReviewModelOverrides; + } else if (isPlainRecord(value)) { + const merged: Record = { ...(next.autoReviewModelOverrides ?? {}) }; + const existingByCanonical = new Map(); + for (const existingKey of Object.keys(merged)) { + existingByCanonical.set(canonicalAutoReviewModelKey(existingKey), existingKey); + } + const submittedCanonicalKeys = new Set(); + for (const [model, target] of Object.entries(value)) { + const key = model.trim(); + const canonicalKey = canonicalAutoReviewModelKey(model); + if (target === null || target === "") { + const previousKey = existingByCanonical.get(canonicalKey); + if (previousKey !== undefined) delete merged[previousKey]; + if (Object.hasOwn(merged, key)) delete merged[key]; + continue; + } + if (typeof target !== "string" || !target.trim()) { + return { error: "autoReviewModelOverrides values must be catalog selectors, null, or empty to remove" }; + } + if (submittedCanonicalKeys.has(canonicalKey)) { + return { error: "autoReviewModelOverrides keys must be unique after trimming and slash normalization" }; + } + submittedCanonicalKeys.add(canonicalKey); + const previousKey = existingByCanonical.get(canonicalKey); + if (previousKey !== undefined && previousKey !== key) delete merged[previousKey]; + merged[key] = target.trim(); + existingByCanonical.set(canonicalKey, key); + } + if (Object.keys(merged).length > 0) next.autoReviewModelOverrides = merged; + else delete next.autoReviewModelOverrides; + } else { + return { error: "autoReviewModelOverrides must be a plain object or null" }; + } + touched = true; + } if (Object.hasOwn(rawBody, "modelAutoCompactTokenLimits")) { const value = rawBody.modelAutoCompactTokenLimits; const error = modelAutoCompactTokenLimitsConfigError(value, { @@ -1069,6 +1120,29 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise; + /** + * Provider-wide auto-review (approval) model for routed models of this provider. + * + * The value is a catalog selector: either a bare model id of this provider + * (for example `deepseek-v4-flash`) or a full public slug (for example + * `opencode-go/deepseek-v4-flash`). During catalog synchronization the + * selector is resolved against the final catalog and stamped as + * `auto_review_model_override` on each routed row of this provider that has + * no per-model override. The root Codex `auto_review_model` remains the + * fallback for every row without a provider stamp. Null or blank clears the + * provider-wide stamp; see `autoReviewModelOverrides` for per-model targets. + */ + autoReviewModel?: string; + /** + * Per-model auto-review (approval) overrides for routed models of this + * provider. Keys are exact upstream model ids under this provider (either + * spelling of a slash-containing id is accepted). Each value is a catalog + * selector with the same meaning as `autoReviewModel`; an entry wins over + * the provider-wide value for its model. Null or blank entries remove the + * model from the map while preserving other entries. + */ + autoReviewModelOverrides?: Record; headers?: Record; /** Default provider-routing preferences for models sent through the canonical OpenRouter API. */ openRouterRouting?: OpenRouterProviderRouting; diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index 794a92bd50..735d815d24 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7293,6 +7293,148 @@ describe("auto_review_model configuration (#1225)", () => { expect(entries[1].auto_review_model_override).toBe(trimmedValue); }); }); + +describe("provider-level auto_review_model overrides", () => { + const { applyConfiguredAutoReviewModelOverride } = require("../../src/codex/catalog/sync"); + + function entries(): Array> { + return [ + { slug: "gpt-5.6-terra", auto_review_model_override: null }, + { slug: "blsc/glm-5.2", auto_review_model_override: null }, + { slug: "blsc/kimi-k3", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-flash", auto_review_model_override: null }, + ]; + } + + function config(providerOverrides?: Record): { providers: Record> } { + return { + providers: { + blsc: { + adapter: "openai-chat", + baseUrl: "https://example.invalid/v1", + ...providerOverrides, + }, + }, + }; + } + + test("provider-wide selector stamps every routed row of that provider", () => { + const models = entries(); + const result = applyConfiguredAutoReviewModelOverride( + models, + null, + config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }), + ); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "blsc/kimi-k3")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override).toBeNull(); + expect(models.find(row => row.slug === "opencode-go/deepseek-v4-flash")?.auto_review_model_override).toBeNull(); + }); + + test("bare same-provider selector is normalized to the provider/model row", () => { + const models = [...entries(), { slug: "other/glm-5.2", auto_review_model_override: null }]; + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ + autoReviewModel: "glm-5.2", + })); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("blsc/glm-5.2"); + expect(models.find(row => row.slug === "blsc/kimi-k3")?.auto_review_model_override) + .toBe("blsc/glm-5.2"); + expect(models.find(row => row.slug === "other/glm-5.2")?.auto_review_model_override).toBeNull(); + }); + + test("per-model override wins over the provider-wide target", () => { + const models = entries(); + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ + autoReviewModel: "gpt-5.6-terra", + autoReviewModelOverrides: { + "kimi-k3": "opencode-go/deepseek-v4-flash", + }, + })); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/kimi-k3")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("gpt-5.6-terra"); + }); + + test("a raw model id containing a slash resolves against the same provider row", () => { + const models = [ + { slug: "zenmux/moonshotai-kimi-k3", auto_review_model_override: null }, + { slug: "zenmux/other-model", auto_review_model_override: null }, + { slug: "blsc/moonshotai-kimi-k3", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, { + providers: { + zenmux: { + adapter: "openai-chat", + baseUrl: "https://zenmux.example.test/v1", + autoReviewModel: "moonshotai/kimi-k3", + }, + }, + }); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "zenmux/moonshotai-kimi-k3")?.auto_review_model_override) + .toBe("zenmux/moonshotai-kimi-k3"); + expect(models.find(row => row.slug === "zenmux/other-model")?.auto_review_model_override) + .toBe("zenmux/moonshotai-kimi-k3"); + expect(models.find(row => row.slug === "blsc/moonshotai-kimi-k3")?.auto_review_model_override).toBeNull(); + }); + + test("partial provider failure is reported even when valid stamps are applied", () => { + const models = entries(); + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ + autoReviewModel: "opencode-go/deepseek-v4-flash", + autoReviewModelOverrides: { "kimi-k3": "missing/reviewer" }, + })); + expect(result).toBe("unresolved"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + }); + + test("root selector remains the fallback and provider stamps survive root removal", () => { + const models = entries(); + const providerConfig = config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }); + applyConfiguredAutoReviewModelOverride(models, "gpt-5.6-terra", providerConfig); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override) + .toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "opencode-go/deepseek-v4-flash")?.auto_review_model_override) + .toBe("gpt-5.6-terra"); + + applyConfiguredAutoReviewModelOverride(models, null, providerConfig); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override).toBeNull(); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "opencode-go/deepseek-v4-flash")?.auto_review_model_override).toBeNull(); + }); + + test("removing the provider selector clears its previous stamps", () => { + const models = entries(); + const providerConfig = config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }); + applyConfiguredAutoReviewModelOverride(models, null, providerConfig); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + applyConfiguredAutoReviewModelOverride(models, null, config()); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override).toBeNull(); + expect(models.find(row => row.slug === "blsc/kimi-k3")?.auto_review_model_override).toBeNull(); + }); + + test("unresolved provider selector fails closed without falling back to the root", () => { + const models = entries(); + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ + autoReviewModel: "missing/reviewer", + })); + expect(result).toBe("unresolved"); + expect(models.every(row => row.auto_review_model_override === null)).toBe(true); + }); +}); + import { ManagementRequest as Request } from "../helpers/management-auth"; describe("#2465 model preset management routes", () => { diff --git a/tests/codex-integration/codex-convergence-account-selectors.test.ts b/tests/codex-integration/codex-convergence-account-selectors.test.ts index 1586978e46..a3f8c62956 100644 --- a/tests/codex-integration/codex-convergence-account-selectors.test.ts +++ b/tests/codex-integration/codex-convergence-account-selectors.test.ts @@ -736,6 +736,58 @@ test("retained and convergence writers resolve, clear, reject, and recover auto- } }); +test("provider-scoped auto-review overrides win on routed rows in both writers", async () => { + primeCodexRuntimeFixture(); + + for (const writer of ["retained", "convergence"] as const) { + const write = async (nextConfig: OcxConfig): Promise => { + if (writer === "retained") { + const result = await syncCatalogModels(nextConfig); + expect(result.catalogWritten).toBe(true); + } else { + const disposition = await convergeCatalogDisposition(nextConfig); + expect(disposition).toMatchObject({ status: "committed" }); + } + return JSON.parse(readFileSync(catalogPath, "utf8")) as RawCatalog; + }; + + const nextConfig = autoReviewConfig(["deepseek-v4-flash", "glm-5.2"]); + nextConfig.providers.static!.autoReviewModel = "deepseek-v4-flash"; + nextConfig.providers.static!.autoReviewModelOverrides = { + "glm-5.2": "gpt-5.5", + }; + + writeAutoReviewModel("gpt-5.5"); + writeCatalog([ + nativeEntry(), + generatedRoutedEntry("static/deepseek-v4-flash"), + generatedRoutedEntry("static/glm-5.2"), + ]); + let catalog = await write(nextConfig); + expect(catalog.models?.find(entry => entry.slug === "static/deepseek-v4-flash")) + .toHaveProperty("auto_review_model_override", "static/deepseek-v4-flash"); + expect(catalog.models?.find(entry => entry.slug === "static/glm-5.2")) + .toHaveProperty("auto_review_model_override", "gpt-5.5"); + expect(catalog.models?.find(entry => entry.slug === "gpt-5.5")) + .toHaveProperty("auto_review_model_override", "gpt-5.5"); + + // Removing the provider-scoped fields restores root fallback: routed rows get the + // root selector again instead of a stale provider stamp. + delete nextConfig.providers.static!.autoReviewModel; + delete nextConfig.providers.static!.autoReviewModelOverrides; + writeCatalog([ + nativeEntry(), + generatedRoutedEntry("static/deepseek-v4-flash"), + generatedRoutedEntry("static/glm-5.2"), + ]); + catalog = await write(nextConfig); + expect(catalog.models?.find(entry => entry.slug === "static/deepseek-v4-flash")) + .toHaveProperty("auto_review_model_override", "gpt-5.5"); + expect(catalog.models?.find(entry => entry.slug === "static/glm-5.2")) + .toHaveProperty("auto_review_model_override", "gpt-5.5"); + } +}); + test("degraded preservation still honors explicit routed visibility policy", async () => { writeCatalog([ nativeEntry(), diff --git a/tests/config/config-load-degrade.test.ts b/tests/config/config-load-degrade.test.ts index 4d8cca68f7..c85ab5f9cb 100644 --- a/tests/config/config-load-degrade.test.ts +++ b/tests/config/config-load-degrade.test.ts @@ -54,6 +54,23 @@ function writeCandidate(modelDisplayNames: unknown, provider = "xai"): void { writeFileSync(getConfigPath(), JSON.stringify(config), "utf8"); } +function writeAutoReviewConfig(autoReviewModel: unknown, autoReviewModelOverrides: unknown): void { + const defaults = getDefaultConfig(); + writeFileSync(getConfigPath(), JSON.stringify({ + ...defaults, + defaultProvider: "xai", + providers: { + xai: { + adapter: "openai-responses", + baseUrl: "https://api.x.ai/v1", + note: "keep me", + autoReviewModel, + autoReviewModelOverrides, + }, + }, + }), "utf8"); +} + test("config validation accepts only safe provider model display names", () => { const valid = validateConfigCandidate(candidate({ "grok-4.6": "Grok 4.6", @@ -129,6 +146,25 @@ test("load warnings never reveal display values or secret shaped provider names" }); +test("load ignores malformed auto-review selectors without dropping the provider", () => { + writeAutoReviewConfig("bad selector", { model: "bad selector" }); + + const loaded = loadConfig(); + + expect(loaded.providers.xai).toMatchObject({ note: "keep me" }); + expect(loaded.providers.xai.autoReviewModel).toBeUndefined(); + expect(loaded.providers.xai.autoReviewModelOverrides).toBeUndefined(); +}); + +test("load preserves valid auto-review selectors and trims boundary whitespace", () => { + writeAutoReviewConfig(" openai/gpt-test ", { "glm-5.2": " gpt-test " }); + + const loaded = loadConfig(); + + expect(loaded.providers.xai.autoReviewModel).toBe("openai/gpt-test"); + expect(loaded.providers.xai.autoReviewModelOverrides).toEqual({ "glm-5.2": "gpt-test" }); +}); + test("Fast rows default on for fresh and omitted config; explicit false and malformed values disable", () => { expect(getDefaultConfig().fastRows).toBe(true); for (const [value, expected] of [[undefined, true], [true, true], [false, false], ["invalid", false]] as const) { diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index 949791f2d6..18031c8ac3 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -30,7 +30,11 @@ import { } from "../../src/server"; import { handleManagementAPI } from "../../src/server/management-api"; import { providerEditorConfigDTO, providerManagementConfigError } from "../../src/server/auth-cors"; -import { providerEmptyToolOutputConfigError } from "../../src/config/provider-validation"; +import { + autoReviewModelOverridesConfigError, + autoReviewModelTargetConfigError, + providerEmptyToolOutputConfigError, +} from "../../src/config/provider-validation"; import { providerServiceTierConfigError, withProviderServiceTierDTO } from "../../src/server/management/provider-capability-config"; import { clearModelCache, markProviderDiscoveryFailed, markProviderDiscoveryOk } from "../../src/codex/model-cache"; import { @@ -675,6 +679,151 @@ describe("provider management validation", () => { } }); + test("provider management validates and patches auto-review selectors", async () => { + expect(autoReviewModelTargetConfigError(" opencode-go/deepseek-v4-flash ")).toBeNull(); + expect(autoReviewModelTargetConfigError("bad slug")).toContain("autoReviewModel"); + expect(autoReviewModelOverridesConfigError({ " model": "gpt-test" })).toContain("keys"); + + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const liveConfig: OcxConfig = { + port: 0, + hostname: "127.0.0.1", + defaultProvider: "relay", + providers: { + relay: { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }, + }, + }; + saveConfig(liveConfig); + const request = async (path: string, init?: RequestInit) => { + const req = new Request(`http://127.0.0.1${path}`, init); + return handleManagementAPI(req, new URL(req.url), liveConfig, { + createManagementConvergeCodex: catalogConvergenceFactory(), + }); + }; + + const reject = await request("/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ autoReviewModelOverrides: { "glm-5.2": "bad target" } }), + }); + expect(reject?.status).toBe(400); + expect(await reject?.json()).toMatchObject({ error: expect.stringContaining("autoReviewModelOverrides") }); + + const set = await request("/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + autoReviewModel: "openai/gpt-test", + autoReviewModelOverrides: { "glm-5.2": "gpt-test" }, + }), + }); + expect(set?.status).toBe(200); + expect(liveConfig.providers.relay?.autoReviewModel).toBe("openai/gpt-test"); + expect(liveConfig.providers.relay?.autoReviewModelOverrides).toEqual({ "glm-5.2": "gpt-test" }); + expect(loadConfig().providers.relay?.autoReviewModelOverrides).toEqual({ "glm-5.2": "gpt-test" }); + + const update = await request("/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ autoReviewModelOverrides: { "GLM-5.2": "gpt-5.6-terra" } }), + }); + expect(update?.status).toBe(200); + expect(liveConfig.providers.relay?.autoReviewModelOverrides).toEqual({ "GLM-5.2": "gpt-5.6-terra" }); + + const remove = await request("/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ autoReviewModelOverrides: { "glm-5.2": null } }), + }); + expect(remove?.status).toBe(200); + expect(liveConfig.providers.relay?.autoReviewModelOverrides).toBeUndefined(); + + const clear = await request("/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ autoReviewModel: null, autoReviewModelOverrides: null }), + }); + expect(clear?.status).toBe(200); + expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModel"); + expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModelOverrides"); + }); + + test("canonical openai provider rejects auto-review fields", async () => { + expect(providerManagementConfigError("openai", { + ...canonicalDirect, + codexAccountMode: "pool", + autoReviewModel: "gpt-test", + })).toContain("autoReviewModel"); + }); + + test("provider POST overwrite preserves auto-review selectors when omitted", async () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const liveConfig: OcxConfig = { + port: 0, + hostname: "127.0.0.1", + defaultProvider: "relay", + providers: { + relay: { + adapter: "openai-chat", + baseUrl: "https://relay.example/v1", + autoReviewModel: "openai/gpt-test", + autoReviewModelOverrides: { "glm-5.2": "gpt-test" }, + }, + }, + }; + saveConfig(liveConfig); + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + try { + const req = new Request("http://127.0.0.1/api/providers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "relay", + provider: { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }, + }), + }); + const response = await handleManagementAPI( + req, + new URL(req.url), + liveConfig, + { createManagementConvergeCodex: catalogConvergenceFactory() }, + ); + expect(response?.status).toBe(200); + expect(liveConfig.providers.relay?.autoReviewModel).toBe("openai/gpt-test"); + expect(liveConfig.providers.relay?.autoReviewModelOverrides).toEqual({ "glm-5.2": "gpt-test" }); + expect(loadConfig().providers.relay?.autoReviewModelOverrides).toEqual({ "glm-5.2": "gpt-test" }); + + const blankReq = new Request("http://127.0.0.1/api/providers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "relay", + provider: { + adapter: "openai-chat", + baseUrl: "https://relay.example/v1", + autoReviewModel: "", + autoReviewModelOverrides: {}, + }, + }), + }); + const blankResponse = await handleManagementAPI( + blankReq, + new URL(blankReq.url), + liveConfig, + { createManagementConvergeCodex: catalogConvergenceFactory() }, + ); + expect(blankResponse?.status).toBe(200); + expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModel"); + expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModelOverrides"); + } finally { + resolvedError.mockRestore(); + } + }); + test("provider management rejects modelCosts rows with extra fields", () => { const error = providerManagementConfigError("blsc", { adapter: "openai-chat", From aaf56474ff90bc0f51151f201560e13f752466d5 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 00:27:27 +0800 Subject: [PATCH 02/14] fix(catalog): drop the dead provider-stamp guard and note API-only fields --- .../src/content/docs/reference/configuration/providers.md | 4 ++++ src/codex/catalog/sync.ts | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index e136b09d58..992b0d02f5 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -300,6 +300,10 @@ place. The canonical `openai` provider does not accept these fields. the whole map; use a map entry of `null` or `""` to remove that model while preserving other entries. Unrelated provider saves preserve previously configured values. +These two fields are reachable today from `config.json` and the provider management API only: +the Dashboard provider editor does not render inputs for them yet, so a browser-only workflow +cannot change them after the fact. + ### Discovered model display names Use `modelDisplayNames` when a provider returns machine friendly ids but the Codex model picker diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index eb3a6ff343..ca0da521cd 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1906,8 +1906,9 @@ export function applyConfiguredAutoReviewModelOverride( const modelSegment = catalogEntryModelSegment(entry); const perModel = modelSegment === undefined ? undefined : plan.perModel.get(providerModelKey(modelSegment)); const selected = perModel ?? plan.wide; - if (selected) stampProviderAutoReviewOverride(entry, selected.target); - if (selected || (perModel !== undefined)) providerStamped.add(entry); + if (!selected) continue; + stampProviderAutoReviewOverride(entry, selected.target); + providerStamped.add(entry); } const rootResult = applyRootSelectorToRemaining(models, rootAutoReviewModel, providerStamped); const providerApplied = [...providerStamped].some(entry => typeof entry.auto_review_model_override === "string"); From c538585e8604a3ed93c1faf5fc257f442a681048 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:02:49 +0800 Subject: [PATCH 03/14] fix(catalog): sweep legacy root stamps in the provider path The provider-scoped path cleared root-derived overrides only from routed rows and rows carrying the new provenance marker, so a catalog written before that marker kept a removed root reviewer on native rows indefinitely. Sweep the legacy whole-catalog signature before provider plans land, reject every clear form of the auto-review fields on the canonical openai PATCH, and document both. --- .../docs/reference/configuration/providers.md | 6 ++ src/codex/catalog/sync.ts | 55 ++++++++++++--- src/server/management/provider-routes.ts | 6 ++ tests/codex-integration/codex-catalog.test.ts | 69 +++++++++++++++++++ .../management-provider-validation.test.ts | 38 ++++++++++ 5 files changed, 163 insertions(+), 11 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 992b0d02f5..d0c323b45e 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -296,6 +296,12 @@ Selectors are resolved against the final catalog on each sync: an unknown target for the override only, emits a diagnostic, and leaves normal upstream auto-review behavior in place. The canonical `openai` provider does not accept these fields. +Removing the root selector clears root stamps from every row, including native rows stamped by +earlier releases that predate OpenCodex's provenance marker. That cleanup recognizes a legacy +stamp by its shape — one value across the whole catalog that a routed row also carries — so a +genuine per-row value matching that shape is cleared with it, and a catalog that has since +diverged from that shape needs one manual sync. Provider stamps are never touched by root removal. + `PATCH /api/providers?name=` accepts both fields. Use `null` to clear the scalar or the whole map; use a map entry of `null` or `""` to remove that model while preserving other entries. Unrelated provider saves preserve previously configured values. diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index ca0da521cd..209c13493b 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1608,11 +1608,13 @@ function clearAutoReviewOverrideValue(entry: RawEntry): void { delete entry[AUTO_REVIEW_ROOT_MARKER]; } -function clearAutoReviewModelOverride( - models: readonly RawEntry[], - sourceModels: readonly RawEntry[] = [], -): void { - const observedModels = [...models, ...sourceModels]; +/** + * Legacy whole-catalog root stamp: releases before AUTO_REVIEW_ROOT_MARKER wrote root stamps that + * are textually identical to an upstream value, so the only way to recognize one is the uniform + * signature the no-provider path relies on — a single value that a routed row also carries. + * Returns the stamped values when the observed rows match that shape. + */ +function legacyRootStampValues(observedModels: readonly RawEntry[]): ReadonlySet | undefined { const configuredValues = new Set(observedModels.flatMap(entry => { const value = entry?.auto_review_model_override; return typeof value === "string" && value.trim() ? [value] : []; @@ -1631,12 +1633,37 @@ function clearAutoReviewModelOverride( || value === undefined || (typeof value === "string" && configuredValues.has(value)); }); + return globalStamp ? configuredValues : undefined; +} + +/** + * Sweep legacy root stamps off the rows a provider plan is about to restamp. + * + * Root removal reaches marker-tagged native rows on its own, but a catalog written before the + * marker only carries the legacy signature — and provider stamping rewrites that signature before + * the root pass could read it, so the sweep has to run first. + */ +function clearLegacyRootStamps(models: readonly RawEntry[], sourceModels: readonly RawEntry[] = []): void { + const legacyStamp = legacyRootStampValues([...models, ...sourceModels]); + if (legacyStamp === undefined) return; + for (const entry of models) { + if (!entry || typeof entry !== "object") continue; + const current = entry.auto_review_model_override; + if (typeof current === "string" && legacyStamp.has(current)) clearAutoReviewOverrideValue(entry); + } +} + +function clearAutoReviewModelOverride( + models: readonly RawEntry[], + sourceModels: readonly RawEntry[] = [], +): void { + const legacyStamp = legacyRootStampValues([...models, ...sourceModels]); for (const entry of models) { if (!entry || typeof entry !== "object") continue; const current = entry.auto_review_model_override; if (isRoutedCatalogEntry(entry) || entry[AUTO_REVIEW_ROOT_MARKER] === true - || (globalStamp && typeof current === "string" && configuredValues.has(current))) { + || (legacyStamp !== undefined && typeof current === "string" && legacyStamp.has(current))) { clearAutoReviewOverrideValue(entry); } } @@ -1855,10 +1882,10 @@ function applyRootSelectorToRemaining( const clearRemaining = (): void => { for (const entry of models) { if (!entry || providerStamped.has(entry)) continue; - // Native rows written by releases before the root marker cannot be told apart from - // upstream values once provider stamps diverge. The no-provider path keeps the legacy - // whole-catalog heuristic; provider-scoped configurations restamp native rows whenever a - // root selector is present, so only a simultaneous upgrade-plus-removal needs a manual sync. + // Native rows written by releases before the root marker cannot be told apart from upstream + // values once provider stamps diverge. clearLegacyRootStamps sweeps the ones the legacy + // uniform signature still recognizes before provider plans land, because provider stamping + // destroys that signature; a catalog that no longer matches it needs a one-off manual sync. if (isRoutedCatalogEntry(entry) || entry[AUTO_REVIEW_ROOT_MARKER] === true) clearAutoReviewOverrideValue(entry); } }; @@ -1893,8 +1920,14 @@ export function applyConfiguredAutoReviewModelOverride( models: RawEntry[] | undefined, rootAutoReviewModel: string | null | undefined, config: Pick, + sourceModels: readonly RawEntry[] = [], ): AutoReviewModelOverrideResult { if (!models || !Array.isArray(models)) return "absent"; + // Runs unconditionally because the sweep only fires on the uniform legacy signature. A resolved + // root selector restamps every row it touches below, so the call is behavior-preserving there; + // with the root absent, invalid, or unresolved those clears are final — which is the point, and + // also the limit: the legacy heuristic cannot tell a root stamp from an identical upstream value. + clearLegacyRootStamps(models, sourceModels); const { plans, failure } = buildProviderReviewPlans(models, config); const providerStamped = new Set(); for (const entry of models) { @@ -1933,7 +1966,7 @@ export function finalizeAutoReviewModelOverride( ): AutoReviewModelOverrideResult { if (models && sourceModels.length > 0) preserveNativeAutoReviewModelOverrides(models, sourceModels); if (config && configHasProviderAutoReview(config)) { - return applyConfiguredAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), config); + return applyConfiguredAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), config, sourceModels); } return applyAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), sourceModels); } diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index a1fba73a49..0485aecc52 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1334,6 +1334,12 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { expect(result).toBe("unresolved"); expect(models.every(row => row.auto_review_model_override === null)).toBe(true); }); + + test("legacy root stamps are swept when provider configuration replaces the root in one step", () => { + // Catalogs written before the provenance marker carry root stamps that look exactly like + // upstream values, so removing the root while adding a provider selector has to fall back to + // the legacy whole-catalog signature — and read it before provider stamping rewrites it. + const sourceModels: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "gpt-5.6-terra" }, + { slug: "gpt-5.5", auto_review_model_override: "gpt-5.6-terra" }, + { slug: "blsc/glm-5.2", auto_review_model_override: "gpt-5.6-terra" }, + { slug: "opencode-go/deepseek-v4-flash", auto_review_model_override: "gpt-5.6-terra" }, + ]; + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "gpt-5.6-terra" }, + { slug: "gpt-5.5", auto_review_model_override: "gpt-5.6-terra" }, + { slug: "blsc/glm-5.2", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-flash", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride( + models, + null, + config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }), + sourceModels, + ); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override).toBeNull(); + expect(models.find(row => row.slug === "gpt-5.5")?.auto_review_model_override).toBeNull(); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.find(row => row.slug === "opencode-go/deepseek-v4-flash")?.auto_review_model_override).toBeNull(); + }); + + test("a catalog with mixed values is not mistaken for a legacy root stamp", () => { + const sourceModels: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "native-reviewer" }, + { slug: "blsc/glm-5.2", auto_review_model_override: "legacy-root" }, + ]; + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "native-reviewer" }, + { slug: "blsc/glm-5.2", auto_review_model_override: null }, + ]; + applyConfiguredAutoReviewModelOverride( + models, + null, + config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }), + sourceModels, + ); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override) + .toBe("native-reviewer"); + }); + + test("a uniform value no routed row carries is not treated as a legacy root stamp", () => { + const sourceModels: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "user-pinned-reviewer" }, + { slug: "gpt-5.5", auto_review_model_override: "user-pinned-reviewer" }, + ]; + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "user-pinned-reviewer" }, + { slug: "gpt-5.5", auto_review_model_override: "user-pinned-reviewer" }, + { slug: "blsc/glm-5.2", auto_review_model_override: null }, + ]; + applyConfiguredAutoReviewModelOverride( + models, + null, + config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }), + sourceModels, + ); + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override) + .toBe("user-pinned-reviewer"); + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index 18031c8ac3..9dfed5ac5f 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1488,6 +1488,44 @@ describe("provider management validation", () => { } }); + test("canonical openai PATCH rejects auto-review fields in every clear form", async () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { openai: { ...canonicalDirect } }, + } as OcxConfig); + const server = startServer(0); + try { + const before = readFileSync(join(TEST_DIR, "config.json")); + // A clear or no-op value would otherwise delete the field from the merged row before the + // canonical-openai guard sees it, answering 200 for a field the provider may not carry. + for (const body of [ + { autoReviewModel: "gpt-test" }, + { autoReviewModel: null }, + { autoReviewModel: "" }, + { autoReviewModelOverrides: null }, + { autoReviewModelOverrides: {} }, + { autoReviewModelOverrides: { "glm-5.2": "" } }, + { autoReviewModelOverrides: { "glm-5.2": null } }, + ]) { + const response = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + expect(response.status).toBe(400); + expect(await response.json()).toMatchObject({ error: expect.stringContaining("autoReviewModel") }); + } + expect(readFileSync(join(TEST_DIR, "config.json"))).toEqual(before); + } finally { + await server.stop(true); + } + }); + test("malformed alias overlays return bounded 4xx without config persistence", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); From cde7acd6728c3fb822221e6346343b967e953324 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:28:19 +0800 Subject: [PATCH 04/14] fix(management): enforce auto-review override key uniqueness for clears A PATCH that cleared one spelling of an override key while setting another resolved in object order instead of being rejected, so the documented uniqueness rule silently depended on key order. Check canonical uniqueness before the tombstone branch. Also pin the catalog-side invariants CodeRabbit asked about: an override key written with a slash matches the encoded routed row, and a bare selector never borrows a sibling provider's row. --- src/server/management/provider-routes.ts | 10 +++--- tests/codex-integration/codex-catalog.test.ts | 35 ++++++++++++++++++ .../management-provider-validation.test.ts | 36 +++++++++++++++++++ 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 0485aecc52..97618443d4 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -544,6 +544,12 @@ function applyProviderPatchFields( for (const [model, target] of Object.entries(value)) { const key = model.trim(); const canonicalKey = canonicalAutoReviewModelKey(model); + // Uniqueness is enforced before the tombstone branch: a clear and a set that normalize to + // the same key would otherwise resolve in object order instead of being rejected. + if (submittedCanonicalKeys.has(canonicalKey)) { + return { error: "autoReviewModelOverrides keys must be unique after trimming and slash normalization" }; + } + submittedCanonicalKeys.add(canonicalKey); if (target === null || target === "") { const previousKey = existingByCanonical.get(canonicalKey); if (previousKey !== undefined) delete merged[previousKey]; @@ -553,10 +559,6 @@ function applyProviderPatchFields( if (typeof target !== "string" || !target.trim()) { return { error: "autoReviewModelOverrides values must be catalog selectors, null, or empty to remove" }; } - if (submittedCanonicalKeys.has(canonicalKey)) { - return { error: "autoReviewModelOverrides keys must be unique after trimming and slash normalization" }; - } - submittedCanonicalKeys.add(canonicalKey); const previousKey = existingByCanonical.get(canonicalKey); if (previousKey !== undefined && previousKey !== key) delete merged[previousKey]; merged[key] = target.trim(); diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index e88975989b..dceea18913 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7502,6 +7502,41 @@ describe("provider-level auto_review_model overrides", () => { expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override) .toBe("user-pinned-reviewer"); }); + + test("an override key written with a slash matches the encoded routed row", () => { + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: null }, + { slug: "zenmux/moonshotai-kimi-k3", auto_review_model_override: null }, + { slug: "zenmux/other-model", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, { + providers: { + zenmux: { + adapter: "openai-chat", + baseUrl: "https://zenmux.example.test/v1", + autoReviewModel: "other-model", + autoReviewModelOverrides: { "moonshotai/kimi-k3": "gpt-5.6-terra" }, + }, + }, + }); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "zenmux/moonshotai-kimi-k3")?.auto_review_model_override) + .toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "zenmux/other-model")?.auto_review_model_override) + .toBe("zenmux/other-model"); + }); + + test("a bare selector never resolves to another provider's routed row", () => { + const models: Array> = [ + { slug: "other/glm-5.2", auto_review_model_override: null }, + { slug: "blsc/kimi-k3", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ autoReviewModel: "glm-5.2" })); + // Bare selectors name this provider's model or a bare catalog row, never a sibling provider's + // encoded slug; an unresolvable one fails closed instead of borrowing the other row. + expect(result).toBe("unresolved"); + expect(models.every(row => row.auto_review_model_override === null)).toBe(true); + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index 9dfed5ac5f..0408848d32 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -750,6 +750,42 @@ describe("provider management validation", () => { expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModelOverrides"); }); + test("a clear sharing a normalized key with a set is rejected instead of racing on order", async () => { + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + const liveConfig: OcxConfig = { + port: 0, + hostname: "127.0.0.1", + defaultProvider: "relay", + providers: { + relay: { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }, + }, + }; + saveConfig(liveConfig); + const request = async (body: Record) => { + const req = new Request("http://127.0.0.1/api/providers?name=relay", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + return handleManagementAPI(req, new URL(req.url), liveConfig, { + createManagementConvergeCodex: catalogConvergenceFactory(), + }); + }; + + for (const overrides of [ + { "glm-5.2": null, "GLM-5.2": "gpt-test" }, + { "GLM-5.2": "gpt-test", "glm-5.2": null }, + { "glm-5.2": null, "GLM-5.2": null }, + ]) { + const response = await request({ autoReviewModelOverrides: overrides }); + expect(response?.status).toBe(400); + expect(await response?.json()).toMatchObject({ error: expect.stringContaining("unique") }); + } + expect(liveConfig.providers.relay).not.toHaveProperty("autoReviewModelOverrides"); + }); + test("canonical openai provider rejects auto-review fields", async () => { expect(providerManagementConfigError("openai", { ...canonicalDirect, From 9b4c4cf09320475a02548052d093c5654f2771e5 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:30:30 +0800 Subject: [PATCH 05/14] docs(providers): move the auto-review workflow into the provider guide The provider guide now owns the operator workflow and the worked example, and the configuration reference keeps the field semantics with a link across, so the two pages stop carrying the same setup text. --- .../src/content/docs/guides/providers.md | 34 +++++++++++++++++++ .../docs/reference/configuration/providers.md | 23 +++---------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index a093f22dc6..aaf6a0fed5 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -999,6 +999,40 @@ dashboard or `custom` in `ocx init` and enter the base URL. See the [Configuration reference](/reference/configuration/) for every provider field (`headers`, `noReasoningModels`, `noVisionModels`, `models`, …). +## Approval reviewer per provider + +Codex asks a second model to review approval requests, and takes that reviewer from +`auto_review_model_override` on the catalog row of the current turn's model. The root +`auto_review_model` in `$CODEX_HOME/config.toml` applies one reviewer to every row. To give a +routed provider its own — usually cheaper — reviewer, set the selector on that provider row in +`~/.opencodex/config.json`: + +```json +{ + "providers": { + "blsc": { + "autoReviewModel": "opencode-go/deepseek-v4-flash", + "autoReviewModelOverrides": { "kimi-k3": "gpt-5.6-terra" } + } + } +} +``` + +`autoReviewModel` covers every routed row of the provider. `autoReviewModelOverrides` targets a +single upstream model id and wins over it. A value is either a bare model id of that same provider +or a public catalog slug such as `opencode-go/deepseek-v4-flash`, and a provider stamp wins over the +root selector on its own rows while the root selector stays the fallback elsewhere. + +Selectors are resolved against the final catalog on the next sync. An unknown target fails closed: +that override is skipped, a diagnostic is printed, and normal upstream auto-review behavior stays +in place. Removing the root selector leaves provider stamps alone, and removing a provider selector +clears only that provider's stamps. + +These fields are configuration/API only — `PATCH /api/providers?name=` accepts them and +the dashboard provider editor does not render inputs for them yet. The canonical `openai` provider +rejects them. Field-by-field rules live in the +[provider configuration reference](/reference/configuration/providers/#auto-review-approval-model-selection). + ## Rate limits in the providers overview The **Rate limits** section of the Providers overview shows live utilization diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index d0c323b45e..88819e69e3 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -264,26 +264,11 @@ optional pin in a hand-edited file is ignored on load without discarding the res Codex reads `auto_review_model_override` from the catalog row of the current turn's model to choose the model that reviews approval requests. The root `auto_review_model` setting in -`$CODEX_HOME/config.toml` applies one reviewer to every catalog row. When different routed -providers should use different (usually cheaper) reviewers, add provider-scoped selectors to the -provider row in `config.json` instead: +`$CODEX_HOME/config.toml` applies one reviewer to every catalog row; the provider-scoped fields +below override it per provider. The [provider guide](/guides/providers/#approval-reviewer-per-provider) +has the operator workflow and a worked example. -```json -{ - "providers": { - "blsc": { - "adapter": "openai-chat", - "baseUrl": "https://llmapi.blsc.cn", - "autoReviewModel": "opencode-go/deepseek-v4-flash", - "autoReviewModelOverrides": { - "kimi-k3": "gpt-5.6-terra" - } - } - } -} -``` - -`autoReviewModel` is the provider-wide target. A value can be a bare model id of that same +`autoReviewModel` is the provider-wide reviewer target. A value can be a bare model id of that same provider (the catalog row is normalized to the `provider/model` slug) or a full public catalog slug such as `opencode-go/deepseek-v4-flash`. `autoReviewModelOverrides` keys are exact upstream model ids of that provider; an entry wins over the provider-wide value for its model. A provider From d0c2d81bd9cecc7062d42f0a90572bc460b04629 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:40:55 +0800 Subject: [PATCH 06/14] docs(catalog): document the auto-review helpers touched by this change CodeRabbit's pre-merge docstring check sat at 76.19%, below the 80% threshold, because the rewritten clear path and the two functions it was extracted from shipped without doc comments. --- src/codex/catalog/sync.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 209c13493b..14299fc0ad 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1596,6 +1596,7 @@ export function isValidAutoReviewModel(value: unknown): value is string { export type AutoReviewModelOverrideResult = "absent" | "applied" | "invalid" | "unresolved"; +/** True when a catalog row was synthesized by opencodex instead of coming from upstream. */ function isRoutedCatalogEntry(entry: RawEntry): boolean { const slug = typeof entry.slug === "string" ? entry.slug : ""; return slug.includes("/") @@ -1653,6 +1654,10 @@ function clearLegacyRootStamps(models: readonly RawEntry[], sourceModels: readon } } +/** + * Clear the root selector from every row this path owns: routed rows, rows stamped by a release + * that writes the provenance marker, and the legacy whole-catalog stamp that predates it. + */ function clearAutoReviewModelOverride( models: readonly RawEntry[], sourceModels: readonly RawEntry[] = [], @@ -1735,6 +1740,10 @@ function stampProviderAutoReviewOverride(entry: RawEntry, target: string): void delete entry[AUTO_REVIEW_ROOT_MARKER]; } +/** + * Apply the root Codex auto-review selector to every catalog row, or clear it when the value is + * absent, blank, malformed, or does not resolve against the assembled catalog. + */ export function applyAutoReviewModelOverride( models: RawEntry[] | undefined, autoReviewModel: string | null | undefined, From 032f2846a0d6402aef4a573b7af428fcf25f1f30 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:50:47 +0800 Subject: [PATCH 07/14] docs(providers): state how bare auto-review selectors resolve A bare value resolves against the configured provider first and then against a bare catalog row, which is what lets a native model such as gpt-5.6-terra be named as a reviewer. Say that outright so the fallback is documented rather than implied. --- docs-site/src/content/docs/guides/providers.md | 5 +++++ .../src/content/docs/reference/configuration/providers.md | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index aaf6a0fed5..d123460692 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -1023,6 +1023,11 @@ single upstream model id and wins over it. A value is either a bare model id of or a public catalog slug such as `opencode-go/deepseek-v4-flash`, and a provider stamp wins over the root selector on its own rows while the root selector stays the fallback elsewhere. +A bare value resolves against the provider's own rows first and then against a bare catalog row, +which is how a native model such as `gpt-5.6-terra` is named; a value that matches neither is left +unresolved. Giving the full slug avoids the question entirely when the reviewer is another provider's +routed model. + Selectors are resolved against the final catalog on the next sync. An unknown target fails closed: that override is skipped, a diagnostic is printed, and normal upstream auto-review behavior stays in place. Removing the root selector leaves provider stamps alone, and removing a provider selector diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 88819e69e3..ad107f1eae 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -270,7 +270,9 @@ has the operator workflow and a worked example. `autoReviewModel` is the provider-wide reviewer target. A value can be a bare model id of that same provider (the catalog row is normalized to the `provider/model` slug) or a full public catalog -slug such as `opencode-go/deepseek-v4-flash`. `autoReviewModelOverrides` keys are exact upstream +slug such as `opencode-go/deepseek-v4-flash`. A bare value resolves against that provider's rows +first and then against a bare catalog row, which is how a native model such as `gpt-5.6-terra` is +named; a value that matches neither is left unresolved. `autoReviewModelOverrides` keys are exact upstream model ids of that provider; an entry wins over the provider-wide value for its model. A provider stamp wins over the root selector on its own routed rows, and the root selector remains the fallback for native rows and routed rows without a provider stamp. Removing a provider selector From a47e77092634fb1f3677036f7bf3c0381a84d7a8 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:53:50 +0800 Subject: [PATCH 08/14] fix(management): reject auto-review fields on the canonical openai POST POST normalized a clear form away before the merged-row guard could see it, so it answered 200 where PATCH now answers 400. Reject the fields on the submitted body, matching PATCH. --- src/server/management/provider-routes.ts | 5 +++++ .../management-provider-validation.test.ts | 17 ++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 97618443d4..a748827865 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1049,6 +1049,11 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { } }); - test("canonical openai PATCH rejects auto-review fields in every clear form", async () => { + test("canonical openai PATCH and POST reject auto-review fields in every clear form", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; @@ -1556,6 +1556,21 @@ describe("provider management validation", () => { expect(response.status).toBe(400); expect(await response.json()).toMatchObject({ error: expect.stringContaining("autoReviewModel") }); } + // POST carries the same prohibition: the clear forms are normalized away before the + // merged-row guard, so they have to be rejected on the submitted body instead. + for (const body of [ + { autoReviewModel: null }, + { autoReviewModelOverrides: {} }, + { autoReviewModelOverrides: { "glm-5.2": null } }, + ]) { + const response = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: { ...canonicalDirect, ...body } }), + }); + expect(response.status).toBe(400); + expect(await response.json()).toMatchObject({ error: expect.stringContaining("autoReviewModel") }); + } expect(readFileSync(join(TEST_DIR, "config.json"))).toEqual(before); } finally { await server.stop(true); From 1f74d06d09f330c809c7815ded09f4702cf98b77 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 01:53:50 +0800 Subject: [PATCH 09/14] test(catalog): pin that the legacy sweep is invisible while the root resolves --- src/codex/catalog/sync.ts | 2 +- tests/codex-integration/codex-catalog.test.ts | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 14299fc0ad..4abc474ec2 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1638,7 +1638,7 @@ function legacyRootStampValues(observedModels: readonly RawEntry[]): ReadonlySet } /** - * Sweep legacy root stamps off the rows a provider plan is about to restamp. + * Sweep legacy root stamps off the rows a root removal owns, before provider plans land. * * Root removal reaches marker-tagged native rows on its own, but a catalog written before the * marker only carries the legacy signature — and provider stamping rewrites that signature before diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index dceea18913..c0efd4b787 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7537,6 +7537,33 @@ describe("provider-level auto_review_model overrides", () => { expect(result).toBe("unresolved"); expect(models.every(row => row.auto_review_model_override === null)).toBe(true); }); + + test("the legacy sweep leaves no trace while a root selector resolves", () => { + const sourceModels: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "legacy-root" }, + { slug: "blsc/glm-5.2", auto_review_model_override: "legacy-root" }, + ]; + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: "legacy-root" }, + { slug: "blsc/glm-5.2", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-flash", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride( + models, + "gpt-5.6-terra", + config({ autoReviewModel: "opencode-go/deepseek-v4-flash" }), + sourceModels, + ); + expect(result).toBe("applied"); + // Every non-provider row ends on the root value and each provider row on the plan target, so + // the sweep is invisible once the root selector resolves. + expect(models.find(row => row.slug === "gpt-5.6-terra")?.auto_review_model_override).toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "opencode-go/deepseek-v4-flash")?.auto_review_model_override) + .toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override) + .toBe("opencode-go/deepseek-v4-flash"); + expect(models.some(row => row.auto_review_model_override === "legacy-root")).toBe(false); + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; From fe8edd6843b63f0d2e5895faa555c7404fdc2afe Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 02:07:33 +0800 Subject: [PATCH 10/14] feat(catalog): report a bare auto-review target that lands outside its provider Resolving a bare value to a bare catalog row is how a native model is named as a reviewer, so it stays usable, but a mistyped target must not be silent: the sync now names the row that actually supplies the reviewer. Same-provider resolutions stay quiet, and the docs state the notice. --- .../src/content/docs/guides/providers.md | 5 +-- .../docs/reference/configuration/providers.md | 3 +- src/codex/catalog/sync.ts | 34 +++++++++++++++++-- tests/codex-integration/codex-catalog.test.ts | 26 ++++++++++++++ 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index d123460692..38865f1bf0 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -1025,8 +1025,9 @@ root selector on its own rows while the root selector stays the fallback elsewhe A bare value resolves against the provider's own rows first and then against a bare catalog row, which is how a native model such as `gpt-5.6-terra` is named; a value that matches neither is left -unresolved. Giving the full slug avoids the question entirely when the reviewer is another provider's -routed model. +unresolved, and a bare value that lands outside the provider prints a note naming the row that +supplies the reviewer. Giving the full slug avoids the question entirely when the reviewer is +another provider's routed model. Selectors are resolved against the final catalog on the next sync. An unknown target fails closed: that override is skipped, a diagnostic is printed, and normal upstream auto-review behavior stays diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index ad107f1eae..574086fca5 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -272,7 +272,8 @@ has the operator workflow and a worked example. provider (the catalog row is normalized to the `provider/model` slug) or a full public catalog slug such as `opencode-go/deepseek-v4-flash`. A bare value resolves against that provider's rows first and then against a bare catalog row, which is how a native model such as `gpt-5.6-terra` is -named; a value that matches neither is left unresolved. `autoReviewModelOverrides` keys are exact upstream +named, and a bare value that lands outside the provider prints a note naming the row that actually +supplies the reviewer; a value that matches neither is left unresolved. `autoReviewModelOverrides` keys are exact upstream model ids of that provider; an entry wins over the provider-wide value for its model. A provider stamp wins over the root selector on its own routed rows, and the root selector remains the fallback for native rows and routed rows without a provider stamp. Removing a provider selector diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 4abc474ec2..917f48a167 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1704,6 +1704,21 @@ function warnProviderAutoReviewModelDiagnostic( ); } +/** + * Note once when a bare selector resolves to a row outside the provider it was configured on. + * + * That is how a native model is named as a reviewer, so it stays usable, but a mistyped target must + * not be silent: the operator sees which catalog row actually supplies the reviewer. + */ +function warnProviderAutoReviewForeignTarget(provider: string, configured: string, target: string): void { + const safeProvider = JSON.stringify(redactSecretString(provider)); + const safeConfigured = JSON.stringify(redactSecretString(configured)); + const safeTarget = JSON.stringify(redactSecretString(target)); + console.warn( + `[opencodex] auto_review_model for provider ${safeProvider} (${safeConfigured}) resolved to ${safeTarget}, which is not a row of that provider; that catalog row supplies the reviewer.`, + ); +} + /** Preserve native upstream overrides and the root-derived provenance marker from source rows. */ function preserveNativeAutoReviewModelOverrides( models: readonly RawEntry[], @@ -1813,7 +1828,7 @@ function resolveProviderReviewTarget( models: readonly RawEntry[], provider: string, configuredRaw: unknown, -): { kind: "valid"; value: ValidProviderReviewTarget } | { kind: "invalid"; configured: string } | { kind: "unresolved"; configured: string } | { kind: "absent" } { +): { kind: "valid"; value: ValidProviderReviewTarget; foreign?: boolean } | { kind: "invalid"; configured: string } | { kind: "unresolved"; configured: string } | { kind: "absent" } { if (typeof configuredRaw !== "string") return { kind: "absent" }; const configured = configuredRaw.trim(); if (!configured) return { kind: "absent" }; @@ -1841,7 +1856,10 @@ function resolveProviderReviewTarget( } if (!match) return { kind: "unresolved", configured }; const target = typeof match.slug === "string" ? match.slug : configured; - return { kind: "valid", value: { configured, target } }; + // A qualified selector may name another provider's row on purpose; only a bare value that lands + // outside this provider is worth reporting. + const foreign = !configured.includes("/") && catalogEntryProviderName(match) !== provider; + return { kind: "valid", value: { configured, target }, ...(foreign ? { foreign: true } : {}) }; } /** Build resolved per-provider plans and emit one diagnostic per bad selector. */ @@ -1859,12 +1877,21 @@ function buildProviderReviewPlans( warnProviderAutoReviewModelDiagnostic(kind, provider, configured); failure ??= kind; }; + const recordForeignTarget = (provider: string, configured: string, target: string): void => { + const signature = `${provider}\u0000foreign\u0000${configured}`; + if (warned.has(signature)) return; + warned.add(signature); + warnProviderAutoReviewForeignTarget(provider, configured, target); + }; for (const [name, provider] of Object.entries(config.providers ?? {})) { if (provider.autoReviewModel === undefined && provider.autoReviewModelOverrides === undefined) continue; const plan: ProviderReviewPlan = { perModel: new Map() }; if (provider.autoReviewModel !== undefined) { const resolved = resolveProviderReviewTarget(models, name, provider.autoReviewModel); - if (resolved.kind === "valid") plan.wide = resolved.value; + if (resolved.kind === "valid") { + plan.wide = resolved.value; + if (resolved.foreign) recordForeignTarget(name, resolved.value.configured, resolved.value.target); + } else if (resolved.kind !== "absent") recordFailure(resolved.kind, name, resolved.configured); } if (provider.autoReviewModelOverrides !== undefined) { @@ -1872,6 +1899,7 @@ function buildProviderReviewPlans( const resolved = resolveProviderReviewTarget(models, name, rawTarget); if (resolved.kind === "valid") { plan.perModel.set(providerModelKey(modelId), resolved.value); + if (resolved.foreign) recordForeignTarget(name, resolved.value.configured, resolved.value.target); } else if (resolved.kind !== "absent") { recordFailure(resolved.kind, name, resolved.configured); } diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index c0efd4b787..6c951b9fde 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7564,6 +7564,32 @@ describe("provider-level auto_review_model overrides", () => { .toBe("opencode-go/deepseek-v4-flash"); expect(models.some(row => row.auto_review_model_override === "legacy-root")).toBe(false); }); + + test("a bare target that resolves outside the provider is used but reported", () => { + const models = entries(); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ autoReviewModel: "gpt-5.6-terra" })); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/glm-5.2")?.auto_review_model_override).toBe("gpt-5.6-terra"); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain("auto_review_model for provider \"blsc\""); + } finally { + warn.mockRestore(); + } + }); + + test("a bare target that resolves inside the provider stays silent", () => { + const models = entries(); + const warn = spyOn(console, "warn").mockImplementation(() => {}); + try { + const result = applyConfiguredAutoReviewModelOverride(models, null, config({ autoReviewModel: "glm-5.2" })); + expect(result).toBe("applied"); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; From b79eca2cc366d1963c0d35f31e73c0a311c87475 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 02:24:31 +0800 Subject: [PATCH 11/14] docs(providers): spell out per-selector fail-closed resolution Both pages now say that autoReviewModel and each autoReviewModelOverrides entry resolve independently and fail closed by themselves, while whatever does resolve is still applied and unstamped rows keep the root selector or upstream behavior. --- docs-site/src/content/docs/guides/providers.md | 10 ++++++---- .../content/docs/reference/configuration/providers.md | 9 ++++++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index 38865f1bf0..739fe284df 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -1029,10 +1029,12 @@ unresolved, and a bare value that lands outside the provider prints a note namin supplies the reviewer. Giving the full slug avoids the question entirely when the reviewer is another provider's routed model. -Selectors are resolved against the final catalog on the next sync. An unknown target fails closed: -that override is skipped, a diagnostic is printed, and normal upstream auto-review behavior stays -in place. Removing the root selector leaves provider stamps alone, and removing a provider selector -clears only that provider's stamps. +Selectors are resolved against the final catalog on the next sync, each one on its own, and each +fails closed by itself: an unresolved `autoReviewModel` prints a diagnostic and stamps no +provider-wide rows, an unresolved `autoReviewModelOverrides` entry prints a diagnostic and stamps +nothing for that model, and whatever does resolve is still applied. Rows without a provider stamp +keep the root selector, or upstream behavior when that is unset. Removing the root selector leaves +provider stamps alone, and removing a provider selector clears only that provider's stamps. These fields are configuration/API only — `PATCH /api/providers?name=` accepts them and the dashboard provider editor does not render inputs for them yet. The canonical `openai` provider diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 574086fca5..aeea2b9f9e 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -280,9 +280,12 @@ fallback for native rows and routed rows without a provider stamp. Removing a pr clears only that provider's stamps; removing the root selector never clears provider stamps. Model ids that contain a slash may be written raw or in their encoded catalog form; both spellings resolve to the same routed row. -Selectors are resolved against the final catalog on each sync: an unknown target fails closed -for the override only, emits a diagnostic, and leaves normal upstream auto-review behavior in -place. The canonical `openai` provider does not accept these fields. +Selectors are resolved against the final catalog on each sync, independently of one another, and +each fails closed on its own: an unresolved `autoReviewModel` emits a diagnostic and stamps no +provider-wide rows, an unresolved `autoReviewModelOverrides` entry emits a diagnostic and stamps +nothing for that model, and any selector that does resolve is still applied. Rows without a provider +stamp keep the root selector, or normal upstream auto-review behavior when that is unset. The +canonical `openai` provider does not accept these fields. Removing the root selector clears root stamps from every row, including native rows stamped by earlier releases that predate OpenCodex's provenance marker. That cleanup recognizes a legacy From 22b05247b78d1d67aa7dc52d33b5f6ec63d13cfb Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 02:37:41 +0800 Subject: [PATCH 12/14] feat(catalog): accept a provider alias as an auto-review override key A routed row's slug always carries the upstream id, and modelAliases publishes a second public name for that id, so a key written in either spelling now selects the same row instead of being ignored. Docs say so. --- .../docs/reference/configuration/providers.md | 6 ++- src/codex/catalog/sync.ts | 12 ++++++ tests/codex-integration/codex-catalog.test.ts | 40 +++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index aeea2b9f9e..36ad367378 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -273,8 +273,10 @@ provider (the catalog row is normalized to the `provider/model` slug) or a full slug such as `opencode-go/deepseek-v4-flash`. A bare value resolves against that provider's rows first and then against a bare catalog row, which is how a native model such as `gpt-5.6-terra` is named, and a bare value that lands outside the provider prints a note naming the row that actually -supplies the reviewer; a value that matches neither is left unresolved. `autoReviewModelOverrides` keys are exact upstream -model ids of that provider; an entry wins over the provider-wide value for its model. A provider +supplies the reviewer; a value that matches neither is left unresolved. `autoReviewModelOverrides` +keys are exact upstream model ids of that provider, or the provider's published alias for one +(`modelAliases`); either spelling names the same routed row, whose slug carries the upstream id. An +entry wins over the provider-wide value for its model. A provider stamp wins over the root selector on its own routed rows, and the root selector remains the fallback for native rows and routed rows without a provider stamp. Removing a provider selector clears only that provider's stamps; removing the root selector never clears provider stamps. diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 917f48a167..7fff057aba 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1905,6 +1905,18 @@ function buildProviderReviewPlans( } } } + // `modelAliases` publishes a second public name for a model id, and a routed row's slug always + // carries the upstream id — so accept an override key written in either spelling. + for (const [modelId, alias] of Object.entries(provider.modelAliases ?? {})) { + if (typeof alias !== "string" || !alias.trim()) continue; + const idKey = providerModelKey(modelId); + const aliasKey = providerModelKey(alias); + if (idKey === aliasKey) continue; + const fromId = plan.perModel.get(idKey); + const fromAlias = plan.perModel.get(aliasKey); + if (fromId !== undefined && fromAlias === undefined) plan.perModel.set(aliasKey, fromId); + else if (fromAlias !== undefined && fromId === undefined) plan.perModel.set(idKey, fromAlias); + } if (plan.wide !== undefined || plan.perModel.size > 0) plans.set(name, plan); } return { plans, failure }; diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index 6c951b9fde..23834dc4db 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7590,6 +7590,46 @@ describe("provider-level auto_review_model overrides", () => { warn.mockRestore(); } }); + + test("an override key written as the provider alias selects the same row", () => { + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: null }, + { slug: "blsc/pin-model", auto_review_model_override: null }, + { slug: "blsc/other-model", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, { + providers: { + blsc: { + adapter: "openai-chat", + baseUrl: "https://blsc.example.test/v1", + modelAliases: { "pin-model": "friendly" }, + autoReviewModelOverrides: { friendly: "gpt-5.6-terra" }, + }, + }, + }); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/pin-model")?.auto_review_model_override).toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "blsc/other-model")?.auto_review_model_override).toBeNull(); + }); + + test("an alias does not displace an override keyed by the upstream id", () => { + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: null }, + { slug: "blsc/pin-model", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, { + providers: { + blsc: { + adapter: "openai-chat", + baseUrl: "https://blsc.example.test/v1", + modelAliases: { "pin-model": "friendly" }, + autoReviewModelOverrides: { "pin-model": "gpt-5.6-terra" }, + }, + }, + }); + expect(result).toBe("applied"); + expect(models.find(row => row.slug === "blsc/pin-model")?.auto_review_model_override).toBe("gpt-5.6-terra"); + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; From a7e2f4e3296d36ef24a7429ad054bac8b0c4df06 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 02:57:24 +0800 Subject: [PATCH 13/14] fix(catalog): keep an alias from propagating onto the row that owns its name The alias API validates against the ids discovery has reported so far, so a cold start can persist an alias that later turns out to be another routed row's id. That key names that row, so the alternate-spelling propagation must skip it. --- src/codex/catalog/sync.ts | 15 ++++++++++++ tests/codex-integration/codex-catalog.test.ts | 23 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 7fff057aba..4712495646 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1823,6 +1823,20 @@ function providerModelKey(modelId: string): string { return canonicalAutoReviewModelKey(modelId); } +/** + * True when another routed row of this provider already carries `alias` as its own model id. + * + * The alias API validates against whatever ids discovery has reported so far, so on a cold start an + * alias can be persisted that later turns out to name a different row. A key using it is then not + * an alternate spelling of the aliased model — it is that row's id — and must not be propagated. + */ +function aliasNamesAnotherRoutedRow(models: readonly RawEntry[], provider: string, alias: string): boolean { + const encoded = encodeRoutedModelId(alias); + return models.some(entry => isRoutedCatalogEntry(entry) + && catalogEntryProviderName(entry) === provider + && catalogEntryModelSegment(entry) === encoded); +} + /** Resolve one configured target against the assembled catalog; bare values name a model of the same provider. */ function resolveProviderReviewTarget( models: readonly RawEntry[], @@ -1909,6 +1923,7 @@ function buildProviderReviewPlans( // carries the upstream id — so accept an override key written in either spelling. for (const [modelId, alias] of Object.entries(provider.modelAliases ?? {})) { if (typeof alias !== "string" || !alias.trim()) continue; + if (aliasNamesAnotherRoutedRow(models, name, alias)) continue; const idKey = providerModelKey(modelId); const aliasKey = providerModelKey(alias); if (idKey === aliasKey) continue; diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index 23834dc4db..56049f2f05 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -7630,6 +7630,29 @@ describe("provider-level auto_review_model overrides", () => { expect(result).toBe("applied"); expect(models.find(row => row.slug === "blsc/pin-model")?.auto_review_model_override).toBe("gpt-5.6-terra"); }); + + test("an alias that names another routed row is not propagated", () => { + const models: Array> = [ + { slug: "gpt-5.6-terra", auto_review_model_override: null }, + { slug: "blsc/pin-model", auto_review_model_override: null }, + { slug: "blsc/friendly", auto_review_model_override: null }, + ]; + const result = applyConfiguredAutoReviewModelOverride(models, null, { + providers: { + blsc: { + adapter: "openai-chat", + baseUrl: "https://blsc.example.test/v1", + // Persisted on a cold start, before discovery reported the row that owns "friendly". + modelAliases: { "pin-model": "friendly" }, + autoReviewModelOverrides: { friendly: "gpt-5.6-terra" }, + }, + }, + }); + expect(result).toBe("applied"); + // The key names the row that literally carries it; the colliding alias is not propagated to it. + expect(models.find(row => row.slug === "blsc/friendly")?.auto_review_model_override).toBe("gpt-5.6-terra"); + expect(models.find(row => row.slug === "blsc/pin-model")?.auto_review_model_override).toBeNull(); + }); }); import { ManagementRequest as Request } from "../helpers/management-auth"; From 0cc58e4eaed34caf2999590f89e82f49d2036a33 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 12 Sep 2026 14:03:51 +0800 Subject: [PATCH 14/14] docs(providers): state the field-by-field fallback when selectors are removed Both pages now say what happens per field: clearing autoReviewModel drops provider-wide stamps from rows without a per-model override, clearing an override entry falls back to the provider-wide selector, clearing both drops every provider stamp, and root removal never clears provider stamps. The guide also points at the reference for the slash and alias key rules. --- docs-site/src/content/docs/guides/providers.md | 13 +++++++++++-- .../docs/reference/configuration/providers.md | 7 +++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/docs-site/src/content/docs/guides/providers.md b/docs-site/src/content/docs/guides/providers.md index 739fe284df..821ca62bd4 100644 --- a/docs-site/src/content/docs/guides/providers.md +++ b/docs-site/src/content/docs/guides/providers.md @@ -1023,6 +1023,11 @@ single upstream model id and wins over it. A value is either a bare model id of or a public catalog slug such as `opencode-go/deepseek-v4-flash`, and a provider stamp wins over the root selector on its own rows while the root selector stays the fallback elsewhere. +An override key may be written as the upstream model id or as the provider's published alias, and an +id that contains a slash may be written raw or in its encoded catalog form — the +[configuration reference](/reference/configuration/providers/#auto-review-approval-model-selection) +lists those field-level rules. + A bare value resolves against the provider's own rows first and then against a bare catalog row, which is how a native model such as `gpt-5.6-terra` is named; a value that matches neither is left unresolved, and a bare value that lands outside the provider prints a note naming the row that @@ -1033,8 +1038,12 @@ Selectors are resolved against the final catalog on the next sync, each one on i fails closed by itself: an unresolved `autoReviewModel` prints a diagnostic and stamps no provider-wide rows, an unresolved `autoReviewModelOverrides` entry prints a diagnostic and stamps nothing for that model, and whatever does resolve is still applied. Rows without a provider stamp -keep the root selector, or upstream behavior when that is unset. Removing the root selector leaves -provider stamps alone, and removing a provider selector clears only that provider's stamps. +keep the root selector, or upstream behavior when that is unset. + +Removing a provider selector falls back field by field: clearing `autoReviewModel` drops +provider-wide stamps from rows that have no per-model override, clearing an `autoReviewModelOverrides` +entry drops that model's stamp so it takes the provider-wide selector when one is configured, and +clearing both drops every provider stamp. Removing the root selector never clears provider stamps. These fields are configuration/API only — `PATCH /api/providers?name=` accepts them and the dashboard provider editor does not render inputs for them yet. The canonical `openai` provider diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 36ad367378..29053f402e 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -278,8 +278,11 @@ keys are exact upstream model ids of that provider, or the provider's published (`modelAliases`); either spelling names the same routed row, whose slug carries the upstream id. An entry wins over the provider-wide value for its model. A provider stamp wins over the root selector on its own routed rows, and the root selector remains the -fallback for native rows and routed rows without a provider stamp. Removing a provider selector -clears only that provider's stamps; removing the root selector never clears provider stamps. +fallback for native rows and routed rows without a provider stamp. +Removing a provider selector falls back field by field: clearing `autoReviewModel` drops +provider-wide stamps from rows that have no per-model override, clearing an `autoReviewModelOverrides` +entry drops that model's stamp so it takes the provider-wide selector when one is configured, and +clearing both drops every provider stamp. Removing the root selector never clears provider stamps. Model ids that contain a slash may be written raw or in their encoded catalog form; both spellings resolve to the same routed row. Selectors are resolved against the final catalog on each sync, independently of one another, and