diff --git a/scripts/README.md b/scripts/README.md index 84baa4fdf..732392dbb 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -215,7 +215,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj `1` is a GraphQL read that failed and ended the run, and `2` is argparse's own usage error. -`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. +`wait` exits `30` with `status=CHECKS_PENDING` on an attested held head while its checks can still move the merge at the timeout. It reads them in this order. A pull request no longer open holds nothing, and a merge reading `UNKNOWN` holds. A `BLOCKED` merge ends the poll on the first read where a required check is stuck in a shape waiting does not clear. A check running long or gated is not such a shape. A failed one is the exception while a newer run of its workflow on the head has not finished, which the digest names `CHECK SUPERSEDED`. Next, a workflow run on the head that has not completed holds, where the read reaches it. The read reaches the head's last 50 check suites and those of its first `CHECKS_WINDOW` rollup contexts. A run's later jobs enter the rollup only as their `needs:` finish, so a green rollup proves nothing while one is open. The status line names those runs. Next, a merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll. Otherwise a required check still settling holds, one running long included, and so does any settling check while no required one has posted. A `BLOCKED` merge also holds while a check the base branch's rules require has not registered. That hold needs the base branch's rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, its status line says that check may sit in the unread part. An empty rollup holds under `BLOCKED`, `BEHIND`, or `DRAFT`. An open head released short of those holds exits `45` with `status=CHECKS_MISSING` where a check it is owed is missing from the read. Each such case falls under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. One is a payload with no commit matching the head, which the digest names `CHECKS UNREADABLE` and which reads no run and no check. Another is a rollup with no check posted, which only a `DIRTY` merge releases, since a conflicted pull request runs no `pull_request` workflow. The last is a `DIRTY` or `BEHIND` merge whose rollup lacks a context the base branch's rules require, or carries it from another app. That one needs the rules read and a rollup that is not `CHECKS TRUNCATED`. Where the rollup holds a type the reader cannot parse, which the digest names `CHECKS PARTIALLY UNREAD`, the status says the check may sit there. Exit `0` can then leave a required check unregistered only under unread rules, a cut rollup, a `DRAFT` merge, or a pull request not open. `--min-rounds N` counts what `status` prints as `rounds=`, every round on every head, refusals included, so N is the `rounds=` a `status` read before a re-request. `wait` exits `40` when no Copilot review covers the head and the reviewer has said why in another shape. A plain comment newer than its newest review carries no commit, so it satisfies no coverage check whatever it says, and its body prints whole because its wording is the only thing separating a refusal from a remark. A comment older than the newest review is spent. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, so `window_blind` guards both sides: blind on comments reads as `answered_outside_review=unknown`, and blind on reviews reports nothing and lets the wait keep polling. A request that returns success and adds neither a pending reviewer nor a review-request event also exits `40`, as `status=REQUEST_NOT_RECORDED`, as does a reviewer writing under a renamed login, `status=REVIEWER_LOGIN_DRIFT`. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index f22b3fb02..adad84f4f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -124,7 +124,9 @@ rules, a `CHECKS TRUNCATED` rollup, a DRAFT merge, or a pull request not open. 30 = still pending at the timeout. On a held head it is any hold `status=CHECKS_PENDING` names, a required check still settling and a workflow run still - open among them. It is also a round past --min-rounds not yet landed. + open among them. Where the rollup holds a type this cannot read, a BLOCKED hold's note + says an unmet required check may sit in its unread part. It is also a round past + --min-rounds not yet landed. 40 = no Copilot review covers the head: the reviewer answered outside a formal review, the request returned success and recorded nothing on the pull request, or the reviewer writes under a login no query filters on. 41 = the @@ -143,11 +145,13 @@ UNSTABLE, or HAS_HOOKS. Either the payload carries no commit matching the head, named `CHECKS UNREADABLE` in the digest, or no check has posted. Or the merge reads DIRTY or BEHIND and a check the base branch's rules require has not registered, or came from - another app. That last needs the rules read and a rollup read whole. + another app. That last needs the rules read and a rollup not `CHECKS TRUNCATED`. Where + the rollup holds a type this cannot read, named `CHECKS PARTIALLY UNREAD` in the + digest, the status says the check may sit there. `status=CHECKS_MISSING` names which. A DIRTY merge runs no pull_request workflow until its conflict is resolved. A required check not yet registered holds a BLOCKED merge to - 30 instead, where the rules were read and the rollup read whole. An empty rollup read - off the head holds a BLOCKED, BEHIND, or DRAFT one. + 30 instead, where the rules and the head's rollup were read, not `CHECKS TRUNCATED`. An + empty rollup read off the head holds a BLOCKED, BEHIND, or DRAFT one. 49 = no Copilot round covers this head and none was requested, the pull request merging into a branch other than the default with no attestation vouching for the head. 64 = the write scope could not diff --git a/scripts/pr_review_checks.py b/scripts/pr_review_checks.py index 94f602a06..5c8d8643f 100644 --- a/scripts/pr_review_checks.py +++ b/scripts/pr_review_checks.py @@ -303,7 +303,8 @@ def checks_never_ran( """Why an open head's gated merge lacks a check it is owed, with its remedy, or "" for none. A CLEAN, UNSTABLE, or HAS_HOOKS merge is owed nothing. The caller passes `required` only for a - merge whose release judges registration, and a context is judged only on a rollup read whole. + merge whose release judges registration, and a context is judged only on the head's own + rollup, not cut. """ if pr.get("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") in MERGE_UNGATED: return "" @@ -312,9 +313,13 @@ def checks_never_ran( if not nodes: return "no check has posted, and a DIRTY merge runs no pull_request workflow until resolved" unmet = ", ".join(required_unregistered(pr, nodes, required)) + unread = checks_unread(nodes) + where = ", sits in the rollup's unread part," if unread else "" + read = "read the head commit's checks and statuses first, then " if unread else "" return unmet and ( - f"a required check has not registered or came from another app, {unmet}, so update a " - "BEHIND head, resolve a DIRTY one's conflict, or correct the check's poster or its rule" + f"a required check has not registered{where} or came from another app, {unmet}, so " + f"{read}update a BEHIND head, resolve a DIRTY one's conflict, or correct the check's " + "poster or its rule" ) @@ -343,7 +348,7 @@ def runs_open(pr: dict) -> list[str]: def required_unregistered( pr: dict, nodes: list[dict], required: frozenset[tuple[str, int | None]] ) -> list[str]: - """The contexts among `required` no rollup node satisfies, sorted, on a rollup read whole.""" + """The `required` contexts no node satisfies, sorted, on the head's own rollup, not cut.""" if checks_unreadable(pr) or checks_truncated(pr): return [] seen = [n for n in nodes if not n.get("unreadable")] @@ -362,7 +367,8 @@ def required_unregistered( def _unmet_note(pr: dict, required: frozenset[tuple[str, int | None]], nodes: list[dict]) -> str: """Names the required contexts still unmet, so a reader sees what the hold waits on.""" unmet = required_unregistered(pr, nodes, required) - return f" (required, unmet: {', '.join(unmet)})" if unmet else "" + where = ", which may sit in the rollup's unread part" if checks_unread(nodes) else "" + return f" (required, unmet: {', '.join(unmet)}{where})" if unmet else "" def checks_truncated(pr: dict) -> bool: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index b18bf0248..aec58f356 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3709,6 +3709,18 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: "came from another app, " + name, ), ("unread payload", unread, 45, "no commit matching the head"), + ( + "partly unread rollup", + payload( + [review(oid=OLD)], + merge="DIRTY", + checks=[check(name="lint"), {"__typename": "SomethingNew", "label": "x"}], + ), + 45, + "has not registered, sits in the rollup's unread part, or came from another app, " + + name + + ", so read the head commit's checks and statuses first, then update a BEHIND head", + ), ("closed", {**closed, "state": "CLOSED"}, 0, ""), ] + [ (merge, payload([review(oid=OLD)], merge=merge, checks=[]), 0, "") @@ -3731,6 +3743,8 @@ def test_a_released_head_exits_by_what_its_read_shows(self) -> None: with mock.patch.object(pr_review.time, "sleep"): self.assertEqual(code, self.cli(["wait", "7", "--timeout", "1"])) self.assertIn(reason, self.out.getvalue()) + if label == "another app": + self.assertNotIn("checks and statuses first", self.out.getvalue()) if code == 0: self.assertNotIn("status=CHECKS_MISSING", self.out.getvalue()) @@ -6277,6 +6291,13 @@ def test_the_unmet_context_is_named(self) -> None: nodes = pr_review_checks.check_nodes(pr) self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", None)}), nodes)) self.assertIn("Gate", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes)) + note = pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes) + self.assertNotIn("unread part", note) + unread = nodes + pr_review_checks.check_nodes( + payload([], merge="BLOCKED", checks=[{"__typename": "SomethingNew", "label": "x"}]) + ) + note = pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), unread) + self.assertIn("Gate, which may sit in the rollup's unread part", note) rollup = pr["commits"]["nodes"][0]["commit"]["statusCheckRollup"] rollup["contexts"]["pageInfo"] = {"hasNextPage": True} self.assertEqual("", pr_review_checks._unmet_note(pr, frozenset({("Gate", 9)}), nodes))