mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-21 14:44:56 +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. |
||
|---|---|---|
| .agents/plugins | ||
| .claude/commands | ||
| .claude-plugin | ||
| .github | ||
| bin | ||
| cmd/opencodereview | ||
| examples | ||
| extensions/vscode | ||
| imgs | ||
| internal | ||
| npm | ||
| pages | ||
| plugins/open-code-review | ||
| scripts | ||
| skills | ||
| .gitattributes | ||
| .gitignore | ||
| .npmignore | ||
| action.yml | ||
| AGENTS.md | ||
| ASSURANCE_CASE.md | ||
| CLAUDE.md | ||
| CODE_OF_CONDUCT.md | ||
| CONTRIBUTING.ja-JP.md | ||
| CONTRIBUTING.ko-KR.md | ||
| CONTRIBUTING.md | ||
| CONTRIBUTING.ru-RU.md | ||
| CONTRIBUTING.zh-CN.md | ||
| go.mod | ||
| go.sum | ||
| GOVERNANCE.md | ||
| install.ps1 | ||
| install.sh | ||
| LICENSE | ||
| Makefile | ||
| package.json | ||
| README.ja-JP.md | ||
| README.ko-KR.md | ||
| README.md | ||
| README.ru-RU.md | ||
| README.zh-CN.md | ||
| ROADMAP.md | ||
| SECURITY.md | ||
OpenCodeReview
English | 简体中文 | 日本語 | 한국어 | Русский
What is Open Code Review?
Open Code Review is an AI-powered code review CLI tool. It originated as Alibaba Group's internal official AI code review assistant — over the past two years, it has served tens of thousands of developers and identified millions of code defects. After thorough validation at massive scale, we incubated it into an open source project for the community. Simply configure a model endpoint to get started.
It reads Git diffs, sends changed files to a configurable LLM via an agent with tool-use capabilities, and generates structured review comments with line-level precision. The agent can read full file contents, search the codebase, inspect other changed files for context, and produce deep reviews — not just surface-level diff feedback. Beyond diff review, ocr scan reviews entire files for auditing unfamiliar codebases or directories that have no meaningful diff.
Visit the official website for more details.
Benchmark
Compared to general-purpose agents (Claude Code), Open Code Review achieves significantly higher Precision and F1 with the same underlying model, while consuming only ~1/9 of the tokens and completing reviews faster. Note that its Recall is lower than general-purpose agents — a deliberate trade-off favoring precision over noise.
A real-world code review benchmark built from 50 popular open-source repositories, 200 real Pull Requests, and 10 programming languages — cross-validated by 80+ senior engineers (1,505 annotated ground-truth issues).
Explore the AACR-Bench dataset on Hugging Face.
| Metric | What it measures | Why it matters |
|---|---|---|
| F1 | Harmonic mean of precision and recall | Best single number for overall review quality |
| Precision | Proportion of reported issues that are real defects | Higher = fewer false alarms to triage |
| Recall | Proportion of real defects that are found | Higher = fewer issues slip through review |
| Avg Time | Wall-clock time per review | Matters for CI pipeline latency |
| Avg Token | Total tokens consumed per review | Directly impacts API cost |
Why Open Code Review?
The Problem with General-Purpose Agents
If you've used general-purpose agents like Claude Code with Skills for code review, you've likely encountered these pain points:
- Incomplete coverage — On larger changesets, agents tend to "cut corners," selectively reviewing only some files and missing others.
- Position drift — Reported issues frequently don't match the actual code location, with line numbers or file references drifting off target.
- Unstable quality — Natural-language-driven Skills are hard to debug, and review quality fluctuates significantly with minor prompt variations.
The root cause: a purely language-driven architecture lacks hard constraints on the review process.
Core Design: Deterministic Engineering × Agent Hybrid
Open Code Review's core philosophy is to combine deterministic engineering with an agent, each handling what it does best.
Deterministic Engineering — Hard Constraints
For review steps that must not go wrong, engineering logic — not the language model — guarantees correctness:
- Precise file selection — Determines exactly which files need review and which should be filtered, ensuring no important change is missed.
- Smart file bundling — Groups related files into a single review unit (e.g.,
message_en.propertiesandmessage_zh.propertiesare bundled together). Each bundle runs as a sub-agent with isolated context — a divide-and-conquer strategy that stays stable on very large changesets and naturally supports concurrent review. - Fine-grained rule matching — Matches review rules to each file's characteristics, keeping the model's attention sharply focused and eliminating information noise at the source. Compared to purely language-driven rule guidance, template-engine-based rule matching is more stable and predictable.
- External positioning and reflection modules — Independent comment-positioning and comment-reflection modules systematically improve both the location accuracy and content accuracy of AI feedback.
Agent — Dynamic Decision-Making
The agent's strengths are concentrated where they matter most — dynamic decisions and dynamic context retrieval:
- Scenario-tuned prompts — Prompt templates deeply optimized for code review, improving effectiveness while reducing token consumption.
- Scenario-tuned toolset — Distilled from deep analysis of tool-call traces in large-scale production data — including call frequency distributions, per-tool repetition rates, and the impact of new tools on the overall call chain — resulting in a purpose-built toolset that is more stable and predictable for code review than a generic agent toolkit.
How to Use
Prerequisites
- Git >= 2.41 — Open Code Review relies on Git for diff generation, code search, and repository operations.
CLI
Install
npm install -g @alibaba-group/open-code-review
After installation, the ocr command is available globally.
For other installation methods (install script, GitHub Release binary, from source), see Installation.
Quick Start
1. Configure LLM
You must configure an LLM before reviewing code, unless you use Delegation Mode.
ocr config provider # Select a built-in provider or add a custom one
ocr config model # Pick a model for the active provider
The interactive UI guides you through provider selection, API key entry, and model configuration, then automatically tests connectivity.
For CLI setup, environment variables, custom providers, and other advanced configuration, see Configuration.
2. Review
cd your-project
# Workspace mode — review all staged, unstaged, and untracked changes
ocr review
# Branch range — reviews feature-branch's changes since it diverged from main (merge-base mode)
ocr review --from main --to feature-branch
# Single commit
ocr review --commit abc123
# Resume an interrupted range or commit review
ocr session list
ocr review --from main --to feature-branch --resume <session-id>
# Full-file scan — review whole files instead of a diff (no git history needed)
ocr scan # scan the entire repository
ocr scan --path internal/agent # scan a directory or specific files
ocr scan --resume <session-id> # resume an interrupted full-file scan
# Delegation mode — let your AI coding agent perform the review itself
# OCR handles file selection and rule resolution; no LLM configuration needed
ocr delegate preview
ocr delegate rule src/main.go src/handler.go
Documentation
Full documentation lives at open-codereview.ai/docs:
- Quickstart — install and run your first review
- Installation — all platforms and package managers
- CLI Reference — every command and flag
- Review Rules — customize review rules with path filtering and targeting
- Configuration — config keys and environment variables
- MCP Server — extend the review agent with external tools
- Coding Agent Integrations — choose the platform you use
- Claude Code — install a plugin with review slash commands
- Codex — install a plugin with callable review skills
- Cursor — install a plugin with portable review skills
- OpenCode — install native review tools and slash commands
- QCA Forward — run delegation mode with the QCA host model and a ready-to-publish template
- Skill-compatible agents — install the portable agent skill
- Review Execution Modes — after integration, choose which LLM performs the review
- Default (OCR-managed) — OCR runs the review using its configured LLM
- Delegation Mode — your coding agent runs the review using its own LLM; no OCR API key required
- CI/CD Integration — GitHub Actions, GitLab CI, GitFlic CI, and Gerrit integration
- Session Viewer — browse and replay review sessions in browser
- Telemetry — OpenTelemetry integration for observability
- FAQ — common questions and troubleshooting
Contributing
This project exists thanks to all the people who contribute. See CONTRIBUTING.md for development setup, coding guidelines, and how to submit pull requests.
License
Apache-2.0 — Copyright 2026 Alibaba