feat(history-sync): learn PN-LID mappings from phoneNumberToLidMappings - #1010
Conversation
HistorySync carries a bulk PN-LID identity table (field 15, phoneNumberToLidMappings) alongside the chats, but the streaming extractor dropped it: a freshly paired client only learned peers' LID-PN pairs one by one from live traffic (message sender_alt, usync, notifications), so LID-addressed chats from the history stayed unresolvable until each peer next showed up live. Extract the pairs during the existing streaming walk (same best-effort leniency as the pushname extractor: a malformed entry is skipped, never fatal; legacy @c.us phones are accepted as PN, wrong namespaces are rejected) and learn them through learn_lid_pn_mappings_batch after the parse. This mirrors whatsmeow, which harvests the same field in storeHistoricalPNLIDMappings on every history-sync chunk. Persist and session/registry migrations run detached like the other batch-learn paths.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughHistory sync now extracts phone-number-to-LID mappings from the protobuf, exposes them on ChangesPN↔LID Mapping Extraction and Learning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HistorySyncProto
participant process_history_sync_streaming
participant extract_lid_mapping
participant process_history_sync_task
participant learn_lid_pn_mappings_batch
participant lid_pn_cache
HistorySyncProto->>process_history_sync_streaming: phoneNumberToLidMappings field
process_history_sync_streaming->>extract_lid_mapping: parse mapping entries
extract_lid_mapping-->>process_history_sync_streaming: filtered lid_mappings
process_history_sync_streaming-->>process_history_sync_task: HistorySyncResult
process_history_sync_task->>learn_lid_pn_mappings_batch: (lid, phone_number) pairs
learn_lid_pn_mappings_batch->>lid_pn_cache: persist PN↔LID learning
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/history_sync.rs | Adds HistoryLidMapping struct, read_str_field helper, and extract_lid_mapping parser; plumbs the new lid_mappings field through HistorySyncResult; schema const-asserts pin the new proto field numbers. Validation is thorough and best-effort approach matches the pushname extractor pattern. |
| src/history_sync.rs | Feeds lid_mappings from HistorySyncResult into learn_lid_pn_mappings_batch with correct pair ordering and LearningSource, consistent with existing usync and groups callers. End-to-end test verifies the mapping lands in the LID-PN cache. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant WA as WhatsApp Server
participant HS as process_history_sync_task
participant PS as process_history_sync_streaming
participant EX as extract_lid_mapping
participant LB as learn_lid_pn_mappings_batch
participant CA as LidPnCache / Store
WA->>HS: HistorySyncNotification (compressed payload)
HS->>PS: decompress + stream parse
loop For each PHONE_NUMBER_TO_LID_MAPPINGS field
PS->>EX: raw field bytes
EX-->>PS: "Option<HistoryLidMapping> (None = skip)"
end
PS-->>HS: "HistorySyncResult { lid_mappings, … }"
alt lid_mappings non-empty
HS->>LB: "Vec<(lid, phone_number)>, MigrationSyncLatest, is_offline=false"
LB->>CA: populate in-memory cache (sync)
LB-->>LB: spawn detached task: persist + migrate registry
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant WA as WhatsApp Server
participant HS as process_history_sync_task
participant PS as process_history_sync_streaming
participant EX as extract_lid_mapping
participant LB as learn_lid_pn_mappings_batch
participant CA as LidPnCache / Store
WA->>HS: HistorySyncNotification (compressed payload)
HS->>PS: decompress + stream parse
loop For each PHONE_NUMBER_TO_LID_MAPPINGS field
PS->>EX: raw field bytes
EX-->>PS: "Option<HistoryLidMapping> (None = skip)"
end
PS-->>HS: "HistorySyncResult { lid_mappings, … }"
alt lid_mappings non-empty
HS->>LB: "Vec<(lid, phone_number)>, MigrationSyncLatest, is_offline=false"
LB->>CA: populate in-memory cache (sync)
LB-->>LB: spawn detached task: persist + migrate registry
end
Reviews (2): Last reviewed commit: "chore: address PR review comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/history_sync.rs (1)
2964-2975: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParity oracle doesn't actually cover the new field.
Look, every other extracted field here (
nct_salt,own_pushname,tc_token_candidates,msg_secret_records) is protected byreference_full_walk+streaming_extraction_matches_full_buffer_reference.lid_mappingsonly got the struct-literal update to keep the compiler happy —reference_full_walknever parsesPHONE_NUMBER_TO_LID_MAPPINGS,parity_fixture()carries no such data, and the parity test never assertslid_mappingsequality. A future streaming-path regression on this field would sail right through this differential test. We built this whole oracle mechanism specifically to catch this class of bug — let's not leave the newest field as the one blind spot.✅ Sketch of the missing parity coverage
tags::history_sync::NCT_SALT if wt == wire_type::LENGTH_DELIMITED => { ... } + tags::history_sync::PHONE_NUMBER_TO_LID_MAPPINGS + if wt == wire_type::LENGTH_DELIMITED => + { + let (len, vlen) = read_varint(&decompressed[pos..]).unwrap(); + pos += vlen; + let end = checked_end(pos, len, decompressed.len()).unwrap(); + if let Some(mapping) = extract_lid_mapping(&decompressed[pos..end]) { + result.lid_mappings.push(mapping); + } + pos = end; + } _ => { pos = skip_field(wt, decompressed, pos).unwrap(); }Then add a
phoneNumberToLidMappingsentry toparity_fixture()andassert_eq!(result.lid_mappings, reference.lid_mappings)instreaming_extraction_matches_full_buffer_reference.🤖 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/history_sync.rs` around lines 2964 - 2975, The parity test coverage for the new lid_mappings field is missing, so a streaming regression could bypass the oracle. Update reference_full_walk to parse PHONE_NUMBER_TO_LID_MAPPINGS into result.lid_mappings, extend parity_fixture() with a phoneNumberToLidMappings entry, and add an assert_eq! for lid_mappings in streaming_extraction_matches_full_buffer_reference so the streaming and full-buffer paths are compared for this field too.
🤖 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/history_sync.rs`:
- Around line 626-676: The two length-delimited parsing branches in
extract_lid_mapping are duplicated and should be consolidated to prevent drift.
Extract the shared read logic into a small helper used for both
tags::phone_number_to_lid_mapping::PN_JID and
tags::phone_number_to_lid_mapping::LID_JID, keeping the existing read_varint,
smoothutf8::from_utf8, and pos advancement behavior unchanged while only varying
the destination variable.
---
Outside diff comments:
In `@wacore/src/history_sync.rs`:
- Around line 2964-2975: The parity test coverage for the new lid_mappings field
is missing, so a streaming regression could bypass the oracle. Update
reference_full_walk to parse PHONE_NUMBER_TO_LID_MAPPINGS into
result.lid_mappings, extend parity_fixture() with a phoneNumberToLidMappings
entry, and add an assert_eq! for lid_mappings in
streaming_extraction_matches_full_buffer_reference so the streaming and
full-buffer paths are compared for this field too.
🪄 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: c28ada07-f125-47c2-9328-6eb00ccc7d14
📒 Files selected for processing (2)
src/history_sync.rswacore/src/history_sync.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 4/5
- In
wacore/src/history_sync.rs, the parity test path aroundreference_full_walkno longer validatesHistorySyncResult.lid_mappings, so a regression in that field could ship without being caught by the full-buffer check — teach the reference walk to populatelid_mappingsand include it in parity assertions before merging. - In
wacore/src/history_sync.rs, the duplicated length-delimited parsing forPN_JIDandLID_JIDincreases the chance of the two paths drifting and producing inconsistent decode behavior over time — extract a shared helper (for exampleread_len_field) to keep fixes and validation consistent.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="wacore/src/history_sync.rs">
<violation number="1" location="wacore/src/history_sync.rs:284">
P3: The full-buffer parity test no longer covers the new `HistorySyncResult.lid_mappings` field, because `reference_full_walk` always leaves it empty and the parity assertions skip it. Consider teaching the reference walk to parse `PHONE_NUMBER_TO_LID_MAPPINGS` and asserting equality so future streaming regressions are caught.</violation>
<violation number="2" location="wacore/src/history_sync.rs:648">
P3: The length-delimited field read logic for `PN_JID` and `LID_JID` is duplicated verbatim (differing only in the target variable). Consider extracting a small helper like `read_len_field(data, &mut pos) -> Option<&str>` to avoid the two copies drifting independently in future edits.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- trim the learn-site comment to motivation only (greptile) - drop the pure-what comment on the streaming match arm (greptile) - dedupe the PN_JID/LID_JID reads into read_str_field (coderabbit, cubic) - teach the full-buffer parity oracle field 15 and assert lid_mappings parity, incl. a wrong-namespace entry both walks must skip (cubic, coderabbit)
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Adds PN-LID mapping from history sync into the production LID-PN cache, touching core logic in wacore and whatsapp-rust. Data integrity and migration timing concerns require human review.
Re-trigger cubic
2 participants
Motivation
HistorySyncships a bulk PN↔LID identity table alongside the chats — field 15,phoneNumberToLidMappings— but the streaming extractor currently drops it. A freshly paired client therefore only learns peers' LID↔PN pairs one at a time from live traffic (messagesender_alt, usync responses, notifications), and LID-addressed chats imported from history stay unresolvable until each peer happens to show up live.whatsmeow harvests exactly this field on every history-sync chunk (
storeHistoricalPNLIDMappings, message.go); this PR brings the same behavior here.What it does
wacore: the streaming walk inprocess_history_sync_streamingextracts eachPhoneNumberToLIDMappingintoHistorySyncResult.lid_mappings(newHistoryLidMapping { phone_number, lid }, bare user parts). Same best-effort leniency as the pushname extractor: malformed entries are skipped, never fatal. Legacy@c.usphones are accepted as PN (whatsmeow normalizes them the same way); wrong namespaces and incomplete entries are rejected. Tag constants are pinned in the existing const-assert block.whatsapp-rust: after a successful parse,process_history_sync_taskfeeds the pairs tolearn_lid_pn_mappings_batch(LearningSource::MigrationSyncLatest), so cache, store, and session/registry migrations follow the same path as the other batch learns (persist + migrations detached).Notes for review
MigrationSyncLatest— its doc comment ("learned from latest history sync migration") finally matches a real history-sync writer. Happy to add a dedicatedHistorySyncvariant instead if you prefer.learn_lid_pn_mappings_batchadditionally runs the registry/session migrations on its detached task; if you'd rather match whatsmeow strictly for large histories, I can switch to the store-only half.Tests
wacore: extraction test covering valid pairs, legacy@c.us, device-suffixed users, wrong namespaces, and incomplete entries.whatsapp-rust: end-to-endprocess_history_sync_tasktest asserting the pair lands in the LID-PN cache.cargo fmt --checkandcargo clippyclean; fullhistory_sync+lid_pntest modules pass.