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 (7 vars) stripped first, then HOME/XDG_CONFIG_HOME/GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM/GIT_CONFIG_NOSYSTEM/GIT_CEILING_DIRECTORIES and the four GIT_AUTHOR_*/GIT_COMMITTER_* identity vars pinned. Order matters: while any location var is set, every pin below it is bypassed.

Note the asymmetry in that list, because it is a sign error waiting to happen: the 7 stripped vars all redirect git to a repository of the environment's choosing, so removal is correct for every one of them. GIT_CEILING_DIRECTORIES does the opposite — it limits the discovery walk — so it is pinned, not stripped. It was in the strip list until review, which meant the fixture was deleting its own fence.

An autouse _isolate_git_location fixture in conftest.py applies the same stripping to os.environ for every test, and additionally chdirs the process into tmp_path with the ceiling pinned at tmp_path.parent. That second part closes a route the stripping does not: pytest runs from agent/, inside the checkout, so a fixture that forgets isolated_git_env could reach the shared config with no GIT_DIR, no -C and no cwd= involved at all — measured on git 2.50.1, a bare git config user.email t@t from a non-repository subdirectory of a repository walks UP and writes the parent's config, rc 0. It now fails loudly with fatal: not in a git directory. Identity 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 (22 tests) and agent/tests/test_git_fixture_isolation.py (36 tests). Both are built to fail if the guard stops guarding, and which test catches which failure is stated precisely below — an earlier revision of this section credited the differential test with a property it does not have, and review was right to measure it:

• The differential test runs the same git config command 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. What that proves is that the mechanism still behaves as documented — an inherited GIT_DIR really does redirect a write into another repository — which is the premise all three layers rest on and which git could in principle change. It does not prove isolated_git_env is doing anything: half A leaks because the test sets GIT_DIR itself, and half B would still be contained if the helper returned a bare dict(os.environ), because the autouse fixture has already stripped and pinned that environment.
• A gutted helper is caught by test_strips_every_location_var / test_pins_config_resolution_and_identity (explicit base=, so no fixture underneath them) and by test_the_strip_has_teeth_out_of_process, which spawns a nested pytest with the hook environment set. That last one exists because the in-process assertion cannot prove the strip runs: those vars are absent from the parent environment in normal use, and an autouse fixture has already finished before any test body starts. Measured with the strip loop deleted from conftest, the in-process test still reports 1 passed while the nested one fails.
• Layer 2 being armed at all is TestLayer2IsArmed, which asserts _SHARED_GIT_CONFIG is not None when run inside a checkout. pytest_sessionstart has three early returns that switch the layer off for a whole session while leaving every other test green — a failure mode with no symptom.
• The three GIT_LOCATION_VARS copies (Python, the gate, the gate's suite) had drifted to 8/7/8 entries. TestCrossCopyParity now enforces the parity the comments were only claiming, per-mirror, plus the specific rule that no copy may strip GIT_CEILING_DIRECTORIES.
• 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	1818 passed, 83.75% coverage (≥ 72% threshold)
//cdk:test	212 suites / 4492 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 cdk and 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.

Mutation-verified, because every new guard in this PR is one whose failure mode is a silent pass. Each of these was removed in turn and the named test confirmed red, then the file was restored and re-checked by sha256:

Removed	Goes red
the GIT_LOCATION_VARS strip loop	test_the_strip_has_teeth_out_of_process (in-process test still passes — that is the point)
monkeypatch.chdir(tmp_path)	test_the_process_is_moved_out_of_the_checkout
the GIT_CEILING_DIRECTORIES pin	test_the_process_is_moved_out_of_the_checkout
pytest_sessionstart's capture	TestLayer2IsArmed
parity in either mirror	TestCrossCopyParity, scoped to the mirror that drifted

Disclosure: pushed with --no-verify (twice, for two different pre-existing reds on main)

The pre-push hook runs the whole-repo security suite, so a finding anywhere in the tree blocks a push from anywhere in the tree. Both bypasses were proved innocent of my diff before being taken, and both reds live on main.

1. security:sast:masking — red on main across 46 files (pre-existing, tracked in #756); CI runs the ratcheted :range variant instead.

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

2. security:sast — one blocking javascript.lang.security.insecure-object-assign at cdk/src/handlers/linear-webhook-processor.ts:926, introduced by 12c9b63f (PR #831) and already on main.

• git diff --name-only origin/main...HEAD → 9 files, none of them linear-webhook-processor.ts
• that file is byte-identical on my branch and origin/main (sha256 5c659dcb…)
• gitleaks (309 commits), osv-scanner, and package tests (pre-push) all passed on this push

So neither bypass carried a finding of mine. main is currently un-pushable through this hook for anyone who branches from it, which is worth fixing on its own — as is the asymmetry between the pre-push gate and the ratcheted CI variant.

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.

[scottschreckengaust]

Pushed as 9ef7068d. All seven numbered findings plus the "Also" are addressed — and a correction on my own process first: I began working to a count of four. That was my miscount, not a triage decision; CHANGES_REQUESTED could not have cleared on 1–4 alone. Findings 5, 6, 7 and the "Also" turned out to be the ones that changed the shape of the fix rather than its details, so the undercount would have been expensive.

Two of your items I pushed back on and one measurement of yours came out differently on this host. Both are below with what I ran.

---

1. Layer 2 disarms on the real corruption shape — fixed

Your --git-common-dir table reproduces exactly here on git 2.50.1, including the asymmetry that matters (one missing leaf → rc 0; two or more missing → rc 128 Invalid path), and a cleaned-up tmp_path is the rc-128 shape. git rev-parse is gone from git_env.py entirely: find_git_dir / shared_git_config_path now walk the filesystem for .git, follow gitdir: and commondir, and honour GIT_DIR — the .mjs algorithm, ported.

The silent-return is gone too, and the distinction you asked for is now a type. GitConfigLookupError means "a repository was found and the guard could not look at it"; None still means "there is nothing here to protect" (no .git above cwd — the built container image). conftest keeps them apart in _SHARED_GIT_CONFIG_UNCHECKED, prints a SHARED GIT CONFIG — COULD NOT CHECK block, and sets session.exitstatus on the same branch as a real mutation.

The docstring that asserted the opposite is replaced with the measured table.

One extra thing fell out of writing that: _config_entries now reads with git config --file <p> --list -z rather than --get-all per key. --get-all exits 1 for both "key absent" and "file unreadable", which is the same ambiguity as your finding 3 — --list -z has no such overlap (rc 0 with empty output for an empty config, rc 128 for unreadable), so a non-zero status there is unambiguously a failure to read. That removes the bug class from the Python side instead of guarding against it.

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

276 branch.* keys is decisive. GitConfigFingerprint now carries signature (values of core.worktree / core.bare / user.name / user.email) separately from digest + names, and only a signature change fails the session. Non-signature drift still prints — note: <path> changed during this run, but no #855 signature key did (keys added: branch.x.remote) — routine git/editor activity looks like this. Not failing. — so the detector stays mechanism-independent without turning a sibling worktree's git fetch into a red at pre-push. Values are captured for the signature keys so an overwritten user.email is still caught; only key names are ever printed.

3. Layer 3 reports OK on a config it could not read — fixed, and it had two holes

chmod 000 reproduces the false OK — 4 rule(s) ... clean exactly as you wrote it. Both fixes are in, because mutation testing showed one alone is not enough: sharedConfigPath() now proves readability with a direct readFileSync and bails 2 before any rule runs, and configValues treats rc 1 as key-absent only when stderr is empty. Deleting either one alone leaves the new test green; deleting both returns exit 0 with OK — 4 rule(s), which is what the test was measured against. The test therefore asserts the contract (exit 2, cannot read, and no OK — in stdout) rather than which internal stop fired, and skips under uid 0 where the scenario cannot be constructed — skipped rather than mocked, since a mocked version would pass whether or not the gate handles it.

4. Layer 3 never executes for rule 1 — conclusion accepted, version corrected, one suggestion declined

Correction: what this repo installs is prek 0.4.8, not 0.3.8 — mise.toml declares prek = "latest" (no pin, which is its own small hazard for a comment that cites a version), and prek --version reports 0.4.8 here. Your conclusion survives the version difference: measured on 0.4.8, prek derives the repo root from its own git rev-parse --show-toplevel and chdirs the hook there, so the omitted cd prologue buys nothing for core.worktree. The comment block at .pre-commit-config.yaml is rewritten to say that plainly: the prologue is omitted because it would be dead code, not because it is a protection, and the actual protection is inside the script (filesystem walk for .git, reads via git config --file from cwd /).

It also now states the known reach, so nobody over-trusts the line: for core.worktree with 2+ missing components prek aborts on that rc-128 rev-parse before any hook runs — but plain git commit fails identically, so that shape is self-announcing. The shape the gate actually catches is core.worktree pointing at a directory that exists (a sibling worktree), where git answers normally and nothing else complains.

Declined: anchoring the script to its own path (import.meta.url) so it could not be redirected. That would break the fail-closed contract the suite pins at check-git-config-clean.test.ts — "outside any repository, exits 2 and says why". A script anchored to its own location always finds the repository it lives in, so a mis-wired hook would report clean forever. Losing a diagnosis in a self-announcing failure is a smaller loss than losing the fail-closed exit 2.

5. Nothing asserts Layer 2 is armed — fixed

TestLayer2IsArmed::test_the_session_hook_captured_a_fingerprint_when_run_in_a_checkout imports conftest and asserts _SHARED_GIT_CONFIG_UNCHECKED is None, _SHARED_GIT_CONFIG is not None, the path is a real file, and the digest is 64 hex chars. It skips when find_git_dir finds no repository, so the container image is still a legitimate pass. Mutation-verified: with pytest_sessionstart's capture removed, this test is the thing that goes red.

6. Layer 1 test is a tautology where the suite actually runs — fixed, out-of-process

Your diagnosis is exact, including why: an autouse fixture has already finished before any test body starts, so nothing in-process can stage the precondition it removes. test_the_strip_has_teeth_out_of_process spawns a nested pytest with GIT_DIR / GIT_COMMON_DIR / GIT_WORK_TREE / GIT_INDEX_FILE set, running the single in-process node id, and asserts it passes.

Two traps worth recording, since both produced a green test for the wrong reason:

• The decoy GIT_DIR must point at a real throwaway repo. Pointing it at a nonexistent path makes the inner session's Layer 2 raise GitConfigLookupError → COULD NOT CHECK → exitstatus = TESTS_FAILED, so the outer rc == 0 assertion fails for a reason unrelated to the strip.
• Anti-vacuity: the nested run loads a probe_ambient plugin that records the GIT_* names present at sessionstart to a witness file, and the outer test asserts GIT_DIR is in it. Without that, a child env that silently failed to inherit the decoy would still report 1 passed.

Mutation-verified, and this is the point of it: with the strip loop deleted from conftest, the in-process test still reports 1 passed while the nested one fails. Your finding's premise is now measured by the suite rather than asserted in a comment. The in-process test keeps a NOTE saying it passes vacuously wherever the suite normally runs and doubles as the inner test, so nobody renames it without updating the node id.

7. The differential test cannot catch a gutted helper — claim corrected

Confirmed: under isolated_git_env → dict(os.environ) the differential test passes, for both reasons you gave. Half A leaks because the test sets GIT_DIR itself; half B is contained by the autouse fixture, not by the function under test.

The claim is corrected in the module docstring and in the PR body, and the accounting is now explicit about which test catches what:

• The differential test proves the git mechanism still behaves as documented — an inherited GIT_DIR really does redirect a write into another repository — which is the premise all three layers rest on and which git could in principle change. Kept for that, labelled as that.
• A gutted helper is caught by test_strips_every_location_var / test_pins_config_resolution_and_identity (both pass an explicit base=, so no fixture underneath them) and by test_the_strip_has_teeth_out_of_process for the real os.environ path.

Also: Layer 1 closes the GIT_DIR route, not repository discovery — fixed, and the ceiling was a sign error

Reproduced before changing anything, and it is worse than "same leak class": from outer/sub where outer is a repo and sub is not, a bare git config user.email t@t with no GIT_DIR, no -C and no cwd= returns rc 0 and writes outer/.git/config. The #855 leak reached with no GIT_DIR at all.

monkeypatch.chdir(tmp_path) is in, and it is not too invasive — the suite is green with it. GIT_CEILING_DIRECTORIES is pinned rather than deleted, and you were right that the deletion was the sign error: GIT_LOCATION_VARS is now 7 entries, all of which redirect git to a repository of the environment's choosing, so removal is correct for every one; the ceiling limits the discovery walk, so it is pinned. It is pinned at tmp_path.parent, not tmp_path, so a test's own repo under tmp_path stays discoverable while the walk can never climb out of the pytest temp tree wherever TMPDIR lands.

One fidelity detail that only shows up on hosts like this one: git resolves ceiling entries through symlinks, so _ceiling_directories() and the fixture both use realpath, not abspath. $HOME here is /home/scoschre → /local/home/scoschre; an abspath spelling silently fails to match and the Python mirror would be strictly weaker than the thing it mirrors. (tmp_path is already physically resolved, so path-equality assertions are unaffected.)

test_the_process_is_moved_out_of_the_checkout asserts all three properties, including that the bare subprocess.run(["git","config","user.email","t@t"]) now fails with fatal: not in a git directory. Mutation-verified against both the chdir and the ceiling pin.

---

Mutation testing

Every new guard here has a silent pass as its failure mode, which is the defect class in your findings 5, 6 and 7. Each was removed in turn and the named test confirmed red, then the file restored and re-verified by sha256:

Removed	Goes red
the GIT_LOCATION_VARS strip loop	test_the_strip_has_teeth_out_of_process — the in-process test stays green, which is the finding-6 premise measured
monkeypatch.chdir(tmp_path)	test_the_process_is_moved_out_of_the_checkout
the GIT_CEILING_DIRECTORIES pin	test_the_process_is_moved_out_of_the_checkout
pytest_sessionstart's capture	TestLayer2IsArmed
the readFileSync proof and the rc-1/stderr check	the unreadable-config test (either alone: still green — see finding 3)
GIT_LOCATION_VARS parity in either mirror	TestCrossCopyParity, scoped to the mirror that drifted

From the non-blocking list

Two are in 9ef7068d:

• The three diverged copies (8/7/8). Resolved by restructuring rather than by re-syncing: 7 stripped vars + an explicit ceiling pin in all three, and the parity is now enforced — TestCrossCopyParity parses GIT_LOCATION_VARS out of the .mjs and the .test.ts and asserts both equal the Python tuple, plus the specific rule that no copy may strip GIT_CEILING_DIRECTORIES. Per-mirror, so a drift names the file that drifted. Skips when a mirror is absent (container image). You are right about the irony, and a comment claiming parity was the wrong instrument.
• The 1-tuple assertion. Now os.environ["GIT_CONFIG_GLOBAL"] != os.path.expanduser("~/.gitconfig"), with a comment on why the containment form the old comment described would be wrong here: TMPDIR is under $HOME on this host, so "not under $HOME" would fail on a correctly-pinned value.

The rest of the non-blocking list — the five mechanism claims in the .mjs prose, the two tests that do not construct the case they claim, the TEST_IDENTITY_NAME triple encoding, rule 2's unconditional core.bare, bash -lc → bash -c, test_registry_loader's check=False, rule-3 sampling, the hook-registration assertion, exit-code constants and gitConfigRead's null folding, the multi-problem case, and the CONTRIBUTING.md / Starlight update — is not in this commit. I am not disputing any of them; I stopped at the blocking set to get this in front of you. Say the word and they go in as a second commit, or tell me which subset you want and I will leave the remainder as follow-up issues. The CONTRIBUTING.md pair and the mechanism claims are the two I would do first: the first is a real gap this PR introduces, and the second matters most on a PR whose primary artifact is the explanation.

Verification

agent pytest 1818 passed (was 1789), 83.75% coverage; cdk/test/scripts/check-git-config-clean.test.ts 22 passed (was 21); agent/tests/test_git_fixture_isolation.py 36 tests (was 27). mise //cdk:eslint clean with no mutation. node scripts/check-git-config-clean.mjs rc 0 before and after the push — checked after, because the leak recurs on push, which is how it was originally found. //cdk:synth still fails on this host for ec2:DescribeAvailabilityZones, unrelated to the diff.

On your closing observation: agreed, and it is now two bypasses on this branch for two different pre-existing reds on main (the security:sast:masking spread and a blocking insecure-object-assign at cdk/src/handlers/linear-webhook-processor.ts:926 from 12c9b63f / #831). Both are disclosed in the PR body with the byte-identity proofs. main is currently un-pushable through this hook for anyone who branches from it, which is its own bug rather than a footnote to mine.

[scottschreckengaust]

All remaining non-blocking items from the review are landed in c1a62315. CI is 8/8 green including build (agentcore) (15m13s).

Mechanism claims corrected. GIT_DIR is the only var git exports differentially to a hook — measured on git 2.50.1: a plain/main checkout exports GIT_INDEX_FILE + GIT_PREFIX and not GIT_DIR/GIT_COMMON_DIR; a linked worktree adds GIT_DIR alone. The earlier claim that git exported the whole 7-var set was wrong. The strip list is now justified by what each var does, not by who sets it, and the measured table lives in agent/tests/git_env.py with conftest.py pointing at it.

Tests that didn't construct what they asserted. TestMcpJsonNotCommittable._git was check=False, so a failed git init/git commit in setup produced a green test: every assertion there is "the secret is NOT in the staged diff", which an empty diff satisfies. Now check=True by default, raising an AssertionError that carries result.stderr (a CalledProcessError message omits it). Demonstrated rather than asserted — with the initial commit broken and check=False restored, test_untracked_mcp_json_cannot_be_staged goes green.

Two follow-ons from that:

• git add .mcp.json legitimately exits non-zero — skip-worktree and sparse-checkout share an index bit, so git reports the path as outside the sparse-checkout definition. That refusal is the protection, so it is now the one explicit check=False call and the message is asserted.
• test_tracked_mcp_json_change_cannot_be_staged gained a control.txt positive control. assert staged == "" alone is satisfied by a repo where git add -u never worked; the assertion is now "staged exactly the control file".

Rule 2's unconditional core.bare. core.bare = true is corruption in a checkout and correct in a bare repo, so rule 2 is gated on basename(commonDir) === '.git' && existsSync(dirname(commonDir)). The skip records itself as core.bare (n/a: no working tree found) so the printed rule count stays the anti-vacuity signal. A bare repo is only reachable via an inherited GIT_DIR — the filesystem walk can't find one, since there's no .git.

Exit codes. Named constants exported from the script, plus one explicit expect([EXIT_CLEAN, EXIT_PROBLEMS_FOUND, EXIT_COULD_NOT_CHECK]).toEqual([0, 1, 2]) pin so a renumbering fails loudly. Derived by parsing the source rather than importing it: importing an ESM script into this CJS suite would execute it against whatever repo jest runs in.

Rule 3 sampling. A rule matching against a list needs per-entry coverage, which is a different question from per-rule coverage — rule 3 is the only such rule. The table is now 14 cases: all 5 FIXTURE_NAMES, all 7 RESERVED_EMAIL_SUFFIXES, plus t@t and the empty value.

Also landed: bash -lc → bash -c (tradeoff documented); hook-registration assertion parsing .pre-commit-config.yaml and the mise.toml stanza, so Layer 3 being wired up is checked rather than assumed; a multi-problem case asserting Found 4 problem(s) and all four what: strings; gitConfigRead null folding; CONTRIBUTING.md + regenerated Starlight mirror explaining why the gate runs first (while the config is corrupted, git status and git revert describe a different directory, so every later hook reasons about a tree that isn't on disk).

Every guard mutation-verified, since vacuity is the defect class here and all of these fail silently. 6 TS mutations and 5 Python mutations, each with the source restored and sha256-checked after. Notably EXIT_PROBLEMS_FOUND = 3 reds only the pin test — exactly as designed.

One local caveat: mise run build cannot finish on this host — //cdk:synth:quiet needs ec2:DescribeAvailabilityZones, denied to the local role. Verified everything else instead: 212 cdk suites / 4507 tests, agent 1820 tests at 83.75% coverage, //cdk:eslint clean, and node scripts/check-git-config-clean.mjs → OK — 4 rule(s) ... clean.