-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: markdown keyerror or unbound error in aiocqhttp adapter #4656
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - 我发现了两个问题,并留下了一些整体反馈:
- 事件处理器现在捕获的是
BaseException,范围过于宽泛(它包括SystemExit、KeyboardInterrupt等);建议改为捕获Exception,并使用logger.exception(...),这样可以保留完整的堆栈信息以便调试。 - 四个事件处理器(
request、notice、group、private)现在重复了几乎完全相同的 try/convert/handle/log 逻辑;建议提取一个共享的辅助函数来减少重复,并确保各个处理器之间的行为保持一致。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- 事件处理器现在捕获的是 `BaseException`,范围过于宽泛(它包括 `SystemExit`、`KeyboardInterrupt` 等);建议改为捕获 `Exception`,并使用 `logger.exception(...)`,这样可以保留完整的堆栈信息以便调试。
- 四个事件处理器(`request`、`notice`、`group`、`private`)现在重复了几乎完全相同的 try/convert/handle/log 逻辑;建议提取一个共享的辅助函数来减少重复,并确保各个处理器之间的行为保持一致。
## Individual Comments
### Comment 1
<location> `astrbot/core/platform/sources/aiocqhttp/aiocqhttp_platform_adapter.py:70-79` </location>
<code_context>
+ abm = await self.convert_message(event)
+ if abm:
+ await self.handle_msg(abm)
+ except BaseException as e:
+ logger.error(f"Handle group message failed: {e}")
+ return
@self.bot.on_message("private")
async def private(event: Event):
- abm = await self.convert_message(event)
- if abm:
- await self.handle_msg(abm)
+ try:
+ abm = await self.convert_message(event)
+ if abm:
+ await self.handle_msg(abm)
+ except BaseException as e:
+ logger.error(f"Handle private message failed: {e}")
+ return
</code_context>
<issue_to_address>
**issue (bug_risk):** 在处理器中使用 `Exception` 并在记录错误日志时包含 traceback。
捕获 `BaseException` 也会拦截 `SystemExit`、`KeyboardInterrupt` 和其他非错误类信号,这通常不是你在一个长时间运行的服务中想要的行为。这里更推荐使用 `except Exception:`,并使用 `logger.exception("Handle private message failed")`,这样可以在不插入 `e` 的情况下记录完整的堆栈信息。其他几个处理器也应采用同样的模式。
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/aiocqhttp/aiocqhttp_platform_adapter.py:63` </location>
<code_context>
@self.bot.on_request()
async def request(event: Event):
- abm = await self.convert_message(event)
</code_context>
<issue_to_address>
**issue (complexity):** 建议提取一个共享的辅助协程,来封装四个事件处理器中通用的 try/except 和消息处理逻辑。
你可以在保留当前错误处理行为的同时,通过为这四个处理器提取一个共享的辅助函数,来消除重复代码和多余的嵌套。
例如:
```python
async def _safe_handle(self, event: Event, context: str):
try:
abm = await self.convert_message(event)
if not abm:
return
await self.handle_msg(abm)
except BaseException as e:
logger.error(f"Handle {context} message failed: {e}")
return
```
然后各个处理器可以变为:
```python
@self.bot.on_request()
async def request(event: Event):
await self._safe_handle(event, "request")
@self.bot.on_notice()
async def notice(event: Event):
await self._safe_handle(event, "notice")
@self.bot.on_message("group")
async def group(event: Event):
await self._safe_handle(event, "group")
@self.bot.on_message("private")
async def private(event: Event):
await self._safe_handle(event, "private")
```
这样可以保证:
- 保留新的 `try/except BaseException` 语义;
- 保留当 `abm` 为假值时跳过处理的行为;
- 保留每个处理器各自的日志信息;
同时去掉了复制粘贴的代码块和每个处理器中的一层缩进,使文件更易阅读和维护。
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据这些反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The event handlers now catch
BaseException, which is overly broad (it includesSystemExit,KeyboardInterrupt, etc.); consider catchingExceptioninstead and usinglogger.exception(...)so the stack trace is preserved for debugging. - The four event handlers (
request,notice,group,private) now duplicate nearly identical try/convert/handle/log logic; consider extracting a shared helper to reduce repetition and keep behavior consistent across handlers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The event handlers now catch `BaseException`, which is overly broad (it includes `SystemExit`, `KeyboardInterrupt`, etc.); consider catching `Exception` instead and using `logger.exception(...)` so the stack trace is preserved for debugging.
- The four event handlers (`request`, `notice`, `group`, `private`) now duplicate nearly identical try/convert/handle/log logic; consider extracting a shared helper to reduce repetition and keep behavior consistent across handlers.
## Individual Comments
### Comment 1
<location> `astrbot/core/platform/sources/aiocqhttp/aiocqhttp_platform_adapter.py:70-79` </location>
<code_context>
+ abm = await self.convert_message(event)
+ if abm:
+ await self.handle_msg(abm)
+ except BaseException as e:
+ logger.error(f"Handle group message failed: {e}")
+ return
@self.bot.on_message("private")
async def private(event: Event):
- abm = await self.convert_message(event)
- if abm:
- await self.handle_msg(abm)
+ try:
+ abm = await self.convert_message(event)
+ if abm:
+ await self.handle_msg(abm)
+ except BaseException as e:
+ logger.error(f"Handle private message failed: {e}")
+ return
</code_context>
<issue_to_address>
**issue (bug_risk):** Use `Exception` and include traceback when logging errors in handlers.
Catching `BaseException` will also intercept `SystemExit`, `KeyboardInterrupt`, and other non-error signals, which is usually not what you want in a long-running service. Prefer `except Exception:` here, and use `logger.exception("Handle private message failed")` so the full traceback is logged without interpolating `e`. The same pattern should be applied to the other handlers.
</issue_to_address>
### Comment 2
<location> `astrbot/core/platform/sources/aiocqhttp/aiocqhttp_platform_adapter.py:63` </location>
<code_context>
@self.bot.on_request()
async def request(event: Event):
- abm = await self.convert_message(event)
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a shared helper coroutine to wrap the common try/except and message-handling logic used by all four event handlers.
You can keep the new error‑handling behavior while removing the duplication and extra nesting by extracting a shared helper for the four handlers.
For example:
```python
async def _safe_handle(self, event: Event, context: str):
try:
abm = await self.convert_message(event)
if not abm:
return
await self.handle_msg(abm)
except BaseException as e:
logger.error(f"Handle {context} message failed: {e}")
return
```
Then the handlers become:
```python
@self.bot.on_request()
async def request(event: Event):
await self._safe_handle(event, "request")
@self.bot.on_notice()
async def notice(event: Event):
await self._safe_handle(event, "notice")
@self.bot.on_message("group")
async def group(event: Event):
await self._safe_handle(event, "group")
@self.bot.on_message("private")
async def private(event: Event):
await self._safe_handle(event, "private")
```
This preserves:
- The new `try/except BaseException` semantics.
- The behavior of skipping when `abm` is falsy.
- The per‑handler log messages.
But it removes the copy‑pasted blocks and one indentation level in each handler, making the file easier to scan and maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
astrbot/core/platform/sources/aiocqhttp/aiocqhttp_platform_adapter.py
Outdated
Show resolved
Hide resolved
| ), # 以防旧版本配置不存在 | ||
| ) | ||
|
|
||
| @self.bot.on_request() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): 建议提取一个共享的辅助协程,来封装四个事件处理器中通用的 try/except 和消息处理逻辑。
你可以在保留当前错误处理行为的同时,通过为这四个处理器提取一个共享的辅助函数,来消除重复代码和多余的嵌套。
例如:
async def _safe_handle(self, event: Event, context: str):
try:
abm = await self.convert_message(event)
if not abm:
return
await self.handle_msg(abm)
except BaseException as e:
logger.error(f"Handle {context} message failed: {e}")
return然后各个处理器可以变为:
@self.bot.on_request()
async def request(event: Event):
await self._safe_handle(event, "request")
@self.bot.on_notice()
async def notice(event: Event):
await self._safe_handle(event, "notice")
@self.bot.on_message("group")
async def group(event: Event):
await self._safe_handle(event, "group")
@self.bot.on_message("private")
async def private(event: Event):
await self._safe_handle(event, "private")这样可以保证:
- 保留新的
try/except BaseException语义; - 保留当
abm为假值时跳过处理的行为; - 保留每个处理器各自的日志信息;
同时去掉了复制粘贴的代码块和每个处理器中的一层缩进,使文件更易阅读和维护。
Original comment in English
issue (complexity): Consider extracting a shared helper coroutine to wrap the common try/except and message-handling logic used by all four event handlers.
You can keep the new error‑handling behavior while removing the duplication and extra nesting by extracting a shared helper for the four handlers.
For example:
async def _safe_handle(self, event: Event, context: str):
try:
abm = await self.convert_message(event)
if not abm:
return
await self.handle_msg(abm)
except BaseException as e:
logger.error(f"Handle {context} message failed: {e}")
returnThen the handlers become:
@self.bot.on_request()
async def request(event: Event):
await self._safe_handle(event, "request")
@self.bot.on_notice()
async def notice(event: Event):
await self._safe_handle(event, "notice")
@self.bot.on_message("group")
async def group(event: Event):
await self._safe_handle(event, "group")
@self.bot.on_message("private")
async def private(event: Event):
await self._safe_handle(event, "private")This preserves:
- The new
try/except BaseExceptionsemantics. - The behavior of skipping when
abmis falsy. - The per‑handler log messages.
But it removes the copy‑pasted blocks and one indentation level in each handler, making the file easier to scan and maintain.
fixes: #4635
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
提高 aiocqhttp 平台适配器在处理传入事件和 Markdown 消息时的健壮性。
Bug 修复:
Original summary in English
Summary by Sourcery
Improve robustness of the aiocqhttp platform adapter when handling incoming events and markdown messages.
Bug Fixes: