fix(mattermost): classify API errors, escalate fatals, lock single-instance, surface audio/slash attachments

[lambertian]

Hardens the Mattermost adapter against eight independently-verified reliability bugs, each with focused regression coverage (15 new tests: 13 red->green for the fixes plus 2 green->green guards asserting genuine 4xx failures stay non-retryable; full file 58 passing, 43 existing + 15). Every fix mirrors an established sibling-adapter convention (IRC/Teams/Discord/Slack) rather than inventing new behavior, and the outbound-error plumbing is unified through one structured error type so transient vs permanent failures are classified once and reused by both send() and edit_message().

1. Outbound sends silently dropped on transient 5xx/429/network failures

Symptom: a momentary network blip, a 5xx, or a 429 rate-limit while posting a response caused the agent's reply to vanish with no retry and no user-facing notice.

Root cause: _api_post swallowed both HTTP error responses (any status >= 400, including 429 and 5xx) and aiohttp.ClientError into a single empty-dict sentinel. send() then returned SendResult(success=False, error="Failed to create post") with retryable defaulting to False. BasePlatformAdapter._send_with_retry decides whether to retry via result.retryable or self._is_retryable_error(error_str); the static string matches none of the retryable patterns and retryable was False, so the exponential-backoff loop was skipped, only the single plain-text fallback attempt ran, and the retry-exhausted user notice (emitted only inside the network branch) never fired.

Fix: a new _MMApiError carries a retryable flag set True for network errors, timeouts, HTTP 429, and 5xx, and False for genuine 4xx. A shared _request_json raises it; send() translates it into SendResult(success=False, error=str(exc), retryable=exc.retryable), mirroring Slack's send() returning str(e). The legacy _api_get/_api_post/_api_put wrappers preserve the empty-dict sentinel for callers that still rely on it.

2. Streamed-response progress editing permanently disabled by one transient edit blip

Symptom: a single transient API failure during streaming downgraded the entire remaining response to spammy append-only sends.

Root cause: edit_message() returned SendResult(success=False, error="Failed to edit post") with retryable unset. The stream consumer sets can_edit = False for the rest of the response when an edit fails, getattr(result, 'retryable', False) is False, and the error contains neither 'flood' nor 'retry after' — all true here regardless of whether the failure was transient.

Fix: edit_message() returns retryable=exc.retryable from the same _MMApiError, so the consumer keeps can_edit alive across transient edit failures and only gives up on real 4xx. Mirrors Teams' send/edit handlers returning retryable=True.

3. connect() retried forever on permanent auth/config failure

Symptom: a revoked/invalid token, missing permissions, or a wrong URL caused the adapter to re-create a session and re-hit users/me forever (backoff-capped) instead of dropping out of the retry queue.

Root cause: both failure exits (missing URL/token; failed GET users/me) returned False without calling _set_fatal_error. The gateway startup path treats a False return with no fatal error as transient and queues it for indefinite reconnection.

Fix: missing config calls _set_fatal_error("config_missing", ..., retryable=False); a non-retryable _MMApiError from users/me (4xx) calls _set_fatal_error("auth_failed", ..., retryable=False) so the gateway drops the platform; a transient _MMApiError (network/5xx) sets retryable=True so a genuinely flaky connection still retries. Mirrors IRC's config_missing and Teams' MISSING_CREDENTIALS non-retryable escalation.

4. Dead WebSocket listener never escalated -> zombie adapter

Symptom: on a permanent WS auth/permission failure the listener task ended while the adapter still reported itself connected; the gateway was never informed and never reconnected, so inbound messages were silently lost.

Root cause: the permanent-auth branch in _ws_loop did a bare return, ending _ws_task while _running stayed True (set by _mark_connected). It never called _set_fatal_error or _notify_fatal_error, and there is no health poll to notice.

