johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(hooks): prevent bd hooks install --chain from destroying user's original hook

Browse Source 
When bd init installs hooks with chaining, it renames the user's original
hook to .old and creates a new bd hook that chains to it. If bd doctor
later runs bd hooks install --chain, it would:

1. Not recognize the inline bd hook (from bd init) as a bd hook
2. Rename it to .old, overwriting the user's original hook
3. Install a new shim hook

This fix addresses two root causes:

1. Detection mismatch: areBdShimsInstalled() and getHookVersion() now
   recognize inline bd hooks (which have "# bd (beads)" marker) in
   addition to shim hooks (which have "# bd-shim" marker)

2. No .old protection: bd hooks install --chain now checks if .old
   already exists before renaming, preserving the user's original hook

Fixes #1120

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Steven Syrek
2026-01-17 08:44:30 +01:00
parent aee86dfae2
commit a5140075f1
3 changed files with 278 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

225
cmd/bd/hooks_test.go
View File

@@ -542,3 +542,228 @@ func TestRunChainedHookSkipsBdShim(t *testing.T) {
}
})
}
// TestGetHookVersionRecognizesInlineHooks verifies that getHookVersion()
// correctly identifies inline bd hooks created by bd init.
// See: https://github.com/steveyegge/beads/issues/1120
func TestGetHookVersionRecognizesInlineHooks(t *testing.T) {
tmpDir := t.TempDir()
// Test inline hook from bd init
inlineHook := filepath.Join(tmpDir, "pre-commit")
inlineContent := `#!/bin/sh
#
# bd (beads) pre-commit hook (chained)
#
# This hook chains bd functionality with your existing pre-commit hook.
if ! command -v bd >/dev/null 2>&1; then
echo "Warning: bd command not found" >&2
exit 0
fi
bd sync --flush-only
`
if err := os.WriteFile(inlineHook, []byte(inlineContent), 0755); err != nil {
t.Fatalf("Failed to create inline hook: %v", err)
}
info, err := getHookVersion(inlineHook)
if err != nil {
t.Fatalf("getHookVersion() failed: %v", err)
}
if !info.IsBdHook {
t.Error("getHookVersion() IsBdHook = false, want true for inline bd hook")
}
if info.IsShim {
t.Error("getHookVersion() IsShim = true, want false for inline bd hook")
}
// Test shim hook (should also be recognized as IsBdHook)
shimHook := filepath.Join(tmpDir, "pre-commit-shim")
shimContent := "#!/bin/sh\n# bd-shim v1\nexec bd hooks run pre-commit \"$@\"\n"
if err := os.WriteFile(shimHook, []byte(shimContent), 0755); err != nil {
t.Fatalf("Failed to create shim hook: %v", err)
}
info, err = getHookVersion(shimHook)
if err != nil {
t.Fatalf("getHookVersion() failed: %v", err)
}
if !info.IsBdHook {
t.Error("getHookVersion() IsBdHook = false, want true for shim")
}
if !info.IsShim {
t.Error("getHookVersion() IsShim = false, want true for shim")
}
// Test non-bd hook
userHook := filepath.Join(tmpDir, "pre-commit-user")
userContent := "#!/bin/sh\necho 'User hook'\n"
if err := os.WriteFile(userHook, []byte(userContent), 0755); err != nil {
t.Fatalf("Failed to create user hook: %v", err)
}
info, err = getHookVersion(userHook)
if err != nil {
t.Fatalf("getHookVersion() failed: %v", err)
}
if info.IsBdHook {
t.Error("getHookVersion() IsBdHook = true, want false for user hook")
}
}
// TestInstallHooksChainingSkipsInlineHook verifies that bd hooks install --chain
// does NOT rename existing inline bd hooks to .old (which would destroy user's original).
// See: https://github.com/steveyegge/beads/issues/1120
func TestInstallHooksChainingSkipsInlineHook(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 that IS an inline bd hook (from bd init)
existingHook := filepath.Join(gitDir, "pre-commit")
inlineContent := `#!/bin/sh
#
# bd (beads) pre-commit hook (chained)
#
bd sync --flush-only
`
if err := os.WriteFile(existingHook, []byte(inlineContent), 0755); err != nil {
t.Fatalf("Failed to create existing inline 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 inline hook was NOT renamed to .old (would destroy user's original)
oldPath := existingHook + ".old"
if _, err := os.Stat(oldPath); !os.IsNotExist(err) {
t.Errorf("inline bd hook was renamed to .old - this would destroy user's original hook!")
}
// Verify new hook was installed (overwrote the inline hook)
if _, err := os.Stat(existingHook); os.IsNotExist(err) {
t.Errorf("New pre-commit hook was not installed")
}
})
}
// TestInstallHooksChainingPreservesExistingOld verifies that bd hooks install --chain
// does NOT overwrite an existing .old file (which would destroy user's original hook).
// See: https://github.com/steveyegge/beads/issues/1120
func TestInstallHooksChainingPreservesExistingOld(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 the user's original hook as .old (simulating bd init already ran)
originalHook := filepath.Join(gitDir, "pre-commit.old")
originalContent := "#!/bin/sh\necho 'User original hook'\n"
if err := os.WriteFile(originalHook, []byte(originalContent), 0755); err != nil {
t.Fatalf("Failed to create original .old hook: %v", err)
}
// Create a current hook that is NOT a bd hook (e.g., user modified it)
currentHook := filepath.Join(gitDir, "pre-commit")
currentContent := "#!/bin/sh\necho 'Some other hook'\n"
if err := os.WriteFile(currentHook, []byte(currentContent), 0755); err != nil {
t.Fatalf("Failed to create current 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 .old was preserved (not overwritten)
preservedContent, err := os.ReadFile(originalHook)
if err != nil {
t.Fatalf("Failed to read preserved .old hook: %v", err)
}
if string(preservedContent) != originalContent {
t.Errorf(".old hook was overwritten! got %q, want %q", string(preservedContent), originalContent)
}
// Verify new hook was installed
if _, err := os.Stat(currentHook); os.IsNotExist(err) {
t.Errorf("New pre-commit hook was not installed")
}
})
}
// TestRunChainedHookSkipsInlineHook verifies that runChainedHook() skips
// .old hooks that are inline bd hooks (to prevent recursion).
// See: https://github.com/steveyegge/beads/issues/1120
func TestRunChainedHookSkipsInlineHook(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 a .old hook that IS an inline bd hook
oldHook := filepath.Join(gitDir, "pre-commit.old")
inlineContent := `#!/bin/sh
#
# bd (beads) pre-commit hook (chained)
#
bd sync --flush-only
`
if err := os.WriteFile(oldHook, []byte(inlineContent), 0755); err != nil {
t.Fatalf("Failed to create .old inline hook: %v", err)
}
// runChainedHook should return 0 (skip the inline hook) instead of executing it
exitCode := runChainedHook("pre-commit", nil)
if exitCode != 0 {
t.Errorf("runChainedHook() = %d, want 0 (should skip inline bd hook)", exitCode)
}
})
}