Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions cmd/opencodereview/emit_run_result_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
)

type mockResultProvider struct {
Expand All @@ -27,6 +28,7 @@ type mockResultProvider struct {
toolCalls map[string]int64
resumeInfo *agent.ResumeInfo
sessionID string
manifest *session.RunManifest
}

func (m *mockResultProvider) Diffs() []model.Diff { return m.diffs }
Expand All @@ -41,6 +43,7 @@ func (m *mockResultProvider) ProjectSummary() string { return m.projectS
func (m *mockResultProvider) ToolCalls() map[string]int64 { return m.toolCalls }
func (m *mockResultProvider) ResumeInfo() *agent.ResumeInfo { return m.resumeInfo }
func (m *mockResultProvider) SessionID() string { return m.sessionID }
func (m *mockResultProvider) Manifest() *session.RunManifest { return m.manifest }

func TestEmitRunResult_JSONNoFiles(t *testing.T) {
ag := &mockResultProvider{filesReviewed: 0}
Expand Down Expand Up @@ -280,6 +283,91 @@ func TestEmitRunResult_JSONNoFilesTraceID(t *testing.T) {
}
}

func TestEmitRunResult_JSONIncludesManifest(t *testing.T) {
ag := &mockResultProvider{
filesReviewed: 3,
inputTokens: 10,
outputTokens: 5,
totalTokens: 15,
manifest: &session.RunManifest{
SchemaVersion: session.ManifestSchemaVersion,
SessionID: "sess-1",
State: session.StatePartial,
Files: session.ManifestFiles{
Selected: []string{"a.go", "b.go"},
Completed: []string{"a.go"},
Failed: []string{"b.go"},
},
},
}
got := captureStdout(t, func() {
if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
var out jsonOutput
if err := json.Unmarshal([]byte(got), &out); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if out.Manifest == nil {
t.Fatal("manifest missing from JSON output")
}
if out.Manifest.State != session.StatePartial {
t.Errorf("manifest.state = %q, want partial", out.Manifest.State)
}
if len(out.Manifest.Files.Selected) != 2 {
t.Errorf("manifest selected = %v", out.Manifest.Files.Selected)
}
// Legacy status must remain unchanged (manifest.state is the NEW contract).
if out.Status != "success" {
t.Errorf("legacy status = %q, want success (unchanged by manifest)", out.Status)
}
}

// TestLegacyStatusValuesUnchanged locks the legacy `status` field to its
// pre-manifest values across the success/warning/error/no-files paths, and
// asserts every path still carries the manifest. manifest.state is the new
// machine contract; status must not drift.
func TestLegacyStatusValuesUnchanged(t *testing.T) {
tests := []struct {
name string
filesReviewed int64
warnings []agent.AgentWarning
manifestState string
wantStatus string
wantManifestState string
}{
{"clean success", 2, nil, session.StatePartial, "success", session.StatePartial},
{"subtask errors", 2, []agent.AgentWarning{{Type: "subtask_error", File: "a.go", Message: "boom"}}, session.StatePartial, "completed_with_errors", session.StatePartial},
{"non-error warnings", 2, []agent.AgentWarning{{Type: "token_threshold_exceeded", Message: "big"}}, session.StatePartial, "completed_with_warnings", session.StatePartial},
{"no files skipped", 0, nil, session.StateSkipped, "skipped", session.StateSkipped},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ag := &mockResultProvider{
filesReviewed: tt.filesReviewed,
warnings: tt.warnings,
manifest: &session.RunManifest{State: tt.manifestState},
}
got := captureStdout(t, func() {
if err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil); err != nil {
t.Fatalf("err: %v", err)
}
})
var out jsonOutput
if err := json.Unmarshal([]byte(got), &out); err != nil {
t.Fatalf("unmarshal: %v", err)
}
if out.Status != tt.wantStatus {
t.Errorf("status = %q, want %q", out.Status, tt.wantStatus)
}
if out.Manifest == nil || out.Manifest.State != tt.wantManifestState {
t.Errorf("manifest.state should be present (%q) regardless of legacy status, got %+v", tt.wantManifestState, out.Manifest)
}
})
}
}

func TestEmitRunResult_JSONIncludesSessionID(t *testing.T) {
ag := &mockResultProvider{filesReviewed: 1, sessionID: "session-99"}
got := captureStdout(t, func() {
Expand Down
12 changes: 11 additions & 1 deletion cmd/opencodereview/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ type reviewOptions struct {
to string
commit string
resume string
waive string // --waive: comma-separated paths to skip (requires --resume)
excludes string // --exclude: comma-separated gitignore-style patterns
outputFormat string
audience string // --audience: "human" (default) or "agent"
Expand All @@ -128,6 +129,7 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
a.StringVar(&opts.to, "to", "", "target ref to end diff at (e.g., 'feature-branch')")
a.StringVarP(&opts.commit, "commit", "c", "", "single commit hash or tag to review (vs its parent)")
a.StringVar(&opts.resume, "resume", "", "resume from a previous review session id")
a.StringVar(&opts.waive, "waive", "", "comma-separated paths to waive (skip but count as covered); requires --resume")
a.StringVar(&opts.excludes, "exclude", "", "comma-separated gitignore-style patterns to exclude; merged with rule.json excludes")
a.StringVarP(&opts.outputFormat, "format", "f", "text", "output format: text or json")
a.IntVar(&opts.concurrency, "concurrency", 8, "max concurrent file reviews")
Expand Down Expand Up @@ -169,6 +171,13 @@ func parseReviewFlags(args []string) (reviewOptions, error) {
if opts.preview && opts.resume != "" {
return opts, fmt.Errorf("--preview and --resume cannot be used together")
}
// Waiving only makes sense on a resumed run: it deliberately leaves diffs
// unreviewed while still satisfying the coverage contract. There is no
// fingerprint-addressed or config-file waive — paths only, and only with
// --resume. (ponytail: path-scoped waives cover the operator use case.)
if opts.waive != "" && opts.resume == "" {
return opts, fmt.Errorf("--waive requires --resume")
}

switch opts.audience {
case "human", "agent":
Expand Down Expand Up @@ -246,7 +255,8 @@ Flags:
--rule string path to JSON file with system review rules
--timeout int concurrent task timeout in minutes (default 10)
--to string target ref to end diff at (e.g., 'feature-branch')
--tools string path to JSON tools config file (default: embedded)`)
--tools string path to JSON tools config file (default: embedded)
--waive string comma-separated paths to waive (skip but count as covered); requires --resume`)
}

// --- config subcommand ---
Expand Down
13 changes: 13 additions & 0 deletions cmd/opencodereview/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@ func TestParseReviewFlags_PreviewWithResume(t *testing.T) {
}
}

func TestParseReviewFlags_WaiveRequiresResume(t *testing.T) {
if _, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--waive", "a.go"}); err == nil {
t.Fatal("expected error for --waive without --resume")
}
opts, err := parseReviewFlags([]string{"--from", "main", "--to", "feature", "--resume", "s1", "--waive", "a.go,b.go"})
if err != nil {
t.Fatalf("--waive with --resume should be valid: %v", err)
}
if opts.waive != "a.go,b.go" {
t.Errorf("waive = %q", opts.waive)
}
}

func TestParseReviewFlags_InvalidAudience(t *testing.T) {
_, err := parseReviewFlags([]string{"--audience", "robot"})
if err == nil {
Expand Down
10 changes: 8 additions & 2 deletions cmd/opencodereview/output.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/open-code-review/open-code-review/internal/agent"
"github.com/open-code-review/open-code-review/internal/model"
"github.com/open-code-review/open-code-review/internal/session"
"github.com/open-code-review/open-code-review/internal/suggestdiff"
)

Expand Down Expand Up @@ -249,6 +250,9 @@ type jsonOutput struct {
ProjectSummary string `json:"project_summary,omitempty"`
Resume *agent.ResumeInfo `json:"resume,omitempty"`
SessionID string `json:"session_id,omitempty"`
// Manifest is the immutable run manifest (coverage + terminal state). It is
// the NEW machine contract; the legacy Status field above is unchanged.
Manifest *session.RunManifest `json:"manifest,omitempty"`
}

func outputJSON(comments []model.LlmComment) error {
Expand All @@ -266,7 +270,7 @@ func outputJSON(comments []model.LlmComment) error {

func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentWarning,
filesReviewed, inputTokens, outputTokens, totalTokens, cacheReadTokens, cacheWriteTokens int64,
duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string) error {
duration time.Duration, projectSummary string, toolCalls map[string]int64, traceID string, resumeInfo *agent.ResumeInfo, sessionID string, manifest *session.RunManifest) error {
out := jsonOutput{
Status: "success",
TraceID: traceID,
Expand All @@ -284,6 +288,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
ProjectSummary: projectSummary,
Resume: resumeInfo,
SessionID: sessionID,
Manifest: manifest,
}
var total int64
for _, v := range toolCalls {
Expand Down Expand Up @@ -317,7 +322,7 @@ func outputJSONWithWarnings(comments []model.LlmComment, warnings []agent.AgentW
return enc.Encode(out)
}

func outputJSONNoFiles(traceID string) error {
func outputJSONNoFiles(traceID string, manifest *session.RunManifest) error {
out := jsonOutput{
Status: "skipped",
TraceID: traceID,
Expand All @@ -326,6 +331,7 @@ func outputJSONNoFiles(traceID string) error {
ToolCalls: &jsonToolCalls{
ByTool: map[string]int64{},
},
Manifest: manifest,
}
enc := json.NewEncoder(os.Stdout)
enc.SetIndent("", " ")
Expand Down
8 changes: 4 additions & 4 deletions cmd/opencodereview/output_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) {
os.Stdout = w

warnings := []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "fail"}}
err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "")
err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil, "abc123trace", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -281,7 +281,7 @@ func TestOutputJSONWithWarnings(t *testing.T) {

comments := []model.LlmComment{{Path: "b.go", Content: "test"}}
warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}}
err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "")
err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3}, "trace-xyz-789", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -319,7 +319,7 @@ func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) {
os.Stdout = w

warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}}
err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "")
err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 0, 0, time.Second, "", nil, "", nil, "", nil)
_ = w.Close()
os.Stdout = old

