Merge pull request #9241 from Kilo-Org/fix/custom-provider-variant-validation

fix(vscode): validate custom provider variant name and show inline error

.changeset/fix-custom-provider-variant-validation.md

@@ -0,0 +1,5 @@
+---
+"kilo-code": patch
+---
+
+Fix custom provider variant name validation showing inline errors correctly when a variant name is left empty

packages/kilo-vscode/eslint.config.mjs

@@ -79,10 +79,6 @@ export default [
     ],
     rules: { complexity: ["error", 27] },
   },
-  {
-    files: ["webview-ui/src/components/settings/CustomProviderDialog.tsx"],
-    rules: { complexity: ["error", 26] },
-  },
   {
     files: ["src/agent-manager/WorktreeStateManager.ts"],
     rules: { complexity: ["error", 24] },

packages/kilo-vscode/tests/unit/custom-provider-dialog-validate.test.ts

@@ -0,0 +1,146 @@
+import { describe, expect, it } from "bun:test"
+import { validateCustomProvider } from "../../webview-ui/src/components/settings/CustomProviderValidation"
+import type { FormState } from "../../webview-ui/src/components/settings/CustomProviderValidation"
+
+// Simple translator that returns the key so tests can assert on key names
+const t = (key: string) => key
+
+function base(): FormState {
+  return {
+    providerID: "my-provider",
+    name: "My Provider",
+    baseURL: "https://example.com/v1",
+    apiKey: "",
+    models: [{ id: "model-1", name: "Model One", reasoning: false, variants: [] }],
+    headers: [],
+    saving: false,
+  }
+}
+
+function args(form: FormState) {
+  return {
+    form,
+    t,
+    editing: false,
+    disabledProviders: [],
+    existingProviderIDs: new Set<string>(),
+  }
+}
+
+describe("validateCustomProvider – variant name validation", () => {
+  it("allows submit when reasoning is enabled with no variants", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeDefined()
+    expect(out.errors.models[0].variants).toEqual([])
+  })
+
+  it("allows submit when reasoning is enabled with a named variant", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    form.models[0].variants = [
+      {
+        name: "fast",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+    ]
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeDefined()
+    expect(out.errors.models[0].variants?.[0]?.name).toBeUndefined()
+  })
+
+  it("blocks submit and reports error when reasoning is enabled with an empty variant name", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    form.models[0].variants = [
+      {
+        name: "",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+    ]
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeUndefined()
+    expect(out.errors.models[0].variants?.[0]?.name).toBe("provider.custom.error.required")
+  })
+
+  it("blocks submit and reports error when reasoning is enabled with a whitespace-only variant name", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    form.models[0].variants = [
+      {
+        name: "   ",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+    ]
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeUndefined()
+    expect(out.errors.models[0].variants?.[0]?.name).toBe("provider.custom.error.required")
+  })
+
+  it("blocks submit and reports duplicate error for two variants with the same name", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    form.models[0].variants = [
+      {
+        name: "fast",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+      {
+        name: "fast",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+    ]
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeUndefined()
+    expect(out.errors.models[0].variants?.[1]?.name).toBe("provider.custom.error.duplicate")
+  })
+
+  it("ignores variants entirely when reasoning is disabled, even if they have empty names", () => {
+    const form = base()
+    form.models[0].reasoning = false
+    form.models[0].variants = [
+      {
+        name: "",
+        enableThinking: undefined,
+        thinking: undefined,
+        reasoningEffort: undefined,
+        chatTemplateArgs: undefined,
+      },
+    ]
+    const out = validateCustomProvider(args(form))
+    // No variant errors produced; form is allowed to submit
+    expect(out.errors.models[0].variants).toEqual([])
+    // Variant is not included in the saved config
+    expect(out.result).toBeDefined()
+    const saved = out.result!.config.models["model-1"] as Record<string, unknown>
+    expect(saved.variants).toBeUndefined()
+  })
+
+  it("persists named variants in the saved config when reasoning is enabled", () => {
+    const form = base()
+    form.models[0].reasoning = true
+    form.models[0].variants = [
+      { name: "eco", enableThinking: true, thinking: undefined, reasoningEffort: "low", chatTemplateArgs: undefined },
+    ]
+    const out = validateCustomProvider(args(form))
+    expect(out.result).toBeDefined()
+    const saved = out.result!.config.models["model-1"] as Record<string, unknown>
+    expect(saved.variants).toEqual({ eco: { enable_thinking: true, reasoningEffort: "low" } })
+  })
+})

packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderDialog.tsx

@@ -7,7 +7,7 @@ import { Spinner } from "@kilocode/kilo-ui/spinner"
 import { TextField } from "@kilocode/kilo-ui/text-field"
 import { showToast } from "@kilocode/kilo-ui/toast"
 import { For, Show, createEffect, createMemo, createSignal, onCleanup } from "solid-js"
-import { createStore } from "solid-js/store"
+import { createStore, reconcile } from "solid-js/store"
 import { useConfig } from "../../context/config"
 import { useLanguage } from "../../context/language"
 import { useProvider } from "../../context/provider"
@@ -22,12 +22,10 @@ import type {
   ModelEntry,
   ReasoningEffortValue,
   ThinkingTypeValue,
-  Translator,
   VariantEntry,
 } from "./CustomProviderModelCard"
-
-const PROVIDER_ID = /^[a-z0-9][a-z0-9-_]*$/
-const OPENAI_COMPATIBLE = "@ai-sdk/openai-compatible"
+import { validateCustomProvider } from "./CustomProviderValidation"
+import type { FormErrors, FormState, HeaderRow } from "./CustomProviderValidation"
 const DEBOUNCE_MS = 500
 const SEARCH_DEBOUNCE_MS = 150
 
@@ -42,183 +40,8 @@ function fuzzy(query: string, target: string) {
   return qi === q.length
 }
 
-type HeaderRow = {
-  key: string
-  value: string
-}
-
-type FormState = {
-  providerID: string
-  name: string
-  baseURL: string
-  apiKey: string
-  models: ModelEntry[]
-  headers: HeaderRow[]
-  saving: boolean
-}
-
-type FormErrors = {
-  providerID: string | undefined
-  name: string | undefined
-  baseURL: string | undefined
-  models: Array<{ id?: string; name?: string; variants?: Array<{ name?: string }> }>
-  headers: Array<{ key?: string; value?: string }>
-}
-
 type FetchedModel = { id: string; name: string }
 
-type ValidateArgs = {
-  form: FormState
-  t: Translator
-  editing: boolean
-  disabledProviders: string[]
-  existingProviderIDs: Set<string>
-  /** Preserved env vars from the existing provider config (edit mode only) */
-  existingEnv?: string[]
-}
-
-function validateCustomProvider(input: ValidateArgs) {
-  const providerID = input.form.providerID.trim()
-  const name = input.form.name.trim()
-  const baseURL = input.form.baseURL.trim()
-  const apiKey = input.form.apiKey.trim()
-
-  const env = apiKey.match(/^\{env:([^}]+)\}$/)?.[1]?.trim()
-  // When editing and apiKey is empty, preserve existing env from the original config
-  const existingEnv = input.editing && !apiKey ? input.existingEnv : undefined
-  const key = apiKey && !env ? apiKey : undefined
-
-  const idError = !providerID
-    ? input.t("provider.custom.error.providerID.required")
-    : !PROVIDER_ID.test(providerID)
-      ? input.t("provider.custom.error.providerID.format")
-      : undefined
-
-  const nameError = !name ? input.t("provider.custom.error.name.required") : undefined
-  const urlError = !baseURL
-    ? input.t("provider.custom.error.baseURL.required")
-    : !/^https?:\/\//.test(baseURL)
-      ? input.t("provider.custom.error.baseURL.format")
-      : undefined
-
-  const disabled = input.disabledProviders.includes(providerID)
-  const existsError = idError
-    ? undefined
-    : input.editing
-      ? undefined
-      : input.existingProviderIDs.has(providerID) && !disabled
-        ? input.t("provider.custom.error.providerID.exists")
-        : undefined
-
-  const seenModels = new Set<string>()
-  const modelErrors = input.form.models.map((m) => {
-    const id = m.id.trim()
-    const modelIdError = !id
-      ? input.t("provider.custom.error.required")
-      : seenModels.has(id)
-        ? input.t("provider.custom.error.duplicate")
-        : (() => {
-            seenModels.add(id)
-            return undefined
-          })()
-    const modelNameError = !m.name.trim() ? input.t("provider.custom.error.required") : undefined
-    const seen = new Set<string>()
-    const verrs = m.reasoning
-      ? m.variants.map((v) => {
-          const n = v.name.trim()
-          const nameError = !n
-            ? input.t("provider.custom.error.required")
-            : seen.has(n)
-              ? input.t("provider.custom.error.duplicate")
-              : (() => {
-                  seen.add(n)
-                  return undefined
-                })()
-          return { name: nameError }
-        })
-      : []
-    return { id: modelIdError, name: modelNameError, variants: verrs }
-  })
-  const modelsValid = modelErrors.every((m) => !m.id && !m.name && m.variants.every((v) => !v.name))
-  const models = Object.fromEntries(
-    input.form.models.map((m) => {
-      const ventries = m.reasoning
-        ? m.variants
-            .filter((v) => v.name.trim())
-            .map((v) => {
-              const cfg: Record<string, unknown> = {}
-              if (v.enableThinking !== undefined) cfg.enable_thinking = v.enableThinking
-              if (v.thinking !== undefined) cfg.thinking = { type: v.thinking }
-              if (v.reasoningEffort !== undefined) cfg.reasoningEffort = v.reasoningEffort
-              if (v.chatTemplateArgs !== undefined) cfg.chat_template_args = { enable_thinking: v.chatTemplateArgs }
-              return [v.name.trim(), cfg]
-            })
-        : []
-      const entry: Record<string, unknown> = { name: m.name.trim() }
-      if (m.reasoning) entry.reasoning = true
-      if (ventries.length > 0) entry.variants = Object.fromEntries(ventries)
-      return [m.id.trim(), entry]
-    }),
-  )
-
-  const seenHeaders = new Set<string>()
-  const headerErrors = input.form.headers.map((h) => {
-    const key = h.key.trim()
-    const value = h.value.trim()
-
-    if (!key && !value) return {}
-    const keyError = !key
-      ? input.t("provider.custom.error.required")
-      : seenHeaders.has(key.toLowerCase())
-        ? input.t("provider.custom.error.duplicate")
-        : (() => {
-            seenHeaders.add(key.toLowerCase())
-            return undefined
-          })()
-    const valueError = !value ? input.t("provider.custom.error.required") : undefined
-    return { key: keyError, value: valueError }
-  })
-  const headersValid = headerErrors.every((h) => !h.key && !h.value)
-  const headers = Object.fromEntries(
-    input.form.headers
-      .map((h) => ({ key: h.key.trim(), value: h.value.trim() }))
-      .filter((h) => !!h.key && !!h.value)
-      .map((h) => [h.key, h.value]),
-  )
-
-  const errors: FormErrors = {
-    providerID: idError ?? existsError,
-    name: nameError,
-    baseURL: urlError,
-    models: modelErrors,
-    headers: headerErrors,
-  }
-
-  const ok = !idError && !existsError && !nameError && !urlError && modelsValid && headersValid
-  if (!ok) return { errors }
-
-  const options = {
-    baseURL,
-    ...(Object.keys(headers).length ? { headers } : {}),
-  }
-
-  return {
-    errors,
-    result: {
-      providerID,
-      name,
-      key,
-      config: {
-        npm: OPENAI_COMPATIBLE,
-        name,
-        ...(env ? { env: [env] } : existingEnv ? { env: existingEnv } : {}),
-        options,
-        models,
-      },
-    },
-  }
-}
-
 export interface CustomProviderDialogProps {
   onBack?: () => void
   /** When set, the dialog opens in edit mode with pre-filled values. */
@@ -545,7 +368,7 @@ const CustomProviderDialog = (props: CustomProviderDialogProps) => {
       existingProviderIDs: new Set(Object.keys(provider.providers())),
       existingEnv: props.existing?.config?.env,
     })
