fix(rollup): a reviewer that never ran must not report zero findings

internal/reviewplan/reviewplan.go

@@ -821,7 +821,7 @@ func (b *builder) renderRollup(ordered []review.Finding, anchored []AnchoredFind
 	var out strings.Builder
 	rollupHeader(&out, b.req)
 	if len(summary.Reviewers) > 0 {
-		writeReviewerTable(&out, summary.Reviewers)
+		writeReviewerTable(&out, summary.Reviewers, summary.Run.ReviewerCoverage)
 		b.writeReviewerSections(&out, anchored, summary.Reviewers)
 		writeReviewerCoverageDiagnostics(&out, summary.Run.ReviewerCoverage)
 		writeReviewerFailureDiagnostics(&out, summary.Run.ReviewerFailures)

internal/reviewplan/summary.go

@@ -243,10 +243,29 @@ func sumDurations(workstreams []WorkstreamUsage, field func(WorkstreamUsage) *in
 	return &total
 }
 
-func writeReviewerTable(out *strings.Builder, reviewers []ReviewerSummary) {
+// writeReviewerTable renders the headline per-reviewer counts.
+//
+// A reviewer that never produced a result must not be shown as "0". Zero
+// findings and "did not run" are the same number and opposite meanings: the
+// first says the code is clean, the second says nothing was examined. Rendering
+// both as 0 let a run where four of five reviewers failed to start read as a
+// clean review, with the failure visible only further down in the coverage
+// section that a reader skimming the summary never reaches.
+func writeReviewerTable(out *strings.Builder, reviewers []ReviewerSummary, coverage []ReviewerCoverageSummary) {
+	produced := make(map[string]bool, len(coverage))
+	for _, entry := range coverage {
+		produced[entry.AgentID] = coverageResultProduced(entry.Status)
+	}
 	out.WriteString("| Reviewer | Findings |\n")
 	out.WriteString("|----------|----------|\n")
 	for _, reviewer := range reviewers {
+		// Absent from coverage means nothing was reported either way; only an
+		// explicit non-producing status is called out, so this cannot mask a
+		// genuine zero.
+		if ran, known := produced[reviewer.Name]; known && !ran {
+			fmt.Fprintf(out, "| %s | ⚠️ did not run |\n", escapeCell(reviewer.Name))
+			continue
+		}
 		fmt.Fprintf(out, "| %s | %d |\n", escapeCell(reviewer.Name), reviewer.Findings)
 	}
 	out.WriteString("\n")
@@ -316,6 +335,18 @@ func writeReviewerCoverageDiagnostics(out *strings.Builder, coverage []ReviewerC
 	out.WriteString("\n")
 }
 
+// ReviewersProducedResults maps each reviewer to whether it actually produced
+// a result. Both the rendered rollup and the JSON view derive their
+// "did not run" state from this, so the two cannot disagree -- which is what
+// Summary's contract promises and what a markdown-only fix would have broken.
+func ReviewersProducedResults(coverage []ReviewerCoverageSummary) map[string]bool {
+	produced := make(map[string]bool, len(coverage))
+	for _, entry := range coverage {
+		produced[entry.AgentID] = coverageResultProduced(entry.Status)
+	}
+	return produced
+}
+
 func coverageResultProduced(status string) bool {
 	switch strings.TrimSpace(status) {
 	case "complete_broad", "complete_constrained", "incomplete_skipped":

internal/reviewplan/summary_failed_reviewer_test.go

@@ -0,0 +1,57 @@
+package reviewplan
+
+import (
+	"strings"
+	"testing"
+)
+
+// A reviewer that never produced a result must not appear as "0".
+//
+// Zero findings and "did not run" are the same number and opposite meanings:
+// one says the code is clean, the other says nothing was examined. Rendering
+// both as 0 let a run where most reviewers failed to start read as a clean
+// review, with the failure visible only in a coverage section further down.
+func TestReviewerTableDistinguishesFailureFromZeroFindings(t *testing.T) {
+	reviewers := []ReviewerSummary{
+		{Name: "security:code-auditor", Findings: 0}, // failed to start
+		{Name: "documentation:docs", Findings: 0},    // genuinely found nothing
+	}
+	coverage := []ReviewerCoverageSummary{
+		{AgentID: "security:code-auditor", Status: "incomplete_failed"},
+		{AgentID: "documentation:docs", Status: "complete_broad"},
+	}
+
+	var out strings.Builder
+	writeReviewerTable(&out, reviewers, coverage)
+	got := out.String()
+
+	for _, line := range strings.Split(got, "\n") {
+		if !strings.Contains(line, "security:code-auditor") {
+			continue
+		}
+		if strings.Contains(line, "| 0 |") {
+			t.Fatalf("a reviewer that did not run is reported as zero findings: %q", line)
+		}
+		if !strings.Contains(line, "did not run") {
+			t.Fatalf("failed reviewer row does not say it did not run: %q", line)
+		}
+	}
+
+	// The reviewer that really did run must still show its honest zero.
+	if !strings.Contains(got, "| documentation:docs | 0 |") {
+		t.Fatalf("a completed reviewer lost its zero count:\n%s", got)
+	}
+}
+
+// A reviewer absent from coverage keeps its count: unknown status must not be
+// reported as a failure, or genuine zeros start reading as breakage.
+func TestReviewerTableKeepsCountWhenCoverageIsUnknown(t *testing.T) {
+	var out strings.Builder
+	writeReviewerTable(&out,
+		[]ReviewerSummary{{Name: "policies:conventions", Findings: 0}},
+		nil,
+	)
+	if !strings.Contains(out.String(), "| policies:conventions | 0 |") {
+		t.Fatalf("unknown coverage should leave the count alone:\n%s", out.String())
+	}
+}

internal/view/review.go

@@ -35,6 +35,11 @@ type ReviewSummary struct {
 type ReviewReviewerSummary struct {
 	Name     string `json:"name"`
 	Findings int    `json:"findings"`
+	// Ran is false when the reviewer produced no result. Without it a failed
+	// reviewer serializes as findings: 0, which a consumer cannot tell from a
+	// genuinely clean one. Omitted when coverage says nothing either way, so an
+	// unknown status is not reported as a failure.
+	Ran *bool `json:"ran,omitempty"`
 }
 
 // ReviewReviewerCoverageSummary describes reviewer coverage rendered in the
@@ -253,8 +258,13 @@ func newReviewSummary(summary reviewplan.Summary) ReviewSummary {
 			ComputeDurationMS: summary.Totals.ComputeDurationMS,
 		},
 	}
+	produced := reviewplan.ReviewersProducedResults(summary.Run.ReviewerCoverage)
 	for _, reviewer := range summary.Reviewers {
-		out.Reviewers = append(out.Reviewers, ReviewReviewerSummary{Name: reviewer.Name, Findings: reviewer.Findings})
+		row := ReviewReviewerSummary{Name: reviewer.Name, Findings: reviewer.Findings}
+		if ran, known := produced[reviewer.Name]; known {
+			row.Ran = &ran
+		}
+		out.Reviewers = append(out.Reviewers, row)
 	}
 	for _, coverage := range summary.Run.ReviewerCoverage {
 		out.Run.ReviewerCoverage = append(out.Run.ReviewerCoverage, ReviewReviewerCoverageSummary{

internal/view/review_reviewer_ran_test.go

@@ -0,0 +1,47 @@
+package view
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"github.com/open-cli-collective/codereview-cli/internal/reviewplan"
+)
+
+// The JSON view must carry the same did-not-run distinction as the rendered
+// rollup. Summary's contract is that both consumers agree; a markdown-only fix
+// would leave a failed reviewer serializing as findings: 0, which is the exact
+// ambiguity being removed.
+func TestReviewSummaryJSONDistinguishesFailedReviewer(t *testing.T) {
+	summary := reviewplan.Summary{
+		Reviewers: []reviewplan.ReviewerSummary{
+			{Name: "security:code-auditor", Findings: 0},
+			{Name: "documentation:docs", Findings: 0},
+			{Name: "policies:conventions", Findings: 0},
+		},
+		Run: reviewplan.RunSummary{
+			ReviewerCoverage: []reviewplan.ReviewerCoverageSummary{
+				{AgentID: "security:code-auditor", Status: "incomplete_failed"},
+				{AgentID: "documentation:docs", Status: "complete_broad"},
+				// policies:conventions absent: status unknown.
+			},
+		},
+	}
+
+	raw, err := json.Marshal(newReviewSummary(summary))
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+	got := string(raw)
+
+	if !strings.Contains(got, `"name":"security:code-auditor","findings":0,"ran":false`) {
+		t.Fatalf("failed reviewer must serialize ran:false, got:\n%s", got)
+	}
+	if !strings.Contains(got, `"name":"documentation:docs","findings":0,"ran":true`) {
+		t.Fatalf("completed reviewer must serialize ran:true, got:\n%s", got)
+	}
+	// Unknown coverage omits the field rather than guessing a failure.
+	if !strings.Contains(got, `"name":"policies:conventions","findings":0}`) {
+		t.Fatalf("unknown coverage must omit ran, got:\n%s", got)
+	}
+}