[fix] Atomicity and collision handling in rename and duplicate (#171) (#187)

* [fix] Rollback worktree on rename branch failure; guard duplicate collision

B5: RenameWorktree now attempts to move the worktree back to its original
path when git branch -m fails. If rollback succeeds the error surfaces
"moved back" plus the retry hint; if rollback itself fails the error
includes the rollback failure and a full manual-recovery command.

B6: The branch and path collision checks in duplicate were already in
place; this commit adds the missing test coverage — a unit test for the
worktree-path collision path (branch absent, directory present) and an
e2e test for `duplicate --as <existing-task>`.

Closes #171

* [fix] Address review feedback: resolver-derived path + remove dead assertion

- cmd/duplicate_test.go: derive worktree path via resolver.WorktreePath
  instead of hardcoded DirName-derived string so the test stays resilient
  to future DirName changes
- tests/e2e/rename_test.go: remove dead `_ = newBranch` suppression
  (newBranch is already used to compute newPath which is asserted)

Author: lugassawan

cmd/duplicate_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/lugassawan/rimba/internal/config"
+	"github.com/lugassawan/rimba/internal/resolver"
 )
 
 func TestDuplicateDefaultBranchError(t *testing.T) {
@@ -208,6 +209,62 @@ func TestDuplicateBranchAlreadyExists(t *testing.T) {
 	}
 }
 
