diff --git a/cmd/ocgo/main.go b/cmd/ocgo/main.go index d3ef1ec..d44fa7e 100644 --- a/cmd/ocgo/main.go +++ b/cmd/ocgo/main.go @@ -3298,11 +3298,28 @@ func saveConfig(cfg Config) error { } func loadConfig() (Config, error) { - cfg := Config{Host: defaultHost, Port: defaultPort, APIKey: os.Getenv("OCGO_API_KEY")} + // Precedence: explicit env var > config file > defaults. + // + // OCGO_API_KEY used to be seeded as the struct default BEFORE + // json.Unmarshal read the config file, so a stale `api_key` in + // config.json silently overwrote it -- the exact opposite of what the + // README promises ("You can also provide the key at runtime with an + // environment variable"). Setting the documented variable and watching + // every request still fail `401 Invalid API key`, with no indication + // which credential was actually used, is a rough way to lose an hour. + // + // Reading the file first and letting a non-empty env var win afterwards + // matches both the documentation and the usual 12-factor ordering, and + // makes it possible to source the key from a secret store at run time + // without ever writing it to disk. + cfg := Config{Host: defaultHost, Port: defaultPort} b, err := os.ReadFile(configFile()) if err == nil { _ = json.Unmarshal(b, &cfg) } + if k := strings.TrimSpace(os.Getenv("OCGO_API_KEY")); k != "" { + cfg.APIKey = k + } if cfg.APIKey == "" { return cfg, errors.New("missing API key; run: ocgo setup") } diff --git a/cmd/ocgo/main_test.go b/cmd/ocgo/main_test.go index c5d6208..b37e103 100644 --- a/cmd/ocgo/main_test.go +++ b/cmd/ocgo/main_test.go @@ -1140,3 +1140,61 @@ func TestSanitizeRawChatToolMessagesDropsLateToolMessage(t *testing.T) { t.Fatalf("expected assistant after placeholder, got %+v", roles[2]) } } + +// writeTempConfig points HOME at a temp dir (configFile() resolves through +// os.UserHomeDir) and writes a config.json holding fileKey. +func writeTempConfig(t *testing.T, fileKey string) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + dir := filepath.Join(home, ".config", "ocgo") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + body := fmt.Sprintf(`{"api_key":%q,"host":"127.0.0.1","port":3456}`, fileKey) + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestEnvKeyOverridesConfigFile(t *testing.T) { + // The regression this pins: OCGO_API_KEY used to be seeded as the struct + // default BEFORE the config file was unmarshalled over it, so a stale + // api_key silently won and every request failed 401 with no indication + // which credential was used -- while the README says the env var works. + writeTempConfig(t, "sk-stale-from-file") + t.Setenv("OCGO_API_KEY", "sk-fresh-from-env") + cfg, err := loadConfig() + if err != nil { + t.Fatal(err) + } + if cfg.APIKey != "sk-fresh-from-env" { + t.Fatalf("APIKey = %q, want the env var to win over the config file", cfg.APIKey) + } +} + +func TestConfigFileUsedWhenEnvUnset(t *testing.T) { + writeTempConfig(t, "sk-from-file") + t.Setenv("OCGO_API_KEY", "") + cfg, err := loadConfig() + if err != nil { + t.Fatal(err) + } + if cfg.APIKey != "sk-from-file" { + t.Fatalf("APIKey = %q, want the config file value when no env var is set", cfg.APIKey) + } +} + +func TestBlankEnvDoesNotClobberConfigFile(t *testing.T) { + // An exported-but-empty OCGO_API_KEY (e.g. `export OCGO_API_KEY=` in a + // profile) must not blank out a working config file. + writeTempConfig(t, "sk-from-file") + t.Setenv("OCGO_API_KEY", " ") + cfg, err := loadConfig() + if err != nil { + t.Fatal(err) + } + if cfg.APIKey != "sk-from-file" { + t.Fatalf("APIKey = %q, want a whitespace-only env var ignored", cfg.APIKey) + } +} diff --git a/ocgo b/ocgo new file mode 100755 index 0000000..7fe1723 Binary files /dev/null and b/ocgo differ