Skip to content

pr_review.py does not read a review's file-coverage line, so a partial review reports as a clean pass #607

Description

@ptr727

Summary

scripts/pr_review.py does not read the coverage statement in a Copilot review body. A review that covered only part of the diff is therefore byte-for-byte identical to a clean full pass in everything the script does read, and status prints review_on_head=yes over it.

That is the exact failure shape the script already exists to guard. Its module docstring names two instances of it, and both are implemented:

  • SUPPRESSED — findings withheld from the inline threads, so polling threads alone reports a clean pass.
  • REFUSAL — a formal review carrying the head and no threads, which the source calls "the clean pass byte for byte, so every coverage check passes over a round that never ran".

Partial file coverage is the third instance, and it is unguarded.

It has happened, and it is not rare enough to ignore

Measured over 332 Copilot review bodies on this repository (every review on the newest 120 pull requests, read via gh pr view --json reviews):

PR Coverage line Outcome
#476 reviewed 10 out of 11 changed files (2 rounds) MERGED
#479 reviewed 16 out of 17 changed files MERGED
#592 reviewed 2 out of 3 changed files (2 rounds) MERGED 2026-08-07

5 partial rounds across 3 pull requests, all merged. #592 is the sharpest case: 3 changed files, one never reviewed, across both rounds, and both rounds reported "generated no comments". It merged today. pr_review.py status would have called that a clean covered head, because it is one everywhere except in a line nothing parses.

The vetted shapes, measured rather than guessed

Four phrasings carry a coverage count, and the counts below are review rounds, not pull requests:

180  Copilot reviewed N out of N changed files in this pull request and generated no new comments.
 50  Copilot reviewed N out of N changed files in this pull request and generated N comment.
 24  Copilot reviewed N out of N changed files in this pull request and generated no comments.
 18  Copilot reviewed N out of N changed files in this pull request and generated N comments.
 32  - **Files reviewed:** N/N changed files

The wording has already drifted once, which is the same reason SUPPRESSED and REFUSAL are alternations rather than single patterns. Note that scripts/test_pr_review.py already carries two of these shapes as fixture filler (Reviewed 3 of 3 changed files at line 43, - **Files reviewed:** 1/1 changed files at line 59) and nothing asserts on either.

The subtlety that decides the design

28 of the 332 bodies carry no coverage statement at all — an overview and a change list, nothing more. #595 is one.

This shape is not historical. It interleaves with the current one throughout, and #576 carries both across its two rounds:

PR 592: out-of      PR 595: none        PR 596: out-of
PR 604: out-of      PR 605: out-of      PR 606: out-of

So "no coverage line" cannot be treated as a failure — it would cry wolf on roughly 8% of reviews, and a guard an agent learns to work around is worse than no guard. It is a known shape that carries no coverage information, which is a different thing from an unknown one.

Asked-for behavior

Fail closed on shapes the script does not recognize, so that an unrecognized shape gets the script fixed rather than passing silently. This is the principle scripts/prose_lint.py already applies in charset-unknown: "An unrecognized character is reported rather than passed. A gate that allows whatever it does not recognize stops gating as the character set grows."

Three buckets, per review round rather than per pull request:

  1. Recognized, N == M — full coverage. Reports as it does today.
  2. Recognized, N < M — partial coverage. A real failure: files in the diff went unreviewed, and the remedy is a re-request or a split, not a merge.
  3. Coverage-shaped but matching no vetted phrasing — unvetted shape. A failure whose remedy is fixing this script, stated in those words in the output. This is the bucket that keeps the vetted list honest as Copilot's wording drifts again.

Plus the two exemptions the corpus requires:

  • Overview-only body (bucket 0) — a vetted shape carrying no count. Must report coverage as unknown, never as pass and never as failure. Silently passing it is the current bug in miniature.
  • Refusal body — carries no coverage line by design, and REFUSAL already classifies it. Must be exempt before any of the above runs, or every refusal becomes a spurious unvetted-shape failure. The corpus holds 0 refusals, so this is reasoned from REFUSED in test_pr_review.py rather than observed here.

Surfaces to change

  • digest() — a coverage field on the digest line, with the three failure states loud in the way review_on_head=NO and refusal=YES already are.
  • status — currently returns 0 unconditionally (pr_review.py:879-882). It needs non-zero codes for buckets 2 and 3; the docstring documents exit codes per subcommand and lists none for status today.
  • wait — same reading, since reviewed_head() is what decides its 0.
  • Exit codes in use: 30, 40, 41, 50, 60-64, 70, 71. Two free numbers are needed.
  • scripts/test_pr_review.py — one test per bucket, and the two existing fixtures promoted from filler to assertions.
  • .github/copilot-instructions.md — the runbook's verify step currently checks commit.oid coverage only, which is what this issue shows to be insufficient.

Why this matters beyond the count

The verify-the-review-covered-the-head check is the load-bearing one in the runbook, because green CI is the weakest signal and mergeStateStatus: CLEAN is explicitly not sufficient to merge. Coverage of the head is currently proven by commit.oid, and coverage of the diff is not proven at all. #592 is what that gap looks like when it lands.

Provenance

Found while reviewing #606, whose own review reported 7 out of 7 — full coverage, and the line that prompted the question.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions