Test commit proposals

[tylerhawkes]

Add mls_sync::with_rollback and refactor result-returning commit path to test commit proposals

Introduce mls_sync::with_rollback to run operations in a rolled-back transaction and reload the group, and refactor the commit path to use it while returning (R, Option<Vec<u8>>, u64); add commit proposal size tests in test_commit_proposals.rs.

📍Where to Start

Start with with_rollback in mls_sync.rs, then review its use in the refactored result-returning function and the tests in test_commit_proposals.rs.

---

📊 Macroscope summarized 70a3eff. 1 file reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review - January 22, 2026 at 22:58 UTC

• Read and analyze changed files
• Review code quality and implementation
• Check for potential bugs or issues
• Evaluate test coverage
• Post review feedback

---

Summary

This PR refactors the generate_commit_with_rollback function by extracting a more general with_rollback helper, and adds a comprehensive test suite for validating proposal+commit workflows. The changes are well-structured with good test coverage.

Code Quality & Best Practices

mls_sync.rs:3203-3245 ✅ Good refactoring

• The extraction of with_rollback as a separate function improves code reusability
• The signature cleanly separates concerns: with_rollback handles the transaction mechanics, while generate_commit_with_rollback adds commit-specific logic on top
• Making with_rollback pub(super) is appropriate for test access

Potential Issues

mls_sync.rs:3203 ⚠️ Visibility consideration
The with_rollback function is now pub(super), which exposes it to the parent module (groups). Consider whether this should remain pub(crate) or stay pub(super) based on intended usage. Given it's used in tests, pub(super) seems appropriate, but worth confirming this is the intended scope.

test_commit_proposals.rs:58-60 ⚠️ Unsafe transmute

unsafe { std::mem::transmute(tree) }

The comment acknowledges this isn't safe. While this is test code, consider:

• Adding a more detailed comment explaining why this is needed and what the safety requirements are
• Filing a tracking issue to remove this once dependencies expose the needed types properly
• This could break silently if the internal representation changes

test_commit_proposals.rs:476-477 ℹ️ Test is ignored

#[xmtp_common::test]
#[ignore]
async fn test_commit_sizes_with_proposals()

The main test is marked with #[ignore]. Consider:

• Adding a doc comment explaining why it's ignored (performance test? manual run only?)
• If this is meant for manual performance testing, consider moving to a benches/ directory or marking it clearly as a benchmark

Performance Considerations

test_commit_proposals.rs:29-33 📊 Test parameters
The constants define test scale (5 iterations, up to 3 testers, up to 9 installations per tester). These seem reasonable for an ignored/manual test, but ensure CI doesn't accidentally run this as it could be slow.

test_commit_proposals.rs:571-591 📊 CSV output
Good use of structured logging and CSV output for performance analysis. This makes it easy to collect and analyze commit size data.

Security Concerns

No security issues identified. The test code appropriately uses test contexts and doesn't expose any production security concerns.

Test Coverage

✅ Excellent test coverage - The new test file is comprehensive:

• Tests proposal generation, commit creation, and group synchronization
• Validates welcome message handling and tree consistency
• Tests member addition/removal workflows
• Includes message delivery validation
• Tests leaf node updates and tree filling

The test demonstrates real-world usage patterns and stress-tests the proposal+commit workflow with multiple participants and installations.

Minor Suggestions

1. test_commit_proposals.rs:230, 364-367 - Several commented-out sync() calls. Consider cleaning these up or adding comments explaining why they're preserved.

2. test_commit_proposals.rs:516-521 - The remove logic comment says "remove one every 3rd iteration" but the code checks if testers.len() > 4. Consider aligning the comment with the actual logic.

---

[tylerhawkes]

• Test commit proposals #3051 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[macroscopeapp]

Serialization errors are silently swallowed via .ok().flatten(), making failures indistinguishable from having no pending commit. Consider propagating the error instead.

🚀 Want me to fix this? Reply ex: "fix it for me".

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.06%. Comparing base (7c06b2c) to head (70a3eff).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3051      +/-   ##
==========================================
- Coverage   74.20%   74.06%   -0.14%     
==========================================
  Files         445      445              
  Lines       54346    54359      +13     
==========================================
- Hits        40326    40263      -63     
- Misses      14020    14096      +76     

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