mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(slack): preserve progress edits on network failures
This commit is contained in:
parent
f716b876a8
commit
fa8a3e328e
5 changed files with 387 additions and 28 deletions
|
|
@ -20226,8 +20226,10 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
async def _roll_progress_overflow_if_needed() -> bool:
|
||||
"""Start fresh editable progress bubbles before a bubble exceeds limit.
|
||||
|
||||
Returns True when it delivered/split the current buffer and the
|
||||
caller should skip the normal send/edit path for this tick.
|
||||
Returns True when it delivered/split the current buffer, or when
|
||||
a transient edit failure left the buffer and message identity
|
||||
intact for a later retry. In either case the caller should skip
|
||||
the normal send/edit path for this tick.
|
||||
"""
|
||||
nonlocal progress_msg_id, progress_lines, can_edit
|
||||
if not progress_lines or not can_edit:
|
||||
|
|
@ -20240,6 +20242,12 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
if progress_msg_id is not None:
|
||||
result = await _edit_progress_message(progress_msg_id, first_text)
|
||||
if not result.success:
|
||||
if getattr(result, "retryable", False):
|
||||
logger.debug(
|
||||
"[%s] Transient overflow edit failure — keeping can_edit=True",
|
||||
adapter.name,
|
||||
)
|
||||
return True
|
||||
can_edit = False
|
||||
# Fall back to the existing non-edit behavior below.
|
||||
return False
|
||||
|
|
|
|||
|
|
@ -2449,6 +2449,43 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
except Exception as e: # pragma: no cover - defensive logging
|
||||
if finalize:
|
||||
await self.stop_typing(chat_id, metadata=metadata)
|
||||
aiohttp_module = globals().get("aiohttp")
|
||||
connection_error_type = getattr(
|
||||
aiohttp_module, "ClientConnectionError", None
|
||||
)
|
||||
permanent_tls_error_types = tuple(
|
||||
error_type
|
||||
for error_type in (
|
||||
getattr(aiohttp_module, "ClientSSLError", None),
|
||||
getattr(aiohttp_module, "ServerFingerprintMismatch", None),
|
||||
)
|
||||
if isinstance(error_type, type)
|
||||
)
|
||||
is_permanent_tls_error = bool(permanent_tls_error_types) and isinstance(
|
||||
e, permanent_tls_error_types
|
||||
)
|
||||
is_transient_transport_error = isinstance(e, TimeoutError) or (
|
||||
isinstance(connection_error_type, type)
|
||||
and isinstance(e, connection_error_type)
|
||||
and not is_permanent_tls_error
|
||||
)
|
||||
if is_transient_transport_error:
|
||||
# chat.update is idempotent: keep this message ID after a
|
||||
# transport failure so a later edit can catch up. Treating the
|
||||
# failure as permanent makes every later tool update a new post.
|
||||
logger.error(
|
||||
"[Slack] transient chat.update failure on message %s in channel %s: %s",
|
||||
message_id,
|
||||
chat_id,
|
||||
e,
|
||||
exc_info=True,
|
||||
)
|
||||
return SendResult(
|
||||
success=False,
|
||||
error=str(e),
|
||||
retryable=True,
|
||||
error_kind="transient",
|
||||
)
|
||||
logger.error(
|
||||
"[Slack] Failed to edit message %s in channel %s: %s",
|
||||
message_id,
|
||||
|
|
|
|||
|
|
@ -115,6 +115,59 @@ class MetadataEditProgressCaptureAdapter(ProgressCaptureAdapter):
|
|||
return SendResult(success=True, message_id=message_id)
|
||||
|
||||
|
||||
class RetryableFirstEditProgressCaptureAdapter(ProgressCaptureAdapter):
|
||||
"""Fail one progress edit transiently, then accept later edits."""
|
||||
|
||||
def __init__(self, platform=Platform.TELEGRAM):
|
||||
super().__init__(platform=platform)
|
||||
self.edit_outcomes = []
|
||||
|
||||
async def edit_message(self, chat_id, message_id, content) -> SendResult:
|
||||
self.edits.append(
|
||||
{
|
||||
"chat_id": chat_id,
|
||||
"message_id": message_id,
|
||||
"content": content,
|
||||
}
|
||||
)
|
||||
if not self.edit_outcomes:
|
||||
self.edit_outcomes.append(False)
|
||||
return SendResult(
|
||||
success=False,
|
||||
error="temporary network failure",
|
||||
retryable=True,
|
||||
error_kind="transient",
|
||||
)
|
||||
self.edit_outcomes.append(True)
|
||||
return SendResult(success=True, message_id=message_id)
|
||||
|
||||
|
||||
class RetryableOverflowEditProgressAdapter(SmallLimitProgressAdapter):
|
||||
"""Fail the first split edit transiently, then keep editing."""
|
||||
|
||||
def __init__(self, platform=Platform.TELEGRAM):
|
||||
super().__init__(platform=platform)
|
||||
self.retryable_edit_failures = 0
|
||||
|
||||
async def edit_message(self, chat_id, message_id, content) -> SendResult:
|
||||
if self.retryable_edit_failures == 0:
|
||||
self.retryable_edit_failures += 1
|
||||
self.edits.append(
|
||||
{
|
||||
"chat_id": chat_id,
|
||||
"message_id": message_id,
|
||||
"content": content,
|
||||
}
|
||||
)
|
||||
return SendResult(
|
||||
success=False,
|
||||
error="temporary network failure",
|
||||
retryable=True,
|
||||
error_kind="transient",
|
||||
)
|
||||
return await super().edit_message(chat_id, message_id, content)
|
||||
|
||||
|
||||
class NonEditingProgressCaptureAdapter(ProgressCaptureAdapter):
|
||||
SUPPORTS_MESSAGE_EDITING = False
|
||||
|
||||
|
|
@ -203,6 +256,31 @@ class DelayedProgressAgent:
|
|||
}
|
||||
|
||||
|
||||
class RetryableEditProgressAgent:
|
||||
"""Keep the turn alive long enough to retry the same progress bubble."""
|
||||
|
||||
def __init__(self, **kwargs):
|
||||
self.tool_progress_callback = kwargs.get("tool_progress_callback")
|
||||
self.tools = []
|
||||
|
||||
def run_conversation(self, message, conversation_history=None, task_id=None):
|
||||
callback = self.tool_progress_callback
|
||||
assert callback is not None
|
||||
callback("tool.started", "terminal", "first command", {})
|
||||
time.sleep(0.5)
|
||||
callback("tool.started", "terminal", "second command", {})
|
||||
time.sleep(1.7)
|
||||
callback("tool.started", "terminal", "third command", {})
|
||||
time.sleep(0.5)
|
||||
callback("tool.started", "terminal", "fourth command", {})
|
||||
time.sleep(0.6)
|
||||
return {
|
||||
"final_response": "done",
|
||||
"messages": [],
|
||||
"api_calls": 1,
|
||||
}
|
||||
|
||||
|
||||
class ManyProgressLinesAgent:
|
||||
"""Emits enough tool-progress lines to exceed a single platform bubble."""
|
||||
|
||||
|
|
@ -837,6 +915,67 @@ async def _run_with_agent(
|
|||
return adapter, result
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_retryable_progress_edit_keeps_same_message_id(monkeypatch, tmp_path):
|
||||
"""A transient edit failure must not create a replacement progress bubble."""
|
||||
adapter, result = await _run_with_agent(
|
||||
monkeypatch,
|
||||
tmp_path,
|
||||
RetryableEditProgressAgent,
|
||||
session_id="sess-progress-retry-same-message",
|
||||
config_data={
|
||||
"display": {
|
||||
"tool_progress": "all",
|
||||
"interim_assistant_messages": False,
|
||||
}
|
||||
},
|
||||
platform=Platform.SLACK,
|
||||
chat_id="C123",
|
||||
chat_type="direct",
|
||||
thread_id="1700000000.000100",
|
||||
adapter_cls=RetryableFirstEditProgressCaptureAdapter,
|
||||
)
|
||||
|
||||
assert result["final_response"] == "done"
|
||||
assert isinstance(adapter, RetryableFirstEditProgressCaptureAdapter)
|
||||
assert len(adapter.sent) == 1
|
||||
assert adapter.edit_outcomes[0] is False
|
||||
assert any(adapter.edit_outcomes[1:])
|
||||
assert {call["message_id"] for call in adapter.edits} == {"progress-1"}
|
||||
assert "fourth command" in adapter.edits[-1]["content"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_retryable_overflow_edit_keeps_editable_bubble_identity(monkeypatch, tmp_path):
|
||||
"""A transient split edit must retain can_edit and the current message ID."""
|
||||
adapter, result = await _run_with_agent(
|
||||
monkeypatch,
|
||||
tmp_path,
|
||||
ManyProgressLinesAgent,
|
||||
session_id="sess-progress-retry-overflow-same-message",
|
||||
config_data={
|
||||
"display": {
|
||||
"tool_progress": "all",
|
||||
"interim_assistant_messages": False,
|
||||
}
|
||||
},
|
||||
platform=Platform.SLACK,
|
||||
chat_id="C123",
|
||||
chat_type="direct",
|
||||
thread_id="1700000000.000100",
|
||||
adapter_cls=RetryableOverflowEditProgressAdapter,
|
||||
)
|
||||
|
||||
assert result["final_response"] == "done"
|
||||
assert isinstance(adapter, RetryableOverflowEditProgressAdapter)
|
||||
assert adapter.retryable_edit_failures == 1
|
||||
assert len(adapter.sent) >= 2
|
||||
assert adapter.edits[0]["message_id"] == "progress-1"
|
||||
assert any(call["message_id"] == "progress-1" for call in adapter.edits[1:])
|
||||
assert adapter.oversized_sends == []
|
||||
assert adapter.oversized_edits == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_agent_rolls_progress_bubble_before_platform_limit(monkeypatch, tmp_path):
|
||||
"""Tool progress should start a second editable bubble before Telegram's limit.
|
||||
|
|
|
|||
|
|
@ -10,9 +10,12 @@ We mock the slack modules at import time to avoid collection errors.
|
|||
|
||||
import asyncio
|
||||
import contextlib
|
||||
import importlib
|
||||
from importlib.machinery import PathFinder
|
||||
import os
|
||||
import sys
|
||||
import time
|
||||
from types import ModuleType
|
||||
from unittest.mock import AsyncMock, MagicMock, patch, call
|
||||
|
||||
import pytest
|
||||
|
|
@ -33,35 +36,55 @@ from gateway.platforms.base import (
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _load_installed_package(name):
|
||||
"""Load a real installed package even if another test left a module mock."""
|
||||
if PathFinder.find_spec(name) is None:
|
||||
return None
|
||||
|
||||
prefix = f"{name}."
|
||||
displaced = {
|
||||
module_name: sys.modules.pop(module_name)
|
||||
for module_name in tuple(sys.modules)
|
||||
if (module_name == name or module_name.startswith(prefix))
|
||||
and not isinstance(sys.modules[module_name], ModuleType)
|
||||
}
|
||||
try:
|
||||
return importlib.import_module(name)
|
||||
except ImportError:
|
||||
sys.modules.update(displaced)
|
||||
return None
|
||||
|
||||
|
||||
def _ensure_slack_mock():
|
||||
"""Install mock slack modules so SlackAdapter can be imported."""
|
||||
if "slack_bolt" in sys.modules and hasattr(sys.modules["slack_bolt"], "__file__"):
|
||||
return # Real library installed
|
||||
"""Install mocks only for Slack dependencies that are actually unavailable."""
|
||||
if _load_installed_package("slack_bolt") is None:
|
||||
slack_bolt = MagicMock()
|
||||
slack_bolt.async_app.AsyncApp = MagicMock
|
||||
slack_bolt.adapter.socket_mode.async_handler.AsyncSocketModeHandler = MagicMock
|
||||
for name, mod in [
|
||||
("slack_bolt", slack_bolt),
|
||||
("slack_bolt.async_app", slack_bolt.async_app),
|
||||
("slack_bolt.adapter", slack_bolt.adapter),
|
||||
("slack_bolt.adapter.socket_mode", slack_bolt.adapter.socket_mode),
|
||||
(
|
||||
"slack_bolt.adapter.socket_mode.async_handler",
|
||||
slack_bolt.adapter.socket_mode.async_handler,
|
||||
),
|
||||
]:
|
||||
sys.modules.setdefault(name, mod)
|
||||
|
||||
slack_bolt = MagicMock()
|
||||
slack_bolt.async_app.AsyncApp = MagicMock
|
||||
slack_bolt.adapter.socket_mode.async_handler.AsyncSocketModeHandler = MagicMock
|
||||
if _load_installed_package("slack_sdk") is None:
|
||||
slack_sdk = MagicMock()
|
||||
slack_sdk.web.async_client.AsyncWebClient = MagicMock
|
||||
for name, mod in [
|
||||
("slack_sdk", slack_sdk),
|
||||
("slack_sdk.web", slack_sdk.web),
|
||||
("slack_sdk.web.async_client", slack_sdk.web.async_client),
|
||||
]:
|
||||
sys.modules.setdefault(name, mod)
|
||||
|
||||
slack_sdk = MagicMock()
|
||||
slack_sdk.web.async_client.AsyncWebClient = MagicMock
|
||||
|
||||
for name, mod in [
|
||||
("slack_bolt", slack_bolt),
|
||||
("slack_bolt.async_app", slack_bolt.async_app),
|
||||
("slack_bolt.adapter", slack_bolt.adapter),
|
||||
("slack_bolt.adapter.socket_mode", slack_bolt.adapter.socket_mode),
|
||||
(
|
||||
"slack_bolt.adapter.socket_mode.async_handler",
|
||||
slack_bolt.adapter.socket_mode.async_handler,
|
||||
),
|
||||
("slack_sdk", slack_sdk),
|
||||
("slack_sdk.web", slack_sdk.web),
|
||||
("slack_sdk.web.async_client", slack_sdk.web.async_client),
|
||||
]:
|
||||
sys.modules.setdefault(name, mod)
|
||||
|
||||
# aiohttp is imported alongside slack-bolt; mock it if missing
|
||||
sys.modules.setdefault("aiohttp", MagicMock())
|
||||
aiohttp_module = _load_installed_package("aiohttp") or MagicMock()
|
||||
sys.modules.setdefault("aiohttp", aiohttp_module)
|
||||
|
||||
|
||||
_ensure_slack_mock()
|
||||
|
|
@ -74,6 +97,15 @@ _slack_mod.SLACK_AVAILABLE = True
|
|||
from plugins.platforms.slack.adapter import SlackAdapter # noqa: E402
|
||||
|
||||
|
||||
def test_slack_mock_bootstrap_preserves_installed_packages():
|
||||
"""Installed Slack dependencies must remain importable as real packages."""
|
||||
for package in ("slack_sdk", "aiohttp"):
|
||||
if PathFinder.find_spec(package) is not None:
|
||||
assert isinstance(sys.modules[package], ModuleType)
|
||||
if PathFinder.find_spec("slack_sdk") is not None:
|
||||
assert isinstance(importlib.import_module("slack_sdk.errors"), ModuleType)
|
||||
|
||||
|
||||
async def _pending_for_fake_task():
|
||||
# Stay pending so done-callbacks attached by the adapter (which would
|
||||
# otherwise schedule a reconnect) don't fire during the test. The pytest
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ from unittest.mock import AsyncMock, MagicMock, call
|
|||
import pytest
|
||||
|
||||
from gateway.config import PlatformConfig
|
||||
from plugins.platforms.slack import adapter as slack_module
|
||||
from plugins.platforms.slack.adapter import SlackAdapter
|
||||
|
||||
|
||||
|
|
@ -42,6 +43,20 @@ class SlackRejectedBlocks(Exception):
|
|||
self.response = {"error": error}
|
||||
|
||||
|
||||
def _slack_connection_key():
|
||||
from aiohttp.client_reqrep import ConnectionKey
|
||||
|
||||
return ConnectionKey(
|
||||
host="slack.com",
|
||||
port=443,
|
||||
is_ssl=True,
|
||||
ssl=True,
|
||||
proxy=None,
|
||||
proxy_auth=None,
|
||||
proxy_headers_hash=None,
|
||||
)
|
||||
|
||||
|
||||
class TestSendMessageBlocks:
|
||||
@pytest.mark.asyncio
|
||||
async def test_disabled_by_default_no_blocks(self):
|
||||
|
|
@ -189,3 +204,131 @@ class TestEditMessageBlocks:
|
|||
assert "blocks" in first and first["blocks"]
|
||||
assert second["blocks"] == []
|
||||
assert second["text"]
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_timeout_error_on_edit_is_retryable_transient(self):
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(side_effect=TimeoutError("timed out"))
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is True
|
||||
assert result.error_kind == "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_dns_connection_error_on_edit_is_retryable_transient(self):
|
||||
from aiohttp import ClientConnectorDNSError
|
||||
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(
|
||||
side_effect=ClientConnectorDNSError(
|
||||
_slack_connection_key(),
|
||||
OSError(8, "nodename nor servname provided, or not known"),
|
||||
)
|
||||
)
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is True
|
||||
assert result.error_kind == "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_slack_api_error_on_edit_is_not_retryable(self):
|
||||
from slack_sdk.errors import SlackApiError
|
||||
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(
|
||||
side_effect=SlackApiError(
|
||||
"message_not_found",
|
||||
{"ok": False, "error": "message_not_found"},
|
||||
)
|
||||
)
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is not True
|
||||
assert result.error_kind != "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_certificate_error_on_edit_is_not_retryable(self):
|
||||
import ssl
|
||||
|
||||
from aiohttp import ClientConnectorCertificateError
|
||||
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(
|
||||
side_effect=ClientConnectorCertificateError(
|
||||
_slack_connection_key(),
|
||||
ssl.SSLCertVerificationError("certificate verify failed"),
|
||||
)
|
||||
)
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is not True
|
||||
assert result.error_kind != "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_tls_integrity_errors_on_edit_are_not_retryable(self):
|
||||
import ssl
|
||||
|
||||
from aiohttp import ClientConnectorSSLError, ServerFingerprintMismatch
|
||||
|
||||
errors = (
|
||||
ClientConnectorSSLError(
|
||||
_slack_connection_key(), ssl.SSLError("handshake failed")
|
||||
),
|
||||
ServerFingerprintMismatch(b"expected", b"got", "slack.com", 443),
|
||||
)
|
||||
for error in errors:
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(side_effect=error)
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is not True
|
||||
assert result.error_kind != "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_plain_os_error_on_edit_is_not_retryable(self):
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(side_effect=OSError("invalid local socket state"))
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is not True
|
||||
assert result.error_kind != "transient"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_lazy_rebound_aiohttp_connection_error_is_retryable(
|
||||
self, monkeypatch
|
||||
):
|
||||
import tools.lazy_deps as lazy_deps
|
||||
|
||||
monkeypatch.setattr(slack_module, "SLACK_AVAILABLE", False)
|
||||
monkeypatch.delattr(slack_module, "aiohttp", raising=False)
|
||||
|
||||
def ensure_and_bind(_group, import_fn, target_globals, *, prompt):
|
||||
assert prompt is False
|
||||
target_globals.update(import_fn())
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(lazy_deps, "ensure_and_bind", ensure_and_bind)
|
||||
|
||||
assert slack_module.check_slack_requirements() is True
|
||||
adapter, client = _make_adapter()
|
||||
client.chat_update = AsyncMock(
|
||||
side_effect=slack_module.aiohttp.ClientConnectionError("connection dropped")
|
||||
)
|
||||
|
||||
result = await adapter.edit_message("C1", "111.222", RICH_MD, finalize=True)
|
||||
|
||||
assert result.success is False
|
||||
assert result.retryable is True
|
||||
assert result.error_kind == "transient"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue