From 04e0681436b29809be35e6cce3515d9b21cf344c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:37:20 -0700 Subject: [PATCH 1/6] Say a Missing Required Check May Sit in a Partly Unread Rollup The promotion review of #3484 found exit 45 asserting a required check had not registered while the rollup holds a type the reader drops, which the digest names CHECKS PARTIALLY UNREAD. The status now says the check is not among those read and may sit in the unread part. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 3 ++- scripts/pr_review_checks.py | 5 +++++ tests/test_pr_review.py | 10 ++++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 84baa4fdf..39909a86a 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -215,7 +215,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `1` is a GraphQL read that failed and ended the run, and `2` is argparse's own usage error. -`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. +`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type this cannot read, named `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. `wait` exits `40` when no Copilot review covers the head and the reviewer has said why in another shape. A plain comment newer than its newest review carries no commit, so it satisfies no coverage check whatever it says, and its body prints whole because its wording is the only thing separating a refusal from a remark. A comment older than the newest review is spent. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, so `window_blind` guards both sides: blind on comments reads as `answered_outside_review=unknown`, and blind on reviews reports nothing and lets the wait keep polling. A request that returns success and adds neither a pending reviewer nor a review-request event also exits `40`, as `status=REQUEST_NOT_RECORDED`, as does a reviewer writing under a renamed login, `status=REVIEWER_LOGIN_DRIFT`. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index f22b3fb02..4a9dee968 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -143,7 +143,8 @@ UNSTABLE, or HAS_HOOKS. Either the payload carries no commit matching the head, named `CHECKS UNREADABLE` in the digest, or no check has posted. Or the merge reads DIRTY or BEHIND and a check the base branch's rules require has not registered, or came from - another app. That last needs the rules read and a rollup read whole. + another app. That last needs the rules read and a rollup read whole. Where the rollup + holds a type this cannot read, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to 30 instead, where the rules were read and the rollup read whole. An empty rollup read diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 94f602a06..c4823b5f7 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -312,6 +312,11 @@ def checks_never_ran( if not nodes: return "no check has posted, and a DIRTY merge runs no pull_request workflow until resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) + if unmet and checks_unread(nodes): + return ( + f"a required check, {unmet}, is not among the checks read and may sit in the " + "rollup's unread part" + ) return unmet and ( f"a required check has not registered or came from another app, {unmet}, so update a " "BEHIND head, resolve a DIRTY one's conflict, or correct the check's poster or its rule" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index b18bf0248..12df106d7 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3709,6 +3709,16 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: "came from another app, " + name, ), ("unread payload", unread, 45, "no commit matching the head"), + ( + "partly unread rollup", + payload( + [review(oid=OLD)], + merge="DIRTY", + checks=[check(name="lint"), {"__typename": "SomethingNew", "label": "x"}], + ), + 45, + "is not among the checks read and may sit in the rollup's unread part", + ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ (merge, payload([review(oid=OLD)], merge=merge, checks=[]), 0, "") From b176ff9c54fcc7c71ec6d43211d9eca1e4b3c41b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:40:04 -0700 Subject: [PATCH 2/6] Keep the Another-App Cause and Remedy Beside a Partly Unread Rollup The local pass found the new clause claiming a check was not read when it was read from another app, and dropping the remedy. The partly unread rollup is now one more cause in the original clause, which keeps the other causes and the remedy, and the test asserts the name. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review_checks.py | 11 ++++------- tests/test_pr_review.py | 3 ++- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index c4823b5f7..6980e2020 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -312,14 +312,11 @@ def checks_never_ran( if not nodes: return "no check has posted, and a DIRTY merge runs no pull_request workflow until resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) - if unmet and checks_unread(nodes): - return ( - f"a required check, {unmet}, is not among the checks read and may sit in the " - "rollup's unread part" - ) + unread = ", sits in the rollup's unread part," if checks_unread(nodes) else "" return unmet and ( - f"a required check has not registered or came from another app, {unmet}, so update a " - "BEHIND head, resolve a DIRTY one's conflict, or correct the check's poster or its rule" + f"a required check has not registered{unread} or came from another app, {unmet}, so " + "update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's poster or " + "its rule" ) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 12df106d7..c81d8f547 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3717,7 +3717,8 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: checks=[check(name="lint"), {"__typename": "SomethingNew", "label": "x"}], ), 45, - "is not among the checks read and may sit in the rollup's unread part", + "has not registered, sits in the rollup's unread part, or came from another app, " + + name, ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ From b8a74e8ad8f883a5a8b8e6ec5001f90c1d8790c4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:42:55 -0700 Subject: [PATCH 3/6] Point a Partly Unread Rollup at the Head Commit's Check Runs The second local pass found the partly unread cause carrying no remedy that reads the check it may hide. The status now leads with reading the head commit's check runs there, and the help and README name the digest's CHECKS PARTIALLY UNREAD and CHECKS TRUNCATED states directly. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 5 +++-- scripts/pr_review_checks.py | 10 ++++++---- tests/test_pr_review.py | 3 ++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 39909a86a..a6769f3ef 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -215,7 +215,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `1` is a GraphQL read that failed and ended the run, and `2` is argparse's own usage error. -`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type this cannot read, named `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. +`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. `wait` exits `40` when no Copilot review covers the head and the reviewer has said why in another shape. A plain comment newer than its newest review carries no commit, so it satisfies no coverage check whatever it says, and its body prints whole because its wording is the only thing separating a refusal from a remark. A comment older than the newest review is spent. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, so `window_blind` guards both sides: blind on comments reads as `answered_outside_review=unknown`, and blind on reviews reports nothing and lets the wait keep polling. A request that returns success and adds neither a pending reviewer nor a review-request event also exits `40`, as `status=REQUEST_NOT_RECORDED`, as does a reviewer writing under a renamed login, `status=REVIEWER_LOGIN_DRIFT`. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 4a9dee968..1ce8ff8b2 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -143,8 +143,9 @@ UNSTABLE, or HAS_HOOKS. Either the payload carries no commit matching the head, named `CHECKS UNREADABLE` in the digest, or no check has posted. Or the merge reads DIRTY or BEHIND and a check the base branch's rules require has not registered, or came from - another app. That last needs the rules read and a rollup read whole. Where the rollup - holds a type this cannot read, the status says the check may sit there. + another app. That last needs the rules read and a rollup not `CHECKS TRUNCATED`. Where + the rollup holds a type this cannot read, named `CHECKS PARTIALLY UNREAD` in the + digest, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to 30 instead, where the rules were read and the rollup read whole. An empty rollup read diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 6980e2020..da12f4115 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -312,11 +312,13 @@ def checks_never_ran( if not nodes: return "no check has posted, and a DIRTY merge runs no pull_request workflow until resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) - unread = ", sits in the rollup's unread part," if checks_unread(nodes) else "" + unread = checks_unread(nodes) + where = ", sits in the rollup's unread part," if unread else "" + read = "read the check runs on the head commit, " if unread else "" return unmet and ( - f"a required check has not registered{unread} or came from another app, {unmet}, so " - "update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's poster or " - "its rule" + f"a required check has not registered{where} or came from another app, {unmet}, so " + f"{read}update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's " + "poster or its rule" ) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index c81d8f547..a1a09250d 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3718,7 +3718,8 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: ), 45, "has not registered, sits in the rollup's unread part, or came from another app, " - + name, + + name + + ", so read the check runs on the head commit, update a BEHIND head", ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ From 9c06fa08d620417756d08818b06148b8c35f73fe Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:46:10 -0700 Subject: [PATCH 4/6] Name the Unread Rollup in the BLOCKED Hold's Unmet Note Too The open findings listed on #3486: the BLOCKED hold's CHECKS_PENDING note now says an unmet required check may sit in the rollup's unread part, the help's hold sentence names CHECKS TRUNCATED rather than a rollup read whole, reading the check runs is a first step before the fixes, and a test proves that remedy appears only on a partly unread rollup. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 4 ++-- scripts/pr_review_checks.py | 9 +++++---- tests/test_pr_review.py | 9 ++++++++- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 1ce8ff8b2..729e4ea36 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -148,8 +148,8 @@ digest, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to - 30 instead, where the rules were read and the rollup read whole. An empty rollup read - off the head holds a BLOCKED, BEHIND, or DRAFT one. + 30 instead, where the rules were read and the rollup is not `CHECKS TRUNCATED`. An empty + rollup read off the head holds a BLOCKED, BEHIND, or DRAFT one. 49 = no Copilot round covers this head and none was requested, the pull request merging into a branch other than the default with no attestation vouching for the head. 64 = the write scope could not diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index da12f4115..25c4adf2e 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -303,7 +303,7 @@ def checks_never_ran( """Why an open head's gated merge lacks a check it is owed, with its remedy, or "" for none. A CLEAN, UNSTABLE, or HAS_HOOKS merge is owed nothing. The caller passes `required` only for a - merge whose release judges registration, and a context is judged only on a rollup read whole. + merge whose release judges registration, and a context is judged only on a rollup not cut. """ if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" @@ -314,7 +314,7 @@ def checks_never_ran( unmet = ", ".join(required_unregistered(pr, nodes, required)) unread = checks_unread(nodes) where = ", sits in the rollup's unread part," if unread else "" - read = "read the check runs on the head commit, " if unread else "" + read = "read the check runs on the head commit first, then " if unread else "" return unmet and ( f"a required check has not registered{where} or came from another app, {unmet}, so " f"{read}update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's " @@ -347,7 +347,7 @@ def runs_open(pr: dict) -> list[str]: def required_unregistered( pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] ) -> list[str]: - """The contexts among `required` no rollup node satisfies, sorted, on a rollup read whole.""" + """The contexts among `required` no rollup node satisfies, sorted, on a rollup read uncut.""" if checks_unreadable(pr) or checks_truncated(pr): return [] seen = [n for n in nodes if not n.get("unreadable")] @@ -366,7 +366,8 @@ def required_unregistered( def _unmet_note(pr: dict, required: frozenset[tuple[str, int | None]], nodes: list[dict]) -> str: """Names the required contexts still unmet, so a reader sees what the hold waits on.""" unmet = required_unregistered(pr, nodes, required) - return f" (required, unmet: {', '.join(unmet)})" if unmet else "" + where = ", or in the rollup's unread part" if checks_unread(nodes) else "" + return f" (required, unmet: {', '.join(unmet)}{where})" if unmet else "" def checks_truncated(pr: dict) -> bool: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index a1a09250d..563f8aa0d 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3719,7 +3719,7 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: 45, "has not registered, sits in the rollup's unread part, or came from another app, " + name - + ", so read the check runs on the head commit, update a BEHIND head", + + ", so read the check runs on the head commit first, then update a BEHIND head", ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ @@ -3743,6 +3743,8 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(code, self.cli(["wait", "7", "--timeout", "1"])) self.assertIn(reason, self.out.getvalue()) + if label == "another app": + self.assertNotIn("read the check runs on the head commit", self.out.getvalue()) if code == 0: self.assertNotIn("status=CHECKS_MISSING", self.out.getvalue()) @@ -6289,6 +6291,11 @@ def test_the_unmet_context_is_named(self) -> None: nodes = pr_review_checks.check_nodes(pr) self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", None)}), nodes)) self.assertIn("Gate", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes)) + unread = nodes + pr_review_checks.check_nodes( + payload([], merge="BLOCKED", checks=[{"__typename": "SomethingNew", "label": "x"}]) + ) + note = pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), unread) + self.assertIn("Gate, or in the rollup's unread part", note) rollup = pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"] rollup["contexts"]["pageInfo"] = {"hasNextPage": True} self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes)) From 7447e0877282ba92cb81f54ca5ab28a744aa18ab Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:49:11 -0700 Subject: [PATCH 5/6] Document the BLOCKED Note's Unread Part and Read Checks and Statuses The local pass found the docs naming the unread part only for exit 45, though the BLOCKED hold's CHECKS_PENDING note names it too, and nothing proving that note's clause is conditional. An unread node may be a commit status, so the remedy reads the head commit's checks and statuses rather than its check runs alone. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 3 ++- scripts/pr_review_checks.py | 2 +- tests/test_pr_review.py | 6 ++++-- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index a6769f3ef..c2ac1dd30 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -215,7 +215,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `1` is a GraphQL read that failed and ended the run, and `2` is argparse's own usage error. -`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. +`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. Its status line names the rollup's unread part as a place that check may sit. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. `wait` exits `40` when no Copilot review covers the head and the reviewer has said why in another shape. A plain comment newer than its newest review carries no commit, so it satisfies no coverage check whatever it says, and its body prints whole because its wording is the only thing separating a refusal from a remark. A comment older than the newest review is spent. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, so `window_blind` guards both sides: blind on comments reads as `answered_outside_review=unknown`, and blind on reviews reports nothing and lets the wait keep polling. A request that returns success and adds neither a pending reviewer nor a review-request event also exits `40`, as `status=REQUEST_NOT_RECORDED`, as does a reviewer writing under a renamed login, `status=REVIEWER_LOGIN_DRIFT`. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 729e4ea36..ce7e3bd67 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -124,7 +124,8 @@ rules, a `CHECKS TRUNCATED` rollup, a DRAFT merge, or a pull request not open. 30 = still pending at the timeout. On a held head it is any hold `status=CHECKS_PENDING` names, a required check still settling and a workflow run still - open among them. It is also a round past --min-rounds not yet landed. + open among them. A BLOCKED hold's note names the rollup's unread part as a place an + unmet required check may sit. It is also a round past --min-rounds not yet landed. 40 = no Copilot review covers the head: the reviewer answered outside a formal review, the request returned success and recorded nothing on the pull request, or the reviewer writes under a login no query filters on. 41 = the diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 25c4adf2e..87a61bef7 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -314,7 +314,7 @@ def checks_never_ran( unmet = ", ".join(required_unregistered(pr, nodes, required)) unread = checks_unread(nodes) where = ", sits in the rollup's unread part," if unread else "" - read = "read the check runs on the head commit first, then " if unread else "" + read = "read the head commit's checks and statuses first, then " if unread else "" return unmet and ( f"a required check has not registered{where} or came from another app, {unmet}, so " f"{read}update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 563f8aa0d..bb468366a 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3719,7 +3719,7 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: 45, "has not registered, sits in the rollup's unread part, or came from another app, " + name - + ", so read the check runs on the head commit first, then update a BEHIND head", + + ", so read the head commit's checks and statuses first, then update a BEHIND head", ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ @@ -3744,7 +3744,7 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: self.assertEqual(code, self.cli(["wait", "7", "--timeout", "1"])) self.assertIn(reason, self.out.getvalue()) if label == "another app": - self.assertNotIn("read the check runs on the head commit", self.out.getvalue()) + self.assertNotIn("checks and statuses first", self.out.getvalue()) if code == 0: self.assertNotIn("status=CHECKS_MISSING", self.out.getvalue()) @@ -6291,6 +6291,8 @@ def test_the_unmet_context_is_named(self) -> None: nodes = pr_review_checks.check_nodes(pr) self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", None)}), nodes)) self.assertIn("Gate", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes)) + note = pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes) + self.assertNotIn("unread part", note) unread = nodes + pr_review_checks.check_nodes( payload([], merge="BLOCKED", checks=[{"__typename": "SomethingNew", "label": "x"}]) ) From 1f9fde034b0f974afa4f678305b022a318a39bbb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:53:10 -0700 Subject: [PATCH 6/6] Condition the Exit 30 Unread-Part Sentences on a Partly Unread Rollup The second local pass on this push found the exit 30 sentences naming the unread part with no condition, and "not CHECKS TRUNCATED" dropping the read-off-the-head precondition the hold needs. Both now say so, the docstrings share one phrasing, and the note says the unmet check may sit in the unread part rather than listing it as another item. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 9 +++++---- scripts/pr_review_checks.py | 7 ++++--- tests/test_pr_review.py | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index c2ac1dd30..732392dbb 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -215,7 +215,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `1` is a GraphQL read that failed and ended the run, and `2` is argparse's own usage error. -`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. Its status line names the rollup's unread part as a place that check may sit. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. +`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, its status line says that check may sit in the unread part. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. `wait` exits `40` when no Copilot review covers the head and the reviewer has said why in another shape. A plain comment newer than its newest review carries no commit, so it satisfies no coverage check whatever it says, and its body prints whole because its wording is the only thing separating a refusal from a remark. A comment older than the newest review is spent. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, so `window_blind` guards both sides: blind on comments reads as `answered_outside_review=unknown`, and blind on reviews reports nothing and lets the wait keep polling. A request that returns success and adds neither a pending reviewer nor a review-request event also exits `40`, as `status=REQUEST_NOT_RECORDED`, as does a reviewer writing under a renamed login, `status=REVIEWER_LOGIN_DRIFT`. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index ce7e3bd67..adad84f4f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -124,8 +124,9 @@ rules, a `CHECKS TRUNCATED` rollup, a DRAFT merge, or a pull request not open. 30 = still pending at the timeout. On a held head it is any hold `status=CHECKS_PENDING` names, a required check still settling and a workflow run still - open among them. A BLOCKED hold's note names the rollup's unread part as a place an - unmet required check may sit. It is also a round past --min-rounds not yet landed. + open among them. Where the rollup holds a type this cannot read, a BLOCKED hold's note + says an unmet required check may sit in its unread part. It is also a round past + --min-rounds not yet landed. 40 = no Copilot review covers the head: the reviewer answered outside a formal review, the request returned success and recorded nothing on the pull request, or the reviewer writes under a login no query filters on. 41 = the @@ -149,8 +150,8 @@ digest, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to - 30 instead, where the rules were read and the rollup is not `CHECKS TRUNCATED`. An empty - rollup read off the head holds a BLOCKED, BEHIND, or DRAFT one. + 30 instead, where the rules and the head's rollup were read, not `CHECKS TRUNCATED`. An + empty rollup read off the head holds a BLOCKED, BEHIND, or DRAFT one. 49 = no Copilot round covers this head and none was requested, the pull request merging into a branch other than the default with no attestation vouching for the head. 64 = the write scope could not diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 87a61bef7..5c8d8643f 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -303,7 +303,8 @@ def checks_never_ran( """Why an open head's gated merge lacks a check it is owed, with its remedy, or "" for none. A CLEAN, UNSTABLE, or HAS_HOOKS merge is owed nothing. The caller passes `required` only for a - merge whose release judges registration, and a context is judged only on a rollup not cut. + merge whose release judges registration, and a context is judged only on the head's own + rollup, not cut. """ if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" @@ -347,7 +348,7 @@ def runs_open(pr: dict) -> list[str]: def required_unregistered( pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] ) -> list[str]: - """The contexts among `required` no rollup node satisfies, sorted, on a rollup read uncut.""" + """The `required` contexts no node satisfies, sorted, on the head's own rollup, not cut.""" if checks_unreadable(pr) or checks_truncated(pr): return [] seen = [n for n in nodes if not n.get("unreadable")] @@ -366,7 +367,7 @@ def required_unregistered( def _unmet_note(pr: dict, required: frozenset[tuple[str, int | None]], nodes: list[dict]) -> str: """Names the required contexts still unmet, so a reader sees what the hold waits on.""" unmet = required_unregistered(pr, nodes, required) - where = ", or in the rollup's unread part" if checks_unread(nodes) else "" + where = ", which may sit in the rollup's unread part" if checks_unread(nodes) else "" return f" (required, unmet: {', '.join(unmet)}{where})" if unmet else "" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index bb468366a..aec58f356 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -6297,7 +6297,7 @@ def test_the_unmet_context_is_named(self) -> None: payload([], merge="BLOCKED", checks=[{"__typename": "SomethingNew", "label": "x"}]) ) note = pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), unread) - self.assertIn("Gate, or in the rollup's unread part", note) + self.assertIn("Gate, which may sit in the rollup's unread part", note) rollup = pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"] rollup["contexts"]["pageInfo"] = {"hasNextPage": True} self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes))