Block the Review Loop on a Shape the Reader Does Not Know

[ptr727]

Closes #607.

What was wrong

A Copilot review body states how many of the pull request's changed files it read, and nothing parsed that line. A round that read part of the diff carries the correct commit.oid, raises no inline threads, and reports "generated no comments", so it is the clean pass byte for byte in everything the loop checks, and status printed review_on_head=yes over it and exited 0.

Measured over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed, and all three merged. #592 is the sharpest: three changed files, one never read, across both rounds, both reporting no comments.

What changed, and why it is wider than the issue asked

Partial coverage is the third instance of a shape this script already answers twice, and the generalization is the point. Every reader here keys on a structural marker, so a marker that changes spelling is a section the reader stops finding and reports as absent. All three failures on record have that shape:

Drift	Result
Suppressed heading reworded	suppressed=0 over a body carrying findings
Suppressed section moved inside another wrapper	suppressed=0 again
Coverage line never parsed	a partial round reported as a covered head

Each was found by the maintainer after it had landed, rather than by the gate. So the digest now vets the reviewer's output as a whole and fails closed:

• coverage=full / PARTIAL / unstated, with exit 42 on a partial round.
• shapes=ok / UNRECOGNIZED, with exit 43 on any heading, <summary>, metadata label, coverage wording or reviewer login the script has no vetted spelling for. It outranks 42, because a reader that does not understand the output cannot be believed about what it read of the diff.

The 43 message states the remedy in two parts: file an issue on the repository hosting the reader, quoting the body the shape came from, and the merge decision is the maintainer's. An unrecognized shape does not say the pull request is bad, only that nothing here can vouch for the review of it.

The inventory is measured, not imagined

With fenced blocks dropped and text reduced to ASCII, all 332 bodies reduce to 7 headings, 6 <summary> texts and 3 metadata labels, and every body carries at least one. Counts normalize to (N) and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section changing, and dropping the emoji is also what keeps the source inside the charset rule.

Two exemptions, both required by the corpus:

• A body stating no coverage reads as unstated, never as pass or failure. 28 of the 332 are an overview and a change list, that shape is current and interleaves with the counted one, and one pull request carries both across its two rounds. Failing on it would cry wolf on about one review in twelve.
• A refusal is exempt, being a bare paragraph by design and already classified. The exemption is the pattern rather than a carve-out, so a refusal reworded stops being exempt and blocks, which is the refusal check's own failure mode caught one rewording later.

The quietest reading is the reviewer login: a rename leaves every filter here matching nothing, so a review that landed reads as rounds=0 and a wait polls out its timeout against it.

Evidence

Both readers were swept over the full corpus before this was written, and over it again after:

• Shape inventory: 0 items raised across all 332 bodies, and 0 across every review and comment author on those 120 pull requests (copilot-pull-request-reviewer and ptr727).
• Coverage: full 299 / unstated 28 / partial 5, the five being exactly Sweep the spaced-hyphen prose class out of the carried docs #476 (x2), Promote the prose sweeps and the suppressed-findings digest to main #479 and Group the backlog by what ships rather than by what it touches #592 (x2).
• Live: Group the backlog by what ships rather than by what it touches #592 reports coverage=PARTIAL shapes=ok and exits 42. Retire the scope-floor cluster and release the one it blocked #595, Clear the comment half of the prose backlog, exemption first #604 and Clear the carried half of the prose backlog, and plan its re-vendor #606 report shapes=ok and exit 0.

166 tests pass, up from 132. The two fixtures the issue named as unasserted filler are promoted to assertions, and a case reads the vetted coverage spellings out of the runbook and hands them to the script's own parser, so the pair fails in both directions on drift. The old fixtures crafted review bodies with no heading, which no real body has, so they were made realistic rather than the check loosened.

Also carried

• .github/copilot-instructions.md - the verify step checked commit.oid only, which is what this shows to be insufficient, plus a new section stating that an unrecognized shape blocks and earns an issue.
• GOVERNANCE.md - merge-gate precondition 4, and the "all four preconditions" restatement swept to five.
• scripts/README.md - both readings.
• TODO.md - a measure cluster to survey whether GitHub publishes anything structured about a Copilot review, since this whole design rests on prose being the only surface. Introspection says it is: PullRequestReview exposes body, bodyText and bodyHTML and no field naming a finding or a file count, and the only Copilot-named GraphQL types configure review-on-push in a ruleset.

