feat(config): api_key_cmd / auth_token_cmd — resolve LLM credential from a command (#236)#13
feat(config): api_key_cmd / auth_token_cmd — resolve LLM credential from a command (#236)#13chethanuk wants to merge 3 commits into
Conversation
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).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 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, and multilingual documentation. ChangesCommand-based credential resolution
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ResolveEndpoint
participant tryProviderConfig
participant resolveKeyCmd
participant ShellCommand
ResolveEndpoint->>tryProviderConfig: resolve provider endpoint
tryProviderConfig->>resolveKeyCmd: resolve api_key_cmd
resolveKeyCmd->>ShellCommand: execute credential command
ShellCommand-->>resolveKeyCmd: return validated stdout credential
resolveKeyCmd-->>tryProviderConfig: provide API key
tryProviderConfig-->>ResolveEndpoint: return endpoint with token
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Add the two coverage gaps from review: a CRLF-terminated command output row in the runner matrix (asserts \r\n fully trimmed), and stderr-warning assertions for the both-set case on both the provider path (api_key + api_key_cmd) and the legacy path (auth_token + auth_token_cmd), via an inlined os.Pipe stderr-capture helper.
|
Adversarial review pass complete (precedence, command-execution safety, error hygiene, edge cases, config round-trip, TUI persistence, docs) — verdict: ship-ready, no correctness bugs. Confirmed correct: static Closed two review-flagged test gaps in ace73aa: a CRLF-output row in the runner matrix, and stderr both-set-warning assertions on both the provider and legacy paths (via an inlined Left as documented judgment (not fixed): unbounded stdout buffer and grandchild-kill on timeout — both are the user's own local config (same trust as any shell command they'd run), and the common credential tools ( Gates: Holding the upstream PR until the maintainer signs off on the §C5 design proposal; ready to open it against that the moment they approve. |
Satisfy Go doc-comment convention (and CodeRabbit's docstring gate) for the per-OS credential-command runner; comments only, no behavior change.
![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.
1 participant
Fork-side staging + review PR for upstream alibaba#236. Upstream PR intentionally held until the maintainer signs off on the §C5 design proposal.
Lets users fetch the LLM provider key via a command (
op read,pass show) instead of storing it plaintext in config.json — same pattern as gitcredential.helper/ AWScredential_process.Design
tryProviderConfig, presets + custom alike): staticapi_keyalways wins (stderr warning if a command is also set) →api_key_cmd→ preset env var → error. Legacyllmblock gets a mirroredauth_token_cmd; an incomplete legacy block never runs the command, a set-but-failing command on a complete block is a hard error.sh -c/cmd /C), 60s timeout, child stderr passes through (pinentry/1Password/op prompts stay visible), stdout trimmed and used in memory only — never logged or written. Empty / whitespace-only / multi-line / timed-out output are all hard errors. No caching.config setsupports + round-trips the fields (not masked — they're command lines, not secrets); TUI clone preservesapi_key_cmd; docs in configuration.md (en/zh/ja).Verification
go build ./...+GOOS=windows go build ./internal/llm/...(Windows arm compile-checked; CI is Linux-only)go test ./internal/llm/... ./cmd/opencodereview/...— pass (table-driven runner matrix: success/trim/non-zero/empty/whitespace/multi-line/not-found/timeout; resolver precedence + legacy-fallthrough rows)go vetclean;make coverage81.3% (≥80% gate)Summary by CodeRabbit
New Features
api_key_cmdfor dynamically fetching provider API keys andauth_token_cmdfor legacy LLM auth tokens via shell commands.api_key/auth_tokentake precedence (with warnings when both static and command forms are set).Documentation
Tests