Expand Down Expand Up @@ -347,7 +347,7 @@ func TestOutputJSONNoFiles(t *testing.T) {
r, w, _ := os.Pipe()
os.Stdout = w

err := outputJSONNoFiles("test-trace-id-456")
err := outputJSONNoFiles("test-trace-id-456", nil)

_ = w.Close()
os.Stdout = old
Expand Down
9 changes: 9 additions & 0 deletions cmd/opencodereview/review_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ func runReview(args []string) error {
rt.PlanToolDefs = append(rt.PlanToolDefs, mcpToolDefs...)
rt.MainToolDefs = append(rt.MainToolDefs, mcpToolDefs...)

var lang string
if rt.AppCfg != nil {
lang = rt.AppCfg.Language
}
runMeta := buildRunMeta(rt.Endpoint, lang, Version, cc.RepoDir, cc.Resolver, opts.concurrency,
resolveRange(cc.RepoDir, opts.from, opts.to, opts.commit))

ag := agent.New(agent.Args{
RepoDir: cc.RepoDir,
From: opts.from,
Expand All @@ -119,6 +126,8 @@ func runReview(args []string) error {
Background: opts.background,
GitRunner: cc.GitRunner,
Resume: resumeState,
RunMeta: runMeta,
WaivePaths: splitPaths(opts.waive),
})

// Silence progress output during execution; restored before the trace
Expand Down
Loading
Loading