fix: unique session config cannot be applied in non-default astrbot config #4232
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
dosubot
bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些高层次的反馈：
- 在
WakingCheckStage.initialize中，直接访问self.ctx.astrbot_config["platform_settings"]会在配置缺少该键时抛出KeyError；可以考虑先使用.get("platform_settings", {})，再调用.get("unique_session", False)。 - 在
WakingCheckStage.process中对unique_session的处理使用了一长串if event.get_platform_name() == ...分支；可以考虑把各平台特定的 session ID 构造逻辑移动到一个映射或按平台划分的辅助函数中，以减少重复并让扩展更容易。
给 AI Agent 的提示
请根据这次代码审查中的评论进行修改：
## 总体评论
- 在 `WakingCheckStage.initialize` 中，直接访问 `self.ctx.astrbot_config["platform_settings"]` 会在配置缺少该键时抛出 `KeyError`；可以考虑先使用 `.get("platform_settings", {})`，再调用 `.get("unique_session", False)`。
- 在 `WakingCheckStage.process` 中对 `unique_session` 的处理使用了一长串 `if event.get_platform_name() == ...` 分支；可以考虑把各平台特定的 session ID 构造逻辑移动到一个映射或按平台划分的辅助函数中，以减少重复并让扩展更容易。
## 逐条评论
### Comment 1
<location> `astrbot/core/pipeline/waking_check/stage.py:57-58` </location>
<code_context>
self.disable_builtin_commands = self.ctx.astrbot_config.get(
"disable_builtin_commands", False
)
+ self.unique_session = self.ctx.astrbot_config["platform_settings"].get(
+ "unique_session",
+ False,
+ )
</code_context>
<issue_to_address>
**issue:** 请为 `platform_settings` 使用更安全的访问方式，以避免在缺少该配置时出现 KeyError。
这里假设 `astrbot_config` 一定包含 `platform_settings`。如果省略了该配置项，pipeline 初始化会抛出 `KeyError`，而 `disable_builtin_commands` 的读取方式则是防御性的。你可以先为字典提供默认值来避免这个问题：
```python
platform_settings = self.ctx.astrbot_config.get("platform_settings", {})
self.unique_session = platform_settings.get("unique_session", False)
```
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/misskey/misskey_adapter.py:641-644` </location>
<code_context>
sender_info,
self.client_self_id,
is_chat=False,
- unique_session=self.unique_session,
)
cache_user_info(
</code_context>
<issue_to_address>
**issue (bug_risk):** 在这里去掉 `unique_session` 会改变 Misskey 房间 `session_id` 的格式，与之前的 `create_base_message` 行为不一致。
如果没有 `unique_session=True`，房间的 `session_id` 会从 `room%{room_id}_{sender_id}` 变为 `room%{room_id}`，而后续 Misskey 的 `waking_check` 重写后会生成 `{group_id}_{sender_id}`，而不是 `room%{room_id}_{sender_id}`。这样会丢失 Misskey 房间 session 上的 `room%` 前缀。如果任何下游消费者（状态、路由、已有数据）依赖该前缀，这可能会破坏兼容性，或与非房间的群组 ID 产生冲突。请确认该前缀是否仍然是必需的；如果是，请在当前 pipeline 中保留它，或者在调整 `waking_check` 时仍保持原有的 `session_id` 结构。
</issue_to_address>
### Comment 3
<location> `astrbot/core/pipeline/waking_check/stage.py:66` </location>
<code_context>
self,
event: AstrMessageEvent,
) -> None | AsyncGenerator[None, None]:
+ # apply unique session
+ if self.unique_session and event.message_obj.type == MessageType.GROUP_MESSAGE:
+ if event.get_platform_name() in ["aiocqhttp", "slack"]:
</code_context>
<issue_to_address>
**issue (complexity):** 建议把 unique-session 的平台分支逻辑抽取到一个专门的 helper/dispatcher 中，这样可以保持 `process` 足够小，并且在平台增多时更易扩展。
你可以保留这个新特性，但通过解耦各平台分支逻辑，降低 `process` 的复杂度和未来的增长。
### 1. 用一个小的 dispatcher 替代 `if/elif` 链
把 session-id 的逻辑移到一个专门的 helper（可以是该类的方法，也可以是模块级函数）中。这样可以保持 `process` 的简单性，并把平台规则集中管理：
```python
# at module level
SESSION_ID_BUILDERS: dict[str, callable] = {
"aiocqhttp": lambda e: f"{e.get_sender_id()}_{e.get_group_id()}",
"slack": lambda e: f"{e.get_sender_id()}_{e.get_group_id()}",
"dingtalk": lambda e: e.get_sender_id(),
"qq_official": lambda e: e.get_sender_id(),
"qq_official_webhook": lambda e: e.get_sender_id(),
"lark": lambda e: f"{e.get_sender_id()}%{e.get_group_id()}",
"misskey": lambda e: f"{e.get_group_id()}_{e.get_sender_id()}",
"wechatpadpro": lambda e: f"{e.get_group_id()}#{e.get_sender_id()}",
}
def build_unique_session_id(event: AstrMessageEvent) -> str | None:
platform = event.get_platform_name()
builder = SESSION_ID_BUILDERS.get(platform)
return builder(event) if builder else None
```
然后 `process` 可以变为：
```python
async def process(
self,
event: AstrMessageEvent,
) -> None | AsyncGenerator[None, None]:
if self.unique_session and event.message_obj.type == MessageType.GROUP_MESSAGE:
sid = build_unique_session_id(event)
if sid is not None:
event.session_id = sid
# existing logic continues...
```
好处：
- `process` 不再会随着平台数量增加而不断膨胀。
- 各平台特定规则被集中管理、相互独立。
- 新增平台时只需要在 `SESSION_ID_BUILDERS` 中增加一行，而不是再写一个分支。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据反馈改进之后的 review。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
WakingCheckStage.initialize, accessingself.ctx.astrbot_config["platform_settings"]directly can raise aKeyErrorfor configs without that key; consider using.get("platform_settings", {})before calling.get("unique_session", False). - The
unique_sessionhandling inWakingCheckStage.processhas a long chain ofif event.get_platform_name() == ...branches; consider moving the platform-specific session ID construction into a mapping or a per-platform helper to reduce duplication and make it easier to extend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WakingCheckStage.initialize`, accessing `self.ctx.astrbot_config["platform_settings"]` directly can raise a `KeyError` for configs without that key; consider using `.get("platform_settings", {})` before calling `.get("unique_session", False)`.
- The `unique_session` handling in `WakingCheckStage.process` has a long chain of `if event.get_platform_name() == ...` branches; consider moving the platform-specific session ID construction into a mapping or a per-platform helper to reduce duplication and make it easier to extend.
## Individual Comments
### Comment 1
<location> `astrbot/core/pipeline/waking_check/stage.py:57-58` </location>
<code_context>
self.disable_builtin_commands = self.ctx.astrbot_config.get(
"disable_builtin_commands", False
)
+ self.unique_session = self.ctx.astrbot_config["platform_settings"].get(
+ "unique_session",
+ False,
+ )
</code_context>
<issue_to_address>
**issue:** Use a safer access pattern for `platform_settings` to avoid KeyError in configs that omit it.
This assumes `astrbot_config` always has `platform_settings`. If that section is omitted, pipeline init will raise a `KeyError`, unlike `disable_builtin_commands` which is read defensively. You can avoid this by first defaulting the dict:
```python
platform_settings = self.ctx.astrbot_config.get("platform_settings", {})
self.unique_session = platform_settings.get("unique_session", False)
```
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/misskey/misskey_adapter.py:641-644` </location>
<code_context>
sender_info,
self.client_self_id,
is_chat=False,
- unique_session=self.unique_session,
)
cache_user_info(
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping `unique_session` here changes Misskey room session_id format vs previous `create_base_message` behavior.
Without `unique_session=True`, room `session_id` changes from `room%{room_id}_{sender_id}` to `room%{room_id}`, and the later Misskey `waking_check` rewrite produces `{group_id}_{sender_id}` instead of `room%{room_id}_{sender_id}`. This drops the `room%` prefix for Misskey room sessions. If any downstream consumers (state, routing, existing data) depend on that prefix, this may break compatibility or collide with non-room group IDs. Please confirm whether the prefix is still required and, if so, either keep it in this pipeline or adjust `waking_check` while preserving the previous `session_id` shape.
</issue_to_address>
### Comment 3
<location> `astrbot/core/pipeline/waking_check/stage.py:66` </location>
<code_context>
self,
event: AstrMessageEvent,
) -> None | AsyncGenerator[None, None]:
+ # apply unique session
+ if self.unique_session and event.message_obj.type == MessageType.GROUP_MESSAGE:
+ if event.get_platform_name() in ["aiocqhttp", "slack"]:
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the unique-session platform branching into a dedicated helper/dispatcher so `process` stays small and easier to extend as platforms grow.
You can keep the new feature but decouple the platform-specific branching to reduce complexity and future growth in `process`.
### 1. Extract a small dispatcher instead of the `if/elif` chain
Move the session-id logic into a dedicated helper (either as a method on this class or a module-level function). This keeps `process` simple and localizes platform rules:
```python
# at module level
SESSION_ID_BUILDERS: dict[str, callable] = {
"aiocqhttp": lambda e: f"{e.get_sender_id()}_{e.get_group_id()}",
"slack": lambda e: f"{e.get_sender_id()}_{e.get_group_id()}",
"dingtalk": lambda e: e.get_sender_id(),
"qq_official": lambda e: e.get_sender_id(),
"qq_official_webhook": lambda e: e.get_sender_id(),
"lark": lambda e: f"{e.get_sender_id()}%{e.get_group_id()}",
"misskey": lambda e: f"{e.get_group_id()}_{e.get_sender_id()}",
"wechatpadpro": lambda e: f"{e.get_group_id()}#{e.get_sender_id()}",
}
def build_unique_session_id(event: AstrMessageEvent) -> str | None:
platform = event.get_platform_name()
builder = SESSION_ID_BUILDERS.get(platform)
return builder(event) if builder else None
```
Then `process` becomes:
```python
async def process(
self,
event: AstrMessageEvent,
) -> None | AsyncGenerator[None, None]:
if self.unique_session and event.message_obj.type == MessageType.GROUP_MESSAGE:
sid = build_unique_session_id(event)
if sid is not None:
event.session_id = sid
# existing logic continues...
```
Benefits:
- `process` no longer grows with each platform.
- Platform-specific rules are centralized and self-contained.
- Adding a new platform is a one-line addition to `SESSION_ID_BUILDERS` instead of another branch.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
fixes: #4195
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
在唤醒检查（waking check）流水线阶段集中处理唯一会话逻辑，使
unique_session配置在各平台以及非默认 AstrBot 配置中表现一致。Bug Fixes（错误修复）:
unique_session配置。Enhancements（增强改进）:
aiocqhttp、qqofficial、qqofficial_webhook、lark、slack、wechatpadpro、dingtalk、misskey、webchat）的会话 ID 生成逻辑统一。Original summary in English
Summary by Sourcery
Centralize unique session handling in the waking check pipeline stage so that unique_session configuration applies consistently across platforms and non-default AstrBot configs.
Bug Fixes:
Enhancements: