fix: pass image inputs through active replies#8119
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Catching
BaseExceptionaroundconvert_to_file_path()is overly broad and will also swallow interrupts/system-exiting errors; consider catchingException(or a narrower exception type) instead so truly fatal errors can still propagate. - The new image collection loop assumes
event.message_obj.messageis always an iterable of components; if this is not guaranteed by the event contract, it would be safer to guard that access or handle non-iterable values gracefully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching `BaseException` around `convert_to_file_path()` is overly broad and will also swallow interrupts/system-exiting errors; consider catching `Exception` (or a narrower exception type) instead so truly fatal errors can still propagate.
- The new image collection loop assumes `event.message_obj.message` is always an iterable of components; if this is not guaranteed by the event contract, it would be safer to guard that access or handle non-iterable values gracefully.
## Individual Comments
### Comment 1
<location path="astrbot/builtin_stars/astrbot/main.py" line_range="185-186" />
<code_context>
+ if isinstance(comp, Image):
+ try:
+ image_urls.append(await comp.convert_to_file_path())
+ except BaseException as e:
+ logger.error(f"主动回复处理图片失败: {e}")
if not conv:
</code_context>
<issue_to_address>
**issue (bug_risk):** Exception handling here is overly broad and loses traceback details.
Catching `BaseException` will also intercept `SystemExit`, `KeyboardInterrupt`, and `asyncio.CancelledError`, which can break normal shutdown and cancellation. Catch `Exception` instead, and use `logger.exception(...)` (without formatting in `e`) so the full traceback is logged:
```python
except Exception:
logger.exception("主动回复处理图片失败")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces image support for active replies by extracting image URLs from message components and passing them to the LLM request. It also includes a unit test to verify this new functionality. The reviewer recommends catching 'Exception' instead of 'BaseException' to avoid intercepting system-level signals and task cancellations during image processing.
| if isinstance(comp, Image): | ||
| try: | ||
| image_urls.append(await comp.convert_to_file_path()) | ||
| except BaseException as e: |
There was a problem hiding this comment.
Catching BaseException is generally discouraged because it includes system-level exceptions such as KeyboardInterrupt and SystemExit. In an asynchronous context, it also catches asyncio.CancelledError, which can interfere with the proper cancellation of tasks (e.g., when the bot is shutting down or a timeout occurs). It is recommended to catch Exception instead to ensure that only standard runtime errors are handled while allowing system signals and task cancellations to propagate correctly.
| except BaseException as e: | |
| except Exception as e: |
References
- When catching exceptions, use 'except Exception:' instead of a bare 'except:' or 'except BaseException:' to avoid catching system-exit exceptions like KeyboardInterrupt and asyncio.CancelledError. (link)
Summary
request_llm()asimage_urlsFixes #8085.
To verify
python -m py_compile astrbot\builtin_stars\astrbot\main.py tests\unit\test_builtin_astrbot_main.pypython -m pytest tests\unit\test_builtin_astrbot_main.py::test_active_reply_passes_image_urls_to_llm -qpython -m ruff check astrbot\builtin_stars\astrbot\main.py tests\unit\test_builtin_astrbot_main.pygit diff --checkSummary by Sourcery
Handle active reply image components by forwarding their file paths to LLM requests while preserving existing text prompt behavior.
Bug Fixes:
Tests: