From 280175975f4871a20c5f0b22341034d6abfa6cb4 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 06:56:11 -0700
Subject: [PATCH 1/9] Strip inherited git discovery variables from the
 origin-owner probe

Fixes #1561. origin_owner() ran `git -C <script dir> remote get-url
origin` with no `env=`, so it inherited the caller's environment. Git
honors GIT_DIR over `-C` for repository discovery, so an inherited
GIT_DIR (set by a git hook, `git bisect run`, or `git rebase --exec`)
made the probe answer for a different repository than the one the
script sits in, which could make the write-scope check pass or fail
for the wrong owner. Strip exactly GIT_DIR, GIT_WORK_TREE,
GIT_COMMON_DIR, and GIT_OBJECT_DIRECTORY from the subprocess
environment before running the probe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/pr_review.py            | 10 ++++++
 scripts/tests/test_pr_review.py | 54 +++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/scripts/pr_review.py b/scripts/pr_review.py
index af6e8cc5..5d6053c0 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -148,6 +148,7 @@
 import argparse
 import io
 import json
+import os
 import re
 import subprocess
 import sys
@@ -1995,6 +1996,14 @@ def origin_owner() -> str | None:
     reached from a hub checkout while the repository being answered is named on the command line,
     so the working directory says nothing about who owns either.
     """
+    # Git honors GIT_DIR over `-C` for repository discovery.
+    # An inherited one (a hook, a `git bisect run`, a `git rebase --exec`) would make this answer for a different repository than the one this script sits in.
+    # Stripped rather than the environment being cleared wholesale, since a wider clearing would drop credentials or proxy settings `gh`/`git` still need.
+    env = {
+        k: v
+        for k, v in os.environ.items()
+        if k not in ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")
+    }
     try:
         url = subprocess.run(
             ["git", "-C", str(HERE), "remote", "get-url", "origin"],
@@ -2003,6 +2012,7 @@ def origin_owner() -> str | None:
             encoding="utf-8",
             timeout=5,
             check=False,
+            env=env,
         ).stdout.strip()
     # A missing git, a timeout, or any other failure all mean the owner cannot be read.
     except Exception:  # noqa: BLE001
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index a23742bf..5aef2e76 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -14,9 +14,11 @@
 import contextlib
 import io
 import json
+import os
 import re
 import subprocess
 import sys
+import tempfile
 import unittest
 from datetime import UTC, datetime, timedelta
 from pathlib import Path
@@ -4185,6 +4187,58 @@ def test_a_cross_owner_refusal_names_that_directory_too(self) -> None:
         self.assertNotIn("this checkout", why)
 
 
+class TestOriginOwnerIgnoresInheritedGitDiscovery(unittest.TestCase):
+    """#1561: an inherited GIT_DIR must not redirect the probe to a different repository."""
+
+    def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
+        repo = parent / name
+        repo.mkdir()
+        subprocess.run(["git", "init", "--quiet", str(repo)], check=True)
+        subprocess.run(
+            [
+                "git",
+                "-C",
+                str(repo),
+                "remote",
+                "add",
+                "origin",
+                f"https://github.com/{owner_slash_repo}.git",
+            ],
+            check=True,
+        )
+        return repo
+
+    def test_a_git_dir_inherited_from_the_caller_does_not_redirect_the_probe(self) -> None:
+        """A git hook, `git bisect run`, and `git rebase --exec` all export GIT_DIR.
+
+        Git honours it over `-C` for repository discovery, so an inherited one that names a
+        different repository must not make this probe answer for that repository instead.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            other = self.make_repo(tmp_path, "other", "unrelated-owner/other-repo")
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            with mock.patch.dict(os.environ, {"GIT_DIR": str(other / ".git")}):
+                self.assertEqual("acme", pr_review.origin_owner())
+
+    def test_the_other_inherited_discovery_variables_are_stripped_too(self) -> None:
+        """GIT_WORK_TREE, GIT_COMMON_DIR, and GIT_OBJECT_DIRECTORY are the same hazard."""
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            other = self.make_repo(tmp_path, "other", "unrelated-owner/other-repo")
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            env = {
+                "GIT_DIR": str(other / ".git"),
+                "GIT_WORK_TREE": str(other),
+                "GIT_COMMON_DIR": str(other / ".git"),
+                "GIT_OBJECT_DIRECTORY": str(other / ".git" / "objects"),
+            }
+            with mock.patch.dict(os.environ, env):
+                self.assertEqual("acme", pr_review.origin_owner())
+
+
 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."""

From e84080a8df0a9e0da7a863daf5e35e4c7c9e76ea Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 06:57:35 -0700
Subject: [PATCH 2/9] Refuse a cross-owner target outright in the wait command

Fixes #1562. in_scope() had exactly two call sites, comment_on_pr and
reply_to_thread, but the wait path in main() called
request_copilot_review (a requestReviews GraphQL mutation) without
ever passing through it. Add the same scope check before wait does
any work, so a cross-owner target exits 64, prints
"status=OUT_OF_SCOPE", and issues no network call at all. Document
exit code 64 in wait's own docstring alongside its other exit codes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/pr_review.py            |  9 +++++++++
 scripts/tests/test_pr_review.py | 25 +++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/scripts/pr_review.py b/scripts/pr_review.py
index 5d6053c0..abe70de3 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -134,6 +134,8 @@
            A pending request remains pending until a review, an answer, or the timeout. GitHub's
            effort-labeled review lifecycle does not always emit `copilot_work_started`, so that
            event is not evidence that distinguishes queued work from abandoned work.
+           64 = the write scope could not be established or excludes the target, checked before
+           the auto-request or any poll, so a cross-owner target reads and writes nothing here.
 
 Reading is the bulk of this and the writing commands are a trade rather than a free win. A
 mutation spelled as a `gh` command in a shell is read by the gh-write-guard PreToolUse hook, and
@@ -2507,6 +2509,13 @@ def main(argv: list[str] | None = None) -> int:
     if a.cmd == "reply":
         return reply_to_thread(owner, repo, a.number, a.match, a.body, a.path, a.resolve)
 
+    # `wait` mutates too, via the auto-request below, and had no scope check of its own until
+    # #1562: the other two write paths refused a cross-owner target outright and this one did not.
+    ok, why = in_scope(owner)
+    if not ok:
+        print(f"status=OUT_OF_SCOPE nothing was written: {why}")
+        return 64
+
     # In-process backoff, so the whole wait costs one agent turn.
     delays = [15, 20, 30, 45, 60, 120]
     start = time.monotonic()
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 5aef2e76..33211f5b 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -323,6 +323,10 @@ def answer(self, *responses: dict) -> mock._patch:
         never reaches the real `gh_graphql` since the mock above replaces it wholesale, so the
         two patches govern disjoint call sites and neither can shadow the other. A case
         exercising the auto-request itself re-patches `gh_graphql` after calling this.
+
+        `origin_owner` is patched to this suite's own "o" too, since `wait` now runs the same
+        in-scope check `comment` and `reply` already did, and this suite's cases run against a
+        real checkout whose actual origin has nothing to do with the "o/r" they exercise.
         """
         queue = list(responses)
 
@@ -330,6 +334,7 @@ def fake(_query, _owner, _repo, _num):
             return queue.pop(0) if len(queue) > 1 else queue[0]
 
         patched = self.enterContext(mock.patch.object(pr_review, "gql", side_effect=fake))
+        self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="o"))
         self.enterContext(
             mock.patch.object(
                 pr_review,
@@ -4239,6 +4244,26 @@ def test_the_other_inherited_discovery_variables_are_stripped_too(self) -> None:
                 self.assertEqual("acme", pr_review.origin_owner())
 
 
+class TestWaitStaysInScope(unittest.TestCase):
+    """#1562: `wait` mutates via the auto-request, so it must refuse cross-owner outright too."""
+
+    def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anything(self) -> None:
+        """Both transports raise on any call, so a reverted check fails fast rather than
+        falling into the real, minutes-long backoff loop with a mocked GraphQL response."""
+        out = self.enterContext(contextlib.redirect_stdout(io.StringIO()))
+        self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="acme"))
+        boom = AssertionError("no network call should be reached for an out-of-scope target")
+        with (
+            mock.patch.object(pr_review, "gql", side_effect=boom) as gql,
+            mock.patch.object(pr_review, "gh_graphql", side_effect=boom) as gh_graphql,
+        ):
+            code = pr_review.main(["wait", "7", "--repo", "someone-else/r"])
+        self.assertEqual(64, code)
+        self.assertIn("status=OUT_OF_SCOPE", out.getvalue())
+        gql.assert_not_called()
+        gh_graphql.assert_not_called()
+
+
 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."""

From e064e713b693d12e711b57c03b2f6ddde633cd06 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 07:00:43 -0700
Subject: [PATCH 3/9] Say what the discovery-set case proves, and why the wait
 refusal precedes the reads

Two claims in the previous two commits described the change more confidently than
the code supports.

The probe's comment justified stripping four names rather than clearing the
environment by naming credentials and proxy settings, which a local `git remote
get-url` never reaches. What it actually needs from the environment is PATH, to be
found at all, and HOME, to read the very config it is reading.

The second discovery case was named for stripping GIT_WORK_TREE, GIT_COMMON_DIR and
GIT_OBJECT_DIRECTORY, and it proves no such thing: it sets GIT_DIR alongside them, so
it fails on an unstripped environment for the reason the first case already covers.
Only GIT_DIR is shown to redirect `remote get-url` away from a `-C` argument, so a
case per name would assert a redirection the other three are not demonstrated to
produce. The case now says it pins the set inherited at once, which is what it holds.

The wait comment dated itself against the state before the fix, which stops being
readable the moment that state is gone. It now says why the refusal sits ahead of the
first read instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/pr_review.py            |  6 +++---
 scripts/tests/test_pr_review.py | 10 ++++++++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/scripts/pr_review.py b/scripts/pr_review.py
index abe70de3..c49cd39a 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -2000,7 +2000,7 @@ def origin_owner() -> str | None:
     """
     # Git honors GIT_DIR over `-C` for repository discovery.
     # An inherited one (a hook, a `git bisect run`, a `git rebase --exec`) would make this answer for a different repository than the one this script sits in.
-    # Stripped rather than the environment being cleared wholesale, since a wider clearing would drop credentials or proxy settings `gh`/`git` still need.
+    # Stripped rather than the environment being cleared wholesale, since git still needs PATH to be found at all and HOME to read the config this very call reads.
     env = {
         k: v
         for k, v in os.environ.items()
@@ -2509,8 +2509,8 @@ def main(argv: list[str] | None = None) -> int:
     if a.cmd == "reply":
         return reply_to_thread(owner, repo, a.number, a.match, a.body, a.path, a.resolve)
 
-    # `wait` mutates too, via the auto-request below, and had no scope check of its own until
-    # #1562: the other two write paths refused a cross-owner target outright and this one did not.
+    # `wait` mutates through the auto-request below, so it refuses a cross-owner target the way `comment` and `reply` do.
+    # Ahead of the first read rather than at the request itself, since a wait that may not request has nothing left to poll for.
     ok, why = in_scope(owner)
     if not ok:
         print(f"status=OUT_OF_SCOPE nothing was written: {why}")
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 33211f5b..5bd4a343 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4227,8 +4227,14 @@ def test_a_git_dir_inherited_from_the_caller_does_not_redirect_the_probe(self) -
             with mock.patch.dict(os.environ, {"GIT_DIR": str(other / ".git")}):
                 self.assertEqual("acme", pr_review.origin_owner())
 
-    def test_the_other_inherited_discovery_variables_are_stripped_too(self) -> None:
-        """GIT_WORK_TREE, GIT_COMMON_DIR, and GIT_OBJECT_DIRECTORY are the same hazard."""
+    def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) -> None:
+        """The four names are stripped as a set, and this pins the set rather than each one.
+
+        Only GIT_DIR is shown above to redirect `remote get-url` away from a `-C` argument, so
+        a case per name would assert a redirection the other three are not demonstrated to
+        produce. What this holds is that all four inherited at once still leave the probe
+        answering for the directory the script sits in.
+        """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
             anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")

From 0c01aa899c86e58574da8d90b3e05e3becce8f34 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 07:20:45 -0700
Subject: [PATCH 4/9] Close Five of Six PR-Review Write-Boundary Findings

Delete the false claim that a hook, `git bisect run`, and `git rebase
--exec` export GIT_DIR, from both the strip's comment in `origin_owner()`
and the matching test docstring; state only the checkable mechanism, that
an inherited GIT_DIR overrides `-C` for repository discovery.

Give `TestOriginOwnerIgnoresInheritedGitDiscovery.make_repo` an
environment stripped of the same four discovery names `origin_owner`
strips, so its `git init`/`git remote add` calls build the throwaway
repos regardless of what the caller inherited, rather than mutating
whatever repository an inherited GIT_DIR happens to name.

Add a case proving GIT_COMMON_DIR alone redirects the probe the same way
GIT_DIR does, and rewrite the "whole set" docstring to say only what the
cases prove: GIT_DIR and GIT_COMMON_DIR each redirect on their own,
GIT_WORK_TREE and GIT_OBJECT_DIRECTORY are stripped as part of the same
set without a demonstrated redirect for this call.

Update scripts/README.md's exit-64 paragraph to name `wait` alongside
`comment` and `reply`, since `wait` now refuses a cross-owner target
before its auto-request the same way the other two do.

Add a table-driven test asserting `comment`, `reply`, and `wait` each
refuse a cross-owner target before touching either transport. The
existing whole-source check only counts which mutation documents exist,
which is how `requestReviews` sat outside the owner check for its whole
life with a green suite; the new test pins the command-level invariant
instead and says in its own docstring that it does not pin the static
"every mutation document sits behind in_scope" property.

Not included: switching the probe from `git remote get-url origin` to
`git config --get remote.origin.url` closes the `insteadOf` vector but
not the write boundary itself. `config --get` is redirectable through a
different, more direct injection: GIT_CONFIG_COUNT/GIT_CONFIG_KEY_0 set
directly to `remote.origin.url` overrides local repo config for that
key, measured on git 2.47.3, where `remote get-url` does not honor that
same env-based override. Reported rather than shipped, since the task
that requested this swap says to stop rather than invent a third
approach when `config --get` also turns out redirectable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/README.md               |  2 +-
 scripts/pr_review.py            |  3 +-
 scripts/tests/test_pr_review.py | 86 ++++++++++++++++++++++++++++++---
 3 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/scripts/README.md b/scripts/README.md
index 3f5eb77c..5adc2661 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. 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.
+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` and `wait` both raise it on the same terms `reply` does, `wait` ahead of the auto-request its own backoff loop would otherwise reach. 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 c49cd39a..74d13190 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -1998,8 +1998,7 @@ def origin_owner() -> str | None:
     reached from a hub checkout while the repository being answered is named on the command line,
     so the working directory says nothing about who owns either.
     """
-    # Git honors GIT_DIR over `-C` for repository discovery.
-    # An inherited one (a hook, a `git bisect run`, a `git rebase --exec`) would make this answer for a different repository than the one this script sits in.
+    # An inherited GIT_DIR overrides the `-C` argument for repository discovery, so the probe must not inherit one.
     # Stripped rather than the environment being cleared wholesale, since git still needs PATH to be found at all and HOME to read the config this very call reads.
     env = {
         k: v
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 5bd4a343..d28a3189 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4195,10 +4195,20 @@ def test_a_cross_owner_refusal_names_that_directory_too(self) -> None:
 class TestOriginOwnerIgnoresInheritedGitDiscovery(unittest.TestCase):
     """#1561: an inherited GIT_DIR must not redirect the probe to a different repository."""
 
