From 9d721cd19ccceacf9cb183c338dc0bfdee5a7999 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 30 Jul 2026 17:07:07 +0300 Subject: [PATCH 1/3] refactor(ui): move session discovery and file I/O to app layer The session picker was the last part of the TUI that reached into internal/session directly: it listed sessions, deleted their files and type-asserted on session.SessionInfo. /export and /share likewise read and wrote session JSONL themselves, so knowledge of the on-disk format was spread across the presentation layer. Session discovery now returns app.SessionSummary values, and the two file-producing commands are expressed as app-layer operations that return sentinel errors the UI maps to its own messages. The picker takes a narrow SessionStore port rather than calling the package directly, which also makes its scope, filter and delete flows testable without touching the filesystem. With this, internal/ui no longer imports internal/session at all and the session file format is owned entirely by the app layer. SessionSystemPromptEntry is folded into WriteShareableSession; it was introduced only as a step towards moving the share path, and the UI no longer needs to know that shared files embed a system-prompt entry. Fixes #101 --- internal/app/session_store.go | 260 ++++++++++++++++ internal/app/session_store_test.go | 447 +++++++++++++++++++++++++++ internal/app/session_view.go | 23 -- internal/app/session_view_test.go | 52 ---- internal/ui/model.go | 173 ++--------- internal/ui/model_test.go | 30 +- internal/ui/session_selector.go | 56 ++-- internal/ui/session_selector_test.go | 311 +++++++++++++++++++ 8 files changed, 1117 insertions(+), 235 deletions(-) create mode 100644 internal/app/session_store.go create mode 100644 internal/app/session_store_test.go create mode 100644 internal/ui/session_selector_test.go diff --git a/internal/app/session_store.go b/internal/app/session_store.go new file mode 100644 index 00000000..5f4f5e8a --- /dev/null +++ b/internal/app/session_store.go @@ -0,0 +1,260 @@ +package app + +import ( + "errors" + "fmt" + "os" + "strings" + "time" + + "github.com/mark3labs/kit/internal/session" +) + +// ErrSessionNotPersisted is returned by operations that need the session's +// backing file when the active session exists only in memory. +var ErrSessionNotPersisted = errors.New("session is not persisted to disk") + +// SessionSummary is an immutable value description of a session stored on +// disk. It carries what a session picker needs to identify, search, sort and +// render a session without opening the file or knowing the JSONL schema. +// +// It is deliberately narrower than the on-disk metadata: fields are added +// here as presentation layers need them, so the persistence format stays free +// to change. +type SessionSummary struct { + // Path is the absolute path to the JSONL file backing the session. + Path string + // ID is the session UUID. + ID string + // Name is the user-defined display name, empty if unnamed. + Name string + // Cwd is the working directory the session was created in. + Cwd string + // Created is the session creation timestamp. + Created time.Time + // Modified is the timestamp of the most recent activity. + Modified time.Time + // MessageCount is the number of message entries in the session. + MessageCount int + // FirstMessage is a preview of the first user message, empty when the + // session holds no user messages. + FirstMessage string +} + +// ListSessions returns summaries of the sessions recorded for cwd, newest +// first. An empty cwd yields no sessions rather than an error, so callers can +// pass through an unknown working directory unconditionally. +func (a *App) ListSessions(cwd string) ([]SessionSummary, error) { + if cwd == "" { + return nil, nil + } + infos, err := session.ListSessions(cwd) + if err != nil { + return nil, fmt.Errorf("list sessions for %q: %w", cwd, err) + } + return sessionSummaries(infos), nil +} + +// ListAllSessions returns summaries of every session across all working +// directories, newest first. +func (a *App) ListAllSessions() ([]SessionSummary, error) { + infos, err := session.ListAllSessions() + if err != nil { + return nil, fmt.Errorf("list all sessions: %w", err) + } + return sessionSummaries(infos), nil +} + +// DeleteSession removes a session file from disk. Deleting the file backing +// the active session is permitted: the session keeps running in memory and +// simply stops being discoverable. +func (a *App) DeleteSession(path string) error { + if path == "" { + return errors.New("session path is required") + } + if err := session.DeleteSession(path); err != nil { + return fmt.Errorf("delete session %q: %w", path, err) + } + return nil +} + +// sessionSummaries projects discovered session metadata into value summaries. +func sessionSummaries(infos []session.SessionInfo) []SessionSummary { + if len(infos) == 0 { + return nil + } + out := make([]SessionSummary, 0, len(infos)) + for _, info := range infos { + out = append(out, SessionSummary{ + Path: info.Path, + ID: info.ID, + Name: info.Name, + Cwd: info.Cwd, + Created: info.Created, + Modified: info.Modified, + MessageCount: info.MessageCount, + FirstMessage: info.FirstMessage, + }) + } + return out +} + +// ExportSession copies the active session's JSONL file to dstPath. When +// dstPath is empty, a name is derived from the session's display name (or a +// short form of its ID when unnamed) and written to the process's working +// directory. +// +// It returns the path written and the number of bytes copied. Returns +// ErrNoSession when no tree session is active, and ErrSessionNotPersisted +// when the session exists only in memory and so has nothing to copy. +func (a *App) ExportSession(dstPath string) (string, int, error) { + snap, ok := a.SessionSnapshot() + if !ok { + return "", 0, ErrNoSession + } + if snap.FilePath == "" { + return "", 0, ErrSessionNotPersisted + } + if dstPath == "" { + dstPath = defaultExportName(snap) + } + + data, err := os.ReadFile(snap.FilePath) + if err != nil { + return "", 0, fmt.Errorf("read session file: %w", err) + } + if err := os.WriteFile(dstPath, data, 0o644); err != nil { + return "", 0, fmt.Errorf("write export file %q: %w", dstPath, err) + } + return dstPath, len(data), nil +} + +// defaultExportName builds the fallback file name for an exported session. +func defaultExportName(snap SessionSnapshot) string { + name := snap.Name + if name == "" { + name = shortSessionID(snap.ID) + } + return fmt.Sprintf("session_%s.jsonl", sanitizeFileName(name)) +} + +// WriteShareableSession writes a shareable copy of the active session to a +// temporary file and returns its path. The copy is the session's JSONL with a +// system-prompt entry inserted directly after the header, so whoever reads +// the shared file can see the system prompt and model the conversation ran +// under. fallbackModelID is used when the session records no model of its own. +// +// The caller owns the returned file and is responsible for removing it. On +// error no file is left behind. Returns ErrNoSession when no tree session is +// active and ErrSessionNotPersisted when the session is in-memory. +func (a *App) WriteShareableSession(systemPrompt, fallbackModelID string) (string, error) { + snap, ok := a.SessionSnapshot() + if !ok { + return "", ErrNoSession + } + if snap.FilePath == "" { + return "", ErrSessionNotPersisted + } + + data, err := os.ReadFile(snap.FilePath) + if err != nil { + return "", fmt.Errorf("read session file: %w", err) + } + sysPromptJSON, err := a.systemPromptEntry(systemPrompt, fallbackModelID) + if err != nil { + return "", err + } + + name := snap.Name + if name == "" { + name = "session" + } + return writeShareFile(sanitizeFileName(name), data, sysPromptJSON) +} + +// systemPromptEntry marshals a system-prompt entry describing the given +// system prompt together with the model and provider currently in effect for +// the session. fallbackModelID is used when the session records no model +// change of its own. +func (a *App) systemPromptEntry(systemPrompt, fallbackModelID string) ([]byte, error) { + tm := a.opts.TreeSession + if tm == nil { + return nil, ErrNoSession + } + _, provider, modelID := tm.BuildContext() + if modelID == "" { + modelID = fallbackModelID + } + data, err := session.MarshalEntry(session.NewSystemPromptEntry(systemPrompt, modelID, provider)) + if err != nil { + return nil, fmt.Errorf("marshal system prompt entry: %w", err) + } + return data, nil +} + +// writeShareFile writes data to a temporary JSONL file with sysPromptJSON +// spliced in after the header line, and returns the temp file's path. +func writeShareFile(name string, data, sysPromptJSON []byte) (tmpPath string, err error) { + tmpFile, err := os.CreateTemp("", fmt.Sprintf("kit-%s-*.jsonl", name)) + if err != nil { + return "", fmt.Errorf("create temp file: %w", err) + } + tmpPath = tmpFile.Name() + defer func() { + _ = tmpFile.Close() + if err != nil { + _ = os.Remove(tmpPath) + } + }() + + // The header is the first line, so we write: + // 1. First line (header) from the original data + // 2. System prompt entry + // 3. Remaining lines from the original data + lines := strings.Split(string(data), "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] // Remove trailing empty line + } + if len(lines) == 0 { + return tmpPath, nil + } + + if _, err = tmpFile.WriteString(lines[0] + "\n"); err != nil { + return "", fmt.Errorf("write temp file: %w", err) + } + if _, err = tmpFile.Write(sysPromptJSON); err != nil { + return "", fmt.Errorf("write system prompt: %w", err) + } + if _, err = tmpFile.WriteString("\n"); err != nil { + return "", fmt.Errorf("write temp file: %w", err) + } + for i := 1; i < len(lines); i++ { + if lines[i] == "" { + continue // Skip empty lines + } + if _, err = tmpFile.WriteString(lines[i] + "\n"); err != nil { + return "", fmt.Errorf("write temp file: %w", err) + } + } + return tmpPath, nil +} + +// sanitizeFileName replaces path separators and other characters that are +// awkward in file names so a session name can be used to build one. +func sanitizeFileName(name string) string { + return strings.Map(func(r rune) rune { + if r == '/' || r == '\\' || r == ':' || r == ' ' { + return '_' + } + return r + }, name) +} + +// shortSessionID returns a file-name-friendly prefix of a session ID, used as +// a fallback export name for unnamed sessions. +func shortSessionID(id string) string { + if len(id) > 12 { + return id[:12] + } + return id +} diff --git a/internal/app/session_store_test.go b/internal/app/session_store_test.go new file mode 100644 index 00000000..82ecfdff --- /dev/null +++ b/internal/app/session_store_test.go @@ -0,0 +1,447 @@ +package app + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mark3labs/kit/internal/session" +) + +// newPersistedApp creates an App backed by a session that is written to disk +// under a sandboxed HOME, and returns the app plus its tree manager. +func newPersistedApp(t *testing.T) (*App, *session.TreeManager) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + + tm, err := session.CreateTreeSession(t.TempDir()) + if err != nil { + t.Fatalf("CreateTreeSession: %v", err) + } + t.Cleanup(func() { _ = tm.Close() }) + return New(Options{TreeSession: tm}, nil), tm +} + +// -------------------------------------------------------------------------- +// Listing +// -------------------------------------------------------------------------- + +func TestListSessionsEmptyCwd(t *testing.T) { + a := New(Options{}, nil) + + got, err := a.ListSessions("") + if err != nil { + t.Fatalf("ListSessions: %v", err) + } + if got != nil { + t.Errorf("ListSessions(\"\") = %v, want nil", got) + } +} + +func TestListSessionsProjectsSummary(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + cwd := t.TempDir() + + tm, err := session.CreateTreeSession(cwd) + if err != nil { + t.Fatalf("CreateTreeSession: %v", err) + } + appendUserMessage(t, tm, "first question") + if _, err := tm.AppendSessionInfo("named session"); err != nil { + t.Fatalf("AppendSessionInfo: %v", err) + } + if err := tm.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + + a := New(Options{}, nil) + got, err := a.ListSessions(cwd) + if err != nil { + t.Fatalf("ListSessions: %v", err) + } + if len(got) != 1 { + t.Fatalf("got %d summaries, want 1", len(got)) + } + + s := got[0] + if s.Path != tm.GetFilePath() { + t.Errorf("Path = %q, want %q", s.Path, tm.GetFilePath()) + } + if s.ID != tm.GetHeader().ID { + t.Errorf("ID = %q, want %q", s.ID, tm.GetHeader().ID) + } + if s.Name != "named session" { + t.Errorf("Name = %q, want %q", s.Name, "named session") + } + if s.Cwd != cwd { + t.Errorf("Cwd = %q, want %q", s.Cwd, cwd) + } + if s.MessageCount != 1 { + t.Errorf("MessageCount = %d, want 1", s.MessageCount) + } + if s.FirstMessage != "first question" { + t.Errorf("FirstMessage = %q, want %q", s.FirstMessage, "first question") + } + if s.Created.IsZero() { + t.Error("Created is zero, want the session's creation time") + } + if s.Modified.IsZero() { + t.Error("Modified is zero, want the last activity time") + } +} + +func TestListAllSessionsSpansWorkingDirectories(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + for _, cwd := range []string{t.TempDir(), t.TempDir()} { + tm, err := session.CreateTreeSession(cwd) + if err != nil { + t.Fatalf("CreateTreeSession: %v", err) + } + appendUserMessage(t, tm, "hello from "+cwd) + if err := tm.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + } + + a := New(Options{}, nil) + got, err := a.ListAllSessions() + if err != nil { + t.Fatalf("ListAllSessions: %v", err) + } + if len(got) != 2 { + t.Fatalf("got %d summaries, want 2 (one per working directory)", len(got)) + } +} + +func TestListSessionsNoSessionsYet(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + a := New(Options{}, nil) + + got, err := a.ListSessions(t.TempDir()) + if err != nil { + t.Fatalf("ListSessions: %v", err) + } + if len(got) != 0 { + t.Errorf("got %d summaries, want 0", len(got)) + } +} + +// -------------------------------------------------------------------------- +// Deleting +// -------------------------------------------------------------------------- + +func TestDeleteSession(t *testing.T) { + a, tm := newPersistedApp(t) + path := tm.GetFilePath() + + if err := a.DeleteSession(path); err != nil { + t.Fatalf("DeleteSession: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("session file still present after delete (stat err = %v)", err) + } +} + +func TestDeleteSessionRequiresPath(t *testing.T) { + a := New(Options{}, nil) + + if err := a.DeleteSession(""); err == nil { + t.Error("DeleteSession(\"\") = nil, want an error") + } +} + +func TestDeleteSessionMissingFile(t *testing.T) { + a := New(Options{}, nil) + + err := a.DeleteSession(filepath.Join(t.TempDir(), "absent.jsonl")) + if err == nil { + t.Fatal("DeleteSession on a missing file = nil, want an error") + } + if !strings.Contains(err.Error(), "absent.jsonl") { + t.Errorf("error %q does not name the offending path", err) + } +} + +// -------------------------------------------------------------------------- +// Export +// -------------------------------------------------------------------------- + +func TestExportSessionNoSession(t *testing.T) { + a := New(Options{}, nil) + + if _, _, err := a.ExportSession(""); !errors.Is(err, ErrNoSession) { + t.Errorf("ExportSession err = %v, want ErrNoSession", err) + } +} + +func TestExportSessionInMemory(t *testing.T) { + a, _ := newSessionApp(t) + + if _, _, err := a.ExportSession(""); !errors.Is(err, ErrSessionNotPersisted) { + t.Errorf("ExportSession err = %v, want ErrSessionNotPersisted", err) + } +} + +func TestExportSessionToExplicitPath(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + + dst := filepath.Join(t.TempDir(), "out.jsonl") + gotPath, written, err := a.ExportSession(dst) + if err != nil { + t.Fatalf("ExportSession: %v", err) + } + if gotPath != dst { + t.Errorf("path = %q, want %q", gotPath, dst) + } + + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if written != len(data) { + t.Errorf("reported %d bytes, file holds %d", written, len(data)) + } + + src, err := os.ReadFile(tm.GetFilePath()) + if err != nil { + t.Fatalf("ReadFile source: %v", err) + } + if string(data) != string(src) { + t.Error("exported file is not a byte-for-byte copy of the session") + } +} + +func TestExportSessionDerivesNameFromSessionName(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + if _, err := tm.AppendSessionInfo("my great/session"); err != nil { + t.Fatalf("AppendSessionInfo: %v", err) + } + + t.Chdir(t.TempDir()) + gotPath, _, err := a.ExportSession("") + if err != nil { + t.Fatalf("ExportSession: %v", err) + } + // Separators and spaces are replaced so the name is usable as a file name. + if gotPath != "session_my_great_session.jsonl" { + t.Errorf("path = %q, want the sanitised session name", gotPath) + } + if _, err := os.Stat(gotPath); err != nil { + t.Errorf("expected the export at %q: %v", gotPath, err) + } +} + +func TestExportSessionDerivesNameFromIDWhenUnnamed(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + + t.Chdir(t.TempDir()) + gotPath, _, err := a.ExportSession("") + if err != nil { + t.Fatalf("ExportSession: %v", err) + } + want := "session_" + shortSessionID(tm.GetHeader().ID) + ".jsonl" + if gotPath != want { + t.Errorf("path = %q, want %q", gotPath, want) + } +} + +func TestExportSessionUnwritableDestination(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + + // A path whose parent directory does not exist. + dst := filepath.Join(t.TempDir(), "missing", "out.jsonl") + if _, _, err := a.ExportSession(dst); err == nil { + t.Fatal("ExportSession to an unwritable path = nil, want an error") + } +} + +// -------------------------------------------------------------------------- +// Share +// -------------------------------------------------------------------------- + +func TestWriteShareableSessionNoSession(t *testing.T) { + a := New(Options{}, nil) + + if _, err := a.WriteShareableSession("prompt", "model"); !errors.Is(err, ErrNoSession) { + t.Errorf("WriteShareableSession err = %v, want ErrNoSession", err) + } +} + +func TestWriteShareableSessionInMemory(t *testing.T) { + a, _ := newSessionApp(t) + + if _, err := a.WriteShareableSession("prompt", "model"); !errors.Is(err, ErrSessionNotPersisted) { + t.Errorf("WriteShareableSession err = %v, want ErrSessionNotPersisted", err) + } +} + +func TestWriteShareableSessionSplicesSystemPrompt(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + if _, err := tm.AppendModelChange("anthropic", "claude-sonnet-4-5"); err != nil { + t.Fatalf("AppendModelChange: %v", err) + } + + tmpPath, err := a.WriteShareableSession("be helpful", "fallback-model") + if err != nil { + t.Fatalf("WriteShareableSession: %v", err) + } + t.Cleanup(func() { _ = os.Remove(tmpPath) }) + + data, err := os.ReadFile(tmpPath) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + lines := strings.Split(strings.TrimRight(string(data), "\n"), "\n") + if len(lines) < 3 { + t.Fatalf("got %d lines, want at least header + system prompt + message", len(lines)) + } + + // Line 0 must remain the session header. + header, err := session.UnmarshalEntry([]byte(lines[0])) + if err != nil { + t.Fatalf("UnmarshalEntry(line 0): %v", err) + } + h, ok := header.(*session.SessionHeader) + if !ok { + t.Fatalf("line 0 is %T, want *session.SessionHeader", header) + } + if h.ID != tm.GetHeader().ID { + t.Errorf("header ID = %q, want %q", h.ID, tm.GetHeader().ID) + } + + // Line 1 must be the spliced-in system prompt. + entry, err := session.UnmarshalEntry([]byte(lines[1])) + if err != nil { + t.Fatalf("UnmarshalEntry(line 1): %v", err) + } + sp, ok := entry.(*session.SystemPromptEntry) + if !ok { + t.Fatalf("line 1 is %T, want *session.SystemPromptEntry", entry) + } + if sp.Content != "be helpful" { + t.Errorf("Content = %q, want %q", sp.Content, "be helpful") + } + if sp.Model != "claude-sonnet-4-5" { + t.Errorf("Model = %q, want the session's model", sp.Model) + } + if sp.Provider != "anthropic" { + t.Errorf("Provider = %q, want %q", sp.Provider, "anthropic") + } +} + +func TestWriteShareableSessionFallsBackToGivenModel(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + + tmpPath, err := a.WriteShareableSession("be helpful", "fallback-model") + if err != nil { + t.Fatalf("WriteShareableSession: %v", err) + } + t.Cleanup(func() { _ = os.Remove(tmpPath) }) + + data, err := os.ReadFile(tmpPath) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + lines := strings.Split(strings.TrimRight(string(data), "\n"), "\n") + entry, err := session.UnmarshalEntry([]byte(lines[1])) + if err != nil { + t.Fatalf("UnmarshalEntry: %v", err) + } + sp := entry.(*session.SystemPromptEntry) + if sp.Model != "fallback-model" { + t.Errorf("Model = %q, want the fallback when the session records none", sp.Model) + } +} + +func TestWriteShareableSessionPreservesAllEntries(t *testing.T) { + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "one") + appendUserMessage(t, tm, "two") + + src, err := os.ReadFile(tm.GetFilePath()) + if err != nil { + t.Fatalf("ReadFile source: %v", err) + } + srcLines := strings.Split(strings.TrimRight(string(src), "\n"), "\n") + + tmpPath, err := a.WriteShareableSession("be helpful", "model") + if err != nil { + t.Fatalf("WriteShareableSession: %v", err) + } + t.Cleanup(func() { _ = os.Remove(tmpPath) }) + + data, err := os.ReadFile(tmpPath) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + shared := strings.Split(strings.TrimRight(string(data), "\n"), "\n") + + // The shared file is the original plus exactly one system-prompt line. + if len(shared) != len(srcLines)+1 { + t.Fatalf("shared file has %d lines, want %d (original + system prompt)", len(shared), len(srcLines)+1) + } + for i, line := range srcLines[1:] { + if shared[i+2] != line { + t.Errorf("entry %d changed:\n got %q\nwant %q", i, shared[i+2], line) + } + } +} + +// -------------------------------------------------------------------------- +// File name helpers +// -------------------------------------------------------------------------- + +func TestSanitizeFileName(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"plain", "session", "session"}, + {"spaces", "my session", "my_session"}, + {"unix separator", "a/b", "a_b"}, + {"windows separator", `a\b`, "a_b"}, + {"colon", "a:b", "a_b"}, + {"mixed", `my project:/a b`, "my_project__a_b"}, + {"empty", "", ""}, + {"unicode kept", "días", "días"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := sanitizeFileName(tt.in); got != tt.want { + t.Errorf("sanitizeFileName(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestShortSessionID(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"long is truncated", "0123456789abcdef", "0123456789ab"}, + {"exact length", "0123456789ab", "0123456789ab"}, + // A short ID must be returned as-is rather than panicking on a slice + // past the end. + {"short is kept", "abc", "abc"}, + {"empty", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := shortSessionID(tt.in); got != tt.want { + t.Errorf("shortSessionID(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} diff --git a/internal/app/session_view.go b/internal/app/session_view.go index 968eb123..9a496acb 100644 --- a/internal/app/session_view.go +++ b/internal/app/session_view.go @@ -266,26 +266,3 @@ func (a *App) ForkSession(cwd, targetID string) error { a.SwitchTreeSession(ts) return nil } - -// SessionSystemPromptEntry marshals a system-prompt entry describing the -// given system prompt together with the model and provider currently in -// effect for the session. It is embedded in exported and shared session files -// so a reader can reconstruct the context the conversation ran under. -// -// fallbackModelID is used when the session records no model change of its -// own. Returns ErrNoSession when no tree session is active. -func (a *App) SessionSystemPromptEntry(systemPrompt, fallbackModelID string) ([]byte, error) { - tm := a.opts.TreeSession - if tm == nil { - return nil, ErrNoSession - } - _, provider, modelID := tm.BuildContext() - if modelID == "" { - modelID = fallbackModelID - } - data, err := session.MarshalEntry(session.NewSystemPromptEntry(systemPrompt, modelID, provider)) - if err != nil { - return nil, fmt.Errorf("marshal system prompt entry: %w", err) - } - return data, nil -} diff --git a/internal/app/session_view_test.go b/internal/app/session_view_test.go index ac5ebe79..a9211e16 100644 --- a/internal/app/session_view_test.go +++ b/internal/app/session_view_test.go @@ -267,9 +267,6 @@ func TestSessionMutationsWithoutSession(t *testing.T) { if err := a.ForkSession(t.TempDir(), "abc"); !errors.Is(err, ErrNoSession) { t.Errorf("ForkSession err = %v, want ErrNoSession", err) } - if _, err := a.SessionSystemPromptEntry("prompt", "model"); !errors.Is(err, ErrNoSession) { - t.Errorf("SessionSystemPromptEntry err = %v, want ErrNoSession", err) - } } func TestSetSessionName(t *testing.T) { @@ -286,52 +283,3 @@ func TestSetSessionName(t *testing.T) { t.Errorf("Name = %q, want %q", snap.Name, "renamed") } } - -func TestSessionSystemPromptEntryUsesSessionModel(t *testing.T) { - a, tm := newSessionApp(t) - appendUserMessage(t, tm, "hello") - if _, err := tm.AppendModelChange("anthropic", "claude-sonnet-4-5"); err != nil { - t.Fatalf("AppendModelChange: %v", err) - } - - data, err := a.SessionSystemPromptEntry("be helpful", "fallback-model") - if err != nil { - t.Fatalf("SessionSystemPromptEntry: %v", err) - } - - entry, err := session.UnmarshalEntry(data) - if err != nil { - t.Fatalf("UnmarshalEntry: %v", err) - } - sp, ok := entry.(*session.SystemPromptEntry) - if !ok { - t.Fatalf("got %T, want *session.SystemPromptEntry", entry) - } - if sp.Content != "be helpful" { - t.Errorf("Content = %q, want %q", sp.Content, "be helpful") - } - if sp.Model != "claude-sonnet-4-5" { - t.Errorf("Model = %q, want the session's model", sp.Model) - } - if sp.Provider != "anthropic" { - t.Errorf("Provider = %q, want %q", sp.Provider, "anthropic") - } -} - -func TestSessionSystemPromptEntryFallsBackToGivenModel(t *testing.T) { - a, tm := newSessionApp(t) - appendUserMessage(t, tm, "hello") - - data, err := a.SessionSystemPromptEntry("be helpful", "fallback-model") - if err != nil { - t.Fatalf("SessionSystemPromptEntry: %v", err) - } - entry, err := session.UnmarshalEntry(data) - if err != nil { - t.Fatalf("UnmarshalEntry: %v", err) - } - sp := entry.(*session.SystemPromptEntry) - if sp.Model != "fallback-model" { - t.Errorf("Model = %q, want the fallback when the session records none", sp.Model) - } -} diff --git a/internal/ui/model.go b/internal/ui/model.go index 04c49dc5..aab6314e 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "os" "os/exec" @@ -121,11 +122,23 @@ type AppController interface { // ForkSession creates a new session in cwd holding the history up to // targetID and switches to it. Used by /fork and tree-node selection. ForkSession(cwd, targetID string) error - // SessionSystemPromptEntry marshals a system-prompt entry describing the - // given prompt plus the session's current model/provider, for embedding - // in exported and shared session files. fallbackModelID is used when the - // session records no model of its own. - SessionSystemPromptEntry(systemPrompt, fallbackModelID string) ([]byte, error) + // ListSessions returns summaries of the sessions recorded for cwd, + // newest first. Used by the session picker. + ListSessions(cwd string) ([]app.SessionSummary, error) + // ListAllSessions returns summaries of every session across all working + // directories, newest first. Used by the session picker's "All" scope. + ListAllSessions() ([]app.SessionSummary, error) + // DeleteSession removes a session file from disk. Used by the session + // picker's delete flow. + DeleteSession(path string) error + // ExportSession copies the active session's file to dstPath, deriving a + // name when dstPath is empty, and reports the path written and byte + // count. Used by /export. + ExportSession(dstPath string) (string, int, error) + // WriteShareableSession writes a shareable copy of the active session + // (with a system-prompt entry spliced in) to a temporary file and returns + // its path. The caller must remove the file. Used by /share. + WriteShareableSession(systemPrompt, fallbackModelID string) (string, error) // SendUIMessage re-injects a UI-internal message into the program's Update // loop asynchronously. Safe to call from any goroutine. Used by extension // command goroutines (and other async UI work) to deliver results back to @@ -1150,7 +1163,7 @@ func NewAppModel(appCtrl AppController, opts AppModelOptions) *AppModel { // If --resume was passed, open the session picker immediately. if opts.ShowSessionPicker { - m.sessionSelector = NewSessionSelector(opts.Cwd, width, height) + m.sessionSelector = NewSessionSelector(appCtrl, opts.Cwd, width, height) m.state = stateSessionSelector } @@ -5590,44 +5603,17 @@ func (m *AppModel) handleEditCommand(args string) tea.Cmd { // // /export path.jsonl — copies to the specified path. func (m *AppModel) handleExportCommand(args string) tea.Cmd { - snap, ok := m.appCtrl.SessionSnapshot() - if !ok { + dstPath, written, err := m.appCtrl.ExportSession(args) + switch { + case errors.Is(err, app.ErrNoSession): m.printSystemMessage("No tree session active.") - return nil - } - - srcPath := snap.FilePath - if srcPath == "" { + case errors.Is(err, app.ErrSessionNotPersisted): m.printSystemMessage("Session is in-memory (not persisted). Nothing to export.") - return nil - } - - // Determine destination path. - dstPath := args - if dstPath == "" { - // Generate a name based on session name or ID. - name := snap.Name - if name == "" { - name = shortSessionID(snap.ID) - } - // Sanitize for filename. - name = sanitizeFileName(name) - dstPath = fmt.Sprintf("session_%s.jsonl", name) - } - - // Copy the file. - data, err := os.ReadFile(srcPath) - if err != nil { - m.printSystemMessage(fmt.Sprintf("Failed to read session file: %v", err)) - return nil - } - - if err := os.WriteFile(dstPath, data, 0644); err != nil { - m.printSystemMessage(fmt.Sprintf("Failed to write export file: %v", err)) - return nil + case err != nil: + m.printSystemMessage(fmt.Sprintf("Failed to export session: %v", err)) + default: + m.printSystemMessage(fmt.Sprintf("Session exported to: %s (%d bytes)", dstPath, written)) } - - m.printSystemMessage(fmt.Sprintf("Session exported to: %s (%d bytes)", dstPath, len(data))) return nil } @@ -5635,14 +5621,14 @@ func (m *AppModel) handleExportCommand(args string) tea.Cmd { // a shareable viewer URL. Requires the GitHub CLI (gh) to be installed and // authenticated. func (m *AppModel) handleShareCommand() tea.Cmd { + // Check the session is shareable before probing for gh, so an in-memory + // session reports that rather than a missing-CLI error. snap, ok := m.appCtrl.SessionSnapshot() if !ok { m.printSystemMessage("No tree session active.") return nil } - - srcPath := snap.FilePath - if srcPath == "" { + if snap.FilePath == "" { m.printSystemMessage("Session is in-memory (not persisted). Nothing to share.") return nil } @@ -5660,32 +5646,12 @@ func (m *AppModel) handleShareCommand() tea.Cmd { return nil } - // Read the original session file. - data, err := os.ReadFile(srcPath) - if err != nil { - m.printSystemMessage(fmt.Sprintf("Failed to read session file: %v", err)) - return nil - } - - // Capture the current system prompt and model info as a system-prompt - // entry so the shared file records the context the conversation ran under. - sysPromptJSON, err := m.appCtrl.SessionSystemPromptEntry( + // The app layer writes the session plus a system-prompt entry recording + // the context the conversation ran under; we own the temp file from here. + tmpPath, err := m.appCtrl.WriteShareableSession( viper.GetString("system-prompt"), viper.GetString("model"), ) - if err != nil { - m.printSystemMessage(fmt.Sprintf("Failed to marshal system prompt: %v", err)) - return nil - } - - name := snap.Name - if name == "" { - name = "session" - } - // Sanitize for filename. - name = sanitizeFileName(name) - - tmpPath, err := buildShareFile(name, data, sysPromptJSON) if err != nil { m.printSystemMessage(fmt.Sprintf("Failed to share session: %v", err)) return nil @@ -5715,56 +5681,6 @@ func (m *AppModel) handleShareCommand() tea.Cmd { } } -// buildShareFile assembles a temp JSONL file containing the session data -// with the system-prompt entry inserted after the header line. On success -// the caller owns the returned file and must remove it when done; on error -// any partially-written temp file has already been cleaned up. -func buildShareFile(name string, data, sysPromptJSON []byte) (tmpPath string, err error) { - tmpFile, err := os.CreateTemp("", fmt.Sprintf("kit-%s-*.jsonl", name)) - if err != nil { - return "", fmt.Errorf("create temp file: %w", err) - } - tmpPath = tmpFile.Name() - defer func() { - _ = tmpFile.Close() - if err != nil { - _ = os.Remove(tmpPath) - } - }() - - // Write the session data with the system prompt entry inserted after the - // header. The header is the first line, so we write: - // 1. First line (header) from original data - // 2. System prompt entry - // 3. Remaining lines from original data - lines := strings.Split(string(data), "\n") - if len(lines) > 0 && lines[len(lines)-1] == "" { - lines = lines[:len(lines)-1] // Remove trailing empty line - } - if len(lines) == 0 { - return tmpPath, nil - } - - if _, err = tmpFile.WriteString(lines[0] + "\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) - } - if _, err = tmpFile.Write(sysPromptJSON); err != nil { - return "", fmt.Errorf("write system prompt: %w", err) - } - if _, err = tmpFile.WriteString("\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) - } - for i := 1; i < len(lines); i++ { - if lines[i] == "" { - continue // Skip empty lines - } - if _, err = tmpFile.WriteString(lines[i] + "\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) - } - } - return tmpPath, nil -} - // handleImportCommand imports a session from a JSONL file. // Usage: /import path.jsonl func (m *AppModel) handleImportCommand(args string) tea.Cmd { @@ -5801,7 +5717,7 @@ func (m *AppModel) handleResumeCommand() tea.Cmd { return nil } - m.sessionSelector = NewSessionSelector(m.cwd, m.width, m.height) + m.sessionSelector = NewSessionSelector(m.appCtrl, m.cwd, m.width, m.height) m.state = stateSessionSelector return nil } @@ -5917,27 +5833,6 @@ func (m *AppModel) renderSessionHistory() { m.pendingGotoBottom = true } -// sanitizeFileName replaces path separators and other characters that are -// awkward in file names with underscores, so a user-chosen session name can -// be embedded in an export/share filename safely. -func sanitizeFileName(name string) string { - return strings.Map(func(r rune) rune { - if r == '/' || r == '\\' || r == ':' || r == ' ' { - return '_' - } - return r - }, name) -} - -// shortSessionID returns a filename-friendly prefix of a session ID, used as -// a fallback export name for unnamed sessions. -func shortSessionID(id string) string { - if len(id) > 12 { - return id[:12] - } - return id -} - // handleSessionInfoCommand shows session statistics. func (m *AppModel) handleSessionInfoCommand() tea.Cmd { snap, ok := m.appCtrl.SessionSnapshot() diff --git a/internal/ui/model_test.go b/internal/ui/model_test.go index 63e7a58c..f6c29699 100644 --- a/internal/ui/model_test.go +++ b/internal/ui/model_test.go @@ -30,6 +30,15 @@ type stubAppController struct { // and sessionHistory is what SessionHistory returns for it. hasSession bool sessionHistory []message.Message + + // sessions/allSessions back the session listing methods, deleted records + // the paths passed to DeleteSession, and listErr/deleteErr let a test + // force the failure paths. + sessions []app.SessionSummary + allSessions []app.SessionSummary + deleted []string + listErr error + deleteErr error } func (s *stubAppController) Run(prompt string) int { @@ -89,8 +98,25 @@ func (s *stubAppController) ForkSession(_, _ string) error { return app.ErrNoSession } -func (s *stubAppController) SessionSystemPromptEntry(_, _ string) ([]byte, error) { - return nil, app.ErrNoSession +func (s *stubAppController) ListSessions(_ string) ([]app.SessionSummary, error) { + return s.sessions, s.listErr +} + +func (s *stubAppController) ListAllSessions() ([]app.SessionSummary, error) { + return s.allSessions, s.listErr +} + +func (s *stubAppController) DeleteSession(path string) error { + s.deleted = append(s.deleted, path) + return s.deleteErr +} + +func (s *stubAppController) ExportSession(_ string) (string, int, error) { + return "", 0, app.ErrNoSession +} + +func (s *stubAppController) WriteShareableSession(_, _ string) (string, error) { + return "", app.ErrNoSession } func (s *stubAppController) SendUIMessage(_ tea.Msg) { diff --git a/internal/ui/session_selector.go b/internal/ui/session_selector.go index ebdf5a42..dc637908 100644 --- a/internal/ui/session_selector.go +++ b/internal/ui/session_selector.go @@ -10,7 +10,7 @@ import ( tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" - "github.com/mark3labs/kit/internal/session" + "github.com/mark3labs/kit/internal/app" "github.com/mark3labs/kit/internal/ui/style" ) @@ -61,14 +61,31 @@ func (m SessionFilterMode) String() string { // controlCharsRe matches ASCII control characters for stripping from previews. var controlCharsRe = regexp.MustCompile(`[\x00-\x1f\x7f]`) +// SessionStore is the slice of the app layer the session picker needs: it +// lists the sessions on disk and deletes them. The picker owns the loading +// itself (rather than being handed a list) because it reloads across scope +// toggles and mutates the list in place after a delete. +type SessionStore interface { + // ListSessions returns summaries of the sessions recorded for cwd, + // newest first. + ListSessions(cwd string) ([]app.SessionSummary, error) + // ListAllSessions returns summaries of every session across all working + // directories, newest first. + ListAllSessions() ([]app.SessionSummary, error) + // DeleteSession removes a session file from disk. + DeleteSession(path string) error +} + // SessionSelectorComponent is a Bubble Tea component that lets the user browse // and select from available sessions. It wraps PopupList in FullScreen mode: // PopupList owns the cursor/search/scroll math/chrome; this component owns // the session list, scope/filter toggles, and delete-confirmation flow. type SessionSelectorComponent struct { - allSessions []session.SessionInfo - cwdSessions []session.SessionInfo - filtered []session.SessionInfo // matches popup.Items() 1:1 + store SessionStore + + allSessions []app.SessionSummary + cwdSessions []app.SessionSummary + filtered []app.SessionSummary // matches popup.Items() 1:1 scope SessionScopeMode filter SessionFilterMode @@ -86,10 +103,11 @@ type SessionSelectorComponent struct { } // NewSessionSelector creates a session selector. It loads sessions for the -// current working directory and all sessions across projects. If cwd is -// empty, only "All" scope is available. -func NewSessionSelector(cwd string, width, height int) *SessionSelectorComponent { +// current working directory and all sessions across projects from store. If +// cwd is empty, only "All" scope is available. +func NewSessionSelector(store SessionStore, cwd string, width, height int) *SessionSelectorComponent { ss := &SessionSelectorComponent{ + store: store, width: width, height: height, active: true, @@ -98,10 +116,10 @@ func NewSessionSelector(cwd string, width, height int) *SessionSelectorComponent // Load sessions (errors are swallowed — empty list is fine). if cwd != "" { - ss.cwdSessions, _ = session.ListSessions(cwd) + ss.cwdSessions, _ = store.ListSessions(cwd) ss.scope = SessionScopeCwd } - ss.allSessions, _ = session.ListAllSessions() + ss.allSessions, _ = store.ListAllSessions() if cwd == "" || len(ss.cwdSessions) == 0 { ss.scope = SessionScopeAll @@ -145,7 +163,7 @@ func (ss *SessionSelectorComponent) Update(msg tea.Msg) (tea.Model, tea.Cmd) { ss.confirmDelete = -1 if idx < len(ss.filtered) { info := ss.filtered[idx] - if err := session.DeleteSession(info.Path); err == nil { + if err := ss.store.DeleteSession(info.Path); err == nil { name := sessionDisplayName(info) ss.removeSession(info.Path) ss.rebuild() @@ -257,7 +275,7 @@ func (ss *SessionSelectorComponent) IsActive() bool { // rebuild applies the scope and filter selections, then publishes the // resulting session list to the popup. func (ss *SessionSelectorComponent) rebuild() { - var source []session.SessionInfo + var source []app.SessionSummary if ss.scope == SessionScopeCwd { source = ss.cwdSessions } else { @@ -265,7 +283,7 @@ func (ss *SessionSelectorComponent) rebuild() { } if ss.filter == SessionFilterNamed { - var named []session.SessionInfo + var named []app.SessionSummary for _, s := range source { if s.Name != "" { named = append(named, s) @@ -291,12 +309,12 @@ func (ss *SessionSelectorComponent) rebuild() { } // syncFiltered refreshes the filtered slice from popup.Items() so cursor -// indices map back to session.SessionInfo for the parent. +// indices map back to app.SessionSummary for the parent. func (ss *SessionSelectorComponent) syncFiltered() { items := ss.popup.Items() - out := make([]session.SessionInfo, 0, len(items)) + out := make([]app.SessionSummary, 0, len(items)) for _, it := range items { - if s, ok := it.Meta.(session.SessionInfo); ok { + if s, ok := it.Meta.(app.SessionSummary); ok { out = append(out, s) } } @@ -308,8 +326,8 @@ func (ss *SessionSelectorComponent) removeSession(path string) { ss.allSessions = removeByPath(ss.allSessions, path) } -func removeByPath(sessions []session.SessionInfo, path string) []session.SessionInfo { - result := make([]session.SessionInfo, 0, len(sessions)) +func removeByPath(sessions []app.SessionSummary, path string) []app.SessionSummary { + result := make([]app.SessionSummary, 0, len(sessions)) for _, s := range sessions { if s.Path != path { result = append(result, s) @@ -328,7 +346,7 @@ func removeByPath(sessions []session.SessionInfo, path string) []session.Session // because each inner Render emits an ANSI reset that drops the background. func (ss *SessionSelectorComponent) renderEntry(item PopupItem, innerWidth int, isCursor bool) string { theme := style.GetTheme() - info, ok := item.Meta.(session.SessionInfo) + info, ok := item.Meta.(app.SessionSummary) if !ok { return item.Label } @@ -392,7 +410,7 @@ func (ss *SessionSelectorComponent) renderEntry(item PopupItem, innerWidth int, // sessionDisplayName returns the best display string for a session: // the name if set, the first message, or a fallback. -func sessionDisplayName(info session.SessionInfo) string { +func sessionDisplayName(info app.SessionSummary) string { if info.Name != "" { return info.Name } diff --git a/internal/ui/session_selector_test.go b/internal/ui/session_selector_test.go new file mode 100644 index 00000000..2b2602b7 --- /dev/null +++ b/internal/ui/session_selector_test.go @@ -0,0 +1,311 @@ +package ui + +import ( + "errors" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + + "github.com/mark3labs/kit/internal/app" +) + +// -------------------------------------------------------------------------- +// Stub SessionStore +// -------------------------------------------------------------------------- + +// stubSessionStore is an in-memory SessionStore for the picker tests. +type stubSessionStore struct { + cwdSessions []app.SessionSummary + allSessions []app.SessionSummary + + listErr error + deleteErr error + + cwdArg string + deleted []string +} + +func (s *stubSessionStore) ListSessions(cwd string) ([]app.SessionSummary, error) { + s.cwdArg = cwd + return s.cwdSessions, s.listErr +} + +func (s *stubSessionStore) ListAllSessions() ([]app.SessionSummary, error) { + return s.allSessions, s.listErr +} + +func (s *stubSessionStore) DeleteSession(path string) error { + if s.deleteErr != nil { + return s.deleteErr + } + s.deleted = append(s.deleted, path) + return nil +} + +// summary builds a SessionSummary with a distinct modification time so +// ordering is observable. +func summary(path, name, first string) app.SessionSummary { + return app.SessionSummary{ + Path: path, + ID: "id-" + path, + Name: name, + Cwd: "/work", + Created: time.Now().Add(-time.Hour), + Modified: time.Now().Add(-time.Minute), + MessageCount: 3, + FirstMessage: first, + } +} + +func keyPress(s string) tea.KeyPressMsg { + return tea.KeyPressMsg{Code: rune(s[0]), Text: s} +} + +// -------------------------------------------------------------------------- +// Loading +// -------------------------------------------------------------------------- + +func TestNewSessionSelectorLoadsFromStore(t *testing.T) { + store := &stubSessionStore{ + cwdSessions: []app.SessionSummary{summary("/a.jsonl", "", "hello")}, + allSessions: []app.SessionSummary{ + summary("/a.jsonl", "", "hello"), + summary("/b.jsonl", "other", "hi"), + }, + } + + ss := NewSessionSelector(store, "/work", 80, 24) + + if store.cwdArg != "/work" { + t.Errorf("ListSessions called with %q, want %q", store.cwdArg, "/work") + } + if ss.scope != SessionScopeCwd { + t.Errorf("scope = %v, want SessionScopeCwd when the cwd has sessions", ss.scope) + } + if len(ss.filtered) != 1 { + t.Errorf("got %d visible sessions, want 1 (cwd scope)", len(ss.filtered)) + } +} + +func TestNewSessionSelectorEmptyCwdUsesAllScope(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{summary("/a.jsonl", "", "hello")}, + } + + ss := NewSessionSelector(store, "", 80, 24) + + if ss.scope != SessionScopeAll { + t.Errorf("scope = %v, want SessionScopeAll when cwd is empty", ss.scope) + } + if len(ss.filtered) != 1 { + t.Errorf("got %d visible sessions, want 1", len(ss.filtered)) + } +} + +func TestNewSessionSelectorFallsBackToAllWhenCwdEmptyResult(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{summary("/a.jsonl", "", "hello")}, + } + + ss := NewSessionSelector(store, "/work", 80, 24) + + if ss.scope != SessionScopeAll { + t.Errorf("scope = %v, want SessionScopeAll when the cwd has no sessions", ss.scope) + } +} + +func TestNewSessionSelectorToleratesListErrors(t *testing.T) { + store := &stubSessionStore{listErr: errors.New("boom")} + + ss := NewSessionSelector(store, "/work", 80, 24) + + if len(ss.filtered) != 0 { + t.Errorf("got %d sessions, want 0 when listing fails", len(ss.filtered)) + } + if !ss.IsActive() { + t.Error("selector should still be active after a listing error") + } +} + +// -------------------------------------------------------------------------- +// Scope and filter +// -------------------------------------------------------------------------- + +func TestSessionSelectorScopeToggle(t *testing.T) { + store := &stubSessionStore{ + cwdSessions: []app.SessionSummary{summary("/a.jsonl", "", "hello")}, + allSessions: []app.SessionSummary{ + summary("/a.jsonl", "", "hello"), + summary("/b.jsonl", "other", "hi"), + }, + } + ss := NewSessionSelector(store, "/work", 80, 24) + + if len(ss.filtered) != 1 { + t.Fatalf("got %d sessions in cwd scope, want 1", len(ss.filtered)) + } + + ss.Update(tea.KeyPressMsg{Code: tea.KeyTab}) + if ss.scope != SessionScopeAll { + t.Fatalf("scope = %v, want SessionScopeAll after tab", ss.scope) + } + if len(ss.filtered) != 2 { + t.Errorf("got %d sessions in all scope, want 2", len(ss.filtered)) + } + + ss.Update(tea.KeyPressMsg{Code: tea.KeyTab}) + if ss.scope != SessionScopeCwd { + t.Errorf("scope = %v, want SessionScopeCwd after a second tab", ss.scope) + } +} + +func TestSessionSelectorNamedFilter(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{ + summary("/a.jsonl", "", "unnamed one"), + summary("/b.jsonl", "named", "hi"), + }, + } + ss := NewSessionSelector(store, "", 80, 24) + + if len(ss.filtered) != 2 { + t.Fatalf("got %d sessions, want 2 unfiltered", len(ss.filtered)) + } + + ss.Update(tea.KeyPressMsg{Code: 'n', Mod: tea.ModCtrl}) + if ss.filter != SessionFilterNamed { + t.Fatalf("filter = %v, want SessionFilterNamed", ss.filter) + } + if len(ss.filtered) != 1 { + t.Fatalf("got %d sessions, want only the named one", len(ss.filtered)) + } + if ss.filtered[0].Name != "named" { + t.Errorf("kept %q, want the named session", ss.filtered[0].Name) + } +} + +// -------------------------------------------------------------------------- +// Selection and deletion +// -------------------------------------------------------------------------- + +func TestSessionSelectorEmitsSelection(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{summary("/a.jsonl", "", "hello")}, + } + ss := NewSessionSelector(store, "", 80, 24) + + _, cmd := ss.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + if cmd == nil { + t.Fatal("expected a command on selection") + } + msg, ok := cmd().(SessionSelectedMsg) + if !ok { + t.Fatalf("got %T, want SessionSelectedMsg", cmd()) + } + if msg.Path != "/a.jsonl" { + t.Errorf("Path = %q, want %q", msg.Path, "/a.jsonl") + } + if ss.IsActive() { + t.Error("selector should be inactive after selecting") + } +} + +func TestSessionSelectorDeleteFlow(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{ + summary("/a.jsonl", "first", "hello"), + summary("/b.jsonl", "second", "hi"), + }, + } + ss := NewSessionSelector(store, "", 80, 24) + + // 'd' arms the confirmation but must not delete yet. + ss.Update(keyPress("d")) + if ss.confirmDelete != 0 { + t.Fatalf("confirmDelete = %d, want 0 (cursor row armed)", ss.confirmDelete) + } + if len(store.deleted) != 0 { + t.Fatalf("deleted %v before confirmation", store.deleted) + } + + // 'y' confirms. + _, cmd := ss.Update(keyPress("y")) + if len(store.deleted) != 1 || store.deleted[0] != "/a.jsonl" { + t.Fatalf("deleted = %v, want [/a.jsonl]", store.deleted) + } + if cmd == nil { + t.Fatal("expected a command after deleting") + } + msg, ok := cmd().(SessionDeletedMsg) + if !ok { + t.Fatalf("got %T, want SessionDeletedMsg", cmd()) + } + if msg.Name != "first" { + t.Errorf("Name = %q, want %q", msg.Name, "first") + } + if len(ss.filtered) != 1 || ss.filtered[0].Path != "/b.jsonl" { + t.Errorf("remaining = %v, want only /b.jsonl", ss.filtered) + } +} + +func TestSessionSelectorDeleteCancelled(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{summary("/a.jsonl", "first", "hello")}, + } + ss := NewSessionSelector(store, "", 80, 24) + + ss.Update(keyPress("d")) + ss.Update(keyPress("n")) + + if ss.confirmDelete != -1 { + t.Errorf("confirmDelete = %d, want -1 after declining", ss.confirmDelete) + } + if len(store.deleted) != 0 { + t.Errorf("deleted %v, want nothing after declining", store.deleted) + } + if len(ss.filtered) != 1 { + t.Errorf("got %d sessions, want the list untouched", len(ss.filtered)) + } +} + +func TestSessionSelectorDeleteErrorKeepsSession(t *testing.T) { + store := &stubSessionStore{ + allSessions: []app.SessionSummary{summary("/a.jsonl", "first", "hello")}, + deleteErr: errors.New("permission denied"), + } + ss := NewSessionSelector(store, "", 80, 24) + + ss.Update(keyPress("d")) + _, cmd := ss.Update(keyPress("y")) + + if cmd != nil { + t.Error("expected no SessionDeletedMsg when deletion fails") + } + if len(ss.filtered) != 1 { + t.Errorf("got %d sessions, want the entry kept when deletion fails", len(ss.filtered)) + } +} + +// -------------------------------------------------------------------------- +// Display +// -------------------------------------------------------------------------- + +func TestSessionDisplayName(t *testing.T) { + tests := []struct { + name string + in app.SessionSummary + want string + }{ + {"name wins", summary("/a", "my name", "first message"), "my name"}, + {"first message fallback", summary("/a", "", "first message"), "first message"}, + {"empty session", summary("/a", "", ""), "(empty session)"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := sessionDisplayName(tt.in); got != tt.want { + t.Errorf("sessionDisplayName() = %q, want %q", got, tt.want) + } + }) + } +} From 07f0c8411d0827f99485323091f3730a75248349 Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 30 Jul 2026 17:43:33 +0300 Subject: [PATCH 2/3] fix(app): address CodeRabbit review on session store (#101) - writeShareFile leaked its temp file on every write failure. tmpPath was a named result, so `return "", err` zeroed it before the deferred cleanup ran and os.Remove("") removed nothing. Split the function so the open file is passed to finishShareFile and the path is held in a local: the named-result/defer interaction that caused the leak is now structurally absent rather than worked around. Close is checked too, so a write that only fails on flush is no longer reported as success. - Log session-listing failures in the picker instead of discarding them; a permissions or disk error was indistinguishable from "no sessions". Skipped: atomic temp-and-rename for ExportSession. os.WriteFile writes through a symlinked destination whereas rename replaces it, so this changes behaviour beyond the refactor and is better decided separately. --- internal/app/session_store.go | 58 ++++++++---- internal/app/session_store_test.go | 142 +++++++++++++++++++++++++++++ internal/ui/session_selector.go | 18 +++- 3 files changed, 197 insertions(+), 21 deletions(-) diff --git a/internal/app/session_store.go b/internal/app/session_store.go index 5f4f5e8a..d48985c9 100644 --- a/internal/app/session_store.go +++ b/internal/app/session_store.go @@ -3,6 +3,7 @@ package app import ( "errors" "fmt" + "io" "os" "strings" "time" @@ -194,19 +195,40 @@ func (a *App) systemPromptEntry(systemPrompt, fallbackModelID string) ([]byte, e // writeShareFile writes data to a temporary JSONL file with sysPromptJSON // spliced in after the header line, and returns the temp file's path. -func writeShareFile(name string, data, sysPromptJSON []byte) (tmpPath string, err error) { +func writeShareFile(name string, data, sysPromptJSON []byte) (string, error) { tmpFile, err := os.CreateTemp("", fmt.Sprintf("kit-%s-*.jsonl", name)) if err != nil { return "", fmt.Errorf("create temp file: %w", err) } - tmpPath = tmpFile.Name() - defer func() { - _ = tmpFile.Close() - if err != nil { - _ = os.Remove(tmpPath) - } - }() + return finishShareFile(tmpFile, data, sysPromptJSON) +} + +// finishShareFile splices sysPromptJSON into data, writes the result to f and +// closes it. If anything fails, f is closed and removed so no partial share +// file is left behind, and the returned path is empty. +// +// The file is taken as a parameter rather than created here so that the +// cleanup-on-failure path is directly testable. +func finishShareFile(f *os.File, data, sysPromptJSON []byte) (string, error) { + path := f.Name() + if err := spliceShareEntries(f, data, sysPromptJSON); err != nil { + _ = f.Close() + _ = os.Remove(path) + return "", err + } + // Close is checked rather than deferred: a buffered write can surface its + // error only here, and reporting success for a truncated share file would + // be worse than failing outright. + if err := f.Close(); err != nil { + _ = os.Remove(path) + return "", fmt.Errorf("close temp file: %w", err) + } + return path, nil +} +// spliceShareEntries writes data to w with sysPromptJSON inserted directly +// after the header line. Empty lines in data are skipped. +func spliceShareEntries(w io.Writer, data, sysPromptJSON []byte) error { // The header is the first line, so we write: // 1. First line (header) from the original data // 2. System prompt entry @@ -216,27 +238,27 @@ func writeShareFile(name string, data, sysPromptJSON []byte) (tmpPath string, er lines = lines[:len(lines)-1] // Remove trailing empty line } if len(lines) == 0 { - return tmpPath, nil + return nil } - if _, err = tmpFile.WriteString(lines[0] + "\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) + if _, err := io.WriteString(w, lines[0]+"\n"); err != nil { + return fmt.Errorf("write temp file: %w", err) } - if _, err = tmpFile.Write(sysPromptJSON); err != nil { - return "", fmt.Errorf("write system prompt: %w", err) + if _, err := w.Write(sysPromptJSON); err != nil { + return fmt.Errorf("write system prompt: %w", err) } - if _, err = tmpFile.WriteString("\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) + if _, err := io.WriteString(w, "\n"); err != nil { + return fmt.Errorf("write temp file: %w", err) } for i := 1; i < len(lines); i++ { if lines[i] == "" { continue // Skip empty lines } - if _, err = tmpFile.WriteString(lines[i] + "\n"); err != nil { - return "", fmt.Errorf("write temp file: %w", err) + if _, err := io.WriteString(w, lines[i]+"\n"); err != nil { + return fmt.Errorf("write temp file: %w", err) } } - return tmpPath, nil + return nil } // sanitizeFileName replaces path separators and other characters that are diff --git a/internal/app/session_store_test.go b/internal/app/session_store_test.go index 82ecfdff..80cfadf8 100644 --- a/internal/app/session_store_test.go +++ b/internal/app/session_store_test.go @@ -445,3 +445,145 @@ func TestShortSessionID(t *testing.T) { }) } } + +// -------------------------------------------------------------------------- +// Share file assembly and cleanup +// -------------------------------------------------------------------------- + +// failingWriter fails on the Nth write, so each error branch of +// spliceShareEntries can be exercised in turn. +type failingWriter struct { + failOn int // 1-based index of the write that fails + n int +} + +func (w *failingWriter) Write(p []byte) (int, error) { + w.n++ + if w.n == w.failOn { + return 0, errors.New("disk full") + } + return len(p), nil +} + +func TestSpliceShareEntriesPropagatesWriteErrors(t *testing.T) { + data := []byte("header\nentry-1\nentry-2\n") + sysPrompt := []byte(`{"type":"system_prompt"}`) + + tests := []struct { + name string + failOn int + wantMsg string + }{ + {"header write", 1, "write temp file"}, + {"system prompt write", 2, "write system prompt"}, + {"separator write", 3, "write temp file"}, + {"entry write", 4, "write temp file"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := spliceShareEntries(&failingWriter{failOn: tt.failOn}, data, sysPrompt) + if err == nil { + t.Fatalf("write %d failed but spliceShareEntries returned nil", tt.failOn) + } + if !strings.Contains(err.Error(), tt.wantMsg) { + t.Errorf("error = %q, want it to mention %q", err, tt.wantMsg) + } + if !strings.Contains(err.Error(), "disk full") { + t.Errorf("error = %q, want the cause wrapped", err) + } + }) + } +} + +func TestSpliceShareEntriesEmptyData(t *testing.T) { + // No lines means nothing to splice; the writer must not be touched, and + // in particular the system prompt must not be written without a header. + w := &failingWriter{failOn: 1} + if err := spliceShareEntries(w, nil, []byte("sys")); err != nil { + t.Fatalf("spliceShareEntries on empty data: %v", err) + } + if w.n != 0 { + t.Errorf("performed %d writes on empty data, want 0", w.n) + } +} + +// TestFinishShareFileRemovesFileOnWriteFailure is the regression test for the +// temp-file leak: the cleanup path must remove the file that was actually +// created. A read-only handle makes every write fail with EBADF. +func TestFinishShareFileRemovesFileOnWriteFailure(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "share.jsonl") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatalf("seed file: %v", err) + } + // Opened read-only, so writes fail. + f, err := os.Open(path) + if err != nil { + t.Fatalf("Open: %v", err) + } + + got, err := finishShareFile(f, []byte("header\nentry\n"), []byte("sys")) + if err == nil { + t.Fatal("finishShareFile on a read-only file = nil, want an error") + } + if got != "" { + t.Errorf("path = %q, want empty on failure", got) + } + if _, statErr := os.Stat(path); !os.IsNotExist(statErr) { + t.Errorf("temp file leaked at %s (stat err = %v)", path, statErr) + } +} + +func TestFinishShareFileReturnsPathOnSuccess(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "share.jsonl") + f, err := os.Create(path) + if err != nil { + t.Fatalf("Create: %v", err) + } + + got, err := finishShareFile(f, []byte("header\nentry\n"), []byte("sys")) + if err != nil { + t.Fatalf("finishShareFile: %v", err) + } + if got != path { + t.Errorf("path = %q, want %q", got, path) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(data) != "header\nsys\nentry\n" { + t.Errorf("content = %q, want the system prompt spliced after the header", data) + } + // The file must be closed; writing through the stale handle should fail. + if _, err := f.WriteString("x"); err == nil { + t.Error("file still open after finishShareFile") + } +} + +// TestWriteShareableSessionLeavesNoTempFilesOnSuccess guards the temp +// directory against accumulating share files across successful runs. +func TestWriteShareableSessionCleansUpAfterCaller(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("TMPDIR", tmpDir) + + a, tm := newPersistedApp(t) + appendUserMessage(t, tm, "hello") + + path, err := a.WriteShareableSession("be helpful", "model") + if err != nil { + t.Fatalf("WriteShareableSession: %v", err) + } + if filepath.Dir(path) != tmpDir { + t.Fatalf("share file at %q, want it under the sandboxed temp dir %q", path, tmpDir) + } + + entries, err := os.ReadDir(tmpDir) + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + if len(entries) != 1 { + t.Errorf("temp dir holds %d files, want exactly the one share file", len(entries)) + } +} diff --git a/internal/ui/session_selector.go b/internal/ui/session_selector.go index dc637908..5abff25c 100644 --- a/internal/ui/session_selector.go +++ b/internal/ui/session_selector.go @@ -9,6 +9,7 @@ import ( "charm.land/bubbles/v2/key" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" + "github.com/charmbracelet/log" "github.com/mark3labs/kit/internal/app" "github.com/mark3labs/kit/internal/ui/style" @@ -114,12 +115,23 @@ func NewSessionSelector(store SessionStore, cwd string, width, height int) *Sess confirmDelete: -1, } - // Load sessions (errors are swallowed — empty list is fine). + // Listing failures degrade to an empty list rather than blocking the + // picker, but they are logged: a permissions or disk error would otherwise + // be indistinguishable from "no sessions yet". Log output is redirected to + // a file while the TUI runs, so this cannot corrupt the alt-screen. if cwd != "" { - ss.cwdSessions, _ = store.ListSessions(cwd) + sessions, err := store.ListSessions(cwd) + if err != nil { + log.Warn("session picker: listing sessions for cwd failed", "cwd", cwd, "err", err) + } + ss.cwdSessions = sessions ss.scope = SessionScopeCwd } - ss.allSessions, _ = store.ListAllSessions() + all, err := store.ListAllSessions() + if err != nil { + log.Warn("session picker: listing all sessions failed", "err", err) + } + ss.allSessions = all if cwd == "" || len(ss.cwdSessions) == 0 { ss.scope = SessionScopeAll From 9adf02a798031c88b315b2096ede6e2d3047029b Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Thu, 30 Jul 2026 17:52:09 +0300 Subject: [PATCH 3/3] test(app): correct stale doc comment on share cleanup test (#101) The comment named a function that had been renamed. Renamed the test to match what it asserts instead: a successful share leaves exactly the returned file behind, since removing it is the caller's job. --- internal/app/session_store_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/app/session_store_test.go b/internal/app/session_store_test.go index 80cfadf8..7f734cbd 100644 --- a/internal/app/session_store_test.go +++ b/internal/app/session_store_test.go @@ -562,9 +562,11 @@ func TestFinishShareFileReturnsPathOnSuccess(t *testing.T) { } } -// TestWriteShareableSessionLeavesNoTempFilesOnSuccess guards the temp -// directory against accumulating share files across successful runs. -func TestWriteShareableSessionCleansUpAfterCaller(t *testing.T) { +// TestWriteShareableSessionLeavesOnlyTheShareFile checks that a successful +// share leaves exactly the file it returns in the temp directory, with no +// half-written strays alongside it. Removing the returned file is the +// caller's job, so it is expected to still be there. +func TestWriteShareableSessionLeavesOnlyTheShareFile(t *testing.T) { tmpDir := t.TempDir() t.Setenv("TMPDIR", tmpDir)