fix(hooks): clear git's repo-relocating env once, at the pre-push entry - #960
Conversation
#958 gave `e2e_init_repo` a scrub, which makes the correct thing easy but not the incorrect thing impossible: any future script that writes a bare `git init` bypasses the helper entirely. That is not hypothetical — #958's OWN selftest did exactly that, and committed into the developer's repo while printing OK. The author knew the trap, was writing the fix, and still bypassed it. The pollution has exactly one source: a git hook. A LINKED worktree's pre-push exports GIT_DIR (a plain checkout's does not — measured both ways), and it outranks `git -C <dir>`. Clearing it at the entry means every downstream child — preflight, lint, every selftest, every script not yet written — is immune without its author knowing the trap exists. That is the shape pre-commit uses: it filters the environment in the framework so third-party hooks cannot get this wrong. githooks(5) prescribes the idiom verbatim, fetched in-session from `man githooks` on git 2.54.0. Not a GIT_* prefix match, which would strip GIT_SSH_COMMAND and the GIT_COMMITTER_* identity. Measured, with the real hook and a probe standing in for preflight, pushing from a linked worktree: before this change: real repo core.bare = true (polluted) after: real repo core.bare = false (clean) Also measured: after the unset, `--show-toplevel`, `--abbrev-ref HEAD`, `--git-dir` and `status` all still resolve the worktree correctly — git falls back to the working directory it set for the hook, so nothing below loses its bearings. Checked for over-reach: the one GIT_CONFIG_COUNT user in this repo is star-history.yml, which runs on a fresh runner checkout with no core.hooksPath, so it never reaches this hook. No selftest, deliberately. CI cannot execute a hook test — the runner uses a fresh plain checkout and invokes recipes directly, never from a hook and never from a linked worktree. A hook selftest would be green locally and never run in CI, which is the trap #958 had to fix in its own wiring. The primary defense (the helper's scrub) keeps its CI-covered selftest; this is depth. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
Claude Security ReviewHead: Findings: 0 — No findings This diff only touches the repo's local .githooks/pre-push dev-tooling script (unsetting git's local env vars before preflight runs) and does not touch the hook shim, socket/pipe transport, config writes, or ingestion of untrusted transcript/JSONL/pack data, so there are no security-relevant changes. Automated read-only review by Claude Code |
Claude ReviewHead: Findings: 1 (1 high, 0 medium) Small, well-justified fix (scrubs git's repo-relocating env vars at the top of the pre-push hook to stop preflight/e2e helpers writing into the real repo from a linked worktree), but the new hook behavior — for a defect class (#893) that has already recurred twice — ships with no automated regression test, unlike the companion fix in #958 which got its own selftest. 1. HIGH —
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
==========================================
- Coverage 95.70% 95.69% -0.01%
==========================================
Files 196 196
Lines 57587 57587
==========================================
- Hits 55113 55109 -4
- Misses 2474 2478 +4
🚀 New features to boost your workflow:
|
…o paths Three lenses. The 10-line change had a defect in the one line that mattered, introduced two regressions, and shipped without the test I argued was impossible. All three of those were my calls. **The guard did not guard.** `unset $(git rev-parse --local-env-vars)` discards the substitution's status — only `unset`'s own counts, and bare `unset` returns 0 — so under `set -euo pipefail` a failing git left the scrub silently not happening and the push proceeded. Measured three ways. This is the same defect as #958 round 1's `return 1`, in a second disguise: I wrote a guard, asserted it was fail-closed, and did not run it. Now an explicit `if ! scrub=$(...) || [ -z "$scrub" ]` with `exit 1`. **Two regressions the change introduced, both found by an adversarial lens:** - `cd "$(git rev-parse --show-toplevel)"` sits after the scrub, which removed the only locator when the hook's CWD is outside the worktree (`git --git-dir=… push` from elsewhere). And `cd "$(...)"` swallows the failure by the same mechanic as `unset $(...)` — nine lines apart, and I fixed one without seeing the other. `toplevel` is now resolved before the scrub. Measured: OLD exit 0, NEW-before-fix exit 1 in /tmp, NEW-after 0. - `SKIP_PREFLIGHT=1` no longer bypassed the new failure, breaking the contract line 8 advertises. The early exit now precedes the scrub, which costs nothing since it exits before anything uses the scrubbed env. **The test I said was impossible.** I argued CI structurally cannot execute a hook test, so shipped none. That conflated testing a push with testing the hook's env contract, which is observable at its `exec`: stub `just`, hand the hook the GIT_DIR a linked worktree exports, read what the child saw. 184ms, no push, no worktree, no toolchain. The failure it now catches is silent by construction — deleting the line reds nothing otherwise. It enumerates `.githooks/*` rather than naming pre-push, so a hook added later fails here instead of inheriting the gap; `pre-commit` is exempt by name while it only runs `cargo fmt`. The control is a literal two-line hook, not one derived by `grep -v` from a real one — deriving it orphans the guard's `if` body, so the mutant exits before `just` and the control silently stops controlling. **The test's own first version was vacuous, caught by running the mutation.** Its `git init` for the fixture ran under the GIT_DIR this suite exports at its top, so `$s/repo/.git` was never created; the hook then died at `toplevel=` and never reached `exec`, `seen` never existed, and `test -z ""` passed against the real hook while the control still fired. The harness was bitten by the bug it tests for. Cleared the list around that init. Both copies now have teeth, verified by mutation: deleting the hook's scrub block reds; a behavioural mutation that keeps the block but unsets nothing also reds; emptying the helper's scrub reds. Also: `LINKED` restored to the hook comment — trimming it on owner feedback turned a true sentence false, since a plain checkout's pre-push exports nothing. And `e2e-common-selftest.sh`'s scrub rationale named the pre-push chain as its cause, which this commit makes false; it now names direct runs and `submodule foreach` (verified in-session: exports GIT_DIR=.git to children). shellcheck 0 · shfmt-check 0 · just lint 0 (19/19) · selftest 9 checks. plan-miss: I scoped this PR as "3 lines, low risk, no test" and was wrong on both halves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
Two-lens review — round 1 disposition (head
|
cd "$(git rev-parse --show-toplevel)" sat after the scrub, which removed the only locator when the hook's CWD is outside the worktree (git --git-dir=… push from elsewhere). And cd "$(...)" swallows the failure by the same mechanic as unset $(...) — nine lines apart, and I fixed one without seeing the other. |
toplevel now resolved before the scrub. OLD exit 0 · NEW-before-fix exit 1 in /tmp · NEW-after 0 |
SKIP_PREFLIGHT=1 no longer bypassed the new failure, breaking the contract line 8 advertises |
early exit now precedes the scrub |
HIGH — "CI structurally cannot execute a hook test" was false — FIXED
I shipped no test on that reasoning. It conflated testing a push with testing the hook's env contract, which is observable at its exec: stub just, hand the hook the GIT_DIR a linked worktree exports, read what the child saw. 184 ms, no push, no worktree, no toolchain.
The failure it guards is silent by construction — delete the line and nothing else reds.
Design details that earned their place:
- Enumerates
.githooks/*rather than namingpre-push, so a hook added later fails here instead of inheriting the gap.pre-commitis exempt by name while it only runscargo fmt. - The control is a literal two-line hook, not one derived by
grep -vfrom a real one — deriving it orphans the guard'sifbody, so the mutant exits beforejustand the control silently stops controlling.
The test's own first version was vacuous — FIXED (caught by running the mutation)
Its fixture git init ran under the GIT_DIR this suite exports at its top, so $s/repo/.git was never created; the hook then died at toplevel= under set -e, never reached exec, seen never existed, and test -z "" passed against the real hook while the control still fired. The harness was bitten by the bug it tests for. Cleared the list around that init.
MEDIUM — e2e-common-selftest.sh's scrub rationale — FIXED
It named the pre-push chain as its cause, which this commit makes false (the hook now scrubs upstream). The line still earns its place, so the cause was replaced: direct just e2e-scrub-selftest runs, and submodule foreach — verified in-session, it exports GIT_DIR=.git to children with no hook involved.
REFUTED — keep exit 1, keep [ -z "$scrub" ]
Lens 1 measured that git prepends its exec-path to a hook's PATH, so a hook invoked by git push cannot fail to find git; --local-env-vars has existed since v1.7.1 (2010). So the guard's population is a git shim that exits 0 printing nothing. Not dead — the trigger can fire — and the alternative is the silent corruption this PR exists to prevent. Blocking is proportionate.
Sharp-edge check on the sibling: e2e-common.sh:47's -z test looks identical but is not the same claim — there the value comes from a pipeline whose status belongs to tr, so -z is the only guard.
Comment
Owner feedback: too long. Trimmed 7 → 4 lines. Trimming also dropped LINKED, turning a true sentence false — a plain checkout's pre-push exports nothing. Restored, still 4 lines.
Teeth, verified by mutation
- delete the hook's scrub block → reds
- keep the block, unset nothing (behavioural) → reds
- empty the helper's scrub → reds
Gates: shellcheck -x 0 · just shfmt-check 0 · just actionlint 0 · just lint 0 (19/19) · selftest 9 checks · full preflight on push, 3174 passed.
Zero open confirmed HIGH at this head. A human merges.
Claude Security ReviewHead: Findings: 0 — No findings The diff hardens the pre-push git hook to scrub git's repo-relocating env vars (GIT_DIR, GIT_INDEX_FILE, etc.) before running preflight, correctly resolving toplevel first and unsetting only git's own fixed local-env-var names, with no exploitable gap found. Automated read-only review by Claude Code |
Claude ReviewHead: Findings: 1 (0 high, 1 medium) The PR extends the git-env scrub (from #893) to the pre-push hook and adds a selftest that exec's every non-exempt 1. MEDIUM —
|
The online review bot found it, and it is right: `hook_hands_just`'s stub only
recorded `${GIT_DIR:-}`, so the check proved GIT_DIR gets scrubbed and nothing
else. A scrub narrowed to `unset GIT_DIR` would leave GIT_INDEX_FILE relocating
the index — the other half of what outranks `git -C <dir>` in #893 — and pass.
The harness was already injecting GIT_INDEX_FILE and never looking at it.
The stub now reports EVERY name from `git rev-parse --local-env-vars` that is
still set at the `exec`, so the assertion covers whatever git's list covers
rather than one name I picked. GIT_WORK_TREE is injected too, since a hook's
children can carry it.
Measured, with the mutation the bot described:
unset $scrub -> unset GIT_DIR : FAIL (green before this commit)
whole scrub block deleted : FAIL
unmutated : 9 checks, exit 0
The check's message named GIT_DIR while testing the whole list; renamed to
"leaks no repo-relocating git env to preflight" so a failure names the real
requirement.
shellcheck 0 · shfmt-check 0 · just lint 0 (19/19).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML
Claude Security ReviewHead: Findings: 0 — No findings The diff hardens the pre-push git hook's env scrub (fail-closed on missing/empty git-reported local-env-vars) and adds a selftest covering it; the scrub still only unsets names sourced from trusted Automated read-only review by Claude Code |
Claude ReviewHead: Findings: 0 — No findings Clean, well-scoped fix: pre-push now scrubs the same repo-relocating git env vars (GIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE) that e2e_init_repo already scrubbed, resolving toplevel before the scrub and failing closed (exit 1) if git's local-env-vars list is empty; the new selftest section drives the actual Automated read-only review by Claude Code |
Round 2 + verification disposition (final head
|
1 participant
Follow-on to #958. That PR scrubbed git's repo-relocating env inside
e2e_init_repo; this moves the same scrub to the one place the pollution comes from.Why the helper is not enough
#958 makes the correct thing easy. It does not make the incorrect thing impossible — any future script that writes a bare
git initbypasses the helper entirely.That is not hypothetical. #958's own selftest did exactly that: it built its fixture with the naive form, committed into the developer's repo, and printed
OK. The author knew the trap, was actively writing the fix for it, and still bypassed it in the same file. A mechanism that the author bypasses at their most alert moment is not a mechanism.Why the hook entry is the right place
The pollution has exactly one source: a git hook. Measured both ways on git 2.54.0:
and
GIT_DIRoutranksgit -C <dir>. Chain:.githooks/pre-push→just preflight→lint→ every selftest and script it fans out to.Clearing it at the entry means every downstream child — preflight, lint, every selftest, every script not yet written — is immune without its author knowing the trap exists.
This is the shape pre-commit uses: its
no_git_env()filters the environment in the framework, so third-party hooks cannot get this wrong. Its comment is blunt — "Too many bugs dealing with environment variables and GIT".githooks(5)prescribes the idiom verbatim (fetched in-session fromman githooks, git 2.54.0). Deliberately not aGIT_*prefix match — that would stripGIT_SSH_COMMANDand theGIT_COMMITTER_*identity, neither of which relocates a repo.Measured
Real hook, a probe standing in for preflight, pushing from a linked worktree:
core.baretruefalseSide effects checked, not assumed: after the
unset,--show-toplevel,--abbrev-ref HEAD,--git-dirandstatusall still resolve the worktree correctly — git falls back to the working directory it sets for the hook, exactly asgithooks(5)describes.Over-reach checked: the only
GIT_CONFIG_COUNTuser in this repo isstar-history.yml, which runs on a fresh runner checkout with nocore.hooksPath, so it never reaches this hook.No selftest, deliberately
CI structurally cannot execute a hook test — the runner uses a fresh plain checkout and invokes recipes directly, never from a hook and never from a linked worktree. That blindness is exactly why #893 landed twice and why #958's selftest bug survived 47 green checks.
A hook selftest would therefore be green locally and never run in CI — the trap #958 had to fix in its own wiring (
just lintalone would have been a no-op). The primary defense, the helper's scrub, keeps its CI-covered selftest. This is depth.Note
core.hooksPathis an absolute path to the main checkout, so this takes effect for every worktree the moment it lands — and correspondingly it does not self-verify on its own push, which is why the before/after control above was run explicitly.🤖 Generated with Claude Code
https://claude.ai/code/session_01N4W4HtEmnF7Bu8jsqafJML