+    # The same four names origin_owner() strips, kept local rather than imported from production.
+    _DISCOVERY_VARS = ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")
+
     def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
+        """Build one throwaway repo under `parent`, immune to the caller's own git environment.
+
+        `git init` and `git remote add` both honor an inherited GIT_DIR the same way the probe
+        does, so without this strip an ambient one sends `git remote add` writing into whatever
+        repository that variable names instead of the throwaway repo this helper just created.
+        """
+        env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
         repo = parent / name
         repo.mkdir()
-        subprocess.run(["git", "init", "--quiet", str(repo)], check=True)
+        subprocess.run(["git", "init", "--quiet", str(repo)], check=True, env=env)
         subprocess.run(
             [
                 "git",
@@ -4210,14 +4220,15 @@ def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
                 f"https://github.com/{owner_slash_repo}.git",
             ],
             check=True,
+            env=env,
         )
         return repo
 
     def test_a_git_dir_inherited_from_the_caller_does_not_redirect_the_probe(self) -> None:
-        """A git hook, `git bisect run`, and `git rebase --exec` all export GIT_DIR.
+        """An inherited GIT_DIR overrides `-C` for repository discovery.
 
-        Git honours it over `-C` for repository discovery, so an inherited one that names a
-        different repository must not make this probe answer for that repository instead.
+        So an inherited one that names a different repository must not make this probe answer
+        for that repository instead.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4227,12 +4238,27 @@ def test_a_git_dir_inherited_from_the_caller_does_not_redirect_the_probe(self) -
             with mock.patch.dict(os.environ, {"GIT_DIR": str(other / ".git")}):
                 self.assertEqual("acme", pr_review.origin_owner())
 