The accepted cost

The shape scan reads every round rather than the head's, so once Copilot changes format, every open pull request blocks at once until the inventory is updated. That is deliberate and the maintainer's call: it is well balanced against a reviewer silently missing a raised finding.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Tightens the repository’s Copilot review gate by teaching scripts/pr_review.py to (1) detect partial file coverage in a formal review and (2) fail closed when the reviewer output contains unrecognized structural markers, so the review loop cannot silently pass on a misread review body.

Changes:

• Extend scripts/pr_review.py to parse per-round “files reviewed” coverage lines, emit coverage=... in the digest, and return blocking exit codes for partial/unvetted coverage and unrecognized shapes.
• Add/expand unit tests to cover full/partial/unstated coverage, shape inventory enforcement, and new exit-code behavior for status/wait.
• Update governance/runbook documentation to incorporate the new merge-gate precondition and the “block on unknown shapes” remedy.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TODO.md	Adds a “measure” backlog cluster to survey whether structured (non-prose) Copilot review data exists.
scripts/test_pr_review.py	Adds coverage/shapes test coverage and new exit-code assertions for status/wait.
scripts/README.md	Documents the new coverage and “unrecognized shapes” behaviors/exit codes.
scripts/pr_review.py	Implements coverage parsing, shape inventory checks, digest fields, and new blocking exit codes.
GOVERNANCE.md	Updates the merge gate to require tooling-recognized reviewer output shapes.
.github/copilot-instructions.md	Updates the review runbook with coverage-of-diff guidance and the “unrecognized shape blocks” procedure.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

scripts/pr_review.py:453

• coverage_statements() currently requires the literal substring "changed file" even for the - **Files reviewed:** ... spelling. If GitHub keeps the Files reviewed bullet but drops the trailing "changed files" text (a plausible wording drift), this function will silently ignore the coverage line and the run will fall back to coverage=unstated instead of failing closed / reporting an unrecognized coverage shape.

def coverage_statements(body: str) -> list[str]:
    """The lines this round states its file coverage on, quotations excluded."""
    return [ln.strip() for ln in FENCE.sub('', body or '').splitlines()
            if COVERAGE_OPENS.match(ln) and 'changed file' in ln.lower()]

scripts/pr_review.py:462

• read_coverage() returns parsed counts without validating basic invariants. If the line parses but the counts are inconsistent (e.g. "reviewed 8 out of 7"), coverage_of() currently treats that as FULL due to >=, which fails open on an obviously invalid coverage statement. This should instead be treated as an unvetted/unrecognized coverage line so the run blocks (exit 43).

    m = COVERAGE_COUNTS.search(line)
    if not m:
        return None
    reviewed, changed = (m.group(1), m.group(2)) if m.group(1) else (m.group(3), m.group(4))
    return int(reviewed), int(changed)

scripts/pr_review.py:95

• COVERAGE_COUNTS only matches "changed files" (plural). If the reviewer emits the grammatically singular form (e.g. "1 out of 1 changed file"), read_coverage() will treat it as unvetted and block the review loop even though the meaning is unchanged.

This issue also appears in the following locations of the same file:

• line 450
• line 458

COVERAGE_COUNTS = re.compile(
    r'reviewed\s+(\d+)\s+out of\s+(\d+)\s+changed files'
    r'|\*\*Files reviewed:\*\*\s*(\d+)\s*/\s*(\d+)\s+changed files', re.IGNORECASE)

[ptr727]

Round 2: three suppressed findings, all three correct, all three fixed in cf2b892

Answered here rather than in a thread, since a suppressed finding reaches none. Each one is the same shape as the bug this pull request exists to fix, one level down in the reader itself, which is a fair thing for the round to have caught.

1. coverage_statements() required the literal "changed file" even for the - **Files reviewed:** spelling.

If GitHub keeps the Files reviewed bullet but drops the trailing "changed files" text (a plausible wording drift), this function will silently ignore the coverage line and the run will fall back to coverage=unstated instead of failing closed.

Reproduced before fixing: coverage_statements('- **Files reviewed:** 4/4') returned [], and the vetted-label check does not catch it either, since Files reviewed is itself a vetted label. So the round would have reported coverage=unstated over a round that stated its coverage plainly.

