Refactor blacklist to separate stage #1891
Closed
+171
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Reviewer's Guide
此 PR 将黑名单处理重构为一个专用的管道阶段，引入了用于在运行时管理黑名单的管理员命令，使用黑名单选项扩展了配置元数据，并为黑名单强制执行添加了全面的测试。
#### Entity relationship diagram for updated blacklist config options
```mermaid
erDiagram
PLATFORM_SETTINGS {
bool enable_id_black_list
list id_blacklist
bool id_blacklist_log
}
PLATFORM_SETTINGS ||--o{ ID_BLACKLIST : contains
ID_BLACKLIST {
string id
}Class diagram for new BlacklistCheckStage and related changesclassDiagram
class BlacklistCheckStage {
+bool enable_blacklist
+list blacklist
+bool bl_log
+initialize(ctx: PipelineContext) None
+process(event: AstrMessageEvent) None | AsyncGenerator
}
BlacklistCheckStage --|> Stage
Stage <|-- BlacklistCheckStage
PipelineContext <.. BlacklistCheckStage : uses
AstrMessageEvent <.. BlacklistCheckStage : uses
Class diagram for new admin blacklist commandsclassDiagram
class Main {
+bl(event: AstrMessageEvent, uid: str=None)
+dbl(event: AstrMessageEvent, uid: str)
}
Main : context
Main : dwl(...)
Main : provider(...)
AstrMessageEvent <.. Main : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
Original review guide in EnglishReviewer's GuideThis PR refactors blacklist handling into a dedicated pipeline stage, introduces admin commands for managing the blacklist at runtime, extends configuration metadata with blacklist options, and adds comprehensive tests for blacklist enforcement. Entity relationship diagram for updated blacklist config optionserDiagram
PLATFORM_SETTINGS {
bool enable_id_black_list
list id_blacklist
bool id_blacklist_log
}
PLATFORM_SETTINGS ||--o{ ID_BLACKLIST : contains
ID_BLACKLIST {
string id
}
Class diagram for new BlacklistCheckStage and related changesclassDiagram
class BlacklistCheckStage {
+bool enable_blacklist
+list blacklist
+bool bl_log
+initialize(ctx: PipelineContext) None
+process(event: AstrMessageEvent) None | AsyncGenerator
}
BlacklistCheckStage --|> Stage
Stage <|-- BlacklistCheckStage
PipelineContext <.. BlacklistCheckStage : uses
AstrMessageEvent <.. BlacklistCheckStage : uses
Class diagram for new admin blacklist commandsclassDiagram
class Main {
+bl(event: AstrMessageEvent, uid: str=None)
+dbl(event: AstrMessageEvent, uid: str)
}
Main : context
Main : dwl(...)
Main : provider(...)
AstrMessageEvent <.. Main : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
嘿 @xunxiing - 我已经审查了你的更改 - 这里有一些反馈：
/bl和/dbl处理程序在每次调用时都会写入配置，这可能会阻塞；考虑批量处理或防抖save_config()以减少 I/O 开销。- 黑名单命令在改变条目之前，不会验证
enable_id_black_list是否已打开；添加一个保护，以防止在禁用该功能时进行修改。 - 列出黑名单的测试仅检查子字符串“黑名单”；通过断言确切的列表内容来收紧这一点，以避免误报。
AI 代理的提示
请解决此代码审查中的评论：
## 总体评论
- `/bl` 和 `/dbl` 处理程序在每次调用时都会写入配置，这可能会阻塞；考虑批量处理或防抖 `save_config()` 以减少 I/O 开销。
- 黑名单命令在改变条目之前，不会验证 `enable_id_black_list` 是否已打开；添加一个保护，以防止在禁用该功能时进行修改。
- 列出黑名单的测试仅检查子字符串“黑名单”；通过断言确切的列表内容来收紧这一点，以避免误报。
## 个别评论
### 评论 1
<location> `tests/test_pipeline.py:251` </location>
<code_context>
+ with caplog.at_level(logging.INFO):
+ await pipeline_scheduler.execute(mock_event)
+ assert any("黑名单" in message for message in caplog.messages)
+ config["platform_settings"]["id_blacklist"].remove(BLACKLIST_SESSION)
+
mock_event = FakeAstrMessageEvent.create_fake_event("test", sender_id="123")
</code_context>
<issue_to_address>
考虑添加禁用黑名单功能的测试。
请添加一个测试，其中 enable_id_black_list 为 False，以确认在禁用黑名单时，黑名单中的 ID 不会被阻止。
</issue_to_address>
### 评论 2
<location> `astrbot/core/config/default.py:28` </location>
<code_context>
"id_whitelist_log": True,
"wl_ignore_admin_on_group": True,
"wl_ignore_admin_on_friend": True,
+ "enable_id_black_list": False,
+ "id_blacklist": [],
+ "id_blacklist_log": True,
</code_context>
<issue_to_address>
考虑将默认值和模式定义保存在不同的部分，以保持关注点的清晰分离。
无需采取任何措施——这些部分服务于不同的目的（默认值与模式定义），应保持分离。
</issue_to_address>帮助我更有用！请点击每个评论上的 👍 或 👎，我将使用反馈来改进你的评论。
Original comment in English
Hey @xunxiing - I've reviewed your changes - here's some feedback:
- The
/bland/dblhandlers write the config on every call which may block; consider batching or debouncingsave_config()to reduce I/O overhead. - The blacklist commands don’t verify if
enable_id_black_listis turned on before mutating entries; add a guard to prevent modifications when the feature is disabled. - The test for listing the blacklist only checks for the substring “黑名单”; tighten this by asserting the exact list contents to avoid false positives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `/bl` and `/dbl` handlers write the config on every call which may block; consider batching or debouncing `save_config()` to reduce I/O overhead.
- The blacklist commands don’t verify if `enable_id_black_list` is turned on before mutating entries; add a guard to prevent modifications when the feature is disabled.
- The test for listing the blacklist only checks for the substring “黑名单”; tighten this by asserting the exact list contents to avoid false positives.
## Individual Comments
### Comment 1
<location> `tests/test_pipeline.py:251` </location>
<code_context>
+ with caplog.at_level(logging.INFO):
+ await pipeline_scheduler.execute(mock_event)
+ assert any("黑名单" in message for message in caplog.messages)
+ config["platform_settings"]["id_blacklist"].remove(BLACKLIST_SESSION)
+
mock_event = FakeAstrMessageEvent.create_fake_event("test", sender_id="123")
</code_context>
<issue_to_address>
Consider adding tests for disabling blacklist functionality.
Please add a test where enable_id_black_list is False to confirm that blacklisted IDs are not blocked when the blacklist is disabled.
</issue_to_address>
### Comment 2
<location> `astrbot/core/config/default.py:28` </location>
<code_context>
"id_whitelist_log": True,
"wl_ignore_admin_on_group": True,
"wl_ignore_admin_on_friend": True,
+ "enable_id_black_list": False,
+ "id_blacklist": [],
+ "id_blacklist_log": True,
</code_context>
<issue_to_address>
Consider keeping default values and schema definitions in separate sections to maintain clear separation of concerns.
No action needed—these sections serve different purposes (default values vs. schema definitions) and should remain separate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| with caplog.at_level(logging.INFO): | ||
| await pipeline_scheduler.execute(mock_event) | ||
| assert any("黑名单" in message for message in caplog.messages) | ||
| config["platform_settings"]["id_blacklist"].remove(BLACKLIST_SESSION) |
There was a problem hiding this comment.
suggestion (testing): 考虑添加禁用黑名单功能的测试。
请添加一个测试，其中 enable_id_black_list 为 False，以确认在禁用黑名单时，黑名单中的 ID 不会被阻止。
Original comment in English
suggestion (testing): Consider adding tests for disabling blacklist functionality.
Please add a test where enable_id_black_list is False to confirm that blacklisted IDs are not blocked when the blacklist is disabled.
| @@ -25,6 +25,9 @@ | |||
| "id_whitelist_log": True, | |||
| "wl_ignore_admin_on_group": True, | |||
| "wl_ignore_admin_on_friend": True, | |||
| "enable_id_black_list": False, | |||
There was a problem hiding this comment.
issue (complexity): 考虑将默认值和模式定义保存在不同的部分，以保持关注点的清晰分离。
无需采取任何措施——这些部分服务于不同的目的（默认值与模式定义），应保持分离。
Original comment in English
issue (complexity): Consider keeping default values and schema definitions in separate sections to maintain clear separation of concerns.
No action needed—these sections serve different purposes (default values vs. schema definitions) and should remain separate.
| event.set_result(MessageEventResult().message(msg)) | ||
| return | ||
|
|
||
| uid = str(uid) |
There was a problem hiding this comment.
suggestion (code-quality): 删除不必要的 int、str、float 或 bool 转换（remove-unnecessary-cast）
Suggested change
| uid = str(uid) | |
| uid = uid |
Original comment in English
suggestion (code-quality): Remove unnecessary casts to int, str, float or bool (remove-unnecessary-cast)
Suggested change
| uid = str(uid) | |
| uid = uid |
|
@sourcery-ai |
Soulter
force-pushed
the
master
branch
2 times, most recently
from
257d742 to
b1e4bff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
解决了#1293
Motivation
引入了专门的
BlacklistCheckStage，并新增了/bl和/dbl聊天命令，用于运行时管理黑名单。同时，配置元数据中新增了启用黑名单、黑名单列表及日志记录等选项；测试套件覆盖了用户 ID、群组 ID 和会话 ID 的黑名单拦截验证。Modifications
initialize()中加载黑名单配置：/bl和/dbl命令/bl [list|<ID>]：查看或添加黑名单/dbl <ID>：删除黑名单Check
requirements.txt和pyproject.toml文件相应位置。好的，这是将 pull request 总结翻译成中文的结果：
Sourcery 总结
将黑名单处理提取到专用管道阶段，并通过命令启用运行时黑名单管理，相应地更新配置模式，并为黑名单强制执行添加测试。
新功能：
BlacklistCheckStage，以便在启用黑名单时按用户、群组或会话 ID 阻止消息。enable_id_black_list、id_blacklist和id_blacklist_log。增强功能：
WhitelistCheckStage之前插入BlacklistCheckStage。id_blacklist_log后，在 INFO 级别记录消息拒绝。测试：
Original summary in English
Summary by Sourcery
Extract blacklist handling into a dedicated pipeline stage and enable runtime blacklist management via commands, update configuration schema accordingly, and add tests for blacklist enforcement.
New Features:
Enhancements:
Tests: