mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-20 14:14:30 +00:00
f75c43af45
* feat(config): resolve api_key/auth_token from a command (#236) 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). * fix(llm): harden the credential command and cover it on Windows 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. * fix(config): drop duplicated license header in testconnection 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. * docs(i18n): sync api_key_cmd configuration docs to ru 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.
215 lines
6.3 KiB
Go
215 lines
6.3 KiB
Go
// SPDX-License-Identifier: Apache-2.0
|
|
// Copyright 2026 alibaba/open-code-review Contributors
|
|
|
|
package viewer
|
|
|
|
import (
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"os"
|
|
"path/filepath"
|
|
"runtime"
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
func TestHandleRepos_Success(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "test-repo")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
writeJSONL(t, filepath.Join(repoDir, "s1.jsonl"),
|
|
`{"type":"session_start","timestamp":"2025-01-01T10:00:00Z"}`)
|
|
|
|
req := httptest.NewRequest("GET", "/", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleRepos(rr, req, root)
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
if !strings.Contains(rr.Body.String(), "test-repo") {
|
|
t.Errorf("response does not contain repo name")
|
|
}
|
|
}
|
|
|
|
func TestHandleRepos_EmptyRoot(t *testing.T) {
|
|
root := t.TempDir()
|
|
req := httptest.NewRequest("GET", "/", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleRepos(rr, req, root)
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
if !strings.Contains(rr.Body.String(), "No session data found") {
|
|
t.Errorf("expected empty-state message in body")
|
|
}
|
|
}
|
|
|
|
func TestHandleRepos_NotFoundForNonRootPath(t *testing.T) {
|
|
root := t.TempDir()
|
|
req := httptest.NewRequest("GET", "/other", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleRepos(rr, req, root)
|
|
|
|
if rr.Code != http.StatusNotFound {
|
|
t.Errorf("status = %d, want 404", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleRepos_UnreadableRoot(t *testing.T) {
|
|
req := httptest.NewRequest("GET", "/", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleRepos(rr, req, "/nonexistent/definitely/missing/root")
|
|
|
|
// DiscoverRepos returns nil for non-existent dirs (treated as empty)
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200 (empty)", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleRepos_PermissionDenied(t *testing.T) {
|
|
// Chmod(0000) on Windows only sets the read-only bit, so ReadDir still
|
|
// succeeds and the handler returns 200. (The Getuid guard below cannot cover
|
|
// this: Getuid returns -1 on Windows, never 0.)
|
|
if runtime.GOOS == "windows" {
|
|
t.Skip("unix permissions not enforced on Windows")
|
|
}
|
|
if os.Getuid() == 0 {
|
|
t.Skip("permission checks are bypassed for root")
|
|
}
|
|
root := t.TempDir()
|
|
// Create a directory that exists but cannot be read
|
|
badDir := filepath.Join(root, "unreadable")
|
|
if err := os.MkdirAll(badDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
// Create a .jsonl inside so it's a valid sessions dir
|
|
writeJSONL(t, filepath.Join(badDir, "s.jsonl"), `{"type":"session_start"}`)
|
|
// Remove read permission on root
|
|
if err := os.Chmod(root, 0000); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
t.Cleanup(func() { _ = os.Chmod(root, 0755) })
|
|
|
|
req := httptest.NewRequest("GET", "/", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleRepos(rr, req, root)
|
|
|
|
if rr.Code != http.StatusInternalServerError {
|
|
t.Errorf("status = %d, want 500", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleSessions_Success(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "myrepo")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
writeJSONL(t, filepath.Join(repoDir, "sess1.jsonl"),
|
|
`{"type":"session_start","timestamp":"2025-03-01T09:00:00Z","cwd":"/home/user/project","model":"gpt-4"}`,
|
|
`{"type":"session_end","duration_seconds":60,"files_reviewed":["a.go"]}`)
|
|
|
|
req := httptest.NewRequest("GET", "/r/myrepo", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSessions(rr, req, root, "myrepo")
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
body := rr.Body.String()
|
|
if !strings.Contains(body, "project") {
|
|
t.Errorf("response does not contain repo display name derived from CWD")
|
|
}
|
|
}
|
|
|
|
func TestHandleSessions_NoCWD(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "repo2")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
writeJSONL(t, filepath.Join(repoDir, "s.jsonl"),
|
|
`{"type":"session_start","timestamp":"2025-01-01T00:00:00Z","model":"m"}`)
|
|
|
|
req := httptest.NewRequest("GET", "/r/repo2", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSessions(rr, req, root, "repo2")
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleSessions_ErrorOnBadDir(t *testing.T) {
|
|
root := t.TempDir()
|
|
req := httptest.NewRequest("GET", "/r/missing", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSessions(rr, req, root, "missing")
|
|
|
|
if rr.Code != http.StatusInternalServerError {
|
|
t.Errorf("status = %d, want 500", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleSession_Success(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "repo")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
writeJSONL(t, filepath.Join(repoDir, "abc123.jsonl"),
|
|
`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj","model":"claude"}`,
|
|
`{"type":"llm_request","filePath":"main.go","taskType":"main_task","request_no":1,"messages":[]}`,
|
|
`{"type":"llm_response","filePath":"main.go","taskType":"main_task","content":"LGTM"}`,
|
|
`{"type":"session_end","duration_seconds":30,"files_reviewed":["main.go"]}`)
|
|
|
|
req := httptest.NewRequest("GET", "/r/repo/abc123", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSession(rr, req, root, "repo", "abc123")
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
if !strings.Contains(rr.Body.String(), "proj") {
|
|
t.Errorf("response does not contain derived display name")
|
|
}
|
|
}
|
|
|
|
func TestHandleSession_NotFound(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "repo")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
req := httptest.NewRequest("GET", "/r/repo/nonexistent", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSession(rr, req, root, "repo", "nonexistent")
|
|
|
|
if rr.Code != http.StatusNotFound {
|
|
t.Errorf("status = %d, want 404", rr.Code)
|
|
}
|
|
}
|
|
|
|
func TestHandleSession_EmptyCWD(t *testing.T) {
|
|
root := t.TempDir()
|
|
repoDir := filepath.Join(root, "repo")
|
|
if err := os.MkdirAll(repoDir, 0755); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
writeJSONL(t, filepath.Join(repoDir, "s.jsonl"),
|
|
`{"type":"session_start","timestamp":"2025-01-01T00:00:00Z","model":"m"}`,
|
|
`{"type":"session_end","duration_seconds":1,"files_reviewed":[]}`)
|
|
|
|
req := httptest.NewRequest("GET", "/r/repo/s", nil)
|
|
rr := httptest.NewRecorder()
|
|
handleSession(rr, req, root, "repo", "s")
|
|
|
|
if rr.Code != http.StatusOK {
|
|
t.Errorf("status = %d, want 200", rr.Code)
|
|
}
|
|
}
|