xmtp_mls: integration test for QR-invite end-to-end flow

[tylerhawkes]

Summary

End-to-end integration test for the QR-invite atomic external commit flow.

crates/xmtp_mls/src/groups/tests/test_external_invite_e2e.rs exercises:

1. Happy path, single installation: admin creates invite, scanner joins via external commit, members converge.
2. Multi-installation joiner: joiner with 3 installations — primary joins via external commit, secondaries via Welcome.
3. Master switch off: external commit is rejected when set_allow_external_commit(false).
4. Stale GroupInfo: invite produced at epoch N, group advances to N+1, joiner retry fails until refreshed blob is fetched.
5. Admin rotation: admin issues a fresh invite (new symmetric key); old-key holders can no longer decrypt the new blob.
6. Tampered blob: blob with mutated ciphertext fails AEAD.

Uses a minimal in-test "service" stub (HashMap keyed by group_id_hash) to simulate the application's transport layer.

Stack

This PR is a merge of two branches:

• tyler/qr-invite-b2-napi-join (bindings/node: add Client.joinGroupByExternalInvite NAPI binding #3676) — pulls in L-9/L-10/L-11.
• tyler/qr-invite-l8-ingestion (xmtp_mls: route Sender::NewMemberCommit through from_external_commit + envelope-timestamp plumbing #3668) — pulls in L-6/L-7/L-8 (validator + ingestion routing).

Both branches reach back through L-4 / L-1 / L-0.

Test plan

• cargo check -p xmtp_mls --tests passes.
• CI exercises the 6 test cases above.

Note

Add integration tests for QR-invite end-to-end flow in xmtp_mls

Adds a new test file test_external_invite_e2e.rs covering the full QR-invite join flow via external commits.

• InviteBlobStore provides an in-memory stub for the invite-blob service, keyed by external_group_id from ExternalInvitePayload
• qr_invite_happy_path_single_installation tests the single-installation join path including sync and post-join message decryption
• qr_invite_happy_path_multi_installation tests atomic join of two installations via external commit with HPKE-wrapped Welcome processing
• Negative tests cover policy-rejected commits (qr_invite_rejected_when_policy_disallows) and stale-epoch rejection (qr_invite_rejected_stale_epoch)
• Additional tests verify chained blob refresh (qr_invite_chain_self_refresh) and inbox-scoped membership updates (qr_invite_membership_update_is_inbox_scoped)

^{Macroscope summarized 9874f1f.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

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

• Read changed files
• Analyze test implementation
• Provide review feedback

---

Overall Assessment

This is a well-structured test file with comprehensive coverage of the QR-invite flow. The tests are clear, well-documented, and follow the repository's testing conventions. Below are specific observations:

Strengths

1. Excellent documentation: The file header (lines 1-56) provides exceptional context about the feature being tested and documents a known upstream issue with clear reproduction details and fix guidance
2. Comprehensive coverage: Tests cover happy paths (single/multi-installation), negative cases (policy rejection, stale epoch), and edge cases (chained refresh, inbox-scoped updates)
3. Clear test structure: Each test follows a consistent pattern: setup → action → verification, making them easy to follow
4. Good naming: Test names clearly describe what they're testing (qr_invite_happy_path_single_installation, etc.)
5. Helper abstraction: InviteBlobStore provides a clean in-memory stub for the invite-blob service

Issues & Suggestions

1. Inconsistent error handling in negative tests (lines 348-417, 423-484)

In qr_invite_rejected_when_policy_disallows and qr_invite_rejected_stale_epoch, the tests use:

let _ = bo.join_group_by_external_invite(...).await;
let _ = group.sync().await;
let _ = charlie_group.sync().await;

Issue: Discarding Result values with let _ silences potential issues and makes debugging harder. The comment on line 400-403 acknowledges this but doesn't explain why both success and failure are acceptable outcomes.

Suggestion: Either:

• Assert that the join succeeds locally but is rejected on sync: assert_ok!(bo.join_group_by_external_invite(...).await); assert!(group.sync().await.is_ok() || group.sync().await.is_err());
• Or use match to explicitly document both valid outcomes with comments explaining each case

2. Hardcoded sleep duration (line 185)

xmtp_common::time::sleep(std::time::Duration::from_millis(500)).await;

Issue: 500ms is arbitrary and may cause flakiness on slower CI systems or create unnecessarily slow tests on fast systems.

Suggestions:

• Use exponential backoff with retries instead of a fixed sleep
• Or make the duration configurable via environment variable
• Or add a comment explaining why 500ms is the minimum safe wait time

3. Redundant test setup duplication

All happy-path tests (lines 126-231, 238-342, 491-572, 579-700) repeat the same setup pattern:

group.sync().await?;
group.enable_proposals(EnableProposalsOptions::test_default()).await?;
group.set_allow_external_commit(true).await?;
group.sync().await?;

Suggestion: Extract this to a helper function like:

async fn setup_group_for_qr_invite(group: &TestMlsGroup) -> Result<(), GroupError> {
    group.sync().await?;
    group.enable_proposals(EnableProposalsOptions::test_default()).await?;
    group.set_allow_external_commit(true).await?;
    group.sync().await
}

This would reduce duplication and make it easier to update the setup process if requirements change.

4. InviteBlobStore::slot_id panics on malformed input (lines 90-97)

fn slot_id(payload_bytes: &[u8]) -> Vec<u8> {
    let payload = ExternalInvitePayloadProto::decode(payload_bytes)
        .expect("invite payload decodes");
    match payload.version {
        Some(PayloadVersion::V1(v1)) => v1.external_group_id,
        None => panic!("invite payload has no version variant"),
    }
}

Issue: While this is test code, panics make debugging harder. If a test accidentally passes malformed data, the panic message doesn't provide context about which test or operation failed.

Suggestion: Return Result and use ? in callers, or at minimum include the payload bytes in the panic message for debugging.

5. Missing test case: tampered blob verification

The PR description mentions testing "Tampered blob: blob with mutated ciphertext fails AEAD" but I don't see this test case in the file.

Verification needed: Was this test case implemented in a different commit/file, or should it be added?

Minor Observations

• Line 582: _bo_2 prefix indicates unused, which is correct for this test scenario
• Line 348-417: The comment on lines 380-383 is helpful but could be more concise
• Lines 628-640: The pre-join assertions could be extracted to a helper to reduce repetition with lines 661-699

Performance Considerations

No significant performance concerns. The sleep on line 185 is the only potential issue (addressed above).

Security Concerns

None. The tests appropriately verify that:

• Policy violations are rejected
• Stale epoch commits are rejected
• AppData updates are properly scoped to the joiner's inbox

---

Review Date: 2026-05-27 18:34 UTC

[macroscopeapp]

🟠 High groups/validated_commit.rs:2692

enforce_app_data_update_scope accepts TlsMapMutation::Delete when the key matches joiner_inbox_id, but this allows a joiner to submit a delta that only deletes their own entry. The joiner then ends up in the MLS group without a GROUP_MEMBERSHIP entry, violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (per ResyncExternalCommitNotSupported), the joiner has no legitimate pre-existing entry to update or delete — the check should reject Delete and Update mutations.

     for mutation in &delta.mutations {
         let key = match mutation {
-            TlsMapMutation::Insert { key, .. }
-            | TlsMapMutation::Update { key, .. }
-            | TlsMapMutation::Delete { key } => key,
+            TlsMapMutation::Insert { key, .. } => key,
+            TlsMapMutation::Update { .. } | TlsMapMutation::Delete { .. } => {
+                return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
+            }
         };
         if key != &joiner_id {
             return Err(CommitValidationError::ExternalCommitAppDataUpdateOutOfScope);
         }
     }

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

In file crates/xmtp_mls/src/groups/validated_commit.rs around lines 2692-2701:

`enforce_app_data_update_scope` accepts `TlsMapMutation::Delete` when the key matches `joiner_inbox_id`, but this allows a joiner to submit a delta that only deletes their own entry. The joiner then ends up in the MLS group without a `GROUP_MEMBERSHIP` entry, violating the invariant that external joins must register the joiner. Since resync external commits are unsupported (per `ResyncExternalCommitNotSupported`), the joiner has no legitimate pre-existing entry to update or delete — the check should reject `Delete` and `Update` mutations.

Evidence trail:
crates/xmtp_mls/src/groups/validated_commit.rs lines 2640-2703 (function `enforce_app_data_update_scope`): line 2641 documents intent as Insert-only, but lines 2692-2701 accept all mutation types; line 2582 shows `ResyncExternalCommitNotSupported`; lines 2743-2748 (`well_formed_membership_update_for`) shows only Insert tested; crates/xmtp_mls_common/src/tls_map.rs lines 486-515 (`apply_delta`) shows Delete on non-existent key returns error but [Insert, Delete] compound succeeds.

[macroscopeapp]

Approvability

Verdict: Approved

This PR adds integration tests for the QR-invite flow with no production code changes. The author owns all modified test files, and the unresolved review comments reference files outside this PR's scope.

No code changes detected at 9874f1f. Prior analysis still applies.

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

[macroscopeapp]

🟡 Medium groups/mod.rs:2016

set_external_commit_policy and set_allow_external_commit do not check if the conversation is a DM before modifying policy, while all other metadata update methods (update_group_name, update_app_data, etc.) return MetadataPermissionsError::DmGroupMetadataForbidden for DMs. Enabling external commits on a DM is semantically meaningless since DMs are fixed 2-person conversations that don't support QR-invite member addition. This inconsistency could lead to confusing downstream errors or undefined behavior.

Consider adding the same DM check used by other metadata update methods.

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

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

`set_external_commit_policy` and `set_allow_external_commit` do not check if the conversation is a DM before modifying policy, while all other metadata update methods (`update_group_name`, `update_app_data`, etc.) return `MetadataPermissionsError::DmGroupMetadataForbidden` for DMs. Enabling external commits on a DM is semantically meaningless since DMs are fixed 2-person conversations that don't support QR-invite member addition. This inconsistency could lead to confusing downstream errors or undefined behavior.

Consider adding the same DM check used by other metadata update methods.

Evidence trail:
crates/xmtp_mls/src/groups/mod.rs lines 2016-2047 (set_external_commit_policy, no DM check), lines 2052-2076 (set_allow_external_commit, delegates to set_external_commit_policy). Compare with DM checks at: line 1901 (update_group_name), line 1927 (update_app_data), line 1994 (update_commit_log_signer), line 2101 (update_permission_policy), line 2159 (update_group_description), line 2199 (update_group_image_url_square). All found via git_grep for 'DmGroupMetadataForbidden' in crates/xmtp_mls/src/groups/mod.rs at REVIEWED_COMMIT.

[macroscopeapp]

🟠 High groups/mls_sync.rs:1217

When an unsupported sender type (e.g., Sender::External, Sender::NewMemberProposal, Sender::Preconfigured) is received, the function returns CommitValidationError::ActorNotMember without calling self.maybe_update_cursor(). This is inconsistent with every other error path in this function, which updates the cursor before returning. Without the cursor update, the same invalid message will be re-processed on every sync, causing an infinite retry loop.

-                        return Err(GroupMessageProcessingError::CommitValidation(
-                            CommitValidationError::ActorNotMember,
-                        ));
+                        self.maybe_update_cursor(&self.context.db(), envelope)?;
+                        return Err(GroupMessageProcessingError::CommitValidation(
+                            CommitValidationError::ActorNotMember,
+                        ));

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

In file crates/xmtp_mls/src/groups/mls_sync.rs around lines 1217-1219:

When an unsupported sender type (e.g., `Sender::External`, `Sender::NewMemberProposal`, `Sender::Preconfigured`) is received, the function returns `CommitValidationError::ActorNotMember` without calling `self.maybe_update_cursor()`. This is inconsistent with every other error path in this function, which updates the cursor before returning. Without the cursor update, the same invalid message will be re-processed on every sync, causing an infinite retry loop.

Evidence trail:
crates/xmtp_mls/src/groups/mls_sync.rs lines 1210-1220 (the `other` branch returning without cursor update), lines 1132-1195 (other error paths calling maybe_update_cursor before return), lines 1223-1235 (post-result cursor update handler bypassed by direct return), lines 2499-2517 (maybe_update_cursor implementation), crates/xmtp_mls/src/groups/validated_commit.rs lines 213-220 (is_retryable returns false for ActorNotMember)

Dismissing prior approval to re-evaluate a65eb44