From d460c90403197b5a8a5d0029bfd483354dbdecb4 Mon Sep 17 00:00:00 2001 From: Duc Nguyen <87276974+itsddvn@users.noreply.github.com> Date: Sun, 28 Jun 2026 00:25:49 +0700 Subject: [PATCH] fix(feishu): gate group pairing by target bot (#1291) * fix(feishu): require exact bot_open_id match for mention detection - Previously, when bot_open_id was empty, ALL mentions were treated as bot mentions - This caused multiple agents in same group to all respond to any mention - Now requires bot_open_id to be set AND match exactly for mentionedBot=true - Fixes issue where CPPAI PM would respond even when other agent was mentioned * fix(feishu): gate group pairing by target bot --------- Co-authored-by: ntduc --- docs/project-changelog.md | 30 +++ internal/channels/feishu/bot.go | 136 ++++++++--- internal/channels/feishu/bot_parse.go | 28 ++- internal/channels/feishu/bot_parse_test.go | 34 ++- internal/channels/feishu/bot_policy.go | 146 ++++++++++- internal/channels/feishu/bot_policy_test.go | 229 ++++++++++++++++++ internal/channels/feishu/feishu.go | 96 +++++++- .../channels/feishu/feishu_lifecycle_test.go | 85 +++++++ 8 files changed, 728 insertions(+), 56 deletions(-) diff --git a/docs/project-changelog.md b/docs/project-changelog.md index d6070edf..a4632cd7 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -4,6 +4,36 @@ Significant changes, features, and fixes in reverse chronological order. --- +## 2026-06-27 + +### Feishu/Lark group pairing stability + +**Fixes** + +- Group pairing now only runs after the target bot is actually mentioned, so + other agents in the same Lark group stay silent and do not create pair codes. +- Feishu group messages with explicit mentions that do not match the current + bot are now dropped before media resolution, policy checks, pairing, or agent + routing, even when `require_mention` is disabled for the channel. +- Feishu WebSocket/webhook message events are now guarded by `header.app_id` + before dispatching to a channel instance, preventing app-routing mismatch from + reaching pairing logic. +- Feishu now re-checks pairing state immediately before sending a pair code, + preventing stale policy decisions from creating duplicate requests. +- Feishu pairing and bot identity logs now include channel/chat/tenant context so + multi-agent Lark groups can be diagnosed without inferring from log order. + +**Tests** + +- Added Feishu regressions for non-target mentions, target mentions, and unknown + bot identity fail-closed behavior. +- Added Feishu WebSocket regressions for app-id mismatch vs match handling. +- Added Feishu regression for non-target mentions when `require_mention` is + disabled. +- Added Feishu regression coverage for already-paired group mentions. + +--- + ## 2026-06-13 ### Cron NO_REPLY delivery suppression (issue #141) diff --git a/internal/channels/feishu/bot.go b/internal/channels/feishu/bot.go index 1918c763..4bfeb537 100644 --- a/internal/channels/feishu/bot.go +++ b/internal/channels/feishu/bot.go @@ -16,33 +16,43 @@ import ( // messageContext holds parsed information from a Feishu message event. type messageContext struct { - ChatID string - MessageID string - SenderID string // sender_id.open_id - ChatType string // "p2p" or "group" - Content string - ContentType string // "text", "post", "image", etc. + ChatID string + MessageID string + SenderID string // sender_id.open_id + ChatType string // "p2p" or "group" + Content string + ContentType string // "text", "post", "image", etc. MentionedBot bool - RootID string // reply-chain root (populated on ANY reply, incl. plain quote reply) - ParentID string // direct parent in reply chain - ThreadID string // set ONLY when message is inside an actual topic thread - Mentions []mentionInfo + RootID string // reply-chain root (populated on ANY reply, incl. plain quote reply) + ParentID string // direct parent in reply chain + ThreadID string // set ONLY when message is inside an actual topic thread + Mentions []mentionInfo } type mentionInfo struct { - Key string // @_user_N placeholder - OpenID string - Name string + Key string // @_user_N placeholder + OpenID string + UserID string + UnionID string + Name string + TenantKey string } // handleMessageEvent processes an incoming Feishu message event. func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { + c.handleMessageEventFrom(ctx, event, "direct") +} + +func (c *Channel) handleMessageEventFrom(ctx context.Context, event *MessageEvent, source string) { // Inject tenant scope so store queries filter by the correct tenant_id. ctx = store.WithTenantID(ctx, c.TenantID()) if event == nil { return } + if !c.shouldProcessMessageEvent(event, source) { + return + } msg := &event.Event.Message sender := &event.Event.Sender @@ -63,6 +73,7 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { if mc == nil { return } + c.logParsedMessage(event, mc, source) // 2a. Slash commands in DMs are rejected early with a clear hint so // they never reach the agent pipeline (otherwise users typing @@ -73,6 +84,21 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { return } + if mc.ChatType == "group" && len(mc.Mentions) > 0 && !mc.MentionedBot { + slog.Info("feishu group message skipped; explicit mention is not this bot", + "source", source, + "decision", "skip_non_target_mention", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "chat_id", mc.ChatID, + "sender_id", mc.SenderID, + "bot_open_id", c.botOpenID, + "mention_count", len(mc.Mentions), + ) + return + } + // 3. Resolve sender name (cached) senderName := c.resolveSenderName(ctx, mc.SenderID) @@ -96,24 +122,45 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { // 5. Group policy if mc.ChatType == "group" { - if !c.checkGroupPolicy(ctx, mc.SenderID, mc.ChatID) { - slog.Debug("feishu group message rejected by policy", "sender_id", mc.SenderID, "chat_id", mc.ChatID) - return - } + isWriterCommand := c.isWriterSlashCommand(mc) - // 5a. Writer management slash commands run AFTER the group policy - // gate so commands cannot bypass allowlists or pairing. Commands - // short-circuit the agent pipeline to avoid consuming LLM tokens. - if c.maybeHandleWriterCommand(ctx, mc) { - return - } - - // 6. RequireMention check — record to history if not mentioned + // 5a. RequireMention check runs before pairing for normal messages so + // non-target bots stay silent in multi-agent groups. Do not create a + // pairing request for messages that did not mention this bot. requireMention := true if c.cfg.RequireMention != nil { requireMention = *c.cfg.RequireMention } - if requireMention && !mc.MentionedBot { + slog.Debug("feishu group mention gate", + "source", source, + "decision", "evaluate_group_gate", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "chat_id", mc.ChatID, + "sender_id", mc.SenderID, + "bot_open_id", c.botOpenID, + "mentioned_bot", mc.MentionedBot, + "mention_count", len(mc.Mentions), + "mentions", formatMentionInfos(mc.Mentions), + "require_mention", requireMention, + "group_policy", c.cfg.GroupPolicy, + "is_writer_command", isWriterCommand, + ) + if !isWriterCommand && requireMention && !mc.MentionedBot { + if !c.canRecordUnmentionedGroupMessage(ctx, mc.SenderID, mc.ChatID) { + slog.Debug("feishu group message skipped; no bot mention and policy not approved", + "source", source, + "decision", "skip_no_mention_unapproved", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "chat_id", mc.ChatID, + "sender_id", mc.SenderID, + ) + return + } + historyKey := mc.ChatID if mc.RootID != "" && c.cfg.TopicSessionMode == "enabled" { historyKey = fmt.Sprintf("%s:topic:%s", mc.ChatID, mc.RootID) @@ -132,11 +179,37 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { cc.EnsureContact(ctx, c.Type(), c.Name(), mc.SenderID, mc.SenderID, senderName, "", "group", "user", "", "") } - slog.Debug("feishu group message recorded (no mention)", - "chat_id", mc.ChatID, "sender", senderName, + slog.Debug("feishu group message recorded without bot mention", + "source", source, + "decision", "record_no_mention_history", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "chat_id", mc.ChatID, + "sender", senderName, ) return } + + if !c.checkGroupPolicy(ctx, mc.SenderID, mc.ChatID) { + slog.Debug("feishu group message rejected by policy", + "source", source, + "decision", "reject_group_policy", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "sender_id", mc.SenderID, + "chat_id", mc.ChatID, + ) + return + } + + // 5b. Writer management slash commands run AFTER the group policy + // gate so commands cannot bypass allowlists or pairing. Commands + // short-circuit the agent pipeline to avoid consuming LLM tokens. + if isWriterCommand && c.maybeHandleWriterCommand(ctx, mc) { + return + } } // 6. DM policy (pairing flow) @@ -179,7 +252,12 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { chatID = fmt.Sprintf("%s:topic:%s", mc.ChatID, mc.RootID) } - slog.Debug("feishu message received", + slog.Debug("feishu message accepted", + "source", source, + "decision", "publish_inbound", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, "sender_id", mc.SenderID, "sender_name", senderName, "chat_id", chatID, diff --git a/internal/channels/feishu/bot_parse.go b/internal/channels/feishu/bot_parse.go index ee3dfbda..e3fcceac 100644 --- a/internal/channels/feishu/bot_parse.go +++ b/internal/channels/feishu/bot_parse.go @@ -31,16 +31,18 @@ func (c *Channel) parseMessageEvent(event *MessageEvent) *messageContext { mentionedBot := false for _, m := range msg.Mentions { mi := mentionInfo{ - Key: m.Key, - OpenID: m.ID.OpenID, - Name: m.Name, + Key: m.Key, + OpenID: m.ID.OpenID, + UserID: m.ID.UserID, + UnionID: m.ID.UnionID, + Name: m.Name, + TenantKey: m.TenantKey, } mentions = append(mentions, mi) - // Check if bot is mentioned. - // If botOpenID is known, match exactly; otherwise treat any mention as bot mention - // (fallback when probeBotInfo fails — better to process than silently drop). - if c.botOpenID == "" || mi.OpenID == c.botOpenID { + // Check if this bot is mentioned. Unknown bot identity must fail closed + // so multi-agent groups do not all respond to another bot's mention. + if c.botOpenID != "" && mi.OpenID == c.botOpenID { mentionedBot = true } } @@ -252,3 +254,15 @@ func resolveMentions(text string, mentions []mentionInfo, botOpenID string) stri } return strings.TrimSpace(text) } + +func formatMentionInfos(mentions []mentionInfo) []string { + if len(mentions) == 0 { + return nil + } + out := make([]string, 0, len(mentions)) + for _, m := range mentions { + out = append(out, fmt.Sprintf("key=%s open_id=%s user_id=%s union_id=%s name=%s tenant=%s", + m.Key, m.OpenID, m.UserID, m.UnionID, m.Name, m.TenantKey)) + } + return out +} diff --git a/internal/channels/feishu/bot_parse_test.go b/internal/channels/feishu/bot_parse_test.go index 745f3ebc..792e8f00 100644 --- a/internal/channels/feishu/bot_parse_test.go +++ b/internal/channels/feishu/bot_parse_test.go @@ -8,9 +8,9 @@ import ( func TestParseMessageContent_Text(t *testing.T) { cases := []struct { - name string - raw string - want string + name string + raw string + want string }{ {"normal text", `{"text":"hello world"}`, "hello world"}, {"empty raw", "", ""}, @@ -214,10 +214,8 @@ func TestResolveMentions_NoBotID_AllMentionsReplacedWithName(t *testing.T) { {Key: "@_user_2", OpenID: "ou_B", Name: "Bob"}, } got := resolveMentions(text, mentions, "") - // botOpenID is empty: all mentions treated as bot → stripped - // (because the condition is: if botOpenID != "" && openID == botOpenID → strip, else → replace with name) - // When botOpenID == "", neither branch strips unless openID matches empty string. - // Both get replaced with @Name. + // botOpenID is empty: no mention is treated as this bot, so all user + // mentions are preserved as readable names. if got != "@Alice and @Bob" { t.Errorf("got %q, want %q", got, "@Alice and @Bob") } @@ -276,6 +274,28 @@ func TestParseMessageEvent_BotMentioned(t *testing.T) { } } +func TestParseMessageEvent_UnknownBotIDDoesNotTreatAnyMentionAsBot(t *testing.T) { + ch := &Channel{} + ev := &MessageEvent{} + ev.Event.Message.MessageType = "text" + ev.Event.Message.Content = `{"text":"@_user_1 please help"}` + ev.Event.Message.Mentions = []EventMention{ + {Key: "@_user_1", ID: struct { + OpenID string `json:"open_id"` + UserID string `json:"user_id"` + UnionID string `json:"union_id"` + }{OpenID: "ou_other_bot"}, Name: "OtherBot"}, + } + + mc := ch.parseMessageEvent(ev) + if mc.MentionedBot { + t.Fatal("MentionedBot must be false when bot open_id is unknown") + } + if mc.Content != "@OtherBot please help" { + t.Fatalf("content = %q, want other mention preserved", mc.Content) + } +} + func TestParseMessageEvent_NoSender(t *testing.T) { ch := &Channel{} ev := &MessageEvent{} diff --git a/internal/channels/feishu/bot_policy.go b/internal/channels/feishu/bot_policy.go index 151f787f..4cdc000e 100644 --- a/internal/channels/feishu/bot_policy.go +++ b/internal/channels/feishu/bot_policy.go @@ -63,6 +63,7 @@ func (c *Channel) checkGroupPolicy(ctx context.Context, senderID, chatID string) if groupPolicy == "" { groupPolicy = "open" } + channelName := c.channelNameForLog() switch groupPolicy { case "disabled": @@ -72,15 +73,30 @@ func (c *Channel) checkGroupPolicy(ctx context.Context, senderID, chatID string) case "pairing": // Feishu groupAllowList bypass: per-user sender allowlist specific to Feishu. if c.isInGroupAllowList(senderID) { + slog.Debug("feishu group policy allowed by group allowlist", + "channel", channelName, + "sender_id", senderID, + "chat_id", chatID, + "group_policy", groupPolicy, + ) return true } // Delegate remaining pairing logic to BaseChannel (handles allowList, approvedGroups, DB check). result := c.CheckGroupPolicy(ctx, senderID, chatID, groupPolicy) + groupSenderID := fmt.Sprintf("group:%s", chatID) + slog.Debug("feishu group policy checked", + "channel", channelName, + "sender_id", senderID, + "group_sender_id", groupSenderID, + "chat_id", chatID, + "group_policy", groupPolicy, + "result", policyResultLabel(result), + "tenant_id", c.TenantID(), + ) switch result { case channels.PolicyAllow: return true case channels.PolicyNeedsPairing: - groupSenderID := fmt.Sprintf("group:%s", chatID) c.sendPairingReply(ctx, groupSenderID, chatID) return false default: @@ -91,6 +107,38 @@ func (c *Channel) checkGroupPolicy(ctx context.Context, senderID, chatID string) } } +func (c *Channel) canRecordUnmentionedGroupMessage(ctx context.Context, senderID, chatID string) bool { + groupPolicy := c.cfg.GroupPolicy + if groupPolicy == "" { + groupPolicy = "open" + } + channelName := c.channelNameForLog() + + switch groupPolicy { + case "disabled": + return false + case "allowlist": + return c.IsAllowed(senderID) || c.isInGroupAllowList(senderID) + case "pairing": + if c.isInGroupAllowList(senderID) { + return true + } + result := c.CheckGroupPolicy(ctx, senderID, chatID, groupPolicy) + slog.Debug("feishu unmentioned group history policy checked", + "channel", channelName, + "sender_id", senderID, + "group_sender_id", fmt.Sprintf("group:%s", chatID), + "chat_id", chatID, + "group_policy", groupPolicy, + "result", policyResultLabel(result), + "tenant_id", c.TenantID(), + ) + return result == channels.PolicyAllow + default: + return true + } +} + func (c *Channel) checkDMPolicy(ctx context.Context, senderID, chatID string) bool { result := c.CheckDMPolicy(ctx, senderID, c.cfg.DMPolicy) switch result { @@ -110,14 +158,61 @@ func (c *Channel) sendPairingReply(ctx context.Context, senderID, chatID string) if ps == nil { return } + channelName := c.channelNameForLog() + + paired, err := ps.IsPaired(ctx, senderID, channelName) + if err != nil { + slog.Warn("security.pairing_check_failed, denying pairing request (fail-closed)", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "error", err, + ) + return + } + if paired { + if strings.HasPrefix(senderID, "group:") { + c.MarkGroupApproved(chatID) + } + slog.Debug("feishu pairing reply skipped; sender already paired", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + ) + return + } + slog.Debug("feishu pairing reply gate needs pairing", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + ) if !c.CanSendPairingNotif(senderID, pairingDebounceTime) { + slog.Debug("feishu pairing reply skipped by debounce", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + ) return } - code, err := ps.RequestPairing(ctx, senderID, c.Name(), chatID, "default", nil) + code, err := ps.RequestPairing(ctx, senderID, channelName, chatID, "default", nil) if err != nil { - slog.Debug("feishu pairing request failed", "sender_id", senderID, "error", err) + slog.Debug("feishu pairing request failed", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + "error", err, + ) return } @@ -128,9 +223,50 @@ func (c *Channel) sendPairingReply(ctx context.Context, senderID, chatID string) receiveIDType := resolveReceiveIDType(chatID) if err := c.sendText(context.Background(), chatID, receiveIDType, replyText, ""); err != nil { - slog.Warn("failed to send feishu pairing reply", "error", err) + slog.Warn("failed to send feishu pairing reply", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + "error", err, + ) } else { c.MarkPairingNotifSent(senderID) - slog.Info("feishu pairing reply sent", "sender_id", senderID, "code", code) + slog.Info("feishu pairing reply sent", + "sender_id", senderID, + "channel", channelName, + "chat_id", chatID, + "tenant_id", c.TenantID(), + "source", pairingReplySource(senderID), + "code", code, + ) + } +} + +func (c *Channel) channelNameForLog() string { + if c == nil || c.BaseChannel == nil { + return "" + } + return c.Name() +} + +func pairingReplySource(senderID string) string { + if strings.HasPrefix(senderID, "group:") { + return "group_policy" + } + return "dm_policy" +} + +func policyResultLabel(result channels.PolicyResult) string { + switch result { + case channels.PolicyAllow: + return "allow" + case channels.PolicyDeny: + return "deny" + case channels.PolicyNeedsPairing: + return "needs_pairing" + default: + return fmt.Sprintf("unknown_%d", result) } } diff --git a/internal/channels/feishu/bot_policy_test.go b/internal/channels/feishu/bot_policy_test.go index d661c0a1..2c8ecdba 100644 --- a/internal/channels/feishu/bot_policy_test.go +++ b/internal/channels/feishu/bot_policy_test.go @@ -2,7 +2,14 @@ package feishu import ( "context" + "encoding/json" + "errors" "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/store" ) // --- isInGroupAllowList --- @@ -73,6 +80,228 @@ func TestCheckGroupPolicy_Pairing_GroupAllowListBypasses(t *testing.T) { } } +type feishuPolicyPairingStore struct { + requests int + lastSenderID string + lastChannel string + lastChatID string + paired map[string]bool +} + +func (s *feishuPolicyPairingStore) RequestPairing(_ context.Context, senderID, channel, chatID, _ string, _ map[string]string) (string, error) { + s.requests++ + s.lastSenderID = senderID + s.lastChannel = channel + s.lastChatID = chatID + return "PAIR1234", nil +} + +func (s *feishuPolicyPairingStore) IsPaired(_ context.Context, senderID, channel string) (bool, error) { + return s.paired[senderID+"|"+channel], nil +} + +func (s *feishuPolicyPairingStore) ApprovePairing(context.Context, string, string) (*store.PairedDeviceData, error) { + return nil, errors.New("not implemented") +} +func (s *feishuPolicyPairingStore) DenyPairing(context.Context, string) error { return nil } +func (s *feishuPolicyPairingStore) RevokePairing(context.Context, string, string) error { + return nil +} +func (s *feishuPolicyPairingStore) ListPending(context.Context) []store.PairingRequestData { + return nil +} +func (s *feishuPolicyPairingStore) ListPaired(context.Context) []store.PairedDeviceData { + return nil +} +func (s *feishuPolicyPairingStore) MigrateGroupChatID(context.Context, string, string, string) error { + return nil +} + +func TestHandleMessageEvent_GroupPairingRequiresTargetBotMention(t *testing.T) { + requireMention := true + ps := &feishuPolicyPairingStore{paired: map[string]bool{}} + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("feishu-test") + ch.botOpenID = "ou_target_bot" + srv := newSimpleMockServer(t, `{"code":0,"msg":"ok","data":{"message_id":"om_sent"}}`) + ch.client = NewLarkClient("app", "secret", srv.URL) + + event := feishuGroupTextEvent(t, "om_non_target", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", "ou_other_bot", "OtherBot"), + }) + ch.handleMessageEvent(context.Background(), event) + + if ps.requests != 0 { + t.Fatalf("pairing requests = %d, want 0 for non-target mention", ps.requests) + } + assertNoFeishuInbound(t, msgBus) +} + +func TestHandleMessageEvent_GroupSkipsExplicitOtherMentionEvenWhenRequireMentionDisabled(t *testing.T) { + requireMention := false + ps := &feishuPolicyPairingStore{paired: map[string]bool{}} + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("lark-cppai-pm") + ch.botOpenID = "ou_0b6fac6e84cf8c773a0c2768147799e2" + srv := newSimpleMockServer(t, `{"code":0,"msg":"ok","data":{"message_id":"om_sent"}}`) + ch.client = NewLarkClient("app", "secret", srv.URL) + + event := feishuGroupTextEvent(t, "om_other_bot_target", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", "ou_3ef6f00429b40780fb7ffc4a972fd835", "itsddvnm"), + }) + ch.handleMessageEvent(context.Background(), event) + + if ps.requests != 0 { + t.Fatalf("pairing requests = %d, want 0 when explicit mention targets another bot", ps.requests) + } + assertNoFeishuInbound(t, msgBus) +} + +func TestHandleMessageEvent_GroupPairingRequestsOnlyWhenTargetMentioned(t *testing.T) { + requireMention := true + ps := &feishuPolicyPairingStore{paired: map[string]bool{}} + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("feishu-test") + ch.botOpenID = "ou_target_bot" + srv := newSimpleMockServer(t, `{"code":0,"msg":"ok","data":{"message_id":"om_sent"}}`) + ch.client = NewLarkClient("app", "secret", srv.URL) + + event := feishuGroupTextEvent(t, "om_target", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", "ou_target_bot", "TargetBot"), + }) + ch.handleMessageEvent(context.Background(), event) + + if ps.requests != 1 { + t.Fatalf("pairing requests = %d, want 1", ps.requests) + } + if ps.lastSenderID != "group:oc_group" { + t.Fatalf("pairing senderID = %q, want group:oc_group", ps.lastSenderID) + } + if ps.lastChannel != "feishu-test" || ps.lastChatID != "oc_group" { + t.Fatalf("unexpected pairing request channel/chat: %s/%s", ps.lastChannel, ps.lastChatID) + } + assertNoFeishuInbound(t, msgBus) +} + +func TestHandleMessageEvent_GroupPairingAllowsAlreadyPairedTargetMention(t *testing.T) { + requireMention := true + ps := &feishuPolicyPairingStore{ + paired: map[string]bool{"group:oc_group|feishu-test": true}, + } + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("feishu-test") + ch.botOpenID = "ou_target_bot" + srv := newSimpleMockServer(t, `{"code":0,"msg":"ok","data":{"name":"Alice"}}`) + ch.client = NewLarkClient("app", "secret", srv.URL) + + event := feishuGroupTextEvent(t, "om_paired_target", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", "ou_target_bot", "TargetBot"), + }) + ch.handleMessageEvent(context.Background(), event) + + if ps.requests != 0 { + t.Fatalf("pairing requests = %d, want 0 for already paired group", ps.requests) + } + msg := assertFeishuInbound(t, msgBus) + if msg.Channel != "feishu-test" || msg.ChatID != "oc_group" || msg.PeerKind != "group" { + t.Fatalf("unexpected inbound route: channel=%q chat=%q peer=%q", msg.Channel, msg.ChatID, msg.PeerKind) + } +} + +func feishuGroupTextEvent(t *testing.T, messageID, senderID, chatID, text string, mentions []EventMention) *MessageEvent { + t.Helper() + content, err := json.Marshal(map[string]string{"text": text}) + if err != nil { + t.Fatalf("marshal text content: %v", err) + } + + ev := &MessageEvent{} + ev.Event.Message.MessageID = messageID + ev.Event.Message.ChatID = chatID + ev.Event.Message.ChatType = "group" + ev.Event.Message.MessageType = "text" + ev.Event.Message.Content = string(content) + ev.Event.Message.Mentions = mentions + ev.Event.Sender.SenderID.OpenID = senderID + return ev +} + +func feishuMention(key, openID, name string) EventMention { + return EventMention{ + Key: key, + ID: struct { + OpenID string `json:"open_id"` + UserID string `json:"user_id"` + UnionID string `json:"union_id"` + }{OpenID: openID}, + Name: name, + } +} + +func assertFeishuInbound(t *testing.T, msgBus *bus.MessageBus) bus.InboundMessage { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + msg, ok := msgBus.ConsumeInbound(ctx) + if !ok { + t.Fatal("expected inbound message") + } + return msg +} + +func assertNoFeishuInbound(t *testing.T, msgBus *bus.MessageBus) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) + defer cancel() + if msg, ok := msgBus.ConsumeInbound(ctx); ok { + t.Fatalf("unexpected inbound message: %+v", msg) + } +} + // --- webhookPath --- func TestWebhookPath_Default(t *testing.T) { diff --git a/internal/channels/feishu/feishu.go b/internal/channels/feishu/feishu.go index 78289d1b..0a2ae6cc 100644 --- a/internal/channels/feishu/feishu.go +++ b/internal/channels/feishu/feishu.go @@ -135,13 +135,12 @@ func (c *Channel) Start(ctx context.Context) error { c.GroupHistory().StartFlusher() slog.Info("starting feishu/lark bot") - // Probe bot identity. Without botOpenID, mention detection degrades to - // "any mention counts as bot mention" (see parseMessageEvent), so retry - // transient failures before giving up. + // Probe bot identity. Group mention detection fails closed without + // botOpenID, so retry transient failures before giving up. if err := c.probeBotInfoWithRetry(ctx, 3); err != nil { - slog.Warn("feishu bot probe failed (will continue)", "error", err) + slog.Warn("feishu bot probe failed (will continue)", "channel", c.Name(), "error", err) } else { - slog.Info("feishu bot connected", "bot_open_id", c.botOpenID) + slog.Info("feishu bot connected", "channel", c.Name(), "bot_open_id", c.botOpenID) } mode := c.cfg.ConnectionMode @@ -275,7 +274,7 @@ func (a *wsEventAdapter) HandleEvent(ctx context.Context, payload []byte) error return fmt.Errorf("parse event: %w", err) } if event.Header.EventType == "im.message.receive_v1" { - a.ch.handleMessageEvent(ctx, &event) + a.ch.handleMessageEventFrom(ctx, &event, "websocket") } return nil } @@ -316,7 +315,7 @@ func (c *Channel) WebhookHandler() (string, http.Handler) { handler := NewWebhookHandler(c.cfg.VerificationToken, c.cfg.EncryptKey, func(event *MessageEvent) { ctx := store.WithTenantID(context.Background(), c.TenantID()) - c.handleMessageEvent(ctx, event) + c.handleMessageEventFrom(ctx, event, "webhook-main") }) return path, http.HandlerFunc(handler) @@ -337,7 +336,7 @@ func (c *Channel) startWebhook(ctx context.Context) error { handler := NewWebhookHandler(c.cfg.VerificationToken, c.cfg.EncryptKey, func(event *MessageEvent) { ctx := store.WithTenantID(context.Background(), c.TenantID()) - c.handleMessageEvent(ctx, event) + c.handleMessageEventFrom(ctx, event, "webhook-server") }) mux := http.NewServeMux() @@ -358,6 +357,87 @@ func (c *Channel) startWebhook(ctx context.Context) error { return nil } +func (c *Channel) shouldProcessMessageEvent(event *MessageEvent, source string) bool { + eventType := strings.TrimSpace(event.Header.EventType) + if eventType != "" && eventType != "im.message.receive_v1" { + slog.Debug("feishu inbound event skipped; unsupported event type", + "source", source, + "decision", "skip_event_type", + "channel", c.Name(), + "event_id", event.Header.EventID, + "event_type", event.Header.EventType, + "event_app_id", appIDForLog(event.Header.AppID), + "configured_app_id", appIDForLog(c.cfg.AppID), + ) + return false + } + + appIDMatch := c.eventAppIDMatches(event) + slog.Debug("feishu inbound message event received", + "source", source, + "decision", "received", + "channel", c.Name(), + "event_id", event.Header.EventID, + "event_type", event.Header.EventType, + "event_app_id", appIDForLog(event.Header.AppID), + "configured_app_id", appIDForLog(c.cfg.AppID), + "app_id_match", appIDMatch, + "message_id", event.Event.Message.MessageID, + "chat_id", event.Event.Message.ChatID, + "chat_type", event.Event.Message.ChatType, + "message_type", event.Event.Message.MessageType, + "sender_open_id", event.Event.Sender.SenderID.OpenID, + "mention_count", len(event.Event.Message.Mentions), + ) + if !appIDMatch { + slog.Info("feishu inbound message skipped; app id mismatch", + "source", source, + "decision", "skip_app_mismatch", + "channel", c.Name(), + "event_id", event.Header.EventID, + "event_app_id", appIDForLog(event.Header.AppID), + "configured_app_id", appIDForLog(c.cfg.AppID), + "message_id", event.Event.Message.MessageID, + "chat_id", event.Event.Message.ChatID, + ) + return false + } + return true +} + +func (c *Channel) eventAppIDMatches(event *MessageEvent) bool { + expected := strings.TrimSpace(c.cfg.AppID) + actual := strings.TrimSpace(event.Header.AppID) + return expected == "" || actual == "" || expected == actual +} + +func (c *Channel) logParsedMessage(event *MessageEvent, mc *messageContext, source string) { + slog.Debug("feishu message parsed", + "source", source, + "decision", "parsed", + "channel", c.Name(), + "event_id", event.Header.EventID, + "message_id", mc.MessageID, + "chat_id", mc.ChatID, + "chat_type", mc.ChatType, + "content_type", mc.ContentType, + "sender_open_id", mc.SenderID, + "bot_open_id", c.botOpenID, + "mentioned_bot", mc.MentionedBot, + "mention_count", len(mc.Mentions), + "mentions", formatMentionInfos(mc.Mentions), + "preview", channels.Truncate(mc.Content, 80), + ) +} + +func appIDForLog(appID string) string { + appID = strings.TrimSpace(appID) + if len(appID) <= 8 { + return appID + } + return appID[:4] + "..." + appID[len(appID)-4:] +} + // --- Bot probe --- func (c *Channel) probeBotInfo(ctx context.Context) error { diff --git a/internal/channels/feishu/feishu_lifecycle_test.go b/internal/channels/feishu/feishu_lifecycle_test.go index 87677abc..746ec374 100644 --- a/internal/channels/feishu/feishu_lifecycle_test.go +++ b/internal/channels/feishu/feishu_lifecycle_test.go @@ -2,10 +2,12 @@ package feishu import ( "context" + "encoding/json" "testing" "time" "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/config" ) @@ -30,6 +32,80 @@ func TestWSEventAdapter_HandleEvent_NonMessageEvent_NoError(t *testing.T) { } } +func TestWSEventAdapter_HandleEvent_AppIDMismatchSkipsMessage(t *testing.T) { + requireMention := true + ps := &feishuPolicyPairingStore{paired: map[string]bool{}} + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app-cppai", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("lark-cppai-pm") + ch.botOpenID = "ou_0b6fac6e84cf8c773a0c2768147799e2" + + event := feishuGroupTextEvent(t, "om_app_mismatch", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", ch.botOpenID, "CPPAI PM"), + }) + event.Header.EventType = "im.message.receive_v1" + event.Header.AppID = "app-itsddnv" + payload := mustMarshalMessageEvent(t, event) + + adapter := &wsEventAdapter{ch: ch} + if err := adapter.HandleEvent(context.Background(), payload); err != nil { + t.Fatalf("HandleEvent error: %v", err) + } + + if ps.requests != 0 { + t.Fatalf("pairing requests = %d, want 0 for app_id mismatch", ps.requests) + } + assertNoFeishuInbound(t, msgBus) +} + +func TestWSEventAdapter_HandleEvent_AppIDMatchProcessesMessage(t *testing.T) { + requireMention := true + ps := &feishuPolicyPairingStore{paired: map[string]bool{}} + msgBus := bus.New() + defer msgBus.Close() + + ch, err := New(config.FeishuConfig{ + AppID: "app-cppai", + AppSecret: "secret", + GroupPolicy: "pairing", + RequireMention: &requireMention, + }, msgBus, ps, nil, nil) + if err != nil { + t.Fatalf("New feishu channel: %v", err) + } + ch.SetName("lark-cppai-pm") + ch.botOpenID = "ou_0b6fac6e84cf8c773a0c2768147799e2" + srv := newSimpleMockServer(t, `{"code":0,"msg":"ok","data":{"message_id":"om_sent"}}`) + ch.client = NewLarkClient("app-cppai", "secret", srv.URL) + + event := feishuGroupTextEvent(t, "om_app_match", "ou_alice", "oc_group", "@_user_1 help", []EventMention{ + feishuMention("@_user_1", ch.botOpenID, "CPPAI PM"), + }) + event.Header.EventType = "im.message.receive_v1" + event.Header.AppID = "app-cppai" + payload := mustMarshalMessageEvent(t, event) + + adapter := &wsEventAdapter{ch: ch} + if err := adapter.HandleEvent(context.Background(), payload); err != nil { + t.Fatalf("HandleEvent error: %v", err) + } + + if ps.requests != 1 { + t.Fatalf("pairing requests = %d, want 1 for matching app_id", ps.requests) + } + assertNoFeishuInbound(t, msgBus) +} + // --- probeBotInfo --- func TestProbeBotInfo_Success(t *testing.T) { @@ -227,3 +303,12 @@ func newLifecycleTestChannel(t *testing.T) *Channel { } return ch } + +func mustMarshalMessageEvent(t *testing.T, event *MessageEvent) []byte { + t.Helper() + payload, err := json.Marshal(event) + if err != nil { + t.Fatalf("marshal message event: %v", err) + } + return payload +}