Skip to content

fix(unified): port reaction_add handler to unified build#1177

Merged
chaodu-agent merged 4 commits into
mainfrom
fix/unified-discord-reaction
Jun 23, 2026
Merged

fix(unified): port reaction_add handler to unified build#1177
chaodu-agent merged 4 commits into
mainfrom
fix/unified-discord-reaction

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Problem

The unified binary (Dockerfile.unified / pre-beta-* images) was missing the reaction_add EventHandler method. Emoji reaction mappings configured via [reactions.mapping] silently did nothing.

Root Cause

When the workspace was restructured (PR #1146), reaction_add was never ported from src/discord.rs to crates/openab-core/src/discord.rs.

Fix

Ported the full reaction_add handler (~247 lines) from the standard build. Removed ctl_registry references (IPC module not yet in unified — tracked separately).

Testing

  • cargo check --features unified
  • cargo clippy --features unified -- -D warnings

Fixes #1176

The unified binary was missing the reaction_add EventHandler method,
causing emoji reaction mappings to silently not work. This was never
ported when the workspace restructure moved code to openab-core.

Ported from src/discord.rs (v0.8.5) to crates/openab-core/src/discord.rs.
Removed ctl_registry references (IPC not yet in unified).

Fixes #1176
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner June 23, 2026 02:04
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean port of the reaction_add handler to the unified build.

What This PR Does

The unified binary (Dockerfile.unified / pre-beta-* images) silently dropped emoji-to-text reaction mappings because reaction_add was never ported during the workspace restructure in PR #1146. This PR restores that functionality.

How It Works

Ports the full reaction_add EventHandler (~247 lines) from the standard build into crates/openab-core/src/discord.rs. The handler:

  1. Ignores bot self-reactions (feedback loop prevention)
  2. Extracts unicode emoji → looks up [reactions.mapping] config
  3. Applies the same bot/user/channel gating as message() (AllowBots, AllowUsers, allowlists, multibot detection)
  4. Resolves thread context and dispatches through the existing Dispatcher
  5. Intentionally omits ctl_registry references (IPC not yet in unified — tracked separately)

Findings

# Severity Finding Location
1 🟢 Faithful port with correct gating parity discord.rs:903-1152
2 🟢 Defense-in-depth bot re-check after to_user() fetch discord.rs:1017-1028
3 🟢 Multibot routing uses message_author_id correctly discord.rs:1094-1098
Finding Details

🟢 F1: Faithful port with correct gating parity

The handler replicates all access-control gates from message(): bot gating (AllowBots enum), user allowlist (is_denied_user), channel/thread detection, AllowUsers mode gating, and multibot thread routing. No logic drift.

🟢 F2: Defense-in-depth bot re-check

After fetching user info via to_user(), the handler re-applies bot gating if the API reveals the reactor is a bot but reaction.member was None. Edge case handled correctly.

🟢 F3: Multibot routing

In MultibotMentions mode, the handler correctly uses reaction.message_author_id to determine which bot should respond — only processing reactions on messages authored by this bot.

Baseline Check
  • PR opened: 2026-06-23
  • Main already has: reactions.mapping config parsing, ReactionsConfig struct with mapping: HashMap<String, String>, but no reaction_add handler in the unified crate
  • Net-new value: Restores the entire event handler that makes [reactions.mapping] actually work in unified builds
What's Good (🟢)
  • Exact parity with standard build gating logic — no shortcuts taken
  • Clean removal of ctl_registry with explicit tracking note in PR body
  • Early-exit pattern (fast returns before expensive API calls)
  • CI cargo check and clippy pass

chaodu-agent added 2 commits June 23, 2026 02:15
F1: Only call bot_participated_in_thread when gating mode requires it
    (Involved/MultibotMentions). Avoids unconditional API calls that
    cause rate-limiting on non-thread channels.

F2: Move is_denied_user check into spawned task after to_user().await
    confirms bot status, fixing premature gating with partial cache.

F3: Reuse detect_thread and build_sender_context helpers instead of
    manual reimplementation, eliminating DRY violations.
F1: Move channel/thread detection + allowlist check BEFORE spawn and
    BEFORE bot_participated_in_thread. Only call participation check
    when is_thread=true AND gating mode requires it. Non-thread
    channels and unallowed threads never trigger the 200-message API
    fetch. No Arc<Mutex> refactoring needed.

F2: Move is_denied_user check into spawned task after to_user().await
    confirms bot status, fixing premature gating with partial cache.

F3: Reuse detect_thread and build_sender_context helpers instead of
    manual reimplementation, eliminating DRY violations.
@chaodu-agent chaodu-agent force-pushed the fix/unified-discord-reaction branch from 6311390 to 8213774 Compare June 23, 2026 02:21
…ng tests

Extract reaction gating logic into a testable pure function and add
8 focused tests covering:
- Mentions mode always rejects reactions
- Non-thread channels skip participation check
- Uninvolved thread rejected
- Involved thread accepted
- MultibotMentions single-bot thread accepted
- MultibotMentions targets this bot accepted
- MultibotMentions targets other bot rejected
- MultibotMentions non-thread rejected

This pins the gating ordering that regressed during review.
@chaodu-agent chaodu-agent enabled auto-merge June 23, 2026 02:38
@chaodu-agent chaodu-agent merged commit ebc2d12 into main Jun 23, 2026
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: emoji reaction mapping not working in pre-beta-kiro snapshot

2 participants