From 2b17a7b4e80bb2bd30bd95d047c31ad17dd339b6 Mon Sep 17 00:00:00 2001 From: Marius Date: Mon, 20 Apr 2026 15:05:49 +0200 Subject: [PATCH] fix: persist custom provider model and variant deletions (#9239) * fix: persist custom provider model and variant deletions The CLI config.update endpoint deep-merges its payload with existing global config, so removing a model or reasoning variant from a custom provider in the UI had no effect on disk. Save payloads now emit null sentinels for removed IDs, and the Provider schema accepts nullable record values so stripNulls can delete them during the merge. Closes #9186 * chore: update kilo-vscode visual regression baselines --------- Co-authored-by: github-actions[bot] --- .changeset/custom-provider-delete-sticks.md | 6 + packages/kilo-vscode/src/provider-actions.ts | 10 +- .../kilo-vscode/src/shared/custom-provider.ts | 36 ++++++ .../tests/unit/custom-provider.test.ts | 51 ++++++++ .../tests/unit/provider-actions-save.test.ts | 109 +++++++++++++++- packages/opencode/src/config/config.ts | 7 +- .../src/kilocode/provider/provider.ts | 2 +- packages/opencode/src/provider/provider.ts | 5 +- .../kilocode/custom-provider-delete.test.ts | 118 ++++++++++++++++++ 9 files changed, 333 insertions(+), 11 deletions(-) create mode 100644 .changeset/custom-provider-delete-sticks.md create mode 100644 packages/opencode/test/kilocode/custom-provider-delete.test.ts diff --git a/.changeset/custom-provider-delete-sticks.md b/.changeset/custom-provider-delete-sticks.md new file mode 100644 index 00000000000..ae10a1ec3f1 --- /dev/null +++ b/.changeset/custom-provider-delete-sticks.md @@ -0,0 +1,6 @@ +--- +"kilo-code": patch +"@kilocode/cli": patch +--- + +Fix custom provider model and variant deletions being silently reverted on save. Removing a model or reasoning variant from a custom provider now actually removes it from your config. diff --git a/packages/kilo-vscode/src/provider-actions.ts b/packages/kilo-vscode/src/provider-actions.ts index 9df4a3eec3b..d23c700be8d 100644 --- a/packages/kilo-vscode/src/provider-actions.ts +++ b/packages/kilo-vscode/src/provider-actions.ts @@ -4,7 +4,11 @@ */ import type { KiloClient } from "@kilocode/sdk/v2" import { validateProviderID as validateProviderIDShared } from "./shared/custom-provider" -import { resolveCustomProviderAuth, sanitizeCustomProviderConfig } from "./shared/custom-provider" +import { + resolveCustomProviderAuth, + sanitizeCustomProviderConfig, + withCustomProviderDeletions, +} from "./shared/custom-provider" import { KILO_AUTO, parseModelString } from "./shared/provider-model" /** @@ -287,10 +291,12 @@ export async function saveCustomProvider( const globalConfig = (await ctx.client.global.config.get({ throwOnError: true })).data ?? {} const disabled = globalConfig.disabled_providers ?? [] const nextDisabled = disabled.filter((item: string) => item !== id) + const existing = (globalConfig.provider as Record | undefined)?.[id] + const patch = withCustomProviderDeletions(existing, sanitized.value) const { data: updated } = await ctx.client.global.config.update( { config: { - provider: { [id]: sanitized.value }, + provider: { [id]: patch }, disabled_providers: nextDisabled, }, }, diff --git a/packages/kilo-vscode/src/shared/custom-provider.ts b/packages/kilo-vscode/src/shared/custom-provider.ts index a2cce5914b1..f75cfcd7319 100644 --- a/packages/kilo-vscode/src/shared/custom-provider.ts +++ b/packages/kilo-vscode/src/shared/custom-provider.ts @@ -147,3 +147,39 @@ export function sanitizeCustomProviderConfig(provider: unknown): { value: Saniti return { value: normalizeCustomProviderConfig(result.data) } } + +type AnyRecord = Record + +function isRecord(v: unknown): v is AnyRecord { + return !!v && typeof v === "object" && !Array.isArray(v) +} + +/** + * Build a provider patch that includes null sentinels for models and variants + * that existed in the previous config but are absent from the new one. The CLI + * `config.update` endpoint deep-merges the payload with the existing config; + * without explicit nulls, removed entries would persist on disk. + */ +export function withCustomProviderDeletions(existing: unknown, next: SanitizedProviderConfig): SanitizedProviderConfig { + if (!isRecord(existing)) return next + const oldModels = isRecord(existing.models) ? existing.models : {} + const patched: AnyRecord = { ...next.models } + + for (const id of Object.keys(oldModels)) { + if (!(id in patched)) { + patched[id] = null + continue + } + const oldModel = oldModels[id] + const oldVariants = isRecord(oldModel) && isRecord(oldModel.variants) ? oldModel.variants : {} + const newModel = patched[id] + if (!isRecord(newModel)) continue + const newVariants = isRecord(newModel.variants) ? newModel.variants : {} + const removedVariants = Object.keys(oldVariants).filter((v) => !(v in newVariants)) + if (removedVariants.length === 0) continue + const nulls = Object.fromEntries(removedVariants.map((v) => [v, null])) + patched[id] = { ...newModel, variants: { ...newVariants, ...nulls } } + } + + return { ...next, models: patched as SanitizedProviderConfig["models"] } +} diff --git a/packages/kilo-vscode/tests/unit/custom-provider.test.ts b/packages/kilo-vscode/tests/unit/custom-provider.test.ts index 86af79b3a8e..1a3ce2c242c 100644 --- a/packages/kilo-vscode/tests/unit/custom-provider.test.ts +++ b/packages/kilo-vscode/tests/unit/custom-provider.test.ts @@ -6,6 +6,7 @@ import { resolveCustomProviderAuth, sanitizeCustomProviderConfig, validateProviderID, + withCustomProviderDeletions, } from "../../src/shared/custom-provider" describe("validateProviderID", () => { @@ -143,3 +144,53 @@ describe("sanitizeCustomProviderConfig", () => { expect("error" in result ? result.error : "").toContain("mcpServer") }) }) + +describe("withCustomProviderDeletions", () => { + const baseNext = { + npm: "@ai-sdk/openai-compatible" as const, + name: "My Provider", + options: { baseURL: "https://example.com/v1" }, + models: { keep: { name: "Keep" } }, + } + + it("passes through unchanged when there is no prior config", () => { + expect(withCustomProviderDeletions(undefined, baseNext)).toEqual(baseNext) + expect(withCustomProviderDeletions({}, baseNext)).toEqual(baseNext) + }) + + it("emits null for models present in existing but absent in next", () => { + const existing = { models: { keep: { name: "Keep" }, gone: { name: "Gone" } } } + const result = withCustomProviderDeletions(existing, baseNext) + const models = result.models as Record + expect(models.keep).toEqual({ name: "Keep" }) + expect(models.gone).toBeNull() + }) + + it("emits null for variants removed from a surviving model", () => { + const existing = { + models: { + keep: { + name: "Keep", + variants: { high: { reasoningEffort: "high" }, low: { reasoningEffort: "low" } }, + }, + }, + } + const next = { + ...baseNext, + models: { + keep: { name: "Keep", variants: { high: { reasoningEffort: "high" } } }, + }, + } as typeof baseNext + const result = withCustomProviderDeletions(existing, next) + const model = (result.models as Record }>).keep + expect(model.variants.high).toEqual({ reasoningEffort: "high" }) + expect(model.variants.low).toBeNull() + }) + + it("does not touch variants on a model that is being deleted", () => { + const existing = { models: { gone: { name: "Gone", variants: { a: {} } } } } + const result = withCustomProviderDeletions(existing, baseNext) + const models = result.models as Record + expect(models.gone).toBeNull() + }) +}) diff --git a/packages/kilo-vscode/tests/unit/provider-actions-save.test.ts b/packages/kilo-vscode/tests/unit/provider-actions-save.test.ts index 2e7dbfe0684..0f550492bf5 100644 --- a/packages/kilo-vscode/tests/unit/provider-actions-save.test.ts +++ b/packages/kilo-vscode/tests/unit/provider-actions-save.test.ts @@ -1,12 +1,14 @@ import { describe, expect, it } from "bun:test" import { fetchProviderData, saveCustomProvider } from "../../src/provider-actions" -function createCtx() { +type ExistingGlobal = { disabled_providers?: string[]; provider?: Record } + +function createCtx(existing: ExistingGlobal = { disabled_providers: [] }) { const calls = { set: [] as Array<{ providerID: string; auth: { type: string; key: string } }>, remove: [] as Array<{ providerID: string }>, posts: [] as unknown[], - config: [] as unknown[], + config: [] as Array<{ config: Record }>, cached: [] as unknown[], refresh: 0, dispose: 0, @@ -26,8 +28,8 @@ function createCtx() { }, global: { config: { - get: async () => ({ data: { disabled_providers: [] } }), - update: async (input: unknown) => { + get: async () => ({ data: existing }), + update: async (input: { config: Record }) => { calls.config.push(input) return { data: input } }, @@ -90,6 +92,105 @@ describe("saveCustomProvider", () => { expect(calls.remove).toHaveLength(0) expect(calls.set).toEqual([{ providerID: "myprovider", auth: { type: "api", key: "sk-test" } }]) }) + + // Regression tests for https://github.com/Kilo-Org/kilocode/issues/9186 + // + // The CLI's config.update endpoint deep-merges its payload with the existing + // global config. When the user removes a model or variant from a custom + // provider and saves, the removed entry stays on disk because the save + // payload only lists the surviving entries. stripNulls in the merge layer + // will remove keys explicitly set to null — the save path must emit these + // sentinels for removed model ids and variant names. + it("emits null sentinels for models removed since last save", async () => { + const existing = { + disabled_providers: [], + provider: { + myprovider: { + npm: "@ai-sdk/openai-compatible", + name: "My Provider", + options: { baseURL: "https://example.com/v1" }, + models: { + "model-keep": { name: "Keep" }, + "model-gone": { name: "Gone" }, + }, + }, + }, + } + const { ctx, calls, setCachedConfig } = createCtx(existing) + + const next = { + name: "My Provider", + options: { baseURL: "https://example.com/v1" }, + models: { + "model-keep": { name: "Keep" }, + }, + } + await saveCustomProvider(ctx, "req", "myprovider", next, undefined, false, null, setCachedConfig) + + expect(calls.config).toHaveLength(1) + const payload = calls.config[0].config.provider as Record }> + expect(payload.myprovider.models["model-keep"]).toBeDefined() + expect(payload.myprovider.models["model-gone"]).toBeNull() + }) + + it("emits null sentinels for variants removed from a model that still exists", async () => { + const existing = { + disabled_providers: [], + provider: { + myprovider: { + npm: "@ai-sdk/openai-compatible", + name: "My Provider", + options: { baseURL: "https://example.com/v1" }, + models: { + "model-1": { + name: "Model One", + reasoning: true, + variants: { + high: { reasoningEffort: "high" }, + low: { reasoningEffort: "low" }, + }, + }, + }, + }, + }, + } + const { ctx, calls, setCachedConfig } = createCtx(existing) + + const next = { + name: "My Provider", + options: { baseURL: "https://example.com/v1" }, + models: { + "model-1": { + name: "Model One", + reasoning: true, + variants: { high: { reasoningEffort: "high" } }, + }, + }, + } + await saveCustomProvider(ctx, "req", "myprovider", next, undefined, false, null, setCachedConfig) + + expect(calls.config).toHaveLength(1) + const model = ( + calls.config[0].config.provider as Record< + string, + { models: Record }> } + > + ).myprovider.models["model-1"] + expect(model.variants).toBeDefined() + expect(model.variants?.high).toBeDefined() + expect(model.variants?.low).toBeNull() + }) + + it("does not emit sentinels when the provider is new", async () => { + const { ctx, calls, setCachedConfig } = createCtx() + + await saveCustomProvider(ctx, "req", "myprovider", createProvider(), undefined, false, null, setCachedConfig) + + expect(calls.config).toHaveLength(1) + const models = (calls.config[0].config.provider as Record }>).myprovider + .models + expect(Object.values(models).every((v) => v !== null)).toBe(true) + }) }) describe("fetchProviderData", () => { diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 8e2c4f7ddf2..e5a89c4c050 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -875,11 +875,14 @@ export namespace Config { variants: z .record( z.string(), + // kilocode_change start - allow null values so removed variants can be deleted via stripNulls on save z .object({ disabled: z.boolean().optional().describe("Disable this variant for the model"), }) - .catchall(z.any()), + .catchall(z.any()) + .nullable(), + // kilocode_change end ) .optional() .describe("Variant-specific configuration"), @@ -929,7 +932,7 @@ export namespace Config { }) .catchall(z.any()) .optional(), - models: z.record(z.string(), Model).optional(), + models: z.record(z.string(), Model.nullable()).optional(), // kilocode_change - allow null values so removed models can be deleted via stripNulls on save }) .partial() .strict() diff --git a/packages/opencode/src/kilocode/provider/provider.ts b/packages/opencode/src/kilocode/provider/provider.ts index 10f4e1e142f..024d778d282 100644 --- a/packages/opencode/src/kilocode/provider/provider.ts +++ b/packages/opencode/src/kilocode/provider/provider.ts @@ -69,7 +69,7 @@ export function patchConfigModel(cfg: any, existing: any) { ai_sdk_provider: cfg.ai_sdk_provider ?? existing?.ai_sdk_provider, variants: cfg.variants ? mapValues( - pickBy(cfg.variants, (v) => !v.disabled), + pickBy(cfg.variants, (v) => !!v && !v.disabled), (v) => omit(v, ["disabled"]), ) : {}, diff --git a/packages/opencode/src/provider/provider.ts b/packages/opencode/src/provider/provider.ts index d208cf988ba..a06da2805c6 100644 --- a/packages/opencode/src/provider/provider.ts +++ b/packages/opencode/src/provider/provider.ts @@ -1125,6 +1125,7 @@ export namespace Provider { } for (const [modelID, model] of Object.entries(provider.models ?? {})) { + if (!model) continue // kilocode_change - null entries are transient delete sentinels const existingModel = parsed.models[model.id ?? modelID] const name = iife(() => { if (model.name) return model.name @@ -1195,7 +1196,7 @@ export namespace Provider { } const merged = mergeDeep(ProviderTransform.variants(parsedModel), model.variants ?? {}) parsedModel.variants = mapValues( - pickBy(merged, (v) => !v.disabled), + pickBy(merged, (v): v is NonNullable => !!v && !v.disabled), // kilocode_change - drop null delete sentinels (v) => omit(v, ["disabled"]), ) parsed.models[modelID] = parsedModel @@ -1357,7 +1358,7 @@ export namespace Provider { if (configVariants && model.variants) { const merged = mergeDeep(model.variants, configVariants) model.variants = mapValues( - pickBy(merged, (v) => !v.disabled), + pickBy(merged, (v): v is NonNullable => !!v && !v.disabled), // kilocode_change - drop null delete sentinels (v) => omit(v, ["disabled"]), ) } diff --git a/packages/opencode/test/kilocode/custom-provider-delete.test.ts b/packages/opencode/test/kilocode/custom-provider-delete.test.ts new file mode 100644 index 00000000000..affa9f703c8 --- /dev/null +++ b/packages/opencode/test/kilocode/custom-provider-delete.test.ts @@ -0,0 +1,118 @@ +// kilocode_change - new file +// +// Regression tests for https://github.com/Kilo-Org/kilocode/issues/9186 +// +// When the user removes a model or a variant from a custom provider and saves, +// the removed entry must disappear from the config on disk. The save path +// relies on `null` sentinels being allowed in the Provider models record and +// in the Model variants record so that `mergeConfig` (merge + stripNulls) can +// delete them cleanly. + +import { describe, expect, it } from "bun:test" +import { Config } from "../../src/config/config" +import { KilocodeConfig } from "../../src/kilocode/config/config" + +describe("Config.Info — null sentinels for custom provider deletes", () => { + it("accepts a null model value inside a provider", () => { + const parsed = Config.Info.safeParse({ + provider: { + myprovider: { + name: "My Provider", + models: { + "model-gone": null, + }, + }, + }, + }) + expect(parsed.success).toBe(true) + }) + + it("accepts a null variant value inside a model", () => { + const parsed = Config.Info.safeParse({ + provider: { + myprovider: { + name: "My Provider", + models: { + "model-1": { + variants: { + low: null, + }, + }, + }, + }, + }, + }) + expect(parsed.success).toBe(true) + }) +}) + +describe("KilocodeConfig.mergeConfig — custom provider model/variant deletion", () => { + it("drops a model from an existing provider when the patch sets it to null", () => { + const existing = { + provider: { + myprovider: { + name: "My Provider", + models: { + "model-keep": { name: "Keep" }, + "model-gone": { name: "Gone" }, + }, + }, + }, + } as unknown as Config.Info + const patch = { + provider: { + myprovider: { + models: { + "model-keep": { name: "Keep" }, + "model-gone": null, + }, + }, + }, + } as unknown as Config.Info + + const merged = KilocodeConfig.mergeConfig(existing, patch) + const models = (merged.provider as Record }>).myprovider.models + expect(models["model-keep"]).toBeDefined() + expect("model-gone" in models).toBe(false) + }) + + it("drops a variant from an existing model when the patch sets it to null", () => { + const existing = { + provider: { + myprovider: { + name: "My Provider", + models: { + "model-1": { + name: "Model One", + variants: { + high: { reasoningEffort: "high" }, + low: { reasoningEffort: "low" }, + }, + }, + }, + }, + }, + } as unknown as Config.Info + const patch = { + provider: { + myprovider: { + models: { + "model-1": { + variants: { + high: { reasoningEffort: "high" }, + low: null, + }, + }, + }, + }, + }, + } as unknown as Config.Info + + const merged = KilocodeConfig.mergeConfig(existing, patch) + const variants = ( + merged.provider as Record }> }> + ).myprovider.models["model-1"].variants + expect(variants.high).toBeDefined() + expect("low" in variants).toBe(false) + }) +})