Hold an Attested Wait Until the Base Branch's Required Checks Register

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.
 

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.
@@ -340,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
@@ -780,6 +782,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 +961,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){ state headRefName headRepository{ url } } }}
+"""
 ATTESTATION = re.compile(
     r"^<!-- fleet-local-review: head=([0-9a-f]{40}) base=(\S+) findings=(\d+|unknown) -->$",
     re.MULTILINE,
@@ -3646,7 +3653,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 +3673,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, 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.
     """
     if pr.get("state", "OPEN") != "OPEN":
         return False
@@ -3679,9 +3697,58 @@ 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 - {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.
+
+    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.
+    """
+    safe = base and not UNSAFE_REF.search(base) and not DOT_SEGMENT.search(base)
+    read = (
+        reread_json(
+            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]}",
+        )
+        if safe
+        else 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 +4925,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 +4998,40 @@ 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 (
+        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]:
+        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
+
+
 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 +5793,7 @@ def still_held(polled: dict) -> bool:
                     datetime.now(UTC),
                     a.check_grace,
                     a.check_stall,
+                    required_for(polled, owner, repo),
                 )
             )
 
@@ -5774,11 +5882,19 @@ 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_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.