diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 3e9bd845e..2c456dca7 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -111,8 +111,10 @@ const ( sessionKeyAgentPrefix = "agent:" pendingTurnPrefix = "pending-" metadataKeyMessageKind = "message_kind" + metadataKeyToolCalls = "tool_calls" messageKindThought = "thought" messageKindToolFeedback = "tool_feedback" + messageKindToolCalls = "tool_calls" metadataKeyAccountID = "account_id" metadataKeyGuildID = "guild_id" metadataKeyTeamID = "team_id" diff --git a/pkg/agent/agent_outbound.go b/pkg/agent/agent_outbound.go index 7e36e4ad8..1728f6f79 100644 --- a/pkg/agent/agent_outbound.go +++ b/pkg/agent/agent_outbound.go @@ -4,13 +4,17 @@ package agent import ( "context" + "encoding/json" "errors" "fmt" + "strings" "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/providers" "github.com/sipeed/picoclaw/pkg/tools" + "github.com/sipeed/picoclaw/pkg/utils" ) func (al *AgentLoop) maybePublishError(ctx context.Context, channel, chatID, sessionKey string, err error) bool { @@ -123,6 +127,95 @@ func (al *AgentLoop) publishPicoReasoning(ctx context.Context, reasoningContent, } } +func (al *AgentLoop) publishPicoToolCallInterim( + ctx context.Context, + ts *turnState, + reasoningContent string, + content string, + toolCalls []providers.ToolCall, +) { + if ts == nil || ts.chatID == "" || al == nil || al.bus == nil { + return + } + + if strings.TrimSpace(reasoningContent) != "" { + pubCtx, pubCancel := context.WithTimeout(ctx, 3*time.Second) + err := al.bus.PublishOutbound( + pubCtx, + outboundMessageForTurnWithKind(ts, reasoningContent, messageKindThought), + ) + pubCancel() + if err != nil && !errors.Is(err, context.DeadlineExceeded) && + !errors.Is(err, context.Canceled) && + !errors.Is(err, bus.ErrBusClosed) { + logger.WarnCF("agent", "Failed to publish pico reasoning", map[string]any{ + "channel": ts.channel, + "chat_id": ts.chatID, + "error": err.Error(), + }) + } + } + + if !ts.opts.AllowInterimPicoPublish { + return + } + + visibleToolCalls := utils.BuildVisibleToolCalls( + toolCalls, + al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), + ) + duplicateToolCallContent := len(visibleToolCalls) > 0 && + utils.ToolCallExplanationDuplicatesContent(content, toolCalls) + + if strings.TrimSpace(content) != "" && !duplicateToolCallContent { + pubCtx, pubCancel := context.WithTimeout(ctx, 3*time.Second) + err := al.bus.PublishOutbound(pubCtx, outboundMessageForTurn(ts, content)) + pubCancel() + if err != nil && !errors.Is(err, context.DeadlineExceeded) && + !errors.Is(err, context.Canceled) && + !errors.Is(err, bus.ErrBusClosed) { + logger.WarnCF("agent", "Failed to publish pico interim assistant content", map[string]any{ + "channel": ts.channel, + "chat_id": ts.chatID, + "error": err.Error(), + }) + } + } + + if len(visibleToolCalls) == 0 { + return + } + + rawToolCalls, err := json.Marshal(visibleToolCalls) + if err != nil { + logger.WarnCF("agent", "Failed to serialize pico tool calls", map[string]any{ + "channel": ts.channel, + "chat_id": ts.chatID, + "error": err.Error(), + }) + return + } + + msg := outboundMessageForTurnWithKind(ts, "", messageKindToolCalls) + if msg.Context.Raw == nil { + msg.Context.Raw = map[string]string{} + } + msg.Context.Raw[metadataKeyToolCalls] = string(rawToolCalls) + + pubCtx, pubCancel := context.WithTimeout(ctx, 3*time.Second) + err = al.bus.PublishOutbound(pubCtx, msg) + pubCancel() + if err != nil && !errors.Is(err, context.DeadlineExceeded) && + !errors.Is(err, context.Canceled) && + !errors.Is(err, bus.ErrBusClosed) { + logger.WarnCF("agent", "Failed to publish pico tool calls", map[string]any{ + "channel": ts.channel, + "chat_id": ts.chatID, + "error": err.Error(), + }) + } +} + func (al *AgentLoop) handleReasoning( ctx context.Context, reasoningContent, channelName, channelID string, diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 01657d43a..4047ab74d 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -1892,7 +1892,7 @@ func TestToolFeedbackExplanationFromResponse_UsesCurrentContentFirst(t *testing. {Role: "tool", Content: "tool output", ToolCallID: "call_1"}, } - got := toolFeedbackExplanationFromResponse(response, messages, 300) + got := toolFeedbackExplanationFromResponse(response, messages) if got != "Read README.md first" { t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want current content", got) } @@ -1936,7 +1936,7 @@ func TestToolFeedbackExplanationFromResponse_UsesExplicitToolCallExtraContent(t {Role: "tool", Content: "tool output", ToolCallID: "call_1"}, } - got := toolFeedbackExplanationFromResponse(response, messages, 300) + got := toolFeedbackExplanationFromResponse(response, messages) if got != "Read README.md first to confirm the current project structure." { t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want explicit tool feedback explanation", got) } @@ -1963,8 +1963,8 @@ func TestToolFeedbackExplanationForToolCall_PrefersToolSpecificExtraContent(t *t }, } - got1 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], nil, 300) - got2 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[1], nil, 300) + got1 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], nil) + got2 := toolFeedbackExplanationForToolCall(response, response.ToolCalls[1], nil) if got1 != "Read README.md first." { t.Fatalf("toolFeedbackExplanationForToolCall() first = %q, want tool-specific explanation", got1) } @@ -1993,7 +1993,7 @@ func TestToolFeedbackExplanationForToolCall_DoesNotReuseAnotherToolCallExplanati {Role: "user", Content: "inspect the config and update the example"}, } - got := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], messages, 300) + got := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], messages) want := utils.ToolFeedbackContinuationHint + ": inspect the config and update the example" if got != want { t.Fatalf("toolFeedbackExplanationForToolCall() = %q, want %q", got, want) @@ -2012,13 +2012,31 @@ func TestToolFeedbackExplanationFromResponse_DoesNotUseReasoningContent(t *testi {Role: "tool", Content: "tool output", ToolCallID: "call_1"}, } - got := toolFeedbackExplanationFromResponse(response, messages, 300) + got := toolFeedbackExplanationFromResponse(response, messages) want := utils.ToolFeedbackContinuationHint + ": Inspect README.md and update the config example." if got != want { t.Fatalf("toolFeedbackExplanationFromResponse() = %q, want latest user content fallback", got) } } +func TestToolFeedbackExplanationForToolCall_DoesNotTruncateLongExplanation(t *testing.T) { + explanation := "Read README.md first to confirm the current project structure before editing the config example." + response := &providers.LLMResponse{ + ToolCalls: []providers.ToolCall{{ + ID: "call_1", + Name: "read_file", + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: explanation, + }, + }}, + } + + got := toolFeedbackExplanationForToolCall(response, response.ToolCalls[0], nil) + if got != explanation { + t.Fatalf("toolFeedbackExplanationForToolCall() = %q, want full explanation", got) + } +} + func TestToolFeedbackArgsPreview_UsesJSONAndTruncates(t *testing.T) { got := toolFeedbackArgsPreview(map[string]any{ "path": "README.md", @@ -2064,6 +2082,43 @@ func (m *picoInterleavedContentProvider) GetDefaultModel() string { return "pico-interleaved-content-model" } +type picoDistinctToolCallContentProvider struct { + calls int +} + +func (m *picoDistinctToolCallContentProvider) 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{ + Content: "intermediate model text", + ToolCalls: []providers.ToolCall{{ + ID: "call_tool_limit_test", + Type: "function", + Name: "tool_limit_test_tool", + Arguments: map[string]any{"value": "x"}, + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read the file before replying.", + }, + }}, + }, nil + } + + return &providers.LLMResponse{ + Content: "final model text", + ToolCalls: []providers.ToolCall{}, + }, nil +} + +func (m *picoDistinctToolCallContentProvider) GetDefaultModel() string { + return "pico-distinct-tool-call-content-model" +} + type toolLimitOnlyProvider struct{} func (m *toolLimitOnlyProvider) Chat( @@ -3987,6 +4042,7 @@ 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") } @@ -4008,7 +4064,7 @@ func TestProcessMessage_PublishesToolFeedbackWhenEnabled(t *testing.T) { if !strings.Contains(outbound.Content, "\"path\":") { t.Fatalf("tool feedback content = %q, want serialized tool arguments", outbound.Content) } - if !strings.Contains(outbound.Content, heartbeatFile) { + if !strings.Contains(outbound.Content, escapedHeartbeatFile) { t.Fatalf("tool feedback content = %q, want tool argument value", outbound.Content) } if strings.Contains(outbound.Content, "Previous turn explanation") { @@ -4250,6 +4306,7 @@ func TestProcessMessage_DoesNotLeakReasoningContentInToolFeedback(t *testing.T) select { case outbound := <-msgBus.OutboundChan(): + escapedHeartbeatFile := strings.ReplaceAll(heartbeatFile, `\`, `\\`) if !strings.Contains(outbound.Content, "`read_file`") { t.Fatalf("tool feedback content = %q, want read_file summary", outbound.Content) } @@ -4262,7 +4319,7 @@ func TestProcessMessage_DoesNotLeakReasoningContentInToolFeedback(t *testing.T) if !strings.Contains(outbound.Content, "\"path\":") { t.Fatalf("tool feedback content = %q, want serialized tool arguments", outbound.Content) } - if !strings.Contains(outbound.Content, heartbeatFile) { + if !strings.Contains(outbound.Content, escapedHeartbeatFile) { t.Fatalf("tool feedback content = %q, want tool argument value", outbound.Content) } if strings.Contains(outbound.Content, "Read README.md first") { @@ -4396,7 +4453,7 @@ func TestRun_PicoPublishesAssistantContentDuringToolCallsWithoutFinalDuplicate(t } msgBus := bus.NewMessageBus() - provider := &picoInterleavedContentProvider{} + provider := &picoDistinctToolCallContentProvider{} al := NewAgentLoop(cfg, msgBus, provider) agent := al.GetRegistry().GetDefaultAgent() @@ -4422,22 +4479,28 @@ func TestRun_PicoPublishesAssistantContentDuringToolCallsWithoutFinalDuplicate(t t.Fatalf("PublishInbound() error = %v", err) } - outputs := make([]string, 0, 2) + outputs := make([]bus.OutboundMessage, 0, 3) deadline := time.After(2 * time.Second) - for len(outputs) < 2 { + for len(outputs) < 3 { select { case outbound := <-msgBus.OutboundChan(): - outputs = append(outputs, outbound.Content) + outputs = append(outputs, outbound) case <-deadline: t.Fatalf("timed out waiting for pico outputs, got %v", outputs) } } - if outputs[0] != "intermediate model text" { - t.Fatalf("first outbound content = %q, want %q", outputs[0], "intermediate model text") + if outputs[0].Content != "intermediate model text" { + t.Fatalf("first outbound content = %q, want %q", outputs[0].Content, "intermediate model text") } - if outputs[1] != "final model text" { - t.Fatalf("second outbound content = %q, want %q", outputs[1], "final model text") + if outputs[1].Context.Raw[metadataKeyMessageKind] != messageKindToolCalls { + t.Fatalf("second outbound = %+v, want tool_calls message", outputs[1]) + } + if !strings.Contains(outputs[1].Context.Raw[metadataKeyToolCalls], "tool_limit_test_tool") { + t.Fatalf("second outbound tool_calls = %q, want tool name", outputs[1].Context.Raw[metadataKeyToolCalls]) + } + if outputs[2].Content != "final model text" { + t.Fatalf("third outbound content = %q, want %q", outputs[2].Content, "final model text") } runCancel() @@ -4552,22 +4615,28 @@ func TestRun_PicoToolFeedbackSuppressesDuplicateInterimAssistantContent(t *testi t.Fatalf("PublishInbound() error = %v", err) } - outputs := make([]string, 0, 2) + outputs := make([]bus.OutboundMessage, 0, 3) deadline := time.After(2 * time.Second) for len(outputs) < 2 { select { case outbound := <-msgBus.OutboundChan(): - outputs = append(outputs, outbound.Content) + outputs = append(outputs, outbound) case <-deadline: t.Fatalf("timed out waiting for pico outputs, got %v", outputs) } } - if outputs[0] != "🔧 `tool_limit_test_tool`\nintermediate model text\n```json\n{\n \"value\": \"x\"\n}\n```" { - t.Fatalf("first outbound content = %q, want tool feedback summary", outputs[0]) + if outputs[0].Context.Raw[metadataKeyMessageKind] != messageKindToolCalls { + t.Fatalf("first outbound = %+v, want tool_calls message", outputs[0]) } - if outputs[1] != "final model text" { - t.Fatalf("second outbound content = %q, want %q", outputs[1], "final model text") + if outputs[0].Content != "" { + t.Fatalf("first outbound content = %q, want empty tool_calls content", outputs[0].Content) + } + if !strings.Contains(outputs[0].Context.Raw[metadataKeyToolCalls], "tool_limit_test_tool") { + t.Fatalf("first outbound tool_calls = %q, want tool name", outputs[0].Context.Raw[metadataKeyToolCalls]) + } + if outputs[1].Content != "final model text" { + t.Fatalf("second outbound content = %q, want %q", outputs[1].Content, "final model text") } runCancel() diff --git a/pkg/agent/agent_utils.go b/pkg/agent/agent_utils.go index 4ba75cde4..bbfb3f2ae 100644 --- a/pkg/agent/agent_utils.go +++ b/pkg/agent/agent_utils.go @@ -115,7 +115,6 @@ func latestUserContent(messages []providers.Message) string { func toolFeedbackExplanationFromResponse( response *providers.LLMResponse, messages []providers.Message, - maxLen int, ) string { if response == nil { return "" @@ -127,7 +126,7 @@ func toolFeedbackExplanationFromResponse( if explanation == "" { explanation = toolFeedbackExplanationFromMessages(messages) } - return utils.Truncate(explanation, maxLen) + return explanation } func toolFeedbackExplanationFromToolCalls(toolCalls []providers.ToolCall) string { @@ -146,22 +145,21 @@ 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) + return explanation } } if response == nil { - return utils.Truncate(toolFeedbackExplanationFromMessages(messages), maxLen) + return toolFeedbackExplanationFromMessages(messages) } explanation := strings.TrimSpace(response.Content) if explanation == "" { explanation = toolFeedbackExplanationFromMessages(messages) } - return utils.Truncate(explanation, maxLen) + return explanation } func toolFeedbackExplanationFromMessages(messages []providers.Message) string { diff --git a/pkg/agent/pipeline_execute.go b/pkg/agent/pipeline_execute.go index 0cf3eaa9a..9935f2c9e 100644 --- a/pkg/agent/pipeline_execute.go +++ b/pkg/agent/pipeline_execute.go @@ -80,13 +80,12 @@ toolLoop: }, ) - if shouldPublishToolFeedback(al.cfg, ts) { + if shouldPublishToolFeedback(al.cfg, ts) && ts.channel != "pico" { toolFeedbackMaxLen := al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength() toolFeedbackExplanation := toolFeedbackExplanationForToolCall( exec.response, tc, messages, - toolFeedbackMaxLen, ) feedbackMsg := utils.FormatToolFeedbackMessage( toolName, @@ -362,13 +361,12 @@ toolLoop: }, ) - if shouldPublishToolFeedback(al.cfg, ts) { + if shouldPublishToolFeedback(al.cfg, ts) && ts.channel != "pico" { toolFeedbackMaxLen := al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength() toolFeedbackExplanation := toolFeedbackExplanationForToolCall( exec.response, tc, messages, - toolFeedbackMaxLen, ) feedbackMsg := utils.FormatToolFeedbackMessage( toolName, diff --git a/pkg/agent/pipeline_llm.go b/pkg/agent/pipeline_llm.go index a954c0ca6..6bf55fa39 100644 --- a/pkg/agent/pipeline_llm.go +++ b/pkg/agent/pipeline_llm.go @@ -10,7 +10,6 @@ import ( "strings" "time" - "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/constants" "github.com/sipeed/picoclaw/pkg/logger" "github.com/sipeed/picoclaw/pkg/providers" @@ -383,7 +382,11 @@ func (p *Pipeline) CallLLM( } reasoningContent := responseReasoningContent(exec.response) - if ts.channel == "pico" { + shouldPublishPicoToolCallInterim := ts.channel == "pico" && len(exec.response.ToolCalls) > 0 + if shouldPublishPicoToolCallInterim { + // Pico tool-call turns publish their reasoning/content/tool summary as a + // structured sequence after the tool-call payload is normalized below. + } else if ts.channel == "pico" { go al.publishPicoReasoning(turnCtx, reasoningContent, ts.chatID) } else { go al.handleReasoning( @@ -419,30 +422,6 @@ func (p *Pipeline) CallLLM( } logger.DebugCF("agent", "LLM response", llmResponseFields) - if al.bus != nil && - ts.channel == "pico" && - len(exec.response.ToolCalls) > 0 && - ts.opts.AllowInterimPicoPublish && - !shouldPublishToolFeedback(al.cfg, ts) { - if strings.TrimSpace(exec.response.Content) != "" { - outCtx, outCancel := context.WithTimeout(turnCtx, 3*time.Second) - publishErr := al.bus.PublishOutbound(outCtx, bus.OutboundMessage{ - Channel: ts.channel, - ChatID: ts.chatID, - Content: exec.response.Content, - }) - outCancel() - if publishErr != nil { - logger.WarnCF("agent", "Failed to publish pico interim tool-call content", map[string]any{ - "error": publishErr.Error(), - "channel": ts.channel, - "chat_id": ts.chatID, - "iteration": iteration, - }) - } - } - } - // No-tool-call path: steering check and direct response if len(exec.response.ToolCalls) == 0 || exec.gracefulTerminal { responseContent := exec.response.Content @@ -499,7 +478,6 @@ func (p *Pipeline) CallLLM( exec.response, tc, exec.messages, - al.cfg.Agents.Defaults.GetToolFeedbackMaxArgsLength(), ) extraContent := tc.ExtraContent if strings.TrimSpace(toolFeedbackExplanation) != "" { @@ -531,6 +509,15 @@ func (p *Pipeline) CallLLM( ts.recordPersistedMessage(assistantMsg) ts.ingestMessage(turnCtx, al, assistantMsg) } + if shouldPublishPicoToolCallInterim { + al.publishPicoToolCallInterim( + turnCtx, + ts, + reasoningContent, + exec.response.Content, + assistantMsg.ToolCalls, + ) + } return ControlToolLoop, nil } diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index 7974a39e4..d56c4fd9b 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -134,6 +134,14 @@ func outboundMessageIsToolFeedback(msg bus.OutboundMessage) bool { return strings.EqualFold(strings.TrimSpace(msg.Context.Raw["message_kind"]), "tool_feedback") } +func outboundMessageBypassesPlaceholderEdit(msg bus.OutboundMessage) bool { + if len(msg.Context.Raw) == 0 { + return false + } + kind := strings.TrimSpace(msg.Context.Raw["message_kind"]) + return strings.EqualFold(kind, "thought") || strings.EqualFold(kind, "tool_calls") +} + func outboundMediaChannel(msg bus.OutboundMediaMessage) string { return msg.Context.Channel } @@ -332,6 +340,12 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } return nil, false } + if outboundMessageBypassesPlaceholderEdit(msg) { + if deleter, ok := ch.(MessageDeleter); ok { + deleter.DeleteMessage(ctx, chatID, entry.id) // best effort + } + return nil, false + } if editor, ok := ch.(MessageEditor); ok { content := msg.Content trackedContent := msg.Content diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index a5d7c2838..6c518780d 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -1131,6 +1131,107 @@ func TestPreSend_ToolFeedbackSeparateMessagesDeletesPlaceholderAndSkipsEdit(t *t } } +func TestPreSend_ThoughtPlaceholderDeleteAndSkipsEdit(t *testing.T) { + m := newTestManager() + + ch := &mockDeletingMessageEditor{ + mockMessageEditor: mockMessageEditor{ + editFn: func(_ context.Context, _, _, _ string) error { + t.Fatal("expected thought message to bypass placeholder edit") + return nil + }, + }, + } + + m.RecordPlaceholder("test", "123", "456") + + msg := testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Content: "thinking trace", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + Raw: map[string]string{ + "message_kind": "thought", + }, + }, + }) + + msgIDs, handled := m.preSend(context.Background(), "test", msg, ch) + if handled { + t.Fatalf( + "expected thought message to fall through so the channel can send a structured message, got %v", + msgIDs, + ) + } + if ch.deleteCalls != 1 { + t.Fatalf("expected placeholder deletion, got %d delete calls", ch.deleteCalls) + } + if ch.deletedChatID != "123" || ch.deletedMessageID != "456" { + t.Fatalf("unexpected placeholder deletion target: %s/%s", ch.deletedChatID, ch.deletedMessageID) + } + if _, ok := m.placeholders.Load("test:123"); ok { + t.Fatal("expected placeholder to be consumed before structured thought send") + } +} + +func TestSendWithRetry_ToolCallsPlaceholderDeleteAndFallsThroughToSend(t *testing.T) { + m := newTestManager() + + ch := &mockDeletingMessageEditor{ + mockMessageEditor: mockMessageEditor{ + mockChannel: mockChannel{ + sendFn: func(_ context.Context, msg bus.OutboundMessage) error { + if got := msg.Context.Raw["message_kind"]; got != "tool_calls" { + t.Fatalf("expected tool_calls message kind, got %q", got) + } + if msg.Content != "" { + t.Fatalf("expected empty tool_calls content, got %q", msg.Content) + } + return nil + }, + }, + editFn: func(_ context.Context, _, _, _ string) error { + t.Fatal("expected tool_calls message to bypass placeholder edit") + return nil + }, + }, + } + + m.RecordPlaceholder("test", "123", "456") + + w := &channelWorker{ + ch: ch, + limiter: rate.NewLimiter(rate.Inf, 1), + } + + msg := testOutboundMessage(bus.OutboundMessage{ + Channel: "test", + ChatID: "123", + Context: bus.InboundContext{ + Channel: "test", + ChatID: "123", + Raw: map[string]string{ + "message_kind": "tool_calls", + "tool_calls": `[{"id":"call_1","type":"function","function":{"name":"read_file","arguments":"{}"},"extra_content":{"tool_feedback_explanation":"Looking up config"}}]`, + }, + }, + }) + + m.sendWithRetry(context.Background(), "test", w, msg) + + if ch.deleteCalls != 1 { + t.Fatalf("expected placeholder deletion, got %d delete calls", ch.deleteCalls) + } + if ch.deletedChatID != "123" || ch.deletedMessageID != "456" { + t.Fatalf("unexpected placeholder deletion target: %s/%s", ch.deletedChatID, ch.deletedMessageID) + } + if len(ch.sentMessages) != 1 { + t.Fatalf("expected structured tool_calls message to be sent once, got %d", len(ch.sentMessages)) + } +} + func TestPreSend_NonToolFeedbackSeparateMessagesClearsTrackedMessageWithoutDismiss(t *testing.T) { m := newTestManager() m.config = &config.Config{ diff --git a/pkg/channels/pico/pico.go b/pkg/channels/pico/pico.go index 31360b3de..9bd8a5b5d 100644 --- a/pkg/channels/pico/pico.go +++ b/pkg/channels/pico/pico.go @@ -23,6 +23,7 @@ import ( "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/identity" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/utils" ) // picoConn represents a single WebSocket connection. @@ -57,8 +58,17 @@ func outboundMessageIsToolFeedback(msg bus.OutboundMessage) bool { return strings.EqualFold(strings.TrimSpace(msg.Context.Raw["message_kind"]), "tool_feedback") } +func outboundMessageIsToolCalls(msg bus.OutboundMessage) bool { + if len(msg.Context.Raw) == 0 { + return false + } + return strings.EqualFold(strings.TrimSpace(msg.Context.Raw["message_kind"]), MessageKindToolCalls) +} + func outboundMessageFinalizesTrackedToolFeedback(msg bus.OutboundMessage) bool { - return !outboundMessageIsToolFeedback(msg) && !outboundMessageIsThought(msg) + return !outboundMessageIsToolFeedback(msg) && + !outboundMessageIsThought(msg) && + !outboundMessageIsToolCalls(msg) } // writeJSON sends a JSON message to the connection with write locking. @@ -289,6 +299,7 @@ func (c *PicoChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]stri } isThought := outboundMessageIsThought(msg) isToolFeedback := outboundMessageIsToolFeedback(msg) + isToolCalls := outboundMessageIsToolCalls(msg) if isToolFeedback { if msgID, handled, err := c.progress.Update(ctx, msg.ChatID, msg.Content); handled { if err != nil { @@ -315,6 +326,12 @@ func (c *PicoChannel) Send(ctx context.Context, msg bus.OutboundMessage) ([]stri PayloadKeyThought: isThought, "message_id": msgID, } + if isToolCalls { + payload[PayloadKeyKind] = MessageKindToolCalls + if toolCalls, ok := picoToolCallsPayload(msg); ok { + payload[PayloadKeyToolCalls] = toolCalls + } + } setContextUsagePayload(payload, msg.ContextUsage) outMsg := newMessage(TypeMessageCreate, payload) @@ -1070,6 +1087,19 @@ func setContextUsagePayload(payload map[string]any, u *bus.ContextUsage) { } } +func picoToolCallsPayload(msg bus.OutboundMessage) ([]utils.VisibleToolCall, bool) { + raw := strings.TrimSpace(msg.Context.Raw[PayloadKeyToolCalls]) + if raw == "" { + return nil, false + } + + var toolCalls []utils.VisibleToolCall + if err := json.Unmarshal([]byte(raw), &toolCalls); err != nil || len(toolCalls) == 0 { + return nil, false + } + return toolCalls, true +} + func (c *PicoChannel) editMessage( ctx context.Context, chatID string, diff --git a/pkg/channels/pico/protocol.go b/pkg/channels/pico/protocol.go index 8a27b8c93..46e8fa3ee 100644 --- a/pkg/channels/pico/protocol.go +++ b/pkg/channels/pico/protocol.go @@ -19,10 +19,13 @@ const ( TypeError = "error" TypePong = "pong" - PayloadKeyContent = "content" - PayloadKeyThought = "thought" + PayloadKeyContent = "content" + PayloadKeyThought = "thought" + PayloadKeyKind = "kind" + PayloadKeyToolCalls = "tool_calls" - MessageKindThought = "thought" + MessageKindThought = "thought" + MessageKindToolCalls = "tool_calls" ) // PicoMessage is the wire format for all Pico Protocol messages. diff --git a/pkg/config/config.go b/pkg/config/config.go index 6bb8d3ce6..305c3a5c0 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -287,7 +287,7 @@ func (d *AgentDefaults) GetMaxMediaSize() int { return DefaultMaxMediaSize } -// GetToolFeedbackMaxArgsLength returns the max visible text length for tool feedback messages. +// GetToolFeedbackMaxArgsLength returns the max visible text length for tool argument previews. func (d *AgentDefaults) GetToolFeedbackMaxArgsLength() int { if d.ToolFeedback.MaxArgsLength > 0 { return d.ToolFeedback.MaxArgsLength diff --git a/pkg/utils/tool_feedback_dedupe.go b/pkg/utils/tool_feedback_dedupe.go new file mode 100644 index 000000000..b1adb60eb --- /dev/null +++ b/pkg/utils/tool_feedback_dedupe.go @@ -0,0 +1,39 @@ +package utils + +import ( + "strings" + + "github.com/sipeed/picoclaw/pkg/providers" +) + +func normalizeToolFeedbackComparisonText(text string) string { + text = strings.ReplaceAll(text, "\r\n", "\n") + text = strings.ReplaceAll(text, "\r", "\n") + text = strings.TrimSpace(text) + if text == "" { + return "" + } + return strings.Join(strings.Fields(text), " ") +} + +func ToolCallExplanationDuplicatesContent(content string, toolCalls []providers.ToolCall) bool { + normalizedContent := normalizeToolFeedbackComparisonText(content) + if normalizedContent == "" || len(toolCalls) == 0 { + return false + } + + for _, tc := range toolCalls { + if tc.ExtraContent == nil { + continue + } + explanation := normalizeToolFeedbackComparisonText(tc.ExtraContent.ToolFeedbackExplanation) + if explanation == "" { + continue + } + if explanation == normalizedContent { + return true + } + } + + return false +} diff --git a/pkg/utils/tool_feedback_dedupe_test.go b/pkg/utils/tool_feedback_dedupe_test.go new file mode 100644 index 000000000..cc587080f --- /dev/null +++ b/pkg/utils/tool_feedback_dedupe_test.go @@ -0,0 +1,55 @@ +package utils + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/providers" +) + +func TestToolCallExplanationDuplicatesContent(t *testing.T) { + t.Run("exact duplicate", func(t *testing.T) { + toolCalls := []providers.ToolCall{{ + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read the file before replying.", + }, + }} + + if !ToolCallExplanationDuplicatesContent("Read the file before replying.", toolCalls) { + t.Fatal("expected duplicated content to be detected") + } + }) + + t.Run("whitespace normalized duplicate", func(t *testing.T) { + toolCalls := []providers.ToolCall{{ + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read the file\nbefore replying.", + }, + }} + + if !ToolCallExplanationDuplicatesContent(" Read the file before replying. ", toolCalls) { + t.Fatal("expected whitespace-only differences to be ignored") + } + }) + + t.Run("distinct content", func(t *testing.T) { + toolCalls := []providers.ToolCall{{ + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read the file before replying.", + }, + }} + + if ToolCallExplanationDuplicatesContent( + "I will summarize the findings after reading the file.", + toolCalls, + ) { + t.Fatal("expected distinct content to remain visible") + } + }) + + t.Run("missing explanation", func(t *testing.T) { + toolCalls := []providers.ToolCall{{}} + if ToolCallExplanationDuplicatesContent("Read the file before replying.", toolCalls) { + t.Fatal("expected empty tool explanations to skip dedupe") + } + }) +} diff --git a/pkg/utils/visible_tool_calls.go b/pkg/utils/visible_tool_calls.go new file mode 100644 index 000000000..8c4d89a51 --- /dev/null +++ b/pkg/utils/visible_tool_calls.go @@ -0,0 +1,106 @@ +package utils + +import ( + "bytes" + "encoding/json" + "strings" + + "github.com/sipeed/picoclaw/pkg/providers" +) + +type VisibleToolCall struct { + ID string `json:"id,omitempty"` + Type string `json:"type,omitempty"` + Function *VisibleToolCallFunction `json:"function,omitempty"` + ExtraContent *VisibleToolCallExtraContent `json:"extra_content,omitempty"` +} + +type VisibleToolCallFunction struct { + Name string `json:"name,omitempty"` + Arguments string `json:"arguments,omitempty"` +} + +type VisibleToolCallExtraContent struct { + ToolFeedbackExplanation string `json:"tool_feedback_explanation,omitempty"` +} + +func BuildVisibleToolCalls( + toolCalls []providers.ToolCall, + maxArgsLen int, +) []VisibleToolCall { + if len(toolCalls) == 0 { + return nil + } + + visible := make([]VisibleToolCall, 0, len(toolCalls)) + for _, tc := range toolCalls { + name, _ := VisibleToolCallNameAndArguments(tc) + argsPreview := VisibleToolCallArgumentsPreview(tc, maxArgsLen) + explanation := "" + if tc.ExtraContent != nil { + explanation = strings.TrimSpace(tc.ExtraContent.ToolFeedbackExplanation) + } + if name == "" && explanation == "" && argsPreview == "" { + continue + } + + visibleCall := VisibleToolCall{ + ID: strings.TrimSpace(tc.ID), + Type: strings.TrimSpace(tc.Type), + } + if visibleCall.Type == "" { + visibleCall.Type = "function" + } + if name != "" || argsPreview != "" { + visibleCall.Function = &VisibleToolCallFunction{ + Name: name, + Arguments: argsPreview, + } + } + if explanation != "" { + visibleCall.ExtraContent = &VisibleToolCallExtraContent{ + ToolFeedbackExplanation: explanation, + } + } + + visible = append(visible, visibleCall) + } + + if len(visible) == 0 { + return nil + } + return visible +} + +func VisibleToolCallNameAndArguments(tc providers.ToolCall) (string, string) { + name := strings.TrimSpace(tc.Name) + argsJSON := "" + if tc.Function != nil { + if name == "" { + name = strings.TrimSpace(tc.Function.Name) + } + argsJSON = strings.TrimSpace(tc.Function.Arguments) + } + if argsJSON == "" && len(tc.Arguments) > 0 { + if encodedArgs, err := json.Marshal(tc.Arguments); err == nil { + argsJSON = string(encodedArgs) + } + } + return name, strings.TrimSpace(argsJSON) +} + +func VisibleToolCallArgumentsPreview(tc providers.ToolCall, maxLen int) string { + _, argsJSON := VisibleToolCallNameAndArguments(tc) + if argsJSON == "" { + return "" + } + + var pretty bytes.Buffer + if err := json.Indent(&pretty, []byte(argsJSON), "", " "); err == nil { + argsJSON = pretty.String() + } + if maxLen > 0 { + return Truncate(argsJSON, maxLen) + } + return argsJSON +} diff --git a/pkg/utils/visible_tool_calls_test.go b/pkg/utils/visible_tool_calls_test.go new file mode 100644 index 000000000..fe9467c57 --- /dev/null +++ b/pkg/utils/visible_tool_calls_test.go @@ -0,0 +1,33 @@ +package utils + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/providers" +) + +func TestBuildVisibleToolCalls_DoesNotTruncateExplanation(t *testing.T) { + explanation := "Read README.md first to confirm the current project structure before editing the config example." + toolCalls := []providers.ToolCall{{ + ID: "call_1", + Type: "function", + Function: &providers.FunctionCall{ + Name: "read_file", + Arguments: `{"path":"README.md","start_line":1,"end_line":10,"extra":"abcdefghijklmnopqrstuvwxyz"}`, + }, + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: explanation, + }, + }} + + visible := BuildVisibleToolCalls(toolCalls, 20) + if len(visible) != 1 { + t.Fatalf("len(visible) = %d, want 1", len(visible)) + } + if visible[0].ExtraContent == nil || visible[0].ExtraContent.ToolFeedbackExplanation != explanation { + t.Fatalf("visible explanation = %#v, want %q", visible[0].ExtraContent, explanation) + } + if visible[0].Function == nil || visible[0].Function.Arguments == "" { + t.Fatalf("visible function = %#v, want truncated args preview", visible[0].Function) + } +} diff --git a/web/backend/api/session.go b/web/backend/api/session.go index 83819f319..cc18ee6e1 100644 --- a/web/backend/api/session.go +++ b/web/backend/api/session.go @@ -2,7 +2,6 @@ package api import ( "bufio" - "bytes" "encoding/json" "errors" "net/http" @@ -53,6 +52,7 @@ type sessionChatMessage struct { Kind string `json:"kind,omitempty"` Media []string `json:"media,omitempty"` Attachments []sessionChatAttachment `json:"attachments,omitempty"` + ToolCalls []utils.VisibleToolCall `json:"tool_calls,omitempty"` } type sessionChatAttachment struct { @@ -456,7 +456,10 @@ func truncateRunes(s string, maxLen int) string { } func sessionChatMessageVisible(msg sessionChatMessage) bool { - return strings.TrimSpace(msg.Content) != "" || len(msg.Media) > 0 || len(msg.Attachments) > 0 + return strings.TrimSpace(msg.Content) != "" || + len(msg.Media) > 0 || + len(msg.Attachments) > 0 || + len(msg.ToolCalls) > 0 } func sessionChatMessagePreview(msg sessionChatMessage) string { @@ -475,6 +478,9 @@ func sessionChatMessagePreview(msg sessionChatMessage) string { } return "[attachment]" } + if len(msg.ToolCalls) > 0 { + return "[tool call]" + } return "" } @@ -521,25 +527,11 @@ func sessionTranscriptMessages( } } - toolSummaryMessages := visibleAssistantToolSummaryMessages(msg.ToolCalls, toolFeedbackMaxArgsLength) - if len(toolSummaryMessages) > 0 { - transcript = append(transcript, toolSummaryMessages...) - } - + toolCallsMsg, hasToolCallsMsg := assistantToolCallsMessage( + msg.ToolCalls, + toolFeedbackMaxArgsLength, + ) visibleToolMessages := visibleAssistantToolMessages(msg.ToolCalls) - if len(visibleToolMessages) > 0 { - transcript = append(transcript, visibleToolMessages...) - } - - // When assistant content exactly matches the rendered tool summary or - // tool-delivered message, skip it to avoid duplicates. Distinct content - // must remain visible in restored session history. - if len(msg.ToolCalls) > 0 && - len(msg.Media) == 0 && - len(attachments) == 0 && - assistantToolCallContentDuplicated(msg.Content, toolSummaryMessages, visibleToolMessages) { - continue - } // Pico web chat can persist both visible `message` tool output and a // later plain assistant reply in the same turn. Hide only the fixed @@ -547,10 +539,19 @@ func sessionTranscriptMessages( content := msg.Content if assistantMessageInternalOnly(msg) { if len(attachments) == 0 { + if hasToolCallsMsg { + transcript = append(transcript, toolCallsMsg) + } + if len(visibleToolMessages) > 0 { + transcript = append(transcript, visibleToolMessages...) + } continue } content = "" } + if hasToolCallsMsg && utils.ToolCallExplanationDuplicatesContent(content, msg.ToolCalls) { + content = "" + } chatMsg := sessionChatMessage{ Role: "assistant", @@ -559,10 +560,22 @@ func sessionTranscriptMessages( Attachments: attachments, } if !sessionChatMessageVisible(chatMsg) { + if hasToolCallsMsg { + transcript = append(transcript, toolCallsMsg) + } + if len(visibleToolMessages) > 0 { + transcript = append(transcript, visibleToolMessages...) + } continue } transcript = append(transcript, chatMsg) + if hasToolCallsMsg { + transcript = append(transcript, toolCallsMsg) + } + if len(visibleToolMessages) > 0 { + transcript = append(transcript, visibleToolMessages...) + } } } @@ -580,51 +593,6 @@ func filterSessionChatMessages(messages []sessionChatMessage) []sessionChatMessa return filtered } -func assistantToolCallContentDuplicated( - content string, - toolSummaryMessages []sessionChatMessage, - visibleToolMessages []sessionChatMessage, -) bool { - content = strings.TrimSpace(content) - if content == "" { - return false - } - - for _, msg := range toolSummaryMessages { - if toolSummaryContainsContent(msg.Content, content) { - return true - } - } - for _, msg := range visibleToolMessages { - if strings.TrimSpace(msg.Content) == content { - return true - } - } - return false -} - -func toolSummaryContainsContent(summary, content string) bool { - summary = strings.TrimSpace(summary) - content = strings.TrimSpace(content) - if summary == "" || content == "" { - return false - } - if summary == content { - return true - } - - _, body, hasBody := strings.Cut(summary, "\n") - if !hasBody { - return false - } - body = strings.TrimSpace(body) - if body == content { - return true - } - firstSection, _, _ := strings.Cut(body, "\n```") - return strings.TrimSpace(firstSection) == content -} - func sessionAttachments(msg providers.Message) []sessionChatAttachment { if len(msg.Attachments) == 0 { return nil @@ -720,80 +688,34 @@ func assistantThoughtMessage(msg providers.Message) (sessionChatMessage, bool) { }, true } -func visibleAssistantToolSummaryMessages( +func assistantToolCallsMessage( toolCalls []providers.ToolCall, toolFeedbackMaxArgsLength int, -) []sessionChatMessage { +) (sessionChatMessage, bool) { if len(toolCalls) == 0 { - return nil + return sessionChatMessage{}, false } if toolFeedbackMaxArgsLength <= 0 { toolFeedbackMaxArgsLength = defaultToolFeedbackMaxArgsLength() } - messages := make([]sessionChatMessage, 0, len(toolCalls)) - for _, tc := range toolCalls { - name, argsJSON := toolCallNameAndArguments(tc) - if strings.TrimSpace(name) == "" { - continue - } - if name == "web_search" || name == "web_fetch" { - continue - } - if name == "message" { - if _, ok := parseMessageToolContent(argsJSON); ok { - continue - } - } - - messages = append(messages, sessionChatMessage{ - Role: "assistant", - Content: utils.FormatToolFeedbackMessage( - name, - visibleAssistantToolFeedbackExplanation(tc, toolFeedbackMaxArgsLength), - visibleAssistantToolArgsPreview(tc, toolFeedbackMaxArgsLength), - ), - }) + visibleToolCalls := utils.BuildVisibleToolCalls(toolCalls, toolFeedbackMaxArgsLength) + if len(visibleToolCalls) == 0 { + return sessionChatMessage{}, false } - return messages -} - -func visibleAssistantToolFeedbackExplanation( - tc providers.ToolCall, - toolFeedbackMaxArgsLength int, -) string { - if tc.ExtraContent != nil { - if explanation := strings.TrimSpace(tc.ExtraContent.ToolFeedbackExplanation); explanation != "" { - return utils.Truncate(explanation, toolFeedbackMaxArgsLength) - } - } - return "" + return sessionChatMessage{ + Role: "assistant", + Kind: "tool_calls", + ToolCalls: visibleToolCalls, + }, true } func visibleAssistantToolArgsPreview( tc providers.ToolCall, toolFeedbackMaxArgsLength int, ) string { - argsJSON := "" - if tc.Function != nil { - argsJSON = tc.Function.Arguments - } - if strings.TrimSpace(argsJSON) == "" && len(tc.Arguments) > 0 { - if encodedArgs, err := json.MarshalIndent(tc.Arguments, "", " "); err == nil { - argsJSON = string(encodedArgs) - } - } - argsJSON = strings.TrimSpace(argsJSON) - if argsJSON == "" { - return "" - } - var pretty bytes.Buffer - if err := json.Indent(&pretty, []byte(argsJSON), "", " "); err == nil { - argsJSON = pretty.String() - } - - return utils.Truncate(argsJSON, toolFeedbackMaxArgsLength) + return utils.VisibleToolCallArgumentsPreview(tc, toolFeedbackMaxArgsLength) } func visibleAssistantToolMessages(toolCalls []providers.ToolCall) []sessionChatMessage { @@ -803,7 +725,7 @@ func visibleAssistantToolMessages(toolCalls []providers.ToolCall) []sessionChatM messages := make([]sessionChatMessage, 0, len(toolCalls)) for _, tc := range toolCalls { - name, argsJSON := toolCallNameAndArguments(tc) + name, argsJSON := utils.VisibleToolCallNameAndArguments(tc) if name != "message" { continue } @@ -820,23 +742,6 @@ func visibleAssistantToolMessages(toolCalls []providers.ToolCall) []sessionChatM return messages } -func toolCallNameAndArguments(tc providers.ToolCall) (string, string) { - name := tc.Name - argsJSON := "" - if tc.Function != nil { - if name == "" { - name = tc.Function.Name - } - argsJSON = tc.Function.Arguments - } - if strings.TrimSpace(argsJSON) == "" && len(tc.Arguments) > 0 { - if encodedArgs, err := json.Marshal(tc.Arguments); err == nil { - argsJSON = string(encodedArgs) - } - } - return name, argsJSON -} - func parseMessageToolContent(argsJSON string) (string, bool) { var args struct { Content string `json:"content"` diff --git a/web/backend/api/session_test.go b/web/backend/api/session_test.go index ec91b9792..760935db7 100644 --- a/web/backend/api/session_test.go +++ b/web/backend/api/session_test.go @@ -32,6 +32,25 @@ func sessionsTestDir(t *testing.T, configPath string) string { return dir } +func assertVisibleToolCallMessage( + t *testing.T, + msg sessionChatMessage, + toolName string, +) utils.VisibleToolCall { + t.Helper() + + if msg.Role != "assistant" || msg.Kind != "tool_calls" { + t.Fatalf("message = %#v, want assistant/tool_calls", msg) + } + if len(msg.ToolCalls) != 1 { + t.Fatalf("len(message.ToolCalls) = %d, want 1", len(msg.ToolCalls)) + } + if got := msg.ToolCalls[0].Function; got == nil || got.Name != toolName { + t.Fatalf("tool call = %#v, want function %q", msg.ToolCalls[0], toolName) + } + return msg.ToolCalls[0] +} + func TestHandleListSessions_JSONLStorage(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup() @@ -516,11 +535,7 @@ func TestHandleGetSession_SkipsTransientThoughtMessages(t *testing.T) { } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - Kind string `json:"kind"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -569,11 +584,7 @@ func TestHandleGetSession_ReconstructsThoughtFromAssistantReasoningContent(t *te } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - Kind string `json:"kind"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -667,11 +678,7 @@ func TestHandleGetSession_ReconstructsRefreshMatrixForThoughtAndToolSummary(t *t } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - Kind string `json:"kind"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -694,20 +701,14 @@ func TestHandleGetSession_ReconstructsRefreshMatrixForThoughtAndToolSummary(t *t assertMessage(2, "assistant", "", "plain visible") assertMessage(3, "user", "", "turn2") assertMessage(4, "assistant", "thought", "tool thought") - if !strings.Contains(resp.Messages[5].Content, "`read_file`") { - t.Fatalf("messages[5] = %#v, want read_file tool summary", resp.Messages[5]) - } + assertVisibleToolCallMessage(t, resp.Messages[5], "read_file") assertMessage(6, "user", "", "turn3") - if !strings.Contains(resp.Messages[7].Content, "`list_dir`") { - t.Fatalf("messages[7] = %#v, want list_dir tool summary", resp.Messages[7]) - } - assertMessage(8, "assistant", "", "tool visible only") + assertMessage(7, "assistant", "", "tool visible only") + assertVisibleToolCallMessage(t, resp.Messages[8], "list_dir") assertMessage(9, "user", "", "turn4") assertMessage(10, "assistant", "thought", "tool mixed thought") - if !strings.Contains(resp.Messages[11].Content, "`exec`") { - t.Fatalf("messages[11] = %#v, want exec tool summary", resp.Messages[11]) - } - assertMessage(12, "assistant", "", "tool visible and thought") + assertMessage(11, "assistant", "", "tool visible and thought") + assertVisibleToolCallMessage(t, resp.Messages[12], "exec") } func TestHandleGetSession_ReconstructsVisibleMessageToolOutputWithoutDuplicateSummary(t *testing.T) { @@ -758,27 +759,20 @@ func TestHandleGetSession_ReconstructsVisibleMessageToolOutputWithoutDuplicateSu } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `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 2", len(resp.Messages)) + if len(resp.Messages) != 3 { + t.Fatalf("len(resp.Messages) = %d, want 3", len(resp.Messages)) } if resp.Messages[0].Role != "user" || resp.Messages[0].Content != "test" { t.Fatalf("first message = %#v, want user/test", resp.Messages[0]) } - if resp.Messages[1].Role != "assistant" || resp.Messages[1].Content != "visible tool output" { - t.Fatalf("assistant message = %#v, want visible tool output", resp.Messages[1]) - } - for _, msg := range resp.Messages { - if msg.Role == "tool" || strings.Contains(msg.Content, "`message`") { - t.Fatalf("unexpected raw tool or duplicate message-tool summary: %#v", msg) - } + assertVisibleToolCallMessage(t, resp.Messages[1], "message") + if resp.Messages[2].Role != "assistant" || resp.Messages[2].Content != "visible tool output" { + t.Fatalf("assistant message = %#v, want visible tool output", resp.Messages[2]) } } @@ -829,25 +823,23 @@ func TestHandleGetSession_PreservesFinalAssistantReplyAfterMessageToolOutput(t * } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) } - if len(resp.Messages) != 3 { - t.Fatalf("len(resp.Messages) = %d, want 3", len(resp.Messages)) + if len(resp.Messages) != 4 { + t.Fatalf("len(resp.Messages) = %d, want 4", len(resp.Messages)) } if resp.Messages[0].Role != "user" || resp.Messages[0].Content != "test" { t.Fatalf("first message = %#v, want user/test", resp.Messages[0]) } - if resp.Messages[1].Role != "assistant" || resp.Messages[1].Content != "visible tool output" { - t.Fatalf("interim assistant message = %#v, want visible tool output", resp.Messages[1]) + assertVisibleToolCallMessage(t, resp.Messages[1], "message") + if resp.Messages[2].Role != "assistant" || resp.Messages[2].Content != "visible tool output" { + t.Fatalf("interim assistant message = %#v, want visible tool output", resp.Messages[2]) } - if resp.Messages[2].Role != "assistant" || resp.Messages[2].Content != "final assistant reply" { - t.Fatalf("final assistant message = %#v, want final assistant reply", resp.Messages[2]) + if resp.Messages[3].Role != "assistant" || resp.Messages[3].Content != "final assistant reply" { + t.Fatalf("final assistant message = %#v, want final assistant reply", resp.Messages[3]) } } @@ -897,6 +889,67 @@ func TestHandleListSessions_MessageCountUsesVisibleTranscript(t *testing.T) { t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) } + var items []sessionListItem + if err := json.Unmarshal(rec.Body.Bytes(), &items); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + if len(items) != 1 { + t.Fatalf("len(items) = %d, want 1", len(items)) + } + if items[0].MessageCount != 3 { + t.Fatalf("items[0].MessageCount = %d, want 3", items[0].MessageCount) + } +} + +func TestHandleListSessions_DeduplicatesAssistantToolCallContentFromVisibleTranscript(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + dir := sessionsTestDir(t, configPath) + store, err := memory.NewJSONLStore(dir) + if err != nil { + t.Fatalf("NewJSONLStore() error = %v", err) + } + + sessionKey := picoSessionPrefix + "list-deduped-tool-content" + for _, msg := range []providers.Message{ + {Role: "user", Content: "check file"}, + { + Role: "assistant", + Content: "Read the file before replying.", + ToolCalls: []providers.ToolCall{ + { + ID: "call_1", + Type: "function", + Function: &providers.FunctionCall{ + Name: "read_file", + Arguments: `{"path":"README.md"}`, + }, + ExtraContent: &providers.ExtraContent{ + ToolFeedbackExplanation: "Read the file before replying.", + }, + }, + }, + }, + {Role: "tool", Content: "raw read_file result", ToolCallID: "call_1"}, + } { + if err := store.AddFullMessage(nil, sessionKey, msg); err != nil { + t.Fatalf("AddFullMessage() error = %v", err) + } + } + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/sessions", 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 items []sessionListItem if err := json.Unmarshal(rec.Body.Bytes(), &items); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -959,10 +1012,7 @@ func TestHandleGetSession_DoesNotDuplicateAssistantToolCallContent(t *testing.T) } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -973,11 +1023,10 @@ func TestHandleGetSession_DoesNotDuplicateAssistantToolCallContent(t *testing.T) if resp.Messages[0].Role != "user" || resp.Messages[0].Content != "check file" { t.Fatalf("first message = %#v, want user/check file", resp.Messages[0]) } - if !strings.Contains(resp.Messages[1].Content, "`read_file`") { - t.Fatalf("tool summary message = %#v, want read_file summary", resp.Messages[1]) - } - if !strings.Contains(resp.Messages[1].Content, "Read the file before replying.") { - t.Fatalf("tool summary message = %#v, want tool explanation", resp.Messages[1]) + toolCall := assertVisibleToolCallMessage(t, resp.Messages[1], "read_file") + if toolCall.ExtraContent == nil || + toolCall.ExtraContent.ToolFeedbackExplanation != "Read the file before replying." { + t.Fatalf("tool call = %#v, want explanation", toolCall) } } @@ -1030,10 +1079,7 @@ func TestHandleGetSession_PreservesDistinctAssistantToolCallContent(t *testing.T } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -1041,13 +1087,11 @@ func TestHandleGetSession_PreservesDistinctAssistantToolCallContent(t *testing.T if len(resp.Messages) != 3 { t.Fatalf("len(resp.Messages) = %d, want 3", len(resp.Messages)) } - if !strings.Contains(resp.Messages[1].Content, "`read_file`") { - t.Fatalf("tool summary message = %#v, want read_file summary", resp.Messages[1]) - } - if resp.Messages[2].Role != "assistant" || - resp.Messages[2].Content != "I will summarize the findings after reading the file." { - t.Fatalf("assistant content = %#v, want preserved distinct content", resp.Messages[2]) + if resp.Messages[1].Role != "assistant" || + resp.Messages[1].Content != "I will summarize the findings after reading the file." { + t.Fatalf("assistant content = %#v, want preserved distinct content", resp.Messages[1]) } + assertVisibleToolCallMessage(t, resp.Messages[2], "read_file") } func TestHandleGetSession_PreservesMediaWhenAssistantToolCallContentDuplicatesSummary(t *testing.T) { @@ -1100,11 +1144,7 @@ func TestHandleGetSession_PreservesMediaWhenAssistantToolCallContentDuplicatesSu } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - Media []string `json:"media"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -1112,23 +1152,16 @@ func TestHandleGetSession_PreservesMediaWhenAssistantToolCallContentDuplicatesSu if len(resp.Messages) != 3 { t.Fatalf("len(resp.Messages) = %d, want 3", len(resp.Messages)) } - if !strings.Contains(resp.Messages[1].Content, "`view_image`") { - t.Fatalf("tool summary message = %#v, want view_image summary", resp.Messages[1]) + if resp.Messages[1].Role != "assistant" { + t.Fatalf("assistant message role = %q, want assistant", resp.Messages[1].Role) } - if resp.Messages[2].Role != "assistant" { - t.Fatalf("assistant message role = %q, want assistant", resp.Messages[2].Role) + if resp.Messages[1].Content != "" { + t.Fatalf("assistant content = %q, want duplicate content suppressed", resp.Messages[1].Content) } - if resp.Messages[2].Content != "Reviewing the generated screenshot." { - t.Fatalf("assistant content = %q, want preserved duplicated content with media", resp.Messages[2].Content) - } - if len(resp.Messages[2].Media) != 1 || resp.Messages[2].Media[0] != "data:image/png;base64,abc123" { - t.Fatalf("assistant media = %#v, want preserved media", resp.Messages[2].Media) - } - for _, msg := range resp.Messages { - if msg.Role == "tool" || strings.Contains(msg.Content, "raw read_file result") { - t.Fatalf("unexpected raw tool result in history: %#v", msg) - } + if len(resp.Messages[1].Media) != 1 || resp.Messages[1].Media[0] != "data:image/png;base64,abc123" { + t.Fatalf("assistant media = %#v, want preserved media", resp.Messages[1].Media) } + assertVisibleToolCallMessage(t, resp.Messages[2], "view_image") } func TestHandleGetSession_PreservesAttachmentsWhenAssistantToolCallContentDuplicatesSummary(t *testing.T) { @@ -1198,21 +1231,19 @@ func TestHandleGetSession_PreservesAttachmentsWhenAssistantToolCallContentDuplic if len(resp.Messages) != 3 { t.Fatalf("len(resp.Messages) = %d, want 3", len(resp.Messages)) } - if !strings.Contains(resp.Messages[1].Content, "`read_file`") { - t.Fatalf("tool summary message = %#v, want read_file summary", resp.Messages[1]) + if resp.Messages[1].Role != "assistant" { + t.Fatalf("assistant message role = %q, want assistant", resp.Messages[1].Role) } - if resp.Messages[2].Role != "assistant" { - t.Fatalf("assistant message role = %q, want assistant", resp.Messages[2].Role) + if resp.Messages[1].Content != "" { + t.Fatalf("assistant content = %q, want duplicate content suppressed", resp.Messages[1].Content) } - if resp.Messages[2].Content != "Reviewing the generated report." { - t.Fatalf("assistant content = %q, want preserved duplicated content", resp.Messages[2].Content) + if len(resp.Messages[1].Attachments) != 1 { + t.Fatalf("len(assistant.Attachments) = %d, want 1", len(resp.Messages[1].Attachments)) } - if len(resp.Messages[2].Attachments) != 1 { - t.Fatalf("len(assistant.Attachments) = %d, want 1", len(resp.Messages[2].Attachments)) - } - if resp.Messages[2].Attachments[0].URL != "https://example.com/report.txt" { - t.Fatalf("attachment url = %q, want report URL", resp.Messages[2].Attachments[0].URL) + if resp.Messages[1].Attachments[0].URL != "https://example.com/report.txt" { + t.Fatalf("attachment url = %q, want report URL", resp.Messages[1].Attachments[0].URL) } + assertVisibleToolCallMessage(t, resp.Messages[2], "read_file") } func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) { @@ -1273,10 +1304,7 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } err = json.Unmarshal(rec.Body.Bytes(), &resp) if err != nil { @@ -1286,18 +1314,15 @@ func TestHandleGetSession_UsesConfiguredToolFeedbackMaxArgsLength(t *testing.T) t.Fatalf("len(resp.Messages) = %d, want at least 2", len(resp.Messages)) } - 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) - } wantArgsPreview := visibleAssistantToolArgsPreview(providers.ToolCall{ Function: &providers.FunctionCall{Arguments: argsJSON}, }, 20) - if !strings.Contains(resp.Messages[1].Content, wantArgsPreview) { - t.Fatalf("tool summary = %q, want args preview %q", resp.Messages[1].Content, wantArgsPreview) + toolCall := assertVisibleToolCallMessage(t, resp.Messages[1], "read_file") + if toolCall.ExtraContent == nil || toolCall.ExtraContent.ToolFeedbackExplanation != explanation { + t.Fatalf("tool call = %#v, want full explanation %q", toolCall, explanation) } - if !strings.Contains(resp.Messages[1].Content, "`read_file`") { - t.Fatalf("tool summary = %q, want read_file summary", resp.Messages[1].Content) + if toolCall.Function == nil || toolCall.Function.Arguments != wantArgsPreview { + t.Fatalf("tool call = %#v, want args preview %q", toolCall, wantArgsPreview) } } @@ -1357,10 +1382,7 @@ func TestHandleGetSession_FallsBackToLegacyToolArgumentsWhenExplanationMissing(t } var resp struct { - Messages []struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"messages"` + Messages []sessionChatMessage `json:"messages"` } if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("Unmarshal() error = %v", err) @@ -1372,11 +1394,9 @@ func TestHandleGetSession_FallsBackToLegacyToolArgumentsWhenExplanationMissing(t wantPreview := visibleAssistantToolArgsPreview(providers.ToolCall{ Function: &providers.FunctionCall{Arguments: argsJSON}, }, 20) - if !strings.Contains(resp.Messages[1].Content, "`read_file`") { - t.Fatalf("tool summary = %q, want read_file summary", resp.Messages[1].Content) - } - if !strings.Contains(resp.Messages[1].Content, wantPreview) { - t.Fatalf("tool summary = %q, want legacy args preview %q", resp.Messages[1].Content, wantPreview) + toolCall := assertVisibleToolCallMessage(t, resp.Messages[1], "read_file") + if toolCall.Function == nil || toolCall.Function.Arguments != wantPreview { + t.Fatalf("tool call = %#v, want legacy args preview %q", toolCall, wantPreview) } } diff --git a/web/frontend/src/api/sessions.ts b/web/frontend/src/api/sessions.ts index d98914a59..edd7d7c27 100644 --- a/web/frontend/src/api/sessions.ts +++ b/web/frontend/src/api/sessions.ts @@ -14,7 +14,7 @@ export interface SessionDetail { messages: { role: "user" | "assistant" content: string - kind?: "normal" | "thought" + kind?: "normal" | "thought" | "tool_calls" media?: string[] attachments?: { type?: "image" | "audio" | "video" | "file" @@ -22,6 +22,17 @@ export interface SessionDetail { filename?: string content_type?: string }[] + tool_calls?: { + id?: string + type?: string + function?: { + name?: string + arguments?: string + } + extra_content?: { + tool_feedback_explanation?: string + } + }[] }[] summary: string created: string diff --git a/web/frontend/src/components/chat/assistant-message.tsx b/web/frontend/src/components/chat/assistant-message.tsx index 814ddc2f9..07a3c0abc 100644 --- a/web/frontend/src/components/chat/assistant-message.tsx +++ b/web/frontend/src/components/chat/assistant-message.tsx @@ -5,6 +5,7 @@ import { IconCopy, IconDownload, IconFileText, + IconTool, } from "@tabler/icons-react" import { useState } from "react" import { useTranslation } from "react-i18next" @@ -17,24 +18,34 @@ import remarkGfm from "remark-gfm" import { Button } from "@/components/ui/button" import { formatMessageTime } from "@/hooks/use-pico-chat" import { cn } from "@/lib/utils" -import { type ChatAttachment } from "@/store/chat" +import { + type AssistantMessageKind, + type ChatAttachment, + type ChatToolCall, +} from "@/store/chat" interface AssistantMessageProps { content: string attachments?: ChatAttachment[] - isThought?: boolean + kind?: AssistantMessageKind + toolCalls?: ChatToolCall[] timestamp?: string | number } export function AssistantMessage({ content, attachments = [], - isThought = false, + kind = "normal", + toolCalls = [], timestamp = "", }: AssistantMessageProps) { const { t } = useTranslation() const [isCopied, setIsCopied] = useState(false) + const isThought = kind === "thought" + const isToolCalls = kind === "tool_calls" + const isCollapsedBlock = isThought || isToolCalls const hasText = content.trim().length > 0 + const hasToolCalls = toolCalls.length > 0 const imageAttachments = attachments.filter( (attachment) => attachment.type === "image", ) @@ -52,9 +63,13 @@ export function AssistantMessage({ }) } + const collapsedLabel = isThought + ? t("chat.reasoningLabel") + : t("chat.toolCallsLabel") + return (
- {!isThought && ( + {!isCollapsedBlock && (
PicoClaw @@ -68,23 +83,27 @@ export function AssistantMessage({
)} - {(hasText || isThought) && ( + {(hasText || isCollapsedBlock || hasToolCalls) && (
- {isThought && ( + {isCollapsedBlock && (
setIsExpanded(!isExpanded)} >
- - {t("chat.reasoningLabel")} + {isThought ? ( + + ) : ( + + )} + {collapsedLabel}
)} - {(!isThought || isExpanded) && hasText && ( + {(!isCollapsedBlock || isExpanded) && isToolCalls && hasToolCalls && ( +
+ {toolCalls.map((toolCall, index) => { + const explanation = + toolCall.extraContent?.toolFeedbackExplanation?.trim() ?? "" + const toolName = toolCall.function?.name?.trim() ?? "" + const toolArguments = toolCall.function?.arguments?.trim() ?? "" + const hasFunctionSummary = toolName || toolArguments + + if (!explanation && !hasFunctionSummary) { + return null + } + + return ( +
0 && "border-border/20 border-t pt-3", + )} + > + {explanation && ( +
+
+ {t("chat.toolCallExplanationLabel")} +
+
+ + {explanation} + +
+
+ )} + + {hasFunctionSummary && ( +
+
+ {t("chat.toolCallFunctionLabel")} +
+
+ {toolName && ( +
+ {toolName} +
+ )} + {toolArguments && ( + 
+                              {toolArguments}
+
+ )} +
+
+ )} +
+ ) + })} +
+ )} + {(!isCollapsedBlock || isExpanded) && !isToolCalls && hasText && (
)} - {!isThought && hasText && ( + {!isCollapsedBlock && hasText && (