fix(mattermost): remove unsafe substring auth-error fallback in WS reconnect loop

plugins/platforms/mattermost/adapter.py

@@ -751,13 +751,9 @@ async def _ws_loop(self) -> None:
                 # Detect permanent auth/permission failures that will never
                 # succeed on retry — stop reconnecting instead of looping forever.
                 import aiohttp
-                err_str = str(exc).lower()
                 if isinstance(exc, aiohttp.WSServerHandshakeError) and exc.status in {401, 403}:
                     logger.error("Mattermost WS auth failed (HTTP %d) — stopping reconnect", exc.status)
                     return
-                if "401" in err_str or "403" in err_str or "unauthorized" in err_str:
-                    logger.error("Mattermost WS permanent error: %s — stopping reconnect", exc)
-                    return
                 logger.warning("Mattermost WS error: %s — reconnecting in %.0fs", exc, delay)
 
             if self._closing:

tests/gateway/test_ws_auth_retry.py

@@ -14,6 +14,7 @@
 # Mattermost: _ws_loop auth-aware retry
 # ---------------------------------------------------------------------------
 
+
 class TestMattermostWSAuthRetry:
     """gateway/platforms/mattermost.py — _ws_loop()"""
 
@@ -30,6 +31,7 @@ def test_401_handshake_stops_reconnect(self):
         )
 
         from plugins.platforms.mattermost.adapter import MattermostAdapter
+
         adapter = MattermostAdapter.__new__(MattermostAdapter)
         adapter._closing = False
 
@@ -47,17 +49,72 @@ async def fake_connect():
         # Should have attempted once and stopped, not retried
         assert call_count == 1
 
+    def test_transient_401_substring_does_not_stop_reconnect(self):
+        """A transient exception whose stringified message merely contains
+        "401" (e.g. a proxy error body with digits) must not be mistaken
+        for a genuine auth rejection. The loop should log a warning and
+        retry, not return."""
+        from plugins.platforms.mattermost.adapter import MattermostAdapter
+
+        adapter = MattermostAdapter.__new__(MattermostAdapter)
+        adapter._closing = False
+
+        call_count = 0
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            if call_count >= 2:
+                # Stop the loop once we've proven a retry happened.
+                adapter._closing = True
+            raise RuntimeError(
+                "proxy returned HTTP/1.1 401 in body but connection reset"
+            )
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        with patch("asyncio.sleep", new=AsyncMock()):
+            asyncio.run(adapter._ws_loop())
+
+        # The substring fallback is gone, so this must retry past the
+        # first attempt instead of returning immediately.
+        assert call_count == 2
+
+    def test_closing_flag_prevents_further_connect_attempts(self):
+        """Existing self._closing early-return behavior is unaffected by
+        the substring-fallback removal: once _closing is set, the loop
+        must not attempt to connect at all."""
+        from plugins.platforms.mattermost.adapter import MattermostAdapter
+
+        adapter = MattermostAdapter.__new__(MattermostAdapter)
+        adapter._closing = True
+
+        call_count = 0
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            raise RuntimeError("should never be called")
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        asyncio.run(adapter._ws_loop())
+
+        assert call_count == 0
+
 
 # ---------------------------------------------------------------------------
 # Matrix: _sync_loop auth-aware retry
 # ---------------------------------------------------------------------------
 
+
 class TestMatrixSyncAuthRetry:
     """gateway/platforms/matrix.py — _sync_loop()"""
 
     def test_unknown_token_sync_error_stops_loop(self):
         """A SyncError with M_UNKNOWN_TOKEN should stop syncing."""
         import types
+
         nio_mock = types.ModuleType("nio")
 
         class SyncError:
@@ -67,6 +124,7 @@ def __init__(self, message):
         nio_mock.SyncError = SyncError
 
         from plugins.platforms.matrix.adapter import MatrixAdapter
+
         adapter = MatrixAdapter.__new__(MatrixAdapter)
         adapter._closing = False
 
@@ -86,6 +144,7 @@ async def fake_sync(timeout=30000, since=None):
 
         async def run():
             import sys
+
             sys.modules["nio"] = nio_mock
             try:
                 await adapter._sync_loop()
@@ -94,5 +153,3 @@ async def run():
 
         asyncio.run(run())
         assert sync_count == 1
-
-

tests/gateway/test_ws_auth_retry_verifier_probe.py

@@ -0,0 +1,142 @@
+"""Adversarial verifier probes for the mattermost-ws-401-classify fix
+(commit fdd1a11ac5).
+
+The implementer's test_ws_auth_retry.py covers:
+  - WSServerHandshakeError(status=401) stops the loop
+  - a transient RuntimeError whose message contains "401" now retries
+  - the pre-existing _closing early-return path is untouched
+
+This file probes boundary/edge cases the implementer's tests did NOT
+cover, per the independent-verifier mandate to go beyond what the
+implementer thought to test:
+
+  1. WSServerHandshakeError(status=403) — the other structured-check
+     status value — must still stop the loop (only 401 was tested).
+  2. WSServerHandshakeError with a non-auth status (e.g. 500) must NOT
+     stop the loop — the structured check must not over-match.
+  3. A transient exception containing "unauthorized" (not "401"/"403")
+     must retry now that the substring fallback is fully removed —
+     the implementer only exercised the "401" substring variant.
+  4. Multiple consecutive transient errors must all retry (not just
+     one) — proves the removed code path isn't silently reintroduced
+     via some other mechanism after N attempts.
+"""
+
+import asyncio
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import aiohttp
+
+from plugins.platforms.mattermost.adapter import MattermostAdapter
+
+
+def _make_adapter(closing: bool = False) -> MattermostAdapter:
+    adapter = MattermostAdapter.__new__(MattermostAdapter)
+    adapter._closing = closing
+    return adapter
+
+
+class TestMattermostWSAuthRetryBoundaryProbes:
+    def test_403_handshake_stops_reconnect(self):
+        """status=403 (the other half of the structured check's {401, 403}
+        set) must also stop the loop. The implementer only tested 401."""
+        exc = aiohttp.WSServerHandshakeError(
+            request_info=MagicMock(),
+            history=(),
+            status=403,
+            message="Forbidden",
+            headers=MagicMock(),
+        )
+
+        adapter = _make_adapter()
+        call_count = 0
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            raise exc
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        asyncio.run(adapter._ws_loop())
+
+        assert call_count == 1
+
+    def test_non_auth_handshake_status_does_not_stop_reconnect(self):
+        """A WSServerHandshakeError with a non-auth status (500) is a
+        structured exception of the RIGHT TYPE but the WRONG status —
+        it must NOT be classified as a permanent auth failure. This
+        guards against an overly broad isinstance-only check that
+        forgets to gate on .status."""
+        exc = aiohttp.WSServerHandshakeError(
+            request_info=MagicMock(),
+            history=(),
+            status=500,
+            message="Internal Server Error",
+            headers=MagicMock(),
+        )
+
+        adapter = _make_adapter()
+        call_count = 0
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            if call_count >= 2:
+                adapter._closing = True
+            raise exc
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        with patch("asyncio.sleep", new=AsyncMock()):
+            asyncio.run(adapter._ws_loop())
+
+        assert call_count == 2
+
+    def test_unauthorized_substring_no_longer_stops_reconnect(self):
+        """Before the fix, a transient error whose message contained the
+        word 'unauthorized' (not digits) would ALSO trip the removed
+        substring fallback. The implementer's regression test only
+        covered the '401' digit-substring case; this proves the
+        'unauthorized' word variant is equally fixed."""
+        adapter = _make_adapter()
+        call_count = 0
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            if call_count >= 2:
+                adapter._closing = True
+            raise RuntimeError(
+                "upstream proxy replied: request unauthorized by WAF rule, retry"
+            )
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        with patch("asyncio.sleep", new=AsyncMock()):
+            asyncio.run(adapter._ws_loop())
+
+        assert call_count == 2
+
+    def test_repeated_transient_errors_all_retry(self):
+        """Guards against a fix that only relaxes classification for the
+        FIRST occurrence (e.g. some hidden retry-budget/counter that
+        starts rejecting after N attempts). Runs 5 consecutive
+        transient errors and confirms every one retries."""
+        adapter = _make_adapter()
+        call_count = 0
+        target_attempts = 5
+
+        async def fake_connect():
+            nonlocal call_count
+            call_count += 1
+            if call_count >= target_attempts:
+                adapter._closing = True
+            raise RuntimeError("403 seen in unrelated proxy diagnostic body")
+
+        adapter._ws_connect_and_listen = fake_connect
+
+        with patch("asyncio.sleep", new=AsyncMock()):
+            asyncio.run(adapter._ws_loop())
+
+        assert call_count == target_attempts