Read why a merge is blocked instead of reporting one word

[ptr727]

mergeStateStatus reports BLOCKED for a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the digest printed that word and stopped there.

Found by running into it. A run polled BLOCKED for twenty-five minutes on a pull request whose only unfinished check was an aggregator job GitHub dispatched and never assigned a runner, and the cause came from the maintainer rather than from any field in the digest. That is the defect: the digest named a state it could not explain.

This is the refusal defect of #584 one gate along. There, a review that covered nothing rendered as coverage. Here, a check nothing is running renders as patience, and both read as a pull request worth waiting on.

What the digest says now

Reconstructed from the observed job states of the run this hit, rather than described:

repo=ptr727/ProjectTemplate pr=590 head=aaaaaaaa rounds=1 review_on_head=yes refusal=no
threads=0 unresolved=0 suppressed=0 (on_head=0 earlier=0) answered_outside_review=no
requested=no merge=BLOCKED checks=2/3 stuck=NOT_PICKED_UP
  CHECK NOT PICKED UP ('Check pull request workflow status job', queued 15m with no runner
  assigned): nothing here starts it, because the runner pool is GitHub-hosted, so re-run the
  workflow or wait on that capacity

The rollup rides the existing full query, so reading the checks costs no extra round-trip, which is the reason this script exists at all.

Three shapes, because each wants the opposite response

• NOT_PICKED_UP is a job GitHub dispatched and assigned no runner. Read from the queued state rather than from a runner name, which GraphQL does not carry, and the state suffices because a job held behind a needs: dependency does not enter the rollup until that dependency finishes, so there is no dependency-blocked queue to mistake for a starved one. Nothing agent-side starts it, since the pool is hosted, so the remedy is a re-run or that capacity, and not a re-request, a rebase, or an empty commit.
• RUNNING_LONG is deliberately the weaker reading and its wording says so, since duration alone cannot separate a hung job from a slow one. This repository's lint job legitimately runs nine to eleven minutes while its aggregator is a single shell conditional, so the threshold is generous, the elapsed time prints for the reader to judge against what the job costs, and nothing asserts a fault.
• FAILED is a verdict rather than a stuck check, reported so no reader deduces a red check from BLOCKED.

Exit 44, and the boundary that makes it mean something

wait gains exit 44 for a review loop that closed against a check in one of those shapes, because 0 was saying the loop closing is the merge gate.

A check merely still running normally is not 44 and exits 0. That boundary is the whole design: the wait returns the moment coverage lands, which on almost every pull request is mid-CI, so taking 42 for a pending check would make 42 the ordinary outcome, and a code that fires always carries nothing. The grace is the pickup grace's five minutes for the reason that one is, and the stall is thirty because a fleet repository building and testing .NET runs longer than this one. Both are flags, and a negative value for either is rejected by name rather than rendering a digest that reports every check stuck from its first read.

Two defects this caught in itself

• A finished check carrying no conclusion yet fell through to the unknown-conclusion branch and reported FAILED, inventing a red check out of a race in the API. Caught by the suite, which is also what said the first test helper was at fault rather than the code, since it failed eight cases at once.
• EXPECTED was missing from the unstarted set, so a required status nobody has posted reached that same branch and reported that same false failure. Caught reading the diff back.

Both have cases now. SKIPPED and NEUTRAL count as passes, since the fleet aggregator pattern skips the conditional jobs and four of the six checks on a green pull request here are skips. An unrecognized conclusion is reported, because a new enum member read as a pass is a red check rendering as a green digest.

Docs

The runbook's BLOCKED bullet said the cause is "usually" unresolved review threads, which is true and is exactly what misled the run. The qualifier stays and the counter-case joins it rather than replacing it. GOVERNANCE.md merge-gate condition 1 now requires the reason to be read rather than inferred, and a new paragraph states that BLOCKED is no more self-explaining than CLEAN is sufficient, including that hosted-runner capacity is not a reason to weaken a gate.

GOVERNANCE.md is verbatim-carried, so a fleet re-vendor is owed, and the TODO.md re-vendor entry now names these sections.

Verification

461 cases pass across the three scripts/ suites, 217 of them in test_pr_review.py, which is 41 more than develop carries. The count read 291 when this was written and the suites have grown on both sides since, so it is restated against the merge rather than left as a number measured on a tree that no longer exists. prose_lint.py --diff origin/develop, repo_gate.py, spec/validate.py, and markdownlint-cli2 over all 44 files are clean. A grep for restatements of the changed merge-gate wording across every markdown file found none stale, and the "all four preconditions" sentence still holds since condition 1 was amended rather than added to. The digest was also run live against a real pull request, where it reports checks=6/6 and no stuck field.

Renumbered from 42 to 44 on the forward-merge

This branch was parked on 2026-08-06 pending the GitHub Actions outage, and develop took exit 42 in the meantime for a round that read fewer files than the pull request changed, with 43 for a shape the reader does not know, both from #607, #608 and #613. The check reading here is 44, ranked under both: 43 says no field can be believed, 42 says part of the diff has no review, and only once those are clear is a wedged required check the thing worth reporting. wait returns report_verdict's code where it has one and reads the checks only where it does not.

[Copilot]

Pull request overview

Updates the scripts/pr_review.py digest and wait logic to explain why mergeStateStatus=BLOCKED occurs by reading the head commit’s check rollup, and documents the resulting behavior/exit codes across the runbook and governance guidance.

Changes:

