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 {
@@ -1136,13 +1186,23 @@ func captureConfigStderr(t *testing.T, fn func()) string {
 	os.Stderr = w
 	defer func() { os.Stderr = old }()
 
+	// Drained concurrently: reading only after fn returns caps the capture at the
+	// OS pipe buffer (64 KiB on Linux, far less on a Windows anonymous pipe) and
+	// a payload past that blocks the writer forever.
+	var data []byte
+	var readErr error
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		data, readErr = io.ReadAll(r)
+	}()
 	fn()
 	if err := w.Close(); err != nil {
 		t.Fatal(err)
 	}
-	data, err := io.ReadAll(r)
-	if err != nil {
-		t.Fatal(err)
+	<-done
+	if readErr != nil {
+		t.Fatal(readErr)
 	}
 	if err := r.Close(); err != nil {
 		t.Fatal(err)

cmd/opencodereview/delegate_exec_test.go

@@ -25,13 +25,23 @@ func captureDelegateStdout(t *testing.T, fn func()) []byte {
 	os.Stdout = w
 	defer func() { os.Stdout = orig }()
 
+	// Drain while fn runs. Reading only after fn returns caps the capture at
+	// whatever the pipe buffer holds: 64 KiB on Linux, far less on a Windows
+	// anonymous pipe, and a payload past that blocks the writer forever.
+	var out []byte
+	var readErr error
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		out, readErr = io.ReadAll(r)
+	}()
 	fn()
 	if err := w.Close(); err != nil {
 		t.Fatalf("close stdout writer: %v", err)
 	}
-	out, err := io.ReadAll(r)
-	if err != nil {
-		t.Fatalf("read stdout: %v", err)
+	<-done
+	if readErr != nil {
+		t.Fatalf("read stdout: %v", readErr)
 	}
 	_ = r.Close()
 	return out

cmd/opencodereview/output_helpers_test.go

@@ -388,11 +388,20 @@ func captureStdout(t *testing.T, fn func()) string {
 		t.Fatalf("os.Pipe: %v", err)
 	}
 	os.Stdout = w
+	// Drain while fn runs. Reading only after fn returns caps the capture at
+	// whatever the pipe buffer holds: 64 KiB on Linux, far less on a Windows
+	// anonymous pipe, and a payload past that blocks the writer forever.
+	var buf bytes.Buffer
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		_, _ = buf.ReadFrom(r)
+	}()
 	fn()
 	_ = w.Close()
 	os.Stdout = old
-	var buf bytes.Buffer
-	_, _ = buf.ReadFrom(r)
+	<-done
+	_ = r.Close()
 	return buf.String()
 }
 
@@ -406,11 +415,19 @@ func captureStderr(t *testing.T, fn func()) string {
 		t.Fatalf("os.Pipe: %v", err)
 	}
 	os.Stderr = w
+	// Drained concurrently for the same reason as captureStdout: an undrained
+	// pipe deadlocks fn once its output exceeds the OS pipe buffer.
+	var buf bytes.Buffer
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		_, _ = buf.ReadFrom(r)
+	}()
 	fn()
 	_ = w.Close()
 	os.Stderr = old
-	var buf bytes.Buffer
-	_, _ = buf.ReadFrom(r)
+	<-done
+	_ = r.Close()
 	return buf.String()
 }
 

cmd/opencodereview/provider_cmd.go

@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"strings"
 
 	tea "charm.land/bubbletea/v2"
 
@@ -239,13 +240,19 @@ 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. Trimmed
+	// because the resolver treats a whitespace-only command as unset, so without
+	// this a command of "   " would satisfy the check here and then fail
+	// resolution with "no api_key or api_key_cmd configured".
+	if result.apiKey == "" && strings.TrimSpace(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 +268,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