feat: add ability to delete custom providers from configuration (#189)

* feat: add ability to delete custom providers from configuration

Implements GitHub issue #136.

CLI: add 'ocr config unset custom_providers.<name>' command to delete
a custom provider from config. If the deleted provider is the active
one, clears 'provider' and 'model' fields and prompts the user.

TUI: press 'd' on Custom tab to delete a provider with y/n confirmation.
Shows warning when deleting the active provider. Deletions are persisted
even if the user cancels provider selection afterward.

- Add runConfigUnset() with key validation and active-provider handling
- Add 'unset' case to parseConfigArgs() and printConfigUsage()
- Add delete confirmation state machine to providerTUIModel
- Add applyProviderDeletions() helper in provider_cmd.go
- Add 10 new tests covering CLI parsing, deletion logic, and TUI flows
- Update README with unset command documentation

* fix: address PR review feedback

- Add defensive bounds check for deleteTargetIdx before deletion
- Use explicit slice copy to avoid retaining references (memory leak)
- Update subCmd comment to reflect 'set' and 'unset' values

* refactor: extract shared deleteCustomProvider and improve test coverage

Address PR review feedback from lizhengfeng101:

- Extract deleteCustomProvider() as a pure function shared by both
  runConfigUnset (CLI) and applyProviderDeletions (TUI)
- Extract unsetCustomProvider() accepting configPath for testability
- Add existence check in applyProviderDeletions (skip non-existent providers)
- Rewrite 3 tests to call actual functions instead of inlining logic
- Remove unused strings import from config_cmd_test.go

* style: use [ocr] WARNING prefix for active provider deletion messages

Align with project convention (output.go, flags.go): warnings use
'[ocr] WARNING' prefix and write to stderr instead of stdout.

* fix: address second round of PR review feedback

- Fix test state pollution in TestUnsetInvalidKey: use t.Run sub-tests
  with independent config files per case (#7)
- Log warning instead of silently swallowing errors in
  applyProviderDeletions (#9)
- Add comment explaining existingCfg snapshot assumption in
  viewCustomTab (#10)
- Simplify runConfigUnset error message to single line for consistency
  with project style (#11)
This commit is contained in:
xujiejie 2026-06-23 21:21:23 +08:00 committed by GitHub
parent bb0cf901ed
commit 9d2800ffaf
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 465 additions and 5 deletions

View file

@ -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 <key> <value>` | — | Set configuration values |
| `ocr config unset custom_providers.<name>` | — | 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

View file

@ -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>")
}
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"`

View file

@ -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)
}
})
}
}

View file

@ -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.<name>\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 <key> <value>
ocr config provider Interactive provider setup
ocr config model Interactive model selection
ocr config unset custom_providers.<name> 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

View file

@ -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")

View file

@ -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) {

View file

@ -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")
}
}