fix(reviewplan): state why a clean review withheld approval by piekstra · Pull Request #574 · open-cli-collective/codereview-cli

piekstra · 2026-08-26T12:21:37Z

Closes #573.

Problem

A review where every reviewer returns zero findings but one reports files in skipped_files is downgraded from APPROVE to COMMENT by hasIncompleteReviewerCoverage. The coercion is right. The silence around it is not.

The posted rollup renders a summary table of zeros and then the ordinary ### Reviewer Coverage list. A review that approved and a review that found nothing but could not approve look identical unless the reader reaches the coverage list, spots one ⚠️ incomplete (skipped files) row among the complete ones, and already knows that incomplete coverage downgrades the event.

Re-running is not a remedy and nothing says so. A reviewer that declined a file declines it again, so the state is deterministic, and the next cr review short-circuits with review already complete. The operator is left with an unapprovable run, no stated cause, and no next step.

This is the third instance of the same shape. Generated lockfiles (#567) and deleted files each got a narrow exemption after a real PR stalled on it; both were cases where cr created a coverage obligation a reviewer was always going to decline, and both presented as a clean review that would not approve and would not say why.

Change

An Approval Withheld section, rendered directly under the findings table whenever that coercion fired. It reports whatever evidence the run produced: each reviewer that returned no result with its error, each reviewer whose coverage status was not complete, and every changed file no reviewer inspected.

### Summary

| Reviewer | Findings |
|----------|----------|
| structure:repo-health | 0 |

### Approval Withheld

No blocking or major findings were reported. Approval is withheld because this run did not cover the change:

- `structure:repo-health` — ⚠️ incomplete (skipped files)
- 2 files inspected by no reviewer:
  - `scripts/run.sh`
  - `tests/big_suite_test.py`

Re-running the same review reproduces this: a reviewer that declined a file declines it again. Closing the gap means bringing these paths into the remit of a reviewer that will read them, or establishing that they need no review.

Three things make it consistent with the gate rather than a second opinion about coverage:

One classification of the status enum. coverageStatusComplete is extracted next to coverageResultProduced, and hasIncompleteReviewerCoverage now calls it instead of enumerating statuses itself. A status neither knows fails toward withholding approval and toward being explained, rather than one of each.

Rendering is the caller's decision. The section is invoked exactly when the coercion fired, so it never re-derives whether coverage was incomplete. Deciding again from the evidence is what let the two disagree in the states most in need of an explanation.

Obligation is scope, not skips. uninspectedFiles reads each entry's Scope, unioned with SkippedFiles so the scope-less unassigned pseudo-entry still contributes, then subtracts everything any reviewer inspected. A reviewer that crashed carries a scope and no file lists at all; one that omitted a file from both lists carries the paths only in its diagnostic. Reading skips alone dropped exactly those. The subtraction runs across reviewers because coverage is an obligation on the review rather than on each reviewer separately.

The gate is evaluated one reviewer at a time while that list is computed across the review, so a reviewer can withhold approval over a file another reviewer read. In that state nothing is unread, and the section says so outright instead of introducing a list and listing nothing:

- `structure:repo-health` — ⚠️ incomplete (skipped files); skipped: `f.go`

Every changed file was read by some reviewer. The withhold comes from the per-reviewer statuses above, which are evaluated one reviewer at a time.

Placement is under the counts rather than above them: the table of zeros is the thing that misleads, so the explanation has to be the next thing read.

Scope

Reporting only. The gate's decision, the event mapping, and the lockfile and deleted-file exemptions are unchanged. The same reviews approve and the same reviews are withheld.

Two things this deliberately does not do:

Make the gate per review. Whether a reviewer's declined file that another reviewer read should withhold approval is a real question and a behavior change. The section names the asymmetry rather than resolving it.
Split skipped_files. The prompt contract specifies it as "assigned changed files you intentionally did not inspect or could not inspect", conflating "outside my remit" with "I could not get to it". That conflation is arguably the root of the recurring exemptions, and separating it is a larger call.

Verification

TestUninspectedFiles and TestRollupApprovalWithheld in internal/reviewplan/summary_approval_withheld_test.go, 11 cases:

a file another reviewer inspected is not reported as unread
a file every reviewer skipped is reported; unassigned paths included, duplicates collapsed
assigned files reported in neither list are reported (the missing path, where skips are empty)
a failed reviewer's whole scope is reported, minus what another reviewer read
the section names the unread file and carries the re-run note
a status whose only evidence is a diagnostic is explained
a failed reviewer's assigned files are named, and the crash is not reported twice inside the section
every-file-read still carries a reviewer line and the explanatory sentence, with no unread claim and no bare colon
an unknown status both withholds and is explained
the rollup shape with no reviewer table renders the section
an approving review and a request-changes review both render without it

The tests gate behavior, not compilation: reverting uninspectedFiles to skips-only fails the neither-list and failed-reviewer cases, and removing the render call fails the rollup cases on the missing heading.

make lint reports 0 issues; make tidy is a no-op.

Review rounds

Three cr rounds on this PR found five real defects, each fixed and each now covered:

The section decided "did this run cover the change?" from different evidence than the coercion, so incomplete_skipped via missing and incomplete_tool withheld approval and rendered nothing.
uninspectedFiles read skip lists only, so a failed reviewer's uniquely assigned files vanished.
The section could assert a cause and list nothing, in the ordinary overlapping-scope case.
incompleteCoverageLines re-listed the complete statuses the gate enumerates, with opposite defaults on an unknown one.
The no-reviewer-table rollup branch is reachable with the coercion fired and was untested.

static-smoke failed once on TestPiRPCReviewerHelperStaysInParentProcessGroup in internal/llmadapters, a package this PR does not touch; earlier runs on the same branch passed, and #500 already tracks flakiness there.

A review where every reviewer returns zero findings but one reports skipped files is downgraded from APPROVE to COMMENT by hasIncompleteReviewerCoverage. The coercion is correct; the silence is not. The rollup renders a table of zeros and the ordinary coverage list, so a review that approved and a review that could not approve look the same unless the reader reaches the coverage list, spots one incomplete entry among the complete ones, and already knows that incomplete coverage downgrades the event. Re-running does not help and nothing says so: a reviewer that declined a file declines it again, and the next cr review short-circuits with 'review already complete'. Adds an Approval Withheld section under the findings table, rendered only when that coercion fired. It names each reviewer that produced no result and every changed file no reviewer inspected, and says that another pass reproduces the same state. The file list subtracts anything another reviewer did inspect. Coverage is an obligation on the review rather than on each reviewer separately, so a file one reviewer declined and another read is covered. Reporting only. The same reviews approve and the same reviews are withheld; the lockfile and deleted-file exemptions are untouched. Closes #573

monit-reviewer

Automated PR Review

Reviewed commit: 2d2f1902fc1a
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	1
policies:conventions	0
architecture:solid-reviewer-agnostic	1

go:implementation-tests (1 finding)

Major - `internal/reviewplan/summary.go:395`

uninspectedFiles only reads entry.SkippedFiles, but internal/pipeline/pipeline.go's buildReviewerCoverage never populates SkippedFiles for a reviewer that failed outright (reviewerCoverageIncompleteFailed): it sets only AgentID, Scope, Status, and Diagnostic, then appends before touching SkippedFiles/InspectedFiles. So when a reviewer crashes or times out, the files that were uniquely assigned to it (its Scope) are dropped from the 'inspected by no reviewer' list entirely, even though nobody read them. The rendered section will name the failed reviewer's error but silently omit exactly the files that failure left uncovered, contradicting the PR's stated goal of listing 'every changed file no reviewer inspected'. The new test 'a reviewer failure is named even with no unread files' does not catch this because it constructs RunSummary by hand and never gives the failed reviewer a ReviewerCoverage entry with a Scope, which is not how the real pipeline populates it (buildReviewerCoverage always appends a coverage entry for a failed reviewer, carrying Scope). Fix: uninspectedFiles should also treat a coverage entry with Status == "incomplete_failed" as having skipped its entire Scope (falling back to entry.Scope when SkippedFiles is empty), and a test should build the coverage list the way buildReviewerCoverage actually does (failed reviewer entry has Scope but no SkippedFiles) to prove the failure's assigned files show up as unread.

structure:repo-health (1 finding)

Minor - `internal/reviewplan/reviewplan.go:830`

The withheld() closure is also invoked in the len(summary.Reviewers) == 0 branch (severity-count table path), so approvalWithheld=true with an empty summary.Reviewers is a reachable state: hasIncompleteReviewerCoverage reads b.req.RunSummary.ReviewerCoverage directly, independent of SelectedReviewers/FindingReviewers, which is what deriveSummary gates summary.Reviewers on. None of the four new cases in TestRollupApprovalWithheld populate that branch — all set SelectedReviewers, forcing the reviewer-table path. If that combination is actually unreachable given how RunSummary is populated elsewhere, a short comment on the guard would save the next reader from re-deriving it; if it is reachable, add a case asserting the section still renders correctly in the no-reviewer-table rollup shape.

architecture:solid-reviewer-agnostic (1 finding)

Major - `internal/reviewplan/summary.go:422`

The section decides "did this run cover the change?" from a different signal than the coercion that made the question necessary, so the two disagree and the explanation disappears in exactly the case the PR is fixing (U-O1; also U-S1 — the definition of incomplete coverage now lives in two modules).

buildReview withholds approval on hasIncompleteReviewerCoverage (reviewplan.go:339), which keys off coverage status: incomplete_skipped, incomplete_failed, incomplete_unassigned, and any unknown status including incomplete_tool. writeApprovalWithheld instead gates on len(failures) == 0 && len(uninspected) == 0, where uninspected comes only from SkippedFiles. Two producible states satisfy the first and not the second:

incomplete_skipped reached via missing rather than skips. pipeline.go:2576-2582 sets that status when assigned files were neither inspected nor skipped; SkippedFiles stays empty and the paths live only in Diagnostic. The reviewer returned a result, so ReviewerFailures is empty too. Approval is withheld, and the rollup renders a table of zeros with no Approval Withheld section — the original failure mode, for the variant most likely to occur, since a reviewer omitting a file from both lists is exactly what that status exists to catch.

incomplete_tool (pipeline.go:2570-2574) with an empty SkippedFiles intersection. Coercion fires through the conservative default branch; the cause is in Diagnostic and is never rendered.

Suggested fix: derive the section from the same evidence the coercion uses instead of from an independent one. Concretely, have uninspectedFiles compute Scope minus the inspected union (the Scope field is already on ReviewerCoverageSummary, summary.go:41), which subsumes both skipped and missing files; and make the section render whenever approvalWithheld is true, listing each coverage entry whose status caused it, with its coverageStatusLabel and Diagnostic as the reason and unread files as one reason among several rather than the gate. That way a new incomplete status added to hasIncompleteReviewerCoverage cannot silently produce a withheld review with no stated cause.

Reviewer Coverage

go:implementation-tests — complete (broad); inspected 3 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go, internal/reviewplan/summary_approval_withheld_test.go; skipped: none; constraints: none
structure:repo-health — complete (broad); inspected 3 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: none
policies:conventions — complete (broad); inspected 2 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/summary.go; skipped: none; constraints: none
architecture:solid-reviewer-agnostic — complete (broad); inspected 2 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: Scope limited to the two assigned files; internal/pipeline/pipeline.go and the new test file were read only as context for how coverage entries are produced.

Inspected files (4)

docs/checkout-native-review-contract.md
internal/reviewplan/reviewplan.go
internal/reviewplan/summary.go
internal/reviewplan/summary_approval_withheld_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 3m 57s | ~$4.58 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.291

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, policies:conventions, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	3m 57s wall · 7m 26s compute
Cost	~$4.58 (est.)
Tokens	88 in / 24.8k out

Per-workstream usage

orchestrator-selection — claude-sonnet-5
- In: 6
- Out: 4.3k
- Cache read: 117.3k
- Cache create: 98.1k
- Cost: ~$0.47 (est.)
- Duration: 47s
go:implementation-tests — claude-sonnet-5
- In: 24
- Out: 6.0k
- Cache read: 883.5k
- Cache create: 137.9k
- Cost: ~$0.87 (est.)
- Duration: 1m 49s
structure:repo-health — claude-sonnet-5
- In: 16
- Out: 4.1k
- Cache read: 512.7k
- Cache create: 133.6k
- Cost: ~$0.72 (est.)
- Duration: 1m 13s
policies:conventions — claude-sonnet-5
- In: 12
- Out: 1.9k
- Cache read: 327.2k
- Cache create: 125.2k
- Cost: ~$0.60 (est.)
- Duration: 38s
architecture:solid-reviewer-agnostic — claude-opus-5
- In: 24
- Out: 8.0k
- Cache read: 792.1k
- Cache create: 132.1k
- Cost: ~$1.42 (est.)
- Duration: 2m 37s
orchestrator-rollup — claude-sonnet-5
- In: 6
- Out: 641
- Cache read: 128.0k
- Cache create: 122.1k
- Cost: ~$0.51 (est.)
- Duration: 20s

Review feedback found the section deciding 'did this run cover the change?' from different evidence than the coercion that made the question necessary, so the two disagreed in exactly the cases most in need of an explanation. The gate keys off coverage status. The section keyed off a non-empty failure list or skip list, which two producible states do not satisfy: incomplete_skipped reached through assigned files reported in neither list, where the paths live only in the diagnostic, and incomplete_tool, whose cause is only a diagnostic. Both withheld approval and rendered nothing, reproducing the original failure. The section now renders whenever the coercion fired and reports whatever evidence exists, so it has no opinion of its own about coverage. uninspectedFiles also missed a failed reviewer's files. buildReviewerCoverage appends an incomplete_failed entry carrying scope and no file lists at all, so reading skips alone dropped exactly the files that failure left unread. Obligation is now the reviewer's scope, with the unassigned pseudo-entry's skips folded in since it carries no scope. Adds cases for assigned-files-in-neither-list, a failed reviewer's scope, a diagnostic-only status, and the rollup shape with no reviewer table, which is reachable because summary.Reviewers derives from SelectedReviewers while the coercion reads ReviewerCoverage. Closes #573

monit-reviewer

Automated PR Review

Reviewed commit: 9038f3a59dab
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	0
policies:conventions	0
architecture:solid-reviewer-agnostic	2

architecture:solid-reviewer-agnostic (2 findings)

Major - `internal/reviewplan/summary.go:458`

The section can assert a cause it then lists no evidence for, and the state that produces it is the ordinary overlapping-scope case this PR's subtraction was written for (U-L1: the function's documented job is to say why approval was withheld, and in this state it says only that it was).

