feat(core): add typed USync query engine - #1063
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds typed USync query and response models, persists hosted-device metadata, and propagates hosting-aware addressing through device parsing, client processing, registry updates, session cleanup, and JID reconstruction. Tests migrate to constructor-based device fixtures. ChangesUSync and hosted device support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TypedUsync
participant DeviceRegistry
participant Jid
Client->>TypedUsync: Build and execute UsyncQuerySpec
TypedUsync-->>Client: Return parsed device-list response
Client->>DeviceRegistry: Store DeviceInfo with is_hosted
DeviceRegistry->>Jid: Reconstruct device JIDs with hosting
Jid-->>Client: Return standard or hosted JIDs
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| wacore/src/iq/usync/query.rs | Adds typed USync validation, wire models, protocol parsing, and Serde support. |
| wacore/src/iq/usync.rs | Routes specialized USync operations through the canonical typed builder and parser. |
| src/client/device_registry.rs | Persists hosted-device metadata and expands Signal cleanup across all address namespaces. |
| src/client/lid_pn.rs | Migrates regular and hosted Signal sessions when PN-to-LID mappings are discovered. |
| wacore/derive/src/lib.rs | Extends generated wire-enum support for adjacent tagged representations. |
Reviews (8): Last reviewed commit: "fix(core): harden typed USync contracts" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/device_registry.rs`:
- Around line 639-641: Update the primary-device insertion in the device-record
reconstruction flow to derive the hosted status from the incoming
device.jid.server and pass that value to DeviceInfo::new instead of always using
None. Ensure reconstruct_device_jids preserves hosted accounts while retaining
the existing primary-device handling for non-hosted devices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8679c340-2935-4cd4-9030-8ceb2fb20287
📒 Files selected for processing (14)
src/client/device_registry.rssrc/client/lid_pn.rssrc/features/signal.rssrc/handlers/notification/device.rssrc/handlers/notification/mod.rssrc/send/mod.rssrc/usync.rsstorages/sqlite-storage/src/sqlite_store.rswacore/binary/src/jid.rswacore/src/adv.rswacore/src/iq/usync.rswacore/src/iq/usync/query.rswacore/src/store/traits.rswacore/src/usync.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3015133d01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/device_registry.rs (2)
814-841: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSender-key row cleanup didn't get the hosted-namespace memo.
We just widened
delete_sessions_for_devicesto sweepServer::Hosted/Server::HostedLidvia the newsignal_namespaces(), butdelete_sender_key_rows_for_deviceright next to it still hardcodes[Server::Lid, Server::Pn]. If a companion device's JID was rendered under@hosted/@hosted.lid(which the new hosting bit makes possible), itssender_key_devicesrow will never match these two candidate servers, sopatch_device_removeleaves a stale row behind. That's exactly the kind of half-cleaned-up state this PR is supposed to be eliminating for hosted accounts — let's not leave a gap right next to the fix.🛠️ Proposed fix: reuse the same namespace set used for session cleanup
let lookup = self.resolve_lookup_keys(user).await; - let servers = [wacore_binary::Server::Lid, wacore_binary::Server::Pn]; - let mut candidates: Vec<String> = Vec::with_capacity(4); - for server in servers { - for key in lookup.all_keys() { - let mut jid = Jid::new(key, server); - jid.device = device_id; - candidates.push(jid.to_string()); - } + let mut candidates: Vec<String> = Vec::with_capacity(4); + for (key, server) in lookup.signal_namespaces() { + let mut jid = Jid::new(key, server); + jid.device = device_id; + candidates.push(jid.to_string()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/device_registry.rs` around lines 814 - 841, Update delete_sender_key_rows_for_device to use the shared signal_namespaces() server set, including hosted and hosted-lid namespaces, instead of the hardcoded Lid/Pn array; keep candidate construction, persistence deletion, and cache invalidation unchanged.
605-662: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrimary device's hosted flag still gets wiped on a raw_id mismatch — need to fix this for real.
Look, I appreciate the comment explaining why we can't classify the primary from
device.jid.server— that's the right call, and the newhosted_companion_does_not_reclassify_primary_devicetest proves it works for the normal companion-add path. But there's a second way this breaks: whenrecord.raw_idmismatches andrecord.devices.clear()fires, we throw away the existing device-0 entry — including whateveris_hostedit had — and then unconditionally re-add it at Line 661 withDeviceInfo::new(0, None)(hosted defaults tofalse). For an actually-hosted primary going through an identity-change event, that flips it back to non-hosted addressing until the next full device sync heals it. Not good enough — capture the prior bit before the clear and carry it through.🛠️ Proposed fix to preserve the primary's hosted bit across a raw_id-mismatch clear
+ let primary_was_hosted = record + .devices + .iter() + .any(|d| d.device_id == 0 && d.is_hosted); + if let Some(bytes) = signed_bytes { if let Some(decoded) = wacore::adv::decode_key_index_list(bytes) { if let Some(stored_raw_id) = record.raw_id && stored_raw_id != decoded.raw_id { ... record.devices.clear(); } ... } ... } ... if !record.devices.iter().any(|d| d.device_id == 0) { record .devices - .push(wacore::store::traits::DeviceInfo::new(0, None)); + .push(wacore::store::traits::DeviceInfo::new(0, None).with_hosting(primary_was_hosted)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/device_registry.rs` around lines 605 - 662, Preserve the primary device’s existing hosted flag when the raw_id mismatch branch in the device-update flow clears record.devices. Capture whether device 0 was hosted before record.devices.clear(), carry that value through the rebuild, and use it when re-adding device 0 in the final record.devices fallback instead of always constructing it with the default hosted=false; retain the current false default when no prior primary entry exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 814-841: Update delete_sender_key_rows_for_device to use the
shared signal_namespaces() server set, including hosted and hosted-lid
namespaces, instead of the hardcoded Lid/Pn array; keep candidate construction,
persistence deletion, and cache invalidation unchanged.
- Around line 605-662: Preserve the primary device’s existing hosted flag when
the raw_id mismatch branch in the device-update flow clears record.devices.
Capture whether device 0 was hosted before record.devices.clear(), carry that
value through the rebuild, and use it when re-adding device 0 in the final
record.devices fallback instead of always constructing it with the default
hosted=false; retain the current false default when no prior primary entry
exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdbc02ac-4325-487b-acf7-000348daa11b
📒 Files selected for processing (3)
src/client/device_registry.rswacore/src/iq/usync.rswacore/src/iq/usync/query.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c88d25160
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/iq/usync.rs (1)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up the error message formatting.
Look, this formatting leaves a dangling colon and space when the error text is empty, giving us sloppy logs like
usync status error 401:. Our systems need to be pristine and professional. Conditionally format this so we don't have trailing artifacts when the server omits the text.✨ Proposed fix for cleaner logs
fn usync_subprotocol_error_message(tag: &str, error: &UsyncSubprotocolError) -> String { let code = error .code .map(|code| code.to_string()) .unwrap_or_else(|| "unknown".to_string()); - let text = error.text.as_deref().unwrap_or(""); - format!("usync {tag} error {code}: {text}") + if let Some(text) = error.text.as_deref().filter(|s| !s.is_empty()) { + format!("usync {tag} error {code}: {text}") + } else { + format!("usync {tag} error {code}") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/iq/usync.rs` around lines 120 - 127, Update usync_subprotocol_error_message so the formatted error includes the colon and space only when error.text is present and non-empty; otherwise return the message ending after the error code without trailing whitespace or punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/iq/usync/query.rs`:
- Around line 1432-1439: Update the generic helper required_u64 to remove the
attrs.finish()? call after extracting the required attribute, while preserving
the existing missing-attribute error and u64 parsing behavior so additional node
attributes are ignored.
---
Outside diff comments:
In `@wacore/src/iq/usync.rs`:
- Around line 120-127: Update usync_subprotocol_error_message so the formatted
error includes the colon and space only when error.text is present and
non-empty; otherwise return the message ending after the error code without
trailing whitespace or punctuation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 918c105b-9192-4dd7-8c2d-e0c4f4485a3d
📒 Files selected for processing (2)
wacore/src/iq/usync.rswacore/src/iq/usync/query.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
coderabbitai
Bot
added
the
size-increase-ok
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/device_registry.rs (1)
648-661: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPreserve the hosted status of the primary device.
Look, I already called this out in a previous review. I see you added a comment arguing that the companion notification shouldn't classify the primary device. But pragmatically, if we unconditionally push a neutral entry with
is_hosted = falsefor a hosted account here,reconstruct_device_jidswill forcefully downgrade its server to PN or LID, and message delivery to the primary device will completely break. We can't let philosophical purity break core functionality. If the companion is hosted, the safest and most accurate bet is that the primary is too. Revert to extracting the hosted status from the incomingdevice.jid.server. We need this working correctly.🐛 Proposed fix to preserve hosted status
- // WA Web `AdvDeviceNotificationApi.handleDeviceAddNotification` re-adds the - // primary (device 0) to the rebuilt list unconditionally. Preserve an - // existing primary and its metadata across a raw_id reset; restore a - // neutral entry only when the input record did not contain one. - // - // The primary's key_index is never read (`filter_devices_by_key_index` keeps - // device 0 regardless and `is_key_index_valid` is not applied to it), so store - // `None` to match how device 0 is recorded everywhere else. Hosting belongs - // to each device-list entry, so the companion notification cannot classify - // the primary. - if !record.devices.iter().any(|d| d.device_id == 0) { - record - .devices - .push(wacore::store::traits::DeviceInfo::new(0, None)); - } + // WA Web `AdvDeviceNotificationApi.handleDeviceAddNotification` re-adds the + // primary (device 0) to the rebuilt list unconditionally. Preserve an + // existing primary and its metadata across a raw_id reset; restore an + // entry with matching hosted status when the input record did not contain one. + // + // The primary's key_index is never read (`filter_devices_by_key_index` keeps + // device 0 regardless and `is_key_index_valid` is not applied to it), so store + // `None` to match how device 0 is recorded everywhere else. + if !record.devices.iter().any(|d| d.device_id == 0) { + let primary_hosted = matches!( + device.jid.server, + wacore_binary::Server::Hosted | wacore_binary::Server::HostedLid + ); + record + .devices + .push(wacore::store::traits::DeviceInfo::new(0, None).with_hosting(primary_hosted)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/device_registry.rs` around lines 648 - 661, Update the primary-device fallback in the device reconstruction flow to preserve hosted status: when adding device ID 0, derive its hosted flag from the incoming primary device JID server instead of creating an unconditionally neutral entry. Ensure reconstruct_device_jids receives the correct hosted classification while retaining existing primary metadata when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/client/device_registry.rs`:
- Around line 648-661: Update the primary-device fallback in the device
reconstruction flow to preserve hosted status: when adding device ID 0, derive
its hosted flag from the incoming primary device JID server instead of creating
an unconditionally neutral entry. Ensure reconstruct_device_jids receives the
correct hosted classification while retaining existing primary metadata when
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1028707d-3648-4914-9b21-571d3881fc87
📒 Files selected for processing (1)
src/client/device_registry.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/device_registry.rs (1)
969-988: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSilent truncation risk on
reconstruct_device_jids'sdevice_id as u16cast.This file already knows how to handle a
device_idthat doesn't fit inu16—patch_device_removeexplicitly doesu16::try_from(device_id)with awarn!+ graceful skip. Here we instead rely ondebug_assert!, which is compiled out in release. If this invariant is ever violated in production, we don't get a panic or a warning — we get a silently wrong device JID reconstructed for message routing. I don't love "trust me, it's fine" logic guarding something this central to hosted addressing. Even if today's upstream writers (checked_u16in the parser,jid.device: u16on the wire) keep this bounded, let's match the pattern we already trust elsewhere in this exact file instead of leaving two different safety postures for the same class of data.🛠️ Proposed fix to mirror patch_device_remove's graceful handling
fn reconstruct_device_jids( query_jid: &Jid, record: &wacore::store::traits::DeviceListRecord, ) -> Vec<Jid> { let base = query_jid.to_non_ad(); record .devices .iter() - .map(|d| { - debug_assert!( - d.device_id <= u16::MAX as u32, - "device_id {} overflows u16", - d.device_id - ); - base.with_device_hosting(d.device_id as u16, d.is_hosted) - }) + .filter_map(|d| match u16::try_from(d.device_id) { + Ok(device_id) => Some(base.with_device_hosting(device_id, d.is_hosted)), + Err(_) => { + warn!("reconstruct_device_jids: device_id {} overflows u16 — skipping", d.device_id); + None + } + }) .collect() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/device_registry.rs` around lines 969 - 988, Update reconstruct_device_jids to handle device IDs that exceed u16 without relying on debug_assert! or silently truncating with as u16. Mirror patch_device_remove by attempting u16::try_from, logging a warning, and skipping invalid devices while preserving reconstruction for valid records.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/iq/usync/query.rs`:
- Around line 1658-1677: Update the test phone literals used by UsyncQuery,
including agent_qualified_bot and the affected tests in the indicated range, to
use a real NPA followed by the fictional 555 exchange and a line number from
0100–0199. Update the corresponding expected InvalidUserJid values consistently
while preserving each test’s validation behavior.
---
Outside diff comments:
In `@src/client/device_registry.rs`:
- Around line 969-988: Update reconstruct_device_jids to handle device IDs that
exceed u16 without relying on debug_assert! or silently truncating with as u16.
Mirror patch_device_remove by attempting u16::try_from, logging a warning, and
skipping invalid devices while preserving reconstruction for valid records.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6052f613-7517-42e2-afaf-b45f6888b1e2
📒 Files selected for processing (4)
src/client/device_registry.rswacore/src/iq/usync.rswacore/src/iq/usync/query.rswacore/src/store/in_memory.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37133ee799
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63d8315e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
…ng newline) References oxidezap/whatsapp-rust#1063
1 participant
Summary
Protocol evidence
The request builders and response parsers were checked against the captured USync modules under
docs/captured-js/WAWeb/Usync/, including the distincttandtsattributes, bot profile versioning, LID attributes, sparse results, per-protocol errors, strict picture/device parsing, and hosted device markers. Hosted Signal addressing follows the capturedWAWeb/Signal/Address.jsnamespace mapping.Compatibility
Persisted
DeviceInfovalues that predateis_hosteddeserialize as regular devices. Constructors were added forDeviceInfoandUsyncDeviceso callers do not need to repeat defaults. The publicUsyncProtocol::Features(Vec<_>)shape and adjacenttype/datarepresentation are preserved while their discriminators now share the protocol wire-tag source of truth.Validation
cargo fmt --all -- --checkcargo clippy --all --tests -- -D warnings