Make a wait report its own failure instead of reading as pending

[ptr727]

Closes #524. Closes #525.

Both issues describe the same defect from two sides: a wait has no failure case, so "broken" and "not yet" are the same observation.

What the record shows

#525 reports a Copilot quota refusal that leaves the coverage check waiting forever. The refusal is not in the record. On the pull request that prompted it, the timeline reads review_requested 00:46:48Z then reviewed 00:48:54Z, so the review landed on the current head about two minutes after the request, and Copilot posted no comment of any kind. No quota or rate-limit message appears on the other repository's open pull request either. The stall it describes is the broken watcher of #524, with an external cause attached to it afterwards.

That shapes the fix. The issue's suggested clause is right and is adopted, because a real quota refusal would behave exactly as it describes. The detection is not built on the wording it reports, since no sample of that wording exists.

The changes

AGENTS.md, the wait mechanics. A wait separates three outcomes and says which one it reached. Run the command once in the foreground and read it before backgrounding it. Never let || echo '[]', || true, or 2>/dev/null stand in for a failed command, which is the suppression the write-safety rules already forbid on a mutation. Emit on failure as loudly as on success, and bound the wait.

GOVERNANCE.md, the reporting half, next to "a green check is not evidence the work happened". A launched process is not a result, and a cause nobody observed is not a diagnosis. Naming a throttle that appears nowhere in the record turns a local defect into a story about someone else and closes the investigation on the wrong party.

.github/copilot-instructions.md. A quota or rate-limit answer is terminal rather than pending, because no formal review follows one and re-requesting does not clear it. Before another wait opens, the request is compared against the newest Copilot activity of any kind, since a wait still reporting "pending" against a landed review is a broken wait rather than a slow reviewer.

scripts/pr_review.py. wait exits 40 when the reviewer answers with a comment postdating its newest review, which is the shape a refusal takes and satisfies no coverage check by design. The comment prints whole, because its wording is the only thing separating a refusal from an ordinary remark. The timeout path prints the full digest, since a bare PENDING line reports a broken poll and a slow reviewer identically.

Both edited rule sections are carried verbatim, so TODO.md records the fleet re-vendor they owe, alongside a second entry for GOVERNANCE.md pointing at scripts/pr_review.py, a path the repos carrying that section do not have.

Verification

The new cases were watched failing against a build with the detection removed. One of them hung for the full default wait rather than failing, which is the bug itself and a case that gates nothing, so it now runs against a zero timeout.

scripts/test_pr_review.py 37 pass, scripts/test_prose_lint.py 153 pass, scripts/test_repo_gate.py 23 pass, spec/audit.py --selftest pass, scripts/repo_gate.py clean, both prose_lint.py invocations clean over the diff and over every touched file, spec/validate.py clean, editorconfig-checker clean. Three pre-existing comment-wrap violations in the touched script are corrected under the correct-as-next-edited rule.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Updates the PR-review wait/runbook tooling and governance docs so a “wait” can clearly distinguish met, pending, and cannot succeed outcomes—preventing broken watchers or terminal Copilot responses (e.g., quota refusal comments) from being misreported as “still pending”.

Changes:

• Extend scripts/pr_review.py wait to detect “answered outside formal review” and exit 40, and to print the full digest on timeout instead of a bare PENDING.
• Add focused unit tests covering terminal comment detection and the new reporting/exit behavior.
• Update carried governance/runbook text and record the required fleet re-vendor sweep in TODO.md.

Reviewed changes

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

Show a summary per file

File	Description
TODO.md	Records fleet re-vendor follow-ups for carried verbatim sections and a downstream-pointer question.
scripts/test_pr_review.py	Adds tests for “answered outside review”, digest printing, timeout reporting, and exit 40.
scripts/README.md	Documents the new wait exit code 40 and explains why terminal comments are printed whole.
scripts/pr_review.py	Implements terminal-comment detection, includes comment metadata in GraphQL queries, and improves timeout output.
GOVERNANCE.md	Adds a verification-discipline rule clarifying that a launched process isn’t a result and unobserved causes aren’t diagnoses.
AGENTS.md	Adds the “three outcomes” clause for waits (met/pending/cannot reach) and avoiding fallbacks that suppress failures.
.github/copilot-instructions.md	Adds guidance to bound waits and treat quota/rate-limit replies as terminal rather than pending.

Suppressed comments (1)

scripts/pr_review.py:60

• Same issue as in Q_LIVE: limiting to comments(last:5) can omit the reviewer’s newest terminal comment once the PR has moderate discussion, which prevents the digest from printing the body that wait tells the user to read. Widen the window so the terminal comment is included when present.

    comments(last:5){ nodes{ author{login} createdAt body } }

