mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-10-30 19:32:39 +08:00
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] <github-actions[bot]@users.noreply.github.com>
This commit is contained in:
co-authored by
github-actions[bot]
parent
6f17bb753b
commit
2b17a7b4e8
@@ -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.
|
||||
@@ -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<string, unknown> | 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,
|
||||
},
|
||||
},
|
||||
|
||||
@@ -147,3 +147,39 @@ export function sanitizeCustomProviderConfig(provider: unknown): { value: Saniti
|
||||
|
||||
return { value: normalizeCustomProviderConfig(result.data) }
|
||||
}
|
||||
|
||||
type AnyRecord = Record<string, unknown>
|
||||
|
||||
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"] }
|
||||
}
|
||||
|
||||
@@ -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<string, unknown>
|
||||
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<string, { variants: Record<string, unknown> }>).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<string, unknown>
|
||||
expect(models.gone).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, unknown> }
|
||||
|
||||
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<string, unknown> }>,
|
||||
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<string, unknown> }) => {
|
||||
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<string, { models: Record<string, unknown> }>
|
||||
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<string, { variants?: Record<string, unknown> }> }
|
||||
>
|
||||
).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<string, { models: Record<string, unknown> }>).myprovider
|
||||
.models
|
||||
expect(Object.values(models).every((v) => v !== null)).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("fetchProviderData", () => {
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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"]),
|
||||
)
|
||||
: {},
|
||||
|
||||
@@ -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<typeof v> => !!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<typeof v> => !!v && !v.disabled), // kilocode_change - drop null delete sentinels
|
||||
(v) => omit(v, ["disabled"]),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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<string, { models: Record<string, unknown> }>).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<string, { models: Record<string, { variants: Record<string, unknown> }> }>
|
||||
).myprovider.models["model-1"].variants
|
||||
expect(variants.high).toBeDefined()
|
||||
expect("low" in variants).toBe(false)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user