diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 78ed114..8065489 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -641,6 +641,11 @@ func setConfigValue(cfg *Config, key, value string) error { switch key { case "provider": + if value != "" { + if err := validateProviderName(value); err != nil { + return err + } + } if cfg.Provider != value { cfg.Model = "" } @@ -995,33 +1000,35 @@ func ensureModelInList(models []string, model string) []string { } func setProviderValue(cfg *Config, key, value string) error { - parts := strings.SplitN(key, ".", 3) - if len(parts) != 3 || parts[1] == "" || parts[2] == "" { + dot := strings.LastIndex(key, ".") + if dot <= len("providers.") || dot == len(key)-1 { return fmt.Errorf("invalid provider key %q: expected providers..", key) } - if _, isPreset := llm.LookupProvider(parts[1]); !isPreset { - return setCustomProviderField(cfg, parts[1], parts[2], key, value) + name, field := key[len("providers."):dot], key[dot+1:] + if _, isPreset := llm.LookupProvider(name); !isPreset { + return setCustomProviderField(cfg, name, field, key, value) + } + entry := cfg.Providers[name] + if err := applyProviderField(name, &entry, field, key, value); err != nil { + return err } if cfg.Providers == nil { cfg.Providers = make(map[string]ProviderEntry) } - entry := cfg.Providers[parts[1]] - if err := applyProviderField(parts[1], &entry, parts[2], key, value); err != nil { - return err - } - cfg.Providers[parts[1]] = entry + cfg.Providers[name] = entry return nil } func setCustomProviderValue(cfg *Config, key, value string) error { - parts := strings.SplitN(key, ".", 3) - if len(parts) != 3 || parts[1] == "" || parts[2] == "" { + dot := strings.LastIndex(key, ".") + if dot <= len("custom_providers.") || dot == len(key)-1 { return fmt.Errorf("invalid custom provider key %q: expected custom_providers..", key) } - if preset, isPreset := llm.LookupProvider(parts[1]); isPreset { - return fmt.Errorf("custom provider name %q conflicts with a preset provider; use providers.%s.%s to configure the preset or choose a different custom provider name", parts[1], preset.Name, parts[2]) + name, field := key[len("custom_providers."):dot], key[dot+1:] + if preset, isPreset := llm.LookupProvider(name); isPreset { + return fmt.Errorf("custom provider name %q conflicts with a preset provider; use providers.%s.%s to configure the preset or choose a different custom provider name", name, preset.Name, field) } - return setCustomProviderField(cfg, parts[1], parts[2], key, value) + return setCustomProviderField(cfg, name, field, key, value) } func isAuxiliaryProviderField(field string) bool { @@ -1033,18 +1040,33 @@ func isAuxiliaryProviderField(field string) bool { } } +func validateProviderName(name string) error { + for _, segment := range strings.Split(name, ".") { + if strings.TrimSpace(segment) == "" { + return fmt.Errorf("invalid provider name %q: dot-separated segments must not be empty or whitespace", name) + } + } + return nil +} + func setCustomProviderField(cfg *Config, name, field, key, value string) error { + if err := validateProviderName(name); err != nil { + return fmt.Errorf("%s: %w", key, err) + } + if _, exists := cfg.CustomProviders[name]; strings.Contains(name, ".") && !exists { + return fmt.Errorf("provider %q is not configured for key %q; create or select it first:\n ocr config set provider %s", name, key, name) + } if _, exists := cfg.CustomProviders[name]; isAuxiliaryProviderField(field) && !exists { providerKey := strings.TrimSuffix(key, "."+field) return fmt.Errorf("provider %q is not configured; set a core field first (protocol is required for every custom provider):\n ocr config set %s.protocol ", name, providerKey) } - if cfg.CustomProviders == nil { - cfg.CustomProviders = make(map[string]ProviderEntry) - } entry := cfg.CustomProviders[name] if err := applyProviderField(name, &entry, field, key, value); err != nil { return err } + if cfg.CustomProviders == nil { + cfg.CustomProviders = make(map[string]ProviderEntry) + } cfg.CustomProviders[name] = entry return nil } diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 4ebb675..618d5ac 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -220,6 +220,118 @@ func TestSetConfigValueProviderEntryNonPresetWritesCustomProvider(t *testing.T) } } +func TestSetConfigValueDottedProviderName(t *testing.T) { + fields := []struct { + field string + value string + }{ + {"url", "https://example.invalid/v1"}, + {"protocol", llm.ProtocolAnthropicBedrock}, + {"model", "some-model"}, + {"api_key", "test-key"}, + {"api_key_cmd", "secret-tool lookup service x"}, + {"models", `["some-model","other-model"]`}, + {"auth_header", "authorization"}, + {"timeout_sec", "120"}, + {"extra_body", `{"temperature":0.2}`}, + {"extra_headers", "X-Gateway=test"}, + {"retry_codes", "400,403"}, + {"aws_region", "us-west-2"}, + {"aws_profile", "test-profile"}, + } + want := ProviderEntry{ + URL: "https://example.invalid/v1", + Protocol: llm.ProtocolAnthropicBedrock, + Model: "some-model", + APIKey: "test-key", + APIKeyCmd: "secret-tool lookup service x", + Models: []string{"some-model", "other-model"}, + AuthHeader: "authorization", + TimeoutSec: 120, + ExtraBody: map[string]any{"temperature": 0.2}, + ExtraHeaders: map[string]string{"X-Gateway": "test"}, + RetryCodes: []int{400, 403}, + AWSRegion: "us-west-2", + AWSProfile: "test-profile", + } + for _, prefix := range []string{"providers", "custom_providers"} { + for _, name := range []string{"mimo-v2.6-flash", "openai.gateway.v2", "foo.api_key"} { + t.Run(prefix+"."+name, func(t *testing.T) { + cfg := &Config{} + if err := setConfigValue(cfg, "provider", name); err != nil { + t.Fatal(err) + } + for _, tt := range fields { + key := prefix + "." + name + "." + tt.field + if err := setConfigValue(cfg, key, tt.value); err != nil { + t.Fatalf("setConfigValue(%q): %v", key, err) + } + } + if !reflect.DeepEqual(cfg.CustomProviders, map[string]ProviderEntry{name: want}) { + t.Errorf("CustomProviders = %#v, want only %q with %#v", cfg.CustomProviders, name, want) + } + if len(cfg.Providers) != 0 || cfg.Provider != name { + t.Errorf("Providers = %#v, Provider = %q", cfg.Providers, cfg.Provider) + } + }) + } + } +} + +func TestSetConfigValueRejectsInvalidProviderNames(t *testing.T) { + for _, name := range []string{" ", ".a", "a.", "a..b", "a. .b", "anthropic."} { + for _, key := range []string{"provider", "providers." + name + ".url", "custom_providers." + name + ".url"} { + t.Run(key+"/"+name, func(t *testing.T) { + cfg := &Config{Provider: "openai", Model: "saved-model", CustomProviders: map[string]ProviderEntry{name: {}}} + before, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + value := "https://example.invalid/v1" + if key == "provider" { + value = name + } + if err := setConfigValue(cfg, key, value); err == nil || !strings.Contains(err.Error(), "invalid provider name") { + t.Fatalf("setConfigValue(%q, %q) = %v, want invalid provider name", key, value, err) + } + after, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatalf("configuration changed after rejected write: %s", after) + } + }) + } + } +} + +func TestSetConfigValueDottedProviderRequiresExistingEntry(t *testing.T) { + for _, prefix := range []string{"providers", "custom_providers"} { + for _, name := range []string{"mimo-v2.6-flash", "foo.api_key", "openai.extra_body"} { + key := prefix + "." + name + ".model" + t.Run(key, func(t *testing.T) { + cfg := &Config{Provider: "openai", Model: "saved-model"} + want := *cfg + err := setConfigValue(cfg, key, "some-model") + if err == nil || !strings.Contains(err.Error(), key) || !strings.Contains(err.Error(), "ocr config set provider "+name) { + t.Fatalf("setConfigValue(%q) = %v, want explicit creation guidance", key, err) + } + if !reflect.DeepEqual(*cfg, want) { + t.Fatalf("configuration changed after rejected write: %#v", cfg) + } + cfg.CustomProviders = map[string]ProviderEntry{name: {}} + if err := setConfigValue(cfg, key, "some-model"); err != nil { + t.Fatal(err) + } + if cfg.Provider != "openai" || cfg.CustomProviders[name].Model != "some-model" { + t.Fatalf("unexpected configuration: %#v", cfg) + } + }) + } + } +} + func TestSetConfigValueProviderEntryModelsJSON(t *testing.T) { cfg := &Config{} @@ -364,10 +476,15 @@ func TestSetConfigValueProviderEntryInvalidKey(t *testing.T) { } func TestSetConfigValueProviderEntryInvalidPath(t *testing.T) { - cfg := &Config{} - - if err := setConfigValue(cfg, "providers.anthropic", "value"); err == nil { - t.Fatal("expected error for incomplete provider path") + for _, prefix := range []string{"providers", "custom_providers"} { + for _, suffix := range []string{"", "anthropic", ".url", "anthropic.", "mimo-v2.6-flash."} { + key := prefix + "." + suffix + t.Run(key, func(t *testing.T) { + if err := setConfigValue(&Config{}, key, "value"); err == nil || !strings.Contains(err.Error(), "invalid") { + t.Fatalf("setConfigValue(%q) = %v, want invalid path error", key, err) + } + }) + } } } diff --git a/cmd/opencodereview/main_test.go b/cmd/opencodereview/main_test.go index b7a16cf..c89da5d 100644 --- a/cmd/opencodereview/main_test.go +++ b/cmd/opencodereview/main_test.go @@ -4,12 +4,79 @@ package main import ( + "bytes" "os" "os/exec" "strings" "testing" ) +func TestRunConfigSetProviderNames(t *testing.T) { + if os.Getenv("OCR_TEST_CONFIG_SET") == "1" { + rootCmd.SetArgs(os.Args[3:]) + os.Exit(run()) + } + setTestHome(t, t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + if err := saveConfig(configPath, &Config{Provider: "openai", Model: "saved-model"}); err != nil { + t.Fatal(err) + } + tests := []struct { + key string + value string + code int + }{ + {"providers.openai.extra_body.model", "some-model", 1}, + {"custom_providers.openai.extra_body.model", "some-model", 1}, + {"providers.openai..url", "https://example.invalid/v1", 1}, + {"providers..a.url", "https://example.invalid/v1", 1}, + {"providers. .url", "https://example.invalid/v1", 1}, + {"custom_providers.anthropic..url", "https://example.invalid/v1", 1}, + {"provider", ".gateway", 1}, + {"provider", "gateway.", 1}, + {"provider", "gateway..v2", 1}, + {"provider", " ", 1}, + {"provider", "mimo-v2.6-flash", 0}, + {"providers.mimo-v2.6-flash.url", "https://example.invalid/v1", 0}, + {"custom_providers.mimo-v2.6-flash.protocol", "openai", 0}, + {"provider", "foo.api_key", 0}, + {"custom_providers.foo.api_key.model", "some-model", 0}, + } + for _, tt := range tests { + t.Run(tt.key+"/"+tt.value, func(t *testing.T) { + before, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + cmd := exec.Command(os.Args[0], "-test.run=^TestRunConfigSetProviderNames$", "--", "config", "set", tt.key, tt.value) + cmd.Env = append(os.Environ(), "OCR_TEST_CONFIG_SET=1", "OCR_NO_UPDATE=1", "OCR_ENABLE_TELEMETRY=0") + output, err := cmd.CombinedOutput() + if cmd.ProcessState == nil || cmd.ProcessState.ExitCode() != tt.code { + t.Fatalf("exit error = %v, want code %d; output:\n%s", err, tt.code, output) + } + after, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if tt.code != 0 && !bytes.Equal(before, after) { + t.Fatalf("configuration changed after rejected write: %s", after) + } + }) + } + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatal(err) + } + if cfg.CustomProviders["mimo-v2.6-flash"].URL != "https://example.invalid/v1" || + cfg.CustomProviders["mimo-v2.6-flash"].Protocol != "openai" || + cfg.CustomProviders["foo.api_key"].Model != "some-model" { + t.Fatalf("dotted provider fields were not persisted: %#v", cfg.CustomProviders) + } +} + func TestRunFlushesTelemetryOnError(t *testing.T) { if os.Getenv("OCR_TEST_RUN_ERROR") == "1" { rootCmd.SetArgs([]string{"--invalid-flag"}) diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 75ad999..59f4670 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -158,6 +158,9 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU if result.provider == "" { return fmt.Errorf("provider name is required") } + if err := validateProviderName(result.provider); err != nil { + return err + } model := result.resolvedModel() if model == "" { return fmt.Errorf("model is required") diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index 70b0599..e861aa8 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -289,6 +289,21 @@ func TestApplyCustomProviderConfig_MissingProvider(t *testing.T) { } } +func TestApplyCustomProviderConfigInvalidName(t *testing.T) { + for _, name := range []string{" ", ".gateway", "gateway.", "gateway..v2"} { + t.Run(name, func(t *testing.T) { + cfg := &Config{Provider: "openai", Model: "saved-model"} + err := applyCustomProviderConfig(filepath.Join(t.TempDir(), "config.json"), cfg, providerTUIResult{provider: name, model: "some-model"}) + if err == nil { + t.Fatal("expected invalid provider name error") + } + if cfg.Provider != "openai" || cfg.Model != "saved-model" || cfg.CustomProviders != nil { + t.Fatalf("configuration changed after rejected write: %#v", cfg) + } + }) + } +} + func TestApplyCustomProviderConfig_MissingModel(t *testing.T) { err := applyCustomProviderConfig("", &Config{}, providerTUIResult{provider: "p", model: ""}) if err == nil { diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index a7a1a5c..02c0b05 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -1246,6 +1246,11 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { m.cpStep = cpStepName return m, m.cpNameInput.Focus() } + if err := validateProviderName(r.provider); err != nil { + m.formError = err.Error() + m.cpStep = cpStepName + return m, m.cpNameInput.Focus() + } if m.customProviderNameTaken(r.provider) { m.formError = fmt.Sprintf(`Provider "%s" already exists`, r.provider) m.cpStep = cpStepName diff --git a/cmd/opencodereview/provider_tui_persist_test.go b/cmd/opencodereview/provider_tui_persist_test.go index 8174309..b9b38d1 100644 --- a/cmd/opencodereview/provider_tui_persist_test.go +++ b/cmd/opencodereview/provider_tui_persist_test.go @@ -184,6 +184,22 @@ func TestApplyCreateCustomProvider(t *testing.T) { return m } + for _, name := range []string{".gateway", "gateway.", "gateway..v2"} { + t.Run("invalid name "+name, func(t *testing.T) { + cfg := &Config{Provider: "openai", Model: "saved-model"} + m := setup(t, cfg, filepath.Join(t.TempDir(), "config.json")) + m.cpNameInput.SetValue(name) + out, _ := m.applyCreateCustomProvider() + got := out.(providerTUIModel) + if got.formError == "" || got.savedInSession || got.cpStep != cpStepName { + t.Fatalf("invalid name was not rejected: formError=%q, saved=%v, step=%v", got.formError, got.savedInSession, got.cpStep) + } + if cfg.Provider != "openai" || cfg.Model != "saved-model" || cfg.CustomProviders != nil { + t.Fatalf("configuration changed after rejected write: %#v", cfg) + } + }) + } + t.Run("nil config guard", func(t *testing.T) { m := setup(t, &Config{}, filepath.Join(t.TempDir(), "c.json")) m.existingCfg = nil diff --git a/extensions/frontend/src/shared/__tests__/providers.test.ts b/extensions/frontend/src/shared/__tests__/providers.test.ts index 5546d4b..4a74bcb 100644 --- a/extensions/frontend/src/shared/__tests__/providers.test.ts +++ b/extensions/frontend/src/shared/__tests__/providers.test.ts @@ -3,10 +3,18 @@ import { PROVIDER_PRESETS as generatedPresets } from '../providers.generated'; import { isPresetProvider, lookupPreset, mergeModelLists, PROVIDER_PRESETS, usesAmbientAuth } from '../providers'; -import { buildOfficialSaveEntries, detectInitialTab, isConfigReady } from '../configUtils'; +import { buildOfficialSaveEntries, detectInitialTab, isConfigReady, isValidProviderName } from '../configUtils'; import { OcrConfig } from '../types'; describe('generated provider presets', () => { + it.each(['my-gateway', 'mimo-v2.6-flash', 'foo.api_key'])('accepts provider name %s', (name) => { + expect(isValidProviderName(name)).toBe(true); + }); + + it.each(['', ' ', '.a', 'a.', 'a..b', 'a. .b'])('rejects provider name %s', (name) => { + expect(isValidProviderName(name)).toBe(false); + }); + it('keeps providers sorted by name with the existing initial provider', () => { const names = PROVIDER_PRESETS.map((preset) => preset.name); expect(names).toEqual([...names].sort()); diff --git a/extensions/frontend/src/shared/configUtils.ts b/extensions/frontend/src/shared/configUtils.ts index d3e9266..d438a3c 100644 --- a/extensions/frontend/src/shared/configUtils.ts +++ b/extensions/frontend/src/shared/configUtils.ts @@ -18,6 +18,10 @@ export interface ConfigEntry { value: string; } +export function isValidProviderName(name: string): boolean { + return name.split('.').every((segment) => segment.trim() !== ''); +} + export function detectInitialTab(config: OcrConfig | null): ProviderTab { if (!config) return 'official'; if (config.provider) { @@ -120,15 +124,15 @@ export function buildCustomCreateSaveEntries(params: { authHeader: string; }): ConfigEntry[] { const entries: ConfigEntry[] = [ + { key: 'provider', value: params.name.trim() }, { key: `custom_providers.${params.name}.protocol`, value: params.protocol }, { key: `custom_providers.${params.name}.url`, value: params.url.trim() }, { key: `custom_providers.${params.name}.model`, value: params.model.trim() }, { key: `custom_providers.${params.name}.api_key`, value: params.apiKey.trim() }, - { key: 'provider', value: params.name.trim() }, ]; const models = params.models.trim(); if (models) { - entries.splice(3, 0, { key: `custom_providers.${params.name}.models`, value: models }); + entries.splice(4, 0, { key: `custom_providers.${params.name}.models`, value: models }); } if (params.authHeader.trim()) { entries.push({ key: `custom_providers.${params.name}.auth_header`, value: params.authHeader.trim() }); diff --git a/extensions/frontend/src/webview/views/ConfigView.tsx b/extensions/frontend/src/webview/views/ConfigView.tsx index 8d2a85a..c86860c 100644 --- a/extensions/frontend/src/webview/views/ConfigView.tsx +++ b/extensions/frontend/src/webview/views/ConfigView.tsx @@ -3,7 +3,7 @@ import { useEffect, useMemo, useState } from 'preact/hooks'; import type { ComponentChildren } from 'preact'; -import { ConfigEntry, ConfigPanelFocus, ProviderTab, buildCustomCreateSaveEntries, buildCustomUpdateSaveEntries, buildOfficialSaveEntries, describeActiveProvider, detectInitialTab, isConfigReady, listCustomProviderNames } from '../../shared/configUtils'; +import { ConfigEntry, ConfigPanelFocus, ProviderTab, buildCustomCreateSaveEntries, buildCustomUpdateSaveEntries, buildOfficialSaveEntries, describeActiveProvider, detectInitialTab, isConfigReady, isValidProviderName, listCustomProviderNames } from '../../shared/configUtils'; import { mergeModelLists, PROVIDER_PRESETS, usesAmbientAuth } from '../../shared/providers'; import { EnvCheckResult, LogLine, OcrConfig } from '../../shared/types'; import { CliStatus, ConnTest } from '../configStore'; @@ -450,8 +450,8 @@ function CustomForm({ const [apiKeyTouched, setApiKeyTouched] = useState(false); const [authHeader, setAuthHeader] = useState(entry?.authHeader ?? ''); - const canSaveCreate = name.trim() !== '' && url.trim() !== '' && model.trim() !== '' && apiKey.trim() !== ''; - const canSaveEdit = !isCreate && name.trim() !== '' && url.trim() !== '' && model.trim() !== '' + const canSaveCreate = isValidProviderName(name) && url.trim() !== '' && model.trim() !== '' && apiKey.trim() !== ''; + const canSaveEdit = !isCreate && isValidProviderName(name) && url.trim() !== '' && model.trim() !== '' && Boolean(entry?.apiKey || apiKey.trim()); const canSave = isCreate ? canSaveCreate : canSaveEdit; diff --git a/extensions/idea/src/main/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraft.kt b/extensions/idea/src/main/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraft.kt index 322059c..a62cc73 100644 --- a/extensions/idea/src/main/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraft.kt +++ b/extensions/idea/src/main/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraft.kt @@ -113,16 +113,22 @@ private fun RawConfig.setProviderEntryField( } } -private fun RawConfig.setCustomProviderField(name: String, field: String, value: String) = +private fun isValidProviderName(name: String): Boolean = name.split('.').all { it.isNotBlank() } + +private fun RawConfig.setCustomProviderField(name: String, field: String, value: String) { + if (!isValidProviderName(name)) return + if ('.' in name && (this["custom_providers"] as? JsonObject)?.containsKey(name) != true) return setProviderEntryField("custom_providers", name, field, value) +} /** `providers..`: if name is a built-in provider, write to providers; otherwise write to custom_providers. */ private fun RawConfig.setProviderValue(key: String, value: String) { - val parts = key.split('.') - if (parts.size != 3) return - val name = parts[1] - val field = parts[2] - if (isPresetProvider(name)) { + val prefix = if (key.startsWith("providers.")) "providers." else "custom_providers." + val dot = key.lastIndexOf('.') + if (dot <= prefix.length || dot == key.length - 1) return + val name = key.substring(prefix.length, dot) + val field = key.substring(dot + 1) + if (prefix == "providers." && isPresetProvider(name)) { // Consistent with isPresetProvider / ConfigUtils: preset keys are stored canonically lowercased, so "OpenAI" // is not judged preset yet written under a capitalized key that later lookups would miss. setProviderEntryField("providers", name.trim().lowercase(Locale.ROOT), field, value) @@ -145,18 +151,14 @@ private fun RawConfig.setLlmField(field: String, element: JsonElement) { } private fun RawConfig.setConfigValue(key: String, value: String) { - if (key.startsWith("providers.")) { + if (key.startsWith("providers.") || key.startsWith("custom_providers.")) { setProviderValue(key, value) return } - if (key.startsWith("custom_providers.")) { - val parts = key.split('.') - if (parts.size == 3) setCustomProviderField(parts[1], parts[2], value) - return - } when (key) { "provider" -> { + if (value.isNotEmpty() && !isValidProviderName(value)) return // Preset providers are stored canonically lowercased, matching the providers bucket keys; custom providers keep the user's original name. val providerName = if (isPresetProvider(value)) value.trim().lowercase(Locale.ROOT) else value // Clear the top-level model when the provider changes: the old model usually does not belong to the new provider diff --git a/extensions/idea/src/test/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraftTest.kt b/extensions/idea/src/test/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraftTest.kt index a3249df..880865e 100644 --- a/extensions/idea/src/test/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraftTest.kt +++ b/extensions/idea/src/test/kotlin/com/alibaba/opencodereview/idea/services/ConfigDraftTest.kt @@ -14,6 +14,58 @@ import kotlin.test.assertTrue class ConfigDraftTest { + @Test + fun `explicitly created dotted providers retain all draft fields`() { + for (prefix in listOf("providers", "custom_providers")) { + for (name in listOf("mimo-v2.6-flash", "foo.api_key", "openai.extra_body")) { + val d = draft( + "{}", + "provider" to name, + "$prefix.$name.protocol" to "openai", + "$prefix.$name.url" to "https://example.invalid/v1", + "$prefix.$name.model" to "some-model", + "$prefix.$name.api_key" to "test-key", + "$prefix.$name.models" to "some-model,other-model", + "$prefix.$name.auth_header" to "authorization", + ) + assertEquals(name, d.str("provider")) + val expected = parseRawConfig( + """{"protocol":"openai","url":"https://example.invalid/v1","model":"some-model","api_key":"test-key","models":["some-model","other-model"],"auth_header":"authorization"}""", + ) + assertEquals(JsonObject(expected), d.obj("custom_providers", name)) + assertNull(d.obj("providers")) + } + } + } + + @Test + fun `dotted field writes require an existing provider without changing the active provider`() { + for (prefix in listOf("providers", "custom_providers")) { + for (name in listOf("mimo-v2.6-flash", "foo.api_key", "openai.extra_body")) { + val base = parseRawConfig("""{"provider":"openai","model":"saved-model"}""") + val entries = listOf(ConfigEntry("$prefix.$name.model", "some-model")) + assertEquals(base, applyConfigEntries(base, entries)) + val existing = parseRawConfig("""{"provider":"openai","custom_providers":{"$name":{}}}""") + val d = applyConfigEntries(existing, entries) + assertEquals("openai", d.str("provider")) + assertEquals("some-model", d.str("custom_providers", name, "model")) + assertEquals(JsonObject(emptyMap()), existing.obj("custom_providers", name)) + } + } + } + + @Test + fun `invalid provider names leave the draft unchanged`() { + for (name in listOf(" ", ".a", "a.", "a..b", "a. .b", "anthropic.")) { + val base = parseRawConfig("""{"provider":"openai","model":"saved-model","custom_providers":{"$name":{}}}""") + assertEquals(base, applyConfigEntries(base, listOf(ConfigEntry("provider", name)))) + for (prefix in listOf("providers", "custom_providers")) { + val entries = listOf(ConfigEntry("$prefix.$name.url", "https://example.invalid/v1")) + assertEquals(base, applyConfigEntries(base, entries)) + } + } + } + private fun draft(base: String, vararg entries: Pair): RawConfig = applyConfigEntries(parseRawConfig(base), entries.map { ConfigEntry(it.first, it.second) }) diff --git a/extensions/vscode/src/extension/services/__tests__/configDraft.test.ts b/extensions/vscode/src/extension/services/__tests__/configDraft.test.ts index 898a0a3..e0274ca 100644 --- a/extensions/vscode/src/extension/services/__tests__/configDraft.test.ts +++ b/extensions/vscode/src/extension/services/__tests__/configDraft.test.ts @@ -2,8 +2,48 @@ // Copyright 2026 alibaba/open-code-review Contributors import { applyConfigEntries } from '../configDraft'; +import { buildCustomCreateSaveEntries } from '@shared/configUtils'; describe('applyConfigEntries', () => { + it.each(['mimo-v2.6-flash', 'foo.api_key', 'openai.extra_body'])('creates %s before applying form fields', (name) => { + const entries = buildCustomCreateSaveEntries({ + name, protocol: 'openai', url: 'https://example.invalid/v1', model: 'some-model', + models: 'some-model,other-model', apiKey: 'test-key', authHeader: 'authorization', + }); + expect(entries[0]).toEqual({ key: 'provider', value: name }); + const base = { provider: 'openai', model: 'saved-model' }; + const draft = applyConfigEntries(base, entries); + expect(draft.provider).toBe(name); + expect(draft.custom_providers).toEqual({ + [name]: { + protocol: 'openai', url: 'https://example.invalid/v1', model: 'some-model', + models: ['some-model', 'other-model'], api_key: 'test-key', auth_header: 'authorization', + }, + }); + expect(base).toEqual({ provider: 'openai', model: 'saved-model' }); + }); + + it.each(['providers', 'custom_providers'])('requires existing dotted names for %s', (prefix) => { + const base = { provider: 'openai', model: 'saved-model' }; + for (const name of ['mimo-v2.6-flash', 'foo.api_key', 'openai.extra_body']) { + const entry = { key: `${prefix}.${name}.model`, value: 'some-model' }; + expect(applyConfigEntries(base, [entry])).toEqual(base); + const existing = { ...base, custom_providers: { [name]: {} } }; + const draft = applyConfigEntries(existing, [entry]); + expect(draft.provider).toBe('openai'); + expect(draft.custom_providers).toEqual({ [name]: { model: 'some-model' } }); + expect(existing.custom_providers[name]).toEqual({}); + } + }); + + it.each([' ', '.a', 'a.', 'a..b', 'a. .b', 'anthropic.'])('rejects invalid name %s', (name) => { + const base = { provider: 'openai', model: 'saved-model', custom_providers: { [name]: {} } }; + expect(applyConfigEntries(base, [{ key: 'provider', value: name }])).toEqual(base); + for (const prefix of ['providers', 'custom_providers']) { + expect(applyConfigEntries(base, [{ key: `${prefix}.${name}.url`, value: 'https://example.invalid/v1' }])).toEqual(base); + } + }); + it('merges built-in provider entries', () => { const draft = applyConfigEntries({}, [ { key: 'provider', value: 'anthropic' }, diff --git a/extensions/vscode/src/extension/services/configDraft.ts b/extensions/vscode/src/extension/services/configDraft.ts index e3e593d..4c31b8a 100644 --- a/extensions/vscode/src/extension/services/configDraft.ts +++ b/extensions/vscode/src/extension/services/configDraft.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 alibaba/open-code-review Contributors -import { ConfigEntry } from '@shared/configUtils'; +import { ConfigEntry, isValidProviderName } from '@shared/configUtils'; import { isPresetProvider } from '@shared/providers'; type RawProviderEntry = Record; @@ -57,6 +57,8 @@ function applyProviderField(entry: RawProviderEntry, field: string, value: strin } function setCustomProviderField(cfg: RawConfig, name: string, field: string, value: string): void { + if (!isValidProviderName(name)) return; + if (name.includes('.') && !Object.prototype.hasOwnProperty.call(cfg.custom_providers ?? {}, name)) return; if (!cfg.custom_providers) cfg.custom_providers = {}; const entry = { ...(cfg.custom_providers[name] ?? {}) }; applyProviderField(entry, field, value); @@ -64,11 +66,12 @@ function setCustomProviderField(cfg: RawConfig, name: string, field: string, val } function setProviderValue(cfg: RawConfig, key: string, value: string): void { - const parts = key.split('.'); - if (parts.length !== 3) return; - const name = parts[1]; - const field = parts[2]; - if (isPresetProvider(name)) { + const prefix = key.startsWith('providers.') ? 'providers.' : 'custom_providers.'; + const dot = key.lastIndexOf('.'); + if (dot <= prefix.length || dot === key.length - 1) return; + const name = key.slice(prefix.length, dot); + const field = key.slice(dot + 1); + if (prefix === 'providers.' && isPresetProvider(name)) { if (!cfg.providers) cfg.providers = {}; const entry = { ...(cfg.providers[name] ?? {}) }; applyProviderField(entry, field, value); @@ -79,18 +82,14 @@ function setProviderValue(cfg: RawConfig, key: string, value: string): void { } function setConfigValue(cfg: RawConfig, key: string, value: string): void { - if (key.startsWith('providers.')) { + if (key.startsWith('providers.') || key.startsWith('custom_providers.')) { setProviderValue(cfg, key, value); return; } - if (key.startsWith('custom_providers.')) { - const parts = key.split('.'); - if (parts.length === 3) setCustomProviderField(cfg, parts[1], parts[2], value); - return; - } switch (key) { case 'provider': + if (value && !isValidProviderName(value)) return; if (cfg.provider !== value) cfg.model = ''; cfg.provider = value; if (isPresetProvider(value)) { diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 4d5dbf6..6115ac6 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -163,6 +163,14 @@ rather than accepted and ignored. ### Custom providers +Names such as `mimo-v2.6-flash` and `foo.api_key` are supported. Before +setting fields on a new name containing dots, create and select it with +`ocr config set provider `; this changes the active provider. Existing +dotted entries can be edited without selecting them, through either +`providers..` or `custom_providers..`. Names must +not start or end with a dot, contain consecutive dots, or have empty or +whitespace-only segments. Dot-free names still support implicit creation. + Any provider name not in the table above is treated as custom and must supply at least `url` and `protocol` (`protocol` is `anthropic`, `openai`, `openai-responses`, or `anthropic-bedrock`): diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 5943ba2..2ae44c0 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -160,6 +160,13 @@ Model: claude-sonnet-5 ### カスタム provider +`mimo-v2.6-flash` や `foo.api_key` のような名前を使用できます。ドットを含む新しい +名前のフィールドを設定する前に、`ocr config set provider ` で作成して選択してください。 +この操作は現在の provider を変更します。既存のエントリは選択せずに +`providers..` または `custom_providers..` で編集できます。 +名前の先頭・末尾のドット、連続したドット、空または空白のみの区切り部分は使用できません。 +ドットを含まない名前は引き続きフィールドの設定時に自動作成されます。 + 上記の表にない provider 名はすべてカスタムとみなされ、少なくとも `url` と `protocol` を指定する必要があります(`protocol` は `anthropic`、`openai`、 `openai-responses`、または `anthropic-bedrock`)。 diff --git a/pages/src/content/docs/ko/configuration.md b/pages/src/content/docs/ko/configuration.md index a3905eb..2aac659 100644 --- a/pages/src/content/docs/ko/configuration.md +++ b/pages/src/content/docs/ko/configuration.md @@ -139,6 +139,8 @@ Bedrock은 `llm.protocol`이나 `OCR_LLM_PROTOCOL`로는 사용할 수 **없습 ### 커스텀 프로바이더 {#custom-providers} +`mimo-v2.6-flash`, `foo.api_key`처럼 점이 포함된 이름을 사용할 수 있습니다. 새로운 이름에 점이 있다면 필드를 설정하기 전에 `ocr config set provider `으로 생성하고 선택하세요. 이 명령은 현재 프로바이더를 변경합니다. 기존 항목은 선택하지 않고도 `providers..` 또는 `custom_providers..`로 수정할 수 있습니다. 이름의 처음이나 끝에 점이 있거나, 점이 연속되거나, 구분된 부분이 비어 있거나 공백으로만 구성된 이름은 허용되지 않습니다. 점이 없는 이름은 필드 설정 시 계속 자동으로 생성됩니다. + 위 표에 없는 프로바이더 이름은 커스텀으로 취급하며 최소한 `url`과 `protocol`을 지정해야 합니다(`protocol`은 `anthropic`, `openai`, `openai-responses`, `anthropic-bedrock` 중 하나): ```bash diff --git a/pages/src/content/docs/ru/configuration.md b/pages/src/content/docs/ru/configuration.md index e35b1de..a9f05dd 100644 --- a/pages/src/content/docs/ru/configuration.md +++ b/pages/src/content/docs/ru/configuration.md @@ -169,6 +169,15 @@ Model: claude-sonnet-5 ### Пользовательские провайдеры +Поддерживаются имена вида `mimo-v2.6-flash` и `foo.api_key`. Прежде чем +задавать поля нового имени с точками, создайте и выберите его командой +`ocr config set provider `; она меняет активного провайдера. +Существующие записи можно редактировать без выбора через +`providers..` или `custom_providers..`. +Имя не должно начинаться или заканчиваться точкой, содержать несколько +точек подряд, пустые части или части только из пробелов. Имена без точек +по-прежнему создаются неявно при настройке полей. + Любое имя провайдера, которого нет в таблице выше, считается пользовательским. Для него необходимо задать как минимум `url` и `protocol` (`protocol` может принимать значения `anthropic`, `openai`, diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index 5acc38d..230f088 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -153,6 +153,12 @@ URL 加一个 token,没有地方放区域或 profile,而 bedrock 这两个 ### 自定义 provider +支持 `mimo-v2.6-flash` 和 `foo.api_key` 这样的名称。为新的含点名称设置字段前, +请先运行 `ocr config set provider ` 创建并选中它;这会更改当前 provider。 +已有的含点条目无需选中即可通过 `providers..` 或 +`custom_providers..` 编辑。名称不能以点开头或结尾、包含连续的点, +也不能包含空白或仅由空白字符组成的分段。不含点的名称仍支持隐式创建。 + 任何不在上表中的 provider 名都视为自定义,至少要提供 `url` 和 `protocol` (`protocol` 取 `anthropic`、`openai`、`openai-responses` 或 `anthropic-bedrock`):