fix(rollup): a reviewer that never ran must not report zero findings - #566
Conversation
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.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f085ea734988
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| harness-engineering:repo-health | 0 |
structure:repo-health (1 finding)
Major - internal/reviewplan/summary.go:243
Summary's doc comment states "the rendered rollup comment and dry-run JSON both consume this object, so they cannot disagree," but this fix only changes the markdown renderer. writeReviewerTable in summary.go now derives a "did not run" state from ReviewerCoverageSummary, but ReviewerSummary.Findings itself is left as a plain int (0) whether a reviewer genuinely found nothing or never ran. internal/view/review.go's newReviewSummary (dry-run/API JSON) copies reviewer.Findings straight through with no coverage awareness, so a failed reviewer still serializes as findings: 0 there — the exact ambiguity this PR sets out to fix, reintroduced in the other consumer of this same struct. Either derive the coverage-aware state once in reviewplan (e.g. expose it via ReviewerSummary or a shared helper) so both consumers stay in sync, or narrow the doc comment to acknowledge the markdown-only scope so a future reader doesn't assume JSON output already carries this fix.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: nonestructure:repo-health— complete (broad); skipped: none; constraints: Scope limited to the three assigned files; the corroborating JSON-consumer code (internal/view/review.go) was read for context but is outside this reviewer's assignment and not inspected/skipped-listed.harness-engineering:repo-health— complete (broad); skipped: none; constraints: none
Inspected files (3)
internal/reviewplan/reviewplan.gointernal/reviewplan/summary.gointernal/reviewplan/summary_failed_reviewer_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 55s | $1.82 | claude-sonnet-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 55s wall · 5m 11s compute |
| Cost | $1.82 |
| Tokens | 104 in / 19.8k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 2.5k
- Cache read: 66.8k
- Cache create: 17.8k
- Cost: $0.17
- Duration: 28s
go:implementation-tests— claude-sonnet-5- In: 22
- Out: 3.5k
- Cache read: 416.2k
- Cache create: 27.6k
- Cost: $0.34
- Duration: 50s
structure:repo-health— claude-sonnet-5- In: 38
- Out: 7.5k
- Cache read: 804.9k
- Cache create: 46.3k
- Cost: $0.63
- Duration: 1m 35s
harness-engineering:repo-health— claude-sonnet-5- In: 32
- Out: 5.4k
- Cache read: 619.9k
- Cache create: 34.2k
- Cost: $0.47
- Duration: 1m 43s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 788
- Cache read: 94.3k
- Cache create: 26.7k
- Cost: $0.20
- Duration: 33s
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f085ea734988
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 0 |
| harness-engineering:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - internal/reviewplan/summary.go:243
Summary's doc comment claims "the rendered rollup comment and dry-run JSON both consume this object, so they cannot disagree," but this fix only makes the markdown renderer (writeReviewerTable) coverage-aware. ReviewerSummary.Findings itself stays a plain int, so internal/view/review.go's newReviewSummary (the dry-run/API JSON path) copies reviewer.Findings straight through with no reference to coverage status — a failed reviewer still serializes as findings: 0 there, the exact zero/did-not-run ambiguity this PR sets out to fix, just in the other consumer of this struct. Derive the coverage-aware distinction once in reviewplan (e.g. add a Ran/DidNotRun field to ReviewerSummary or expose a shared helper both writeReviewerTable and newReviewSummary call) so the two consumers cannot drift, or at minimum narrow this doc comment to say it covers table rendering only so a future reader doesn't assume the JSON output already carries this fix.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonestructure:repo-health— complete (constrained); skipped: none; constraints: Head SHA is unchanged from the prior review round; the previously raised JSON/markdown consistency concern is already tracked as a settled inline thread on internal/reviewplan/summary.go, so it is not re-raised here.harness-engineering:repo-health— complete (constrained); skipped: none; constraints: The prior finding about ReviewerSummary/dry-run JSON not carrying the coverage-derived did-not-run state (internal/view/review.go's newReviewSummary) is already tracked as a settled inline thread and internal/view/review.go is outside this assignment's allowed_files, so it is not re-raised here.
Inspected files (3)
internal/reviewplan/reviewplan.gointernal/reviewplan/summary.gointernal/reviewplan/summary_failed_reviewer_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 19s | $1.53 | claude-sonnet-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 1m 19s wall · 1m 50s compute |
| Cost | $1.53 |
| Tokens | 40 in / 7.8k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 12
- Out: 3.4k
- Cache read: 291.0k
- Cache create: 41.2k
- Cost: $0.39
- Duration: 45s
structure:repo-health— claude-sonnet-5- In: 8
- Out: 1.4k
- Cache read: 230.6k
- Cache create: 55.3k
- Cost: $0.42
- Duration: 20s
harness-engineering:repo-health— claude-sonnet-5- In: 14
- Out: 2.5k
- Cache read: 415.1k
- Cache create: 56.2k
- Cost: $0.50
- Duration: 32s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 476
- Cache read: 102.4k
- Cache create: 30.2k
- Cost: $0.22
- Duration: 12s
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.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 51086247864f
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| harness-engineering:repo-health | 0 |
structure:repo-health (1 finding)
Minor - internal/reviewplan/summary.go:254
writeReviewerTable still builds its own local
producedmap by hand (lines 255-258) instead of calling the new exportedReviewersProducedResultshelper added a few lines below it in the same file. The helper's own doc comment says "Both the rendered rollup and the JSON view derive their 'did not run' state from this, so the two cannot disagree," but that guarantee only holds for the JSON view (internal/view/review.go), which does call it -- the markdown table computes the identical coverage->produced mapping independently. Two implementations of the same rule in one file can drift silently (e.g. if coverageResultProduced's status set changes and only one call site is updated). Have writeReviewerTable call reviewplan.ReviewersProducedResults(coverage) instead of re-deriving the map inline, so there is exactly one source of truth backing the stated contract.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonestructure:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):internal/reviewplan/reviewplan.go,internal/reviewplan/summary.go,internal/reviewplan/summary_failed_reviewer_test.go; skipped: none; constraints: internal/view/review.go and internal/view/review_reviewer_ran_test.go, which also changed in this revision, are outside this reviewer's assigned/allowed files and were read only for cross-file context, not inspected as part of this review.harness-engineering:repo-health— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):internal/reviewplan/reviewplan.go,internal/reviewplan/summary.go,internal/reviewplan/summary_failed_reviewer_test.go; skipped: none; constraints: none
Inspected files (5)
internal/reviewplan/reviewplan.gointernal/reviewplan/summary.gointernal/reviewplan/summary_failed_reviewer_test.gointernal/view/review.gointernal/view/review_reviewer_ran_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 51s | $2.51 | claude-sonnet-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health, harness-engineering:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 1m 51s wall · 3m 10s compute |
| Cost | $2.51 |
| Tokens | 70 in / 13.5k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 26
- Out: 4.7k
- Cache read: 944.8k
- Cache create: 69.3k
- Cost: $0.77
- Duration: 1m 04s
structure:repo-health— claude-sonnet-5- In: 22
- Out: 4.2k
- Cache read: 881.2k
- Cache create: 74.5k
- Cost: $0.77
- Duration: 58s
harness-engineering:repo-health— claude-sonnet-5- In: 16
- Out: 3.8k
- Cache read: 640.9k
- Cache create: 77.3k
- Cost: $0.71
- Duration: 51s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 803
- Cache read: 112.1k
- Cache create: 34.8k
- Cost: $0.25
- Duration: 15s
| // 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) { |
There was a problem hiding this comment.
writeReviewerTable still builds its own local produced map by hand (lines 255-258) instead of calling the new exported ReviewersProducedResults helper added a few lines below it in the same file. The helper's own doc comment says "Both the rendered rollup and the JSON view derive their 'did not run' state from this, so the two cannot disagree," but that guarantee only holds for the JSON view (internal/view/review.go), which does call it -- the markdown table computes the identical coverage->produced mapping independently. Two implementations of the same rule in one file can drift silently (e.g. if coverageResultProduced's status set changes and only one call site is updated). Have writeReviewerTable call reviewplan.ReviewersProducedResults(coverage) instead of re-deriving the map inline, so there is exactly one source of truth backing the stated contract.
Reply inline to this comment.
2 participants
The problem
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, this is what a run looked like where four of five reviewers failed to start:
Only
documentation:docsactually ran — it is the one agent that needs no repo checkout. The failures were recorded, but asincomplete_failedin a coverage section well below the headline table. The summary is the part that gets read, pasted, and acted on, and it said all-clear.I nearly merged on that reading, twice.
Scope
The outcome logic is already correct —
hasIncompleteReviewerCoveragecoerces approval away when coverage is incomplete, so the PR was never actually approved. This change is purely about what a human reads.The fix
A reviewer whose coverage status did not produce a result renders as
⚠️ did not runinstead of a count, reusing the existingcoverageResultProducedhelper.Reviewers absent from coverage keep their number, so an unknown status cannot turn a genuine zero into a phantom failure. That case has its own test.
Tests
Two tests, both confirmed to fail with the change reverted:
0, while a genuinely clean reviewer keeps its honest0in the same tableFull suite green.
Related
#565 fixes the cause of that particular fleet failure (the workbench has no
refs/, so per-reviewer clones fail). This one fixes the reporting, which matters for any reviewer failure regardless of cause — the two are independent.