diff --git a/internal/session/groups.go b/internal/session/groups.go index 14f034c1..ed3af5d8 100644 --- a/internal/session/groups.go +++ b/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. diff --git a/internal/session/groups_test.go b/internal/session/groups_test.go index 32c53aef..36261cc7 100644 --- a/internal/session/groups_test.go +++ b/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"}, diff --git a/internal/ui/home.go b/internal/ui/home.go index 9d85fa3f..73458d83 100644 --- a/internal/ui/home.go +++ b/internal/ui/home.go @@ -9685,7 +9685,11 @@ func (h *Home) reapplyPendingGroupOps() bool { slog.String("old_path", op.oldPath), slog.String("target", target)) continue } - h.groupTree.RenameGroup(op.oldPath, op.name) + if err := h.groupTree.RenameGroup(op.oldPath, op.name); err != nil { + uiLog.Warn("pending_group_rename_failed", + slog.String("old_path", op.oldPath), slog.String("name", op.name), slog.String("err", err.Error())) + continue + } h.instancesMu.Lock() h.instances = h.groupTree.GetAllInstances() h.instancesMu.Unlock() @@ -9757,7 +9761,10 @@ func (h *Home) handleGroupDialogKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { 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, }) diff --git a/internal/ui/web_mutator.go b/internal/ui/web_mutator.go index ace2b85d..8f6080e7 100644 --- a/internal/ui/web_mutator.go +++ b/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 {