WPB-21964: move conversation creation to wire-subsystems by blackheaven · Pull Request #4977 · wireapp/wire-server

blackheaven · 2026-01-22T17:01:02Z

https://wearezeta.atlassian.net/browse/WPB-21964

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

Copilot

Pull request overview

This PR refactors conversation creation logic by moving it from Galley to the wire-subsystems library, aligning with the architectural goal of separating concerns into reusable subsystems.

Changes:

Moved conversation creation logic from Galley.API.Create to Wire.ConversationSubsystem.Interpreter
Relocated utility modules: Galley.API.Error → Galley.Types.Error, Galley.API.One2One → Wire.ConversationSubsystem.One2One, Galley.API.Util → Wire.ConversationSubsystem.Util, Galley.Effects.ClientStore → Wire.Effects.ClientStore
Removed Galley.Validation module (functionality moved to interpreter)
Updated background-worker to include new dependencies and error handlers
Updated imports across numerous files to reflect module movements

Reviewed changes

Copilot reviewed 63 out of 64 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
services/galley/src/Galley/API/Create.hs	Simplified to delegate conversation creation to subsystem
libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs	New implementation containing conversation creation logic
libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs	Utility functions moved from Galley
libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs	New module for federation-related conversation logic
libs/wire-subsystems/src/Wire/ConversationSubsystem/Types.hs	Configuration types for conversation subsystem
services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs	Updated to support new conversation subsystem dependencies
libs/galley-types/src/Galley/Types/Error.hs	Error types moved to galley-types library
services/galley/src/Galley/Effects.hs	Reordered effect stack and added new error types
Multiple import updates	Updated imports across ~30 files to reflect module movements

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

services/brig/src/Brig/IO/Intra.hs

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

services/galley/src/Galley/API/Mapping.hs

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

battermann

Great effort! Thanks!. I feel there is more to do and to be said to get this really clean, so here are a few comments already.

charts/background-worker/templates/configmap.yaml

libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs

libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs

libs/wire-subsystems/src/Wire/ConversationSubsystem/Types.hs

battermann · 2026-01-29T07:50:21Z

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

+        . mapError (const ("Invalid input" :: Text) :: InvalidInput -> Text)
        . mapError @MigrationError (T.pack . show)
