fix(dingtalk): honor mention-only groups and strip leading mentions (#2054)

* fix(dingtalk): honor @mention flag in mention-only groups

* fix(dingtalk): strip leading mentions in group payloads

---------

Co-authored-by: Alix-007 <267018309+Alix-007@users.noreply.github.com>

pkg/channels/dingtalk/dingtalk.go

@@ -6,6 +6,7 @@ package dingtalk
 import (
 	"context"
 	"fmt"
+	"strings"
 	"sync"
 
 	"github.com/open-dingtalk/dingtalk-stream-sdk-go/chatbot"
@@ -135,13 +136,17 @@ func (c *DingTalkChannel) onChatBotMessageReceived(
 	ctx context.Context,
 	data *chatbot.BotCallbackDataModel,
 ) ([]byte, error) {
+	if data == nil {
+		return nil, nil
+	}
+
 	// Extract message content from Text field
-	content := data.Text.Content
+	content := strings.TrimSpace(data.Text.Content)
 	if content == "" {
 		// Try to extract from Content interface{} if Text is empty
 		if contentMap, ok := data.Content.(map[string]any); ok {
 			if textContent, ok := contentMap["content"].(string); ok {
-				content = textContent
+				content = strings.TrimSpace(textContent)
 			}
 		}
 	}
@@ -150,12 +155,19 @@ func (c *DingTalkChannel) onChatBotMessageReceived(
 		return nil, nil // Ignore empty messages
 	}
 
-	senderID := data.SenderStaffId
-	senderNick := data.SenderNick
-	chatID := senderID
-	if data.ConversationType != "1" {
-		// For group chats
-		chatID = data.ConversationId
+	senderID := strings.TrimSpace(data.SenderStaffId)
+	if senderID == "" {
+		senderID = strings.TrimSpace(data.SenderId)
+	}
+	senderNick := strings.TrimSpace(data.SenderNick)
+
+	chatID := strings.TrimSpace(data.ConversationId)
+	if chatID == "" && data.ConversationType == "1" {
+		// Fallback for direct chats when conversation_id is absent.
+		chatID = senderID
+	}
+	if chatID == "" {
+		return nil, nil
 	}
 
 	// Store the session webhook for this chat so we can reply later
@@ -171,11 +183,19 @@ func (c *DingTalkChannel) onChatBotMessageReceived(
 
 	var peer bus.Peer
 	if data.ConversationType == "1" {
-		peer = bus.Peer{Kind: "direct", ID: senderID}
+		peerID := senderID
+		if peerID == "" {
+			peerID = chatID
+		}
+		peer = bus.Peer{Kind: "direct", ID: peerID}
 	} else {
 		peer = bus.Peer{Kind: "group", ID: data.ConversationId}
+		isMentioned := data.IsInAtList
+		if isMentioned {
+			content = stripLeadingAtMentions(content)
+		}
 		// In group chats, apply unified group trigger filtering
-		respond, cleaned := c.ShouldRespondInGroup(false, content)
+		respond, cleaned := c.ShouldRespondInGroup(isMentioned, content)
 		if !respond {
 			return nil, nil
 		}
@@ -189,10 +209,18 @@ func (c *DingTalkChannel) onChatBotMessageReceived(
 	})
 
 	// Build sender info
+	platformID := senderID
+	if platformID == "" {
+		platformID = chatID
+	}
+	resolvedSenderID := senderID
+	if resolvedSenderID == "" {
+		resolvedSenderID = platformID
+	}
 	sender := bus.SenderInfo{
 		Platform:    "dingtalk",
-		PlatformID:  senderID,
-		CanonicalID: identity.BuildCanonicalID("dingtalk", senderID),
+		PlatformID:  platformID,
+		CanonicalID: identity.BuildCanonicalID("dingtalk", platformID),
 		DisplayName: senderNick,
 	}
 
@@ -201,7 +229,7 @@ func (c *DingTalkChannel) onChatBotMessageReceived(
 	}
 
 	// Handle the message through the base channel
-	c.HandleMessage(ctx, peer, "", senderID, chatID, content, nil, metadata, sender)
+	c.HandleMessage(ctx, peer, "", resolvedSenderID, chatID, content, nil, metadata, sender)
 
 	// Return nil to indicate we've handled the message asynchronously
 	// The response will be sent through the message bus
@@ -229,3 +257,19 @@ func (c *DingTalkChannel) SendDirectReply(ctx context.Context, sessionWebhook, c
 
 	return nil
 }
+
+func stripLeadingAtMentions(content string) string {
+	fields := strings.Fields(content)
+	if len(fields) == 0 {
+		return ""
+	}
+
+	i := 0
+	for i < len(fields) && strings.HasPrefix(fields[i], "@") {
+		i++
+	}
+	if i == 0 {
+		return strings.TrimSpace(content)
+	}
+	return strings.Join(fields[i:], " ")
+}

pkg/channels/dingtalk/dingtalk_test.go

@@ -0,0 +1,131 @@
+package dingtalk
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/open-dingtalk/dingtalk-stream-sdk-go/chatbot"
+
+	"github.com/sipeed/picoclaw/pkg/bus"
+	"github.com/sipeed/picoclaw/pkg/config"
+)
+
+func newTestDingTalkChannel(t *testing.T, cfg config.DingTalkConfig) (*DingTalkChannel, *bus.MessageBus) {
+	t.Helper()
+
+	if cfg.ClientID == "" {
+		cfg.ClientID = "test-client-id"
+	}
+	if cfg.ClientSecret() == "" {
+		cfg.SetClientSecret("test-client-secret")
+	}
+
+	msgBus := bus.NewMessageBus()
+	ch, err := NewDingTalkChannel(cfg, msgBus)
+	if err != nil {
+		t.Fatalf("new channel: %v", err)
+	}
+	return ch, msgBus
+}
+
+func mustReceiveInbound(t *testing.T, msgBus *bus.MessageBus) bus.InboundMessage {
+	t.Helper()
+	select {
+	case msg := <-msgBus.InboundChan():
+		return msg
+	case <-time.After(time.Second):
+		t.Fatal("expected inbound message")
+		return bus.InboundMessage{}
+	}
+}
+
+func TestOnChatBotMessageReceived_GroupMentionOnlyUsesIsInAtListAndStripsMention(t *testing.T) {
+	ch, msgBus := newTestDingTalkChannel(t, config.DingTalkConfig{
+		GroupTrigger: config.GroupTriggerConfig{MentionOnly: true},
+	})
+
+	_, err := ch.onChatBotMessageReceived(context.Background(), &chatbot.BotCallbackDataModel{
+		Text:             chatbot.BotCallbackDataTextModel{Content: "  @bot /help  "},
+		SenderStaffId:    "staff-123",
+		SenderNick:       "Alice",
+		ConversationType: "2",
+		ConversationId:   "group-abc",
+		SessionWebhook:   "https://example.com/webhook",
+		IsInAtList:       true,
+	})
+	if err != nil {
+		t.Fatalf("handler returned error: %v", err)
+	}
+
+	inbound := mustReceiveInbound(t, msgBus)
+	if inbound.Channel != "dingtalk" {
+		t.Fatalf("channel=%q", inbound.Channel)
+	}
+	if inbound.ChatID != "group-abc" {
+		t.Fatalf("chat_id=%q", inbound.ChatID)
+	}
+	if inbound.Peer.Kind != "group" || inbound.Peer.ID != "group-abc" {
+		t.Fatalf("peer=%+v", inbound.Peer)
+	}
+	if inbound.Content != "/help" {
+		t.Fatalf("content=%q", inbound.Content)
+	}
+}
+
+func TestOnChatBotMessageReceived_DirectFallbackSenderIDUsesConversationID(t *testing.T) {
+	ch, msgBus := newTestDingTalkChannel(t, config.DingTalkConfig{})
+
+	_, err := ch.onChatBotMessageReceived(context.Background(), &chatbot.BotCallbackDataModel{
+		Text:             chatbot.BotCallbackDataTextModel{Content: "ping"},
+		SenderStaffId:    "",
+		SenderId:         "openid-user-42",
+		SenderNick:       "Bob",
+		ConversationType: "1",
+		ConversationId:   "conv-direct-42",
+		SessionWebhook:   "https://example.com/webhook-direct",
+	})
+	if err != nil {
+		t.Fatalf("handler returned error: %v", err)
+	}
+
+	inbound := mustReceiveInbound(t, msgBus)
+	if inbound.ChatID != "conv-direct-42" {
+		t.Fatalf("chat_id=%q", inbound.ChatID)
+	}
+	if inbound.Peer.Kind != "direct" || inbound.Peer.ID != "openid-user-42" {
+		t.Fatalf("peer=%+v", inbound.Peer)
+	}
+	if inbound.SenderID != "dingtalk:openid-user-42" {
+		t.Fatalf("sender_id=%q", inbound.SenderID)
+	}
+
+	if _, ok := ch.sessionWebhooks.Load("conv-direct-42"); !ok {
+		t.Fatal("expected session webhook keyed by conversation_id")
+	}
+	if _, ok := ch.sessionWebhooks.Load(""); ok {
+		t.Fatal("unexpected empty chat_id webhook key")
+	}
+}
+
+func TestStripLeadingAtMentions(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		wantOut string
+	}{
+		{name: "single mention and command", input: "@bot /help", wantOut: "/help"},
+		{name: "multiple mentions", input: "@bot @alice /new", wantOut: "/new"},
+		{name: "no mention", input: "/help", wantOut: "/help"},
+		{name: "mention only", input: "@bot", wantOut: ""},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := stripLeadingAtMentions(tt.input)
+			if got != tt.wantOut {
+				t.Fatalf("stripLeadingAtMentions(%q)=%q want=%q", tt.input, got, tt.wantOut)
+			}
+		})
+	}
+}