From 36292107fcc92dd076599ab550b58cdb89770415 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 11 Oct 2026 12:36:20 -0700 Subject: [PATCH 1/4] Hold an Attested Wait Until the Base Branch's Required Checks Register The wait read a BLOCKED merge with every registered check passed as nothing left to wait for, because the rollup only lists checks that have registered. It now reads the required contexts from the base branch's rules and holds while one is absent. Attest also waits up to about a minute for GitHub to move the pull request's head to a push whose remote tip is the checkout's own commit, instead of refusing at once. Co-Authored-By: Claude Sonnet 5.5 --- scripts/pr_review.py | 102 ++++++++++++++++++++++++++++- tests/test_pr_review.py | 141 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 239 insertions(+), 4 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 5e2b01576..2757006b0 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -268,7 +268,8 @@ no wait clears: queued with nothing acting on it, expected and never posted, running far past what the job costs, or failed, though on a held head a required check running long is still polled, as 30 states. A check merely still running normally is not this and - exits 0, or on a held head is polled while it can still move the merge. Neither is a + exits 0, or on a held head is polled while it can still move the merge, which includes + a context the base branch's rules require and the head has not yet registered. Neither is a stuck check its rollup node reads as not required, since the rollup carries checks no ruleset requires, nor any stuck check on a merge that is not BLOCKED. The digest reports the check in every case, so a shape outside 44 is still named rather than lost. @@ -780,6 +781,7 @@ def strip_fences( # The REST compare endpoint's own ceiling on the file list it returns, which it does not flag. COMPARE_FILE_CAP = 300 READ_RETRY_DELAYS = (2, 5) +ATTEST_HEAD_DELAYS = (3, 5, 10, 15, 15, 12) # What a git ref may not hold before it is interpolated into a REST path. # Refused rather than allow-listed, git permitting more than a branch name usually carries. # An allow-list of the usual ASCII refused `release/1.0+build` and any non-ASCII name alike. @@ -958,6 +960,10 @@ def strip_fences( query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} """ +Q_ATTEST_BRANCH = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefName headRepository{ url } } }} +""" ATTESTATION = re.compile( r"^$", re.MULTILINE, @@ -3646,7 +3652,12 @@ def checks_settling(nodes: list[dict], now: datetime, grace: float, stall: float def held_checks_open( - pr: dict, nodes: list[dict], now: datetime, grace: float, stall: float + pr: dict, + nodes: list[dict], + now: datetime, + grace: float, + stall: float, + required: frozenset[str] = frozenset(), ) -> bool: """True where a covered head's checks can still move its merge, so a held wait polls on. @@ -3661,6 +3672,12 @@ def held_checks_open( reading UNKNOWN holds it whatever the rollup carries, since GitHub has not yet decided the word the exit code is chosen by. A pull request no longer open has no merge left to move, and GitHub reads UNKNOWN on it for good. + + A BLOCKED merge also holds it while a context the base branch's rules require is absent from the + rollup by exact name, which `required` carries. The rollup only knows the checks that have + registered, so a push whose other workflows have not started reads five of five passed and + BLOCKED. An empty `required` is a rules read that found nothing or did not happen, and it + holds nothing. """ if pr.get("state", "OPEN") != "OPEN": return False @@ -3679,9 +3696,49 @@ def held_checks_open( return True if settling and not any(n.get("required") for n in nodes): return True + if merge == "BLOCKED" and required - {n["name"] for n in nodes if not n.get("unreadable")}: + return True return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) +@functools.cache +def required_contexts(owner: str, repo: str, base: str) -> frozenset[str] | None: + """Every status check context the base branch's rules require, or None where it was unread. + + Read from the rules rather than the rollup, since the rollup lists only the checks that have + registered on the head, which is exactly the set a push in flight is short of. None rather than + an empty set wherever the read did not happen, so an unreadable rules endpoint never reads as + nothing being required. The read is one page of a hundred rules, and a full page is taken as + possibly cut short and returned as None. Memoized for the run, which also prints the note once. + """ + proc = ( + gh_rest( + f"repos/{owner}/{repo}/rules/branches/{base}?per_page=100", + '{n: length, c: [.[] | select(.type=="required_status_checks")' + " | .parameters.required_status_checks[].context]}", + ) + if base and not UNSAFE_REF.search(base) and not DOT_SEGMENT.search(base) + else None + ) + try: + read = json.loads(proc.stdout) if proc is not None and proc.returncode == 0 else None + except ValueError: + read = None + answer = read if isinstance(read, dict) else {} + contexts = answer.get("c") + if ( + not isinstance(contexts, list) + or not all(isinstance(c, str) and c for c in contexts) + or answer.get("n", 100) >= 100 + ): + 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." + ) + return None + return frozenset(contexts) + + def checks_truncated(pr: dict) -> bool: """True where the head's rollup carries more contexts than the query asked for. @@ -4858,11 +4915,17 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: f"status=CHECKOUT_NOT_READ nothing was written: {checkout} is not a readable checkout" ) return 67 + registered = True + if local.stdout.strip() != head: + head, registered = await_registered_head( + owner, repo, num, checkout, local.stdout.strip(), head + ) if local.stdout.strip() != head: print( f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout is at " f"{local.stdout.strip()[:8]} and the pull request's head is {head[:8]}, so push or " "fetch until they agree" + + ("" if registered else ". The push landed and GitHub had not registered it yet") ) return 67 if dirty.stdout.strip(): @@ -4925,6 +4988,31 @@ def attest(owner: str, repo: str, num: int, checkout: str) -> int: return comment_on_pr(owner, repo, num, body) +def await_registered_head( + owner: str, repo: str, num: int, checkout: str, local: str, head: str +) -> tuple[str, bool]: + """The pull request's head after waiting for GitHub to register a push, and whether it did. + + A push moves the remote branch at once and the pull request's head a moment later, so an + attest straight after a push read the old head and refused. The wait applies only where the + remote branch tip is the checkout's own commit, which is what makes the head merely late. + Any other tip, or one that cannot be read, returns at once with the head as read. + """ + meta = gql(Q_ATTEST_BRANCH, owner, repo, num) or {} + ref, source = meta.get("headRefName") or "", (meta.get("headRepository") or {}).get("url") or "" + if not ref or not source or UNSAFE_REF.search(ref) or DOT_SEGMENT.search(ref): + return head, True + tip = _git(checkout, "ls-remote", source, f"refs/heads/{ref}") + if tip.returncode != 0 or tip.stdout.split()[:1] != [local]: + return head, True + for delay in ATTEST_HEAD_DELAYS: + time.sleep(delay) + head = (gql(Q_ATTEST_TARGET, owner, repo, num) or {}).get("headRefOid") or head + if head == local: + return head, True + return head, False + + def receipt_reading(checkout: str, base: str) -> tuple[bool, str, str]: """Whether a recorded pass covers the checkout's content, its findings, and what was read. @@ -5686,6 +5774,7 @@ def still_held(polled: dict) -> bool: datetime.now(UTC), a.check_grace, a.check_stall, + required_contexts(owner, repo, polled.get("baseRefName") or "") or frozenset(), ) ) @@ -5774,7 +5863,14 @@ def held_read() -> dict: # Only once the review itself is sound does a stuck required check decide the code. if verdict: return verdict - if covered and held_checks_open(final, checks, now, a.check_grace, a.check_stall): + if covered and held_checks_open( + final, + checks, + now, + a.check_grace, + a.check_stall, + required_contexts(owner, repo, final.get("baseRefName") or "") or frozenset(), + ): print( "status=CHECKS_PENDING an attested local pass covers this head, and by the " "timeout a check had not concluded, none had posted, or the merge was still " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 16140fad4..914fc2b37 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -504,6 +504,8 @@ def setUp(self) -> None: self.addCleanup(pr_review.tree_changed_at.cache_clear) pr_review.run_cause.cache_clear() self.addCleanup(pr_review.run_cause.cache_clear) + pr_review.required_contexts.cache_clear() + self.addCleanup(pr_review.required_contexts.cache_clear) self.enterContext( mock.patch.object( pr_review, @@ -7916,6 +7918,55 @@ def test_an_unattested_held_head_is_not_polled_for_its_checks(self) -> None: self.assertEqual(49, self.cli(["wait", "7", "--timeout", "1"])) slept.assert_not_called() + def rules(self, *contexts: str) -> mock.MagicMock: + """Patch `gh_rest` to answer the base branch's rules read with these required contexts.""" + out = json.dumps({"n": 2, "c": list(contexts)}) + return self.enterContext( + mock.patch.object( + pr_review, "gh_rest", return_value=subprocess.CompletedProcess([], 0, out, "") + ) + ) + + def test_a_required_check_not_yet_registered_holds_an_attested_head(self) -> None: + """Five of five passed and BLOCKED is a push whose other workflows have not registered.""" + self.rules("Check pull request workflow status job", "Later workflow") + short = self.into( + payload([review(oid=OLD)], merge="BLOCKED", checks=[check()]), attest=True + ) + full = self.into( + payload([review(oid=OLD)], checks=[check(), check(name="Later workflow")]), attest=True + ) + self.answer(short, short, full) + 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"])) + slept.assert_called_once() + self.out.seek(0) + self.out.truncate() + self.answer(short) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("status=CHECKS_PENDING", self.out.getvalue()) + + def test_every_required_check_registered_does_not_hold_a_blocked_head(self) -> None: + self.rules("Check pull request workflow status job") + pr = self.into(payload([review(oid=OLD)], merge="BLOCKED", 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.assertNotEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + slept.assert_not_called() + + 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) + self.answer(pr) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertNotEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertIn("rules for develop were unread", self.out.getvalue()) + def test_a_check_nothing_requires_does_not_hold_an_attested_head(self) -> None: """A merge reading CLEAN, UNSTABLE, or HAS_HOOKS has no required check outstanding.""" running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) @@ -10171,6 +10222,40 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No gh_graphql.assert_not_called() +class TestRequiredContexts(unittest.TestCase): + """The rules read names what is required, and the hold arm compares it with the rollup.""" + + def setUp(self) -> None: + pr_review.required_contexts.cache_clear() + self.addCleanup(pr_review.required_contexts.cache_clear) + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + def read(self, out: str, code: int = 0) -> frozenset[str] | None: + proc = subprocess.CompletedProcess([], code, out, "") + with mock.patch.object(pr_review, "gh_rest", return_value=proc): + return pr_review.required_contexts("o", "r", "develop") + + def test_the_contexts_are_read_from_the_rules(self) -> None: + self.assertEqual(frozenset({"a", "b"}), self.read('{"n": 3, "c": ["a", "b"]}')) + + def test_a_read_that_did_not_happen_is_none_not_empty(self) -> None: + for out, code in (("", 1), ("not json", 0), ('{"n": 1}', 0), ('{"n": 100, "c": []}', 0)): + with self.subTest(out=out, code=code): + pr_review.required_contexts.cache_clear() + self.assertIsNone(self.read(out, code)) + + def test_blocked_with_a_required_context_absent_holds(self) -> None: + pr = payload([], merge="BLOCKED", checks=[check()]) + nodes = pr_review.check_nodes(pr) + now = datetime.now(UTC) + want = frozenset({"Check pull request workflow status job", "Later"}) + self.assertTrue(pr_review.held_checks_open(pr, nodes, now, 600, 3600, want)) + self.assertFalse(pr_review.held_checks_open(pr, nodes, now, 600, 3600, want - {"Later"})) + self.assertFalse(pr_review.held_checks_open(pr, nodes, now, 600, 3600)) + clean = {**pr, "mergeStateStatus": "CLEAN"} + self.assertFalse(pr_review.held_checks_open(clean, nodes, now, 600, 3600, want)) + + class TestAttest(unittest.TestCase): """A local pass is published only for the content the pull request's head carries.""" @@ -10213,7 +10298,8 @@ def post(_o: str, _r: str, _n: int, body: str) -> int: return 0 self.enterContext(mock.patch.object(pr_review, "comment_on_pr", side_effect=post)) - self.enterContext(contextlib.redirect_stdout(io.StringIO())) + self.out = io.StringIO() + self.enterContext(contextlib.redirect_stdout(self.out)) def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> int: """Run `attest` with a stub `local_review.py` that records how it was called.""" @@ -10251,6 +10337,59 @@ def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: for cwd, _ in calls: self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) + def lagging(self, tip: str, heads: list[str]) -> int: + """Run `attest` while the pull request's head reads each of `heads` in turn.""" + replies = iter(heads) + target = {"headRefOid": heads[0], "baseRefName": "develop"} + + def gql(query: str, *_: object) -> dict: + if query == pr_review.Q_ATTEST_BRANCH: + return {"headRefName": "feature", "headRepository": {"url": "https://x.test/o/r"}} + return {**target, "headRefOid": next(replies, heads[-1])} + + self.slept: list[float] = [] + remote = subprocess.CompletedProcess([], 0, f"{tip}\trefs/heads/feature\n", "") + original = pr_review._git + + def git(checkout: str, *args: str) -> subprocess.CompletedProcess: + return remote if args[0] == "ls-remote" else original(checkout, *args) + + merge_base = subprocess.CompletedProcess([], 0, self.head + "\n", "") + stub = Path(self.dir).parent / "stub-lag.py" + stub.write_text( + "import json, sys\n" + "print(json.dumps({'covered': True, 'receiptProblems': [], 'findings': {'a': 0}}))\n" + ) + self.addCleanup(stub.unlink) + with ( + mock.patch.object(pr_review, "gql", side_effect=gql), + mock.patch.object(pr_review, "_git", side_effect=git), + mock.patch.object(pr_review, "gh_rest", return_value=merge_base), + mock.patch.object(pr_review, "LOCAL_REVIEW", stub), + mock.patch.object(pr_review.time, "sleep", side_effect=self.slept.append), + ): + return pr_review.attest("o", "r", 7, self.dir) + + def test_a_head_that_catches_up_within_the_bound_is_attested(self) -> None: + old = "d" * 40 + self.assertEqual(0, self.lagging(self.head, [old, old, self.head])) + self.assertEqual(2, len(self.slept)) + self.assertEqual(1, len(self.posted)) + + def test_a_head_that_never_catches_up_is_refused_after_the_bound(self) -> None: + old = "d" * 40 + self.assertEqual(67, self.lagging(self.head, [old])) + self.assertLessEqual(sum(self.slept), 60) + self.assertGreater(sum(self.slept), 0) + self.assertIn("had not registered it yet", self.out.getvalue()) + self.assertEqual([], self.posted) + + def test_a_remote_tip_that_is_not_the_checkout_is_refused_at_once(self) -> None: + old = "d" * 40 + self.assertEqual(67, self.lagging("e" * 40, [old])) + self.assertEqual([], self.slept) + self.assertNotIn("had not registered", self.out.getvalue()) + def test_a_checkout_measuring_another_merge_base_is_refused(self) -> None: self.assertEqual(67, self.run_attest(self.head, base="e" * 40)) self.assertEqual([], self.posted) From 00c377a87ef76f4078d85ee2afb07581155bdfc2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 11 Oct 2026 12:43:39 -0700 Subject: [PATCH 2/4] Hold Only on a Whole Rollup and Read the Rules Only for a Blocked Merge Review of the first commit found the hold fired on an unreadable or truncated rollup, read the rules on every covered head, skipped the retry schedule, and called a head that moved to a third commit unregistered. Each is fixed and tested, and the README states the new pending case. Co-Authored-By: Claude Sonnet 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 41 +++++++++++++++++++++++++++-------------- tests/test_pr_review.py | 27 +++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 15 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index e552bff8e..ee5808c76 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -182,7 +182,7 @@ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent. -`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. +`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also a check the base branch's rules require that the head has not registered yet, on a `BLOCKED` merge whose rollup was read whole. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. `wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 2757006b0..1d2f84f3f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -3673,8 +3673,8 @@ def held_checks_open( the exit code is chosen by. A pull request no longer open has no merge left to move, and GitHub reads UNKNOWN on it for good. - A BLOCKED merge also holds it while a context the base branch's rules require is absent from the - rollup by exact name, which `required` carries. The rollup only knows the checks that have + A BLOCKED merge also holds it, where the rollup was read whole, while a context the base + branch's rules require is absent from it by exact name, which `required` carries. The rollup only knows the checks that have registered, so a push whose other workflows have not started reads five of five passed and BLOCKED. An empty `required` is a rules read that found nothing or did not happen, and it holds nothing. @@ -3696,11 +3696,23 @@ def held_checks_open( return True if settling and not any(n.get("required") for n in nodes): return True - if merge == "BLOCKED" and required - {n["name"] for n in nodes if not n.get("unreadable")}: + if ( + merge == "BLOCKED" + and not checks_unreadable(pr) + and not checks_truncated(pr) + and required - {n["name"] for n in nodes if not n.get("unreadable")} + ): return True return not nodes and merge in ("BLOCKED", "BEHIND", "DRAFT") and not checks_unreadable(pr) +def required_for(pr: dict, owner: str, repo: str) -> frozenset[str]: + """The required contexts a BLOCKED merge can be short of, empty wherever none can matter.""" + if pr.get("mergeStateStatus") != "BLOCKED": + return frozenset() + return required_contexts(owner, repo, pr.get("baseRefName") or "") or frozenset() + + @functools.cache def required_contexts(owner: str, repo: str, base: str) -> frozenset[str] | None: """Every status check context the base branch's rules require, or None where it was unread. @@ -3711,19 +3723,16 @@ def required_contexts(owner: str, repo: str, base: str) -> frozenset[str] | None nothing being required. The read is one page of a hundred rules, and a full page is taken as possibly cut short and returned as None. Memoized for the run, which also prints the note once. """ - proc = ( - gh_rest( + safe = base and not UNSAFE_REF.search(base) and not DOT_SEGMENT.search(base) + read = ( + reread_json( f"repos/{owner}/{repo}/rules/branches/{base}?per_page=100", '{n: length, c: [.[] | select(.type=="required_status_checks")' " | .parameters.required_status_checks[].context]}", ) - if base and not UNSAFE_REF.search(base) and not DOT_SEGMENT.search(base) + if safe else None ) - try: - read = json.loads(proc.stdout) if proc is not None and proc.returncode == 0 else None - except ValueError: - read = None answer = read if isinstance(read, dict) else {} contexts = answer.get("c") if ( @@ -5005,11 +5014,14 @@ def await_registered_head( tip = _git(checkout, "ls-remote", source, f"refs/heads/{ref}") if tip.returncode != 0 or tip.stdout.split()[:1] != [local]: return head, True + first = head for delay in ATTEST_HEAD_DELAYS: time.sleep(delay) head = (gql(Q_ATTEST_TARGET, owner, repo, num) or {}).get("headRefOid") or head if head == local: return head, True + if head != first: + return head, True return head, False @@ -5774,7 +5786,7 @@ def still_held(polled: dict) -> bool: datetime.now(UTC), a.check_grace, a.check_stall, - required_contexts(owner, repo, polled.get("baseRefName") or "") or frozenset(), + required_for(polled, owner, repo), ) ) @@ -5869,12 +5881,13 @@ def held_read() -> dict: now, a.check_grace, a.check_stall, - required_contexts(owner, repo, final.get("baseRefName") or "") or frozenset(), + required_for(final, owner, repo), ): print( "status=CHECKS_PENDING an attested local pass covers this head, and by the " - "timeout a check had not concluded, none had posted, or the merge was still " - "UNKNOWN, so the merge is not ready yet: wait again, or read the checks above" + "timeout a check had not concluded, none had posted, a check the base branch " + "requires had not registered, or the merge was still UNKNOWN, so the merge is " + "not ready yet: wait again, or read the checks above" ) return 30 # The review loop closing is not the merge gate, and 0 alone was saying it was. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 914fc2b37..f0ac1bca4 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -10255,6 +10255,27 @@ def test_blocked_with_a_required_context_absent_holds(self) -> None: clean = {**pr, "mergeStateStatus": "CLEAN"} self.assertFalse(pr_review.held_checks_open(clean, nodes, now, 600, 3600, want)) + def test_a_rollup_not_read_whole_never_holds_on_an_absent_context(self) -> None: + now = datetime.now(UTC) + want = frozenset({"Later"}) + other = payload([], merge="BLOCKED", checks=[check()], rollup_oid="f" * 40) + self.assertFalse( + pr_review.held_checks_open(other, pr_review.check_nodes(other), now, 600, 3600, want) + ) + cut = payload([], merge="BLOCKED", checks=[check()]) + cut["commits"]["nodes"][0]["commit"]["statusCheckRollup"]["contexts"]["pageInfo"] = { + "hasNextPage": True + } + self.assertFalse( + pr_review.held_checks_open(cut, pr_review.check_nodes(cut), now, 600, 3600, want) + ) + + def test_the_rules_are_read_only_for_a_blocked_merge(self) -> None: + with mock.patch.object(pr_review, "gh_rest") as rest: + clean = payload([], merge="CLEAN", checks=[check()]) + self.assertEqual(frozenset(), pr_review.required_for(clean, "o", "r")) + rest.assert_not_called() + class TestAttest(unittest.TestCase): """A local pass is published only for the content the pull request's head carries.""" @@ -10384,6 +10405,12 @@ def test_a_head_that_never_catches_up_is_refused_after_the_bound(self) -> None: self.assertIn("had not registered it yet", self.out.getvalue()) self.assertEqual([], self.posted) + def test_a_head_that_moves_to_a_third_commit_is_not_called_unregistered(self) -> None: + old, other = "d" * 40, "c" * 40 + self.assertEqual(67, self.lagging(self.head, [old, other])) + self.assertEqual(1, len(self.slept)) + self.assertNotIn("had not registered", self.out.getvalue()) + def test_a_remote_tip_that_is_not_the_checkout_is_refused_at_once(self) -> None: old = "d" * 40 self.assertEqual(67, self.lagging("e" * 40, [old])) From f986606f2d6c19d318d4aecbd70f2fc3ebb8440c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 11 Oct 2026 12:49:19 -0700 Subject: [PATCH 3/4] Skip the Attest Head Wait for a Pull Request That Is Not Open A closed or merged pull request never moves its head, so waiting a minute for it and then calling the push unregistered would mislead. It now refuses at once as before. Co-Authored-By: Claude Sonnet 5.5 --- scripts/pr_review.py | 16 +++++++++++----- tests/test_pr_review.py | 15 ++++++++++++++- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 1d2f84f3f..6b5a30f8d 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -962,7 +962,7 @@ def strip_fences( """ Q_ATTEST_BRANCH = """ query($o:String!,$r:String!,$n:Int!){ - repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefName headRepository{ url } } }} + repository(owner:$o,name:$r){ pullRequest(number:$n){ state headRefName headRepository{ url } } }} """ ATTESTATION = re.compile( r"^$", @@ -3674,9 +3674,9 @@ def held_checks_open( reads UNKNOWN on it for good. A BLOCKED merge also holds it, where the rollup was read whole, while a context the base - branch's rules require is absent from it by exact name, which `required` carries. The rollup only knows the checks that have - registered, so a push whose other workflows have not started reads five of five passed and - BLOCKED. An empty `required` is a rules read that found nothing or did not happen, and it + branch's rules require is absent from it by exact name, which `required` carries. The rollup + only knows the checks that have registered, so a push whose other workflows have not started + reads five of five passed and BLOCKED. An empty `required` is a rules read that found nothing or did not happen, and it holds nothing. """ if pr.get("state", "OPEN") != "OPEN": @@ -5009,7 +5009,13 @@ def await_registered_head( """ meta = gql(Q_ATTEST_BRANCH, owner, repo, num) or {} ref, source = meta.get("headRefName") or "", (meta.get("headRepository") or {}).get("url") or "" - if not ref or not source or UNSAFE_REF.search(ref) or DOT_SEGMENT.search(ref): + if ( + meta.get("state") != "OPEN" + or not ref + or not source + or UNSAFE_REF.search(ref) + or DOT_SEGMENT.search(ref) + ): return head, True tip = _git(checkout, "ls-remote", source, f"refs/heads/{ref}") if tip.returncode != 0 or tip.stdout.split()[:1] != [local]: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index f0ac1bca4..f47be6194 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -10365,7 +10365,11 @@ def lagging(self, tip: str, heads: list[str]) -> int: def gql(query: str, *_: object) -> dict: if query == pr_review.Q_ATTEST_BRANCH: - return {"headRefName": "feature", "headRepository": {"url": "https://x.test/o/r"}} + return { + "state": "OPEN", + "headRefName": "feature", + "headRepository": {"url": "https://x.test/o/r"}, + } return {**target, "headRefOid": next(replies, heads[-1])} self.slept: list[float] = [] @@ -10405,6 +10409,15 @@ def test_a_head_that_never_catches_up_is_refused_after_the_bound(self) -> None: self.assertIn("had not registered it yet", self.out.getvalue()) self.assertEqual([], self.posted) + def test_a_closed_pull_request_is_refused_without_waiting(self) -> None: + old = "d" * 40 + replies = {"state": "MERGED", "headRefName": "feature", "headRepository": {"url": "u"}} + with mock.patch.object(pr_review, "gql", return_value=replies): + head, registered = pr_review.await_registered_head( + "o", "r", 7, self.dir, self.head, old + ) + self.assertEqual((old, True), (head, registered)) + def test_a_head_that_moves_to_a_third_commit_is_not_called_unregistered(self) -> None: old, other = "d" * 40, "c" * 40 self.assertEqual(67, self.lagging(self.head, [old, other])) From dcdfaf25915137590a1bf1363cd5e25bd7e159b2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 11 Oct 2026 12:55:23 -0700 Subject: [PATCH 4/4] Encode the Base Branch Name in the Rules Read A base such as release/1.0 now travels as one explicit path segment, so the rules read cannot depend on how the API splits a raw slash. Co-Authored-By: Claude Sonnet 5.5 --- scripts/pr_review.py | 3 ++- tests/test_pr_review.py | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 6b5a30f8d..49155575a 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -341,6 +341,7 @@ import tarfile import time import unicodedata +import urllib.parse from collections.abc import Callable, Iterable, Iterator from datetime import UTC, datetime from pathlib import Path @@ -3726,7 +3727,7 @@ def required_contexts(owner: str, repo: str, base: str) -> frozenset[str] | None safe = base and not UNSAFE_REF.search(base) and not DOT_SEGMENT.search(base) read = ( reread_json( - f"repos/{owner}/{repo}/rules/branches/{base}?per_page=100", + f"repos/{owner}/{repo}/rules/branches/{urllib.parse.quote(base, safe='')}?per_page=100", '{n: length, c: [.[] | select(.type=="required_status_checks")' " | .parameters.required_status_checks[].context]}", ) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index f47be6194..da7af79df 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -10238,6 +10238,12 @@ def read(self, out: str, code: int = 0) -> frozenset[str] | None: def test_the_contexts_are_read_from_the_rules(self) -> None: self.assertEqual(frozenset({"a", "b"}), self.read('{"n": 3, "c": ["a", "b"]}')) + def test_a_slash_in_the_base_is_one_path_segment(self) -> None: + proc = subprocess.CompletedProcess([], 0, '{"n": 1, "c": []}', "") + with mock.patch.object(pr_review, "gh_rest", return_value=proc) as rest: + pr_review.required_contexts("o", "r", "release/1.0") + self.assertIn("/rules/branches/release%2F1.0?", rest.call_args.args[0]) + def test_a_read_that_did_not_happen_is_none_not_empty(self) -> None: for out, code in (("", 1), ("not json", 0), ('{"n": 1}', 0), ('{"n": 100, "c": []}', 0)): with self.subTest(out=out, code=code):