From 2aa6351501190d650e9430f63445dd4696677fee Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Thu, 3 Sep 2026 15:40:32 +0800 Subject: [PATCH 1/7] feat(catalog): provider-level auto-review model override --- src/codex/catalog/parsing.ts | 2 + src/codex/catalog/provider-fetch.ts | 121 ++++- src/codex/catalog/sync.ts | 138 ++++- src/codex/convergence.ts | 3 + src/config.ts | 25 + src/config/provider-validation.ts | 125 +++++ src/providers/derive.ts | 52 ++ src/providers/registry.ts | 4 + src/router.ts | 11 + src/server/auth-cors.ts | 11 + src/server/management/provider-routes.ts | 72 ++- src/types/provider.ts | 16 +- structure/02_config-and-codex-home.md | 32 ++ tests/auto-review-model-override.test.ts | 484 ++++++++++++++++++ tests/codex-integration/codex-catalog.test.ts | 93 ++++ ...odex-convergence-account-selectors.test.ts | 52 ++ .../codex-gather-authority.test.ts | 6 +- .../provider-config-batch-management.test.ts | 31 ++ .../management-provider-validation.test.ts | 245 +++++++++ 19 files changed, 1493 insertions(+), 30 deletions(-) create mode 100644 tests/auto-review-model-override.test.ts diff --git a/src/codex/catalog/parsing.ts b/src/codex/catalog/parsing.ts index 0677da9a8a..a7763ffbcd 100644 --- a/src/codex/catalog/parsing.ts +++ b/src/codex/catalog/parsing.ts @@ -142,6 +142,8 @@ export interface CatalogModel { * "shell" leaves tool_mode unset so Codex declares top-level shell tools (exec_command). */ codexToolMode?: "code_mode_only" | "shell"; + /** Codex auto-review (approvals) model override for this routed row. */ + autoReviewModelOverride?: string; /** Normalized upstream capability names retained for management/API consumers (#485 follow-up). */ capabilities?: string[]; /** OpenCodex-only catalog ownership marker; Codex ignores the serialized extension field. */ diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index f810bfb422..ace2815546 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -35,7 +35,7 @@ import { modelInList } from "../../types"; import { CODEX_REASONING_LEVELS, codexEffortRank, configuredReasoningEfforts, modelRecordValue, sanitizeCodexReasoningEfforts } from "../../reasoning-effort"; import { isModelVisionSidecarConsumer } from "../../vision/eligibility"; import { getModelMetadata, getModelMetadataCaseInsensitive, listModelMetadata, resolveMetadataProvider, type ModelMetadata } from "../../generated/model-metadata"; -import { enrichProviderFromRegistry, shouldCaseFoldMetadataModelId } from "../../providers/derive"; +import { enrichProviderFromRegistry, resolveAutoReviewModel, shouldCaseFoldMetadataModelId } from "../../providers/derive"; import { captureFastPolicyAuthority, fastPolicyForModel, @@ -47,7 +47,7 @@ import { parseAntigravityAvailableModels, registerAntigravityDiscoveredWireModel import { applyProviderContextCap, providerContextCap, resolveUnknownRoutedContextWindow } from "../../providers/context-cap"; import { clampAutoCompactTokenLimit } from "../../providers/auto-compact-budget"; import { effectiveModelAliases } from "../../providers/default-aliases"; -import { routedSlug, slugEquals, slugEquivalenceKey, slugsEquivalent } from "../../providers/slug-codec"; +import { encodeRoutedModelId, routedSlug, slugEquals, slugEquivalenceKey, slugsEquivalent } from "../../providers/slug-codec"; import { CODEX_GPT5_IDENTITY_LINE } from "../../adapters/identity"; import { filterCursorConfiguredModelsByLiveDiscovery } from "../../adapters/cursor/discovery"; import { fetchCursorUsableModels } from "../../adapters/cursor/live-models"; @@ -176,6 +176,13 @@ interface CapturedProviderGather { * synthesized for combo derivation instead (#1305). */ readonly retainConfiguredModelIds?: ReadonlySet; + /** + * Model ids captured from the enriched provider before any outbound await. + * Auto-review membership must never read the registry after capture: a + * custom-destination flight can otherwise observe a registry state it was + * not admitted under (tests/codex-gather-authority.test.ts). + */ + readonly knownModelIds: ReadonlyArray; } interface GatherFlightCapture { @@ -424,6 +431,7 @@ function captureProviderGather( enrichProviderFromRegistry(name, enriched); const registryTransportMatch = providerMatchesRegistryTransport(name, enriched); const provider = recursivelyFreeze(enriched); + const knownModelIds = Object.freeze([...new Set(provider.models ?? [])]); const fastPolicyAuthority = captureFastPolicyAuthority( name, provider, @@ -464,6 +472,7 @@ function captureProviderGather( return Object.freeze({ name, provider, + knownModelIds, discovery, policy, request, @@ -759,12 +768,68 @@ function configuredVerbositySupport(name: string, prov: OcxProviderConfig | unde return prov.supportsVerbosity; } +const warnedAutoReviewTargets = new Set(); + +/** + * Resolve and normalize the Codex auto-review override for one routed row. + * A target that matches a known native id of the provider is encoded into the + * provider's one-slash catalog slug; a namespaced (cross-provider) target is kept + * verbatim and checked against the assembled catalog at sync time; unknown bare + * targets are skipped (fail closed) with a deduped, redacted warning. + */ +function resolveAutoReviewOverrideForRow( + providerName: string, + provider: OcxProviderConfig | undefined, + modelId: string, + knownModelIds?: ReadonlyArray, +): string | undefined { + const target = resolveAutoReviewModel(provider, modelId); + if (target === null) return undefined; + // Membership is case-folded to match the per-model lookup in + // resolveAutoReviewModel (exact -> :family -> case-fold). The canonical id from + // the provider's own list is what gets slug-encoded, so casing drift in a + // configured target cannot leak into catalog slugs. The CURRENT row id wins + // when provider or captured entries differ only by case: the emitted slug + // must match the catalog row's own id casing or final catalog validation + // drops the override. The captured snapshot (never the registry) supplies + // registry-seeded membership for rows assembled after asynchronous discovery. + const foldedKnown = new Map(); + for (const id of [ + modelId, + ...(provider?.models ?? []), + ...(knownModelIds ?? []), + ]) { + if (!foldedKnown.has(id.toLowerCase())) foldedKnown.set(id.toLowerCase(), id); + } + const canonical = foldedKnown.get(target.toLowerCase()); + if (target.includes("/") && canonical === undefined) { + // Cross-provider catalog slug: kept verbatim; final existence is checked + // against the assembled catalog at sync time. + return target; + } + if (canonical === undefined) { + const key = providerName + "/" + target; + if (!warnedAutoReviewTargets.has(key)) { + warnedAutoReviewTargets.add(key); + console.warn( + "[opencodex] autoReviewModel target " + JSON.stringify(redactSecretString(target)) + + " for " + JSON.stringify(redactSecretString(providerName)) + "/" + + JSON.stringify(redactSecretString(modelId)) + + " is not a known model of that provider; catalog override skipped.", + ); + } + return undefined; + } + return routedSlug(providerName, encodeRoutedModelId(canonical)); +} + export function applyProviderConfigHints( name: string, prov: OcxProviderConfig, model: CatalogModel, providerCap?: number, metadataModelIdCaseFold?: boolean, + knownModelIds?: ReadonlyArray, effectiveAlias?: string | null, ): CatalogModel { const displayName = configuredModelDisplayName(prov, model.id); @@ -780,6 +845,7 @@ export function applyProviderConfigHints( const configuredMaxInput = configuredMaxInputTokens(prov, model.id); const maxOutputTokens = routedMaxOutputTokens(name, prov, model, model.id, metadataModelIdCaseFold); const configuredAutoCompact = configuredAutoCompactTokenLimit(prov, model.id); + const autoReviewOverride = resolveAutoReviewOverrideForRow(name, prov, model.id, knownModelIds); let inputModalities = configuredInputModalities(prov, model.id); // The shared vision-sidecar consumer predicate keeps catalog advertisement and request-time // planning aligned. The catalog must still advertise image input — the Codex app @@ -816,6 +882,9 @@ export function applyProviderConfigHints( ...(displayName !== undefined ? { displayName } : {}), ...(providerAlias !== undefined ? { providerAlias } : {}), ...(hintedWindow !== undefined ? { contextWindow: hintedWindow } : {}), + // Always set the key so a re-hint clears a stale override after the operator + // removes the config; JSON serialization drops the undefined value. + autoReviewModelOverride: autoReviewOverride, ...(inputModalities ? { inputModalities } : {}), ...(reasoningEfforts !== undefined ? { reasoningEfforts } : {}), ...(configuredMaxInput !== undefined @@ -874,9 +943,10 @@ export function catalogHintsFromProviderConfig( id: string, contextCap?: number, metadataModelIdCaseFold?: boolean, + knownModelIds?: ReadonlyArray, effectiveAlias?: string | null, ): Partial { - const hinted = applyProviderConfigHints(name, prov, { id, provider: name }, contextCap, metadataModelIdCaseFold, effectiveAlias); + const hinted = applyProviderConfigHints(name, prov, { id, provider: name }, contextCap, metadataModelIdCaseFold, knownModelIds, effectiveAlias); const { provider: _provider, id: _id, ...hints } = hinted; return hints; } @@ -887,9 +957,10 @@ export function applyConfigHintsToCachedModels( models: CatalogModel[], contextCap?: number, metadataModelIdCaseFold?: boolean, + knownModelIds?: ReadonlyArray, effectiveAlias?: string | null, ): CatalogModel[] { - return models.map(model => applyProviderConfigHints(name, prov, model, contextCap, metadataModelIdCaseFold, effectiveAlias)); + return models.map(model => applyProviderConfigHints(name, prov, model, contextCap, metadataModelIdCaseFold, knownModelIds, effectiveAlias)); } @@ -1517,7 +1588,7 @@ async function fetchProviderModelsWithAuth( const configured: CatalogModel[] = configuredIds.map(id => ({ id, provider: name, - ...catalogHintsFromProviderConfig(name, prov, id, contextCap, metadataModelIdCaseFold, captured.effectiveAlias), + ...catalogHintsFromProviderConfig(name, prov, id, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias), })); const withConfiguredRetention = ( models: CatalogModel[], @@ -1530,6 +1601,7 @@ async function fetchProviderModelsWithAuth( configured, retainConfiguredModelIds: captured.retainConfiguredModelIds, contextCap, + knownModelIds: captured.knownModelIds, seedVertexDefault, retainComboTargets: options?.retainComboTargets, metadataModelIdCaseFold, @@ -1571,7 +1643,7 @@ async function fetchProviderModelsWithAuth( : [{ id: prov.defaultModel, provider: name, - ...catalogHintsFromProviderConfig(name, prov, prov.defaultModel, contextCap, metadataModelIdCaseFold, captured.effectiveAlias), + ...catalogHintsFromProviderConfig(name, prov, prov.defaultModel, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias), }]; const vertexDefaultSeed = seedVertexDefault ? configured[0] : undefined; const withVertexDefaultSeed = (models: CatalogModel[]): CatalogModel[] => ( @@ -1588,7 +1660,7 @@ async function fetchProviderModelsWithAuth( const cachedCursor = getFreshCached(name, ttlMs); if (cachedCursor) { return observed( - withConfiguredRetention(applyConfigHintsToCachedModels(name, prov, cachedCursor, undefined, metadataModelIdCaseFold, captured.effectiveAlias)), + withConfiguredRetention(applyConfigHintsToCachedModels(name, prov, cachedCursor, undefined, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias)), "authoritative", ); } @@ -1596,7 +1668,7 @@ async function fetchProviderModelsWithAuth( const cooling = getStaleCached(name); return observed( withConfiguredRetention( - cooling ? applyConfigHintsToCachedModels(name, prov, cooling, undefined, metadataModelIdCaseFold, captured.effectiveAlias) : configured, + cooling ? applyConfigHintsToCachedModels(name, prov, cooling, undefined, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias) : configured, ), "degraded", ); @@ -1637,7 +1709,7 @@ async function fetchProviderModelsWithAuth( const staleCursor = getStaleCached(name); return observed( withConfiguredRetention( - staleCursor ? applyConfigHintsToCachedModels(name, prov, staleCursor, undefined, metadataModelIdCaseFold, captured.effectiveAlias) : configured, + staleCursor ? applyConfigHintsToCachedModels(name, prov, staleCursor, undefined, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias) : configured, ), "degraded", ); @@ -1655,7 +1727,7 @@ async function fetchProviderModelsWithAuth( if (fresh) { return observed( withConfiguredRetention( - withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, fresh, contextCap, metadataModelIdCaseFold, captured.effectiveAlias)), + withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, fresh, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias)), ), "authoritative", ); // dedups Codex's frequent /v1/models polling within the TTL @@ -1667,7 +1739,7 @@ async function fetchProviderModelsWithAuth( return observed( withConfiguredRetention( stale - ? withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, stale, contextCap, metadataModelIdCaseFold, captured.effectiveAlias)) + ? withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, stale, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias)) : failedDiscoveryConfigured, ), "degraded", @@ -1707,7 +1779,7 @@ async function fetchProviderModelsWithAuth( return { models: withConfiguredRetention( stale - ? withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, stale, contextCap, metadataModelIdCaseFold, captured.effectiveAlias)) + ? withVertexDefaultSeed(applyConfigHintsToCachedModels(name, prov, stale, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias)) : failedDiscoveryConfigured, ), fallback: stale ? "stale" : "configured", @@ -1792,7 +1864,7 @@ async function fetchProviderModelsWithAuth( reasoningEfforts: [], ...(model.contextWindow ? { contextWindow: model.contextWindow } : {}), ...(model.inputModalities ? { inputModalities: model.inputModalities } : {}), - }, contextCap, metadataModelIdCaseFold, captured.effectiveAlias)); + }, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias)); const forCache = withConfiguredRetention(live, { retainComboTargets: false }); if (!setCached(name, forCache, Date.now(), cacheGeneration)) { return observed(withConfiguredRetention(configured), "degraded"); @@ -1855,7 +1927,7 @@ async function fetchProviderModelsWithAuth( provider: name, ...(ownedBy ? { owned_by: ownedBy } : {}), ...discoveredHints, - }, contextCap, metadataModelIdCaseFold, captured.effectiveAlias); + }, contextCap, metadataModelIdCaseFold, captured.knownModelIds, captured.effectiveAlias); }) .filter(m => shouldExposeProviderModel(name, m.id)); // Capture the count BEFORE the alias/configured augmentation below pushes extra rows into @@ -1950,6 +2022,7 @@ export function mergeConfiguredModelsIntoLiveCatalog(opts: { configured: readonly CatalogModel[]; retainConfiguredModelIds?: ReadonlySet; contextCap?: number; + knownModelIds?: ReadonlyArray; seedVertexDefault?: boolean; retainComboTargets?: boolean; metadataModelIdCaseFold?: boolean; @@ -1960,6 +2033,7 @@ export function mergeConfiguredModelsIntoLiveCatalog(opts: { configured, retainConfiguredModelIds, contextCap, + knownModelIds, seedVertexDefault, retainComboTargets = true, metadataModelIdCaseFold, @@ -1971,7 +2045,7 @@ export function mergeConfiguredModelsIntoLiveCatalog(opts: { if (present.has(candidate.id)) continue; const dated = out.find(live => isDatedVariantId(live.id, candidate.id)); if (dated) { - out.push(applyProviderConfigHints(name, prov, { ...dated, id: candidate.id }, contextCap, metadataModelIdCaseFold)); + out.push(applyProviderConfigHints(name, prov, { ...dated, id: candidate.id }, contextCap, metadataModelIdCaseFold, knownModelIds)); present.add(candidate.id); continue; } @@ -2144,6 +2218,8 @@ async function gatherRoutedModelsUncached( // vision-sidecar model advertised text-only, blocking image attachments app-side). // Enrich a CLONE: hydrated defaults must never leak into the persisted config. const activeProviders = capture.providers; + // Configured custom rows join the per-provider membership used for bare-target auto-review resolution. + const knownModelIdsByName = new Map(activeProviders.map(provider => [provider.name, [...(provider.knownModelIds ?? []), ...(config.customModels ?? []).filter(cm => cm.provider === provider.name).map(cm => cm.modelId)]])); const providerResults = await Promise.all( activeProviders.map(provider => fetchProviderModelsWithAuth( provider, @@ -2179,6 +2255,7 @@ async function gatherRoutedModelsUncached( config.providers, config, metadataModelIdCaseFoldByProvider, + knownModelIdsByName, ) // Drop image/video generation models (e.g. Grok image/video) by default. Cursor's static catalog // intentionally mirrors Cursor's public model table, including Gemini image preview, so the @@ -2373,6 +2450,12 @@ async function gatherRoutedModelsUncached( const supportsServiceTier = fastPolicy ? serviceTierSupportFromPolicy(fastPolicy) : undefined; + const autoReviewOverride = resolveAutoReviewOverrideForRow( + cm.provider, + effectiveProvider, + cm.modelId, + knownModelIdsByName.get(cm.provider), + ); const base: CatalogModel = { id: cm.modelId, provider: cm.provider, @@ -2459,7 +2542,11 @@ async function gatherRoutedModelsUncached( ...(base.supportsReasoningSummaries === undefined && replaced.supportsReasoningSummaries !== undefined ? { supportsReasoningSummaries: replaced.supportsReasoningSummaries } : {}), ...(base.codexToolMode === undefined && replaced.codexToolMode !== undefined ? { codexToolMode: replaced.codexToolMode } : {}), ...(base.capabilities === undefined && replaced.capabilities !== undefined ? { capabilities: replaced.capabilities } : {}), + ...(base.autoReviewModelOverride === undefined && replaced.autoReviewModelOverride !== undefined + ? { autoReviewModelOverride: replaced.autoReviewModelOverride } + : {}), } : base; + if (autoReviewOverride !== undefined) merged.autoReviewModelOverride = autoReviewOverride; // Vision-sidecar coverage only: when the enriched provider's shared predicate matches // noVisionModels or text-without-image modelInputModalities, advertise image input so the // Codex app lets images reach the sidecar (#349/#344). Deliberately NOT the full @@ -2588,6 +2675,7 @@ function augmentRoutedModelsWithCapturedOpenAiApiRows( : existingById.get(id) ?? { provider: OPENAI_API_PROVIDER_ID, id }, policy.virtualModels?.[id]?.wireModelId ?? id, ); + const autoReviewOverride = resolveAutoReviewOverrideForRow(OPENAI_API_PROVIDER_ID, configured, id, policy.models); return { provider: OPENAI_API_PROVIDER_ID, id, @@ -2596,6 +2684,7 @@ function augmentRoutedModelsWithCapturedOpenAiApiRows( ...(maxInputTokens ? { maxInputTokens } : {}), ...(maxOutputTokens !== undefined ? { maxOutputTokens } : {}), ...(autoCompactTokenLimit !== undefined ? { autoCompactTokenLimit } : {}), + ...(autoReviewOverride !== undefined ? { autoReviewModelOverride: autoReviewOverride } : {}), ...(policy.modelInputModalities?.[id] ? { inputModalities: [...policy.modelInputModalities[id]!] } : {}), ...(policy.modelReasoningEfforts?.[id] ? { reasoningEfforts: [...policy.modelReasoningEfforts[id]!] } : {}), }; @@ -2625,6 +2714,7 @@ export function augmentRoutedModelsWithMetadata( providers?: Record, caps?: Pick, metadataModelIdCaseFoldByProvider?: ReadonlyMap, + knownModelIdsByProvider?: ReadonlyMap>, ): CatalogModel[] { const out = [...models]; const seen = new Set(out.map(m => `${m.provider}/${m.id}`)); @@ -2655,6 +2745,7 @@ export function augmentRoutedModelsWithMetadata( model, contextCap, metadataModelIdCaseFoldByProvider?.get(provider), + knownModelIdsByProvider?.get(provider), ) : {}), }); diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 3f5f472baf..20879765f3 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -371,6 +371,12 @@ export function deriveEntry( if (model) applyCatalogMetadata(e, model.provider, model.id, model.contextCap); applyCatalogModelMetadata(e, model); if (model?.catalogKind) e.opencodex_catalog_kind = model.catalogKind; + // Codex auto-review (approvals) override: the provider-fetch layer already + // normalized the target to a catalog slug; stamp it verbatim. Rows without + // an override keep the template's null. + if (model?.autoReviewModelOverride) { + e.auto_review_model_override = model.autoReviewModelOverride; + } } else { applyNativeOpenAiContextOverride(e, contextCap); if (isGpt56NativeSlug(slug)) ensureGpt56ReasoningLevels(e); @@ -418,6 +424,9 @@ export function deriveEntry( if (model && isRouted) applyCatalogMetadata(entry, model.provider, model.id, model.contextCap); applyCatalogModelMetadata(entry, model); if (model?.catalogKind) entry.opencodex_catalog_kind = model.catalogKind; + if (model?.autoReviewModelOverride) { + entry.auto_review_model_override = model.autoReviewModelOverride; + } if (!isRouted) applyNativeOpenAiContextOverride(entry, contextCap); return ensureStrictCatalogFields(normalizeServiceTiers(entry), { preserveExactInputModalities: preserveExact, @@ -690,6 +699,7 @@ export function buildCatalogEntriesFromObservedState({ export function resetCatalogRuntimeStateForTests(): void { resetBundledCatalogCacheForTests(); + lastAppliedRootAutoReviewModel = null; lastDropWarnSignature.clear(); openAiApiCollisionWarnings.clear(); comboCatalogWarningSignatures.clear(); @@ -1453,6 +1463,23 @@ export function isValidAutoReviewModel(value: unknown): value is string { export type AutoReviewModelOverrideResult = "absent" | "applied" | "invalid" | "unresolved"; +// Root-selector provenance across regenerate/apply cycles. When a root +// auto_review_model was previously stamped by this module and is later removed, +// native rows that still carry that exact selector are stale copies and must be +// cleared even when routed provider stamps now differ (so the old single-value +// heuristic cannot infer them). +let lastAppliedRootAutoReviewModel: string | null = null; +export interface AutoReviewModelOverrideOptions { + /** + * Preserve provider-derived per-row overrides on routed entries. Routed rows + * regenerated from provider config carry their stamp before this pass runs; + * with this flag the root selector (or its absence) cannot wipe those values. + * Without the flag the historical behavior is kept: absent/invalid root state + * clears every routed row so stale root-selector residue cannot persist. + */ + retainRoutedOverrides?: boolean; +} + function isRoutedCatalogEntry(entry: RawEntry): boolean { const slug = typeof entry.slug === "string" ? entry.slug : ""; return slug.includes("/") @@ -1462,6 +1489,8 @@ function isRoutedCatalogEntry(entry: RawEntry): boolean { function clearAutoReviewModelOverride( models: readonly RawEntry[], sourceModels: readonly RawEntry[] = [], + retainRoutedOverrides = false, + previousRootSelector: string | null = null, ): void { const observedModels = [...models, ...sourceModels]; const configuredValues = new Set(observedModels.flatMap(entry => { @@ -1485,9 +1514,20 @@ function clearAutoReviewModelOverride( for (const entry of models) { if (!entry || typeof entry !== "object") continue; const current = entry.auto_review_model_override; - if (isRoutedCatalogEntry(entry) - || (globalStamp && typeof current === "string" && configuredValues.has(current))) { + if (isRoutedCatalogEntry(entry)) { + // A freshly derived provider stamp (nonblank valid string) is kept when + // retaining is requested; everything else on routed rows is cleared so + // stale root-selector residue cannot persist across regenerations. + if (retainRoutedOverrides && typeof current === "string" && isValidAutoReviewModel(current)) continue; entry.auto_review_model_override = null; + continue; + } + if (typeof current === "string") { + const matchesPreviousRoot = previousRootSelector !== null && previousRootSelector !== "" + && current === previousRootSelector; + const matchesHeuristic = previousRootSelector === null + && globalStamp && configuredValues.has(current); + if (matchesPreviousRoot || matchesHeuristic) entry.auto_review_model_override = null; } } } @@ -1527,42 +1567,64 @@ export function applyAutoReviewModelOverride( models: RawEntry[] | undefined, autoReviewModel: string | null | undefined, sourceModels: readonly RawEntry[] = [], + options: AutoReviewModelOverrideOptions = {}, ): AutoReviewModelOverrideResult { if (!models || !Array.isArray(models)) return "absent"; + const retainRouted = options.retainRoutedOverrides === true; if (autoReviewModel === null || autoReviewModel === undefined) { - clearAutoReviewModelOverride(models, sourceModels); + clearAutoReviewModelOverride(models, sourceModels, retainRouted, lastAppliedRootAutoReviewModel); + lastAppliedRootAutoReviewModel = null; return "absent"; } const trimmed = autoReviewModel.trim(); if (!trimmed) { - clearAutoReviewModelOverride(models, sourceModels); + clearAutoReviewModelOverride(models, sourceModels, retainRouted, lastAppliedRootAutoReviewModel); + lastAppliedRootAutoReviewModel = null; return "absent"; } if (!isValidAutoReviewModel(trimmed)) { - clearAutoReviewModelOverride(models, sourceModels); + clearAutoReviewModelOverride(models, sourceModels, retainRouted, lastAppliedRootAutoReviewModel); + lastAppliedRootAutoReviewModel = null; warnAutoReviewModelDiagnostic("invalid", trimmed); return "invalid"; } if (!configuredCatalogEntry(models, trimmed)) { - clearAutoReviewModelOverride(models, sourceModels); + clearAutoReviewModelOverride(models, sourceModels, retainRouted, lastAppliedRootAutoReviewModel); + lastAppliedRootAutoReviewModel = null; warnAutoReviewModelDiagnostic("unresolved", trimmed); return "unresolved"; } for (const entry of models) { - if (entry && typeof entry === "object") { - entry.auto_review_model_override = trimmed; + if (!entry || typeof entry !== "object") continue; + // Provider-level per-row stamps win for their routed rows; the root selector + // is the fallback for native rows and routed rows without a provider stamp. + if (retainRouted && isRoutedCatalogEntry(entry)) { + const current = entry.auto_review_model_override; + if (typeof current === "string" && isValidAutoReviewModel(current)) continue; } + entry.auto_review_model_override = trimmed; } + lastAppliedRootAutoReviewModel = trimmed; return "applied"; } -/** Apply the root Codex auto-review selector after the final catalog merge. */ +/** + * Apply the root Codex auto-review selector after the final catalog merge. + * Provider-level per-row stamps on routed entries survive by default: they are + * the operator's provider-scoped choice and outrank the root fallback selector. + * Native rows keep upstream-retained values while no root selector exists, and + * receive the root selector (like unstamped routed rows) when one is set. + */ export function finalizeAutoReviewModelOverride( models: RawEntry[] | undefined, sourceModels: readonly RawEntry[] = [], + options: AutoReviewModelOverrideOptions = {}, ): AutoReviewModelOverrideResult { if (models && sourceModels.length > 0) preserveNativeAutoReviewModelOverrides(models, sourceModels); - return applyAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), sourceModels); + return applyAutoReviewModelOverride(models, readConfiguredAutoReviewModel(), sourceModels, { + retainRoutedOverrides: true, + ...options, + }); } function writeRetainedCatalogSync({ @@ -1792,6 +1854,7 @@ function writeRetainedCatalogSync({ }); clampCatalogModelsToCodexSupport(catalog.models); finalizeAutoReviewModelOverride(catalog.models, catalogModelsForMerge); + validateAutoReviewOverridesAgainstCatalog(catalog.models as RawEntry[]); const added = goEntries.length + accountBoundEntries.length; const content = `${JSON.stringify(catalog, null, 2)}\n`; @@ -2082,3 +2145,58 @@ export function invalidateCodexModelsCache(options?: CodexCatalogSyncOptions): b ); return outcome.kind === "completed" && outcome.value; } + +/** + * Final fail-closed pass over the assembled catalog: every stamped + * auto_review_model_override must name a model that is actually emitted. + * Typo'd, disabled, or allowlisted-away targets are replaced with null (one + * redacted warning per target) so every routed row keeps the same field shape + * as template-cloned entries instead of reaching Codex. + */ +export function validateAutoReviewOverridesAgainstCatalog(entries: readonly RawEntry[]): void { + // Only nonblank string slugs are emitted catalog selectors; a missing or + // malformed slug must never become a matchable "undefined"/"null" string. + const slugs = new Set(entries.flatMap(entry => + typeof entry.slug === "string" && entry.slug.trim() !== "" ? [entry.slug] : [], + )); + const warned = new Set(); + for (const entry of entries) { + const override = entry.auto_review_model_override; + // Wrong-shaped values must never persist: JSON.stringify would otherwise + // keep a numeric or object override unchanged. null/undefined already + // serialize as the canonical empty shape, so normalize everything else + // before the native-row preservation branch below (native rows must not + // bypass this fail-closed normalization). + if (override !== undefined && override !== null && typeof override !== "string") { + entry.auto_review_model_override = null; + } + const current = entry.auto_review_model_override; + // Native rows with a valid slug carry upstream-retained values (for example + // a Codex-side "native-upstream" selector) that opencodex must preserve + // verbatim. Preserve only a valid nonblank native string; blank strings and + // residual wrong shapes are normalized to the canonical empty shape instead + // of reaching Codex, without requiring the value to resolve to an emitted + // routed slug. Routed rows and malformed catalog rows with missing/invalid + // slugs still fall through to the fail-closed pass below. + if (!isRoutedCatalogEntry(entry) && typeof entry.slug === "string" && entry.slug.trim() !== "") { + if (typeof current === "string" && current.trim() !== "") continue; + // undefined and null are both canonical empty shapes (undefined is + // omitted by JSON.stringify, null is explicit); only residual blank + // strings and wrong-shaped values are normalized so native rows cannot + // bypass the fail-closed pass. + if (current !== undefined && current !== null) entry.auto_review_model_override = null; + continue; + } + if (typeof current !== "string") continue; + if (!slugs.has(current)) { + if (!warned.has(current)) { + warned.add(current); + console.warn( + "[opencodex] autoReviewModel override " + JSON.stringify(redactSecretString(current)) + + " does not match any catalog model; skipped.", + ); + } + entry.auto_review_model_override = null; + } + } +} diff --git a/src/codex/convergence.ts b/src/codex/convergence.ts index df765a7853..a6449712c9 100644 --- a/src/codex/convergence.ts +++ b/src/codex/convergence.ts @@ -46,6 +46,7 @@ import { finalizeAutoReviewModelOverride, mergeCatalogEntriesFromObservedState, mergeCatalogModelsWithNativeRecovery, + validateAutoReviewOverridesAgainstCatalog, orderForSubagents, } from "./catalog/sync"; import { multiAgentV2EnabledFromConfigText } from "./features"; @@ -378,6 +379,8 @@ function prepareCatalog( ); finalizeAutoReviewModelOverride(mergedModels, catalogModels); catalog.models = mergedModels; + // Fail-closed final pass: an override must name a model actually emitted. + validateAutoReviewOverridesAgainstCatalog(catalog.models as RawEntry[]); return catalog; } diff --git a/src/config.ts b/src/config.ts index fdcda9547c..3b1e8b19ad 100644 --- a/src/config.ts +++ b/src/config.ts @@ -8,6 +8,7 @@ import { DEFAULT_SUBAGENT_MODELS, SUBAGENT_MODELS_VERSION } from "./config/subag export { DEFAULT_SUBAGENT_MODELS } from "./config/subagent-models"; import { apiKeyTransportConfigError, + autoReviewModelConfigError, booleanRecordConfigError, modelAdapterRecordConfigError, modelDisplayNamesConfigError, @@ -18,6 +19,7 @@ import { providerBaseUrlConfigError, providerHeadersConfigError, reasoningSummaryDeliveryRecordConfigError, + sanitizeAutoReviewOverridesForLoad, upstreamHttpVersionConfigError, } from "./config/provider-validation"; import { @@ -540,6 +542,11 @@ const providerConfigSchema = z.object({ statelessResponses: z.boolean().optional(), requiresAdjacentResponsesToolResults: z.boolean().optional(), annotateEmptyToolOutputs: z.boolean().optional(), + autoReviewModel: z.string().trim().min(1).refine(value => !/\s/.test(value), "must not contain whitespace").optional(), + autoReviewModelOverrides: z.record( + z.string().min(1).refine(key => !/\s/.test(key), "keys must not contain whitespace"), + z.string().trim().min(1).refine(value => !/\s/.test(value), "must not contain whitespace"), + ).optional(), fastWire: fastWireSchema.nullable().optional(), supportsServiceTier: z.boolean().optional(), modelSupportsServiceTier: z.record(z.string().min(1), z.boolean()).optional(), @@ -587,17 +594,22 @@ const providerConfigSchema = z.object({ export { isValidProviderName, hasOwnProvider } from "./config/provider-name"; export { + autoReviewModelConfigError, apiKeyTransportConfigError, booleanRecordConfigError, modelAdapterRecordConfigError, modelDisplayNamesConfigError, nonBlankStringArrayConfigError, + normalizeAutoReviewModelField, + normalizeAutoReviewModelFields, + normalizeAutoReviewModelOverridesField, normalizeNonBlankStringArray, positiveIntegerConfigError, positiveIntegerRecordConfigError, providerBaseUrlConfigError, providerHeadersConfigError, reasoningSummaryDeliveryRecordConfigError, + sanitizeAutoReviewOverridesForLoad, upstreamHttpVersionConfigError, } from "./config/provider-validation"; @@ -1377,6 +1389,18 @@ const configSchema = z.object({ message: modelAdaptersError, }); } + const autoReviewError = autoReviewModelConfigError( + name, + (provider as { autoReviewModel?: unknown }).autoReviewModel, + (provider as { autoReviewModelOverrides?: unknown }).autoReviewModelOverrides, + ); + if (autoReviewError) { + ctx.addIssue({ + code: "custom", + path: ["providers", redactSecretString(name), "autoReviewModel"], + message: autoReviewError, + }); + } const preferHostedToolsError = modelPreferHostedToolsConfigError( (provider as { modelPreferHostedTools?: unknown }).modelPreferHostedTools, "modelPreferHostedTools", @@ -2178,6 +2202,7 @@ export function loadConfig(): OcxConfig { sanitizeModelDisplayNamesForLoad(parsed); sanitizeRetryOn429ForLoad(parsed); sanitizeModelCostsForLoad(parsed); + sanitizeAutoReviewOverridesForLoad(parsed); const result = configSchema.safeParse(parsed); if (result.success) { const config = normalizeApiKeyIds(result.data as OcxConfig); diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index 326914a758..900ed61264 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -118,6 +118,131 @@ export function normalizeNonBlankStringArray(value: readonly string[]): string[] return [...new Set(value.map(entry => entry.trim()))]; } +/** + * Validate the Codex auto-review model override shape at the management write + * boundary. Returns an error string, or null when the fields may be persisted. + */ +export function autoReviewModelConfigError(name: string, model: unknown, overrides: unknown): string | null { + if (name === "openai" && (model !== undefined || overrides !== undefined)) { + return "provider openai must not include autoReviewModel or autoReviewModelOverrides"; + } + if (model !== undefined) { + const trimmed = typeof model === "string" ? model.trim() : ""; + if (trimmed === "" || /\s/.test(trimmed)) { + return "autoReviewModel must be a nonblank model id without whitespace"; + } + } + if (overrides === undefined) return null; + if (!overrides || typeof overrides !== "object" || Array.isArray(overrides)) { + return "autoReviewModelOverrides must be an object mapping model ids to approval model ids"; + } + for (const [key, value] of Object.entries(overrides as Record)) { + const trimmedKey = key.trim(); + if (trimmedKey === "" || /\s/.test(trimmedKey)) { + return "autoReviewModelOverrides keys must be nonblank model ids without whitespace"; + } + const trimmedValue = typeof value === "string" ? value.trim() : ""; + if (trimmedValue === "" || /\s/.test(trimmedValue)) { + return "autoReviewModelOverrides values must be nonblank model ids without whitespace"; + } + } + return null; +} + +/** Normalize one autoReviewModel field with PATCH-style null-to-clear semantics. */ +export function normalizeAutoReviewModelField(value: unknown): + | { value: string } + | { clear: true } + | { error: string } { + if (value === null) return { clear: true }; + if (typeof value !== "string" || value.trim() === "" || /\s/.test(value.trim())) { + return { error: "autoReviewModel must be a nonblank model id without whitespace, or null to clear" }; + } + return { value: value.trim() }; +} + +/** Normalize one autoReviewModelOverrides field with PATCH-style null-to-clear semantics. */ +export function normalizeAutoReviewModelOverridesField(value: unknown): + | { value: Record } + | { clear: true } + | { error: string } { + if (value === null) return { clear: true }; + if (!value || typeof value !== "object" || Array.isArray(value)) { + return { error: "autoReviewModelOverrides must be an object mapping model ids to approval model ids, or null to clear" }; + } + const cleaned: Record = {}; + for (const [key, entry] of Object.entries(value as Record)) { + const trimmedKey = key.trim(); + const trimmedEntry = typeof entry === "string" ? entry.trim() : ""; + if (trimmedKey === "" || /\s/.test(trimmedKey) || trimmedEntry === "" || /\s/.test(trimmedEntry)) { + return { error: "autoReviewModelOverrides entries must be nonblank model ids without whitespace" }; + } + if (Object.hasOwn(cleaned, trimmedKey)) { + return { error: "autoReviewModelOverrides keys must be unique after trimming" }; + } + cleaned[trimmedKey] = trimmedEntry; + } + return { value: cleaned }; +} + +/** + * Trim auto-review fields in place on a provider object that already passed + * boundary validation (POST path). Returns an error string only when the + * caller skipped validation; provider-routes always validates first. + */ +export function normalizeAutoReviewModelFields(name: string, provider: { + autoReviewModel?: unknown; + autoReviewModelOverrides?: unknown; +}): string | null { + const error = autoReviewModelConfigError(name, provider.autoReviewModel, provider.autoReviewModelOverrides); + if (error) return error; + if (typeof provider.autoReviewModel === "string") { + provider.autoReviewModel = provider.autoReviewModel.trim(); + } + if (provider.autoReviewModelOverrides !== undefined) { + const normalized = normalizeAutoReviewModelOverridesField(provider.autoReviewModelOverrides); + if ("error" in normalized) return normalized.error; + if ("value" in normalized) provider.autoReviewModelOverrides = normalized.value; + } + return null; +} + +/** + * Load-time sanitizer for hand-edited configs: malformed auto-review fields are + * trimmed and dropped instead of retiring the whole config. The strict + * management write boundary still rejects bad input before it reaches disk. + */ +export function sanitizeAutoReviewOverridesForLoad(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 provider of Object.values(providers as Record)) { + if (!provider || typeof provider !== "object" || Array.isArray(provider)) continue; + const row = provider as Record; + if (row.autoReviewModel !== undefined) { + const value = typeof row.autoReviewModel === "string" ? row.autoReviewModel.trim() : ""; + row.autoReviewModel = value !== "" && !/\s/.test(value) ? value : undefined; + } + if (row.autoReviewModelOverrides !== undefined) { + const overrides = row.autoReviewModelOverrides; + if (!overrides || typeof overrides !== "object" || Array.isArray(overrides)) { + row.autoReviewModelOverrides = undefined; + continue; + } + const cleaned: Record = {}; + for (const [key, value] of Object.entries(overrides as Record)) { + const trimmedKey = key.trim(); + if (trimmedKey === "" || /\s/.test(trimmedKey)) continue; + const trimmedValue = typeof value === "string" ? value.trim() : ""; + if (trimmedValue === "" || /\s/.test(trimmedValue)) continue; + cleaned[trimmedKey] = trimmedValue; + } + row.autoReviewModelOverrides = Object.keys(cleaned).length > 0 ? cleaned : undefined; + } + } +} + export function booleanRecordConfigError(value: unknown, field: string): string | null { if (value === undefined) return null; if (!value || typeof value !== "object" || Array.isArray(value)) return `${field} must be a plain object`; diff --git a/src/providers/derive.ts b/src/providers/derive.ts index 02852fce39..ac4578b92e 100644 --- a/src/providers/derive.ts +++ b/src/providers/derive.ts @@ -260,6 +260,10 @@ export function providerConfigSeed(entry: ProviderRegistryEntry): OcxProviderCon ...(entry.annotateEmptyToolOutputs !== undefined ? { annotateEmptyToolOutputs: entry.annotateEmptyToolOutputs } : {}), + ...(entry.autoReviewModel !== undefined ? { autoReviewModel: entry.autoReviewModel } : {}), + ...(entry.autoReviewModelOverrides !== undefined + ? { autoReviewModelOverrides: { ...entry.autoReviewModelOverrides } } + : {}), ...(entry.autoToolChoiceOnlyModels ? { autoToolChoiceOnlyModels: [...entry.autoToolChoiceOnlyModels] } : {}), ...(entry.preserveReasoningContentModels ? { preserveReasoningContentModels: [...entry.preserveReasoningContentModels] } : {}), ...(entry.requiresReasoningPlaceholderModels ? { requiresReasoningPlaceholderModels: [...entry.requiresReasoningPlaceholderModels] } : {}), @@ -524,6 +528,12 @@ export function enrichProviderFromRegistry(name: string, prov: OcxProviderConfig if (prov.annotateEmptyToolOutputs === undefined && seed.annotateEmptyToolOutputs !== undefined) { prov.annotateEmptyToolOutputs = seed.annotateEmptyToolOutputs; } + if (prov.autoReviewModel === undefined && seed.autoReviewModel !== undefined) { + prov.autoReviewModel = seed.autoReviewModel; + } + if (prov.autoReviewModelOverrides === undefined && seed.autoReviewModelOverrides !== undefined) { + prov.autoReviewModelOverrides = { ...seed.autoReviewModelOverrides }; + } // Registry-only metadata (never seeded into saved config): backfill straight from // the entry so an explicit user value stays distinguishable from the default. if (prov.fastWire === undefined && entry.fastWire !== undefined) { @@ -626,6 +636,48 @@ function customPreset(): DerivedProviderPreset { return { id: "custom", label: "Custom provider", adapter: "openai-chat", baseUrl: "", auth: "key" }; } +/** + * Resolve the Codex auto-review (approvals) model for one routed model id. + * Per-model overrides win over the provider-wide default; both are opt-in and + * trimmed. Returns the configured target (bare id or `provider/model` slug) or + * null when the operator left the session-model behavior untouched. + */ +export function resolveAutoReviewModel( + provider: OcxProviderConfig | undefined, + modelId: string, +): string | null { + if (!provider) return null; + const perModel = autoReviewOverrideForModel(provider.autoReviewModelOverrides, modelId); + if (typeof perModel === "string" && perModel.trim() !== "") return perModel.trim(); + if (typeof provider.autoReviewModel === "string" && provider.autoReviewModel.trim() !== "") { + return provider.autoReviewModel.trim(); + } + return null; +} + +/** Per-model override lookup: exact model, exact :family, folded model, folded :family. */ +function autoReviewOverrideForModel( + overrides: Record | undefined, + modelId: string, +): string | undefined { + if (!overrides) return undefined; + if (Object.prototype.hasOwnProperty.call(overrides, modelId)) return overrides[modelId]; + const colon = modelId.indexOf(":"); + const folded = modelId.toLowerCase(); + for (const [key, value] of Object.entries(overrides)) { + if (key.toLowerCase() === folded) return value; + } + // The case-insensitive family lookup must run AFTER the folded full-model + // loop: a model-specific override for a colon-suffixed id wins over the + // family default even when both keys differ only in casing. + if (colon > 0) { + const family = modelId.slice(0, colon); + const familyMatch = Object.entries(overrides).find(([key]) => key.toLowerCase() === family.toLowerCase()); + if (familyMatch) return familyMatch[1]; + } + return undefined; +} + function formatInitLabel(entry: ProviderRegistryEntry): string { if (entry.authKind === "forward") return "OpenAI — ChatGPT login (no key; account pool default, Direct selectable)"; if (entry.authKind === "oauth") { diff --git a/src/providers/registry.ts b/src/providers/registry.ts index f78e82b965..c3fbc6a184 100644 --- a/src/providers/registry.ts +++ b/src/providers/registry.ts @@ -218,6 +218,10 @@ export interface ProviderRegistryEntry { * to stay contiguous. This is seeded/backfilled like other fixed wire capabilities. */ requiresAdjacentResponsesToolResults?: boolean; + /** Optional registry default for the Codex auto-review model (provider-wide). */ + autoReviewModel?: string; + /** Optional registry per-model auto-review defaults (model id -> approval model id). */ + autoReviewModelOverrides?: Record; /** * When enabled, tool results that are present but empty are annotated on the wire. * Seeded/backfilled like other fixed wire capabilities. diff --git a/src/router.ts b/src/router.ts index 758f34e751..7173db4225 100644 --- a/src/router.ts +++ b/src/router.ts @@ -382,6 +382,17 @@ export function routedProviderConfig(providerName: string, provider: OcxProvider && registryEntry.annotateEmptyToolOutputs !== undefined ? { annotateEmptyToolOutputs: registryEntry.annotateEmptyToolOutputs } : {}), + ...(provider.autoReviewModel === undefined && registryEntry.autoReviewModel !== undefined + ? { autoReviewModel: registryEntry.autoReviewModel } + : {}), + ...(provider.autoReviewModelOverrides !== undefined || registryEntry.autoReviewModelOverrides !== undefined + ? { + autoReviewModelOverrides: { + ...(registryEntry.autoReviewModelOverrides ?? {}), + ...(provider.autoReviewModelOverrides ?? {}), + }, + } + : {}), ...(provider.fastWire === undefined && registryEntry.fastWire !== undefined ? { fastWire: cloneFastWire(registryEntry.fastWire), diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index ccc23c5ef5..ab90a20a70 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -863,6 +863,8 @@ const PROVIDER_CONFIG_FIELD_POLICY = { escapeBuiltinToolNames: "editor", anthropicEofTolerance: "editor", noVisionModels: "editor", + autoReviewModel: "editor", + autoReviewModelOverrides: "editor", googleMode: "editor", project: "editor", location: "editor", @@ -1006,6 +1008,15 @@ export function safeConfigDTO(config: OcxConfig): unknown { } const selection = initialModelSelection(provider); if (selection) dto.initialModelSelection = selection; + if (typeof dto.autoReviewModel === "string") dto.autoReviewModel = redactSecretString(dto.autoReviewModel); + if (dto.autoReviewModelOverrides !== undefined && typeof dto.autoReviewModelOverrides === "object") { + dto.autoReviewModelOverrides = Object.fromEntries( + Object.entries(dto.autoReviewModelOverrides as Record).map(([key, value]) => [ + redactSecretString(key), + redactSecretString(typeof value === "string" ? value : String(value)), + ]), + ); + } providers[name] = dto; } return { diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index e26420e003..ac9c3cc4c7 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -7,6 +7,7 @@ import { clearGatherRoutedModelsInflight } from "../../codex/catalog/provider-fe import { DEFAULT_SUBAGENT_MODELS, adoptPersistedProviderIntoLiveConfig, + autoReviewModelConfigError, codexAutoStartEnabled, hasOwnProvider, isValidProviderName, @@ -14,6 +15,9 @@ import { multiAgentGuidanceEnabled, mutatePersistedConfig, nonBlankStringArrayConfigError, + normalizeAutoReviewModelField, + normalizeAutoReviewModelFields, + normalizeAutoReviewModelOverridesField, normalizeNonBlankStringArray, providerBaseUrlConfigError, providerHeadersConfigError, @@ -34,6 +38,7 @@ import { upsertOAuthProvider, } from "../../oauth"; import { replaceProviderAccountSet } from "../../oauth/store"; +import { redactSecretString } from "../../lib/redact"; import { providerDestinationResolvedError } from "../../lib/destination-policy"; import { reconcileLiveStateStores } from "../../lib/state-store-registrations"; import { ProviderOutboundPolicyError, providerOutboundGet, providerOutboundPost, providerRedirectError } from "../../lib/provider-outbound"; @@ -101,7 +106,6 @@ import { LOCAL_PROVIDER_RELOAD_PATH, } from "../../lib/local-provider-reload-contract"; import { refreshUserCostOverlays } from "../../usage/user-cost-overlays"; -import { redactSecretString } from "../../lib/redact"; import { XAI_RESPONSES_OPT_IN_MODELS, xaiResponsesOptInState, @@ -122,7 +126,6 @@ type ProviderPatchApplication = enablingOpenAi: boolean; headersTouched: boolean; }; - const PROVIDER_ALIAS_OVERLAY_FIELDS = ["alias", "modelAliases", "defaultAliases"] as const; type ProviderAliasOverlayField = typeof PROVIDER_ALIAS_OVERLAY_FIELDS[number]; @@ -251,6 +254,10 @@ function providerEditorCandidate( ?? providerEmptyToolOutputConfigError(name, transportCandidate) ?? providerServiceTierConfigError(name, transportCandidate); if (providerError) return { ok: false, status: 400, error: providerError, code: "invalid_provider" }; + const normalizationError = normalizeAutoReviewModelFields(name, merged); + if (normalizationError) { + return { ok: false, status: 400, error: normalizationError, code: "invalid_provider" }; + } providers[name] = merged; } @@ -273,7 +280,7 @@ function providerEditorCandidate( if (!validated.ok) { return { ok: false, status: 400, error: validated.error, code: "invalid_provider_editor_config" }; } - return { ok: true, config: candidate, removedProviders }; + return { ok: true, config: validated.config, removedProviders }; } function adoptProviderEditorCandidate(live: OcxConfig, persisted: OcxConfig): void { @@ -294,6 +301,16 @@ function adoptProviderEditorCandidate(live: OcxConfig, persisted: OcxConfig): vo else live.modelDiscovery = structuredClone(persisted.modelDiscovery); } +/** Redacted auto-review boundary error for provider writes. */ +function providerAutoReviewConfigError(name: string, provider: unknown): string | null { + if (!provider || typeof provider !== "object") return null; + const raw = provider as Record; + const error = autoReviewModelConfigError(name, raw.autoReviewModel, raw.autoReviewModelOverrides); + if (!error) return null; + if (name === "openai") return error; + return "provider " + JSON.stringify(redactSecretString(name)) + " " + error; +} + /** * Apply the recognized PATCH field mask onto a provider copy. The caller runs this once * for validation and again inside the config mutation lock against the newest provider, @@ -336,6 +353,31 @@ function applyProviderPatchFields( else delete next.defaultModel; touched = true; } + if (name === "openai" && (Object.hasOwn(rawBody, "autoReviewModel") || Object.hasOwn(rawBody, "autoReviewModelOverrides"))) { + return { error: "provider openai must not include autoReviewModel or autoReviewModelOverrides" }; + } + if (Object.hasOwn(rawBody, "autoReviewModel")) { + const normalized = normalizeAutoReviewModelField(rawBody.autoReviewModel); + if ("error" in normalized) return { error: normalized.error }; + if ("clear" in normalized) { + delete next.autoReviewModel; + touched = true; + } else { + next.autoReviewModel = normalized.value; + touched = true; + } + } + if (Object.hasOwn(rawBody, "autoReviewModelOverrides")) { + const normalized = normalizeAutoReviewModelOverridesField(rawBody.autoReviewModelOverrides); + if ("error" in normalized) return { error: normalized.error }; + if ("clear" in normalized) { + delete next.autoReviewModelOverrides; + touched = true; + } else { + next.autoReviewModelOverrides = normalized.value; + touched = true; + } + } if (Object.hasOwn(rawBody, "authMode")) { if (typeof rawBody.authMode !== "string") return { error: "authMode must be a string" }; const mode = rawBody.authMode.trim(); @@ -686,6 +728,13 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise [ + redactSecretString(key), + redactSecretString(value), + ])), modelSupportsServiceTier: p.modelSupportsServiceTier, noStructuredOutputModels: p.noStructuredOutputModels, retainModels: p.retainModels, @@ -891,6 +940,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise approval model id). Wins over + * `autoReviewModel`; same target grammar. Absent/undefined keeps session-model + * behavior for that model. + */ + autoReviewModelOverrides?: Record; /** * Provider fallback for canonical Fast capability over an OpenAI `service_tier` wire. * This pure tri-state feeds catalog publication, routing eligibility, compatibility diff --git a/structure/02_config-and-codex-home.md b/structure/02_config-and-codex-home.md index bb8ec5630f..b457a22534 100644 --- a/structure/02_config-and-codex-home.md +++ b/structure/02_config-and-codex-home.md @@ -455,3 +455,35 @@ the residual directory for manual review; there is no recursive-delete fallback. ## Remote client key files Client connection metadata stores a stable `apiKeyId` and a non-secret rotation `pendingOperation`. The current data secret remains only in `service-api-token`; a bounded rotation temporarily keeps the old secret in owner-only `service-api-token.prev`. Commit or recovery clears the marker before orphan cleanup. `ocx disconnect` is local-only and leaves remote revocation to the hub's **Integrations → API Keys** page. Hub and local usage stores are not mirrored. + +## Auto-review (approval) model override + +Codex picks its auto-review subagent from the session model's catalog row field +`auto_review_model_override`; when absent it uses the session model itself. Providers may opt in +per provider or per model: + +- `providers..autoReviewModel`: provider-wide default approval model for every routed row. +- `providers..autoReviewModelOverrides`: object mapping a session model id to its approval + model; the per-model value wins. + +A root Codex auto_review_model (config.toml, issue #1225) stays supported as the global +fallback: sync stamps it onto native rows and onto routed rows that carry no provider-level +override. Provider-level per-row values outrank that root selector on their own routed rows, +and removing the root selector never wipes provider-derived stamps. The provider fields are +therefore the per-provider scoped choice, while the root selector covers everything else. + +Targets are either a bare model id (resolved to `/` in the catalog) or a +`provider/model` catalog slug of any configured provider. The override is stamped onto the +routed catalog row during sync as `auto_review_model_override`; unknown bare targets are skipped +with a warning rather than emitted. A bare target resolves only when the id is also listed in +the provider's configured `models` or a matching registry entry; on pure live-discovery +providers without a static `models` list, add the sibling target id to `models` first (the +row's own model id always resolves). A final sync pass drops overrides that do not name an +emitted catalog model (fail closed). Malformed hand-edited values are sanitized at load instead +of retiring the config; the strict management boundary still rejects them. + +Canonical OpenAI providers (whose native/account rows are not routed) and combo aliases are +excluded from the override; those sessions keep Codex's session-model behavior. The management +API preserves the fields on unrelated provider saves and exposes PATCH/null clearing, but the v1 +GUI does not yet render editors — configure through the config file or `ocx config set`. The +feature is opt-in — no vendor defaults. diff --git a/tests/auto-review-model-override.test.ts b/tests/auto-review-model-override.test.ts new file mode 100644 index 0000000000..d2dbd8826c --- /dev/null +++ b/tests/auto-review-model-override.test.ts @@ -0,0 +1,484 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { resolveAutoReviewModel } from "../src/providers/derive"; +import { applyAutoReviewModelOverride, deriveEntry, resetCatalogRuntimeStateForTests, validateAutoReviewOverridesAgainstCatalog } from "../src/codex/catalog/sync"; +import { applyProviderConfigHints } from "../src/codex/catalog/provider-fetch"; +import { autoReviewModelConfigError, normalizeAutoReviewModelFields } from "../src/config/provider-validation"; +import { loadConfig } from "../src/config"; +import { PROVIDER_REGISTRY } from "../src/providers/registry"; +import { providerConfigSeed } from "../src/providers/derive"; +import { routedProviderConfig } from "../src/router"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import type { CatalogModel } from "../src/codex/catalog/parsing"; +import type { OcxProviderConfig } from "../src/types"; + +describe("resolveAutoReviewModel", () => { + test("per-model override wins over provider-wide", () => { + const provider = { + autoReviewModel: "deepseek-v4-pro", + autoReviewModelOverrides: { "deepseek-v4-flash-vision-exp": "deepseek-v4-flash" }, + } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash-vision-exp")).toBe("deepseek-v4-flash"); + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash")).toBe("deepseek-v4-pro"); + }); + + test("provider-wide default applies when no per-model entry exists", () => { + const provider = { autoReviewModel: " deepseek-v4-flash " } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "anything")).toBe("deepseek-v4-flash"); + }); + + test("per-model lookup falls back to the :family prefix", () => { + const provider = { + autoReviewModelOverrides: { + "deepseek-v4-flash-vision-exp": "deepseek-v4-flash", + }, + } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash-vision-exp:beta")).toBe("deepseek-v4-flash"); + }); + + test("per-model lookup case-folds override keys", () => { + const provider = { + autoReviewModelOverrides: { + "DEEPSEEK-V4-FLASH": "deepseek-v4-pro", + }, + } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash")).toBe("deepseek-v4-pro"); + }); + + test("per-model lookup case-folds the :family prefix", () => { + const provider = { + autoReviewModelOverrides: { + "DEEPSEEK-V4-FLASH": "deepseek-v4-pro", + }, + } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash:beta")).toBe("deepseek-v4-pro"); + }); + + test("a case-differing full-model override wins over the case-folded family", () => { + const provider = { + autoReviewModelOverrides: { + "DEEPSEEK-V4-FLASH:beta": "deepseek-v4-flash-specific", + "DEEPSEEK-V4-FLASH": "deepseek-v4-pro", + }, + } as OcxProviderConfig; + expect(resolveAutoReviewModel(provider, "deepseek-v4-flash:beta")).toBe("deepseek-v4-flash-specific"); + }); + + test("returns null when nothing is configured", () => { + expect(resolveAutoReviewModel({} as OcxProviderConfig, "m")).toBeNull(); + expect(resolveAutoReviewModel(undefined, "m")).toBeNull(); + }); +}); + +describe("catalog stamping", () => { + const template = { auto_review_model_override: null, context_window: 272000 } as Record; + + test("stamps a pre-normalized provider/model slug verbatim", () => { + const model: CatalogModel = { + id: "deepseek-v4-flash-vision-exp", + provider: "deepseek", + autoReviewModelOverride: "deepseek/deepseek-v4-flash", + }; + const entry = deriveEntry(template, "deepseek/deepseek-v4-flash-vision-exp", "desc", 5, model); + expect(entry.auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + }); + + test("stamps the override in the no-template fallback branch", () => { + const model: CatalogModel = { + id: "m", + provider: "blsc", + autoReviewModelOverride: "deepseek/deepseek-v4-flash", + }; + const entry = deriveEntry(null, "blsc/m", "desc", 5, model); + expect(entry.auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + }); + + test("no override keeps the template value", () => { + const model: CatalogModel = { id: "m", provider: "blsc" }; + const entry = deriveEntry(template, "blsc/m", "desc", 5, model); + expect(entry.auto_review_model_override).toBeNull(); + }); +}); + +describe("provider-fetch hints", () => { + test("attaches the override for a known bare target", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com", + models: ["deepseek-v4-flash", "deepseek-v4-flash-vision-exp"], + autoReviewModel: "deepseek-v4-flash", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("deepseek", prov, { + id: "deepseek-v4-flash-vision-exp", + provider: "deepseek", + }); + expect(hinted.autoReviewModelOverride).toBe("deepseek/deepseek-v4-flash"); + }); + + test("case-differing target matches and encodes the canonical provider id", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com", + models: ["deepseek-v4-flash"], + autoReviewModel: "DeepSeek-V4-Flash", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("deepseek", prov, { + id: "deepseek-v4-flash-vision-exp", + provider: "deepseek", + }); + expect(hinted.autoReviewModelOverride).toBe("deepseek/deepseek-v4-flash"); + }); + + test("current row id wins case-folded canonicalization so the slug matches the emitted row", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com", + models: ["DeepSeek-V4-Flash"], + autoReviewModel: "DeepSeek-V4-Flash", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("deepseek", prov, { + id: "deepseek-v4-flash", + provider: "deepseek", + }); + expect(hinted.autoReviewModelOverride).toBe("deepseek/deepseek-v4-flash"); + const entries: Array> = [ + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: hinted.autoReviewModelOverride }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + }); + + test("captured known-model ids back membership without a registry read", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://custom-openai.example/v1", + autoReviewModel: "DeepSeek-V4-Flash", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints( + "deepseek", + prov, + { id: "deepseek-v4-flash", provider: "deepseek" }, + undefined, + undefined, + ["DeepSeek-V4-Flash"], + ); + expect(hinted.autoReviewModelOverride).toBe("deepseek/deepseek-v4-flash"); + }); + + test("encodes a slashed native id into the provider slug", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://zenmux.example/v1", + models: ["moonshotai/kimi-k3-free"], + autoReviewModel: "moonshotai/kimi-k3-free", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("zenmux", prov, { + id: "moonshotai/kimi-k3-free", + provider: "zenmux", + }); + expect(hinted.autoReviewModelOverride).toBe("zenmux/moonshotai-kimi-k3-free"); + }); + + test("keeps a namespaced cross-provider target verbatim", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://blsc.example/v1", + models: ["glm-5.2"], + autoReviewModel: "deepseek/deepseek-v4-flash", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("blsc", prov, { id: "glm-5.2", provider: "blsc" }); + expect(hinted.autoReviewModelOverride).toBe("deepseek/deepseek-v4-flash"); + }); + + test("skips an unknown bare target without stamping", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://api.deepseek.com", + models: ["deepseek-v4-flash"], + autoReviewModel: "does-not-exist", + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("deepseek", prov, { + id: "deepseek-v4-flash-vision-exp", + provider: "deepseek", + }); + expect(hinted.autoReviewModelOverride).toBeUndefined(); + }); + + test("clears a stale override when the config is removed", () => { + const prov = { + adapter: "openai-chat", + baseUrl: "https://blsc.example/v1", + models: ["glm-5.2"], + } as unknown as OcxProviderConfig; + const hinted = applyProviderConfigHints("blsc", prov, { + id: "glm-5.2", + provider: "blsc", + autoReviewModelOverride: "stale/deepseek-v4-pro", + }); + expect(hinted.autoReviewModelOverride).toBeUndefined(); + }); +}); + +describe("assembled-catalog validation", () => { + test("drops overrides that do not name an emitted model", () => { + const entries: Array> = [ + { slug: "blsc/glm-5.2", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + { slug: "deepseek/deepseek-v4-flash" }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + entries[0].auto_review_model_override = "blsc/does-not-exist"; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBeNull(); + }); + + test("native rows keep upstream-retained overrides even when the target is not emitted", () => { + const entries: Array> = [ + { slug: "gpt-5.4", auto_review_model_override: "native-upstream" }, + { slug: "static/deepseek-v4-flash", auto_review_model_override: "static/deepseek-v4-flash" }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBe("native-upstream"); + expect(entries[1].auto_review_model_override).toBe("static/deepseek-v4-flash"); + }); + + test("missing or malformed slugs never become matchable override targets", () => { + const entries: Array> = [ + { slug: undefined, auto_review_model_override: "undefined" }, + { slug: null, auto_review_model_override: "null" }, + { slug: "", auto_review_model_override: "blsc/m" }, + { slug: "other/m" }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBeNull(); + expect(entries[1].auto_review_model_override).toBeNull(); + expect(entries[2].auto_review_model_override).toBeNull(); + }); + + test("normalizes wrong-shaped routed overrides to null before serialization", () => { + const entries: Array> = [ + { slug: "blsc/glm-5.2", auto_review_model_override: 42 }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: { nested: true } }, + { slug: "static/null", auto_review_model_override: null }, + { slug: "static/undefined", auto_review_model_override: undefined }, + { slug: "deepseek/deepseek-v4-pro" }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBeNull(); + expect(entries[1].auto_review_model_override).toBeNull(); + expect(entries[2].auto_review_model_override).toBeNull(); + expect(entries[3].auto_review_model_override).toBeUndefined(); + }); + + test("native rows normalize wrong-shaped overrides but keep valid nonblank strings", () => { + const entries: Array> = [ + { slug: "gpt-5.4", auto_review_model_override: 42 }, + { slug: "gpt-5.4-mini", auto_review_model_override: { nested: true } }, + { slug: "gpt-5.4-nano", auto_review_model_override: "" }, + { slug: "gpt-5.4-plus", auto_review_model_override: "native-upstream" }, + { slug: "gpt-5.4-ultra", auto_review_model_override: null }, + ]; + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBeNull(); + expect(entries[1].auto_review_model_override).toBeNull(); + expect(entries[2].auto_review_model_override).toBeNull(); + expect(entries[3].auto_review_model_override).toBe("native-upstream"); + expect(entries[4].auto_review_model_override).toBeNull(); + }); +}); + +describe("load sanitization", () => { + let testRoot = ""; + let previousHome: string | undefined; + + beforeEach(() => { + previousHome = process.env.OPENCODEX_HOME; + testRoot = mkdtempSync(join(import.meta.dir, ".tmp-auto-review-load-")); + process.env.OPENCODEX_HOME = testRoot; + }); + + afterEach(() => { + if (previousHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousHome; + rmSync(testRoot, { recursive: true, force: true }); + }); + + test("malformed auto-review fields are sanitized instead of retiring the config", () => { + writeFileSync(join(testRoot, "config.json"), JSON.stringify({ + port: 10100, + defaultProvider: "test", + providers: { + test: { + adapter: "openai-chat", + baseUrl: "https://example.test/v1", + apiKey: "sk-test", + autoReviewModel: " ", + autoReviewModelOverrides: { " bad key ": "deepseek-v4-flash", "glm-5.2": " " }, + }, + }, + })); + const loaded = loadConfig(); + expect(loaded.providers.test?.autoReviewModel).toBeUndefined(); + expect(loaded.providers.test?.autoReviewModelOverrides).toBeUndefined(); + }); +}); + +describe("management validation and normalization", () => { + test("management rejects malformed overrides", () => { + expect(autoReviewModelConfigError("custom", " ", undefined)).toContain("autoReviewModel"); + expect(autoReviewModelConfigError("custom", undefined, { m: 42 })).toContain("autoReviewModelOverrides"); + expect(autoReviewModelConfigError("custom", "deepseek-v4-flash", { "deepseek-v4-flash-vision-exp": "deepseek-v4-pro" })).toBeNull(); + expect(autoReviewModelConfigError("openai", "deepseek-v4-flash", undefined)) + .toContain("provider openai must not include autoReviewModel"); + }); + + test("POST normalization trims auto-review fields before persistence", () => { + const provider = { + autoReviewModel: " deepseek-v4-flash ", + autoReviewModelOverrides: { + " deepseek-v4-flash-vision-exp ": " deepseek-v4-pro ", + }, + }; + expect(normalizeAutoReviewModelFields("custom", provider)).toBeNull(); + expect(provider.autoReviewModel).toBe("deepseek-v4-flash"); + expect(provider.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash-vision-exp": "deepseek-v4-pro", + }); + }); + + test("POST normalization rejects whitespace inside ids", () => { + const provider = { autoReviewModel: "deep seek-v4-flash" }; + expect(normalizeAutoReviewModelFields("custom", provider)).toContain("autoReviewModel"); + }); +}); + +describe("routed provider merge", () => { + test("autoReviewModelOverrides merge per key with provider winning on overlap", () => { + const entry = PROVIDER_REGISTRY.find(e => e.id === "deepseek"); + expect(entry).toBeDefined(); + const saved = entry!.autoReviewModelOverrides; + entry!.autoReviewModelOverrides = { + "deepseek-v4-flash": "deepseek-v4-pro", + shared: "registry-default", + }; + try { + const provider = providerConfigSeed(entry!); + provider.autoReviewModelOverrides = { + "deepseek-v4-flash-vision-exp": "deepseek-v4-flash", + shared: "provider-override", + }; + const routed = routedProviderConfig("deepseek", provider); + expect(routed.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash": "deepseek-v4-pro", + "deepseek-v4-flash-vision-exp": "deepseek-v4-flash", + shared: "provider-override", + }); + } finally { + if (saved === undefined) delete entry!.autoReviewModelOverrides; + else entry!.autoReviewModelOverrides = saved; + } + }); + + test("registry autoReviewModel fallback applies when the provider leaves it undefined", () => { + const entry = PROVIDER_REGISTRY.find(e => e.id === "deepseek"); + expect(entry).toBeDefined(); + const saved = entry!.autoReviewModel; + entry!.autoReviewModel = "deepseek/deepseek-v4-flash"; + try { + const provider = providerConfigSeed(entry!); + delete provider.autoReviewModel; + const routed = routedProviderConfig("deepseek", provider); + expect(routed.autoReviewModel).toBe("deepseek/deepseek-v4-flash"); + } finally { + if (saved === undefined) delete entry!.autoReviewModel; + else entry!.autoReviewModel = saved; + } + }); +}); + +describe("global auto_review_model precedence (provider stamp vs root selector)", () => { + test("absent root selector keeps provider-stamped routed rows after apply", () => { + const entries: Array> = [ + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + { slug: "deepseek/deepseek-v4-pro", auto_review_model_override: null }, + ]; + + const result = applyAutoReviewModelOverride(entries, null, [], { retainRoutedOverrides: true }); + + expect(result).toBe("absent"); + expect(entries[0].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(entries[1].auto_review_model_override).toBeNull(); + }); + + test("present root selector fills native and unstamped routed rows but keeps provider stamps", () => { + const entries: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: "native-upstream" }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + { slug: "deepseek/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "blsc/glm-5.2", auto_review_model_override: undefined }, + ]; + + const result = applyAutoReviewModelOverride( + entries, + " deepseek/deepseek-v4-pro ", + [], + { retainRoutedOverrides: true }, + ); + + expect(result).toBe("applied"); + // Provider-scoped stamp outranks the root fallback on its own routed row. + expect(entries[1].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + // Native and unstamped routed rows receive the root selector. + expect(entries[0].auto_review_model_override).toBe("deepseek/deepseek-v4-pro"); + expect(entries[2].auto_review_model_override).toBe("deepseek/deepseek-v4-pro"); + expect(entries[3].auto_review_model_override).toBe("deepseek/deepseek-v4-pro"); + }); + + test("legacy apply behavior is unchanged when retainRoutedOverrides is not set", () => { + const entries: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: "native-upstream" }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + ]; + + applyAutoReviewModelOverride(entries, null); + + expect(entries[0].auto_review_model_override).toBe("native-upstream"); + expect(entries[1].auto_review_model_override).toBeNull(); + }); + + test("removing a previously applied root selector clears stale native copies but keeps differing provider stamps", () => { + resetCatalogRuntimeStateForTests(); + try { + const entries: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: "opencode-go/deepseek-v4-pro" }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + { slug: "blsc/glm-5.2", auto_review_model_override: "blsc/glm-5.2" }, + ]; + + const applied = applyAutoReviewModelOverride( + entries, + "opencode-go/deepseek-v4-pro", + [], + { retainRoutedOverrides: true }, + ); + expect(applied).toBe("applied"); + expect(entries[0].auto_review_model_override).toBe("opencode-go/deepseek-v4-pro"); + expect(entries[2].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(entries[3].auto_review_model_override).toBe("blsc/glm-5.2"); + + const removed = applyAutoReviewModelOverride( + entries, + null, + [], + { retainRoutedOverrides: true }, + ); + expect(removed).toBe("absent"); + expect(entries[0].auto_review_model_override).toBeNull(); + expect(entries[2].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(entries[3].auto_review_model_override).toBe("blsc/glm-5.2"); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); +}); diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index 37d8c69798..c5865c0cad 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -3703,6 +3703,99 @@ describe("Codex catalog routed normalization", () => { expect(pinned.multi_agent_reasoning_effort).toBe("xhigh"); }); + test("trusted openai-api rebuild keeps the configured auto-review override", () => { + const apiRows = augmentRoutedModelsWithRegistryOpenAiApiRows([], openAiApiCatalogConfig({ + models: ["daybreak-blue-latest"], + autoReviewModel: "daybreak-blue-latest", + })); + const row = apiRows.find(candidate => candidate.provider === "openai-apikey" && candidate.id === "daybreak-blue-latest"); + expect(row?.autoReviewModelOverride).toBe("openai-apikey/daybreak-blue-latest"); + }); + + test("trusted openai-api rebuild drops the auto-review override when the provider setting is removed", () => { + const withOverride = augmentRoutedModelsWithRegistryOpenAiApiRows([], openAiApiCatalogConfig({ + models: ["daybreak-blue-latest"], + autoReviewModel: "daybreak-blue-latest", + })); + const rowWith = withOverride.find(candidate => candidate.provider === "openai-apikey" && candidate.id === "daybreak-blue-latest"); + expect(rowWith?.autoReviewModelOverride).toBe("openai-apikey/daybreak-blue-latest"); + + // A rebuild without the provider setting must not retain the stale override. + const withoutOverride = augmentRoutedModelsWithRegistryOpenAiApiRows(withOverride, openAiApiCatalogConfig({ + models: ["daybreak-blue-latest"], + })); + const rowWithout = withoutOverride.find(candidate => candidate.provider === "openai-apikey" && candidate.id === "daybreak-blue-latest"); + expect(rowWithout?.autoReviewModelOverride).toBeUndefined(); + }); + + test("gatherRoutedModels custom-model rebuild drops the auto-review override when the provider setting is removed", async () => { + resetCatalogRuntimeStateForTests(); + try { + const base = { + port: 10100, + defaultProvider: "openai", + providers: { + openai: { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + authMode: "key", + liveModels: false, + autoReviewModel: "custom-approver", + }, + }, + customModels: [{ id: "custom-approver", provider: "openai", modelId: "custom-approver" }], + } as Parameters[0]; + const withOverride = await gatherRoutedModelsDirect(base); + const rowWith = withOverride.find(model => model.provider === "openai" && model.id === "custom-approver"); + expect(rowWith?.autoReviewModelOverride).toBe("openai/custom-approver"); + + const without = { + ...base, + providers: { + openai: { + ...base.providers.openai, + autoReviewModel: undefined, + }, + }, + }; + const rebuilt = await gatherRoutedModelsDirect(without); + const rowWithout = rebuilt.find(model => model.provider === "openai" && model.id === "custom-approver"); + expect(rowWithout?.autoReviewModelOverride).toBeUndefined(); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); + + test("gatherRoutedModels resolves a bare auto-review target to a sibling custom row", async () => { + resetCatalogRuntimeStateForTests(); + try { + const base = { + port: 10100, + defaultProvider: "openai", + providers: { + openai: { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + authMode: "key", + liveModels: false, + autoReviewModelOverrides: { worker: "reviewer" }, + }, + }, + customModels: [ + { id: "worker", provider: "openai", modelId: "worker" }, + { id: "reviewer", provider: "openai", modelId: "reviewer" }, + ], + } as Parameters[0]; + const models = await gatherRoutedModelsDirect(base); + const worker = models.find(model => model.provider === "openai" && model.id === "worker"); + expect(worker?.autoReviewModelOverride).toBe("openai/reviewer"); + const reviewer = models.find(model => model.provider === "openai" && model.id === "reviewer"); + expect(reviewer?.autoReviewModelOverride).toBeUndefined(); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); + test("Daybreak metadata inheritance rejects noncanonical providers", async () => { const models = await gatherRoutedModels({ port: 10100, diff --git a/tests/codex-integration/codex-convergence-account-selectors.test.ts b/tests/codex-integration/codex-convergence-account-selectors.test.ts index ae83c1f2bf..29bbd1b6a1 100644 --- a/tests/codex-integration/codex-convergence-account-selectors.test.ts +++ b/tests/codex-integration/codex-convergence-account-selectors.test.ts @@ -706,6 +706,58 @@ test("retained and convergence writers resolve, clear, reject, and recover auto- } }); +test("retained and convergence writers keep provider auto-review stamps and use the root selector only as fallback", async () => { + primeCodexRuntimeFixture(); + + const providerAutoReviewConfig = (): OcxConfig => { + const nextConfig = config(false); + nextConfig.providers.static = { + adapter: "openai-chat", + baseUrl: "https://static.example.test/v1", + liveModels: false, + models: ["deepseek-v4-flash", "deepseek-v4-pro"], + autoReviewModel: "deepseek-v4-flash", + }; + return nextConfig; + }; + + 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; + }; + + // No root selector: provider-level stamps survive sync untouched. + writeAutoReviewModel(); + writeCatalog(autoReviewSeed(null)); + let catalog = await write(providerAutoReviewConfig()); + 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/deepseek-v4-pro")) + .toHaveProperty("auto_review_model_override", "static/deepseek-v4-flash"); + expect(catalog.models?.find(entry => entry.slug === "gpt-5.4")) + .toHaveProperty("auto_review_model_override", "native-upstream"); + + // Root selector present: routed provider stamps keep winning; the root value + // becomes the fallback applied to native rows without a provider stamp. + writeAutoReviewModel("static/deepseek-v4-pro"); + writeCatalog(autoReviewSeed(null)); + catalog = await write(providerAutoReviewConfig()); + 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/deepseek-v4-pro")) + .toHaveProperty("auto_review_model_override", "static/deepseek-v4-flash"); + expect(catalog.models?.find(entry => entry.slug === "gpt-5.4")) + .toHaveProperty("auto_review_model_override", "static/deepseek-v4-pro"); + } +}); + test("degraded preservation still honors explicit routed visibility policy", async () => { writeCatalog([ nativeEntry(), diff --git a/tests/codex-integration/codex-gather-authority.test.ts b/tests/codex-integration/codex-gather-authority.test.ts index ae3204a0a7..2b476d389a 100644 --- a/tests/codex-integration/codex-gather-authority.test.ts +++ b/tests/codex-integration/codex-gather-authority.test.ts @@ -128,6 +128,8 @@ describe("catalog gather discovery-policy authority", () => { baseUrl: "https://custom-openai.example/v1", authMode: "key", apiKey: "custom-openai-secret", + models: ["custom-only"], + autoReviewModel: "custom-only", }, }, }); @@ -155,8 +157,10 @@ describe("catalog gather discovery-policy authority", () => { try { responseGate.resolve(); const models = await pending; - expect(models.filter(model => model.provider === "openai-apikey").map(model => model.id)) + const rows = models.filter(model => model.provider === "openai-apikey"); + expect(rows.map(model => model.id)) .toEqual(["custom-only"]); + expect(rows[0]?.autoReviewModelOverride).toBe("openai-apikey/custom-only"); } finally { PROVIDER_REGISTRY.find = originalFind; responseGate.resolve(); diff --git a/tests/providers/provider-config-batch-management.test.ts b/tests/providers/provider-config-batch-management.test.ts index c0646c98f9..e992442ce7 100644 --- a/tests/providers/provider-config-batch-management.test.ts +++ b/tests/providers/provider-config-batch-management.test.ts @@ -170,6 +170,37 @@ describe("atomic provider editor batch", () => { }); }); + test("provider editor PUT persists normalized trimmed auto-review fields", async () => { + const liveConfig = seededConfig(); + saveConfig(liveConfig); + const baseline = editorBaseline(liveConfig); + const next: EditorConfig = structuredClone(baseline); + next.providers.alpha = { + ...baseline.providers.alpha, + autoReviewModel: " alpha-approver ", + autoReviewModelOverrides: { " alpha-worker ": " alpha-reviewer " }, + }; + + const destinationSpy = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + let response: Response | null; + try { + response = await putBatch(liveConfig, { baseline, next }); + } finally { + destinationSpy.mockRestore(); + } + expect(response?.status).toBe(200); + const persisted = loadConfig(); + expect(persisted.providers.alpha).toMatchObject({ + autoReviewModel: "alpha-approver", + autoReviewModelOverrides: { "alpha-worker": "alpha-reviewer" }, + }); + const onDisk = JSON.parse(readFileSync(getConfigPath(), "utf8")) as OcxConfig; + expect(onDisk.providers.alpha).toMatchObject({ + autoReviewModel: "alpha-approver", + autoReviewModelOverrides: { "alpha-worker": "alpha-reviewer" }, + }); + }); + test("updates several providers in one commit and preserves credentials and private fields", async () => { const liveConfig = seededConfig(); saveConfig(liveConfig); diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index 35a7924ebe..f556486dcc 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1034,6 +1034,208 @@ describe("provider management validation", () => { } }); + test("provider POST normalizes auto-review fields before persisting config.json", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + + const server = startServer(0); + try { + const create = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "auto-review", + provider: { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + autoReviewModel: " deepseek-v4-flash ", + autoReviewModelOverrides: { + " deepseek-v4-flash-vision-exp ": " deepseek-v4-pro ", + }, + }, + }), + }); + expect(create.status).toBe(200); + const persisted = JSON.parse(readFileSync(join(TEST_DIR, "config.json"), "utf8")) as OcxConfig; + expect(persisted.providers["auto-review"]?.autoReviewModel).toBe("deepseek-v4-flash"); + expect(persisted.providers["auto-review"]?.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash-vision-exp": "deepseek-v4-pro", + }); + const listed = await fetch(new URL("/api/providers", server.url)).then(response => response.json()) as Array<{ + name: string; + autoReviewModel?: string; + autoReviewModelOverrides?: Record; + }>; + const row = listed.find(provider => provider.name === "auto-review"); + expect(row?.autoReviewModel).toBe("deepseek-v4-flash"); + expect(row?.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash-vision-exp": "deepseek-v4-pro", + }); + } finally { + await server.stop(true); + } + }); + + test("provider GET redacts credential-shaped auto-review values", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + + const server = startServer(0); + try { + const tokenModel = "sk-" + "live-" + "a".repeat(30); + const tokenOverride = "sk-" + "live-" + "b".repeat(30); + const create = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "auto-review", + provider: { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + autoReviewModel: tokenModel, + autoReviewModelOverrides: { "deepseek-v4-flash": tokenOverride }, + }, + }), + }); + expect(create.status).toBe(200); + + const listed = await fetch(new URL("/api/providers", server.url)).then(response => response.json()) as Array<{ + name: string; + autoReviewModel?: string; + autoReviewModelOverrides?: Record; + }>; + const row = listed.find(provider => provider.name === "auto-review"); + expect(row?.autoReviewModel).not.toContain(tokenModel); + expect(row?.autoReviewModel).toContain("[REDACTED]"); + expect(row?.autoReviewModelOverrides?.["deepseek-v4-flash"]).not.toContain(tokenOverride); + expect(row?.autoReviewModelOverrides?.["deepseek-v4-flash"]).toContain("[REDACTED]"); + } finally { + await server.stop(true); + } + }); + + + test("GET /api/config redacts credential-shaped auto-review values", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + + const server = startServer(0); + try { + const tokenModel = "sk-" + "live-" + "c".repeat(30); + const tokenOverride = "sk-" + "live-" + "d".repeat(30); + const create = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "auto-review", + provider: { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + autoReviewModel: tokenModel, + autoReviewModelOverrides: { "deepseek-v4-flash": tokenOverride }, + }, + }), + }); + expect(create.status).toBe(200); + + const dto = await fetch(new URL("/api/config", server.url)).then(response => response.json()) as { + providers: Record; + }>; + }; + const row = dto.providers["auto-review"]; + expect(row?.autoReviewModel).toBeDefined(); + expect(row?.autoReviewModel).not.toContain(tokenModel); + expect(row?.autoReviewModel).toContain("[REDACTED]"); + expect(row?.autoReviewModelOverrides?.["deepseek-v4-flash"]).toBeDefined(); + expect(row?.autoReviewModelOverrides?.["deepseek-v4-flash"]).not.toContain(tokenOverride); + expect(row?.autoReviewModelOverrides?.["deepseek-v4-flash"]).toContain("[REDACTED]"); + } finally { + await server.stop(true); + } + }); + + test("provider POST redacts token-shaped names in auto-review errors", async () => { + + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + + const server = startServer(0); + try { + const tokenName = "sk-" + "live-" + "a".repeat(30); + const response = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: tokenName, + provider: { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + autoReviewModel: " ", + }, + }), + }); + expect(response.status).toBe(400); + const body = (await response.json()) as { error: string }; + expect(body.error).toContain("autoReviewModel"); + expect(body.error).not.toContain(tokenName); + } finally { + await server.stop(true); + } + }); + + test("canonical openai rejects auto-review fields without persisting", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + ...config("127.0.0.1"), + providers: poolProviders(), + }); + + const server = startServer(0); + try { + const before = JSON.stringify(loadConfig().providers.openai); + const post = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "openai", + provider: { + adapter: "openai-responses", + baseUrl: "https://chatgpt.com/backend-api/codex", + authMode: "forward", + codexAccountMode: "pool", + autoReviewModel: "deepseek-v4-flash", + autoReviewModelOverrides: { "deepseek-v4-flash-vision-exp": "deepseek-v4-pro" }, + }, + }), + }); + expect(post.status).toBe(400); + expect(await post.json()).toMatchObject({ error: expect.stringContaining("autoReviewModel") }); + + const patch = await fetch(new URL("/api/providers?name=openai", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ autoReviewModel: "deepseek-v4-flash" }), + }); + expect(patch.status).toBe(400); + expect(await patch.json()).toMatchObject({ error: expect.stringContaining("autoReviewModel") }); + expect(JSON.stringify(loadConfig().providers.openai)).toBe(before); + } finally { + await server.stop(true); + } + }); + // #1409: the add/edit form's payload type has no member for contextWindow or test("canonical OpenAI can set, clear, and persist annotateEmptyToolOutputs via PATCH", async () => { // The canonical seed comparison rejects any provider that diverges from the built-in @@ -1412,6 +1614,49 @@ describe("provider management validation", () => { } }); + test("provider POST overwrite preserves auto-review fields when the payload omits them", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig(config("127.0.0.1")); + + const server = startServer(0); + try { + const create = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "relay", + provider: { + adapter: "openai-chat", + baseUrl: "https://relay.example/v1", + autoReviewModel: "approver-a", + autoReviewModelOverrides: { "coder-1": "approver-b" }, + }, + }), + }); + expect(create.status).toBe(200); + expect(loadConfig().providers.relay?.autoReviewModel).toBe("approver-a"); + expect(loadConfig().providers.relay?.autoReviewModelOverrides).toEqual({ "coder-1": "approver-b" }); + + // An unrelated overwrite that omits both fields must preserve the operator's + // auto-review configuration instead of erasing it. + const overwrite = await fetch(new URL("/api/providers", server.url), { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "relay", + provider: { adapter: "openai-chat", baseUrl: "https://relay.example/v1" }, + }), + }); + expect(overwrite.status).toBe(200); + expect(loadConfig().providers.relay?.autoReviewModel).toBe("approver-a"); + expect(loadConfig().providers.relay?.autoReviewModelOverrides).toEqual({ "coder-1": "approver-b" }); + } finally { + await server.stop(true); + } + }); + // #1409: the add/edit form's payload type has no member for contextWindow or // modelContextWindows, so an overwrite arrives without them. Registry enrichment then fills // the absent fields from the seed and the stored row loses the user's values — for From d458ba347657bd540dd050c3597069b88107c378 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Thu, 3 Sep 2026 17:42:12 +0800 Subject: [PATCH 2/7] fix(catalog): retain/vertex membership, durable root provenance, canonical stamps, POST re-read --- src/codex/catalog/provider-fetch.ts | 14 +++++- src/codex/catalog/sync.ts | 30 +++++++++---- src/server/management/provider-routes.ts | 8 ++-- tests/auto-review-model-override.test.ts | 43 +++++++++++++++++++ tests/codex-integration/codex-catalog.test.ts | 28 ++++++++++++ .../provider-config-batch-management.test.ts | 30 +++++++++++++ 6 files changed, 140 insertions(+), 13 deletions(-) diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index ace2815546..3803ca7e6f 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -431,7 +431,19 @@ function captureProviderGather( enrichProviderFromRegistry(name, enriched); const registryTransportMatch = providerMatchesRegistryTransport(name, enriched); const provider = recursivelyFreeze(enriched); - const knownModelIds = Object.freeze([...new Set(provider.models ?? [])]); + // Mirror the exact static seed used later by fetchProviderModelsWithAuth: the + // Vertex default and retainModels participate in catalog emission even when they are + // absent from provider.models, so membership for bare auto-review targets must + // include them too (a retain-only model cannot otherwise be named as an approver). + const seedVertexDefault = provider.adapter === "google" + && provider.googleMode === "vertex" + && (provider.models?.length ?? 0) === 0 + && Boolean(provider.defaultModel); + const knownModelIds = Object.freeze([...new Set([ + ...(seedVertexDefault && provider.defaultModel ? [provider.defaultModel] : []), + ...(provider.models ?? []), + ...(provider.retainModels ?? []), + ])]); const fastPolicyAuthority = captureFastPolicyAuthority( name, provider, diff --git a/src/codex/catalog/sync.ts b/src/codex/catalog/sync.ts index 20879765f3..0ac7187b11 100644 --- a/src/codex/catalog/sync.ts +++ b/src/codex/catalog/sync.ts @@ -1514,11 +1514,16 @@ function clearAutoReviewModelOverride( for (const entry of models) { if (!entry || typeof entry !== "object") continue; const current = entry.auto_review_model_override; + const rootMarker = entry.opencodex_auto_review_root; + const staleRootCopy = typeof rootMarker === "string" + && typeof current === "string" + && current === rootMarker; + if (rootMarker !== undefined) delete entry.opencodex_auto_review_root; if (isRoutedCatalogEntry(entry)) { // A freshly derived provider stamp (nonblank valid string) is kept when // retaining is requested; everything else on routed rows is cleared so // stale root-selector residue cannot persist across regenerations. - if (retainRoutedOverrides && typeof current === "string" && isValidAutoReviewModel(current)) continue; + if (retainRoutedOverrides && typeof current === "string" && isValidAutoReviewModel(current) && !staleRootCopy) continue; entry.auto_review_model_override = null; continue; } @@ -1527,7 +1532,7 @@ function clearAutoReviewModelOverride( && current === previousRootSelector; const matchesHeuristic = previousRootSelector === null && globalStamp && configuredValues.has(current); - if (matchesPreviousRoot || matchesHeuristic) entry.auto_review_model_override = null; + if (staleRootCopy || matchesPreviousRoot || matchesHeuristic) entry.auto_review_model_override = null; } } } @@ -1549,17 +1554,20 @@ 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, rootMarker: entry.opencodex_auto_review_root }); } 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 ?? null; + if (saved.rootMarker !== undefined) entry.opencodex_auto_review_root = saved.rootMarker; + else delete entry.opencodex_auto_review_root; } } @@ -1588,12 +1596,17 @@ export function applyAutoReviewModelOverride( warnAutoReviewModelDiagnostic("invalid", trimmed); return "invalid"; } - if (!configuredCatalogEntry(models, trimmed)) { + const matchedConfigured = configuredCatalogEntry(models, trimmed); + if (!matchedConfigured) { clearAutoReviewModelOverride(models, sourceModels, retainRouted, lastAppliedRootAutoReviewModel); lastAppliedRootAutoReviewModel = null; warnAutoReviewModelDiagnostic("unresolved", trimmed); return "unresolved"; } + // Stamp the matched catalog row’s actual slug: final validation compares exact slugs, so a + // case-equivalent or otherwise equivalent selector must not be written in raw form only to + // be dropped by validateAutoReviewOverridesAgainstCatalog. + const canonicalRoot = typeof matchedConfigured.slug === "string" ? matchedConfigured.slug : trimmed; for (const entry of models) { if (!entry || typeof entry !== "object") continue; // Provider-level per-row stamps win for their routed rows; the root selector @@ -1602,9 +1615,10 @@ export function applyAutoReviewModelOverride( const current = entry.auto_review_model_override; if (typeof current === "string" && isValidAutoReviewModel(current)) continue; } - entry.auto_review_model_override = trimmed; + entry.auto_review_model_override = canonicalRoot; + entry.opencodex_auto_review_root = canonicalRoot; } - lastAppliedRootAutoReviewModel = trimmed; + lastAppliedRootAutoReviewModel = canonicalRoot; return "applied"; } diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index ac9c3cc4c7..4c7ea774a0 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1063,11 +1063,11 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { + resetCatalogRuntimeStateForTests(); + try { + // Rows as they would exist on disk after a previous root stamp and a process restart: + // only the per-row marker (not the module variable) identifies the stale native copy. + const entries: Array> = [ + { + slug: "gpt-5.5", + auto_review_model_override: "opencode-go/deepseek-v4-pro", + opencodex_auto_review_root: "opencode-go/deepseek-v4-pro", + }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + { slug: "blsc/glm-5.2", auto_review_model_override: "blsc/glm-5.2" }, + ]; + const removed = applyAutoReviewModelOverride(entries, null, [], { retainRoutedOverrides: true }); + expect(removed).toBe("absent"); + expect(entries[0].auto_review_model_override).toBeNull(); + expect(entries[0].opencodex_auto_review_root).toBeUndefined(); + expect(entries[2].auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(entries[3].auto_review_model_override).toBe("blsc/glm-5.2"); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); + + test("raw-vs-encoded equivalent root selector is stamped with the matched catalog slug and survives final validation", () => { + resetCatalogRuntimeStateForTests(); + try { + const entries: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + ]; + const applied = applyAutoReviewModelOverride(entries, "opencode-go/deepseek/v4-pro", [], { retainRoutedOverrides: true }); + expect(applied).toBe("applied"); + expect(entries[0].auto_review_model_override).toBe("opencode-go/deepseek-v4-pro"); + validateAutoReviewOverridesAgainstCatalog(entries); + expect(entries[0].auto_review_model_override).toBe("opencode-go/deepseek-v4-pro"); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); }); diff --git a/tests/codex-integration/codex-catalog.test.ts b/tests/codex-integration/codex-catalog.test.ts index c5865c0cad..971ab63e6c 100644 --- a/tests/codex-integration/codex-catalog.test.ts +++ b/tests/codex-integration/codex-catalog.test.ts @@ -3796,6 +3796,34 @@ describe("Codex catalog routed normalization", () => { } }); + test("gatherRoutedModels resolves a bare auto-review target to a retain-only model", async () => { + resetCatalogRuntimeStateForTests(); + try { + const base = { + port: 10100, + defaultProvider: "openai", + providers: { + openai: { + adapter: "openai-responses", + baseUrl: "https://example.invalid/v1", + authMode: "key", + liveModels: false, + models: ["worker"], + retainModels: ["reviewer"], + autoReviewModelOverrides: { worker: "reviewer" }, + }, + }, + } as Parameters[0]; + const models = await gatherRoutedModelsDirect(base); + const reviewer = models.find(model => model.provider === "openai" && model.id === "reviewer"); + expect(reviewer).toBeDefined(); + const worker = models.find(model => model.provider === "openai" && model.id === "worker"); + expect(worker?.autoReviewModelOverride).toBe("openai/reviewer"); + } finally { + resetCatalogRuntimeStateForTests(); + } + }); + test("Daybreak metadata inheritance rejects noncanonical providers", async () => { const models = await gatherRoutedModels({ port: 10100, diff --git a/tests/providers/provider-config-batch-management.test.ts b/tests/providers/provider-config-batch-management.test.ts index e992442ce7..ee0004c8fd 100644 --- a/tests/providers/provider-config-batch-management.test.ts +++ b/tests/providers/provider-config-batch-management.test.ts @@ -391,4 +391,34 @@ describe("atomic provider editor batch", () => { error: "Full config PUT is disabled. Use /api/providers POST for provider changes.", }); }); + + test("provider POST does not overwrite auto-review values changed during DNS validation", async () => { + const liveConfig = seededConfig(); + liveConfig.providers.alpha.autoReviewModel = "old-reviewer"; + saveConfig(liveConfig); + + const destinationSpy = spyOn(destinationPolicy, "providerDestinationResolvedError").mockImplementation(async () => { + liveConfig.providers.alpha.autoReviewModel = "concurrent-reviewer"; + return null; + }); + let response: Response | null; + try { + const request = new Request("http://127.0.0.1/api/providers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + name: "alpha", + provider: { adapter: "openai-chat", baseUrl: "https://alpha.example.test/v1", defaultModel: "alpha-old" }, + }), + }); + response = await handleManagementAPI(request, new URL(request.url), liveConfig, { + createManagementConvergeCodex: catalogConvergenceFactory(() => {}), + }); + } finally { + destinationSpy.mockRestore(); + } + expect(response?.status).toBe(200); + expect(liveConfig.providers.alpha.autoReviewModel).toBe("concurrent-reviewer"); + expect(loadConfig().providers.alpha.autoReviewModel).toBe("concurrent-reviewer"); + }); }); From 6a1a6a91c445a5867d0893ced8b063326443051c Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Thu, 3 Sep 2026 17:51:33 +0800 Subject: [PATCH 3/7] fix(catalog): re-read live provider row after DNS await; cover durable marker via finalize path --- src/server/management/provider-routes.ts | 57 +++++++++-------- tests/auto-review-model-override.test.ts | 62 ++++++++++++++++++- .../provider-config-batch-management.test.ts | 10 ++- 3 files changed, 100 insertions(+), 29 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 4c7ea774a0..cad11f4cbe 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1004,70 +1004,73 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise serialize -> regenerate -> remove through the finalize path", () => { + resetCatalogRuntimeStateForTests(); + const originalCodexHome = process.env.CODEX_HOME; + const tempHome = mkdtempSync(join(tmpdir(), "ocx-auto-review-home-")); + try { + const configPath = join(tempHome, "config.toml"); + writeFileSync(configPath, 'auto_review_model = "opencode-go/deepseek-v4-pro"\n'); + process.env.CODEX_HOME = tempHome; + + // Phase 1: a regenerate runs while the root selector is configured. The routed row + // carries its provider-derived stamp; native and unstamped routed rows receive the + // root selector with the durable per-row marker. + const onDiskBeforeRoot: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + ]; + const freshWithRoot: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + ]; + expect(finalizeAutoReviewModelOverride(freshWithRoot, onDiskBeforeRoot)).toBe("applied"); + expect(freshWithRoot[0]!.auto_review_model_override).toBe("opencode-go/deepseek-v4-pro"); + expect(freshWithRoot[0]!.opencodex_auto_review_root).toBe("opencode-go/deepseek-v4-pro"); + expect(freshWithRoot[1]!.auto_review_model_override).toBe("opencode-go/deepseek-v4-pro"); + expect(freshWithRoot[1]!.opencodex_auto_review_root).toBe("opencode-go/deepseek-v4-pro"); + expect(freshWithRoot[2]!.auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(freshWithRoot[2]!.opencodex_auto_review_root).toBeUndefined(); + + // The stamped rows are what a retained sync would serialize to disk before restart. + const onDiskAfterRoot = structuredClone(freshWithRoot) as Array>; + + // Phase 2: the operator removes the root selector and the process restarts, so the + // module-level selector memory is gone; only the durable marker identifies stale + // native copies when rows are preserved through the next regenerate. + writeFileSync(configPath, "# no auto_review_model configured\n"); + resetCatalogRuntimeStateForTests(); + const freshAfterRemoval: Array> = [ + { slug: "gpt-5.5", auto_review_model_override: null }, + { slug: "opencode-go/deepseek-v4-pro", auto_review_model_override: null }, + { slug: "deepseek/deepseek-v4-flash", auto_review_model_override: "deepseek/deepseek-v4-flash" }, + ]; + expect(finalizeAutoReviewModelOverride(freshAfterRemoval, onDiskAfterRoot)).toBe("absent"); + expect(freshAfterRemoval[0]!.auto_review_model_override).toBeNull(); + expect(freshAfterRemoval[0]!.opencodex_auto_review_root).toBeUndefined(); + expect(freshAfterRemoval[1]!.auto_review_model_override).toBeNull(); + expect(freshAfterRemoval[1]!.opencodex_auto_review_root).toBeUndefined(); + expect(freshAfterRemoval[2]!.auto_review_model_override).toBe("deepseek/deepseek-v4-flash"); + expect(freshAfterRemoval[2]!.opencodex_auto_review_root).toBeUndefined(); + validateAutoReviewOverridesAgainstCatalog(freshAfterRemoval); + } finally { + if (originalCodexHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = originalCodexHome; + rmSync(tempHome, { recursive: true, force: true }); + resetCatalogRuntimeStateForTests(); + } + }); + test("raw-vs-encoded equivalent root selector is stamped with the matched catalog slug and survives final validation", () => { resetCatalogRuntimeStateForTests(); try { diff --git a/tests/providers/provider-config-batch-management.test.ts b/tests/providers/provider-config-batch-management.test.ts index ee0004c8fd..5ebeeb9442 100644 --- a/tests/providers/provider-config-batch-management.test.ts +++ b/tests/providers/provider-config-batch-management.test.ts @@ -398,7 +398,13 @@ describe("atomic provider editor batch", () => { saveConfig(liveConfig); const destinationSpy = spyOn(destinationPolicy, "providerDestinationResolvedError").mockImplementation(async () => { - liveConfig.providers.alpha.autoReviewModel = "concurrent-reviewer"; + // A concurrent PATCH replaces the provider row object rather than mutating it in + // place, so the pre-await snapshot used for carry-over must not be the write source. + liveConfig.providers.alpha = { + ...liveConfig.providers.alpha, + autoReviewModel: "concurrent-reviewer", + modelDisplayNames: { "alpha-old": "Concurrent label" }, + }; return null; }); let response: Response | null; @@ -420,5 +426,7 @@ describe("atomic provider editor batch", () => { expect(response?.status).toBe(200); expect(liveConfig.providers.alpha.autoReviewModel).toBe("concurrent-reviewer"); expect(loadConfig().providers.alpha.autoReviewModel).toBe("concurrent-reviewer"); + expect(liveConfig.providers.alpha.modelDisplayNames?.["alpha-old"]).toBe("Concurrent label"); + expect(loadConfig().providers.alpha.modelDisplayNames?.["alpha-old"]).toBe("Concurrent label"); }); }); From a9efbaa9abb28b5690ab78f82c1fb941be03f236 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Thu, 3 Sep 2026 20:14:34 +0800 Subject: [PATCH 4/7] fix(catalog): do not restore prohibited auto-review fields on canonical openai POST --- src/server/management/provider-routes.ts | 7 ++- .../management-provider-validation.test.ts | 48 +++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index cad11f4cbe..f936a27ade 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1066,10 +1066,13 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { } }); + test("an unrelated canonical openai POST clears legacy auto-review fields", 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); + // A legacy row that predates the prohibition can still be carried by a live + // process that started before the validation landed; an unrelated full edit + // must not resurrect either field into the persisted document. + const liveConfig = { + port: 0, + openaiProviderTierVersion: 2, + defaultProvider: "openai", + providers: { + openai: { + ...canonicalDirect, + autoReviewModel: "deepseek-v4-flash", + autoReviewModelOverrides: { "deepseek-v4-flash-vision-exp": "deepseek-v4-pro" }, + }, + }, + } as OcxConfig; + const resolvedError = spyOn(destinationPolicy, "providerDestinationResolvedError").mockResolvedValue(null); + try { + const request = new Request("http://127.0.0.1/api/providers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ name: "openai", provider: canonicalDirect }), + }); + const response = await handleManagementAPI(request, new URL(request.url), liveConfig, { + createManagementConvergeCodex: catalogConvergenceFactory(), + }); + expect(response?.status).toBe(200); + const persisted = JSON.parse(readFileSync(join(TEST_DIR, "config.json"), "utf8")) as { + providers?: { openai?: Record }; + }; + expect(persisted.providers?.openai?.autoReviewModel).toBeUndefined(); + expect(persisted.providers?.openai?.autoReviewModelOverrides).toBeUndefined(); + expect(liveConfig.providers.openai?.autoReviewModel).toBeUndefined(); + expect(liveConfig.providers.openai?.autoReviewModelOverrides).toBeUndefined(); + } finally { + resolvedError.mockRestore(); + } + }); + test("general provider writes cannot introduce a provider alias collision", async () => { if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); From 55597d0cbc48bd79acca9a65b5d2e0760a4ac8a2 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Fri, 4 Sep 2026 00:44:26 +0800 Subject: [PATCH 5/7] test(providers): use removeTreeWithRetry where rmSync was not imported --- tests/server/management-provider-validation.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/server/management-provider-validation.test.ts b/tests/server/management-provider-validation.test.ts index b79c2e63b4..67d43aa9af 100644 --- a/tests/server/management-provider-validation.test.ts +++ b/tests/server/management-provider-validation.test.ts @@ -1035,7 +1035,7 @@ describe("provider management validation", () => { }); test("provider POST normalizes auto-review fields before persisting config.json", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig(config("127.0.0.1")); @@ -1079,7 +1079,7 @@ describe("provider management validation", () => { }); test("provider GET redacts credential-shaped auto-review values", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig(config("127.0.0.1")); @@ -1120,7 +1120,7 @@ describe("provider management validation", () => { test("GET /api/config redacts credential-shaped auto-review values", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig(config("127.0.0.1")); @@ -1164,7 +1164,7 @@ describe("provider management validation", () => { test("provider POST redacts token-shaped names in auto-review errors", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig(config("127.0.0.1")); @@ -1194,7 +1194,7 @@ describe("provider management validation", () => { }); test("canonical openai rejects auto-review fields without persisting", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig({ @@ -1663,7 +1663,7 @@ describe("provider management validation", () => { }); test("provider POST overwrite preserves auto-review fields when the payload omits them", async () => { - if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + if (existsSync(TEST_DIR)) removeTreeWithRetry(TEST_DIR); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; saveConfig(config("127.0.0.1")); From 8bb58ce24571f5ea6a63b6eacf7612da47e565ee Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 5 Sep 2026 20:30:25 +0800 Subject: [PATCH 6/7] fix(catalog): canonicalize override keys, merge registry defaults, sanitize openai load fields --- src/config/provider-validation.ts | 24 ++++- src/providers/derive.ts | 7 +- src/server/management/provider-routes.ts | 5 +- tests/auto-review-model-override.test.ts | 117 ++++++++++++++++++++++- 4 files changed, 144 insertions(+), 9 deletions(-) diff --git a/src/config/provider-validation.ts b/src/config/provider-validation.ts index 900ed61264..9f5ff85ba2 100644 --- a/src/config/provider-validation.ts +++ b/src/config/provider-validation.ts @@ -171,15 +171,18 @@ export function normalizeAutoReviewModelOverridesField(value: unknown): return { error: "autoReviewModelOverrides must be an object mapping model ids to approval model ids, or null to clear" }; } const cleaned: Record = {}; + const canonicalSeen = new Set(); for (const [key, entry] of Object.entries(value as Record)) { const trimmedKey = key.trim(); const trimmedEntry = typeof entry === "string" ? entry.trim() : ""; if (trimmedKey === "" || /\s/.test(trimmedKey) || trimmedEntry === "" || /\s/.test(trimmedEntry)) { return { error: "autoReviewModelOverrides entries must be nonblank model ids without whitespace" }; } - if (Object.hasOwn(cleaned, trimmedKey)) { - return { error: "autoReviewModelOverrides keys must be unique after trimming" }; + const canonicalKey = trimmedKey.toLowerCase(); + if (canonicalSeen.has(canonicalKey)) { + return { error: "autoReviewModelOverrides keys must be unique after trimming and case folding" }; } + canonicalSeen.add(canonicalKey); cleaned[trimmedKey] = trimmedEntry; } return { value: cleaned }; @@ -217,9 +220,14 @@ export function sanitizeAutoReviewOverridesForLoad(parsed: unknown): void { const root = parsed as Record; const providers = root.providers; if (!providers || typeof providers !== "object" || Array.isArray(providers)) return; - for (const provider of Object.values(providers as Record)) { + for (const [name, provider] of Object.entries(providers as Record)) { if (!provider || typeof provider !== "object" || Array.isArray(provider)) continue; const row = provider as Record; + if (name === "openai") { + delete row.autoReviewModel; + delete row.autoReviewModelOverrides; + continue; + } if (row.autoReviewModel !== undefined) { const value = typeof row.autoReviewModel === "string" ? row.autoReviewModel.trim() : ""; row.autoReviewModel = value !== "" && !/\s/.test(value) ? value : undefined; @@ -231,14 +239,22 @@ export function sanitizeAutoReviewOverridesForLoad(parsed: unknown): void { continue; } const cleaned: Record = {}; + const canonicalSeen = new Set(); + let collision = false; for (const [key, value] of Object.entries(overrides as Record)) { const trimmedKey = key.trim(); if (trimmedKey === "" || /\s/.test(trimmedKey)) continue; const trimmedValue = typeof value === "string" ? value.trim() : ""; if (trimmedValue === "" || /\s/.test(trimmedValue)) continue; + const canonicalKey = trimmedKey.toLowerCase(); + if (canonicalSeen.has(canonicalKey)) { + collision = true; + break; + } + canonicalSeen.add(canonicalKey); cleaned[trimmedKey] = trimmedValue; } - row.autoReviewModelOverrides = Object.keys(cleaned).length > 0 ? cleaned : undefined; + row.autoReviewModelOverrides = collision || Object.keys(cleaned).length === 0 ? undefined : cleaned; } } } diff --git a/src/providers/derive.ts b/src/providers/derive.ts index ac4578b92e..aea49c1d9e 100644 --- a/src/providers/derive.ts +++ b/src/providers/derive.ts @@ -531,8 +531,11 @@ export function enrichProviderFromRegistry(name: string, prov: OcxProviderConfig if (prov.autoReviewModel === undefined && seed.autoReviewModel !== undefined) { prov.autoReviewModel = seed.autoReviewModel; } - if (prov.autoReviewModelOverrides === undefined && seed.autoReviewModelOverrides !== undefined) { - prov.autoReviewModelOverrides = { ...seed.autoReviewModelOverrides }; + if (seed.autoReviewModelOverrides !== undefined || prov.autoReviewModelOverrides !== undefined) { + prov.autoReviewModelOverrides = { + ...(seed.autoReviewModelOverrides ?? {}), + ...(prov.autoReviewModelOverrides ?? {}), + }; } // Registry-only metadata (never seeded into saved config): backfill straight from // the entry so an explicit user value stays distinguishable from the default. diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index f936a27ade..f22ae35914 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -1077,7 +1077,10 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { expect(loaded.providers.test?.autoReviewModel).toBeUndefined(); expect(loaded.providers.test?.autoReviewModelOverrides).toBeUndefined(); }); + + test("load sanitization removes override maps with canonical collisions and keeps unrelated providers", () => { + writeFileSync(join(testRoot, "config.json"), JSON.stringify({ + port: 10100, + defaultProvider: "caseCollider", + providers: { + caseCollider: { + adapter: "openai-chat", + baseUrl: "https://case.example.test/v1", + apiKey: "sk-test", + autoReviewModelOverrides: { + "DEEPSEEK-V4-FLASH": "deepseek-v4-pro", + "deepseek-v4-flash": "deepseek-v4-flash", + }, + }, + trimCollider: { + adapter: "openai-chat", + baseUrl: "https://trim.example.test/v1", + apiKey: "sk-test", + autoReviewModelOverrides: { + " model-a ": "deepseek-v4-pro", + "model-a": "deepseek-v4-flash", + }, + }, + unrelated: { + adapter: "openai-chat", + baseUrl: "https://unrelated.example.test/v1", + apiKey: "sk-test", + autoReviewModel: "deepseek-v4-flash", + }, + }, + })); + const loaded = loadConfig(); + expect(loaded.providers.caseCollider?.autoReviewModelOverrides).toBeUndefined(); + expect(loaded.providers.trimCollider?.autoReviewModelOverrides).toBeUndefined(); + expect(loaded.providers.unrelated?.autoReviewModel).toBe("deepseek-v4-flash"); + expect(loaded.providers.caseCollider).toBeDefined(); + expect(loaded.providers.trimCollider).toBeDefined(); + }); + + test("load sanitizer strips prohibited auto-review fields from the openai provider", () => { + const parsed = { + providers: { + openai: { + adapter: "openai-responses", + baseUrl: "https://api.openai.com/v1", + autoReviewModel: "deepseek-v4-flash", + autoReviewModelOverrides: { "gpt-5.4": "gpt-5.4-nano" }, + }, + unrelated: { + adapter: "openai-chat", + baseUrl: "https://example.test/v1", + autoReviewModel: "deepseek-v4-flash", + }, + }, + }; + sanitizeAutoReviewOverridesForLoad(parsed); + expect(parsed.providers.openai.autoReviewModel).toBeUndefined(); + expect(parsed.providers.openai.autoReviewModelOverrides).toBeUndefined(); + expect(parsed.providers.unrelated.autoReviewModel).toBe("deepseek-v4-flash"); + }); }); describe("management validation and normalization", () => { @@ -351,6 +416,26 @@ describe("management validation and normalization", () => { const provider = { autoReviewModel: "deep seek-v4-flash" }; expect(normalizeAutoReviewModelFields("custom", provider)).toContain("autoReviewModel"); }); + + test("POST normalization rejects case-folded duplicate override keys", () => { + const provider = { + autoReviewModelOverrides: { + "DEEPSEEK-V4-FLASH": "deepseek-v4-pro", + "deepseek-v4-flash": "deepseek-v4-flash", + }, + }; + expect(normalizeAutoReviewModelFields("custom", provider)).toContain("unique after trimming and case folding"); + }); + + test("POST normalization rejects whitespace-colliding duplicate override keys", () => { + const provider = { + autoReviewModelOverrides: { + " model-a ": "deepseek-v4-pro", + "model-a": "deepseek-v4-flash", + }, + }; + expect(normalizeAutoReviewModelFields("custom", provider)).toContain("unique after trimming and case folding"); + }); }); describe("routed provider merge", () => { @@ -395,6 +480,34 @@ describe("routed provider merge", () => { else entry!.autoReviewModel = saved; } }); + + test("enrichment merges registry and provider override maps per key with provider winning", () => { + const entry = PROVIDER_REGISTRY.find(e => e.id === "deepseek"); + expect(entry).toBeDefined(); + const saved = entry!.autoReviewModelOverrides; + entry!.autoReviewModelOverrides = { + registryOnly: "deepseek/deepseek-v4-pro", + shared: "registry-default", + }; + try { + const provider = providerConfigSeed(entry!); + // Simulate a hand-edited map that replaced the seeded defaults wholesale + // before enrichment, which previously hid the disjoint registry entries. + provider.autoReviewModelOverrides = { + providerOnly: "deepseek/deepseek-v4-flash", + shared: "provider-override", + }; + enrichProviderFromRegistry("deepseek", provider); + expect(provider.autoReviewModelOverrides).toEqual({ + registryOnly: "deepseek/deepseek-v4-pro", + providerOnly: "deepseek/deepseek-v4-flash", + shared: "provider-override", + }); + } finally { + if (saved === undefined) delete entry!.autoReviewModelOverrides; + else entry!.autoReviewModelOverrides = saved; + } + }); }); describe("global auto_review_model precedence (provider stamp vs root selector)", () => { From a0f35833d21423d34c31de9ba882f80db52baad3 Mon Sep 17 00:00:00 2001 From: HarryZhou <2373256746@qq.com> Date: Sat, 5 Sep 2026 20:53:15 +0800 Subject: [PATCH 7/7] fix(catalog): preserve provider precedence across case-variant override keys --- src/providers/derive.ts | 34 +++++++++++++++-- src/router.ts | 14 ++++--- tests/auto-review-model-override.test.ts | 48 ++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/providers/derive.ts b/src/providers/derive.ts index aea49c1d9e..3d19e90fcf 100644 --- a/src/providers/derive.ts +++ b/src/providers/derive.ts @@ -453,6 +453,32 @@ export function hasLegacyClinePassReasoningEfforts(name: string, prov: OcxProvid && prov.reasoningEfforts[0] === "low"; } +/** + * Merge auto-review override maps with provider precedence that is case-insensitive. + * + * The per-model resolver case-folds lookup keys, so an inherited registry entry such as + * "DEEPSEEK-V4-FLASH" would otherwise stay beside a provider override "deepseek-v4-flash" + * and win (or lose) depending on the routed model's casing. A provider entry replaces any + * inherited entry whose key matches after case folding, while disjoint entries are retained. + */ +export function mergeAutoReviewModelOverrides( + inherited: Record | undefined, + provider: Record | undefined, +): Record | undefined { + if (inherited === undefined && provider === undefined) return undefined; + const merged: Record = { ...(inherited ?? {}) }; + const foldedToKey = new Map(); + for (const key of Object.keys(merged)) foldedToKey.set(key.toLowerCase(), key); + for (const [key, value] of Object.entries(provider ?? {})) { + const folded = key.toLowerCase(); + const inheritedKey = foldedToKey.get(folded); + if (inheritedKey !== undefined && inheritedKey !== key) delete merged[inheritedKey]; + merged[key] = value; + foldedToKey.set(folded, key); + } + return merged; +} + export function enrichProviderFromRegistry(name: string, prov: OcxProviderConfig): void { const entry = PROVIDER_REGISTRY.find(row => row.id === name); if (!entry || !providerMatchesRegistryTransportWithStaticGuards(name, prov)) { @@ -532,10 +558,10 @@ export function enrichProviderFromRegistry(name: string, prov: OcxProviderConfig prov.autoReviewModel = seed.autoReviewModel; } if (seed.autoReviewModelOverrides !== undefined || prov.autoReviewModelOverrides !== undefined) { - prov.autoReviewModelOverrides = { - ...(seed.autoReviewModelOverrides ?? {}), - ...(prov.autoReviewModelOverrides ?? {}), - }; + prov.autoReviewModelOverrides = mergeAutoReviewModelOverrides( + seed.autoReviewModelOverrides, + prov.autoReviewModelOverrides, + ); } // Registry-only metadata (never seeded into saved config): backfill straight from // the entry so an explicit user value stays distinguishable from the default. diff --git a/src/router.ts b/src/router.ts index 7173db4225..9cdf91ea4c 100644 --- a/src/router.ts +++ b/src/router.ts @@ -18,7 +18,11 @@ import { providerCodexAccountMode, registryModelServiceTierCapabilityApplies, } from "./providers/registry"; -import { applyDirectReasoningEffortContracts, hasLegacyClinePassReasoningEfforts } from "./providers/derive"; +import { + applyDirectReasoningEffortContracts, + hasLegacyClinePassReasoningEfforts, + mergeAutoReviewModelOverrides, +} from "./providers/derive"; import { cloneFastWire } from "./providers/fastwire"; import { providerMatchesRegistryTransportWithStaticGuards, @@ -387,10 +391,10 @@ export function routedProviderConfig(providerName: string, provider: OcxProvider : {}), ...(provider.autoReviewModelOverrides !== undefined || registryEntry.autoReviewModelOverrides !== undefined ? { - autoReviewModelOverrides: { - ...(registryEntry.autoReviewModelOverrides ?? {}), - ...(provider.autoReviewModelOverrides ?? {}), - }, + autoReviewModelOverrides: mergeAutoReviewModelOverrides( + registryEntry.autoReviewModelOverrides, + provider.autoReviewModelOverrides, + ), } : {}), ...(provider.fastWire === undefined && registryEntry.fastWire !== undefined diff --git a/tests/auto-review-model-override.test.ts b/tests/auto-review-model-override.test.ts index 6b15f277d8..59465e0c06 100644 --- a/tests/auto-review-model-override.test.ts +++ b/tests/auto-review-model-override.test.ts @@ -508,6 +508,54 @@ describe("routed provider merge", () => { else entry!.autoReviewModelOverrides = saved; } }); + + test("routed merge replaces a case-variant registry override with the provider value", () => { + const entry = PROVIDER_REGISTRY.find(e => e.id === "deepseek"); + expect(entry).toBeDefined(); + const saved = entry!.autoReviewModelOverrides; + entry!.autoReviewModelOverrides = { + "DEEPSEEK-V4-FLASH": "deepseek/deepseek-v4-pro", + "vision-exp": "deepseek/deepseek-v4-flash", + }; + try { + const provider = providerConfigSeed(entry!); + provider.autoReviewModelOverrides = { + "deepseek-v4-flash": "deepseek/deepseek-v4-flash", + }; + const routed = routedProviderConfig("deepseek", provider); + expect(routed.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash": "deepseek/deepseek-v4-flash", + "vision-exp": "deepseek/deepseek-v4-flash", + }); + } finally { + if (saved === undefined) delete entry!.autoReviewModelOverrides; + else entry!.autoReviewModelOverrides = saved; + } + }); + + test("enrichment replaces a case-variant registry override with the provider value", () => { + const entry = PROVIDER_REGISTRY.find(e => e.id === "deepseek"); + expect(entry).toBeDefined(); + const saved = entry!.autoReviewModelOverrides; + entry!.autoReviewModelOverrides = { + "DEEPSEEK-V4-FLASH": "deepseek/deepseek-v4-pro", + "vision-exp": "deepseek/deepseek-v4-flash", + }; + try { + const provider = providerConfigSeed(entry!); + provider.autoReviewModelOverrides = { + "deepseek-v4-flash": "deepseek/deepseek-v4-flash", + }; + enrichProviderFromRegistry("deepseek", provider); + expect(provider.autoReviewModelOverrides).toEqual({ + "deepseek-v4-flash": "deepseek/deepseek-v4-flash", + "vision-exp": "deepseek/deepseek-v4-flash", + }); + } finally { + if (saved === undefined) delete entry!.autoReviewModelOverrides; + else entry!.autoReviewModelOverrides = saved; + } + }); }); describe("global auto_review_model precedence (provider stamp vs root selector)", () => {