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) + }) +})