diff --git a/gateway/platforms/telegram.py b/gateway/platforms/telegram.py index 459b8255338b..362e7e690bde 100644 --- a/gateway/platforms/telegram.py +++ b/gateway/platforms/telegram.py @@ -4300,6 +4300,33 @@ def _is_group_chat(self, message: Message) -> bool: chat_type = str(getattr(chat, "type", "")).split(".")[-1].lower() return chat_type in {"group", "supergroup"} + @classmethod + def _effective_message_thread_id(cls, message: Message) -> Optional[str]: + """Return the routable thread id for a Telegram message. + + Forum supergroup messages posted in the General topic arrive with + ``message_thread_id=None``, while Telegram itself addresses that topic + as thread id ``1``. Private chats are the opposite footgun: Telegram + may put ``message_thread_id`` on ordinary DM replies, but those ids are + not valid send targets unless Telegram marks the message as a real topic + message. Gates, skill binding, and outbound routing must agree on the + same normalized value. + """ + chat = getattr(message, "chat", None) + chat_type = str(getattr(chat, "type", "")).split(".")[-1].lower() if chat else "" + raw = getattr(message, "message_thread_id", None) + is_topic_message = bool(getattr(message, "is_topic_message", False)) + is_forum_group = chat_type in ("group", "supergroup") and getattr(chat, "is_forum", False) + if raw is not None: + if is_forum_group or (chat_type in ("group", "supergroup") and is_topic_message): + return str(raw) + if chat_type == "private" and is_topic_message: + return str(raw) + return None + if is_forum_group: + return cls._GENERAL_TOPIC_THREAD_ID + return None + def _is_reply_to_bot(self, message: Message) -> bool: if not self._bot or not getattr(message, "reply_to_message", None): return False @@ -4372,45 +4399,45 @@ def _iter_sources(): yield getattr(message, "text", None) or "", getattr(message, "entities", None) or [] yield getattr(message, "caption", None) or "", getattr(message, "caption_entities", None) or [] - # Telegram parses mentions server-side and emits MessageEntity objects - # (type=mention for @username, type=text_mention for @FirstName targeting - # a user without a public username). Those entities are authoritative: - # raw substring matches like "foo@hermes_bot.example" are not mentions - # (bug #12545). Entities also correctly handle @handles inside URLs, code - # blocks, and quoted text, where a regex scan would over-match. + # Telegram parses mentions server-side and emits MessageEntity objects; + # trust those rather than substring-scanning the raw text. A naive + # ``"@hermes_bot" in text`` would over-match: ``foo@hermes_bot.example`` + # in a URL or code block is not a mention (bug #12545), while entities + # correctly delimit only the addressable spans. + # + # Three entity shapes count as addressing this bot: + # - ``mention``: inline ``@botname`` + # - ``text_mention``: tap-mention of a user that has no @username + # - ``bot_command``: ``/cmd@botname`` — Telegram's group command + # menu emits the whole token as a single + # bot_command entity (no separate mention). + # Accept only when the ``@suffix`` matches this + # bot; reject ``/cmd`` (no suffix) and + # ``/cmd@other_bot`` so multi-bot groups stay + # disambiguated under require_mention (#15415). for source_text, entities in _iter_sources(): for entity in entities: entity_type = str(getattr(entity, "type", "")).split(".")[-1].lower() - if entity_type == "mention" and expected: - offset = int(getattr(entity, "offset", -1)) - length = int(getattr(entity, "length", 0)) - if offset < 0 or length <= 0: - continue - if source_text[offset:offset + length].strip().lower() == expected: - return True - elif entity_type == "text_mention": + if entity_type == "text_mention": user = getattr(entity, "user", None) if user and getattr(user, "id", None) == bot_id: return True - elif entity_type == "bot_command" and expected: - # Telegram's official group-disambiguation form for slash - # commands (``/cmd@botname``) is emitted as a single - # ``bot_command`` entity covering the whole span — there - # is no accompanying ``mention`` entity. Treat it as a - # direct address to this bot when the ``@botname`` suffix - # matches. This is the form Telegram's own command menu - # autocomplete produces in groups, so dropping it at the - # mention gate would break /new, /reset, /help, ... for - # every group that has ``require_mention`` enabled (#15415). - offset = int(getattr(entity, "offset", -1)) - length = int(getattr(entity, "length", 0)) - if offset < 0 or length <= 0: - continue - command_text = source_text[offset:offset + length] - at_index = command_text.find("@") + continue + if not expected: + continue + offset = int(getattr(entity, "offset", -1)) + length = int(getattr(entity, "length", 0)) + if offset < 0 or length <= 0: + continue + span = source_text[offset:offset + length] + if entity_type == "mention": + if span.strip().lower() == expected: + return True + elif entity_type == "bot_command": + at_index = span.find("@") if at_index < 0: continue - if command_text[at_index:].strip().lower() == expected: + if span[at_index:].strip().lower() == expected: return True if bot_username and re.fullmatch(r"[a-z0-9_]{2,29}bot", bot_username, re.IGNORECASE): return bot_username in self._extract_bot_mention_usernames(message) @@ -4491,7 +4518,7 @@ def _should_process_message(self, message: Message, *, is_command: bool = False) if not self._is_group_chat(message): return True - thread_id = getattr(message, "message_thread_id", None) + thread_id = self._effective_message_thread_id(message) allowed_topics = self._telegram_allowed_topics() if allowed_topics: topic_id = str(thread_id) if thread_id is not None else self._GENERAL_TOPIC_THREAD_ID @@ -5262,29 +5289,8 @@ def _build_message_event( elif telegram_chat_type == "channel": chat_type = "channel" - # Resolve Telegram topic name and skill binding. - # Only preserve message_thread_id when Telegram marks the message as - # a real topic/forum message. Telegram can also populate - # message_thread_id for ordinary reply UI anchors; treating those as - # durable session threads fragments workflows such as CAPTCHA/login - # handoffs where the user later replies "done" in the same group. - # Private chats have the same pitfall: only real DM topic messages - # (is_topic_message=True) should keep the thread id, otherwise sends - # can hit Telegram's 'Message thread not found' error (#3206). - thread_id_raw = message.message_thread_id - is_topic_message = bool(getattr(message, "is_topic_message", False)) - is_forum_group = getattr(chat, "is_forum", False) is True - thread_id_str = None - if thread_id_raw is not None: - if chat_type == "group" and (is_topic_message or is_forum_group): - thread_id_str = str(thread_id_raw) - elif chat_type == "dm" and is_topic_message: - thread_id_str = str(thread_id_raw) - # For forum groups without an explicit topic, default to the - # General-topic id so the gateway routes back to the General topic - # rather than dropping into the bot's main channel (#22423). - if chat_type == "group" and thread_id_str is None and is_forum_group: - thread_id_str = self._GENERAL_TOPIC_THREAD_ID + # Resolve routable thread id for DM topics and forum group topics. + thread_id_str = self._effective_message_thread_id(message) chat_topic = None topic_skill = None diff --git a/tests/gateway/conftest.py b/tests/gateway/conftest.py index 965933de41b2..9e8bda97b6d9 100644 --- a/tests/gateway/conftest.py +++ b/tests/gateway/conftest.py @@ -55,10 +55,14 @@ def _ensure_telegram_mock() -> None: mod.constants.ParseMode.MARKDOWN = "Markdown" mod.constants.ParseMode.MARKDOWN_V2 = "MarkdownV2" mod.constants.ParseMode.HTML = "HTML" - mod.constants.ChatType.PRIVATE = "private" - mod.constants.ChatType.GROUP = "group" - mod.constants.ChatType.SUPERGROUP = "supergroup" - mod.constants.ChatType.CHANNEL = "channel" + for chat_type_name, chat_type_value in ( + ("PRIVATE", "private"), + ("GROUP", "group"), + ("SUPERGROUP", "supergroup"), + ("CHANNEL", "channel"), + ): + setattr(mod.constants.ChatType, chat_type_name, chat_type_value) + setattr(mod.ChatType, chat_type_name, chat_type_value) # Real exception classes so ``except (NetworkError, ...)`` clauses # in production code don't blow up with TypeError. diff --git a/tests/gateway/test_dm_topics.py b/tests/gateway/test_dm_topics.py index cf89fcaacab4..d70a658bc183 100644 --- a/tests/gateway/test_dm_topics.py +++ b/tests/gateway/test_dm_topics.py @@ -28,10 +28,14 @@ def _ensure_telegram_mock(): telegram_mod = MagicMock() telegram_mod.ext.ContextTypes.DEFAULT_TYPE = type(None) telegram_mod.constants.ParseMode.MARKDOWN_V2 = "MarkdownV2" - telegram_mod.constants.ChatType.GROUP = "group" - telegram_mod.constants.ChatType.SUPERGROUP = "supergroup" - telegram_mod.constants.ChatType.CHANNEL = "channel" - telegram_mod.constants.ChatType.PRIVATE = "private" + for chat_type_name, chat_type_value in ( + ("GROUP", "group"), + ("SUPERGROUP", "supergroup"), + ("CHANNEL", "channel"), + ("PRIVATE", "private"), + ): + setattr(telegram_mod.constants.ChatType, chat_type_name, chat_type_value) + setattr(telegram_mod.ChatType, chat_type_name, chat_type_value) for name in ("telegram", "telegram.ext", "telegram.constants", "telegram.request"): sys.modules.setdefault(name, telegram_mod) @@ -225,7 +229,7 @@ def test_persist_dm_topic_thread_id_writes_config(tmp_path): config_file = tmp_path / ".hermes" / "config.yaml" config_file.parent.mkdir(parents=True) - with open(config_file, "w") as f: + with open(config_file, "w", encoding="utf-8") as f: yaml.dump(config_data, f) adapter = _make_adapter() @@ -234,7 +238,7 @@ def test_persist_dm_topic_thread_id_writes_config(tmp_path): patch.dict(os.environ, {"HERMES_HOME": str(tmp_path / ".hermes")}): adapter._persist_dm_topic_thread_id(111, "General", 999) - with open(config_file) as f: + with open(config_file, encoding="utf-8") as f: result = yaml.safe_load(f) topics = result["platforms"]["telegram"]["extra"]["dm_topics"][0]["topics"] @@ -265,7 +269,7 @@ def test_persist_dm_topic_thread_id_skips_if_already_set(tmp_path): config_file = tmp_path / ".hermes" / "config.yaml" config_file.parent.mkdir(parents=True) - with open(config_file, "w") as f: + with open(config_file, "w", encoding="utf-8") as f: yaml.dump(config_data, f) adapter = _make_adapter() @@ -273,7 +277,7 @@ def test_persist_dm_topic_thread_id_skips_if_already_set(tmp_path): with patch.object(Path, "home", return_value=tmp_path): adapter._persist_dm_topic_thread_id(111, "General", 999) - with open(config_file) as f: + with open(config_file, encoding="utf-8") as f: result = yaml.safe_load(f) topics = result["platforms"]["telegram"]["extra"]["dm_topics"][0]["topics"] @@ -409,7 +413,7 @@ def test_get_dm_topic_info_hot_reloads_from_config(tmp_path): } config_file = tmp_path / ".hermes" / "config.yaml" config_file.parent.mkdir(parents=True) - with open(config_file, "w") as f: + with open(config_file, "w", encoding="utf-8") as f: yaml.dump(config_data, f) with patch.object(Path, "home", return_value=tmp_path), \ @@ -449,15 +453,14 @@ def test_cache_dm_topic_from_message_no_overwrite(): def _make_mock_message(chat_id=111, chat_type="private", text="hello", thread_id=None, user_id=42, user_name="Test User", forum_topic_created=None, - is_topic_message=None, is_forum=None): + is_topic_message=None, is_forum=False): """Create a mock Telegram Message for _build_message_event tests.""" chat = SimpleNamespace( id=chat_id, type=chat_type, title=None, + is_forum=is_forum, ) - if is_forum is not None: - chat.is_forum = is_forum # Add full_name attribute for DM chats if not hasattr(chat, "full_name"): chat.full_name = user_name @@ -574,10 +577,9 @@ def test_build_message_event_preserves_true_dm_topic_thread_id(): # ── _build_message_event: group_topics skill binding ── -# The telegram mock sets sys.modules["telegram.constants"] = telegram_mod (root mock), -# so `from telegram.constants import ChatType` in telegram.py resolves to -# telegram_mod.ChatType — not telegram_mod.constants.ChatType. We must use -# the same ChatType object the production code sees so equality checks work. +# The telegram mock maps both ``telegram.constants.ChatType`` and root +# ``telegram.ChatType`` to the same string values so imports in telegram.py and +# these tests exercise the same comparisons as python-telegram-bot constants. from telegram.constants import ChatType as _ChatType # noqa: E402 @@ -664,6 +666,39 @@ def test_group_topic_no_skill_binding(): assert event.source.chat_topic == "General" +def test_group_topic_general_topic_normalization_sets_skill_binding(): + """Forum General-topic messages should bind using normalized thread id 1. + + Telegram forum supergroup messages in the General topic arrive with + ``message_thread_id=None``. ``_build_message_event`` must use the same + effective thread id as the gating path so configured ``group_topics`` + bindings for thread id 1 still set source metadata and auto_skill. + """ + from gateway.platforms.base import MessageType + + adapter = _make_adapter(group_topics_config=[ + { + "chat_id": -1001234567890, + "topics": [ + {"name": "General", "thread_id": 1, "skill": "daily-review"}, + ], + } + ]) + + msg = _make_mock_message( + chat_id=-1001234567890, + chat_type=_ChatType.SUPERGROUP, + thread_id=None, + text="general update", + is_forum=True, + ) + event = adapter._build_message_event(msg, MessageType.TEXT) + + assert event.source.thread_id == "1" + assert event.source.chat_topic == "General" + assert event.auto_skill == "daily-review" + + def test_group_topic_unmapped_thread_id(): """Thread ID not in config should fall through — no skill, no topic name.""" from gateway.platforms.base import MessageType diff --git a/tests/gateway/test_telegram_group_gating.py b/tests/gateway/test_telegram_group_gating.py index 0b0e177ea5ed..1b7af7ad907c 100644 --- a/tests/gateway/test_telegram_group_gating.py +++ b/tests/gateway/test_telegram_group_gating.py @@ -2,9 +2,22 @@ from types import SimpleNamespace from unittest.mock import AsyncMock +import pytest + from gateway.config import Platform, PlatformConfig, load_gateway_config +@pytest.fixture(autouse=True) +def _scrub_telegram_group_env(monkeypatch): + for var in ( + "TELEGRAM_REQUIRE_MENTION", + "TELEGRAM_MENTION_PATTERNS", + "TELEGRAM_FREE_RESPONSE_CHATS", + "TELEGRAM_IGNORED_THREADS", + ): + monkeypatch.delenv(var, raising=False) + + def _make_adapter( require_mention=None, free_response_chats=None, @@ -79,17 +92,23 @@ def _group_message( entities=None, caption=None, caption_entities=None, + is_forum=False, + chat_type="group", + is_topic_message=None, ): reply_to_message = None if reply_to_bot: reply_to_message = SimpleNamespace(from_user=SimpleNamespace(id=999)) + if is_topic_message is None: + is_topic_message = thread_id is not None return SimpleNamespace( text=text, caption=caption, entities=entities or [], caption_entities=caption_entities or [], message_thread_id=thread_id, - chat=SimpleNamespace(id=chat_id, type="group"), + is_topic_message=is_topic_message, + chat=SimpleNamespace(id=chat_id, type=chat_type, is_forum=is_forum), from_user=SimpleNamespace(id=from_user_id), reply_to_message=reply_to_message, ) @@ -118,11 +137,11 @@ def _mention_entities(text, mentions): def _bot_command_entity(text, command): - """Entity Telegram emits for a ``/cmd`` or ``/cmd@botname`` token. + """Build a Telegram ``bot_command`` entity covering ``command``. - Telegram parses slash commands server-side. For ``/cmd@botname`` the - client does NOT emit a separate ``mention`` entity — the whole span - is a single ``bot_command`` entity. + Telegram represents ``/cmd@botname`` as a single ``BOT_COMMAND`` entity — + no separate ``mention`` entity is emitted — so tests for ``/cmd@botname`` + handling must use this shape rather than a fake mention entity. """ offset = text.index(command) return SimpleNamespace(type="bot_command", offset=offset, length=len(command)) @@ -144,7 +163,8 @@ def test_group_messages_can_require_direct_trigger_via_config(): assert adapter._should_process_message(_group_message("/status"), is_command=True) is False # Telegram's group command menu sends ``/cmd@botname`` as a single # ``bot_command`` entity spanning the whole token (no separate mention - # entity). We must accept it so the menu works when require_mention is on. + # entity). We must inspect the bot_command suffix so the menu works when + # require_mention is on. assert adapter._should_process_message( _group_message( "/status@hermes_bot", @@ -322,8 +342,75 @@ def test_allowed_topics_do_not_filter_dms(): def test_allowed_topics_treat_missing_thread_as_general_topic(): adapter = _make_adapter(require_mention=False, allowed_topics=["1"]) - assert adapter._should_process_message(_group_message("hello", thread_id=None)) is True - assert adapter._should_process_message(_group_message("hello", thread_id=8)) is False + assert adapter._should_process_message(_group_message("hello", thread_id=None, is_forum=True)) is True + assert adapter._should_process_message(_group_message("hello", thread_id=8, is_forum=True)) is False + + +def test_ignored_threads_drop_general_topic_in_forum_groups(): + adapter = _make_adapter(require_mention=False, ignored_threads=[1]) + + assert adapter._should_process_message(_group_message("hello", thread_id=None, is_forum=True)) is False + assert adapter._should_process_message(_group_message("hello", thread_id=None, is_forum=False)) is True + + +def test_ignored_threads_drop_general_topic_in_forum_supergroups(): + """Real Telegram forum groups are ``supergroup`` chats, not ``group``. + + The General topic normalization in ``_effective_message_thread_id`` keys + off ``is_forum`` for both ``group`` and ``supergroup``; this test pins + the supergroup path so a future refactor can't silently regress the + common production shape. + """ + adapter = _make_adapter(require_mention=False, ignored_threads=[1]) + + assert adapter._should_process_message( + _group_message("hello", thread_id=None, is_forum=True, chat_type="supergroup") + ) is False + # Non-forum supergroup with no thread_id must still pass — there is no + # General topic to normalize, so the ignored_threads gate has nothing + # to match. + assert adapter._should_process_message( + _group_message("hello", thread_id=None, is_forum=False, chat_type="supergroup") + ) is True + + +def test_ignored_threads_beats_free_response_chats_for_general_topic(): + """``ignored_threads`` must take precedence over ``free_response_chats`` + even for the normalized General-topic id (``1``). + + The General topic arrives with ``message_thread_id=None``; normalization + rewrites it to ``"1"`` so the ignored-thread check fires before the + free-response chat allowlist short-circuits the gate. Regressions here + would silently re-open a topic the operator had explicitly muted just + because the surrounding chat is otherwise free-response. + """ + adapter = _make_adapter( + require_mention=False, + free_response_chats=["-200"], + ignored_threads=[1], + ) + + # Forum supergroup, free-response chat, General topic (thread_id=None + # → normalized to 1) → must drop because ignored_threads wins. + assert adapter._should_process_message( + _group_message( + "hello", + chat_id=-200, + thread_id=None, + is_forum=True, + chat_type="supergroup", + ) + ) is False + # Same chat, a non-ignored topic → must still pass via free_response_chats. + assert adapter._should_process_message( + _group_message( + "hello", + chat_id=-200, + thread_id=99, + is_forum=True, + chat_type="supergroup", + ) + ) is True def test_regex_mention_patterns_allow_custom_wake_words():