+    def test_a_git_common_dir_inherited_from_the_caller_does_not_redirect_the_probe(self) -> None:
+        """An inherited GIT_COMMON_DIR redirects the probe on its own, the same way GIT_DIR does.
+
+        Config is read from the common dir, so this one needs no other inherited name alongside
+        it to make the probe answer for the wrong repository.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            other = self.make_repo(tmp_path, "other", "unrelated-owner/other-repo")
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            with mock.patch.dict(os.environ, {"GIT_COMMON_DIR": str(other / ".git")}):
+                self.assertEqual("acme", pr_review.origin_owner())
+
     def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) -> None:
         """The four names are stripped as a set, and this pins the set rather than each one.
 
-        Only GIT_DIR is shown above to redirect `remote get-url` away from a `-C` argument, so
-        a case per name would assert a redirection the other three are not demonstrated to
-        produce. What this holds is that all four inherited at once still leave the probe
+        GIT_DIR and GIT_COMMON_DIR each redirect `remote get-url` away from a `-C` argument on
+        their own, and each has its own case above. GIT_WORK_TREE and GIT_OBJECT_DIRECTORY are
+        stripped as part of the same discovery set without a demonstrated redirect for this
+        call. What this holds is that all four inherited at once still leave the probe
         answering for the directory the script sits in.
         """
         with tempfile.TemporaryDirectory() as tmp:
@@ -4270,6 +4296,52 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin
         gh_graphql.assert_not_called()
 
 
+# Update this table when a fourth write subcommand joins the parser's own `cmd` choices.
+WRITE_COMMANDS: dict[str, list[str]] = {
+    "comment": ["comment", "7", "--repo", "someone-else/r", "--body", "Fixed."],
+    "reply": [
+        "reply",
+        "7",
+        "--repo",
+        "someone-else/r",
+        "--match",
+        "retry count",
+        "--body",
+        "Fixed.",
+    ],
+    "wait": ["wait", "7", "--repo", "someone-else/r"],
+}
+
+
+class TestEveryWriteCommandRefusesCrossOwner(unittest.TestCase):
+    """`requestReviews` once sat outside `in_scope` for its whole life with a green suite.
+
+    The whole-source check in TestContract counts which mutation documents exist, which held
+    that regression invisible since a document can exist and still be reachable without the
+    owner check running first. This pins the command-level invariant instead: every write
+    subcommand refuses a cross-owner target before either transport is touched. It does not
+    pin the static property TestContract's count implied, that every mutation document in the
+    source sits behind `in_scope`; a write reachable by some path this table does not drive
+    would still slip past it.
+    """
+
+    def test_each_write_command_refuses_before_reaching_either_transport(self) -> None:
+        for cmd, argv in WRITE_COMMANDS.items():
+            with self.subTest(cmd=cmd):
+                boom = AssertionError(f"{cmd}: no network call should be reached out of scope")
+                with (
+                    contextlib.redirect_stdout(io.StringIO()) as out,
+                    mock.patch.object(pr_review, "origin_owner", return_value="acme"),
+                    mock.patch.object(pr_review, "gql", side_effect=boom) as gql,
+                    mock.patch.object(pr_review, "gh_graphql", side_effect=boom) as gh_graphql,
+                ):
+                    code = pr_review.main(argv)
+                self.assertEqual(64, code)
+                self.assertIn("status=OUT_OF_SCOPE", out.getvalue())
+                gql.assert_not_called()
+                gh_graphql.assert_not_called()
+
+
 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."""