• Extend pr_review.py’s GraphQL query to include statusCheckRollup for the head commit, summarize checks=N/M, and print per-check “stuck” explanations.
• Add check-shape classification (NOT_PICKED_UP, RUNNING_LONG, FAILED) and introduce wait exit code 42 when review coverage is present but checks are deemed non-mergeable.
• Update docs/governance/runbook and expand the test suite to cover rollup normalization and new digest/wait behaviors.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
TODO.md	Updates the fleet re-vendor entry to mention the new merge-blocked reasoning requirement.
scripts/test_pr_review.py	Adds fixtures/helpers and extensive new unit coverage for rollup normalization, check-shape classification, digest output, and wait exit 42 behavior.
scripts/README.md	Documents the new checks=/stuck= digest fields and the new wait exit code 42 semantics.
scripts/pr_review.py	Implements rollup fetching, check normalization/classification, digest enhancements, and wait exit 42 handling.
GOVERNANCE.md	Tightens merge gate #1 to require reading the reason for BLOCKED and documents why BLOCKED is not self-explaining.
.github/copilot-instructions.md	Updates the merge-state “gotchas” section to reflect the new digest behavior and exit 42 meaning.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

scripts/pr_review.py:313

• age() only catches ValueError, but datetime.fromisoformat() can return a naive datetime if the timestamp ever arrives without a timezone offset, and subtracting an aware now would raise TypeError (crashing digest/wait). Catch TypeError as well (or normalize naive timestamps to UTC) so unknown/odd stamp formats degrade to None as intended.

def age(stamp: str, now: datetime) -> float | None:
    """Seconds from `stamp` to `now`, or None where the stamp is absent or unparseable.

    None is returned rather than zero or a raise, because every caller treats an unknown age as
    "cannot judge this" and reports nothing. Zero would read as a check that just started, which
    is the reading that reports a job stuck for hours as fresh.
    """
    if not stamp:
        return None
    try:
        # `fromisoformat` reads the trailing Z only from 3.11, and a fleet machine may carry less.
        return (now - datetime.fromisoformat(stamp.replace('Z', '+00:00'))).total_seconds()
    except ValueError:
        return None

scripts/pr_review.py:327

• This always reads the first commit node’s rollup, but the surrounding comments/docs indicate the rollup should be taken from the commit whose oid == headRefOid (to avoid rendering a rollup from “a push ago”). Consider selecting the commit node by matching pr['headRefOid'] (and falling back safely if not found) so the code behavior matches the documented intent.

    commits = ((pr.get('commits') or {}).get('nodes') or [])
    rollup = ((commits[0].get('commit') or {}) if commits else {}).get('statusCheckRollup') or {}
    out = []

scripts/test_pr_review.py:820

• These wait-path tests intentionally use the real clock in pr_review.main(), but they feed it timestamps derived from the fixed NOW. That makes the test sensitive to wall-clock time (e.g., if the suite runs before NOW, the computed age is negative and the check won’t read as starved). To keep the tests stable long-term, generate started using the real current time (e.g., datetime.now(timezone.utc) - timedelta(...)) or patch pr_review.datetime.now() in the test so the “real clock” is controlled.

    def test_wait_exits_forty_two_where_the_review_closed_but_a_check_is_starved(self) -> None:
        """Exit 0 was saying the review loop closing is the merge gate, and it is not.

        The age is read against the real clock here rather than the suite's fixed NOW, so the
        timestamp is one whose age only grows: fifteen minutes at NOW and more on any later run,
        which is past the five-minute grace under every clock this ever runs on.
        """
        self.answer(payload([review()], merge='BLOCKED', checks=[
            check(name='gate', status='QUEUED', conclusion='', started=ago(900))]))

scripts/pr_review.py:629

• The code relies on check_grace and check_stall being ordered (and a test asserts the constants are), but a user can currently pass --check-stall smaller than --check-grace, which would invert the intended meanings (report “running long” earlier than “not picked up”). Consider validating a.check_grace < a.check_stall for CLI inputs and rejecting inversions with a clear argparse error.

    for name in ('check_grace', 'check_stall'):
        if getattr(a, name) < 0:
            ap.error(f'--{name.replace("_", "-")} cannot be negative')

[ptr727]

Round 2 raised four findings as low-confidence, collapsed in the review body with no inline threads, so they are answered here with each quoted. All four are real and all four are fixed in 565055b. One of them was a false clean in this very change.

1. check_nodes read commits[0] rather than the commit matching headRefOid.

This always reads the first commit node's rollup, but the surrounding comments/docs indicate the rollup should be taken from the commit whose oid == headRefOid (to avoid rendering a rollup from "a push ago").

The sharpest of the four, and it caught a defect in the test as much as in the code. The case meant to hold this asserted that the fixture's commit equalled the head, which tests the payload rather than the code, so it passed while the code read position. Position is not identity, and trusting it is the same shape as counting a review without reading it, which is the defect the rest of this script exists to prevent.

Selection is now by oid. A head matching no commit reports nothing rather than falling back to another node, because a fallback is that same stale read reached by a different route. The absence is not left silent either: checks=0/0 reads as a fact about the head, so a CHECKS UNREADABLE line says it is this reading having failed. A payload with no commits at all is not a failed match and does not say it. Two cases now assert the selection by giving the connection a rollup on a non-head commit and by putting a decoy node ahead of the head's.

2. age() caught ValueError but not TypeError.

datetime.fromisoformat() can return a naive datetime if the timestamp ever arrives without a timezone offset, and subtracting an aware now would raise TypeError (crashing digest/wait).

Correct, and the reason it matters is the direction of the failure. A zone-less stamp parses, so it never reaches the existing guard, and the TypeError escapes a function whose documented contract is to degrade to None on an odd stamp. A crash is the worst possible outcome for a script whose entire job is to report what the state is. Both exceptions are caught now, returning None rather than normalizing, since assuming UTC would invent a timezone the stamp did not carry. The zone-less case is listed in the unreadable-stamp case explicitly, because it is the one that does not look unparseable.

3. The wait-path cases measured an age from the suite's fixed NOW against the real clock.

