Skip group info mismatch error for broken groups#4883
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements logic to skip group info mismatch errors for MLS conversations that were already in a broken state before the group info diagnostics feature was enabled. The implementation allows systems to gracefully handle pre-existing broken groups without blocking future commits.
Key Changes
- Refactored group state validation to detect and tolerate pre-existing mismatches
- Added logic to check if a stored group already has a state mismatch before rejecting new commits
- Added comprehensive test coverage for the new behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
services/galley/src/Galley/API/MLS/Message.hs |
Updated checkGroupState calls to pass ConvOrSubConv instead of Maybe TeamId |
services/galley/src/Galley/API/MLS/GroupInfoCheck.hs |
Refactored validation logic to extract pure groupStateMismatch function and added existingGroupStateMismatch to check for pre-existing issues |
libs/wire-subsystems/src/Wire/ConversationStore.hs |
Added helper function getConvOrSubGroupInfo to retrieve group info for conversations or subconversations |
integration/test/Test/MLS.hs |
Added test case testGroupInfoAlreadyBroken to verify commits are allowed when group was already broken |
changelog.d/2-features/mls-skip-error-for-broken-groups |
Added changelog entry documenting the feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void $ sendAndConsumeCommitBundle mp2 {groupInfo = mp1.groupInfo} | ||
|
|
||
| -- enable feature | ||
| do |
There was a problem hiding this comment.
Is the purpose of the 'do' just for visual structuring?
There was a problem hiding this comment.
Usually I add a nested block when there are some bound variables that I don't want to leak into the parent scope, but here in fact it doesn't seem necessary. I don't mind it in any case, but I can remove it if you want.
There was a problem hiding this comment.
Yes, makes sense.
Personally, instead of a comment and a do block I would extract it as a function named enableFeatures. But you don't have to change it, it's fine. This is a super nitpick :)
Commits with a broken group info are now let through if the group was already broken.
https://wearezeta.atlassian.net/browse/WPB-21363
Checklist
changelog.d