Scrub git's exported environment in the pre-commit hook (issue #14)

Confidence Score: 4/5

The PR appears safe to merge, with two non-blocking gaps in the future-proof regression checker.

The runtime hook and pytest scrub implement the intended isolation, while the accepted concerns are limited to shell-parser cases that could let later hook edits evade the regression test.

Files Needing Attention: tests/test_pre_commit_git_env.py

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Commit[git commit] --> Hook[pre-commit hook]
  Hook --> Scrub[Unset inherited Git environment]
  Scrub --> Root[Resolve repository root]
  Root --> Tests[Run pytest]
  Tests --> FixtureGit[Fixture Git subprocesses]
  FixtureGit --> TempRepos[Resolve temporary repositories]

_{Reviews (1): Last reviewed commit: "Scrub git's exported environment in the ..." | Re-trigger Greptile}

@claude review

Claude finished @yonasb's task in 4m 54s —— View job

Review: PR #81 — scrub git's exported environment in the pre-commit hook

• Read the diff (.githooks/pre-commit, conftest.py, tests/test_pre_commit_git_env.py)
• Assess the hook fix for correctness and completeness
• Trace the shell parser in the new test by hand
• Evaluate the two Greptile findings
• Check CLAUDE.md compliance

⚠️ Verification caveat: every Bash call except git log was denied approval in this job, so I could not run pytest, bash -n, or exercise the parser. Everything below is from reading the code. If you want the parser findings confirmed by execution, re-run me with pytest/python3 allowed.

Verdict

The fix itself is right, and it's the right layer. One unset above the cd, at the point where the variables enter the process tree, covers pytest, everything pytest shells out to, and every dispatch arm — including one that reaches the suite without loading the root conftest. The rationale for placing it above the cd rather than just above pytest ("no window to reason about") is the correct call even though rev-parse --show-toplevel isn't itself at risk. conftest.py's addition of GIT_CONFIG_PARAMETERS keeps the inner layer in step, and reading the required set out of the conftest AST instead of hardcoding it is a genuinely good structural choice.