+func TestDuplicateWorktreePathAlreadyExists(t *testing.T) {
+	repoDir := t.TempDir()
+	wtDir := filepath.Join(repoDir, "worktrees")
+	_ = os.MkdirAll(wtDir, 0755)
+	cfg := &config.Config{DefaultSource: branchMain, WorktreeDir: "worktrees"}
+
+	// Create the target worktree path on disk (branch doesn't exist, but directory does)
+	destBranch := resolver.FullBranchName("", "feature/", "orphaned")
+	destPath := resolver.WorktreePath(wtDir, destBranch)
+	_ = os.MkdirAll(destPath, 0755)
+
+	worktreeOut := strings.Join([]string{
+		wtPrefix + repoDir,
+		headABC123,
+		branchRefMain,
+		"",
+		wtFeatureLogin,
+		headDEF456,
+		branchRefFeatureLogin,
+		"",
+	}, "\n")
+
+	r := &mockRunner{
+		run: func(args ...string) (string, error) {
+			if len(args) >= 2 && args[1] == cmdGitCommonDir {
+				return filepath.Join(repoDir, ".git"), nil
+			}
+			if len(args) >= 2 && args[1] == cmdShowToplevel {
+				return repoDir, nil
+			}
+			if len(args) >= 1 && args[0] == cmdRevParse {
+				return "", errGitFailed // BranchExists returns false
+			}
+			return worktreeOut, nil
+		},
+		runInDir: noopRunInDir,
+	}
+	restore := overrideNewRunner(r)
+	defer restore()
+
+	cmd, _ := newTestCmd()
+	cmd.Flags().String(flagAs, "", "")
+	cmd.Flags().Bool(flagSkipDeps, false, "")
+	cmd.Flags().Bool(flagSkipHooks, false, "")
+	_ = cmd.Flags().Set(flagAs, "orphaned")
+	cmd.SetContext(config.WithConfig(context.Background(), cfg))
+
+	err := duplicateCmd.RunE(cmd, []string{"login"})
+	if err == nil {
+		t.Fatal("expected error for existing worktree path")
+	}
+	if !strings.Contains(err.Error(), "already exists") {
+		t.Errorf("error = %q, want 'already exists'", err.Error())
+	}
+}
+
 func TestDuplicateWorktreeNotFound(t *testing.T) {
 	repoDir := t.TempDir()
 	cfg := &config.Config{DefaultSource: branchMain, WorktreeDir: "worktrees"}

internal/operations/rename.go

@@ -38,7 +38,12 @@ func RenameWorktree(r git.Runner, wt resolver.WorktreeInfo, newTask, wtDir strin
 	}
 
 	if err := git.RenameBranch(r, wt.Branch, newBranch); err != nil {
-		return RenameResult{}, fmt.Errorf("worktree moved but failed to rename branch %q: %w\nTo complete manually: git branch -m %s %s", wt.Branch, err, wt.Branch, newBranch)
+		if rbErr := git.MoveWorktree(r, newPath, wt.Path, force); rbErr != nil {
+			return RenameResult{}, fmt.Errorf("failed to rename branch %q → %q: %w\nRollback failed — worktree is at %s: %w\nTo recover: git worktree move %s %s && git branch -m %s %s",
+				wt.Branch, newBranch, err, newPath, rbErr, newPath, wt.Path, wt.Branch, newBranch)
+		}
+		return RenameResult{}, fmt.Errorf("failed to rename branch %q → %q: %w\nWorktree moved back to %s\nTo retry: git branch -m %s %s",
+			wt.Branch, newBranch, err, wt.Path, wt.Branch, newBranch)
 	}
 
 	return RenameResult{

internal/operations/rename_test.go

@@ -11,6 +11,7 @@ import (
 const (
 	cmdRevParse     = "rev-parse"
 	cmdWorktree     = "worktree"
+	cmdMove         = "move"
 	wtDir           = "/worktrees"
 	branchAuth      = "feature/auth"
 	errMoveFailed   = "move failed"
@@ -71,7 +72,7 @@ func TestRenameWorktreeMoveFails(t *testing.T) {
 			if len(args) >= 1 && args[0] == cmdRevParse {
 				return "", errGitFailed
 			}
-			if len(args) >= 2 && args[0] == cmdWorktree && args[1] == "move" {
+			if len(args) >= 2 && args[0] == cmdWorktree && args[1] == cmdMove {
 				return "", errors.New(errMoveFailed)
 			}
 			return "", nil
@@ -87,11 +88,16 @@ func TestRenameWorktreeMoveFails(t *testing.T) {
 }
 
 func TestRenameWorktreeBranchRenameFails(t *testing.T) {
+	moveCount := 0
 	r := &mockRunner{
 		run: func(args ...string) (string, error) {
 			if len(args) >= 1 && args[0] == cmdRevParse {
 				return "", errGitFailed
 			}
+			if len(args) >= 2 && args[0] == cmdWorktree && args[1] == cmdMove {
+				moveCount++
+				return "", nil
+			}
 			if len(args) >= 2 && args[0] == "branch" && args[1] == "-m" {
 				return "", errors.New(errRenameFailed)
 			}
@@ -105,8 +111,49 @@ func TestRenameWorktreeBranchRenameFails(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected error from branch rename failure")
 	}
-	if !strings.Contains(err.Error(), "worktree moved but failed") {
-		t.Errorf("error = %q, want 'worktree moved but failed'", err.Error())
+	if !strings.Contains(err.Error(), "failed to rename branch") {
+		t.Errorf("error = %q, want 'failed to rename branch'", err.Error())
+	}
+	if !strings.Contains(err.Error(), "moved back") {
+		t.Errorf("error = %q, want 'moved back' (rollback confirmation)", err.Error())
+	}
+	if moveCount != 2 {
+		t.Errorf("expected 2 worktree move calls (forward + rollback), got %d", moveCount)
+	}
+}
+
+func TestRenameWorktreeBranchRenameFailsRollbackFails(t *testing.T) {
+	moveCount := 0
+	r := &mockRunner{
+		run: func(args ...string) (string, error) {
+			if len(args) >= 1 && args[0] == cmdRevParse {
+				return "", errGitFailed
+			}
+			if len(args) >= 2 && args[0] == cmdWorktree && args[1] == cmdMove {
+				moveCount++
+				if moveCount == 2 {
+					return "", errors.New("rollback move failed")
+				}
+				return "", nil
+			}
+			if len(args) >= 2 && args[0] == "branch" && args[1] == "-m" {
+				return "", errors.New(errRenameFailed)
+			}
+			return "", nil
+		},
+		runInDir: noopRunInDir,
+	}
+
+	wt := resolver.WorktreeInfo{Branch: branchFeature, Path: pathWtFeatureLogin}
+	_, err := RenameWorktree(r, wt, "auth", wtDir, false)
+	if err == nil {
+		t.Fatal("expected error from branch rename + rollback failure")
+	}
+	if !strings.Contains(err.Error(), "failed to rename branch") {
+		t.Errorf("error = %q, want 'failed to rename branch'", err.Error())
+	}
+	if !strings.Contains(err.Error(), "Rollback failed") {
+		t.Errorf("error = %q, want 'Rollback failed'", err.Error())
 	}
 }
 

tests/e2e/duplicate_test.go

@@ -176,6 +176,20 @@ func TestDuplicateCopiesDirectory(t *testing.T) {
 	}
 }
 
+func TestDuplicateAsCollision(t *testing.T) {
+	if testing.Short() {
+		t.Skip(skipE2E)
+	}
+
+	repo := setupInitializedRepo(t)
+	rimbaSuccess(t, repo, "add", taskDupColSrc)
+	rimbaSuccess(t, repo, "add", taskDupColDst)
+
+	// Duplicate source --as destination that already exists: must fail before any writes.
+	r := rimbaFail(t, repo, "duplicate", taskDupColSrc, "--as", taskDupColDst)
+	assertContains(t, r.Stderr, "already exists")
+}
+
 func TestDuplicateSourceNotFound(t *testing.T) {
 	if testing.Short() {
 		t.Skip(skipE2E)

tests/e2e/e2e_test.go

@@ -51,6 +51,8 @@ const (
 	taskDupA      = "task-a"
 	taskDupB      = "task-b"
 	taskMyCopy    = "my-copy"
+	taskDupColSrc = "dup-col-src"
+	taskDupColDst = "dup-col-dst"
 	secretContent = "SECRET=test"
 
 	// Merge test constants.

tests/e2e/rename_test.go

@@ -129,22 +129,36 @@ func TestRenameFailsBranchExists(t *testing.T) {
 	assertContains(t, r.Stderr, "already exists")
 }
 
-func TestRenamePartialFailBranchHint(t *testing.T) {
+func TestRenamePartialFailRollback(t *testing.T) {
 	if testing.Short() {
 		t.Skip(skipE2E)
 	}
 
 	repo := setupInitializedRepo(t)
 	rimbaSuccess(t, repo, "add", "--bugfix", "rn-partial-old")
 
+	cfg := loadConfig(t, repo)
+	wtDir := filepath.Join(repo, cfg.WorktreeDir)
+	oldBranch := resolver.BranchName(bugfixPrefix, "rn-partial-old")
+	oldPath := resolver.WorktreePath(wtDir, oldBranch)
+	newBranch := resolver.BranchName(bugfixPrefix, "rn-partial-new")
+	newPath := resolver.WorktreePath(wtDir, newBranch)
+
 	// Create a sub-branch that blocks the rename target in the git ref namespace.
 	// "bugfix/rn-partial-new/sub" makes bugfix/rn-partial-new a directory in
 	// .git/refs/heads/, so git branch -m cannot create it as a file.
 	testutil.GitCmd(t, repo, "branch", "bugfix/rn-partial-new/sub")
 
 	r := rimbaFail(t, repo, "rename", "rn-partial-old", "rn-partial-new")
-	assertContains(t, r.Stderr, "worktree moved but failed to rename branch")
+
+	// Error should report the branch rename failure and successful rollback.
+	assertContains(t, r.Stderr, "failed to rename branch")
+	assertContains(t, r.Stderr, "moved back")
 	assertContains(t, r.Stderr, "git branch -m")
+
+	// Worktree should be back at its original path (rollback succeeded).
+	assertFileExists(t, oldPath)
+	assertFileNotExists(t, newPath)
 }
 
 func TestRenameFailsNoArgs(t *testing.T) {