fix(tests): make git-fixture isolation structural, not per-file (#855)

.pre-commit-config.yaml

@@ -22,6 +22,24 @@ repos:
 
   - repo: local
     hooks:
+      # Runs at BOTH stages, and first: while the shared config is corrupted,
+      # `git status` and `git revert` lie, so every later hook is reasoning about a
+      # tree that is not the one on disk.
+      #
+      # Deliberately the ONLY hook here without `cd "$(git rev-parse
+      # --show-toplevel)"`: `core.worktree`, one of the values this gate detects,
+      # REDIRECTS --show-toplevel, so that prologue would cd the check out of the
+      # repository whenever it had something to find. `mise run` locates mise.toml by
+      # walking the filesystem, and the script locates the shared config the same
+      # way, so neither needs git to answer correctly. See #855.
+      - id: git-config-clean
+        name: shared .git/config uncorrupted (#855)
+        entry: bash -lc 'mise run check:git-config-clean'
+        language: system
+        pass_filenames: false
+        always_run: true
+        stages: [pre-commit, pre-push]
+
       - id: gitleaks
         name: gitleaks (staged)
         entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run security:secrets:staged'

agent/tests/conftest.py

@@ -9,6 +9,13 @@
 import pytest
 
 from models import TaskConfig
+from tests.git_env import (
+    GIT_LOCATION_VARS,
+    TEST_IDENTITY_EMAIL,
+    TEST_IDENTITY_NAME,
+    fingerprint_git_config,
+    shared_git_config_path,
+)
 
 # Session-wide hang backstop. SIGALRM (pytest-timeout method="signal") fires only
 # in the MAIN thread during a test's *call* phase, so a deadlock in a WORKER
@@ -58,15 +65,93 @@ def _reap_on_hang() -> None:
 _hang_watchdog.start()
 
 
+# Layer 2 of the #855 git-config guard: DETECT. Captured at session start and
+# re-read at session finish. `None` means there is nothing to protect (no git, or
+# not inside a checkout — e.g. the built container image), which is a real
+# no-risk case rather than a failure to look.
+_SHARED_GIT_CONFIG: tuple[str, tuple[str, frozenset[str]]] | None = None
+
+
+def pytest_sessionstart(session):
+    """Fingerprint the repository-shared ``.git/config`` before any test runs (#855).
+
+    This is the backstop for the autouse fixture below, and it is deliberately
+    mechanism-INDEPENDENT: it does not care *how* the file was written, so it also
+    catches routes the fixture does not anticipate. Four previous fixes for this leak
+    were each scoped to one file and each was defeated by the next file added; a
+    whole-session before/after comparison cannot be outrun that way.
+    """
+    global _SHARED_GIT_CONFIG
+    path = shared_git_config_path()
+    if path is None:
+        return
+    fingerprint = fingerprint_git_config(path)
+    if fingerprint is None:
+        return
+    _SHARED_GIT_CONFIG = (path, fingerprint)
+
+
+def _report_shared_git_config_mutation(session) -> None:
+    """Fail the session if the shared ``.git/config`` changed during the run (#855).
+
+    Reports key NAMES only, never values: a ``.git/config`` may hold a remote URL
+    with embedded credentials, and this text goes to CI logs.
+
+    Does not repair the file. A test suite that silently rewrites ``.git/config``
+    would be the same class of surprise as the bug it is guarding against — so this
+    prints the exact remedy and leaves the decision to a human.
+    """
+    if _SHARED_GIT_CONFIG is None:
+        return
+    path, (digest_before, names_before) = _SHARED_GIT_CONFIG
+    current = fingerprint_git_config(path)
+    if current is None:
+        detail = "the file is now unreadable or gone"
+    else:
+        digest_after, names_after = current
+        if digest_after == digest_before:
+            return
+        added = sorted(names_after - names_before)
+        removed = sorted(names_before - names_after)
+        changed = sorted(names_after & names_before)
+        parts = []
+        if added:
+            parts.append(f"keys added: {', '.join(added)}")
+        if removed:
+            parts.append(f"keys removed: {', '.join(removed)}")
+        if not added and not removed:
+            parts.append(f"value(s) changed among: {', '.join(changed)}")
+        detail = "; ".join(parts)
+
+    print(
+        f"\nSHARED GIT CONFIG MUTATED — {path}\n"
+        f"  {detail}\n"
+        "  A test wrote into the repository's shared config. This is the #855 leak: a\n"
+        "  fixture shelling out to git while a GIT_DIR is inherited from the environment\n"
+        "  (which git exports to hooks in a linked worktree) escapes cwd, --local and the\n"
+        "  GIT_CONFIG_* pins alike.\n"
+        "  Fix the fixture: pass env=isolated_git_env(repo) from tests/git_env.py.\n"
+        f"  Clean up the repo:  git config --file {path} --unset-all core.worktree\n"
+        f"                      git config --file {path} --remove-section user",
+        file=sys.stderr,
+        flush=True,
+    )
+    session.exitstatus = pytest.ExitCode.TESTS_FAILED
+
+
 def pytest_sessionfinish(session, exitstatus):
-    """Cancel the hang watchdog on a clean session finish.
+    """Cancel the hang watchdog on a clean session finish, then run the #855 check.
 
-    Without this, a legitimately slow-but-passing suite that finishes just after
+    Without the cancel, a legitimately slow-but-passing suite that finishes just after
     the 600s deadline (e.g. during teardown / coverage write) would be hard-exited
     by ``_reap_on_hang`` and turn green red with a thread-dump uncorrelated to any
     failed test. ``Timer.cancel()`` is a no-op if the timer already fired (a true
-    hang), so this only prevents the false-positive kill."""
+    hang), so this only prevents the false-positive kill.
+
+    The config check runs here rather than as a test because no test can observe a
+    mutation made by a test that runs after it."""
     _hang_watchdog.cancel()
+    _report_shared_git_config_mutation(session)
 
 
 class FakeRunCmd:
@@ -163,6 +248,46 @@ def make_task_config(**overrides) -> TaskConfig:
 ]
 
 
+@pytest.fixture(autouse=True)
+def _isolate_git_location(monkeypatch, tmp_path):
+    """Layer 1 of the #855 guard: PREVENT. Applies to every test, unconditionally.
+
+    Placement is the whole point. #720/#731 got the *content* of this right but put it
+    in a per-class fixture inside ``test_post_hooks.py``, so #665 was free to add a
+    fresh unguarded ``_git()`` helper in ``test_registry_loader.py`` seven days later
+    and reopen the leak. An autouse fixture in ``conftest.py`` is the only placement
+    that also covers test files nobody has written yet.
+
+    Two distinct jobs:
+
+    1. **Strip the repo-LOCATION vars.** While any of them is set, ``git -C <tmp>``,
+       ``cwd=``, ``--local`` and the ``GIT_CONFIG_*`` pins are all bypassed, because
+       an explicit ``GIT_DIR`` overrides repository discovery outright. Git exports
+       these to hooks in a linked worktree, which is exactly how this suite runs as a
+       pre-push gate from ``.worktrees/``.
+
+    2. **Pin config resolution and identity.** So that a fixture which shells out to
+       git *without* using ``isolated_git_env`` still cannot reach the developer's
+       ``~/.gitconfig``, and any commit it makes is attributed to the reserved test
+       identity rather than to whoever happens to be running the suite.
+
+    Production code is a beneficiary too, not just fixtures: ``post_hooks`` and
+    ``repo`` shell out to git with the ambient environment, so an inherited ``GIT_DIR``
+    would point the code under test at the real repository and the assertions would
+    silently describe the wrong one.
+    """
+    for var in GIT_LOCATION_VARS:
+        monkeypatch.delenv(var, raising=False)
+
+    monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / ".gitconfig-test"))
+    monkeypatch.setenv("GIT_CONFIG_SYSTEM", os.devnull)
+    monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1")
+    monkeypatch.setenv("GIT_AUTHOR_NAME", TEST_IDENTITY_NAME)
+    monkeypatch.setenv("GIT_AUTHOR_EMAIL", TEST_IDENTITY_EMAIL)
+    monkeypatch.setenv("GIT_COMMITTER_NAME", TEST_IDENTITY_NAME)
+    monkeypatch.setenv("GIT_COMMITTER_EMAIL", TEST_IDENTITY_EMAIL)
+
+
 @pytest.fixture(autouse=True)
 def _clean_env(monkeypatch):
     """Remove agent-related env vars and reset the AWS session cache each test.

agent/tests/git_env.py

@@ -0,0 +1,152 @@
+"""Single source of truth for isolating test git invocations (#855).
+
+Four earlier fixes for the same leak (#622/#623, #695, #720/#731, #665) were each
+placed in the file where the leak was observed, so none of them could protect the
+next test file to shell out to git — #665 added a fresh unguarded helper seven days
+after #731 hardened a different file. This module exists so there is exactly one
+definition to import, and ``tests/conftest.py`` applies it to every test whether or
+not the test author knew to ask.
+
+The mechanism, because it is not obvious from any single call site:
+
+An explicit ``GIT_DIR`` overrides repository **discovery** outright. That beats
+``git -C <path>``, ``cwd=``, ``HOME=``, ``--local``, and the ``GIT_CONFIG_*`` pins
+*simultaneously* — ``--local`` in particular resolves relative to ``GIT_DIR``, so it
+is no defence. Git exports ``GIT_DIR``/``GIT_COMMON_DIR`` to hooks **only in a linked
+worktree** (they are unset in a normal checkout), which is exactly how this suite runs
+as a pre-push gate from ``.worktrees/``. Under that environment
+``git -C <tmp> config user.email t@t`` writes into the *real* shared ``.git/config``
+and ``git -C <tmp> init`` re-inits the *real* repository instead of creating one in
+``<tmp>``.
+
+That is why the bug reads as unreproducible: run the same tests by hand from the main
+checkout and nothing leaks.
+"""
+
+from __future__ import annotations
+
+import hashlib
+import os
+import subprocess
+
+# Repo-LOCATION vars, as distinct from config-CONTENT vars. Stripping these is
+# load-bearing, not tidiness: while any one of them is set, every other containment
+# measure below is bypassed. Keep this tuple as the only copy in the tree.
+GIT_LOCATION_VARS: tuple[str, ...] = (
+    "GIT_DIR",
+    "GIT_COMMON_DIR",
+    "GIT_WORK_TREE",
+    "GIT_INDEX_FILE",
+    "GIT_OBJECT_DIRECTORY",
+    "GIT_ALTERNATE_OBJECT_DIRECTORIES",
+    "GIT_PREFIX",
+    "GIT_CEILING_DIRECTORIES",
+)
+
+# RFC-2606 reserved TLD: unroutable by construction, and recognisable in a stray
+# commit. #720 was filed because the literal `t <t@t>` from a fixture was transcribed
+# into a real repo's config and then into real commits.
+TEST_IDENTITY_NAME = "ABCA Test"
+TEST_IDENTITY_EMAIL = "abca-test@example.invalid"
+
+# `git config` timeout. Bounded so a wedged git cannot stall the session-level
+# fingerprint and burn the suite's wall-clock budget.
+_GIT_TIMEOUT_S = 30
+
+
+def isolated_git_env(repo, base: dict[str, str] | None = None) -> dict[str, str]:
+    """Return an environment in which git cannot reach outside *repo*.
+
+    Order matters. The location vars are removed **first**, because the pins added
+    afterwards are all ineffective while a ``GIT_DIR`` is still present.
+
+    *repo* doubles as ``HOME``, so a fixture that transcribes a bare
+    ``git config user.email ...`` (no ``--local``) lands in a throwaway file rather
+    than the developer's ``~/.gitconfig``.
+    """
+    env = {k: v for k, v in (base or os.environ).items() if k not in GIT_LOCATION_VARS}
+    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",
+            # Identity via env, not config: these outrank every config file, so a
+            # commit is correctly attributed even if a config write is missed.
+            "GIT_AUTHOR_NAME": TEST_IDENTITY_NAME,
+            "GIT_AUTHOR_EMAIL": TEST_IDENTITY_EMAIL,
+            "GIT_COMMITTER_NAME": TEST_IDENTITY_NAME,
+            "GIT_COMMITTER_EMAIL": TEST_IDENTITY_EMAIL,
+        }
+    )
+    return env
+
+
+def shared_git_config_path() -> str | None:
+    """Absolute path of the repository-shared ``.git/config``, or None if unavailable.
+
+    Resolved via ``--git-common-dir`` rather than ``--show-toplevel`` **on purpose**.
+    ``core.worktree`` — one of the values this leak writes — changes what
+    ``--show-toplevel`` returns, so an already-polluted repo would make this function
+    compute a path that does not exist and report "nothing to protect": the pollution
+    would disable its own detector. ``--git-common-dir`` is answered from the gitdir
+    alone and also resolves to the *shared* ``.git`` when called from a linked
+    worktree, which is the file actually at risk. Requires git >= 2.31 for
+    ``--path-format``.
+
+    Returns None when there is no repository to protect (no git on PATH, or running
+    outside a checkout — e.g. inside the built container image). That is a genuine
+    "no risk" case, not a failure to look.
+    """
+    try:
+        result = subprocess.run(
+            ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"],
+            capture_output=True,
+            text=True,
+            check=False,
+            timeout=_GIT_TIMEOUT_S,
+        )
+    except (OSError, subprocess.SubprocessError):
+        return None
+    if result.returncode != 0:
+        return None
+    common_dir = result.stdout.strip()
+    if not common_dir:
+        return None
+    config = os.path.join(common_dir, "config")
+    return config if os.path.isfile(config) else None
+
+
+def fingerprint_git_config(path: str) -> tuple[str, frozenset[str]] | None:
+    """Digest *path* plus its key names, or None if it cannot be read.
+
+    Deliberately returns key **names** and not values. A ``.git/config`` can legally
+    hold a remote URL with embedded credentials, so a change report built from this
+    can name what moved without printing anything secret.
+    """
+    try:
+        with open(path, "rb") as handle:
+            raw = handle.read()
+    except OSError:
+        return None
+    digest = hashlib.sha256(raw).hexdigest()
+    names = frozenset(_config_key_names(path))
+    return digest, names
+
+
+def _config_key_names(path: str) -> list[str]:
+    """Config key names in *path*, via git itself so the parse matches git's."""
+    try:
+        result = subprocess.run(
+            ["git", "config", "--file", path, "--list", "--name-only"],
+            capture_output=True,
+            text=True,
+            check=False,
+            timeout=_GIT_TIMEOUT_S,
+        )
+    except (OSError, subprocess.SubprocessError):
+        return []
+    if result.returncode != 0:
+        return []
+    return [line for line in result.stdout.splitlines() if line]