-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix qq_offical 定时任务中 send_message_to_user 工具调用失败 #6604
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -238,6 +238,9 @@ async def _send_by_session_common( | |||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| elif session.message_type == MessageType.FRIEND_MESSAGE: | ||||||||||||||||||||||||
| # 参考 https://bot.q.qq.com/wiki/develop/pythonsdk/api/message/post_message.html | ||||||||||||||||||||||||
| # msg_id 缺失时认为是主动推送,而似乎至少在私聊上主动推送是没有被限制的,这里直接移除 msg_id 可以避免越权或 msg_id 不可用的bug | ||||||||||||||||||||||||
| payload.pop("msg_id", None) | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 同样的,推荐在有条件的时候才移除msg_id,比如离上一条消息很久才移除msg_id |
||||||||||||||||||||||||
| payload["msg_seq"] = random.randint(1, 10000) | ||||||||||||||||||||||||
|
Comment on lines
240
to
244
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Behavior and comment are inconsistent regarding "msg_id 缺失时". The comment describes special handling only when
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 注释写错了吗
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 对,你现在这段实现本身是没问题的,主要是注释的表述和实际行为有点对不上。
所以建议只是把注释改得更贴近现在的逻辑,比如: elif session.message_type == MessageType.FRIEND_MESSAGE:
# 参考 https://bot.q.qq.com/wiki/develop/pythonsdk/api/message/post_message.html
# 好友私聊统一移除 msg_id,将其视为主动推送(官方文档中,私聊场景下主动推送目前未见明显限制),
# 以避免可能的越权或 msg_id 不可用问题
payload.pop("msg_id", None)
payload["msg_seq"] = random.randint(1, 10000)你现在修的逻辑是对的,只要把注释再润色一下就更清晰了。 |
||||||||||||||||||||||||
| if image_base64: | ||||||||||||||||||||||||
| media = await QQOfficialMessageEvent.upload_group_and_c2c_image( | ||||||||||||||||||||||||
|
|
@@ -268,9 +271,6 @@ async def _send_by_session_common( | |||||||||||||||||||||||
| if media: | ||||||||||||||||||||||||
| payload["media"] = media | ||||||||||||||||||||||||
| payload["msg_type"] = 7 | ||||||||||||||||||||||||
| # QQ API rejects msg_id for media (video/file) messages sent | ||||||||||||||||||||||||
| # via the proactive tool-call path; remove it to avoid 越权 error. | ||||||||||||||||||||||||
| payload.pop("msg_id", None) | ||||||||||||||||||||||||
| if file_source: | ||||||||||||||||||||||||
| media = await QQOfficialMessageEvent.upload_group_and_c2c_media( | ||||||||||||||||||||||||
| send_helper, # type: ignore | ||||||||||||||||||||||||
|
|
@@ -282,7 +282,6 @@ async def _send_by_session_common( | |||||||||||||||||||||||
| if media: | ||||||||||||||||||||||||
| payload["media"] = media | ||||||||||||||||||||||||
| payload["msg_type"] = 7 | ||||||||||||||||||||||||
| payload.pop("msg_id", None) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| ret = await QQOfficialMessageEvent.post_c2c_message( | ||||||||||||||||||||||||
| send_helper, # type: ignore | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
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 (bug_risk): Unconditionally dropping msg_id may break reply semantics for friend messages.
By always removing
msg_idforFRIEND_MESSAGE, valid reply flows can no longer be treated as replies by the API. If the goal is only to avoid 越权 for proactive pushes whenmsg_idis missing or invalid, make this removal conditional (e.g., only whenmsg_idis absent, fails validation, or on specific proactive paths) so legitimate replies can still includemsg_id.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.
无条件移除msg_id容易会出现无法预期的行为,有条件再移除吗,这样应该要记录msg_id的时间戳,过期丢弃这种写法,有点麻烦,遇到bug再修复吧
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.
确实,要做成“有条件移除”而且判断 msg_id 是否过期的话,就得像你说的那样额外记时间戳、做过期逻辑,会复杂不少,尤其是现在只是为了解一个定时任务场景的 bug。
我这边的担心主要是两个点:
语义层面上的退化:
现在这一改动会让 QQ 订阅号的所有好友消息都走“主动推送”语义,而不再是真正意义上的“回复某条消息”,这在以后如果有人想做“精确回复/引用某条私聊消息”的功能时,可能会踩坑。
不可预期行为的来源:
不可预期的其实不是“删 msg_id”这一行为本身,而是“未来维护者/使用者不知道这里把所有好友消息都强制转成了主动推送”,导致排查问题时思路被带偏。
既然现在你更倾向于先用这一简单策略把 #6599 修掉、后面有需要再做精细化处理,那我有两个小建议,可以让后续的 debug 成本低一点:
把 trade-off 明确写在注释里(就在你现在这两行注释下面补一句 TODO 也行),类似:
在 Issue 或后续 TODO 里记录这个限制:
比如在 [Bug] qq_offical 定时任务中 send_message_to_user 工具调用失败:"请求参数msg_id无效或越权" #6599 或一个新的跟踪 issue 里写明:
is_proactive),仅在主动推送路径上移除 msg_id。这样的话,现在这版改动既能立刻解决实际问题,也把潜在坑用注释/文档标出来了,后面真遇到相关 bug 的人也比较容易顺着线索找到这里。
总之,我这边不是强阻塞意见,只是把可能的后续影响说清楚,方便你和后面的维护者做权衡。