fix: isolate empty-mention waiter by sender without changing default session scope

[hedssaz]

Fixes #9377

In group chats, an empty mention (@bot with no text) starts a 60s
session_waiter to wait for follow-up content. The waiter is registered with
DefaultSessionFilter, whose key is unified_msg_origin (group-level) and does
not include the sender. As a result, the first non-empty message from any
other member during the wait window is intercepted, gets a bot At prepended,
and is re-dispatched — producing a reply to the wrong user / wrong context. Full
analysis, reproduction and captured logs are in #9377.

This PR fixes the empty-mention flow without changing the default session
scope, so third-party plugins that rely on the current group-level default are
unaffected.

Why not simply change DefaultSessionFilter

Changing the default from unified_msg_origin to include the sender would
change the default runtime behavior of session_waiter, which is exported to
plugins through astrbot.api.util. The group-level default has been in place
since v3.5.4 (~15 months, 138 version tags) and is relied upon by real
plugins that omit session_filter=, for example:

• astrbot_plugin_kimi_datasource_api registers a waiter manually with
  SessionWaiter(DefaultSessionFilter(), event.unified_msg_origin, ...). The
  registration key is the raw UMO while lookups go through
  DefaultSessionFilter.filter(event). If the default changed, the keys would no
  longer match and its login waiter would never trigger.
• astrbot_plugin_decision_roulette and astrbot_plugin_TurtleSoup use the
  default filter to collect input from multiple group members; a per-sender
  default would only accept the initiator.

So the fix is scoped to the one place that actually needs per-sender isolation.

Modifications

• astrbot/core/utils/session_waiter.py: add SenderSessionFilter, a new
  optional SessionFilter keyed by unified_msg_origin + sender_id.
  DefaultSessionFilter is unchanged, and no new names are added to the
  public astrbot.api.util surface. This is purely additive.
  The key uses a length-prefixed encoding (f"{len(umo)}:{umo}:{sender}")
  rather than plain concatenation: both the UMO and the sender id are arbitrary
  platform strings (e.g. Telegram topic-group UMOs contain #), so a plain
  separator would allow constructible key collisions that defeat the isolation.
• astrbot/builtin_stars/astrbot/main.py: the empty-mention waiter now passes
  session_filter=SenderSessionFilter(). When the sender id is unavailable it
  skips waiting entirely, to avoid silently degrading back to group scope.
• tests/test_session_waiter_cross_user.py (new): 7 session-key regression
  tests (module loaded standalone).
• tests/unit/test_empty_mention_sender_scope.py (new): 3 production-wiring
  tests driving the real Main.handle_empty_mention and
  Main.handle_session_control_agent (following the existing
  Main.__new__(Main) pattern in tests/unit/test_group_chat_context_wiring.py).

The other issues noted in #9377 (empty-message_str messages being swallowed
during the wait window; _cleanup / FILTERS behavior with same-key waiters)
are independent of the session key and will be handled in separate PRs.

Screenshots or Test Results

$ ruff format --check <changed files + test files> && ruff check <same>
All checks passed!

$ pytest tests/test_session_waiter_cross_user.py \
         tests/unit/test_empty_mention_sender_scope.py -o asyncio_mode=auto -q
..........                                                               [100%]
10 passed

Session-key coverage (tests/test_session_waiter_cross_user.py):

1. test_default_filter_returns_umo_unchanged — the default filter still returns
   unified_msg_origin (locks the default semantics in place);
2. test_default_manual_umo_registration_still_triggers — reproduces the
   kimi_datasource_api manual-registration pattern and asserts it still
   triggers (guards against regressing plugins that depend on the group default);
3. test_sender_filter_isolates_users_in_group — another member's message is no
   longer intercepted (primary goal);
4. test_sender_filter_accepts_original_sender — the original sender's follow-up
   is still captured;
