Skip to content

fix(matrix): stop classifying auth errors by substring match - #57375

Open
gmoranxyz wants to merge 1 commit into
NousResearch:mainfrom
gmoranxyz:fix/matrix-auth-error-substring-match
Open

fix(matrix): stop classifying auth errors by substring match#57375
gmoranxyz wants to merge 1 commit into
NousResearch:mainfrom
gmoranxyz:fix/matrix-auth-error-substring-match

Conversation

@gmoranxyz

Copy link
Copy Markdown

Summary

  • The matrix sync loop's permanent-auth-error detection matched "401"/"403"/"unauthorized"/"forbidden" against str(exc), including the raw body of non-Matrix error responses.
  • A transient reverse-proxy error page (returned when the homeserver briefly restarted) happened to contain the digits "401" inside an SVG path coordinate in its embedded logo. That falsely tripped the permanent-auth-error path and permanently killed the sync loop on what was actually a transient connection blip — the bot stayed disconnected for hours until the process was manually restarted, even though the homeserver was healthy again within seconds.
  • This switches the check to use the structured http_status/errcode attributes that mautrix's own exception types (MatrixRequestError and subclasses) set for genuine Matrix API error responses, instead of substring-matching arbitrary exception text.

Test plan

  • python -m py_compile plugins/platforms/matrix/adapter.py
  • Run scripts/run_tests.sh tests/ for matrix platform coverage, if any exists

The sync loop's permanent-auth-error detection matched "401"/"403"/
"unauthorized"/"forbidden" against str(exc), including the raw body
of non-Matrix error responses. A transient Umbrel proxy error page
(returned when Synapse briefly restarted) contained an SVG path
coordinate with the digits "401" in it, which falsely tripped the
permanent-auth-error path and permanently killed the sync loop on a
purely transient connection blip.

Use the structured http_status/errcode from mautrix's own exception
types instead, which are only set for real Matrix API error
responses.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/matrix Matrix adapter (E2EE) sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the Matrix permanent-auth classifier cluster: #56532 (issue), #56547 and #57150 (fix the false-negative — loop won't stop on 'Unable to introspect the access token'). This PR fixes the opposite false-positive — the loop wrongly stopping on a transient proxy error page that coincidentally contained '401' — by replacing substring matching on str(exc) with mautrix's structured http_status/errcode. That structured approach would largely subsume the keyword-list PRs. Flagging the cluster so a maintainer can pick the canonical classifier.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for replacing text-based classification with the SDK's structured error data. The false-positive is present on current main at plugins/platforms/matrix/adapter.py:2335-2346, and the patch applies cleanly.

Problems

  • tests/gateway/test_ws_auth_retry.py:155-188 currently requires a plain RuntimeError("HTTP 401 Unauthorized") to stop after one attempt. With this patch it has neither http_status nor errcode, so it reaches the retry branch forever. Update that test to the structured SDK contract and add the proxy-body false-positive regression.
  • The new errcode list omits M_UNAUTHORIZED; mautrix 0.21.0 declares it at upstream mautrix/errors/request.py:188-190.

Suggested changes

  • Cover both a structured Matrix auth exception that stops and an unstructured exception whose text contains 401 that retries/recoveres.
  • Add M_UNAUTHORIZED to the errcode classification if errcode is intended to be independently terminal.

Automated hermes-sweeper review.

http_status = getattr(exc, "http_status", None)
errcode = (getattr(exc, "errcode", None) or "").upper()
if http_status in (401, 403) or errcode in (
"M_UNKNOWN_TOKEN",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include M_UNAUTHORIZED in this structured errcode set. The pinned mautrix 0.21.0 exposes it as a standard request error (mautrix/errors/request.py:188-190), so errcode-based classification otherwise omits one of the auth codes the old text matcher covered.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Two open PRs address the same Matrix sync-loop false positive: #57375 removes exception-text matching in favor of structured mautrix status/errcode fields, while #66878 adds a dedicated classifier, preserves bounded fallback parsing for unstructured errors, and includes direct classifier tests for the reported 502/SVG case.

Related pull requests

  • #57375 related — (+11/-7) — keep open pending consolidation: the diff directly removes the unsafe substring match that caused transient proxy bodies containing 401/403 to halt sync, but it lacks regression coverage, breaks the existing plain-RuntimeError expectation, and omits M_UNAUTHORIZED, as identified by the keep_open review on #57375.
  • #66878 duplicate — (+104/-8) — preferred consolidation base: the diff fixes the same root cause while covering structured statuses, leading textual statuses, and the reported 502/SVG payload; consistent with the keep_open review on #66878, it still needs an async _sync_loop regression proving retry-versus-return behavior, and salvage should explicitly decide whether retaining bounded unstructured-text fallback is acceptable.

Duplicates

#57375 and #66878 substantially duplicate the replacement of broad str(exc) substring classification for Matrix permanent-auth failures; #66878 is the broader implementation and test-bearing variant.

Suggested consolidation

Merge #66878 after adding the requested loop-level regression and explicitly resolving the bounded unstructured-text fallback policy; also include M_UNAUTHORIZED if errcode alone is terminal. Then close #57375 as superseded by #66878, carrying over its stricter structured-data rationale rather than merging both competing classifiers.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup57375 ["PRs duplicating each other"]
        P57375["PR #57375 (open)"]
        P66878["PR #66878 (open)"]
    end
    class P57375 open
    class P66878 open
    class P57375 target
    click P57375 "https://github.com/NousResearch/hermes-agent/pull/57375"
    click P66878 "https://github.com/NousResearch/hermes-agent/pull/66878"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 8 kB of PR diffs, 3 kB of issue/PR text, 3 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/matrix Matrix adapter (E2EE) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants