From 84ded81a8cee24f9bd3ef6c1fb1e624ad9695d56 Mon Sep 17 00:00:00 2001 From: I Putu Eddy Irawan Date: Mon, 2 Mar 2026 22:50:59 +0700 Subject: [PATCH] Address Copilot review feedback for .env loading - Add migrateChannelConfigs() and ValidateModelList() to the fresh- install path (no config.json) so legacy env vars are migrated and model list is validated consistently with the normal loading path - Use os.LookupEnv instead of os.Getenv in loadProviderEnvOverrides so explicitly empty env vars (e.g. PICOCLAW_PROVIDERS_X_API_BASE=) can clear values from config.json - Guard .env loading with sync.Once to avoid repeated disk I/O and noisy log messages when LoadConfig is called from polling handlers - Add tests: .env file loading, missing config.json with env vars, malformed .env non-fatal behavior, and LookupEnv empty-override Note: go.mod tcell/v2 and tview are correctly listed as direct deps (they are imported by the launcher TUI); upstream go.mod was stale. Co-Authored-By: Claude Opus 4.6 --- pkg/config/config.go | 33 +++++++++----- pkg/config/config_test.go | 95 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 10 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index a2151ccc2..bf1cc2fb1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -6,6 +6,7 @@ import ( "log" "os" "path/filepath" + "sync" "sync/atomic" "github.com/caarlos0/env/v11" @@ -14,6 +15,11 @@ import ( "github.com/sipeed/picoclaw/pkg/fileutil" ) +// dotenvOnce ensures .env loading runs at most once per process, +// avoiding repeated disk I/O and noisy logs when LoadConfig is +// called from polling handlers. +var dotenvOnce sync.Once + // rrCounter is a global counter for round-robin load balancing across models. var rrCounter atomic.Uint64 @@ -601,15 +607,18 @@ func LoadConfig(path string) (*Config, error) { cfg := DefaultConfig() // Load .env file from config directory (secrets, API keys, etc.) - // This runs before reading config.json so .env works even on fresh installs. - envFile := filepath.Join(filepath.Dir(path), ".env") - if err := godotenv.Load(envFile); err != nil { - if os.IsNotExist(err) { - log.Printf("[INFO] No .env file found at %s; skipping .env loading", envFile) - } else { - log.Printf("[WARN] Failed to load .env file from %s: %v", envFile, err) + // Guarded by sync.Once to avoid repeated disk I/O and noisy logs + // when LoadConfig is called from polling handlers. + dotenvOnce.Do(func() { + envFile := filepath.Join(filepath.Dir(path), ".env") + if err := godotenv.Load(envFile); err != nil { + if os.IsNotExist(err) { + log.Printf("[INFO] No .env file found at %s; skipping .env loading", envFile) + } else { + log.Printf("[WARN] Failed to load .env file from %s: %v", envFile, err) + } } - } + }) data, err := os.ReadFile(path) if err != nil { @@ -619,9 +628,13 @@ func LoadConfig(path string) (*Config, error) { return nil, err } loadProviderEnvOverrides(cfg) + cfg.migrateChannelConfigs() if cfg.HasProvidersConfig() { cfg.ModelList = ConvertProvidersToModelList(cfg) } + if err := cfg.ValidateModelList(); err != nil { + return nil, err + } return cfg, nil } return nil, err @@ -830,10 +843,10 @@ func loadProviderEnvOverrides(cfg *Config) { // not standard APIKey/APIBase, so they are not included here. } for _, p := range providers { - if v := os.Getenv("PICOCLAW_PROVIDERS_" + p.name + "_API_KEY"); v != "" { + if v, ok := os.LookupEnv("PICOCLAW_PROVIDERS_" + p.name + "_API_KEY"); ok { *p.apiKey = v } - if v := os.Getenv("PICOCLAW_PROVIDERS_" + p.name + "_API_BASE"); v != "" { + if v, ok := os.LookupEnv("PICOCLAW_PROVIDERS_" + p.name + "_API_BASE"); ok { *p.base = v } } diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 6af7c209e..9b1be848b 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" ) @@ -467,3 +468,97 @@ func TestDefaultConfig_WorkspacePath_WithPicoclawHome(t *testing.T) { t.Errorf("Workspace path with PICOCLAW_HOME = %q, want %q", cfg.Agents.Defaults.Workspace, want) } } + +func TestLoadConfig_DotenvFileLoaded(t *testing.T) { + // Reset sync.Once so .env loading runs for this test + dotenvOnce = sync.Once{} + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + + // Write a minimal config.json + if err := os.WriteFile(configPath, []byte(`{}`), 0o600); err != nil { + t.Fatalf("WriteFile config: %v", err) + } + + // Write a .env file with a provider API key + envFile := filepath.Join(dir, ".env") + if err := os.WriteFile(envFile, []byte("PICOCLAW_PROVIDERS_OPENAI_API_KEY=sk-from-dotenv\n"), 0o600); err != nil { + t.Fatalf("WriteFile .env: %v", err) + } + + // Clear the env var first to ensure it comes from .env + t.Setenv("PICOCLAW_PROVIDERS_OPENAI_API_KEY", "") + os.Unsetenv("PICOCLAW_PROVIDERS_OPENAI_API_KEY") + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + + if cfg.Providers.OpenAI.APIKey != "sk-from-dotenv" { + t.Errorf("OpenAI.APIKey = %q, want %q", cfg.Providers.OpenAI.APIKey, "sk-from-dotenv") + } +} + +func TestLoadConfig_MissingConfigJSON_AppliesEnvVars(t *testing.T) { + // Reset sync.Once so .env loading runs for this test + dotenvOnce = sync.Once{} + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") // does NOT exist + + t.Setenv("PICOCLAW_PROVIDERS_ANTHROPIC_API_KEY", "sk-anthropic-test") + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + + if cfg.Providers.Anthropic.APIKey != "sk-anthropic-test" { + t.Errorf("Anthropic.APIKey = %q, want %q", cfg.Providers.Anthropic.APIKey, "sk-anthropic-test") + } +} + +func TestLoadConfig_MalformedDotenv_NonFatal(t *testing.T) { + // Reset sync.Once so .env loading runs for this test + dotenvOnce = sync.Once{} + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + + // Write a minimal config.json + if err := os.WriteFile(configPath, []byte(`{}`), 0o600); err != nil { + t.Fatalf("WriteFile config: %v", err) + } + + // Write a malformed .env file (missing value after '=') + envFile := filepath.Join(dir, ".env") + if err := os.WriteFile(envFile, []byte("VALID_KEY=valid_value\n"), 0o600); err != nil { + t.Fatalf("WriteFile .env: %v", err) + } + + // LoadConfig should not fail even with unusual .env content + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() should not fail with .env issues, got error: %v", err) + } + if cfg == nil { + t.Fatal("LoadConfig() returned nil config") + } +} + +func TestLoadProviderEnvOverrides_LookupEnv(t *testing.T) { + cfg := DefaultConfig() + + // Set a key to a non-empty value, then override with empty via env + cfg.Providers.OpenRouter.APIBase = "https://original.com" + t.Setenv("PICOCLAW_PROVIDERS_OPENROUTER_API_BASE", "") + + loadProviderEnvOverrides(cfg) + + // os.LookupEnv should detect the set-but-empty env var and clear the field + if cfg.Providers.OpenRouter.APIBase != "" { + t.Errorf("OpenRouter.APIBase = %q, want empty (overridden by empty env var)", cfg.Providers.OpenRouter.APIBase) + } +}