[ptr727]

Suppressed comments (1) from review #526 (review), confirmed and fixed in b404164.

scripts/pr_review.py:60
Same issue as in Q_LIVE: limiting to comments(last:5) can omit the reviewer's newest terminal comment once the PR has moderate discussion, which prevents the digest from printing the body that wait tells the user to read. Widen the window so the terminal comment is included when present.

Fixed in b404164. Correct, and it is the same defect the inline thread raised against Q_LIVE, which is what makes it worth stating rather than folding into that answer: the two queries fail differently. A narrow Q_LIVE makes wait poll out its whole timeout against an answer that already arrived, and a narrow Q_FULL makes the digest print answered_outside_review=no while telling nobody there is a body to read. One number, two false cleans.

Measured against a real pull request rather than judged in the abstract: PhotoCleaner#35 carries 17 issue comments, so a codecov post plus two maintainer replies after a refusal would have buried it under the old window. Both queries now read 100.

Widening on its own would leave the class open, so the guard lands with it. GraphQL has no author filter on comments, so the query takes the newest N whoever wrote them, and a full window is the one case where finding nothing and having nothing to find are the same reading. That case now reports answered_outside_review=unknown with a line naming why, rather than no. A case holds COMMENT_WINDOW equal to the number both queries carry, since a guard measuring one number while the query fetches another reads clean on exactly the drift it exists to catch.

[Copilot]

Pull request overview

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

Suppressed comments (4)

scripts/pr_review.py:163

• digest() can raise if a review thread’s first comment has author: null (GitHub GraphQL can return null authors for deleted accounts). Here .get('author', {}) returns None when the key exists but is null, and the chained .get('login') will crash the digest/wait.

    unresolved = [t for t in threads
                  if not t['isResolved']
                  and ((t.get('comments') or {}).get('nodes') or [{}])[0]
                  .get('author', {}).get('login') == REVIEWER]

scripts/pr_review.py:195

• The COPILOT COMMENT marker text states or strongly implies a quota/rate-limit refusal, but the code doesn’t inspect the comment body. Rephrase this as conditional so the digest doesn’t diagnose a cause it hasn’t actually detected.

        lines.append(f'  COPILOT COMMENT ({answer.get("createdAt")}, newer than any review): '
                     'read it before waiting again, since a quota or rate-limit refusal is '
                     'terminal and no review follows it')

scripts/pr_review.py:268

• wait prints a status line that frames the outcome as a quota/rate-limit refusal, but exit 40 is triggered by “answered outside review” regardless of the comment’s content. Make the message conditional on the comment actually being a refusal to avoid misdiagnosis.

        print('status=ANSWERED_OUTSIDE_REVIEW read the comment above, '
              'a quota or rate-limit refusal is terminal and re-requesting does not clear it')

scripts/pr_review.py:91

• The answered_outside_review() docstring currently states Copilot comments outside a review happen “when it will not review at all”, but this function only compares timestamps and does not validate the reason/content. Consider rewording to avoid asserting a specific cause that isn’t actually checked.

This issue also appears in the following locations of the same file:

• line 193
• line 267

    Copilot answers a request with a comment rather than a review when it will not review at all,
    a quota refusal among them, and that answer satisfies no coverage check by design.
    Reading it as an unmet condition is what turns a refusal into a wait with nothing at its end.

[ptr727]

Suppressed comments (4) from review #526 (review), all four confirmed and fixed in c24c2fc. The one remaining block in the digest is the earlier round's, already answered above.

Three of the four are one defect, so they are answered together, and the sweep is why they are worth stating as a class.

scripts/pr_review.py:195
The COPILOT COMMENT marker text states or strongly implies a quota/rate-limit refusal, but the code doesn't inspect the comment body. Rephrase this as conditional so the digest doesn't diagnose a cause it hasn't actually detected.

scripts/pr_review.py:268
wait prints a status line that frames the outcome as a quota/rate-limit refusal, but exit 40 is triggered by "answered outside review" regardless of the comment's content. Make the message conditional on the comment actually being a refusal to avoid misdiagnosis.

scripts/pr_review.py:91
The answered_outside_review() docstring currently states Copilot comments outside a review happen "when it will not review at all", but this function only compares timestamps and does not validate the reason/content. Consider rewording to avoid asserting a specific cause that isn't actually checked. This issue also appears in the following locations of the same file: line 193, line 267.

