From cdc092475aaf729d258efb7dc608a06eb3fc5cd3 Mon Sep 17 00:00:00 2001 From: zhangweiii Date: Thu, 19 Feb 2026 10:08:47 +0800 Subject: [PATCH] fix: improve ack reaction logic and Feishu message handling - Fix group-mentions scope to properly check IsGroup - Add allowlist check in Feishu message handler - Fix mention detection to only apply in direct chats - Remove remove_ack_after_reply configuration option - Change default ack_reaction to empty string (disabled by default) - Add comprehensive unit tests for ack_reactions --- config/config.example.json | 5 +- pkg/channels/ack_reactions.go | 18 ++- pkg/channels/ack_reactions_test.go | 196 +++++++++++++++++++++++++++++ pkg/channels/feishu_64.go | 12 +- pkg/config/config.go | 9 +- 5 files changed, 219 insertions(+), 21 deletions(-) create mode 100644 pkg/channels/ack_reactions_test.go diff --git a/config/config.example.json b/config/config.example.json index 050195829..af79ffcfe 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -130,9 +130,8 @@ "monitor_usb": true }, "messages": { - "ack_reaction": "OK", - "ack_reaction_scope": "group-mentions", - "remove_ack_after_reply": false + "ack_reaction": "", + "ack_reaction_scope": "group-mentions" }, "gateway": { "host": "0.0.0.0", diff --git a/pkg/channels/ack_reactions.go b/pkg/channels/ack_reactions.go index 8202c002b..2317c4c2a 100644 --- a/pkg/channels/ack_reactions.go +++ b/pkg/channels/ack_reactions.go @@ -69,6 +69,10 @@ func ShouldAckReaction(params AckReactionParams) bool { // Group mentions only if scope == AckReactionScopeGroupMentions { + // Not a group message, don't ack + if !params.IsGroup { + return false + } // Not a mentionable group, don't ack if !params.IsMentionableGroup { return false @@ -90,8 +94,6 @@ func ShouldAckReaction(params AckReactionParams) bool { // AckReactionManager manages the lifecycle of acknowledgment reactions type AckReactionManager struct { - // RemoveAfterReply indicates whether to remove the ack after reply - RemoveAfterReply bool // ReactionValue is the current reaction value (emoji) ReactionValue string // Added indicates if the ack reaction has been added @@ -99,11 +101,10 @@ type AckReactionManager struct { } // NewAckReactionManager creates a new ack reaction manager -func NewAckReactionManager(removeAfterReply bool, reaction string) *AckReactionManager { +func NewAckReactionManager(reaction string) *AckReactionManager { return &AckReactionManager{ - RemoveAfterReply: removeAfterReply, - ReactionValue: reaction, - Added: false, + ReactionValue: reaction, + Added: false, } } @@ -111,8 +112,3 @@ func NewAckReactionManager(removeAfterReply bool, reaction string) *AckReactionM func (m *AckReactionManager) MarkAdded() { m.Added = true } - -// ShouldRemoveAfterReply determines whether to remove the ack after reply -func (m *AckReactionManager) ShouldRemoveAfterReply() bool { - return m.RemoveAfterReply && m.Added && m.ReactionValue != "" -} diff --git a/pkg/channels/ack_reactions_test.go b/pkg/channels/ack_reactions_test.go new file mode 100644 index 000000000..4ff16a3d7 --- /dev/null +++ b/pkg/channels/ack_reactions_test.go @@ -0,0 +1,196 @@ +package channels + +import "testing" + +func TestShouldAckReaction(t *testing.T) { + tests := []struct { + name string + params AckReactionParams + want bool + }{ + // Scope: off/none (disabled) + { + name: "scope off disables all", + params: AckReactionParams{Scope: AckReactionScopeOff, IsDirect: true, IsGroup: true}, + want: false, + }, + { + name: "scope none alias disables all", + params: AckReactionParams{Scope: AckReactionScopeNone, IsDirect: true, IsGroup: true}, + want: false, + }, + // Scope: all + { + name: "scope all enables for direct messages", + params: AckReactionParams{Scope: AckReactionScopeAll, IsDirect: true, IsGroup: false}, + want: true, + }, + { + name: "scope all enables for group messages", + params: AckReactionParams{Scope: AckReactionScopeAll, IsDirect: false, IsGroup: true}, + want: true, + }, + // Scope: direct + { + name: "scope direct enables for direct messages", + params: AckReactionParams{Scope: AckReactionScopeDirect, IsDirect: true, IsGroup: false}, + want: true, + }, + { + name: "scope direct disables for group messages", + params: AckReactionParams{Scope: AckReactionScopeDirect, IsDirect: false, IsGroup: true}, + want: false, + }, + // Scope: group-all + { + name: "scope group-all enables for group messages", + params: AckReactionParams{Scope: AckReactionScopeGroupAll, IsDirect: false, IsGroup: true}, + want: true, + }, + { + name: "scope group-all disables for direct messages", + params: AckReactionParams{Scope: AckReactionScopeGroupAll, IsDirect: true, IsGroup: false}, + want: false, + }, + // Scope: group-mentions (mentionable group, require mention, can detect) + { + name: "scope group-mentions with all conditions met", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + WasMentioned: true, + }, + want: true, + }, + { + name: "scope group-mentions bypass mention requirement", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + ShouldBypassMention: true, + }, + want: true, + }, + { + name: "scope group-mentions not in group", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: true, + IsGroup: false, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + WasMentioned: true, + }, + want: false, + }, + { + name: "scope group-mentions not mentionable group", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: false, + RequireMention: true, + CanDetectMention: true, + WasMentioned: true, + }, + want: false, + }, + { + name: "scope group-mentions no mention required", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: false, + CanDetectMention: true, + WasMentioned: true, + }, + want: false, + }, + { + name: "scope group-mentions cannot detect mention", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: false, + WasMentioned: true, + }, + want: false, + }, + { + name: "scope group-mentions not mentioned", + params: AckReactionParams{ + Scope: AckReactionScopeGroupMentions, + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + WasMentioned: false, + }, + want: false, + }, + // Default scope (empty) - should default to group-mentions + { + name: "empty scope defaults to group-mentions with all conditions met", + params: AckReactionParams{ + Scope: "", + IsDirect: false, + IsGroup: true, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + WasMentioned: true, + }, + want: true, + }, + { + name: "empty scope defaults to group-mentions not in group", + params: AckReactionParams{ + Scope: "", + IsDirect: true, + IsGroup: false, + IsMentionableGroup: true, + RequireMention: true, + CanDetectMention: true, + WasMentioned: true, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ShouldAckReaction(tt.params); got != tt.want { + t.Errorf("ShouldAckReaction() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestAckReactionManager(t *testing.T) { + t.Run("mark added", func(t *testing.T) { + m := NewAckReactionManager("OK") + if m.Added { + t.Error("expected Added to be false initially") + } + m.MarkAdded() + if !m.Added { + t.Error("expected Added to be true after MarkAdded") + } + }) +} diff --git a/pkg/channels/feishu_64.go b/pkg/channels/feishu_64.go index 88c2e665d..c304b7ceb 100644 --- a/pkg/channels/feishu_64.go +++ b/pkg/channels/feishu_64.go @@ -151,14 +151,24 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. content = "[empty message]" } + // Check allowlist before processing + if !c.IsAllowed(senderID) { + logger.DebugCF("feishu", "Message blocked by allowlist", map[string]interface{}{ + "sender_id": senderID, + }) + return nil + } + // Determine chat type: p2p = direct, group = group chat chatType := stringValue(message.ChatType) isGroup := chatType == "group" isDirect := chatType == "p2p" // Check if bot was mentioned + // Without a reliable bot ID in scope, we conservatively only + // treat mentions in direct (p2p) chats as relevant to the bot. wasMentioned := false - if message.Mentions != nil && len(message.Mentions) > 0 { + if isDirect && message.Mentions != nil && len(message.Mentions) > 0 { wasMentioned = true } diff --git a/pkg/config/config.go b/pkg/config/config.go index 735e933c1..0bcf96993 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -171,10 +171,8 @@ type DevicesConfig struct { type MessagesConfig struct { // AckReaction is the emoji used to acknowledge inbound messages (empty to disable) AckReaction string `json:"ack_reaction" env:"PICOCLAW_MESSAGES_ACK_REACTION"` - // AckReactionScope controls when to send ack reactions: "all", "direct", "group-all", "group-mentions", "off" + // AckReactionScope controls when to send ack reactions: "all", "direct", "group-all", "group-mentions", "off" (or "none" to disable) AckReactionScope string `json:"ack_reaction_scope" env:"PICOCLAW_MESSAGES_ACK_REACTION_SCOPE"` - // RemoveAckAfterReply removes the ack reaction after reply is sent - RemoveAckAfterReply bool `json:"remove_ack_after_reply" env:"PICOCLAW_MESSAGES_REMOVE_ACK_AFTER_REPLY"` } type ProvidersConfig struct { @@ -343,9 +341,8 @@ func DefaultConfig() *Config { MonitorUSB: true, }, Messages: MessagesConfig{ - AckReaction: "OK", - AckReactionScope: "group-mentions", - RemoveAckAfterReply: false, + AckReaction: "", + AckReactionScope: "group-mentions", }, } }