Both Greptile comments are stale — they were filed against 3d42001, and f80af03 ("Close two evasion paths") addresses them: _reintroduced_after now scans past the boundary for re-assignments, and function f { / f () { / split-brace spellings are all handled via state["func_pending"] and the () punctuation token, each with a negative control (lines 710–735, 889–892). No action needed on those two.

I found three gaps of my own in the parser, one of which is the same class of defect the file already treats as a negative control.

1. prev_sep is reset on every line, so a trailing && / || / | erases the gating context (P2)

tests/test_pre_commit_git_env.py:274 — _walk initialises prev_sep = None as a local, per call, i.e. per line. stack and state are deliberately carried across lines by the caller; prev_sep is not. Bash treats a line ending in &&, || or | as a continuation without needing a backslash, so:

[ -n "$SCRUB" ] &&
    unset GIT_DIR GIT_WORK_TREE …
cd "$(git rev-parse --show-toplevel)" || exit 1
python3 -m pytest -v

The first line closes [ -n "$SCRUB" ] with next_sep="&&" and produces no further statement. The next line's unset gets prev_sep=None, depth=0, next_sep=None → unconditional is True at line 453 and every name is credited. This is exactly the _UNSET_BEHIND_AND defect (line 738), just wrapped across a newline, and it isn't flagged: the line.endswith("\\") check at line 413 only catches backslash continuations, not &&-at-EOL. Same for | at EOL, which additionally hides the subshell.

Cheapest fix: carry prev_sep in the state dict alongside func_pending. Cheapest fail-closed fix: treat any line whose last token is in _SEPS as unmodellable, like the backslash case. Either way it wants a negative control for the multi-line && form. Fix this →

2. _OPAQUE is only enforced above the boundary (P2)

_scan reports source / eval / trap before the boundary (line 447), and _SOURCED_SCRUB is a negative control. But below the boundary, _reintroduced_after looks for assignments and exports only. So this passes:

unset GIT_DIR …
cd "$(git rev-parse --show-toplevel)" || exit 1
source ./scripts/env.sh          # re-exports GIT_DIR
python3 -m pytest -v

Everything below the boundary is still upstream of the dispatch — which is the entire justification the docstring gives for _reintroduced_after existing (lines 330–336). An opaque command there defeats the scrub just as completely as export GIT_DIR=…, which is caught. This is the surviving half of Greptile's first comment: the re-introduction pass closed the assignment path but not the opaque-command path. Fix this →

3. A backgrounded or piped { … } group is credited (P3)

_ENV_TRANSPARENT = {"group"} is correct in isolation — a brace group runs in the current shell — but the transparency is decided at the inner statement's command word, before the group's trailing separator is known. For { unset GIT_DIR …; } & the inner statement records depth=0, prev_sep=None, next_sep=";" and is credited, while the whole group actually ran in a background subshell and the hook keeps every variable. Same for { …; } | cat. Lower likelihood than #1, but it's the identical failure mode to _UNSET_BACKGROUNDED, which the file already considers worth a control. Fix this →

Non-blocking observations

Proportionality. 1067 lines of hand-rolled shell parser guarding a one-line unset is a real maintenance liability, and findings 1–3 are the predictable consequence: the parser is now itself a thing that can be wrong, and nothing tests it except the fixtures in the same file. The 25 negative controls and 6 positive controls are well chosen and the reasoning in the docstring is unusually good, so I'm not asking you to delete it — but note that the guard checks syntax, and there is no test that the hook, given a poisoned environment, actually hands a clean one to its children. tests/test_repo_guards.py does exactly that for conftest.py (spawns a child with a hostile GIT_DIR at a throwaway victim repo). A behavioural analogue would be short and would catch classes of defect the parser structurally cannot. Worth considering as a complement, not a replacement.

Four copies of the variable list. conftest.py's tuple, the hook's unset, _CANON, and the _GUARD1 literal. Two are pinned by tests, which is the point — but it means a purely cosmetic reflow of Guard 1 (line length, quote style) breaks _mutate's needle and reds 5 tests. The assertion message at line 1025 explains it clearly, so this is documented brittleness rather than a trap. Fine as-is; just be aware before running any formatter over conftest.py.

GIT_CONFIG_* family. Given the hook explicitly unsets names defensively that git does not export ("the rest of the names below are unset defensively"), GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, and the GIT_CONFIG_COUNT / GIT_CONFIG_KEY_<n> / GIT_CONFIG_VALUE_<n> trio are the same class as GIT_CONFIG_PARAMETERS — they inject config into every child git. Not incident-critical (none of them redirect a command at another repo), so I'd only add them if you want the defensive set to be internally consistent.

GIT_INDEX_VERSION omission is correctly reasoned — it selects an on-disk format, it can't redirect.

One-line if heuristic (line 425) fires on any line containing both an if and an fi token, including inside an unquoted echo. Fail-closed and negligible in practice.

Compliance

• Changelog: none required. CLAUDE.md scopes CHANGELOG.md updates to changes in a skill's behaviour; this touches only .githooks/, conftest.py, and repo-level tests/. ✅
• Install surface: no skills/ or maintainers/ files touched; the new test lives in the repo-level tests/. ✅
• main protection / no bulk add: branch PR, three explicit files. ✅
• Behaviour change on GIT_CONFIG_PARAMETERS: flagged in the PR body, and every helper in the suite passes its own -c user.email / -c user.name. Correctly called out.

None of findings 1–3 affect the runtime behaviour of the hook — the shipped unset is unconditional, at the top, and complete. They are all future-proofing gaps in the regression guard, so this is mergeable as-is if you'd rather land the fix and tighten the parser in a follow-up. My preference would be #1 before merge, since it's a one-line evasion that requires no backslash and no unusual shell. · branch issue-14/githooks-pre-commit-leaks