That makes the test sensitive to wall-clock time (e.g., if the suite runs before NOW, the computed age is negative and the check won't read as starved).

Right, and my docstring's "the age only grows" was the assumption talking rather than a property. Those cases now take real_ago, derived from the same clock main reads, and the fixed-NOW helper documents that a wait case cannot use it. One case that had reached for a huge --check-grace to dodge this no longer needs the flag at all.

4. --check-stall could be passed smaller than --check-grace.

The code relies on check_grace and check_stall being ordered (and a test asserts the constants are), but a user can currently pass --check-stall smaller than --check-grace.

A case asserted the two constants were ordered while the flags stayed free to invert them, which is the gap between a rule and its check one level down, and it is a shape this repository has been bitten by before. The parser rejects the inversion by name, equal values included, since equal thresholds collapse the two readings into one.

296 cases pass across the three scripts/ suites, and prose_lint, repo_gate, spec/validate, and markdownlint-cli2 are clean.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

scripts/pr_review.py:570

• The NOT_PICKED_UP message hard-codes a “no runner assigned / GitHub-hosted capacity” remedy, but NOT_PICKED_UP is also returned for StatusContext state EXPECTED (meaning “required status not posted yet”). For EXPECTED, this wording is inaccurate and can send readers toward the wrong remediation. Suggestion: preserve the original rollup node kind (e.g., add kind / typename in check_nodes) and either (a) split EXPECTED into a separate shape (e.g. NOT_POSTED) with its own message, or (b) vary the NOT_PICKED_UP wording based on the underlying node type/state.

    for node, shape in stuck:
        # Each shape carries its own remedy, which is the whole point of telling them apart.
        # A reader handed one word for all three retries the wrong thing, or waits on a queue.
        elapsed = age(node.get('started') or '', now)
        mins = 'age unknown' if elapsed is None else f'{int(elapsed // 60)}m'
        if shape == 'NOT_PICKED_UP':
            lines.append(f'  CHECK NOT PICKED UP ({node["name"]!r}, queued {mins} with no '
                         'runner assigned): nothing here starts it, because the runner pool is '
                         'GitHub-hosted, so re-run the workflow or wait on that capacity')

scripts/pr_review.py:21

• The exit-code comment says 42 includes a required check that is “failed”, but the implemented condition later is stuck and mergeStateStatus == 'BLOCKED'. If failed required checks can yield a different mergeStateStatus (commonly DIRTY/UNSTABLE in GitHub’s enum), 42 may not fire even though the digest can report stuck=FAILED. Please either broaden the condition to match the documented behavior or narrow the documentation here (and in scripts/README.md) to the statuses the code actually treats as 42.

           42 = the review loop closed but a required check is in a shape no wait clears:
           queued with nothing acting on it, running far past what the job costs, or
           failed. A check merely still running normally is not this, and exits 0.

scripts/pr_review.py:356

• The normalization treats every non-CheckRun context as a StatusContext. That works for today’s query, but it will silently mis-normalize (and potentially mis-report) if GitHub adds a new union member under statusCheckRollup.contexts.nodes or if the query evolves. Suggestion (mandatory for robustness): make the branch explicit (elif __typename == 'StatusContext') and ignore/skip unknown types (optionally recording them for debugging) rather than forcing them into the StatusContext shape.

    for n in ((rollup.get('contexts') or {}).get('nodes') or []):
        if n.get('__typename') == 'CheckRun':
            out.append({'name': n.get('name') or '', 'state': n.get('status') or '',
                        'conclusion': n.get('conclusion') or '',
                        'started': n.get('startedAt') or ''})
        else:
            # A StatusContext reports one field for both, so its state doubles as its conclusion.
            # Its PENDING means the posting system reported the run as under way.
            # A CheckRun's PENDING means the opposite, dispatched and not begun.
            # So the same string is two states, and the shape is knowable only here.
            # Left alone, a long external build reports as queued with no runner assigned.
            # That names a cause the run does not have, on a system that did pick it up.
            state = n.get('state') or ''
            out.append({'name': n.get('context') or '',
                        'state': 'IN_PROGRESS' if state == 'PENDING' else state,
                        'conclusion': state, 'started': n.get('createdAt') or ''})

TODO.md:35

• This line’s indentation changed from an indented sub-bullet ( |- ...) to a top-level bullet (|- ...). If this item is meant to remain nested under a parent list item, Markdown rendering will change (and may break numbering/structure). Suggestion: restore the previous indentation level if nesting was intentional.

- Re-vendor the changed `verbatim` content across the fleet, which is one sweep covering five files. `repo-config/configure.sh` is carried `verbatim` with `appliesTo: "*"` and the hub swept it to one sentence per line. In `AGENTS.md`, "Context and Delegation Discipline" carries the wait rule's failure clause and "Where the Rules Live" carries a row for the new section named below. In `GOVERNANCE.md`, "Verification Discipline" carries the rule that a launched process is not a result, "PR Review Etiquette" carries the five outcomes that close a finding and now also a merge-gate condition 1 that reads why a merge is blocked rather than inferring it, plus the paragraph saying `BLOCKED` is no more self-explaining than `CLEAN` is sufficient, "Repository Boundaries and Write Safety" carries the rule that a refused write is reported rather than re-shaped, and "Representative Data in Agent-Authored Text" is an entirely new carried section that no downstream repo holds, which the audit reports as a missing section rather than as drift. Three further `GOVERNANCE.md` sections differ by a single word each, "Documentation Style Conventions", "Communicating with the User" and "Repository Details", where the format's name was capitalized to the convention `CODESTYLE.md` "Markdown and Spelling" now states, so they are byte-mismatched for a reason a reader of the diff would otherwise call cosmetic. Two comment lines in `.markdownlint-cli2.jsonc` took the same capitalization, and that file is `verbatim` and `whole`, so every downstream copy is byte-mismatched on a config nothing else changed about. `CODESTYLE.md` carries the new item and is the fifth file, at `intent` rather than `verbatim` fidelity, so it reaches the fleet as a rule each repo adopts in its own copy rather than as bytes to match, and the same mixed spelling is waiting in every downstream tree. Every repo already holding a copy of a changed section is byte-mismatched against the hub until it takes the new one, which the audit reports as stale rather than modified. This sweep is also the follow-through [#489][issue-489] and [#379][issue-379] were waiting on, and the `.editorconfig` line in [#353][issue-353] rides the same visit to each repo. Regenerate [reports/divergences.md][divergences-report] before using it as the work list, since the committed copy is dated 2026-07-22 and therefore predates the router split, which shows in it naming "Repository Boundaries and Write Safety", "Git and Commit Rules" and "Verification Discipline" as `AGENTS.md` sections when all three now live in `GOVERNANCE.md`. A stale ledger is the same hazard as a stale exemption, in that it hands out a work list measured against a tree that no longer exists.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (5)

scripts/pr_review.py:570

• The NOT_PICKED_UP message always says "no runner assigned", but NOT_STARTED also includes StatusContext EXPECTED (an unposted required status), which is not a runner-capacity issue. This can produce a misleading remedy for EXPECTED contexts.

        mins = 'age unknown' if elapsed is None else f'{int(elapsed // 60)}m'
        if shape == 'NOT_PICKED_UP':
            lines.append(f'  CHECK NOT PICKED UP ({node["name"]!r}, queued {mins} with no '
                         'runner assigned): nothing here starts it, because the runner pool is '
                         'GitHub-hosted, so re-run the workflow or wait on that capacity')

scripts/pr_review.py:735

• This exit-42 path and message claim a "required check" is stuck, but the code only checks mergeStateStatus == "BLOCKED" plus presence of any stuck rollup context. Since BLOCKED can also be caused by approvals/threads, and the rollup may include non-required checks, this can mislabel the merge blocker.

        if stuck and final.get('mergeStateStatus') == 'BLOCKED':
            print('status=CHECKS_NOT_MERGEABLE the review loop is closed, and a required check '
                  'is in a shape waiting does not clear: read the block above, since a starved '
                  'check wants a re-run, a long one a judgment, and a failed one a fix')
            return 42

scripts/test_pr_review.py:728

• check(status='PENDING') is not a valid GraphQL CheckRun.status value (PENDING is a StatusContext state). To keep this test representative, use CheckRun QUEUED here to model "dispatched but not begun" vs StatusContext PENDING (running).

        # The CheckRun spelling keeps the opposite reading, which is what makes the shape decide.
        self.assertEqual('NOT_PICKED_UP',
                         self.shape(check(status='PENDING', conclusion='', started=ago(900))))

scripts/test_pr_review.py:633

• These fixtures exercise CheckRun.status values (WAITING/PENDING/REQUESTED) that GitHub GraphQL does not return for CheckRunStatusState (it is QUEUED/IN_PROGRESS/COMPLETED). Using impossible states makes the test less representative of the live payload.

        for state in ('QUEUED', 'WAITING', 'PENDING', 'REQUESTED'):
            with self.subTest(state=state):
                self.assertEqual('NOT_PICKED_UP', self.shape(
                    check(status=state, conclusion='', started=ago(900))))

scripts/pr_review.py:42

• NOT_STARTED lists states (WAITING/PENDING/REQUESTED) that are not returned by the GraphQL enums this script queries (CheckRun.status is QUEUED/IN_PROGRESS/COMPLETED; StatusContext.state uses EXPECTED/PENDING/etc). Keeping impossible values here (and in tests) reduces readability and makes fixtures less representative.

# A check that has not started, in every spelling the two rollup enums carry between them.
# QUEUED is the one a starved job wears, and WAITING, PENDING and REQUESTED cover a gate.
# Those three are a CheckRun's, where PENDING means dispatched and not begun.
# EXPECTED is a StatusContext's, meaning a required status nothing has posted yet.
# A StatusContext's own PENDING is the opposite and is translated away in check_nodes.

[ptr727]

A second suppressed block from that same round, which the digest surfaced only because it reads every round rather than the head. I had answered one block of four and pushed before this one existed to be read, so this is the round-scoping hazard catching me in the middle of a change about exactly that class of miss. Three of the four are real and fixed in c2d56c6. One is declined with the diff.

1. NOT_PICKED_UP hard-codes a runner remedy but is also returned for EXPECTED.

For EXPECTED, this wording is inaccurate and can send readers toward the wrong remediation.

Right, and this is a finding created by the previous round's fix, since EXPECTED joined the unstarted set one commit ago to stop it reporting as a red check. Folding it in there solved that and bought this: no runner is owed a status nothing has posted, so re-running a workflow clears nothing, and the starved wording points at the runner pool when the thing missing is a poster. Wrong in both directions, as you say.

Taken as your option (a), a separate shape. NOT_POSTED now carries its own line, CHECK NEVER POSTED, naming that no runner is owed it. A case asserts the two messages do not borrow each other's remedy, which is the only reason the shapes are told apart at all.

2. The 42 docstring lists a failed check but the code also requires BLOCKED.

If failed required checks can yield a different mergeStateStatus (commonly DIRTY/UNSTABLE), 42 may not fire even though the digest can report stuck=FAILED.

Narrowed the documentation, which is your second option, because the code is right: a failed required check reads BLOCKED, while UNSTABLE is what a failed non-required one reads and DIRTY is a conflict. So 42 not firing on UNSTABLE is correct behaviour and the docstring was simply short of saying so. The direction that matters is the one you name: a reader who sees stuck=FAILED beside exit 0 and finds no stated condition concludes the field is unreliable. The docstring now names BLOCKED and all four shapes, scripts/README.md says why, and a case holds the docstring to both.

3. Every non-CheckRun node is treated as a StatusContext.

It will silently mis-normalize if GitHub adds a new union member.

Agreed, and the failure would be quiet in the worst way: a third member renders nameless, because the label is read under a key it does not use, and reports as a red check, because the state read off it is not there. So an unknown member would arrive as an anonymous failure. The branch is explicit now and unknown members are skipped, which loses a check that cannot be read where forcing invents a verdict for one, and checks_tally counts what was read so the skip shows as a smaller total. A case feeds a SomethingNew node beside a real one and asserts only the real one is counted.

4. Declined: TODO.md indentation.

This line's indentation changed from an indented sub-bullet ( - ...) to a top-level bullet (- ...).

Checked against the diff rather than the rendering, and the level is unchanged. git diff origin/develop -- TODO.md gives exactly two changed lines:

-- Re-vendor the changed `verbatim` cont
+- Re-vendor the changed `verbatim` cont

Both begin - Re-vendor at column zero, verified through cat -A so no leading whitespace is hidden. The entry has always been a top-level bullet, and the edit was a substring replacement inside the line that touched no indentation. Nothing to restore.

299 cases pass across the three scripts/ suites, and prose_lint, repo_gate, and markdownlint-cli2 are clean.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

scripts/pr_review.py:133

• The rollup contexts are hard-limited to 100 (contexts(first:100)), but the code doesn’t detect/report truncation. If a PR has >100 contexts, required checks could be omitted from checks=N/M and stuck= (silent false-clean risk). Consider querying pageInfo{hasNextPage} for contexts and surfacing an explicit warning in the digest (e.g., CHECKS TRUNCATED) when hasNextPage is true.

    commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state
      contexts(first:100){ nodes{
        __typename
        ... on CheckRun{ name status conclusion startedAt }
        ... on StatusContext{ context state createdAt }
      }}}}}}

