Add contacts API

[rygine]

Add a contacts API with database tables, FTS triggers, and device sync import/export across xmtp_mls and xmtp_db

Introduce contacts CRUD in xmtp_mls, add ORM models and migrations for contact data with FTS5 and JSON view in xmtp_db, and integrate contacts into backup/export/import and FFI/device sync.

📍Where to Start

Start with the contacts high-level API in xmtp_mls/src/contacts/mod.rs, then follow database interactions in xmtp_db/src/encrypted_store/contacts/mod.rs and the migration in xmtp_db/migrations/2025-12-28-000000_create_contacts/up.sql.

Changes since #2817 opened

• Modified contact and companion table CRUD operations in contacts::DbConnection<C> QueryContacts implementation to return inserted rows directly using get_result, check rows_affected and return diesel::result::Error::NotFound for updates and deletes when no rows match, and improved get_contacts query logic to ignore empty search strings, properly build FTS search patterns, and clamp limit/offset to non-negative i64 bounds [324e0b6]
• Updated contacts::FullContact From<RawFullContact> implementation to deserialize companion field JSON with error handling that logs warnings including inbox_id context and defaults to empty vectors on parse failure [324e0b6]
• Moved contact-related methods from client::Client implementation to new dedicated contacts::client module [324e0b6]
• Enhanced contact tests with increased timing tolerance and expanded cascade delete verification using raw SQL count queries [324e0b6]

---

📊 Macroscope summarized 324e0b6. 20 files reviewed, 25 issues evaluated, 24 issues filtered, 1 comment posted

🗂️ Filtered Issues

xmtp_archive/src/export_stream/contact_save.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 14: The backup_records function never updates state.cursor, causing an infinite loop when called repeatedly. Each call loads the same cursor value (line 14), fetches the same batch of records, but never increments the cursor. The caller likely expects the cursor to advance after each batch. [ Already posted ]

xmtp_db/src/encrypted_store/contacts/addresses.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 71: The From<AddressData> for NewContactAddress sets contact_id: 0 with a comment saying it will be set later. If the caller forgets to set the correct contact_id before insertion, the address will be associated with contact ID 0, which may not exist or may be the wrong contact. [ Low confidence ]

xmtp_db/src/encrypted_store/contacts/mod.rs — 1 comment posted, 5 evaluated, 4 filtered

• line 566: OFFSET without LIMIT causes a SQL syntax error in SQLite. At lines 566-568, when query.offset is Some but query.limit is None, the generated SQL will contain OFFSET X without a preceding LIMIT clause. SQLite requires LIMIT when OFFSET is used, causing a runtime database error. [ Already posted ]
• line 596: In contacts_paged, the limit and offset parameters are directly formatted into the SQL string without validation. If negative values are passed, SQLite will fail with an error like "LIMIT clause must not be negative". This is inconsistent with get_contacts which clamps these values to non-negative using .clamp(0, i64::MAX). [ Already posted ]
• line 1224: The test relies on a time-based assertion (updated_at_ns > original_created_at) with a 20ms sleep, but this could still be flaky if the system clock has low resolution or if the updated_at_ns is set before the update_contact call rather than during it. If update_contact doesn't actually update the updated_at_ns field, the assertion on line 1224 will fail. [ Already posted ]
• line 1720: The test accesses results[0], results[1], and results[2] without first asserting that results.len() == 3. If get_contacts returns fewer than 3 items due to a bug in the underlying implementation, the test will panic with an index out of bounds error rather than providing a clear assertion failure message indicating the actual vs expected count. [ Low confidence ]

xmtp_mls/src/contacts/mod.rs — 0 comments posted, 10 evaluated, 10 filtered

• line 82: The update method modifies the contact in the database but does not update the Contact struct's fields (e.g., display_name, first_name, etc.). After calling update(), the Contact instance holds stale data that no longer matches the database, which can lead to inconsistent state if the caller continues using the same Contact object. [ Low confidence ]
• line 158: The addresses() method uses filter_map with ContactAddress::new, which silently drops addresses when the constructor returns None. If valid address data in the database fails to construct a ContactAddress for any reason, it will be silently omitted from the results with no warning or error. [ Already posted ]
• line 304: The test assumes phones[0] will be the first phone number added ("555-1234"), but unless phone_numbers() guarantees insertion order, this test could be flaky. Many storage backends do not guarantee retrieval order matches insertion order. The assertion on line 304-305 may intermittently fail if the underlying implementation returns phone numbers in a different order (e.g., by ID, alphabetically, or non-deterministically). [ Already posted ]
• line 439: Accessing updated_phones[0] without first verifying that updated_phones is non-empty could cause a panic with an unclear index-out-of-bounds error if the update operation unexpectedly removes the phone number entry. Adding assert_eq!(updated_phones.len(), 1); before accessing the element would make test failures more diagnostic. [ Low confidence ]
• line 489: Direct indexing with emails[0], updated[0].email, and updated[0].delete() at lines 489, 494, and 496 will panic with an unclear "index out of bounds" error if the emails() call returns an empty vector. While this is test code and panicking indicates a bug, the error message would be unhelpful. Consider using assert!(!emails.is_empty()) before accessing or using emails.first().expect("email should exist") for clearer failure messages. [ Already posted ]
• line 519: Accessing urls[0] without first asserting that the vector is non-empty. If add_url fails to add the URL (despite returning Ok), this will panic with an index out of bounds error rather than a meaningful test failure. Consider using assert_eq!(urls.len(), 1) before indexing or using urls.first().expect("expected one url"). [ Low confidence ]
• line 526: Accessing updated[0] without verifying the vector is non-empty. If the update operation unexpectedly removes the URL or the refresh logic has a bug, this will panic with an unclear index out of bounds error. Adding an explicit length assertion before indexing would provide clearer test diagnostics. [ Already posted ]
• line 549: Index access wallets[0] on line 549 assumes add_wallet_address succeeded and returned a non-empty collection. If the wallet wasn't added (e.g., due to a silent failure or race condition), this would panic rather than providing a meaningful test failure message. Consider using wallets.first().expect("wallet should exist after add") or an explicit assertion on wallets.len() before accessing. [ Low confidence ]
• line 583: Unchecked index access at addrs[0] on line 583 could panic if contact.addresses() returns an empty vector. While the test expects one address after add_address, if the underlying implementation has a bug or the add fails silently, this would cause an unclear panic rather than a meaningful test failure. Consider using addrs.first().expect("expected one address after add") or adding an assertion like assert_eq!(addrs.len(), 1) before indexing. [ Low confidence ]
• line 592: Unchecked index access at updated[0] on lines 592, 593, and 595 could panic if the vector is empty. Adding an explicit length assertion before indexing would make test failures more informative. [ Already posted ]

xmtp_mls/src/contacts/url.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 25: The update method modifies the database but does not update the struct's url and label fields. After calling update(), the ContactUrl instance will contain stale values that don't match the database state. Callers accessing self.url or self.label after an update will see the old values instead of the newly updated ones. [ Low confidence ]

xmtp_mls/src/groups/device_sync/contact_sync.rs — 0 comments posted, 6 evaluated, 6 filtered

• line 51: The import_contacts function logs warnings for failed contact imports but continues processing. If many contacts fail to import due to a systemic issue (e.g., database corruption), the function will return a partial success count without indicating the failure rate, potentially masking serious problems. [ Low confidence ]
• line 77: The import_single_contact function compares contact_save.updated_at_ns with existing_contact.updated_at_ns to determine if an update should occur, but if the source device's clock is behind, valid updates may be silently ignored. Additionally, if updated_at_ns is 0 or unset in the proto, this comparison may incorrectly skip updates. [ Low confidence ]
• line 116: The delete_all_child_data function at line 116-119 only deletes addresses that have Some(id), but AddressData::id could be None for addresses returned from get_addresses. If the database returns addresses with id: None, those addresses will not be deleted, leaving orphaned data. [ Low confidence ]
• line 273: The 10ms sleep at line 273 is a fragile timing assumption. On heavily loaded systems or slow CI environments, system clock granularity or scheduling delays could result in both timestamps being identical or even reversed, causing the test to fail intermittently. A more robust approach would be to explicitly control the timestamps in the test data or use a larger sleep duration. [ Already posted ]
• line 311: The 10ms sleep on line 311 relies on timestamp granularity to differentiate "older" from "newer" contacts. On systems with coarse-grained timestamps (e.g., some filesystems or databases using second-level precision), 10ms may not be sufficient to create a distinguishable timestamp difference, causing the test to be flaky. [ Already posted ]
• line 324: The assertion assert_eq!(imported_count, 1) on line 324 may be incorrect. If import_contacts returns the count of contacts that were actually updated/imported, and the test verifies that the contact was NOT updated (because the existing one is newer), then imported_count should be 0, not 1. This assertion either tests the wrong expected value or indicates a misunderstanding of what import_contacts returns, potentially causing the test to pass with incorrect behavior. [ Low confidence ]

