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

.pre-commit-config.yaml

@@ -22,6 +22,45 @@ 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.
+      #
+      # The ONLY hook here without a `cd "$(git rev-parse --show-toplevel)"` prologue,
+      # but do NOT read that as the protection — measured on prek 0.4.8, prek chdirs a
+      # hook to the repo root itself, derived from its own `git rev-parse
+      # --show-toplevel`, so this hook starts exactly where the prologue would have put
+      # it either way. It is omitted because it would be dead code, not because it is
+      # dangerous. The protection is inside the script: it locates the shared config by
+      # walking the filesystem for `.git` and reads it via `git config --file` from cwd
+      # `/`, so it never asks git a question `core.worktree` can redirect. `mise run`
+      # likewise finds mise.toml by walking the filesystem.
+      #
+      # Known reach, so nobody over-trusts this line: when `core.worktree` names a path
+      # with 2+ missing components, prek itself aborts on that rc-128 rev-parse before
+      # any hook runs — but plain `git commit` fails the same way, so that shape is
+      # self-announcing rather than silent. The shape this gate actually catches is
+      # `core.worktree` pointing at a directory that EXISTS (a sibling worktree), where
+      # git answers normally and nothing else complains. See #855.
+      #
+      # `bash -c`, NOT `-lc` like the hooks below, and that is not a style choice. A
+      # login shell sources the user's profile BEFORE running the command, so a `cd` in
+      # a profile relocates cwd — and unlike every other hook here, this one has no
+      # `cd "$(git rev-parse --show-toplevel)"` prologue to put it back, so both the
+      # `mise.toml` lookup and the script's `.git` walk would start from the wrong
+      # place. The hooks below are immune because they re-cd; this one is immune only by
+      # not sourcing the profile. Tradeoff accepted knowingly: `-l` is what puts `mise`
+      # on PATH for a git client that does not source the profile itself, so if this
+      # hook ever reports `mise: command not found` from a GUI client, the fix is an
+      # absolute path to mise — not restoring `-l`.
+      - id: git-config-clean
+        name: shared .git/config uncorrupted (#855)
+        entry: bash -c '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'

CONTRIBUTING.md

@@ -93,6 +93,7 @@ PRs labeled `auto-approve` are approved automatically by the `auto-approve` work
 
 `mise run install` automatically installs [prek](https://github.com/j178/prek) git hooks. These run on every commit and push:
 
+- **both stages** - `mise run check:git-config-clean` runs first, before anything else, and fails if your repository's shared `.git/config` carries the test-fixture leak signature (`core.worktree`, `core.bare` on a checkout, or a fixture identity in `[user]`). It runs first because while that config is corrupted `git status` and `git revert` describe a *different* directory, so every later hook — and every judgement you make about your own working tree — is about a tree that is not the one on disk. See [#855](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/issues/855); if it fires, run the remedy it prints rather than bypassing it.
 - **pre-commit** - Whitespace/EOF checks, gitleaks on staged changes, linters (ESLint, Ruff, astro check) for touched files.
 - **pre-push** - Security scans (`mise run hooks:pre-push:security`) and tests across all packages (`mise run hooks:pre-push:tests`).
 

agent/tests/conftest.py

@@ -9,6 +9,16 @@
 import pytest
 
 from models import TaskConfig
+from tests.git_env import (
+    GIT_LOCATION_VARS,
+    TEST_IDENTITY_EMAIL,
+    TEST_IDENTITY_NAME,
+    GitConfigFingerprint,
+    GitConfigLookupError,
+    fingerprint_git_config,
+    shared_git_config_path,
+    signature_keys_changed,
+)
 
 # 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 +68,149 @@ 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` at or
+# above cwd — e.g. the built container image), which is a real no-risk case rather than
+# a failure to look.
+_SHARED_GIT_CONFIG: tuple[str, GitConfigFingerprint] | None = None
+
+# Why the fingerprint could not be taken, when a repository WAS found. Distinct from
+# `_SHARED_GIT_CONFIG is None`, and the distinction is load-bearing: "nothing to
+# protect" is a pass, "could not look at the thing I am protecting" is a failure. The
+# first version of this file collapsed the two into a silent `return`, so a config too
+# broken for the resolver to describe — the exact state the guard is for — switched the
+# guard off and reported nothing.
+_SHARED_GIT_CONFIG_UNCHECKED: 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, _SHARED_GIT_CONFIG_UNCHECKED
+    try:
+        path = shared_git_config_path()
+    except GitConfigLookupError as exc:
+        _SHARED_GIT_CONFIG_UNCHECKED = str(exc)
+        return
+    if path is None:
+        return
+    fingerprint = fingerprint_git_config(path)
+    if fingerprint is None:
+        _SHARED_GIT_CONFIG_UNCHECKED = f"{path} was located but could not be read or parsed by git"
+        return
+    _SHARED_GIT_CONFIG = (path, fingerprint)
+
+
+def _describe_key_drift(before: frozenset[str], after: frozenset[str]) -> str:
+    """Which key names moved between two fingerprints. Names only, never values."""
+    added = sorted(after - before)
+    removed = sorted(before - after)
+    parts = []
+    if added:
+        parts.append(f"keys added: {', '.join(added)}")
+    if removed:
+        parts.append(f"keys removed: {', '.join(removed)}")
+    if not parts:
+        parts.append(f"value(s) changed among: {', '.join(sorted(after & before))}")
+    return "; ".join(parts)
+
+
+def _report_shared_git_config_mutation(session) -> None:
+    """Fail the session if the shared ``.git/config`` was corrupted during the run (#855).
+
+    Three outcomes, and the middle one is why this is not a single digest comparison:
+
+    * a **signature** key moved (``core.worktree``, ``core.bare``, ``user.*``) — the leak.
+      Fails the session with a remedy.
+    * some **other** key moved — reported as a note and nothing more. The shared config is
+      written by ordinary work too (``git fetch`` rewriting ``remote.*``, ``push -u``
+      adding ``branch.<name>.remote``), and this suite runs as a pre-push hook while other
+      worktrees may be active. A red naming no fixture and offering no remedy teaches
+      people to re-run past the gate.
+    * the config could not be fingerprinted at all — also a failure, see below.
+
+    Reports key NAMES only, never values: a ``.git/config`` may hold a remote URL with
+    embedded credentials, and this text goes to CI logs. ``mise run
+    check:git-config-clean`` prints the offending values, which are safe for the signature
+    keys specifically.
+
+    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_UNCHECKED is not None:
+        print(
+            "\nSHARED GIT CONFIG — COULD NOT CHECK\n"
+            f"  {_SHARED_GIT_CONFIG_UNCHECKED}\n"
+            "  A repository was found but the #855 guard could not fingerprint its shared\n"
+            "  config, so this run proves nothing about whether a fixture leaked into it.\n"
+            "  That is itself the signature of a broken repo: `core.worktree` naming a\n"
+            "  path that no longer exists makes every `git rev-parse` in the tree abort.\n"
+            "  Diagnose with:  mise run check:git-config-clean",
+            file=sys.stderr,
+            flush=True,
+        )
+        session.exitstatus = pytest.ExitCode.TESTS_FAILED
+        return
+
+    if _SHARED_GIT_CONFIG is None:
+        return
+    path, before = _SHARED_GIT_CONFIG
+    after = fingerprint_git_config(path)
+
+    if after is None:
+        detail = "the file is now unreadable, gone, or no longer parses"
+    else:
+        if after.digest == before.digest:
+            return
+        drift = _describe_key_drift(before.names, after.names)
+        moved = signature_keys_changed(before, after)
+        if not moved:
+            # Real, but not the leak. Say so and leave the session's verdict alone.
+            print(
+                f"\nnote: {path} changed during this run, but no #855 signature key did\n"
+                f"  ({drift}) — routine git/editor activity looks like this. Not failing.",
+                file=sys.stderr,
+                flush=True,
+            )
+            return
+        detail = f"signature key(s) changed: {', '.join(moved)} — {drift}"
+
+    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 +307,72 @@ 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.
+
+    Three distinct jobs, and the third was added late because the first two do not
+    cover the route they appear to:
+
+    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
+       ``GIT_DIR`` to a hook in a linked worktree — which is exactly how this suite runs
+       as a pre-push gate from ``.worktrees/`` — and that one var is enough. The other
+       six are stripped for what they *do*, not because git sets them; see the measured
+       hook-environment table in ``tests/git_env.py``, which corrects an earlier claim
+       here that git exported the whole set.
+
+    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.
+
+    3. **Move the process out of the checkout, and cap discovery.** Jobs 1 and 2 close
+       the ``GIT_DIR`` route; neither touches repository discovery from the inherited
+       cwd, and pytest runs from ``agent/`` — *inside* the checkout. So with exactly
+       the environment jobs 1 and 2 produce, a plain
+       ``subprocess.run(["git", "config", "user.email", "t@t"])`` with no ``cwd=`` and
+       no ``-C`` still walks up from ``agent/`` and writes the shared config: same
+       leak, different route, and reached by precisely the author this fixture is
+       advertised to protect — the one who forgot ``isolated_git_env``. Standing in
+       ``tmp_path`` instead makes that command fail loudly (``fatal: not in a git
+       directory``) rather than succeed somewhere it should not.
+
+       ``GIT_CEILING_DIRECTORIES`` is re-set for the same reason, and note that job 1
+       *deletes* it, which widens discovery rather than narrowing it. Pinned to
+       ``tmp_path.parent`` — not ``tmp_path`` — so a test's own repository under
+       ``tmp_path`` is still discoverable while the walk can never climb out of the
+       pytest temp tree, whatever ``TMPDIR`` points at on this machine.
+
+    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.chdir(tmp_path)
+    # realpath because git resolves ceiling entries through symlinks and so does
+    # ``git_env._ceiling_directories``; a logical spelling would match neither on a host
+    # where TMPDIR or $HOME is a symlink.
+    monkeypatch.setenv("GIT_CEILING_DIRECTORIES", os.path.realpath(tmp_path.parent))
+    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.