fix(dingtalk): encode conversation type in session key to fix proactive routing

ReconstructReplyCtx hard-coded isGroup=true, causing 1:1 sessions with a non-empty conversationId to be incorrectly routed to groupMessages/send. Changes: - Encode conversation type ('g' for group, 'd' for direct) in session key format: dingtalk:{convType}:{conversationId}[:{senderStaffId}] - Parse convType in ReconstructReplyCtx to correctly set isGroup - Add unit tests for session-key parsing, reply-quote formatting, and proactive routing (group vs 1:1) Fixes: chenhg5/cc-connect#702 review feedback
2026-07-08 07:51:12 +08:00 · 2026-04-22 15:08:52 +08:00
parent e1b5b8ffc6
commit be7537d9bc
2 changed files with 241 additions and 8 deletions
--- a/platform/dingtalk/dingtalk.go
+++ b/platform/dingtalk/dingtalk.go
@@ -233,11 +233,16 @@ func (p *Platform) onMessage(data *chatbot.BotCallbackDataModel, richText *richT
 		return
 	}

+	convType := "d" // direct (1:1)
+	if data.ConversationType == "2" {
+		convType = "g" // group
+	}
+
 	var sessionKey string
 	if p.shareSessionInChannel {
-		sessionKey = fmt.Sprintf("dingtalk:%s", data.ConversationId)
+		sessionKey = fmt.Sprintf("dingtalk:%s:%s", convType, data.ConversationId)
 	} else {
-		sessionKey = fmt.Sprintf("dingtalk:%s:%s", data.ConversationId, data.SenderStaffId)
+		sessionKey = fmt.Sprintf("dingtalk:%s:%s:%s", convType, data.ConversationId, data.SenderStaffId)
 	}

 	// Handle audio messages
@@ -905,29 +910,39 @@ func (p *Platform) formatReplyContent(richText *richTextContent, fallback string
 }

 // ReconstructReplyCtx implements core.ReplyContextReconstructor.
-// Session key format: "dingtalk:{conversationId}:{senderStaffId}" or "dingtalk:{conversationId}"
+// Session key format: "dingtalk:{convType}:{conversationId}:{senderStaffId}" or "dingtalk:{convType}:{conversationId}"
+// where convType is "g" (group) or "d" (direct/1:1).
 func (p *Platform) ReconstructReplyCtx(sessionKey string) (any, error) {
 	if !strings.HasPrefix(sessionKey, "dingtalk:") {
 		return nil, fmt.Errorf("dingtalk: not a dingtalk session key: %q", sessionKey)
 	}

 	stripped := strings.TrimPrefix(sessionKey, "dingtalk:")
-	parts := strings.SplitN(stripped, ":", 2)
+	parts := strings.SplitN(stripped, ":", 3)

-	conversationId := parts[0]
+	if len(parts) < 2 {
+		return nil, fmt.Errorf("dingtalk: invalid session key format: %q", sessionKey)
+	}
+
+	convType := parts[0]
+	if convType != "g" && convType != "d" {
+		return nil, fmt.Errorf("dingtalk: invalid conversation type %q in session key: %q", convType, sessionKey)
+	}
+
+	conversationId := parts[1]
 	if conversationId == "" {
 		return nil, fmt.Errorf("dingtalk: empty conversationId in session key: %q", sessionKey)
 	}

 	var senderStaffId string
-	if len(parts) > 1 {
-		senderStaffId = parts[1]
+	if len(parts) > 2 {
+		senderStaffId = parts[2]
 	}

 	return replyContext{
 		conversationId: conversationId,
 		senderStaffId:  senderStaffId,
-		isGroup:        true,
+		isGroup:        convType == "g",
 		proactive:      true,
 	}, nil
 }
--- a/platform/dingtalk/dingtalk_test.go
+++ b/platform/dingtalk/dingtalk_test.go
@@ -1,6 +1,7 @@
 package dingtalk

 import (
+	"encoding/json"
 	"net/http"
 	"sync"
 	"testing"
@@ -134,3 +135,220 @@ func TestPlatform_AccessTokenFieldsExist(t *testing.T) {

 	t.Log("Platform token caching fields exist and are accessible")
 }
+
+// ──────────────────────────────────────────────────────────────
+// ReconstructReplyCtx tests
+// ──────────────────────────────────────────────────────────────
+
+func TestReconstructReplyCtx_GroupSharedSession(t *testing.T) {
+	p := &Platform{}
+	rctx, err := p.ReconstructReplyCtx("dingtalk:g:conv123")
+	if err != nil {
+		t.Fatalf("ReconstructReplyCtx() error = %v", err)
+	}
+	rc := rctx.(replyContext)
+	if rc.conversationId != "conv123" {
+		t.Errorf("conversationId = %q, want %q", rc.conversationId, "conv123")
+	}
+	if rc.senderStaffId != "" {
+		t.Errorf("senderStaffId = %q, want empty", rc.senderStaffId)
+	}
+	if !rc.isGroup {
+		t.Error("isGroup = false, want true for group session")
+	}
+	if !rc.proactive {
+		t.Error("proactive = false, want true")
+	}
+}
+
+func TestReconstructReplyCtx_GroupPerUserSession(t *testing.T) {
+	p := &Platform{}
+	rctx, err := p.ReconstructReplyCtx("dingtalk:g:conv123:user456")
+	if err != nil {
+		t.Fatalf("ReconstructReplyCtx() error = %v", err)
+	}
+	rc := rctx.(replyContext)
+	if rc.conversationId != "conv123" {
+		t.Errorf("conversationId = %q, want %q", rc.conversationId, "conv123")
+	}
+	if rc.senderStaffId != "user456" {
+		t.Errorf("senderStaffId = %q, want %q", rc.senderStaffId, "user456")
+	}
+	if !rc.isGroup {
+		t.Error("isGroup = false, want true for group session")
+	}
+}
+
+func TestReconstructReplyCtx_DirectSession(t *testing.T) {
+	p := &Platform{}
+	rctx, err := p.ReconstructReplyCtx("dingtalk:d:conv789:user111")
+	if err != nil {
+		t.Fatalf("ReconstructReplyCtx() error = %v", err)
+	}
+	rc := rctx.(replyContext)
+	if rc.conversationId != "conv789" {
+		t.Errorf("conversationId = %q, want %q", rc.conversationId, "conv789")
+	}
+	if rc.senderStaffId != "user111" {
+		t.Errorf("senderStaffId = %q, want %q", rc.senderStaffId, "user111")
+	}
+	if rc.isGroup {
+		t.Error("isGroup = true, want false for direct session")
+	}
+	if !rc.proactive {
+		t.Error("proactive = false, want true")
+	}
+}
+
+func TestReconstructReplyCtx_InvalidPrefix(t *testing.T) {
+	p := &Platform{}
+	_, err := p.ReconstructReplyCtx("telegram:g:conv123")
+	if err == nil {
+		t.Fatal("expected error for non-dingtalk prefix")
+	}
+}
+
+func TestReconstructReplyCtx_InvalidConvType(t *testing.T) {
+	p := &Platform{}
+	_, err := p.ReconstructReplyCtx("dingtalk:x:conv123")
+	if err == nil {
+		t.Fatal("expected error for invalid conversation type")
+	}
+}
+
+func TestReconstructReplyCtx_EmptyConversationId(t *testing.T) {
+	p := &Platform{}
+	_, err := p.ReconstructReplyCtx("dingtalk:g:")
+	if err == nil {
+		t.Fatal("expected error for empty conversationId")
+	}
+}
+
+func TestReconstructReplyCtx_TooFewParts(t *testing.T) {
+	p := &Platform{}
+	_, err := p.ReconstructReplyCtx("dingtalk:")
+	if err == nil {
+		t.Fatal("expected error for too few parts")
+	}
+}
+
+// ──────────────────────────────────────────────────────────────
+// formatReplyContent tests
+// ──────────────────────────────────────────────────────────────
+
+func TestFormatReplyContent_WithQuotedText(t *testing.T) {
+	p := &Platform{}
+	repliedContent, _ := json.Marshal(repliedTextContent{Text: "original message"})
+	richText := &richTextContent{
+		Content:    "user reply",
+		IsReplyMsg: true,
+		RepliedMsg: &repliedMessage{
+			MsgType: "text",
+			Content: repliedContent,
+		},
+	}
+	result := p.formatReplyContent(richText, "fallback")
+	expected := "引用: \"original message\"\n\nuser reply"
+	if result != expected {
+		t.Errorf("formatReplyContent() = %q, want %q", result, expected)
+	}
+}
+
+func TestFormatReplyContent_EmptyContent_UsesFallback(t *testing.T) {
+	p := &Platform{}
+	repliedContent, _ := json.Marshal(repliedTextContent{Text: "quoted"})
+	richText := &richTextContent{
+		Content:    "",
+		IsReplyMsg: true,
+		RepliedMsg: &repliedMessage{
+			MsgType: "text",
+			Content: repliedContent,
+		},
+	}
+	result := p.formatReplyContent(richText, "fallback text")
+	expected := "引用: \"quoted\"\n\nfallback text"
+	if result != expected {
+		t.Errorf("formatReplyContent() = %q, want %q", result, expected)
+	}
+}
+
+func TestFormatReplyContent_NilRepliedMsg(t *testing.T) {
+	p := &Platform{}
+	richText := &richTextContent{
+		Content:    "just a message",
+		IsReplyMsg: true,
+		RepliedMsg: nil,
+	}
+	result := p.formatReplyContent(richText, "fallback")
+	if result != "just a message" {
+		t.Errorf("formatReplyContent() = %q, want %q", result, "just a message")
+	}
+}
+
+func TestFormatReplyContent_NonTextMsgType(t *testing.T) {
+	p := &Platform{}
+	richText := &richTextContent{
+		Content:    "user reply",
+		IsReplyMsg: true,
+		RepliedMsg: &repliedMessage{
+			MsgType: "image",
+			Content: json.RawMessage(`{}`),
+		},
+	}
+	result := p.formatReplyContent(richText, "fallback")
+	if result != "user reply" {
+		t.Errorf("formatReplyContent() = %q, want %q", result, "user reply")
+	}
+}
+
+func TestFormatReplyContent_EmptyQuotedText(t *testing.T) {
+	p := &Platform{}
+	repliedContent, _ := json.Marshal(repliedTextContent{Text: ""})
+	richText := &richTextContent{
+		Content:    "user reply",
+		IsReplyMsg: true,
+		RepliedMsg: &repliedMessage{
+			MsgType: "text",
+			Content: repliedContent,
+		},
+	}
+	result := p.formatReplyContent(richText, "fallback")
+	if result != "user reply" {
+		t.Errorf("formatReplyContent() = %q, want %q", result, "user reply")
+	}
+}
+
+// ──────────────────────────────────────────────────────────────
+// Proactive routing tests
+// ──────────────────────────────────────────────────────────────
+
+func TestProactiveRouting_GroupSessionUsesGroupAPI(t *testing.T) {
+	// Verify that a group session key produces a replyContext with isGroup=true,
+	// which sendProactiveMessage would route to groupMessages/send.
+	p := &Platform{}
+	rctx, err := p.ReconstructReplyCtx("dingtalk:g:conv123:user456")
+	if err != nil {
+		t.Fatalf("ReconstructReplyCtx() error = %v", err)
+	}
+	rc := rctx.(replyContext)
+	if !rc.isGroup || rc.conversationId == "" {
+		t.Errorf("group routing: isGroup=%v, conversationId=%q; want isGroup=true with non-empty conversationId", rc.isGroup, rc.conversationId)
+	}
+}
+
+func TestProactiveRouting_DirectSessionUsesDirectAPI(t *testing.T) {
+	// Verify that a direct session key produces a replyContext with isGroup=false,
+	// which sendProactiveMessage would route to oToMessages/batchSend.
+	p := &Platform{}
+	rctx, err := p.ReconstructReplyCtx("dingtalk:d:conv789:user111")
+	if err != nil {
+		t.Fatalf("ReconstructReplyCtx() error = %v", err)
+	}
+	rc := rctx.(replyContext)
+	if rc.isGroup {
+		t.Error("direct routing: isGroup=true, want false for 1:1 session")
+	}
+	if rc.senderStaffId != "user111" {
+		t.Errorf("direct routing: senderStaffId=%q, want %q", rc.senderStaffId, "user111")
+	}
+}