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

.github/workflows/ci.yml

@@ -85,6 +85,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

@@ -4,6 +4,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 )
@@ -37,7 +38,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

@@ -85,33 +85,77 @@ 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)
+	if warning := legacyLLMShadowWarning(cfg.Provider, key); warning != "" {
+		fmt.Fprint(os.Stderr, warning)
+	}
 	return nil
 }
 
-func runConfigUnset(key string) error {
-	parts := strings.SplitN(key, ".", 2)
-	if len(parts) != 2 || parts[1] == "" {
-		return fmt.Errorf("unset supports custom_providers.<name> and mcp_servers.<name>")
-	}
+// 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 {
 		return err
 	}
 
+	if key == "provider" {
+		return unsetActiveProvider(configPath)
+	}
+
+	parts := strings.SplitN(key, ".", 2)
+	if len(parts) != 2 || parts[1] == "" {
+		return fmt.Errorf("unset supports provider, custom_providers.<name>, and mcp_servers.<name>")
+	}
+
 	switch parts[0] {
 	case "custom_providers":
 		return unsetCustomProvider(configPath, parts[1])
 	case "mcp_servers":
 		return unsetMCPServer(configPath, parts[1])
 	default:
-		return fmt.Errorf("unset supports custom_providers.<name> and mcp_servers.<name>")
+		return fmt.Errorf("unset supports provider, custom_providers.<name>, and mcp_servers.<name>")
+	}
+}
+
+func unsetActiveProvider(configPath string) error {
+	cfg, err := loadOrCreateConfig(configPath)
+	if err != nil {
+		return fmt.Errorf("load config: %w", err)
+	}
+
+	cfg.Provider = ""
+	cfg.Model = ""
+	if err := saveConfig(configPath, cfg); err != nil {
+		return err
+	}
+
+	fmt.Println("Cleared active provider and model.")
+	return nil
+}
+
+func legacyLLMShadowWarning(provider, key string) string {
+	if provider == "" || !strings.HasPrefix(key, "llm.") {
+		return ""
+	}
+	section := "custom_providers"
+	if _, isPreset := llm.LookupProvider(provider); isPreset {
+		section = "providers"
 	}
+	return fmt.Sprintf("[ocr] WARNING: provider %q is active and takes precedence over llm.* settings.\n"+
+		"[ocr] Use 'ocr config set %s.%s.<field> <value>' to configure the active provider,\n"+
+		"[ocr] or run 'ocr config unset provider' to disable provider-based config.\n", provider, section, provider)
 }
 
 func unsetCustomProvider(configPath, name string) error {
@@ -190,6 +234,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"`
@@ -228,6 +273,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
@@ -333,6 +379,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 {
@@ -407,7 +455,7 @@ func setConfigValue(cfg *Config, key, value string) error {
 		}
 		cfg.Llm.ExtraBody = m
 	default:
-		return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, 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, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key)
+		return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, 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, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key)
 	}
 	return nil
 }
@@ -416,6 +464,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":
 		entry.URL = value
 	case "protocol":
@@ -451,7 +501,7 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error {
 		}
 		entry.ExtraHeaders = parsed
 	default:
-		return fmt.Errorf("unknown provider field %q: supported fields are api_key, url, protocol, model, models, auth_header, extra_body, extra_headers", 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", field)
 	}
 	return nil
 }