Merge pull request #1136 from sjsyrek/fix/hook-chaining-preserves-original-1120

fix(hooks): prevent bd hooks install --chain from destroying user's original hook
2026-01-17 00:03:46 -08:00
parent c8187137f5 a5140075f1
commit 24bff73c54
3 changed files with 278 additions and 23 deletions
--- a/cmd/bd/doctor/git.go
+++ b/cmd/bd/doctor/git.go
@@ -24,6 +24,10 @@ const (
 // bdShimMarker identifies bd shim hooks (GH#946)
 const bdShimMarker = "# bd-shim"

+// bdInlineHookMarker identifies inline hooks created by bd init (GH#1120)
+// These hooks have the logic embedded directly rather than calling bd hooks run
+const bdInlineHookMarker = "# bd (beads)"
+
 // bdHooksRunPattern matches hooks that call bd hooks run
 var bdHooksRunPattern = regexp.MustCompile(`\bbd\s+hooks\s+run\b`)

@@ -150,10 +154,11 @@ func CheckGitHooks() DoctorCheck {
 	}
 }

-// areBdShimsInstalled checks if the installed hooks are bd shims or call bd hooks run.
+// areBdShimsInstalled checks if the installed hooks are bd shims, call bd hooks run,
+// or are inline bd hooks created by bd init.
 // This helps detect when bd hooks are installed directly but an external manager config exists.
-// Returns (true, installedHooks) if bd shims are detected, (false, nil) otherwise.
-// (GH#946)
+// Returns (true, installedHooks) if bd hooks are detected, (false, nil) otherwise.
+// (GH#946, GH#1120)
 func areBdShimsInstalled(hooksDir string) (bool, []string) {
 	hooks := []string{"pre-commit", "post-merge", "pre-push"}
 	var bdHooks []string
@@ -165,8 +170,10 @@ func areBdShimsInstalled(hooksDir string) (bool, []string) {
 			continue
 		}
 		contentStr := string(content)
-		// Check for bd-shim marker or bd hooks run call
-		if strings.Contains(contentStr, bdShimMarker) || bdHooksRunPattern.MatchString(contentStr) {
+		// Check for bd-shim marker, bd hooks run call, or inline bd hook marker (from bd init)
+		if strings.Contains(contentStr, bdShimMarker) ||
+			strings.Contains(contentStr, bdInlineHookMarker) ||
+			bdHooksRunPattern.MatchString(contentStr) {
 			bdHooks = append(bdHooks, hookName)
 		}
 	}
--- a/cmd/bd/hooks.go
+++ b/cmd/bd/hooks.go
@@ -37,6 +37,10 @@ func getEmbeddedHooks() (map[string]string, error) {
 const hookVersionPrefix = "# bd-hooks-version: "
 const shimVersionPrefix = "# bd-shim "

+// inlineHookMarker identifies inline hooks created by bd init (GH#1120)
+// These hooks have the logic embedded directly rather than using shims
+const inlineHookMarker = "# bd (beads)"
+
 // HookStatus represents the status of a single git hook
 type HookStatus struct {
 	Name      string
@@ -92,8 +96,9 @@ func CheckGitHooks() []HookStatus {

 // hookVersionInfo contains version information extracted from a hook file
 type hookVersionInfo struct {
-	Version string // bd version (for legacy hooks) or shim version
-	IsShim  bool   // true if this is a thin shim
+	Version  string // bd version (for legacy hooks) or shim version
+	IsShim   bool   // true if this is a thin shim
+	IsBdHook bool   // true if this is any type of bd hook (shim or inline)
 }

 // getHookVersion extracts the version from a hook file
@@ -106,24 +111,33 @@ func getHookVersion(path string) (hookVersionInfo, error) {
 	defer file.Close()

 	scanner := bufio.NewScanner(file)
-	// Read first few lines looking for version marker
+	// Read first few lines looking for version marker or bd hook marker
 	lineCount := 0
-	for scanner.Scan() && lineCount < 10 {
+	var content strings.Builder
+	for scanner.Scan() && lineCount < 15 {
 		line := scanner.Text()
+		content.WriteString(line)
+		content.WriteString("\n")
 		// Check for thin shim marker first
 		if strings.HasPrefix(line, shimVersionPrefix) {
 			version := strings.TrimSpace(strings.TrimPrefix(line, shimVersionPrefix))
-			return hookVersionInfo{Version: version, IsShim: true}, nil
+			return hookVersionInfo{Version: version, IsShim: true, IsBdHook: true}, nil
 		}
 		// Check for legacy version marker
 		if strings.HasPrefix(line, hookVersionPrefix) {
 			version := strings.TrimSpace(strings.TrimPrefix(line, hookVersionPrefix))
-			return hookVersionInfo{Version: version, IsShim: false}, nil
+			return hookVersionInfo{Version: version, IsShim: false, IsBdHook: true}, nil
 		}
 		lineCount++
 	}

-	// No version found (old hook)
+	// Check if it's an inline bd hook (from bd init) - GH#1120
+	// These don't have version markers but have "# bd (beads)" comment
+	if strings.Contains(content.String(), inlineHookMarker) {
+		return hookVersionInfo{IsBdHook: true}, nil
+	}
+
+	// No version found and not a bd hook
 	return hookVersionInfo{}, nil
 }

@@ -363,17 +377,26 @@ func installHooksWithOptions(embeddedHooks map[string]string, force bool, shared
 		if _, err := os.Stat(hookPath); err == nil {
 			if chain {
 				// Chain mode - rename to .old so bd hooks run can call it
-				// But skip if existing hook is already a bd shim - renaming it would
-				// cause infinite recursion. See: https://github.com/steveyegge/beads/issues/843
+				// But skip if existing hook is already a bd hook (shim or inline) - renaming it would
+				// cause infinite recursion or destroy user's original hook. See: GH#843, GH#1120
 				versionInfo, verr := getHookVersion(hookPath)
-				if verr != nil || !versionInfo.IsShim {
-					// Not a bd shim - safe to rename for chaining
+				if verr != nil || !versionInfo.IsBdHook {
+					// Not a bd hook - 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)
+					// Safety check: don't overwrite existing .old file (GH#1120)
+					// This prevents destroying user's original hook if bd hooks install --chain
+					// is run multiple times or after bd init already created .old
+					if _, oldErr := os.Stat(oldPath); oldErr == nil {
+						// .old already exists - the user's original hook is there
+						// Just overwrite the current hook without renaming
+						// (the existing .old will be chained by the new hook)
+					} else {
+						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)
+				// If it IS a bd hook, just overwrite it (no rename needed)
 			} else if !force {
 				// Default mode - back it up
 				backupPath := hookPath + ".backup"
@@ -492,12 +515,12 @@ func runChainedHook(hookName string, args []string) int {
 		return 0 // Not executable
 	}

-	// Check if .old is itself a bd shim - skip to prevent infinite recursion
+	// Check if .old is itself a bd hook (shim or inline) - 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
+	// renaming an existing bd hook to .old. See: GH#843, GH#1120
 	versionInfo, err := getHookVersion(oldHookPath)
-	if err == nil && versionInfo.IsShim {
-		// Skip execution - .old is a bd shim which would call us again
+	if err == nil && versionInfo.IsBdHook {
+		// Skip execution - .old is a bd hook which would call us again
 		return 0
 	}

--- a/cmd/bd/hooks_test.go
+++ b/cmd/bd/hooks_test.go
@@ -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)
+		}
+	})
+}