feat(session): add 'ocr session rm' to delete a saved session

cmd/opencodereview/cli_reference_compare_docs_test.go

@@ -10,11 +10,6 @@ import (
 	"testing"
 )
 
-// TestCLIReferenceDocumentsSessionCompare pins that every locale of the CLI
-// reference documents `ocr session compare`. The four files are hand-synced
-// (see PR #920), so the usual failure is a new command landing in `en` only.
-// ponytail: substring checks, not a markdown parse - the whole point is to
-// catch a missing file, and a parser would not catch it any better.
 func TestCLIReferenceDocumentsSessionCompare(t *testing.T) {
 	for _, locale := range []string{"en", "zh", "ja", "ru"} {
 		t.Run(locale, func(t *testing.T) {
@@ -37,14 +32,6 @@ func TestCLIReferenceDocumentsSessionCompare(t *testing.T) {
 	}
 }
 
-// TestCLIReferenceDocumentsSessionExport is the sibling guard for
-// `ocr session export`. It pins the command surface — the summary row, the
-// reference section, the default no-id form and --output — across every
-// locale, including `ko`, which the compare pin above predates.
-//
-// The note that the exported page embeds reviewed source is currently only in
-// the English page, so it is not pinned here; translating it is left to the
-// locale maintainers.
 func TestCLIReferenceDocumentsSessionExport(t *testing.T) {
 	for _, locale := range []string{"en", "zh", "ja", "ru", "ko"} {
 		t.Run(locale, func(t *testing.T) {
@@ -67,10 +54,6 @@ func TestCLIReferenceDocumentsSessionExport(t *testing.T) {
 	}
 }
 
-// TestCLIReferenceUsesSubtaskUnit pins that every locale of the CLI reference
-// describes --concurrency/--timeout/--max-tools/--max-tokens/--no-filter in
-// terms of a subtask, not a file or file group. The five files are
-// hand-synced; the usual failure is one locale keeping the old unit.
 func TestCLIReferenceUsesSubtaskUnit(t *testing.T) {
 	type localePin struct {
 		locale string
@@ -135,3 +118,28 @@ func firstLineContaining(body, marker string) (string, bool) {
 	}
 	return "", false
 }
+
+func TestCLIReferenceDocumentsSessionRm(t *testing.T) {
+	for _, locale := range []string{"en", "zh", "ja", "ru", "ko"} {
+		t.Run(locale, func(t *testing.T) {
+			path := filepath.Join("..", "..", "pages", "src", "content", "docs", locale, "cli-reference.md")
+			body, err := os.ReadFile(path)
+			if err != nil {
+				t.Fatalf("read %s: %v", path, err)
+			}
+			for _, want := range []string{
+				"`ocr session rm <id>`",                                     // command-summary table row
+				"### `ocr session rm`",                                      // reference section
+				"`ocr session delete <id>`",                                 // alias
+				"ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes", // the flag in use, with an id shaped like a real one
+				"`--yes`", // the flag itself, however the locale punctuates the row
+				"`-y`",    // and its shorthand
+				"stdin",   // the non-interactive rule
+			} {
+				if !strings.Contains(string(body), want) {
+					t.Errorf("%s: missing %q", path, want)
+				}
+			}
+		})
+	}
+}

cmd/opencodereview/session_cmd.go

@@ -4,10 +4,12 @@
 package main
 
 import (
+	"bufio"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -83,9 +85,6 @@ var sessionCompareCmd = &cobra.Command{
 	Short:   "Compare the findings of two sessions",
 	Long:    "Group the findings of two review sessions into new, persisting, resolved and not-reviewed.\nFindings are matched on path, category and the offending snippet, so a finding that only moved down the file still counts as persisting.\nUse --json for machine-readable output.",
 	Args:    exactArgs(2),
-	// Both positionals are session ids, so the first one already typed must not
-	// stop completion of the second - hence the args reset rather than plain
-	// completeSessionIDs, which stops after one argument.
 	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
 		if len(args) > 1 {
 			return nil, cobra.ShellCompDirectiveNoFileComp
@@ -122,10 +121,56 @@ var sessionExportCmd = &cobra.Command{
 	},
 }
 
+var sessionRmRepoDir string
+var sessionRmYes bool
+
+var sessionRmCmd = &cobra.Command{
+	Use:     "rm [flags] <session-id>",
+	Aliases: []string{"delete", "remove"},
+	Short:   "Delete one saved review session",
+	Long: "Delete a persisted review session from ~/.opencodereview/sessions/.\n\n" +
+		"The id is enough on its own, so this works from anywhere; --repo only narrows\n" +
+		"the search. An id saved for more than one repository is listed, not guessed at.\n\n" +
+		"The file cannot be recovered, so this confirms first unless --yes is given.",
+	Example: "  ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b\n" +
+		"  ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes\n" +
+		"  ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project",
+	Args:              exactArgs(1),
+	ValidArgsFunction: completeSessionIDsAnywhere,
+	RunE: func(cmd *cobra.Command, args []string) error {
+		return runSessionRm(cmd, args[0])
+	},
+}
+
 // completeSessionIDs offers the persisted session ids for the current repo
 // (or --repo, when already typed) as shell completions, newest first, with a
 // short summary as the completion description. It completes one positional;
 // a command with two session ids (compare) resets args per positional.
+func completeSessionIDsAnywhere(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
+	if len(args) != 0 {
+		return nil, cobra.ShellCompDirectiveNoFileComp
+	}
+	if repo, _ := cmd.Flags().GetString("repo"); strings.TrimSpace(repo) != "" {
+		return completeSessionIDs(cmd, args, toComplete)
+	}
+	locations, err := session.ListAllSessionIDs()
+	if err != nil {
+		return nil, cobra.ShellCompDirectiveNoFileComp
+	}
+	completions := make([]string, 0, len(locations))
+	for _, loc := range locations {
+		if !strings.HasPrefix(loc.SessionID, toComplete) {
+			continue
+		}
+		repo := loc.RepoDir
+		if repo == "" {
+			repo = "(repository not recorded)"
+		}
+		completions = append(completions, loc.SessionID+"\t"+repo)
+	}
+	return completions, cobra.ShellCompDirectiveNoFileComp
+}
+
 func completeSessionIDs(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
 	if len(args) != 0 {
 		return nil, cobra.ShellCompDirectiveNoFileComp
@@ -171,11 +216,15 @@ func init() {
 	sessionExportCmd.Flags().StringVar(&sessionExportRepoDir, "repo", "", "root directory of the git repository (default: current dir)")
 	addOutputPathFlag(sessionExportCmd, &sessionExportOutput)
 
+	sessionRmCmd.Flags().StringVar(&sessionRmRepoDir, "repo", "", "only look for the session under this repository (default: every repository)")
+	sessionRmCmd.Flags().BoolVarP(&sessionRmYes, "yes", "y", false, "delete without asking for confirmation")
+
 	sessionCmd.AddCommand(sessionListCmd)
 	sessionCmd.AddCommand(sessionShowCmd)
 	sessionCmd.AddCommand(sessionCommentsCmd)
 	sessionCmd.AddCommand(sessionCompareCmd)
 	sessionCmd.AddCommand(sessionExportCmd)
+	sessionCmd.AddCommand(sessionRmCmd)
 }
 
 // runSessionExport writes one session's standalone HTML page to --output, or to
@@ -196,7 +245,6 @@ func runSessionExport(sessionID string) (retErr error) {
 		if len(summaries) == 0 {
 			return fmt.Errorf("no sessions found for %s; run a review first, or pass a session id", resolvedRepo)
 		}
-		// ListSessions sorts StartTime descending, so [0] is the newest.
 		sessionID = summaries[0].SessionID
 	}
 
@@ -317,15 +365,11 @@ func runSessionCompare(beforeID, afterID string) error {
 	if err != nil {
 		return fmt.Errorf("load session %q: %w", afterID, err)
 	}
-	// Comparing findings across repositories is meaningless, so it is an error
-	// rather than a warning. A different review mode or range still compares
-	// usefully (a full scan against a diff run, say), so that only warns.
 	if beforeSummary.RepoDir != afterSummary.RepoDir {
 		return fmt.Errorf("sessions belong to different repositories: %s was recorded in %s, %s in %s",
 			beforeID, beforeSummary.RepoDir, afterID, afterSummary.RepoDir)
 	}
 	if beforeSummary.ReviewMode != afterSummary.ReviewMode {
-		// stderr, never stdout: --json output is piped into other tools.
 		fmt.Fprintf(os.Stderr, "[ocr] WARNING review modes differ (%s vs %s); the two runs may not have looked at the same files\n",
 			displayMode(beforeSummary.ReviewMode), displayMode(afterSummary.ReviewMode))
 	}
@@ -440,6 +484,137 @@ func parseFilterSet(s string) map[string]bool {
 	return set
 }
 
+// runSessionRm deletes one persisted session, confirming first unless --yes.
+func runSessionRmAnywhere(cmd *cobra.Command, sessionID string) error {
+	found, err := session.FindSessionsByID(sessionID)
+	if err != nil {
+		return err
+	}
+	switch len(found) {
+	case 0:
+		return fmt.Errorf("no session %q is saved for any repository; run 'ocr session list' to see what is saved", sessionID)
+	case 1:
+	default:
+		var b strings.Builder
+		fmt.Fprintf(&b, "session %q is saved for more than one repository:\n", sessionID)
+		for _, loc := range found {
+			repo := loc.RepoDir
+			if repo == "" {
+				repo = "(repository not recorded)"
+			}
+			fmt.Fprintf(&b, "  %s\n", repo)
+		}
+		b.WriteString("\nRe-run with --repo to say which one to delete from.")
+		return errors.New(b.String())
+	}
+
+	loc := found[0]
+	summary, summaryErr := session.LoadSummaryAt(loc.Path, sessionID, loc.RepoDir)
+	if summaryErr != nil {
+		return fmt.Errorf("cannot read session %q: %w", sessionID, summaryErr)
+	}
+	readable := summary != nil && !summary.StartTime.IsZero()
+
+	if !sessionRmYes {
+		if readable {
+			fmt.Fprintf(cmd.OutOrStdout(), "Delete session %s?\n  repo:     %s\n  branch:   %s\n  started:  %s\n  files:    %s\n  comments: %d\n",
+				sessionID, summary.RepoDir, summary.GitBranch, describeStart(*summary), describeFiles(*summary), summary.TotalComments)
+		} else {
+			fmt.Fprintf(cmd.OutOrStdout(), "Delete session %s? (its metadata could not be read)\n", sessionID)
+		}
+		ok, err := confirmDeletion(cmd)
+		if err != nil {
+			return err
+		}
+		if !ok {
+			fmt.Fprintln(cmd.OutOrStdout(), "Cancelled.")
+			return nil
+		}
+	}
+
+	if err := session.DeleteSessionAt(loc); err != nil {
+		if errors.Is(err, session.ErrSessionNotFound) {
+			return fmt.Errorf("no session %q is saved for any repository; run 'ocr session list' to see what is saved", sessionID)
+		}
+		return err
+	}
+	fmt.Fprintf(cmd.OutOrStdout(), "Deleted session %s\n", sessionID)
+	return nil
+}
+
+func runSessionRm(cmd *cobra.Command, sessionID string) error {
+	if err := session.ValidateSessionID(sessionID); err != nil {
+		return err
+	}
+
+	if strings.TrimSpace(sessionRmRepoDir) == "" {
+		return runSessionRmAnywhere(cmd, sessionID)
+	}
+
+	repoDir, err := resolveWorkingDirForSession(sessionRmRepoDir)
+	if err != nil {
+		return err
+	}
+
+	summary, summaryErr := session.LoadSummary(repoDir, sessionID)
+	switch {
+	case summaryErr != nil && errors.Is(summaryErr, fs.ErrNotExist):
+		return fmt.Errorf("no session %q for this repository; run 'ocr session list' to see what is saved", sessionID)
+	case summaryErr != nil:
+		return fmt.Errorf("cannot read session %q: %w", sessionID, summaryErr)
+	}
+
+	if err := session.CheckSessionRepo(repoDir, sessionID); err != nil {
+		if errors.Is(err, session.ErrSessionUnverifiable) {
+			return fmt.Errorf("%w\n\nRun 'ocr session rm %s' without --repo: the id locates the file on\nits own, so nothing has to be verified against a repository.", err, sessionID)
+		}
+		if errors.Is(err, session.ErrSessionOtherRepo) {
+			return fmt.Errorf("%w\n\nTwo repository paths can share one session directory, so this session is\nvisible here but belongs elsewhere. Delete it from the repository that\nrecorded it.", err)
+		}
+		return err
+	}
+
+	readable := summary != nil && !summary.StartTime.IsZero()
+
+	if !sessionRmYes {
+		if readable {
+			fmt.Fprintf(cmd.OutOrStdout(), "Delete session %s?\n  repo:     %s\n  branch:   %s\n  started:  %s\n  files:    %s\n  comments: %d\n",
+				sessionID, summary.RepoDir, summary.GitBranch, describeStart(*summary), describeFiles(*summary), summary.TotalComments)
+		} else {
+			fmt.Fprintf(cmd.OutOrStdout(), "Delete session %s? (its metadata could not be read)\n", sessionID)
+		}
+		ok, err := confirmDeletion(cmd)
+		if err != nil {
+			return err
+		}
+		if !ok {
+			fmt.Fprintln(cmd.OutOrStdout(), "Cancelled.")
+			return nil
+		}
+	}
+
+	if err := session.DeleteSession(repoDir, sessionID); err != nil {
+		if errors.Is(err, session.ErrSessionNotFound) {
+			return fmt.Errorf("no session %q for this repository; run 'ocr session list' to see what is saved", sessionID)
+		}
+		return err
+	}
+	fmt.Fprintf(cmd.OutOrStdout(), "Deleted session %s\n", sessionID)
+	return nil
+}
+
+// confirmDeletion reads a yes/no answer; a non-interactive stdin answers no.
+func confirmDeletion(cmd *cobra.Command) (bool, error) {
+	fmt.Fprint(cmd.OutOrStdout(), "Type 'y' to confirm: ")
+	reader := bufio.NewReader(cmd.InOrStdin())
+	line, err := reader.ReadString('\n')
+	if err != nil && !errors.Is(err, io.EOF) {
+		return false, fmt.Errorf("read confirmation: %w", err)
+	}
+	answer := strings.ToLower(strings.TrimSpace(line))
+	return answer == "y" || answer == "yes", nil
+}
+
 // resolveWorkingDirForSession accepts an explicit --repo flag value and falls
 // back to the current working directory. Unlike resolveRepoDir it does not
 // require the target to be a git repository, so users can inspect sessions