scripts/pr_review.py:548

• checks_tally(pr) and checks_stuck(pr, ...) both call check_nodes(pr), so the rollup is normalized twice per digest. Consider calling check_nodes(pr) once in digest, then computing (ok, total) and stuck from that shared list to avoid duplicated parsing and keep the digest logic single-pass.

    ok, total = checks_tally(pr)
    stuck = checks_stuck(pr, now, grace, stall)

scripts/pr_review.py:361

• In normalized check nodes, the started field is startedAt for CheckRun but createdAt for StatusContext. Since those timestamps have different semantics, the shared key name is misleading and makes later code read like it’s comparing equivalent ‘start times’. Consider renaming the normalized key to something shape-agnostic (e.g., stamp/since) and updating age(...) call sites accordingly.

            out.append({'name': n.get('context') or '',
                        'state': 'IN_PROGRESS' if state == 'PENDING' else state,
                        'conclusion': state, 'started': n.get('createdAt') or ''})

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[ptr727]

Two more suppressed blocks, from rounds queued earlier that landed late. Worth recording as a mechanism note: three clear-and-re-request cycles each eventually produced a review, so REQUEST_NOT_PICKED_UP was reporting slowness rather than a dropped request, and every one of those reviews carried different findings. Eight findings here. Four real and fixed in 1abc9c7, three declined on the schema, one already fixed a commit earlier.

