fix(gateway): don't mark an entire chat dead on thread/message-level not_found (salvage #55780) - #56225
Conversation
…not_found NousResearch#55115 added the dead-target registry so confirmed-dead delivery targets are short-circuited. Its documented scope (gateway/dead_targets.py) is deliberately narrow: only *whole-chat* deaths -- the `forbidden` and chat-level `not_found` (`chat not found`) kinds -- should be recorded; "Thread/topic-level not_found is NOT recorded here ... a deleted topic does not mean the parent chat is dead." But the implementation doesn't honor that scope. classify_send_error collapses chat-level "chat not found" AND thread/message-level not_found ("thread not found", "topic_deleted", "message_id_invalid", "message to edit/reply not found") into one "not_found" kind, _DEAD_ERROR_KINDS contains "not_found" wholesale, and deliver()'s except marks the PARENT chat_id dead. So a single deleted Telegram topic or edited-away message permanently marks the entire chat (and every future scheduled / cron / agent delivery to it) dead -- silently. The adapter self-heal the docstring relies on only covers the non-private-group thread retry; named-DM-topic and message-level failures propagate to deliver()'s except and wrongly kill the whole chat. Add is_chat_level_not_found() (factoring the not_found substrings into chat-level vs sub-chat-level constants) and gate the delivery dead-path: a "not_found" only marks the target dead when it is chat-level. classify_send_error's public contract is unchanged (still returns "not_found" for every shape); only the mark_dead decision is refined, restoring the registry's documented scope. Cross-platform: telegram/slack/discord delivery all flow through classify_send_error -> mark_dead. Adds regression tests through the real deliver() path plus helper/classifier units.
|
Ran the full hermes-pr-review Phase 2c (4-part structured review + hermes-agent-dev checks). One real finding folded as a follow-up commit (d7c3a5d): |
|
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.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Follow-up to the NousResearch#55780 dead-target not_found blast-radius fix (merged in NousResearch#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 NousResearch#56225.
Summary
Salvage of #55780 by @r266-tech (rebased onto current
main+ AUTHOR_MAP entry). Stops the gateway from marking an entire chat dead when a send fails with a thread/topic/message-levelnot_found— a deleted forum topic or an edited-away message must not black-hole every future delivery to the parent chat.Original PR was 194 commits behind
main; @r266-tech's commit is cherry-picked verbatim (authorship preserved). I added one AUTHOR_MAP chore commit under my identity socheck-attributionresolves their noreply email.The bug
classify_send_errorcollapses several distinct Telegram failures into a single"not_found"kind:chat not found(the chat/user/group itself is gone → truly dead)message thread not found,topic_deleted,message to edit/reply not found,message_id_invalid(the parent chat is still reachable)The dead-target path treats any
"not_found"as a dead target, so a single deleted forum topic or edited-away message marks the whole chat dead and short-circuits all future deliveries to it.The fix
is_chat_level_not_found()ingateway/platforms/base.py, splitting the existing sixnot_foundsubstrings into_CHAT_LEVEL_NOT_FOUND_SUBSTRINGSand_SUBCHAT_NOT_FOUND_SUBSTRINGS(byte-identical set to before — pure refactor ofclassify_send_error, no string added/dropped/re-bucketed).gateway/delivery.py::_classify_dead_from_error_text: only a chat-levelnot_foundmarks a target dead; sub-chatnot_founddoes not. When both markers are present, the sub-chat reading wins (conservative — never kill a chat that may still be reachable).Review (this salvage)
Ran our review workflow (classifier-correctness / false-negative hunt) + my own trace. Clean, no findings.
main; the chat-genuinely-dead errors (user is deactivated,bot was blocked,not a member) route to the separateforbiddenbucket, which is not gated and still marks dead. Casing is safe (both functions lowercase).if kind == "not_found"only —forbiddenand every other dead kind pass through unchanged.PEER_ID_INVALID/group chat was migratedclassify asunknownand are never dead-marked → infinite retries. This is a latent gap on currentmain, out of scope here; happy to open a separate follow-up if wanted.Tests
New tests cover: chat-level
not_foundmarks dead; each of the five sub-chatnot_foundvariants does not mark the parent chat dead; theis_chat_level_not_foundhelper (chat-level True, sub-chat False, both-present → False); and the_classify_dead_from_error_textgate.Supersedes #55780. Full credit to @r266-tech.