From cf529a3fb57ba588048fa9e90b25ac4bc0847ba8 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 07:51:36 -0700
Subject: [PATCH 5/9] Read remote.origin.url from local config, not remote
 get-url, in origin_owner

git remote get-url applies insteadOf rewriting, so an inherited config
rewrite can make the probe name an owner the checkout does not have.
git config --local --get reads this repository's own config file
directly, which is the question origin_owner() is actually asking:
which owner does the checkout this script sits in belong to. The
existing GIT_DIR/GIT_COMMON_DIR/GIT_WORK_TREE/GIT_OBJECT_DIRECTORY
environment strip stays required for a separate reason: it decides
which repository's local config is read, while --local decides which
scope of that repository's config is read.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/pr_review.py            |  6 ++-
 scripts/tests/test_pr_review.py | 66 +++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/scripts/pr_review.py b/scripts/pr_review.py
index 74d13190..6d7cab5b 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -1998,8 +1998,10 @@ def origin_owner() -> str | None:
     reached from a hub checkout while the repository being answered is named on the command line,
     so the working directory says nothing about who owns either.
     """
-    # An inherited GIT_DIR overrides the `-C` argument for repository discovery, so the probe must not inherit one.
+    # An inherited GIT_DIR or GIT_COMMON_DIR overrides the `-C` argument for repository discovery, so the probe must not inherit either one.
     # Stripped rather than the environment being cleared wholesale, since git still needs PATH to be found at all and HOME to read the config this very call reads.
+    # `git remote get-url` applies `insteadOf` rewriting, so an injected rewrite can make it name an owner this checkout does not have.
+    # `--local` reads this repository's own config file directly, which is the question actually being asked: which owner does the checkout this script sits in belong to.
     env = {
         k: v
         for k, v in os.environ.items()
@@ -2007,7 +2009,7 @@ def origin_owner() -> str | None:
     }
     try:
         url = subprocess.run(
-            ["git", "-C", str(HERE), "remote", "get-url", "origin"],
+            ["git", "-C", str(HERE), "config", "--local", "--get", "remote.origin.url"],
             capture_output=True,
             text=True,
             encoding="utf-8",
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index d28a3189..7e82ec91 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4275,6 +4275,72 @@ def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) ->
             with mock.patch.dict(os.environ, env):
                 self.assertEqual("acme", pr_review.origin_owner())
 
+    def test_an_instead_of_rewrite_injected_via_git_config_count_does_not_redirect_the_probe(
+        self,
+    ) -> None:
+        """`insteadOf` rewriting is the vector `git remote get-url` applies and `--local` does not.
+
+        The injected config rewrites this checkout's real `acme` origin to read as
+        `unrelated-owner`. `--local` reads the repository's own config file directly rather than
+        asking git to resolve and rewrite the URL, so the probe still answers for the real owner.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            env = {
+                "GIT_CONFIG_COUNT": "1",
+                "GIT_CONFIG_KEY_0": "url.https://github.com/unrelated-owner/.insteadof",
+                "GIT_CONFIG_VALUE_0": "https://github.com/acme/",
+            }
+            with mock.patch.dict(os.environ, env):
+                self.assertEqual("acme", pr_review.origin_owner())
+
+    def test_the_same_instead_of_rewrite_delivered_via_a_git_config_global_file_does_not_redirect_the_probe(
+        self,
+    ) -> None:
+        """Same rewrite as above, delivered through `GIT_CONFIG_GLOBAL` instead of the count/key/value form.
+
+        `--local` reads only this repository's own config file, so a rewrite sitting in a global
+        config file the environment points at is never consulted and the probe still answers for
+        the real owner.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            global_config = tmp_path / "injected-global-gitconfig"
+            global_config.write_text(
+                '[url "https://github.com/unrelated-owner/"]\n'
+                "\tinsteadOf = https://github.com/acme/\n",
+                encoding="utf-8",
+            )
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            with mock.patch.dict(os.environ, {"GIT_CONFIG_GLOBAL": str(global_config)}):
+                self.assertEqual("acme", pr_review.origin_owner())
+
+    def test_a_direct_remote_origin_url_override_injected_via_git_config_count_does_not_redirect_the_probe(
+        self,
+    ) -> None:
+        """A `remote.origin.url` override injected the same count/key/value way as the rewrite above.
+
+        This one is not a regression guard for the reverted `git remote get-url origin` command:
+        that command ignores this particular override and already answered `acme` before this
+        change. What `--local` guards against here is a plain `git config --get` with no
+        `--local`, which does read this override and answers wrong; `--local` restricts the read
+        to this repository's own config file, so the injected override is never applied.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            env = {
+                "GIT_CONFIG_COUNT": "1",
+                "GIT_CONFIG_KEY_0": "remote.origin.url",
+                "GIT_CONFIG_VALUE_0": "https://github.com/unrelated-owner/other-repo.git",
+            }
+            with mock.patch.dict(os.environ, env):
+                self.assertEqual("acme", pr_review.origin_owner())
+
 
 class TestWaitStaysInScope(unittest.TestCase):
     """#1562: `wait` mutates via the auto-request, so it must refuse cross-owner outright too."""

From 19bedea29fdcbd8569627f0b02430c6f649db381 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 07:53:04 -0700
Subject: [PATCH 6/9] Name the command the probe now runs on every surface that
 states it

Switching the probe to `git config --local --get remote.origin.url` left four
surfaces still naming `git remote get-url origin`: the function's own docstring,
both refusal messages a reader sees when a write is refused, and the exit-64
paragraph in the scripts README. A refusal that names a command the script does not
run sends a reader to reproduce something that cannot reproduce the refusal.

The assertion pinning the refusal's text moves with it, and one test docstring that
described which variables redirect `remote get-url` now says the probe instead, since
the measurement holds for whichever command the probe runs.

Three mentions of the old command survive deliberately: the comment giving the reason
the probe does not use it, and the two cases that name it as the command they revert
to in order to prove a vector.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/README.md               | 2 +-
 scripts/pr_review.py            | 6 +++---
 scripts/tests/test_pr_review.py | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/README.md b/scripts/README.md
