toolchain: precheck passes under the pre-commit hook from a worktree - #1807
Merged
sbryngelson merged 4 commits intoSep 4, 2026
Merged
Conversation
…eck passes from worktrees
Contributor
There was a problem hiding this comment.
Warning
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.
Pull request overview
Fixes failures when running the coverage toolchain under a git hook launched from a worktree by ensuring git subprocesses ignore inherited GIT_* hook environment variables and operate on the intended repository directory.
Changes:
- Centralized “scrub
GIT_*from environment” logic intomfc.test.coverageand applied it to the shared git runner. - Updated
check_coverage_map_health.pyto route its git invocations through the centralized helper. - Removed the duplicated env-scrub helper from
test_coverage_unit.pyand imported the shared one instead.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
toolchain/mfc/test/test_coverage_unit.py |
Removes local _env_without_git helper and imports the shared implementation from mfc.test.coverage. |
toolchain/mfc/test/coverage.py |
Introduces _env_without_git() and applies it in _git() so git calls don’t inherit hook-provided GIT_* variables. |
.github/scripts/check_coverage_map_health.py |
Switches raw subprocess.run(["git", ...]) calls to use the shared _git() helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1807 +/- ##
=======================================
Coverage 62.26% 62.26%
=======================================
Files 84 84
Lines 21558 21558
Branches 3188 3188
=======================================
Hits 13423 13423
Misses 5937 5937
Partials 2198 2198 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…_git for the health script
sbryngelson
added a commit
that referenced
this pull request
Sep 4, 2026
Nothing marked an outage any more once the PyPI classifier went, so the two remaining check call sites could only ever act on a stale marker -- which is exactly what happened: a marker written before that fix landed kept failing the CCE cpu lane afterwards. The design decision behind removing it rather than repairing it: the breaker converts a local failure into a global one, and every recorded instance of it firing was a false positive caused by something else (a syscheck install timeout, a compile error, and #1813's retry deleting build/venv so attempt 2 could not reach PyPI). It reddened #1805, #1807 and #1811 for twenty minutes at a time. Meanwhile the cost it was protecting against is small: Frontier fetches dependencies on the login node, before any allocation is committed. Removes ci-outage.sh, both check call sites, the exit-78 relay through monitor_slurm_job.sh and run_monitored_slurm_job.sh, and the tests that pinned all of it. The node-fault path (77) is untouched and still covered -- 28 tests across preflight, monitor and requeue still pass. Net -319 lines.
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.
Committing from a git worktree fails the pre-commit hook: git exports
GIT_DIRandGIT_INDEX_FILEto hooks, and neithercwdnorgit -Coverrides them, so the throwaway repositories intest_coverage_unit.pyend up querying the committing repository instead. Two tests fail (test_verified_after_last_change_*) and the commit is blocked, leaving--no-verifyas the only way through.The test file already scrubbed
GIT_*for its own setup calls. This moves that scrub intocoverage._gitand routes the three raw git calls incheck_coverage_map_health.pythrough it, so every git subprocess the coverage machinery makes targets the directory it was given.Verified by making this commit through the hook from a worktree: precheck passes, the two tests pass under the hook's environment, and the full toolchain suite is 583 passed.