diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 1f463fbc9..60ba49204 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -462,9 +462,66 @@ func (al *AgentLoop) startNewSessionForMessage(msg bus.InboundMessage) (string, overrideKey := buildSessionOverrideKey(msg.Channel, msg.ChatID, agent.ID) al.sessionOverride.Set(overrideKey, newSessionKey) + // Clean up the old session file from disk to prevent unbounded accumulation. + if sessionKey != "" { + if err := agent.Sessions.Delete(sessionKey); err != nil { + logger.WarnCF("agent", "Failed to delete old session file", map[string]any{ + "session_key": sessionKey, + "error": err.Error(), + }) + } + } + return formatNewSessionResponse(al.cfg, agent), nil } +// clearSessionForMessage clears the current session history and summary in-place, +// keeping the same session key. Unlike startNewSessionForMessage, this does not +// create a new session key or set an override. +func (al *AgentLoop) clearSessionForMessage(msg bus.InboundMessage) (string, error) { + if constants.IsInternalChannel(msg.Channel) { + return "", fmt.Errorf("clear command is not supported in internal channels") + } + + route := al.registry.ResolveRoute(routing.RouteInput{ + Channel: msg.Channel, + AccountID: msg.Metadata["account_id"], + Peer: extractPeer(msg), + ParentPeer: extractParentPeer(msg), + GuildID: msg.Metadata["guild_id"], + TeamID: msg.Metadata["team_id"], + }) + + agent, ok := al.registry.GetAgent(route.AgentID) + if !ok { + agent = al.registry.GetDefaultAgent() + } + if agent == nil { + return "", fmt.Errorf("no agent available to clear session") + } + + // Resolve the active session key, honoring any existing override. + sessionKey := route.SessionKey + if msg.SessionKey != "" && strings.HasPrefix(msg.SessionKey, "agent:") { + sessionKey = msg.SessionKey + } + if override, ok := al.sessionOverride.Get(buildSessionOverrideKey(msg.Channel, msg.ChatID, agent.ID)); ok { + sessionKey = override + } + + if sessionKey == "" { + return "", fmt.Errorf("no active session to clear") + } + + agent.Sessions.ClearSession(sessionKey) + + if err := agent.Sessions.Save(sessionKey); err != nil { + return "", err + } + + return "Conversation history cleared.", nil +} + func formatNewSessionResponse(cfg *config.Config, agent *AgentInstance) string { provider, model := resolveAgentModelDisplay(cfg, agent) if model == "" { @@ -1156,13 +1213,25 @@ func (al *AgentLoop) handleCommand(ctx context.Context, msg bus.InboundMessage) args := parts[1:] switch cmd { - case "/new", "/clear": + case "/new": if len(args) > 0 { - return fmt.Sprintf("Usage: %s", cmd), true + return "Usage: /new", true } response, err := al.startNewSessionForMessage(msg) if err != nil { - return fmt.Sprintf("Error: %v", err), true + logger.ErrorCF("agent", "Failed to start new session", map[string]any{"error": err.Error()}) + return "Sorry, I couldn't start a new session. Please try again.", true + } + return response, true + + case "/clear": + if len(args) > 0 { + return "Usage: /clear", true + } + response, err := al.clearSessionForMessage(msg) + if err != nil { + logger.ErrorCF("agent", "Failed to clear session", map[string]any{"error": err.Error()}) + return "Sorry, I couldn't clear the conversation. Please try again.", true } return response, true diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 0c116edcf..16fdf06f1 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -429,9 +429,11 @@ func TestNewSessionCommand_SuccessAndIsolation(t *testing.T) { if len(newHistory) != 2 { t.Fatalf("Expected new session history to have 2 messages, got %d", len(newHistory)) } + + // After /new, the old session is deleted to prevent unbounded disk accumulation. oldHistoryAfter := agent.Sessions.GetHistory(oldKey) - if len(oldHistoryAfter) != len(oldHistory) { - t.Fatalf("Expected old session history to remain unchanged") + if len(oldHistoryAfter) != 0 { + t.Fatalf("Expected old session to be deleted after /new, got %d messages", len(oldHistoryAfter)) } } @@ -501,7 +503,7 @@ func TestNewSessionCommand_RejectsArgs(t *testing.T) { } } -func TestNewSessionCommand_PreservesOldSessionOnDisk(t *testing.T) { +func TestNewSessionCommand_DeletesOldSessionFromDisk(t *testing.T) { provider := &simpleMockProvider{response: "OK"} al, tmpDir := newTestAgentLoop(t, provider) defer os.RemoveAll(tmpDir) @@ -524,7 +526,6 @@ func TestNewSessionCommand_PreservesOldSessionOnDisk(t *testing.T) { filename := strings.ReplaceAll(oldKey, ":", "_") + ".json" sessionsDir := filepath.Join(agent.Workspace, "sessions") filePath := filepath.Join(sessionsDir, filename) - _ = os.Remove(filePath) _ = helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ Channel: "test", @@ -533,12 +534,134 @@ func TestNewSessionCommand_PreservesOldSessionOnDisk(t *testing.T) { Content: "/new", }) - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("Expected session file to exist after /new: %v", err) + // After /new, the old session file should be deleted from disk to prevent + // unbounded accumulation of session files. + if _, err := os.Stat(filePath); !os.IsNotExist(err) { + t.Fatalf("Expected old session file to be deleted after /new, but it still exists") } - if !strings.Contains(string(data), "hello") { - t.Fatalf("Expected session file to contain prior message") +} + +func TestClearCommand_ClearsHistoryInPlace(t *testing.T) { + provider := &simpleMockProvider{response: "OK"} + al, tmpDir := newTestAgentLoop(t, provider) + defer os.RemoveAll(tmpDir) + + agent := al.registry.GetDefaultAgent() + if agent == nil { + t.Fatal("No default agent found") + } + + helper := testHelper{al: al} + ctx := context.Background() + + // Send a message to populate history + _ = helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "hello", + }) + + oldKey := strings.ToLower(routing.BuildAgentMainSessionKey(agent.ID)) + oldHistory := agent.Sessions.GetHistory(oldKey) + if len(oldHistory) == 0 { + t.Fatalf("Expected session history to be populated before /clear") + } + + // Set a summary to verify it gets cleared too + agent.Sessions.SetSummary(oldKey, "Test summary") + + // Use /clear + response := helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "/clear", + }) + if response != "Conversation history cleared." { + t.Fatalf("Expected clear confirmation, got %q", response) + } + + // Verify: same session key, no override set + overrideKey := buildSessionOverrideKey("test", "chat1", agent.ID) + if _, ok := al.sessionOverride.Get(overrideKey); ok { + t.Fatalf("Expected /clear NOT to set a session override") + } + + // Verify: history is empty + clearedHistory := agent.Sessions.GetHistory(oldKey) + if len(clearedHistory) != 0 { + t.Fatalf("Expected session history to be cleared, got %d messages", len(clearedHistory)) + } + + // Verify: summary is empty + if agent.Sessions.GetSummary(oldKey) != "" { + t.Fatalf("Expected session summary to be cleared") + } +} + +func TestClearCommand_RejectsArgs(t *testing.T) { + provider := &simpleMockProvider{response: "OK"} + al, tmpDir := newTestAgentLoop(t, provider) + defer os.RemoveAll(tmpDir) + + helper := testHelper{al: al} + ctx := context.Background() + response := helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "/clear extra", + }) + if response != "Usage: /clear" { + t.Fatalf("Expected usage response, got %q", response) + } +} + +func TestClearCommand_SubsequentMessagesUseSameSession(t *testing.T) { + provider := &simpleMockProvider{response: "OK"} + al, tmpDir := newTestAgentLoop(t, provider) + defer os.RemoveAll(tmpDir) + + agent := al.registry.GetDefaultAgent() + if agent == nil { + t.Fatal("No default agent found") + } + + helper := testHelper{al: al} + ctx := context.Background() + + // Send a message, then clear, then send another + _ = helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "first message", + }) + + sessionKey := strings.ToLower(routing.BuildAgentMainSessionKey(agent.ID)) + + _ = helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "/clear", + }) + + _ = helper.executeAndGetResponse(t, ctx, bus.InboundMessage{ + Channel: "test", + SenderID: "user1", + ChatID: "chat1", + Content: "second message", + }) + + // History should only contain the post-clear message exchange + history := agent.Sessions.GetHistory(sessionKey) + if len(history) != 2 { + t.Fatalf("Expected 2 messages (post-clear user+assistant), got %d", len(history)) + } + if history[0].Content != "second message" { + t.Fatalf("Expected first post-clear message to be 'second message', got %q", history[0].Content) } } diff --git a/pkg/session/manager.go b/pkg/session/manager.go index 08f0b0ad2..5c5ac60de 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -265,6 +265,43 @@ func (sm *SessionManager) loadSessions() error { return nil } +// Delete removes a session from memory and deletes its file from disk. +func (sm *SessionManager) Delete(key string) error { + sm.mu.Lock() + delete(sm.sessions, key) + sm.mu.Unlock() + + if sm.storage == "" { + return nil + } + + filename := sanitizeFilename(key) + if filename == "." || !filepath.IsLocal(filename) || strings.ContainsAny(filename, `/\`) { + return os.ErrInvalid + } + + sessionPath := filepath.Join(sm.storage, filename+".json") + if err := os.Remove(sessionPath); err != nil && !os.IsNotExist(err) { + return err + } + return nil +} + +// ClearSession resets a session's messages and summary in-place, keeping the same key. +func (sm *SessionManager) ClearSession(key string) { + sm.mu.Lock() + defer sm.mu.Unlock() + + session, ok := sm.sessions[key] + if !ok { + return + } + + session.Messages = []providers.Message{} + session.Summary = "" + session.Updated = time.Now() +} + // SetHistory updates the messages of a session. func (sm *SessionManager) SetHistory(key string, history []providers.Message) { sm.mu.Lock() diff --git a/pkg/session/manager_test.go b/pkg/session/manager_test.go index 5ef5f4349..bde6c9866 100644 --- a/pkg/session/manager_test.go +++ b/pkg/session/manager_test.go @@ -72,3 +72,87 @@ func TestSave_RejectsPathTraversal(t *testing.T) { } } } + +func TestDelete_RemovesSessionFromMemoryAndDisk(t *testing.T) { + tmpDir := t.TempDir() + sm := NewSessionManager(tmpDir) + + key := "agent:main:test" + sm.GetOrCreate(key) + sm.AddMessage(key, "user", "hello") + + if err := sm.Save(key); err != nil { + t.Fatalf("Save failed: %v", err) + } + + expectedFile := filepath.Join(tmpDir, "agent_main_test.json") + if _, err := os.Stat(expectedFile); os.IsNotExist(err) { + t.Fatalf("expected session file to exist before delete") + } + + if err := sm.Delete(key); err != nil { + t.Fatalf("Delete failed: %v", err) + } + + // Verify removed from memory + history := sm.GetHistory(key) + if len(history) != 0 { + t.Errorf("expected empty history after delete, got %d messages", len(history)) + } + + // Verify removed from disk + if _, err := os.Stat(expectedFile); !os.IsNotExist(err) { + t.Errorf("expected session file to be deleted from disk") + } +} + +func TestDelete_NonexistentKeyNoError(t *testing.T) { + tmpDir := t.TempDir() + sm := NewSessionManager(tmpDir) + + // Deleting a key that doesn't exist should not error + if err := sm.Delete("nonexistent:key"); err != nil { + t.Errorf("Delete of nonexistent key should not error, got: %v", err) + } +} + +func TestClearSession_ResetsHistoryAndSummary(t *testing.T) { + tmpDir := t.TempDir() + sm := NewSessionManager(tmpDir) + + key := "agent:main:clear-test" + sm.GetOrCreate(key) + sm.AddMessage(key, "user", "hello") + sm.AddMessage(key, "assistant", "hi there") + sm.SetSummary(key, "User said hello") + + // Verify pre-conditions + history := sm.GetHistory(key) + if len(history) != 2 { + t.Fatalf("expected 2 messages before clear, got %d", len(history)) + } + if sm.GetSummary(key) == "" { + t.Fatalf("expected non-empty summary before clear") + } + + sm.ClearSession(key) + + // Verify history is cleared + history = sm.GetHistory(key) + if len(history) != 0 { + t.Errorf("expected 0 messages after clear, got %d", len(history)) + } + + // Verify summary is cleared + if sm.GetSummary(key) != "" { + t.Errorf("expected empty summary after clear, got %q", sm.GetSummary(key)) + } +} + +func TestClearSession_NonexistentKeyNoPanic(t *testing.T) { + tmpDir := t.TempDir() + sm := NewSessionManager(tmpDir) + + // Clearing a nonexistent session should not panic + sm.ClearSession("nonexistent:key") +}