From b4921504300e9af2e7d97456a01be91d2103e497 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 16:16:39 -0700 Subject: [PATCH 1/2] Scope the No-Check-Posted Cause to DIRTY and Give It a Remedy The third promotion review of #3484 found the exit 45 entry and README row offering "no check posted" for any gated merge, though only DIRTY releases one, and that cause naming no remedy. It now says to resolve the head's conflict with its base, and the test table checks a reason only where one is expected. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 10 +++++----- scripts/pr_review_checks.py | 5 ++++- tests/test_pr_review.py | 3 ++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 732392dbb..eeae0d7a2 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, judged on rules read and a whole rollup. | +| `45` | `wait` | An open attested held head that nothing holds, its merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. Its rollup was unreadable, a `DIRTY` merge has 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. | diff --git a/scripts/pr_review.py b/scripts/pr_review.py index adad84f4f..0d8da0014 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -143,11 +143,11 @@ a held head a check running long or held by its environment polls to 30 instead. 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 not `CHECKS TRUNCATED`. Where - the rollup holds a type this cannot read, named `CHECKS PARTIALLY UNREAD` in the - digest, the status says the check may sit there. + `CHECKS UNREADABLE` in the digest, or a DIRTY merge has no check 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 not + `CHECKS TRUNCATED`. Where the rollup holds a type this cannot read, named + `CHECKS PARTIALLY UNREAD` in the digest, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to 30 instead, where the rules and the head's rollup were read, not `CHECKS TRUNCATED`. An diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 5c8d8643f..01f19f154 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -311,7 +311,10 @@ 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 no pull_request workflow until resolved" + return ( + "no check has posted, so resolve the head's conflict with its base, since a DIRTY " + "merge runs no pull_request workflow until then" + ) unmet = ", ".join(required_unregistered(pr, nodes, required)) unread = checks_unread(nodes) where = ", sits in the rollup's unread part," if unread else "" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index aec58f356..adddc0b3d 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3742,7 +3742,8 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: 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 reason: + self.assertIn(reason, self.out.getvalue()) if label == "another app": self.assertNotIn("checks and statuses first", self.out.getvalue()) if code == 0: From ba37c856defe608b768cc039af5d9f4542445dc5 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 16:19:35 -0700 Subject: [PATCH 2/2] Scope the Exit 0 Paragraph's No-Check-Posted Clause to DIRTY The local pass found the exit 0 paragraph still offering exit 45 for a rollup with none posted under any gated merge, and no test pinning the new remedy. The paragraph now scopes that clause to DIRTY, and the test asserts the remedy. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 9 +++++---- tests/test_pr_review.py | 5 ++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 0d8da0014..5a47703ac 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 - 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 under unread - rules, a `CHECKS TRUNCATED` rollup, a DRAFT merge, or a pull request not open. + open attested held head nothing holds exits 45 where its rollup was unreadable or, under + DIRTY, 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 under unread rules, a `CHECKS TRUNCATED` rollup, a DRAFT merge, or a pull request + not open. 30 = still pending at the timeout. On a held head it is any hold `status=CHECKS_PENDING` names, a required check still settling and a workflow run still open among them. Where the rollup holds a type this cannot read, a BLOCKED hold's note diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index adddc0b3d..e33f9b84d 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3592,7 +3592,10 @@ 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("but no check has posted", self.out.getvalue()) + self.assertIn( + "but no check has posted, so resolve the head's conflict", + 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."""