Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/platform/sources/qqofficial/qqofficial_platform_adapter.py" line_range="582-595" />
<code_context>
+ if self._session_scene.get(session_id) == scene:
+ return
+ if scene in ("group", "channel"):
+ try:
+ await sp.put_async(
+ "qqofficial",
+ f"{self.meta().id}:{self.appid}",
+ f"group_scene:{session_id}",
+ scene,
+ )
+ except Exception:
+ # Leave the cache unchanged so the next event retries persistence.
+ logger.exception(
+ "[QQOfficial] Failed to persist delivery scene for session %s",
+ session_id,
+ )
+ return
self._session_scene[session_id] = scene
</code_context>
<issue_to_address>
**issue (bug_risk):** When `sp.put_async` fails, `remember_session_scene` catches the exception and leaves only the in-memory route unchanged, but the caller still commits the incoming event. If the process restarts before another event for that destination arrives, the route is lost and proactive messages are again unable to resolve their delivery scene.
**Triggers:** When the SharedPreferences write fails and no subsequent message arrives for the same group or channel before a restart.
**Suggested fix:** Do not commit the event until route persistence succeeds, or enqueue a durable retry so a failed route write is retried independently of future incoming events.
```suggestion
try:
await sp.put_async(
"qqofficial",
f"{self.meta().id}:{self.appid}",
f"group_scene:{session_id}",
scene,
)
except Exception:
logger.exception(
"[QQOfficial] Failed to persist delivery scene for session %s",
session_id,
)
raise
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and a persisted group or channel route now determines which QQ API receives proactive notifications after a restart, and cached reply IDs are no longer used. If the route is wrong or stale, messages may be sent through the wrong delivery path or fail; reverting stops future misrouting but cannot recall notifications already sent.
Blocking findings: astrbot/core/platform/sources/qqofficial/qqofficial_platform_adapter.py:595
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
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
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.
Problem
QQ Official proactive group messages can be silently skipped after AstrBot restarts. The adapter currently keeps the delivery scene only in the in-memory
_session_scenemapping, so a saved UMO cannot determine whether aGROUP_MESSAGEdestination is a QQ group or a guild text channel after restart.This is the second problem described in #9831. The first problem in that issue, concerning the semantics of
/sidunderunique_session, is outside this PR's scope.Changes
SharedPreferencesstore.msg_id.Verification
ruff format .passed.ruff check .passed.git diff --checkpassed.Context.send_message()path;msg_id, returned new QQ message IDs, and were confirmed received in the QQ client.This PR is related to #9831 and addresses its second reported problem.
Summary by Sourcery
Persist QQ Official delivery routes so saved sessions can reliably send proactive group and guild-channel messages after restarts.
Bug Fixes:
Enhancements:
Tests: