Skip to content

Conversation

@Soulter
Copy link
Member

@Soulter Soulter commented Jan 24, 2026

fixes: #4635

Modifications / 改动点

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果


Checklist / 检查清单

  • 😊 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。/ If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  • 👀 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。/ My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  • 🤓 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到了 requirements.txtpyproject.toml 文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
  • 😮 我的更改没有引入恶意代码。/ My changes do not introduce malicious code.

Summary by Sourcery

提高 aiocqhttp 平台适配器在处理传入事件和 Markdown 消息时的健壮性。

Bug 修复:

  • 通过捕获并记录错误,防止 aiocqhttp 的 request、notice、group 和 private 处理器中的异常导致整个处理流程崩溃。
  • 正确处理群聊中的 Markdown 消息段,避免在构造纯文本内容时出现键错误或未绑定变量问题。
Original summary in English

Summary by Sourcery

Improve robustness of the aiocqhttp platform adapter when handling incoming events and markdown messages.

Bug Fixes:

  • Prevent exceptions in aiocqhttp request, notice, group, and private handlers from crashing processing by catching and logging errors.
  • Handle markdown message segments in groups correctly to avoid key errors or unbound variable issues when constructing plain text content.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 24, 2026
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - 我发现了两个问题,并留下了一些整体反馈:

  • 事件处理器现在捕获的是 BaseException,范围过于宽泛(它包括 SystemExitKeyboardInterrupt 等);建议改为捕获 Exception,并使用 logger.exception(...),这样可以保留完整的堆栈信息以便调试。
  • 四个事件处理器(requestnoticegroupprivate)现在重复了几乎完全相同的 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>

Sourcery 对开源项目免费——如果你觉得我们的评审有帮助,欢迎分享 ✨
帮我变得更有用!请对每条评论点 👍 或 👎,我会根据这些反馈改进后续的评审。
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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

), # 以防旧版本配置不存在
)

@self.bot.on_request()
Copy link
Contributor

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}")
        return

Then 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 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.

@dosubot dosubot bot added the area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. label Jan 24, 2026
@Soulter Soulter merged commit f7c228e into master Jan 24, 2026
5 checks passed
@Soulter Soulter deleted the fix/markdown-error branch January 24, 2026 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:platform The bug / feature is about IM platform adapter, such as QQ, Lark, Telegram, WebChat and so on. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]exception=KeyError('markdown')> aiocqhttp_platform_adapter.py

2 participants