From 47a5b4c40f7a7c4a0d3b6c1e86624ac20a3e9af6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alby=20Hern=C3=A1ndez?= <donfumero@gmail.com>
Date: Sat, 18 Apr 2026 21:33:38 +0100
Subject: [PATCH] revert: remove duplicated !reset conversation-close from bots

The previous commit (3528d6b) duplicated logic that had already been
implemented on 2026-04-10 by commit 75cc2de. The ConversationRecorder
middleware intercepts every session DELETE and calls
Executor.CloseConversationSession, which already handles both admin and
user perspectives for every client and for the admin API alike.

Reverted:
- SetConversationStore + closeConversations in telegram/slack/discord bots
- conversations field + wiring in clientManager (main.go)

Remaining wins from the previous commit stay in place:
- Telegram thread-aware error messages (MessageThreadID on error paths)
- Slack/Discord multimodal inlineData support
- MemoryCard border polish (final form: no active override)

Added decision #22 to .agents/DECISIONS.md pinning the middleware-based
design and listing the "do not" rules so this mistake is not repeated.
---
 .agents/DECISIONS.md                          | 21 ++++++++++++
 .agents/TODO.md                               |  3 +-
 .../admin-ui/src/views/memory/MemoryCard.vue  |  2 +-
 server/clients/discord/bot.go                 | 27 ---------------
 server/clients/slack/bot.go                   | 27 ---------------
 server/clients/telegram/bot.go                | 34 -------------------
 server/main.go                                | 27 ++++++---------
 7 files changed, 34 insertions(+), 107 deletions(-)

diff --git a/.agents/DECISIONS.md b/.agents/DECISIONS.md
index 38413ed..491e458 100644
--- a/.agents/DECISIONS.md
+++ b/.agents/DECISIONS.md
@@ -514,3 +514,24 @@ TTS and STT proxies were hardcoded to OpenAI-compatible endpoints (`/v1/audio/sp
 **Do not**: Hardcode endpoint paths in the proxy handlers. Do not add provider-specific logic to `main.go` — all translation lives in the provider package. Do not put provider-specific fields as top-level fields on `TTSRef` or `BackendRef` — use the typed config structs.
 
 **Files**: `server/voice/provider.go`, `server/voice/registry.go`, `server/voice/openai/openai.go`, `server/voice/gemini/gemini.go`, `server/api/admin/voice.go`, `server/main.go` (proxy refactor + blank imports), `server/store/types.go` (typed config structs), `server/store/store.go` (migration), `frontend/admin-ui/src/views/agents/AgentDialog.vue`.
+
+---
+
+## 22. Conversation close on reset is a middleware concern, not a client one
+
+**Date**: 2026-04-18
+**Status**: Implemented (since 75cc2de, 2026-04-10)
+
+When a chat client (Telegram/Slack/Discord) or the admin UI triggers a session reset, the corresponding conversation record must be marked `Closed` so the next message starts a fresh conversation instead of appending to the old one.
+
+This responsibility lives in **one place only**: the `ConversationRecorder` middleware in `server/middleware/recorder.go`. Every `DELETE /api/v1/agent/apps/{app}/users/{user}/sessions/{session}` is intercepted regardless of origin; after the delete succeeds the middleware calls `executor.CloseConversationSession(sessionID, agentID)`, which internally closes both the `admin` and `user` perspectives via `ConversationStore.CloseBySession`.
+
+**Do not**:
+
+- Re-implement `CloseBySession` inside individual client bots (Telegram `handleResetCommand`, Slack/Discord reset command handlers). The middleware already handles it transparently for every session delete, including those triggered by the admin `/conversations/{id}/reset-session` endpoint.
+- Add `SetConversationStore` methods to `Client` structs. The conversation store stays behind the executor/middleware boundary — clients have no business touching it.
+- Duplicate the `admin`+`user` perspective loop. It belongs in `Executor.CloseConversationSession`, a single function.
+
+**Files**: `server/middleware/recorder.go` (interception), `server/clients/executor.go` (`CloseConversationSession`), `server/store/conversations.go` (`Closed` flag + `CloseBySession` + `FindBySession` skipping closed records).
+
+**History**: Fix was introduced by commit `75cc2de` on 2026-04-10. A later audit attempted to re-add the close-on-reset logic inside each client bot; this was reverted in favour of the original middleware-based design.
diff --git a/.agents/TODO.md b/.agents/TODO.md
index fd72858..1bbd43a 100644
--- a/.agents/TODO.md
+++ b/.agents/TODO.md
@@ -4,11 +4,10 @@
 
 ### This branch (`feature/todo-audit-cleanup`, 2026-04-18)
 
-- **Conversation split on `!reset`** — Telegram/Slack/Discord clients now call `CloseBySession` on both admin+user perspectives after a successful reset so the next message creates a fresh conversation record. Wiring via new `SetConversationStore` on each client; store injected through `clientManager` in `server/main.go`.
 - **Telegram: thread-aware error messages** — the 4 remaining error-path `SendMessage` calls in `telegram/bot.go` now pass `MessageThreadID`, keeping error replies in the origin topic.
 - **Slack multimodal (inlineData)** — new `extractInlineDataFromFiles` in `slack/bot.go` processes non-audio files (`image/*`, `application/pdf`, `text/*`, etc.) from DMs, encodes them as base64 `inlineData` parts. `callAgentSSE` and `processMessage` now accept a `[]map[string]interface{}` parts slice. 5MB/file limit.
 - **Discord multimodal (inlineData)** — new `extractInlineDataFromAttachments` in `discord/bot.go` processes non-audio message attachments the same way. Voice messages still handled separately by `handleVoice`.
-- **MemoryCard border: grey at rest, green on hover** — removed the `!border-green-500/30` override when a provider is marked active; active state is now signalled by a subtle ring only, matching the "quiet by default, color on hover" design rule from `ADMIN_UI_DESIGN_SYSTEM.md`.
+- **MemoryCard border** — removed the active-state override so the card follows the normal Card hover behaviour (grey at rest, green tint on hover). Active state is indicated solely by the radio button at the top-left.
 
 ### Earlier audit (2026-04-18)
 
diff --git a/frontend/admin-ui/src/views/memory/MemoryCard.vue b/frontend/admin-ui/src/views/memory/MemoryCard.vue
index 778773c..27a306f 100644
--- a/frontend/admin-ui/src/views/memory/MemoryCard.vue
+++ b/frontend/admin-ui/src/views/memory/MemoryCard.vue
@@ -1,5 +1,5 @@
 <template>
-  <Card color="green" :class="active ? 'ring-1 ring-green-500/20' : ''">
+  <Card color="green">
     <div class="flex items-start gap-3">
       <!-- Radio toggle -->
       <button
diff --git a/server/clients/discord/bot.go b/server/clients/discord/bot.go
index 571a005..4bf4cb5 100644
--- a/server/clients/discord/bot.go
+++ b/server/clients/discord/bot.go
@@ -53,32 +53,6 @@ type Client struct {
 
 	showToolsMu sync.RWMutex
 	showTools   bool
-
-	// conversations is optional: when set, reset commands close the active
-	// conversation so subsequent messages produce a fresh record.
-	conversations *store.ConversationStore
-}
-
-// SetConversationStore enables conversation closing on !reset.
-func (c *Client) SetConversationStore(cs *store.ConversationStore) {
-	c.conversations = cs
-}
-
-// closeConversations closes admin+user perspectives for the session.
-func (c *Client) closeConversations(sessionID, agentID string) {
-	if c.conversations == nil {
-		return
-	}
-	for _, perspective := range []string{"admin", "user"} {
-		if err := c.conversations.CloseBySession(sessionID, agentID, perspective); err != nil {
-			c.logger.Debug("No active conversation to close after reset",
-				"session", sessionID,
-				"agent", agentID,
-				"perspective", perspective,
-				"error", err,
-			)
-		}
-	}
 }
 
 func New(clientDef store.ClientDefinition, agentURL string, agents []AgentInfo, s interface {
@@ -588,7 +562,6 @@ func (c *Client) handleBotCommand(s *discordgo.Session, m *discordgo.MessageCrea
 			s.ChannelMessageSend(m.ChannelID, "Failed to reset session.")
 			return true
 		}
-		c.closeConversations(sessionID, agentID)
 		c.logger.Info("Session reset", "channel", m.ChannelID, "agent", agentID, "session", sessionID)
 		agent := c.getAgentInfo(agentID)
 		label := agentID
diff --git a/server/clients/slack/bot.go b/server/clients/slack/bot.go
index fe3ae9e..1618f34 100644
--- a/server/clients/slack/bot.go
+++ b/server/clients/slack/bot.go
@@ -62,32 +62,6 @@ type Client struct {
 	seen   map[string]struct{}
 
 	botUserID string
-
-	// conversations is optional: when set, reset commands close the active
-	// conversation so subsequent messages produce a fresh record.
-	conversations *store.ConversationStore
-}
-
-// SetConversationStore enables conversation closing on !reset.
-func (c *Client) SetConversationStore(cs *store.ConversationStore) {
-	c.conversations = cs
-}
-
-// closeConversations closes admin+user perspectives for the session.
-func (c *Client) closeConversations(sessionID, agentID string) {
-	if c.conversations == nil {
-		return
-	}
-	for _, perspective := range []string{"admin", "user"} {
-		if err := c.conversations.CloseBySession(sessionID, agentID, perspective); err != nil {
-			c.logger.Debug("No active conversation to close after reset",
-				"session", sessionID,
-				"agent", agentID,
-				"perspective", perspective,
-				"error", err,
-			)
-		}
-	}
 }
 
 func New(clientDef store.ClientDefinition, agentURL string, agents []AgentInfo, s interface {
@@ -458,7 +432,6 @@ func (c *Client) handleBotCommand(userID, channelID, text, threadTS string) bool
 			c.postMessage(channelID, "Failed to reset session.", threadTS)
 			return true
 		}
-		c.closeConversations(sessionID, agentID)
 		c.logger.Info("Session reset", "channel", channelID, "agent", agentID, "session", sessionID)
 		agent := c.getAgentInfo(agentID)
 		label := agentID
diff --git a/server/clients/telegram/bot.go b/server/clients/telegram/bot.go
index 113aafd..d011695 100644
--- a/server/clients/telegram/bot.go
+++ b/server/clients/telegram/bot.go
@@ -84,38 +84,6 @@ type Client struct {
 	showTools   bool
 
 	botUsername string
-
-	// conversations is optional: when set, reset commands close the active
-	// conversation so subsequent messages produce a fresh record instead of
-	// appending to the old one.
-	conversations *store.ConversationStore
-}
-
-// SetConversationStore enables conversation closing on !reset. Safe to call
-// before Start(); clients created without a conversation store continue to
-// work, they just won't split records after resets.
-func (c *Client) SetConversationStore(cs *store.ConversationStore) {
-	c.conversations = cs
-}
-
-// closeConversations closes the admin+user perspectives of the active
-// conversation for the given session, if a conversation store is configured.
-// Missing conversations are silently ignored — they may simply not exist yet
-// (first reset before any message was logged).
-func (c *Client) closeConversations(sessionID, agentID string) {
-	if c.conversations == nil {
-		return
-	}
-	for _, perspective := range []string{"admin", "user"} {
-		if err := c.conversations.CloseBySession(sessionID, agentID, perspective); err != nil {
-			c.logger.Debug("No active conversation to close after reset",
-				"session", sessionID,
-				"agent", agentID,
-				"perspective", perspective,
-				"error", err,
-			)
-		}
-	}
 }
 
 // New creates a Telegram client ready to be started. It validates the bot token
@@ -406,8 +374,6 @@ func (c *Client) handleResetCommand(ctx *th.Context, msg telego.Message) error {
 		"session", sessionID,
 	)
 
-	c.closeConversations(sessionID, agentID)
-
 	_, _ = ctx.Bot().SendMessage(ctx, &telego.SendMessageParams{
 		ChatID:          tu.ID(msg.Chat.ID),
 		MessageThreadID: msg.MessageThreadID,
diff --git a/server/main.go b/server/main.go
index 701970b..c1fcdb3 100644
--- a/server/main.go
+++ b/server/main.go
@@ -112,7 +112,7 @@ func main() {
 	watchStoreChanges(ctx, dataStore, agentRouter)
 
 	// Start schedulers and clients
-	cronScheduler, clientManager := startClients(ctx, cfg, dataStore, convoStore, executor)
+	cronScheduler, clientManager := startClients(ctx, cfg, dataStore, executor)
 
 	// Graceful shutdown
 	startGracefulShutdown(adminServer, adminCtx, adminCancel, userServer, userCtx, userCancel, cronScheduler, clientManager, voiceDetector)
@@ -344,11 +344,11 @@ func watchStoreChanges(ctx context.Context, dataStore *store.Store, agentRouter
 
 // startClients initializes and starts external messaging clients (Discord, Slack, Telegram, etc.)
 // and the cron job scheduler for automated tasks.
-func startClients(ctx context.Context, cfg *config.Config, dataStore *store.Store, convoStore *store.ConversationStore, executor *clients.Executor) (*cron.Scheduler, *clientManager) {
+func startClients(ctx context.Context, cfg *config.Config, dataStore *store.Store, executor *clients.Executor) (*cron.Scheduler, *clientManager) {
 	cronScheduler := cron.NewScheduler(executor, dataStore, slog.Default())
 	go cronScheduler.Start(ctx)
 
-	cm := newClientManager(dataStore, convoStore, cfg.Server.Port, slog.Default())
+	cm := newClientManager(dataStore, cfg.Server.Port, slog.Default())
 	cm.start(ctx)
 
 	return cronScheduler, cm
@@ -621,10 +621,9 @@ func checkDependencies(cfg *config.Config) {
 // It subscribes to store changes and reconciles running clients: stopping
 // removed/disabled ones and starting new/re-enabled ones automatically.
 type clientManager struct {
-	store         *store.Store
-	conversations *store.ConversationStore
-	agentURL      string
-	logger        *slog.Logger
+	store    *store.Store
+	agentURL string
+	logger   *slog.Logger
 
 	mu      sync.Mutex
 	running map[string]*managedClient
@@ -636,13 +635,12 @@ type managedClient struct {
 	hash   string
 }
 
-func newClientManager(s *store.Store, cs *store.ConversationStore, port int, logger *slog.Logger) *clientManager {
+func newClientManager(s *store.Store, port int, logger *slog.Logger) *clientManager {
 	return &clientManager{
-		store:         s,
-		conversations: cs,
-		agentURL:      fmt.Sprintf("http://127.0.0.1:%d/api/v1/agent", port),
-		logger:        logger,
-		running:       make(map[string]*managedClient),
+		store:    s,
+		agentURL: fmt.Sprintf("http://127.0.0.1:%d/api/v1/agent", port),
+		logger:   logger,
+		running:  make(map[string]*managedClient),
 	}
 }
 
@@ -750,7 +748,6 @@ func (m *clientManager) startTelegram(ctx context.Context, cl store.ClientDefini
 		m.logger.Error("Failed to create Telegram client", "client", cl.Name, "error", err)
 		return
 	}
-	tgClient.SetConversationStore(m.conversations)
 
 	clientCtx, cancel := context.WithCancel(ctx)
 	m.running[cl.ID] = &managedClient{stop: tgClient.Stop, cancel: cancel, hash: clientHash(cl)}
@@ -796,7 +793,6 @@ func (m *clientManager) startSlack(ctx context.Context, cl store.ClientDefinitio
 		m.logger.Error("Failed to create Slack client", "client", cl.Name, "error", err)
 		return
 	}
-	skClient.SetConversationStore(m.conversations)
 
 	clientCtx, cancel := context.WithCancel(ctx)
 	m.running[cl.ID] = &managedClient{stop: skClient.Stop, cancel: cancel, hash: clientHash(cl)}
@@ -842,7 +838,6 @@ func (m *clientManager) startDiscord(ctx context.Context, cl store.ClientDefinit
 		m.logger.Error("Failed to create Discord client", "client", cl.Name, "error", err)
 		return
 	}
-	dcClient.SetConversationStore(m.conversations)
 
 	clientCtx, cancel := context.WithCancel(ctx)
 	m.running[cl.ID] = &managedClient{stop: dcClient.Stop, cancel: cancel, hash: clientHash(cl)}