diff --git a/internal/reviewplan/reviewplan.go b/internal/reviewplan/reviewplan.go index 0d3a818..26e5dcb 100644 --- a/internal/reviewplan/reviewplan.go +++ b/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) diff --git a/internal/reviewplan/summary.go b/internal/reviewplan/summary.go index 46ad226..9b33eb3 100644 --- a/internal/reviewplan/summary.go +++ b/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": diff --git a/internal/reviewplan/summary_failed_reviewer_test.go b/internal/reviewplan/summary_failed_reviewer_test.go new file mode 100644 index 0000000..0ec91e8 --- /dev/null +++ b/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()) + } +} diff --git a/internal/view/review.go b/internal/view/review.go index b480387..aecb6e0 100644 --- a/internal/view/review.go +++ b/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{ diff --git a/internal/view/review_reviewer_ran_test.go b/internal/view/review_reviewer_ran_test.go new file mode 100644 index 0000000..389317c --- /dev/null +++ b/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) + } +}