The two openers now carry different text requirements, because the text each needs beside it differs. The bullet's own label is the marker, so it qualifies whatever follows the counts. The sentence opener is the reviewer's name, which prose also opens a line with, so that one keeps the requirement.

2. read_coverage() validated no invariant, so "reviewed 8 out of 7" read as FULL through the >=.

This should instead be treated as an unvetted/unrecognized coverage line so the run blocks (exit 43).

Correct, and the >= was a deliberate simplification that was simply wrong. A round claiming it read more files than the pull request changed is not a round that read them all, it is a line this script is parsing wrongly, and reading it as full coverage fails open on the very statement saying something is off.

read_coverage() now returns None for a pair that cannot both be true, which is one point of truth rather than two: coverage_of() reads it as UNVETTED and unrecognized_in() lists it, so the same line takes the same remedy on both paths. The comparison in coverage_of() is == accordingly.

3. COVERAGE_COUNTS required the plural "changed files".

If the reviewer emits the grammatically singular form (e.g. "1 out of 1 changed file"), read_coverage() will treat it as unvetted and block the review loop even though the meaning is unchanged.

Correct, and this one is the cry-wolf direction rather than the fail-open one, which makes it worth naming separately: a one-file round saying 1 changed file means exactly what it says, and stopping every open pull request in the fleet over a grammatical agreement is the guard an agent learns to work around. The opener already accepted the singular through a substring test, so the two halves of the reader disagreed with each other. Both now read changed files?.

Evidence. All three reproduced against the code before the fix and verified after. The corpus sweep is unchanged at 0 unrecognized items across all 332 review bodies and full 299 / unstated 28 / partial 5, so none of the three widened anything into a false positive. Four cases were added, one per finding plus one holding the sentence opener to its text requirement, and the suite is at 170.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/pr_review.py:103

• The bullet form is treated as a coverage line based on its label alone (see the comment above COVERAGE_BULLET and is_coverage_line()), but COVERAGE_COUNTS currently requires the trailing "changed file(s)" words for the bullet alternative. If GitHub ever emits a shorter bullet like - **Files reviewed:** 4/4 (no suffix), this will be classified as an unrecognized coverage line and block the loop unnecessarily.

Consider parsing the X/Y counts for the bullet form even when the trailing words are missing, while still requiring the words for the sentence form to avoid false positives.

COVERAGE_COUNTS = re.compile(
    r'reviewed\s+(\d+)\s+out of\s+(\d+)\s+changed files?'
    r'|\*\*Files reviewed:\*\*\s*(\d+)\s*/\s*(\d+)\s+changed files?', re.IGNORECASE)

[ptr727]

Round 3: one suppressed finding, correct, fixed in f604f65

The bullet form is treated as a coverage line based on its label alone [...] but COVERAGE_COUNTS currently requires the trailing "changed file(s)" words for the bullet alternative. If GitHub ever emits a shorter bullet like - **Files reviewed:** 4/4 (no suffix), this will be classified as an unrecognized coverage line and block the loop unnecessarily.

Correct, and it is the exact complement of what round 2 asked for and I half-delivered. Making the bullet a coverage line on its label alone moved the failure from a silent unstated to a block, rather than making the line readable. Reproduced before fixing:

detected: ['- **Files reviewed:** 4/4']
parsed  : None
verdict : ('unvetted', '- **Files reviewed:** 4/4')

Blocking there is the cry-wolf direction: 4/4 is unambiguous, and stopping every open pull request over a suffix the script does not need is the guard an agent learns to work around. The trailing words are now optional for the bullet, for the same reason its label identifies the line, and stay required for the sentence, whose words are what separate it from prose.

The class of bug is worth naming, since it is the third instance in this pull request: detection and parsing keyed on different things, so a line one half accepted the other half rejected. After this the two disagree only where the line genuinely carries no readable count, and a bullet like - **Files reviewed:** all of them still blocks, correctly.

The case that missed it is the more useful finding. It asserted detection on the bare form and parsing only on the suffixed form:

self.assertEqual(['- **Files reviewed:** 4/4'], pr_review.coverage_statements('- **Files reviewed:** 4/4'))
self.assertEqual((4, 4), pr_review.read_coverage('- **Files reviewed:** 4/4 changed file'))

Two assertions written as if they covered one thing, split across two different inputs, so the gap sat exactly between them. It now asserts detection, parsing and the verdict across all three spellings, plus a case holding the no-counts bullet to blocking.

Corpus sweep unchanged: 0 unrecognized items across all 332 bodies, full 299 / unstated 28 / partial 5. Suite at 171.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/copilot-instructions.md:173

• This exemption bullet calls the no-coverage shape "unknown", but the tooling/docs in this PR (and scripts/pr_review.py status) use the literal coverage=unstated for that case. Using a different term here makes it harder to map the runbook guidance to the actual digest output.

- **Exempt: a body stating no coverage at all.** 28 of those 332 bodies are an overview and a change list and nothing more. That shape is current, interleaves with the counted one throughout, and one pull request carries both across its two rounds, so treating it as a failure cries wolf on about one review in twelve and teaches an agent to work around the gate. It reads as **unknown**, never as a pass and never as a failure.

[ptr727]

Round 4: one suppressed finding, correct, fixed in b538613

This exemption bullet calls the no-coverage shape "unknown", but the tooling/docs in this PR (and scripts/pr_review.py status) use the literal coverage=unstated for that case.

Correct. Round 1 raised the same mismatch two lines below this one, and the sweep that answered it stopped at the line the finding named instead of grepping the term. That is the recurring failure in this repository's own rules, so it is worth stating plainly rather than quietly fixing: a finding names an instance, and the fix is for the term.

Grepping unknown across the four documents this pull request touches turns up exactly one more stale use, this one, now reading `coverage=unstated` in the literal the digest prints. The other hits are unrelated and correct: answered_outside_review=unknown is a different field that really does print that word, charset-unknown is another gate's finding name, and commit unknown is the suppressed block's marker for a null commit.

The constant carried the same mismatch and is worth more than the doc line. UNKNOWN = 'unknown' printed as unstated through COVERAGE_FIELD, so the name in the source and the word in the output were two different words for one state, which is precisely what renaming the field was meant to stop. Anyone reading coverage_of() had to map one onto the other. It is UNSTATED = 'unstated' now, and COVERAGE_FIELD is back to being about upper-casing the two blocking states and nothing else.

Corpus sweep after the rename: 0 unrecognized items across all 332 bodies, full 299 / unstated 28 / partial 5. Suite at 171, prose gate, markdownlint and editorconfig-checker all clean.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

TODO.md:322

• Same issue as above: [scripts/pr_review.py][scripts] resolves to ./scripts/README.md (the [scripts] reference), not to scripts/pr_review.py. Using a code span for the file path avoids a misleading link.

  - **Checked** - `develop` at `20916ad` on 2026-08-07, reading the live GraphQL schema by introspection and one review over REST, against the reader in [`scripts/pr_review.py`][scripts].

TODO.md:317

• The reference label [scripts] points to ./scripts/README.md, so linking scripts/pr_review.py with [scripts] is misleading (the link text looks like the file path but resolves to the README). Use an inline code span for the file path and keep [scripts] as the separate reference to the scripts README.

This issue also appears on line 322 of the same file.

One pull request, after a survey, deciding whether anything stands between this fleet's review loop and the raw prose of a Copilot review. Today [`scripts/pr_review.py`][scripts] reads the review body as text and holds a vetted inventory of the headings, collapsed sections, metadata labels and coverage wordings it recognizes, blocking on anything it does not. That design is correct for a prose surface and it carries a cost the maintainer has accepted deliberately: a wording change at GitHub blocks every open pull request in the fleet at once, until the inventory is updated. The cost is worth paying against a reviewer silently missing a raised finding, which is the failure it replaces, but it is worth paying only for as long as prose is the only surface on offer.

**State** `measure`. **Touches** [`scripts/pr_review.py`][scripts] and the runbook section in [`.github/copilot-instructions.md`][copilot-instructions], once the survey says whether there is anything to move to. **Cost** a survey first, then either nothing or a rewrite of the reading layer, which is the larger of the two outcomes and the reason the survey comes first.

scripts/test_pr_review.py:659

• This test name still uses unknown even though the implementation and assertions use the coverage=unstated spelling. Renaming the test improves consistency and avoids confusion with the separate answered_outside_review=unknown state.

    def test_a_round_stating_no_coverage_at_all_is_unknown_rather_than_either_verdict(self) -> None:

scripts/test_pr_review.py:704

• This docstring uses "Unknown" for the no-coverage case, but the code models that state as coverage=unstated. Using the same term as the digest output makes the test intent clearer.

        """Unknown is the absence of a statement rather than a bad one, so it loses to a count."""

