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

[steveonjava]

Summary

The Mattermost WebSocket reconnect loop in _ws_loop() misclassifies transient
errors as permanent auth failures. A substring fallback sits below the correct
WSServerHandshakeError/.status check. Any exception whose stringified message
happens to contain "401", "403", or "unauthorized" kills the reconnect loop
forever, even when the underlying cause is temporary.

Root cause

The exception handler in plugins/platforms/mattermost/adapter.py first checks
isinstance(exc, aiohttp.WSServerHandshakeError) and exc.status in {401, 403},
which correctly catches the one case where the server explicitly rejects auth.
Below that, a catch-all substring check on str(exc).lower() treats any
coincidental mention of "401", "403", or "unauthorized" as a permanent failure:

err_str = str(exc).lower()
if "401" in err_str or "403" in err_str or "unauthorized" in err_str:
    logger.error("Mattermost WS permanent error: %s", exc)
    return

A proxy returning a 401 in its response body but then resetting the connection,
or an unrelated error whose message happens to contain the word "unauthorized",
both get classified as permanent auth failures and stop the retry loop
permanently.

Fix

Delete the redundant substring fallback. The structured WSServerHandshakeError
check already catches the one legitimate permanent-auth case (HTTP 401/403 from
the Mattermost server). No other exception in the aiohttp exception hierarchy
simultaneously signals permanent auth failure and evades the structured check.

Regression tests

Added in tests/gateway/test_ws_auth_retry.py:

• test_transient_401_substring_does_not_stop_reconnect: a RuntimeError
  whose message contains "401" now retries instead of returning.
• test_closing_flag_prevents_further_connect_attempts: the existing
  _closing early-return path is untouched.

Added in tests/gateway/test_ws_auth_retry_verifier_probe.py (independent
verifier, adversarial edge cases):

• test_403_handshake_stops_reconnect: status=403 still stops the loop.
• test_non_auth_handshake_status_does_not_stop_reconnect: status=500
  does not stop the loop.
• test_unauthorized_substring_no_longer_stops_reconnect: the word
  "unauthorized" no longer trips a false permanent-fatal.
• test_repeated_transient_errors_all_retry: five consecutive transient
  errors all retry (not just the first one).

Files changed

• plugins/platforms/mattermost/adapter.py: removed 4 lines (substring fallback)
• tests/gateway/test_ws_auth_retry.py: added 2 new tests (+57 lines)
• tests/gateway/test_ws_auth_retry_verifier_probe.py: new file, 4 boundary-case probes (+142 lines)

Refs #35645

This PR conflicts with #35645 (open). Both change the same _ws_loop() exception
handler in opposite directions, and an earlier version of this description
incorrectly claimed zero line overlap. Verified with a three-way merge against
main (b3aa561f), merging this PR's bd674b78 with #35645's c94baf52:
exactly one conflict in plugins/platforms/mattermost/adapter.py, on the
substring branch.

#35645 keeps the substring-detected branch and escalates it through a new
_escalate_ws_fatal with retryable=False. This PR deletes the branch so the
same exceptions fall through to the reconnect path. The escalation in #35645 is
correct for the structured WSServerHandshakeError case and fixes a real zombie
adapter bug. The disagreement is only about the substring fallback underneath it.

These are two different error classes, which is the reason for the split:
WSServerHandshakeError with exc.status in {401, 403} is a structured auth
rejection from the server, and both PRs keep that branch untouched. A
RuntimeError whose message merely contains "401" is a transient network failure
that happens to include three digits.

Ordering hazard for whoever resolves the conflict: this PR also removes the
err_str = str(exc).lower() assignment, which becomes unused once the branch is
gone. Keeping #35645's side of the hunk without restoring that assignment raises
NameError on the first WebSocket exception. It fails on the error path, so
happy-path tests will not catch it.

Proposed reconciliation: convert the branch to a structured attribute check
rather than deleting it, then escalate through _escalate_ws_fatal when it
genuinely fires. That preserves #35645's escalation contract while preventing
transient errors from being classified as permanent.

The Matrix adapter has the same substring bug on main today. #57375
(fix(matrix): stop classifying auth errors by substring match) proposes the
attribute-check approach there, switching to http_status and errcode on the
exception object. That PR is still open and not merged; an earlier version of
this description described it as a fix Matrix had already received, which was
wrong.

[spfcraze]

This was generated by AI during triage.

Summary:
The description cites #35645 as a non-overlapping sibling ("zero line overlap"), but that open PR keeps the exact substring branch this PR deletes and escalates it as a non-retryable fatal — the opposite classification for the same error class.

Problems:

• fix(mattermost): classify API errors, escalate fatals, lock single-instance, surface audio/slash attachments #35645 (open) keeps the substring-detected permanent-error path in _ws_loop() and escalates it: its body says "the substring-detected permanent-error path now escalate instead of returning silently" and records it with retryable=False.
• This PR's own test_transient_401_substring_does_not_stop_reconnect asserts the same error class — a RuntimeError whose message contains "401" — now retries instead of returning.

Solution:
A description that names the conflict: #35645 (open) keeps the substring-detected branch and escalates it as a non-retryable fatal, while this PR's removal — pinned by its own tests — makes the same errors retry.

---

Checked against bd674b7 — the tip of feat/mattermost-ws-401-classify when this was written — and 3671c9f, main at the same moment.

[steveonjava]

You're right that the "zero line overlap" line is wrong, and I'll fix it. I ran a real three-way merge to check: with main (b3aa561f) as base, merging bd674b78 against #35645's c94baf52 produces exactly one conflict in adapter.py, and it is this branch. My description shouldn't have claimed otherwise.

But the framing of "the opposite classification for the same error class" is where I'd push back, because these are not the same error class. WSServerHandshakeError with exc.status in {401, 403} is a structured auth rejection from the server. RuntimeError("proxy returned HTTP/1.1 401 in body but connection reset") is a transient network failure that merely contains three digits. This PR keeps the first branch untouched. Both PRs keep it. The only thing removed is the substring guess underneath it, and #35645 doesn't reclassify that error, it inherits the existing classification and adds escalation on top.

That distinction matters a great deal here, because what's on main right now is genuinely bad. Any exception whose stringified message happens to contain "401", "403", or "unauthorized" permanently kills the WebSocket listener. A proxy error body, a 502 page that quotes an upstream 401, a DNS message mentioning an unauthorized resolver. Worse, the branch does a bare return, so _running stays True from _mark_connected() and is_connected() keeps returning True. The bot goes dark, the gateway is never told, and nothing reconnects. You don't get an error, you get a Mattermost bot that looks healthy in every status check and silently stops receiving messages until someone notices and restarts it. A momentary blip takes the integration down permanently.

Now stack #35645 on top of that unchanged: it converts the same false positive into _set_fatal_error(retryable=False), which tells the gateway to drop the platform and not retry. The escalation is correct and I want it. Applied to the handshake path it fixes a real zombie bug. Applied to the substring path it makes a transient proxy hiccup permanently unrecoverable instead of merely silent, and there's an ordering hazard nobody has flagged: this PR also deletes the err_str = str(exc).lower() assignment. Resolve the conflict by keeping #35645's side and you get a NameError on the first WebSocket exception, on the error path, where no happy-path test will ever see it.

One correction on the other side. #57375 is not merged, and my description was wrong to imply it was, but the substring problem it fixes is live on Matrix today too, so this is a pattern across adapters and not a one-off. The reconciliation is straightforward and I'll do the work: make the branch an attribute check rather than deleting it, then escalate through _escalate_ws_fatal when it genuinely fires. That keeps #35645's contract intact and stops transient errors from being sentenced as permanent. Given #35645 is mergeable: false, last touched May 31, and bundles the API classification, platform lock, and attachment changes, I'd rather not leave main in this state while it rebases. Happy to land the narrow fix now and rebase to the attribute check, or do it in whatever order you prefer, but I don't think this one should sit.

[teknium1]

Merged via PR #85157 (#85157) — both of your commits were cherry-picked onto current main with your authorship preserved in git history, including the adversarial verifier-probe file.

Your review-thread analysis was exactly right on all three points (structured vs substring error classes, the silent-zombie _running hazard, and the err_str NameError ordering trap for #35645), so we shipped the escalation half ourselves on top: the genuine 401/403 handshake branch now sets a non-retryable mattermost_auth_error and notifies the gateway fatal handler instead of the bare return. One small fix on the probe file: module-level import aiohttp became pytest.importorskip so collection survives envs without the optional dep.

Thanks for an unusually thorough contribution — the write-up in the comments here was better than most PR bodies.