diff --git a/config/config.example.json b/config/config.example.json index d87f711d0..91b896f96 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -16,7 +16,9 @@ "tool_feedback": { "enabled": false, "max_args_length": 300, - "separate_messages": false + "separate_messages": false, + "subagents": true, + "style": "raw" } } }, diff --git a/docs/operations/debug.md b/docs/operations/debug.md index eacd72380..9304f9a2b 100644 --- a/docs/operations/debug.md +++ b/docs/operations/debug.md @@ -66,7 +66,9 @@ Debug logs are server-side only. If you want the agent to send a visible notific "tool_feedback": { "enabled": true, "max_args_length": 300, - "separate_messages": true + "separate_messages": true, + "subagents": true, + "style": "raw" } } } @@ -80,6 +82,40 @@ When `enabled` is `true`, every tool call sends a short message to the chat befo {"query": "picoclaw release notes"} ``` +Set `style` to `working_summary` to use a compact, non-argument progress message that can be edited in place as tools run: + +```json +{ + "agents": { + "defaults": { + "tool_feedback": { + "enabled": true, + "separate_messages": false, + "subagents": true, + "style": "working_summary" + } + } + } +} +``` + +The message starts as: + +```text +Working... +• tool: `read_file` — `README.md` +``` + +When more tools run, editable channels merge recent tool lines into the same progress message: + +```text +Working... +• tool: `read_file` — `README.md` +• tool: `exec` — `test.sh` +• tool: `write_file` — `config.json` +``` + +The `working_summary` style intentionally does not show raw tool arguments, explanations, URLs, command arguments, environment variables, or full paths. It only shows the tool name plus a safe basename for file tools and executable commands where available. This keeps progress visible without exposing secrets in chat history. ### Options @@ -87,15 +123,19 @@ When `enabled` is `true`, every tool call sends a short message to the chat befo |---|---|---|---| | `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 | +| `subagents` | bool | `true` | Also publish visible tool feedback for subagent turns when `enabled` is true | | `max_args_length` | int | `300` | Maximum characters of the serialised arguments included in the notification | +| `style` | string | `raw` | Feedback format. Use `raw` for the original tool/explanation/argument preview, or `working_summary` for compact progress lines without raw arguments | ### Environment variables -Both fields can also be set via environment variables: +These fields can also be set via environment variables: ```bash PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED=true PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH=300 +PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES=false +PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_STYLE=working_summary ``` > **Note:** `tool_feedback` is independent of `--debug` mode. It works in production and does not require the gateway to be started with any special flag. diff --git a/pkg/agent/adapters/channelmanager.go b/pkg/agent/adapters/channelmanager.go index ad0840e86..98eeaed59 100644 --- a/pkg/agent/adapters/channelmanager.go +++ b/pkg/agent/adapters/channelmanager.go @@ -49,3 +49,12 @@ func (a *channelManagerAdapter) DismissToolFeedback( ) { a.inner.DismissToolFeedback(ctx, channel, chatID, outboundCtx) } + +func (a *channelManagerAdapter) DismissToolFeedbackForSession( + ctx context.Context, + channel, chatID string, + outboundCtx *bus.InboundContext, + sessionKey string, +) { + a.inner.DismissToolFeedbackForSession(ctx, channel, chatID, outboundCtx, sessionKey) +} diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 5749149c1..686ea9863 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -261,7 +261,14 @@ func (al *AgentLoop) Run(ctx context.Context) error { return } if continued != "" { - al.PublishResponseIfNeeded(ctx, target.Channel, target.ChatID, target.SessionKey, continued) + al.publishResponseWithContextIfNeeded( + ctx, + target.Channel, + target.ChatID, + target.SessionKey, + continued, + &m.Context, + ) } return } diff --git a/pkg/agent/agent_outbound.go b/pkg/agent/agent_outbound.go index f4a01adfd..3c4292e24 100644 --- a/pkg/agent/agent_outbound.go +++ b/pkg/agent/agent_outbound.go @@ -41,6 +41,14 @@ func (al *AgentLoop) publishResponseOrError( } func (al *AgentLoop) PublishResponseIfNeeded(ctx context.Context, channel, chatID, sessionKey, response string) { + al.publishResponseWithContextIfNeeded(ctx, channel, chatID, sessionKey, response, nil) +} + +func (al *AgentLoop) publishResponseWithContextIfNeeded( + ctx context.Context, + channel, chatID, sessionKey, response string, + inboundCtx *bus.InboundContext, +) { if response == "" { return } @@ -74,12 +82,21 @@ func (al *AgentLoop) PublishResponseIfNeeded(ctx context.Context, channel, chatI return } + agent := al.agentForSession(sessionKey) + agentID := "" + if agent != nil { + agentID = agent.ID + } msg := bus.OutboundMessage{ - Context: bus.NewOutboundContext(channel, chatID, ""), - Content: response, + Channel: channel, + ChatID: chatID, + Context: outboundContextFromInbound(inboundCtx, channel, chatID, ""), + AgentID: agentID, + SessionKey: sessionKey, + Content: response, } if sessionKey != "" { - msg.ContextUsage = computeContextUsage(al.agentForSession(sessionKey), sessionKey) + msg.ContextUsage = computeContextUsage(agent, sessionKey) } al.bus.PublishOutbound(ctx, msg) logger.InfoCF("agent", "Published outbound response", diff --git a/pkg/agent/agent_steering.go b/pkg/agent/agent_steering.go index 9b136e7cd..610bc2fad 100644 --- a/pkg/agent/agent_steering.go +++ b/pkg/agent/agent_steering.go @@ -58,7 +58,14 @@ func (al *AgentLoop) runTurnWithSteering(ctx context.Context, initialMsg bus.Inb // Publish final response if finalResponse != "" { - al.PublishResponseIfNeeded(ctx, target.Channel, target.ChatID, target.SessionKey, finalResponse) + al.publishResponseWithContextIfNeeded( + ctx, + target.Channel, + target.ChatID, + target.SessionKey, + finalResponse, + &initialMsg.Context, + ) } } diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 7a869ec94..505539533 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -194,6 +194,45 @@ func newTestAgentLoop( return al, cfg, msgBus, provider, func() { os.RemoveAll(tmpDir) } } +func TestPublishResponseWithContextIfNeeded_PreservesSessionMetadata(t *testing.T) { + al, _, msgBus, _, cleanup := newTestAgentLoop(t) + defer cleanup() + + inboundCtx := &bus.InboundContext{ + Channel: "telegram", + ChatID: "-100123", + ChatType: "group", + TopicID: "6", + SenderID: "user-1", + } + al.publishResponseWithContextIfNeeded( + context.Background(), + "telegram", + "-100123", + "session-async-1", + "done", + inboundCtx, + ) + + select { + case outbound := <-msgBus.OutboundChan(): + if outbound.Channel != "telegram" || outbound.ChatID != "-100123" { + t.Fatalf("unexpected outbound target: channel=%q chat=%q", outbound.Channel, outbound.ChatID) + } + if outbound.Context.TopicID != "6" { + t.Fatalf("outbound topic_id = %q, want 6", outbound.Context.TopicID) + } + if outbound.AgentID != routing.DefaultAgentID { + t.Fatalf("outbound agent_id = %q, want %q", outbound.AgentID, routing.DefaultAgentID) + } + if outbound.SessionKey != "session-async-1" { + t.Fatalf("outbound session_key = %q, want session-async-1", outbound.SessionKey) + } + case <-time.After(2 * time.Second): + t.Fatal("expected outbound response") + } +} + func TestNewAgentLoop_RegistersWebSearchTool(t *testing.T) { cfg := config.DefaultConfig() cfg.Agents.Defaults.Workspace = t.TempDir() @@ -2124,6 +2163,29 @@ func TestToolFeedbackArgsPreview_UsesJSONAndTruncates(t *testing.T) { } } +func TestShouldPublishToolFeedback_DisablesSubagentFeedback(t *testing.T) { + subagents := false + cfg := config.DefaultConfig() + cfg.Agents.Defaults.ToolFeedback = config.ToolFeedbackConfig{ + Enabled: true, + Subagents: &subagents, + } + + if shouldPublishToolFeedback(cfg, &turnState{ + channel: "telegram", + sessionKey: "subturn-1", + }) { + t.Fatal("shouldPublishToolFeedback() = true for disabled subagent feedback, want false") + } + + if !shouldPublishToolFeedback(cfg, &turnState{ + channel: "telegram", + sessionKey: "chat-1", + }) { + t.Fatal("shouldPublishToolFeedback() = false for main turn, want true") + } +} + type picoInterleavedContentProvider struct { calls int } @@ -4043,6 +4105,7 @@ func TestProcessHeartbeat_DoesNotPublishToolFeedback(t *testing.T) { ToolFeedback: config.ToolFeedbackConfig{ Enabled: true, MaxArgsLength: 300, + Style: utils.ToolFeedbackStyleWorkingSummary, }, }, }, @@ -4089,6 +4152,7 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(t *testing.T) { ToolFeedback: config.ToolFeedbackConfig{ Enabled: true, MaxArgsLength: 300, + Style: utils.ToolFeedbackStyleWorkingSummary, }, }, }, @@ -4118,7 +4182,6 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(t *testing.T) { select { case outbound := <-msgBus.OutboundChan(): - escapedHeartbeatFile := strings.ReplaceAll(heartbeatFile, `\`, `\\`) if outbound.Channel != "telegram" { t.Fatalf("tool feedback channel = %q, want %q", outbound.Channel, "telegram") } @@ -4128,20 +4191,13 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(t *testing.T) { if outbound.Context.Channel != "telegram" || outbound.Context.ChatID != "chat-1" { t.Fatalf("unexpected tool feedback context: %+v", outbound.Context) } - if !strings.Contains(outbound.Content, "`read_file`") { + if !strings.Contains(outbound.Content, "tool: `read_file`") { t.Fatalf("tool feedback content = %q, want read_file summary", 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, "\"path\":") { - t.Fatalf("tool feedback content = %q, want serialized tool arguments", outbound.Content) - } - if !strings.Contains(outbound.Content, escapedHeartbeatFile) { - t.Fatalf("tool feedback content = %q, want tool argument value", outbound.Content) + if strings.Contains(outbound.Content, utils.ToolFeedbackContinuationHint) || + strings.Contains(outbound.Content, "check tool feedback") || + strings.Contains(outbound.Content, "\"path\":") { + t.Fatalf("tool feedback content = %q, should only include compact tool names", outbound.Content) } if strings.Contains(outbound.Content, "Previous turn explanation") { t.Fatalf("tool feedback content = %q, want no previous assistant fallback", outbound.Content) @@ -4353,6 +4409,7 @@ func TestProcessMessage_DoesNotLeakReasoningContentInToolFeedback(t *testing.T) ToolFeedback: config.ToolFeedbackConfig{ Enabled: true, MaxArgsLength: 300, + Style: utils.ToolFeedbackStyleWorkingSummary, }, }, }, @@ -4383,20 +4440,14 @@ func TestProcessMessage_DoesNotLeakReasoningContentInToolFeedback(t *testing.T) select { case outbound := <-msgBus.OutboundChan(): escapedHeartbeatFile := strings.ReplaceAll(heartbeatFile, `\`, `\\`) - if !strings.Contains(outbound.Content, "`read_file`") { + if !strings.Contains(outbound.Content, "tool: `read_file`") { t.Fatalf("tool feedback content = %q, want read_file summary", 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, "\"path\":") { - t.Fatalf("tool feedback content = %q, want serialized tool arguments", outbound.Content) - } - if !strings.Contains(outbound.Content, escapedHeartbeatFile) { - t.Fatalf("tool feedback content = %q, want tool argument value", outbound.Content) + if strings.Contains(outbound.Content, utils.ToolFeedbackContinuationHint) || + strings.Contains(outbound.Content, "check reasoning fallback") || + strings.Contains(outbound.Content, "\"path\":") || + strings.Contains(outbound.Content, escapedHeartbeatFile) { + t.Fatalf("tool feedback content = %q, should only include compact tool names", outbound.Content) } if strings.Contains(outbound.Content, "Read README.md first") { t.Fatalf("tool feedback content = %q, should not leak hidden reasoning", outbound.Content) diff --git a/pkg/agent/agent_utils.go b/pkg/agent/agent_utils.go index 9228b6d55..442a4a3fc 100644 --- a/pkg/agent/agent_utils.go +++ b/pkg/agent/agent_utils.go @@ -178,7 +178,52 @@ func shouldPublishToolFeedback(cfg *config.Config, ts *turnState) bool { if ts == nil || ts.channel == "" || ts.opts.SuppressToolFeedback { return false } - return cfg != nil && cfg.Agents.Defaults.IsToolFeedbackEnabled() + if cfg == nil || !cfg.Agents.Defaults.IsToolFeedbackEnabled() { + return false + } + if strings.HasPrefix(strings.TrimSpace(ts.sessionKey), "subturn-") && + !cfg.Agents.Defaults.IsSubagentToolFeedbackEnabled() { + return false + } + return true +} + +func toolFeedbackTitleForTurn(ts *turnState) string { + if ts == nil || !strings.HasPrefix(strings.TrimSpace(ts.sessionKey), "subturn-") { + return "" + } + if ts.agent == nil { + return "Subagent" + } + if title := displayAgentName(ts.agent.ID); title != "" { + return title + } + return "Subagent" +} + +func displayAgentName(agentID string) string { + agentID = strings.TrimSpace(agentID) + if agentID == "" { + return "" + } + parts := strings.FieldsFunc(agentID, func(r rune) bool { + return r == '-' || r == '_' || r == ' ' + }) + var b strings.Builder + for _, part := range parts { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if b.Len() > 0 { + b.WriteByte(' ') + } + b.WriteString(strings.ToUpper(part[:1])) + if len(part) > 1 { + b.WriteString(part[1:]) + } + } + return b.String() } func cloneEventArguments(args map[string]any) map[string]any { diff --git a/pkg/agent/hooks_test.go b/pkg/agent/hooks_test.go index 4deef38c7..d0e4809e8 100644 --- a/pkg/agent/hooks_test.go +++ b/pkg/agent/hooks_test.go @@ -17,6 +17,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" ) func newHookTestLoop( @@ -809,6 +810,7 @@ func TestAgentLoop_Hooks_ToolFeedbackUsesRewrittenToolName(t *testing.T) { defer cleanup() al.cfg.Agents.Defaults.ToolFeedback.Enabled = true + al.cfg.Agents.Defaults.ToolFeedback.Style = utils.ToolFeedbackStyleWorkingSummary al.RegisterTool(&echoTextTool{}) al.RegisterTool(&echoTextRewrittenTool{}) if err := al.MountHook(NamedHook("tool-rename", &toolRenameHook{})); err != nil { @@ -835,10 +837,10 @@ func TestAgentLoop_Hooks_ToolFeedbackUsesRewrittenToolName(t *testing.T) { select { case outbound := <-msgBus.OutboundChan(): - if !strings.Contains(outbound.Content, "`echo_text_rewritten`") { + if !strings.Contains(outbound.Content, "tool: `echo_text_rewritten`") { t.Fatalf("tool feedback content = %q, want rewritten tool name", outbound.Content) } - if strings.Contains(outbound.Content, "`echo_text`") { + if strings.Contains(outbound.Content, "tool: `echo_text`\n") { t.Fatalf("tool feedback content = %q, want no original tool name", outbound.Content) } case <-time.After(2 * time.Second): diff --git a/pkg/agent/interfaces/interfaces.go b/pkg/agent/interfaces/interfaces.go index 2efec05e1..829eaa75d 100644 --- a/pkg/agent/interfaces/interfaces.go +++ b/pkg/agent/interfaces/interfaces.go @@ -51,4 +51,16 @@ type ChannelManager interface { // outboundCtx carries topic/thread info needed for channels that use // scoped tracker keys (e.g., Telegram forum topics); may be nil. DismissToolFeedback(ctx context.Context, channel, chatID string, outboundCtx *bus.InboundContext) + + // DismissToolFeedbackForSession clears a session-scoped tool feedback + // message. This is used for background sub-turns whose progress messages + // are visible in the originating chat, but whose final result is delivered + // asynchronously through the parent turn instead of as a direct channel + // response. + DismissToolFeedbackForSession( + ctx context.Context, + channel, chatID string, + outboundCtx *bus.InboundContext, + sessionKey string, + ) } diff --git a/pkg/agent/pipeline_execute.go b/pkg/agent/pipeline_execute.go index 567e56d17..68d52f817 100644 --- a/pkg/agent/pipeline_execute.go +++ b/pkg/agent/pipeline_execute.go @@ -174,11 +174,21 @@ toolLoop: tc, messages, ) - feedbackMsg := utils.FormatToolFeedbackMessage( + feedbackMsg := utils.FormatToolFeedbackMessageWithStyle( + al.cfg.Agents.Defaults.GetToolFeedbackStyle(), toolName, toolFeedbackExplanation, toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), ) + if title := toolFeedbackTitleForTurn(ts); title != "" { + feedbackMsg = utils.FormatToolFeedbackMessageWithStyleAndTitle( + al.cfg.Agents.Defaults.GetToolFeedbackStyle(), + title, + toolName, + toolFeedbackExplanation, + toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), + ) + } fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) fbCancel() @@ -461,11 +471,21 @@ toolLoop: tc, messages, ) - feedbackMsg := utils.FormatToolFeedbackMessage( + feedbackMsg := utils.FormatToolFeedbackMessageWithStyle( + al.cfg.Agents.Defaults.GetToolFeedbackStyle(), toolName, toolFeedbackExplanation, toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), ) + if title := toolFeedbackTitleForTurn(ts); title != "" { + feedbackMsg = utils.FormatToolFeedbackMessageWithStyleAndTitle( + al.cfg.Agents.Defaults.GetToolFeedbackStyle(), + title, + toolName, + toolFeedbackExplanation, + toolFeedbackArgsPreview(toolArgs, toolFeedbackMaxLen), + ) + } fbCtx, fbCancel := context.WithTimeout(turnCtx, 3*time.Second) _ = al.bus.PublishOutbound(fbCtx, outboundMessageForTurnWithKind(ts, feedbackMsg, messageKindToolFeedback)) fbCancel() diff --git a/pkg/agent/subturn.go b/pkg/agent/subturn.go index 86617d02f..b136e902a 100644 --- a/pkg/agent/subturn.go +++ b/pkg/agent/subturn.go @@ -462,6 +462,17 @@ func spawnSubTurn( // Result Delivery Strategy (Async vs Sync) if cfg.Async { + if al != nil && al.channelManager != nil && childTS.channel != "" { + dismissCtx, dismissCancel := context.WithTimeout(context.Background(), 5*time.Second) + al.channelManager.DismissToolFeedbackForSession( + dismissCtx, + childTS.channel, + childTS.chatID, + childTS.opts.Dispatch.InboundContext, + childID, + ) + dismissCancel() + } deliverSubTurnResult(al, parentTS, childID, result) } diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index d345a5d0b..17dc9a763 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -191,6 +191,15 @@ func trackedToolFeedbackMessageChatID(ch Channel, chatID string, outboundCtx *bu return strings.TrimSpace(chatID) } +func candidateToolFeedbackMessageChatIDs(raw, resolved string) []string { + raw = strings.TrimSpace(raw) + resolved = strings.TrimSpace(resolved) + if raw == "" || raw == resolved { + return []string{resolved} + } + return []string{resolved, raw} +} + func dismissTrackedToolFeedbackMessage( ctx context.Context, ch Channel, @@ -210,6 +219,38 @@ func dismissTrackedToolFeedbackMessage( } } +func dismissTrackedToolFeedbackMessageForSession( + ctx context.Context, + ch Channel, + chatID string, + outboundCtx *bus.InboundContext, + sessionKey string, +) { + sessionKey = strings.TrimSpace(sessionKey) + if sessionKey == "" { + dismissTrackedToolFeedbackMessage(ctx, ch, chatID, outboundCtx) + return + } + resolvedChatID := trackedToolFeedbackMessageChatID(ch, chatID, outboundCtx) + if cleaner, ok := ch.(toolFeedbackMessageCleaner); ok { + for _, candidate := range candidateToolFeedbackMessageChatIDs(chatID, resolvedChatID) { + if candidate == "" { + continue + } + cleaner.DismissToolFeedbackMessage(ctx, candidate+"#session:"+sessionKey) + } + return + } + if tracker, ok := ch.(toolFeedbackMessageTracker); ok { + for _, candidate := range candidateToolFeedbackMessageChatIDs(chatID, resolvedChatID) { + if candidate == "" { + continue + } + tracker.ClearToolFeedbackMessage(candidate + "#session:" + sessionKey) + } + } +} + func clearTrackedToolFeedbackMessage( ch Channel, chatID string, @@ -239,6 +280,16 @@ func (m *Manager) DismissToolFeedback( dismissTrackedToolFeedbackMessage(ctx, ch, chatID, outboundCtx) } +func (m *Manager) DismissToolFeedbackForSession( + ctx context.Context, channelName, chatID string, outboundCtx *bus.InboundContext, sessionKey string, +) { + ch, ok := m.GetChannel(channelName) + if !ok { + return + } + dismissTrackedToolFeedbackMessageForSession(ctx, ch, chatID, outboundCtx, sessionKey) +} + func prepareToolFeedbackMessageContent(ch Channel, content string) string { prepared := strings.TrimSpace(content) if prepared == "" { diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index 8c2f6ecf8..2f0e57472 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -919,6 +919,7 @@ type mockMessageEditor struct { recordedContent string clearedChatID string dismissedChatID string + dismissedChatIDs []string } func (m *mockMessageEditor) EditMessage(ctx context.Context, chatID, messageID, content string) error { @@ -937,6 +938,7 @@ func (m *mockMessageEditor) ClearToolFeedbackMessage(chatID string) { func (m *mockMessageEditor) DismissToolFeedbackMessage(_ context.Context, chatID string) { m.dismissedChatID = chatID + m.dismissedChatIDs = append(m.dismissedChatIDs, chatID) } func (m *mockMessageEditor) FinalizeToolFeedbackMessage( @@ -979,6 +981,34 @@ func (m *mockResolvedToolFeedbackEditor) ToolFeedbackMessageChatID( return chatID } +func TestDismissToolFeedbackForSession_UsesResolvedTopicScopedKey(t *testing.T) { + m := newTestManager() + ch := &mockResolvedToolFeedbackEditor{ + resolveChatIDFn: func(chatID string, outboundCtx *bus.InboundContext) string { + if chatID != "-100123" { + t.Fatalf("chatID = %q, want -100123", chatID) + } + if outboundCtx == nil || outboundCtx.TopicID != "6" { + t.Fatalf("unexpected outbound context: %+v", outboundCtx) + } + return "-100123/6" + }, + } + m.channels["telegram"] = ch + + m.DismissToolFeedbackForSession( + context.Background(), + "telegram", + "-100123", + &bus.InboundContext{Channel: "telegram", ChatID: "-100123", TopicID: "6"}, + "subturn-1", + ) + + if len(ch.dismissedChatIDs) == 0 || ch.dismissedChatIDs[0] != "-100123/6#session:subturn-1" { + t.Fatalf("dismissed chatIDs = %v, want topic-scoped session key first", ch.dismissedChatIDs) + } +} + type mockPreparedToolFeedbackEditor struct { mockMessageEditor prepareFn func(content string) string diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index 0965bcedc..e3dd6b132 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -233,7 +233,7 @@ func (c *TelegramChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([] if isToolFeedback { toolFeedbackContent = fitToolFeedbackForTelegram(msg.Content, useMarkdownV2, 4096) } - trackedChatID := telegramToolFeedbackChatKey(msg.ChatID, &msg.Context) + trackedChatID := telegramToolFeedbackTrackerKey(msg) if isToolFeedback { if msgID, handled, err := c.progress.Update(ctx, trackedChatID, toolFeedbackContent); handled { if err != nil { @@ -441,6 +441,7 @@ func (c *TelegramChannel) StartTyping(ctx context.Context, chatID string) (func( // EditMessage implements channels.MessageEditor. func (c *TelegramChannel) EditMessage(ctx context.Context, chatID string, messageID string, content string) error { useMarkdownV2 := c.tgCfg.UseMarkdownV2 + chatID = telegramToolFeedbackDeliveryChatID(chatID) cid, _, err := parseTelegramChatID(chatID) if err != nil { return err @@ -497,6 +498,7 @@ func (c *TelegramChannel) EditMessage(ctx context.Context, chatID string, messag // DeleteMessage implements channels.MessageDeleter. func (c *TelegramChannel) DeleteMessage(ctx context.Context, chatID string, messageID string) error { + chatID = telegramToolFeedbackDeliveryChatID(chatID) cid, _, err := parseTelegramChatID(chatID) if err != nil { return err @@ -518,6 +520,23 @@ func outboundMessageIsToolFeedback(msg bus.OutboundMessage) bool { return strings.EqualFold(strings.TrimSpace(msg.Context.Raw["message_kind"]), "tool_feedback") } +func telegramToolFeedbackTrackerKey(msg bus.OutboundMessage) string { + key := telegramToolFeedbackChatKey(msg.ChatID, &msg.Context) + sessionKey := strings.TrimSpace(msg.SessionKey) + if key == "" || sessionKey == "" { + return key + } + return key + "#session:" + sessionKey +} + +func telegramToolFeedbackDeliveryChatID(chatID string) string { + chatID = strings.TrimSpace(chatID) + if idx := strings.Index(chatID, "#session:"); idx >= 0 { + return strings.TrimSpace(chatID[:idx]) + } + return chatID +} + func (c *TelegramChannel) currentToolFeedbackMessage(chatID string) (string, bool) { if c.progress == nil { return "", false @@ -559,7 +578,7 @@ func (c *TelegramChannel) dismissTrackedToolFeedbackMessage(ctx context.Context, return } c.ClearToolFeedbackMessage(chatID) - _ = c.DeleteMessage(ctx, chatID, messageID) + _ = c.DeleteMessage(ctx, telegramToolFeedbackDeliveryChatID(chatID), messageID) } func (c *TelegramChannel) finalizeTrackedToolFeedbackMessage( @@ -572,7 +591,7 @@ func (c *TelegramChannel) finalizeTrackedToolFeedbackMessage( if !ok || editFn == nil { return nil, false } - if err := editFn(ctx, chatID, msgID, content); err != nil { + if err := editFn(ctx, telegramToolFeedbackDeliveryChatID(chatID), msgID, content); err != nil { c.RecordToolFeedbackMessage(chatID, msgID, baseContent) return nil, false } diff --git a/pkg/channels/telegram/telegram_test.go b/pkg/channels/telegram/telegram_test.go index 14d025064..0d1d94c3a 100644 --- a/pkg/channels/telegram/telegram_test.go +++ b/pkg/channels/telegram/telegram_test.go @@ -375,6 +375,115 @@ func TestSend_TopicReplyDoesNotFinalizeDifferentTopicToolFeedback(t *testing.T) assert.True(t, ok, "tool feedback in the original topic should remain tracked") } +func TestSend_FinalReplyDoesNotFinalizeDifferentSessionToolFeedback(t *testing.T) { + nextMessageID := 0 + caller := &stubCaller{ + callFn: func(ctx context.Context, url string, data *ta.RequestData) (*ta.Response, error) { + nextMessageID++ + return successResponseWithMessageID(t, nextMessageID), nil + }, + } + ch := newTestChannel(t, caller) + ch.progress = channels.NewToolFeedbackAnimator(ch.EditMessage) + + baseCtx := bus.InboundContext{ + Channel: "telegram", + ChatID: "-1001234567890", + TopicID: "42", + } + _, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "-1001234567890", + SessionKey: "subturn-1", + Content: "Working...\n• tool: `read_file`", + Context: bus.InboundContext{ + Channel: "telegram", + ChatID: "-1001234567890", + TopicID: "42", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + }, + }) + require.NoError(t, err) + + ids, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "-1001234567890", + SessionKey: "main-session", + Content: "test", + Context: baseCtx, + }) + require.NoError(t, err) + require.Len(t, caller.calls, 2) + assert.Equal(t, []string{"2"}, ids) + assert.Contains(t, caller.calls[1].URL, "sendMessage") + assert.NotContains(t, caller.calls[1].URL, "editMessageText") + + msgID, ok := ch.currentToolFeedbackMessage("-1001234567890/42#session:subturn-1") + require.True(t, ok, "subturn tool feedback should remain tracked") + assert.Equal(t, "1", msgID) +} + +func TestSend_SessionScopedToolFeedbackUpdatesExistingTelegramMessage(t *testing.T) { + nextMessageID := 0 + caller := &stubCaller{ + callFn: func(ctx context.Context, url string, data *ta.RequestData) (*ta.Response, error) { + nextMessageID++ + return successResponseWithMessageID(t, nextMessageID), nil + }, + } + ch := newTestChannel(t, caller) + ch.progress = channels.NewToolFeedbackAnimator(ch.EditMessage) + + baseCtx := bus.InboundContext{ + Channel: "telegram", + ChatID: "-1001234567890", + TopicID: "42", + Raw: map[string]string{ + "message_kind": "tool_feedback", + }, + } + + _, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "-1001234567890", + SessionKey: "subturn-1", + Content: "Working...\n• tool: `read_file`", + Context: baseCtx, + }) + require.NoError(t, err) + + ids, err := ch.Send(context.Background(), bus.OutboundMessage{ + ChatID: "-1001234567890", + SessionKey: "subturn-1", + Content: "Working...\n• tool: `read_file`\n• tool: `mcp_gpt_researcher_deep_research`", + Context: baseCtx, + }) + require.NoError(t, err) + require.Equal(t, []string{"1"}, ids) + require.Len(t, caller.calls, 2) + assert.Contains(t, caller.calls[1].URL, "editMessageText") + assert.NotContains(t, caller.calls[1].URL, "%23session") + assert.NotContains(t, caller.calls[1].URL, "#session") +} + +func TestDismissToolFeedbackMessage_SessionScopedKeyDeletesTelegramMessage(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) + ch.RecordToolFeedbackMessage("-1001234567890/42#session:subturn-1", "7", "Working...\n• tool: `read_file`") + + ch.DismissToolFeedbackMessage(context.Background(), "-1001234567890/42#session:subturn-1") + + require.Len(t, caller.calls, 1) + assert.Contains(t, caller.calls[0].URL, "deleteMessage") + assert.NotContains(t, caller.calls[0].URL, "%23session") + assert.NotContains(t, caller.calls[0].URL, "#session") + _, ok := ch.currentToolFeedbackMessage("-1001234567890/42#session:subturn-1") + assert.False(t, ok) +} + func TestFinalizeTrackedToolFeedbackMessage_StopsTrackingBeforeEdit(t *testing.T) { ch := newTestChannel(t, &stubCaller{ callFn: func(context.Context, string, *ta.RequestData) (*ta.Response, error) { @@ -442,6 +551,20 @@ func TestFitToolFeedbackForTelegram_ReservesAnimationFrame(t *testing.T) { } } +func TestParseContent_WorkingSummaryToolNamesStayCode(t *testing.T) { + content := "Working...\n• tool: `inventorydb__get_location`" + + htmlContent := parseContent(content, false) + if !strings.Contains(htmlContent, "inventorydb__get_location") { + t.Fatalf("parseContent() HTML = %q, want tool name code span", htmlContent) + } + + markdownV2Content := parseContent(content, true) + if !strings.Contains(markdownV2Content, "`inventorydb__get_location`") { + t.Fatalf("parseContent() MarkdownV2 = %q, want tool name code span", markdownV2Content) + } +} + 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 b424612bf..b9d772203 100644 --- a/pkg/channels/tool_feedback_animator.go +++ b/pkg/channels/tool_feedback_animator.go @@ -8,6 +8,8 @@ import ( ) const toolFeedbackAnimationInterval = 3 * time.Second +const workingSummaryToolFeedbackAnimationInterval = 10 * time.Second +const maxMergedToolFeedbackLines = 8 const initialToolFeedbackAnimationFrame = "" @@ -122,13 +124,17 @@ func (a *ToolFeedbackAnimator) Update(ctx context.Context, chatID, content strin return "", false, nil } - animatedContent := InitialAnimatedToolFeedbackContent(content) + mergedContent := content + if isWorkingSummaryToolFeedback(baseContent) || isWorkingSummaryToolFeedback(content) { + mergedContent = mergeToolFeedbackContent(baseContent, content) + } + animatedContent := InitialAnimatedToolFeedbackContent(mergedContent) if err := a.editFn(ctx, strings.TrimSpace(chatID), msgID, animatedContent); err != nil { a.Record(chatID, msgID, baseContent) return "", true, err } - a.Record(chatID, msgID, content) + a.Record(chatID, msgID, mergedContent) return msgID, true, nil } @@ -163,7 +169,7 @@ func (a *ToolFeedbackAnimator) detach(chatID string) *toolFeedbackAnimationState func (a *ToolFeedbackAnimator) run(chatID string, entry *toolFeedbackAnimationState) { defer close(entry.done) - ticker := time.NewTicker(toolFeedbackAnimationInterval) + ticker := time.NewTicker(toolFeedbackAnimationIntervalFor(entry.baseContent)) defer ticker.Stop() frameIdx := 1 @@ -227,6 +233,77 @@ func appendToolFeedbackFrame(firstLine, frame string) string { return firstLine + frame } +func mergeToolFeedbackContent(previous, next string) string { + previous = strings.TrimSpace(previous) + next = strings.TrimSpace(next) + if previous == "" { + return next + } + if next == "" { + return previous + } + + lines := make([]string, 0, maxMergedToolFeedbackLines+1) + seen := make(map[string]struct{}) + addLine := func(line string) { + line = strings.TrimSpace(line) + if line == "" || isWorkingSummaryHeader(line) { + return + } + if _, ok := seen[line]; ok { + return + } + seen[line] = struct{}{} + lines = append(lines, line) + } + + for _, line := range strings.Split(previous, "\n") { + addLine(line) + } + for _, line := range strings.Split(next, "\n") { + addLine(line) + } + if len(lines) > maxMergedToolFeedbackLines { + lines = lines[len(lines)-maxMergedToolFeedbackLines:] + } + header := workingSummaryHeader(previous) + if candidate := workingSummaryHeader(next); candidate != "" { + header = candidate + } + if header == "" { + header = "Working..." + } + if len(lines) == 0 { + return header + } + return header + "\n" + strings.Join(lines, "\n") +} + +func isWorkingSummaryToolFeedback(content string) bool { + return isWorkingSummaryHeader(workingSummaryHeader(content)) +} + +func workingSummaryHeader(content string) string { + firstLine, _, _ := strings.Cut(strings.TrimSpace(content), "\n") + firstLine = strings.TrimSpace(firstLine) + if isWorkingSummaryHeader(firstLine) { + return firstLine + } + return "" +} + +func isWorkingSummaryHeader(line string) bool { + line = strings.TrimSpace(line) + return strings.EqualFold(line, "Working...") || strings.HasSuffix(strings.ToLower(line), " working...") +} + +func toolFeedbackAnimationIntervalFor(content string) time.Duration { + if isWorkingSummaryToolFeedback(content) { + return workingSummaryToolFeedbackAnimationInterval + } + return toolFeedbackAnimationInterval +} + func stopToolFeedbackAnimation(entry *toolFeedbackAnimationState) { if entry == nil { return diff --git a/pkg/channels/tool_feedback_animator_test.go b/pkg/channels/tool_feedback_animator_test.go index a23284548..52adc6f13 100644 --- a/pkg/channels/tool_feedback_animator_test.go +++ b/pkg/channels/tool_feedback_animator_test.go @@ -75,16 +75,17 @@ func TestToolFeedbackAnimator_UpdateStopsTrackingBeforeEdit(t *testing.T) { if messageID != "msg-1" { t.Fatalf("messageID = %q, want msg-1", messageID) } - if content != "🔧 `write_file`\nUpdating config" { + want := "Working...\n• tool: `read_file`\n• tool: `write_file`" + if content != want { t.Fatalf("content = %q, want updated animated content", content) } return nil }) defer animator.StopAll() - animator.Record("chat-1", "msg-1", "🔧 `read_file`\nChecking config") + animator.Record("chat-1", "msg-1", "Working...\n• tool: `read_file`") - msgID, handled, err := animator.Update(context.Background(), "chat-1", "🔧 `write_file`\nUpdating config") + msgID, handled, err := animator.Update(context.Background(), "chat-1", "Working...\n• tool: `write_file`") if err != nil { t.Fatalf("Update() error = %v", err) } @@ -96,6 +97,51 @@ func TestToolFeedbackAnimator_UpdateStopsTrackingBeforeEdit(t *testing.T) { } } +func TestToolFeedbackAnimator_UpdateRawFeedbackReplacesContent(t *testing.T) { + var animator *ToolFeedbackAnimator + animator = NewToolFeedbackAnimator(func(_ context.Context, chatID, messageID, content string) error { + if _, ok := animator.Current(chatID); ok { + t.Fatal("expected tracked tool feedback to be stopped before edit") + } + if messageID != "msg-1" { + t.Fatalf("messageID = %q, want msg-1", messageID) + } + want := "🔧 `write_file`\nWriting config" + if content != want { + t.Fatalf("content = %q, want replacement content", content) + } + return nil + }) + defer animator.StopAll() + + animator.Record("chat-1", "msg-1", "🔧 `read_file`\nReading config") + + msgID, handled, err := animator.Update(context.Background(), "chat-1", "🔧 `write_file`\nWriting config") + if err != nil { + t.Fatalf("Update() error = %v", err) + } + if !handled { + t.Fatal("Update() handled = false, want true") + } + if msgID != "msg-1" { + t.Fatalf("Update() msgID = %q, want msg-1", msgID) + } +} + +func TestToolFeedbackAnimationIntervalForWorkingSummary(t *testing.T) { + got := toolFeedbackAnimationIntervalFor("Working...\n• tool: `read_file`") + if got != workingSummaryToolFeedbackAnimationInterval { + t.Fatalf("toolFeedbackAnimationIntervalFor() = %v, want %v", got, workingSummaryToolFeedbackAnimationInterval) + } +} + +func TestToolFeedbackAnimationIntervalForRawFeedback(t *testing.T) { + got := toolFeedbackAnimationIntervalFor("🔧 `read_file`\nReading config") + if got != toolFeedbackAnimationInterval { + t.Fatalf("toolFeedbackAnimationIntervalFor() = %v, want %v", got, toolFeedbackAnimationInterval) + } +} + func TestToolFeedbackAnimator_UpdateFailureRestoresTracking(t *testing.T) { editErr := errors.New("edit failed") animator := NewToolFeedbackAnimator(func(context.Context, string, string, string) error { @@ -103,9 +149,9 @@ func TestToolFeedbackAnimator_UpdateFailureRestoresTracking(t *testing.T) { }) defer animator.StopAll() - animator.Record("chat-1", "msg-1", "🔧 `read_file`\nChecking config") + animator.Record("chat-1", "msg-1", "Working...\n• tool: `read_file`") - msgID, handled, err := animator.Update(context.Background(), "chat-1", "🔧 `write_file`\nUpdating config") + msgID, handled, err := animator.Update(context.Background(), "chat-1", "Working...\n• tool: `write_file`") if !handled { t.Fatal("Update() handled = false, want true") } @@ -119,3 +165,20 @@ func TestToolFeedbackAnimator_UpdateFailureRestoresTracking(t *testing.T) { t.Fatalf("Current() after failed Update = (%q, %v), want (msg-1, true)", currentID, ok) } } + +func TestMergeToolFeedbackContent_PreservesNamedWorkingSummaryHeader(t *testing.T) { + got := mergeToolFeedbackContent( + "Deep Research working...\n• tool: `read_file`", + "Deep Research working...\n• tool: `web_fetch`", + ) + want := "Deep Research working...\n• tool: `read_file`\n• tool: `web_fetch`" + if got != want { + t.Fatalf("mergeToolFeedbackContent() = %q, want %q", got, want) + } +} + +func TestIsWorkingSummaryToolFeedback_AcceptsNamedHeader(t *testing.T) { + if !isWorkingSummaryToolFeedback("Deep Research working...\n• tool: `read_file`") { + t.Fatal("expected named working summary to be recognized") + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 53eaef059..3910df770 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -370,9 +370,11 @@ 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"` - SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` + 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"` + Subagents *bool `json:"subagents,omitempty"` + Style string `json:"style,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_STYLE"` } type AgentDefaults struct { @@ -425,6 +427,13 @@ func (d *AgentDefaults) IsToolFeedbackEnabled() bool { return d.ToolFeedback.Enabled } +// IsSubagentToolFeedbackEnabled returns true when subagent turns should publish +// visible tool feedback. It defaults to true for backward compatibility when +// tool_feedback itself is enabled. +func (d *AgentDefaults) IsSubagentToolFeedbackEnabled() bool { + return d.ToolFeedback.Subagents == nil || *d.ToolFeedback.Subagents +} + // 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. @@ -432,6 +441,10 @@ func (d *AgentDefaults) IsToolFeedbackSeparateMessagesEnabled() bool { return d.ToolFeedback.SeparateMessages } +func (d *AgentDefaults) GetToolFeedbackStyle() string { + return strings.TrimSpace(d.ToolFeedback.Style) +} + // 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 d744e15dc..42fc9e847 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1098,9 +1098,15 @@ 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.IsSubagentToolFeedbackEnabled() { + t.Fatal("DefaultConfig().Agents.Defaults.IsSubagentToolFeedbackEnabled() should default to true") + } if cfg.Agents.Defaults.ToolFeedback.SeparateMessages { t.Fatal("DefaultConfig().Agents.Defaults.ToolFeedback.SeparateMessages should be false") } + if got := cfg.Agents.Defaults.GetToolFeedbackStyle(); got != "" { + t.Fatalf("DefaultConfig().Agents.Defaults.GetToolFeedbackStyle() = %q, want empty/raw default", got) + } } func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) { @@ -1123,9 +1129,55 @@ func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) { "agents.defaults.tool_feedback.enabled should remain false when unset in config file", ) } + if !cfg.Agents.Defaults.IsSubagentToolFeedbackEnabled() { + t.Fatal("agents.defaults.tool_feedback.subagents should default to true when unset") + } if cfg.Agents.Defaults.ToolFeedback.SeparateMessages { t.Fatal("agents.defaults.tool_feedback.separate_messages should remain false when unset in config file") } + if got := cfg.Agents.Defaults.GetToolFeedbackStyle(); got != "" { + t.Fatalf("agents.defaults.tool_feedback.style = %q, want empty/raw default when unset", got) + } +} + +func TestLoadConfig_ToolFeedbackStyle(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile( + configPath, + []byte(`{"version":1,"agents":{"defaults":{"tool_feedback":{"enabled":true,"style":"working_summary"}}}}`), + 0o600, + ); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + if got := cfg.Agents.Defaults.GetToolFeedbackStyle(); got != "working_summary" { + t.Fatalf("agents.defaults.tool_feedback.style = %q, want working_summary", got) + } +} + +func TestLoadConfig_ToolFeedbackSubagentsFalse(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile( + configPath, + []byte(`{"version":1,"agents":{"defaults":{"tool_feedback":{"enabled":true,"subagents":false}}}}`), + 0o600, + ); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + if cfg.Agents.Defaults.IsSubagentToolFeedbackEnabled() { + t.Fatal("agents.defaults.tool_feedback.subagents = true, want false") + } } func TestLoadConfig_WebPreferNativeDefaultsTrueWhenUnset(t *testing.T) { diff --git a/pkg/utils/tool_feedback.go b/pkg/utils/tool_feedback.go index 1834d7f78..c2b5f2b2c 100644 --- a/pkg/utils/tool_feedback.go +++ b/pkg/utils/tool_feedback.go @@ -4,6 +4,8 @@ import ( "bytes" "encoding/json" "fmt" + "path/filepath" + "regexp" "strings" ) @@ -30,6 +32,8 @@ func FormatArgsJSON(args map[string]any, prettyPrint, disableEscapeHTML bool) st return strings.TrimSpace(buf.String()) } +const ToolFeedbackStyleWorkingSummary = "working_summary" + // FormatToolFeedbackMessage renders a tool feedback message for chat channels. // It keeps the tool name on the first line for animation and can include both // a human explanation and the serialized tool arguments in the body. @@ -57,6 +61,187 @@ func FormatToolFeedbackMessage(toolName, explanation, argsPreview string) string return fmt.Sprintf("\U0001f527 `%s`\n%s", toolName, body) } +// FormatToolFeedbackMessageWithStyle renders alternate tool feedback styles. +// The working_summary style intentionally omits explanations and raw arguments: +// progress UI should not leak internal paths, large JSON blobs, secrets, or +// model-drafted reasoning-like text. +func FormatToolFeedbackMessageWithStyle(style, toolName, explanation, argsPreview string) string { + return FormatToolFeedbackMessageWithStyleAndTitle(style, "", toolName, explanation, argsPreview) +} + +func FormatToolFeedbackMessageWithStyleAndTitle( + style, title, toolName, explanation, argsPreview string, +) string { + if strings.EqualFold(strings.TrimSpace(style), ToolFeedbackStyleWorkingSummary) { + return FormatWorkingSummaryToolFeedbackMessageWithTitle(title, toolName, argsPreview) + } + return FormatToolFeedbackMessage(toolName, explanation, argsPreview) +} + +func FormatWorkingSummaryToolFeedbackMessage(toolName, argsPreview string) string { + return FormatWorkingSummaryToolFeedbackMessageWithTitle("", toolName, argsPreview) +} + +func FormatWorkingSummaryToolFeedbackMessageWithTitle(title, toolName, argsPreview string) string { + toolName = strings.TrimSpace(toolName) + header := workingSummaryHeader(title) + if toolName == "" { + return header + } + line := fmt.Sprintf("• tool: `%s`", sanitizeToolFeedbackCodeSpan(toolName)) + if summary := summarizeToolFeedbackArgs(toolName, argsPreview); summary != "" { + line += fmt.Sprintf(" — `%s`", sanitizeToolFeedbackCodeSpan(summary)) + } + return header + "\n" + line +} + +func workingSummaryHeader(title string) string { + title = strings.TrimSpace(title) + if title == "" { + return "Working..." + } + return title + " working..." +} + +func sanitizeToolFeedbackCodeSpan(text string) string { + return strings.ReplaceAll(text, "`", "'") +} + +func summarizeToolFeedbackArgs(toolName, argsPreview string) string { + argsPreview = strings.TrimSpace(argsPreview) + if argsPreview == "" { + return "" + } + + var args map[string]any + if err := json.Unmarshal([]byte(argsPreview), &args); err != nil { + return "" + } + + normalizedToolName := strings.ToLower(strings.TrimSpace(toolName)) + if strings.Contains(normalizedToolName, "exec") { + if command := firstStringArg(args, "command"); command != "" { + return truncateToolFeedbackSummary(summarizeExecCommand(command)) + } + return "" + } + if isFileToolFeedbackTool(normalizedToolName) { + if summary := firstStringArg(args, "path", "file_path", "filepath"); summary != "" { + return truncateToolFeedbackSummary(summarizeFilePath(summary)) + } + } + return "" +} + +func isFileToolFeedbackTool(toolName string) bool { + return strings.Contains(toolName, "read_file") || + strings.Contains(toolName, "write_file") || + strings.Contains(toolName, "edit_file") || + strings.Contains(toolName, "append_file") || + strings.Contains(toolName, "list_dir") +} + +func firstStringArg(args map[string]any, keys ...string) string { + for _, key := range keys { + if value, ok := args[key]; ok { + if s, ok := value.(string); ok { + if normalized := normalizeToolFeedbackSummary(s); normalized != "" { + return normalized + } + } + } + } + return "" +} + +func normalizeToolFeedbackSummary(text string) string { + return redactToolFeedbackSecrets(strings.Join(strings.Fields(text), " ")) +} + +func summarizeFilePath(path string) string { + path = strings.TrimSpace(path) + if path == "" { + return "" + } + + cleaned := filepath.Clean(path) + slashed := filepath.ToSlash(cleaned) + if idx := strings.LastIndex(slashed, "/workspace/"); idx >= 0 { + relative := strings.TrimPrefix(slashed[idx+len("/workspace/"):], "/") + if relative != "" && relative != "." { + return relative + } + } + + if !filepath.IsAbs(cleaned) { + return slashed + } + return filepath.Base(cleaned) +} + +func truncateToolFeedbackSummary(text string) string { + const maxRunes = 96 + runes := []rune(text) + if len(runes) <= maxRunes { + return text + } + return strings.TrimSpace(string(runes[:maxRunes-3])) + "..." +} + +func summarizeExecCommand(command string) string { + fields := strings.Fields(command) + for i := 0; i < len(fields); i++ { + token := strings.Trim(fields[i], `"'`) + if token == "" || isShellAssignment(token) { + continue + } + switch token { + case "env", "command", "time", "timeout", "sudo": + continue + case "bash", "sh", "zsh", "fish", "python", "python3", "node", "deno", "bun", "uv", "uvx", "npx": + for j := i + 1; j < len(fields); j++ { + next := strings.Trim(fields[j], `"'`) + if next == "" || strings.HasPrefix(next, "-") || isShellAssignment(next) { + continue + } + return filepath.Base(next) + } + return token + default: + return filepath.Base(token) + } + } + return "" +} + +func isShellAssignment(token string) bool { + return strings.Contains(token, "=") && !strings.HasPrefix(token, "/") && !strings.HasPrefix(token, ".") +} + +var ( + toolFeedbackSecretValuePatterns = []*regexp.Regexp{ + regexp.MustCompile(`(?i)\bsk-(?:proj|or-v1)?-[A-Za-z0-9_-]{16,}`), + regexp.MustCompile(`\bAIza[A-Za-z0-9_-]{20,}`), + regexp.MustCompile(`\bmat_[A-Za-z0-9_-]{16,}`), + regexp.MustCompile(`\b\d{6,}:[A-Za-z0-9_-]{20,}`), + regexp.MustCompile(`\b(?:ghp|github_pat|glpat|xox[baprs])_[A-Za-z0-9_-]{16,}`), + } + toolFeedbackSecretKVPattern = regexp.MustCompile(`(?i)(\b(?:api[_-]?key|access[_-]?token|auth[_-]?token|token|secret|password|authorization)\b\s*[=:]\s*)("[^"]*"|'[^']*'|[^\s&]+)`) + toolFeedbackSecretFlagPattern = regexp.MustCompile(`(?i)(--(?:api-key|access-token|auth-token|token|secret|password|authorization)(?:=|\s+))("[^"]*"|'[^']*'|[^\s&]+)`) +) + +func redactToolFeedbackSecrets(text string) string { + if text == "" { + return "" + } + for _, pattern := range toolFeedbackSecretValuePatterns { + text = pattern.ReplaceAllString(text, "[redacted]") + } + text = toolFeedbackSecretKVPattern.ReplaceAllString(text, "${1}[redacted]") + text = toolFeedbackSecretFlagPattern.ReplaceAllString(text, "${1}[redacted]") + return text +} + // 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. diff --git a/pkg/utils/tool_feedback_test.go b/pkg/utils/tool_feedback_test.go index da4accce4..96b36c148 100644 --- a/pkg/utils/tool_feedback_test.go +++ b/pkg/utils/tool_feedback_test.go @@ -11,7 +11,7 @@ func TestFormatToolFeedbackMessage(t *testing.T) { "I will read README.md first to confirm the current project structure.", "{\n \"path\": \"README.md\"\n}", ) - want := "\U0001f527 `read_file`\nI will read README.md first to confirm the current project structure.\n```json\n{\n \"path\": \"README.md\"\n}\n```" + want := "🔧 `read_file`\nI will read README.md first to confirm the current project structure.\n```json\n{\n \"path\": \"README.md\"\n}\n```" if got != want { t.Fatalf("FormatToolFeedbackMessage() = %q, want %q", got, want) } @@ -19,7 +19,7 @@ func TestFormatToolFeedbackMessage(t *testing.T) { func TestFormatToolFeedbackMessage_EmptyExplanationShowsArgs(t *testing.T) { got := FormatToolFeedbackMessage("read_file", "", "{\n \"path\": \"README.md\"\n}") - want := "\U0001f527 `read_file`\n```json\n{\n \"path\": \"README.md\"\n}\n```" + want := "🔧 `read_file`\n```json\n{\n \"path\": \"README.md\"\n}\n```" if got != want { t.Fatalf("FormatToolFeedbackMessage() = %q, want %q", got, want) } @@ -35,12 +35,112 @@ func TestFormatToolFeedbackMessage_EmptyToolNameOmitsToolLine(t *testing.T) { func TestFormatToolFeedbackMessage_EmptyExplanationAndArgsKeepsOnlyToolLine(t *testing.T) { got := FormatToolFeedbackMessage("read_file", "", "") - want := "\U0001f527 `read_file`" + want := "🔧 `read_file`" if got != want { t.Fatalf("FormatToolFeedbackMessage() = %q, want %q", got, want) } } +func TestFormatToolFeedbackMessageWithStyle_WorkingSummary(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "read_file", + "I will read README.md first to confirm the current project structure.", + "{\n \"path\": \"README.md\"\n}", + ) + want := "Working...\n• tool: `read_file` — `README.md`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyleAndTitle_WorkingSummary(t *testing.T) { + got := FormatToolFeedbackMessageWithStyleAndTitle( + "working_summary", + "DeepResearch", + "read_file", + "", + "{\"path\":\"README.md\"}", + ) + want := "DeepResearch working...\n• tool: `read_file` — `README.md`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyleAndTitle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummaryShowsFileBasenameOnly(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "write_file", + "", + "{\"path\":\"/home/user/private/config.json\"}", + ) + want := "Working...\n• tool: `write_file` — `config.json`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummaryShowsWorkspaceRelativeFile(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "read_file", + "", + "{\"path\":\"/home/server/.picoclaw/spouse/workspace/memory/MEMORY.md\"}", + ) + want := "Working...\n• tool: `read_file` — `memory/MEMORY.md`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummaryShowsExecCommand(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "exec", + "", + "{\n \"action\": \"run\",\n \"command\": \"scripts/gog_me forms add-question FORM --title Name --type paragraph\",\n \"timeout\": 120\n}", + ) + want := "Working...\n• tool: `exec` — `gog_me`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummaryShowsExecScriptNameOnly(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "exec", + "", + "{\"command\":\"OPENAI_API_KEY=sk-proj-abcdefghijklmnopqrstuvwxyz0123456789 bash -lc /home/server/.picoclaw/main/workspace/tmp_add_questions_anya_form_api.sh --api-key sk-or-v1-abcdefghijklmnopqrstuvwxyz0123456789\"}", + ) + want := "Working...\n• tool: `exec` — `tmp_add_questions_anya_form_api.sh`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummarySanitizesCodeSpan(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle("working_summary", "read_file", "", "{\"path\":\"bad`path\"}") + want := "Working...\n• tool: `read_file` — `bad'path`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %q, want %q", got, want) + } +} + +func TestFormatToolFeedbackMessageWithStyle_WorkingSummaryOmitsNonFileToolArgs(t *testing.T) { + got := FormatToolFeedbackMessageWithStyle( + "working_summary", + "web_fetch", + "", + `{"url":"https://example.test/?token=mat_abcdefghijklmnopqrstuvwxyz0123456789"}`, + ) + want := "Working...\n• tool: `web_fetch`" + if got != want { + t.Fatalf("FormatToolFeedbackMessageWithStyle() = %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.",