mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(tui): derive gateway-owned sources from the Platform enum, not a hardcoded list
The salvaged guard used a hand-maintained frozenset of 14 platform names — several of which (line, wechat, facebook, imessage, googlechat) aren't actual Hermes Platform values, while real ones (whatsapp_cloud, feishu, wecom, dingtalk, qqbot, yuanbao, plugin platforms like irc) were missing. Resolve the source through gateway.config.Platform instead (built-ins + registered plugin platforms via _missing_), with an explicit exclusion set for self-owned/local sources. Adds tests for the guard and both reap paths.
This commit is contained in:
parent
f5ef7ee9da
commit
48788032da
2 changed files with 127 additions and 14 deletions
83
tests/tui_gateway/test_gateway_owned_session_reap.py
Normal file
83
tests/tui_gateway/test_gateway_owned_session_reap.py
Normal file
|
|
@ -0,0 +1,83 @@
|
|||
"""Tests for #60609: the TUI backend must not end gateway-owned sessions.
|
||||
|
||||
``_finalize_session`` (and thus the ws-orphan reaper / session.close paths
|
||||
that funnel into it) marks the session ended in state.db. For sessions the
|
||||
messaging gateway owns (telegram, discord, ...), that write creates the
|
||||
Groundhog Day routing loop described in #60609 — the gateway self-heal
|
||||
drops the ended-but-routed entry, recovers hours-old parent context, and
|
||||
loops. The TUI is only a viewer of those sessions.
|
||||
"""
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from tui_gateway.server import _finalize_session, _is_gateway_owned_source
|
||||
|
||||
|
||||
class TestIsGatewayOwnedSource:
|
||||
def test_builtin_gateway_platforms_are_owned(self):
|
||||
for src in ("telegram", "discord", "whatsapp", "slack", "signal",
|
||||
"matrix", "mattermost", "bluebubbles", "sms", "email"):
|
||||
assert _is_gateway_owned_source(src) is True, src
|
||||
|
||||
def test_case_and_whitespace_normalized(self):
|
||||
assert _is_gateway_owned_source(" Telegram ") is True
|
||||
|
||||
def test_tui_owned_sources_are_not(self):
|
||||
for src in ("tui", "cli", "webui", "desktop", "cron", "subagent",
|
||||
"test", "acp", ""):
|
||||
assert _is_gateway_owned_source(src) is False, src
|
||||
|
||||
def test_local_and_server_endpoints_are_not(self):
|
||||
# Platform enum members, but their sessions aren't owned by a remote
|
||||
# chat surface — reaping them keeps /resume clean.
|
||||
for src in ("local", "webhook", "api_server", "msgraph_webhook"):
|
||||
assert _is_gateway_owned_source(src) is False, src
|
||||
|
||||
def test_arbitrary_strings_are_not(self):
|
||||
assert _is_gateway_owned_source("hermesbench-task-xyz") is False
|
||||
assert _is_gateway_owned_source(None) is False
|
||||
|
||||
|
||||
def _make_session(session_id="sess_1"):
|
||||
agent = MagicMock()
|
||||
agent.session_id = session_id
|
||||
return {
|
||||
"agent": agent,
|
||||
"history": [{"role": "user", "content": "x"}],
|
||||
"history_lock": None,
|
||||
"session_key": session_id,
|
||||
}
|
||||
|
||||
|
||||
class TestFinalizeSkipsGatewaySessions:
|
||||
@patch("tui_gateway.server._get_db")
|
||||
def test_gateway_session_not_ended(self, mock_get_db):
|
||||
db = MagicMock()
|
||||
db.get_session.return_value = {"id": "sess_1", "source": "telegram"}
|
||||
mock_get_db.return_value = db
|
||||
|
||||
_finalize_session(_make_session(), end_reason="ws_orphan_reap")
|
||||
|
||||
db.end_session.assert_not_called()
|
||||
|
||||
@patch("tui_gateway.server._get_db")
|
||||
def test_tui_session_still_ended(self, mock_get_db):
|
||||
db = MagicMock()
|
||||
db.get_session.return_value = {"id": "sess_1", "source": "tui"}
|
||||
mock_get_db.return_value = db
|
||||
|
||||
_finalize_session(_make_session(), end_reason="ws_orphan_reap")
|
||||
|
||||
db.end_session.assert_called_once_with("sess_1", "ws_orphan_reap")
|
||||
|
||||
@patch("tui_gateway.server._get_db")
|
||||
def test_missing_row_still_ended(self, mock_get_db):
|
||||
"""A session with no state.db row can't be gateway-owned — keep the
|
||||
pre-existing reap behavior."""
|
||||
db = MagicMock()
|
||||
db.get_session.return_value = None
|
||||
mock_get_db.return_value = db
|
||||
|
||||
_finalize_session(_make_session(), end_reason="tui_close")
|
||||
|
||||
db.end_session.assert_called_once_with("sess_1", "tui_close")
|
||||
|
|
@ -499,6 +499,42 @@ def _transfer_active_session_slot(
|
|||
return False
|
||||
|
||||
|
||||
# Session sources the TUI/desktop backend must never end in state.db: the
|
||||
# messaging gateway owns those sessions' lifecycle — the TUI is only a viewer
|
||||
# (a resume of a Telegram/Discord/... session). Ending one creates the
|
||||
# #60609 Groundhog Day routing loop (see _finalize_session). Sources the
|
||||
# TUI backend itself creates ("tui", plus whatever a client passes as its
|
||||
# own ``source``) and the CLI's own sessions are NOT gateway-owned.
|
||||
_NON_GATEWAY_SOURCES = frozenset({
|
||||
"", "tui", "cli", "webui", "desktop", "cron", "subagent", "test",
|
||||
"local", "acp", "webhook", "api_server", "msgraph_webhook",
|
||||
})
|
||||
|
||||
|
||||
def _is_gateway_owned_source(source: str) -> bool:
|
||||
"""True when ``source`` names a messaging-gateway platform whose session
|
||||
lifecycle belongs to the gateway, not to this TUI backend.
|
||||
|
||||
Structural rather than a hardcoded platform list: any source that
|
||||
resolves to a known gateway ``Platform`` (built-in enum member OR a
|
||||
registered platform plugin, via ``Platform._missing_``) counts, so new
|
||||
platforms are covered automatically. Local/self-owned sources are
|
||||
excluded explicitly — ``local``/``webhook``/``api_server`` are Platform
|
||||
members but their sessions are not owned by a remote chat surface that
|
||||
routes by session_key, so reaping them is safe and keeps /resume clean.
|
||||
"""
|
||||
src = (source or "").strip().lower()
|
||||
if src in _NON_GATEWAY_SOURCES:
|
||||
return False
|
||||
try:
|
||||
from gateway.config import Platform
|
||||
|
||||
Platform(src) # raises ValueError for arbitrary non-platform strings
|
||||
return True
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _finalize_session(session: dict | None, end_reason: str = "tui_close") -> None:
|
||||
"""Best-effort finalize hook + memory commit for a session.
|
||||
|
||||
|
|
@ -586,22 +622,16 @@ def _finalize_session(session: dict | None, end_reason: str = "tui_close") -> No
|
|||
try:
|
||||
db = _get_db()
|
||||
if db is not None:
|
||||
# Don't end gateway-originated sessions — the gateway owns their
|
||||
# lifecycle. The TUI is a viewer, not the owner. Ending a
|
||||
# gateway session in state.db triggers a Groundhog Day routing
|
||||
# loop: the gateway's #54878 self-heal detects the stale entry,
|
||||
# recovers to the parent session, context compression splits
|
||||
# back to the reaped child, and the cycle repeats on every
|
||||
# inbound message. (#60609)
|
||||
_GATEWAY_SOURCES = frozenset({
|
||||
"bluebubbles", "telegram", "discord", "signal",
|
||||
"whatsapp", "sms", "slack", "mattermost",
|
||||
"matrix", "line", "wechat", "facebook",
|
||||
"imessage", "googlechat",
|
||||
})
|
||||
# Don't end gateway-originated sessions — the gateway owns
|
||||
# their lifecycle. The TUI is a viewer, not the owner.
|
||||
# Ending a gateway session in state.db triggers a Groundhog
|
||||
# Day routing loop: the gateway's #54878 self-heal detects
|
||||
# the stale entry, recovers to the parent session, context
|
||||
# compression splits back to the reaped child, and the cycle
|
||||
# repeats on every inbound message. (#60609)
|
||||
row = db.get_session(session_id)
|
||||
source = (row or {}).get("source", "")
|
||||
if source not in _GATEWAY_SOURCES:
|
||||
if not _is_gateway_owned_source(source):
|
||||
db.end_session(session_id, end_reason)
|
||||
except Exception:
|
||||
pass
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue