fix(test): stop the suite escaping its fixtures; open a pin-bump PR on merge to main #51
fix/75-test-git-env-isolation
into main
Two changes to this repository’s CI and test infrastructure. They are unrelated in mechanism but both are about CI doing what it appears to do.
- Closes #75 — the test suite could reach out of its fixtures and corrupt the developer’s checkout, via an inherited
GIT_DIR.git pushin this repo is safe again. - Refs #81 — a merge to
mainnow opens a PR onfangorn/anvilbumping its pinnedex_git_objectstorereference, so a fix here stops sitting unconsumed.
Part 1 — the suite escaping its fixtures (#75)
The bug
.githooks/pre-push runs mix test, and git exports GIT_DIR, GIT_WORK_TREE and friends into hook processes. So every git push ran the whole suite aimed at the repository being pushed. Twenty test files shell out to git; their fixture commits, config writes and ref updates landed on the developer’s real checkout.
This has now hit three people. Most recently it fired during PR #50’s push and left that checkout with core.bare=true, remote.origin.url pointed at a fixture’s HTTP daemon, a stray lfs.url, origin/main force-updated to a fixture commit, the branch being pushed force-updated to a fixture commit named pushed, and both indexes rewritten.
GIT_CEILING_DIRECTORIES does not cover this and structurally cannot: it guards repository discovery, and a redirect variable means discovery never happens.
Reproduced
Against a throwaway repository rather than a real checkout, so the reproduction is evidence and not another incident:
$ GIT_DIR=<throwaway>/.git GIT_WORK_TREE=<throwaway> mix test
Result: 930/1043 passed
$ # the throwaway afterwards
lfs.url http://127.0.0.1:61357/interop_multi_48902/info/lfs (was unset)
refs/heads/main 6f49a38 (was ad33e0c)
commits 3 (was 1)
The existing isolation test also fails under that environment, which is the cleanest statement of why the ceiling is not enough:
5) test git cannot escape a scratch dir upward into the project repo
git escaped the scratch dir and mutated a real repo:
Why a scrub alone was not the fix
#75 records that scrubbing GIT_DIR in test_helper.exs made the suite green but moved the corruption. I measured where it moves to, and it is a second, independent route: System.cmd("git", args) with no cd: runs in the project root, where git finds this project’s .git immediately and the ceiling is powerless because nothing ascends.
$ cd <project> && GIT_CEILING_DIRECTORIES=<project> git config --local test.injected yes
$ git config --get test.injected
yes
Seven call sites had that shape. For completeness I also tested the route the issue speculated about — discovery from below, out of a fixture dir under the project root. With the ceiling set it is already blocked, so it is not part of this fix.
The fix
test/support/git_env.ex owns the list of variables through which git can be redirected, and neutralises them two ways:
scrub_inherited!/0— drops them from the test process, called fromtest_helper.exsbeforeExUnit.start/1, so nothing inherits a redirection regardless of whether a call site uses the module.cmd/3— the directory is a required argument and every redirect variable is explicitly cleared for the child, so an ambient value cannot be reintroduced.
The seven undirected call sites now name their directory. GitDaemon.git_at/3 gains the scrubbed environment; its dir stays optional because git clone <url> <dest> names its target explicitly.
The hook is untouched. It was right to run the suite; the suite was wrong to be unsafe to run.
Result
| before | after | |
|---|---|---|
GIT_DIR=<throwaway> mix test |
930/1043 | 1048 passed, 0 failures |
| throwaway config / refs / commits | lfs.url written, main advanced 1 → 3 commits |
unchanged |
And the proof that matters — a real git push with the hook enabled, the operation behind every incident:
$ git push -u origin fix/75-test-git-env-isolation
Result: 1048 passed (2 properties, 1046 tests), 52 excluded
* [new branch] fix/75-test-git-env-isolation -> fix/75-test-git-env-isolation
The checkout afterwards is byte-identical to a snapshot taken before it, except for the refs the push is supposed to create. git push in this repo is safe again and --no-verify is no longer required.
Regression coverage
TestIsolationTest states the invariant as three routes, each guarded, using a throwaway repository as the victim so a regression fails an assertion instead of corrupting a checkout. Under a poisoned GIT_DIR before the fix, 5 of 7 fail — including git init itself and the pre-existing ceiling test.
The scan test (“no test file shells out to git without pinning it to a directory”) is what stops the shape coming back. I verified it is not vacuous by injecting an offending call into an unrelated file: reported at delta_test.exs:80, green again on removal.
Two bugs I introduced and then found, both of which would have shipped as flakes:
TestIsolationTesthad to becomeasync: false. It setsGIT_DIRprocess-globally to reproduce the hook, and every git subprocess inherits it; run concurrently the window before cleanup leaked into whichever async test happened to shell out. It surfaced as a neighbouring test failing withGIT_WORK_TREE not allowed without specifying GIT_DIR.- The scanner flagged itself once
mix formatinserted a blank line that pushed its owncd:check outside a 5-line window. It now builds its needle at runtime and uses a wider one.
Part 2 — bumping Anvil’s pin on merge to main (#81)
The gap
Anvil consumes this library as a git dependency pinned by SHA. Nothing moved that pin, so a fix can land on main and sit unconsumed indefinitely — which is the situation right now: #78’s receive-pack fix and #75 above are both invisible to Anvil until someone remembers mix deps.update ex_git_objectstore by hand.
A step gated on merges to main now opens a PR on fangorn/anvil moving the pin to that commit, so the change goes through Anvil’s review and full test suite. The PR is opened, never merged — Anvil’s CI is the gate and a human merges.
The pin lives in two places
Worth stating because the issue describes it as “mix.lock’s entry and nothing else”, which would not have worked:
| file | what it carries |
|---|---|
mix.exs |
ref: "<sha>" on the dependency — authoritative |
mix.lock |
the resolved SHA, twice |
Mix treats a lock that disagrees with mix.exs as stale and re-resolves from the dependency, so editing only the lock does nothing at all. Both move together, and a file that does not contain the reference in the expected shape is an error rather than something to write over.
Gating
Compares the checked-out SHA against origin/main, following the existing release step. CI checks out a detached HEAD so the branch name is always HEAD, and branch contains 'main' is a substring match that has already matched feat/324-git-maintenance (fangorn/anvil#234).
Idempotency
One fixed branch, force-pushed; repeated merges update the single PR opened from it. The listing is read with an explicit --limit well past the paginated default of 30 — an existing bump PR past the first page would read as “none open” and get a duplicate created, which is precisely the pile-up this is meant to prevent.
The credential is a prerequisite
This step will fail until an admin provisions a secret. That is deliberate.
The runner injects ANVIL_TOKEN scoped to the dispatching repository; it cannot write to fangorn/anvil (fangorn/anvil#390). The step reads a separately provisioned:
name: ANVIL_PIN_BUMP_TOKEN
value: an Anvil token that can push a branch to fangorn/anvil
and open a pull request on it (contents: write)
Deliberately not ANVIL_TOKEN — CI secrets are merged over the job environment (runner_executor_controller.ex), so a secret by that name would silently replace the injected per-job token for every other step in the job.
Absent, the step fails and prints exactly that. A silent skip would leave Anvil pinned to an old commit with nothing to show anything was missed, which is the bug being fixed.
What is tested, and what isn’t
Everything decidable is a pure function in ci/pin_bump.exs, tested directly rather than through a subprocess: rewriting each pinned reference, deciding update-vs-create, reading the credential, decoding the CLI listing, rendering the body. I/O is confined to main/1 and decides nothing.
Not unit-tested, and I would rather say so than write a test that asserts nothing: the shell in .anvil.yml (the gate, the CLI download) and main/1’s orchestration. Testing those meaningfully needs a real runner, a real cross-repo credential, and a real Anvil to open a PR against — the first genuine exercise is the first merge to main after the secret exists. What I did instead of pretending otherwise:
- ran the rewriters against Anvil’s actual
mix.exsandmix.lock, not just fixtures — exactly one line changes in each, and reversing the SHA reproduces both originals byte for byte; - ran
decode_prs/1against the live CLI payload — 17 open PRs, correctly parsed out of thepull_requestsenvelope; - ran the missing-credential path end to end and confirmed it exits non-zero with the actionable message;
- validated the YAML parses and the step’s
depends_onis the full green set.
ci/ is outside elixirc_paths, so none of this ships in the published package. .formatter.exs now covers it so the format step still checks it.
Two bugs the tests caught before this could write anything
Regex.replacewith\1followed by a SHA beginning with a digit parses as group 15, silently eating the first character and writing a corruptmix.lock. Uses\g{1}.- The CLI returns an object with
pull_requests, not a bare array. The first version read every listing as empty and would have opened a duplicate PR on every single merge — defeating the one requirement that matters most here.
Requirements
- REQ-GIT-084 — the suite cannot reach any repository but its own fixtures, in any ambient git environment.
- REQ-CI-001 — a merge to main opens or updates a single PR on the consumer bumping its pinned reference.
- REQ-CI-002 — the bump moves every place the reference is pinned, and nothing else.
- REQ-CI-003 — the cross-repo credential is required explicitly and its absence fails with an actionable message.
All created before the tests referencing them; every new test carries @tag requirements: [...].
Gates
mix test |
1071 passed, 0 failures (52 excluded :s3) |
mix test under a poisoned GIT_DIR |
1048 passed, 0 failures |
mix test under the real pre-push hook |
1048 passed, 0 failures |
mix dialyzer |
Total errors: 0 |
mix credo --strict |
unchanged from this branch’s baseline — adds none |
mix compile --warnings-as-errors |
clean |
mix format --check-formatted |
clean, now including ci/ |