fix(coord): repair the git-hook installation model — ledger via pre-commit, plus a push guard#4
Merged
Merged
Conversation
…e hook
Two tools cannot both own .git/hooks/pre-commit. install-git-hooks.ps1 refused to overwrite a foreign
hook, so `pre-commit install` responded by renaming ours to pre-commit.legacy and calling it from its
own shim. That works on POSIX and FAILS ON WINDOWS -- pre-commit invokes the legacy hook from a Python
subprocess, which cannot resolve the /bin/sh shebang:
ExecutableNotFoundError: Executable `/bin/sh` not found
Measured 2026-07-27: EVERY commit in the repo was blocked until the shim was uninstalled. Note the
trap -- pre-commit.legacy EXISTED, so a presence check reported success. Only a real commit revealed it.
The ledger gate is now a `local` hook in .pre-commit-config.yaml (id: ledger-gate, always_run,
pass_filenames: false -- it inspects the staged TREE, not a file list). pre-commit owns the hook file
alone, so the contention is impossible rather than managed, and a fresh clone gets every gate from
`pre-commit install` with no hand-merged file to remember.
install-git-hooks.ps1 now:
* installs ONLY the claim gate (commit-msg -- it cannot move, since only commit-msg receives the
message file);
* MIGRATES an existing install by removing our old standalone pre-commit hook and its ledger_check.py
copy. Leaving them would make the next `pre-commit install` chain to .legacy and re-break commits;
* reports the real state under -Status, including a STALE leftover hook; and
* fails LOUD when pre-commit is absent, because the ledger gate's availability is now coupled to it.
That coupling is a genuine trade-off of this design, so it is announced rather than buried.
Verified by a real commit -- both gates observed firing, not inferred from a file existing.
The 17 existing tests exercise ledger_check.py's logic against a throwaway repo. None of them notices
if the gate is never invoked -- and that is exactly what just happened: the gate had to move out of
.git/hooks/pre-commit because two tools contend for that file and their chaining fails on Windows.
Every logic test stayed green throughout.
Two wiring assertions:
* the ledger-gate hook exists in .pre-commit-config.yaml, points at ledger_check.py, and carries
always_run + pass_filenames: false (it inspects the staged TREE, so without always_run a commit
touching no file it "owns" would skip the gate entirely);
* install-git-hooks.ps1 does NOT write a pre-commit hook again, and DOES still migrate an old
standalone install away -- otherwise the two-owner conflict returns and re-breaks every commit.
Mutation-verified: renaming the hook id reds it, and so does flipping always_run to false.
Uses a plain `import yaml`, deliberately not pytest.importorskip. pyyaml is pinned in
requirements.lock and constraints.lock so it is always present where CI runs; importorskip would turn
a missing dependency into a silent SKIP, which is the precise failure mode this test exists to catch.
Since the cutover this repository IS the published artifact. A push to main is publication, immediately, and cannot be taken back: deleting the ref later does not un-publish content that was fetched, mirrored or indexed in between. There is no publish step left to catch anything. Server-side branch protection requires a PR and 12 status checks, but enforce_admins is FALSE, so the repository owner bypasses all of it. The realistic trigger is not malice -- it is one click, because VS Code's Sync/Push button does not distinguish "my feature branch" from "main", and the editor is where most pushes originate. The old mirror clone carried a Gate-Provenance pre-push hook that covered this class; it was quarantined at cutover and nothing replaced it. Protects refs/heads/main and refs/heads/cla-signatures (written by the CLA action, never by a human -- and every PR wedges if it is damaged). Deletions are refused too: deleting main is worse than pushing to it. WHAT THIS IS NOT. A guardrail, not a boundary: --no-verify skips it and it is local-only, so another machine is unprotected. It removes the ACCIDENT, not the capability. The durable server-side fix is enforce_admins=true, deliberately NOT done yet: a known flaky test (BACKLOG #17) has now blocked two consecutive PRs, and removing the override while a flake can strand a merge would trade an accidental-push risk for a cannot-ship risk. Sequence it after #17. The escape hatch is MEFOR_ALLOW_DIRECT_PUSH=1, deliberately NOT --no-verify: a distinct variable is greppable in shell history and cannot be reached by the muscle memory that skips every other hook. Fails OPEN on malformed stdin. The guard exists to stop an accident; crashing on unexpected input would BE one. VERIFIED end-to-end, not by inspection: a real `git push origin <branch>:main` is REFUSED (exit 1), while the control -- pushing the branch to its own ref -- still succeeds. Eight tests drive the exact stdin contract git uses for a pre-push hook, plus a wiring test that the installer actually installs it; mutation-verified by disabling the protection and watching four go red.
wshallwshall
changed the title
Collaborator
Author
|
Merged with --admin, bypassing one required check: test (windows-2025, py3.14) failed twice at e4ebe05 with only tests/test_harness_monitor.py::test_monitor_observes_engine (BACKLOG #17, a known flake) — 9025 passed around it. windows-2022 and ubuntu both passed on the identical commit, this PR touches no harness/monitor code, and main is already red from the same test. All 11 other required contexts were verified green, zero non-required failures, head SHA re-verified as the tested one. Bypass scoped to that one context by name. |
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
1 participant
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.
Three commits fixing how this repo's git hooks are installed. The first two were forced by a real breakage; the third closes an exposure the cutover created.
1. Two tools cannot own
.git/hooks/pre-commitinstall-git-hooks.ps1installed the ledger gate there and refused to overwrite a foreign hook. Sopre-commit installdid the only thing it can — migration mode: rename ours topre-commit.legacyand call it from its own shim.That works on POSIX. It fails on Windows, because pre-commit invokes the legacy hook from a Python subprocess that cannot resolve
#!/bin/sh:Every commit in the repo was blocked until the shim was uninstalled.
The part worth remembering:
pre-commit.legacyexisted. A presence check — exactly what the runbook prescribed — reported success. Only a real commit revealed it.Fix: the ledger gate becomes a
localhook in.pre-commit-config.yaml(always_run,pass_filenames: false— it inspects the staged tree, not a file list). pre-commit owns the file alone, so the contention is impossible rather than managed, and a fresh clone gets every gate frompre-commit installwith nothing hand-merged to remember.install-git-hooks.ps1now installs only the claim gate (commit-msgcannot move — only it receives the message file), migrates an old install away, and fails loud when pre-commit is absent. That last point is a genuine trade-off, stated rather than buried: the ledger gate's availability is now coupled to pre-commit being installed.2. The tests were the actual gap
The 17 existing ledger tests exercise the logic against a throwaway repo. None notices if the gate is never invoked — they stayed green through the entire breakage. Two wiring assertions added, both mutation-verified.
3. Pre-push guard — refuse a direct push to
mainSince the cutover this repo is the published artifact. A push to
mainis publication, immediately and irreversibly. Branch protection requires a PR and 12 checks, butenforce_adminsis false, so the owner bypasses all of it — and VS Code's Sync button doesn't distinguishmainfrom a feature branch. The old mirror clone's Gate-Provenance pre-push hook covered this class; it was quarantined at cutover and nothing replaced it.Protects
mainandcla-signatures(written by the CLA action, never a human — and every PR wedges if it's damaged). Deletions refused too.What this is not: a guardrail, not a boundary.
--no-verifyskips it; it's local-only. It removes the accident, not the capability. The durable fix isenforce_admins=true, deliberately not done yet — BACKLOG #17 has blocked two consecutive PRs, and removing the override while a flake can strand a merge trades an accidental-push risk for a cannot-ship risk. Sequence it after #17.Escape hatch is
MEFOR_ALLOW_DIRECT_PUSH=1, deliberately not--no-verify: greppable in history, and not reachable by the muscle memory that skips every other hook.Verified by doing, not inspecting
pre-commit install, no.legacymainrefusedgit push <branch>:main→ exit 1always_runreds the wiring testsruff13 andmypy21 — both identical to a baseline measured onmain. Leak scan exit 0 with all 21 detectors.