feat(pipeline): 预回应表情功能 - 外层包装 + 自动撤回#7009
feat(pipeline): 预回应表情功能 - 外层包装 + 自动撤回#7009Sclock wants to merge 15 commits intoAstrBotDevs:masterfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…onfig Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sStage emoji code PreAckEmojiManager now wraps pipeline execution in scheduler.execute(), completely outside the onion model. Old emoji logic removed from PreProcessStage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- PreAckEmojiManager now returns EmojiRef(emoji, reaction_id) instead of str - Lark react() returns reaction_id instead of storing on event as side-effect - remove_react() accepts optional reaction_id parameter across all platforms - Discord remove_react adds hasattr guard for consistency with react() - Added 2 new tests for Lark reaction_id flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求引入了预回应表情功能,允许机器人在接收到命令时自动添加表情,并在处理完成后根据配置自动撤回。核心改动包括引入一个独立的表情管理器,实现了对 Telegram、飞书和 Discord 平台的表情撤回支持,并提供了细粒度的配置选项。此外,还修复了流水线洋葱模型中的一个执行 bug,并优化了飞书 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new behavior where
event.reactmay return areaction_id(used byPreAckEmojiManager) conflicts with existing-> Nonetype hints and base-class expectations (e.g.,LarkEvent.reactandAstrMessageEvent.react), so consider updating the method signatures and/or base interface to explicitly allow and document areaction_idreturn value (and optional emoji where applicable) to keep the contract consistent across platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new behavior where `event.react` may return a `reaction_id` (used by `PreAckEmojiManager`) conflicts with existing `-> None` type hints and base-class expectations (e.g., `LarkEvent.react` and `AstrMessageEvent.react`), so consider updating the method signatures and/or base interface to explicitly allow and document a `reaction_id` return value (and optional emoji where applicable) to keep the contract consistent across platforms.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/lark/lark_event.py" line_range="560" />
<code_context>
async def react(self, emoji: str) -> None:
</code_context>
<issue_to_address>
**issue:** Return type of `react` no longer matches its annotation and base-class contract.
This still returns a `reaction_id` (or `None`) despite the `-> None` annotation and the base `AstrMessageEvent.react` contract documenting fire-and-forget behavior. Either update the annotation and base contract to match the new behavior, or keep `react` as `-> None` and expose a separate method to obtain the `reaction_id`, to avoid confusing readers and breaking type checking.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request refactors the pre-acknowledgement emoji functionality into a new PreAckEmojiManager, enabling the addition and conditional removal of emojis for Telegram, Lark, and Discord platforms. This involves updating configuration with an auto_remove option, implementing remove_react methods in platform-specific event classes, and integrating the manager into the PipelineScheduler's execution flow. New unit tests for the emoji manager and pipeline execution have been added. A critical issue was identified in the _process_stages method, where generator stages could cause subsequent pipeline stages to execute multiple times, and a fix has been suggested to prevent this unintended behavior.
| active_event_registry.register(event) | ||
| emoji = await self.pre_ack_emoji_mgr.add_emoji(event) | ||
| try: | ||
| await self._process_stages(event) |
There was a problem hiding this comment.
PR 描述中提到修复了洋葱模型重复执行后续阶段的 bug,并且添加了对应的单元测试 (test_pipeline_execution.py)。但我在 _process_stages 方法中没有看到对应的修复。
目前的实现中,当一个 generator stage (i) 执行完毕后(async for 循环结束),外层 for 循环会继续执行 i+1,导致 i+1 以及之后的所有阶段被再次执行。这会导致 test_pipeline_execution.py 中的测试失败。
为了修复这个问题,需要在 async for 循环之后判断是否真的发生了 yield。如果发生了,就应该 break 外层循环,因为后续阶段已经在递归调用 _process_stages(event, i + 1) 中处理过了。
建议将 _process_stages 方法修改如下:
async def _process_stages(self, event: AstrMessageEvent, from_stage=0) -> None:
"""依次执行各个阶段
Args:
event (AstrMessageEvent): 事件对象
from_stage (int): 从第几个阶段开始执行, 默认从0开始
"""
for i in range(from_stage, len(self.stages)):
stage = self.stages[i] # 获取当前要执行的阶段
coroutine = stage.process(event)
if isinstance(coroutine, AsyncGenerator):
did_yield = False
async for _ in coroutine:
did_yield = True
# 此处是前置处理完成后的暂停点(yield), 下面开始执行后续阶段
if event.is_stopped():
logger.debug(
f"阶段 {stage.__class__.__name__} 已终止事件传播。",
)
break
# 递归调用, 处理所有后续阶段
await self._process_stages(event, i + 1)
# 此处是后续所有阶段处理完毕后返回的点, 执行后置处理
if event.is_stopped():
logger.debug(
f"阶段 {stage.__class__.__name__} 已终止事件传播。",
)
break
if did_yield:
break
else:
# 如果返回的是普通协程(不含yield的async函数), 则不进入下一层(基线条件)
# 简单地等待它执行完成, 然后继续执行下一个阶段
await coroutine
if event.is_stopped():
logger.debug(f"阶段 {stage.__class__.__name__} 已终止事件传播。")
break这个修改引入了 did_yield 标志位,确保只有在 generator stage 真正 yield 并进入内层逻辑后,才会中断外层循环,从而避免了重复执行的问题。对于没有 yield 的 generator stage,其行为保持不变,会继续执行后续 stage.
async def _process_stages(self, event: AstrMessageEvent, from_stage=0) -> None:
"""依次执行各个阶段
Args:
event (AstrMessageEvent): 事件对象
from_stage (int): 从第几个阶段开始执行, 默认从0开始
"""
for i in range(from_stage, len(self.stages)):
stage = self.stages[i] # 获取当前要执行的阶段
coroutine = stage.process(event)
if isinstance(coroutine, AsyncGenerator):
did_yield = False
async for _ in coroutine:
did_yield = True
# 此处是前置处理完成后的暂停点(yield), 下面开始执行后续阶段
if event.is_stopped():
logger.debug(
f"阶段 {stage.__class__.__name__} 已终止事件传播。",
)
break
# 递归调用, 处理所有后续阶段
await self._process_stages(event, i + 1)
# 此处是后续所有阶段处理完毕后返回的点, 执行后置处理
if event.is_stopped():
logger.debug(
f"阶段 {stage.__class__.__name__} 已终止事件传播。",
)
break
if did_yield:
break
else:
# 如果返回的是普通协程(不含yield的async函数), 则不进入下一层(基线条件)
# 简单地等待它执行完成, 然后继续执行下一个阶段
await coroutine
if event.is_stopped():
logger.debug(f"阶段 {stage.__class__.__name__} 已终止事件传播。")
breakAddress Sourcery review: Lark react() returns reaction_id but type hint was -> None. Updated base class and Lark override to -> str | None. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Root cause: add_emoji runs before _process_stages, so is_at_or_wake_command has not been set by WakingCheckStage yet. For Lark and Telegram this was always False, preventing emoji. Non-wake messages will have their emoji auto-removed in finally block after WakingCheckStage stops the event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a generator stage yields, _process_stages recursively handles all subsequent stages. Without the did_yield break, the outer for-loop continued iterating and re-executed those stages. The did_yield flag breaks the outer loop after a yielding generator completes, while non-yielding generators (return before yield) continue normally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…yield" This reverts commit 9e56476.
…r onion yield"" This reverts commit 828891f.
Summary
PreAckEmojiManager类,在PipelineScheduler.execute()中作为前后包装运行,完全位于洋葱模型外部remove_react()方法,支持表情撤回auto_remove字段和 Discord 平台默认值,三个平台可独立配置PreProcessStage移除旧的表情逻辑,统一由PreAckEmojiManager管理did_yieldbug:防止 generator stage yield 后外层循环重复执行后续阶段EmojiRefdataclass 解耦飞书reaction_id的存储,避免 event 上的副作用Test Plan
🤖 Generated with Claude Code
Summary by Sourcery
Add a centralized pre-acknowledgement emoji manager around the pipeline scheduler to send and optionally auto-remove typing reactions, and strengthen pipeline execution correctness for onion-style stages.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: