Repository navigation
Read a Per-Repository Review Policy Before Asking Any Bot for a Review - #3881
Merged
Merged
Conversation
Each pull request bot costs money or agent time per round, so which bots review which pull requests, when, and how often is now a per-repository choice. A repository states it in `.github/pr-review.json`, read from the pull request's base branch so a pull request cannot change its own policy. A repository carrying none reads `spec/pr-review.default.json`, which keeps the gate's behavior as it was, so nothing changes until a repository opts in. `wait` requests whatever the policy owes that has not started on its own. Copilot keeps its review-request mutation, and CodeRabbit and Qodo are now asked by comment. So the skills and the rule text no longer depend on whether a repository's reviews arrive automatically or on request, and a repository can be cut over, tested, or rolled back alone. A request can wait for the head's checks to pass first, which is the setting the push-then-build measurement compares. A spent Copilot cap hands a new head to an attested local pass. The CodeRabbit and Qodo reading and asking live in a new pr_review_side.py, so the gate stays under its size ceiling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The local strict pass on the review-policy change found that a stale request or answer could read as one on a head pushed later, since a commit's own timestamp is not its push. A head now arrives with its earliest check, and green is judged with the gate's own check shapes, so a head with no check posted yet is pending inside the grace rather than green. An automatic reviewer the policy gates on is no longer asked while it has begun or may still begin, which would have bought a second round. The policy read now retries through `reread`, so a transient failure no longer ends a run that never used to read it. A Copilot entry with `gate` off hands every head to the local pass. The schema and the reader are held equal by a test, and copilot now refuses `command` in both. The held-head notes and the docstrings no longer state that only a non-default base is held, and the skill text names the hub default as a default. The repo-config README still describes the ruleset as configure.sh applies it today, which the follow-up that makes configure.sh read the policy changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a Fresh Read The second local strict pass found that the side reviewers were judged on a stale or absent read. They were never asked once Copilot already covered the head, and they were judged after a wait for green checks on the read taken before it. Each is now judged on a fresh read, read again after that wait, and the liveness read carries the base so the policy rides it too. A head now arrives when GitHub created its first check suite, which happens at the push. A head with no suite yet reads as just arrived, so a request made before the checks start still counts for that head, and no check yet never reads as green. The policy read projects its content as JSON, so the retried read stops at the first good one rather than reading every retry. A side request held by failed checks exits 40 like the Copilot one, and an automatic Qodo round counts toward its cap. `mutated` moves to `gh_graphql.py`, so the gate and the side module share one comment-path helper, and the policy reader moves into `pr_review_policy.py`. The `--help` exit text names the two new statuses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The maintainer chose to simplify the comment-triggered reviewers after the third local strict pass. Making the merge wait on CodeRabbit or Qodo, and filling a missed automatic start, kept producing defects, because neither bot leaves a reliable record of which push it reviewed. So Copilot keeps the whole policy, its gate included, and CodeRabbit and Qodo are asked by comment once per new head when their policy says explicit, after green checks where it says so. The reader refuses `gate: true` for them, an automatic one is never asked, and the merge never waits on either. Their findings reach the digest as they always did. The asks now run before Copilot's own wait for green checks, so an on-open ask is never delayed behind it, and each reviewer is judged again after any wait. A check stuck in a shape no wait clears is named and exits 44 rather than polling the timeout out. The side-review gate, its poll, `status=SIDE_REVIEW_PENDING`, and the unused `committedDate` field are gone, and a wait-level test covers an ask on a head Copilot already covered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Owner
Author
|
A recorded local strict-review pass covers head |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Active correctness and documentation issues remain, including a critical path that can bypass the required green-check prerequisite and several moderate policy, deduplication, and request-ordering defects.
12 open issues
What changed in this PR
This pull request introduces repository-owned pull request review policies while preserving the existing hub default behavior. Policies control reviewer coverage, triggers, timing, round limits, and Copilot gating.
Changes:
- Adds the policy schema, default configuration, parser, base-branch loading, and validation tests.
- Integrates policy-aware Copilot, CodeRabbit, and Qodo requests, check holds, and new exit statuses.
- Updates governance documentation, review guidance, generated skills, digests, and size checks.
This pull request requires changes.
| File | Description |
|---|---|
tests/test_pr_review.py |
Updates existing gate fixtures and expectations for policy-aware payloads. |
tests/test_pr_review_size.py |
Adds the side-review module to size checks. |
tests/test_pr_review_policy.py |
Tests policy parsing, coverage, side reviews, check holds, and wait behavior. |
tests/test_pr_merge.py |
Recognizes the new pre-review check status. |
spec/pr-review.schema.json |
Defines the repository review-policy schema. |
spec/pr-review.default.json |
Defines the behavior-preserving default policy. |
scripts/README.md |
Documents policy behavior, reviewer requests, and exit statuses. |
scripts/pr_review.py |
Loads policies and coordinates reviewer requests and check holds. |
scripts/pr_review_side.py |
Implements CodeRabbit and Qodo requests and pre-review check handling. |
scripts/pr_review_rounds.py |
Applies policy-driven Copilot coverage and local-pass decisions. |
scripts/pr_review_policy.py |
Reads, validates, caches, and defaults repository policies. |
scripts/pr_review_answers.py |
Routes the new check status through redrive handling. |
scripts/gh_graphql.py |
Provides shared mutation-response traversal. |
GOVERNANCE.md |
Updates the provider-agnostic review-loop contract. |
docs/pr-reviewer-reference.md |
Documents policy-controlled reviewer triggers. |
.github/skills/pr-review-conduct/SKILL.md |
Updates the generated review-conduct guidance. |
.github/skills/drive-pr/SKILL.md |
Updates the generated pull-request driving guidance. |
.github/copilot-instructions.md |
Documents policy-controlled Copilot coverage. |
.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md |
Carries the generated Claude review-conduct skill. |
.claude-plugin/fleet-skills/skills/drive-pr/SKILL.md |
Carries the generated Claude pull-request driving skill. |
.claude-plugin/fleet-skills/.source-digests/pr-review-conduct |
Updates the generated review-conduct source digest. |
.claude-plugin/fleet-skills/.source-digests/drive-pr |
Updates the generated driving-skill source digest. |
.agents/skills/pr-review-conduct/SKILL.md |
Updates the authoritative review-conduct skill. |
.agents/skills/drive-pr/SKILL.md |
Updates the authoritative pull-request driving skill. |
Review effort: Lite
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3881 +/- ##
===========================================
+ Coverage 83.31% 83.69% +0.37%
===========================================
Files 29 32 +3
Lines 9956 10290 +334
===========================================
+ Hits 8295 8612 +317
- Misses 1661 1678 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Copilot's first round on the review-policy pull request found these, each fixed here:
- `checks_hold` dropped a check node it could not read, so a rollup holding only one read as
green. An unread node now holds the review as `UNREAD`.
- The Copilot auto-request asked whether the policy requests Copilot but not whether it covers
the base. It now asks both, through `copilot_wanted`.
- A side reviewer waiting for green checks blocked the Copilot request. The side asks now run
in two phases, on-open before Copilot's own wait and after-checks after its request. A
reviewer already asked or answered is skipped before any wait.
- `{"version": true}` passed, since `True == 1`. A boolean version is refused.
- Before a head's first suite, an ask read as never made, so the next run asked again. The
head's commit time now bounds it from below until a suite exists.
- The rule and skill text claimed `wait` requests every review the policy owes. It now says
`wait` sends each request the policy asks for, and that an automatic bot starts on its own.
The README's automatic-Copilot sentence and its exit-44 row are corrected, and the `--help`
text moves the new 44 case under the 44 entry.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The local strict pass on the review-round fixes found two of them wrong. The base check added to the Copilot auto-request skipped the round `wait` forces on a pull request into develop that carries a partial round or a truncated history. The held-head logic already excludes every base the policy does not cover, so the check is reverted. Staged test payloads now carry the hub default policy, so every command-level case runs the path a live run takes. The commit-time fallback for a side ask read a field the live query never fetched. The query fetches `committedDate` again, and an ask now counts for the head where it postdates the head's commit, the one stamp that never moves once the head exists. The GOVERNANCE, drive-pr, and pr_review_policy wording now says `wait` sends each request the policy asks for, and that only a gated Copilot has a missed start filled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
A recorded local strict-review pass covers head |
The reader and the schema now refuse a Copilot policy that is explicit with gate false, since wait requests Copilot only for a round the merge waits on. Where the policy has Copilot off or ungated, a partial round or a truncated history no longer stops a local pass from covering the head, so wait no longer polls its timeout for a round it never asked for. The comment, attest and claims reads skip the policy fetch they never consult. Tests cover green checks reaching an after-checks request and --request skipping the hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Where Copilot is off, ungated, or past its round cap, no Copilot round is coming, so its refusals, partial rounds, pending requests, and cut history on the head no longer stop an attested local pass from covering it. A spent cap with a partial round on record therefore no longer requests a paid round past the cap. A capped side reviewer is not asked once the comments run past the read window, since its rounds cannot then be counted. The hosted reviewer check predicate is shared, and the no-op policy flags on reads that never select a base are dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
A recorded local strict-review pass covers head |
Answers findings 1 to 8 of the local pass over 9946909. - wait reads past a head refusal where copilot_settled holds, and drops it at the exit, so a settled head exits as status reads it. - checks_hold reads an open workflow run as PENDING, as held_checks_open does, so a request never goes out between needs: stages. - The after-checks side asks run after the Copilot poll, and a held Copilot request is judged again on a fresh read after the check wait. - The exit-49, excluded-files, and explicit-trigger texts, the maxRounds description, and the skill, Copilot-instructions, README, and reference statements now say what copilot_settled waives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Answers findings 9 to 13 of the local pass over 9946909. - pr_review_payload.py holds the policy's payload key and its two repeated reads, the default branch and a bot's entry. promotion, copilot_settled, side_reviewers, and copilot_policy share them, and pr_review_policy's PAYLOAD_KEY is that key. - copilot_rounds becomes reviewed_heads(pr, login), which CodeRabbit's round count reuses. - The whole-source write guards scan pr_review_side.py and pr_review_policy.py too, and the new module joins the reader set. - pr_review_policy.py and pr_review_payload.py get size ceilings, and pr_review_rounds.py drops to 695 lines of its 700. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Answers six of the seven findings of the local pass over e4622ee. - A side ask on a pull request no longer open is skipped, since a peer can merge or close it while wait polls. - A capped bot whose review history runs past the read window counts as past its cap, so wait never requests a Copilot round over a cap it cannot count. - A conflicted head with no check reads DIRTY and exits 44 naming the conflict, rather than PENDING until the timeout. - An answer outside a review no longer keeps a settled Copilot's head from its local pass, so it exits 49 rather than 40. - The after-checks side ask reads the checks once rather than polling, so no wait that says it stops goes on polling. - wait's usage text names the side asks and the held Copilot request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
A recorded local strict-review pass covers head |
This was referenced Nov 5, 2026
ptr727
added a commit
that referenced
this pull request
Nov 5, 2026
…mes in UTC (#3888) ## Why #3881 left two defects in `scripts/pr_review_side.py`, and #3885 adds two design items and a documentation gap. Each sits on the review-cutover track (#3877), and the pilot needs the first one fixed before any repository gives CodeRabbit or Qodo `when: after-checks`. ## What - **An after-checks side reviewer is asked once CI settles (#3885 item 1).** `side.ask` read the head's checks once, after the Copilot poll. So where a Copilot round already covered the head, or landed first, a check still running gave `PENDING` and only a note. Nothing asked later, since the merge gate's `run_wait` passes `may_request=False`. Now a wait that polls for a Copilot round hands the ask the `await_green` partial, which polls the checks on the wait's own clock and is bounded by `--timeout`. Every other wait reads them once. That covers the checks-hold, unrecorded-request, quota and refusal stops, exit 49, and a held head, whose own poll already waits on the checks. A wait that polled re-reads the payload before its digest and exit code, so a check that went green during the side poll is not judged stuck from a stale read. A pull request merged or closed while its checks settled is asked nothing. - **Ask times compare as instants (#3887).** `side_asked_on_head` compared a comment's UTC `createdAt` with the head's `committedDate` as text, and a `GitTimestamp` keeps the committer's offset. Both now go through `pr_review_checks.utc_instant`. - **Exit 44 names its DIRTY and unreadable holds (#3885 item 2)**, in the `wait` usage text and `scripts/README.md`. The exit-0 text now also names the after-checks exception to "does not poll the checks". - **`PAYLOAD_KEY` is gone (#3885 item 3).** Callers read `pr_review_payload.POLICY_KEY`, and the test asserting the two agree is dropped. - **One post-and-confirm helper (#3885 item 4).** `side.post_comment` posts a conversation comment and says whether its URL and exact body confirm it. `comment_on_pr` and the side asks both call it. `scripts/pr_review.py` sits at 2199 of its 2200-line ceiling after this change. The next change that adds logic there has to move some first, as #3880 plans. Refs #3885, #3887, #3877. The closing keywords belong on the develop -> main promotion PR. ## Verification - The full suite, `uv run --no-project --with-requirements tests/requirements.txt python -m unittest discover -s tests`, ran 3539 tests with 11 skipped, all passing, at 6bd9a5c. The `test_pr_review*` suites pass at the head, 709 tests. - The new `TestSideAskTiming` cases in `tests/test_pr_review_policy.py` were each run against the pre-change scripts or with their fix reverted, and each one failed. Those are the outlasting-CI ask, the poll hand-off per wait path, the merged-during-poll ask, the fresh digest read, and both UTC offsets. - ruff format and check are clean. The pre-commit hook's mypy, prose and EOL gates pass. ## Local-review findings, disposed Three recorded `local-strict-review` passes at the `frontier` tier covered the branch. The third, at 08cb68a, raised nothing. - `introduced`, fixed in bc8e6ae: the digest and exit code came from a payload read before the side poll, so a check that went green during it could read as `NOT_PICKED_UP` and exit 44. - `introduced`, fixed in bc8e6ae: a held head handed the ask a poll, which could delay exit 46 by up to `--timeout`. - `introduced`, fixed in bc8e6ae: a pull request merged while its checks settled could still be asked. - `introduced`, fixed in bc8e6ae: the usage text and the README `0` row still said a covered head's exit 0 never polls the checks. - `introduced`, fixed in 08cb68a: the usage text and README still said any polling wait awaits the checks after the held-head change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ptr727
added a commit
that referenced
this pull request
Nov 6, 2026
…t Policy (#3892) ## Summary Promotes #3881, #3884, #3886, #3888, #3889, and #3891 to `main`. Closes #3200. Closes #3885. Closes #3887. - **Per-repository review policy (#3881).** `scripts/pr_review.py` reads which bots review a repository, on which bases, when, and how often, from the repository's own policy, with the hub default reproducing today's behavior. - **Review return on cost (#3886).** `scripts/backlog_metrics.py` measures reviewer cost per useful finding. It is now a uv script, so the metrics server's weekly timer must run it as `uv run --no-project --script scripts/backlog_metrics.py` from this release on. - **After-checks side reviewer (#3888).** An explicit after-checks CodeRabbit or Qodo is asked once CI settles, and ask times compare in UTC. - **Commit in place for an operational repository (#3889).** The policy file became `.github/agent-policy.json`, with its schema under `review` and a `guard` section that lets `gh-write-guard` allow an in-place commit on a listed branch. - **Agent policy rendered from the registry (#3891).** `repo-config/configure.sh` renders `.github/agent-policy.json` from `registry/repos.json` and applies the bot settings it implies: the Copilot ruleset rule, CodeRabbit's auto-review keys, and Qodo's auto-feedback key. Once this is on `main`, `configure.sh check` reports agent-policy drift on every repository until `apply --checkout` has been run there. - **Dependabot (#3884).** The actions-deps group bump. Each feature pull request carried recorded local strict review passes, with every finding fixed, declined with evidence, or filed (#3890). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is the first step of the revised review cutover on #3877. Which bots review a repository's pull requests, on which bases, when, and how often is now a setting in that repository's own
.github/pr-review.json. A repository carrying none reads the hub'sspec/pr-review.default.json. That default reproduces the gate's current behavior exactly, so this pull request changes nothing until a repository opts in. Each repository can then be cut over, tested, or rolled back on its own, and the reviewer cost is tuned per repository.spec/pr-review.schema.json, andscripts/pr_review_policy.pyreads and validates it. A test holds the two equal. The policy is read from the pull request's base branch, so a pull request cannot change the rules its own review runs under.triggerisauto,explicit, oroff.baseslists the branches it reviews.whencan hold the request until the head's checks pass. That makes the push, wait for a green build, then review question a per-repository setting the metrics can compare.roundsandmaxRoundscap the rounds. A spent cap hands a new head to an attested local pass.gatedecides whether the merge waits on Copilot. AnexplicitCopilot must gate, and the reader and schema refuse one that does not, sincewaitrequests Copilot only for a round the merge waits on.off, ungated, or past its cap, no Copilot round is coming. So its refusals, partial rounds, pending requests, and cut history no longer stop an attested local pass from covering the head.explicit, each is asked by comment once per new head:@coderabbitai review, orfull review, and/review. The ask can optionally wait for green checks. The merge never waits on either, and the reader refusesgate: truefor them. Their findings reach the digest as they always did.waitrequests what the policy owes, and the text names the hub default as a default.GOVERNANCE.md"PR Review Etiquette",drive-pr,pr-review-conduct,.github/copilot-instructions.md,docs/pr-reviewer-reference.md, andscripts/README.mdare updated, and the generated skill trees regenerated.status=CHECKS_BEFORE_REVIEWexits 30 while checks run, 40 when they fail, and 44 when one is stuck or the head is conflicted. It routes toredrive.scripts/pr_review_side.py, and the policy reads off a payload in a newscripts/pr_review_payload.py, sopr_review.pystays under its size ceiling.mutatedmoves toscripts/gh_graphql.pyso both modules share it.Not in this pull request:
repo-config/configure.shapplying each repository's policy to the bot-side settings. That covers the Copilot ruleset rule and the CodeRabbit and Qodo files, through a local checkout.feature/review-metricspull request.Verification
uv run --no-project --with-requirements tests/requirements.txt python -m unittest discover -s tests -p 'test_*.py'. The gate tests that predate the policy pass unchanged under the default, which shows the change is behavior-neutral there.tests/test_pr_review_policy.py, new, covers parsing and refusals, schema parity, base matching, caps, and the Copilot cover decision with the default proven equal to the old one. It also covers head arrival, check holds, and the side asks. Wait-level cases cover a red build exiting 40 without a request, green checks reaching the request, and--requestskipping the hold.develop, and the eol gate all pass.Open findings from the local strict pass (edit budget spent)
The fourteen findings listed for 9946909 are answered. Thirteen are fixed in ad9feaa and e4622ee, and the boolean
versionrefusal is kept, sinceTrue == 1would otherwise parse as version 1. Of the seven from the pass over e4622ee, six are fixed in 67612c8. The rest are open, allintroduced:scripts/pr_review_side.pyaround line 279: an explicit after-checks CodeRabbit or Qodo is never asked when CI outlasts Copilot's round. The ask reads the checks once after the Copilot poll and gets PENDING.waitthen exits 0, and the merge gate's ownwaitrequests nothing.scripts/pr_review_side.pyaround line 75: a DIRTY or unreadable rollup exits 44 throughchecks_status. The wait usage text and the README exit table give 44 only for a stuck check. The held path documents a DIRTY head as 45.side_asksinscripts/pr_review_side.pyrepeats the trusted-and-shown filter thattrusted_commentsinscripts/pr_review_rounds.pyowns. This belongs with Share the Attestation Marker and Reviewer Logins Between the Gate and the Metrics Report #3880, below.PAYLOAD_KEY = POLICY_KEYinscripts/pr_review_policy.pygives one constant a second name. The test asserting the two agree, and the docstring sentences explaining them, go with it.request_side_reviewsinscripts/pr_review_side.pyrepeats the post-and-confirm thatcomment_on_prinscripts/pr_review.pydoes.Dropped findings
ruff checkreporting E402 onmutated = shared.mutated: disproved, sinceruff check scripts/pr_review.pywith the pinned config passes.Deferred
side_asksandtrusted_commentson one node-level trusted-comment reader belongs with it.pr_review_rounds.pyhas no headroom for that here.Refs #3877, #3758.
🤖 Generated with Claude Code