Require an explicit repository on the review digest

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 <SHA>`, `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 <n>` 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 <n> --repo <owner>/<name>` 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.
 

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.

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,13 @@ 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 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)')
@@ -364,7 +372,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)

scripts/test_pr_review.py

@@ -473,38 +473,42 @@ 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())
 
     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,15 +556,15 @@ 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:
         """It costs a second call, and inside the grace a pending request is just work in flight."""
         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."""