Conversation
Plugin Version StatusVersions are auto-bumped in PRs. Manual bumps to higher versions are preserved.
|
Adds a PreToolUse hook that intercepts git push commands and runs git pull (rebase by default) to keep the branch up to date. If the pull/rebase fails due to conflicts, the push is blocked with resolution instructions. If new commits were pulled, a summary is printed. If already up to date, stays silent. The pull strategy is configurable via plugins.settings.yaml: scm-utils.prePushPullStrategy: "rebase" | "merge" https://claude.ai/code/session_018sEvJmud9yWTS1dDRPpbhC
58e3c88 to
8272084
Compare
There was a problem hiding this comment.
❌ Some changes need to be made — the hook is non-functional as written and several earlier blockers are still open
❌ Both source lines fail at runtime → the hook errors on every Bash call (thread)
⚠️ Wrong JSON path: reads .command, must be .tool_input.command — hook never matches a push (thread)
⚠️ Force pushes (-f/--force/--force-with-lease) not excluded — pre-pull breaks rebase-then-force-push (thread)
⚠️ Dirty working tree → push blocked with a misleading "conflicts" message (thread)
📝 Interaction with git-spice's reject-push hook (thread)
✅ Clean pass-through (no stdout on exit 0) follows the hook-output-patterns rule
🖱️ Click to expand for full details
source lines fail at runtime → the hook errors on every Bash call (thread).command, must be .tool_input.command — hook never matches a push (thread)-f/--force/--force-with-lease) not excluded — pre-pull breaks rebase-then-force-push (thread)📝 Interaction with git-spice's reject-push hook (thread)
✅ Clean pass-through (no stdout on exit 0) follows the
hook-output-patterns ruleNo author fix commits have landed since my last review (HEAD 57d3d05 is only an auto-bump + merge from main), so all previously-raised blockers remain. I re-verified each against the current branch state on disk.
❌ Blocker — both source lines fail; hook errors on every Bash call
prepush-pull.sh:30-32 sources two libs from a local lib/ dir, but neither resolves:
${PLUGIN_ROOT}/lib/log.sh— no such file or symlink exists underplugins/scm-utils/lib/(onlyplugin-config-read.shis created in this PR).${PLUGIN_ROOT}/lib/plugin-config-read.sh— a dangling symlink to../../../shared/lib/plugin-config-read.sh;shared/lib/no longer exists in the repo (readlink -ereturns nothing).
Under set -euo pipefail the first failed source exits the script non-zero on every Bash tool invocation (the matcher is Bash), so the hook never runs git pull and instead surfaces an error each time.
This is exactly the deprecated layout called out in .claude/rules/shared-libs.md: the shared/lib/ + per-plugin symlink approach broke after Claude Code v2.1.117 dropped symlink preservation in plugin caches. The plugin already declares the shared-lib dependency in plugin.json, so the script should source from the dependency's data dir using the documented _wait_for_shared_lib pattern (${CLAUDE_PLUGIN_DATA%/*}/shared-lib-ai-mktpl/lib) and the two lib/ symlinks should be removed. Details in the thread.
⚠️ Wrong JSON path for the command
prepush-pull.sh:36 reads .command, but PreToolUse input nests the command under .tool_input.command. Even after the source issue is fixed, command stays empty, the push regex never matches, and the hook silently no-ops. Every other tool_input reader in the repo — including this plugin's own PostToolUse hook — uses the nested path. Suggestion provided inline.
⚠️ Force pushes are not excluded
The regex on line 40 matches all git push, including --force / -f / --force-with-lease. Running git pull --rebase before an intentional force-push (the normal flow after a rebase, per this repo's auto-pr-management rule) re-introduces remote commits or causes spurious conflicts. Force-push variants should be detected and skipped. See the thread.
⚠️ Dirty working tree → misleading block
git pull --rebase refuses to run with unstaged/uncommitted changes (cannot pull with rebase: You have unstaged changes). That trips the if ! git pull --rebase branch and blocks the push with a "conflicts with the remote" message that doesn't describe the real cause. Pre-check the working tree (or special-case the message). See the thread.
📝 git-spice interaction
Both this hook and plugins/git-spice/hooks/scripts/reject-git-push.sh are PreToolUse Bash hooks matching git push. Ordering determines whether a pull happens before git-spice blocks the push. Informational — captured in the thread.
Design consideration (non-blocking)
Mutating git state (git pull --rebase) inside a PreToolUse hook on every push is surprising and can fight intentional workflows (force-push after rebase, git-spice stacks). Worth a prominent note in the README that the behavior auto-rewrites local history, and consider gating it behind an explicit opt-in setting rather than defaulting on.
Minor — perf ordering
Once the libs are fixed, source them after the git push regex check so non-push Bash calls (the vast majority) don't pay the cost of reading config/sourcing libs.
Scoring rationale
- Quality 40% — one hard runtime blocker (broken
source), a second correctness bug (.command) that nullifies the hook, plus two unaddressed⚠️items from prior reviews. - Security N/A — no security surface; the hook runs local git commands only.
- Simplicity 80% — logic is readable and well-commented, but duplicated rebase/merge blocks and config/lib sourcing-before-gate add avoidable weight.
- Confidence 92% — broken symlinks, missing
log.sh, and the jq path were all verified directly on disk; the only residual uncertainty is exact Claude Code hook-failure surfacing behavior.
Recommended follow-ups (non-blocking):
plugins/git-spice/hooks/scripts/reject-git-push.shappears to share the same.command(should be.tool_input.command) latent bug — worth a separate PR.- Adding a new hook is arguably a minor feature bump (
0.3.0) rather than the auto-applied patch (0.2.4) perversioning.md; optional.
Footnotes
-
Workflow Run: https://github.com/nsheaps/ai-mktpl/actions/runs/27070306095/attempts/1 ↩
-
Shared libraries rule (deprecated symlink layout): .claude/rules/shared-libs.md ↩
|
|
||
| # Read tool input from stdin | ||
| input=$(cat) | ||
| command=$(echo "$input" | jq -r '.command // empty' 2>/dev/null || true) |
There was a problem hiding this comment.
.command, but PreToolUse input nests it under .tool_input.
The PreToolUse hook input schema delivers the Bash command at .tool_input.command, not top-level .command. With the current path, command is always the empty string, the git push regex on line 40 never matches, and the hook silently exits 0 — i.e. it never pulls, even after the source blocker above is fixed.
Evidence from this repo: every other tool_input reader uses the nested path, including this plugin's own PostToolUse hook:
plugins/scm-utils/hooks/scripts/post-git-push-reminder.sh:9→.tool_input.commandplugins/github-app/hooks/scripts/github-token-check.sh:112→.tool_input.commandplugins/agentic-behavior/hooks/scripts/validate-git-state-before-exit.sh:10→.tool_input.commandplugins/skill-required/hooks/scripts/check-skill-required.sh:121→.tool_input.command
The only other hook using top-level .command is plugins/git-spice/hooks/scripts/reject-git-push.sh:18 — which this script was modeled on, and which appears to carry the same latent bug (worth a separate follow-up there).
| command=$(echo "$input" | jq -r '.command // empty' 2>/dev/null || true) | |
| command=$(echo "$input" | jq -r '.tool_input.command // empty' 2>/dev/null || true) |
Summary
Adds a PreToolUse hook to the scm-utils plugin that automatically pulls before
git pushto keep branches in sync with the remote.git pushcommands via a PreToolUse hook on the Bash toolgit pull --rebase(default) orgit pull --no-rebasebefore pushingplugins.settings.yaml(scm-utils.prePushPullStrategy: "rebase" | "merge")Changes
hooks/scripts/prepush-pull.sh— New PreToolUse hook script (30s timeout)hooks/hooks.json— Register PreToolUse hook entry (matcher:Bash)lib/plugin-config-read.sh— Symlink shared config library for strategy settingscm-utils.settings.yaml— Default settings (rebase strategy)README.md— Document the hook behavior and configurationTest plan
prePushPullStrategy: "merge"and verify merge is used instead of rebasehttps://claude.ai/code/session_018sEvJmud9yWTS1dDRPpbhC