From 97a9c1205f6797421e491504b07b2ff23df47a32 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:19:37 -0700 Subject: [PATCH 1/7] Stop Requesting Copilot Into a Reached Rate Limit A Copilot round reading only "encountered an error" is what the weekly rate limit posts, its cause written to the reviewer's own Actions run. pr_review.py now reads that run's job log: a logged rate limit reports as refusal=QUOTA with the reset time, and anything else as refusal=ERROR, a possible quota hit. wait sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits 46. It also no longer requests while the repository-wide quota signal is set, where it used to request first and stop after. --ignore-quota-signal overrides both. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 3 +- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 3 +- .github/skills/pr-review-conduct/SKILL.md | 3 +- scripts/README.md | 2 +- scripts/pr_review.py | 158 ++++++++++++++++-- tests/test_pr_review.py | 99 +++++++++-- 7 files changed, 238 insertions(+), 32 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 90a1a98c..5f880c84 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -62,7 +62,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin A refusal is not that coverage, so this item stays unsatisfied under one, and the loop clears it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing - the loop does clears, an account-quota refusal carrying the current head, which is the case + the loop does clears, the pull request's newest Copilot review being an account-quota refusal + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 7916f766..f991a0d1 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -0331a2ae5b800214 +1b318dd7057deb85 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 90a1a98c..5f880c84 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -62,7 +62,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin A refusal is not that coverage, so this item stays unsatisfied under one, and the loop clears it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing - the loop does clears, an account-quota refusal carrying the current head, which is the case + the loop does clears, the pull request's newest Copilot review being an account-quota refusal + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 90a1a98c..5f880c84 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -62,7 +62,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin A refusal is not that coverage, so this item stays unsatisfied under one, and the loop clears it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing - the loop does clears, an account-quota refusal carrying the current head, which is the case + the loop does clears, the pull request's newest Copilot review being an account-quota refusal + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. diff --git a/scripts/README.md b/scripts/README.md index 4c9e2a1e..df323174 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. -`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46`, since a request into a reached limit spends what it cannot recover. Otherwise the script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. Over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed and all three merged, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8d676bb6..3e603bb1 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -60,7 +60,7 @@ re-requested on the same head states coverage or carries a table only by chance. A refusal naming the account quota still reads as absent here, exit 0, since a refusal covers no head either. Its printed digest line carries `refusal=QUOTA` - regardless. `wait` is where that state gets its own exit codes, 46 and 47 below, + regardless, and `refusal=ERROR` for an error refusal whose run logged no rate limit. `wait` is where that state gets its own exit codes, 46 and 47 below, because only `wait` is the command a caller might otherwise poll out a timeout on. `unresolved` counts every tracked reviewer's own open thread, not only Copilot's: CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are @@ -190,8 +190,12 @@ 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. - 46 = the review carrying the head is a refusal naming the account quota specifically, - printed above under COPILOT REFUSED THIS ROUND. That is an account-level state a + 46 = the newest Copilot review on the pull request, on this head or an earlier one, + is a refusal naming the account quota, or one saying only that it encountered an + error, printed above under COPILOT REFUSED THIS ROUND. The weekly rate limit posts + that error body and writes its cause to the reviewer's own Actions run, so that run's + job log is read: a logged rate limit reports as the quota with its reset time, and + anything else as a possible quota hit. Either way no request is sent. That is an account-level state a re-request or a further wait does not clear, unlike 41's other causes (a file count over the limit, cleared by splitting the pull request), so it is its own code rather than folded into 41: proceed on the other reviewers' coverage instead of retrying. @@ -327,6 +331,14 @@ # This script's own corpus and this file both quote the sentence below its overview, same as the refusal wording itself does. # Observed once here: "Copilot was unable to review this pull request because the user who requested the review has reached their quota limit." QUOTA = re.compile(r"reached (?:their|its|his|her|your|my) quota limit", re.IGNORECASE) +ERROR_REFUSAL = re.compile(r"encountered an error", re.IGNORECASE) +COPILOT_RUN_PATH = "dynamic/agents/copilot-pull-request-reviewer" +RUN_ERROR_TYPE = re.compile(r"errorType: '([a-z_]+)'") +RUN_RATE_LIMIT = re.compile( + r"(You.ve reached your [^\n]*?rate limit\.[^\n]*?)(?= or switch| Learn More|$)", re.MULTILINE +) +ANSI = re.compile(r"\x1b\[[0-9;]*m") +ISO_STAMP = re.compile(r"\d{4}-\d\d-\d\dT\d\d:\d\d:\d\dZ") # A structural marker rather than prose, so reading it needs no per-bot wording model the way `QUOTA` above needs one for Copilot's free-text refusal. # Observed on CodeRabbit, a plain PR comment rather than a formal review: "". # The service name is captured rather than assumed. @@ -1216,6 +1228,78 @@ def quota_refusal(node: dict) -> bool: return bool(QUOTA.search(refusal_of(node))) +def possible_quota(node: dict) -> bool: + """True where a refusal on this node names an error rather than any cause. + + The weekly rate limit posts exactly this body, its cause written only to the task log, so it + reads as a possible quota hit rather than as a transient failure. A re-request into a reached + limit spends nothing it can recover and returns the same body, which is why one is enough. + """ + return bool(ERROR_REFUSAL.search(refusal_of(node))) + + +@functools.cache +def run_cause(owner: str, repo: str, oid: str, before: str) -> tuple[str, str] | None: + """The error type and rate-limit sentence the reviewer's own failed run logged, or None. + + An error refusal's body names no cause, and the run that produced it does: the reviewer runs + as an Actions workflow on the reviewed commit, and its job log states the weekly limit and + when it resets. The run read is the newest failed one on that commit created no later than + the review, so a later run on the same commit is not read as this round's. + + None wherever any read fails or finds nothing, which leaves the round a possible quota hit + rather than a confirmed one or a cleared one. Memoized for the run, so the digest and the exit + code read one log once. + """ + if not oid or not ISO_STAMP.fullmatch(before): + return None + proc = gh_rest( + f"repos/{owner}/{repo}/actions/runs?event=dynamic&head_sha={oid}&per_page=50", + f'[.workflow_runs[] | select(.path == "{COPILOT_RUN_PATH}" and .conclusion == "failure"' + f' and .created_at <= "{before}")] | max_by(.created_at) | .id // empty', + ) + run = proc.stdout.strip() + if proc.returncode != 0 or not run.isdigit(): + return None + proc = gh_rest(f"repos/{owner}/{repo}/actions/runs/{run}/jobs", ".jobs[0].id // empty") + job = proc.stdout.strip() + if proc.returncode != 0 or not job.isdigit(): + return None + proc = gh_rest(f"repos/{owner}/{repo}/actions/jobs/{job}/logs", raw=True) + if proc.returncode != 0: + return None + log = ANSI.sub("", proc.stdout) + kind, said = RUN_ERROR_TYPE.search(log), RUN_RATE_LIMIT.search(log) + if kind is None: + return None + return kind.group(1), said.group(1).strip() if said else "" + + +def confirmed_quota(owner: str, repo: str, node: dict) -> str: + """The run log's rate-limit sentence where an error refusal's run logged a rate limit, else "".""" + if not possible_quota(node): + return "" + cause = run_cause( + owner, repo, (node.get("commit") or {}).get("oid") or "", node.get("submittedAt") or "" + ) + if cause is None or cause[0] != "rate_limit": + return "" + return cause[1] or "the run logged a rate limit and stated no reset" + + +def stopping_refusal(pr: dict) -> dict | None: + """The reviewer's newest review on this pull request, on any head, where it is a quota refusal or an error. + + Read across heads rather than on the head alone, since a push after such a refusal moves the + head and the account state the refusal reports does not move with it. A genuine round since + spends it, the newest review being the one read. + """ + newest = newest_of(reviewer_nodes(pr, "reviews")) + if newest is None or not (quota_refusal(newest) or possible_quota(newest)): + return None + return newest + + def refusing_review(pr: dict) -> dict | None: """The reviewer's newest refusal carrying the current head, where one is there. @@ -2992,7 +3076,15 @@ def digest( # That tells a reader to split a pull request the reviewer has just reviewed. refusal = None if on_head else refusing_review(pr) # Read once and handed to the line below, since `quota_refusal` re-walks `refusal_of`. - refusal_field = "no" if not refusal else ("QUOTA" if quota_refusal(refusal) else "YES") + refusal_field = ( + "no" + if not refusal + else "QUOTA" + if quota_refusal(refusal) or confirmed_quota(owner, repo, refusal) + else "ERROR" + if possible_quota(refusal) + else "YES" + ) blind = [f for f in ("reviews", "comments") if window_blind(pr, f)] answered = "yes" if answer else ("unknown" if blind else "no") # Normalized once and handed to both readers, since the parse is the cost here. @@ -3122,6 +3214,8 @@ def digest( lines += [ f" {ln.rstrip()}" for ln in (refusal.get("body") or "").splitlines() if ln.strip() ] + if limit := confirmed_quota(owner, repo, refusal): + lines.append(f" RATE LIMIT FROM THE REVIEWER'S RUN LOG: {limit}") if unknown: # First of the blocks, since it says how far the rest of them can be trusted. lines.append( @@ -3632,13 +3726,20 @@ def reply_to_thread( return 0 -def gh_rest(path: str, jq: str | None = None) -> subprocess.CompletedProcess: +def gh_rest(path: str, jq: str | None = None, raw: bool = False) -> subprocess.CompletedProcess: """One REST read, returned whole so the caller can tell an absent object from an unread one. Unlike `gh_graphql` this does not raise on a non-zero exit, because a 404 here is an answer the caller acts on rather than a failure. Reads only: every path passed in is a GET. + + `raw` is for a job log, which carries terminal escape sequences that `gh` refuses to print + without being told it may. """ - argv = ["gh", "api", path] + (["--jq", jq] if jq else []) + argv = ( + ["gh", "api", path] + + (["--jq", jq] if jq else []) + + (["--allow-escape-sequences"] if raw else []) + ) try: return subprocess.run( argv, capture_output=True, text=True, encoding="utf-8", timeout=30, check=False @@ -3848,9 +3949,10 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument( "--ignore-quota-signal", action="store_true", - help="wait: poll the full --timeout even where the reviewer's own most recent " - "activity elsewhere in this repository is a quota-limit refusal with nothing " - "answering it since, pass this once the quota is believed to have reset", + help="wait: request and poll the full --timeout even where the reviewer's own most " + "recent activity elsewhere in this repository is a quota-limit refusal with nothing " + "answering it since, or this pull request's newest Copilot review is a quota or " + "error refusal, pass this once the quota is believed to have reset", ) ap.add_argument( "--min-rounds", @@ -3976,12 +4078,20 @@ def main(argv: list[str] | None = None) -> int: # Read from the same, unfiltered history rather than one that drops this pull request's own entries: a genuine review on an earlier head of this same pull request, superseded since by a push, is real evidence about the account and not a self-reference to discard. # A refusal on this pull request's own current head still never reaches this signal, since it is caught directly and at higher priority first. signal = None if a.ignore_quota_signal else quota_signal(history) + stopped = None if a.ignore_quota_signal else stopping_refusal(pr) # Request before the first poll, not just at the call site: a caller expects `wait` to make a review happen, not merely to watch for one. # Two prior gaps this closed, a push superseding an already-answered request and an auto-seed that never fired, both left nothing outstanding for the loop below to ever see land. # Skipped once a review already covers the head, once Copilot has already answered outside a formal review, or once something is already in the request set, so a second `wait` on the same PR never double-requests. recorded: bool | None = None final: dict | None = None - if not done and not answer and not drift and not reviewer_requested(pr): + if ( + not done + and not answer + and not drift + and not signal + and not stopped + and not reviewer_requested(pr) + ): line, recorded = request_copilot_review( owner, repo, a.number, pr["id"], copilot_bot_id(history), delays[0] ) @@ -3998,6 +4108,13 @@ def main(argv: list[str] | None = None) -> int: "request, no pending reviewer and no review-request event, so this wait stops here " "rather than polling --timeout out against a request that does not exist." ) + elif stopped: + print( + "note: this pull request's newest Copilot review is a refusal naming the account " + "quota or an error, which is what the weekly rate limit posts, so this wait requests " + "nothing and stops here. Pass --ignore-quota-signal to request and poll anyway, " + "once the limit is believed to have reset." + ) elif signal: # The poll below is skipped rather than shortened, because there is nothing partial about this signal. # The reviewer's own most recent word anywhere in the repository is the account quota, and nothing has answered it since. @@ -4092,15 +4209,34 @@ def main(argv: list[str] | None = None) -> int: # A refusal before an answer, since it names the round that declined where 40 names none. # The digest prints both bodies regardless, so the narrower code costs the reader nothing. refusal = refusing_review(final) + if refusal is None and not a.ignore_quota_signal: + refusal = stopping_refusal(final) if refusal and quota_refusal(refusal): print( - "status=COPILOT_QUOTA_EXHAUSTED the review carrying the head declined because the " + "status=COPILOT_QUOTA_EXHAUSTED the newest Copilot review declined because the " "requesting account has reached its Copilot review quota, printed above under " "COPILOT REFUSED THIS ROUND: that is an account-level state, not one this pull " "request or a re-request clears, so proceed on the coverage the other reviewers " "already gave this pull request rather than waiting on Copilot again" ) return 46 + if refusal and (limit := confirmed_quota(owner, repo, refusal)): + print( + "status=COPILOT_QUOTA_EXHAUSTED the newest Copilot review on this pull request says " + "it encountered an error, and the reviewer's own run log names the cause: " + f"{limit}. Do not re-request before then, and proceed on the coverage the other " + "reviewers already gave" + ) + return 46 + if refusal and possible_quota(refusal): + print( + "status=COPILOT_ERROR_POSSIBLE_QUOTA the newest Copilot review on this pull request " + "says it encountered an error and did not review, printed above under COPILOT " + "REFUSED THIS ROUND, which is the body the weekly rate limit posts. Read it as a " + "possible quota hit: do not re-request, proceed on the coverage the other reviewers " + "already gave, and hand the state to the maintainer" + ) + return 46 if refusal: print( "status=REVIEW_IS_A_REFUSAL the review carrying the head says it did not review, " diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index cc261e85..10c99708 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -259,6 +259,17 @@ def summarized(paths: list[str], covers: str = COVERED) -> str: "review from Copilot again." ) +ERROR_REFUSED = ( + "Copilot encountered an error and was unable to review this pull request. You can try again " + "by re-requesting a review." +) +RATE_LIMIT_LOG = ( + "2026-08-02T10:59:00.0000000Z Error creating PR review request: SessionModelError: You've " + "reached your weekly rate limit. Please wait for your limit to reset on August 9, 2026 at " + "12:00 AM or switch to auto model to continue. Learn More (https://docs.example.test).\n" + "2026-08-02T10:59:00.0000000Z errorType: 'rate_limit',\n" +) + # Quoted from the corpus rather than invented: the body a refused review carried, byte for byte. QUOTA_REFUSED = ( "Copilot was unable to review this pull request because the user who requested the " @@ -433,6 +444,8 @@ def setUp(self) -> None: # Without this a case inherits another's compares and the suite becomes order-dependent. pr_review.changed_at.cache_clear() self.addCleanup(pr_review.changed_at.cache_clear) + pr_review.run_cause.cache_clear() + self.addCleanup(pr_review.run_cause.cache_clear) self.enterContext( mock.patch.object( pr_review, @@ -5342,19 +5355,6 @@ def test_a_reviewer_pending_on_the_confirming_read_is_polled_for(self) -> None: self.assertNotIn("recorded nothing", out) self.assertNotIn("status=REQUEST_NOT_RECORDED", out) - def test_an_unrecorded_request_outranks_the_repo_wide_quota_signal(self) -> None: - """Read on this pull request, so it ranks above the reading from elsewhere.""" - self.answer(payload([])) - unchanged = request_state(events=("RRE_1",)) - self.wire_history([hist_review(962, QUOTA_REFUSED)], (unchanged, unchanged)) - with mock.patch.object(pr_review.time, "sleep") as slept: - self.assertEqual(48, self.cli(["wait", "7"])) - slept.assert_called_once_with(15) - out = self.out.getvalue() - self.assertNotIn("status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE", out) - self.assertIn("note: the review request returned success and recorded nothing", out) - self.assertNotIn("note: the reviewer's own most recent activity", out) - def test_a_drifted_login_in_the_answer_is_not_read_as_unrecorded(self) -> None: """A renamed reviewer is what the poll reports, so the request is not judged on its spelling.""" drifted = {"__typename": "Bot", "login": "copilot-pull-request-reviewer-v2"} @@ -5460,9 +5460,7 @@ def test_a_silent_head_short_circuits_on_the_repo_wide_quota_signal(self) -> Non self.assertIn("status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE", out) self.assertIn("#962", out) self.assertIn("note: the reviewer's own most recent activity", out) - # The auto-request still fires: it is harmless and idempotent. - # It costs nothing extra either, since the same history read already answered the bot-id lookup it needs. - self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) def test_the_current_pull_requests_own_bot_id_still_seeds_the_auto_request(self) -> None: """An earlier round on this very pull request is still a valid id to request with.""" @@ -5546,6 +5544,75 @@ def test_ignore_quota_signal_restores_plain_polling(self) -> None: self.assertNotIn("note: the reviewer's own most recent activity", out) self.assertIn("status=PENDING", out) + def run_log(self, log: str | None) -> None: + """Answer the three reads `run_cause` makes, or fail all three where `log` is None.""" + + def rest( + path: str, jq: str | None = None, raw: bool = False + ) -> subprocess.CompletedProcess: + if log is None: + return subprocess.CompletedProcess([], 1, "", "not read") + if "/actions/runs?" in path: + return subprocess.CompletedProcess([], 0, "101\n", "") + if path.endswith("/jobs"): + return subprocess.CompletedProcess([], 0, "202\n", "") + if path.endswith("/actions/jobs/202/logs") and raw: + return subprocess.CompletedProcess([], 0, "\x1b[36;1m" + log, "") + return subprocess.CompletedProcess([], 1, "", "unexpected read") + + self.enterContext(mock.patch.object(pr_review, "gh_rest", side_effect=rest)) + + def test_an_error_round_on_an_earlier_head_stops_the_auto_request(self) -> None: + """The weekly limit does not move with a push, so the next head is not requested into it.""" + self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(None) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(46, self.cli(["wait", "7"])) + slept.assert_not_called() + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + self.assertIn("status=COPILOT_ERROR_POSSIBLE_QUOTA", self.out.getvalue()) + + def test_a_rate_limit_in_the_run_log_confirms_the_quota_and_names_the_reset(self) -> None: + self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + self.wire_history([hist_review(7, ERROR_REFUSED)]) + self.run_log(RATE_LIMIT_LOG) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(46, self.cli(["wait", "7"])) + out = self.out.getvalue() + self.assertIn("status=COPILOT_QUOTA_EXHAUSTED", out) + self.assertIn("reset on August 9, 2026 at 12:00 AM. Do not re-request", out) + + def test_ignore_quota_signal_requests_past_an_error_round(self) -> None: + self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0", "--ignore-quota-signal"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_genuine_round_since_the_error_spends_it(self) -> None: + pr = payload( + [ + review(oid=OLD, body=ERROR_REFUSED, at=EARLY, rid="PRR_a"), + review(oid=OLD, body=OVERVIEW + "\n" + COVERED, at=LATE, rid="PRR_b"), + ] + ) + self.assertIsNone(pr_review.stopping_refusal(pr)) + + def test_the_digest_reads_an_error_round_by_what_its_run_logged(self) -> None: + for log, field, line in ( + (None, "refusal=ERROR", False), + (RATE_LIMIT_LOG, "refusal=QUOTA", True), + ("2026-08-02T10:59:00Z errorType: 'internal',\n", "refusal=ERROR", False), + ): + with self.subTest(field=field, line=line): + pr_review.run_cause.cache_clear() + self.answer(payload([review(body=ERROR_REFUSED)])) + self.run_log(log) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn(field, out) + self.assertEqual(line, "RATE LIMIT FROM THE REVIEWER'S RUN LOG" in out) + class TestCopilotHistoryReadings(GqlCase): """The readings built from the reviewer's own review history across the repository.""" From 555f463a755fb633ffdb4135458e3cf7276b40e2 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:33:05 -0700 Subject: [PATCH 2/7] Read the Quota Stop From the Full Payload The liveness query carries no review bodies, so the stop never fired there. wait now reads it from a full read, still polls a request already pending, and the digest reports a quota or error refusal on an earlier head. Any rate_limit errorType in the log counts, a log that is not UTF-8 no longer crashes the read, and the 46 lines name the override. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 14 ++-- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 14 ++-- .github/skills/pr-review-conduct/SKILL.md | 14 ++-- scripts/README.md | 2 +- scripts/pr_review.py | 73 ++++++++++++------- tests/test_pr_review.py | 65 ++++++++++++++++- 7 files changed, 133 insertions(+), 51 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 5f880c84..0ba807d2 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -63,9 +63,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing the loop does clears, the pull request's newest Copilot review being an account-quota refusal - or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case - "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state - read from the reviewer's activity elsewhere when this head carries none of its own, and exit + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the + case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account + state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy @@ -153,9 +153,11 @@ must have done. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own coverage of the current head, and the loop's own re-request step below is where a missing one is answered, on the terms stated there. A refusal naming the account quota is its own case rather - than a review: it covers no head, so the gate stays unsatisfied, and nothing the loop does - clears it, since the refusal names no time to wait for and re-requesting returns it again. That - one goes to the maintainer, rather than into a wait with no stated end. + than a review, and so is one saying only that Copilot encountered an error, which is what the + weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the + loop does clears it, since re-requesting returns it again and spends quota doing so. Where the + reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index f991a0d1..ddc9d6fb 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -1b318dd7057deb85 +ffa271d490747a42 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 5f880c84..0ba807d2 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -63,9 +63,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing the loop does clears, the pull request's newest Copilot review being an account-quota refusal - or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case - "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state - read from the reviewer's activity elsewhere when this head carries none of its own, and exit + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the + case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account + state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy @@ -153,9 +153,11 @@ must have done. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own coverage of the current head, and the loop's own re-request step below is where a missing one is answered, on the terms stated there. A refusal naming the account quota is its own case rather - than a review: it covers no head, so the gate stays unsatisfied, and nothing the loop does - clears it, since the refusal names no time to wait for and re-requesting returns it again. That - one goes to the maintainer, rather than into a wait with no stated end. + than a review, and so is one saying only that Copilot encountered an error, which is what the + weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the + loop does clears it, since re-requesting returns it again and spends quota doing so. Where the + reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 5f880c84..0ba807d2 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -63,9 +63,9 @@ visible comments, routinely still carries a finding nobody has answered. Treatin it where it can. A file-count refusal is cleared by splitting the pull request, which is the only cause on record that the loop can clear. `pr_review.py wait` exit `46` is the one nothing the loop does clears, the pull request's newest Copilot review being an account-quota refusal - or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case - "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account state - read from the reviewer's activity elsewhere when this head carries none of its own, and exit + or an error refusal read as a possible quota hit, on this head or an earlier one, which is the + case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account + state read from the reviewer's activity elsewhere when this head carries none of its own, and exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy @@ -153,9 +153,11 @@ must have done. - **Copilot's absence blocks, and is answered elsewhere.** Merge Gate item 2 requires Copilot's own coverage of the current head, and the loop's own re-request step below is where a missing one is answered, on the terms stated there. A refusal naming the account quota is its own case rather - than a review: it covers no head, so the gate stays unsatisfied, and nothing the loop does - clears it, since the refusal names no time to wait for and re-requesting returns it again. That - one goes to the maintainer, rather than into a wait with no stated end. + than a review, and so is one saying only that Copilot encountered an error, which is what the + weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the + loop does clears it, since re-requesting returns it again and spends quota doing so. Where the + reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's `docs/pr-reviewer-reference.md` records what each one does, what shapes it, and which repositories diff --git a/scripts/README.md b/scripts/README.md index df323174..457a75b0 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. -`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46`, since a request into a reached limit spends what it cannot recover. Otherwise the script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46`, since a request into a reached limit spends what it cannot recover. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. Over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed and all three merged, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 3e603bb1..52105d39 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -60,7 +60,9 @@ re-requested on the same head states coverage or carries a table only by chance. A refusal naming the account quota still reads as absent here, exit 0, since a refusal covers no head either. Its printed digest line carries `refusal=QUOTA` - regardless, and `refusal=ERROR` for an error refusal whose run logged no rate limit. `wait` is where that state gets its own exit codes, 46 and 47 below, + regardless, and `refusal=ERROR` for an error refusal whose run log names no rate + limit or could not be read. Either field reads a refusal on an earlier head where it + is the pull request's newest Copilot review and nothing covers the head. `wait` is where that state gets its own exit codes, 46 and 47 below, because only `wait` is the command a caller might otherwise poll out a timeout on. `unresolved` counts every tracked reviewer's own open thread, not only Copilot's: CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are @@ -165,7 +167,8 @@ on the current head, then print the digest. The auto-request is skipped once a review already covers the head, once Copilot has already answered outside a formal review, or once one is already in the pending request set, so calling `wait` again on - the same PR never double-requests. It reads the Copilot reviewer's bot id from the + the same PR never double-requests. It is also skipped under 46's and 47's quota + readings below, since a request into a reached limit spends quota and returns nothing. It reads the Copilot reviewer's bot id from the repository's own most recently updated PRs rather than a fixed id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that narrow window carries no Copilot activity at all, since an outage that outlasts HISTORY_PRS PRs would otherwise @@ -195,18 +198,21 @@ error, printed above under COPILOT REFUSED THIS ROUND. The weekly rate limit posts that error body and writes its cause to the reviewer's own Actions run, so that run's job log is read: a logged rate limit reports as the quota with its reset time, and - anything else as a possible quota hit. Either way no request is sent. That is an account-level state a - re-request or a further wait does not clear, unlike 41's other causes (a file count + anything else, an unreadable log included, as a possible quota hit. Either way no + request is sent, and a request already pending is still polled for. That is an + account-level state a re-request or a further wait does not clear, unlike 41's other + causes (a file count over the limit, cleared by splitting the pull request), so it is its own code rather than folded into 41: proceed on the other reviewers' coverage instead of retrying. 47 = this pull request's current head carries no Copilot activity of its own, and the reviewer's own most recent activity found in the repository, a review or comment - on any pull request including an earlier round on this one, is that same - account-quota refusal with nothing having answered it since. The poll that would + on any other pull request, is that same account-quota refusal with nothing having + answered it since. A refusal on this pull request itself is 46 instead. The poll that would otherwise have run is skipped for this reason, printed as a `note:` line before the - digest, rather than spent finding the same account state out a call late. Pass - --ignore-quota-signal to poll --timeout anyway once the quota is believed to have - reset. 46 is read directly from the current head and always takes priority over 47, + digest, rather than spent finding the same account state out a call late, and no + request is sent. Pass --ignore-quota-signal to request and poll --timeout anyway once + the quota is believed to have reset. 46 is read from this pull request's own reviews + and always takes priority over 47, so a genuine 0/40/41/42/43/45 on this pull request outranks 47 whenever both would otherwise apply. A pending request remains pending until a review, an answer, or the timeout. GitHub's @@ -217,8 +223,8 @@ requesting account's Copilot allowance was exhausted, where no refusal was posted to read and clearing the set and requesting again changed nothing. It is decided one poll interval after the request, and the rest of the poll is skipped, since no - request exists to answer. It outranks 47, being read on this pull request, - and ranks under 0/40/41/42/43/44/45/46. `status` cannot report it, since a request that + request exists to answer. It cannot meet 46 or 47, since neither sends a request, + and ranks under 0/40/41/42/43/44/45. `status` cannot report it, since a request that recorded nothing leaves nothing for a later read to find. 64 = the write scope could not be established or excludes the target, checked before the auto-request or any poll, so a cross-owner target reads and writes nothing here. @@ -1231,9 +1237,9 @@ def quota_refusal(node: dict) -> bool: def possible_quota(node: dict) -> bool: """True where a refusal on this node names an error rather than any cause. - The weekly rate limit posts exactly this body, its cause written only to the task log, so it + The weekly rate limit posts exactly this body, its cause written only to the job log, so it reads as a possible quota hit rather than as a transient failure. A re-request into a reached - limit spends nothing it can recover and returns the same body, which is why one is enough. + limit spends quota it cannot recover and returns the same body, which is why one is enough. """ return bool(ERROR_REFUSAL.search(refusal_of(node))) @@ -1269,10 +1275,11 @@ def run_cause(owner: str, repo: str, oid: str, before: str) -> tuple[str, str] | if proc.returncode != 0: return None log = ANSI.sub("", proc.stdout) - kind, said = RUN_ERROR_TYPE.search(log), RUN_RATE_LIMIT.search(log) - if kind is None: + kinds, said = RUN_ERROR_TYPE.findall(log), RUN_RATE_LIMIT.search(log) + if not kinds: return None - return kind.group(1), said.group(1).strip() if said else "" + kind = "rate_limit" if "rate_limit" in kinds else kinds[-1] + return kind, said.group(1).strip() if said else "" def confirmed_quota(owner: str, repo: str, node: dict) -> str: @@ -1284,7 +1291,7 @@ def confirmed_quota(owner: str, repo: str, node: dict) -> str: ) if cause is None or cause[0] != "rate_limit": return "" - return cause[1] or "the run logged a rate limit and stated no reset" + return cause[1] or "the run logged a rate limit and stated no reset time" def stopping_refusal(pr: dict) -> dict | None: @@ -3074,7 +3081,7 @@ def digest( # Spent where coverage of the same head landed, the precedence the exit codes already hold. # Reported regardless, it prints `review_on_head=yes refusal=YES` over a reviewed head. # That tells a reader to split a pull request the reviewer has just reviewed. - refusal = None if on_head else refusing_review(pr) + refusal = None if on_head else (refusing_review(pr) or stopping_refusal(pr)) # Read once and handed to the line below, since `quota_refusal` re-walks `refusal_of`. refusal_field = ( "no" @@ -3207,9 +3214,9 @@ def digest( # A quota refusal is account-level state nothing here clears, so the caller proceeds on the other reviewers' coverage. # This reads neither cause, only that the round declined. lines.append( - " COPILOT REFUSED THIS ROUND: the review carrying the head says it did " - "not review, so it covers nothing and re-requesting the same head repeats " - "it, and the body below is what says which remedy applies" + " COPILOT REFUSED THIS ROUND: the newest Copilot review on this pull request says " + "it did not review, so it covers nothing and re-requesting repeats it, and the body " + "below is what says which remedy applies" ) lines += [ f" {ln.rstrip()}" for ln in (refusal.get("body") or "").splitlines() if ln.strip() @@ -3742,7 +3749,13 @@ def gh_rest(path: str, jq: str | None = None, raw: bool = False) -> subprocess.C ) try: return subprocess.run( - argv, capture_output=True, text=True, encoding="utf-8", timeout=30, check=False + argv, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace" if raw else "strict", + timeout=30, + check=False, ) except (OSError, subprocess.SubprocessError): return subprocess.CompletedProcess(argv, 1, "", "gh could not be run") @@ -4078,7 +4091,11 @@ def main(argv: list[str] | None = None) -> int: # Read from the same, unfiltered history rather than one that drops this pull request's own entries: a genuine review on an earlier head of this same pull request, superseded since by a push, is real evidence about the account and not a self-reference to discard. # A refusal on this pull request's own current head still never reaches this signal, since it is caught directly and at higher priority first. signal = None if a.ignore_quota_signal else quota_signal(history) - stopped = None if a.ignore_quota_signal else stopping_refusal(pr) + stopped = ( + None + if a.ignore_quota_signal or done or answer or drift + else stopping_refusal(gql(Q_FULL, owner, repo, a.number)) + ) # Request before the first poll, not just at the call site: a caller expects `wait` to make a review happen, not merely to watch for one. # Two prior gaps this closed, a push superseding an already-answered request and an auto-seed that never fired, both left nothing outstanding for the loop below to ever see land. # Skipped once a review already covers the head, once Copilot has already answered outside a formal review, or once something is already in the request set, so a second `wait` on the same PR never double-requests. @@ -4108,7 +4125,7 @@ def main(argv: list[str] | None = None) -> int: "request, no pending reviewer and no review-request event, so this wait stops here " "rather than polling --timeout out against a request that does not exist." ) - elif stopped: + elif stopped and not reviewer_requested(pr): print( "note: this pull request's newest Copilot review is a refusal naming the account " "quota or an error, which is what the weekly rate limit posts, so this wait requests " @@ -4224,8 +4241,9 @@ def main(argv: list[str] | None = None) -> int: print( "status=COPILOT_QUOTA_EXHAUSTED the newest Copilot review on this pull request says " "it encountered an error, and the reviewer's own run log names the cause: " - f"{limit}. Do not re-request before then, and proceed on the coverage the other " - "reviewers already gave" + f"{limit}. Do not re-request until the limit resets, then pass " + "--ignore-quota-signal, and proceed meanwhile on the coverage the other reviewers " + "already gave" ) return 46 if refusal and possible_quota(refusal): @@ -4234,7 +4252,8 @@ def main(argv: list[str] | None = None) -> int: "says it encountered an error and did not review, printed above under COPILOT " "REFUSED THIS ROUND, which is the body the weekly rate limit posts. Read it as a " "possible quota hit: do not re-request, proceed on the coverage the other reviewers " - "already gave, and hand the state to the maintainer" + "already gave, and hand the state to the maintainer, who can pass " + "--ignore-quota-signal once the limit is believed to have reset" ) return 46 if refusal: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 10c99708..9b065638 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5342,6 +5342,7 @@ def test_a_review_on_the_final_read_outranks_an_unrecorded_request(self) -> None def test_a_reviewer_pending_on_the_confirming_read_is_polled_for(self) -> None: """An answer lagging the request it reports is corrected by the next read, so the wait polls.""" self.answer( + payload([review(oid=OLD)]), payload([review(oid=OLD)]), payload([review(oid=OLD)], pending=True), payload([review()]), @@ -5544,6 +5545,58 @@ def test_ignore_quota_signal_restores_plain_polling(self) -> None: self.assertNotIn("note: the reviewer's own most recent activity", out) self.assertIn("status=PENDING", out) + def bodyless(self, oid: str) -> dict: + """A liveness payload, whose reviews carry no body, as `Q_LIVE` asks for none.""" + return payload([{k: v for k, v in review(oid=oid).items() if k != "body"}]) + + def test_a_pending_request_past_an_error_round_is_still_polled_for(self) -> None: + """The stop withholds a request, and a review already on its way still lands.""" + self.answer( + payload([review(oid=OLD)], pending=True), + payload([review(oid=OLD, body=ERROR_REFUSED)], pending=True), + payload([review(oid=OLD, body=ERROR_REFUSED), review(rid="PRR_new")]), + ) + calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_called() + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + + def test_ignore_quota_signal_requests_past_the_repo_wide_signal(self) -> None: + self.answer(payload([])) + calls = self.wire_history([hist_review(962, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0", "--ignore-quota-signal"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_status_reports_an_error_round_on_an_earlier_head(self) -> None: + self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("review_on_head=NO", out) + self.assertIn("refusal=ERROR", out) + self.assertIn("COPILOT REFUSED THIS ROUND", out) + + def test_the_run_read_is_bounded_to_the_reviewer_s_failed_runs_before_the_round(self) -> None: + seen: list[str] = [] + + def rest( + path: str, jq: str | None = None, raw: bool = False + ) -> subprocess.CompletedProcess: + seen.append(jq or "") + return subprocess.CompletedProcess([], 1, "", "not read") + + with mock.patch.object(pr_review, "gh_rest", side_effect=rest): + pr_review.run_cause("o", "r", HEAD, LATE) + self.assertIn(f'.path == "{pr_review.COPILOT_RUN_PATH}"', seen[0]) + self.assertIn('.conclusion == "failure"', seen[0]) + self.assertIn(f'.created_at <= "{LATE}"', seen[0]) + + def test_any_rate_limit_error_in_the_log_is_the_cause(self) -> None: + log = "x errorType: 'network',\n" + RATE_LIMIT_LOG + "y errorType: 'internal',\n" + self.run_log(log) + cause = pr_review.run_cause("o", "r", HEAD, LATE) + self.assertEqual("rate_limit", cause[0] if cause else None) + def run_log(self, log: str | None) -> None: """Answer the three reads `run_cause` makes, or fail all three where `log` is None.""" @@ -5563,18 +5616,22 @@ def rest( self.enterContext(mock.patch.object(pr_review, "gh_rest", side_effect=rest)) def test_an_error_round_on_an_earlier_head_stops_the_auto_request(self) -> None: - """The weekly limit does not move with a push, so the next head is not requested into it.""" - self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + """The weekly limit does not move with a push, so the next head is not requested into it. + + The liveness read carries no bodies, as the real query does not, so the stop is read + from the full payload that follows it. + """ + self.answer(self.bodyless(OLD), payload([review(oid=OLD, body=ERROR_REFUSED)])) calls = self.wire_history([hist_review(7, ERROR_REFUSED)]) self.run_log(None) with mock.patch.object(pr_review.time, "sleep") as slept: - self.assertEqual(46, self.cli(["wait", "7"])) + self.assertEqual(46, self.cli(["wait", "7", "--timeout", "0"])) slept.assert_not_called() self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) self.assertIn("status=COPILOT_ERROR_POSSIBLE_QUOTA", self.out.getvalue()) def test_a_rate_limit_in_the_run_log_confirms_the_quota_and_names_the_reset(self) -> None: - self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) + self.answer(self.bodyless(OLD), payload([review(oid=OLD, body=ERROR_REFUSED)])) self.wire_history([hist_review(7, ERROR_REFUSED)]) self.run_log(RATE_LIMIT_LOG) with mock.patch.object(pr_review.time, "sleep"): From df80cd0570d8ca54436873f2770e4db4def6a4e5 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:37:19 -0700 Subject: [PATCH 3/7] Poll a Pending Request Under the Repo-Wide Quota Signal The signal branch skipped the poll even with a request already pending, which also cut the 46 path short. Both quota branches now hold back only the request. The stale-refusal test reads its own pull request's reviews and expects 46, and the docs are reflowed. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/pr-review-conduct/SKILL.md | 6 +- .../.source-digests/pr-review-conduct | 2 +- .../skills/pr-review-conduct/SKILL.md | 6 +- .github/skills/pr-review-conduct/SKILL.md | 6 +- scripts/README.md | 2 +- scripts/pr_review.py | 77 +++++++++---------- tests/test_pr_review.py | 25 ++++-- 7 files changed, 68 insertions(+), 56 deletions(-) diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 0ba807d2..5fc00d32 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -65,8 +65,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the loop does clears, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account - state read from the reviewer's activity elsewhere when this head carries none of its own, and exit - `41` holding across several heads with no cause its body names reaches it the slower way. + state read from the reviewer's activity elsewhere when this head carries none of its own, and + exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy this item on the exact state it exists to catch. That is where the @@ -156,7 +156,7 @@ must have done. than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + reviewer's run log names a reset time `pr_review.py` reports it. Either refusal goes to the maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index ddc9d6fb..5544d2e4 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -ffa271d490747a42 +8dd1d8770904de15 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 0ba807d2..5fc00d32 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -65,8 +65,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the loop does clears, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account - state read from the reviewer's activity elsewhere when this head carries none of its own, and exit - `41` holding across several heads with no cause its body names reaches it the slower way. + state read from the reviewer's activity elsewhere when this head carries none of its own, and + exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy this item on the exact state it exists to catch. That is where the @@ -156,7 +156,7 @@ must have done. than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + reviewer's run log names a reset time `pr_review.py` reports it. Either refusal goes to the maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 0ba807d2..5fc00d32 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -65,8 +65,8 @@ visible comments, routinely still carries a finding nobody has answered. Treatin the loop does clears, the pull request's newest Copilot review being an account-quota refusal or an error refusal read as a possible quota hit, on this head or an earlier one, which is the case "Which Reviewers a Repository Actually Has" below states. Exit `47` is that same account - state read from the reviewer's activity elsewhere when this head carries none of its own, and exit - `41` holding across several heads with no cause its body names reaches it the slower way. + state read from the reviewer's activity elsewhere when this head carries none of its own, and + exit `41` holding across several heads with no cause its body names reaches it the slower way. Those three are `wait`'s alone: `status` exits 0 over a refusal, carrying it as `refusal=` in the digest line instead, so reading that exit code as the absence of one would falsely satisfy this item on the exact state it exists to catch. That is where the @@ -156,7 +156,7 @@ must have done. than a review, and so is one saying only that Copilot encountered an error, which is what the weekly rate limit posts. Either covers no head, so the gate stays unsatisfied, and nothing the loop does clears it, since re-requesting returns it again and spends quota doing so. Where the - reviewer's run log names a reset time `pr_review.py` reports it. That case goes to the + reviewer's run log names a reset time `pr_review.py` reports it. Either refusal goes to the maintainer, rather than into a wait. Where a reviewer's behavior still surprises you after reading what it posted, the hub's diff --git a/scripts/README.md b/scripts/README.md index 457a75b0..d393bd9b 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. -`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46`, since a request into a reached limit spends what it cannot recover. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46` unless a request already pending lands, since a request into a reached limit spends what it cannot recover. Where nothing covers the head, the digest's `refusal=` field reads that refusal from the earlier head too. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. Over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed and all three merged, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 52105d39..0f8bd95a 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -58,12 +58,13 @@ comparison could not be read, run `status` again. Past those, hand the state to the maintainer rather than retrying into it, since a round re-requested on the same head states coverage or carries a table only by chance. - A refusal naming the account quota still reads as absent here, exit 0, since a - refusal covers no head either. Its printed digest line carries `refusal=QUOTA` - regardless, and `refusal=ERROR` for an error refusal whose run log names no rate - limit or could not be read. Either field reads a refusal on an earlier head where it - is the pull request's newest Copilot review and nothing covers the head. `wait` is where that state gets its own exit codes, 46 and 47 below, - because only `wait` is the command a caller might otherwise poll out a timeout on. + A refusal naming the account quota still reads as absent here, exit 0, since a refusal + covers no head either. Its printed digest line carries `refusal=QUOTA` regardless, and + `refusal=ERROR` for an error refusal whose run log names no rate limit or could not be + read. Either field reads a refusal on an earlier head where it is the pull request's + newest Copilot review and nothing covers the head. `wait` is where that state gets its + own exit codes, 46 and 47 below, because only `wait` is the command a caller might + otherwise poll out a timeout on. `unresolved` counts every tracked reviewer's own open thread, not only Copilot's: CodeRabbit (`coderabbitai`) and qodo (`qodo-free-for-open-source-projects`) are tracked at the identity and thread-resolution level, since an open thread blocks a @@ -166,17 +167,17 @@ wait Request a review where none is outstanding, then poll until Copilot's review lands on the current head, then print the digest. The auto-request is skipped once a review already covers the head, once Copilot has already answered outside a formal - review, or once one is already in the pending request set, so calling `wait` again on - the same PR never double-requests. It is also skipped under 46's and 47's quota - readings below, since a request into a reached limit spends quota and returns nothing. It reads the Copilot reviewer's bot id from the - repository's own most recently updated PRs rather than a fixed id: the last - HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that narrow window carries no - Copilot activity at all, since an outage that outlasts HISTORY_PRS PRs would otherwise - empty it on every call for as long as the outage runs. Requests nothing - (falling back to polling only) where both windows come up empty, since a repository - with no Copilot review in either has nothing to read the id from and a fabricated one - is never an option. The loop runs in-process, so a 45-minute wait costs one agent - turn, not 90. + review, or once one is already in the pending request set, so calling `wait` again on the + same PR never double-requests. It is also skipped under 46's and 47's quota readings + below, since a request into a reached limit spends quota and returns the same refusal. It + reads the Copilot reviewer's bot id from the repository's own most recently updated PRs + rather than a fixed id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that + narrow window carries no Copilot activity at all, since an outage that outlasts + HISTORY_PRS PRs would otherwise empty it on every call for as long as the outage runs. + Requests nothing (falling back to polling only) where both windows come up empty, since a + repository with no Copilot review in either has nothing to read the id from and a + fabricated one is never an option. The loop runs in-process, so a 45-minute wait costs + one agent turn, not 90. Exit 0 = review present, 30 = still pending at timeout (pending is not failure), 40 = Copilot answered outside a formal review, so read the printed body. 40 reports the shape of that answer and reads nothing of its cause: an answer @@ -193,27 +194,25 @@ 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. - 46 = the newest Copilot review on the pull request, on this head or an earlier one, - is a refusal naming the account quota, or one saying only that it encountered an - error, printed above under COPILOT REFUSED THIS ROUND. The weekly rate limit posts - that error body and writes its cause to the reviewer's own Actions run, so that run's - job log is read: a logged rate limit reports as the quota with its reset time, and - anything else, an unreadable log included, as a possible quota hit. Either way no - request is sent, and a request already pending is still polled for. That is an - account-level state a re-request or a further wait does not clear, unlike 41's other - causes (a file count - over the limit, cleared by splitting the pull request), so it is its own code rather - than folded into 41: proceed on the other reviewers' coverage instead of retrying. - 47 = this pull request's current head carries no Copilot activity of its own, and - the reviewer's own most recent activity found in the repository, a review or comment - on any other pull request, is that same account-quota refusal with nothing having - answered it since. A refusal on this pull request itself is 46 instead. The poll that would - otherwise have run is skipped for this reason, printed as a `note:` line before the - digest, rather than spent finding the same account state out a call late, and no - request is sent. Pass --ignore-quota-signal to request and poll --timeout anyway once - the quota is believed to have reset. 46 is read from this pull request's own reviews - and always takes priority over 47, - so a genuine 0/40/41/42/43/45 on this pull request outranks 47 whenever both + 46 = the newest Copilot review on the pull request, on this head or an earlier one, is a + refusal naming the account quota, or one saying only that it encountered an error, + printed above under COPILOT REFUSED THIS ROUND. The weekly rate limit posts that error + body and writes its cause to the reviewer's own Actions run, so that run's job log is + read: a logged rate limit reports as the quota with its reset time, and anything else, an + unreadable log included, as a possible quota hit. Either way no request is sent, and a + request already pending is still polled for. That is an account-level state a re-request + or a further wait does not clear, unlike 41's other causes (a file count over the limit, + cleared by splitting the pull request), so it is its own code rather than folded into 41: + proceed on the other reviewers' coverage instead of retrying. + 47 = this pull request's current head carries no Copilot activity of its own, and the + reviewer's own most recent activity found in the repository, a review or comment on any + other pull request, is that same account-quota refusal with nothing having answered it + since. A refusal on this pull request itself is 46 instead. The poll that would otherwise + have run is skipped for this reason, printed as a `note:` line before the digest, rather + than spent finding the same account state out a call late, and no request is sent. Pass + --ignore-quota-signal to request and poll --timeout anyway once the quota is believed to + have reset. 46 is read from this pull request's own reviews and always takes priority + over 47, so a genuine 0/40/41/42/43/45 on this pull request outranks 47 whenever both would otherwise apply. A pending request remains pending until a review, an answer, or the timeout. GitHub's effort-labeled review lifecycle does not always emit `copilot_work_started`, so that @@ -4132,7 +4131,7 @@ def main(argv: list[str] | None = None) -> int: "nothing and stops here. Pass --ignore-quota-signal to request and poll anyway, " "once the limit is believed to have reset." ) - elif signal: + elif signal and not reviewer_requested(pr): # The poll below is skipped rather than shortened, because there is nothing partial about this signal. # The reviewer's own most recent word anywhere in the repository is the account quota, and nothing has answered it since. # Polling this pull request's own silence for up to 45 minutes would only relearn that same account state a call late. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 9b065638..ee160325 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5504,15 +5504,15 @@ def test_a_genuine_review_on_this_pull_requests_own_earlier_head_clears_the_sign self.assertNotIn("COPILOT_QUOTA_EXHAUSTED_REPO_WIDE", out) self.assertIn("status=PENDING", out) - def test_a_stale_refusal_on_this_pull_requests_own_earlier_head_still_signals(self) -> None: - """This pull request's own older refusal is exactly as much evidence as any other pull - request's, once nothing on the current head answers it directly first.""" - self.answer(payload([])) + def test_a_stale_refusal_on_this_pull_requests_own_earlier_head_is_46(self) -> None: + """This pull request's own older refusal is read from its own reviews, ahead of 47.""" + self.answer(payload([review(oid=OLD, body=QUOTA_REFUSED, at=EARLY)])) self.wire_history([hist_review(7, QUOTA_REFUSED, at=EARLY)]) with mock.patch.object(pr_review.time, "sleep") as slept: - self.assertEqual(47, self.cli(["wait", "7"])) + self.assertEqual(46, self.cli(["wait", "7", "--timeout", "0"])) slept.assert_not_called() - self.assertIn("status=COPILOT_QUOTA_EXHAUSTED_REPO_WIDE", self.out.getvalue()) + self.assertIn("status=COPILOT_QUOTA_EXHAUSTED", self.out.getvalue()) + self.assertNotIn("REPO_WIDE", self.out.getvalue()) def test_a_genuine_review_since_the_refusal_clears_the_signal(self) -> None: """The most recent record settles it: a working round after the refusal spends it.""" @@ -5562,6 +5562,19 @@ def test_a_pending_request_past_an_error_round_is_still_polled_for(self) -> None slept.assert_called() self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_pending_request_past_a_quota_round_is_still_polled_for(self) -> None: + """The repo-wide signal holds back a request too, and a pending one still lands.""" + self.answer( + payload([review(oid=OLD)], pending=True), + payload([review(oid=OLD, body=QUOTA_REFUSED)], pending=True), + payload([review(oid=OLD, body=QUOTA_REFUSED), review(rid="PRR_new")]), + ) + calls = self.wire_history([hist_review(7, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(0, self.cli(["wait", "7"])) + slept.assert_called() + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_ignore_quota_signal_requests_past_the_repo_wide_signal(self) -> None: self.answer(payload([])) calls = self.wire_history([hist_review(962, QUOTA_REFUSED)]) From 592bbd27e3b303fa13fb161c1b8a4fbee9f2866a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:45:02 -0700 Subject: [PATCH 4/7] Read a Job Log on a gh Older Than 2.97 The escape-sequence flag arrived in gh 2.97 and the supported floor is 2.47, so a raw read refused on an unknown flag is retried without it. The pending-poll claim is narrowed to the earlier-head case the stop covers. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 28 +++++++++++++++++----------- tests/test_pr_review.py | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index d393bd9b..a0f86128 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. -`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46` unless a request already pending lands, since a request into a reached limit spends what it cannot recover. Where nothing covers the head, the digest's `refusal=` field reads that refusal from the earlier head too. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46` unless a request already pending after a refusal on an earlier head lands, since a request into a reached limit spends what it cannot recover. Where nothing covers the head, the digest's `refusal=` field reads that refusal from the earlier head too. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. Over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed and all three merged, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 0f8bd95a..91a3c4a2 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -200,10 +200,11 @@ body and writes its cause to the reviewer's own Actions run, so that run's job log is read: a logged rate limit reports as the quota with its reset time, and anything else, an unreadable log included, as a possible quota hit. Either way no request is sent, and a - request already pending is still polled for. That is an account-level state a re-request - or a further wait does not clear, unlike 41's other causes (a file count over the limit, - cleared by splitting the pull request), so it is its own code rather than folded into 41: - proceed on the other reviewers' coverage instead of retrying. + request already pending after a refusal on an earlier head is still polled for. That is + an account-level state a re-request or a further wait does not clear, unlike 41's other + causes (a file count over the limit, cleared by splitting the pull request), so it is its + own code rather than folded into 41: proceed on the other reviewers' coverage instead of + retrying. 47 = this pull request's current head carries no Copilot activity of its own, and the reviewer's own most recent activity found in the repository, a review or comment on any other pull request, is that same account-quota refusal with nothing having answered it @@ -3738,14 +3739,19 @@ def gh_rest(path: str, jq: str | None = None, raw: bool = False) -> subprocess.C Unlike `gh_graphql` this does not raise on a non-zero exit, because a 404 here is an answer the caller acts on rather than a failure. Reads only: every path passed in is a GET. - `raw` is for a job log, which carries terminal escape sequences that `gh` refuses to print - without being told it may. + `raw` is for a job log, which carries terminal escape sequences that `gh` 2.97 and later + refuse to print without `--allow-escape-sequences`. An older `gh` has no such flag and + prints the log as it is, so a run refused on the flag is retried without it. """ - argv = ( - ["gh", "api", path] - + (["--jq", jq] if jq else []) - + (["--allow-escape-sequences"] if raw else []) - ) + base = ["gh", "api", path] + (["--jq", jq] if jq else []) + proc = _gh_run(base + (["--allow-escape-sequences"] if raw else []), raw) + if raw and proc.returncode != 0 and "unknown flag" in proc.stderr: + proc = _gh_run(base, raw) + return proc + + +def _gh_run(argv: list[str], raw: bool) -> subprocess.CompletedProcess: + """Run one `gh` read, a log decoded leniently since its bytes are the runner's own.""" try: return subprocess.run( argv, diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index ee160325..c5d47e7d 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -2967,6 +2967,25 @@ def test_a_round_stating_coverage_on_the_head_carries_nothing(self) -> None: self.assertNotIn("carried", out) +class TestRawLogRead(unittest.TestCase): + """A job log read keeps working on a `gh` older than the escape-sequence flag.""" + + def test_an_unknown_flag_is_retried_without_it(self) -> None: + refused = subprocess.CompletedProcess([], 1, "", "unknown flag: --allow-escape-sequences") + read = subprocess.CompletedProcess([], 0, "the log", "") + with mock.patch.object(pr_review, "_gh_run", side_effect=[refused, read]) as run: + proc = pr_review.gh_rest("repos/o/r/actions/jobs/1/logs", raw=True) + self.assertEqual("the log", proc.stdout) + self.assertIn("--allow-escape-sequences", run.call_args_list[0].args[0]) + self.assertNotIn("--allow-escape-sequences", run.call_args_list[1].args[0]) + + def test_another_failure_is_not_retried(self) -> None: + failed = subprocess.CompletedProcess([], 1, "", "HTTP 404") + with mock.patch.object(pr_review, "_gh_run", return_value=failed) as run: + pr_review.gh_rest("repos/o/r/actions/jobs/1/logs", raw=True) + self.assertEqual(1, run.call_count) + + class TestTheDeltaSinceTheCarriedRound(unittest.TestCase): """What changed between the round that stated coverage and the head, reported not judged.""" From e530f2684bbbc5d3155dd139c8f9de25ad9d455d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:46:56 -0700 Subject: [PATCH 5/7] State the Pending Poll Under 47 and Bind the README Clauses Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_review.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index a0f86128..e4289c5c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -188,7 +188,7 @@ Everything else is decidable and says so. One of the reviewer's own nodes in vie The digest reports the completed head review's effective effort as `lite`, `balanced`, or `max` when its metadata provides one. It reports `effort_source=default` for `Default ()` and `effort_source=explicit` for a bare level. Missing metadata reports `unknown` for both fields. Effort is informational and never changes the coverage or completion verdict. The workflow never selects or changes the user-controlled setting. -`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, and exits `46` unless a request already pending after a refusal on an earlier head lands, since a request into a reached limit spends what it cannot recover. Where nothing covers the head, the digest's `refusal=` field reads that refusal from the earlier head too. Past those two, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. +`wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. One body names no cause at all, "Copilot encountered an error and was unable to review this pull request", which is what the weekly rate limit posts, so `status` and `wait` read the job log of the reviewer's own failed Actions run on that commit: a logged rate limit reports as `refusal=QUOTA` with the reset time it states, and anything else as `refusal=ERROR`, a possible quota hit. `wait` sends no request while the pull request's newest Copilot review is a quota or error refusal, on this head or an earlier one, since a request into a reached limit spends what it cannot recover, and exits `46` unless a request already pending after a refusal on an earlier head lands. Where nothing covers the head, the digest's `refusal=` field reads that refusal from the earlier head too. Past the quota and error refusals, the script reads no cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The `41` reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. `status` and `wait` both exit `42` where the round covering the head read **fewer files than the pull request changed**, or where an earlier round read fewer and the round covering the head states no coverage of its own, and `43` where it states its coverage in a wording this script does not read. Coverage of the head was the only coverage anything checked, and coverage of the diff is a second reading stated in a line nothing parsed: a partial round carries the right `commit.oid`, raises no threads, and reports "generated no comments", so it is the clean pass byte for byte in everything read. Over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed and all three merged, one of them leaving a file of three unread across **both** its rounds. This is the third instance of the shape `refusal` and `suppressed` are the first two, and the only one nothing was reading. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 91a3c4a2..0edbde8b 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -210,11 +210,11 @@ other pull request, is that same account-quota refusal with nothing having answered it since. A refusal on this pull request itself is 46 instead. The poll that would otherwise have run is skipped for this reason, printed as a `note:` line before the digest, rather - than spent finding the same account state out a call late, and no request is sent. Pass - --ignore-quota-signal to request and poll --timeout anyway once the quota is believed to - have reset. 46 is read from this pull request's own reviews and always takes priority - over 47, so a genuine 0/40/41/42/43/45 on this pull request outranks 47 whenever both - would otherwise apply. + than spent finding the same account state out a call late, and no request is sent, while + a request already pending is still polled for. Pass --ignore-quota-signal to request and + poll --timeout anyway once the quota is believed to have reset. 46 is read from this pull + request's own reviews and always takes priority over 47, so a genuine 0/40/41/42/43/45 on + this pull request outranks 47 whenever both would otherwise apply. A pending request remains pending until a review, an answer, or the timeout. GitHub's effort-labeled review lifecycle does not always emit `copilot_work_started`, so that event is not evidence that distinguishes queued work from abandoned work. @@ -3745,7 +3745,7 @@ def gh_rest(path: str, jq: str | None = None, raw: bool = False) -> subprocess.C """ base = ["gh", "api", path] + (["--jq", jq] if jq else []) proc = _gh_run(base + (["--allow-escape-sequences"] if raw else []), raw) - if raw and proc.returncode != 0 and "unknown flag" in proc.stderr: + if raw and proc.returncode != 0 and "unknown flag: --allow-escape-sequences" in proc.stderr: proc = _gh_run(base, raw) return proc From 7edefc147e9179b119d3abbb1ddb74731d0087df Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:52:36 -0700 Subject: [PATCH 6/7] Spend an Earlier-Head Refusal on a Newer Copilot Comment Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 5 ++++- tests/test_pr_review.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 0edbde8b..8dd5b8ec 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1299,8 +1299,11 @@ def stopping_refusal(pr: dict) -> dict | None: Read across heads rather than on the head alone, since a push after such a refusal moves the head and the account state the refusal reports does not move with it. A genuine round since - spends it, the newest review being the one read. + spends it, the newest review being the one read, and so does a plain comment of the + reviewer's since, which `answered_outside_review` already reads as spending one. """ + if answered_outside_review(pr): + return None newest = newest_of(reviewer_nodes(pr, "reviews")) if newest is None or not (quota_refusal(newest) or possible_quota(newest)): return None diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index c5d47e7d..fa6e5b1c 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5601,6 +5601,16 @@ def test_ignore_quota_signal_requests_past_the_repo_wide_signal(self) -> None: self.cli(["wait", "7", "--timeout", "0", "--ignore-quota-signal"]) self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + def test_a_newer_comment_spends_an_earlier_head_refusal(self) -> None: + pr = payload( + [review(oid=OLD, body=ERROR_REFUSED, at=EARLY)], + comments=[comment(at=LATE)], + ) + self.assertIsNone(pr_review.stopping_refusal(pr)) + self.answer(pr) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=no", out) + def test_status_reports_an_error_round_on_an_earlier_head(self) -> None: self.answer(payload([review(oid=OLD, body=ERROR_REFUSED)])) out, _ = pr_review.digest("o", "r", 7) From 3bc4a847826265d029b8dd0a8bf6d4e07f2fdaf6 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Thu, 1 Oct 2026 19:58:20 -0700 Subject: [PATCH 7/7] Let the Override Request Past a Current-Head Refusal The body-less liveness read counts a head refusal as done, so with --ignore-quota-signal the full read is consulted and the round baseline moves past that refusal, the same way --min-rounds holds out for a new round. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 5 +++++ tests/test_pr_review.py | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8dd5b8ec..a21b9401 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -4086,6 +4086,11 @@ def main(argv: list[str] | None = None) -> int: start = time.monotonic() pr = gql(Q_LIVE, owner, repo, a.number) done, answer = head_review_done(pr, a.min_rounds), answered_outside_review(pr) + if done and a.ignore_quota_signal: + full = gql(Q_FULL, owner, repo, a.number) + if refusing_review(full) and not reviewed_head(full): + a.min_rounds = max(a.min_rounds, len(reviewer_nodes(full, "reviews"))) + done = False # A drifted login matches no filter here, so `done` stays false however long this runs. # Waiting it out reports a review that landed as one that never did, at the timeout. # The liveness query carries the authors, so this costs the loop no extra call. diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index fa6e5b1c..617f8313 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5594,6 +5594,19 @@ def test_a_pending_request_past_a_quota_round_is_still_polled_for(self) -> None: slept.assert_called() self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + def test_ignore_quota_signal_requests_past_a_current_head_refusal(self) -> None: + """The body-less liveness read counts a head refusal as done, so the override re-reads it.""" + refused = review(body=QUOTA_REFUSED, at=EARLY) + self.answer( + payload([{k: v for k, v in refused.items() if k != "body"}]), + payload([refused]), + payload([refused, review(at=LATE, rid="PRR_new")]), + ) + calls = self.wire_history([hist_review(7, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--ignore-quota-signal"])) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + def test_ignore_quota_signal_requests_past_the_repo_wide_signal(self) -> None: self.answer(payload([])) calls = self.wire_history([hist_review(962, QUOTA_REFUSED)])