diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index cf856d7c..fc8ba252 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -37,6 +37,18 @@ Auto-review on push is configured (via the branch ruleset's `copilot_code_review **A review with no inline comments is still a completed review, not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop, and it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent. +**The one exception is a review that says it did not review, and it is delivered in exactly that shape.** Copilot answers a pull request it will not take on with a formal review, `state: COMMENTED`, carrying the correct `commit.oid` and **zero** inline threads, whose whole body is a refusal: "Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again." Every coverage check passes, the rule above says an empty review is the clean pass, and the two together read a round that never happened as a round that found nothing. Observed on a pull request of 301 changed files, one over the limit, which was one command from merging on it. **The limit is 300 changed files and the remedy is to split the pull request**, since re-requesting the same head repeats the refusal: the file count is what it declined on and re-requesting does not change it. A repository committing binary or generated data alongside code crosses that line easily. Match the refusal on the body's **opening line** rather than anywhere in it, because a review discussing the wording is not one carrying it, and one line rather than two, because a review's first line is its heading and its second is the overview prose where such a description sits. Match an alternation for the same reason the suppressed heading takes one: + +```sh +# A review whose opening line declines the round. That line is the unit, since a refusal is +# the whole body and a match further down is a review quoting the wording rather than refusing. +# The dot spans both spellings of the apostrophe, the typographic one Copilot writes and the +# ASCII one, and it also keeps this filter usable inside single quotes, which neither survives. +gh api repos///pulls//reviews --jq \ + '.[] | select([(.body // "") | split("\n")[] | select(. != "")][0] // "" + | test("wasn.t able to review|was not able to review|unable to review")) | {commit_id, body}' +``` + **Read the low-confidence findings, which are not inline threads.** A review body can carry a collapsed `
` block of findings Copilot withheld from the inline threads, and those findings appear nowhere in `reviewThreads`, so a loop that polls threads alone never sees them and reports a clean pass. **Match the block on more than one phrasing.** Its heading has appeared both as `Suppressed comments (N)` and as "Comments suppressed due to low confidence", so a filter keyed on either one alone silently reports zero suppressed findings on a review that has them, the same false clean this rule exists to prevent, one level up in the detection. **The section moves as well as it is worded, so match the heading wherever it sits.** It has appeared as its own `
` wrapper with a matching ``, as a bare heading in the body, and as a Markdown heading nested inside the `Review details` wrapper, whose `` names the wrapper and not the section. A filter reading a wrapper's `` reports zero on the nested shape, and the count it needs is the heading's own `(N)` rather than the wrapper's. They have been right repeatedly, including a rule stated more broadly than its check enforced and a check that skipped fenced blocks in every rule but one. Read the body of every review, investigate each suppressed finding on the same footing as an inline one, and answer it in the PR conversation, since a suppressed finding has no thread to reply on or resolve. ```sh @@ -141,7 +153,7 @@ gh api repos///issues//comments --jq \ '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' ``` -Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. +Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. The exception is the refusal above, which is a formal review on the head with no inline comments and covers nothing, so path (1) exits 0 over a round that never ran. Read the body of the review the SHA matched, not only the SHA. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. ### Bounded Retry Workflow diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 29c665fa..8e743ab5 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -246,7 +246,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom **Do not merge, and do not enable auto-merge, unless ALL of these hold:** 1. Required status checks are green (`mergeStateStatus: CLEAN`), **and** -2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, **and** +2. A Copilot review is confirmed on the **current head SHA** by matching the review's commit SHA to the head, not an earlier push, because a push makes required checks go green **before** the re-review lands, so a green merge-state can precede the current-head review and never signals readiness on its own, and the matched review is **read** rather than only counted, because Copilot declines a pull request it will not take on with a formal review carrying that same head SHA and no findings, which matches the SHA and covers nothing, **and** 3. **Every** Copilot finding on that head SHA is closed out, with all review threads resolved, **and** any issue-level Copilot comments (which have no resolve action) triaged and replied to, **and** the low-confidence findings collapsed in the review body investigated and answered, since those appear in no thread and a loop that polls threads alone reports a clean pass while they stand, so zero outstanding findings remain, **and** 4. The maintainer has given **explicit** permission to merge. @@ -258,7 +258,7 @@ The repo runs a review loop on every PR: local agent iteration plus remote autom 1. Push changes to the PR branch. 2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (reliable end-to-end, per the runbook). The UI is only a fallback. -3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. +3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head, so proceed. Do not re-trigger it or treat the absence of comments as a missing review. A review whose body says it did not review is the one exception, and it is terminal in the other direction: nothing follows it, re-requesting the same head repeats it, and the body names what has to change first. 4. Triage findings. 5. Apply fixes or write a rationale for declines. 6. Reply to each thread and resolve what was addressed. diff --git a/scripts/README.md b/scripts/README.md index c1c8a9b9..a5272633 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -101,6 +101,8 @@ python3 scripts/pr_review.py wait 452 --repo ptr727/ProjectTemplate --timeout 27 Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script, which holds its no-mutation contract: the digest names the state and the runbook carries the two mutations that clear and re-raise the request. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it. +`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. + The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled. The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index b0faa377..95161235 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -15,6 +15,7 @@ 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 carrying no commit covers no head, so the wait ends and the reader decides. + 41 = the review carrying the head says it did not review, so it covers nothing. 50 = the request is pending and nothing picked it up, which no amount of waiting changes. Recovery is two mutations, and they stay in the runbook. @@ -33,6 +34,14 @@ # The alternation is the runbook's, since the heading wording has changed once already. # Matching one phrasing alone reports zero on a review that has them. SUPPRESSED = re.compile(r'Suppressed comments|low confidence', re.IGNORECASE) +# A refusal declines the round as a formal review carrying the head and no threads. +# That is the clean pass byte for byte, so every coverage check passes over a round that never ran. +# The alternation is the runbook's for the same reason the one above is. +# One phrasing is one rewording away from reading a refusal as a review. +# The dot covers the apostrophe in the typographic spelling and the ASCII one alike. +# It also keeps the published filter usable inside single quotes, which neither survives. +REFUSAL = re.compile(r'wasn.t able to review|was not able to review|unable to review', + re.IGNORECASE) DETAILS = re.compile(r'
(.*?)
', re.DOTALL | re.IGNORECASE) SUMMARY = re.compile(r'(.*?)', re.DOTALL | re.IGNORECASE) TAGS = re.compile(r'', re.IGNORECASE) @@ -197,10 +206,60 @@ def stall_of(owner: str, repo: str, num: int, pr: dict) -> str: return never_picked_up(timeline(owner, repo, num)) +def refusal_of(node: dict) -> str: + """The review's body where its opening line says the reviewer did not review, otherwise empty. + + Read over the opening rather than the whole body, because a refusal replaces the review and + is the only thing the body carries, where a review that merely quotes the wording carries it + below its own overview. This script and its documentation are that quotation, so a body-wide + match would report the pull request adding this check as a refusal, which is the false + positive the suppressed-block matcher already had once. + + The opening is one line rather than two, and the second line is where the cost of widening + it shows: a review's first line is its heading and its second is the overview prose, which + is exactly where a review describing this check states the wording. Reading two lines passed + every case here except that one, which is the case that matters. A refusal introduced by a + heading would sit below the opening and be missed, and answering that shape means telling it + from an overview rather than reading one line further. + """ + body = node.get('body') or '' + opening = next((ln for ln in body.splitlines() if ln.strip()), '') + return body if REFUSAL.search(opening) else '' + + +def refusing_review(pr: dict) -> dict | None: + """The reviewer's newest refusal carrying the current head, where one is there. + + Head-scoped, unlike a suppressed finding, because a refusal is a statement about one commit: + a push retires it, and the round the push raises either reviews that head or refuses it in + its own right. + + A refusal alongside a genuine review of the same head is spent too, and that is the caller's + reading rather than this one's, since what spends it is coverage this cannot see from a + refusal alone. Both callers hold it: `main` returns 0 on `reviewed_head` before reaching + here, and the digest reports the field only where nothing covers the head. + """ + head = pr['headRefOid'] + refusals = [n for n in reviewer_nodes(pr, 'reviews') + if (n.get('commit') or {}).get('oid') == head and refusal_of(n)] + return max(refusals, key=lambda n: n.get('submittedAt') or '') if refusals else None + + def reviewed_head(pr: dict) -> bool: - """True where one of the reviewer's own reviews carries the current head's commit.""" + """True where one of the reviewer's own reviews covers the current head's commit. + + A refusal is not coverage. It is a formal review, `state: COMMENTED`, carrying the head's + commit and raising no threads, so it satisfies every check a clean pass does and renders a + digest identical to one. That is how a pull request of 301 changed files, one over the + reviewer's limit, sat one command from merging on a review that never ran. + + The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is + deliberate rather than a gap: 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. No exit code and no + merge decision is taken from the liveness reading. + """ head = pr['headRefOid'] - return any((n.get('commit') or {}).get('oid') == head + return any((n.get('commit') or {}).get('oid') == head and not refusal_of(n) for n in reviewer_nodes(pr, 'reviews')) @@ -270,7 +329,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, stalled = stall_of(owner, repo, num, pr) if stalled is None else stalled head = pr['headRefOid'] revs = reviewer_nodes(pr, 'reviews') - on_head = [n for n in revs if (n.get('commit') or {}).get('oid') == head] + # A refusal carries the head and covers nothing, so it counts as a round and not as coverage. + # Reading it as coverage prints `review_on_head=yes` over a review that says it did not run. + on_head = [n for n in revs + if (n.get('commit') or {}).get('oid') == head and not refusal_of(n)] threads = pr['reviewThreads']['nodes'] # A deleted account leaves `author` present and null, which `.get('author', {})` returns as # None rather than as the default, so the chained lookup crashes the whole digest. @@ -291,6 +353,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, finding_count(b) for b in on_head_blocks) answer = answered_outside_review(pr) + # 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) blind = [f for f in ('reviews', 'comments') if window_blind(pr, f)] answered = 'yes' if answer else ('unknown' if blind else 'no') lines = [ @@ -298,6 +364,10 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, # A digest of the wrong pull request is well-formed, so naming it is what shows the miss. f'repo={owner}/{repo} pr={num} head={head[:8]} rounds={len(revs)} ' f'review_on_head={"yes" if on_head else "NO"} ' + # A field of its own, since `rounds=1 review_on_head=NO` is also what a stale round is. + # The two want opposite responses, one a re-request and the other a split pull request. + # Upper-case for the reason `NO` is, as a state that blocks a merge is not one to skim. + f'refusal={"YES" if refusal else "no"} ' f'threads={len(threads)} unresolved={len(unresolved)} ' f'suppressed={sum(finding_count(b) for n, b in blocks)} ' f'(on_head={sum(finding_count(b) for b in on_head_blocks)} earlier={stale}) ' @@ -305,6 +375,15 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, f'requested={"yes" if reviewer_requested(pr) else "no"} ' f'merge={pr.get("mergeStateStatus")}' ] + if refusal: + # Printed whole for the reason the comment below is, as the wording carries the remedy. + # A file-count refusal is cleared by splitting the pull request and a quota one by waiting. + # 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') + lines += [f' {ln.rstrip()}' for ln in (refusal.get('body') or '').splitlines() + if ln.strip()] if stalled: lines.append(f' REQUEST NOT PICKED UP (requested {stalled}, no copilot_work_started ' 'since): clear the request and re-request, per the runbook') @@ -424,6 +503,14 @@ def main(argv: list[str] | None = None) -> int: print(f'waited={int(time.monotonic()-start)}s') if reviewed_head(final): return 0 + # 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. + if refusing_review(final): + print('status=REVIEW_IS_A_REFUSAL the review carrying the head says it did not review, ' + 'so it covers nothing and no further review follows it: read the body above, ' + 'since a file-count refusal is cleared by splitting the pull request and a quota ' + 'one by waiting, and re-requesting this head clears neither') + return 41 # An answer before a stall, because the reviewer saying something outranks it saying nothing. if answered_outside_review(final): print('status=ANSWERED_OUTSIDE_REVIEW the reviewer answered without reviewing, ' diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 392f4463..e9758651 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -60,6 +60,11 @@ def nested(heading: str = '### Suppressed comments (2)', '- **Review effort level:** Lite\n
\n') +REFUSED = ("Copilot wasn't able to review this pull request because it exceeds the maximum " + 'number of files (300). Try reducing the number of changed files and requesting a ' + 'review from Copilot again.') + + def thread(tid: str, resolved: bool = False, login: str = pr_review.REVIEWER, body: str = 'A finding.', path: str = 'a.py', line: int = 1) -> dict: return {'id': tid, 'isResolved': resolved, @@ -424,6 +429,106 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No self.assertIn('suppressed=0', out) +class TestRefusal(GqlCase): + """The review that says it did not review, which carries the head and covers nothing. + + It is a formal review with the correct commit and no threads, so every check a clean pass + satisfies it satisfies too, and the digest it renders is the clean pass byte for byte. The + pull request it was observed on had 301 changed files, one over the reviewer's limit, and was + one command from merging on a round that never ran. + """ + + def test_a_refusal_on_the_head_is_not_coverage(self) -> None: + self.answer(payload([review(body=REFUSED)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=NO', out) + self.assertIn('refusal=YES', out) + # It happened, so it is still a round. + # What it is not is a review of anything. + self.assertIn('rounds=1', out) + self.assertFalse(pr_review.reviewed_head(payload([review(body=REFUSED)]))) + + def test_the_body_prints_whole_under_a_marker_naming_the_remedy(self) -> None: + """Its wording is what separates a file-count refusal from a quota one, so it is not cut.""" + self.answer(payload([review(body=REFUSED)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('COPILOT REFUSED THIS ROUND', out) + self.assertIn(REFUSED, out) + + def test_each_documented_phrasing_counts_including_the_typographic_apostrophe(self) -> None: + """One phrasing alone is one rewording away from reporting a refusal as a review.""" + # The typographic apostrophe is an escape, since the charset rule governs this file too. + # The case is about the byte the reviewer sends rather than the character on screen. + for body in ("Copilot wasn't able to review this pull request because it is too large.", + 'Copilot wasn\u2019t able to review this pull request.', + 'Copilot was not able to review this pull request.', + 'Copilot is unable to review this pull request right now.'): + with self.subTest(body=body): + self.assertEqual(body, pr_review.refusal_of({'body': body})) + + def test_a_clean_pass_and_an_ordinary_review_are_not_refusals(self) -> None: + for body in ('Reviewed 3 of 3 changed files and generated no comments.', + collapsed(), nested(), ''): + with self.subTest(body=body[:40]): + self.assertEqual('', pr_review.refusal_of({'body': body})) + + def test_a_review_quoting_the_wording_below_its_overview_is_not_a_refusal(self) -> None: + """This pull request's own review body is that quotation, and the shape has bitten once. + + The suppressed matcher read the whole body and reported the review that discussed + suppressed findings as carrying them. The opening is the unit for that reason: a refusal + is the whole body, so a match further down is a review describing the wording. + + This is also what fixes the unit at one line. The overview prose is the second line of + every review body, and reading two lines reports this review as a refusal of itself. + """ + body = ('## Pull request overview\n\nThis PR treats a review that says Copilot ' + "wasn't able to review a pull request as a terminal state rather than " + 'as coverage.\n\n- The digest now reports `unable to review` separately.\n') + self.assertEqual('', pr_review.refusal_of({'body': body})) + self.answer(payload([review(body=body)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + + def test_a_refusal_from_an_earlier_round_is_spent(self) -> None: + """A refusal is a statement about one commit, so the push that changed it retires it.""" + self.answer(payload([review(oid=OLD, body=REFUSED), review()])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + + def test_a_genuine_review_of_the_same_head_outranks_a_refusal_of_it(self) -> None: + """Coverage that landed is coverage, whatever an earlier round of the same head said. + + The digest has to agree with the exit codes here, which return 0 on that coverage and + never reach 41. Reported unconditionally the field read `review_on_head=yes refusal=YES` + on one line, which tells a reader to split a pull request that has just been reviewed, + and tells an automated one that a state it treats as actionable is outstanding. + """ + pr = payload([review(body=REFUSED, at=EARLY), review(at=LATE)]) + self.assertTrue(pr_review.reviewed_head(pr)) + self.answer(pr) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('review_on_head=yes', out) + self.assertIn('refusal=no', out) + self.assertNotIn('COPILOT REFUSED THIS ROUND', out) + self.assertNotIn(REFUSED, out) + + def test_the_wait_returns_zero_where_coverage_followed_a_refusal_of_the_same_head(self) -> None: + """The digest and the exit code read one payload, so neither may spend it differently.""" + self.answer(payload([review(body=REFUSED, at=EARLY), review(at=LATE)])) + with contextlib.redirect_stdout(io.StringIO()) as out, \ + mock.patch.object(pr_review.time, 'sleep'): + self.assertEqual(0, pr_review.main(['wait', '7', '--repo', 'o/r'])) + self.assertNotIn('status=REVIEW_IS_A_REFUSAL', out.getvalue()) + + def test_a_human_review_carrying_the_wording_is_not_the_reviewer_refusing(self) -> None: + self.answer(payload([review(login='ptr727', body=REFUSED), review(oid=OLD)])) + out, _ = pr_review.digest('o', 'r', 7) + self.assertIn('refusal=no', out) + + class TestDigestReportsTheAnswer(GqlCase): def test_the_comment_prints_whole_under_a_marker_naming_it_terminal(self) -> None: """Its wording is what separates a refusal from a remark, so it is not truncated.""" @@ -527,6 +632,31 @@ def test_wait_ends_on_an_answer_outside_a_review_instead_of_waiting_it_out(self) self.assertIn('status=ANSWERED_OUTSIDE_REVIEW', out) self.assertIn('quota', out) + def test_wait_exits_forty_one_on_a_review_that_says_it_did_not_review(self) -> None: + """Returning zero here is the failure: the digest is the clean pass byte for byte.""" + self.answer(payload([review(body=REFUSED)])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(41, self.cli(['wait', '7', '--timeout', '0'])) + # Terminal, so it ends the wait rather than polling out the timeout against it. + slept.assert_not_called() + out = self.out.getvalue() + self.assertIn('status=REVIEW_IS_A_REFUSAL', out) + self.assertIn('review_on_head=NO', out) + self.assertIn(REFUSED, out) + + def test_the_liveness_reading_ends_the_wait_and_the_full_read_refuses_it(self) -> None: + """The liveness query carries no bodies, so a refusal reads there as ordinary coverage. + + That is what ends the loop, and nothing decides on it: the full read that follows every + wait carries the body and is where the exit code comes from. + """ + bodyless = {k: v for k, v in review().items() if k != 'body'} + self.answer(payload([bodyless]), payload([review(body=REFUSED)])) + with mock.patch.object(pr_review.time, 'sleep') as slept: + self.assertEqual(41, self.cli(['wait', '7', '--timeout', '600'])) + slept.assert_not_called() + self.assertIn('status=REVIEW_IS_A_REFUSAL', self.out.getvalue()) + def test_a_landed_review_wins_over_an_older_answer(self) -> None: """Coverage is the success case, and a spent comment does not downgrade it to 40.""" self.answer(payload([review(at=LATE)], comments=[comment(at=EARLY)])) @@ -681,6 +811,13 @@ def test_the_suppressed_pattern_is_the_runbook_alternation(self) -> None: text = RUNBOOK.read_text(encoding='utf-8') self.assertIn(f'test("{pr_review.SUPPRESSED.pattern}")', text) + def test_the_refusal_pattern_is_the_runbook_alternation(self) -> None: + """A refusal reworded once is a refusal read as coverage, so the pattern is not a memory.""" + text = RUNBOOK.read_text(encoding='utf-8') + self.assertIn(f'test("{pr_review.REFUSAL.pattern}")', text) + # The published filter is single-quoted, which no spelling of the apostrophe survives. + self.assertNotIn("'", pr_review.REFUSAL.pattern) + def test_no_mutation_reaches_this_script(self) -> None: """Mutations stay as explicit `gh` calls so the write-guard hook and review still see them.""" source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8')