From 96a25e7e40108756d96696a5a046643b2a71ba4a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:03:15 -0700 Subject: [PATCH 1/3] Let a Copilot File Table Carry Forward on an Identical File Set A full file table on an earlier round now counts as coverage of a later head whose rounds carry no table, under the bound a coverage statement already carries under: the pull request changes exactly the same set of files at both commits. Only the newest round carrying a table is consulted, and only where no round covering the head carries one of its own. A partial table, a partial on record anywhere on the pull request, or a change set that moved or could not be read still goes to the maintainer. status prints coverage=carried:table and names the round and the delta. The Merge Gate text in GOVERNANCE.md, pr-review-conduct, the Copilot runbook, and scripts/README.md states the carry. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 5 +- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 5 +- .github/copilot-instructions.md | 2 +- .github/skills/pr-review-conduct/SKILL.md | 5 +- GOVERNANCE.md | 2 +- scripts/README.md | 2 +- scripts/pr_review.py | 110 +++++++++++++++--- tests/test_pr_review.py | 97 +++++++++++++-- 9 files changed, 201 insertions(+), 29 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 90a1a98ca..4de8f2a29 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -48,7 +48,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does stands in under the bound a statement + carries under, the pull request changing the same set of files at both commits, which reads as + `coverage=carried:table`. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 7916f7668..3463a3d78 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -0331a2ae5b800214 +de557a036d5dd212 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 90a1a98ca..4de8f2a29 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -48,7 +48,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does stands in under the bound a statement + carries under, the pull request changing the same set of files at both commits, which reads as + `coverage=carried:table`. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b78d5b376..e92312e11 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -28,7 +28,7 @@ For every review: The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. -A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 90a1a98ca..4de8f2a29 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -48,7 +48,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does stands in under the bound a statement + carries under, the pull request changing the same set of files at both commits, which reads as + `coverage=carried:table`. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 32b2072f9..6dac2599b 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -212,7 +212,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. +The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. diff --git a/scripts/README.md b/scripts/README.md index 4c9e2a1eb..16ed43662 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -192,7 +192,7 @@ The digest reports the completed head review's effective effort as `lite`, `bala `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Where no round covering the head carries a table of its own, the newest round that does stands in under that same change-set bound, reading as `carried:table`. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 was **head-scoped**, unlike a suppressed finding and like a refusal, on the reasoning that 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. The carry is what qualifies that: five of ten rounds in the second format raise no statement at all, so the push often raises a round that reads nothing back, and a partial carries to the head under the same change-set bound a full one does, which is a 42 decided by a round that is not on the head. Where one head carries two rounds through a re-request the worst of them still 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. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8d676bb68..ddcaff8b5 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -21,7 +21,9 @@ Exit 0 = no Copilot review covers the head yet, or every output shape is recognized and full diff coverage is stated for this head, by the round covering it or by the carry below, or where nothing states or carries any, a round covering the head names - exactly the changed files in its own file table, `coverage=table` in the digest. + exactly the changed files in its own file table, `coverage=table` in the digest, or + where no round covering the head carries a table, the newest round that does names + them under the same change-set bound, `coverage=carried:table`. `review_on_head` and `rounds=` in the digest name Copilot's own coverage specifically, the reviewer this script requests and waits for, never "no review of any kind covers this head": a @@ -47,7 +49,8 @@ set, so this covers three states: nothing ever stated coverage, the round that did describes a different set of changed files, or that comparison could not be read. Past those, a round covering the head whose own table names exactly the changed - files stands in, and only where no Copilot round on the pull request, on any commit, + files stands in, or where none carries a table, the newest round that does under + the same change-set bound, and only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, and the whole review history is in view, since over the rounds measured the table names the whole changed set on partial rounds too. The digest names why no table stood in. @@ -482,6 +485,7 @@ def strip_fences( # It ranks nowhere in `SEVERITY`, since it is one of the four wearing another round's name. CARRIED = "carried" TABLE = "table" +NO_HEAD_TABLE = "no round covering the head carries a file table of its own" 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. @@ -2039,7 +2043,7 @@ def partial_shaped(pr: dict) -> str: return "" -def table_shortfall(pr: dict) -> str: +def table_shortfall(pr: dict, named: list[str] | None = None) -> str: """Why no round covering the head names exactly this pull request's changed files, or "". Empty where the table stands in, which is the coverage reading `table_covers` gives. @@ -2047,6 +2051,11 @@ def table_shortfall(pr: dict) -> str: differ: a missing or mismatched table may be answered by another round, a changed-file list longer than the window this reads by a push bringing it back within it, and a partial on record by nothing but the maintainer's reading. + + `named` is an earlier round's table where `table_reading` matches a carried one, and the + head's own table otherwise. Every other check reads the pull request either way, since a + partial on record or a cut-short list refuses a carried table exactly as it refuses one on + the head. """ if reviews_truncated(pr): return ( @@ -2058,9 +2067,10 @@ def table_shortfall(pr: dict) -> str: f"a Copilot round on this pull request states or appears to state partial coverage, " f"'{partial}', so the maintainer reads it rather than the table" ) - named = head_table(pr) + if named is None: + named = head_table(pr) if not named: - return "no round covering the head carries a file table of its own" + return NO_HEAD_TABLE files = pr.get("files") if files is None: return "the changed-file list is absent from the query, so the table has nothing to match" @@ -2125,6 +2135,66 @@ def table_covers(pr: dict) -> bool: return not table_shortfall(pr) +def carried_table(pr: dict) -> tuple[list[str], str] | None: + """The newest round on this pull request carrying a file table, its table, and its commit. + + None where no round carries one. A refusal is skipped for the reason `carried_coverage` + skips it, being a round that read nothing. + """ + tabled = [ + node + for node in reviewer_nodes(pr, "reviews") + if not refusal_of(node) and file_table(node.get("body") or "") + ] + newest = newest_of(tabled) + if newest is None: + return None + return file_table(newest.get("body") or ""), (newest.get("commit") or {}).get("oid") or "" + + +def table_reading(owner: str, repo: str, pr: dict) -> tuple[str, str]: + """Whether a file table stands in for this head's coverage, as the commit it was read on and why not. + + The shortfall is empty where a table stands in. The commit is empty where the head's own + table does, and names the earlier round where that round's table carries. + + A table carries under the bound a statement does, the pull request changing exactly the same + set of files at both commits, which `carry_holds` reads. Three pull requests in one session + each had a full table on their first round and a re-review on the next head carrying no + table, with the file set unchanged between the two, and the maintainer accepted the first + round's table every time. Only the newest round carrying a table is consulted, and only where + no round covering the head carries one of its own, since a head table that misses the diff is + the newer reading and an older one matching does not overrule it. + """ + shortfall = table_shortfall(pr) + if shortfall != NO_HEAD_TABLE: + return "", shortfall + earlier = carried_table(pr) + if earlier is None: + return "", f"{NO_HEAD_TABLE}, and no earlier round carries one either" + named, carried_from = earlier + if not carried_from: + return "", ( + f"{NO_HEAD_TABLE}, and the newest round that does names no commit, so there is no " + f"change set of its own to compare" + ) + whose = f"{NO_HEAD_TABLE}, while the newest round that does is on {carried_from[:8]}" + if why := table_shortfall(pr, named): + return carried_from, f"{whose}, where {why}" + kept = carry_holds(owner, repo, pr, carried_from) + if kept is None: + return carried_from, ( + f"{whose}, and the change set could not be read at both commits, so its table is " + f"not carried" + ) + if not kept: + return carried_from, ( + f"{whose}, and the pull request changes a different set of files at the two " + f"commits, so its table describes a diff this head no longer has" + ) + return carried_from, "" + + def badge_text(match: re.Match[str]) -> str: """The severity a badge states, taken from whichever quoting its `alt` uses. @@ -2297,7 +2367,7 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: carried = None if carried is not None: state, line, _from_head = carried - if state == UNSTATED and table_covers(pr): + if state == UNSTATED and not table_reading(owner, repo, pr)[1]: state = TABLE if state == PARTIAL: # The unread count comes from the line that decided PARTIAL, never from past rounds. @@ -2345,7 +2415,8 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: "states none is the ordinary shape of the second overview format and carries the " "newest round that states some forward, bounded on the change set, and failing " "that, a round covering the head whose own file table names exactly the changed " - "files stands in, where no round on the pull request states or appears to state " + "files stands in, or where none carries a table, the newest round that does, under " + "the same change-set bound, where no round on the pull request states or appears to state " "partial coverage. Reaching here means no table stood in, for the reason the digest " "above names, and also one of three things it says which of: no round ever stated " "coverage, the round that did describes a different set of changed files than this " @@ -2934,7 +3005,10 @@ def digest( cover, cover_line, _carried_from = candidate else: carried = None - if cover == UNSTATED and on_head and table_covers(pr): + table_from, table_short = ( + table_reading(owner, repo, pr) if cover == UNSTATED and on_head else ("", "") + ) + if cover == UNSTATED and on_head and not table_short: cover = TABLE unknown = unrecognized_shapes(pr) threads = pr["reviewThreads"]["nodes"] @@ -3051,7 +3125,7 @@ def digest( # Those two readings are what `review_on_head=yes` alone conflates. # `carried:` prefixes the state rather than replacing it. # What the earlier round said is the reading, and which round said it is the provenance. - f"coverage={CARRIED + ':' if carried else ''}{COVERAGE_FIELD[cover]} " + f"coverage={CARRIED + ':' if carried or (cover == TABLE and table_from) else ''}{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'} " @@ -3176,14 +3250,22 @@ def digest( f"{carried_from[:8] or 'a commit this cannot name'}, and {carried_since}, but {why}" ) if cover == TABLE: + count = len(changed_paths(pr)[0]) + files = f"{count} changed file{'' if count == 1 else 's'}" lines.append( - f" COVERAGE IS READ FROM THE FILE TABLE: no round states coverage of this head and " - f"none carries to it, and a round covering it names exactly the " - f"{len(changed_paths(pr)[0])} changed " - f"file{'' if len(changed_paths(pr)[0]) == 1 else 's'} in its own table" + " COVERAGE IS READ FROM THE FILE TABLE: no round states coverage of this head and " + "none carries to it, and " + + ( + f"no round covering it carries a file table, so the newest round that does, on " + f"{table_from[:8]}, carries its table, which names exactly the {files}, the " + f"pull request changing that same set at both commits, and " + f"{delta_since(owner, repo, table_from, head)}" + if table_from + else f"a round covering it names exactly the {files} in its own table" + ) ) elif cover == UNSTATED and on_head: - lines.append(f" NO FILE TABLE STANDS IN: {table_shortfall(pr)}") + lines.append(f" NO FILE TABLE STANDS IN: {table_short}") 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. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index cc261e857..ab1d2b9f5 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -2592,14 +2592,8 @@ def test_a_badge_missing_its_alt_does_not_swallow_the_next_entry(self) -> None: self.assertNotIn("", pr_review.normal(f"{single} {MISSED_TITLE}")) -class TestCoverageCarriesForward(GqlCase): - """An earlier round's coverage statement stands until something changes it. - - Five of ten rounds measured in the second overview format state no coverage, and the two - measured on one drive were re-reviews of a one-line and a four-file delta. Blocking each of - those asks for a re-request that produced the marker in one of the four measured, so the - block clears by chance rather than by asking and a reader learns to route around it. - """ +class CarryCase(GqlCase): + """The rounds and compares both carries read, a statement's and a table's.""" NONE = OVERVIEW + "\n**Findings:** None" FULL = OVERVIEW + "\n" @@ -2635,6 +2629,16 @@ def rest(path: str, jq: str | None = None) -> subprocess.CompletedProcess: return mock.patch.object(pr_review, "gh_rest", side_effect=rest) + +class TestCoverageCarriesForward(CarryCase): + """An earlier round's coverage statement stands until something changes it. + + Five of ten rounds measured in the second overview format state no coverage, and the two + measured on one drive were re-reviews of a one-line and a four-file delta. Blocking each of + those asks for a re-request that produced the marker in one of the four measured, so the + block clears by chance rather than by asking and a reader learns to route around it. + """ + def test_the_newest_round_that_states_any_coverage_is_the_one_carried(self) -> None: pr = self.rounds( review(oid=OLD, body=self.PART, at=EARLY, rid="PRR_a"), @@ -2954,6 +2958,83 @@ def test_a_round_stating_coverage_on_the_head_carries_nothing(self) -> None: self.assertNotIn("carried", out) +class TestFileTableCarriesForward(CarryCase): + """An earlier round's full file table carries to a head whose rounds carry none. + + Three pull requests in one session each had a table naming every changed file on round one + and a re-review on the next head carrying no table, the file set unchanged, and the + maintainer accepted the first table every time. The bound is the one a statement carries + under, the same set of changed files at both commits. + """ + + def tabled(self, head_body: str | None = None, files: list[str] | None = None) -> dict: + changed = ["a.py", "b.py"] if files is None else files + return payload( + [ + review(oid=OLD, body=summarized(["a.py", "b.py"], covers=""), at=EARLY, rid="A"), + review(oid=HEAD, body=self.NONE if head_body is None else head_body, at=LATE), + ], + files=changed, + ) + + def verdict(self, pr: dict) -> tuple[int, str]: + with contextlib.redirect_stdout(io.StringIO()): + code = pr_review.report_verdict(pr, "o", "r") + out, _ = pr_review.digest("o", "r", 7, pr=pr) + return code, out + + def test_a_full_table_on_an_unchanged_file_set_closes_the_head(self) -> None: + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["b.py", "a.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(0, code) + self.assertIn("coverage=carried:table ", out) + self.assertIn(f"the newest round that does, on {OLD[:8]}, carries its table", out) + self.assertIn("names exactly the 2 changed files", out) + + def test_a_moved_file_set_keeps_the_table_where_it_was(self) -> None: + with self.compare(**{OLD: ["a.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(45, code) + self.assertIn("coverage=unstated ", out) + self.assertIn("a diff this head no longer has", out) + + def test_a_file_set_that_could_not_be_read_carries_no_table(self) -> None: + with self.compare(**{HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(45, code) + self.assertIn("could not be read at both commits, so its table is not carried", out) + + def test_a_partial_table_does_not_carry(self) -> None: + """A table that leaves out a changed file is partial, carried or not.""" + pr = self.tabled(files=["a.py", "b.py", "c.py"]) + with self.compare(**{OLD: ["a.py", "b.py", "c.py"], HEAD: ["a.py", "b.py", "c.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn(f"is on {OLD[:8]}, where the table leaves out c.py", out) + + def test_a_partial_on_record_keeps_a_carried_table_out(self) -> None: + pr = self.tabled() + pr["reviews"]["nodes"].insert(0, review(oid=OLD, body=self.PART, at=EARLY, rid="P")) + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(42, code) + self.assertNotIn("FILE TABLE", out) + + def test_a_head_table_that_misses_the_diff_is_not_overruled_by_an_earlier_one(self) -> None: + pr = self.tabled(head_body=summarized(["a.py"], covers="")) + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("NO FILE TABLE STANDS IN: the table leaves out b.py", out) + self.assertNotIn("/compare/", " ".join(self.calls)) + + def test_no_table_anywhere_says_so(self) -> None: + pr = self.rounds(review(oid=HEAD, body=self.NONE)) + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("no earlier round carries one either", out) + + class TestTheDeltaSinceTheCarriedRound(unittest.TestCase): """What changed between the round that stated coverage and the head, reported not judged.""" From 486ef067c00df0719d8cdac8917f7fc423c61feb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:10:43 -0700 Subject: [PATCH 2/3] Close the Table-Carry Review Gaps Test the partial and truncated-history guards on the carried-table path itself, state the exact-match condition beside the change-set bound on every surface, fold the dead table_covers reasoning into table_reading, and correct its docstring. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 7 +-- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 7 +-- .github/copilot-instructions.md | 2 +- .github/skills/pr-review-conduct/SKILL.md | 7 +-- GOVERNANCE.md | 2 +- scripts/README.md | 2 +- scripts/pr_review.py | 53 ++++++++----------- tests/test_pr_review.py | 19 +++++-- 9 files changed, 55 insertions(+), 46 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 4de8f2a29..6eff6f102 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -49,9 +49,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head - carries a table of its own, the newest round that does stands in under the bound a statement - carries under, the pull request changing the same set of files at both commits, which reads as - `coverage=carried:table`. The table stands in only where no + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 3463a3d78..4025d7e15 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -de557a036d5dd212 +b526db5d9b32632a diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 4de8f2a29..6eff6f102 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -49,9 +49,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head - carries a table of its own, the newest round that does stands in under the bound a statement - carries under, the pull request changing the same set of files at both commits, which reads as - `coverage=carried:table`. The table stands in only where no + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e92312e11..4fbb79f20 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -28,7 +28,7 @@ For every review: The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. -A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 4de8f2a29..6eff6f102 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -49,9 +49,10 @@ visible comments, routinely still carries a finding nobody has answered. Treatin file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head - carries a table of its own, the newest round that does stands in under the bound a statement - carries under, the pull request changing the same set of files at both commits, which reads as - `coverage=carried:table`. The table stands in only where no + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. The coverage this item diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 6dac2599b..51e8ab1bc 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -212,7 +212,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. +The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. diff --git a/scripts/README.md b/scripts/README.md index 16ed43662..d5fb06fcc 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -192,7 +192,7 @@ The digest reports the completed head review's effective effort as `lite`, `bala `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Where no round covering the head carries a table of its own, the newest round that does stands in under that same change-set bound, reading as `carried:table`. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under that same change-set bound, reading as `carried:table`. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 was **head-scoped**, unlike a suppressed finding and like a refusal, on the reasoning that 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. The carry is what qualifies that: five of ten rounds in the second format raise no statement at all, so the push often raises a round that reads nothing back, and a partial carries to the head under the same change-set bound a full one does, which is a 42 decided by a round that is not on the head. Where one head carries two rounds through a re-request the worst of them still 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. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ddcaff8b5..32e2163cf 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -49,8 +49,8 @@ set, so this covers three states: nothing ever stated coverage, the round that did describes a different set of changed files, or that comparison could not be read. Past those, a round covering the head whose own table names exactly the changed - files stands in, or where none carries a table, the newest round that does under - the same change-set bound, and only where no Copilot round on the pull request, on any commit, + files stands in, or where none carries a table, the newest round that does, its table + naming exactly the changed files, under the same change-set bound, and only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, and the whole review history is in view, since over the rounds measured the table names the whole changed set on partial rounds too. The digest names why no table stood in. @@ -482,7 +482,6 @@ def strip_fences( # A round that did state full coverage settles the question over one that stated nothing. UNVETTED, PARTIAL, FULL, UNSTATED = "unvetted", "partial", "full", "unstated" # Not a state a round reports, but the reading that carries an earlier round's forward. -# It ranks nowhere in `SEVERITY`, since it is one of the four wearing another round's name. CARRIED = "carried" TABLE = "table" NO_HEAD_TABLE = "no round covering the head carries a file table of its own" @@ -2046,7 +2045,7 @@ def partial_shaped(pr: dict) -> str: def table_shortfall(pr: dict, named: list[str] | None = None) -> str: """Why no round covering the head names exactly this pull request's changed files, or "". - Empty where the table stands in, which is the coverage reading `table_covers` gives. + Empty where the table stands in, which is the coverage reading `table_reading` gives. Otherwise the reason, which the digest prints under an unstated head, since the remedies differ: a missing or mismatched table may be answered by another round, a changed-file list longer than the window this reads by a push bringing it back within it, and a partial on @@ -2111,30 +2110,6 @@ def table_shortfall(pr: dict, named: list[str] | None = None) -> str: ) -def table_covers(pr: dict) -> bool: - """Whether a round covering the head names exactly this pull request's changed files in its table. - - The reason it does not is `table_shortfall`, which this is the empty case of. - - The coverage reading for a head that no round states coverage on and no earlier statement - carries to. Copilot's Balanced review effort, the default since 2026-09-28, writes the second - overview format with a file table and almost never a coverage statement, whatever the review - instructions ask, so without this reading no pull request reviewed at that effort could close - its loop. - - It is weaker than a statement, and `table_against_diff` says why: over the rounds measured, - the table names the whole changed set on partial rounds as well as full ones. So it stands - in only where nothing on record says any round read part of a diff: no Copilot round on the - pull request, on any commit, carries a coverage-shaped count that is not a full one, quoted - or not, and the whole review history is in view. A pull request that ever had a partial - round goes to the maintainer, since whether that partial still describes this diff is what - a carry bound refusing, failing, or never reaching it cannot settle. A table naming a file - the diff does not carry, or leaving one out, is not this reading either, and neither is a - changed-file list the query cut short or returned malformed. - """ - return not table_shortfall(pr) - - def carried_table(pr: dict) -> tuple[list[str], str] | None: """The newest round on this pull request carrying a file table, its table, and its commit. @@ -2156,7 +2131,24 @@ def table_reading(owner: str, repo: str, pr: dict) -> tuple[str, str]: """Whether a file table stands in for this head's coverage, as the commit it was read on and why not. The shortfall is empty where a table stands in. The commit is empty where the head's own - table does, and names the earlier round where that round's table carries. + table is the one read, and names the earlier round whenever one was consulted, whether or not + its table carries. + + This is the coverage reading for a head that no round states coverage on and no earlier + statement carries to. Copilot's Balanced review effort, the default since 2026-09-28, writes + the second overview format with a file table and almost never a coverage statement, whatever + the review instructions ask, so without this reading no pull request reviewed at that effort + could close its loop. + + It is weaker than a statement, and `table_against_diff` says why: over the rounds measured, + the table names the whole changed set on partial rounds as well as full ones. So it stands + in only where nothing on record says any round read part of a diff: no Copilot round on the + pull request, on any commit, carries a coverage-shaped count that is not a full one, quoted + or not, and the whole review history is in view. A pull request that ever had a partial + round goes to the maintainer, since whether that partial still describes this diff is what + a carry bound refusing, failing, or never reaching it cannot settle. A table naming a file + the diff does not carry, or leaving one out, is not this reading either, and neither is a + changed-file list the query cut short or returned malformed. A table carries under the bound a statement does, the pull request changing exactly the same set of files at both commits, which `carry_holds` reads. Three pull requests in one session @@ -2415,7 +2407,8 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: "states none is the ordinary shape of the second overview format and carries the " "newest round that states some forward, bounded on the change set, and failing " "that, a round covering the head whose own file table names exactly the changed " - "files stands in, or where none carries a table, the newest round that does, under " + "files stands in, or where none carries a table, the newest round that does, its table " + "naming exactly the changed files, under " "the same change-set bound, where no round on the pull request states or appears to state " "partial coverage. Reaching here means no table stood in, for the reason the digest " "above names, and also one of three things it says which of: no round ever stated " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index ab1d2b9f5..35b683cac 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3013,12 +3013,25 @@ def test_a_partial_table_does_not_carry(self) -> None: self.assertIn(f"is on {OLD[:8]}, where the table leaves out c.py", out) def test_a_partial_on_record_keeps_a_carried_table_out(self) -> None: + """The partial sits on a commit the statement carry refuses, so the table path decides.""" + older = "c" * 40 pr = self.tabled() - pr["reviews"]["nodes"].insert(0, review(oid=OLD, body=self.PART, at=EARLY, rid="P")) + pr["reviews"]["nodes"].insert( + 0, review(oid=older, body=self.PART, at="2026-08-02T09:00:00Z", rid="P") + ) + with self.compare(**{older: ["a.py"], OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("COVERAGE IS NOT CARRIED", out) + self.assertIn("NO FILE TABLE STANDS IN: a Copilot round on this pull request states", out) + + def test_a_review_history_past_the_window_keeps_a_carried_table_out(self) -> None: + pr = self.tabled() + pr["reviews"]["pageInfo"]["hasPreviousPage"] = True with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): code, out = self.verdict(pr) - self.assertEqual(42, code) - self.assertNotIn("FILE TABLE", out) + self.assertEqual(45, code) + self.assertIn("NO FILE TABLE STANDS IN: the review history is longer", out) def test_a_head_table_that_misses_the_diff_is_not_overruled_by_an_earlier_one(self) -> None: pr = self.tabled(head_body=summarized(["a.py"], covers="")) From 469fb1f9f64d40b4ef1b6c1a464bb5c56ec98f23 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:17:44 -0700 Subject: [PATCH 3/3] Test the Newest-Table Choice and Order the README Guard Cover which earlier table is consulted and that a refusal is skipped, place the README's partial guard after the carried-table sentence so it binds both readings, correct table_reading's commit wording, and rewrap the exit-45 text. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 18 +++++++++--------- tests/test_pr_review.py | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index d5fb06fcc..e7998cee6 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -192,7 +192,7 @@ The digest reports the completed head review's effective effort as `lite`, `bala `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under that same change-set bound, reading as `carried:table`. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 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, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under that same change-set bound, reading as `carried:table`. Either table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 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 was **head-scoped**, unlike a suppressed finding and like a refusal, on the reasoning that 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. The carry is what qualifies that: five of ten rounds in the second format raise no statement at all, so the push often raises a round that reads nothing back, and a partial carries to the head under the same change-set bound a full one does, which is a 42 decided by a round that is not on the head. Where one head carries two rounds through a re-request the worst of them still 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. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 32e2163cf..9782200c9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -50,8 +50,9 @@ describes a different set of changed files, or that comparison could not be read. Past those, a round covering the head whose own table names exactly the changed files stands in, or where none carries a table, the newest round that does, its table - naming exactly the changed files, under the same change-set bound, and only where no Copilot round on the pull request, on any commit, - states or appears to state partial coverage, quoted or not, and the whole review + naming exactly the changed files, under the same change-set bound. Either table + stands in only where no Copilot round on the pull request, on any commit, states or + appears to state partial coverage, quoted or not, and the whole review history is in view, since over the rounds measured the table names the whole changed set on partial rounds too. The digest names why no table stood in. Copilot's Balanced review effort, the default since 2026-09-28, writes the table and @@ -2130,9 +2131,9 @@ def carried_table(pr: dict) -> tuple[list[str], str] | None: def table_reading(owner: str, repo: str, pr: dict) -> tuple[str, str]: """Whether a file table stands in for this head's coverage, as the commit it was read on and why not. - The shortfall is empty where a table stands in. The commit is empty where the head's own - table is the one read, and names the earlier round whenever one was consulted, whether or not - its table carries. + The shortfall is empty where a table stands in. The commit names the earlier round's commit + where one was consulted and names one, whether or not its table carries, and is empty + otherwise. This is the coverage reading for a head that no round states coverage on and no earlier statement carries to. Copilot's Balanced review effort, the default since 2026-09-28, writes @@ -2407,10 +2408,9 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: "states none is the ordinary shape of the second overview format and carries the " "newest round that states some forward, bounded on the change set, and failing " "that, a round covering the head whose own file table names exactly the changed " - "files stands in, or where none carries a table, the newest round that does, its table " - "naming exactly the changed files, under " - "the same change-set bound, where no round on the pull request states or appears to state " - "partial coverage. Reaching here means no table stood in, for the reason the digest " + "files stands in, or where none carries a table, the newest round that does, its " + "table naming exactly the changed files, under the same change-set bound, where no " + "round on the pull request states or appears to state partial coverage. Reaching here means no table stood in, for the reason the digest " "above names, and also one of three things it says which of: no round ever stated " "coverage, the round that did describes a different set of changed files than this " "head has, or that comparison could not be read. Confirm the head branch carries " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 35b683cac..9aedf6eab 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3041,6 +3041,38 @@ def test_a_head_table_that_misses_the_diff_is_not_overruled_by_an_earlier_one(se self.assertIn("NO FILE TABLE STANDS IN: the table leaves out b.py", out) self.assertNotIn("/compare/", " ".join(self.calls)) + def test_only_the_newest_earlier_table_is_consulted(self) -> None: + """An older matching table does not stand in for a newer one that misses the diff.""" + older = "c" * 40 + for first, second, code in ( + (["a.py", "b.py"], ["a.py"], 45), + (["a.py"], ["a.py", "b.py"], 0), + ): + with self.subTest(newer=second): + pr_review.changed_at.cache_clear() + pr = payload( + [ + review(oid=older, body=summarized(first, covers=""), at=EARLY, rid="A"), + review(oid=OLD, body=summarized(second, covers=""), at=LATE, rid="B"), + review(oid=HEAD, body=self.NONE, at="2026-08-02T12:00:00Z", rid="C"), + ], + files=["a.py", "b.py"], + ) + same = ["a.py", "b.py"] + with self.compare(**{older: same, OLD: same, HEAD: same}): + self.assertEqual(code, self.verdict(pr)[0]) + + def test_a_refusal_carrying_a_table_is_not_the_table_carried(self) -> None: + refused = REFUSED + "\n\n" + summarized(["a.py", "b.py"], covers="") + pr = payload( + [ + review(oid=OLD, body=refused, at=EARLY, rid="A"), + review(oid=HEAD, body=self.NONE, at=LATE, rid="B"), + ], + files=["a.py", "b.py"], + ) + self.assertIsNone(pr_review.carried_table(pr)) + def test_no_table_anywhere_says_so(self) -> None: pr = self.rounds(review(oid=HEAD, body=self.NONE)) code, out = self.verdict(pr)