The gate is per reviewer (hasIncompleteReviewerCoverage, reviewplan.go:339, keyed on status) while the evidence is per review (uninspectedFiles subtracts anything any reviewer inspected). When reviewer A skips a file reviewer B read, A's status is incomplete_skipped, so the coercion fires, but there is no failure, no uninspected file, and no diagnostic — pipeline.go:2578-2582 sets Diagnostic only when missing is non-empty, so a plain skip carries none. Verified against this head with a throwaway test (removed afterwards; git status clean) on coverage [{a, incomplete_skipped, scope [f.go], skipped [f.go]}, {b, complete_broad, inspected [f.go]}], no failures:
### Approval Withheld

No blocking or major findings were reported. Approval is withheld because this run did not cover the change:


Re-running the same review reproduces this: ... Closing the gap means bringing these paths into the remit of a reviewer that will read them, ...
A colon introducing nothing, a claim that the run did not cover the change when every changed file was in fact read, and a closing instruction about paths that were never named. An operator lands in the same place as before the PR, now with a contradiction to resolve first. Overlapping assignment plus one reviewer declining a file another reads is common enough that this is not a corner case, and no test in summary_approval_withheld_test.go covers it (U-T1).

Suggested fix, keeping the always-render invariant that resolved the earlier finding: make the evidence total, not the section, the thing that varies. When failures, diagnostics, and uninspected files are all empty, emit a line per coverage entry whose status is not complete — - + codeSpan(AgentID) + — + coverageStatusLabel(status) — so the reader sees which reviewer's status withheld the event, and soften the lead-in for that case (the run's obligation was not discharged by every reviewer, rather than the change was not covered). Reporting-only, no change to the gate.

