feat(config): resolve api_key/auth_token from a command (#236) - #13
feat(config): resolve api_key/auth_token from a command (#236)#13chethanuk wants to merge 4 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds command-based retrieval for provider API keys and legacy LLM auth tokens, including configuration fields, platform-specific execution, timeout and output validation, resolution precedence, tests, cloning support, cross-platform CI, review-rule coverage, diff parsing, and multilingual documentation. ChangesCommand-based credential resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/llm/keycmd_unix.go`:
- Around line 10-11: Update newKeyCmd in the Unix implementation to create the
shell in its own process group using SysProcAttr.Setpgid, then ensure context
cancellation terminates that entire process group rather than only the shell.
Preserve the existing sh -c invocation and credential-command timeout behavior.
In `@internal/llm/resolver_keycmd_test.go`:
- Around line 66-69: Update captureStderr to close the read end r after os.Pipe
succeeds, using deferred cleanup so every invocation releases the file
descriptor while preserving the existing pipe and stderr-capture behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4512d2c-7a62-4360-87bf-fdfd8c6a3275
📒 Files selected for processing (4)
internal/llm/keycmd_test.gointernal/llm/keycmd_unix.gointernal/llm/keycmd_windows.gointernal/llm/resolver_keycmd_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/llm/keycmd_windows.go
- internal/llm/keycmd_test.go
| // 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. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the process execution configuration for Setpgid usage
ast-grep outline internal/llm/keycmd_unix.go
cat internal/llm/keycmd_unix.goRepository: chethanuk/open-code-review
Length of output: 595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for Setpgid / process-group handling =="
rg -n "Setpgid|SysProcAttr|Setctty|Credential.*cmd|newKeyCmd|CommandContext\\(" internal . || true
echo
echo "== behavior probe: does killing /bin/sh terminate its child? =="
python3 - <<'PY'
import os, signal, subprocess, time, sys
# Spawn a shell that launches a long-lived child and then waits.
# The shell itself should exit when terminated, but the child should keep running
# if it does not receive a forwarded signal.
p = subprocess.Popen(
["sh", "-c", "sleep 60"],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
time.sleep(0.2)
pid = p.pid
# Find child PID via /proc, if available.
child = None
proc_status = f"/proc/{pid}/task/{pid}/children"
try:
with open(proc_status) as f:
data = f.read().strip()
if data:
child = int(data.split()[0])
except Exception as e:
print(f"could not read child pid from {proc_status}: {e}")
os.kill(pid, signal.SIGKILL)
try:
p.wait(timeout=2)
except subprocess.TimeoutExpired:
print("shell did not exit after SIGKILL")
sys.exit(1)
print(f"shell exited with code {p.returncode}")
if child is not None:
alive = os.path.exists(f"/proc/{child}")
print(f"child pid={child} alive_after_shell_kill={alive}")
PYRepository: chethanuk/open-code-review
Length of output: 4150
Kill the credential command’s process group on Unix exec.CommandContext(ctx, "sh", "-c", cmd) only stops the shell; the credential command can keep running after cancellation. Set SysProcAttr.Setpgid here and terminate the whole group on cancel.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llm/keycmd_unix.go` around lines 10 - 11, Update newKeyCmd in the
Unix implementation to create the shell in its own process group using
SysProcAttr.Setpgid, then ensure context cancellation terminates that entire
process group rather than only the shell. Preserve the existing sh -c invocation
and credential-command timeout behavior.
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("os.Pipe: %v", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the read end of the pipe to prevent file descriptor leaks.
The read end of the pipe (r) is never closed, which leaks a file descriptor each time captureStderr is called. While tests are short-lived, it is good practice to clean up resources to prevent file descriptor exhaustion in larger test suites.
🔧 Proposed fix
r, w, err := os.Pipe()
if err != nil {
t.Fatalf("os.Pipe: %v", err)
}
+ defer r.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("os.Pipe: %v", err) | |
| } | |
| r, w, err := os.Pipe() | |
| if err != nil { | |
| t.Fatalf("os.Pipe: %v", err) | |
| } | |
| defer r.Close() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llm/resolver_keycmd_test.go` around lines 66 - 69, Update
captureStderr to close the read end r after os.Pipe succeeds, using deferred
cleanup so every invocation releases the file descriptor while preserving the
existing pipe and stderr-capture behavior.
6cea509 to
7971ab4
Compare
chethanuk
changed the title
7971ab4 to
ae88821
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/opencodereview/flags.go (1)
317-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
*_cmdexample toocr confighelp.The new keys appear in the "Supported keys"/"Provider fields" lists but nowhere in the Examples block, which is where users copy from. A one-liner next to the existing api_key/auth_token examples would surface the feature.
📝 Suggested help-text addition
# Set API key via environment variable (recommended) or config: # export ANTHROPIC_API_KEY=sk-ant-xxx ocr config set providers.anthropic.api_key "$ANTHROPIC_API_KEY" + # Or fetch it at review time from a secret manager (nothing stored in config): + ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key"Also applies to: 345-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/opencodereview/flags.go` around lines 317 - 320, Update the `ocr config` help Examples block in `flags.go` to include a one-line `providers.anthropic.api_key` command alongside the existing `api_key` and `auth_token` examples, showing the environment-variable usage already demonstrated in the diff..github/workflows/ci.yml (1)
93-136: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNew Windows job's checkout doesn't set
persist-credentials: false.Static analysis flags this on the new
actions/checkout@v7step (line 97). It mirrors the existing pattern already used by thetestandcross-compilejobs in this file, so it's not a new regression introduced by this diff, but worth tightening across all three jobs at some point since the job only builds/tests and doesn't need the persisted git credential.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 93 - 136, The Windows job’s checkout step should disable persisted Git credentials. Update the checkout action in the windows job to set persist-credentials to false, matching the existing test and cross-compile job pattern.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/opencodereview/flags_test.go`:
- Around line 251-280: The TestPrintConfigUsage_ListsMatchSetConfigValueError
test only compares the two lists and does not verify the newly supported fields
exist. Add explicit assertions that api_key_cmd and llm.auth_token_cmd appear in
their appropriate sections of both captured usage and canonical error output,
while preserving the existing synchronization checks.
In `@internal/llm/keycmd_test.go`:
- Line 131: Handle the ignored read-pipe close errors in both
TestResolveKeyCmd_StdinWired variants: update internal/llm/keycmd_test.go lines
131-131 and internal/llm/keycmd_windows_test.go lines 110-110 to defer a
function that explicitly discards r.Close()’s error instead of deferring
r.Close() directly.
In `@pages/src/content/docs/en/configuration.md`:
- Around line 130-141: Update the command-credential documentation in
pages/src/content/docs/en/configuration.md lines 130-141 to add exact examples
for custom_providers.<name>.api_key_cmd and the legacy llm.auth_token_cmd,
including their precedence relative to static credentials and environment
variables. Make the equivalent additions in
pages/src/content/docs/zh/configuration.md lines 122-131, covering both
configuration paths and the same precedence guidance.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 93-136: The Windows job’s checkout step should disable persisted
Git credentials. Update the checkout action in the windows job to set
persist-credentials to false, matching the existing test and cross-compile job
pattern.
In `@cmd/opencodereview/flags.go`:
- Around line 317-320: Update the `ocr config` help Examples block in `flags.go`
to include a one-line `providers.anthropic.api_key` command alongside the
existing `api_key` and `auth_token` examples, showing the environment-variable
usage already demonstrated in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b434c4a9-fc52-4516-a904-ee89f9ad86a1
📒 Files selected for processing (25)
.github/workflows/ci.ymlcmd/opencodereview/background_file_test.gocmd/opencodereview/config_cmd.gocmd/opencodereview/config_cmd_test.gocmd/opencodereview/flags.gocmd/opencodereview/flags_test.gocmd/opencodereview/provider_cmd.gocmd/opencodereview/provider_cmd_test.gocmd/opencodereview/provider_tui.gocmd/opencodereview/provider_tui_funcs_test.gocmd/opencodereview/provider_tui_test.gointernal/config/rules/system_rules_test.gointernal/llm/keycmd.gointernal/llm/keycmd_test.gointernal/llm/keycmd_unix.gointernal/llm/keycmd_windows.gointernal/llm/keycmd_windows_test.gointernal/llm/resolver.gointernal/llm/resolver_keycmd_test.gointernal/llm/resolver_test.gointernal/viewer/handler_test.gointernal/viewer/store_load_test.gopages/src/content/docs/en/configuration.mdpages/src/content/docs/ja/configuration.mdpages/src/content/docs/zh/configuration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pages/src/content/docs/ja/configuration.md
| if err != nil { | ||
| t.Fatalf("os.Pipe: %v", err) | ||
| } | ||
| defer r.Close() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unchecked r.Close() in both TestResolveKeyCmd_StdinWired variants. errcheck reports the deferred Close on the read end of the stdin pipe; the same line exists in the Windows-tagged twin, so lint fails on whichever platform is linted.
internal/llm/keycmd_test.go#L131-L131: replacedefer r.Close()withdefer func() { _ = r.Close() }().internal/llm/keycmd_windows_test.go#L110-L110: apply the identical change so the Windows lint job stays clean.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 131-131: Error return value of r.Close is not checked
(errcheck)
📍 Affects 2 files
internal/llm/keycmd_test.go#L131-L131(this comment)internal/llm/keycmd_windows_test.go#L110-L110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llm/keycmd_test.go` at line 131, Handle the ignored read-pipe close
errors in both TestResolveKeyCmd_StdinWired variants: update
internal/llm/keycmd_test.go lines 131-131 and
internal/llm/keycmd_windows_test.go lines 110-110 to defer a function that
explicitly discards r.Close()’s error instead of deferring r.Close() directly.
Source: Linters/SAST tools
13aa10b to
57194c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/llm/resolver_keycmd_test.go (1)
100-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
r(pipe read end) is never closed — file descriptor leak (duplicate of prior finding).
w.Close()is called but the read endrfromos.Pipe()is never closed. Now used by 3 test call sites (captureStderrinvoked in tests b2, e2, b3, e4), each leaking one fd.🧰 Proposed fix
r, w, err := os.Pipe() if err != nil { t.Fatalf("os.Pipe: %v", err) } + defer func() { _ = r.Close() }() orig := os.Stderr🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/resolver_keycmd_test.go` around lines 100 - 120, Close the pipe read end in captureStderr after io.ReadAll completes, including cleanup on read failure, so every os.Pipe descriptor is released while preserving the captured output behavior.internal/llm/keycmd_test.go (1)
131-131: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnchecked
r.Close()in bothTestResolveKeyCmd_StdinWiredvariants (duplicate of a prior consolidated finding).errcheckflags the deferredCloseon the stdin pipe's read end in both the Unix and Windows test files.
internal/llm/keycmd_test.go#L131-L131: replacedefer r.Close()withdefer func() { _ = r.Close() }().internal/llm/keycmd_windows_test.go#L123-L123: apply the identical change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/keycmd_test.go` at line 131, Handle the ignored close errors in both TestResolveKeyCmd_StdinWired variants: update the deferred reader cleanup at internal/llm/keycmd_test.go:131 and internal/llm/keycmd_windows_test.go:123 to explicitly discard r.Close() errors via a deferred closure.Source: Linters/SAST tools
internal/llm/keycmd_unix.go (1)
10-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCredential-command descendants aren't terminated on cancellation, on either platform. Both
newKeyCmdimplementations build the child viaexec.CommandContext, which on timeout/cancellation only signals the direct shell/cmd.exeprocess — a grandchild the credential command backgrounds (e.g.sleep 200 &orstart /b) keeps running.keycmd.go'sWaitDelaybounds how long our process waits on it, but doesn't kill it.
internal/llm/keycmd_unix.go#L10-L14: setSysProcAttr.Setpgid: trueand use a customCancel(orWaitDelayalone won't suffice) tosyscall.Kill(-pid, syscall.SIGKILL)the whole process group on cancellation — low effort, already flagged in a prior review.internal/llm/keycmd_windows.go#L35-L47: associate the process with a Windows Job Object configured for kill-on-close so descendants are terminated with the job; this requiresCreateJobObject/SetInformationJobObject/AssignProcessToJobObjectviagolang.org/x/sys/windows— materially more effort, reasonable to defer givenapi_key_cmdis a trusted, locally-authored command and this only bounds a rare edge case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/keycmd_unix.go` around lines 10 - 14, The Unix newKeyCmd implementation must terminate credential-command descendants on cancellation by creating the shell in its own process group via SysProcAttr.Setpgid and using a custom Cancel handler to kill the entire group with SIGKILL; retain context timeout and cancellation behavior. The Windows implementation at internal/llm/keycmd_windows.go lines 35-47 requires no direct change and may defer Job Object support.
🧹 Nitpick comments (3)
cmd/opencodereview/config_cmd_test.go (1)
1090-1112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
captureConfigStderrreads only afterfnreturns.Fine for these one-line warnings, but any future test emitting >64 KiB to stderr will deadlock on the pipe. A goroutine draining
rconcurrently (or reading beforew.Close()) would make the helper safe for arbitrary output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/opencodereview/config_cmd_test.go` around lines 1090 - 1112, Update captureConfigStderr to drain the read end of the pipe concurrently while fn executes, rather than waiting until fn returns to call io.ReadAll. Coordinate the reader completion before returning and preserve the existing stderr restoration, pipe cleanup, and fatal-error handling..github/workflows/ci.yml (1)
97-97: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon the Windows job's checkout.The checkout step doesn't disable credential persistence, so the git credential store (and the ambient
GITHUB_TOKEN) stays on disk for the rest of the job — which then runsgo test ./...against PR-authored code. Since this PR's own tests demonstrate exec'ing arbitrary shell/cmd.execommands, a malicious test in a future PR could read and exfiltrate the token.🔧 Proposed fix
- uses: actions/checkout@v7 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 97, Update the Windows job’s actions/checkout step to set persist-credentials to false, ensuring checkout does not leave the ambient GITHUB_TOKEN or Git credentials available during the subsequent go test ./... execution.Source: Linters/SAST tools
internal/llm/keycmd_windows.go (1)
35-47: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffDescendant processes aren't bounded on cancellation (Windows counterpart to the Unix gap).
exec.CommandContext(ctx, "cmd.exe")only killscmd.exeitself on timeout; a grandchild the command line spawns (e.g.start /bor&-chained backgrounding) can outlive the timeout.keycmd.go'sWaitDelaybounds how long we wait on it, but doesn't terminate it. A full fix would associate the process with a Windows Job Object (kill-on-close) — materially more work than the Unix-sideSetpgidfix, given the low likelihood of a legitimateapi_key_cmdbackgrounding a subprocess.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/keycmd_windows.go` around lines 35 - 47, Update newKeyCmd to associate the spawned cmd.exe process and its descendants with a Windows Job Object configured to terminate all processes when the job closes, while preserving the existing CommandContext command-line construction and cancellation behavior. Ensure the job handle is closed reliably so descendant processes cannot outlive cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/rules/system_rules_test.go`:
- Around line 1240-1243: Update the symlink creation error handling in the test
to skip only the documented Windows SeCreateSymbolicLinkPrivilege failure; use
the existing platform/error information to identify that case, and fail the test
for all other os.Symlink errors instead of calling t.Skipf unconditionally.
---
Duplicate comments:
In `@internal/llm/keycmd_test.go`:
- Line 131: Handle the ignored close errors in both TestResolveKeyCmd_StdinWired
variants: update the deferred reader cleanup at internal/llm/keycmd_test.go:131
and internal/llm/keycmd_windows_test.go:123 to explicitly discard r.Close()
errors via a deferred closure.
In `@internal/llm/keycmd_unix.go`:
- Around line 10-14: The Unix newKeyCmd implementation must terminate
credential-command descendants on cancellation by creating the shell in its own
process group via SysProcAttr.Setpgid and using a custom Cancel handler to kill
the entire group with SIGKILL; retain context timeout and cancellation behavior.
The Windows implementation at internal/llm/keycmd_windows.go lines 35-47
requires no direct change and may defer Job Object support.
In `@internal/llm/resolver_keycmd_test.go`:
- Around line 100-120: Close the pipe read end in captureStderr after io.ReadAll
completes, including cleanup on read failure, so every os.Pipe descriptor is
released while preserving the captured output behavior.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 97: Update the Windows job’s actions/checkout step to set
persist-credentials to false, ensuring checkout does not leave the ambient
GITHUB_TOKEN or Git credentials available during the subsequent go test ./...
execution.
In `@cmd/opencodereview/config_cmd_test.go`:
- Around line 1090-1112: Update captureConfigStderr to drain the read end of the
pipe concurrently while fn executes, rather than waiting until fn returns to
call io.ReadAll. Coordinate the reader completion before returning and preserve
the existing stderr restoration, pipe cleanup, and fatal-error handling.
In `@internal/llm/keycmd_windows.go`:
- Around line 35-47: Update newKeyCmd to associate the spawned cmd.exe process
and its descendants with a Windows Job Object configured to terminate all
processes when the job closes, while preserving the existing CommandContext
command-line construction and cancellation behavior. Ensure the job handle is
closed reliably so descendant processes cannot outlive cancellation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df5f3dc9-3027-4fdb-8901-db4c327af68d
📒 Files selected for processing (36)
.github/workflows/ci.ymlcmd/opencodereview/background_file_test.gocmd/opencodereview/config_cmd.gocmd/opencodereview/config_cmd_test.gocmd/opencodereview/flags.gocmd/opencodereview/flags_test.gocmd/opencodereview/provider_cmd.gocmd/opencodereview/provider_cmd_test.gocmd/opencodereview/provider_tui.gocmd/opencodereview/provider_tui_funcs_test.gocmd/opencodereview/provider_tui_test.gointernal/config/allowlist/allowed_ext_test.gointernal/config/allowlist/supported_file_types.jsoninternal/config/rules/rule_docs/composer_json.mdinternal/config/rules/rule_docs/php.mdinternal/config/rules/rule_docs/protobuf.mdinternal/config/rules/system_rules.jsoninternal/config/rules/system_rules_test.gointernal/diff/parser.gointernal/diff/parser_test.gointernal/llm/keycmd.gointernal/llm/keycmd_test.gointernal/llm/keycmd_unix.gointernal/llm/keycmd_windows.gointernal/llm/keycmd_windows_test.gointernal/llm/resolver.gointernal/llm/resolver_keycmd_test.gointernal/llm/resolver_test.gointernal/viewer/handler_test.gointernal/viewer/store_load_test.gopages/src/content/docs/en/configuration.mdpages/src/content/docs/en/review-rules.mdpages/src/content/docs/ja/configuration.mdpages/src/content/docs/ja/review-rules.mdpages/src/content/docs/zh/configuration.mdpages/src/content/docs/zh/review-rules.md
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/opencodereview/flags.go
- pages/src/content/docs/ja/configuration.md
- pages/src/content/docs/zh/configuration.md
- pages/src/content/docs/en/configuration.md
| // 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) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fail on unexpected symlink errors instead of skipping them all.
t.Skipf currently hides every os.Symlink failure on every platform. Restrict the skip to the documented Windows privilege case and fail otherwise, so this symlink-safety test cannot silently stop running.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/rules/system_rules_test.go` around lines 1240 - 1243, Update
the symlink creation error handling in the test to skip only the documented
Windows SeCreateSymbolicLinkPrivilege failure; use the existing platform/error
information to identify that case, and fail the test for all other os.Symlink
errors instead of calling t.Skipf unconditionally.
57194c3 to
fe68c63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/llm/resolver_keycmd_test.go (1)
148-171: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
captureStderrstill leaks the pipe's read end.
rfromos.Pipe()is read viaio.ReadAllbut never closed, leaking a file descriptor on every call. Same issue flagged in a prior review pass.🔧 Proposed fix
r, w, err := os.Pipe() if err != nil { t.Fatalf("os.Pipe: %v", err) } + defer r.Close() orig := os.Stderr os.Stderr = w🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/resolver_keycmd_test.go` around lines 148 - 171, Update captureStderr to close the pipe read end r after io.ReadAll completes, including when reading fails, while preserving the existing captured output and error handling.internal/llm/keycmd_test.go (1)
126-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUnchecked
r.Close()error (errcheck).golangci-lint still flags the deferred
r.Close()here; same finding raised in a prior review pass.🔧 Proposed fix
- defer r.Close() + defer func() { _ = r.Close() }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/keycmd_test.go` around lines 126 - 153, Update TestResolveKeyCmd_StdinWired so the deferred r.Close call explicitly handles or discards its error in a way accepted by errcheck, while preserving the existing cleanup behavior and test flow.Source: Linters/SAST tools
internal/llm/keycmd_unix.go (1)
10-14: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOrphaned credential-command descendants keep running after cancellation (still unaddressed).
newKeyCmdonly wrapssh -c cmd;exec.CommandContext's Kill targets the shell process only, so a backgrounded grandchild (e.g.sleep 200 &) survives past the timeout/cancellation — this is exactly whatWaitDelayinkeycmd.goworks around for the wait, but the orphan process itself is never terminated. Same concern raised previously: run the shell in its own process group (SysProcAttr.Setpgid) and kill the group on cancellation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/llm/keycmd_unix.go` around lines 10 - 14, Update newKeyCmd to start the Unix shell in its own process group using the appropriate SysProcAttr configuration, and ensure cancellation terminates the entire process group rather than only the shell. Preserve the existing context, shell, and command invocation while covering backgrounded credential-command descendants.
🧹 Nitpick comments (1)
cmd/opencodereview/provider_tui_funcs_test.go (1)
1840-2033: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood precedence coverage; missing a "clear saved token" case.
These tables thoroughly cover static/
_cmd/env precedence, but none of theTestHandleManualFormEnter_AuthTokenGatecases set a non-emptymanualTokenOriginal(viaAuthToken) and then simulate clearing the field to empty while unmasked. That gap is exactly what let theresult()bug inprovider_tui.go(see review comment on lines 1955-1958) go unnoticed — reverting to the old token silently instead of persisting the clear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/opencodereview/provider_tui_funcs_test.go` around lines 1840 - 2033, Add a case to TestHandleManualFormEnter_AuthTokenGate with a saved AuthToken, then clear the unmasked manual token input and confirm the step. Assert the flow advances and result().apiKey is empty, verifying result() does not restore manualTokenOriginal after the field is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 97: Update the actions/checkout step in the new Windows job to set
persist-credentials to false, leaving the existing checkout behavior and job
steps unchanged.
In `@cmd/opencodereview/provider_tui.go`:
- Around line 1955-1958: Update the manual token handling near manualTokenInput
so the saved manualTokenOriginal is restored only when manualTokenMasked is
true; remove the additional empty-input/original-token condition. Preserve the
trimmed input, including an explicit empty value, when the field is unmasked,
and add a regression test covering clearing an existing saved token through
unmasked empty input.
---
Duplicate comments:
In `@internal/llm/keycmd_test.go`:
- Around line 126-153: Update TestResolveKeyCmd_StdinWired so the deferred
r.Close call explicitly handles or discards its error in a way accepted by
errcheck, while preserving the existing cleanup behavior and test flow.
In `@internal/llm/keycmd_unix.go`:
- Around line 10-14: Update newKeyCmd to start the Unix shell in its own process
group using the appropriate SysProcAttr configuration, and ensure cancellation
terminates the entire process group rather than only the shell. Preserve the
existing context, shell, and command invocation while covering backgrounded
credential-command descendants.
In `@internal/llm/resolver_keycmd_test.go`:
- Around line 148-171: Update captureStderr to close the pipe read end r after
io.ReadAll completes, including when reading fails, while preserving the
existing captured output and error handling.
---
Nitpick comments:
In `@cmd/opencodereview/provider_tui_funcs_test.go`:
- Around line 1840-2033: Add a case to TestHandleManualFormEnter_AuthTokenGate
with a saved AuthToken, then clear the unmasked manual token input and confirm
the step. Assert the flow advances and result().apiKey is empty, verifying
result() does not restore manualTokenOriginal after the field is cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccbae5dc-6310-4e0d-bc6f-d74443d60d1d
📒 Files selected for processing (25)
.github/workflows/ci.ymlcmd/opencodereview/background_file_test.gocmd/opencodereview/config_cmd.gocmd/opencodereview/config_cmd_test.gocmd/opencodereview/flags.gocmd/opencodereview/flags_test.gocmd/opencodereview/provider_cmd.gocmd/opencodereview/provider_cmd_test.gocmd/opencodereview/provider_tui.gocmd/opencodereview/provider_tui_funcs_test.gocmd/opencodereview/provider_tui_test.gointernal/config/rules/system_rules_test.gointernal/llm/keycmd.gointernal/llm/keycmd_test.gointernal/llm/keycmd_unix.gointernal/llm/keycmd_windows.gointernal/llm/keycmd_windows_test.gointernal/llm/resolver.gointernal/llm/resolver_keycmd_test.gointernal/llm/resolver_test.gointernal/viewer/handler_test.gointernal/viewer/store_load_test.gopages/src/content/docs/en/configuration.mdpages/src/content/docs/ja/configuration.mdpages/src/content/docs/zh/configuration.md
🚧 Files skipped from review as they are similar to previous changes (6)
- pages/src/content/docs/en/configuration.md
- cmd/opencodereview/background_file_test.go
- pages/src/content/docs/zh/configuration.md
- pages/src/content/docs/ja/configuration.md
- internal/llm/keycmd_windows_test.go
- cmd/opencodereview/flags.go
| runs-on: windows-latest | ||
| timeout-minutes: 20 | ||
| steps: | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on the new Windows job's checkout.
The default actions/checkout behavior persists the GitHub token in the local git config for the remainder of the job, which is unnecessary here since nothing beyond building/testing the checked-out code runs afterward.
🔧 Proposed fix
- uses: actions/checkout@v7
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v7 | |
| - uses: actions/checkout@v7 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 97-97: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 97, Update the actions/checkout step in the
new Windows job to set persist-credentials to false, leaving the existing
checkout behavior and job steps unchanged.
Source: Linters/SAST tools
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).
fe68c63 to
2772b40
Compare
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
User descriptionDescriptionAdds ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key"Today the only options are a plaintext PrecedenceOne 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 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 Backward compatibilityBoth keys are new and Since the value is executed as a shell command, Type of Change
Hardening found while validatingEach of these was a way the feature did not actually work end to end:
Separately, WindowsThe command line goes to A command string is not portable between the two arms — Why this adds a
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // 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. | ||
| func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { | ||
| return exec.CommandContext(ctx, "sh", "-c", cmd) |
There was a problem hiding this comment.
Suggestion: The context cancellation only kills the intermediate sh process; any command it starts can survive past the timeout because Unix process descendants are not terminated automatically. For example, a helper that launches a long-running child can leave that child running after credential resolution returns, leaking resources and allowing the configured command to continue indefinitely. Start the shell in its own process group and terminate the entire group when the context expires, or otherwise ensure descendants are cleaned up. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Timed-out credential helpers can leave orphan processes.
- ⚠️ Repeated review attempts can accumulate lingering helper processes.
- ⚠️ Secret-manager child processes may continue after resolution fails.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/llm/keycmd_unix.go
**Line:** 16:16
**Comment:**
*Resource Leak: The context cancellation only kills the intermediate `sh` process; any command it starts can survive past the timeout because Unix process descendants are not terminated automatically. For example, a helper that launches a long-running child can leave that child running after credential resolution returns, leaking resources and allowing the configured command to continue indefinitely. Start the shell in its own process group and terminate the entire group when the context expires, or otherwise ensure descendants are cleaned up.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| // Mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): | ||
| // an already-configured api_key_cmd satisfies the requirement, so picking a | ||
| // model for such a provider must not fail and abandon the save. | ||
| if result.apiKey == "" && cfg.Providers[result.provider].APIKeyCmd == "" { |
There was a problem hiding this comment.
Suggestion: This save-time check also treats a whitespace-only APIKeyCmd as configured, whereas tryProviderConfig trims it and ignores it. As a result, saving an empty key can preserve an unusable command and produce a configuration that the resolver rejects or handles differently from the wizard. Trim the configured command before this comparison. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Provider wizard saves unusable credential configuration.
- ❌ Reviews fail later when no environment key exists.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/provider_cmd.go
**Line:** 245:245
**Comment:**
*Api Mismatch: This save-time check also treats a whitespace-only `APIKeyCmd` as configured, whereas `tryProviderConfig` trims it and ignores it. As a result, saving an empty key can preserve an unusable command and produce a configuration that the resolver rejects or handles differently from the wizard. Trim the configured command before this comparison.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if m.apiKeyCmdForStep() != "" { | ||
| return true, "" | ||
| } |
There was a problem hiding this comment.
Suggestion: Whitespace-only commands are accepted here because this checks only != "", while the resolver trims api_key_cmd and treats whitespace-only values as unset. The wizard can therefore confirm a provider that later fails credential resolution or unexpectedly falls back to the environment. Apply the same trimming rule before treating the command as configured. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ TUI credential validation disagrees with runtime resolution.
- ❌ Custom-provider reviews can fail after configuration succeeds.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/provider_tui.go
**Line:** 959:961
**Comment:**
*Api Mismatch: Whitespace-only commands are accepted here because this checks only `!= ""`, while the resolver trims `api_key_cmd` and treats whitespace-only values as unset. The wizard can therefore confirm a provider that later fails credential resolution or unexpectedly falls back to the environment. Apply the same trimming rule before treating the command as configured.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if cmd := m.manualAuthTokenCmd(); cmd != "" { | ||
| s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "llm.auth_token_cmd", cmd)) + "\n") |
There was a problem hiding this comment.
Suggestion: The rendered TUI now writes the complete credential command into the terminal. Commands can contain inline secrets, access tokens, or sensitive vault paths, so this exposes them to screenshots, terminal recordings, and UI capture even though the resolved credential itself is intended to remain undisclosed. Display only a redacted indicator or a non-sensitive fingerprint of the command. [security]
Severity Level: Major ⚠️
- ⚠️ Manual TUI output can expose sensitive credential arguments.
- ⚠️ Terminal recordings may retain command-line secrets.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/provider_tui.go
**Line:** 2282:2283
**Comment:**
*Security: The rendered TUI now writes the complete credential command into the terminal. Commands can contain inline secrets, access tokens, or sensitive vault paths, so this exposes them to screenshots, terminal recordings, and UI capture even though the resolved credential itself is intended to remain undisclosed. Display only a redacted indicator or a non-sensitive fingerprint of the command.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if cmd := m.apiKeyCmdForStep(); cmd != "" { | ||
| s.WriteString("\n") | ||
| s.WriteString(tuiDimStyle.Render(keyCmdConfiguredHintLine(" ", "api_key_cmd", cmd))) | ||
| s.WriteString("\n") |
There was a problem hiding this comment.
Suggestion: The API-key command is also rendered verbatim, exposing any inline credential or sensitive secret-manager arguments through terminal output and recordings. Redact the command contents and show only that a command is configured. [security]
Severity Level: Major ⚠️
- ⚠️ API-key TUI output can expose sensitive command arguments.
- ⚠️ Terminal recordings may retain embedded credentials.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** cmd/opencodereview/provider_tui.go
**Line:** 2389:2392
**Comment:**
*Security: The API-key command is also rendered verbatim, exposing any inline credential or sensitive secret-manager arguments through terminal output and recordings. Redact the command contents and show only that a command is configured.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // shortly after the context dies. | ||
| out := &cappedBuffer{max: keyCmdMaxOutput} | ||
| c.Stdout = out | ||
| c.WaitDelay = keyCmdWaitDelay |
There was a problem hiding this comment.
Suggestion: WaitDelay only closes the inherited stdout pipe after the deadline; it does not terminate grandchildren spawned by sh -c. A command such as sleep 200 & printf tok leaves the background process alive after resolution returns, potentially retaining stdin, credentials, and other resources. Run the helper in a process group and terminate the group on timeout, or otherwise track and clean up descendants. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Timed-out credential helpers leave descendant processes running.
- ⚠️ Descendants may retain inherited stdin and sensitive process state.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** internal/llm/keycmd.go
**Line:** 81:81
**Comment:**
*Resource Leak: `WaitDelay` only closes the inherited stdout pipe after the deadline; it does not terminate grandchildren spawned by `sh -c`. A command such as `sleep 200 & printf tok` leaves the background process alive after resolution returns, potentially retaining stdin, credentials, and other resources. Run the helper in a process group and terminate the group on timeout, or otherwise track and clean up descendants.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
2772b40 to
8871236
Compare
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
User descriptionDescriptionAdds ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key"Today the only options are a plaintext PrecedenceOne 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 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 Backward compatibilityBoth keys are new and Since the value is executed as a shell command, Type of Change
Hardening found while validatingEach of these was a way the feature did not actually work end to end:
Separately, WindowsThe command line goes to A command string is not portable between the two arms — Why this adds a
|
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.
8871236 to
1bf219a
Compare
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.
1 participant
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.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.Separately,
cloneProviderEntrywas copying fields by hand and had never been updated forTimeoutSecandExtraHeaders, so editing any provider throughocr config providersilently erased both — atimeout_secof 900 reverted to the 300s default and customextra_headersdisappeared. 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.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 commits do 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.gofmt -s -lclean ·go vet ./...clean ·go test -race -count=1 ./...green · coverage 81.8% (gate 80%) ·govulncheckclean ·GOOS=windows go vet ./...clean · full suite green onwindows-latest. Each of the three commits independently passes 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 alibaba#236