Files
历代星辰 e95bdda4f2 feat(cli): add --output flag to write review/scan results to a file (#852)
* feat(cli): add --output flag to write review/scan results to a file

Add `--output <path>` / `-o` to `ocr review` and `ocr scan`, writing the
result JSON or text directly to a UTF-8 file instead of stdout. The file
is created lazily on the first write so a failed run never truncates an
existing target; text-mode files are ANSI-stripped so terminal color
codes never pollute the result file. A `[ocr] Results written to <path>`
hint is printed to stderr once the file is actually created, and failure
output keeps going to stderr so agents always find the failure reason.

Closes #851

Signed-off-by: 历代星辰

* test(cli): cover --output flag parsing and file output behavior

Add tests for --output/-o flag parsing, the stripAnsiWriter state machine
(including escape sequences split across Write calls), lazy file creation
(failed runs leave existing targets untouched, never-written targets are
not created), and the Results-written stderr hint. Adapt existing
emitRunResult / renderComment / outputPreview call sites to the new
io.Writer parameter.

Signed-off-by: 历代星辰

* fix(test): isolate USERPROFILE so Windows tests never touch the real OCR home

os.UserHomeDir() prefers USERPROFILE over HOME on Windows, so
t.Setenv("HOME", ...) alone left tests reading and writing the
developer's real ~/.opencodereview: config tests overwrote config.json
and session/agent tests polluted the sessions store. Add a setTestHome
helper (per affected package) that also overrides USERPROFILE, and route
every scattered HOME override through it.

Signed-off-by: 历代星辰

* fix(cli): propagate output write failures and strip multi-byte ANSI escapes

Address the ocr review findings on the --output feature:
- lazyFileWriter now records the first write error (Err()) and emits the
  "Results written" hint only after a successful write; emitRunResult and
  outputPreview check it after text rendering, so a failed --output write
  (permission, disk full) exits non-zero like JSON mode already does
  instead of silently exiting 0 with no file.
- stripAnsiWriter keeps multi-byte escapes (ESC + intermediate byte,
  DCS/PM/APC strings) inside the escape state so trailing bytes are
  discarded with the sequence instead of leaking into the result file.

Signed-off-by: 历代星辰

* fix(cli): re-parse trailing byte after bare-ESC OSC termination and fix Write return semantics

Addresses review comments on #852:

- stripAnsiWriter ansiOSCEsc: a non-ST byte after ESC is no longer dropped.
  The OSC ends at a bare ESC terminator and the trailing byte is re-parsed —
  an ESC starts a new escape sequence, any other byte is forwarded as text.
  Previously the first byte after a bare-ESC-terminated OSC was silently lost.
- stripAnsiWriter Write: report the underlying dst error but return len(p),
  since the state machine has already consumed the input; returning 0 made a
  caller retrying on n < len(p) feed the same bytes through twice.
- Clarify --output help text: default is stdout and '-' also means stdout.

Adds regression tests for both bugs (bare-ESC + trailing text, bare-ESC + new
escape, and dst-failure return contract).

Signed-off-by: 历代星辰

* fix(cli): reinforce ANSI stripper state machine and validate format flag

* docs(cli): document output flag across localized references and READMEs

* fix(cli): cap escape intermediate bytes and normalize format in output handlers

---------

Signed-off-by: 历代星辰
2026-08-24 15:37:29 +08:00

283 lines
9.1 KiB
Go

// SPDX-License-Identifier: Apache-2.0
// Copyright 2026 alibaba/open-code-review Contributors
package main
import (
"context"
"encoding/json"
"io"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
"github.com/alibaba/open-code-review/internal/delegate"
)
func captureDelegateStdout(t *testing.T, fn func()) []byte {
t.Helper()
orig := os.Stdout
r, w, err := os.Pipe()
if err != nil {
t.Fatalf("create stdout pipe: %v", err)
}
os.Stdout = w
defer func() { os.Stdout = orig }()
// Drain while fn runs. Reading only after fn returns caps the capture at
// whatever the pipe buffer holds: 64 KiB on Linux, far less on a Windows
// anonymous pipe, and a payload past that blocks the writer forever.
var out []byte
var readErr error
done := make(chan struct{})
go func() {
defer close(done)
out, readErr = io.ReadAll(r)
}()
fn()
if err := w.Close(); err != nil {
t.Fatalf("close stdout writer: %v", err)
}
<-done
if readErr != nil {
t.Fatalf("read stdout: %v", readErr)
}
_ = r.Close()
return out
}
// gitCommitFile writes a file and commits it, returning after the commit lands.
func gitCommitFile(t *testing.T, dir, name, content, msg string) {
t.Helper()
if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil {
t.Fatalf("write %s: %v", name, err)
}
for _, args := range [][]string{{"add", "."}, {"commit", "-m", msg}} {
cmd := exec.Command("git", append([]string{"-C", dir}, args...)...)
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("git %v: %v: %s", args, err, out)
}
}
}
// silenceStdout redirects os.Stdout to /dev/null for the duration of fn so the
// delegate commands' Printf output does not clutter test logs.
func silenceStdout(t *testing.T, fn func()) {
t.Helper()
orig := os.Stdout
devnull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0)
if err != nil {
t.Fatalf("open devnull: %v", err)
}
os.Stdout = devnull
defer func() {
os.Stdout = orig
_ = devnull.Close()
}()
fn()
}
// setTestHome redirects the user home resolved by os.UserHomeDir() to dir for
// the duration of the test. Setting HOME alone is NOT enough on Windows:
// os.UserHomeDir() prefers USERPROFILE there, so tests would still read and
// write the developer's real ~/.opencodereview (and its config.json). Setting
// USERPROFILE is a no-op on non-Windows platforms.
func setTestHome(t *testing.T, dir string) {
t.Helper()
t.Setenv("HOME", dir)
t.Setenv("USERPROFILE", dir)
}
// freshOCRHome points the OCR home at a temp dir so a test can assert on what a
// command wrote there. It also neutralizes global git config: git resolves that
// via XDG_CONFIG_HOME as well, so overriding HOME alone would still pick up the
// developer's settings (e.g. commit.gpgsign, which fails without their keyring).
func freshOCRHome(t *testing.T) string {
t.Helper()
home := t.TempDir()
setTestHome(t, home)
t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull)
t.Setenv("GIT_CONFIG_SYSTEM", os.DevNull)
return home
}
// assertNoSessionStore fails if the session store was created under home.
// Preview neither runs nor finalizes a review, so it must not open persistence
// at all; session.New creates this directory before writing its JSONL file.
func assertNoSessionStore(t *testing.T, home string) {
t.Helper()
store := filepath.Join(home, ".opencodereview", "sessions")
if _, err := os.Stat(store); !os.IsNotExist(err) {
t.Errorf("preview created the session store at %s (stat err = %v)", store, err)
}
}
func TestExecuteDelegatePreview_Workspace(t *testing.T) {
dir := initTestGitRepo(t)
// Uncommitted change so the workspace preview has at least one entry.
if err := os.WriteFile(filepath.Join(dir, "app.go"), []byte("package app\n"), 0o644); err != nil {
t.Fatalf("write app.go: %v", err)
}
silenceStdout(t, func() {
if err := executeDelegatePreview(delegateOptions{repoDir: dir}); err != nil {
t.Fatalf("executeDelegatePreview(workspace) error: %v", err)
}
})
}
func TestExecuteDelegatePreview_Range(t *testing.T) {
dir := initTestGitRepo(t)
gitCommitFile(t, dir, "b.go", "package b\n", "second commit")
silenceStdout(t, func() {
err := executeDelegatePreview(delegateOptions{repoDir: dir, from: "HEAD~1", to: "HEAD"})
if err != nil {
t.Fatalf("executeDelegatePreview(range) error: %v", err)
}
})
}
func TestExecuteDelegatePreview_Commit(t *testing.T) {
dir := initTestGitRepo(t)
gitCommitFile(t, dir, "c.go", "package c\n", "add c")
silenceStdout(t, func() {
// commit mode auto-fills background from the commit message.
err := executeDelegatePreview(delegateOptions{repoDir: dir, commit: "HEAD"})
if err != nil {
t.Fatalf("executeDelegatePreview(commit) error: %v", err)
}
})
}
// TestExecuteDelegatePreviewCreatesNoSession covers the third preview entry
// point, which built its agent the same leaky way as review and scan.
func TestExecuteDelegatePreviewCreatesNoSession(t *testing.T) {
home := freshOCRHome(t)
dir := initTestGitRepo(t)
gitCommitFile(t, dir, "d.go", "package d\n", "add d")
silenceStdout(t, func() {
if err := executeDelegatePreview(delegateOptions{repoDir: dir, commit: "HEAD"}); err != nil {
t.Fatalf("executeDelegatePreview error: %v", err)
}
})
assertNoSessionStore(t, home)
}
func TestExecuteDelegateRule(t *testing.T) {
dir := initTestGitRepo(t)
silenceStdout(t, func() {
err := executeDelegateRule(delegateOptions{repoDir: dir}, []string{"README.md"})
if err != nil {
t.Fatalf("executeDelegateRule error: %v", err)
}
})
}
func TestExecuteDelegatePreviewJSON(t *testing.T) {
dir := initTestGitRepo(t)
if err := os.WriteFile(filepath.Join(dir, "app.go"), []byte("package app\n"), 0o644); err != nil {
t.Fatalf("write app.go: %v", err)
}
out := captureDelegateStdout(t, func() {
if err := executeDelegatePreview(delegateOptions{repoDir: dir, format: "json"}); err != nil {
t.Fatalf("executeDelegatePreview(json) error: %v", err)
}
})
var got delegatePreviewJSON
if err := json.Unmarshal(out, &got); err != nil {
t.Fatalf("decode preview JSON: %v\n%s", err, out)
}
if got.SchemaVersion != delegateSchemaVersion || got.Mode != "workspace" {
t.Fatalf("unexpected envelope: %#v", got)
}
if len(got.ReviewableFiles) != 1 || got.ReviewableFiles[0].Path != "app.go" {
t.Fatalf("reviewable_files = %#v", got.ReviewableFiles)
}
if got.ReviewableFiles == nil || got.ExcludedFiles == nil {
t.Fatal("JSON arrays must not be null")
}
}
func TestExecuteDelegateRuleJSON(t *testing.T) {
dir := initTestGitRepo(t)
out := captureDelegateStdout(t, func() {
if err := executeDelegateRule(delegateOptions{repoDir: dir, format: "json"}, []string{"README.md"}); err != nil {
t.Fatalf("executeDelegateRule(json) error: %v", err)
}
})
var got delegateRulesJSON
if err := json.Unmarshal(out, &got); err != nil {
t.Fatalf("decode rules JSON: %v\n%s", err, out)
}
if got.SchemaVersion != delegateSchemaVersion || len(got.Groups) != 1 {
t.Fatalf("unexpected rules envelope: %#v", got)
}
if len(got.Groups[0].Files) != 1 || got.Groups[0].Files[0] != "README.md" || got.Groups[0].Rule == "" {
t.Fatalf("unexpected rule group: %#v", got.Groups[0])
}
}
func TestRuleGroupsJSONEmptyFiles(t *testing.T) {
groups := ruleGroupsJSON([]delegate.RuleGroup{{ID: 1}})
if len(groups) != 1 {
t.Fatalf("groups = %#v", groups)
}
if groups[0].Files == nil || len(groups[0].Files) != 0 {
t.Fatalf("files must be an empty, non-nil slice: %#v", groups[0].Files)
}
payload, err := json.Marshal(groups[0])
if err != nil {
t.Fatalf("marshal rule group: %v", err)
}
if string(payload) == "" || !json.Valid(payload) {
t.Fatalf("invalid JSON: %s", payload)
}
var decoded map[string]any
if err := json.Unmarshal(payload, &decoded); err != nil {
t.Fatalf("decode rule group: %v", err)
}
if files, ok := decoded["files"].([]any); !ok || len(files) != 0 {
t.Fatalf("files JSON must be []: %s", payload)
}
}
func TestLoadDelegateContext_BackgroundFile(t *testing.T) {
dir := initTestGitRepo(t)
bgPath := filepath.Join(dir, "bg.txt")
if err := os.WriteFile(bgPath, []byte("extra background"), 0o644); err != nil {
t.Fatalf("write bg: %v", err)
}
dc, err := loadDelegateContext(delegateOptions{repoDir: dir, backgroundFile: "bg.txt", background: "base"})
if err != nil {
t.Fatalf("loadDelegateContext error: %v", err)
}
if !strings.Contains(dc.opts.background, "extra background") {
t.Errorf("expected file content to win, got %q", dc.opts.background)
}
if strings.Contains(dc.opts.background, "base") {
t.Errorf("inline --background should be ignored when --background-file is set, got %q", dc.opts.background)
}
}
func TestLoadDelegateContext_NotGitRepo(t *testing.T) {
dir := t.TempDir()
if _, err := loadDelegateContext(delegateOptions{repoDir: dir}); err == nil {
t.Fatal("expected error for non-git dir")
}
}
func TestDelegateContextMergeBase_Range(t *testing.T) {
dir := initTestGitRepo(t)
gitCommitFile(t, dir, "d.go", "package d\n", "add d")
dc, err := loadDelegateContext(delegateOptions{repoDir: dir, from: "HEAD~1", to: "HEAD"})
if err != nil {
t.Fatalf("loadDelegateContext error: %v", err)
}
if got := dc.mergeBase(context.Background()); got == "" {
t.Error("expected non-empty merge base for range mode")
}
}