Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/session/comments.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
// earlier one, and a subsequent review_item_failed drops it. Comments that
// were persisted without a path inherit the record's file path.
func LoadComments(repoDir, sessionID string) ([]model.LlmComment, error) {
path, err := SessionFilePath(repoDir, sessionID)
path, err := findSessionFile(repoDir, sessionID)
if err != nil {
return nil, err
}
Expand Down
68 changes: 44 additions & 24 deletions internal/session/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,40 +96,60 @@ type summaryRecord struct {
// SessionsDir returns the on-disk directory that holds JSONL session files
// for a given repository. It does not create the directory.
func SessionsDir(repoDir string) (string, error) {
home, err := os.UserHomeDir()
if err != nil {
return "", fmt.Errorf("resolve home dir: %w", err)
}
return filepath.Join(home, ".opencodereview", sessionSubDir, encodeRepoPath(repoDir)), nil
current, _, err := sessionDirectories(repoDir)
return current, err
}

// ListSessions enumerates all persisted sessions for the given repository
// directory, sorted by StartTime descending (most recent first). Missing
// directories return an empty slice with no error.
func ListSessions(repoDir string) ([]Summary, error) {
dir, err := SessionsDir(repoDir)
currentDir, legacyDir, err := sessionDirectories(repoDir)
if err != nil {
return nil, err
}
entries, err := os.ReadDir(dir)
if err != nil {
if os.IsNotExist(err) {
return nil, nil
summaries := make([]Summary, 0)
seenSessionIDs := make(map[string]struct{})
readDir := func(dir string, legacy bool) error {
Comment on lines +111 to +113

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
Performance regression: The original code pre-allocated slice capacity using make([]Summary, 0, len(entries)) based on the directory entry count. The new code uses make([]Summary, 0) without capacity, which will cause multiple slice reallocations as sessions are appended. While individually small, this adds unnecessary allocations especially when listing repositories with many sessions across both directories.

Suggestion:

Suggested change
summaries := make([]Summary, 0)
readDir := func(dir string, legacy bool) error {
// Pre-allocate with estimated capacity; readDir will append from both directories
summaries := make([]Summary, 0, 32)
readDir := func(dir string, legacy bool) error {

entries, readErr := os.ReadDir(dir)
if readErr != nil {
if os.IsNotExist(readErr) {
return nil
}
return fmt.Errorf("read sessions dir %q: %w", dir, readErr)
}
return nil, fmt.Errorf("read sessions dir %q: %w", dir, err)
}
summaries := make([]Summary, 0, len(entries))
for _, entry := range entries {
name := entry.Name()
if entry.IsDir() || !strings.HasSuffix(name, ".jsonl") {
continue
for _, entry := range entries {
name := entry.Name()
if entry.IsDir() || !strings.HasSuffix(name, ".jsonl") {
continue
}
path := filepath.Join(dir, name)
if legacy {
matches, matchErr := sessionFileBelongsToRepo(path, repoDir)
if matchErr != nil || !matches {
continue
}
}
Comment on lines +127 to +132

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug · medium
The sessionFileBelongsToRepo validation is only applied to legacy directory entries (legacy == true), not current directory entries. This asymmetry assumes the current directory path computation is collision-proof. While RepoSessionKey likely uses a more robust encoding (possibly with hashing based on test references to 'v2-' prefix), if the path computation ever has a bug or edge case, sessions from other repositories would be silently included from current directories without validation. Consider whether defensive validation should apply to both directories, or document why current directories are guaranteed to be isolated.

sessionID := strings.TrimSuffix(name, ".jsonl")
if _, seen := seenSessionIDs[sessionID]; seen {
continue
}
summary, loadErr := loadSummaryFromFile(path, sessionID, repoDir)
if loadErr != nil {
continue
}
summaries = append(summaries, *summary)
seenSessionIDs[sessionID] = struct{}{}
}
sessionID := strings.TrimSuffix(name, ".jsonl")
summary, err := loadSummaryFromFile(filepath.Join(dir, name), sessionID, repoDir)
if err != nil {
continue
return nil
}
if err := readDir(currentDir, false); err != nil {
return nil, err
}
if legacyDir != currentDir {
if err := readDir(legacyDir, true); err != nil {
return nil, err
}
summaries = append(summaries, *summary)
}
Comment thread
amh1k marked this conversation as resolved.
sort.Slice(summaries, func(i, j int) bool {
return summaries[i].StartTime.After(summaries[j].StartTime)
Expand All @@ -140,7 +160,7 @@ func ListSessions(repoDir string) ([]Summary, error) {
// LoadSummary loads a single session's Summary. Errors when the session
// file is missing or unreadable.
func LoadSummary(repoDir, sessionID string) (*Summary, error) {
path, err := SessionFilePath(repoDir, sessionID)
path, err := findSessionFile(repoDir, sessionID)
if err != nil {
return nil, err
}
Expand All @@ -149,7 +169,7 @@ func LoadSummary(repoDir, sessionID string) (*Summary, error) {

// LoadDetail returns the summary plus per-file item records for one session.
func LoadDetail(repoDir, sessionID string) (*Summary, []ItemDetail, error) {
path, err := SessionFilePath(repoDir, sessionID)
path, err := findSessionFile(repoDir, sessionID)
if err != nil {
return nil, nil, err
}
Expand Down
199 changes: 199 additions & 0 deletions internal/session/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package session

import (
"encoding/json"
"os"
"path/filepath"
"testing"
"time"
Expand All @@ -24,6 +26,203 @@ func TestListSessions_EmptyRepoReturnsNil(t *testing.T) {
}
}

func TestListSessions_FiltersCollidingLegacySessionsByRepository(t *testing.T) {
tmpHome := t.TempDir()
t.Setenv("HOME", tmpHome)
base := t.TempDir()
repoA := filepath.Join(base, "team", "service-api")
repoB := filepath.Join(base, "team-service", "api")
if err := os.MkdirAll(repoA, 0755); err != nil {
t.Fatal(err)
}
if err := os.MkdirAll(repoB, 0755); err != nil {
t.Fatal(err)
}

legacyDir := filepath.Join(tmpHome, ".opencodereview", "test-sessions", encodeRepoPath(repoA))
if err := os.MkdirAll(legacyDir, 0700); err != nil {
t.Fatal(err)
}
writeJSONL := func(sessionID, cwd string) {
t.Helper()
path := filepath.Join(legacyDir, sessionID+".jsonl")
data, err := json.Marshal(map[string]string{
"type": "session_start",
"sessionId": sessionID,
"timestamp": "2026-08-01T00:00:00Z",
"cwd": cwd,
})
if err != nil {
t.Fatal(err)
}
data = append(data, '\n')
if err := os.WriteFile(path, []byte(data), 0600); err != nil {
t.Fatal(err)
}
}
writeJSONL("repo-a-session", repoA)
writeJSONL("repo-b-session", repoB)
commentRecord, err := json.Marshal(map[string]any{
"type": "review_item_done",
"filePath": "legacy.go",
"fingerprint": "fp-legacy",
"comments": []model.LlmComment{{Content: "legacy comment"}},
})
if err != nil {
t.Fatal(err)
}
commentFile, err := os.OpenFile(filepath.Join(legacyDir, "repo-a-session.jsonl"), os.O_APPEND|os.O_WRONLY, 0600)
if err != nil {
t.Fatal(err)
}
if _, err := commentFile.Write(append(commentRecord, '\n')); err != nil {
commentFile.Close()
t.Fatal(err)
}
if err := commentFile.Close(); err != nil {
t.Fatal(err)
}

gotA, err := ListSessions(repoA)
if err != nil {
t.Fatalf("ListSessions(repoA): %v", err)
}
if len(gotA) != 1 || gotA[0].SessionID != "repo-a-session" {
t.Fatalf("repo A sessions = %+v, want only repo-a-session", gotA)
}

gotB, err := ListSessions(repoB)
if err != nil {
t.Fatalf("ListSessions(repoB): %v", err)
}
if len(gotB) != 1 || gotB[0].SessionID != "repo-b-session" {
t.Fatalf("repo B sessions = %+v, want only repo-b-session", gotB)
}

if _, err := LoadSummary(repoA, "repo-b-session"); !os.IsNotExist(err) {
t.Fatalf("LoadSummary(repoA, repo-b-session) error = %v, want not found", err)
}

detail, items, err := LoadDetail(repoA, "repo-a-session")
if err != nil {
t.Fatalf("LoadDetail(repoA, repo-a-session): %v", err)
}
if detail.RepoDir != repoA || len(items) != 1 || items[0].FilePath != "legacy.go" {
t.Fatalf("legacy detail = (%+v, %+v), want repo A legacy item", detail, items)
}
if _, _, err := LoadDetail(repoB, "repo-a-session"); !os.IsNotExist(err) {
t.Fatalf("LoadDetail(repoB, repo-a-session) error = %v, want not found", err)
}

state, err := LoadResumeState(repoA, "repo-a-session")
if err != nil {
t.Fatalf("LoadResumeState(repoA, repo-a-session): %v", err)
}
if state.RepoDir != repoA {
t.Fatalf("legacy resume RepoDir = %q, want %q", state.RepoDir, repoA)
}
if _, err := LoadResumeState(repoB, "repo-a-session"); !os.IsNotExist(err) {
t.Fatalf("LoadResumeState(repoB, repo-a-session) error = %v, want not found", err)
}

comments, err := LoadComments(repoA, "repo-a-session")
if err != nil {
t.Fatalf("LoadComments(repoA, repo-a-session): %v", err)
}
if len(comments) != 1 || comments[0].Content != "legacy comment" || comments[0].Path != "legacy.go" {
t.Fatalf("legacy comments = %+v, want one inherited-path comment", comments)
}
if _, err := LoadComments(repoB, "repo-a-session"); !os.IsNotExist(err) {
t.Fatalf("LoadComments(repoB, repo-a-session) error = %v, want not found", err)
}
}

func TestListSessions_DeduplicatesCurrentAndLegacySessionID(t *testing.T) {
t.Setenv("HOME", t.TempDir())
repoDir := t.TempDir()
currentDir, legacyDir, err := sessionDirectories(repoDir)
if err != nil {
t.Fatal(err)
}
for _, dir := range []string{currentDir, legacyDir} {
if err := os.MkdirAll(dir, 0700); err != nil {
t.Fatal(err)
}
}

const sessionID = "duplicate-session"
writeStart := func(dir, model string) {
t.Helper()
data, marshalErr := json.Marshal(map[string]string{
"type": "session_start",
"sessionId": sessionID,
"timestamp": "2026-08-01T00:00:00Z",
"cwd": repoDir,
"model": model,
})
if marshalErr != nil {
t.Fatal(marshalErr)
}
if err := os.WriteFile(filepath.Join(dir, sessionID+".jsonl"), append(data, '\n'), 0600); err != nil {
t.Fatal(err)
}
}
writeStart(currentDir, "current-model")
writeStart(legacyDir, "legacy-model")

summaries, err := ListSessions(repoDir)
if err != nil {
t.Fatal(err)
}
if len(summaries) != 1 {
t.Fatalf("sessions = %+v, want one deduplicated session", summaries)
}
if summaries[0].SessionID != sessionID || summaries[0].Model != "current-model" {
t.Fatalf("session = %+v, want current-format version", summaries[0])
}
}

func TestNewSessionsUseDistinctDirectoriesForCollidingPaths(t *testing.T) {
tmpHome := t.TempDir()
t.Setenv("HOME", tmpHome)
base := t.TempDir()
repoA := filepath.Join(base, "team", "service-api")
repoB := filepath.Join(base, "team-service", "api")
if err := os.MkdirAll(repoA, 0755); err != nil {
t.Fatal(err)
}
if err := os.MkdirAll(repoB, 0755); err != nil {
t.Fatal(err)
}

first := New(repoA, "main", "mock", SessionOptions{ReviewMode: ReviewModeWorkspace})
if err := first.Finalize(); err != nil {
t.Fatal(err)
}
second := New(repoB, "main", "mock", SessionOptions{ReviewMode: ReviewModeWorkspace})
if err := second.Finalize(); err != nil {
t.Fatal(err)
}

dirA, err := SessionsDir(repoA)
if err != nil {
t.Fatal(err)
}
dirB, err := SessionsDir(repoB)
if err != nil {
t.Fatal(err)
}
if dirA == dirB {
t.Fatalf("session directories collided: %q", dirA)
}
if _, err := os.Stat(filepath.Join(dirA, first.SessionID+".jsonl")); err != nil {
t.Fatalf("repo A session missing from its directory: %v", err)
}
if _, err := os.Stat(filepath.Join(dirB, second.SessionID+".jsonl")); err != nil {
t.Fatalf("repo B session missing from its directory: %v", err)
}
}

func TestListSessions_SortsAndAggregates(t *testing.T) {
tmpHome := t.TempDir()
t.Setenv("HOME", tmpHome)
Expand Down
Loading