Minor - `internal/reviewplan/summary.go:429`

incompleteCoverageDiagnostics re-lists the complete statuses ("", complete_broad, complete_constrained) that hasIncompleteReviewerCoverage (reviewplan.go:342) already enumerates, so adding a status means editing both, and the two failure modes differ: the gate defaults to withholding on an unknown status while this switch defaults to printing its diagnostic, so a drift shows up as a withheld review whose explanation is silent about the reviewer that caused it (U-O1: the next case should be one edit, not two).

Suggested fix: extract func coverageStatusComplete(status string) bool next to the existing coverageResultProduced (summary.go:353) and have both call sites use it, so the classification lives in one place and the conservative default is shared.

Reviewer Coverage

go:implementation-tests — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go, internal/reviewplan/summary_approval_withheld_test.go; skipped: none; constraints: none
structure:repo-health — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: none
policies:conventions — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/summary.go; skipped: none; constraints: none
architecture:solid-reviewer-agnostic — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: Scope limited to the two assigned files; internal/pipeline/pipeline.go (buildReviewerCoverage) and summary_approval_withheld_test.go were read only as context for how coverage entries are produced and what is covered by tests. The prior finding on this PR (section could render nothing while approval was withheld) is resolved at 9038f3a; this review covers the new revision.

Inspected files (4)

