From 43bdfaf3ba43ea24e05b35c70aa6c9fa0100bed4 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Fri, 17 Jul 2026 22:00:52 +0200 Subject: [PATCH 1/4] feat(config): resolve api_key/auth_token from a command (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `api_key_cmd` (provider entries) and `auth_token_cmd` (legacy llm block) so the LLM credential can be fetched from a secret manager at review time instead of stored plaintext in config.json — same pattern as git credential.helper / AWS credential_process. Resolution precedence (single site, presets and custom providers alike): static api_key always wins (stderr warning if a command is also set) → api_key_cmd → preset env var → error. The legacy llm block gets a mirrored auth_token_cmd; an incomplete legacy block never executes the command, and a set-but-failing command on a complete block is a hard error (never a silent fallback). Command execution is a build-tag split (sh -c / cmd /C) with a 60s timeout; the child's stderr passes through so pinentry/1Password/op prompts stay visible. Stdout is trimmed and used in memory only — never written to config or logged. Empty, whitespace-only, multi-line, and timed-out output are all hard errors. No caching (resolution runs once per process). - config set: api_key_cmd/auth_token_cmd are settable and round-trip; not masked (they are command lines, not secrets). - TUI cloneProviderEntry preserves api_key_cmd. - docs: 'API key from a command' section in configuration.md (en/zh/ja). Tests: table-driven runner matrix (success/trim/non-zero/empty/ whitespace/multi-line/not-found/timeout) + resolver precedence and legacy-fallthrough rows. Coverage 81.3%; Windows arm compile-checked (CI is Linux-only). --- cmd/opencodereview/config_cmd.go | 11 +- cmd/opencodereview/config_cmd_test.go | 4 +- cmd/opencodereview/provider_tui.go | 1 + cmd/opencodereview/provider_tui_funcs_test.go | 37 +++++ .../config/testconnection/testconnection.go | 3 + internal/llm/keycmd.go | 53 +++++++ internal/llm/keycmd_test.go | 75 +++++++++ internal/llm/keycmd_unix.go | 15 ++ internal/llm/keycmd_windows.go | 15 ++ internal/llm/resolver.go | 49 ++++-- internal/llm/resolver_keycmd_test.go | 146 ++++++++++++++++++ internal/llm/resolver_test.go | 2 +- pages/src/content/docs/en/configuration.md | 33 ++++ pages/src/content/docs/ja/configuration.md | 33 ++++ pages/src/content/docs/zh/configuration.md | 29 ++++ 15 files changed, 492 insertions(+), 14 deletions(-) create mode 100644 internal/llm/keycmd.go create mode 100644 internal/llm/keycmd_test.go create mode 100644 internal/llm/keycmd_unix.go create mode 100644 internal/llm/keycmd_windows.go create mode 100644 internal/llm/resolver_keycmd_test.go diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 8a5bfce7..d9dba4af 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -285,6 +285,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 +326,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 +388,7 @@ var supportedConfigKeys = []string{ "mcp_servers..", "llm.url", "llm.auth_token", + "llm.auth_token_cmd", "llm.auth_header", "llm.model", "llm.protocol", @@ -463,6 +466,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 +551,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 +560,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 +612,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 2084d3a4..17e333a7 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1018,8 +1018,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 { diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 584b5db0..a7a6e7fb 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -1184,6 +1184,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, diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 3af2d93d..3227af9e 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,6 +202,7 @@ 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", @@ -213,6 +215,9 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { 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) } @@ -245,6 +250,38 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { } } +// TestCloneProviderEntry_CopiesEveryField fails when a field is added to +// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and +// ExtraHeaders were once silently dropped. DeepEqual catches a dropped field; +// the reflect sweep is what stops a zero-valued fixture from hiding one. +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) { orig := ProviderEntry{ APIKey: "key", diff --git a/internal/config/testconnection/testconnection.go b/internal/config/testconnection/testconnection.go index d6867d8a..45b1a892 100644 --- a/internal/config/testconnection/testconnection.go +++ b/internal/config/testconnection/testconnection.go @@ -1,6 +1,9 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 alibaba/open-code-review Contributors +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + // Package testconnection loads the LLM test connection task configuration. package testconnection diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go new file mode 100644 index 00000000..f7b8e049 --- /dev/null +++ b/internal/llm/keycmd.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "fmt" + "os" + "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 + +// 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. 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 + + out, err := c.Output() + if ctx.Err() == context.DeadlineExceeded { + return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + } + if err != nil { + // Covers non-zero exit and command-not-found (the shell exits non-zero + // and prints its not-found message on the child's stderr). + return "", fmt.Errorf("%s failed: %w", label, err) + } + + // Trim a trailing line break; multi-line output past that is ambiguous and refused. + trimmed := strings.TrimRight(string(out), "\r\n") + if strings.Contains(trimmed, "\n") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + } + 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 00000000..6991a0af --- /dev/null +++ b/internal/llm/keycmd_test.go @@ -0,0 +1,75 @@ +//go:build !windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "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: "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"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + } + for _, tt := range tests { + tt := tt + 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) { + orig := keyCmdTimeout + keyCmdTimeout = 50 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout = orig }) + + _, err := resolveKeyCmd("sleep 5", "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()) + } +} + +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 00000000..9832ca9c --- /dev/null +++ b/internal/llm/keycmd_unix.go @@ -0,0 +1,15 @@ +//go:build !windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "os/exec" +) + +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "sh", "-c", cmd) +} diff --git a/internal/llm/keycmd_windows.go b/internal/llm/keycmd_windows.go new file mode 100644 index 00000000..496eec69 --- /dev/null +++ b/internal/llm/keycmd_windows.go @@ -0,0 +1,15 @@ +//go:build windows + +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "context" + "os/exec" +) + +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "cmd", "/C", cmd) +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 835a076b..9d4b97b3 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -240,9 +240,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 +252,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"` @@ -318,13 +320,24 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, } apiKey := entry.APIKey - if apiKey == "" { - if isPreset && preset.EnvVar != "" { - apiKey = os.Getenv(preset.EnvVar) + 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 entry.APIKeyCmd != "" { + fmt.Fprintf(os.Stderr, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case entry.APIKeyCmd != "": + resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err } + apiKey = resolved + case isPreset && preset.EnvVar != "": + apiKey = os.Getenv(preset.EnvVar) } if apiKey == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + 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 @@ -451,9 +464,27 @@ 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. + token := cfg.Llm.AuthToken + if cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { return ResolvedEndpoint{}, false, nil } + switch { + case token != "": + // Static auth_token always wins; warn if a command is also set. + if cfg.Llm.AuthTokenCmd != "" { + fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") + } + case cfg.Llm.AuthTokenCmd != "": + // Otherwise-complete legacy block with a set-but-failing command is a hard error. + resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } // llm.protocol (normalized) wins over use_anthropic when set. protocol := "" @@ -499,7 +530,7 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, 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 00000000..fef665c6 --- /dev/null +++ b/internal/llm/resolver_keycmd_test.go @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "encoding/json" + "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") + } +} + +// (b) static api_key wins even when api_key_cmd is also set. +func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(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"}, + }, + }) + 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") + } +} + +// (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()) + } +} + +// (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") + } +} + +// (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_test.go b/internal/llm/resolver_test.go index 6a60ad48..b5c8c3b1 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -834,7 +834,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) } }) diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 58f0cbf6..865d2af9 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -148,6 +148,39 @@ 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, or multi-line output is a hard error (OCR never silently falls +back). It must complete within 60 seconds. The command's stderr is passed +through to your terminal, so interactive prompts (pinentry, Touch ID) still +work. ### Additional retry status codes diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 1ff441a9..39743790 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -146,6 +146,39 @@ 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 回実行され、成功する必要があります。非ゼロ終了、 +空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする +ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの +stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も +引き続き動作します。 ### 追加のリトライ対象ステータスコード diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index f3a33e61..7969f7ee 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -138,6 +138,35 @@ 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` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 +被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 +到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 ### 额外的重试状态码 From 1bf219aae4a046fcfea1e1cabd311dfe8cdaf6b6 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 10:12:52 +0200 Subject: [PATCH 2/4] fix(llm): harden the credential command and cover it on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on the api_key_cmd/auth_token_cmd path, plus the CI job that actually exercises its Windows arm. The 60s timeout was not a real bound. It killed the shell, but a helper that leaves a background process holding the inherited stdout pipe (gpg-agent, pinentry, a first-use `op` daemon) kept Cmd.Wait blocked on the read long after the context died — `api_key_cmd = "sleep 200 & printf tok"` hung for over 90s. Buffer stdout through a writer os/exec copies in its own goroutine and set WaitDelay, which is what lets Wait force the pipe closed; ErrWaitDelay on its own is not a failure, since the command exited and its output is already buffered. Three more ways a resolved value could not be used: - Stdin was /dev/null, so a helper needing a passphrase saw EOF or refused to prompt for lack of a tty. Wired to os.Stdin, which is safe because no path resolves an endpoint while the bubbletea TUI is reading stdin. - Output was unbounded; `cat /dev/urandom` grew the heap without limit. Capped at 64KiB, refusing the write so the child dies of SIGPIPE. - Control bytes reached the Authorization header, where net/http rejects them as an opaque `invalid header field value`. Rejected up front with the offending byte and offset, matching httpguts.ValidHeaderFieldValue. A lone interior CR survived both TrimRight and TrimSpace, so it is now caught as multi-line output. Ordering: the command ran before the rest of the config was known to be usable, so `ocr review --model nonexistent` fired a biometric prompt and only then failed on the model name. Execution is deferred past validation at both sites — the source selection in tryProviderConfig, and ResolveEndpointWithModelOverride, which parsed OCR_LLM_TIMEOUT and OCR_LLM_EXTRA_HEADERS after resolving the credential. A whitespace-only static api_key also used to win precedence over a working api_key_cmd and send `Authorization: Bearer `; it now normalizes to unset, and the Manual TUI tab trims its token like the other two tabs. `ocr config provider` rejected api_key_cmd-only providers in both directions: non-interactively applyOfficialProviderConfig demanded a static key or an env var, and interactively the API-key step could not be confirmed because the field renders blank for such a provider. Both now treat a configured command as satisfying the requirement, and the error messages name the option that would fix it. Windows: the command line goes to cmd.exe through SysProcAttr.CmdLine with /S rather than through Args, because os/exec quotes Args with syscall.EscapeArg, which targets CommandLineToArgvW; cmd.exe is a documented exception whose escaping mangles any command containing a double quote, so `op read "op://Private/My Vault/api-key"` arrived as a single literal filename. Args stays at its one-element default rather than nil (syscall.StartProcess ignores argv when CmdLine is set) so Cmd.String() cannot panic on Args[1:]. CI ran only self-hosted Linux, and the cross-compile job proves the windows arms compile but never runs them, so keycmd_windows.go had zero coverage on any platform. Adds a windows-latest job that vets, tests, builds and smoke-tests natively. It installs Go with setup-go instead of the shared golang:1.26.5 image because GitHub does not support `container:` on Windows runners (actions/runner#904); no -race, since the detector needs a C toolchain there and races are OS-independent; no coverage gate, since the //go:build !windows files legitimately put the total under the Linux job's 80%. Six existing tests needed a guard for that job, none a behavior change: three assert an unreadable path is skipped, but Chmod(0000) on Windows only sets the read-only bit (and their os.Getuid() == 0 guard cannot cover it, since Getuid returns -1 there); TestSaveConfig asserts the 0600 the config is written with, which Windows reports as 0666; the symlink-safety test needs a privilege an unelevated CI account lacks; and the "absolute unchanged" background-path case was passing a rooted but non-absolute path, so it had been exercising the relative branch. Running that job turned up more of the same, all of it in tests and none of it needing a production change. os.UserHomeDir reads USERPROFILE on Windows and never falls back to HOME, so every test that redirects a home dir was quietly reading the real profile: TestLoadGlobalRule, TestShellRCFiles, TestTryShellRC and the session writer-creation test now set both. So do the retry e2e helper and TestLoadLLMRuntime_BadAppConfig, where it had gone past reading the wrong profile to failing outright. The e2e test blocks session persistence by occupying $HOME/.opencodereview/ sessions with a regular file, and on Windows found the runner's real directory already sitting there, so the setup write died with "is a directory"; the config test wrote its invalid config.json into a temp home nothing read, so resolution reported a missing endpoint instead of the parse failure the test is named for. unwritableConfigPath put the config below a regular-file parent, which Windows reports as ERROR_PATH_NOT_FOUND; os.IsNotExist accepts that, so loadOrCreateConfig read it as "no config yet" and the six save-failure tests never reached the rollback they are named for. It now points at a directory, which fails both the write and the reload on every platform, so those six keep their coverage rather than taking a skip. Two do get one, the mechanism being absent rather than different: the chmod(0000) sniff error in internal/scan, and ReadDir on a regular file, which comes back as an empty listing on Windows instead of ENOTDIR. captureStdout and captureStderr -- and the two helpers shaped like them in the delegate and config tests -- drained their pipe only after the captured function returned, so that function could write one pipe buffer and then blocked forever. That is what hung TestReviewE2E_RecoveredAndFailedReachesJSONExit for the package's entire 10m budget. Linux only hid it: 1MiB through the old helper deadlocks there too. They now drain concurrently, which fixes the bug instead of skipping the test. Docs (en/zh/ja) spell out the failure modes, the 60s budget including the time spent answering a prompt, the inherited stdin/stderr, the extra 5s a daemon holding the pipe costs, and that config.json is trusted input because the value is executed as a shell command. Review follow-ups in the same pass. A whitespace-only api_key_cmd was the one credential field this path had not normalized: it is empty to `sh` but non-empty to Go, so it suppressed the env-var fallback and then failed with "produced empty output". It now reads as unset, the same as the equivalent typo in api_key. Same for auth_token_cmd on the legacy block. The wizard checked those same fields for emptiness without the trim, so `ocr config provider` would accept a command of " ", save a config with no static key, and leave the resolver to refuse it with "no api_key or api_key_cmd configured". Both gates read through apiKeyCmdForStep and manualAuthTokenCmd, so the trim goes in those two accessors and covers the render sites with them; applyOfficialProviderConfig reads the entry directly and gets its own. The TUI never showed that a command already satisfies the credential step, so the API-key field looked unconfigured on a provider that resolves fine; it now says so on both the provider tabs and the Manual tab. The hint names the config key rather than echoing the command. Usually the command is a bare reference to a secret manager, but nothing stops a user inlining a credential into it (`VAULT_TOKEN=hvs.xxx vault kv get ...`), and this wizard masks every other secret it puts on screen -- one user-authored string printed verbatim into screenshots and terminal recordings was the hole in that. There is exactly one command per provider, so the key name is enough to identify which one is configured. Left as it is, deliberately: SysProcAttr.Setpgid would let us SIGKILL the whole process group and so reap a grandchild the command backgrounded, which `sleep 200 & printf tok` does leak today. It would also put the child outside the terminal's foreground process group, where it takes SIGTTIN the moment it reads the tty -- measured, a child running `read -r x 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 f1fbca7e..b365c894 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 a7a6e7fb..2acc0286 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 { + // 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)) + } if p.EnvVar != "" { - return fmt.Sprintf("API key is required (or set $%s)", p.EnvVar) + alternatives = append(alternatives, fmt.Sprintf("set $%s", p.EnvVar)) + } + if len(alternatives) == 0 { + return "API key is required" } - 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 { @@ -1617,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 } @@ -1919,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 } @@ -2224,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") } @@ -2326,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 != "" { @@ -2404,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 5afeaf65..0aedec8d 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 3227af9e..6b2ca982 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -208,7 +208,11 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { 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) @@ -237,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) { @@ -248,12 +268,17 @@ 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") + } } -// TestCloneProviderEntry_CopiesEveryField fails when a field is added to -// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and -// ExtraHeaders were once silently dropped. DeepEqual catches a dropped field; -// the reflect sweep is what stops a zero-valued fixture from hiding one. +// 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", @@ -1861,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, }, - } - 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) - } -} - -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"}, + { + name: "official typed key", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + typedKey: "sk-typed", + wantOK: true, }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_CustomEmpty(t *testing.T) { - cfg := &Config{ - Provider: "stepfun", - CustomProviders: map[string]ProviderEntry{ - "stepfun": {APIKey: ""}, + { + 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 = tabCustom - m.customIdx = 0 - m.step = stepAPIKey - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required" { - 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_MaskedSavedKey(t *testing.T) { - cfg := &Config{ - Provider: "deepseek", - Providers: map[string]ProviderEntry{ - "deepseek": {APIKey: "keep-me"}, +// 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 - m.loadExistingAPIKey() - 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) + } + + result, _ := m.handleManualFormEnter() + m2 := result.(providerTUIModel) + + 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 0a32e5b8..cc924010 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 f84cf6bf..5e108b89 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 0713f821..7af11f59 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 6f88434f..028dde91 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 34e95608..7d70fb88 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1259,7 +1259,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{ @@ -1647,9 +1650,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) @@ -1658,7 +1670,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 { @@ -1671,7 +1683,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) @@ -1686,7 +1698,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 index f7b8e049..306cf2f4 100644 --- a/internal/llm/keycmd.go +++ b/internal/llm/keycmd.go @@ -4,9 +4,12 @@ package llm import ( + "bytes" "context" + "errors" "fmt" "os" + "os/exec" "strings" "time" ) @@ -15,36 +18,113 @@ import ( // 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. 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. +// (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 - out, err := c.Output() + 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 { - return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + // 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) } - if err != nil { + // 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). + // 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. - trimmed := strings.TrimRight(string(out), "\r\n") - if strings.Contains(trimmed, "\n") { - return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + // 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) diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go index 6991a0af..19139df8 100644 --- a/internal/llm/keycmd_test.go +++ b/internal/llm/keycmd_test.go @@ -6,6 +6,7 @@ package llm import ( + "os" "strings" "testing" "time" @@ -21,16 +22,35 @@ func TestResolveKeyCmd(t *testing.T) { {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 { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") @@ -54,11 +74,14 @@ func TestResolveKeyCmd(t *testing.T) { } func TestResolveKeyCmd_Timeout(t *testing.T) { - orig := keyCmdTimeout + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay keyCmdTimeout = 50 * time.Millisecond - t.Cleanup(func() { keyCmdTimeout = orig }) + // `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", "api_key_cmd for provider \"x\"") + _, err := resolveKeyCmd("sleep 5 2>/dev/null", "api_key_cmd for provider \"x\"") if err == nil { t.Fatal("expected timeout error, got nil") } @@ -67,6 +90,71 @@ func TestResolveKeyCmd_Timeout(t *testing.T) { } } +// 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") { diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go index 9832ca9c..6051f2c9 100644 --- a/internal/llm/keycmd_unix.go +++ b/internal/llm/keycmd_unix.go @@ -10,6 +10,25 @@ import ( "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 9d4b97b3..85b08342 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) { - if ep.Source == "" { - ep.Source = source - } - ep.Model = stripModelSuffix(ep.Model) - envTimeout, ok, err := parseTimeoutEnv() +// 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 ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", source, err) - } - if ok { - ep.Timeout = envTimeout + return envOverrides{}, err } if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { - envHeaders, err := ParseExtraHeaders(raw) + env.headers, err = ParseExtraHeaders(raw) if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", source, err) + 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) + if env.hasTimeout { + ep.Timeout = env.timeout + } + 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. @@ -319,24 +351,46 @@ 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 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 entry.APIKeyCmd != "" { - fmt.Fprintf(os.Stderr, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) - } - case entry.APIKeyCmd != "": - resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) - if err != nil { - return ResolvedEndpoint{}, false, err - } - apiKey = resolved - case isPreset && preset.EnvVar != "": - apiKey = os.Getenv(preset.EnvVar) - } - if apiKey == "" { + 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 + } + } + // 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) } @@ -443,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, @@ -467,23 +533,26 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, // 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 cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { + 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 } - switch { - case token != "": - // Static auth_token always wins; warn if a command is also set. - if cfg.Llm.AuthTokenCmd != "" { - fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") - } - case cfg.Llm.AuthTokenCmd != "": - // Otherwise-complete legacy block with a set-but-failing command is a hard error. - resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") - if err != nil { - return ResolvedEndpoint{}, false, err - } - token = resolved + // 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. @@ -528,6 +597,18 @@ 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: token, diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go index fef665c6..f5b742be 100644 --- a/internal/llm/resolver_keycmd_test.go +++ b/internal/llm/resolver_keycmd_test.go @@ -1,10 +1,16 @@ +//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" @@ -42,13 +48,52 @@ func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { } } -// (b) static api_key wins even when api_key_cmd is also set. +// (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: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + "anthropic": { + APIKey: "sk-static", + APIKeyCmd: "touch " + marker + "; printf 'sk-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, }, }) ep, err := ResolveEndpoint(cfgPath) @@ -58,6 +103,207 @@ func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { 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). @@ -101,6 +347,33 @@ func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { } } +// (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) @@ -120,6 +393,31 @@ func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { } } +// (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) { diff --git a/internal/llm/resolver_shellrc_test.go b/internal/llm/resolver_shellrc_test.go index e2a084d2..cd0fd597 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 b5c8c3b1..bf07bb0e 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) { @@ -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 7d9ee362..81b1b7fc 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 5ba6cac0..c0772003 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 604518d9..b15f67f0 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 b56d892d..40a46961 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 bc0dc04c..94d4fab2 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 865d2af9..5827c0a6 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -148,6 +148,7 @@ 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 @@ -177,10 +178,24 @@ 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, or multi-line output is a hard error (OCR never silently falls -back). It must complete within 60 seconds. The command's stderr is passed -through to your terminal, so interactive prompts (pinentry, Touch ID) still -work. +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 diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 39743790..15747704 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -146,6 +146,7 @@ Ollama は API key を無視しますが、カスタム provider は空でない } } ``` + ### API key をコマンドで取得する key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット @@ -175,10 +176,24 @@ ocr config set providers.anthropic.api_key_cmd \ 設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 -空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする -ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの -stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も -引き続き動作します。 +空の出力、複数行の出力、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` +で書き込みます）。 ### 追加のリトライ対象ステータスコード diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index 7969f7ee..fa407160 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -138,6 +138,7 @@ provider 没有环境变量回退），所以设任意占位值即可。模型 } } ``` + ### 通过命令获取 API key 除了把 key 直接写进配置文件，还可以用 `api_key_cmd` 在运行时从密钥管理器 @@ -164,9 +165,21 @@ ocr config set providers.anthropic.api_key_cmd \ 优先级：静态 `api_key` 始终优先（两者都设置时忽略命令并打印警告）；否则运行 `api_key_cmd`；只有两者都未设置时，OCR 才回退到 provider 对应的环境变量。 -命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 -被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 -到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 +命令在每次 `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` 权限）。 ### 额外的重试状态码 From 34466685f7ce753093c622f90ba8872f4e622260 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Mon, 17 Aug 2026 07:46:16 +0200 Subject: [PATCH 3/4] fix(config): drop duplicated license header in testconnection The SPDX and copyright block was emitted twice at the top of internal/config/testconnection/testconnection.go, a rebase artifact from the first commit on this branch rather than an intentional change. The file is now byte-identical to main. make license-check passed throughout: it verifies a valid header is present, not that there is only one. --- internal/config/testconnection/testconnection.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/config/testconnection/testconnection.go b/internal/config/testconnection/testconnection.go index 45b1a892..d6867d8a 100644 --- a/internal/config/testconnection/testconnection.go +++ b/internal/config/testconnection/testconnection.go @@ -1,9 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 alibaba/open-code-review Contributors -// SPDX-License-Identifier: Apache-2.0 -// Copyright 2026 alibaba/open-code-review Contributors - // Package testconnection loads the LLM test connection task configuration. package testconnection From 29018b2b84fb046202e3e330e9448fe968061f71 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Mon, 17 Aug 2026 08:19:37 +0200 Subject: [PATCH 4/4] docs(i18n): sync api_key_cmd configuration docs to ru The en, ja and zh pages gained the "API key from a command" section; ru was left behind. Adds the same section, in the same position, with the config keys and shell snippets untranslated as the rest of the file does. --- pages/src/content/docs/ru/configuration.md | 54 ++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/pages/src/content/docs/ru/configuration.md b/pages/src/content/docs/ru/configuration.md index 39f0925a..4ff62cf4 100644 --- a/pages/src/content/docs/ru/configuration.md +++ b/pages/src/content/docs/ru/configuration.md @@ -156,6 +156,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 для временных