From 9ea8e6167ac8006b6cc180a0dfcd386f0f009d3b Mon Sep 17 00:00:00 2001 From: lxowalle Date: Fri, 17 Apr 2026 14:49:56 +0800 Subject: [PATCH] fix(tool-feedback): unify fallback and single-message delivery --- docs/channels/discord/README.md | 2 +- pkg/agent/loop.go | 18 +++++++++----- pkg/agent/loop_test.go | 19 +++++++++++---- pkg/channels/manager.go | 28 +++++++++++++++------- pkg/channels/manager_test.go | 25 ++++++++++++++++++++ pkg/config/config.go | 2 +- pkg/providers/protocoltypes/types.go | 3 ++- pkg/utils/tool_feedback.go | 34 +++++++++++++++++++++++++++ pkg/utils/tool_feedback_test.go | 16 +++++++++++++ web/backend/api/session.go | 16 ++++--------- web/backend/api/session_test.go | 6 ++++- web/frontend/src/i18n/locales/en.json | 6 ++--- web/frontend/src/i18n/locales/zh.json | 6 ++--- 13 files changed, 139 insertions(+), 42 deletions(-) diff --git a/docs/channels/discord/README.md b/docs/channels/discord/README.md index 180834324..741bc64a1 100644 --- a/docs/channels/discord/README.md +++ b/docs/channels/discord/README.md @@ -54,7 +54,7 @@ Discord can show three different kinds of "working" feedback: ```text 🔧 `web_search` -{"query":"picoclaw release notes"} +Checking the latest PicoClaw release notes before I answer. ``` If you only see `Bot is typing`, check that `placeholder.enabled` or `tool_feedback.enabled` is actually set in your runtime config. diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 76fd23e02..af44d4a0e 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -851,13 +851,10 @@ func outboundMessageForTurnWithKind(ts *turnState, content, kind string) bus.Out return msg } -func previousAssistantContent(messages []providers.Message) string { +func latestUserContent(messages []providers.Message) string { for i := len(messages) - 1; i >= 0; i-- { msg := messages[i] - if msg.Role == "user" { - break - } - if msg.Role != "assistant" { + if msg.Role != "user" { continue } if content := strings.TrimSpace(msg.Content); content != "" { @@ -883,7 +880,10 @@ func toolFeedbackExplanationFromResponse( explanation = strings.TrimSpace(response.ReasoningContent) } if explanation == "" { - explanation = previousAssistantContent(messages) + explanation = latestUserContent(messages) + if explanation != "" { + explanation = utils.ToolFeedbackContinuationHint + ": " + explanation + } } return utils.Truncate(explanation, maxLen) } @@ -2738,6 +2738,12 @@ turnLoop: for _, tc := range normalizedToolCalls { argumentsJSON, _ := json.Marshal(tc.Arguments) extraContent := tc.ExtraContent + if strings.TrimSpace(toolFeedbackExplanation) != "" { + if extraContent == nil { + extraContent = &providers.ExtraContent{} + } + extraContent.ToolFeedbackExplanation = toolFeedbackExplanation + } thoughtSignature := "" if tc.Function != nil { thoughtSignature = tc.Function.ThoughtSignature diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index d463cb2a2..49555d8ec 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -24,6 +24,7 @@ import ( "github.com/sipeed/picoclaw/pkg/routing" "github.com/sipeed/picoclaw/pkg/session" "github.com/sipeed/picoclaw/pkg/tools" + "github.com/sipeed/picoclaw/pkg/utils" ) type fakeChannel struct{ id string } @@ -1827,7 +1828,7 @@ func TestToolFeedbackExplanationFromResponse_FallsBackToReasoningContent(t *test } } -func TestToolFeedbackExplanationFromResponse_UsesPreviousAssistantContentAsLastResort(t *testing.T) { +func TestToolFeedbackExplanationFromResponse_UsesLatestUserContentAsLastResort(t *testing.T) { response := &providers.LLMResponse{ Content: "", ReasoningContent: "", @@ -1835,12 +1836,14 @@ func TestToolFeedbackExplanationFromResponse_UsesPreviousAssistantContentAsLastR messages := []providers.Message{ {Role: "user", Content: "check file"}, {Role: "assistant", Content: "Previous turn explanation"}, + {Role: "user", Content: "Inspect README.md and update the config example."}, {Role: "tool", Content: "tool output", ToolCallID: "call_1"}, } got := toolFeedbackExplanationFromResponse(response, messages, 300) - if got != "Previous turn explanation" { - t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want previous assistant content", got) + want := utils.ToolFeedbackContinuationHint + ": Inspect README.md and update the config example." + if got != want { + t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want latest user content fallback", got) } } @@ -3744,8 +3747,14 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(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, "Why this tool should be executed") { - t.Fatalf("tool feedback content = %q, want no fallback explanation", 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 tool feedback") { + t.Fatalf("tool feedback content = %q, want current user intent fallback", outbound.Content) + } + if strings.Contains(outbound.Content, "Previous turn explanation") { + t.Fatalf("tool feedback content = %q, want no previous assistant fallback", outbound.Content) } if outbound.AgentID != "main" { t.Fatalf("tool feedback agent_id = %q, want main", outbound.AgentID) diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index 3ce5468eb..f7959a9f7 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -26,6 +26,7 @@ import ( "github.com/sipeed/picoclaw/pkg/health" "github.com/sipeed/picoclaw/pkg/logger" "github.com/sipeed/picoclaw/pkg/media" + "github.com/sipeed/picoclaw/pkg/utils" ) const ( @@ -820,14 +821,16 @@ func (m *Manager) runWorker(ctx context.Context, name string, w *channelWorker) if m.config != nil && m.config.Agents.Defaults.SplitOnMarker { if markerChunks := SplitByMarker(msg.Content); len(markerChunks) > 1 { for _, chunk := range markerChunks { - chunks = append(chunks, splitByLength(chunk, maxLen)...) + chunkMsg := msg + chunkMsg.Content = chunk + chunks = append(chunks, splitOutboundMessageContent(chunkMsg, maxLen)...) } } } // Step 2: Fallback to length-based splitting if no chunks from marker if len(chunks) == 0 { - chunks = splitByLength(msg.Content, maxLen) + chunks = splitOutboundMessageContent(msg, maxLen) } // Step 3: Send all chunks @@ -842,12 +845,16 @@ func (m *Manager) runWorker(ctx context.Context, name string, w *channelWorker) } } -// splitByLength splits content by maxLen if needed, otherwise returns single chunk. -func splitByLength(content string, maxLen int) []string { - if maxLen > 0 && len([]rune(content)) > maxLen { - return SplitMessage(content, maxLen) +// splitOutboundMessageContent splits regular outbound content by maxLen, but +// keeps tool feedback in a single message by truncating the explanation body. +func splitOutboundMessageContent(msg bus.OutboundMessage, maxLen int) []string { + if maxLen > 0 && len([]rune(msg.Content)) > maxLen { + if outboundMessageIsToolFeedback(msg) { + return []string{utils.FitToolFeedbackMessage(msg.Content, maxLen)} + } + return SplitMessage(msg.Content, maxLen) } - return []string{content} + return []string{msg.Content} } // sendWithRetry sends a message through the channel with rate limiting and @@ -1311,13 +1318,16 @@ func (m *Manager) SendMessage(ctx context.Context, msg bus.OutboundMessage) erro if mlp, ok := w.ch.(MessageLengthProvider); ok { maxLen = mlp.MaxMessageLength() } - if maxLen > 0 && len([]rune(msg.Content)) > maxLen { - for _, chunk := range SplitMessage(msg.Content, maxLen) { + if chunks := splitOutboundMessageContent(msg, maxLen); len(chunks) > 1 { + for _, chunk := range chunks { chunkMsg := msg chunkMsg.Content = chunk m.sendWithRetry(ctx, channelName, w, chunkMsg) } } else { + if len(chunks) == 1 { + msg.Content = chunks[0] + } m.sendWithRetry(ctx, channelName, w, msg) } return nil diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index 356b274b2..2fe21b75b 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/utils" ) // mockChannel is a test double that delegates Send to a configurable function. @@ -907,6 +908,30 @@ func TestPreSendMedia_DismissesTrackedMessage(t *testing.T) { } } +func TestSplitOutboundMessageContent_ToolFeedbackTruncatesInsteadOfSplitting(t *testing.T) { + msg := testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Content: "\U0001f527 `read_file`\nRead README.md first to confirm the current project structure before editing the config example.", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + }, + }) + + chunks := splitOutboundMessageContent(msg, 40) + if len(chunks) != 1 { + t.Fatalf("len(chunks) = %d, want 1", len(chunks)) + } + want := utils.FitToolFeedbackMessage(msg.Content, 40) + if chunks[0] != want { + t.Fatalf("chunk = %q, want %q", chunks[0], want) + } +} + func TestPreSend_PlaceholderEditFails_FallsThrough(t *testing.T) { m := newTestManager() diff --git a/pkg/config/config.go b/pkg/config/config.go index 5bc96fb12..547060bd6 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -286,7 +286,7 @@ func (d *AgentDefaults) GetMaxMediaSize() int { return DefaultMaxMediaSize } -// GetToolFeedbackMaxArgsLength returns the max args preview length for tool feedback messages. +// GetToolFeedbackMaxArgsLength returns the max visible text length for tool feedback messages. func (d *AgentDefaults) GetToolFeedbackMaxArgsLength() int { if d.ToolFeedback.MaxArgsLength > 0 { return d.ToolFeedback.MaxArgsLength diff --git a/pkg/providers/protocoltypes/types.go b/pkg/providers/protocoltypes/types.go index 194c1aa6f..1189577f1 100644 --- a/pkg/providers/protocoltypes/types.go +++ b/pkg/providers/protocoltypes/types.go @@ -11,7 +11,8 @@ type ToolCall struct { } type ExtraContent struct { - Google *GoogleExtra `json:"google,omitempty"` + Google *GoogleExtra `json:"google,omitempty"` + ToolFeedbackExplanation string `json:"tool_feedback_explanation,omitempty"` } type GoogleExtra struct { diff --git a/pkg/utils/tool_feedback.go b/pkg/utils/tool_feedback.go index 5dcd38393..1a8b6c747 100644 --- a/pkg/utils/tool_feedback.go +++ b/pkg/utils/tool_feedback.go @@ -5,6 +5,8 @@ import ( "strings" ) +const ToolFeedbackContinuationHint = "Continuing the current task." + // FormatToolFeedbackMessage renders the model-provided explanation for why a // tool is being executed. When the model does not provide one, it keeps only // the tool line and does not expose raw arguments or fallback text. @@ -21,3 +23,35 @@ func FormatToolFeedbackMessage(toolName, explanation string) string { return fmt.Sprintf("\U0001f527 `%s`\n%s", toolName, explanation) } + +// FitToolFeedbackMessage keeps tool feedback within a single outbound message. +// It preserves the first line when possible and truncates the explanation body +// instead of letting the message be split into multiple chunks. +func FitToolFeedbackMessage(content string, maxLen int) string { + content = strings.TrimSpace(content) + if content == "" || maxLen <= 0 { + return "" + } + if len([]rune(content)) <= maxLen { + return content + } + + firstLine, rest, hasRest := strings.Cut(content, "\n") + firstLine = strings.TrimSpace(firstLine) + rest = strings.TrimSpace(rest) + + if !hasRest || rest == "" { + return Truncate(firstLine, maxLen) + } + + if len([]rune(firstLine)) >= maxLen { + return Truncate(firstLine, maxLen) + } + + remaining := maxLen - len([]rune(firstLine)) - 1 + if remaining <= 0 { + return Truncate(firstLine, maxLen) + } + + return firstLine + "\n" + Truncate(rest, remaining) +} diff --git a/pkg/utils/tool_feedback_test.go b/pkg/utils/tool_feedback_test.go index 98f8d47ce..07cbf3351 100644 --- a/pkg/utils/tool_feedback_test.go +++ b/pkg/utils/tool_feedback_test.go @@ -25,3 +25,19 @@ func TestFormatToolFeedbackMessage_EmptyToolNameOmitsToolLine(t *testing.T) { t.Fatalf("FormatToolFeedbackMessage() = %q, want %q", got, want) } } + +func TestFitToolFeedbackMessage_TruncatesBodyWithinSingleMessage(t *testing.T) { + got := FitToolFeedbackMessage("\U0001f527 `read_file`\nRead README.md first to confirm the current project structure.", 40) + want := "\U0001f527 `read_file`\nRead README.md first to..." + if got != want { + t.Fatalf("FitToolFeedbackMessage() = %q, want %q", got, want) + } +} + +func TestFitToolFeedbackMessage_TruncatesSingleLineMessage(t *testing.T) { + got := FitToolFeedbackMessage("\U0001f527 `read_file`", 10) + want := "\U0001f527 `read..." + if got != want { + t.Fatalf("FitToolFeedbackMessage() = %q, want %q", got, want) + } +} diff --git a/web/backend/api/session.go b/web/backend/api/session.go index 3e6560570..c560325ea 100644 --- a/web/backend/api/session.go +++ b/web/backend/api/session.go @@ -529,31 +529,23 @@ func visibleAssistantToolSummaryMessages( messages := make([]sessionChatMessage, 0, len(toolCalls)) for _, tc := range toolCalls { name := tc.Name - argsJSON := "" if tc.Function != nil { if name == "" { name = tc.Function.Name } - argsJSON = tc.Function.Arguments } if strings.TrimSpace(name) == "" { continue } - if strings.TrimSpace(argsJSON) == "" && len(tc.Arguments) > 0 { - if encodedArgs, err := json.Marshal(tc.Arguments); err == nil { - argsJSON = string(encodedArgs) - } - } - - argsPreview := strings.TrimSpace(argsJSON) - if argsPreview == "" { - argsPreview = "{}" + explanation := "" + if tc.ExtraContent != nil { + explanation = strings.TrimSpace(tc.ExtraContent.ToolFeedbackExplanation) } messages = append(messages, sessionChatMessage{ Role: "assistant", - Content: utils.FormatToolFeedbackMessage(name, utils.Truncate(argsPreview, toolFeedbackMaxArgsLength)), + Content: utils.FormatToolFeedbackMessage(name, utils.Truncate(explanation, toolFeedbackMaxArgsLength)), }) } diff --git a/web/backend/api/session_test.go b/web/backend/api/session_test.go index dbb5f9cce..e2cc7b010 100644 --- a/web/backend/api/session_test.go +++ b/web/backend/api/session_test.go @@ -629,6 +629,7 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) } argsJSON := `{"path":"README.md","start_line":1,"end_line":10,"extra":"abcdefghijklmnopqrstuvwxyz"}` + explanation := "Read README.md first to confirm the current project structure before editing the config example." sessionKey := picoSessionPrefix + "detail-tool-summary-max-args" err = store.AddFullMessage(nil, sessionKey, providers.Message{Role: "user", Content: "check file"}) if err != nil { @@ -643,6 +644,9 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) Name: "read_file", Arguments: argsJSON, }, + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: explanation, + }, }}, }) if err != nil { @@ -675,7 +679,7 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) t.Fatalf("len(resp.Messages) = %d, want at least 2", len(resp.Messages)) } - wantPreview := utils.Truncate(argsJSON, 20) + wantPreview := utils.Truncate(explanation, 20) if !strings.Contains(resp.Messages[1].Content, wantPreview) { t.Fatalf("tool summary = %q, want preview %q", resp.Messages[1].Content, wantPreview) } diff --git a/web/frontend/src/i18n/locales/en.json b/web/frontend/src/i18n/locales/en.json index c96d4b71b..7a5c58b30 100644 --- a/web/frontend/src/i18n/locales/en.json +++ b/web/frontend/src/i18n/locales/en.json @@ -592,9 +592,9 @@ "split_on_marker": "Chatty Mode", "split_on_marker_hint": "Split long messages into short ones like real human chatting.", "tool_feedback_enabled": "Tool Feedback", - "tool_feedback_enabled_hint": "Send a short tool-call preview into the current chat before each tool execution.", - "tool_feedback_max_args_length": "Tool Feedback Args Preview Length", - "tool_feedback_max_args_length_hint": "Maximum number of argument characters shown in each tool feedback message. Set to 0 to use the default.", + "tool_feedback_enabled_hint": "Send a short execution note into the current chat before each tool runs.", + "tool_feedback_max_args_length": "Tool Feedback Length", + "tool_feedback_max_args_length_hint": "Maximum number of characters shown in each tool feedback message. Set to 0 to use the default.", "exec_enabled": "Allow Commands", "exec_enabled_hint": "Enable or disable command execution for the app. When disabled, no command requests will run.", "allow_remote": "Allow Remote Commands", diff --git a/web/frontend/src/i18n/locales/zh.json b/web/frontend/src/i18n/locales/zh.json index 4a9e59cf4..aaebfa625 100644 --- a/web/frontend/src/i18n/locales/zh.json +++ b/web/frontend/src/i18n/locales/zh.json @@ -592,9 +592,9 @@ "split_on_marker": "连续短消息", "split_on_marker_hint": "像真人聊天一样，把长难句拆成多条短消息快速发出", "tool_feedback_enabled": "工具反馈", - "tool_feedback_enabled_hint": "在每次执行工具前，先向当前会话发送一条简短的工具调用预览", - "tool_feedback_max_args_length": "工具反馈参数预览长度", - "tool_feedback_max_args_length_hint": "每条工具反馈消息中展示的参数字符上限。设为 0 时使用默认值", + "tool_feedback_enabled_hint": "在每次执行工具前，先向当前会话发送一条简短的执行说明", + "tool_feedback_max_args_length": "工具反馈长度", + "tool_feedback_max_args_length_hint": "每条工具反馈消息中展示的字符上限。设为 0 时使用默认值", "exec_enabled": "允许命令执行", "exec_enabled_hint": "控制应用是否允许执行命令。关闭后，所有命令请求都不会执行", "allow_remote": "允许远程命令执行",