From 1489a88e1ae587723d533b3d8b33b397bdb45b77 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Mon, 5 Jan 2026 14:48:47 +0100 Subject: [PATCH 1/3] cleanup more, make it owned by strategy Entire-Checkpoint: 94de498254e2 --- cmd/entire/cli/clean.go | 90 ---- cmd/entire/cli/cleanup.go | 176 ++++++++ .../cli/{clean_test.go => cleanup_test.go} | 109 +++-- cmd/entire/cli/root.go | 1 - cmd/entire/cli/session.go | 1 + cmd/entire/cli/strategy/auto_commit.go | 108 +++++ cmd/entire/cli/strategy/clean.go | 103 ----- cmd/entire/cli/strategy/cleanup.go | 420 ++++++++++++++++++ cmd/entire/cli/strategy/manual_commit.go | 26 ++ cmd/entire/cli/strategy/strategy.go | 12 + 10 files changed, 825 insertions(+), 221 deletions(-) delete mode 100644 cmd/entire/cli/clean.go create mode 100644 cmd/entire/cli/cleanup.go rename cmd/entire/cli/{clean_test.go => cleanup_test.go} (70%) delete mode 100644 cmd/entire/cli/strategy/clean.go create mode 100644 cmd/entire/cli/strategy/cleanup.go diff --git a/cmd/entire/cli/clean.go b/cmd/entire/cli/clean.go deleted file mode 100644 index 0a3fba36bf..0000000000 --- a/cmd/entire/cli/clean.go +++ /dev/null @@ -1,90 +0,0 @@ -package cli - -import ( - "fmt" - "io" - - "entire.io/cli/cmd/entire/cli/strategy" - "github.com/spf13/cobra" -) - -func newCleanCmd() *cobra.Command { - var forceFlag bool - - cmd := &cobra.Command{ - Use: "clean", - Short: "Clean up shadow branches", - Long: `Remove ephemeral shadow branches created by the manual-commit strategy. - -Shadow branches (entire/) store checkpoint metadata and can -accumulate over time. This command helps clean them up. - -Without --force, shows a preview of branches that would be deleted. -With --force, actually deletes the branches. - -The entire/sessions branch is never deleted as it contains permanent -session metadata.`, - RunE: func(cmd *cobra.Command, _ []string) error { - return runClean(cmd.OutOrStdout(), forceFlag) - }, - } - - cmd.Flags().BoolVarP(&forceFlag, "force", "f", false, "Actually delete branches (otherwise just preview)") - - return cmd -} - -func runClean(w io.Writer, force bool) error { - // List all shadow branches - branches, err := strategy.ListShadowBranches() - if err != nil { - return fmt.Errorf("failed to list shadow branches: %w", err) - } - - return runCleanWithBranches(w, force, branches) -} - -// runCleanWithBranches is the core logic for cleaning branches. -// Separated for testability. -func runCleanWithBranches(w io.Writer, force bool, branches []string) error { - // Handle no branches case - if len(branches) == 0 { - fmt.Fprintln(w, "No shadow branches to clean up.") - return nil - } - - // Preview mode (default) - if !force { - fmt.Fprintf(w, "%d shadow branches found:\n", len(branches)) - for _, branch := range branches { - fmt.Fprintf(w, " %s\n", branch) - } - fmt.Fprintln(w, "") - fmt.Fprintln(w, "Run with --force to delete these branches.") - return nil - } - - // Force mode - delete branches - deleted, failed, err := strategy.DeleteShadowBranches(branches) - if err != nil { - return fmt.Errorf("failed to delete shadow branches: %w", err) - } - - // Report results - if len(deleted) > 0 { - fmt.Fprintf(w, "Deleted %d shadow branches:\n", len(deleted)) - for _, branch := range deleted { - fmt.Fprintf(w, " %s\n", branch) - } - } - - if len(failed) > 0 { - fmt.Fprintf(w, "\nFailed to delete %d branches:\n", len(failed)) - for _, branch := range failed { - fmt.Fprintf(w, " %s\n", branch) - } - return fmt.Errorf("failed to delete %d branches", len(failed)) - } - - return nil -} diff --git a/cmd/entire/cli/cleanup.go b/cmd/entire/cli/cleanup.go new file mode 100644 index 0000000000..0d53b788b6 --- /dev/null +++ b/cmd/entire/cli/cleanup.go @@ -0,0 +1,176 @@ +package cli + +import ( + "fmt" + "io" + + "entire.io/cli/cmd/entire/cli/strategy" + "github.com/spf13/cobra" +) + +func newCleanCmd() *cobra.Command { + var forceFlag bool + + cmd := &cobra.Command{ + Use: "cleanup", + Short: "Clean up orphaned session data", + Long: `Remove orphaned session data that wasn't cleaned up automatically. + +This command finds and removes orphaned data from any strategy: + + Shadow branches (entire/) + Created by manual-commit strategy. Normally auto-cleaned when sessions + are condensed during commits. + + Session state files (.git/entire-sessions/) + Track active sessions. Orphaned when no checkpoints or shadow branches + reference them. + + Checkpoint metadata (entire/sessions branch) + For auto-commit checkpoints: orphaned when commits are rebased/squashed + and no commit references the checkpoint ID anymore. + Manual-commit checkpoints are permanent (condensed history) and are + never considered orphaned. + +Without --force, shows a preview of items that would be deleted. +With --force, actually deletes the orphaned items. + +The entire/sessions branch itself is never deleted.`, + RunE: func(cmd *cobra.Command, _ []string) error { + return runClean(cmd.OutOrStdout(), forceFlag) + }, + } + + cmd.Flags().BoolVarP(&forceFlag, "force", "f", false, "Actually delete items (otherwise just preview)") + + return cmd +} + +func runClean(w io.Writer, force bool) error { + // List all cleanup items + items, err := strategy.ListAllCleanupItems() + if err != nil { + return fmt.Errorf("failed to list orphaned items: %w", err) + } + + return runCleanWithItems(w, force, items) +} + +// runCleanWithItems is the core logic for cleaning orphaned items. +// Separated for testability. +func runCleanWithItems(w io.Writer, force bool, items []strategy.CleanupItem) error { + // Handle no items case + if len(items) == 0 { + fmt.Fprintln(w, "No orphaned items to clean up.") + return nil + } + + // Group items by type for display + var branches, states, checkpoints []strategy.CleanupItem + for _, item := range items { + switch item.Type { + case strategy.CleanupTypeShadowBranch: + branches = append(branches, item) + case strategy.CleanupTypeSessionState: + states = append(states, item) + case strategy.CleanupTypeCheckpoint: + checkpoints = append(checkpoints, item) + } + } + + // Preview mode (default) + if !force { + fmt.Fprintf(w, "Found %d orphaned items:\n\n", len(items)) + + if len(branches) > 0 { + fmt.Fprintf(w, "Shadow branches (%d):\n", len(branches)) + for _, item := range branches { + fmt.Fprintf(w, " %s\n", item.ID) + } + fmt.Fprintln(w) + } + + if len(states) > 0 { + fmt.Fprintf(w, "Session states (%d):\n", len(states)) + for _, item := range states { + fmt.Fprintf(w, " %s\n", item.ID) + } + fmt.Fprintln(w) + } + + if len(checkpoints) > 0 { + fmt.Fprintf(w, "Checkpoint metadata (%d):\n", len(checkpoints)) + for _, item := range checkpoints { + fmt.Fprintf(w, " %s\n", item.ID) + } + fmt.Fprintln(w) + } + + fmt.Fprintln(w, "Run with --force to delete these items.") + return nil + } + + // Force mode - delete items + result, err := strategy.DeleteAllCleanupItems(items) + if err != nil { + return fmt.Errorf("failed to delete orphaned items: %w", err) + } + + // Report results + totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints) + totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + + if totalDeleted > 0 { + fmt.Fprintf(w, "Deleted %d items:\n", totalDeleted) + + if len(result.ShadowBranches) > 0 { + fmt.Fprintf(w, "\n Shadow branches (%d):\n", len(result.ShadowBranches)) + for _, branch := range result.ShadowBranches { + fmt.Fprintf(w, " %s\n", branch) + } + } + + if len(result.SessionStates) > 0 { + fmt.Fprintf(w, "\n Session states (%d):\n", len(result.SessionStates)) + for _, state := range result.SessionStates { + fmt.Fprintf(w, " %s\n", state) + } + } + + if len(result.Checkpoints) > 0 { + fmt.Fprintf(w, "\n Checkpoints (%d):\n", len(result.Checkpoints)) + for _, cp := range result.Checkpoints { + fmt.Fprintf(w, " %s\n", cp) + } + } + } + + if totalFailed > 0 { + fmt.Fprintf(w, "\nFailed to delete %d items:\n", totalFailed) + + if len(result.FailedBranches) > 0 { + fmt.Fprintf(w, "\n Shadow branches:\n") + for _, branch := range result.FailedBranches { + fmt.Fprintf(w, " %s\n", branch) + } + } + + if len(result.FailedStates) > 0 { + fmt.Fprintf(w, "\n Session states:\n") + for _, state := range result.FailedStates { + fmt.Fprintf(w, " %s\n", state) + } + } + + if len(result.FailedCheckpoints) > 0 { + fmt.Fprintf(w, "\n Checkpoints:\n") + for _, cp := range result.FailedCheckpoints { + fmt.Fprintf(w, " %s\n", cp) + } + } + + return fmt.Errorf("failed to delete %d items", totalFailed) + } + + return nil +} diff --git a/cmd/entire/cli/clean_test.go b/cmd/entire/cli/cleanup_test.go similarity index 70% rename from cmd/entire/cli/clean_test.go rename to cmd/entire/cli/cleanup_test.go index 2f0eb07d88..ff13d86107 100644 --- a/cmd/entire/cli/clean_test.go +++ b/cmd/entire/cli/cleanup_test.go @@ -7,6 +7,7 @@ import ( "testing" "entire.io/cli/cmd/entire/cli/paths" + "entire.io/cli/cmd/entire/cli/strategy" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" @@ -64,7 +65,7 @@ func setupCleanTestRepo(t *testing.T) (*git.Repository, plumbing.Hash) { return repo, commitHash } -func TestRunClean_NoShadowBranches(t *testing.T) { +func TestRunClean_NoOrphanedItems(t *testing.T) { setupCleanTestRepo(t) var stdout bytes.Buffer @@ -74,8 +75,8 @@ func TestRunClean_NoShadowBranches(t *testing.T) { } output := stdout.String() - if !strings.Contains(output, "No shadow branches") { - t.Errorf("Expected 'No shadow branches' message, got: %s", output) + if !strings.Contains(output, "No orphaned items") { + t.Errorf("Expected 'No orphaned items' message, got: %s", output) } } @@ -106,8 +107,8 @@ func TestRunClean_PreviewMode(t *testing.T) { output := stdout.String() // Should show preview header - if !strings.Contains(output, "shadow branches found") { - t.Errorf("Expected 'shadow branches found' in output, got: %s", output) + if !strings.Contains(output, "orphaned items") { + t.Errorf("Expected 'orphaned items' in output, got: %s", output) } // Should list the shadow branches @@ -253,10 +254,10 @@ func TestRunClean_Subdirectory(t *testing.T) { } } -func TestRunClean_PartialFailureReturnsError(t *testing.T) { - // This test verifies that runClean returns a non-zero exit code when some - // branch deletions fail. We use runCleanWithBranches to inject a list - // containing both existing and non-existing branches. +func TestRunCleanWithItems_PartialFailure(t *testing.T) { + // This test verifies that runCleanWithItems returns an error when some + // deletions fail. We use runCleanWithItems to inject a list + // containing both existing and non-existing items. repo, commitHash := setupCleanTestRepo(t) @@ -266,17 +267,18 @@ func TestRunClean_PartialFailureReturnsError(t *testing.T) { t.Fatalf("failed to create shadow branch: %v", err) } - // Call runCleanWithBranches with a mix of existing and non-existing branches - // This simulates the scenario where a branch was deleted between - // ListShadowBranches and DeleteShadowBranches calls - branches := []string{"entire/abc1234", "entire/nonexistent1234567"} + // Call runCleanWithItems with a mix of existing and non-existing branches + items := []strategy.CleanupItem{ + {Type: strategy.CleanupTypeShadowBranch, ID: "entire/abc1234", Reason: "test"}, + {Type: strategy.CleanupTypeShadowBranch, ID: "entire/nonexistent1234567", Reason: "test"}, + } var stdout bytes.Buffer - err := runCleanWithBranches(&stdout, true, branches) // force=true + err := runCleanWithItems(&stdout, true, items) // force=true // Should return an error because one branch failed to delete if err == nil { - t.Fatal("runCleanWithBranches() should return error when branches fail to delete") + t.Fatal("runCleanWithItems() should return error when items fail to delete") } // Error message should indicate the failure @@ -286,35 +288,38 @@ func TestRunClean_PartialFailureReturnsError(t *testing.T) { // Output should show the successful deletion output := stdout.String() - if !strings.Contains(output, "Deleted 1 shadow") { + if !strings.Contains(output, "Deleted 1 items") { t.Errorf("Output should show successful deletion, got: %s", output) } // Output should also show the failures - if !strings.Contains(output, "Failed to delete 1 branch") { + if !strings.Contains(output, "Failed to delete 1 items") { t.Errorf("Output should show failures, got: %s", output) } } -func TestRunClean_AllFailuresReturnsError(t *testing.T) { - // Test that error is returned when ALL branches fail to delete +func TestRunCleanWithItems_AllFailures(t *testing.T) { + // Test that error is returned when ALL items fail to delete setupCleanTestRepo(t) - // Call runCleanWithBranches with only non-existing branches - branches := []string{"entire/nonexistent1234567", "entire/alsononexistent"} + // Call runCleanWithItems with only non-existing branches + items := []strategy.CleanupItem{ + {Type: strategy.CleanupTypeShadowBranch, ID: "entire/nonexistent1234567", Reason: "test"}, + {Type: strategy.CleanupTypeShadowBranch, ID: "entire/alsononexistent", Reason: "test"}, + } var stdout bytes.Buffer - err := runCleanWithBranches(&stdout, true, branches) // force=true + err := runCleanWithItems(&stdout, true, items) // force=true - // Should return an error because all branches failed to delete + // Should return an error because all items failed to delete if err == nil { - t.Fatal("runCleanWithBranches() should return error when branches fail to delete") + t.Fatal("runCleanWithItems() should return error when items fail to delete") } // Error message should indicate 2 failures - if !strings.Contains(err.Error(), "failed to delete 2 branches") { - t.Errorf("Error should mention 'failed to delete 2 branches', got: %v", err) + if !strings.Contains(err.Error(), "failed to delete 2 items") { + t.Errorf("Error should mention 'failed to delete 2 items', got: %v", err) } // Output should NOT show any successful deletions @@ -324,7 +329,57 @@ func TestRunClean_AllFailuresReturnsError(t *testing.T) { } // Output should show the failures - if !strings.Contains(output, "Failed to delete 2 branches") { + if !strings.Contains(output, "Failed to delete 2 items") { t.Errorf("Output should show failures, got: %s", output) } } + +func TestRunCleanWithItems_NoItems(t *testing.T) { + setupCleanTestRepo(t) + + var stdout bytes.Buffer + err := runCleanWithItems(&stdout, false, []strategy.CleanupItem{}) + if err != nil { + t.Fatalf("runCleanWithItems() error = %v", err) + } + + output := stdout.String() + if !strings.Contains(output, "No orphaned items") { + t.Errorf("Expected 'No orphaned items' message, got: %s", output) + } +} + +func TestRunCleanWithItems_MixedTypes_Preview(t *testing.T) { + setupCleanTestRepo(t) + + // Test preview mode with different cleanup types + items := []strategy.CleanupItem{ + {Type: strategy.CleanupTypeShadowBranch, ID: "entire/abc1234", Reason: "test"}, + {Type: strategy.CleanupTypeSessionState, ID: "session-123", Reason: "no checkpoints"}, + {Type: strategy.CleanupTypeCheckpoint, ID: "checkpoint-abc", Reason: "orphaned"}, + } + + var stdout bytes.Buffer + err := runCleanWithItems(&stdout, false, items) // preview mode + if err != nil { + t.Fatalf("runCleanWithItems() error = %v", err) + } + + output := stdout.String() + + // Should show all types + if !strings.Contains(output, "Shadow branches") { + t.Errorf("Expected 'Shadow branches' section, got: %s", output) + } + if !strings.Contains(output, "Session states") { + t.Errorf("Expected 'Session states' section, got: %s", output) + } + if !strings.Contains(output, "Checkpoint metadata") { + t.Errorf("Expected 'Checkpoint metadata' section, got: %s", output) + } + + // Should show total count + if !strings.Contains(output, "Found 3 orphaned items") { + t.Errorf("Expected 'Found 3 orphaned items', got: %s", output) + } +} diff --git a/cmd/entire/cli/root.go b/cmd/entire/cli/root.go index 69a3833b6d..fb33a7a131 100644 --- a/cmd/entire/cli/root.go +++ b/cmd/entire/cli/root.go @@ -52,7 +52,6 @@ func NewRootCmd() *cobra.Command { cmd.AddCommand(newStartCmd()) cmd.AddCommand(newVersionCmd()) cmd.AddCommand(newExplainCmd()) - cmd.AddCommand(newCleanCmd()) // Replace default help command with custom one that supports -t flag cmd.SetHelpCommand(commands.NewHelpCmd(cmd)) diff --git a/cmd/entire/cli/session.go b/cmd/entire/cli/session.go index 350f72595e..5b2fd8c444 100644 --- a/cmd/entire/cli/session.go +++ b/cmd/entire/cli/session.go @@ -47,6 +47,7 @@ func newSessionCmd() *cobra.Command { cmd.AddCommand(newSessionListCmd()) cmd.AddCommand(newSessionResumeCmd()) cmd.AddCommand(newSessionCurrentCmd()) + cmd.AddCommand(newCleanCmd()) return cmd } diff --git a/cmd/entire/cli/strategy/auto_commit.go b/cmd/entire/cli/strategy/auto_commit.go index e210d31a12..50779e1c48 100644 --- a/cmd/entire/cli/strategy/auto_commit.go +++ b/cmd/entire/cli/strategy/auto_commit.go @@ -989,6 +989,114 @@ func (s *AutoCommitStrategy) InitializeSession(sessionID string) error { return nil } +// ListOrphanedItems returns orphaned items created by the auto-commit strategy. +// For auto-commit, checkpoints are orphaned when no commit has an Entire-Checkpoint +// trailer referencing them (e.g., after rebasing or squashing). +func (s *AutoCommitStrategy) ListOrphanedItems() ([]CleanupItem, error) { + repo, err := OpenRepository() + if err != nil { + return nil, fmt.Errorf("failed to open repository: %w", err) + } + + // Get checkpoint store (lazily initialized) + cpStore, err := s.getCheckpointStore() + if err != nil { + return nil, fmt.Errorf("failed to get checkpoint store: %w", err) + } + + // Get all checkpoints from entire/sessions branch + checkpoints, err := cpStore.ListCommitted(context.Background()) + if err != nil { + return []CleanupItem{}, nil //nolint:nilerr // No checkpoints is not an error for cleanup + } + + if len(checkpoints) == 0 { + return []CleanupItem{}, nil + } + + // Filter to only auto-commit checkpoints (identified by strategy in metadata) + autoCommitCheckpoints := make(map[string]bool) + for _, cp := range checkpoints { + result, readErr := cpStore.ReadCommitted(context.Background(), cp.CheckpointID) + if readErr != nil || result == nil { + continue + } + // Only consider checkpoints created by this strategy + if result.Metadata.Strategy == StrategyNameAutoCommit || result.Metadata.Strategy == StrategyNameDual { + autoCommitCheckpoints[cp.CheckpointID] = true + } + } + + if len(autoCommitCheckpoints) == 0 { + return []CleanupItem{}, nil + } + + // Find checkpoint IDs referenced in commits + referencedCheckpoints := s.findReferencedCheckpoints(repo) + + // Find orphaned checkpoints + var items []CleanupItem + for checkpointID := range autoCommitCheckpoints { + if !referencedCheckpoints[checkpointID] { + items = append(items, CleanupItem{ + Type: CleanupTypeCheckpoint, + ID: checkpointID, + Reason: "no commit references this checkpoint", + }) + } + } + + return items, nil +} + +// findReferencedCheckpoints scans commits for Entire-Checkpoint trailers. +func (s *AutoCommitStrategy) findReferencedCheckpoints(repo *git.Repository) map[string]bool { + referenced := make(map[string]bool) + + refs, err := repo.References() + if err != nil { + return referenced + } + + visited := make(map[plumbing.Hash]bool) + + _ = refs.ForEach(func(ref *plumbing.Reference) error { //nolint:errcheck // Best effort + if !ref.Name().IsBranch() { + return nil + } + // Skip entire/* branches + branchName := strings.TrimPrefix(ref.Name().String(), "refs/heads/") + if strings.HasPrefix(branchName, "entire/") { + return nil + } + + iter, iterErr := repo.Log(&git.LogOptions{From: ref.Hash()}) + if iterErr != nil { + return nil //nolint:nilerr // Best effort + } + + count := 0 + _ = iter.ForEach(func(c *object.Commit) error { //nolint:errcheck // Best effort + count++ + if count > 1000 { + return errors.New("limit reached") + } + if visited[c.Hash] { + return nil + } + visited[c.Hash] = true + + if checkpointID, found := paths.ParseCheckpointTrailer(c.Message); found { + referenced[checkpointID] = true + } + return nil + }) + return nil + }) + + return referenced +} + //nolint:gochecknoinits // Standard pattern for strategy registration func init() { // Register auto-commit as the primary strategy name diff --git a/cmd/entire/cli/strategy/clean.go b/cmd/entire/cli/strategy/clean.go deleted file mode 100644 index d31f2799c5..0000000000 --- a/cmd/entire/cli/strategy/clean.go +++ /dev/null @@ -1,103 +0,0 @@ -package strategy - -import ( - "fmt" - "regexp" - "strings" - - "github.com/go-git/go-git/v5/plumbing" -) - -// shadowBranchPattern matches shadow branch names: entire/<7+ hex chars> -// The pattern requires at least 7 hex characters after "entire/" -var shadowBranchPattern = regexp.MustCompile(`^entire/[0-9a-fA-F]{7,}$`) - -// IsShadowBranch returns true if the branch name matches the shadow branch pattern. -// Shadow branches have the format "entire/" where the commit hash -// is at least 7 hex characters. The "entire/sessions" branch is NOT a shadow branch. -func IsShadowBranch(branchName string) bool { - // Explicitly exclude entire/sessions - if branchName == "entire/sessions" { - return false - } - return shadowBranchPattern.MatchString(branchName) -} - -// ListShadowBranches returns all shadow branches in the repository. -// Shadow branches match the pattern "entire/" (7+ hex chars). -// The "entire/sessions" branch is excluded as it stores permanent metadata. -// Returns an empty slice (not nil) if no shadow branches exist. -func ListShadowBranches() ([]string, error) { - repo, err := OpenRepository() - if err != nil { - return nil, fmt.Errorf("failed to open git repository: %w", err) - } - - refs, err := repo.References() - if err != nil { - return nil, fmt.Errorf("failed to get references: %w", err) - } - - var shadowBranches []string - - err = refs.ForEach(func(ref *plumbing.Reference) error { - // Only look at branch references - if !ref.Name().IsBranch() { - return nil - } - - // Extract branch name without refs/heads/ prefix - branchName := strings.TrimPrefix(ref.Name().String(), "refs/heads/") - - if IsShadowBranch(branchName) { - shadowBranches = append(shadowBranches, branchName) - } - return nil - }) - if err != nil { - return nil, fmt.Errorf("failed to iterate references: %w", err) - } - - // Ensure we return empty slice, not nil - if shadowBranches == nil { - shadowBranches = []string{} - } - - return shadowBranches, nil -} - -// DeleteShadowBranches deletes the specified branches from the repository. -// Returns two slices: successfully deleted branches and branches that failed to delete. -// Individual branch deletion failures do not stop the operation - all branches are attempted. -// Returns an error only if the repository cannot be opened. -func DeleteShadowBranches(branches []string) (deleted []string, failed []string, err error) { - if len(branches) == 0 { - return []string{}, []string{}, nil - } - - repo, err := OpenRepository() - if err != nil { - return nil, nil, fmt.Errorf("failed to open git repository: %w", err) - } - - for _, branch := range branches { - refName := plumbing.NewBranchReferenceName(branch) - - // Check if reference exists before trying to delete - ref, err := repo.Reference(refName, true) - if err != nil { - failed = append(failed, branch) - continue - } - - // Delete the reference - if err := repo.Storer.RemoveReference(ref.Name()); err != nil { - failed = append(failed, branch) - continue - } - - deleted = append(deleted, branch) - } - - return deleted, failed, nil -} diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go new file mode 100644 index 0000000000..610d05e020 --- /dev/null +++ b/cmd/entire/cli/strategy/cleanup.go @@ -0,0 +1,420 @@ +package strategy + +import ( + "context" + "fmt" + "regexp" + "strings" + + "entire.io/cli/cmd/entire/cli/checkpoint" + "entire.io/cli/cmd/entire/cli/paths" + "entire.io/cli/cmd/entire/cli/session" + + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" +) + +// CleanupType identifies the type of orphaned item. +type CleanupType string + +const ( + CleanupTypeShadowBranch CleanupType = "shadow-branch" + CleanupTypeSessionState CleanupType = "session-state" + CleanupTypeCheckpoint CleanupType = "checkpoint" +) + +// CleanupItem represents an orphaned item that can be cleaned up. +type CleanupItem struct { + Type CleanupType + ID string // Branch name, session ID, or checkpoint ID + Reason string // Why this item is considered orphaned +} + +// CleanupResult contains the results of a cleanup operation. +type CleanupResult struct { + ShadowBranches []string // Deleted shadow branches + SessionStates []string // Deleted session state files + Checkpoints []string // Deleted checkpoint metadata + FailedBranches []string // Shadow branches that failed to delete + FailedStates []string // Session states that failed to delete + FailedCheckpoints []string // Checkpoints that failed to delete +} + +// shadowBranchPattern matches shadow branch names: entire/<7+ hex chars> +// The pattern requires at least 7 hex characters after "entire/" +var shadowBranchPattern = regexp.MustCompile(`^entire/[0-9a-fA-F]{7,}$`) + +// IsShadowBranch returns true if the branch name matches the shadow branch pattern. +// Shadow branches have the format "entire/" where the commit hash +// is at least 7 hex characters. The "entire/sessions" branch is NOT a shadow branch. +func IsShadowBranch(branchName string) bool { + // Explicitly exclude entire/sessions + if branchName == "entire/sessions" { + return false + } + return shadowBranchPattern.MatchString(branchName) +} + +// ListShadowBranches returns all shadow branches in the repository. +// Shadow branches match the pattern "entire/" (7+ hex chars). +// The "entire/sessions" branch is excluded as it stores permanent metadata. +// Returns an empty slice (not nil) if no shadow branches exist. +func ListShadowBranches() ([]string, error) { + repo, err := OpenRepository() + if err != nil { + return nil, fmt.Errorf("failed to open git repository: %w", err) + } + + refs, err := repo.References() + if err != nil { + return nil, fmt.Errorf("failed to get references: %w", err) + } + + var shadowBranches []string + + err = refs.ForEach(func(ref *plumbing.Reference) error { + // Only look at branch references + if !ref.Name().IsBranch() { + return nil + } + + // Extract branch name without refs/heads/ prefix + branchName := strings.TrimPrefix(ref.Name().String(), "refs/heads/") + + if IsShadowBranch(branchName) { + shadowBranches = append(shadowBranches, branchName) + } + return nil + }) + if err != nil { + return nil, fmt.Errorf("failed to iterate references: %w", err) + } + + // Ensure we return empty slice, not nil + if shadowBranches == nil { + shadowBranches = []string{} + } + + return shadowBranches, nil +} + +// DeleteShadowBranches deletes the specified branches from the repository. +// Returns two slices: successfully deleted branches and branches that failed to delete. +// Individual branch deletion failures do not stop the operation - all branches are attempted. +// Returns an error only if the repository cannot be opened. +func DeleteShadowBranches(branches []string) (deleted []string, failed []string, err error) { + if len(branches) == 0 { + return []string{}, []string{}, nil + } + + repo, err := OpenRepository() + if err != nil { + return nil, nil, fmt.Errorf("failed to open git repository: %w", err) + } + + for _, branch := range branches { + refName := plumbing.NewBranchReferenceName(branch) + + // Check if reference exists before trying to delete + ref, err := repo.Reference(refName, true) + if err != nil { + failed = append(failed, branch) + continue + } + + // Delete the reference + if err := repo.Storer.RemoveReference(ref.Name()); err != nil { + failed = append(failed, branch) + continue + } + + deleted = append(deleted, branch) + } + + return deleted, failed, nil +} + +// ListOrphanedSessionStates returns session state files that are orphaned. +// A session state is orphaned if: +// - No checkpoints on entire/sessions reference this session ID +// - No shadow branch exists for the session's base commit +// +// This is strategy-agnostic as session states are shared by all strategies. +func ListOrphanedSessionStates() ([]CleanupItem, error) { + repo, err := OpenRepository() + if err != nil { + return nil, fmt.Errorf("failed to open git repository: %w", err) + } + + // Get all session states + store, err := session.NewStateStore() + if err != nil { + return nil, fmt.Errorf("failed to create state store: %w", err) + } + + states, err := store.List(context.Background()) + if err != nil { + return nil, fmt.Errorf("failed to list session states: %w", err) + } + + if len(states) == 0 { + return []CleanupItem{}, nil + } + + // Get all checkpoints to find which sessions have checkpoints + cpStore := checkpoint.NewGitStore(repo) + + sessionsWithCheckpoints := make(map[string]bool) + checkpoints, listErr := cpStore.ListCommitted(context.Background()) + if listErr == nil { + for _, cp := range checkpoints { + sessionsWithCheckpoints[cp.SessionID] = true + } + } + + // Get all shadow branches + shadowBranches, _ := ListShadowBranches() //nolint:errcheck // Best effort + shadowBranchSet := make(map[string]bool) + for _, branch := range shadowBranches { + // Extract commit hash from branch name (entire/) + if strings.HasPrefix(branch, "entire/") { + hash := strings.TrimPrefix(branch, "entire/") + shadowBranchSet[hash] = true + } + } + + var orphaned []CleanupItem + for _, state := range states { + // Check if session has checkpoints on entire/sessions + hasCheckpoints := sessionsWithCheckpoints[state.SessionID] + + // Check if shadow branch exists for base commit + hasShadowBranch := shadowBranchSet[state.BaseCommit] + + // Session is orphaned if it has no checkpoints AND no shadow branch + if !hasCheckpoints && !hasShadowBranch { + reason := "no checkpoints or shadow branch found" + orphaned = append(orphaned, CleanupItem{ + Type: CleanupTypeSessionState, + ID: state.SessionID, + Reason: reason, + }) + } + } + + return orphaned, nil +} + +// DeleteOrphanedSessionStates deletes the specified session state files. +func DeleteOrphanedSessionStates(sessionIDs []string) (deleted []string, failed []string, err error) { + if len(sessionIDs) == 0 { + return []string{}, []string{}, nil + } + + store, err := session.NewStateStore() + if err != nil { + return nil, nil, fmt.Errorf("failed to create state store: %w", err) + } + + for _, sessionID := range sessionIDs { + if err := store.Clear(context.Background(), sessionID); err != nil { + failed = append(failed, sessionID) + } else { + deleted = append(deleted, sessionID) + } + } + + return deleted, failed, nil +} + +// DeleteOrphanedCheckpoints removes checkpoint directories from the entire/sessions branch. +func DeleteOrphanedCheckpoints(checkpointIDs []string) (deleted []string, failed []string, err error) { + if len(checkpointIDs) == 0 { + return []string{}, []string{}, nil + } + + repo, err := OpenRepository() + if err != nil { + return nil, nil, fmt.Errorf("failed to open git repository: %w", err) + } + + // Get sessions branch + refName := plumbing.NewBranchReferenceName(paths.MetadataBranchName) + ref, err := repo.Reference(refName, true) + if err != nil { + return nil, nil, fmt.Errorf("sessions branch not found: %w", err) + } + + parentCommit, err := repo.CommitObject(ref.Hash()) + if err != nil { + return nil, nil, fmt.Errorf("failed to get commit: %w", err) + } + + baseTree, err := parentCommit.Tree() + if err != nil { + return nil, nil, fmt.Errorf("failed to get tree: %w", err) + } + + // Flatten tree to entries + entries := make(map[string]object.TreeEntry) + if err := checkpoint.FlattenTree(repo, baseTree, "", entries); err != nil { + return nil, nil, fmt.Errorf("failed to flatten tree: %w", err) + } + + // Remove entries for each checkpoint + checkpointSet := make(map[string]bool) + for _, id := range checkpointIDs { + checkpointSet[id] = true + } + + // Find and remove entries matching checkpoint paths + for path := range entries { + for checkpointID := range checkpointSet { + cpPath := paths.CheckpointPath(checkpointID) + if strings.HasPrefix(path, cpPath+"/") { + delete(entries, path) + } + } + } + + // Build new tree + newTreeHash, err := checkpoint.BuildTreeFromEntries(repo, entries) + if err != nil { + return nil, nil, fmt.Errorf("failed to build tree: %w", err) + } + + // Create commit + commit := &object.Commit{ + Author: object.Signature{ + Name: "Entire CLI", + Email: "cli@entire.io", + When: parentCommit.Author.When, + }, + Committer: object.Signature{ + Name: "Entire CLI", + Email: "cli@entire.io", + When: parentCommit.Committer.When, + }, + Message: fmt.Sprintf("Cleanup: removed %d orphaned checkpoints", len(checkpointIDs)), + TreeHash: newTreeHash, + ParentHashes: []plumbing.Hash{ref.Hash()}, + } + + obj := repo.Storer.NewEncodedObject() + if err := commit.Encode(obj); err != nil { + return nil, nil, fmt.Errorf("failed to encode commit: %w", err) + } + + commitHash, err := repo.Storer.SetEncodedObject(obj) + if err != nil { + return nil, nil, fmt.Errorf("failed to store commit: %w", err) + } + + // Update branch reference + newRef := plumbing.NewHashReference(refName, commitHash) + if err := repo.Storer.SetReference(newRef); err != nil { + return nil, nil, fmt.Errorf("failed to update branch: %w", err) + } + + // All checkpoints deleted successfully + 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() ([]CleanupItem, error) { + var items []CleanupItem + var firstErr error + + // Iterate over all registered strategies + for _, name := range List() { + // Skip legacy names to avoid duplicates + if IsLegacyStrategyName(name) { + continue + } + + strat, err := Get(name) + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + + // Check if strategy implements OrphanedItemsLister + if lister, ok := strat.(OrphanedItemsLister); ok { + stratItems, err := lister.ListOrphanedItems() + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + items = append(items, stratItems...) + } + } + + // Orphaned session states (strategy-agnostic) + states, err := ListOrphanedSessionStates() + if err != nil { + if firstErr == nil { + firstErr = err + } + } else { + items = append(items, states...) + } + + return items, firstErr +} + +// DeleteAllCleanupItems deletes all specified cleanup items. +func DeleteAllCleanupItems(items []CleanupItem) (*CleanupResult, error) { + result := &CleanupResult{} + + // Group items by type + var branches, states, checkpoints []string + for _, item := range items { + switch item.Type { + case CleanupTypeShadowBranch: + branches = append(branches, item.ID) + case CleanupTypeSessionState: + states = append(states, item.ID) + case CleanupTypeCheckpoint: + checkpoints = append(checkpoints, item.ID) + } + } + + // Delete shadow branches + if len(branches) > 0 { + deleted, failed, err := DeleteShadowBranches(branches) + if err != nil { + return result, err + } + result.ShadowBranches = deleted + result.FailedBranches = failed + } + + // Delete session states + if len(states) > 0 { + deleted, failed, err := DeleteOrphanedSessionStates(states) + if err != nil { + return result, err + } + result.SessionStates = deleted + result.FailedStates = failed + } + + // Delete checkpoints + if len(checkpoints) > 0 { + deleted, failed, err := DeleteOrphanedCheckpoints(checkpoints) + if err != nil { + return result, err + } + result.Checkpoints = deleted + result.FailedCheckpoints = failed + } + + return result, nil +} diff --git a/cmd/entire/cli/strategy/manual_commit.go b/cmd/entire/cli/strategy/manual_commit.go index ddac630dee..4eb31db7cc 100644 --- a/cmd/entire/cli/strategy/manual_commit.go +++ b/cmd/entire/cli/strategy/manual_commit.go @@ -152,6 +152,32 @@ func (s *ManualCommitStrategy) EnsureSetup() error { return nil } +// ListOrphanedItems returns orphaned items created by the manual-commit strategy. +// This includes: +// - Shadow branches that weren't auto-cleaned during commit condensation +// - Session state files with no corresponding checkpoints or shadow branches +func (s *ManualCommitStrategy) ListOrphanedItems() ([]CleanupItem, error) { + var items []CleanupItem + + // Shadow branches (should have been auto-cleaned after condensation) + branches, err := ListShadowBranches() + if err != nil { + return nil, err + } + for _, branch := range branches { + items = append(items, CleanupItem{ + Type: CleanupTypeShadowBranch, + ID: branch, + Reason: "shadow branch (should have been auto-cleaned)", + }) + } + + // Orphaned session states are detected by ListOrphanedSessionStates + // which is strategy-agnostic (checks both shadow branches and checkpoints) + + return items, nil +} + //nolint:gochecknoinits // Standard pattern for strategy registration func init() { // Register manual-commit as the primary strategy name diff --git a/cmd/entire/cli/strategy/strategy.go b/cmd/entire/cli/strategy/strategy.go index dcc432dda2..87ecc05c01 100644 --- a/cmd/entire/cli/strategy/strategy.go +++ b/cmd/entire/cli/strategy/strategy.go @@ -483,3 +483,15 @@ type SessionSource interface { // GetAdditionalSessions returns sessions not yet on entire/sessions branch. GetAdditionalSessions() ([]*Session, error) } + +// OrphanedItemsLister is an optional interface for strategies that can identify +// orphaned items (shadow branches, session states, checkpoints) that should be +// cleaned up. This is used by the "entire session cleanup" command. +// +// ListAllCleanupItems() automatically discovers all registered strategies, checks +// if they implement OrphanedItemsLister, and combines their orphaned items. +type OrphanedItemsLister interface { + // ListOrphanedItems returns items created by this strategy that are now orphaned. + // Each strategy defines what "orphaned" means for its own data structures. + ListOrphanedItems() ([]CleanupItem, error) +} From dc7259fe0828043f918992accce910b780922dab Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Mon, 5 Jan 2026 17:34:09 +0100 Subject: [PATCH 2/3] make sure we compare the same commit parts Entire-Checkpoint: 9878b713483d --- cmd/entire/cli/checkpoint/temporary.go | 10 +++++++--- cmd/entire/cli/strategy/auto_commit.go | 2 +- cmd/entire/cli/strategy/manual_commit_session.go | 6 ++++-- cmd/entire/cli/strategy/manual_commit_test.go | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/cmd/entire/cli/checkpoint/temporary.go b/cmd/entire/cli/checkpoint/temporary.go index 3d3cdd73b6..4481be198a 100644 --- a/cmd/entire/cli/checkpoint/temporary.go +++ b/cmd/entire/cli/checkpoint/temporary.go @@ -23,6 +23,10 @@ const ( // ShadowBranchPrefix is the prefix for shadow branches. ShadowBranchPrefix = "entire/" + // ShadowBranchHashLength is the number of hex characters used in shadow branch names. + // Shadow branches are named "entire/" using the first 7 characters of the commit hash. + ShadowBranchHashLength = 7 + // gitDir and entireDir are excluded from tree operations. gitDir = ".git" entireDir = ".entire" @@ -441,10 +445,10 @@ func (s *GitStore) DeleteShadowBranch(baseCommit string) error { } // ShadowBranchNameForCommit returns the shadow branch name for a base commit hash. -// Uses the first 7 characters of the commit hash. +// Uses the first ShadowBranchHashLength characters of the commit hash. func ShadowBranchNameForCommit(baseCommit string) string { - if len(baseCommit) >= 7 { - return ShadowBranchPrefix + baseCommit[:7] + if len(baseCommit) >= ShadowBranchHashLength { + return ShadowBranchPrefix + baseCommit[:ShadowBranchHashLength] } return ShadowBranchPrefix + baseCommit } diff --git a/cmd/entire/cli/strategy/auto_commit.go b/cmd/entire/cli/strategy/auto_commit.go index 50779e1c48..203dc42c0c 100644 --- a/cmd/entire/cli/strategy/auto_commit.go +++ b/cmd/entire/cli/strategy/auto_commit.go @@ -960,7 +960,7 @@ func (s *AutoCommitStrategy) InitializeSession(sessionID string) error { return fmt.Errorf("failed to get HEAD: %w", err) } - baseCommit := head.Hash().String()[:7] + baseCommit := head.Hash().String() // Check if session state already exists (e.g., session resuming) existing, err := LoadSessionState(sessionID) diff --git a/cmd/entire/cli/strategy/manual_commit_session.go b/cmd/entire/cli/strategy/manual_commit_session.go index 8fa03fc241..cf5a1da1bc 100644 --- a/cmd/entire/cli/strategy/manual_commit_session.go +++ b/cmd/entire/cli/strategy/manual_commit_session.go @@ -5,6 +5,8 @@ import ( "fmt" "time" + "entire.io/cli/cmd/entire/cli/checkpoint" + "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" ) @@ -232,8 +234,8 @@ func (s *ManualCommitStrategy) initializeSession(repo *git.Repository, sessionID // getShadowBranchNameForCommit returns the shadow branch name for the given base commit. func getShadowBranchNameForCommit(baseCommit string) string { - if len(baseCommit) >= 7 { - return shadowBranchPrefix + baseCommit[:7] + if len(baseCommit) >= checkpoint.ShadowBranchHashLength { + return shadowBranchPrefix + baseCommit[:checkpoint.ShadowBranchHashLength] } return shadowBranchPrefix + baseCommit } diff --git a/cmd/entire/cli/strategy/manual_commit_test.go b/cmd/entire/cli/strategy/manual_commit_test.go index 3a174b78c9..b2c440d748 100644 --- a/cmd/entire/cli/strategy/manual_commit_test.go +++ b/cmd/entire/cli/strategy/manual_commit_test.go @@ -1167,7 +1167,7 @@ func TestShadowStrategy_PrepareCommitMsg_ReusesLastCheckpointID(t *testing.T) { // (simulating state after first commit with condensation) state := &SessionState{ SessionID: "test-session", - BaseCommit: initialCommit.String()[:7], + BaseCommit: initialCommit.String(), WorktreePath: dir, StartedAt: time.Now(), CheckpointCount: 1, From 158e36f407151bb9f9b55209ab37fe7fd5a7336c Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Mon, 5 Jan 2026 17:35:30 +0100 Subject: [PATCH 3/3] review feedback - make sure we compare the right parts of commit hashes - make sure we don't delete fresh created session states - log what gets cleaned up Entire-Checkpoint: 9878b713483d --- cmd/entire/cli/checkpoint/checkpoint_test.go | 64 +++++++ cmd/entire/cli/checkpoint/committed.go | 7 + cmd/entire/cli/strategy/clean_test.go | 168 +++++++++++++++++++ cmd/entire/cli/strategy/cleanup.go | 101 ++++++++++- 4 files changed, 339 insertions(+), 1 deletion(-) diff --git a/cmd/entire/cli/checkpoint/checkpoint_test.go b/cmd/entire/cli/checkpoint/checkpoint_test.go index 92d4229b1e..4f849f3eb5 100644 --- a/cmd/entire/cli/checkpoint/checkpoint_test.go +++ b/cmd/entire/cli/checkpoint/checkpoint_test.go @@ -1,7 +1,12 @@ package checkpoint import ( + "os" + "path/filepath" "testing" + + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" ) func TestCheckpointType_Values(t *testing.T) { @@ -16,3 +21,62 @@ func TestCheckpointType_Values(t *testing.T) { t.Errorf("expected zero value of Type to be Temporary, got %d", defaultType) } } + +func TestCopyMetadataDir_SkipsSymlinks(t *testing.T) { + // Create a temp directory for the test + tempDir := t.TempDir() + + // Initialize a git repository + repo, err := git.PlainInit(tempDir, false) + if err != nil { + t.Fatalf("failed to init git repo: %v", err) + } + + // Create metadata directory structure + metadataDir := filepath.Join(tempDir, "metadata") + if err := os.MkdirAll(metadataDir, 0755); err != nil { + t.Fatalf("failed to create metadata dir: %v", err) + } + + // Create a regular file that should be included + regularFile := filepath.Join(metadataDir, "regular.txt") + if err := os.WriteFile(regularFile, []byte("regular content"), 0644); err != nil { + t.Fatalf("failed to create regular file: %v", err) + } + + // Create a sensitive file outside the metadata directory + sensitiveFile := filepath.Join(tempDir, "sensitive.txt") + if err := os.WriteFile(sensitiveFile, []byte("SECRET DATA"), 0644); err != nil { + t.Fatalf("failed to create sensitive file: %v", err) + } + + // Create a symlink inside metadata directory pointing to the sensitive file + symlinkPath := filepath.Join(metadataDir, "sneaky-link") + if err := os.Symlink(sensitiveFile, symlinkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // Create GitStore and call copyMetadataDir + store := NewGitStore(repo) + entries := make(map[string]object.TreeEntry) + + err = store.copyMetadataDir(metadataDir, "checkpoint/", entries) + if err != nil { + t.Fatalf("copyMetadataDir failed: %v", err) + } + + // Verify regular file was included + if _, ok := entries["checkpoint/regular.txt"]; !ok { + t.Error("regular.txt should be included in entries") + } + + // Verify symlink was NOT included (security fix) + if _, ok := entries["checkpoint/sneaky-link"]; ok { + t.Error("symlink should NOT be included in entries - this would allow reading files outside the metadata directory") + } + + // Verify the correct number of entries + if len(entries) != 1 { + t.Errorf("expected 1 entry, got %d", len(entries)) + } +} diff --git a/cmd/entire/cli/checkpoint/committed.go b/cmd/entire/cli/checkpoint/committed.go index d1de1c2a94..278c762fae 100644 --- a/cmd/entire/cli/checkpoint/committed.go +++ b/cmd/entire/cli/checkpoint/committed.go @@ -584,6 +584,13 @@ func (s *GitStore) copyMetadataDir(metadataDir, basePath string, entries map[str return nil } + // Skip symlinks to prevent reading files outside the metadata directory. + // A symlink could point to sensitive files (e.g., /etc/passwd) which would + // then be captured in the checkpoint and stored in git history. + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + // Get relative path within metadata dir relPath, err := filepath.Rel(metadataDir, path) if err != nil { diff --git a/cmd/entire/cli/strategy/clean_test.go b/cmd/entire/cli/strategy/clean_test.go index 0abe7c605b..4d3ae126bf 100644 --- a/cmd/entire/cli/strategy/clean_test.go +++ b/cmd/entire/cli/strategy/clean_test.go @@ -2,6 +2,7 @@ package strategy import ( "testing" + "time" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" @@ -283,3 +284,170 @@ func TestDeleteShadowBranches_Empty(t *testing.T) { t.Errorf("DeleteShadowBranches([]) = (%v, %v), want ([], [])", deleted, failed) } } + +// TestListOrphanedSessionStates_RecentSessionNotOrphaned tests that recently started +// sessions are NOT marked as orphaned, even if they have no checkpoints yet. +// +// P1 Bug: A session that just started (via InitializeSession) but hasn't created +// its first checkpoint yet would be incorrectly marked as orphaned because it has: +// - A session state file +// - No checkpoints on entire/sessions +// - No shadow branch (if using auto-commit strategy, or before first checkpoint) +// +// This test should FAIL with the current implementation, demonstrating the bug. +func TestListOrphanedSessionStates_RecentSessionNotOrphaned(t *testing.T) { + // Setup: create a temp git repo + dir := t.TempDir() + repo, err := git.PlainInit(dir, false) + if err != nil { + t.Fatalf("failed to init git repo: %v", err) + } + + t.Chdir(dir) + + // Create initial commit + emptyTreeHash := plumbing.NewHash("4b825dc642cb6eb9a060e54bf8d69288fbee4904") + commitHash, err := createCommit(repo, emptyTreeHash, plumbing.ZeroHash, "initial commit", "test", "test@test.com") + if err != nil { + t.Fatalf("failed to create initial commit: %v", err) + } + + // Create HEAD reference + headRef := plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.NewBranchReferenceName("master")) + if err := repo.Storer.SetReference(headRef); err != nil { + t.Fatalf("failed to set HEAD: %v", err) + } + masterRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName("master"), commitHash) + if err := repo.Storer.SetReference(masterRef); err != nil { + t.Fatalf("failed to set master: %v", err) + } + + // Create a session state file that was JUST started (simulating InitializeSession) + // This session has no checkpoints and no shadow branch yet + state := &SessionState{ + SessionID: "recent-session-123", + BaseCommit: commitHash.String(), // Full 40-char hash + StartedAt: time.Now(), // Just started! + CheckpointCount: 0, // No checkpoints yet + } + if err := SaveSessionState(state); err != nil { + t.Fatalf("SaveSessionState() error = %v", err) + } + + // List orphaned session states + orphaned, err := ListOrphanedSessionStates() + if err != nil { + t.Fatalf("ListOrphanedSessionStates() error = %v", err) + } + + // The recently started session should NOT be marked as orphaned + // because it's actively being used (StartedAt is recent) + for _, item := range orphaned { + if item.ID == "recent-session-123" { + t.Errorf("ListOrphanedSessionStates() incorrectly marked recent session as orphaned.\n"+ + "Session was started %v ago, which is too recent to be considered orphaned.\n"+ + "Expected: session to be protected from cleanup during active use.\n"+ + "Got: session marked as orphaned with reason: %q", + time.Since(state.StartedAt), item.Reason) + } + } +} + +// TestListOrphanedSessionStates_HashLengthMismatch tests that session states are correctly +// matched against shadow branches even when hash lengths differ. +// +// P1 Bug: Shadow branches use 7-char hashes (e.g., "entire/abc1234") but session states +// store the full 40-char BaseCommit hash. The current comparison at line 192 does: +// +// shadowBranchSet[state.BaseCommit] +// +// where shadowBranchSet has 7-char keys but state.BaseCommit is 40 chars. +// This comparison always fails, causing valid sessions to be marked as orphaned. +// +// This test should FAIL with the current implementation, demonstrating the bug. +func TestListOrphanedSessionStates_HashLengthMismatch(t *testing.T) { + // Setup: create a temp git repo + dir := t.TempDir() + repo, err := git.PlainInit(dir, false) + if err != nil { + t.Fatalf("failed to init git repo: %v", err) + } + + t.Chdir(dir) + + // Create initial commit + emptyTreeHash := plumbing.NewHash("4b825dc642cb6eb9a060e54bf8d69288fbee4904") + commitHash, err := createCommit(repo, emptyTreeHash, plumbing.ZeroHash, "initial commit", "test", "test@test.com") + if err != nil { + t.Fatalf("failed to create initial commit: %v", err) + } + + // Create HEAD reference + headRef := plumbing.NewSymbolicReference(plumbing.HEAD, plumbing.NewBranchReferenceName("master")) + if err := repo.Storer.SetReference(headRef); err != nil { + t.Fatalf("failed to set HEAD: %v", err) + } + masterRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName("master"), commitHash) + if err := repo.Storer.SetReference(masterRef); err != nil { + t.Fatalf("failed to set master: %v", err) + } + + // Create a shadow branch using the 7-char hash (matching real behavior) + // Real code: shadowBranch := "entire/" + baseHead[:7] + shortHash := commitHash.String()[:7] + shadowBranchName := "entire/" + shortHash + shadowRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName(shadowBranchName), commitHash) + if err := repo.Storer.SetReference(shadowRef); err != nil { + t.Fatalf("failed to create shadow branch: %v", err) + } + + // Create a session state with the FULL 40-char hash (matching real behavior) + // Real code: state.BaseCommit = head.Hash().String() + fullHash := commitHash.String() + state := &SessionState{ + SessionID: "session-with-shadow-branch", + BaseCommit: fullHash, // Full 40-char hash! + StartedAt: time.Now().Add(-1 * time.Hour), + CheckpointCount: 1, + } + if err := SaveSessionState(state); err != nil { + t.Fatalf("SaveSessionState() error = %v", err) + } + + // Verify the shadow branch exists and uses short hash + shadowBranches, err := ListShadowBranches() + if err != nil { + t.Fatalf("ListShadowBranches() error = %v", err) + } + if len(shadowBranches) != 1 || shadowBranches[0] != shadowBranchName { + t.Fatalf("Expected shadow branch %q, got %v", shadowBranchName, shadowBranches) + } + + // Verify the hash length mismatch exists + t.Logf("Shadow branch hash (7 chars): %q", shortHash) + t.Logf("Session BaseCommit (40 chars): %q", fullHash) + t.Logf("Are they equal? %v (they should match by prefix)", shortHash == fullHash) + + // List orphaned session states + orphaned, err := ListOrphanedSessionStates() + if err != nil { + t.Fatalf("ListOrphanedSessionStates() error = %v", err) + } + + // The session should NOT be marked as orphaned because it HAS a shadow branch! + // The shadow branch exists (entire/<7-char-hash>), but the current code compares + // the 7-char hash against the 40-char BaseCommit, which always fails. + for _, item := range orphaned { + if item.ID == "session-with-shadow-branch" { + t.Errorf("ListOrphanedSessionStates() incorrectly marked session as orphaned due to hash length mismatch.\n"+ + "Shadow branch exists: %q (uses 7-char hash: %q)\n"+ + "Session BaseCommit: %q (40-char hash)\n"+ + "The comparison shadowBranchSet[state.BaseCommit] fails because:\n"+ + " - shadowBranchSet contains key %q (7 chars)\n"+ + " - state.BaseCommit is %q (40 chars)\n"+ + "Expected: session to be recognized as having a shadow branch.\n"+ + "Got: session marked as orphaned with reason: %q", + shadowBranchName, shortHash, fullHash, shortHash, fullHash, item.Reason) + } + } +} diff --git a/cmd/entire/cli/strategy/cleanup.go b/cmd/entire/cli/strategy/cleanup.go index 610d05e020..43fe1d6e2a 100644 --- a/cmd/entire/cli/strategy/cleanup.go +++ b/cmd/entire/cli/strategy/cleanup.go @@ -3,10 +3,13 @@ package strategy import ( "context" "fmt" + "log/slog" "regexp" "strings" + "time" "entire.io/cli/cmd/entire/cli/checkpoint" + "entire.io/cli/cmd/entire/cli/logging" "entire.io/cli/cmd/entire/cli/paths" "entire.io/cli/cmd/entire/cli/session" @@ -14,6 +17,17 @@ import ( "github.com/go-git/go-git/v5/plumbing/object" ) +const ( + // sessionGracePeriod is the minimum age a session must have before it can be + // considered orphaned. This protects active sessions that haven't created + // their first checkpoint yet. + sessionGracePeriod = 10 * time.Minute + + // shadowBranchHashLength is the number of hex characters used in shadow branch names. + // Shadow branches are named "entire/" using a 7-char prefix of the commit hash. + shadowBranchHashLength = 7 +) + // CleanupType identifies the type of orphaned item. type CleanupType string @@ -184,12 +198,24 @@ func ListOrphanedSessionStates() ([]CleanupItem, error) { } var orphaned []CleanupItem + now := time.Now() + for _, state := range states { + // Skip sessions that started recently - they may be actively in use + // but haven't created their first checkpoint yet + if now.Sub(state.StartedAt) < sessionGracePeriod { + continue + } + // Check if session has checkpoints on entire/sessions hasCheckpoints := sessionsWithCheckpoints[state.SessionID] // Check if shadow branch exists for base commit - hasShadowBranch := shadowBranchSet[state.BaseCommit] + // Shadow branches use 7-char hash prefixes, so we need to match by prefix + hasShadowBranch := false + if len(state.BaseCommit) >= shadowBranchHashLength { + hasShadowBranch = shadowBranchSet[state.BaseCommit[:shadowBranchHashLength]] + } // Session is orphaned if it has no checkpoints AND no shadow branch if !hasCheckpoints && !hasShadowBranch { @@ -370,8 +396,16 @@ func ListAllCleanupItems() ([]CleanupItem, error) { } // DeleteAllCleanupItems deletes all specified cleanup items. +// Logs each deletion for audit purposes. func DeleteAllCleanupItems(items []CleanupItem) (*CleanupResult, error) { result := &CleanupResult{} + logCtx := logging.WithComponent(context.Background(), "cleanup") + + // Build ID-to-Reason map for logging after deletion + reasonMap := make(map[string]string) + for _, item := range items { + reasonMap[item.ID] = item.Reason + } // Group items by type var branches, states, checkpoints []string @@ -394,6 +428,23 @@ func DeleteAllCleanupItems(items []CleanupItem) (*CleanupResult, error) { } result.ShadowBranches = deleted result.FailedBranches = failed + + // Log deleted branches + for _, id := range deleted { + logging.Info(logCtx, "deleted orphaned shadow branch", + slog.String("type", string(CleanupTypeShadowBranch)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } + // Log failed branches + for _, id := range failed { + logging.Warn(logCtx, "failed to delete orphaned shadow branch", + slog.String("type", string(CleanupTypeShadowBranch)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } } // Delete session states @@ -404,6 +455,23 @@ func DeleteAllCleanupItems(items []CleanupItem) (*CleanupResult, error) { } result.SessionStates = deleted result.FailedStates = failed + + // Log deleted session states + for _, id := range deleted { + logging.Info(logCtx, "deleted orphaned session state", + slog.String("type", string(CleanupTypeSessionState)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } + // Log failed session states + for _, id := range failed { + logging.Warn(logCtx, "failed to delete orphaned session state", + slog.String("type", string(CleanupTypeSessionState)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } } // Delete checkpoints @@ -414,6 +482,37 @@ func DeleteAllCleanupItems(items []CleanupItem) (*CleanupResult, error) { } result.Checkpoints = deleted result.FailedCheckpoints = failed + + // Log deleted checkpoints + for _, id := range deleted { + logging.Info(logCtx, "deleted orphaned checkpoint", + slog.String("type", string(CleanupTypeCheckpoint)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } + // Log failed checkpoints + for _, id := range failed { + logging.Warn(logCtx, "failed to delete orphaned checkpoint", + slog.String("type", string(CleanupTypeCheckpoint)), + slog.String("id", id), + slog.String("reason", reasonMap[id]), + ) + } + } + + // Log summary + totalDeleted := len(result.ShadowBranches) + len(result.SessionStates) + len(result.Checkpoints) + totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + if totalDeleted > 0 || totalFailed > 0 { + logging.Info(logCtx, "cleanup completed", + slog.Int("deleted_branches", len(result.ShadowBranches)), + slog.Int("deleted_session_states", len(result.SessionStates)), + slog.Int("deleted_checkpoints", len(result.Checkpoints)), + slog.Int("failed_branches", len(result.FailedBranches)), + slog.Int("failed_session_states", len(result.FailedStates)), + slog.Int("failed_checkpoints", len(result.FailedCheckpoints)), + ) } return result, nil