fix(qq_official): remove msg_id from FRIEND_MESSAGE payload to fix proactive send failures#6673
Open
NayukiChiba wants to merge 2 commits intoAstrBotDevs:masterfrom
Open
fix(qq_official): remove msg_id from FRIEND_MESSAGE payload to fix proactive send failures#6673NayukiChiba wants to merge 2 commits intoAstrBotDevs:masterfrom
NayukiChiba wants to merge 2 commits intoAstrBotDevs:masterfrom
Conversation
Remove msg_id from payload to prevent errors with proactive tool-call path and avoid permission issues.
添加 7 个测试用例，覆盖 _send_by_session_common 中 FRIEND_MESSAGE 类型的 msg_id 处理逻辑： - 验证纯文本、图片、视频、文件私聊消息的 payload 中 msg_id 被正确移除 - 验证 GROUP_MESSAGE 类型的 payload 仍保留 msg_id - 验证 FRIEND_MESSAGE payload 中 msg_seq 正常存在 - 验证无缓存 msg_id 时提前返回的行为 Fixes AstrBotDevs#6599, AstrBotDevs#6670 Ref: AstrBotDevs#6604
dosubot
bot
added
the
size:L
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在解决 QQ 官方适配器中，定时任务通过 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
bot
added
the
area:platform
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The behavior where FRIEND_MESSAGE proactively returns early when no cached msg_id exists is only documented in the test docstring; consider adding an explicit comment (or TODO) in the adapter implementation around that early-return branch so future maintainers understand this limitation without having to read the tests.
- The four FRIEND_MESSAGE tests that assert msg_id removal for plain/image/video/file are very similar; you could reduce duplication and make them easier to maintain by parametrizing over content type and expected media handling instead of repeating the setup/patching logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The behavior where FRIEND_MESSAGE proactively returns early when no cached msg_id exists is only documented in the test docstring; consider adding an explicit comment (or TODO) in the adapter implementation around that early-return branch so future maintainers understand this limitation without having to read the tests.
- The four FRIEND_MESSAGE tests that assert msg_id removal for plain/image/video/file are very similar; you could reduce duplication and make them easier to maintain by parametrizing over content type and expected media handling instead of repeating the setup/patching logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: AstrBotTeam's Space pr4697的改动View Suggested Changes@@ -442,6 +442,33 @@
- **普通文本回复**：对于普通文本回复，可以直接在对话中输出，无需使用此工具
> **注意**：在 `skills_like` 工具调用模式下，该工具描述已明确列出所有支持的消息类型，确保 AI 能够正确理解该工具支持发送多媒体消息，避免使用 Python 工具等绕弯路的方式处理媒体文件。
+
+#### 平台特性说明
+
+##### QQ 官方平台好友消息支持（PR #6673）
+[PR #6673](https://github.com/AstrBotDevs/AstrBot/pull/6673) 修复了 QQ 官方平台在主动向 QQ 好友发送消息（FRIEND_MESSAGE 类型）时的问题，确保定时任务和后台任务场景下的消息发送功能正常工作。
+
+**问题背景**
+
+修复前，当通过定时任务或后台任务使用 `send_message_to_user` 工具向 QQ 好友主动发送消息时，系统会因 payload 中携带过期的 `msg_id` 而触发 QQ API 错误："请求参数msg_id无效或越权"。该问题不仅影响媒体消息（图片、视频、文件），也影响纯文本消息和所有其他消息类型。
+
+**技术修复**
+
+根据 [QQ 官方文档](https://bot.q.qq.com/wiki/develop/pythonsdk/api/message/post_message.html)，QQ API 在 `msg_id` 参数缺失时会将消息视为主动推送。对于好友私聊消息（C2C 消息），主动推送是允许的。PR #6673 的修复方案是：
+
+- 在 QQ 官方平台适配器的 `FRIEND_MESSAGE` 分支入口处统一移除 `msg_id` 参数
+- 确保所有通过 `send_by_session` 路径发送的好友消息均作为主动推送处理
+- 该修复仅影响主动推送路径，不影响正常的即时回复（即时回复通过 `QQOfficialMessageEvent._post_send()` 路径处理）
+
+**修复范围**
+
+修复后，QQ 官方平台现已支持在以下场景下可靠地向 QQ 好友主动发送消息：
+
+- **定时任务（Cron Job）**：通过 `send_message_to_user` 工具定时向好友推送消息
+- **后台任务**：子代理后台任务完成后主动通知用户
+- **所有消息类型**：纯文本、图片、视频、文件、语音等所有消息类型均正常工作
+
+该修复确保 QQ 官方平台在主动代理系统中的功能完整性，与 Telegram、Slack、Discord 等其他平台保持一致的主动消息推送能力。
#### 使用示例
在定时任务触发后，主动调用 `send_message_to_user` 工具，将生成的日报、图片或其他内容直接发送给用户。Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Comment on lines
+64
to
+306
| class TestFriendMessageMsgIdRemoval: | ||
| """Verify msg_id is removed from FRIEND_MESSAGE payloads.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_plain_text_friend_message_should_not_contain_msg_id(self): | ||
| """Plain text c2c message payload must NOT contain msg_id. | ||
|
|
||
| On master (before the fix), the payload still has msg_id for plain | ||
| text friend messages. This causes the QQ API to reject the message | ||
| with a '请求参数msg_id无效或越权' error when the msg_id is stale. | ||
| """ | ||
| adapter = _build_adapter() | ||
| session_id = "test-user-openid" | ||
| stale_msg_id = "STALE_MSG_ID_FROM_HOURS_AGO" | ||
|
|
||
| # Simulate a cached (stale) msg_id — this is what the cron task sees | ||
| adapter.remember_session_message_id(session_id, stale_msg_id) | ||
| adapter.remember_session_scene(session_id, "friend") | ||
|
|
||
| session = _build_session(session_id, MessageType.FRIEND_MESSAGE) | ||
| chain = _plain_chain("Hello from cron task!") | ||
|
|
||
| captured_payloads: list[dict] = [] | ||
|
|
||
| async def mock_post_c2c_message(send_helper, openid, **kwargs): | ||
| captured_payloads.append(kwargs) | ||
| return {"id": "new_msg_id_123"} | ||
|
|
||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.post_c2c_message", | ||
| side_effect=mock_post_c2c_message, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent._parse_to_qqofficial", | ||
| return_value=( | ||
| "Hello from cron task!", | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| ), | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.platform.Platform.send_by_session", | ||
| new_callable=AsyncMock, | ||
| ): | ||
| await adapter._send_by_session_common(session, chain) | ||
|
|
||
| assert len(captured_payloads) == 1, "Expected exactly one post_c2c_message call" | ||
| payload = captured_payloads[0] | ||
| assert "msg_id" not in payload, ( | ||
| f"msg_id should have been removed from FRIEND_MESSAGE payload, " | ||
| f"but found msg_id={payload.get('msg_id')!r}. " | ||
| f"This is the bug described in #6599 and #6670." | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_image_friend_message_should_not_contain_msg_id(self): | ||
| """Image c2c message payload must NOT contain msg_id.""" | ||
| adapter = _build_adapter() | ||
| session_id = "test-user-openid" | ||
| stale_msg_id = "STALE_MSG_ID_FROM_HOURS_AGO" | ||
|
|
||
| adapter.remember_session_message_id(session_id, stale_msg_id) | ||
| adapter.remember_session_scene(session_id, "friend") | ||
|
|
||
| session = _build_session(session_id, MessageType.FRIEND_MESSAGE) | ||
| chain = MessageChain( | ||
| chain=[ | ||
| Plain(text="Check this image"), | ||
| Image(file="http://example.com/img.png"), | ||
| ] | ||
| ) | ||
|
|
||
| captured_payloads: list[dict] = [] | ||
|
|
||
| async def mock_post_c2c_message(send_helper, openid, **kwargs): | ||
| captured_payloads.append(kwargs) | ||
| return {"id": "new_msg_id_456"} | ||
|
|
||
| mock_media = MagicMock() | ||
|
|
||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.post_c2c_message", | ||
| side_effect=mock_post_c2c_message, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent._parse_to_qqofficial", | ||
| return_value=( | ||
| "Check this image", | ||
| "base64_image_data", | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| ), | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.upload_group_and_c2c_image", | ||
| new_callable=AsyncMock, | ||
| return_value=mock_media, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.platform.Platform.send_by_session", | ||
| new_callable=AsyncMock, | ||
| ): | ||
| await adapter._send_by_session_common(session, chain) | ||
|
|
||
| assert len(captured_payloads) == 1 | ||
| payload = captured_payloads[0] | ||
| assert "msg_id" not in payload, ( | ||
| f"msg_id should have been removed from image FRIEND_MESSAGE payload, " | ||
| f"but found msg_id={payload.get('msg_id')!r}" | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_video_friend_message_should_not_contain_msg_id(self): | ||
| """Video c2c message payload must NOT contain msg_id. | ||
|
|
||
| Before the fix, the video path already removed msg_id (line 271-273), | ||
| but only inside the `if media:` block. The fix moves the removal to | ||
| the top of the FRIEND_MESSAGE branch so it covers all cases. | ||
| """ | ||
| adapter = _build_adapter() | ||
| session_id = "test-user-openid" | ||
| stale_msg_id = "STALE_MSG_ID" | ||
|
|
||
| adapter.remember_session_message_id(session_id, stale_msg_id) | ||
| adapter.remember_session_scene(session_id, "friend") | ||
|
|
||
| session = _build_session(session_id, MessageType.FRIEND_MESSAGE) | ||
| chain = _plain_chain("video message") | ||
|
|
||
| captured_payloads: list[dict] = [] | ||
|
|
||
| async def mock_post_c2c_message(send_helper, openid, **kwargs): | ||
| captured_payloads.append(kwargs) | ||
| return {"id": "new_msg_id_789"} | ||
|
|
||
| mock_media = MagicMock() | ||
|
|
||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.post_c2c_message", | ||
| side_effect=mock_post_c2c_message, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent._parse_to_qqofficial", | ||
| return_value=( | ||
| "video message", | ||
| None, | ||
| None, | ||
| None, | ||
| "http://example.com/video.mp4", | ||
| None, | ||
| None, | ||
| ), | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.upload_group_and_c2c_media", | ||
| new_callable=AsyncMock, | ||
| return_value=mock_media, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.platform.Platform.send_by_session", | ||
| new_callable=AsyncMock, | ||
| ): | ||
| await adapter._send_by_session_common(session, chain) | ||
|
|
||
| assert len(captured_payloads) == 1 | ||
| payload = captured_payloads[0] | ||
| assert "msg_id" not in payload, ( | ||
| f"msg_id should have been removed from video FRIEND_MESSAGE payload, " | ||
| f"but found msg_id={payload.get('msg_id')!r}" | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_file_friend_message_should_not_contain_msg_id(self): | ||
| """File c2c message payload must NOT contain msg_id.""" | ||
| adapter = _build_adapter() | ||
| session_id = "test-user-openid" | ||
| stale_msg_id = "STALE_MSG_ID" | ||
|
|
||
| adapter.remember_session_message_id(session_id, stale_msg_id) | ||
| adapter.remember_session_scene(session_id, "friend") | ||
|
|
||
| session = _build_session(session_id, MessageType.FRIEND_MESSAGE) | ||
| chain = _plain_chain("file message") | ||
|
|
||
| captured_payloads: list[dict] = [] | ||
|
|
||
| async def mock_post_c2c_message(send_helper, openid, **kwargs): | ||
| captured_payloads.append(kwargs) | ||
| return {"id": "new_msg_id_abc"} | ||
|
|
||
| mock_media = MagicMock() | ||
|
|
||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.post_c2c_message", | ||
| side_effect=mock_post_c2c_message, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent._parse_to_qqofficial", | ||
| return_value=( | ||
| "file message", | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| "/path/to/file.pdf", | ||
| "file.pdf", | ||
| ), | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.upload_group_and_c2c_media", | ||
| new_callable=AsyncMock, | ||
| return_value=mock_media, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.platform.Platform.send_by_session", | ||
| new_callable=AsyncMock, | ||
| ): | ||
| await adapter._send_by_session_common(session, chain) | ||
|
|
||
| assert len(captured_payloads) == 1 | ||
| payload = captured_payloads[0] | ||
| assert "msg_id" not in payload, ( | ||
| f"msg_id should have been removed from file FRIEND_MESSAGE payload, " | ||
| f"but found msg_id={payload.get('msg_id')!r}" | ||
| ) |
Contributor
There was a problem hiding this comment.
在 TestFriendMessageMsgIdRemoval 这个类中，四个测试用例（test_plain_text..., test_image..., test_video..., test_file...）的结构和大部分代码都非常相似。为了减少代码重复并提高可维护性，可以考虑使用 pytest.mark.parametrize 来重构这些测试。
通过参数化，您可以将不同的消息类型（纯文本、图片、视频、文件）及其对应的 _parse_to_qqofficial 返回值和额外的 patch 作为参数传入一个通用的测试函数中。这样可以将四个测试用例合并为一个，使测试代码更简洁、更易于扩展。
Comment on lines
+410
to
+449
| class TestNoMsgIdReturnEarly: | ||
| """Verify that when there is NO cached msg_id, the adapter returns early.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_no_cached_msg_id_returns_early(self): | ||
| """If no msg_id is cached for the session, _send_by_session_common returns early. | ||
|
|
||
| This means proactive messages to users who have never messaged the bot | ||
| will silently fail. This is the existing behavior (not changed by the PR). | ||
| """ | ||
| adapter = _build_adapter() | ||
| session_id = "never-messaged-user" | ||
| # Do NOT call remember_session_message_id | ||
|
|
||
| session = _build_session(session_id, MessageType.FRIEND_MESSAGE) | ||
| chain = _plain_chain("This should not be sent") | ||
|
|
||
| mock_post = AsyncMock() | ||
|
|
||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent.post_c2c_message", | ||
| mock_post, | ||
| ): | ||
| with patch( | ||
| "astrbot.core.platform.sources.qqofficial.qqofficial_message_event" | ||
| ".QQOfficialMessageEvent._parse_to_qqofficial", | ||
| return_value=( | ||
| "This should not be sent", | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| None, | ||
| ), | ||
| ): | ||
| await adapter._send_by_session_common(session, chain) | ||
|
|
||
| mock_post.assert_not_called() |
Contributor
There was a problem hiding this comment.
这个测试用例正确地验证了当前代码的行为：如果没有缓存的 msg_id，即使是 FRIEND_MESSAGE 也不会被发送。然而，在本次 PR 移除了 FRIEND_MESSAGE 的 msg_id 之后，这个前置检查（if not msg_id: return）实际上阻止了向新用户（即从未与机器人互动过的用户）发送主动消息。
既然 FRIEND_MESSAGE 的主动消息不再需要 msg_id，可以考虑调整 _send_by_session_common 中的逻辑，仅对非 FRIEND_MESSAGE 类型的消息进行 msg_id 存在性检查。
这将使此修复更加完善，能够支持真正意义上的主动好友消息。如果采纳此建议，这个测试用例也需要相应地修改，以验证在没有缓存 msg_id 的情况下，post_c2c_message 会被 调用，并且其 payload 中不包含 msg_id。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Modifications / 改动点
基于 #6604 的修复，并补充了单元测试。
问题
在定时任务（cron job）中调用
send_message_to_user工具向 QQ 好友发送私聊消息时，由于 payload 中携带了过期的 msg_id，QQ API 返回
请求参数msg_id无效或越权错误。修复
在 _send_by_session_common 的
FRIEND_MESSAGE分支入口处统一移除 msg_id，使消息被视为主动推送。此修改仅影响主动推送路径（send_by_session），
正常即时回复走的是
QQOfficialMessageEvent._post_send()路径，不受影响。测试
新增 tests/unit/test_qqofficial_msg_id_fix.py，包含 7 个测试用例：
Summary by Sourcery
Ensure QQ Official proactive friend messages are treated as push messages by removing msg_id from their payloads and add regression tests around msg_id handling.
Bug Fixes:
Tests: