fix: cli command output consistency

cmd/entire/cli/clean.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/charmbracelet/huh"
 	"github.com/entireio/cli/cmd/entire/cli/logging"
 	"github.com/entireio/cli/cmd/entire/cli/paths"
 	"github.com/entireio/cli/cmd/entire/cli/strategy"
@@ -40,21 +41,24 @@ This command finds and removes orphaned data from any strategy:
     Cached transcripts and other temporary data. Safe to delete when no
     active sessions are using them.
 
-Default: shows a preview of items that would be deleted.
-With --force, actually deletes the orphaned items.
+Default: shows a preview and asks for confirmation before deleting.
+With --force, deletes without prompting.
 
 The entire/checkpoints/v1 branch itself is never deleted.`,
 		RunE: func(cmd *cobra.Command, _ []string) error {
-			return runClean(cmd.Context(), cmd.OutOrStdout(), forceFlag)
+			return runClean(cmd.Context(), cmd, forceFlag)
 		},
 	}
 
-	cmd.Flags().BoolVarP(&forceFlag, "force", "f", false, "Actually delete items (default: dry run)")
+	cmd.Flags().BoolVarP(&forceFlag, "force", "f", false, "Delete without prompting for confirmation")
 
 	return cmd
 }
 
-func runClean(ctx context.Context, w io.Writer, force bool) error {
+func runClean(ctx context.Context, cmd *cobra.Command, force bool) error {
+	w := cmd.OutOrStdout()
+	errW := cmd.ErrOrStderr()
+
 	// Initialize logging so structured logs go to .entire/logs/ instead of stderr.
 	// Error is non-fatal: if logging init fails, logs go to stderr (acceptable fallback).
 	logging.SetLogLevelGetter(GetLogLevel)
@@ -72,10 +76,47 @@ func runClean(ctx context.Context, w io.Writer, force bool) error {
 	tempFiles, err := listTempFiles(ctx)
 	if err != nil {
 		// Non-fatal: continue with other cleanup items
-		fmt.Fprintf(w, "Warning: failed to list temp files: %v\n", err)
+		fmt.Fprintf(errW, "Warning: failed to list temp files: %v\n", err)
+	}
+
+	// Force mode: skip preview and confirmation
+	if force {
+		return runCleanWithItems(ctx, w, true, items, tempFiles)
+	}
+
+	// Show preview
+	if err := runCleanWithItems(ctx, w, false, items, tempFiles); err != nil {
+		return err
 	}
 
-	return runCleanWithItems(ctx, w, force, items, tempFiles)
+	// If nothing to clean, we're done (preview already printed the message)
+	totalItems := len(items) + len(tempFiles)
+	if totalItems == 0 {
+		return nil
+	}
+
+	// Interactive confirmation
+	var confirmed bool
+	form := NewAccessibleForm(
+		huh.NewGroup(
+			huh.NewConfirm().
+				Title("Delete these items?").
+				Affirmative("Yes, delete").
+				Negative("Cancel").
+				Value(&confirmed),
+		),
+	)
+
+	if err := form.Run(); err != nil {
+		return handleFormCancellation(w, "Clean", err)
+	}
+
+	if !confirmed {
+		fmt.Fprintln(w, "Clean cancelled.")
+		return nil
+	}
+
+	return runCleanWithItems(ctx, w, true, items, tempFiles)
 }
 
 // listTempFiles returns files in .entire/tmp/ that are safe to delete,
@@ -172,7 +213,7 @@ func runCleanWithItems(ctx context.Context, w io.Writer, force bool, items []str
 	// Preview mode (default)
 	if !force {
 		totalItems := len(items) + len(tempFiles)
-		fmt.Fprintf(w, "Found %d items to clean:\n\n", totalItems)
+		fmt.Fprintf(w, "Found %d %s to clean:\n\n", totalItems, itemWord(totalItems))
 
 		if len(branches) > 0 {
 			fmt.Fprintf(w, "Shadow branches (%d):\n", len(branches))
@@ -206,7 +247,7 @@ func runCleanWithItems(ctx context.Context, w io.Writer, force bool, items []str
 			fmt.Fprintln(w)
 		}
 
-		fmt.Fprintln(w, "Run with --force to delete these items.")
+		fmt.Fprintln(w, "Use --force to skip this prompt.")
 		return nil
 	}
 
@@ -224,70 +265,78 @@ func runCleanWithItems(ctx context.Context, w io.Writer, force bool, items []str
 	totalFailed := len(result.FailedBranches) + len(result.FailedStates) + len(result.FailedCheckpoints) + len(failedTempFiles)
 
 	if totalDeleted > 0 {
-		fmt.Fprintf(w, "Deleted %d items:\n", totalDeleted)
+		fmt.Fprintf(w, "✓ Deleted %d %s:\n", totalDeleted, itemWord(totalDeleted))
 
 		if len(result.ShadowBranches) > 0 {
-			fmt.Fprintf(w, "\n  Shadow branches (%d):\n", len(result.ShadowBranches))
+			fmt.Fprintf(w, "\nShadow branches (%d):\n", len(result.ShadowBranches))
 			for _, branch := range result.ShadowBranches {
-				fmt.Fprintf(w, "    %s\n", branch)
+				fmt.Fprintf(w, "  %s\n", branch)
 			}
 		}
 
 		if len(result.SessionStates) > 0 {
-			fmt.Fprintf(w, "\n  Session states (%d):\n", len(result.SessionStates))
+			fmt.Fprintf(w, "\nSession states (%d):\n", len(result.SessionStates))
 			for _, state := range result.SessionStates {
-				fmt.Fprintf(w, "    %s\n", state)
+				fmt.Fprintf(w, "  %s\n", state)
 			}
 		}
 
 		if len(result.Checkpoints) > 0 {
-			fmt.Fprintf(w, "\n  Checkpoints (%d):\n", len(result.Checkpoints))
+			fmt.Fprintf(w, "\nCheckpoints (%d):\n", len(result.Checkpoints))
 			for _, cp := range result.Checkpoints {
-				fmt.Fprintf(w, "    %s\n", cp)
+				fmt.Fprintf(w, "  %s\n", cp)
 			}
 		}
 
 		if len(deletedTempFiles) > 0 {
-			fmt.Fprintf(w, "\n  Temp files (%d):\n", len(deletedTempFiles))
+			fmt.Fprintf(w, "\nTemp files (%d):\n", len(deletedTempFiles))
 			for _, file := range deletedTempFiles {
-				fmt.Fprintf(w, "    %s\n", file)
+				fmt.Fprintf(w, "  %s\n", file)
 			}
 		}
 	}
 
 	if totalFailed > 0 {
-		fmt.Fprintf(w, "\nFailed to delete %d items:\n", totalFailed)
+		fmt.Fprintf(w, "\nFailed to delete %d %s:\n", totalFailed, itemWord(totalFailed))
 
 		if len(result.FailedBranches) > 0 {
-			fmt.Fprintf(w, "\n  Shadow branches:\n")
+			fmt.Fprintf(w, "\nShadow branches:\n")
 			for _, branch := range result.FailedBranches {
-				fmt.Fprintf(w, "    %s\n", branch)
+				fmt.Fprintf(w, "  %s\n", branch)
 			}
 		}
 
 		if len(result.FailedStates) > 0 {
-			fmt.Fprintf(w, "\n  Session states:\n")
+			fmt.Fprintf(w, "\nSession states:\n")
 			for _, state := range result.FailedStates {
-				fmt.Fprintf(w, "    %s\n", state)
+				fmt.Fprintf(w, "  %s\n", state)
 			}
 		}
 
 		if len(result.FailedCheckpoints) > 0 {
-			fmt.Fprintf(w, "\n  Checkpoints:\n")
+			fmt.Fprintf(w, "\nCheckpoints:\n")
 			for _, cp := range result.FailedCheckpoints {
-				fmt.Fprintf(w, "    %s\n", cp)
+				fmt.Fprintf(w, "  %s\n", cp)
 			}
 		}
 
 		if len(failedTempFiles) > 0 {
-			fmt.Fprintf(w, "\n  Temp files:\n")
+			fmt.Fprintf(w, "\nTemp files:\n")
 			for _, fe := range failedTempFiles {
-				fmt.Fprintf(w, "    %s: %v\n", fe.File, fe.Err)
+				fmt.Fprintf(w, "  %s: %v\n", fe.File, fe.Err)
 			}
 		}
 
-		return fmt.Errorf("failed to delete %d items", totalFailed)
+		return fmt.Errorf("failed to delete %d %s", totalFailed, itemWord(totalFailed))
 	}
 
 	return nil
 }
+
+// itemWord returns "item" or "items" based on count.
+func itemWord(n int) string {
+	if n == 1 {
+		return "item"
+	}
+	return "items"
+}

cmd/entire/cli/clean_test.go

@@ -12,8 +12,19 @@ import (
 	"github.com/go-git/go-git/v6"
 	"github.com/go-git/go-git/v6/plumbing"
 	"github.com/go-git/go-git/v6/plumbing/object"
+	"github.com/spf13/cobra"
 )
 
+// newTestCleanCmd creates a cobra.Command with captured stdout for testing runClean.
+func newTestCleanCmd(t *testing.T) (*cobra.Command, *bytes.Buffer) {
+	t.Helper()
+	cmd := &cobra.Command{}
+	cmd.SetContext(context.Background())
+	var stdout bytes.Buffer
+	cmd.SetOut(&stdout)
+	return cmd, &stdout
+}
+
 func setupCleanTestRepo(t *testing.T) (*git.Repository, plumbing.Hash) {
 	t.Helper()
 
@@ -70,9 +81,9 @@ func TestRunClean_NoOrphanedItems(t *testing.T) {
 	setupCleanTestRepo(t)
 
 	var stdout bytes.Buffer
-	err := runClean(context.Background(), &stdout, false)
+	err := runCleanWithItems(context.Background(), &stdout, false, []strategy.CleanupItem{}, nil)
 	if err != nil {
-		t.Fatalf("runClean() error = %v", err)
+		t.Fatalf("runCleanWithItems() error = %v", err)
 	}
 
 	output := stdout.String()
@@ -99,17 +110,24 @@ func TestRunClean_PreviewMode(t *testing.T) {
 		t.Fatalf("failed to create %s: %v", paths.MetadataBranchName, err)
 	}
 
+	// Use runClean with force=true would need TTY for confirmation,
+	// so test preview output via runCleanWithItems directly.
+	items, err := strategy.ListAllCleanupItems(context.Background())
+	if err != nil {
+		t.Fatalf("ListAllCleanupItems() error = %v", err)
+	}
+
 	var stdout bytes.Buffer
-	err := runClean(context.Background(), &stdout, false) // force=false
+	err = runCleanWithItems(context.Background(), &stdout, false, items, nil)
 	if err != nil {
-		t.Fatalf("runClean() error = %v", err)
+		t.Fatalf("runCleanWithItems() error = %v", err)
 	}
 
 	output := stdout.String()
 
 	// Should show preview header
-	if !strings.Contains(output, "items to clean") {
-		t.Errorf("Expected 'items to clean' in output, got: %s", output)
+	if !strings.Contains(output, "to clean") {
+		t.Errorf("Expected 'to clean' in output, got: %s", output)
 	}
 
 	// Should list the shadow branches
@@ -151,17 +169,17 @@ func TestRunClean_ForceMode(t *testing.T) {
 		}
 	}
 
-	var stdout bytes.Buffer
-	err := runClean(context.Background(), &stdout, true) // force=true
+	cmd, stdout := newTestCleanCmd(t)
+	err := runClean(cmd.Context(), cmd, true) // force=true skips confirmation
 	if err != nil {
 		t.Fatalf("runClean() error = %v", err)
 	}
 
 	output := stdout.String()
 
-	// Should show deletion confirmation
-	if !strings.Contains(output, "Deleted") {
-		t.Errorf("Expected 'Deleted' in output, got: %s", output)
+	// Should show deletion confirmation with ✓ prefix
+	if !strings.Contains(output, "✓ Deleted") {
+		t.Errorf("Expected '✓ Deleted' in output, got: %s", output)
 	}
 
 	// Branches should be deleted
@@ -187,8 +205,8 @@ func TestRunClean_SessionsBranchPreserved(t *testing.T) {
 		t.Fatalf("failed to create entire/checkpoints/v1: %v", err)
 	}
 
-	var stdout bytes.Buffer
-	err := runClean(context.Background(), &stdout, true) // force=true
+	cmd, _ := newTestCleanCmd(t)
+	err := runClean(cmd.Context(), cmd, true) // force=true
 	if err != nil {
 		t.Fatalf("runClean() error = %v", err)
 	}
@@ -211,8 +229,8 @@ func TestRunClean_NotGitRepository(t *testing.T) {
 	t.Chdir(dir)
 	paths.ClearWorktreeRootCache()
 
-	var stdout bytes.Buffer
-	err := runClean(context.Background(), &stdout, false)
+	cmd, _ := newTestCleanCmd(t)
+	err := runClean(cmd.Context(), cmd, true) // force=true to skip TTY prompt
 
 	// Should return error for non-git directory
 	if err == nil {
@@ -243,10 +261,16 @@ func TestRunClean_Subdirectory(t *testing.T) {
 	t.Chdir(subDir)
 	paths.ClearWorktreeRootCache()
 
+	// Use ListAllCleanupItems + runCleanWithItems to test preview without TTY
+	items, err := strategy.ListAllCleanupItems(context.Background())
+	if err != nil {
+		t.Fatalf("ListAllCleanupItems() from subdirectory error = %v", err)
+	}
+
 	var stdout bytes.Buffer
-	err = runClean(context.Background(), &stdout, false)
+	err = runCleanWithItems(context.Background(), &stdout, false, items, nil)
 	if err != nil {
-		t.Fatalf("runClean() from subdirectory error = %v", err)
+		t.Fatalf("runCleanWithItems() from subdirectory error = %v", err)
 	}
 
 	output := stdout.String()
@@ -282,20 +306,24 @@ func TestRunCleanWithItems_PartialFailure(t *testing.T) {
 		t.Fatal("runCleanWithItems() should return error when items fail to delete")
 	}
 
-	// Error message should indicate the failure
-	if !strings.Contains(err.Error(), "failed to delete") {
-		t.Errorf("Error should mention 'failed to delete', got: %v", err)
+	// Error message should indicate the failure with correct grammar
+	if !strings.Contains(err.Error(), "failed to delete 1 item") {
+		t.Errorf("Error should mention 'failed to delete 1 item', got: %v", err)
+	}
+	// Verify singular (not "1 items")
+	if strings.Contains(err.Error(), "1 items") {
+		t.Errorf("Error should use singular 'item' for count 1, got: %v", err)
 	}
 
-	// Output should show the successful deletion
+	// Output should show the successful deletion with ✓ and singular grammar
 	output := stdout.String()
-	if !strings.Contains(output, "Deleted 1 items") {
-		t.Errorf("Output should show successful deletion, got: %s", output)
+	if !strings.Contains(output, "✓ Deleted 1 item:") {
+		t.Errorf("Output should show '✓ Deleted 1 item:', got: %s", output)
 	}
 
-	// Output should also show the failures
-	if !strings.Contains(output, "Failed to delete 1 items") {
-		t.Errorf("Output should show failures, got: %s", output)
+	// Output should also show the failure with singular grammar
+	if !strings.Contains(output, "Failed to delete 1 item:") {
+		t.Errorf("Output should show 'Failed to delete 1 item:', got: %s", output)
 	}
 }
 
@@ -318,20 +346,20 @@ func TestRunCleanWithItems_AllFailures(t *testing.T) {
 		t.Fatal("runCleanWithItems() should return error when items fail to delete")
 	}
 
-	// Error message should indicate 2 failures
+	// Error message should indicate 2 failures with plural grammar
 	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
+	// Output should NOT show any successful deletions (no ✓ Deleted line)
 	output := stdout.String()
-	if strings.Contains(output, "Deleted") {
+	if strings.Contains(output, "✓ Deleted") {
 		t.Errorf("Output should not show successful deletions, got: %s", output)
 	}
 
-	// Output should show the failures
-	if !strings.Contains(output, "Failed to delete 2 items") {
-		t.Errorf("Output should show failures, got: %s", output)
+	// Output should show the failures with plural grammar
+	if !strings.Contains(output, "Failed to delete 2 items:") {
+		t.Errorf("Output should show 'Failed to delete 2 items:', got: %s", output)
 	}
 }