Real

1. contexts(first:100) has no truncation guard.

If a PR has >100 contexts, required checks could be omitted from checks=N/M and stuck= (silent false-clean risk).

The best finding of the eight, because it is the window_blind failure this script already guards the review and comment connections against, one connection along, and I added a third connection without the guard. A required check past the window would be absent from the tally and the stuck reading alike, so the digest would render a clean pass over a check it never saw. A fleet repository with a matrix build reaches a hundred contexts long before this one does. CHECKS TRUNCATED now reports it from the connection's own hasNextPage.

2. check_nodes is called twice per digest.

checks_tally(pr) and checks_stuck(pr, ...) both call check_nodes(pr), so the rollup is normalized twice per digest.

Correct, and pointed in a script whose stated reason for existing is that the reading is the cost. Both readers now take the normalized list and the digest parses once. A case counts the calls rather than reading the output, because the output is identical either way, which is exactly what let the second call go unnoticed.

3. The 42 message claims a required check is the blocker.

Since BLOCKED can also be caused by approvals/threads, and the rollup may include non-required checks, this can mislabel the merge blocker.

Right, and it is the same overclaim I had just narrowed the exit code to avoid, left standing in the wording. Gating on BLOCKED establishes that something gates the merge, not that this check is it. The message now states the two facts as a coincidence and says outright that which of them gates the merge is not read here.

4. started holds startedAt for one shape and createdAt for the other.

The shared key name is misleading and makes later code read like it's comparing equivalent 'start times'.

Agreed, renamed to since. One name over two meanings invites exactly the comparison the normalization exists to make safe.

Declined, on introspection rather than on argument

Three findings held that WAITING, PENDING and REQUESTED are not members of CheckStatusState, and that the constant and the fixtures using them are therefore unrepresentative. The live schema disagrees:

CheckStatusState: REQUESTED, QUEUED, IN_PROGRESS, COMPLETED, WAITING, PENDING

All three are real members, so the set is right and the cases model states GitHub can actually return. Keeping them is what makes the fixtures representative rather than less so. The StatusState side of the finding was accurate and is already reflected, since EXPECTED and PENDING are that enum's and are handled per shape.

While in the schema, the union:

StatusCheckRollupContext: CheckRun, StatusContext

Two members today, which is why the unknown-member skip added a commit ago is robustness against a future addition rather than a fix for a live defect. Its comment said so, and this confirms it.

Already fixed

