feat(discord): configurable reaction emojis for processing lifecycle - #22100
feat(discord): configurable reaction emojis for processing lifecycle#22100btorresgil wants to merge 1 commit into
Conversation
|
I took a pass over this because it matches #22365 exactly. The implementation shape looks good and matches the existing Discord reaction lifecycle. A few edge cases that may be worth adding before merge:
I also tested the same behavior locally with focused coverage around:
Validation command I used locally: pytest tests/gateway/test_discord_reactions.py tests/gateway/test_discord_reply_mode.py -qOne process suggestion: keep this PR scoped to |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused implementation and lifecycle coverage. The requested configurability is still needed: current main hardcodes the three lifecycle emojis in plugins/platforms/discord/adapter.py:1974-1986.
Problems
- The PR targets the pre-plugin adapter path,
gateway/platforms/discord.py. Commitcc8e5ec2afbfd10a3cff4e710210dd9ecae64a33moved the live adapter toplugins/platforms/discord/adapter.pywithout a compatibility shim. - Current main routes top-level Discord YAML through
gateway/config.py:1252-1287into the plugin hook,plugins/platforms/discord/adapter.py:8229-8352. The PR'sPlatformConfig.extrabridge must be adapted to that hook; otherwisereaction_emojiswill not reach the env-driven runtime adapter.
Suggested changes
- Port the resolver and lifecycle substitutions to
plugins/platforms/discord/adapter.py. - Add the YAML translation to
_apply_yaml_configand test config loading as well as direct lifecycle behavior.
Automated hermes-sweeper review.
| bridged["group_allow_from"] = platform_cfg["group_allow_from"] | ||
| if plat in (Platform.DISCORD, Platform.SLACK) and "channel_skill_bindings" in platform_cfg: | ||
| bridged["channel_skill_bindings"] = platform_cfg["channel_skill_bindings"] | ||
| if plat == Platform.DISCORD and "reaction_emojis" in platform_cfg: |
There was a problem hiding this comment.
Current main moved Discord's YAML bridge into plugins/platforms/discord/adapter.py::_apply_yaml_config in cc8e5ec2. Please port this configuration handling to that plugin hook; the legacy bridge is no longer the runtime path.
What does this PR do?
Adds optional
reaction_emojisconfig block underdiscord:to let users override the thinking/done/error emojis used during message processing. The masterdiscord.reactionsswitch andDISCORD_REACTIONSenv var continue to control all reaction activity as before.Config shape:
reaction_emojisis optional — omitting it preserves legacy behavior exactly.Resolution semantics
For each slot (
thinking,done,error):nullor""Lifecycle behavior
thinkingresolves, add it.thinkingif it was enabled; adddoneorerrorper outcome. Cancelled path removes thinking only, adds no terminal emoji.Related Issue
Fixes #22365
Type of Change
Changes Made
gateway/platforms/discord.py—_resolve_emoji()helper + updated start/complete handlersgateway/config.py—reaction_emojisfield added to Discord config modeltests/gateway/test_discord_reactions.py— 15 new/updated tests covering defaults, disable semantics, trim, cancelled path, custom emojis, and master-switch overridewebsite/docs/user-guide/messaging/discord.md— documents the new config blockHow to Test
nullhas the same behavior as empty string)Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A