dark/nether-voicebridge
1
0
Fork 1
Code Issues 37 Pull requests Projects 1 Releases 3 Packages 1 Activity Actions

Kitchen Sink - Remove Access Token Path, Bot Name/Localpart Fix #31

Closed
shadowdrake wants to merge 0 commits from shadowdrake/nether-voicebridge:master into master
pull from: shadowdrake/nether-voicebridge:master
merge into: dark:master
Conversation 4 Commits - Files changed -
shadowdrake commented 2026-06-19 06:37:24 +00:00
Collaborator
Copy link

Three changes here in prep for deployment on a MDAD-deployed server - #9

  1. Removal from documentation and code of access token path - it's not used any more so it should be cleaned up. It's worth taking another pass through the code to be sure I got it all... I probably didn't.
  2. Removed the error-on start if config.toml is missing. Instead it will check for environment variables. I probably should return to this and if there is no config.toml and no environment variables set, it should generate an example config.toml and exit gracefully with a message with the config file path... but this is a start
  3. Synapse gets unhappy if the bot username is inside of the sender_localpart namespace - this fixes that. During the troubleshooting process I re-worked the sync to be selective about event subscriptions... this wasn't the root cause I thought it was but this is still a worthwhile improvement. It works on Synapse but please test against tuwunel before deploy
Three changes here in prep for deployment on a MDAD-deployed server - #9 1) Removal from documentation and code of access token path - it's not used any more so it should be cleaned up. It's worth taking another pass through the code to be sure I got it all... I probably didn't. 2) Removed the error-on start if config.toml is missing. Instead it will check for environment variables. I probably should return to this and if there is no config.toml and no environment variables set, it should generate an example config.toml and exit gracefully with a message with the config file path... but this is a start 3) Synapse gets unhappy if the bot username is inside of the sender_localpart namespace - this fixes that. During the troubleshooting process I re-worked the sync to be selective about event subscriptions... this wasn't the root cause I thought it was but this is still a worthwhile improvement. **It works on Synapse but please test against tuwunel** before deploy
robocub commented 2026-06-19 16:58:55 +00:00
Collaborator
Copy link

Thanks for this — the access-token removal, the env-var config fallback, and the _bot localpart fix all look good and we want them.

One blocker before merge: this also migrates the bot's sync from /v3/sync to Simplified Sliding Sync, and a live A/B on Tuwunel shows the SSS path regresses live call-membership detection. Under SSS the SyncCallMemberEvent room handler stops firing after the initial sync, so the bridge never sees a human join the call — no puppet is assigned, ghosts never go live, and the Matrix user sees no ghosts. The same config on current /v3/sync master works correctly.

Tuwunel does speak native SSS (no build/.build() error), so this isn't a homeserver-support issue — the required_state CallMember deltas just aren't reaching the event handler (likely because the room is only a subscribe_to_rooms subscription with no sliding-sync list).

Importantly, the _bot rename alone fixes the Synapse sender_localpart sync block — the ghosts already sync fine on /v3/sync as non-sender namespace users, which is the proof that a non-sender bot user doesn't need SSS. So the SSS rewrite isn't required for the Synapse fix.

Could you split it — keep #1/#2/_bot, and drop (or land separately, with SSS proven on Tuwunel and the ghosts migrated too for consistency) the sync rewrite? Happy to share the A/B logs.

Thanks for this — the access-token removal, the env-var config fallback, and the `_bot` localpart fix all look good and we want them. One blocker before merge: this also migrates the bot's sync from `/v3/sync` to **Simplified Sliding Sync**, and a live A/B on Tuwunel shows the SSS path **regresses live call-membership detection**. Under SSS the `SyncCallMemberEvent` room handler stops firing after the initial sync, so the bridge never sees a human join the call — no puppet is assigned, ghosts never go live, and the Matrix user sees no ghosts. The same config on current `/v3/sync` `master` works correctly. Tuwunel **does** speak native SSS (no build/`.build()` error), so this isn't a homeserver-support issue — the `required_state` `CallMember` deltas just aren't reaching the event handler (likely because the room is only a `subscribe_to_rooms` subscription with no sliding-sync list). Importantly, the `_bot` rename alone fixes the Synapse `sender_localpart` sync block — the ghosts already sync fine on `/v3/sync` as non-sender namespace users, which is the proof that a non-sender bot user doesn't need SSS. So the SSS rewrite isn't required for the Synapse fix. Could you split it — keep #1/#2/`_bot`, and drop (or land separately, with SSS proven on Tuwunel **and** the ghosts migrated too for consistency) the sync rewrite? Happy to share the A/B logs.
shadowdrake commented 2026-06-19 17:27:07 +00:00
Author
Collaborator
Copy link

@robocub - did a test build and as-is runs & works okay on Synapse! The sync calls aren't failing. I don't have any users in a call now but this feels pretty safe to go

@robocub - did a test build and as-is runs & works okay on Synapse! The sync calls aren't failing. I don't have any users in a call now but this feels pretty safe to go
dark commented 2026-06-23 18:22:36 +00:00
Owner
Copy link

Merged.

Merged.
dark closed this pull request 2026-06-23 18:22:36 +00:00
robocub commented 2026-06-23 18:29:34 +00:00
Collaborator
Copy link

Merged & released 🎉

Landed on master in 60d2107 (merged --no-ff, so every commit is preserved) and shipped in v0.2.1 — stable .deb/.rpm/tarball + Docker image on the release page. Thanks @shadowdrake, and to jasonlaguidice for the commits!

Shipped scope: appservice-login-only auth (the access_token path is removed), @{sender_localpart}_bot bot derivation, ghost localpart prefix derived from sender_localpart (closes #37), and the CONFIG_PATH→env fallback. The sliding-sync rewrite was dropped — reverted in 0a2cb58 "Restore sync status" — so the bridge stays on classic /v3/sync (an earlier A/B test showed SSS stopped the m.call.member handler from firing on Tuwunel).

Follow-up review pass (commit e091eef) on top:

  • is_ghost_identity now requires an all-numeric suffix, so the derived @{sender_localpart}_bot user isn't misclassified as a ghost — otherwise the startup sweep would retract the bot's own m.call.member.
  • the prior default nether-voicebridge_<digits> prefix is recognized as legacy, so changing sender_localpart cleans up orphaned ghosts at startup.
  • docs/CHANGELOG: the appservice namespace-stem rule + an "Upgrading" section.

Live-verified both directions in encrypted and unencrypted rooms before the release. Closing as merged-by-hand (Forgejo didn't auto-flag it since the merge landed via a local branch rather than the UI).

Merged & released 🎉 Landed on `master` in 60d2107 (merged `--no-ff`, so every commit is preserved) and shipped in **v0.2.1** — stable `.deb`/`.rpm`/tarball + Docker image on the release page. Thanks @shadowdrake, and to jasonlaguidice for the commits! **Shipped scope:** appservice-login-only auth (the `access_token` path is removed), `@{sender_localpart}_bot` bot derivation, ghost localpart prefix derived from `sender_localpart` (**closes #37**), and the `CONFIG_PATH`→env fallback. The sliding-sync rewrite was dropped — reverted in 0a2cb58 *"Restore sync status"* — so the bridge stays on classic `/v3/sync` (an earlier A/B test showed SSS stopped the `m.call.member` handler from firing on Tuwunel). **Follow-up review pass** (commit e091eef) on top: - `is_ghost_identity` now requires an all-numeric suffix, so the derived `@{sender_localpart}_bot` user isn't misclassified as a ghost — otherwise the startup sweep would retract the bot's own `m.call.member`. - the prior default `nether-voicebridge_<digits>` prefix is recognized as legacy, so changing `sender_localpart` cleans up orphaned ghosts at startup. - docs/CHANGELOG: the appservice namespace-stem rule + an "Upgrading" section. Live-verified both directions in encrypted and unencrypted rooms before the release. Closing as merged-by-hand (Forgejo didn't auto-flag it since the merge landed via a local branch rather than the UI).

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
dark/nether-voicebridge!31
No description provided.