From c3f8d2db1bae99c1901270ad7cb848534138f07e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:05:00 -0700 Subject: [PATCH 1/8] Exit 45 When a Held Wait Releases a Head Whose Required Checks Never Ran A held wait exited 0 on a DIRTY merge, a BEHIND merge with a required context unregistered, and a payload with no commit matching the head, though the help said the required checks had settled. Each now exits 45 with status=CHECKS_NOT_RUN naming which, and the wait help and the README's exit table and held-wait paragraph say so together. Closes #3471 Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 5 +-- scripts/pr_review.py | 34 +++++++++++++++----- scripts/pr_review_checks.py | 24 +++++++++++++- tests/test_pr_review.py | 62 ++++++++++++++++++++++++++++++++++--- 4 files changed, 111 insertions(+), 14 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 850f08f4..489e1798 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -199,12 +199,13 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | Code | Command | Meaning | | --- | --- | --- | -| `0` | `status`, `wait` | No Copilot review covers the head yet (`status`), or the review covering it states no partial coverage and its verdict counts what it flags. On a held head, an attested local pass whose checks no longer hold the poll, which does not prove its required checks ran. | +| `0` | `status`, `wait` | No Copilot review covers the head yet (`status`), or the review covering it states no partial coverage and its verdict counts what it flags. On a held head, an attested local pass whose checks no longer hold the poll, short of `45`. | | `30` | `wait` | Still pending at the timeout, which is pending rather than failed. On a held head, `status=CHECKS_PENDING`. A round past `--min-rounds` not yet landed. | | `40` | `wait` | No Copilot review covers the head: the reviewer answered outside a formal review, the request returned success and recorded nothing, or the reviewer writes under a login no query filters on. | | `41` | `wait` | The newest round is a refusal or a quota state, its kind printed in the status line, and in `refusal=` for a refusal on this pull request. | | `42` | `status`, `wait` | The round covering the head states partial coverage. Or a non-refusal head round exists while an earlier round states partial, and one reads `UNKNOWN` or none states coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | +| `45` | `wait` | An attested held head that nothing holds, under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`, whose required checks have not run: no commit matching the head, no check posted, or a required context unregistered. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | | `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | @@ -214,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`. So exit `0` on a held head does not prove its required checks ran. A payload with no commit matching the head, which the digest names `CHECKS UNREADABLE`, reads no run and no check. Short of those holds, a merge with some check posted, or a `DIRTY` one with none, can release with a required check unregistered. `--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`. A head released short of those holds under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` exits `45` with `status=CHECKS_NOT_RUN` where its required checks have not run. One case 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 is how a `DIRTY` merge reads, since a conflicted pull request runs no `pull_request` workflow. The last is a context the base branch's rules require that has not registered on a rollup that is not `CHECKS TRUNCATED`. Exit `0` on a held head under such a merge therefore means its required checks registered, except where the base branch's rules were unread or the rollup was cut. `--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 1fa20559..8cdbf4bd 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -116,13 +116,10 @@ Exit 0 = review present, or on a held head an attested local pass whose checks no longer hold the poll. On an open pull request whose head commit the payload carries, every workflow run the read reaches on that head has then completed. The read reaches the - head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. It - does not prove the required checks ran. - A payload with no commit matching the head, named `CHECKS UNREADABLE` in the digest, - reads no run and no check. A required check not yet registered holds only a BLOCKED - merge whose base branch rules were read and whose rollup is not `CHECKS TRUNCATED`. An - empty rollup holds only a BLOCKED, BEHIND, or DRAFT one. Short of those holds, a merge with - some check posted, or a DIRTY one with none, can exit 0 with a required check unregistered. + head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. A + CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, exit + 0 means every check the base branch's rules require has registered, except where those + rules were unread or the rollup is `CHECKS TRUNCATED`, since 45 takes the rest. 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. @@ -140,6 +137,14 @@ is in a shape no wait clears. It is queued with nothing acting on it, expected and never posted, running far past the job's cost, held by its environment, or failed. On a held head a check running long or held by its environment polls to 30 instead. + 45 = an attested local pass covers a held head and nothing holds the poll, but the merge + reads other than CLEAN, UNSTABLE, or HAS_HOOKS and its required checks have not run. + The payload carries no commit matching the head, named `CHECKS UNREADABLE` in the + digest, no check has posted, or a context the base branch's rules require has not + registered on a rollup read whole. `status=CHECKS_NOT_RUN` 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, and an empty rollup 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 @@ -177,10 +182,12 @@ CHECK_GRACE, CHECK_STALL, CHECKS_WINDOW, + MERGE_UNGATED, WORKFLOW_KEYS_UNAVAILABLE, _unmet_note, check_lines, check_nodes, + checks_never_ran, checks_settling, checks_stuck, checks_tally, @@ -2030,6 +2037,19 @@ def held_read() -> dict: "the merge, because BLOCKED is also worn by a thread or a missing approval" ) return 44 + merge = final.get("mergeStateStatus") + if covered and merge not in MERGE_UNGATED: + rules = required_contexts(owner, repo, final.get("baseRefName") or "") or frozenset() + if unrun := checks_never_ran(final, checks, rules): + print( + f"status=CHECKS_NOT_RUN an attested local pass covers this head and nothing " + f"holds the poll, but the merge reads {merge} and {unrun}, so the merge is " + "not ready and the poll does not clear it: a DIRTY merge runs no " + "pull_request workflow until its conflict is resolved, a BEHIND one may " + "need the head updated from its base, and a payload read off another " + "commit wants status run again" + ) + return 45 return 0 # Every refusal and quota state is 41, the kind named by its status line. refusal = refusing_review(final) diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index bd044afd..843be8ac 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -20,6 +20,8 @@ # SKIPPED and NEUTRAL count as passed, since the fleet aggregator skips conditional jobs. CHECK_OK = frozenset({"SUCCESS", "SKIPPED", "NEUTRAL"}) +MERGE_UNGATED = frozenset({"CLEAN", "UNSTABLE", "HAS_HOOKS"}) + WORKFLOW_KEYS_UNAVAILABLE = "workflowKeysUnavailable" # How long a queued check is simply starting, the pickup grace's five minutes. @@ -283,7 +285,7 @@ def held_checks_open( return False if runs_open(pr): return True - if merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS"): + if merge in MERGE_UNGATED: return False settling = checks_settling(nodes, now, grace, stall) if any(n.get("required") for n in settling): @@ -300,6 +302,26 @@ def held_checks_open( return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) +def checks_never_ran( + pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] +) -> str: + """Why an open head's merge is waiting on checks that never ran on it, or "" for none known. + + A CLEAN, UNSTABLE, or HAS_HOOKS merge waits on nothing. Short of one, a payload with no commit + matching the head read no check, a rollup with none posted ran none, and a context in + `required` that the rollup does not carry has not run, judged only on a rollup read whole. + """ + if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: + return "" + if checks_unreadable(pr): + return "the payload carries no commit matching the head, so no check was read" + if not nodes: + return "no check has posted on the head" + if not checks_truncated(pr) and (unmet := required_unmet(required, nodes)): + return f"a check the base branch requires has not registered: {', '.join(unmet)}" + return "" + + def runs_open(pr: dict) -> list[str]: """The head's workflow runs whose suite has not completed, each named `run `, sorted. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index a31a9cdb..69811171 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3571,13 +3571,15 @@ def test_a_required_check_running_at_the_timeout_outranks_an_optional_failure(se self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: - """A conflicted pull request runs no `pull_request` workflow, so none is coming.""" + """A conflicted pull request runs no `pull_request` workflow, so none is coming, and its + release names that rather than exiting 0 over checks that never ran. + """ for merge, code in ( ("BLOCKED", 30), ("BEHIND", 30), ("DRAFT", 30), ("UNKNOWN", 30), - ("DIRTY", 0), + ("DIRTY", 45), ): with self.subTest(merge=merge): pr = self.into(payload([review(oid=OLD)], merge=merge, checks=[]), attest=True) @@ -3664,6 +3666,40 @@ def test_every_required_check_registered_does_not_hold_a_blocked_head(self) -> N self.assertNotEqual(30, self.cli(["wait", "7", "--timeout", "0"])) slept.assert_not_called() + def test_a_required_check_unregistered_under_a_released_merge_exits_45(self) -> None: + """A BEHIND or DIRTY merge is not held on an unregistered required check, so its release + names the check rather than exiting 0 as though the required checks settled. + """ + self.rules("Check pull request workflow status job", "Later workflow") + for merge in ("BEHIND", "DIRTY"): + with self.subTest(merge=merge): + self.out.seek(0) + self.out.truncate() + pr = self.into( + payload([review(oid=OLD)], merge=merge, checks=[check()]), attest=True + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(45, self.cli(["wait", "7", "--timeout", "1"])) + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn("status=CHECKS_NOT_RUN", out) + self.assertIn(f"merge reads {merge}", out) + self.assertIn("has not registered: Later workflow", out) + + def test_a_released_merge_with_every_required_check_registered_exits_0(self) -> None: + self.rules("Check pull request workflow status job") + for merge in ("BEHIND", "DIRTY"): + with self.subTest(merge=merge): + pr = self.into( + payload([review(oid=OLD)], merge=merge, checks=[check()]), attest=True + ) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) + def test_an_unread_rules_read_keeps_the_old_result_and_says_so(self) -> None: """The suite's default `gh_rest` fails, which must never read as nothing being required.""" pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", checks=[check()]), attest=True) @@ -3932,13 +3968,17 @@ def test_a_required_check_running_long_holds_the_poll_rather_than_ending_it(self self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) def test_an_unreadable_rollup_is_not_held_as_one_not_yet_registered(self) -> None: - """A rollup read off another commit is empty here, and waiting does not clear that.""" + """A rollup read off another commit is empty here, and waiting does not clear that, so + the release names the unread payload rather than exiting 0 over checks it never read. + """ pr = payload([review(oid=OLD)], merge="BLOCKED", checks=[check()], rollup_oid="d" * 40) self.answer(self.into(pr, attest=True)) self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) with mock.patch.object(pr_review.time, "sleep") as slept: - self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) + self.assertEqual(45, self.cli(["wait", "7", "--timeout", "1"])) slept.assert_not_called() + self.assertIn("status=CHECKS_NOT_RUN", self.out.getvalue()) + self.assertIn("no commit matching the head", self.out.getvalue()) def test_an_outside_answer_outranks_pending_after_a_mid_poll_request(self) -> None: running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) @@ -6072,6 +6112,20 @@ def test_blocked_with_a_required_context_absent_holds(self) -> None: clean = {**pr, "mergeStateStatus": "CLEAN"} self.assertFalse(pr_review_checks.held_checks_open(clean, nodes, now, 600, 3600, want)) + def test_checks_never_ran_judges_only_a_gated_open_merge_on_a_rollup_read_whole(self) -> None: + """A cut rollup can hold the absent context past its window, so it proves nothing.""" + want = frozenset({("Later", None)}) + pr = payload([], merge="BEHIND", checks=[check()]) + nodes = pr_review_checks.check_nodes(pr) + self.assertIn("Later", pr_review_checks.checks_never_ran(pr, nodes, want)) + self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes, frozenset())) + for ungated in ({**pr, "mergeStateStatus": "CLEAN"}, {**pr, "state": "MERGED"}): + self.assertEqual("", pr_review_checks.checks_never_ran(ungated, nodes, want)) + pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"]["contexts"]["pageInfo"] = { + "hasNextPage": True + } + self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes, want)) + def test_a_rollup_not_read_whole_never_holds_on_an_absent_context(self) -> None: now = datetime.now(UTC) want = frozenset({("Later", None)}) From b2b0ec1e7da1b9c6a30599ed2e642052c5ba35ad Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:13:30 -0700 Subject: [PATCH 2/8] Judge Exit 45's Unregistered Check on DIRTY and BEHIND Alone The local review found the exit 45 text claiming more than the read proves, a DRAFT merge reaching it with a remedy that does not apply, and the rules read even where no path used them. The unregistered check now applies to DIRTY and BEHIND only, reads the rules there alone, shares one whole-rollup helper with the BLOCKED hold, and names a check from another app. The help and README state each exception. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 4 +-- scripts/pr_review.py | 51 ++++++++++++++------------ scripts/pr_review_checks.py | 33 ++++++++--------- tests/test_pr_review.py | 71 ++++++++++++++++++++++++++++++------- 4 files changed, 104 insertions(+), 55 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 489e1798..c908d82b 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -205,7 +205,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | `41` | `wait` | The newest round is a refusal or a quota state, its kind printed in the status line, and in `refusal=` for a refusal on this pull request. | | `42` | `status`, `wait` | The round covering the head states partial coverage. Or a non-refusal head round exists while an earlier round states partial, and one reads `UNKNOWN` or none states coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | -| `45` | `wait` | An attested held head that nothing holds, under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`, whose required checks have not run: no commit matching the head, no check posted, or a required context unregistered. | +| `45` | `wait` | An attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its checks were unread or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | | `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | @@ -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`. A head released short of those holds under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` exits `45` with `status=CHECKS_NOT_RUN` where its required checks have not run. One case 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 is how a `DIRTY` merge reads, since a conflicted pull request runs no `pull_request` workflow. The last is a context the base branch's rules require that has not registered on a rollup that is not `CHECKS TRUNCATED`. Exit `0` on a held head under such a merge therefore means its required checks registered, except where the base branch's rules were unread or the rollup was cut. `--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`. A head released short of those holds exits `45` with `status=CHECKS_NOT_RUN` in three cases, each 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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 8cdbf4bd..2b8f6f4e 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -117,9 +117,11 @@ hold the poll. On an open pull request whose head commit the payload carries, every workflow run the read reaches on that head has then completed. The read reaches the head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. A - CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, exit - 0 means every check the base branch's rules require has registered, except where those - rules were unread or the rollup is `CHECKS TRUNCATED`, since 45 takes the rest. + CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, an + attested held head exits 45 where its checks were unread or none posted. A DIRTY or + BEHIND one also exits 45 where a required check has not registered. Exit 0 can then + leave a required check unregistered only where the rules were unread, the rollup is + `CHECKS TRUNCATED`, or the merge reads DRAFT. 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. @@ -137,14 +139,14 @@ is in a shape no wait clears. It is queued with nothing acting on it, expected and never posted, running far past the job's cost, held by its environment, or failed. On a held head a check running long or held by its environment polls to 30 instead. - 45 = an attested local pass covers a held head and nothing holds the poll, but the merge - reads other than CLEAN, UNSTABLE, or HAS_HOOKS and its required checks have not run. - The payload carries no commit matching the head, named `CHECKS UNREADABLE` in the - digest, no check has posted, or a context the base branch's rules require has not - registered on a rollup read whole. `status=CHECKS_NOT_RUN` 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, and an empty rollup holds a BLOCKED, - BEHIND, or DRAFT one. + 45 = an attested held head that nothing holds, its merge other than CLEAN, 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. + `status=CHECKS_NOT_RUN` 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, and an empty rollup 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 @@ -182,7 +184,6 @@ CHECK_GRACE, CHECK_STALL, CHECKS_WINDOW, - MERGE_UNGATED, WORKFLOW_KEYS_UNAVAILABLE, _unmet_note, check_lines, @@ -192,6 +193,7 @@ checks_stuck, checks_tally, held_checks_open, + required_unregistered, runs_open, ) from pr_review_rounds import ( @@ -2038,18 +2040,23 @@ def held_read() -> dict: ) return 44 merge = final.get("mergeStateStatus") - if covered and merge not in MERGE_UNGATED: + unrun = checks_never_ran(final, checks) if covered else "" + if covered and not unrun and merge in ("DIRTY", "BEHIND"): rules = required_contexts(owner, repo, final.get("baseRefName") or "") or frozenset() - if unrun := checks_never_ran(final, checks, rules): - print( - f"status=CHECKS_NOT_RUN an attested local pass covers this head and nothing " - f"holds the poll, but the merge reads {merge} and {unrun}, so the merge is " - "not ready and the poll does not clear it: a DIRTY merge runs no " - "pull_request workflow until its conflict is resolved, a BEHIND one may " - "need the head updated from its base, and a payload read off another " - "commit wants status run again" + if unmet := required_unregistered(final, checks, rules): + unrun = ( + "a check the base branch requires has not registered, or came from " + f"another app: {', '.join(unmet)}" ) - return 45 + if unrun: + print( + f"status=CHECKS_NOT_RUN an attested local pass covers this head and nothing holds " + f"the poll, but the merge reads {merge} and {unrun}, so the merge is not ready " + "and the poll does not clear it: a DIRTY merge runs no pull_request workflow " + "until its conflict is resolved, a BEHIND one may need the head updated from its " + "base, and a payload read off another commit wants status run again" + ) + return 45 return 0 # Every refusal and quota state is 41, the kind named by its status line. refusal = refusing_review(final) diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 843be8ac..6a3ccaf1 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -292,34 +292,22 @@ def held_checks_open( return True if settling and not any(n.get("required") for n in nodes): return True - if ( - merge == "BLOCKED" - and not checks_unreadable(pr) - and not checks_truncated(pr) - and required_unmet(required, nodes) - ): + if merge == "BLOCKED" and required_unregistered(pr, nodes, required): return True return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) -def checks_never_ran( - pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] -) -> str: - """Why an open head's merge is waiting on checks that never ran on it, or "" for none known. +def checks_never_ran(pr: dict, nodes: list[dict]) -> str: + """Why an open head's gated merge has no check of its own to settle on, or "" where it has. - A CLEAN, UNSTABLE, or HAS_HOOKS merge waits on nothing. Short of one, a payload with no commit - matching the head read no check, a rollup with none posted ran none, and a context in - `required` that the rollup does not carry has not run, judged only on a rollup read whole. + A CLEAN, UNSTABLE, or HAS_HOOKS merge is gated by nothing. Under any other merge, a payload with + no commit matching the head read no check, and a rollup with none posted ran none. """ if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" if checks_unreadable(pr): return "the payload carries no commit matching the head, so no check was read" - if not nodes: - return "no check has posted on the head" - if not checks_truncated(pr) and (unmet := required_unmet(required, nodes)): - return f"a check the base branch requires has not registered: {', '.join(unmet)}" - return "" + return "" if nodes else "no check has posted on the head" def runs_open(pr: dict) -> list[str]: @@ -359,6 +347,15 @@ def required_unmet(required: frozenset[tuple[str, int | None]], nodes: list[dict ) +def required_unregistered( + pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] +) -> list[str]: + """The contexts among `required` the head's rollup lacks, judged only on a rollup read whole.""" + if checks_unreadable(pr) or checks_truncated(pr): + return [] + return required_unmet(required, nodes) + + def _unmet_note(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_unmet(required, nodes) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 69811171..7d44b036 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3571,8 +3571,9 @@ def test_a_required_check_running_at_the_timeout_outranks_an_optional_failure(se self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: - """A conflicted pull request runs no `pull_request` workflow, so none is coming, and its - release names that rather than exiting 0 over checks that never ran. + """A conflicted pull request runs no `pull_request` workflow, so none is coming. + + Its release names that state rather than exiting 0 over checks that never ran. """ for merge, code in ( ("BLOCKED", 30), @@ -3585,8 +3586,13 @@ def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: pr = self.into(payload([review(oid=OLD)], merge=merge, checks=[]), attest=True) self.answer(pr) self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + self.out.seek(0) + self.out.truncate() with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(code, self.cli(["wait", "7", "--timeout", "0"])) + if code == 45: + self.assertIn("status=CHECKS_NOT_RUN", self.out.getvalue()) + self.assertIn("no check has posted on the head", self.out.getvalue()) def test_an_optional_check_does_not_hold_once_the_required_gate_concluded(self) -> None: """A merge BLOCKED on a thread is not waiting on a check nothing requires.""" @@ -3667,8 +3673,9 @@ def test_every_required_check_registered_does_not_hold_a_blocked_head(self) -> N slept.assert_not_called() def test_a_required_check_unregistered_under_a_released_merge_exits_45(self) -> None: - """A BEHIND or DIRTY merge is not held on an unregistered required check, so its release - names the check rather than exiting 0 as though the required checks settled. + """A BEHIND or DIRTY merge is not held on an unregistered required check. + + Its release names the check rather than exiting 0 as though the required checks settled. """ self.rules("Check pull request workflow status job", "Later workflow") for merge in ("BEHIND", "DIRTY"): @@ -3686,7 +3693,36 @@ def test_a_required_check_unregistered_under_a_released_merge_exits_45(self) -> out = self.out.getvalue() self.assertIn("status=CHECKS_NOT_RUN", out) self.assertIn(f"merge reads {merge}", out) - self.assertIn("has not registered: Later workflow", out) + self.assertIn("has not registered, or came from another app: Later workflow", out) + + def test_a_released_merge_exits_0_where_the_unregistered_check_is_unproven(self) -> None: + """Unread rules, a cut rollup, and a DRAFT merge each leave registration unjudged.""" + cut = self.into(payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), attest=True) + cut["commits"]["nodes"][0]["commit"]["statusCheckRollup"]["contexts"]["pageInfo"] = { + "hasNextPage": True + } + cases = { + "unread": ( + self.into( + payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), attest=True + ), + False, + ), + "truncated": (cut, True), + "draft": ( + self.into(payload([review(oid=OLD)], merge="DRAFT", checks=[check()]), attest=True), + True, + ), + } + for label, (pr, rules) in cases.items(): + with self.subTest(label): + pr_review.required_contexts.cache_clear() + if rules: + self.rules("Check pull request workflow status job", "Later workflow") + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "1"])) def test_a_released_merge_with_every_required_check_registered_exits_0(self) -> None: self.rules("Check pull request workflow status job") @@ -3968,8 +4004,9 @@ def test_a_required_check_running_long_holds_the_poll_rather_than_ending_it(self self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) def test_an_unreadable_rollup_is_not_held_as_one_not_yet_registered(self) -> None: - """A rollup read off another commit is empty here, and waiting does not clear that, so - the release names the unread payload rather than exiting 0 over checks it never read. + """A rollup read off another commit is empty here, and waiting does not clear that. + + The release names the unread payload rather than exiting 0 over checks it never read. """ pr = payload([review(oid=OLD)], merge="BLOCKED", checks=[check()], rollup_oid="d" * 40) self.answer(self.into(pr, attest=True)) @@ -6112,19 +6149,27 @@ def test_blocked_with_a_required_context_absent_holds(self) -> None: clean = {**pr, "mergeStateStatus": "CLEAN"} self.assertFalse(pr_review_checks.held_checks_open(clean, nodes, now, 600, 3600, want)) - def test_checks_never_ran_judges_only_a_gated_open_merge_on_a_rollup_read_whole(self) -> None: + def test_checks_never_ran_judges_only_an_open_gated_merge(self) -> None: + pr = payload([], merge="DIRTY", checks=[]) + self.assertIn("no check has posted", pr_review_checks.checks_never_ran(pr, [])) + nodes = pr_review_checks.check_nodes(payload([], merge="DIRTY", checks=[check()])) + self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes)) + for ungated in ({**pr, "mergeStateStatus": "CLEAN"}, {**pr, "state": "MERGED"}): + self.assertEqual("", pr_review_checks.checks_never_ran(ungated, [])) + other = payload([], merge="BLOCKED", checks=[check()], rollup_oid="f" * 40) + self.assertIn("no commit matching", pr_review_checks.checks_never_ran(other, [])) + + def test_required_unregistered_judges_only_a_rollup_read_whole(self) -> None: """A cut rollup can hold the absent context past its window, so it proves nothing.""" want = frozenset({("Later", None)}) pr = payload([], merge="BEHIND", checks=[check()]) nodes = pr_review_checks.check_nodes(pr) - self.assertIn("Later", pr_review_checks.checks_never_ran(pr, nodes, want)) - self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes, frozenset())) - for ungated in ({**pr, "mergeStateStatus": "CLEAN"}, {**pr, "state": "MERGED"}): - self.assertEqual("", pr_review_checks.checks_never_ran(ungated, nodes, want)) + self.assertEqual(["Later"], pr_review_checks.required_unregistered(pr, nodes, want)) + self.assertEqual([], pr_review_checks.required_unregistered(pr, nodes, frozenset())) pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"]["contexts"]["pageInfo"] = { "hasNextPage": True } - self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes, want)) + self.assertEqual([], pr_review_checks.required_unregistered(pr, nodes, want)) def test_a_rollup_not_read_whole_never_holds_on_an_absent_context(self) -> None: now = datetime.now(UTC) From ee530a4bd6fac64c6f9a137b597d2a7b3e334bdb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:19:25 -0700 Subject: [PATCH 3/8] Read Exit 45's Rules Through required_for and Name It CHECKS_MISSING The second local review found the unread-payload remedy pointing at status, which cannot reach exit 45, and no open-state guard on the DIRTY and BEHIND path. The rules now come through required_for, the unread-rules note no longer promises a hold this path never makes, and the status names checks missing from the read rather than checks not run, since an unread payload's checks may have run. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 34 +++++++++++++++-------------- tests/test_pr_review.py | 47 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 62 insertions(+), 21 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index c908d82b..84386884 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`. A head released short of those holds exits `45` with `status=CHECKS_NOT_RUN` in three cases, each 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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`. A head released short of those holds exits `45` with `status=CHECKS_MISSING` in three cases, each 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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 2b8f6f4e..33ed839c 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -118,10 +118,10 @@ workflow run the read reaches on that head has then completed. The read reaches the head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. A CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, an - attested held head exits 45 where its checks were unread or none posted. A DIRTY or - BEHIND one also exits 45 where a required check has not registered. Exit 0 can then - leave a required check unregistered only where the rules were unread, the rollup is - `CHECKS TRUNCATED`, or the merge reads DRAFT. + attested held head nothing holds exits 45 where its checks were unread or none posted. + A DIRTY or BEHIND one also exits 45 where a required check has not registered. Exit 0 + can then leave a required check unregistered only where the rules were unread, the + rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. 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. @@ -144,7 +144,7 @@ `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. - `status=CHECKS_NOT_RUN` names which. A DIRTY merge runs no pull_request workflow until + `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, and an empty rollup holds a BLOCKED, BEHIND, or DRAFT one. 49 = no Copilot round covers this head and none was requested, the pull request @@ -673,9 +673,11 @@ def parsed_json(proc: subprocess.CompletedProcess) -> object: return None -def required_for(pr: dict, owner: str, repo: str) -> frozenset[tuple[str, int | None]]: - """The required entries a BLOCKED merge can be short of, empty wherever none can matter.""" - if pr.get("mergeStateStatus") != "BLOCKED": +def required_for( + pr: dict, owner: str, repo: str, gated: frozenset[str] = frozenset({"BLOCKED"}) +) -> frozenset[tuple[str, int | None]]: + """The required entries a merge in `gated` can be short of, empty wherever none can matter.""" + if pr.get("mergeStateStatus") not in gated: return frozenset() return required_contexts(owner, repo, pr.get("baseRefName") or "") or frozenset() @@ -716,7 +718,7 @@ def required_contexts(owner: str, repo: str, base: str) -> frozenset[tuple[str, ): print( f"note: the rules for {base} were unread, so which checks they require is unknown and " - "this wait cannot hold for one that has not registered yet." + "this wait cannot judge one that has not registered." ) return None return frozenset((e[0], e[1]) for e in entries) @@ -2041,8 +2043,8 @@ def held_read() -> dict: return 44 merge = final.get("mergeStateStatus") unrun = checks_never_ran(final, checks) if covered else "" - if covered and not unrun and merge in ("DIRTY", "BEHIND"): - rules = required_contexts(owner, repo, final.get("baseRefName") or "") or frozenset() + if covered and not unrun and final.get("state", "OPEN") == "OPEN": + rules = required_for(final, owner, repo, frozenset({"DIRTY", "BEHIND"})) if unmet := required_unregistered(final, checks, rules): unrun = ( "a check the base branch requires has not registered, or came from " @@ -2050,11 +2052,11 @@ def held_read() -> dict: ) if unrun: print( - f"status=CHECKS_NOT_RUN an attested local pass covers this head and nothing holds " - f"the poll, but the merge reads {merge} and {unrun}, so the merge is not ready " - "and the poll does not clear it: a DIRTY merge runs no pull_request workflow " - "until its conflict is resolved, a BEHIND one may need the head updated from its " - "base, and a payload read off another commit wants status run again" + f"status=CHECKS_MISSING an attested local pass covers this head and nothing holds " + f"the poll, but {unrun}, and the merge reading {merge} is not ready while the " + "poll does not clear it: a DIRTY merge runs no pull_request workflow until its " + "conflict is resolved, a BEHIND one may need the head updated from its base, and " + "a payload read off another commit wants wait run again" ) return 45 return 0 diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 7d44b036..1df61a35 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3591,7 +3591,7 @@ def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(code, self.cli(["wait", "7", "--timeout", "0"])) if code == 45: - self.assertIn("status=CHECKS_NOT_RUN", self.out.getvalue()) + self.assertIn("status=CHECKS_MISSING", self.out.getvalue()) self.assertIn("no check has posted on the head", self.out.getvalue()) def test_an_optional_check_does_not_hold_once_the_required_gate_concluded(self) -> None: @@ -3691,10 +3691,49 @@ def test_a_required_check_unregistered_under_a_released_merge_exits_45(self) -> self.assertEqual(45, self.cli(["wait", "7", "--timeout", "1"])) slept.assert_not_called() out = self.out.getvalue() - self.assertIn("status=CHECKS_NOT_RUN", out) - self.assertIn(f"merge reads {merge}", out) + self.assertIn("status=CHECKS_MISSING", out) + self.assertIn(f"merge reading {merge}", out) self.assertIn("has not registered, or came from another app: Later workflow", out) + def test_a_released_head_exits_by_what_its_read_shows(self) -> None: + """Another app's check, an unread payload, an ungated merge, and a closed pull request.""" + name = "Check pull request workflow status job" + other_app = json.dumps({"n": 1, "c": [[name, 999]]}) + unread = payload([review(oid=OLD)], merge="DIRTY", checks=[check()], rollup_oid="d" * 40) + closed = payload([review(oid=OLD)], merge="DIRTY", checks=[check()]) + cases = [ + ( + "another app", + payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), + 45, + "came from another app: " + name, + ), + ("unread payload", unread, 45, "no commit matching the head"), + ("closed", {**closed, "state": "CLOSED"}, 0, ""), + ] + [ + (merge, payload([review(oid=OLD)], merge=merge, checks=[]), 0, "") + for merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS") + ] + for label, pr, code, reason in cases: + with self.subTest(label): + pr_review.required_contexts.cache_clear() + self.out.seek(0) + self.out.truncate() + self.enterContext( + mock.patch.object( + pr_review, + "gh_rest", + return_value=subprocess.CompletedProcess([], 0, other_app, ""), + ) + ) + self.answer(self.into(pr, attest=True)) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(code, self.cli(["wait", "7", "--timeout", "1"])) + self.assertIn(reason, self.out.getvalue()) + if code == 0: + self.assertNotIn("status=CHECKS_MISSING", self.out.getvalue()) + def test_a_released_merge_exits_0_where_the_unregistered_check_is_unproven(self) -> None: """Unread rules, a cut rollup, and a DRAFT merge each leave registration unjudged.""" cut = self.into(payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), attest=True) @@ -4014,7 +4053,7 @@ def test_an_unreadable_rollup_is_not_held_as_one_not_yet_registered(self) -> Non with mock.patch.object(pr_review.time, "sleep") as slept: self.assertEqual(45, self.cli(["wait", "7", "--timeout", "1"])) slept.assert_not_called() - self.assertIn("status=CHECKS_NOT_RUN", self.out.getvalue()) + self.assertIn("status=CHECKS_MISSING", self.out.getvalue()) self.assertIn("no commit matching the head", self.out.getvalue()) def test_an_outside_answer_outranks_pending_after_a_mid_poll_request(self) -> None: From 995ce78ad234a770c86109d2db28615d823f1fbd Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:26:09 -0700 Subject: [PATCH 4/8] Give Each Missing-Check Cause Its Own Remedy in CHECKS_MISSING The three findings the local pass left open on #3483: the 45 entry's hold sentence now names the conditions the hold needs, the README no longer counts its cases, and each cause carries its own remedy, a check from another app included. checks_never_ran now takes the rules and decides the whole state, so wait holds no second guard. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 24 +++++++----------------- scripts/pr_review_checks.py | 20 ++++++++++++++------ tests/test_pr_review.py | 20 ++++++++++++-------- 4 files changed, 34 insertions(+), 32 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 84386884..1f11a3cf 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`. A head released short of those holds exits `45` with `status=CHECKS_MISSING` in three cases, each 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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`. A 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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 33ed839c..a3ccf8a4 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -146,7 +146,8 @@ another app. That last needs the rules read and a rollup read whole. `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, and an empty rollup holds a BLOCKED, BEHIND, or DRAFT one. + 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. 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 @@ -193,7 +194,6 @@ checks_stuck, checks_tally, held_checks_open, - required_unregistered, runs_open, ) from pr_review_rounds import ( @@ -2041,22 +2041,12 @@ def held_read() -> dict: "the merge, because BLOCKED is also worn by a thread or a missing approval" ) return 44 - merge = final.get("mergeStateStatus") - unrun = checks_never_ran(final, checks) if covered else "" - if covered and not unrun and final.get("state", "OPEN") == "OPEN": - rules = required_for(final, owner, repo, frozenset({"DIRTY", "BEHIND"})) - if unmet := required_unregistered(final, checks, rules): - unrun = ( - "a check the base branch requires has not registered, or came from " - f"another app: {', '.join(unmet)}" - ) - if unrun: + rules = required_for(final, owner, repo, frozenset({"DIRTY", "BEHIND"})) + if covered and (unrun := checks_never_ran(final, checks, rules)): print( - f"status=CHECKS_MISSING an attested local pass covers this head and nothing holds " - f"the poll, but {unrun}, and the merge reading {merge} is not ready while the " - "poll does not clear it: a DIRTY merge runs no pull_request workflow until its " - "conflict is resolved, a BEHIND one may need the head updated from its base, and " - "a payload read off another commit wants wait run again" + "status=CHECKS_MISSING an attested local pass covers this head and nothing holds " + f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, and it " + "is not ready while the poll does not clear it" ) return 45 return 0 diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 6a3ccaf1..ad0da4da 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -297,17 +297,25 @@ def held_checks_open( return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) -def checks_never_ran(pr: dict, nodes: list[dict]) -> str: - """Why an open head's gated merge has no check of its own to settle on, or "" where it has. +def checks_never_ran( + pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] +) -> str: + """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 gated by nothing. Under any other merge, a payload with - no commit matching the head read no check, and a rollup with none posted ran 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. """ if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" if checks_unreadable(pr): - return "the payload carries no commit matching the head, so no check was read" - return "" if nodes else "no check has posted on the head" + return "the payload carries no commit matching the head, so no check was read: wait again" + if not nodes: + return "no check has posted, and a DIRTY merge runs none until its conflict is resolved" + unmet = ", ".join(required_unregistered(pr, nodes, required)) + 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" + ) def runs_open(pr: dict) -> list[str]: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 1df61a35..d1339849 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3592,7 +3592,7 @@ def test_an_empty_rollup_holds_any_merge_but_a_conflicted_one(self) -> None: self.assertEqual(code, self.cli(["wait", "7", "--timeout", "0"])) if code == 45: self.assertIn("status=CHECKS_MISSING", self.out.getvalue()) - self.assertIn("no check has posted on the head", self.out.getvalue()) + self.assertIn("but no check has posted", self.out.getvalue()) def test_an_optional_check_does_not_hold_once_the_required_gate_concluded(self) -> None: """A merge BLOCKED on a thread is not waiting on a check nothing requires.""" @@ -3692,8 +3692,8 @@ def test_a_required_check_unregistered_under_a_released_merge_exits_45(self) -> slept.assert_not_called() out = self.out.getvalue() self.assertIn("status=CHECKS_MISSING", out) - self.assertIn(f"merge reading {merge}", out) - self.assertIn("has not registered, or came from another app: Later workflow", out) + self.assertIn(f"The merge reads {merge}", out) + self.assertIn("came from another app, Later workflow, so update a BEHIND head", out) def test_a_released_head_exits_by_what_its_read_shows(self) -> None: """Another app's check, an unread payload, an ungated merge, and a closed pull request.""" @@ -3706,7 +3706,7 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: "another app", payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), 45, - "came from another app: " + name, + "came from another app, " + name, ), ("unread payload", unread, 45, "no commit matching the head"), ("closed", {**closed, "state": "CLOSED"}, 0, ""), @@ -6190,13 +6190,17 @@ def test_blocked_with_a_required_context_absent_holds(self) -> None: def test_checks_never_ran_judges_only_an_open_gated_merge(self) -> None: pr = payload([], merge="DIRTY", checks=[]) - self.assertIn("no check has posted", pr_review_checks.checks_never_ran(pr, [])) + self.assertIn("no check has posted", pr_review_checks.checks_never_ran(pr, [], frozenset())) nodes = pr_review_checks.check_nodes(payload([], merge="DIRTY", checks=[check()])) - self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes)) + self.assertEqual("", pr_review_checks.checks_never_ran(pr, nodes, frozenset())) + later = pr_review_checks.checks_never_ran(pr, nodes, frozenset({("Later", None)})) + self.assertIn("has not registered or came from another app, Later", later) for ungated in ({**pr, "mergeStateStatus": "CLEAN"}, {**pr, "state": "MERGED"}): - self.assertEqual("", pr_review_checks.checks_never_ran(ungated, [])) + self.assertEqual("", pr_review_checks.checks_never_ran(ungated, [], frozenset())) other = payload([], merge="BLOCKED", checks=[check()], rollup_oid="f" * 40) - self.assertIn("no commit matching", pr_review_checks.checks_never_ran(other, [])) + self.assertIn( + "no commit matching", pr_review_checks.checks_never_ran(other, [], frozenset()) + ) def test_required_unregistered_judges_only_a_rollup_read_whole(self) -> None: """A cut rollup can hold the absent context past its window, so it proves nothing.""" From 0ead5a306e83cb10ceb61a3617486700179bd5ac Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:31:12 -0700 Subject: [PATCH 5/8] Read Exit 45's Rules Only for an Attested Head The local pass on the fix push found the DIRTY and BEHIND rules read ahead of the attestation guard, so a Copilot-covered head made an API call and could print an unread-rules note on a run that judged nothing. The summary lines now name a check from another app, the unreadable remedy points at gh pr checks rather than another wait that can read the same mismatch, and _unmet_note judges only a rollup read whole. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 15 +++++++++------ scripts/pr_review_checks.py | 6 +++--- tests/test_pr_review.py | 19 ++++++++++++++----- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 1f11a3cf..6de33526 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -205,7 +205,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | `41` | `wait` | The newest round is a refusal or a quota state, its kind printed in the status line, and in `refusal=` for a refusal on this pull request. | | `42` | `status`, `wait` | The round covering the head states partial coverage. Or a non-refusal head round exists while an earlier round states partial, and one reads `UNKNOWN` or none states coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | -| `45` | `wait` | An attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its checks were unread or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check. | +| `45` | `wait` | An attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its checks were unread or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check or has it from another app. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | | `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | diff --git a/scripts/pr_review.py b/scripts/pr_review.py index a3ccf8a4..c118e01f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -119,9 +119,9 @@ head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. A CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, an attested held head nothing holds exits 45 where its checks were unread or none posted. - A DIRTY or BEHIND one also exits 45 where a required check has not registered. Exit 0 - can then leave a required check unregistered only where the rules were unread, the - rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. + A DIRTY or BEHIND one also exits 45 where a required check is unregistered or from + another app. Exit 0 can then leave a required check unregistered only where the rules + were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. 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. @@ -2024,7 +2024,7 @@ def held_read() -> dict: "had not completed, a check the base branch requires had not registered or came " "from another app, or the merge was still UNKNOWN, so the merge is not ready yet: " "wait again, or read the checks above" - + _unmet_note(required_for(final, owner, repo), checks) + + _unmet_note(final, required_for(final, owner, repo), checks) + (f" (not completed: {', '.join(runs)})" if runs else "") ) return 30 @@ -2041,8 +2041,11 @@ def held_read() -> dict: "the merge, because BLOCKED is also worn by a thread or a missing approval" ) return 44 - rules = required_for(final, owner, repo, frozenset({"DIRTY", "BEHIND"})) - if covered and (unrun := checks_never_ran(final, checks, rules)): + if covered and ( + unrun := checks_never_ran( + final, checks, required_for(final, owner, repo, frozenset({"DIRTY", "BEHIND"})) + ) + ): print( "status=CHECKS_MISSING an attested local pass covers this head and nothing holds " f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, and it " diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index ad0da4da..46fc4bdc 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -308,7 +308,7 @@ def checks_never_ran( if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" if checks_unreadable(pr): - return "the payload carries no commit matching the head, so no check was read: wait again" + return "the payload carries no commit matching the head, so no check was read: run gh pr checks" if not nodes: return "no check has posted, and a DIRTY merge runs none until its conflict is resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) @@ -364,9 +364,9 @@ def required_unregistered( return required_unmet(required, nodes) -def _unmet_note(required: frozenset[tuple[str, int | None]], nodes: list[dict]) -> str: +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_unmet(required, nodes) + unmet = required_unregistered(pr, nodes, required) return f" (required, unmet: {', '.join(unmet)})" if unmet else "" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index d1339849..babf1b4b 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3734,6 +3734,13 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: if code == 0: self.assertNotIn("status=CHECKS_MISSING", self.out.getvalue()) + def test_a_head_no_attestation_covers_reads_no_rules_for_exit_45(self) -> None: + """A Copilot round covering the head ends the wait on its own verdict, judging no check.""" + self.answer(payload([review()], merge="DIRTY", checks=[])) + with mock.patch.object(pr_review, "required_contexts") as read: + self.assertEqual(0, self.cli(["wait", "7"])) + read.assert_not_called() + def test_a_released_merge_exits_0_where_the_unregistered_check_is_unproven(self) -> None: """Unread rules, a cut rollup, and a DRAFT merge each leave registration unjudged.""" cut = self.into(payload([review(oid=OLD)], merge="BEHIND", checks=[check()]), attest=True) @@ -6266,11 +6273,13 @@ def test_an_unbound_entry_is_met_by_a_same_name_node_of_either_type(self) -> Non self.assertFalse(self.held([status_context("Gate")], ("Gate", None))) def test_the_unmet_context_is_named(self) -> None: - nodes = pr_review_checks.check_nodes( - payload([], merge="BLOCKED", checks=[check(name="Gate")]) - ) - self.assertEqual("", pr_review_checks._unmet_note(frozenset({("Gate", None)}), nodes)) - self.assertIn("Gate", pr_review_checks._unmet_note(frozenset({("Gate", 9)}), nodes)) + pr = payload([], merge="BLOCKED", checks=[check(name="Gate")]) + 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)) + rollup = pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"] + rollup["contexts"]["pageInfo"] = {"hasNextPage": True} + self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes)) def test_the_rules_are_read_only_for_a_blocked_merge(self) -> None: with mock.patch.object(pr_review, "gh_rest") as rest: From 5528f34b6ce9c88a4447556dd586bf843f1ba238 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:37:26 -0700 Subject: [PATCH 6/8] Say Exit 45 Needs an Open Pull Request and Merge the Unmet Readers The second local pass on the fix push found the help and README never saying the pull request must be open, though a closed one exits 0. required_unmet folds into required_unregistered, its only caller, and a test pins that the exit 45 rules read skips every merge but DIRTY and BEHIND. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 4 ++-- scripts/pr_review.py | 17 +++++++++-------- scripts/pr_review_checks.py | 17 ++++++----------- tests/test_pr_review.py | 22 +++++++++++++++------- 4 files changed, 32 insertions(+), 28 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 6de33526..b7427460 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -205,7 +205,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | `41` | `wait` | The newest round is a refusal or a quota state, its kind printed in the status line, and in `refusal=` for a refusal on this pull request. | | `42` | `status`, `wait` | The round covering the head states partial coverage. Or a non-refusal head round exists while an earlier round states partial, and one reads `UNKNOWN` or none states coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | -| `45` | `wait` | An attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its checks were unread or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check or has it from another app. | +| `45` | `wait` | An open attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its rollup was unreadable or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check or has it from another app. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | | `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | @@ -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`. A 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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. `--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`. Exit `0` can then leave a required check unregistered only where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. A pull request no longer open exits `0` as well. `--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 c118e01f..51e56670 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -118,10 +118,11 @@ workflow run the read reaches on that head has then completed. The read reaches the head's last 50 check suites and those of its first CHECKS_WINDOW rollup contexts. A CLEAN, UNSTABLE, or HAS_HOOKS merge owes no required check. Under any other merge, an - attested held head nothing holds exits 45 where its checks were unread or none posted. - A DIRTY or BEHIND one also exits 45 where a required check is unregistered or from - another app. Exit 0 can then leave a required check unregistered only where the rules - were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. + open attested held head nothing holds exits 45 where its rollup was unreadable or none + posted. A DIRTY or BEHIND one also exits 45 where a required check is unregistered or + from another app. Exit 0 can then leave a required check unregistered only where the + rules were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. A pull + request no longer open exits 0 as well. 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. @@ -139,8 +140,8 @@ is in a shape no wait clears. It is queued with nothing acting on it, expected and never posted, running far past the job's cost, held by its environment, or failed. On a held head a check running long or held by its environment polls to 30 instead. - 45 = an attested held head that nothing holds, its merge other than CLEAN, UNSTABLE, or - HAS_HOOKS. Either the payload carries no commit matching the head, named + 45 = an open attested held head that nothing holds, its merge other than CLEAN, + 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. @@ -2048,8 +2049,8 @@ def held_read() -> dict: ): print( "status=CHECKS_MISSING an attested local pass covers this head and nothing holds " - f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, and it " - "is not ready while the poll does not clear it" + f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, which " + "is not ready, and polling longer does not change that" ) return 45 return 0 diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 46fc4bdc..de289953 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -340,8 +340,12 @@ def runs_open(pr: dict) -> list[str]: return sorted(named) -def required_unmet(required: frozenset[tuple[str, int | None]], nodes: list[dict]) -> list[str]: - """The contexts among `required` that no rollup node satisfies, sorted.""" +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.""" + if checks_unreadable(pr) or checks_truncated(pr): + return [] seen = [n for n in nodes if not n.get("unreadable")] return sorted( { @@ -355,15 +359,6 @@ def required_unmet(required: frozenset[tuple[str, int | None]], nodes: list[dict ) -def required_unregistered( - pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] -) -> list[str]: - """The contexts among `required` the head's rollup lacks, judged only on a rollup read whole.""" - if checks_unreadable(pr) or checks_truncated(pr): - return [] - return required_unmet(required, nodes) - - 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) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index babf1b4b..86014aa0 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3714,18 +3714,18 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: (merge, payload([review(oid=OLD)], merge=merge, checks=[]), 0, "") for merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS") ] + self.enterContext( + mock.patch.object( + pr_review, + "gh_rest", + return_value=subprocess.CompletedProcess([], 0, other_app, ""), + ) + ) for label, pr, code, reason in cases: with self.subTest(label): pr_review.required_contexts.cache_clear() self.out.seek(0) self.out.truncate() - self.enterContext( - mock.patch.object( - pr_review, - "gh_rest", - return_value=subprocess.CompletedProcess([], 0, other_app, ""), - ) - ) self.answer(self.into(pr, attest=True)) self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) with mock.patch.object(pr_review.time, "sleep"): @@ -6287,6 +6287,14 @@ def test_the_rules_are_read_only_for_a_blocked_merge(self) -> None: self.assertEqual(frozenset(), pr_review.required_for(clean, "o", "r")) rest.assert_not_called() + def test_the_rules_for_exit_45_are_read_only_for_a_dirty_or_behind_merge(self) -> None: + gated = frozenset({"DIRTY", "BEHIND"}) + with mock.patch.object(pr_review, "gh_rest") as rest: + for merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS", "BLOCKED", "DRAFT"): + pr = payload([], merge=merge, checks=[check()]) + self.assertEqual(frozenset(), pr_review.required_for(pr, "o", "r", gated)) + rest.assert_not_called() + class TestRunsOpen(unittest.TestCase): """A head's workflow run that has not completed holds a held wait whatever the merge reads.""" From e2eb0df86750ff9fe3fe2233c46914e8cb4c879a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:42:06 -0700 Subject: [PATCH 7/8] Point an Unreadable Payload at the Head Commit's Own Check Runs The local pass on the last push found the unreadable remedy naming a command that reads the same last commit, the status line claiming the poll cannot help a payload that a later read may fix, and the README row and the closed sentence wider than the code. required_for now skips a pull request no longer open, so a closed one reads no rules. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 4 ++-- scripts/pr_review.py | 8 ++++---- scripts/pr_review_checks.py | 2 +- tests/test_pr_review.py | 2 ++ 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index b7427460..3060ebf9 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -205,7 +205,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj | `41` | `wait` | The newest round is a refusal or a quota state, its kind printed in the status line, and in `refusal=` for a refusal on this pull request. | | `42` | `status`, `wait` | The round covering the head states partial coverage. Or a non-refusal head round exists while an earlier round states partial, and one reads `UNKNOWN` or none states coverage. | | `44` | `wait` | The review loop closed, the merge reads `BLOCKED`, and a required check is in a shape no wait clears. | -| `45` | `wait` | An open attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its rollup was unreadable or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check or has it from another app. | +| `45` | `wait` | An open attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its rollup was unreadable or none posted, or a `DIRTY` or `BEHIND` merge lacks a required check or has it from another app, judged on rules read and a whole rollup. | | `49` | `wait` | No Copilot round covers the head and none was requested, on a pull request into a branch other than the default with no attestation. | | `50` | `status`, `wait` | The round covering the head opens on a verdict other than the clean one while it counts no finding. Or a head round carries a part the reader cannot place, and nothing acknowledges its fingerprint. | | `60` to `63` | `reply` | No thread matched, more than one did, the reply returned no comment url, or the resolve did not report the thread resolved. | @@ -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 where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. A pull request no longer open exits `0` as well. `--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`. Exit `0` can then leave a required check unregistered only where the rules were unread, the rollup was cut, or the merge reads `DRAFT`. A pull request no longer open never exits `45`. `--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 51e56670..06cba3a3 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -122,7 +122,7 @@ posted. A DIRTY or BEHIND one also exits 45 where a required check is unregistered or from another app. Exit 0 can then leave a required check unregistered only where the rules were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. A pull - request no longer open exits 0 as well. + request no longer open never exits 45. 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. @@ -678,7 +678,7 @@ def required_for( pr: dict, owner: str, repo: str, gated: frozenset[str] = frozenset({"BLOCKED"}) ) -> frozenset[tuple[str, int | None]]: """The required entries a merge in `gated` can be short of, empty wherever none can matter.""" - if pr.get("mergeStateStatus") not in gated: + if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") not in gated: return frozenset() return required_contexts(owner, repo, pr.get("baseRefName") or "") or frozenset() @@ -2049,8 +2049,8 @@ def held_read() -> dict: ): print( "status=CHECKS_MISSING an attested local pass covers this head and nothing holds " - f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, which " - "is not ready, and polling longer does not change that" + f"the poll, but {unrun}. The merge reads {final.get('mergeStateStatus')}, so it is " + "not ready to merge" ) return 45 return 0 diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index de289953..4a29499a 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -308,7 +308,7 @@ def checks_never_ran( if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" if checks_unreadable(pr): - return "the payload carries no commit matching the head, so no check was read: run gh pr checks" + return "the payload carries no commit matching the head, so read the check runs on the head commit" if not nodes: return "no check has posted, and a DIRTY merge runs none until its conflict is resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 86014aa0..b18bf024 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -6293,6 +6293,8 @@ def test_the_rules_for_exit_45_are_read_only_for_a_dirty_or_behind_merge(self) - for merge in ("CLEAN", "UNSTABLE", "HAS_HOOKS", "BLOCKED", "DRAFT"): pr = payload([], merge=merge, checks=[check()]) self.assertEqual(frozenset(), pr_review.required_for(pr, "o", "r", gated)) + closed = {**payload([], merge="DIRTY", checks=[check()]), "state": "CLOSED"} + self.assertEqual(frozenset(), pr_review.required_for(closed, "o", "r", gated)) rest.assert_not_called() From 8f29cadd36b5e90bd99fe44afb5c6699612dc403 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 14:47:12 -0700 Subject: [PATCH 8/8] Name the pull_request Workflow a DIRTY Merge Does Not Run A push-triggered workflow still runs on a conflicted head, so the empty-rollup cause names the pull_request workflow, as the help and README already do. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review_checks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 4a29499a..94f602a0 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -310,7 +310,7 @@ def checks_never_ran( if checks_unreadable(pr): return "the payload carries no commit matching the head, so read the check runs on the head commit" if not nodes: - return "no check has posted, and a DIRTY merge runs none until its conflict is resolved" + return "no check has posted, and a DIRTY merge runs no pull_request workflow until resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) return unmet and ( f"a required check has not registered or came from another app, {unmet}, so update a "