From 705471b4c29068dfa5f3598c66fb8a366bc8ea6a Mon Sep 17 00:00:00 2001 From: phantom5099 <1011668688@qq.com> Date: Fri, 17 Apr 2026 22:49:23 +0800 Subject: [PATCH 1/5] =?UTF-8?q?refactor(config):=E5=8E=BB=E6=8E=89env?= =?UTF-8?q?=E6=96=87=E4=BB=B6=E6=AE=8B=E7=95=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/guides/configuration.md | 1 - internal/cli/root.go | 5 +- internal/cli/root_test.go | 43 +- internal/config/env_platform.go | 24 +- internal/config/env_platform_test.go | 6 + internal/config/env_platform_windows.go | 37 +- internal/config/envfile.go | 191 -------- internal/config/envfile_test.go | 300 ------------ internal/config/envvar.go | 56 +++ internal/config/envvar_test.go | 49 ++ internal/config/loader_test.go | 11 +- internal/config/provider_loader.go | 12 +- .../config/state/service_provider_create.go | 283 ++++++++++++ .../state/service_provider_create_test.go | 206 +++++++++ internal/provider/openaicompat/discovery.go | 30 +- .../openaicompat/openaicompat_test.go | 53 +++ internal/tui/bootstrap/builder.go | 1 + internal/tui/bootstrap/builder_test.go | 14 + internal/tui/core/app/app.go | 2 + internal/tui/core/app/commands_test.go | 7 + internal/tui/core/app/update.go | 352 ++++---------- internal/tui/core/app/update_test.go | 431 +++++------------- internal/tui/core/app/view.go | 2 + 23 files changed, 951 insertions(+), 1165 deletions(-) delete mode 100644 internal/config/envfile.go delete mode 100644 internal/config/envfile_test.go create mode 100644 internal/config/envvar.go create mode 100644 internal/config/envvar_test.go create mode 100644 internal/config/state/service_provider_create.go create mode 100644 internal/config/state/service_provider_create_test.go diff --git a/docs/guides/configuration.md b/docs/guides/configuration.md index e78dfbbd..4cf8e1e1 100644 --- a/docs/guides/configuration.md +++ b/docs/guides/configuration.md @@ -13,7 +13,6 @@ - 自动清理旧版 `providers` / `provider_overrides` - 自动兼容 `workdir`、`default_workdir` 等旧字段 -- 自动加载 `.env` ## 配置文件位置 diff --git a/internal/cli/root.go b/internal/cli/root.go index 7ccc856e..efbac23d 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -13,7 +13,6 @@ import ( "github.com/spf13/viper" "neo-code/internal/app" - "neo-code/internal/config" "neo-code/internal/updater" "neo-code/internal/version" ) @@ -115,12 +114,12 @@ func defaultRootProgramLauncher(ctx context.Context, opts app.BootstrapOptions) return err } -// defaultGlobalPreload runs lightweight startup preloads such as persisted env. +// defaultGlobalPreload 负责执行启动前预检查，避免在命令执行前遗漏上下文取消信号。 func defaultGlobalPreload(ctx context.Context) error { if err := ctx.Err(); err != nil { return err } - return config.LoadPersistedEnv("") + return nil } // defaultSilentUpdateCheck 在后台异步检查新版本并缓存退出后提示文案。 diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index ceac8d22..1b6bdd93 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -1425,46 +1425,7 @@ func TestDefaultSilentUpdateCheckSetsSanitizedNotice(t *testing.T) { t.Fatal("expected update notice to be set") } -func TestDefaultGlobalPreloadLoadsPersistedEnv(t *testing.T) { - home := t.TempDir() - t.Setenv("HOME", home) - t.Setenv("USERPROFILE", home) - - envPath := filepath.Join(home, ".neocode", ".env") - if err := os.MkdirAll(filepath.Dir(envPath), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(envPath, []byte("NEOCODE_PRELOAD_KEY=loaded-value\n"), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - - restore := captureEnvForRootTest(t, "NEOCODE_PRELOAD_KEY") - defer restore() - if err := os.Unsetenv("NEOCODE_PRELOAD_KEY"); err != nil { - t.Fatalf("Unsetenv() error = %v", err) - } - - if err := defaultGlobalPreload(context.Background()); err != nil { - t.Fatalf("defaultGlobalPreload() error = %v", err) - } - if got := os.Getenv("NEOCODE_PRELOAD_KEY"); got != "loaded-value" { - t.Fatalf("expected loaded env value, got %q", got) - } -} - -func TestDefaultGlobalPreloadKeepsExistingProcessEnv(t *testing.T) { - home := t.TempDir() - t.Setenv("HOME", home) - t.Setenv("USERPROFILE", home) - - envPath := config.EnvFilePath("") - if err := os.MkdirAll(filepath.Dir(envPath), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(envPath, []byte("NEOCODE_PRELOAD_KEEP=file-value\n"), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - +func TestDefaultGlobalPreloadNoop(t *testing.T) { restore := captureEnvForRootTest(t, "NEOCODE_PRELOAD_KEEP") defer restore() if err := os.Setenv("NEOCODE_PRELOAD_KEEP", "process-value"); err != nil { @@ -1475,7 +1436,7 @@ func TestDefaultGlobalPreloadKeepsExistingProcessEnv(t *testing.T) { t.Fatalf("defaultGlobalPreload() error = %v", err) } if got := os.Getenv("NEOCODE_PRELOAD_KEEP"); got != "process-value" { - t.Fatalf("expected existing process env value, got %q", got) + t.Fatalf("defaultGlobalPreload should not mutate process env, got %q", got) } } diff --git a/internal/config/env_platform.go b/internal/config/env_platform.go index 1573c605..caf66acc 100644 --- a/internal/config/env_platform.go +++ b/internal/config/env_platform.go @@ -2,18 +2,40 @@ package config +import ( + "errors" + "strings" +) + // PersistUserEnvVar persists a key/value pair into user-level environment storage. -// On non-Windows platforms, NeoCode currently relies on .env persistence and process env. +// 非 Windows 平台当前不做系统级持久化，但会统一执行输入校验。 func PersistUserEnvVar(key string, value string) error { + if err := ValidateEnvVarName(strings.TrimSpace(key)); err != nil { + return err + } + if strings.ContainsAny(value, "\r\n") { + return errors.New("config: env value contains newline") + } return nil } // DeleteUserEnvVar 删除用户级环境变量；非 Windows 平台当前无需额外处理。 func DeleteUserEnvVar(key string) error { + if err := ValidateEnvVarName(strings.TrimSpace(key)); err != nil { + return err + } return nil } // LookupUserEnvVar 查询用户级环境变量；非 Windows 平台当前无单独存储，统一返回不存在。 func LookupUserEnvVar(key string) (string, bool, error) { + if err := ValidateEnvVarName(strings.TrimSpace(key)); err != nil { + return "", false, err + } return "", false, nil } + +// SupportsUserEnvPersistence 返回当前平台是否支持用户级环境变量持久化。 +func SupportsUserEnvPersistence() bool { + return false +} diff --git a/internal/config/env_platform_test.go b/internal/config/env_platform_test.go index 287ea4a5..611ebefe 100644 --- a/internal/config/env_platform_test.go +++ b/internal/config/env_platform_test.go @@ -28,3 +28,9 @@ func TestLookupUserEnvVarNoopOnNonWindows(t *testing.T) { t.Fatalf("LookupUserEnvVar() value = %q, want empty", value) } } + +func TestSupportsUserEnvPersistenceOnNonWindows(t *testing.T) { + if SupportsUserEnvPersistence() { + t.Fatal("SupportsUserEnvPersistence() = true, want false") + } +} diff --git a/internal/config/env_platform_windows.go b/internal/config/env_platform_windows.go index df1ea19c..3570f5d2 100644 --- a/internal/config/env_platform_windows.go +++ b/internal/config/env_platform_windows.go @@ -3,7 +3,6 @@ package config import ( - "errors" "fmt" "strings" @@ -15,14 +14,11 @@ const windowsUserEnvironmentKey = `Environment` // PersistUserEnvVar persists key/value into Windows user environment variables. func PersistUserEnvVar(key string, value string) error { normalizedKey := strings.TrimSpace(key) - if normalizedKey == "" { - return errors.New("config: env key is empty") - } - if strings.ContainsAny(normalizedKey, " \t\r\n=") { - return fmt.Errorf("config: env key %q is invalid", normalizedKey) + if err := ValidateEnvVarName(normalizedKey); err != nil { + return err } if strings.ContainsAny(value, "\r\n") { - return errors.New("config: env value contains newline") + return fmt.Errorf("config: env value contains newline") } envKey, _, err := registry.CreateKey(registry.CURRENT_USER, windowsUserEnvironmentKey, registry.SET_VALUE) @@ -40,16 +36,13 @@ func PersistUserEnvVar(key string, value string) error { // DeleteUserEnvVar 删除 Windows 用户级环境变量，不存在时视为成功。 func DeleteUserEnvVar(key string) error { normalizedKey := strings.TrimSpace(key) - if normalizedKey == "" { - return errors.New("config: env key is empty") - } - if strings.ContainsAny(normalizedKey, " \t\r\n=") { - return fmt.Errorf("config: env key %q is invalid", normalizedKey) + if err := ValidateEnvVarName(normalizedKey); err != nil { + return err } envKey, err := registry.OpenKey(registry.CURRENT_USER, windowsUserEnvironmentKey, registry.SET_VALUE) if err != nil { - if errors.Is(err, registry.ErrNotExist) { + if err == registry.ErrNotExist { return nil } return fmt.Errorf("config: open windows user env: %w", err) @@ -57,7 +50,7 @@ func DeleteUserEnvVar(key string) error { defer envKey.Close() if err := envKey.DeleteValue(normalizedKey); err != nil { - if errors.Is(err, registry.ErrNotExist) { + if err == registry.ErrNotExist { return nil } return fmt.Errorf("config: delete windows user env %q: %w", normalizedKey, err) @@ -68,16 +61,13 @@ func DeleteUserEnvVar(key string) error { // LookupUserEnvVar 查询 Windows 用户级环境变量，不存在时返回 exists=false。 func LookupUserEnvVar(key string) (string, bool, error) { normalizedKey := strings.TrimSpace(key) - if normalizedKey == "" { - return "", false, errors.New("config: env key is empty") - } - if strings.ContainsAny(normalizedKey, " \t\r\n=") { - return "", false, fmt.Errorf("config: env key %q is invalid", normalizedKey) + if err := ValidateEnvVarName(normalizedKey); err != nil { + return "", false, err } envKey, err := registry.OpenKey(registry.CURRENT_USER, windowsUserEnvironmentKey, registry.QUERY_VALUE) if err != nil { - if errors.Is(err, registry.ErrNotExist) { + if err == registry.ErrNotExist { return "", false, nil } return "", false, fmt.Errorf("config: open windows user env: %w", err) @@ -86,10 +76,15 @@ func LookupUserEnvVar(key string) (string, bool, error) { value, _, err := envKey.GetStringValue(normalizedKey) if err != nil { - if errors.Is(err, registry.ErrNotExist) { + if err == registry.ErrNotExist { return "", false, nil } return "", false, fmt.Errorf("config: read windows user env %q: %w", normalizedKey, err) } return value, true, nil } + +// SupportsUserEnvPersistence 返回当前平台是否支持用户级环境变量持久化。 +func SupportsUserEnvPersistence() bool { + return true +} diff --git a/internal/config/envfile.go b/internal/config/envfile.go deleted file mode 100644 index b92f9cc7..00000000 --- a/internal/config/envfile.go +++ /dev/null @@ -1,191 +0,0 @@ -package config - -import ( - "errors" - "fmt" - "os" - "path/filepath" - "strconv" - "strings" -) - -const envFileName = ".env" - -// EnvFilePath returns the persisted env file path under the config base dir. -func EnvFilePath(baseDir string) string { - trimmed := strings.TrimSpace(baseDir) - if trimmed == "" { - trimmed = defaultBaseDir() - } - return filepath.Join(trimmed, envFileName) -} - -// PersistEnvVar upserts an env key/value pair into the persisted .env file. -func PersistEnvVar(baseDir string, key string, value string) error { - normalizedKey := strings.TrimSpace(key) - if normalizedKey == "" { - return errors.New("config: env key is empty") - } - if strings.ContainsAny(normalizedKey, " \t\r\n=") { - return fmt.Errorf("config: env key %q is invalid", normalizedKey) - } - if strings.ContainsAny(value, "\r\n") { - return errors.New("config: env value contains newline") - } - - envPath := EnvFilePath(baseDir) - if err := os.MkdirAll(filepath.Dir(envPath), 0o755); err != nil { - return fmt.Errorf("config: create env dir: %w", err) - } - - var lines []string - data, readErr := os.ReadFile(envPath) - switch { - case readErr == nil: - lines = strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") - case os.IsNotExist(readErr): - lines = nil - default: - return fmt.Errorf("config: read env file: %w", readErr) - } - - updated := false - for i := range lines { - currentKey, _, ok := parseEnvAssignment(lines[i]) - if !ok { - continue - } - if currentKey == normalizedKey { - lines[i] = formatEnvAssignment(normalizedKey, value) - updated = true - break - } - } - if !updated { - lines = append(lines, formatEnvAssignment(normalizedKey, value)) - } - - content := strings.Join(lines, "\n") - content = strings.TrimRight(content, "\n") + "\n" - if err := os.WriteFile(envPath, []byte(content), 0o600); err != nil { - return fmt.Errorf("config: write env file: %w", err) - } - return nil -} - -// LoadPersistedEnv loads key/value pairs from persisted .env into process env. -// Existing process env keys are kept as-is. -func LoadPersistedEnv(baseDir string) error { - envPath := EnvFilePath(baseDir) - data, err := os.ReadFile(envPath) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("config: read env file: %w", err) - } - - lines := strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") - for _, line := range lines { - key, value, ok := parseEnvAssignment(line) - if !ok { - continue - } - if _, exists := os.LookupEnv(key); exists { - continue - } - if err := os.Setenv(key, value); err != nil { - return fmt.Errorf("config: set env %q: %w", key, err) - } - } - return nil -} - -// RemovePersistedEnvVar 从持久化 .env 文件中删除指定键；文件不存在时视为成功。 -func RemovePersistedEnvVar(baseDir string, key string) error { - normalizedKey := strings.TrimSpace(key) - if normalizedKey == "" { - return errors.New("config: env key is empty") - } - if strings.ContainsAny(normalizedKey, " \t\r\n=") { - return fmt.Errorf("config: env key %q is invalid", normalizedKey) - } - - envPath := EnvFilePath(baseDir) - data, err := os.ReadFile(envPath) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("config: read env file: %w", err) - } - - lines := strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") - filtered := make([]string, 0, len(lines)) - for _, line := range lines { - currentKey, _, ok := parseEnvAssignment(line) - if ok && currentKey == normalizedKey { - continue - } - filtered = append(filtered, line) - } - - content := strings.Join(filtered, "\n") - content = strings.TrimRight(content, "\n") - if content != "" { - content += "\n" - } - if err := os.WriteFile(envPath, []byte(content), 0o600); err != nil { - return fmt.Errorf("config: write env file: %w", err) - } - return nil -} - -func parseEnvAssignment(line string) (string, string, bool) { - trimmed := strings.TrimSpace(line) - if trimmed == "" || strings.HasPrefix(trimmed, "#") { - return "", "", false - } - if strings.HasPrefix(trimmed, "export ") { - trimmed = strings.TrimSpace(strings.TrimPrefix(trimmed, "export ")) - } - - eq := strings.IndexByte(trimmed, '=') - if eq <= 0 { - return "", "", false - } - - key := strings.TrimSpace(trimmed[:eq]) - if key == "" { - return "", "", false - } - rawValue := strings.TrimSpace(trimmed[eq+1:]) - return key, parseEnvValue(rawValue), true -} - -func parseEnvValue(raw string) string { - if len(raw) >= 2 && raw[0] == '\'' && raw[len(raw)-1] == '\'' { - return raw[1 : len(raw)-1] - } - if len(raw) >= 2 && raw[0] == '"' && raw[len(raw)-1] == '"' { - if unquoted, err := strconv.Unquote(raw); err == nil { - return unquoted - } - return raw[1 : len(raw)-1] - } - return raw -} - -func formatEnvAssignment(key string, value string) string { - return key + "=" + encodeEnvValue(value) -} - -func encodeEnvValue(value string) string { - if value == "" { - return `""` - } - if strings.ContainsAny(value, " \t\r\n#\"'") { - return strconv.Quote(value) - } - return value -} diff --git a/internal/config/envfile_test.go b/internal/config/envfile_test.go deleted file mode 100644 index 9e0e2b9a..00000000 --- a/internal/config/envfile_test.go +++ /dev/null @@ -1,300 +0,0 @@ -package config - -import ( - "os" - "path/filepath" - "strings" - "testing" -) - -func TestPersistEnvVarCreatesAndUpdatesEntry(t *testing.T) { - baseDir := t.TempDir() - path := EnvFilePath(baseDir) - - if err := PersistEnvVar(baseDir, "KIMI_API_KEY", "sk-first"); err != nil { - t.Fatalf("PersistEnvVar() first error = %v", err) - } - if err := PersistEnvVar(baseDir, "KIMI_API_KEY", "sk-second"); err != nil { - t.Fatalf("PersistEnvVar() second error = %v", err) - } - - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("ReadFile(%q) error = %v", path, err) - } - text := string(data) - if strings.Count(text, "KIMI_API_KEY=") != 1 { - t.Fatalf("expected exactly one key line, got %q", text) - } - if !strings.Contains(text, "KIMI_API_KEY=sk-second\n") { - t.Fatalf("expected updated value in env file, got %q", text) - } -} - -func TestPersistEnvVarPreservesOtherLines(t *testing.T) { - baseDir := t.TempDir() - path := EnvFilePath(baseDir) - original := "# comment\nOTHER_KEY=1\n\n" - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(path, []byte(original), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - - if err := PersistEnvVar(baseDir, "NEW_KEY", "value with space"); err != nil { - t.Fatalf("PersistEnvVar() error = %v", err) - } - - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("ReadFile() error = %v", err) - } - text := string(data) - if !strings.Contains(text, "# comment\n") || !strings.Contains(text, "OTHER_KEY=1\n") { - t.Fatalf("expected old lines to be preserved, got %q", text) - } - if !strings.Contains(text, "NEW_KEY=\"value with space\"\n") { - t.Fatalf("expected quoted inserted line, got %q", text) - } -} - -func TestLoadPersistedEnvLoadsMissingKeysOnly(t *testing.T) { - baseDir := t.TempDir() - path := EnvFilePath(baseDir) - content := "EXISTING_KEY=from-file\nNEW_KEY=\"hello world\"\n# ignored\n" - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(path, []byte(content), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - - restoreExisting := captureEnv(t, "EXISTING_KEY") - defer restoreExisting() - restoreNew := captureEnv(t, "NEW_KEY") - defer restoreNew() - - if err := os.Setenv("EXISTING_KEY", "from-process"); err != nil { - t.Fatalf("Setenv() error = %v", err) - } - if err := os.Unsetenv("NEW_KEY"); err != nil { - t.Fatalf("Unsetenv() error = %v", err) - } - - if err := LoadPersistedEnv(baseDir); err != nil { - t.Fatalf("LoadPersistedEnv() error = %v", err) - } - - if got := os.Getenv("EXISTING_KEY"); got != "from-process" { - t.Fatalf("expected EXISTING_KEY to keep process value, got %q", got) - } - if got := os.Getenv("NEW_KEY"); got != "hello world" { - t.Fatalf("expected NEW_KEY loaded from env file, got %q", got) - } -} - -func TestPersistEnvVarRejectsInvalidInput(t *testing.T) { - baseDir := t.TempDir() - if err := PersistEnvVar(baseDir, "", "value"); err == nil { - t.Fatal("expected empty key error") - } - if err := PersistEnvVar(baseDir, "BAD KEY", "value"); err == nil { - t.Fatal("expected invalid key error") - } - if err := PersistEnvVar(baseDir, "KEY", "line1\nline2"); err == nil { - t.Fatal("expected newline value error") - } -} - -func TestRemovePersistedEnvVarRemovesEntryOnly(t *testing.T) { - baseDir := t.TempDir() - path := EnvFilePath(baseDir) - content := "KEEP=1\nREMOVE=2\nKEEP_AGAIN=3\n" - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(path, []byte(content), 0o600); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - - if err := RemovePersistedEnvVar(baseDir, "REMOVE"); err != nil { - t.Fatalf("RemovePersistedEnvVar() error = %v", err) - } - - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("ReadFile() error = %v", err) - } - got := string(data) - if strings.Contains(got, "REMOVE=2") { - t.Fatalf("expected key to be removed, got %q", got) - } - if !strings.Contains(got, "KEEP=1") || !strings.Contains(got, "KEEP_AGAIN=3") { - t.Fatalf("expected other lines preserved, got %q", got) - } -} - -func TestRemovePersistedEnvVarHandlesMissingFileAndInvalidKey(t *testing.T) { - baseDir := t.TempDir() - - if err := RemovePersistedEnvVar(baseDir, "MISSING"); err != nil { - t.Fatalf("expected missing file to be ignored, got %v", err) - } - if err := RemovePersistedEnvVar(baseDir, " "); err == nil { - t.Fatal("expected empty key error") - } - if err := RemovePersistedEnvVar(baseDir, "BAD KEY"); err == nil { - t.Fatal("expected invalid key error") - } -} - -func TestParseEnvAssignmentAndValueVariants(t *testing.T) { - tests := []struct { - line string - wantKey string - wantVal string - wantOkay bool - }{ - {line: "", wantOkay: false}, - {line: "# comment", wantOkay: false}, - {line: "NO_EQUALS", wantOkay: false}, - {line: "export KEY=value", wantKey: "KEY", wantVal: "value", wantOkay: true}, - {line: "KEY='single quoted value'", wantKey: "KEY", wantVal: "single quoted value", wantOkay: true}, - {line: `KEY="line\tvalue"`, wantKey: "KEY", wantVal: "line\tvalue", wantOkay: true}, - {line: `KEY="unterminated`, wantKey: "KEY", wantVal: `"unterminated`, wantOkay: true}, - {line: "SPACED = plain ", wantKey: "SPACED", wantVal: "plain", wantOkay: true}, - } - for _, tt := range tests { - key, val, ok := parseEnvAssignment(tt.line) - if ok != tt.wantOkay { - t.Fatalf("parseEnvAssignment(%q) ok = %v, want %v", tt.line, ok, tt.wantOkay) - } - if !tt.wantOkay { - continue - } - if key != tt.wantKey || val != tt.wantVal { - t.Fatalf("parseEnvAssignment(%q) = (%q,%q), want (%q,%q)", tt.line, key, val, tt.wantKey, tt.wantVal) - } - } -} - -func TestEncodeEnvValue(t *testing.T) { - tests := []struct { - value string - want string - }{ - {value: "", want: `""`}, - {value: "plain", want: "plain"}, - {value: "has space", want: `"has space"`}, - {value: `has"quote`, want: `"has\"quote"`}, - {value: "has#hash", want: `"has#hash"`}, - } - for _, tt := range tests { - if got := encodeEnvValue(tt.value); got != tt.want { - t.Fatalf("encodeEnvValue(%q) = %q, want %q", tt.value, got, tt.want) - } - } -} - -func TestEnvFilePathFallbackToDefaultBaseDir(t *testing.T) { - got := EnvFilePath(" ") - if got != filepath.Join(defaultBaseDir(), envFileName) { - t.Fatalf("EnvFilePath(blank) = %q", got) - } -} - -func TestPersistEnvVarErrorPaths(t *testing.T) { - t.Run("base dir is file", func(t *testing.T) { - tempRoot := t.TempDir() - fileBase := filepath.Join(tempRoot, "not-a-dir") - if err := os.WriteFile(fileBase, []byte("x"), 0o600); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := PersistEnvVar(fileBase, "KEY", "value"); err == nil { - t.Fatal("expected mkdir error") - } - }) - - t.Run("read env file failed", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.MkdirAll(envPath, 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := PersistEnvVar(baseDir, "KEY", "value"); err == nil { - t.Fatal("expected read error when env path is a directory") - } - }) - - t.Run("write env file failed", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.WriteFile(envPath, []byte("KEY=old\n"), 0o400); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := PersistEnvVar(baseDir, "KEY", "new"); err == nil { - t.Fatal("expected write error") - } - }) -} - -func TestLoadPersistedEnvErrorPaths(t *testing.T) { - t.Run("read env file failed", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.MkdirAll(envPath, 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := LoadPersistedEnv(baseDir); err == nil { - t.Fatal("expected read error when env path is a directory") - } - }) - - t.Run("setenv failed on invalid key", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.WriteFile(envPath, []byte("BAD\x00KEY=value\n"), 0o600); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := LoadPersistedEnv(baseDir); err == nil { - t.Fatal("expected setenv error") - } - }) -} - -func TestRemovePersistedEnvVarErrorPaths(t *testing.T) { - t.Run("read env file failed", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.MkdirAll(envPath, 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := RemovePersistedEnvVar(baseDir, "KEY"); err == nil { - t.Fatal("expected read error") - } - }) - - t.Run("write env file failed", func(t *testing.T) { - baseDir := t.TempDir() - envPath := EnvFilePath(baseDir) - if err := os.WriteFile(envPath, []byte("KEY=value\n"), 0o400); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := RemovePersistedEnvVar(baseDir, "KEY"); err == nil { - t.Fatal("expected write error") - } - }) -} - -func captureEnv(t *testing.T, key string) func() { - t.Helper() - value, exists := os.LookupEnv(key) - return func() { - if exists { - _ = os.Setenv(key, value) - return - } - _ = os.Unsetenv(key) - } -} diff --git a/internal/config/envvar.go b/internal/config/envvar.go new file mode 100644 index 00000000..839d03ea --- /dev/null +++ b/internal/config/envvar.go @@ -0,0 +1,56 @@ +package config + +import ( + "errors" + "fmt" + "regexp" + "runtime" + "strings" +) + +var envVarNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + +var protectedEnvVarNames = map[string]struct{}{ + "PATH": {}, + "HOME": {}, + "USERPROFILE": {}, + "SHELL": {}, + "PWD": {}, + "COMSPEC": {}, + "PATHEXT": {}, + "WINDIR": {}, + "SYSTEMROOT": {}, + "TEMP": {}, + "TMP": {}, + "LD_LIBRARY_PATH": {}, + "DYLD_LIBRARY_PATH": {}, + "XDG_CONFIG_HOME": {}, +} + +// ValidateEnvVarName 校验环境变量名格式，避免无效名称进入配置与持久化链路。 +func ValidateEnvVarName(name string) error { + trimmed := strings.TrimSpace(name) + if trimmed == "" { + return errors.New("config: env key is empty") + } + if !envVarNamePattern.MatchString(trimmed) { + return fmt.Errorf("config: env key %q is invalid", trimmed) + } + return nil +} + +// NormalizeEnvVarNameForCompare 统一环境变量名比较键，Windows 上按大小写不敏感处理。 +func NormalizeEnvVarNameForCompare(name string) string { + trimmed := strings.TrimSpace(name) + if runtime.GOOS == "windows" { + return strings.ToUpper(trimmed) + } + return trimmed +} + +// IsProtectedEnvVarName 判断名称是否命中受保护变量，避免覆盖关键系统变量。 +func IsProtectedEnvVarName(name string) bool { + key := strings.ToUpper(strings.TrimSpace(name)) + _, found := protectedEnvVarNames[key] + return found +} diff --git a/internal/config/envvar_test.go b/internal/config/envvar_test.go new file mode 100644 index 00000000..3f17a659 --- /dev/null +++ b/internal/config/envvar_test.go @@ -0,0 +1,49 @@ +package config + +import "testing" + +func TestValidateEnvVarName(t *testing.T) { + t.Parallel() + + valid := []string{"OPENAI_API_KEY", "A", "_TOKEN", "KEY_123"} + for _, name := range valid { + if err := ValidateEnvVarName(name); err != nil { + t.Fatalf("ValidateEnvVarName(%q) error = %v", name, err) + } + } + + invalid := []string{"", " ", "BAD KEY", "1START", "A-B", "A.B", "A=B"} + for _, name := range invalid { + if err := ValidateEnvVarName(name); err == nil { + t.Fatalf("ValidateEnvVarName(%q) expected error", name) + } + } +} + +func TestNormalizeEnvVarNameForCompare(t *testing.T) { + t.Parallel() + + if got := NormalizeEnvVarNameForCompare(" OPENAI_API_KEY "); got == "" { + t.Fatal("NormalizeEnvVarNameForCompare() should not return empty for valid key") + } +} + +func TestIsProtectedEnvVarName(t *testing.T) { + t.Parallel() + + for _, name := range []string{ + "PATH", + "path", + "Home", + "USERPROFILE", + "comspec", + "LD_LIBRARY_PATH", + } { + if !IsProtectedEnvVarName(name) { + t.Fatalf("expected %q to be protected", name) + } + } + if IsProtectedEnvVarName("OPENAI_API_KEY") { + t.Fatal("OPENAI_API_KEY should not be protected") + } +} diff --git a/internal/config/loader_test.go b/internal/config/loader_test.go index b1d126a5..842f6ef8 100644 --- a/internal/config/loader_test.go +++ b/internal/config/loader_test.go @@ -1060,11 +1060,14 @@ func TestLoadCustomProvidersReadDirAndStatErrors(t *testing.T) { defer func() { _ = os.Chmod(providersPath, 0o755) }() providers, err := loadCustomProviders(baseDir) - if err != nil { - t.Fatalf("expected read providers dir fallback, got %v", err) + if err == nil { + t.Fatal("expected read providers dir error") + } + if providers != nil { + t.Fatalf("expected nil providers on read error, got %d", len(providers)) } - if len(providers) != 0 { - t.Fatalf("expected empty providers on read fallback, got %d", len(providers)) + if !strings.Contains(err.Error(), "read providers dir") { + t.Fatalf("expected read providers dir error, got %v", err) } }) diff --git a/internal/config/provider_loader.go b/internal/config/provider_loader.go index 75ed0270..c4911e8b 100644 --- a/internal/config/provider_loader.go +++ b/internal/config/provider_loader.go @@ -65,9 +65,14 @@ func loadCustomProviders(baseDir string) ([]ProviderConfig, error) { entries, err := os.ReadDir(providersDir) if err != nil { if os.IsNotExist(err) { + if _, statErr := os.Stat(providersDir); statErr == nil { + return nil, fmt.Errorf("config: read providers dir: %w", err) + } else if !os.IsNotExist(statErr) { + return nil, fmt.Errorf("config: read providers dir: %w", statErr) + } return nil, nil } - return nil, nil + return nil, fmt.Errorf("config: read providers dir: %w", err) } sort.Slice(entries, func(i, j int) bool { @@ -277,6 +282,11 @@ func DeleteCustomProvider(baseDir string, name string) error { return os.RemoveAll(providersDir) } +// ValidateCustomProviderName 校验 provider 名称，拒绝路径穿越和分隔符语义。 +func ValidateCustomProviderName(name string) error { + return validateCustomProviderName(name) +} + // validateCustomProviderName 校验 provider 名称，拒绝路径穿越和分隔符语义。 func validateCustomProviderName(name string) error { trimmed := strings.TrimSpace(name) diff --git a/internal/config/state/service_provider_create.go b/internal/config/state/service_provider_create.go new file mode 100644 index 00000000..2dfe36a1 --- /dev/null +++ b/internal/config/state/service_provider_create.go @@ -0,0 +1,283 @@ +package state + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "time" + + "neo-code/internal/config" + "neo-code/internal/provider" +) + +var providerCreateFlowMu sync.Mutex +var persistUserEnvVarForCreate = config.PersistUserEnvVar +var deleteUserEnvVarForCreate = config.DeleteUserEnvVar +var lookupUserEnvVarForCreate = config.LookupUserEnvVar + +const providerCreateRollbackReloadTimeout = 3 * time.Second + +// CreateCustomProviderInput 定义新增自定义 Provider 所需的输入参数。 +type CreateCustomProviderInput struct { + Name string + Driver string + BaseURL string + APIKeyEnv string + APIKey string + APIStyle string + DeploymentMode string + APIVersion string +} + +type createCustomProviderNormalizedInput struct { + Name string + Driver string + BaseURL string + APIKeyEnv string + APIKey string + APIStyle string + DeploymentMode string + APIVersion string +} + +type providerConfigSnapshot struct { + Exists bool + Content []byte +} + +// CreateCustomProvider 负责创建/更新自定义 Provider，并统一处理环境变量写入与失败回滚。 +func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomProviderInput) (Selection, error) { + if err := s.validate(); err != nil { + return Selection{}, err + } + if err := ctx.Err(); err != nil { + return Selection{}, err + } + + normalized, err := normalizeCreateCustomProviderInput(input) + if err != nil { + return Selection{}, err + } + + providerCreateFlowMu.Lock() + defer providerCreateFlowMu.Unlock() + + cfgSnapshot := s.manager.Get() + if err := validateCustomProviderCreateConflict(cfgSnapshot, normalized); err != nil { + return Selection{}, err + } + + previousProcessEnvValue, hadPreviousProcessEnv := os.LookupEnv(normalized.APIKeyEnv) + previousUserEnvValue, hadPreviousUserEnv, err := lookupUserEnvVarForCreate(normalized.APIKeyEnv) + if err != nil { + return Selection{}, fmt.Errorf("selection: lookup user env: %w", err) + } + + providerPath := filepath.Join(s.manager.BaseDir(), "providers", normalized.Name, "provider.yaml") + providerSnapshot, err := loadProviderConfigSnapshot(providerPath) + if err != nil { + return Selection{}, fmt.Errorf("selection: snapshot provider config: %w", err) + } + + providerSaved := false + userEnvPersisted := false + processEnvApplied := false + rollback := func(originalErr error) error { + rolledErr := rollbackCreateCustomProvider( + s.manager.BaseDir(), + normalized.Name, + normalized.APIKeyEnv, + hadPreviousProcessEnv, + previousProcessEnvValue, + hadPreviousUserEnv, + previousUserEnvValue, + providerSaved, + userEnvPersisted, + processEnvApplied, + providerSnapshot, + originalErr, + ) + if providerSaved { + reloadCtx, cancel := context.WithTimeout(context.Background(), providerCreateRollbackReloadTimeout) + defer cancel() + if _, reloadErr := s.manager.Reload(reloadCtx); reloadErr != nil { + return fmt.Errorf("%w (post-rollback reload failed: %v)", rolledErr, reloadErr) + } + } + return rolledErr + } + + if err := config.SaveCustomProvider( + s.manager.BaseDir(), + normalized.Name, + normalized.Driver, + normalized.BaseURL, + normalized.APIKeyEnv, + normalized.APIStyle, + normalized.DeploymentMode, + normalized.APIVersion, + ); err != nil { + return Selection{}, fmt.Errorf("selection: save provider config: %w", err) + } + providerSaved = true + + if err := persistUserEnvVarForCreate(normalized.APIKeyEnv, normalized.APIKey); err != nil { + return Selection{}, rollback(fmt.Errorf("selection: persist user env: %w", err)) + } + userEnvPersisted = true + + if err := os.Setenv(normalized.APIKeyEnv, normalized.APIKey); err != nil { + return Selection{}, rollback(fmt.Errorf("selection: apply process env: %w", err)) + } + processEnvApplied = true + + if _, err := s.manager.Reload(ctx); err != nil { + return Selection{}, rollback(fmt.Errorf("selection: reload config snapshot: %w", err)) + } + + selection, err := s.SelectProvider(ctx, normalized.Name) + if err != nil { + return Selection{}, rollback(fmt.Errorf("selection: select provider: %w", err)) + } + + return selection, nil +} + +// normalizeCreateCustomProviderInput 统一裁剪新增 Provider 输入并执行基础字段校验。 +func normalizeCreateCustomProviderInput(input CreateCustomProviderInput) (createCustomProviderNormalizedInput, error) { + normalized := createCustomProviderNormalizedInput{ + Name: strings.TrimSpace(input.Name), + Driver: strings.TrimSpace(input.Driver), + BaseURL: strings.TrimSpace(input.BaseURL), + APIKeyEnv: strings.TrimSpace(input.APIKeyEnv), + APIKey: strings.TrimSpace(input.APIKey), + APIStyle: strings.TrimSpace(input.APIStyle), + DeploymentMode: strings.TrimSpace(input.DeploymentMode), + APIVersion: strings.TrimSpace(input.APIVersion), + } + + if err := config.ValidateCustomProviderName(normalized.Name); err != nil { + return createCustomProviderNormalizedInput{}, err + } + if normalized.Driver == "" { + return createCustomProviderNormalizedInput{}, errors.New("selection: provider driver is empty") + } + if normalized.APIKey == "" { + return createCustomProviderNormalizedInput{}, errors.New("selection: provider api key is empty") + } + if err := config.ValidateEnvVarName(normalized.APIKeyEnv); err != nil { + return createCustomProviderNormalizedInput{}, err + } + if config.IsProtectedEnvVarName(normalized.APIKeyEnv) { + return createCustomProviderNormalizedInput{}, fmt.Errorf("selection: env key %q is protected", normalized.APIKeyEnv) + } + + return normalized, nil +} + +// validateCustomProviderCreateConflict 校验新增 Provider 的名称与环境变量名是否与现有配置冲突。 +func validateCustomProviderCreateConflict(cfg config.Config, input createCustomProviderNormalizedInput) error { + existingProvider, err := cfg.ProviderByName(input.Name) + if err == nil && existingProvider.Source == config.ProviderSourceBuiltin { + return fmt.Errorf("selection: provider %q duplicates builtin provider", input.Name) + } + + targetProviderName := provider.NormalizeKey(input.Name) + targetEnvName := config.NormalizeEnvVarNameForCompare(input.APIKeyEnv) + for _, providerCfg := range cfg.Providers { + if provider.NormalizeKey(providerCfg.Name) == targetProviderName { + continue + } + if config.NormalizeEnvVarNameForCompare(providerCfg.APIKeyEnv) == targetEnvName { + return fmt.Errorf( + "selection: env key %q duplicates provider %q", + input.APIKeyEnv, + strings.TrimSpace(providerCfg.Name), + ) + } + } + return nil +} + +// loadProviderConfigSnapshot 读取 provider.yaml 快照，用于失败回滚恢复原始状态。 +func loadProviderConfigSnapshot(path string) (providerConfigSnapshot, error) { + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return providerConfigSnapshot{}, nil + } + return providerConfigSnapshot{}, err + } + return providerConfigSnapshot{ + Exists: true, + Content: append([]byte(nil), data...), + }, nil +} + +// rollbackCreateCustomProvider 回滚新增 Provider 过程中的副作用，保证失败后状态一致。 +func rollbackCreateCustomProvider( + baseDir string, + providerName string, + apiKeyEnv string, + hadPreviousProcessEnv bool, + previousProcessEnvValue string, + hadPreviousUserEnv bool, + previousUserEnvValue string, + providerSaved bool, + userEnvPersisted bool, + processEnvApplied bool, + providerSnapshot providerConfigSnapshot, + originalErr error, +) error { + rollbackErrs := make([]error, 0, 3) + + if processEnvApplied { + if hadPreviousProcessEnv { + if err := os.Setenv(apiKeyEnv, previousProcessEnvValue); err != nil { + rollbackErrs = append(rollbackErrs, fmt.Errorf("restore process env: %w", err)) + } + } else if err := os.Unsetenv(apiKeyEnv); err != nil { + rollbackErrs = append(rollbackErrs, fmt.Errorf("unset process env: %w", err)) + } + } + + if userEnvPersisted { + if hadPreviousUserEnv { + if err := persistUserEnvVarForCreate(apiKeyEnv, previousUserEnvValue); err != nil { + rollbackErrs = append(rollbackErrs, fmt.Errorf("restore user env: %w", err)) + } + } else if err := deleteUserEnvVarForCreate(apiKeyEnv); err != nil { + rollbackErrs = append(rollbackErrs, fmt.Errorf("delete user env: %w", err)) + } + } + + if providerSaved { + if err := restoreProviderConfigSnapshot(baseDir, providerName, providerSnapshot); err != nil { + rollbackErrs = append(rollbackErrs, fmt.Errorf("restore provider config: %w", err)) + } + } + + if len(rollbackErrs) == 0 { + return originalErr + } + return fmt.Errorf("%w (rollback failed: %v)", originalErr, errors.Join(rollbackErrs...)) +} + +// restoreProviderConfigSnapshot 恢复 provider.yaml 快照；若原先不存在则删除新增目录。 +func restoreProviderConfigSnapshot(baseDir string, providerName string, snapshot providerConfigSnapshot) error { + providerDir := filepath.Join(baseDir, "providers", providerName) + if !snapshot.Exists { + return config.DeleteCustomProvider(baseDir, providerName) + } + if err := os.RemoveAll(providerDir); err != nil { + return err + } + if err := os.MkdirAll(providerDir, 0o755); err != nil { + return err + } + return os.WriteFile(filepath.Join(providerDir, "provider.yaml"), snapshot.Content, 0o644) +} diff --git a/internal/config/state/service_provider_create_test.go b/internal/config/state/service_provider_create_test.go new file mode 100644 index 00000000..61313000 --- /dev/null +++ b/internal/config/state/service_provider_create_test.go @@ -0,0 +1,206 @@ +package state + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + configpkg "neo-code/internal/config" + "neo-code/internal/provider" + providertypes "neo-code/internal/provider/types" +) + +func TestCreateCustomProviderSuccess(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{ + {ID: "custom-model", Name: "custom-model"}, + }, + }) + + input := CreateCustomProviderInput{ + Name: "company-gateway", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: "COMPANY_GATEWAY_API_KEY", + APIKey: "test-key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + } + + restore := captureEnvForCreateProvider(t, input.APIKeyEnv) + defer restore() + _ = os.Unsetenv(input.APIKeyEnv) + + selection, err := service.CreateCustomProvider(context.Background(), input) + if err != nil { + t.Fatalf("CreateCustomProvider() error = %v", err) + } + if selection.ProviderID != input.Name { + t.Fatalf("expected provider %q, got %+v", input.Name, selection) + } + if strings.TrimSpace(os.Getenv(input.APIKeyEnv)) != input.APIKey { + t.Fatalf("expected process env %q to be set", input.APIKeyEnv) + } + + providerPath := filepath.Join(manager.BaseDir(), "providers", input.Name, "provider.yaml") + data, readErr := os.ReadFile(providerPath) + if readErr != nil { + t.Fatalf("read provider config: %v", readErr) + } + providerText := string(data) + if !strings.Contains(providerText, "api_key_env: "+input.APIKeyEnv) { + t.Fatalf("expected provider config to persist env name, got %q", providerText) + } + if strings.Contains(providerText, input.APIKey) { + t.Fatalf("provider config should not persist api key, got %q", providerText) + } +} + +func TestCreateCustomProviderRollbackOnSelectFailure(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), errorCatalogStub{err: context.DeadlineExceeded}) + + input := CreateCustomProviderInput{ + Name: "rollback-gateway", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: "ROLLBACK_GATEWAY_API_KEY", + APIKey: "new-key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + } + + restore := captureEnvForCreateProvider(t, input.APIKeyEnv) + defer restore() + if err := os.Setenv(input.APIKeyEnv, "old-key"); err != nil { + t.Fatalf("Setenv() error = %v", err) + } + + if _, err := service.CreateCustomProvider(context.Background(), input); err == nil { + t.Fatal("expected CreateCustomProvider() to fail") + } + + if got := os.Getenv(input.APIKeyEnv); got != "old-key" { + t.Fatalf("expected process env rollback, got %q", got) + } + providerDir := filepath.Join(manager.BaseDir(), "providers", input.Name) + if _, statErr := os.Stat(providerDir); !os.IsNotExist(statErr) { + t.Fatalf("expected provider dir rollback, stat err = %v", statErr) + } + cfgAfterRollback := manager.Get() + if _, findErr := cfgAfterRollback.ProviderByName(input.Name); findErr == nil { + t.Fatalf("expected provider %q to be absent from manager snapshot after rollback", input.Name) + } +} + +func TestCreateCustomProviderRejectsEnvConflicts(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{{ID: "m1", Name: "m1"}}, + }) + + _, err := service.CreateCustomProvider(context.Background(), CreateCustomProviderInput{ + Name: "conflict-provider", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: configpkg.OpenAIDefaultAPIKeyEnv, + APIKey: "key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + }) + if err == nil || !strings.Contains(err.Error(), "duplicates provider") { + t.Fatalf("expected duplicate env error, got %v", err) + } +} + +func TestCreateCustomProviderRejectsProtectedEnvName(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{{ID: "m1", Name: "m1"}}, + }) + + _, err := service.CreateCustomProvider(context.Background(), CreateCustomProviderInput{ + Name: "protected-env-provider", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: "PATH", + APIKey: "key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + }) + if err == nil || !strings.Contains(err.Error(), "protected") { + t.Fatalf("expected protected env error, got %v", err) + } +} + +func TestCreateCustomProviderRejectsInvalidProviderName(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{{ID: "m1", Name: "m1"}}, + }) + + _, err := service.CreateCustomProvider(context.Background(), CreateCustomProviderInput{ + Name: "../invalid-provider", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: "INVALID_PROVIDER_NAME_API_KEY", + APIKey: "key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + }) + if err == nil || !strings.Contains(err.Error(), "provider name") { + t.Fatalf("expected invalid provider name error, got %v", err) + } +} + +func captureEnvForCreateProvider(t *testing.T, key string) func() { + t.Helper() + + value, exists := os.LookupEnv(key) + return func() { + if exists { + _ = os.Setenv(key, value) + return + } + _ = os.Unsetenv(key) + } +} + +func stubUserEnvOpsForCreateProvider(t *testing.T) (func(), func(), func()) { + t.Helper() + + prevPersist := persistUserEnvVarForCreate + prevDelete := deleteUserEnvVarForCreate + prevLookup := lookupUserEnvVarForCreate + + persistUserEnvVarForCreate = func(key string, value string) error { return nil } + deleteUserEnvVarForCreate = func(key string) error { return nil } + lookupUserEnvVarForCreate = func(key string) (string, bool, error) { return "", false, nil } + + return func() { persistUserEnvVarForCreate = prevPersist }, + func() { deleteUserEnvVarForCreate = prevDelete }, + func() { lookupUserEnvVarForCreate = prevLookup } +} diff --git a/internal/provider/openaicompat/discovery.go b/internal/provider/openaicompat/discovery.go index bb0510dd..43361a88 100644 --- a/internal/provider/openaicompat/discovery.go +++ b/internal/provider/openaicompat/discovery.go @@ -13,7 +13,29 @@ import ( // openAIModelsResponse 表示 /models 端点的响应结构。 type openAIModelsResponse struct { - Data []map[string]any `json:"data"` + Data any `json:"data"` +} + +// decodeModelsData 兼容解析 data 字段，支持数组与单对象两种常见返回格式。 +func decodeModelsData(data any) ([]map[string]any, error) { + switch value := data.(type) { + case nil: + return nil, nil + case []any: + models := make([]map[string]any, 0, len(value)) + for _, item := range value { + model, ok := item.(map[string]any) + if !ok { + continue + } + models = append(models, model) + } + return models, nil + case map[string]any: + return []map[string]any{value}, nil + default: + return nil, fmt.Errorf("unsupported models data type %T", data) + } } // fetchModels 从 OpenAI 兼容的 /models 端点获取原始模型列表。 @@ -47,5 +69,9 @@ func (p *Provider) fetchModels(ctx context.Context) ([]map[string]any, error) { if err := json.NewDecoder(resp.Body).Decode(&payload); err != nil { return nil, fmt.Errorf("%sdecode models response: %w", shared.ErrorPrefix, err) } - return payload.Data, nil + models, err := decodeModelsData(payload.Data) + if err != nil { + return nil, fmt.Errorf("%sdecode models response: %w", shared.ErrorPrefix, err) + } + return models, nil } diff --git a/internal/provider/openaicompat/openaicompat_test.go b/internal/provider/openaicompat/openaicompat_test.go index 7f002a83..d93abff8 100644 --- a/internal/provider/openaicompat/openaicompat_test.go +++ b/internal/provider/openaicompat/openaicompat_test.go @@ -838,6 +838,59 @@ func TestFetchModelsDecodeError(t *testing.T) { } } +func TestFetchModelsAcceptsObjectData(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": map[string]any{ + "id": "gpt-4o-mini", + "name": "GPT-4o Mini", + }, + }) + })) + defer server.Close() + + p, err := New(resolvedConfig(server.URL, "decode-key")) + if err != nil { + t.Fatalf("New() error = %v", err) + } + p.client = server.Client() + + models, err := p.fetchModels(context.Background()) + if err != nil { + t.Fatalf("fetchModels() error = %v", err) + } + if len(models) != 1 { + t.Fatalf("expected 1 model, got %d", len(models)) + } + if id, _ := models[0]["id"].(string); id != "gpt-4o-mini" { + t.Fatalf("expected model id %q, got %#v", "gpt-4o-mini", models[0]["id"]) + } +} + +func TestFetchModelsRejectsUnsupportedDataType(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"data": "unexpected"}) + })) + defer server.Close() + + p, err := New(resolvedConfig(server.URL, "decode-key")) + if err != nil { + t.Fatalf("New() error = %v", err) + } + p.client = server.Client() + + _, err = p.fetchModels(context.Background()) + if err == nil || !strings.Contains(err.Error(), "decode models response") { + t.Fatalf("expected decode models response error, got %v", err) + } +} + func TestDiscoverModelsSkipsInvalidEntriesAndDedupes(t *testing.T) { t.Parallel() diff --git a/internal/tui/bootstrap/builder.go b/internal/tui/bootstrap/builder.go index 951b20e7..0a027ff0 100644 --- a/internal/tui/bootstrap/builder.go +++ b/internal/tui/bootstrap/builder.go @@ -18,6 +18,7 @@ type ProviderService interface { ListModels(ctx context.Context) ([]providertypes.ModelDescriptor, error) ListModelsSnapshot(ctx context.Context) ([]providertypes.ModelDescriptor, error) SetCurrentModel(ctx context.Context, modelID string) (configstate.Selection, error) + CreateCustomProvider(ctx context.Context, input configstate.CreateCustomProviderInput) (configstate.Selection, error) } // Options 定义 bootstrap 装配输入。 diff --git a/internal/tui/bootstrap/builder_test.go b/internal/tui/bootstrap/builder_test.go index b6bf3677..873a5135 100644 --- a/internal/tui/bootstrap/builder_test.go +++ b/internal/tui/bootstrap/builder_test.go @@ -99,6 +99,13 @@ func (s *testProviderService) SetCurrentModel(ctx context.Context, modelID strin return configstate.Selection{}, nil } +func (s *testProviderService) CreateCustomProvider( + ctx context.Context, + input configstate.CreateCustomProviderInput, +) (configstate.Selection, error) { + return configstate.Selection{}, nil +} + func TestBuild(t *testing.T) { t.Run("success", func(t *testing.T) { manager := &config.Manager{} @@ -310,6 +317,13 @@ func (s noopProviderService) SetCurrentModel(ctx context.Context, modelID string return configstate.Selection{}, nil } +func (s noopProviderService) CreateCustomProvider( + ctx context.Context, + input configstate.CreateCustomProviderInput, +) (configstate.Selection, error) { + return configstate.Selection{}, nil +} + func TestBuildFactoryErrors(t *testing.T) { manager := &config.Manager{} runtimeSvc := noopRuntime{} diff --git a/internal/tui/core/app/app.go b/internal/tui/core/app/app.go index 37769e16..e89c81f6 100644 --- a/internal/tui/core/app/app.go +++ b/internal/tui/core/app/app.go @@ -58,6 +58,7 @@ type ProviderController interface { ListModels(ctx context.Context) ([]providertypes.ModelDescriptor, error) ListModelsSnapshot(ctx context.Context) ([]providertypes.ModelDescriptor, error) SetCurrentModel(ctx context.Context, modelID string) (configstate.Selection, error) + CreateCustomProvider(ctx context.Context, input configstate.CreateCustomProviderInput) (configstate.Selection, error) } // appServices 聚合 App 需要的服务依赖，避免与渲染状态混在同一层级。 @@ -128,6 +129,7 @@ type providerAddFormState struct { APIStyle string DeploymentMode string APIVersion string + APIKeyEnv string APIKey string Error string ErrorIsHard bool diff --git a/internal/tui/core/app/commands_test.go b/internal/tui/core/app/commands_test.go index 34c5c2bc..ce0ccade 100644 --- a/internal/tui/core/app/commands_test.go +++ b/internal/tui/core/app/commands_test.go @@ -175,6 +175,13 @@ func (s errorProviderService) SetCurrentModel(ctx context.Context, modelID strin return configstate.Selection{}, s.err } +func (s errorProviderService) CreateCustomProvider( + ctx context.Context, + input configstate.CreateCustomProviderInput, +) (configstate.Selection, error) { + return configstate.Selection{}, s.err +} + func TestExecuteLocalCommandErrors(t *testing.T) { app, _ := newTestApp(t) snapshot := app.currentStatusSnapshot() diff --git a/internal/tui/core/app/update.go b/internal/tui/core/app/update.go index 19e3e7be..19a72607 100644 --- a/internal/tui/core/app/update.go +++ b/internal/tui/core/app/update.go @@ -4,10 +4,10 @@ import ( "context" "errors" "fmt" - "os" "path/filepath" "strings" "time" + "unicode" "unicode/utf8" "github.com/charmbracelet/bubbles/key" @@ -17,6 +17,7 @@ import ( "github.com/charmbracelet/lipgloss" "neo-code/internal/config" + configstate "neo-code/internal/config/state" "neo-code/internal/memo" "neo-code/internal/provider" providertypes "neo-code/internal/provider/types" @@ -43,13 +44,12 @@ const ( ) const providerAddSelectTimeout = 10 * time.Second +const providerAddNonPersistentEnvWarning = "API key is applied to the current process only on this platform; persist it in your shell profile for future sessions." const sessionSwitchBusyMessage = "cannot switch sessions while run or compact is active" var panelOrder = []panel{panelTranscript, panelActivity, panelInput} -var persistProviderUserEnvVar = config.PersistUserEnvVar -var deleteProviderUserEnvVar = config.DeleteUserEnvVar -var lookupProviderUserEnvVar = config.LookupUserEnvVar +var supportsUserEnvPersistence = config.SupportsUserEnvPersistence func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { var cmds []tea.Cmd @@ -2187,6 +2187,7 @@ const ( providerAddFieldAPIStyle providerAddFieldDeploymentMode providerAddFieldAPIVersion + providerAddFieldAPIKeyEnv providerAddFieldAPIKey ) @@ -2206,7 +2207,7 @@ func providerAddVisibleFields(driver string) []providerAddFieldID { fields = append(fields, providerAddFieldAPIVersion) } - fields = append(fields, providerAddFieldAPIKey) + fields = append(fields, providerAddFieldAPIKeyEnv, providerAddFieldAPIKey) return fields } @@ -2248,6 +2249,7 @@ func (a *App) startProviderAddForm() { APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, DeploymentMode: "", APIVersion: "", + APIKeyEnv: "", APIKey: "", Error: "", ErrorIsHard: false, @@ -2294,6 +2296,8 @@ func (a *App) handleProviderAddFormInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { a.providerAddForm.DeploymentMode = trimLastRune(a.providerAddForm.DeploymentMode) case providerAddFieldAPIVersion: a.providerAddForm.APIVersion = trimLastRune(a.providerAddForm.APIVersion) + case providerAddFieldAPIKeyEnv: + a.providerAddForm.APIKeyEnv = trimLastRune(a.providerAddForm.APIKeyEnv) case providerAddFieldAPIKey: a.providerAddForm.APIKey = trimLastRune(a.providerAddForm.APIKey) } @@ -2328,19 +2332,23 @@ func (a *App) handleProviderAddFormInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return a, nil default: if len(typed.Runes) > 0 { - switch currentProviderAddField(a.providerAddForm) { - case providerAddFieldName: - a.providerAddForm.Name += string(typed.Runes) - case providerAddFieldBaseURL: - a.providerAddForm.BaseURL += string(typed.Runes) - case providerAddFieldAPIStyle: - a.providerAddForm.APIStyle += string(typed.Runes) - case providerAddFieldDeploymentMode: - a.providerAddForm.DeploymentMode += string(typed.Runes) - case providerAddFieldAPIVersion: - a.providerAddForm.APIVersion += string(typed.Runes) - case providerAddFieldAPIKey: - a.providerAddForm.APIKey += string(typed.Runes) + if cleanInput := sanitizeProviderAddInputRunes(typed.Runes); cleanInput != "" { + switch currentProviderAddField(a.providerAddForm) { + case providerAddFieldName: + a.providerAddForm.Name += cleanInput + case providerAddFieldBaseURL: + a.providerAddForm.BaseURL += cleanInput + case providerAddFieldAPIStyle: + a.providerAddForm.APIStyle += cleanInput + case providerAddFieldDeploymentMode: + a.providerAddForm.DeploymentMode += cleanInput + case providerAddFieldAPIVersion: + a.providerAddForm.APIVersion += cleanInput + case providerAddFieldAPIKeyEnv: + a.providerAddForm.APIKeyEnv += cleanInput + case providerAddFieldAPIKey: + a.providerAddForm.APIKey += cleanInput + } } } } @@ -2381,24 +2389,27 @@ type providerAddRequest struct { APIStyle string DeploymentMode string APIVersion string + APIKeyEnv string APIKey string } type providerAddResultMsg struct { - Name string - Model string - Error string + Name string + Model string + Error string + Warning string } func buildProviderAddRequest(form providerAddFormState) (providerAddRequest, string) { request := providerAddRequest{ - Name: strings.TrimSpace(form.Name), - Driver: provider.NormalizeProviderDriver(form.Driver), - BaseURL: strings.TrimSpace(form.BaseURL), - APIStyle: strings.TrimSpace(form.APIStyle), - DeploymentMode: strings.TrimSpace(form.DeploymentMode), - APIVersion: strings.TrimSpace(form.APIVersion), - APIKey: strings.TrimSpace(form.APIKey), + Name: normalizeProviderAddFieldValue(form.Name), + Driver: provider.NormalizeProviderDriver(normalizeProviderAddFieldValue(form.Driver)), + BaseURL: normalizeProviderAddFieldValue(form.BaseURL), + APIStyle: normalizeProviderAddFieldValue(form.APIStyle), + DeploymentMode: normalizeProviderAddFieldValue(form.DeploymentMode), + APIVersion: normalizeProviderAddFieldValue(form.APIVersion), + APIKeyEnv: normalizeProviderAddFieldValue(form.APIKeyEnv), + APIKey: normalizeProviderAddFieldValue(form.APIKey), } if request.Name == "" { @@ -2410,6 +2421,12 @@ func buildProviderAddRequest(form providerAddFormState) (providerAddRequest, str if request.APIKey == "" { return providerAddRequest{}, "API Key is required" } + if request.APIKeyEnv == "" { + return providerAddRequest{}, "API Key Env is required" + } + if err := config.ValidateEnvVarName(request.APIKeyEnv); err != nil { + return providerAddRequest{}, err.Error() + } switch request.Driver { case provider.DriverOpenAICompat: @@ -2445,34 +2462,26 @@ func buildProviderAddRequest(form providerAddFormState) (providerAddRequest, str return request, "" } -func providerAddAPIKeyEnv(name string) string { - upper := strings.ToUpper(strings.TrimSpace(name)) - if upper == "" { - return "CUSTOM_PROVIDER_API_KEY" +// sanitizeProviderAddInputRunes 过滤 provider 表单输入中的控制字符，避免不可见字符污染配置字段。 +func sanitizeProviderAddInputRunes(runes []rune) string { + if len(runes) == 0 { + return "" } - var b strings.Builder - lastUnderscore := false - for _, r := range upper { - if (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') { - b.WriteRune(r) - lastUnderscore = false + var builder strings.Builder + builder.Grow(len(runes)) + for _, r := range runes { + if unicode.IsControl(r) || unicode.In(r, unicode.Cf) { continue } - if !lastUnderscore { - b.WriteByte('_') - lastUnderscore = true - } + builder.WriteRune(r) } + return builder.String() +} - normalized := strings.Trim(b.String(), "_") - if normalized == "" { - normalized = "CUSTOM_PROVIDER" - } - if normalized[0] >= '0' && normalized[0] <= '9' { - normalized = "P_" + normalized - } - return normalized + "_API_KEY" +// normalizeProviderAddFieldValue 对 provider 表单字段做统一清理，去除控制字符并裁剪首尾空白。 +func normalizeProviderAddFieldValue(value string) string { + return strings.TrimSpace(sanitizeProviderAddInputRunes([]rune(value))) } // trimLastRune 按 UTF-8 rune 删除字符串末尾一个字符，避免按字节截断导致乱码。 @@ -2505,173 +2514,25 @@ func sanitizeProviderAddError(err error, secrets ...string) string { return text } -type fileSnapshot struct { - Exists bool - Content []byte -} - -// loadFileSnapshot 读取目标文件当前快照，用于 provider add 失败时恢复原始状态。 -func loadFileSnapshot(path string) (fileSnapshot, error) { - data, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - return fileSnapshot{}, nil - } - return fileSnapshot{}, err - } - return fileSnapshot{ - Exists: true, - Content: append([]byte(nil), data...), - }, nil -} - -// restoreEnvFileSnapshot 将 .env 恢复到提交前快照，避免覆盖场景下丢失原有键值。 -func restoreEnvFileSnapshot(path string, snapshot fileSnapshot) error { - if !snapshot.Exists { - if err := os.Remove(path); err != nil && !os.IsNotExist(err) { - return err - } - return nil - } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err - } - return os.WriteFile(path, snapshot.Content, 0o600) -} - -// restoreProviderConfigSnapshot 恢复 provider.yaml 快照；若原先不存在则清理新建目录。 -func restoreProviderConfigSnapshot(baseDir string, providerName string, snapshot fileSnapshot) error { - providerDir := filepath.Join(baseDir, "providers", providerName) - if !snapshot.Exists { - return config.DeleteCustomProvider(baseDir, providerName) - } - if err := os.RemoveAll(providerDir); err != nil { - return err - } - if err := os.MkdirAll(providerDir, 0o755); err != nil { - return err - } - providerPath := filepath.Join(providerDir, "provider.yaml") - return os.WriteFile(providerPath, snapshot.Content, 0o644) -} - func (a *App) runProviderAddFlow(request providerAddRequest) tea.Cmd { baseDir := a.configManager.BaseDir() - configManager := a.configManager providerSvc := a.providerSvc return func() tea.Msg { - apiKeyEnv := providerAddAPIKeyEnv(request.Name) - previousProcessEnvValue, hadPreviousProcessEnv := os.LookupEnv(apiKeyEnv) - previousUserEnvValue, hadPreviousUserEnv, err := lookupProviderUserEnvVar(apiKeyEnv) - if err != nil { - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("lookup user environment variable: %w", err), request.APIKey, baseDir), - } - } - - providerPath := filepath.Join(baseDir, "providers", request.Name, "provider.yaml") - providerSnapshot, err := loadFileSnapshot(providerPath) - if err != nil { - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("snapshot provider config: %w", err), request.APIKey, baseDir), - } - } - - envPath := config.EnvFilePath(baseDir) - envSnapshot, err := loadFileSnapshot(envPath) - if err != nil { - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("snapshot env file: %w", err), request.APIKey, baseDir), - } - } - - rollback := func(processEnvApplied bool, userEnvPersisted bool, envPersisted bool, providerSaved bool, originalErr error) error { - return rollbackProviderAddSideEffects( - baseDir, - request.Name, - apiKeyEnv, - processEnvApplied, - userEnvPersisted, - envPersisted, - hadPreviousProcessEnv, - previousProcessEnvValue, - hadPreviousUserEnv, - previousUserEnvValue, - providerSaved, - envPath, - envSnapshot, - providerSnapshot, - originalErr, - ) - } - - providerSaved := false - envPersisted := false - userEnvPersisted := false - processEnvApplied := false - - if err := config.SaveCustomProvider( - baseDir, - request.Name, - request.Driver, - request.BaseURL, - apiKeyEnv, - request.APIStyle, - request.DeploymentMode, - request.APIVersion, - ); err != nil { - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("save provider config: %w", err), request.APIKey, baseDir), - } - } - providerSaved = true - if err := config.PersistEnvVar(baseDir, apiKeyEnv, request.APIKey); err != nil { - err = rollback(processEnvApplied, userEnvPersisted, envPersisted, providerSaved, err) - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("persist api key: %w", err), request.APIKey, baseDir), - } - } - envPersisted = true - if err := persistProviderUserEnvVar(apiKeyEnv, request.APIKey); err != nil { - err = rollback(processEnvApplied, userEnvPersisted, envPersisted, providerSaved, err) - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError( - fmt.Errorf("persist user environment variable: %w", err), - request.APIKey, - baseDir, - ), - } - } - userEnvPersisted = true - if err := os.Setenv(apiKeyEnv, request.APIKey); err != nil { - err = rollback(processEnvApplied, userEnvPersisted, envPersisted, providerSaved, err) - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("apply api key env: %w", err), request.APIKey, baseDir), - } - } - processEnvApplied = true - if _, err := configManager.Reload(context.Background()); err != nil { - err = rollback(processEnvApplied, userEnvPersisted, envPersisted, providerSaved, err) - return providerAddResultMsg{ - Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("reload config snapshot: %w", err), request.APIKey, baseDir), - } - } - ctx, cancel := context.WithTimeout(context.Background(), providerAddSelectTimeout) defer cancel() - selection, err := providerSvc.SelectProvider(ctx, request.Name) + selection, err := providerSvc.CreateCustomProvider(ctx, configstate.CreateCustomProviderInput{ + Name: request.Name, + Driver: request.Driver, + BaseURL: request.BaseURL, + APIStyle: request.APIStyle, + DeploymentMode: request.DeploymentMode, + APIVersion: request.APIVersion, + APIKeyEnv: request.APIKeyEnv, + APIKey: request.APIKey, + }) if err != nil { - err = rollback(processEnvApplied, userEnvPersisted, envPersisted, providerSaved, err) if errors.Is(err, context.DeadlineExceeded) { err = fmt.Errorf( "model discovery timed out after %s; check base URL, API key, and network connectivity", @@ -2680,77 +2541,23 @@ func (a *App) runProviderAddFlow(request providerAddRequest) tea.Cmd { } return providerAddResultMsg{ Name: request.Name, - Error: sanitizeProviderAddError(fmt.Errorf("select provider: %w", err), request.APIKey, baseDir), + Error: sanitizeProviderAddError(fmt.Errorf("create provider: %w", err), request.APIKey, baseDir), } } return providerAddResultMsg{ - Name: request.Name, - Model: strings.TrimSpace(selection.ModelID), - } - } -} - -// rollbackProviderAddSideEffects 回滚 provider add 过程中已落地的副作用，避免失败后残留配置与密钥。 -func rollbackProviderAddSideEffects( - baseDir string, - providerName string, - apiKeyEnv string, - processEnvApplied bool, - userEnvPersisted bool, - envPersisted bool, - hadPreviousEnv bool, - previousEnvValue string, - hadPreviousUserEnv bool, - previousUserEnvValue string, - providerSaved bool, - envPath string, - envSnapshot fileSnapshot, - providerSnapshot fileSnapshot, - originalErr error, -) error { - rollbackErrs := make([]error, 0, 4) - - if processEnvApplied { - if hadPreviousEnv { - if err := os.Setenv(apiKeyEnv, previousEnvValue); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("restore process env: %w", err)) - } - } else { - if err := os.Unsetenv(apiKeyEnv); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("unset process env: %w", err)) - } - } - } - - if userEnvPersisted { - if hadPreviousUserEnv { - if err := persistProviderUserEnvVar(apiKeyEnv, previousUserEnvValue); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("restore user env: %w", err)) - } - } else { - if err := deleteProviderUserEnvVar(apiKeyEnv); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("delete user env: %w", err)) - } - } - } - - if envPersisted { - if err := restoreEnvFileSnapshot(envPath, envSnapshot); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("restore persisted env: %w", err)) - } - } - - if providerSaved { - if err := restoreProviderConfigSnapshot(baseDir, providerName, providerSnapshot); err != nil { - rollbackErrs = append(rollbackErrs, fmt.Errorf("restore provider config: %w", err)) + Name: request.Name, + Model: strings.TrimSpace(selection.ModelID), + Warning: providerAddPersistenceWarning(), } } +} - if len(rollbackErrs) == 0 { - return originalErr +func providerAddPersistenceWarning() string { + if supportsUserEnvPersistence() { + return "" } - return fmt.Errorf("%w (rollback failed: %v)", originalErr, errors.Join(rollbackErrs...)) + return providerAddNonPersistentEnvWarning } func (a *App) handleProviderAddResultMsg(msg providerAddResultMsg) { @@ -2777,6 +2584,9 @@ func (a *App) handleProviderAddResultMsg(msg providerAddResultMsg) { a.state.CurrentModel = msg.Model } a.appendActivity("provider", "Provider added", msg.Name, false) + if msg.Warning != "" { + a.appendActivity("provider", "Provider key persistence", msg.Warning, false) + } if err := a.refreshProviderPicker(); err != nil { a.appendActivity("system", "Failed to refresh providers", err.Error(), true) diff --git a/internal/tui/core/app/update_test.go b/internal/tui/core/app/update_test.go index 7bbdbc2e..0a940795 100644 --- a/internal/tui/core/app/update_test.go +++ b/internal/tui/core/app/update_test.go @@ -32,6 +32,8 @@ type stubProviderService struct { selectErr error selectDelay time.Duration selectResponse configstate.Selection + createErr error + createResponse configstate.Selection } func (s stubProviderService) ListProviderOptions(ctx context.Context) ([]configstate.ProviderOption, error) { @@ -78,6 +80,23 @@ func (s stubProviderService) SetCurrentModel(ctx context.Context, modelID string return configstate.Selection{ProviderID: providerID, ModelID: modelID}, nil } +func (s stubProviderService) CreateCustomProvider( + ctx context.Context, + input configstate.CreateCustomProviderInput, +) (configstate.Selection, error) { + if s.createErr != nil { + return configstate.Selection{}, s.createErr + } + if strings.TrimSpace(s.createResponse.ProviderID) != "" || strings.TrimSpace(s.createResponse.ModelID) != "" { + return s.createResponse, nil + } + modelID := "" + if len(s.models) > 0 { + modelID = s.models[0].ID + } + return configstate.Selection{ProviderID: input.Name, ModelID: modelID}, nil +} + type stubRuntime struct { events chan agentruntime.RuntimeEvent prepareInputs []agentruntime.PrepareInput @@ -273,6 +292,7 @@ func TestSubmitProviderAddFormRequiresAnthropicBaseURL(t *testing.T) { app.providerAddForm.Name = "anthropic-gateway" app.providerAddForm.Driver = provider.DriverAnthropic + app.providerAddForm.APIKeyEnv = "ANTHROPIC_GATEWAY_API_KEY" app.providerAddForm.APIKey = "test-key" app.providerAddForm.BaseURL = "" @@ -286,9 +306,9 @@ func TestSubmitProviderAddFormRequiresAnthropicBaseURL(t *testing.T) { } func TestSubmitProviderAddFormAsyncSuccess(t *testing.T) { - restorePersistUserEnv := persistProviderUserEnvVar - persistProviderUserEnvVar = func(key string, value string) error { return nil } - t.Cleanup(func() { persistProviderUserEnvVar = restorePersistUserEnv }) + prevSupportsUserEnvPersistence := supportsUserEnvPersistence + supportsUserEnvPersistence = func() bool { return false } + t.Cleanup(func() { supportsUserEnvPersistence = prevSupportsUserEnvPersistence }) providerName := "team-gateway" modelID := "gateway-model" @@ -303,7 +323,7 @@ func TestSubmitProviderAddFormAsyncSuccess(t *testing.T) { }, }, models: []providertypes.ModelDescriptor{{ID: modelID, Name: modelID}}, - selectResponse: configstate.Selection{ + createResponse: configstate.Selection{ ProviderID: providerName, ModelID: modelID, }, @@ -314,6 +334,7 @@ func TestSubmitProviderAddFormAsyncSuccess(t *testing.T) { app.providerAddForm.Name = providerName app.providerAddForm.Driver = provider.DriverOpenAICompat app.providerAddForm.BaseURL = "https://team-gateway.example.com/v1" + app.providerAddForm.APIKeyEnv = "TEAM_GATEWAY_API_KEY" app.providerAddForm.APIKey = "sk-test-123" app.providerAddForm.APIStyle = provider.OpenAICompatibleAPIStyleChatCompletions @@ -325,25 +346,16 @@ func TestSubmitProviderAddFormAsyncSuccess(t *testing.T) { t.Fatalf("expected form to enter submitting state") } - envName := providerAddAPIKeyEnv(providerName) - defer os.Unsetenv(envName) - msg := cmd() - if _, ok := msg.(providerAddResultMsg); !ok { + result, ok := msg.(providerAddResultMsg) + if !ok { t.Fatalf("expected providerAddResultMsg, got %T", msg) } - if got := strings.TrimSpace(os.Getenv(envName)); got != "sk-test-123" { - t.Fatalf("expected API key in env %s, got %q", envName, got) - } - envData, readErr := os.ReadFile(config.EnvFilePath(app.configManager.BaseDir())) - if readErr != nil { - t.Fatalf("expected persisted env file, read error = %v", readErr) - } - if !strings.Contains(string(envData), envName+"=sk-test-123") { - t.Fatalf("expected env file to persist API key, got %q", string(envData)) + if !strings.Contains(result.Warning, "current process only") { + t.Fatalf("expected non-persistent env warning, got %q", result.Warning) } - next, _ := app.Update(msg) + next, _ := app.Update(result) app = next.(App) if app.providerAddForm != nil { t.Fatalf("expected provider add form to close on success") @@ -354,16 +366,22 @@ func TestSubmitProviderAddFormAsyncSuccess(t *testing.T) { if app.state.CurrentModel != modelID { t.Fatalf("expected current model %q, got %q", modelID, app.state.CurrentModel) } + var foundPersistenceNotice bool + for _, entry := range app.activities { + if entry.Title == "Provider key persistence" && strings.Contains(entry.Detail, "current process only") { + foundPersistenceNotice = true + break + } + } + if !foundPersistenceNotice { + t.Fatal("expected provider key persistence activity notice") + } } func TestSubmitProviderAddFormRedactsSensitiveError(t *testing.T) { - restorePersistUserEnv := persistProviderUserEnvVar - persistProviderUserEnvVar = func(key string, value string) error { return nil } - t.Cleanup(func() { persistProviderUserEnvVar = restorePersistUserEnv }) - secretKey := "sk-secret-456" service := stubProviderService{ - selectErr: errors.New("authentication failed for key " + secretKey), + createErr: errors.New("authentication failed for key " + secretKey), } app, _ := newTestAppWithProviderService(t, service) app.startProviderAddForm() @@ -371,6 +389,7 @@ func TestSubmitProviderAddFormRedactsSensitiveError(t *testing.T) { app.providerAddForm.Name = "redact-gateway" app.providerAddForm.Driver = provider.DriverOpenAICompat app.providerAddForm.BaseURL = "https://redact-gateway.example.com/v1" + app.providerAddForm.APIKeyEnv = "REDACT_GATEWAY_API_KEY" app.providerAddForm.APIKey = secretKey cmd := app.submitProviderAddForm() @@ -390,213 +409,21 @@ func TestSubmitProviderAddFormRedactsSensitiveError(t *testing.T) { } } -func TestSubmitProviderAddFormRollsBackPersistedStateOnSelectFailure(t *testing.T) { - restorePersistUserEnv := persistProviderUserEnvVar - restoreDeleteUserEnv := deleteProviderUserEnvVar - persistProviderUserEnvVar = func(key string, value string) error { return nil } - deleteProviderUserEnvVar = func(key string) error { return nil } - t.Cleanup(func() { persistProviderUserEnvVar = restorePersistUserEnv }) - t.Cleanup(func() { deleteProviderUserEnvVar = restoreDeleteUserEnv }) - - providerName := "rollback-gateway" - envName := providerAddAPIKeyEnv(providerName) - restoreEnv := captureEnv(t, envName) - defer restoreEnv() - if err := os.Setenv(envName, "previous-value"); err != nil { - t.Fatalf("Setenv() error = %v", err) - } - - service := stubProviderService{ - selectErr: errors.New("select failed"), - } - app, _ := newTestAppWithProviderService(t, service) - app.startProviderAddForm() - app.providerAddForm.Name = providerName - app.providerAddForm.Driver = provider.DriverOpenAICompat - app.providerAddForm.BaseURL = "https://rollback.example.com/v1" - app.providerAddForm.APIKey = "sk-failed-rollback" - app.providerAddForm.APIStyle = provider.OpenAICompatibleAPIStyleChatCompletions - - cmd := app.submitProviderAddForm() - if cmd == nil { - t.Fatalf("expected async command") - } - msg := cmd() - result, ok := msg.(providerAddResultMsg) - if !ok { - t.Fatalf("expected providerAddResultMsg, got %T", msg) - } - if strings.TrimSpace(result.Error) == "" { - t.Fatalf("expected failure result") - } - - if got := os.Getenv(envName); got != "previous-value" { - t.Fatalf("expected process env restored, got %q", got) - } - - envData, readErr := os.ReadFile(config.EnvFilePath(app.configManager.BaseDir())) - if readErr != nil && !os.IsNotExist(readErr) { - t.Fatalf("read env file: %v", readErr) - } - if strings.Contains(string(envData), envName+"=") { - t.Fatalf("expected persisted env key rollback, got %q", string(envData)) - } - - providerPath := filepath.Join(app.configManager.BaseDir(), "providers", providerName) - if _, err := os.Stat(providerPath); !os.IsNotExist(err) { - t.Fatalf("expected provider dir rollback, stat err = %v", err) - } -} - -func TestSubmitProviderAddFormRollsBackOnUserEnvPersistFailure(t *testing.T) { - restorePersistUserEnv := persistProviderUserEnvVar - restoreDeleteUserEnv := deleteProviderUserEnvVar - persistProviderUserEnvVar = func(key string, value string) error { return errors.New("user env failed") } - deleteProviderUserEnvVar = func(key string) error { return nil } - t.Cleanup(func() { persistProviderUserEnvVar = restorePersistUserEnv }) - t.Cleanup(func() { deleteProviderUserEnvVar = restoreDeleteUserEnv }) - - providerName := "user-env-fail" - envName := providerAddAPIKeyEnv(providerName) - restoreEnv := captureEnv(t, envName) - defer restoreEnv() - _ = os.Unsetenv(envName) - +func TestSubmitProviderAddFormRequiresAPIKeyEnv(t *testing.T) { app, _ := newTestApp(t) app.startProviderAddForm() - app.providerAddForm.Name = providerName - app.providerAddForm.Driver = provider.DriverOpenAICompat - app.providerAddForm.BaseURL = "https://rollback.example.com/v1" - app.providerAddForm.APIKey = "sk-failed" - app.providerAddForm.APIStyle = provider.OpenAICompatibleAPIStyleChatCompletions - - cmd := app.submitProviderAddForm() - if cmd == nil { - t.Fatalf("expected async command") - } - msg := cmd() - result, ok := msg.(providerAddResultMsg) - if !ok { - t.Fatalf("expected providerAddResultMsg, got %T", msg) - } - if strings.TrimSpace(result.Error) == "" { - t.Fatalf("expected failure result") - } - - if got := os.Getenv(envName); got != "" { - t.Fatalf("expected process env to stay unset, got %q", got) - } - - envData, readErr := os.ReadFile(config.EnvFilePath(app.configManager.BaseDir())) - if readErr != nil && !os.IsNotExist(readErr) { - t.Fatalf("read env file: %v", readErr) - } - if strings.Contains(string(envData), envName+"=") { - t.Fatalf("expected persisted env key rollback, got %q", string(envData)) - } - - providerPath := filepath.Join(app.configManager.BaseDir(), "providers", providerName) - if _, err := os.Stat(providerPath); !os.IsNotExist(err) { - t.Fatalf("expected provider dir rollback, stat err = %v", err) - } -} - -func TestSubmitProviderAddFormRestoresExistingStateOnSelectFailure(t *testing.T) { - restorePersistUserEnv := persistProviderUserEnvVar - restoreDeleteUserEnv := deleteProviderUserEnvVar - restoreLookupUserEnv := lookupProviderUserEnvVar - userEnvStore := map[string]string{} - persistProviderUserEnvVar = func(key string, value string) error { - userEnvStore[key] = value - return nil - } - deleteProviderUserEnvVar = func(key string) error { - delete(userEnvStore, key) - return nil - } - lookupProviderUserEnvVar = func(key string) (string, bool, error) { - value, ok := userEnvStore[key] - return value, ok, nil - } - t.Cleanup(func() { persistProviderUserEnvVar = restorePersistUserEnv }) - t.Cleanup(func() { deleteProviderUserEnvVar = restoreDeleteUserEnv }) - t.Cleanup(func() { lookupProviderUserEnvVar = restoreLookupUserEnv }) - - providerName := "restore-existing-provider" - envName := providerAddAPIKeyEnv(providerName) - restoreEnv := captureEnv(t, envName) - defer restoreEnv() - - app, _ := newTestAppWithProviderService(t, stubProviderService{ - selectErr: errors.New("select failed"), - }) - - if err := config.SaveCustomProvider( - app.configManager.BaseDir(), - providerName, - provider.DriverOpenAICompat, - "https://old.example.com/v1", - envName, - provider.OpenAICompatibleAPIStyleChatCompletions, - "", - "", - ); err != nil { - t.Fatalf("SaveCustomProvider() error = %v", err) - } - if err := config.PersistEnvVar(app.configManager.BaseDir(), envName, "old-file-key"); err != nil { - t.Fatalf("PersistEnvVar() error = %v", err) - } - if err := os.Setenv(envName, "old-process-key"); err != nil { - t.Fatalf("Setenv() error = %v", err) - } - userEnvStore[envName] = "old-user-key" - - app.startProviderAddForm() - app.providerAddForm.Name = providerName + app.providerAddForm.Name = "gateway" app.providerAddForm.Driver = provider.DriverOpenAICompat - app.providerAddForm.BaseURL = "https://new.example.com/v1" - app.providerAddForm.APIKey = "new-key" - app.providerAddForm.APIStyle = provider.OpenAICompatibleAPIStyleChatCompletions + app.providerAddForm.BaseURL = "https://example.com/v1" + app.providerAddForm.APIKey = "test-key" + app.providerAddForm.APIKeyEnv = "" cmd := app.submitProviderAddForm() - if cmd == nil { - t.Fatalf("expected async command") - } - msg := cmd() - result, ok := msg.(providerAddResultMsg) - if !ok { - t.Fatalf("expected providerAddResultMsg, got %T", msg) - } - if strings.TrimSpace(result.Error) == "" { - t.Fatalf("expected failure result") - } - - if got := os.Getenv(envName); got != "old-process-key" { - t.Fatalf("expected process env restored, got %q", got) - } - if got := userEnvStore[envName]; got != "old-user-key" { - t.Fatalf("expected user env restored, got %q", got) - } - - envData, readErr := os.ReadFile(config.EnvFilePath(app.configManager.BaseDir())) - if readErr != nil { - t.Fatalf("read env file: %v", readErr) - } - if !strings.Contains(string(envData), envName+"=old-file-key") { - t.Fatalf("expected persisted env restored, got %q", string(envData)) - } - - providerPath := filepath.Join(app.configManager.BaseDir(), "providers", providerName, "provider.yaml") - providerData, readProviderErr := os.ReadFile(providerPath) - if readProviderErr != nil { - t.Fatalf("read provider config: %v", readProviderErr) - } - providerText := string(providerData) - if !strings.Contains(providerText, "https://old.example.com/v1") { - t.Fatalf("expected provider config restored, got %q", providerText) + if cmd != nil { + t.Fatalf("expected nil command for invalid env key") } - if strings.Contains(providerText, "https://new.example.com/v1") { - t.Fatalf("expected new provider config to be rolled back, got %q", providerText) + if !strings.Contains(app.providerAddForm.Error, "API Key Env is required") { + t.Fatalf("expected env key validation error, got %q", app.providerAddForm.Error) } } @@ -612,101 +439,6 @@ func TestTrimLastRune(t *testing.T) { } } -func TestLoadAndRestoreEnvFileSnapshot(t *testing.T) { - tmp := t.TempDir() - envPath := filepath.Join(tmp, ".env") - - missingSnapshot, err := loadFileSnapshot(envPath) - if err != nil { - t.Fatalf("loadFileSnapshot(missing) error = %v", err) - } - if missingSnapshot.Exists { - t.Fatalf("expected missing snapshot") - } - - if err := os.WriteFile(envPath, []byte("A=1\n"), 0o600); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - existingSnapshot, err := loadFileSnapshot(envPath) - if err != nil { - t.Fatalf("loadFileSnapshot(existing) error = %v", err) - } - if !existingSnapshot.Exists { - t.Fatalf("expected existing snapshot") - } - - if err := os.WriteFile(envPath, []byte("A=2\n"), 0o600); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := restoreEnvFileSnapshot(envPath, existingSnapshot); err != nil { - t.Fatalf("restoreEnvFileSnapshot(existing) error = %v", err) - } - data, err := os.ReadFile(envPath) - if err != nil { - t.Fatalf("ReadFile() error = %v", err) - } - if string(data) != "A=1\n" { - t.Fatalf("expected snapshot content restored, got %q", string(data)) - } - - if err := restoreEnvFileSnapshot(envPath, missingSnapshot); err != nil { - t.Fatalf("restoreEnvFileSnapshot(missing) error = %v", err) - } - if _, err := os.Stat(envPath); !os.IsNotExist(err) { - t.Fatalf("expected env file removed for missing snapshot, stat err = %v", err) - } -} - -func TestRestoreProviderConfigSnapshot(t *testing.T) { - baseDir := t.TempDir() - providerName := "snapshot-provider" - providerPath := filepath.Join(baseDir, "providers", providerName, "provider.yaml") - if err := os.MkdirAll(filepath.Dir(providerPath), 0o755); err != nil { - t.Fatalf("MkdirAll() error = %v", err) - } - if err := os.WriteFile(providerPath, []byte("name: old\n"), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - - snapshot, err := loadFileSnapshot(providerPath) - if err != nil { - t.Fatalf("loadFileSnapshot() error = %v", err) - } - if err := os.WriteFile(providerPath, []byte("name: new\n"), 0o644); err != nil { - t.Fatalf("WriteFile() error = %v", err) - } - if err := restoreProviderConfigSnapshot(baseDir, providerName, snapshot); err != nil { - t.Fatalf("restoreProviderConfigSnapshot(existing) error = %v", err) - } - data, err := os.ReadFile(providerPath) - if err != nil { - t.Fatalf("ReadFile() error = %v", err) - } - if string(data) != "name: old\n" { - t.Fatalf("expected provider snapshot restored, got %q", string(data)) - } - - missingSnapshot := fileSnapshot{} - if err := restoreProviderConfigSnapshot(baseDir, providerName, missingSnapshot); err != nil { - t.Fatalf("restoreProviderConfigSnapshot(missing) error = %v", err) - } - if _, err := os.Stat(filepath.Dir(providerPath)); !os.IsNotExist(err) { - t.Fatalf("expected provider dir removed for missing snapshot, stat err = %v", err) - } -} - -func captureEnv(t *testing.T, key string) func() { - t.Helper() - value, exists := os.LookupEnv(key) - return func() { - if exists { - _ = os.Setenv(key, value) - return - } - _ = os.Unsetenv(key) - } -} - func TestAppUpdateBasic(t *testing.T) { app, _ := newTestApp(t) @@ -2622,6 +2354,20 @@ func TestCurrentProviderAddFieldAndInputHandling(t *testing.T) { t.Fatalf("expected name field append, got %q", app.providerAddForm.Name) } + app.providerAddForm.Step = 4 // api key env + model, cmd = app.handleProviderAddFormInput(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'\x00', 'D', 'E', 'E', 'P'}}) + if cmd != nil { + t.Fatalf("expected nil cmd for env key rune input") + } + ptr, ok = model.(*App) + if !ok { + t.Fatalf("expected *App model, got %T", model) + } + app = *ptr + if app.providerAddForm.APIKeyEnv != "DEEP" { + t.Fatalf("expected control chars filtered from env key, got %q", app.providerAddForm.APIKeyEnv) + } + model, _ = app.handleProviderAddFormInput(tea.KeyMsg{Type: tea.KeyTab}) ptr, ok = model.(*App) if !ok { @@ -2728,13 +2474,22 @@ func TestBuildProviderAddRequest(t *testing.T) { if _, err := buildProviderAddRequest(providerAddFormState{Name: "demo", Driver: provider.DriverGemini}); !strings.Contains(err, "API Key is required") { t.Fatalf("expected missing key error, got %q", err) } + if _, err := buildProviderAddRequest(providerAddFormState{ + Name: "demo", + Driver: provider.DriverGemini, + APIKey: "k", + APIKeyEnv: "", + }); !strings.Contains(err, "API Key Env is required") { + t.Fatalf("expected missing env key error, got %q", err) + } }) t.Run("openai compat applies defaults", func(t *testing.T) { req, err := buildProviderAddRequest(providerAddFormState{ - Name: "openai-compat", - Driver: provider.DriverOpenAICompat, - APIKey: "k", + Name: "openai-compat", + Driver: provider.DriverOpenAICompat, + APIKey: "k", + APIKeyEnv: "OPENAI_COMPAT_API_KEY", }) if err != "" { t.Fatalf("unexpected error: %s", err) @@ -2747,11 +2502,27 @@ func TestBuildProviderAddRequest(t *testing.T) { } }) + t.Run("strips control chars from env key before validation", func(t *testing.T) { + req, err := buildProviderAddRequest(providerAddFormState{ + Name: "openai-compat", + Driver: provider.DriverOpenAICompat, + APIKey: "k", + APIKeyEnv: "\x00OPENAI_COMPAT_API_KEY", + }) + if err != "" { + t.Fatalf("unexpected error: %s", err) + } + if req.APIKeyEnv != "OPENAI_COMPAT_API_KEY" { + t.Fatalf("expected sanitized env key, got %q", req.APIKeyEnv) + } + }) + t.Run("gemini applies defaults and clears unrelated fields", func(t *testing.T) { req, err := buildProviderAddRequest(providerAddFormState{ Name: "gemini", Driver: provider.DriverGemini, APIKey: "k", + APIKeyEnv: "GEMINI_GATEWAY_API_KEY", APIStyle: "x", APIVersion: "v", DeploymentMode: "d", @@ -2766,18 +2537,20 @@ func TestBuildProviderAddRequest(t *testing.T) { t.Run("anthropic/custom require base url", func(t *testing.T) { if _, err := buildProviderAddRequest(providerAddFormState{ - Name: "anthropic", - Driver: provider.DriverAnthropic, - APIKey: "k", + Name: "anthropic", + Driver: provider.DriverAnthropic, + APIKey: "k", + APIKeyEnv: "ANTHROPIC_GATEWAY_API_KEY", }); !strings.Contains(err, "Base URL is required") { t.Fatalf("expected anthropic base url error, got %q", err) } if _, err := buildProviderAddRequest(providerAddFormState{ - Name: "custom", - Driver: "custom-driver", - APIKey: "k", - BaseURL: "", + Name: "custom", + Driver: "custom-driver", + APIKey: "k", + APIKeyEnv: "CUSTOM_DRIVER_API_KEY", + BaseURL: "", }); !strings.Contains(err, "Base URL is required for custom driver") { t.Fatalf("expected custom base url error, got %q", err) } diff --git a/internal/tui/core/app/view.go b/internal/tui/core/app/view.go index 83fa314f..f5791ca1 100644 --- a/internal/tui/core/app/view.go +++ b/internal/tui/core/app/view.go @@ -234,6 +234,8 @@ func (a App) renderProviderAddForm() string { fields = append(fields, renderField{label: "Deployment Mode", value: a.providerAddForm.DeploymentMode}) case providerAddFieldAPIVersion: fields = append(fields, renderField{label: "API Version", value: a.providerAddForm.APIVersion}) + case providerAddFieldAPIKeyEnv: + fields = append(fields, renderField{label: "API Key Env", value: a.providerAddForm.APIKeyEnv, required: true}) case providerAddFieldAPIKey: fields = append(fields, renderField{label: "API Key", value: maskedSecret(a.providerAddForm.APIKey), required: true}) } From 8ca9d12587450c19165f6be2cb98ba399b6d5fcb Mon Sep 17 00:00:00 2001 From: xgopilot Date: Fri, 17 Apr 2026 15:07:33 +0000 Subject: [PATCH 2/5] fix(config/state): use service-scoped lock for custom provider create Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com> --- internal/config/state/service.go | 2 ++ internal/config/state/service_provider_create.go | 6 ++---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/config/state/service.go b/internal/config/state/service.go index 4167f387..460e3e06 100644 --- a/internal/config/state/service.go +++ b/internal/config/state/service.go @@ -4,6 +4,7 @@ import ( "context" "errors" "strings" + "sync" "neo-code/internal/config" providertypes "neo-code/internal/provider/types" @@ -14,6 +15,7 @@ type Service struct { manager *config.Manager supporters DriverSupporter catalogs ModelCatalog + createMu sync.Mutex } // NewService 创建选择状态服务实例。 diff --git a/internal/config/state/service_provider_create.go b/internal/config/state/service_provider_create.go index 2dfe36a1..c4311bb6 100644 --- a/internal/config/state/service_provider_create.go +++ b/internal/config/state/service_provider_create.go @@ -7,14 +7,12 @@ import ( "os" "path/filepath" "strings" - "sync" "time" "neo-code/internal/config" "neo-code/internal/provider" ) -var providerCreateFlowMu sync.Mutex var persistUserEnvVarForCreate = config.PersistUserEnvVar var deleteUserEnvVarForCreate = config.DeleteUserEnvVar var lookupUserEnvVarForCreate = config.LookupUserEnvVar @@ -63,8 +61,8 @@ func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomPr return Selection{}, err } - providerCreateFlowMu.Lock() - defer providerCreateFlowMu.Unlock() + s.createMu.Lock() + defer s.createMu.Unlock() cfgSnapshot := s.manager.Get() if err := validateCustomProviderCreateConflict(cfgSnapshot, normalized); err != nil { From d5f67567d81a4ebbce6c5cb26787e23392e31524 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Fri, 17 Apr 2026 15:24:40 +0000 Subject: [PATCH 3/5] fix(config): address provider create lock and env validation regressions Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com> --- internal/config/env_platform_windows.go | 14 ++- internal/config/env_platform_windows_test.go | 24 +++++ internal/config/manager.go | 17 +++- internal/config/state/service.go | 2 - .../config/state/service_provider_create.go | 4 +- .../state/service_provider_create_test.go | 95 +++++++++++++++++++ internal/tui/core/app/update.go | 3 + internal/tui/core/app/update_test.go | 29 ++++++ 8 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 internal/config/env_platform_windows_test.go diff --git a/internal/config/env_platform_windows.go b/internal/config/env_platform_windows.go index 3570f5d2..4d356c30 100644 --- a/internal/config/env_platform_windows.go +++ b/internal/config/env_platform_windows.go @@ -3,6 +3,7 @@ package config import ( + "errors" "fmt" "strings" @@ -42,7 +43,7 @@ func DeleteUserEnvVar(key string) error { envKey, err := registry.OpenKey(registry.CURRENT_USER, windowsUserEnvironmentKey, registry.SET_VALUE) if err != nil { - if err == registry.ErrNotExist { + if isWindowsRegistryNotExist(err) { return nil } return fmt.Errorf("config: open windows user env: %w", err) @@ -50,7 +51,7 @@ func DeleteUserEnvVar(key string) error { defer envKey.Close() if err := envKey.DeleteValue(normalizedKey); err != nil { - if err == registry.ErrNotExist { + if isWindowsRegistryNotExist(err) { return nil } return fmt.Errorf("config: delete windows user env %q: %w", normalizedKey, err) @@ -67,7 +68,7 @@ func LookupUserEnvVar(key string) (string, bool, error) { envKey, err := registry.OpenKey(registry.CURRENT_USER, windowsUserEnvironmentKey, registry.QUERY_VALUE) if err != nil { - if err == registry.ErrNotExist { + if isWindowsRegistryNotExist(err) { return "", false, nil } return "", false, fmt.Errorf("config: open windows user env: %w", err) @@ -76,7 +77,7 @@ func LookupUserEnvVar(key string) (string, bool, error) { value, _, err := envKey.GetStringValue(normalizedKey) if err != nil { - if err == registry.ErrNotExist { + if isWindowsRegistryNotExist(err) { return "", false, nil } return "", false, fmt.Errorf("config: read windows user env %q: %w", normalizedKey, err) @@ -84,6 +85,11 @@ func LookupUserEnvVar(key string) (string, bool, error) { return value, true, nil } +// isWindowsRegistryNotExist 统一判断注册表“键/值不存在”错误，兼容包装错误场景。 +func isWindowsRegistryNotExist(err error) bool { + return errors.Is(err, registry.ErrNotExist) +} + // SupportsUserEnvPersistence 返回当前平台是否支持用户级环境变量持久化。 func SupportsUserEnvPersistence() bool { return true diff --git a/internal/config/env_platform_windows_test.go b/internal/config/env_platform_windows_test.go new file mode 100644 index 00000000..4a150412 --- /dev/null +++ b/internal/config/env_platform_windows_test.go @@ -0,0 +1,24 @@ +//go:build windows + +package config + +import ( + "errors" + "fmt" + "testing" + + "golang.org/x/sys/windows/registry" +) + +func TestIsWindowsRegistryNotExistWrapped(t *testing.T) { + wrapped := fmt.Errorf("wrapped: %w", registry.ErrNotExist) + if !isWindowsRegistryNotExist(wrapped) { + t.Fatal("expected wrapped ErrNotExist to be treated as not-exist") + } +} + +func TestIsWindowsRegistryNotExistMismatch(t *testing.T) { + if isWindowsRegistryNotExist(errors.New("other error")) { + t.Fatal("expected non-ErrNotExist error to be false") + } +} diff --git a/internal/config/manager.go b/internal/config/manager.go index 4837307d..22cbdb6f 100644 --- a/internal/config/manager.go +++ b/internal/config/manager.go @@ -7,9 +7,10 @@ import ( ) type Manager struct { - mu sync.RWMutex - loader *Loader - config *Config + mu sync.RWMutex + providerCreateMu sync.Mutex + loader *Loader + config *Config } func NewManager(loader *Loader) *Manager { @@ -94,3 +95,13 @@ func (m *Manager) BaseDir() string { func (m *Manager) ConfigPath() string { return m.loader.ConfigPath() } + +// LockProviderCreate 锁定自定义 Provider 创建事务，确保同一 Manager 维度串行执行。 +func (m *Manager) LockProviderCreate() { + m.providerCreateMu.Lock() +} + +// UnlockProviderCreate 释放自定义 Provider 创建事务锁。 +func (m *Manager) UnlockProviderCreate() { + m.providerCreateMu.Unlock() +} diff --git a/internal/config/state/service.go b/internal/config/state/service.go index 460e3e06..4167f387 100644 --- a/internal/config/state/service.go +++ b/internal/config/state/service.go @@ -4,7 +4,6 @@ import ( "context" "errors" "strings" - "sync" "neo-code/internal/config" providertypes "neo-code/internal/provider/types" @@ -15,7 +14,6 @@ type Service struct { manager *config.Manager supporters DriverSupporter catalogs ModelCatalog - createMu sync.Mutex } // NewService 创建选择状态服务实例。 diff --git a/internal/config/state/service_provider_create.go b/internal/config/state/service_provider_create.go index c4311bb6..774a618c 100644 --- a/internal/config/state/service_provider_create.go +++ b/internal/config/state/service_provider_create.go @@ -61,8 +61,8 @@ func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomPr return Selection{}, err } - s.createMu.Lock() - defer s.createMu.Unlock() + s.manager.LockProviderCreate() + defer s.manager.UnlockProviderCreate() cfgSnapshot := s.manager.Get() if err := validateCustomProviderCreateConflict(cfgSnapshot, normalized); err != nil { diff --git a/internal/config/state/service_provider_create_test.go b/internal/config/state/service_provider_create_test.go index 61313000..30f4a075 100644 --- a/internal/config/state/service_provider_create_test.go +++ b/internal/config/state/service_provider_create_test.go @@ -5,7 +5,9 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" + "time" configpkg "neo-code/internal/config" "neo-code/internal/provider" @@ -176,6 +178,99 @@ func TestCreateCustomProviderRejectsInvalidProviderName(t *testing.T) { } } +func TestCreateCustomProviderSerializesAcrossServicesSharingManager(t *testing.T) { + restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + + manager := newSelectionTestManager(t, testDefaultConfig()) + failingService := NewService(manager, newDriverSupporterStub(), errorCatalogStub{err: context.DeadlineExceeded}) + successService := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{ + {ID: "shared-model", Name: "shared-model"}, + }, + }) + + reachedPersist := make(chan struct{}) + releasePersist := make(chan struct{}) + var notifyOnce sync.Once + persistUserEnvVarForCreate = func(key string, value string) error { + if value == "key-a" { + notifyOnce.Do(func() { close(reachedPersist) }) + <-releasePersist + } + return nil + } + + inputA := CreateCustomProviderInput{ + Name: "shared-gateway", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://shared.example.com/v1", + APIKeyEnv: "SHARED_GATEWAY_API_KEY", + APIKey: "key-a", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + } + inputB := inputA + inputB.APIKey = "key-b" + + restore := captureEnvForCreateProvider(t, inputA.APIKeyEnv) + defer restore() + _ = os.Unsetenv(inputA.APIKeyEnv) + + errACh := make(chan error, 1) + errBCh := make(chan error, 1) + + go func() { + _, err := failingService.CreateCustomProvider(context.Background(), inputA) + errACh <- err + }() + + select { + case <-reachedPersist: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting first create flow to reach persist stage") + } + + go func() { + _, err := successService.CreateCustomProvider(context.Background(), inputB) + errBCh <- err + }() + + select { + case err := <-errBCh: + t.Fatalf("expected second create to wait for manager lock, got early result err=%v", err) + case <-time.After(120 * time.Millisecond): + } + + close(releasePersist) + + if err := <-errACh; err == nil { + t.Fatal("expected first create to fail on model selection") + } + if err := <-errBCh; err != nil { + t.Fatalf("expected second create to succeed, got %v", err) + } + + cfg := manager.Get() + providerCfg, err := cfg.ProviderByName(inputA.Name) + if err != nil { + t.Fatalf("expected provider %q to exist after serialized create, got %v", inputA.Name, err) + } + if strings.TrimSpace(providerCfg.APIKeyEnv) != inputA.APIKeyEnv { + t.Fatalf("expected provider api_key_env %q, got %q", inputA.APIKeyEnv, providerCfg.APIKeyEnv) + } + + providerPath := filepath.Join(manager.BaseDir(), "providers", inputA.Name, "provider.yaml") + data, readErr := os.ReadFile(providerPath) + if readErr != nil { + t.Fatalf("read provider config: %v", readErr) + } + if !strings.Contains(string(data), "api_key_env: "+inputA.APIKeyEnv) { + t.Fatalf("expected provider config to remain after concurrent create flow, got %q", string(data)) + } +} + func captureEnvForCreateProvider(t *testing.T, key string) func() { t.Helper() diff --git a/internal/tui/core/app/update.go b/internal/tui/core/app/update.go index 19a72607..bc0c4316 100644 --- a/internal/tui/core/app/update.go +++ b/internal/tui/core/app/update.go @@ -2427,6 +2427,9 @@ func buildProviderAddRequest(form providerAddFormState) (providerAddRequest, str if err := config.ValidateEnvVarName(request.APIKeyEnv); err != nil { return providerAddRequest{}, err.Error() } + if config.IsProtectedEnvVarName(request.APIKeyEnv) { + return providerAddRequest{}, fmt.Sprintf("API Key Env %q is protected", request.APIKeyEnv) + } switch request.Driver { case provider.DriverOpenAICompat: diff --git a/internal/tui/core/app/update_test.go b/internal/tui/core/app/update_test.go index 0a940795..2b54cb4a 100644 --- a/internal/tui/core/app/update_test.go +++ b/internal/tui/core/app/update_test.go @@ -427,6 +427,24 @@ func TestSubmitProviderAddFormRequiresAPIKeyEnv(t *testing.T) { } } +func TestSubmitProviderAddFormRejectsProtectedAPIKeyEnv(t *testing.T) { + app, _ := newTestApp(t) + app.startProviderAddForm() + app.providerAddForm.Name = "gateway" + app.providerAddForm.Driver = provider.DriverOpenAICompat + app.providerAddForm.BaseURL = "https://example.com/v1" + app.providerAddForm.APIKey = "test-key" + app.providerAddForm.APIKeyEnv = "PATH" + + cmd := app.submitProviderAddForm() + if cmd != nil { + t.Fatalf("expected nil command for protected env key") + } + if !strings.Contains(app.providerAddForm.Error, "is protected") { + t.Fatalf("expected protected env key validation error, got %q", app.providerAddForm.Error) + } +} + func TestTrimLastRune(t *testing.T) { if got := trimLastRune(""); got != "" { t.Fatalf("trimLastRune(empty) = %q, want empty", got) @@ -2517,6 +2535,17 @@ func TestBuildProviderAddRequest(t *testing.T) { } }) + t.Run("rejects protected env key", func(t *testing.T) { + if _, err := buildProviderAddRequest(providerAddFormState{ + Name: "openai-compat", + Driver: provider.DriverOpenAICompat, + APIKey: "k", + APIKeyEnv: "PATH", + }); !strings.Contains(err, "protected") { + t.Fatalf("expected protected env key error, got %q", err) + } + }) + t.Run("gemini applies defaults and clears unrelated fields", func(t *testing.T) { req, err := buildProviderAddRequest(providerAddFormState{ Name: "gemini", From e4eadfa306107943f3c8b627410709f4fae21fe6 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Fri, 17 Apr 2026 15:43:15 +0000 Subject: [PATCH 4/5] fix(config/state): rollback on save failure and add cross-process create lock Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com> --- .../config/state/service_provider_create.go | 39 ++++- .../state/service_provider_create_test.go | 160 +++++++++++++++++- 2 files changed, 188 insertions(+), 11 deletions(-) diff --git a/internal/config/state/service_provider_create.go b/internal/config/state/service_provider_create.go index 774a618c..04a9ff1a 100644 --- a/internal/config/state/service_provider_create.go +++ b/internal/config/state/service_provider_create.go @@ -16,8 +16,11 @@ import ( var persistUserEnvVarForCreate = config.PersistUserEnvVar var deleteUserEnvVarForCreate = config.DeleteUserEnvVar var lookupUserEnvVarForCreate = config.LookupUserEnvVar +var saveCustomProviderForCreate = config.SaveCustomProvider const providerCreateRollbackReloadTimeout = 3 * time.Second +const providerCreateCrossProcessLockName = ".provider-create.lock" +const providerCreateCrossProcessLockRetryInterval = 50 * time.Millisecond // CreateCustomProviderInput 定义新增自定义 Provider 所需的输入参数。 type CreateCustomProviderInput struct { @@ -64,6 +67,12 @@ func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomPr s.manager.LockProviderCreate() defer s.manager.UnlockProviderCreate() + releaseCrossProcessLock, err := lockProviderCreateCrossProcess(ctx, s.manager.BaseDir()) + if err != nil { + return Selection{}, err + } + defer releaseCrossProcessLock() + cfgSnapshot := s.manager.Get() if err := validateCustomProviderCreateConflict(cfgSnapshot, normalized); err != nil { return Selection{}, err @@ -109,7 +118,8 @@ func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomPr return rolledErr } - if err := config.SaveCustomProvider( + providerSaved = true + if err := saveCustomProviderForCreate( s.manager.BaseDir(), normalized.Name, normalized.Driver, @@ -119,9 +129,8 @@ func (s *Service) CreateCustomProvider(ctx context.Context, input CreateCustomPr normalized.DeploymentMode, normalized.APIVersion, ); err != nil { - return Selection{}, fmt.Errorf("selection: save provider config: %w", err) + return Selection{}, rollback(fmt.Errorf("selection: save provider config: %w", err)) } - providerSaved = true if err := persistUserEnvVarForCreate(normalized.APIKeyEnv, normalized.APIKey); err != nil { return Selection{}, rollback(fmt.Errorf("selection: persist user env: %w", err)) @@ -279,3 +288,27 @@ func restoreProviderConfigSnapshot(baseDir string, providerName string, snapshot } return os.WriteFile(filepath.Join(providerDir, "provider.yaml"), snapshot.Content, 0o644) } + +// lockProviderCreateCrossProcess 使用基于目录的锁，串行化同一工作目录下跨进程的创建流程。 +func lockProviderCreateCrossProcess(ctx context.Context, baseDir string) (func(), error) { + lockPath := filepath.Join(baseDir, providerCreateCrossProcessLockName) + for { + if err := os.Mkdir(lockPath, 0o700); err == nil { + return func() { + _ = os.Remove(lockPath) + }, nil + } else if !os.IsExist(err) { + return nil, fmt.Errorf("selection: acquire create lock: %w", err) + } + + timer := time.NewTimer(providerCreateCrossProcessLockRetryInterval) + select { + case <-ctx.Done(): + if !timer.Stop() { + <-timer.C + } + return nil, ctx.Err() + case <-timer.C: + } + } +} diff --git a/internal/config/state/service_provider_create_test.go b/internal/config/state/service_provider_create_test.go index 30f4a075..a9b6f331 100644 --- a/internal/config/state/service_provider_create_test.go +++ b/internal/config/state/service_provider_create_test.go @@ -15,10 +15,11 @@ import ( ) func TestCreateCustomProviderSuccess(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ @@ -66,10 +67,11 @@ func TestCreateCustomProviderSuccess(t *testing.T) { } func TestCreateCustomProviderRollbackOnSelectFailure(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) service := NewService(manager, newDriverSupporterStub(), errorCatalogStub{err: context.DeadlineExceeded}) @@ -107,10 +109,11 @@ func TestCreateCustomProviderRollbackOnSelectFailure(t *testing.T) { } func TestCreateCustomProviderRejectsEnvConflicts(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ @@ -131,10 +134,11 @@ func TestCreateCustomProviderRejectsEnvConflicts(t *testing.T) { } func TestCreateCustomProviderRejectsProtectedEnvName(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ @@ -155,10 +159,11 @@ func TestCreateCustomProviderRejectsProtectedEnvName(t *testing.T) { } func TestCreateCustomProviderRejectsInvalidProviderName(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ @@ -179,10 +184,11 @@ func TestCreateCustomProviderRejectsInvalidProviderName(t *testing.T) { } func TestCreateCustomProviderSerializesAcrossServicesSharingManager(t *testing.T) { - restorePersist, restoreDelete, restoreLookup := stubUserEnvOpsForCreateProvider(t) + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) defer restorePersist() defer restoreDelete() defer restoreLookup() + defer restoreSave() manager := newSelectionTestManager(t, testDefaultConfig()) failingService := NewService(manager, newDriverSupporterStub(), errorCatalogStub{err: context.DeadlineExceeded}) @@ -271,6 +277,141 @@ func TestCreateCustomProviderSerializesAcrossServicesSharingManager(t *testing.T } } +func TestCreateCustomProviderRollbackOnSaveProviderFailure(t *testing.T) { + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + defer restoreSave() + + manager := newSelectionTestManager(t, testDefaultConfig()) + service := NewService(manager, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{{ID: "m1", Name: "m1"}}, + }) + input := CreateCustomProviderInput{ + Name: "save-failed-provider", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://llm.example.com/v1", + APIKeyEnv: "SAVE_FAILED_PROVIDER_API_KEY", + APIKey: "key", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + } + + saveCustomProviderForCreate = func( + baseDir string, + name string, + driver string, + baseURL string, + apiKeyEnv string, + apiStyle string, + deploymentMode string, + apiVersion string, + ) error { + providerDir := filepath.Join(baseDir, "providers", name) + if err := os.MkdirAll(providerDir, 0o755); err != nil { + return err + } + return context.DeadlineExceeded + } + + if _, err := service.CreateCustomProvider(context.Background(), input); err == nil { + t.Fatal("expected CreateCustomProvider() to fail when saving provider config") + } + + providerDir := filepath.Join(manager.BaseDir(), "providers", input.Name) + if _, statErr := os.Stat(providerDir); !os.IsNotExist(statErr) { + t.Fatalf("expected rollback to remove provider dir, stat err = %v", statErr) + } + if got := strings.TrimSpace(os.Getenv(input.APIKeyEnv)); got != "" { + t.Fatalf("expected process env to stay untouched, got %q", got) + } + cfg := manager.Get() + if _, err := cfg.ProviderByName(input.Name); err == nil { + t.Fatalf("expected provider %q absent after save failure rollback", input.Name) + } +} + +func TestCreateCustomProviderSerializesAcrossManagersSharingBaseDir(t *testing.T) { + restorePersist, restoreDelete, restoreLookup, restoreSave := stubUserEnvOpsForCreateProvider(t) + defer restorePersist() + defer restoreDelete() + defer restoreLookup() + defer restoreSave() + + baseDir := t.TempDir() + loaderA := configpkg.NewLoader(baseDir, testDefaultConfig()) + loaderB := configpkg.NewLoader(baseDir, testDefaultConfig()) + managerA := configpkg.NewManager(loaderA) + managerB := configpkg.NewManager(loaderB) + if _, err := managerA.Load(context.Background()); err != nil { + t.Fatalf("managerA.Load() error = %v", err) + } + if _, err := managerB.Load(context.Background()); err != nil { + t.Fatalf("managerB.Load() error = %v", err) + } + + failingService := NewService(managerA, newDriverSupporterStub(), errorCatalogStub{err: context.DeadlineExceeded}) + successService := NewService(managerB, newDriverSupporterStub(), catalogMethodsStub{ + listModels: []providertypes.ModelDescriptor{{ID: "m1", Name: "m1"}}, + }) + + reachedPersist := make(chan struct{}) + releasePersist := make(chan struct{}) + var notifyOnce sync.Once + persistUserEnvVarForCreate = func(key string, value string) error { + if value == "key-a" { + notifyOnce.Do(func() { close(reachedPersist) }) + <-releasePersist + } + return nil + } + + inputA := CreateCustomProviderInput{ + Name: "shared-by-managers", + Driver: provider.DriverOpenAICompat, + BaseURL: "https://shared.example.com/v1", + APIKeyEnv: "SHARED_BY_MANAGERS_API_KEY", + APIKey: "key-a", + APIStyle: provider.OpenAICompatibleAPIStyleChatCompletions, + } + inputB := inputA + inputB.APIKey = "key-b" + + errACh := make(chan error, 1) + errBCh := make(chan error, 1) + + go func() { + _, err := failingService.CreateCustomProvider(context.Background(), inputA) + errACh <- err + }() + + select { + case <-reachedPersist: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting first create flow to reach persist stage") + } + + go func() { + _, err := successService.CreateCustomProvider(context.Background(), inputB) + errBCh <- err + }() + + select { + case err := <-errBCh: + t.Fatalf("expected second manager create to wait for cross-process lock, got early err=%v", err) + case <-time.After(120 * time.Millisecond): + } + + close(releasePersist) + + if err := <-errACh; err == nil { + t.Fatal("expected first manager create to fail on model selection") + } + if err := <-errBCh; err != nil { + t.Fatalf("expected second manager create to succeed, got %v", err) + } +} + func captureEnvForCreateProvider(t *testing.T, key string) func() { t.Helper() @@ -284,18 +425,21 @@ func captureEnvForCreateProvider(t *testing.T, key string) func() { } } -func stubUserEnvOpsForCreateProvider(t *testing.T) (func(), func(), func()) { +func stubUserEnvOpsForCreateProvider(t *testing.T) (func(), func(), func(), func()) { t.Helper() prevPersist := persistUserEnvVarForCreate prevDelete := deleteUserEnvVarForCreate prevLookup := lookupUserEnvVarForCreate + prevSave := saveCustomProviderForCreate persistUserEnvVarForCreate = func(key string, value string) error { return nil } deleteUserEnvVarForCreate = func(key string) error { return nil } lookupUserEnvVarForCreate = func(key string) (string, bool, error) { return "", false, nil } + saveCustomProviderForCreate = configpkg.SaveCustomProvider return func() { persistUserEnvVarForCreate = prevPersist }, func() { deleteUserEnvVarForCreate = prevDelete }, - func() { lookupUserEnvVarForCreate = prevLookup } + func() { lookupUserEnvVarForCreate = prevLookup }, + func() { saveCustomProviderForCreate = prevSave } } From 7caf4562abd4a01678ce193bd387e846f01919c3 Mon Sep 17 00:00:00 2001 From: xgopilot Date: Fri, 17 Apr 2026 16:33:51 +0000 Subject: [PATCH 5/5] fix(config/state): recover stale provider create lock after unclean exit Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com> --- .../config/state/service_provider_create.go | 85 ++++++++++++++++++- .../state/service_provider_create_test.go | 51 +++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) diff --git a/internal/config/state/service_provider_create.go b/internal/config/state/service_provider_create.go index 04a9ff1a..aa6a07f4 100644 --- a/internal/config/state/service_provider_create.go +++ b/internal/config/state/service_provider_create.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" "time" "neo-code/internal/config" @@ -20,7 +21,10 @@ var saveCustomProviderForCreate = config.SaveCustomProvider const providerCreateRollbackReloadTimeout = 3 * time.Second const providerCreateCrossProcessLockName = ".provider-create.lock" +const providerCreateCrossProcessLockLeaseName = ".lease" const providerCreateCrossProcessLockRetryInterval = 50 * time.Millisecond +const providerCreateCrossProcessLockStaleThreshold = 30 * time.Second +const providerCreateCrossProcessLockHeartbeatInterval = 2 * time.Second // CreateCustomProviderInput 定义新增自定义 Provider 所需的输入参数。 type CreateCustomProviderInput struct { @@ -294,12 +298,25 @@ func lockProviderCreateCrossProcess(ctx context.Context, baseDir string) (func() lockPath := filepath.Join(baseDir, providerCreateCrossProcessLockName) for { if err := os.Mkdir(lockPath, 0o700); err == nil { + if err := touchProviderCreateLockLease(lockPath, time.Now()); err != nil { + _ = os.RemoveAll(lockPath) + return nil, fmt.Errorf("selection: initialize create lock lease: %w", err) + } + stopHeartbeat := startProviderCreateLockHeartbeat(lockPath) return func() { - _ = os.Remove(lockPath) + stopHeartbeat() + _ = os.RemoveAll(lockPath) }, nil } else if !os.IsExist(err) { return nil, fmt.Errorf("selection: acquire create lock: %w", err) } + reclaimed, reclaimErr := tryReclaimStaleProviderCreateLock(lockPath, time.Now()) + if reclaimErr != nil { + return nil, fmt.Errorf("selection: reclaim stale create lock: %w", reclaimErr) + } + if reclaimed { + continue + } timer := time.NewTimer(providerCreateCrossProcessLockRetryInterval) select { @@ -312,3 +329,69 @@ func lockProviderCreateCrossProcess(ctx context.Context, baseDir string) (func() } } } + +// startProviderCreateLockHeartbeat 周期性刷新锁租约，避免长流程被误判为陈旧锁。 +func startProviderCreateLockHeartbeat(lockPath string) func() { + done := make(chan struct{}) + var once sync.Once + + go func() { + ticker := time.NewTicker(providerCreateCrossProcessLockHeartbeatInterval) + defer ticker.Stop() + for { + select { + case <-done: + return + case <-ticker.C: + _ = touchProviderCreateLockLease(lockPath, time.Now()) + } + } + }() + + return func() { + once.Do(func() { + close(done) + }) + } +} + +// tryReclaimStaleProviderCreateLock 尝试回收超时未续租的锁目录。 +func tryReclaimStaleProviderCreateLock(lockPath string, now time.Time) (bool, error) { + leasePath := providerCreateLockLeasePath(lockPath) + info, err := os.Stat(leasePath) + if err != nil { + if !os.IsNotExist(err) { + return false, err + } + // 兼容旧锁：租约文件缺失时使用目录时间判定陈旧。 + info, err = os.Stat(lockPath) + if err != nil { + if os.IsNotExist(err) { + return true, nil + } + return false, err + } + } + + if now.Sub(info.ModTime()) < providerCreateCrossProcessLockStaleThreshold { + return false, nil + } + if err := os.RemoveAll(lockPath); err != nil && !os.IsNotExist(err) { + return false, err + } + return true, nil +} + +// touchProviderCreateLockLease 写入/刷新锁租约时间戳。 +func touchProviderCreateLockLease(lockPath string, now time.Time) error { + leasePath := providerCreateLockLeasePath(lockPath) + if err := os.WriteFile(leasePath, []byte(now.UTC().Format(time.RFC3339Nano)), 0o600); err != nil { + return err + } + return os.Chtimes(leasePath, now, now) +} + +// providerCreateLockLeasePath 返回锁租约文件路径。 +func providerCreateLockLeasePath(lockPath string) string { + return filepath.Join(lockPath, providerCreateCrossProcessLockLeaseName) +} diff --git a/internal/config/state/service_provider_create_test.go b/internal/config/state/service_provider_create_test.go index a9b6f331..3a3beace 100644 --- a/internal/config/state/service_provider_create_test.go +++ b/internal/config/state/service_provider_create_test.go @@ -2,6 +2,7 @@ package state import ( "context" + "errors" "os" "path/filepath" "strings" @@ -412,6 +413,56 @@ func TestCreateCustomProviderSerializesAcrossManagersSharingBaseDir(t *testing.T } } +func TestLockProviderCreateCrossProcessReclaimsStaleLock(t *testing.T) { + baseDir := t.TempDir() + lockPath := filepath.Join(baseDir, providerCreateCrossProcessLockName) + if err := os.Mkdir(lockPath, 0o700); err != nil { + t.Fatalf("Mkdir() error = %v", err) + } + + staleTime := time.Now().Add(-providerCreateCrossProcessLockStaleThreshold - time.Second) + if err := touchProviderCreateLockLease(lockPath, staleTime); err != nil { + t.Fatalf("touchProviderCreateLockLease() error = %v", err) + } + + release, err := lockProviderCreateCrossProcess(context.Background(), baseDir) + if err != nil { + t.Fatalf("lockProviderCreateCrossProcess() error = %v", err) + } + defer release() + + leaseInfo, statErr := os.Stat(providerCreateLockLeasePath(lockPath)) + if statErr != nil { + t.Fatalf("Stat() lease error = %v", statErr) + } + if !leaseInfo.ModTime().After(staleTime) { + t.Fatalf("expected reclaimed lock lease modtime after stale time, got %v <= %v", leaseInfo.ModTime(), staleTime) + } +} + +func TestLockProviderCreateCrossProcessWaitsForFreshLockUntilContextDone(t *testing.T) { + baseDir := t.TempDir() + lockPath := filepath.Join(baseDir, providerCreateCrossProcessLockName) + if err := os.Mkdir(lockPath, 0o700); err != nil { + t.Fatalf("Mkdir() error = %v", err) + } + if err := touchProviderCreateLockLease(lockPath, time.Now()); err != nil { + t.Fatalf("touchProviderCreateLockLease() error = %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Millisecond) + defer cancel() + + release, err := lockProviderCreateCrossProcess(ctx, baseDir) + if err == nil { + release() + t.Fatal("expected lockProviderCreateCrossProcess() to fail on context timeout") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected context deadline exceeded, got %v", err) + } +} + func captureEnvForCreateProvider(t *testing.T, key string) func() { t.Helper()