Stop the coverage unit tests committing into the developer's own repository - #1697
Merged
Merged
Conversation
test_coverage_unit.py builds a throwaway git repo with `git -C <tmpdir>`. That sets the working directory but does NOT override an inherited GIT_DIR or GIT_INDEX_FILE. Git exports both when it runs a hook. MFC's pre-commit hook runs precheck, precheck runs this suite (toolchain/bootstrap/lint.sh), so committing anything ran three `git commit` calls against the developer's own checkout instead of the temporary one. The result is three commits titled "map" on the current branch, the first of which deletes every file in the repository, because the helper's `git add -A` had replaced the index with a single-file tree. It is silent: pytest reports every test passing while it happens, and the only visible symptom is the in-flight commit dying with "cannot lock ref 'HEAD'" because the child commits moved HEAD underneath it. Observed on a real branch, which then pushed the deletion to its pull request. Scrub GIT_* from the environment for the four git invocations that back these fixtures, and add a regression test that pins GIT_DIR at a path git cannot write and asserts the commit still lands in the throwaway repo. Without the scrub that test fails with "/nonexistent.git: Permission denied"; the rest of the suite passes either way, which is the point.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Prevents coverage unit-test fixtures from accidentally running git operations against a developer’s real checkout when GIT_DIR/GIT_INDEX_FILE are inherited (e.g., via pre-commit hook execution), and adds a regression test to lock in the isolation behavior.
Changes:
- Introduced an environment scrubber to remove
GIT_*variables for git subprocess calls in test fixtures. - Updated fixture helpers to pass the scrubbed environment to git commands and the coverage-map guard script.
- Added a regression test ensuring throwaway repos remain isolated even when
GIT_DIR/GIT_INDEX_FILEare set.
Comment on lines
+499
to
+505
| with patch.dict(os.environ, {"GIT_DIR": "/nonexistent.git", "GIT_INDEX_FILE": "/nonexistent.index"}): | ||
| assert not [k for k in _env_without_git() if k.startswith("GIT_")] | ||
| with tempfile.TemporaryDirectory() as d: | ||
| repo = _repo_with_committed_map(d, {"k1": ["src/simulation/m_rhs.fpp"]}) | ||
| # The commit is in the throwaway repo, so it went nowhere near GIT_DIR. | ||
| log = subprocess.run(["git", "-C", str(repo), "log", "--oneline"], capture_output=True, text=True, check=True, env=_env_without_git()) | ||
| assert log.stdout.strip().endswith("map") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1697 +/- ##
=======================================
Coverage 61.04% 61.04%
=======================================
Files 83 83
Lines 20978 20978
Branches 3099 3099
=======================================
Hits 12807 12807
Misses 6126 6126
Partials 2045 2045 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sbryngelson
added a commit
to sbryngelson/MFC
that referenced
this pull request
Jul 29, 2026
1b95701 moved the f slug to rocm/7.0.2 as the OLCF-recommended pairing for cpe/26.03, and applied the same version to famd and amdfund. That broke the AMD lanes: amd/7.2.0 with rocm/7.0.2 makes Lmod fail the load outright. module load python cmake cpe/25.09 PrgEnv-amd amd/7.2.0 rocm/7.0.2 -> rc=1, 'These module(s) ... cannot be loaded as requested: darshan-runtime' module load python cmake cpe/25.09 PrgEnv-amd amd/7.2.0 rocm/7.2.0 -> rc=0 modules.sh issues all modules in a single 'module load', so a failure loses every module including python -- which is why CI reported 'Python 3.6.15 (python3) is out of date. Required >= 3.9.' rather than anything about ROCm. Six AMD lanes failed (gpu-omp x3, cpu x2, Case Opt) while the same lanes are green on master. The 7.0.2 pairing rationale is specific to cpe/26.03 on the CCE slug, so keep it there and leave famd/amdfund on the version their compilers pair with. Verified both slugs now load rc=0. Committed with --no-verify: this branch does not yet carry the fix from MFlowCode#1697, so the pre-commit hook's pytest run would commit into the repository. Precheck was run manually and passed 7/7.
sbryngelson
added a commit
to sbryngelson/MFC
that referenced
this pull request
Jul 29, 2026
Brings in MFlowCode#1697 (toolchain unit tests no longer commit into the contributor's own repository when run from the pre-commit hook) and MFlowCode#1696 (build dependency/configure skips keyed on real state rather than CMakeCache.txt). MFlowCode#1697 matters for this branch specifically: until now a commit here with the hook installed could have pytest rewrite the branch, which is why the previous two commits used --no-verify.
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
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.
The coverage unit tests build a throwaway git repo with
git -C <tmpdir>. That sets the working directory but does not override an inheritedGIT_DIR/GIT_INDEX_FILE.Git exports both when it runs a hook. Our pre-commit hook runs
precheck, and precheck runs this suite (toolchain/bootstrap/lint.sh:36). So on any machine using the hook we tell contributors to use,git commitran threegit commitcalls against the contributor's own checkout.The damage:
The first one deletes every file in the repository, because the helper's
git add -Ahad already replaced the index with its single-file tree.It is silent. pytest reports every test passing while it happens. The only visible symptom is that the commit you were making dies with
fatal: cannot lock ref 'HEAD': is at <x> but expected <y>— because the child commits moved HEAD out from under it. I hit this on a real branch and force-pushed the 2090-file deletion to its PR before noticing.The fix
Scrub
GIT_*from the environment for the four git invocations backing these fixtures.Verification
Reproduced and confirmed in a sandbox, simulating the hook environment exactly (
GIT_DIR/GIT_INDEX_FILEexported at a scratch repo, then running the suite):mapcommits appear58 passed59 passedThe added regression test pins
GIT_DIRat a path git cannot write and asserts the commit still lands in the throwaway repo. Without the scrub it fails with/nonexistent.git: Permission denied; the rest of the suite passes either way, which is exactly why this went unnoticed.Final end-to-end check: committing this PR itself with the hook enabled moved HEAD exactly once.
Merge order
Independent of my other open PRs — no shared files. Worth taking first regardless, since without it any contributor running the hook can silently rewrite their branch.