-    setErrors(output.errors)
+    setErrors(reconcile(output.errors))
     return output.result
   }
 

packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderValidation.ts

@@ -0,0 +1,200 @@
+import type { ModelEntry, VariantEntry } from "./CustomProviderModelCard"
+
+type Translator = (key: string, params?: Record<string, string>) => string
+
+export type HeaderRow = {
+  key: string
+  value: string
+}
+
+export type FormState = {
+  providerID: string
+  name: string
+  baseURL: string
+  apiKey: string
+  models: ModelEntry[]
+  headers: HeaderRow[]
+  saving: boolean
+}
+
+export type FormErrors = {
+  providerID: string | undefined
+  name: string | undefined
+  baseURL: string | undefined
+  models: Array<{ id?: string; name?: string; variants?: Array<{ name?: string }> }>
+  headers: Array<{ key?: string; value?: string }>
+}
+
+type ValidateArgs = {
+  form: FormState
+  t: Translator
+  editing: boolean
+  disabledProviders: string[]
+  existingProviderIDs: Set<string>
+  /** Preserved env vars from the existing provider config (edit mode only) */
+  existingEnv?: string[]
+}
+
+type ValidateResult = {
+  errors: FormErrors
+  result?: {
+    providerID: string
+    name: string
+    key: string | undefined
+    config: {
+      npm: string
+      name: string
+      env?: string[]
+      options: { baseURL: string; headers?: Record<string, string> }
+      models: Record<string, unknown>
+    }
+  }
+}
+
+const PROVIDER_ID = /^[a-z0-9][a-z0-9-_]*$/
+const OPENAI_COMPATIBLE = "@ai-sdk/openai-compatible"
+
+function checkVariant(v: VariantEntry, seen: Set<string>, t: Translator) {
+  const n = v.name.trim()
+  if (!n) return { name: t("provider.custom.error.required") }
+  if (seen.has(n)) return { name: t("provider.custom.error.duplicate") }
+  seen.add(n)
+  return { name: undefined }
+}
+
+function checkModel(m: ModelEntry, seenModels: Set<string>, t: Translator) {
+  const id = m.id.trim()
+  let idErr: string | undefined
+  if (!id) idErr = t("provider.custom.error.required")
+  else if (seenModels.has(id)) idErr = t("provider.custom.error.duplicate")
+  else seenModels.add(id)
+
+  const nameErr = !m.name.trim() ? t("provider.custom.error.required") : undefined
+  const seen = new Set<string>()
+  const variants = m.reasoning ? m.variants.map((v) => checkVariant(v, seen, t)) : []
+  return { id: idErr, name: nameErr, variants }
+}
+
+function checkHeader(h: HeaderRow, seenKeys: Set<string>, t: Translator) {
+  const key = h.key.trim()
+  const value = h.value.trim()
+  if (!key && !value) return {}
+
+  let keyErr: string | undefined
+  if (!key) keyErr = t("provider.custom.error.required")
+  else if (seenKeys.has(key.toLowerCase())) keyErr = t("provider.custom.error.duplicate")
+  else seenKeys.add(key.toLowerCase())
+
+  const valueErr = !value ? t("provider.custom.error.required") : undefined
+  return { key: keyErr, value: valueErr }
+}
+
+function checkProviderID(id: string, editing: boolean, disabled: string[], existing: Set<string>, t: Translator) {
+  const idErr = !id
+    ? t("provider.custom.error.providerID.required")
+    : !PROVIDER_ID.test(id)
+      ? t("provider.custom.error.providerID.format")
+      : undefined
+  const existsErr =
+    idErr || editing || !existing.has(id) || disabled.includes(id)
+      ? undefined
+      : t("provider.custom.error.providerID.exists")
+  return { idErr, existsErr }
+}
+
+function serializeVariant(v: VariantEntry): [string, Record<string, unknown>] {
+  const cfg: Record<string, unknown> = {}
+  if (v.enableThinking !== undefined) cfg.enable_thinking = v.enableThinking
+  if (v.thinking !== undefined) cfg.thinking = { type: v.thinking }
+  if (v.reasoningEffort !== undefined) cfg.reasoningEffort = v.reasoningEffort
+  if (v.chatTemplateArgs !== undefined) cfg.chat_template_args = { enable_thinking: v.chatTemplateArgs }
+  return [v.name.trim(), cfg]
+}
+
+function serializeModel(m: ModelEntry): [string, Record<string, unknown>] {
+  const ventries = m.reasoning ? m.variants.filter((v) => v.name.trim()).map(serializeVariant) : []
+  const entry: Record<string, unknown> = { name: m.name.trim() }
+  if (m.reasoning) entry.reasoning = true
+  if (ventries.length > 0) entry.variants = Object.fromEntries(ventries)
+  return [m.id.trim(), entry]
+}
+
+function resolveEnv(rawEnv: string | undefined, savedEnv: string[] | undefined) {
+  if (rawEnv) return { env: [rawEnv] }
+  if (savedEnv) return { env: savedEnv }
+  return {}
+}
+
+export function validateCustomProvider(input: ValidateArgs): ValidateResult {
+  const providerID = input.form.providerID.trim()
+  const name = input.form.name.trim()
+  const baseURL = input.form.baseURL.trim()
+  const apiKey = input.form.apiKey.trim()
+
+  const rawEnv = apiKey.match(/^\{env:([^}]+)\}$/)?.[1]?.trim()
+  // When editing and apiKey is empty, preserve existing env from the original config
+  const savedEnv = input.editing && !apiKey ? input.existingEnv : undefined
+  const key = apiKey && !rawEnv ? apiKey : undefined
+
+  const { idErr, existsErr } = checkProviderID(
+    providerID,
+    input.editing,
+    input.disabledProviders,
+    input.existingProviderIDs,
+    input.t,
+  )
+
+  const nameError = !name ? input.t("provider.custom.error.name.required") : undefined
+  const urlError = !baseURL
+    ? input.t("provider.custom.error.baseURL.required")
+    : !/^https?:\/\//.test(baseURL)
+      ? input.t("provider.custom.error.baseURL.format")
+      : undefined
+
+  const seenModels = new Set<string>()
+  const modelErrors = input.form.models.map((m) => checkModel(m, seenModels, input.t))
+  const modelsValid = modelErrors.every((m) => !m.id && !m.name && m.variants.every((v) => !v.name))
+
+  const seenHeaders = new Set<string>()
+  const headerErrors = input.form.headers.map((h) => checkHeader(h, seenHeaders, input.t))
+  const headersValid = headerErrors.every((h) => !h.key && !h.value)
+
+  const errors: FormErrors = {
+    providerID: idErr ?? existsErr,
+    name: nameError,
+    baseURL: urlError,
+    models: modelErrors,
+    headers: headerErrors,
+  }
+
+  const ok = !idErr && !existsErr && !nameError && !urlError && modelsValid && headersValid
+  if (!ok) return { errors }
+
+  const headers = Object.fromEntries(
+    input.form.headers
+      .map((h) => ({ key: h.key.trim(), value: h.value.trim() }))
+      .filter((h) => !!h.key && !!h.value)
+      .map((h) => [h.key, h.value]),
+  )
+
+  const options = {
+    baseURL,
+    ...(Object.keys(headers).length ? { headers } : {}),
+  }
+
+  return {
+    errors,
+    result: {
+      providerID,
+      name,
+      key,
+      config: {
+        npm: OPENAI_COMPATIBLE,
+        name,
+        ...resolveEnv(rawEnv, savedEnv),
+        options,
+        models: Object.fromEntries(input.form.models.map(serializeModel)),
+      },
+    },
+  }
+}