diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index e14a8e6b..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 @@ -52,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 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, 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 + 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 @@ -143,20 +147,23 @@ 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 + 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 @@ -185,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 @@ -199,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 c52b2a67..aed1424f 100644 --- a/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct +++ b/.claude-plugin/fleet-skills/.source-digests/pr-review-conduct @@ -1 +1 @@ -e611a6183eb65c72 +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 e14a8e6b..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 @@ -52,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 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, 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 + 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 @@ -143,20 +147,23 @@ 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 + 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 @@ -185,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 @@ -199,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 4fbb79f2..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. 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. 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 e14a8e6b..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 @@ -52,12 +57,11 @@ visible comments, routinely still carries a finding nobody has answered. Treatin 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, 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 + 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 @@ -143,20 +147,23 @@ 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 + 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 @@ -185,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 @@ -199,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 51e8ab1b..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. 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 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/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/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 d3bd8bde..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. 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 11df02c2..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 @@ -31,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 @@ -65,9 +81,10 @@ A refusal naming the account quota still reads as absent here, exit 0, since a refusal covers no head either. Its printed digest line carries `refusal=QUOTA` regardless, and `refusal=ERROR` for an error refusal whose run log names no rate limit or could not be - read. Either field reads a refusal on an earlier head where it is the pull request's - newest Copilot review and nothing covers the head. `wait` is where that state gets its - own exit codes, 46 and 47 below, because only `wait` is the command a caller might + 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 @@ -168,21 +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 is also skipped under 46's and 47's quota readings - below, since a request into a reached limit spends quota and returns the same refusal. It - reads the Copilot reviewer's bot id from the repository's own most recently updated PRs - rather than a fixed id: the last HISTORY_PRS, widened once to HISTORY_PRS_WIDE where that - narrow window carries no Copilot activity at all, since an outage that outlasts - HISTORY_PRS PRs would otherwise empty it on every call for as long as the outage runs. - Requests nothing (falling back to polling only) where both windows come up empty, since a - repository with no Copilot review in either has nothing to read the id from and a - fabricated one is never an option. The loop runs in-process, so a 45-minute wait costs - one agent turn, not 90. - Exit 0 = review present, 30 = still pending at timeout (pending is not failure), + 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, 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. @@ -230,6 +254,10 @@ 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. @@ -503,6 +531,7 @@ def strip_fences( # Not a state a round reports, but the reading that carries an earlier round's forward. 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. @@ -515,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. @@ -763,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 @@ -836,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. @@ -3097,6 +3139,9 @@ def digest( ) 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. @@ -3148,10 +3193,7 @@ 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) or stopping_refusal(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" @@ -3204,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 "") @@ -3363,6 +3405,13 @@ def digest( ) elif cover == UNSTATED and on_head: 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. @@ -3731,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: @@ -4008,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. @@ -4050,6 +4330,12 @@ def main(argv: list[str] | None = None) -> int: "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", type=int, @@ -4086,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. @@ -4099,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 []) @@ -4135,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) @@ -4179,10 +4478,16 @@ 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 done or answer or drift - else stopping_refusal(gql(Q_FULL, owner, repo, a.number)) + 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. @@ -4195,6 +4500,7 @@ def main(argv: list[str] | None = None) -> int: and not drift and not signal and not stopped + and not held and not reviewer_requested(pr) ): line, recorded = request_copilot_review( @@ -4213,6 +4519,13 @@ def main(argv: list[str] | None = None) -> int: "request, no pending reviewer and no review-request event, so this wait stops here " "rather than polling --timeout out against a request that does not exist." ) + elif 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 " @@ -4276,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. @@ -4344,6 +4664,14 @@ def main(argv: list[str] | None = None) -> int: "--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/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 e8e713ce..e25a0870 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -5733,6 +5733,163 @@ def test_ignore_quota_signal_requests_past_a_current_head_refusal(self) -> None: 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)]) @@ -7316,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. @@ -7352,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.