lzh/picoclaw
1
0
Fork 0
mirror of https://github.com/sipeed/picoclaw.git synced 2026-06-12 18:08:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(tool-feedback): add separate message mode for chat feedback (#2644)

Browse Source 
* feat(tool-feedback): add separate message mode for chat feedback

* add parameter in conf
This commit is contained in:
Mauro
2026-04-24 05:49:41 +02:00
committed by GitHub
parent 0d1b041d74
commit 9fc72c1fb3
12 changed files with 292 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/channels/manager.go
+61 -10
View File
@@ -170,6 +170,20 @@ func dismissTrackedToolFeedbackMessage(
}
}
func clearTrackedToolFeedbackMessage(
ch Channel,
chatID string,
outboundCtx *bus.InboundContext,
) {
trackedChatID := trackedToolFeedbackMessageChatID(ch, chatID, outboundCtx)
if trackedChatID == "" {
return
}
if tracker, ok := ch.(toolFeedbackMessageTracker); ok {
tracker.ClearToolFeedbackMessage(trackedChatID)
}
}
func prepareToolFeedbackMessageContent(ch Channel, content string) string {
prepared := strings.TrimSpace(content)
if prepared == "" {
@@ -183,6 +197,13 @@ func prepareToolFeedbackMessageContent(ch Channel, content string) string {
return prepared
}
func (m *Manager) toolFeedbackSeparateMessagesEnabled() bool {
if m == nil || m.config == nil {
return false
}
return m.config.Agents.Defaults.IsToolFeedbackSeparateMessagesEnabled()
}
// RecordPlaceholder registers a placeholder message for later editing.
// Implements PlaceholderRecorder.
func (m *Manager) RecordPlaceholder(channel, chatID, placeholderID string) {
@@ -264,6 +285,7 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess
}
isToolFeedback := outboundMessageIsToolFeedback(msg)
separateToolFeedbackMessages := m.toolFeedbackSeparateMessagesEnabled()
// 3. If a stream already finalized this chat, stale tool feedback must be
// dropped without consuming the final-response marker. Streaming finalization
@@ -288,14 +310,28 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess
}
}
if !isToolFeedback {
dismissTrackedToolFeedbackMessage(ctx, ch, chatID, &msg.Context)
if separateToolFeedbackMessages {
clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context)
} else {
dismissTrackedToolFeedbackMessage(ctx, ch, chatID, &msg.Context)
}
}
return nil, true
}
if separateToolFeedbackMessages {
clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context)
}
// 5. Try editing placeholder
if v, loaded := m.placeholders.LoadAndDelete(key); loaded {
if entry, ok := v.(placeholderEntry); ok && entry.id != "" {
if isToolFeedback && separateToolFeedbackMessages {
if deleter, ok := ch.(MessageDeleter); ok {
deleter.DeleteMessage(ctx, chatID, entry.id) // best effort
}
return nil, false
}
if editor, ok := ch.(MessageEditor); ok {
content := msg.Content
trackedContent := msg.Content
@@ -345,6 +381,10 @@ func (m *Manager) preSendMedia(ctx context.Context, name string, msg bus.Outboun
// 3. Clear any finalized stream marker for this chat before media delivery.
m.streamActive.LoadAndDelete(key)
if m.toolFeedbackSeparateMessagesEnabled() {
clearTrackedToolFeedbackMessage(ch, chatID, &msg.Context)
}
// 4. Delete placeholder if present.
if v, loaded := m.placeholders.LoadAndDelete(key); loaded {
if entry, ok := v.(placeholderEntry); ok && entry.id != "" {
@@ -408,15 +448,26 @@ func (m *Manager) GetStreamer(ctx context.Context, channelName, chatID string) (
return &finalizeHookStreamer{
Streamer: streamer,
onFinalize: func(finalizeCtx context.Context) {
dismissTrackedToolFeedbackMessage(
finalizeCtx,
ch,
chatID,
&bus.InboundContext{
Channel: channelName,
ChatID: chatID,
},
)
if m.toolFeedbackSeparateMessagesEnabled() {
clearTrackedToolFeedbackMessage(
ch,
chatID,
&bus.InboundContext{
Channel: channelName,
ChatID: chatID,
},
)
} else {
dismissTrackedToolFeedbackMessage(
finalizeCtx,
ch,
chatID,
&bus.InboundContext{
Channel: channelName,
ChatID: chatID,
},
)
}
m.streamActive.Store(key, true)
},
}, true
pkg/channels/manager_test.go
+184
View File
@@ -804,6 +804,20 @@ type mockResolvedToolFeedbackEditor struct {
resolveChatIDFn func(chatID string, outboundCtx *bus.InboundContext) string
}
type mockDeletingMessageEditor struct {
mockMessageEditor
deleteCalls int
deletedChatID string
deletedMessageID string
}
func (m *mockDeletingMessageEditor) DeleteMessage(_ context.Context, chatID, messageID string) error {
m.deleteCalls++
m.deletedChatID = chatID
m.deletedMessageID = messageID
return nil
}
func (m *mockResolvedToolFeedbackEditor) ToolFeedbackMessageChatID(
chatID string,
outboundCtx *bus.InboundContext,
@@ -1062,6 +1076,101 @@ func TestPreSend_NonToolFeedbackDefersTrackedMessageFinalizationToChannelSend(t
}
}
func TestPreSend_ToolFeedbackSeparateMessagesDeletesPlaceholderAndSkipsEdit(t *testing.T) {
m := newTestManager()
m.config = &config.Config{
Agents: config.AgentsConfig{
Defaults: config.AgentDefaults{
ToolFeedback: config.ToolFeedbackConfig{
Enabled: true,
SeparateMessages: true,
},
},
},
}
ch := &mockDeletingMessageEditor{
mockMessageEditor: mockMessageEditor{
editFn: func(_ context.Context, _, _, _ string) error {
t.Fatal("expected placeholder edit to be skipped in separate message mode")
return nil
},
},
}
m.RecordPlaceholder("test", "123", "456")
msg := testOutboundMessage(bus.OutboundMessage{
Channel: "test",
ChatID: "123",
Content: "hello",
Context: bus.InboundContext{
Channel: "test",
ChatID: "123",
Raw: map[string]string{
"message_kind": "tool_feedback",
},
},
})
msgIDs, handled := m.preSend(context.Background(), "test", msg, ch)
if handled {
t.Fatalf("expected preSend to fall through so the channel can send a new message, got %v", msgIDs)
}
if ch.deleteCalls != 1 {
t.Fatalf("expected placeholder deletion, got %d delete calls", ch.deleteCalls)
}
if ch.deletedChatID != "123" || ch.deletedMessageID != "456" {
t.Fatalf("unexpected placeholder deletion target: %s/%s", ch.deletedChatID, ch.deletedMessageID)
}
if ch.recordedMessageID != "" {
t.Fatalf("expected no tracked placeholder record, got %q", ch.recordedMessageID)
}
if ch.clearedChatID != "123" {
t.Fatalf("expected tracked tool feedback state to be cleared before sending, got %q", ch.clearedChatID)
}
}
func TestPreSend_NonToolFeedbackSeparateMessagesClearsTrackedMessageWithoutDismiss(t *testing.T) {
m := newTestManager()
m.config = &config.Config{
Agents: config.AgentsConfig{
Defaults: config.AgentDefaults{
ToolFeedback: config.ToolFeedbackConfig{
Enabled: true,
SeparateMessages: true,
},
},
},
}
ch := &mockMessageEditor{}
msg := testOutboundMessage(bus.OutboundMessage{
Channel: "test",
ChatID: "123",
Content: "final reply",
Context: bus.InboundContext{
Channel: "test",
ChatID: "123",
},
})
_, handled := m.preSend(context.Background(), "test", msg, ch)
if handled {
t.Fatal("expected preSend to leave final delivery to the channel")
}
if ch.clearedChatID != "123" {
t.Fatalf("expected tracked tool feedback state to be cleared, got %q", ch.clearedChatID)
}
if ch.dismissedChatID != "" {
t.Fatalf("expected tracked tool feedback message to be preserved, got dismissal for %q", ch.dismissedChatID)
}
if ch.finalizeCalled {
t.Fatal("expected separate message mode to skip in-place finalization")
}
}
func TestPreSend_StaleToolFeedbackDoesNotConsumeStreamActiveMarker(t *testing.T) {
m := newTestManager()
m.streamActive.Store("test:123", true)
@@ -1153,6 +1262,38 @@ func TestPreSendMedia_LeavesTrackedMessageForChannelSend(t *testing.T) {
}
}
func TestPreSendMedia_SeparateMessagesClearsTrackedMessageWithoutDismiss(t *testing.T) {
m := newTestManager()
m.config = &config.Config{
Agents: config.AgentsConfig{
Defaults: config.AgentDefaults{
ToolFeedback: config.ToolFeedbackConfig{
Enabled: true,
SeparateMessages: true,
},
},
},
}
ch := &mockMessageEditor{}
m.preSendMedia(context.Background(), "test", bus.OutboundMediaMessage{
ChatID: "123",
Context: bus.InboundContext{
Channel: "test",
ChatID: "123",
},
}, ch)
if ch.clearedChatID != "123" {
t.Fatalf("expected tracked tool feedback state to be cleared before media delivery, got %q", ch.clearedChatID)
}
if ch.dismissedChatID != "" {
t.Fatalf("expected tracked tool feedback message to be preserved"+
" for media delivery, got %q", ch.dismissedChatID)
}
}
func TestSplitOutboundMessageContent_ToolFeedbackTruncatesInsteadOfSplitting(t *testing.T) {
msg := testOutboundMessage(bus.OutboundMessage{
Channel: "test",
@@ -1232,6 +1373,49 @@ func TestGetStreamer_FinalizeDismissesTrackedToolFeedback(t *testing.T) {
}
}
func TestGetStreamer_FinalizeSeparateMessagesClearsTrackedToolFeedback(t *testing.T) {
m := newTestManager()
m.config = &config.Config{
Agents: config.AgentsConfig{
Defaults: config.AgentDefaults{
ToolFeedback: config.ToolFeedbackConfig{
Enabled: true,
SeparateMessages: true,
},
},
},
}
ch := &mockStreamingChannel{
mockMessageEditor: mockMessageEditor{},
streamer: &mockStreamer{
finalizeFn: func(_ context.Context, content string) error {
if content != "final reply" {
t.Fatalf("unexpected finalize content: %q", content)
}
return nil
},
},
}
m.channels["test"] = ch
streamer, ok := m.GetStreamer(context.Background(), "test", "123")
if !ok {
t.Fatal("expected streamer to be available")
}
if err := streamer.Finalize(context.Background(), "final reply"); err != nil {
t.Fatalf("Finalize() error = %v", err)
}
if ch.clearedChatID != "123" {
t.Fatalf("expected tracked tool feedback to be cleared for chat 123, got %q", ch.clearedChatID)
}
if ch.dismissedChatID != "" {
t.Fatalf("expected tracked tool feedback message to be preserved, got dismissal for %q", ch.dismissedChatID)
}
if _, ok := m.streamActive.Load("test:123"); !ok {
t.Fatal("expected streamActive marker to be recorded after finalize")
}
}
func TestGetStreamer_FinalizeDismissesResolvedTrackedToolFeedback(t *testing.T) {
m := newTestManager()
ch := &mockStreamingChannel{
pkg/config/config.go
+10 -2
View File
@@ -247,8 +247,9 @@ type SubTurnConfig struct {
}
type ToolFeedbackConfig struct {
Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"`
MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"`
Enabled bool `json:"enabled" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_ENABLED"`
MaxArgsLength int `json:"max_args_length" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_MAX_ARGS_LENGTH"`
SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"`
}
type AgentDefaults struct {
@@ -299,6 +300,13 @@ func (d *AgentDefaults) IsToolFeedbackEnabled() bool {
return d.ToolFeedback.Enabled
}
// IsToolFeedbackSeparateMessagesEnabled returns true when each tool feedback
// update should be sent as its own chat message instead of editing a single
// in-place progress message.
func (d *AgentDefaults) IsToolFeedbackSeparateMessagesEnabled() bool {
return d.ToolFeedback.SeparateMessages
}
// GetModelName returns the effective model name for the agent defaults.
// It prefers the new "model_name" field but falls back to "model" for backward compatibility.
func (d *AgentDefaults) GetModelName() string {
pkg/config/config_test.go
+6
View File
@@ -787,6 +787,9 @@ func TestDefaultConfig_ToolFeedbackDisabled(t *testing.T) {
if cfg.Agents.Defaults.ToolFeedback.Enabled {
t.Fatal("DefaultConfig().Agents.Defaults.ToolFeedback.Enabled should be false")
}
if cfg.Agents.Defaults.ToolFeedback.SeparateMessages {
t.Fatal("DefaultConfig().Agents.Defaults.ToolFeedback.SeparateMessages should be false")
}
}
func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) {
@@ -807,6 +810,9 @@ func TestLoadConfig_ToolFeedbackDefaultsFalseWhenUnset(t *testing.T) {
if cfg.Agents.Defaults.ToolFeedback.Enabled {
t.Fatal("agents.defaults.tool_feedback.enabled should remain false when unset in config file")
}
if cfg.Agents.Defaults.ToolFeedback.SeparateMessages {
t.Fatal("agents.defaults.tool_feedback.separate_messages should remain false when unset in config file")
}
}
func TestLoadConfig_WebPreferNativeDefaultsTrueWhenUnset(t *testing.T) {
pkg/config/defaults.go
+3 -2
View File
@@ -35,8 +35,9 @@ func DefaultConfig() *Config {
SummarizeTokenPercent: 75,
SteeringMode: "one-at-a-time",
ToolFeedback: ToolFeedbackConfig{
Enabled: false,
MaxArgsLength: 300,
Enabled: false,
MaxArgsLength: 300,
SeparateMessages: false,
},
SplitOnMarker: false,
},