diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 6eff6f10..e14a8e6b 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -66,10 +66,11 @@ 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 - "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. + 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. 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,9 +157,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. 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 `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 4025d7e1..c52b2a67 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -b526db5d9b32632a +e611a6183eb65c72 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 6eff6f10..e14a8e6b 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -66,10 +66,11 @@ 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 - "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. + 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. 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,9 +157,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. 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 `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 6eff6f10..e14a8e6b 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -66,10 +66,11 @@ 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 - "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. + 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. 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,9 +157,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. 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 `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 e7998cee..d3bd8bde 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, 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 9782200c..11df02c2 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -62,10 +62,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. `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 @@ -168,16 +171,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 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 @@ -194,21 +198,27 @@ 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 - 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 - 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, - so a genuine 0/40/41/42/43/45 on this pull request outranks 47 whenever both - would otherwise apply. + 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 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 + 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, 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. @@ -217,8 +227,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. @@ -331,6 +341,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. @@ -1220,6 +1238,82 @@ 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 job log, so it + reads as a possible quota hit rather than as a transient failure. A re-request into a reached + 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))) + + +@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) + kinds, said = RUN_ERROR_TYPE.findall(log), RUN_RATE_LIMIT.search(log) + if not kinds: + return None + 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: + """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 time" + + +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, 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 + return newest + + def refusing_review(pr: dict) -> dict | None: """The reviewer's newest refusal carrying the current head, where one is there. @@ -3057,9 +3151,17 @@ 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" 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. @@ -3182,13 +3284,15 @@ 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() ] + 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( @@ -3707,16 +3811,34 @@ 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` 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 []) + 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: --allow-escape-sequences" 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, 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") @@ -3923,9 +4045,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", @@ -4038,6 +4161,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. @@ -4051,12 +4179,24 @@ 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 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. 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] ) @@ -4073,7 +4213,14 @@ 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 signal: + 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 " + "nothing and stops here. Pass --ignore-quota-signal to request and poll anyway, " + "once the limit is believed to have reset." + ) + 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. @@ -4167,15 +4314,36 @@ 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 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): + 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, who can pass " + "--ignore-quota-signal once the limit is believed to have reset" + ) + 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 9aedf6ea..e8e713ce 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, @@ -2958,6 +2971,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 TestFileTableCarriesForward(CarryCase): """An earlier round's full file table carries to a head whose rounds carry none. @@ -5455,6 +5487,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()]), @@ -5468,19 +5501,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"} @@ -5586,9 +5606,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.""" @@ -5631,15 +5649,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.""" @@ -5672,6 +5690,167 @@ 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_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_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)]) + 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_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) + 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.""" + + 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. + + 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", "--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(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"): + 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."""