fix(tui): persist official-tab models and refine saved secret hint (#260)

* fix(tui): persist official-tab models and refine saved secret hint

Persist user-added models to providers.<name>.models on the official tab.
When an API key or auth token is already saved, show a replace hint with a
prefix/suffix fingerprint (skipped for short keys), use a fixed mask placeholder,
and ensure typing or paste replaces the saved value instead of re-saving it.

* fix(tui): model add/delete UX and config wizard hardening
- Add model add/delete in config provider and config model (official + custom)
- Show d Delete only on model rows; green highlight when selected
- Improve Esc cancel text; track savedInSession to avoid misleading messages
- Reload config on save failure; read registry models fresh after reload
- Export llm.ModelListContains; fix config model persist using registry-only check

* fix(tui): defer provider config until confirm and harden API key UX
Only persist provider/model on wizard confirm; keep in-session picks via
sessionModelPick. Validate API key before quit, clear saved keys when emptied,
and improve official env-var hints and custom edit clear behavior.

* feat(tui): show active model suffix on official provider list
Align Official tab with Custom: display (model) next to the active preset
when cfg.Provider matches and a global model is configured.

Author: 5wxwxwxw5

cmd/opencodereview/config_cmd.go

@@ -480,23 +480,13 @@ func ensureModelInList(models []string, model string) []string {
 	if model == "" {
 		return models
 	}
-	if modelListContains(models, model) {
+	if llm.ModelListContains(models, model) {
 		return models
 	}
 	out := append([]string(nil), models...)
 	return append(out, model)
 }
 
-func modelListContains(models []string, target string) bool {
-	target = strings.TrimSpace(target)
-	for _, model := range models {
-		if model == target {
-			return true
-		}
-	}
-	return false
-}
-
 func setProviderValue(cfg *Config, key, value string) error {
 	parts := strings.SplitN(key, ".", 3)
 	if len(parts) != 3 || parts[1] == "" || parts[2] == "" {

cmd/opencodereview/provider_cmd.go

@@ -32,13 +32,9 @@ func runConfigProvider() error {
 	final := finalModel.(providerTUIModel)
 
 	if !final.confirmed {
-		// TUI persists changes (create/edit/model/add/delete) directly to disk
-		// during the session, so the on-disk file is already up to date for any
-		// savedInSession operation. No additional post-TUI apply step is needed.
-		if final.savedInSession {
-			return nil
-		}
-		fmt.Println("Cancelled.")
+		// TUI persists changes during the session; Esc only abandons the final
+		// provider/API-key confirmation step.
+		printWizardCancelled(final.savedInSession, "Configuration changes")
 		return nil
 	}
 
@@ -55,6 +51,16 @@ func runConfigProvider() error {
 	return applyOfficialProviderConfig(configPath, cfg, result)
 }
 
+// printWizardCancelled prints the standard Esc-cancel message for config wizards.
+// changesDescription is a short noun phrase, e.g. "Configuration changes".
+func printWizardCancelled(savedInSession bool, changesDescription string) {
+	if savedInSession {
+		fmt.Printf("Cancelled. (%s made during this session were kept.)\n", changesDescription)
+		return
+	}
+	fmt.Println("Cancelled.")
+}
+
 func applyProviderDeletions(configPath string, cfg *Config, names []string) (bool, error) {
 	clearedActive := false
 	for _, name := range names {
@@ -133,7 +139,8 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 	if result.provider == "" {
 		return fmt.Errorf("provider name is required")
 	}
-	if result.model == "" {
+	model := result.resolvedModel()
+	if model == "" {
 		return fmt.Errorf("model is required")
 	}
 
@@ -142,11 +149,11 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 	}
 
 	entry := cfg.CustomProviders[result.provider]
-	entry.Model = result.model
+	entry.Model = model
 	if len(result.models) > 0 {
 		entry.Models = append([]string(nil), result.models...)
 	}
-	entry.Models = ensureModelInList(entry.Models, result.model)
+	entry.Models = ensureModelInList(entry.Models, model)
 	if result.url != "" {
 		entry.URL = result.url
 	}
@@ -162,14 +169,16 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 	}
 	if result.apiKey != "" {
 		entry.APIKey = result.apiKey
+	} else {
+		entry.APIKey = ""
 	}
 	cfg.CustomProviders[result.provider] = entry
 
 	if !result.isEdit {
 		cfg.Provider = result.provider
-		cfg.Model = result.model
+		cfg.Model = model
 	} else if cfg.Provider == result.provider {
-		cfg.Model = result.model
+		cfg.Model = model
 	}
 
 	if err := saveConfig(configPath, cfg); err != nil {
@@ -182,13 +191,13 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 		} else {
 			fmt.Printf("\nCustom provider %q updated (not currently active).\n", result.provider)
 		}
-		fmt.Printf("Model: %s\n", result.model)
+		fmt.Printf("Model: %s\n", model)
 		fmt.Println("\nTip: run 'ocr config model' to switch model later.")
 		return nil
 	}
 
 	fmt.Printf("\nProvider set to: %s (custom)\n", result.provider)
-	fmt.Printf("Model: %s\n", result.model)
+	fmt.Printf("Model: %s\n", model)
 
 	fmt.Println("\nTesting connection...")
 	if err := runLLMTest(); err != nil {
@@ -202,7 +211,11 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
 }
 
 func applyOfficialProviderConfig(configPath string, cfg *Config, result providerTUIResult) error {
-	if result.provider == "" || result.model == "" {
+	if result.provider == "" {
+		return fmt.Errorf("provider and model are required")
+	}
+	model := result.resolvedModel()
+	if model == "" {
 		return fmt.Errorf("provider and model are required")
 	}
 
@@ -223,27 +236,30 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider
 	}
 
 	entry := cfg.Providers[result.provider]
-	entry.Model = result.model
+	entry.Model = model
 	if len(result.models) > 0 {
 		entry.Models = mergeModelLists(entry.Models, result.models)
 	}
 	if result.apiKey != "" {
 		entry.APIKey = result.apiKey
+	} else {
+		// Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR.
+		entry.APIKey = ""
 	}
 	cfg.Providers[result.provider] = entry
 
 	if cfg.Provider != result.provider {
 		cfg.Model = ""
 	}
 	cfg.Provider = result.provider
-	cfg.Model = result.model
+	cfg.Model = model
 
 	if err := saveConfig(configPath, cfg); err != nil {
 		return err
 	}
 
 	fmt.Printf("\nProvider set to: %s\n", result.provider)
-	fmt.Printf("Model: %s\n", result.model)
+	fmt.Printf("Model: %s\n", model)
 
 	fmt.Println("\nTesting connection...")
 	if err := runLLMTest(); err != nil {
@@ -274,8 +290,10 @@ func runConfigModel() error {
 	currentModel := ""
 	provider := llm.Provider{Name: cfg.Provider, DisplayName: cfg.Provider}
 	isCustom := false
+	registryModels := []string(nil)
 	if preset, isPreset := llm.LookupProvider(cfg.Provider); isPreset {
 		provider = preset
+		registryModels = append([]string(nil), preset.Models...)
 		if entry, ok := cfg.Providers[cfg.Provider]; ok {
 			currentModel = activeModelForProvider(cfg, cfg.Provider, entry)
 			provider.Models = mergeModelLists(provider.Models, entry.Models)
@@ -293,7 +311,15 @@ func runConfigModel() error {
 		provider.Models = mergeModelLists(entry.Models)
 	}
 
-	m := newModelTUI(provider, currentModel)
+	m := newModelTUIConfig(modelTUIConfig{
+		Provider:       provider,
+		CurrentModel:   currentModel,
+		RegistryModels: registryModels,
+		ExistingCfg:    cfg,
+		ConfigPath:     configPath,
+		ProviderName:   cfg.Provider,
+		IsCustom:       isCustom,
+	})
 	p := tea.NewProgram(m)
 	finalModel, err := p.Run()
 	if err != nil {
@@ -302,7 +328,7 @@ func runConfigModel() error {
 
 	final := finalModel.(modelTUIModel)
 	if final.cancelled {
-		fmt.Println("Cancelled.")
+		printWizardCancelled(final.savedInSession, "Model list changes")
 		return nil
 	}
 
@@ -325,7 +351,9 @@ func runConfigModel() error {
 		}
 		entry := cfg.Providers[cfg.Provider]
 		entry.Model = selectedModel
-		if !modelListContains(provider.Models, selectedModel) {
+		// Use registry-only list: provider.Models was captured before the TUI and
+		// may include stale entry.Models from add/delete during the session.
+		if !llm.ModelListContains(registryModels, selectedModel) {
 			entry.Models = ensureModelInList(entry.Models, selectedModel)
 		}
 		cfg.Providers[cfg.Provider] = entry

cmd/opencodereview/provider_cmd_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"encoding/json"
+	"io"
 	"os"
 	"path/filepath"
 	"testing"
@@ -203,3 +204,175 @@ func TestApplyOfficialProviderConfig_MissingFields(t *testing.T) {
 		t.Fatal("expected error for missing provider/model")
 	}
 }
+
+func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
+	t.Setenv("DEEPSEEK_API_KEY", "sk-from-env")
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+	cfg := &Config{
+		Provider: "deepseek",
+		Model:    "deepseek-v4-flash",
+		Providers: map[string]ProviderEntry{
+			"deepseek": {
+				APIKey: "old-saved-key",
+				Model:  "deepseek-v4-flash",
+			},
+		},
+	}
+
+	err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{
+		provider: "deepseek",
+		model:    "deepseek-v4-flash",
+		apiKey:   "",
+	})
+	if err != nil {
+		t.Fatalf("applyOfficialProviderConfig: %v", err)
+	}
+	if got := cfg.Providers["deepseek"].APIKey; got != "" {
+		t.Errorf("in-memory APIKey = %q, want empty", got)
+	}
+	diskCfg, err := loadOrCreateConfig(configPath)
+	if err != nil {
+		t.Fatalf("load config: %v", err)
+	}
+	if got := diskCfg.Providers["deepseek"].APIKey; got != "" {
+		t.Errorf("persisted APIKey = %q, want empty", got)
+	}
+}
+
+func TestApplyCustomProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+	cfg := &Config{
+		Provider: "aaa",
+		Model:    "test",
+		CustomProviders: map[string]ProviderEntry{
+			"aaa": {
+				URL:      "https://example.com/v1",
+				Protocol: "openai",
+				APIKey:   "old-saved-key",
+				Model:    "test",
+				Models:   []string{"test"},
+			},
+		},
+	}
+
+	err := applyCustomProviderConfig(configPath, cfg, providerTUIResult{
+		provider: "aaa",
+		model:    "test",
+		models:   []string{"test"},
+		apiKey:   "",
+		isCustom: true,
+		url:      "https://example.com/v1",
+		protocol: "openai",
+	})
+	if err != nil {
+		t.Fatalf("applyCustomProviderConfig: %v", err)
+	}
+	if got := cfg.CustomProviders["aaa"].APIKey; got != "" {
+		t.Errorf("APIKey = %q, want empty", got)
+	}
+}
+
+func TestProviderTUIResult_ResolvedModel(t *testing.T) {
+	r := providerTUIResult{
+		provider: "baidu-qianfan",
+		model:    "glm-5",
+	}
+	if got := r.resolvedModel(); got != "glm-5" {
+		t.Errorf("resolvedModel() = %q, want glm-5", got)
+	}
+
+	r = providerTUIResult{
+		provider: "baidu-qianfan",
+		sessionModelPick: map[string]string{
+			"baidu-qianfan": "glm-5",
+		},
+	}
+	if got := r.resolvedModel(); got != "glm-5" {
+		t.Errorf("resolvedModel() from session pick = %q, want glm-5", got)
+	}
+
+	r = providerTUIResult{provider: "baidu-qianfan"}
+	if got := r.resolvedModel(); got != "" {
+		t.Errorf("resolvedModel() = %q, want empty", got)
+	}
+}
+
+func TestApplyOfficialProviderConfig_UsesSessionModelPick(t *testing.T) {
+	t.Setenv("QIANFAN_API_KEY", "sk-from-env")
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+	cfg := &Config{
+		Provider: "deepseek",
+		Model:    "deepseek-v4-flash",
+		Providers: map[string]ProviderEntry{
+			"deepseek": {Model: "deepseek-v4-flash"},
+		},
+	}
+
+	err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{
+		provider: "baidu-qianfan",
+		apiKey:   "",
+		sessionModelPick: map[string]string{
+			"baidu-qianfan": "glm-5",
+		},
+	})
+	if err != nil {
+		t.Fatalf("applyOfficialProviderConfig: %v", err)
+	}
+	if cfg.Provider != "baidu-qianfan" {
+		t.Errorf("Provider = %q, want baidu-qianfan", cfg.Provider)
+	}
+	if cfg.Model != "glm-5" {
+		t.Errorf("Model = %q, want glm-5", cfg.Model)
+	}
+}
+
+func TestPrintWizardCancelled(t *testing.T) {
+	tests := []struct {
+		name           string
+		savedInSession bool
+		scope          string
+		want           string
+	}{
+		{
+			name:           "no changes",
+			savedInSession: false,
+			scope:          "Configuration changes",
+			want:           "Cancelled.\n",
+		},
+		{
+			name:           "provider wizard kept changes",
+			savedInSession: true,
+			scope:          "Configuration changes",
+			want:           "Cancelled. (Configuration changes made during this session were kept.)\n",
+		},
+		{
+			name:           "model wizard kept changes",
+			savedInSession: true,
+			scope:          "Model list changes",
+			want:           "Cancelled. (Model list changes made during this session were kept.)\n",
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			old := os.Stdout
+			r, w, err := os.Pipe()
+			if err != nil {
+				t.Fatal(err)
+			}
+			os.Stdout = w
+			printWizardCancelled(tc.savedInSession, tc.scope)
+			w.Close()
+			os.Stdout = old
+			got, err := io.ReadAll(r)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(got) != tc.want {
+				t.Errorf("output = %q, want %q", string(got), tc.want)
+			}
+		})
+	}
+}