Make pr_review.py wait Request a Review, Not Just Poll for One - #685
Merged
Conversation
Twice in one session, wait polled the full 45-minute timeout for a review nothing had asked for: once after a push superseded the prior request, once after a re-request attempt that never actually issued a mutation. wait itself never requested anything, contrary to what a caller reasonably expects from a command named wait, since the manual requestReviews recipe documented in .github/copilot-instructions.md is a separate step every caller has to remember on every push. wait now issues that request itself, once, before its first poll, whenever nothing is already outstanding and the head is not already covered. The bot id is read fresh from the repository's own last 20 PRs every time (never fabricated, never cached across runs), and requests are additive (union:true) so a human reviewer requested alongside Copilot is never dropped. A repository with no Copilot review anywhere to read the id from falls back to plain polling rather than crashing or guessing. This is a second write path this script now owns, deliberately: it was previously excluded by name in test_the_only_writes_are_the_two_the_reply_path_owns testing an explicit prior design decision (confirmed against .github/copilot-instructions.md's own text, which already described requestReviews as a manual, undocumented-by-this-script mutation). Renamed the test and updated its assertions for the third deliberate write, added its mutation and runbook-mention checks alongside the existing two, and added a same-string ban on union:false, since the destructive clear-and-recover form stays a manual runbook step, never something this script issues on its own. Added targeted tests for all three auto-request shapes (bot id resolved and the request fires, no bot id found anywhere and it falls back to polling, already requested and it is not re-requested), each verified to fail against the pre-fix code before trusting them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates scripts/pr_review.py wait to proactively request a GitHub Copilot review (via requestReviews) when no review is outstanding and the head SHA is not yet covered, reducing cases where wait times out simply because no request was ever issued.
Changes:
- Extend the live GraphQL query to include the PR node id and add an auto-request step before the first polling loop iteration.
- Add GraphQL documents to discover the Copilot reviewer bot id from recent PR reviews and to issue the
requestReviewsmutation usingunion:true. - Update and add unit tests to cover the new auto-request behavior and to guard the script’s mutation surface.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/pr_review.py | Implements auto-request logic for wait, adds bot-id discovery query and the requestReviews mutation. |
| scripts/test_pr_review.py | Updates payload shapes and adds/adjusts tests to validate the new auto-request flow and mutation guardrails. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
request_copilot_review()'s docstring claimed it never raises, but it calls gh_graphql() directly (via copilot_bot_id() and the mutation), which does raise on a genuine API failure. Corrected the docstring: it raises the same as every other write in this script, since silently swallowing that into a polling fallback would recreate the exact blind-wait failure this function exists to prevent, just from a different cause. The one quiet path stays finding no bot id at all, which is not a failure. Q_BOT_ID's comment claimed the bot id comes from the 'most recent PRs' reviews', but reviews(first:20) fetches the oldest reviews in each PR's own connection, not the newest. Functionally harmless (the id is invariant per account, so which review it is read from never matters once one is found), but the claim was wrong. Reworded to describe what each part of the query actually does instead. Both caught by Copilot's own review of this PR, requested by the very wait fix this PR ships (the auto-request fired for real and a review landed in under 3 minutes), the first live dogfood test of the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/pr_review.py:40
- The module docstring still describes
replyas "The only writing path here", butwaitnow issues arequestReviewsmutation as well. Update the docstring so it no longer claimsreplyis the only state-changing subcommand.
wait Request a review where none is outstanding, then poll until Copilot's review lands
on the current head, then print the digest. The auto-request is skipped once a
review already covers the head, once Copilot has already answered outside a formal
review, or once one is already in the pending request set, so calling `wait` again on
the same PR never double-requests. It reads the Copilot reviewer's bot id from the
The --help text for reply still said 'The only writing path here', stale since wait now issues requestReviews too. Dropped the claim rather than updating it to name both, since the subcommand's own help text is not the place to track that count, this file's tests are. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Answering a suppressed (low-confidence) finding not in any thread, scripts/pr_review.py:40: confirmed real. Fixed in bbe8c87: dropped the 'The only writing path here' claim from reply's --help text rather than updating it to name both writes, since the subcommand help text isn't the place to track that count, this file's own tests are (see test_the_only_writes_are_the_three_named_here). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/test_pr_review.py:2425
- The
union:falseguard can be bypassed if the GraphQL document is formatted with newlines/tabs around the colon (e.g.,union:\nfalse). Stripping only spaces makes this test less reliable than intended; use a whitespace-tolerant regex or strip all whitespace before matching so the guard actually enforces the rule.
self.assertNotIn('union:false', source.replace(' ', ''),
'the additive form is the only one this script issues, since dropping '
'a pending human reviewer is the runbook\'s manual recovery path, never '
'an automatic one')
source.replace(' ', '') left a newline- or tab-separated
'union:\nfalse' able to slip past the guard undetected, which
would silently defeat the very check meant to catch a destructive
mutation form entering this script. ''.join(source.split()) strips
every whitespace variant, matching what the guard actually intends
to enforce.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Answering a suppressed (low-confidence) finding not in any thread, scripts/test_pr_review.py:2422: confirmed real. Fixed in 2d06909: switched from source.replace(' ', '') to ''.join(source.split()), which strips every whitespace variant (tabs, newlines) rather than only spaces, so a reformatted 'union:\nfalse' can no longer slip past the guard undetected. Also re-answering the round-2 suppressed finding (scripts/pr_review.py:40, the stale 'only writing path' claim), which is showing again from its earlier round per this tool's own no-retirement behavior for suppressed findings: already fixed in bbe8c87 and answered in #685 (comment), nothing further to do there. |
ptr727
added a commit
that referenced
this pull request
Aug 15, 2026
…Python CI Gates (#718) Thirty-one squashes, `56f4d7d..d54862a`. 115 files, +20436/-5298. **Merge with a merge commit, never a squash, and never with `--delete-branch`.** This pull request's head is `develop` itself. ## What lands **Fleet Skills.** The `.agents/skills/` source tree, the generated `.claude-plugin/` distribution, `scripts/build_dist.py` with its `--check` gate, and `scripts/skills_install.py` with its host stamp (#676). Packaged as skills on top of the scaffold: PR review conduct and Copilot instructions upkeep (#677), comment and doc style (#678), resync-a-repo and fleet-conformance-check (#679), the per-language codestyles (#680), git commit conventions and operational vs release workflow (#681), stand up a repo (#683), and repo-worktree (#717). Coverage gaps closed in three passes (#690, #691, #692) plus the P4 sentence-length opt-in (#697). **Host setup.** The Windows host-setup tooling and its PowerShell gate (#674), the Windows bootstrap loader (#682), Docker install and upgrade on Linux and Windows with a version floor (#701, #705), a `uv` floor in `spec/host-tools.json` (#698), self-healing of a shadowing `uv`, `jq`, or `git-restore-mtime` copy (#689), node's real winget package id (#696), and a README for the Linux host-setup nuances (#710). **Python and CI.** Python tooling in CI with the script tests moved to `scripts/tests` (#704), `ruff format` adopted and gated (#709), and the PSScriptAnalyzer claim conditioned on repos that carry `.ps1` files (#686). **Conduct rules.** Triage-order and scope guardrails in pr-review-conduct (#684), `pr_review.py wait` requesting a review rather than only polling for one (#685), a tech-agnostic signed-commit verification (#708), execution rather than analogy to verify platform-specific code (#715), and a unique worktree for every task (#717). **Docs.** The fleet map and gap register with peer messaging declared (#687), mermaid flow diagrams in the kept-authority docs (#702), and the map pointed at the shipped diagrams and current tooling (#703). ## Issues this promotion closes Each landed on `develop` on its own pull request. The keyword fires only on a merge into `main`, so it sits here rather than on the feature pull requests. Closes #700 Closes #707 Closes #711 Closes #712 Closes #714 Closes #688 #699 stays open on purpose: #717 shipped the layout convention and the skill, and the physical migration of existing checkouts is still tracked there. ## Review record Every squash closed its own Copilot loop on its own pull request before merging to `develop`. This promotion carries no new content of its own, so its review is the merged tree as a whole. ## Consequence worth stating The `GOVERNANCE.md` and `AGENTS.md` sections these squashes changed become the canonical the moment this reaches `main`, and every carrying repository reads as drifted from that point until it resyncs. That is the ordinary consequence of a canonical moving rather than a defect. The Skills installer added here is also how a machine picks the new skills up, so a session that keeps restating a rule already packaged as a skill is the signal to run it.
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.
Why
Twice in one session,
waitpolled the full 45-minute timeout for a review nothing had asked for: once after a push superseded the prior request, once after a re-request attempt that never actually issued a mutation.waitnever requested anything itself, contrary to what a caller reasonably expects from a command namedwait— the manualrequestReviewsrecipe documented in.github/copilot-instructions.mdis a separate step every caller has to remember on every push, and forgetting it fails silently (a full timeout, not an error) rather than loudly.What changed
waitnow issues the request itself, once, before its first poll, whenever nothing is already outstanding and the head isn't already covered. The bot id is read fresh from the repository's own last 20 PRs every time (never fabricated, never cached), and requests are additive (union:true) so a human reviewer requested alongside Copilot is never dropped. A repository with no Copilot review anywhere to read the id from falls back to plain polling rather than crashing or guessing.This is a deliberate second write path, not an oversight fixed in passing: it was previously excluded by name in a guard test (
test_the_only_writes_are_the_two_the_reply_path_owns) asserting an explicit prior design decision, itself consistent with.github/copilot-instructions.md's own text describingrequestReviewsas a manual mutation this script doesn't issue. Renamed that test and updated its assertions for the third deliberate write (nowtest_the_only_writes_are_the_three_named_here), added the mutation and runbook-mention checks alongside the existing two, and added an explicit ban onunion:false(the destructive clear-and-recover form stays a manual runbook step, never automatic).Added targeted tests for all three auto-request shapes: bot id resolved and the request fires, no bot id found anywhere and it falls back to polling, already requested and it is not re-requested. Each was verified to fail against the pre-fix code before trusting it (reverted
pr_review.pyalone, ran the three new tests, confirmed 3 of 4 failed as expected — the fourth, "already requested," passed even pre-fix since that no-op case existed by coincidence of no request-issuing code existing yet, not something this PR broke).Verification
🤖 Generated with Claude Code