diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0b2f7a7c..26c5a68c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -22,6 +22,45 @@ repos: - repo: local hooks: + # Runs at BOTH stages, and first: while the shared config is corrupted, + # `git status` and `git revert` lie, so every later hook is reasoning about a + # tree that is not the one on disk. + # + # The ONLY hook here without a `cd "$(git rev-parse --show-toplevel)"` prologue, + # but do NOT read that as the protection — measured on prek 0.4.8, prek chdirs a + # hook to the repo root itself, derived from its own `git rev-parse + # --show-toplevel`, so this hook starts exactly where the prologue would have put + # it either way. It is omitted because it would be dead code, not because it is + # dangerous. The protection is inside the script: it locates the shared config by + # walking the filesystem for `.git` and reads it via `git config --file` from cwd + # `/`, so it never asks git a question `core.worktree` can redirect. `mise run` + # likewise finds mise.toml by walking the filesystem. + # + # Known reach, so nobody over-trusts this line: when `core.worktree` names a path + # with 2+ missing components, prek itself aborts on that rc-128 rev-parse before + # any hook runs — but plain `git commit` fails the same way, so that shape is + # self-announcing rather than silent. The shape this gate actually catches is + # `core.worktree` pointing at a directory that EXISTS (a sibling worktree), where + # git answers normally and nothing else complains. See #855. + # + # `bash -c`, NOT `-lc` like the hooks below, and that is not a style choice. A + # login shell sources the user's profile BEFORE running the command, so a `cd` in + # a profile relocates cwd — and unlike every other hook here, this one has no + # `cd "$(git rev-parse --show-toplevel)"` prologue to put it back, so both the + # `mise.toml` lookup and the script's `.git` walk would start from the wrong + # place. The hooks below are immune because they re-cd; this one is immune only by + # not sourcing the profile. Tradeoff accepted knowingly: `-l` is what puts `mise` + # on PATH for a git client that does not source the profile itself, so if this + # hook ever reports `mise: command not found` from a GUI client, the fix is an + # absolute path to mise — not restoring `-l`. + - id: git-config-clean + name: shared .git/config uncorrupted (#855) + entry: bash -c 'mise run check:git-config-clean' + language: system + pass_filenames: false + always_run: true + stages: [pre-commit, pre-push] + - id: gitleaks name: gitleaks (staged) entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run security:secrets:staged' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 672bdc1a..e97dd3e0 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 0ea21a8b..45cb8690 100644 --- a/agent/tests/conftest.py +++ b/agent/tests/conftest.py @@ -9,6 +9,16 @@ import pytest from models import TaskConfig +from tests.git_env import ( + GIT_LOCATION_VARS, + TEST_IDENTITY_EMAIL, + TEST_IDENTITY_NAME, + GitConfigFingerprint, + GitConfigLookupError, + fingerprint_git_config, + shared_git_config_path, + signature_keys_changed, +) # Session-wide hang backstop. SIGALRM (pytest-timeout method="signal") fires only # in the MAIN thread during a test's *call* phase, so a deadlock in a WORKER @@ -58,15 +68,149 @@ def _reap_on_hang() -> None: _hang_watchdog.start() +# Layer 2 of the #855 git-config guard: DETECT. Captured at session start and +# re-read at session finish. `None` means there is nothing to protect (no `.git` at or +# above cwd — e.g. the built container image), which is a real no-risk case rather than +# a failure to look. +_SHARED_GIT_CONFIG: tuple[str, GitConfigFingerprint] | None = None + +# Why the fingerprint could not be taken, when a repository WAS found. Distinct from +# `_SHARED_GIT_CONFIG is None`, and the distinction is load-bearing: "nothing to +# protect" is a pass, "could not look at the thing I am protecting" is a failure. The +# first version of this file collapsed the two into a silent `return`, so a config too +# broken for the resolver to describe — the exact state the guard is for — switched the +# guard off and reported nothing. +_SHARED_GIT_CONFIG_UNCHECKED: str | None = None + + +def pytest_sessionstart(session): + """Fingerprint the repository-shared ``.git/config`` before any test runs (#855). + + This is the backstop for the autouse fixture below, and it is deliberately + mechanism-INDEPENDENT: it does not care *how* the file was written, so it also + catches routes the fixture does not anticipate. Four previous fixes for this leak + were each scoped to one file and each was defeated by the next file added; a + whole-session before/after comparison cannot be outrun that way. + """ + global _SHARED_GIT_CONFIG, _SHARED_GIT_CONFIG_UNCHECKED + try: + path = shared_git_config_path() + except GitConfigLookupError as exc: + _SHARED_GIT_CONFIG_UNCHECKED = str(exc) + return + if path is None: + return + fingerprint = fingerprint_git_config(path) + if fingerprint is None: + _SHARED_GIT_CONFIG_UNCHECKED = f"{path} was located but could not be read or parsed by git" + return + _SHARED_GIT_CONFIG = (path, fingerprint) + + +def _describe_key_drift(before: frozenset[str], after: frozenset[str]) -> str: + """Which key names moved between two fingerprints. Names only, never values.""" + added = sorted(after - before) + removed = sorted(before - after) + parts = [] + if added: + parts.append(f"keys added: {', '.join(added)}") + if removed: + parts.append(f"keys removed: {', '.join(removed)}") + if not parts: + parts.append(f"value(s) changed among: {', '.join(sorted(after & before))}") + return "; ".join(parts) + + +def _report_shared_git_config_mutation(session) -> None: + """Fail the session if the shared ``.git/config`` was corrupted during the run (#855). + + Three outcomes, and the middle one is why this is not a single digest comparison: + + * a **signature** key moved (``core.worktree``, ``core.bare``, ``user.*``) — the leak. + Fails the session with a remedy. + * some **other** key moved — reported as a note and nothing more. The shared config is + written by ordinary work too (``git fetch`` rewriting ``remote.*``, ``push -u`` + adding ``branch..remote``), and this suite runs as a pre-push hook while other + worktrees may be active. A red naming no fixture and offering no remedy teaches + people to re-run past the gate. + * the config could not be fingerprinted at all — also a failure, see below. + + Reports key NAMES only, never values: a ``.git/config`` may hold a remote URL with + embedded credentials, and this text goes to CI logs. ``mise run + check:git-config-clean`` prints the offending values, which are safe for the signature + keys specifically. + + Does not repair the file. A test suite that silently rewrites ``.git/config`` would be + the same class of surprise as the bug it is guarding against — so this prints the exact + remedy and leaves the decision to a human. + """ + if _SHARED_GIT_CONFIG_UNCHECKED is not None: + print( + "\nSHARED GIT CONFIG — COULD NOT CHECK\n" + f" {_SHARED_GIT_CONFIG_UNCHECKED}\n" + " A repository was found but the #855 guard could not fingerprint its shared\n" + " config, so this run proves nothing about whether a fixture leaked into it.\n" + " That is itself the signature of a broken repo: `core.worktree` naming a\n" + " path that no longer exists makes every `git rev-parse` in the tree abort.\n" + " Diagnose with: mise run check:git-config-clean", + file=sys.stderr, + flush=True, + ) + session.exitstatus = pytest.ExitCode.TESTS_FAILED + return + + if _SHARED_GIT_CONFIG is None: + return + path, before = _SHARED_GIT_CONFIG + after = fingerprint_git_config(path) + + if after is None: + detail = "the file is now unreadable, gone, or no longer parses" + else: + if after.digest == before.digest: + return + drift = _describe_key_drift(before.names, after.names) + moved = signature_keys_changed(before, after) + if not moved: + # Real, but not the leak. Say so and leave the session's verdict alone. + print( + f"\nnote: {path} changed during this run, but no #855 signature key did\n" + f" ({drift}) — routine git/editor activity looks like this. Not failing.", + file=sys.stderr, + flush=True, + ) + return + detail = f"signature key(s) changed: {', '.join(moved)} — {drift}" + + print( + f"\nSHARED GIT CONFIG MUTATED — {path}\n" + f" {detail}\n" + " A test wrote into the repository's shared config. This is the #855 leak: a\n" + " fixture shelling out to git while a GIT_DIR is inherited from the environment\n" + " (which git exports to hooks in a linked worktree) escapes cwd, --local and the\n" + " GIT_CONFIG_* pins alike.\n" + " Fix the fixture: pass env=isolated_git_env(repo) from tests/git_env.py.\n" + f" Clean up the repo: git config --file {path} --unset-all core.worktree\n" + f" git config --file {path} --remove-section user", + file=sys.stderr, + flush=True, + ) + session.exitstatus = pytest.ExitCode.TESTS_FAILED + + def pytest_sessionfinish(session, exitstatus): - """Cancel the hang watchdog on a clean session finish. + """Cancel the hang watchdog on a clean session finish, then run the #855 check. - Without this, a legitimately slow-but-passing suite that finishes just after + Without the cancel, a legitimately slow-but-passing suite that finishes just after the 600s deadline (e.g. during teardown / coverage write) would be hard-exited by ``_reap_on_hang`` and turn green red with a thread-dump uncorrelated to any failed test. ``Timer.cancel()`` is a no-op if the timer already fired (a true - hang), so this only prevents the false-positive kill.""" + hang), so this only prevents the false-positive kill. + + The config check runs here rather than as a test because no test can observe a + mutation made by a test that runs after it.""" _hang_watchdog.cancel() + _report_shared_git_config_mutation(session) class FakeRunCmd: @@ -163,6 +307,72 @@ def make_task_config(**overrides) -> TaskConfig: ] +@pytest.fixture(autouse=True) +def _isolate_git_location(monkeypatch, tmp_path): + """Layer 1 of the #855 guard: PREVENT. Applies to every test, unconditionally. + + Placement is the whole point. #720/#731 got the *content* of this right but put it + in a per-class fixture inside ``test_post_hooks.py``, so #665 was free to add a + fresh unguarded ``_git()`` helper in ``test_registry_loader.py`` seven days later + and reopen the leak. An autouse fixture in ``conftest.py`` is the only placement + that also covers test files nobody has written yet. + + Three distinct jobs, and the third was added late because the first two do not + cover the route they appear to: + + 1. **Strip the repo-LOCATION vars.** While any of them is set, ``git -C ``, + ``cwd=``, ``--local`` and the ``GIT_CONFIG_*`` pins are all bypassed, because + an explicit ``GIT_DIR`` overrides repository discovery outright. Git exports + ``GIT_DIR`` to a hook in a linked worktree — which is exactly how this suite runs + as a pre-push gate from ``.worktrees/`` — and that one var is enough. The other + six are stripped for what they *do*, not because git sets them; see the measured + hook-environment table in ``tests/git_env.py``, which corrects an earlier claim + here that git exported the whole set. + + 2. **Pin config resolution and identity.** So that a fixture which shells out to + git *without* using ``isolated_git_env`` still cannot reach the developer's + ``~/.gitconfig``, and any commit it makes is attributed to the reserved test + identity rather than to whoever happens to be running the suite. + + 3. **Move the process out of the checkout, and cap discovery.** Jobs 1 and 2 close + the ``GIT_DIR`` route; neither touches repository discovery from the inherited + cwd, and pytest runs from ``agent/`` — *inside* the checkout. So with exactly + the environment jobs 1 and 2 produce, a plain + ``subprocess.run(["git", "config", "user.email", "t@t"])`` with no ``cwd=`` and + no ``-C`` still walks up from ``agent/`` and writes the shared config: same + leak, different route, and reached by precisely the author this fixture is + advertised to protect — the one who forgot ``isolated_git_env``. Standing in + ``tmp_path`` instead makes that command fail loudly (``fatal: not in a git + directory``) rather than succeed somewhere it should not. + + ``GIT_CEILING_DIRECTORIES`` is re-set for the same reason, and note that job 1 + *deletes* it, which widens discovery rather than narrowing it. Pinned to + ``tmp_path.parent`` — not ``tmp_path`` — so a test's own repository under + ``tmp_path`` is still discoverable while the walk can never climb out of the + pytest temp tree, whatever ``TMPDIR`` points at on this machine. + + Production code is a beneficiary too, not just fixtures: ``post_hooks`` and + ``repo`` shell out to git with the ambient environment, so an inherited ``GIT_DIR`` + would point the code under test at the real repository and the assertions would + silently describe the wrong one. + """ + for var in GIT_LOCATION_VARS: + monkeypatch.delenv(var, raising=False) + + monkeypatch.chdir(tmp_path) + # realpath because git resolves ceiling entries through symlinks and so does + # ``git_env._ceiling_directories``; a logical spelling would match neither on a host + # where TMPDIR or $HOME is a symlink. + monkeypatch.setenv("GIT_CEILING_DIRECTORIES", os.path.realpath(tmp_path.parent)) + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / ".gitconfig-test")) + monkeypatch.setenv("GIT_CONFIG_SYSTEM", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + monkeypatch.setenv("GIT_AUTHOR_NAME", TEST_IDENTITY_NAME) + monkeypatch.setenv("GIT_AUTHOR_EMAIL", TEST_IDENTITY_EMAIL) + monkeypatch.setenv("GIT_COMMITTER_NAME", TEST_IDENTITY_NAME) + monkeypatch.setenv("GIT_COMMITTER_EMAIL", TEST_IDENTITY_EMAIL) + + @pytest.fixture(autouse=True) def _clean_env(monkeypatch): """Remove agent-related env vars and reset the AWS session cache each test. diff --git a/agent/tests/git_env.py b/agent/tests/git_env.py new file mode 100644 index 00000000..d8e759d9 --- /dev/null +++ b/agent/tests/git_env.py @@ -0,0 +1,373 @@ +"""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. 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. +""" + +from __future__ import annotations + +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. +# +# 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", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_PREFIX", +) + +# 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" + +# 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*. + + 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``. + + ``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", + # 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 _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``. + + 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. + """ + 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") + 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) -> 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 + 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_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", "-z"], + capture_output=True, + text=True, + check=False, + timeout=_GIT_TIMEOUT_S, + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + 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 new file mode 100644 index 00000000..bc46090a --- /dev/null +++ b/agent/tests/test_git_fixture_isolation.py @@ -0,0 +1,915 @@ +"""Tests for the git-fixture isolation guard (#855). + +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 +purpose-built fake shared repo. +""" + +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( + ["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`` 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() + _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_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. + + 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. + + 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() + + # --- 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. + + 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] + + 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") + # 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." + ) + + 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. + + 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) + + toplevel = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + assert toplevel.stdout.strip() == str(elsewhere), "expected git to be redirected" + + 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 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) -> 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 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" + 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" + + before = self._fingerprint(config) + _git(repo, "config", "--local", "core.worktree", str(tmp_path)) + after = self._fingerprint(config) + + 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" + 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") + + before = self._fingerprint(config) + _git(repo, "config", "--local", "remote.origin.url", url_with_credential) + after = self._fingerprint(config) + + 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(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). + + 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, *, 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 + + 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) + 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)) + config.unlink() + + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + assert "unreadable, gone, or no longer parses" in capsys.readouterr().err diff --git a/agent/tests/test_post_hooks.py b/agent/tests/test_post_hooks.py index 3768aec1..6ffc6b2f 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 21717ecb..c1d1f31d 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: @@ -294,18 +295,41 @@ 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: - return subprocess.run( + def _git(repo, *args, check=True) -> subprocess.CompletedProcess: + # ``env=`` is load-bearing (#855). Without it, an inherited GIT_DIR — which + # 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. 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 + # 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") @@ -329,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 @@ -338,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 new file mode 100644 index 00000000..af0cd09a --- /dev/null +++ b/cdk/test/scripts/check-git-config-clean.test.ts @@ -0,0 +1,661 @@ +/** + * 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'; +// 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. + * + * 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`, 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', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_PREFIX', +]; + +/** 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, + // 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', + 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 }); + }); + + 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(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'); + 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(EXIT_CLEAN); + }); + + 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(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(EXIT_CLEAN); + }); + + 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(EXIT_CLEAN); + }); + }); + + 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(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( + `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. + // + // 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'); + 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(EXIT_PROBLEMS_FOUND); + 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(EXIT_PROBLEMS_FOUND); + 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(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.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, '-')}`); + appendConfig(repo, section, lines); + + const result = runGate(repo); + + expect(result.status).toBe(EXIT_PROBLEMS_FOUND); + 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('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 + // 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(EXIT_COULD_NOT_CHECK); + 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(EXIT_COULD_NOT_CHECK); + 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(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. + 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', () => { + 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(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 + // 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/repo'); + + for (const key of GIT_LOCATION_VARS) { + expect(env[key]).toBeUndefined(); + } + 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', () => { + // 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/docs/src/content/docs/developer-guide/Contributing.md b/docs/src/content/docs/developer-guide/Contributing.md index 86f5f0b3..ccfba32c 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/mise.toml b/mise.toml index e8914808..962c1165 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 00000000..09f014b8 --- /dev/null +++ b/scripts/check-git-config-clean.mjs @@ -0,0 +1,449 @@ +#!/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. 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 + * 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 — 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 + * 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. + * + * 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. 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 — 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. + * + * 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 { 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. + * + * 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']); + +/** + * 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) { + // `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 ?? '', + }; +} + +function bail(message) { + console.error(`check-git-config-clean: ${message}`); + process.exit(EXIT_COULD_NOT_CHECK); +} + +/** + * 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; + } +} + +/** + * 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(); + + // 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.', + ); + } + + // 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.'); + } + + const hasWorkingTree = basename(commonDir) === '.git' && existsSync(dirname(commonDir)); + return { path: config, hasWorkingTree }; +} + +/** 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 — 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.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( + `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('.'); +} + +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: + '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`, + }); + } + + // --- 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)'); + } + + // --- 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}.`, + ); +} + +// 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();