+        . mapError @InternalError (TL.toStrict . internalErrorDescription)
+        . mapError @UnreachableBackends (T.pack . show)
+        . mapError @NonFederatingBackends (T.pack . show)
+        . mapError @TeamCollaboratorsError (const ("Team collaborators error" :: Text))
+        . mapError @TeamFeatureStoreError (const ("Team feature store error" :: Text))
+        . mapError @(Tagged OperationDenied ()) (const ("Operation denied" :: Text))
+        . mapError @(Tagged 'NotATeamMember ()) (const ("Not a team member" :: Text))
+        . mapError @(Tagged 'ConvAccessDenied ()) (const ("Conversation access denied" :: Text))
+        . mapError @(Tagged 'NotConnected ()) (const ("Not connected" :: Text))
+        . mapError @(Tagged 'MLSNotEnabled ()) (const ("MLS not enabled" :: Text))
+        . mapError @(Tagged 'MLSNonEmptyMemberList ()) (const ("MLS non-empty member list" :: Text))
+        . mapError @(Tagged 'MissingLegalholdConsent ()) (const ("Missing legalhold consent" :: Text))
+        . mapError @(Tagged 'NonBindingTeam ()) (const ("Non-binding team" :: Text))
+        . mapError @(Tagged 'NoBindingTeamMembers ()) (const ("No binding team members" :: Text))
+        . mapError @(Tagged 'TeamNotFound ()) (const ("Team not found" :: Text))
+        . mapError @(Tagged 'InvalidOperation ()) (const ("Invalid operation" :: Text))
+        . mapError @(Tagged 'ConvNotFound ()) (const ("Conversation not found" :: Text))
+        . mapError @(Tagged 'ChannelsNotEnabled ()) (const ("Channels not enabled" :: Text))
+        . mapError @(Tagged 'NotAnMlsConversation ()) (const ("Not an MLS conversation" :: Text))


This seems verbose and like some duplication. Don't we have instances for these somewhere. Can we reuse them? Or can this be extracted into a singel mapping function?

in galley they are converted to http errors, should we somehow group them?

cosmetic detail, later!

battermann · 2026-01-29T08:00:22Z

services/galley/src/Galley/API/Create.hs

+  let remoteDomains = void <$> snd (partitionQualified lusr $ newConv.newConvQualifiedUsers)
+  enforceFederationProtocol (baseProtocolToProtocol newConv.newConvProtocol) remoteDomains
+  checkFederationStatus (RemoteDomains $ Set.fromList remoteDomains)


I wonder if checking the federation status should be implemented in a Federation subsystem, and if the conversation subsystem interpreter should be using it instead of having this here? Should that be part of this PR, too?

Does the new design answers your questions?

I like the FederationSubsystem, however here we still have code that integrates conversation and federation subsystems, my understanding is that we want to remove this responsibility from the services (galley in this case).

I strongly agree, but there are places we use it directly:

services/galley/src/Galley/API/Internal.hs:188:getFederationStatus

services/galley/src/Galley/API/Action.hs:585:enforceFederationProtocol

services/galley/src/Galley/API/Action.hs:607:checkFederationStatus

What should I do then?

i say leave it for later.

libs/wire-subsystems/src/Wire/ClientStore.hs

services/galley/src/Galley/API/Create.hs

Copilot

Pull request overview

Copilot reviewed 79 out of 80 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

services/background-worker/test/Test/Wire/Util.hs

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

integration/test/Test/FeatureFlags.hs

libs/wire-subsystems/src/Wire/ConversationSubsystem/Types.hs

libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs

libs/wire-subsystems/src/Wire/GalleyAPIAccess/Rpc.hs

services/galley/src/Galley/API/Internal.hs

libs/galley-types/src/Galley/Types/Teams.hs

libs/wire-subsystems/src/Wire/FederationSubsystem/Interpreter.hs

libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs

libs/wire-api/src/Wire/API/Conversation/Config.hs

battermann · 2026-02-04T14:38:13Z

services/galley/src/Galley/API/Create.hs

+  let remoteDomains = void <$> snd (partitionQualified lusr $ newConv.newConvQualifiedUsers)
+  enforceFederationProtocol (baseProtocolToProtocol newConv.newConvProtocol) remoteDomains
+  checkFederationStatus (RemoteDomains $ Set.fromList remoteDomains)


I like the FederationSubsystem, however here we still have code that integrates conversation and federation subsystems, my understanding is that we want to remove this responsibility from the services (galley in this case).

charts/background-worker/templates/configmap.yaml

charts/background-worker/values.yaml

supersven · 2026-02-06T12:57:13Z

@blackheaven some compilation error slipped in:

background-worker> src/Wire/BackgroundWorker/Jobs/Registry.hs:71:50: error: [GHC-61689]
background-worker>     Module ‘Wire.FeaturesConfigSubsystem.Interpreter’ does not export ‘TeamFeatureStoreError’.
background-worker>    |
background-worker> 71 | import Wire.FeaturesConfigSubsystem.Interpreter (TeamFeatureStoreError, runFeaturesConfigSubsystem)
background-worker>    |                                                  ^^^^^^^^^^^^^^^^^^^^^
background-worker>

(https://concourse.ops.zinfra.io/builds/118785733#L696f7b80:14799)

libs/wire-api/src/Wire/API/Conversation/Config.hs

services/galley/src/Galley/API/Public/Conversation.hs

libs/wire-subsystems/src/Wire/FederationSubsystem.hs

libs/wire-subsystems/src/Wire/GalleyAPIAccess.hs

services/background-worker/src/Wire/BackgroundWorker/Env.hs

Co-authored-by: Sven Tennie <sven.tennie@wire.com>

fisx

i'm not done, but i have read all the easy parts, and i'll keep reading for a little longer.

there are some types in wire-api that weren't there now, or that didn't have aeson instances. maybe add roundtrip tests, and possibly golden tests for those? especially if they are not fully implemented in schema-profunctor.

feel free to fix all my comments in separate PRs, or ignore them if they are not worth the trouble! :)

changelog.d/5-internal/WPD-21964-conv-subsystems

charts/background-worker/templates/configmap.yaml

libs/wire-api/src/Wire/API/Team/FeatureFlags.hs

libs/wire-subsystems/src/Wire/FederationSubsystem/Internals.hs

fisx · 2026-02-13T15:12:33Z

services/galley/src/Galley/API/Action.hs

      zusrMembership <- join <$> forM storedConv.metadata.cnvmTeam (TeamSubsystem.internalGetTeamMember (qUnqualified origUser))
      for_ zusrMembership $ \tm -> unless (tm `hasPermission` ModifyConvName) $ throwS @'InvalidOperation
-      cn <- rangeChecked (cupName action)
+      cn <- either (throw . InvalidRange . fromString) pure $ checkedEither (cupName action)


why? i liked rangeChecked...

services/galley/src/Galley/App.hs

libs/wire-subsystems/src/Wire/FederationSubsystem/Interpreter.hs

libs/wire-subsystems/test/unit/Wire/MockInterpreters/GalleyAPIAccess.hs

blackheaven · 2026-02-13T15:46:53Z

I'll have a look, thanks for the review!

fisx

still happy with everything, but i need to go. i'll continue reading later.

thoughts for next time: this PR was way too big (yes, i know, i'm no better :). i think some changes could have easily been factored out (federation subsystem?), and i'm tempted to that we need a rule to separate refactoring PRs from PRs that change semantics, and not mix both in one.

libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs

fisx · 2026-02-13T15:52:50Z

services/galley/src/Galley/API/Create.hs

+  let remoteDomains = void <$> snd (partitionQualified lusr $ newConv.newConvQualifiedUsers)
+  enforceFederationProtocol (baseProtocolToProtocol newConv.newConvProtocol) remoteDomains
+  checkFederationStatus (RemoteDomains $ Set.fromList remoteDomains)


i say leave it for later.

blackheaven · 2026-02-13T16:16:56Z

thoughts for next time: this PR was way too big (yes, i know, i'm no better :). i think some changes could have easily been factored out (federation subsystem?), and i'm tempted to that we need a rule to separate refactoring PRs from PRs that change semantics, and not mix both in one.

I know, I'm down the rabbit hole, it's an excerpt, of an excerpt, of an excerpt, of an excerpt, of a PR (not kidding).

fisx

I'm done reading and still happy :)

libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

fisx · 2026-02-15T16:36:56Z

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

+                makeReqFresh fpr url rb = makeVerifiedRequestFreshManagerIO env.logger fpr url rb
+             in LegalHoldEnv {makeVerifiedRequest = makeReq, makeVerifiedRequestFreshManager = makeReqFresh}
      runFinal @IO
+        . unsafelyPerformConcurrency @_ @'Unsafe


sorry, i just have to write this every time i read "unsafePerformConcurrency": we need to get rid of that! (but not now)

fisx · 2026-02-15T16:37:45Z

services/background-worker/src/Wire/BackgroundWorker/Jobs/Registry.hs

+        . mapError (const ("Invalid input" :: Text) :: InvalidInput -> Text)
        . mapError @MigrationError (T.pack . show)
+        . mapError @InternalError (TL.toStrict . internalErrorDescription)
+        . mapError @UnreachableBackends (T.pack . show)
+        . mapError @NonFederatingBackends (T.pack . show)
+        . mapError @TeamCollaboratorsError (const ("Team collaborators error" :: Text))
+        . mapError @TeamFeatureStoreError (const ("Team feature store error" :: Text))
+        . mapError @(Tagged OperationDenied ()) (const ("Operation denied" :: Text))
+        . mapError @(Tagged 'NotATeamMember ()) (const ("Not a team member" :: Text))
+        . mapError @(Tagged 'ConvAccessDenied ()) (const ("Conversation access denied" :: Text))
+        . mapError @(Tagged 'NotConnected ()) (const ("Not connected" :: Text))
+        . mapError @(Tagged 'MLSNotEnabled ()) (const ("MLS not enabled" :: Text))
+        . mapError @(Tagged 'MLSNonEmptyMemberList ()) (const ("MLS non-empty member list" :: Text))
+        . mapError @(Tagged 'MissingLegalholdConsent ()) (const ("Missing legalhold consent" :: Text))
+        . mapError @(Tagged 'NonBindingTeam ()) (const ("Non-binding team" :: Text))
+        . mapError @(Tagged 'NoBindingTeamMembers ()) (const ("No binding team members" :: Text))
+        . mapError @(Tagged 'TeamNotFound ()) (const ("Team not found" :: Text))
+        . mapError @(Tagged 'InvalidOperation ()) (const ("Invalid operation" :: Text))
+        . mapError @(Tagged 'ConvNotFound ()) (const ("Conversation not found" :: Text))
+        . mapError @(Tagged 'ChannelsNotEnabled ()) (const ("Channels not enabled" :: Text))
+        . mapError @(Tagged 'NotAnMlsConversation ()) (const ("Not an MLS conversation" :: Text))


cosmetic detail, later!

Co-authored-by: Matthias Fischmann <mf@zerobuzz.net>

blackheaven requested review from a team as code owners January 22, 2026 17:01

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 22, 2026

blackheaven force-pushed the gdifolco/WPB-21964-extract-create-conversations-wire-subsystems branch 5 times, most recently from 1e8e214 to e7b9141 Compare January 28, 2026 11:14

battermann requested a review from Copilot January 29, 2026 07:16

Copilot started reviewing on behalf of battermann January 29, 2026 07:17 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

battermann reviewed Jan 29, 2026

View reviewed changes

blackheaven requested a review from battermann January 29, 2026 16:45

blackheaven force-pushed the gdifolco/WPB-21964-extract-create-conversations-wire-subsystems branch 3 times, most recently from 7b92819 to c7b2a42 Compare February 3, 2026 21:05

battermann requested a review from Copilot February 4, 2026 13:46

Copilot started reviewing on behalf of battermann February 4, 2026 13:47 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

battermann reviewed Feb 4, 2026

View reviewed changes

blackheaven requested a review from battermann February 4, 2026 17:41

supersven reviewed Feb 5, 2026

View reviewed changes

charts/background-worker/templates/configmap.yaml Show resolved Hide resolved

supersven reviewed Feb 5, 2026

View reviewed changes

charts/background-worker/values.yaml Outdated Show resolved Hide resolved

supersven reviewed Feb 6, 2026

View reviewed changes

libs/wire-api/src/Wire/API/Conversation/Config.hs Outdated Show resolved Hide resolved

supersven reviewed Feb 6, 2026

View reviewed changes

services/galley/src/Galley/API/Public/Conversation.hs Outdated Show resolved Hide resolved

supersven reviewed Feb 6, 2026

View reviewed changes

libs/wire-subsystems/src/Wire/FederationSubsystem.hs Outdated Show resolved Hide resolved

supersven reviewed Feb 6, 2026

View reviewed changes

libs/wire-subsystems/src/Wire/GalleyAPIAccess.hs Outdated Show resolved Hide resolved

supersven reviewed Feb 6, 2026

View reviewed changes

services/background-worker/src/Wire/BackgroundWorker/Env.hs Outdated Show resolved Hide resolved

blackheaven and others added 10 commits February 9, 2026 16:54

fix: Leif feedbacks

3fb9f76

Update charts/background-worker/values.yaml

1ecc5dc

Co-authored-by: Sven Tennie <sven.tennie@wire.com>

Update charts/background-worker/templates/configmap.yaml

0548896

Co-authored-by: Sven Tennie <sven.tennie@wire.com>

fix: Sven feedbacls (incl. moving away from wrapped interface)

2d04d40

fix: ToSchema instance

4a9aef2

Hello CI

eb0b706

fix: add back dropped tests

9680f42

fix: rebased

026e044

fix: rebased

4c4190c

fix: rebased

5d9b4f7

blackheaven force-pushed the gdifolco/WPB-21964-extract-create-conversations-wire-subsystems branch from 4d46c23 to 5d9b4f7 Compare February 9, 2026 17:08

blackheaven added 7 commits February 9, 2026 22:11

fix: history regression

ad067df

fix: history regression

c6ff963

Hello CI

a2b2bb1

Hello CI

d8bb8c5

Hello CI

3f3f1a2

fix: try to batch notifications for gundeck regression

9571918

Hello CI

3446961

fisx reviewed Feb 13, 2026

View reviewed changes

fisx approved these changes Feb 15, 2026

View reviewed changes

fisx and others added 7 commits February 15, 2026 17:47

nit-pick

6bc2740

fix: Matthias feedbacks (part1)

bbaf8c9

fix: Matthias feedbacks (part2)

b054911

fix: Matthias feedbacks (part3)

772d630

Update libs/wire-subsystems/src/Wire/ConversationSubsystem/Util.hs

550495a

Co-authored-by: Matthias Fischmann <mf@zerobuzz.net>

fix: Matthias feedbacks (part4)

cc79b55

fix: Matthias feedbacks (part5)

4e66864

Conversation

blackheaven commented Jan 22, 2026 • edited by fisx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

battermann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

supersven commented Feb 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

blackheaven commented Jan 22, 2026 •

edited by fisx

Loading

battermann left a comment •

edited

Loading