fix(groups): reject rename when target path collides

internal/session/groups.go

@@ -1,6 +1,7 @@
 package session
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -12,6 +13,12 @@ import (
 	"github.com/asheshgoplani/agent-deck/internal/git"
 )
 
+// ErrGroupAlreadyExists is returned by RenameGroup when the target path collides with an existing group.
+var ErrGroupAlreadyExists = errors.New("group already exists at target path")
+
+// ErrGroupNotFound is returned by RenameGroup when oldPath does not resolve to an existing group.
+var ErrGroupNotFound = errors.New("group not found")
+
 // DefaultGroupName is the display name for the default group where ungrouped sessions go
 const DefaultGroupName = "My Sessions"
 
@@ -1148,10 +1155,11 @@ func (t *GroupTree) RenameTargetPath(oldPath, newName string) string {
 }
 
 // RenameGroup renames a group and updates all subgroups.
-func (t *GroupTree) RenameGroup(oldPath, newName string) {
+// Returns ErrGroupNotFound if oldPath doesn't exist, or ErrGroupAlreadyExists if the target path collides.
+func (t *GroupTree) RenameGroup(oldPath, newName string) error {
 	group, exists := t.Groups[oldPath]
 	if !exists {
-		return
+		return fmt.Errorf("%w: %s", ErrGroupNotFound, oldPath)
 	}
 
 	// Sanitize name to prevent path traversal and security issues
@@ -1160,7 +1168,19 @@ func (t *GroupTree) RenameGroup(oldPath, newName string) {
 
 	if newPath == oldPath {
 		group.Name = sanitizedName
-		return
+		return nil
+	}
+
+	if _, clash := t.Groups[newPath]; clash {
+		return fmt.Errorf("%w: %s", ErrGroupAlreadyExists, newPath)
+	}
+	for path := range t.Groups {
+		if strings.HasPrefix(path, oldPath+"/") {
+			newSubPath := newPath + path[len(oldPath):]
+			if _, clash := t.Groups[newSubPath]; clash {
+				return fmt.Errorf("%w: %s", ErrGroupAlreadyExists, newSubPath)
+			}
+		}
 	}
 
 	// Update all sessions in the group
@@ -1202,6 +1222,7 @@ func (t *GroupTree) RenameGroup(oldPath, newName string) {
 	t.Expanded[newPath] = group.Expanded
 
 	t.rebuildGroupList()
+	return nil
 }
 
 // MoveGroupTo reparents a group (and its entire subtree) under destParentPath.

internal/session/groups_test.go

@@ -1,6 +1,7 @@
 package session
 
 import (
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -524,6 +525,70 @@ func TestRenameSubgroup(t *testing.T) {
 	}
 }
 
+func TestRenameGroup_ReportsNotFound(t *testing.T) {
+	tree := NewGroupTree([]*Instance{})
+	tree.CreateGroup("real")
+
+	err := tree.RenameGroup("stale-name", "whatever")
+	if !errors.Is(err, ErrGroupNotFound) {
+		t.Fatalf("expected ErrGroupNotFound, got %v", err)
+	}
+	if tree.Groups["real"] == nil {
+		t.Error("existing group should be untouched")
+	}
+}
+
+func TestRenameGroup_RejectsCollision(t *testing.T) {
+	tree := NewGroupTree([]*Instance{})
+	tree.CreateGroup("source")
+	tree.CreateGroup("target")
+	tree.Groups["source"].Sessions = []*Instance{{ID: "s1", GroupPath: "source"}}
+	tree.Groups["target"].Sessions = []*Instance{{ID: "t1", GroupPath: "target"}}
+
+	err := tree.RenameGroup("source", "target")
+	if !errors.Is(err, ErrGroupAlreadyExists) {
+		t.Fatalf("expected ErrGroupAlreadyExists, got %v", err)
+	}
+
+	src := tree.Groups["source"]
+	if src == nil {
+		t.Fatal("source group should still exist")
+	}
+	if len(src.Sessions) != 1 || src.Sessions[0].ID != "s1" {
+		t.Errorf("source sessions should be intact, got %+v", src.Sessions)
+	}
+
+	tgt := tree.Groups["target"]
+	if tgt == nil {
+		t.Fatal("target group should still exist")
+	}
+	if len(tgt.Sessions) != 1 || tgt.Sessions[0].ID != "t1" {
+		t.Errorf("target sessions should be intact, got %+v", tgt.Sessions)
+	}
+}
+
+func TestRenameGroup_RejectsSubtreeCollision(t *testing.T) {
+	tree := NewGroupTree([]*Instance{})
+	tree.CreateGroup("Alpha")
+	tree.CreateSubgroup("Alpha", "Child")
+	tree.CreateGroup("Beta")
+	tree.CreateSubgroup("Beta", "Child")
+	tree.Groups["Beta/Child"].Sessions = []*Instance{{ID: "victim", GroupPath: "Beta/Child"}}
+
+	err := tree.RenameGroup("Alpha", "Beta")
+	if !errors.Is(err, ErrGroupAlreadyExists) {
+		t.Fatalf("expected ErrGroupAlreadyExists, got %v", err)
+	}
+
+	if tree.Groups["Alpha"] == nil || tree.Groups["Alpha/Child"] == nil {
+		t.Error("Alpha subtree should be intact after rejected rename")
+	}
+	victim := tree.Groups["Beta/Child"]
+	if victim == nil || len(victim.Sessions) != 1 || victim.Sessions[0].ID != "victim" {
+		t.Errorf("Beta/Child and its session should survive, got %+v", victim)
+	}
+}
+
 func TestDeleteGroup(t *testing.T) {
 	instances := []*Instance{
 		{ID: "1", Title: "session-1", GroupPath: "to-delete"},

internal/ui/home.go

@@ -9685,7 +9685,7 @@
 						slog.String("old_path", op.oldPath), slog.String("target", target))
 					continue
 				}
 				h.groupTree.RenameGroup(op.oldPath, op.name)
 				h.instancesMu.Lock()
 				h.instances = h.groupTree.GetAllInstances()
 				h.instancesMu.Unlock()
@@ -9757,7 +9757,10 @@
 			name := h.groupDialog.GetValue()
 			if name != "" {
 				oldPath := h.groupDialog.GetGroupPath()
-				h.groupTree.RenameGroup(oldPath, name)
+				if err := h.groupTree.RenameGroup(oldPath, name); err != nil {
+					h.setError(err)
+					break
+				}
 				h.pendingGroupOps = append(h.pendingGroupOps, pendingGroupOp{
 					kind: groupOpRename, oldPath: oldPath, name: name,
 				})

internal/ui/web_mutator.go

@@ -532,7 +532,9 @@ func (m *WebMutator) RenameGroup(groupPath, newName string) error {
 		return err
 	}
 	defer unlock()
-	m.h.groupTree.RenameGroup(groupPath, newName)
+	if err := m.h.groupTree.RenameGroup(groupPath, newName); err != nil {
+		return err
+	}
 
 	storage, err := session.NewStorageWithProfile(m.h.profile)
 	if err != nil {