xmtp_mls: route Sender::NewMemberCommit through from_external_commit validator

[tylerhawkes]

Summary

Adds the dispatch arm in mls_sync.rs that routes incoming commits with Sender::NewMemberCommit to L-7's ValidatedCommit::from_external_commit validator (the QR-invite flow). The existing member-commit path remains intact for Sender::Member(idx).

Dispatch flow:

1. Match on processed_message.sender().
2. Sender::Member(idx) — existing from_staged_commit path.
3. Sender::NewMemberCommit — source external_commit_allowed via L-6's is_external_commit_allowed(mls_group) helper. Source immutable_metadata + mutable_metadata via the capability-aware path (registry-aware for migrated groups). Call from_external_commit(..., external_commit_allowed). Skip committer-leaf-index extraction (joiner not in tree yet from existing members' POV). Converge to merge_staged_commit_logged.
4. Other (NewMemberProposal, External) — explicit rejection with CommitValidationError::ActorNotMember, traced.

Post-merge new-installation registration happens through the existing handler — the joiner's other installations (from the atomic external commit's by-value Add proposals) get registered through the standard pipeline.

Depends on #3667 (L-7 validator).

Test plan

• cargo check -p xmtp_mls — clean.
• End-to-end integration coverage in T-1 (separate PR) exercises the dispatch arm against real external commits including the master-switch-off rejection path.

🤖 Generated with Claude Code

Note

Route Sender::NewMemberCommit through external commit validation in MLS message processor

• Adds a new branch in the StagedCommitMessage handler in mls_sync.rs for Sender::NewMemberCommit that validates atomic external/join commits using ValidatedCommit::from_external_commit, with capability-aware policy and metadata resolution.
• Expands ProtocolMessage type acceptance to include PublicMessage frames (e.g., external commits) in addition to PrivateMessage; other frame types return UnsupportedMessageType.
• Extends extract_message_sender to handle Sender::NewMemberCommit by deriving inbox id and installation id from the staged commit's update-path leaf credential.
• Rejects unsupported sender types with CommitValidationError::ActorNotMember and advances the processing cursor before returning non-retryable errors on the new path.

^{Macroscope summarized be09e92.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review — 2026-05-27 18:35 UTC

Summary

This PR successfully implements the dispatch logic for routing Sender::NewMemberCommit through the external commit validator. The code is well-structured and follows existing patterns. I've identified two minor issues beyond what Macroscope already flagged.

---

Issues Found

1. Unused variable _group_permissions (lines 1126-1144)

The _group_permissions variable is computed but never used. While this appears intentional (perhaps for future validation or as defensive code), the leading underscore suggests it's acknowledged. However, the computation involves fallible operations that could fail, making this dead code path in case of errors.

Suggestion: If this is genuinely unused in the current implementation, consider adding a comment explaining why it's sourced but not yet consumed, or removing it entirely until needed.

Location: crates/xmtp_mls/src/groups/mls_sync.rs:1126-1144

---

2. Inconsistent error handling pattern between dispatch arms

The Sender::NewMemberCommit arm has extensive inline error handling with multiple self.maybe_update_cursor() + early returns (lines 1132-1195), while the Sender::Member arm delegates all error handling to the outer match (lines 1109-1115). This creates an inconsistency.

Both the migrated and non-migrated paths in the NewMemberCommit arm explicitly call maybe_update_cursor and return early on metadata extraction failures. However, the outer error handler at lines 1224-1234 also calls maybe_update_cursor for non-retryable errors.

Concern: This could result in double cursor updates if metadata extraction succeeds but from_external_commit returns a non-retryable error. The cursor would be updated once in the catch block (e.g., line 1132), then potentially again at line 1228.

Suggestion: Consider refactoring the NewMemberCommit arm to match the pattern used by Sender::Member — extract metadata into a helper function that returns Result<_, CommitValidationError> and let the outer error handler manage cursor updates consistently.

Location: crates/xmtp_mls/src/groups/mls_sync.rs:1117-1221

---

3. Missing test coverage (acknowledged in PR description)

The PR description notes that end-to-end integration tests are deferred to T-1. Codecov reports 10% patch coverage with 9 lines missing coverage in this file. The critical dispatch logic at lines 1117-1221 appears to be entirely untested in this PR.

Recommendation: Before merging the T-1 integration PR, ensure coverage includes:

• External commit acceptance when allow_external_commit is true
• External commit rejection when allow_external_commit is false
• Handling of malformed metadata in the NewMemberCommit path
• The other sender type rejection path (lines 1210-1220)

---

Positive Observations

• ✅ Clear dispatch logic with well-documented flow in comments (lines 1099-1105)
• ✅ Proper fail-closed behavior via is_external_commit_allowed (lines 1198-1201)
• ✅ Explicit rejection of unsupported sender types with trace logging (lines 1210-1220)
• ✅ Symmetric handling between both commit paths converging at merge_staged_commit_logged
• ✅ Comprehensive inline documentation explaining the capability-aware metadata sourcing

---

Tasks

• Read modified file (mls_sync.rs)
• Read validator context (validated_commit.rs)
• Analyze dispatch logic, error handling, and code quality
• Post review feedback

---

[macroscopeapp]

🟠 High groups/validated_commit.rs:2692

enforce_app_data_update_scope permits TlsMapMutation::Delete mutations when they target the joiner's own inbox id, but a delete-only delta leaves the joiner in the MLS group with no GROUP_MEMBERSHIP entry — violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (ResyncExternalCommitNotSupported), the joiner has no pre-existing entry to delete or update; only Insert should be allowed.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/validated_commit.rs around line 2692:

`enforce_app_data_update_scope` permits `TlsMapMutation::Delete` mutations when they target the joiner's own inbox id, but a delete-only delta leaves the joiner in the MLS group with no `GROUP_MEMBERSHIP` entry — violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (`ResyncExternalCommitNotSupported`), the joiner has no pre-existing entry to delete or update; only `Insert` should be allowed.

Evidence trail:
crates/xmtp_mls/src/groups/validated_commit.rs lines 2692-2701 (REVIEWED_COMMIT): Loop permits Delete/Update mutations as long as key matches joiner_id.
crates/xmtp_mls/src/groups/validated_commit.rs lines 948-1032 (REVIEWED_COMMIT): `from_external_commit` has no downstream check that delta contains an Insert.
crates/xmtp_mls/src/groups/validated_commit.rs line 2582 (REVIEWED_COMMIT): ResyncExternalCommitNotSupported confirms joiner has no pre-existing entry.
crates/xmtp_mls/src/groups/validated_commit.rs lines 2641-2642 (REVIEWED_COMMIT): Docstring states intent is only Insert.
crates/xmtp_mls_common/src/tls_map.rs line 386 (REVIEWED_COMMIT): TlsMapMutation enum with Insert/Update/Delete variants.

[macroscopeapp]

🟠 High groups/mod.rs:2015

set_external_commit_policy allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file crates/xmtp_mls/src/groups/mod.rs around line 2015:

`set_external_commit_policy` allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: `if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }`

Evidence trail:
crates/xmtp_mls/src/groups/mod.rs lines 2015-2046 (set_external_commit_policy missing DM check), lines 1987-2004 (update_commit_log_signer with DM check at 1993-1995), lines 2051-2060 (set_allow_external_commit wrapper). git_grep for DmGroupMetadataForbidden shows the check at lines 1901, 1927, 1994, 2086, 2144, 2184, 2482 — all other metadata update paths.

[macroscopeapp]

Approvability

Verdict: Needs human review

2 blocking correctness issues found. This PR enables a new external commit/QR-invite join code path with significant runtime behavior changes. Two unresolved high-severity findings identify potential security issues: improper mutation handling in external commit validation and missing DM policy checks. These warrant human review.

^{You can customize Macroscope's approvability policy. Learn more.}

[codecov]

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (tyler/qr-invite-l7-validator@6d0ec42). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/groups/mls_sync.rs	10.00%	9 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                       @@
##             tyler/qr-invite-l7-validator    #3668   +/-   ##
===============================================================
  Coverage                                ?   83.91%           
===============================================================
  Files                                   ?      401           
  Lines                                   ?    59586           
  Branches                                ?        0           
===============================================================
  Hits                                    ?    50002           
  Misses                                  ?     9584           
  Partials                                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.