diff --git a/README.md b/README.md index b50bc2f..574def5 100644 --- a/README.md +++ b/README.md @@ -343,6 +343,7 @@ See the [`examples/`](./examples/) directory for integration examples: | `ocr config provider` | — | Interactive provider setup (built-in, custom, or manual) | | `ocr config model` | — | Interactive model selection for the active provider | | `ocr config set ` | — | Set configuration values | +| `ocr config unset custom_providers.` | — | Delete a custom provider | | `ocr llm test` | — | Test LLM connectivity | | `ocr llm providers` | — | List built-in LLM providers | | `ocr viewer` | `ocr v` | Launch WebUI session viewer on `localhost:5483` | @@ -376,6 +377,9 @@ ocr config provider ocr config model ocr llm providers +# Delete a custom provider +ocr config unset custom_providers.my-gateway + # Preview which files will be reviewed (no LLM calls) ocr review --preview ocr review -c abc123 -p diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 6f2c3b1..ccd3afe 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -47,6 +47,8 @@ func runConfig(args []string) error { switch action.subCmd { case "set": return runConfigSet(action.key, action.value) + case "unset": + return runConfigUnset(action.key) default: return fmt.Errorf("unknown config sub-command: %s", action.subCmd) } @@ -80,6 +82,68 @@ func runConfigSet(key, value string) error { return nil } +func runConfigUnset(key string) error { + parts := strings.SplitN(key, ".", 2) + if len(parts) != 2 || parts[0] != "custom_providers" || parts[1] == "" { + return fmt.Errorf("unset only supports custom_providers.") + } + name := parts[1] + + configPath, err := defaultConfigPath() + if err != nil { + return err + } + + return unsetCustomProvider(configPath, name) +} + +func unsetCustomProvider(configPath, name string) error { + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + return fmt.Errorf("load config: %w", err) + } + + wasActive, err := deleteCustomProvider(cfg, name) + if err != nil { + return err + } + + if err := saveConfig(configPath, cfg); err != nil { + return err + } + + fmt.Printf("Deleted custom provider %q.\n", name) + if wasActive { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: active provider was deleted; 'provider' and 'model' have been cleared.\n") + fmt.Fprintf(os.Stderr, "[ocr] Run 'ocr config provider' to select a new provider.\n") + } + return nil +} + +// deleteCustomProvider removes a custom provider from cfg in memory. +// Returns true if the deleted provider was the active one. +func deleteCustomProvider(cfg *Config, name string) (bool, error) { + if cfg.CustomProviders == nil { + return false, fmt.Errorf("custom provider %q not found", name) + } + if _, exists := cfg.CustomProviders[name]; !exists { + return false, fmt.Errorf("custom provider %q not found", name) + } + + wasActive := cfg.Provider == name + delete(cfg.CustomProviders, name) + if len(cfg.CustomProviders) == 0 { + cfg.CustomProviders = nil + } + + if wasActive { + cfg.Provider = "" + cfg.Model = "" + } + + return wasActive, nil +} + // ProviderEntry holds per-provider configuration in the providers map. type ProviderEntry struct { APIKey string `json:"api_key,omitempty"` diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 2c8df5b..875b2f6 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1,6 +1,8 @@ package main -import "testing" +import ( + "testing" +) func TestSetConfigValueAuthHeaderNormalizesKnownValues(t *testing.T) { cfg := &Config{} @@ -218,3 +220,122 @@ func TestSetConfigValueModelWithCustomProvider(t *testing.T) { t.Errorf("top-level Model = %q, want empty (should write to custom provider entry)", cfg.Model) } } + +// --- unset tests --- + +func TestParseConfigArgsUnset(t *testing.T) { + action, err := parseConfigArgs([]string{"unset", "custom_providers.my-gateway"}) + if err != nil { + t.Fatalf("parseConfigArgs: %v", err) + } + if action.subCmd != "unset" { + t.Errorf("subCmd = %q, want %q", action.subCmd, "unset") + } + if action.key != "custom_providers.my-gateway" { + t.Errorf("key = %q, want %q", action.key, "custom_providers.my-gateway") + } +} + +func TestParseConfigArgsUnsetMissingKey(t *testing.T) { + _, err := parseConfigArgs([]string{"unset"}) + if err == nil { + t.Fatal("expected error for missing key") + } +} + +func TestUnsetCustomProvider(t *testing.T) { + dir := t.TempDir() + configPath := dir + "/config.json" + + cfg := &Config{ + Provider: "anthropic", + CustomProviders: map[string]ProviderEntry{ + "my-gateway": {URL: "https://gw.example.com/v1", Protocol: "openai", Model: "llama-3"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + if err := unsetCustomProvider(configPath, "my-gateway"); err != nil { + t.Fatalf("unsetCustomProvider: %v", err) + } + + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.CustomProviders != nil { + t.Errorf("CustomProviders should be nil after deleting the only entry, got %v", cfg.CustomProviders) + } + if cfg.Provider != "anthropic" { + t.Errorf("Provider = %q, want %q (should be untouched)", cfg.Provider, "anthropic") + } +} + +func TestUnsetActiveCustomProvider(t *testing.T) { + dir := t.TempDir() + configPath := dir + "/config.json" + + cfg := &Config{ + Provider: "my-gateway", + Model: "fallback-model", + CustomProviders: map[string]ProviderEntry{ + "my-gateway": {URL: "https://gw.example.com/v1", Protocol: "openai", Model: "llama-3"}, + "other-gateway": {URL: "https://other.example.com/v1", Protocol: "openai", Model: "other-model"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + if err := unsetCustomProvider(configPath, "my-gateway"); err != nil { + t.Fatalf("unsetCustomProvider: %v", err) + } + + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.Provider != "" { + t.Errorf("Provider = %q, want empty after deleting active provider", cfg.Provider) + } + if cfg.Model != "" { + t.Errorf("Model = %q, want empty after deleting active provider", cfg.Model) + } + if _, exists := cfg.CustomProviders["my-gateway"]; exists { + t.Error("my-gateway should have been deleted") + } + if _, exists := cfg.CustomProviders["other-gateway"]; !exists { + t.Error("other-gateway should still exist") + } +} + +func TestUnsetInvalidKey(t *testing.T) { + tests := []struct { + name string + wantErr bool + }{ + {"my-gateway", false}, + {"nonexistent", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + configPath := dir + "/config.json" + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "my-gateway": {URL: "https://gw.example.com/v1"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + err := unsetCustomProvider(configPath, tt.name) + if (err != nil) != tt.wantErr { + t.Errorf("unsetCustomProvider(%q): err=%v, wantErr=%v", tt.name, err, tt.wantErr) + } + }) + } +} diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 6c3d4f0..b5fc56a 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -233,7 +233,7 @@ Flags: // --- config subcommand --- type configAction struct { - subCmd string // "set" + subCmd string // "set", "unset" key string value string } @@ -254,8 +254,16 @@ func parseConfigArgs(args []string) (configAction, error) { key: args[1], value: args[2], }, nil + case "unset": + if len(args) < 2 { + return configAction{}, fmt.Errorf("usage: ocr config unset custom_providers.\ne.g., ocr config unset custom_providers.my-gateway") + } + return configAction{ + subCmd: "unset", + key: args[1], + }, nil default: - return configAction{}, fmt.Errorf("unknown config sub-command: %s\nAvailable: set, provider, model", subCmd) + return configAction{}, fmt.Errorf("unknown config sub-command: %s\nAvailable: set, unset, provider, model", subCmd) } } @@ -264,8 +272,9 @@ func printConfigUsage() { Usage: ocr config set - ocr config provider Interactive provider setup - ocr config model Interactive model selection + ocr config unset custom_providers. Delete a custom provider + ocr config provider Interactive provider setup + ocr config model Interactive model selection Examples: # Provider setup (interactive) @@ -287,6 +296,9 @@ Examples: ocr config set custom_providers.my-gateway.models '["llama-3-70b","llama-3-8b"]' ocr config set custom_providers.my-gateway.api_key "$MY_API_KEY" + # Delete a custom provider + ocr config unset custom_providers.my-gateway + # Legacy endpoint configuration ocr config set llm.url https://xx/v1/openai/chat/completions ocr config set llm.auth_token xxxxxxxxxx diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 5f3a545..9e29394 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -30,7 +30,22 @@ func runConfigProvider() error { } final := finalModel.(providerTUIModel) + + if len(final.deletedProviders) > 0 { + clearedActive, err := applyProviderDeletions(configPath, cfg, final.deletedProviders) + if err != nil { + return err + } + if clearedActive && !final.confirmed { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: active provider was deleted; 'provider' and 'model' have been cleared.\n") + fmt.Fprintf(os.Stderr, "[ocr] Run 'ocr config provider' to select a new provider.\n") + } + } + if !final.confirmed { + if len(final.deletedProviders) > 0 { + return nil + } fmt.Println("Cancelled.") return nil } @@ -48,6 +63,25 @@ func runConfigProvider() error { return applyOfficialProviderConfig(configPath, cfg, result) } +func applyProviderDeletions(configPath string, cfg *Config, names []string) (bool, error) { + clearedActive := false + for _, name := range names { + wasActive, err := deleteCustomProvider(cfg, name) + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] skip delete %q: %v\n", name, err) + continue + } + if wasActive { + clearedActive = true + } + fmt.Printf("Deleted custom provider %q.\n", name) + } + if err := saveConfig(configPath, cfg); err != nil { + return false, err + } + return clearedActive, nil +} + func applyManualConfig(configPath string, cfg *Config, result providerTUIResult) error { if result.url == "" { return fmt.Errorf("URL is required for manual configuration") diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 820cb7e..52e9493 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -111,6 +111,12 @@ type providerTUIModel struct { existingCfg *Config confirmed bool cancelled bool + + // --- delete confirmation --- + confirmingDelete bool + deleteTargetIdx int + deleteTargetName string + deletedProviders []string } func collectCustomProviders(cfg *Config) []customProviderListItem { @@ -370,6 +376,10 @@ func (m providerTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.updateManualForm(key, msg) } + if m.step == stepProvider && m.confirmingDelete { + return m.updateDeleteConfirm(key) + } + switch key { case "ctrl+c": m.cancelled = true @@ -413,6 +423,14 @@ func (m providerTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.activeTab = (m.activeTab + 1) % tabCount } return m, nil + + case "d": + if m.step == stepProvider && m.activeTab == tabCustom && !m.creatingCustom && m.customIdx < len(m.customProviders) { + m.confirmingDelete = true + m.deleteTargetIdx = m.customIdx + m.deleteTargetName = m.customProviders[m.customIdx].name + } + return m, nil } default: @@ -648,6 +666,33 @@ func (m providerTUIModel) updateManualForm(key string, msg tea.KeyPressMsg) (tea } } +func (m providerTUIModel) updateDeleteConfirm(key string) (tea.Model, tea.Cmd) { + switch key { + case "y", "Y": + if m.deleteTargetIdx < 0 || m.deleteTargetIdx >= len(m.customProviders) { + m.confirmingDelete = false + return m, nil + } + m.deletedProviders = append(m.deletedProviders, m.deleteTargetName) + newList := make([]customProviderListItem, 0, len(m.customProviders)-1) + newList = append(newList, m.customProviders[:m.deleteTargetIdx]...) + newList = append(newList, m.customProviders[m.deleteTargetIdx+1:]...) + m.customProviders = newList + if m.customIdx >= len(m.customProviders) && m.customIdx > 0 { + m.customIdx = len(m.customProviders) - 1 + } + m.confirmingDelete = false + return m, nil + case "n", "N", "esc": + m.confirmingDelete = false + return m, nil + case "ctrl+c": + m.cancelled = true + return m, tea.Quit + } + return m, nil +} + func (m providerTUIModel) handleManualFormEnter() (tea.Model, tea.Cmd) { switch m.manualStep { case manualStepURL: @@ -968,6 +1013,10 @@ func (m providerTUIModel) viewProvider(s *strings.Builder) { s.WriteString("\n") if m.creatingCustom || m.inManualForm { s.WriteString(tuiHelpStyle.Render(" Enter Confirm · Esc Back")) + } else if m.confirmingDelete { + s.WriteString(tuiHelpStyle.Render(" y Confirm · n/Esc Cancel")) + } else if m.activeTab == tabCustom && m.customIdx < len(m.customProviders) { + s.WriteString(tuiHelpStyle.Render(" Enter Select · d Delete · Tab/Arrow Navigate · Esc Cancel")) } else { s.WriteString(tuiHelpStyle.Render(" Enter to select · Tab/Arrow keys to navigate · Esc to cancel")) } @@ -1034,6 +1083,19 @@ func (m providerTUIModel) viewCustomTab(s *strings.Builder) { s.WriteString(cursor + tuiDimStyle.Render(addLabel)) } s.WriteString("\n") + + if m.confirmingDelete { + s.WriteString("\n") + prompt := fmt.Sprintf(" Delete %q?", m.deleteTargetName) + // existingCfg is the config snapshot from TUI startup; it reflects + // the on-disk active provider, not any in-session selection changes. + if m.existingCfg != nil && m.existingCfg.Provider == m.deleteTargetName { + prompt += " This is the active provider." + } + prompt += " (y/n)" + s.WriteString(tuiSelectedItemStyle.Render(prompt)) + s.WriteString("\n") + } } func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) { diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index a0e774a..cb54b83 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -533,3 +533,166 @@ func TestCollectCustomProviders_SortedByName(t *testing.T) { t.Errorf("second = %q, want %q", result[1].name, "zzz-provider") } } + +// --- Delete custom provider tests --- + +func dKey() tea.KeyPressMsg { + return tea.KeyPressMsg{Code: 'd'} +} + +func yKey() tea.KeyPressMsg { + return tea.KeyPressMsg{Code: 'y'} +} + +func nKey() tea.KeyPressMsg { + return tea.KeyPressMsg{Code: 'n'} +} + +func TestProviderTUI_DeleteCustomProvider(t *testing.T) { + cfg := &Config{ + Provider: "anthropic", + CustomProviders: map[string]ProviderEntry{ + "my-llm": {URL: "https://custom.api/v1", Protocol: "openai", Model: "custom-model"}, + }, + } + m := newProviderTUI(cfg) + + // Switch to custom tab + result, _ := m.Update(rightKey()) + m2 := result.(providerTUIModel) + if m2.activeTab != tabCustom { + t.Fatalf("tab = %d, want %d", m2.activeTab, tabCustom) + } + + // Select the existing provider (index 0), press d + m2.customIdx = 0 + result, _ = m2.Update(dKey()) + m3 := result.(providerTUIModel) + if !m3.confirmingDelete { + t.Fatal("pressing d should set confirmingDelete = true") + } + if m3.deleteTargetName != "my-llm" { + t.Errorf("deleteTargetName = %q, want %q", m3.deleteTargetName, "my-llm") + } + + // Confirm with y + result, _ = m3.Update(yKey()) + m4 := result.(providerTUIModel) + if m4.confirmingDelete { + t.Error("confirmingDelete should be false after y") + } + if len(m4.deletedProviders) != 1 || m4.deletedProviders[0] != "my-llm" { + t.Errorf("deletedProviders = %v, want [my-llm]", m4.deletedProviders) + } + if len(m4.customProviders) != 0 { + t.Errorf("customProviders length = %d, want 0", len(m4.customProviders)) + } +} + +func TestProviderTUI_DeleteCustomProviderCancel(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "my-llm": {URL: "https://custom.api/v1", Protocol: "openai", Model: "custom-model"}, + }, + } + m := newProviderTUI(cfg) + + // Switch to custom tab, select provider, press d + result, _ := m.Update(rightKey()) + m2 := result.(providerTUIModel) + m2.customIdx = 0 + result, _ = m2.Update(dKey()) + m3 := result.(providerTUIModel) + if !m3.confirmingDelete { + t.Fatal("should be confirming delete") + } + + // Cancel with n + result, _ = m3.Update(nKey()) + m4 := result.(providerTUIModel) + if m4.confirmingDelete { + t.Error("confirmingDelete should be false after n") + } + if len(m4.deletedProviders) != 0 { + t.Error("deletedProviders should be empty after cancel") + } + if len(m4.customProviders) != 1 { + t.Error("customProviders should still have 1 entry after cancel") + } +} + +func TestProviderTUI_DeleteOnAddOptionIgnored(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "my-llm": {URL: "https://custom.api/v1", Protocol: "openai"}, + }, + } + m := newProviderTUI(cfg) + + // Switch to custom tab + result, _ := m.Update(rightKey()) + m2 := result.(providerTUIModel) + + // Move to "Add" option (index 1, since there's 1 provider) + m2.customIdx = len(m2.customProviders) + result, _ = m2.Update(dKey()) + m3 := result.(providerTUIModel) + if m3.confirmingDelete { + t.Error("pressing d on Add option should not trigger delete confirmation") + } +} + +func TestProviderTUI_DeleteActiveCustomProvider(t *testing.T) { + cfg := &Config{ + Provider: "my-llm", + CustomProviders: map[string]ProviderEntry{ + "my-llm": {URL: "https://custom.api/v1", Protocol: "openai", Model: "custom-model"}, + }, + } + m := newProviderTUI(cfg) + + // Should auto-select custom tab with active provider + if m.activeTab != tabCustom { + t.Fatalf("should auto-select custom tab, got %d", m.activeTab) + } + + // Press d on the active provider + m.customIdx = 0 + result, _ := m.Update(dKey()) + m2 := result.(providerTUIModel) + if !m2.confirmingDelete { + t.Fatal("should be confirming delete") + } + + // Confirm + result, _ = m2.Update(yKey()) + m3 := result.(providerTUIModel) + if len(m3.deletedProviders) != 1 || m3.deletedProviders[0] != "my-llm" { + t.Errorf("deletedProviders = %v, want [my-llm]", m3.deletedProviders) + } +} + +func TestProviderTUI_DeleteEscCancels(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "my-llm": {URL: "https://custom.api/v1", Protocol: "openai"}, + }, + } + m := newProviderTUI(cfg) + + result, _ := m.Update(rightKey()) + m2 := result.(providerTUIModel) + m2.customIdx = 0 + result, _ = m2.Update(dKey()) + m3 := result.(providerTUIModel) + + // Esc should cancel confirmation + result, _ = m3.Update(escKey()) + m4 := result.(providerTUIModel) + if m4.confirmingDelete { + t.Error("Esc should cancel delete confirmation") + } + if len(m4.deletedProviders) != 0 { + t.Error("no providers should be deleted after Esc") + } +}