chore(hooks): restore the canonical git hooks - #57
Conversation
The hooks here predated the current shim architecture, and their headers had also been rewritten from `ResQ Software` to `ResQ Systems, Inc.`, so none matched the SHA-256 digests `install-hooks.sh` verifies before writing them. The rewrite was not a local edit: `resq copyright` defaults to the second author string and treats the difference as a mismatch, stripping each header and writing its own. Every repository in the org carrying hooks had drifted the same way. resq-software/crates#170 stops it at the source; this restores the files and sets `core.hooksPath`, without which the hooks never ran at all. Written by `resq hooks update`, so any `local-*` override is untouched. Verified byte-identical to `crates/resq-cli/templates/git-hooks`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThe six Git hooks now use canonical shell shims. Commit hooks normalize and validate messages, validation hooks delegate checks and enforce branch policies, and checkout or merge hooks report lockfile changes. Executable repository-local hooks can extend each default hook. Git hook standardization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.git-hooks/prepare-commit-msg (1)
13-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEarly exits skip the local hook delegation.
Lines 14 and 18 return before the delegation block at lines 28-31.
local-prepare-commit-msgtherefore never runs for merge, squash, message, or amend commits, and never runs on a detached HEAD. The other five hooks always reach theirlocal-*delegation. If the shim contract is "the local hook always runs", replace these exits with a skip of only the ticket logic.♻️ Proposed restructure
-case "$COMMIT_SOURCE" in - merge|squash|message|commit) exit 0 ;; -esac - -BRANCH=$(git symbolic-ref --short HEAD 2>/dev/null || echo "") -[ -z "$BRANCH" ] && exit 0 - -TICKET=$(grep -oE '[A-Z]{2,}-[0-9]+' <<<"$BRANCH" | head -1 || true) -if [ -n "$TICKET" ]; then +TICKET="" +case "$COMMIT_SOURCE" in + merge|squash|message|commit) ;; + *) + BRANCH=$(git symbolic-ref --short HEAD 2>/dev/null || echo "") + [ -n "$BRANCH" ] && TICKET=$(grep -oE '[A-Z]{2,}-[0-9]+' <<<"$BRANCH" | head -1 || true) + ;; +esac + +if [ -n "$TICKET" ]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.git-hooks/prepare-commit-msg around lines 13 - 18, Update the prepare-commit-msg flow around COMMIT_SOURCE and BRANCH so these conditions skip only ticket-processing logic rather than exiting the hook; ensure local-prepare-commit-msg is still delegated for merge, squash, message, amend, and detached-HEAD commits, consistently with the other hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.git-hooks/commit-msg:
- Around line 12-15: Validate the commit-message file path immediately after
assigning INPUT_FILE in .git-hooks/commit-msg#L12-L15, exiting with a clear
error when it is empty or not a regular file before head runs; apply the same
validation to COMMIT_MSG_FILE in .git-hooks/prepare-commit-msg#L20-L26 before
cat executes.
In @.git-hooks/post-checkout:
- Around line 19-22: Update the bun lockfile checks in .git-hooks/post-checkout
lines 19-22 and .git-hooks/post-merge lines 12-17 to use grep -qE with the
^bun\.lockb?$ extended-regex pattern, replacing the non-portable BRE \?. Apply
the same change at both affected sites.
- Around line 16-23: Update .git-hooks/README.md to document that post-checkout
only prints resynchronization hints, including the relevant commands, instead of
automatically running bun install. Remove or revise references to the removed
inline pre-commit and pre-push logic so the README matches the current hook
behavior.
In @.git-hooks/pre-push:
- Around line 65-72: Update the local hook delegation around LOCAL_HOOK to pipe
PUSH_REFS with printf rather than using the unquoted heredoc. Ensure empty
PUSH_REFS produces no input, preserves ref data without escape-sequence
processing, and retains propagation of the local hook’s non-zero status under
pipefail.
---
Nitpick comments:
In @.git-hooks/prepare-commit-msg:
- Around line 13-18: Update the prepare-commit-msg flow around COMMIT_SOURCE and
BRANCH so these conditions skip only ticket-processing logic rather than exiting
the hook; ensure local-prepare-commit-msg is still delegated for merge, squash,
message, amend, and detached-HEAD commits, consistently with the other hooks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe347ab9-6013-46de-b22b-92fa92e59bf5
📒 Files selected for processing (6)
.git-hooks/commit-msg.git-hooks/post-checkout.git-hooks/post-merge.git-hooks/pre-commit.git-hooks/pre-push.git-hooks/prepare-commit-msg
| INPUT_FILE="${1:-}" | ||
| PATTERN="^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?(!)?: .+$" | ||
|
|
||
| # Validate only the first line (subject). Strip an optional [TICKET-123] prefix | ||
| # inserted by prepare-commit-msg so the two hooks don't conflict. | ||
| FIRST_LINE=$(head -1 "$INPUT_FILE") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
${1:-} defaults do not protect the file read. Both commit-message hooks default the message-file path to an empty string, then pass that empty path to a command. The command fails, and set -euo pipefail aborts the hook with a raw tool error instead of a clear diagnostic. Validate the path once, immediately after assignment.
.git-hooks/commit-msg#L12-L15: after line 12, exit with an explicit error when$INPUT_FILEis empty or is not a regular file, beforehead -1runs..git-hooks/prepare-commit-msg#L20-L26: apply the same check to$COMMIT_MSG_FILEbefore thecaton line 22.
📍 Affects 2 files
.git-hooks/commit-msg#L12-L15(this comment).git-hooks/prepare-commit-msg#L20-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.git-hooks/commit-msg around lines 12 - 15, Validate the commit-message file
path immediately after assigning INPUT_FILE in .git-hooks/commit-msg#L12-L15,
exiting with a clear error when it is empty or not a regular file before head
runs; apply the same validation to COMMIT_MSG_FILE in
.git-hooks/prepare-commit-msg#L20-L26 before cat executes.
| if [ "$IS_BRANCH_CHECKOUT" = "1" ] && [ "$PREV_HEAD" != "$NEW_HEAD" ]; then | ||
| CHANGED=$(git diff --name-only "$PREV_HEAD" "$NEW_HEAD" 2>/dev/null || true) | ||
|
|
||
| # Skip if both heads are the same (no actual branch change) | ||
| if [ "$PREV_HEAD" = "$NEW_HEAD" ]; then | ||
| exit 0 | ||
| grep -q "^Cargo\.lock$" <<<"$CHANGED" && echo "📦 Cargo.lock changed — run: cargo build" | ||
| grep -q "^bun\.lockb\?$" <<<"$CHANGED" && echo "📦 bun.lock changed — run: bun install" | ||
| grep -q "^uv\.lock$" <<<"$CHANGED" && echo "📦 uv.lock changed — run: uv sync" | ||
| grep -q "^flake\.lock$" <<<"$CHANGED" && echo "📦 flake.lock changed — exit and re-enter: nix develop" | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update .git-hooks/README.md to match the new behavior.
.git-hooks/README.md lines 21-26 still state that post-checkout and post-merge run bun install automatically. This hook now only prints a resynchronization hint. The README also still describes the removed inline pre-commit and pre-push logic. Developers who read the README will expect dependencies to install themselves.
I can prepare the README update. Do you want me to open an issue to track it?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.git-hooks/post-checkout around lines 16 - 23, Update .git-hooks/README.md
to document that post-checkout only prints resynchronization hints, including
the relevant commands, instead of automatically running bun install. Remove or
revise references to the removed inline pre-commit and pre-push logic so the
README matches the current hook behavior.
| grep -q "^Cargo\.lock$" <<<"$CHANGED" && echo "📦 Cargo.lock changed — run: cargo build" | ||
| grep -q "^bun\.lockb\?$" <<<"$CHANGED" && echo "📦 bun.lock changed — run: bun install" | ||
| grep -q "^uv\.lock$" <<<"$CHANGED" && echo "📦 uv.lock changed — run: uv sync" | ||
| grep -q "^flake\.lock$" <<<"$CHANGED" && echo "📦 flake.lock changed — exit and re-enter: nix develop" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-portable BRE \? in the bun lockfile pattern. Both lockfile shims copy the same basic-regex pattern ^bun\.lockb\?$. \? is a GNU extension and is not reliable on BSD grep. Switch both to extended regex.
.git-hooks/post-checkout#L19-L22: change line 20 togrep -qE "^bun\.lockb?$"..git-hooks/post-merge#L12-L17: change line 15 togrep -qE "^bun\.lockb?$".
📍 Affects 2 files
.git-hooks/post-checkout#L19-L22(this comment).git-hooks/post-merge#L12-L17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.git-hooks/post-checkout around lines 19 - 22, Update the bun lockfile
checks in .git-hooks/post-checkout lines 19-22 and .git-hooks/post-merge lines
12-17 to use grep -qE with the ^bun\.lockb?$ extended-regex pattern, replacing
the non-portable BRE \?. Apply the same change at both affected sites.
| # ── Local override (language-specific checks) ─────────────────────────────── | ||
| LOCAL_HOOK="$(git rev-parse --show-toplevel)/.git-hooks/local-pre-push" | ||
| if [ -x "$LOCAL_HOOK" ]; then | ||
| # Re-supply captured stdin so local hooks that inspect refs still work. | ||
| "$LOCAL_HOOK" "$@" <<EOF | ||
| $PUSH_REFS | ||
| EOF | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Feed the captured refs without heredoc side effects.
Two problems exist in the delegation at lines 69-71. If $PUSH_REFS is empty, the heredoc still sends one blank line, so a local hook that loops over refs processes a blank record. The comment at lines 28-31 documents this same hazard for the canonical guard, but the fix is not applied here. The heredoc delimiter is also unquoted, so backslash sequences in ref data are processed.
🛠️ Proposed fix
LOCAL_HOOK="$(git rev-parse --show-toplevel)/.git-hooks/local-pre-push"
if [ -x "$LOCAL_HOOK" ]; then
# Re-supply captured stdin so local hooks that inspect refs still work.
- "$LOCAL_HOOK" "$@" <<EOF
-$PUSH_REFS
-EOF
+ if [ -n "$PUSH_REFS" ]; then
+ printf '%s\n' "$PUSH_REFS" | "$LOCAL_HOOK" "$@"
+ else
+ "$LOCAL_HOOK" "$@" </dev/null
+ fi
fiNote: with printf ... | "$LOCAL_HOOK" and pipefail active, a non-zero local hook status still aborts the push, which preserves the AGENTS.md fail-fast contract for cargo check --workspace --quiet.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ── Local override (language-specific checks) ─────────────────────────────── | |
| LOCAL_HOOK="$(git rev-parse --show-toplevel)/.git-hooks/local-pre-push" | |
| if [ -x "$LOCAL_HOOK" ]; then | |
| # Re-supply captured stdin so local hooks that inspect refs still work. | |
| "$LOCAL_HOOK" "$@" <<EOF | |
| $PUSH_REFS | |
| EOF | |
| fi | |
| # ── Local override (language-specific checks) ─────────────────────────────── | |
| LOCAL_HOOK="$(git rev-parse --show-toplevel)/.git-hooks/local-pre-push" | |
| if [ -x "$LOCAL_HOOK" ]; then | |
| # Re-supply captured stdin so local hooks that inspect refs still work. | |
| if [ -n "$PUSH_REFS" ]; then | |
| printf '%s\n' "$PUSH_REFS" | "$LOCAL_HOOK" "$@" | |
| else | |
| "$LOCAL_HOOK" "$@" </dev/null | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.git-hooks/pre-push around lines 65 - 72, Update the local hook delegation
around LOCAL_HOOK to pipe PUSH_REFS with printf rather than using the unquoted
heredoc. Ensure empty PUSH_REFS produces no input, preserves ref data without
escape-sequence processing, and retains propagation of the local hook’s non-zero
status under pipefail.
1 participant
The hooks here predated the current shim architecture, and their headers had also been rewritten from
ResQ SoftwaretoResQ Systems, Inc.— so none matched the SHA-256 digestsinstall-hooks.shverifies before writing them.The rewrite was not a local edit.
resq copyrightdefaults to the second author string, reads the difference as an author mismatch, strips the header and writes its own. Checked across the twenty ResQ repositories available locally: six carry hooks and not one still matched canonical.resq-software/crates#170 stops it at the source. This restores the files here.
Also: these hooks were never running
core.hooksPathwas unset, so git never looked in.git-hooks/. The files were committed but inert.resq hooks updatesets it, so the hooks now actually execute — worth knowing, since this repo's commits were not being checked at all.What changed
The six canonical hooks, written by
resq hooks update, which only touches those six names — anylocal-*override is untouched. All six are byte-identical tocrates/resq-cli/templates/git-hooks, the source the pinned digests are taken from.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements