From 2377e5234e49f35f370b349d75ecd12cc5b0be7a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:05:08 -0700 Subject: [PATCH 1/5] Exit 45 When a Held Wait Releases a Head Whose Required Checks Never Ran (#3483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary A held `wait` exited `0` once nothing held the poll, even where the head's required checks never ran: a `DIRTY` merge, a `BEHIND` merge with a required context unregistered, and a payload whose rollup commit is not the head. The maintainer decided on #3471 that these paths get a new exit code naming that state. - `wait` now exits `45` with `status=CHECKS_MISSING` on an open, attested held head the poll released, under a merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`, where the payload carries no commit matching the head or no check has posted. - It also exits `45` where the merge reads `DIRTY` or `BEHIND` and a context the base branch's rules require is missing from a rollup read whole, or came from another app. - A `DRAFT` merge, unread rules, and a `CHECKS TRUNCATED` rollup stay at exit `0`, and a pull request no longer open never exits `45`. The help and README name each exception. - Each cause in the status line carries its own remedy. - `pr_review_checks.py` gains `checks_never_ran`, `required_unregistered`, which replaces `required_unmet` and now guards the `BLOCKED` hold and `_unmet_note` alike, and `MERGE_UNGATED`. `required_for` takes the merge states it reads rules for, and reads none for a pull request no longer open. - The `wait` help, `scripts/README.md`'s exit table, and its held-wait paragraph are rewritten together. Part of #3471, which the promotion pull request closes. ## Local pass findings Every `introduced` finding the local passes raised is fixed on this branch, the earlier list in this body included. These were declined as `style` on text this change wrote, a preference between defensible forms per fleet-code-review's rule to omit preferences: - The exit `45` decision living partly in `required_for`'s `gated` argument rather than wholly in `checks_never_ran`. Keeping it in the caller is what limits the rules read to `DIRTY` and `BEHIND`. - `gated` naming the merges whose rules are read, where the prose calls any merge outside `MERGE_UNGATED` gated. - The rules read running before the unreadable or empty-rollup cause decides the outcome. The read is memoized, and its note is accurate there. - The help's exit-0 sentence on where a required check can stay unregistered reading as wider than the attested held head. Its "then" ties it to the attested clause before it. - The unreadable-payload string running past 100 characters. No gate enforces the width on a string literal. ## Test plan - `python -m unittest test_pr_review test_pr_review_size test_pr_review_copilot` from `tests/`: 534 tests pass. - Mutations removing the open-state guard, or reading the rules ahead of the attestation guard, each fail a test. - The pre-commit gates (ruff, mypy, prose, eol) pass, and `prose_lint.py --check sentence-length` is clean over the diff. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 5 +- scripts/pr_review.py | 47 +++++++--- scripts/pr_review_checks.py | 44 ++++++--- tests/test_pr_review.py | 179 ++++++++++++++++++++++++++++++++++-- 4 files changed, 241 insertions(+), 34 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 850f08f47..3060ebf9c 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 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. | @@ -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`. 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 1fa205590..06cba3a3f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -116,13 +116,13 @@ 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, 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 where the + rules were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. A pull + 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. @@ -140,6 +140,15 @@ 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 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. + `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. 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 @@ -181,6 +190,7 @@ _unmet_note, check_lines, check_nodes, + checks_never_ran, checks_settling, checks_stuck, checks_tally, @@ -664,9 +674,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("state", "OPEN") != "OPEN" or pr.get("mergeStateStatus") not in gated: return frozenset() return required_contexts(owner, repo, pr.get("baseRefName") or "") or frozenset() @@ -707,7 +719,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) @@ -2013,7 +2025,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 @@ -2030,6 +2042,17 @@ def held_read() -> dict: "the merge, because BLOCKED is also worn by a thread or a missing approval" ) return 44 + 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')}, so it is " + "not ready to merge" + ) + 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 bd044afd2..94f602a06 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,23 +285,39 @@ 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): 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 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. + """ + 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 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" + 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]: """The head's workflow runs whose suite has not completed, each named `run `, sorted. @@ -322,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( { @@ -337,9 +359,9 @@ def required_unmet(required: frozenset[tuple[str, int | None]], nodes: list[dict ) -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 a31a9cdb9..b18bf0248 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -3571,20 +3571,28 @@ 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. + + Its release names that state 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) 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_MISSING", 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.""" @@ -3664,6 +3672,116 @@ 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. + + 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_MISSING", 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.""" + 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") + ] + 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.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_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) + 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") + 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 +4050,18 @@ 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. + + 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_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: running = check(status="IN_PROGRESS", conclusion="", started=real_ago(60)) @@ -6072,6 +6195,32 @@ 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_an_open_gated_merge(self) -> None: + pr = payload([], merge="DIRTY", checks=[]) + 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, 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, [], frozenset())) + other = payload([], merge="BLOCKED", checks=[check()], rollup_oid="f" * 40) + 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.""" + want = frozenset({("Later", None)}) + pr = payload([], merge="BEHIND", checks=[check()]) + nodes = pr_review_checks.check_nodes(pr) + 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.required_unregistered(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)}) @@ -6124,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: @@ -6136,6 +6287,16 @@ 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)) + closed = {**payload([], merge="DIRTY", checks=[check()]), "state": "CLOSED"} + self.assertEqual(frozenset(), pr_review.required_for(closed, "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 41ed0d7f7aa6ad1e8cbae0b5df130179d21d9016 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 15:28:34 -0700 Subject: [PATCH 2/5] List a Pull Request No Longer Open Among Exit 0's Unregistered Cases (#3485) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The promotion review of #3484 found the `wait` help and `scripts/README.md` saying exit `0` on an attested held head can leave a required check unregistered only under unread rules, a cut rollup, or a `DRAFT` merge. A pull request no longer open can too, so the list now names it in both places, replacing the separate sentence that said such a pull request never exits `45`. Behavior is unchanged. Part of #3471, which the promotion pull request closes. ## Pre-existing findings, not filed - `scripts/README.md`, the held-wait paragraph (`pre-existing`, `style`): "a cut rollup" where the help says `CHECKS TRUNCATED`, and "then" following the `DIRTY` and `BEHIND` sentence, both wording the earlier text already carried. ## Test plan - `prose_lint.py --check sentence-length` is clean over the diff, and the pre-commit gates pass. - Two local passes checked the list against `checks_never_ran`, `required_unregistered`, `required_for`, and the end of `wait`, and found it complete. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 3060ebf9c..84baa4fdf 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 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 `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 `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 06cba3a3f..f22b3fb02 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -120,9 +120,8 @@ 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 where the - rules were unread, the rollup is `CHECKS TRUNCATED`, or the merge reads DRAFT. A pull - request no longer open never exits 45. + 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. It is also a round past --min-rounds not yet landed. From 6a1990f89690b8ca6b551ab4ead0dc1b4365b843 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 16:07:29 -0700 Subject: [PATCH 3/5] Say a Missing Required Check May Sit in a Partly Unread Rollup (#3486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The promotion review of #3484 found exit `45` saying a required check had not registered while the head's rollup holds a type the reader drops, which the digest names `CHECKS PARTIALLY UNREAD`. The required check could sit there. - Where the rollup is partly unread, the `CHECKS_MISSING` clause names "sits in the rollup's unread part" as one more possible cause, and its remedy leads with reading the head commit's checks and statuses. The other causes and remedies are kept. - The `BLOCKED` hold's `CHECKS_PENDING` note says an unmet required check may sit in the unread part, on the same condition. - The `wait` help and `scripts/README.md` say so for both, and the help's hold precondition names the head's own rollup read and not `CHECKS TRUNCATED`. - Exit codes are unchanged. GitHub's rollup union holds only `CheckRun` and `StatusContext` today, so this is reachable only after an API change. Part of #3471, which the promotion pull request closes. ## Local pass findings Every `introduced` finding the local passes raised is fixed on this branch, the earlier list in this body included. Pre-existing findings, filed: - `scripts/pr_review_checks.py`, the empty-rollup checks (`pre-existing`): a rollup of only unreadable nodes is not read as empty. Filed as #3488. Declined as `style` on text this change wrote, a preference between defensible forms per fleet-code-review's rule to omit preferences: - `_unmet_note` with several unmet contexts reads "A, B, which may sit in the rollup's unread part", where "which" can attach to the last name alone. - The help, README, and digest each name the unreadable type in slightly different words. ## Test plan - `python -m unittest test_pr_review test_pr_review_size test_pr_review_copilot` from `tests/`: 534 tests pass, with positive and negative cases for each conditional clause and remedy. - The pre-commit gates pass, and `prose_lint.py --check sentence-length` is clean over the diff. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 12 ++++++++---- scripts/pr_review_checks.py | 16 +++++++++++----- tests/test_pr_review.py | 21 +++++++++++++++++++++ 4 files changed, 41 insertions(+), 10 deletions(-) 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)) From 578a265b756c8e8692b280c526b9f677fc77926c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 16:32:39 -0700 Subject: [PATCH 4/5] Scope the No-Check-Posted Cause to DIRTY and Give It a Remedy (#3490) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The third promotion review of #3484 raised three findings, each fixed here: - The `wait` help's exit 0 paragraph and `45` entry, and the README's `45` row, offered "no check posted" for any merge other than `CLEAN`, `UNSTABLE`, or `HAS_HOOKS`. The empty-rollup hold keeps `BLOCKED`, `BEHIND`, and `DRAFT` at `30`, so each now scopes that cause to `DIRTY`. - The no-check-posted cause named no remedy. It now says to resolve the head's conflict with its base, and a test pins it. - The read-shape test table asserted `assertIn("", ...)` on its exit 0 rows, which always passes. It now checks a reason only where one is expected, and those rows stay covered by their `CHECKS_MISSING` absence check. Behavior and exit codes are unchanged. Part of #3471, which the promotion pull request closes. ## Test plan - `python -m unittest test_pr_review test_pr_review_size test_pr_review_copilot` from `tests/`: 534 tests pass. - The pre-commit gates pass, and `prose_lint.py --check sentence-length` is clean over the diff. - The recorded local pass traced the cause's reach to `DIRTY` alone and found no finding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 19 ++++++++++--------- scripts/pr_review_checks.py | 5 ++++- tests/test_pr_review.py | 8 ++++++-- 4 files changed, 21 insertions(+), 13 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..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 @@ -143,11 +144,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..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.""" @@ -3742,7 +3745,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 f87bf625df9cf6fa0bcfb7ad93633fe4c66e7e7e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 16:57:43 -0700 Subject: [PATCH 5/5] Restore the Caveat That a Review-Present Exit 0 Does Not Poll the Checks (#3491) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The fourth promotion review of #3484 found the rewrite of the `wait` help's exit 0 text dropping its caveat that exit 0 does not prove the required checks ran. - The help now says, right after its opening exit 0 line, that a review-present exit 0 does not poll the checks, so it does not prove the required checks ran. The README's exit 0 row says the same. - The help's claim that every workflow run on the head has completed is scoped to an open attested held head, the one case that polls its runs. Behavior is unchanged. Part of #3471, which the promotion pull request closes. ## Pre-existing findings, not filed - `scripts/pr_review.py` help, the exit 0 "only under" sentence (`pre-existing`): its subject is "Exit 0", tied to the attested held head only by "then", so read alone it reads as covering a review-present exit 0 too. ## Dropped findings - `scripts/README.md`, the exit 0 row (`style`): "on that review" reaches back past the held-head clause. - `scripts/pr_review.py` help (`style`): "held head whose commit the payload carries" reads the commit as separate from the head. ## Test plan - `prose_lint.py --check sentence-length` is clean over the diff, and the pre-commit gates pass. - The recorded local pass checked each changed sentence against the end of `wait` and found it true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index eeae0d7a2..6b54bfeb2 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -199,7 +199,7 @@ 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, short of `45`. | +| `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`. Exit `0` on that review does not poll the checks, so it does not prove the required checks ran. | | `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. | diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 5a47703ac..d37e2249c 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -114,8 +114,9 @@ carries any Copilot activity. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. 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 + hold the poll. Exit 0 on a review present does not poll the checks, so it does not prove + the required checks ran. On an open attested held head whose 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, an open attested held head nothing holds exits 45 where its rollup was unreadable or, under