fangorn/ex_git_objectstore
public
ref:a7e0745724e439f7d28232e1afcf80e06a4cb800
fix(test): stop the suite escaping its fixtures; open a pin-bump PR on merge to main (#51)
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 push` in this repo is safe again.
- **Refs #81** — a merge to `main` now opens a PR on `fangorn/anvil` bumping its pinned `ex_git_objectstore` reference, 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 from `test_helper.exs` before `ExUnit.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:
- `TestIsolationTest` had to become `async: false`. It sets `GIT_DIR` process-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 with `GIT_WORK_TREE not allowed without specifying GIT_DIR`.
- The scanner flagged **itself** once `mix format` inserted a blank line that pushed its own `cd:` 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.exs` and `mix.lock`**, not just fixtures — exactly one line changes in each, and reversing the SHA reproduces both originals byte for byte;
- ran `decode_prs/1` against the **live CLI payload** — 17 open PRs, correctly parsed out of the `pull_requests` envelope;
- 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_on` is 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.replace` with `\1` followed by a SHA beginning with a digit parses as group **15**, silently eating the first character and writing a corrupt `mix.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/` |
SHA:
a7e0745724e439f7d28232e1afcf80e06a4cb800
Author:
Anvil <noreply@anvil.fangorn.io>
Date:
2026-07-31 04:18
Parents:
850623a
12 files changed
+1192
-12
| Type | ||
|---|---|---|
|
|
.anvil.yml | +38 −0 |
|
||
|
|
.formatter.exs | +4 −1 |
|
||
|
|
ci/pin_bump.exs | +471 −0 |
|
||
|
|
test/ci/pin_bump_test.exs | +284 −0 |
|
||
|
|
test/ex_git_objectstore/integration/git_repo_test.exs | +6 −1 |
|
||
|
|
test/ex_git_objectstore/integration/protocol_interop_test.exs | +2 −1 |
|
||
|
|
test/ex_git_objectstore/protocol/upload_pack_reuse_test.exs | +1 −1 |
|
||
|
|
test/ex_git_objectstore/protocol/upload_pack_v2_test.exs | +4 −3 |
|
||
|
|
test/ex_git_objectstore/test_isolation_test.exs | +208 −4 |
|
||
|
|
test/support/git_daemon.ex | +10 −1 |
|
||
|
|
test/support/git_env.ex | +149 −0 |
|
||
|
|
test/test_helper.exs | +15 −0 |
|
||