From 1c7e1f191122b2dc076f8ee9a2f88cdfd54527b0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 11 Sep 2026 21:06:10 -0700 Subject: [PATCH] Name the Directory the Scope Refusal Probed, Not the Checkout (#1563) `scripts/pr_review.py` resolves the owner a write may stay within from the script's own directory, which is deliberate: the helper is hub-hosted rather than carried, so it is reached from a hub checkout while the repository being answered is named on the command line, and the working directory says nothing about who owns either. Its refusals called that directory "this checkout", which an operator reads as the working directory. A copy of the script outside any checkout therefore produced a refusal the operator could disprove on the spot with `git remote get-url origin` in the tree they were standing in, with nothing on screen explaining the disagreement. Message only, per the issue. The anchoring, the refusal, and the refusal writing nothing are unchanged. ## What changed - Both refusals name the directory that was actually probed, and describe the probe rather than claiming which repository encloses that directory. - The unreadable-owner refusal names a remedy, scoped to the cause it actually fixes rather than stated as the diagnosis. - `HERE` is a module constant, the single reader for the probe and for the messages that name it, matching the shape six sibling scripts already use. - `reply`'s exit-64 gloss and `origin_owner`'s docstring each said less than the code does, and `scripts/README.md` carried the same "this checkout" claim. All three now agree with the messages. Before, from a copy outside any checkout: ``` status=OUT_OF_SCOPE nothing was written: this checkout has no readable `origin`, so the owner a write would stay within cannot be established, and an unverified scope is not a scope ``` After, against the same reproduction: ``` status=OUT_OF_SCOPE nothing was written: this script got no owner from `git remote get-url origin` against the directory it sits in, /tmp/scratch, so the owner a write would stay within cannot be established, and an unverified scope is not a scope. That directory is this script's own rather than the working directory, so a readable `origin` in the working directory says nothing about this, and the remedy for a copy of this script sitting outside a checkout is to run the copy that lives inside a hub checkout instead of this one ``` ## Verification Reproduced the issue's own scenario against the fix, a copy of the script outside any checkout and a cross-owner target from a fleet checkout, and read both messages back. 322 cases in the module and 1403 under discovery, plus `prose_lint`, `repo_gate --check eol`, `spec/validate.py`, `ruff format --check` and `ruff check`, all clean. The new probe case was proved by mutation rather than by passing: against a copy whose probe reads `Path.cwd()` instead of `HERE`, `test_the_probe_reads_the_directory_the_refusal_names` fails. Without that, the assertion passed on a path coincidence whenever the working directory happened to equal the anchor. Five local review rounds ran over this diff. Each message clause was checked against every state that reaches its branch, meaning a missing `git`, a timeout, no remote named `origin`, a directory under no repository, an `origin` URL the owner regex cannot parse, and an inherited `GIT_DIR`. Three earlier wordings were rejected for failing one of those: one asserted the probe had executed when a missing `git` means it never did, one named a remedy that read as an instruction to change directory and so reproduced the same refusal, and one claimed a repository encloses the probed directory, which an inherited `GIT_DIR` falsifies. ## Raised separately rather than folded in Two real defects surfaced during review that are code changes rather than wording, so they are filed instead of carried here: - #1561, the probe inherits `GIT_DIR`, so an inherited one redirects it to a different repository. The messages here describe the probe rather than the directory's repository, which keeps them true under that leak, but the leak itself is untouched. - #1562, `wait`'s Copilot review request is a mutation that never passes through `in_scope`, so the in-process owner check does not cover it. Addresses #1557. The closing reference goes on the promotion pull request, since a `develop`-targeted one never registers. --- scripts/README.md | 2 +- scripts/pr_review.py | 26 ++++++++++++++++-------- scripts/tests/test_pr_review.py | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index f8eb76e8..3f5eb77c 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -209,7 +209,7 @@ The digest also reports the **suppressed findings** a review body collapses into `reply` posts one answer and resolves one thread, and it exists because the hand-run form keeps failing the same way rather than because a wrapper is tidier. Three instances are on record, each an agent that had read the rule against hand-typing a node id and reached for the literal regardless, the last of them refused by the `gh-write-guard` hook an hour after quoting that same rule in a pull request body. A shape that fails while the reader knows the rule is a shape to remove rather than a rule to restate, so the selector is the finding's own words and there is deliberately no argument a `PRRT_...` id fits in: the id is read from the query in the same run and passed straight to the mutation, and a case asserts the id the mutation carries is the one that run just read. The words are matched against the thread's opening comment rather than against a line number, because a fix push moves the line and every lookup keyed to one then misses, which is how three replies posted against nothing while the resolves still succeeded and closed the threads carrying no answer. Matching is case-insensitive, since the text is quoted back out of a digest by a reader. -Every failure is a stop rather than a fallback, because each alternative closes a finding while leaving it unanswered, which is the state a reviewer reads as addressed. No match exits `60` and prints the open threads, since a no-match and an already-answered thread read identically from here. Two matches exit `61` and print both candidates rather than taking the first, `head -n 1` being how a reply lands on the wrong finding. A reply whose response carries no `url`, or a body that came back empty, exits `62` **without** resolving. A resolve that does not report `isResolved` exits `63`, with the reply already posted, so the thread is open behind an answer rather than silently assumed closed. `--resolve` is opt-in rather than the default, because a decline is resolved only once its evidence is in the thread. A target under an owner other than this checkout's exits `64` before anything is read at all, and that refusal takes no environment-variable escape: a grant this process can be handed is one the caller sets on the command that runs it, and a grant the caller writes for itself is not a grant, so the cross-owner case goes through the runbook's explicit `gh` path where the hook reads the maintainer's grant from the session instead. +Every failure is a stop rather than a fallback, because each alternative closes a finding while leaving it unanswered, which is the state a reviewer reads as addressed. No match exits `60` and prints the open threads, since a no-match and an already-answered thread read identically from here. Two matches exit `61` and print both candidates rather than taking the first, `head -n 1` being how a reply lands on the wrong finding. A reply whose response carries no `url`, or a body that came back empty, exits `62` **without** resolving. A resolve that does not report `isResolved` exits `63`, with the reply already posted, so the thread is open behind an answer rather than silently assumed closed. `--resolve` is opt-in rather than the default, because a decline is resolved only once its evidence is in the thread. Exit `64` is the scope refusal, and `comment` raises it on the same terms `reply` does. It covers two states rather than one: a target under an owner other than the one `git remote get-url origin` names when run against the directory this script sits in, and an owner that run could not determine at all. Both refuse before reading anything from GitHub. That owner is read from the script's own directory rather than from the working directory, since the helper is reached from a hub checkout while the repository being answered is named on the command line, so both refusals name the directory they probed rather than leaving a reader to take it as the one they are standing in. That refusal takes no environment-variable escape: a grant this process can be handed is one the caller sets on the command that runs it, and a grant the caller writes for itself is not a grant, so the cross-owner case goes through the runbook's explicit `gh` path where the hook reads the maintainer's grant from the session instead. What this trades away is stated rather than glossed. A mutation spelled as a `gh` command in a shell is read by the `gh-write-guard` hook and one this script performs is not. The script removes that failure at its source instead: every node ID comes from a live query in the same run. The owner check is also enforced in-process. The whole-source test guards against every other state-changing call and counts each reviewed mutation document. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 1d098d11..af6e8cc5 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -88,7 +88,7 @@ finding, queries the id itself, and offers no argument an id fits in. Exit 0 = 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 target is under another owner. + 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 @@ -156,6 +156,10 @@ from datetime import UTC, datetime from pathlib import Path +# The directory this script sits in, and the anchor every scope answer is read from. +# One value serves the owner probe and the refusal naming it, so the two cannot disagree. +HERE = Path(__file__).resolve().parent + REVIEWER = "copilot-pull-request-reviewer" # Other review bots this repository has trialed alongside Copilot. # Tracked at the identity level only, login and commit oid, never body prose, except where a reader below names one explicitly. @@ -1985,7 +1989,7 @@ def digest( def origin_owner() -> str | None: - """The owner of the checkout this script sits in, or None where that cannot be read. + """The owner `git remote get-url origin` yields against this script's own directory, or None. Anchored on the script's own directory rather than the working directory, because this is reached from a hub checkout while the repository being answered is named on the command line, @@ -1993,7 +1997,7 @@ def origin_owner() -> str | None: """ try: url = subprocess.run( - ["git", "-C", str(Path(__file__).resolve().parent), "remote", "get-url", "origin"], + ["git", "-C", str(HERE), "remote", "get-url", "origin"], capture_output=True, text=True, encoding="utf-8", @@ -2021,14 +2025,20 @@ def in_scope(target_owner: str) -> tuple[bool, str]: origin = origin_owner() if origin is None: return False, ( - "this checkout has no readable `origin`, so the owner a write would stay " - "within cannot be established, and an unverified scope is not a scope" + "this script got no owner from `git remote get-url origin` against the directory " + f"it sits in, {HERE}, so the owner a write would stay within cannot be " + "established, and an unverified scope is not a scope. That directory is this " + "script's own rather than the working directory, so a readable `origin` in the " + "working directory says nothing about this, and the remedy for a copy of this " + "script sitting outside a checkout is to run the copy that lives inside a hub " + "checkout instead of this one" ) if target_owner.lower() != origin: return False, ( - f"the target is under {target_owner}, and this checkout is under {origin}. " - "A different owner is the shape this refuses outright: take it through the " - "runbook mutations, where the write-guard hook reads the maintainer grant" + f"the target is under {target_owner}, and `git remote get-url origin`, run against " + f"the directory this script sits in, {HERE}, named {origin}. A different owner is " + "the shape this refuses outright: take it through the runbook mutations, where the " + "write-guard hook reads the maintainer grant" ) return True, "" diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 8b512ef9..a23742bf 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -4149,6 +4149,42 @@ def test_the_backoff_is_bounded_and_non_decreasing(self) -> None: self.assertEqual(delays, sorted(delays)) +class TestScopeRefusalNamesTheDirectoryItProbed(unittest.TestCase): + """These refusals name the directory probed, the reader's own being a different one.""" + + ANCHOR = Path("/anchor-this-script-sits-in") + + def test_the_probe_reads_the_directory_the_refusal_names(self) -> None: + """A message naming a directory the probe never read sends the reader at the wrong tree.""" + self.enterContext(mock.patch.object(pr_review, "HERE", self.ANCHOR)) + with mock.patch.object(pr_review.subprocess, "run") as run: + run.return_value = mock.Mock(stdout="https://github.com/o/r.git\n") + self.assertEqual("o", pr_review.origin_owner()) + argv = run.call_args.args[0] + self.assertEqual(str(self.ANCHOR), argv[argv.index("-C") + 1]) + + def test_an_unreadable_origin_names_that_directory_and_a_remedy(self) -> None: + """A claim naming no directory is one the operator disproves in the wrong tree.""" + self.enterContext(mock.patch.object(pr_review, "HERE", self.ANCHOR)) + with mock.patch.object(pr_review, "origin_owner", return_value=None): + ok, why = pr_review.in_scope("o") + self.assertFalse(ok) + self.assertIn(str(self.ANCHOR), why) + self.assertIn("hub checkout", why) + self.assertIn("instead of this one", why) + self.assertIn("git remote get-url origin", why) + self.assertNotIn("this checkout", why) + + def test_a_cross_owner_refusal_names_that_directory_too(self) -> None: + """The same ambiguity, on the branch that reports both owners.""" + self.enterContext(mock.patch.object(pr_review, "HERE", self.ANCHOR)) + with mock.patch.object(pr_review, "origin_owner", return_value="o"): + ok, why = pr_review.in_scope("someone-else") + self.assertFalse(ok) + self.assertIn(str(self.ANCHOR), why) + self.assertNotIn("this checkout", why) + + class TestHarness(unittest.TestCase): def test_this_module_collects_a_plausible_number_of_cases(self) -> None: """A module whose cases fail to load still reports OK, which is a pass proving nothing."""