fix(status): use LID addressing, skip unresolvable recipients - #568
Merged
Merged
Conversation
`send_status_message` used to force every recipient to PN addressing
via an in-memory-only lookup, then abort the whole send if any LID had
no PN mapping in the cache. That made status reactions to LID-only
contacts fail indefinitely — the bot can decrypt the status (Signal
state is under the LID), but the status sender has no way to learn the
poster's phone number without a DM or usync round-trip.
This mismatch also contradicts WA Web. `WAWebEncryptAndSendStatusMsg`
(`docs/captured-js/WAWeb/Encrypt/AndSendStatusMsg.js:46`) does:
compactMap(x.list, WAWebLidMigrationUtils.toUserLid)
i.e. every recipient is mapped to its LID, and `compactMap` silently
drops entries that can't be resolved. Status is LID-addressed, not PN,
since the LID migration.
Changes:
- `send_status_message` now resolves each recipient to its LID form
(LID passes through; PN goes through the cache-aside
`Client::get_lid_pn_entry`, which falls back to the backend on a
cold cache — added in PR #565). Unresolvable entries are skipped
silently, matching WA Web. `AddressingMode::Lid` and `own_lid` drive
the downstream encryption path.
- New pure helper `wacore::send::assemble_status_participants` owns
the dedup + own-LID anchoring + empty-list guard. Linear Vec scan
(no HashSet allocation — status lists are small) and it consumes
the resolved JIDs by value, so CompactString lives exactly once per
kept recipient. 6 unit tests cover dedup, compactMap-style skipping,
the empty-after-skip error path, and the ad-vs-bare own-LID case.
- New helper `Client::resolve_recipient_to_lid` mirrors
`WAWebLidMigrationUtils.toUserLid` and is the single call site for
PN→LID resolution in the status path (DRY).
- The sender-key-name derivation and `resolve_skdm_targets` now key
off `own_lid` to match the stanza's addressing mode.
Error only when every resolved recipient is unresolvable — a single
LID-only contact no longer breaks the whole send.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds LID-based recipient resolution and participant assembly for status messages: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Resolver as resolve_recipient_to_lid
participant Cache as get_lid_pn_entry
participant Assembler as assemble_status_participants
participant Wacore as wacore::send
Client->>Resolver: resolve_recipient_to_lid(jid)
alt input is LID
Resolver-->>Client: Some(bare LID)
else input is PN
Resolver->>Cache: get_lid_pn_entry(pn)
alt OK(Some(entry))
Cache-->>Resolver: Some(LID)
Resolver-->>Client: Some(LID)
else OK(None) or Err
Cache-->>Resolver: None / Err
Resolver-->>Client: None (warn on Err)
end
else non-user JID
Resolver-->>Client: None
end
Client->>Assembler: [Option<Jid>, ...], own_lid
Assembler->>Assembler: drop None -> dedupe by user -> ensure own bare user present
Assembler-->>Client: Vec<Jid>
Client->>Wacore: build GroupInfo(AddressingMode::Lid) & send
Wacore-->>Client: status send result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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)
src/send.rs (1)
331-335:⚠️ Potential issue | 🟠 MajorRequire a real own LID before sending a LID-addressed status.
This path now assumes
own_lidis the sender identity for sender-key lookup, participant anchoring, and exact-sender filtering inprepare_group_stanza. Falling back toown_jidhere means we can emitaddressing_mode="lid"while deriving the sender key from PN, and we can re-include the sending device in SKDM fanout. That should fail fast, not silently downgrade.Suggested fix
- let own_lid = device_snapshot - .lid - .take() - .unwrap_or_else(|| own_jid.clone()); + let own_lid = device_snapshot + .lid + .take() + .ok_or_else(|| anyhow!("LID not set, cannot send status"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 331 - 335, The code currently falls back to own_jid when device_snapshot.lid is missing which allows emitting addressing_mode="lid" while using the wrong sender identity; change the logic around device_snapshot.lid/own_lid (the variable set here and any callers like prepare_group_stanza) to require a real LID: if device_snapshot.lid is None, return an error or early-fail before constructing a LID-addressed status instead of using unwrap_or_else(|| own_jid.clone()); ensure prepare_group_stanza and any sender-key lookup / SKDM fanout logic use that guaranteed LID and do not proceed when no device LID exists (fail fast with a clear error referencing own_lid/device_snapshot.lid).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/send.rs`:
- Around line 336-345: The loop that currently rejects only
groups/status/broadcasts should instead fail fast for any non-user JID so
invalid types don't reach resolve_recipient_to_lid() and get silently dropped;
update the recipients validation in the block iterating over recipients to
assert the JID is a user (e.g., use an existing jid.is_user() check or compare
JID kind to the user/PN/LID variant) and return an Err with the same style
message when it's not, referencing the same identifiers (recipients, jid,
resolve_recipient_to_lid) so mixed valid+invalid lists cannot partially send.
---
Outside diff comments:
In `@src/send.rs`:
- Around line 331-335: The code currently falls back to own_jid when
device_snapshot.lid is missing which allows emitting addressing_mode="lid" while
using the wrong sender identity; change the logic around
device_snapshot.lid/own_lid (the variable set here and any callers like
prepare_group_stanza) to require a real LID: if device_snapshot.lid is None,
return an error or early-fail before constructing a LID-addressed status instead
of using unwrap_or_else(|| own_jid.clone()); ensure prepare_group_stanza and any
sender-key lookup / SKDM fanout logic use that guaranteed LID and do not proceed
when no device LID exists (fail fast with a clear error referencing
own_lid/device_snapshot.lid).
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 03eaed02-d235-41c1-8249-2fc9c027b89a
📒 Files selected for processing (3)
src/client/lid_pn.rssrc/send.rswacore/src/send.rs
Benchmark Results59 unchanged benchmark(s)
|
Two review findings confirmed against the current code; both legitimate. Fixes: - Reject any recipient that is not a user JID (PN or LID) upfront. Previously only group/status/broadcast JIDs errored; anything else (newsletter, hosted, server, etc.) silently became `None` in resolve_recipient_to_lid and got filtered by the compactMap-style skip, masking caller bugs. WA Web's `toUserLid` starts with `asUserWidOrThrow(e)` and propagates the throw, which is the behavior we now match. - Refuse to send when `device_snapshot.lid` is None. The prior `unwrap_or_else(|| own_jid.clone())` would emit `addressing_mode="lid"` on the stanza while signing with the PN sender, and the sender key would be stored under the wrong address. Post-LID-migration every real account has a device LID, so the missing-LID branch is a "not ready to send status" state, not something to paper over. Both wire behaviors (reject non-user and require device LID) match WA Web; the existing `assemble_status_participants` tests already cover downstream behavior, and the whole workspace test suite stays green.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/send.rs`:
- Around line 295-308: The docblock for status message LID addressing is too
verbose and implementation-specific; trim it to state the external contract
(e.g., "Status messages use LID addressing: recipients already in LID pass
through, PN recipients are resolved to LID via cache-aside lookup, and
unresolved recipients are silently skipped; resulting GroupInfo uses
AddressingMode::Lid and stanzas are signed with own_lid and emit
addressing_mode=\"lid\""). Remove file-paths, step-by-step pipeline rationale
(e.g., WAWebEncryptAndSendStatusMsg, WAWebLidMigrationUtils, compactMap) and
move those details into tests or a design note; keep references to
Client::get_lid_pn_entry, GroupInfo, AddressingMode::Lid, prepare_group_stanza
and own_lid only as needed to state behavior, not implementation.
- Around line 372-377: The sender-key lookup uses a device-scoped own_lid but
receiver-side expects a bare LID; change the construction of the sender key so
it normalizes own_lid to the non-device (bare) LID before converting to protocol
address: call own_lid.to_non_ad().to_protocol_address() and use that result when
creating the SenderKeyName in the block that currently calls
SenderKeyName::from_parts(&to_str, sender_address.as_str()); also verify
prepare_group_stanza uses the same bare form (replace any use of
own_lid.to_protocol_address() with own_lid.to_non_ad().to_protocol_address()
where this status path builds sender keys) so both write and read use the same
user@lid identity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+372
to
377
| // Sender key name tracks the addressing mode of the group stanza. | ||
| // Since status now uses LID addressing (see send_status_message | ||
| // header), the key is stored under own_lid, matching the address | ||
| // prepare_group_stanza derives internally. | ||
| let sender_address = own_lid.to_protocol_address(); | ||
| let sender_key_name = SenderKeyName::from_parts(&to_str, sender_address.as_str()); |
There was a problem hiding this comment.
Normalize the sender-key identity to the bare LID before building the key name.
Right now this keys sender-key state off the device-scoped own_lid. src/message.rs loads sender keys with info.source.sender.to_non_ad(), so this status path can write/look up user:device@lid while receive-side lookup uses user@lid. That mismatch will keep missing cached sender-key state and can break follow-up status decrypt/reuse. Use own_lid.to_non_ad().to_protocol_address() here, and make sure prepare_group_stanza is using the same bare form.
Suggested fix
let force_skdm = {
use wacore::libsignal::store::sender_key_name::SenderKeyName;
@@
- let sender_address = own_lid.to_protocol_address();
+ let sender_address = own_lid.to_non_ad().to_protocol_address();
let sender_key_name = SenderKeyName::from_parts(&to_str, sender_address.as_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/send.rs` around lines 372 - 377, The sender-key lookup uses a
device-scoped own_lid but receiver-side expects a bare LID; change the
construction of the sender key so it normalizes own_lid to the non-device (bare)
LID before converting to protocol address: call
own_lid.to_non_ad().to_protocol_address() and use that result when creating the
SenderKeyName in the block that currently calls
SenderKeyName::from_parts(&to_str, sender_address.as_str()); also verify
prepare_group_stanza uses the same bare form (replace any use of
own_lid.to_protocol_address() with own_lid.to_non_ad().to_protocol_address()
where this status path builds sender keys) so both write and read use the same
user@lid identity.
Collaborator
Author
There was a problem hiding this comment.
I checked the statement "receiver-side expects bare LID" in docs/captured-js/WAWeb/Signal/Address.js:15-34 and Signal/CommonUtils.js:30-48:
toString = function () {
var t = this.wid.device != null && this.wid.device !== 0 ? ":" + this.wid.device : "";
...
return [u.user, t, "@lid"].join("");
}
WA Web includes :device in the name when device != 0, and createSignalLikeSenderKeyName uses this toString() + .0. It forms device-scoped, not bare.
The current code uses own_lid.to_protocol_address() both in the force_skdm check and within prepare_group_stanza (line 1054). Both match each other and match with WA Web. Changing to .to_non_ad().to_protocol_address() would break:
Parity with WA Web receivers (which expect device-scoped).
Consistency with the group send path, which already works with device-scoped.
Applying this finding would introduce a bug where there wasn't one. The reviewer started from an incorrect premise about what the receiver reads.
There was a problem hiding this comment.
@jlucaso1, you're right — I was wrong on the underlying premise. WA Web's SenderKeyName derivation is device-scoped: toString() includes :device when device !== 0, and createSignalLikeSenderKeyName appends .0 to that full device-scoped address. The current code using own_lid.to_protocol_address() (device-scoped) is consistent between the force-SKDM check and prepare_group_stanza, and matches what WA Web produces. My suggestion to normalize to .to_non_ad() would have broken that parity and introduced a real bug. I'm withdrawing this comment — the implementation is correct as-is.
✏️ Learnings added
Learnt from: jlucaso1
Repo: oxidezap/whatsapp-rust PR: 568
File: src/send.rs:374-379
Timestamp: 2026-04-18T16:30:22.140Z
Learning: In the whatsapp-rust codebase (src/send.rs), WA Web derives sender-key names using a device-scoped LID address, not a bare one. WA Web's Signal/Address.js toString() includes `:device` when device != 0, and createSignalLikeSenderKeyName appends `.0` to that full string. So the correct form for SenderKeyName construction is `own_lid.to_protocol_address()` (device-scoped), NOT `own_lid.to_non_ad().to_protocol_address()`. Both the force_skdm check in send_status_message and prepare_group_stanza use device-scoped own_lid consistently, matching WA Web behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
![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: 083f903350
ℹ️ 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".
First review finding is valid: the docblock referenced a captured-js file path and line number, which the project style rule (.claude/rules/code-style.md) forbids. Trimmed both the `send_status_message` header and `assemble_status_participants` comment to state the external contract plus a WA Web module name (allowed per the same rule), dropping the pipeline/compactMap rationale that duplicates what the code already shows. Second review finding is rejected after verifying against WA Web. `WAWebSignalAddress.toString` (docs/captured-js/WAWeb/Signal/Address.js) includes the device in the sender-key name when device != 0, and `createSignalLikeSenderKeyName` wraps that via SignalAddress + ".0". Our code matches: both `send_status_message` (force_skdm check) and `prepare_group_stanza` derive the sender address from the device-scoped `own_lid` via `to_protocol_address()`. Switching to `to_non_ad()` would break parity with WA Web receivers and diverge from the working group send path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87f3b38a83
ℹ️ 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".
Both Codex findings confirmed against the current code. send_status_message now populates GroupInfo's lid_to_pn_map with own LID↔PN plus every resolved recipient whose input was a PN. Without this, prepare_group_stanza's full-SKDM path (force_skdm=true on first status send) could not translate LID participants back to PN for device resolution, and LID usync on own JID is unreliable — own companion devices would drop out of the distribution list. resolve_skdm_targets now takes &GroupInfo and applies the same LID→PN fallback prepare_group_stanza uses: in LID mode, LID participants with a known PN mapping are queried via PN; the resulting device JIDs are converted back to LID via phone_device_jid_to_lid so they match the cache and the stanza's addressing. Fixes the incremental-SKDM case (force_skdm=false) where a newly paired companion invalidated the sender-key-devices cache and resolve_skdm_targets returned an LID device list that missed own companions. Group sends benefit from the same fallback (the bug was pre-existing there too — groups exercise the same LID usync path).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765a31821d
ℹ️ 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".
| e | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
When get_lid_pn_entry fails (e.g., transient DB/backend outage), this path converts the error into None, and send_status_message treats that recipient as merely “unresolvable” and silently drops it. That means status sends can partially succeed while omitting valid recipients without surfacing an error to callers, which is especially risky for status revoke flows that must target the original audience. Lookup failures should be surfaced distinctly from a true “no mapping” miss.
Useful? React with 👍 / 👎.
jlucaso1
added a commit
that referenced
this pull request
Apr 18, 2026
Status reactions started getting NACK'd with 479 SmaxInvalid after PR #568 actually put stanzas on the wire (previously they aborted client-side). Root cause: `send_status_message` was attaching `<meta status_setting>` to every non-revoke send, including reactions. That meta describes the POSTER's privacy on their own status. WA Web attaches it only inside `WAWebEncryptAndSendStatusMsg`, which runs for status posts. Reactions route through `WAWebSendReactionMsgAction` → `WAWebSendAddonMsgChatAction` → `sendAddonRecord` and never visit the status-post path, so the meta is absent. Baileys doesn't emit `status_setting` at all. Only whatsapp-rust was gratuitously attaching it to reactions, which the server rejects. Extracted `wacore::send::status_carries_privacy_meta(&Message)` as a pure helper (true only for actual posts — not reactions, not revokes) and drove `send_status_message` off it. Six unit tests pin the classification: text post, image post, reaction, enc-reaction, revoke, non-revoke protocol message. Fixes reactions to LID-only contacts not appearing on the poster's side and the resulting silent failure in consumer bots.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Reacting to a status from a LID-only contact (someone whose status the bot can decrypt but whose phone number it has never learned) used to fail permanently with:
Two compounding issues caused the break, and the fix has to touch both the addressing direction AND the device-resolution fallback that the new direction depends on:
Cache-aside bypass.
send_status_messagecalledlid_pn_cache.get_phone_numberdirectly — in-memory only. Mappings persisted in the backend but not yet loaded were invisible.Client::get_lid_pn_entryhas a cache-aside fallback (PR fix: cache-aside fallback in get_lid_pn_entry #565) but the status path was not using it.Wrong addressing direction. The code forced every recipient to PN and aborted if any LID could not be resolved. That contradicts current WA Web behaviour. In
WAWebEncryptAndSendStatusMsg:WA Web maps every recipient through
toUserLid(returns the LID ornull), andcompactMapsilently drops the nulls. Since the LID 1:1 migration, status is LID-addressed, not PN.Device resolution in LID mode misses own companions. LID usync is unreliable for own JID.
prepare_group_stanzaalready works around this viaGroupInfo::phone_jid_for_lid_user+phone_device_jid_to_lid— but only if theGroupInfohas anlid_to_pn_mappopulated, and only in the full-SKDM path.resolve_skdm_targetshad no equivalent fallback.What changed
Addressing + resolution
send_status_messageuses LID addressing. Each recipient is resolved to its LID form: LID passes through, PN goes through the cache-asideClient::get_lid_pn_entry. Unresolvable recipients are skipped silently.AddressingMode::Lidon theGroupInfo, signed withown_lid, emitsaddressing_mode="lid".GroupInfois built viawith_lid_to_pn_mapseeded with own's LID↔PN plus every resolved (PN→LID) recipient pair. That's the mapprepare_group_stanzaneeds for its fallback to work on first send.resolve_skdm_targetsnow takes&GroupInfo. In LID mode it queries devices via PN where the map has a mapping (same pattern asprepare_group_stanza), then converts the resulting device JIDs back to LID viaphone_device_jid_to_lidso they match the cache and the stanza. Fixes the incremental-SKDM case where a new companion invalidated the device cache and the old direct-LID query missed own companions. Group sends benefit from the same fallback (bug was pre-existing there too).resolve_recipient_to_lid. Mirrors WA Web'sasUserWidOrThrowinsidetoUserLid.device_snapshot.lidisNone. Avoids emittingaddressing_mode="lid"while signing with a PN sender.Helpers
wacore::send::assemble_status_participants— pure helper, dedup + own-LID anchoring + empty-list guard. LinearVecscan (noHashSetalloc). Six unit tests: dedup, compactMap-style skip, empty-after-skip error, ad-vs-bare own.Client::resolve_recipient_to_lid— single DRY call site for per-JID PN→LID resolution. MirrorsWAWebLidMigrationUtils.toUserLid.own_lid.to_protocol_address()— matchesWAWebSignalAddress.toString(device suffix included when non-zero) and stays consistent withprepare_group_stanza.Evidence of wire-format parity
compactMap)asUserWidOrThrow)getCurrentLidget_lid_pn_entrycache-asidelid_to_pn_mapuser:device@lid.0user:device@pn.0user:device@lid.0Test plan
cargo fmt --allcargo clippy --all --tests --exclude e2e-tests— cleancargo test --workspace --exclude e2e-tests --exclude bench-integration— 29 suites green (544 wacore lib + 12 wire_enum_serde + per-crate)wacore::send::assemble_status_participants: dedup, compactMap-style skip, empty-list error, own-LID anchoring, ad-vs-bare handlingresolve_skdm_targets+prepare_group_stanzaverified to share the same LID→PN fallback pathsend_status_messageseedslid_to_pn_mapfor own + resolved recipients so the fallback has data to work withBreaking changes
None at the caller surface.
status().send_text(...)etc. still take&[Jid]— LIDs and PNs both work, with strictly more resolving correctly now. The wire format change (addressing_mode="lid"on status stanzas) matches WA Web and is what the server expects post-LID-migration.The internal
Client::resolve_skdm_targetssignature changed from(group_jid, &[Jid], &Jid)to(group_jid, &GroupInfo, &Jid).pub(crate), so no public-API impact.