diff --git a/.changeset/fix-custom-provider-variant-validation.md b/.changeset/fix-custom-provider-variant-validation.md new file mode 100644 index 00000000000..f05024c2968 --- /dev/null +++ b/.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 diff --git a/packages/kilo-vscode/eslint.config.mjs b/packages/kilo-vscode/eslint.config.mjs index a95ef16cd33..f07e7b327e6 100644 --- a/packages/kilo-vscode/eslint.config.mjs +++ b/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] }, diff --git a/packages/kilo-vscode/tests/unit/custom-provider-dialog-validate.test.ts b/packages/kilo-vscode/tests/unit/custom-provider-dialog-validate.test.ts new file mode 100644 index 00000000000..337dd77a144 --- /dev/null +++ b/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(), + } +} + +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 + 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 + expect(saved.variants).toEqual({ eco: { enable_thinking: true, reasoningEffort: "low" } }) + }) +}) diff --git a/packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderDialog.tsx b/packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderDialog.tsx index 1267d9a014e..390225adc45 100644 --- a/packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderDialog.tsx +++ b/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 - /** 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() - 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() - 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 = {} - 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 = { 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() - 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 } diff --git a/packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderValidation.ts b/packages/kilo-vscode/webview-ui/src/components/settings/CustomProviderValidation.ts new file mode 100644 index 00000000000..1438e707225 --- /dev/null +++ b/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 + +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 + /** 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 } + models: Record + } + } +} + +const PROVIDER_ID = /^[a-z0-9][a-z0-9-_]*$/ +const OPENAI_COMPATIBLE = "@ai-sdk/openai-compatible" + +function checkVariant(v: VariantEntry, seen: Set, 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, 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() + const variants = m.reasoning ? m.variants.map((v) => checkVariant(v, seen, t)) : [] + return { id: idErr, name: nameErr, variants } +} + +function checkHeader(h: HeaderRow, seenKeys: Set, 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, 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] { + const cfg: Record = {} + 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] { + const ventries = m.reasoning ? m.variants.filter((v) => v.name.trim()).map(serializeVariant) : [] + const entry: Record = { 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() + 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() + 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)), + }, + }, + } +}