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

[chethanuk]

Description

Adds 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 in plaintext in config.json — the same pattern as git credential.helper and AWS credential_process.

ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key"

Today the only options are a plaintext api_key in config.json or a preset environment variable. Neither keeps the secret off disk on a shared or backed-up machine.

Precedence

One resolution site, presets and custom providers alike:

static api_key  → wins (stderr warning if a command is also set)
api_key_cmd     → runs; any failure is a hard error
preset env var  → only when neither of the above is set
                → error

A failing command is never a silent fallback to the environment variable. That is the point: a helper that fails because a vault is locked must not quietly send a stale $ANTHROPIC_API_KEY and produce a review billed to the wrong account. auth_token_cmd mirrors this, with one difference — an incomplete legacy block (no url, no model) falls through to later strategies without running the command, so an unused legacy stanza cannot trigger a credential prompt.

The resolved value is used in memory only: never written back to config, never logged. It resolves once per process, and only after the cheap config validation has passed, so ocr review --model nonexistent fails on the model name instead of first prompting for Touch ID.

Backward compatibility

Both keys are new and omitempty, so an existing config.json round-trips byte-identically. Every added branch is guarded on a non-empty command string, so behavior is unchanged for anyone who does not set them. ocr config set accepts both and does not mask them on read-back — a command line is not itself a secret.

Since the value is executed as a shell command, config.json becomes trusted input. Documented alongside the 0600 mode OCR already writes it with.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

Hardening found while validating

Each of these was a way the feature did not actually work end to end:

• The 60s timeout was not a bound. It killed the shell, but a helper leaving a background process on the inherited stdout pipe (gpg-agent, pinentry, a first-use op daemon) kept Cmd.Wait blocked on the read. api_key_cmd = "sleep 200 & printf tok" hung for over 90 seconds. Fixed by buffering stdout through a writer os/exec copies in its own goroutine and setting WaitDelay, which is what lets Wait force the pipe closed.
• stdin was /dev/null, so a helper needing a passphrase saw EOF, or refused to prompt at all for lack of a tty. Now inherits 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. Now rejected up front with the offending byte and offset, matching httpguts.ValidHeaderFieldValue. A lone interior CR survived both TrimRight and TrimSpace, so it is caught as multi-line output.
• A whitespace-only credential out-ranked a working command and sent Authorization: Bearer , unrecoverable without hand-editing the config. Whitespace-only now normalizes to unset for the static key, the env var and the Manual TUI tab's token.
• ocr config provider rejected api_key_cmd-only providers in both directions — non-interactively applyOfficialProviderConfig demanded a static key or 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 errors name the option that would fix them.

Separately, cloneProviderEntry was copying fields by hand and had never been updated for TimeoutSec and ExtraHeaders, so editing any provider through ocr config provider silently erased both — a timeout_sec of 900 reverted to the 300s default and custom extra_headers disappeared. Pre-existing and unrelated to this feature, so it is its own commit, and its regression test walks the struct with reflection so the next added field cannot be dropped silently.

Windows

The command line goes to cmd.exe through SysProcAttr.CmdLine with /S, not through Args. os/exec quotes Args with syscall.EscapeArg, which targets CommandLineToArgvW; exec.Command's own documentation names cmd.exe as an exception with a different unquoting algorithm and says to supply the full command line yourself. Without this, op read "op://Private/My Vault/api-key" arrives as a single literal filename.

A command string is not portable between the two arms — %VAR% and ^ are cmd.exe metacharacters, $VAR and \ escaping do not apply — so an sh-authored api_key_cmd generally needs a Windows-specific rewrite. Documented.

Why this adds a windows-latest job

I would rather not ship a Windows code path that no test has ever executed. The existing cross-compile job runs go build -o /dev/null ./... under GOOS=windows, which proves the arm compiles and nothing more — before this PR, keycmd_windows.go had zero executed coverage on any platform, and the five Windows-only assertions in encodeRepoPath had never run either.

