From b2b54fe4f73bd1be1587a975d2f307f4966f420f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 21 Oct 2026 20:14:59 -0700 Subject: [PATCH 1/4] Share One Git Location Variable List Across Self-Tests A self-test or test that builds a temporary repository with git -C or cwd= inherits any GIT_DIR, GIT_INDEX_FILE, and similar variable a git hook sets, so it writes into the hook's repository. spec/audit.py --selftest cleared nothing and ran init, config, add, and commit that way. - spec/git_env.py holds the full list (git rev-parse --local-env-vars plus three discovery controls), a without_git_location() copy helper, and a git_location_cleared() context manager. - spec/audit.py wraps its self-test in the context manager. - scripts/local_review.py, tests/test_pr_review.py, and tests/test_pre_commit_hook.py use the shared list in place of their own variants. - tests/test_git_env.py runs the audit self-test under a GIT_DIR naming a scratch repository and asserts its config and history are unchanged. Refs #3324 Co-Authored-By: Claude Sonnet 5.5 --- scripts/local_review.py | 5 +- spec/audit.py | 10 ++++ spec/git_env.py | 52 +++++++++++++++++++++ tests/test_git_env.py | 87 +++++++++++++++++++++++++++++++++++ tests/test_pr_review.py | 12 ++--- tests/test_pre_commit_hook.py | 9 +++- 6 files changed, 165 insertions(+), 10 deletions(-) create mode 100644 spec/git_env.py create mode 100755 tests/test_git_env.py diff --git a/scripts/local_review.py b/scripts/local_review.py index a594dd20f..5a9b6a75b 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -119,6 +119,9 @@ from datetime import UTC, datetime from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "spec")) +from git_env import GIT_LOCATION_VARS + RECEIPT_VERSION = 1 RECEIPT_NAME = "local-review-receipt.json" # Kept in the git directory rather than the working tree, so staging the tree does not stage these. @@ -216,7 +219,7 @@ class CannotRun(Exception): # A git hook runs with GIT_INDEX_FILE pointing at the commit it is gating, measured on a real pre-commit hook. # A partial commit points it at a pending index rather than the real one. # Inheriting that would silently read a different index than the one this branch would push. -INHERITED_REDIRECTS = ("GIT_INDEX_FILE", "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES") +INHERITED_REDIRECTS = GIT_LOCATION_VARS def git(*args: str, root: Path | None = None, env: dict[str, str] | None = None) -> str: diff --git a/spec/audit.py b/spec/audit.py index dee4204e0..6403550ed 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -54,6 +54,7 @@ from typing import Any import validate # sibling, import-safe (its main is guarded) +from git_env import git_location_cleared ROOT = pathlib.Path(__file__).resolve().parent.parent @@ -3764,6 +3765,15 @@ def read_text(path): def _selftest(): + """Run the self-test with no variable naming a repository in the environment. + + A git hook sets `GIT_DIR`. A case building a temporary repository would otherwise build it inside the hook's own repository. + """ + with git_location_cleared(): + return _selftest_cases() + + +def _selftest_cases(): """Exercise the interface engine on synthetic fixtures, no network - verify the mechanism, not the fleet.""" pr_check = " check-workflow-status:\n name: Check pull request workflow status job\n needs: [changes]\n runs-on: ubuntu-latest\n" pr_head = "name: Test\non: pull_request\njobs:\n changes:\n runs-on: ubuntu-latest\n" diff --git a/spec/git_env.py b/spec/git_env.py new file mode 100644 index 000000000..9e1112566 --- /dev/null +++ b/spec/git_env.py @@ -0,0 +1,52 @@ +"""The environment variables that name a git repository, and the ways to run without them. + +A git hook sets some of them (`GIT_DIR`, `GIT_INDEX_FILE`, and the like). A script or test that +builds a temporary repository with `git -C ` or `cwd=` and inherits them builds it +inside the hook's own repository instead, and reads its answers back from there. + +The list is what `git rev-parse --local-env-vars` prints, plus the three names that steer +discovery without being local to a repository. +""" + +from __future__ import annotations + +import contextlib +import os +from collections.abc import Iterator, Mapping + +GIT_LOCATION_VARS = ( + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_CONFIG", + "GIT_CONFIG_PARAMETERS", + "GIT_CONFIG_COUNT", + "GIT_OBJECT_DIRECTORY", + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_IMPLICIT_WORK_TREE", + "GIT_GRAFT_FILE", + "GIT_INDEX_FILE", + "GIT_NO_REPLACE_OBJECTS", + "GIT_REPLACE_REF_BASE", + "GIT_PREFIX", + "GIT_SHALLOW_FILE", + "GIT_COMMON_DIR", + "GIT_CEILING_DIRECTORIES", + "GIT_DISCOVERY_ACROSS_FILESYSTEM", + "GIT_NAMESPACE", +) + + +def without_git_location(env: Mapping[str, str] | None = None) -> dict[str, str]: + """Return a copy of `env` (default: the process environment) with every location variable removed.""" + source = os.environ if env is None else env + return {k: v for k, v in source.items() if k not in GIT_LOCATION_VARS} + + +@contextlib.contextmanager +def git_location_cleared() -> Iterator[None]: + """Remove every location variable from the process environment, restoring them on exit.""" + cleared = {name: os.environ.pop(name) for name in GIT_LOCATION_VARS if name in os.environ} + try: + yield + finally: + os.environ.update(cleared) diff --git a/tests/test_git_env.py b/tests/test_git_env.py new file mode 100755 index 000000000..ecfe3743e --- /dev/null +++ b/tests/test_git_env.py @@ -0,0 +1,87 @@ +#!/usr/bin/env python3 +"""The repository-location variables a self-test must not inherit from a git hook.""" + +from __future__ import annotations + +import os +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO / "spec")) +import git_env + + +def _git(cwd: Path, *args: str, env: dict[str, str] | None = None) -> str: + """A checked git call that reads UTF-8, run with no ambient location variable.""" + proc = subprocess.run( + ["git", *args], + cwd=cwd, + env=git_env.without_git_location(env), + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + return proc.stdout + + +class TestLocationList(unittest.TestCase): + def test_the_list_covers_every_name_git_calls_local(self) -> None: + printed = _git(REPO, "rev-parse", "--local-env-vars").split() + + self.assertEqual(sorted(set(printed) - set(git_env.GIT_LOCATION_VARS)), []) + + def test_without_git_location_drops_only_those_names(self) -> None: + env = {"GIT_DIR": "x", "GIT_INDEX_FILE": "y", "GIT_AUTHOR_NAME": "n", "PATH": "p"} + + self.assertEqual(git_env.without_git_location(env), {"GIT_AUTHOR_NAME": "n", "PATH": "p"}) + + def test_the_context_manager_restores_the_environment(self) -> None: + os.environ["GIT_DIR"] = "sentinel" + try: + with git_env.git_location_cleared(): + self.assertNotIn("GIT_DIR", os.environ) + self.assertEqual(os.environ["GIT_DIR"], "sentinel") + finally: + del os.environ["GIT_DIR"] + + +class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): + def test_the_selftest_leaves_the_ambient_repository_unchanged(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + scratch = Path(tmp) + _git(scratch, "init", "-q", "--initial-branch=develop", ".") + _git(scratch, "config", "user.email", "scratch@example.invalid") + _git(scratch, "config", "user.name", "scratch") + _git(scratch, "commit", "-q", "--allow-empty", "-m", "seed") + + def state() -> tuple[str, str, str]: + return ( + _git(scratch, "config", "--local", "--list"), + _git(scratch, "log", "--all", "--format=%H %s"), + _git(scratch, "status", "--porcelain"), + ) + + before = state() + env = dict(os.environ) + env["GIT_DIR"] = str(scratch / ".git") + env["GIT_WORK_TREE"] = str(scratch) + subprocess.run( + [sys.executable, str(REPO / "spec" / "audit.py"), "--selftest"], + env=env, + capture_output=True, + text=True, + encoding="utf-8", + check=False, + timeout=600, + ) + + self.assertEqual(state(), before) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index eace4722a..8c31079bd 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -34,6 +34,9 @@ sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "scripts")) import pr_review +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "spec")) +from git_env import without_git_location + REPO = Path(__file__).resolve().parent.parent RUNBOOK = REPO / ".github" / "copilot-instructions.md" GOVERNANCE = REPO / "GOVERNANCE.md" @@ -11467,9 +11470,6 @@ class TestOriginOwnerIgnoresInheritedEnvironment(unittest.TestCase): covered here. """ - # 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. @@ -11477,11 +11477,7 @@ def _clean_env(self) -> dict[str, str]: 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") - } + return {k: v for k, v in without_git_location().items() if 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. diff --git a/tests/test_pre_commit_hook.py b/tests/test_pre_commit_hook.py index d40ea16e0..8eb0574f9 100755 --- a/tests/test_pre_commit_hook.py +++ b/tests/test_pre_commit_hook.py @@ -22,6 +22,9 @@ import unittest from pathlib import Path +sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "spec")) +from git_env import without_git_location + REPO = Path(__file__).resolve().parents[1] HOOK = REPO / ".husky" / "pre-commit" GATE_ENTRY = REPO / "scripts" / "prose_lint.py" @@ -48,7 +51,11 @@ def setUp(self) -> None: uvx = stubs / "uvx" uvx.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") uvx.chmod(0o755) - self.env = {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} + self.env = { + k: v + for k, v in without_git_location().items() + if not k.startswith(("GIT_CONFIG", "GIT_AUTHOR", "GIT_COMMITTER")) + } self.env.pop("PROSE_ALLOW_COMMENTS", None) self.env["GIT_CONFIG_GLOBAL"] = str(empty) self.env["GIT_CONFIG_SYSTEM"] = str(empty) From 12886f79c44940bcac1e13eecd48786e99f0204b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 21 Oct 2026 20:20:28 -0700 Subject: [PATCH 2/4] Answer Local Review of the Git Location Helper Drop the three discovery names from the shared list, keep the pre-commit hook test on its stricter GIT_ prefix strip, and make the new test restore the environment and assert the self-test passed. Refs #3324 Co-Authored-By: Claude Sonnet 5.5 --- spec/git_env.py | 6 +----- tests/test_git_env.py | 10 +++++----- tests/test_pr_review.py | 4 ++-- tests/test_pre_commit_hook.py | 9 +-------- 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/spec/git_env.py b/spec/git_env.py index 9e1112566..7c3f3b2d4 100644 --- a/spec/git_env.py +++ b/spec/git_env.py @@ -4,8 +4,7 @@ builds a temporary repository with `git -C ` or `cwd=` and inherits them builds it inside the hook's own repository instead, and reads its answers back from there. -The list is what `git rev-parse --local-env-vars` prints, plus the three names that steer -discovery without being local to a repository. +The list is what `git rev-parse --local-env-vars` prints. """ from __future__ import annotations @@ -30,9 +29,6 @@ "GIT_PREFIX", "GIT_SHALLOW_FILE", "GIT_COMMON_DIR", - "GIT_CEILING_DIRECTORIES", - "GIT_DISCOVERY_ACROSS_FILESYSTEM", - "GIT_NAMESPACE", ) diff --git a/tests/test_git_env.py b/tests/test_git_env.py index ecfe3743e..c450ab720 100755 --- a/tests/test_git_env.py +++ b/tests/test_git_env.py @@ -9,6 +9,7 @@ import tempfile import unittest from pathlib import Path +from unittest import mock REPO = Path(__file__).resolve().parent.parent sys.path.insert(0, str(REPO / "spec")) @@ -41,13 +42,10 @@ def test_without_git_location_drops_only_those_names(self) -> None: self.assertEqual(git_env.without_git_location(env), {"GIT_AUTHOR_NAME": "n", "PATH": "p"}) def test_the_context_manager_restores_the_environment(self) -> None: - os.environ["GIT_DIR"] = "sentinel" - try: + with mock.patch.dict(os.environ, {"GIT_DIR": "sentinel"}): with git_env.git_location_cleared(): self.assertNotIn("GIT_DIR", os.environ) self.assertEqual(os.environ["GIT_DIR"], "sentinel") - finally: - del os.environ["GIT_DIR"] class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): @@ -70,7 +68,7 @@ def state() -> tuple[str, str, str]: env = dict(os.environ) env["GIT_DIR"] = str(scratch / ".git") env["GIT_WORK_TREE"] = str(scratch) - subprocess.run( + proc = subprocess.run( [sys.executable, str(REPO / "spec" / "audit.py"), "--selftest"], env=env, capture_output=True, @@ -80,6 +78,8 @@ def state() -> tuple[str, str, str]: timeout=600, ) + self.assertEqual(proc.returncode, 0, proc.stdout + proc.stderr) + self.assertIn("SELFTEST PASS", proc.stdout) self.assertEqual(state(), before) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 8c31079bd..964ec4e9f 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -11474,8 +11474,8 @@ 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. + the cases exist to rule out, so it drops at least what the probe drops, and every other + repository-local variable git names besides. """ return {k: v for k, v in without_git_location().items() if not k.startswith("GIT_CONFIG")} diff --git a/tests/test_pre_commit_hook.py b/tests/test_pre_commit_hook.py index 8eb0574f9..d40ea16e0 100755 --- a/tests/test_pre_commit_hook.py +++ b/tests/test_pre_commit_hook.py @@ -22,9 +22,6 @@ import unittest from pathlib import Path -sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "spec")) -from git_env import without_git_location - REPO = Path(__file__).resolve().parents[1] HOOK = REPO / ".husky" / "pre-commit" GATE_ENTRY = REPO / "scripts" / "prose_lint.py" @@ -51,11 +48,7 @@ def setUp(self) -> None: uvx = stubs / "uvx" uvx.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") uvx.chmod(0o755) - self.env = { - k: v - for k, v in without_git_location().items() - if not k.startswith(("GIT_CONFIG", "GIT_AUTHOR", "GIT_COMMITTER")) - } + self.env = {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} self.env.pop("PROSE_ALLOW_COMMENTS", None) self.env["GIT_CONFIG_GLOBAL"] = str(empty) self.env["GIT_CONFIG_SYSTEM"] = str(empty) From 7dde5f0de6b88232a5be277995aaefdcedc19f05 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 21 Oct 2026 20:34:54 -0700 Subject: [PATCH 3/4] Remove Location Variables Set Inside the Cleared Context Answers a review finding: the context manager restored only the names that existed on entry, so a variable set inside it outlived it. Refs #3324 Co-Authored-By: Claude Sonnet 5.5 --- spec/git_env.py | 2 ++ tests/test_git_env.py | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/spec/git_env.py b/spec/git_env.py index 7c3f3b2d4..90800fb59 100644 --- a/spec/git_env.py +++ b/spec/git_env.py @@ -45,4 +45,6 @@ def git_location_cleared() -> Iterator[None]: try: yield finally: + for name in GIT_LOCATION_VARS: + os.environ.pop(name, None) os.environ.update(cleared) diff --git a/tests/test_git_env.py b/tests/test_git_env.py index c450ab720..1feb957b1 100755 --- a/tests/test_git_env.py +++ b/tests/test_git_env.py @@ -48,6 +48,15 @@ def test_the_context_manager_restores_the_environment(self) -> None: self.assertEqual(os.environ["GIT_DIR"], "sentinel") +class TestSetInsideTheContext(unittest.TestCase): + def test_a_variable_set_inside_the_context_does_not_outlive_it(self) -> None: + with mock.patch.dict(os.environ): + os.environ.pop("GIT_INDEX_FILE", None) + with git_env.git_location_cleared(): + os.environ["GIT_INDEX_FILE"] = "inside" + self.assertNotIn("GIT_INDEX_FILE", os.environ) + + class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): def test_the_selftest_leaves_the_ambient_repository_unchanged(self) -> None: with tempfile.TemporaryDirectory() as tmp: From 435c250884e3ed86756051c91c6f9879ef37f26c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 21 Oct 2026 20:57:26 -0700 Subject: [PATCH 4/4] Skip the Audit Self-Test Case on Windows The self-test aborts there at a probe unrelated to this change, filed as #3338. Refs #3324 Co-Authored-By: Claude Sonnet 5.5 --- tests/test_git_env.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_git_env.py b/tests/test_git_env.py index 1feb957b1..de0453bf8 100755 --- a/tests/test_git_env.py +++ b/tests/test_git_env.py @@ -57,6 +57,10 @@ def test_a_variable_set_inside_the_context_does_not_outlive_it(self) -> None: self.assertNotIn("GIT_INDEX_FILE", os.environ) +@unittest.skipIf( + sys.platform == "win32", + "audit.py --selftest fails on Windows at its invalid-byte filename probe", +) class TestAuditSelftestIgnoresAmbientGitDir(unittest.TestCase): def test_the_selftest_leaves_the_ambient_repository_unchanged(self) -> None: with tempfile.TemporaryDirectory() as tmp: