From 2cbf3a54972aa4d5beeea35e714feb7e2c0f8ea7 Mon Sep 17 00:00:00 2001 From: Peter Chanthamynavong Date: Mon, 19 Jan 2026 10:11:06 -0800 Subject: [PATCH] fix: Validate sync-branch at config-time and runtime (closes #1166) (#1168) * fix(config): validate sync-branch at config time Add sync-branch validation to validateYamlConfigValue() to reject main/master at config time, preventing the validation bypass in GH#1166. - Add case for sync-branch and sync.branch keys - Inline validation logic to avoid import cycle (config <-> syncbranch) - Add unit tests for rejection (main/master) and acceptance (valid names) Part of: #1166 * fix(sync): add runtime guard for sync-branch == current-branch Add dynamic runtime check before worktree operations to catch cases where sync-branch matches the current branch. This provides defense in depth for manual YAML edits, pre-fix configs, or non-main/master branch names (trunk, develop, production, etc.). - Check IsSyncBranchSameAsCurrent() after hasSyncBranchConfig is set - Position check BEFORE worktree entry (CWD changes inside worktree) - Add integration test TestSync_FailsWhenOnSyncBranch Part of: #1166 * docs: note main/master restriction in sync-branch FAQ Clarifies that git worktrees cannot checkout the same branch in multiple locations, so main/master cannot be used as sync branch. --- cmd/bd/sync.go | 11 ++++ cmd/bd/sync_modes_test.go | 79 +++++++++++++++++++++++++++++ docs/PROTECTED_BRANCHES.md | 2 +- internal/config/yaml_config.go | 7 +++ internal/config/yaml_config_test.go | 50 ++++++++++++++++++ 5 files changed, 148 insertions(+), 1 deletion(-) diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index a8b94463..aabcc49b 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -273,6 +273,17 @@ The --full flag provides the legacy full sync behavior for backwards compatibili } hasSyncBranchConfig := syncBranchName != "" + // GH#1166: Block sync if currently on the sync branch + // This must happen BEFORE worktree operations - after entering a worktree, + // GetCurrentBranch() would return the worktree's branch, not the original. + if hasSyncBranchConfig { + if syncbranch.IsSyncBranchSameAsCurrent(ctx, syncBranchName) { + FatalError("Cannot sync to '%s': it's your current branch. "+ + "Checkout a different branch first, or use a dedicated sync branch like 'beads-sync'.", + syncBranchName) + } + } + // bd-wayc3: Check for redirect + sync-branch incompatibility // Redirect and sync-branch are mutually exclusive: // - Redirect says: "My database is in another repo (I am a client)" diff --git a/cmd/bd/sync_modes_test.go b/cmd/bd/sync_modes_test.go index 0b18a76e..b92a9da8 100644 --- a/cmd/bd/sync_modes_test.go +++ b/cmd/bd/sync_modes_test.go @@ -773,3 +773,82 @@ func TestExportOnlySync(t *testing.T) { } t.Log("✓ issues.jsonl is committed after export-only sync") } + +// TestSync_FailsWhenOnSyncBranch verifies that bd sync fails gracefully +// when the current branch matches the configured sync branch. +// This is the runtime guard for GH#1166 - prevents bd sync from attempting +// to create a worktree for a branch that's already checked out. +// +// The issue: If sync.branch = "main" and user is on "main", bd sync would +// try to create a worktree for "main" which fails because it's already checked out. +// Worse, some code paths could commit all staged files instead of just .beads/. +func TestSync_FailsWhenOnSyncBranch(t *testing.T) { + ctx := context.Background() + tmpDir, cleanup := setupGitRepo(t) + defer cleanup() + + // Get current branch name (should be "main" from setupGitRepo) + currentBranch, err := syncbranch.GetCurrentBranch(ctx) + if err != nil { + t.Fatalf("failed to get current branch: %v", err) + } + t.Logf("Current branch: %s", currentBranch) + + // Test 1: IsSyncBranchSameAsCurrent returns true when branch matches + if !syncbranch.IsSyncBranchSameAsCurrent(ctx, currentBranch) { + t.Error("IsSyncBranchSameAsCurrent should return true when sync branch matches current branch") + } + t.Log("✓ IsSyncBranchSameAsCurrent correctly detects same-branch condition") + + // Test 2: IsSyncBranchSameAsCurrent returns false for different branch + if syncbranch.IsSyncBranchSameAsCurrent(ctx, "beads-sync") { + t.Error("IsSyncBranchSameAsCurrent should return false for different branch name") + } + t.Log("✓ IsSyncBranchSameAsCurrent correctly allows different branch names") + + // Test 3: Setup sync.branch config to match current branch (the problematic state) + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.MkdirAll(beadsDir, 0755); err != nil { + t.Fatalf("failed to create .beads dir: %v", err) + } + + dbPath := filepath.Join(beadsDir, "beads.db") + testStore, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("failed to create store: %v", err) + } + defer testStore.Close() + + // Configure sync.branch to match current branch (this is the bug condition) + if err := testStore.SetConfig(ctx, "sync.branch", currentBranch); err != nil { + t.Fatalf("failed to set sync.branch: %v", err) + } + t.Logf("✓ Configured sync.branch = %s (matches current branch)", currentBranch) + + // Verify the config is stored correctly + syncBranchValue, err := testStore.GetConfig(ctx, "sync.branch") + if err != nil { + t.Fatalf("failed to get sync.branch config: %v", err) + } + if syncBranchValue != currentBranch { + t.Errorf("sync.branch config = %q, want %q", syncBranchValue, currentBranch) + } + + // Test 4: The runtime guard logic (same as in sync.go) + // This simulates what happens in the sync command + syncBranchName := syncBranchValue + hasSyncBranchConfig := syncBranchName != "" + + if hasSyncBranchConfig { + if syncbranch.IsSyncBranchSameAsCurrent(ctx, syncBranchName) { + // This is the expected behavior - we caught the misconfiguration + t.Log("✓ Runtime guard correctly detected sync-branch = current-branch condition") + } else { + t.Error("FAIL: Runtime guard did not detect sync-branch = current-branch condition") + } + } else { + t.Error("FAIL: sync.branch config should be set") + } + + t.Log("✓ TestSync_FailsWhenOnSyncBranch PASSED") +} diff --git a/docs/PROTECTED_BRANCHES.md b/docs/PROTECTED_BRANCHES.md index df55a092..a5980713 100644 --- a/docs/PROTECTED_BRANCHES.md +++ b/docs/PROTECTED_BRANCHES.md @@ -426,7 +426,7 @@ No! This is a pure git solution that works on any platform. Just protect your `m ### Can I use a different branch name? -Yes! Use any branch name you want: +Yes! Use any branch name except `main` or `master` (git worktrees cannot checkout the same branch in multiple locations): ```bash bd init --branch my-custom-branch diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index 3d22f7f0..51bf292d 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -297,6 +297,13 @@ func validateYamlConfigValue(key, value string) error { if depth < 1 { return fmt.Errorf("hierarchy.max-depth must be at least 1, got %d", depth) } + case "sync-branch", "sync.branch": + // GH#1166: Validate sync branch name at config time + // Note: Cannot import syncbranch due to import cycle, so inline the validation. + // This mirrors syncbranch.ValidateSyncBranchName() logic. + if value == "main" || value == "master" { + return fmt.Errorf("cannot use '%s' as sync branch: git worktrees prevent checking out the same branch in multiple locations. Use a dedicated branch like 'beads-sync' instead", value) + } } return nil } diff --git a/internal/config/yaml_config_test.go b/internal/config/yaml_config_test.go index c59c9447..7f9635ac 100644 --- a/internal/config/yaml_config_test.go +++ b/internal/config/yaml_config_test.go @@ -382,3 +382,53 @@ func TestValidateYamlConfigValue_OtherKeys(t *testing.T) { t.Errorf("unexpected error for routing.mode: %v", err) } } + +// TestValidateYamlConfigValue_SyncBranch_RejectsMain tests that main/master are rejected as sync branch (GH#1166) +func TestValidateYamlConfigValue_SyncBranch_RejectsMain(t *testing.T) { + tests := []struct { + name string + key string + value string + }{ + {"sync-branch main", "sync-branch", "main"}, + {"sync-branch master", "sync-branch", "master"}, + {"sync.branch main", "sync.branch", "main"}, + {"sync.branch master", "sync.branch", "master"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateYamlConfigValue(tt.key, tt.value) + if err == nil { + t.Errorf("expected error for %s=%s, got nil", tt.key, tt.value) + } + if err != nil && !strings.Contains(err.Error(), "cannot use") { + t.Errorf("expected 'cannot use' error, got: %v", err) + } + }) + } +} + +// TestValidateYamlConfigValue_SyncBranch_AcceptsValid tests that valid branch names are accepted (GH#1166) +func TestValidateYamlConfigValue_SyncBranch_AcceptsValid(t *testing.T) { + tests := []struct { + name string + key string + value string + }{ + {"sync-branch beads-sync", "sync-branch", "beads-sync"}, + {"sync-branch feature/test", "sync-branch", "feature/test"}, + {"sync.branch beads-sync", "sync.branch", "beads-sync"}, + {"sync.branch develop", "sync.branch", "develop"}, + {"sync-branch empty", "sync-branch", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateYamlConfigValue(tt.key, tt.value) + if err != nil { + t.Errorf("unexpected error for %s=%s: %v", tt.key, tt.value, err) + } + }) + } +}