From 3ac4bf4ce636f39e0974f124318104fe6704974e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 5 Aug 2026 17:49:47 -0700 Subject: [PATCH 1/2] Require an explicit repository on the review digest `--repo` defaulted to this repository, so a run that omitted it read a pull request here rather than the one in front of it. The number resolved, the digest rendered, and no field in the output disagreed. Two runs in other repositories digested a pull request here before the maintainer caught it, one of them recovering with "my run digested the hub's #107". A default cannot be made safe for this argument. Every pull request number is valid in every repository, so the wrong target is never a lookup failure and never surfaces as one. ## Two changes, because the argument and the output each hid the miss `--repo` is now required and carries no default, and its value must be `OWNER/NAME`. A bare `ProjectTemplate` is the near-miss a required argument still admits, and it previously raised an unpacking traceback rather than naming which half was missing. The digest line now leads with `repo=OWNER/NAME`, since a digest of the wrong pull request is well-formed and a number alone reads as correct anywhere. Naming the repository is what makes a misdirected run visible in its own output. ## Coverage `TestCli` runs go through a helper that supplies the repository, and three cases were added: a run naming none is rejected, each malformed shape is rejected by name, and the digest names what it read. The `--pickup-grace -1` case now passes `--repo` and asserts on the grace error, because it was about to exit on the missing argument instead and pass for the wrong reason. `GOVERNANCE.md` "PR Review Etiquette" and `scripts/README.md` update their invocations to match. Co-Authored-By: Claude Opus 5 (1M context) --- GOVERNANCE.md | 2 +- scripts/README.md | 6 ++-- scripts/pr_review.py | 17 ++++++++-- scripts/test_pr_review.py | 67 +++++++++++++++++++++++++++++---------- 4 files changed, 69 insertions(+), 23 deletions(-) diff --git a/GOVERNANCE.md b/GOVERNANCE.md index c8f53703..29c665fa 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -298,7 +298,7 @@ After the final push on a PR, sweep older threads from earlier rounds whose code **Answering a suppressed finding is a different act from replying in a thread, and it carries its own pairing.** A threaded reply sits under the comment it answers and the UI shows whether it is resolved. A suppressed finding has neither, so an answer that does not carry its own context is unverifiable: the maintainer cannot tell that it was seen, which finding it addresses, or whether any were skipped, and has to ask. An answer therefore **quotes the finding** in a blockquote, with its `file:line` anchor and enough of Copilot's own words to identify it, **carries one bold verdict per finding** (`Fixed in `, `Disproven`, or `No change needed`) so the outcomes are scannable without reading prose, **states the `(N)` count** the block heading gives so N answers can be checked against N findings, and **links the review** that raised them, since a PR accumulates rounds and an unlinked answer is ambiguous about which one it closes. One comment per review round keeps the answers together. -**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. `scripts/pr_review.py status ` reports every round and marks which are from earlier ones. +**Read every round, not only the head.** A suppressed finding has no resolved state, so a push does not retire it: the finding simply stops appearing in a head-scoped query while remaining unanswered. Treating "superseded by a push" as "answered" is how rounds of findings go unanswered. `scripts/pr_review.py status --repo /` reports every round and marks which are from earlier ones, and it names the repository because a pull request number resolves in every repository and a digest of the wrong one is well-formed. **The review's own overview cannot be trusted to say whether findings exist.** A body that reads "Copilot reviewed N out of N changed files and generated no new comments" routinely carries a collapsed block of suppressed findings directly beneath that sentence. Read the body for the block rather than the summary line, because the summary line and `reviewDecision` and an empty unresolved-thread list all agree that a review with four outstanding findings is clean. diff --git a/scripts/README.md b/scripts/README.md index 3bc339b4..c1c8a9b9 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -89,10 +89,12 @@ A stale-backticked-path check was built and **rejected**: a template repo legiti One compact digest of a pull request's Copilot review state, replacing a sequence of one-`gh`-call-per-turn polls. `status` prints the digest, and `wait` runs the backoff in-process so a long review wait costs one agent turn instead of one per poll. Read-only by design: the mutations (re-request, reply, resolve) stay as explicit `gh` calls so they remain visible to the `gh-write-guard` hook and to review, and their runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. ```sh -python3 scripts/pr_review.py status 452 -python3 scripts/pr_review.py wait 452 --timeout 2700 +python3 scripts/pr_review.py status 452 --repo ptr727/ProjectTemplate +python3 scripts/pr_review.py wait 452 --repo ptr727/ProjectTemplate --timeout 2700 ``` +`--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. + `wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. `wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 6d883241..6f81b961 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -294,7 +294,9 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, blind = [f for f in ('reviews', 'comments') if window_blind(pr, f)] answered = 'yes' if answer else ('unknown' if blind else 'no') lines = [ - f'pr={num} head={head[:8]} rounds={len(revs)} ' + # 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'threads={len(threads)} unresolved={len(unresolved)} ' f'suppressed={sum(finding_count(b) for n, b in blocks)} ' @@ -355,7 +357,12 @@ def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser() ap.add_argument('cmd', choices=['status', 'wait']) ap.add_argument('number', type=int) - ap.add_argument('--repo', default='ptr727/ProjectTemplate') + # 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. + # The number resolves there, the digest renders, and nothing in the output disagrees. + # Two runs read a pull request here while one in their own repository was the subject. + ap.add_argument('--repo', required=True, metavar='OWNER/NAME', + help='the repository holding the pull request, since a number resolves in any') ap.add_argument('--timeout', type=int, default=2700, help='seconds (default 45m)') ap.add_argument('--pickup-grace', type=int, default=300, help='seconds before the first pickup read, and between reads (default 5m)') @@ -364,7 +371,11 @@ def main(argv: list[str] | None = None) -> int: # That is the per-poll REST pattern the interval exists to prevent. if a.pickup_grace < 0: ap.error('--pickup-grace cannot be negative') - owner, repo = a.repo.split('/', 1) + # A bare name is the near-miss a required argument still admits, and unpacking it raises a + # ValueError traceback rather than saying which half is missing. + owner, _, repo = a.repo.partition('/') + if not owner or not repo or '/' in repo: + ap.error(f'--repo takes OWNER/NAME, not {a.repo!r}') if a.cmd == 'status': out, _ = digest(owner, repo, a.number) diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index 419cef3c..392f4463 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -473,16 +473,20 @@ class TestCli(GqlCase): def setUp(self) -> None: self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) + def cli(self, argv: list[str]) -> int: + """Every run names its repository, since the parser supplies no default for one.""" + return pr_review.main([*argv, '--repo', 'o/r']) + def test_status_prints_the_digest_and_exits_zero(self) -> None: self.answer(payload([review()])) - self.assertEqual(0, pr_review.main(['status', '7'])) + self.assertEqual(0, self.cli(['status', '7'])) self.assertIn('pr=7', self.out.getvalue()) def test_wait_returns_zero_once_the_review_lands_on_the_head(self) -> None: """The first poll already sees it, so the loop body never runs.""" self.answer(payload([review()])) with mock.patch.object(pr_review.time, 'sleep') as slept: - self.assertEqual(0, pr_review.main(['wait', '7'])) + self.assertEqual(0, self.cli(['wait', '7'])) slept.assert_not_called() self.assertIn('waited=', self.out.getvalue()) @@ -490,21 +494,21 @@ def test_wait_polls_again_after_a_pending_round(self) -> None: """Each iteration re-reads the head, since a push during the wait moves it.""" self.answer(payload([review(oid=OLD)]), payload([review()])) with mock.patch.object(pr_review.time, 'sleep') as slept: - self.assertEqual(0, pr_review.main(['wait', '7'])) + self.assertEqual(0, self.cli(['wait', '7'])) self.assertEqual(1, slept.call_count) def test_wait_exits_thirty_at_the_timeout_rather_than_reporting_success(self) -> None: """Pending is not failure and not success, so it takes a code of its own.""" self.answer(payload([review(oid=OLD)])) with mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) + self.assertEqual(30, self.cli(['wait', '7', '--timeout', '0'])) self.assertIn('status=PENDING', self.out.getvalue()) def test_the_timeout_carries_the_digest_rather_than_a_bare_pending_line(self) -> None: """A wait that ends with no evidence reports a slow reviewer and a broken poll alike.""" self.answer(payload([review(oid=OLD)], [thread('T1')])) with mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(30, pr_review.main(['wait', '7', '--timeout', '0'])) + self.assertEqual(30, self.cli(['wait', '7', '--timeout', '0'])) out = self.out.getvalue() self.assertIn('review_on_head=NO', out) self.assertIn('unresolved=1', out) @@ -517,7 +521,7 @@ def test_wait_ends_on_an_answer_outside_a_review_instead_of_waiting_it_out(self) """ self.answer(payload([review(oid=OLD)], comments=[comment()])) with mock.patch.object(pr_review.time, 'sleep') as slept: - self.assertEqual(40, pr_review.main(['wait', '7', '--timeout', '0'])) + self.assertEqual(40, self.cli(['wait', '7', '--timeout', '0'])) slept.assert_not_called() out = self.out.getvalue() self.assertIn('status=ANSWERED_OUTSIDE_REVIEW', out) @@ -527,7 +531,7 @@ def test_a_landed_review_wins_over_an_older_answer(self) -> None: """Coverage is the success case, and a spent comment does not downgrade it to 40.""" self.answer(payload([review(at=LATE)], comments=[comment(at=EARLY)])) with mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(0, pr_review.main(['wait', '7'])) + self.assertEqual(0, self.cli(['wait', '7'])) def test_wait_stops_on_a_request_nothing_picked_up(self) -> None: """Waiting on cannot start a request nothing is acting on, so the wait says so and ends. @@ -539,7 +543,7 @@ def test_wait_stops_on_a_request_nothing_picked_up(self) -> None: with mock.patch.object(pr_review, 'timeline', return_value=[('review_requested', LATE)]), \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(50, pr_review.main( + self.assertEqual(50, self.cli( ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) out = self.out.getvalue() self.assertIn('status=REQUEST_NOT_PICKED_UP', out) @@ -552,7 +556,7 @@ def test_a_request_being_worked_on_is_not_stopped_on(self) -> None: return_value=[('review_requested', EARLY), ('copilot_work_started', LATE)]), \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(0, pr_review.main( + self.assertEqual(0, self.cli( ['wait', '7', '--pickup-grace', '0', '--timeout', '600'])) def test_the_pickup_read_waits_out_the_grace_rather_than_running_per_poll(self) -> None: @@ -560,7 +564,7 @@ def test_the_pickup_read_waits_out_the_grace_rather_than_running_per_poll(self) self.answer(payload([review(oid=OLD)], pending=True), payload([review()], pending=True)) with mock.patch.object(pr_review, 'timeline', return_value=[]) as seen, \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(0, pr_review.main( + self.assertEqual(0, self.cli( ['wait', '7', '--pickup-grace', '9999', '--timeout', '600'])) seen.assert_not_called() @@ -575,7 +579,7 @@ def test_the_pickup_read_runs_on_its_own_interval_once_the_grace_is_out(self) -> with mock.patch.object(pr_review.time, 'monotonic', side_effect=count(0, 30)), \ mock.patch.object(pr_review, 'timeline', return_value=picked_up) as seen, \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(30, pr_review.main( + self.assertEqual(30, self.cli( ['wait', '7', '--pickup-grace', '300', '--timeout', '1200'])) # Roughly one read per grace interval over the wait, never one per poll. self.assertGreaterEqual(seen.call_count, 1) @@ -591,7 +595,7 @@ def test_a_review_landing_during_the_last_read_wins_over_the_stalled_code(self) with mock.patch.object(pr_review, 'timeline', return_value=[('review_requested', LATE)]), \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(0, pr_review.main( + self.assertEqual(0, self.cli( ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) out = self.out.getvalue() self.assertIn('review_on_head=yes', out) @@ -601,7 +605,7 @@ def test_a_review_landing_during_the_last_read_wins_over_the_timeout(self) -> No """Same disagreement at the other exit: printing coverage and returning PENDING.""" self.answer(payload([review(oid=OLD)]), payload([review()])) with mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(0, pr_review.main(['wait', '7', '--timeout', '0'])) + self.assertEqual(0, self.cli(['wait', '7', '--timeout', '0'])) out = self.out.getvalue() self.assertIn('review_on_head=yes', out) self.assertNotIn('status=PENDING', out) @@ -613,7 +617,7 @@ def test_a_request_picked_up_after_the_loop_read_it_is_not_reported_as_stalled(s with mock.patch.object(pr_review, 'timeline', side_effect=[[('review_requested', LATE)], picked_up]), \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(30, pr_review.main( + self.assertEqual(30, self.cli( ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) out = self.out.getvalue() self.assertNotIn('status=REQUEST_NOT_PICKED_UP', out) @@ -625,7 +629,7 @@ def test_an_answer_outranks_a_stall_when_both_are_true(self) -> None: with mock.patch.object(pr_review, 'timeline', return_value=[('review_requested', LATE)]), \ mock.patch.object(pr_review.time, 'sleep'): - self.assertEqual(40, pr_review.main( + self.assertEqual(40, self.cli( ['wait', '7', '--pickup-grace', '0', '--timeout', '0'])) def test_the_repo_argument_splits_into_owner_and_name(self) -> None: @@ -634,6 +638,33 @@ def test_the_repo_argument_splits_into_owner_and_name(self) -> None: self.assertEqual(0, pr_review.main(['status', '7', '--repo', 'owner/name'])) self.assertEqual(('owner', 'name', 7), dig.call_args.args) + def test_a_run_naming_no_repo_is_rejected_rather_than_sent_somewhere(self) -> None: + """A default would send it to one repository, and every number resolves there. + + That is the failure this had twice: the digest rendered, nothing in it disagreed, and + the run was reading a pull request in a repository nobody had named. + """ + self.answer(payload([review()])) + with contextlib.redirect_stderr(io.StringIO()) as err: + with self.assertRaises(SystemExit): + pr_review.main(['status', '7']) + self.assertIn('--repo', err.getvalue()) + + def test_a_repo_that_is_not_owner_slash_name_is_rejected_by_name(self) -> None: + """The near-miss a required argument still admits, and unpacking it is a bare traceback.""" + for bad in ('ProjectTemplate', 'ptr727/', '/ProjectTemplate', 'a/b/c', ''): + with self.subTest(repo=bad): + with contextlib.redirect_stderr(io.StringIO()) as err: + with self.assertRaises(SystemExit): + pr_review.main(['status', '7', '--repo', bad]) + self.assertIn('OWNER/NAME', err.getvalue()) + + def test_the_digest_names_the_repository_it_read(self) -> None: + """A digest of the wrong pull request is well-formed, so the line has to say which one.""" + self.answer(payload([review()])) + self.assertEqual(0, self.cli(['status', '7'])) + self.assertIn('repo=o/r pr=7', self.out.getvalue()) + class TestContract(unittest.TestCase): def test_the_reviewer_login_matches_the_runbook_graphql_form(self) -> None: @@ -712,9 +743,11 @@ def test_the_timeline_filter_takes_the_reviewer_s_own_requests_only(self) -> Non def test_a_negative_pickup_grace_is_rejected_rather_than_read_as_every_poll(self) -> None: """It leaves the next reading behind the clock, which is the per-poll pattern returning.""" - with contextlib.redirect_stderr(io.StringIO()): + with contextlib.redirect_stderr(io.StringIO()) as err: with self.assertRaises(SystemExit): - pr_review.main(['wait', '7', '--pickup-grace', '-1']) + pr_review.main(['wait', '7', '--repo', 'o/r', '--pickup-grace', '-1']) + # The repository is named, or this exits on the missing argument and proves nothing. + self.assertIn('pickup-grace', err.getvalue()) def test_a_failed_timeline_read_raises_rather_than_reading_as_no_events(self) -> None: """An empty list reads as no request pending, which is the false clean one level up.""" From 873283bbc4d2c097a335e5ac5f11cfaa0cdce406 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 5 Aug 2026 17:53:57 -0700 Subject: [PATCH 2/2] Complete the --repo help sentence It ended on "resolves in any", which reads as a truncated string in `--help` rather than as a clause. The text now says what the reader needs, that a pull request number identifies no repository on its own. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/pr_review.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 6f81b961..b0faa377 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -362,7 +362,8 @@ def main(argv: list[str] | None = None) -> int: # The number resolves there, the digest renders, and nothing in the output disagrees. # Two runs read a pull request here while one in their own repository was the subject. ap.add_argument('--repo', required=True, metavar='OWNER/NAME', - help='the repository holding the pull request, since a number resolves in any') + help='the repository the pull request is in, since a pull ' + 'request number identifies no repository on its own') ap.add_argument('--timeout', type=int, default=2700, help='seconds (default 45m)') ap.add_argument('--pickup-grace', type=int, default=300, help='seconds before the first pickup read, and between reads (default 5m)')