Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
63 changes: 39 additions & 24 deletions internal/session/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,40 +96,55 @@ 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)
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")
summary, loadErr := loadSummaryFromFile(path, sessionID, repoDir)
if loadErr != nil {
continue
}
summaries = append(summaries, *summary)
}
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 +155,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 +164,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
154 changes: 154 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,158 @@ 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 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
32 changes: 2 additions & 30 deletions internal/session/persist.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"io"
"os"
"path/filepath"
"strings"
"sync"
"time"

Expand All @@ -21,7 +20,7 @@ import (
var sessionSubDir = "sessions"

// jsonlWriter streams session records to a JSONL file under
// $HOME/.opencodereview/sessions/<encoded-repo-path>/<session-id>.jsonl.
// $HOME/.opencodereview/sessions/<repo-key>/<session-id>.jsonl.
// It is safe for concurrent use by multiple goroutines.
type jsonlWriter struct {
mu sync.Mutex
Expand Down Expand Up @@ -73,40 +72,13 @@ func generateUUID() string {
b[0:4], b[4:6], b[6:8], b[8:10], b[10:])
}

func encodeRepoPath(p string) string {
// Handle empty or invalid input
if p == "" {
return "empty"
}

vol := filepath.VolumeName(p)
p = p[len(vol):]

// Trim leading path separators
p = strings.TrimLeft(p, "/\\")

// Replace separators with -
p = strings.ReplaceAll(p, "/", "-")
p = strings.ReplaceAll(p, "\\", "-")

// Replace colons (from Windows drive letters)
vol = strings.ReplaceAll(vol, ":", "_")

// Handle edge case where path was only separators or volume name
result := vol + p
if result == "" {
return "empty"
}
return result
}

func (jw *jsonlWriter) open() error {
home, err := os.UserHomeDir()
if err != nil {
return fmt.Errorf("resolve home dir: %w", err)
}

sessionDir := filepath.Join(home, ".opencodereview", sessionSubDir, encodeRepoPath(jw.repoDir))
sessionDir := filepath.Join(home, ".opencodereview", sessionSubDir, RepoSessionKey(jw.repoDir))
Comment thread
amh1k marked this conversation as resolved.
if err := os.MkdirAll(sessionDir, 0700); err != nil {
return fmt.Errorf("create session dir: %w", err)
}
Expand Down
28 changes: 26 additions & 2 deletions internal/session/persist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,30 @@ func TestEncodeRepoPath(t *testing.T) {
}
}

func TestRepoSessionKey_DistinguishesCollidingLegacyPaths(t *testing.T) {
base := t.TempDir()
pathA := filepath.Join(base, "team", "service-api")
pathB := filepath.Join(base, "team-service", "api")

if encodeRepoPath(pathA) != encodeRepoPath(pathB) {
t.Fatalf("test paths no longer collide under the legacy encoder: %q vs %q", encodeRepoPath(pathA), encodeRepoPath(pathB))
}
if RepoSessionKey(pathA) == RepoSessionKey(pathB) {
t.Fatalf("RepoSessionKey(%q) and RepoSessionKey(%q) collided", pathA, pathB)
}
if !strings.HasPrefix(RepoSessionKey(pathA), "v2-") {
t.Fatalf("RepoSessionKey(%q) = %q, want versioned key", pathA, RepoSessionKey(pathA))
}
if !IsRepoSessionKey(RepoSessionKey(pathA)) {
t.Fatalf("IsRepoSessionKey rejected generated key %q", RepoSessionKey(pathA))
}
for _, legacy := range []string{"repo", "v2-repo", "v2-repo-not-a-digest"} {
if IsRepoSessionKey(legacy) {
t.Errorf("IsRepoSessionKey(%q) = true for legacy-shaped key", legacy)
}
}
}

func readJSONLRecords(t *testing.T, path string) []map[string]any {
t.Helper()
f, err := os.Open(path)
Expand All @@ -136,7 +160,7 @@ func sessionJSONLPath(t *testing.T, repoDir, sessionID string) string {
if err != nil {
t.Fatalf("home dir: %v", err)
}
return filepath.Join(home, ".opencodereview", "test-sessions", encodeRepoPath(repoDir), sessionID+".jsonl")
return filepath.Join(home, ".opencodereview", "test-sessions", RepoSessionKey(repoDir), sessionID+".jsonl")
}

func TestSetErrorIncrementsCounter(t *testing.T) {
Expand Down Expand Up @@ -221,7 +245,7 @@ func TestSessionFilePermissions(t *testing.T) {
jw.WriteSessionStart(time.Now())
defer jw.flushAndClose()

sessionDir := filepath.Join(tmpHome, ".opencodereview", "test-sessions", encodeRepoPath(repoDir))
sessionDir := filepath.Join(tmpHome, ".opencodereview", "test-sessions", RepoSessionKey(repoDir))
sessionFile := filepath.Join(sessionDir, sessionID+".jsonl")

dirInfo, err := os.Stat(sessionDir)
Expand Down
Loading
Loading