From 53b017f03e5490d0543c6b2be3b220a4a822c5e3 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:05:54 +0530 Subject: [PATCH] refactor(gateway): share error-text blob between not_found classifiers Follow-up to the #55780 dead-target not_found blast-radius fix (merged in #56225). classify_send_error and is_chat_level_not_found each built their own lowercased error blob, but divergently: classify_send_error appended the exception CLASS NAME while is_chat_level_not_found did not. A caller passing exc= to both could get inconsistent answers on the same failure. - Extract _error_blob(exc, error_text) as the single source of truth both classifiers use (str(exc) when non-empty + class name; no stray leading space). - Align is_chat_level_not_found's signature to (exc, error_text), matching classify_send_error, removing the swapped-positional footgun; update the sole caller and the three tests to keyword form. - Add a regression guard asserting _error_blob keeps the class name. Surfaced by the hermes-pr-review Phase 2c structured review of #56225. --- gateway/delivery.py | 2 +- gateway/platforms/base.py | 41 ++++++++++++++++++++---------- tests/gateway/test_dead_targets.py | 22 +++++++++++++--- 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/gateway/delivery.py b/gateway/delivery.py index 978d22e3e..77b245d29 100644 --- a/gateway/delivery.py +++ b/gateway/delivery.py @@ -133,7 +133,7 @@ def _classify_dead_from_error_text(error_text: Optional[str]) -> Optional[str]: # Only a whole-chat not_found means the target is dead — a deleted forum topic # or an edited-away message must not mark the entire chat (and all of its future # deliveries) dead. See gateway.dead_targets' documented scope. - if kind == "not_found" and not is_chat_level_not_found(error_text): + if kind == "not_found" and not is_chat_level_not_found(error_text=error_text): return None return kind diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index 437ba9f16..b6834a669 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -1931,6 +1931,26 @@ _SUBCHAT_NOT_FOUND_SUBSTRINGS = ( ) +def _error_blob(exc: Optional[BaseException] = None, error_text: str = "") -> str: + """Build the lowercased text blob both send-error classifiers match against. + + Single source of truth so ``classify_send_error`` and + ``is_chat_level_not_found`` can never drift (e.g. one including the + exception class name and the other not) and silently disagree on the same + failure. Includes ``str(exc)`` (when non-empty) and the exception's class + name, plus any explicit ``error_text``. + """ + parts = [] + if error_text: + parts.append(error_text) + if exc is not None: + exc_str = str(exc) + if exc_str: + parts.append(exc_str) + parts.append(exc.__class__.__name__) + return " ".join(parts).lower() + + def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> str: """Map a send exception / error string to a :data:`SEND_ERROR_KINDS` value. @@ -1939,13 +1959,7 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s use. Conservative — anything unrecognized returns ``"unknown"`` so callers never mistake an unclassified failure for a benign one. """ - parts = [] - if error_text: - parts.append(error_text) - if exc is not None: - parts.append(str(exc)) - parts.append(exc.__class__.__name__) - blob = " ".join(parts).lower() + blob = _error_blob(exc, error_text) if not blob.strip(): return "unknown" if "message_too_long" in blob or "too long" in blob or "message is too long" in blob: @@ -1988,7 +2002,7 @@ def classify_send_error(exc: Optional[BaseException], error_text: str = "") -> s return "unknown" -def is_chat_level_not_found(error_text: str = "", exc: Optional[BaseException] = None) -> bool: +def is_chat_level_not_found(exc: Optional[BaseException] = None, error_text: str = "") -> bool: """Whether a ``not_found`` failure means the *whole chat* is gone. :func:`classify_send_error` collapses chat-level and thread/topic/message-level @@ -1997,13 +2011,12 @@ def is_chat_level_not_found(error_text: str = "", exc: Optional[BaseException] = forum topic or an edited-away message leaves the parent chat reachable. When both a chat-level and a sub-chat marker are present, the sub-chat reading wins (conservative: never kill a chat that may still be reachable). + + Argument order mirrors :func:`classify_send_error` (``exc`` first) and both + share :func:`_error_blob`, so the two classifiers cannot disagree on the same + failure. """ - parts = [] - if error_text: - parts.append(error_text) - if exc is not None: - parts.append(str(exc)) - blob = " ".join(parts).lower() + blob = _error_blob(exc, error_text) if any(s in blob for s in _SUBCHAT_NOT_FOUND_SUBSTRINGS): return False return any(s in blob for s in _CHAT_LEVEL_NOT_FOUND_SUBSTRINGS) diff --git a/tests/gateway/test_dead_targets.py b/tests/gateway/test_dead_targets.py index 3d94212a0..2860f0252 100644 --- a/tests/gateway/test_dead_targets.py +++ b/tests/gateway/test_dead_targets.py @@ -225,19 +225,19 @@ class TestNotFoundBlastRadius: def test_is_chat_level_not_found_chat_level(self): from gateway.platforms.base import is_chat_level_not_found - assert is_chat_level_not_found("Bad Request: chat not found") is True + assert is_chat_level_not_found(error_text="Bad Request: chat not found") is True @pytest.mark.parametrize("message", _SUBCHAT_NOT_FOUND_MESSAGES) def test_is_chat_level_not_found_subchat(self, message): from gateway.platforms.base import is_chat_level_not_found - assert is_chat_level_not_found(message) is False + assert is_chat_level_not_found(error_text=message) is False def test_subchat_marker_wins_when_both_present(self): from gateway.platforms.base import is_chat_level_not_found # Conservative: if a sub-chat marker is present, never kill the whole chat. - assert is_chat_level_not_found("chat not found; message thread not found") is False + assert is_chat_level_not_found(error_text="chat not found; message thread not found") is False def test_classify_dead_from_error_text_gates_not_found(self): from gateway.delivery import _classify_dead_from_error_text @@ -246,3 +246,19 @@ class TestNotFoundBlastRadius: assert _classify_dead_from_error_text("Bad Request: chat not found") == "not_found" assert _classify_dead_from_error_text("Bad Request: message thread not found") is None assert _classify_dead_from_error_text("httpx.ReadTimeout: connection timed out") is None + + def test_error_blob_is_shared_source_of_truth(self): + # Regression guard: classify_send_error and is_chat_level_not_found must + # both derive their match text from the SAME _error_blob helper (which + # includes the exception CLASS NAME), so they can never drift. Before + # this consolidation is_chat_level_not_found built its own blob from + # str(exc) only, omitting the class name classify_send_error included. + from gateway.platforms import base + + class TopicDeleted(Exception): + pass + + # Empty message: the only signal is the class name — _error_blob keeps it, + # with no stray leading space from an empty str(exc). + assert base._error_blob(TopicDeleted()) == "topicdeleted" + assert base._error_blob(TopicDeleted("boom")) == "boom topicdeleted"