-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: when session_id including ":" #4380
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 - 我在这里给出了一些高层次的反馈:
- 建议在
from_str中添加一些基本的校验或错误处理(例如检查分割结果的长度),这样当会话字符串格式不正确时,可以抛出比split默认产生的ValueError更清晰的错误。 - 可以在注释或文档字符串中简单说明一下:
session_id是允许包含:的,并且使用split(":", 2)是有意为之,用来确保剩余部分被保留为 session ID。
面向 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- Consider adding minimal validation or error handling in `from_str` (e.g., checking the split result length) so malformed session strings fail with a clearer error than the default `ValueError` from `split`.
- It may be helpful to briefly document in a comment or docstring that `session_id` is allowed to contain `:` and that `split(":", 2)` is intentional to preserve the remainder as the session ID.帮我变得更有用!请对每条评论点击 👍 或 👎,我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've left some high level feedback:
- Consider adding minimal validation or error handling in
from_str(e.g., checking the split result length) so malformed session strings fail with a clearer error than the defaultValueErrorfromsplit. - It may be helpful to briefly document in a comment or docstring that
session_idis allowed to contain:and thatsplit(":", 2)is intentional to preserve the remainder as the session ID.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding minimal validation or error handling in `from_str` (e.g., checking the split result length) so malformed session strings fail with a clearer error than the default `ValueError` from `split`.
- It may be helpful to briefly document in a comment or docstring that `session_id` is allowed to contain `:` and that `split(":", 2)` is intentional to preserve the remainder as the session ID.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where MessageSession.from_str() would incorrectly parse session strings when the session_id component contains colons, such as Matrix room IDs (e.g., !roomid:server.com).
Key change:
- Modified
split(":")tosplit(":", 2)to limit splitting to the first two colons, allowing thesession_idto contain colons
增强代码健全性,session_id可能包含冒号(如matrix的房间)
Modifications / 改动点
astrbot/core/platform/message_session.py
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
错误修复:
Original summary in English
Summary by Sourcery
Bug Fixes: