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?

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

Copilot · 2026-02-04T13:52:53Z

integration/test/Test/FeatureFlags.hs

+testInternalGetConfiguredFeatureFlags :: (HasCallStack) => App ()
+testInternalGetConfiguredFeatureFlags = do
+  response <- Internal.getConfiguredFeatureFlags
+  response.status `shouldMatchInt` 200


Missing test implementation: The test case testInternalGetConfiguredFeatureFlags is added but there's no verification of the response content beyond the status code. Consider adding assertions to verify the structure and content of the returned feature flags to ensure the endpoint returns valid data.

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: Copilot <175728472+Copilot@users.noreply.github.com>

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! :)

fisx · 2026-02-13T13:33:57Z

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

shouldn't you say something about moving conversation stuff to wire-subsystems here?

fisx · 2026-02-13T13:35:12Z

charts/background-worker/templates/configmap.yaml

    gundeck:
      host: gundeck
      port: 8080
+    spar:


should this also be mentioned in the changelog?

fisx · 2026-02-13T14:49:32Z

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

+class FeatureFlagsToPair f cfgs where
+  featureFlagsToPair :: NP f cfgs -> [A.Pair]


Suggested change

class FeatureFlagsToPair f cfgs where

featureFlagsToPair :: NP f cfgs -> [A.Pair]

class FeatureFlagsToPairs f cfgs where

featureFlagsToPairs :: NP f cfgs -> [A.Pair]

fisx · 2026-02-13T15:06:02Z

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

+firstConflictOrFullyConnected :: [Remote NonConnectedBackends] -> FederationStatus
+firstConflictOrFullyConnected =


Suggested change

firstConflictOrFullyConnected :: [Remote NonConnectedBackends] -> FederationStatus

firstConflictOrFullyConnected =

firstMissingConnectionOrFullyConnected :: [Remote NonConnectedBackends] -> FederationStatus

firstMissingConnectionOrFullyConnected =

this would make the explanation in the comment redundant.

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...

fisx · 2026-02-13T15:18:57Z

services/galley/src/Galley/App.hs

        . runInputConst (e ^. cstate)
        . mapError toResponse
        . mapError toResponse
+        . mapError toResponse


maybe this is a good reason to invent callNTimesConscutively? :)

These are various types, it can be a fun challenge to make a piece of code for this.

fisx · 2026-02-13T15:24:27Z

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

+      NotConnectedDomains dom1 dom2 -> throw (NonFederatingBackends dom1 dom2)
+  GetFederationStatus req -> getFederationStatus' req
+  where
+    getFederationStatus' ::


the pattern in many other places in wire-subsystems is to call it getFederationStatusImpl and declare it at the top level of the module.

fisx · 2026-02-13T15:29:24Z

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

  GetFeatureConfigForTeam tid -> pure $ getFeatureConfigForTeamImpl configs tid
+  GetConfiguredFeatureFlags ->
+    pure $
+      FeatureLegalHoldDisabledByDefault


isn't this the Default instance from Wire.API.Team.Feature? if not: why? could one be defined there, or just a defaultThing, without the type class complicating things?

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.

fisx · 2026-02-13T15:43:53Z

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

+
+----------------------------------------------------------------------------
+-- Notifications
+notifyConversationUpdated ::


Suggested change

notifyConversationUpdated ::

notifyConversationUpdated ::

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).

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 18 commits February 9, 2026 16:54

fix: rebase

9d7cfa9

fix: background-worker.integration.yaml

191c917

Hello CI

bff3f0a

refactor: expose all default features in galley

5219c95

Hello CI

3f905d8

fix: hard coded config in background-worker Registry

121a8f3

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

ebb5b86

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: copilot feedbacks

b70679a

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

		class FeatureFlagsToPair f cfgs where
		featureFlagsToPair :: NP f cfgs -> [A.Pair]

		firstConflictOrFullyConnected :: [Remote NonConnectedBackends] -> FederationStatus
		firstConflictOrFullyConnected =

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!

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!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

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!

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

blackheaven commented Jan 22, 2026 •

edited by fisx

Loading

battermann left a comment •

edited

Loading