docs/checkout-native-review-contract.md
internal/reviewplan/reviewplan.go
internal/reviewplan/summary.go
internal/reviewplan/summary_approval_withheld_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 57s | ~$4.83 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.291

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, policies:conventions, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	2m 57s wall · 5m 43s compute
Cost	~$4.83 (est.)
Tokens	64 in / 20.9k out

Per-workstream usage

go:implementation-tests — claude-sonnet-5
- In: 10
- Out: 6.2k
- Cache read: 346.4k
- Cache create: 188.4k
- Cost: ~$0.90 (est.)
- Duration: 1m 23s
structure:repo-health — claude-sonnet-5
- In: 22
- Out: 4.6k
- Cache read: 982.6k
- Cache create: 181.3k
- Cost: ~$1.04 (est.)
- Duration: 1m 25s
policies:conventions — claude-sonnet-5
- In: 8
- Out: 1.1k
- Cache read: 194.5k
- Cache create: 150.2k
- Cost: ~$0.64 (est.)
- Duration: 27s
architecture:solid-reviewer-agnostic — claude-opus-5
- In: 18
- Out: 7.2k
- Cache read: 737.6k
- Cache create: 179.1k
- Cost: ~$1.67 (est.)
- Duration: 1m 58s
orchestrator-rollup — claude-sonnet-5
- In: 6
- Out: 1.9k
- Cache read: 135.1k
- Cache create: 137.3k
- Cost: ~$0.58 (est.)
- Duration: 29s

