diff --git a/gateway/run.py b/gateway/run.py index 0f74680bf079..3363631d5172 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -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 diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index ef27b760d319..b98cfec5a8e4 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -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, diff --git a/tests/gateway/test_run_progress_topics.py b/tests/gateway/test_run_progress_topics.py index 9892b38b90d3..6e7926588db7 100644 --- a/tests/gateway/test_run_progress_topics.py +++ b/tests/gateway/test_run_progress_topics.py @@ -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. diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index 17eb94f94e50..30071775d8fd 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -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 diff --git a/tests/gateway/test_slack_block_kit_adapter.py b/tests/gateway/test_slack_block_kit_adapter.py index afed8411a1ba..ab35f4130481 100644 --- a/tests/gateway/test_slack_block_kit_adapter.py +++ b/tests/gateway/test_slack_block_kit_adapter.py @@ -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"