From 9fc72c1fb328cf791da5ff239b45c087ea276349 Mon Sep 17 00:00:00 2001 From: Mauro Date: Fri, 24 Apr 2026 05:49:41 +0200 Subject: [PATCH] feat(tool-feedback): add separate message mode for chat feedback (#2644) * feat(tool-feedback): add separate message mode for chat feedback * add parameter in conf --- config/config.example.json | 3 +- docs/operations/debug.md | 4 +- pkg/channels/manager.go | 71 ++++++- pkg/channels/manager_test.go | 184 ++++++++++++++++++ pkg/config/config.go | 12 +- pkg/config/config_test.go | 6 + pkg/config/defaults.go | 5 +- .../src/components/config/config-page.tsx | 1 + .../src/components/config/config-sections.tsx | 12 ++ .../src/components/config/form-model.ts | 6 + web/frontend/src/i18n/locales/en.json | 2 + web/frontend/src/i18n/locales/zh.json | 2 + 12 files changed, 292 insertions(+), 16 deletions(-) diff --git a/config/config.example.json b/config/config.example.json index 858472488..30460c231 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -13,7 +13,8 @@ "split_on_marker": false, "tool_feedback": { "enabled": false, - "max_args_length": 300 + "max_args_length": 300, + "separate_messages": false } } }, diff --git a/docs/operations/debug.md b/docs/operations/debug.md index b9e776f0f..eacd72380 100644 --- a/docs/operations/debug.md +++ b/docs/operations/debug.md @@ -65,7 +65,8 @@ Debug logs are server-side only. If you want the agent to send a visible notific "defaults": { "tool_feedback": { "enabled": true, - "max_args_length": 300 + "max_args_length": 300, + "separate_messages": true } } } @@ -85,6 +86,7 @@ When `enabled` is `true`, every tool call sends a short message to the chat befo | Field | Type | Default | Description | |---|---|---|---| | `enabled` | bool | `false` | Send a chat notification for each tool call | +| `separate_messages` | bool | `false` | Keep every tool feedback update as a separate chat message instead of reusing a single placeholder/progress message | | `max_args_length` | int | `300` | Maximum characters of the serialised arguments included in the notification | ### Environment variables diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index 2ffb1bb10..7974a39e4 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -170,6 +170,20 @@ func dismissTrackedToolFeedbackMessage( } } +func clearTrackedToolFeedbackMessage( + ch Channel, + chatID string, + outboundCtx *bus.InboundContext, +) { + trackedChatID := trackedToolFeedbackMessageChatID(ch, chatID, outboundCtx) + if trackedChatID == "" { + return + } + if tracker, ok := ch.(toolFeedbackMessageTracker); ok { + tracker.ClearToolFeedbackMessage(trackedChatID) + } +} + func prepareToolFeedbackMessageContent(ch Channel, content string) string { prepared := strings.TrimSpace(content) if prepared == "" { @@ -183,6 +197,13 @@ func prepareToolFeedbackMessageContent(ch Channel, content string) string { return prepared } +func (m *Manager) toolFeedbackSeparateMessagesEnabled() bool { + if m == nil || m.config == nil { + return false + } + return m.config.Agents.Defaults.IsToolFeedbackSeparateMessagesEnabled() +} + // RecordPlaceholder registers a placeholder message for later editing. // Implements PlaceholderRecorder. func (m *Manager) RecordPlaceholder(channel, chatID, placeholderID string) { @@ -264,6 +285,7 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } isToolFeedback := outboundMessageIsToolFeedback(msg) + separateToolFeedbackMessages := m.toolFeedbackSeparateMessagesEnabled() // 3. If a stream already finalized this chat, stale tool feedback must be // dropped without consuming the final-response marker. Streaming finalization @@ -288,14 +310,28 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } } if !isToolFeedback { - dismissTrackedToolFeedbackMessage(ctx, ch, chatID, &msg.Context) + if separateToolFeedbackMessages { + clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context) + } else { + dismissTrackedToolFeedbackMessage(ctx, ch, chatID, &msg.Context) + } } return nil, true } + if separateToolFeedbackMessages { + clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context) + } + // 5. Try editing placeholder if v, loaded := m.placeholders.LoadAndDelete(key); loaded { if entry, ok := v.(placeholderEntry); ok && entry.id != "" { + if isToolFeedback && separateToolFeedbackMessages { + if deleter, ok := ch.(MessageDeleter); ok { + deleter.DeleteMessage(ctx, chatID, entry.id) // best effort + } + return nil, false + } if editor, ok := ch.(MessageEditor); ok { content := msg.Content trackedContent := msg.Content @@ -345,6 +381,10 @@ func (m *Manager) preSendMedia(ctx context.Context, name string, msg bus.Outboun // 3. Clear any finalized stream marker for this chat before media delivery. m.streamActive.LoadAndDelete(key) + if m.toolFeedbackSeparateMessagesEnabled() { + clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context) + } + // 4. Delete placeholder if present. if v, loaded := m.placeholders.LoadAndDelete(key); loaded { if entry, ok := v.(placeholderEntry); ok && entry.id != "" { @@ -408,15 +448,26 @@ func (m *Manager) GetStreamer(ctx context.Context, channelName, chatID string) ( return &finalizeHookStreamer{ Streamer: streamer, onFinalize: func(finalizeCtx context.Context) { - dismissTrackedToolFeedbackMessage( - finalizeCtx, - ch, - chatID, - &bus.InboundContext{ - Channel: channelName, - ChatID: chatID, - }, - ) + if m.toolFeedbackSeparateMessagesEnabled() { + clearTrackedToolFeedbackMessage( + ch, + chatID, + &bus.InboundContext{ + Channel: channelName, + ChatID: chatID, + }, + ) + } else { + dismissTrackedToolFeedbackMessage( + finalizeCtx, + ch, + chatID, + &bus.InboundContext{ + Channel: channelName, + ChatID: chatID, + }, + ) + } m.streamActive.Store(key, true) }, }, true diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index 273c90468..a5d7c2838 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -804,6 +804,20 @@ type mockResolvedToolFeedbackEditor struct { resolveChatIDFn func(chatID string, outboundCtx *bus.InboundContext) string } +type mockDeletingMessageEditor struct { + mockMessageEditor + deleteCalls int + deletedChatID string + deletedMessageID string +} + +func (m *mockDeletingMessageEditor) DeleteMessage(_ context.Context, chatID, messageID string) error { + m.deleteCalls++ + m.deletedChatID = chatID + m.deletedMessageID = messageID + return nil +} + func (m *mockResolvedToolFeedbackEditor) ToolFeedbackMessageChatID( chatID string, outboundCtx *bus.InboundContext, @@ -1062,6 +1076,101 @@ func TestPreSend_NonToolFeedbackDefersTrackedMessageFinalizationToChannelSend(t } } +func TestPreSend_ToolFeedbackSeparateMessagesDeletesPlaceholderAndSkipsEdit(t *testing.T) { + m := newTestManager() + m.config = &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + ToolFeedback: config.ToolFeedbackConfig{ + Enabled: true, + SeparateMessages: true, + }, + }, + }, + } + + ch := &mockDeletingMessageEditor{ + mockMessageEditor: mockMessageEditor{ + editFn: func(_ context.Context, _, _, _ string) error { + t.Fatal("expected placeholder edit to be skipped in separate message mode") + return nil + }, + }, + } + + m.RecordPlaceholder("test", "123", "456") + + msg := testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Content: "hello", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + }, + }) + + msgIDs, handled := m.preSend(context.Background(), "test", msg, ch) + if handled { + t.Fatalf("expected preSend to fall through so the channel can send a new message, got %v", msgIDs) + } + if ch.deleteCalls != 1 { + t.Fatalf("expected placeholder deletion, got %d delete calls", ch.deleteCalls) + } + if ch.deletedChatID != "123" || ch.deletedMessageID != "456" { + t.Fatalf("unexpected placeholder deletion target: %s/%s", ch.deletedChatID, ch.deletedMessageID) + } + if ch.recordedMessageID != "" { + t.Fatalf("expected no tracked placeholder record, got %q", ch.recordedMessageID) + } + if ch.clearedChatID != "123" { + t.Fatalf("expected tracked tool feedback state to be cleared before sending, got %q", ch.clearedChatID) + } +} + +func TestPreSend_NonToolFeedbackSeparateMessagesClearsTrackedMessageWithoutDismiss(t *testing.T) { + m := newTestManager() + m.config = &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + ToolFeedback: config.ToolFeedbackConfig{ + Enabled: true, + SeparateMessages: true, + }, + }, + }, + } + + ch := &mockMessageEditor{} + + msg := testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Content: "final reply", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + }, + }) + + _, handled := m.preSend(context.Background(), "test", msg, ch) + if handled { + t.Fatal("expected preSend to leave final delivery to the channel") + } + if ch.clearedChatID != "123" { + t.Fatalf("expected tracked tool feedback state to be cleared, got %q", ch.clearedChatID) + } + if ch.dismissedChatID != "" { + t.Fatalf("expected tracked tool feedback message to be preserved, got dismissal for %q", ch.dismissedChatID) + } + if ch.finalizeCalled { + t.Fatal("expected separate message mode to skip in-place finalization") + } +} + func TestPreSend_StaleToolFeedbackDoesNotConsumeStreamActiveMarker(t *testing.T) { m := newTestManager() m.streamActive.Store("test:123", true) @@ -1153,6 +1262,38 @@ func TestPreSendMedia_LeavesTrackedMessageForChannelSend(t *testing.T) { } } +func TestPreSendMedia_SeparateMessagesClearsTrackedMessageWithoutDismiss(t *testing.T) { + m := newTestManager() + m.config = &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + ToolFeedback: config.ToolFeedbackConfig{ + Enabled: true, + SeparateMessages: true, + }, + }, + }, + } + + ch := &mockMessageEditor{} + + m.preSendMedia(context.Background(), "test", bus.OutboundMediaMessage{ + ChatID: "123", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + }, + }, ch) + + if ch.clearedChatID != "123" { + t.Fatalf("expected tracked tool feedback state to be cleared before media delivery, got %q", ch.clearedChatID) + } + if ch.dismissedChatID != "" { + t.Fatalf("expected tracked tool feedback message to be preserved"+ + " for media delivery, got %q", ch.dismissedChatID) + } +} + func TestSplitOutboundMessageContent_ToolFeedbackTruncatesInsteadOfSplitting(t *testing.T) { msg := testOutboundMessage(bus.OutboundMessage{ Channel: "test", @@ -1232,6 +1373,49 @@ func TestGetStreamer_FinalizeDismissesTrackedToolFeedback(t *testing.T) { } } +func TestGetStreamer_FinalizeSeparateMessagesClearsTrackedToolFeedback(t *testing.T) { + m := newTestManager() + m.config = &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + ToolFeedback: config.ToolFeedbackConfig{ + Enabled: true, + SeparateMessages: true, + }, + }, + }, + } + ch := &mockStreamingChannel{ + mockMessageEditor: mockMessageEditor{}, + streamer: &mockStreamer{ + finalizeFn: func(_ context.Context, content string) error { + if content != "final reply" { + t.Fatalf("unexpected finalize content: %q", content) + } + return nil + }, + }, + } + m.channels["test"] = ch + + streamer, ok := m.GetStreamer(context.Background(), "test", "123") + if !ok { + t.Fatal("expected streamer to be available") + } + if err := streamer.Finalize(context.Background(), "final reply"); err != nil { + t.Fatalf("Finalize() error = %v", err) + } + if ch.clearedChatID != "123" { + t.Fatalf("expected tracked tool feedback to be cleared for chat 123, got %q", ch.clearedChatID) + } + if ch.dismissedChatID != "" { + t.Fatalf("expected tracked tool feedback message to be preserved, got dismissal for %q", ch.dismissedChatID) + } + if _, ok := m.streamActive.Load("test:123"); !ok { + t.Fatal("expected streamActive marker to be recorded after finalize") + } +} + func TestGetStreamer_FinalizeDismissesResolvedTrackedToolFeedback(t *testing.T) { m := newTestManager() ch := &mockStreamingChannel{ diff --git a/pkg/config/config.go b/pkg/config/config.go index 161108638..6bb8d3ce6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -247,8 +247,9 @@ type SubTurnConfig struct { } type ToolFeedbackConfig struct { - Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` - MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"` + MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"` + SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` } type AgentDefaults struct { @@ -299,6 +300,13 @@ func (d *AgentDefaults) IsToolFeedbackEnabled() bool { return d.ToolFeedback.Enabled } +// IsToolFeedbackSeparateMessagesEnabled returns true when each tool feedback +// update should be sent as its own chat message instead of editing a single +// in-place progress message. +func (d *AgentDefaults) IsToolFeedbackSeparateMessagesEnabled() bool { + return d.ToolFeedback.SeparateMessages +} + // GetModelName returns the effective model name for the agent defaults. // It prefers the new "model_name" field but falls back to "model" for backward compatibility. func (d *AgentDefaults) GetModelName() string { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 624cc7305..2be1bcc67 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -787,6 +787,9 @@ func TestDefaultConfig_ToolFeedbackDisabled(t *testing.T) { if cfg.Agents.Defaults.ToolFeedback.Enabled { t.Fatal("DefaultConfig().Agents.Defaults.ToolFeedback.Enabled should be false") } + if cfg.Agents.Defaults.ToolFeedback.SeparateMessages { + t.Fatal("DefaultConfig().Agents.Defaults.ToolFeedback.SeparateMessages should be false") + } } func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) { @@ -807,6 +810,9 @@ func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) { if cfg.Agents.Defaults.ToolFeedback.Enabled { t.Fatal("agents.defaults.tool_feedback.enabled should remain false when unset in config file") } + if cfg.Agents.Defaults.ToolFeedback.SeparateMessages { + t.Fatal("agents.defaults.tool_feedback.separate_messages should remain false when unset in config file") + } } func TestLoadConfig_WebPreferNativeDefaultsTrueWhenUnset(t *testing.T) { diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 35ef7cdd8..f3aaca7ab 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -35,8 +35,9 @@ func DefaultConfig() *Config { SummarizeTokenPercent: 75, SteeringMode: "one-at-a-time", ToolFeedback: ToolFeedbackConfig{ - Enabled: false, - MaxArgsLength: 300, + Enabled: false, + MaxArgsLength: 300, + SeparateMessages: false, }, SplitOnMarker: false, }, diff --git a/web/frontend/src/components/config/config-page.tsx b/web/frontend/src/components/config/config-page.tsx index f50503dec..cc1a4624e 100644 --- a/web/frontend/src/components/config/config-page.tsx +++ b/web/frontend/src/components/config/config-page.tsx @@ -244,6 +244,7 @@ export function ConfigPage() { tool_feedback: { enabled: form.toolFeedbackEnabled, max_args_length: toolFeedbackMaxArgsLength, + separate_messages: form.toolFeedbackSeparateMessages, }, max_tokens: maxTokens, context_window: contextWindow, diff --git a/web/frontend/src/components/config/config-sections.tsx b/web/frontend/src/components/config/config-sections.tsx index 25c335ab1..fa6b3a079 100644 --- a/web/frontend/src/components/config/config-sections.tsx +++ b/web/frontend/src/components/config/config-sections.tsx @@ -113,6 +113,18 @@ export function AgentDefaultsSection({ } /> + {form.toolFeedbackEnabled && ( + + onFieldChange("toolFeedbackSeparateMessages", checked) + } + /> + )} + {form.toolFeedbackEnabled && (