fix: prevent infinite loop in bd hooks run when .old is a bd shim (#843)
When bd hooks install --chain renamed an existing bd shim to .old, subsequent bd hooks run would execute the .old shim, which would call bd hooks run again, creating infinite recursion. Two fixes: 1. installHooks(): Skip renaming to .old if existing hook is a bd shim 2. runChainedHook(): Skip executing .old if it is a bd shim 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
committed by
Steve Yegge
parent
d6045ab297
commit
050d1a4413
@@ -345,10 +345,17 @@ func installHooks(embeddedHooks map[string]string, force bool, shared bool, chai
|
|||||||
if _, err := os.Stat(hookPath); err == nil {
|
if _, err := os.Stat(hookPath); err == nil {
|
||||||
if chain {
|
if chain {
|
||||||
// Chain mode - rename to .old so bd hooks run can call it
|
// Chain mode - rename to .old so bd hooks run can call it
|
||||||
oldPath := hookPath + ".old"
|
// But skip if existing hook is already a bd shim - renaming it would
|
||||||
if err := os.Rename(hookPath, oldPath); err != nil {
|
// cause infinite recursion. See: https://github.com/steveyegge/beads/issues/843
|
||||||
return fmt.Errorf("failed to rename %s to .old for chaining: %w", hookName, err)
|
versionInfo, verr := getHookVersion(hookPath)
|
||||||
|
if verr != nil || !versionInfo.IsShim {
|
||||||
|
// Not a bd shim - safe to rename for chaining
|
||||||
|
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)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
// If it IS a bd shim, just overwrite it (no rename needed)
|
||||||
} else if !force {
|
} else if !force {
|
||||||
// Default mode - back it up
|
// Default mode - back it up
|
||||||
backupPath := hookPath + ".backup"
|
backupPath := hookPath + ".backup"
|
||||||
@@ -444,6 +451,15 @@ func runChainedHook(hookName string, args []string) int {
|
|||||||
return 0 // Not executable
|
return 0 // Not executable
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if .old is itself a bd shim - skip to prevent infinite recursion
|
||||||
|
// This can happen if user runs `bd hooks install --chain` multiple times,
|
||||||
|
// renaming an existing bd shim to .old. See: https://github.com/steveyegge/beads/issues/843
|
||||||
|
versionInfo, err := getHookVersion(oldHookPath)
|
||||||
|
if err == nil && versionInfo.IsShim {
|
||||||
|
// Skip execution - .old is a bd shim which would call us again
|
||||||
|
return 0
|
||||||
|
}
|
||||||
|
|
||||||
// Run the chained hook
|
// Run the chained hook
|
||||||
// #nosec G204 -- hookName is from controlled list, path is from git directory
|
// #nosec G204 -- hookName is from controlled list, path is from git directory
|
||||||
cmd := exec.Command(oldHookPath, args...)
|
cmd := exec.Command(oldHookPath, args...)
|
||||||
|
|||||||
@@ -459,3 +459,86 @@ func TestHasBeadsJSONL(t *testing.T) {
|
|||||||
t.Error("hasBeadsJSONL() = false, want true (issues.jsonl exists)")
|
t.Error("hasBeadsJSONL() = false, want true (issues.jsonl exists)")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestInstallHooksChainingSkipsBdShim verifies that bd hooks install --chain
|
||||||
|
// does NOT rename existing bd shims to .old (which would cause infinite recursion).
|
||||||
|
// See: https://github.com/steveyegge/beads/issues/843
|
||||||
|
func TestInstallHooksChainingSkipsBdShim(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 a bd shim
|
||||||
|
existingHook := filepath.Join(gitDir, "pre-commit")
|
||||||
|
shimContent := "#!/bin/sh\n# bd-shim v1\nexec bd hooks run pre-commit \"$@\"\n"
|
||||||
|
if err := os.WriteFile(existingHook, []byte(shimContent), 0755); err != nil {
|
||||||
|
t.Fatalf("Failed to create existing shim 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 shim was NOT renamed to .old (would cause infinite loop)
|
||||||
|
oldPath := existingHook + ".old"
|
||||||
|
if _, err := os.Stat(oldPath); !os.IsNotExist(err) {
|
||||||
|
t.Errorf("bd shim was renamed to .old - this would cause infinite recursion!")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify new hook was installed (overwrote the shim)
|
||||||
|
if _, err := os.Stat(existingHook); os.IsNotExist(err) {
|
||||||
|
t.Errorf("New pre-commit hook was not installed")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestRunChainedHookSkipsBdShim verifies that runChainedHook() skips
|
||||||
|
// .old hooks that are bd shims (to prevent infinite recursion).
|
||||||
|
// See: https://github.com/steveyegge/beads/issues/843
|
||||||
|
func TestRunChainedHookSkipsBdShim(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 a bd shim (simulating the problematic state)
|
||||||
|
oldHook := filepath.Join(gitDir, "pre-commit.old")
|
||||||
|
shimContent := "#!/bin/sh\n# bd-shim v1\nexec bd hooks run pre-commit \"$@\"\n"
|
||||||
|
if err := os.WriteFile(oldHook, []byte(shimContent), 0755); err != nil {
|
||||||
|
t.Fatalf("Failed to create .old shim hook: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// runChainedHook should return 0 (skip the shim) instead of executing it
|
||||||
|
exitCode := runChainedHook("pre-commit", nil)
|
||||||
|
if exitCode != 0 {
|
||||||
|
t.Errorf("runChainedHook() = %d, want 0 (should skip bd shim)", exitCode)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user