Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
dosubot
bot
added
size:XL
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 4 个问题，并给出了一些整体性的反馈：
- 在
handle_message中，解码错误处理器仍然捕获json.JSONDecodeError并记录特定于 JSON 的日志信息，但现在代码使用的是来自satori-python-core的decode()；建议更新捕获的异常类型和日志文本，使其与新的解码器行为保持一致，从而更准确地暴露解码问题。 - 有多处代码现在默认认为某些对象一定存在且非空（例如在
SatoriPlatformEvent.__init__中的adapter.logins[0]，以及在handle_event和convert_satori_message中的event.login.user/event.user）；增加防御式检查或更清晰的不变式可以降低在连接尚未完全就绪或事件数据不完整时发生IndexError/AttributeError的风险。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handle_message`, the decoding error handler still catches `json.JSONDecodeError` and logs a JSON-specific message, but the code now uses `decode()` from `satori-python-core`; consider updating the caught exception type and log text to match the new decoder’s behavior so decoding issues are surfaced accurately.
- Several places now assume presence of objects that may be `None` or empty (e.g., `adapter.logins[0]` in `SatoriPlatformEvent.__init__`, and `event.login.user` / `event.user` in `handle_event` and `convert_satori_message`); adding defensive checks or clearer invariants would reduce the risk of `IndexError`/`AttributeError` when the connection is not fully ready or events are partial.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/satori/satori_adapter.py" line_range="297" />
<code_context>
+ else:
+ logger.debug(f"解析事件失败: {e}")
+ else:
+ self.sequence = event.sn
+ if event.type == EventType.MESSAGE_CREATED:
+ if event.user and event.user.id == event.login.user.id:
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `event.sn` being `None` before assigning to `self.sequence`.
`self.sequence` was previously always an int from the raw payload. Now it can be set from `event.sn`, which may be `None` for some servers or event types. If `self.sequence` becomes `None`, comparisons like `if self.sequence > 0:` in `send_identify` will raise `TypeError`. You could either keep the existing `self.sequence` when `event.sn` is `None`, or only update it when `event.sn` is not `None`.
</issue_to_address>
### Comment 2
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="34" />
<code_context>
) -> None:
# 更新平台元数据
- if adapter and hasattr(adapter, "logins") and adapter.logins:
- current_login = adapter.logins[0]
- platform_name = current_login.get("platform", "satori")
- user = current_login.get("user", {})
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `adapter.logins[0]` unconditionally can fail before READY is received.
The previous version guarded access to `adapter.logins`; now `adapter.logins[0]` can raise `IndexError` if the event is created before the READY frame populates `logins` or if the list is empty. Please either check that `adapter.logins` is non-empty before indexing, or define and use a safe fallback `platform_meta.id` when no login info is available.
</issue_to_address>
### Comment 3
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="216-217" />
<code_context>
+ if isinstance(component, At):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `At` conversion always sets an id, changing the previous name-only behaviour.
Previously, falsy `component.qq` produced `<at name="..."/>` with no `id`, matching Satori’s name-based mention semantics. Now `id=str(component.qq)` always sets an `id`, so `qq=None` yields `id="None"` plus `name`, which may change how mentions resolve. To preserve behaviour, only set `id` when `qq` is truthy and omit it otherwise (e.g., `E.at(id=str(component.qq), name=component.name)` vs `E.at(name=component.name)`).
</issue_to_address>
### Comment 4
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="222-231" />
<code_context>
+ if isinstance(component, Image):
</code_context>
<issue_to_address>
**issue (bug_risk):** The element constructors use `url=` but the parser reads from `.src`, which likely results in empty media URLs.
In `_convert_component_to_satori`, `Image`, `File`, `Record`, and `Video` are instantiated with `url=...`, but `parse_satori_elements` reads `item.src` for these types. If the `E.*` helpers expect `src=` (per the protocol), `url` may be ignored and `.src` will be `None`, so media elements won’t survive parsing. Please align with the actual satori-python-core signatures (e.g. use `src=`/`title=`) so `parse_satori_elements` can correctly read `item.src`.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 4 issues, and left some high level feedback:
- In
handle_message, the decoding error handler still catchesjson.JSONDecodeErrorand logs a JSON-specific message, but the code now usesdecode()fromsatori-python-core; consider updating the caught exception type and log text to match the new decoder’s behavior so decoding issues are surfaced accurately. - Several places now assume presence of objects that may be
Noneor empty (e.g.,adapter.logins[0]inSatoriPlatformEvent.__init__, andevent.login.user/event.userinhandle_eventandconvert_satori_message); adding defensive checks or clearer invariants would reduce the risk ofIndexError/AttributeErrorwhen the connection is not fully ready or events are partial.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handle_message`, the decoding error handler still catches `json.JSONDecodeError` and logs a JSON-specific message, but the code now uses `decode()` from `satori-python-core`; consider updating the caught exception type and log text to match the new decoder’s behavior so decoding issues are surfaced accurately.
- Several places now assume presence of objects that may be `None` or empty (e.g., `adapter.logins[0]` in `SatoriPlatformEvent.__init__`, and `event.login.user` / `event.user` in `handle_event` and `convert_satori_message`); adding defensive checks or clearer invariants would reduce the risk of `IndexError`/`AttributeError` when the connection is not fully ready or events are partial.
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/satori/satori_adapter.py" line_range="297" />
<code_context>
+ else:
+ logger.debug(f"解析事件失败: {e}")
+ else:
+ self.sequence = event.sn
+ if event.type == EventType.MESSAGE_CREATED:
+ if event.user and event.user.id == event.login.user.id:
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `event.sn` being `None` before assigning to `self.sequence`.
`self.sequence` was previously always an int from the raw payload. Now it can be set from `event.sn`, which may be `None` for some servers or event types. If `self.sequence` becomes `None`, comparisons like `if self.sequence > 0:` in `send_identify` will raise `TypeError`. You could either keep the existing `self.sequence` when `event.sn` is `None`, or only update it when `event.sn` is not `None`.
</issue_to_address>
### Comment 2
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="34" />
<code_context>
) -> None:
# 更新平台元数据
- if adapter and hasattr(adapter, "logins") and adapter.logins:
- current_login = adapter.logins[0]
- platform_name = current_login.get("platform", "satori")
- user = current_login.get("user", {})
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `adapter.logins[0]` unconditionally can fail before READY is received.
The previous version guarded access to `adapter.logins`; now `adapter.logins[0]` can raise `IndexError` if the event is created before the READY frame populates `logins` or if the list is empty. Please either check that `adapter.logins` is non-empty before indexing, or define and use a safe fallback `platform_meta.id` when no login info is available.
</issue_to_address>
### Comment 3
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="216-217" />
<code_context>
+ if isinstance(component, At):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `At` conversion always sets an id, changing the previous name-only behaviour.
Previously, falsy `component.qq` produced `<at name="..."/>` with no `id`, matching Satori’s name-based mention semantics. Now `id=str(component.qq)` always sets an `id`, so `qq=None` yields `id="None"` plus `name`, which may change how mentions resolve. To preserve behaviour, only set `id` when `qq` is truthy and omit it otherwise (e.g., `E.at(id=str(component.qq), name=component.name)` vs `E.at(name=component.name)`).
</issue_to_address>
### Comment 4
<location path="astrbot/core/platform/sources/satori/satori_event.py" line_range="222-231" />
<code_context>
+ if isinstance(component, Image):
</code_context>
<issue_to_address>
**issue (bug_risk):** The element constructors use `url=` but the parser reads from `.src`, which likely results in empty media URLs.
In `_convert_component_to_satori`, `Image`, `File`, `Record`, and `Video` are instantiated with `url=...`, but `parse_satori_elements` reads `item.src` for these types. If the `E.*` helpers expect `src=` (per the protocol), `url` may be ignored and `.src` will be `None`, so media elements won’t survive parsing. Please align with the actual satori-python-core signatures (e.g. use `src=`/`title=`) so `parse_satori_elements` can correctly read `item.src`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| else: | ||
| logger.debug(f"解析事件失败: {e}") | ||
| else: | ||
| self.sequence = event.sn |
There was a problem hiding this comment.
issue (bug_risk): 在将 event.sn 赋值给 self.sequence 之前，需要防止 event.sn 为 None。
之前 self.sequence 始终是来自原始负载的 int。现在它可以从 event.sn 设置，而对某些服务器或事件类型来说，event.sn 可能为 None。如果 self.sequence 变成 None，那么在 send_identify 中像 if self.sequence > 0: 这样的比较会抛出 TypeError。你可以在 event.sn 为 None 时保留现有的 self.sequence，或者只在 event.sn 非 None 时更新它。
Original comment in English
issue (bug_risk): Guard against event.sn being None before assigning to self.sequence.
self.sequence was previously always an int from the raw payload. Now it can be set from event.sn, which may be None for some servers or event types. If self.sequence becomes None, comparisons like if self.sequence > 0: in send_identify will raise TypeError. You could either keep the existing self.sequence when event.sn is None, or only update it when event.sn is not None.
| @@ -30,74 +34,67 @@ def __init__( | |||
| adapter: "SatoriPlatformAdapter", | |||
There was a problem hiding this comment.
issue (bug_risk): 无条件访问 adapter.logins[0] 在收到 READY 之前可能失败。
之前的版本对 adapter.logins 的访问做了防护；现在如果在 READY 帧填充 logins 之前就创建事件，或者列表为空时，adapter.logins[0] 会抛出 IndexError。请在取下标前检查 adapter.logins 是否非空，或者在没有登录信息时定义并使用一个安全的后备 platform_meta.id。
Original comment in English
issue (bug_risk): Accessing adapter.logins[0] unconditionally can fail before READY is received.
The previous version guarded access to adapter.logins; now adapter.logins[0] can raise IndexError if the event is created before the READY frame populates logins or if the list is empty. Please either check that adapter.logins is non-empty before indexing, or define and use a safe fallback platform_meta.id when no login info is available.
| if isinstance(component, At): | ||
| if component.qq: | ||
| return f'<at id="{component.qq}"/>' | ||
| if component.name: | ||
| return f'<at name="{component.name}"/>' | ||
|
|
||
| elif isinstance(component, Image): | ||
| try: | ||
| image_base64 = await component.convert_to_base64() | ||
| if image_base64: | ||
| return f'<img src="data:image/jpeg;base64,{image_base64}"/>' | ||
| except Exception as e: | ||
| logger.error(f"图片转换为base64失败: {e}") | ||
|
|
||
| elif isinstance(component, File): | ||
| return ( | ||
| f'<file src="{component.file}" name="{component.name or "文件"}"/>' | ||
| ) | ||
|
|
||
| elif isinstance(component, Record): | ||
| try: | ||
| record_base64 = await component.convert_to_base64() | ||
| if record_base64: | ||
| return f'<audio src="data:audio/wav;base64,{record_base64}"/>' | ||
| except Exception as e: | ||
| logger.error(f"语音转换为base64失败: {e}") | ||
|
|
||
| elif isinstance(component, Reply): | ||
| return f'<reply id="{component.id}"/>' | ||
|
|
||
| elif isinstance(component, Video): | ||
| try: | ||
| video_path_url = await component.convert_to_file_path() | ||
| if video_path_url: | ||
| return f'<video src="{video_path_url}"/>' | ||
| except Exception as e: | ||
| logger.error(f"视频文件转换失败: {e}") | ||
|
|
||
| elif isinstance(component, Forward): | ||
| return f'<message id="{component.id}" forward/>' | ||
|
|
||
| # 对于其他未处理的组件类型，返回空字符串 | ||
| return "" | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"转换消息组件失败: {e}") | ||
| return "" | ||
|
|
||
| async def _convert_node_to_satori(self, node: Node) -> str: | ||
| """将单个转发节点转换为 Satori 格式""" | ||
| try: | ||
| content_parts = [] | ||
| if node.content: | ||
| for content_component in node.content: | ||
| component_content = await self._convert_component_to_satori( | ||
| content_component, | ||
| ) | ||
| if component_content: | ||
| content_parts.append(component_content) | ||
|
|
||
| content = "".join(content_parts) | ||
| return E.at(str(component.qq), name=component.name) |
There was a problem hiding this comment.
suggestion (bug_risk): At 的转换现在总是设置 id，改变了之前仅使用名称的行为。
之前，当 component.qq 为假值时，会生成没有 id 的 <at name="..."/>，这与 Satori 基于名称的提及语义相匹配。现在 id=str(component.qq) 总是会设置 id，因此 qq=None 会得到带有 id="None" 和 name 的元素，这可能会改变提及的解析方式。为保持原有行为，建议只在 qq 为真值时设置 id，否则省略它（例如在 qq 有效时使用 E.at(id=str(component.qq), name=component.name)，否则使用 E.at(name=component.name)）。
Original comment in English
suggestion (bug_risk): At conversion always sets an id, changing the previous name-only behaviour.
Previously, falsy component.qq produced <at name="..."/> with no id, matching Satori’s name-based mention semantics. Now id=str(component.qq) always sets an id, so qq=None yields id="None" plus name, which may change how mentions resolve. To preserve behaviour, only set id when qq is truthy and omit it otherwise (e.g., E.at(id=str(component.qq), name=component.name) vs E.at(name=component.name)).
| if isinstance(component, Image): | ||
| try: | ||
| image_base64 = await component.convert_to_base64() | ||
| if image_base64: | ||
| return f'<img src="data:image/jpeg;base64,{image_base64}"/>' | ||
| return E.image(url=f"data:image/jpeg;base64,{image_base64}") | ||
| except Exception as e: | ||
| logger.error(f"图片转换为base64失败: {e}") | ||
|
|
||
| elif isinstance(component, File): | ||
| return ( | ||
| f'<file src="{component.file}" name="{component.name or "文件"}"/>' | ||
| ) | ||
| if isinstance(component, File): | ||
| return E.file(url=component.file, name=component.name or "文件") | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): 元素构造函数使用的是 url=，但解析器读取的是 .src，这很可能会导致媒体 URL 为空。
在 _convert_component_to_satori 中，Image、File、Record 和 Video 是通过 url=... 实例化的，但 parse_satori_elements 对这些类型读取的是 item.src。如果 E.* 帮助函数（按协议）期望的是 src=，那么 url 可能会被忽略，.src 会是 None，从而导致媒体元素在解析过程中丢失。请根据实际的 satori-python-core 函数签名进行调整（例如使用 src=/title=），以便 parse_satori_elements 能正确读取 item.src。
Original comment in English
issue (bug_risk): The element constructors use url= but the parser reads from .src, which likely results in empty media URLs.
In _convert_component_to_satori, Image, File, Record, and Video are instantiated with url=..., but parse_satori_elements reads item.src for these types. If the E.* helpers expect src= (per the protocol), url may be ignored and .src will be None, so media elements won’t survive parsing. Please align with the actual satori-python-core signatures (e.g. use src=/title=) so parse_satori_elements can correctly read item.src.
1 participant
resolve #6084
Modifications / 改动点
satori-python-core包，对 satori 适配器进行优化satori-python-core解析为 Satori 消息元素模型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.由 Sourcery 生成的总结
将
satori-python-core库集成到 Satori 平台适配器中，以便在事件、消息和 API 交互中使用类型化模型，同时改进消息/会话处理。新功能：
satori-python-core模型支持 Satori 消息的解析与构造，而不是依赖原始 JSON 和 XML 内容。增强改进：
构建：
pyproject.toml和requirements.txt中添加satori-python-core>=1.3.0作为运行时依赖。Original summary in English
Summary by Sourcery
Integrate the satori-python-core library into the Satori platform adapter to use typed models for events, messages, and API interactions while improving message/session handling.
New Features:
Enhancements:
Build: