feat(config): resolve api_key/auth_token from a command (#236)

.github/workflows/ci.yml

@@ -110,6 +110,55 @@ jobs:
           echo "$HELP" | grep -q "rules"
           rm -f ./opencodereview
 
+  # Runs the suite natively on Windows, which the cross-compile job below cannot
+  # do: it only proves the windows arms of the build-tag splits compile. GitHub
+  # does not support `container:` on Windows runners
+  # (actions/runner#904), so this job installs Go directly instead of reusing the
+  # golang:1.26.5 image the other jobs share.
+  windows:
+    runs-on: windows-latest
+    timeout-minutes: 20
+    steps:
+      - uses: actions/checkout@v7
+
+      - uses: actions/setup-go@v7
+        with:
+          go-version: '1.26.5'
+          cache: true
+
+      - name: Vet
+        run: go vet ./...
+
+      # No -race here: the race detector needs a working C toolchain on Windows,
+      # and races are OS-independent, so the Linux job above already covers them.
+      # This job is here for the OS-specific behavior instead. No coverage gate
+      # either -- the //go:build !windows test files legitimately drop the total
+      # below the 80% the Linux job enforces.
+      - name: Test
+        run: go test -count=1 ./...
+
+      - name: Build
+        run: go build -o opencodereview.exe ./cmd/opencodereview
+
+      # Same assertions as the Linux smoke test, under git-bash so the script is
+      # shared verbatim rather than reimplemented in PowerShell.
+      - name: Smoke test
+        shell: bash
+        run: |
+          ./opencodereview.exe --version
+          ./opencodereview.exe --version | grep -q "open-code-review"
+          HELP=$(./opencodereview.exe --help)
+          echo "$HELP" | grep -q "Commands:"
+          echo "$HELP" | grep -q "review"
+          echo "$HELP" | grep -q "scan"
+          echo "$HELP" | grep -q "delegate"
+          echo "$HELP" | grep -q "config"
+          echo "$HELP" | grep -q "llm"
+          echo "$HELP" | grep -q "viewer"
+          echo "$HELP" | grep -q "session"
+          echo "$HELP" | grep -q "rules"
+          rm -f ./opencodereview.exe
+
   cross-compile:
     runs-on: self-hosted
     timeout-minutes: 10

cmd/opencodereview/background_file_test.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 )
@@ -40,7 +41,14 @@ func TestResolveBackgroundFilePath(t *testing.T) {
 	})
 
 	t.Run("absolute unchanged", func(t *testing.T) {
+		// FromSlash is not enough on its own: it only swaps separators, and
+		// `\etc\context.md` is rooted but not absolute on Windows, where
+		// filepath.IsAbs wants a volume. Without the drive letter this case
+		// exercised the relative branch instead of the one it names.
 		abs := filepath.FromSlash("/etc/context.md")
+		if runtime.GOOS == "windows" {
+			abs = `C:\etc\context.md`
+		}
 		if got := resolveBackgroundFilePath(repo, abs); got != abs {
 			t.Errorf("resolveBackgroundFilePath = %q, want %q (absolute must be untouched)", got, abs)
 		}

cmd/opencodereview/config_cmd.go

@@ -126,8 +126,7 @@ func runConfigSet(key, value string) error {
 	}
 
 	displayValue := value
-	normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", ""))
-	if strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken") {
+	if shouldMaskConfigValue(key) {
 		displayValue = maskKey(value)
 	}
 	fmt.Printf("Set %s = %s\n", key, displayValue)
@@ -137,6 +136,15 @@ func runConfigSet(key, value string) error {
 	return nil
 }
 
+// shouldMaskConfigValue reports whether the echoed value of a config key holds a
+// secret and must be masked. Matching on the normalized suffix covers both
+// snake_case and Go field spellings of api_key/auth_token at any path depth,
+// while the *_cmd variants stay unmasked: a command line is not a secret.
+func shouldMaskConfigValue(key string) bool {
+	normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", ""))
+	return strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken")
+}
+
 func runConfigUnset(key string) error {
 	configPath, err := defaultConfigPath()
 	if err != nil {
@@ -285,6 +293,7 @@ func deleteCustomProvider(cfg *Config, name string) (bool, error) {
 // ProviderEntry holds per-provider configuration in the providers map.
 type ProviderEntry struct {
 	APIKey       string            `json:"api_key,omitempty"`
+	APIKeyCmd    string            `json:"api_key_cmd,omitempty"` // shell command whose stdout is the api key; used when api_key is empty
 	URL          string            `json:"url,omitempty"`
 	Protocol     string            `json:"protocol,omitempty"`
 	Model        string            `json:"model,omitempty"`
@@ -325,6 +334,7 @@ type Config struct {
 type LlmConfig struct {
 	URL          string            `json:"url,omitempty"`
 	AuthToken    string            `json:"auth_token,omitempty"`
+	AuthTokenCmd string            `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty
 	AuthHeader   string            `json:"auth_header,omitempty"`
 	Model        string            `json:"model,omitempty"`
 	Protocol     string            `json:"protocol,omitempty"`      // canonical protocol name; takes priority over UseAnthropic
@@ -386,6 +396,7 @@ var supportedConfigKeys = []string{
 	"mcp_servers.<name>.<field>",
 	"llm.url",
 	"llm.auth_token",
+	"llm.auth_token_cmd",
 	"llm.auth_header",
 	"llm.model",
 	"llm.protocol",
@@ -463,6 +474,8 @@ func setConfigValue(cfg *Config, key, value string) error {
 		cfg.Llm.URL = value
 	case "llm.auth_token", "llm.AuthToken":
 		cfg.Llm.AuthToken = value
+	case "llm.auth_token_cmd", "llm.AuthTokenCmd":
+		cfg.Llm.AuthTokenCmd = value
 	case "llm.auth_header", "llm.AuthHeader":
 		normalized, err := llm.NormalizeAuthHeader(value)
 		if err != nil {
@@ -546,7 +559,7 @@ func setConfigValue(cfg *Config, key, value string) error {
 		}
 		cfg.Llm.RetryCodes = codes
 	default:
-		return fmt.Errorf("unknown config key: %s\nSupported keys: %s\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key, strings.Join(supportedConfigKeys, ", "))
+		return fmt.Errorf("unknown config key: %s\nSupported keys: %s\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key, strings.Join(supportedConfigKeys, ", "))
 	}
 	return nil
 }
@@ -555,6 +568,8 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error {
 	switch field {
 	case "api_key":
 		entry.APIKey = value
+	case "api_key_cmd":
+		entry.APIKeyCmd = value
 	case "url":
 		trimmedURL := strings.TrimSpace(value)
 		if trimmedURL != "" {
@@ -605,7 +620,7 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error {
 		}
 		entry.RetryCodes = codes
 	default:
-		return fmt.Errorf("unknown provider field %q: supported fields are api_key, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes", field)
+		return fmt.Errorf("unknown provider field %q: supported fields are api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes", field)
 	}
 	return nil
 }

cmd/opencodereview/config_cmd_test.go

@@ -151,6 +151,56 @@ func TestSetConfigValueProviderEntry(t *testing.T) {
 	}
 }
 
+func TestSetConfigValueKeyCmdFields(t *testing.T) {
+	// A typo in any of these case labels would silently degrade to "unknown
+	// provider field" / "unknown config key", so assert the field each key writes.
+	const value = "op read op://dev/anthropic/api-key"
+	tests := []struct {
+		name string
+		key  string
+		got  func(cfg *Config) string
+	}{
+		{"preset provider api_key_cmd", "providers.anthropic.api_key_cmd", func(cfg *Config) string { return cfg.Providers["anthropic"].APIKeyCmd }},
+		{"custom provider api_key_cmd", "custom_providers.my-gateway.api_key_cmd", func(cfg *Config) string { return cfg.CustomProviders["my-gateway"].APIKeyCmd }},
+		{"llm auth_token_cmd", "llm.auth_token_cmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }},
+		{"llm AuthTokenCmd alias", "llm.AuthTokenCmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := &Config{}
+			if err := setConfigValue(cfg, tt.key, value); err != nil {
+				t.Fatalf("setConfigValue %s: %v", tt.key, err)
+			}
+			if got := tt.got(cfg); got != value {
+				t.Errorf("%s = %q, want %q", tt.key, got, value)
+			}
+		})
+	}
+}
+
+func TestShouldMaskConfigValue(t *testing.T) {
+	// api_key/auth_token values are secrets; the *_cmd variants are command
+	// lines, so they print unmasked.
+	tests := []struct {
+		key  string
+		want bool
+	}{
+		{"llm.auth_token", true},
+		{"llm.auth_token_cmd", false},
+		{"providers.x.api_key", true},
+		{"providers.x.api_key_cmd", false},
+		{"providers.x.APIKeyCmd", false},
+		{"llm.AuthToken", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			if got := shouldMaskConfigValue(tt.key); got != tt.want {
+				t.Errorf("shouldMaskConfigValue(%q) = %v, want %v", tt.key, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestSetConfigValueProviderEntryNonPresetWritesCustomProvider(t *testing.T) {
 	cfg := &Config{}
 
@@ -1018,8 +1068,8 @@ func TestSetConfigValueUnknownKeyMessage(t *testing.T) {
 		t.Fatal("expected error for unknown key")
 	}
 	want := "unknown config key: bogus.key\n" +
-		"Supported keys: provider, model, max_tokens, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, llm.retry_codes, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" +
-		"Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\n" +
+		"Supported keys: provider, model, max_tokens, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, llm.retry_codes, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" +
+		"Provider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\n" +
 		"Protocol values: anthropic, openai, openai-responses\n" +
 		"MCP server fields: type, command, args, env, url, headers, tools, setup"
 	if err.Error() != want {

cmd/opencodereview/provider_cmd.go

@@ -239,13 +239,16 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider
 
 	preset, isPreset := llm.LookupProvider(result.provider)
 
-	if result.apiKey == "" {
+	// Mirror the resolver's precedence (static api_key -> api_key_cmd -> env var):
+	// an already-configured api_key_cmd satisfies the requirement, so picking a
+	// model for such a provider must not fail and abandon the save.
+	if result.apiKey == "" && cfg.Providers[result.provider].APIKeyCmd == "" {
 		if isPreset && preset.EnvVar != "" {
 			if os.Getenv(preset.EnvVar) == "" {
-				return fmt.Errorf("API key is required for provider %s (configure it or set $%s)", result.provider, preset.EnvVar)
+				return fmt.Errorf("API key is required for provider %s (configure it, set providers.%s.api_key_cmd, or set $%s)", result.provider, result.provider, preset.EnvVar)
 			}
 		} else {
-			return fmt.Errorf("API key is required for provider %s", result.provider)
+			return fmt.Errorf("API key is required for provider %s (configure it or set providers.%s.api_key_cmd)", result.provider, result.provider)
 		}
 	}
 
@@ -261,7 +264,8 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider
 	if result.apiKey != "" {
 		entry.APIKey = result.apiKey
 	} else {
-		// Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR.
+		// Confirmed empty key: clear saved api_key so the resolver falls back to
+		// api_key_cmd (when set) or $ENV_VAR.
 		entry.APIKey = ""
 	}
 	cfg.Providers[result.provider] = entry

cmd/opencodereview/provider_cmd_test.go

@@ -8,9 +8,37 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 )
 
+// isolateLLMConnectionTest keeps the "Testing connection..." step that ends
+// every apply*Config call away from the developer's own machine. Without it
+// resolveConfigPath() falls back to ~/.opencodereview/config.json and `go test`
+// resolves a real endpoint: with providers.<name>.api_key_cmd configured that
+// runs the credential helper and blocks on a pinentry/Touch ID prompt for up to
+// the 60s credential timeout, and with a static key it fires a real request.
+//
+// The path points at a file that does not exist, so resolution fails fast the
+// way it already does on a machine with no config. HOME is redirected into an
+// empty temp dir as well, so the shell-rc strategy has nothing to read either.
+func isolateLLMConnectionTest(t *testing.T) {
+	t.Helper()
+	dir := t.TempDir()
+	t.Setenv("OCR_CONFIG_PATH", filepath.Join(dir, "no-such-config.json"))
+	// Both, because os.UserHomeDir reads USERPROFILE on Windows and never falls
+	// back to HOME -- setting HOME alone would leave the shell-rc strategy reading
+	// the real profile.
+	t.Setenv("HOME", dir)
+	t.Setenv("USERPROFILE", dir)
+	for _, k := range []string{
+		"OCR_LLM_URL", "OCR_LLM_TOKEN", "OCR_LLM_MODEL",
+		"ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_MODEL",
+	} {
+		t.Setenv(k, "")
+	}
+}
+
 func TestMaskKey(t *testing.T) {
 	tests := []struct {
 		name string
@@ -50,8 +78,12 @@ func TestSaveConfig(t *testing.T) {
 	if err != nil {
 		t.Fatalf("stat: %v", err)
 	}
-	if perm := info.Mode().Perm(); perm != 0o600 {
-		t.Errorf("perm = %o, want 600", perm)
+	// Windows reports 0666 regardless of the mode passed to OpenFile, so only the
+	// unix arms can assert the 0600 the config file is written with.
+	if runtime.GOOS != "windows" {
+		if perm := info.Mode().Perm(); perm != 0o600 {
+			t.Errorf("perm = %o, want 600", perm)
+		}
 	}
 
 	data, err := os.ReadFile(path)
@@ -209,6 +241,7 @@ func TestApplyOfficialProviderConfig_MissingFields(t *testing.T) {
 }
 
 func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
+	isolateLLMConnectionTest(t)
 	t.Setenv("DEEPSEEK_API_KEY", "sk-from-env")
 	dir := t.TempDir()
 	configPath := filepath.Join(dir, "config.json")
@@ -243,7 +276,41 @@ func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
 	}
 }
 
+// A provider configured with only api_key_cmd must survive a trip through the
+// TUI: picking a model returns an empty apiKey, which must not be mistaken for
+// "no credential" and abandon the save.
+func TestApplyOfficialProviderConfig_APIKeyCmdSatisfiesRequirement(t *testing.T) {
+	isolateLLMConnectionTest(t)
+	t.Setenv("DEEPSEEK_API_KEY", "")
+	configPath := filepath.Join(t.TempDir(), "config.json")
+	cfg := &Config{
+		Providers: map[string]ProviderEntry{
+			"deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"},
+		},
+	}
+
+	err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{
+		provider: "deepseek",
+		model:    "deepseek-v4-flash",
+		apiKey:   "",
+	})
+	if err != nil {
+		t.Fatalf("api_key_cmd should satisfy the API key requirement: %v", err)
+	}
+	diskCfg, err := loadOrCreateConfig(configPath)
+	if err != nil {
+		t.Fatalf("load config: %v", err)
+	}
+	if diskCfg.Provider != "deepseek" || diskCfg.Model != "deepseek-v4-flash" {
+		t.Errorf("save was abandoned: provider=%q model=%q", diskCfg.Provider, diskCfg.Model)
+	}
+	if got := diskCfg.Providers["deepseek"].APIKeyCmd; got != "op read op://dev/deepseek/api-key" {
+		t.Errorf("persisted api_key_cmd = %q, want it preserved", got)
+	}
+}
+
 func TestApplyCustomProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
+	isolateLLMConnectionTest(t)
 	dir := t.TempDir()
 	configPath := filepath.Join(dir, "config.json")
 	cfg := &Config{
@@ -303,6 +370,7 @@ func TestProviderTUIResult_ResolvedModel(t *testing.T) {
 }
 
 func TestApplyOfficialProviderConfig_UsesSessionModelPick(t *testing.T) {
+	isolateLLMConnectionTest(t)
 	t.Setenv("QIANFAN_API_KEY", "sk-from-env")
 	dir := t.TempDir()
 	configPath := filepath.Join(dir, "config.json")