feat(config): resolve api_key/auth_token from a command (#236) - #605
Merged
Conversation
Contributor
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
chethanuk
force-pushed
the
feat/api-key-cmd
branch
3 times, most recently
from
July 30, 2026 17:12
57194c3 to
fe68c63
Compare
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).
chethanuk
force-pushed
the
feat/api-key-cmd
branch
2 times, most recently
from
August 16, 2026 20:07
2772b40 to
8871236
Compare
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 </dev/tty` answers in 7ms as written and returns nothing at all under Setpgid. That read is what pinentry and `op`'s fallback prompt do, which is the case c.Stdin = os.Stdin exists to support and the docs promise. The group has to be chosen at Start, so this cannot be narrowed to the timeout path, and reaping the grandchild properly needs tcsetpgrp-style job control. A process the user's own command asked to background, outliving a CLI that exits seconds later exactly as it would from their shell, is not worth a broken credential prompt. keycmd_unix.go records the measurement so the trade is not re-litigated. The static-key-wins tests asserted only on the resolved token, which would have held just as well if the command ran and its output were discarded — i.e. a spurious biometric prompt on every review of a config that keeps a command as a fallback. They now use a filesystem witness to assert non-execution. The docs note that a command written for `sh` is generally not portable to `cmd.exe`, since the Windows arm is where that bites.
chethanuk
force-pushed
the
feat/api-key-cmd
branch
from
August 16, 2026 20:16
8871236 to
1bf219a
Compare
Collaborator
|
Duplicate SPDX license header in The file currently has the SPDX/copyright header block duplicated at the top: // 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 testconnectionThis looks like a rebase/merge artifact rather than an intentional change — the file only needs one header. Please remove the duplicate block before merging. |
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.
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.
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds
api_key_cmd(provider entries) andauth_token_cmd(legacyllmblock) so the LLM credential can be fetched from a secret manager at review time instead of stored in plaintext inconfig.json— the same pattern asgit credential.helperand AWScredential_process.Today the only options are a plaintext
api_keyinconfig.jsonor a preset environment variable. Neither keeps the secret off disk on a shared or backed-up machine.Why a command rather than a keyring library
#236 proposes an
ocr auth key save/list/deletesurface over a cross-platform keyring binding, with an encrypted-file fallback for headless Linux. A command hits the same acceptance criteria without OCR owning any of that: the OS keyring is already reachable through the tool each platform ships with, so there is no binding to vendor, no fallback to implement, and no keyring-locked/denied/unavailable error taxonomy to maintain — the helper reports its own.It also covers the managers a keyring binding would not: 1Password,
pass,gopass, Vault. Config stores a command, never the secret; the value resolves at runtime; environment variables keep working for CI. Both are documented (en/zh/ja).What this does not add is the
ocr auth key savehalf — writing the credential into the keyring. That stays with the platform's own tooling (security add-generic-password,secret-tool store). Say the word if you would rather have the wrapper commands too and I will follow up; the resolution path here does not change either way.Precedence
One resolution site, presets and custom providers alike:
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_KEYand produce a review billed to the wrong account.auth_token_cmdmirrors this, with one difference — an incomplete legacy block (nourl, nomodel) 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 nonexistentfails on the model name instead of first prompting for Touch ID.Backward compatibility
Both keys are new and
omitempty, so an existingconfig.jsonround-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 setaccepts 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.jsonbecomes trusted input. Documented alongside the0600mode OCR already writes it with.Type of Change
Hardening found while validating
Each of these was a way the feature did not actually work end to end:
gpg-agent,pinentry, a first-useopdaemon) keptCmd.Waitblocked on the read.api_key_cmd = "sleep 200 & printf tok"hung for over 90 seconds. Fixed by buffering stdout through a writeros/execcopies in its own goroutine and settingWaitDelay, which is what letsWaitforce the pipe closed./dev/null, so a helper needing a passphrase saw EOF, or refused to prompt at all for lack of a tty. Now inheritsos.Stdin, which is safe because no path resolves an endpoint while the bubbletea TUI is reading stdin.cat /dev/urandomgrew the heap without limit. Capped at 64KiB, refusing the write so the child dies ofSIGPIPE.Authorizationheader, wherenet/httprejects them as an opaqueinvalid header field value. Now rejected up front with the offending byte and offset, matchinghttpguts.ValidHeaderFieldValue. A lone interior CR survived bothTrimRightandTrimSpace, so it is caught as multi-line output.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 providerrejectedapi_key_cmd-only providers in both directions — non-interactivelyapplyOfficialProviderConfigdemanded 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.cloneProviderEntryalso has to carry the new field, since it copiesProviderEntrymember by member and would otherwise eraseapi_key_cmdwhenever a provider is edited throughocr config provider. That is the same shape as thetimeout_sec/extra_headersdrop fixed upstream in the meantime; the regression test here walks the struct with reflection, so the next added field cannot be dropped silently either.Windows
The command line goes to
cmd.exethroughSysProcAttr.CmdLinewith/S, not throughArgs.os/execquotesArgswithsyscall.EscapeArg, which targetsCommandLineToArgvW;exec.Command's own documentation namescmd.exeas 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^arecmd.exemetacharacters,$VARand\escaping do not apply — so an sh-authoredapi_key_cmdgenerally needs a Windows-specific rewrite. Documented.Why this adds a
windows-latestjobI would rather not ship a Windows code path that no test has ever executed. The existing
cross-compilejob runsgo build -o /dev/null ./...underGOOS=windows, which proves the arm compiles and nothing more — before this PR,keycmd_windows.gohad zero executed coverage on any platform, and the five Windows-only assertions inencodeRepoPathhad never run either.That gap was not theoretical. On its first green run the job found a pre-existing bug:
TestResolveBackgroundFilePath/absolute_unchangedwas passingfilepath.FromSlash("/etc/context.md"), which is rooted but not absolute on Windows (filepath.IsAbswants a volume), so the subtest named "absolute" had been silently exercising the relative branch. It also confirmed everycmd.exerow in the new test table passes on a real runner rather than on my reasoning aboutcmd /?.Implementation notes:
setup-gorather than reusing the sharedgolang:1.26.5image, because GitHub does not supportcontainer: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 anubuntuhost, so there is no way to matrix this in from the Linux job.-race: the detector needs a C toolchain on Windows, and races are OS-independent, so the Linux job already covers them.//go:build !windowstest 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 existingos.Getuid() == 0guard cannot cover that, sinceGetuidreturns-1there and never0;TestSaveConfigasserts the0600the config is written with, which Windows reports as0666; the symlink-safety test needsSeCreateSymbolicLinkPrivilege, 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 commit does not depend on it.How Has This Been Tested?
make testpasses locallyTable-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,
WaitDelaybounding 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
WaitDelayfix and the prompt-before-validation ordering bug.Rebased onto current
main.gofmt -s -lclean ·go vet ./...clean ·go test -race -count=1 ./...green · coverage 91.3% (gate 80%) ·make checkclean ·GOOS=windows go vet ./...clean. Both commits independently pass build, vet and test.Checklist
go fmt,go vet)Limitations
api_key_cmd-only config still reads as unconfigured there. Out of scope here; happy to file it separately.ocrinvocation.Related Issues
Closes #236