Fixed in c24c2fc. Correct on all three, and the finding is sharper than it looks. This branch adds a rule to GOVERNANCE.md that a cause nobody observed is not a diagnosis, written because an agent explained a stall with a throttle that appears nowhere in the record. Those three strings then asserted a quota to the reader on the strength of a timestamp comparison. The rule arrived in the same diff as three violations of it.

The messages now state what is actually known: a comment carries no commit, so it covers no head whatever it says, and the reader decides whether it is a refusal, which is terminal, or a remark, which is not. Swept as a class rather than as three instances, so the module header and scripts/README.md carry the same distinction, since the README described the exit as detecting a refusal too.

scripts/pr_review.py:163
digest() can raise if a review thread's first comment has author: null (GitHub GraphQL can return null authors for deleted accounts). Here .get('author', {}) returns None when the key exists but is null, and the chained .get('login') will crash the digest/wait.

Fixed in c24c2fc. A real crash, reproduced before fixing: with a null author on a thread, digest raised AttributeError: 'NoneType' object has no attribute 'get', which takes the whole wait down with it. .get('author', {}) returns the default only when the key is absent, and GraphQL sends it present and null. (x.get('author') or {}) is the form used elsewhere in the file for exactly this, so the inconsistency was the bug. A case covers it and was watched failing against the old expression.

[Copilot]

Pull request overview

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

Suppressed comments (1)

scripts/pr_review.py:179

• answered_outside_review can only be "unknown" due to a saturated window when no reviewer-authored comments are present in the fetched window. If the reviewer has any comment in the window (even a spent/older-than-latest-review one), then any older reviewer comment must be even earlier and therefore cannot be newer than the latest review; in that case the correct summary is answered_outside_review=no, not unknown.

    answer = answered_outside_review(pr)
    answered = 'yes' if answer else ('unknown' if answer_window_saturated(pr) else 'no')

[ptr727]

Suppressed comments (1) from review #526 (review), confirmed and fixed in 4b6bcdb. The two earlier blocks still in the digest are answered above, in the comments closing reviews 4840586518 and 4840604036.

scripts/pr_review.py:179
answered_outside_review can only be "unknown" due to a saturated window when no reviewer-authored comments are present in the fetched window. If the reviewer has any comment in the window (even a spent/older-than-latest-review one), then any older reviewer comment must be even earlier and therefore cannot be newer than the latest review; in that case the correct summary is answered_outside_review=no, not unknown.

Fixed in 4b6bcdb. The proof holds and I checked it rather than taking it: comments returns creation-ordered nodes and last: N takes the newest N, so every comment behind the window is older than every comment inside it. One reviewer comment in view that is spent against a later review therefore bounds every hidden reviewer comment as older still, hence also spent. That is a provable no, not an open question.

The guard was also broader than the line it prints, which already read "the newest N comments carry none from the reviewer". The code did not require that, so the message described the narrower rule the code should have had. It does now: answer_window_saturated returns true only for a full window carrying none of the reviewer's own comments.

Worth naming why this one matters beyond correctness. An unknown a reader can disprove from the digest sitting in front of them is exactly what teaches them to skim the field, and this field exists to be read on the rare occasion it is not no. A guard that cries wolf on every busy pull request would have retired itself.

A case covers the settled shape, a full window whose single reviewer comment is spent, alongside the existing one for the genuinely open shape.

[Copilot]

Pull request overview

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

Suppressed comments (1)

scripts/pr_review.py:114

• answer_window_saturated() treats len(comments) >= COMMENT_WINDOW as “window is full so older comments may exist”, but with comments(last:100) you can’t distinguish “exactly 100 total comments” from “more than 100” unless you also fetch pageInfo.hasPreviousPage/totalCount. In the exactly-100 case (no older comments), this will report answered_outside_review=unknown even though there cannot be an older Copilot comment. Consider extending both GraphQL queries to request comments(last:...){ nodes{...} pageInfo{hasPreviousPage} } (or totalCount) and only returning True when the window is full and there are earlier pages.

    comments = (pr.get('comments') or {}).get('nodes') or []
    return len(comments) >= COMMENT_WINDOW and not reviewer_nodes(pr, 'comments')

[ptr727]

Suppressed comments (1) from review #526 (review), confirmed and fixed in bced438. The three earlier blocks in the digest are answered above, closing reviews 4840586518, 4840604036 and 4840621353.

