Open
Conversation
dosubot
bot
added
the
size:XL
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些总体反馈：
- 在
HeiheMessageEvent.send_streaming中，你先把generator完全消费到一个缓冲区里，然后仍然调用super().send_streaming(generator, use_fallback)。但这时传给父类的已经是一个被耗尽的 generator。建议要么这里不要再调用super().send_streaming，要么重构逻辑，让基类实现负责流式发送，以避免重复处理和语义上的困惑。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- In `HeiheMessageEvent.send_streaming`, you fully consume the `generator` into a buffer and then still call `super().send_streaming(generator, use_fallback)`, which will receive an already-exhausted generator; consider either not calling `super().send_streaming` here or restructuring to let the base implementation handle streaming to avoid double handling and confusion.
## Individual Comments
### Comment 1
<location> `astrbot/core/platform/sources/heihe/heihe_adapter.py:273-277` </location>
<code_context>
+ logger.debug("[heihe] skip non-json frame: %s", raw[:200])
+ return
+
+ if isinstance(data, list):
+ for item in data:
+ if isinstance(item, dict):
+ await self._handle_packet(item)
+ return
+ if isinstance(data, dict):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Batch packet handling does not isolate errors per item, so one bad element can drop the rest of the batch.
Because each list element is awaited directly via `_handle_packet` without its own error handling, any unexpected exception will abort processing of the remaining items. If the server can send multiple events in one frame, consider wrapping each call in `try/except` so you can log the failure and continue with later items.
```suggestion
if isinstance(data, list):
for item in data:
if isinstance(item, dict):
try:
await self._handle_packet(item)
except Exception:
logger.exception("[heihe] failed to handle packet in batch; item=%r", item)
return
```
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/heihe/heihe_adapter.py:314` </location>
<code_context>
+ keys = payload.keys()
+ return "content" in keys or "text" in keys or "message" in keys
+
+ def _convert_message(
+ self,
+ payload: Mapping[str, Any],
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting small helpers for repeated field extraction and connection cleanup to make the adapter logic more readable and maintainable without changing behavior.
You can reduce complexity without changing behavior by:
### 1. Deduplicating the repeated “first non-empty field” logic
Patterns like these appear many times:
```python
sender_id = str(
sender_data.get("id")
or sender_data.get("user_id")
or payload.get("sender_id")
or payload.get("user_id")
or "",
).strip()
```
A small helper keeps the intent in one place and makes call sites shorter and less error-prone:
```python
def _first(
self,
*mappings: Mapping[str, Any],
keys: tuple[str, ...],
default: str = "",
) -> str:
for mapping in mappings:
for k in keys:
if not mapping:
continue
v = mapping.get(k)
if v is not None and v != "":
return str(v).strip()
return default
```
Then you can simplify the field extraction:
```python
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
channel_id = self._first(
payload,
keys=("channel_id", "room_id", "chat_id", "session_id"),
)
guild_id = self._first(
payload,
keys=("guild_id", "server_id", "group_id"),
)
```
You can keep behavior identical while making `_convert_message` much easier to scan and maintain.
Similarly for timestamp and message ID you can add small helpers if desired:
```python
def _first_int(self, *values: Any, default: int | None = None) -> int | None:
for v in values:
if isinstance(v, int):
return v
return default
```
### 2. Deduplicating connection cleanup
The teardown logic is currently duplicated in `terminate` and `_connect_and_loop`’s `finally`:
```python
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None
```
Extracting a small helper keeps the shutdown path consistent:
```python
async def _cleanup_connection(self) -> None:
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None
```
Then use it in both places:
```python
async def terminate(self) -> None:
self.running = False
await self._cleanup_connection()
```
```python
async def _connect_and_loop(self) -> None:
...
try:
async for raw in websocket:
await self._handle_incoming(raw)
finally:
await self._cleanup_connection()
```
### 3. Slightly structuring `_convert_message` into focused helpers
Without changing behavior, you can pull out a couple of small “read-only” helpers to reduce nesting and make intent clearer:
```python
def _extract_sender(
self,
payload: Mapping[str, Any],
) -> tuple[str, str]:
sender_data_obj = (
payload.get("sender")
or payload.get("author")
or payload.get("user")
or {}
)
sender_data = sender_data_obj if isinstance(sender_data_obj, Mapping) else {}
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
return sender_id, sender_name
```
And use it in `_convert_message`:
```python
def _convert_message(...):
...
sender_id, sender_name = self._extract_sender(payload)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
if self.ignore_self_message and sender_id and self_id and sender_id == self_id:
return None
channel_id = self._first(payload, keys=("channel_id", "room_id", "chat_id", "session_id"))
guild_id = self._first(payload, keys=("guild_id", "server_id", "group_id"))
...
```
You can later follow the same pattern for timestamps or context (group vs private) if you want more separation, while keeping protocol flexibility as-is.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
HeiheMessageEvent.send_streaming, you fully consume thegeneratorinto a buffer and then still callsuper().send_streaming(generator, use_fallback), which will receive an already-exhausted generator; consider either not callingsuper().send_streaminghere or restructuring to let the base implementation handle streaming to avoid double handling and confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `HeiheMessageEvent.send_streaming`, you fully consume the `generator` into a buffer and then still call `super().send_streaming(generator, use_fallback)`, which will receive an already-exhausted generator; consider either not calling `super().send_streaming` here or restructuring to let the base implementation handle streaming to avoid double handling and confusion.
## Individual Comments
### Comment 1
<location> `astrbot/core/platform/sources/heihe/heihe_adapter.py:273-277` </location>
<code_context>
+ logger.debug("[heihe] skip non-json frame: %s", raw[:200])
+ return
+
+ if isinstance(data, list):
+ for item in data:
+ if isinstance(item, dict):
+ await self._handle_packet(item)
+ return
+ if isinstance(data, dict):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Batch packet handling does not isolate errors per item, so one bad element can drop the rest of the batch.
Because each list element is awaited directly via `_handle_packet` without its own error handling, any unexpected exception will abort processing of the remaining items. If the server can send multiple events in one frame, consider wrapping each call in `try/except` so you can log the failure and continue with later items.
```suggestion
if isinstance(data, list):
for item in data:
if isinstance(item, dict):
try:
await self._handle_packet(item)
except Exception:
logger.exception("[heihe] failed to handle packet in batch; item=%r", item)
return
```
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/heihe/heihe_adapter.py:314` </location>
<code_context>
+ keys = payload.keys()
+ return "content" in keys or "text" in keys or "message" in keys
+
+ def _convert_message(
+ self,
+ payload: Mapping[str, Any],
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting small helpers for repeated field extraction and connection cleanup to make the adapter logic more readable and maintainable without changing behavior.
You can reduce complexity without changing behavior by:
### 1. Deduplicating the repeated “first non-empty field” logic
Patterns like these appear many times:
```python
sender_id = str(
sender_data.get("id")
or sender_data.get("user_id")
or payload.get("sender_id")
or payload.get("user_id")
or "",
).strip()
```
A small helper keeps the intent in one place and makes call sites shorter and less error-prone:
```python
def _first(
self,
*mappings: Mapping[str, Any],
keys: tuple[str, ...],
default: str = "",
) -> str:
for mapping in mappings:
for k in keys:
if not mapping:
continue
v = mapping.get(k)
if v is not None and v != "":
return str(v).strip()
return default
```
Then you can simplify the field extraction:
```python
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
channel_id = self._first(
payload,
keys=("channel_id", "room_id", "chat_id", "session_id"),
)
guild_id = self._first(
payload,
keys=("guild_id", "server_id", "group_id"),
)
```
You can keep behavior identical while making `_convert_message` much easier to scan and maintain.
Similarly for timestamp and message ID you can add small helpers if desired:
```python
def _first_int(self, *values: Any, default: int | None = None) -> int | None:
for v in values:
if isinstance(v, int):
return v
return default
```
### 2. Deduplicating connection cleanup
The teardown logic is currently duplicated in `terminate` and `_connect_and_loop`’s `finally`:
```python
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None
```
Extracting a small helper keeps the shutdown path consistent:
```python
async def _cleanup_connection(self) -> None:
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None
```
Then use it in both places:
```python
async def terminate(self) -> None:
self.running = False
await self._cleanup_connection()
```
```python
async def _connect_and_loop(self) -> None:
...
try:
async for raw in websocket:
await self._handle_incoming(raw)
finally:
await self._cleanup_connection()
```
### 3. Slightly structuring `_convert_message` into focused helpers
Without changing behavior, you can pull out a couple of small “read-only” helpers to reduce nesting and make intent clearer:
```python
def _extract_sender(
self,
payload: Mapping[str, Any],
) -> tuple[str, str]:
sender_data_obj = (
payload.get("sender")
or payload.get("author")
or payload.get("user")
or {}
)
sender_data = sender_data_obj if isinstance(sender_data_obj, Mapping) else {}
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
return sender_id, sender_name
```
And use it in `_convert_message`:
```python
def _convert_message(...):
...
sender_id, sender_name = self._extract_sender(payload)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
if self.ignore_self_message and sender_id and self_id and sender_id == self_id:
return None
channel_id = self._first(payload, keys=("channel_id", "room_id", "chat_id", "session_id"))
guild_id = self._first(payload, keys=("guild_id", "server_id", "group_id"))
...
```
You can later follow the same pattern for timestamps or context (group vs private) if you want more separation, while keeping protocol flexibility as-is.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+273
to
+277
| if isinstance(data, list): | ||
| for item in data: | ||
| if isinstance(item, dict): | ||
| await self._handle_packet(item) | ||
| return |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): 批量处理数据包时没有针对每个元素进行错误隔离，因此某一个元素出错会导致整批后续元素都被丢弃。
目前每个列表元素都直接通过 _handle_packet 被 await，但没有单独的错误处理。一旦发生未预期的异常，就会中断剩余元素的处理。如果服务器可能在一个帧中发送多个事件，建议为每次调用包一层 try/except，这样既能记录失败日志，又能继续处理后面的元素。
Suggested change
| if isinstance(data, list): | |
| for item in data: | |
| if isinstance(item, dict): | |
| await self._handle_packet(item) | |
| return | |
| if isinstance(data, list): | |
| for item in data: | |
| if isinstance(item, dict): | |
| try: | |
| await self._handle_packet(item) | |
| except Exception: | |
| logger.exception("[heihe] failed to handle packet in batch; item=%r", item) | |
| return |
Original comment in English
suggestion (bug_risk): Batch packet handling does not isolate errors per item, so one bad element can drop the rest of the batch.
Because each list element is awaited directly via _handle_packet without its own error handling, any unexpected exception will abort processing of the remaining items. If the server can send multiple events in one frame, consider wrapping each call in try/except so you can log the failure and continue with later items.
Suggested change
| if isinstance(data, list): | |
| for item in data: | |
| if isinstance(item, dict): | |
| await self._handle_packet(item) | |
| return | |
| if isinstance(data, list): | |
| for item in data: | |
| if isinstance(item, dict): | |
| try: | |
| await self._handle_packet(item) | |
| except Exception: | |
| logger.exception("[heihe] failed to handle packet in batch; item=%r", item) | |
| return |
| keys = payload.keys() | ||
| return "content" in keys or "text" in keys or "message" in keys | ||
|
|
||
| def _convert_message( |
Contributor
There was a problem hiding this comment.
issue (complexity): 建议为重复的字段提取逻辑和连接清理逻辑抽取一些小的辅助函数，在不改变行为的前提下，让整个适配器逻辑更易读、更易维护。
你可以通过以下方式在不改变行为的前提下降低复杂度：
1. 去重“第一个非空字段”的重复逻辑
类似下面的模式出现了很多次：
sender_id = str(
sender_data.get("id")
or sender_data.get("user_id")
or payload.get("sender_id")
or payload.get("user_id")
or "",
).strip()抽一个小 helper，可以把意图集中在一个地方，也能让调用点更短、更不容易出错：
def _first(
self,
*mappings: Mapping[str, Any],
keys: tuple[str, ...],
default: str = "",
) -> str:
for mapping in mappings:
for k in keys:
if not mapping:
continue
v = mapping.get(k)
if v is not None and v != "":
return str(v).strip()
return default然后可以简化字段提取逻辑：
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
channel_id = self._first(
payload,
keys=("channel_id", "room_id", "chat_id", "session_id"),
)
guild_id = self._first(
payload,
keys=("guild_id", "server_id", "group_id"),
)这样可以在保持行为一致的情况下，让 _convert_message 更易于阅读和维护。
时间戳和消息 ID 也可以按需增加类似的小 helper：
def _first_int(self, *values: Any, default: int | None = None) -> int | None:
for v in values:
if isinstance(v, int):
return v
return default2. 去重连接清理逻辑
目前在 terminate 和 _connect_and_loop 的 finally 中有重复的资源清理逻辑：
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None抽取一个小 helper 可以让关闭路径保持一致：
async def _cleanup_connection(self) -> None:
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = None然后在两个地方复用它：
async def terminate(self) -> None:
self.running = False
await self._cleanup_connection()async def _connect_and_loop(self) -> None:
...
try:
async for raw in websocket:
await self._handle_incoming(raw)
finally:
await self._cleanup_connection()3. 轻度拆分 _convert_message 为更聚焦的 helper
在不改变行为的前提下，可以抽出几个“只读”的小 helper，减少嵌套、让意图更清晰：
def _extract_sender(
self,
payload: Mapping[str, Any],
) -> tuple[str, str]:
sender_data_obj = (
payload.get("sender")
or payload.get("author")
or payload.get("user")
or {}
)
sender_data = sender_data_obj if isinstance(sender_data_obj, Mapping) else {}
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
return sender_id, sender_name并在 _convert_message 中使用它：
def _convert_message(...):
...
sender_id, sender_name = self._extract_sender(payload)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
if self.ignore_self_message and sender_id and self_id and sender_id == self_id:
return None
channel_id = self._first(payload, keys=("channel_id", "room_id", "chat_id", "session_id"))
guild_id = self._first(payload, keys=("guild_id", "server_id", "group_id"))
...后续如果需要对时间戳或上下文（群聊 vs 私聊）做进一步拆分，也可以沿用同样的模式，同时保持当前协议兼容性不变。
Original comment in English
issue (complexity): Consider extracting small helpers for repeated field extraction and connection cleanup to make the adapter logic more readable and maintainable without changing behavior.
You can reduce complexity without changing behavior by:
1. Deduplicating the repeated “first non-empty field” logic
Patterns like these appear many times:
sender_id = str(
sender_data.get("id")
or sender_data.get("user_id")
or payload.get("sender_id")
or payload.get("user_id")
or "",
).strip()A small helper keeps the intent in one place and makes call sites shorter and less error-prone:
def _first(
self,
*mappings: Mapping[str, Any],
keys: tuple[str, ...],
default: str = "",
) -> str:
for mapping in mappings:
for k in keys:
if not mapping:
continue
v = mapping.get(k)
if v is not None and v != "":
return str(v).strip()
return defaultThen you can simplify the field extraction:
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
channel_id = self._first(
payload,
keys=("channel_id", "room_id", "chat_id", "session_id"),
)
guild_id = self._first(
payload,
keys=("guild_id", "server_id", "group_id"),
)You can keep behavior identical while making _convert_message much easier to scan and maintain.
Similarly for timestamp and message ID you can add small helpers if desired:
def _first_int(self, *values: Any, default: int | None = None) -> int | None:
for v in values:
if isinstance(v, int):
return v
return default2. Deduplicating connection cleanup
The teardown logic is currently duplicated in terminate and _connect_and_loop’s finally:
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = NoneExtracting a small helper keeps the shutdown path consistent:
async def _cleanup_connection(self) -> None:
if self.heartbeat_task:
self.heartbeat_task.cancel()
try:
await self.heartbeat_task
except asyncio.CancelledError:
pass
self.heartbeat_task = None
if self.ws:
try:
await self.ws.close()
except Exception:
pass
self.ws = NoneThen use it in both places:
async def terminate(self) -> None:
self.running = False
await self._cleanup_connection()async def _connect_and_loop(self) -> None:
...
try:
async for raw in websocket:
await self._handle_incoming(raw)
finally:
await self._cleanup_connection()3. Slightly structuring _convert_message into focused helpers
Without changing behavior, you can pull out a couple of small “read-only” helpers to reduce nesting and make intent clearer:
def _extract_sender(
self,
payload: Mapping[str, Any],
) -> tuple[str, str]:
sender_data_obj = (
payload.get("sender")
or payload.get("author")
or payload.get("user")
or {}
)
sender_data = sender_data_obj if isinstance(sender_data_obj, Mapping) else {}
sender_id = self._first(
sender_data,
payload,
keys=("id", "user_id", "sender_id"),
)
sender_name = self._first(
sender_data,
keys=("nickname", "name", "username"),
default=sender_id or "unknown",
)
return sender_id, sender_nameAnd use it in _convert_message:
def _convert_message(...):
...
sender_id, sender_name = self._extract_sender(payload)
self_id = self._first(
payload,
keys=("self_id", "bot_id"),
default=self.bot_id or self.meta().id,
)
if self.ignore_self_message and sender_id and self_id and sender_id == self_id:
return None
channel_id = self._first(payload, keys=("channel_id", "room_id", "chat_id", "session_id"))
guild_id = self._first(payload, keys=("guild_id", "server_id", "group_id"))
...You can later follow the same pattern for timestamps or context (group vs private) if you want more separation, while keeping protocol flexibility as-is.
dosubot
bot
added
the
area:platform
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
#4728
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
为小黑盒（Xiaoheihe）机器人平台新增基于 WebSocket 的平台适配器，并将其集成到平台管理器中。
新功能：
Original summary in English
Summary by Sourcery
Add a new WebSocket-based platform adapter for the Xiaoheihe (小黑盒) bot platform and integrate it into the platform manager.
New Features: