diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 23a1067..5ce3680 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/cmd/opencodereview/background_file_test.go b/cmd/opencodereview/background_file_test.go index 87f216e..131b78c 100644 --- a/cmd/opencodereview/background_file_test.go +++ b/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) } diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 8a5bfce..58b0dd4 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/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..", "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 } diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 2084d3a..54f9dc0 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/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.., custom_providers.., mcp_servers.., 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.., custom_providers.., mcp_servers.., 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) diff --git a/cmd/opencodereview/delegate_exec_test.go b/cmd/opencodereview/delegate_exec_test.go index ab2e8e6..90123e0 100644 --- a/cmd/opencodereview/delegate_exec_test.go +++ b/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 diff --git a/cmd/opencodereview/output_helpers_test.go b/cmd/opencodereview/output_helpers_test.go index f0976f3..48e1d80 100644 --- a/cmd/opencodereview/output_helpers_test.go +++ b/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() } diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 2552623..3fa20b5 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/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 diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index f1fbca7..b365c89 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/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..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") diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 584b5db..2acc028 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -907,11 +907,48 @@ func officialProviderEnvKeySet(p llm.Provider) bool { return p.EnvVar != "" && os.Getenv(p.EnvVar) != "" } +// officialAPIKeyRequiredError mirrors the wording applyOfficialProviderConfig +// uses for the same failure, so the interactive and non-interactive paths name +// the same options in the same order (static key -> api_key_cmd -> env var). func officialAPIKeyRequiredError(p llm.Provider) string { - if p.EnvVar != "" { - return fmt.Sprintf("API key is required (or set $%s)", p.EnvVar) + // Each alternative is independently gated: a provider with no Name still gets + // the env-var hint, and vice versa. Naming api_key_cmd here is the point -- + // the step used to reject a provider that resolves fine through a command. + var alternatives []string + if p.Name != "" { + alternatives = append(alternatives, fmt.Sprintf("set providers.%s.api_key_cmd", p.Name)) } - return "API key is required" + if p.EnvVar != "" { + alternatives = append(alternatives, fmt.Sprintf("set $%s", p.EnvVar)) + } + if len(alternatives) == 0 { + return "API key is required" + } + return fmt.Sprintf("API key is required (configure it, %s)", strings.Join(alternatives, ", or ")) +} + +// apiKeyCmdForStep returns the api_key_cmd already configured for the provider +// the API-key step is editing, reading the same config entry loadExistingAPIKey +// reads the static key from. The step serves the Official and Custom tabs; the +// Manual tab has its own form and uses llm.auth_token_cmd instead. +// +// Trimmed because the resolver treats a whitespace-only command as unset (see +// tryOCRConfig). Returning it verbatim would let this step accept an empty API +// key on the strength of an `api_key_cmd` of " ", saving a config the resolver +// then rejects with "no api_key or api_key_cmd configured". +func (m providerTUIModel) apiKeyCmdForStep() string { + switch m.activeTab { + case tabOfficial: + if m.existingCfg == nil { + return "" + } + return strings.TrimSpace(m.existingCfg.Providers[m.currentProvider().Name].APIKeyCmd) + case tabCustom: + if cp, ok := m.selectedCustomProvider(); ok { + return strings.TrimSpace(m.customProviderEntry(cp.name, cp.entry).APIKeyCmd) + } + } + return "" } func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { @@ -921,6 +958,12 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { if !m.apiKeyMasked && strings.TrimSpace(m.apiKeyInput.Value()) != "" { return true, "" } + // Resolver precedence is static key -> api_key_cmd -> env var, so an already + // configured command satisfies the requirement: the field renders blank for + // such a provider and must still be confirmable. + if m.apiKeyCmdForStep() != "" { + return true, "" + } if m.activeTab == tabOfficial { p := m.currentProvider() if officialProviderEnvKeySet(p) { @@ -928,6 +971,9 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { } return false, officialAPIKeyRequiredError(p) } + if cp, ok := m.selectedCustomProvider(); ok && cp.name != "" { + return false, fmt.Sprintf("API key is required (configure it or set custom_providers.%s.api_key_cmd)", cp.name) + } return false, "API key is required" } @@ -1046,7 +1092,17 @@ func authHeaderFormError(raw string) string { ) } -const manualAuthTokenRequiredError = "Auth token is required (whitespace-only input is not accepted)" +const manualAuthTokenRequiredError = "Auth token is required (configure it or set llm.auth_token_cmd; whitespace-only input is not accepted)" + +// manualAuthTokenCmd returns the configured llm.auth_token_cmd, which the +// resolver runs when llm.auth_token is empty. Trimmed for the same reason as +// apiKeyCmdForStep: the resolver treats a whitespace-only command as unset. +func (m providerTUIModel) manualAuthTokenCmd() string { + if m.existingCfg == nil { + return "" + } + return strings.TrimSpace(m.existingCfg.Llm.AuthTokenCmd) +} func (m providerTUIModel) handleCustomFormEnter() (tea.Model, tea.Cmd) { switch m.cpStep { @@ -1184,6 +1240,7 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { func cloneProviderEntry(v ProviderEntry) ProviderEntry { out := ProviderEntry{ APIKey: v.APIKey, + APIKeyCmd: v.APIKeyCmd, URL: v.URL, Protocol: v.Protocol, Model: v.Model, @@ -1616,7 +1673,9 @@ func (m providerTUIModel) handleManualFormEnter() (tea.Model, tea.Cmd) { m.manualStep = manualStepAuthToken return m, m.manualTokenInput.Focus() case manualStepAuthToken: - if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" { + // Same precedence as the provider tabs: an already configured + // llm.auth_token_cmd stands in for a typed or saved token. + if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" && m.manualAuthTokenCmd() == "" { m.formError = manualAuthTokenRequiredError return m, nil } @@ -1918,7 +1977,10 @@ func (m providerTUIModel) result() providerTUIResult { return providerTUIResult{} case tabManual: - apiKey := m.manualTokenInput.Value() + // Trim like the Official and Custom tabs: a whitespace-only token must + // never persist, or it wins precedence over a working auth_token_cmd + // and sends "Authorization: Bearer ". + apiKey := strings.TrimSpace(m.manualTokenInput.Value()) if m.manualTokenMasked || (apiKey == "" && m.manualTokenOriginal != "") { apiKey = m.manualTokenOriginal } @@ -2223,6 +2285,9 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { if m.manualTokenMasked && m.manualTokenOriginal != "" { s.WriteString(tuiDimStyle.Render(" "+savedSecretReplaceHint(m.manualTokenOriginal)) + "\n") } + if m.manualAuthTokenCmd() != "" { + s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "llm.auth_token_cmd")) + "\n") + } case manualStepAuthHeader: s.WriteString(" " + m.manualAuthHeaderInput.View() + "\n") } @@ -2325,6 +2390,14 @@ func (m providerTUIModel) viewAPIKey(s *strings.Builder) { s.WriteString("\n") } + // Mirrors the env-var hint below: the step is already satisfied, so say so + // rather than leaving an empty field that looks unconfigured. + if m.apiKeyCmdForStep() != "" { + s.WriteString("\n") + s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "api_key_cmd"))) + s.WriteString("\n") + } + if m.activeTab == tabOfficial { provider := m.currentProvider() if envKey := os.Getenv(provider.EnvVar); envKey != "" { @@ -2403,6 +2476,28 @@ func officialAPIKeyEnvSetHintLine(envVar string, hasSavedKey bool) string { return " " + officialAPIKeyEnvSetHint(envVar, hasSavedKey) } +// keyCmdConfiguredHint explains why this step accepts an empty field. A +// provider configured only by command renders a blank input -- the command line +// is not the secret, but it is also not the value being edited here -- so +// without this the user has no way to tell a credential is already wired up, +// and no way to know that leaving the field empty is the correct action. +// keyLabel names the config key so the hint points at what to edit instead. +// +// The command itself is deliberately not echoed. It is usually a bare reference +// (`op read op://...`), but nothing stops a user from inlining a secret into it +// (`VAULT_TOKEN=hvs.xxx vault kv get ...`), and this wizard masks every other +// credential it displays -- printing one user-authored string verbatim into +// screenshots and terminal recordings is the one hole in that. Naming the config +// key is what the hint is for and is enough to identify the command: there is +// exactly one per provider, so the user knows which value to go read or edit. +func keyCmdConfiguredHint(keyLabel string) string { + return fmt.Sprintf("%s is set; leave empty to keep using it.", keyLabel) +} + +func keyCmdConfiguredHintLine(indent, keyLabel string) string { + return indent + keyCmdConfiguredHint(keyLabel) +} + // --- Styles --- const tuiCursor = "▸" diff --git a/cmd/opencodereview/provider_tui_cpinput_test.go b/cmd/opencodereview/provider_tui_cpinput_test.go index 5afeaf6..0aedec8 100644 --- a/cmd/opencodereview/provider_tui_cpinput_test.go +++ b/cmd/opencodereview/provider_tui_cpinput_test.go @@ -24,14 +24,42 @@ func TestIsUserEditMsg(t *testing.T) { } } -// TestOfficialAPIKeyRequiredError covers the with-EnvVar and without branches. +// TestOfficialAPIKeyRequiredError covers every combination of the two hints the +// message can offer. Both are independently gated, so a provider carrying only +// one of Name/EnvVar must still be told about that one. func TestOfficialAPIKeyRequiredError(t *testing.T) { - got := officialAPIKeyRequiredError(llm.Provider{EnvVar: "MY_KEY"}) - if got != "API key is required (or set $MY_KEY)" { - t.Errorf("got %q, want mention of $MY_KEY", got) + tests := []struct { + name string + provider llm.Provider + want string + }{ + { + name: "env var only", + provider: llm.Provider{EnvVar: "MY_KEY"}, + want: "API key is required (configure it, set $MY_KEY)", + }, + { + name: "name only", + provider: llm.Provider{Name: "acme"}, + want: "API key is required (configure it, set providers.acme.api_key_cmd)", + }, + { + name: "name and env var", + provider: llm.Provider{Name: "acme", EnvVar: "MY_KEY"}, + want: "API key is required (configure it, set providers.acme.api_key_cmd, or set $MY_KEY)", + }, + { + name: "neither", + provider: llm.Provider{}, + want: "API key is required", + }, } - if got := officialAPIKeyRequiredError(llm.Provider{}); got != "API key is required" { - t.Errorf("got %q, want generic message", got) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := officialAPIKeyRequiredError(tt.provider); got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) } } diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 3af2d93..6b2ca98 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -6,6 +6,7 @@ package main import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -201,18 +202,26 @@ func TestRenderListName_Inactive(t *testing.T) { func TestCloneProviderEntry_WithExtraBody(t *testing.T) { orig := ProviderEntry{ APIKey: "key", + APIKeyCmd: "op read op://dev/anthropic/api-key", URL: "http://localhost", Protocol: "openai", Model: "gpt-4", Models: []string{"gpt-4", "gpt-3.5"}, AuthHeader: "Authorization", + TimeoutSec: 45, ExtraBody: map[string]any{"temperature": 0.7, "stream": true}, + ExtraHeaders: map[string]string{ + "X-Trace": "on", + }, } clone := cloneProviderEntry(orig) if clone.APIKey != orig.APIKey || clone.URL != orig.URL || clone.Protocol != orig.Protocol { t.Error("basic fields not copied") } + if clone.APIKeyCmd != orig.APIKeyCmd { + t.Errorf("APIKeyCmd not copied: got %q, want %q", clone.APIKeyCmd, orig.APIKeyCmd) + } if len(clone.Models) != 2 || clone.Models[0] != "gpt-4" { t.Errorf("Models not cloned: %v", clone.Models) } @@ -232,6 +241,22 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { if len(orig.Models) != 2 { t.Error("modifying clone should not affect original Models") } + + if clone.TimeoutSec != orig.TimeoutSec { + t.Errorf("TimeoutSec not copied: got %d, want %d", clone.TimeoutSec, orig.TimeoutSec) + } + if clone.ExtraHeaders == nil { + // Fatal, not Error: writing to the nil map below would panic instead of + // reporting which field was dropped. + t.Fatal("ExtraHeaders should not be nil") + } + if clone.ExtraHeaders["X-Trace"] != "on" { + t.Errorf("ExtraHeaders not copied: %v", clone.ExtraHeaders) + } + clone.ExtraHeaders["X-New"] = "1" + if _, ok := orig.ExtraHeaders["X-New"]; ok { + t.Error("modifying clone should not affect original ExtraHeaders") + } } func TestCloneProviderEntry_NilExtraBody(t *testing.T) { @@ -243,6 +268,43 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { if clone.ExtraBody != nil { t.Error("ExtraBody should remain nil") } + if clone.ExtraHeaders != nil { + t.Error("ExtraHeaders should remain nil") + } +} + +// cloneProviderEntry lists fields by hand, which is how timeout_sec and +// extra_headers came to be silently dropped on the save-rollback paths. This +// fails when a field is added to ProviderEntry but not to the clone: the +// non-zero check forces the fixture to grow, and DeepEqual then catches the +// omission. It catches a dropped field, not an aliased one -- DeepEqual +// compares values, not identity; the sibling tests above cover aliasing. +func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { + orig := ProviderEntry{ + APIKey: "key", + APIKeyCmd: "op read op://dev/x/api-key", + URL: "http://localhost", + Protocol: "openai", + Model: "gpt-4", + Models: []string{"gpt-4"}, + AuthHeader: "Authorization", + TimeoutSec: 45, + RetryCodes: []int{403}, + ExtraBody: map[string]any{"temperature": 0.7}, + ExtraHeaders: map[string]string{"X-Trace": "on"}, + } + + rv := reflect.ValueOf(orig) + for i := range rv.NumField() { + if rv.Field(i).IsZero() { + t.Fatalf("fixture leaves %s zero-valued; set it so the clone is actually checked", + rv.Type().Field(i).Name) + } + } + + if clone := cloneProviderEntry(orig); !reflect.DeepEqual(clone, orig) { + t.Errorf("clone dropped a field:\n got %+v\nwant %+v", clone, orig) + } } func TestCloneProviderEntry_TimeoutAndRetryCodes(t *testing.T) { @@ -1824,83 +1886,226 @@ func TestProviderTUI_ResultUsesSessionModelPickWhenSelectionEmpty(t *testing.T) } } -func TestApiKeyStepCanConfirm_OfficialEmptyWithoutEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, +// apiKeyStepCanConfirm gates the final Enter of `ocr config provider`. It has to +// mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): +// a provider configured with only api_key_cmd renders a blank key field, and +// blocking it there made the feature unreachable from the documented wizard. +func TestApiKeyStepCanConfirm(t *testing.T) { + tests := []struct { + name string + env string + cfg *Config + customTab bool + typedKey string + wantOK bool + wantErrMsg string + }{ + { + name: "official saved api_key", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKey: "keep-me"}}, + }, + wantOK: true, + }, + { + name: "official typed key", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + typedKey: "sk-typed", + wantOK: true, + }, + { + name: "official api_key_cmd only", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"}}, + }, + wantOK: true, + }, + { + name: "official nothing configured", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it, set providers.deepseek.api_key_cmd, or set $DEEPSEEK_API_KEY)", + }, + { + // The resolver treats a whitespace-only command as unset, so opening the + // gate on one would save a config it then refuses to resolve. + name: "official whitespace-only api_key_cmd", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKeyCmd: " "}}, + }, + wantOK: false, + wantErrMsg: "API key is required (configure it, set providers.deepseek.api_key_cmd, or set $DEEPSEEK_API_KEY)", + }, + { + name: "official env var set", + env: "sk-from-env", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: true, + }, + { + name: "custom saved api_key", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKey: "sk-custom"}}, + }, + wantOK: true, + }, + { + name: "custom api_key_cmd only", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKeyCmd: "op read op://dev/stepfun/api-key"}}, + }, + wantOK: true, + }, + { + name: "custom nothing configured", + customTab: true, + cfg: &Config{Provider: "stepfun", CustomProviders: map[string]ProviderEntry{"stepfun": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it or set custom_providers.stepfun.api_key_cmd)", + }, + { + name: "custom whitespace-only api_key_cmd", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKeyCmd: " \t "}}, + }, + wantOK: false, + wantErrMsg: "API key is required (configure it or set custom_providers.stepfun.api_key_cmd)", }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required (or set $DEEPSEEK_API_KEY)" { - t.Errorf("errMsg = %q", errMsg) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("DEEPSEEK_API_KEY", tc.env) + m := newProviderTUI(tc.cfg, "") + if tc.customTab { + m.activeTab = tabCustom + m.customIdx = 0 + } else { + m.activeTab = tabOfficial + } + m.step = stepAPIKey + // loadExistingAPIKey is what the wizard runs on entering the step, and + // is the only thing that populates apiKeyOriginal / the mask. + m.loadExistingAPIKey() + if tc.typedKey != "" { + m.apiKeyInput.SetValue(tc.typedKey) + } + + ok, errMsg := m.apiKeyStepCanConfirm() + if ok != tc.wantOK { + t.Fatalf("apiKeyStepCanConfirm() ok = %v, want %v (errMsg = %q)", ok, tc.wantOK, errMsg) + } + if errMsg != tc.wantErrMsg { + t.Errorf("errMsg = %q, want %q", errMsg, tc.wantErrMsg) + } + }) } } -func TestApiKeyStepCanConfirm_OfficialEmptyWithEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "sk-from-env") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, +// The Manual tab's auth-token gate is the legacy twin of apiKeyStepCanConfirm: +// llm.auth_token_cmd has to stand in for an empty field the same way. +func TestHandleManualFormEnter_AuthTokenGate(t *testing.T) { + tests := []struct { + name string + llmCfg LlmConfig + typedToken string + wantAdvance bool + // wantAPIKey is the token result() must persist once the step confirms. + wantAPIKey string + }{ + { + name: "saved auth_token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthToken: "tok-saved"}, + wantAdvance: true, + wantAPIKey: "tok-saved", + }, + { + name: "typed token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: "tok-typed", + wantAdvance: true, + wantAPIKey: "tok-typed", + }, + { + name: "auth_token_cmd only", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + wantAdvance: true, + }, + { + // auth_token_cmd opens the gate, so whitespace typed at this step + // confirms. It must not be saved as auth_token: a non-empty token + // wins precedence and would silently shadow the working command. + name: "auth_token_cmd with whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + typedToken: " ", + wantAdvance: true, + }, + { + name: "nothing configured", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + wantAdvance: false, + }, + { + // Same rule as the api_key_cmd gate: the resolver reads a + // whitespace-only command as unset, so it must not open the gate here. + name: "whitespace-only auth_token_cmd", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: " "}, + wantAdvance: false, + }, + { + name: "whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: " ", + wantAdvance: false, }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) - } -} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := newProviderTUI(&Config{Llm: tc.llmCfg}, "") + m.activeTab = tabManual + m.inManualForm = true + m.manualStep = manualStepAuthToken + if tc.typedToken != "" { + m.manualTokenInput.SetValue(tc.typedToken) + } -func TestApiKeyStepCanConfirm_CustomEmpty(t *testing.T) { - cfg := &Config{ - Provider: "stepfun", - CustomProviders: map[string]ProviderEntry{ - "stepfun": {APIKey: ""}, - }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabCustom - m.customIdx = 0 - m.step = stepAPIKey + result, _ := m.handleManualFormEnter() + m2 := result.(providerTUIModel) - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required" { - t.Errorf("errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_MaskedSavedKey(t *testing.T) { - cfg := &Config{ - Provider: "deepseek", - Providers: map[string]ProviderEntry{ - "deepseek": {APIKey: "keep-me"}, - }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - m.loadExistingAPIKey() - - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) + if tc.wantAdvance { + if m2.manualStep != manualStepAuthHeader { + t.Fatalf("manualStep = %d, want manualStepAuthHeader (%d); formError = %q", + m2.manualStep, manualStepAuthHeader, m2.formError) + } + if m2.formError != "" { + t.Errorf("formError = %q, want empty", m2.formError) + } + if got := m2.result().apiKey; got != tc.wantAPIKey { + t.Errorf("result().apiKey = %q, want %q", got, tc.wantAPIKey) + } + return + } + if m2.manualStep != manualStepAuthToken { + t.Fatalf("manualStep = %d, want to stay on manualStepAuthToken (%d)", + m2.manualStep, manualStepAuthToken) + } + if m2.formError != manualAuthTokenRequiredError { + t.Errorf("formError = %q, want %q", m2.formError, manualAuthTokenRequiredError) + } + if !strings.Contains(m2.formError, "llm.auth_token_cmd") { + t.Errorf("formError should name llm.auth_token_cmd, got %q", m2.formError) + } + }) } } diff --git a/cmd/opencodereview/provider_tui_savefail_test.go b/cmd/opencodereview/provider_tui_savefail_test.go index 0a32e5b..cc92401 100644 --- a/cmd/opencodereview/provider_tui_savefail_test.go +++ b/cmd/opencodereview/provider_tui_savefail_test.go @@ -10,17 +10,24 @@ import ( "testing" ) -// unwritableConfigPath returns a config path whose parent is a regular file, so -// any saveConfig / loadOrCreateConfig against it fails (ENOTDIR). This is the -// lever used to drive the save-failure + rollback branches of the TUI handlers. +// unwritableConfigPath returns a config path that is itself a directory, so any +// saveConfig / loadOrCreateConfig against it fails. This is the lever used to +// drive the save-failure + rollback branches of the TUI handlers. +// +// A directory rather than the more obvious "parent is a regular file" trick: +// Windows reports a path below a non-directory parent as ERROR_PATH_NOT_FOUND, +// which os.IsNotExist accepts, so loadOrCreateConfig read that as "no config +// yet" and returned an empty Config instead of an error. The reload then looked +// like a success and the rollback branch never ran. A directory fails the write +// on every platform, and reading it yields either an error or empty bytes that +// fail to parse as JSON, so the reload fails everywhere too. func unwritableConfigPath(t *testing.T) string { t.Helper() - dir := t.TempDir() - blocker := filepath.Join(dir, "blocker") - if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { - t.Fatalf("write blocker: %v", err) + path := filepath.Join(t.TempDir(), "config.json") + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatalf("mkdir blocking config dir: %v", err) } - return filepath.Join(blocker, "config.json") + return path } // TestUpdateDeleteConfirm_SaveFailure drives the provider-delete confirm handler diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index f84cf6b..5e108b8 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -2350,8 +2350,10 @@ func TestProviderTUI_OfficialApiKeyEmptyWithoutEnvBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required (or set $DASHSCOPE_API_KEY)" { - t.Errorf("formError = %q", m2.formError) + // The exact prose is pinned by TestApiKeyStepCanConfirm; this test covers the + // Enter-key wiring, so compare against the helper and never drift again. + if want := officialAPIKeyRequiredError(m2.currentProvider()); m2.formError != want { + t.Errorf("formError = %q, want %q", m2.formError, want) } if cmd != nil { t.Error("Enter without key or env should not quit") @@ -2413,8 +2415,10 @@ func TestProviderTUI_CustomExistingApiKeyEmptyBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required" { - t.Errorf("formError = %q, want %q", m2.formError, "API key is required") + // Prefix, not the full string: this test covers Enter-key gating, and the + // exact wording is pinned by TestApiKeyStepCanConfirm. + if !strings.HasPrefix(m2.formError, "API key is required") { + t.Errorf("formError = %q, want it to start with %q", m2.formError, "API key is required") } if cmd != nil { t.Error("Enter with cleared key should not quit") @@ -2659,6 +2663,7 @@ func TestProviderTUI_DeleteModelPreservesActiveModel(t *testing.T) { } func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") models := []string{"test-model", "test-model-2", "bbb", "aaa", "test-model-3"} @@ -2702,6 +2707,7 @@ func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { } func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{} @@ -2727,6 +2733,7 @@ func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { } func TestApplyCustomProviderConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{ @@ -2875,6 +2882,7 @@ func TestEnterEditCustomProvider_ProtocolIndex(t *testing.T) { // mirrored for the two protocols that have a boolean equivalent so older // binaries can still read the config. func TestApplyManualConfig_DoubleWritesProtocolAndUseAnthropic(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") @@ -2979,3 +2987,69 @@ func TestProviderTUIResult_ManualProtocolIsCanonical(t *testing.T) { } } } + +func TestKeyCmdConfiguredHint(t *testing.T) { + got := keyCmdConfiguredHint("api_key_cmd") + want := "api_key_cmd is set; leave empty to keep using it." + if got != want { + t.Errorf("hint = %q, want %q", got, want) + } +} + +// A provider configured only by command renders a blank API-key field, so +// without this hint there is nothing on screen distinguishing "credential +// already wired up" from "nothing configured". +func TestProviderTUI_ViewAPIKey_ShowsAPIKeyCmdHint(t *testing.T) { + cfg := &Config{ + Provider: "deepseek", + Model: "deepseek-v4-flash", + Providers: map[string]ProviderEntry{ + "deepseek": {APIKeyCmd: "op read op://dev/deepseek/key", Model: "deepseek-v4-flash"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "deepseek" { + m.officialIdx = i + break + } + } + m.step = stepAPIKey + m.loadExistingAPIKey() + m.apiKeyInput.Focus() + + got := stripANSI(m.View().Content) + want := "api_key_cmd is set; leave empty to keep using it." + if !strings.Contains(got, want) { + t.Errorf("view missing api_key_cmd hint; want %q; got:\n%s", want, got) + } + // The command can carry an inlined secret, so it must not reach the screen. + if strings.Contains(got, "op read op://dev/deepseek/key") { + t.Errorf("view renders the api_key_cmd verbatim; got:\n%s", got) + } +} + +func TestProviderTUI_ViewAPIKey_NoCmdHintWhenUnset(t *testing.T) { + cfg := &Config{ + Provider: "deepseek", + Model: "deepseek-v4-flash", + Providers: map[string]ProviderEntry{ + "deepseek": {Model: "deepseek-v4-flash"}, + }, + } + m := newProviderTUI(cfg, "") + m.activeTab = tabOfficial + for i, p := range m.providers { + if p.Name == "deepseek" { + m.officialIdx = i + break + } + } + m.step = stepAPIKey + m.loadExistingAPIKey() + + if got := stripANSI(m.View().Content); strings.Contains(got, "api_key_cmd is set") { + t.Errorf("view should not claim api_key_cmd is set when it is not; got:\n%s", got) + } +} diff --git a/cmd/opencodereview/retry_fake_llm_test.go b/cmd/opencodereview/retry_fake_llm_test.go index 0713f82..7af11f5 100644 --- a/cmd/opencodereview/retry_fake_llm_test.go +++ b/cmd/opencodereview/retry_fake_llm_test.go @@ -166,10 +166,15 @@ func retryTestRepo(t *testing.T) string { // startFakeLLM starts srv and points the OCR_LLM_* endpoint resolution at it, // with HOME/XDG_CONFIG_HOME redirected so the developer's real config and // session directory (both under $HOME/.opencodereview) are never touched. +// Those paths resolve through os.UserHomeDir, which reads USERPROFILE on +// Windows and never falls back to HOME, so redirecting HOME alone left these +// runs writing to the real profile. Set both; the one that does not apply is +// harmless. func startFakeLLM(t *testing.T, srv *fakeLLM) { t.Helper() home := t.TempDir() t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) server := httptest.NewServer(srv) diff --git a/cmd/opencodereview/shared_llmruntime_test.go b/cmd/opencodereview/shared_llmruntime_test.go index 6f88434..028dde9 100644 --- a/cmd/opencodereview/shared_llmruntime_test.go +++ b/cmd/opencodereview/shared_llmruntime_test.go @@ -88,6 +88,11 @@ func TestLoadLLMRuntime_UnresolvableEndpoint(t *testing.T) { func TestLoadLLMRuntime_BadAppConfig(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) + // defaultConfigPath resolves through os.UserHomeDir, which reads USERPROFILE + // on Windows and never falls back to HOME, so redirecting HOME alone left + // the invalid config below in a directory nobody reads. Set both; the one + // that does not apply is harmless. + t.Setenv("USERPROFILE", home) cfgDir := filepath.Join(home, ".opencodereview") if err := os.MkdirAll(cfgDir, 0o755); err != nil { t.Fatalf("mkdir: %v", err) diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 6bd61ea..4a0c99f 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1264,7 +1264,10 @@ func TestResolveRuleEntries_SymlinkSafety(t *testing.T) { // The extension check on the resolved path should reject .json. symlinkPath := filepath.Join(dir, "evil.md") if err := os.Symlink(sensitiveFile, symlinkPath); err != nil { - t.Fatal(err) + // Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which + // an unelevated CI account does not have. Same skip the other symlink tests + // in this repo already use. + t.Skipf("cannot create symlink: %v", err) } entries := []ProjectRuleEntry{ @@ -1652,9 +1655,18 @@ func TestLoadGlobalRule(t *testing.T) { globalRulePath := func(home string) string { return filepath.Join(home, ".opencodereview", "rule.json") } + // loadGlobalRule resolves the home dir with os.UserHomeDir, which reads + // USERPROFILE on Windows and never falls back to HOME. Setting HOME alone + // left the subtests reading the real profile, where the rule file they just + // wrote does not exist. Set both; the one that does not apply is harmless. + setHome := func(t *testing.T, home string) { + t.Helper() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + } t.Run("missing file is not an error", func(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + setHome(t, t.TempDir()) pr, err := loadGlobalRule() if err != nil || pr != nil { t.Fatalf("expected nil,nil for missing global rule: pr=%v err=%v", pr, err) @@ -1663,7 +1675,7 @@ func TestLoadGlobalRule(t *testing.T) { t.Run("read error when path is a directory", func(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) // Create the rule.json path as a directory so ReadFile fails with a // non-NotExist error (EISDIR), exercising the wrapped-error branch. if err := os.MkdirAll(globalRulePath(home), 0o755); err != nil { @@ -1676,7 +1688,7 @@ func TestLoadGlobalRule(t *testing.T) { t.Run("unmarshal error on invalid JSON", func(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) path := globalRulePath(home) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir parent: %v", err) @@ -1691,7 +1703,7 @@ func TestLoadGlobalRule(t *testing.T) { t.Run("valid file returns rule", func(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) path := globalRulePath(home) if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatalf("mkdir parent: %v", err) diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go new file mode 100644 index 0000000..306cf2f --- /dev/null +++ b/internal/llm/keycmd.go @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "strings" + "time" +) + +// keyCmdTimeout bounds how long an api_key_cmd / auth_token_cmd may run. +// It is a package var (not const) so tests can shrink it. +var keyCmdTimeout = 60 * time.Second + +// keyCmdWaitDelay bounds how long Wait keeps waiting on the child's stdout pipe +// after the command's own deadline has passed. Package var (not const) so tests +// can shrink it, same as keyCmdTimeout. +var keyCmdWaitDelay = 5 * time.Second + +// keyCmdMaxOutput caps how much of a credential command's stdout we buffer. +const keyCmdMaxOutput = 64 << 10 + +// errKeyCmdOutputTooLarge aborts the stdout copy once the cap is hit. It never +// reaches the caller: cappedBuffer.overflow is what produces the error message. +var errKeyCmdOutputTooLarge = errors.New("credential command output exceeds cap") + +// cappedBuffer collects at most max bytes and records whether more were offered. +// Refusing the write makes os/exec's copier close the pipe, so a runaway command +// (`cat /dev/urandom`) dies of SIGPIPE instead of growing our heap without bound. +type cappedBuffer struct { + max int + buf bytes.Buffer + overflow bool +} + +func (b *cappedBuffer) Write(p []byte) (int, error) { + if b.buf.Len()+len(p) > b.max { + b.overflow = true + return 0, errKeyCmdOutputTooLarge + } + return b.buf.Write(p) +} + +// resolveKeyCmd runs a credential-fetching shell command and returns its +// trimmed, single-line stdout. label names the source (e.g. +// `api_key_cmd for provider "x"`) and is used in error messages. +// +// The child's stderr is wired to the process stderr so interactive prompts +// (pinentry, 1Password, `op`) stay visible, and its stdin to the process stdin +// so those prompts can be answered. Any failure is a hard error, never a silent +// fallback. The resolved credential is used in memory only and is never written +// to config or logged. +func resolveKeyCmd(cmd, label string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout) + defer cancel() + + c := newKeyCmd(ctx, cmd) + c.Stderr = os.Stderr + // With Stdin nil, os/exec hands the child /dev/null, so a helper that needs + // to prompt for a passphrase gets EOF or refuses to prompt at all because it + // sees no tty. Safe to hand over os.Stdin because no code path resolves an + // endpoint while the bubbletea TUI (which also reads os.Stdin) is running: + // ResolveEndpoint's only callers are the non-TUI review/scan and `ocr llm + // test` paths. Adding an in-TUI connection test would break that. + c.Stdin = os.Stdin + // Buffer stdout through cappedBuffer rather than an *os.File so os/exec does + // the copying in its own goroutine: that is what lets WaitDelay force the + // pipe closed. exec.CommandContext SIGKILLs only the shell, so a grandchild + // (gpg-agent, pinentry, `op`) that inherited the stdout pipe keeps it open + // and Wait blocks on the read long past the timeout -- reproducible with + // api_key_cmd = "sleep 200 & printf tok". WaitDelay makes Wait give up + // shortly after the context dies. + out := &cappedBuffer{max: keyCmdMaxOutput} + c.Stdout = out + c.WaitDelay = keyCmdWaitDelay + + err := c.Run() + // Checked first so a timeout reports as such instead of as the SIGKILL exit + // status it produces. (Run has already joined every stdout copier, so the + // buffer below is safe to read on all paths.) + if ctx.Err() == context.DeadlineExceeded { + // Wrap ctx.Err() so callers can errors.Is(err, context.DeadlineExceeded). + return "", fmt.Errorf("%s timed out after %s: %w", label, keyCmdTimeout, ctx.Err()) + } + if out.overflow { + return "", fmt.Errorf("%s produced more than 64KiB of output", label) + } + // ErrWaitDelay only means an orphaned grandchild still holds the pipe; the + // command itself exited fine and its output is already buffered, so use it + // rather than surfacing an exec-internal error. + if err != nil && !errors.Is(err, exec.ErrWaitDelay) { + // Covers non-zero exit and command-not-found (the shell exits non-zero + // and prints its not-found message on the child's stderr). ExitError.Stderr + // stays nil because we assigned c.Stderr, so no output can leak here. + return "", fmt.Errorf("%s failed: %w", label, err) + } + + // Trim a trailing line break; multi-line output past that is ambiguous and refused. + // ContainsAny (not Contains "\n") so a lone interior CR is caught too: TrimRight + // leaves it, TrimSpace below only strips the edges, and a CR inside a credential + // makes net/http reject the Authorization header with an opaque error. + trimmed := strings.TrimRight(out.buf.String(), "\r\n") + if strings.ContainsAny(trimmed, "\n\r") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential (pipe through 'head -n1' if your command prints more)", label) + } + // Same reason as the line-break check, wider net: httpguts.ValidHeaderFieldValue + // (what net/http enforces) rejects every byte below 0x20 except SP and TAB, plus + // DEL. A NUL or VT smuggled in by e.g. `printf 'sk-a\0b'` would otherwise reach + // net/http as the opaque `invalid header field value for "Authorization"`. + // + // Deliberately before the TrimSpace below, so a trailing control byte is an + // error naming its offset rather than silently stripped: only TAB, SP and the + // line breaks already handled above are things a credential command can + // plausibly append by accident. Offsets are therefore into the pre-TrimSpace + // string, which is what the command actually produced. + for i := 0; i < len(trimmed); i++ { + if b := trimmed[i]; (b < 0x20 && b != '\t') || b == 0x7f { + return "", fmt.Errorf("%s produced a control byte 0x%02X at offset %d; a credential must not contain control characters", label, b, i) + } + } + + key := strings.TrimSpace(trimmed) + if key == "" { + return "", fmt.Errorf("%s produced empty output", label) + } + return key, nil +} diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go new file mode 100644 index 0000000..19139df --- /dev/null +++ b/internal/llm/keycmd_test.go @@ -0,0 +1,163 @@ +//go:build !windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "os" + "strings" + "testing" + "time" +) + +func TestResolveKeyCmd(t *testing.T) { + tests := []struct { + name string + cmd string + want string + wantErr string // substring the error must contain; "" means success + }{ + {name: "success", cmd: "printf 'sk-test\\n'", want: "sk-test"}, + {name: "trailing whitespace trimmed", cmd: "printf ' sk-test \\n'", want: "sk-test"}, + {name: "no trailing newline", cmd: "printf 'sk-test'", want: "sk-test"}, + {name: "crlf line ending trimmed", cmd: "printf 'sk-crlf\\r\\n'", want: "sk-crlf"}, + {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, + {name: "false", cmd: "false", wantErr: "failed:"}, + {name: "empty output", cmd: "true", wantErr: "produced empty output"}, + {name: "empty printf", cmd: "printf ''", wantErr: "produced empty output"}, + {name: "whitespace-only output", cmd: "printf ' \\n'", wantErr: "produced empty output"}, + {name: "multi-line output", cmd: "printf 'a\\nb\\n'", wantErr: "produced multi-line output"}, + // A lone interior CR is a line break too, and one that survives both + // TrimRight("\r\n") and TrimSpace. Refuse it here rather than let it reach + // net/http, which rejects the Authorization header with an opaque error. + {name: "interior carriage return", cmd: "printf 'a\\rb'", wantErr: "produced multi-line output"}, + {name: "multi-line error names the fix", cmd: "printf 'a\\nb\\n'", wantErr: "pipe through 'head -n1'"}, + // Every other control byte net/http rejects (httpguts.ValidHeaderFieldValue: + // anything < 0x20 except TAB, plus DEL) must be named here rather than reach + // the request as an opaque "invalid header field value" failure. + {name: "nul byte", cmd: "printf 'sk-a\\0b'", wantErr: "control byte 0x00 at offset 4"}, + {name: "vertical tab", cmd: "printf 'sk-a\\013b'", wantErr: "control byte 0x0B at offset 4"}, + {name: "form feed", cmd: "printf 'sk-a\\014b'", wantErr: "control byte 0x0C at offset 4"}, + {name: "delete byte", cmd: "printf 'sk-a\\177b'", wantErr: "control byte 0x7F at offset 4"}, + // TAB is legal in a header value, so it survives (interior only; TrimSpace + // takes the edges). + {name: "interior tab kept", cmd: "printf 'sk-a\\tb\\n'", want: "sk-a\tb"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + // Boundary: exactly the cap is fine, one byte more is refused. The child + // dies of SIGPIPE as soon as we stop accepting, so this stays fast. + {name: "output exactly at cap", cmd: "head -c 65536 /dev/zero | tr '\\0' a", want: strings.Repeat("a", keyCmdMaxOutput)}, + {name: "output over cap", cmd: "yes aaaaaaaaaa | head -c 200000 | tr -d '\\n'", wantErr: "produced more than 64KiB of output"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (output %q)", tt.wantErr, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveKeyCmd_Timeout(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + // `sleep 5` inherits the stdout pipe and outlives the SIGKILL'd shell, so + // without a shrunk WaitDelay this test waits the full default 5s. + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + _, err := resolveKeyCmd("sleep 5 2>/dev/null", "api_key_cmd for provider \"x\"") + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// A grandchild that inherited the stdout pipe keeps it open after the shell +// exits, which used to block Wait until the grandchild died. WaitDelay bounds +// that: this must finish in well under the 30s sleep. +func TestResolveKeyCmd_WaitDelayBoundsOrphanHoldingPipe(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + // The grandchild must keep the inherited *stdout* pipe open (that is the case + // under test) but not our stderr: it outlives the test, and `go test` reads + // the test binary's stderr until EOF, so leaving it attached would stall the + // run for the full sleep even though resolveKeyCmd returned immediately. + start := time.Now() + _, err := resolveKeyCmd("sleep 30 2>/dev/null & printf tok", `api_key_cmd for provider "x"`) + elapsed := time.Since(start) + + if elapsed > 5*time.Second { + t.Fatalf("took %s; WaitDelay did not bound the orphaned grandchild", elapsed) + } + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child /dev/null, `read` sees EOF and prints +// nothing, so this would fail with "produced empty output" instead. +// +// os.Stdin under `go test` is not a usable prompt source, so swap in a pipe. +// Mutating the global is safe here: this test is not parallel, and the only +// parallel tests in the package are subtests of TestResolveKeyCmd, which +// finishes before any later top-level test starts. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + // Written and closed up front (well under the pipe buffer, so no blocking) + // so the child reads a full line and then EOF. + if _, err := w.WriteString("passphrase-from-stdin\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + got, err := resolveKeyCmd(`read -r x; printf %s "$x"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("false", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go new file mode 100644 index 0000000..6051f2c --- /dev/null +++ b/internal/llm/keycmd_unix.go @@ -0,0 +1,34 @@ +//go:build !windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "os/exec" +) + +// newKeyCmd builds the OS-specific shell invocation (sh -c on Unix) that runs a +// credential command under ctx, so its timeout and cancellation are honored. +// +// Deliberately no SysProcAttr.Setpgid, even though it would let us SIGKILL the +// whole process group and so reap a grandchild the command backgrounded +// (`sleep 200 & printf tok` does outlive resolution today). Setpgid puts the +// child in a group that is not the terminal's foreground group, so the moment it +// reads the tty it takes SIGTTIN and stops -- measured: a child running +// `read -r x nul", `api_key_cmd for provider "x"`) + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child NUL, findstr reads EOF immediately and +// prints nothing, so this would fail with "produced empty output" instead. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + if _, err := w.WriteString("passphrase-from-stdin\r\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + // findstr "^" copies every stdin line to stdout; ^ is passed through verbatim + // under /S rather than treated as cmd.exe's escape character. + got, err := resolveKeyCmd(`findstr "^"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("exit 1", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 835a076..85b0834 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -45,10 +45,11 @@ const ( // openai | openai-responses). Takes priority // over OCR_USE_ANTHROPIC when set. envOCRLLMProtocol = "OCR_LLM_PROTOCOL" - // envOCRLLMTimeout is a global override applied by finalizeResolvedEndpoint after - // ResolveEndpointWithOptions selects a strategy, rather than inside tryOCREnv like other OCR_LLM_* vars. - // This lets it override timeout for all resolution paths (OCR env, config file, - // provider config, Claude Code env, shell RC). + // envOCRLLMTimeout is a global override parsed at the top of + // ResolveEndpointWithOptions and applied by finalizeResolvedEndpoint to + // whichever strategy resolves, rather than inside tryOCREnv like other + // OCR_LLM_* vars. This lets it override timeout for all resolution paths + // (OCR env, config file, provider config, Claude Code env, shell RC). envOCRLLMTimeout = "OCR_LLM_TIMEOUT" envOCRUseAnthropic = "OCR_USE_ANTHROPIC" ) @@ -83,6 +84,18 @@ func ResolveEndpointWithModelOverride(configPath, modelOverride string) (Resolve func ResolveEndpointWithOptions(configPath string, opts ResolveOptions) (ResolvedEndpoint, error) { opts.Provider = strings.TrimSpace(opts.Provider) opts.Model = strings.TrimSpace(opts.Model) + + // The global env overrides are parsed before any strategy runs, even though + // they are applied to the endpoint afterwards. Parsing them inside + // finalizeResolvedEndpoint would let a typo'd OCR_LLM_TIMEOUT ("30s") or an + // unparseable OCR_LLM_EXTRA_HEADERS abort resolution *after* api_key_cmd + // already prompted 1Password/pinentry/Touch ID for a credential that then + // gets discarded. + env, err := parseEnvOverrides() + if err != nil { + return ResolvedEndpoint{}, err + } + if opts.Provider != "" { ep, ok, err := tryOCRConfig(configPath, opts) if err != nil { @@ -95,7 +108,7 @@ func ResolveEndpointWithOptions(configPath string, opts ResolveOptions) (Resolve } return ResolvedEndpoint{}, fmt.Errorf("resolve OCR config file: provider %q is not configured in %s section because the config file does not exist", opts.Provider, section) } - return finalizeResolvedEndpoint("OCR config file", ep) + return finalizeResolvedEndpoint("OCR config file", ep, env), nil } strategies := []struct { @@ -114,39 +127,58 @@ func ResolveEndpointWithOptions(configPath string, opts ResolveOptions) (Resolve return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", strategy.name, err) } if ok && ep.URL != "" && ep.Token != "" && ep.Model != "" { - return finalizeResolvedEndpoint(strategy.name, ep) + return finalizeResolvedEndpoint(strategy.name, ep, env), nil } } return ResolvedEndpoint{}, fmt.Errorf("no valid LLM endpoint configured; one of OCR_LLM_URL/OCR_LLM_TOKEN/OCR_LLM_MODEL, ~/.opencodereview/config.json, or ANTHROPIC_BASE_URL/ANTHROPIC_AUTH_TOKEN/ANTHROPIC_MODEL must be set") } -func finalizeResolvedEndpoint(source string, ep ResolvedEndpoint) (ResolvedEndpoint, error) { +// envOverrides holds the global OCR_LLM_* overrides that apply to whichever +// strategy resolves the endpoint. Parsed once, up front — see the call site in +// ResolveEndpointWithOptions for why the timing matters. +type envOverrides struct { + timeout time.Duration + hasTimeout bool + headers map[string]string +} + +func parseEnvOverrides() (envOverrides, error) { + var env envOverrides + var err error + env.timeout, env.hasTimeout, err = parseTimeoutEnv() + if err != nil { + return envOverrides{}, err + } + if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { + env.headers, err = ParseExtraHeaders(raw) + if err != nil { + return envOverrides{}, fmt.Errorf("%s: %w", envOCRLLMExtraHeaders, err) + } + } + return env, nil +} + +// finalizeResolvedEndpoint stamps the source label, strips the model suffix and +// applies the global env overrides, which win over config-file values. +func finalizeResolvedEndpoint(source string, ep ResolvedEndpoint, env envOverrides) ResolvedEndpoint { if ep.Source == "" { ep.Source = source } ep.Model = stripModelSuffix(ep.Model) - envTimeout, ok, err := parseTimeoutEnv() - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", source, err) + if env.hasTimeout { + ep.Timeout = env.timeout } - if ok { - ep.Timeout = envTimeout - } - if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { - envHeaders, err := ParseExtraHeaders(raw) - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", source, err) - } + if env.headers != nil { if ep.ExtraHeaders == nil { - ep.ExtraHeaders = envHeaders + ep.ExtraHeaders = env.headers } else { - for key, value := range envHeaders { + for key, value := range env.headers { ep.ExtraHeaders[key] = value } } } - return ep, nil + return ep } // parseTimeoutEnv reads and validates the OCR_LLM_TIMEOUT environment variable. @@ -240,9 +272,10 @@ type llmFileConfig struct { AuthToken string `json:"auth_token,omitempty"` AuthHeader string `json:"auth_header,omitempty"` Model string `json:"model,omitempty"` - Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic - UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty - TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds + AuthTokenCmd string `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty + Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic + UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty + TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` RetryCodes []int `json:"retry_codes,omitempty"` @@ -251,6 +284,7 @@ type llmFileConfig struct { // providerEntryConfig represents a single provider entry in config.json. type providerEntryConfig 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"` @@ -317,14 +351,47 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("provider %q is set but not configured in %s section", cfg.Provider, section) } + // Pick the credential source here, but run api_key_cmd only just before + // returning (see below): a config typo must not trigger a secret-manager + // prompt before the cheap validation below has had a chance to fail. + // A whitespace-only api_key is a typo, not a credential: treat it as unset so + // it cannot silently shadow a working api_key_cmd (which otherwise resolves to + // a 401 with the command never running). A key with real content is used + // verbatim -- unlike command stdout, which has a mechanical trailing newline + // to strip, a static value has no artifact that trimming must undo. apiKey := entry.APIKey - if apiKey == "" { - if isPreset && preset.EnvVar != "" { - apiKey = os.Getenv(preset.EnvVar) + if strings.TrimSpace(apiKey) == "" { + apiKey = "" + } + // Same rule for the command: `sh -c " "` exits 0 with no output, so a + // whitespace-only api_key_cmd would suppress the env fallback and then fail + // with "produced empty output". Treating it as unset keeps the typo from + // being more disruptive than the equivalent typo in api_key. + apiKeyCmd := entry.APIKeyCmd + if strings.TrimSpace(apiKeyCmd) == "" { + apiKeyCmd = "" + } + switch { + case apiKey != "": + // Static api_key always wins. Warn (don't error) if a command is also set, + // so a config that keeps api_key_cmd as a deliberate fallback still works. + if apiKeyCmd != "" { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case apiKeyCmd == "" && isPreset && preset.EnvVar != "": + // Env var is the last resort: only when neither api_key nor api_key_cmd + // is set, and only for preset providers (custom ones have no fallback). + // Same whitespace rule as the static key above, so `export + // ANTHROPIC_API_KEY=" "` reports "no api_key configured" instead of + // sending `Authorization: Bearer ` and getting an opaque 401. + if v := os.Getenv(preset.EnvVar); strings.TrimSpace(v) != "" { + apiKey = v } } - if apiKey == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + // No credential at all is still an error here, before any other validation: + // only the command's *execution* is deferred, not the emptiness check. + if apiKey == "" && apiKeyCmd == "" { + return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) } var url, protocol, authHeader, model string @@ -430,6 +497,18 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, url = ensureMessagesSuffix(url) } + // Single api_key_cmd resolution site for both preset and custom providers, + // as late as possible: everything above can fail without running the + // command. apiKey is empty here only when api_key_cmd is set (guaranteed by + // the emptiness check above), and a failing command is a hard error. + if apiKey == "" { + resolved, err := resolveKeyCmd(apiKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err + } + apiKey = resolved + } + return ResolvedEndpoint{ URL: url, Token: apiKey, @@ -451,9 +530,30 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, if modelOverride != "" { model = modelOverride } - if cfg.Llm.URL == "" || cfg.Llm.AuthToken == "" || model == "" { + // Fall through to later strategies when the legacy block is incomplete. This + // includes the case where neither auth_token nor auth_token_cmd is set — and, + // critically, an incomplete block (e.g. missing url) never runs auth_token_cmd. + // "Incomplete" is judged after modelOverride is applied above, so a block + // missing only `model` is complete under --model and does run the command; + // that is the documented contract of ResolveEndpointWithModelOverride. + // Whitespace-only auth_token is treated as unset, same as api_key above, so it + // cannot shadow a working auth_token_cmd; same rule for the command itself. + token := cfg.Llm.AuthToken + if strings.TrimSpace(token) == "" { + token = "" + } + tokenCmd := cfg.Llm.AuthTokenCmd + if strings.TrimSpace(tokenCmd) == "" { + tokenCmd = "" + } + if cfg.Llm.URL == "" || model == "" || (token == "" && tokenCmd == "") { return ResolvedEndpoint{}, false, nil } + // Static auth_token always wins; warn if a command is also set. The command + // itself runs only just before returning, after the validation below. + if token != "" && tokenCmd != "" { + fmt.Fprintln(os.Stderr, "[ocr] WARNING: llm config has both auth_token and auth_token_cmd set; using the static auth_token") + } // llm.protocol (normalized) wins over use_anthropic when set. protocol := "" @@ -497,9 +597,21 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } + // Runs last, after every cheap validation above: token is empty here only for + // an otherwise-complete block whose auth_token_cmd is set (guaranteed by the + // incompleteness check above), so a failing command is a hard error and an + // incomplete or invalid block never prompts for a credential. + if token == "" { + resolved, err := resolveKeyCmd(tokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } + return ResolvedEndpoint{ URL: cfg.Llm.URL, - Token: cfg.Llm.AuthToken, + Token: token, Model: model, Protocol: protocol, AuthHeader: authHeader, diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go new file mode 100644 index 0000000..f5b742b --- /dev/null +++ b/internal/llm/resolver_keycmd_test.go @@ -0,0 +1,444 @@ +//go:build !windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +// Every test in this file drives a credential command, and all of them are POSIX +// shell (`printf`, `exit N`), which would run through `cmd /C` on Windows. + +package llm + +import ( + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeConfigJSON(t *testing.T, cfg configFile) string { + t.Helper() + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + p := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(p, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + return p +} + +// (a) api_key_cmd resolves when no static key is present. +func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q", ep.Token, "sk-from-cmd") + } +} + +// (a2) the command runs exactly once per resolution. "No caching" is correct +// today only because resolution happens once per process; a second call would +// mean a second pinentry prompt per review. +func TestResolveEndpoint_APIKeyCmdRunsExactlyOnce(t *testing.T) { + clearAllEnv(t) + counter := filepath.Join(t.TempDir(), "runs") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": { + APIKeyCmd: "echo run >> " + counter + "; printf 'sk-once\\n'", + Model: "claude-sonnet-4-6", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-once" { + t.Fatalf("Token = %q, want %q", ep.Token, "sk-once") + } + data, err := os.ReadFile(counter) + if err != nil { + t.Fatalf("read counter file: %v", err) + } + if got := strings.Count(string(data), "\n"); got != 1 { + t.Errorf("api_key_cmd ran %d times, want exactly 1 (counter file %q)", got, data) + } +} + +// (b) static api_key wins even when api_key_cmd is also set — and the command +// does not run at all. Asserting only on ep.Token would pass just as well if the +// command ran and its output were discarded, which for a real config means a +// pinentry/Touch ID prompt on every review that keeps a command as a fallback. +func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { + clearAllEnv(t) + marker := filepath.Join(t.TempDir(), "ran") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": { + APIKey: "sk-static", + APIKeyCmd: "touch " + marker + "; printf 'sk-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } + if _, err := os.Stat(marker); err == nil { + t.Error("api_key_cmd executed even though a static api_key was set") + } +} + +// (b4) a whitespace-only api_key_cmd is a typo, not a command: it must not +// suppress the env-var fallback the way a real command does. Same rule the +// static api_key already follows. +func TestResolveEndpoint_WhitespaceOnlyCmdFallsBackToEnv(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", "sk-from-env") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: " ", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-env" { + t.Errorf("Token = %q, want %q (whitespace-only api_key_cmd must be treated as unset)", ep.Token, "sk-from-env") + } +} + +// (b5) same rule on the legacy block: whitespace-only auth_token_cmd leaves the +// block incomplete rather than running an empty command and hard-failing. +func TestResolveEndpoint_LegacyWhitespaceOnlyCmdIsUnset(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_BASE_URL", "https://env.test") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "sk-from-env") + t.Setenv("ANTHROPIC_MODEL", "m") + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{URL: "https://example.test", Model: "m", AuthTokenCmd: " \t "}, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-env" { + t.Errorf("Token = %q, want %q (whitespace-only auth_token_cmd must be treated as unset)", ep.Token, "sk-from-env") + } +} + +// captureStderr swaps os.Stderr for a pipe around fn and returns what was written. +// Output here is tiny, so reading after the writer is closed avoids any pipe-buffer +// deadlock without a goroutine. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + orig := os.Stderr + os.Stderr = w + defer func() { os.Stderr = orig }() + + fn() + + if err := w.Close(); err != nil { + t.Fatalf("close pipe writer: %v", err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read captured stderr: %v", err) + } + return string(out) +} + +// (b2) when both api_key and api_key_cmd are set, a warning is emitted on stderr +// and the resolved token is still the static api_key. +func TestResolveEndpoint_BothSetWarnsAndUsesStaticKey(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } + // Match the message, not the log prefix, so this does not break when the + // warning prefix is restyled. + want := `provider "anthropic" has both api_key and api_key_cmd set; using the static api_key` + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (e2) legacy path: both auth_token and auth_token_cmd set -> warning + static wins. +func TestResolveEndpoint_LegacyBothSetWarnsAndUsesStaticToken(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "legacy-static", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-static" { + t.Errorf("Token = %q, want %q (static auth_token must win)", ep.Token, "legacy-static") + } + want := "llm config has both auth_token and auth_token_cmd set; using the static auth_token" + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (b3) a whitespace-only api_key is a typo, not a credential: it must not shadow +// the command (which used to resolve Token=" " -> 401, command never run), and +// the both-set warning must stay quiet since nothing is really being shadowed. +func TestResolveEndpoint_WhitespaceOnlyStaticKeyUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: " ", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only api_key must not shadow api_key_cmd)", ep.Token, "sk-from-cmd") + } + if strings.Contains(stderr, "both api_key and api_key_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } +} + +// (e3b) the same whitespace rule reaches the env-var fallback, which is the last +// source in the chain and had been exempt: a whitespace-only value there used to +// resolve successfully and send `Authorization: Bearer `, producing an opaque 401 +// instead of naming the missing credential. +func TestResolveEndpoint_WhitespaceOnlyEnvVarIsNotACredential(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", " ") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected an error: a whitespace-only env var is not a credential") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error %q does not name the missing credential", err.Error()) + } +} + +// (e4) same on the legacy path. +func TestResolveEndpoint_LegacyWhitespaceOnlyStaticTokenUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "\t\n ", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only auth_token must not shadow auth_token_cmd)", ep.Token, "legacy-from-cmd") + } + if strings.Contains(stderr, "both auth_token and auth_token_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } +} + +// (c) custom provider with api_key_cmd resolves (custom providers have no env fallback). +func TestResolveEndpoint_CustomProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + APIKeyCmd: "printf 'gw-token\\n'", + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-8b", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "gw-token" { + t.Errorf("Token = %q, want %q", ep.Token, "gw-token") + } +} + +// (d) a failing api_key_cmd is a hard error, not a silent fallback. +func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected hard error from failing api_key_cmd, got nil") + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } +} + +// (d2) the property the design calls non-negotiable: a misconfigured credential +// command must never silently downgrade to an env var. TestResolveEndpoint_ +// ProviderAPIKeyCmdFailsHard runs under clearAllEnv, so it would still pass if +// someone reintroduced an env-var fallback on command failure; this one sets the +// preset's env var so that regression cannot hide. +func TestResolveEndpoint_APIKeyCmdFailureDoesNotFallBackToEnv(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing api_key_cmd, got nil (Token %q)", ep.Token) + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } + // Not an assertion on ep: every error path returns a zero ResolvedEndpoint, so + // ep.Token is "" by construction whenever err != nil. The witness that no + // fallback happened is err being non-nil at all -- with the env var set, a + // silent fallback would have returned success. +} + +// (e) legacy auth_token_cmd resolves on an otherwise-complete llm block. +func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "printf 'legacy-token\\n'", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-token" { + t.Errorf("Token = %q, want %q", ep.Token, "legacy-token") + } +} + +// (e3) legacy path: an otherwise-complete llm block whose auth_token_cmd fails is +// a hard error. The Claude Code env vars are set to prove it does not fall through +// to that strategy -- a failing credential command must not be papered over by a +// lower-priority source. +func TestResolveEndpoint_LegacyAuthTokenCmdFailsHard(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_BASE_URL", "https://cc.example.com") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "cc-env-token") + t.Setenv("ANTHROPIC_MODEL", "claude-sonnet-4-6") + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing auth_token_cmd, got nil (Source %q, Token %q)", ep.Source, ep.Token) + } + if !strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("error %q does not mention auth_token_cmd", err.Error()) + } +} + +// (f) an incomplete legacy block (missing url) with auth_token_cmd set does NOT +// run the command and falls through to later strategies. +func TestResolveEndpoint_LegacyIncompleteDoesNotRunCmd(t *testing.T) { + clearAllEnv(t) + // Command would exit non-zero if ever executed; if it ran, we'd see that + // error instead of the generic "no valid endpoint" fall-through error. + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + // URL intentionally omitted -> incomplete + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected no-endpoint error, got nil") + } + if strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("command should not have run for incomplete legacy config; error: %v", err) + } + if !strings.Contains(err.Error(), "no valid LLM endpoint") { + t.Errorf("expected fall-through no-endpoint error, got: %v", err) + } +} diff --git a/internal/llm/resolver_shellrc_test.go b/internal/llm/resolver_shellrc_test.go index e2a084d..cd0fd59 100644 --- a/internal/llm/resolver_shellrc_test.go +++ b/internal/llm/resolver_shellrc_test.go @@ -9,9 +9,19 @@ import ( "testing" ) +// setShellRCHome points os.UserHomeDir at dir. shellRCFiles resolves the home +// dir through os.UserHomeDir, which reads USERPROFILE on Windows and never +// falls back to HOME, so redirecting HOME alone left these tests scanning the +// real profile. Set both; the one that does not apply is harmless. +func setShellRCHome(t *testing.T, dir string) { + t.Helper() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) +} + func TestShellRCFiles(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setShellRCHome(t, home) if got := shellRCFiles(); len(got) != 0 { t.Errorf("shellRCFiles() with no rc files = %v, want empty", got) @@ -29,7 +39,7 @@ func TestShellRCFiles(t *testing.T) { func TestTryShellRC(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setShellRCHome(t, home) // No rc files: not found, no error. if _, ok, err := tryShellRC(""); ok || err != nil { diff --git a/internal/llm/resolver_test.go b/internal/llm/resolver_test.go index 6a60ad4..bf07bb0 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -271,6 +271,14 @@ func clearAllEnv(t *testing.T) { } { t.Setenv(k, "") } + // Point os.UserHomeDir at an empty dir so the tryShellRC strategy cannot read + // the developer's (or a self-hosted CI runner's) real ~/.zshrc: one exporting + // the ANTHROPIC_* trio would resolve a live endpoint and break every test that + // asserts resolution fails. HOME covers Unix, USERPROFILE Windows; setting the + // one that does not apply is harmless. + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) } func writeResolverConfig(t *testing.T, cfg configFile) (string, []byte) { @@ -834,7 +842,7 @@ func TestResolveEndpoint_MiniMaxProviderRejectsOtherRegionEnv(t *testing.T) { }) _, err := ResolveEndpointWithOptions(path, ResolveOptions{Provider: tt.provider}) - if err == nil || !strings.Contains(err.Error(), "has no api_key configured and no environment variable fallback found") { + if err == nil || !strings.Contains(err.Error(), "has no api_key or api_key_cmd configured and no environment variable fallback found") { t.Fatalf("error = %v", err) } }) @@ -970,6 +978,38 @@ func TestResolveEndpoint_CustomProviderMissingFields(t *testing.T) { } } +func TestResolveEndpoint_CustomProviderNoEnvFallback(t *testing.T) { + clearAllEnv(t) + // A preset provider would pick this up; a custom provider must not, since it + // has no associated env var. The api_key/api_key_cmd precedence relies on it. + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + + cfg := configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-70b", + // No api_key and no api_key_cmd. + }, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected error: custom providers have no environment variable fallback") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error = %v, want the missing-credential error", err) + } +} + func TestResolveEndpoint_CustomProviderModelFromTopLevel(t *testing.T) { clearAllEnv(t) @@ -1133,6 +1173,110 @@ func TestResolveEndpointWithModelOverride_InvalidModelInPresetList(t *testing.T) } } +func TestResolveEndpointWithModelOverride_InvalidModelDoesNotRunAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + + // The command is guaranteed to fail, so the error it would produce doubles as + // a witness that it ran: a bad --model must fail on validation instead, with + // no secret-manager prompt. + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpointWithModelOverride(cfgPath, "claude-opsu-4-6") + if err == nil { + t.Fatal("expected error for invalid model override") + } + if !strings.Contains(err.Error(), "not available for provider") { + t.Errorf("error message should mention model unavailability, got: %v", err) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before model validation, got: %v", err) + } +} + +// A bad global env override must be rejected before any strategy runs, for the +// same reason as the model check above: OCR_LLM_TIMEOUT="30s" (the field wants a +// bare integer) used to be parsed only after an endpoint resolved, so the user +// authenticated to 1Password/Touch ID and then got a config error. Same witness +// trick: the command cannot succeed, so its error proves it ran. +func TestResolveEndpointWithModelOverride_BadEnvOverrideDoesNotRunAPIKeyCmd(t *testing.T) { + tests := []struct { + name string + env string + value string + wantErr string + wantErr2 string + }{ + { + name: "non-integer timeout", + env: "OCR_LLM_TIMEOUT", + value: "30s", + wantErr: "OCR_LLM_TIMEOUT must be an integer (seconds)", + }, + { + name: "negative timeout", + env: "OCR_LLM_TIMEOUT", + value: "-30", + wantErr: "OCR_LLM_TIMEOUT", + }, + { + name: "reserved extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "authorization=leak", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "reserved header", + }, + { + name: "malformed extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "no-equals-sign", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "expected key=value", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearAllEnv(t) + t.Setenv(tt.env, tt.value) + + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected error for %s=%q", tt.env, tt.value) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr) + } + if tt.wantErr2 != "" && !strings.Contains(err.Error(), tt.wantErr2) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr2) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before %s was validated, got: %v", tt.env, err) + } + }) + } +} + func TestResolveEndpointWithModelOverride_ValidModelInCustomProviderList(t *testing.T) { clearAllEnv(t) diff --git a/internal/scan/provider_more_test.go b/internal/scan/provider_more_test.go index 7d9ee36..81b1b7f 100644 --- a/internal/scan/provider_more_test.go +++ b/internal/scan/provider_more_test.go @@ -7,6 +7,7 @@ import ( "context" "os" "path/filepath" + "runtime" "sort" "testing" @@ -60,6 +61,12 @@ func TestProvider_Enumerate_NonRegularSkip(t *testing.T) { // TestProvider_Enumerate_SniffError covers the binary-sniff error branch: a file // that cannot be opened for sniffing is skipped with a warning. func TestProvider_Enumerate_SniffError(t *testing.T) { + // Chmod(0000) on Windows only sets the read-only bit, so os.Open still + // succeeds and locked.go is enumerated instead of skipped. (The Geteuid + // guard below cannot cover this: Geteuid returns -1 on Windows, never 0.) + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Geteuid() == 0 { t.Skip("root bypasses file permission checks") } diff --git a/internal/session/list_error_test.go b/internal/session/list_error_test.go index 5ba6cac..c077200 100644 --- a/internal/session/list_error_test.go +++ b/internal/session/list_error_test.go @@ -6,6 +6,7 @@ package session import ( "os" "path/filepath" + "runtime" "testing" ) @@ -13,6 +14,13 @@ import ( // error): when the computed sessions dir path is occupied by a regular file, // os.ReadDir fails with ENOTDIR and ListSessions must surface it. func TestListSessions_DirIsFile(t *testing.T) { + // There is no ENOTDIR to observe on Windows: os.Open of the blocking file + // succeeds, and the directory query against that handle comes back in a form + // os.(*File).readdir reports as an empty listing rather than an error, so + // ListSessions returns no sessions and no error and this branch is unreachable. + if runtime.GOOS == "windows" { + t.Skip("os.ReadDir does not report ENOTDIR for a regular file on Windows") + } t.Setenv("HOME", t.TempDir()) repoDir := t.TempDir() diff --git a/internal/session/persist_test.go b/internal/session/persist_test.go index 604518d..b15f67f 100644 --- a/internal/session/persist_test.go +++ b/internal/session/persist_test.go @@ -243,7 +243,12 @@ func TestSessionFilePermissions(t *testing.T) { func TestFinalizeSurfacesWriterCreationErrorWithoutStdout(t *testing.T) { tmpHome := t.TempDir() + // The writer resolves the home dir with os.UserHomeDir, which reads + // USERPROFILE on Windows and never falls back to HOME. With HOME alone the + // blocking file below landed in the temp dir while the writer kept using the + // real profile, so creation succeeded and there was no failure to surface. t.Setenv("HOME", tmpHome) + t.Setenv("USERPROFILE", tmpHome) // A regular file at this path makes creation of the sessions directory fail // deterministically on every platform. diff --git a/internal/viewer/handler_test.go b/internal/viewer/handler_test.go index b56d892..40a4696 100644 --- a/internal/viewer/handler_test.go +++ b/internal/viewer/handler_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -70,6 +71,12 @@ func TestHandleRepos_UnreadableRoot(t *testing.T) { } func TestHandleRepos_PermissionDenied(t *testing.T) { + // Chmod(0000) on Windows only sets the read-only bit, so ReadDir still + // succeeds and the handler returns 200. (The Getuid guard below cannot cover + // this: Getuid returns -1 on Windows, never 0.) + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/internal/viewer/store_load_test.go b/internal/viewer/store_load_test.go index bc0dc04..94d4fab 100644 --- a/internal/viewer/store_load_test.go +++ b/internal/viewer/store_load_test.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "testing" "github.com/alibaba/open-code-review/internal/session" @@ -572,6 +573,11 @@ func TestLoadSession_ToolCallWithoutRequest(t *testing.T) { } func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so ReadDir still succeeds + // and the repo is discovered rather than skipped. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } @@ -598,6 +604,11 @@ func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { } func TestListSessions_SkipsUnreadableFiles(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so the "bad" file is still + // readable and gets counted as a second session. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index f5a61fb..7b4fcc5 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -151,6 +151,54 @@ The `timeout_sec` keys are not supported by `ocr config set` — edit } ``` +### API key from a command + +Instead of storing a key in the config file, `api_key_cmd` fetches it at +runtime from a secret manager (1Password, `pass`, `gopass`, …). Its trimmed, +single-line stdout becomes the key. The same option is available for the +legacy `llm` block as `auth_token_cmd`. + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +Your OS keyring works the same way, through the tool it already ships with, so +the key lives in the Keychain or Secret Service rather than in `config.json`: + +```bash +# macOS Keychain +ocr config set providers.anthropic.api_key_cmd \ + "security find-generic-password -s ocr-anthropic -w" + +# Linux (Secret Service: GNOME Keyring, KWallet, …) +ocr config set providers.anthropic.api_key_cmd \ + "secret-tool lookup service ocr-anthropic" +``` + +Precedence: a static `api_key` always wins (if both are set, the command is +ignored and a warning is printed); otherwise `api_key_cmd` runs; only if +neither is set does OCR fall back to the provider's environment variable. + +The command runs once per `ocr` invocation and must succeed: a non-zero exit, +empty output, multi-line output, or more than 64KiB of output is a hard error +(OCR never silently falls back). It must complete within 60 seconds, which +includes any time you spend answering a prompt. The command inherits your +terminal's stdin and stderr, so interactive prompts (pinentry, Touch ID) both +appear and can be answered. If the command leaves a background daemon holding +its stdout pipe (`gpg-agent`, a first-use `op` daemon), the credential still +arrives but every `ocr` run pauses an extra 5 seconds waiting for that pipe to +close — redirect the daemon's output (`>/dev/null 2>&1`) to get rid of the wait. + +On Windows the command runs through `cmd.exe`, not `sh`, so a command written +for one is generally not portable to the other: `%VAR%` and `^` are `cmd.exe` +metacharacters, while `$VAR` expansion and `\` escaping do not apply there. +Quoted arguments are passed through verbatim, so +`op read "op://Private/My Vault/api-key"` works as written. + +Since the value is executed as a shell command, `config.json` is trusted +input — keep it owned by you and not writable by anyone else (OCR writes it +with `0600` permissions). + ### Additional retry status codes Some LLM providers use non-standard 4xx status codes for transient errors, such diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 8fd5e2a..de2b5a9 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -149,6 +149,54 @@ Ollama は API key を無視しますが、カスタム provider は空でない } ``` +### API key をコマンドで取得する + +key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット +マネージャー(1Password、`pass`、`gopass` など)から取得できます。前後の空白を +除いた 1 行の stdout が key になります。レガシーの `llm` ブロックにも同等の +`auth_token_cmd` があります。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +OS 標準のキーリングも同じ方法で使えます。OS に付属するコマンドをそのまま指定 +すれば、key は `config.json` ではなく Keychain や Secret Service に保存されます。 + +```bash +# macOS Keychain +ocr config set providers.anthropic.api_key_cmd \ + "security find-generic-password -s ocr-anthropic -w" + +# Linux(Secret Service: GNOME Keyring、KWallet など) +ocr config set providers.anthropic.api_key_cmd \ + "secret-tool lookup service ocr-anthropic" +``` + +優先順位:静的な `api_key` が常に優先されます(両方設定されている場合はコマンドを +無視し、警告を表示します)。それ以外の場合は `api_key_cmd` を実行します。どちらも +設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 + +コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 +空の出力、複数行の出力、64KiB を超える出力はいずれもハードエラーです(OCR が黙って +フォールバックすることはありません)。コマンドはプロンプトへの応答時間も含めて +60 秒以内に完了する必要があります。コマンドは端末の stdin と stderr を引き継ぐため、 +対話的なプロンプト(pinentry、Touch ID)は表示も応答も可能です。コマンドが stdout +パイプを保持したままバックグラウンドのデーモン(`gpg-agent`、初回起動時の `op` +デーモン)を残すと、認証情報は取得できるものの `ocr` の実行ごとにパイプが閉じるのを +5 秒余分に待つことになるため、デーモンの出力をリダイレクト(`>/dev/null 2>&1`) +してください。 + +Windows ではコマンドは `sh` ではなく `cmd.exe` 経由で実行されるため、一方向けに +書いたコマンドは通常そのままでは移植できません。`%VAR%` と `^` は `cmd.exe` の +メタ文字であり、`$VAR` の展開や `\` によるエスケープは適用されません。引用符付きの +引数はそのまま渡されるため、`op read "op://Private/My Vault/api-key"` は記述どおりに +動作します。 + +この値は shell コマンドとして実行されるため、`config.json` は信頼された入力です。 +自分の所有のまま、他のユーザーが書き込めない状態に保ってください(OCR は `0600` +で書き込みます)。 + ### 追加のリトライ対象ステータスコード 一部の LLM プロバイダーでは、レート制限に対して `403` や `400` を返すなど、 diff --git a/pages/src/content/docs/ru/configuration.md b/pages/src/content/docs/ru/configuration.md index 0b86dac..f745bb8 100644 --- a/pages/src/content/docs/ru/configuration.md +++ b/pages/src/content/docs/ru/configuration.md @@ -158,6 +158,60 @@ Ollama игнорирует API-ключ, однако для пользоват } ``` +### Получение API-ключа из команды + +Вместо того чтобы хранить ключ в файле конфигурации, параметр `api_key_cmd` +получает его во время выполнения из менеджера секретов (1Password, `pass`, +`gopass`, …). Ключом становится однострочный вывод команды в stdout с +отброшенными пробелами по краям. Тот же параметр доступен и для устаревшего +раздела `llm` — под именем `auth_token_cmd`. + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +Точно так же работает связка ключей вашей ОС — через команду, которая уже +входит в её состав, поэтому ключ хранится в Keychain или Secret Service, а не +в `config.json`: + +```bash +# macOS Keychain +ocr config set providers.anthropic.api_key_cmd \ + "security find-generic-password -s ocr-anthropic -w" + +# Linux (Secret Service: GNOME Keyring, KWallet, …) +ocr config set providers.anthropic.api_key_cmd \ + "secret-tool lookup service ocr-anthropic" +``` + +Приоритет: заданный `api_key` всегда имеет приоритет над командой (если заданы +оба, команда игнорируется и выводится предупреждение); иначе выполняется +`api_key_cmd`; и только если не задано ни то, ни другое, OCR возвращается к +переменной окружения провайдера. + +Команда выполняется один раз за запуск `ocr` и должна завершиться успешно: +ненулевой код возврата, пустой вывод, многострочный вывод или вывод объёмом +больше 64 КиБ считаются ошибкой и прерывают работу (OCR никогда не переключается +на резервный вариант молча). Команда должна уложиться в 60 секунд, включая +время, которое вы тратите на ответ на запрос. Команда наследует stdin и stderr +вашего терминала, поэтому интерактивные запросы (pinentry, Touch ID) и +отображаются, и допускают ответ. Если команда оставляет после себя фоновую +службу, удерживающую её канал stdout (`gpg-agent`, запускаемая при первом +использовании служба `op`), учётные данные всё равно будут получены, но каждый +запуск `ocr` дополнительно ждёт 5 секунд, пока этот канал не закроется, — +перенаправьте вывод службы (`>/dev/null 2>&1`), чтобы избавиться от ожидания. + +В Windows команда выполняется через `cmd.exe`, а не через `sh`, поэтому +команда, написанная для одной из этих оболочек, как правило, не переносится в +другую: `%VAR%` и `^` — метасимволы `cmd.exe`, а раскрытие `$VAR` и +экранирование через `\` там не действуют. Аргументы в кавычках передаются без +изменений, поэтому `op read "op://Private/My Vault/api-key"` работает как +написано. + +Поскольку это значение выполняется как команда оболочки, `config.json` +считается доверенным вводом — он должен принадлежать вам и быть недоступен для +записи другим пользователям (OCR записывает его с правами `0600`). + ### Дополнительные HTTP-коды для повторных попыток Некоторые LLM-провайдеры используют нестандартные HTTP-коды 4xx для временных diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index e33f243..1ee062b 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -141,6 +141,48 @@ provider 没有环境变量回退),所以设任意占位值即可。模型 } ``` +### 通过命令获取 API key + +除了把 key 直接写进配置文件,还可以用 `api_key_cmd` 在运行时从密钥管理器 +(1Password、`pass`、`gopass` 等)获取。命令去除首尾空白后的单行 stdout 即为 +key。旧版 `llm` 配置块也有对应的 `auth_token_cmd`。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +操作系统自带的密钥环同理,直接用系统已有的命令即可,key 保存在 Keychain 或 +Secret Service 中,而不是 `config.json` 里: + +```bash +# macOS Keychain +ocr config set providers.anthropic.api_key_cmd \ + "security find-generic-password -s ocr-anthropic -w" + +# Linux(Secret Service:GNOME Keyring、KWallet 等) +ocr config set providers.anthropic.api_key_cmd \ + "secret-tool lookup service ocr-anthropic" +``` + +优先级:静态 `api_key` 始终优先(两者都设置时忽略命令并打印警告);否则运行 +`api_key_cmd`;只有两者都未设置时,OCR 才回退到 provider 对应的环境变量。 + +命令在每次 `ocr` 调用时运行一次,且必须成功:非零退出、空输出、多行输出或超过 +64KiB 的输出都会被视为硬错误(OCR 绝不会静默回退)。命令须在 60 秒内完成,这也 +包括你回应提示所花的时间。命令会继承你终端的 stdin 和 stderr,因此交互式提示 +(pinentry、Touch ID)既能显示也能作答。如果命令留下了仍持有其 stdout 管道的后台 +守护进程(`gpg-agent`、首次使用时启动的 `op` 守护进程),凭据依然能取到,但每次 +`ocr` 调用都会额外等待 5 秒直到该管道关闭——把守护进程的输出重定向掉 +(`>/dev/null 2>&1`)即可消除这段等待。 + +在 Windows 上命令通过 `cmd.exe` 而非 `sh` 执行,因此为其中一方编写的命令通常 +无法直接移植到另一方:`%VAR%` 和 `^` 是 `cmd.exe` 的元字符,而 `$VAR` 展开和 `\` +转义在那里并不适用。带引号的参数会原样传递,因此 +`op read "op://Private/My Vault/api-key"` 可以按原样使用。 + +由于这个值会作为 shell 命令执行,`config.json` 属于可信输入——请确保它归你所有、 +其他用户不可写(OCR 写入时使用 `0600` 权限)。 + ### 额外的重试状态码 有些 LLM 提供商会用非标准的 4xx 状态码表示临时错误,例如在限流时返回 `403` 或