fix: add missing platform adapter filter types#7738
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Changing
PlatformAdapterType.ALLfrom a bitwise union of all flags to a standaloneenum.auto()flag is a behavioral change; any existing code that relied onPlatformAdapterType.ALL & SOME_FLAGbeing truthy will now break, so consider either preserving the union semantics or introducing a separateANYflag for the new behavior. - The special-casing of
PlatformAdapterType.ALLinfilter()effectively turns it into "always match", butALLitself no longer represents all concrete adapter bits; it may be clearer and less error-prone to keepALLas a true union of all adapter flags and have callers explicitly passNoneor a dedicated value to mean "no filtering". - Given that
filter()is annotated to returnbool, you might want to make its return paths explicit (e.g., defaulting toFalsewhenplatform_typeisNoneor the adapter name is unknown) to avoid implicitNonereturns and make the behavior easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `PlatformAdapterType.ALL` from a bitwise union of all flags to a standalone `enum.auto()` flag is a behavioral change; any existing code that relied on `PlatformAdapterType.ALL & SOME_FLAG` being truthy will now break, so consider either preserving the union semantics or introducing a separate `ANY` flag for the new behavior.
- The special-casing of `PlatformAdapterType.ALL` in `filter()` effectively turns it into "always match", but `ALL` itself no longer represents all concrete adapter bits; it may be clearer and less error-prone to keep `ALL` as a true union of all adapter flags and have callers explicitly pass `None` or a dedicated value to mean "no filtering".
- Given that `filter()` is annotated to return `bool`, you might want to make its return paths explicit (e.g., defaulting to `False` when `platform_type` is `None` or the adapter name is unknown) to avoid implicit `None` returns and make the behavior easier to reason about.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/filter/platform_adapter_type.py" line_range="65-69" />
<code_context>
self.platform_type = platform_adapter_type_or_str
def filter(self, event: AstrMessageEvent, cfg: AstrBotConfig) -> bool:
+ if (
+ self.platform_type is not None
+ and self.platform_type & PlatformAdapterType.ALL
+ ):
+ return True
+
adapter_name = event.get_platform_name()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The ALL-flag early return changes behavior for composite filters and may bypass future platform-specific logic.
This early return means any filter whose mask includes `PlatformAdapterType.ALL` will always accept the event and skip the rest of the method, making `ALL | QQOFFICIAL` equivalent to `ALL` and bypassing any future per-platform checks. If `ALL` is meant as “no restriction”, a distinct sentinel (not a bit) might be clearer; if it should remain a true bitmask of all platforms, you may want to avoid short‑circuiting here so that callers can still express combinations like "all except X" in the future.
Suggested implementation:
```python
def filter(self, event: AstrMessageEvent, cfg: AstrBotConfig) -> bool:
# If the filter is explicitly "ALL", accept any platform without restriction.
# For composite masks (e.g., ALL | QQOFFICIAL), we still run the per-platform logic below.
if self.platform_type == PlatformAdapterType.ALL:
return True
adapter_name = event.get_platform_name()
```
```python
if adapter_name in ADAPTER_NAME_2_TYPE and self.platform_type is not None:
return bool(ADAPTER_NAME_2_TYPE[adapter_name] & self.platform_type)
```
If `PlatformAdapterType.ALL` is *not* currently defined as a standalone value but rather as a bitmask of all platforms (e.g., `QQOFFICIAL | DISCORD | ...`), you may want to:
1. Consider introducing a distinct sentinel for “no restriction” (e.g., `PlatformAdapterType.UNRESTRICTED`) and use that in this equality check instead of `ALL`.
2. Audit call sites setting `platform_type` to ensure they use the intended “no restriction” sentinel (or `ALL`) consistently.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if ( | ||
| self.platform_type is not None | ||
| and self.platform_type & PlatformAdapterType.ALL | ||
| ): | ||
| return True |
There was a problem hiding this comment.
suggestion (bug_risk): The ALL-flag early return changes behavior for composite filters and may bypass future platform-specific logic.
This early return means any filter whose mask includes PlatformAdapterType.ALL will always accept the event and skip the rest of the method, making ALL | QQOFFICIAL equivalent to ALL and bypassing any future per-platform checks. If ALL is meant as “no restriction”, a distinct sentinel (not a bit) might be clearer; if it should remain a true bitmask of all platforms, you may want to avoid short‑circuiting here so that callers can still express combinations like "all except X" in the future.
Suggested implementation:
def filter(self, event: AstrMessageEvent, cfg: AstrBotConfig) -> bool:
# If the filter is explicitly "ALL", accept any platform without restriction.
# For composite masks (e.g., ALL | QQOFFICIAL), we still run the per-platform logic below.
if self.platform_type == PlatformAdapterType.ALL:
return True
adapter_name = event.get_platform_name() if adapter_name in ADAPTER_NAME_2_TYPE and self.platform_type is not None:
return bool(ADAPTER_NAME_2_TYPE[adapter_name] & self.platform_type)If PlatformAdapterType.ALL is not currently defined as a standalone value but rather as a bitmask of all platforms (e.g., QQOFFICIAL | DISCORD | ...), you may want to:
- Consider introducing a distinct sentinel for “no restriction” (e.g.,
PlatformAdapterType.UNRESTRICTED) and use that in this equality check instead ofALL. - Audit call sites setting
platform_typeto ensure they use the intended “no restriction” sentinel (orALL) consistently.
There was a problem hiding this comment.
Code Review
This pull request adds several new platform adapter types to the PlatformAdapterType enum and updates the corresponding name mapping. Feedback identifies a logic issue where defining the ALL flag using enum.auto() prevents it from functioning as a proper bitmask, which in turn makes the new conditional check in the filter method redundant. It is also suggested to add an explicit mapping for the 'all' string to support configuration-based lookups.
| WEIXIN_OC = enum.auto() | ||
| MATTERMOST = enum.auto() | ||
| WEBCHAT = enum.auto() | ||
| ALL = enum.auto() |
There was a problem hiding this comment.
In an enum.Flag, using enum.auto() for ALL makes it a single distinct bit rather than a bitmask of all other members. This breaks the expected behavior of a "match all" flag when performing bitwise operations (e.g., ALL ^ TELEGRAM would still match TELEGRAM because the ALL bit is checked separately in the filter method). It is better to define ALL as a bitmask OR of all other platform members.
ALL = (
AIOCQHTTP
| QQOFFICIAL
| QQOFFICIAL_WEBHOOK
| TELEGRAM
| WECOM
| WECOM_AI_BOT
| LARK
| DINGTALK
| DISCORD
| SLACK
| KOOK
| VOCECHAT
| WEIXIN_OFFICIAL_ACCOUNT
| SATORI
| MISSKEY
| LINE
| MATRIX
| WEIXIN_OC
| MATTERMOST
| WEBCHAT
)| "webchat": PlatformAdapterType.WEBCHAT, | ||
| } |
There was a problem hiding this comment.
The string "all" is missing from the ADAPTER_NAME_2_TYPE mapping. Adding it allows users to specify all platforms in configurations that rely on string-to-type conversion.
| "webchat": PlatformAdapterType.WEBCHAT, | |
| } | |
| "webchat": PlatformAdapterType.WEBCHAT, | |
| "all": PlatformAdapterType.ALL, | |
| } |
| if ( | ||
| self.platform_type is not None | ||
| and self.platform_type & PlatformAdapterType.ALL | ||
| ): | ||
| return True | ||
|
|
There was a problem hiding this comment.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add missing platform adapter types and update platform filtering logic to handle them consistently.
New Features:
Bug Fixes: