diff --git a/src/cli/provider.ts b/src/cli/provider.ts index 34eb1cd7079..a418a1d589f 100644 --- a/src/cli/provider.ts +++ b/src/cli/provider.ts @@ -12,6 +12,7 @@ import { apiKeyTransportConfigError, hasOwnProvider, isValidProviderName, loadCo import { hasHelpFlag } from "./help"; import { getProviderRegistryEntry, PROVIDER_REGISTRY } from "../providers/registry"; import { providerConfigSeed } from "../providers/derive"; +import { dropProviderCustomModels } from "../providers/provider-id-rewrite"; import type { OcxProviderConfig } from "../types"; import { findLiveProxy } from "../server/proxy-liveness"; import { syncModelsToCodex } from "../codex/sync"; @@ -302,6 +303,7 @@ function handleRemove(args: string[]): void { } delete config.providers[name]; + const droppedCustomModels = dropProviderCustomModels(config, name); validateAndSave(config); @@ -312,11 +314,16 @@ function handleRemove(args: string[]): void { remainingProviders: Object.keys(config.providers), defaultProvider: config.defaultProvider, needsSync: true, + ...(droppedCustomModels > 0 ? { droppedCustomModels } : {}), }, null, 2)); return; } console.log(`✅ Provider "${name}" removed.`); + if (droppedCustomModels > 0) { + const plural = droppedCustomModels === 1 ? "model" : "models"; + console.log(` Also removed ${droppedCustomModels} custom ${plural} that belonged to it.`); + } } // --------------------------------------------------------------------------- diff --git a/src/providers/provider-id-rewrite.ts b/src/providers/provider-id-rewrite.ts index 2a52399a72c..f34e78b3260 100644 --- a/src/providers/provider-id-rewrite.ts +++ b/src/providers/provider-id-rewrite.ts @@ -148,3 +148,32 @@ export function rewriteProviderReferences(config: OcxConfig, from: string, to: s return { changed, collisions }; } + +/** + * Drop the custom-model rows that belonged to a provider being removed. + * + * The sibling of the rename pass above. `rewriteProviderReferences` already + * carries `customModels[].provider` across a rename, so the array tracks the + * provider lifecycle — but removal used to delete only `config.providers[name]` + * and leave the rows behind. Those orphans still reach `/api/models` and the + * generated Codex catalog, which key on the row rather than on provider + * existence, so they surface as models that resolve to nothing (#1273). + * + * Only the rows are touched: the `customModelCatalogMigration` marker records + * one-time ownership of pre-marker rows and must survive removal unchanged, or + * an older binary's view of that ownership silently changes. + * + * Returns the number of rows dropped so callers can report it. + */ +export function dropProviderCustomModels(config: OcxConfig, provider: string): number { + const existing = config.customModels; + if (!Array.isArray(existing) || existing.length === 0) return 0; + const kept = existing.filter(model => model.provider !== provider); + if (kept.length === existing.length) return 0; + // Match the add/remove routes: an emptied list is dropped rather than left as + // `[]`, so the `customModels` field is absent either way. Only that field — + // the `customModelCatalogMigration` marker is deliberately left in place. + if (kept.length > 0) config.customModels = kept; + else delete config.customModels; + return existing.length - kept.length; +} diff --git a/src/server/management/provider-routes.ts b/src/server/management/provider-routes.ts index a196deabc94..8a4a50d98ed 100644 --- a/src/server/management/provider-routes.ts +++ b/src/server/management/provider-routes.ts @@ -615,13 +615,20 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise 0 ? { droppedCustomModels } : {}), + catalogRefresh, + }); } if (url.pathname === "/api/provider-context-caps" && req.method === "GET") { diff --git a/tests/cli-provider.test.ts b/tests/cli-provider.test.ts index 20b9d42cf5d..e7b8e1e6aab 100644 --- a/tests/cli-provider.test.ts +++ b/tests/cli-provider.test.ts @@ -223,6 +223,45 @@ describe("ocx provider", () => { } }); + test("provider remove drops that provider's custom models (#1273)", () => { + const { dir } = freshConfig({ + providers: { + openai: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward" }, + huggingface: { adapter: "openai-chat", baseUrl: "https://api.hf.test/v1", apiKey: "k" }, + }, + customModels: [ + { id: "keep-1", provider: "openai", modelId: "kept-model" }, + { id: "drop-1", provider: "huggingface", modelId: "DeepSeek-V4-Flash-0731" }, + ], + // Seeded so the assertion below proves removal does not rewrite one-time + // ownership: an older binary must keep seeing the same legacy slugs. + customModelCatalogMigration: { + version: 1, + legacyOwnedSlugs: ["huggingface/DeepSeek-V4-Flash-0731", "openai/kept-model"], + }, + }); + try { + const result = runCli(["provider", "remove", "huggingface", "--json"], { OPENCODEX_HOME: dir }); + expect(result.status).toBe(0); + expect(JSON.parse(result.stdout)).toMatchObject({ + action: "removed", + provider: "huggingface", + droppedCustomModels: 1, + }); + + const config = readConfig(dir); + expect(config.customModels).toEqual([ + { id: "keep-1", provider: "openai", modelId: "kept-model" }, + ]); + expect(config.customModelCatalogMigration).toEqual({ + version: 1, + legacyOwnedSlugs: ["huggingface/DeepSeek-V4-Flash-0731", "openai/kept-model"], + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + test("provider remove rejects default provider", () => { const { dir } = freshConfig(); try { diff --git a/tests/management-provider-validation.test.ts b/tests/management-provider-validation.test.ts index 1eb622584e7..81ab8133be1 100644 --- a/tests/management-provider-validation.test.ts +++ b/tests/management-provider-validation.test.ts @@ -1072,6 +1072,69 @@ describe("provider management validation", () => { } }); + test("provider deletion removes that provider's custom models (#1273)", async () => { + if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); + mkdirSync(TEST_DIR, { recursive: true }); + process.env.OPENCODEX_HOME = TEST_DIR; + saveConfig({ + port: 0, + defaultProvider: "test-openai", + providers: { + "test-openai": { + adapter: "openai-chat", + baseUrl: "https://api.example.test/v1", + apiKey: "sk-secret-value", + }, + removable: { + adapter: "openai-chat", + baseUrl: "https://api.removable.test/v1", + apiKey: "sk-removable", + }, + }, + customModels: [ + { id: "keep-1", provider: "test-openai", modelId: "kept-model" }, + { id: "drop-1", provider: "removable", modelId: "ghost-model" }, + ], + // Seeded so the assertion below covers the real persistence path, not just + // the helper: `projectCustomModelCatalogMigration` runs inside the save and + // must carry this marker through a provider delete unchanged. + customModelCatalogMigration: { + version: 1, + legacyOwnedSlugs: ["removable/ghost-model", "test-openai/kept-model"], + }, + } as unknown as Parameters[0]); + + const server = startServer(0); + try { + const response = await fetch(new URL("/api/providers?name=removable", server.url), { + method: "DELETE", + }); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ success: true, droppedCustomModels: 1 }); + + // The dashboard model page reads this route; a surviving row here is the + // ghost model users see pointing at a provider that no longer exists. + const customModels = await fetch(new URL("/api/custom-models", server.url)); + expect(await customModels.json()).toEqual([ + { id: "keep-1", provider: "test-openai", modelId: "kept-model" }, + ]); + + const persisted = JSON.parse(readFileSync(join(TEST_DIR, "config.json"), "utf8")) as { + customModels?: unknown; + customModelCatalogMigration?: unknown; + }; + expect(persisted.customModels).toEqual([ + { id: "keep-1", provider: "test-openai", modelId: "kept-model" }, + ]); + expect(persisted.customModelCatalogMigration).toEqual({ + version: 1, + legacyOwnedSlugs: ["removable/ghost-model", "test-openai/kept-model"], + }); + } finally { + await server.stop(true); + } + }); + test("provider management switches the default and reassigns it when removed", async () => { if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/provider-id-rewrite.test.ts b/tests/provider-id-rewrite.test.ts index 693653b0f92..5c6fc5a8b09 100644 --- a/tests/provider-id-rewrite.test.ts +++ b/tests/provider-id-rewrite.test.ts @@ -1,7 +1,7 @@ import { expect, test } from "bun:test"; import { comboConfigError } from "../src/combos"; import { providerContextCap } from "../src/providers/context-cap"; -import { rewriteProviderReferences } from "../src/providers/provider-id-rewrite"; +import { dropProviderCustomModels, rewriteProviderReferences } from "../src/providers/provider-id-rewrite"; import type { OcxConfig, OcxProviderConfig } from "../src/types"; const FROM = "alibaba-token-plan"; @@ -131,3 +131,81 @@ test("does not touch providers[*].selectedModels", () => { expect(rewriteProviderReferences(config, FROM, TO).changed).toBe(0); expect(config.providers.openrouter!.selectedModels).toEqual([`${FROM}/qwen3.7-max`]); }); + +// --------------------------------------------------------------------------- +// dropProviderCustomModels — the removal sibling of the rename pass (#1273) +// --------------------------------------------------------------------------- + +function customModelsConfig(models: Array<{ id: string; provider: string; modelId: string }>): OcxConfig { + return { + providers: { + huggingface: { adapter: "openai-chat" }, + "agnes-ai": { adapter: "openai-chat" }, + }, + customModels: models, + } as unknown as OcxConfig; +} + +test("removal drops only the departing provider's custom models", () => { + const config = customModelsConfig([ + { id: "a", provider: "agnes-ai", modelId: "agnes-2.5-flash" }, + { id: "b", provider: "huggingface", modelId: "DeepSeek-V4-Flash-0731" }, + { id: "c", provider: "huggingface", modelId: "another-model" }, + ]); + + expect(dropProviderCustomModels(config, "huggingface")).toBe(2); + expect(config.customModels).toEqual([ + { id: "a", provider: "agnes-ai", modelId: "agnes-2.5-flash" }, + ] as OcxConfig["customModels"]); +}); + +test("removing the last custom model deletes the key rather than leaving []", () => { + // The add/remove routes drop an emptied list, so the `customModels` field is + // absent either way. Only that field: the `customModelCatalogMigration` + // marker is deliberately preserved, so the two configs are not identical. + const config = customModelsConfig([ + { id: "b", provider: "huggingface", modelId: "DeepSeek-V4-Flash-0731" }, + ]); + + expect(dropProviderCustomModels(config, "huggingface")).toBe(1); + expect(Object.hasOwn(config, "customModels")).toBe(false); +}); + +test("a provider with no custom models is a no-op that leaves the array identical", () => { + const rows = [{ id: "a", provider: "agnes-ai", modelId: "agnes-2.5-flash" }]; + const config = customModelsConfig(rows); + const before = config.customModels; + + expect(dropProviderCustomModels(config, "huggingface")).toBe(0); + // Same reference, not merely a deep-equal copy: an untouched save must not + // look like a mutation to anything comparing identity. + expect(config.customModels).toBe(before); +}); + +test("an absent customModels key is left absent", () => { + const config = { providers: { huggingface: { adapter: "openai-chat" } } } as unknown as OcxConfig; + expect(dropProviderCustomModels(config, "huggingface")).toBe(0); + expect(Object.hasOwn(config, "customModels")).toBe(false); +}); + +test("removal leaves the custom-model ownership marker untouched", () => { + // legacyOwnedSlugs records one-time ownership of pre-marker rows. Rewriting it + // here would change an older binary's view of what it may delete, which the + // migration module explicitly warns against. + const config = customModelsConfig([ + { id: "a", provider: "agnes-ai", modelId: "agnes-2.5-flash" }, + { id: "b", provider: "huggingface", modelId: "DeepSeek-V4-Flash-0731" }, + ]); + const marker = { + version: 1, + legacyOwnedSlugs: ["agnes-ai/agnes-2.5-flash", "huggingface/DeepSeek-V4-Flash-0731"], + }; + (config as unknown as Record).customModelCatalogMigration = marker; + + dropProviderCustomModels(config, "huggingface"); + + expect((config as unknown as Record).customModelCatalogMigration).toEqual({ + version: 1, + legacyOwnedSlugs: ["agnes-ai/agnes-2.5-flash", "huggingface/DeepSeek-V4-Flash-0731"], + }); +});