From 24c9126cf2bdb8a8532bb7024ed92f011e50557a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Thu, 26 Feb 2026 13:51:40 -0600 Subject: [PATCH 1/3] fix: skip PASS-verdict reviews in roborev fix (#372) Reviews that passed (verdict "P") have no findings to fix. Both fixSingleJob and runFixBatch now check the verdict before invoking an agent, matching the behavior already present in refine's findFailedReviewForBranch. Passing reviews are auto-marked as addressed. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/fix.go | 27 ++++++ cmd/roborev/fix_test.go | 205 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+) diff --git a/cmd/roborev/fix.go b/cmd/roborev/fix.go index 38b1f907..681761f7 100644 --- a/cmd/roborev/fix.go +++ b/cmd/roborev/fix.go @@ -612,6 +612,15 @@ func firstLine(s string) string { return truncateString(s, 80) } +// jobVerdict returns the verdict for a job. Uses the stored verdict +// if available, otherwise parses from the review output. +func jobVerdict(job *storage.ReviewJob, review *storage.Review) string { + if job.Verdict != nil && *job.Verdict != "" { + return *job.Verdict + } + return storage.ParseVerdict(review.Output) +} + func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOptions) error { ctx := cmd.Context() if ctx == nil { @@ -634,6 +643,17 @@ func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOpti return fmt.Errorf("fetch review: %w", err) } + // Skip reviews that passed — no findings to fix + if jobVerdict(job, review) == "P" { + if !opts.quiet { + cmd.Printf("Job %d: review passed, skipping fix\n", jobID) + } + if err := markJobAddressed(serverAddr, jobID); err != nil && !opts.quiet { + cmd.Printf("Warning: could not mark job %d as addressed: %v\n", jobID, err) + } + return nil + } + if !opts.quiet { cmd.Printf("Job %d analysis output:\n", jobID) cmd.Println(strings.Repeat("-", 60)) @@ -798,6 +818,13 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, newestFirst } continue } + if jobVerdict(job, review) == "P" { + if !opts.quiet { + cmd.Printf("Skipping job %d (review passed)\n", id) + } + _ = markJobAddressed(serverAddr, id) + continue + } entries = append(entries, batchEntry{jobID: id, job: job, review: review}) } diff --git a/cmd/roborev/fix_test.go b/cmd/roborev/fix_test.go index b2fef018..696bdfa1 100644 --- a/cmd/roborev/fix_test.go +++ b/cmd/roborev/fix_test.go @@ -1292,3 +1292,208 @@ func TestFixWorktreeRepoResolution(t *testing.T) { } }) } + +func TestJobVerdict(t *testing.T) { + pass := "P" + fail := "F" + empty := "" + + tests := []struct { + name string + verdict *string + output string + want string + }{ + { + name: "stored PASS verdict", + verdict: &pass, + output: "some output", + want: "P", + }, + { + name: "stored FAIL verdict", + verdict: &fail, + output: "No issues found.", + want: "F", + }, + { + name: "nil verdict falls back to parse", + verdict: nil, + output: "No issues found.", + want: "P", + }, + { + name: "empty verdict falls back to parse", + verdict: &empty, + output: "## Issues\n- Bug in foo.go", + want: "F", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + job := &storage.ReviewJob{Verdict: tt.verdict} + review := &storage.Review{Output: tt.output} + got := jobVerdict(job, review) + if got != tt.want { + t.Errorf("jobVerdict() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestFixSingleJobSkipsPassVerdict(t *testing.T) { + repo := createTestRepo(t, map[string]string{ + "main.go": "package main\n", + }) + + var addressCalls atomic.Int32 + var agentCalled atomic.Int32 + + ts, _ := newMockServer(t, MockServerOpts{ + ReviewOutput: "No issues found.", + OnJobs: func(w http.ResponseWriter, r *http.Request) { + writeJSON(w, map[string]any{ + "jobs": []storage.ReviewJob{{ + ID: 99, + Status: storage.JobStatusDone, + Agent: "test", + }}, + }) + }, + }) + // Wrap the server to track address calls + wrapper := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/review/address" { + addressCalls.Add(1) + w.WriteHeader(http.StatusOK) + return + } + // Proxy to the original mock + proxy, err := http.NewRequest(r.Method, ts.URL+r.URL.String(), r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + resp, err := http.DefaultClient.Do(proxy) + if err != nil { + http.Error(w, err.Error(), http.StatusBadGateway) + return + } + defer resp.Body.Close() + w.WriteHeader(resp.StatusCode) + io.Copy(w, resp.Body) + })) + t.Cleanup(wrapper.Close) + patchServerAddr(t, wrapper.URL) + + cmd, output := newTestCmd(t) + + // Use a fake agent that tracks invocations + agent.Register(&agent.FakeAgent{ + NameStr: "test-pass-skip", + ReviewFn: func(_ context.Context, _, _, _ string, _ io.Writer) (string, error) { + agentCalled.Add(1) + return "", nil + }, + }) + + opts := fixOptions{agentName: "test-pass-skip"} + + err := fixSingleJob(cmd, repo.Dir, 99, opts) + if err != nil { + t.Fatalf("fixSingleJob: %v", err) + } + + outputStr := output.String() + if !strings.Contains(outputStr, "review passed, skipping fix") { + t.Errorf("expected skip message, got:\n%s", outputStr) + } + if agentCalled.Load() != 0 { + t.Error("agent should not have been invoked for passing review") + } + if addressCalls.Load() != 1 { + t.Errorf("expected 1 address call, got %d", addressCalls.Load()) + } +} + +func TestFixBatchSkipsPassVerdict(t *testing.T) { + repo := createTestRepo(t, map[string]string{ + "main.go": "package main\n", + }) + + var addressCalls atomic.Int32 + + passVerdict := "P" + + _ = newMockDaemonBuilder(t). + WithHandler("/api/jobs", func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + if q.Get("id") == "10" { + writeJSON(w, map[string]any{ + "jobs": []storage.ReviewJob{{ + ID: 10, + Status: storage.JobStatusDone, + Agent: "test", + Verdict: &passVerdict, + }}, + }) + } else if q.Get("id") == "20" { + writeJSON(w, map[string]any{ + "jobs": []storage.ReviewJob{{ + ID: 20, + Status: storage.JobStatusDone, + Agent: "test", + }}, + }) + } else { + writeJSON(w, map[string]any{ + "jobs": []storage.ReviewJob{}, + "has_more": false, + }) + } + }). + WithHandler("/api/review", func(w http.ResponseWriter, r *http.Request) { + jobID := r.URL.Query().Get("job_id") + if jobID == "10" { + writeJSON(w, storage.Review{ + JobID: 10, + Output: "No issues found.", + }) + } else { + writeJSON(w, storage.Review{ + JobID: 20, + Output: "## Issues\n- Bug in foo.go", + }) + } + }). + WithHandler("/api/review/address", func(w http.ResponseWriter, r *http.Request) { + addressCalls.Add(1) + w.WriteHeader(http.StatusOK) + }). + WithHandler("/api/enqueue", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }). + Build() + + out, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error { + return runFixBatch( + cmd, + []int64{10, 20}, + "", + false, + fixOptions{agentName: "test", reasoning: "fast"}, + ) + }) + if err != nil { + t.Fatalf("runFixBatch: %v", err) + } + + if !strings.Contains(out, "Skipping job 10 (review passed)") { + t.Errorf("expected skip message for job 10, got:\n%s", out) + } + // Job 20 (FAIL) should be processed — its findings should appear + if !strings.Contains(out, "Bug in foo.go") { + t.Errorf("expected FAIL job findings in output, got:\n%s", out) + } +} From 2748f5fcf9f741f564fcb80c17e0b4d792431091 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Thu, 26 Feb 2026 13:55:10 -0600 Subject: [PATCH 2/3] fix: log markJobAddressed errors in batch path, assert addressed job IDs in test Address review feedback: the batch PASS-skip path silently discarded markJobAddressed errors, inconsistent with fixSingleJob and the later batch marking logic. Also strengthen TestFixBatchSkipsPassVerdict to assert that job 10 is actually marked addressed by recording posted job_id values. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/fix.go | 4 +++- cmd/roborev/fix_test.go | 22 ++++++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/fix.go b/cmd/roborev/fix.go index 681761f7..87bea8ac 100644 --- a/cmd/roborev/fix.go +++ b/cmd/roborev/fix.go @@ -822,7 +822,9 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, newestFirst if !opts.quiet { cmd.Printf("Skipping job %d (review passed)\n", id) } - _ = markJobAddressed(serverAddr, id) + if err := markJobAddressed(serverAddr, id); err != nil && !opts.quiet { + cmd.Printf("Warning: could not mark job %d as addressed: %v\n", id, err) + } continue } entries = append(entries, batchEntry{jobID: id, job: job, review: review}) diff --git a/cmd/roborev/fix_test.go b/cmd/roborev/fix_test.go index 696bdfa1..4b8e1326 100644 --- a/cmd/roborev/fix_test.go +++ b/cmd/roborev/fix_test.go @@ -11,7 +11,9 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" + "sync" "sync/atomic" "testing" "time" @@ -1422,7 +1424,8 @@ func TestFixBatchSkipsPassVerdict(t *testing.T) { "main.go": "package main\n", }) - var addressCalls atomic.Int32 + var mu sync.Mutex + var addressedJobIDs []int64 passVerdict := "P" @@ -1468,7 +1471,14 @@ func TestFixBatchSkipsPassVerdict(t *testing.T) { } }). WithHandler("/api/review/address", func(w http.ResponseWriter, r *http.Request) { - addressCalls.Add(1) + var body struct { + JobID int64 `json:"job_id"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err == nil { + mu.Lock() + addressedJobIDs = append(addressedJobIDs, body.JobID) + mu.Unlock() + } w.WriteHeader(http.StatusOK) }). WithHandler("/api/enqueue", func(w http.ResponseWriter, r *http.Request) { @@ -1496,4 +1506,12 @@ func TestFixBatchSkipsPassVerdict(t *testing.T) { if !strings.Contains(out, "Bug in foo.go") { t.Errorf("expected FAIL job findings in output, got:\n%s", out) } + + // Verify PASS job 10 was marked addressed during the skip phase + mu.Lock() + ids := addressedJobIDs + mu.Unlock() + if !slices.Contains(ids, int64(10)) { + t.Errorf("expected job 10 to be marked addressed, got IDs: %v", ids) + } } From ba197df88335e0dd0552c95f61fb1784ade7ae09 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Thu, 26 Feb 2026 14:12:34 -0600 Subject: [PATCH 3/3] fix: unregister test agent to avoid polluting global registry The test-pass-skip agent registered in TestFixSingleJobSkipsPassVerdict leaked into TestSelectRefineAgentCodexFallback, causing it to find an agent when it expects none. Add t.Cleanup to unregister after the test. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/fix_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/roborev/fix_test.go b/cmd/roborev/fix_test.go index 4b8e1326..3e193b55 100644 --- a/cmd/roborev/fix_test.go +++ b/cmd/roborev/fix_test.go @@ -1399,6 +1399,7 @@ func TestFixSingleJobSkipsPassVerdict(t *testing.T) { return "", nil }, }) + t.Cleanup(func() { agent.Unregister("test-pass-skip") }) opts := fixOptions{agentName: "test-pass-skip"}