The NOT_PICKED_UP wording for EXPECTED was raised again here, and it was fixed in the previous commit as a separate NOT_POSTED shape with a CHECK NEVER POSTED line of its own. That round's copy of the finding is answered in the comment above this one.

302 cases pass across the three scripts/ suites, and prose_lint, repo_gate, and markdownlint-cli2 are clean.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

scripts/pr_review.py:635

• digest(..., checks=...) assumes checks is already normalized to contain name/state/conclusion/since, but the rendering path indexes node[...] directly. If a caller accidentally passes raw GraphQL rollup nodes (or any other dict shape), this will raise KeyError and take down the digest. Consider (mandatory) either (1) validating/normalizing checks inside digest (e.g., always run through a normalizer when keys are missing), or (2) making the contract explicit in the type/name/docstring and using .get(...) defaults in the rendering path to avoid hard crashes.

        if shape == 'NOT_PICKED_UP':
            lines.append(f'  CHECK NOT PICKED UP ({node["name"]!r}, queued {mins} with no '
                         'runner assigned): nothing here starts it, because the runner pool is '
                         'GitHub-hosted, so re-run the workflow or wait on that capacity')
        elif shape == 'NOT_POSTED':
            lines.append(f'  CHECK NEVER POSTED ({node["name"]!r}, expected {mins} and not '
                         'reported): a required status whose poster has not spoken, so no runner '
                         'is owed it and re-running a workflow here clears nothing')
        elif shape == 'RUNNING_LONG':
            lines.append(f'  CHECK RUNNING LONG ({node["name"]!r}, running {mins}): it has a '
                         'runner, so it is not starved, and whether this is hung or merely slow '
                         'is a judgment against what this job normally costs')
        else:
            lines.append(f'  CHECK FAILED ({node["name"]!r}, {node["state"]}/'
                         f'{node["conclusion"]}): a verdict rather than a stuck check, so read '
                         'the run and fix it, since no wait and no re-run clears a real failure')

scripts/pr_review.py:619

• mins can become negative if the local machine clock is behind GitHub timestamps (or any clock skew yields elapsed < 0), producing confusing output like queued -3m and potentially masking how long something has actually been waiting. Consider clamping negative elapsed to 0 (or treating it as unknown) before formatting/reporting.

        elapsed = age(node.get('since') or '', now)
        mins = 'age unknown' if elapsed is None else f'{int(elapsed // 60)}m'

scripts/pr_review.py:138

• CHECKS_WINDOW was introduced to keep the truncation message and the query window from drifting, but the query still hardcodes contexts(first:100). To make drift impossible even when tests aren’t run, consider defining Q_FULL with string interpolation so it uses CHECKS_WINDOW directly (instead of duplicating 100 here).

    commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state
      contexts(first:100){ pageInfo{ hasNextPage } nodes{
        __typename
        ... on CheckRun{ name status conclusion startedAt }
        ... on StatusContext{ context state createdAt }
      }}}}}}

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[ptr727]

Four more suppressed blocks, raised on 1abc9c7f, carrying six distinct findings after de-duplication. All six real, all fixed in 7d2b2b2, alongside the inline thread answered above.

1. CHECKS_WINDOW did not actually drive the query.

To make the constant actually enforce sync (and avoid future mismatches), build Q_FULL using CHECKS_WINDOW rather than duplicating the literal.

Raised three times across the blocks, and correct: a constant sitting beside a hard-coded contexts(first:100) documents the literal rather than governing it, and the case I added to hold the two together only holds where someone runs it. The constant is substituted into Q_FULL now, so a change to it cannot leave the query behind. It is a .replace on a token rather than an f-string, because GraphQL is braces from end to end and interpolation would need every one of them doubled, which is unreadable against the schema. A case asserts the rendered query carries the constant.

2. wait recomputed the stuck shapes the digest had already computed.

That duplicates work and risks divergence if parameters/logic change later.

Right, and the divergence is the part that matters here rather than the work: the exit code and the printed digest have to agree, and computing the shapes twice made that agreement a coincidence of the arguments passed rather than a property. This file already established the idiom for exactly that, in how stalled is threaded through, so the digest now takes the caller's list.

3. The rendering path indexed node["name"].

If a caller accidentally passes raw GraphQL rollup nodes, this will raise KeyError and take down the digest.

Agreed and fixed, for the same reason age catches two exceptions rather than one: a bad shape reaching the one call whose whole job is to report the state should cost a field, never the report. Every field reads with .get and an unnamed node renders as 'unnamed'. A case drives it with a raw node through checks.

4. mins can render negative under clock skew.

Producing confusing output like queued -3m.

Fixed by clamping the display while the comparison keeps the raw value, so skew cannot make a check report as stuck either. One correction to the premise, checked rather than assumed: through the CLI this is unreachable, because a negative age cannot exceed a non-negative threshold and the parser refuses a negative one, so no shape is judged and mins never renders. It is reachable on the library path, which takes the thresholds directly, so the clamp is hardening there rather than a live defect. The case drives that path and also asserts the CLI refuses the threshold that would reach it.

5. --check-grace help described only the queued shape.

Update the help text to reflect that it governs both "never posted" and "not picked up" thresholds.

Fixed. The grace does govern both, deliberately, since an unposted status and an unstarted job are both judged on how long nothing has happened.

6. The runbook's 42 list omitted the unposted shape.

This makes the guidance incomplete/misleading for the new behavior.

The one that stings, because it is a documented sweep failure rather than a code defect: I had updated the module docstring and scripts/README.md when NOT_POSTED was added and left .github/copilot-instructions.md stating the old three-shape list, which is the third surface. Fixed, and the runbook now also carries the BLOCKED condition it was missing. A grep for the old shape list across the tree is clean.

7. The payload fixture read if checks.

That makes it impossible to represent a real rollup with zero contexts versus a null rollup.

