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

[scottschreckengaust]

Summary

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. This makes the isolation structural — one shared helper, one automatic fixture, and a gate outside the test suite entirely — so the next test file to shell out to git cannot re-introduce it.

Refs #855. Prior attempts: #622 → #623, #720 → #731. Not auto-closing; leaving #855 for a human to close after review.

The mechanism (why the earlier fixes kept losing)

GIT_DIR overrides repository discovery. It therefore outranks -C, --local, the process cwd, HOME, and the GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM pins simultaneously — a write aimed at a throwaway directory lands in the real repository regardless of how carefully the destination was specified.

Git exports GIT_DIR/GIT_COMMON_DIR to hooks only in a linked worktree. That is why this never reproduces from a plain checkout, and why it fires precisely in the contribution flow this repo documents: mise run hooks:pre-push:tests → pytest → a fixture running git init / git config with the hook's GIT_DIR still in its environment.

The damage is not cosmetic. core.worktree in the shared config pins every linked worktree to one directory, so git status reports the wrong tree, the root's own untracked files disappear from it, and git revert silently no-ops. [user] replacement destroys commit signing attribution — the failure that started this issue.

#	Where	What the fix did	How it was defeated
1	#622 → #623	stopped git config --global clobbering ~/.gitconfig	the write moved to the repo-local config
2–3	#720 → #731	hardened agent/tests/test_post_hooks.py's identities	#665 added agent/tests/test_registry_loader.py, which writes user.name/user.email of its own
4	#855 (this)	structural: one helper + autouse fixture + session detector + commit/push gate	—

Three layers

Layer 1 — prevent. New agent/tests/git_env.py is the single definition of the isolated environment: GIT_LOCATION_VARS (8 vars) stripped first, then HOME/XDG_CONFIG_HOME/GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM/GIT_CONFIG_NOSYSTEM and the four GIT_AUTHOR_*/GIT_COMMITTER_* identity vars pinned. Order matters: while any location var is set, every pin below it is bypassed. An autouse _isolate_git_location fixture in conftest.py applies the same stripping to os.environ for every test, so a fixture that forgets isolated_git_env still cannot reach outside its tmp_path. Identity now arrives via env vars — test_registry_loader.py's two git config user.* writes are deleted, and test_post_hooks.py's three duplicate copies of this logic are deleted in favour of the shared helper (−89 lines).

Layer 2 — detect. pytest_sessionstart fingerprints the shared config; pytest_sessionfinish re-reads it and, on any change, prints the offending key names with copy-pasteable remedies and sets session.exitstatus = TESTS_FAILED. Key names, not values, because a remote.*.url can embed credentials and this is printed into CI logs.

Layer 3 — refuse. scripts/check-git-config-clean.mjs + mise run check:git-config-clean, wired first in .pre-commit-config.yaml at both pre-commit and pre-push. Blocks the operation while the config carries the signature, no matter which tool wrote it.

Layer 2, proved end-to-end

Not asserted — run. A throwaway probe test appended [user] name = t / email = t@t to a fake shared config, with GIT_COMMON_DIR aimed at a scratch directory (the real repository was never in scope). The probe was deleted afterwards:

1 passed
=== SHARED GIT CONFIG MUTATED — <scratch>/config ===
  keys added: user.email, user.name
...
pytest exit status: 1

A green test run that still exits 1 — which is the whole point, since no test can observe a mutation made by a test scheduled after it. The no-mutation counter-case exited 0.

Layer 3, verbatim output

Run against a repo carrying the full signature:

check-git-config-clean: <repo>/.git/config carries the #855 leak signature.

  ✖ core.worktree = <repo>/deleted-pytest-tmp
      pins every linked worktree to one directory: the root reads as dirty, its own untracked files vanish from `git status`, and `git revert` no-ops.
      fix: git config --file <repo>/.git/config --unset-all core.worktree

  ✖ core.bare = true
  ✖ user.name = t
  ✖ user.email = t@t
      fix: git config --file <repo>/.git/config --remove-section user

Found 4 problem(s). ... Do not bypass this hook: the state it is reporting
makes `git status` and `git revert` lie to you.

Two design points worth the reviewer's attention:

• 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 (the corruption switching off its own alarm), and --git-common-dir aborts with fatal: Invalid path when core.worktree names a directory that no longer exists — i.e. a deleted pytest tmp_path, the shape this leak actually leaves behind. The first draft of the script had exactly that bug; cdk/test/scripts/check-git-config-clean.test.ts has a test for it. Reads go through git config --file <path> (git's own parser, no repository discovery) from cwd: '/' with the location vars stripped, because git config --file still performs repository setup for its working directory first.
• The rules match the leak's signature, not merely unusual settings. A real per-repo user.email is common and deliberately not flagged; core.bare = false (which git init writes itself) is not flagged. A gate that fired on legitimate configuration would be switched off rather than fixed.

Exit codes: 0 clean · 1 corruption found · 2 could not check. Case 2 is a failure: an unreadable config is exactly the state in which a leak would go unnoticed.

Tests

cdk/test/scripts/check-git-config-clean.test.ts (20 tests) and agent/tests/test_git_fixture_isolation.py (14 tests). Both are built to fail if the guard stops guarding:

• The central Python test is differential — the same git config command run twice, once with an inherited GIT_DIR and once through isolated_git_env, asserted to escape in the first case and be contained in the second. A test that only checked the contained case would still pass if isolated_git_env were quietly reduced to dict(os.environ). The "leak" half writes into a purpose-built fake shared repo under tmp_path.
• The gate's clean-case test asserts the rule list it printed, because a gate that inspected nothing would also exit 0.
• The jest suite's own git calls go through a TypeScript mirror of isolated_git_env, asserted on — jest here may itself be running under the pre-push hook, and a test suite for this gate that caused the leak while setting up would be a poor joke. Its last test re-hashes the real shared config and asserts it is byte-identical to the digest captured in beforeAll.

check-git-config-clean.test.ts lives under cdk/test/ for a root-level script for the same reason as the existing check-constants-sync.test.ts: there is no test tree at the repo root. It exercises a subprocess, so it contributes nothing to cdk/src coverage.

Verification

Suite	Result
agent pytest	1789 passed, 83.75% coverage (≥ 72% threshold)
//cdk:test	205 suites / 4366 tests passed
//cli:test	57 suites / 791 tests passed
//docs:build, link-check, drift-prevention, jira-forge-app	passed
//cdk:synth:quiet	fails, pre-existing — IAM denies ec2:DescribeAvailabilityZones in this account; unrelated to this change

The strongest evidence the leak is closed is not an assertion: after full 4366-test cdk and 1789-test agent runs, the shared .git/config was byte-identical — verified by content (no core.worktree, no [user] section), not by git status, which this corruption is capable of falsifying. The gate also passed in its first live pre-commit invocation on this very commit, and the commit is signed (G) under the correct identity — the step that failed when the leak last struck.

Disclosure: pushed with --no-verify

The pre-push hook runs whole-repo security:sast:masking, which is currently red on main across 46 files (pre-existing, tracked in #756); CI runs the ratcheted :range variant instead. Before bypassing I proved my own diff innocent:

• security:sast:masking:range with baseline origin/main → rc 0
• full security:sast configs against only my 7 changed code files → rc 0
• comm -12 of the 46 flagged files against my 9 changed files → empty
• security:secrets:range → no leaks

So the bypass carried none of my findings. This asymmetry between the pre-push gate and CI is itself worth fixing separately.

Scope notes

• Local-only by design. A CI runner's git config is ephemeral and rebuilt per job, so there is nothing there to protect; the gate is wired to the hooks, not to a workflow.
• Known limitation: submodules. Git legitimately sets core.worktree in a submodule's own config, so committing from inside one would flag rule 1. This repo has no submodules; if that changes, exempt them explicitly rather than dropping the rule. Documented in the script header.

🤖 Generated with Claude Code

[isadeks]

Request changes — good diagnosis, all three layers fail open

The mechanism write-up is the best part of this PR and I want to be clear that I am not disputing it: GIT_DIR outranking -C, --local, cwd, HOME and the GIT_CONFIG_* pins simultaneously is real, the worktree-only export is real, and the decision to resolve the config path without git at all is correct and non-obvious. I reproduced the core leak before reviewing — standing inside a brand-new scratch repo, git config --local user.name wrote into the other repository's shared config and the scratch repo's own config got nothing. Going structural after four recurrences is the right call.

The problem is that each of the three layers, as built, goes quiet in the state it exists to catch, and one of them will fire on innocent activity while wired into pre-push. Everything below is a bounded fix, not a design objection.

1. Layer 2 disarms on the real corruption shape

agent/tests/git_env.py:102-118 resolves via git rev-parse --path-format=absolute --git-common-dir, returns None on any non-zero exit, and conftest.py:104-105 then early-returns with no message and no marker. I probed four shapes of core.worktree on git 2.50.1:

core.worktree value	rev-parse
absolute, exists	rc 0
absolute, leaf missing	rc 0
absolute, 2+ missing	rc 128 Invalid path
relative, missing	rc 128 cannot chdir

A cleaned-up pytest tmp_path — .../pytest-of-<user>/pytest-N/test_x0 — is the 2-or-more-missing shape. So Layer 2 is blind precisely once the leak has occurred, and it is silent about it rather than reporting "could not check".

The docstring at git_env.py:88-95 asserts the opposite ("--git-common-dir is answered from the gitdir alone", so pollution cannot "disable its own detector"). scripts/check-git-config-clean.mjs:51-65 — in this same PR — gets this exactly right and is why Layer 3 walks the filesystem instead. Layer 2 did not inherit that lesson.

Suggested fix: port the .mjs resolution into git_env.py (walk up for .git, follow gitdir:/commondir, honour GIT_DIR), and make "could not check" loud — when a .git is found but the path or fingerprint cannot be produced, print a COULD NOT CHECK line and set session.exitstatus exactly as the mutation branch does. Reserve the silent pass for the one genuine case: no .git anywhere, e.g. inside the built container image.

2. The whole-file digest false-positives on normal git use

fingerprint_git_config (git_env.py:121-135) digests the entire file, and conftest.py:110 returns only when the digest is unchanged — so any byte change fails the session. The shared config in my checkout carries 276 branch.* keys, which is direct evidence that non-pytest processes write that file as a matter of routine. A git fetch, a git checkout -b in a sibling worktree, a gh pr call, or an editor writing branch.*.vscode-merge-base during a 70-second suite will turn a green run red under a banner asserting "This is the #855 leak: a test wrote into the repository's shared config", with a remedy of --remove-section user — telling the developer to delete their own identity for something no test did.

This is the failure mode your own header warns about at check-git-config-clean.mjs:47-49: a gate that fires on legitimate configuration gets switched off rather than fixed. And because the suite runs at pre-push, it blocks the push.

Suggested fix: fingerprint only the signature keys — the same set Layer 3 rules on — or keep the digest but exclude branch.* and remote.* churn.

3. Layer 3 reports OK on a config it could not read

check-git-config-clean.mjs:219 treats status === 1 as "key absent", but git config --file <p> --get-all <k> also exits 1 when the file cannot be read: permission-denied is only a warning on stderr, and configValues discards stderr. Every rule then returns no values, the anti-vacuity success line prints, and the gate exits 0:

$ chmod 000 <cfg>   # cfg carries core.worktree
$ git config --file <cfg> --get-all core.worktree
warning: unable to access '<cfg>': Permission denied   rc=1
$ GIT_DIR=... node scripts/check-git-config-clean.mjs
check-git-config-clean: OK — 4 rule(s) (...) clean   rc=0

That contradicts the contract stated at lines 67-69, where an unreadable config is explicitly meant to exit 2. Suggested fix: treat rc 1 as key-absent only when stderr is empty; better still, readFileSync the config once in sharedConfigPath() so unreadability is proven up front and exits 2 before any rule runs.

4. Layer 3 never executes for rule 1

Verified under prek 0.3.8 specifically, since that is what this repo installs — not pre-commit:

core.worktree -> existing dir:
  error: No `prek.toml` or `.pre-commit-config.yaml` found
core.worktree -> deeply missing dir:
  fatal: Invalid path '/private/tmp/prektest/gone'

In both cases the hook entry never runs. prek resolves the repository root and chdirs there itself before invoking any hook, so the deliberately-omitted cd "$(git rev-parse --show-toplevel)" prologue buys nothing for core.worktree — the framework has already been redirected. The reasoning at .pre-commit-config.yaml:25-33 therefore describes a protection that is not in effect for the flagship rule.

In fairness the developer is still blocked from committing, so this is fail-closed in outcome. What is lost is the diagnosis — the carefully written remedy is replaced by "no config file found", which reads like a broken prek install. Rules 2 and 3 are reachable normally; I confirmed the omitted prologue is a harmless no-op for those.

5. Nothing asserts Layer 2 is armed, and disarming it is invisible

conftest.py:75-91 (pytest_sessionstart) has no test. With an early return inserted so nothing is captured, tests/test_git_fixture_isolation.py stays 14/14 and the full agent suite stays 1789/1789 green.

It is undetectable by construction: with nothing captured, _report_shared_git_config_mutation early-returns at conftest.py:104-105, and test_is_inert_when_there_was_nothing_to_protect (test_git_fixture_isolation.py:294) asserts that as a pass. "The detector armed" and "the detector found nothing to protect" are indistinguishable to the suite. This is the vacuity problem the .mjs gate solves with its rulesChecked count — mutating that count away is caught — and Layer 2 has no equivalent. One assertion that _SHARED_GIT_CONFIG is not None when run inside a checkout would close it.

6. Layer 1 test is a tautology where the suite actually runs

conftest.py:279-280 — the location-var strip — is the single most important line in Layer 1. Deleting just that loop, leaving the pins in place, leaves the suite 14/14 green. It only reddens when GIT_DIR happens to be exported into the runner (GIT_DIR=... uv run pytest gives 2 failures).

So test_ambient_location_vars_are_stripped (test_git_fixture_isolation.py:135-141) asserts nothing under plain pytest, in CI, or under mise run build, because os.environ has no location var to strip in those environments. The one environment where it has teeth — the pre-push hook in a linked worktree — is the one nobody runs the suite in on purpose. Getting an env var in place before an autouse fixture runs needs an out-of-process run: pytester or a subprocess pytest with GIT_DIR set.

7. The differential test cannot catch a gutted helper

The PR body and test_git_fixture_isolation.py:5-8 both claim that a test checking only the contained case "would still pass if isolated_git_env were quietly reduced to dict(os.environ)", positioning the differential test as the thing that closes that hole. Under exactly that mutation, test_an_inherited_git_dir_escapes_but_isolated_env_contains passes. Half A leaks because the test sets leaky_env["GIT_DIR"] itself at :109 after calling the function, so it leaks regardless of the implementation; Half B is contained by the ambient autouse fixture, which has already removed GIT_DIR from os.environ, not by the function under test.

The mutation was caught, but by the two plain unit assertions at :75-87 — and both of those pass an explicit base=, so nothing in the suite asserts the strip on the real os.environ path. The test is worth keeping: it proves the git mechanism still behaves as documented. The claim about it should be corrected, since it is cited as the reason the suite cannot be gutted.

Also: Layer 1 closes the GIT_DIR route, not repository discovery

_isolate_git_location strips the location vars and pins GIT_CONFIG_*, but nothing constrains discovery from the inherited cwd, and pytest runs from agent/ inside the checkout. With exactly the fixture's environment, a subprocess.run(["git","config","user.email","t@t"]) with no cwd= or -C still writes the shared config. That is the same leak class, reached by a different route, for the author who "forgot isolated_git_env" — which is the population Layer 1 is advertised to protect. monkeypatch.chdir(tmp_path) in the same autouse fixture makes git fail loudly instead (fatal: not in a git directory); if that is too invasive for the suite, the docstring should say plainly that Layer 1 covers the GIT_DIR route only.

Note also that the fixture deletes GIT_CEILING_DIRECTORIES, which widens discovery rather than narrowing it — setting it to the tmp root would be strictly better, and test_git_fixture_isolation.py:189-191 already sets it back by hand, which reads as an acknowledgement of the hazard.

Non-blocking

Mechanism claims that measure false. On a PR whose primary artifact is explanation, these are what will mislead the next person:

• .mjs:38 and :256 — "pins EVERY linked worktree to one directory". Only the main worktree is redirected; a linked worktree ignores core.worktree from the common config. The other two symptoms in that sentence are correct.
• .mjs:43 and :269 — core.bare = true is not "the other stamp the same git init leaves behind". They are mutually exclusive: GIT_DIR alone writes core.bare=true and no core.worktree; GIT_DIR plus GIT_WORK_TREE writes core.worktree and bare=false. Both rules are worth keeping, but the causal story sends a reader hunting for a co-occurring key that cannot exist.
• .mjs:257 — "git revert no-ops" understates it. Revert succeeds, creates a commit, and writes the reverted file into the hijacked directory while the root's file is untouched. Strictly worse than a no-op and worth saying so.
• GIT_COMMON_DIR is never exported to hooks in either shape, contrary to git_env.py:15-16, conftest.py:265-267 and test_git_fixture_isolation.py:60-63. Separately, GIT_INDEX_FILE is exported in a plain checkout, which the "unset in a normal checkout" framing denies. The stripping is right; the reason given is too broad.
• .mjs:52 — --show-toplevel is redirected from the main worktree, but returns correctly from a linked one. Since linked worktrees are the stated habitat, the qualifier matters.

Two tests do not construct the case they claim. check-git-config-clean.test.ts:247-254 uses a single missing leaf under an existing scratch dir, and test_git_fixture_isolation.py:154-183 uses an existing directory. Both are rc-0 shapes per the table in finding 1, so nothing aborts and neither test discriminates the chosen design from the rejected git-based one. Pointing core.worktree at path.join(scratch, 'gone', 'deleted-tmp-path') fixes the first; the second needs a relative or 2-plus-missing path written by raw file append.

Three copies of GIT_LOCATION_VARS, already diverged. git_env.py:35 has 8 entries and calls itself "the only copy in the tree"; .mjs:105 has 7 and says it mirrors that; .test.ts:74 has 8 and says the same. The omission in the .mjs is deliberate and harmless — :133 sets GIT_CEILING_DIRECTORIES itself — but the comment invites someone to "fix" a drift that is intentional, while a genuine future drift has nothing to catch it: the TS test's only assertion about the set iterates its own local copy. Same shape for the identity: TEST_IDENTITY_NAME is defined at git_env.py:49, re-hardcoded at .test.ts:97, and encoded a third time lowercased as 'abca test' in FIXTURE_NAMES — rename the constant and the gate quietly stops recognising the identity its own fixtures write, which is a false pass in a gate whose worst outcome is a false pass. This repo already has the pattern for this (check-constants-sync.ts, check-types-sync.ts, check-coverage-thresholds-sync.ts, cli/test/constants-parity.test.ts). Worth naming the irony: the thesis here is one definition so the next copy cannot fork from it, and the PR ships three.

Smaller items:

• Rule 2 (.mjs:262-273) is titled and worded as "core.bare on a repo that has a working tree" but flags core.bare = true unconditionally; reached via an inherited GIT_DIR it would flag a genuinely bare repo with a message asserting something it never checked.
• .pre-commit-config.yaml:37 drops the cd prologue but keeps bash -lc, so a cd in the user's profile relocates both the mise.toml lookup and the .git walk. bash -c avoids it.
• test_registry_loader.py:298-312 keeps check=False on _git and no call site inspects returncode, while its own new comment says check=False is why the leak stayed invisible. If setup fails, staged.stdout is '' and the assertions at :344-346 and :357-360 pass vacuously.
• test_git_fixture_isolation.py:150 — assert expanduser("~") not in (os.environ["GIT_CONFIG_GLOBAL"],) is membership against a 1-tuple, so it asserts inequality rather than the substring containment the comment describes.
• Rule-3 data is thinly sampled: 1 of 5 FIXTURE_NAMES exercised (not including abca test, which is what the fixtures actually write) and 2 of 7 RESERVED_EMAIL_SUFFIXES. Both are one-line additions to the existing test.each.
• Nothing asserts the hook is registered in .pre-commit-config.yaml at both stages or in mise.toml; deleting that stanza disarms Layer 3 with all 20 tests green — the same class as finding 5.
• Exit codes 0/1/2 are bare literals documented only in prose; three exported constants imported by the test would tie the contract to both sides. Relatedly, gitConfigRead returns status: number | null and folds spawn-failure into the same branch as a real non-zero exit, which can render git exited null.
• No multi-problem case anywhere in the suite, so the --get-all multi-value loops and the Found N problem(s) plural never run with N greater than 1.

Documentation

CONTRIBUTING.md:96-97 enumerates what runs at pre-commit and at pre-push. This PR adds a hook at both stages and updates neither line, nor the generated mirror at docs/src/content/docs/developer-guide/Contributing.md (mise //docs:sync). Heads-up that #679 edits those same two lines, so whichever lands second will need to reconcile.

The submodule limitation and the local-only scope are both documented honestly, and I appreciated the --no-verify disclosure with the four checks proving the diff innocent. One observation rather than a request: this is now the second PR in the queue where a pre-push gate that is red on main for unrelated reasons forced a bypass. The asymmetry you flag between the pre-push scan and CI's ratcheted variant looks worth its own issue.

What I verified as solid

• The session.exitstatus mechanism, checked against pytest 9.1.1 _pytest/main.py: pytest_sessionfinish runs whenever sessionstart completed, the chdir back happens first, and nothing recomputes the status afterwards. No xdist, no wider-scoped fixtures in agent/tests, single conftest.py — so the ordering holes that would normally worry me do not exist here.
• Layer 3's exit-2 paths for no-repo, missing config and a non-runnable git; always_run: true plus pass_filenames: false plus both stages, with no file filter that could skip it; mise run propagates exit 1 end to end.
• Nine of nine mutations to the .mjs gate were caught, including the anti-vacuity count and the exit(1) to exit(0) flip. The suffix-list-not-regex choice is right, and the one-trailing-newline strip at :229-233 is a genuinely subtle call, correctly reasoned and covered.
• _report_shared_git_config_mutation's own error handling: a config that vanishes at finish correctly fails. The asymmetry is only at sessionstart.

Governance is clean: #855 carries approved, is assigned, and is P1; the branch matches the convention; no cdk/src/ changes, so bootstrap policy coverage is not applicable here. All eight CI checks are green.