From 905e1b1aff544395a059ed1191d33aacbed38460 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 6 Aug 2026 00:45:34 +0000 Subject: [PATCH 1/2] test(agent): hard-isolate git fixtures from the developer's real identity (#720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `t ` has now clobbered a real .git/config three times. The fixture looked safe — every call passes cwd=repo after a `git init` — but cwd is NOT containment. Two ways it escapes: 1. A bare `git config` (no --local) walks UP to the nearest enclosing repo when the target dir is not itself a repo root. 2. `git init` at a LINKED WORKTREE root does not create a nested repo — it re-initializes the SHARED .git, so the following `git config` writes into the real config. Reproduced end-to-end; this is the path that bit us, since all development here happens in .worktrees/. Because the leaked value is a valid identity, git accepts it silently and every later commit is authored `t ` until someone notices. Fixes, defence in depth: - Pin HOME / XDG_CONFIG_HOME / GIT_CONFIG_GLOBAL / GIT_CONFIG_SYSTEM into the tmp repo and set GIT_CONFIG_NOSYSTEM, so git cannot resolve a config file outside tmp even if a future edit drops --local again. - Set GIT_AUTHOR_*/GIT_COMMITTER_* so commits need no config at all. - Use --local on the config writes. - Use an RFC-2606 reserved identity (abca-test@example.invalid) so a transcribed value is self-evidently a fixture and harmless if it ever does leak. Verified: 18/18 pass, and with a sentinel identity pinned in the real .git/config the full suite leaves it byte-identical. Co-Authored-By: Claude --- agent/tests/test_post_hooks.py | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/agent/tests/test_post_hooks.py b/agent/tests/test_post_hooks.py index b52b755e2..f8484b34a 100644 --- a/agent/tests/test_post_hooks.py +++ b/agent/tests/test_post_hooks.py @@ -6,6 +6,7 @@ ``shell.run_cmd`` (mutating git/gh commands) — both faked with recorders. """ +import os import subprocess from types import SimpleNamespace @@ -278,14 +279,36 @@ class TestReconcileAgentBranch: @staticmethod def _git(repo, *args): - subprocess.run(["git", *args], cwd=repo, check=True, capture_output=True, text=True) + # Hard-isolate from the developer's real git identity (#720). `cwd` alone + # is NOT containment: a bare `git config` walks up to the nearest + # enclosing repo, and `git init` at a linked-worktree root re-inits the + # SHARED .git rather than creating a nested one — so both can write + # straight into the real .git/config. Pinning the HOME/config env vars + # means even a transcribed `git config user.email` cannot escape tmp. + env = { + **os.environ, + "HOME": str(repo), + "XDG_CONFIG_HOME": str(repo), + "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "ABCA Test", + "GIT_AUTHOR_EMAIL": "abca-test@example.invalid", + "GIT_COMMITTER_NAME": "ABCA Test", + "GIT_COMMITTER_EMAIL": "abca-test@example.invalid", + } + subprocess.run( + ["git", *args], cwd=repo, check=True, capture_output=True, text=True, env=env + ) def _make_repo(self, tmp_path): repo = tmp_path / "repo" repo.mkdir() self._git(repo, "init", "-q") - self._git(repo, "config", "user.email", "t@t") - self._git(repo, "config", "user.name", "t") + # RFC-2606 reserved domain, and --local so the write cannot escape this + # repo even if the enclosing-repo fallback above is ever reintroduced. + self._git(repo, "config", "--local", "user.email", "abca-test@example.invalid") + self._git(repo, "config", "--local", "user.name", "ABCA Test") (repo / "f.txt").write_text("base\n") self._git(repo, "add", "-A") self._git(repo, "commit", "-qm", "base") From 0b6ab1711ef0b921d0ba81ac201164d616bf7390 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 6 Aug 2026 01:29:26 +0000 Subject: [PATCH 2/2] test(agent): strip inherited GIT_DIR so fixture isolation actually holds (#720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses @theagenticguy's review on #731. Reproduced the gap exactly as described: `**os.environ` passed `GIT_DIR` through, and an explicit GIT_DIR overrides repository discovery outright — so it defeats cwd, HOME and the GIT_CONFIG_* pins together, and `--local` is no defence because it resolves relative to GIT_DIR. Git exports GIT_DIR to hooks in a LINKED WORKTREE (verified: a hook in a worktree sees `/.git/worktrees/`; unset in a normal repo), which is precisely how this suite runs as a pre-push gate from .worktrees/. Confirmed the damage before fixing: with the reviewed env dict plus an inherited GIT_DIR, the outer repo's config gained BOTH the fixture identity AND `bare = true` — strictly worse than the original #720 incident. Fixes: - Build the env by FILTERING the repo-location vars out of os.environ (GIT_DIR, GIT_COMMON_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_PREFIX, GIT_CEILING_DIRECTORIES) before overlaying the pins, per the suggested shape. - Add an autouse fixture clearing those vars for the whole class. The env dict alone was NOT enough: `post_hooks` itself shells out to git with the ambient environment (e.g. `_current_branch`), so an inherited GIT_DIR pointed PRODUCTION code at the real repo — `test_current_branch_reports_none_when_ detached` then asserted against the wrong repository. Found by running the sentinel check under GIT_DIR, which is why that extension was worth adding. - Route the read-only helpers (`_head_sha`, `_sha_of`) through the same isolated env, so they cannot read the real repo's HEAD. - Add a regression test that sets GIT_DIR/GIT_WORK_TREE at a stand-in repo and asserts its config is untouched. Verified: 19/19 pass, and the full suite run with GIT_DIR + GIT_WORK_TREE exported at the real repo leaves `.git/config` BYTE-IDENTICAL (previously it clobbered the sentinel). Mutation-tested: restoring `**os.environ` fails the new guard. agent:quality clean, 1460 tests pass. Co-Authored-By: Claude --- agent/tests/test_post_hooks.py | 138 +++++++++++++++++++++++++++++---- 1 file changed, 124 insertions(+), 14 deletions(-) diff --git a/agent/tests/test_post_hooks.py b/agent/tests/test_post_hooks.py index f8484b34a..3768aec1b 100644 --- a/agent/tests/test_post_hooks.py +++ b/agent/tests/test_post_hooks.py @@ -10,6 +10,8 @@ import subprocess from types import SimpleNamespace +import pytest + import post_hooks from models import RepoSetup from tests.conftest import FakeRunCmd, make_task_config @@ -277,28 +279,90 @@ class TestReconcileAgentBranch: higher confidence than faking subprocess. The two seams (subprocess.run for the branch read, run_cmd for the mutating ops) both hit the tmp repo.""" + # Repo-LOCATION vars. An explicit GIT_DIR overrides repository discovery + # outright, so it beats cwd, HOME, the GIT_CONFIG_* pins and `--local` + # alike. Git exports these to hooks in a LINKED WORKTREE (unset in a normal + # repo), which is exactly how this suite runs as a pre-push gate from + # .worktrees/. + _GIT_LOCATION_VARS = ( + "GIT_DIR", + "GIT_COMMON_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_PREFIX", + "GIT_CEILING_DIRECTORIES", + ) + + @pytest.fixture(autouse=True) + def _clear_ambient_git_location(self, monkeypatch): + """Strip repo-location vars for the whole class (#720). + + Not just for the fixture helpers: ``post_hooks`` itself shells out to + git with the ambient environment (e.g. ``_current_branch``), so an + inherited GIT_DIR would point PRODUCTION code at the real repo instead + of the tmp one — the assertions would silently describe the wrong + repository. + """ + for var in self._GIT_LOCATION_VARS: + monkeypatch.delenv(var, raising=False) + @staticmethod - def _git(repo, *args): + def _isolated_env(repo): # Hard-isolate from the developer's real git identity (#720). `cwd` alone # is NOT containment: a bare `git config` walks up to the nearest # enclosing repo, and `git init` at a linked-worktree root re-inits the # SHARED .git rather than creating a nested one — so both can write # straight into the real .git/config. Pinning the HOME/config env vars # means even a transcribed `git config user.email` cannot escape tmp. + # + # Dropping the repo-LOCATION vars first is load-bearing, not tidiness. + # An explicit GIT_DIR overrides repository discovery outright, so it + # defeats cwd, HOME and the GIT_CONFIG_* pins together — and `--local` + # resolves relative to it, so that is no defence either. Git exports + # GIT_DIR to hooks in a LINKED WORKTREE (it is unset in a normal repo), + # which is exactly how this suite runs as a pre-push gate from + # .worktrees/: inheriting it re-opens #720 and additionally stamps + # `bare = true` on the real repo. env = { - **os.environ, - "HOME": str(repo), - "XDG_CONFIG_HOME": str(repo), - "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), - "GIT_CONFIG_SYSTEM": os.devnull, - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_AUTHOR_NAME": "ABCA Test", - "GIT_AUTHOR_EMAIL": "abca-test@example.invalid", - "GIT_COMMITTER_NAME": "ABCA Test", - "GIT_COMMITTER_EMAIL": "abca-test@example.invalid", + k: v + for k, v in os.environ.items() + if k + not in { + "GIT_DIR", + "GIT_COMMON_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_PREFIX", + "GIT_CEILING_DIRECTORIES", + } } + env.update( + { + "HOME": str(repo), + "XDG_CONFIG_HOME": str(repo), + "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "ABCA Test", + "GIT_AUTHOR_EMAIL": "abca-test@example.invalid", + "GIT_COMMITTER_NAME": "ABCA Test", + "GIT_COMMITTER_EMAIL": "abca-test@example.invalid", + } + ) + return env + + def _git(self, repo, *args): subprocess.run( - ["git", *args], cwd=repo, check=True, capture_output=True, text=True, env=env + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + env=self._isolated_env(repo), ) def _make_repo(self, tmp_path): @@ -316,14 +380,60 @@ def _make_repo(self, tmp_path): self._git(repo, "branch", "-M", "main") return str(repo) + def test_fixture_cannot_touch_an_outer_repo_even_with_git_dir_set(self, tmp_path, monkeypatch): + """Regression guard for #720: the fixture must stay contained when a + repo-location var is present in the ambient environment. + + Git exports GIT_DIR to hooks in a linked worktree, which is how this + suite runs as a pre-push gate. An inherited GIT_DIR overrides repository + discovery entirely, so it beats cwd/HOME/GIT_CONFIG_* *and* `--local` — + the fixture would re-init and rewrite the real shared repo. Fails if + _git ever stops stripping those vars.""" + outer = tmp_path / "outer" + outer.mkdir() + # Build the stand-in "real" repo with the location vars still cleared by + # the autouse fixture, so this setup lands in tmp and not the actual repo. + subprocess.run(["git", "init", "-q"], cwd=outer, check=True, capture_output=True) + sentinel_config = outer / ".git" / "config" + subprocess.run( + ["git", "config", "--local", "user.email", "sentinel@example.invalid"], + cwd=outer, + check=True, + capture_output=True, + ) + before = sentinel_config.read_text() + + # Exactly what a pre-push hook in a linked worktree hands us. + monkeypatch.setenv("GIT_DIR", str(outer / ".git")) + monkeypatch.setenv("GIT_WORK_TREE", str(outer)) + + self._make_repo(tmp_path) + + assert sentinel_config.read_text() == before, ( + "fixture escaped into the outer repo — _git must strip GIT_DIR/" + "GIT_WORK_TREE before overlaying its pins (#720)" + ) + def _head_sha(self, repo): + # Same isolated env as _git: an inherited GIT_DIR would make this read + # the REAL repo's HEAD instead of the fixture's. return subprocess.run( - ["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True + ["git", "rev-parse", "HEAD"], + cwd=repo, + check=True, + capture_output=True, + text=True, + env=self._isolated_env(repo), ).stdout.strip() def _sha_of(self, repo, ref): return subprocess.run( - ["git", "rev-parse", ref], cwd=repo, check=True, capture_output=True, text=True + ["git", "rev-parse", ref], + cwd=repo, + check=True, + capture_output=True, + text=True, + env=self._isolated_env(repo), ).stdout.strip() def test_reconciles_when_agent_on_own_branch(self, tmp_path):