[ptr727]

Round 5: four suppressed findings, all correct, all fixed in 2c3e12d

1 and 2. [scripts/pr_review.py][scripts] resolves to scripts/README.md.

The reference label [scripts] points to ./scripts/README.md, so linking scripts/pr_review.py with [scripts] is misleading (the link text looks like the file path but resolves to the README).

Correct, and worth more than a formatting nit: the link text names one file and the link goes to another, so a reader following it lands on the README looking for a reader that is not there. Both are code spans now. The one place the entry does mean the README, the Open bullet about the standard-library constraint, keeps the label, since that one is about what scripts/README.md records.

3 and 4. Two test names and a docstring still spelled the state unknown.

This test name still uses unknown even though the implementation and assertions use the coverage=unstated spelling.

Correct. This is the previous round's rename reaching the cases that describe the state rather than assert on it, which the grep behind that round did not cover because it was scoped to the documents. grep -in unknown scripts/test_pr_review.py now returns only the unrelated and correct uses: the answered_outside_review=unknown cases, the window-blindness cases, and the commit unknown marker for a null commit on a suppressed block.

Three of the last four rounds have found an instance of this same rename, each in a surface the previous sweep did not cover, which says the sweep kept being scoped to where the finding pointed rather than to the term. Recorded here because it is the repository's own documented failure mode and the record is worth more than the four one-word fixes.

Corpus sweep unchanged: 0 unrecognized items across all 332 bodies, full 299 / unstated 28 / partial 5. Suite at 171, prose gate, markdownlint and editorconfig-checker clean.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/pr_review.py:1211

• wait only calls report_verdict(final) when reviewed_head(final) is true. That prevents the new unrecognized-shape gate (exit 43) from ever triggering in cases where the review did land but reviewed_head stays false (e.g. reviewer-login drift, which unrecognized_shapes() is explicitly meant to catch). This can make wait time out and exit as PENDING/30 instead of exiting 43 as documented.

    if reviewed_head(final):
        # Coverage of the head is what this returns 0 on.
        # Coverage of the diff is a second reading it used to take on trust.
        # A round can carry the head and have read only part of it.
        return report_verdict(final)

[ptr727]

Round 6: one suppressed finding, correct, and the best of the run. Fixed in 467ade5

wait only calls report_verdict(final) when reviewed_head(final) is true. That prevents the new unrecognized-shape gate (exit 43) from ever triggering in cases where the review did land but reviewed_head stays false (e.g. reviewer-login drift, which unrecognized_shapes() is explicitly meant to catch). This can make wait time out and exit as PENDING/30 instead of exiting 43 as documented.

Correct, and it is the sharpest kind of finding: the gate could not fire for the one drift it was written to catch. reviewed_head filters on the reviewer's login, so a renamed login leaves it false, and gating the verdict behind it meant the login check was structurally unreachable. Reproduced against a payload whose only difference is the login:

shapes seen  : ['reviewer login: copilot-code-review-agent, where every query here filters on copilot-pull-request-reviewer']
reviewed_head: False
wait exit    : 30
digest said  : ... review_on_head=NO coverage=unstated shapes=UNRECOGNIZED ...

The digest printing shapes=UNRECOGNIZED while the code returns 30 is the digest disagreeing with the exit code, which this same file already names as its own failure mode, in the comment above the final read: "a reader resolves that by believing the code, dropping the review it was just shown." The gate had reintroduced it one branch further down.

The verdict is no longer gated on that reading: if unrecognized_shapes(final) or reviewed_head(final). After the fix, exit 43 with the status line.

The loop had a second half of the same bug. With the exit code fixed, a drifted login still polled the full 45-minute timeout before reaching it, because done can never become true. That would have made the source comment beside READS_AS_REVIEWER false about its own code, since it claims a drift is what leaves "a wait polling out its whole timeout against a review sitting in plain sight". reviewer_login_drift() is factored out and read from the authors alone, so the liveness query answers it and the loop terminates on it at no extra call. Verified: with --timeout 600 the wait now exits 43 with slept.call_count == 0.

Three cases added: the drift reaching 43 without polling, the same gap through the body reader on a round covering no head, and an ordinary pending round still returning 30, so the gate cannot swallow the common case. Suite at 174.

Corpus sweep unchanged: 0 unrecognized items across all 332 bodies, full 299 / unstated 28 / partial 5.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.