xmtp_mls/src/mls_store.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 169: In find_groups_with_inbox_ids, the call to load_mls_group_with_lock(...).ok() on line 169 silently converts all errors to None, causing groups that fail to load (due to transient errors, database locks, or corruption) to be silently excluded from results. This means a temporary failure could cause matching groups to be incorrectly omitted without any error indication to the caller. [ Already posted ]

[graphite-app]

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.

[claude]

Claude finished @rygine's task —— View job

---

Code Review Complete ✓

Review Date: January 2, 2026 05:20 UTC

---

Summary

This is a well-architected contacts API implementation with comprehensive FTS5 search, proper database normalization, and device sync integration. The code quality is generally high with good test coverage. I've identified several important issues not already covered in previous reviews:

---

Critical Issues

1. Cursor Not Advanced in Backup Stream ⚠️

File: xmtp_archive/src/export_stream/contact_save.rs:24

The backup_records implementation fetches a batch of contacts but never advances the cursor. This causes an infinite loop, repeatedly returning the same contacts during backup operations.

// Current implementation - cursor never increments
let cursor = state.cursor.load(Ordering::SeqCst);
let batch = state.db.contacts_paged(Self::BATCH_SIZE, cursor)?;
// Missing: state.cursor.fetch_add(batch.len() as i64, Ordering::SeqCst);

Impact: Backups will hang or timeout, and only the first 100 contacts will ever be backed up.

---

2. Potential SQLite Bind Variable Limit Violation

Context: While reviewing similar patterns in xmtp_db/src/encrypted_store/consent_record.rs, I noticed the codebase has methods that build IN clauses dynamically. If similar patterns exist for contacts batch operations, they could hit SQLite's 999 bind variable limit.

Recommendation: Review any future batch operations on contacts to ensure they chunk appropriately (≤900 items per batch) when using IN clauses.

---

Performance Concerns

3. FTS Trigger Performance on Bulk Imports

File: xmtp_db/migrations/2025-12-28-000000_create_contacts/up.sql:179-541

The migration documentation notes this, but it's worth emphasizing: Every companion table insert/update/delete triggers a full FTS rebuild for that contact. During device sync with hundreds of contacts, this could cause significant performance degradation.

Current behavior: For a contact with 5 phone numbers, adding them one-by-one triggers 5 FTS rebuilds.

Mitigation options:

• Document expected performance characteristics for large imports
• Consider batching companion data inserts at the application layer
• For very large imports, consider temporarily disabling triggers (though this adds complexity)

---

4. Silent Query Failures in find_groups_with_inbox_ids

File: xmtp_mls/src/mls_store.rs:165 (from previous context)

When loading MLS groups fails, errors are silently swallowed with .ok(), returning an incomplete result set without any indication to the caller. This could hide database corruption or connectivity issues.

Recommendation: At minimum, log errors at WARN level. Ideally, surface these to callers via a return type that distinguishes partial failures.

---

Code Quality Issues

5. Inconsistent Error Handling in addresses() Method

File: xmtp_mls/src/contacts/mod.rs:157

The addresses() method uses filter_map with ContactAddress::new, silently dropping addresses where construction fails:

.filter_map(|s| ContactAddress::new(self.context.clone(), s))

This is inconsistent with phone_numbers(), emails(), urls(), and wallet_addresses() which all use plain .map(). If address data is malformed, users won't know addresses are missing.

