diff --git a/.agents/skills/drive-pr/SKILL.md b/.agents/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.agents/skills/drive-pr/SKILL.md +++ b/.agents/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.agents/skills/merge-and-release/SKILL.md b/.agents/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.agents/skills/merge-and-release/SKILL.md +++ b/.agents/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/.claude-plugin/fleet-skills/.source-digests/drive-pr b/.claude-plugin/fleet-skills/.source-digests/drive-pr index 932f8d99a..f510ab023 100644 --- a/.claude-plugin/fleet-skills/.source-digests/drive-pr +++ b/.claude-plugin/fleet-skills/.source-digests/drive-pr @@ -1 +1 @@ -aeb0b23f1960e8f7 +227f27065490536c diff --git a/.claude-plugin/fleet-skills/.source-digests/merge-and-release b/.claude-plugin/fleet-skills/.source-digests/merge-and-release index cfde66251..8dd862964 100644 --- a/.claude-plugin/fleet-skills/.source-digests/merge-and-release +++ b/.claude-plugin/fleet-skills/.source-digests/merge-and-release @@ -1 +1 @@ -0d6d711570caaa67 +f94d03359a89a1ea diff --git a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md b/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/.github/skills/drive-pr/SKILL.md b/.github/skills/drive-pr/SKILL.md index be6f4f542..6fbe9062f 100644 --- a/.github/skills/drive-pr/SKILL.md +++ b/.github/skills/drive-pr/SKILL.md @@ -107,7 +107,22 @@ promotion PR once the fix lands, is the early exit this skill exists to prevent. to delete it, which fails when `develop` is already checked out somewhere else, the ordinary case in this layout. Instead run repo-worktree's post-merge cleanup from the base clone, remove the worktree and delete the now-merged local task branch, then verify before deleting the remote - one, which is this skill's own step rather than that one's. Both remote commands resolve `origin`, + one, which is this skill's own step rather than that one's. Delete the remote one with + `python3 scripts/pr_review.py cleanup --repo /`, run from a hub checkout. + It takes the pull request rather than a branch name and does the verify-then-delete below in + code. It refuses unless the pull request merged into `develop` from a head in that same + repository, and refuses a permanent branch or one any ruleset or branch protection covers. It + checks the tip is still at the pull request's `headRefOid` before deleting, and reads the ref + back to confirm. + Exit `0` is done, the branch deleted or already gone. Any other exit stops and reports rather + than deleting by hand: `1` is a refusal, a moved tip among them, `2` a read that did not answer + or a delete it could not confirm, and `64` a target under an owner other than the hub + checkout's own `origin`, or that `origin` unreadable. A registry refusal is a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the hand form below is the fallback, and where this seat cannot + run that either, report its commands to the maintainer, paste-ready with the values substituted. + Both remote commands of the hand form resolve `origin`, so they hold only where the pull request's head branch lives in this repository, which step 1 guarantees by branching here. A pull request opened from a fork follows `upstream-contribution-workflow` instead and neither command applies to it, since `origin` would diff --git a/.github/skills/merge-and-release/SKILL.md b/.github/skills/merge-and-release/SKILL.md index ab771a2f8..cd721748a 100644 --- a/.github/skills/merge-and-release/SKILL.md +++ b/.github/skills/merge-and-release/SKILL.md @@ -227,7 +227,18 @@ skill covers all of it, scoped down by what the maintainer actually asks for. use `git branch -D -- ""` here, safe only because the GitHub-state check just proved that exact branch finished, the narrow post-squash exception git-commit-conventions describes, never applied to an unverified branch. Then, only when the remote branch still - exists, delete it the same way, `git push origin --delete -- ""`. + exists, delete it with `python3 scripts/pr_review.py cleanup --repo owner/repo`, run + from a hub checkout, `` being the merged pull request the check above returned. It + re-reads that pull request and repeats the merged, base, head-repository, and tip checks in + code, refuses a permanent or protected branch, and confirms the delete by reading the ref + back. Exit `0` is done, and any other exit stops and reports: `1` is a refusal, `2` an + unanswered read or an unconfirmed delete, and `64` a target under an owner other than the hub + checkout's own `origin` or that `origin` unreadable, a registry refusal being a `1`. It runs + under the host-safety installer's `Bash(python3 scripts/pr_review.py:*)` allow rule, + which is what lets auto mode complete it. + On a host without that rule the fallback is `git push origin --delete -- ""` once the + tip checks above pass, and where this seat cannot run it, report it to the maintainer, + paste-ready with the values substituted. Never `--force-with-lease` here, git-commit-conventions forbids it unconditionally, the GitHub-state check just completed is the verification gate, not a compare-and-swap at delete time. Never apply this sweep to `develop` or `main` diff --git a/scripts/README.md b/scripts/README.md index dcabc6393..5b3594605 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -164,7 +164,7 @@ A missing required tool prints its platform source and runnable remedy. An `inst ## `pr_review.py` -One compact interface to a pull request's Copilot review loop. `status` prints the digest, and `wait` runs the backoff in-process. `comment` posts a PR-conversation answer, while `reply` answers one thread, or each copy of a finding posted twice, and optionally resolves it. `claims` reads the description against the branch it describes. The runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. +One compact interface to a pull request's Copilot review loop. `status` prints the digest, and `wait` runs the backoff in-process. `comment` posts a PR-conversation answer, while `reply` answers one thread, or each copy of a finding posted twice, and optionally resolves it. `claims` reads the description against the branch it describes. `cleanup` deletes a merged pull request's head branch once it is verified. The runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. ```sh python3 scripts/pr_review.py status 452 --repo ptr727/ProjectTemplate @@ -175,6 +175,7 @@ python3 scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ 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 +python3 scripts/pr_review.py cleanup 452 --repo ptr727/ProjectTemplate ``` **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. Where it is attested, `wait` polls the head's checks instead. The poll runs only while the checks can still move the merge, as the `30` paragraph below lists. Once they settle, or the pull request closes, the head ends as covered. Where it is not attested, `wait` exits `49`, 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. The attestation conditions above also let one cover a head whose Copilot round states no coverage, has none carried to it, and carries no table. `status` and `wait` then read `review_on_head=yes` with `coverage=local` rather than exiting `45`. Neither reading applies to the head the first round itself landed on, since that head is no fix push after it. The review loop attests every push a recorded pass covers, the opening push included, and the comment outlives the receipt that removing the worktree deletes. A promotion reads the comment only where `pr-review-conduct` Merge Gate item 2 falls back to per-file evidence. @@ -183,7 +184,7 @@ python3 scripts/pr_review.py attest 452 --repo ptr727/ProjectTemplate --checkout **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. -`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also a check the base branch's rules require that the head has not registered yet, on a `BLOCKED` merge whose rollup was read whole. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one. +`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. On an attested held head it exits `30` with `status=CHECKS_PENDING` at the timeout. That is a required check still settling, one running long included, or any check while no required one has posted. It is also a check the base branch's rules require that the head has not registered yet, on a `BLOCKED` merge whose rollup was read whole. It is also no check posted on a `BLOCKED`, `BEHIND`, or `DRAFT` merge, or the merge still `UNKNOWN`. A merge reading `CLEAN`, `UNSTABLE`, or `HAS_HOOKS` ends the poll ahead of all of these. On a `BLOCKED` merge three shapes of required check end the poll at once even while another required check still runs, and the exit is then `44` unless a review verdict outranks it. A failed check ends it on the first read. A status expected and never posted, and a check queued with no runner, end it only once each is past `--check-grace`. A required check merely running long ends nothing. A round requested while that poll runs ends it with `status=PENDING`, so the next `wait` polls for that round. A quota reading outranks that line with its own `46` or `47`, and an answer outside a review with `40`. `--min-rounds N` counts what `status` prints as `rounds=`, every round the reviewer posted on the pull request. That count spans every head and includes refusals, and the wait holds out for a round past the Nth. N is therefore the `rounds=` a `status` read before a re-request. One above it waits for the round after the one the re-request was for, so the round that lands leaves the wait at `30`. Where that round lands during the wait, the `status=PENDING` line names the position it still waits for. A round already there when `wait` began leaves the bare line, since nothing then tells it apart from a correct count. `wait`'s failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, an unknown author's open thread left uncounted, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the five mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, `resolveReviewThread`, and `deleteRef`, so a sixth arriving is a write nobody reviewed as one. `wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers. @@ -238,6 +239,8 @@ Every failure is a stop rather than a fallback, because each alternative closes What this trades away is stated rather than glossed. A mutation spelled as a `gh` command in a shell is read by the `gh-write-guard` hook and one this script performs is not. The script removes that failure at its source instead: every node ID comes from a live query in the same run. The owner check is also enforced in-process. The whole-source test guards against every other state-changing call and counts each reviewed mutation document. +`cleanup` deletes the head branch of a merged pull request, the verify-then-delete `drive-pr` and `merge-and-release` prescribe after a merge into `develop`. It exists because both hand forms fail from an agent seat in auto mode. The `gh-write-guard` hook denies `git push origin --delete` from a primary checkout, and Claude Code's auto-mode classifier refuses the ref API delete as a destructive git operation on some days and allows it on others. A command matching a narrow `permissions.allow` rule is settled before that classifier runs, and the host-safety installer already registers `Bash(python3 scripts/pr_review.py:*)`, so the delete runs here under that rule with no settings change. That rule approves any arguments, so every check lives in the subcommand. It takes the pull request number and `--repo`, never a branch name, and reads the branch from the pull request. It refuses unless the pull request is merged, its base is `develop`, its head repository is `--repo` itself, and the head branch is neither `develop`, `main`, nor the default branch read live. Past the same owner check `comment` and `reply` make, `handoff.py`'s own write-scope check, imported rather than copied, binds the owner to `registry/repos.json` and admits a repository only where it is registered or a fork. A branch any ruleset or branch protection covers is refused, since the hook's per-branch rule check never sees a delete made from inside this script. The ref is read by its exact qualified name, and the delete is sent only where that read's tip is the pull request's `headRefOid`, by the node id the same read returned. The delete carries no expected tip of its own, so this is verify-then-delete rather than a compare-and-swap, the same gate the hand form is. The delete is confirmed by reading the ref back rather than by the mutation's own answer, so a delete that reports a failure and landed still reads as done, and one that reports success and left the ref reads as unconfirmed. Exit `0` is the branch deleted and confirmed gone, or already gone. Exit `1` is a refusal, the registry check's among them, and so is a tip that moved after the merge, which names both commits. Exit `2` is a read that did not answer, the rule read included, which never reads as no rules, or a delete the read back could not confirm. A usage error also exits `2`, argparse's own code. Exit `64` is the owner check `comment` and `reply` make against this checkout's `origin`, which also exits `64` where that `origin` cannot be read, run before the registry and before any read. No `--dry-run` exists, since no other subcommand here has one. + `claims` checks that a description does not contradict its own branch, and it exists for the same reason `sha-pin` now resolves: a reference that points at nothing is a silent failure caught by a reviewer or not at all. Three stale descriptions in one session generated six review findings between them, each a reviewer noticing that the body named a commit or a behavior the branch no longer carried. It reads the commits a body **claims** the branch carries and the `uses:` refs it quotes, confirms the head tree still carries each, and exits `70` where one does not. Prose claims stay **out of scope**, since judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] rejects for exactly the reason it would fail here. **Scanning for bare SHAs was built first and the corpus rejected it.** Over the 25 most recent merged pull requests it raised four findings, and every one was correct prose: a `develop` commit named as history, a SHA inside a pasted digest, and two commits in `ptr727/Blog` written without a URL. Nothing in the *form* of a bare SHA separates those from a real claim, and separating them by meaning is the heuristic already ruled out above. A path arm was measured on the same corpus and is worse, flagging 54 of 215 backticked candidates, nearly all of them bare basenames, `origin/develop`, and other repositories. What survives is the verb: a commit counts only where the body says it was fixed, landed, shipped, added, introduced, corrected, resolved, carried or amended in it. That alternation raises exactly one reference over the same 25, and that one is true. It is an **inclusion** list, so a phrasing nobody thought of costs a detection rather than producing a finding, which is the direction to be incomplete in, and a claimed SHA still has to carry a digit as a backstop on the list growing later, since `accede` and `defaced` inflect into all-hex English words. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 5110606f2..775c795a0 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -31,6 +31,25 @@ reviewer or not at all, and three stale descriptions in one session generated six review findings between them. Read-only. Exit 0 = every reference resolves, 70 = one does not, 71 = there were references and none could be read, so nothing was decided. + cleanup Delete a merged pull request's head branch, the verify-then-delete `drive-pr` and + `merge-and-release` prescribe, done here so it runs under the one allow rule the + host-safety installer registers for this script rather than as a hand-typed delete. + That rule approves any arguments, so every check lives here. It takes the pull request + number, never a branch name, and reads the branch from the pull request. It refuses + unless the pull request is merged into `develop` from a head in `--repo` itself, and + the head branch is none of `develop`, `main`, or the default branch read live. The + owner is bound to `registry/repos.json`, and the repository must be registered or a + fork, by `handoff.py`'s own write-scope check. It refuses a branch any ruleset or + branch protection covers, since the write guard's per-branch check never sees a delete + made in here. The ref is read exactly, its tip is checked against the pull request's + `headRefOid` before the delete is sent, and the delete is confirmed by reading the ref + back. The delete itself carries no expected tip, so this is verify-then-delete rather + than a compare-and-swap. No `--dry-run` exists, since no other subcommand here has + one. Exit 0 = deleted and confirmed gone, or already gone, 1 = refused, a + moved tip naming both commits among them, 2 = a read did not answer, a partial one + included, or the delete could not be confirmed, which argparse's own usage error + shares, 64 = the write scope could not be established or excludes the target, checked + before the registry and before any read. status One digest line, any unresolved threads, and any suppressed findings. Read-only. 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 @@ -351,6 +370,7 @@ from typing import NamedTuple import gh_graphql as shared +import handoff # The directory this script sits in, and the anchor every scope answer is read from. # One value serves the owner probe and the refusal naming it, so the two cannot disagree. @@ -999,6 +1019,22 @@ def strip_fences( resolveReviewThread(input:{threadId:$threadId}){ thread{ id isResolved } }} """ +Q_CLEANUP_TARGET = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ + defaultBranchRef{ name } + pullRequest(number:$n){ + merged baseRefName headRefName headRefOid headRepository{ nameWithOwner } } }} +""" +Q_CLEANUP_REF = """ +query($o:String!,$r:String!,$q:String!){ + repository(owner:$o,name:$r){ ref(qualifiedName:$q){ id target{ oid } } }} +""" +M_DELETE_REF = """ +mutation($refId:ID!){ + deleteRef(input:{refId:$refId}){ clientMutationId }} +""" + WORKFLOW_KEYS_UNAVAILABLE = "workflowKeysUnavailable" @@ -2069,23 +2105,28 @@ def reread_json(path: str, jq: str) -> object: None wherever nothing parseable came back, an absence GitHub answered included, so the caller has one value to refuse on. """ + return parsed_json(reread(path, jq)) - def parsed(proc: subprocess.CompletedProcess) -> object: - if proc.returncode != 0: - return None - try: - return json.loads(proc.stdout.strip()) - except json.JSONDecodeError: - return None +def reread(path: str, jq: str) -> subprocess.CompletedProcess: + """The last of `reread_json`'s reads, whole, so a caller can say why nothing parsed.""" read = functools.partial(gh_rest, path, jq) - proc = backoff( + return backoff( read(), read, - lambda p: not answered_absent(p) and parsed(p) is None, + lambda p: not answered_absent(p) and parsed_json(p) is None, delays=READ_RETRY_DELAYS, ) - return parsed(proc) + + +def parsed_json(proc: subprocess.CompletedProcess) -> object: + """The read's JSON, or None where it failed or printed nothing parseable.""" + if proc.returncode != 0: + return None + try: + return json.loads(proc.stdout.strip()) + except json.JSONDecodeError: + return None def tree_at(owner: str, repo: str, commit: str) -> dict[str, tuple[str, str]] | None: @@ -5571,6 +5612,183 @@ def check_claims(owner: str, repo: str, num: int) -> int: return 0 +class CleanupUnread(Exception): + """A read `cleanup` needed did not produce an answer, reported at exit 2.""" + + +def cleanup_read(query: str, **variables: object) -> dict: + """One GraphQL read for `cleanup`, raising rather than ending the run at exit 1. + + `gh_graphql` exits 1 on a failed read, which is `cleanup`'s refusal code, so a read that could + not answer would be reported as a refusal the caller can act on. + """ + try: + return shared.request(query, variables).data + except shared.GraphQLError as exc: + sys.stderr.write(exc.detail) + raise CleanupUnread(str(exc)) from exc + + +def cleanup_rest(path: str, what: str) -> object: + """One REST read for `cleanup`, retried, raising with its stderr where nothing parsed.""" + proc = reread(path, ".") + data = parsed_json(proc) + if data is None: + sys.stderr.write(proc.stderr) + raise CleanupUnread(f"the {what} read failed rc={proc.returncode} or printed no JSON") + return data + + +def cleanup_ref(owner: str, repo: str, branch: str) -> dict | None: + """The branch's ref node, or None where the branch does not exist. + + The read is by exact qualified name, so a branch sharing a suffix never answers for it. The + node id it returns goes straight to the delete, so no caller ever holds one to mistype. + """ + data = cleanup_read(Q_CLEANUP_REF, o=owner, r=repo, q=f"refs/heads/{branch}") + repository = data.get("repository") + if not isinstance(repository, dict): + raise CleanupUnread(f"{owner}/{repo} returned no repository for the ref read") + ref = repository.get("ref") + if ref is None: + return None + oid = (ref.get("target") or {}).get("oid") if isinstance(ref, dict) else None + if not isinstance(ref, dict) or not ref.get("id") or not oid: + raise CleanupUnread(f"the ref read for {branch!r} returned no id and tip: {ref!r}") + return ref + + +def branch_rules(owner: str, repo: str, branch: str) -> list[str]: + """What protects the branch, rulesets and classic protection alike, empty where nothing does. + + The branch travels inside a REST path, so a name that would change the path is not read, and + an unread rule set is never reported as no rules. + """ + if UNSAFE_REF.search(branch) or DOT_SEGMENT.search(branch): + raise CleanupUnread( + f"the head branch {branch!r} holds a character that changes a REST path, so the " + "rules applying to it cannot be read" + ) + rules = cleanup_rest(f"repos/{owner}/{repo}/rules/branches/{branch}", "branch rules") + if not isinstance(rules, list) or not all(isinstance(r, dict) for r in rules): + raise CleanupUnread(f"the branch rules read returned no list of rules: {rules!r}"[:400]) + found = [f"ruleset rule {r.get('type') or 'unnamed'}" for r in rules] + detail = cleanup_rest(f"repos/{owner}/{repo}/branches/{branch}", "branch protection") + protected = detail.get("protected") if isinstance(detail, dict) else None + if not isinstance(protected, bool): + raise CleanupUnread(f"the branch read returned no protected flag: {detail!r}"[:400]) + if protected: + found.append("branch protection") + return found + + +def cleanup(owner: str, repo: str, num: int) -> int: + """Delete the head branch of merged pull request `num` once every check passes. + + Returns 0 where the branch is deleted and the read back confirms it, or is already gone, 1 on + a refusal, 2 where a read did not answer or the delete could not be confirmed, and 64 where + the write scope `comment` and `reply` check excludes the target. Past that check, the owner is + bound to the fleet registry and the repository must be registered or a fork, by `handoff.py`'s + own write-scope check, reused rather than copied so the two cannot drift apart. Its refusal + text is printed as `handoff.py` words it. + """ + ok, why = in_scope(owner) + if not ok: + print(f"status=OUT_OF_SCOPE nothing was deleted: {why}") + return 64 + target = f"{owner}/{repo}" + try: + handoff.require_write_scope(argparse.Namespace(cmd="cleanup", repo=target)) + except handoff.Refusal as exc: + print(f"status=REFUSED nothing was deleted, refused by handoff.py's write scope: {exc}") + return 1 + except handoff.Execution as exc: + print(f"status=UNREAD nothing was deleted: {exc}") + return 2 + try: + return _cleanup(owner, repo, num) + except CleanupUnread as exc: + print(f"status=UNREAD nothing was deleted: {exc}") + return 2 + + +def _cleanup(owner: str, repo: str, num: int) -> int: + """The checks, the delete, and the read back, raising `CleanupUnread` on any unanswered read. + + A branch anything protects is refused whatever that protection would allow, since the write + guard's per-branch rule check never sees a delete made from inside this script. A delete that + reports a failure may still have landed, so the read back decides the outcome either way. + """ + target = f"{owner}/{repo}" + data = cleanup_read(Q_CLEANUP_TARGET, o=owner, r=repo, n=num) + repository = data.get("repository") or {} + pr = repository.get("pullRequest") + default = (repository.get("defaultBranchRef") or {}).get("name") + if not isinstance(pr, dict) or not default: + raise CleanupUnread(f"{target} #{num} returned no pull request or no default branch") + branch = pr.get("headRefName") + head = pr.get("headRefOid") + if not branch or not head: + raise CleanupUnread(f"{target} #{num} returned no head branch or head commit") + head_repo = (pr.get("headRepository") or {}).get("nameWithOwner") or "" + refusals = [] + if pr.get("merged") is not True: + refusals.append("the pull request is not merged") + if pr.get("baseRefName") != "develop": + refusals.append(f"its base is {pr.get('baseRefName')!r}, not 'develop'") + if head_repo.lower() != target.lower(): + refusals.append(f"its head lives in {head_repo or 'no repository'}, not {target}") + if branch in ("develop", "main", default): + refusals.append(f"its head branch {branch!r} is a permanent branch") + if refusals: + print(f"status=REFUSED nothing was deleted from {target} #{num}: {', and '.join(refusals)}") + return 1 + + ref = cleanup_ref(owner, repo, branch) + if ref is None: + print(f"status=ALREADY_GONE branch={branch} is not on {target}, so nothing was deleted") + return 0 + rules = branch_rules(owner, repo, branch) + if rules: + print( + f"status=REFUSED nothing was deleted: {branch!r} on {target} is covered by " + f"{', '.join(rules)}, so its delete is the maintainer's call" + ) + return 1 + tip = ref["target"]["oid"] + if tip != head: + print( + f"status=TIP_MOVED nothing was deleted: {branch!r} on {target} is at {tip}, and " + f"#{num} merged {head}, so something reached the branch after the merge or the name " + "was reused" + ) + return 1 + + print(f"deleting: {branch} on {target} at {tip}") + try: + response = shared.request(M_DELETE_REF, {"refId": ref["id"]}).data + print(f" response: {json.dumps(response)[:400]}") + except shared.GraphQLError as exc: + print(f" the delete reported a failure: {exc}") + sys.stderr.write(exc.detail) + try: + after = cleanup_ref(owner, repo, branch) + except CleanupUnread as exc: + print( + f"status=DELETE_NOT_CONFIRMED the delete of {branch!r} on {target} was sent, and " + f"reading the ref back did not answer: {exc}" + ) + return 2 + if after is not None: + print( + f"status=DELETE_NOT_CONFIRMED {branch!r} on {target} still reads at " + f"{after['target']['oid']} after the delete" + ) + return 2 + print(f"status=DELETED branch={branch} oid={tip}, confirmed by reading the ref back") + return 0 + + def utf8_console() -> None: """Write the console streams as UTF-8, whatever the host code page is. @@ -5586,7 +5804,9 @@ def utf8_console() -> None: def main(argv: list[str] | None = None) -> int: utf8_console() ap = argparse.ArgumentParser() - ap.add_argument("cmd", choices=["attest", "claims", "comment", "status", "reply", "wait"]) + ap.add_argument( + "cmd", choices=["attest", "claims", "cleanup", "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. @@ -5738,6 +5958,9 @@ def main(argv: list[str] | None = None) -> int: if a.cmd == "claims": return check_claims(owner, repo, a.number) + if a.cmd == "cleanup": + return cleanup(owner, repo, a.number) + if a.cmd == "attest": return attest(owner, repo, a.number, a.checkout or ".") diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index df3dba642..f5512b451 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -9665,17 +9665,19 @@ def test_the_runbook_names_partial_coverage_as_a_state_that_blocks_a_merge(self) self.assertIn("A round reporting partial coverage of the diff blocks the merge", text) self.assertIn("a coverage statement that does not reach this head", text) - def test_the_only_writes_are_the_four_named_here(self) -> None: - """Every write this script makes is one of four, and each arrived as a reviewed change. + def test_the_only_writes_are_the_five_named_here(self) -> None: + """Every write this script makes is one of five, and each arrived as a reviewed change. The read subcommands are the bulk of it and a mutation reaching them is a digest that writes, so the whole-source guard stays and is narrowed to the documents the script actually needs rather than dropped when the first of them arrived. `requestReviews` closes the gap where `wait` only polled and never asked. `addComment` owns body-only - finding responses. The reply and resolve pair owns inline threads. `union:true` only + finding responses. The reply and resolve pair owns inline threads. `deleteRef` is + `cleanup`'s, by an id read in the same run, so no REST verb enters for it. `union:true` only adds to the request set, so it cannot drop a requested human reviewer. `docs/pr-reviewer-reference.md` keeps the `union:false` clear-and-recover form as a - manual operation, the runbook carrying no mutation of its own. + manual operation, the runbook carrying no mutation of its own. A sixth document arriving + is a write nobody reviewed as one rather than a style drift. """ source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8") for verb in ( @@ -9702,8 +9704,8 @@ def test_the_only_writes_are_the_four_named_here(self) -> None: "reviewer is pending, which no script can decide", ) # `mutation(` opens a document, so the count is the number of documents. - # A fifth arriving is a write nobody reviewed as one rather than a style drift. - self.assertEqual(4, source.count("mutation(")) + self.assertEqual(5, source.count("mutation(")) + self.assertIn("deleteRef", source) self.assertIn("addComment", source) self.assertIn("addPullRequestReviewThreadReply", source) self.assertIn("resolveReviewThread", source) @@ -10236,6 +10238,7 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin ], "wait": ["wait", "7", "--repo", "someone-else/r"], "attest": ["attest", "7", "--repo", "someone-else/r"], + "cleanup": ["cleanup", "7", "--repo", "someone-else/r"], } # The parser's remaining `cmd` choices, none of which write. @@ -10264,12 +10267,16 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No mock.patch.object(pr_review, "origin_owner", return_value="acme"), mock.patch.object(pr_review, "gql", side_effect=boom) as gql, mock.patch.object(pr_review, "gh_graphql", side_effect=boom) as gh_graphql, + mock.patch.object(pr_review.shared, "request", side_effect=boom) as request, + mock.patch.object(pr_review, "gh_rest", side_effect=boom) as rest, ): code = pr_review.main(argv) self.assertEqual(64, code) self.assertIn("status=OUT_OF_SCOPE", out.getvalue()) gql.assert_not_called() gh_graphql.assert_not_called() + request.assert_not_called() + rest.assert_not_called() class TestRequiredContexts(unittest.TestCase): @@ -10806,5 +10813,249 @@ def test_the_dedupe_falls_back_to_the_app_slug_for_a_null_workflow_run(self) -> self.assertEqual([("lint", "FAILURE"), ("lint", "SUCCESS")], kept) +CLEANUP_HEAD = "a" * 40 +CLEANUP_BRANCH = "feature/example-topic" + + +def cleanup_pr(**overrides: object) -> dict: + """A merged pull request into `develop` from a head in o/r, every field overridable.""" + pr: dict[str, object] = { + "merged": True, + "baseRefName": "develop", + "headRefName": CLEANUP_BRANCH, + "headRefOid": CLEANUP_HEAD, + "headRepository": {"nameWithOwner": "o/r"}, + } + pr.update(overrides) + return pr + + +class CleanupCase(unittest.TestCase): + """Drive `cleanup` against crafted reads, so no case reaches GitHub or deletes anything.""" + + def setUp(self) -> None: + self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) + self.enterContext(contextlib.redirect_stderr(io.StringIO())) + self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="o")) + self.enterContext(mock.patch.object(pr_review, "READ_RETRY_DELAYS", ())) + self.enterContext( + mock.patch.object(pr_review.handoff, "registry", return_value=("o", {"r"})) + ) + self.docs: list[tuple[str, dict]] = [] + self.rest: list[str] = [] + + def wire( + self, + pr: dict | None = None, + default: str = "main", + refs: list[object] | None = None, + rules: object = (), + protected: object = False, + delete_fails: bool = False, + ) -> None: + """Answer each document in turn, `refs` holding one answer per ref read in order. + + A ref answer is a tip, None for no such branch, or an exception the read raises. + """ + pending = list(refs if refs is not None else [CLEANUP_HEAD, None]) + + def fake(query: str, variables: dict, **_: object) -> pr_review.shared.Response: + self.docs.append((query, variables)) + if "pullRequest(number" in query: + repository = {"defaultBranchRef": {"name": default}, "pullRequest": pr} + return pr_review.shared.Response({"repository": repository}, []) + if "deleteRef" in query: + if delete_fails: + raise pr_review.shared.GraphQLError("gh graphql reported errors") + return pr_review.shared.Response({"deleteRef": {"clientMutationId": None}}, []) + if "qualifiedName" in query: + answer = pending.pop(0) + if isinstance(answer, Exception): + raise answer + ref = None if answer is None else {"id": "REF_read", "target": {"oid": answer}} + return pr_review.shared.Response({"repository": {"ref": ref}}, []) + raise AssertionError(f"unexpected document: {query[:60]}") + + def rest( + path: str, jq: str | None = None, raw: bool = False + ) -> subprocess.CompletedProcess: + self.rest.append(path) + if "/rules/branches/" in path: + body = rules + else: + body = {"name": CLEANUP_BRANCH, "protected": protected} + if isinstance(body, Exception): + return subprocess.CompletedProcess([], 1, "", "gh: Not Found (HTTP 404)") + text = json.dumps(list(body) if isinstance(body, tuple) else body) + return subprocess.CompletedProcess([], 0, text, "") + + self.enterContext(mock.patch.object(pr_review.shared, "request", side_effect=fake)) + self.enterContext(mock.patch.object(pr_review, "gh_rest", side_effect=rest)) + + def run_cleanup(self, repo: str = "o/r") -> int: + return pr_review.main(["cleanup", "7", "--repo", repo]) + + def deleted(self) -> bool: + return any("deleteRef" in q for q, _ in self.docs) + + +class TestCleanupDeletesOnlyAVerifiedMergedBranch(CleanupCase): + def test_a_matching_tip_is_deleted_by_the_id_just_read_and_confirmed_gone(self) -> None: + """Two exact reads by qualified name, the tip before the delete and the read back after.""" + self.wire(cleanup_pr()) + self.assertEqual(0, self.run_cleanup()) + delete = next(v for q, v in self.docs if "deleteRef" in q) + self.assertEqual({"refId": "REF_read"}, delete) + reads = [v for q, v in self.docs if "qualifiedName" in q] + self.assertEqual([f"refs/heads/{CLEANUP_BRANCH}"] * 2, [v["q"] for v in reads]) + self.assertIn("status=DELETED", self.out.getvalue()) + + def test_a_branch_already_gone_is_done_and_deletes_nothing(self) -> None: + self.wire(cleanup_pr(), refs=[None]) + self.assertEqual(0, self.run_cleanup()) + self.assertFalse(self.deleted()) + self.assertIn("status=ALREADY_GONE", self.out.getvalue()) + + def test_a_moved_tip_refuses_and_names_both_commits(self) -> None: + moved = "b" * 40 + self.wire(cleanup_pr(), refs=[moved]) + self.assertEqual(1, self.run_cleanup()) + self.assertFalse(self.deleted()) + out = self.out.getvalue() + self.assertIn("status=TIP_MOVED", out) + self.assertIn(moved, out) + self.assertIn(CLEANUP_HEAD, out) + + def test_a_ref_still_there_after_the_delete_is_not_reported_deleted(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD]) + self.assertEqual(2, self.run_cleanup()) + self.assertTrue(self.deleted()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + self.assertNotIn("status=DELETED", self.out.getvalue()) + + def test_a_read_back_that_does_not_answer_is_not_reported_deleted(self) -> None: + unread = pr_review.shared.GraphQLError("gh graphql failed rc=1") + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, unread]) + self.assertEqual(2, self.run_cleanup()) + self.assertTrue(self.deleted()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + + def test_a_delete_reporting_failure_is_decided_by_the_read_back(self) -> None: + """A failed-looking delete may have landed, so the ref read decides rather than the error.""" + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, None], delete_fails=True) + self.assertEqual(0, self.run_cleanup()) + out = self.out.getvalue() + self.assertIn("the delete reported a failure", out) + self.assertIn("status=DELETED", out) + + def test_a_delete_reporting_failure_with_the_ref_still_there_exits_2(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD], delete_fails=True) + self.assertEqual(2, self.run_cleanup()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + + +class TestCleanupRefusesBeforeAnyDelete(CleanupCase): + def assert_refused(self, code: int = 1, status: str = "status=REFUSED") -> None: + self.assertEqual(code, self.run_cleanup()) + self.assertFalse(self.deleted()) + self.assertIn(status, self.out.getvalue()) + + def test_an_unmerged_pull_request_is_refused(self) -> None: + self.wire(cleanup_pr(merged=False)) + self.assert_refused() + + def test_a_base_other_than_develop_is_refused(self) -> None: + self.wire(cleanup_pr(baseRefName="main")) + self.assert_refused() + + def test_a_head_in_another_repository_is_refused(self) -> None: + self.wire(cleanup_pr(headRepository={"nameWithOwner": "o/r-fork"})) + self.assert_refused() + + def test_a_head_repository_gone_is_refused(self) -> None: + self.wire(cleanup_pr(headRepository=None)) + self.assert_refused() + + def test_a_develop_head_is_refused(self) -> None: + self.wire(cleanup_pr(headRefName="develop")) + self.assert_refused() + + def test_a_main_head_is_refused_where_main_is_not_the_default(self) -> None: + self.wire(cleanup_pr(headRefName="main"), default="trunk") + self.assert_refused() + + def test_the_default_branch_read_live_is_refused(self) -> None: + self.wire(cleanup_pr(headRefName="trunk"), default="trunk") + self.assert_refused() + + def test_a_ruleset_on_the_branch_is_refused(self) -> None: + self.wire(cleanup_pr(), rules=[{"type": "deletion"}]) + self.assert_refused() + self.assertIn("deletion", self.out.getvalue()) + + def test_classic_branch_protection_is_refused(self) -> None: + self.wire(cleanup_pr(), protected=True) + self.assert_refused() + + def test_an_unread_rule_set_stops_at_2_rather_than_reading_as_none(self) -> None: + self.wire(cleanup_pr(), rules=RuntimeError("unread")) + self.assert_refused(2, "status=UNREAD") + + def test_a_rule_read_that_is_not_a_list_stops_at_2(self) -> None: + self.wire(cleanup_pr(), rules={"message": "Upgrade to GitHub Pro"}) + self.assert_refused(2, "status=UNREAD") + + def test_an_unread_protection_flag_stops_at_2(self) -> None: + self.wire(cleanup_pr(), protected=None) + self.assert_refused(2, "status=UNREAD") + + def test_a_branch_name_that_changes_the_rest_path_stops_at_2(self) -> None: + self.wire(cleanup_pr(headRefName="feature/a?b"), refs=[CLEANUP_HEAD]) + self.assert_refused(2, "status=UNREAD") + self.assertEqual([], self.rest) + + def test_an_unread_pull_request_stops_at_2(self) -> None: + self.wire(None) + self.assert_refused(2, "status=UNREAD") + + def test_an_unread_tip_stops_at_2(self) -> None: + self.wire(cleanup_pr(), refs=[pr_review.shared.GraphQLError("gh graphql failed rc=1")]) + self.assert_refused(2, "status=UNREAD") + + def test_an_owner_other_than_the_checkout_is_out_of_scope_before_any_read(self) -> None: + self.wire(cleanup_pr()) + self.assertEqual(64, self.run_cleanup(repo="x/r")) + self.assertEqual([], self.docs) + self.assertIn("status=OUT_OF_SCOPE", self.out.getvalue()) + + def test_an_owner_other_than_the_registry_is_refused_before_any_read(self) -> None: + self.wire(cleanup_pr()) + with mock.patch.object(pr_review.handoff, "registry", return_value=("p", {"r"})): + self.assertEqual(1, self.run_cleanup()) + self.assertEqual([], self.docs) + self.assertIn("status=REFUSED", self.out.getvalue()) + + def test_an_unregistered_repository_that_is_not_a_fork_is_refused(self) -> None: + self.wire(cleanup_pr()) + flags = {"hasIssuesEnabled": True, "isFork": False} + with mock.patch.object(pr_review.handoff, "gh_json", return_value=flags): + self.assertEqual(1, self.run_cleanup(repo="o/unlisted")) + self.assertEqual([], self.docs) + + def test_an_unregistered_fork_under_the_owner_is_admitted(self) -> None: + self.wire(cleanup_pr(headRepository={"nameWithOwner": "o/forked"})) + flags = {"hasIssuesEnabled": True, "isFork": True} + with mock.patch.object(pr_review.handoff, "gh_json", return_value=flags): + self.assertEqual(0, self.run_cleanup(repo="o/forked")) + self.assertTrue(self.deleted()) + + def test_an_unreadable_registry_stops_at_2(self) -> None: + self.wire(cleanup_pr()) + failure = pr_review.handoff.Execution("could not read the fleet registry") + with mock.patch.object(pr_review.handoff, "registry", side_effect=failure): + self.assertEqual(2, self.run_cleanup()) + self.assertEqual([], self.docs) + + if __name__ == "__main__": unittest.main(verbosity=2)