Skip to content

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Sep 24, 2025

  • Return informative diagnostics on group info mismatch.
  • Add field to mls feature for enabling group info diagnostics per team.

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

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 24, 2025
@pcapriotti pcapriotti force-pushed the mls-group-info-check-flag branch 3 times, most recently from a45c77a to 57a48ba Compare October 1, 2025 09:07
@pcapriotti pcapriotti marked this pull request as ready for review October 1, 2025 09:30
@pcapriotti pcapriotti requested review from a team as code owners October 1, 2025 09:30
@pcapriotti pcapriotti requested a review from Copilot October 1, 2025 09:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the generic MLSGroupInfoMismatch error with detailed GroupInfoDiagnostics to provide more informative error responses when MLS group info validation fails. It also adds a team-level feature flag to control when group info diagnostics are enabled.

  • Replaced MLSGroupInfoMismatch error with GroupInfoDiagnostics containing detailed information (commit, group info, clients, etc.)
  • Added mlsGroupInfoDiagnostics field to the MLS feature configuration for per-team control
  • Updated error handling across MLS message processing and federation APIs

Reviewed Changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
services/galley/src/Galley/API/MLS/GroupInfoCheck.hs New module implementing group info validation with detailed error reporting
services/galley/src/Galley/API/MLS/Message.hs Updated MLS message processing to use new group info diagnostics
libs/wire-api/src/Wire/API/Error/Galley.hs Added GroupInfoDiagnostics error type and removed MLSGroupInfoMismatch
libs/wire-api/src/Wire/API/Team/Feature.hs Added mlsGroupInfoDiagnostics field to MLS feature configuration
libs/wire-api/src/Wire/API/Team/Member/Error.hs New module for team member permission error handling
integration/test/Test/MLS.hs Updated test to use new diagnostics response format
Multiple test files Updated to include groupInfoDiagnostics field in MLS configurations
Comments suppressed due to low confidence (2)

services/galley/src/Galley/API/MLS/Message.hs:1

  • This import is removed but view is still used on line 85 in the new GroupInfoCheck module. Verify that the import has been moved to the appropriate location.
-- This file is part of the Wire Server implementation.

integration/test/Testlib/Assertions.hs:1

  • The function signature change from shouldMatchBase64 a b to shouldMatchBase64 a bytestring is a breaking change. The parameter b is now expected to be raw bytes instead of a MakesValue, which changes the API contract.
{-# LANGUAGE CPP #-}

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 15 to 24
.= object
["mlsConversationReset" .= True]
]
defSetting =
object
[ "status" .= "enabled",
"config" .= object ["mlsConversationReset" .= False]
"config"
.= object
[ "mlsConversationReset" .= False
]
Copy link

Copilot AI Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The formatting change breaks the object construction across multiple lines unnecessarily. Keep the original compact format for consistency with similar patterns in the codebase.

Copilot uses AI. Check for mistakes.

@pcapriotti pcapriotti force-pushed the mls-group-info-check-flag branch from 00c68cb to 6c3911f Compare October 2, 2025 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants