Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .agents/DECISIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
3 changes: 1 addition & 2 deletions .agents/TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion frontend/admin-ui/src/views/memory/MemoryCard.vue
Original file line number Diff line number Diff line change
@@ -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
Expand Down
27 changes: 0 additions & 27 deletions server/clients/discord/bot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
27 changes: 0 additions & 27 deletions server/clients/slack/bot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
34 changes: 0 additions & 34 deletions server/clients/telegram/bot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
27 changes: 11 additions & 16 deletions server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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),
}
}

Expand Down Expand Up @@ -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)}
Expand Down Expand Up @@ -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)}
Expand Down Expand Up @@ -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)}
Expand Down