fix(jid): make agent identity, the AD form, and device dedup agree - #1182
Conversation
`agent` is only meaningful where the server renders it. On Pn/Lid/Hosted/HostedLid the wire spells the server as a domain byte instead, and `Display` omits any agent set there. `integrator` only exists on interop. Off those servers neither field is encoded, printed, or read back — two JIDs differing only there address the same thing. They were in the derived `PartialEq`/`Hash` anyway, so a JID picked up a different identity depending on where it was parsed from. #1178 stopped the decoder minting one such difference; this stops the difference from mattering at all, wherever it comes from. `PartialEq` and `Hash` are now written out for both `Jid` and `JidRef`, plus the cross-type comparison, all routed through one `identity_extras` so they cannot disagree — a `Hash` that disagreed with `Eq` would put an entry in a `HashMap<Jid, _>` that could never be looked up again. The two tests that broke were the ones asserting the old behaviour: both existed to prove a bundle stays invisible until the lookup key is normalised, which is the workaround this removes. They now assert the bundle is found either way.
`push_ad_to` wrote `self.agent` into the agent position. WA Web writes a literal `.0` there: its `formatFull` spelling hardcodes the string, and its Wid has no agent field to read one from (`WAWebWid`). The captured bundle is the ground truth here — this is the string the server recomputes to validate the phash, so a JID that carried an agent produced a hash the server would reject. The only production caller is `participant_list_hash`; the rest are tests, the fuzz target and a bench. This also closes the hole a reviewer flagged in the identity change: `ResolvedGroupDevices::phash` memoises by JID, so two JIDs that compare equal must produce the same AD string, or the memo can serve a hash computed for a different one. They now do.
Two corrections from an adversarial review of the identity change. `integrator` is no longer normalised. It is only ever non-zero on interop, but `is_same_chat_as` and `jids_share_user_identity` compare it unconditionally — folding it into `==` made those disagree, and the disagreement reached real decisions (quote `remote_jid`, and a retry that refuses a direct retransmission). Equality now matches them. `sort_dedup_by_device` keyed on the raw `agent`, which contradicts the rule the rest of the change establishes: two AD JIDs differing only there are one device, encode to the same AD-JID, and resolve to the same Signal address — yet both survived the dedup the group fan-out relies on to collapse duplicate destinations, so one session could get two concurrent encryption jobs. Latent before, but formalising the identity made it a contradiction. Now keyed on user/server/device.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesJID equality and hashing now normalize inert JID identity and prekey flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PrekeyResponse
participant SessionSetup
participant PrekeyMap
PrekeyResponse->>PrekeyMap: store parsed raw JID
SessionSetup->>PrekeyMap: look up requested JID directly
PrekeyMap-->>SessionSetup: match by normalized JID identity
Possibly related PRs
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 |
![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: 59a1f01cf8
ℹ️ 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".
| jids.dedup_by(|a, b| { | ||
| a.user == b.user && a.server == b.server && a.agent == b.agent && a.device == b.device | ||
| }); | ||
| jids.dedup_by(|a, b| a.user == b.user && a.server == b.server && a.device == b.device); |
There was a problem hiding this comment.
When a device list contains @bot or @interop JIDs with the same user/device but different agents, this predicate collapses them even though those servers render the agent and the new Jid::eq implementation correctly treats them as distinct identities. Any fan-out using this helper can therefore silently drop one valid destination; normalize the agent only for servers where renders_agent() is false, while retaining it in both the sort key and dedup predicate elsewhere.
Useful? React with 👍 / 👎.
![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/binary/src/jid.rs (1)
766-787: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the public AD formatter restricted.
push_ad_tois fine as “literal.0for the phash”, but becauseto_ad_string()publically delegates to it, callers usingto_ad_string()for Bot/Interop JIDs will also get the wrong agentless form. Either keeppush_ad_toprivate to the phash path or rename/adjust it so no production caller depends on it elsewhere.🤖 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/binary/src/jid.rs` around lines 766 - 787, The public AD formatting path must not reuse the phash-specific literal “.0” formatter. Restrict push_ad_to to the participant_list_hash path or rename it to clearly mark that scope, and update that caller accordingly; ensure to_ad_string() continues using the agent-aware formatter required for Bot/Interop JIDs.
🤖 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/types/jid.rs`:
- Around line 111-126: Clarify the documentation for sort_dedup_by_device to
state that its user/server/device key intentionally omits Jid::eq’s integrator
field and assumes integrator is zero for fan-out inputs. Keep the existing
deduplication behavior unchanged, and explicitly describe the interop/integrator
carve-out so the identity claim is precise.
---
Outside diff comments:
In `@wacore/binary/src/jid.rs`:
- Around line 766-787: The public AD formatting path must not reuse the
phash-specific literal “.0” formatter. Restrict push_ad_to to the
participant_list_hash path or rename it to clearly mark that scope, and update
that caller accordingly; ensure to_ad_string() continues using the agent-aware
formatter required for Bot/Interop JIDs.
🪄 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: 5f429bf0-3f35-460e-a5f8-012cb6edafbe
📒 Files selected for processing (4)
src/client/sessions.rswacore/binary/src/jid.rswacore/src/send/tests.rswacore/src/types/jid.rs
| /// Sort and deduplicate by device identity (user + server + device). | ||
| /// | ||
| /// Keyed on the same rule as `Jid`'s equality, and deliberately not on the raw | ||
| /// `agent`: on the AD servers an agent is not part of the device's identity, so | ||
| /// two JIDs carrying different ones encode to the same AD-JID and resolve to the | ||
| /// same Signal address. Keying on it here would let both survive and give the | ||
| /// group fan-out two jobs against one session. | ||
| pub fn sort_dedup_by_device(jids: &mut Vec<Jid>) { | ||
| jids.sort_unstable_by(|a, b| { | ||
| a.user | ||
| .cmp(&b.user) | ||
| .then_with(|| a.server.cmp(&b.server)) | ||
| .then_with(|| a.agent.cmp(&b.agent)) | ||
| .then_with(|| a.device.cmp(&b.device)) | ||
| }); | ||
| jids.dedup_by(|a, b| { | ||
| a.user == b.user && a.server == b.server && a.agent == b.agent && a.device == b.device | ||
| }); | ||
| jids.dedup_by(|a, b| a.user == b.user && a.server == b.server && a.device == b.device); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Tighten the doc claim — this key isn't actually "the same rule as Jid's equality."
Jid::eq folds in integrator; this dedup key doesn't and never has. Right now that's probably fine — integrator is supposed to be nonzero only for interop, which shouldn't be in this fan-out list — but if that assumption ever breaks, this function will silently collapse two genuinely different devices into one, and someone won't find out until a message goes to the wrong place. Either tighten the doc to spell out the integrator carve-out explicitly, or add a debug_assert to catch a caller violating it. I want us to be precise about what "identity" means here, not just directionally correct.
📝 Doc clarification
-/// Sort and deduplicate by device identity (user + server + device).
-///
-/// Keyed on the same rule as `Jid`'s equality, and deliberately not on the raw
-/// `agent`: on the AD servers an agent is not part of the device's identity, so
+/// Sort and deduplicate by device identity (user + server + device).
+///
+/// Deliberately narrower than `Jid`'s equality: it also omits `integrator`,
+/// relying on callers never passing interop-addressed JIDs (where `integrator`
+/// is meaningful) into this fan-out path. It is not keyed on the raw `agent`:
+/// on the AD servers an agent is not part of the device's identity, so📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Sort and deduplicate by device identity (user + server + device). | |
| /// | |
| /// Keyed on the same rule as `Jid`'s equality, and deliberately not on the raw | |
| /// `agent`: on the AD servers an agent is not part of the device's identity, so | |
| /// two JIDs carrying different ones encode to the same AD-JID and resolve to the | |
| /// same Signal address. Keying on it here would let both survive and give the | |
| /// group fan-out two jobs against one session. | |
| pub fn sort_dedup_by_device(jids: &mut Vec<Jid>) { | |
| jids.sort_unstable_by(|a, b| { | |
| a.user | |
| .cmp(&b.user) | |
| .then_with(|| a.server.cmp(&b.server)) | |
| .then_with(|| a.agent.cmp(&b.agent)) | |
| .then_with(|| a.device.cmp(&b.device)) | |
| }); | |
| jids.dedup_by(|a, b| { | |
| a.user == b.user && a.server == b.server && a.agent == b.agent && a.device == b.device | |
| }); | |
| jids.dedup_by(|a, b| a.user == b.user && a.server == b.server && a.device == b.device); | |
| } | |
| /// Sort and deduplicate by device identity (user + server + device). | |
| /// | |
| /// Deliberately narrower than `Jid`'s equality: it also omits `integrator`, | |
| /// relying on callers never passing interop-addressed JIDs (where `integrator` | |
| /// is meaningful) into this fan-out path. It is not keyed on the raw `agent`: | |
| /// on the AD servers an agent is not part of the device's identity, so | |
| /// two JIDs carrying different ones encode to the same AD-JID and resolve to the | |
| /// same Signal address. Keying on it here would let both survive and give the | |
| /// group fan-out two jobs against one session. | |
| pub fn sort_dedup_by_device(jids: &mut Vec<Jid>) { | |
| jids.sort_unstable_by(|a, b| { | |
| a.user | |
| .cmp(&b.user) | |
| .then_with(|| a.server.cmp(&b.server)) | |
| .then_with(|| a.device.cmp(&b.device)) | |
| }); | |
| jids.dedup_by(|a, b| a.user == b.user && a.server == b.server && a.device == b.device); | |
| } |
🤖 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/types/jid.rs` around lines 111 - 126, Clarify the documentation
for sort_dedup_by_device to state that its user/server/device key intentionally
omits Jid::eq’s integrator field and assumes integrator is zero for fan-out
inputs. Keep the existing deduplication behavior unchanged, and explicitly
describe the interop/integrator carve-out so the identity claim is precise.
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Three corrections from review. `sort_dedup_by_device` dropped `agent` entirely, which was wrong in the other direction: `@bot`/`@interop` DO render the agent, `==` treats those JIDs as different devices, and collapsing them silently loses a fan-out destination. It now keys on exactly what equality compares — user, server, device, integrator, `identity_agent` — so the two cannot disagree either way. `Jid::identity_agent` is public for callers that build their own key over a JID. `push_ad_to`/`to_ad_string` are renamed to `push_phash_form_to`/ `to_phash_form_string`. The old names read as a general AD rendering, but the agent position is a literal `0` (WA Web's `formatFull`, which hardcodes it with no per-server carve-out) and the only caller is the participant hash. The doc now points at `Display` for the addressed form. `normalize_for_prekey_bundle` is gone, with its call sites and the `encryption_jid.agent = 0` in encrypt.rs. Both existed to clear a field that no longer reaches identity. This is breaking twice over — a public method disappears, and the raw agent now survives on the public `PreKeyFetchOutcome::bundles` keys, `RejectedDevice::jid`, and the `IdentityChange.user` event payload — which is acceptable pre-1.0.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b22f60c0
ℹ️ 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".
| pub fn to_ad_string(&self) -> String { | ||
| /// See [`Jid::push_phash_form_to`]. Not a general JID rendering — use | ||
| /// `Display`/`to_string` for that. | ||
| pub fn to_phash_form_string(&self) -> String { |
There was a problem hiding this comment.
Running cargo fuzz run parse_jid now fails to compile because wacore/binary/fuzz/fuzz_targets/parse_jid.rs:88 still calls the removed Jid::to_ad_string() method. This fuzz crate is intentionally detached from the main workspace, so the normal workspace build and clippy jobs do not catch the stale call; update the target and its nearby comment to use to_phash_form_string().
Useful? React with 👍 / 👎.
`parse_jid.rs` still called `to_ad_string`, so `cargo fuzz run parse_jid` no longer compiled. The fuzz crate is deliberately outside the workspace, so neither the build nor the clippy job covers it — the rename passed CI with the target broken.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f202f319f
ℹ️ 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".
| &j.user, | ||
| j.server, | ||
| j.device, | ||
| j.integrator, |
There was a problem hiding this comment.
When a resolved set contains two JIDs that differ only in integrator, this key now retains both even though wacore/src/send/group.rs:249-254 defines this helper's group-send identity as excluding integrator because it is absent from the <to jid> and phash forms. Both Jid::push_phash_form_to and write_signal_address_to also omit the field, so the surviving entries become duplicate hash inputs and encryption jobs for the same wire/Signal destination; keep integrator out of this wire-device key rather than forcing it to match the broader Jid equality relation.
Useful? React with 👍 / 👎.
…atch `PartialEq` normalised both sides through `identity_agent` on every comparison. Equal raw agents are already equal identity agents — the servers matched one line above, so both sides normalise the same way — and that is the overwhelmingly common case, since nothing off the wire carries an agent on the AD servers. Measured A/B in a single binary, pinned core, with an unmodified control, three rounds: `Jid::eq` 2.39 ns -> 2.12 ns, **-11%**. That recovers most of what #1182 cost equality. The shortcut is load-bearing on the `self.server == other.server` check preceding it; the comment says so, because reordering the conjunction would silently make it wrong. Not applied to `Hash`: the same shortcut measured +4.6% there, since the SipHash dominates and the extra branch does not pay for itself.
1 participant
Replaces #1180 and #1181, which were stacked. Same work, one diff against
main, so a reviewer sees the whole thing at once.Follow-up to #1178. That PR stopped the decoder writing the AD-JID domain byte into
agent; this makes the rest of the codebase agree on what a JID's identity actually is.1. An inert
agentis not identityagentis only meaningful where the server renders it. On Pn/Lid/Hosted/HostedLid the wire spells the server as a domain byte instead — nothing encodes an agent set there (server_to_domain_type), nothing prints it (renders_agent), nothing hashes it. Two JIDs differing only there address the same device.It was in the derived
PartialEq/Hashanyway, so a JID took on a different identity depending on where it was parsed from — the wire, or the store, which keeps JIDs as text.PartialEq/Hashare now written out forJidandJidRefplus the cross-type comparison, all through oneidentity_agentso they cannot drift. This is the ruleis_same_chat_asalready used; it just was not the rule==used.2. The hashed form writes
.0, matching WA WebThe formatter behind the participant hash (
push_ad_to, renamed in §4) wroteself.agentinto the agent position. WA Web writes a literal".0"there and never reads an agent off its Wid:WAWebWidhas no agent field at all, and the.0is unconditional — there is no per-server carve-out. This string is what the server recomputes to validate the phash, so any JID carrying an agent produced a hash the server would reject — a pre-existing parity bug, reachable becauseswap_pn_lid_namespacepreservesagentacross namespace conversion.The only production caller is
participant_list_hash. This also makes the phash memo sound: it keys by JID, so equal JIDs must produce equal strings.3. Device dedup keyed on the raw agent
sort_dedup_by_devicesorted and deduplicated on(user, server, agent, device), which is wrong in both directions:@bot/@interopdevices, which do render it, silently losing a fan-out destination.It now keys on exactly what
==compares: user, server, device, integrator, andidentity_agent.Jid::identity_agentis public so callers building their own key over a JID can use the same rule instead of the raw field.4. The formatter is named for what it is
push_ad_to/to_ad_stringread as a general AD rendering, but the agent position is a literal0and the only caller is the participant hash — a name that invites exactly the misuse §2 fixed. Renamed topush_phash_form_to/to_phash_form_string, with the doc pointing atDisplayfor the addressed form.What is deliberately NOT included
integratorstays in identity. An earlier revision normalised it off interop, on the grounds that the bundle only ever produces it there (createInteropJid, and theJID_INTEROPbranch oftoString— it exists on no other JID type). That part is true, butis_same_chat_asandjids_share_user_identitycompare it unconditionally, and making==disagree with them reached real decisions: quoteremote_jidconstruction, and a retry path that refuses a direct retransmission. Not worth the contradiction for a value the wire never produces there.Breaking changes
Taken deliberately — the crate is pre-1.0.
Jid::normalize_for_prekey_bundleis removed, with its call sites and theencryption_jid.agent = 0inencrypt.rs. Both existed to clear a field that no longer reaches identity.Jid::push_ad_to/to_ad_stringare renamed topush_phash_form_to/to_phash_form_string.PreKeyFetchOutcome::bundleskeys,RejectedDevice::jid, and — via the encryption JID returned onIdentityChange::ReplacedExisting— the serializableIdentityChange.userevent. The value is inert for identity, so lookups and comparisons are unaffected; only the rendered/serialised field differs.Verification
Four tests, covering the changes above:
inert_agent_stays_out_of_identity— for each AD server: agent not splitting identity,Hashagreeing withEqon every such pair, owned/borrowed/cross-type agreeing,integratorstill splitting and agreeing withis_same_chat_as; and the other direction, where interop does render the agent and it still splits.phash_form_writes_the_agent_position_as_zero_like_wa_web— pins the spelling, and the equal-JIDs-equal-strings property the phash memo depends on.device_dedup_collapses_jids_that_differ_only_in_an_inert_agent— asserts one identity and one Signal address as preconditions, then that the dedup collapses them, and that a different device still survives.device_dedup_keeps_agents_apart_where_the_server_renders_them— the mirror case:@bot/@interopagents, and interop integrators, must NOT be merged.Two existing tests changed. Both asserted that a prekey bundle stays invisible until the lookup key is normalised — the workaround this removes — and their doc comments name the symptom it caused: "No pre-key bundle returned". They now assert the bundle is found either way. They were pinning the defect, not guarding against one.
Suites: 125 wacore-binary, 1471 wacore (
--features voip), 1281 whatsapp-rust. Clippy clean.cargo miri test -p wacore-binary --libgreen (115).Provenance
Sections 2, 3 and 4 exist because reviews pushed back on the identity change. Verifying the objections against
docs/captured-jsshowed one was pointing at a real bug — ours, not the PR's — while another rested on a premise the bundle contradicts (there is no per-server carve-out around WA Web's.0; it is unconditional). Theintegratorreversal and the both-directions dedup fix came from the same passes.