Recommendation: Use .map() for consistency, or add logging when addresses are dropped.

---

6. Missing Validation: inbox_id Uniqueness

File: xmtp_db/src/encrypted_store/contacts/mod.rs:461

The contacts table lacks a UNIQUE constraint on inbox_id. The migration at line 27 creates an index but doesn't enforce uniqueness:

CREATE INDEX idx_contacts_inbox_id ON contacts(inbox_id);

Current state: Multiple contacts can have the same inbox_id, causing ambiguity in lookups.

Recommendation: Add a UNIQUE constraint in a follow-up migration, or document why duplicates are intentionally allowed.

---

Security Observations

7. SQL Injection Prevention - Good ✓

File: xmtp_db/src/encrypted_store/contacts/mod.rs:532-556

The SQL query construction properly uses parameter binding for user input (search strings) and validates that interpolated values (sort fields, pagination) come from trusted enum sources. The SAFETY comments are helpful and accurate.

---

8. JSON Deserialization Error Handling

File: xmtp_db/src/encrypted_store/contacts/mod.rs:278-322

The code properly logs JSON parse failures with context (inbox_id, field name, error) when deserializing companion data. The warning logs are appropriate for diagnosing corruption without crashing the application. This is a good defensive pattern.

---

Test Coverage

9. Test Coverage Gaps

Good coverage:

• CRUD operations for all entity types
• Cascade delete verification (test actually checks companion tables - nice!)
• FTS search across name and companion fields
• Device sync with timestamp-based conflict resolution
• Pagination and sorting

Missing coverage:

• Empty string search behavior (does FTS handle this correctly?)
• Concurrent modifications (race conditions already noted by Macroscope)
• Very large contact lists (>1000 contacts) for pagination/FTS performance
• Special characters in search strings (FTS quote escaping)
• Edge cases in ContactAddress::new that cause None returns

---

Minor Issues

10. Test assumes array order - xmtp_mls/src/contacts/mod.rs:304: Test only checks phones[0], not phones[1], potentially missing ordering bugs.

11. Timestamp precision in tests - Multiple tests use 10-20ms sleeps to ensure timestamp differences. While the code now uses 20ms (good!), document that this may still be flaky in extreme CI environments.

---

Positive Observations ⭐

• Excellent database design: Proper normalization with companion tables and the contact_list view provides clean JSON aggregation
• Comprehensive FTS implementation: Trigram tokenizer enables substring matching across all fields
• Proper cascade deletes: Foreign key constraints with ON DELETE CASCADE are correctly configured
• Good error handling: Update/delete operations check affected row counts and return NotFound errors
• Device sync integration: Timestamp-based conflict resolution with proper child data synchronization
• Well-documented: Migration file and module-level docs explain architecture and performance trade-offs

---

Recommendations Summary

Must fix before merge:

1. Fix cursor advancement in xmtp_archive/src/export_stream/contact_save.rs:24

Should address soon:
2. Add UNIQUE constraint on contacts.inbox_id or document why duplicates are allowed
3. Improve error handling in addresses() method for consistency
4. Add logging/metrics for silently dropped errors in find_groups_with_inbox_ids

Consider for future optimization:
5. Document FTS trigger performance characteristics for large imports
6. Add test coverage for edge cases and larger datasets
7. Review batch operations for SQLite bind variable limits

---

[codecov]

Codecov Report

❌ Patch coverage is 95.52239% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.27%. Comparing base (58eeffb) to head (aa79461).

Files with missing lines	Patch %	Lines
xmtp_mls/src/contacts.rs	95.89%	29 Missing ⚠️
xmtp_db/src/encrypted_store/consent_record.rs	91.25%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2817      +/-   ##
==========================================
+ Coverage   73.95%   74.27%   +0.32%     
==========================================
  Files         393      394       +1     
  Lines       50208    51008     +800     
==========================================
+ Hits        37133    37888     +755     
- Misses      13075    13120      +45     

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

[macroscopeapp]

FTS search uses cl alias, but is_favorite and ORDER BY columns aren’t qualified, which can be ambiguous. Consider aliasing the non-search path and qualifying these with cl. (e.g., cl.is_favorite, cl.display_name).

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