That gap was not theoretical. On its first green run the job found a pre-existing bug: TestResolveBackgroundFilePath/absolute_unchanged was passing filepath.FromSlash("/etc/context.md"), which is rooted but not absolute on Windows (filepath.IsAbs wants a volume), so the subtest named "absolute" had been silently exercising the relative branch. It also confirmed every cmd.exe row in the new test table passes on a real runner rather than on my reasoning about cmd /?.

Implementation notes:

• Installs Go via setup-go rather than reusing the shared golang:1.26.5 image, because GitHub does not support container: on Windows runners — the runner errors with "container operations are only supported on Linux runners" (actions/runner#904, #1402; the PR that attempted it, #1801, was never merged). Windows containers also cannot run on an ubuntu host, so there is no way to matrix this in from the Linux job.
• No -race: the detector needs a C toolchain on Windows, and races are OS-independent, so the Linux job already covers them.
• No coverage gate: the //go:build !windows test files legitimately put the total under the 80% the Linux job enforces.

Six existing tests needed a guard, none of them a behavior change: three assert an unreadable path is skipped, but Chmod(0000) on Windows only sets the read-only bit — and their existing os.Getuid() == 0 guard cannot cover that, since Getuid returns -1 there and never 0; TestSaveConfig asserts the 0600 the config is written with, which Windows reports as 0666; the symlink-safety test needs SeCreateSymbolicLinkPrivilege, which an unelevated CI account lacks (six sibling symlink tests already skip for this); and the background-path case above.

One thing to flag for the maintainers: every other job in this repo is runs-on: self-hosted, and this is the first GitHub-hosted one. If GitHub-hosted runners are disabled or unbilled for the org, the job will queue rather than run. I have verified it green end to end on a fork (run log), so if you would prefer it on a self-hosted Windows runner, dropped, or split into its own PR, say which and I will adjust — the feature commits do not depend on it.

How Has This Been Tested?

• make test passes locally
• Manual testing (below)

Table-driven runner matrix on both arms: success, whitespace and CRLF trimming, no trailing newline, non-zero exit, command-not-found, empty, whitespace-only, multi-line, interior CR, NUL/VT/FF/DEL control bytes, interior TAB preserved, output exactly at the 64KiB cap and one byte over, timeout, WaitDelay bounding an orphan holding the pipe, and inherited stdin. Plus resolver precedence rows, legacy fall-through, "runs exactly once per process", and "an invalid model never runs the command".

Verified end to end against a local fake LLM server, including reproducing the >90s hang that motivated the WaitDelay fix and the prompt-before-validation ordering bug.

gofmt -s -l clean · go vet ./... clean · go test -race -count=1 ./... green · coverage 81.8% (gate 80%) · govulncheck clean · GOOS=windows go vet ./... clean · full suite green on windows-latest. Each of the three commits independently passes build, vet and test.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Limitations

• The VS Code extension's config parser does not read the new keys, so an api_key_cmd-only config still reads as unconfigured there. Out of scope here; happy to file it separately.
• No caching — the command runs once per ocr invocation.

Related Issues

Closes #236

[github-actions]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[github-actions]

[security · high]
Command injection via embedded double quotes. The cmd string is interpolated directly into the cmd.exe command line with only outer double-quote wrapping. If cmd contains a " character (e.g., from a config value like op read "op://Private/My Vault/api-key"), it closes the outer quote early and allows everything after it to be interpreted as separate cmd.exe tokens. Combined with cmd.exe metacharacters like &, |, or ^, this enables unintended command execution.

For example, a cmd value of foo" & whoami & " would produce:

cmd.exe /S /C "foo" & whoami & ""

executing whoami outside the intended command.

Consider either:

1. Rejecting or escaping embedded " characters in cmd before interpolation (e.g., replacing " with \" or "" depending on cmd.exe's escape rules under /S /C).
2. Documenting explicitly that api_key_cmd values must not contain double quotes on Windows and validating this constraint.
3. Using an alternative approach such as writing the command to a temporary .cmd file and executing that, avoiding inline interpolation entirely.

[chethanuk]

@lizhengfeng101 ready for review when you have a moment 🙏