fix: skip PASS-verdict reviews in roborev fix

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,15 @@ 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)
+			}
+			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})
 	}
 

cmd/roborev/fix_test.go

@@ -11,7 +11,9 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"slices"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -1292,3 +1294,225 @@ 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
+		},
+	})
+	t.Cleanup(func() { agent.Unregister("test-pass-skip") })
+
+	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 mu sync.Mutex
+	var addressedJobIDs []int64
+
+	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) {
+			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) {
+			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)
+	}
+
+	// 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)
+	}
+}