diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 90a1a98c..5ebb0e61 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,10 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -48,12 +53,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -62,10 +70,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 @@ -138,23 +147,28 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **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. + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own 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, 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 @@ -178,11 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -192,7 +212,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct index 7916f766..aed1424f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -0331a2ae5b800214 +115e04e054ba1096 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..5ebb0e61 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,10 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -48,12 +53,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -62,10 +70,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 @@ -138,23 +147,28 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **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. + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own 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, 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 @@ -178,11 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -192,7 +212,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b78d5b37..fb2402f9 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -26,9 +26,9 @@ For every review: 4. Use an inline comment when a changed line can anchor the finding. Use the review body only when no valid inline anchor exists. 5. End the review body with the exact machine-readable marker required by the `fleet-code-review` skill. -The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. +The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, `attest`, `comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request conversation. Its status gate verifies the current head, diff coverage, output shape, inline threads, body-only findings, and required checks. -A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. +A formal review with no findings is complete only when it covers the current head and full diff coverage of the change set that head has is stated, or read from a file table as below. The round covering the head states it, or the newest round that states it at all does and the pull request changes the same set of files at both commits, which is the only condition under which a statement carries forward. Only that newest round is consulted, so an older round whose change set does match carries nothing. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. A round reporting partial coverage of the diff blocks the merge, and so does a refusal, a coverage statement that does not reach this head, meaning absent from every round or carried by none because the change set moved or could not be compared, with no file table standing in for it, an unrecognized output shape, an unresolved thread, or a body-only finding. On a pull request into a branch other than the default, a fix push after Copilot's first round is covered by an attested local pass rather than another Copilot round, `review_on_head=local` in the digest, while a pull request into the default branch, a promotion among them, always carries a Copilot round on its head. Re-run the loop after every fix push. Never infer review completion from `mergeStateStatus: CLEAN`. Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default, reading that field as acceptance of the request rather than as delivery of a round. Do not clear the request on that first timeout, because it may still be active. Where a second bounded wait times out as well, read the pending set, clear it only where no human or team reviewer is requested alongside the bot, and rerun `wait`, which then has nothing outstanding to defer to and requests afresh, or polls and says so on its own auto-request line where it finds no reviewer node id to request with. The clear replaces that set rather than adding to it and nothing restores a request it drops, so a stall on a pull request that has a human or team reviewer requested goes to the maintainer, and so does one still pending after the wait that follows a clear. That clear is a recovery step the script does not implement, and `docs/pr-reviewer-reference.md`, in the hub checkout the script is run from, carries it. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 90a1a98c..5ebb0e61 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -1,9 +1,9 @@ --- name: pr-review-conduct description: >- - Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate - fleet repo: requesting a review after a push, triaging findings, replying and resolving threads, - and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, + Governs opening, driving, and merging a pull request review loop in a ptr727/ProjectTemplate fleet + repo: requesting a review at its defined moments, triaging findings, replying and resolving + threads, and deciding whether a PR is actually mergeable. Use this whenever about to open a PR, immediately after creating one, about to merge a PR, enable auto-merge, ask the maintainer for merge permission, push a fix and move on without re-checking review state, or judge a PR "green" or "clean" from CI or mergeStateStatus alone. Triggers even when the request sounds routine, @@ -37,10 +37,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 1. Required status checks are green, and where they are not, the reason is **read**, never inferred. `BLOCKED` covers a failed check, a required check nothing is running, an unresolved thread, and a missing approval alike, and the response differs by cause. -2. A review is confirmed on the **current head SHA**, matched by commit SHA rather than assumed - from a green merge-state. A push makes checks go green *before* the re-review lands, and the - matched review is **read**, not just counted. A review can carry the head SHA and still decline - the PR outright, or say it read only part of the changed files. Where the round covering the +2. A review is confirmed on the **current head SHA**. On a pull request into a branch other than the + default, a head after Copilot's first round can carry an attested local pass instead, + `review_on_head=local` in the digest, where the whole review history is in view and no round on + record states or appears to state partial coverage, and a pull request into the default branch, a + promotion among them, always carries a Copilot round on its head. A review is matched by commit + SHA rather than assumed from a green merge-state. A push makes checks go green *before* the + re-review lands, and the matched review is **read**, not just counted. A review can carry the + head SHA and still decline the PR outright, or say it read only part of the changed files. Where + the round covering the head states no coverage at all, the newest round that does state some stands in for it, and only where the pull request changes the same set of files at both commits, since a statement about a diff this head no longer has says nothing about this one. A head round's own @@ -48,12 +53,15 @@ visible comments, routinely still carries a finding nobody has answered. Treatin both commits. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, `coverage=table` in the digest, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the - head, stated on it or carried to it, wins over that table. The table stands in only where no - Copilot round on the pull request, on any commit, states or appears to state partial - coverage, so a pull request that ever had a partial round goes to the maintainer. The - coverage this item - requires is Copilot's, and CodeRabbit and Qodo are advisory, since the hub's - `docs/pr-reviewer-evaluation.md` "Status" names Copilot the incumbent and says no candidate is + head, stated on it or carried to it, wins over that table. Where no round covering the head + carries a table of its own, the newest round that does, its table naming exactly the changed + files, stands in under the bound a statement carries under, the pull request changing the same + set of files at both commits, and that reading shows as `coverage=carried:table`. The table + stands in only where no Copilot round on the pull request, on any commit, states or appears to + state partial coverage, so a pull request that ever had a partial round goes to the maintainer. + The coverage this item requires is Copilot's, or the attested local pass above, and CodeRabbit + and Qodo are advisory, since the hub's `docs/pr-reviewer-evaluation.md` "Status" names Copilot + the incumbent and says no candidate is a required reviewer: an advisory reviewer's absence blocks nothing, while its findings owe item 3 exactly as Copilot's do. `pr_review.py`'s `review_on_head` names Copilot's own coverage specifically, not "no review of any kind covers this head": an advisory reviewer carrying the @@ -62,10 +70,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 @@ -138,23 +147,28 @@ must have done. automatically, which is not the same as not reviewing at all. Comment the reviewer's documented review command, such as `@coderabbitai review`, and wait for the result as with any other requested review. The agent driving the loop posts that comment itself, on the same standing as - requesting a review after a push. + requesting a Copilot round, and at most once per pull request, on the head the drive judges + final, and never after a rate-limit notice, since the reviewer caps its reviews per pull request + and its absence blocks nothing. - **A notice naming when the reviewer can next run is a rate limit, and asking does not clear it.** It reads like the skip notice above and is the opposite case: the trigger returns the same notice rather than a review, so a loop that keeps asking waits on something no amount of asking - produces. Wait for the time it names, or proceed on the reviewers that did run, since an advisory - reviewer blocks nothing. + produces. Do not ask again on that pull request, and proceed on the reviewers that did run, + since an advisory reviewer blocks nothing. - **Silence is not evidence, and is never read as one on its own.** A reviewer that has posted nothing may not have started yet, may not cover this repository at all, or may have reviewed and had nothing to say, which Merge Gate item 2 describes as its own ordinary shape and which posts no comment to read. Read the reviews themselves rather than the comments alone, since the third case appears only there. - **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. + coverage of the current head, or on a fix push into a branch other than the default an attested + local pass, and the loop's own 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, 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 @@ -178,11 +192,17 @@ Run `local-strict-review` against the branch's current diff before every push th 1. Push changes to the PR branch and open the pull request when it does not exist. 2. Run `scripts/pr_review.py status --repo /` once in the foreground and read its output. -3. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it - explicitly, which step 4's `wait` is what does, though it skips the request where a review - already covers the head, where the answer came outside a formal review, where it detects - drift, and where something is already in the request set, which is the condition the recovery - below clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. +3. Request a Copilot round at the defined moments only: the pull request's first round, and the head + of a pull request into the default branch, a promotion among them. Auto-trigger is unreliable, so + request it explicitly, which step 4's `wait` is what does. On a fix push into any other branch + after the first round, `wait` requests nothing, since that push is covered by the pass the + paragraph above records. Publish it once the push lands by running `scripts/pr_review.py attest + --repo / --checkout `, which refuses unless the checkout is the + pushed head and a recorded pass covers it, and pass `wait --request` where a fix deserves a + Copilot round anyway. `wait` also skips the request where a review already covers the head, where + the answer came outside a formal review, where it detects drift, under a quota or error refusal, + and where something is already in the request set, which is the condition the recovery below + clears. Requesting in the pull request UI is the maintainer's route rather than this loop's. 4. Run a bounded `scripts/pr_review.py wait --repo /` in a background process and read its terminal output. A completed review raising **no findings** is a valid terminal outcome, so do not re-trigger it or read silence as a missing review. A review whose body says it declined to review is the one @@ -192,7 +212,8 @@ Run `local-strict-review` against the branch's current diff before every push th 6. Apply fixes or write a rationale for declines. 7. Reply to each thread, and resolve what was addressed and what was declined on evidence the reviewer could check for itself, per outcome 2 below. -8. Re-run the loop after every fix push until the checks are green and no finding remains open. +8. Re-run the loop after every fix push until the checks are green, the current head is covered, + and no finding remains open. The review effort setting is user-controlled. The workflow never selects or changes it. `status` reports `effort=lite`, `effort=balanced`, or `effort=max` when the completed review exposes that metadata, lowercased, and names an inherited setting apart from a chosen one in a separate `effort_source=default|explicit` field, both reading `unknown` when no effort line parses. Missing effort metadata reports `unknown` and does not change coverage or completion. A pending effort-labeled request can complete without a `copilot_work_started` timeline event, so absence of that event never proves the request is abandoned. The bounded timeout reports `PENDING` when no review or terminal answer arrives. `requested=yes` reports that the request was accepted rather than that a round is coming. An accepted request can sit unpicked, printing the same digest as one about to be served, so a driver reading that field as progress is waiting on evidence it does not hold. After a timeout carrying it, rerun `wait` for another bounded interval by default, because the request may still be active. Where a second bounded wait times out as well, read the pending set, and clear it only where no human or team reviewer is requested alongside the bot, because the clear replaces that set rather than adding to it and nothing restores a request it drops. A stall on a pull request that has a human or team reviewer requested goes to the maintainer instead, and so does one still pending after the wait that follows a clear. The clear leaves the next `wait` nothing outstanding to defer to, so that run requests afresh, and its own auto-request line is what says so, since `wait` reads the reviewer's node id out of the repository's recent reviews and polls without requesting where it finds none. The hub's `docs/pr-reviewer-reference.md` carries the mutation, and an agent seat can run it, where removing and re-adding the reviewer in the pull request UI is a step only the maintainer can take. This recovery replaces only the review request and never changes the effort setting. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. diff --git a/AUDIT.md b/AUDIT.md index bf4fc530..26e65f55 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -223,7 +223,7 @@ flowchart LR Sections 0-9 (the audit and its report) are **read-only** and never touch the target. **Converging** is the separate follow-on phase: the drift the report found is **resolved by applying fixes to the target repo**, not left as a report. The convergence loop: - **Apply via a pull request on the target repo.** Branch from the target's `develop` (or `main` for a `main`-only repo), make the fix, and open a PR. Never push a fix directly to a protected branch, and never hand-edit a target outside a PR. -- **Drive the PR's review to green** - the same loop the hub runs (see [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette], and the [Copilot review runbook][copilot-runbook] in `.github/copilot-instructions.md` for that one reviewer's mechanics): request review on every push, address and resolve every thread from every reviewer the repo has configured, and confirm the review covers the head SHA. +- **Drive the PR's review to green** - the same loop the hub runs (see [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette], and the [Copilot review runbook][copilot-runbook] in `.github/copilot-instructions.md` for that one reviewer's mechanics): request review at the defined moments, address and resolve every thread from every reviewer the repo has configured, and confirm the review covers the head SHA. - **Merge only with explicit maintainer approval.** The agent drives to green and stops. The maintainer merges. - **One focused PR per drift class**, cross-referencing the audit finding. A sprawling all-drifts PR draws many review rounds and never feels done. - **A `hub-only:` finding converges by deleting the file, not by updating it.** That prefix is how `spec/audit.py` reports the **carried-scope** dimension section 4 names. It is the one class where the fix removes content, so it is easy to convert into a re-vendor by reflex and end up refreshing a copy that should not exist. Delete the repo's copy and reach the hub's per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. Confirm the disposition is `retire` before deleting anything: an untriaged hit may be the repo's own content at a shared path, and deleting that destroys work the hub never owned. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 32b2072f..50714e9d 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -212,7 +212,7 @@ The checks that separate work actually done from work that merely reports succes ## PR Review Etiquette -The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review on every push. Confirm it covers the current head SHA and the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green and the current-head review leaves no finding open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. +The provider-agnostic review-loop contract every fleet repo follows starts when a pull request opens. Open every fleet-owned pull request ready for review. Draft state is reserved for the separately documented upstream contribution workflow while a third-party contribution is still being prepared. Creating the pull request is not a terminal handoff. Run the review status once in the foreground. Then start the bounded review wait in a background process. Request a review at defined moments rather than on every push: when the pull request opens, and on the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered instead by the recorded local pass the push already owes, published to the pull request so the review status can read it. Confirm that a review, or for such a fix push that published pass, covers the current head SHA, and that a review covers the full diff rather than only part of it. Where the round covering the head states no coverage at all, read the newest round that does state some as covering this head only where the pull request changes the same set of files at both commits, a head round's own statement always winning over a carried one. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files covers it, which is the reading a Copilot round at Balanced review effort gives, and a statement that reaches the head, stated on it or carried to it, wins over that table. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under the bound a statement carries under, the pull request changing the same set of files at both commits. The table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, so a pull request that ever had a partial round goes to the maintainer. Triage every finding, including low-confidence findings collapsed into the review body rather than threads. Reply to and resolve every addressed finding. Repeat after every fix until the checks are green, the current head is covered, and no finding is left open. Only an explicit maintainer instruction may stop, defer, or alter this default. Silence or a request that says only "open a PR" is not such an instruction. Never merge on a green or CLEAN merge state alone. That state does not prove the review covered the current head SHA and full diff. It also does not expose unanswered low-confidence findings that opened no thread. This is packaged as the `pr-review-conduct` Skill at `.agents/skills/pr-review-conduct/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. The summary above sketches the contract. Read the skill for the merge gate, the expected loop, and how a finding is closed. diff --git a/README.md b/README.md index 164d6f22..ca0f101f 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ This repo is the single home for those rules, a machine-readable spec they are c - **Stand a repository up** - carry the baseline a project is owed for its declared types and workflow model, per [STANDUP.md][standup]. An absent file is a baseline that never arrived rather than drift. - **Audit a repository** - read a live project against the spec and report its drift, per [AUDIT.md][audit]. The audit never edits what it measures, so a fix is a separate change. - **Resync a repository** - bring an already-stood-up project up to the current hub, per [RESYNC.md][resync]. It audits for the findings and then applies them in order, which includes deleting what the hub hosts rather than carries. -- **Close the review loop** - request a review on every push, confirm it covered the head commit, triage every finding, reply and resolve, and escalate when stuck, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette]. +- **Close the review loop** - request a review at the defined moments, confirm the head commit is covered, triage every finding, reply and resolve, and escalate when stuck, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette]. - **Carried against reached** - a project carries the content it is audited against and reaches the machinery that is identical everywhere, per [GOVERNANCE.md "Hub-Hosted Tooling"][governance-hub-hosted-tooling]. ## What It Achieves diff --git a/RESYNC.md b/RESYNC.md index ecc78f83..fbe5f733 100644 --- a/RESYNC.md +++ b/RESYNC.md @@ -118,7 +118,7 @@ The other half is section 4 of [`AUDIT.md`][audit]: no check belonging to a proj - **One focused pull request per drift class**, branched from the target's `develop`, cross-referencing the finding it closes. A sprawling all-drifts pull request draws many review rounds and never feels done. - **Never push a fix directly to a protected branch**, and never hand-edit a target outside a pull request. An operational repository commits to `develop` directly by design, and a conformance change is still a reviewable change. -- **Close the review loop.** Request a review on every push, confirm it covered the head commit, and answer and resolve every thread, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] and the [Copilot review runbook][copilot-runbook]. +- **Close the review loop.** Request a review at the defined moments, confirm the head commit is covered, and answer and resolve every thread, per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] and the [Copilot review runbook][copilot-runbook]. - **The maintainer merges.** The agent drives to green and stops. - **Fix systemic drift in the hub instead.** Where many repositories share a drift, fix the rule or add a check here and let a re-audit re-flag it, rather than hand-patching each repository for a shared cause. diff --git a/TODO.md b/TODO.md index d11a9543..0ddf1504 100644 --- a/TODO.md +++ b/TODO.md @@ -155,7 +155,7 @@ One pull request giving a repo a declared way to say what it needs at runtime, t - **Settled** - Blog needs it immediately, since it deploys on the proxmox host through HomeAutomation-Config's Docker Compose stack and carries the copy destinations and the internal URI. - **Settled** - Adopting it in the hub comes first, since the hub carries neither piece. - **Settled** - The GitHub side has the same missing axis, surfaced by the `hugo` type, since a deploy's credentials are per-environment secrets and variables while `stores` is a closed enum of `actions` and `dependabot`, and [`spec/audit.py`][audit] seeds its map with those two keys and indexes it unguarded, so adding an `environments` value raises a key error for every repo whose publish maps to that mechanism. - - **Settled** - An optional `environments` block is legal in [`spec/secrets.schema.json`][secrets-schema] so a repo may declare its per-environment names, and no tool reads one where it exists, which is honest and is not a gate, so a clean audit says nothing about whether an environment is configured. + - **Settled** - An optional `environments` block is legal in [`spec/secrets.schema.json`][secrets-schema], but it has no per-repo dimension and downstream copies of `spec/secrets.json` are retired, so no repo declares its per-environment names there, and no tool would read one, which is honest and is not a gate, so a clean audit says nothing about whether an environment is configured. ### The Docker Image Freshness Rule diff --git a/docs/pr-reviewer-reference.md b/docs/pr-reviewer-reference.md index 57afc9ba..f07e2d19 100644 --- a/docs/pr-reviewer-reference.md +++ b/docs/pr-reviewer-reference.md @@ -24,7 +24,7 @@ The consequence a review loop actually needs: **a private repository has Copilot The repository already has first-class status, wait, comment, reply, resolution, coverage, and output-shape handling in `scripts/pr_review.py`. -A review is requested by that script rather than by hand. `wait` requests one on the current head where nothing it reads already settles the round and nothing is outstanding, so an accepted request that is never picked up is a state it cannot clear for itself. Clearing the request set is what leaves the next `wait` nothing to defer to. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. Nothing in the commands below is the script, so none of its scope refusals reaches them, and the owner in the target is checked by whoever runs them: +A review is requested by that script rather than by hand. `wait` requests one on the current head where nothing it reads already settles the round, nothing is outstanding, and the head is not a fix push an attested local pass covers, so an accepted request that is never picked up is a state it cannot clear for itself. Clearing the request set is what leaves the next `wait` nothing to defer to. A `wait` ending `REQUEST_NOT_RECORDED`, exit 48, is a different state and takes none of this recovery: the request returned success and left neither a pending reviewer nor a review-request event, which is how an exhausted Copilot allowance has shown itself, so clearing and requesting again does not clear it and it goes to the maintainer. Nothing in the commands below is the script, so none of its scope refusals reaches them, and the owner in the target is checked by whoever runs them: ```sh PR_NODE=$(gh pr view "" --repo "/" --json id --jq '.id') diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index 6b8d611d..87ff96e9 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -68,7 +68,7 @@ The sequencing consequence is that a hub task lands on `develop`, promotes to `m ### Secrets and Permissions -Every hub task declares the secrets it needs by name under `on.workflow_call.secrets`, and a caller maps each one explicitly. Most are `required: true`. A mechanism's secret is `required: false` where the task treats it as one of several opt-in targets, such as `DOCKER_HUB_USERNAME`/`DOCKER_HUB_ACCESS_TOKEN` in `build-release-task.yml`. A package-registry credential is not among them, since `NUGET_USERNAME` and the PyPI OIDC exchange are read by the caller stub's own `publish-nuget` / `publish-pypi` job rather than passed into the task, per [Adopting the Release Chain][adopting-the-release-chain]. The same names are `required: true` in a task built around that one mechanism instead, such as `DOCKER_HUB_USERNAME`/`DOCKER_HUB_ACCESS_TOKEN` in `build-docker-task.yml`. Whether `secrets: inherit` is used is decided by the call's own boundary, not by the fleet's preference. [GitHub documents the keyword][gh-reusing-workflows] for a caller in the same organization or enterprise as the called workflow, and the fleet is a personal account. So a cross-repository call to a hub task names each secret it passes, and `inherit` is never used on one. A call whose job needs none passes no `secrets:` key, which is what the [Adopting the Gates][adopting-the-gates] `validate` stub does. A call by local path stays inside one repository. There the caller's own secret store is the one the called workflow reads, so `inherit` is available. Availability is not a reason to use it, and this repository's own local-path calls name their secrets or pass none. Both shapes run in the fleet today, one repo carrying a local-path `inherit` call beside a cross-repository call that names its secrets, and another proving an inherited value reaches a publishing task that authenticates from it. The declared names are the ones [`spec/secrets.json`][secrets] already declares for the mechanism the task implements, so the secret audit and the workflow agree by construction. An environment-scoped secret is the exception. `DEPLOY_SSH_PRIVATE_KEY` and the `SITE_AUTH_TOKEN_ID`/`SITE_AUTH_TOKEN` pair beside it cross a GitHub Environment boundary `spec/secrets.json` has no vocabulary for, per its `deploy-ssh` mechanism note. +Every hub task declares the secrets it needs by name under `on.workflow_call.secrets`, and a caller maps each one explicitly. Most are `required: true`. A mechanism's secret is `required: false` where the task treats it as one of several opt-in targets, such as `DOCKER_HUB_USERNAME`/`DOCKER_HUB_ACCESS_TOKEN` in `build-release-task.yml`. A package-registry credential is not among them, since `NUGET_USERNAME` and the PyPI OIDC exchange are read by the caller stub's own `publish-nuget` / `publish-pypi` job rather than passed into the task, per [Adopting the Release Chain][adopting-the-release-chain]. The same names are `required: true` in a task built around that one mechanism instead, such as `DOCKER_HUB_USERNAME`/`DOCKER_HUB_ACCESS_TOKEN` in `build-docker-task.yml`. Whether `secrets: inherit` is used is decided by the call's own boundary, not by the fleet's preference. [GitHub documents the keyword][gh-reusing-workflows] for a caller in the same organization or enterprise as the called workflow, and the fleet is a personal account. So a cross-repository call to a hub task names each secret it passes, and `inherit` is never used on one. A call whose job needs none passes no `secrets:` key, which is what the [Adopting the Gates][adopting-the-gates] `validate` stub does. A call by local path stays inside one repository. There the caller's own secret store is the one the called workflow reads, so `inherit` is available. Availability is not a reason to use it, and this repository's own local-path calls name their secrets or pass none. Both shapes run in the fleet today, one repo carrying a local-path `inherit` call beside a cross-repository call that names its secrets, and another proving an inherited value reaches a publishing task that authenticates from it. The declared names are the ones [`spec/secrets.json`][secrets] already declares for the mechanism the task implements, so the secret audit and the workflow agree by construction. An environment-scoped secret is the exception. `spec/secrets.json` declares none of `DEPLOY_SSH_PRIVATE_KEY` and the `SITE_AUTH_TOKEN_ID`/`SITE_AUTH_TOKEN` pair beside it, since each lives in a GitHub Environment rather than the repository store, and no hub audit or configuration tool inventories the secrets an environment holds, per the `deploy-ssh` mechanism note there. The deploy task itself fails its run when one it needs is missing or empty, which is a runtime check rather than an audit. A hub task declares no job-level `permissions:` where every write goes through the App token, and the caller sets `permissions: {}`. A called workflow can only keep or reduce the caller's grant. A callee job naming a scope the caller did not grant fails at startup even when its `if:` is false. Declaring nothing in the callee is therefore the shape that cannot fail against any caller, and it gives `GITHUB_TOKEN` no scope. A task whose job genuinely writes with `GITHUB_TOKEN`, such as a release upload, declares that scope in the callee job and documents it in the stub's comment so the caller grants it. diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index ddd0770f..2ae9e7ed 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -72,6 +72,15 @@ note() { NOTE_TEXTS+=("$2") } +hide_home() { + local path="$1" + if [[ -n ${HOME:-} && ($path == "$HOME" || $path == "$HOME/"*) ]]; then + printf '~%s' "${path#"$HOME"}" + else + printf '%s' "$path" + fi +} + json_string() ( export LC_ALL=C local s="$1" out="" i n b c cp need min j hi lo @@ -1038,7 +1047,7 @@ tool_note() { local resolved resolved=$(ripgrep_download_path) if [[ -n $resolved ]]; then - note "ripgrep" "$resolved is an unowned downloaded copy and an install or upgrade removes it before apt installs Ripgrep" + note "ripgrep" "$(hide_home "$resolved") is an unowned downloaded copy and an install or upgrade removes it before apt installs Ripgrep" fi ;; dotnet) @@ -1060,9 +1069,9 @@ tool_note() { resolved=$(tool_shadow_path "$tool") if [[ -n $resolved ]]; then if [[ -x "$BIN_DIR/$tool" ]]; then - note "$tool" "$resolved comes first on the PATH and shadows the managed copy at $BIN_DIR/$tool" + note "$tool" "$(hide_home "$resolved") comes first on the PATH and shadows the managed copy at $BIN_DIR/$tool" else - note "$tool" "$resolved is installed outside $BIN_DIR and keeps answering once the managed copy is installed" + note "$tool" "$(hide_home "$resolved") is installed outside $BIN_DIR and keeps answering once the managed copy is installed" fi fi ;; @@ -1410,7 +1419,9 @@ resolve_selection() { done for tool in "${MANAGED_TOOLS[@]}"; do for requested in "${REQUESTED[@]}"; do - [[ $tool == "$requested" ]] && SELECTED+=("$tool") + if [[ $tool == "$requested" ]]; then + SELECTED+=("$tool") + fi done done } diff --git a/repo-config/README.md b/repo-config/README.md index 06fe9c09..54253ca5 100644 --- a/repo-config/README.md +++ b/repo-config/README.md @@ -12,9 +12,9 @@ Hub-only repository and branch configuration held as committed files, kept out o Two workflow models share `main.json` but differ on `develop` (registry `workflowModel`, default `release`): - **`release`** (`develop.json`): `develop` requires squash merges with linear history and a PR, the feature-branch pipeline. -- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`; no PR, no status-check, no Copilot-on-push. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). +- **`operational`** (`operational/develop.json`): `develop` takes **direct signed pushes**, carrying only `deletion`, `non_fast_forward`, and `required_signatures`, with no PR, no status-check, and no Copilot review rule. CI runs on the push as advisory feedback. Read the dropped rules as an allowance rather than a prohibition, since a PR into `develop` remains legal and the lint workflow triggers on it, with its result reported and not required (a required check here would gate the direct push as well). This is for live-service config repos that edit `develop` directly and promote a known-good snapshot to `main` via an occasional PR (see [GOVERNANCE.md "Branching Model"][governance-branching-model]). -`main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. +`main` (both models) requires merge-commit merges (no linear-history rule), signed commits, a passing `Check pull request workflow status job`, resolved review threads, and Copilot review, and blocks force-pushes and deletion, so a `develop -> main` promotion is always gated even when `develop` takes direct commits. The Copilot review rule in `develop.json` and `main.json` reviews a pull request when it opens and not on each push or while it is a draft, since a later round is requested deliberately per [GOVERNANCE.md "PR Review Etiquette"][governance-pr-review-etiquette] rather than spent on every push. Every ruleset intentionally leaves "Require branches to be up to date before merging" **off**, per [GOVERNANCE.md "Branching Model"][governance-branching-model]. The result is **exactly two rulesets named `develop` and `main`**, and the names are load-bearing (`GOVERNANCE.md` and the workflows reference them). Only the `develop` *content* varies by model. The required check binds by name and only turns green after the repo's PR workflow runs once. @@ -79,6 +79,7 @@ A repository linked to a project of its own is left alone, like a label the payl [governance-communicating-with-the-user]: ../GOVERNANCE.md#communicating-with-the-user [governance-durable-knowledge]: ../GOVERNANCE.md#durable-knowledge-and-self-improvement [governance-hub-hosted-tooling]: ../GOVERNANCE.md#hub-hosted-tooling +[governance-pr-review-etiquette]: ../GOVERNANCE.md#pr-review-etiquette [governance-verification-discipline]: ../GOVERNANCE.md#verification-discipline [project-json]: ./project.json [repo-config-doc]: ../docs/repo-config.md diff --git a/repo-config/develop.json b/repo-config/develop.json index 89cac061..fcd488c4 100644 --- a/repo-config/develop.json +++ b/repo-config/develop.json @@ -52,8 +52,8 @@ }, { "parameters": { - "review_draft_pull_requests": true, - "review_on_push": true + "review_draft_pull_requests": false, + "review_on_push": false }, "type": "copilot_code_review" } diff --git a/repo-config/main.json b/repo-config/main.json index 6c7b76b6..d51074df 100644 --- a/repo-config/main.json +++ b/repo-config/main.json @@ -49,8 +49,8 @@ }, { "parameters": { - "review_draft_pull_requests": true, - "review_on_push": true + "review_draft_pull_requests": false, + "review_on_push": false }, "type": "copilot_code_review" } diff --git a/scripts/README.md b/scripts/README.md index 4c9e2a1e..13e4ae6c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -172,8 +172,11 @@ python3 scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ --body "Suppressed findings (1): **Disproven** - the target is checked before the write." python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ --match "retry count is off by one" --body "Fixed in abc1234: the loop now stops at n." --resolve +python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout ../worktree ``` +**A Copilot round is requested at defined moments rather than on every push.** The two moments are a pull request's first round and the head of a pull request into the default branch, a promotion among them. On a pull request into any other branch, a fix push after the first round is covered by the recorded local pass the push already owes. That pass lives in the checkout's git directory, where nothing on GitHub can read it, so `attest` publishes it as a comment carrying ``, and only after confirming the checkout is the pushed head, holds no other change, and has a covering pass in `local_review.py status`. `status` then reads that head as `review_on_head=local` with `coverage=local`, where the attestation comes from an owner, member, or collaborator, stands as a line of its own outside a fence, and no round on record states or appears to state partial coverage with the whole history in view. `wait` requests nothing on such a head, ending as covered where it is attested and exiting `49` where it is not, and `--request` asks for a round anyway. A partial on record or a history past the window requests a round instead, since no attestation can clear either, and a request already pending is polled for as before. `attest` also refuses where the checkout's merge base with the base branch is not the pull request's own. The comment carries the findings count the covering pass recorded, which `status` prints beside the reading and nothing gates on, since a local pass's findings are advisory. The rulesets review a pull request when it opens and not on each push, since a trigger on push would spend a round however the tooling chose. A default branch the payload does not name reads as a promotion, so an unreadable field costs a Copilot request rather than passing a head on a local pass alone. + `--repo` is required and carries no default. A default names one repository, and a run from anywhere else resolves its number there instead: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. **Do not infer one field's spelling from another's.** Several states print upper-case, `merge=` reproduces GitHub's own enum verbatim, and a field can arrive behind a prefix, so a pattern built by analogy with a neighboring field silently never fires. `review_on_head` is the one that catches a watcher, rendering `yes` against `NO`, so a pattern written for `YES` waits out its whole bound over a review that landed. A watcher that cannot fire reads exactly like a review that has not landed, so run the command once, read a real line, and write the pattern against that. Anchor the pattern on what is unique to the field being read, meaning that field's own name with its value, and for a pair nested in a parenthesized group the group's name ahead of it, since values repeat across fields and the groups carry the same inner names as each other. A watcher testing for a bare negative matches a line a covered review wrote, so it reports no review over a review that landed and waits out its whole bound saying so. That one matches on the wrong evidence rather than failing to match, which is worse than the casing trap above, since its answer is the digest's own inverted rather than absent. Give the watcher a branch that exits non-zero where the digest cannot be read at all, or a pattern that never fires and a command that never ran are equally silent. @@ -188,11 +191,11 @@ 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. -The reading fails closed, so a coverage-shaped line that parses to no counts is a failure whose remedy is stated as fixing this script rather than reading past it, which is what keeps the vetted spellings honest as the wording drifts, as it has once for each of the other two patterns. The tail of the sentence is deliberately outside the unit: it says how many comments the round raised, which is not coverage, and reading it would fail every merge over a sentence ending. There are two exemptions, and the first is the one that decides the design. **A body stating no coverage at all reads as `unstated`**, never as a pass and never as a failure, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too, and it stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 28 of those 332 bodies are an overview and a change list and nothing more, that shape is current rather than historical and interleaves with the counted one throughout, and one pull request carries both across its two rounds, so failing on it would cry wolf on about one review in twelve and a guard an agent learns to work around is worse than none. **A refusal is exempt** because it states no coverage by design and is already classified, and reading it as a round would grow a spurious second failure on top of the one naming its remedy. +The reading fails closed, so a coverage-shaped line that parses to no counts is a failure whose remedy is stated as fixing this script rather than reading past it, which is what keeps the vetted spellings honest as the wording drifts, as it has once for each of the other two patterns. The tail of the sentence is deliberately outside the unit: it says how many comments the round raised, which is not coverage, and reading it would fail every merge over a sentence ending. There are two exemptions, and the first is the one that decides the design. **A body stating no coverage at all reads as `unstated`**, never as a pass and never as a failure, and where an earlier round on the same pull request states some, that round's reading carries to the head, bounded on the change set: it carries only where the pull request changes the same set of files at both commits, which is the condition measured on this fleet for the reviewer's own marker carrying, and it does not carry where either side's compare could not be read, since a bound that fell open when it could not be measured would be no bound. The delta between the two heads is reported beside the carried reading either way. Where no statement reaches the head either way, a round covering it whose own file table names exactly the changed files reads as `table`, which passes, since Copilot's Balanced review effort, the default since 2026-09-28, writes that table and almost never a statement, and a statement that reaches the head, stated on it or carried to it, wins over it, because the table names the whole changed set on partial rounds too. Where no round covering the head carries a table of its own, the newest round that does, its table naming exactly the changed files, stands in under that same change-set bound, reading as `carried:table`. Either table stands in only where no Copilot round on the pull request, on any commit, states or appears to state partial coverage, quoted or not, so a pull request that ever had a partial round goes to the maintainer. Past that, exit `45` names three states rather than one, and its own message says which the run reached: a pull request no round ever stated coverage on, one whose change set has moved since the round that did, and one where that comparison could not be read: 28 of those 332 bodies are an overview and a change list and nothing more, that shape is current rather than historical and interleaves with the counted one throughout, and one pull request carries both across its two rounds, so failing on it would cry wolf on about one review in twelve and a guard an agent learns to work around is worse than none. **A refusal is exempt** because it states no coverage by design and is already classified, and reading it as a round would grow a spurious second failure on top of the one naming its remedy. The line is matched at its **start** rather than anywhere in the body, since both spellings are structural: across those bodies every coverage statement opens its line, 272 with the reviewer's own name and 32 as the `Review details` bullet, and none sits mid-sentence. A body-wide match reports the pull request that adds this check as a partial round, which is the false positive the suppressed matcher and the refusal matcher have each had once already, and fenced blocks are dropped for the same reason, 131 of the bodies carrying one and this change putting both spellings into the diff a review of it quotes. The cost is named rather than hidden: a wording that moves the statement off the line start reads as no statement rather than as one this cannot parse. The reading was **head-scoped**, unlike a suppressed finding and like a refusal, on the reasoning that a partial round describes one commit's diff and the push that changes that diff raises a round reading the whole of the new one. The carry is what qualifies that: five of ten rounds in the second format raise no statement at all, so the push often raises a round that reads nothing back, and a partial carries to the head under the same change-set bound a full one does, which is a 42 decided by a round that is not on the head. Where one head carries two rounds through a re-request the worst of them still reports, since the one naming files it did not read is the one to answer. A case reads the vetted spellings out of the runbook and hands them to this script's own parser, so the pair stays in step in both directions. diff --git a/scripts/local_review.py b/scripts/local_review.py index 135ac404..0a6c96a4 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -919,6 +919,7 @@ def cmd_status(args: argparse.Namespace) -> int: "changedPaths": changed, "covered": bool(passes), "reviewers": [p["reviewer"] for p in passes], + "findings": {p["reviewer"]: p.get("findings") for p in passes}, "receiptProblems": problems, }, indent=2, diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 8d676bb6..ee520ed3 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -8,6 +8,18 @@ Discipline" for the rule this implements. Subcommands + attest Publish that a recorded local pass covers the pull request's head, as a comment carrying + `` that `status` and + `wait` read, an attestation counting only against the base it names. It reads the receipt + `local_review.py` keeps in the checkout's git directory, which nothing on GitHub can, and + refuses unless the checkout (--checkout, default the current directory) is at the pull + request's head with no change beyond it, its merge base with the base branch is the pull + request's own, and `local_review.py status` reports a covering pass against the base, + read once for the coverage and the findings count alike. Exit 0 = posted, 64 = the write + scope could not be established or excludes the target, 65 = the pull request could not be + read, 66 = the response did not confirm the comment, 67 = the checkout is not the head, + holds changes, or measures another merge base, or that merge base could not be read, 68 = + no current local pass covers the content. comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR node id is read in the same run, and the returned comment URL and body confirm the write. Exit 0 = done, 64 = write scope could not be established or excludes the @@ -21,7 +33,9 @@ Exit 0 = no Copilot review covers the head yet, or every output shape is recognized and full diff coverage is stated for this head, by the round covering it or by the carry below, or where nothing states or carries any, a round covering the head names - exactly the changed files in its own file table, `coverage=table` in the digest. + exactly the changed files in its own file table, `coverage=table` in the digest, or + where no round covering the head carries a table, the newest round that does names + them under the same change-set bound, `coverage=carried:table`. `review_on_head` and `rounds=` in the digest name Copilot's own coverage specifically, the reviewer this script requests and waits for, never "no review of any kind covers this head": a @@ -29,6 +43,10 @@ commit while Copilot's own `review_on_head` still reads `NO`, and an empty body from that other reviewer on that head is its own ordinary "reviewed, nothing to flag" shape, the same reading an empty-bodied Copilot round already gets, not a gap. + `review_on_head=local` reads a head no Copilot round covers on a pull request into a + branch other than the default, after Copilot's first round, where an attestation vouches + for it and no round on record states or appears to state partial coverage, with + `coverage=local` beside it. Use `wait` when review presence is the condition, since `status` reports an absent review without treating it as a failure. 42 = a round read fewer files than the pull request changed, so part of the diff @@ -47,8 +65,10 @@ set, so this covers three states: nothing ever stated coverage, the round that did describes a different set of changed files, or that comparison could not be read. Past those, a round covering the head whose own table names exactly the changed - files stands in, and only where no Copilot round on the pull request, on any commit, - states or appears to state partial coverage, quoted or not, and the whole review + files stands in, or where none carries a table, the newest round that does, its table + naming exactly the changed files, under the same change-set bound. Either table + stands in only where no Copilot round on the pull request, on any commit, states or + appears to state partial coverage, quoted or not, and the whole review history is in view, since over the rounds measured the table names the whole changed set on partial rounds too. The digest names why no table stood in. Copilot's Balanced review effort, the default since 2026-09-28, writes the table and @@ -58,10 +78,14 @@ 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 the pull request's newest Copilot review where it is a quota or + error refusal, on an earlier head or over a head a genuine round covered before it, and a + file-count refusal is spent by coverage of the same 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 @@ -161,20 +185,28 @@ done, 60 = no thread matched, 61 = more than one did, 62 = the reply returned no comment url so nothing was resolved, 63 = the resolve did not report the thread resolved, 64 = the write scope could not be established or excludes the target. - 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 + 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 the same refusal. It is skipped too + on a pull request into a branch other than the default once Copilot has reviewed it at + all, since a fix push there is covered by an attested local pass: an attested head ends + the wait as covered, and one with no attestation exits 49 naming the `attest` step. + --request asks for a round anyway. A pull request into the default branch, a promotion + among them, a pull request Copilot has not reviewed yet, and one with a partial on record + or a review history past the window are requested as before. The comment also carries the + findings count the pass recorded, shown and not gated. 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), + 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, or on a held head an attested local pass, 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 carrying no commit covers no head, so the wait ends and the reader decides. @@ -190,21 +222,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. @@ -213,9 +251,13 @@ 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. + 49 = no Copilot round covers this head and none was requested, the pull request merging + into a branch other than the default after Copilot's first round, and no attestation + vouches for the head. Run the local strict review, record it, push, and run `attest`, or + pass --request. 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. @@ -327,6 +369,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. @@ -479,9 +529,10 @@ def strip_fences( # A round that did state full coverage settles the question over one that stated nothing. UNVETTED, PARTIAL, FULL, UNSTATED = "unvetted", "partial", "full", "unstated" # Not a state a round reports, but the reading that carries an earlier round's forward. -# It ranks nowhere in `SEVERITY`, since it is one of the four wearing another round's name. CARRIED = "carried" TABLE = "table" +LOCAL = "local" +NO_HEAD_TABLE = "no round covering the head carries a file table of its own" SEVERITY = (UNVETTED, PARTIAL, FULL, UNSTATED) # Upper-case for the two that block a merge, for the reason `review_on_head=NO` is upper-case. # `unstated` rather than `unknown`, since a body carrying no count is a shape this knows. @@ -493,6 +544,7 @@ def strip_fences( FULL: "full", UNSTATED: "unstated", TABLE: "table", + LOCAL: "local", } # Every structural marker the reviewer's own bodies carry, measured over the same 333. @@ -741,10 +793,11 @@ def strip_fences( query($o:String!,$r:String!,$n:Int!){ repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName mergeable mergeStateStatus + baseRepository{ defaultBranchRef{ name } } reviews(last:100){ nodes{ id author{login} state commit{oid} submittedAt body } pageInfo{ hasPreviousPage } } reviewThreads(first:100){ nodes{ id isResolved comments(first:1){ nodes{ author{login} path line body pullRequestReview{ id } } } } pageInfo{ hasNextPage } } - comments(last:100){ nodes{ author{login} createdAt body } pageInfo{ hasPreviousPage } } + comments(last:100){ nodes{ author{login} authorAssociation createdAt body } pageInfo{ hasPreviousPage } } reviewRequests(first:10){ nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } files(first:__FILES_WINDOW__){ pageInfo{ hasNextPage } nodes{ path } } commits(last:1){ nodes{ commit{ oid statusCheckRollup{ state @@ -814,6 +867,17 @@ def strip_fences( repository(owner:$o,name:$r){ pullRequest(number:$n){ id url } }} """ +Q_ATTEST_TARGET = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ headRefOid baseRefName } }} +""" +ATTESTATION = re.compile( + r"^$", + re.MULTILINE, +) +TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +LOCAL_REVIEW = Path(__file__).resolve().parent / "local_review.py" + # The conversation-comment and thread mutations the runbook publishes. # `url` is fetched because it is the one field that confirms a comment or reply carried a body. # A reply that posted empty still returns a comment, and three did, each then resolved. @@ -1216,6 +1280,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. @@ -2039,14 +2179,19 @@ def partial_shaped(pr: dict) -> str: return "" -def table_shortfall(pr: dict) -> str: +def table_shortfall(pr: dict, named: list[str] | None = None) -> str: """Why no round covering the head names exactly this pull request's changed files, or "". - Empty where the table stands in, which is the coverage reading `table_covers` gives. + Empty where the table stands in, which is the coverage reading `table_reading` gives. Otherwise the reason, which the digest prints under an unstated head, since the remedies differ: a missing or mismatched table may be answered by another round, a changed-file list longer than the window this reads by a push bringing it back within it, and a partial on record by nothing but the maintainer's reading. + + `named` is an earlier round's table where `table_reading` matches a carried one, and the + head's own table otherwise. Every other check reads the pull request either way, since a + partial on record or a cut-short list refuses a carried table exactly as it refuses one on + the head. """ if reviews_truncated(pr): return ( @@ -2058,9 +2203,10 @@ def table_shortfall(pr: dict) -> str: f"a Copilot round on this pull request states or appears to state partial coverage, " f"'{partial}', so the maintainer reads it rather than the table" ) - named = head_table(pr) + if named is None: + named = head_table(pr) if not named: - return "no round covering the head carries a file table of its own" + return NO_HEAD_TABLE files = pr.get("files") if files is None: return "the changed-file list is absent from the query, so the table has nothing to match" @@ -2101,16 +2247,35 @@ def table_shortfall(pr: dict) -> str: ) -def table_covers(pr: dict) -> bool: - """Whether a round covering the head names exactly this pull request's changed files in its table. +def carried_table(pr: dict) -> tuple[list[str], str] | None: + """The newest round on this pull request carrying a file table, its table, and its commit. + + None where no round carries one. A refusal is skipped for the reason `carried_coverage` + skips it, being a round that read nothing. + """ + tabled = [ + node + for node in reviewer_nodes(pr, "reviews") + if not refusal_of(node) and file_table(node.get("body") or "") + ] + newest = newest_of(tabled) + if newest is None: + return None + return file_table(newest.get("body") or ""), (newest.get("commit") or {}).get("oid") or "" + + +def table_reading(owner: str, repo: str, pr: dict) -> tuple[str, str]: + """Whether a file table stands in for this head's coverage, as the commit it was read on and why not. - The reason it does not is `table_shortfall`, which this is the empty case of. + The shortfall is empty where a table stands in. The commit names the earlier round's commit + where one was consulted and names one, whether or not its table carries, and is empty + otherwise. - The coverage reading for a head that no round states coverage on and no earlier statement - carries to. Copilot's Balanced review effort, the default since 2026-09-28, writes the second - overview format with a file table and almost never a coverage statement, whatever the review - instructions ask, so without this reading no pull request reviewed at that effort could close - its loop. + This is the coverage reading for a head that no round states coverage on and no earlier + statement carries to. Copilot's Balanced review effort, the default since 2026-09-28, writes + the second overview format with a file table and almost never a coverage statement, whatever + the review instructions ask, so without this reading no pull request reviewed at that effort + could close its loop. It is weaker than a statement, and `table_against_diff` says why: over the rounds measured, the table names the whole changed set on partial rounds as well as full ones. So it stands @@ -2121,8 +2286,42 @@ def table_covers(pr: dict) -> bool: a carry bound refusing, failing, or never reaching it cannot settle. A table naming a file the diff does not carry, or leaving one out, is not this reading either, and neither is a changed-file list the query cut short or returned malformed. + + A table carries under the bound a statement does, the pull request changing exactly the same + set of files at both commits, which `carry_holds` reads. Three pull requests in one session + each had a full table on their first round and a re-review on the next head carrying no + table, with the file set unchanged between the two, and the maintainer accepted the first + round's table every time. Only the newest round carrying a table is consulted, and only where + no round covering the head carries one of its own, since a head table that misses the diff is + the newer reading and an older one matching does not overrule it. """ - return not table_shortfall(pr) + shortfall = table_shortfall(pr) + if shortfall != NO_HEAD_TABLE: + return "", shortfall + earlier = carried_table(pr) + if earlier is None: + return "", f"{NO_HEAD_TABLE}, and no earlier round carries one either" + named, carried_from = earlier + if not carried_from: + return "", ( + f"{NO_HEAD_TABLE}, and the newest round that does names no commit, so there is no " + f"change set of its own to compare" + ) + whose = f"{NO_HEAD_TABLE}, while the newest round that does is on {carried_from[:8]}" + if why := table_shortfall(pr, named): + return carried_from, f"{whose}, where {why}" + kept = carry_holds(owner, repo, pr, carried_from) + if kept is None: + return carried_from, ( + f"{whose}, and the change set could not be read at both commits, so its table is " + f"not carried" + ) + if not kept: + return carried_from, ( + f"{whose}, and the pull request changes a different set of files at the two " + f"commits, so its table describes a diff this head no longer has" + ) + return carried_from, "" def badge_text(match: re.Match[str]) -> str: @@ -2297,7 +2496,7 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: carried = None if carried is not None: state, line, _from_head = carried - if state == UNSTATED and table_covers(pr): + if state == UNSTATED and not table_reading(owner, repo, pr)[1]: state = TABLE if state == PARTIAL: # The unread count comes from the line that decided PARTIAL, never from past rounds. @@ -2345,8 +2544,9 @@ def report_verdict(pr: dict, owner: str, repo: str) -> int: "states none is the ordinary shape of the second overview format and carries the " "newest round that states some forward, bounded on the change set, and failing " "that, a round covering the head whose own file table names exactly the changed " - "files stands in, where no round on the pull request states or appears to state " - "partial coverage. Reaching here means no table stood in, for the reason the digest " + "files stands in, or where none carries a table, the newest round that does, its " + "table naming exactly the changed files, under the same change-set bound, where no " + "round on the pull request states or appears to state partial coverage. Reaching here means no table stood in, for the reason the digest " "above names, and also one of three things it says which of: no round ever stated " "coverage, the round that did describes a different set of changed files than this " "head has, or that comparison could not be read. Confirm the head branch carries " @@ -2934,8 +3134,14 @@ def digest( cover, cover_line, _carried_from = candidate else: carried = None - if cover == UNSTATED and on_head and table_covers(pr): + table_from, table_short = ( + table_reading(owner, repo, pr) if cover == UNSTATED and on_head else ("", "") + ) + if cover == UNSTATED and on_head and not table_short: cover = TABLE + local = not on_head and local_cover(pr) + if local: + cover = LOCAL unknown = unrecognized_shapes(pr) threads = pr["reviewThreads"]["nodes"] # True where the connection cut off before this pull request's actual thread count. @@ -2987,12 +3193,17 @@ def digest( unlisted = unlisted_findings(manifest) 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) + refusal = stopping_refusal(pr) or (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. @@ -3035,7 +3246,7 @@ def digest( # The repository leads the line, since a number alone reads as correct anywhere. # 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'} " + f"review_on_head={'yes' if on_head else 'local' if local else 'NO'} " # Present only where at least one other tracked reviewer has posted on this exact head. # No verdict rides on it, unlike `review_on_head`, since nothing here reads what a CodeRabbit or qodo round said, only that one landed. + (f"other_reviewed={','.join(other_on_head)} " if other_on_head else "") @@ -3051,7 +3262,7 @@ def digest( # Those two readings are what `review_on_head=yes` alone conflates. # `carried:` prefixes the state rather than replacing it. # What the earlier round said is the reading, and which round said it is the provenance. - f"coverage={CARRIED + ':' if carried else ''}{COVERAGE_FIELD[cover]} " + f"coverage={CARRIED + ':' if carried or (cover == TABLE and table_from) else ''}{COVERAGE_FIELD[cover]} " # Every other field on this line is a reading of the review. # This one says whether the readings can be believed at all, so it is not a count. f"shapes={'UNRECOGNIZED' if unknown else 'ok'} " @@ -3115,13 +3326,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( @@ -3176,14 +3389,29 @@ def digest( f"{carried_from[:8] or 'a commit this cannot name'}, and {carried_since}, but {why}" ) if cover == TABLE: + count = len(changed_paths(pr)[0]) + files = f"{count} changed file{'' if count == 1 else 's'}" lines.append( - f" COVERAGE IS READ FROM THE FILE TABLE: no round states coverage of this head and " - f"none carries to it, and a round covering it names exactly the " - f"{len(changed_paths(pr)[0])} changed " - f"file{'' if len(changed_paths(pr)[0]) == 1 else 's'} in its own table" + " COVERAGE IS READ FROM THE FILE TABLE: no round states coverage of this head and " + "none carries to it, and " + + ( + f"no round covering it carries a file table, so the newest round that does, on " + f"{table_from[:8]}, carries its table, which names exactly the {files}, the " + f"pull request changing that same set at both commits, and " + f"{delta_since(owner, repo, table_from, head)}" + if table_from + else f"a round covering it names exactly the {files} in its own table" + ) ) elif cover == UNSTATED and on_head: - lines.append(f" NO FILE TABLE STANDS IN: {table_shortfall(pr)}") + lines.append(f" NO FILE TABLE STANDS IN: {table_short}") + elif cover == LOCAL: + lines.append( + " COVERAGE IS READ FROM THE LOCAL PASS: no Copilot round covers this head, which " + "follows the pull request's first round, and a comment from an owner, member, or " + "collaborator attests a recorded local strict-review pass over exactly this head's " + f"content, which recorded {attestation(pr)} finding(s)" + ) if cover == PARTIAL: # The line prints under the marker for the reason a suppressed block does. # The counts say how much of the diff went unread, and no thread carries them. @@ -3552,6 +3780,237 @@ def comment_on_pr(owner: str, repo: str, num: int, body: str) -> int: return 0 +def attest(owner: str, repo: str, num: int, checkout: str) -> int: + """Publish that a recorded local pass covers this pull request's head. Returns an exit code. + + The receipt `local_review.py` records lives in the checkout's git directory, so nothing on + GitHub can read it, and the review gate for a fix push needs to. This reads the receipt where it + lives and posts a comment the gate can read, and only after four checks, each of which would + otherwise vouch for content the pull request does not carry: the checkout's HEAD is the pull + request's head, the checkout holds no change beyond that commit, its merge base with the base + branch is the pull request's own, and `local_review.py status` reports a current pass against + the pull request's base, one read answering the coverage and the findings count alike. The first + two are read again once the check returns, since the checkout can move while it runs. + """ + ok, why = in_scope(owner) + if not ok: + print(f"status=OUT_OF_SCOPE nothing was written: {why}") + return 64 + target = gql(Q_ATTEST_TARGET, owner, repo, num) or {} + head, base = target.get("headRefOid") or "", target.get("baseRefName") or "" + if not head or not base: + print( + f"status=TARGET_NOT_READ nothing was written: {owner}/{repo} #{num} did not return " + "its head commit and base branch" + ) + return 65 + local = _git(checkout, "rev-parse", "HEAD") + dirty = _git( + checkout, "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=none" + ) + if local.returncode != 0 or dirty.returncode != 0: + print( + f"status=CHECKOUT_NOT_READ nothing was written: {checkout} is not a readable checkout" + ) + return 67 + if local.stdout.strip() != head: + print( + f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout is at " + f"{local.stdout.strip()[:8]} and the pull request's head is {head[:8]}, so push or " + "fetch until they agree" + ) + return 67 + if dirty.stdout.strip(): + print( + "status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout holds changes the " + "head commit does not, so a pass over it does not describe what was pushed" + ) + return 67 + if UNSAFE_REF.search(base) or DOT_SEGMENT.search(base): + print( + f"status=MERGE_BASE_NOT_READ nothing was written: the base branch {base!r} carries a " + "character that would change the compare path rather than travel along it" + ) + return 67 + ours = _git(checkout, "merge-base", f"refs/remotes/origin/{base}", "HEAD") + theirs = gh_rest( + f"repos/{owner}/{repo}/compare/{base}...{head}", ".merge_base_commit.sha // empty" + ) + if ours.returncode != 0 or theirs.returncode != 0 or not theirs.stdout.strip(): + print( + f"status=MERGE_BASE_NOT_READ nothing was written: the merge base with {base} could " + "not be read in the checkout or from GitHub, so the pass's scope cannot be compared " + "with the pull request's" + ) + print(f" {(ours.stderr or theirs.stderr).strip()[:400]}") + return 67 + if ours.stdout.strip() != theirs.stdout.strip(): + print( + f"status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout's merge base with " + f"origin/{base} is not the pull request's own, so a pass against it measured a " + "different change set. Fetch, or attest from a checkout of the pull request's own " + "repository" + ) + return 67 + covered, findings, said = receipt_reading(checkout, base) + if not covered: + print( + f"status=NO_LOCAL_PASS nothing was written: `local_review.py status --target {base}` " + "reports no recorded pass covering this content. Run the local strict review, " + "record it, and attest again" + ) + print(f" {said.strip()[:400]}") + return 68 + after = _git(checkout, "rev-parse", "HEAD") + still = _git( + checkout, "status", "--porcelain", "--untracked-files=all", "--ignore-submodules=none" + ) + if after.stdout.strip() != head or still.returncode != 0 or still.stdout.strip(): + print( + "status=CHECKOUT_NOT_THE_HEAD nothing was written: the checkout moved or changed " + "while the pass was being checked, so the check no longer describes the head" + ) + return 67 + body = ( + f"A recorded local strict-review pass covers head `{head}`, the content this pull " + f"request carries at that commit against `{base}`, and it recorded {findings} " + f"finding{'' if findings == '1' else 's'}.\n\n" + f"" + ) + return comment_on_pr(owner, repo, num, body) + + +def receipt_reading(checkout: str, base: str) -> tuple[bool, str, str]: + """Whether a recorded pass covers the checkout's content, its findings, and what was read. + + One `local_review.py status` read answers both, so the coverage and the count describe the + same receipt, where a check followed by a second read could straddle a record replacing it. + The findings are the covering passes' counts summed, or "unknown" where any recorded none, + and they are shown rather than gated, since a local pass's findings are advisory. + """ + try: + proc = subprocess.run( + [sys.executable, str(LOCAL_REVIEW), "status", "--target", base], + cwd=checkout, + capture_output=True, + text=True, + encoding="utf-8", + timeout=120, + check=False, + ) + except (OSError, subprocess.SubprocessError): + return False, "unknown", "local_review.py could not be run" + try: + data = json.loads(proc.stdout) + except ValueError: + return False, "unknown", proc.stdout or proc.stderr + if proc.returncode != 0 or not isinstance(data, dict) or data.get("covered") is not True: + return False, "unknown", proc.stdout or proc.stderr + if data.get("receiptProblems"): + return False, "unknown", proc.stdout + counts = data.get("findings") + if ( + not isinstance(counts, dict) + or not counts + or not all(isinstance(n, int) and n >= 0 for n in counts.values()) + ): + return True, "unknown", proc.stdout + return True, str(sum(counts.values())), proc.stdout + + +def _git(checkout: str, *args: str) -> subprocess.CompletedProcess: + """One read-only git command in `checkout`, returned whole rather than raised.""" + try: + return subprocess.run( + ["git", "-C", checkout, *args], + capture_output=True, + text=True, + encoding="utf-8", + timeout=30, + check=False, + ) + except (OSError, subprocess.SubprocessError): + return subprocess.CompletedProcess([], 1, "", "git could not be run") + + +def attested(pr: dict) -> bool: + """Whether an attestation vouches for this pull request's head, per `attestation`.""" + return attestation(pr) is not None + + +def attestation(pr: dict) -> str | None: + """The findings count an owner's, member's, or collaborator's attestation of this head carries. + + None where no attestation vouches for the head against the pull request's current base, so + a retargeted pull request needs a new one, and "unknown" where the pass recorded no count. + + Read over every comment rather than the reviewer's own, since an attestation is the + maintainer's account speaking. The association is what keeps a passer-by's comment carrying + the same marker from vouching for anything. The marker counts only as a line of its own + outside a fence, so a comment quoting it in a span or a code block vouches for nothing. + """ + head, base = pr.get("headRefOid") or "", pr.get("baseRefName") or "" + for node in (pr.get("comments") or {}).get("nodes") or []: + if (node.get("authorAssociation") or "") not in TRUSTED_ASSOCIATIONS: + continue + body = CODE_SPAN.sub(" ", strip_fences(node.get("body") or "", to_end=True)) + for m in ATTESTATION.finditer(body): + if m.group(1) == head and m.group(2) == base: + return m.group(3) + return None + + +def promotion(pr: dict) -> bool: + """Whether the pull request merges into the repository's default branch. + + That is the promotion, whose own Copilot round is the backstop reading the whole diff, so + the local pass never stands in for it. A default branch the payload does not name reads as + a promotion too, since the failure that way is one more Copilot request rather than a gate + passed on a local pass alone. + """ + default = ((pr.get("baseRepository") or {}).get("defaultBranchRef") or {}).get("name") or "" + return not default or pr.get("baseRefName") == default + + +def first_round_done(pr: dict) -> bool: + """Whether Copilot has reviewed this pull request at all, on any head, a refusal not counting.""" + return any(not refusal_of(n) for n in reviewer_nodes(pr, "reviews")) + + +def holds(pr: dict) -> bool: + """Whether this head is a fix push the local pass covers, so `wait` requests no round for it. + + Read again on the payload the verdict is graded on, since a push between two reads moves the + head the hold was decided for. + """ + return ( + not promotion(pr) + and first_round_done(pr) + and not reviews_truncated(pr) + and not partial_shaped(pr) + and not reviewer_requested(pr) + ) + + +def local_cover(pr: dict) -> bool: + """Whether a recorded local pass stands in for a Copilot round on this head. + + Only on a pull request into a branch other than the default, after Copilot's first round, on + a head its writer attests, and only where nothing on record says any round read part of a + diff and the whole review history is in view, the bound a file table stands in under. A fix + push after the first round is covered by the local pass, and a promotion keeps its own + Copilot round. A refusal on this head is Copilot's own word on it and is never overruled. + """ + return ( + not refusing_review(pr) + and not promotion(pr) + and first_round_done(pr) + and attested(pr) + and not reviews_truncated(pr) + and not partial_shaped(pr) + ) + + def reply_to_thread( owner: str, repo: str, num: int, match: str, body: str, path: str | None, resolve: bool ) -> int: @@ -3632,16 +4091,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") @@ -3811,7 +4288,7 @@ def utf8_console() -> None: def main(argv: list[str] | None = None) -> int: utf8_console() ap = argparse.ArgumentParser() - ap.add_argument("cmd", choices=["claims", "comment", "status", "reply", "wait"]) + ap.add_argument("cmd", choices=["attest", "claims", "comment", "status", "reply", "wait"]) ap.add_argument("number", type=int) # No default, because the wrong repository is the failure this argument has actually had. # A default names one repository, and every run from elsewhere silently reads that one. @@ -3848,9 +4325,16 @@ 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( + "--request", + action="store_true", + help="wait: request a Copilot round even on a fix push into a branch other than the " + "default, which an attested local pass otherwise covers", ) ap.add_argument( "--min-rounds", @@ -3888,6 +4372,12 @@ def main(argv: list[str] | None = None) -> int: action="store_true", help="reply: resolve the thread once the reply is confirmed", ) + ap.add_argument( + "--checkout", + metavar="DIR", + help="attest: the checkout holding the pull request's head and its recorded local pass " + "(default the current directory)", + ) a = ap.parse_args(argv) # Named for the command they belong to, since one silently ignored reads as one that took effect. # A `status` given --body reports a clean digest and writes nothing. @@ -3901,6 +4391,10 @@ def main(argv: list[str] | None = None) -> int: for flag, value in reply_only.items(): if value is not None: ap.error(f"{flag} belongs to `reply`, not `{a.cmd}`") + if a.cmd != "wait" and a.request: + ap.error(f"--request belongs to `wait`, not `{a.cmd}`") + if a.cmd != "attest" and a.checkout is not None: + ap.error(f"--checkout belongs to `attest`, not `{a.cmd}`") if a.cmd not in ("comment", "reply") and a.body is not None: ap.error(f"--body belongs to `comment` or `reply`, not `{a.cmd}`") required = ["--body"] + (["--match"] if a.cmd == "reply" else []) @@ -3937,6 +4431,9 @@ def main(argv: list[str] | None = None) -> int: if a.cmd == "claims": return check_claims(owner, repo, a.number) + if a.cmd == "attest": + return attest(owner, repo, a.number, a.checkout or ".") + if a.cmd == "comment": return comment_on_pr(owner, repo, a.number, a.body) @@ -3963,6 +4460,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. @@ -3976,12 +4478,31 @@ 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) + snapshot = None if done or drift else gql(Q_FULL, owner, repo, a.number) + stopped = ( + None if a.ignore_quota_signal or answer or snapshot is None else stopping_refusal(snapshot) + ) + held = ( + snapshot is not None + and not a.request + and not reviewer_requested(pr) + and holds(snapshot) + and (not answer or attested(snapshot)) + ) # 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 held + and not reviewer_requested(pr) + ): line, recorded = request_copilot_review( owner, repo, a.number, pr["id"], copilot_bot_id(history), delays[0] ) @@ -3998,7 +4519,21 @@ 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 held: + print( + "note: this pull request merges into a branch other than the default and Copilot " + "has reviewed it already, so a fix push is covered by an attested local pass rather " + "than another Copilot round, and this wait requests nothing. Pass --request to ask " + "for a round anyway." + ) + 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. @@ -4054,7 +4589,14 @@ def main(argv: list[str] | None = None) -> int: # Gating the verdict behind it left the login check unable to reach an exit code. # The digest above printed `shapes=UNRECOGNIZED` the whole time it did so. # Coverage of the head is the other half, returning 0 only once the diff is covered too. - if unrecognized_shapes(final) or head_review_done(final, a.min_rounds): + held = held and holds(final) + covered = held and local_cover(final) + halted = None if a.ignore_quota_signal else stopping_refusal(final) + if ( + unrecognized_shapes(final) + or covered + or (not halted and head_review_done(final, a.min_rounds)) + ): verdict = report_verdict(final, owner, repo) # The check reading ranks under both of those, and never replaces either. # An unreadable shape means no field here can be believed, this one included. @@ -4092,15 +4634,44 @@ 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 held and not refusal: + print( + "status=AWAITING_LOCAL_PASS no Copilot round covers this head and none was requested, " + "since a fix push after the first round is covered by a local pass. Run the local " + "strict review, record it, push, and run `pr_review.py attest` from the checkout, " + "or pass --request to ask Copilot for a round instead" + ) + return 49 if refusal: print( "status=REVIEW_IS_A_REFUSAL the review carrying the head says it did not review, " diff --git a/spec/project-types.json b/spec/project-types.json index 59031dab..2dd82d3d 100644 --- a/spec/project-types.json +++ b/spec/project-types.json @@ -112,7 +112,7 @@ "hugo": { "detect": ["hugo.yaml", "hugo.toml", "config/_default/hugo.yaml"], "intentRefs": ["WORKFLOW.md"], - "note": "Named for the generator rather than for the transport, because what a repo builds and where the result lands are separate axes. The destination is publish[] ({ target, mechanism }), so a repo changes transport without changing type. Every assert below is phrased without naming the generator except hugo.build.strict, where a generator-specific flag is the letter, so promoting the generic ones to a shared type when a second generator arrives is a registry edit. Deploy credentials are per-environment GitHub Environment secrets and variables, which spec/secrets.json cannot yet express, so a repo does not list them in its registry requiredSecrets: spec/audit.py resolves that list against the repository actions store and would report an environment-scoped name as missing.", + "note": "Named for the generator rather than for the transport, because what a repo builds and where the result lands are separate axes. The destination is publish[] ({ target, mechanism }), so a repo changes transport without changing type. Every assert below is phrased without naming the generator except hugo.build.strict, where a generator-specific flag is the letter, so promoting the generic ones to a shared type when a second generator arrives is a registry edit. Deploy credentials are per-environment GitHub Environment secrets and variables, which no audit or configuration tool here inventories, the hub's deploy-site-task.yml failing its own run on an empty one it needs instead. A repo does not list them in its registry requiredSecrets, because spec/audit.py resolves that list against the repository actions store and would report an environment-scoped name as missing.", "checks": [ { "id": "hugo.build.strict", "verdict": "letter", "assert": "The site build fails on a generator warning rather than rendering around it (hugo --gc --minify --panicOnWarning), and the pull request gate and the deploy run the same build command rather than two variants.", "workflowRef": "WORKFLOW.md#d1---pr-fast-feedback-smoke" }, { "id": "hugo.urls.parity", "verdict": "letter", "assert": "A URL contract gate compares the built tree against a committed list of the URLs that must render and the URLs that must redirect, and asserts a minimum length on each list before comparing it, since a truncated list makes every assertion below it pass vacuously. This is the type's check of record, standing in for the unit tests a site does not have.", "workflowRef": "WORKFLOW.md#6-per-project-type-test-walkthroughs" }, diff --git a/spec/secrets.json b/spec/secrets.json index 02eca3dd..65088572 100644 --- a/spec/secrets.json +++ b/spec/secrets.json @@ -1,6 +1,6 @@ { "$schema": "./secrets.schema.json", - "note": "Secrets the audit cross-checks. `baseline` applies to every fleet repo (the App-signed merge-bot runs everywhere). `mechanisms` are per-target/per-feature additions: a repo requires the baseline plus the mechanisms its declared publish target (`targetMechanisms`) or declared type (`typeMechanisms`) maps to. All three mappings resolve from the registry entry rather than from workflow content: nothing reads a repo's Actions files to infer a mechanism, and `workflowNeeds` records what a mechanism needs to appear in a workflow for a human or agent reading the audit, rather than being a detector. `featureMechanisms` is shape-validated but claims nothing today, since the one feature it names (codecov) is claimed through `typeMechanisms` wherever the repo carries tests for that type instead. Baseline secrets are implicit and are NOT repeated in a repo's registry `requiredSecrets`, which lists only the domain-specific additions. `typeMechanisms` are per-language requirements: a `csharp` or `python` repo carrying tests for that language must carry the mapped mechanism (codecov) regardless of opt-in, a lint-only language other than Python excepted. A configured secret that no applicable mechanism claims is a stale-secret finding; a present `forbids` secret is a defect. `environments`, where a repo carries it, lists the per-environment GitHub Environment secrets and variables its deploy needs. It is operator documentation rather than part of the mechanism audit: no tool reads it, because neither `spec/validate.py` nor `spec/audit.py` queries an environment-scoped store, so a clean audit is not evidence that an environment is configured. `environmentSecrets` names what one environment carries and another does not, so a name audit does not read a single-environment credential as missing everywhere else.", + "note": "Secrets the audit cross-checks. `baseline` applies to every fleet repo (the App-signed merge-bot runs everywhere). `mechanisms` are per-target/per-feature additions: a repo requires the baseline plus the mechanisms its declared publish target (`targetMechanisms`) or declared type (`typeMechanisms`) maps to. All three mappings resolve from the registry entry rather than from workflow content: nothing reads a repo's Actions files to infer a mechanism, and `workflowNeeds` records what a mechanism needs to appear in a workflow for a human or agent reading the audit, rather than being a detector. `featureMechanisms` is shape-validated but claims nothing today, since the one feature it names (codecov) is claimed through `typeMechanisms` wherever the repo carries tests for that type instead. Baseline secrets are implicit and are NOT repeated in a repo's registry `requiredSecrets`, which lists only the domain-specific additions. `typeMechanisms` are per-language requirements: a `csharp` or `python` repo carrying tests for that language must carry the mapped mechanism (codecov) regardless of opt-in, a lint-only language other than Python excepted. A configured secret that no applicable mechanism claims is a stale-secret finding, and a present `forbids` secret is a defect. The schema still allows an `environments` block, whose `environmentSecrets` map would name what one environment carries and another does not, but the block has no per-repo dimension and downstream copies of this file are retired, so no repository has a place here to declare its environment names, and this file carries none. No tool would read one either: neither `spec/validate.py` nor `spec/audit.py` queries an environment-scoped store, so a clean audit is not evidence that an environment is configured.", "baseline": { "requires": ["CODEGEN_APP_CLIENT_ID", "CODEGEN_APP_PRIVATE_KEY"], "forbids": ["CODEGEN_APP_ID"], @@ -44,7 +44,7 @@ "forbids": [], "workflowNeeds": ["environment:", "IdentitiesOnly=yes"], "stores": [], - "note": "A deploy to a filesystem on a host the project owns, reached over SSH. requires and stores are empty deliberately rather than for want of credentials: the key and the host values are per-environment GitHub Environment secrets and variables, which this file has no vocabulary for and neither validate.py nor audit.py queries. Listing the names would force them into the repo's registry requiredSecrets, which the audit resolves against the repository actions store, so a correctly configured repo would report every one of them as missing. A repo declares them in its own environments block below instead. The key is confined at the far end by an authorized_keys forced command rooted at the deploy tree, so the workflow names no host path." + "note": "A deploy to a filesystem on a host the project owns, reached over SSH. requires and stores are empty deliberately rather than for want of credentials: the key and the host values are per-environment GitHub Environment secrets and variables, which neither validate.py nor audit.py queries. Listing the names would force them into the repo's registry requiredSecrets, which the audit resolves against the repository actions store, so a correctly configured repo would report every one of them as missing. The key is confined at the far end by an authorized_keys forced command rooted at the deploy tree, so the workflow names no host path." } }, "targetMechanisms": { diff --git a/tests/test_install_tools.py b/tests/test_install_tools.py index 5356acec..ec4f6131 100755 --- a/tests/test_install_tools.py +++ b/tests/test_install_tools.py @@ -151,6 +151,42 @@ def test_docker_inside_wsl_names_docker_desktop_as_its_mechanism(self) -> None: self.assertEqual(result.returncode, 0, result.stderr) self.assertEqual(result.stdout, expected) + def test_selection_naming_a_tool_other_than_the_last_succeeds(self) -> None: + result = self.run_bash('REQUESTED=(jq)\nresolve_selection\nprintf "%s\\n" "${SELECTED[@]}"') + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, "jq\n") + + def test_note_paths_under_home_are_written_with_a_tilde(self) -> None: + body = """ +HOME=/srv/example-user +JSON_OUTPUT=true +SELECTED=(jq) +tool_shadow_path() { printf '/srv/example-user/.local/bin/%s' "$1"; } +jq_version() { :; } +jq_target() { :; } +report +""" + result = self.run_bash(body) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotIn("/srv/example-user", result.stdout) + notes = json.loads(result.stdout)["tools"][0]["notes"] + self.assertEqual(len(notes), 1) + self.assertTrue(notes[0].startswith("~/.local/bin/jq "), notes[0]) + + def test_hide_home_only_rewrites_a_whole_leading_home_component(self) -> None: + cases = { + "/srv/example-user": "~", + "/srv/example-user/bin": "~/bin", + "/srv/example-user2/bin": "/srv/example-user2/bin", + "/usr/local/bin": "/usr/local/bin", + "": "", + } + for path, expected in cases.items(): + with self.subTest(path=path): + result = self.run_bash('HOME=/srv/example-user\nhide_home "$1"', path) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, expected) + def test_report_with_no_rows_is_still_an_object(self) -> None: result = self.run_bash("JSON_OUTPUT=true\nSELECTED=()\nreport") self.assertEqual(result.returncode, 0, result.stderr) diff --git a/tests/test_local_review.py b/tests/test_local_review.py index ab702ab0..56214f0b 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -1420,6 +1420,7 @@ def test_status_emits_json_carrying_the_reviewers(self) -> None: self.assertEqual(code, 0) data = json.loads(buf.getvalue()) self.assertEqual(data["reviewers"], ["agent-skill"]) + self.assertEqual(data["findings"], {"agent-skill": 2}) self.assertTrue(data["covered"]) def test_record_reports_what_it_recorded(self) -> None: diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index cc261e85..e25a0870 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, @@ -2592,14 +2605,8 @@ def test_a_badge_missing_its_alt_does_not_swallow_the_next_entry(self) -> None: self.assertNotIn("", pr_review.normal(f"{single} {MISSED_TITLE}")) -class TestCoverageCarriesForward(GqlCase): - """An earlier round's coverage statement stands until something changes it. - - Five of ten rounds measured in the second overview format state no coverage, and the two - measured on one drive were re-reviews of a one-line and a four-file delta. Blocking each of - those asks for a re-request that produced the marker in one of the four measured, so the - block clears by chance rather than by asking and a reader learns to route around it. - """ +class CarryCase(GqlCase): + """The rounds and compares both carries read, a statement's and a table's.""" NONE = OVERVIEW + "\n**Findings:** None" FULL = OVERVIEW + "\n" @@ -2635,6 +2642,16 @@ def rest(path: str, jq: str | None = None) -> subprocess.CompletedProcess: return mock.patch.object(pr_review, "gh_rest", side_effect=rest) + +class TestCoverageCarriesForward(CarryCase): + """An earlier round's coverage statement stands until something changes it. + + Five of ten rounds measured in the second overview format state no coverage, and the two + measured on one drive were re-reviews of a one-line and a four-file delta. Blocking each of + those asks for a re-request that produced the marker in one of the four measured, so the + block clears by chance rather than by asking and a reader learns to route around it. + """ + def test_the_newest_round_that_states_any_coverage_is_the_one_carried(self) -> None: pr = self.rounds( review(oid=OLD, body=self.PART, at=EARLY, rid="PRR_a"), @@ -2954,6 +2971,147 @@ 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. + + Three pull requests in one session each had a table naming every changed file on round one + and a re-review on the next head carrying no table, the file set unchanged, and the + maintainer accepted the first table every time. The bound is the one a statement carries + under, the same set of changed files at both commits. + """ + + def tabled(self, head_body: str | None = None, files: list[str] | None = None) -> dict: + changed = ["a.py", "b.py"] if files is None else files + return payload( + [ + review(oid=OLD, body=summarized(["a.py", "b.py"], covers=""), at=EARLY, rid="A"), + review(oid=HEAD, body=self.NONE if head_body is None else head_body, at=LATE), + ], + files=changed, + ) + + def verdict(self, pr: dict) -> tuple[int, str]: + with contextlib.redirect_stdout(io.StringIO()): + code = pr_review.report_verdict(pr, "o", "r") + out, _ = pr_review.digest("o", "r", 7, pr=pr) + return code, out + + def test_a_full_table_on_an_unchanged_file_set_closes_the_head(self) -> None: + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["b.py", "a.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(0, code) + self.assertIn("coverage=carried:table ", out) + self.assertIn(f"the newest round that does, on {OLD[:8]}, carries its table", out) + self.assertIn("names exactly the 2 changed files", out) + + def test_a_moved_file_set_keeps_the_table_where_it_was(self) -> None: + with self.compare(**{OLD: ["a.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(45, code) + self.assertIn("coverage=unstated ", out) + self.assertIn("a diff this head no longer has", out) + + def test_a_file_set_that_could_not_be_read_carries_no_table(self) -> None: + with self.compare(**{HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(self.tabled()) + self.assertEqual(45, code) + self.assertIn("could not be read at both commits, so its table is not carried", out) + + def test_a_partial_table_does_not_carry(self) -> None: + """A table that leaves out a changed file is partial, carried or not.""" + pr = self.tabled(files=["a.py", "b.py", "c.py"]) + with self.compare(**{OLD: ["a.py", "b.py", "c.py"], HEAD: ["a.py", "b.py", "c.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn(f"is on {OLD[:8]}, where the table leaves out c.py", out) + + def test_a_partial_on_record_keeps_a_carried_table_out(self) -> None: + """The partial sits on a commit the statement carry refuses, so the table path decides.""" + older = "c" * 40 + pr = self.tabled() + pr["reviews"]["nodes"].insert( + 0, review(oid=older, body=self.PART, at="2026-08-02T09:00:00Z", rid="P") + ) + with self.compare(**{older: ["a.py"], OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("COVERAGE IS NOT CARRIED", out) + self.assertIn("NO FILE TABLE STANDS IN: a Copilot round on this pull request states", out) + + def test_a_review_history_past_the_window_keeps_a_carried_table_out(self) -> None: + pr = self.tabled() + pr["reviews"]["pageInfo"]["hasPreviousPage"] = True + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("NO FILE TABLE STANDS IN: the review history is longer", out) + + def test_a_head_table_that_misses_the_diff_is_not_overruled_by_an_earlier_one(self) -> None: + pr = self.tabled(head_body=summarized(["a.py"], covers="")) + with self.compare(**{OLD: ["a.py", "b.py"], HEAD: ["a.py", "b.py"]}): + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("NO FILE TABLE STANDS IN: the table leaves out b.py", out) + self.assertNotIn("/compare/", " ".join(self.calls)) + + def test_only_the_newest_earlier_table_is_consulted(self) -> None: + """An older matching table does not stand in for a newer one that misses the diff.""" + older = "c" * 40 + for first, second, code in ( + (["a.py", "b.py"], ["a.py"], 45), + (["a.py"], ["a.py", "b.py"], 0), + ): + with self.subTest(newer=second): + pr_review.changed_at.cache_clear() + pr = payload( + [ + review(oid=older, body=summarized(first, covers=""), at=EARLY, rid="A"), + review(oid=OLD, body=summarized(second, covers=""), at=LATE, rid="B"), + review(oid=HEAD, body=self.NONE, at="2026-08-02T12:00:00Z", rid="C"), + ], + files=["a.py", "b.py"], + ) + same = ["a.py", "b.py"] + with self.compare(**{older: same, OLD: same, HEAD: same}): + self.assertEqual(code, self.verdict(pr)[0]) + + def test_a_refusal_carrying_a_table_is_not_the_table_carried(self) -> None: + refused = REFUSED + "\n\n" + summarized(["a.py", "b.py"], covers="") + pr = payload( + [ + review(oid=OLD, body=refused, at=EARLY, rid="A"), + review(oid=HEAD, body=self.NONE, at=LATE, rid="B"), + ], + files=["a.py", "b.py"], + ) + self.assertIsNone(pr_review.carried_table(pr)) + + def test_no_table_anywhere_says_so(self) -> None: + pr = self.rounds(review(oid=HEAD, body=self.NONE)) + code, out = self.verdict(pr) + self.assertEqual(45, code) + self.assertIn("no earlier round carries one either", out) + + class TestTheDeltaSinceTheCarriedRound(unittest.TestCase): """What changed between the round that stated coverage and the head, reported not judged.""" @@ -5329,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()]), @@ -5342,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"} @@ -5460,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.""" @@ -5505,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.""" @@ -5546,6 +5690,324 @@ 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 into(self, pr: dict, base: str = "develop", attest: bool = False) -> dict: + """A pull request into `base` on a repository whose default branch is main.""" + pr = {**pr, "baseRefName": base, "baseRepository": {"defaultBranchRef": {"name": "main"}}} + if attest: + marker = { + "author": {"login": "maintainer"}, + "authorAssociation": "OWNER", + "createdAt": LATE, + "body": f"Attested.\n\n", + } + pr["comments"] = {"nodes": [marker], "pageInfo": {"hasPreviousPage": False}} + return pr + + def test_a_newer_quota_refusal_outranks_older_coverage_of_the_head(self) -> None: + """A re-request answered by the limit is the newest word, whatever the head already had.""" + pr = payload( + [ + review(at=EARLY, rid="PRR_a"), + review(body=QUOTA_REFUSED, at=LATE, rid="PRR_b"), + ] + ) + self.answer(pr) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=QUOTA", out) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(46, self.cli(["wait", "7", "--timeout", "0"])) + + def test_an_older_file_count_refusal_still_yields_to_coverage(self) -> None: + pr = payload([review(body=REFUSED, at=EARLY, rid="PRR_a"), review(at=LATE, rid="PRR_b")]) + self.answer(pr) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn("refusal=no", out) + + def test_a_fix_push_after_the_first_round_is_not_requested(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]))) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep") as slept: + self.assertEqual(49, 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=AWAITING_LOCAL_PASS", self.out.getvalue()) + + def test_an_attested_fix_push_closes_the_wait(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + out = self.out.getvalue() + self.assertIn("review_on_head=local", out) + self.assertIn("coverage=local", out) + + def test_request_asks_for_a_round_on_a_fix_push(self) -> None: + self.answer(self.into(payload([review(oid=OLD)]))) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0", "--request"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_promotion_and_a_first_round_are_still_requested(self) -> None: + for pr in ( + self.into(payload([review(oid=OLD)]), base="main", attest=True), + self.into(payload([])), + ): + with self.subTest(base=pr["baseRefName"]): + self.out.seek(0) + self.out.truncate() + self.answer(pr) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_an_attested_head_behind_a_stale_quota_refusal_is_covered(self) -> None: + rounds = [ + review(oid="a" * 39 + "1", at=EARLY, rid="PRR_a"), + review(oid=OLD, body=QUOTA_REFUSED, at=LATE, rid="PRR_b"), + ] + self.answer(self.into(payload(rounds), attest=True)) + self.wire_history([hist_review(7, QUOTA_REFUSED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + + def test_a_partial_on_record_requests_a_round_rather_than_holding(self) -> None: + part = OVERVIEW + "\n" + self.answer(self.into(payload([review(oid=OLD, body=part)]), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + self.assertNotIn("AWAITING_LOCAL_PASS", self.out.getvalue()) + + def test_an_attested_head_past_an_old_plain_answer_is_covered(self) -> None: + pr = self.into(payload([review(oid=OLD, at=EARLY)], comments=[comment(at=LATE)])) + attested = self.into(pr, attest=True) + attested["comments"]["nodes"].append(comment(at=LATE)) + self.answer(attested) + self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(0, self.cli(["wait", "7", "--timeout", "0"])) + + def test_a_pending_request_on_a_fix_push_is_polled_for(self) -> None: + self.answer(self.into(payload([review(oid=OLD)], pending=True), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(30, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_push_during_a_held_wait_grades_the_new_head(self) -> None: + """An attestation of the head read first does not cover the head the verdict reads.""" + first = self.into(payload([review(oid=OLD)]), attest=True) + moved = self.into(payload([review(oid=OLD)]), attest=True) + moved["headRefOid"] = "c" * 40 + self.answer(first, first, moved) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.assertEqual(49, self.cli(["wait", "7", "--timeout", "0"])) + self.assertEqual(0, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_truncated_review_history_requests_a_round_rather_than_holding(self) -> None: + self.answer(self.into(payload([review(oid=OLD)], older_reviews=True), attest=True)) + calls = self.wire_history([hist_review(7, OVERVIEW + "\n" + COVERED)]) + with mock.patch.object(pr_review.time, "sleep"): + self.cli(["wait", "7", "--timeout", "0"]) + self.assertEqual(1, len([c for c in calls if "requestReviews" in c[0]])) + + def test_a_truncated_review_history_is_no_local_cover(self) -> None: + pr = self.into(payload([review(oid=OLD)], older_reviews=True), attest=True) + self.assertFalse(pr_review.local_cover(pr)) + + def test_checkout_belongs_to_attest_alone(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + pr_review.main(["status", "7", "--repo", "o/r", "--checkout", "."]) + + def test_request_belongs_to_wait_alone(self) -> None: + with contextlib.redirect_stderr(io.StringIO()), self.assertRaises(SystemExit): + pr_review.main(["status", "7", "--repo", "o/r", "--request"]) + + def test_a_refusal_on_the_head_is_not_overruled_by_an_attestation(self) -> None: + rounds = [ + review(oid=OLD, at=EARLY, rid="PRR_a"), + review(body=REFUSED, at=LATE, rid="PRR_b"), + ] + pr = self.into(payload(rounds), attest=True) + self.assertFalse(pr_review.local_cover(pr)) + + def test_status_reads_an_attested_head_unless_a_partial_is_on_record(self) -> None: + part = OVERVIEW + "\n" + for rounds, field in ( + ([review(oid=OLD)], "review_on_head=local"), + ([review(oid=OLD, body=part)], "review_on_head=NO"), + ): + with self.subTest(field=field): + self.answer(self.into(payload(rounds), attest=True)) + out, _ = pr_review.digest("o", "r", 7) + self.assertIn(field, out) + + 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.""" @@ -7011,6 +7473,7 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin "Fixed.", ], "wait": ["wait", "7", "--repo", "someone-else/r"], + "attest": ["attest", "7", "--repo", "someone-else/r"], } # The parser's remaining `cmd` choices, none of which write. @@ -7047,6 +7510,195 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No gh_graphql.assert_not_called() +class TestAttest(unittest.TestCase): + """A local pass is published only for the content the pull request's head carries.""" + + def setUp(self) -> None: + self.dir = self.enterContext(tempfile.TemporaryDirectory()) + env = {**os.environ, "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_SYSTEM": os.devnull} + for args in ( + ["init", "-q", "-b", "feature"], + [ + "-c", + "user.name=t", + "-c", + "user.email=t@example.test", + "commit", + "-q", + "--allow-empty", + "-m", + "base", + ], + ): + subprocess.run(["git", "-C", self.dir, *args], check=True, env=env) + subprocess.run( + ["git", "-C", self.dir, "update-ref", "refs/remotes/origin/develop", "HEAD"], + check=True, + env=env, + ) + self.head = subprocess.run( + ["git", "-C", self.dir, "rev-parse", "HEAD"], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + env=env, + ).stdout.strip() + self.enterContext(mock.patch.object(pr_review, "in_scope", return_value=(True, ""))) + self.posted: list[str] = [] + + def post(_o: str, _r: str, _n: int, body: str) -> int: + self.posted.append(body) + return 0 + + self.enterContext(mock.patch.object(pr_review, "comment_on_pr", side_effect=post)) + self.enterContext(contextlib.redirect_stdout(io.StringIO())) + + def run_attest(self, head: str, check_exit: int = 0, base: str | None = None) -> int: + """Run `attest` with a stub `local_review.py` that records how it was called.""" + record = Path(self.dir).parent / f"calls-{check_exit}.txt" + stub = Path(self.dir).parent / f"stub-{check_exit}.py" + stub.write_text( + "import json, os, sys\n" + f"open({str(record)!r}, 'a').write(os.getcwd() + '|' + ' '.join(sys.argv[1:]) + '\\n')\n" + "if sys.argv[1] == 'status':\n" + f" print(json.dumps({{'covered': {check_exit == 0}, 'receiptProblems': []," + " 'findings': {'agent-skill': 2, 'coderabbit-cli': 1}}))\n" + f"sys.exit({check_exit})\n" + ) + self.addCleanup(stub.unlink) + self.addCleanup(lambda: record.unlink(missing_ok=True)) + self.record = record + target = {"headRefOid": head, "baseRefName": "develop"} + merge_base = subprocess.CompletedProcess([], 0, (base or self.head) + "\n", "") + with ( + mock.patch.object(pr_review, "gql", return_value=target), + mock.patch.object(pr_review, "gh_rest", return_value=merge_base), + mock.patch.object(pr_review, "LOCAL_REVIEW", stub), + ): + return pr_review.attest("o", "r", 7, self.dir) + + def test_a_covered_head_is_attested_by_its_full_commit(self) -> None: + self.assertEqual(0, self.run_attest(self.head)) + self.assertEqual(1, len(self.posted)) + self.assertIn( + f"\n", + self.posted[0], + ) + calls = [line.split("|") for line in self.record.read_text().splitlines()] + self.assertEqual(["status --target develop"], [c[1] for c in calls]) + for cwd, _ in calls: + self.assertEqual(os.path.realpath(self.dir), os.path.realpath(cwd)) + + def test_a_checkout_measuring_another_merge_base_is_refused(self) -> None: + self.assertEqual(67, self.run_attest(self.head, base="e" * 40)) + self.assertEqual([], self.posted) + + def test_a_checkout_that_moves_during_the_check_is_refused(self) -> None: + original = pr_review._git + reads: list[str] = [] + + def moved(checkout: str, *args: str) -> subprocess.CompletedProcess: + proc = original(checkout, *args) + if args == ("rev-parse", "HEAD"): + reads.append("HEAD") + if len(reads) > 1: + return subprocess.CompletedProcess([], 0, "f" * 40 + "\n", "") + return proc + + with mock.patch.object(pr_review, "_git", side_effect=moved): + self.assertEqual(67, self.run_attest(self.head)) + self.assertEqual([], self.posted) + + def test_a_base_that_would_change_the_compare_path_is_refused(self) -> None: + target = {"headRefOid": self.head, "baseRefName": "develop#frag"} + with ( + mock.patch.object(pr_review, "gql", return_value=target), + mock.patch.object(pr_review, "gh_rest") as rest, + ): + self.assertEqual(67, pr_review.attest("o", "r", 7, self.dir)) + rest.assert_not_called() + self.assertEqual([], self.posted) + + def test_an_unread_pull_request_is_refused(self) -> None: + with mock.patch.object(pr_review, "gql", return_value={}): + self.assertEqual(65, pr_review.attest("o", "r", 7, self.dir)) + self.assertEqual([], self.posted) + + def test_a_checkout_at_another_commit_is_refused(self) -> None: + self.assertEqual(67, self.run_attest("f" * 40)) + self.assertEqual([], self.posted) + + def test_a_checkout_holding_changes_is_refused(self) -> None: + (Path(self.dir) / "extra.txt").write_text("not in the head\n") + self.assertEqual(67, self.run_attest(self.head)) + self.assertEqual([], self.posted) + + def test_no_current_local_pass_is_refused(self) -> None: + self.assertEqual(68, self.run_attest(self.head, check_exit=1)) + self.assertEqual([], self.posted) + + +class TestAttestationReadings(unittest.TestCase): + """What the gate reads an attestation, a promotion, and a first round from.""" + + def comment(self, body: str, association: str = "OWNER") -> dict: + return {"body": body, "authorAssociation": association, "author": {"login": "someone"}} + + def test_the_attestation_carries_the_findings_count(self) -> None: + for line, expected in ( + (f"", "4"), + (f"", "unknown"), + (f"", None), + (f"", None), + ): + with self.subTest(line=line): + pr = { + "headRefOid": HEAD, + "baseRefName": "develop", + "comments": {"nodes": [self.comment(line)]}, + } + self.assertEqual(expected, pr_review.attestation(pr)) + + def test_only_a_writer_s_marker_for_the_current_head_attests(self) -> None: + marker = f"" + for nodes, expected in ( + ([self.comment(marker)], True), + ([self.comment(marker, "COLLABORATOR")], True), + ([self.comment(marker, "NONE")], False), + ([self.comment(marker, "CONTRIBUTOR")], False), + ( + [self.comment(f"")], + False, + ), + ([self.comment(f"Attest posts `{marker}` as its last line.")], False), + ([self.comment(f"Quoted:\n\n```\n{marker}\n```\n")], False), + ([self.comment(f"A span `\n{marker}\n` across lines.")], False), + ([], False), + ): + with self.subTest(nodes=nodes): + pr = {"headRefOid": HEAD, "baseRefName": "develop", "comments": {"nodes": nodes}} + self.assertIs(expected, pr_review.attested(pr)) + + def test_a_promotion_is_a_pull_request_into_the_default_branch(self) -> None: + for base, default, expected in ( + ("main", "main", True), + ("develop", "main", False), + ("develop", None, True), + ): + with self.subTest(base=base, default=default): + pr = { + "baseRefName": base, + "baseRepository": {"defaultBranchRef": {"name": default} if default else None}, + } + self.assertIs(expected, pr_review.promotion(pr)) + + def test_a_refusal_is_not_a_first_round(self) -> None: + self.assertFalse(pr_review.first_round_done(payload([review(body=REFUSED)]))) + self.assertTrue(pr_review.first_round_done(payload([review(oid=OLD)]))) + self.assertFalse(pr_review.first_round_done(payload([]))) + + class TestWriteCommandsPartitionParserChoices(unittest.TestCase): """WRITE_COMMANDS and READ_ONLY_COMMANDS must together account for every parser choice.