diff --git a/devlog/_plan/260902_nonbug_adoption_backlog/040_wp4_retain_models.md b/devlog/_plan/260902_nonbug_adoption_backlog/040_wp4_retain_models.md new file mode 100644 index 0000000000..23983de803 --- /dev/null +++ b/devlog/_plan/260902_nonbug_adoption_backlog/040_wp4_retain_models.md @@ -0,0 +1,111 @@ +# wp4 — #1690 retainModels allowlist (two rival PRs) + +Issue #1690 (score 58, labels enhancement/catalog). Two open drafts implement it: + +| | PR #2860 (rrmlima) | PR #2122 (chilung-cgu) | +| --- | --- | --- | +| size | +132/-2, 3 files | +726/-25, 15 files | +| CI at head | fully green (test 1-4, macos, ci) | only hygiene/label/target ran | +| base | e546c160b, 310 behind dev; cherry-picks cleanly onto `fcf0da257` | same base | +| retention | `shouldRetainConfiguredProviderModel(name, id, prov)` + `modelInList` | new `providerRetainModels` set inside the merge loop | +| ids must also be in `models`? | yes (purely retentive) | no (`retainModels` folded into `configuredIds`) | +| config validation | none (relies on `.passthrough()`) | zod `retainModels` + `nonBlankStringArrayConfigError` | +| management API / DTO | none | none (neither exposes it via PATCH) | +| rename migration | none | adds `retainModels` to `MODEL_ID_LISTS` | +| 404 diagnostic | none | new module state + `warnRetainedModel404Once` wired into 4 request handlers, `CatalogModel.retainedWithoutDiscovery` | +| docs | none | 5 locales, one table row each | + +## Decision + +Adopt **#2860 as the base commit** (cherry-picked as `2be9d505d` on +`codex/retain-models-1690`), then add the pieces that make the opt-in +discoverable and safe to hand-edit. #2122 is closed as superseded with credit for +the config/migration design. + +Why #2860 over #2122: the retention decision belongs in the one predicate the +merge loop already consults; #2122 adds a second set beside it. #2122's 404 +diagnostic requires module-level maps keyed by provider, a new `CatalogModel` +field, and edits to `core.ts`, `chat-completions.ts`, `chat-native.ts`, +`claude-messages.ts` — four hot request paths — to print one warning that the +upstream error body already carries (`model_not_found`). That is the wrong +trade for this cycle; the existing `warnDroppedConfiguredIdsOnce` stays as the +diagnostic for ids that are *not* retained. + +## What this cycle adds on top of #2860 + +1. **`retainModels` alone is enough.** `configuredIds` in + `fetchProviderModelsWithAuth` becomes the ordered union of the Vertex seed, + `prov.models`, and `prov.retainModels`. An operator who writes + `"retainModels": ["gemini-3.7-flash"]` should not have to repeat the id in + `models`; requiring both is the footgun #2122 correctly avoided. #2860's + "does not invent ids" test flips to assert the union. +2. **Schema + load normalization** (`src/config.ts`): `retainModels: + z.array(z.string().min(1)).transform(normalizeNonBlankStringArray).optional()` + next to `noStructuredOutputModels`, plus the same `superRefine` entry so a + hand-edited `"retainModels": "x"` fails with a path instead of being silently + passed through. +3. **Management PATCH + DTO** (`src/server/management/provider-routes.ts`): + `retainModels` accepted like `noStructuredOutputModels` (`null` clears, + empty array clears, validated with `nonBlankStringArrayConfigError`), and + returned in the safe provider DTO so the dashboard/API round-trips it. +4. **CLI opt-in** (`src/cli/provider-runtime.ts`): + `ocx provider edit --retain-models `. This is the easy + switch: one flag, no JSON editing, `-` clears. Usage string and + `skills/ocx` surface are regenerated if the capability registry changes + (it does not — `provider edit` already exists; only the flag list grows). +5. **Rename migration** (`src/providers/model-rename-migration.ts`): + `"retainModels"` added to `MODEL_ID_LISTS` so a retired id is renamed + rather than resurrected as a ghost row. +6. **Docs** (`docs-site/.../reference/configuration/providers.md`): one table + row after `selectedModels`, and a short paragraph in "Static model + allowlists" contrasting `selectedModels` (narrows) with `retainModels` + (preserves). English only this cycle; locales already lag on + `noStructuredOutputModels` and a missing row does not contradict. + +## Explicitly not in this cycle + +- Seeding `CALLABLE_CONFIGURED_COMPATIBILITY_MODELS` with antigravity + `gemini-3.7-flash` (issue step 4). That is a product default, and #1683 is a + separate issue; with the config key available it no longer needs a release. +- GUI field. The dashboard provider editor is untouched; the PATCH contract is + ready for it, and a later PR can add the input with its screenshot. +- 404-time warning. See "Decision". + +## Acceptance criteria + +- `retainModels` absent/empty → catalog identical to today (existing + `tests/codex-catalog.test.ts` retention tests untouched and green). +- `retainModels: ["x"]`, live omits `x`, `x` not in `models` → `x` present + with provider hints applied; `droppedConfiguredIds` excludes it. +- `retainModels: ["x"]`, live returns `x` → single row, no duplicate. +- `liveModels: false` → `retainModels` ids are part of the static list. +- Config load rejects `retainModels: "x"` / `[""]` with a + `providers..retainModels` path; trims and dedupes valid input. +- Management PATCH sets/clears; DTO echoes; CLI flag round-trips through PATCH. +- A test through `fetchProviderModels` (not only `mergeConfiguredModelsIntoLiveCatalog`) proves a retain-only id survives both live discovery and `liveModels: false` (audit r1 blocker 2). +- CLI treats `-` before `csv` so `--retain-models -` clears (audit r1 blocker 3). +- `providerCatalogFingerprint` includes `retainModels`. +- Migration renames a retired id inside `retainModels`. +- `bun x tsc --noEmit` clean, `bun run privacy:scan` clean, focused files: + `tests/catalog-retain-models.test.ts`, `tests/codex-catalog.test.ts`, + `tests/management-provider-validation.test.ts`, + `tests/model-rename-migration.test.ts` (if present), provider-runtime CLI test. + +## Files + +- `src/codex/catalog/provider-fetch.ts` — configuredIds union (on top of #2860). +- `src/config.ts` — schema + superRefine. +- `src/server/management/provider-routes.ts` — PATCH + DTO. +- `src/server/auth-cors.ts` — `providerManagementConfigError` validation + safe-config DTO key list (audit r1 blocker 1). +- `src/cli/provider-runtime.ts` — `--retain-models`. +- `src/providers/model-rename-migration.ts` — list entry. +- `src/types/provider.ts` — already added by #2860 (doc comment adjusted for union). +- `docs-site/src/content/docs/reference/configuration/providers.md`. +- `tests/catalog-retain-models.test.ts` (extend), `tests/management-provider-validation.test.ts` (extend). + +## Closure + +PR targets `dev`, `Closes #1690`, description names #2860 as the carried +source commit (`12e69c200`) and #2122 as design input. After landing: close +#1690 with the landing SHA, close #2860 as landed-via-carry (author credited in +the squash trailer), close #2122 as superseded with the reasoning above. diff --git a/devlog/_plan/260902_nonbug_adoption_backlog/041_wp4_audit_r1_synthesis.md b/devlog/_plan/260902_nonbug_adoption_backlog/041_wp4_audit_r1_synthesis.md new file mode 100644 index 0000000000..c48bfd145b --- /dev/null +++ b/devlog/_plan/260902_nonbug_adoption_backlog/041_wp4_audit_r1_synthesis.md @@ -0,0 +1,30 @@ +# wp4 audit r1 — synthesis + +Reviewer: grok-4.6 subagent (Feynman), read-only, 7 questions on 040. Verdict: **near-pass / GO-WITH-FIXES** (3 blockers, 3 suggestions). + +## Answers that confirm the plan + +- Union (Q1): safe only if written as an ordered dedupe set `[vertexDefault?, ...models, ...retainModels]`, replacing the current `seed ? [default] : models` ternary. `configured` is the single seed for static `liveModels:false`, the Cursor filter, the degraded fallback, `droppedConfiguredIds`, and hints, so a retain-only id gets the same context/effort maps as a `models[]` entry. The Vertex seed predicate (`models.length === 0`) stays untouched. +- Family match (Q2): acceptable, same semantics as `noVisionModels`. `retainModels: ["gpt-oss"]` keeps `gpt-oss:120b` and also invents a bare `gpt-oss` row through the union — extra row, never a drop. +- selectedModels precedence (Q3): `filterCatalogVisibleModels` and `sync.ts` still hide a retained id when `selectedModels` is non-empty and omits it. Keep that; document "retain ≠ visible". +- PATCH (Q4): copy `provider-routes.ts:386` verbatim; also GET list (:540). +- CLI (Q5): no `takeListOption`; use `csv(takeOption)` and special-case `"-"` **before** `csv` (`csv("-")` yields `["-"]`). +- #2122 (Q6): union, schema, `MODEL_ID_LISTS` are the necessary parts; the 404 module maps and `retainedWithoutDiscovery` are not. The `withConfiguredRetention(live→forCache)` change is incidental — the double merge is the OCX-111 combo-cache contract. Do not copy. +- No-regression (Q7): absent/empty is a no-op; kimi/xai tables unchanged. + +## Blockers folded into 040 + +1. `src/server/auth-cors.ts` was missing from the file list: `providerManagementConfigError` (~693) must validate `retainModels` with `nonBlankStringArrayConfigError`, and the safe-config DTO key list (~794) must include it, otherwise PATCH validation and DTO echo silently miss. +2. The "liveModels:false / retain-only present" criterion cannot be proven through `mergeConfiguredModelsIntoLiveCatalog` alone. Add a test that goes through `fetchProviderModels`/the gather path so the union at :1307 is actually exercised. +3. CLI: handle `-` before `csv`. + +## Suggestions taken + +- Docs state that `selectedModels` still narrows what is visible even for retained ids. +- `retainModels` added to `providerCatalogFingerprint` (:573) so two providers differing only in that list do not share a discovery flight. +- Flip #2860's "does not invent ids" test to assert the union. + +## Disposition + +All three blockers are additive edits inside the already-planned files plus one file (`auth-cors.ts`). No scope change. Proceed to B. + diff --git a/docs-site/src/content/docs/reference/configuration/providers.md b/docs-site/src/content/docs/reference/configuration/providers.md index 0cfd77f71e..d6b37c8086 100644 --- a/docs-site/src/content/docs/reference/configuration/providers.md +++ b/docs-site/src/content/docs/reference/configuration/providers.md @@ -83,6 +83,7 @@ differing backup and rewrites known legacy namespaced selected ids to bare ids. | `models?` | `string[]` | Seed/fallback model list. With `liveModels: false`, these are the only discovered models. | | `liveModels?` | `boolean` | Fetch the live catalog on start/sync (default `true`). Custom providers use `${baseUrl}/models`; built-ins may use a registry URL and filter. | | `selectedModels?` | `string[]` | Catalog allowlist after discovery. Non-empty exposes only those ids; empty or omitted exposes all discovered models. | +| `retainModels?` | `string[]` | Ids kept in the catalog even when live discovery omits them. They need not be repeated in `models`. Empty or omitted keeps today's behavior. | | `contextWindow?` | `number` | Provider-wide context fallback when upstream metadata is absent; otherwise a cap that retains smaller live metadata. The Models dashboard exposes this separately from `providerContextCaps`. | | `modelContextWindows?` | `Record` | Per-model context fallbacks/caps. These override `contextWindow`: an unknown window uses the configured value, while smaller live metadata remains authoritative. | | `modelInputModalities?` | `Record` | Per-model input hints such as `["text"]` or `["text", "image"]`. | @@ -575,6 +576,14 @@ silently replaced or truncated. Use `selectedModels` when discovery should still run but only selected ids should appear in Codex and `/v1/models`. The dashboard retains the full discovered list for later allowlist changes. +Use `retainModels` for the opposite problem: a provider whose `/models` endpoint omits an id that is +still callable (a private deployment, a preview id, an OpenAI-compatible gateway with a partial +listing). Listed ids are kept in the routed catalog with the same context and effort hints as +`models`, and they survive `liveModels: false` too. `selectedModels` still narrows what is visible, +so an id must be in both lists when an allowlist is active. Retaining an id does not make the +upstream accept it; a wrong id fails at request time with the upstream error. From the CLI: +`ocx provider edit --retain-models gemini-3.7-flash,other-id` (`-` clears). + Preview GPT-5.6 fallback entries use the same mechanism. The OpenAI API-key preset seeds base and Pro ids with context `922000` and max input `922000`; OpenRouter seeds `openai/gpt-5.6-sol`, `openai/gpt-5.6-terra`, and `openai/gpt-5.6-luna` with context `922000`. Pool/Direct advertises diff --git a/src/cli/provider-runtime.ts b/src/cli/provider-runtime.ts index 183d2e32a7..ebeac3d4ae 100644 --- a/src/cli/provider-runtime.ts +++ b/src/cli/provider-runtime.ts @@ -24,6 +24,7 @@ const USAGE = `Usage: [--auth-mode ] [--note ] [--api-key-transport ] [--headers ] [--enabled ] [--live-models ] + [--retain-models ] [--allow-private-network ] [--json] ocx provider test [--json] ocx provider quota [--refresh] [--json] @@ -48,6 +49,7 @@ async function edit(argv: string[], deps: RuntimeApiDeps): Promise { const note = cleared(takeOption(args, "--note")); const apiKeyTransport = cleared(takeOption(args, "--api-key-transport")); const headers = takeOption(args, "--headers"); + const retainModelsRaw = takeOption(args, "--retain-models"); const enabled = takeBooleanOption(args, "--enabled"); const liveModels = takeBooleanOption(args, "--live-models"); const allowPrivateNetwork = takeBooleanOption(args, "--allow-private-network"); @@ -74,6 +76,10 @@ async function edit(argv: string[], deps: RuntimeApiDeps): Promise { } } if (enabled !== undefined) patch.disabled = !enabled; + if (retainModelsRaw !== undefined) { + // `-` clears, matching the other `edit` scalars; test before csv() or it becomes ["-"]. + patch.retainModels = retainModelsRaw.trim() === "-" ? null : csv(retainModelsRaw); + } if (liveModels !== undefined) patch.liveModels = liveModels; if (allowPrivateNetwork !== undefined) patch.allowPrivateNetwork = allowPrivateNetwork; if (Object.keys(patch).length === 0) throw new CliUsageError("at least one edit option is required", USAGE); diff --git a/src/codex/catalog/provider-fetch.ts b/src/codex/catalog/provider-fetch.ts index bc86c8c6e4..45bdfa57b0 100644 --- a/src/codex/catalog/provider-fetch.ts +++ b/src/codex/catalog/provider-fetch.ts @@ -571,6 +571,7 @@ function providerCatalogFingerprint(name: string, prov: OcxProviderConfig): Reco base: prov.baseUrl ?? "", adapter: prov.adapter ?? "", models: [...(prov.models ?? [])].sort(), + retain: [...(prov.retainModels ?? [])].sort(), selected: [...(prov.selectedModels ?? [])].sort(), defaultModel: prov.defaultModel ?? null, ctx: prov.contextWindow ?? null, @@ -1304,7 +1305,14 @@ async function fetchProviderModelsWithAuth( && prov.googleMode === "vertex" && (prov.models?.length ?? 0) === 0 && Boolean(prov.defaultModel); - const configuredIds = seedVertexDefault && prov.defaultModel ? [prov.defaultModel] : (prov.models ?? []); + // Ordered dedupe union: Vertex seed, then `models`, then `retainModels`. `configured` is the + // single seed for the static path, the degraded fallback, drop diagnostics, and provider hints, + // so a retain-only id must enter here or it never exists to be retained (#1690). + const configuredIds = [...new Set([ + ...(seedVertexDefault && prov.defaultModel ? [prov.defaultModel] : []), + ...(prov.models ?? []), + ...(prov.retainModels ?? []), + ])]; const configured: CatalogModel[] = configuredIds.map(id => ({ id, provider: name, @@ -1700,9 +1708,14 @@ export function shouldExposeProviderModel(providerName: string, modelId: string) return true; } -export function shouldRetainConfiguredProviderModel(providerName: string, modelId: string): boolean { +export function shouldRetainConfiguredProviderModel( + providerName: string, + modelId: string, + prov?: OcxProviderConfig, +): boolean { if (CALLABLE_CONFIGURED_COMPATIBILITY_MODELS[providerName]?.has(modelId)) return true; if (providerName === "opencode-free") return modelId === "big-pickle" || modelId.endsWith("-free"); + if (modelInList(prov?.retainModels, modelId)) return true; return false; } @@ -1748,7 +1761,7 @@ export function mergeConfiguredModelsIntoLiveCatalog(opts: { } if ( seedVertexDefault === true - || shouldRetainConfiguredProviderModel(name, candidate.id) + || shouldRetainConfiguredProviderModel(name, candidate.id, prov) || (retainComboTargets && retainConfiguredModelIds?.has(candidate.id) === true) ) { out.push(candidate); diff --git a/src/config.ts b/src/config.ts index c17a86d1ff..e28e69fafd 100644 --- a/src/config.ts +++ b/src/config.ts @@ -529,6 +529,9 @@ const providerConfigSchema = z.object({ noStructuredOutputModels: z.array(z.string().min(1)) .transform(normalizeNonBlankStringArray) .optional(), + retainModels: z.array(z.string().min(1)) + .transform(normalizeNonBlankStringArray) + .optional(), omitReasoningEffortWithToolsModels: z.array(z.string().min(1)) .transform(normalizeNonBlankStringArray) .optional(), @@ -1368,6 +1371,17 @@ const configSchema = z.object({ message: structuredOutputOptOutError, }); } + const retainModelsError = nonBlankStringArrayConfigError( + (provider as { retainModels?: unknown }).retainModels, + "retainModels", + ); + if (retainModelsError) { + ctx.addIssue({ + code: "custom", + path: ["providers", redactSecretString(name), "retainModels"], + message: retainModelsError, + }); + } const toolReasoningOptOutError = nonBlankStringArrayConfigError( (provider as { omitReasoningEffortWithToolsModels?: unknown }).omitReasoningEffortWithToolsModels, "omitReasoningEffortWithToolsModels", diff --git a/src/providers/model-rename-migration.ts b/src/providers/model-rename-migration.ts index 3895386b58..13129264da 100644 --- a/src/providers/model-rename-migration.ts +++ b/src/providers/model-rename-migration.ts @@ -99,6 +99,9 @@ const MODEL_ID_LISTS = [ // their catalog instead of being renamed. OAuth reconciliation does not cover this // field, so the rename has to. "selectedModels", + // Same reasoning as `selectedModels`: a retired id pinned here would be resurrected as a + // ghost row on every discovery instead of following the rename (#1690). + "retainModels", "noVisionModels", "noReasoningModels", "noTemperatureModels", diff --git a/src/server/auth-cors.ts b/src/server/auth-cors.ts index 934e09ae59..10516ca31c 100644 --- a/src/server/auth-cors.ts +++ b/src/server/auth-cors.ts @@ -694,6 +694,8 @@ export function providerManagementConfigError(name: unknown, provider: unknown): "noStructuredOutputModels", ); if (structuredOutputOptOutError) return `provider ${name} ${structuredOutputOptOutError}`; + const retainModelsError = nonBlankStringArrayConfigError(raw.retainModels, "retainModels"); + if (retainModelsError) return `provider ${name} ${retainModelsError}`; const toolReasoningOptOutError = nonBlankStringArrayConfigError( raw.omitReasoningEffortWithToolsModels, "omitReasoningEffortWithToolsModels", @@ -792,6 +794,7 @@ export function safeConfigDTO(config: OcxConfig): unknown { "noTopPModels", "noPenaltyModels", "noStructuredOutputModels", + "retainModels", "omitReasoningEffortWithToolsModels", "upstreamHttpVersion", "autoToolChoiceOnlyModels", diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index 56c9d700c9..22da830989 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -396,6 +396,19 @@ function applyProviderPatchFields( } touched = true; } + if (Object.hasOwn(rawBody, "retainModels")) { + const value = rawBody.retainModels; + if (value === null) { + delete next.retainModels; + } else { + const error = nonBlankStringArrayConfigError(value, "retainModels"); + if (error) return { error }; + const models = normalizeNonBlankStringArray(value as string[]); + if (models.length > 0) next.retainModels = models; + else delete next.retainModels; + } + touched = true; + } if (Object.hasOwn(rawBody, "omitReasoningEffortWithToolsModels")) { const value = rawBody.omitReasoningEffortWithToolsModels; if (value === null) { @@ -538,6 +551,7 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise { + globalThis.fetch = originalFetch; + clearModelCache(); + resetCatalogRuntimeStateForTests(); +}); + +function stubLiveModels(ids: string[]): void { + globalThis.fetch = (async (input: RequestInfo | URL) => { + if (!String(input).includes("/models")) return new Response(null, { status: 404 }); + return Response.json({ data: ids.map(id => ({ id })) }); + }) as typeof fetch; +} + +function discoveryConfig(prov: Partial): OcxConfig { + return withStubbedProviderFetch({ + port: 10100, + defaultProvider: "demo", + providers: { + demo: { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + apiKey: "k", + liveModels: true, + ...prov, + }, + }, + } as unknown as OcxConfig); +} + +function provider(overrides: Partial = {}): OcxProviderConfig { + return { + adapter: "openai-chat", + baseUrl: "https://example.test/v1", + apiKey: "sk-test", + authMode: "key", + ...overrides, + }; +} + +function configured(ids: string[]) { + return ids.map(id => ({ id, provider: "demo" })); +} + +function live(ids: string[]) { + return ids.map(id => ({ id, provider: "demo" })); +} + +describe("shouldRetainConfiguredProviderModel", () => { + test("empty retainModels does not change behavior", () => { + expect(shouldRetainConfiguredProviderModel("demo", "any-id")).toBe(false); + expect(shouldRetainConfiguredProviderModel("demo", "any-id", provider())).toBe(false); + expect( + shouldRetainConfiguredProviderModel("demo", "any-id", provider({ retainModels: [] })), + ).toBe(false); + }); + + test("retainModels preserves listed id", () => { + expect( + shouldRetainConfiguredProviderModel( + "demo", + "kept-id", + provider({ retainModels: ["kept-id", "another"] }), + ), + ).toBe(true); + }); + + test("retainModels supports the family-suffix matcher used elsewhere", () => { + // modelInList treats entries ending with ":tag" as a wildcard for `id:tag` siblings. + expect( + shouldRetainConfiguredProviderModel( + "demo", + "kimi-k2.5:free", + provider({ retainModels: ["kimi-k2.5:free"] }), + ), + ).toBe(true); + expect( + shouldRetainConfiguredProviderModel( + "demo", + "kimi-k2.5:free", + provider({ retainModels: ["kimi-k2.5"] }), + ), + ).toBe(true); + }); + + test("built-in kimi / xai hardcoded tables still win", () => { + // Mirrors the canonical compatibility allow-list; ensures the new branch is purely additive. + expect(shouldRetainConfiguredProviderModel("kimi", "k3[1m]")).toBe(true); + expect(shouldRetainConfiguredProviderModel("xai", "grok-4.3")).toBe(true); + expect(shouldRetainConfiguredProviderModel("opencode-free", "big-pickle")).toBe(true); + }); +}); + +describe("mergeConfiguredModelsIntoLiveCatalog with retainModels", () => { + test("merge only retains what the caller seeded — the union happens at discovery", () => { + const prov = provider({ + models: ["configured-id"], + retainModels: ["configured-id", "ghost-id"], + }); + const { models, droppedConfiguredIds } = mergeConfiguredModelsIntoLiveCatalog({ + name: "demo", + provider: prov, + models: live([]), + configured: configured(["configured-id"]), + }); + expect(models.map(m => m.id)).toEqual(["configured-id"]); + expect(droppedConfiguredIds).toEqual([]); + }); + + test("retainModels keeps a configured id when live discovery omits it", () => { + const prov = provider({ + models: ["kept-id", "dropped-id"], + retainModels: ["kept-id"], + }); + const { models, droppedConfiguredIds } = mergeConfiguredModelsIntoLiveCatalog({ + name: "demo", + provider: prov, + models: live(["other-live-id"]), + configured: configured(["kept-id", "dropped-id"]), + }); + expect(models.map(m => m.id).sort()).toEqual(["kept-id", "other-live-id"]); + expect(droppedConfiguredIds).toEqual(["dropped-id"]); + }); + + test("live discovery empty (404-style) still keeps retained rows and surfaces the rest", () => { + const prov = provider({ + models: ["kept-id", "dropped-id"], + retainModels: ["kept-id"], + }); + const { models, droppedConfiguredIds } = mergeConfiguredModelsIntoLiveCatalog({ + name: "demo", + provider: prov, + models: live([]), + configured: configured(["kept-id", "dropped-id"]), + }); + expect(models.map(m => m.id)).toEqual(["kept-id"]); + expect(droppedConfiguredIds).toEqual(["dropped-id"]); + }); +}); + +describe("retainModels through provider discovery (#1690)", () => { + test("a retain-only id survives live discovery that omits it, with provider hints applied", async () => { + stubLiveModels(["live-id"]); + const models = await gatherRoutedModelsDirect(discoveryConfig({ + models: ["seen-id"], + retainModels: ["retained-only"], + modelContextWindows: { "retained-only": 123_456 }, + })); + const demo = models.filter(m => m.provider === "demo"); + expect(demo.map(m => m.id).sort()).toEqual(["live-id", "retained-only"]); + expect(demo.find(m => m.id === "retained-only")?.contextWindow).toBe(123_456); + }); + + test("a retained id the live catalog also returns yields one row", async () => { + stubLiveModels(["both-id"]); + const models = await gatherRoutedModelsDirect(discoveryConfig({ retainModels: ["both-id"] })); + expect(models.filter(m => m.provider === "demo").map(m => m.id)).toEqual(["both-id"]); + }); + + test("liveModels: false lists retainModels alongside models", async () => { + const models = await gatherRoutedModelsDirect(discoveryConfig({ + liveModels: false, + models: ["static-id"], + retainModels: ["static-id", "retained-only"], + })); + expect(models.filter(m => m.provider === "demo").map(m => m.id).sort()).toEqual(["retained-only", "static-id"]); + }); + + test("absent retainModels keeps today's drop behavior", async () => { + stubLiveModels(["live-id"]); + const models = await gatherRoutedModelsDirect(discoveryConfig({ models: ["unseen-id"] })); + expect(models.filter(m => m.provider === "demo").map(m => m.id)).toEqual(["live-id"]); + }); +}); diff --git a/tests/cli-headless-parity.test.ts b/tests/cli-headless-parity.test.ts index f9b9762eb6..94e223286a 100644 --- a/tests/cli-headless-parity.test.ts +++ b/tests/cli-headless-parity.test.ts @@ -329,6 +329,24 @@ describe("headless GUI parity CLI", () => { expect(clearRuntime.requests[0]?.body).toEqual({ headers: null }); }); + test("provider edit --retain-models sends the csv list and - clears it", async () => { + const runtime = fakeRuntime(); + const code = await handleProviderRuntimeCommand("edit", [ + "agw", "--retain-models", " gemini-3.7-flash, other-id ,gemini-3.7-flash", "--json", + ], runtime.deps); + expect(code).toBe(0); + expect(runtime.requests).toEqual([{ + path: "/api/providers?name=agw", + method: "PATCH", + body: { retainModels: ["gemini-3.7-flash", "other-id"] }, + }]); + + const clearRuntime = fakeRuntime(); + const clearCode = await handleProviderRuntimeCommand("edit", ["agw", "--retain-models", "-", "--json"], clearRuntime.deps); + expect(clearCode).toBe(0); + expect(clearRuntime.requests[0]?.body).toEqual({ retainModels: null }); + }); + test("provider edit rejects malformed --headers JSON without a request", async () => { const runtime = fakeRuntime(); const code = await handleProviderRuntimeCommand("edit", ["agw", "--headers", "{not json"], runtime.deps); diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index ec255b61f8..d8a0bdf29d 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -330,6 +330,57 @@ describe("provider management validation", () => { .toEqual(["deepseek-v4-flash", "other-model"]); }); + test("validates, exposes, and normalizes retainModels (#1690)", () => { + const provider = { + adapter: "openai-chat", + baseUrl: "https://relay.example/v1", + retainModels: ["gemini-3.7-flash"], + }; + expect(providerManagementConfigError("relay", provider)).toBeNull(); + for (const retainModels of ["gemini-3.7-flash", [""], [" "], [42]]) { + expect(providerManagementConfigError("relay", { ...provider, retainModels })) + .toContain("retainModels"); + } + + const dto = safeConfigDTO({ + port: 10100, + defaultProvider: "relay", + providers: { relay: provider }, + } as OcxConfig) as { providers: Record }; + expect(dto.providers.relay?.retainModels).toEqual(["gemini-3.7-flash"]); + + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + writeFileSync(join(TEST_DIR, "config.json"), JSON.stringify({ + ...config("127.0.0.1"), + defaultProvider: "relay", + providers: { + relay: { + adapter: "openai-chat", + baseUrl: "https://relay.example/v1", + retainModels: [" gemini-3.7-flash ", "gemini-3.7-flash", " other-model "], + }, + }, + })); + expect(loadConfig().providers.relay?.retainModels).toEqual(["gemini-3.7-flash", "other-model"]); + + writeFileSync(join(TEST_DIR, "config.json"), JSON.stringify({ + ...config("127.0.0.1"), + defaultProvider: "relay", + providers: { relay: { ...provider, retainModels: "gemini-3.7-flash" } }, + })); + // Invalid config falls back to defaults (with a backup) rather than throwing; the relay + // provider must be gone, proving the schema rejected the string form with a path. + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + expect(loadConfig().providers.relay).toBeUndefined(); + expect(errorSpy.mock.calls.map(call => String(call[0])).join("\n")).toContain("providers.relay.retainModels"); + } finally { + errorSpy.mockRestore(); + } + }); + test("validates, exposes, and normalizes tool-bearing reasoning-effort opt-outs", () => { const provider = { adapter: "openai-chat", @@ -2206,6 +2257,37 @@ describe("provider management validation", () => { providers: Record; }; expect(saved.providers["structured-output-toggle"].noStructuredOutputModels).toBeUndefined(); + + const retainInvalid = await fetch(new URL("/api/providers?name=structured-output-toggle", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ retainModels: "gemini-3.7-flash" }), + }); + expect(retainInvalid.status).toBe(400); + + const retainRes = await fetch(new URL("/api/providers?name=structured-output-toggle", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ retainModels: [" gemini-3.7-flash ", "gemini-3.7-flash"] }), + }); + expect(retainRes.status).toBe(200); + const retainList = await fetch(new URL("/api/providers", server.url)).then(response => response.json()) as Array<{ + name: string; + retainModels?: string[]; + }>; + expect(retainList.find(provider => provider.name === "structured-output-toggle")?.retainModels) + .toEqual(["gemini-3.7-flash"]); + + const retainClear = await fetch(new URL("/api/providers?name=structured-output-toggle", server.url), { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ retainModels: null }), + }); + expect(retainClear.status).toBe(200); + const retainSaved = await fetch(new URL("/api/config", server.url)).then(response => response.json()) as { + providers: Record; + }; + expect(retainSaved.providers["structured-output-toggle"].retainModels).toBeUndefined(); } finally { await server.stop(true); } diff --git a/tests/model-rename-migration.test.ts b/tests/model-rename-migration.test.ts index a4648b04ad..61796018ce 100644 --- a/tests/model-rename-migration.test.ts +++ b/tests/model-rename-migration.test.ts @@ -34,6 +34,7 @@ function staleConfig(): OcxConfig { modelDefaultReasoningEfforts: { "qwen3.8-max-preview": "xhigh" }, preserveReasoningContentModels: ["glm-5.2", "qwen3.8-max-preview", "qwen3.7-max"], thinkingBudgetModels: ["qwen3.8-max-preview", "qwen3.7-max"], + retainModels: ["qwen3.8-max-preview"], }, }, disabledModels: ["alibaba-token-plan-intl/qwen3.8-max-preview", "other/model"], @@ -56,6 +57,7 @@ describe("registry model rename migration (#1610)", () => { expect(prov.modelDefaultReasoningEfforts?.["qwen3.8-max"]).toBe("xhigh"); expect(prov.preserveReasoningContentModels).toEqual(["glm-5.2", "qwen3.8-max", "qwen3.7-max"]); expect(prov.thinkingBudgetModels).toEqual(["qwen3.8-max", "qwen3.7-max"]); + expect(prov.retainModels).toEqual(["qwen3.8-max"]); expect(warnings.some(w => w.includes("qwen3.8-max"))).toBe(true); }); @@ -82,6 +84,7 @@ describe("registry model rename migration (#1610)", () => { prov.modelDefaultReasoningEfforts = {}; prov.preserveReasoningContentModels = ["qwen3.8-max"]; prov.thinkingBudgetModels = ["qwen3.7-max"]; + prov.retainModels = ["qwen3.8-max"]; clean.disabledModels = ["other/model"]; const { changed, warnings } = projectModelRenames(clean, [RENAME]);