Fix: a new _escalate_ws_fatal records _set_fatal_error("ws_auth_failed", ..., retryable=False) (which also clears _running) and awaits _notify_fatal_error(), bridging the dead listener back to the gateway's fatal machinery. Both the WSServerHandshakeError 401/403 path and the substring-detected permanent-error path now escalate instead of returning silently. Mirrors IRC's receive-loop finally, which calls _set_fatal_error + _notify_fatal_error on connection loss.

5. disconnect() left is_connected and runtime status stuck at 'connected'

Symptom: after a clean shutdown, is_connected kept returning True and the runtime status file still reported 'connected'.

Root cause: disconnect() closed the task/socket/session but never called _mark_disconnected(), so _running stayed True.

Fix: disconnect() now calls _mark_disconnected() after closing the session (and releasing the lock), matching IRC and Teams.

6. Single-instance lock missing -> two gateways double-process every post

Symptom: two gateway processes sharing one token each open their own /api/v4/websocket listener and both dispatch handle_message for the same post, producing duplicate agent runs/replies.

Root cause: connect() never acquired a scoped platform lock; the per-process in-memory MessageDeduplicator cannot dedup across processes.

Fix: connect() calls _acquire_platform_lock("mattermost-token", f"{base_url}|{token}", "Mattermost bot token") after config validation, before opening the session; disconnect() calls _release_platform_lock(). Every post-acquire failure exit (transient connect_failed, permanent auth_failed, and the not me or "id" not in me branch) also calls _release_platform_lock() before returning False, so a permanent auth failure cannot leak the lock and strand the token. A conflict records a non-retryable fatal error automatically. Mirrors Discord, which releases the lock on its post-acquire failure exits (timeout, generic exception), and Signal/Slack.

7. Slash-captioned attachment mis-typed COMMAND -> file dropped

Symptom: uploading a file with a caption that begins with '/' (e.g. /notes for review, or a pasted path) produced a message tagged COMMAND with the media never surfaced; downstream document surfacing requires message_type == DOCUMENT, so the cached file was silently dropped.

Root cause: msg_type was set to COMMAND from the leading slash before attachments were inspected, and the media-type override was guarded on msg_type == MessageType.TEXT only, so it never ran for the slash case.

Fix: the override guard is relaxed to msg_type in {MessageType.TEXT, MessageType.COMMAND}, so a downloaded attachment's media type (PHOTO/AUDIO/DOCUMENT) wins over a provisional COMMAND tag and the file is surfaced to the agent.

8. All inbound audio force-classified VOICE (force-STT), never offered as a file

Symptom: any uploaded audio (music, a podcast, a non-speech clip) was force-routed to speech-to-text and the agent was never handed the actual file.

Root cause: every audio/ attachment was mapped to MessageType.VOICE, which run.py consumes only via transcription; the file path is surfaced to the agent only for MessageType.AUDIO.

Fix: inbound audio is classified MessageType.AUDIO. Mattermost has no distinct voice-note concept, so an audio upload is an ordinary file; run.py now surfaces the cached path to the agent. Mirrors Discord, which reserves VOICE for true voice-message attachments and uses AUDIO for ordinary audio files.

Overlap

