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 7 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
Allow .proto files through the extension allowlist and map them to a dedicated protobuf review rule focused on wire compatibility. Part of alibaba#470.
ae88821 to
13aa10b
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).
cloneProviderEntry copied fields by hand and was never updated when TimeoutSec and ExtraHeaders were added to ProviderEntry, so editing any provider through `ocr config provider` silently erased both from the saved config: a per-provider timeout_sec of 900 reverted to the 300s default and custom extra_headers disappeared. Return the literal directly and use maps.Clone for both maps, which also preserves nil (matching each field's omitempty) where the old hand-rolled loop only special-cased ExtraBody. The regression test walks ProviderEntry with reflection and fails if the fixture leaves any field zero, so the next added field cannot be dropped without the DeepEqual catching it. Pre-existing bug, independent of the api_key_cmd work in this branch — separate commit so it can be bisected or cherry-picked on its own.
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.
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. 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 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 names the configured command on both the provider tabs and the Manual tab, rendering the command line and not the secret. 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.
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
| // Trim like the Official and Custom tabs: a whitespace-only token must | ||
| // never persist, or it wins precedence over a working auth_token_cmd | ||
| // and sends "Authorization: Bearer ". | ||
| apiKey := strings.TrimSpace(m.manualTokenInput.Value()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Manual tab silently restores a saved auth token even after the user clears it.
apiKey := strings.TrimSpace(m.manualTokenInput.Value()) correctly trims, but the guard m.manualTokenMasked || (apiKey == "" && m.manualTokenOriginal != "") reverts to manualTokenOriginal whenever the trimmed input is empty and an original token exists — even when the field is not masked (i.e., the user actively edited/cleared it). This diverges from the tabOfficial/tabCustom pattern just above (lines 1883-1888, 1934-1939), which only substitutes the original when apiKeyMasked is true and otherwise persists whatever the trimmed input is, including empty (this is exactly what lets TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey clear a saved key).
Practical impact: a user with an existing llm.auth_token who configures llm.auth_token_cmd and tries to clear the static token via the Manual wizard cannot — the old token is silently restored, and since static credentials take resolver precedence over commands per this PR's stated resolution order, the command is silently ignored with no error surfaced.
All cases in TestHandleManualFormEnter_AuthTokenGate still pass with the extra disjunct removed (the "saved auth_token" case is already covered by manualTokenMasked alone).
🐛 Proposed fix to align with the Official/Custom tab pattern
case tabManual:
// Trim like the Official and Custom tabs: a whitespace-only token must
// never persist, or it wins precedence over a working auth_token_cmd
// and sends "Authorization: Bearer ".
apiKey := strings.TrimSpace(m.manualTokenInput.Value())
- if m.manualTokenMasked || (apiKey == "" && m.manualTokenOriginal != "") {
+ if m.manualTokenMasked {
apiKey = m.manualTokenOriginal
}Consider also adding a regression test case (e.g., existing saved token + explicit clear via unmasked empty input → expect result().apiKey == "") to lock in the fix.
🤖 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.go` around lines 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.
3 participants
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