From f085ea734988281f496718553d451929e0534cd9 Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 13 Aug 2026 14:42:02 -0400 Subject: [PATCH 1/2] fix(rollup): a reviewer that never ran must not report zero findings Zero findings and "did not run" are the same number and opposite meanings. One says the code was examined and is clean; the other says nothing was examined. The summary table rendered both as 0. On a real PR that made a run where four of five reviewers failed to start read as a clean review: | security:code-auditor | 0 | | rust:implementation-tests | 0 | | architecture:solid | 0 | | policies:conventions | 0 | | documentation:docs | 1 | Only the last one had actually run. The failures were recorded, but in a coverage section far below the headline table, so anyone reading the summary -- which is the part that gets pasted into a decision -- saw an all-clear. The outcome logic was already correct: hasIncompleteReviewerCoverage coerces approval away when coverage is incomplete, so the PR was never approved. This is purely about what a human reads and acts on. A non-producing coverage status now renders as "did not run" instead of a count. Reviewers absent from coverage keep their number, so an unknown status cannot turn a genuine zero into a phantom failure -- covered by its own test. Both tests were confirmed to fail with the change reverted. --- internal/reviewplan/reviewplan.go | 2 +- internal/reviewplan/summary.go | 21 ++++++- .../summary_failed_reviewer_test.go | 57 +++++++++++++++++++ 3 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 internal/reviewplan/summary_failed_reviewer_test.go diff --git a/internal/reviewplan/reviewplan.go b/internal/reviewplan/reviewplan.go index 0d3a8185..26e5dcb6 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 46ad226d..aa7c8f33 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") diff --git a/internal/reviewplan/summary_failed_reviewer_test.go b/internal/reviewplan/summary_failed_reviewer_test.go new file mode 100644 index 00000000..0ec91e85 --- /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()) + } +} From 51086247864ff16e162eba217493f10ffd677e2b Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 13 Aug 2026 15:26:36 -0400 Subject: [PATCH 2/2] fix(view): carry the did-not-run distinction into the JSON summary too Review caught that the previous commit fixed only the markdown renderer, while Summary's own contract says the rollup comment and the JSON view "both consume this object, so they cannot disagree". They disagreed: newReviewSummary copied Findings straight through, so a reviewer that never ran still serialized as findings: 0 -- the exact ambiguity this change exists to remove, reintroduced in the other consumer. The state is now derived once, by reviewplan.ReviewersProducedResults, and read by both. The JSON gains ran, omitted when coverage says nothing either way so an unknown status is not reported as a failure. Test asserts all three cases in one payload -- failed, completed, unknown -- and was confirmed to fail with the mapping reverted. --- internal/reviewplan/summary.go | 12 ++++++ internal/view/review.go | 12 +++++- internal/view/review_reviewer_ran_test.go | 47 +++++++++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 internal/view/review_reviewer_ran_test.go diff --git a/internal/reviewplan/summary.go b/internal/reviewplan/summary.go index aa7c8f33..9b33eb3b 100644 --- a/internal/reviewplan/summary.go +++ b/internal/reviewplan/summary.go @@ -335,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/view/review.go b/internal/view/review.go index b480387a..aecb6e06 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 00000000..389317c3 --- /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) + } +}