From f5108030c28b2fb0e6345831a65dd65d22b6d54d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 3 Apr 2026 08:39:07 -0700 Subject: [PATCH 1/2] fix: make `entire clean --all` clean all sessions, not just orphaned Previously, `--all` only found orphaned sessions (those with no shadow branch AND no checkpoints). Active sessions were invisible to `--all`, so users had to use `--session ` to clean them individually. Co-Authored-By: Claude Opus 4.6 (1M context) Entire-Checkpoint: cb0888567b77 --- cmd/entire/cli/clean.go | 19 +++++---- cmd/entire/cli/clean_test.go | 63 ++++++++++++++++++++++++++++-- cmd/entire/cli/strategy/cleanup.go | 36 +++++++++++++++++ 3 files changed, 104 insertions(+), 14 deletions(-) diff --git a/cmd/entire/cli/clean.go b/cmd/entire/cli/clean.go index b2531f960..316fb3ba6 100644 --- a/cmd/entire/cli/clean.go +++ b/cmd/entire/cli/clean.go @@ -34,10 +34,9 @@ By default, cleans session state and shadow branches for the current HEAD: - Session state files (.git/entire-sessions/.json) - Shadow branch (entire/-) -Use --all to clean all orphaned Entire data across the repository: - - Orphaned shadow branches - - Orphaned session state files - - Orphaned checkpoint entries on entire/checkpoints/v1 +Use --all to clean all Entire session data across the repository: + - All session state files (.git/entire-sessions/) + - All shadow branches - Temporary files (.entire/tmp/) The entire/checkpoints/v1 branch itself is preserved. @@ -79,7 +78,7 @@ Use --dry-run to preview what would be deleted without prompting.`, } cmd.Flags().BoolVarP(&forceFlag, "force", "f", false, "Skip confirmation prompt and override active session guard") - cmd.Flags().BoolVarP(&allFlag, "all", "a", false, "Clean all orphaned data across the repository") + cmd.Flags().BoolVarP(&allFlag, "all", "a", false, "Clean all session data across the repository") cmd.Flags().BoolVarP(&dryRunFlag, "dry-run", "d", false, "Preview what would be deleted without deleting") cmd.Flags().StringVar(&sessionFlag, "session", "", "Clean a specific session by ID") @@ -257,12 +256,12 @@ func runCleanSession(ctx context.Context, cmd *cobra.Command, strat *strategy.Ma return nil } -// runCleanAll cleans all orphaned data across the repository (old `entire clean` behavior). +// runCleanAll cleans all session data across the repository. func runCleanAll(ctx context.Context, cmd *cobra.Command, force, dryRun bool) error { - // List all cleanup items - items, err := strategy.ListAllCleanupItems(ctx) + // List all items (sessions, shadow branches) — not just orphaned ones + items, err := strategy.ListAllItems(ctx) if err != nil { - return fmt.Errorf("failed to list orphaned items: %w", err) + return fmt.Errorf("failed to list items: %w", err) } // List temp files @@ -282,7 +281,7 @@ func runCleanAllWithItems(ctx context.Context, cmd *cobra.Command, force, dryRun errW := cmd.ErrOrStderr() // Handle no items case if len(items) == 0 && len(tempFiles) == 0 { - fmt.Fprintln(w, "No orphaned items to clean up.") + fmt.Fprintln(w, "No items to clean up.") return nil } diff --git a/cmd/entire/cli/clean_test.go b/cmd/entire/cli/clean_test.go index 606c7458e..382ce8135 100644 --- a/cmd/entire/cli/clean_test.go +++ b/cmd/entire/cli/clean_test.go @@ -360,8 +360,8 @@ func TestCleanCmd_All_NoOrphanedItems(t *testing.T) { } output := stdout.String() - if !strings.Contains(output, "No orphaned items") { - t.Errorf("Expected 'No orphaned items' message, got: %s", output) + if !strings.Contains(output, "No items to clean up") { + t.Errorf("Expected 'No items to clean up' message, got: %s", output) } } @@ -583,6 +583,61 @@ func TestCleanCmd_All_Subdirectory(t *testing.T) { } } +// Regression test: --all should find sessions that have a shadow branch. +// Previously, --all only cleaned orphaned sessions (no shadow branch AND no checkpoints), +// so active sessions with a shadow branch were invisible to --all. +func TestCleanCmd_All_FindsSessionWithShadowBranch(t *testing.T) { + repo, commitHash := setupCleanTestRepo(t) + + wt, err := repo.Worktree() + if err != nil { + t.Fatalf("failed to get worktree: %v", err) + } + worktreePath := wt.Filesystem.Root() + worktreeID, err := paths.GetWorktreeID(worktreePath) + if err != nil { + t.Fatalf("failed to get worktree ID: %v", err) + } + + // Create shadow branch for the session's base commit + shadowBranch := checkpoint.ShadowBranchNameForCommit(commitHash.String(), worktreeID) + shadowRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName(shadowBranch), commitHash) + if err := repo.Storer.SetReference(shadowRef); err != nil { + t.Fatalf("failed to create shadow branch: %v", err) + } + + // Create session state file — this session HAS a shadow branch, + // so it was NOT considered orphaned by the old --all behavior + sessionFile := createSessionStateFile(t, worktreePath, "2026-02-02-active-session", commitHash) + + cmd := newCleanCmd() + var stdout bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetArgs([]string{"--all", "--force"}) + + err = cmd.Execute() + if err != nil { + t.Fatalf("clean --all --force error = %v", err) + } + + output := stdout.String() + + // Session should be cleaned + if _, err := os.Stat(sessionFile); !os.IsNotExist(err) { + t.Error("session state file should be deleted by --all") + } + + // Shadow branch should be cleaned + refName := plumbing.NewBranchReferenceName(shadowBranch) + if _, err := repo.Reference(refName, true); err == nil { + t.Error("shadow branch should be deleted by --all") + } + + if !strings.Contains(output, "Deleted") { + t.Errorf("Expected 'Deleted' in output, got: %s", output) + } +} + // --- runCleanAllWithItems unit tests --- func TestRunCleanAllWithItems_PartialFailure(t *testing.T) { @@ -663,8 +718,8 @@ func TestRunCleanAllWithItems_NoItems(t *testing.T) { } output := stdout.String() - if !strings.Contains(output, "No orphaned items") { - t.Errorf("Expected 'No orphaned items' message, got: %s", output) + if !strings.Contains(output, "No items to clean up") { + t.Errorf("Expected 'No items to clean up' message, got: %s", output) } } diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 08125b07a..40e3737aa 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -360,6 +360,42 @@ func ListAllCleanupItems(ctx context.Context) ([]CleanupItem, error) { return items, firstErr } +// ListAllItems returns all Entire items (not just orphaned) for full cleanup. +// This includes all shadow branches and all session states regardless of +// whether they have checkpoints or active shadow branches. +func ListAllItems(ctx context.Context) ([]CleanupItem, error) { + var items []CleanupItem + + // All shadow branches + strat := NewManualCommitStrategy() + stratItems, err := strat.ListOrphanedItems(ctx) + if err != nil { + return nil, fmt.Errorf("listing shadow branches: %w", err) + } + items = append(items, stratItems...) + + // All session states (not just orphaned) + store, err := session.NewStateStore(ctx) + if err != nil { + return nil, fmt.Errorf("failed to create state store: %w", err) + } + + states, err := store.List(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list session states: %w", err) + } + + for _, state := range states { + items = append(items, CleanupItem{ + Type: CleanupTypeSessionState, + ID: state.SessionID, + Reason: "clean all", + }) + } + + return items, nil +} + // DeleteAllCleanupItems deletes all specified cleanup items. // Logs each deletion for audit purposes. func DeleteAllCleanupItems(ctx context.Context, items []CleanupItem) (*CleanupResult, error) { From 1f42ebfb7ef53db01077d50c8a5ed6814309cf07 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 3 Apr 2026 09:16:56 -0700 Subject: [PATCH 2/2] fix: address PR review feedback for clean --all - Use ListShadowBranches directly instead of ListOrphanedItems to avoid fragile coupling to orphan-detection logic - Remove dead ListAllCleanupItems function (no callers after prior commit) - Fix Short description still referencing "orphaned" - Remove listTempFiles active-session filter for --all path since those sessions are being deleted anyway (use listAllTempFiles instead) Co-Authored-By: Claude Opus 4.6 (1M context) Entire-Checkpoint: 14ecffdcb72a --- cmd/entire/cli/clean.go | 31 ++++++---------------- cmd/entire/cli/strategy/cleanup.go | 41 ++++++++---------------------- 2 files changed, 19 insertions(+), 53 deletions(-) diff --git a/cmd/entire/cli/clean.go b/cmd/entire/cli/clean.go index 316fb3ba6..30cbec024 100644 --- a/cmd/entire/cli/clean.go +++ b/cmd/entire/cli/clean.go @@ -27,7 +27,7 @@ func newCleanCmd() *cobra.Command { cmd := &cobra.Command{ Use: "clean", - Short: "Clean up session data and orphaned Entire data", + Short: "Clean up Entire session data", Long: `Clean up Entire session data for the current HEAD commit. By default, cleans session state and shadow branches for the current HEAD: @@ -264,8 +264,8 @@ func runCleanAll(ctx context.Context, cmd *cobra.Command, force, dryRun bool) er return fmt.Errorf("failed to list items: %w", err) } - // List temp files - tempFiles, err := listTempFiles(ctx) + // List temp files — skip active-session filter since --all deletes those sessions + tempFiles, err := listAllTempFiles(ctx) if err != nil { // Non-fatal: continue with other cleanup items fmt.Fprintf(cmd.ErrOrStderr(), "Warning: failed to list temp files: %v\n", err) @@ -274,7 +274,7 @@ func runCleanAll(ctx context.Context, cmd *cobra.Command, force, dryRun bool) er return runCleanAllWithItems(ctx, cmd, force, dryRun, items, tempFiles) } -// runCleanAllWithItems is the core logic for cleaning all orphaned items. +// runCleanAllWithItems is the core logic for cleaning all items. // Separated for testability — tests pass a cmd without a TTY and use force or dryRun to avoid prompts. func runCleanAllWithItems(ctx context.Context, cmd *cobra.Command, force, dryRun bool, items []strategy.CleanupItem, tempFiles []string) error { w := cmd.OutOrStdout() @@ -442,10 +442,9 @@ func runCleanAllWithItems(ctx context.Context, cmd *cobra.Command, force, dryRun return nil } -// listTempFiles returns files in .entire/tmp/ that are safe to delete, -// excluding files belonging to active sessions. -// Uses os.DirFS + fs.WalkDir to confine listing to the temp directory. -func listTempFiles(ctx context.Context) ([]string, error) { +// listAllTempFiles returns all files in .entire/tmp/ without filtering. +// Used by --all since those sessions are being deleted anyway. +func listAllTempFiles(ctx context.Context) ([]string, error) { absDir, err := paths.AbsPath(ctx, paths.EntireTmpDir) if err != nil { return nil, fmt.Errorf("failed to resolve temp dir: %w", err) @@ -459,14 +458,6 @@ func listTempFiles(ctx context.Context) ([]string, error) { } defer root.Close() - // Build set of active session IDs to protect their temp files - activeSessionIDs := make(map[string]bool) - if states, listErr := strategy.ListSessionStates(ctx); listErr == nil { - for _, state := range states { - activeSessionIDs[state.SessionID] = true - } - } - var files []string err = fs.WalkDir(root.FS(), ".", func(_ string, d fs.DirEntry, err error) error { if err != nil { @@ -475,13 +466,7 @@ func listTempFiles(ctx context.Context) ([]string, error) { if d.IsDir() { return nil } - // Skip temp files belonging to active sessions (e.g., "session-id.json") - name := d.Name() - sessionID := strings.TrimSuffix(name, ".json") - if sessionID != name && activeSessionIDs[sessionID] { - return nil - } - files = append(files, name) + files = append(files, d.Name()) return nil }) if os.IsNotExist(err) { diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 40e3737aa..3b562845f 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -335,44 +335,25 @@ func DeleteOrphanedCheckpoints(ctx context.Context, checkpointIDs []string) (del return checkpointIDs, []string{}, nil } -// ListAllCleanupItems returns all orphaned items across all categories. -// It iterates over all registered strategies and calls ListOrphanedItems on those -// that implement OrphanedItemsLister. -// Returns an error if the repository cannot be opened. -func ListAllCleanupItems(ctx context.Context) ([]CleanupItem, error) { - var items []CleanupItem - var firstErr error - - strat := NewManualCommitStrategy() - stratItems, err := strat.ListOrphanedItems(ctx) - if err != nil { - return nil, fmt.Errorf("listing orphaned items: %w", err) - } - items = append(items, stratItems...) - // Orphaned session states (strategy-agnostic) - states, err := ListOrphanedSessionStates(ctx) - if err != nil { - return nil, err - } - - items = append(items, states...) - - return items, firstErr -} - -// ListAllItems returns all Entire items (not just orphaned) for full cleanup. +// ListAllItems returns all Entire items for full cleanup. // This includes all shadow branches and all session states regardless of // whether they have checkpoints or active shadow branches. func ListAllItems(ctx context.Context) ([]CleanupItem, error) { var items []CleanupItem - // All shadow branches - strat := NewManualCommitStrategy() - stratItems, err := strat.ListOrphanedItems(ctx) + // All shadow branches (using ListShadowBranches directly, not + // ListOrphanedItems, so this won't break if orphan filtering is added) + branches, err := ListShadowBranches(ctx) if err != nil { return nil, fmt.Errorf("listing shadow branches: %w", err) } - items = append(items, stratItems...) + for _, branch := range branches { + items = append(items, CleanupItem{ + Type: CleanupTypeShadowBranch, + ID: branch, + Reason: "clean all", + }) + } // All session states (not just orphaned) store, err := session.NewStateStore(ctx)