π§ chore: unify git hooks in .githooks/ β one location, no hand-copied duplicates - #193
Merged
Merged
Conversation
β¦ duplicates
All hooks now live in .githooks/ and are enabled with a single
`git config core.hooksPath .githooks`. Removes the copy-into-.git/hooks step
that had let the tracked template and the installed hook drift apart.
Why this was broken:
- scripts/pre-push documented its own install as `cp scripts/pre-push
.git/hooks/pre-push`, but the tracked copy had no customer-reference leak
scan. Following the documented install silently removed that guard.
- .githooks/post-checkout documented `git config core.hooksPath .githooks`,
which would have made git look only in .githooks/ β a directory that held
neither pre-commit nor pre-push. Enabling one hook would have disabled the
two that matter. core.hooksPath was in fact unset, so post-checkout had
never run at all.
Changes:
- .githooks/{pre-commit,pre-push,post-checkout} + README.md; scripts/pre-commit
and scripts/pre-push deleted (scripts/qc.ps1 stays).
- Customer patterns move to untracked .githooks/customer-patterns.local. The
list is itself the thing being hidden, so it must never be tracked. When it
is missing, pre-push prints a loud "SKIPPED β nothing was checked" warning
instead of passing silently: a guard that quietly stops running is worse
than no guard, because it is still trusted.
- pre-push skips the Rust QC gate when the branch changes no .rs/Cargo.* files
vs origin/develop. Compared against origin/develop rather than the pushed
ref range because stdin is already consumed by the master guard, and this
avoids the all-zeros remote_sha case for a new branch. Every uncertain case
runs the gate rather than skipping it.
- Leak scan uses `git grep -lIE` instead of `git ls-files | xargs grep`:
space-safe paths, no argument-length limit, no stderr to suppress.
- ci.yml: add chore/** to the push branch allowlist β same prefix-allowlist
footgun that left fix/** with no CI at all until PR #192.
- .gitattributes: drop the now-dead scripts/pre-commit eol=lf rule.
Master-push guard is unchanged.
Verified by running the hook directly: master push blocked (exit 1); normal
push passes with QC correctly skipped (exit 0); missing pattern file warns
loudly and exits 0; a planted customer reference in a tracked file blocks
(exit 1); the Rust-path filter matches src/foo.rs and Cargo.toml and not
docs/x.md, proving the skip can actually un-skip.
Review-fixes:
- [Important] `|| true` on the leak scan turned a git grep *failure* into a
clean pass, and the comment-stripper could manufacture that failure: an
inline `#` in a valid pattern like `Acme(NV#|BV)` was truncated to `Acme(NV`,
producing an invalid ERE that exits 128. One bad line poisoned the whole
alternation, so a real leak sailed through with "No customer references
detected." Now: only whole-line comments are stripped (a mid-line `#` stays
part of the pattern), and the git grep exit status is branched on β 0 blocks,
1 is clean, anything else prints the loud SKIPPED banner. One rule overall:
configuration problems warn, actual leaks block.
- [Important] RELEASING.md still documented `cp scripts/pre-commit
.git/hooks/pre-commit` β a copy of a file this commit deletes, into a
directory core.hooksPath makes git ignore. Replaced with the single
`git config core.hooksPath .githooks` install.
- [Important] The QC-skip regex missed `rust-toolchain.toml` and `.cargo/`,
both tracked here: a toolchain bump changes no .rs file yet flips fmt,
clippy and tests, so the gate was skipped for exactly the change most likely
to need it. Widened to cover rust-toolchain, .cargo/, scripts/qc.*, and
rustfmt/clippy.toml (the latter two not tracked today, listed so adding one
later cannot silently reopen the gap).
- [doc] README.md said `hooks git install` writes to `.git/hooks/`. It resolves
the target with `git rev-parse --git-path hooks`, so it honours
core.hooksPath and chains into an existing hook as a marker-delimited block.
Corrected; no code change needed.
Re-verified after the fixes: master push blocked (exit 1); normal push passes
with QC skipped (exit 0); an invalid regex in the pattern file now warns
loudly and exits 0 instead of reporting clean; a mid-line `#` survives
parsing; rust-toolchain.toml, .cargo/config.toml, scripts/qc.ps1, src/a.rs and
Cargo.lock all trigger the gate while docs-only paths do not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
flupkede
pushed a commit
that referenced
this pull request
Aug 5, 2026
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.
What
All git hooks now live in
.githooks/and are enabled by a single setting:No more copying files into
.git/hooks/.Why
Two independent traps, both of the same shape β an instruction that silently disables a guard:
scripts/pre-pushdocumented its own install ascp scripts/pre-push .git/hooks/pre-push, but the tracked copy had no customer-reference leak scan. Following the documented install silently removed that guard..githooks/post-checkoutdocumentedgit config core.hooksPath .githooks, which would have made git look only in.githooks/β a directory holding neitherpre-commitnorpre-push. Enabling one hook would have disabled the two that matter.core.hooksPathwas in fact unset, sopost-checkouthad never run at all.Changes
.githooks/{pre-commit,pre-push,post-checkout}+README.md.scripts/pre-commitandscripts/pre-pushdeleted;scripts/qc.ps1stays..githooks/customer-patterns.local. The list is the thing being hidden, so it must never be tracked.git grepfail all print a loudSKIPPED β nothing was checkedbanner and let the push through. A guard that stops running quietly is worse than no guard, because it is still trusted.pre-pushskips the Rust QC gate when the branch changes no Rust. Compared againstorigin/developrather than the pushed ref range β stdin is already consumed by the master guard, and this avoids the all-zerosremote_shacase for a new branch. Every uncertain case runs the gate.git grep -lIEinstead ofgit ls-files | xargs grep: space-safe paths, noARG_MAXlimit, no stderr to suppress.ci.yml: addchore/**to the push allowlist β the same prefix-allowlist footgun that leftfix/**with no CI at all until π fix: stop waking the scale-to-zero federated peer (and stop it self-warming afterwards)Β #192.RELEASING.mdno longer documents thecpinstall;README.md'shooks git installparagraph corrected (it resolves the target viagit rev-parse --git-path hooks, so it already honourscore.hooksPath).The master-push guard is functionally unchanged.
Verification
The hook was run directly against a throwaway repo, not just read:
master(incl. delete,HEAD:master, multi-ref)SKIPPED, exit 0SKIPPED, exit 0 β previously reported "clean"rust-toolchain.toml,.cargo/,scripts/qc.*,*.rs,Cargo.*This PR's own push exercised it end to end: QC correctly skipped, leak scan ran clean.
Two review rounds. Round 1 raised three Important findings β a
|| truethat turned agit grepfailure into a clean pass, a stalecpinstruction inRELEASING.md, and a QC-skip regex that missedrust-toolchain.tomland.cargo/. All three were fixed and amended into the single commit; round 2 passed with nothing new.Follow-up (not in this PR)
.claude/commands/merge.mdstill describes the pre-commit hook as bumping the patch version and rebuilding the binary. That stopped being true before this branch β the bump is now CI-side on PR merge. Its/mergeguardrail aborts onchore/*on that false premise, so it would refuse this very branch. Left alone deliberately: correcting it means changing merge-workflow behaviour, which deserves its own PR.