fix: bind session_waiter to sender to prevent cross-user message interception in group chats - #9442
Open
xiaoyuyu6420 wants to merge 2 commits into
Open
Conversation
…rception in group chats (AstrBotDevs#9377) - DefaultSessionFilter now returns composite key '{unified_msg_origin}:{sender_id}' instead of only unified_msg_origin, so two distinct senders in the same group produce different session keys - Empty sender_id falls back to '<unknown>' placeholder to avoid cross-user key collision, with a warning logged - SessionFilter now inherits abc.ABC to enforce abstract method contract - _cleanup() uses identity check (is) instead of equality to prevent stale timeout cleanup from evicting a newer waiter registered under the same key - register_wait() logs a warning when overwriting an existing waiter - empty_mention_waiter now processes non-text messages (e.g. pure images) as responses instead of silently dropping them - Degenerate case (empty message_str AND empty message chain) stops the controller to prevent the waiter from lingering and intercepting messages - Added 11 unit tests covering composite key, cleanup race, non-text handling, and unique_session compatibility
dosubot
Bot
added
size:M
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
![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 found 4 issues, and left some high level feedback:
- The tests for
empty_mention_waitermanually reimplement parts of the handler logic instead of invoking the real function, which makes them brittle to future changes in the implementation; consider refactoring the handler into a callable that can be imported and exercised directly in tests. - Using the
"<unknown>"placeholder for emptysender_idstill allows cross-user collisions among all unknown senders in the same conversation; depending on how often this occurs, you might want to incorporate an additional discriminator (e.g., event ID) or explicitly disable session waiting when the sender cannot be resolved. - The
_cleanuplogic linearly scans and mutates the globalFILTERSlist on every cleanup; if many waiters are active this could become a hotspot, so consider tracking filters in a keyed structure (e.g., mapping session ID to filter) to avoid repeated list walks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests for `empty_mention_waiter` manually reimplement parts of the handler logic instead of invoking the real function, which makes them brittle to future changes in the implementation; consider refactoring the handler into a callable that can be imported and exercised directly in tests.
- Using the `"<unknown>"` placeholder for empty `sender_id` still allows cross-user collisions among all unknown senders in the same conversation; depending on how often this occurs, you might want to incorporate an additional discriminator (e.g., event ID) or explicitly disable session waiting when the sender cannot be resolved.
- The `_cleanup` logic linearly scans and mutates the global `FILTERS` list on every cleanup; if many waiters are active this could become a hotspot, so consider tracking filters in a keyed structure (e.g., mapping session ID to filter) to avoid repeated list walks.
## Individual Comments
### Comment 1
<location path="tests/unit/test_session_waiter.py" line_range="208-217" />
<code_context>
+ def test_degenerate_empty_event_stops_controller(self):
</code_context>
<issue_to_address>
**issue (testing):** The degenerate empty-event test never asserts that `controller.stop` was called, so it only partially verifies the behavior.
In `test_degenerate_empty_event_stops_controller`, the test currently only verifies that execution reaches the `return` inside the simulated handler, not that `controller.stop` is actually called. The comment about asserting this behavior doesn’t match the code. Please refactor so the handler logic is invoked via a callable (or the real handler), and then assert `controller.stop.assert_called_once()` afterwards to cover both the early return and the session termination behavior.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_session_waiter.py" line_range="89-95" />
<code_context>
+ assert "alice" in key
+ assert key == "platform:g:42:alice"
+
+ def test_empty_sender_id_uses_unknown_placeholder(self):
+ """When sender_id is empty, use '<unknown>' placeholder to avoid
+ cross-user key collision (S-F1 fix)."""
+ filter_ = DefaultSessionFilter()
+ event = _make_event(sender_id="")
+ key = filter_.filter(event)
+ assert key == "platform:group:123:<unknown>"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that a warning is logged when `sender_id` is empty to fully cover the new behavior.
This test validates the `<unknown>` placeholder, but the new behavior also logs a warning when `sender_id` is empty. To guard against future refactors dropping that warning, please capture logs (e.g., via `caplog`) and assert that the warning is emitted with the expected message when `sender_id == ""`.
Suggested implementation:
```python
import asyncio
import copy
import logging
from unittest.mock import MagicMock
```
```python
def test_empty_sender_id_uses_unknown_placeholder(self, caplog):
"""When sender_id is empty, use '<unknown>' placeholder to avoid
cross-user key collision (S-F1 fix) and log a warning."""
filter_ = DefaultSessionFilter()
event = _make_event(sender_id="")
# Capture warnings emitted during filtering
with caplog.at_level(logging.WARNING):
key = filter_.filter(event)
assert key == "platform:group:123:<unknown>"
# Assert that a warning was logged about the empty sender_id
warnings = [
record
for record in caplog.records
if record.levelno == logging.WARNING
]
assert warnings, "Expected a warning when sender_id is empty"
# Be specific about the expected warning message to guard against regressions
assert any(
"sender_id" in record.getMessage() and "<unknown>" in record.getMessage()
for record in warnings
), "Expected warning message to mention empty sender_id and '<unknown>' placeholder"
```
1. The assertions on the warning message should be aligned with the actual text used in `DefaultSessionFilter`'s logging call. Update the `"sender_id"` / `"<unknown>"` substring checks to match the precise warning message (e.g., the full string from `log.warning(...)`) once you confirm it.
2. If `DefaultSessionFilter` logs via a module-specific logger (e.g., `logger = logging.getLogger(__name__)`) and tests use a different logging configuration, you may optionally scope `caplog` with `caplog.at_level(logging.WARNING, logger="astrbot.core.utils.session_waiter")` (or the appropriate logger name) to ensure the warning is captured reliably.
</issue_to_address>
### Comment 3
<location path="tests/unit/test_session_waiter.py" line_range="109-118" />
<code_context>
+ def test_cleanup_does_not_evict_newer_waiter(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the cleanup race tests to also assert correct `FILTERS` behavior and logging when skipping stale cleanup.
Since `_cleanup` now also walks `FILTERS` and logs a warning when it skips removing a newer waiter, please add assertions that (1) the relevant filter remains in `FILTERS` and (2) the warning is emitted. This will round out coverage of the race-condition fix and guard against regressions in filter pruning or logging behavior.
Suggested implementation:
```python
def test_cleanup_does_not_evict_newer_waiter(self, caplog):
"""When a newer waiter is registered under the same key, stale cleanup
must not remove it, and associated filters/logging behave correctly."""
import logging
from astrbot.core.utils.session_waiter import SessionFilter, SessionWaiter, _cleanup, FILTERS, WAITERS
class TestFilter(SessionFilter):
def filter(self, event):
return "test:session"
# Create a filter and compute its key for the event
filt = TestFilter()
event = _make_event()
key = filt.filter(event)
# Simulate a stale waiter and then a newer waiter replacing it
stale_waiter = SessionWaiter(filt)
newer_waiter = SessionWaiter(filt)
# The newer waiter is the one actually registered under the key
WAITERS[key] = newer_waiter
FILTERS[key] = filt
# Run cleanup with the stale waiter while capturing warnings
with caplog.at_level(logging.WARNING):
_cleanup(stale_waiter)
# 1) The filter for this key must remain registered in FILTERS
assert key in FILTERS
assert FILTERS[key] is filt
# 2) A warning must be logged indicating stale cleanup was skipped
assert any(
"stale cleanup" in record.message or "skip" in record.message
for record in caplog.records
)
```
The exact API of `SessionWaiter`, `_cleanup`, `FILTERS`, and `WAITERS`, as well as the logger name and warning message, may differ slightly in your codebase. You may need to:
1. Adjust the import list to match the actual exported names from `astrbot.core.utils.session_waiter` (e.g., if `_cleanup` is a method on a class or if `FILTERS/WAITERS` live in a different module).
2. Update the `_cleanup` call signature (e.g., ` _cleanup(key, stale_waiter)` or `_cleanup()` depending on the implementation).
3. Refine the logging assertion to match the exact warning text or logger used (for example, scoping `caplog.at_level(logging.WARNING, logger="astrbot.core.utils.session_waiter")` and asserting a specific message substring).
4. If the test suite already uses a helper to access or reset `FILTERS/WAITERS`, prefer that helper instead of direct assignment in the test.
</issue_to_address>
### Comment 4
<location path="tests/unit/test_session_waiter.py" line_range="237" />
<code_context>
+# ---------------------------------------------------------------------------
+
+
+class TestUniqueSessionCompatibility:
+ """R2: Composite key must work with unique_session mode where umo already
+ encodes the sender."""
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for the `register_wait` overwrite warning when reusing a composite session key.
The current tests cover composite key behavior and cleanup races but not the warning path when a waiter is overwritten for the same session key. Please add a test that registers a `SessionWaiter`, then registers another with the same `session_id`, and asserts that the warning is logged and that the second waiter replaces the first in `USER_SESSIONS`. This will exercise the overwrite-detection logic.
Suggested implementation:
```python
# ---------------------------------------------------------------------------
# AC5: unique_session compatibility
# ---------------------------------------------------------------------------
class TestRegisterWaitOverwriteWarning:
"""Tests that re-registering a waiter for the same composite session key
logs a warning and replaces the existing waiter."""
def setup_method(self):
_clear_global_state()
def test_register_wait_logs_overwrite_warning_for_composite_key(self, caplog):
# Arrange: build a composite session id from sender + conversation
filter_ = DefaultSessionFilter()
event = _make_event(
sender_id="user-123",
conversation_id="conv-xyz",
umo={"unique_session": True},
)
# First waiter gets registered for this composite key
waiter1 = SessionWaiter(
session_id=filter_.session_id(event),
sender_id=event.sender_id,
)
register_wait(filter_, event, waiter1)
# Act: register a second waiter for the same composite session key
waiter2 = SessionWaiter(
session_id=filter_.session_id(event),
sender_id=event.sender_id,
)
with caplog.at_level(logging.WARNING):
register_wait(filter_, event, waiter2)
# Assert: warning was logged and the second waiter replaced the first
assert "overwriting existing SessionWaiter" in caplog.text
composite_key = filter_.session_id(event)
assert USER_SESSIONS[composite_key] is waiter2
class TestUniqueSessionCompatibility:
```
To fully integrate this test, you will likely need to:
1. Ensure the following imports exist at the top of the file (or add them if missing):
- `import logging`
- `from bot.session_waiter import SessionWaiter, register_wait, USER_SESSIONS` (adjust module path to match your project).
2. Align the `_make_event(...)` call with your existing test helpers:
- Use the same keyword arguments as other tests in this file (e.g. if `umo` is represented differently, or if `sender_id` / `conversation_id` fields have different names).
3. Match the logger used for the overwrite warning:
- If the warning is emitted on a specific logger (e.g. `logger = logging.getLogger("bot.session_waiter")`), change `with caplog.at_level(logging.WARNING):` to `with caplog.at_level(logging.WARNING, logger="bot.session_waiter")` or whatever name is used in the implementation.
4. Update the assertion message string to exactly match the warning text in `register_wait` (e.g. `"register_wait: overwriting existing SessionWaiter"`), so the test reliably exercises the overwrite-detection path.
5. If `SessionWaiter` requires more constructor arguments (e.g. `controller`, `timeout`, `context`), supply minimal valid values in both `waiter1` and `waiter2` to make the test pass while focusing on the overwrite behavior.
</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
+208
to
+217
| def test_degenerate_empty_event_stops_controller(self): | ||
| """When both message_str and get_messages() are empty, the handler | ||
| should stop the controller (ending the waiter session) and return.""" | ||
| event = _make_event() | ||
| event.message_str = "" | ||
| event.get_messages.return_value = [] | ||
|
|
||
| controller = MagicMock(spec=SessionController) | ||
| controller.future = asyncio.Future() | ||
| controller.future.set_result(None) |
Contributor
There was a problem hiding this comment.
issue (testing): The degenerate empty-event test never asserts that controller.stop was called, so it only partially verifies the behavior.
In test_degenerate_empty_event_stops_controller, the test currently only verifies that execution reaches the return inside the simulated handler, not that controller.stop is actually called. The comment about asserting this behavior doesn’t match the code. Please refactor so the handler logic is invoked via a callable (or the real handler), and then assert controller.stop.assert_called_once() afterwards to cover both the early return and the session termination behavior.
Comment on lines
+89
to
+95
| def test_empty_sender_id_uses_unknown_placeholder(self): | ||
| """When sender_id is empty, use '<unknown>' placeholder to avoid | ||
| cross-user key collision (S-F1 fix).""" | ||
| filter_ = DefaultSessionFilter() | ||
| event = _make_event(sender_id="") | ||
| key = filter_.filter(event) | ||
| assert key == "platform:group:123:<unknown>" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider asserting that a warning is logged when sender_id is empty to fully cover the new behavior.
This test validates the <unknown> placeholder, but the new behavior also logs a warning when sender_id is empty. To guard against future refactors dropping that warning, please capture logs (e.g., via caplog) and assert that the warning is emitted with the expected message when sender_id == "".
Suggested implementation:
import asyncio
import copy
import logging
from unittest.mock import MagicMock def test_empty_sender_id_uses_unknown_placeholder(self, caplog):
"""When sender_id is empty, use '<unknown>' placeholder to avoid
cross-user key collision (S-F1 fix) and log a warning."""
filter_ = DefaultSessionFilter()
event = _make_event(sender_id="")
# Capture warnings emitted during filtering
with caplog.at_level(logging.WARNING):
key = filter_.filter(event)
assert key == "platform:group:123:<unknown>"
# Assert that a warning was logged about the empty sender_id
warnings = [
record
for record in caplog.records
if record.levelno == logging.WARNING
]
assert warnings, "Expected a warning when sender_id is empty"
# Be specific about the expected warning message to guard against regressions
assert any(
"sender_id" in record.getMessage() and "<unknown>" in record.getMessage()
for record in warnings
), "Expected warning message to mention empty sender_id and '<unknown>' placeholder"- The assertions on the warning message should be aligned with the actual text used in
DefaultSessionFilter's logging call. Update the"sender_id"/"<unknown>"substring checks to match the precise warning message (e.g., the full string fromlog.warning(...)) once you confirm it. - If
DefaultSessionFilterlogs via a module-specific logger (e.g.,logger = logging.getLogger(__name__)) and tests use a different logging configuration, you may optionally scopecaplogwithcaplog.at_level(logging.WARNING, logger="astrbot.core.utils.session_waiter")(or the appropriate logger name) to ensure the warning is captured reliably.
Comment on lines
+109
to
+118
| def test_cleanup_does_not_evict_newer_waiter(self): | ||
| """When a newer waiter is registered under the same key, stale cleanup | ||
| must not remove it.""" | ||
| from astrbot.core.utils.session_waiter import SessionFilter | ||
|
|
||
| class TestFilter(SessionFilter): | ||
| def filter(self, event): | ||
| return "test:session" | ||
|
|
||
| filt = TestFilter() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend the cleanup race tests to also assert correct FILTERS behavior and logging when skipping stale cleanup.
Since _cleanup now also walks FILTERS and logs a warning when it skips removing a newer waiter, please add assertions that (1) the relevant filter remains in FILTERS and (2) the warning is emitted. This will round out coverage of the race-condition fix and guard against regressions in filter pruning or logging behavior.
Suggested implementation:
def test_cleanup_does_not_evict_newer_waiter(self, caplog):
"""When a newer waiter is registered under the same key, stale cleanup
must not remove it, and associated filters/logging behave correctly."""
import logging
from astrbot.core.utils.session_waiter import SessionFilter, SessionWaiter, _cleanup, FILTERS, WAITERS
class TestFilter(SessionFilter):
def filter(self, event):
return "test:session"
# Create a filter and compute its key for the event
filt = TestFilter()
event = _make_event()
key = filt.filter(event)
# Simulate a stale waiter and then a newer waiter replacing it
stale_waiter = SessionWaiter(filt)
newer_waiter = SessionWaiter(filt)
# The newer waiter is the one actually registered under the key
WAITERS[key] = newer_waiter
FILTERS[key] = filt
# Run cleanup with the stale waiter while capturing warnings
with caplog.at_level(logging.WARNING):
_cleanup(stale_waiter)
# 1) The filter for this key must remain registered in FILTERS
assert key in FILTERS
assert FILTERS[key] is filt
# 2) A warning must be logged indicating stale cleanup was skipped
assert any(
"stale cleanup" in record.message or "skip" in record.message
for record in caplog.records
)The exact API of SessionWaiter, _cleanup, FILTERS, and WAITERS, as well as the logger name and warning message, may differ slightly in your codebase. You may need to:
- Adjust the import list to match the actual exported names from
astrbot.core.utils.session_waiter(e.g., if_cleanupis a method on a class or ifFILTERS/WAITERSlive in a different module). - Update the
_cleanupcall signature (e.g.,_cleanup(key, stale_waiter)or_cleanup()depending on the implementation). - Refine the logging assertion to match the exact warning text or logger used (for example, scoping
caplog.at_level(logging.WARNING, logger="astrbot.core.utils.session_waiter")and asserting a specific message substring). - If the test suite already uses a helper to access or reset
FILTERS/WAITERS, prefer that helper instead of direct assignment in the test.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestUniqueSessionCompatibility: |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for the register_wait overwrite warning when reusing a composite session key.
The current tests cover composite key behavior and cleanup races but not the warning path when a waiter is overwritten for the same session key. Please add a test that registers a SessionWaiter, then registers another with the same session_id, and asserts that the warning is logged and that the second waiter replaces the first in USER_SESSIONS. This will exercise the overwrite-detection logic.
Suggested implementation:
# ---------------------------------------------------------------------------
# AC5: unique_session compatibility
# ---------------------------------------------------------------------------
class TestRegisterWaitOverwriteWarning:
"""Tests that re-registering a waiter for the same composite session key
logs a warning and replaces the existing waiter."""
def setup_method(self):
_clear_global_state()
def test_register_wait_logs_overwrite_warning_for_composite_key(self, caplog):
# Arrange: build a composite session id from sender + conversation
filter_ = DefaultSessionFilter()
event = _make_event(
sender_id="user-123",
conversation_id="conv-xyz",
umo={"unique_session": True},
)
# First waiter gets registered for this composite key
waiter1 = SessionWaiter(
session_id=filter_.session_id(event),
sender_id=event.sender_id,
)
register_wait(filter_, event, waiter1)
# Act: register a second waiter for the same composite session key
waiter2 = SessionWaiter(
session_id=filter_.session_id(event),
sender_id=event.sender_id,
)
with caplog.at_level(logging.WARNING):
register_wait(filter_, event, waiter2)
# Assert: warning was logged and the second waiter replaced the first
assert "overwriting existing SessionWaiter" in caplog.text
composite_key = filter_.session_id(event)
assert USER_SESSIONS[composite_key] is waiter2
class TestUniqueSessionCompatibility:To fully integrate this test, you will likely need to:
- Ensure the following imports exist at the top of the file (or add them if missing):
import loggingfrom bot.session_waiter import SessionWaiter, register_wait, USER_SESSIONS(adjust module path to match your project).
- Align the
_make_event(...)call with your existing test helpers:- Use the same keyword arguments as other tests in this file (e.g. if
umois represented differently, or ifsender_id/conversation_idfields have different names).
- Use the same keyword arguments as other tests in this file (e.g. if
- Match the logger used for the overwrite warning:
- If the warning is emitted on a specific logger (e.g.
logger = logging.getLogger("bot.session_waiter")), changewith caplog.at_level(logging.WARNING):towith caplog.at_level(logging.WARNING, logger="bot.session_waiter")or whatever name is used in the implementation.
- If the warning is emitted on a specific logger (e.g.
- Update the assertion message string to exactly match the warning text in
register_wait(e.g."register_wait: overwriting existing SessionWaiter"), so the test reliably exercises the overwrite-detection path. - If
SessionWaiterrequires more constructor arguments (e.g.controller,timeout,context), supply minimal valid values in bothwaiter1andwaiter2to make the test pass while focusing on the overwrite behavior.
Address Sourcery review comments on PR AstrBotDevs#9442: - Add caplog assertions to verify warnings for empty sender_id, cleanup race skip, and register_wait overwrite - Extend cleanup-race test to assert FILTERS removal behavior (stale filter removed, newer filter retained) and skip-cleanup warning - Extend cleanup-self test to assert filter is removed from FILTERS - Add degenerate handler test to assert controller.stop is called and event is NOT re-queued (prevent infinite loop) - Add TestRegisterWaitOverwriteWarning to cover overwrite-detection path - Extract _simulate_empty_mention_handler and _StubFilter helpers to reduce duplicated setup and make assertions clearer - Convert handler tests to async with proper side-effect verification
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
1 participant
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
Closes #9377
In group chats,
session_waiteruses onlyunified_msg_originas the session key, so when User A triggers a waiter (e.g. @bot), any other group member's message can satisfy the waiter and intercept the response. Additionally,empty_mention_waitersilently drops non-text messages (e.g. pure images) by returning early without stopping the controller, causing the waiter to linger and continue intercepting messages.Root Cause
DefaultSessionFilter.filter()returnsevent.unified_msg_originalone, which is identical for all users in the same group conversation._cleanup()usesUSER_SESSIONS.pop()unconditionally, so a stale timeout cleanup can evict a newer waiter registered under the same key.empty_mention_waiterreturns early whenmessage_stris empty without callingcontroller.stop(), leaving the waiter active.Changes
astrbot/core/utils/session_waiter.pyDefaultSessionFilter.filter()now returnsf"{unified_msg_origin}:{sender_id}"so two distinct senders in the same group produce different keys. Emptysender_idfalls back to"<unknown>"placeholder (with warning) to avoid cross-user collision.SessionFilternow inheritsabc.ABCto enforce the abstract method contract at instantiation time._cleanup()race fix: Uses identity check (is) instead ofdict.pop()— only removes fromUSER_SESSIONSandFILTERSif the stored instance is self, preventing stale cleanup from evicting a newer waiter.register_wait()overwrite warning: Logs a warning when overwriting an existing waiter for the same session key.astrbot/builtin_stars/astrbot/main.pyempty_mention_waiterno longer returns early on emptymessage_str; non-text messages (e.g. pure images) are now processed as responses and re-queued with the @bot prefix.message_strandget_messages()are empty,controller.stop()is called before returning so the waiter session ends cleanly instead of lingering.tests/unit/test_session_waiter.py(new)<unknown>placeholder (AC4–AC5)empty_mention_waiter: non-text triggers re-queue, degenerate empty stops controllerunique_session=TruecompatibilityTest Plan
ruff checkandruff formatpassSummary by Sourcery
Bind session waiters to both conversation and sender, and ensure cleanup and empty-mention handling do not cause cross-user interception or lingering sessions in group chats.
Bug Fixes:
Enhancements:
Tests: