fix(session): prevent repository session path collisions - #913
Conversation
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| summaries := make([]Summary, 0) | ||
| readDir := func(dir string, legacy bool) error { |
There was a problem hiding this comment.
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:
| 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 { |
| if legacy { | ||
| matches, matchErr := sessionFileBelongsToRepo(path, repoDir) | ||
| if matchErr != nil || !matches { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| func readSessionCWD(path string) (string, error) { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer f.Close() | ||
|
|
||
| reader := bufio.NewReader(f) | ||
| for { | ||
| line, readErr := reader.ReadBytes('\n') | ||
| var rec struct { | ||
| Type string `json:"type"` | ||
| CWD string `json:"cwd"` | ||
| } | ||
| if json.Unmarshal(line, &rec) == nil && rec.Type == "session_start" { | ||
| return rec.CWD, nil | ||
| } | ||
| if readErr == io.EOF { | ||
| return "", nil | ||
| } | ||
| if readErr != nil { | ||
| return "", readErr | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In readSessionCWD, if a session file is truncated (no newline and EOF reached), the function returns "", nil instead of an error. This silent failure can cause the caller (DiscoverRepos) to group the session under a legacy key rather than skipping it. Consider returning an error for truncated sessions or at least documenting this behavior.
1 participant
Description
Fixes repository session path collisions by replacing the legacy separator-based directory name with a collision-resistant, versioned key derived from the canonical repository path.
Legacy sessions remain readable and are filtered by their recorded repository path. Viewer discovery, session listing, comments, details, and resume loading now preserve repository isolation.
Type of Change
How Has This Been Tested?
make testpasses locallyAdditional checks performed:
go test -race -count=1 ./...make checkmake coverage— 91.1% coveragegovulncheck ./...git diff --checkTests cover new-session storage, legacy compatibility, resume loading, comment loading, viewer discovery, mixed legacy/current sessions, and cross-repository isolation.
Checklist
go fmt,go vet)Related Issues
Fixes #894