Correct, and the two states are genuinely different: an empty rollup is a pull request whose checks have not registered yet, which is what checks=0/0 currently reads on this very pull request, while a null one has none at all. The fixture compares against None.

309 cases pass across the three scripts/ suites, and prose_lint --diff, repo_gate, and markdownlint-cli2 over all 44 files are clean.

[ptr727]

Parked pending the GitHub Actions outage

Not blocked on anything in this change. GitHub Actions is in a major outage, incident qcvjkzcs7j74, investigating since 15:22 UTC and still unresolved at 18:46 UTC. The status text names the exact symptoms this pull request has been showing:

Workflow runs are still failing, and jobs may remain queued for an extended period before starting or may time out.

plus Copilot code review degradation and delayed webhook deliveries.

That accounts for every anomaly here. checks=0/0 on the current head means no run has been assigned a runner at all, and the un-picked-up review requests were slow rather than dropped — each clear-and-re-request eventually produced a review, and each carried different findings, which is how fourteen rounds accumulated on a five-commit branch. The new reporting was reading the outage correctly rather than false-positiving, which is a better shakedown than a synthetic case would have given it.

State at parking

• Head 7d2b2b2. Four review threads, all resolved with substantive replies. Seven suppressed blocks, all answered in this conversation quoting each finding.
• 20 distinct findings across the loop: 16 real and fixed, 4 declined with evidence — three on schema introspection (CheckStatusState does contain WAITING, PENDING, and REQUESTED), one on the diff (the TODO.md bullet's indentation never changed).
• 309 cases pass across the three scripts/ suites. prose_lint.py --diff, repo_gate.py, spec/validate.py, and markdownlint-cli2 over all 44 files are clean.
• No round has landed on 7d2b2b2 yet, and no CI has run on it.

To finish

One review round on the current head and one green run. The last five batches were consistency and robustness findings with no new semantics, so the loop is converging rather than still opening. Then it is a squash into develop, which is the maintainer's call.

One follow-up this surfaced, deliberately not fixed here

never_picked_up's message asserts "nothing is working on this and waiting on will not start it". Today disproved that: every request it reported eventually produced a review. The reading is accurate — there was no pickup event — but the wording overstates what a missing event proves, which is the same overclaim shape this round narrowed in the 42 message. It deserves its own evidence rather than a rewrite off one outage.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

scripts/pr_review.py:814

• main() computes stuck for the exit-code decision, but digest() recomputes stuck shapes again (and re-parses timestamps via age()), which duplicates work and risks drift if the classification logic evolves. Consider passing the already-computed stuck into digest() (e.g., an optional stuck= parameter), or refactoring so a single function returns both the digest and the computed stuck shapes used for the exit code.

    now = datetime.now(timezone.utc)
    # Parsed here and handed down, so the digest and the exit code share one read of the rollup.
    # Deriving the stuck shapes from that list costs no parse, which is what was doubled.
    checks = check_nodes(final)
    stuck = checks_stuck(checks, now, a.check_grace, a.check_stall)
    out, _ = digest(owner, repo, a.number, pr=final, stalled=stalled, now=now,
                    grace=a.check_grace, stall=a.check_stall, checks=checks)

scripts/pr_review.py:760

• This error message prints the raw values but not the unit, even though flags are expressed in seconds. To make CLI failures self-explanatory, include the unit (e.g., "got 300s and 1800s") or echo the full flag/value pairs so users don’t misread them as minutes.

        ap.error('--check-grace must be less than --check-stall, since a queued check is '
                 f'judged sooner than a running one (got {a.check_grace} and {a.check_stall})')

scripts/pr_review.py:324

• stamp.replace('Z', '+00:00') replaces all occurrences of Z, not just a trailing UTC suffix. It’s safer to only rewrite when the string endswith 'Z' (or to slice/concatenate the last character) so an unexpected Z elsewhere can’t be silently altered before parsing.

        return (now - datetime.fromisoformat(stamp.replace('Z', '+00:00'))).total_seconds()

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

scripts/README.md:138

• This sentence refers to the "42 message" for the stuck-check explanation, but the stuck-check exit code introduced/used by this PR is 44. The message naming should match the implemented exit code to avoid confusion.

The contexts connection is guarded the way the review and comment windows are, since it has the same failure. A rollup past a hundred contexts would drop the rest silently, so a required check among them would be absent from the tally and the stuck reading alike and the digest would render a clean pass over a check it never saw, which a fleet repository with a large matrix build reaches long before this one does. `CHECKS TRUNCATED` says so instead. The rollup is normalized once per digest and the list handed to both readers, since the parse is what this script exists to spend once, and the two readers calling it separately was invisible in the output, which is what let it pass. The `42` message is worded as a coincidence rather than a cause, because nothing here proves the stuck check is the blocker: `BLOCKED` is also worn by an open thread or a missing approval, so naming the check as the blocker would assert a link this cannot read. The normalized stamp key is `since` rather than `started`, since it holds a `CheckRun`'s `startedAt` for one shape and a `StatusContext`'s `createdAt` for the other, and one name over two different meanings reads like a comparison of equivalents.

scripts/README.md:134

• This paragraph refers to exit code 42 for the stuck-check path, but the code/docs in this PR define stuck required checks (with mergeStateStatus: BLOCKED) as exit 44. Keeping 42 here will mislead readers about which condition wait is signaling.

This issue also appears on line 138 of the same file.

A check merely still running normally is **not** any of these and exits `0`. That boundary is the whole design, because `wait` returns the moment coverage lands and on almost every pull request the checks are still going at that instant, so taking `42` for a pending check would make `42` the ordinary outcome and a code that fires always carries nothing. `42` additionally requires `mergeStateStatus: BLOCKED`, which the module docstring's own list of shapes has to name rather than only implying, because a reader who sees `stuck=FAILED` and exit `0` on a merge that is `UNSTABLE` should find the condition written down rather than infer the field is unreliable. It is because a rollup carries checks the ruleset does not require and four of the six on a green pull request here are exactly that, so the code borrows GitHub's own reading of which checks gate a merge rather than reading the ruleset's contexts over another call. `CLEAN` proves no required gate is outstanding whatever else the rollup is doing, and without that condition a stuck check nothing requires returns `42` on a mergeable pull request. The digest names the check either way, so the narrower code costs the reader nothing. Both of those came out of this change's own review.

.github/copilot-instructions.md:319

• This bullet says wait exits 42 for stuck required checks on mergeStateStatus: BLOCKED, but this PR introduces that behavior as exit code 44 (with 42 reserved for partial coverage). Updating this avoids sending readers to the wrong exit-code meaning.

- **`BLOCKED` never says which gate, so never infer one.** The same word covers a red check, a required check nothing is running, an unresolved thread, and a missing approval, and the bullet above says "most often" rather than "always" for that reason. Read the checks instead of guessing: `pr_review.py status` prints `checks=N/M` beside the merge word and names a stuck one, and it exits `42` from `wait` where the merge reads `BLOCKED`, the review loop closed, and a check is starved, expected and never posted, running far past what the job costs, or failed. A **queued check with no runner** is the case that reads exactly like patience: a run here polled `BLOCKED` for twenty-five minutes on a pull request whose only unfinished check was an aggregator job GitHub dispatched and never assigned a runner, and the cause came from the maintainer rather than from any field. Nothing agent-side starts that job, because the pool is GitHub-hosted, so the remedy is a re-run of the workflow or waiting on that capacity, and it is **not** a re-request, a rebase, or an empty commit. A job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so a queued check is never a dependency waiting its turn.

[ptr727]

Answering the three suppressed findings raised on 5b6e887 here, since a suppressed comment has no thread to resolve. All three accepted, all fixed in 6fc35d9.

This sentence refers to the "42 message" for the stuck-check explanation, but the stuck-check exit code introduced/used by this PR is 44.

This paragraph refers to exit code 42 for the stuck-check path, but the code/docs in this PR define stuck required checks (with mergeStateStatus: BLOCKED) as exit 44.

This bullet says wait exits 42 for stuck required checks on mergeStateStatus: BLOCKED, but this PR introduces that behavior as exit code 44.

All three correct, all three the same defect, and it is mine rather than the merge's.

This branch was parked on 2026-08-06 and develop has since taken exit 42 for a round that read fewer files than the pull request changed, with 43 for a shape the reader does not know. Both arrived in #607, #608 and #613 while this sat. The check reading here was renumbered to 44 and ranked under both, since 43 says no field can be believed and 42 says part of the diff has no review, and only once those are clear is a wedged required check the thing worth reporting.

The renumber swept the docstring, the code and the tests, and stopped there. It missed three prose statements, which is sweeping by instance rather than by term, and it is the failure this repository has a rule about.

Swept by term now. Every remaining 42 in scripts/README.md and the runbook is the coverage reading and correctly stays, and no other file in the tree states a code for the stuck reading. Verified with a tree-wide search for stuck beside a 4x code rather than by re-reading the two files the findings named.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/pr_review.py:1063

• The CHECK FAILED digest line reads "the run and fix it", which is grammatically incorrect and unclear about the next step. Make the message explicit (e.g., read logs/fix) so the digest remains actionable.

            lines.append(f'  CHECK FAILED ({name!r}, {node.get("state")}/'
                         f'{node.get("conclusion")}): a verdict rather than a stuck check, so '
                         'the run and fix it, since no wait and no re-run clears a real failure')

scripts/pr_review.py:51

• The PR description states that wait gains exit code 42 for stuck required checks, but the implementation and docs use exit code 44 (42 is already used for partial coverage). Please align the PR description to avoid confusion for users relying on it.

           44 = the review loop closed, the merge reads BLOCKED, and a check is in a shape no
           wait clears: queued with nothing acting on it, expected and never posted, running
           far past what the job costs, or failed. A check merely still running normally is
           not this and exits 0, and neither is a stuck check on a merge that is not BLOCKED,
           since the rollup carries checks no ruleset requires. The digest reports the check
           in both cases, so a shape outside 44 is still named rather than lost.

[ptr727]

Answering both suppressed findings raised on 6fc35d9 here, since a suppressed comment has no thread to resolve. Both accepted, fixed in 887638f and in the description above.

The CHECK FAILED digest line reads "the run and fix it", which is grammatically incorrect and unclear about the next step. Make the message explicit (e.g., read logs/fix) so the digest remains actionable.

Correct, and it lands on the one shape of the four whose remedy is actually a fix. A starved check wants a re-run, an unposted one wants its poster, a long one wants a judgment, and only this one wants someone to read the run, which is the line that failed to say so. It reads "read the run and fix what failed" now.

The PR description states that wait gains exit code 42 for stuck required checks, but the implementation and docs use exit code 44 (42 is already used for partial coverage). Please align the PR description.

Correct, and it is the same failure as the round before, one surface further out. The renumber swept the code, the docstring, scripts/README.md, the runbook and the tests, and stopped at the tree. The description is not in the tree and says what this change does, so it was the last place still promising the old code.

The description now carries 44 throughout and a section saying where the renumber came from, which is that this branch was parked on 2026-08-06 and develop took 42 for partial coverage and 43 for an unreadable shape while it sat.

One thing corrected beyond the finding, since re-reading the description for this found it. It claimed 291 cases across the three scripts/ suites, measured before the outage. The three now run 461, with 217 in test_pr_review.py against develop's 176. Both sides grew, so the number is restated against the merge rather than left as a measurement of a tree that no longer exists.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.