• fix(mattermost): use per-call aiohttp ClientSession to avoid cross-loop bug #35343 (open) rewrites the same _api_get/_api_post/_api_put helpers and the connect() session setup to use a per-call aiohttp.ClientSession (cross-loop fix). This is a direct conflict with the _request_json refactor here, which also consolidates those helpers and rebuilds the session/auth path in connect(). Reconciliation: keep fix(mattermost): use per-call aiohttp ClientSession to avoid cross-loop bug #35343's per-call-session lifetime as the base and re-apply this change's transient/permanent error classification on top — route each per-call request through _request_json so the retryable distinction and the _MMApiError plumbing survive, rather than reverting to the empty-dict sentinel. The two are complementary (session lifetime vs error typing) but must be merged by hand, not auto-applied.
• fix: clean Mattermost progress breadcrumbs #33012 (open) edits the Mattermost progress/breadcrumb path and adds to the adapter near edit_message; it touches a method this change also rewrites (edit_message now returns retryable=...). No logical conflict, but the hunks overlap and will need a manual merge.
• feat(mattermost): add interactive approval buttons via send_exec_approval #29373 (open) adds interactive approval buttons and inserts a large block into the adapter spanning the connect()/disconnect() region this change also modifies (lock release, _mark_disconnected). Different feature, shared methods — expect a textual merge in connect/disconnect.
• fix(mattermost): accept leading-space slash commands #32210 (open) accepts leading-space slash commands in the same adapter; fix 7 here concerns attachment classification when a caption begins with '/', a distinct code path, so the behaviors are independent even though both touch command handling.
• fix(simplex): escalate non-retryable WebSocket handshake rejections instead of looping forever #35557 (SimpleX) escalates non-retryable WebSocket handshake rejections — the same bug class as fix 4 but in a different adapter; no file overlap. This change applies the identical contract to Mattermost.
• fix(feishu): map audio messages to VOICE for STT transcription (#30822) #30829 / fix(feishu): classify native voice messages as VOICE for auto-transcription #29235 (Feishu) map audio to VOICE; fix 8 is the inverse for Mattermost (VOICE -> AUDIO) and is platform-specific because Mattermost has no voice-note concept. Different adapter, no overlap.
• No open or merged Mattermost PR touches the platform-lock, fatal-escalation, _mark_disconnected, or retryable-SendResult contracts introduced here.

[tonydwb]

Code Review Summary

Verdict: Approved ✅ — Mattermost platform reliability fixes.

Changes

Classify API errors, escalate fatals, lock single-instance, surface audio/slash attachments.

---

Reviewed by Hermes Agent

[teknium1]

Thanks for the focused Mattermost reliability coverage. Current main still has the retry, lifecycle, and inbound-media defects this targets, so the work is useful to salvage.

Problems

• PR plugins/platforms/mattermost/adapter.py:142 removes the current .. API-path validation. Main added that protection in d836b2bac after event-controlled IDs could steer authenticated bearer-token requests; keep the validation in the shared _request_json() path.
• PR plugins/platforms/mattermost/adapter.py:253 assumes a lock conflict is non-retryable, but _acquire_platform_lock() currently records retryable=True at gateway/platforms/base.py:2769. The new conflict-test expectation therefore does not match the helper it invokes.
• The branch's direct send() request path predates current _post_preserving_thread() delivery behavior at plugins/platforms/mattermost/adapter.py:367; preserve that behavior during reconciliation.

Suggested changes

• Put the path guard in _request_json() and retain wrapper compatibility.
• Align lock-conflict semantics and its regression test with the shared helper, or make any helper-contract change explicit and cross-platform.
• Reapply typed transient/permanent errors onto the current thread-preserving send path.

Automated hermes-sweeper review.

[teknium1]

Please retain the .. path rejection before constructing this URL. Current main added it in d836b2bac because WebSocket-event IDs can be attacker-controlled and otherwise steer authenticated bearer-token requests; the legacy wrappers no longer protect this shared request path.

[teknium1]

_acquire_platform_lock() currently records lock conflicts with retryable=True (gateway/platforms/base.py:2769), so this failure will enter the reconnect queue. The new test/comment expecting a non-retryable fatal needs to match that shared contract or deliberately change it.

[teknium1]

Heads-up on scope overlap: PR #85157 (merged, salvaging #80489) just changed the _ws_loop() error handling this PR touches — the substring fallback (err_str = str(exc).lower() + "401"/"403"/"unauthorized" matching) is removed from main, and the structured WSServerHandshakeError 401/403 branch now escalates through the fatal-error hook as a non-retryable mattermost_auth_error.

That supersedes this PR's substring-escalation half (and note the ordering hazard flagged in the #80489 thread: a conflict resolution that keeps this branch's side would reference the now-deleted err_str and NameError on the first WS exception). The rest of this PR's scope — API error classification, single-instance platform lock, audio/slash attachment surfacing — is untouched and still welcome. A rebase onto current main dropping the WS-loop hunk would get this reviewable again.