aws-samples / aws-samples/sample-autonomous-cloud-coding-agents
test(agent): git fixtures leak into the shared .git/config under an inherited GIT_DIR — make the isolation structural (4th recurrence of #622/#720)
- Dominant language
- TypeScript
- Stars
- 143
- Forks
- 46
- Avg merge
- 3d 9h
- Merged PRs (30d)
- 20
Description
## Summary
Python test fixtures in `agent/tests/` that shell out to `git` **write into the
repository's shared `.git/config`** whenever the test process inherits a `GIT_DIR`
from its environment — which is exactly what happens when the suite runs from a
**linked worktree under a git hook**. The result is a repo-local `[user]` section
and a `core.worktree` entry in `.git/config` that silently mis-attribute every
subsequent commit and redirect the primary checkout's working tree.
This has now recurred **four times**. Each prior fix was file-local, so the next
test file to shell out to `git` reintroduced it. This issue asks for a
**mechanism-level** fix that covers test files that do not exist yet.
## Impact
Observed in practice, in this order of nastiness:
1. **`core.worktree` in a non-bare repo redirects the primary checkout.** `git status`
reports a foreign branch, the primary checkout's real untracked files vanish from
the listing, and `git checkout --` / `git revert` operate on the *other* tree and
still **exit 0**. A revert can appear to succeed while changing nothing.
2. **Commit mis-attribution.** A repo-local `[user]` **always** shadows `~/.gitconfig`
— global identity is not a backstop. Commits land as `t `.
3. **The developer's `~/.gitconfig` is not the only casualty.** `git -C init`
under an inherited `GIT_DIR` does not create a repo in ``; it **re-inits the
shared repository**.
## Root cause (reproduced, not inferred)
Two facts combine:
**1. `GIT_DIR` outranks everything a fixture might use for containment.** An explicit
`GIT_DIR` overrides repository *discovery* outright — it beats `-C `, `--local`,
`cwd=`, `HOME=`, and the `GIT_CONFIG_GLOBAL` / `GIT_CONFIG_SYSTEM` pins
**simultaneously**. A fixture doing `git -C config user.email t@t` looks
contained and is not.
**2. Git exports `GIT_DIR` and `GIT_COMMON_DIR` to hooks *only* in a linked worktree.**
They are unset when a hook runs in a normal checkout. This is why the bug looks
unreproducible: running `uv run pytest` by hand from the main checkout is harmless.
It fires when `prek`'s `pre-push` hook runs the suite from `.worktrees//`.
`core.worktree` specifically requires **both** `GIT_DIR` *and* `GIT_WORK_TREE` to be
set during `git init`, and records the `GIT_WORK_TREE` value. Nothing in this
repository writes `core.worktree` explicitly — verified by grepping all `*.py`,
`*.ts`, `*.mjs`, `*.sh`, `*.toml`, `*.yaml`, `*.json`. It is purely environmental.
### Reproduction
```bash
# A real repo with a real identity, plus a linked worktree.
git init -q real && git -C real config user.name RealDev \
&& git -C real config user.email real@dev.example
git -C real commit -q --allow-empty -m init
git -C real worktree add -q ../wt -b probe
# Export what git exports to a hook in a linked worktree.
export GIT_DIR="$PWD/real/.git/worktrees/wt" GIT_COMMON_DIR="$PWD/real/.git"
# Replay agent/tests/test_registry_loader.py:306-308 verbatim.
mkdir sandbox
git -C sandbox init -q
git -C sandbox config user.email t@t
git -C sandbox config user.name t
git config --file real/.git/config --get-regexp '^user\.' # => user.name t / user.email t@t
ls -a sandbox # => no .git; the shared repo was re-inited
```
## Why the previous four attempts did not hold
| # | Fix | Scope | Why it did not generalize |
|---|-----|-------|---------------------------|
| #622 → #623 | `GIT_AUTHOR_*`/`GIT_COMMITTER_*` env vars instead of `git config --global` | `agent/src/pipeline.py` | Fixed **production** code only; said nothing about test fixtures. |
| #695 | — | orchestration arc | Touched the area incidentally; no isolation contract. |
| #720 → #731 | Hard-isolate git fixtures from the developer's real identity | `agent/tests/test_post_hooks.py` | Thorough, but lives in a **per-class fixture in one file**. |
| #665 | Registry asset resolution | `agent/tests/test_registry_loader.py` | Landed a **fresh unguarded `_git()` helper** 7 days after #731 hardened the other file. |
The pattern is consistent: every fix was placed in the file where the leak was
observed, so it could not protect the next file. #731's `_isolated_env` is the right
*content*; it is in the wrong *place*.
## Proposed fix — three layers
Each layer catches what the one above it misses, so no single mistake reaches
`.git/config`.
### Layer 1 — Prevent (`agent/tests/conftest.py`, autouse)
A session/function-scoped `autouse` fixture that strips the ambient git location
variables from every test's environment and pins config resolution:
```
GIT_DIR GIT_COMMON_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY
GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_PREFIX GIT_CEILING_DIRECTORIES
```
plus `GIT_CONFIG_GLOBAL` → a tmp path, `GIT_CONFIG_SYSTEM=os.devnull`,
`GIT_CONFIG_NOSYSTEM=1`, and RFC-2606 reserved identity via `GIT_AUTHOR_*` /
`GIT_COMMITTER_*`. `conftest.py` is the only placement that covers test files that
do not exist yet.
Promote `_GIT_LOCATION_VARS` and the isolation helper **out of**
`test_post_hooks.py` so there is exactly one definition, and have that file import
it — otherwise the two drift.
### Layer 2 — Detect (`conftest.py`, `pytest_sessionstart` / `pytest_sessionfinish`)
Hash the shared config resolved via `git rev-parse --path-format=absolute
--git-common-dir` at session start, re-hash at session end, and **fail the run** with
a printed diff if it changed. This is **mechanism-independent**: it catches any future
route to the file, including ones Layer 1 does not anticipate.
Resolution detail that matters: do **not** use `git rev-parse --show-toplevel`.
`core.worktree` changes what it returns, so an already-polluted repo makes the check
compute a path that does not exist and report "clean" — the pollution would disable
its own detector. `--git-common-dir` answers from the gitdir alone.
### Layer 3 — Refuse (`check:git-config-clean` in pre-commit **and** pre-push)
A repo check that fails if `.git/config` contains `core.worktree` or a `[user]`
section, printing the exact `git config --file --unset-all` remedy. Pre-commit
catches it before a mis-attributed commit is created; pre-push catches whatever the
hook-run test suite just wrote.
### Also in scope
Audit the other test files that shell out to `git` and were never covered:
`agent/tests/test_shell.py`, `agent/tests/test_server.py`.
## Authorization
Per [ADR-003](https://github.com/aws-samples/sample-autonomous-cloud-coding-agents/blob/main/docs/decisions/ADR-003-contribution-governance.md), this issue
consolidates and supersedes the four prior partial fixes:
- #622 (issue, closed) / #623 (PR, merged) — production `git config --global` clobber
- #695 (PR, merged) — incidental
- #720 (issue, closed) / #731 (PR, merged) — `test_post_hooks.py` fixture isolation
- #665 (PR, merged) — introduced the current unguarded `test_registry_loader.py` helper
Filing a new issue rather than reopening #720 because the scope is different: #720
was "use a reserved identity in one file's fixtures"; this is "make the isolation
structural so a fifth recurrence is not possible."
## Acceptance criteria
- [ ] The reproduction above leaves `.git/config` byte-identical when run through the suite.
- [ ] A deliberately unguarded new fixture (`git -C config user.name x` with `GIT_DIR` set) **fails** the test run rather than silently mutating the shared config — i.e. the detector is proven live, not assumed.
- [ ] `_GIT_LOCATION_VARS` has exactly one definition in the tree.
- [ ] `test_shell.py` and `test_server.py` git call sites are audited and covered.
- [ ] `check:git-config-clean` fails on a config containing `core.worktree` or `[user]`, and prints a copy-pasteable remedy.
- [ ] `mise run build` green.
Contributor guide
Assessment
This issue has not been assessed yet.