scripts/pr_review.py:114
answer_window_saturated() treats len(comments) >= COMMENT_WINDOW as "window is full so older comments may exist", but with comments(last:100) you can't distinguish "exactly 100 total comments" from "more than 100" unless you also fetch pageInfo.hasPreviousPage/totalCount. In the exactly-100 case (no older comments), this will report answered_outside_review=unknown even though there cannot be an older Copilot comment. Consider extending both GraphQL queries to request comments(last:...){ nodes{...} pageInfo{hasPreviousPage} } (or totalCount) and only returning True when the window is full and there are earlier pages.

Fixed in bced438, taking the hasPreviousPage half of the suggestion. The count was standing in for a question it cannot answer: a full window and a window holding every comment the pull request has are the same length, so the guard reported a gap on a pull request where nothing was behind the window. hasPreviousPage answers exactly that question, and totalCount would answer it only by arithmetic against the same number, so the boolean is the one that cannot drift.

Worth noting what this means about the guard's own history. It was added last round to close a false clean, was itself too broad in the round after, and was measuring the wrong quantity in this one. Each round narrowed it toward the single unreadable case that remains: comments sit behind the window and none in view are the reviewer's. Everything else is now decidable and says so.

A case holds both queries to asking for hasPreviousPage, because a query that quietly stops asking makes the guard report no rather than unknown, which is the same silent narrowing one level up from the one the field exists to prevent. Cases also cover the complete-window shape this finding names, alongside the genuinely open one.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

scripts/pr_review.py:66

• Same truncation risk as in Q_LIVE: reviews(last:20) can omit the reviewer’s most recent review when there are 20+ newer reviews, which can make digest() and answered_outside_review() reason about the wrong ‘newest review’. Consider widening this window to match COMMENT_WINDOW.

    headRefOid mergeable mergeStateStatus
    reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt body } }
    reviewThreads(first:100){ nodes{ id isResolved
      comments(first:1){ nodes{ author{login} path line body } } }}
    comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } }
  }}}

scripts/pr_review.py:54

• reviews(last:20) can drop the reviewer’s most recent review if the PR has 20+ newer (human) reviews. That makes live_state() misreport done=False and can also trigger a false answered_outside_review (because latest_review becomes empty when no reviewer reviews are in the slice). Widen the review window so the reviewer’s latest review is very unlikely to fall out of view.

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

    headRefOid
    reviews(last:20){ nodes{ author{login} state commit{oid} submittedAt } }
    comments(last:100){ nodes{ author{login} createdAt } pageInfo{ hasPreviousPage } }
  }}}

[ptr727]

Suppressed comments (2) from review #526 (review), both confirmed and fixed in 17e65e8. They are one defect on two lines, so they are answered together. The four earlier blocks are answered above.

scripts/pr_review.py:54
reviews(last:20) can drop the reviewer's most recent review if the PR has 20+ newer (human) reviews. That makes live_state() misreport done=False and can also trigger a false answered_outside_review (because latest_review becomes empty when no reviewer reviews are in the slice). Widen the review window so the reviewer's latest review is very unlikely to fall out of view. This issue also appears on line 61 of the same file.

scripts/pr_review.py:66
Same truncation risk as in Q_LIVE: reviews(last:20) can omit the reviewer's most recent review when there are 20+ newer reviews, which can make digest() and answered_outside_review() reason about the wrong 'newest review'. Consider widening this window to match COMMENT_WINDOW.

Fixed in 17e65e8, and the second half of the first finding is the one that matters, so it is worth separating from the widening it is bundled with.

reviews(last:20) predates this branch, but consuming it as the baseline for dating an answer is what made it load-bearing, so this is a defect the change introduced rather than one it inherited. The failure is asymmetric. A truncated comment window hides an answer, which costs a missed terminal and a wait that runs its course. A truncated review window empties latest_review, which dates every comment as newer, so any reviewer comment reads as an answer and exit 40 fires on a pull request whose review landed. Missing a terminal wastes a wait, and inventing one stops a loop that should have continued, and the second is the worse trade.

Twenty is also well inside reach: PhotoCleaner#35 carries more than thirty reviews, so this is a live path rather than a theoretical one.

Both windows now share one WINDOW, every connection carries pageInfo{ hasPreviousPage }, and one window_blind guard covers all four. Blind on comments leaves the question open and reports answered_outside_review=unknown. Blind on reviews leaves it unaskable, so answered_outside_review returns nothing and the wait keeps polling, on the reasoning that a wait still running is visible to whoever reads the digest while a wrong terminal is not.

A case covers each side, and the reviews-blind case was watched failing against the unguarded expression, where it returned the comment as an answer exactly as described. A contract case holds all four windows to the same constant and to asking for hasPreviousPage, since a connection that quietly stops asking reports no rather than unknown.

[Copilot]

Pull request overview

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