fix(hooks): run pre-commit pytest without git's repo-locating env
git exports GIT_DIR and GIT_INDEX_FILE to hooks. The pre-commit pytest step inherited them, and tests/dev/test_gen_config_deploy.py's scratch repos (git init --bare / clone / commit / push in tmp dirs) therefore ran against the repository being committed to: on 2026-10-27 a commit that staged infra files left its branch on nine "base / docs only / mc config" commits by author `t`, swapped the index for the fixture's tree, and wrote uploadpack.allowAnySHA1InWant into the .git/config that every worktree shares. It only stopped there because `git push origin` found no `origin` remote. - sem.hooks: the pytest step gets the hook env minus GIT_DIR, GIT_INDEX_FILE, GIT_WORK_TREE, GIT_PREFIX, GIT_COMMON_DIR and the object-dir variables. - test_gen_config_deploy: its git helper and detector runner drop all GIT_* from os.environ, so any wrapper that exports them is safe too. - Regression test: a nested `git init` under a hook-style GIT_DIR lands in its own directory. Refs #1365
This commit is contained in:
@@ -233,6 +233,27 @@ def compute_test_targets(
|
|||||||
return deduped, ", ".join(reasons) if reasons else "changed files"
|
return deduped, ", ".join(reasons) if reasons else "changed files"
|
||||||
|
|
||||||
|
|
||||||
|
# Variables git exports to hooks that pin every nested git call to *this*
|
||||||
|
# repository. Tests that build scratch repos in tmp dirs inherited them and
|
||||||
|
# committed into the repo being committed to (and wrote its shared config).
|
||||||
|
_HOOK_GIT_VARS = (
|
||||||
|
"GIT_DIR",
|
||||||
|
"GIT_INDEX_FILE",
|
||||||
|
"GIT_WORK_TREE",
|
||||||
|
"GIT_PREFIX",
|
||||||
|
"GIT_COMMON_DIR",
|
||||||
|
"GIT_OBJECT_DIRECTORY",
|
||||||
|
"GIT_ALTERNATE_OBJECT_DIRECTORIES",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _test_env() -> dict[str, str]:
|
||||||
|
"""The hook's environment minus git's repo-locating variables."""
|
||||||
|
import os
|
||||||
|
|
||||||
|
return {k: v for k, v in os.environ.items() if k not in _HOOK_GIT_VARS}
|
||||||
|
|
||||||
|
|
||||||
def run_step(label: str, cmd: list[str], **kwargs) -> int:
|
def run_step(label: str, cmd: list[str], **kwargs) -> int:
|
||||||
"""Run a subprocess step, printing the label."""
|
"""Run a subprocess step, printing the label."""
|
||||||
print(f"==> pre-commit: {label}")
|
print(f"==> pre-commit: {label}")
|
||||||
@@ -318,6 +339,7 @@ def main() -> int:
|
|||||||
["uv", "run", "python", "-m", "pytest"]
|
["uv", "run", "python", "-m", "pytest"]
|
||||||
+ targets
|
+ targets
|
||||||
+ ["--no-cov", "--tb=short", "-q", "-n", "auto"],
|
+ ["--no-cov", "--tb=short", "-q", "-n", "auto"],
|
||||||
|
env=_test_env(),
|
||||||
)
|
)
|
||||||
if rc != 0:
|
if rc != 0:
|
||||||
return rc
|
return rc
|
||||||
|
|||||||
@@ -104,9 +104,16 @@ def test_image_jobs_gate_on_changes_outputs_not_head_commit():
|
|||||||
# ── Behavioural: run the detector script against real git history ──────
|
# ── Behavioural: run the detector script against real git history ──────
|
||||||
|
|
||||||
|
|
||||||
|
def _clean_env() -> dict[str, str]:
|
||||||
|
"""os.environ without GIT_* — under a pre-commit hook GIT_DIR and
|
||||||
|
GIT_INDEX_FILE point at the real repo, and every scratch-repo command
|
||||||
|
here would commit into it instead."""
|
||||||
|
return {k: v for k, v in os.environ.items() if not k.startswith("GIT_")}
|
||||||
|
|
||||||
|
|
||||||
def _git(*args: str, cwd: Path) -> str:
|
def _git(*args: str, cwd: Path) -> str:
|
||||||
env = {
|
env = {
|
||||||
**os.environ,
|
**_clean_env(),
|
||||||
"GIT_AUTHOR_NAME": "t",
|
"GIT_AUTHOR_NAME": "t",
|
||||||
"GIT_AUTHOR_EMAIL": "t@example.invalid",
|
"GIT_AUTHOR_EMAIL": "t@example.invalid",
|
||||||
"GIT_COMMITTER_NAME": "t",
|
"GIT_COMMITTER_NAME": "t",
|
||||||
@@ -170,7 +177,7 @@ def _run_detector(checkout: Path, before: str, sha: str, tmp_path: Path) -> dict
|
|||||||
["bash", "-e", "-c", step["run"]],
|
["bash", "-e", "-c", step["run"]],
|
||||||
cwd=checkout,
|
cwd=checkout,
|
||||||
env={
|
env={
|
||||||
**os.environ,
|
**_clean_env(),
|
||||||
"BEFORE": before,
|
"BEFORE": before,
|
||||||
"GITHUB_SHA": sha,
|
"GITHUB_SHA": sha,
|
||||||
"GITHUB_OUTPUT": str(out_file),
|
"GITHUB_OUTPUT": str(out_file),
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ from sem.hooks import (
|
|||||||
_changed_test_dirs,
|
_changed_test_dirs,
|
||||||
_classify,
|
_classify,
|
||||||
_staged_files,
|
_staged_files,
|
||||||
|
_test_env,
|
||||||
_top_level_tests,
|
_top_level_tests,
|
||||||
check_syntax,
|
check_syntax,
|
||||||
compute_test_targets,
|
compute_test_targets,
|
||||||
@@ -195,6 +196,28 @@ class TestRunStep:
|
|||||||
assert rc == 1
|
assert rc == 1
|
||||||
|
|
||||||
|
|
||||||
|
class TestTestEnv:
|
||||||
|
def test_strips_repo_locating_git_vars(self, monkeypatch):
|
||||||
|
monkeypatch.setenv("GIT_DIR", "/repo/.git")
|
||||||
|
monkeypatch.setenv("GIT_INDEX_FILE", "/repo/.git/index")
|
||||||
|
monkeypatch.setenv("GIT_AUTHOR_NAME", "kept")
|
||||||
|
env = _test_env()
|
||||||
|
assert "GIT_DIR" not in env
|
||||||
|
assert "GIT_INDEX_FILE" not in env
|
||||||
|
assert env["GIT_AUTHOR_NAME"] == "kept"
|
||||||
|
|
||||||
|
def test_nested_git_sees_its_own_repo(self, tmp_path, monkeypatch):
|
||||||
|
"""Under a hook, `git init` in a tmp dir must not land in GIT_DIR."""
|
||||||
|
import subprocess
|
||||||
|
|
||||||
|
monkeypatch.setenv("GIT_DIR", str(tmp_path / "hooked.git"))
|
||||||
|
scratch = tmp_path / "scratch"
|
||||||
|
scratch.mkdir()
|
||||||
|
subprocess.run(["git", "init", "-q"], cwd=scratch, env=_test_env(), check=True)
|
||||||
|
assert (scratch / ".git").is_dir()
|
||||||
|
assert not (tmp_path / "hooked.git").exists()
|
||||||
|
|
||||||
|
|
||||||
class TestMain:
|
class TestMain:
|
||||||
@patch("sem.hooks._staged_files", return_value=[])
|
@patch("sem.hooks._staged_files", return_value=[])
|
||||||
def test_no_staged_files_returns_zero(self, mock_staged):
|
def test_no_staged_files_returns_zero(self, mock_staged):
|
||||||
|
|||||||
@@ -1470,7 +1470,7 @@ class TestSemHooksMainEntry:
|
|||||||
):
|
):
|
||||||
call_count = [0]
|
call_count = [0]
|
||||||
|
|
||||||
def step_side_effect(label, cmd):
|
def step_side_effect(label, cmd, **_kw):
|
||||||
call_count[0] += 1
|
call_count[0] += 1
|
||||||
if "notebook run" in label:
|
if "notebook run" in label:
|
||||||
return 1 # line 348
|
return 1 # line 348
|
||||||
|
|||||||
@@ -100,7 +100,7 @@ class TestSemHooksFormatCheckFails:
|
|||||||
|
|
||||||
call_count = [0]
|
call_count = [0]
|
||||||
|
|
||||||
def step_side_effect(label, cmd):
|
def step_side_effect(label, cmd, **_kw):
|
||||||
call_count[0] += 1
|
call_count[0] += 1
|
||||||
if "format" in label:
|
if "format" in label:
|
||||||
return 1
|
return 1
|
||||||
@@ -144,7 +144,7 @@ class TestSemHooksPytestFails:
|
|||||||
|
|
||||||
call_count = [0]
|
call_count = [0]
|
||||||
|
|
||||||
def step_side_effect(label, cmd):
|
def step_side_effect(label, cmd, **_kw):
|
||||||
call_count[0] += 1
|
call_count[0] += 1
|
||||||
if "pytest" in label:
|
if "pytest" in label:
|
||||||
return 1
|
return 1
|
||||||
@@ -170,7 +170,7 @@ class TestSemHooksNotebookFail:
|
|||||||
|
|
||||||
call_count = [0]
|
call_count = [0]
|
||||||
|
|
||||||
def step_side_effect(label, cmd):
|
def step_side_effect(label, cmd, **_kw):
|
||||||
call_count[0] += 1
|
call_count[0] += 1
|
||||||
if "marimo" in label:
|
if "marimo" in label:
|
||||||
return 1
|
return 1
|
||||||
|
|||||||
Reference in New Issue
Block a user