fix: scope the default session waiter identity to the sender - #9753
Open
icyaaaww wants to merge 1 commit into
Open
fix: scope the default session waiter identity to the sender#9753icyaaaww wants to merge 1 commit into
icyaaaww wants to merge 1 commit into
Conversation
DefaultSessionFilter keyed sessions on unified_msg_origin alone, which is identical for every member of a group chat. Any member's next message therefore hit a waiter another member had registered: the message was stopped, given a synthetic At component and re-dispatched, so the bot replied to the wrong user with the wrong context. Keying on unified_msg_origin plus sender_id closes that hole without reintroducing AstrBotDevs#1326, where a sender_id-only key let one user's waiter capture their messages in other sessions. This also restores the behaviour the session-control guide documents. Fixes AstrBotDevs#9377
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="tests/unit/test_session_waiter.py" line_range="107-116" />
<code_context>
+@pytest.mark.asyncio
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the timeout/cleanup path of SessionWaiter
This test only covers the success path where the owner’s follow-up arrives and the waiter fires, ending with `USER_SESSIONS` empty. Please add a separate async test for the timeout path: register a waiter with a short timeout, ensure no matching follow-up arrives, then assert that after the timeout (1) the waiter is removed from `USER_SESSIONS`, and (2) non‑owner messages during the wait do not trigger the waiter. That will exercise both success and timeout branches.
Suggested implementation:
```python
@session_waiter(timeout=5)
@pytest.mark.asyncio
async def test_waiter_times_out_and_ignores_non_owner_messages():
"""A registered waiter times out cleanly and ignores non-owner messages during the wait."""
USER_SESSIONS.clear()
session_filter = DefaultSessionFilter()
owner_event = make_event("member_a", "group_1", text="@bot")
non_owner_event = make_event("member_b", "group_1", text="@bot")
triggered: list[str] = []
@session_waiter(timeout=0.1)
async def handler(session, event):
triggered.append(event.message.text)
# Register the waiter via the owner event
await handler(session_filter.filter(owner_event), owner_event)
# Ensure a session is registered for the owner and no one else
assert "member_a" in USER_SESSIONS
assert "member_b" not in USER_SESSIONS
# Send a matching follow-up from a non-owner member; it must not trigger the waiter
await handler(session_filter.filter(non_owner_event), non_owner_event)
assert triggered == []
# Wait for the timeout to elapse
await asyncio.sleep(0.2)
# After timeout, the waiter is cleaned up
assert "member_a" not in USER_SESSIONS
```
1. Ensure `asyncio` is imported at the top of `tests/unit/test_session_waiter.py`:
```python
import asyncio
```
If there is an existing import section, add it there rather than creating a new one.
2. Place the new test near the other `SessionWaiter` tests (e.g., immediately after `test_waiter_ignores_other_members_and_accepts_the_owner`) to keep related coverage together. If the indentation in the snippet is only due to doc formatting, this patch will already do that; otherwise, you may need to re-indent the `@session_waiter(timeout=5)` line so it remains at the correct scope.
3. If `session_waiter` registration semantics differ (e.g., if the first call does not register the waiter), adjust the test to use the correct way of creating and registering a waiter so that `USER_SESSIONS` is populated for `"member_a"` and then cleaned up on timeout.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+107
to
+116
| @pytest.mark.asyncio | ||
| async def test_waiter_ignores_other_members_and_accepts_the_owner(): | ||
| """A registered waiter only fires for the member that created it.""" | ||
| USER_SESSIONS.clear() | ||
| session_filter = DefaultSessionFilter() | ||
| owner_event = make_event("member_a", "group_1", text="@bot") | ||
| other_event = make_event("member_b", "group_1", text="unrelated chatter") | ||
|
|
||
| triggered: list[str] = [] | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for the timeout/cleanup path of SessionWaiter
This test only covers the success path where the owner’s follow-up arrives and the waiter fires, ending with USER_SESSIONS empty. Please add a separate async test for the timeout path: register a waiter with a short timeout, ensure no matching follow-up arrives, then assert that after the timeout (1) the waiter is removed from USER_SESSIONS, and (2) non‑owner messages during the wait do not trigger the waiter. That will exercise both success and timeout branches.
Suggested implementation:
@session_waiter(timeout=5)
@pytest.mark.asyncio
async def test_waiter_times_out_and_ignores_non_owner_messages():
"""A registered waiter times out cleanly and ignores non-owner messages during the wait."""
USER_SESSIONS.clear()
session_filter = DefaultSessionFilter()
owner_event = make_event("member_a", "group_1", text="@bot")
non_owner_event = make_event("member_b", "group_1", text="@bot")
triggered: list[str] = []
@session_waiter(timeout=0.1)
async def handler(session, event):
triggered.append(event.message.text)
# Register the waiter via the owner event
await handler(session_filter.filter(owner_event), owner_event)
# Ensure a session is registered for the owner and no one else
assert "member_a" in USER_SESSIONS
assert "member_b" not in USER_SESSIONS
# Send a matching follow-up from a non-owner member; it must not trigger the waiter
await handler(session_filter.filter(non_owner_event), non_owner_event)
assert triggered == []
# Wait for the timeout to elapse
await asyncio.sleep(0.2)
# After timeout, the waiter is cleaned up
assert "member_a" not in USER_SESSIONS- Ensure
asynciois imported at the top oftests/unit/test_session_waiter.py:If there is an existing import section, add it there rather than creating a new one.import asyncio
- Place the new test near the other
SessionWaitertests (e.g., immediately aftertest_waiter_ignores_other_members_and_accepts_the_owner) to keep related coverage together. If the indentation in the snippet is only due to doc formatting, this patch will already do that; otherwise, you may need to re-indent the@session_waiter(timeout=5)line so it remains at the correct scope. - If
session_waiterregistration semantics differ (e.g., if the first call does not register the waiter), adjust the test to use the correct way of creating and registering a waiter so thatUSER_SESSIONSis populated for"member_a"and then cleaned up on timeout.
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.
Fixes #9377.
In a group chat,
DefaultSessionFilterkeyed a session waiter onevent.unified_msg_originalone. That value is identical for every member of the group, so a waiter registered by member A was matched by any other member's next message.handle_session_control_agentthen stopped that message (other plugins never saw it), inserted a syntheticAtcomponent pointing at the bot, and re-dispatched it through the pipeline — the bot answered an unrelated message from member B as if it were A's follow-up input.This is reachable in the default configuration:
platform_settings.empty_mention_waitingdefaults toTrueandplatform_settings.unique_sessiondefaults toFalse. The empty-mention waiter inastrbot/builtin_stars/astrbot/main.pyregisters a 60 s waiter without passing asession_filter, so it inherited the group-wide key.It also contradicts the documented contract.
docs/zh/dev/star/guides/session-control.mdstates that the default identity is based onsender_idand that a custom filter is required to treat a whole group as one session — the opposite of the actual behaviour.Note that reverting to a
sender_id-only key would bring back the defect #1326 fixed, where a waiter registered by one user captured that same user's messages in other groups and private chats. The key needs both parts, which is what this PR does.Modifications / 改动点
astrbot/core/utils/session_waiter.py:DefaultSessionFilter.filter()now returnsf"{event.unified_msg_origin}!{event.get_sender_id()}"instead ofevent.unified_msg_origin. Session waiters are isolated per sender and per session, so neither cross-member interception nor cross-session interception is possible. Callers that genuinely want a group-wide session keep doing what the guide already describes: pass a customSessionFilter.tests/unit/test_session_waiter.py(new): regression coverage for the default session identity.Two lines of behaviour change, no public API change, no new dependency.
Scope note: this PR fixes the cross-member interception the issue reports, and deliberately leaves the two "连带问题" items listed at the end of #9377 alone, since they are separate defects in different code paths. Their blast radius does shrink a lot once the key includes the sender: the unconditional
event.stop_event()inhandle_session_control_agentcan now only swallow a message from the member who registered the waiter (previously any member's image message), and theSessionWaiter._cleanup()pop can now only affect two waiters registered by the same member in the same session. I am happy to send follow-up PRs for either if you would like them handled separately.The default waiter becomes strictly more selective, and it moves toward the behaviour the developer guide already documents. Plugins that intentionally rely on a group-wide waiter should already be passing their own
SessionFilter, as shown in the "自定义会话 ID 算子" section of the guide.Screenshots or Test Results / 运行截图或测试结果
Verification steps
uv syncuv run pytest tests/unit/test_session_waiter.py -vuv run ruff format --check .anduv run ruff check .uv run pytest tests -qto check for regressionsNew tests, before the fix (
DefaultSessionFilterstill returningunified_msg_origin) — the group-chat interception is reproduced:member_breaching a waiter thatmember_aregistered is exactly the reported bug.New tests, after the fix:
The four cases cover: two members of one group get distinct identities; the same member in the same group is stable across messages; one member across two groups and a private chat gets three distinct identities (the #1326 guard); and an end-to-end waiter run where another member's message does not trigger the waiter while the owner's follow-up does, with
USER_SESSIONScleaned up afterwards.Lint:
Full suite:
2132 passed, 34 failed, 1 skipped. The 34 failures are pre-existing and environment-specific to my Windows development machine (file://URI drive-letter parsing, symlink creation, POSIX shell/python3invocation). I confirmed they are unrelated by stashing this change and re-running the same selection — the failure set is byte-for-byte identical:Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
No new feature — this is a bug fix for [Bug] 群聊中空提及等待器会截获其他成员的消息(session_waiter 未绑定发送者) #9377, which contains the maintainer-facing root-cause analysis.
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Prevent default session waiters from intercepting unrelated messages by isolating them per sender and message session.
Bug Fixes:
Tests: