fix(channels/whatsapp-web): pass LID JIDs unchanged to whatsapp-rust 0.6+ - #6973
Conversation
Audacity88
left a comment
There was a problem hiding this comment.
Context: I reviewed current head 4b8a6b53 against the live PR state, green GitHub CI, the current diff in Cargo.lock, crates/zeroclaw-channels/Cargo.toml, and crates/zeroclaw-channels/src/whatsapp_web.rs, the earlier #6845 LID-to-phone workaround, #6706's whatsapp-rust 0.6 migration, and upstream oxidezap/whatsapp-rust #636. I did not run local cargo for this pass.
🟢 What looks good — deferring LID namespace handling to whatsapp-rust is the right boundary
The ZeroClaw-side direction makes sense. #6845's local LID→phone conversion was reasonable before the 0.6 send path fixed the participant namespace problem, but after upstream #636 the lower-level library needs to keep the stanza in a consistent namespace. Passing the chat JID through unchanged is a cleaner boundary than having ZeroClaw convert a LID chat back to a phone JID and then expecting the library to repair the namespace again.
The helper tests also pin the intended local behavior clearly: LID DMs, phone-number DMs, and groups now preserve the original chat JID.
🔴 Blocking — the dependency roll-forward is broader than the PR body and validation describe
The body currently frames this as only outbound reply routing, and says the blast radius is just whatsapp_web.rs with an additive change. The actual dependency pin moves the whatsapp-rust family from 9734fb2 to cbcdd2a, and that upstream compare is broader than #636 alone. In addition to the final LID namespace fix, it spans several WhatsApp runtime fixes before the #636 squash merge, including changes in upstream client, message, retry, prekey, send, and libsignal paths.
That broader roll-forward may be exactly the right dependency point, but the merge record should say that plainly. For a live channel-runtime dependency pin, I do not want to merge a PR whose public scope/rollback/validation says “one outbound reply helper” while the shipped dependency update carries more WhatsApp protocol behavior than that.
Please update the PR body so it matches the real change:
- describe the pin as a
whatsapp-rust0.6 family roll-forward to the current post-#636 commit, not just a localcompute_reply_targetcleanup; - replace the “additive” blast-radius language with a narrow behavior-fix description;
- fill the medium-risk rollback fields or otherwise make the rollback plan concrete;
- spell out the exact manual WhatsApp smoke scenario used for this bug, especially whether it covered a LID-addressed DM and/or self-chat reply path.
I can handle the maintainer-side label cleanup separately once the body is accurate.
🔵 Suggestion — clean the stale local comments while touching the body
Two small comments/log strings still describe the old conversion behavior: the reply_target != chat debug log says LID→phone reply target, and the ChannelMessage comment says self-chat with LID JIDs uses a resolved phone JID. They are not the main blocker, but they will be misleading after this PR because compute_reply_target() now always passes through unchanged.
3da47fb to
422b330
Compare
|
@Audacity88 thanks for feedback, ready for review. |
Audacity88
left a comment
There was a problem hiding this comment.
Context: I re-reviewed current head 422b3305 after @tmigone updated the branch and PR body. I checked the current PR state, my prior CHANGES_REQUESTED review, top-level comments, formal reviews, inline comments, full diff across Cargo.lock, crates/zeroclaw-channels/Cargo.toml, and crates/zeroclaw-channels/src/whatsapp_web.rs, current green GitHub checks, and the upstream whatsapp-rust commit range described in the PR body. I did not run local cargo for this pass.
✅ Resolved — the dependency roll-forward is now accurately scoped
The PR body now says this is a whatsapp-rust family roll-forward from 9734fb2 to cbcdd2a, not only a local compute_reply_target helper cleanup. That matches the actual lockfile and manifest diff, and it names the upstream commits that sit between the old pin and the post-#636 pin: self-DM compliance, deadlock/session recovery, ACK recipient preservation, softer stream errors, prekey re-upload after re-pair, and the LID namespace fix.
That clears my previous blocker. The merge record now makes the WhatsApp protocol dependency blast radius visible instead of hiding it behind the local passthrough helper change.
✅ Resolved — stale LID-to-phone comments were cleaned up
The old LID→phone reply target debug branch is gone, and the ChannelMessage comment now says replies use the originating chat JID unchanged while whatsapp-rust handles LID addressing internally. That matches the new behavior and avoids leaving future readers with the pre-0.6 mental model.
🔴 Blocking — remove the unused passthrough helper parameters
One small code-hygiene issue remains, and I missed it in the first pass. compute_reply_target() is now a private passthrough helper, but the signature keeps the old is_lid, is_group, and mapped_phone inputs as _is_lid, _is_group, and _mapped_phone only to silence unused-parameter warnings.
Those are not required trait/API/callback parameters, so please simplify the helper to take only the chat JID and update the call site/tests accordingly. The intended behavior stays the same: LID DMs, phone-number DMs, and group JIDs all pass through unchanged.
🟢 What looks good — the local boundary is now deliberately thin
The ZeroClaw-side code now preserves the incoming chat JID for LID DMs, phone-number DMs, and groups, and the helper tests pin those three cases directly. That is the right local contract now that the upstream library owns LID namespace repair inside the WhatsApp send path.
Separate reviewer-side metadata cleanup: the live labels are dependencies, channel, and channel:whatsapp, while the PR body snapshot says channel, channel:whatsapp, and risk: medium. Align the label snapshot and risk label outside the author-facing code blocker.
4ae15b9 to
fc81b8e
Compare
Audacity88
left a comment
There was a problem hiding this comment.
Context: I re-reviewed current head fc81b8e after @tmigone's latest update. I checked the live PR state, PR body, top-level comments, formal reviews, inline comments, the current diff, the current whatsapp_web.rs source at the PR head, and the green GitHub checks. I did not run local cargo for this pass.
✅ Resolved — the passthrough helper signature is now clean
The previous blocker is addressed. compute_reply_target() now takes only chat_jid, the call site passes only &chat, and the helper tests now assert the LID DM, phone-number DM, and group passthrough cases without carrying the stale _is_lid, _is_group, or _mapped_phone parameters.
That clears my prior code-hygiene request.
🔴 Blocking — the later event path can still convert LID DMs before ChannelMessage
One more issue surfaced while checking the final source, and it changes the merge-readiness call. The PR body, helper comment, and ChannelMessage comment now say ZeroClaw passes the chat JID through unchanged and lets whatsapp-rust handle LID addressing internally. But the event path still has a later non-group LID block after let mut reply_target = Self::compute_reply_target(&chat);:
- when
reply_targetis a LID JID, it builds phone candidates; - calls
Self::resolve_deliverable_reply_target(&reply_target, &lid_candidates); - and, when
convertedis true, assignsreply_target = resolvedbefore building theChannelMessage.
That means a LID DM with a known phone candidate can still be turned into digits@s.whatsapp.net before the reply target reaches the outbound path. That preserves the old local LID-to-phone behavior for the successful-lookup case, while the public scope says the fix is to stop doing that and keep the LID namespace consistent for whatsapp-rust 0.6+.
Please make one of these true before merge:
- if the intended fix is true LID passthrough, remove or bypass that later deliverable-target conversion for WhatsApp Web reply targets, then update the
resolve_deliverable_reply_target_*tests/log path so they no longer describe the reply target contract; or - if that later conversion is still intentionally required for a different path, update the PR title/body/helper comment/
ChannelMessagecomment/tests to describe the actual two-stage behavior, and add validation showing the later conversion does not recreate the ACK 400 LID DM scenario.
I am keeping this blocking because the code and merge record currently disagree on the central behavior being fixed. If upstream #636 needs a consistent LID namespace, this later conversion is exactly the path that could still reintroduce the mixed-namespace failure whenever lookup succeeds.
🟢 What looks good — the dependency roll-forward record is now clear
The broader whatsapp-rust family roll-forward is now visible in the PR body and rollback section, and the important risk/scope/type/size label snapshot now matches the live medium-risk channel/dependency shape. Once the reply-target contract is made consistent, the public record side of my earlier review is in good shape.
…0.6+ Signed-off-by: Tomás Migone <tomasmigone@gmail.com>
Signed-off-by: Tomás Migone <tomasmigone@gmail.com>
…les it internally Signed-off-by: Tomás Migone <tomasmigone@gmail.com>
e08e27c to
85df15d
Compare
|
@Audacity88 latest feedback was addressed, ready for review. Worth noting that since last time a fix was merged to master with #7008 that's partially made obsolete with this PR, so I reverted some of it. |
Audacity88
left a comment
There was a problem hiding this comment.
Context: I re-reviewed current head 85df15d after @tmigone's latest update. I checked the live PR state, PR body, top-level comments/reviews, the current diff and source for Cargo.lock, crates/zeroclaw-channels/Cargo.toml, and crates/zeroclaw-channels/src/whatsapp_web.rs, plus the visible GitHub checks. I did not rerun local cargo or a live WhatsApp smoke test for this pass.
✅ Resolved — the event path now preserves LID reply targets
The blocker from my last review is addressed. The later non-group LID conversion block has been removed, so the inbound event path now computes reply_target from Self::compute_reply_target(&chat) and sends that value into ChannelMessage without the old resolve_deliverable_reply_target() / lookup-based LID-to-phone rewrite.
That makes the code match the PR's stated contract: reply to the originating chat JID unchanged and let whatsapp-rust 0.6+ handle LID addressing internally. The updated helper tests also pin passthrough for LID DMs, phone-number DMs, and group chats.
✅ Resolved — outbound send and typing paths are passthrough too
The outbound recipient helper is now a synchronous trim-and-pass-through function, and send(), start_typing(), and stop_typing() all use that result before converting to a WhatsApp JID. That removes the previous client-cache LID-to-phone conversion path from ordinary outbound replies and typing state updates.
🟢 What looks good — scope and validation record now line up
The PR body now reflects the broader whatsapp-rust family roll-forward, the rollback note, and the medium-risk channel/dependency shape. All visible checks on the current head are green.
Approving this now. My approval is based on source review plus visible CI and the author's reported manual LID/phone/self-chat validation; I did not independently validate against a live WhatsApp account.
|
@Audacity88 just updated brtanch with latest master, ci passes but I guess you have to merge the PR because I don't see the button 😄 |
Summary
Base branch:
master(all contributions)What changed and why:
compute_reply_targetto convert LID JIDs to phone JIDs because LIDs couldn't receive messages directly.whatsapp-rust0.6+ which handles LID→PN conversion internally and expects consistent namespace throughout the message stanza. The previous workaround now causes ACK 400 errors due to namespace mismatch.compute_reply_targetto a passthroughwhatsapp-rustdependency family from9734fb2tocbcdd2a(6 upstream commits), which includes:resolve_deliverable_reply_target) in the inbound event handler. This is unnecessary after thewhatsapp-rustdependency bump and conflicts with the library's internal namespace handling so it has been removed.Scope boundary: Outbound and inbound reply routing + upstream dependency roll-forward. LID JIDs are now passed through unchanged at all stages; the library handles namespace consistency internally.
Blast radius: WhatsApp Web channel (
whatsapp_web.rs) and thewhatsapp-rustfamily crates (whatsapp-rust,wacore,wacore-binary,waproto, transport/http-client). The upstream changes touch core protocol, client, prekeys, and send paths, all within the WhatsApp Web feature gate.Linked issue(s):
Labels:
dependencies,channel,channel:whatsapp,bug,risk: medium,size: SValidation Evidence (required)
Local validation is the signal CI cannot replace. Run the full battery and paste literal output (tails, failures, warnings — not "all passed").
Commands run and tail output:
Click to toggle command line validation output
tomi@vesta:zeroclaw✗ cargo fmt --all -- --check
tomi@vesta:zeroclaw✗ cargo clippy --all-targets -- -D warnings
[all pass no warnings]
tomi@vesta:zeroclaw✗ cargo test --package zeroclaw-channels --features whatsapp-web
running 3 tests
test whatsapp_web::tests::compute_reply_target_preserves_group ... ok
test whatsapp_web::tests::compute_reply_target_preserves_lid_dm ... ok
test whatsapp_web::tests::compute_reply_target_preserves_pn_dm ... ok
xxxxx@lid). Message delivered successfully, ACK returned without error 400.xxxxx@s.whatsapp.net). Message delivered successfully.self_chat_mode=true.Security & Privacy Impact (required)
Yes/No for each. Answer any
Yeswith a 1–2 sentence explanation.NoNoNoNoYes, describe the risk and mitigation:Compatibility (required)
YesNoNoorYesto either: exact upgrade steps for existing users:Rollback (required for
risk: mediumandrisk: high)git revert <sha>to restore previouswhatsapp-rustpin at9734fb2and the oldcompute_reply_targetconversion logic. Alternatively, manually editCargo.tomlto pin back to9734fb2and restore the LID→PN conversion incompute_reply_target.whatsapp-web). Disabling the feature excludes all affected code paths.Supersede Attribution (required only when
Supersedes #is used)#<pr> by @<author>, one per line): -Co-authored-bytrailers added in commit messages for incorporated contributors? -No, why (inspiration-only, no direct code/design carry-over): -