* 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.
This commit is contained in:
committed by
GitHub
parent
4fffdb7fae
commit
2cbf3a5497
@@ -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)"
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user