From 8a2f1db45ad1776b2d82381da73be83283c706b8 Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:43:34 +0000 Subject: [PATCH 1/3] fix(tests): make git-fixture isolation structural, not per-file (#855) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth encounter with one leak (#622/#623, #695, #720/#731, #665): an agent test shells out to git, git resolves the repository from an inherited GIT_DIR rather than the cwd it was handed, and the write lands in the real shared .git/config — core.worktree, core.bare, and a `t ` identity replacing the developer's own. Downstream, `git status` reports the root dirty while hiding its untracked files, and `git revert` silently no-ops. Why it reads as unreproducible: git exports GIT_DIR/GIT_COMMON_DIR to hooks ONLY in a linked worktree. Under that env `git -C init` re-inits the real repository and `git -C config user.email t@t` writes the real shared config; run the same tests by hand from the main checkout and nothing leaks. GIT_DIR overrides repository discovery, so it defeats -C, cwd, HOME, --local and the GIT_CONFIG_* pins simultaneously. `check=False` is why it stayed silent: `git init` against an initialised repo exits 0. Each earlier fix hardened the single file where the leak was observed, so none could protect the next file to shell out to git — #665 introduced a fresh unguarded helper seven days after #731 hardened a different one. Three layers instead of a fifth patch. Layer 1 PREVENT — agent/tests/git_env.py becomes the only definition of the location-var tuple and the env builder; conftest's `_isolate_git_location` autouse fixture applies it to every test whether or not the author knew to ask. test_post_hooks.py's private copies are deleted (both files now import the shared one), and test_registry_loader._init_repo no longer writes `git config user.*` at all: identity arrives via GIT_AUTHOR_*/GIT_COMMITTER_*, which outrank every config file. Layer 2 DETECT — pytest_sessionstart fingerprints the shared config (sha256 + key NAMES, never values: a remote URL may embed credentials) and pytest_sessionfinish fails the session if it moved. Mechanism-independent, so it also catches routes Layer 1 does not anticipate. Layer 3 REFUSE — scripts/check-git-config-clean.mjs, `mise run check:git-config-clean`, wired into pre-commit and pre-push. It resolves the config path WITHOUT git, because no `git rev-parse` form survives the state it must report: core.worktree redirects --show-toplevel (the corruption disabling its own alarm), and when it names a deleted pytest tmp_path even --git-common-dir aborts with `fatal: Invalid path`. Exit 0 clean / 1 corrupt / 2 could-not-check — an unreadable config is precisely where a leak hides. Rules match the leak's signature rather than merely unusual settings: a real per-repo identity and a users.noreply.github.com address are deliberately not flagged, since a gate that fired on legitimate configuration would be switched off rather than fixed. Tests: 20 in cdk/test/scripts/check-git-config-clean.test.ts (every rule asserted by making it fire, including core.worktree at a path that no longer exists — the shape that broke two earlier designs) and 14 in agent/tests/test_git_fixture_isolation.py, including a differential witness that an inherited GIT_DIR escapes while isolated_git_env contains. agent 1789 passed / cdk 4366 passed / cli 791 passed; //cdk:synth:quiet fails pre-existing on ec2:DescribeAvailabilityZones (IAM, unrelated). Refs #855, #622, #623, #695, #720, #731, #665 Co-Authored-By: Claude Opus 5 --- .pre-commit-config.yaml | 18 + agent/tests/conftest.py | 131 +++++- agent/tests/git_env.py | 152 +++++++ agent/tests/test_git_fixture_isolation.py | 312 ++++++++++++++ agent/tests/test_post_hooks.py | 111 ++--- agent/tests/test_registry_loader.py | 15 +- .../scripts/check-git-config-clean.test.ts | 393 ++++++++++++++++++ mise.toml | 4 + scripts/check-git-config-clean.mjs | 318 ++++++++++++++ 9 files changed, 1365 insertions(+), 89 deletions(-) create mode 100644 agent/tests/git_env.py create mode 100644 agent/tests/test_git_fixture_isolation.py create mode 100644 cdk/test/scripts/check-git-config-clean.test.ts create mode 100644 scripts/check-git-config-clean.mjs diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0b2f7a7ca..8a6974bdf 100644 --- a/.pre-commit-config.yaml +++ b/.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' diff --git a/agent/tests/conftest.py b/agent/tests/conftest.py index 0ea21a8bb..dea0770ba 100644 --- a/agent/tests/conftest.py +++ b/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 ``, + ``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. diff --git a/agent/tests/git_env.py b/agent/tests/git_env.py new file mode 100644 index 000000000..530165757 --- /dev/null +++ b/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 ``, ``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 config user.email t@t`` writes into the *real* shared ``.git/config`` +and ``git -C init`` re-inits the *real* repository instead of creating one in +````. + +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 ` 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] diff --git a/agent/tests/test_git_fixture_isolation.py b/agent/tests/test_git_fixture_isolation.py new file mode 100644 index 000000000..a9e49fafa --- /dev/null +++ b/agent/tests/test_git_fixture_isolation.py @@ -0,0 +1,312 @@ +"""Tests for the git-fixture isolation guard (#855). + +The point of this file is that the guard is *proven live* rather than assumed. The +central test is differential: the **same** git command is run twice, once with an +inherited ``GIT_DIR`` and once through ``isolated_git_env``, and it is asserted to +escape in the first case and be contained in the second. A test that only checked the +contained case would still pass if ``isolated_git_env`` were quietly reduced to +``dict(os.environ)``. + +Every repository these tests touch is built inside ``tmp_path``. Nothing here writes +to the real repository — the "leak" half of the differential test leaks into a +purpose-built fake shared repo. +""" + +from __future__ import annotations + +import os +import subprocess +from types import SimpleNamespace + +import pytest + +from tests.git_env import ( + GIT_LOCATION_VARS, + TEST_IDENTITY_EMAIL, + TEST_IDENTITY_NAME, + fingerprint_git_config, + isolated_git_env, + shared_git_config_path, +) + + +def _git(repo, *args, env=None, check=True) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", "-C", str(repo), *args], + capture_output=True, + text=True, + check=check, + env=env if env is not None else isolated_git_env(repo), + timeout=60, + ) + + +def _config_get(config_path, key) -> str | None: + """Read *key* from *config_path*, or None when absent.""" + result = subprocess.run( + ["git", "config", "--file", str(config_path), "--get", key], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + return result.stdout.strip() if result.returncode == 0 else None + + +@pytest.fixture +def shared_repo(tmp_path): + """A real repo with a real-looking identity, plus a linked worktree. + + Stands in for the developer's checkout. The linked worktree matters because that is + the only configuration in which git exports ``GIT_DIR``/``GIT_COMMON_DIR`` to a + hook — which is why this leak never reproduces from a normal checkout. + """ + repo = tmp_path / "shared" + repo.mkdir() + _git(repo, "init", "-q") + _git(repo, "config", "--local", "user.name", "RealDev") + _git(repo, "config", "--local", "user.email", "real@dev.example") + _git(repo, "commit", "-q", "--allow-empty", "-m", "base") + _git(repo, "worktree", "add", "-q", str(tmp_path / "wt"), "-b", "probe") + return repo + + +class TestIsolatedGitEnv: + def test_strips_every_location_var(self, tmp_path): + base = dict.fromkeys(GIT_LOCATION_VARS, "/somewhere/else") + env = isolated_git_env(tmp_path, base=base) + assert not [var for var in GIT_LOCATION_VARS if var in env] + + def test_pins_config_resolution_and_identity(self, tmp_path): + env = isolated_git_env(tmp_path, base={}) + assert env["HOME"] == str(tmp_path) + assert env["GIT_CONFIG_GLOBAL"] == os.path.join(str(tmp_path), ".gitconfig-test") + assert env["GIT_CONFIG_SYSTEM"] == os.devnull + assert env["GIT_CONFIG_NOSYSTEM"] == "1" + assert env["GIT_AUTHOR_EMAIL"] == TEST_IDENTITY_EMAIL + assert env["GIT_COMMITTER_NAME"] == TEST_IDENTITY_NAME + + def test_an_inherited_git_dir_escapes_but_isolated_env_contains(self, tmp_path, shared_repo): + """The differential test. Same command, two environments, opposite outcomes. + + Half A reproduces the bug against a fake shared repo: with ``GIT_DIR`` present, + ``git -C config user.name`` still finds the shared repository and + writes there. Note what this defeats — ``-C`` pointing at a directory that is + not a repository at all, plus ``HOME``/``XDG_CONFIG_HOME``/``GIT_CONFIG_GLOBAL`` + all pinned to a throwaway path. Repository *discovery* is what ``GIT_DIR`` + overrides, so none of those pins are consulted. + + Half B is the same write through ``isolated_git_env``, which lands in the + sandbox's own config and leaves the shared repo byte-identical. + """ + shared_config = shared_repo / ".git" / "config" + before = shared_config.read_bytes() + + # --- Half A: the leak, witnessed --- + escapes = tmp_path / "escapes" + escapes.mkdir() + leaky_env = isolated_git_env(escapes) + leaky_env["GIT_DIR"] = str(shared_repo / ".git" / "worktrees" / "wt") + leaky_env["GIT_COMMON_DIR"] = str(shared_repo / ".git") + + _git(escapes, "config", "user.name", "leaked", env=leaky_env) + + assert not (escapes / ".git").exists(), "the write should not have landed locally" + assert _config_get(shared_config, "user.name") == "leaked", ( + "expected the inherited GIT_DIR to redirect this write into the shared " + "config — if this assertion fails the mechanism has changed and the guard " + "may no longer be guarding anything" + ) + + # Restore, so Half B starts from the original bytes. + shared_config.write_bytes(before) + + # --- Half B: the same write, contained --- + contained = tmp_path / "contained" + contained.mkdir() + _git(contained, "init", "-q") + _git(contained, "config", "user.name", "contained") + + assert _config_get(contained / ".git" / "config", "user.name") == "contained" + assert shared_config.read_bytes() == before, "shared config must be untouched" + + +class TestAutouseFixture: + def test_ambient_location_vars_are_stripped(self): + """``conftest._isolate_git_location`` has already run for this test. + + Asserted on ``os.environ`` rather than on a passed-in env because the risk is a + fixture that shells out with the *inherited* environment. + """ + assert not [var for var in GIT_LOCATION_VARS if var in os.environ] + + def test_ambient_config_resolution_is_pinned(self): + assert os.environ["GIT_CONFIG_SYSTEM"] == os.devnull + assert os.environ["GIT_CONFIG_NOSYSTEM"] == "1" + assert os.environ["GIT_AUTHOR_EMAIL"] == TEST_IDENTITY_EMAIL + # Pinned to a per-test tmp path, so a bare `git config user.email` cannot reach + # the developer's ~/.gitconfig even from a fixture that forgot isolated_git_env. + assert os.environ["GIT_CONFIG_GLOBAL"].endswith(".gitconfig-test") + assert os.path.expanduser("~") not in (os.environ["GIT_CONFIG_GLOBAL"],) + + +class TestSharedConfigResolution: + def test_resolves_through_git_common_dir_not_show_toplevel(self, tmp_path, monkeypatch): + """``core.worktree`` must not be able to disable the detector. + + This is the failure the resolution choice exists to avoid: ``core.worktree`` + redirects ``--show-toplevel``, so a detector built on it computes a path that + does not exist in a polluted repo and reports "nothing to protect" — the + pollution switching off its own alarm. ``--git-common-dir`` is answered from the + gitdir alone. + """ + repo = tmp_path / "polluted" + repo.mkdir() + _git(repo, "init", "-q") + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + _git(repo, "config", "--local", "core.worktree", str(elsewhere)) + + monkeypatch.chdir(repo) + + # The rejected approach: redirected away from the real repo. + toplevel = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + assert toplevel.stdout.strip() != str(repo) + + # The chosen approach: still the real shared config. + assert shared_git_config_path() == str(repo / ".git" / "config") + + def test_returns_none_outside_a_repository(self, tmp_path, monkeypatch): + outside = tmp_path / "not-a-repo" + outside.mkdir() + monkeypatch.chdir(outside) + # GIT_CEILING_DIRECTORIES stops discovery from walking up into whatever + # repository happens to contain tmp_path on this machine. + monkeypatch.setenv("GIT_CEILING_DIRECTORIES", str(tmp_path)) + assert shared_git_config_path() is None + + +class TestFingerprint: + @staticmethod + def _fingerprint(config) -> tuple[str, frozenset[str]]: + """``fingerprint_git_config`` narrowed to non-None. + + It returns ``None`` for an unreadable path — a real case, covered by its own + test below — so unpacking the result directly is a type error (ty + ``not-iterable``). Asserting here keeps that contract visible instead of + annotating it away, and a None would fail the assertion rather than raise an + opaque unpacking error further down. + """ + result = fingerprint_git_config(str(config)) + assert result is not None, f"expected {config} to be readable" + return result + + def test_detects_an_added_key_and_names_it(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + + digest_before, names_before = self._fingerprint(config) + _git(repo, "config", "--local", "core.worktree", str(tmp_path)) + digest_after, names_after = self._fingerprint(config) + + assert digest_after != digest_before + assert names_after - names_before == {"core.worktree"} + + def test_detects_a_value_change_without_capturing_the_value(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + # A remote URL with a userinfo segment: the realistic reason `.git/config` + # must never be echoed. Synthetic — reserved domain (RFC 2606), and the + # userinfo is the literal word `placeholder`. Named for what it is (a URL) + # rather than `secret`, which made ruff S105 read it as a hardcoded + # credential; nothing here is one. + url_with_credential = "https://user:placeholder@example.invalid/repo.git" + _git(repo, "config", "--local", "remote.origin.url", "https://example.invalid/a.git") + + digest_before, names_before = self._fingerprint(config) + _git(repo, "config", "--local", "remote.origin.url", url_with_credential) + digest_after, names_after = self._fingerprint(config) + + assert digest_after != digest_before, "a value-only change must still be detected" + assert names_after == names_before, "no key was added, so the name set is stable" + # The reason names-not-values: this data is printed into CI logs on failure. + assert url_with_credential not in str(names_after) + + def test_returns_none_for_an_unreadable_path(self, tmp_path): + assert fingerprint_git_config(str(tmp_path / "nope" / "config")) is None + + +class TestMutationReport: + """The session-level detector's decision logic (``conftest``, Layer 2). + + Unit-tested here because the hook itself cannot be exercised from inside the + session it guards: no test can observe a mutation made by a test that runs after + it, which is precisely why the check lives in ``pytest_sessionfinish``. + """ + + @staticmethod + def _repo_with_config(tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + return repo, repo / ".git" / "config" + + def _run_report(self, monkeypatch, config, fingerprint): + from tests import conftest + + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", (str(config), fingerprint)) + session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) + conftest._report_shared_git_config_mutation(session) + return session + + def test_fails_the_session_when_the_config_changed(self, tmp_path, monkeypatch, capsys): + repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + _git(repo, "config", "--local", "user.email", "t@t") + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + message = capsys.readouterr().err + assert "SHARED GIT CONFIG MUTATED" in message + # The remedy must be copy-pasteable, not a description of one. + assert f"git config --file {config} --remove-section user" in message + assert "user.email" in message + + def test_leaves_a_clean_session_alone(self, tmp_path, monkeypatch): + _repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.OK + + def test_is_inert_when_there_was_nothing_to_protect(self, monkeypatch): + """No repository (e.g. running inside the built container image) must not fail + the suite — that is a genuine no-risk case, not a failure to look.""" + from tests import conftest + + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", None) + session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) + conftest._report_shared_git_config_mutation(session) + assert session.exitstatus == pytest.ExitCode.OK + + def test_reports_a_config_that_vanished(self, tmp_path, monkeypatch, capsys): + _repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + config.unlink() + + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + assert "unreadable or gone" in capsys.readouterr().err diff --git a/agent/tests/test_post_hooks.py b/agent/tests/test_post_hooks.py index 3768aec1b..6ffc6b2f1 100644 --- a/agent/tests/test_post_hooks.py +++ b/agent/tests/test_post_hooks.py @@ -6,15 +6,13 @@ ``shell.run_cmd`` (mutating git/gh commands) — both faked with recorders. """ -import os import subprocess from types import SimpleNamespace -import pytest - import post_hooks from models import RepoSetup from tests.conftest import FakeRunCmd, make_task_config +from tests.git_env import isolated_git_env # post_hooks.py keys scripted results off the exact label (FakeRunCmd's default # exact-match mode), so e.g. returncodes={"push": 1} does not bleed into the @@ -279,81 +277,16 @@ 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 _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 = { - 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 + # Containment comes from ``tests/git_env.isolated_git_env`` (#855), applied + # explicitly below AND to every test by the ``_isolate_git_location`` autouse + # fixture in ``tests/conftest.py``. This class used to carry its own copies of + # the location-var tuple and the env builder; they were correct but reachable + # from nowhere else, so #665 added a fresh unguarded helper in + # test_registry_loader.py and reopened the leak. One definition, imported. + # + # Kept explicit here on top of the autouse fixture because ``post_hooks`` + # itself shells out to git: an env passed per call documents at the call site + # that these fixtures must never touch a repo outside ``tmp_path``. def _git(self, repo, *args): subprocess.run( @@ -362,7 +295,7 @@ def _git(self, repo, *args): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ) def _make_repo(self, tmp_path): @@ -391,15 +324,25 @@ def test_fixture_cannot_touch_an_outer_repo_even_with_git_dir_set(self, tmp_path _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) + # Build the stand-in "real" repo through isolated_git_env rather than the + # ambient environment. The test is *about* a hostile ambient env, so its own + # setup must not depend on one being clean — otherwise a regression in the + # conftest fixture would make this guard build its sentinel in the actual + # repository, i.e. cause the very leak it exists to detect. + subprocess.run( + ["git", "init", "-q"], + cwd=outer, + check=True, + capture_output=True, + env=isolated_git_env(outer), + ) sentinel_config = outer / ".git" / "config" subprocess.run( ["git", "config", "--local", "user.email", "sentinel@example.invalid"], cwd=outer, check=True, capture_output=True, + env=isolated_git_env(outer), ) before = sentinel_config.read_text() @@ -423,7 +366,7 @@ def _head_sha(self, repo): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ).stdout.strip() def _sha_of(self, repo, ref): @@ -433,7 +376,7 @@ def _sha_of(self, repo, ref): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ).stdout.strip() def test_reconciles_when_agent_on_own_branch(self, tmp_path): diff --git a/agent/tests/test_registry_loader.py b/agent/tests/test_registry_loader.py index 21717ecbb..ce3c963e6 100644 --- a/agent/tests/test_registry_loader.py +++ b/agent/tests/test_registry_loader.py @@ -13,6 +13,7 @@ apply_resolved_assets, build_skill_prompt_fragment, ) +from tests.git_env import isolated_git_env def _read_mcp(repo_dir) -> dict: @@ -295,17 +296,27 @@ class TestMcpJsonNotCommittable: @staticmethod def _git(repo, *args) -> subprocess.CompletedProcess: + # ``env=`` is load-bearing (#855). Without it, an inherited GIT_DIR — which + # git exports to hooks in a linked worktree, i.e. whenever this suite runs + # as a pre-push gate from .worktrees/ — overrides repository discovery, so + # `-C ` is ignored and every command below operates on the REAL + # repository. ``check=False`` is why that stayed invisible: re-initing the + # real repo and rewriting its config both exit 0. return subprocess.run( ["git", "-C", str(repo), *args], capture_output=True, text=True, check=False, + env=isolated_git_env(repo), ) def _init_repo(self, tmp_path): + # No `git config user.*` here on purpose: isolated_git_env supplies the + # identity through GIT_AUTHOR_*/GIT_COMMITTER_*, which outrank every config + # file, so the commit below is attributed without any config write at all. + # The two writes this replaces are the literal source of #720 — `t ` + # was transcribed into a real repository's config and then into real commits. self._git(tmp_path, "init", "-q") - self._git(tmp_path, "config", "user.email", "t@t") - self._git(tmp_path, "config", "user.name", "t") (tmp_path / "README.md").write_text("x") self._git(tmp_path, "add", "README.md") self._git(tmp_path, "commit", "-qm", "init") diff --git a/cdk/test/scripts/check-git-config-clean.test.ts b/cdk/test/scripts/check-git-config-clean.test.ts new file mode 100644 index 000000000..4cec828a6 --- /dev/null +++ b/cdk/test/scripts/check-git-config-clean.test.ts @@ -0,0 +1,393 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Tests for `scripts/check-git-config-clean.mjs` — Layer 3 of the #855 git-config + * guard, the pre-commit/pre-push gate. + * + * WHY THESE EXIST: it is a GATE, and a gate's worst failure is a false pass. This + * one has two independent ways to reach one: a detection rule that stops matching, + * and a config-path resolution that quietly points somewhere harmless. The second is + * not hypothetical — the first draft resolved the path with + * `git rev-parse --git-common-dir`, which ABORTS when `core.worktree` names a + * missing directory, so on the most common real shape of this corruption it could + * only say "could not check". So every rule is asserted by making it fire, and the + * hostile-resolution cases have tests of their own. + * + * WHY THIS LIVES UNDER `cdk/test/` for a ROOT-level script: same reason as + * `check-constants-sync.test.ts` — there is no test tree at the repo root, and + * `cdk/` is the only workspace with a Jest runner that can reach `../../scripts`. + * Deliberate placement, not misrouting. The suite exercises a subprocess, so it + * contributes nothing to `cdk/src` coverage. + * + * NOTE ON THIS FILE'S OWN GIT CALLS: they go through `isolatedGitEnv`, a TypeScript + * mirror of `agent/tests/git_env.py`. That is not ceremony. Jest here may itself be + * running under the pre-push hook, where git has exported `GIT_DIR` — and an + * inherited `GIT_DIR` would make `git init ` re-init the REAL repository. A + * test suite for this gate that caused the leak while setting up would be a poor + * joke, so the isolation is applied and then asserted on (see the last describe). + */ + +import { execFileSync, spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const REPO_ROOT = path.resolve(__dirname, '../../..'); +const SCRIPT = path.join(REPO_ROOT, 'scripts/check-git-config-clean.mjs'); + +/** + * The shared config of the checkout this suite is running in. + * + * NOT `join(REPO_ROOT, '.git', 'config')`: in a linked worktree — which is how this + * repo's own contribution flow works — `.git` is a FILE pointing elsewhere, so that + * path does not exist. Asked of git rather than hand-resolved because the script under + * test resolves it without git, and a hand-rolled copy here would agree with the + * script's bugs instead of catching them. + */ +function realSharedConfigPath(): string { + const commonDir = execFileSync( + 'git', + ['-C', REPO_ROOT, 'rev-parse', '--path-format=absolute', '--git-common-dir'], + { encoding: 'utf-8' }, + ).trim(); + return path.join(commonDir, 'config'); +} + +/** Repo-location vars — mirrors `GIT_LOCATION_VARS` in `agent/tests/git_env.py`. */ +const 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', +]; + +/** An environment in which git cannot reach outside `repo`. */ +function isolatedGitEnv(repo: string): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env }; + // Removed FIRST: while any is set, every pin below is bypassed. + for (const key of GIT_LOCATION_VARS) delete env[key]; + return { + ...env, + HOME: repo, + XDG_CONFIG_HOME: repo, + GIT_CONFIG_GLOBAL: path.join(repo, '.gitconfig-test'), + GIT_CONFIG_SYSTEM: '/dev/null', + 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', + }; +} + +function git(repo: string, args: readonly string[]): void { + const result = spawnSync('git', ['-C', repo, ...args], { + encoding: 'utf-8', + env: isolatedGitEnv(repo), + }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed (${result.status}): ${result.stderr}`); + } +} + +interface RunResult { + readonly status: number; + readonly stdout: string; + readonly stderr: string; +} + +/** Run the gate with `cwd` (and optionally extra env), capturing the outcome. */ +function runGate(cwd: string, extraEnv: NodeJS.ProcessEnv = {}): RunResult { + try { + const stdout = execFileSync(process.execPath, [SCRIPT], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...isolatedGitEnv(cwd), ...extraEnv }, + }); + return { status: 0, stdout, stderr: '' }; + } catch (err) { + const e = err as { status?: number; stdout?: string; stderr?: string }; + return { status: e.status ?? -1, stdout: e.stdout ?? '', stderr: e.stderr ?? '' }; + } +} + +let scratch: string; + +/** A fresh, clean repository under the scratch dir. */ +function freshRepo(name: string): string { + const repo = path.join(scratch, name); + fs.mkdirSync(repo, { recursive: true }); + git(repo, ['init', '-q']); + return repo; +} + +/** Set a local config key, bypassing git (which may refuse on a broken repo). */ +function appendConfig(repo: string, section: string, lines: readonly string[]): void { + const configPath = path.join(repo, '.git', 'config'); + fs.appendFileSync(configPath, `[${section}]\n${lines.map((l) => `\t${l}\n`).join('')}`); +} + +describe('check-git-config-clean', () => { + // A handful of subprocess spawns plus git inits. + jest.setTimeout(60_000); + + /** Digest of the real shared config, captured before any test body runs. */ + let sharedConfigDigestAtStart: string; + + beforeAll(() => { + // os.tmpdir() honours TMPDIR, which the pre-push hook points at + // ~/.cache/cdk-tmp — so this does not land in a RAM-backed /tmp there. + // + // realpathSync because assertions below compare against paths the SCRIPT + // printed, and the script derives them from `process.cwd()`, which Node reports + // physically. On a machine where $HOME is a symlink (e.g. /home/x → + // /local/home/x) the logical and physical spellings differ, and the remedy + // strings would never match. + scratch = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'abca-git-config-clean-'))); + sharedConfigDigestAtStart = execFileSync('git', ['hash-object', realSharedConfigPath()], { + encoding: 'utf-8', + }).trim(); + }); + + afterAll(() => { + fs.rmSync(scratch, { recursive: true, force: true }); + }); + + describe('the clean cases', () => { + test('a fresh repository passes, and says what it checked', () => { + const result = runGate(freshRepo('clean')); + + expect(result.status).toBe(0); + // The rule list is the anti-vacuity assertion: a gate that inspected NOTHING + // would also exit 0. Naming them means a dropped rule shows up here. + expect(result.stdout).toContain('core.worktree'); + expect(result.stdout).toContain('core.bare'); + expect(result.stdout).toContain('user.name'); + expect(result.stdout).toContain('user.email'); + expect(result.stdout).toMatch(/OK — 4 rule\(s\)/); + }); + + test('THIS repository passes', () => { + // Not a self-test for its own sake: this is a third detection surface, after + // the conftest fixture (prevent) and the session hook (detect). If a + // contributor's shared config is polluted, the cdk suite says so here. + const result = runGate(REPO_ROOT); + + expect(result.stderr).toBe(''); + expect(result.status).toBe(0); + }); + + test('a real per-repo identity is NOT flagged', () => { + // The false-positive side, and the reason the rules match the leak's + // SIGNATURE rather than the mere presence of a [user] section. Per-repo + // identities are common; a gate that failed on them would be switched off + // instead of fixed. + const repo = freshRepo('real-identity'); + git(repo, ['config', '--local', 'user.name', 'Ada Lovelace']); + git(repo, ['config', '--local', 'user.email', 'ada@example-corp.dev']); + + expect(runGate(repo).status).toBe(0); + }); + + test('a GitHub noreply address is NOT flagged', () => { + const repo = freshRepo('noreply'); + git(repo, ['config', '--local', 'user.email', '1234+ada@users.noreply.github.com']); + + expect(runGate(repo).status).toBe(0); + }); + + test('core.bare = false is NOT flagged', () => { + // `git init` writes this itself, so flagging it would fail every repository. + const repo = freshRepo('bare-false'); + git(repo, ['config', '--local', 'core.bare', 'false']); + + expect(runGate(repo).status).toBe(0); + }); + }); + + describe('core.worktree — the #622/#720 signature', () => { + test('is rejected, with a copy-pasteable remedy', () => { + const repo = freshRepo('worktree'); + const elsewhere = path.join(scratch, 'elsewhere'); + fs.mkdirSync(elsewhere, { recursive: true }); + git(repo, ['config', '--local', 'core.worktree', elsewhere]); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.worktree'); + // The remedy must be runnable as printed, not a description of one. + expect(result.stderr).toContain( + `git config --file ${path.join(repo, '.git', 'config')} --unset-all core.worktree`, + ); + }); + + test('is rejected even when it points at a path that no longer EXISTS', () => { + // The case that drove the design. `core.worktree` left behind by a fixture + // names a pytest tmp_path, which is deleted at the end of the session — and + // `git rev-parse` (any form) then aborts with `fatal: Invalid path`, as does + // `git config` run from inside the repo. A gate that resolved its own target + // through git could only report "could not check" on the most common real + // shape of this corruption. Written with fs.appendFileSync because git itself + // refuses to set the second key once the first has broken the repo. + const repo = freshRepo('worktree-missing'); + appendConfig(repo, 'core', [`worktree = ${path.join(scratch, 'deleted-tmp-path')}`]); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.worktree'); + expect(result.stderr).toContain('deleted-tmp-path'); + }); + + test('is found in the SHARED config when run from a linked worktree', () => { + // Linked worktrees are where this leak happens, so resolution has to follow + // the `commondir` pointer rather than stopping at the per-worktree gitdir. + const repo = freshRepo('shared'); + fs.writeFileSync(path.join(repo, 'f.txt'), 'x\n'); + git(repo, ['add', '-A']); + git(repo, ['commit', '-qm', 'base']); + const linked = path.join(scratch, 'linked-wt'); + git(repo, ['worktree', 'add', '-q', linked, '-b', 'probe']); + + appendConfig(repo, 'user', ['name = t', 'email = t@t']); + + const result = runGate(linked); + + expect(result.status).toBe(1); + expect(result.stderr).toContain(path.join(repo, '.git', 'config')); + expect(result.stderr).toContain('user.name'); + }); + }); + + describe('core.bare on a checkout', () => { + test('is rejected', () => { + const repo = freshRepo('bare-true'); + git(repo, ['config', '--local', 'core.bare', 'true']); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.bare'); + expect(result.stderr).toContain('--unset-all core.bare'); + }); + }); + + describe('fixture identities — the #720 sighting', () => { + test.each([ + ['user.name = t', 'user', ['name = t']], + ['user.email = t@t (no dot in the domain)', 'user', ['email = t@t']], + ['a reserved .invalid domain', 'user', ['email = abca-test@example.invalid']], + ['example.com', 'user', ['email = someone@example.com']], + ['an empty value', 'user', ['name = ']], + ])('%s is rejected', (_label, section, lines) => { + const repo = freshRepo(`identity-${_label.replace(/[^a-z0-9]+/gi, '-')}`); + appendConfig(repo, section, lines); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('--remove-section user'); + }); + + test('names the offending value so the human can see what replaced theirs', () => { + const repo = freshRepo('identity-named'); + appendConfig(repo, 'user', ['email = t@t']); + + expect(runGate(repo).stderr).toContain('user.email = t@t'); + }); + }); + + describe('cannot-check is a FAILURE, not a pass', () => { + test('outside any repository, exits 2 and says why', () => { + // Fail-closed. Silently exiting 0 here would make a mis-wired hook look like a + // clean repo forever. + // + // Run from `/` rather than a scratch dir: resolution walks UP for `.git`, so a + // scratch dir's verdict would depend on where TMPDIR points (inside a checkout + // on some machines, outside on others). `/` has no parent, so the walk + // terminates immediately and the outcome is the same everywhere. + expect(fs.existsSync('/.git')).toBe(false); // the one assumption `/` makes + + const result = runGate('/'); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('no `.git` found'); + }); + + test('a missing .git/config exits 2 rather than reporting clean', () => { + const repo = freshRepo('no-config'); + fs.rmSync(path.join(repo, '.git', 'config')); + + const result = runGate(repo); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('does not exist'); + }); + }); + + describe('an inherited GIT_DIR — the hook environment', () => { + test('is honoured for locating the repo, and does not blind the check', () => { + // Git exports GIT_DIR to hooks in a linked worktree. For a WRITE that is the + // hazard this whole issue is about; for the gate's READ it is the accurate + // answer, so it is used — and must still find the corruption. + const repo = freshRepo('git-dir-env'); + appendConfig(repo, 'user', ['email = t@t']); + const unrelated = path.join(scratch, 'unrelated-cwd'); + fs.mkdirSync(unrelated, { recursive: true }); + + const result = runGate(unrelated, { GIT_DIR: path.join(repo, '.git') }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain(path.join(repo, '.git', 'config')); + }); + }); + + describe("this suite's own git isolation", () => { + test('isolatedGitEnv strips every location var and pins config resolution', () => { + // Asserted because the isolation is what stops these tests from re-creating + // the bug while setting up: with a GIT_DIR inherited from the pre-push hook, + // `git init ` re-inits the real repository. + const env = isolatedGitEnv('/somewhere'); + + for (const key of GIT_LOCATION_VARS) { + expect(env[key]).toBeUndefined(); + } + expect(env.HOME).toBe('/somewhere'); + expect(env.GIT_CONFIG_GLOBAL).toBe('/somewhere/.gitconfig-test'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + }); + + test('the real repository config is byte-identical after this suite has run', () => { + // The blunt instrument, and the one that would actually have caught #622, + // #695, #720 and #665. Declared last so it runs last in file order. + const digest = execFileSync('git', ['hash-object', realSharedConfigPath()], { + encoding: 'utf-8', + }).trim(); + + expect(digest).toBe(sharedConfigDigestAtStart); + }); + }); +}); diff --git a/mise.toml b/mise.toml index 34c8b7875..4c40349bb 100644 --- a/mise.toml +++ b/mise.toml @@ -106,6 +106,10 @@ run = "yarn knip" description = "Dead-code ratchet (#282): fail only if knip's issue count rises above knip-baseline.json. Advisory in CI for now; flips to blocking once the baseline is driven to zero." run = "node scripts/check-deadcode-ratchet.mjs" +[tasks."check:git-config-clean"] +description = "Shared .git/config corruption gate (#855): fail if the repository's shared config carries the test-fixture leak signature — core.worktree, core.bare on a checkout, or a fixture identity in [user]. Local-only by design (a CI runner's config is ephemeral); wired into pre-commit and pre-push." +run = "node scripts/check-git-config-clean.mjs" + [tasks."check:transitive-pin-sync"] description = "Transitive-pin sync guard (#712): fail if a package pinned in root `resolutions` resolves below that floor in integrations/jira-forge-app's npm lockfile — the standalone project root `resolutions` can't reach." run = "node scripts/check-transitive-pin-sync.mjs" diff --git a/scripts/check-git-config-clean.mjs b/scripts/check-git-config-clean.mjs new file mode 100644 index 000000000..1bc6460f4 --- /dev/null +++ b/scripts/check-git-config-clean.mjs @@ -0,0 +1,318 @@ +#!/usr/bin/env node +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Shared `.git/config` corruption gate (issue #855; recurrences #622, #695, #720, #665). + * + * Layer 3 of three. Layer 1 (`agent/tests/git_env.py` + the `_isolate_git_location` + * autouse fixture in `agent/tests/conftest.py`) PREVENTS the leak; Layer 2 + * (`pytest_sessionstart`/`pytest_sessionfinish` in the same conftest) DETECTS a + * mutation during a test run. This layer REFUSES: it runs at pre-commit and + * pre-push and blocks the operation while the repository's shared config carries + * the leak's signature, no matter which tool wrote it. + * + * Three layers rather than one because the same bug has now been "fixed" four + * times. Each earlier fix hardened the one test file where the leak was observed, + * and each was defeated by the next file to shell out to git. A gate outside the + * test suite entirely cannot be outrun that way. + * + * WHAT IT LOOKS FOR — the signature, not merely unusual settings: + * + * 1. `core.worktree` — never legitimate in a normal checkout. Set, it pins EVERY + * linked worktree to one directory, so the root reads as dirty, the root's own + * untracked files disappear from `git status`, and `git revert` silently + * no-ops. Written when a fixture runs `git init` with both GIT_DIR and + * GIT_WORK_TREE inherited from the environment. + * 2. `core.bare = true` on a repo that has a working tree — the other stamp the + * same `git init` leaves behind. + * 3. `user.name`/`user.email` holding a value no human would have: a reserved + * documentation domain (RFC 2606), a domain with no dot, or one of the literal + * fixture identities used in this repo. A real per-repo identity is COMMON and + * deliberately NOT flagged — a gate that fired on legitimate configuration + * would be switched off rather than fixed. + * + * WHY THE CONFIG PATH IS RESOLVED WITHOUT GIT AT ALL: no `git rev-parse` form + * survives the state being detected. `--show-toplevel` is redirected by + * `core.worktree` outright — the corruption disabling its own alarm — and + * `--git-common-dir` merely fails differently: when `core.worktree` names a path + * that no longer exists (a deleted pytest `tmp_path`, i.e. the shape this leak + * actually leaves behind), rev-parse aborts with `fatal: Invalid path`, so a check + * built on it can only report "could not check" and never name the cause. Walking + * the filesystem for `.git` is deterministic and reads no config, so it answers + * correctly on a repository too broken for git to describe. The + * `.pre-commit-config.yaml` entry for this hook is likewise the only one in the file + * that does NOT `cd "$(git rev-parse --show-toplevel)"`. + * + * Reads are delegated to `git config --file ` so the parse is git's own, and + * because `--file` involves no repository discovery — the one git operation this + * corruption cannot reach. + * + * Exit codes: 0 clean · 1 corruption found (with remedy) · 2 could not check. + * Case 2 is a failure, not a pass: an unreadable config or a git that cannot answer + * is exactly the state in which a leak would go unnoticed. + * + * Known limitation: submodules. Git legitimately sets `core.worktree` in a + * submodule's own config, and `--git-common-dir` resolves to whichever repository + * cwd belongs to — so committing from inside a submodule would flag rule 1. This + * repo has no submodules; if that changes, exempt them explicitly rather than + * dropping the rule. + */ + +import { existsSync, readFileSync, statSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { dirname, isAbsolute, join, resolve } from 'node:path'; + +/** Literal identities used by fixtures in this tree. `t ` is the #720 sighting. */ +const FIXTURE_NAMES = new Set(['t', 'test', 'abca test', 'test user', 'your name']); + +/** + * Reserved / documentation domains (RFC 2606 + RFC 6761). An address here can never + * be a real deliverable identity, so finding one in a repo config means a fixture + * put it there. + */ +const RESERVED_EMAIL_SUFFIXES = [ + '.invalid', + '.test', + '.example', + '.localhost', + '@example.com', + '@example.net', + '@example.org', +]; + +/** + * Repo-location vars, mirroring `GIT_LOCATION_VARS` in `agent/tests/git_env.py`. + * Stripped before reading, for the same reason the fixtures strip them: while any is + * set, git resolves a repository from the environment instead of from what we asked. + */ +const GIT_LOCATION_VARS = [ + 'GIT_DIR', + 'GIT_COMMON_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_PREFIX', +]; + +/** + * Run git against an explicit config file from OUTSIDE any repository. + * + * The cwd and the env pins are both load-bearing, and the reason is unobvious: + * `git config --file ` reads only that file, but git still performs + * REPOSITORY SETUP for its working directory first — so run inside a repo whose + * `core.worktree` names a missing directory, it aborts with `fatal: Invalid path` + * before reading anything. That is the exact state this gate has to report on, so + * the read cannot happen from inside the repository. cwd `/` plus + * `GIT_CEILING_DIRECTORIES` leaves discovery nothing to find, and the location vars + * are dropped so an inherited `GIT_DIR` (git sets one for hooks in a linked + * worktree) cannot put the broken repository back. + * + * Never throws, never uses a shell. + */ +function gitConfigRead(args) { + const env = { ...process.env }; + for (const key of GIT_LOCATION_VARS) delete env[key]; + env.GIT_CEILING_DIRECTORIES = '/'; + env.GIT_CONFIG_NOSYSTEM = '1'; + + const result = spawnSync('git', args, { encoding: 'utf8', cwd: '/', env }); + if (result.error) { + return { status: null, stdout: '', stderr: String(result.error.message) }; + } + return { + status: result.status, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + }; +} + +function bail(message) { + console.error(`check-git-config-clean: ${message}`); + process.exit(2); +} + +/** + * The gitdir for the tree we are operating on, found without consulting any config. + * + * `GIT_DIR` is honoured when present because git sets it for hooks and it names the + * exact tree being committed to. Note the asymmetry with the leak itself: an + * inherited `GIT_DIR` is dangerous for a WRITE aimed at somewhere else, and + * authoritative for a READ that wants this repository. + */ +function findGitDir() { + if (process.env.GIT_DIR) return resolve(process.env.GIT_DIR); + + let dir = process.cwd(); + for (;;) { + const candidate = join(dir, '.git'); + if (existsSync(candidate)) { + const stat = statSync(candidate); + if (stat.isDirectory()) return candidate; + if (stat.isFile()) { + // Linked worktree (or a submodule): `gitdir: `, possibly relative to + // the directory holding the `.git` file. + const match = /^gitdir:\s*(.+)$/m.exec(readFileSync(candidate, 'utf8')); + if (!match) { + bail(`${candidate} is a file but has no \`gitdir:\` line — cannot locate the repository.`); + } + const pointed = match[1].trim(); + return isAbsolute(pointed) ? pointed : resolve(dir, pointed); + } + bail(`${candidate} is neither a file nor a directory.`); + } + const parent = dirname(dir); + if (parent === dir) { + bail( + 'no `.git` found in this directory or any parent, so there is no shared ' + + 'config to check. This is a git-hook gate — run it from a checkout.', + ); + } + dir = parent; + } +} + +/** Absolute path of the repository-shared config, or exit 2 explaining why not. */ +function sharedConfigPath() { + const gitDir = findGitDir(); + + // A linked worktree's gitdir holds a `commondir` pointer to the SHARED `.git`, + // which is the file at risk — a per-worktree config would not be. + let commonDir = gitDir; + const commonDirFile = join(gitDir, 'commondir'); + if (existsSync(commonDirFile)) { + const pointed = readFileSync(commonDirFile, 'utf8').trim(); + if (pointed) commonDir = isAbsolute(pointed) ? pointed : resolve(gitDir, pointed); + } + + const config = join(commonDir, 'config'); + if (!existsSync(config) || !statSync(config).isFile()) { + bail( + `${config} does not exist or is not a file. Every git repository has one, so ` + + 'this repository is in an unexpected state — check it by hand.', + ); + } + return config; +} + +/** All values of `key` in `configPath` (empty array when unset). */ +function configValues(configPath, key) { + const result = gitConfigRead(['config', '--file', configPath, '--get-all', key]); + // rc 1 is git's "key not present" — the normal, clean case. + if (result.status === 1) return []; + if (result.status !== 0) { + bail( + `cannot read ${key} from ${configPath} ` + + `(${result.stderr.trim() || `git exited ${result.status}`}).`, + ); + } + // Strip only the ONE trailing newline git ends its output with, rather than + // filtering empty lines out: `name =` with no value is a real state (a fixture + // interpolating an unset variable writes it) and prints as an empty line, so a + // blanket filter would drop the very value that has to be reported. rc 0 means + // at least one value was found, so the result is never an empty list here. + const stdout = result.stdout.endsWith('\n') ? result.stdout.slice(0, -1) : result.stdout; + return stdout.split('\n'); +} + +/** True when this identity value could not belong to a real contributor. */ +function isFixtureIdentity(key, value) { + const v = value.trim().toLowerCase(); + if (v === '') return true; + if (key === 'user.name') return FIXTURE_NAMES.has(v); + if (RESERVED_EMAIL_SUFFIXES.some((suffix) => v.endsWith(suffix))) return true; + // No dot in the domain means it is not a resolvable FQDN — `t@t`, `a@b`. + const domain = v.split('@')[1]; + return domain !== undefined && !domain.includes('.'); +} + +const configPath = sharedConfigPath(); +const problems = []; +const rulesChecked = []; + +// --- Rule 1: core.worktree --------------------------------------------------- +rulesChecked.push('core.worktree'); +for (const value of configValues(configPath, 'core.worktree')) { + problems.push({ + what: `core.worktree = ${value}`, + why: + 'pins every linked worktree to one directory: the root reads as dirty, its ' + + 'own untracked files vanish from `git status`, and `git revert` no-ops.', + fix: `git config --file ${configPath} --unset-all core.worktree`, + }); +} + +// --- Rule 2: core.bare on a repo that has a working tree --------------------- +rulesChecked.push('core.bare'); +for (const value of configValues(configPath, 'core.bare')) { + if (value.trim().toLowerCase() !== 'true') continue; + problems.push({ + what: `core.bare = ${value}`, + why: + 'this repository has a working tree, so it is not bare. The same stray ' + + '`git init` that writes core.worktree stamps this.', + fix: `git config --file ${configPath} --unset-all core.bare`, + }); +} + +// --- Rule 3: fixture identities ---------------------------------------------- +for (const key of ['user.name', 'user.email']) { + rulesChecked.push(key); + for (const value of configValues(configPath, key)) { + if (!isFixtureIdentity(key, value)) continue; + problems.push({ + what: `${key} = ${value === '' ? '(empty)' : value}`, + why: + 'not a value a contributor would set — a reserved domain, a domain with no ' + + 'dot, or a literal fixture identity. Commits made under it are ' + + 'unattributable, and it silently replaced whatever was configured before.', + fix: `git config --file ${configPath} --remove-section user`, + }); + } +} + +if (problems.length > 0) { + console.error(`check-git-config-clean: ${configPath} carries the #855 leak signature.\n`); + for (const { what, why, fix } of problems) { + console.error(` ✖ ${what}`); + console.error(` ${why}`); + console.error(` fix: ${fix}\n`); + } + console.error( + 'A test or script shelled out to git with a GIT_DIR inherited from the ' + + 'environment (git exports one to hooks in a linked worktree), which overrides ' + + 'repository discovery and so defeats cwd, --local and the GIT_CONFIG_* pins ' + + 'alike. In agent/tests, build the environment with ' + + 'isolated_git_env() from tests/git_env.py.\n', + ); + console.error( + `Found ${problems.length} problem(s). Repair the config with the command(s) ` + + 'above, then re-run. Do not bypass this hook: the state it is reporting ' + + 'makes `git status` and `git revert` lie to you.', + ); + process.exit(1); +} + +// The counts are the anti-vacuity signal: a check that inspected nothing would +// also exit 0. +console.log( + `check-git-config-clean: OK — ${rulesChecked.length} rule(s) ` + + `(${rulesChecked.join(', ')}) clean in ${configPath}.`, +); From 9ef7068d2f9bb051e8c04a020cc4463c6008f8bd Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:17:48 +0000 Subject: [PATCH 2/3] fix(tests): close the discovery route and de-vacuify the #855 guard (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses isadeks' review on #856. Findings 1-4 landed earlier; this covers 5, 6, 7 and the "Also". Layer 1 closed only the GIT_DIR *redirect* route, leaving repository *discovery* wide open — and pytest runs from `agent/`, inside the checkout. Measured on git 2.50.1: from a non-repository subdirectory of a repository, a bare `git config user.email t@t` with no GIT_DIR, no -C and no cwd= walks UP and writes the parent's config, rc 0. That is the #855 leak reached without any GIT_DIR at all, and reached by exactly the author the fixture is advertised to protect — the one who forgot isolated_git_env. - conftest `_isolate_git_location` gains job 3: `monkeypatch.chdir(tmp_path)` plus a `GIT_CEILING_DIRECTORIES` pin, so that command now fails loudly (`fatal: not in a git directory`) instead of succeeding somewhere it should not. Suite unaffected by the chdir: 1818 passed. - GIT_LOCATION_VARS drops to 7. GIT_CEILING_DIRECTORIES was in it, so the fixture was deleting its own fence: every other entry REDIRECTS git and is correctly removed, while the ceiling LIMITS the walk and removing it widens discovery. It is now pinned — at `tmp_path.parent`, not tmp_path, so a test's own repo stays discoverable. This also resolves the 8/7/8 divergence across the three mirrors. - `_ceiling_directories`/`find_git_dir` use realpath, not abspath: git resolves ceiling entries through symlinks (verified), so an abspath copy silently fails to match wherever $HOME or TMPDIR is a symlink — the shape this repo's dev hosts have. - New `TestLayer2IsArmed` asserts `_SHARED_GIT_CONFIG is not None` when run inside a checkout. sessionstart has three early returns that disarm the layer for a whole session while leaving every test green. - New `test_the_strip_has_teeth_out_of_process` spawns a nested pytest with the hook environment set. The in-process assertion could not prove the strip runs: the vars are absent from the parent environment normally, and an autouse fixture has already run before any test body starts. Measured with the strip loop deleted — in-process test PASSES, this one FAILS. - New `TestCrossCopyParity` enforces the GIT_LOCATION_VARS mirror claim the comments were only asserting, per-mirror. - The differential test's docstring is corrected: it proves the git mechanism still behaves as documented, NOT that a gutted `isolated_git_env` would be caught. It was cited as the latter. Mutation-verified: strip loop, chdir, ceiling pin, sessionstart capture and both mirrors each produce a red when removed. Tests: agent 1818 passed; cdk Layer 3 gate suite 22 passed. --- .pre-commit-config.yaml | 22 +- agent/tests/conftest.py | 144 ++++- agent/tests/git_env.py | 293 +++++++-- agent/tests/test_git_fixture_isolation.py | 603 ++++++++++++++++-- .../scripts/check-git-config-clean.test.ts | 83 ++- scripts/check-git-config-clean.mjs | 50 +- 6 files changed, 1066 insertions(+), 129 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8a6974bdf..89ac87712 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -26,12 +26,22 @@ repos: # `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. + # 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. - id: git-config-clean name: shared .git/config uncorrupted (#855) entry: bash -lc 'mise run check:git-config-clean' diff --git a/agent/tests/conftest.py b/agent/tests/conftest.py index dea0770ba..1d52a2682 100644 --- a/agent/tests/conftest.py +++ b/agent/tests/conftest.py @@ -13,8 +13,11 @@ 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 @@ -66,10 +69,18 @@ def _reap_on_hang() -> None: # 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 +# 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): @@ -81,47 +92,95 @@ def pytest_sessionstart(session): 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() + 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 _report_shared_git_config_mutation(session) -> None: - """Fail the session if the shared ``.git/config`` changed during the run (#855). +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) - 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. +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..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, (digest_before, names_before) = _SHARED_GIT_CONFIG - current = fingerprint_git_config(path) - if current is None: - detail = "the file is now unreadable or gone" + 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: - digest_after, names_after = current - if digest_after == digest_before: + if after.digest == before.digest: 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) + 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" @@ -258,7 +317,8 @@ def _isolate_git_location(monkeypatch, tmp_path): 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: + 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 ``, ``cwd=``, ``--local`` and the ``GIT_CONFIG_*`` pins are all bypassed, because @@ -271,6 +331,23 @@ def _isolate_git_location(monkeypatch, tmp_path): ``~/.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 @@ -279,6 +356,11 @@ def _isolate_git_location(monkeypatch, tmp_path): 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") diff --git a/agent/tests/git_env.py b/agent/tests/git_env.py index 530165757..2a33a5cc2 100644 --- a/agent/tests/git_env.py +++ b/agent/tests/git_env.py @@ -28,10 +28,23 @@ import hashlib import os import subprocess +from typing import NamedTuple # 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. +# measure below is bypassed. +# +# Every entry REDIRECTS git to a repository of the environment's choosing, so the +# correct treatment for all of them is removal. ``GIT_CEILING_DIRECTORIES`` is +# deliberately NOT here even though it also affects resolution, because it does the +# opposite thing: it LIMITS the discovery walk. Deleting it widens what git can reach, +# so it is *pinned* below instead of stripped. It was in this tuple until a review +# pointed out that the fixture was therefore removing its own fence. +# +# Mirrored — with the same 7 entries and the same explicit ceiling pin — in +# ``scripts/check-git-config-clean.mjs`` and ``cdk/test/scripts/check-git-config-clean.test.ts``. +# ``tests/test_git_fixture_isolation.py`` asserts the three copies agree, because a +# mirror nobody checks drifts. GIT_LOCATION_VARS: tuple[str, ...] = ( "GIT_DIR", "GIT_COMMON_DIR", @@ -40,7 +53,6 @@ "GIT_OBJECT_DIRECTORY", "GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_PREFIX", - "GIT_CEILING_DIRECTORIES", ) # RFC-2606 reserved TLD: unroutable by construction, and recognisable in a stray @@ -49,11 +61,43 @@ TEST_IDENTITY_NAME = "ABCA Test" TEST_IDENTITY_EMAIL = "abca-test@example.invalid" +# The keys that ARE the leak, and the only ones a mutation is failed on. Kept in step +# with the three rules in ``scripts/check-git-config-clean.mjs`` (Layer 3) so the two +# layers cannot disagree about what counts as corruption. +# +# Scoped deliberately. A whole-file digest is a strictly stronger *detector* but a +# worse *gate*: the shared config is written by routine work too — ``git fetch`` can +# rewrite ``remote.*``, ``git checkout -b``/``push -u`` add ``branch..remote`` +# and ``.merge``, and this suite runs as a pre-push hook while the developer may have +# another worktree open. Failing the suite on that churn produces a red that names no +# fixture and has no remedy, and a gate people learn to re-run past is not a gate. +# Non-signature drift is still reported (see ``conftest``), just not fatal. +SIGNATURE_KEYS: tuple[str, ...] = ( + "core.worktree", + "core.bare", + "user.name", + "user.email", +) + # `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 +class GitConfigLookupError(RuntimeError): + """A repository WAS found, but its shared config could not be resolved or read. + + Distinct from ``None`` on purpose, and the distinction is the whole point: ``None`` + means "there is nothing here to protect" (no ``.git`` anywhere above cwd — the built + container image, for instance), which is a genuine no-risk pass. This exception means + "there is something to protect and the guard could not look at it", which is + indistinguishable from a leak going unnoticed and must be reported loudly. The + earlier version of this module collapsed both into ``None``, so the one state the + detector exists for — a config too broken for git to describe — silently switched it + off. + """ + + def isolated_git_env(repo, base: dict[str, str] | None = None) -> dict[str, str]: """Return an environment in which git cannot reach outside *repo*. @@ -63,12 +107,19 @@ def isolated_git_env(repo, base: dict[str, str] | None = None) -> dict[str, str] *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``. + + ``GIT_CEILING_DIRECTORIES`` is pinned to *repo*'s parent rather than dropped. That + closes the route the stripping does not: a command aimed at a directory which turns + out not to be a repository — ``git -C /scratch config user.email t@t`` — walks + UP, and if ``TMPDIR`` happens to sit inside a checkout on this machine the walk + finds it. The parent, not *repo* itself, so *repo* stays discoverable. """ 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_CEILING_DIRECTORIES": os.path.dirname(os.path.abspath(str(repo))), "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), "GIT_CONFIG_SYSTEM": os.devnull, "GIT_CONFIG_NOSYSTEM": "1", @@ -83,70 +134,222 @@ def isolated_git_env(repo, base: dict[str, str] | None = None) -> dict[str, str] return env +def _ceiling_directories() -> frozenset[str]: + """``GIT_CEILING_DIRECTORIES`` as a set of resolved absolute paths. + + Honoured by the walk below so it stops where git's own discovery would. Without it a + test that chdirs into a throwaway directory would get a verdict that depends on + whether ``TMPDIR`` happens to sit inside somebody's checkout — and that is not + hypothetical: measured on git 2.50.1, a bare ``git config user.email t@t`` run from a + non-repository subdirectory of a repository walks UP and writes the parent's config, + rc 0. That is the #855 leak reached without any ``GIT_DIR`` at all. + + ``realpath``, not ``abspath``, because git resolves ceiling entries through symlinks + (verified: a ceiling spelled through a symlinked ``$HOME`` still stops git's walk). An + ``abspath`` copy silently fails to match on any host where ``$HOME`` or ``TMPDIR`` is a + symlink, which is the shape this repo's own dev hosts have — the mirror would then be + strictly weaker than the thing it claims to mirror. + + NOTE the one deliberate asymmetry with ``scripts/check-git-config-clean.mjs``: its walk + ignores ceilings entirely. That is correct there and wrong here. The gate must find the + repository it is about to let a commit into, so an ambient ceiling must not be able to + make it skip (fail-open); this suite must be reproducible wherever ``TMPDIR`` lands. + """ + raw = os.environ.get("GIT_CEILING_DIRECTORIES", "") + return frozenset(os.path.realpath(part) for part in raw.split(os.pathsep) if part) + + +def find_git_dir(start: str | None = None) -> str | None: + """The gitdir for the tree containing *start*, found WITHOUT consulting any config. + + Mirrors ``findGitDir`` in ``scripts/check-git-config-clean.mjs``; the two must agree, + because a leak the gate refuses at pre-push but the suite does not detect (or vice + versa) is a layer that only appears to be there. + + ``GIT_DIR`` wins when set, because git exports it to hooks in a linked worktree and + it names the exact tree being committed to. Note the asymmetry with the leak itself: + an inherited ``GIT_DIR`` is the hazard for a WRITE aimed somewhere else, and the + authoritative answer for a READ that wants *this* repository. + + Returns None when no ``.git`` exists at or above *start*. + """ + env_dir = os.environ.get("GIT_DIR") + if env_dir: + return os.path.abspath(env_dir) + + ceilings = _ceiling_directories() + # realpath to match the ceiling set above, which git resolves through symlinks. + directory = os.path.realpath(start if start is not None else os.getcwd()) + while True: + if directory in ceilings: + return None + candidate = os.path.join(directory, ".git") + if os.path.isdir(candidate): + return candidate + if os.path.isfile(candidate): + # Linked worktree (or a submodule): a `gitdir: ` pointer, possibly + # relative to the directory holding the `.git` file. + try: + with open(candidate, encoding="utf-8") as handle: + contents = handle.read() + except OSError as exc: + raise GitConfigLookupError(f"cannot read {candidate}: {exc}") from exc + pointed = next( + ( + line.partition(":")[2].strip() + for line in contents.splitlines() + if line.startswith("gitdir:") + ), + "", + ) + if not pointed: + raise GitConfigLookupError( + f"{candidate} is a file with no `gitdir:` line — cannot locate the repository." + ) + return ( + pointed + if os.path.isabs(pointed) + else os.path.normpath(os.path.join(directory, pointed)) + ) + parent = os.path.dirname(directory) + if parent == directory: + return None + directory = parent + + 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. + """Absolute path of the repository-shared ``.git/config``. + + Resolved by walking the filesystem for ``.git`` and following the ``commondir`` + pointer — **no ``git rev-parse`` at all**, because no form of it survives the state + being detected. Measured on git 2.50.1, with ``core.worktree`` set (the key this leak + writes): + + ============================================ ========================== + ``core.worktree`` value ``--git-common-dir`` + ============================================ ========================== + absolute, exists rc 0 + absolute, one missing leaf rc 0 + absolute, two or more missing components rc 128 ``Invalid path`` + relative, missing rc 128 ``cannot chdir`` + ============================================ ========================== + + A deleted pytest ``tmp_path`` is the third shape, so the previous implementation + returned None — "nothing to protect" — in precisely the case this guard exists for: + the pollution disabling its own detector. (``--show-toplevel`` is worse still: it is + *redirected* rather than failing, so it answers confidently with the wrong tree.) The + filesystem walk reads no config, so it answers correctly on a repository too broken + for git to describe. + + Returns None only when there is genuinely nothing to protect: no ``.git`` at or above + cwd, e.g. inside the built container image. Raises ``GitConfigLookupError`` when a + repository IS found but its shared config cannot be resolved — see that class for why + the two cases must not be collapsed. """ - 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: + git_dir = find_git_dir() + if git_dir is None: return None + + # A linked worktree's gitdir holds a `commondir` pointer to the SHARED `.git`, which + # is the file at risk; a per-worktree config would not be. + common_dir = git_dir + commondir_file = os.path.join(git_dir, "commondir") + if os.path.isfile(commondir_file): + try: + with open(commondir_file, encoding="utf-8") as handle: + pointed = handle.read().strip() + except OSError as exc: + raise GitConfigLookupError(f"cannot read {commondir_file}: {exc}") from exc + if pointed: + common_dir = ( + pointed + if os.path.isabs(pointed) + else os.path.normpath(os.path.join(git_dir, pointed)) + ) + config = os.path.join(common_dir, "config") - return config if os.path.isfile(config) else None + if not os.path.isfile(config): + raise GitConfigLookupError( + f"{config} does not exist or is not a file. Every git repository has one, so " + f"{git_dir} is in an unexpected state — check it by hand." + ) + return config + + +class GitConfigFingerprint(NamedTuple): + """What the shared config looked like at one moment. + ``signature`` is what a mutation is *failed* on; ``digest``/``names`` cover the whole + file and are reported but not fatal. Splitting the two is what lets the detector stay + mechanism-independent — it still notices any write, however it arrived — without + turning routine ``branch.*``/``remote.*`` churn into a red suite. + """ + + signature: tuple[tuple[str, tuple[str, ...]], ...] + digest: str + names: frozenset[str] -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. +def fingerprint_git_config(path: str) -> GitConfigFingerprint | None: + """Fingerprint *path*, or None if it cannot be read or parsed. + + Values are captured for the signature keys so a value-only change is caught (a + ``user.email`` overwritten with a fixture's), but only key **names** are ever + reported: a ``.git/config`` can legally hold a remote URL with embedded credentials, + and the report goes to CI logs. """ 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 + entries = _config_entries(path) + if entries is None: + return None + return GitConfigFingerprint( + signature=tuple((key, entries.get(key, ())) for key in SIGNATURE_KEYS), + digest=hashlib.sha256(raw).hexdigest(), + names=frozenset(entries), + ) + + +def signature_keys_changed(before: GitConfigFingerprint, after: GitConfigFingerprint) -> list[str]: + """Signature keys whose value set differs between the two fingerprints.""" + after_values = dict(after.signature) + return [key for key, values in before.signature if after_values.get(key, ()) != values] -def _config_key_names(path: str) -> list[str]: - """Config key names in *path*, via git itself so the parse matches git's.""" +def _config_entries(path: str) -> dict[str, tuple[str, ...]] | None: + """Every key -> values in *path*, parsed by git itself. None if git could not read it. + + ``--list -z`` rather than ``--get-all`` per key, and that choice removes a bug class + rather than guarding against one: ``--get-all`` exits **1** both for "key absent" and + for "file unreadable" (permission-denied is only a stderr *warning*), so a caller that + reads the exit code cannot tell a clean config from one it never opened. + ``--list -z`` has no such overlap — rc 0 with empty output for an empty or + comment-only file, rc 128 for unreadable or malformed. So a non-zero status here is + unambiguously a failure to read, never a clean result. + + Record format is ``key\\nvalue\\0``; a valueless key (an implicit-true bool, written + as a bare ``bare`` under ``[core]``) arrives as ``key\\0`` with no newline. + """ try: result = subprocess.run( - ["git", "config", "--file", path, "--list", "--name-only"], + ["git", "config", "--file", path, "--list", "-z"], capture_output=True, text=True, check=False, timeout=_GIT_TIMEOUT_S, ) except (OSError, subprocess.SubprocessError): - return [] + return None if result.returncode != 0: - return [] - return [line for line in result.stdout.splitlines() if line] + return None + + entries: dict[str, list[str]] = {} + for record in result.stdout.split("\0"): + if not record: + continue + key, separator, value = record.partition("\n") + entries.setdefault(key, []).append(value if separator else "") + return {key: tuple(values) for key, values in entries.items()} diff --git a/agent/tests/test_git_fixture_isolation.py b/agent/tests/test_git_fixture_isolation.py index a9e49fafa..d14e33387 100644 --- a/agent/tests/test_git_fixture_isolation.py +++ b/agent/tests/test_git_fixture_isolation.py @@ -1,11 +1,22 @@ """Tests for the git-fixture isolation guard (#855). -The point of this file is that the guard is *proven live* rather than assumed. The -central test is differential: the **same** git command is run twice, once with an -inherited ``GIT_DIR`` and once through ``isolated_git_env``, and it is asserted to -escape in the first case and be contained in the second. A test that only checked the -contained case would still pass if ``isolated_git_env`` were quietly reduced to -``dict(os.environ)``. +The point of this file is that the guard is *proven live* rather than assumed, and the +honest accounting of which test proves what — corrected after a review measured the +earlier version of this paragraph and found it overclaiming — is: + +* **A gutted helper** (``isolated_git_env`` quietly reduced to ``dict(os.environ)``) is + caught by ``TestIsolatedGitEnv.test_strips_every_location_var`` / + ``test_pins_config_resolution_and_identity``, which pass an explicit ``base=`` and so + do not depend on the ambient environment, and by + ``TestAutouseFixture.test_the_strip_has_teeth_out_of_process``, which is the only test + that exercises the strip on the real ``os.environ`` path. +* **The git mechanism still behaving as documented** — that an inherited ``GIT_DIR`` + really does redirect a write into another repository, which is the premise the whole + guard rests on — is what the differential test proves. It is *not* what catches a + gutted helper: half A leaks because the test sets ``GIT_DIR`` itself, and half B is + contained by the autouse fixture rather than by the function under test. +* **The detector being armed** is ``TestLayer2IsArmed``; **the pre-push gate's rules** + are in ``cdk/test/scripts/check-git-config-clean.test.ts``. Every repository these tests touch is built inside ``tmp_path``. Nothing here writes to the real repository — the "leak" half of the differential test leaks into a @@ -15,20 +26,34 @@ from __future__ import annotations import os +import re import subprocess +import sys +from pathlib import Path from types import SimpleNamespace import pytest from tests.git_env import ( GIT_LOCATION_VARS, + SIGNATURE_KEYS, TEST_IDENTITY_EMAIL, TEST_IDENTITY_NAME, + GitConfigFingerprint, + GitConfigLookupError, + find_git_dir, fingerprint_git_config, isolated_git_env, shared_git_config_path, + signature_keys_changed, ) +# Set in the environment of the nested pytest run spawned by +# ``test_the_strip_has_teeth_out_of_process``. Belt-and-braces against a future edit +# broadening that run's node id into something that re-collects the spawning test and +# forks forever. +_NESTED_RUN_MARKER = "ABCA_855_NESTED_PYTEST" + def _git(repo, *args, env=None, check=True) -> subprocess.CompletedProcess: return subprocess.run( @@ -86,6 +111,45 @@ def test_pins_config_resolution_and_identity(self, tmp_path): assert env["GIT_AUTHOR_EMAIL"] == TEST_IDENTITY_EMAIL assert env["GIT_COMMITTER_NAME"] == TEST_IDENTITY_NAME + def test_pins_the_discovery_ceiling_at_the_parent_not_the_repo(self, tmp_path): + """The ceiling is SET, not stripped — and set one level out. + + Both halves matter. Stripping it would widen discovery (see the note on + ``GIT_LOCATION_VARS``), and pinning it at *repo* itself would make *repo* + undiscoverable, so ``git -C repo status`` in a caller's fixture would start + failing. The parent is the only value that fences the walk without breaking the + sandbox it is fencing. + """ + repo = tmp_path / "sandbox" + env = isolated_git_env(repo, base={}) + + assert env["GIT_CEILING_DIRECTORIES"] == str(tmp_path) + assert env["GIT_CEILING_DIRECTORIES"] != str(repo) + + def test_the_ceiling_actually_stops_a_write_from_escaping_upward(self, tmp_path): + """The ``-C `` route, which stripping ``GIT_DIR`` does not close. + + Measured on git 2.50.1 before this pin existed: from a non-repository directory + inside a repository, a bare ``git config user.email t@t`` walks UP and writes the + *parent's* config, rc 0 — the #855 leak with no ``GIT_DIR`` anywhere. It matters + because ``TMPDIR`` sits inside a checkout on some dev machines, so whether a + fixture leaked depended on the host. + """ + outer = tmp_path / "outer" + (outer / "sub").mkdir(parents=True) + _git(outer, "init", "-q") + outer_config = outer / ".git" / "config" + before = outer_config.read_bytes() + + result = _git(outer / "sub", "config", "user.email", "t@t", check=False) + + assert result.returncode != 0, ( + "a write from a non-repository directory reached a repository above it — " + f"the GIT_CEILING_DIRECTORIES pin is not holding (stdout={result.stdout!r})" + ) + assert "not in a git directory" in result.stderr + assert outer_config.read_bytes() == before + def test_an_inherited_git_dir_escapes_but_isolated_env_contains(self, tmp_path, shared_repo): """The differential test. Same command, two environments, opposite outcomes. @@ -98,6 +162,20 @@ def test_an_inherited_git_dir_escapes_but_isolated_env_contains(self, tmp_path, Half B is the same write through ``isolated_git_env``, which lands in the sandbox's own config and leaves the shared repo byte-identical. + + WHAT THIS DOES AND DOES NOT PROVE, because the first version of this docstring + claimed the wrong thing and the claim was cited as the reason the suite could not + be gutted. What it proves is that the *mechanism* still works as documented: an + inherited ``GIT_DIR`` really does redirect a write into another repository, which + is the premise every layer of this guard is built on, and which git could in + principle change. What it does NOT prove is that ``isolated_git_env`` is doing + anything: half A leaks because *this test* sets ``GIT_DIR`` on the env it passes, + and half B would still be contained if the helper returned a bare + ``dict(os.environ)``, because ``conftest._isolate_git_location`` has already + stripped and pinned that environment. The tests that fail on a gutted helper are + ``test_strips_every_location_var`` / ``test_pins_config_resolution_and_identity`` + (explicit ``base=``, so no fixture underneath them) and + ``TestAutouseFixture.test_the_strip_has_teeth_out_of_process``. """ shared_config = shared_repo / ".git" / "config" before = shared_config.read_bytes() @@ -137,6 +215,12 @@ def test_ambient_location_vars_are_stripped(self): Asserted on ``os.environ`` rather than on a passed-in env because the risk is a fixture that shells out with the *inherited* environment. + + NOTE this passes vacuously wherever the suite normally runs, because those vars + are absent from the parent environment to begin with — it only has teeth in the + environment git gives a hook in a linked worktree. It doubles as the *inner* test + of ``test_the_strip_has_teeth_out_of_process`` below, which supplies exactly that + environment; do not rename it without updating the node id there. """ assert not [var for var in GIT_LOCATION_VARS if var in os.environ] @@ -147,29 +231,249 @@ def test_ambient_config_resolution_is_pinned(self): # Pinned to a per-test tmp path, so a bare `git config user.email` cannot reach # the developer's ~/.gitconfig even from a fixture that forgot isolated_git_env. assert os.environ["GIT_CONFIG_GLOBAL"].endswith(".gitconfig-test") - assert os.path.expanduser("~") not in (os.environ["GIT_CONFIG_GLOBAL"],) + # Inequality against the real path, not a "not under $HOME" containment check: + # TMPDIR is itself under $HOME on this repo's dev hosts (~/.cache/...), so a + # containment form would fail on a correctly pinned value. + assert os.environ["GIT_CONFIG_GLOBAL"] != os.path.expanduser("~/.gitconfig") + + def test_the_process_is_moved_out_of_the_checkout(self, tmp_path): + """Job 3 of the fixture, asserted: cwd is a throwaway and the walk is fenced. + + Stripping ``GIT_DIR`` closes the redirect route and leaves repository DISCOVERY + wide open, and pytest is invoked from ``agent/`` — inside the checkout. So the + author this fixture exists to protect, the one who forgot ``isolated_git_env``, + could still write the shared config with no ``-C``, no ``cwd=`` and no ``GIT_DIR`` + involved at all. + """ + assert Path.cwd() == tmp_path + assert os.environ["GIT_CEILING_DIRECTORIES"] == os.path.realpath(tmp_path.parent) + + # No cwd=, no -C, no isolated env: the shape of the mistake being guarded. + result = subprocess.run( + ["git", "config", "user.email", "t@t"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + + assert result.returncode != 0, ( + "a bare `git config` from a test found a repository to write to — either cwd " + "is back inside the checkout or the discovery ceiling is gone" + ) + assert "not in a git directory" in result.stderr + + def test_the_strip_has_teeth_out_of_process(self, tmp_path): + """The only test that proves the strip RUNS, rather than assuming it. + + ``test_ambient_location_vars_are_stripped`` cannot: an autouse fixture has already + run by the time any test body starts, so nothing in-process can put a ``GIT_DIR`` + in place for it to remove — and with the vars absent from the parent environment + (their normal state outside a hook), deleting the strip loop from ``conftest`` + leaves that test green. Proving it therefore needs an out-of-process run with the + hook environment set, and the inner run's own assertion as the verdict. + """ + if _NESTED_RUN_MARKER in os.environ: + pytest.skip("already inside the nested run — do not recurse") + + # A REAL repository, not a decoy path. Layer 2 resolves the shared config from + # GIT_DIR at session start and reports "could not check" as a session FAILURE, so + # a nonexistent GIT_DIR would fail the inner run for a reason unrelated to the + # strip — and the failure would read as a pass of this test. + decoy = tmp_path / "decoy" + decoy.mkdir() + _git(decoy, "init", "-q") + + # Anti-vacuity for the nested run itself: record what git vars were actually in + # its environment at session start, before any fixture could touch them. + witness = tmp_path / "ambient-at-sessionstart.txt" + plugin = tmp_path / "probe_ambient.py" + plugin.write_text( + "import os, pathlib\n" + "def pytest_sessionstart(session):\n" + f" pathlib.Path({str(witness)!r}).write_text(\n" + ' "|".join(k for k in sorted(os.environ) if k.startswith("GIT_")))\n', + encoding="utf-8", + ) + + child = {k: v for k, v in os.environ.items() if k != "GIT_CEILING_DIRECTORIES"} + child.update( + { + _NESTED_RUN_MARKER: "1", + "PYTHONPATH": os.pathsep.join( + part for part in (str(tmp_path), os.environ.get("PYTHONPATH", "")) if part + ), + # Exactly what git exports to a hook in a linked worktree — four of the + # seven, so the strip has real work to do. + "GIT_DIR": str(decoy / ".git"), + "GIT_COMMON_DIR": str(decoy / ".git"), + "GIT_WORK_TREE": str(decoy), + "GIT_INDEX_FILE": str(decoy / ".git" / "index"), + } + ) + + inner = ( + f"{Path(__file__).resolve()}" + "::TestAutouseFixture::test_ambient_location_vars_are_stripped" + ) + result = subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "probe_ambient", inner], + cwd=str(Path(__file__).resolve().parents[1]), + env=child, + capture_output=True, + text=True, + check=False, + timeout=90, + ) + + observed = witness.read_text(encoding="utf-8") if witness.exists() else "" + assert "GIT_DIR" in observed, ( + "the nested session did not start with an ambient GIT_DIR, so whatever it " + f"reported says nothing about the strip. Saw: {observed!r}\n" + f"{result.stdout[-2000:]}" + ) + assert result.returncode == 0, ( + "the autouse fixture failed to strip the repo-location vars git exports to " + f"hooks — this is the #855 leak, live.\n{result.stdout[-4000:]}\n" + f"{result.stderr[-2000:]}" + ) + assert "1 passed" in result.stdout, ( + f"expected the inner run to execute exactly one test:\n{result.stdout[-2000:]}" + ) + + +class TestCrossCopyParity: + """``GIT_LOCATION_VARS`` exists three times; nothing enforced that they agreed. + + Python (here), the pre-push gate (``scripts/check-git-config-clean.mjs``) and that + gate's own suite (``cdk/test/scripts/check-git-config-clean.test.ts``) each carry a + copy, and they had in fact drifted — the review that prompted this test found 8/7/8 + entries across the three. Sibling of ``test_signature_keys_match_the_layer_3_gate``, + which does the same job for ``SIGNATURE_KEYS``. + """ + + MIRRORS = ( + ("scripts", "check-git-config-clean.mjs"), + ("cdk", "test", "scripts", "check-git-config-clean.test.ts"), + ) + + @pytest.mark.parametrize("parts", MIRRORS, ids=lambda parts: parts[-1]) + def test_the_mirrors_list_the_same_vars(self, parts): + mirror = Path(__file__).resolve().parents[2].joinpath(*parts) + if not mirror.is_file(): + # Only `agent/` is bundled into the container image. + pytest.skip(f"{mirror} not present in this tree") + + text = mirror.read_text(encoding="utf-8") + block = re.search(r"GIT_LOCATION_VARS\s*=\s*\[(.*?)\]", text, re.DOTALL) + assert block is not None, f"no GIT_LOCATION_VARS array found in {mirror}" + mirrored = set(re.findall(r"'(GIT_[A-Z_]+)'", block.group(1))) + + assert mirrored == set(GIT_LOCATION_VARS), ( + f"{mirror} lists a different set of repo-location vars than " + "agent/tests/git_env.py. Note GIT_CEILING_DIRECTORIES must NOT be in any of " + "them — it is pinned, not stripped, because deleting it WIDENS discovery." + ) + + @pytest.mark.parametrize("parts", MIRRORS, ids=lambda parts: parts[-1]) + def test_no_mirror_strips_the_discovery_ceiling(self, parts): + """The specific drift that motivated this: the fence in the strip list. + + Checked separately from set equality because this is the failure with teeth — + equality would also flag a harmless reordering, and a reader hitting a red needs + to know which of the two happened. + """ + mirror = Path(__file__).resolve().parents[2].joinpath(*parts) + if not mirror.is_file(): + pytest.skip(f"{mirror} not present in this tree") + + text = mirror.read_text(encoding="utf-8") + block = re.search(r"GIT_LOCATION_VARS\s*=\s*\[(.*?)\]", text, re.DOTALL) + assert block is not None, f"no GIT_LOCATION_VARS array found in {mirror}" + + assert "GIT_CEILING_DIRECTORIES" not in block.group(1), ( + f"{mirror} deletes GIT_CEILING_DIRECTORIES along with the redirect vars. That " + "removes git's own fence: the walk can then climb out of wherever it started " + "and find a repository above it." + ) + assert "GIT_CEILING_DIRECTORIES" in text, ( + f"{mirror} neither strips nor sets GIT_CEILING_DIRECTORIES, so its discovery " + "walk is unfenced — see git_env._ceiling_directories for the measured escape." + ) class TestSharedConfigResolution: - def test_resolves_through_git_common_dir_not_show_toplevel(self, tmp_path, monkeypatch): - """``core.worktree`` must not be able to disable the detector. - - This is the failure the resolution choice exists to avoid: ``core.worktree`` - redirects ``--show-toplevel``, so a detector built on it computes a path that - does not exist in a polluted repo and reports "nothing to protect" — the - pollution switching off its own alarm. ``--git-common-dir`` is answered from the - gitdir alone. + """Resolution must survive a repository too broken for git to describe. + + These tests pick the corruption shape adversarially. An earlier version of this class + set ``core.worktree`` to a directory that EXISTS — which is the one polluted shape + where ``git rev-parse --git-common-dir`` still succeeds — so it confirmed the design + against the benign look-alike and never exercised the case the guard is for. + """ + + @staticmethod + def _write_config(repo, section, lines) -> None: + """Append a config section by hand. + + Not ``git config``: once ``core.worktree`` names a missing path, git refuses to + operate in that repository at all — including refusing to write or unset the very + key that broke it. + """ + config = repo / ".git" / "config" + body = "".join(f"\t{line}\n" for line in lines) + with config.open("a", encoding="utf-8") as handle: + handle.write(f"[{section}]\n{body}") + + def test_resolves_when_rev_parse_cannot_answer_at_all(self, tmp_path, monkeypatch): + """The differential test for resolution, on the shape that actually bites. + + A deleted pytest ``tmp_path`` leaves ``core.worktree`` naming an absolute path + with two or more missing components, and in that state EVERY ``git rev-parse`` + form aborts rc 128 ``fatal: Invalid path`` — including ``--git-common-dir``, which + this function used to be built on and which therefore reported "nothing to + protect" exactly when there was something to protect. """ repo = tmp_path / "polluted" repo.mkdir() _git(repo, "init", "-q") + self._write_config(repo, "core", [f"worktree = {tmp_path / 'gone' / 'deeper' / 'tmp'}"]) + + monkeypatch.chdir(repo) + + # The rejected approaches, asserted to be unusable rather than assumed to be. + for form in (["--show-toplevel"], ["--path-format=absolute", "--git-common-dir"]): + probe = subprocess.run( + ["git", "rev-parse", *form], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + assert probe.returncode != 0, ( + f"`git rev-parse {' '.join(form)}` unexpectedly succeeded on a repo with a " + "missing core.worktree — if this now works, re-check whether the " + "filesystem walk is still needed" + ) + + # The chosen approach: reads no config, so it still answers. + assert shared_git_config_path() == str(repo / ".git" / "config") + + def test_resolves_when_core_worktree_redirects_to_a_real_directory(self, tmp_path, monkeypatch): + """The other shape: ``core.worktree`` names a directory that exists. + + Here git answers happily and *wrongly* — ``--show-toplevel`` reports the redirect + target. This is the shape seen in the real recurrences (it points at a sibling + worktree), and the dangerous one, because nothing else in the tree complains. + """ + repo = tmp_path / "redirected" + repo.mkdir() + _git(repo, "init", "-q") elsewhere = tmp_path / "elsewhere" elsewhere.mkdir() _git(repo, "config", "--local", "core.worktree", str(elsewhere)) monkeypatch.chdir(repo) - # The rejected approach: redirected away from the real repo. toplevel = subprocess.run( ["git", "rev-parse", "--show-toplevel"], capture_output=True, @@ -177,31 +481,72 @@ def test_resolves_through_git_common_dir_not_show_toplevel(self, tmp_path, monke check=False, timeout=60, ) - assert toplevel.stdout.strip() != str(repo) + assert toplevel.stdout.strip() == str(elsewhere), "expected git to be redirected" - # The chosen approach: still the real shared config. assert shared_git_config_path() == str(repo / ".git" / "config") + def test_resolves_to_the_SHARED_config_from_a_linked_worktree(self, shared_repo, monkeypatch): + """A linked worktree's own gitdir is not the file at risk; the shared one is. + + Resolution has to follow the ``commondir`` pointer, or the guard would fingerprint + a per-worktree config that no leak ever touches — protection that looks present and + is not. This is also the only configuration in which git exports ``GIT_DIR`` to + hooks, i.e. the only one in which the leak happens at all, so it is the shape the + guard runs in for real. + """ + worktree_gitdir = shared_repo / ".git" / "worktrees" / "wt" + assert (worktree_gitdir / "commondir").is_file(), "expected a commondir pointer" + + # GIT_DIR honoured exactly as git's own hooks receive it. + monkeypatch.setenv("GIT_DIR", str(worktree_gitdir)) + assert shared_git_config_path() == str(shared_repo / ".git" / "config") + def test_returns_none_outside_a_repository(self, tmp_path, monkeypatch): outside = tmp_path / "not-a-repo" outside.mkdir() monkeypatch.chdir(outside) - # GIT_CEILING_DIRECTORIES stops discovery from walking up into whatever - # repository happens to contain tmp_path on this machine. + # GIT_CEILING_DIRECTORIES stops the walk from climbing into whatever repository + # happens to contain tmp_path on this machine. Honoured by `find_git_dir` for the + # same reason git honours it. monkeypatch.setenv("GIT_CEILING_DIRECTORIES", str(tmp_path)) assert shared_git_config_path() is None + def test_raises_rather_than_reporting_nothing_when_the_config_is_missing( + self, tmp_path, monkeypatch + ): + """A repo whose config vanished is a failure to look, not a no-risk pass. + + The two must not collapse into ``None``: ``None`` makes the session hook return + silently, which is how the detector previously switched itself off. + """ + repo = tmp_path / "no-config" + repo.mkdir() + _git(repo, "init", "-q") + (repo / ".git" / "config").unlink() + monkeypatch.chdir(repo) + + with pytest.raises(GitConfigLookupError, match="does not exist"): + shared_git_config_path() + class TestFingerprint: + """Whole-file detection, signature-scoped judgement. + + The split is the design: ``digest``/``names`` notice ANY write however it arrived + (mechanism-independence is what four file-scoped fixes lacked), while ``signature`` + decides what is worth failing a suite over. Both halves are tested here, including + the case that motivated the split — routine ``remote.*`` churn. + """ + @staticmethod - def _fingerprint(config) -> tuple[str, frozenset[str]]: + def _fingerprint(config) -> GitConfigFingerprint: """``fingerprint_git_config`` narrowed to non-None. It returns ``None`` for an unreadable path — a real case, covered by its own - test below — so unpacking the result directly is a type error (ty - ``not-iterable``). Asserting here keeps that contract visible instead of - annotating it away, and a None would fail the assertion rather than raise an - opaque unpacking error further down. + test below — so using the result directly is a type error (ty + ``possibly-unbound-attribute``). Asserting here keeps that contract visible + instead of annotating it away, and a None fails with a readable message rather + than an opaque ``AttributeError`` further down. """ result = fingerprint_git_config(str(config)) assert result is not None, f"expected {config} to be readable" @@ -213,12 +558,12 @@ def test_detects_an_added_key_and_names_it(self, tmp_path): _git(repo, "init", "-q") config = repo / ".git" / "config" - digest_before, names_before = self._fingerprint(config) + before = self._fingerprint(config) _git(repo, "config", "--local", "core.worktree", str(tmp_path)) - digest_after, names_after = self._fingerprint(config) + after = self._fingerprint(config) - assert digest_after != digest_before - assert names_after - names_before == {"core.worktree"} + assert after.digest != before.digest + assert after.names - before.names == {"core.worktree"} def test_detects_a_value_change_without_capturing_the_value(self, tmp_path): repo = tmp_path / "repo" @@ -233,19 +578,147 @@ def test_detects_a_value_change_without_capturing_the_value(self, tmp_path): url_with_credential = "https://user:placeholder@example.invalid/repo.git" _git(repo, "config", "--local", "remote.origin.url", "https://example.invalid/a.git") - digest_before, names_before = self._fingerprint(config) + before = self._fingerprint(config) _git(repo, "config", "--local", "remote.origin.url", url_with_credential) - digest_after, names_after = self._fingerprint(config) + after = self._fingerprint(config) - assert digest_after != digest_before, "a value-only change must still be detected" - assert names_after == names_before, "no key was added, so the name set is stable" + assert after.digest != before.digest, "a value-only change must still be detected" + assert after.names == before.names, "no key was added, so the name set is stable" # The reason names-not-values: this data is printed into CI logs on failure. - assert url_with_credential not in str(names_after) + assert url_with_credential not in str(after.names) + assert url_with_credential not in str(after.signature) + + def test_routine_remote_churn_is_seen_but_is_not_a_signature_change(self, tmp_path): + """The false positive the signature scoping exists to prevent. + + ``git fetch`` rewrites ``remote.*`` and ``push -u`` adds ``branch..remote``, + and this suite runs as a pre-push hook while other worktrees may be active. Under + a whole-file verdict that churn reds the suite with no fixture to blame and no + remedy to offer — and a gate people learn to re-run past has stopped being a gate. + """ + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + + before = self._fingerprint(config) + _git(repo, "config", "--local", "branch.main.remote", "origin") + after = self._fingerprint(config) + + assert after.digest != before.digest, "the write must still be DETECTED" + assert signature_keys_changed(before, after) == [], "but it must not be JUDGED a leak" + + @pytest.mark.parametrize( + ("key", "value"), + [ + ("core.worktree", "/somewhere/else"), + ("core.bare", "true"), + ("user.name", "t"), + ("user.email", "t@t"), + ], + ) + def test_every_signature_key_is_reported_when_it_changes(self, tmp_path, key, value): + """Parametrised over the whole tuple so adding a key without wiring it is caught. + + ``core.bare`` in particular is written by git as a bare ``bare`` under + ``[core]`` — a valueless key in ``--list -z`` output — so it is the one most likely + to be silently dropped by a parser change. + """ + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + + before = self._fingerprint(config) + _git(repo, "config", "--local", key, value) + after = self._fingerprint(config) + + assert signature_keys_changed(before, after) == [key] + + def test_a_signature_key_being_REMOVED_is_a_change(self, tmp_path): + """Direction matters: the leak also manifests as an identity being replaced. + + A comparison that only looked at the *after* side's keys would miss a removal, and + ``user.email`` disappearing is how a developer's configured identity gets lost. + """ + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + _git(repo, "config", "--local", "user.email", "real@dev.example") + + before = self._fingerprint(config) + _git(repo, "config", "--local", "--unset", "user.email") + after = self._fingerprint(config) + + assert signature_keys_changed(before, after) == ["user.email"] + + def test_signature_keys_match_the_layer_3_gate(self): + """Cross-layer drift guard for a claim that is otherwise only a comment. + + ``git_env.SIGNATURE_KEYS`` documents itself as kept in step with + ``scripts/check-git-config-clean.mjs``. Nothing enforced that, and an unenforced + parity claim is the kind that quietly stops being true — a key added to one layer + only would mean pre-push refuses a state the suite ignores, or the reverse. + """ + gate = Path(__file__).resolve().parents[2] / "scripts" / "check-git-config-clean.mjs" + if not gate.is_file(): + # `scripts/` is not bundled into the agent container image; only `agent/` is. + pytest.skip(f"{gate} not present in this tree") + + quoted = set(re.findall(r"'((?:core|user)\.[A-Za-z]+)'", gate.read_text(encoding="utf-8"))) + assert quoted == set(SIGNATURE_KEYS), ( + "Layer 2 (this suite) and Layer 3 (the pre-push gate) disagree about which keys " + "are the #855 signature. Update both, or the layers protect different things." + ) def test_returns_none_for_an_unreadable_path(self, tmp_path): assert fingerprint_git_config(str(tmp_path / "nope" / "config")) is None +class TestLayer2IsArmed: + """Is the detector actually watching THIS run, or silently disarmed? + + ``TestMutationReport`` below exercises the report's decision logic by injecting + fingerprints, which is the right way to test the branches but says nothing about + whether ``pytest_sessionstart`` captured anything in the first place. It has three + early ``return`` paths, and two of them — ``path is None`` ("nothing to protect") and + a ``GitConfigLookupError`` — turn the whole layer off for the session while leaving + every test in this file green. That is a failure mode with no symptom: the suite would + report a clean bill of health on a leak it never looked for. + + So this asserts on the module-level state left behind by the real session start. + """ + + def test_the_session_hook_captured_a_fingerprint_when_run_in_a_checkout(self): + from tests import conftest + + # Anchored at this file, NOT cwd: the autouse fixture has moved the process to a + # tmp_path fenced by GIT_CEILING_DIRECTORIES, so a cwd-relative lookup correctly + # finds nothing and would make this test vacuous. + if find_git_dir(str(Path(__file__).resolve().parent)) is None: + # The genuine no-risk case: no `.git` above the tests, e.g. the agent + # container image, where `agent/` is copied in without the repository. + pytest.skip("not running inside a git checkout — there is nothing to protect") + + assert conftest._SHARED_GIT_CONFIG_UNCHECKED is None, ( + "Layer 2 found a repository but could not fingerprint its shared config, so " + "this whole run proves nothing about whether a fixture leaked into it: " + f"{conftest._SHARED_GIT_CONFIG_UNCHECKED}" + ) + assert conftest._SHARED_GIT_CONFIG is not None, ( + "Layer 2 is DISARMED for this session: pytest_sessionstart took an early " + "return even though there is a checkout above these tests, so the " + "before/after comparison at session finish will compare nothing." + ) + + path, fingerprint = conftest._SHARED_GIT_CONFIG + assert os.path.isfile(path), f"{path} was fingerprinted but is not a file" + # A fingerprint of an empty/unparsed file would still be a truthy tuple, so check + # the digest is a real sha256 rather than merely present. + assert len(fingerprint.digest) == 64 + + class TestMutationReport: """The session-level detector's decision logic (``conftest``, Layer 2). @@ -261,10 +734,11 @@ def _repo_with_config(tmp_path): _git(repo, "init", "-q") return repo, repo / ".git" / "config" - def _run_report(self, monkeypatch, config, fingerprint): + def _run_report(self, monkeypatch, config, fingerprint, *, unchecked=None): from tests import conftest monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", (str(config), fingerprint)) + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG_UNCHECKED", unchecked) session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) conftest._report_shared_git_config_mutation(session) return session @@ -297,10 +771,67 @@ def test_is_inert_when_there_was_nothing_to_protect(self, monkeypatch): from tests import conftest monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", None) + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG_UNCHECKED", None) session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) conftest._report_shared_git_config_mutation(session) assert session.exitstatus == pytest.ExitCode.OK + def test_FAILS_when_the_config_could_not_be_checked_at_all(self, monkeypatch, capsys): + """The counterpart to the test above, and the distinction the whole guard rests on. + + "Nothing to protect" is a pass; "could not look at the thing I am protecting" is a + failure. Collapsing the two into a silent ``return`` is what let a repository too + broken for ``git rev-parse`` to describe — the exact state this guard exists for — + switch the guard off and report success. + """ + from tests import conftest + + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", None) + monkeypatch.setattr( + conftest, "_SHARED_GIT_CONFIG_UNCHECKED", "/x/.git/config does not exist" + ) + session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) + conftest._report_shared_git_config_mutation(session) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + message = capsys.readouterr().err + assert "COULD NOT CHECK" in message + assert "/x/.git/config does not exist" in message + # Must point at the tool that can diagnose it, not just complain. + assert "mise run check:git-config-clean" in message + + def test_does_not_fail_the_session_on_non_signature_drift(self, tmp_path, monkeypatch, capsys): + """Detected, reported, not fatal — the middle verdict. + + ``branch.main.remote`` is what ``git push -u`` writes, from another worktree, while + this suite is running as a pre-push hook. Failing on it produces a red naming no + fixture and offering no remedy. + """ + repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + _git(repo, "config", "--local", "branch.main.remote", "origin") + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.OK + message = capsys.readouterr().err + assert "no #855 signature key did" in message + assert "branch.main.remote" in message + assert "SHARED GIT CONFIG MUTATED" not in message + + def test_names_the_signature_key_that_moved(self, tmp_path, monkeypatch, capsys): + """The failure has to say WHICH key, or the remedy is guesswork.""" + repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + _git(repo, "config", "--local", "core.worktree", str(tmp_path)) + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + message = capsys.readouterr().err + assert "signature key(s) changed: core.worktree" in message + assert f"git config --file {config} --unset-all core.worktree" in message + def test_reports_a_config_that_vanished(self, tmp_path, monkeypatch, capsys): _repo, config = self._repo_with_config(tmp_path) fingerprint = fingerprint_git_config(str(config)) @@ -309,4 +840,4 @@ def test_reports_a_config_that_vanished(self, tmp_path, monkeypatch, capsys): session = self._run_report(monkeypatch, config, fingerprint) assert session.exitstatus == pytest.ExitCode.TESTS_FAILED - assert "unreadable or gone" in capsys.readouterr().err + assert "unreadable, gone, or no longer parses" in capsys.readouterr().err diff --git a/cdk/test/scripts/check-git-config-clean.test.ts b/cdk/test/scripts/check-git-config-clean.test.ts index 4cec828a6..e700e5125 100644 --- a/cdk/test/scripts/check-git-config-clean.test.ts +++ b/cdk/test/scripts/check-git-config-clean.test.ts @@ -70,7 +70,16 @@ function realSharedConfigPath(): string { return path.join(commonDir, 'config'); } -/** Repo-location vars — mirrors `GIT_LOCATION_VARS` in `agent/tests/git_env.py`. */ +/** + * Repo-location vars — mirrors `GIT_LOCATION_VARS` in `agent/tests/git_env.py`, and + * `agent/tests/test_git_fixture_isolation.py` asserts the two lists (plus the copy in + * the script itself) stay identical. + * + * Every entry REDIRECTS git to a repository of the environment's choosing, so removal + * is the right treatment for all of them. `GIT_CEILING_DIRECTORIES` is deliberately NOT + * here: it does the opposite, LIMITING the discovery walk, so deleting it widens what + * git can reach. It is pinned below instead. + */ const GIT_LOCATION_VARS = [ 'GIT_DIR', 'GIT_COMMON_DIR', @@ -79,7 +88,6 @@ const GIT_LOCATION_VARS = [ 'GIT_OBJECT_DIRECTORY', 'GIT_ALTERNATE_OBJECT_DIRECTORIES', 'GIT_PREFIX', - 'GIT_CEILING_DIRECTORIES', ]; /** An environment in which git cannot reach outside `repo`. */ @@ -91,6 +99,10 @@ function isolatedGitEnv(repo: string): NodeJS.ProcessEnv { ...env, HOME: repo, XDG_CONFIG_HOME: repo, + // The route stripping does NOT close: a git command aimed at a directory that turns + // out not to be a repository walks UP, and TMPDIR sits inside a checkout on some dev + // machines. The PARENT, not `repo` itself, so `repo` stays discoverable. + GIT_CEILING_DIRECTORIES: path.dirname(path.resolve(repo)), GIT_CONFIG_GLOBAL: path.join(repo, '.gitconfig-test'), GIT_CONFIG_SYSTEM: '/dev/null', GIT_CONFIG_NOSYSTEM: '1', @@ -346,6 +358,44 @@ describe('check-git-config-clean', () => { expect(result.status).toBe(2); expect(result.stderr).toContain('does not exist'); }); + + // Root bypasses file permissions, so `chmod 000` is still readable there and the + // scenario cannot be constructed. Skipped rather than faked: a test that asserted + // this via a mock would pass whether or not the real gate handles it. + const testUnlessRoot = process.getuid?.() === 0 ? test.skip : test; + + testUnlessRoot('an UNREADABLE .git/config exits 2 rather than reporting clean', () => { + // The false pass this file exists to prevent, and one the gate really had: + // `git config --file --get-all ` exits **1** with only a stderr + // *warning* — byte-identical to git's "key not present" — so every rule came back + // empty, no rule fired, and the gate printed `OK — 4 rule(s) ... clean` and exited + // 0 about a file it had never opened. Worse than a missed detection: it is an + // affirmative all-clear on an unexamined config. + // + // Asserts the CONTRACT (exit 2, no all-clear), not which internal check fired, and + // that wording is deliberate: mutation-tested, the gate turns out to have two + // independent stops for this input — the up-front `readFileSync` proof and + // `configValues` refusing to read rc 1 as "absent" while stderr is non-empty. + // Deleting either one alone still leaves this test green; deleting both returns + // exit 0 with `OK — 4 rule(s)`, which is what it was measured against. + const repo = freshRepo('unreadable-config'); + const configPath = path.join(repo, '.git', 'config'); + appendConfig(repo, 'user', ['email = t@t']); // corruption that MUST NOT be missed + fs.chmodSync(configPath, 0o000); + + try { + const result = runGate(repo); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('cannot read'); + expect(result.stderr).toContain(configPath); + // The specific regression: no all-clear may be printed about an unread file. + expect(result.stdout).not.toContain('OK —'); + } finally { + // Restored so the scratch teardown is not fighting permissions. + fs.chmodSync(configPath, 0o600); + } + }); }); describe('an inherited GIT_DIR — the hook environment', () => { @@ -370,14 +420,37 @@ describe('check-git-config-clean', () => { // Asserted because the isolation is what stops these tests from re-creating // the bug while setting up: with a GIT_DIR inherited from the pre-push hook, // `git init ` re-inits the real repository. - const env = isolatedGitEnv('/somewhere'); + const env = isolatedGitEnv('/somewhere/repo'); for (const key of GIT_LOCATION_VARS) { expect(env[key]).toBeUndefined(); } - expect(env.HOME).toBe('/somewhere'); - expect(env.GIT_CONFIG_GLOBAL).toBe('/somewhere/.gitconfig-test'); + expect(env.HOME).toBe('/somewhere/repo'); + expect(env.GIT_CONFIG_GLOBAL).toBe('/somewhere/repo/.gitconfig-test'); expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + // SET, not stripped — and one level OUT, so `repo` itself stays discoverable while + // the walk can never climb above it. + expect(env.GIT_CEILING_DIRECTORIES).toBe('/somewhere'); + }); + + test('the discovery ceiling stops a write from escaping into a repo above', () => { + // The route stripping does not close, and the one this suite is itself exposed to: + // `scratch` is under TMPDIR, which is inside a checkout on some machines. Measured + // on git 2.50.1 without the pin, this write lands in the parent repo's config, rc 0. + const outer = freshRepo('ceiling-outer'); + const sub = path.join(outer, 'not-a-repo'); + fs.mkdirSync(sub, { recursive: true }); + const outerConfig = path.join(outer, '.git', 'config'); + const before = fs.readFileSync(outerConfig); + + const result = spawnSync('git', ['-C', sub, 'config', 'user.email', 't@t'], { + encoding: 'utf-8', + env: isolatedGitEnv(sub), + }); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain('not in a git directory'); + expect(fs.readFileSync(outerConfig)).toEqual(before); }); test('the real repository config is byte-identical after this suite has run', () => { diff --git a/scripts/check-git-config-clean.mjs b/scripts/check-git-config-clean.mjs index 1bc6460f4..9a2ba1b58 100644 --- a/scripts/check-git-config-clean.mjs +++ b/scripts/check-git-config-clean.mjs @@ -56,13 +56,33 @@ * actually leaves behind), rev-parse aborts with `fatal: Invalid path`, so a check * built on it can only report "could not check" and never name the cause. Walking * the filesystem for `.git` is deterministic and reads no config, so it answers - * correctly on a repository too broken for git to describe. The - * `.pre-commit-config.yaml` entry for this hook is likewise the only one in the file - * that does NOT `cd "$(git rev-parse --show-toplevel)"`. + * correctly on a repository too broken for git to describe. + * + * WHAT THE HOOK RUNNER DOES, MEASURED (prek 0.4.8, git 2.50.1) — because the reach of + * this gate depends on it and the first version of this comment guessed wrong: + * + * - prek chdirs a hook to the repository root ITSELF, derived from its own + * `git rev-parse --show-toplevel`. So omitting the `cd "$(git rev-parse + * --show-toplevel)"` prologue that every other hook in `.pre-commit-config.yaml` + * carries buys this hook nothing — it is already standing where that prologue + * would have put it. The prologue is omitted anyway (one less dependency on a git + * command that can lie), but it is NOT what protects the check. What protects the + * check is resolution-by-filesystem plus reading through `--file` from cwd `/`. + * - When `core.worktree` names a path with two or more missing components, prek + * ABORTS at startup on that same rc-128 rev-parse, before invoking any hook. The + * gate therefore cannot be what catches that shape — but nothing slips through + * either, because plain `git commit` fails identically. That shape is + * self-announcing; every git command in the tree refuses. + * - When `core.worktree` names a path that EXISTS — the shape actually seen in + * #622/#720/#855, pointing at a sibling worktree — git answers normally, prek runs, + * and this gate fires with the right config and the right diagnosis. That is the + * silent-and-dangerous case, and it is the one covered. * * Reads are delegated to `git config --file ` so the parse is git's own, and * because `--file` involves no repository discovery — the one git operation this - * corruption cannot reach. + * corruption cannot reach. Readability is proved with a direct `readFileSync` first, + * because `--get-all` reports an unreadable file and an absent key with the same + * exit status (see `configValues`). * * Exit codes: 0 clean · 1 corruption found (with remedy) · 2 could not check. * Case 2 is a failure, not a pass: an unreadable config or a git that cannot answer @@ -209,14 +229,32 @@ function sharedConfigPath() { + 'this repository is in an unexpected state — check it by hand.', ); } + + // Prove the file is READABLE before any rule is allowed to report on it. Without + // this the gate has a false pass: `git config --file --get-all ` + // exits **1** with only a stderr *warning*, which is byte-identical to git's + // "key not present", so every rule would come back empty and the summary would print + // `OK — 4 rule(s) clean` about a file it never opened. Reproduced with `chmod 000`. + // Exit 2 is the documented verdict for that state (see the header), not exit 0. + try { + readFileSync(config); + } catch (err) { + bail(`cannot read ${config} (${err.message}). Refusing to report on a config that ` + + 'could not be opened — an unreadable config is exactly the state in which a ' + + 'leak would go unnoticed.'); + } return config; } /** All values of `key` in `configPath` (empty array when unset). */ function configValues(configPath, key) { const result = gitConfigRead(['config', '--file', configPath, '--get-all', key]); - // rc 1 is git's "key not present" — the normal, clean case. - if (result.status === 1) return []; + // rc 1 is git's "key not present" — the normal, clean case — but ONLY when git had + // nothing to complain about. git also exits 1 when it could not access the file at + // all, emitting `warning: unable to access ...` and no fatal. `sharedConfigPath` + // already proved readability, so this is the belt to that braces: a non-empty stderr + // on an rc 1 means the read did not happen and "no values" is not a finding. + if (result.status === 1 && result.stderr.trim() === '') return []; if (result.status !== 0) { bail( `cannot read ${key} from ${configPath} ` From c1a6231539ee6df98bcfe9667a12a6f6b79df22f Mon Sep 17 00:00:00 2001 From: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:53:57 +0000 Subject: [PATCH 3/3] test(855): land the rest of isadeks' non-blocking review (#856) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine items from the review that were acknowledged but not in 9ef7068d. Every guard here was mutation-tested, because each one's failure mode is a silent pass. Mechanism prose, corrected against measurement rather than reworded: * `git_env.py` / `conftest.py` / `test_git_fixture_isolation.py` claimed git exports `GIT_DIR`/`GIT_COMMON_DIR` to hooks and that both are unset in a normal checkout. Measured on git 2.50.1 by dumping the hook environment in both shapes: `GIT_DIR` is the SOLE differentiator, `GIT_COMMON_DIR` is exported in NEITHER, and `GIT_INDEX_FILE`/`GIT_PREFIX` in BOTH. The 7-var strip list is justified by what the vars do, not by who sets them. The gate: * Exit codes 0/1/2 were bare literals documented only in prose. Now exported as `EXIT_CLEAN`/`EXIT_PROBLEMS_FOUND`/`EXIT_COULD_NOT_CHECK` behind an ESM main-module guard, so importing them does not run the gate. The TS suite parses them out of the source (a real import would execute the gate against whatever repo jest runs in) and pins the numbers once — otherwise both sides would follow a renumbering and stay green. * `bash -lc` -> `bash -c` on the hook entry. A login shell sources the profile before the command, and this is the one hook with no `cd "$(git rev-parse --show-toplevel)"` prologue to undo a profile `cd`. Tests that now construct what they assert: * Rule 3 is the only rule matching against a LIST, and the table sampled 2 of 7 reserved suffixes and 1 of 5 fixture names — the other ten entries could be deleted with the suite green. All twelve are covered now. * A genuine bare repository must NOT be flagged (the gated `core.bare` rule from 9ef7068d had no test; ungating it now reds this test), and the skip is asserted to appear in the printed rule count so the fix cannot become a silent one. * A multi-problem case, so the `--get-all` multi-value loop and the `Found N problem(s)` plural finally execute. Downgrading `--get-all` to `--get` now reds it. * Layer 3's WIRING is asserted: the hook stanza is parsed (not grepped) for both stages and the mise task, and the task is checked to resolve to this script. Deleting the stanza previously disarmed the layer with all tests green. * `TestCrossCopyParity` now covers the identity, not just the var list. The fixture name has a fourth encoding — lowercased inside the gate's `FIXTURE_NAMES` — and the email's suffix must appear in `RESERVED_EMAIL_SUFFIXES`. Drift there keeps Layer 1 writing a value Layer 3 no longer recognises, with both suites green. * `test_registry_loader.py`: `_git` was `check=False` with no call site reading `returncode`, and every assertion in the class is "the secret is NOT in the staged diff" — which an empty diff satisfies. Proven vacuous: break the initial commit and the test still passes. `check=True` by default now, the one legitimately-failing call (`git add` on a skip-worktree path) asserts its refusal, and a control file proves `git add -u` actually staged something. Docs: * `CONTRIBUTING.md` did not mention the gate at all; the mirror is regenerated. Refs #855 --- .pre-commit-config.yaml | 13 +- CONTRIBUTING.md | 1 + agent/tests/conftest.py | 7 +- agent/tests/git_env.py | 30 ++- agent/tests/test_git_fixture_isolation.py | 76 +++++- agent/tests/test_registry_loader.py | 50 +++- .../scripts/check-git-config-clean.test.ts | 227 ++++++++++++++-- .../docs/developer-guide/Contributing.md | 1 + scripts/check-git-config-clean.mjs | 249 ++++++++++++------ 9 files changed, 541 insertions(+), 113 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 89ac87712..26c5a68c7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -42,9 +42,20 @@ repos: # 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 -lc 'mise run check:git-config-clean' + entry: bash -c 'mise run check:git-config-clean' language: system pass_filenames: false always_run: true diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 672bdc1a9..e97dd3e0c 100644 --- a/CONTRIBUTING.md +++ b/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`). diff --git a/agent/tests/conftest.py b/agent/tests/conftest.py index 1d52a2682..45cb86901 100644 --- a/agent/tests/conftest.py +++ b/agent/tests/conftest.py @@ -323,8 +323,11 @@ def _isolate_git_location(monkeypatch, tmp_path): 1. **Strip the repo-LOCATION vars.** While any of them is set, ``git -C ``, ``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/``. + ``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 diff --git a/agent/tests/git_env.py b/agent/tests/git_env.py index 2a33a5cc2..d8e759d98 100644 --- a/agent/tests/git_env.py +++ b/agent/tests/git_env.py @@ -12,12 +12,30 @@ An explicit ``GIT_DIR`` overrides repository **discovery** outright. That beats ``git -C ``, ``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 config user.email t@t`` writes into the *real* shared ``.git/config`` -and ``git -C init`` re-inits the *real* repository instead of creating one in -````. +is no defence. Under that environment ``git -C config user.email t@t`` writes +into the *real* shared ``.git/config`` and ``git -C init`` re-inits the *real* +repository instead of creating one in ````. + +``GIT_DIR`` specifically is what a linked worktree adds. Measured on git 2.50.1 by +dumping the hook environment from a ``pre-commit`` hook in both shapes — and this table +replaces a broader claim that was simply wrong, which matters because it was the stated +reason for stripping three vars rather than one: + +==================== =========== ================= ================== ============== +shape ``GIT_DIR`` ``GIT_COMMON_DIR`` ``GIT_INDEX_FILE`` ``GIT_PREFIX`` +==================== =========== ================= ================== ============== +plain/main checkout absent absent present present +linked worktree **present** absent present present +==================== =========== ================= ================== ============== + +So ``GIT_DIR`` is the sole differentiator, ``GIT_COMMON_DIR`` is exported in *neither* +shape, and ``GIT_INDEX_FILE``/``GIT_PREFIX`` are exported in *both* — "unset in a normal +checkout" was true only of ``GIT_DIR``. Stripping all seven is still correct: any of +them, however it arrives (a wrapper script, a developer's shell, an outer hook), bypasses +the pins. The list is justified by what the vars *do*, not by a claim about who sets them. + +This is also why the bug reads as unreproducible: run the same tests by hand from the +main checkout and nothing leaks, because ``GIT_DIR`` is not there to be inherited. That is why the bug reads as unreproducible: run the same tests by hand from the main checkout and nothing leaks. diff --git a/agent/tests/test_git_fixture_isolation.py b/agent/tests/test_git_fixture_isolation.py index d14e33387..bc46090a9 100644 --- a/agent/tests/test_git_fixture_isolation.py +++ b/agent/tests/test_git_fixture_isolation.py @@ -83,8 +83,10 @@ def shared_repo(tmp_path): """A real repo with a real-looking identity, plus a linked worktree. Stands in for the developer's checkout. The linked worktree matters because that is - the only configuration in which git exports ``GIT_DIR``/``GIT_COMMON_DIR`` to a - hook — which is why this leak never reproduces from a normal checkout. + the only configuration in which git exports ``GIT_DIR`` to a hook — measured, and the + reason this leak never reproduces from a normal checkout. ``GIT_COMMON_DIR`` is + exported in *neither* shape despite an earlier claim here; the table is in + ``tests/git_env.py``. """ repo = tmp_path / "shared" repo.mkdir() @@ -401,6 +403,76 @@ def test_no_mirror_strips_the_discovery_ceiling(self, parts): "walk is unfenced — see git_env._ceiling_directories for the measured escape." ) + def test_the_gate_recognises_the_identity_these_fixtures_write(self): + """Layer 1's identity must be in Layer 3's vocabulary. + + A THIRD encoding of ``TEST_IDENTITY_NAME`` lives in the gate, lowercased, inside + ``FIXTURE_NAMES`` — and a fourth relationship holds for the email, whose domain + suffix has to appear in ``RESERVED_EMAIL_SUFFIXES``. Neither is a copy the gate can + derive, because the gate deliberately has no import from ``agent/``. + + The failure this prevents is quiet and asymmetric: rename the constant here to + something the gate does not list, and the fixtures keep writing an identity that + the pre-push gate no longer recognises as a leak. Every test in both suites stays + green — Layer 1 still isolates, Layer 3 still runs — while the exact value the + layers were built around walks straight through the last one. + """ + gate = Path(__file__).resolve().parents[2] / "scripts" / "check-git-config-clean.mjs" + if not gate.is_file(): + pytest.skip(f"{gate} not present in this tree") + text = gate.read_text(encoding="utf-8") + + names_block = re.search(r"FIXTURE_NAMES\s*=\s*new Set\(\[(.*?)\]\)", text, re.DOTALL) + assert names_block is not None, f"no FIXTURE_NAMES set found in {gate}" + fixture_names = set(re.findall(r"'([^']*)'", names_block.group(1))) + # Lowercased on both sides: the gate lowercases the config value before the lookup, + # so `ABCA Test` is meant to match the entry `abca test`. + assert TEST_IDENTITY_NAME.lower() in fixture_names, ( + f"{gate} FIXTURE_NAMES does not contain {TEST_IDENTITY_NAME.lower()!r}, so the " + f"gate would not flag the name these fixtures set ({TEST_IDENTITY_NAME!r}). " + "Add it there or change it here — the two must agree." + ) + + suffix_block = re.search(r"RESERVED_EMAIL_SUFFIXES\s*=\s*\[(.*?)\]", text, re.DOTALL) + assert suffix_block is not None, f"no RESERVED_EMAIL_SUFFIXES array found in {gate}" + suffixes = re.findall(r"'([^']*)'", suffix_block.group(1)) + assert any(TEST_IDENTITY_EMAIL.lower().endswith(s) for s in suffixes), ( + f"{gate} RESERVED_EMAIL_SUFFIXES matches nothing in {TEST_IDENTITY_EMAIL!r}, so " + "the gate would not flag the address these fixtures set. Note the gate has a " + "structural fallback (a domain with no dot), but relying on it here would make " + "the guarantee accidental — `abca-test@example.invalid` has a dot." + ) + + def test_the_ts_suite_sets_up_git_with_the_same_identity(self): + """The gate's own suite hardcodes the identity, because it cannot import Python. + + Not pedantry about duplication: that suite runs ``git init``/``commit`` while jest + may itself be running under the pre-push hook. If its hardcoded identity drifts from + this one, a stray commit made during a failed setup carries a value the gate does + not recognise — the one shape #720 was filed for. + """ + suite = ( + Path(__file__).resolve().parents[2] + / "cdk" + / "test" + / "scripts" + / "check-git-config-clean.test.ts" + ) + if not suite.is_file(): + pytest.skip(f"{suite} not present in this tree") + text = suite.read_text(encoding="utf-8") + + for var, expected in ( + ("GIT_AUTHOR_NAME", TEST_IDENTITY_NAME), + ("GIT_COMMITTER_NAME", TEST_IDENTITY_NAME), + ("GIT_AUTHOR_EMAIL", TEST_IDENTITY_EMAIL), + ("GIT_COMMITTER_EMAIL", TEST_IDENTITY_EMAIL), + ): + assert f"{var}: '{expected}'" in text, ( + f"{suite} does not set {var} to {expected!r} (the value in " + "agent/tests/git_env.py). The two copies must match." + ) + class TestSharedConfigResolution: """Resolution must survive a repository too broken for git to describe. diff --git a/agent/tests/test_registry_loader.py b/agent/tests/test_registry_loader.py index ce3c963e6..c1d1f31d0 100644 --- a/agent/tests/test_registry_loader.py +++ b/agent/tests/test_registry_loader.py @@ -295,20 +295,33 @@ class TestMcpJsonNotCommittable: it to the PR. apply_mcp_assets marks it skip-worktree to block that (#246 B4).""" @staticmethod - def _git(repo, *args) -> subprocess.CompletedProcess: + def _git(repo, *args, check=True) -> subprocess.CompletedProcess: # ``env=`` is load-bearing (#855). Without it, an inherited GIT_DIR — which - # git exports to hooks in a linked worktree, i.e. whenever this suite runs + # git exports to a hook in a linked worktree, i.e. whenever this suite runs # as a pre-push gate from .worktrees/ — overrides repository discovery, so # `-C ` is ignored and every command below operates on the REAL - # repository. ``check=False`` is why that stayed invisible: re-initing the - # real repo and rewriting its config both exit 0. - return subprocess.run( + # repository. A silent ``check=False`` is why that stayed invisible: re-initing + # the real repo and rewriting its config both exit 0. + # + # ``check=True`` by DEFAULT, and that is the point rather than tidiness. Every + # assertion in this class is of the form "the secret is NOT in the staged diff", + # which an empty diff satisfies — so a failed `git init` or `git commit` in setup + # would produce a green test that had never built the scenario it names. Raised + # rather than passed to ``subprocess.run(check=True)`` so the message carries + # git's stderr, which a CalledProcessError does not print. + result = subprocess.run( ["git", "-C", str(repo), *args], capture_output=True, text=True, check=False, env=isolated_git_env(repo), ) + if check and result.returncode != 0: + raise AssertionError( + f"setup failed: git {' '.join(args)} exited {result.returncode}: " + f"{result.stderr.strip() or '(no stderr)'}" + ) + return result def _init_repo(self, tmp_path): # No `git config user.*` here on purpose: isolated_git_env supplies the @@ -340,7 +353,14 @@ def test_untracked_mcp_json_cannot_be_staged(self, tmp_path): assert (tmp_path / ".mcp.json").exists() # ...but the safety-net `git add -u` (and even an explicit add) cannot stage it. self._git(tmp_path, "add", "-u") - self._git(tmp_path, "add", ".mcp.json") + # The ONE call here allowed to fail, and its failure is the protection rather than + # a tolerated error: skip-worktree makes git report the path as outside the + # sparse-checkout definition and refuse. Asserted, so a future git that silently + # succeeded would be caught here instead of only in the diff below. + explicit_add = self._git(tmp_path, "add", ".mcp.json", check=False) + assert explicit_add.returncode != 0 + assert "sparse-checkout" in explicit_add.stderr + staged = self._git(tmp_path, "diff", "--cached") assert "SUPERSECRET" not in staged.stdout assert "sk-live-abc123" not in staged.stdout @@ -349,11 +369,25 @@ def test_tracked_mcp_json_change_cannot_be_staged(self, tmp_path): # The dangerous case Scott reproduced: the repo already tracks .mcp.json. self._init_repo(tmp_path) (tmp_path / ".mcp.json").write_text('{"mcpServers":{}}\n') - self._git(tmp_path, "add", ".mcp.json") + # A tracked file that SHOULD be staged, committed alongside. It is the control: + # `assert staged == ""` on its own is satisfied by a repo where `git add -u` never + # worked at all, which is indistinguishable from the protection working. With the + # control, the assertion becomes "add -u staged exactly the other file", so a + # broken setup fails loudly instead of reading as a pass. + (tmp_path / "control.txt").write_text("before\n") + self._git(tmp_path, "add", ".mcp.json", "control.txt") self._git(tmp_path, "commit", "-qm", "track mcp") + apply_mcp_assets(str(tmp_path), [self._secret_asset()]) + (tmp_path / "control.txt").write_text("after\n") + # git add -u stages tracked-but-modified files — must skip .mcp.json now. self._git(tmp_path, "add", "-u") + staged_names = self._git(tmp_path, "diff", "--cached", "--name-only").stdout.split() + assert staged_names == ["control.txt"], ( + "expected `git add -u` to stage the control file and nothing else; " + f"staged {staged_names!r}" + ) staged = self._git(tmp_path, "diff", "--cached") - assert staged.stdout.strip() == "" + assert ".mcp.json" not in staged.stdout assert "SUPERSECRET" not in staged.stdout diff --git a/cdk/test/scripts/check-git-config-clean.test.ts b/cdk/test/scripts/check-git-config-clean.test.ts index e700e5125..af0cd09a3 100644 --- a/cdk/test/scripts/check-git-config-clean.test.ts +++ b/cdk/test/scripts/check-git-config-clean.test.ts @@ -48,10 +48,41 @@ import { execFileSync, spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; +// js-yaml 4's `load` uses the core schema — no custom types are constructed — so it is +// the safe reader here, unlike the v3 API of the same name. +import * as yaml from 'js-yaml'; const REPO_ROOT = path.resolve(__dirname, '../../..'); const SCRIPT = path.join(REPO_ROOT, 'scripts/check-git-config-clean.mjs'); +/** + * The gate's exit contract, read out of the gate rather than re-typed here. + * + * Parsed instead of `import`ed on purpose: the script is ESM and this suite is compiled + * to CJS, and — more importantly — importing it would EXECUTE it against whatever + * repository jest happens to be running in. Parsing keeps one definition without that. + * + * A missing or renamed export throws here, so the coupling is real: the contract cannot + * be changed on one side only. The numeric values are pinned once, immediately below, + * so a silently *edited* value is caught too — a test that only mirrored the source + * would follow it anywhere. + */ +function exitCode(name: string): number { + const source = fs.readFileSync(SCRIPT, 'utf-8'); + const match = new RegExp(String.raw`export const ${name} = (\d+);`).exec(source); + if (!match) { + throw new Error( + `scripts/check-git-config-clean.mjs no longer exports \`${name}\`. ` + + 'The exit contract is asserted against these symbols — update both sides.', + ); + } + return Number(match[1]); +} + +const EXIT_CLEAN = exitCode('EXIT_CLEAN'); +const EXIT_PROBLEMS_FOUND = exitCode('EXIT_PROBLEMS_FOUND'); +const EXIT_COULD_NOT_CHECK = exitCode('EXIT_COULD_NOT_CHECK'); + /** * The shared config of the checkout this suite is running in. * @@ -187,11 +218,23 @@ describe('check-git-config-clean', () => { fs.rmSync(scratch, { recursive: true, force: true }); }); + test('the exit contract is 0 clean / 1 problems / 2 could-not-check', () => { + // Pinned once, here, and referenced by name everywhere else. Without this line the + // suite would only assert that it agrees with whatever the script currently says — + // `exitCode()` reads the values out of the source, so an edit to `EXIT_CLEAN = 3` + // would move both sides together and every other assertion would stay green. + // + // The numbers themselves are the interface: 1 vs 2 is what lets a caller tell "your + // config is corrupt" from "I could not look", and prek treats every non-zero the + // same, so nothing downstream would notice them being swapped. + expect([EXIT_CLEAN, EXIT_PROBLEMS_FOUND, EXIT_COULD_NOT_CHECK]).toEqual([0, 1, 2]); + }); + describe('the clean cases', () => { test('a fresh repository passes, and says what it checked', () => { const result = runGate(freshRepo('clean')); - expect(result.status).toBe(0); + expect(result.status).toBe(EXIT_CLEAN); // The rule list is the anti-vacuity assertion: a gate that inspected NOTHING // would also exit 0. Naming them means a dropped rule shows up here. expect(result.stdout).toContain('core.worktree'); @@ -208,7 +251,7 @@ describe('check-git-config-clean', () => { const result = runGate(REPO_ROOT); expect(result.stderr).toBe(''); - expect(result.status).toBe(0); + expect(result.status).toBe(EXIT_CLEAN); }); test('a real per-repo identity is NOT flagged', () => { @@ -220,14 +263,14 @@ describe('check-git-config-clean', () => { git(repo, ['config', '--local', 'user.name', 'Ada Lovelace']); git(repo, ['config', '--local', 'user.email', 'ada@example-corp.dev']); - expect(runGate(repo).status).toBe(0); + expect(runGate(repo).status).toBe(EXIT_CLEAN); }); test('a GitHub noreply address is NOT flagged', () => { const repo = freshRepo('noreply'); git(repo, ['config', '--local', 'user.email', '1234+ada@users.noreply.github.com']); - expect(runGate(repo).status).toBe(0); + expect(runGate(repo).status).toBe(EXIT_CLEAN); }); test('core.bare = false is NOT flagged', () => { @@ -235,7 +278,7 @@ describe('check-git-config-clean', () => { const repo = freshRepo('bare-false'); git(repo, ['config', '--local', 'core.bare', 'false']); - expect(runGate(repo).status).toBe(0); + expect(runGate(repo).status).toBe(EXIT_CLEAN); }); }); @@ -248,7 +291,7 @@ describe('check-git-config-clean', () => { const result = runGate(repo); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain('core.worktree'); // The remedy must be runnable as printed, not a description of one. expect(result.stderr).toContain( @@ -264,12 +307,31 @@ describe('check-git-config-clean', () => { // through git could only report "could not check" on the most common real // shape of this corruption. Written with fs.appendFileSync because git itself // refuses to set the second key once the first has broken the repo. + // + // TWO missing components (`gone/deleted-tmp-path`), and that is the whole test. + // Measured on git 2.50.1, a single missing LEAF under an existing directory is an + // rc-0 shape: rev-parse answers fine, so a version of this test using one would + // assert the outcome without ever constructing the state, and would pass just as + // happily against the git-based resolution this design rejected. const repo = freshRepo('worktree-missing'); - appendConfig(repo, 'core', [`worktree = ${path.join(scratch, 'deleted-tmp-path')}`]); + const missing = path.join(scratch, 'gone', 'deleted-tmp-path'); + appendConfig(repo, 'core', [`worktree = ${missing}`]); + expect(fs.existsSync(path.dirname(missing))).toBe(false); // the shape, asserted + + // The premise, measured rather than assumed: the rejected resolution really is + // unusable here. If a future git makes this succeed, this line fails and whoever + // sees it can re-evaluate the filesystem walk instead of inheriting a comment. + for (const form of [['--show-toplevel'], ['--path-format=absolute', '--git-common-dir']]) { + const probe = spawnSync('git', ['-C', repo, 'rev-parse', ...form], { + encoding: 'utf8', + env: isolatedGitEnv(repo), + }); + expect(probe.status).not.toBe(0); + } const result = runGate(repo); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain('core.worktree'); expect(result.stderr).toContain('deleted-tmp-path'); }); @@ -288,7 +350,7 @@ describe('check-git-config-clean', () => { const result = runGate(linked); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain(path.join(repo, '.git', 'config')); expect(result.stderr).toContain('user.name'); }); @@ -301,18 +363,67 @@ describe('check-git-config-clean', () => { const result = runGate(repo); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain('core.bare'); expect(result.stderr).toContain('--unset-all core.bare'); }); + + test('a GENUINE bare repository is not flagged, and the skip is declared', () => { + // The other half of the rule, and the reason it is gated: `core.bare = true` is + // corruption in a checkout and the correct, documented state of a bare repository. + // An ungated rule would report the *normal* config of a bare repo as the #855 leak + // and print a remedy — `--unset-all core.bare` — that BREAKS it. + // + // Reached the way it is reachable in practice: via an inherited `GIT_DIR`. The + // filesystem walk cannot arrive at a bare repo on its own (there is no `.git` to + // find), so an env-provided gitdir is the whole exposure. + // + // The second assertion is what stops the fix from being a silent skip. `OK — 4 + // rule(s)` is the anti-vacuity signal, so a rule that did not run has to say so in + // the count rather than quietly leaving it looking like a full pass. + const bare = path.join(scratch, 'genuine.git'); + git(scratch, ['init', '--bare', '-q', bare]); + expect( + fs.readFileSync(path.join(bare, 'config'), 'utf-8'), + ).toMatch(/bare\s*=\s*true/); // the premise: git itself wrote this + const unrelated = path.join(scratch, 'bare-cwd'); + fs.mkdirSync(unrelated, { recursive: true }); + + const result = runGate(unrelated, { GIT_DIR: bare }); + + expect(result.status).toBe(EXIT_CLEAN); + expect(result.stdout).toContain('core.bare (n/a: no working tree found)'); + }); }); describe('fixture identities — the #720 sighting', () => { + // Every branch of `isFixtureIdentity` and every entry of `RESERVED_EMAIL_SUFFIXES`, + // rather than a sample. The earlier table hit 2 of the 7 suffixes and one of the 5 + // fixture names, so five suffixes and four names were assertion-free: deleting any of + // them left the suite green while the gate stopped recognising a shape it names in its + // own header. Rule 3 is the only rule matching against a LIST, so it is the only one + // where per-entry coverage is a distinct question from per-rule coverage. test.each([ + // --- FIXTURE_NAMES (all five) --- ['user.name = t', 'user', ['name = t']], - ['user.email = t@t (no dot in the domain)', 'user', ['email = t@t']], + ['user.name = test', 'user', ['name = test']], + // The identity this repo's own fixtures set. Matched case-insensitively — the + // script lowercases before the lookup, and `agent/tests/git_env.py` spells it + // `ABCA Test`, so a case-sensitive comparison would miss the very value the + // fixtures write. `TestCrossCopyParity` holds the two spellings together. + ['user.name = ABCA Test (this repo\'s own fixture identity)', 'user', ['name = ABCA Test']], + ['user.name = Test User', 'user', ['name = Test User']], + ['user.name = Your Name (a copy-pasted placeholder)', 'user', ['name = Your Name']], + // --- RESERVED_EMAIL_SUFFIXES (all seven) --- ['a reserved .invalid domain', 'user', ['email = abca-test@example.invalid']], + ['a reserved .test domain', 'user', ['email = ada@corp.test']], + ['a reserved .example domain', 'user', ['email = ada@corp.example']], + ['a reserved .localhost domain', 'user', ['email = ada@build.localhost']], ['example.com', 'user', ['email = someone@example.com']], + ['example.net', 'user', ['email = someone@example.net']], + ['example.org', 'user', ['email = someone@example.org']], + // --- the two structural branches --- + ['user.email = t@t (no dot in the domain)', 'user', ['email = t@t']], ['an empty value', 'user', ['name = ']], ])('%s is rejected', (_label, section, lines) => { const repo = freshRepo(`identity-${_label.replace(/[^a-z0-9]+/gi, '-')}`); @@ -320,7 +431,7 @@ describe('check-git-config-clean', () => { const result = runGate(repo); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain('--remove-section user'); }); @@ -332,6 +443,42 @@ describe('check-git-config-clean', () => { }); }); + describe('several problems at once — the real shape of the leak', () => { + test('every problem is reported, including a repeated key', () => { + // Until this test, every failing case produced exactly ONE problem, which left two + // code paths unexecuted by the suite: the inner `for (const value of ...)` loop + // (only ever one value, so an implementation that read just the first would have + // passed) and the `Found N problem(s)` summary (only ever `1`, so an off-by-one or a + // hardcoded count would have passed). + // + // The repeated `email` line is not contrived. `[user]` sections appended by + // successive fixture runs stack up rather than replace — which is exactly how #720 + // was found, and why the gate reads `--get-all` instead of `--get`. Written with + // appendFileSync because `git config --local` would overwrite the first value. + const repo = freshRepo('multi-problem'); + const elsewhere = path.join(scratch, 'multi-elsewhere'); + fs.mkdirSync(elsewhere, { recursive: true }); + git(repo, ['config', '--local', 'core.worktree', elsewhere]); + appendConfig(repo, 'user', ['name = t', 'email = t@t']); + appendConfig(repo, 'user', ['email = someone@example.com']); + + const result = runGate(repo); + + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); + // Exact, not `toBeGreaterThan`: the count is the assertion. core.worktree + user.name + // + BOTH user.email values. + expect(result.stderr).toContain('Found 4 problem(s)'); + for (const expected of [ + `core.worktree = ${elsewhere}`, + 'user.name = t', + 'user.email = t@t', + 'user.email = someone@example.com', + ]) { + expect(result.stderr).toContain(expected); + } + }); + }); + describe('cannot-check is a FAILURE, not a pass', () => { test('outside any repository, exits 2 and says why', () => { // Fail-closed. Silently exiting 0 here would make a mis-wired hook look like a @@ -345,7 +492,7 @@ describe('check-git-config-clean', () => { const result = runGate('/'); - expect(result.status).toBe(2); + expect(result.status).toBe(EXIT_COULD_NOT_CHECK); expect(result.stderr).toContain('no `.git` found'); }); @@ -355,7 +502,7 @@ describe('check-git-config-clean', () => { const result = runGate(repo); - expect(result.status).toBe(2); + expect(result.status).toBe(EXIT_COULD_NOT_CHECK); expect(result.stderr).toContain('does not exist'); }); @@ -386,7 +533,7 @@ describe('check-git-config-clean', () => { try { const result = runGate(repo); - expect(result.status).toBe(2); + expect(result.status).toBe(EXIT_COULD_NOT_CHECK); expect(result.stderr).toContain('cannot read'); expect(result.stderr).toContain(configPath); // The specific regression: no all-clear may be printed about an unread file. @@ -410,11 +557,59 @@ describe('check-git-config-clean', () => { const result = runGate(unrelated, { GIT_DIR: path.join(repo, '.git') }); - expect(result.status).toBe(1); + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); expect(result.stderr).toContain(path.join(repo, '.git', 'config')); }); }); + describe('Layer 3 is actually wired up', () => { + // The same defect class as an ungated rule: every test above proves the SCRIPT + // behaves, and none of them proves anything ever RUNS it. Delete the + // `.pre-commit-config.yaml` stanza and Layer 3 is gone with the whole suite still + // green — the gate becomes a file nobody invokes. These two tests are the only place + // the wiring is asserted, so they are load-bearing rather than tidy. + + test('the hook is registered at BOTH stages and calls the mise task', () => { + // Parsed, not grepped: `stages: [pre-commit, pre-push]` appearing anywhere in a + // 150-line file with fourteen hooks says nothing about which hook carries it — + // every other local hook here declares stages too. + const config = yaml.load( + fs.readFileSync(path.join(REPO_ROOT, '.pre-commit-config.yaml'), 'utf-8'), + ) as { + repos: { repo: string; hooks: { id: string; entry?: string; stages?: string[] }[] }[]; + }; + const hook = config.repos + .flatMap((r) => r.hooks) + .find((h) => h.id === 'git-config-clean'); + + expect(hook).toBeDefined(); + expect(hook!.entry).toContain('mise run check:git-config-clean'); + // Both, and the pre-push half is the one that matters most: while the config is + // corrupted `git status` and `git revert` describe a different directory, so a + // developer can push a branch they believe they reverted. + expect(hook!.stages).toEqual(['pre-commit', 'pre-push']); + // `bash -c`, never `-lc`. A login shell sources the profile before the command, and + // this is the one hook with no `cd "$(git rev-parse --show-toplevel)"` prologue to + // undo a profile `cd` — so a relocated cwd would break both the `mise.toml` lookup + // and the script's own `.git` walk. See the comment above the stanza. + expect(hook!.entry).not.toContain('-lc'); + }); + + test('the mise task the hook names exists and points at this script', () => { + // The indirection the hook relies on: `mise run check:git-config-clean` is a name, + // and nothing else checks that the name resolves. Scoped to the stanza rather than + // matched against the whole file, so a task of some other name running the same + // script would not satisfy it. + const miseToml = fs.readFileSync(path.join(REPO_ROOT, 'mise.toml'), 'utf-8'); + const stanza = /^\[tasks\."check:git-config-clean"\]$([\s\S]*?)(?=^\[|\Z)/m.exec(miseToml); + + expect(stanza).not.toBeNull(); + const scriptRelative = path.relative(REPO_ROOT, SCRIPT); + expect(stanza![1]).toContain(`run = "node ${scriptRelative}"`); + expect(fs.existsSync(SCRIPT)).toBe(true); + }); + }); + describe("this suite's own git isolation", () => { test('isolatedGitEnv strips every location var and pins config resolution', () => { // Asserted because the isolation is what stops these tests from re-creating diff --git a/docs/src/content/docs/developer-guide/Contributing.md b/docs/src/content/docs/developer-guide/Contributing.md index 86f5f0b3e..ccfba32ca 100644 --- a/docs/src/content/docs/developer-guide/Contributing.md +++ b/docs/src/content/docs/developer-guide/Contributing.md @@ -97,6 +97,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`). diff --git a/scripts/check-git-config-clean.mjs b/scripts/check-git-config-clean.mjs index 9a2ba1b58..09f014b83 100644 --- a/scripts/check-git-config-clean.mjs +++ b/scripts/check-git-config-clean.mjs @@ -35,13 +35,33 @@ * * WHAT IT LOOKS FOR — the signature, not merely unusual settings: * - * 1. `core.worktree` — never legitimate in a normal checkout. Set, it pins EVERY - * linked worktree to one directory, so the root reads as dirty, the root's own - * untracked files disappear from `git status`, and `git revert` silently - * no-ops. Written when a fixture runs `git init` with both GIT_DIR and - * GIT_WORK_TREE inherited from the environment. - * 2. `core.bare = true` on a repo that has a working tree — the other stamp the - * same `git init` leaves behind. + * 1. `core.worktree` — never legitimate in a normal checkout. Two corrections to + * the obvious reading of it, both measured on git 2.50.1 because the first draft + * of this comment got both wrong: + * + * Set in the SHARED config it redirects the MAIN worktree only. `git -C + * rev-parse --show-toplevel` answers with the hijacked directory, the root reads + * as dirty, and the root's own untracked files disappear from `git status`. A + * LINKED worktree ignores it — it reports its own path and a clean status. So + * the blast radius is smaller than "every worktree", and less uniform, which is + * worse for diagnosis: whether a developer sees the corruption depends on which + * worktree they happen to be standing in. + * + * And `git revert` does not no-op, which understates it. It SUCCEEDS, creates a + * revert commit, and writes the reverted content into the hijacked directory + * while the root's copy of the file is left untouched. The log says the revert + * happened, the tree says it did not, and the content is in a third place + * neither of them names. + * + * Written when a fixture runs `git init` with both GIT_DIR and GIT_WORK_TREE + * inherited from the environment. + * 2. `core.bare = true` on a repo that has a working tree. NOT the other stamp of + * the same `git init`: measured, the two are mutually exclusive outcomes. + * `GIT_DIR` alone writes `core.bare=true` and no `core.worktree`; `GIT_DIR` plus + * `GIT_WORK_TREE` writes `core.worktree` and `core.bare=false`. Both rules are + * needed because both inherited shapes occur in this repo's fixtures — but + * neither key implies the other, so do not go hunting for a partner that cannot + * be there. * 3. `user.name`/`user.email` holding a value no human would have: a reserved * documentation domain (RFC 2606), a domain with no dot, or one of the literal * fixture identities used in this repo. A real per-repo identity is COMMON and @@ -50,7 +70,11 @@ * * WHY THE CONFIG PATH IS RESOLVED WITHOUT GIT AT ALL: no `git rev-parse` form * survives the state being detected. `--show-toplevel` is redirected by - * `core.worktree` outright — the corruption disabling its own alarm — and + * `core.worktree` outright — the corruption disabling its own alarm — though note the + * qualifier that matters here, since linked worktrees are this gate's stated habitat: + * it is redirected from the MAIN worktree and answers correctly from a linked one. + * A gate built on it would therefore be right or wrong depending on where it was + * invoked, which is harder to reason about than a uniform failure. Meanwhile * `--git-common-dir` merely fails differently: when `core.worktree` names a path * that no longer exists (a deleted pytest `tmp_path`, i.e. the shape this leak * actually leaves behind), rev-parse aborts with `fatal: Invalid path`, so a check @@ -84,7 +108,9 @@ * because `--get-all` reports an unreadable file and an absent key with the same * exit status (see `configValues`). * - * Exit codes: 0 clean · 1 corruption found (with remedy) · 2 could not check. + * Exit codes: 0 clean · 1 corruption found (with remedy) · 2 could not check — named + * as `EXIT_CLEAN` / `EXIT_PROBLEMS_FOUND` / `EXIT_COULD_NOT_CHECK` and exported, so the + * suite asserts the same symbols this script exits with rather than its own literals. * Case 2 is a failure, not a pass: an unreadable config or a git that cannot answer * is exactly the state in which a leak would go unnoticed. * @@ -97,9 +123,33 @@ import { existsSync, readFileSync, statSync } from 'node:fs'; import { spawnSync } from 'node:child_process'; -import { dirname, isAbsolute, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { basename, dirname, isAbsolute, join, resolve } from 'node:path'; + +/** + * The exit contract, named rather than spelled as bare literals at each `process.exit`. + * Exported so `cdk/test/scripts/check-git-config-clean.test.ts` asserts against these + * symbols instead of re-encoding 0/1/2 on its own side — the contract then has one + * definition and a change to it cannot pass silently. + * + * Importing this file for the constants does NOT run the gate: the rules execute only + * under the `isMain` guard at the bottom. + */ +export const EXIT_CLEAN = 0; +export const EXIT_PROBLEMS_FOUND = 1; +export const EXIT_COULD_NOT_CHECK = 2; -/** Literal identities used by fixtures in this tree. `t ` is the #720 sighting. */ +/** + * Literal identities used by fixtures in this tree. `t ` is the #720 sighting. + * + * Lowercased for case-insensitive comparison, which means `abca test` here is a THIRD + * encoding of `TEST_IDENTITY_NAME` from `agent/tests/git_env.py` (the second being + * `cdk/test/scripts/check-git-config-clean.test.ts`). Rename the Python constant + * without touching this line and the gate quietly stops recognising the identity this + * repo's own fixtures write — a false pass in a gate whose worst outcome is a false + * pass. `TestCrossCopyParity` in `agent/tests/test_git_fixture_isolation.py` asserts + * the three agree; do not rely on this comment to keep them together. + */ const FIXTURE_NAMES = new Set(['t', 'test', 'abca test', 'test user', 'your name']); /** @@ -155,10 +205,16 @@ function gitConfigRead(args) { const result = spawnSync('git', args, { encoding: 'utf8', cwd: '/', env }); if (result.error) { - return { status: null, stdout: '', stderr: String(result.error.message) }; + // `spawnError` rather than folding this into `status: null`: git never ran at all + // (not on PATH, ENOMEM, EACCES), which is a different fact from git running and + // exiting non-zero. Folded together, the caller's diagnostic reads `git exited + // null` — a message that describes neither case and sends the reader looking for + // a git bug instead of a missing binary. + return { status: null, spawnError: String(result.error.message), stdout: '', stderr: '' }; } return { status: result.status, + spawnError: null, stdout: result.stdout ?? '', stderr: result.stderr ?? '', }; @@ -166,7 +222,7 @@ function gitConfigRead(args) { function bail(message) { console.error(`check-git-config-clean: ${message}`); - process.exit(2); + process.exit(EXIT_COULD_NOT_CHECK); } /** @@ -209,7 +265,17 @@ function findGitDir() { } } -/** Absolute path of the repository-shared config, or exit 2 explaining why not. */ +/** + * The repository-shared config, or exit 2 explaining why not. + * + * Returns `{ path, hasWorkingTree }`. The second field exists because rule 2 must not + * assert something it never checked: `core.bare = true` is CORRUPTION in a checkout and + * the NORMAL state of a genuine bare repository, and this gate can be pointed at a bare + * repository by an inherited `GIT_DIR`. Proven by the shape of the resolved common dir — + * a `.git` whose parent exists — which holds for a plain checkout and for a linked + * worktree (whose `commondir` points back at the root's `.git`) and fails for a bare + * repo, where the gitdir is the repository itself. + */ function sharedConfigPath() { const gitDir = findGitDir(); @@ -243,7 +309,9 @@ function sharedConfigPath() { + 'could not be opened — an unreadable config is exactly the state in which a ' + 'leak would go unnoticed.'); } - return config; + + const hasWorkingTree = basename(commonDir) === '.git' && existsSync(dirname(commonDir)); + return { path: config, hasWorkingTree }; } /** All values of `key` in `configPath` (empty array when unset). */ @@ -254,6 +322,9 @@ function configValues(configPath, key) { // all, emitting `warning: unable to access ...` and no fatal. `sharedConfigPath` // already proved readability, so this is the belt to that braces: a non-empty stderr // on an rc 1 means the read did not happen and "no values" is not a finding. + if (result.spawnError) { + bail(`could not run git to read ${key} from ${configPath} (${result.spawnError}).`); + } if (result.status === 1 && result.stderr.trim() === '') return []; if (result.status !== 0) { bail( @@ -281,76 +352,98 @@ function isFixtureIdentity(key, value) { return domain !== undefined && !domain.includes('.'); } -const configPath = sharedConfigPath(); -const problems = []; -const rulesChecked = []; +function main() { + const { path: configPath, hasWorkingTree } = sharedConfigPath(); + const problems = []; + const rulesChecked = []; -// --- Rule 1: core.worktree --------------------------------------------------- -rulesChecked.push('core.worktree'); -for (const value of configValues(configPath, 'core.worktree')) { - problems.push({ - what: `core.worktree = ${value}`, - why: - 'pins every linked worktree to one directory: the root reads as dirty, its ' - + 'own untracked files vanish from `git status`, and `git revert` no-ops.', - fix: `git config --file ${configPath} --unset-all core.worktree`, - }); -} - -// --- Rule 2: core.bare on a repo that has a working tree --------------------- -rulesChecked.push('core.bare'); -for (const value of configValues(configPath, 'core.bare')) { - if (value.trim().toLowerCase() !== 'true') continue; - problems.push({ - what: `core.bare = ${value}`, - why: - 'this repository has a working tree, so it is not bare. The same stray ' - + '`git init` that writes core.worktree stamps this.', - fix: `git config --file ${configPath} --unset-all core.bare`, - }); -} - -// --- Rule 3: fixture identities ---------------------------------------------- -for (const key of ['user.name', 'user.email']) { - rulesChecked.push(key); - for (const value of configValues(configPath, key)) { - if (!isFixtureIdentity(key, value)) continue; + // --- Rule 1: core.worktree ------------------------------------------------- + rulesChecked.push('core.worktree'); + for (const value of configValues(configPath, 'core.worktree')) { problems.push({ - what: `${key} = ${value === '' ? '(empty)' : value}`, + what: `core.worktree = ${value}`, why: - 'not a value a contributor would set — a reserved domain, a domain with no ' - + 'dot, or a literal fixture identity. Commits made under it are ' - + 'unattributable, and it silently replaced whatever was configured before.', - fix: `git config --file ${configPath} --remove-section user`, + 'redirects the main worktree: `git status` describes that directory instead of ' + + 'this one, files untracked here vanish from it, and `git revert` succeeds ' + + 'while writing the reverted content there rather than into this checkout.', + fix: `git config --file ${configPath} --unset-all core.worktree`, }); } -} -if (problems.length > 0) { - console.error(`check-git-config-clean: ${configPath} carries the #855 leak signature.\n`); - for (const { what, why, fix } of problems) { - console.error(` ✖ ${what}`); - console.error(` ${why}`); - console.error(` fix: ${fix}\n`); + // --- Rule 2: core.bare on a repo that HAS a working tree ------------------- + // Gated, because the rule's name is a claim and an ungated rule does not check it: + // `core.bare = true` is corruption in a checkout and the correct, normal state of a + // genuine bare repository. Reached via an inherited `GIT_DIR` this gate can be + // pointed at a bare repo, where flagging it would be a false positive — and a gate + // that fires on legitimate configuration gets switched off rather than fixed, which + // is the failure mode this whole file is written to avoid. + if (hasWorkingTree) { + rulesChecked.push('core.bare'); + for (const value of configValues(configPath, 'core.bare')) { + if (value.trim().toLowerCase() !== 'true') continue; + problems.push({ + what: `core.bare = ${value}`, + why: + 'this repository has a working tree, so it is not bare. Written by a stray ' + + '`git init` that inherited a GIT_DIR without a GIT_WORK_TREE (which is why ' + + 'core.worktree is NOT expected alongside it — the two are exclusive).', + fix: `git config --file ${configPath} --unset-all core.bare`, + }); + } + } else { + // Recorded in the rule list rather than silently dropped: the printed count is the + // anti-vacuity signal, so a rule that did not run has to say so. + rulesChecked.push('core.bare (n/a: no working tree found)'); } - console.error( - 'A test or script shelled out to git with a GIT_DIR inherited from the ' - + 'environment (git exports one to hooks in a linked worktree), which overrides ' - + 'repository discovery and so defeats cwd, --local and the GIT_CONFIG_* pins ' - + 'alike. In agent/tests, build the environment with ' - + 'isolated_git_env() from tests/git_env.py.\n', - ); - console.error( - `Found ${problems.length} problem(s). Repair the config with the command(s) ` - + 'above, then re-run. Do not bypass this hook: the state it is reporting ' - + 'makes `git status` and `git revert` lie to you.', + + // --- Rule 3: fixture identities -------------------------------------------- + for (const key of ['user.name', 'user.email']) { + rulesChecked.push(key); + for (const value of configValues(configPath, key)) { + if (!isFixtureIdentity(key, value)) continue; + problems.push({ + what: `${key} = ${value === '' ? '(empty)' : value}`, + why: + 'not a value a contributor would set — a reserved domain, a domain with no ' + + 'dot, or a literal fixture identity. Commits made under it are ' + + 'unattributable, and it silently replaced whatever was configured before.', + fix: `git config --file ${configPath} --remove-section user`, + }); + } + } + + if (problems.length > 0) { + console.error(`check-git-config-clean: ${configPath} carries the #855 leak signature.\n`); + for (const { what, why, fix } of problems) { + console.error(` ✖ ${what}`); + console.error(` ${why}`); + console.error(` fix: ${fix}\n`); + } + console.error( + 'A test or script shelled out to git with a GIT_DIR inherited from the ' + + 'environment (git exports one to hooks in a linked worktree), which overrides ' + + 'repository discovery and so defeats cwd, --local and the GIT_CONFIG_* pins ' + + 'alike. In agent/tests, build the environment with ' + + 'isolated_git_env() from tests/git_env.py.\n', + ); + console.error( + `Found ${problems.length} problem(s). Repair the config with the command(s) ` + + 'above, then re-run. Do not bypass this hook: the state it is reporting ' + + 'makes `git status` and `git revert` lie to you.', + ); + process.exit(EXIT_PROBLEMS_FOUND); + } + + // The counts are the anti-vacuity signal: a check that inspected nothing would + // also exit 0. + console.log( + `check-git-config-clean: OK — ${rulesChecked.length} rule(s) ` + + `(${rulesChecked.join(', ')}) clean in ${configPath}.`, ); - process.exit(1); } -// The counts are the anti-vacuity signal: a check that inspected nothing would -// also exit 0. -console.log( - `check-git-config-clean: OK — ${rulesChecked.length} rule(s) ` - + `(${rulesChecked.join(', ')}) clean in ${configPath}.`, -); +// Run only when invoked as a script, so the exit-code constants above can be imported +// by the test without the gate executing against whatever repo jest happens to be in. +const isMain = process.argv[1] !== undefined + && resolve(process.argv[1]) === fileURLToPath(import.meta.url); +if (isMain) main();