diff --git a/cmd/opencodereview/cli_reference_compare_docs_test.go b/cmd/opencodereview/cli_reference_compare_docs_test.go index 4a12f772..575b8846 100644 --- a/cmd/opencodereview/cli_reference_compare_docs_test.go +++ b/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 `", // command-summary table row + "### `ocr session rm`", // reference section + "`ocr session delete `", // 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) + } + } + }) + } +} diff --git a/cmd/opencodereview/session_cmd.go b/cmd/opencodereview/session_cmd.go index f05ca0a5..f07141c6 100644 --- a/cmd/opencodereview/session_cmd.go +++ b/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] ", + 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 diff --git a/cmd/opencodereview/session_rm_test.go b/cmd/opencodereview/session_rm_test.go new file mode 100644 index 00000000..14cf8c1e --- /dev/null +++ b/cmd/opencodereview/session_rm_test.go @@ -0,0 +1,536 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/alibaba/open-code-review/internal/session" +) + +func seedRmSession(t *testing.T, _ /*home*/, repoDir, sessionID string) string { + t.Helper() + dir, err := session.SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dir, sessionID+".jsonl") + if err := os.WriteFile(path, []byte("{}\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return path +} + +func TestSessionRm_DeclinedConfirmationKeepsTheSession(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSessionRecorded(t, repoDir, "20250601-100000-aaaaaa", "main") + + sessionRmRepoDir = repoDir + sessionRmYes = false + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("n\n")) + + if err := runSessionRm(sessionRmCmd, "20250601-100000-aaaaaa"); err != nil { + t.Fatalf("runSessionRm: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("session was deleted despite a declined confirmation: %v", err) + } + if !strings.Contains(out.String(), "Cancelled") { + t.Errorf("expected the command to say it cancelled, got %q", out.String()) + } +} + +func TestSessionRm_NonInteractiveStdinDoesNotDelete(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSessionRecorded(t, repoDir, "20250601-100000-aaaaaa", "main") + + sessionRmRepoDir = repoDir + sessionRmYes = false + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + sessionRmCmd.SetOut(&bytes.Buffer{}) + sessionRmCmd.SetIn(strings.NewReader("")) + + if err := runSessionRm(sessionRmCmd, "20250601-100000-aaaaaa"); err != nil { + t.Fatalf("runSessionRm: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("session was deleted on an empty stdin: %v", err) + } +} + +func TestSessionRm_YesFlagDeletesWithoutPrompting(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSessionRecorded(t, repoDir, "20250601-100000-aaaaaa", "main") + + sessionRmRepoDir = repoDir + sessionRmYes = true + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("")) + + if err := runSessionRm(sessionRmCmd, "20250601-100000-aaaaaa"); err != nil { + t.Fatalf("runSessionRm: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("session still present after --yes: %v", err) + } + if !strings.Contains(out.String(), "Deleted session") { + t.Errorf("expected a confirmation line, got %q", out.String()) + } +} + +func TestSessionRm_UnknownIDExplainsHowToList(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + + sessionRmRepoDir = repoDir + sessionRmYes = true + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + sessionRmCmd.SetOut(&bytes.Buffer{}) + sessionRmCmd.SetIn(strings.NewReader("")) + + err := runSessionRm(sessionRmCmd, "20250601-999999-zzzzzz") + if err == nil { + t.Fatal("expected an error for an unknown session id") + } + if !strings.Contains(err.Error(), "ocr session list") { + t.Errorf("error should point at 'ocr session list', got %q", err.Error()) + } +} + +func TestSessionRm_RejectsTraversalIDsBeforeTouchingTheFilesystem(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + + sessionRmRepoDir = repoDir + sessionRmYes = true + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("")) + + for _, id := range []string{"../escape", "../../escape", "a/b", "..", "."} { + t.Run(id, func(t *testing.T) { + err := runSessionRm(sessionRmCmd, id) + if err == nil { + t.Fatalf("runSessionRm(%q) returned no error", id) + } + if strings.Contains(err.Error(), "ocr session list") { + t.Errorf("runSessionRm(%q) reported a missing session; an invalid id must be refused as invalid", id) + } + }) + } +} + +func TestSessionRm_CorruptSessionIsDeletableAndDescribedHonestly(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + + dir, err := session.SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dir, "20250601-100000-corrupt.jsonl") + if err := os.WriteFile(path, []byte("this is not json\n{\"also\": not json\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + sessionRmRepoDir = "" + sessionRmYes = false + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("y\n")) + + if err := runSessionRm(sessionRmCmd, "20250601-100000-corrupt"); err != nil { + t.Fatalf("runSessionRm: %v", err) + } + if !strings.Contains(out.String(), "could not be read") { + t.Errorf("a corrupt session should be described as unreadable, got %q", out.String()) + } + if strings.Contains(out.String(), "started: 0001-01-01") { + t.Errorf("zero-value metadata was printed as if real: %q", out.String()) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("corrupt session was not deleted: %v", err) + } +} + +func TestSessionRm_OtherRepoRefusedBeforePrompting(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + + base := t.TempDir() + repoA := filepath.Join(base, "a-b", "c") + repoB := filepath.Join(base, "a", "b-c") + for _, d := range []string{repoA, repoB} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + } + dirA, _ := session.SessionsDir(repoA) + dirB, _ := session.SessionsDir(repoB) + if dirA != dirB { + t.Skip("paths no longer collide; the encoding may have been fixed") + } + if err := os.MkdirAll(dirB, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dirB, "20250601-100000-bbbbbb.jsonl") + rec := `{"type":"session_start","cwd":"` + strings.ReplaceAll(repoB, `\`, `\\`) + `","sessionId":"20250601-100000-bbbbbb","timestamp":"2025-06-01T10:00:00Z"}` + "\n" + if err := os.WriteFile(path, []byte(rec), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + sessionRmRepoDir = repoA + sessionRmYes = false + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("y\n")) + + err := runSessionRm(sessionRmCmd, "20250601-100000-bbbbbb") + if err == nil { + t.Fatal("deleting another repository's session must fail") + } + if !strings.Contains(err.Error(), "different repository") { + t.Errorf("error should name the cause, got %q", err.Error()) + } + if strings.Contains(out.String(), "Delete session") { + t.Errorf("the user was prompted before the refusal: %q", out.String()) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("the other repository's session was deleted: %v", statErr) + } +} + +func makeUnreadable(t *testing.T, path string) { + t.Helper() + if err := os.Chmod(path, 0o000); err != nil { + t.Fatalf("Chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + if _, err := os.ReadFile(path); err == nil { + t.Skipf("this platform does not enforce the file mode: %s is still readable after chmod 000", path) + } +} + +func TestSessionRm_UnreadableSessionIsReportedNotTreatedAsCorrupt(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSession(t, home, repoDir, "20250601-100000-locked") + makeUnreadable(t, path) + + sessionRmRepoDir = repoDir + sessionRmYes = true + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + sessionRmCmd.SetOut(&bytes.Buffer{}) + sessionRmCmd.SetIn(strings.NewReader("")) + + err := runSessionRm(sessionRmCmd, "20250601-100000-locked") + if err == nil { + t.Fatal("an unreadable session must be reported, not deleted") + } + if !strings.Contains(err.Error(), "cannot read session") { + t.Errorf("error should say the file could not be read, got %q", err.Error()) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("an unreadable session was deleted anyway: %v", statErr) + } +} + +func TestSessionRm_PromptMatchesTheListFormatting(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + + dir, err := session.SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dir, "20250601-100000-fmt.jsonl") + rec := `{"type":"session_start","cwd":"` + strings.ReplaceAll(repoDir, `\`, `\\`) + + `","sessionId":"20250601-100000-fmt","timestamp":"2025-06-01T10:00:00Z","gitBranch":"feature-x"}` + "\n" + if err := os.WriteFile(path, []byte(rec), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + sessionRmRepoDir = repoDir + sessionRmYes = false + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + out := &bytes.Buffer{} + sessionRmCmd.SetOut(out) + sessionRmCmd.SetIn(strings.NewReader("n\n")) + + if err := runSessionRm(sessionRmCmd, "20250601-100000-fmt"); err != nil { + t.Fatalf("runSessionRm: %v", err) + } + got := out.String() + if strings.Contains(got, "T10:00:00Z") { + t.Errorf("prompt used RFC3339; 'ocr session list' shows a local timestamp: %q", got) + } + for _, want := range []string{"repo:", "branch:", "started:", "files:", "comments:"} { + if !strings.Contains(got, want) { + t.Errorf("prompt is missing %q: %q", want, got) + } + } +} + +func seedRmSessionRecorded(t *testing.T, repoDir, sessionID, branch string) string { + t.Helper() + path := seedRmSession(t, "", repoDir, sessionID) + rec := `{"type":"session_start","cwd":"` + strings.ReplaceAll(repoDir, `\`, `\\`) + + `","sessionId":"` + sessionID + `","timestamp":"2025-06-01T10:00:00Z","gitBranch":"` + branch + `"}` + "\n" + if err := os.WriteFile(path, []byte(rec), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return path +} + +func runSessionRmForTest(t *testing.T, repoFlag, sessionID, stdin string, yes bool) (string, error) { + t.Helper() + sessionRmRepoDir = repoFlag + sessionRmYes = yes + t.Cleanup(func() { sessionRmRepoDir, sessionRmYes = "", false }) + var out bytes.Buffer + sessionRmCmd.SetOut(&out) + sessionRmCmd.SetIn(strings.NewReader(stdin)) + err := runSessionRm(sessionRmCmd, sessionID) + return out.String(), err +} + +func TestSessionRm_FindsTheSessionWithoutBeingToldTheRepository(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSessionRecorded(t, repoDir, "20250601-100000-alone", "main") + + out, err := runSessionRmForTest(t, "", "20250601-100000-alone", "y\n", false) + if err != nil { + t.Fatalf("runSessionRm: %v", err) + } + if !strings.Contains(out, repoDir) { + t.Errorf("the prompt must name the repository the session was found in, got %q", out) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Errorf("session was not deleted: %v", statErr) + } +} + +func TestSessionRm_ListsCandidatesRatherThanGuessing(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoA := t.TempDir() + repoB := t.TempDir() + pathA := seedRmSessionRecorded(t, repoA, "20250601-100000-shared", "main") + pathB := seedRmSessionRecorded(t, repoB, "20250601-100000-shared", "feature") + + _, err := runSessionRmForTest(t, "", "20250601-100000-shared", "y\n", true) + if err == nil { + t.Fatal("an id saved for two repositories must not be deleted on a guess") + } + for _, want := range []string{repoA, repoB, "--repo"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error should mention %q so the caller can disambiguate, got %q", want, err.Error()) + } + } + for _, path := range []string{pathA, pathB} { + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("an ambiguous id deleted %s anyway: %v", path, statErr) + } + } +} + +func TestSessionRm_RepoFlagNarrowsAnAmbiguousID(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoA := t.TempDir() + repoB := t.TempDir() + pathA := seedRmSessionRecorded(t, repoA, "20250601-100000-shared", "main") + pathB := seedRmSessionRecorded(t, repoB, "20250601-100000-shared", "feature") + + if _, err := runSessionRmForTest(t, repoB, "20250601-100000-shared", "", true); err != nil { + t.Fatalf("runSessionRm --repo: %v", err) + } + if _, statErr := os.Stat(pathB); !os.IsNotExist(statErr) { + t.Errorf("the named repository's session was not deleted: %v", statErr) + } + if _, statErr := os.Stat(pathA); statErr != nil { + t.Errorf("the other repository's session was deleted too: %v", statErr) + } +} + +func TestSessionRm_UnknownIDAnywhereSaysSoWithoutPrompting(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + seedRmSessionRecorded(t, t.TempDir(), "20250601-100000-present", "main") + + out, err := runSessionRmForTest(t, "", "20250601-100000-missing", "y\n", false) + if err == nil { + t.Fatal("an id that is saved nowhere must be an error") + } + if !strings.Contains(err.Error(), "ocr session list") { + t.Errorf("error should point at the listing command, got %q", err.Error()) + } + if strings.Contains(out, "Delete session") { + t.Errorf("the caller must not be asked to confirm deleting something that is not there, got %q", out) + } +} + +func TestSessionRm_TraversalIDIsRefusedBeforeSearchingEveryRepository(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + + for _, id := range []string{"../escape", "sub/../../escape", `..\escape`, ".", ".."} { + if _, err := session.FindSessionsByID(id); err == nil { + t.Errorf("FindSessionsByID(%q) must be refused", id) + } + if _, err := runSessionRmForTest(t, "", id, "", true); err == nil { + t.Errorf("ocr session rm %q must be refused", id) + } + } +} + +func TestSessionRm_RepoFlagRefusesAnUnverifiableSession(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSession(t, home, repoDir, "20250601-100000-nocwd") + + out, err := runSessionRmForTest(t, repoDir, "20250601-100000-nocwd", "y\n", true) + if err == nil { + t.Fatal("a session that records no repository must not be deleted on the strength of --repo") + } + if !errors.Is(err, session.ErrSessionUnverifiable) { + t.Errorf("error should be ErrSessionUnverifiable, got %v", err) + } + if !strings.Contains(err.Error(), "without --repo") { + t.Errorf("error should say how to delete it instead, got %q", err.Error()) + } + if strings.Contains(out, "Delete session") { + t.Errorf("the caller must not be prompted before the refusal, got %q", out) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("the session was deleted anyway: %v", statErr) + } +} + +func TestSessionRm_UnverifiableSessionIsStillDeletableByIDAlone(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + repoDir := t.TempDir() + path := seedRmSession(t, home, repoDir, "20250601-100000-nocwd") + + if _, err := runSessionRmForTest(t, "", "20250601-100000-nocwd", "", true); err != nil { + t.Fatalf("runSessionRm without --repo: %v", err) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Errorf("the session was not deleted: %v", statErr) + } +} + +func TestSessionRm_CompletionOffersSessionsFromEveryRepository(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + here := t.TempDir() + elsewhere := t.TempDir() + seedRmSessionRecorded(t, here, "20250601-100000-here", "main") + seedRmSessionRecorded(t, elsewhere, "20250601-100000-away", "main") + + sessionRmRepoDir = "" + t.Cleanup(func() { sessionRmRepoDir = "" }) + if err := sessionRmCmd.Flags().Set("repo", ""); err != nil { + t.Fatalf("clear --repo: %v", err) + } + + got, _ := completeSessionIDsAnywhere(sessionRmCmd, nil, "20250601") + joined := strings.Join(got, "\n") + for _, want := range []string{"20250601-100000-here", "20250601-100000-away"} { + if !strings.Contains(joined, want) { + t.Errorf("completion is missing %q; got %q", want, joined) + } + } + if !strings.Contains(joined, elsewhere) { + t.Errorf("completion should name the repository each id belongs to; got %q", joined) + } +} + +func TestSessionRm_CompletionNarrowsWithRepoFlag(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + here := t.TempDir() + elsewhere := t.TempDir() + seedRmSessionRecorded(t, here, "20250601-100000-here", "main") + seedRmSessionRecorded(t, elsewhere, "20250601-100000-away", "main") + + if err := sessionRmCmd.Flags().Set("repo", here); err != nil { + t.Fatalf("set --repo: %v", err) + } + t.Cleanup(func() { _ = sessionRmCmd.Flags().Set("repo", ""); sessionRmRepoDir = "" }) + + joined := strings.Join(firstOf(completeSessionIDsAnywhere(sessionRmCmd, nil, "20250601")), "\n") + if !strings.Contains(joined, "20250601-100000-here") { + t.Errorf("completion lost this repository's session; got %q", joined) + } + if strings.Contains(joined, "20250601-100000-away") { + t.Errorf("--repo should scope the completion; got %q", joined) + } +} + +func firstOf(completions []string, _ cobra.ShellCompDirective) []string { return completions } diff --git a/internal/session/delete.go b/internal/session/delete.go new file mode 100644 index 00000000..efdf5801 --- /dev/null +++ b/internal/session/delete.go @@ -0,0 +1,275 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "sort" + "strings" +) + +// ErrSessionNotFound reports that no persisted session matched the id. +var ErrSessionNotFound = errors.New("session not found") + +// ErrSessionOtherRepo reports that a session records a different repository. +var ErrSessionOtherRepo = errors.New("session belongs to a different repository") + +// ErrSessionOutsideRoot reports that a Location falls outside the sessions directory. +var ErrSessionOutsideRoot = errors.New("session path is outside the sessions directory") + +// ErrSessionUnverifiable reports that a session records no repository to check --repo against. +var ErrSessionUnverifiable = errors.New("session records no repository") + +// ValidateSessionID rejects ids that are not a single path element. +func ValidateSessionID(sessionID string) error { + if strings.TrimSpace(sessionID) == "" { + return fmt.Errorf("session id is required") + } + if sessionID != filepath.Base(sessionID) || + strings.ContainsAny(sessionID, `/\`) || + sessionID == "." || sessionID == ".." { + return fmt.Errorf("invalid session id %q: must not contain a path", sessionID) + } + return nil +} + +// CheckSessionRepo reports whether a session may be operated on from repoDir. +func CheckSessionRepo(repoDir, sessionID string) error { + if err := ValidateSessionID(sessionID); err != nil { + return err + } + path, err := sessionPath(repoDir, sessionID) + if err != nil { + return err + } + if _, err := os.Stat(path); err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%w: %s", ErrSessionNotFound, sessionID) + } + return fmt.Errorf("stat session %q: %w", sessionID, err) + } + recorded, ok := recordedRepoDir(path) + if !ok { + return fmt.Errorf( + "%w: %s records no repository, so it cannot be verified as this one; delete it by id alone, without --repo", + ErrSessionUnverifiable, sessionID, + ) + } + if !sameRepoPath(recorded, repoDir) { + return fmt.Errorf("%w: %s was recorded for %s", ErrSessionOtherRepo, sessionID, recorded) + } + return nil +} + +// sessionPath resolves a validated session id to its file inside the sessions directory. +func sessionPath(repoDir, sessionID string) (string, error) { + dir, err := SessionsDir(repoDir) + if err != nil { + return "", err + } + path := filepath.Join(dir, sessionID+".jsonl") + if filepath.Dir(path) != filepath.Clean(dir) { + return "", fmt.Errorf("invalid session id %q: resolves outside the sessions directory", sessionID) + } + return path, nil +} + +// DeleteSession removes one persisted session for a repository. +func DeleteSession(repoDir, sessionID string) error { + if err := CheckSessionRepo(repoDir, sessionID); err != nil { + return err + } + path, err := sessionPath(repoDir, sessionID) + if err != nil { + return err + } + if err := os.Remove(path); err != nil { + return fmt.Errorf("delete session %q: %w", sessionID, err) + } + return nil +} + +// recordedRepoDir returns the working directory a session recorded, and whether it had one. +func recordedRepoDir(path string) (string, bool) { + var cwd string + if err := walkSessionFile(path, func(rec summaryRecord) { + if rec.Cwd != "" { + cwd = rec.Cwd + } + }); err != nil { + return "", false + } + if cwd == "" { + return "", false + } + return cwd, true +} + +// sameRepoPath reports whether two repository paths name the same directory. +func sameRepoPath(a, b string) bool { + ca, cb := filepath.Clean(a), filepath.Clean(b) + if ca == cb { + return true + } + fa, errA := os.Stat(ca) + fb, errB := os.Stat(cb) + if errA != nil || errB != nil { + return false + } + return os.SameFile(fa, fb) +} + +// Location is one on-disk session file, with the repository it recorded. +type Location struct { + SessionID string + Path string + RepoDir string + EncodedDir string +} + +// FindSessionsByID returns every session file matching an id, in any repository. +func FindSessionsByID(sessionID string) ([]Location, error) { + if err := ValidateSessionID(sessionID); err != nil { + return nil, err + } + home, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("resolve home dir: %w", err) + } + root := filepath.Join(home, ".opencodereview", sessionSubDir) + entries, err := os.ReadDir(root) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read sessions dir %q: %w", root, err) + } + + var found []Location + for _, entry := range entries { + if !entry.IsDir() { + continue + } + path := filepath.Join(root, entry.Name(), sessionID+".jsonl") + info, statErr := os.Stat(path) + if statErr != nil { + if os.IsNotExist(statErr) { + continue + } + return nil, fmt.Errorf("stat session %q: %w", path, statErr) + } + if info.IsDir() { + continue + } + repoDir, _ := recordedRepoDir(path) + found = append(found, Location{ + SessionID: sessionID, + Path: path, + RepoDir: repoDir, + EncodedDir: entry.Name(), + }) + } + sort.Slice(found, func(i, j int) bool { + if found[i].RepoDir != found[j].RepoDir { + return found[i].RepoDir < found[j].RepoDir + } + return found[i].EncodedDir < found[j].EncodedDir + }) + return found, nil +} + +// sessionsRoot returns the directory that holds every repository's sessions. +func sessionsRoot() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolve home dir: %w", err) + } + return filepath.Join(home, ".opencodereview", sessionSubDir), nil +} + +// DeleteSessionAt removes one session file found by FindSessionsByID. +func DeleteSessionAt(loc Location) error { + path, err := resolveLocationPath(loc) + if err != nil { + return err + } + if err := os.Remove(path); err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("%w: %s", ErrSessionNotFound, loc.SessionID) + } + return fmt.Errorf("delete session %q: %w", loc.SessionID, err) + } + return nil +} + +// resolveLocationPath rebuilds a Location's path from its parts and checks the two agree. +func resolveLocationPath(loc Location) (string, error) { + if err := ValidateSessionID(loc.SessionID); err != nil { + return "", err + } + if loc.EncodedDir == "" || + loc.EncodedDir != filepath.Base(loc.EncodedDir) || + strings.ContainsAny(loc.EncodedDir, `/\`) || + loc.EncodedDir == "." || loc.EncodedDir == ".." { + return "", fmt.Errorf("%w: %q is not a single sessions directory", ErrSessionOutsideRoot, loc.EncodedDir) + } + root, err := sessionsRoot() + if err != nil { + return "", err + } + want := filepath.Join(root, loc.EncodedDir, loc.SessionID+".jsonl") + if filepath.Clean(loc.Path) != want { + return "", fmt.Errorf("%w: %s is not %s", ErrSessionOutsideRoot, loc.Path, want) + } + return want, nil +} + +// ListAllSessionIDs returns every saved session id. +func ListAllSessionIDs() ([]Location, error) { + root, err := sessionsRoot() + if err != nil { + return nil, err + } + entries, err := os.ReadDir(root) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("read sessions dir %q: %w", root, err) + } + + var found []Location + for _, entry := range entries { + if !entry.IsDir() { + continue + } + files, readErr := os.ReadDir(filepath.Join(root, entry.Name())) + if readErr != nil { + continue + } + for _, f := range files { + name := f.Name() + if f.IsDir() || !strings.HasSuffix(name, ".jsonl") { + continue + } + id := strings.TrimSuffix(name, ".jsonl") + if ValidateSessionID(id) != nil { + continue + } + path := filepath.Join(root, entry.Name(), name) + repoDir, _ := recordedRepoDir(path) + found = append(found, Location{ + SessionID: id, + Path: path, + RepoDir: repoDir, + EncodedDir: entry.Name(), + }) + } + } + sort.Slice(found, func(i, j int) bool { return found[i].SessionID < found[j].SessionID }) + return found, nil +} diff --git a/internal/session/delete_test.go b/internal/session/delete_test.go new file mode 100644 index 00000000..ebf7c1b7 --- /dev/null +++ b/internal/session/delete_test.go @@ -0,0 +1,361 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "errors" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +func seedSession(t *testing.T, repoDir, sessionID string) string { + t.Helper() + dir, err := SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dir, sessionID+".jsonl") + if err := os.WriteFile(path, []byte("{}\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return path +} + +func seedAttributedSession(t *testing.T, repoDir, sessionID string) string { + t.Helper() + path := seedSession(t, repoDir, sessionID) + rec := `{"type":"session_start","cwd":"` + strings.ReplaceAll(repoDir, `\`, `\\`) + + `","sessionId":"` + sessionID + `","timestamp":"2025-06-01T10:00:00Z"}` + "\n" + if err := os.WriteFile(path, []byte(rec), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + return path +} + +func TestDeleteSession_RemovesOnlyTheNamedSession(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + + target := seedAttributedSession(t, repoDir, "20250601-100000-aaaaaa") + keep := seedAttributedSession(t, repoDir, "20250601-110000-bbbbbb") + + if err := DeleteSession(repoDir, "20250601-100000-aaaaaa"); err != nil { + t.Fatalf("DeleteSession: %v", err) + } + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Errorf("target session still present: %v", err) + } + if _, err := os.Stat(keep); err != nil { + t.Errorf("unrelated session was removed: %v", err) + } +} + +func TestDeleteSession_MissingSessionIsReportedNotSilentlyIgnored(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + seedSession(t, repoDir, "20250601-100000-aaaaaa") + + err := DeleteSession(repoDir, "20250601-999999-zzzzzz") + if !errors.Is(err, ErrSessionNotFound) { + t.Fatalf("want ErrSessionNotFound, got %v", err) + } +} + +func TestDeleteSession_RefusesIdsThatEscapeTheSessionsDirectory(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + + dir, err := SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + outside := filepath.Join(filepath.Dir(dir), "important.jsonl") + if err := os.WriteFile(outside, []byte("keep me\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + for _, id := range []string{ + "../important", + "../../important", + "..", + ".", + "sub/../../important", + `..\important`, + "a/b", + } { + t.Run(id, func(t *testing.T) { + err := DeleteSession(repoDir, id) + if err == nil { + t.Fatalf("DeleteSession(%q) returned no error", id) + } + if errors.Is(err, ErrSessionNotFound) { + t.Errorf("DeleteSession(%q) treated a traversal as a missing session; it must be refused outright", id) + } + }) + } + + if _, err := os.Stat(outside); err != nil { + t.Fatalf("a file outside the sessions directory was removed: %v", err) + } +} + +func TestDeleteSession_RefusesAnEmptyID(t *testing.T) { + setTestHome(t, t.TempDir()) + for _, id := range []string{"", " "} { + if err := DeleteSession(t.TempDir(), id); err == nil { + t.Errorf("DeleteSession(%q) returned no error", id) + } + } +} + +func TestDeleteSession_RefusesASessionRecordedForAnotherRepo(t *testing.T) { + home := t.TempDir() + setTestHome(t, home) + + base := t.TempDir() + repoA := filepath.Join(base, "a-b", "c") + repoB := filepath.Join(base, "a", "b-c") + for _, d := range []string{repoA, repoB} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + } + + dirA, err := SessionsDir(repoA) + if err != nil { + t.Fatalf("SessionsDir(A): %v", err) + } + dirB, err := SessionsDir(repoB) + if err != nil { + t.Fatalf("SessionsDir(B): %v", err) + } + if dirA != dirB { + t.Skipf("paths no longer collide (%s vs %s); the encoding may have been fixed", dirA, dirB) + } + + if err := os.MkdirAll(dirB, 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dirB, "20250601-100000-bbbbbb.jsonl") + rec := `{"type":"session_start","cwd":` + strconv.Quote(repoB) + `,"sessionId":"20250601-100000-bbbbbb"}` + "\n" + if err := os.WriteFile(path, []byte(rec), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + err = DeleteSession(repoA, "20250601-100000-bbbbbb") + if !errors.Is(err, ErrSessionOtherRepo) { + t.Fatalf("want ErrSessionOtherRepo, got %v", err) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("another repository's session was deleted: %v", statErr) + } + + if err := DeleteSession(repoB, "20250601-100000-bbbbbb"); err != nil { + t.Fatalf("DeleteSession from the owning repo: %v", err) + } +} + +func TestDeleteSession_UnattributableSessionIsRefusedAgainstARepository(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + path := seedSession(t, repoDir, "20250601-100000-nocwd") + + err := DeleteSession(repoDir, "20250601-100000-nocwd") + if !errors.Is(err, ErrSessionUnverifiable) { + t.Fatalf("DeleteSession error = %v, want ErrSessionUnverifiable", err) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("session was deleted anyway: %v", statErr) + } +} + +func TestDeleteSessionAt_UnattributableSessionIsStillDeletable(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + path := seedSession(t, repoDir, "20250601-100000-nocwd") + + found, err := FindSessionsByID("20250601-100000-nocwd") + if err != nil { + t.Fatalf("FindSessionsByID: %v", err) + } + if len(found) != 1 { + t.Fatalf("found %d sessions, want 1", len(found)) + } + if err := DeleteSessionAt(found[0]); err != nil { + t.Fatalf("DeleteSessionAt: %v", err) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Errorf("session still present: %v", statErr) + } + _ = repoDir +} + +func TestSameRepoPath_OneDirectorySpelledTwoWays(t *testing.T) { + base := t.TempDir() + repo := filepath.Join(base, "a-b") + if err := os.MkdirAll(repo, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + t.Run("trailing separator and dot segments", func(t *testing.T) { + if !sameRepoPath(repo, repo+string(os.PathSeparator)) { + t.Error("a trailing separator must not read as a different repository") + } + if !sameRepoPath(repo, filepath.Join(repo, ".")) { + t.Error("a dot segment must not read as a different repository") + } + }) + + t.Run("reached through a symlink", func(t *testing.T) { + link := filepath.Join(base, "link-to-a-b") + if err := os.Symlink(repo, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if !sameRepoPath(repo, link) { + t.Error("a symlink to the same directory must compare equal") + } + }) + + t.Run("case variation", func(t *testing.T) { + upper := filepath.Join(base, "A-B") + if _, err := os.Stat(upper); err != nil { + t.Skip("case-sensitive filesystem: A-B and a-b are different directories here") + } + if !sameRepoPath(repo, upper) { + t.Error("on a case-insensitive filesystem the two spellings are one directory") + } + }) + + t.Run("genuinely different directories stay distinct", func(t *testing.T) { + other := filepath.Join(base, "c-d") + if err := os.MkdirAll(other, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if sameRepoPath(repo, other) { + t.Error("two different directories must not compare equal") + } + }) + + t.Run("a vanished directory falls back to exact spelling", func(t *testing.T) { + gone := filepath.Join(base, "removed") + if !sameRepoPath(gone, gone) { + t.Error("an identical path must match even when it no longer exists") + } + if sameRepoPath(gone, filepath.Join(base, "removed-other")) { + t.Error("without a filesystem to ask, differing paths must not match") + } + }) +} + +func TestDeleteSessionAt_RefusesPathOutsideSessionsRoot(t *testing.T) { + setTestHome(t, t.TempDir()) + outside := filepath.Join(t.TempDir(), "victim.jsonl") + if err := os.WriteFile(outside, []byte("not a session\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + err := DeleteSessionAt(Location{SessionID: "victim", Path: outside, EncodedDir: "anything"}) + if !errors.Is(err, ErrSessionOutsideRoot) { + t.Fatalf("DeleteSessionAt error = %v, want ErrSessionOutsideRoot", err) + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Errorf("a file outside the sessions root was deleted: %v", statErr) + } +} + +func TestDeleteSessionAt_RefusesATraversingEncodedDir(t *testing.T) { + home := t.TempDir() + setTestHome(t, home) + outside := filepath.Join(home, "important.jsonl") + if err := os.WriteFile(outside, []byte("keep me\n"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + for _, encoded := range []string{"..", ".", "../..", "a/b", `a\b`, ""} { + loc := Location{ + SessionID: "important", + EncodedDir: encoded, + Path: filepath.Join(home, ".opencodereview", sessionSubDir, encoded, "important.jsonl"), + } + if err := DeleteSessionAt(loc); !errors.Is(err, ErrSessionOutsideRoot) { + t.Errorf("EncodedDir %q: error = %v, want ErrSessionOutsideRoot", encoded, err) + } + } + if _, statErr := os.Stat(outside); statErr != nil { + t.Errorf("a file outside the sessions root was deleted: %v", statErr) + } +} + +func TestDeleteSessionAt_RefusesAPathThatDisagreesWithItsParts(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + path := seedSession(t, repoDir, "20250601-100000-aaaaaa") + + found, err := FindSessionsByID("20250601-100000-aaaaaa") + if err != nil || len(found) != 1 { + t.Fatalf("FindSessionsByID: %v, %d results", err, len(found)) + } + forged := found[0] + forged.EncodedDir = forged.EncodedDir + "-other" + + if err := DeleteSessionAt(forged); !errors.Is(err, ErrSessionOutsideRoot) { + t.Errorf("error = %v, want ErrSessionOutsideRoot", err) + } + if _, statErr := os.Stat(path); statErr != nil { + t.Errorf("the session was deleted on a Location that does not describe it: %v", statErr) + } +} + +func TestDeleteSessionAt_DeletesALocationFromTheSearch(t *testing.T) { + setTestHome(t, t.TempDir()) + repoDir := t.TempDir() + path := seedSession(t, repoDir, "20250601-100000-aaaaaa") + + found, err := FindSessionsByID("20250601-100000-aaaaaa") + if err != nil || len(found) != 1 { + t.Fatalf("FindSessionsByID: %v, %d results", err, len(found)) + } + if err := DeleteSessionAt(found[0]); err != nil { + t.Fatalf("DeleteSessionAt: %v", err) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Errorf("session still present: %v", statErr) + } +} + +func TestFindSessionsByID_ReportsAStatFailureRatherThanSkippingIt(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root traverses a 0000 directory regardless of its mode") + } + home := t.TempDir() + setTestHome(t, home) + repoDir := t.TempDir() + path := seedSession(t, repoDir, "20250601-100000-aaaaaa") + + dir := filepath.Dir(path) + if err := os.Chmod(dir, 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + if _, err := os.Stat(path); err == nil || os.IsNotExist(err) { + t.Skip("this environment does not enforce directory modes") + } + + found, err := FindSessionsByID("20250601-100000-aaaaaa") + if err == nil { + t.Fatalf("a stat failure must be reported, got %d results and no error", len(found)) + } + if os.IsNotExist(err) { + t.Errorf("a permission failure was reported as a missing file: %v", err) + } +} diff --git a/internal/session/list.go b/internal/session/list.go index 0fd424b5..c18acbdf 100644 --- a/internal/session/list.go +++ b/internal/session/list.go @@ -340,3 +340,13 @@ func parseRecordTime(s string) time.Time { } return time.Time{} } + +// LoadSummaryAt reads one session summary from an exact file path. +// +// LoadSummary derives the path from a repository, which the global id search +// deliberately does not have: it found the file first and learns the repository +// from the record inside it. fallbackRepoDir is only used when the file carries +// no cwd of its own. +func LoadSummaryAt(path, sessionID, fallbackRepoDir string) (*Summary, error) { + return loadSummaryFromFile(path, sessionID, fallbackRepoDir) +} diff --git a/pages/src/content/docs/en/cli-reference.md b/pages/src/content/docs/en/cli-reference.md index 8f187785..d14cdcfc 100644 --- a/pages/src/content/docs/en/cli-reference.md +++ b/pages/src/content/docs/en/cli-reference.md @@ -82,6 +82,7 @@ ocr review --commit HEAD | gh issue comment 123 --body-file - | `ocr session comments ` | `ocr sessions comments ` | Print the review comments recorded in one session. | | `ocr session compare ` | `ocr session diff ` | Compare two sessions' findings: new, persisting, resolved, not reviewed. | | `ocr session export [id]` | — | Export one session as a self-contained HTML file. | +| `ocr session rm ` | `ocr session delete `, `ocr session remove ` | Delete one saved review session. | | `ocr viewer` | — | Launch the local web UI for past review sessions (`localhost:5483`). | | `ocr version` | — | Print version, commit, platform, build date, and GitHub URL. | @@ -500,6 +501,33 @@ treat the file with the same care as the repository itself before publishing it. | `--repo ` | current dir | Repository whose session should be exported. | | `--output `, `-o` | stdout | Write the HTML to a file instead of stdout. | +### `ocr session rm` + +Deletes one persisted session from `~/.opencodereview/sessions/`. + +```bash +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project +``` + +The id is enough on its own, so the command runs from any directory. If the same +id is saved for more than one repository, the candidates are listed and nothing +is deleted; pass `--repo` to pick one. + +The session's repository, branch, start time, file count and comment count are +printed, and you are asked to confirm. **A non-interactive stdin answers no**, so +a pipeline or a CI job must pass `--yes` (`-y`) to skip the prompt. + +A session whose metadata cannot be parsed is still deletable; one that cannot be +read at all is reported instead. With `--repo`, a session that records a +different repository, or none, is refused: delete it by id alone. + +| Flag | Default | Description | +|---|---|---| +| `--repo ` | every repository | Only look for the session under this repository. | +| `--yes`, `-y` | `false` | Skip the confirmation prompt. | + ## `ocr rules` Rule introspection. There is exactly one subcommand: diff --git a/pages/src/content/docs/ja/cli-reference.md b/pages/src/content/docs/ja/cli-reference.md index d2af911f..088d2ad2 100644 --- a/pages/src/content/docs/ja/cli-reference.md +++ b/pages/src/content/docs/ja/cli-reference.md @@ -80,6 +80,7 @@ ocr review --commit HEAD | gh issue comment 123 --body-file - | `ocr session comments ` | `ocr sessions comments ` | 1つのセッションに記録されたレビューコメントを表示します。 | | `ocr session compare ` | `ocr session diff ` | 2つのセッションの指摘を比較します：新規・継続・解決済み・未レビュー。 | | `ocr session export [id]` | — | 1つのセッションを自己完結型の HTML ファイルとしてエクスポートします。 | +| `ocr session rm ` | `ocr session delete `, `ocr session remove ` | 保存済みのレビューセッションを1つ削除します。 | | `ocr viewer` | — | 過去のレビューセッション用のローカル Web UI を起動します（`localhost:5483`）。 | | `ocr version` | — | バージョン、commit、プラットフォーム、ビルド日、GitHub URL を出力します。 | @@ -475,6 +476,34 @@ ocr session export 20250601-100000-abc123 -o review.html | `--repo ` | カレントディレクトリ | エクスポートするセッションが属するリポジトリ。 | | `--output `、`-o` | 標準出力 | HTML を標準出力ではなくファイルに書き出します。 | +### `ocr session rm` + +`~/.opencodereview/sessions/` から保存済みのセッションを1つ削除します。 + +```bash +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project +``` + +id だけで十分なので、どのディレクトリからでも実行できます。同じ id が複数の +リポジトリに保存されている場合は、候補を一覧表示して何も削除しません。`--repo` +で1つを指定してください。 + +セッションのリポジトリ、ブランチ、開始時刻、ファイル数、コメント数を表示し、 +確認を求めます。**非対話的な stdin は「いいえ」として扱われる**ため、パイプラインや +CI ジョブで確認を省略するには `--yes`（`-y`）を渡してください。 + +メタデータを解析できないセッションも削除できます。まったく読み取れない場合は、 +削除せずにエラーを報告します。`--repo` を指定した場合、別のリポジトリを記録している +セッションや、リポジトリを記録していないセッションは拒否されます。その場合は id +だけで削除してください。 + +| フラグ | デフォルト | 説明 | +|---|---|---| +| `--repo ` | すべてのリポジトリ | このリポジトリの下だけでセッションを探します。 | +| `--yes`、`-y` | `false` | 確認プロンプトを省略します。 | + ## `ocr rules` ルールの自己確認です。サブコマンドは 1 つだけです: diff --git a/pages/src/content/docs/ko/cli-reference.md b/pages/src/content/docs/ko/cli-reference.md index d2a7591f..4572a2d1 100644 --- a/pages/src/content/docs/ko/cli-reference.md +++ b/pages/src/content/docs/ko/cli-reference.md @@ -81,6 +81,7 @@ ocr review --commit HEAD | gh issue comment 123 --body-file - | `ocr session comments ` | `ocr sessions comments ` | 세션에 기록된 리뷰 코멘트를 출력합니다. | | `ocr session compare ` | `ocr session diff ` | 두 세션의 지적을 비교합니다: 새로 생긴 것, 남아 있는 것, 해결된 것, 리뷰하지 않은 것. | | `ocr session export [id]` | — | 세션 하나를 단일 HTML 파일로 내보냅니다. | +| `ocr session rm ` | `ocr session delete `, `ocr session remove ` | 저장된 리뷰 세션 하나를 삭제합니다. | | `ocr viewer` | — | 지난 리뷰 세션을 볼 수 있는 로컬 웹 UI를 띄웁니다(`localhost:5483`). | | `ocr version` | — | 버전, 커밋, 플랫폼, 빌드 날짜, GitHub URL을 출력합니다. | @@ -489,6 +490,33 @@ ocr session export 20250601-100000-abc123 -o review.html | `--repo ` | 현재 디렉터리 | 내보낼 세션이 속한 저장소. | | `--output `, `-o` | 표준 출력 | HTML을 표준 출력 대신 파일로 씁니다. | +### `ocr session rm` {#ocr-session-rm} + +`~/.opencodereview/sessions/`에 저장된 세션 하나를 삭제합니다. + +```bash +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project +``` + +id만으로 충분하므로 어느 디렉터리에서든 실행할 수 있습니다. 같은 id가 여러 +저장소에 저장되어 있으면 후보를 보여주고 아무것도 삭제하지 않습니다. `--repo`로 +하나를 지정하세요. + +세션의 저장소, 브랜치, 시작 시각, 파일 수, 댓글 수를 출력하고 확인을 요청합니다. +**비대화형 stdin은 "아니오"로 처리되므로**, 파이프라인이나 CI 작업에서 프롬프트를 +건너뛰려면 `--yes`(`-y`)를 전달해야 합니다. + +메타데이터를 해석할 수 없는 세션도 삭제할 수 있습니다. 아예 읽을 수 없는 경우에는 +삭제하지 않고 오류를 보고합니다. `--repo`를 지정하면 다른 저장소를 기록했거나 +저장소를 기록하지 않은 세션은 거부됩니다. 그때는 id만으로 삭제하세요. + +| 플래그 | 기본값 | 설명 | +|---|---|---| +| `--repo ` | 모든 저장소 | 이 저장소 아래에서만 세션을 찾습니다. | +| `--yes`, `-y` | `false` | 확인 프롬프트를 건너뜁니다. | + ## `ocr rules` {#ocr-rules} 규칙을 들여다보는 명령입니다. 하위 명령은 하나뿐입니다: diff --git a/pages/src/content/docs/ru/cli-reference.md b/pages/src/content/docs/ru/cli-reference.md index 9f8346cf..dbcf1be2 100644 --- a/pages/src/content/docs/ru/cli-reference.md +++ b/pages/src/content/docs/ru/cli-reference.md @@ -81,6 +81,7 @@ ocr review --commit HEAD | gh issue comment 123 --body-file - | `ocr session comments ` | `ocr sessions comments ` | Выводит комментарии ревью, записанные в одной сессии. | | `ocr session compare ` | `ocr session diff ` | Сравнивает находки двух сессий: новые, сохранившиеся, устранённые, не проверенные. | | `ocr session export [id]` | — | Экспортирует одну сессию в автономный HTML-файл. | +| `ocr session rm ` | `ocr session delete `, `ocr session remove ` | Удаляет одну сохранённую сессию ревью. | | `ocr viewer` | — | Запускает локальный веб-интерфейс для просмотра прошлых сессий ревью (`localhost:5483`). | | `ocr version` | — | Выводит версию, коммит, платформу, дату сборки и URL GitHub. | @@ -481,6 +482,35 @@ id сессии. Без `-o` HTML выводится в stdout. | `--repo ` | текущий каталог | Репозиторий, сессия которого экспортируется. | | `--output `, `-o` | stdout | Записать HTML в файл вместо stdout. | +### `ocr session rm` + +Удаляет одну сохранённую сессию из `~/.opencodereview/sessions/`. + +```bash +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project +``` + +Идентификатора достаточно самого по себе, поэтому команда работает из любого +каталога. Если один и тот же идентификатор сохранён для нескольких репозиториев, +команда перечисляет кандидатов и ничего не удаляет; укажите нужный через `--repo`. + +Команда печатает репозиторий, ветку, время начала, число файлов и число +комментариев сессии и просит подтверждение. **Неинтерактивный stdin означает +«нет»**, поэтому в конвейере или в задании CI нужно передать `--yes` (`-y`), +чтобы пропустить запрос. + +Сессию с неразбираемыми метаданными удалить можно; ту, которую вообще не удаётся +прочитать, команда не удаляет, а сообщает об ошибке. С `--repo` сессия, которая +записала другой репозиторий или не записала никакого, отклоняется: удалите её +только по идентификатору. + +| Флаг | По умолчанию | Описание | +|---|---|---| +| `--repo ` | все репозитории | Искать сессию только в этом репозитории. | +| `--yes`, `-y` | `false` | Пропустить запрос подтверждения. | + ## `ocr rules` Проверка правил. Доступна ровно одна подкоманда: diff --git a/pages/src/content/docs/zh/cli-reference.md b/pages/src/content/docs/zh/cli-reference.md index be90e072..c74faff2 100644 --- a/pages/src/content/docs/zh/cli-reference.md +++ b/pages/src/content/docs/zh/cli-reference.md @@ -80,6 +80,7 @@ ocr review --commit HEAD | gh issue comment 123 --body-file - | `ocr session comments ` | `ocr sessions comments ` | 输出单个会话中记录的评审评论。 | | `ocr session compare ` | `ocr session diff ` | 对比两个会话的问题：新增、仍存在、已解决、未评审。 | | `ocr session export [id]` | — | 将单个会话导出为自包含的 HTML 文件。 | +| `ocr session rm ` | `ocr session delete `, `ocr session remove ` | 删除一个已保存的评审会话。 | | `ocr viewer` | — | 启动用于历史评审会话的本地 Web UI（`localhost:5483`）。 | | `ocr version` | — | 打印版本、commit、平台、构建日期与 GitHub URL。 | @@ -471,6 +472,30 @@ ocr session export 20250601-100000-abc123 -o review.html | `--repo ` | 当前目录 | 要导出会话的仓库。 | | `--output `、`-o` | 标准输出 | 将 HTML 写入文件而不是标准输出。 | +### `ocr session rm` + +从 `~/.opencodereview/sessions/` 中删除一个已保存的会话。 + +```bash +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --yes +ocr session rm 9f2c1b4a-7e35-4d61-b2f0-6c8a41d9e72b --repo ~/work/my-project +``` + +只要会话 id 就够了，因此在任何目录下都能运行。如果同一个 id 在多个仓库中都存在， +命令会列出候选项并且不删除任何内容；用 `--repo` 指定其中一个。 + +命令会打印该会话的仓库、分支、开始时间、文件数和评论数，并请你确认。**非交互式的 +stdin 一律视为「否」**，因此流水线或 CI 任务需要传入 `--yes`（`-y`）来跳过确认。 + +元数据无法解析的会话仍然可以删除；完全读不了的会话则会报错而不是删除。使用 +`--repo` 时，记录了其他仓库或没有记录仓库的会话会被拒绝：这种情况请只用 id 删除。 + +| 标志 | 默认值 | 说明 | +|---|---|---| +| `--repo ` | 所有仓库 | 只在该仓库下查找这个会话。 | +| `--yes`、`-y` | `false` | 跳过确认提示。 | + ## `ocr rules` 规则自查。只有一个子命令：