feat: add configurable ack reaction support for Feishu#347
feat: add configurable ack reaction support for Feishu#347zhangweiii wants to merge 1 commit intosipeed:mainfrom
Conversation
Add global messages configuration to control acknowledgment reactions across all channels. Implement Feishu message reaction (emoji) support based on configuration scope. Features: - Add MessagesConfig with ack_reaction, ack_reaction_scope, remove_ack_after_reply - Add ShouldAckReaction() for cross-channel ack reaction logic - Implement Feishu message reaction via Lark API - Update all channels to receive MessagesConfig via BaseChannel Configuration options: - ack_reaction: emoji to use (e.g., "OK"), empty to disable - ack_reaction_scope: "all", "direct", "group-all", "group-mentions", "off" - remove_ack_after_reply: reserved for future use Refs: inspired by openclaw implementation
There was a problem hiding this comment.
Pull request overview
This PR introduces a global messages configuration intended to control acknowledgment (ack) reactions across channels, and adds Feishu emoji reaction support using the Lark API.
Changes:
- Add
config.MessagesConfig(ack emoji, scope, and “remove after reply” toggle) and include it inConfigdefaults + example config. - Plumb
MessagesConfigthroughBaseChanneland update all channel constructors/manager wiring accordingly. - Add
ShouldAckReactionhelper and implement Feishu inbound-message emoji reactions.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/config.go | Adds MessagesConfig to main config and sets defaults for ack behavior. |
| config/config.example.json | Documents the new messages configuration block. |
| pkg/channels/base.go | Stores MessagesConfig on BaseChannel and exposes MessagesConfig() accessor. |
| pkg/channels/ack_reactions.go | Introduces scope constants + ShouldAckReaction helper (and a reaction lifecycle struct). |
| pkg/channels/feishu_64.go | Adds Feishu inbound-message reaction via Lark “CreateMessageReaction”. |
| pkg/channels/feishu_32.go | Updates Feishu stub constructor signature for 32-bit builds. |
| pkg/channels/manager.go | Passes m.config.Messages into channel constructors. |
| pkg/channels/slack.go | Updates constructor signature to accept MessagesConfig and pass it into BaseChannel. |
| pkg/channels/slack_test.go | Updates tests for the new Slack constructor signature. |
| pkg/channels/telegram.go | Passes cfg.Messages into BaseChannel. |
| pkg/channels/whatsapp.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/discord.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/dingtalk.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/line.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/maixcam.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/onebot.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/qq.go | Updates constructor signature and passes MessagesConfig into BaseChannel. |
| pkg/channels/base_test.go | Updates test helper construction for NewBaseChannel signature change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| MonitorUSB: true, | ||
| }, | ||
| Messages: MessagesConfig{ | ||
| AckReaction: "OK", |
There was a problem hiding this comment.
Defaulting AckReaction to a non-empty value enables reactions for existing installs that upgrade without adding a messages section (because DefaultConfig() seeds the struct before JSON unmarshal). Consider defaulting AckReaction to empty (feature off) to avoid an unexpected behavior change, and rely on config.example.json/docs to show how to enable it.
| AckReaction: "OK", | |
| AckReaction: "", |
| 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" |
There was a problem hiding this comment.
The AckReactionScope comment/docs list doesn't include the supported alias value "none" (handled in ShouldAckReaction). Update the inline comment (and ideally the example config) so users know both disable values are accepted.
| // 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) |
| // Group mentions only | ||
| if scope == AckReactionScopeGroupMentions { | ||
| // Not a mentionable group, don't ack | ||
| if !params.IsMentionableGroup { | ||
| return false | ||
| } | ||
| // No mention required, don't ack (avoid over-acknowledging) | ||
| if !params.RequireMention { | ||
| return false | ||
| } | ||
| // Can't detect mentions, don't ack | ||
| if !params.CanDetectMention { | ||
| return false | ||
| } | ||
| // Mentioned or bypass required, ack | ||
| return params.WasMentioned || params.ShouldBypassMention | ||
| } |
There was a problem hiding this comment.
In AckReactionScopeGroupMentions mode, ShouldAckReaction never checks params.IsGroup, so it can return true for non-group messages if the caller sets WasMentioned/flags accordingly. Add an if !params.IsGroup { return false } guard (or otherwise incorporate IsGroup) to keep the behavior consistent with the scope name.
| // ShouldAckReaction determines whether an ack reaction should be sent based on parameters | ||
| // Reference: openclaw implementation | ||
| func ShouldAckReaction(params AckReactionParams) bool { | ||
| // Default to group-mentions if not specified | ||
| scope := params.Scope | ||
| if scope == "" { | ||
| scope = AckReactionScopeGroupMentions | ||
| } | ||
|
|
||
| // Disabled cases | ||
| if scope == AckReactionScopeOff || scope == AckReactionScopeNone { | ||
| return false | ||
| } | ||
|
|
||
| // All messages | ||
| if scope == AckReactionScopeAll { | ||
| return true | ||
| } | ||
|
|
||
| // Direct messages only | ||
| if scope == AckReactionScopeDirect { | ||
| return params.IsDirect | ||
| } | ||
|
|
||
| // All group messages | ||
| if scope == AckReactionScopeGroupAll { | ||
| return params.IsGroup | ||
| } | ||
|
|
||
| // Group mentions only | ||
| if scope == AckReactionScopeGroupMentions { | ||
| // Not a mentionable group, don't ack | ||
| if !params.IsMentionableGroup { | ||
| return false | ||
| } | ||
| // No mention required, don't ack (avoid over-acknowledging) | ||
| if !params.RequireMention { | ||
| return false | ||
| } | ||
| // Can't detect mentions, don't ack | ||
| if !params.CanDetectMention { | ||
| return false | ||
| } | ||
| // Mentioned or bypass required, ack | ||
| return params.WasMentioned || params.ShouldBypassMention | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
ShouldAckReaction (and the scope constants) introduce new cross-channel behavior logic but there are no unit tests covering the expected truth table for each scope/flag combination. Add table-driven tests for the main combinations (all/direct/group-all/group-mentions/off/none, including default scope when empty) to prevent regressions as more channels adopt this helper.
| // Check if bot was mentioned | ||
| wasMentioned := false | ||
| if message.Mentions != nil && len(message.Mentions) > 0 { |
There was a problem hiding this comment.
wasMentioned is set to true whenever the message contains any mentions, which doesn't necessarily mean the bot/app was mentioned (it could be another user). This will cause group-mentions scope to ack reactions on unrelated mentions. Instead, determine whether this bot was mentioned by inspecting the mention entries and matching against the bot user/app ID.
| // Check if bot was mentioned | |
| wasMentioned := false | |
| if message.Mentions != nil && len(message.Mentions) > 0 { | |
| // 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 isDirect && message.Mentions != nil && len(message.Mentions) > 0 { |
| // Add emoji ack reaction based on configuration | ||
| ackReaction := c.MessagesConfig().AckReaction | ||
| if ackReaction != "" && messageID != "" { | ||
| shouldAck := ShouldAckReaction(AckReactionParams{ | ||
| Scope: AckReactionScope(c.MessagesConfig().AckReactionScope), | ||
| IsDirect: isDirect, | ||
| IsGroup: isGroup, | ||
| IsMentionableGroup: true, // Feishu groups support mentions | ||
| RequireMention: true, | ||
| CanDetectMention: true, | ||
| WasMentioned: wasMentioned, | ||
| ShouldBypassMention: false, | ||
| }) | ||
|
|
||
| if shouldAck { | ||
| if err := c.addMessageReaction(ctx, messageID, ackReaction); err != nil { | ||
| logger.ErrorCF("feishu", "Failed to add emoji reaction", map[string]interface{}{ | ||
| "error": err.Error(), | ||
| "message_id": messageID, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| c.HandleMessage(senderID, chatID, content, nil, metadata) |
There was a problem hiding this comment.
The ack reaction is added before HandleMessage(...) runs the allowlist check. As a result, blocked senders can still get an acknowledgment reaction. Gate the reaction logic behind c.IsAllowed(senderID) (or move it after an explicit allowlist check) so you only acknowledge messages you will actually process.
| @@ -48,7 +48,7 @@ func NewSlackChannel(cfg config.SlackConfig, messageBus *bus.MessageBus) (*Slack | |||
|
|
|||
| socketClient := socketmode.New(api) | |||
|
|
|||
| base := NewBaseChannel("slack", cfg, messageBus, cfg.AllowFrom) | |||
| base := NewBaseChannel("slack", cfg, messagesCfg, messageBus, cfg.AllowFrom) | |||
|
|
|||
There was a problem hiding this comment.
messagesCfg is now threaded into Slack via BaseChannel, but Slack’s existing ack behavior still uses hardcoded reactions ("eyes" / "white_check_mark") and does not consult MessagesConfig or ShouldAckReaction. If the intent is truly global, wire Slack’s reactions to the new config (including scope + disable) or clarify that Slack is intentionally out of scope for now.
| // MessagesConfig controls global message behavior for all channels | ||
| 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 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"` | ||
| } |
There was a problem hiding this comment.
RemoveAckAfterReply is introduced in MessagesConfig but is not used anywhere in the codebase (including Feishu reactions). Either wire this option into the ack reaction lifecycle (remove reaction after sending a reply) or remove it from the config surface to avoid a misleading setting.
📝 Description
Add global messages configuration to control acknowledgment reactions across all channels. Implement Feishu message reaction
(emoji) support based on configuration scope.
Key changes:
MessagesConfigwithack_reaction,ack_reaction_scope,remove_ack_after_replyShouldAckReaction()for cross-channel ack reaction logicMessagesConfigviaBaseChannelUse case: When users mention the bot in group chats, automatically add an "OK" 👌 emoji reaction to indicate the message has been
received.
🗣️ Type of Change
🔗 Linked Issue
📚 Technical Context (Skip for Docs)
lacking unified configuration
🧪 Test Environment & Hardware
📸 Proof of Work (Optional for Docs)
Configuration example:
{ "messages": { "ack_reaction": "OK", "ack_reaction_scope": "group-mentions", "remove_ack_after_reply": false } }Scope options:
☑️ Checklist