Skip to content

πŸ”§ chore: unify git hooks in .githooks/ β€” one location, no hand-copied duplicates - #193

Merged
flupkede merged 1 commit into
developfrom
chore/unify-git-hooks
Aug 5, 2026
Merged

πŸ”§ chore: unify git hooks in .githooks/ β€” one location, no hand-copied duplicates#193
flupkede merged 1 commit into
developfrom
chore/unify-git-hooks

Conversation

@flupkede

@flupkede flupkede commented Aug 5, 2026

Copy link
Copy Markdown
Owner

What

All git hooks now live in .githooks/ and are enabled by a single setting:

git config core.hooksPath .githooks

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-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 holding 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, gitignored .githooks/customer-patterns.local. The list is the thing being hidden, so it must never be tracked.
  • One rule for the scan: configuration problems warn, actual leaks block. A missing file, an empty file, or a pattern that makes git grep fail all print a loud SKIPPED β€” nothing was checked banner and let the push through. A guard that stops running quietly is worse than no guard, because it is still trusted.
  • pre-push skips the Rust QC gate when the branch changes no Rust. Compared against origin/develop rather than the pushed ref range β€” 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.
  • Leak scan uses git grep -lIE instead of git ls-files | xargs grep: space-safe paths, no ARG_MAX limit, no stderr to suppress.
  • ci.yml: add chore/** to the push allowlist β€” the same prefix-allowlist footgun that left fix/** with no CI at all until πŸ› fix: stop waking the scale-to-zero federated peer (and stop it self-warming afterwards)Β #192.
  • RELEASING.md no longer documents the cp install; README.md's hooks git install paragraph corrected (it resolves the target via git rev-parse --git-path hooks, so it already honours core.hooksPath).

The master-push guard is functionally unchanged.

Verification

The hook was run directly against a throwaway repo, not just read:

Case Result
Push to master (incl. delete, HEAD:master, multi-ref) blocked, exit 1
Normal branch push exit 0, QC skipped, scan clean
Pattern file missing / empty loud SKIPPED, exit 0
Invalid regex in pattern file, real leak present loud SKIPPED, exit 0 β€” previously reported "clean"
Planted customer reference in a tracked file blocked, exit 1
rust-toolchain.toml, .cargo/, scripts/qc.*, *.rs, Cargo.* gate runs
Docs-only paths gate skipped

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 || true that turned a git grep failure into a clean pass, a stale cp instruction in RELEASING.md, and a QC-skip regex that missed rust-toolchain.toml and .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.md still 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 /merge guardrail aborts on chore/* 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.

… 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
flupkede merged commit e24436f into develop Aug 5, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant