diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 424f394..649cf01 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -156,6 +156,42 @@ gh api repos///issues//comments --jq \ Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. The exception is the refusal above, which is a formal review on the head with no inline comments and covers nothing, so path (1) exits 0 over a round that never ran. Read the body of the review the SHA matched, not only the SHA. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. +**Coverage of the head is not coverage of the diff, and the second one is stated in a line nothing above reads.** A review body says how many of the pull request's changed files it read, and a round that read fewer than the pull request changed is byte for byte the clean pass in everything else: the same `commit.oid`, the same absent threads, the same "generated no comments". 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. One of them changed three files, left one unread across **both** its rounds, and reported no comments each time. This is the third instance of the shape the refusal above and the suppressed block below are the first two, so read it the same way: **fail closed on a wording you do not recognize**, since a gate that allows whatever it does not recognize stops gating as the wording drifts, and both of those wordings have drifted once already. + +Two spellings carry the count, and both are current rather than one superseding the other. Each opens its own line, which is what separates the round stating its coverage from prose mentioning changed files, that prose being what a review of a change to this rule looks like: + +```text +Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments. +- **Files reviewed:** 2/3 changed files +``` + +The sentence tail after the first spelling reports how many comments the round raised and appears in four wordings. It is not coverage, so it is not part of what has to be recognized, and the counts are. Read them into three verdicts and two exemptions: + +- **Counts equal** - the round read the whole diff. This is the clean pass. +- **Counts unequal** - files in the diff have no review at all. Re-request on this head or split the pull request, rather than merging on it. +- **Coverage-shaped and unreadable** - the remedy is to fix the reader, not to read past it. The vetted spellings live in `scripts/pr_review.py` and here, and they stay in step because a case reads them out of this file. +- **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 `coverage=unstated`, never as a pass and never as a failure. +- **Exempt: a refusal.** It carries no coverage line by design, and the refusal rule above has already classified it. Read it here as well and every refusal grows a spurious second failure on top of the one that names its remedy. + +`scripts/pr_review.py status --repo /` reports this as `coverage=full`, `coverage=PARTIAL`, `coverage=UNVETTED` or `coverage=unstated`, and exits `42` on a partial round. An unreadable wording exits `43` instead, as one of the unrecognized shapes below rather than as a case of its own, since both say the reader is what needs fixing. Read it by hand as: + +```sh +gh pr view --json reviews --jq \ + '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .body + | split("\n")[] | select(test("^(Copilot|[-*] \\*\\*Files reviewed:).*changed files?"))' +``` + +### A Shape Nothing Recognizes Blocks the Loop and Earns an Issue + +**Every rule above keys on a marker in what Copilot sent, so a marker that changes spelling is a section the reader stops finding and reports as absent.** That is not a hypothetical: all three failures on record here have exactly that shape. The suppressed heading was reworded and the count went to zero. The suppressed section moved inside another wrapper and the count went to zero again. The coverage line was never read at all. Each one reported a clean pass over a review it had misread, and each was caught by the maintainer after it had already landed, rather than by the gate. + +**So an unrecognized shape is a blocking outcome, and its remedy is an issue rather than a judgment call.** When any reader here meets a heading, a collapsed section, a metadata line, a coverage wording or a reviewer login it has no vetted spelling for, the review loop **does not close**, whatever else the digest says. Do not read past it, do not infer what the new wording probably means, and do not treat a body that looks clean as a clean review, because "looks clean" is precisely what a misread review looks like. Two things follow, in this order: + +1. **File an issue on the hub, `ptr727/ProjectTemplate`**, which hosts `scripts/pr_review.py` and holds the vetted inventory. Name each unrecognized shape and quote the review body it came from, so the fix is made against the real wording rather than a paraphrase. The issue is filed even when the shape turns out to be cosmetic, since "cosmetic" is a conclusion drawn after reading the body and not before. +2. **The merge decision is the maintainer's**, not the agent's and not the script's. An unrecognized shape does not mean the pull request is bad, it means nothing here can vouch for the review of it. Report the state, hand it over, and stop. + +`scripts/pr_review.py status --repo /` reports this as `shapes=UNRECOGNIZED`, lists each shape under a marker naming the remedy, and exits `43`. `wait` carries the same code, so a wait cannot end on a clean zero over output nothing read. The vetted inventory lives in that script and is small on purpose: measured over 332 Copilot review bodies on this repository, with fenced blocks dropped and text reduced to ASCII, the whole corpus is seven headings, six `` texts and three metadata labels, and every body carries at least one of them. A body carrying none is itself the unrecognized shape, which is what catches a rewrite that changes everything at once, the refusal wording drifting among it. + ### Bounded Retry Workflow This path is only for a **genuinely missing** review, meaning no Copilot review (formal *or* issue comment) covers the current head SHA after polling. A review that covered the head but produced no comments is a clean pass, not a missing review, so do not enter this retry path for it. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 3f49194..48f6c8f 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -265,9 +265,10 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom **Do not merge, and do not enable auto-merge, unless ALL of these hold:** 1. Required status checks are green (`mergeStateStatus: CLEAN`), **and** -2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, **and** +2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, and because a review can carry the head SHA and still say it read only part of the diff, which is the same clean pass in everything a SHA match can see, so the body's own count of the files it read is checked against the files the pull request changed, **and** 3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and** -4. The maintainer has given **explicit** permission to merge. +4. Nothing in the reviewer's output was a shape the review tooling could not read, since every reader keys on a marker and a marker that changes spelling is a section the reader stops finding and reports as absent, which is how three separate misreadings each reported a clean pass over a review they had not understood. An unrecognized shape blocks this gate on its own, and the remedy is an issue filed against the repository hosting the reader, naming the shape and quoting the body it came from, rather than a judgment about what the new wording probably meant, **and** +5. The maintainer has given **explicit** permission to merge. `mergeStateStatus: CLEAN` reflects **only** required statuses, and never open bot review comments, so `CLEAN` alone is **never** sufficient to merge. A green/`CLEAN` PR with an unresolved Copilot finding fails this gate, so treat it as "not mergeable" no matter what the merge-state field says. The agent never merges on its own (consistent with "default to staging", and merging is maintainer-authorized). @@ -283,7 +284,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom 6. Reply to each thread and resolve what was addressed. 7. Re-run the loop after every fix push until no actionable findings remain. -Drive the loop to green, meaning a review confirmed on the latest head SHA and every actionable finding closed, then stop and apply the **Merge Gate** above: all four preconditions must hold, and `mergeStateStatus: CLEAN` alone never satisfies it. +Drive the loop to green, meaning a review confirmed on the latest head SHA and every actionable finding closed, then stop and apply the **Merge Gate** above: all five preconditions must hold, and `mergeStateStatus: CLEAN` alone never satisfies it. For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract, and that file owns the mechanics. diff --git a/TODO.md b/TODO.md index 1cbac71..88f6175 100644 --- a/TODO.md +++ b/TODO.md @@ -310,6 +310,23 @@ One pull request routing the disproof record from the provider-agnostic contract - **Settled** - The write side is where the gap bites rather than the read side, because an agent following the loop is already routed to the runbook for mechanics and an agent posting a decline is routed nowhere. - **Settled** - "Durable Knowledge and Self-Improvement" already requires durable knowledge to reach a committed file, so this states where one class of it goes rather than adding an obligation. +### A Programmatic Reading of a Copilot Review + +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` 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` 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. + +- **Find out whether GitHub publishes a structured form of a Copilot review, and decide whether to read that instead of the prose.** A schema, an API surface, a published payload, or a maintained library, anything that would make a wording change a non-event rather than a fleet-wide block. + - **Blocked by** - Nothing. The prose reader ships either way, so this decides what replaces it rather than whether the loop has a gate. + - **Issue** - None filed. The prose reader and its vetted inventory shipped under [#607][issue-607], which is the change this would supersede. + - **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`. + - **Open** - Whether `bodyHTML` is a better surface than the Markdown body, since it arrives as a rendered tree whose structure survives a change in Markdown syntax, while leaving the wording drift the inventory exists for exactly where it is. + - **Open** - Whether any third-party library tracks this output, and whether depending on one is acceptable at all, given that [`scripts/README.md`][scripts] holds these scripts to the standard library with no third-party packages. + - **Open** - Whether the review's own inline threads and their metadata carry enough to derive coverage and suppression without reading the body, which would narrow the prose surface rather than replace it. + - **Settled** - The public API carries no structured Copilot review as of the date above. GraphQL `PullRequestReview` exposes `body`, `bodyText` and `bodyHTML` and no field naming a finding, a file count, or a withheld section, and REST returns the same prose body beside its ids and its state. + - **Settled** - The only Copilot-named types in the GraphQL schema are `CopilotCodeReviewParameters` and its input form, which configure review-on-push inside a branch ruleset and describe nothing about a review that has run, so the schema search that looks promising by name answers a different question. + - **Settled** - A negative finding is the deliverable as much as a positive one, and it is recorded here rather than re-derived, since the reading layer's design rests on prose being the only surface and that premise is worth re-checking rather than assuming. + ## Standalone Chores Small work with no research to preserve, selectable one bullet at a time. @@ -448,6 +465,7 @@ Each was checked against the tree and has nothing left to do anywhere. Closing i [issue-580]: https://github.com/ptr727/ProjectTemplate/issues/580 [issue-585]: https://github.com/ptr727/ProjectTemplate/issues/585 [issue-597]: https://github.com/ptr727/ProjectTemplate/issues/597 +[issue-607]: https://github.com/ptr727/ProjectTemplate/issues/607 diff --git a/scripts/README.md b/scripts/README.md index 5cfa0a3..923c05f 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -118,6 +118,16 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie `wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. 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, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. + +The reading fails closed, so a coverage-shaped line that parses to no counts is a failure whose remedy is stated as fixing this script rather than reading past it, which is what keeps the vetted spellings honest as the wording drifts, as it has once for each of the other two patterns. The tail of the sentence is deliberately outside the unit: it says how many comments the round raised, which is not coverage, and reading it would fail every merge over a sentence ending. There are two exemptions, and the first is the one that decides the design. **A body stating no coverage at all reads as `unstated`**, never as a pass and never as a failure: 28 of those 332 bodies are an overview and a change list and nothing more, that shape is current rather than historical and interleaves with the counted one throughout, and one pull request carries both across its two rounds, so failing on it would cry wolf on about one review in twelve and a guard an agent learns to work around is worse than none. **A refusal is exempt** because it states no coverage by design and is already classified, and reading it as a round would grow a spurious second failure on top of the one naming its remedy. + +The line is matched at its **start** rather than anywhere in the body, since both spellings are structural: across those bodies every coverage statement opens its line, 272 with the reviewer's own name and 32 as the `Review details` bullet, and none sits mid-sentence. A body-wide match reports the pull request that adds this check as a partial round, which is the false positive the suppressed matcher and the refusal matcher have each had once already, and fenced blocks are dropped for the same reason, 131 of the bodies carrying one and this change putting both spellings into the diff a review of it quotes. The cost is named rather than hidden: a wording that moves the statement off the line start reads as no statement rather than as one this cannot parse. The reading is **head-scoped**, unlike a suppressed finding and like a refusal, because a partial round describes one commit's diff and the push that changes that diff raises a round reading the whole of the new one, and where one head carries two rounds through a re-request the worst of them reports, since the one naming files it did not read is the one to answer. A case reads the vetted spellings out of the runbook and hands them to this script's own parser, so the pair stays in step in both directions. + +`status` and `wait` both exit `43` where the reviewer sent a **shape this script has no reader for**, and that outcome outranks every other reading here, because a reader that does not understand the output cannot be believed about what it found in it. Every rule in this script 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 exact shape: the suppressed heading reworded and the count went to zero, the suppressed section moved inside another wrapper and the count went to zero again, and the coverage line was never parsed at all. Each reported a clean pass over a review it had misread, and each was found by the maintainer afterwards rather than by the gate. The digest carries `shapes=UNRECOGNIZED`, lists each shape it could not place, and names the remedy in two parts: **file an issue on the hub repository that hosts this script**, quoting the body the shape came from, and leave the merge decision to the maintainer, since an unrecognized shape does not say the pull request is bad, only that nothing here can vouch for the review of it. + +The vetted inventory is measured rather than imagined, and it is small because the output is regular. Across those 332 bodies, with fenced blocks dropped and text reduced to ASCII, the whole corpus is seven headings, six `` texts and three metadata labels. Counts are normalized to `(N)` and the verdict headings' colored circle is dropped before comparing, since both change on every review without the section having changed, and dropping the emoji is also what keeps this repository's charset rule satisfied. A body carrying **no** heading at all is itself unrecognized, which is what catches a rewrite that changes every marker at once, and a **refusal is exempt** because it is a bare paragraph by design and `REFUSAL` is its vetted spelling. That 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 last reading is the quietest: a reviewer **login** that reads as this reviewer without being the spelling every query filters on, since a rename leaves every filter matching nothing and the digest then reports a review that landed as no review at all. A case runs the whole inventory over the measured corpus, where it raises nothing. + The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index b3b8de5..2e1a47a 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -14,6 +14,14 @@ review findings between them. Read-only. Exit 0 = every reference resolves, 70 = one does not, 71 = there were references and none could be read, so nothing was decided. status One digest line, any unresolved threads, and any suppressed findings. Read-only. + Exit 0 = every shape in the reviewer's output is one this reads, and the round + covering the head read the whole diff or stated nothing about what it read. + 42 = that round read fewer files than the pull request changed, so part of the diff + has no review at all, and the remedy is a re-request or a split. + 43 = the reviewer sent a shape this script has no reader for, so no field here can + be believed. The remedy is an issue on the repository hosting this script, and the + review loop does not close until the reader is fixed. Merging regardless is the + maintainer's decision rather than the agent's. reply Answer one thread selected by its text, and resolve it on request. The only writing path here, and it exists because the hand-run form keeps failing the same way: a node id typed into a mutation, which resolves globally and so @@ -30,6 +38,9 @@ 40 reports the shape of that answer and reads nothing of its cause: an answer carrying no commit covers no head, so the wait ends and the reader decides. 41 = the review carrying the head says it did not review, so it covers nothing. + 42 and 43 = the review landed and `status`'s two blocking readings apply to it, + since a wait ending on a round that covered half the diff, or on output nothing here + can read, has ended on something other than a review of this pull request. 50 = the request is pending and nothing picked it up, which no amount of waiting changes. Recovery is two mutations, and they stay in the runbook. @@ -61,6 +72,86 @@ # It also keeps the published filter usable inside single quotes, which neither survives. REFUSAL = re.compile(r'wasn.t able to review|was not able to review|unable to review', re.IGNORECASE) +# A round states how much of the diff it read on a line of its own. +# A round that read part of it is the clean pass elsewhere, same commit and threads and digest. +# Five such rounds landed across three merged pull requests here. +# One of them read 2 of 3 changed files across both its rounds and merged. +# This is the third instance of the shape the two patterns above answer. +# It is also the only one nothing was reading. +# The line is anchored at its start rather than matched body-wide, both spellings being structural. +# Over 332 review bodies every coverage statement opens its line and not one sits mid-sentence. +# 272 of them open with the reviewer's own name and 32 are the `Review details` bullet. +# A body-wide match reports the pull request adding this check as a partial round. +# That is the false positive the suppressed matcher and the refusal matcher have each had once. +# The cost of the anchor is named rather than hidden. +# A wording that moves the statement off the line start reads as no statement at all. +# Two openers rather than one alternation, because the text each needs beside it differs. +# The bullet's own label is the marker, so it is a coverage line whatever follows the counts. +# Requiring the trailing words there made a bullet that drops them read as no statement at all. +# The sentence opener is the reviewer's name, which prose also opens a line with. +# That one keeps the text requirement, since the name alone does not say the line states coverage. +COVERAGE_BULLET = re.compile(r'\s*[-*]\s*\*\*Files reviewed:', re.IGNORECASE) +COVERAGE_SENTENCE = re.compile(r'\s*Copilot\b', re.IGNORECASE) +# The count pair itself, in the two spellings the corpus carries. +# The comment tail one of them ends on is deliberately not part of the unit. +# It says how many comments the round raised, which is not coverage. +# A fifth wording of it would fail every merge over a sentence ending read correctly. +# The plural is optional, since a one-file round reading `1 changed file` means what it says. +# Blocking on that is the cry-wolf case, a fleet-wide stop over a grammatical agreement. +# The bullet's trailing words are optional for the reason its label alone identifies the line. +# Detection and parsing disagreeing there turned a readable `4/4` into a block on a readable line. +# What is left blocking is a bullet carrying no counts, which genuinely states no coverage. +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) +# A fenced block is a quotation rather than a statement, and 131 of those bodies carry one. +# This change puts both spellings into the source and the runbook, so a review of it quotes them. +# A quoted count read as this round's own is a coverage figure nobody stated. +FENCE = re.compile(r'^ {0,3}```.*?^ {0,3}```[^\n]*', re.DOTALL | re.MULTILINE) +# The readings a round's coverage carries, worst first. +# A head carries more than one round only through a re-request. +# Where two disagree, the one naming files it did not read is the one to answer. +# `UNSTATED` sits last rather than beside the failures, being the absence of a statement. +# A round that did state full coverage settles the question over one that stated nothing. +UNVETTED, PARTIAL, FULL, UNSTATED = 'unvetted', 'partial', 'full', 'unstated' +SEVERITY = (UNVETTED, PARTIAL, FULL, UNSTATED) +# Upper-case for the two that block a merge, for the reason `review_on_head=NO` is upper-case. +# `unstated` rather than `unknown`, since a body carrying no count is a shape this knows. +# What this script does not know is the separate `shapes` field, and one word for both hides it. +# The constant carries that name too, so no reader has to map a name here onto another word. +COVERAGE_FIELD = {UNVETTED: 'UNVETTED', PARTIAL: 'PARTIAL', FULL: 'full', + UNSTATED: 'unstated'} + +# Every structural marker the reviewer's own bodies carry, measured over the same 332. +# A body is read for these rather than trusted, because every reader below keys on one of them. +# A heading this script has no spelling for is a section it will not find, reported as absent. +# That is the shape of all three failures already on record here, each caught after it landed. +# The lists are small because the output is regular: 7 headings, 6 summaries and 3 labels. +# Counts are normalized to `(N)` and non-ASCII is dropped before comparing. +# The verdict headings carry a colored circle, so the emoji is what would drift most cheaply. +# Dropping it also keeps this file inside the charset rule that governs the repository. +VETTED_HEADINGS = { + '## Pull request overview', '### Reviewed changes', '### Ready to approve', + '### Changes recommended', '### Not ready to approve', '### Human review recommended', + '### Suppressed comments (N)', +} +VETTED_SUMMARIES = { + 'Pull request overview', 'Show a summary per file', 'File summaries', 'Review details', + 'Suppressed comments (N)', 'Comments suppressed due to low confidence (N)', +} +VETTED_LABELS = {'Files reviewed', 'Comments generated', 'Review effort level'} +MARKDOWN_HEADING = re.compile(r'\s*#{1,6}\s') +# The `Review details` metadata bullets, of which the coverage line is one. +LABEL_LINE = re.compile(r'\s*[-*]\s+\*\*([^*]+):\*\*') +# A login that reads as this reviewer without being the spelling every query here filters on. +# A rename leaves every filter matching nothing, so a review that landed reads as none at all. +# A wait then polls out its whole timeout against a review sitting in plain sight. +# `copilot-swe-agent` is the coding agent rather than the reviewer, and does not match this. +READS_AS_REVIEWER = re.compile(r'copilot.*review', re.IGNORECASE) +# The repository this script is hosted in, named because an unrecognized shape is fixed here. +# It is a literal rather than the pull request's own repository. +# That one is where the shape was seen, not where the reader failing on it lives. +HUB = 'ptr727/ProjectTemplate' DETAILS = re.compile(r'
(.*?)
', re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r'(.*?)', re.DOTALL | re.IGNORECASE) TAGS = re.compile(r'', re.IGNORECASE) @@ -342,22 +433,195 @@ def refusing_review(pr: dict) -> dict | None: return max(refusals, key=lambda n: n.get('submittedAt') or '') if refusals else None -def reviewed_head(pr: dict) -> bool: - """True where one of the reviewer's own reviews covers the current head's commit. +def head_reviews(pr: dict) -> list[dict]: + """The reviewer's own reviews that cover the current head, refusals excluded. A refusal is not coverage. It is a formal review, `state: COMMENTED`, carrying the head's commit and raising no threads, so it satisfies every check a clean pass does and renders a digest identical to one. That is how a pull request of 301 changed files, one over the reviewer's limit, sat one command from merging on a review that never ran. + One list rather than a predicate beside a filter, because coverage and the count of rounds on + the head are read from the same set and a second spelling of it drifts from the first. + """ + head = pr['headRefOid'] + return [n for n in reviewer_nodes(pr, 'reviews') + if (n.get('commit') or {}).get('oid') == head and not refusal_of(n)] + + +def reviewed_head(pr: dict) -> bool: + """True where one of the reviewer's own reviews covers the current head's commit. + The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate rather than a gap: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart. No exit code and no merge decision is taken from the liveness reading. """ - head = pr['headRefOid'] - return any((n.get('commit') or {}).get('oid') == head and not refusal_of(n) - for n in reviewer_nodes(pr, 'reviews')) + return bool(head_reviews(pr)) + + +def is_coverage_line(line: str) -> bool: + """Whether this line is the reviewer stating its file coverage, rather than prose about it.""" + if COVERAGE_BULLET.match(line): + return True + return bool(COVERAGE_SENTENCE.match(line)) and 'changed file' in line.lower() + + +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 is_coverage_line(ln)] + + +def read_coverage(line: str) -> tuple[int, int] | None: + """The (reviewed, changed) counts the line states, or None where they cannot be believed. + + None covers a wording this has no vetted spelling for and a pair that cannot both be true + alike, since each leaves the same question unanswered and each takes the same remedy. A round + reporting 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 + exactly the statement that says something is off. + """ + 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)) if int(reviewed) <= int(changed) else None + + +def coverage_of(node: dict) -> tuple[str, str]: + """This round's coverage reading, with the line it was read from. + + A round making no statement at all reads as unstated rather than a pass or a failure. 28 of + the 332 bodies measured carry an overview and a change list and nothing more, that shape + interleaves with the counted one throughout rather than preceding it, and one pull request + carries both across its two rounds. Failing on it would cry wolf on roughly one review in + twelve, and a guard an agent learns to work around is worse than no guard. Passing it as + coverage is the bug this whole reading exists to remove, one shape over. + + A wording that is coverage-shaped and parses to no counts is the failure whose remedy is + fixing this script, since a gate that allows whatever it does not recognize stops gating as + the wording drifts, which it has done once already for each of the two patterns above. + """ + worst, detail = UNSTATED, '' + for line in coverage_statements(node.get('body') or ''): + counts = read_coverage(line) + state = UNVETTED if counts is None else (FULL if counts[0] == counts[1] else PARTIAL) + if SEVERITY.index(state) < SEVERITY.index(worst): + worst, detail = state, line + return worst, detail + + +def head_coverage(pr: dict) -> tuple[str, str]: + """The worst coverage the rounds covering the current head state, and the line saying it. + + Head-scoped for the reason a refusal is, and unlike a suppressed finding: a partial round is + a statement about one commit's diff, and the push that changes that diff raises a round which + reads the whole of the new one. A refusal needs no exemption of its own here, `head_reviews` + having dropped it already, and reading one would report the round that declined as a wording + this script fails to recognize. + """ + worst, detail = UNSTATED, '' + for node in head_reviews(pr): + state, line = coverage_of(node) + if SEVERITY.index(state) < SEVERITY.index(worst): + worst, detail = state, line + return worst, detail + + +def normal(text: str) -> str: + """A marker reduced to what a vetted list compares: ASCII, single spaces, counts as `(N)`. + + The verdict headings carry a colored circle and the suppressed heading carries its finding + count, so both drift on every review without the section having changed at all. + """ + ascii_only = ''.join(c for c in text if ord(c) < 128) + return re.sub(r'\s+', ' ', re.sub(r'\(\d+\)', '(N)', ascii_only)).strip() + + +def unrecognized_in(body: str) -> list[str]: + """Every marker in one review body this script has no vetted spelling for. + + Read over the body with fenced blocks removed, for the reason the coverage line is: a review + quoting a heading is not a review carrying one, and this script's own pull requests quote + these lists in full. + + A body carrying no heading at all is reported rather than passed, since every one of the 332 + measured opens on a heading and a body with none is a format nothing here has seen. A refusal + is the exemption, being a bare paragraph by design and already classified as one. + """ + # A refusal is a bare paragraph rather than a review body, and `REFUSAL` is its spelling. + # It carries no marker to check, and its own opening line is not a coverage statement. + # Its wording drifting is still caught, since `refusal_of` then stops matching. + # What is left of a drifted refusal is a body with no heading, which is the arm below. + if refusal_of({'body': body}): + return [] + plain = FENCE.sub('', body or '') + headings = [normal(ln) for ln in plain.splitlines() if MARKDOWN_HEADING.match(ln)] + labels = [normal(m.group(1)) for m in map(LABEL_LINE.match, plain.splitlines()) if m] + found = [f'heading: {h}' for h in dict.fromkeys(headings) if h not in VETTED_HEADINGS] + found += [f'summary: {normal(s)}' for s in dict.fromkeys(SUMMARY.findall(plain)) + if normal(s) not in VETTED_SUMMARIES] + found += [f'metadata label: {la}' for la in dict.fromkeys(labels) if la not in VETTED_LABELS] + found += [f'coverage line: {ln}' for ln in coverage_statements(body) + if read_coverage(ln) is None] + if not headings and not refusal_of({'body': body}): + found.append('body carrying no heading at all, which no measured review body does') + return found + + +def unrecognized_shapes(pr: dict) -> list[str]: + """Everything about this pull request's reviewer output that this script cannot read. + + Every round rather than the head's, because this asks whether the reader still understands + the reviewer rather than what the reviewer said about this commit. A shape that arrived one + round ago is one every later round will carry. + """ + found = [] + for node in reviewer_nodes(pr, 'reviews'): + where = ((node.get('commit') or {}).get('oid') or '')[:8] or 'commit unknown' + found += [f'{item} (round {where})' for item in + unrecognized_in(node.get('body') or '')] + return found + reviewer_login_drift(pr) + + +def reviewer_login_drift(pr: dict) -> list[str]: + """Logins that read as this reviewer without being the spelling every query here filters on. + + Read from the authors alone, so the liveness query answers it as well as the full one. That + is what lets the wait stop on a drift rather than poll its whole timeout out against a review + sitting in plain sight, which is the failure this reading exists to name. + """ + logins = {(n.get('author') or {}).get('login') or '' + for field in ('reviews', 'comments') + for n in ((pr.get(field) or {}).get('nodes') or [])} + return [f'reviewer login: {login}, where every query here filters on {REVIEWER}' + for login in sorted(logins) + if login != REVIEWER and READS_AS_REVIEWER.search(login)] + + +def report_verdict(pr: dict) -> int: + """Print the blocking verdict's own status line, and return the exit code it carries. + + The unrecognized shape outranks the coverage one, because a reader that does not understand + the output cannot be trusted about what it read of the diff either. + """ + # A coverage line this cannot parse is one of the shapes below rather than a case of its own. + # It exits here with the remedy that fits it, the reader being what needs the fix. + if unrecognized_shapes(pr): + print(f'status=UNRECOGNIZED_REVIEWER_OUTPUT this script does not know one or more shapes ' + f'in what the reviewer sent, listed above, so nothing it reports about this review ' + f'is trustworthy and the review loop does not close on this digest. File an issue ' + f'on {HUB} naming each shape and quoting the body it came from, since this script ' + f'is hosted there and the fix lands there. Merging this pull request anyway is the ' + f'maintainer\'s decision to take and not this script\'s, and not the agent\'s.') + return 43 + state, _ = head_coverage(pr) + if state == PARTIAL: + print('status=COVERAGE_IS_PARTIAL the review covering the head read fewer files than the ' + 'pull request changed, so part of the diff has no review at all: re-request on this ' + 'head, or split the pull request, rather than merging it') + return 42 + return 0 def live_state(owner: str, repo: str, num: int) -> tuple[str, bool, dict | None]: @@ -426,10 +690,11 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, stalled = stall_of(owner, repo, num, pr) if stalled is None else stalled head = pr['headRefOid'] revs = reviewer_nodes(pr, 'reviews') - # A refusal carries the head and covers nothing, so it counts as a round and not as coverage. - # Reading it as coverage prints `review_on_head=yes` over a review that says it did not run. - on_head = [n for n in revs - if (n.get('commit') or {}).get('oid') == head and not refusal_of(n)] + # `revs` is every round and `on_head` is the ones that reviewed this commit. + # A refusal sits in the first and not the second, being a round that covered nothing. + on_head = head_reviews(pr) + cover, cover_line = head_coverage(pr) + unknown = unrecognized_shapes(pr) threads = pr['reviewThreads']['nodes'] # A deleted account leaves `author` present and null, which `.get('author', {})` returns as # None rather than as the default, so the chained lookup crashes the whole digest. @@ -461,6 +726,12 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. f'repo={owner}/{repo} pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' + # A field of its own beside that one, since a round can cover the head and read part. + # Those two readings are what `review_on_head=yes` alone conflates. + f'coverage={COVERAGE_FIELD[cover]} ' + # Every other field on this line is a reading of the review. + # This one says whether the readings can be believed at all, so it is not a count. + f'shapes={"UNRECOGNIZED" if unknown else "ok"} ' # A field of its own, since `rounds=1 review_on_head=NO` is also what a stale round is. # The two want opposite responses, one a re-request and the other a split pull request. # Upper-case for the reason `NO` is, as a state that blocks a merge is not one to skim. @@ -481,6 +752,22 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, 'it, and the body below is what says which remedy applies') lines += [f' {ln.rstrip()}' for ln in (refusal.get('body') or '').splitlines() if ln.strip()] + if unknown: + # First of the blocks, since it says how far the rest of them can be trusted. + lines.append(f' UNRECOGNIZED REVIEWER OUTPUT ({len(unknown)}): the shapes below are ones ' + 'this script has no reader for, so every other field here is a reading of ' + 'output it does not fully understand and a clean digest does not mean a ' + f'clean review. File an issue on {HUB}, which hosts this script, naming each ' + 'shape and quoting the body it came from, before closing the review loop. ' + 'Whether to merge anyway is the maintainer\'s call rather than the agent\'s') + lines += [f' {item}' for item in unknown] + if cover == PARTIAL: + # The line prints under the marker for the reason a suppressed block does. + # The counts say how much of the diff went unread, and no thread carries them. + lines.append(' COVERAGE IS PARTIAL: the review covering the head read fewer files than ' + 'the pull request changed, so files in the diff have no review at all and ' + 'the remedy is a re-request or a split rather than a merge') + lines.append(f' {cover_line}') if stalled: lines.append(f' REQUEST NOT PICKED UP (requested {stalled}, no copilot_work_started ' 'since): clear the request and re-request, per the runbook') @@ -877,9 +1164,12 @@ def main(argv: list[str] | None = None) -> int: return check_claims(owner, repo, a.number) if a.cmd == 'status': - out, _ = digest(owner, repo, a.number) + # One payload renders the digest and decides the code, for the reason `wait` reads one. + # Fetched twice, a round landing between them prints one pull request and grades another. + pr = gql(Q_FULL, owner, repo, a.number) + out, _ = digest(owner, repo, a.number, pr=pr) print(out) - return 0 + return report_verdict(pr) if a.cmd == 'reply': return reply_to_thread(owner, repo, a.number, a.match, a.body, a.path, a.resolve) @@ -889,10 +1179,14 @@ def main(argv: list[str] | None = None) -> int: start = time.monotonic() pr = gql(Q_LIVE, owner, repo, a.number) done, answer = reviewed_head(pr), answered_outside_review(pr) + # A drifted login matches no filter here, so `done` stays false however long this runs. + # Waiting it out reports a review that landed as one that never did, at the timeout. + # The liveness query carries the authors, so this costs the loop no extra call. + drift = reviewer_login_drift(pr) stalled = '' i = 0 next_pickup = a.pickup_grace - while not done and not answer: + while not done and not answer and not drift: elapsed = time.monotonic() - start # Read the pickup before the clock, so a request nothing acted on reports as itself. # Running the clock out instead would report it exactly as a slow reviewer. @@ -910,6 +1204,7 @@ def main(argv: list[str] | None = None) -> int: # Re-read head each iteration: a push during the wait moves it. pr = gql(Q_LIVE, owner, repo, a.number) done, answer = reviewed_head(pr), answered_outside_review(pr) + drift = reviewer_login_drift(pr) # One payload decides the digest and the exit code together. # Read separately, a review landing between them prints coverage and returns a stalled code. @@ -923,8 +1218,15 @@ def main(argv: list[str] | None = None) -> int: out, _ = digest(owner, repo, a.number, pr=final, stalled=stalled) print(out) print(f'waited={int(time.monotonic()-start)}s') - if reviewed_head(final): - return 0 + # The shape reading comes first and is not gated on coverage of the head. + # `reviewed_head` is itself one of the readings a drift breaks. + # A renamed login matches no filter here, so it reads as no review at all. + # Every arm below then reports a review that landed as a pending one. + # Gating the verdict behind it left the login check unable to reach an exit code. + # The digest above printed `shapes=UNRECOGNIZED` the whole time it did so. + # Coverage of the head is the other half, returning 0 only once the diff is covered too. + if unrecognized_shapes(final) or reviewed_head(final): + return report_verdict(final) # A refusal before an answer, since it names the round that declined where 40 names none. # The digest prints both bodies regardless, so the narrower code costs the reader nothing. if refusing_review(final): diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index adeca42..712c8c2 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -27,7 +27,12 @@ LATE = '2026-08-02T11:00:00Z' -def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = '', +# The shape 28 of the 332 measured bodies carry: an overview, and no count of what was read. +# A body of no text at all is not one of the shapes, and the reader now says so, correctly. +OVERVIEW = '## Pull request overview\n\nThe change is narrow.\n' + + +def review(login: str = pr_review.REVIEWER, oid: str = HEAD, body: str = OVERVIEW, at: str = EARLY) -> dict: return {'author': {'login': login}, 'state': 'COMMENTED', 'commit': {'oid': oid}, 'body': body, 'submittedAt': at} @@ -38,25 +43,40 @@ def comment(login: str = pr_review.REVIEWER, at: str = LATE, return {'author': {'login': login}, 'createdAt': at, 'body': body} +COVERED = ('Copilot reviewed 3 out of 3 changed files in this pull request and generated ' + 'no new comments.') + + def collapsed(heading: str = 'Comments suppressed due to low confidence (1)', - finding: str = 'a.py:12 The retry count is off by one.') -> str: - return (f'Reviewed 3 of 3 changed files.\n\n
\n{heading}\n\n' + finding: str = 'a.py:12 The retry count is off by one.', + covers: str = COVERED) -> str: + """The section as its own `
` wrapper, under the round's own coverage line. + + The coverage line is the reviewer's, quoted from the corpus rather than invented: it sat here + as filler that nothing asserted on, which is one of the two places the shape was already in + this file while no case read it. + """ + return (f'{OVERVIEW}\n{covers}\n\n
\n{heading}\n\n' f'{finding}\n\n
\n') def nested(heading: str = '### Suppressed comments (2)', - finding: str = '**a.py:12**\n* The retry count is off by one.') -> str: + finding: str = '**a.py:12**\n* The retry count is off by one.', + covers: str = '- **Files reviewed:** 1/1 changed files') -> str: """The section as a Markdown heading nested inside the `Review details` wrapper. The live shape as of 2026-08-05: the section is no longer its own `
` wrapper with a matching ``, it is a Markdown heading inside the wrapper that also carries the round's file and effort metadata, which trails the findings rather than preceding them. + + That metadata is where this shape states its coverage, and it is the second spelling of the + line rather than a second wrapper. It sat here as filler that nothing asserted on too. """ return ('### Ready to approve\n\nThe change is narrow.\n\n' '
\nFile summaries\n\n' '| File | Description |\n\n
\n\n' f'
\nReview details\n\n{heading}\n\n{finding}\n\n' - '- **Files reviewed:** 1/1 changed files\n' + f'{covers}\n' '- **Review effort level:** Lite\n
\n') @@ -350,14 +370,15 @@ def test_the_finding_prints_whole_under_a_marker_naming_the_answer(self) -> None def test_a_body_naming_the_block_outside_a_details_wrapper_still_reports(self) -> None: """Reporting zero because the markup moved is the failure the whole case guards.""" - self.answer(payload([review(body='Suppressed comments (1)\n\na.py:12 Off by one.')])) + self.answer(payload([review(body=OVERVIEW + '\nSuppressed comments (1)\n\n' + 'a.py:12 Off by one.')])) out, _ = pr_review.digest('o', 'r', 7) self.assertIn('suppressed=1', out) self.assertIn('a.py:12 Off by one.', out) def test_the_per_file_summary_block_beside_it_is_not_a_finding(self) -> None: """Every real body collapses a file table too, and reporting that is noise, not a finding.""" - body = ('
\nShow a summary per file\n\n' + body = (OVERVIEW + '\n
\nShow a summary per file\n\n' '| File | Description |\n\n
\n' + collapsed()) self.answer(payload([review(body=body)])) out, _ = pr_review.digest('o', 'r', 7) @@ -529,6 +550,403 @@ def test_a_human_review_carrying_the_wording_is_not_the_reviewer_refusing(self) self.assertIn('refusal=no', out) +class TestCoverage(GqlCase): + """The round that covered the head and read part of the diff, which is a clean pass elsewhere. + + Measured over 332 Copilot review bodies on this repository: five rounds across three pull + requests reported reading fewer files than the pull request changed, and all three merged. + One of them changed three files, left one unread across both its rounds, and reported + "generated no comments" each time. + """ + + def digest_for(self, *reviews: dict) -> str: + self.answer(payload(list(reviews))) + out, _ = pr_review.digest('o', 'r', 7) + return out + + def test_each_vetted_spelling_of_a_full_round_reads_as_full(self) -> None: + """The census: four tails on the first spelling, and the `Review details` bullet.""" + for covers in ( + 'Copilot reviewed 7 out of 7 changed files in this pull request and generated ' + 'no new comments.', + 'Copilot reviewed 7 out of 7 changed files in this pull request and generated ' + 'no comments.', + 'Copilot reviewed 7 out of 7 changed files in this pull request and generated ' + '1 comment.', + 'Copilot reviewed 7 out of 7 changed files in this pull request and generated ' + '4 comments.', + '- **Files reviewed:** 7/7 changed files', + ): + with self.subTest(covers=covers[:44]): + self.assertEqual((pr_review.FULL, covers), + pr_review.coverage_of({'body': covers})) + + def test_a_round_that_read_part_of_the_diff_is_a_failure_the_digest_names(self) -> None: + """PR 592's shape: three changed files, one never read, and it merged.""" + line = ('Copilot reviewed 2 out of 3 changed files in this pull request and generated ' + 'no comments.') + out = self.digest_for(review(body=OVERVIEW + '\n' + line)) + self.assertIn('coverage=PARTIAL', out) + self.assertIn('COVERAGE IS PARTIAL', out) + # The counts print, since they are what say how much went unread and no thread carries it. + self.assertIn(line, out) + # The round did cover the head. + # That reading was correct, and it was not the whole reading. + self.assertIn('review_on_head=yes', out) + + def test_the_second_spelling_reports_a_partial_round_too(self) -> None: + """Both spellings appear in the corpus to this day, so neither replaces the other.""" + out = self.digest_for(review(body=nested(covers='- **Files reviewed:** 2/3 changed files'))) + self.assertIn('coverage=PARTIAL', out) + + def test_the_bullet_spelling_is_a_coverage_line_on_its_label_alone(self) -> None: + """Its label is the marker, so dropping the words after the counts is not dropping it. + + Requiring them read a bullet that had lost them as no statement at all, which is a + silent `unstated` over a round that stated its coverage plainly. + """ + # Detected and read, which are two assertions rather than one. + # Asserting only the first left the bare bullet blocking on a line it could read. + for line in ('- **Files reviewed:** 4/4', '- **Files reviewed:** 4/4 changed file', + '- **Files reviewed:** 4/4 changed files'): + with self.subTest(line=line): + self.assertEqual([line], pr_review.coverage_statements(line)) + self.assertEqual((4, 4), pr_review.read_coverage(line)) + self.assertEqual(pr_review.FULL, pr_review.coverage_of({'body': line})[0]) + + def test_a_bullet_carrying_no_counts_still_blocks(self) -> None: + """What is left after the words are optional is a line that states no coverage at all.""" + self.assertIsNone(pr_review.read_coverage('- **Files reviewed:** all of them')) + self.assertEqual(pr_review.UNVETTED, + pr_review.coverage_of({'body': '- **Files reviewed:** all of them'})[0]) + + def test_the_reviewer_s_name_alone_is_not_a_coverage_line(self) -> None: + """The other opener keeps its text requirement, prose opening lines with that name too.""" + self.assertFalse(pr_review.is_coverage_line('Copilot answers a request with a comment.')) + + def test_a_singular_changed_file_reads_rather_than_blocks(self) -> None: + """A one-file round means what it says, and stopping the fleet over an -s is crying wolf.""" + self.assertEqual((1, 1), pr_review.read_coverage( + 'Copilot reviewed 1 out of 1 changed file in this pull request and generated ' + 'no comments.')) + + def test_counts_that_cannot_both_be_true_block_rather_than_read_as_full(self) -> None: + """A round claiming it read more files than were changed is one this is parsing wrongly. + + Read as full coverage it fails open on the very statement saying something is off, which + is the shape of every other failure here. + """ + line = ('Copilot reviewed 8 out of 7 changed files in this pull request and generated ' + 'no comments.') + self.assertIsNone(pr_review.read_coverage(line)) + self.assertEqual(pr_review.UNVETTED, pr_review.coverage_of({'body': line})[0]) + self.assertIn(f'coverage line: {line}', pr_review.unrecognized_in(OVERVIEW + '\n' + line)) + + def test_a_coverage_line_that_parses_to_nothing_names_this_script(self) -> None: + """The wording has drifted once for each of the two patterns beside this one. + + Passing a shape it does not recognize is how a gate stops gating as the wording moves, + so the unrecognized shape is reported and its remedy is stated as fixing this script. + """ + line = 'Copilot reviewed most of the changed files in this pull request.' + out = self.digest_for(review(body=OVERVIEW + '\n' + line)) + self.assertIn('coverage=UNVETTED', out) + # It is one of the unrecognized shapes rather than a report of its own. + # Both say the reader needs fixing, and one of them saying it once is the whole message. + self.assertIn('UNRECOGNIZED REVIEWER OUTPUT (1)', out) + self.assertIn(f'coverage line: {line}', out) + + def test_a_round_stating_no_coverage_at_all_is_unstated_rather_than_a_verdict(self) -> None: + """28 of the 332 bodies are an overview and a change list, and that shape is current. + + It interleaves with the counted one throughout rather than preceding it, and one pull + request carries both across its two rounds, so failing on it would cry wolf on about one + review in twelve. Reporting it as coverage is the bug this reading exists to remove. + """ + body = ('## Pull request overview\n\nThis PR updates the backlog.\n\n' + '**Changes:**\n- Delete the shipped cluster from `TODO.md`.\n') + out = self.digest_for(review(body=body)) + self.assertIn('coverage=unstated', out) + self.assertNotIn('COVERAGE IS PARTIAL', out) + self.assertIn('shapes=ok', out) + + def test_a_refusal_is_exempt_rather_than_an_unrecognized_shape(self) -> None: + """It states no coverage by design, and `head_reviews` has already dropped it. + + Read as a round, every refusal becomes a spurious unvetted-shape failure sitting on top + of the `refusal=YES` that already names the state and its remedy. + """ + out = self.digest_for(review(body=REFUSED)) + self.assertIn('refusal=YES', out) + self.assertIn('coverage=unstated', out) + self.assertIn('shapes=ok', out) + + def test_coverage_is_read_from_the_head_rather_than_from_a_superseded_round(self) -> None: + """A partial round describes one commit's diff, and the push that changes it is answered + by a round reading the whole of the new one.""" + old = (OVERVIEW + '\nCopilot reviewed 2 out of 3 changed files in this pull request ' + 'and generated no comments.') + out = self.digest_for(review(oid=OLD, body=old), + review(body=OVERVIEW + '\n' + COVERED)) + self.assertIn('coverage=full', out) + self.assertNotIn('COVERAGE IS PARTIAL', out) + + def test_the_worst_of_two_rounds_on_one_head_is_what_reports(self) -> None: + """A head carries two rounds through a re-request, and both read the same diff.""" + partial = ('Copilot reviewed 2 out of 3 changed files in this pull request and ' + 'generated no comments.') + self.assertEqual(pr_review.PARTIAL, + pr_review.head_coverage(payload([review(body=COVERED, at=EARLY), + review(body=partial, at=LATE)]))[0]) + + + def test_a_round_that_states_full_coverage_settles_it_over_one_stating_none(self) -> None: + """Unstated is the absence of a statement rather than a bad one, so it loses to a count.""" + self.assertEqual(pr_review.FULL, + pr_review.head_coverage(payload([review(body='## Overview\n\nNarrow.'), + review(body=COVERED)]))[0]) + + def test_prose_mentioning_changed_files_is_not_this_round_stating_its_coverage(self) -> None: + """The false positive the suppressed matcher and the refusal matcher have each had once. + + A review of the pull request that adds this check discusses the wording it adds, and a + body-wide match reads that discussion as the round's own count. + """ + body = ('## Pull request overview\n\nThis PR reads the line saying Copilot reviewed 2 ' + 'out of 3 changed files, so a partial round stops reporting as a clean pass.\n\n' + '- The digest now carries a `coverage` field.\n') + out = self.digest_for(review(body=body)) + self.assertIn('coverage=unstated', out) + + def test_a_quoted_line_in_a_fenced_block_is_not_this_round_stating_its_coverage(self) -> None: + """131 of the 332 bodies carry a fence, and this change puts both spellings in the diff.""" + body = ('### Ready to approve\n\nThe vetted spellings read:\n\n```\n' + 'Copilot reviewed 2 out of 3 changed files in this pull request and generated ' + 'no comments.\n- **Files reviewed:** 4/9 changed files\n```\n\n' + COVERED + '\n') + body = '### Ready to approve\n\n' + body + out = self.digest_for(review(body=body)) + self.assertIn('coverage=full', out) + self.assertNotIn('COVERAGE IS PARTIAL', out) + + def test_a_human_review_carrying_a_coverage_line_is_not_the_reviewer_s_round(self) -> None: + partial = ('Copilot reviewed 2 out of 3 changed files in this pull request and ' + 'generated no comments.') + out = self.digest_for(review(login='ptr727', body=partial), + review(body=OVERVIEW + '\n' + COVERED)) + self.assertIn('coverage=full', out) + + +class TestUnrecognizedShapes(GqlCase): + """A shape this script has no reader for blocks, rather than being read past. + + 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: + a suppressed heading reworded, a suppressed section moved inside another wrapper, and a + coverage line nothing parsed. Each was caught after it had already reported a clean pass. + + The inventory is measured rather than imagined. Over the same 332 review bodies, with fenced + blocks removed and text reduced to ASCII, the whole corpus is 7 headings, 6 summaries and 3 + metadata labels, and every body carries at least one of them. + """ + + def digest_for(self, *reviews: dict) -> str: + self.answer(payload(list(reviews))) + out, _ = pr_review.digest('o', 'r', 7) + return out + + def test_every_vetted_marker_together_reads_as_recognized(self) -> None: + """The corpus shape in one body, so the lists are held against what they were built from.""" + self.assertEqual([], pr_review.unrecognized_in(nested())) + self.assertEqual([], pr_review.unrecognized_in(collapsed())) + self.assertEqual([], pr_review.unrecognized_in(OVERVIEW)) + + def test_a_heading_that_is_not_in_the_inventory_blocks(self) -> None: + """A renamed section is one the reader stops finding, which it reports as nothing there.""" + out = self.digest_for(review(body=OVERVIEW + '\n### Confidence assessment\n\nHigh.\n')) + self.assertIn('shapes=UNRECOGNIZED', out) + self.assertIn('heading: ### Confidence assessment', out) + + def test_a_details_summary_that_is_not_in_the_inventory_blocks(self) -> None: + """The suppressed section has already moved between wrappers once.""" + body = OVERVIEW + '\n
\nWithheld findings\n\nx\n
\n' + out = self.digest_for(review(body=body)) + self.assertIn('summary: Withheld findings', out) + + def test_a_metadata_label_that_is_not_in_the_inventory_blocks(self) -> None: + """The coverage line arrived as one of these bullets, so the next reading may too.""" + out = self.digest_for(review(body=nested() + '\n- **Confidence:** high\n')) + self.assertIn('metadata label: Confidence', out) + + def test_a_body_carrying_no_heading_at_all_blocks(self) -> None: + """Every measured body opens on a heading, so one with none is a format never seen. + + This is the arm that catches a rewrite wholesale rather than marker by marker, and it is + also what catches the refusal wording drifting, since a refusal stops being exempt. + """ + self.assertIn('body carrying no heading at all', + ' '.join(pr_review.unrecognized_in('Looks good to me.'))) + self.assertIn('body carrying no heading at all', + ' '.join(pr_review.unrecognized_in(''))) + + def test_a_refusal_is_exempt_because_it_is_already_a_vetted_shape(self) -> None: + """It is a bare paragraph by design, and `refusal=YES` already names its remedy.""" + self.assertEqual([], pr_review.unrecognized_in(REFUSED)) + out = self.digest_for(review(body=REFUSED)) + self.assertIn('shapes=ok', out) + self.assertIn('refusal=YES', out) + + def test_a_refusal_whose_wording_drifted_stops_being_exempt_and_blocks(self) -> None: + """The exemption is the pattern, so losing the pattern loses the exemption, not the check. + + This is the failure the refusal check was built for arriving one rewording later: the + body would read as an ordinary review carrying the head and raising nothing. + """ + drifted = 'Copilot has declined to review this pull request because it is too large.' + self.assertEqual('', pr_review.refusal_of({'body': drifted})) + out = self.digest_for(review(body=drifted)) + self.assertIn('shapes=UNRECOGNIZED', out) + self.assertIn('refusal=no', out) + + def test_the_emoji_and_the_finding_count_are_normalized_rather_than_vetted(self) -> None: + """Both change without the section changing, so comparing them raw blocks every review.""" + for heading in ('### \U0001F7E2 Ready to approve', '### \U0001F7E1 Changes recommended', + '### Suppressed comments (4)', '### Suppressed comments (11)'): + with self.subTest(heading=heading): + self.assertEqual([], pr_review.unrecognized_in(f'{heading}\n\nText.\n')) + + def test_a_marker_quoted_in_a_fenced_block_is_not_one_the_review_carries(self) -> None: + """This change publishes the inventory, so a review of it quotes the lot back.""" + body = (OVERVIEW + '\nThe vetted headings are:\n\n```\n### Confidence assessment\n' + '- **Confidence:** high\n```\n') + self.assertEqual([], pr_review.unrecognized_in(body)) + + def test_a_reviewer_login_that_is_not_the_one_every_query_filters_on_blocks(self) -> None: + """A rename leaves every filter here matching nothing, which reads as no review at all. + + That is the quietest drift of the lot: the digest reports `rounds=0 review_on_head=NO` + over a review that landed, and a wait polls out its whole timeout against it. + """ + pr = payload([review(login='copilot-code-review-agent')]) + found = ' '.join(pr_review.unrecognized_shapes(pr)) + self.assertIn('reviewer login: copilot-code-review-agent', found) + + def test_the_coding_agent_and_a_human_are_not_the_reviewer_renamed(self) -> None: + """`copilot-swe-agent` edits code and is not this reviewer under another name.""" + for login in ('copilot-swe-agent', 'ptr727', 'codecov[bot]', 'dependabot[bot]'): + with self.subTest(login=login): + self.assertEqual([], pr_review.unrecognized_shapes(payload([review(login=login)]))) + + def test_the_block_names_the_hub_and_leaves_the_merge_to_the_maintainer(self) -> None: + """The remedy is an issue where the reader lives, and the merge is not this script's call.""" + out = self.digest_for(review(body=OVERVIEW + '\n### Confidence assessment\n')) + self.assertIn('UNRECOGNIZED REVIEWER OUTPUT (1)', out) + self.assertIn(f'File an issue on {pr_review.HUB}', out) + self.assertIn("maintainer's call rather than the agent's", out) + + def test_every_round_is_read_rather_than_the_head_s(self) -> None: + """This asks whether the reader still understands the reviewer, which is not head-scoped. + + A shape that arrived one round ago is one every later round carries, so waiting for it to + reach the head is waiting through the rounds it is already misreading. + """ + out = self.digest_for(review(oid=OLD, body=OVERVIEW + '\n### Confidence assessment\n'), + review(body=OVERVIEW)) + self.assertIn('shapes=UNRECOGNIZED', out) + self.assertIn(f'(round {OLD[:8]})', out) + + +class TestCoverageExitCodes(GqlCase): + """`status` returned 0 unconditionally, so a partial round reported as a covered head.""" + + def setUp(self) -> None: + self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + def partial(self) -> dict: + return review(body=OVERVIEW + '\nCopilot reviewed 2 out of 3 changed files in this ' + 'pull request and generated no comments.') + + def test_status_exits_forty_two_on_a_round_that_read_part_of_the_diff(self) -> None: + self.answer(payload([self.partial()])) + self.assertEqual(42, pr_review.main(['status', '7', '--repo', 'o/r'])) + self.assertIn('status=COVERAGE_IS_PARTIAL', self.out.getvalue()) + + def test_status_exits_forty_three_on_a_wording_it_does_not_read(self) -> None: + self.answer(payload([review(body=OVERVIEW + + '\nCopilot reviewed some of the changed files.')])) + self.assertEqual(43, pr_review.main(['status', '7', '--repo', 'o/r'])) + out = self.out.getvalue() + self.assertIn('status=UNRECOGNIZED_REVIEWER_OUTPUT', out) + self.assertIn('coverage=UNVETTED', out) + + def test_status_still_exits_zero_on_a_full_round_and_on_a_silent_one(self) -> None: + """Failing the silent shape would fail roughly one review in twelve on this repository.""" + for body in (OVERVIEW + '\n' + COVERED, nested(), OVERVIEW): + with self.subTest(body=body[:40]): + self.answer(payload([review(body=body)])) + self.assertEqual(0, pr_review.main(['status', '7', '--repo', 'o/r'])) + + def test_wait_carries_the_same_codes_rather_than_ending_on_a_partial_round(self) -> None: + """`reviewed_head` is what decided its zero, and coverage of the head is not coverage + of the diff.""" + self.answer(payload([self.partial()])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(42, pr_review.main(['wait', '7', '--repo', 'o/r'])) + self.assertIn('coverage=PARTIAL', self.out.getvalue()) + + def test_status_and_wait_both_exit_forty_three_on_an_unrecognized_shape(self) -> None: + """Neither may report a clean pass over output the reader does not understand.""" + self.answer(payload([review(body=OVERVIEW + '\n### Confidence assessment\n')])) + self.assertEqual(43, pr_review.main(['status', '7', '--repo', 'o/r'])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(43, pr_review.main(['wait', '7', '--repo', 'o/r'])) + self.assertIn('status=UNRECOGNIZED_REVIEWER_OUTPUT', self.out.getvalue()) + + def test_an_unrecognized_shape_outranks_the_partial_coverage_code(self) -> None: + """A reader that does not understand the output cannot be believed about the diff either.""" + body = (OVERVIEW + '\n### Confidence assessment\n\nCopilot reviewed 2 out of 3 changed ' + 'files in this pull request and generated no comments.\n') + self.answer(payload([review(body=body)])) + self.assertEqual(43, pr_review.main(['status', '7', '--repo', 'o/r'])) + + def test_a_drifted_login_reaches_the_code_rather_than_timing_out_as_pending(self) -> None: + """The gate could not fire for the one drift it was written to catch. + + `reviewed_head` filters on the login, so a renamed reviewer leaves it false, and gating + the verdict behind it meant the login check never reached an exit code. The digest + printed `shapes=UNRECOGNIZED` and the wait returned 30, which is the digest disagreeing + with the code, and an automated reader settles that by believing the code. + """ + pr = payload([review(login='copilot-code-review-agent')]) + self.assertFalse(pr_review.reviewed_head(pr)) + self.answer(pr) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(43, pr_review.main(['wait', '7', '--repo', 'o/r', + '--timeout', '600'])) + # Terminal, so it stops rather than polling out the timeout against a landed review. + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn('status=UNRECOGNIZED_REVIEWER_OUTPUT', out) + self.assertIn('shapes=UNRECOGNIZED', out) + + def test_a_shape_on_a_round_that_covers_no_head_still_reaches_the_code(self) -> None: + """The same gap with the body reader rather than the login, since both sit behind it.""" + self.answer(payload([review(oid=OLD, body=OVERVIEW + '\n### Confidence assessment\n')])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(43, pr_review.main(['wait', '7', '--repo', 'o/r', '--timeout', '0'])) + + def test_a_pending_round_with_nothing_unrecognized_still_reports_pending(self) -> None: + """The gate is not allowed to swallow the ordinary wait, which is most of them.""" + self.answer(payload([review(oid=OLD)])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(30, pr_review.main(['wait', '7', '--repo', 'o/r', '--timeout', '0'])) + + def test_a_refusal_still_exits_forty_one_rather_than_on_a_coverage_code(self) -> None: + """The refusal names the round that declined, where a coverage code names a round.""" + self.answer(payload([review(body=REFUSED)])) + with mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(41, pr_review.main(['wait', '7', '--repo', 'o/r', '--timeout', '0'])) + + class TestDigestReportsTheAnswer(GqlCase): def test_the_comment_prints_whole_under_a_marker_naming_it_terminal(self) -> None: """Its wording is what separates a refusal from a remark, so it is not truncated.""" @@ -1244,6 +1662,28 @@ def test_the_refusal_pattern_is_the_runbook_alternation(self) -> None: # The published filter is single-quoted, which no spelling of the apostrophe survives. self.assertNotIn("'", pr_review.REFUSAL.pattern) + def test_the_vetted_coverage_spellings_are_the_ones_the_runbook_publishes(self) -> None: + """The wording drifts, so the vetted list is read out of the runbook rather than recalled. + + The two published lines are pulled out with the script's own opener and handed to its own + parser, which holds the pair in step in both directions: a spelling the runbook adds and + this cannot read fails here, and so does one this reads that the runbook never named. + """ + text = RUNBOOK.read_text(encoding='utf-8') + published = [ln.strip() for ln in text.splitlines() + if pr_review.is_coverage_line(ln)] + self.assertEqual(2, len(published), published) + for line in published: + with self.subTest(line=line[:44]): + self.assertIsNotNone(pr_review.read_coverage(line)) + # The published filter is single-quoted in the shell, which no apostrophe survives. + self.assertNotIn("'", pr_review.COVERAGE_COUNTS.pattern) + + def test_the_runbook_names_the_partial_round_as_a_state_that_blocks_a_merge(self) -> None: + """A verify step reading `commit.oid` alone is what let five partial rounds merge.""" + text = RUNBOOK.read_text(encoding='utf-8') + self.assertIn('Coverage of the head is not coverage of the diff', text) + def test_the_only_writes_are_the_two_the_reply_path_owns(self) -> None: """One writing path, and everything else that changes state stays out of this script.