5. test_sender_filter_key_is_collision_free — UMOs / sender ids containing
   separator characters (#, :) cannot produce colliding keys;
6. test_sender_filter_isolates_users_with_hash_in_umo — isolation holds for
   Telegram topic-group style UMOs that contain #;
7. test_sender_filter_does_not_leak_across_conversations — the same user's
   messages in other groups / DMs are not intercepted.

Production-wiring coverage (tests/unit/test_empty_mention_sender_scope.py,
drives the real handlers rather than a re-implementation):

8. test_other_members_message_passes_through — BOB's message is not stopped,
   not modified, and not re-enqueued while ALICE's waiter is active;
9. test_original_senders_followup_is_redispatched — ALICE's follow-up gets the
   bot At injected, is stopped and re-enqueued, and the waiter is cleaned up;
10. test_empty_sender_id_skips_waiting — with no reliable sender id the
    handler finishes immediately and registers no waiter.

Verified that the wiring tests actually guard the fix: with the main.py
change reverted to master, tests 8 and 10 fail; with the fix in place all 10
pass.

---

• This is NOT a breaking change.

The default session scope is unchanged; only the built-in empty-mention waiter
opts into per-sender isolation. SenderSessionFilter is an additive core
utility and is not exported through astrbot.api.util.

Checklist

• 😊 If new features are added, they have been discussed with the authors
  via issues/emails. (Bug fix, proposed in [Bug] 群聊中空提及等待器会截获其他成员的消息（session_waiter 未绑定发送者） #9377.)
• 👀 My changes have been well-tested, with verification steps and results
  provided above.
• 🤓 No new dependencies are introduced.
• 😮 My changes do not introduce malicious code.

Summary by Sourcery

Fix empty-mention session handling in group chats by isolating the waiter per sender while preserving existing group-level session semantics.

Bug Fixes:

• Prevent empty-mention waiters in group chats from intercepting follow-up messages from other members and replying in the wrong context.
• Ensure empty-mention handling skips session waiting when no reliable sender ID is available to avoid unintended group-scoped behavior.

Enhancements:

• Introduce an optional SenderSessionFilter that scopes session keys by conversation origin and sender using a collision-safe encoding without changing the default session filter.
• Strengthen session key behavior with explicit guarantees against key collisions and cross-conversation leakage.

Tests:

• Add regression tests that lock in DefaultSessionFilter behavior and validate SenderSessionFilter isolation, collision safety, and cross-conversation separation.
• Add production wiring tests for empty-mention handling to verify correct per-sender redispatch, non-interference with other members, and behavior when sender ID is missing.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[xiaoyuyu6420]

Review

I've reviewed this PR in the context of issue #9377 alongside the two other open PRs (#9422, #9442) that address the same bug. Here's my analysis.

Strengths

1. Length-prefixed key encoding ({len(umo)}:{umo}:{sender_id}) — this is technically the most robust approach to key collision resistance. Telegram topic-group origins contain #, and : appears in all UMO formats (platform:MessageType:id), so plain concatenation with a separator has a theoretical collision surface. This encoding is uniquely decodable.

2. Comprehensive test suite — two separate test files covering both unit-level key logic and integration-level Main.handle_empty_mention wiring. The regression guard for astrbot_plugin_kimi_datasource_api (manual UMO registration pattern) is particularly valuable.

3. Opt-in design preserves backward compatibility — DefaultSessionFilter is unchanged, so third-party plugins that rely on group-level keys are not affected.

Issues

1. Does not fix the non-text message swallowing bug (mentioned in #9377 as a "连带问题")

The issue report explicitly documents that pure image messages during the wait window are silently dropped and the waiter stays alive:

empty_mention_waiter handler:
    if not event.message_str or not event.message_str.strip():
        return  # ← pure image is silently dropped, waiter keeps running

This PR only adds SenderSessionFilter to the call site but does not modify the handler body. After this fix, Alice's pure image in the wait window is still silently consumed (by Alice herself now, not by Bob — an improvement, but the message is still lost).

2. Does not fix the _cleanup race condition (mentioned in #9377 as a "连带问题")

The issue report documents that SessionWaiter._cleanup() unconditionally calls USER_SESSIONS.pop(self.session_id). If a newer waiter is registered under the same key before the old one times out, the old waiter's cleanup evicts the new one. This PR does not address this.

3. Skipping the waiter on empty sender_id is a behavioral regression

if str(event.get_sender_id()):
    await empty_mention_waiter(event, session_filter=SenderSessionFilter())

When sender_id is empty/falsy, the waiter is simply not started. On platforms where get_sender_id() may temporarily return empty, the bot silently stops responding to empty mentions with no feedback to the user. A fallback placeholder (e.g., <unknown>) with a warning log would be safer — it degrades gracefully instead of silently dropping functionality.

4. Only fixes empty_mention_waiter call site, not is_wake_prefix_only

The issue report notes: "仅含唤醒前缀的消息（is_wake_prefix_only 分支）会启动同一个等待器，跨用户截获问题同样适用于该场景". The same session_waiter is used in both branches, but this PR only passes SenderSessionFilter to the empty_mention_waiter call. The wake-prefix-only path still uses DefaultSessionFilter and is still vulnerable to cross-user interception.

Summary

This PR correctly identifies the core isolation problem and proposes a clean opt-in API with excellent test coverage. However, it addresses only one of the three issues documented in #9377 (cross-user interception), leaving the non-text swallowing and cleanup race unfixed. The empty-sender-id skip also introduces a minor behavioral regression.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8597ae2751

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38e036c065

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f93952b64c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[hedssaz]

@xiaoyuyu6420 Thanks for the detailed review — and for cross-referencing the other PRs.

Review points 1 and 2 (non-text swallowing, _cleanup race) — correct, and intentional.

I reported all three problems in #9377 and split them deliberately so the session-key change can land without also changing semantics that plugins depend on. I'm happy to send follow-up PRs, or fold those fixes into this one if the maintainers prefer.

Review point 4 (wake-prefix path) — I believe this path is already covered.

There is exactly one @session_waiter in the file and one call site. Both branches merge at the shared guard:

if not (is_empty_mention or is_wake_prefix_only):
    return

Everything after that guard follows the same path and reaches:

await empty_mention_waiter(
    event,
    session_filter=SenderSessionFilter(),
)

There is no second call that still uses DefaultSessionFilter. Both paths are also subject to the same requirement that a usable sender ID be available. If you have a scenario where the wake-prefix path does not receive the sender-scoped filter, I'll add a regression test.

Review point 3 (empty sender_id) — agreed, and thanks for catching the missing log; added, with a test.

Two clarifications:

• With the default configuration (empty_mention_waiting_need_reply enabled), the bot still replies. That reply is emitted before the waiter is registered, so only the subsequent 60-second wait is skipped, not the response itself.
• On the <unknown> placeholder: two members who both yield an empty sender ID would collide on {umo}:<unknown>, reintroducing cross-user interception for exactly the users whose identities cannot be verified. Skipping fails closed (a feature is unavailable); the placeholder fails open (the wrong user's message may be consumed). For a bug whose symptom is replying to the wrong person, failing closed seems safer.

Why this PR leaves DefaultSessionFilter alone

To be upfront, changing the default probably would not break many plugins. For most of them, per-sender keying would be neutral or better — “reply with the song number,” “enter the verification code,” and “reply 确认 to proceed” are all flows where only the initiator should answer, whereas today any member of the group can do so.

I did find plugins whose behavior would change.

astrbot_plugin_decision_roulette collects options through the default waiter, so today any member who replies in the group can add one; with per-sender keying, only the initiator's replies would be accepted.

astrbot_plugin_kimi_datasource_api constructs its waiter as:

SessionWaiter(
    DefaultSessionFilter(),
    pending.session_id,
    False,
)

Here, pending.session_id is the raw UMO. Registration therefore uses that raw key, while lookups go through DefaultSessionFilter.filter(event). Changing the default would make the two keys permanently differ, causing its chat-control waiter — which handles cancel, restart, and status during login — to stop receiving messages.

Its separate OAuth polling task could still complete the login, so this would be a partial regression rather than a total failure.

What concerns me is not the number of affected plugins, but the fact that this failure is silent: no error is raised; the waiter simply stops firing. Candidate call sites can be found through static search, but whether a given plugin requires group scope or sender scope is encoded in its workflow logic rather than its call signature, so the complete affected set cannot be determined statically.

That feels like a change to make deliberately, with a changelog entry, rather than implicitly inside a bug fix. Either way, it is a maintainer decision. This PR keeps the blast radius small while leaving that decision open.

For what it's worth, the current documentation describes a per-sender default, while the implementation has returned unified_msg_origin since #1326 / cfae6550. I can only date the earliest public version of that documentation page to 2025-11-16, so I cannot determine which behavior came first.

#9422 also keeps DefaultSessionFilter unchanged and scopes the fix to the built-in empty-mention waiter.

Summary: Review point 4 is already covered; the warning and test requested in review point 3 have been added; and review points 1 and 2 are accurate but intentionally scoped out. Whichever approach is chosen for #9377, I would suggest retaining a regression test for the manual UMO-registration pattern, since that failure mode is silent.

[xiaoyuyu6420]

@hedssaz Thanks for the thorough response — and for the correction on point 4.

You're right about point 4. I re-read the control flow: both is_empty_mention and is_wake_prefix_only merge after the guard at line 72 and reach the single empty_mention_waiter(event) call at line 143. There is no second call site using DefaultSessionFilter. My comment was factually wrong — I apologize for the noise.

On point 3 (<unknown> vs skip): You make a strong argument. The distinction between "fails closed" (feature unavailable) and "fails open" (wrong user's message consumed) is the right framing. For a bug whose symptom is replying to the wrong person, failing closed is indeed safer. I retract that suggestion — your approach is better for this specific bug class.

The only scenario where the placeholder would be preferable is if get_sender_id() returning empty is common on some platform (making the feature frequently unavailable). But that's speculative without data, and the conservative choice is correct.

On points 1 and 2 (intentional scope split): Makes sense. Keeping the session-key change independent of the semantic changes (non-text handling, cleanup race) lets each land on its own merit. If the maintainers prefer to address all three together, #9442 covers them; if they prefer the incremental approach, your PR + follow-ups is cleaner.

On the regression test for manual UMO registration: Strongly agree. Whichever approach wins, a test guarding against the silent failure mode (registration key ≠ lookup key) should be retained. I've added a similar guard in #9442's TestDecoratorPathCompatibility, but yours in test_default_manual_umo_registration_still_triggers is more direct because it tests the actual breakage scenario.

Appreciate the constructive discussion — this is exactly the kind of review dialogue that makes the end result better.