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)

Abierto
#855 2 comentarios 0 reacciones 1 asignado Reclamado por @scottschreckengaust Ver en GitHub
agent-runtime approved bug P1 tooling
Lenguaje dominante
TypeScript
Estrellas
143
Forks
46
Merge medio
3 d 9 h
PR fusionados (30 d)
20

Descripción

## 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.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.