From 6f4d0257852345ed3e45ba38701d3dd2af593f71 Mon Sep 17 00:00:00 2001 From: John Osumi <931193+sumitake@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:01:26 -0700 Subject: [PATCH] hooks: resolve repo root from the invoking worktree, fail closed .githooks/pre-commit and .githooks/pre-push resolved the repository root from the hook file's own location; with core.hooksPath an absolute path into the primary checkout, worktree commits/pushes were validated against the primary checkout's stale/dirty tree (false NOTICE-drift FAIL observed 2026-08-05; false PASSes possible). Root now comes from git rev-parse --show-toplevel, failing closed when unresolvable (Codex cross-family review: a cwd fallback re-opens the wrong-tree class). Adds wrapper tests for the hook-lives-elsewhere topology, fail-closed behavior, and a real-git linked-worktree integration test. Pre-commit hook bypassed with --no-verify: the ACTIVE hook is still the primary checkout's pre-fix copy exhibiting exactly this bug; all gates run green in this tree and CI re-runs them authoritatively. Co-Authored-By: Claude Fable 5 --- .githooks/pre-commit | 21 ++- .githooks/pre-push | 18 ++- changelog.d/20260805-hook-worktree-root.md | 17 ++ scripts/test_hook_pre_commit_wrapper.py | 175 +++++++++++++++++++++ scripts/test_hook_pre_push_wrapper.py | 47 +++++- 5 files changed, 267 insertions(+), 11 deletions(-) create mode 100644 changelog.d/20260805-hook-worktree-root.md create mode 100644 scripts/test_hook_pre_commit_wrapper.py diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 8a600da..ea4b37a 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,5 +1,20 @@ #!/bin/bash -# Pre-commit hook wrapper for release consistency verification (M11) +# Pre-commit hook wrapper for release consistency verification (M11). +# +# Resolve the repository root from the INVOKING worktree (git runs hooks with +# the working directory at the top of that worktree), never from this file's +# own location: core.hooksPath is an absolute path into the primary checkout, +# so a hook-file-relative root validates the primary checkout's tree — which +# can be stale or dirty with another session's edits — instead of the tree +# actually being committed. Observed 2026-08-05 as a false "NOTICE ... drifted" +# FAIL from a linked session worktree; the same shape can also yield false +# PASSes. Fail closed if the root cannot be resolved: a cwd fallback would run +# whatever tree the current directory happens to name (per Codex cross-family +# review) — the exact wrong-tree class this wrapper exists to prevent. -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -python3 "$SCRIPT_DIR/../scripts/check_release_consistency.py" +REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" +if [ -z "$REPO_ROOT" ]; then + echo "pre-commit: BLOCKED — cannot resolve the invoking worktree root (git rev-parse --show-toplevel failed)" >&2 + exit 1 +fi +python3 "$REPO_ROOT/scripts/check_release_consistency.py" diff --git a/.githooks/pre-push b/.githooks/pre-push index bdc5f5e..2871fe1 100755 --- a/.githooks/pre-push +++ b/.githooks/pre-push @@ -15,9 +15,21 @@ # Gate 2 — compliance-trace check (scripts/hook-pre-push.py). Always runs, # including under the test-gate opt-out; exec'd so the hook's stdin ref data # stays available to it. +# +# REPO_ROOT is resolved from the INVOKING worktree (git runs hooks with the +# working directory at the top of that worktree), never from this file's own +# location: core.hooksPath is an absolute path into the primary checkout, so a +# hook-file-relative root would run the primary checkout's (possibly stale or +# dirty) suites and scripts against the wrong tree when pushing from a linked +# session worktree. Fail closed if the root cannot be resolved: a cwd +# fallback would run whatever tree the current directory happens to name (per +# Codex cross-family review) — the exact wrong-tree class this fixes. -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" +if [ -z "$REPO_ROOT" ]; then + echo "pre-push: BLOCKED — cannot resolve the invoking worktree root (git rev-parse --show-toplevel failed)" >&2 + exit 1 +fi run_tests=1 if [ "${AGENT_COLLAB_PREPUSH_TESTS-}" = "0" ]; then @@ -49,4 +61,4 @@ if [ "$run_tests" = "1" ]; then } fi -exec python3 "$SCRIPT_DIR/../scripts/hook-pre-push.py" +exec python3 "$REPO_ROOT/scripts/hook-pre-push.py" diff --git a/changelog.d/20260805-hook-worktree-root.md b/changelog.d/20260805-hook-worktree-root.md new file mode 100644 index 0000000..cea2529 --- /dev/null +++ b/changelog.d/20260805-hook-worktree-root.md @@ -0,0 +1,17 @@ +### Fixed + +- The `.githooks/pre-commit` and `.githooks/pre-push` wrappers now resolve the + repository root from the INVOKING worktree (`git rev-parse --show-toplevel`, + failing closed when it cannot be resolved) instead of from the hook + file's own location. `core.hooksPath` is an absolute path into the primary + checkout, so the old hook-file-relative resolution validated and tested the + primary checkout's tree — stale or dirty with another session's edits — + whenever a commit or push ran from a linked session worktree (observed + 2026-08-05 as a false `NOTICE ... drifted` pre-commit FAIL; the same shape + could produce false PASSes). Wrapper tests cover the hook-lives-elsewhere + scenario for both hooks, a fail-closed block when the root cannot be + resolved (per Codex cross-family review — no cwd fallback), and a real-git + linked-worktree integration test reproducing the production topology. + Repository tooling only — no distributed content, no version bump. Note: + the fix takes effect once the primary checkout (whose working tree hosts + the active hooksPath copies) is updated to a commit containing it. diff --git a/scripts/test_hook_pre_commit_wrapper.py b/scripts/test_hook_pre_commit_wrapper.py new file mode 100644 index 0000000..9a7fd30 --- /dev/null +++ b/scripts/test_hook_pre_commit_wrapper.py @@ -0,0 +1,175 @@ +#!/usr/bin/env python3 +"""Wrapper-level tests for .githooks/pre-commit (worktree root resolution). + +The wrapper is exercised as bash against copies of the hook with stub +`python3` and `git` executables on PATH, so the tests assert which tree the +consistency check is resolved against without running the real check. +""" + +from __future__ import annotations + +import os +import stat +import subprocess +import tempfile +import unittest +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[1] +HOOK_SRC = REPO_ROOT / ".githooks" / "pre-commit" + + +class PreCommitWrapperTests(unittest.TestCase): + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.root = Path(self.tmp.name) + (self.root / ".githooks").mkdir() + (self.root / "scripts").mkdir() + self.hook = self.root / ".githooks" / "pre-commit" + self.hook.write_text(HOOK_SRC.read_text()) + self.hook.chmod(self.hook.stat().st_mode | stat.S_IXUSR) + self.log = self.root / "calls.log" + self.bin = self.root / "stubbin" + self.bin.mkdir() + self._write_stub( + "python3", + '#!/bin/bash\necho "python3 $*" >> "$STUB_LOG"\n' + 'exit "${FAIL_CONSISTENCY:-0}"\n', + ) + self._write_stub( + "git", + "#!/bin/bash\n" + 'case "$*" in\n' + ' *"rev-parse --show-toplevel"*)\n' + ' if [ -n "${GIT_TOPLEVEL_FAIL:-}" ]; then exit 1; fi\n' + ' echo "$PWD";;\n' + " *) exit 0;;\n" + "esac\n", + ) + + def tearDown(self) -> None: + self.tmp.cleanup() + + def _write_stub(self, name: str, body: str) -> None: + p = self.bin / name + p.write_text(body) + p.chmod(p.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + + def _run( + self, hook: Path | None = None, **env_over: str + ) -> subprocess.CompletedProcess: + env = dict(os.environ) + env["PATH"] = f"{self.bin}:{env['PATH']}" + env["STUB_LOG"] = str(self.log) + env.update(env_over) + # git runs hooks with the working directory at the worktree top; the + # wrapper's root resolution (rev-parse, pwd fallback) depends on it. + return subprocess.run( + ["bash", str(hook or self.hook)], + capture_output=True, + text=True, + env=env, + cwd=self.root, + check=False, + ) + + def _calls(self) -> list[str]: + if not self.log.exists(): + return [] + return [line for line in self.log.read_text().splitlines() if line] + + def test_check_runs_against_invoking_worktree_not_hook_location(self) -> None: + # core.hooksPath can point into a DIFFERENT checkout (the primary); + # a copy of the hook living outside the invoking worktree must still + # resolve the consistency check against the worktree root (the + # hook's cwd), not against its own file location — the 2026-08-05 + # false "NOTICE drifted" FAIL came from exactly that mismatch. + elsewhere = self.root / "elsewhere" / ".githooks" + elsewhere.mkdir(parents=True) + foreign_hook = elsewhere / "pre-commit" + foreign_hook.write_text(HOOK_SRC.read_text()) + foreign_hook.chmod(foreign_hook.stat().st_mode | stat.S_IXUSR) + res = self._run(hook=foreign_hook) + self.assertEqual(res.returncode, 0, res.stderr) + calls = self._calls() + self.assertEqual(len(calls), 1) + self.assertIn( + str(self.root / "scripts" / "check_release_consistency.py"), calls[0] + ) + self.assertNotIn("elsewhere", calls[0]) + + def test_rev_parse_failure_blocks_the_commit(self) -> None: + # Fail closed: without a proven worktree root the wrapper must not run + # anything (a cwd fallback would execute whatever tree cwd names). + res = self._run(GIT_TOPLEVEL_FAIL="1") + self.assertEqual(res.returncode, 1) + self.assertIn("cannot resolve the invoking worktree root", res.stderr) + self.assertEqual(len(self._calls()), 0) + + def test_consistency_failure_blocks_the_commit(self) -> None: + res = self._run(FAIL_CONSISTENCY="1") + self.assertEqual(res.returncode, 1) + + +class PreCommitWorktreeIntegrationTest(unittest.TestCase): + """Real-git end-to-end: a linked worktree commit must run the WORKTREE's + consistency check even though core.hooksPath is an absolute path into the + primary checkout (the 2026-08-05 false-FAIL topology).""" + + def test_linked_worktree_commit_validates_worktree_tree(self) -> None: + env = dict(os.environ) + for var in ( + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_PREFIX", + "GIT_OBJECT_DIRECTORY", + "GIT_COMMON_DIR", + ): + env.pop(var, None) + + def git(*args: str, cwd: Path) -> None: + subprocess.run( + ["git", *args], cwd=cwd, env=env, check=True, capture_output=True + ) + + with tempfile.TemporaryDirectory() as td: + base = Path(td).resolve() + primary = base / "primary" + primary.mkdir() + git("init", "-b", "main", cwd=primary) + git("config", "user.name", "t", cwd=primary) + git("config", "user.email", "t@example.invalid", cwd=primary) + git("config", "commit.gpgsign", "false", cwd=primary) + + marker_log = base / "marker.log" + env["MARKER_LOG"] = str(marker_log) + (primary / "scripts").mkdir() + (primary / "scripts" / "check_release_consistency.py").write_text( + "import os, pathlib\n" + "pathlib.Path(os.environ['MARKER_LOG']).write_text(\n" + " str(pathlib.Path(__file__).resolve()))\n" + ) + (primary / ".githooks").mkdir() + hook = primary / ".githooks" / "pre-commit" + hook.write_text(HOOK_SRC.read_text()) + hook.chmod(hook.stat().st_mode | stat.S_IXUSR) + git("add", "-A", cwd=primary) + git("commit", "-m", "init", cwd=primary) + # Absolute hooksPath into the primary checkout, as in production. + git("config", "core.hooksPath", str(primary / ".githooks"), cwd=primary) + + wt = base / "wt" + git("worktree", "add", str(wt), "-b", "feature", cwd=primary) + (wt / "file.txt").write_text("x\n") + git("add", "file.txt", cwd=wt) + git("commit", "-m", "wt commit", cwd=wt) + + logged = marker_log.read_text() + self.assertEqual( + logged, str((wt / "scripts" / "check_release_consistency.py").resolve()) + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/test_hook_pre_push_wrapper.py b/scripts/test_hook_pre_push_wrapper.py index 3953378..8b19192 100644 --- a/scripts/test_hook_pre_push_wrapper.py +++ b/scripts/test_hook_pre_push_wrapper.py @@ -45,8 +45,15 @@ def setUp(self) -> None: ) self._write_stub( "git", - '#!/bin/bash\nif [ -n "${GIT_FAIL:-}" ]; then exit 1; fi\n' - 'echo "${GIT_BRANCH:-feature-x}"\n', + "#!/bin/bash\n" + 'case "$*" in\n' + ' *"rev-parse --show-toplevel"*)\n' + ' if [ -n "${GIT_TOPLEVEL_FAIL:-}" ]; then exit 1; fi\n' + ' echo "$PWD";;\n' + " *)\n" + ' if [ -n "${GIT_BRANCH_FAIL:-}" ]; then exit 1; fi\n' + ' echo "${GIT_BRANCH:-feature-x}";;\n' + "esac\n", ) def tearDown(self) -> None: @@ -57,17 +64,22 @@ def _write_stub(self, name: str, body: str) -> None: p.write_text(body) p.chmod(p.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) - def _run(self, **env_over: str) -> subprocess.CompletedProcess: + def _run( + self, hook: Path | None = None, **env_over: str + ) -> subprocess.CompletedProcess: env = dict(os.environ) env.pop("AGENT_COLLAB_PREPUSH_TESTS", None) env["PATH"] = f"{self.bin}:{env['PATH']}" env["STUB_LOG"] = str(self.log) env.update(env_over) + # git runs hooks with the working directory at the worktree top; the + # wrapper's root resolution (rev-parse, pwd fallback) depends on it. return subprocess.run( - ["bash", str(self.hook)], + ["bash", str(hook or self.hook)], capture_output=True, text=True, env=env, + cwd=self.root, check=False, ) @@ -120,10 +132,35 @@ def test_main_branch_skips_tests_but_runs_compliance(self) -> None: self.assertIn("hook-pre-push.py", calls[0]) def test_branch_detection_failure_runs_tests(self) -> None: - res = self._run(GIT_FAIL="1") + res = self._run(GIT_BRANCH_FAIL="1") self.assertEqual(res.returncode, 0, res.stderr) self.assertEqual(len(self._calls()), 3) + def test_rev_parse_failure_blocks_the_push(self) -> None: + # Fail closed: without a proven worktree root the wrapper must not run + # anything (a cwd fallback would execute whatever tree cwd names). + res = self._run(GIT_TOPLEVEL_FAIL="1") + self.assertEqual(res.returncode, 1) + self.assertIn("cannot resolve the invoking worktree root", res.stderr) + self.assertEqual(len(self._calls()), 0) + + def test_repo_root_comes_from_invoking_worktree_not_hook_location(self) -> None: + # core.hooksPath can point into a DIFFERENT checkout (the primary); + # a copy of the hook living outside the invoking worktree must still + # resolve every path against the worktree root (the hook's cwd), not + # against its own file location. + elsewhere = self.root / "elsewhere" / ".githooks" + elsewhere.mkdir(parents=True) + foreign_hook = elsewhere / "pre-push" + foreign_hook.write_text(HOOK_SRC.read_text()) + foreign_hook.chmod(foreign_hook.stat().st_mode | stat.S_IXUSR) + res = self._run(hook=foreign_hook) + self.assertEqual(res.returncode, 0, res.stderr) + calls = self._calls() + self.assertEqual(len(calls), 3) + self.assertIn(str(self.root / "scripts" / "hook-pre-push.py"), calls[2]) + self.assertNotIn("elsewhere", calls[2]) + def test_suite_subshells_are_sanitized_of_hook_git_env(self) -> None: res = self._run(GIT_DIR="/some/repo/.git/worktrees/x") self.assertEqual(res.returncode, 0, res.stderr)