From deebd9487815bcf0d084833ca04fcd7ed1b4d31f Mon Sep 17 00:00:00 2001 From: Nitish Agarwal <1592163+nitishagar@users.noreply.github.com> Date: Sun, 26 Jul 2026 09:56:16 +0530 Subject: [PATCH 1/2] feat(dismiss): remember dismissed findings across review runs (#59) Add a per-repo, filesystem-based dismissal store (~/.opencodereview/dismissals/.json) that records SHA-256 fingerprints of findings a user dismissed. A review whose store exists automatically suppresses dismissed findings from its output via a read-side filter applied to the returned comment slice; a review with no store behaves byte-identically to today. The fingerprint is SHA256(path + start_line + end_line + normalizedContent), where normalization is TrimSpace + ToLower. Identity is exact-content by design (documented limitation: rephrased findings resurface); semantic identity is deferred as future work. New ocr dismiss subcommand manages the store: - add record a dismissal - list show recorded dismissals - remove remove a dismissal Loading is fail-safe: a missing file is the no-opt-in case (one stat, no read); a corrupt/unreadable file yields an empty store plus a warning and is left untouched. Saving is atomic via temp+rename at mode 0600. Storage lives in internal/session/dismissal.go (DismissalStore, DismissalFingerprint, DismissalFilter, LoadComments). The filter is wired into Agent.Run on the success path only (err == nil), so a failed review's partial output is never masked. No existing session JSONL, resume index, or collector output is mutated. The new session.LoadComments helper walks a session JSONL and returns the flattened comment list (existing LoadDetail exposes only counts), which dismiss add uses to resolve findings by index or path:line. --- cmd/opencodereview/dismiss_cmd.go | 360 +++++++++++++++++++ cmd/opencodereview/dismiss_cmd_test.go | 297 ++++++++++++++++ cmd/opencodereview/main.go | 5 + cmd/opencodereview/review_cmd.go | 32 ++ cmd/opencodereview/review_cmd_test.go | 70 ++++ internal/agent/agent.go | 13 + internal/agent/dismiss_test.go | 273 +++++++++++++++ internal/session/dismissal.go | 304 ++++++++++++++++ internal/session/dismissal_test.go | 461 +++++++++++++++++++++++++ internal/session/testing.go | 4 + 10 files changed, 1819 insertions(+) create mode 100644 cmd/opencodereview/dismiss_cmd.go create mode 100644 cmd/opencodereview/dismiss_cmd_test.go create mode 100644 internal/agent/dismiss_test.go create mode 100644 internal/session/dismissal.go create mode 100644 internal/session/dismissal_test.go diff --git a/cmd/opencodereview/dismiss_cmd.go b/cmd/opencodereview/dismiss_cmd.go new file mode 100644 index 00000000..2820a466 --- /dev/null +++ b/cmd/opencodereview/dismiss_cmd.go @@ -0,0 +1,360 @@ +package main + +import ( + "fmt" + "io" + "os" + "strconv" + "strings" + "text/tabwriter" + "time" + + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" +) + +// runDismiss dispatches the `ocr dismiss` subcommand, which records and +// manages dismissed findings (per-repo, under ~/.opencodereview/dismissals/). +// Dismissed findings are suppressed from `ocr review` output via the read-side +// DismissalFilter (see internal/agent). This command is the recording surface. +func runDismiss(args []string) error { + if len(args) == 0 { + printDismissUsage() + return nil + } + switch args[0] { + case "add": + return runDismissAdd(args[1:]) + case "list", "ls": + return runDismissList(args[1:]) + case "remove", "rm": + return runDismissRemove(args[1:]) + case "-h", "--help": + printDismissUsage() + return nil + default: + return fmt.Errorf("unknown dismiss sub-command: %s\nRun 'ocr dismiss -h' for usage", args[0]) + } +} + +// runDismissAdd records a dismissal: `ocr dismiss add [--repo DIR]`. +// is either a 0-based index into the session's flattened comment +// list, or a path:line form (`file.go:42`) that resolves to the first comment +// (in JSONL file-record order) whose Path matches and whose [StartLine,EndLine] +// contains the line. If multiple comments qualify, the command reports all +// candidates with their indices and asks the user to disambiguate via the index. +func runDismissAdd(args []string) error { + a := newOcrFlagSet("ocr dismiss add") + var repoDir string + a.StringVar(&repoDir, "repo", "", "root directory of the git repository (default: current dir)") + if err := a.Parse(args); err != nil { + return err + } + if a.showHelp { + printDismissAddUsage() + return nil + } + + rest := a.fs.Args() + if len(rest) < 2 { + printDismissAddUsage() + return fmt.Errorf("dismiss add requires a session ID and a finding reference") + } + sessionID := rest[0] + findingRef := rest[1] + + resolvedRepo, err := resolveWorkingDirForSession(repoDir) + if err != nil { + return err + } + + comments, err := session.LoadComments(resolvedRepo, sessionID) + if err != nil { + return fmt.Errorf("load session %q: %w", sessionID, err) + } + if len(comments) == 0 { + return fmt.Errorf("session %q has no review comments to dismiss", sessionID) + } + + target, err := resolveFindingRef(comments, findingRef) + if err != nil { + return err + } + + // Load the existing store. On corrupt/unreadable, LoadDismissals returns a + // non-nil store (for its path) alongside an error. We treat corruption as a + // hard error for `dismiss add` rather than silently wiping: the user must + // acknowledge it before recording new dismissals (D6 — file left untouched). + store, err := session.LoadDismissals(resolvedRepo) + if err != nil { + path := resolvedRepo + if store != nil { + path = store.Path() + } + return fmt.Errorf("load dismissal store %q: %w (the corrupt file was left untouched; inspect or remove it manually)", path, err) + } + + fp := session.DismissalFingerprint(target) + store.Record(session.DismissalEntry{ + Fingerprint: fp, + Path: target.Path, + ContentPreview: truncateForPreview(target.Content), + DismissedAt: time.Now().UTC().Format(time.RFC3339), + SourceSessionID: sessionID, + }) + if err := store.Save(); err != nil { + return fmt.Errorf("save dismissal store: %w", err) + } + fmt.Printf("Dismissed %s:%d-%d from session %s\n", target.Path, target.StartLine, target.EndLine, sessionID) + fmt.Printf("Fingerprint: %s\n", fp) + fmt.Println("Run 'ocr review' again to suppress this finding from future reviews.") + return nil +} + +// runDismissList prints the recorded dismissals for a repo as a table. +func runDismissList(args []string) error { + a := newOcrFlagSet("ocr dismiss list") + var repoDir string + a.StringVar(&repoDir, "repo", "", "root directory of the git repository (default: current dir)") + if err := a.Parse(args); err != nil { + return err + } + if a.showHelp { + printDismissListUsage() + return nil + } + + resolvedRepo, err := resolveWorkingDirForSession(repoDir) + if err != nil { + return err + } + store, err := session.LoadDismissals(resolvedRepo) + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: dismissal store corrupt (%v); showing no dismissals. The file was left untouched.\n", err) + } + entries := store.List() + if len(entries) == 0 { + fmt.Printf("No dismissed findings for %s\n", resolvedRepo) + return nil + } + printDismissTable(os.Stdout, entries) + return nil +} + +// runDismissRemove deletes a dismissal by fingerprint (or its list index). +func runDismissRemove(args []string) error { + a := newOcrFlagSet("ocr dismiss remove") + var repoDir string + a.StringVar(&repoDir, "repo", "", "root directory of the git repository (default: current dir)") + if err := a.Parse(args); err != nil { + return err + } + if a.showHelp { + printDismissRemoveUsage() + return nil + } + + rest := a.fs.Args() + if len(rest) == 0 { + printDismissRemoveUsage() + return fmt.Errorf("dismiss remove requires a fingerprint or list index") + } + ref := rest[0] + + resolvedRepo, err := resolveWorkingDirForSession(repoDir) + if err != nil { + return err + } + store, err := session.LoadDismissals(resolvedRepo) + if err != nil { + return fmt.Errorf("load dismissal store: %w", err) + } + + fp, err := resolveRemoveRef(store, ref) + if err != nil { + return err + } + if !store.Remove(fp) { + return fmt.Errorf("no dismissed finding matching %q", ref) + } + if err := store.Save(); err != nil { + return fmt.Errorf("save dismissal store: %w", err) + } + fmt.Printf("Removed dismissal %s\n", fp) + return nil +} + +// resolveFindingRef resolves a 0-based index or path:line reference to a +// comment in the flattened session comment list (JSONL file-record order). +func resolveFindingRef(comments []model.LlmComment, ref string) (model.LlmComment, error) { + // Index form: a non-negative integer. + if n, err := strconv.Atoi(ref); err == nil { + if n < 0 || n >= len(comments) { + return model.LlmComment{}, fmt.Errorf("finding index %d out of range (session has %d comments; valid 0..%d)", n, len(comments), len(comments)-1) + } + return comments[n], nil + } + // path:line form: first comment in file-record order whose Path matches and + // whose [StartLine, EndLine] contains the line. Multiple matches => report + // candidates with indices and ask the user to disambiguate via the index form. + path, line, err := parsePathLine(ref) + if err != nil { + return model.LlmComment{}, fmt.Errorf("finding reference must be a 0-based index or path:line (e.g. \"3\" or \"file.go:42\"): %v", err) + } + var matches []int + for i, c := range comments { + if c.Path == path && c.StartLine <= line && line <= c.EndLine { + matches = append(matches, i) + } + } + switch len(matches) { + case 0: + return model.LlmComment{}, fmt.Errorf("no finding in session matches %s:%d", path, line) + case 1: + return comments[matches[0]], nil + default: + var b strings.Builder + fmt.Fprintf(&b, "multiple findings in session match %s:%d — disambiguate using the index form:\n", path, line) + for _, idx := range matches { + c := comments[idx] + fmt.Fprintf(&b, " [%d] %s:%d-%d %s\n", idx, c.Path, c.StartLine, c.EndLine, truncateForPreview(c.Content)) + } + return model.LlmComment{}, fmt.Errorf("%s", b.String()) + } +} + +// resolveRemoveRef resolves a remove reference to a fingerprint. It accepts the +// full fingerprint, the short prefix (≥4 chars) shown by `dismiss list`, or a +// 0-based list index. Fingerprint forms are checked before the index form so an +// all-decimal hex prefix cannot be misread as an index. +func resolveRemoveRef(store *session.DismissalStore, ref string) (string, error) { + entries := store.List() + // Exact fingerprint match. + if store.Contains(ref) { + return ref, nil + } + // Short fingerprint prefix (first 12 chars, as printed by `dismiss list`). + if len(ref) >= 4 { + var match string + count := 0 + for _, e := range entries { + if strings.HasPrefix(e.Fingerprint, ref) { + match = e.Fingerprint + count++ + } + } + if count == 1 { + return match, nil + } + } + // List index form (checked last so a decimal-looking fingerprint prefix is + // never misread as an index). + if n, err := strconv.Atoi(ref); err == nil && n >= 0 && n < len(entries) { + return entries[n].Fingerprint, nil + } + return "", fmt.Errorf("no dismissed finding matching %q (use 'ocr dismiss list' to see fingerprints/indices)", ref) +} + +// parsePathLine splits a "file.go:42" reference into path and line. +func parsePathLine(ref string) (string, int, error) { + idx := strings.LastIndex(ref, ":") + if idx <= 0 || idx == len(ref)-1 { + return "", 0, fmt.Errorf("missing ':line' in %q", ref) + } + path := ref[:idx] + line, err := strconv.Atoi(ref[idx+1:]) + if err != nil || line < 1 { + return "", 0, fmt.Errorf("invalid line number in %q", ref) + } + return path, line, nil +} + +// truncateForPreview caps a comment body for storage/display, collapsing +// newlines/tabs to spaces first so the preview stays one line. +func truncateForPreview(s string) string { + const max = 160 + s = strings.ReplaceAll(strings.ReplaceAll(s, "\n", " "), "\t", " ") + runes := []rune(s) + if len(runes) <= max { + return s + } + return string(runes[:max-1]) + "…" +} + +func printDismissTable(w io.Writer, entries []session.DismissalEntry) { + tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) + fmt.Fprintln(tw, "INDEX\tFINGERPRINT\tPATH\tPREVIEW\tDISMISSED AT") + for i, e := range entries { + fmt.Fprintf(tw, "%d\t%s\t%s\t%s\t%s\n", i, shortFingerprint(e.Fingerprint), e.Path, truncateForPreview(e.ContentPreview), e.DismissedAt) + } + tw.Flush() +} + +func shortFingerprint(fp string) string { + if len(fp) > 12 { + return fp[:12] + } + return fp +} + +func printDismissUsage() { + fmt.Println(`Usage: + ocr dismiss [flags] + +Sub-commands: + add Record a dismissal of a finding from a session + list, ls List dismissed findings for the current repo + remove, rm Remove a recorded dismissal + + is a 0-based index into the session's flattened comment list, or a +path:line form (e.g. "file.go:42") that resolves to the first comment whose line +range contains the line. + +Flags apply to all sub-commands: + --repo string Root directory of the git repository (default: current dir) + +Use "ocr dismiss add -h", "ocr dismiss list -h", or "ocr dismiss remove -h" for details.`) +} + +func printDismissAddUsage() { + fmt.Println(`Usage: + ocr dismiss add [flags] + +Record a dismissal for a single finding from a review session. The finding's +fingerprint (SHA256 of path + line range + normalized content) is stored under +~/.opencodereview/dismissals/.json; subsequent 'ocr review' runs +suppress findings whose fingerprint matches. + +Arguments: + A session id from 'ocr session list'. + A 0-based index into the session's flattened comment list, or + a path:line form (e.g. "file.go:42") resolving to the first + comment whose [start,end] line range contains the line. + +Flags: + --repo string Root directory of the git repository (default: current dir)`) +} + +func printDismissListUsage() { + fmt.Println(`Usage: + ocr dismiss list [flags] + ocr dismiss ls [flags] + +List dismissed findings recorded for the repository, ordered by dismissal time. + +Flags: + --repo string Root directory of the git repository (default: current dir)`) +} + +func printDismissRemoveUsage() { + fmt.Println(`Usage: + ocr dismiss remove [flags] + ocr dismiss rm [flags] + +Remove a recorded dismissal so the finding reappears in future reviews. The +reference may be the full fingerprint, the 12-char prefix shown by 'dismiss +list', or the list index printed in the INDEX column. + +Flags: + --repo string Root directory of the git repository (default: current dir)`) +} diff --git a/cmd/opencodereview/dismiss_cmd_test.go b/cmd/opencodereview/dismiss_cmd_test.go new file mode 100644 index 00000000..9b1d1d0c --- /dev/null +++ b/cmd/opencodereview/dismiss_cmd_test.go @@ -0,0 +1,297 @@ +package main + +import ( + "bytes" + "os" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" +) + +// dismissTestSetup isolates the per-repo dismissal store under a temp HOME and +// returns the repo dir + a session id with one recorded comment to dismiss. +func dismissTestSetup(t *testing.T, comments []model.LlmComment) (repoDir, sessionID string) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + repoDir = t.TempDir() + sh := session.New(repoDir, "main", "test-model", session.SessionOptions{ + ReviewMode: session.ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + }) + sh.RecordReviewItemDone("a.go", "a.go", "a.go", "fp-a", comments) + sh.Finalize() + return repoDir, sh.SessionID +} + +func TestDismissAddListRemove(t *testing.T) { + comments := []model.LlmComment{ + {Path: "a.go", StartLine: 1, EndLine: 2, Content: "fix the bug"}, + } + repoDir, sessionID := dismissTestSetup(t, comments) + + // add: record the dismissal via 0-based index. + out := captureStdout(t, func() { + if err := runDismiss([]string{"add", "--repo", repoDir, sessionID, "0"}); err != nil { + t.Fatalf("dismiss add: %v", err) + } + }) + fp := session.DismissalFingerprint(comments[0]) + if !strings.Contains(out, fp) { + t.Errorf("dismiss add output missing fingerprint %q:\n%s", fp, out) + } + + // list: the dismissal appears with the short fingerprint. + listOut := captureStdout(t, func() { + if err := runDismiss([]string{"list", "--repo", repoDir}); err != nil { + t.Fatalf("dismiss list: %v", err) + } + }) + if !strings.Contains(listOut, "INDEX") || !strings.Contains(listOut, "a.go") { + t.Errorf("dismiss list output missing table/a.go:\n%s", listOut) + } + + // remove by short fingerprint prefix (first 12 chars, as list shows). + short := fp[:12] + if err := runDismiss([]string{"remove", "--repo", repoDir, short}); err != nil { + t.Fatalf("dismiss remove: %v", err) + } + + // list is now empty. + emptyOut := captureStdout(t, func() { + if err := runDismiss([]string{"list", "--repo", repoDir}); err != nil { + t.Fatalf("dismiss list (2): %v", err) + } + }) + if !strings.Contains(emptyOut, "No dismissed findings") { + t.Errorf("expected empty-list message after remove, got:\n%s", emptyOut) + } +} + +func TestDismissAddByIdempotent(t *testing.T) { + comments := []model.LlmComment{ + {Path: "a.go", StartLine: 1, EndLine: 1, Content: "dup"}, + } + repoDir, sessionID := dismissTestSetup(t, comments) + for i := 0; i < 2; i++ { + if err := runDismiss([]string{"add", "--repo", repoDir, sessionID, "0"}); err != nil { + t.Fatalf("dismiss add #%d: %v", i+1, err) + } + } + store, err := session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + if got := len(store.List()); got != 1 { + t.Errorf("after re-add, store has %d entries, want 1 (idempotent)", got) + } +} + +func TestDismissAddPathLineRef(t *testing.T) { + comments := []model.LlmComment{ + {Path: "a.go", StartLine: 10, EndLine: 20, Content: "in range"}, + } + repoDir, sessionID := dismissTestSetup(t, comments) + // Resolve by path:line where line is within [10,20]. + if err := runDismiss([]string{"add", "--repo", repoDir, sessionID, "a.go:15"}); err != nil { + t.Fatalf("dismiss add path:line: %v", err) + } + store, err := session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + if !store.Contains(session.DismissalFingerprint(comments[0])) { + t.Error("path:line ref did not record the matching comment's fingerprint") + } +} + +func TestDismissAddPathLineAmbiguous(t *testing.T) { + // Two overlapping comments qualify for a.go:15 -> disambiguation error. + comments := []model.LlmComment{ + {Path: "a.go", StartLine: 10, EndLine: 20, Content: "first"}, + {Path: "a.go", StartLine: 12, EndLine: 18, Content: "second"}, + } + repoDir, sessionID := dismissTestSetup(t, comments) + err := runDismiss([]string{"add", "--repo", repoDir, sessionID, "a.go:15"}) + if err == nil { + t.Fatal("expected disambiguation error for overlapping comments") + } + if !strings.Contains(err.Error(), "multiple findings") { + t.Fatalf("expected 'multiple findings' error, got: %v", err) + } + // Nothing should have been recorded. + store, err := session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + if got := len(store.List()); got != 0 { + t.Errorf("ambiguous add recorded %d entries, want 0", got) + } +} + +func TestDismissAddIndexOutOfRange(t *testing.T) { + comments := []model.LlmComment{{Path: "a.go", StartLine: 1, EndLine: 1, Content: "x"}} + repoDir, sessionID := dismissTestSetup(t, comments) + err := runDismiss([]string{"add", "--repo", repoDir, sessionID, "9"}) + if err == nil { + t.Fatal("expected out-of-range error") + } + if !strings.Contains(err.Error(), "out of range") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDismissAddCorruptStoreFails(t *testing.T) { + // D6: a corrupt store must not be silently wiped by `dismiss add`. + comments := []model.LlmComment{{Path: "a.go", StartLine: 1, EndLine: 1, Content: "x"}} + repoDir, sessionID := dismissTestSetup(t, comments) + path, err := session.DismissalFilePath(repoDir) + if err != nil { + t.Fatalf("DismissalFilePath: %v", err) + } + garbage := []byte("{ not json") + if err := os.MkdirAll(dirOf(path), 0700); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + if err := os.WriteFile(path, garbage, 0600); err != nil { + t.Fatalf("write corrupt store: %v", err) + } + + err = runDismiss([]string{"add", "--repo", repoDir, sessionID, "0"}) + if err == nil { + t.Fatal("expected error when store is corrupt") + } + if !strings.Contains(err.Error(), "corrupt") && !strings.Contains(err.Error(), "left untouched") { + t.Fatalf("expected corruption-related error, got: %v", err) + } + // The corrupt file is untouched. + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read after: %v", err) + } + if !bytes.Equal(after, garbage) { + t.Errorf("corrupt store was modified by dismiss add:\n before=%q\n after=%q", garbage, after) + } +} + +func TestDismissRemoveNotFound(t *testing.T) { + comments := []model.LlmComment{{Path: "a.go", StartLine: 1, EndLine: 1, Content: "x"}} + repoDir, _ := dismissTestSetup(t, comments) + err := runDismiss([]string{"remove", "--repo", repoDir, "deadbeefdead"}) + if err == nil { + t.Fatal("expected not-found error") + } + if !strings.Contains(err.Error(), "no dismissed finding") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDismissListCorruptStoreWarnsAndShowsEmpty(t *testing.T) { + // D6: `dismiss list` on a corrupt store warns and shows empty, does not crash. + repoDir := t.TempDir() + t.Setenv("HOME", t.TempDir()) + path, err := session.DismissalFilePath(repoDir) + if err != nil { + t.Fatalf("DismissalFilePath: %v", err) + } + if err := os.MkdirAll(dirOf(path), 0700); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + if err := os.WriteFile(path, []byte("{ broken"), 0600); err != nil { + t.Fatalf("write corrupt store: %v", err) + } + out := captureStdout(t, func() { + if err := runDismiss([]string{"list", "--repo", repoDir}); err != nil { + t.Fatalf("dismiss list on corrupt store errored: %v", err) + } + }) + if !strings.Contains(out, "No dismissed findings") { + t.Errorf("expected empty-list message on corrupt store, got:\n%s", out) + } +} + +func TestRunDismissUsage(t *testing.T) { + out := captureStdout(t, func() { + if err := runDismiss(nil); err != nil { + t.Fatalf("runDismiss: %v", err) + } + }) + if !strings.Contains(out, "add") || !strings.Contains(out, "list") || !strings.Contains(out, "remove") { + t.Errorf("usage missing subcommands:\n%s", out) + } +} + +func TestRunDismissUnknownSubcommand(t *testing.T) { + err := runDismiss([]string{"bogus"}) + if err == nil { + t.Fatal("expected error for unknown subcommand") + } + if !strings.Contains(err.Error(), "unknown dismiss sub-command") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestParsePathLine(t *testing.T) { + tests := []struct { + ref string + wantP string + wantL int + wantErr bool + }{ + {"a.go:42", "a.go", 42, false}, + {"dir/a.go:1", "dir/a.go", 1, false}, + {"a.go", "", 0, true}, + {"a.go:", "", 0, true}, + {":42", "", 0, true}, + {"a.go:0", "", 0, true}, + {"a.go:abc", "", 0, true}, + } + for _, tt := range tests { + gotP, gotL, err := parsePathLine(tt.ref) + if tt.wantErr { + if err == nil { + t.Errorf("parsePathLine(%q): expected error, got none", tt.ref) + } + continue + } + if err != nil { + t.Errorf("parsePathLine(%q): unexpected error: %v", tt.ref, err) + continue + } + if gotP != tt.wantP || gotL != tt.wantL { + t.Errorf("parsePathLine(%q) = (%q,%d), want (%q,%d)", tt.ref, gotP, gotL, tt.wantP, tt.wantL) + } + } +} + +func TestTruncateForPreview(t *testing.T) { + short := "hello world" + if got := truncateForPreview(short); got != short { + t.Errorf("truncate short = %q, want %q", got, short) + } + // Newlines/tabs collapse. + if got := truncateForPreview("a\nb\tc"); got != "a b c" { + t.Errorf("truncate whitespace = %q, want %q", got, "a b c") + } + // Long content truncated with ellipsis. + long := strings.Repeat("x", 300) + got := truncateForPreview(long) + if !strings.HasSuffix(got, "…") { + t.Errorf("truncate long missing ellipsis: %q", got[len(got)-5:]) + } + if len([]rune(got)) != 160 { + t.Errorf("truncate long length = %d runes, want 160", len([]rune(got))) + } +} + +// dirOf returns the directory portion of a path (filepath.Dir wrapper kept +// local to avoid an extra import dance in the test body). +func dirOf(p string) string { + i := strings.LastIndexByte(p, '/') + if i < 0 { + return "." + } + return p[:i] +} diff --git a/cmd/opencodereview/main.go b/cmd/opencodereview/main.go index 40f10504..c04cb96b 100644 --- a/cmd/opencodereview/main.go +++ b/cmd/opencodereview/main.go @@ -60,6 +60,8 @@ func dispatch() error { return runDelegate(args[1:]) case "session", "sessions": return runSession(args[1:]) + case "dismiss": + return runDismiss(args[1:]) case "-h", "--help": printTopLevelUsage() return nil @@ -83,6 +85,7 @@ Commands: llm LLM utility commands viewer Start the WebUI session viewer session, sessions List and inspect saved review sessions + dismiss Record and manage dismissed findings (suppress from review) version Show version information Examples: @@ -96,6 +99,7 @@ Examples: ocr llm test Test LLM connectivity ocr llm providers List built-in providers ocr session list List saved review sessions + ocr dismiss add Dismiss a finding from a session ocr version Show version info Use "ocr review -h" for more information about review. @@ -104,6 +108,7 @@ Use "ocr rules -h" for more information about rules. Use "ocr config" for more information about config. Use "ocr llm" for more information about LLM utilities. Use "ocr session -h" for more information about session inspection. +Use "ocr dismiss -h" for more information about managing dismissed findings. GitHub: https://github.com/alibaba/open-code-review`) } diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 8cf53275..6131d716 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -70,6 +70,11 @@ func runReview(args []string) error { return err } + // Dismissal filter: loaded only when the per-repo dismissal store exists. + // Absence → nil (D2: byte-identical to the stateless default; one stat, no read). + // Corrupt/unreadable → nil + warning (D6: fail safe, file left untouched). + dismissalFilter := loadDismissalFilter(cc.RepoDir) + rt, err := loadLLMRuntime(cc.Template, opts.toolConfigPath, opts.model) if err != nil { return err @@ -119,6 +124,7 @@ func runReview(args []string) error { Background: opts.background, GitRunner: cc.GitRunner, Resume: resumeState, + Dismissals: dismissalFilter, }) // Silence progress output during execution; restored before the trace @@ -190,6 +196,32 @@ func reviewModeFromOptions(opts reviewOptions) string { return session.ReviewModeWorkspace } +// loadDismissalFilter loads the per-repo dismissal filter for review suppression. +// +// It performs a single stat of the dismissal store path and returns nil when the +// store does not exist (D2: byte-identical to the stateless default — no read, +// no allocation). When the store exists but is corrupt or unreadable, it prints +// a warning to stderr and returns nil so the review proceeds stateless (D6/AS5: +// fail safe; the corrupt file is left untouched). On a successful load it returns +// a DismissalFilter built from the store's fingerprints. +func loadDismissalFilter(repoDir string) *session.DismissalFilter { + path, err := session.DismissalFilePath(repoDir) + if err != nil { + // Could not resolve home dir; there is no store to consult. Stay stateless. + return nil + } + if _, err := os.Stat(path); err != nil { + // Missing (or otherwise unreadable): no opt-in. One stat, no read. + return nil + } + store, err := session.LoadDismissals(repoDir) + if err != nil { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: dismissal store %q is corrupt or unreadable (%v); proceeding without suppression. The file was left untouched.\n", path, err) + return nil + } + return session.NewDismissalFilter(store) +} + // resolveRepoDir resolves the repo dir for `ocr rules check`. It delegates to // resolveWorkingDir(requireGit=true) so it anchors at the git top-level just // like the review path — keeping rule resolution consistent when run from a diff --git a/cmd/opencodereview/review_cmd_test.go b/cmd/opencodereview/review_cmd_test.go index eaad1bfc..750cf05b 100644 --- a/cmd/opencodereview/review_cmd_test.go +++ b/cmd/opencodereview/review_cmd_test.go @@ -1,8 +1,13 @@ package main import ( + "os" + "path/filepath" "strings" "testing" + + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" ) func TestValidateReviewRefsRejectsOptionLikeCommit(t *testing.T) { @@ -54,3 +59,68 @@ func TestParseReviewFlagsAllowsFromAndTo(t *testing.T) { t.Fatalf("unexpected opts: from=%q to=%q", opts.from, opts.to) } } + +// TestLoadDismissalFilterReturnsNilWhenAbsent verifies D2: when no dismissal +// store exists, loadDismissalFilter returns nil (one stat, no read) so the +// review is byte-identical to the stateless default. +func TestLoadDismissalFilterReturnsNilWhenAbsent(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + if f := loadDismissalFilter(repoDir); f != nil { + t.Errorf("loadDismissalFilter with no store returned non-nil: %v", f) + } +} + +// TestLoadDismissalFilterReturnsFilterWhenStoreExists verifies that a present, +// valid store produces a non-nil filter that suppresses the recorded finding. +func TestLoadDismissalFilterReturnsFilterWhenStoreExists(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + target := model.LlmComment{Path: "a.go", StartLine: 1, EndLine: 2, Content: "bug"} + store, err := session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + store.Record(session.DismissalEntry{Fingerprint: session.DismissalFingerprint(target)}) + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + + f := loadDismissalFilter(repoDir) + if f == nil { + t.Fatal("loadDismissalFilter returned nil despite a valid store existing") + } + out := f.Suppress([]model.LlmComment{target, {Path: "a.go", StartLine: 9, EndLine: 9, Content: "other"}}) + if len(out) != 1 || out[0].Content != "other" { + t.Errorf("filter did not suppress the recorded finding: %+v", out) + } +} + +// TestLoadDismissalFilterCorruptReturnsNil verifies D6/AS5: a corrupt store +// makes loadDismissalFilter print a warning and return nil (proceed stateless), +// leaving the file untouched. +func TestLoadDismissalFilterCorruptReturnsNil(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + path, err := session.DismissalFilePath(repoDir) + if err != nil { + t.Fatalf("DismissalFilePath: %v", err) + } + garbage := []byte("{ broken json") + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + if err := os.WriteFile(path, garbage, 0600); err != nil { + t.Fatalf("write corrupt store: %v", err) + } + if f := loadDismissalFilter(repoDir); f != nil { + t.Errorf("loadDismissalFilter with corrupt store returned non-nil: %v", f) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read after: %v", err) + } + if string(after) != string(garbage) { + t.Errorf("corrupt store was modified by loadDismissalFilter") + } +} diff --git a/internal/agent/agent.go b/internal/agent/agent.go index cd797e63..d0499c20 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -117,6 +117,12 @@ type Args struct { // Resume is an optional read-only checkpoint index from a previous review session. Resume *session.ResumeState + + // Dismissals is an optional read-side filter of dismissed findings. + // When non-nil, findings whose dismissal fingerprint is in the store are + // suppressed from the returned comments; nil = stateless (today's behavior), + // so a review with no dismissal store on disk is byte-identical to before. + Dismissals *session.DismissalFilter } // Agent orchestrates the AI-powered code review. LLM tool-use loop / memory @@ -228,6 +234,13 @@ func (a *Agent) Run(ctx context.Context) ([]model.LlmComment, error) { // Step 2: Dispatch per-file subtasks concurrently comments, err := a.dispatchSubtasks(ctx) + // Suppress dismissed findings on the success path only: when err != nil, + // dispatchSubtasks returns a partial/nil slice and masking it would hide a + // failure. Suppress returns a new slice (D3: input untouched) and is a no-op + // when Dismissals is nil (D2: byte-identical to the stateless default). + if err == nil && a.args.Dismissals != nil { + comments = a.args.Dismissals.Suppress(comments) + } if len(comments) > 0 { telemetry.RecordCommentsGenerated(ctx, int64(len(comments))) } diff --git a/internal/agent/dismiss_test.go b/internal/agent/dismiss_test.go new file mode 100644 index 00000000..cab71566 --- /dev/null +++ b/internal/agent/dismiss_test.go @@ -0,0 +1,273 @@ +package agent + +import ( + "context" + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/config/template" + "github.com/open-code-review/open-code-review/internal/llm" + "github.com/open-code-review/open-code-review/internal/model" + "github.com/open-code-review/open-code-review/internal/session" + "github.com/open-code-review/open-code-review/internal/tool" +) + +// readSessionJSONL reads the JSONL for a session id from the test-dismissals +// dir under HOME. Comparison is over the comment-bearing records only; we trim +// volatile fields (uuid, parentUuid, timestamp, sessionId) so two runs of the +// same review produce comparable bytes. +func readSessionJSONL(t *testing.T, repoDir, sessionID string) string { + t.Helper() + path, err := session.SessionFilePath(repoDir, sessionID) + if err != nil { + t.Fatalf("SessionFilePath: %v", err) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read session: %v", err) + } + var out strings.Builder + for _, line := range strings.Split(strings.TrimRight(string(raw), "\n"), "\n") { + trimmed := stripVolatileFields(line) + if trimmed != "" { + out.WriteString(trimmed) + out.WriteByte('\n') + } + } + return out.String() +} + +// initGitWorkspaceRepo creates a temp git repo with one committed file and an +// uncommitted modification, so a workspace-mode review has exactly one diff. +func initGitWorkspaceRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v: %s", args, out) + } + } + run("git", "init") + run("git", "config", "user.email", "test@test.com") + run("git", "config", "user.name", "Test") + if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n"), 0o644); err != nil { + t.Fatalf("write main.go: %v", err) + } + run("git", "add", ".") + run("git", "commit", "-m", "initial") + // Uncommitted change -> workspace diff. + if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n\nfunc main() {}\n"), 0o644); err != nil { + t.Fatalf("modify main.go: %v", err) + } + return dir +} + +// codeCommentsResponse emits a code_comment tool call whose comments slice is +// the provided set. Mirrors codeCommentResponse but with multiple comments and +// explicit path/content so fingerprints are deterministic. +func codeCommentsResponse(path string, comments []model.LlmComment) *llm.ChatResponse { + content := "" + raw := make([]map[string]any, len(comments)) + for i, c := range comments { + raw[i] = map[string]any{ + "content": c.Content, + "path": firstNonEmpty(c.Path, path), + } + } + args, _ := json.Marshal(map[string]any{"path": path, "comments": raw}) + return &llm.ChatResponse{ + Choices: []llm.Choice{{ + Message: llm.ResponseMessage{ + Content: &content, + ToolCalls: []llm.ToolCall{{ + ID: "call_comment", + Type: "function", + Function: llm.FunctionCall{ + Name: "code_comment", + Arguments: string(args), + }, + }}, + }, + }}, + Model: "fake", + Usage: &llm.UsageInfo{PromptTokens: 50, CompletionTokens: 20}, + } +} + +func firstNonEmpty(a, b string) string { + if a != "" { + return a + } + return b +} + +// stripVolatileFields removes run-to-run-volatile keys (uuid/parentUuid/ +// timestamp/sessionId/cwd/duration_seconds) from a JSONL record so two runs of +// the same review can be compared on their comment-bearing content (D3/AS4). +func stripVolatileFields(line string) string { + var rec map[string]any + if err := json.Unmarshal([]byte(line), &rec); err != nil { + return line + } + for _, k := range []string{"uuid", "parentUuid", "timestamp", "sessionId", "cwd", "duration_seconds"} { + delete(rec, k) + } + b, err := json.Marshal(rec) + if err != nil { + return line + } + return string(b) +} + +// buildDismissTestAgent constructs an Agent over repoDir with a fake LLM that +// emits the given comments for the single workspace diff. Mirrors the +// scaffolding in TestDispatchSubtasks_WithFakeLLM but with a real repo so Run +// can load diffs. +func buildDismissTestAgent(t *testing.T, repoDir string, emitted []model.LlmComment, filter *session.DismissalFilter) *Agent { + t.Helper() + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + + client := &fakeAgentClient{responses: []*llm.ChatResponse{ + codeCommentsResponse("main.go", emitted), + agentTaskDoneResponse(), + }} + a := New(Args{ + RepoDir: repoDir, + LLMClient: client, + Model: "fake", + CommentCollector: collector, + Tools: reg, + Dismissals: filter, + Template: template.Template{ + MaxTokens: 100000, + MaxToolRequestTimes: 10, + MainTask: template.LlmConversation{ + Messages: []template.ChatMessage{ + {Role: "user", Content: "Review {{diff}} for {{current_file_path}}"}, + }, + }, + }, + MainToolDefs: []llm.ToolDef{ + {Type: "function", Function: llm.FunctionDef{Name: "task_done", Description: "done"}}, + {Type: "function", Function: llm.FunctionDef{Name: "code_comment", Description: "comment"}}, + }, + }) + return a +} + +// TestAgentRunSuppressesDismissed runs the full Agent.Run pipeline against a +// temp repo. The fake LLM emits two findings (D1/D2/D3): with a filter that +// dismisses the first, Run must return only the second; with a nil filter, Run +// must return both unchanged (byte-identical to the stateless default). +func TestAgentRunSuppressesDismissed(t *testing.T) { + emitted := []model.LlmComment{ + {Path: "main.go", Content: "dismiss me", StartLine: 0, EndLine: 0}, + {Path: "main.go", Content: "keep me", StartLine: 0, EndLine: 0}, + } + + t.Run("filter suppresses dismissed finding", func(t *testing.T) { + repo := initGitWorkspaceRepo(t) + // Build a filter dismissing the first emitted comment's fingerprint. + store, err := session.LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + store.Record(session.DismissalEntry{Fingerprint: session.DismissalFingerprint(emitted[0])}) + filter := session.NewDismissalFilter(store) + + a := buildDismissTestAgent(t, repo, emitted, filter) + got, err := a.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 comment (dismissed one suppressed), got %d: %+v", len(got), got) + } + if got[0].Content != "keep me" { + t.Errorf("surviving comment = %q, want %q", got[0].Content, "keep me") + } + }) + + t.Run("nil filter is byte-identical (stateless default)", func(t *testing.T) { + repo := initGitWorkspaceRepo(t) + a := buildDismissTestAgent(t, repo, emitted, nil) + got, err := a.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 comments with nil filter (no suppression), got %d: %+v", len(got), got) + } + }) +} + +// TestAgentRunEmptyFilterSuppressesNothing guards the empty-set fast path: an +// empty (but non-nil) store must not drop any findings. +func TestAgentRunEmptyFilterSuppressesNothing(t *testing.T) { + repo := initGitWorkspaceRepo(t) + emitted := []model.LlmComment{{Path: "main.go", Content: "keep", StartLine: 0, EndLine: 0}} + store, err := session.LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + filter := session.NewDismissalFilter(store) // empty set + a := buildDismissTestAgent(t, repo, emitted, filter) + got, err := a.Run(context.Background()) + if err != nil { + t.Fatalf("Run: %v", err) + } + if len(got) != 1 { + t.Fatalf("empty filter dropped comments: got %d, want 1", len(got)) + } +} + +// TestAgentRunSessionJSONLUnchangedWithDismissalStore verifies D3/AS4: the +// recorded session JSONL is byte-identical whether or not a dismissal filter is +// applied (suppression is read-side only; the collector and session writes are +// untouched). +func TestAgentRunSessionJSONLUnchangedWithDismissalStore(t *testing.T) { + emitted := []model.LlmComment{ + {Path: "main.go", Content: "dismiss me", StartLine: 0, EndLine: 0}, + {Path: "main.go", Content: "keep me", StartLine: 0, EndLine: 0}, + } + + // Run 1: nil filter (stateless default). + repoNoFilter := initGitWorkspaceRepo(t) + a1 := buildDismissTestAgent(t, repoNoFilter, emitted, nil) + if _, err := a1.Run(context.Background()); err != nil { + t.Fatalf("Run (no filter): %v", err) + } + noFilterJSONL := readSessionJSONL(t, repoNoFilter, a1.SessionID()) + + // Run 2: active dismissal filter. + repoWithFilter := initGitWorkspaceRepo(t) + store, err := session.LoadDismissals(repoWithFilter) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + store.Record(session.DismissalEntry{Fingerprint: session.DismissalFingerprint(emitted[0])}) + a2 := buildDismissTestAgent(t, repoWithFilter, emitted, session.NewDismissalFilter(store)) + if _, err := a2.Run(context.Background()); err != nil { + t.Fatalf("Run (with filter): %v", err) + } + withFilterJSONL := readSessionJSONL(t, repoWithFilter, a2.SessionID()) + + // The comments recorded in the JSONL must be identical: the dismissal + // filter suppresses only the returned slice, not what the collector wrote. + if noFilterJSONL != withFilterJSONL { + t.Errorf("session JSONL differs with dismissal store present (D3/AS4 violation):\n--- no filter ---\n%s\n--- with filter ---\n%s", noFilterJSONL, withFilterJSONL) + } + // And specifically: the dismissed comment must still be present in the JSONL + // (suppression is not destructive to the recorded record). + if !strings.Contains(withFilterJSONL, "dismiss me") { + t.Errorf("dismissed finding absent from session JSONL (D3: suppression must not remove recorded findings): %s", withFilterJSONL) + } +} diff --git a/internal/session/dismissal.go b/internal/session/dismissal.go new file mode 100644 index 00000000..ee607976 --- /dev/null +++ b/internal/session/dismissal.go @@ -0,0 +1,304 @@ +package session + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/open-code-review/open-code-review/internal/model" +) + +// dismissalSubDir is the per-repo dismissal store directory, a sibling of +// sessionSubDir under the ~/.opencodereview root. Mirrored to a test dir by +// UseTestSessions so dismissal tests stay offline and hermetic. +var dismissalSubDir = "dismissals" + +// DismissalEntry records one dismissed finding for human inspection +// (ocr dismiss list) and cross-run traceability. Suppression keys only on +// Fingerprint; the remaining fields are descriptive. +type DismissalEntry struct { + Fingerprint string `json:"fingerprint"` + Path string `json:"path"` + ContentPreview string `json:"content_preview"` + DismissedAt string `json:"dismissed_at"` + SourceSessionID string `json:"source_session_id"` +} + +// DismissalFile is the on-disk JSON document for a per-repo dismissal store. +// Version is serialized as 1; future schema changes (e.g. semantic identity) +// bump this so old stores migrate rather than break. +type DismissalFile struct { + Version int `json:"version"` + Repo string `json:"repo"` + Dismissals []DismissalEntry `json:"dismissals"` +} + +// DismissalStore is a per-repo, filesystem-backed record of dismissed findings. +// +// LoadDismissals opens the store; Save persists changes. Contains performs an +// O(1) membership check by fingerprint. The store holds an in-memory map keyed +// by Fingerprint so the suppression check during a review is a single map lookup. +type DismissalStore struct { + repoDir string + path string + entries map[string]DismissalEntry + dirty bool +} + +// newDismissalStore resolves the on-disk path for a repo's dismissal store +// without touching disk. It mirrors the session substrate (encodeRepoPath + +// the ~/.opencodereview root) so a repo's dismissals and sessions never collide. +func newDismissalStore(repoDir string) (*DismissalStore, error) { + path, err := dismissalPath(repoDir) + if err != nil { + return nil, err + } + return &DismissalStore{ + repoDir: repoDir, + path: path, + entries: make(map[string]DismissalEntry), + }, nil +} + +// DismissalFilePath returns the on-disk path of a repo's dismissal store without +// touching disk. It is the cheap existence-check entry point used by the review +// path (one stat, no read when absent — D2). Missing home dir is an error. +func DismissalFilePath(repoDir string) (string, error) { + return dismissalPath(repoDir) +} + +func dismissalPath(repoDir string) (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolve home dir: %w", err) + } + return filepath.Join(home, ".opencodereview", dismissalSubDir, encodeRepoPath(repoDir)+".json"), nil +} + +// LoadDismissals reads the per-repo dismissal store. +// +// Missing file → empty store, nil error (the common no-opt-in case; no warning). +// Unreadable or unparseable → empty store, non-nil error (caller logs a warning +// and proceeds stateless); the file is left untouched (D6/AS5: do not overwrite a +// corrupt store, which would destroy evidence of how it got corrupt). +func LoadDismissals(repoDir string) (*DismissalStore, error) { + store, err := newDismissalStore(repoDir) + if err != nil { + return nil, err + } + data, err := os.ReadFile(store.path) + if err != nil { + if os.IsNotExist(err) { + // No opt-in: stateless behavior. Return an empty store so callers can + // still Record/Save into it if they want (e.g. ocr dismiss add). + return store, nil + } + return store, fmt.Errorf("read dismissal store %q: %w", store.path, err) + } + var file DismissalFile + if err := json.Unmarshal(data, &file); err != nil { + // Corrupt: fail safe. Return an empty store + error; leave the file on disk + // untouched (D6/AS5). Callers print a warning and proceed stateless. + return store, fmt.Errorf("parse dismissal store %q: %w", store.path, err) + } + for _, e := range file.Dismissals { + // Last-write-wins on duplicate fingerprints within the file; canonical map. + store.entries[e.Fingerprint] = e + } + return store, nil +} + +// Path returns the resolved on-disk path of the store (for warnings/errors). +func (s *DismissalStore) Path() string { return s.path } + +// Contains reports whether a fingerprint is recorded as dismissed. +func (s *DismissalStore) Contains(fp string) bool { + _, ok := s.entries[fp] + return ok +} + +// Record adds or replaces a dismissal entry by fingerprint and marks the store +// dirty. Re-recording the same fingerprint is idempotent (AS3): the entry is +// updated in place, but Save produces one record, not duplicates. +func (s *DismissalStore) Record(entry DismissalEntry) { + s.entries[entry.Fingerprint] = entry + s.dirty = true +} + +// Remove deletes a dismissal entry by fingerprint. It returns true if an entry +// was present. The store is marked dirty only when something was removed. +func (s *DismissalStore) Remove(fp string) bool { + if _, ok := s.entries[fp]; !ok { + return false + } + delete(s.entries, fp) + s.dirty = true + return true +} + +// List returns the recorded dismissals sorted by DismissedAt (then Fingerprint) +// for stable output in `ocr dismiss list`. +func (s *DismissalStore) List() []DismissalEntry { + out := make([]DismissalEntry, 0, len(s.entries)) + for _, e := range s.entries { + out = append(out, e) + } + sort.Slice(out, func(i, j int) bool { + if out[i].DismissedAt != out[j].DismissedAt { + return out[i].DismissedAt < out[j].DismissedAt + } + return out[i].Fingerprint < out[j].Fingerprint + }) + return out +} + +// Save persists the store to disk if it has changed since load. +// +// It is a no-op (returns nil) when the store is not dirty. Otherwise it writes +// to a temp file in the same directory and atomically renames it over the +// target (atomic on POSIX and Windows for same-directory renames), guarding +// against torn reads/writes on crash (A7). Mode 0600 matches session JSONL so +// dismissal content (which may quote code) is not world-readable. +func (s *DismissalStore) Save() error { + if !s.dirty { + return nil + } + file := DismissalFile{ + Version: 1, + Repo: s.repoDir, + Dismissals: s.List(), + } + data, err := json.MarshalIndent(file, "", " ") + if err != nil { + return fmt.Errorf("marshal dismissal store: %w", err) + } + dir := filepath.Dir(s.path) + if err := os.MkdirAll(dir, 0700); err != nil { + return fmt.Errorf("create dismissal dir: %w", err) + } + tmp, err := os.CreateTemp(dir, ".dismissal-*.tmp") + if err != nil { + return fmt.Errorf("create dismissal temp file: %w", err) + } + tmpName := tmp.Name() + // Clean up the temp file if any step below fails; rename takes ownership on success. + cleanup := func() { os.Remove(tmpName) } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + cleanup() + return fmt.Errorf("write dismissal temp file: %w", err) + } + if err := tmp.Close(); err != nil { + cleanup() + return fmt.Errorf("close dismissal temp file: %w", err) + } + if err := os.Chmod(tmpName, 0600); err != nil { + cleanup() + return fmt.Errorf("chmod dismissal temp file: %w", err) + } + if err := os.Rename(tmpName, s.path); err != nil { + cleanup() + return fmt.Errorf("rename dismissal temp file: %w", err) + } + s.dirty = false + return nil +} + +// normalizeComment is the canonicalization contract for dismissal identity (A1). +// It is deliberately minimal: TrimSpace collapses trailing/leading whitespace +// differences (a likely serialization-vs-display divergence) and ToLower handles +// ASCII case. Rephrasing/stylistic differences are explicitly out of scope (A1 +// known limitation); code-fence/citation stripping is intentionally not done so +// the contract stays trivially deterministic. +func normalizeComment(s string) string { + return strings.ToLower(strings.TrimSpace(s)) +} + +// DismissalFingerprint returns the stable per-finding identity for a comment +// (A1): SHA256(path + "\x00" + start_line + "\x00" + end_line + "\x00" + normalizedContent). +// Path and line numbers are taken verbatim (already canonical); Content is +// normalized via normalizeComment. Two findings on the same file differ on +// StartLine/EndLine/Content (D4). +func DismissalFingerprint(c model.LlmComment) string { + h := sha256.New() + fmt.Fprintf(h, "%s\x00%d\x00%d\x00%s", c.Path, c.StartLine, c.EndLine, normalizeComment(c.Content)) + return hex.EncodeToString(h.Sum(nil)) +} + +// DismissalFilter is a read-only set of dismissed fingerprints used to suppress +// findings from a review's returned comment slice. It is constructed once at +// review start (single goroutine, before subtask dispatch) and passed by +// reference to Agent.Run. +type DismissalFilter struct { + set map[string]struct{} +} + +// NewDismissalFilter builds a filter from a store's recorded fingerprints. A +// nil store yields a filter with an empty set (Suppress is then a no-op that +// returns its input unchanged). +func NewDismissalFilter(store *DismissalStore) *DismissalFilter { + f := &DismissalFilter{set: make(map[string]struct{})} + if store == nil { + return f + } + for fp := range store.entries { + f.set[fp] = struct{}{} + } + return f +} + +// Suppress returns a new slice omitting any comment whose dismissal fingerprint +// is in the filter's set (D1). It never mutates the input slice (D3) and +// preserves element order. When the set is empty it returns the input slice +// unchanged (zero-alloc fast path). +func (f *DismissalFilter) Suppress(comments []model.LlmComment) []model.LlmComment { + if f == nil || len(f.set) == 0 { + return comments + } + out := make([]model.LlmComment, 0, len(comments)) + for _, c := range comments { + if _, dismissed := f.set[DismissalFingerprint(c)]; !dismissed { + out = append(out, c) + } + } + return out +} + +// LoadComments walks a session JSONL and returns the flattened list of review +// comments in file-record order (the order records appear in the JSONL × +// comment order within each review_item_done/review_item_reused record). +// +// It exists because session.LoadDetail exposes only comment *counts* +// (ItemDetail.Comments is an int; the raw json.RawMessage is discarded), so +// `ocr dismiss add` has no existing public path to the comment bodies. This +// helper is additive: no existing session function signature changes, and it +// reuses the same SessionFilePath substrate as LoadResumeState/LoadDetail. +func LoadComments(repoDir, sessionID string) ([]model.LlmComment, error) { + path, err := SessionFilePath(repoDir, sessionID) + if err != nil { + return nil, err + } + var comments []model.LlmComment + walkErr := walkSessionFile(path, func(rec summaryRecord) { + if rec.Type != "review_item_done" && rec.Type != "review_item_reused" { + return + } + if len(rec.Comments) == 0 { + return + } + var batch []model.LlmComment + if err := json.Unmarshal(rec.Comments, &batch); err != nil { + return // malformed comment payload in one record does not poison the rest + } + comments = append(comments, batch...) + }) + if walkErr != nil { + return nil, walkErr + } + return comments, nil +} diff --git a/internal/session/dismissal_test.go b/internal/session/dismissal_test.go new file mode 100644 index 00000000..21537a65 --- /dev/null +++ b/internal/session/dismissal_test.go @@ -0,0 +1,461 @@ +package session + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/open-code-review/open-code-review/internal/model" +) + +// dismissTestRepo returns a unique repo dir for a test so concurrent -race +// tests never collide on the same encoded store path. UseTestSessions (called +// in persist_test.go init) redirects the dismissal subdir to test-dismissals. +func dismissTestRepo(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "myrepo") +} + +// writeDismissalStore writes raw bytes to the dismissal store path for a repo. +func writeDismissalStore(t *testing.T, repoDir string, data []byte) { + t.Helper() + path, err := dismissalPath(repoDir) + if err != nil { + t.Fatalf("dismissalPath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(path, data, 0600); err != nil { + t.Fatalf("write store: %v", err) + } +} + +func readDismissalStore(t *testing.T, repoDir string) []byte { + t.Helper() + path, err := dismissalPath(repoDir) + if err != nil { + t.Fatalf("dismissalPath: %v", err) + } + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read store: %v", err) + } + return b +} + +func TestDismissalFingerprintStable(t *testing.T) { + c := model.LlmComment{Path: "main.go", StartLine: 10, EndLine: 12, Content: "Fix this"} + fp1 := DismissalFingerprint(c) + fp2 := DismissalFingerprint(c) + if fp1 != fp2 { + t.Fatalf("fingerprint not stable across calls: %q vs %q", fp1, fp2) + } + if len(fp1) != 64 { + t.Fatalf("fingerprint is not 64 hex chars: %d (%q)", len(fp1), fp1) + } + + // Whitespace-only differences collapse via TrimSpace (AS8). + trimmed := DismissalFingerprint(model.LlmComment{Path: "main.go", StartLine: 10, EndLine: 12, Content: " \n\tFix this \r\n"}) + if trimmed != fp1 { + t.Errorf("TrimSpace normalization failed: trim=%q base=%q", trimmed, fp1) + } + + // Case-only differences collapse via ToLower (AS8). + lowered := DismissalFingerprint(model.LlmComment{Path: "main.go", StartLine: 10, EndLine: 12, Content: "FIX THIS"}) + if lowered != fp1 { + t.Errorf("ToLower normalization failed: lower=%q base=%q", lowered, fp1) + } + + // Different path/line/content produce different fingerprints (D4/AS8). + differents := []model.LlmComment{ + {Path: "other.go", StartLine: 10, EndLine: 12, Content: "Fix this"}, // different path + {Path: "main.go", StartLine: 99, EndLine: 12, Content: "Fix this"}, // different start line + {Path: "main.go", StartLine: 10, EndLine: 99, Content: "Fix this"}, // different end line + {Path: "main.go", StartLine: 10, EndLine: 12, Content: "different"}, // different content + {Path: "main.go", StartLine: 10, EndLine: 12, Content: "Fix this "}, // trailing space -> same after trim (sanity) + } + for i, d := range differents { + got := DismissalFingerprint(d) + if i == 4 { + if got != fp1 { + t.Errorf("case %d: expected same FP after trim, got different: %q vs %q", i, got, fp1) + } + continue + } + if got == fp1 { + t.Errorf("case %d: expected different fingerprint, got same %q", i, got) + } + } +} + +func TestNormalizeComment(t *testing.T) { + tests := []struct{ in, want string }{ + {" Hello ", "hello"}, + {"\n\tWorld\r\n", "world"}, + {"Already", "already"}, + {"", ""}, + {" ", ""}, + } + for _, tt := range tests { + if got := normalizeComment(tt.in); got != tt.want { + t.Errorf("normalizeComment(%q) = %q, want %q", tt.in, got, tt.want) + } + } +} + +func TestDismissalStoreRoundTrip(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals (empty): %v", err) + } + entry := DismissalEntry{ + Fingerprint: DismissalFingerprint(model.LlmComment{Path: "a.go", StartLine: 1, EndLine: 2, Content: "bug"}), + Path: "a.go", + ContentPreview: "bug", + DismissedAt: "2026-07-26T10:00:00Z", + } + store.Record(entry) + if !store.dirty { + t.Fatal("Record did not mark store dirty") + } + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + if store.dirty { + t.Fatal("Save did not clear dirty") + } + if !store.Contains(entry.Fingerprint) { + t.Fatal("Contains false before reload despite in-memory record") + } + + // Reload in a fresh store instance (simulates a new process — AS6/D5). + reloaded, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals (reload): %v", err) + } + if !reloaded.Contains(entry.Fingerprint) { + t.Fatal("reloaded store does not contain the recorded fingerprint") + } + got := reloaded.List() + if len(got) != 1 { + t.Fatalf("reloaded store has %d entries, want 1", len(got)) + } + if got[0].Path != "a.go" || got[0].ContentPreview != "bug" { + t.Errorf("reloaded entry mismatch: %+v", got[0]) + } + // Save is a no-op when not dirty (round-trips to identical bytes). + firstBytes := readDismissalStore(t, repo) + if err := reloaded.Save(); err != nil { + t.Fatalf("no-op Save: %v", err) + } + secondBytes := readDismissalStore(t, repo) + if string(firstBytes) != string(secondBytes) { + t.Errorf("no-op Save rewrote the file") + } +} + +func TestDismissalStoreMissingFileIsEmpty(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals on missing file returned error: %v (want nil)", err) + } + if store == nil { + t.Fatal("LoadDismissals on missing file returned nil store") + } + if got := store.List(); len(got) != 0 { + t.Fatalf("missing-file store has %d entries, want 0", len(got)) + } + // No file should have been created merely by loading. + if _, err := os.Stat(store.Path()); !os.IsNotExist(err) { + t.Fatalf("load created a file at %q (want no touch)", store.Path()) + } +} + +func TestDismissalStoreCorruptFailsSafe(t *testing.T) { + repo := dismissTestRepo(t) + garbage := []byte("{ this is : not valid json,,,") + writeDismissalStore(t, repo, garbage) + + store, err := LoadDismissals(repo) + if err == nil { + t.Fatal("LoadDismissals on corrupt file returned nil error (want non-nil)") + } + if store == nil { + t.Fatal("LoadDismissals on corrupt file returned nil store (want empty store)") + } + // Fail-safe: empty store, no suppression will happen. + if got := store.List(); len(got) != 0 { + t.Fatalf("corrupt-file store has %d entries, want 0 (fail-safe empty)", len(got)) + } + // D6/AS5: the corrupt file must be left untouched on disk. + after := readDismissalStore(t, repo) + if string(after) != string(garbage) { + t.Fatalf("corrupt file was modified on load:\n before=%q\n after=%q", garbage, after) + } +} + +func TestDismissalStoreUnreadableFileFailsSafe(t *testing.T) { + // Cover the unreadable (non-IsNotExist) branch of LoadDismissals by making + // the file unreadable. Skip on systems where we cannot enforce permission + // bits (running as root ignores 0o000). + if os.Geteuid() == 0 { + t.Skip("cannot test unreadable file as root") + } + repo := dismissTestRepo(t) + garbage := []byte("{not json}") + writeDismissalStore(t, repo, garbage) + path, err := dismissalPath(repo) + if err != nil { + t.Fatalf("dismissalPath: %v", err) + } + if err := os.Chmod(path, 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { os.Chmod(path, 0o600) }) + + store, err := LoadDismissals(repo) + if err == nil { + t.Fatal("expected non-nil error when file is unreadable") + } + if store == nil { + t.Fatal("expected non-nil store on unreadable file (fail-safe)") + } + // File left untouched: restore read permission and compare bytes. + if err := os.Chmod(path, 0o600); err != nil { + t.Fatalf("chmod restore: %v", err) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read after restore: %v", err) + } + if string(after) != string(garbage) { + t.Errorf("unreadable file was modified on load:\n before=%q\n after=%q", garbage, after) + } +} + +func TestDismissalStoreRecordIdempotent(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + fp := DismissalFingerprint(model.LlmComment{Path: "x.go", StartLine: 5, EndLine: 5, Content: "dup"}) + store.Record(DismissalEntry{Fingerprint: fp, Path: "x.go", ContentPreview: "first"}) + store.Record(DismissalEntry{Fingerprint: fp, Path: "x.go", ContentPreview: "second"}) + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + reloaded, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("reload: %v", err) + } + got := reloaded.List() + if len(got) != 1 { + t.Fatalf("idempotent Record produced %d entries, want 1", len(got)) + } + // Last-write-wins on duplicate fingerprint within the map. + if got[0].ContentPreview != "second" { + t.Errorf("idempotent Record preview = %q, want %q (last-write-wins)", got[0].ContentPreview, "second") + } +} + +func TestDismissalStoreRemove(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + fp := DismissalFingerprint(model.LlmComment{Path: "y.go", StartLine: 1, EndLine: 1, Content: "rm"}) + store.Record(DismissalEntry{Fingerprint: fp, Path: "y.go"}) + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + if !store.Contains(fp) { + t.Fatal("Contains false before Remove") + } + if store.Remove("not-present") { + t.Error("Remove returned true for absent fingerprint") + } + if !store.Remove(fp) { + t.Fatal("Remove returned false for present fingerprint") + } + if store.Contains(fp) { + t.Error("Contains true after Remove") + } + if err := store.Save(); err != nil { + t.Fatalf("Save after remove: %v", err) + } + reloaded, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("reload: %v", err) + } + if reloaded.Contains(fp) { + t.Error("removed fingerprint survived reload") + } +} + +func TestDismissalStoreListSorted(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + store.Record(DismissalEntry{Fingerprint: "c", DismissedAt: "2026-07-26T03:00:00Z"}) + store.Record(DismissalEntry{Fingerprint: "a", DismissedAt: "2026-07-26T01:00:00Z"}) + store.Record(DismissalEntry{Fingerprint: "b", DismissedAt: "2026-07-26T02:00:00Z"}) + got := store.List() + if len(got) != 3 { + t.Fatalf("List returned %d entries, want 3", len(got)) + } + // Sorted by DismissedAt ascending. + if got[0].Fingerprint != "a" || got[1].Fingerprint != "b" || got[2].Fingerprint != "c" { + t.Errorf("List not sorted by DismissedAt: %+v", got) + } +} + +func TestDismissalFilterSuppress(t *testing.T) { + repo := dismissTestRepo(t) + store, err := LoadDismissals(repo) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + cA := model.LlmComment{Path: "a.go", StartLine: 1, EndLine: 2, Content: "alpha"} + cB := model.LlmComment{Path: "a.go", StartLine: 5, EndLine: 6, Content: "beta"} + cC := model.LlmComment{Path: "b.go", StartLine: 1, EndLine: 1, Content: "gamma"} + // Dismiss only cA's fingerprint. + store.Record(DismissalEntry{Fingerprint: DismissalFingerprint(cA)}) + filter := NewDismissalFilter(store) + + input := []model.LlmComment{cA, cB, cC} + // Snapshot input to prove Suppress does not mutate it (D3). + inputCopy := make([]model.LlmComment, len(input)) + copy(inputCopy, input) + + out := filter.Suppress(input) + if len(out) != 2 { + t.Fatalf("Suppress returned %d comments, want 2 (cA dismissed)", len(out)) + } + for _, c := range out { + if c.Content == "alpha" { + t.Errorf("dismissed comment alpha survived Suppress: %+v", c) + } + } + // Order preserved: cB then cC. + if out[0].Content != "beta" || out[1].Content != "gamma" { + t.Errorf("Suppress changed order: %+v", out) + } + // Input slice unmodified (D3). + for i := range input { + if input[i] != inputCopy[i] { + t.Errorf("Suppress mutated input[%d]: %+v vs %+v", i, input[i], inputCopy[i]) + } + } +} + +func TestDismissalFilterEmptySetReturnsInput(t *testing.T) { + // Empty store -> empty set -> Suppress returns input slice as-is (zero-alloc fast path). + store, err := LoadDismissals(dismissTestRepo(t)) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + filter := NewDismissalFilter(store) + input := []model.LlmComment{{Path: "a.go", Content: "x"}} + out := filter.Suppress(input) + if len(out) != 1 || &out[0] != &input[0] { + t.Fatalf("empty-set Suppress did not return input slice as-is: len=%d same-alloc=%v", len(out), len(out) > 0 && &out[0] == &input[0]) + } +} + +func TestDismissalFilterNilSafe(t *testing.T) { + // NewDismissalFilter(nil) must not panic and yields a no-op filter. + filter := NewDismissalFilter(nil) + out := filter.Suppress([]model.LlmComment{{Path: "a.go", Content: "x"}}) + if len(out) != 1 { + t.Fatalf("nil-store filter dropped comments: %d", len(out)) + } + // Suppress on a nil receiver must not panic and returns the input. + var nilFilter *DismissalFilter + got := nilFilter.Suppress([]model.LlmComment{{Path: "a.go", Content: "x"}}) + if len(got) != 1 { + t.Fatalf("nil-receiver Suppress dropped comments: %d", len(got)) + } +} + +func TestDismissalFilePathResolvesUnderRoot(t *testing.T) { + repo := dismissTestRepo(t) + path, err := DismissalFilePath(repo) + if err != nil { + t.Fatalf("DismissalFilePath: %v", err) + } + if !strings.Contains(path, dismissalSubDir) { + t.Errorf("DismissalFilePath %q does not contain dismissal subdir %q", path, dismissalSubDir) + } + // Distinct from the session path (no collision). + sessPath, err := SessionFilePath(repo, "sess123") + if err != nil { + t.Fatalf("SessionFilePath: %v", err) + } + if path == sessPath { + t.Errorf("dismissal path collides with session path: %q", path) + } +} + +func TestLoadCommentsWalksSessionJSONL(t *testing.T) { + // Build a session JSONL by hand with two done records carrying comments, + // plus a non-comment record type that must be skipped. + repo := dismissTestRepo(t) + path, err := SessionFilePath(repo, "sess-abc") + if err != nil { + t.Fatalf("SessionFilePath: %v", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + t.Fatalf("mkdir: %v", err) + } + lines := []string{ + `{"type":"session_start","sessionId":"sess-abc","timestamp":"2026-07-26T10:00:00Z","cwd":"` + repo + `","gitBranch":"main","model":"fake"}`, + `{"type":"review_item_done","filePath":"a.go","fingerprint":"fa","comments":[{"path":"a.go","content":"alpha","start_line":1,"end_line":2},{"path":"a.go","content":"alpha2","start_line":3,"end_line":4}]}`, + `{"type":"llm_request","filePath":"a.go"}`, + `{"type":"review_item_reused","filePath":"b.go","fingerprint":"fb","comments":[{"path":"b.go","content":"beta","start_line":1,"end_line":1}]}`, + `{"type":"review_item_done","filePath":"c.go","fingerprint":"fc"}`, + `{"type":"session_end","sessionId":"sess-abc"}`, + } + if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0600); err != nil { + t.Fatalf("write session: %v", err) + } + + got, err := LoadComments(repo, "sess-abc") + if err != nil { + t.Fatalf("LoadComments: %v", err) + } + if len(got) != 3 { + t.Fatalf("LoadComments returned %d comments, want 3", len(got)) + } + // File-record order: a.go's two comments, then b.go's one comment. The + // llm_request and the comment-less c.go record are skipped. + wantContents := []string{"alpha", "alpha2", "beta"} + for i, want := range wantContents { + if got[i].Content != want { + t.Errorf("LoadComments[%d].Content = %q, want %q", i, got[i].Content, want) + } + } + // Fingerprint computed from LoadComments output matches one computed from + // an equivalent in-memory comment (D1 cross-process consistency). + fp := DismissalFingerprint(model.LlmComment{Path: "b.go", StartLine: 1, EndLine: 1, Content: "beta"}) + if DismissalFingerprint(got[2]) != fp { + t.Errorf("LoadComments comment fingerprint mismatch: got %q want %q", DismissalFingerprint(got[2]), fp) + } +} + +func TestLoadCommentsMissingSession(t *testing.T) { + got, err := LoadComments(dismissTestRepo(t), "nope") + if err == nil { + t.Fatal("LoadComments on missing session returned nil error") + } + if got != nil && len(got) != 0 { + t.Errorf("LoadComments on missing session returned %d comments, want 0", len(got)) + } +} diff --git a/internal/session/testing.go b/internal/session/testing.go index 1632ba74..9cd5619a 100644 --- a/internal/session/testing.go +++ b/internal/session/testing.go @@ -3,8 +3,12 @@ package session // UseTestSessions redirects session persistence to the "test-sessions" // subdirectory so that test runs do not pollute the real sessions store. // +// It also redirects the dismissal store to a "test-dismissals" subdirectory so +// dismissal tests stay offline and hermetic alongside session tests. +// // It must be called from init() in a _test.go file or from TestMain, // before any test goroutines start. It is NOT safe for concurrent use. func UseTestSessions() { sessionSubDir = "test-sessions" + dismissalSubDir = "test-dismissals" } From 0afec2263670d2d498bc33c92f3f6b9500dab82f Mon Sep 17 00:00:00 2001 From: Nitish Agarwal <1592163+nitishagar@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:53:49 +0530 Subject: [PATCH 2/2] fix(dismiss): address PR #509 review feedback Four fixes from the automated review on #509: 1. runDismissList: guard against a nil store. LoadDismissals returns (nil, err) when the store path cannot be resolved (HOME unset); the previous code printed a warning then dereferenced store.List(), panicking. Return an error on a nil store instead. 2. loadDismissalFilter: drop the explicit os.Stat before LoadDismissals. ReadFile inside LoadDismissals already performs the existence check, so the stat was redundant and opened a stat->read TOCTOU window in which a deleted file returned an empty filter instead of nil. Rely on LoadDismissals and return nil for an empty/missing store to keep the "nil filter == stateless" invariant (D2). 3. resolveRemoveRef: when a short-fingerprint prefix matches more than one entry, report the ambiguity explicitly rather than falling through to a generic "no match" error (consistent with resolveFindingRef). 4. printDismissTable: stop double-truncating ContentPreview, which is already capped at store time. Adds regression tests for the nil-store panic guard and the ambiguous prefix report. --- cmd/opencodereview/dismiss_cmd.go | 21 +++++++++-- cmd/opencodereview/dismiss_cmd_test.go | 51 ++++++++++++++++++++++++++ cmd/opencodereview/review_cmd.go | 33 ++++++++++------- 3 files changed, 87 insertions(+), 18 deletions(-) diff --git a/cmd/opencodereview/dismiss_cmd.go b/cmd/opencodereview/dismiss_cmd.go index 2820a466..974107c4 100644 --- a/cmd/opencodereview/dismiss_cmd.go +++ b/cmd/opencodereview/dismiss_cmd.go @@ -130,6 +130,13 @@ func runDismissList(args []string) error { } store, err := session.LoadDismissals(resolvedRepo) if err != nil { + // LoadDismissals returns (nil, err) only when the store path cannot be + // resolved (e.g. HOME unresolvable); otherwise it returns (emptyStore, err) + // for a corrupt/unreadable file so we can still list nothing safely. Guard + // both: on a nil store we cannot proceed (no path to consult). + if store == nil { + return fmt.Errorf("load dismissal store: %w", err) + } fmt.Fprintf(os.Stderr, "[ocr] WARNING: dismissal store corrupt (%v); showing no dismissals. The file was left untouched.\n", err) } entries := store.List() @@ -236,16 +243,21 @@ func resolveRemoveRef(store *session.DismissalStore, ref string) (string, error) // Short fingerprint prefix (first 12 chars, as printed by `dismiss list`). if len(ref) >= 4 { var match string - count := 0 + var matches []string for _, e := range entries { if strings.HasPrefix(e.Fingerprint, ref) { match = e.Fingerprint - count++ + matches = append(matches, e.Fingerprint) } } - if count == 1 { + if len(matches) == 1 { return match, nil } + if len(matches) > 1 { + // Report the ambiguity explicitly rather than falling through to a + // generic "no match" error (consistent with resolveFindingRef). + return "", fmt.Errorf("prefix %q matches %d dismissals; provide more characters or use 'ocr dismiss list' to disambiguate (matches: %s)", ref, len(matches), shortFingerprint(matches[0])) + } } // List index form (checked last so a decimal-looking fingerprint prefix is // never misread as an index). @@ -285,7 +297,8 @@ func printDismissTable(w io.Writer, entries []session.DismissalEntry) { tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) fmt.Fprintln(tw, "INDEX\tFINGERPRINT\tPATH\tPREVIEW\tDISMISSED AT") for i, e := range entries { - fmt.Fprintf(tw, "%d\t%s\t%s\t%s\t%s\n", i, shortFingerprint(e.Fingerprint), e.Path, truncateForPreview(e.ContentPreview), e.DismissedAt) + // ContentPreview is already truncated at store time; use it directly. + fmt.Fprintf(tw, "%d\t%s\t%s\t%s\t%s\n", i, shortFingerprint(e.Fingerprint), e.Path, e.ContentPreview, e.DismissedAt) } tw.Flush() } diff --git a/cmd/opencodereview/dismiss_cmd_test.go b/cmd/opencodereview/dismiss_cmd_test.go index 9b1d1d0c..2c18b9b5 100644 --- a/cmd/opencodereview/dismiss_cmd_test.go +++ b/cmd/opencodereview/dismiss_cmd_test.go @@ -212,6 +212,57 @@ func TestDismissListCorruptStoreWarnsAndShowsEmpty(t *testing.T) { } } +func TestDismissListNoPanicWhenStorePathUnresolvable(t *testing.T) { + // Regression: LoadDismissals returns (nil, err) when HOME is unresolvable; + // runDismissList must return an error, not panic on a nil store deref. + t.Setenv("HOME", "") + repoDir := t.TempDir() + err := runDismiss([]string{"list", "--repo", repoDir}) + if err == nil { + t.Fatal("expected error when store path cannot be resolved (HOME unset), got nil") + } +} + +func TestDismissRemoveAmbiguousPrefixReports(t *testing.T) { + // Regression: a short-fingerprint prefix matching multiple entries must + // report the ambiguity explicitly, not fall through to a generic "no match". + // Use resolveRemoveRef directly with crafted entries sharing a known prefix + // (real SHA-256 fingerprints rarely collide on a prefix, so the store is + // constructed with explicit fingerprint strings). + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + store, err := session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("LoadDismissals: %v", err) + } + store.Record(session.DismissalEntry{Fingerprint: "abc1230000000000000000000000000000000000000000000000000000000000", Path: "a.go"}) + store.Record(session.DismissalEntry{Fingerprint: "abc1239999999999999999999999999999999999999999999999999999999999", Path: "b.go"}) + if err := store.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + store, err = session.LoadDismissals(repoDir) + if err != nil { + t.Fatalf("reload: %v", err) + } + + _, err = resolveRemoveRef(store, "abc1") + if err == nil { + t.Fatal("expected ambiguity error for shared prefix, got nil") + } + if !strings.Contains(err.Error(), "matches") || !strings.Contains(err.Error(), "disambiguate") { + t.Errorf("expected ambiguity-reporting error, got: %v", err) + } + + // Sanity: a unique prefix still resolves. + fp, err := resolveRemoveRef(store, "abc1230") + if err != nil { + t.Fatalf("unique prefix should resolve: %v", err) + } + if !strings.HasPrefix(fp, "abc1230") { + t.Errorf("resolved fingerprint %q does not extend the prefix", fp) + } +} + func TestRunDismissUsage(t *testing.T) { out := captureStdout(t, func() { if err := runDismiss(nil); err != nil { diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go index 6131d716..e8337b63 100644 --- a/cmd/opencodereview/review_cmd.go +++ b/cmd/opencodereview/review_cmd.go @@ -198,25 +198,30 @@ func reviewModeFromOptions(opts reviewOptions) string { // loadDismissalFilter loads the per-repo dismissal filter for review suppression. // -// It performs a single stat of the dismissal store path and returns nil when the -// store does not exist (D2: byte-identical to the stateless default — no read, -// no allocation). When the store exists but is corrupt or unreadable, it prints +// It returns nil when there is no dismissal store or the store is empty, so a +// review with no dismissals on disk is byte-identical to the stateless default +// (D2: nil filter ⇒ Agent.Run passes comments through untouched). LoadDismissals +// already performs the existence check internally via os.ReadFile's IsNotExist +// handling, so no separate stat is needed (which also removes any stat→read +// TOCTOU window). When the store exists but is corrupt or unreadable, it prints // a warning to stderr and returns nil so the review proceeds stateless (D6/AS5: -// fail safe; the corrupt file is left untouched). On a successful load it returns -// a DismissalFilter built from the store's fingerprints. +// fail safe; the corrupt file is left untouched). On a successful load with at +// least one entry it returns a DismissalFilter built from the store's fingerprints. func loadDismissalFilter(repoDir string) *session.DismissalFilter { - path, err := session.DismissalFilePath(repoDir) + store, err := session.LoadDismissals(repoDir) if err != nil { - // Could not resolve home dir; there is no store to consult. Stay stateless. - return nil - } - if _, err := os.Stat(path); err != nil { - // Missing (or otherwise unreadable): no opt-in. One stat, no read. + if store == nil { + // Store path could not be resolved (e.g. HOME unresolvable): stay stateless. + return nil + } + fmt.Fprintf(os.Stderr, "[ocr] WARNING: dismissal store %q is corrupt or unreadable (%v); proceeding without suppression. The file was left untouched.\n", store.Path(), err) return nil } - store, err := session.LoadDismissals(repoDir) - if err != nil { - fmt.Fprintf(os.Stderr, "[ocr] WARNING: dismissal store %q is corrupt or unreadable (%v); proceeding without suppression. The file was left untouched.\n", path, err) + // Empty store (missing file or an explicit empty store) ⇒ nil filter so the + // review is byte-identical to having no store (D2). An empty filter would also + // be a no-op via Suppress's fast path, but nil keeps the "nil = stateless" + // invariant clean and skips allocating the filter. + if len(store.List()) == 0 { return nil } return session.NewDismissalFilter(store)