Notify users via management-room notice when Discord invalidates their token#32
Merged
Conversation
…r token The bridge already detects bad-credentials events (invalidAuthHandler, handlePossible40002) and pushes a BridgeState update, but BridgeState is a webhook-only signal that most Matrix clients never render -- users only find out bridging stopped once they notice messages have stopped syncing, sometimes hours or days later (see docs/specs/2026-07-02-token-invalidation-notice.md, filed as #30). Add an m.notice to the user's management room alongside the existing BridgeState push, gated by a new token_invalidation_notices config option (default on). Dedupes on a single sentBadCredentialsNotice flag shared by both trigger points, since handlePossible40002 can be hit repeatedly by retried REST calls while the account stays broken; the flag releases on send failure so a later event can retry, and resets on the next successful Login. disconnectedHandler (transient disconnects) is untouched, so plain reconnects never notice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found during code review: the notice always posts to user.ManagementRoom, where commands never require the !discord prefix (per the bridge's own help text) -- the message was telling users to type the prefixed form, which would confuse anyone copying it literally. Also matches the existing ⚠ escape convention used for the warning glyph elsewhere in the file instead of a literal unicode character in source. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a user-visible Matrix management-room notification when Discord invalidates a user token, complementing the existing BridgeState bad-credentials signal that many Matrix clients don’t surface.
Changes:
- Send an
m.noticeinto the user’s management room oninvalidAuthHandler(4004) andhandlePossible40002(40002), with per-invalidation deduping viasentBadCredentialsNotice. - Introduce
bridge.token_invalidation_noticesconfig toggle (documented inexample-config.yaml) to enable/disable these notices. - Add unit tests for gating logic and dedup lifecycle; include a detailed investigation/spec writeup.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| user.go | Adds notice-sending logic + per-invalidation dedup flag, wired into existing bad-credentials trigger points. |
| user_notice_test.go | Adds tests for notice gating and dedup flag lifecycle behavior. |
| example-config.yaml | Documents new token_invalidation_notices option (default true). |
| docs/specs/2026-07-02-token-invalidation-notice.md | Adds investigation/spec writeup motivating the change and describing scope. |
| config/bridge.go | Adds the new config field to the bridge config struct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 245684ad3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow-up on code-review suggestions that were flagged non-blocking but worth acting on: - Added badCredentialsNoticeRetryCooldown (1 min) so a burst of handlePossible40002 calls during a sustained homeserver outage can't turn into a real send attempt on every single failed call. The dedup/cooldown decision is extracted into badCredentialsNoticeAttemptDecision, a pure function taking the current time as a parameter so the cooldown interaction is unit-testable without real timers. - Extracted a minimal noticeSender interface (matching *appservice.IntentAPI's SendMessageEvent signature) and moved the actual content-building/dispatch into sendBadCredentialsNoticeContent, which now has a real test double (fakeNoticeSender) exercising room targeting, event type, message body, and error propagation -- closing the "no integration test for actual dispatch" gap flagged in review, without needing a live Matrix homeserver or a bigger interface change to DiscordBridge.Bot itself. - Added an inline comment on invalidAuthHandler's early unlock explaining why (avoids deadlocking with sendBadCredentialsNotice, and brings it in line with handlePossible40002's existing locking), which was previously only explained in a commit message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Follow-up push addressing the remaining non-blocking review suggestions from the original pass:
|
…overy step Three issues raised by automated review (Copilot, Codex, Sentry), all verified before fixing: - config/upgrade.go didn't copy token_invalidation_notices, so a config upgrade would silently discard a user's customized value back to whatever example-config.yaml currently defaults to. Added the missing helper.Copy call, matching message_error_notices right above it. - sendBadCredentialsNotice read user.ManagementRoom twice with no lock shared with SetManagementRoom, so a concurrent room change between the gating check and the send could route the notice to the wrong room. Now read once into a local and reused for both. - Verified empirically (commands.go's fnLoginToken/fnLoginQR both check IsLoggedIn() and refuse with "You're already logged in"): handlePossible40002 never clears DiscordToken, so the notice's old universal "Run `login`" instruction was actively wrong for that path -- the user is still logged in, running login would just get refused. sendBadCredentialsNotice now takes an explicit recovery string per caller: invalidAuthHandler keeps "Run `login`" (Logout does clear the token there), handlePossible40002 gets "resolve it on Discord, no need to log in again". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The spec documented real production hostnames, incident timestamps, and account-specific session details from the investigation behind this PR. Per repo owner's decision, moving this out of git entirely rather than redacting in place -- it's preserved outside the repo for anyone who needs the full investigation trail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sentBadCredentialsNotice only reset on the next Login, but handlePossible40002 never logs the user out or clears DiscordToken -- so a session can keep running for a long time after one successful notice with no further Login to reset the flag. Any later bad-credentials event in that same session (the same still-broken account, or an entirely distinct new issue, including a real 4004) would have been silently suppressed for the rest of the session. Disagreed with the suggested fix (reset in readyHandler): a gateway reconnect has nothing to do with whether the underlying account issue actually resolved, especially for the REST-triggered 40002 path, and resetting there risked reintroducing notice spam on ordinary reconnects while a problem is still unresolved. Instead, replaced the permanent "already sent" block with a resend cooldown (1 hour) parallel to the existing failure-retry cooldown (1 minute) -- alreadySent now means "the last attempt succeeded", gating a longer cooldown rather than blocking forever. A persisting or recurring issue gets a fresh reminder well within a day instead of never again. Added a direct regression test for the exact scenario Sentry flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR #31 (the backoff-reset fix) merged into main while this PR was still in review, bringing user_reconnect_test.go's newTestUser helper into the same package as this PR's own copy in user_notice_test.go -- CI caught the redeclaration once both were combined. Removed the duplicate here; the shared helper now lives in user_reconnect_test.go only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a13ee79d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| user.bridgeStateLock.Lock() | ||
| attempt, skipReason := badCredentialsNoticeAttemptDecision(user.sentBadCredentialsNotice, user.lastBadCredentialsNoticeAttempt, time.Now()) |
There was a problem hiding this comment.
When a 40002 notice is sent successfully, this decision treats any later bad-credentials notice as already sent for the next hour, regardless of errorCode or recovery text. If Discord then closes the gateway with 4004 during that window, the invalidAuthHandler notice is skipped even though that path logs the user out and needs the opposite instruction (login instead of “do not run login”), leaving the user with only stale 40002 advice after their token has been cleared. Key the cooldown by the notice type/recovery, or let the logout/4004 notice override the 40002 cooldown.
Useful? React with 👍 / 👎.
Author
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #30.
The bridge already detects bad-credentials events (
invalidAuthHandler,handlePossible40002) and pushes aBridgeStateupdate, butBridgeStateis a webhook-only signal most Matrix clients never render — users only find out bridging stopped once they notice messages have stopped syncing, sometimes hours or days later.m.noticeto the user's management room alongside the existingBridgeStatepush, gated by a newtoken_invalidation_noticesconfig option (default on).sentBadCredentialsNoticeflag shared by both trigger points, sincehandlePossible40002can be hit repeatedly by retried REST calls while the account stays broken. The flag releases on send failure so a later event can retry, and resets on the next successfulLogin.disconnectedHandler(transient disconnects) is untouched, so a plain reconnect never notices — verified by diff, not just by inspection.`!discord login`), but the notice always lands inuser.ManagementRoom, where the bridge's own help text says commands never need the prefix — fixed before merge.Includes the investigation writeup (
docs/specs/2026-07-02-token-invalidation-notice.md) that led here, including a separate, more severe finding (a live reconnect storm) tracked in a companion PR rather than bundled into this one, since the code doesn't overlap.Test plan
go build ./.../go vet ./.../gofmt— cleango test ./...— cleanbadCredentialsNoticeSkipReason) table-driven across config-toggle × management-room-present; dedup flag lifecycle.user.bridge.Bot), so the actualSendMessageEventdispatch isn't covered by an automated test — recommend a manual end-to-end check against a disposable test account (force a4004/40002, confirm the notice lands and a re-login doesn't duplicate it) before wide rollout.🤖 Generated with Claude Code