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..90800fb59 --- /dev/null +++ b/spec/git_env.py @@ -0,0 +1,50 @@ +"""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. +""" + +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", +) + + +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: + 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 new file mode 100755 index 000000000..de0453bf8 --- /dev/null +++ b/tests/test_git_env.py @@ -0,0 +1,100 @@ +#!/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 +from unittest import mock + +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: + 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") + + +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) + + +@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: + 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) + proc = 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(proc.returncode, 0, proc.stdout + proc.stderr) + self.assertIn("SELFTEST PASS", proc.stdout) + 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..964ec4e9f 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,21 +11470,14 @@ 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. 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 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.