index 5adc2661..62f312b4 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. Exit `64` is the scope refusal, and `comment` and `wait` both raise it on the same terms `reply` does, `wait` ahead of the auto-request its own backoff loop would otherwise reach. 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.
+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` and `wait` both raise it on the same terms `reply` does, `wait` ahead of the auto-request its own backoff loop would otherwise reach. It covers two states rather than one: a target under an owner other than the one `git config --local --get remote.origin.url` 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 6d7cab5b..6c39b611 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -1992,7 +1992,7 @@ def digest(
 
 
 def origin_owner() -> str | None:
-    """The owner `git remote get-url origin` yields against this script's own directory, or None.
+    """The owner `git config --local --get remote.origin.url` 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,
@@ -2038,7 +2038,7 @@ def in_scope(target_owner: str) -> tuple[bool, str]:
     origin = origin_owner()
     if origin is None:
         return False, (
-            "this script got no owner from `git remote get-url origin` against the directory "
+            "this script got no owner from `git config --local --get remote.origin.url` 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 "
@@ -2048,7 +2048,7 @@ def in_scope(target_owner: str) -> tuple[bool, str]:
         )
     if target_owner.lower() != origin:
         return False, (
-            f"the target is under {target_owner}, and `git remote get-url origin`, run against "
+            f"the target is under {target_owner}, and `git config --local --get remote.origin.url`, 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"
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 7e82ec91..e977b2a2 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4179,7 +4179,7 @@ def test_an_unreadable_origin_names_that_directory_and_a_remedy(self) -> None:
         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.assertIn("git config --local --get remote.origin.url", why)
         self.assertNotIn("this checkout", why)
 
     def test_a_cross_owner_refusal_names_that_directory_too(self) -> None:
@@ -4255,7 +4255,7 @@ def test_a_git_common_dir_inherited_from_the_caller_does_not_redirect_the_probe(
     def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) -> None:
         """The four names are stripped as a set, and this pins the set rather than each one.
 
-        GIT_DIR and GIT_COMMON_DIR each redirect `remote get-url` away from a `-C` argument on
+        GIT_DIR and GIT_COMMON_DIR each redirect the probe away from a `-C` argument on
         their own, and each has its own case above. GIT_WORK_TREE and GIT_OBJECT_DIRECTORY are
         stripped as part of the same discovery set without a demonstrated redirect for this
         call. What this holds is that all four inherited at once still leave the probe

From 3f1b3282f2ad93eae1876f4e7d548877d5dcd1aa Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 08:16:23 -0700
Subject: [PATCH 7/9] Restore `remote get-url origin` and strip GIT_CONFIG*
 instead of --local

`git config --local --get remote.origin.url` was itself wrong in three ways:
it reads the LAST of multiple `url` entries where `remote get-url` reads the
FIRST (an inversion that widens write scope under two origin urls), it does
not follow `include.path`/`includeIf`, and a URL-shorthand alias origin
yields nothing to parse. Go back to `git remote get-url origin` and close the
injection vector at its actual source: strip the four discovery variables
plus every GIT_CONFIG*-prefixed name from the probe's environment instead.

Also corrects four false claims from prior rounds: an unpinned "four names
stripped as a set" test docstring, a `wait` comment that misdescribed why the
scope check precedes the read, a README line describing the auto-request as
inside `wait`'s backoff loop, and a test class named for only the discovery
half of what it covers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/README.md               |   2 +-
 scripts/pr_review.py            |  18 ++---
 scripts/tests/test_pr_review.py | 135 +++++++++++++++++++++++++++-----
 3 files changed, 124 insertions(+), 31 deletions(-)

diff --git a/scripts/README.md b/scripts/README.md
index 62f312b4..f671e771 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. Exit `64` is the scope refusal, and `comment` and `wait` both raise it on the same terms `reply` does, `wait` ahead of the auto-request its own backoff loop would otherwise reach. It covers two states rather than one: a target under an owner other than the one `git config --local --get remote.origin.url` 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.
+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` and `wait` both raise it on the same terms `reply` does, `wait` ahead of both the auto-request and the backoff loop that follows it, since the check runs before either 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 6c39b611..3c752e3c 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -1992,24 +1992,24 @@ def digest(
 
 
 def origin_owner() -> str | None:
-    """The owner `git config --local --get remote.origin.url` yields against this script's own directory, or None.
+    """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,
     so the working directory says nothing about who owns either.
     """
-    # An inherited GIT_DIR or GIT_COMMON_DIR overrides the `-C` argument for repository discovery, so the probe must not inherit either one.
-    # Stripped rather than the environment being cleared wholesale, since git still needs PATH to be found at all and HOME to read the config this very call reads.
-    # `git remote get-url` applies `insteadOf` rewriting, so an injected rewrite can make it name an owner this checkout does not have.
-    # `--local` reads this repository's own config file directly, which is the question actually being asked: which owner does the checkout this script sits in belong to.
+    # An inherited GIT_DIR or GIT_COMMON_DIR overrides the `-C` argument and points the probe at another repository's config.
+    # An inherited GIT_CONFIG* channel injects config into this call, and `remote get-url` applies `insteadOf` rewriting, so an injected rewrite would make it name an owner this checkout does not have.
+    # The environment is stripped rather than cleared, since git still needs PATH to be found at all, and HOME so the checkout's own global config and `safe.directory` still apply.
     env = {
         k: v
         for k, v in os.environ.items()
         if k not in ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")
+        and not k.startswith("GIT_CONFIG")
     }
     try:
         url = subprocess.run(
-            ["git", "-C", str(HERE), "config", "--local", "--get", "remote.origin.url"],
+            ["git", "-C", str(HERE), "remote", "get-url", "origin"],
             capture_output=True,
             text=True,
             encoding="utf-8",
@@ -2038,7 +2038,7 @@ def in_scope(target_owner: str) -> tuple[bool, str]:
     origin = origin_owner()
     if origin is None:
         return False, (
-            "this script got no owner from `git config --local --get remote.origin.url` against the directory "
+            "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 "
@@ -2048,7 +2048,7 @@ def in_scope(target_owner: str) -> tuple[bool, str]:
         )
     if target_owner.lower() != origin:
         return False, (
-            f"the target is under {target_owner}, and `git config --local --get remote.origin.url`, run against "
+            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"
@@ -2511,7 +2511,7 @@ def main(argv: list[str] | None = None) -> int:
         return reply_to_thread(owner, repo, a.number, a.match, a.body, a.path, a.resolve)
 
     # `wait` mutates through the auto-request below, so it refuses a cross-owner target the way `comment` and `reply` do.
-    # Ahead of the first read rather than at the request itself, since a wait that may not request has nothing left to poll for.
+    # The policy is that a cross-owner target is not touched at all, so the refusal precedes the reading half too.
     ok, why = in_scope(owner)
     if not ok:
         print(f"status=OUT_OF_SCOPE nothing was written: {why}")
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index e977b2a2..329c055a 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4179,7 +4179,7 @@ def test_an_unreadable_origin_names_that_directory_and_a_remedy(self) -> None:
         self.assertIn(str(self.ANCHOR), why)
         self.assertIn("hub checkout", why)
         self.assertIn("instead of this one", why)
-        self.assertIn("git config --local --get remote.origin.url", 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:
@@ -4192,10 +4192,15 @@ def test_a_cross_owner_refusal_names_that_directory_too(self) -> None:
         self.assertNotIn("this checkout", why)
 
 
-class TestOriginOwnerIgnoresInheritedGitDiscovery(unittest.TestCase):
-    """#1561: an inherited GIT_DIR must not redirect the probe to a different repository."""
+class TestOriginOwnerIgnoresInheritedEnvironment(unittest.TestCase):
+    """#1561: an inherited environment must not redirect the probe, by either mechanism.
 
-    # The same four names origin_owner() strips, kept local rather than imported from production.
+    A `GIT_DIR`/`GIT_WORK_TREE`/`GIT_COMMON_DIR`/`GIT_OBJECT_DIRECTORY` name overrides repository
+    discovery, and a `GIT_CONFIG*` name injects config into the call, so both halves are covered
+    here rather than only the discovery names the class was previously named for.
+    """
+
+    # The same four discovery names origin_owner() strips, kept local rather than imported from production.
     _DISCOVERY_VARS = ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")
 
     def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
@@ -4253,13 +4258,12 @@ def test_a_git_common_dir_inherited_from_the_caller_does_not_redirect_the_probe(
                 self.assertEqual("acme", pr_review.origin_owner())
 
     def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) -> None:
-        """The four names are stripped as a set, and this pins the set rather than each one.
+        """GIT_DIR and GIT_COMMON_DIR each redirect the probe on their own, and each has its own case above.
 
-        GIT_DIR and GIT_COMMON_DIR each redirect the probe away from a `-C` argument on
-        their own, and each has its own case above. GIT_WORK_TREE and GIT_OBJECT_DIRECTORY are
-        stripped as part of the same discovery set without a demonstrated redirect for this
-        call. What this holds is that all four inherited at once still leave the probe
-        answering for the directory the script sits in.
+        GIT_WORK_TREE and GIT_OBJECT_DIRECTORY are stripped as part of the same documented
+        discovery set, with no redirect of their own demonstrated for this call. What this holds
+        is that all four inherited at once still leave the probe answering for the directory the
+        script sits in.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4278,11 +4282,12 @@ def test_the_whole_inherited_discovery_set_does_not_redirect_the_probe(self) ->
     def test_an_instead_of_rewrite_injected_via_git_config_count_does_not_redirect_the_probe(
         self,
     ) -> None:
-        """`insteadOf` rewriting is the vector `git remote get-url` applies and `--local` does not.
+        """`remote get-url` applies `insteadOf` rewriting, so an unstripped injection here would rewrite the answer.
 
         The injected config rewrites this checkout's real `acme` origin to read as
-        `unrelated-owner`. `--local` reads the repository's own config file directly rather than
-        asking git to resolve and rewrite the URL, so the probe still answers for the real owner.
+        `unrelated-owner`. The `GIT_CONFIG` prefix strip removes `GIT_CONFIG_COUNT`,
+        `GIT_CONFIG_KEY_0`, and `GIT_CONFIG_VALUE_0` before the call runs, so the real config
+        governs and the probe still answers for the real owner.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4301,9 +4306,10 @@ def test_the_same_instead_of_rewrite_delivered_via_a_git_config_global_file_does
     ) -> None:
         """Same rewrite as above, delivered through `GIT_CONFIG_GLOBAL` instead of the count/key/value form.
 
-        `--local` reads only this repository's own config file, so a rewrite sitting in a global
-        config file the environment points at is never consulted and the probe still answers for
-        the real owner.
+        `remote get-url` merges the global config into what it resolves, so an unstripped
+        `GIT_CONFIG_GLOBAL` pointing at a file carrying this rewrite would still rewrite the
+        answer. `GIT_CONFIG_GLOBAL` matches the same `GIT_CONFIG` prefix, so it is stripped
+        before the call runs and the probe still answers for the real owner.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4323,11 +4329,11 @@ def test_a_direct_remote_origin_url_override_injected_via_git_config_count_does_
     ) -> None:
         """A `remote.origin.url` override injected the same count/key/value way as the rewrite above.
 
-        This one is not a regression guard for the reverted `git remote get-url origin` command:
-        that command ignores this particular override and already answered `acme` before this
-        change. What `--local` guards against here is a plain `git config --get` with no
-        `--local`, which does read this override and answers wrong; `--local` restricts the read
-        to this repository's own config file, so the injected override is never applied.
+        Not a regression guard for the current probe: measured directly, `git remote get-url`
+        ignores a config-injected `remote.origin.url` value regardless of whether the
+        `GIT_CONFIG` prefix is stripped, so this case passes with or without that strip. It stays
+        in place for a future change that switches the probe to a command that does honor this
+        vector, such as `git config --get remote.origin.url`, which the strip does guard against.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4341,6 +4347,93 @@ def test_a_direct_remote_origin_url_override_injected_via_git_config_count_does_
             with mock.patch.dict(os.environ, env):
                 self.assertEqual("acme", pr_review.origin_owner())
 
+    def test_two_origin_urls_resolve_to_the_first_one(self) -> None:
+        """`remote.origin.url` may carry more than one value; `remote get-url` returns the first.
+
+        `git config --get` instead returns the last, which is the inversion this round exists to
+        close: with two urls, that command answers a different owner than the one a write should
+        stay within. Both commands are run against the same repository here so the difference is
+        the proof, rather than reverting and restoring the production probe.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
+            subprocess.run(
+                [
+                    "git",
+                    "-C",
+                    str(anchor),
+                    "config",
+                    "--local",
+                    "--add",
+                    "remote.origin.url",
+                    "https://github.com/second-owner/anchor-repo.git",
+                ],
+                check=True,
+                env=env,
+            )
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            self.assertEqual("acme", pr_review.origin_owner())
+            reverted = subprocess.run(
+                ["git", "-C", str(anchor), "config", "--local", "--get", "remote.origin.url"],
+                capture_output=True,
+                text=True,
+                encoding="utf-8",
+                check=True,
+                env=env,
+            ).stdout.strip()
+            self.assertIn("second-owner", reverted)
+
+    def test_an_origin_url_delivered_via_include_path_still_resolves(self) -> None:
+        """A checkout whose `remote.origin.url` arrives through `include.path` rather than a literal value.
+
+        `git config --local` does not follow `include.path`/`includeIf`, so under that command
+        this state read as no owner and every write refused. `remote get-url` does follow
+        includes, so the probe still resolves the real owner. Both commands are run against the
+        same repository here so the difference is the proof, rather than reverting and restoring
+        the production probe.
+        """
+        with tempfile.TemporaryDirectory() as tmp:
+            tmp_path = Path(tmp)
+            anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
+            env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
+            subprocess.run(
+                ["git", "-C", str(anchor), "config", "--local", "--unset", "remote.origin.url"],
+                check=True,
+                env=env,
+            )
+            included = tmp_path / "origin-include.gitconfig"
+            included.write_text(
+                '[remote "origin"]\n\turl = https://github.com/acme/anchor-repo.git\n',
+                encoding="utf-8",
+            )
+            subprocess.run(
+                [
+                    "git",
+                    "-C",
+                    str(anchor),
+                    "config",
+                    "--local",
+                    "--add",
+                    "include.path",
+                    str(included),
+                ],
+                check=True,
+                env=env,
+            )
+            self.enterContext(mock.patch.object(pr_review, "HERE", anchor))
+            self.assertEqual("acme", pr_review.origin_owner())
+            reverted = subprocess.run(
+                ["git", "-C", str(anchor), "config", "--local", "--get", "remote.origin.url"],
+                capture_output=True,
+                text=True,
+                encoding="utf-8",
+                check=False,
+                env=env,
+            )
+            self.assertEqual(1, reverted.returncode)
+
 
 class TestWaitStaysInScope(unittest.TestCase):
     """#1562: `wait` mutates via the auto-request, so it must refuse cross-owner outright too."""

From a9547e3045669e56a6d6cfe0ebd023a9b4fd77b5 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 08:43:01 -0700
Subject: [PATCH 8/9] Correct seven prose claims in the PR-review
 write-boundary round

Rewrites the origin_owner() env-strip comment to state its actual limit
(closes inherited discovery/GIT_CONFIG* channels, cannot close a caller-
controlled HOME/XDG_CONFIG_HOME/PATH), fixes a test class docstring's false
claim that all four stripped names override discovery, corrects a
maintenance note to name the real trigger for a stale write-command table
(a subcommand's write status changing, not only one being added), adds
README `wait` coverage for exit 64, corrects a README sentence that
understated the mutation-document count the whole-source test actually
pins, and makes three test docstrings self-contained against a squash
merge instead of pointing at this branch's own intermediate commits.

Adds one new assertion tying WRITE_COMMANDS/READ_ONLY_COMMANDS to the
parser's own `cmd` choices, so a subcommand added without being classified
fails the suite instead of silently falling through both sets.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/README.md               |  4 ++-
 scripts/pr_review.py            |  2 ++
 scripts/tests/test_pr_review.py | 60 ++++++++++++++++++++++++++-------
 3 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/scripts/README.md b/scripts/README.md
index f671e771..7e21a7de 100644
--- a/scripts/README.md
+++ b/scripts/README.md
@@ -168,7 +168,9 @@ python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \
 
 `--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 holds the script to exactly the two mutations `reply` owns, so a third arriving is a write nobody reviewed as one.
+`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 holds the script to exactly the four mutation documents named `addComment`, `requestReviews`, `addPullRequestReviewThreadReply`, and `resolveReviewThread`, so a fifth arriving is a write nobody reviewed as one.
+
+`wait` exits `64` when the target is out of scope, the same refusal `comment` and `reply` raise, checked before either the auto-request or the backoff loop reaches GitHub. The `reply` paragraph below states what that refusal covers.
 
 `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 3c752e3c..bb364e66 100755
--- a/scripts/pr_review.py
+++ b/scripts/pr_review.py
@@ -2000,6 +2000,8 @@ def origin_owner() -> str | None:
     """
     # An inherited GIT_DIR or GIT_COMMON_DIR overrides the `-C` argument and points the probe at another repository's config.
     # An inherited GIT_CONFIG* channel injects config into this call, and `remote get-url` applies `insteadOf` rewriting, so an injected rewrite would make it name an owner this checkout does not have.
+    # This strip closes the channels an inherited tooling environment sets: the four discovery names it drops, and any name starting with GIT_CONFIG that injects config into this one call.
+    # It does not close, and cannot close, an inherited HOME, XDG_CONFIG_HOME, or PATH, because a caller who controls any of those already controls the process that would perform the write, leaving nothing here for this check to defend.
     # The environment is stripped rather than cleared, since git still needs PATH to be found at all, and HOME so the checkout's own global config and `safe.directory` still apply.
     env = {
         k: v
diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 329c055a..549569b6 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -11,6 +11,7 @@
 
 from __future__ import annotations
 
+import argparse
 import contextlib
 import io
 import json
@@ -4195,9 +4196,11 @@ def test_a_cross_owner_refusal_names_that_directory_too(self) -> None:
 class TestOriginOwnerIgnoresInheritedEnvironment(unittest.TestCase):
     """#1561: an inherited environment must not redirect the probe, by either mechanism.
 
-    A `GIT_DIR`/`GIT_WORK_TREE`/`GIT_COMMON_DIR`/`GIT_OBJECT_DIRECTORY` name overrides repository
-    discovery, and a `GIT_CONFIG*` name injects config into the call, so both halves are covered
-    here rather than only the discovery names the class was previously named for.
+    `GIT_DIR` and `GIT_COMMON_DIR` each override repository discovery on their own. `GIT_WORK_TREE`
+    and `GIT_OBJECT_DIRECTORY` are stripped as part of the same discovery set but demonstrate no
+    redirect of their own here, and `GIT_OBJECT_DIRECTORY` overrides the object store rather than
+    discovery. A `GIT_CONFIG*` name injects config into the call instead, so both mechanisms are
+    covered here.
     """
 
     # The same four discovery names origin_owner() strips, kept local rather than imported from production.
@@ -4350,9 +4353,9 @@ def test_a_direct_remote_origin_url_override_injected_via_git_config_count_does_
     def test_two_origin_urls_resolve_to_the_first_one(self) -> None:
         """`remote.origin.url` may carry more than one value; `remote get-url` returns the first.
 
-        `git config --get` instead returns the last, which is the inversion this round exists to
-        close: with two urls, that command answers a different owner than the one a write should
-        stay within. Both commands are run against the same repository here so the difference is
+        `git config --get` instead returns the last: with two urls, that command names a
+        different owner than the one a write should stay within, the opposite of what this probe
+        needs. Both commands are run against the same repository here so the difference is
         the proof, rather than reverting and restoring the production probe.
         """
         with tempfile.TemporaryDirectory() as tmp:
@@ -4388,11 +4391,11 @@ def test_two_origin_urls_resolve_to_the_first_one(self) -> None:
     def test_an_origin_url_delivered_via_include_path_still_resolves(self) -> None:
         """A checkout whose `remote.origin.url` arrives through `include.path` rather than a literal value.
 
-        `git config --local` does not follow `include.path`/`includeIf`, so under that command
-        this state read as no owner and every write refused. `remote get-url` does follow
-        includes, so the probe still resolves the real owner. Both commands are run against the
-        same repository here so the difference is the proof, rather than reverting and restoring
-        the production probe.
+        `git config --local` does not follow `include.path`/`includeIf`, so a probe built on that
+        command instead would read no owner here and refuse every write. `remote get-url` does
+        follow includes, so the probe in this script still resolves the real owner. Both commands
+        are run against the same repository here so the difference is the proof, rather than
+        reverting and restoring the production probe.
         """
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
@@ -4455,7 +4458,8 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin
         gh_graphql.assert_not_called()
 
 
-# Update this table when a fourth write subcommand joins the parser's own `cmd` choices.
+# `wait` was already in the parser's `cmd` choices when it became a write, by acquiring a `requestReviews` mutation, with no subcommand added.
+# Update this table whenever a command's write status changes, not only when a new one is added.
 WRITE_COMMANDS: dict[str, list[str]] = {
     "comment": ["comment", "7", "--repo", "someone-else/r", "--body", "Fixed."],
     "reply": [
@@ -4471,6 +4475,10 @@ def test_a_cross_owner_target_is_refused_before_the_wait_reads_or_writes_anythin
     "wait": ["wait", "7", "--repo", "someone-else/r"],
 }
 
+# The parser's remaining `cmd` choices, none of which write.
+# The partition test below checks that this set and WRITE_COMMANDS together match the parser's choices.
+READ_ONLY_COMMANDS = ["claims", "status"]
+
 
 class TestEveryWriteCommandRefusesCrossOwner(unittest.TestCase):
     """`requestReviews` once sat outside `in_scope` for its whole life with a green suite.
@@ -4501,6 +4509,34 @@ def test_each_write_command_refuses_before_reaching_either_transport(self) -> No
                 gh_graphql.assert_not_called()
 
 
+class TestWriteCommandsPartitionParserChoices(unittest.TestCase):
+    """WRITE_COMMANDS and READ_ONLY_COMMANDS must together account for every parser choice.
+
+    `wait` became a write without a subcommand being added, so nothing prompted the table to be
+    checked. This ties the two hand-maintained sets to the parser's own `cmd` choices instead of
+    trusting them to stay in step: a subcommand added to the parser and left out of both sets
+    fails this test until someone classifies it as a write or a read.
+    """
+
+    def test_write_and_read_only_commands_together_equal_the_parsers_cmd_choices(self) -> None:
+        original_add_argument = argparse.ArgumentParser.add_argument
+        captured_choices = []
+
+        def capture(self, *args, **kwargs):
+            action = original_add_argument(self, *args, **kwargs)
+            if args and args[0] == "cmd":
+                captured_choices.append(action.choices)
+            return action
+
+        with (
+            mock.patch.object(argparse.ArgumentParser, "add_argument", capture),
+            contextlib.redirect_stderr(io.StringIO()),
+            self.assertRaises(SystemExit),
+        ):
+            pr_review.main([])
+        self.assertEqual(set(captured_choices[0]), set(WRITE_COMMANDS) | set(READ_ONLY_COMMANDS))
+
+
 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."""

From 5b03d49465dc2c7bccbfcd64e6549a04673bb037 Mon Sep 17 00:00:00 2001
From: Pieter Viljoen <ptr727@users.noreply.github.com>
Date: Sun, 13 Sep 2026 09:01:48 -0700
Subject: [PATCH 9/9] Build the throwaway repositories on the same environment
 the probe runs on

The fixture stripped the four discovery names and left every GIT_CONFIG channel in
place, so it isolated itself from half of what the probe isolates itself from. The
two halves have to match: a fixture built under an influence the cases exist to rule
out is a fixture that cannot rule it out.

Three call sites built that environment inline with the same comprehension, which is
how they came to disagree with the probe in the first place, so they now share one
helper and a later change to the rule reaches all of them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/tests/test_pr_review.py | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py
index 549569b6..5cb8c257 100755
--- a/scripts/tests/test_pr_review.py
+++ b/scripts/tests/test_pr_review.py
@@ -4206,6 +4206,19 @@ class TestOriginOwnerIgnoresInheritedEnvironment(unittest.TestCase):
     # The same four discovery names origin_owner() strips, kept local rather than imported from production.
     _DISCOVERY_VARS = ("GIT_DIR", "GIT_WORK_TREE", "GIT_COMMON_DIR", "GIT_OBJECT_DIRECTORY")
 
+    def _clean_env(self) -> dict[str, str]:
+        """The caller's environment minus every channel the probe itself drops.
+
+        A fixture that left them in place would build its repositories under the same influence
+        the cases exist to rule out, and the two halves have to match for that reason rather
+        than because the setup commands are known to read each one.
+        """
+        return {
+            k: v
+            for k, v in os.environ.items()
+            if k not in self._DISCOVERY_VARS and not k.startswith("GIT_CONFIG")
+        }
+
     def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
         """Build one throwaway repo under `parent`, immune to the caller's own git environment.
 
@@ -4213,7 +4226,7 @@ def make_repo(self, parent: Path, name: str, owner_slash_repo: str) -> Path:
         does, so without this strip an ambient one sends `git remote add` writing into whatever
         repository that variable names instead of the throwaway repo this helper just created.
         """
-        env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
+        env = self._clean_env()
         repo = parent / name
         repo.mkdir()
         subprocess.run(["git", "init", "--quiet", str(repo)], check=True, env=env)
@@ -4361,7 +4374,7 @@ def test_two_origin_urls_resolve_to_the_first_one(self) -> None:
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
             anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
-            env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
+            env = self._clean_env()
             subprocess.run(
                 [
                     "git",
@@ -4400,7 +4413,7 @@ def test_an_origin_url_delivered_via_include_path_still_resolves(self) -> None:
         with tempfile.TemporaryDirectory() as tmp:
             tmp_path = Path(tmp)
             anchor = self.make_repo(tmp_path, "anchor", "acme/anchor-repo")
-            env = {k: v for k, v in os.environ.items() if k not in self._DISCOVERY_VARS}
+            env = self._clean_env()
             subprocess.run(
                 ["git", "-C", str(anchor), "config", "--local", "--unset", "remote.origin.url"],
                 check=True,