From f77b290d4335ce11d4a3defac0bf857b675e48ee Mon Sep 17 00:00:00 2001 From: beads/crew/fang Date: Tue, 30 Dec 2025 20:40:09 -0800 Subject: [PATCH] fix: git hook chaining now works correctly (GH#816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes: 1. `bd init` chaining was broken - the code referenced `.old` hooks but never actually renamed the existing hooks. Added the missing os.Rename(). 2. `bd hooks install` now supports --chain flag to chain with existing hooks (e.g., pre-commit framework). When used, existing hooks are renamed to .old and bd hooks run will call them before the bd logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- cmd/bd/hooks.go | 89 ++++++++++++++++++++++++++++++++++++++-- cmd/bd/hooks_test.go | 72 +++++++++++++++++++++++++++++--- cmd/bd/init_git_hooks.go | 15 +++++-- 3 files changed, 163 insertions(+), 13 deletions(-) diff --git a/cmd/bd/hooks.go b/cmd/bd/hooks.go index d6a2ef9b..38d463f9 100644 --- a/cmd/bd/hooks.go +++ b/cmd/bd/hooks.go @@ -182,6 +182,9 @@ By default, hooks are installed to .git/hooks/ in the current repository. Use --shared to install to a versioned directory (.beads-hooks/) that can be committed to git and shared with team members. +Use --chain to preserve existing hooks and run them before bd hooks. This is +useful if you have pre-commit framework hooks or other custom hooks. + Installed hooks: - pre-commit: Flush changes to JSONL before commit - post-merge: Import JSONL after pull/merge @@ -191,6 +194,7 @@ Installed hooks: Run: func(cmd *cobra.Command, args []string) { force, _ := cmd.Flags().GetBool("force") shared, _ := cmd.Flags().GetBool("shared") + chain, _ := cmd.Flags().GetBool("chain") embeddedHooks, err := getEmbeddedHooks() if err != nil { @@ -206,7 +210,7 @@ Installed hooks: os.Exit(1) } - if err := installHooks(embeddedHooks, force, shared); err != nil { + if err := installHooks(embeddedHooks, force, shared, chain); err != nil { if jsonOutput { output := map[string]interface{}{ "error": err.Error(), @@ -224,12 +228,17 @@ Installed hooks: "success": true, "message": "Git hooks installed successfully", "shared": shared, + "chained": chain, } jsonBytes, _ := json.MarshalIndent(output, "", " ") fmt.Println(string(jsonBytes)) } else { fmt.Println("✓ Git hooks installed successfully") fmt.Println() + if chain { + fmt.Println("Mode: chained (existing hooks renamed to .old and will run first)") + fmt.Println() + } if shared { fmt.Println("Hooks installed to: .beads-hooks/") fmt.Println("Git config set: core.hooksPath=.beads-hooks") @@ -307,7 +316,7 @@ var hooksListCmd = &cobra.Command{ }, } -func installHooks(embeddedHooks map[string]string, force bool, shared bool) error { +func installHooks(embeddedHooks map[string]string, force bool, shared bool, chain bool) error { // Get actual git directory (handles worktrees where .git is a file) gitDir, err := git.GetGitDir() if err != nil { @@ -334,13 +343,20 @@ func installHooks(embeddedHooks map[string]string, force bool, shared bool) erro // Check if hook already exists if _, err := os.Stat(hookPath); err == nil { - // Hook exists - back it up unless force is set - if !force { + if chain { + // Chain mode - rename to .old so bd hooks run can call it + oldPath := hookPath + ".old" + if err := os.Rename(hookPath, oldPath); err != nil { + return fmt.Errorf("failed to rename %s to .old for chaining: %w", hookName, err) + } + } else if !force { + // Default mode - back it up backupPath := hookPath + ".backup" if err := os.Rename(hookPath, backupPath); err != nil { return fmt.Errorf("failed to backup %s: %w", hookName, err) } } + // If force is set and not chaining, we just overwrite } // Write hook file @@ -408,11 +424,55 @@ func uninstallHooks() error { // Hook Implementation Functions (called by thin shims via 'bd hooks run') // ============================================================================= +// runChainedHook runs a .old hook if it exists. Returns the exit code. +// If the hook doesn't exist, returns 0 (success). +func runChainedHook(hookName string, args []string) int { + // Get the hooks directory + gitDir, err := git.GetGitDir() + if err != nil { + return 0 // Not a git repo, nothing to chain + } + + oldHookPath := filepath.Join(gitDir, "hooks", hookName+".old") + + // Check if the .old hook exists and is executable + info, err := os.Stat(oldHookPath) + if err != nil { + return 0 // No chained hook + } + if info.Mode().Perm()&0111 == 0 { + return 0 // Not executable + } + + // Run the chained hook + // #nosec G204 -- hookName is from controlled list, path is from git directory + cmd := exec.Command(oldHookPath, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.Stdin = os.Stdin + + if err := cmd.Run(); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + return exitErr.ExitCode() + } + // Other error - treat as failure + fmt.Fprintf(os.Stderr, "Warning: chained hook %s failed: %v\n", hookName, err) + return 1 + } + + return 0 +} + // runPreCommitHook flushes pending changes to JSONL before commit. // Returns 0 on success (or if not applicable), non-zero on error. // //nolint:unparam // Always returns 0 by design - warnings don't block commits func runPreCommitHook() int { + // Run chained hook first (if exists) + if exitCode := runChainedHook("pre-commit", nil); exitCode != 0 { + return exitCode + } + // Check if we're in a bd workspace if _, err := os.Stat(".beads"); os.IsNotExist(err) { return 0 // Not a bd workspace, nothing to do @@ -449,6 +509,11 @@ func runPreCommitHook() int { // //nolint:unparam // Always returns 0 by design - warnings don't block merges func runPostMergeHook() int { + // Run chained hook first (if exists) + if exitCode := runChainedHook("post-merge", nil); exitCode != 0 { + return exitCode + } + // Skip during rebase if isRebaseInProgress() { return 0 @@ -485,6 +550,11 @@ func runPostMergeHook() int { // runPrePushHook prevents pushing stale JSONL. // Returns 0 to allow push, non-zero to block. func runPrePushHook() int { + // Run chained hook first (if exists) + if exitCode := runChainedHook("pre-push", nil); exitCode != 0 { + return exitCode + } + // Check if we're in a bd workspace if _, err := os.Stat(".beads"); os.IsNotExist(err) { return 0 @@ -552,6 +622,11 @@ func runPrePushHook() int { // //nolint:unparam // Always returns 0 by design - warnings don't block checkouts func runPostCheckoutHook(args []string) int { + // Run chained hook first (if exists) + if exitCode := runChainedHook("post-checkout", args); exitCode != 0 { + return exitCode + } + // Only run on branch checkouts (flag=1) if len(args) >= 3 && args[2] != "1" { return 0 @@ -616,6 +691,11 @@ func runPostCheckoutHook(args []string) int { // //nolint:unparam // Always returns 0 by design - we don't block commits func runPrepareCommitMsgHook(args []string) int { + // Run chained hook first (if exists) + if exitCode := runChainedHook("prepare-commit-msg", args); exitCode != 0 { + return exitCode + } + if len(args) < 1 { return 0 // No message file provided } @@ -967,6 +1047,7 @@ installed bd version - upgrading bd automatically updates hook behavior.`, func init() { hooksInstallCmd.Flags().Bool("force", false, "Overwrite existing hooks without backup") hooksInstallCmd.Flags().Bool("shared", false, "Install hooks to .beads-hooks/ (versioned) instead of .git/hooks/") + hooksInstallCmd.Flags().Bool("chain", false, "Chain with existing hooks (run them before bd hooks)") hooksCmd.AddCommand(hooksInstallCmd) hooksCmd.AddCommand(hooksUninstallCmd) diff --git a/cmd/bd/hooks_test.go b/cmd/bd/hooks_test.go index f4c166ad..46f851ed 100644 --- a/cmd/bd/hooks_test.go +++ b/cmd/bd/hooks_test.go @@ -51,7 +51,7 @@ func TestInstallHooks(t *testing.T) { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, false, false); err != nil { + if err := installHooks(hooks, false, false, false); err != nil { t.Fatalf("installHooks() failed: %v", err) } @@ -103,7 +103,7 @@ func TestInstallHooksBackup(t *testing.T) { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, false, false); err != nil { + if err := installHooks(hooks, false, false, false); err != nil { t.Fatalf("installHooks() failed: %v", err) } @@ -148,7 +148,7 @@ func TestInstallHooksForce(t *testing.T) { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, true, false); err != nil { + if err := installHooks(hooks, true, false, false); err != nil { t.Fatalf("installHooks() failed: %v", err) } @@ -176,7 +176,7 @@ func TestUninstallHooks(t *testing.T) { if err != nil { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, false, false); err != nil { + if err := installHooks(hooks, false, false, false); err != nil { t.Fatalf("installHooks() failed: %v", err) } @@ -211,7 +211,7 @@ func TestHooksCheckGitHooks(t *testing.T) { if err != nil { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, false, false); err != nil { + if err := installHooks(hooks, false, false, false); err != nil { t.Fatalf("installHooks() failed: %v", err) } @@ -245,7 +245,7 @@ func TestInstallHooksShared(t *testing.T) { t.Fatalf("getEmbeddedHooks() failed: %v", err) } - if err := installHooks(hooks, false, true); err != nil { + if err := installHooks(hooks, false, true, false); err != nil { t.Fatalf("installHooks() with shared=true failed: %v", err) } @@ -283,6 +283,66 @@ func TestInstallHooksShared(t *testing.T) { }) } +func TestInstallHooksChaining(t *testing.T) { + tmpDir := t.TempDir() + runInDir(t, tmpDir, func() { + if err := exec.Command("git", "init").Run(); err != nil { + t.Skipf("Skipping test: git init failed: %v", err) + } + + gitDirPath, err := git.GetGitDir() + if err != nil { + t.Fatalf("git.GetGitDir() failed: %v", err) + } + gitDir := filepath.Join(gitDirPath, "hooks") + if err := os.MkdirAll(gitDir, 0750); err != nil { + t.Fatalf("Failed to create hooks directory: %v", err) + } + + // Create an existing hook + existingHook := filepath.Join(gitDir, "pre-commit") + existingContent := "#!/bin/sh\necho old hook\n" + if err := os.WriteFile(existingHook, []byte(existingContent), 0755); err != nil { + t.Fatalf("Failed to create existing hook: %v", err) + } + + hooks, err := getEmbeddedHooks() + if err != nil { + t.Fatalf("getEmbeddedHooks() failed: %v", err) + } + + // Install with chain=true + if err := installHooks(hooks, false, false, true); err != nil { + t.Fatalf("installHooks() with chain=true failed: %v", err) + } + + // Verify the original hook was renamed to .old + oldPath := existingHook + ".old" + if _, err := os.Stat(oldPath); os.IsNotExist(err) { + t.Errorf("Existing hook was not renamed to .old for chaining") + } + + oldContent, err := os.ReadFile(oldPath) + if err != nil { + t.Fatalf("Failed to read .old hook: %v", err) + } + if string(oldContent) != existingContent { + t.Errorf(".old hook content mismatch: got %q, want %q", string(oldContent), existingContent) + } + + // Verify new hook was installed + if _, err := os.Stat(existingHook); os.IsNotExist(err) { + t.Errorf("New pre-commit hook was not installed") + } + + // Verify .backup was NOT created (chain mode uses .old, not .backup) + backupPath := existingHook + ".backup" + if _, err := os.Stat(backupPath); !os.IsNotExist(err) { + t.Errorf("Backup was created but should not be in chain mode") + } + }) +} + func TestFormatHookWarnings(t *testing.T) { tests := []struct { name string diff --git a/cmd/bd/init_git_hooks.go b/cmd/bd/init_git_hooks.go index d4d1eb77..40afbf24 100644 --- a/cmd/bd/init_git_hooks.go +++ b/cmd/bd/init_git_hooks.go @@ -160,6 +160,16 @@ func installGitHooks() error { switch choice { case "1", "": chainHooks = true + // Chain mode - rename existing hooks to .old so they can be called + for _, hook := range existingHooks { + if hook.exists && !hook.isBdHook { + oldPath := hook.path + ".old" + if err := os.Rename(hook.path, oldPath); err != nil { + return fmt.Errorf("failed to rename %s to .old: %w", hook.name, err) + } + fmt.Printf(" Renamed %s to %s\n", hook.name, filepath.Base(oldPath)) + } + } case "2": // Overwrite mode - backup existing hooks for _, hook := range existingHooks { @@ -211,12 +221,11 @@ func installGitHooks() error { // buildPreCommitHook generates the pre-commit hook content func buildPreCommitHook(chainHooks bool, existingHooks []hookInfo) string { if chainHooks { - // Find existing pre-commit hook + // Find existing pre-commit hook (already renamed to .old by caller) var existingPreCommit string for _, hook := range existingHooks { if hook.name == "pre-commit" && hook.exists && !hook.isBdHook { existingPreCommit = hook.path + ".old" - // Note: caller handles the rename break } } @@ -309,7 +318,7 @@ exit 0 // buildPostMergeHook generates the post-merge hook content func buildPostMergeHook(chainHooks bool, existingHooks []hookInfo) string { if chainHooks { - // Find existing post-merge hook + // Find existing post-merge hook (already renamed to .old by caller) var existingPostMerge string for _, hook := range existingHooks { if hook.name == "post-merge" && hook.exists && !hook.isBdHook {