diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index af44d4a0e..ad1e6af31 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -874,20 +874,56 @@ func toolFeedbackExplanationFromResponse( } explanation := strings.TrimSpace(response.Content) if explanation == "" { - explanation = strings.TrimSpace(response.Reasoning) + explanation = toolFeedbackExplanationFromToolCalls(response.ToolCalls) } if explanation == "" { - explanation = strings.TrimSpace(response.ReasoningContent) - } - if explanation == "" { - explanation = latestUserContent(messages) - if explanation != "" { - explanation = utils.ToolFeedbackContinuationHint + ": " + explanation - } + explanation = toolFeedbackExplanationFromMessages(messages) } return utils.Truncate(explanation, maxLen) } +func toolFeedbackExplanationFromToolCalls(toolCalls []providers.ToolCall) string { + for _, tc := range toolCalls { + if tc.ExtraContent == nil { + continue + } + if explanation := strings.TrimSpace(tc.ExtraContent.ToolFeedbackExplanation); explanation != "" { + return explanation + } + } + return "" +} + +func toolFeedbackExplanationForToolCall( + response *providers.LLMResponse, + toolCall providers.ToolCall, + messages []providers.Message, + maxLen int, +) string { + if toolCall.ExtraContent != nil { + if explanation := strings.TrimSpace(toolCall.ExtraContent.ToolFeedbackExplanation); explanation != "" { + return utils.Truncate(explanation, maxLen) + } + } + if response == nil { + return utils.Truncate(toolFeedbackExplanationFromMessages(messages), maxLen) + } + + explanation := strings.TrimSpace(response.Content) + if explanation == "" { + explanation = toolFeedbackExplanationFromMessages(messages) + } + return utils.Truncate(explanation, maxLen) +} + +func toolFeedbackExplanationFromMessages(messages []providers.Message) string { + explanation := latestUserContent(messages) + if explanation != "" { + return utils.ToolFeedbackContinuationHint + ": " + explanation + } + return "" +} + func shouldPublishToolFeedback(cfg *config.Config, ts *turnState) bool { if ts == nil || ts.channel == "" || ts.opts.SuppressToolFeedback { return false @@ -2723,12 +2759,6 @@ turnLoop: "count": len(normalizedToolCalls), "iteration": iteration, }) - toolFeedbackExplanation := toolFeedbackExplanationFromResponse( - response, - messages, - al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), - ) - allResponsesHandled := len(normalizedToolCalls) > 0 assistantMsg := providers.Message{ Role: "assistant", @@ -2737,6 +2767,12 @@ turnLoop: } for _, tc := range normalizedToolCalls { argumentsJSON, _ := json.Marshal(tc.Arguments) + toolFeedbackExplanation := toolFeedbackExplanationForToolCall( + response, + tc, + messages, + al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), + ) extraContent := tc.ExtraContent if strings.TrimSpace(toolFeedbackExplanation) != "" { if extraContent == nil { @@ -2822,6 +2858,12 @@ turnLoop: // Send tool feedback to chat channel if enabled (same as normal tool execution) if shouldPublishToolFeedback(al.cfg, ts) { + toolFeedbackExplanation := toolFeedbackExplanationForToolCall( + response, + tc, + messages, + al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), + ) feedbackMsg := utils.FormatToolFeedbackMessage(toolName, toolFeedbackExplanation) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) @@ -3095,6 +3137,12 @@ turnLoop: // Send tool feedback to chat channel if enabled (from HEAD) if shouldPublishToolFeedback(al.cfg, ts) { + toolFeedbackExplanation := toolFeedbackExplanationForToolCall( + response, + tc, + messages, + al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), + ) feedbackMsg := utils.FormatToolFeedbackMessage(tc.Name, toolFeedbackExplanation) fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 49555d8ec..1ed42b21b 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -1794,6 +1794,43 @@ func (m *toolFeedbackReasoningProvider) GetDefaultModel() string { return "tool-feedback-reasoning-model" } +type toolFeedbackExtraContentProvider struct { + filePath string + calls int +} + +func (m *toolFeedbackExtraContentProvider) Chat( + ctx context.Context, + messages []providers.Message, + tools []providers.ToolDefinition, + model string, + opts map[string]any, +) (*providers.LLMResponse, error) { + m.calls++ + if m.calls == 1 { + return &providers.LLMResponse{ + ToolCalls: []providers.ToolCall{{ + ID: "call_explicit_read_file", + Type: "function", + Name: "read_file", + Arguments: map[string]any{"path": m.filePath}, + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read README.md first to confirm the current project structure.", + }, + }}, + }, nil + } + + return &providers.LLMResponse{ + Content: "DONE", + ToolCalls: []providers.ToolCall{}, + }, nil +} + +func (m *toolFeedbackExtraContentProvider) GetDefaultModel() string { + return "tool-feedback-extra-content-model" +} + func TestToolFeedbackExplanationFromResponse_UsesCurrentContentFirst(t *testing.T) { response := &providers.LLMResponse{ Content: "Read README.md first", @@ -1811,10 +1848,15 @@ func TestToolFeedbackExplanationFromResponse_UsesCurrentContentFirst(t *testing. } } -func TestToolFeedbackExplanationFromResponse_FallsBackToReasoningContent(t *testing.T) { +func TestToolFeedbackExplanationFromResponse_UsesExplicitToolCallExtraContent(t *testing.T) { response := &providers.LLMResponse{ - Content: "", - ReasoningContent: "current reasoning fallback", + ToolCalls: []providers.ToolCall{{ + ID: "call_1", + Name: "read_file", + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read README.md first to confirm the current project structure.", + }, + }}, } messages := []providers.Message{ {Role: "user", Content: "check file"}, @@ -1823,15 +1865,73 @@ func TestToolFeedbackExplanationFromResponse_FallsBackToReasoningContent(t *test } got := toolFeedbackExplanationFromResponse(response, messages, 300) - if got != "current reasoning fallback" { - t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want reasoning fallback", got) + if got != "Read README.md first to confirm the current project structure." { + t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want explicit tool feedback explanation", got) } } -func TestToolFeedbackExplanationFromResponse_UsesLatestUserContentAsLastResort(t *testing.T) { +func TestToolFeedbackExplanationForToolCall_PrefersToolSpecificExtraContent(t *testing.T) { + response := &providers.LLMResponse{ + Content: "Shared explanation", + ToolCalls: []providers.ToolCall{ + { + ID: "call_1", + Name: "read_file", + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read README.md first.", + }, + }, + { + ID: "call_2", + Name: "edit_file", + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Update config example after reading it.", + }, + }, + }, + } + + got1 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], nil, 300) + got2 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[1], nil, 300) + if got1 != "Read README.md first." { + t.Fatalf("toolFeedbackExplanationForToolCall() first = %q, want tool-specific explanation", got1) + } + if got2 != "Update config example after reading it." { + t.Fatalf("toolFeedbackExplanationForToolCall() second = %q, want tool-specific explanation", got2) + } +} + +func TestToolFeedbackExplanationForToolCall_DoesNotReuseAnotherToolCallExplanation(t *testing.T) { + response := &providers.LLMResponse{ + ToolCalls: []providers.ToolCall{ + { + ID: "call_1", + Name: "read_file", + }, + { + ID: "call_2", + Name: "edit_file", + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Update config example after reading it.", + }, + }, + }, + } + messages := []providers.Message{ + {Role: "user", Content: "inspect the config and update the example"}, + } + + got := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], messages, 300) + want := utils.ToolFeedbackContinuationHint + ": inspect the config and update the example" + if got != want { + t.Fatalf("toolFeedbackExplanationForToolCall() = %q, want %q", got, want) + } +} + +func TestToolFeedbackExplanationFromResponse_DoesNotUseReasoningContent(t *testing.T) { response := &providers.LLMResponse{ Content: "", - ReasoningContent: "", + ReasoningContent: "hidden reasoning should not be shown", } messages := []providers.Message{ {Role: "user", Content: "check file"}, @@ -3770,7 +3870,7 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(t *testing.T) { } } -func TestProcessMessage_PublishesToolFeedbackFromReasoningContent(t *testing.T) { +func TestProcessMessage_DoesNotLeakReasoningContentInToolFeedback(t *testing.T) { tmpDir := t.TempDir() heartbeatFile := filepath.Join(tmpDir, "tool-feedback-reasoning.txt") if err := os.WriteFile(heartbeatFile, []byte("tool feedback task"), 0o644); err != nil { @@ -3819,11 +3919,17 @@ func TestProcessMessage_PublishesToolFeedbackFromReasoningContent(t *testing.T) if !strings.Contains(outbound.Content, "`read_file`") { t.Fatalf("tool feedback content = %q, want read_file summary", outbound.Content) } - if !strings.Contains(outbound.Content, "Read README.md first") { - t.Fatalf("tool feedback content = %q, want reasoning fallback", outbound.Content) + if !strings.Contains(outbound.Content, utils.ToolFeedbackContinuationHint) { + t.Fatalf("tool feedback content = %q, want continuation hint fallback", outbound.Content) + } + if !strings.Contains(outbound.Content, "check reasoning fallback") { + t.Fatalf("tool feedback content = %q, want current user intent fallback", outbound.Content) + } + if strings.Contains(outbound.Content, "Read README.md first") { + t.Fatalf("tool feedback content = %q, should not leak hidden reasoning", outbound.Content) } case <-time.After(2 * time.Second): - t.Fatal("expected outbound tool feedback for reasoning fallback") + t.Fatal("expected outbound tool feedback without leaking reasoning") } } diff --git a/pkg/channels/discord/discord.go b/pkg/channels/discord/discord.go index 34cf1fee2..e26e17710 100644 --- a/pkg/channels/discord/discord.go +++ b/pkg/channels/discord/discord.go @@ -49,6 +49,8 @@ type DiscordChannel struct { botUserID string // stored for mention checking bus *bus.MessageBus tts tts.TTSProvider + playTTSFn func(context.Context, *discordgo.VoiceConnection, string, uint64) + ttsVoiceFn func(string) (*discordgo.VoiceConnection, bool) voiceMu sync.RWMutex voiceSSRC map[string]map[uint32]string // guildID -> ssrc -> userID @@ -95,6 +97,8 @@ func NewDiscordChannel( bus: bus, voiceSSRC: make(map[string]map[uint32]string), } + ch.playTTSFn = ch.playTTS + ch.ttsVoiceFn = ch.voiceConnectionForTTS ch.progress = channels.NewToolFeedbackAnimator(ch.EditMessage) return ch, nil } @@ -180,26 +184,12 @@ func (c *DiscordChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]s } c.ClearToolFeedbackMessage(channelID) } - } else { - c.DismissToolFeedbackMessage(ctx, channelID) } - - if c.tts != nil && !isToolFeedback { - if ch, err := c.session.State.Channel(channelID); err == nil && ch.GuildID != "" { - if vc, ok := c.session.VoiceConnections[ch.GuildID]; ok && vc != nil { - // Cancel any previous TTS playback - c.ttsMu.Lock() - if c.cancelTTS != nil { - c.cancelTTS() - } - ttsCtx, ttsCancel := context.WithCancel(c.ctx) - c.ttsPlayID++ - playID := c.ttsPlayID - c.cancelTTS = ttsCancel - c.ttsMu.Unlock() - - go c.playTTS(ttsCtx, vc, msg.Content, playID) - } + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(channelID) + c.maybeStartTTS(channelID, msg.Content, isToolFeedback) + if !isToolFeedback { + if msgIDs, handled := c.FinalizeToolFeedbackMessage(ctx, msg); handled { + return msgIDs, nil } } @@ -213,12 +203,61 @@ func (c *DiscordChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]s } if isToolFeedback { c.RecordToolFeedbackMessage(channelID, msgID, msg.Content) - } else { - c.ClearToolFeedbackMessage(channelID) + } else if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, channelID, trackedMsgID) } return []string{msgID}, nil } +func (c *DiscordChannel) maybeStartTTS(channelID, content string, isToolFeedback bool) { + if c.tts == nil || isToolFeedback { + return + } + + voiceFn := c.ttsVoiceFn + if voiceFn == nil { + voiceFn = c.voiceConnectionForTTS + } + vc, ok := voiceFn(channelID) + if !ok || vc == nil { + return + } + + // Cancel any previous TTS playback. + c.ttsMu.Lock() + if c.cancelTTS != nil { + c.cancelTTS() + } + ttsCtx, ttsCancel := context.WithCancel(c.ctx) + c.ttsPlayID++ + playID := c.ttsPlayID + c.cancelTTS = ttsCancel + playFn := c.playTTSFn + c.ttsMu.Unlock() + + if playFn == nil { + playFn = c.playTTS + } + go playFn(ttsCtx, vc, content, playID) +} + +func (c *DiscordChannel) voiceConnectionForTTS(channelID string) (*discordgo.VoiceConnection, bool) { + if c.session == nil || c.session.State == nil { + return nil, false + } + + ch, err := c.session.State.Channel(channelID) + if err != nil || ch == nil || ch.GuildID == "" { + return nil, false + } + + vc, ok := c.session.VoiceConnections[ch.GuildID] + if !ok || vc == nil { + return nil, false + } + return vc, true +} + // SendMedia implements the channels.MediaSender interface. func (c *DiscordChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMessage) ([]string, error) { if !c.IsRunning() { @@ -229,7 +268,7 @@ func (c *DiscordChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMes if channelID == "" { return nil, fmt.Errorf("channel ID is empty") } - c.DismissToolFeedbackMessage(ctx, channelID) + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(channelID) store := c.GetMediaStore() if store == nil { @@ -311,6 +350,9 @@ func (c *DiscordChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMes if r.err != nil { return nil, fmt.Errorf("discord send media: %w", channels.ErrTemporary) } + if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, channelID, trackedMsgID) + } return []string{r.id}, nil case <-sendCtx.Done(): // Close all file readers @@ -366,6 +408,13 @@ func (c *DiscordChannel) currentToolFeedbackMessage(chatID string) (string, bool return c.progress.Current(chatID) } +func (c *DiscordChannel) takeToolFeedbackMessage(chatID string) (string, string, bool) { + if c.progress == nil { + return "", "", false + } + return c.progress.Take(chatID) +} + func (c *DiscordChannel) RecordToolFeedbackMessage(chatID, messageID, content string) { if c.progress == nil { return @@ -385,8 +434,39 @@ func (c *DiscordChannel) DismissToolFeedbackMessage(ctx context.Context, chatID if !ok { return } + c.dismissTrackedToolFeedbackMessage(ctx, chatID, msgID) +} + +func (c *DiscordChannel) dismissTrackedToolFeedbackMessage(ctx context.Context, chatID, messageID string) { + if strings.TrimSpace(chatID) == "" || strings.TrimSpace(messageID) == "" { + return + } c.ClearToolFeedbackMessage(chatID) - _ = c.DeleteMessage(ctx, chatID, msgID) + _ = c.DeleteMessage(ctx, chatID, messageID) +} + +func (c *DiscordChannel) finalizeTrackedToolFeedbackMessage( + ctx context.Context, + chatID string, + content string, + editFn func(context.Context, string, string, string) error, +) ([]string, bool) { + msgID, baseContent, ok := c.takeToolFeedbackMessage(chatID) + if !ok || editFn == nil { + return nil, false + } + if err := editFn(ctx, chatID, msgID, content); err != nil { + c.RecordToolFeedbackMessage(chatID, msgID, baseContent) + return nil, false + } + return []string{msgID}, true +} + +func (c *DiscordChannel) FinalizeToolFeedbackMessage(ctx context.Context, msg bus.OutboundMessage) ([]string, bool) { + if outboundMessageIsToolFeedback(msg) { + return nil, false + } + return c.finalizeTrackedToolFeedbackMessage(ctx, msg.ChatID, msg.Content, c.EditMessage) } func (c *DiscordChannel) sendChunk(ctx context.Context, channelID, content, replyToID string) (string, error) { diff --git a/pkg/channels/discord/discord_test.go b/pkg/channels/discord/discord_test.go index 254ea7ceb..e33ff81fd 100644 --- a/pkg/channels/discord/discord_test.go +++ b/pkg/channels/discord/discord_test.go @@ -9,12 +9,28 @@ import ( "reflect" "sync" "testing" + "time" "github.com/bwmarrin/discordgo" + "github.com/sipeed/picoclaw/pkg/audio/tts" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" ) +type stubTTSProvider struct{} + +func (stubTTSProvider) Name() string { return "stub-tts" } + +func (stubTTSProvider) Synthesize(context.Context, string) (io.ReadCloser, error) { + return io.NopCloser(&noopReader{}), nil +} + +type noopReader struct{} + +func (*noopReader) Read(p []byte) (int, error) { + return 0, io.EOF +} + func TestApplyDiscordProxy_CustomProxy(t *testing.T) { session, err := discordgo.New("Bot test-token") if err != nil { @@ -109,11 +125,9 @@ func TestSend_NonToolFeedbackDeletesTrackedProgressMessage(t *testing.T) { mu.Unlock() switch { - case r.Method == http.MethodDelete && r.URL.Path == "/channels/chat-1/messages/prog-1": - w.WriteHeader(http.StatusNoContent) - case r.Method == http.MethodPost && r.URL.Path == "/channels/chat-1/messages": + case r.Method == http.MethodPatch && r.URL.Path == "/channels/chat-1/messages/prog-1": w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"id":"final-1"}`) + _, _ = io.WriteString(w, `{"id":"prog-1"}`) default: t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) } @@ -154,7 +168,7 @@ func TestSend_NonToolFeedbackDeletesTrackedProgressMessage(t *testing.T) { if err != nil { t.Fatalf("Send() error = %v", err) } - if got, want := ids, []string{"final-1"}; !reflect.DeepEqual(got, want) { + if got, want := ids, []string{"prog-1"}; !reflect.DeepEqual(got, want) { t.Fatalf("Send() ids = %v, want %v", got, want) } if _, ok := ch.currentToolFeedbackMessage("chat-1"); ok { @@ -164,10 +178,114 @@ func TestSend_NonToolFeedbackDeletesTrackedProgressMessage(t *testing.T) { mu.Lock() defer mu.Unlock() wantRequests := []string{ - "DELETE /channels/chat-1/messages/prog-1", - "POST /channels/chat-1/messages", + "PATCH /channels/chat-1/messages/prog-1", } if !reflect.DeepEqual(requests, wantRequests) { t.Fatalf("requests = %v, want %v", requests, wantRequests) } } + +func TestFinalizeTrackedToolFeedbackMessage_StopsTrackingBeforeEdit(t *testing.T) { + ch := &DiscordChannel{ + progress: channels.NewToolFeedbackAnimator(nil), + } + ch.RecordToolFeedbackMessage("chat-1", "msg-1", "🔧 `read_file`") + + msgIDs, handled := ch.finalizeTrackedToolFeedbackMessage( + context.Background(), + "chat-1", + "final reply", + func(_ context.Context, chatID, messageID, content string) error { + if _, ok := ch.currentToolFeedbackMessage(chatID); ok { + t.Fatal("expected tracked tool feedback to be stopped before edit") + } + if chatID != "chat-1" || messageID != "msg-1" || content != "final reply" { + t.Fatalf("unexpected edit args: %s %s %s", chatID, messageID, content) + } + return nil + }, + ) + if !handled { + t.Fatal("expected finalizeTrackedToolFeedbackMessage to handle tracked message") + } + if got, want := msgIDs, []string{"msg-1"}; !reflect.DeepEqual(got, want) { + t.Fatalf("finalizeTrackedToolFeedbackMessage() ids = %v, want %v", got, want) + } +} + +func TestSend_NonToolFeedbackFinalizerStillStartsTTS(t *testing.T) { + var ( + mu sync.Mutex + requests []string + ) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + requests = append(requests, r.Method+" "+r.URL.Path) + mu.Unlock() + + switch { + case r.Method == http.MethodPatch && r.URL.Path == "/channels/chat-1/messages/prog-1": + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"id":"prog-1"}`) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) + defer server.Close() + + origChannels := discordgo.EndpointChannels + discordgo.EndpointChannels = server.URL + "/channels/" + defer func() { + discordgo.EndpointChannels = origChannels + }() + + session, err := discordgo.New("Bot test-token") + if err != nil { + t.Fatalf("discordgo.New() error: %v", err) + } + session.Client = server.Client() + + ttsStarted := make(chan string, 1) + ch := &DiscordChannel{ + BaseChannel: channels.NewBaseChannel("discord", nil, bus.NewMessageBus(), nil), + session: session, + ctx: context.Background(), + typingStop: make(map[string]chan struct{}), + voiceSSRC: make(map[string]map[uint32]string), + tts: tts.TTSProvider(stubTTSProvider{}), + } + ch.ttsVoiceFn = func(string) (*discordgo.VoiceConnection, bool) { + return &discordgo.VoiceConnection{}, true + } + ch.playTTSFn = func(_ context.Context, _ *discordgo.VoiceConnection, text string, _ uint64) { + ttsStarted <- text + } + ch.progress = channels.NewToolFeedbackAnimator(ch.EditMessage) + ch.SetRunning(true) + ch.RecordToolFeedbackMessage("chat-1", "prog-1", "🔧 `read_file`") + + ids, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "chat-1", + Content: "final reply", + Context: bus.InboundContext{ + Channel: "discord", + ChatID: "chat-1", + }, + }) + if err != nil { + t.Fatalf("Send() error = %v", err) + } + if got, want := ids, []string{"prog-1"}; !reflect.DeepEqual(got, want) { + t.Fatalf("Send() ids = %v, want %v", got, want) + } + + select { + case got := <-ttsStarted: + if got != "final reply" { + t.Fatalf("TTS content = %q, want final reply", got) + } + case <-time.After(2 * time.Second): + t.Fatal("expected TTS to start for finalized tracked tool feedback reply") + } +} diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 180c472b9..13dfe771a 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -156,6 +156,7 @@ func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]st } isToolFeedback := outboundMessageIsToolFeedback(msg) + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(msg.ChatID) if isToolFeedback { animatedContent := channels.InitialAnimatedToolFeedbackContent(msg.Content) if msgID, ok := c.currentToolFeedbackMessage(msg.ChatID); ok { @@ -169,7 +170,6 @@ func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]st if msgIDs, handled := c.FinalizeToolFeedbackMessage(ctx, msg); handled { return msgIDs, nil } - c.DismissToolFeedbackMessage(ctx, msg.ChatID) } // Build interactive card with markdown content @@ -181,15 +181,14 @@ func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]st if err != nil { // If card build fails, fall back to plain text msgID, sendErr := c.sendText(ctx, msg.ChatID, sendContent) - if sendErr == nil && isToolFeedback { - c.RecordToolFeedbackMessage(msg.ChatID, msgID, msg.Content) - } - if !isToolFeedback { - c.ClearToolFeedbackMessage(msg.ChatID) - } if sendErr != nil { return nil, sendErr } + if isToolFeedback { + c.RecordToolFeedbackMessage(msg.ChatID, msgID, msg.Content) + } else if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) + } return []string{msgID}, nil } @@ -198,8 +197,8 @@ func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]st if err == nil { if isToolFeedback { c.RecordToolFeedbackMessage(msg.ChatID, msgID, msg.Content) - } else { - c.ClearToolFeedbackMessage(msg.ChatID) + } else if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) } return []string{msgID}, nil } @@ -220,8 +219,8 @@ func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]st if textErr == nil { if isToolFeedback { c.RecordToolFeedbackMessage(msg.ChatID, msgID, msg.Content) - } else { - c.ClearToolFeedbackMessage(msg.ChatID) + } else if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) } return []string{msgID}, nil } @@ -329,6 +328,13 @@ func (c *FeishuChannel) currentToolFeedbackMessage(chatID string) (string, bool) return c.progress.Current(chatID) } +func (c *FeishuChannel) takeToolFeedbackMessage(chatID string) (string, string, bool) { + if c.progress == nil { + return "", "", false + } + return c.progress.Take(chatID) +} + func (c *FeishuChannel) RecordToolFeedbackMessage(chatID, messageID, content string) { if c.progress == nil { return @@ -348,8 +354,15 @@ func (c *FeishuChannel) DismissToolFeedbackMessage(ctx context.Context, chatID s if !ok { return } + c.dismissTrackedToolFeedbackMessage(ctx, chatID, msgID) +} + +func (c *FeishuChannel) dismissTrackedToolFeedbackMessage(ctx context.Context, chatID, messageID string) { + if strings.TrimSpace(chatID) == "" || strings.TrimSpace(messageID) == "" { + return + } c.ClearToolFeedbackMessage(chatID) - _ = c.DeleteMessage(ctx, chatID, msgID) + _ = c.DeleteMessage(ctx, chatID, messageID) } func (c *FeishuChannel) finalizeTrackedToolFeedbackMessage( @@ -358,14 +371,14 @@ func (c *FeishuChannel) finalizeTrackedToolFeedbackMessage( content string, editFn func(context.Context, string, string, string) error, ) ([]string, bool) { - msgID, ok := c.currentToolFeedbackMessage(chatID) + msgID, baseContent, ok := c.takeToolFeedbackMessage(chatID) if !ok || editFn == nil { return nil, false } if err := editFn(ctx, chatID, msgID, content); err != nil { + c.RecordToolFeedbackMessage(chatID, msgID, baseContent) return nil, false } - c.ClearToolFeedbackMessage(chatID) return []string{msgID}, true } @@ -448,7 +461,7 @@ func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMess if !c.IsRunning() { return nil, channels.ErrNotRunning } - c.DismissToolFeedbackMessage(ctx, msg.ChatID) + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(msg.ChatID) if msg.ChatID == "" { return nil, fmt.Errorf("chat ID is empty: %w", channels.ErrSendFailed) @@ -465,6 +478,10 @@ func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMess } } + if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) + } + return nil, nil } diff --git a/pkg/channels/feishu/feishu_64_test.go b/pkg/channels/feishu/feishu_64_test.go index 5afc2d360..236f3ed23 100644 --- a/pkg/channels/feishu/feishu_64_test.go +++ b/pkg/channels/feishu/feishu_64_test.go @@ -312,6 +312,34 @@ func TestFinalizeTrackedToolFeedbackMessage_ClearAfterSuccessfulEdit(t *testing. } } +func TestFinalizeTrackedToolFeedbackMessage_StopsTrackingBeforeEdit(t *testing.T) { + ch := &FeishuChannel{ + progress: channels.NewToolFeedbackAnimator(nil), + } + ch.RecordToolFeedbackMessage("chat-1", "msg-1", "🔧 `read_file`") + + msgIDs, handled := ch.finalizeTrackedToolFeedbackMessage( + context.Background(), + "chat-1", + "final reply", + func(_ context.Context, chatID, messageID, content string) error { + if _, ok := ch.currentToolFeedbackMessage(chatID); ok { + t.Fatal("expected tracked tool feedback to be stopped before edit") + } + if chatID != "chat-1" || messageID != "msg-1" || content != "final reply" { + t.Fatalf("unexpected edit args: %s %s %s", chatID, messageID, content) + } + return nil + }, + ) + if !handled { + t.Fatal("expected finalizeTrackedToolFeedbackMessage to handle tracked message") + } + if len(msgIDs) != 1 || msgIDs[0] != "msg-1" { + t.Fatalf("unexpected msgIDs: %v", msgIDs) + } +} + func TestFinalizeTrackedToolFeedbackMessage_EditFailureKeepsTrackedMessage(t *testing.T) { ch := &FeishuChannel{ progress: channels.NewToolFeedbackAnimator(nil), diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index f7959a9f7..bf59cff2e 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -138,6 +138,16 @@ func outboundMediaChatID(msg bus.OutboundMediaMessage) string { return msg.ChatID } +func dismissTrackedToolFeedbackMessage(ctx context.Context, ch Channel, chatID string) { + if cleaner, ok := ch.(toolFeedbackMessageCleaner); ok { + cleaner.DismissToolFeedbackMessage(ctx, chatID) + return + } + if tracker, ok := ch.(toolFeedbackMessageTracker); ok { + tracker.ClearToolFeedbackMessage(chatID) + } +} + // RecordPlaceholder registers a placeholder message for later editing. // Implements PlaceholderRecorder. func (m *Manager) RecordPlaceholder(channel, chatID, placeholderID string) { @@ -218,17 +228,13 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } } - if !outboundMessageIsToolFeedback(msg) { + isToolFeedback := outboundMessageIsToolFeedback(msg) + if !isToolFeedback { if finalizer, ok := ch.(toolFeedbackMessageFinalizer); ok { if msgIDs, handled := finalizer.FinalizeToolFeedbackMessage(ctx, msg); handled { return msgIDs, true } } - if cleaner, ok := ch.(toolFeedbackMessageCleaner); ok { - cleaner.DismissToolFeedbackMessage(ctx, chatID) - } else if tracker, ok := ch.(toolFeedbackMessageTracker); ok { - tracker.ClearToolFeedbackMessage(chatID) - } } // 3. If a stream already finalized this message, delete the placeholder and skip send @@ -243,6 +249,9 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } } } + if !isToolFeedback { + dismissTrackedToolFeedbackMessage(ctx, ch, chatID) + } return nil, true } @@ -251,12 +260,14 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess if entry, ok := v.(placeholderEntry); ok && entry.id != "" { if editor, ok := ch.(MessageEditor); ok { content := msg.Content - if outboundMessageIsToolFeedback(msg) { + if isToolFeedback { content = InitialAnimatedToolFeedbackContent(msg.Content) } if err := editor.EditMessage(ctx, chatID, entry.id, content); err == nil { - if tracker, ok := ch.(toolFeedbackMessageTracker); ok && outboundMessageIsToolFeedback(msg) { + if tracker, ok := ch.(toolFeedbackMessageTracker); ok && isToolFeedback { tracker.RecordToolFeedbackMessage(chatID, entry.id, msg.Content) + } else if !isToolFeedback { + dismissTrackedToolFeedbackMessage(ctx, ch, chatID) } return []string{entry.id}, true } @@ -276,12 +287,6 @@ func (m *Manager) preSendMedia(ctx context.Context, name string, msg bus.Outboun chatID := outboundMediaChatID(msg) key := name + ":" + chatID - if cleaner, ok := ch.(toolFeedbackMessageCleaner); ok { - cleaner.DismissToolFeedbackMessage(ctx, chatID) - } else if tracker, ok := ch.(toolFeedbackMessageTracker); ok { - tracker.ClearToolFeedbackMessage(chatID) - } - // 1. Stop typing if v, loaded := m.typingStops.LoadAndDelete(key); loaded { if entry, ok := v.(typingEntry); ok { @@ -360,22 +365,27 @@ func (m *Manager) GetStreamer(ctx context.Context, channelName, chatID string) ( // Mark streamActive on Finalize so preSend knows to clean up the placeholder key := channelName + ":" + chatID return &finalizeHookStreamer{ - Streamer: streamer, - onFinalize: func() { m.streamActive.Store(key, true) }, + Streamer: streamer, + onFinalize: func(finalizeCtx context.Context) { + dismissTrackedToolFeedbackMessage(finalizeCtx, ch, chatID) + m.streamActive.Store(key, true) + }, }, true } // finalizeHookStreamer wraps a Streamer to run a hook on Finalize. type finalizeHookStreamer struct { Streamer - onFinalize func() + onFinalize func(context.Context) } func (s *finalizeHookStreamer) Finalize(ctx context.Context, content string) error { if err := s.Streamer.Finalize(ctx, content); err != nil { return err } - s.onFinalize() + if s.onFinalize != nil { + s.onFinalize(ctx) + } return nil } @@ -817,8 +827,9 @@ func (m *Manager) runWorker(ctx context.Context, name string, w *channelWorker) // Collect all message chunks to send var chunks []string - // Step 1: Try marker-based splitting if enabled - if m.config != nil && m.config.Agents.Defaults.SplitOnMarker { + // Step 1: Try marker-based splitting if enabled. + // Tool feedback must stay a single message, so it skips marker splitting. + if m.config != nil && m.config.Agents.Defaults.SplitOnMarker && !outboundMessageIsToolFeedback(msg) { if markerChunks := SplitByMarker(msg.Content); len(markerChunks) > 1 { for _, chunk := range markerChunks { chunkMsg := msg diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index 2fe21b75b..a50e7b334 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -13,6 +13,7 @@ import ( "golang.org/x/time/rate" "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/utils" ) @@ -100,6 +101,33 @@ func (m *mockDeletingMediaChannel) DismissToolFeedbackMessage(_ context.Context, m.dismissedChatID = chatID } +type mockStreamer struct { + finalizeFn func(context.Context, string) error +} + +func (m *mockStreamer) Update(context.Context, string) error { return nil } + +func (m *mockStreamer) Finalize(ctx context.Context, content string) error { + if m.finalizeFn != nil { + return m.finalizeFn(ctx, content) + } + return nil +} + +func (m *mockStreamer) Cancel(context.Context) {} + +type mockStreamingChannel struct { + mockMessageEditor + streamer Streamer +} + +func (m *mockStreamingChannel) BeginStream(context.Context, string) (Streamer, error) { + if m.streamer == nil { + return nil, errors.New("missing streamer") + } + return m.streamer, nil +} + // newTestManager creates a minimal Manager suitable for unit tests. func newTestManager() *Manager { return &Manager{ @@ -835,7 +863,7 @@ func TestPreSend_ToolFeedbackPlaceholderEditRecordsTrackedMessage(t *testing.T) } } -func TestPreSend_NonToolFeedbackDismissesTrackedMessage(t *testing.T) { +func TestPreSend_NonToolFeedbackLeavesTrackedMessageForChannelSend(t *testing.T) { m := newTestManager() ch := &mockMessageEditor{} @@ -853,8 +881,8 @@ func TestPreSend_NonToolFeedbackDismissesTrackedMessage(t *testing.T) { if edited { t.Fatal("expected preSend to fall through when no placeholder exists") } - if ch.dismissedChatID != "123" { - t.Fatalf("expected tracked tool feedback to be dismissed for chat 123, got %q", ch.dismissedChatID) + if ch.dismissedChatID != "" { + t.Fatalf("expected tracked tool feedback cleanup to be deferred to channel send, got %q", ch.dismissedChatID) } } @@ -891,7 +919,7 @@ func TestPreSend_NonToolFeedbackFinalizerHandlesMessage(t *testing.T) { } } -func TestPreSendMedia_DismissesTrackedMessage(t *testing.T) { +func TestPreSendMedia_LeavesTrackedMessageForChannelSend(t *testing.T) { m := newTestManager() ch := &mockDeletingMediaChannel{} @@ -903,8 +931,8 @@ func TestPreSendMedia_DismissesTrackedMessage(t *testing.T) { }, }, ch) - if ch.dismissedChatID != "123" { - t.Fatalf("expected tracked tool feedback to be dismissed for media chat 123, got %q", ch.dismissedChatID) + if ch.dismissedChatID != "" { + t.Fatalf("expected tracked tool feedback cleanup to be deferred to channel media send, got %q", ch.dismissedChatID) } } @@ -932,6 +960,126 @@ func TestSplitOutboundMessageContent_ToolFeedbackTruncatesInsteadOfSplitting(t * } } +func TestGetStreamer_FinalizeDismissesTrackedToolFeedback(t *testing.T) { + m := newTestManager() + 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.dismissedChatID != "123" { + t.Fatalf("expected tracked tool feedback to be dismissed for chat 123, got %q", ch.dismissedChatID) + } + if _, ok := m.streamActive.Load("test:123"); !ok { + t.Fatal("expected streamActive marker to be recorded after finalize") + } +} + +func TestGetStreamer_FinalizeFailureDoesNotDismissTrackedToolFeedback(t *testing.T) { + m := newTestManager() + ch := &mockStreamingChannel{ + mockMessageEditor: mockMessageEditor{}, + streamer: &mockStreamer{ + finalizeFn: func(context.Context, string) error { + return errors.New("finalize failed") + }, + }, + } + 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.Fatal("expected Finalize() to fail") + } + if ch.dismissedChatID != "" { + t.Fatalf("expected no tool feedback dismissal on finalize failure, got %q", ch.dismissedChatID) + } + if _, ok := m.streamActive.Load("test:123"); ok { + t.Fatal("expected no streamActive marker after finalize failure") + } +} + +func TestRunWorker_ToolFeedbackSkipsMarkerSplitting(t *testing.T) { + m := newTestManager() + m.config = &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + SplitOnMarker: true, + }, + }, + } + + var ( + mu sync.Mutex + received []string + ) + ch := &mockChannelWithLength{ + mockChannel: mockChannel{ + sendFn: func(_ context.Context, msg bus.OutboundMessage) error { + mu.Lock() + received = append(received, msg.Content) + mu.Unlock() + return nil + }, + }, + maxLen: 200, + } + + w := &channelWorker{ + ch: ch, + queue: make(chan bus.OutboundMessage, 1), + done: make(chan struct{}), + limiter: rate.NewLimiter(rate.Inf, 1), + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go m.runWorker(ctx, "test", w) + + content := "🔧 `read_file`\nRead current config first.<|[SPLIT]|>Then update the example." + w.queue <- testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Content: content, + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + }, + }) + + time.Sleep(100 * time.Millisecond) + + mu.Lock() + defer mu.Unlock() + if len(received) != 1 { + t.Fatalf("len(received) = %d, want 1", len(received)) + } + if received[0] != content { + t.Fatalf("received[0] = %q, want %q", received[0], content) + } +} + func TestPreSend_PlaceholderEditFails_FallsThrough(t *testing.T) { m := newTestManager() diff --git a/pkg/channels/telegram/command_registration.go b/pkg/channels/telegram/command_registration.go index d3152ec3d..c6b362601 100644 --- a/pkg/channels/telegram/command_registration.go +++ b/pkg/channels/telegram/command_registration.go @@ -66,6 +66,10 @@ func (c *TelegramChannel) startCommandRegistration(ctx context.Context, defs []c if register == nil { register = c.RegisterCommands } + delayFn := c.commandRegDelayFn + if delayFn == nil { + delayFn = commandRegistrationDelay + } regCtx, cancel := context.WithCancel(ctx) c.commandRegCancel = cancel @@ -91,7 +95,7 @@ func (c *TelegramChannel) startCommandRegistration(ctx context.Context, defs []c return } - delay := commandRegistrationDelay(attempt) + delay := delayFn(attempt) logger.WarnCF("telegram", "Telegram command registration failed; will retry", map[string]any{ "error": err.Error(), "retry_after": delay.String(), diff --git a/pkg/channels/telegram/command_registration_test.go b/pkg/channels/telegram/command_registration_test.go index 26f891b2e..c30c6f68d 100644 --- a/pkg/channels/telegram/command_registration_test.go +++ b/pkg/channels/telegram/command_registration_test.go @@ -31,14 +31,12 @@ func TestStartCommandRegistration_DoesNotBlock(t *testing.T) { } func TestStartCommandRegistration_RetriesUntilSuccessThenStops(t *testing.T) { - ch := &TelegramChannel{} + ch := &TelegramChannel{ + commandRegDelayFn: func(int) time.Duration { return 5 * time.Millisecond }, + } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - origBackoff := commandRegistrationBackoff - commandRegistrationBackoff = []time.Duration{5 * time.Millisecond} - defer func() { commandRegistrationBackoff = origBackoff }() - var attempts atomic.Int32 ch.registerFunc = func(context.Context, []commands.Definition) error { n := attempts.Add(1) @@ -69,12 +67,10 @@ func TestStartCommandRegistration_RetriesUntilSuccessThenStops(t *testing.T) { } func TestStartCommandRegistration_StopsAfterCancel(t *testing.T) { - ch := &TelegramChannel{} + ch := &TelegramChannel{ + commandRegDelayFn: func(int) time.Duration { return 5 * time.Millisecond }, + } ctx, cancel := context.WithCancel(context.Background()) - - origBackoff := commandRegistrationBackoff - commandRegistrationBackoff = []time.Duration{5 * time.Millisecond} - defer func() { commandRegistrationBackoff = origBackoff }() defer cancel() var attempts atomic.Int32 diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index f6c1c1e7b..f38e2b42c 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -54,8 +54,9 @@ type TelegramChannel struct { tgCfg *config.TelegramSettings progress *channels.ToolFeedbackAnimator - registerFunc func(context.Context, []commands.Definition) error - commandRegCancel context.CancelFunc + registerFunc func(context.Context, []commands.Definition) error + commandRegDelayFn func(int) time.Duration + commandRegCancel context.CancelFunc } func NewTelegramChannel( @@ -198,17 +199,25 @@ func (c *TelegramChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([] } isToolFeedback := outboundMessageIsToolFeedback(msg) + toolFeedbackContent := msg.Content if isToolFeedback { - animatedContent := channels.InitialAnimatedToolFeedbackContent(msg.Content) + toolFeedbackContent = fitToolFeedbackForTelegram(msg.Content, useMarkdownV2, 4096) + } + if isToolFeedback { + animatedContent := channels.InitialAnimatedToolFeedbackContent(toolFeedbackContent) if msgID, ok := c.currentToolFeedbackMessage(msg.ChatID); ok { if err := c.EditMessage(ctx, msg.ChatID, msgID, animatedContent); err == nil { - c.RecordToolFeedbackMessage(msg.ChatID, msgID, msg.Content) + c.RecordToolFeedbackMessage(msg.ChatID, msgID, toolFeedbackContent) return []string{msgID}, nil } c.ClearToolFeedbackMessage(msg.ChatID) } - } else { - c.DismissToolFeedbackMessage(ctx, msg.ChatID) + } + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(msg.ChatID) + if !isToolFeedback { + if msgIDs, handled := c.FinalizeToolFeedbackMessage(ctx, msg); handled { + return msgIDs, nil + } } // The Manager already splits messages to ≤4000 chars (WithMaxMessageLength), @@ -218,7 +227,7 @@ func (c *TelegramChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([] var messageIDs []string queue := []string{msg.Content} if isToolFeedback { - queue = []string{channels.InitialAnimatedToolFeedbackContent(msg.Content)} + queue = []string{channels.InitialAnimatedToolFeedbackContent(toolFeedbackContent)} } for len(queue) > 0 { chunk := queue[0] @@ -227,6 +236,13 @@ func (c *TelegramChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([] content := parseContent(chunk, useMarkdownV2) if len([]rune(content)) > 4096 { + if isToolFeedback { + fittedChunk := fitToolFeedbackForTelegram(chunk, useMarkdownV2, 4096) + if fittedChunk != "" && fittedChunk != chunk { + queue = append([]string{fittedChunk}, queue...) + continue + } + } runeChunk := []rune(chunk) ratio := float64(len(runeChunk)) / float64(len([]rune(content))) smallerLen := int(float64(4096) * ratio * 0.95) // 5% safety margin @@ -294,9 +310,9 @@ func (c *TelegramChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([] } if isToolFeedback && len(messageIDs) > 0 { - c.RecordToolFeedbackMessage(msg.ChatID, messageIDs[0], msg.Content) - } else if !isToolFeedback { - c.ClearToolFeedbackMessage(msg.ChatID) + c.RecordToolFeedbackMessage(msg.ChatID, messageIDs[0], toolFeedbackContent) + } else if !isToolFeedback && hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) } return messageIDs, nil @@ -480,6 +496,13 @@ func (c *TelegramChannel) currentToolFeedbackMessage(chatID string) (string, boo return c.progress.Current(chatID) } +func (c *TelegramChannel) takeToolFeedbackMessage(chatID string) (string, string, bool) { + if c.progress == nil { + return "", "", false + } + return c.progress.Take(chatID) +} + func (c *TelegramChannel) RecordToolFeedbackMessage(chatID, messageID, content string) { if c.progress == nil { return @@ -499,8 +522,39 @@ func (c *TelegramChannel) DismissToolFeedbackMessage(ctx context.Context, chatID if !ok { return } + c.dismissTrackedToolFeedbackMessage(ctx, chatID, msgID) +} + +func (c *TelegramChannel) dismissTrackedToolFeedbackMessage(ctx context.Context, chatID, messageID string) { + if strings.TrimSpace(chatID) == "" || strings.TrimSpace(messageID) == "" { + return + } c.ClearToolFeedbackMessage(chatID) - _ = c.DeleteMessage(ctx, chatID, msgID) + _ = c.DeleteMessage(ctx, chatID, messageID) +} + +func (c *TelegramChannel) finalizeTrackedToolFeedbackMessage( + ctx context.Context, + chatID string, + content string, + editFn func(context.Context, string, string, string) error, +) ([]string, bool) { + msgID, baseContent, ok := c.takeToolFeedbackMessage(chatID) + if !ok || editFn == nil { + return nil, false + } + if err := editFn(ctx, chatID, msgID, content); err != nil { + c.RecordToolFeedbackMessage(chatID, msgID, baseContent) + return nil, false + } + return []string{msgID}, true +} + +func (c *TelegramChannel) FinalizeToolFeedbackMessage(ctx context.Context, msg bus.OutboundMessage) ([]string, bool) { + if outboundMessageIsToolFeedback(msg) { + return nil, false + } + return c.finalizeTrackedToolFeedbackMessage(ctx, msg.ChatID, msg.Content, c.EditMessage) } // SendPlaceholder implements channels.PlaceholderCapable. @@ -534,7 +588,7 @@ func (c *TelegramChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMe if !c.IsRunning() { return nil, channels.ErrNotRunning } - c.DismissToolFeedbackMessage(ctx, msg.ChatID) + trackedMsgID, hasTrackedMsg := c.currentToolFeedbackMessage(msg.ChatID) chatID, threadID, err := resolveTelegramOutboundTarget(msg.ChatID, &msg.Context) if err != nil { @@ -643,6 +697,10 @@ func (c *TelegramChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMe } } + if hasTrackedMsg { + c.dismissTrackedToolFeedbackMessage(ctx, msg.ChatID, trackedMsgID) + } + return messageIDs, nil } @@ -1014,6 +1072,37 @@ func parseContent(text string, useMarkdownV2 bool) string { return markdownToTelegramHTML(text) } +func fitToolFeedbackForTelegram(content string, useMarkdownV2 bool, maxParsedLen int) string { + content = strings.TrimSpace(content) + if content == "" || maxParsedLen <= 0 { + return "" + } + if len([]rune(parseContent(content, useMarkdownV2))) <= maxParsedLen { + return content + } + + low := 1 + high := len([]rune(content)) + best := utils.Truncate(content, 1) + + for low <= high { + mid := (low + high) / 2 + candidate := utils.FitToolFeedbackMessage(content, mid) + if candidate == "" { + high = mid - 1 + continue + } + if len([]rune(parseContent(candidate, useMarkdownV2))) <= maxParsedLen { + best = candidate + low = mid + 1 + continue + } + high = mid - 1 + } + + return best +} + // parseTelegramChatID splits "chatID/threadID" into its components. // Returns threadID=0 when no "/" is present (non-forum messages). func parseTelegramChatID(chatID string) (int64, int, error) { diff --git a/pkg/channels/telegram/telegram_group_command_filter_test.go b/pkg/channels/telegram/telegram_group_command_filter_test.go index 614b2ca7f..20b2004a9 100644 --- a/pkg/channels/telegram/telegram_group_command_filter_test.go +++ b/pkg/channels/telegram/telegram_group_command_filter_test.go @@ -108,7 +108,7 @@ func TestHandleMessage_GroupMentionOnly_BotCommandEntity(t *testing.T) { t.Fatalf("handleMessage error: %v", err) } - ctx, cancel := context.WithTimeout(context.Background(), 200*time.Microsecond) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) defer cancel() select { case <-ctx.Done(): diff --git a/pkg/channels/telegram/telegram_test.go b/pkg/channels/telegram/telegram_test.go index 6ce8f8615..74c56441a 100644 --- a/pkg/channels/telegram/telegram_test.go +++ b/pkg/channels/telegram/telegram_test.go @@ -282,10 +282,8 @@ func TestSend_NonToolFeedbackDeletesTrackedProgressMessage(t *testing.T) { caller := &stubCaller{ callFn: func(ctx context.Context, url string, data *ta.RequestData) (*ta.Response, error) { switch { - case strings.Contains(url, "deleteMessage"): - return successBoolResponse(t), nil - case strings.Contains(url, "sendMessage"): - return successResponseWithMessageID(t, 2), nil + case strings.Contains(url, "editMessageText"): + return successResponseWithMessageID(t, 1), nil default: t.Fatalf("unexpected API call: %s", url) return nil, nil @@ -301,14 +299,64 @@ func TestSend_NonToolFeedbackDeletesTrackedProgressMessage(t *testing.T) { }) assert.NoError(t, err) - assert.Equal(t, []string{"2"}, ids) - require.Len(t, caller.calls, 2) - assert.Contains(t, caller.calls[0].URL, "deleteMessage") - assert.Contains(t, caller.calls[1].URL, "sendMessage") + assert.Equal(t, []string{"1"}, ids) + require.Len(t, caller.calls, 1) + assert.Contains(t, caller.calls[0].URL, "editMessageText") _, ok := ch.currentToolFeedbackMessage("12345") assert.False(t, ok, "tracked tool feedback should be cleared after final reply") } +func TestFinalizeTrackedToolFeedbackMessage_StopsTrackingBeforeEdit(t *testing.T) { + ch := newTestChannel(t, &stubCaller{ + callFn: func(context.Context, string, *ta.RequestData) (*ta.Response, error) { + t.Fatal("unexpected API call") + return nil, nil + }, + }) + ch.RecordToolFeedbackMessage("12345", "1", "🔧 `read_file`") + + msgIDs, handled := ch.finalizeTrackedToolFeedbackMessage( + context.Background(), + "12345", + "final reply", + func(_ context.Context, chatID, messageID, content string) error { + _, ok := ch.currentToolFeedbackMessage(chatID) + assert.False(t, ok, "tracked tool feedback should be stopped before edit") + assert.Equal(t, "12345", chatID) + assert.Equal(t, "1", messageID) + assert.Equal(t, "final reply", content) + return nil + }, + ) + + assert.True(t, handled) + assert.Equal(t, []string{"1"}, msgIDs) +} + +func TestSend_ToolFeedbackStaysSingleMessageAfterHTMLExpansion(t *testing.T) { + caller := &stubCaller{ + callFn: func(ctx context.Context, url string, data *ta.RequestData) (*ta.Response, error) { + return successResponse(t), nil + }, + } + ch := newTestChannel(t, caller) + + _, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "12345", + Content: "🔧 `read_file`\n" + strings.Repeat("<", 2000), + Context: bus.InboundContext{ + Channel: "telegram", + ChatID: "12345", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + }, + }) + + assert.NoError(t, err) + assert.Len(t, caller.calls, 1, "tool feedback should stay a single Telegram message after HTML escaping") +} + func TestSend_LongMessage_SingleCall(t *testing.T) { // With WithMaxMessageLength(4000), the Manager pre-splits messages before // they reach Send(). A message at exactly 4000 chars should go through diff --git a/pkg/channels/tool_feedback_animator.go b/pkg/channels/tool_feedback_animator.go index 5c72b2f3b..12fe3f367 100644 --- a/pkg/channels/tool_feedback_animator.go +++ b/pkg/channels/tool_feedback_animator.go @@ -82,16 +82,22 @@ func (a *ToolFeedbackAnimator) Clear(chatID string) { if a == nil || strings.TrimSpace(chatID) == "" { return } - var entry *toolFeedbackAnimationState - a.mu.Lock() - if old, ok := a.entries[chatID]; ok { - entry = old - delete(a.entries, chatID) - } - a.mu.Unlock() + entry := a.detach(chatID) stopToolFeedbackAnimation(entry) } +func (a *ToolFeedbackAnimator) Take(chatID string) (string, string, bool) { + if a == nil || strings.TrimSpace(chatID) == "" { + return "", "", false + } + entry := a.detach(chatID) + if entry == nil || strings.TrimSpace(entry.messageID) == "" { + return "", "", false + } + stopToolFeedbackAnimation(entry) + return entry.messageID, entry.baseContent, true +} + func (a *ToolFeedbackAnimator) StopAll() { if a == nil { return @@ -109,6 +115,17 @@ func (a *ToolFeedbackAnimator) StopAll() { } } +func (a *ToolFeedbackAnimator) detach(chatID string) *toolFeedbackAnimationState { + if a == nil || strings.TrimSpace(chatID) == "" { + return nil + } + a.mu.Lock() + defer a.mu.Unlock() + entry := a.entries[chatID] + delete(a.entries, chatID) + return entry +} + func (a *ToolFeedbackAnimator) run(chatID string, entry *toolFeedbackAnimationState) { defer close(entry.done) diff --git a/pkg/channels/tool_feedback_animator_test.go b/pkg/channels/tool_feedback_animator_test.go index fa129b45b..6089b321b 100644 --- a/pkg/channels/tool_feedback_animator_test.go +++ b/pkg/channels/tool_feedback_animator_test.go @@ -42,3 +42,22 @@ func TestToolFeedbackAnimator_RecordCurrentAndClear(t *testing.T) { t.Fatalf("Current() after Clear = (%q, %v), want (\"\", false)", msgID, ok) } } + +func TestToolFeedbackAnimator_TakeStopsTrackingAndReturnsState(t *testing.T) { + animator := NewToolFeedbackAnimator(nil) + animator.Record("chat-1", "msg-1", "🔧 `read_file`\nChecking config") + + msgID, baseContent, ok := animator.Take("chat-1") + if !ok { + t.Fatal("Take() = not found, want tracked message") + } + if msgID != "msg-1" { + t.Fatalf("Take() msgID = %q, want msg-1", msgID) + } + if baseContent != "🔧 `read_file`\nChecking config" { + t.Fatalf("Take() baseContent = %q", baseContent) + } + if _, ok := animator.Current("chat-1"); ok { + t.Fatal("expected tracked message to be removed after Take()") + } +} diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 5f67e7e17..7feed68fc 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -462,13 +462,13 @@ func defaultChannels() ChannelsConfig { "use_markdown_v2": false, }, }, - "feishu": map[string]any{}, - "discord": map[string]any{ - "placeholder": map[string]any{"enabled": true, "text": []string{"Thinking... 💭"}}, - }, - "maixcam": map[string]any{ - "settings": map[string]any{"host": "0.0.0.0", "port": 18790}, - }, + "feishu": map[string]any{}, + "discord": map[string]any{ + "placeholder": map[string]any{"enabled": true, "text": []string{"Thinking... 💭"}}, + }, + "maixcam": map[string]any{ + "settings": map[string]any{"host": "0.0.0.0", "port": 18790}, + }, "qq": map[string]any{ "settings": map[string]any{"max_message_length": 2000}, }, diff --git a/pkg/providers/common/common.go b/pkg/providers/common/common.go index 90142fb8b..f0edb4640 100644 --- a/pkg/providers/common/common.go +++ b/pkg/providers/common/common.go @@ -70,11 +70,23 @@ func NewHTTPClient(proxy string) *http.Client { // It mirrors protocoltypes.Message but omits SystemParts, which is an // internal field that would be unknown to third-party endpoints. type openaiMessage struct { - Role string `json:"role"` - Content string `json:"content"` - ReasoningContent string `json:"reasoning_content,omitempty"` - ToolCalls []ToolCall `json:"tool_calls,omitempty"` - ToolCallID string `json:"tool_call_id,omitempty"` + Role string `json:"role"` + Content string `json:"content"` + ReasoningContent string `json:"reasoning_content,omitempty"` + ToolCalls []openaiToolCall `json:"tool_calls,omitempty"` + ToolCallID string `json:"tool_call_id,omitempty"` +} + +type openaiToolCall struct { + ID string `json:"id"` + Type string `json:"type,omitempty"` + Function *openaiFunctionCall `json:"function,omitempty"` +} + +type openaiFunctionCall struct { + Name string `json:"name"` + Arguments string `json:"arguments"` + ThoughtSignature string `json:"thought_signature,omitempty"` } // SerializeMessages converts internal Message structs to the OpenAI wire format. @@ -84,12 +96,13 @@ type openaiMessage struct { func SerializeMessages(messages []Message) []any { out := make([]any, 0, len(messages)) for _, m := range messages { + toolCalls := serializeToolCalls(m.ToolCalls) if len(m.Media) == 0 { out = append(out, openaiMessage{ Role: m.Role, Content: m.Content, ReasoningContent: m.ReasoningContent, - ToolCalls: m.ToolCalls, + ToolCalls: toolCalls, ToolCallID: m.ToolCallID, }) continue @@ -132,8 +145,8 @@ func SerializeMessages(messages []Message) []any { if m.ToolCallID != "" { msg["tool_call_id"] = m.ToolCallID } - if len(m.ToolCalls) > 0 { - msg["tool_calls"] = m.ToolCalls + if len(toolCalls) > 0 { + msg["tool_calls"] = toolCalls } if m.ReasoningContent != "" { msg["reasoning_content"] = m.ReasoningContent @@ -143,6 +156,44 @@ func SerializeMessages(messages []Message) []any { return out } +func serializeToolCalls(toolCalls []ToolCall) []openaiToolCall { + if len(toolCalls) == 0 { + return nil + } + + out := make([]openaiToolCall, 0, len(toolCalls)) + for _, tc := range toolCalls { + wireCall := openaiToolCall{ + ID: tc.ID, + Type: tc.Type, + } + + if tc.Function != nil { + wireCall.Function = &openaiFunctionCall{ + Name: tc.Function.Name, + Arguments: tc.Function.Arguments, + ThoughtSignature: tc.Function.ThoughtSignature, + } + } else if tc.Name != "" || len(tc.Arguments) > 0 || tc.ThoughtSignature != "" { + argsJSON := "{}" + if len(tc.Arguments) > 0 { + if encoded, err := json.Marshal(tc.Arguments); err == nil { + argsJSON = string(encoded) + } + } + wireCall.Function = &openaiFunctionCall{ + Name: tc.Name, + Arguments: argsJSON, + ThoughtSignature: tc.ThoughtSignature, + } + } + + out = append(out, wireCall) + } + + return out +} + func parseDataAudioURL(mediaURL string) (format, data string, ok bool) { if !strings.HasPrefix(mediaURL, "data:audio/") { return "", "", false @@ -185,6 +236,7 @@ func ParseResponse(body io.Reader) (*LLMResponse, error) { Google *struct { ThoughtSignature string `json:"thought_signature"` } `json:"google"` + ToolFeedbackExplanation string `json:"tool_feedback_explanation"` } `json:"extra_content"` } `json:"tool_calls"` } `json:"message"` @@ -228,11 +280,17 @@ func ParseResponse(body io.Reader) (*LLMResponse, error) { ThoughtSignature: thoughtSignature, } - if thoughtSignature != "" { - toolCall.ExtraContent = &ExtraContent{ - Google: &GoogleExtra{ + if tc.ExtraContent != nil { + extraContent := &ExtraContent{ + ToolFeedbackExplanation: tc.ExtraContent.ToolFeedbackExplanation, + } + if thoughtSignature != "" { + extraContent.Google = &GoogleExtra{ ThoughtSignature: thoughtSignature, - }, + } + } + if extraContent.Google != nil || strings.TrimSpace(extraContent.ToolFeedbackExplanation) != "" { + toolCall.ExtraContent = extraContent } } diff --git a/pkg/providers/common/common_test.go b/pkg/providers/common/common_test.go index c107bb665..b268d37a5 100644 --- a/pkg/providers/common/common_test.go +++ b/pkg/providers/common/common_test.go @@ -162,6 +162,43 @@ func TestSerializeMessages_StripsSystemParts(t *testing.T) { } } +func TestSerializeMessages_StripsInternalToolCallExtraContent(t *testing.T) { + messages := []Message{ + { + Role: "assistant", + ToolCalls: []ToolCall{{ + ID: "call_1", + Type: "function", + Function: &FunctionCall{ + Name: "read_file", + Arguments: `{"path":"README.md"}`, + ThoughtSignature: "sig-1", + }, + ExtraContent: &ExtraContent{ + Google: &GoogleExtra{ + ThoughtSignature: "sig-ignored-here", + }, + ToolFeedbackExplanation: "Read README.md first.", + }, + }}, + }, + } + + result := SerializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatalf("json.Marshal() error = %v", err) + } + payload := string(data) + if strings.Contains(payload, "extra_content") { + t.Fatalf("serialized payload should not include internal extra_content: %s", payload) + } + if !strings.Contains(payload, "thought_signature") { + t.Fatalf("serialized payload should preserve function thought_signature: %s", payload) + } +} + // --- ParseResponse tests --- func TestParseResponse_BasicContent(t *testing.T) { @@ -234,6 +271,27 @@ func TestParseResponse_WithReasoningContent(t *testing.T) { } } +func TestParseResponse_WithToolFeedbackExplanationExtraContent(t *testing.T) { + body := `{"choices":[{"message":{"content":"","tool_calls":[{"id":"call_1","type":"function","function":{"name":"test_tool","arguments":"{}"},"extra_content":{"tool_feedback_explanation":"Check the current config before editing."}}]},"finish_reason":"tool_calls"}]}` + out, err := ParseResponse(strings.NewReader(body)) + if err != nil { + t.Fatalf("ParseResponse() error = %v", err) + } + if len(out.ToolCalls) != 1 { + t.Fatalf("len(ToolCalls) = %d, want 1", len(out.ToolCalls)) + } + if out.ToolCalls[0].ExtraContent == nil { + t.Fatal("ExtraContent is nil") + } + if out.ToolCalls[0].ExtraContent.ToolFeedbackExplanation != "Check the current config before editing." { + t.Fatalf( + "ToolFeedbackExplanation = %q, want %q", + out.ToolCalls[0].ExtraContent.ToolFeedbackExplanation, + "Check the current config before editing.", + ) + } +} + func TestParseResponse_InvalidJSON(t *testing.T) { _, err := ParseResponse(strings.NewReader("not json")) if err == nil { diff --git a/web/backend/api/session_test.go b/web/backend/api/session_test.go index e2cc7b010..fe24de8d3 100644 --- a/web/backend/api/session_test.go +++ b/web/backend/api/session_test.go @@ -691,6 +691,80 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) } } +func TestHandleGetSession_DoesNotExposeLegacyToolArgumentsWhenExplanationMissing(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + cfg.Agents.Defaults.ToolFeedback.MaxArgsLength = 20 + if err := config.SaveConfig(configPath, cfg); err != nil { + t.Fatalf("SaveConfig() error = %v", err) + } + + dir := sessionsTestDir(t, configPath) + store, err := memory.NewJSONLStore(dir) + if err != nil { + t.Fatalf("NewJSONLStore() error = %v", err) + } + + argsJSON := `{"path":"README.md","start_line":1,"end_line":10,"extra":"abcdefghijklmnopqrstuvwxyz"}` + sessionKey := picoSessionPrefix + "detail-tool-summary-legacy-args" + if err := store.AddFullMessage(nil, sessionKey, providers.Message{Role: "user", Content: "check file"}); err != nil { + t.Fatalf("AddFullMessage(user) error = %v", err) + } + if err := store.AddFullMessage(nil, sessionKey, providers.Message{ + Role: "assistant", + ToolCalls: []providers.ToolCall{{ + ID: "call_1", + Type: "function", + Function: &providers.FunctionCall{ + Name: "read_file", + Arguments: argsJSON, + }, + }}, + }); err != nil { + t.Fatalf("AddFullMessage(assistant) error = %v", err) + } + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/sessions/detail-tool-summary-legacy-args", nil) + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + + var resp struct { + Messages []struct { + Role string `json:"role"` + Content string `json:"content"` + } `json:"messages"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + if len(resp.Messages) < 2 { + t.Fatalf("len(resp.Messages) = %d, want at least 2", len(resp.Messages)) + } + + if !strings.Contains(resp.Messages[1].Content, "`read_file`") { + t.Fatalf("tool summary = %q, want read_file summary", resp.Messages[1].Content) + } + if resp.Messages[1].Content != "🔧 `read_file`" { + t.Fatalf("tool summary = %q, want tool name only when explanation is missing", resp.Messages[1].Content) + } + if strings.Contains(resp.Messages[1].Content, argsJSON) { + t.Fatalf("tool summary = %q, should not expose legacy args", resp.Messages[1].Content) + } +} + func TestHandleGetSession_IncludesMediaOnlyMessages(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()