From a8b2b525ec4c19c1a683457bbd353eebc6fad834 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Fri, 29 May 2026 14:04:37 +0200 Subject: [PATCH] fix(mcp): resolve active session from store for mem_save instead of manual-save fallback (#386) --- internal/mcp/mcp.go | 29 +++++++-- internal/mcp/mcp_test.go | 120 +++++++++++++++++++++++++++++++++++ internal/store/store.go | 44 +++++++++++++ internal/store/store_test.go | 108 +++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+), 4 deletions(-) diff --git a/internal/mcp/mcp.go b/internal/mcp/mcp.go index f1c0da13..e4958c29 100644 --- a/internal/mcp/mcp.go +++ b/internal/mcp/mcp.go @@ -1100,7 +1100,7 @@ func handleSave(s *store.Store, cfg MCPConfig, activity *SessionActivity) server typ = "manual" } if sessionID == "" { - sessionID = defaultSessionID(project) + sessionID = resolveFallbackSessionID(s, project) } suggestedTopicKey := suggestTopicKey(typ, title, content) @@ -1350,7 +1350,7 @@ func handleSavePrompt(s *store.Store, cfg MCPConfig, activity *SessionActivity) project, _ := store.NormalizeProject(detRes.Project) if sessionID == "" { - sessionID = defaultSessionID(project) + sessionID = resolveFallbackSessionID(s, project) } // Ensure the implicit MCP session exists with the current working directory. @@ -1647,7 +1647,7 @@ func handleSessionSummary(s *store.Store, cfg MCPConfig, activity *SessionActivi project, _ := store.NormalizeProject(detRes.Project) if sessionID == "" { - sessionID = defaultSessionID(project) + sessionID = resolveFallbackSessionID(s, project) } // Ensure the implicit MCP session exists with the current working directory. @@ -1767,7 +1767,7 @@ func handleCapturePassive(s *store.Store, cfg MCPConfig, activity *SessionActivi } if sessionID == "" { - sessionID = defaultSessionID(project) + sessionID = resolveFallbackSessionID(s, project) _ = ensureImplicitSessionWithCWD(s, sessionID, project) } @@ -2695,6 +2695,27 @@ func defaultSessionID(project string) string { return "manual-save-" + project } +// resolveFallbackSessionID resolves the session a write should attach to when +// the caller did not provide an explicit session_id. +// +// It first consults the persisted sessions table for the most recent active +// (un-ended) session of the project (issue #386). The SessionStart hook +// registers a UUID session via the HTTP server, a SEPARATE process from this +// MCP (stdio) server; the two share only the SQLite store, so the active +// session must be resolved from disk rather than from any in-process map. +// +// When no active session exists for the project (or the store query fails for +// any reason), it falls back to the manual-save-{project} session, preserving +// the prior behavior for projects with no live session. +func resolveFallbackSessionID(s *store.Store, project string) string { + if s != nil { + if id, ok, err := s.MostRecentActiveSession(project); err == nil && ok { + return id + } + } + return defaultSessionID(project) +} + func intArg(req mcp.CallToolRequest, key string, defaultVal int) int { v, ok := req.GetArguments()[key].(float64) if !ok { diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 4cc47d7a..c063f54c 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -324,6 +324,126 @@ func TestHandleSaveRecordsActivityForExplicitSessionID(t *testing.T) { } } +// TestHandleSaveResolvesActiveSessionFromStore reproduces issue #386: the +// SessionStart hook registers a UUID session via POST /sessions (a separate +// process from the MCP server, sharing only the SQLite store). A later +// mem_save with no explicit session_id must attach to that UUID session, +// resolved from the persisted sessions table — NOT fall back to +// manual-save-{project}. The two processes never share in-memory state, so +// store-based resolution is the only thing that survives the process split. +func TestHandleSaveResolvesActiveSessionFromStore(t *testing.T) { + s := newMCPTestStore(t) + + // Simulate the SessionStart hook registering a UUID session (POST /sessions + // ultimately calls store.CreateSession). + const uuidSession = "0c8e7f2a-1b34-4d9e-9a77-aaaabbbbcccc" + if err := s.CreateSession(uuidSession, "engram", "/work/engram"); err != nil { + t.Fatalf("create UUID session: %v", err) + } + + // mem_save with NO session_id — exactly what the proactive protocol does. + h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "title": "Active session resolution", + "content": "**What**: saved without session_id\n**Why**: repro for #386", + "type": "bugfix", + "project": "engram", + }}}) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if res.IsError { + t.Fatalf("unexpected save error: %s", callResultText(t, res)) + } + + obs, err := s.RecentObservations("engram", "project", 5) + if err != nil { + t.Fatalf("recent observations: %v", err) + } + if len(obs) == 0 { + t.Fatalf("expected at least one observation, got none") + } + if obs[0].SessionID != uuidSession { + t.Fatalf("expected observation to attach to active UUID session %q, got %q (regression #386: fell back to manual-save)", uuidSession, obs[0].SessionID) + } +} + +// TestHandleSaveFallsBackToManualSaveWhenNoActiveSession is the regression +// guard for the preserved behavior: when there is no un-ended session for the +// project, mem_save with no session_id must still use manual-save-{project}. +func TestHandleSaveFallsBackToManualSaveWhenNoActiveSession(t *testing.T) { + s := newMCPTestStore(t) + + h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "title": "No active session", + "content": "**What**: saved with no active session\n**Why**: fallback regression guard", + "type": "bugfix", + "project": "engram", + }}}) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if res.IsError { + t.Fatalf("unexpected save error: %s", callResultText(t, res)) + } + + obs, err := s.RecentObservations("engram", "project", 5) + if err != nil { + t.Fatalf("recent observations: %v", err) + } + if len(obs) == 0 { + t.Fatalf("expected at least one observation, got none") + } + if want := defaultSessionID("engram"); obs[0].SessionID != want { + t.Fatalf("expected fallback to %q with no active session, got %q", want, obs[0].SessionID) + } +} + +// TestHandleSaveResolvesMostRecentActiveSession covers the multi-session edge +// case: two un-ended sessions exist; mem_save must attach to the most recent. +func TestHandleSaveResolvesMostRecentActiveSession(t *testing.T) { + s := newMCPTestStore(t) + + if err := s.CreateSession("uuid-older", "engram", "/work/engram"); err != nil { + t.Fatalf("create older session: %v", err) + } + if _, err := s.DB().Exec(`UPDATE sessions SET started_at = ? WHERE id = ?`, "2025-01-01 00:00:00", "uuid-older"); err != nil { + t.Fatalf("backdate older session: %v", err) + } + if err := s.CreateSession("uuid-newer", "engram", "/work/engram"); err != nil { + t.Fatalf("create newer session: %v", err) + } + if _, err := s.DB().Exec(`UPDATE sessions SET started_at = ? WHERE id = ?`, "2025-06-01 00:00:00", "uuid-newer"); err != nil { + t.Fatalf("set newer session started_at: %v", err) + } + + h := handleSave(s, MCPConfig{}, NewSessionActivity(10*time.Minute)) + res, err := h(context.Background(), mcppkg.CallToolRequest{Params: mcppkg.CallToolParams{Arguments: map[string]any{ + "title": "Most recent active session", + "content": "**What**: saved with two active sessions\n**Why**: multi-session edge case", + "type": "bugfix", + "project": "engram", + }}}) + if err != nil { + t.Fatalf("handler error: %v", err) + } + if res.IsError { + t.Fatalf("unexpected save error: %s", callResultText(t, res)) + } + + obs, err := s.RecentObservations("engram", "project", 5) + if err != nil { + t.Fatalf("recent observations: %v", err) + } + if len(obs) == 0 { + t.Fatalf("expected at least one observation, got none") + } + if obs[0].SessionID != "uuid-newer" { + t.Fatalf("expected most recent active session uuid-newer, got %q", obs[0].SessionID) + } +} + func TestHandleSaveWithNilActivityStillSucceeds(t *testing.T) { s := newMCPTestStore(t) h := handleSave(s, MCPConfig{}, nil) diff --git a/internal/store/store.go b/internal/store/store.go index 0978de38..5d5d58d1 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -2015,6 +2015,50 @@ func (s *Store) GetSession(id string) (*Session, error) { return &sess, nil } +// MostRecentActiveSession resolves the active (un-ended) session for a project +// from the persisted sessions table. It returns the session ID and ok=true when +// such a session exists, or ok=false when none does. +// +// This is the cross-process resolution that fixes issue #386: the SessionStart +// hook registers a UUID session via the HTTP server (POST /sessions) in one +// process, while mem_save runs in the separate MCP (stdio) process. The two +// share only the SQLite store, so the active session must be read from disk — +// never from in-memory state. +// +// Selection rules: +// - Scope to the (normalized) project. +// - Require ended_at IS NULL — ended sessions are never returned, so stale +// sessions naturally fall out without any explicit clearing step. +// - Exclude the manual-save fallback sessions (id LIKE 'manual-save%'); those +// are created by the fallback path itself and must not be resolved as "the +// active session", which would make resolution circular. +// - When multiple un-ended sessions exist, pick the MOST RECENT by +// started_at DESC, with id DESC as a deterministic tie-breaker. +func (s *Store) MostRecentActiveSession(project string) (string, bool, error) { + project, _ = NormalizeProject(project) + if project == "" { + return "", false, nil + } + + var id string + err := s.db.QueryRow(` + SELECT id + FROM sessions + WHERE LOWER(project) = ? + AND ended_at IS NULL + AND id NOT LIKE 'manual-save%' + ORDER BY datetime(started_at) DESC, id DESC + LIMIT 1 + `, project).Scan(&id) + if errors.Is(err, sql.ErrNoRows) { + return "", false, nil + } + if err != nil { + return "", false, err + } + return id, true, nil +} + func (s *Store) RecentSessions(project string, limit int) ([]SessionSummary, error) { // Normalize project filter for case-insensitive matching project, _ = NormalizeProject(project) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index dac41370..01a85438 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -8039,3 +8039,111 @@ func TestDeleteProjectOrphansMemoryRelations(t *testing.T) { t.Errorf("expected relation judgment_status = orphaned after hard delete, got %q", judgmentStatus) } } + +func TestMostRecentActiveSessionReturnsUnEndedSession(t *testing.T) { + s := newTestStore(t) + + // A hook-registered UUID session, never ended. + if err := s.CreateSession("uuid-active-1", "engram", "/work/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + + id, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if !ok || id != "uuid-active-1" { + t.Fatalf("expected active session uuid-active-1, got id=%q ok=%v", id, ok) + } +} + +func TestMostRecentActiveSessionSkipsEndedSessions(t *testing.T) { + s := newTestStore(t) + + if err := s.CreateSession("uuid-ended-1", "engram", "/work/engram"); err != nil { + t.Fatalf("create session: %v", err) + } + if err := s.EndSession("uuid-ended-1", "done"); err != nil { + t.Fatalf("end session: %v", err) + } + + _, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if ok { + t.Fatalf("expected no active session when the only session is ended, got ok=%v", ok) + } +} + +func TestMostRecentActiveSessionNoSessionsReturnsFalse(t *testing.T) { + s := newTestStore(t) + + _, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if ok { + t.Fatalf("expected ok=false for a project with no sessions, got ok=%v", ok) + } +} + +func TestMostRecentActiveSessionPicksMostRecentWhenMultipleActive(t *testing.T) { + s := newTestStore(t) + + // Two un-ended UUID sessions for the same project; the newer started_at wins. + if err := s.CreateSession("uuid-old", "engram", "/work/engram"); err != nil { + t.Fatalf("create old session: %v", err) + } + if _, err := s.db.Exec(`UPDATE sessions SET started_at = ? WHERE id = ?`, "2025-01-01 00:00:00", "uuid-old"); err != nil { + t.Fatalf("backdate old session: %v", err) + } + if err := s.CreateSession("uuid-new", "engram", "/work/engram"); err != nil { + t.Fatalf("create new session: %v", err) + } + if _, err := s.db.Exec(`UPDATE sessions SET started_at = ? WHERE id = ?`, "2025-06-01 00:00:00", "uuid-new"); err != nil { + t.Fatalf("set new session started_at: %v", err) + } + + id, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if !ok || id != "uuid-new" { + t.Fatalf("expected most recent active session uuid-new, got id=%q ok=%v", id, ok) + } +} + +func TestMostRecentActiveSessionIgnoresManualSaveSessions(t *testing.T) { + s := newTestStore(t) + + // The manual-save fallback session is also un-ended, but it must NOT be + // resolved as "the active session" — otherwise resolution becomes circular. + if err := s.CreateSession("manual-save-engram", "engram", "/work/engram"); err != nil { + t.Fatalf("create manual-save session: %v", err) + } + + _, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if ok { + t.Fatalf("expected manual-save session to be ignored, got ok=%v", ok) + } +} + +func TestMostRecentActiveSessionScopedByProject(t *testing.T) { + s := newTestStore(t) + + if err := s.CreateSession("uuid-other-proj", "other", "/work/other"); err != nil { + t.Fatalf("create session: %v", err) + } + + _, ok, err := s.MostRecentActiveSession("engram") + if err != nil { + t.Fatalf("MostRecentActiveSession: %v", err) + } + if ok { + t.Fatalf("expected no active session for engram when only 'other' has one, got ok=%v", ok) + } +}