…cation Two more from review. The section could assert a cause and then list nothing. When reviewer A skips a file reviewer B read, A's status is incomplete_skipped so the gate fires, but there is no failure, no unread file, and no diagnostic: a plain skip records one only when files were missing from both lists. The rollup rendered a heading, a colon, and a blank. Every reviewer with a non-complete status now gets a line, and when nothing went unread the section says that outright instead of introducing an empty list. The underlying asymmetry is that the gate is per reviewer while the file list is per review; that is now stated rather than papered over. incompleteCoverageLines also re-listed the complete statuses that hasIncompleteReviewerCoverage enumerates, so a new status meant two edits with opposite defaults: the gate withheld on an unknown status while the explanation stayed silent about it. Both now call coverageStatusComplete, which treats unknown as incomplete. A reviewer's skipped paths are spelled out on its own line only where the unread-file list does not already carry them, so a short block does not print the same path twice. Closes #573

monit-reviewer

Automated PR Review

Reviewed commit: 85b3f3b44fc3
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	0
policies:conventions	0
architecture:solid-reviewer-agnostic	1

architecture:solid-reviewer-agnostic (1 finding)

Minor - `internal/reviewplan/summary.go:515`

The len(failures) > 0 half of this condition sends a reviewer crash down the decline wording, which states a cause that is not the one that fired and a remedy the operator should not follow (U-L1: the section's job is to state why, and here it states the wrong why).

When a reviewer failed and every file in its scope was read by another reviewer, uninspected is empty but failures is not, so the branch is skipped. Rendered against this head (throwaway test, removed; git status clean) with coverage [{a, incomplete_failed, scope [f.go]}, {b, complete_broad, inspected [f.go]}] and failure a: reviewer process exited unexpectedly:
- `a` did not produce a result: reviewer process exited unexpectedly

Re-running the same review reproduces this: a reviewer that declined a file declines it again. Closing the gap means bringing these paths into the remit of a reviewer that will read them, or establishing that they need no review.
"These paths" names nothing, since no path was listed. The determinism claim is about declines and does not hold for a crash: a reviewer that exited or timed out may well succeed on a re-run, so the one action that could clear the run is the action the text tells the operator not to bother with. The lead-in ("this run did not cover the change") is also false here, which is the state the new sentence at line 520 was written for.

Suggested fix: condition the closing sentence on what actually happened rather than on the file list alone. Gate the decline paragraph on len(uninspected) > 0, keep the line 520 sentence for the no-unread-files case, and when failures are present add a separate sentence for them (a reviewer that did not produce a result may or may not on another pass; re-running is worth one attempt, and cr review short-circuits with review already complete unless forced). No test in summary_approval_withheld_test.go covers failure-with-no-unread-files (a failed reviewer's assigned files are named as unread asserts the opposite case), so the fix should carry one (U-T1).

Reviewer Coverage

go:implementation-tests — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go, internal/reviewplan/summary_approval_withheld_test.go; skipped: none; constraints: none
structure:repo-health — complete (constrained); inspected 3 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: none
policies:conventions — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): docs/checkout-native-review-contract.md, internal/reviewplan/summary.go; skipped: none; constraints: none
architecture:solid-reviewer-agnostic — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): internal/reviewplan/reviewplan.go, internal/reviewplan/summary.go; skipped: none; constraints: Both earlier findings on this PR are resolved at 85b3f3b: the section now always carries at least one bullet, and coverageStatusComplete is the single status classification shared by the gate and the rollup. Scope limited to the two assigned files; summary_approval_withheld_test.go and internal/pipeline/pipeline.go (buildReviewerCoverage) were read only as context for coverage-entry shape and test coverage.

