fix(doctor): detect sync-branch hook incompatibility (#532)
Add new bd doctor check that detects when sync-branch is configured but the pre-push hook is too old (< 0.29.0) to support it. This causes circular "bd sync" failures where the hook recommends running bd sync but the user is already running bd sync. The check: - Returns error when hook version < 0.29.0 with sync-branch configured - Returns warning for custom (non-bd) hooks that can't be verified - Returns OK when hook is compatible or sync-branch not configured Also adds checkSyncBranchHookQuick() for --check-health mode. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
195
cmd/bd/doctor.go
195
cmd/bd/doctor.go
@@ -63,6 +63,9 @@ var (
|
|||||||
// ConfigKeyHintsDoctor is the config key for suppressing doctor hints
|
// ConfigKeyHintsDoctor is the config key for suppressing doctor hints
|
||||||
const ConfigKeyHintsDoctor = "hints.doctor"
|
const ConfigKeyHintsDoctor = "hints.doctor"
|
||||||
|
|
||||||
|
// minSyncBranchHookVersion is the minimum hook version that supports sync-branch bypass (issue #532)
|
||||||
|
const minSyncBranchHookVersion = "0.29.0"
|
||||||
|
|
||||||
var doctorCmd = &cobra.Command{
|
var doctorCmd = &cobra.Command{
|
||||||
Use: "doctor [path]",
|
Use: "doctor [path]",
|
||||||
Short: "Check beads installation health",
|
Short: "Check beads installation health",
|
||||||
@@ -473,6 +476,11 @@ func runCheckHealth(path string) {
|
|||||||
issues = append(issues, issue)
|
issues = append(issues, issue)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check 3: Sync-branch hook compatibility (issue #532)
|
||||||
|
if issue := checkSyncBranchHookQuick(path); issue != "" {
|
||||||
|
issues = append(issues, issue)
|
||||||
|
}
|
||||||
|
|
||||||
// If any issues found, print hint
|
// If any issues found, print hint
|
||||||
if len(issues) > 0 {
|
if len(issues) > 0 {
|
||||||
printCheckHealthHint(issues)
|
printCheckHealthHint(issues)
|
||||||
@@ -600,6 +608,75 @@ func checkHooksQuick() string {
|
|||||||
return fmt.Sprintf("Git hooks outdated: %s (%s → %s)", strings.Join(outdatedHooks, ", "), oldestVersion, Version)
|
return fmt.Sprintf("Git hooks outdated: %s (%s → %s)", strings.Join(outdatedHooks, ", "), oldestVersion, Version)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// checkSyncBranchHookQuick does a fast check for sync-branch hook compatibility (issue #532).
|
||||||
|
// Returns empty string if OK, otherwise returns issue description.
|
||||||
|
func checkSyncBranchHookQuick(path string) string {
|
||||||
|
// Check if sync-branch is configured
|
||||||
|
syncBranch := syncbranch.GetFromYAML()
|
||||||
|
if syncBranch == "" {
|
||||||
|
return "" // sync-branch not configured, nothing to check
|
||||||
|
}
|
||||||
|
|
||||||
|
// Get git directory
|
||||||
|
cmd := exec.Command("git", "rev-parse", "--git-dir")
|
||||||
|
cmd.Dir = path
|
||||||
|
output, err := cmd.Output()
|
||||||
|
if err != nil {
|
||||||
|
return "" // Not a git repo, skip
|
||||||
|
}
|
||||||
|
gitDir := strings.TrimSpace(string(output))
|
||||||
|
if !filepath.IsAbs(gitDir) {
|
||||||
|
gitDir = filepath.Join(path, gitDir)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Find pre-push hook (check shared hooks first)
|
||||||
|
var hookPath string
|
||||||
|
hooksPathCmd := exec.Command("git", "config", "--get", "core.hooksPath")
|
||||||
|
hooksPathCmd.Dir = path
|
||||||
|
if hooksPathOutput, err := hooksPathCmd.Output(); err == nil {
|
||||||
|
sharedHooksDir := strings.TrimSpace(string(hooksPathOutput))
|
||||||
|
if !filepath.IsAbs(sharedHooksDir) {
|
||||||
|
sharedHooksDir = filepath.Join(path, sharedHooksDir)
|
||||||
|
}
|
||||||
|
hookPath = filepath.Join(sharedHooksDir, "pre-push")
|
||||||
|
} else {
|
||||||
|
hookPath = filepath.Join(gitDir, "hooks", "pre-push")
|
||||||
|
}
|
||||||
|
|
||||||
|
content, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
||||||
|
if err != nil {
|
||||||
|
return "" // No pre-push hook, covered by other checks
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if bd hook and extract version
|
||||||
|
hookStr := string(content)
|
||||||
|
if !strings.Contains(hookStr, "bd-hooks-version:") {
|
||||||
|
return "" // Not a bd hook, can't check
|
||||||
|
}
|
||||||
|
|
||||||
|
var hookVersion string
|
||||||
|
for _, line := range strings.Split(hookStr, "\n") {
|
||||||
|
if strings.Contains(line, "bd-hooks-version:") {
|
||||||
|
parts := strings.SplitN(line, ":", 2)
|
||||||
|
if len(parts) == 2 {
|
||||||
|
hookVersion = strings.TrimSpace(parts[1])
|
||||||
|
}
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if hookVersion == "" {
|
||||||
|
return "" // Can't determine version
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if version < minSyncBranchHookVersion (when sync-branch bypass was added)
|
||||||
|
if compareVersions(hookVersion, minSyncBranchHookVersion) < 0 {
|
||||||
|
return fmt.Sprintf("Pre-push hook (%s) incompatible with sync-branch mode (requires %s+)", hookVersion, minSyncBranchHookVersion)
|
||||||
|
}
|
||||||
|
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
func runDiagnostics(path string) doctorResult {
|
func runDiagnostics(path string) doctorResult {
|
||||||
result := doctorResult{
|
result := doctorResult{
|
||||||
Path: path,
|
Path: path,
|
||||||
@@ -619,6 +696,13 @@ func runDiagnostics(path string) doctorResult {
|
|||||||
result.Checks = append(result.Checks, hooksCheck)
|
result.Checks = append(result.Checks, hooksCheck)
|
||||||
// Don't fail overall check for missing hooks, just warn
|
// Don't fail overall check for missing hooks, just warn
|
||||||
|
|
||||||
|
// Check sync-branch hook compatibility (issue #532)
|
||||||
|
syncBranchHookCheck := checkSyncBranchHookCompatibility(path)
|
||||||
|
result.Checks = append(result.Checks, syncBranchHookCheck)
|
||||||
|
if syncBranchHookCheck.Status == statusError {
|
||||||
|
result.OverallOK = false
|
||||||
|
}
|
||||||
|
|
||||||
// If no .beads/, skip remaining checks
|
// If no .beads/, skip remaining checks
|
||||||
if installCheck.Status != statusOK {
|
if installCheck.Status != statusOK {
|
||||||
return result
|
return result
|
||||||
@@ -1934,6 +2018,117 @@ func checkGitHooks() doctorCheck {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// checkSyncBranchHookCompatibility checks if pre-push hook is compatible with sync-branch mode.
|
||||||
|
// When sync-branch is configured, the pre-push hook must have the sync-branch bypass logic
|
||||||
|
// (added in version 0.29.0). Without it, users experience circular "bd sync" failures (issue #532).
|
||||||
|
func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
||||||
|
// Check if sync-branch is configured
|
||||||
|
syncBranch := syncbranch.GetFromYAML()
|
||||||
|
if syncBranch == "" {
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusOK,
|
||||||
|
Message: "N/A (sync-branch not configured)",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// sync-branch is configured - check pre-push hook version
|
||||||
|
// Get actual git directory (handles worktrees where .git is a file)
|
||||||
|
cmd := exec.Command("git", "rev-parse", "--git-dir")
|
||||||
|
cmd.Dir = path
|
||||||
|
output, err := cmd.Output()
|
||||||
|
if err != nil {
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusOK,
|
||||||
|
Message: "N/A (not a git repository)",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
gitDir := strings.TrimSpace(string(output))
|
||||||
|
if !filepath.IsAbs(gitDir) {
|
||||||
|
gitDir = filepath.Join(path, gitDir)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check for pre-push hook in standard location or shared hooks location
|
||||||
|
var hookPath string
|
||||||
|
|
||||||
|
// First check if core.hooksPath is configured (shared hooks)
|
||||||
|
hooksPathCmd := exec.Command("git", "config", "--get", "core.hooksPath")
|
||||||
|
hooksPathCmd.Dir = path
|
||||||
|
if hooksPathOutput, err := hooksPathCmd.Output(); err == nil {
|
||||||
|
sharedHooksDir := strings.TrimSpace(string(hooksPathOutput))
|
||||||
|
if !filepath.IsAbs(sharedHooksDir) {
|
||||||
|
sharedHooksDir = filepath.Join(path, sharedHooksDir)
|
||||||
|
}
|
||||||
|
hookPath = filepath.Join(sharedHooksDir, "pre-push")
|
||||||
|
} else {
|
||||||
|
// Use standard .git/hooks location
|
||||||
|
hookPath = filepath.Join(gitDir, "hooks", "pre-push")
|
||||||
|
}
|
||||||
|
|
||||||
|
hookContent, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
||||||
|
if err != nil {
|
||||||
|
// No pre-push hook installed - different issue, covered by checkGitHooks
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusOK,
|
||||||
|
Message: "N/A (no pre-push hook installed)",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check if this is a bd hook and extract version
|
||||||
|
hookStr := string(hookContent)
|
||||||
|
if !strings.Contains(hookStr, "bd-hooks-version:") {
|
||||||
|
// Not a bd hook - can't determine compatibility
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusWarning,
|
||||||
|
Message: "Pre-push hook is not a bd hook",
|
||||||
|
Detail: "Cannot verify sync-branch compatibility with custom hooks",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Extract version from hook
|
||||||
|
var hookVersion string
|
||||||
|
for _, line := range strings.Split(hookStr, "\n") {
|
||||||
|
if strings.Contains(line, "bd-hooks-version:") {
|
||||||
|
parts := strings.SplitN(line, ":", 2)
|
||||||
|
if len(parts) == 2 {
|
||||||
|
hookVersion = strings.TrimSpace(parts[1])
|
||||||
|
}
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if hookVersion == "" {
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusWarning,
|
||||||
|
Message: "Could not determine pre-push hook version",
|
||||||
|
Detail: "Cannot verify sync-branch compatibility",
|
||||||
|
Fix: "Run 'bd hooks install --force' to update hooks",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// minSyncBranchHookVersion added sync-branch bypass logic
|
||||||
|
// If hook version < minSyncBranchHookVersion, it will cause circular "bd sync" failures
|
||||||
|
if compareVersions(hookVersion, minSyncBranchHookVersion) < 0 {
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusError,
|
||||||
|
Message: fmt.Sprintf("Pre-push hook incompatible with sync-branch mode (version %s)", hookVersion),
|
||||||
|
Detail: fmt.Sprintf("Hook version %s lacks sync-branch bypass (requires %s+). This causes circular 'bd sync' failures during push.", hookVersion, minSyncBranchHookVersion),
|
||||||
|
Fix: "Run 'bd hooks install --force' to update hooks",
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return doctorCheck{
|
||||||
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
Status: statusOK,
|
||||||
|
Message: fmt.Sprintf("Pre-push hook compatible with sync-branch (version %s)", hookVersion),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func checkSchemaCompatibility(path string) doctorCheck {
|
func checkSchemaCompatibility(path string) doctorCheck {
|
||||||
beadsDir := filepath.Join(path, ".beads")
|
beadsDir := filepath.Join(path, ".beads")
|
||||||
|
|
||||||
|
|||||||
@@ -1092,3 +1092,176 @@ func TestExportDiagnosticsInvalidPath(t *testing.T) {
|
|||||||
t.Error("Expected error for invalid path, got nil")
|
t.Error("Expected error for invalid path, got nil")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestCheckSyncBranchHookCompatibility tests the sync-branch hook compatibility check (issue #532)
|
||||||
|
// Note: We use BEADS_SYNC_BRANCH env var to control sync-branch detection because the config
|
||||||
|
// package reads from the actual beads repo's config.yaml. Only test cases with syncBranchEnv
|
||||||
|
// set to a non-empty value are reliable.
|
||||||
|
func TestCheckSyncBranchHookCompatibility(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
syncBranchEnv string // BEADS_SYNC_BRANCH env var (must be non-empty to override config.yaml)
|
||||||
|
hasGitDir bool
|
||||||
|
hookVersion string // Empty means no hook, "custom" means non-bd hook
|
||||||
|
expectedStatus string
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, no git repo",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: false,
|
||||||
|
hookVersion: "",
|
||||||
|
expectedStatus: statusOK, // N/A case
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, no pre-push hook",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "",
|
||||||
|
expectedStatus: statusOK, // Covered by other check
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, custom hook",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "custom",
|
||||||
|
expectedStatus: statusWarning,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, old hook (0.24.2)",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.24.2",
|
||||||
|
expectedStatus: statusError,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, old hook (0.28.0)",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.28.0",
|
||||||
|
expectedStatus: statusError,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, compatible hook (0.29.0)",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.29.0",
|
||||||
|
expectedStatus: statusOK,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "sync-branch configured, newer hook (0.30.0)",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.30.0",
|
||||||
|
expectedStatus: statusOK,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range tests {
|
||||||
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
|
||||||
|
// Always set environment variable to control sync-branch detection
|
||||||
|
// This overrides any config.yaml value in the actual beads repo
|
||||||
|
t.Setenv("BEADS_SYNC_BRANCH", tc.syncBranchEnv)
|
||||||
|
|
||||||
|
if tc.hasGitDir {
|
||||||
|
// Initialize a real git repo (git rev-parse needs this)
|
||||||
|
cmd := exec.Command("git", "init")
|
||||||
|
cmd.Dir = tmpDir
|
||||||
|
if err := cmd.Run(); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Create pre-push hook if specified
|
||||||
|
if tc.hookVersion != "" {
|
||||||
|
hooksDir := filepath.Join(tmpDir, ".git", "hooks")
|
||||||
|
hookPath := filepath.Join(hooksDir, "pre-push")
|
||||||
|
var hookContent string
|
||||||
|
if tc.hookVersion == "custom" {
|
||||||
|
hookContent = "#!/bin/sh\n# Custom hook\nexit 0\n"
|
||||||
|
} else {
|
||||||
|
hookContent = fmt.Sprintf("#!/bin/sh\n# bd-hooks-version: %s\nexit 0\n", tc.hookVersion)
|
||||||
|
}
|
||||||
|
if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
check := checkSyncBranchHookCompatibility(tmpDir)
|
||||||
|
|
||||||
|
if check.Status != tc.expectedStatus {
|
||||||
|
t.Errorf("Expected status %s, got %s (message: %s)", tc.expectedStatus, check.Status, check.Message)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Error case should have a fix message
|
||||||
|
if tc.expectedStatus == statusError && check.Fix == "" {
|
||||||
|
t.Error("Expected fix message for error status")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestCheckSyncBranchHookQuick tests the quick sync-branch hook check (issue #532)
|
||||||
|
// Note: We use BEADS_SYNC_BRANCH env var to control sync-branch detection.
|
||||||
|
func TestCheckSyncBranchHookQuick(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
syncBranchEnv string
|
||||||
|
hasGitDir bool
|
||||||
|
hookVersion string
|
||||||
|
expectIssue bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "old hook with sync-branch",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.24.0",
|
||||||
|
expectIssue: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "compatible hook with sync-branch",
|
||||||
|
syncBranchEnv: "beads-sync",
|
||||||
|
hasGitDir: true,
|
||||||
|
hookVersion: "0.29.0",
|
||||||
|
expectIssue: false,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tc := range tests {
|
||||||
|
t.Run(tc.name, func(t *testing.T) {
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
|
||||||
|
// Always set environment variable to control sync-branch detection
|
||||||
|
// This overrides any config.yaml value in the actual beads repo
|
||||||
|
t.Setenv("BEADS_SYNC_BRANCH", tc.syncBranchEnv)
|
||||||
|
|
||||||
|
if tc.hasGitDir {
|
||||||
|
// Initialize a real git repo (git rev-parse needs this)
|
||||||
|
cmd := exec.Command("git", "init")
|
||||||
|
cmd.Dir = tmpDir
|
||||||
|
if err := cmd.Run(); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if tc.hookVersion != "" {
|
||||||
|
hooksDir := filepath.Join(tmpDir, ".git", "hooks")
|
||||||
|
hookPath := filepath.Join(hooksDir, "pre-push")
|
||||||
|
hookContent := fmt.Sprintf("#!/bin/sh\n# bd-hooks-version: %s\nexit 0\n", tc.hookVersion)
|
||||||
|
if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
issue := checkSyncBranchHookQuick(tmpDir)
|
||||||
|
|
||||||
|
if tc.expectIssue && issue == "" {
|
||||||
|
t.Error("Expected issue to be reported, got empty string")
|
||||||
|
}
|
||||||
|
if !tc.expectIssue && issue != "" {
|
||||||
|
t.Errorf("Expected no issue, got: %s", issue)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user