fix(tests): unset inherited GIT_DIR/GIT_INDEX_FILE for hermetic fixture tests#614
Merged
Conversation
…ic fixtures git exports GIT_DIR/GIT_INDEX_FILE/GIT_WORK_TREE/GIT_COMMON_DIR/GIT_PREFIX into hook environments (e.g. lefthook pre-push). Fixture-building tests that run `git init`/`git worktree add` in a mktemp dir had those nested git commands hijacked back at the real repo when run from a hook — fixtures silently failed to build, assertions diverged from the standalone run, and stray commits could land on the live branch (surfaced in #587: 6/9 fail + 46 stray commits). Unset the five inherited git vars once, centrally, in init_test_framework so every current and future fixture test is hermetic regardless of how it was invoked. Remove the now-redundant per-test unset from test_golem_notify.sh (#587) and add tests/unit/test_framework_git_hermetic.sh, which exports a bogus GIT_DIR in a child shell and asserts both that the vars are cleared and that a fixture `git init` lands in the temp dir. Closes #599
Address pre-PR adversarial review findings: - Move the inherited git-env unset to framework.sh module scope so the framework bootstrap (the sub-module `source` calls) is protected too, not just test bodies. init_test_framework keeps a belt-and-suspenders re-clear. This also lets test_golem_notify.sh rely on the central guard during its own `source framework.sh`, closing the implicit-invariant gap. - Pass FRAMEWORK_SH through the environment instead of interpolating it into the `bash -c` string, and quote the static prefix, so a repo path containing a single quote can't break out of the source argument. - Use full paths for `env`/`bash` in run_in_polluted_env, matching the sibling test_golem_notify.sh convention (CLAUDE.md full-paths rule). - Add a positive assertion that the fixture gitdir resolves inside the temp dir, not merely "not the bogus path", closing a third-location gap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/framework.shdid not clear inherited git environment variables(
GIT_DIR,GIT_INDEX_FILE,GIT_WORK_TREE,GIT_COMMON_DIR,GIT_PREFIX).git exports these into hook environments (e.g. lefthook
pre-push), so any testthat builds a throwaway git fixture (
git init/git worktree addin amktempdir) had its nested git commands hijacked back at the real repowhen run from a hook — fixtures silently failed to build, assertions diverged
from the standalone run, and stray commits could land on the live branch
(surfaced in #587: 6/9 fail + 46 stray
initcommits).This clears the five inherited git vars once, centrally, at
framework.shmodule scope (before the sub-module
sourcecalls), so every current andfuture fixture test — and the framework bootstrap itself — is hermetic
regardless of how the suite is invoked (standalone,
run_unit_tests.sh, or agit hook).
init_test_frameworkkeeps a belt-and-suspenders re-clear.Changes
tests/framework.sh— unset inherited git env at module scope + re-clear ininit_test_framework.tests/unit/claude/test_golem_notify.sh— drop the now-redundant per-testunsetfrom fix(skills): golem-notify.sh records golem-? (issue number lost) when Notification fires outside worktree root #587 (proves the central guard suffices).tests/unit/test_framework_git_hermetic.sh— new regression test: exports a bogusGIT_DIRin a clean child shell, runsinit_test_framework, and asserts (a) all five vars are cleared and (b) a fixturegit initresolves inside the temp dir, never the inherited bogusGIT_DIR.Acceptance criteria
init_test_framework(and module scope) unsetsGIT_DIR,GIT_INDEX_FILE,GIT_WORK_TREE,GIT_COMMON_DIR,GIT_PREFIXbefore any test body runs.test_golem_notify.shpasses 9/9 underenv GIT_DIR=... GIT_INDEX_FILE=...with its per-testunsetremoved.GIT_DIR→ fixturegit initstill lands in the temp dir).Test plan
env GIT_DIR=$(git rev-parse --git-dir) GIT_INDEX_FILE=$(git rev-parse --git-path index) bash tests/unit/claude/test_golem_notify.sh→ 9/9.test_framework_git_hermetic.sh→ 2/2, standalone and under a polluted git env.update-versions.sh,test_pre_review_gates.sh,test_patterns_sh.sh) → Failed: 0 under hook env.Review
Adversarial pre-PR review (5 dimensions + judge + gatekeeper): 0 blocking, 7 deferrable. The genuinely valuable findings were resolved in this PR rather than deferred — module-scope guard placement (closes the bootstrap gap), env-var passing of the framework path (apostrophe-path quoting safety), full command paths for
env/bash, and a positive "gitdir inside temp dir" assertion. Two low-confidence findings were assessed as incorrect/low-value and discarded (one claimed outer-shell$tmpexpansion that the passing positive assertion disproves; one PATH-narrowing suggestion of marginal value).Closes #599