Inspected files (4)

docs/checkout-native-review-contract.md
internal/reviewplan/reviewplan.go
internal/reviewplan/summary.go
internal/reviewplan/summary_approval_withheld_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 46s | ~$5.87 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.291

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, policies:conventions, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	2m 46s wall · 5m 14s compute
Cost	~$5.87 (est.)
Tokens	68 in / 16.0k out

Per-workstream usage

go:implementation-tests — claude-sonnet-5
- In: 20
- Out: 4.3k
- Cache read: 1.1M
- Cache create: 234.6k
- Cost: ~$1.27 (est.)
- Duration: 1m 12s
structure:repo-health — claude-sonnet-5
- In: 18
- Out: 4.4k
- Cache read: 936.0k
- Cache create: 229.0k
- Cost: ~$1.21 (est.)
- Duration: 1m 12s
policies:conventions — claude-sonnet-5
- In: 10
- Out: 1.3k
- Cache read: 328.2k
- Cache create: 175.9k
- Cost: ~$0.78 (est.)
- Duration: 29s
architecture:solid-reviewer-agnostic — claude-opus-5
- In: 14
- Out: 5.1k
- Cache read: 641.2k
- Cache create: 238.1k
- Cost: ~$1.94 (est.)
- Duration: 1m 39s
orchestrator-rollup — claude-sonnet-5
- In: 6
- Out: 807
- Cache read: 149.2k
- Cache create: 165.0k
- Cost: ~$0.68 (est.)
- Duration: 41s

monit-reviewer · 2026-08-26T12:50:05Z

+		}
+	}
+	out.WriteString("\n")
+	if len(uninspected) == 0 && len(failures) == 0 {


The len(failures) > 0 half of this condition sends a reviewer crash down the decline wording, which states a cause that is not the one that fired and a remedy the operator should not follow (U-L1: the section's job is to state why, and here it states the wrong why).

When a reviewer failed and every file in its scope was read by another reviewer, uninspected is empty but failures is not, so the branch is skipped. Rendered against this head (throwaway test, removed; git status clean) with coverage [{a, incomplete_failed, scope [f.go]}, {b, complete_broad, inspected [f.go]}] and failure a: reviewer process exited unexpectedly:

- `a` did not produce a result: reviewer process exited unexpectedly Re-running the same review reproduces this: a reviewer that declined a file declines it again. Closing the gap means bringing these paths into the remit of a reviewer that will read them, or establishing that they need no review.

"These paths" names nothing, since no path was listed. The determinism claim is about declines and does not hold for a crash: a reviewer that exited or timed out may well succeed on a re-run, so the one action that could clear the run is the action the text tells the operator not to bother with. The lead-in ("this run did not cover the change") is also false here, which is the state the new sentence at line 520 was written for.

Suggested fix: condition the closing sentence on what actually happened rather than on the file list alone. Gate the decline paragraph on len(uninspected) > 0, keep the line 520 sentence for the no-unread-files case, and when failures are present add a separate sentence for them (a reviewer that did not produce a result may or may not on another pass; re-running is worth one attempt, and cr review short-circuits with review already complete unless forced). No test in summary_approval_withheld_test.go covers failure-with-no-unread-files (a failed reviewer's assigned files are named as unread asserts the opposite case), so the fix should carry one (U-T1).

Reply inline to this comment.

monit-reviewer requested changes Aug 26, 2026

View reviewed changes

Comment thread internal/reviewplan/reviewplan.go

Comment thread internal/reviewplan/summary.go Outdated

Comment thread internal/reviewplan/summary.go

monit-reviewer requested changes Aug 26, 2026

View reviewed changes

Comment thread internal/reviewplan/summary.go Outdated

Comment thread internal/reviewplan/summary.go

monit-reviewer approved these changes Aug 26, 2026

View reviewed changes

piekstra marked this pull request as ready for review August 26, 2026 16:01

piekstra merged commit 783e587 into main Aug 26, 2026
19 of 20 checks passed

piekstra deleted the piekstra/approval-withheld-diagnostic branch August 26, 2026 16:02

Navigation Menu

Uh oh!

fix(reviewplan): state why a clean review withheld approval - #574

fix(reviewplan): state why a clean review withheld approval#574
piekstra merged 3 commits into
mainfrom
piekstra/approval-withheld-diagnostic

piekstra commented Aug 26, 2026 •

edited

Loading

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Aug 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

piekstra commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Scope

Verification

Review rounds

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Major - internal/reviewplan/summary.go:395

Minor - internal/reviewplan/reviewplan.go:830

Major - internal/reviewplan/summary.go:422

Reviewer Coverage

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Major - internal/reviewplan/summary.go:458

Minor - internal/reviewplan/summary.go:429

Reviewer Coverage

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Minor - internal/reviewplan/summary.go:515

Reviewer Coverage

Uh oh!

monit-reviewer Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

piekstra commented Aug 26, 2026 •

edited

Loading

Major - `internal/reviewplan/summary.go:395`

Minor - `internal/reviewplan/reviewplan.go:830`

Major - `internal/reviewplan/summary.go:422`

Major - `internal/reviewplan/summary.go:458`

Minor - `internal/reviewplan/summary.go:429`

Minor - `internal/reviewplan/summary.go:515`