fix: route pn sends to mapped lid#2692
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
📝 WalkthroughWalkthroughAdds cached PN-to-LID lookup helpers in the signal mapping store and uses them to rewrite outbound PN/hosted-PN sends to LID addressing, with tests covering store lookup behavior and message send resolution. ChangesPN to LID Resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/__tests__/Signal/lid-mapping.test.ts (1)
25-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a hosted-PN stored lookup case.
The source helper has a new
decoded.server === 'hosted'branch, but these tests only cover@s.whatsapp.net. Add one assertion for12345@hosted -> 98765@hosted.lidso the local-only hosted path is locked down.🤖 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/__tests__/Signal/lid-mapping.test.ts` around lines 25 - 56, The getStoredLIDForPN tests in lid-mapping.test.ts only cover `@s.whatsapp.net` inputs, so add a case for the new hosted-PN branch in lidMappingStore.getStoredLIDForPN. Create an assertion for a stored mapping like 12345@hosted returning 98765@hosted.lid, and verify it still uses the local lid-mapping lookup path without calling mockPnToLIDFunc.src/__tests__/Socket/messages-send.test.ts (1)
1-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSolid unit coverage for
resolveMessageSendJid; wiring intosendMessageremains untested.All five branches (mapped PN, unmapped PN, hosted PN→hosted LID, non-PN passthrough, stored-value-not-a-LID) are covered and match the implementation. However, the actual wiring in
sendMessage(settingfullMsg.key.remoteJidAlt/addressingMode, callinggenerateWAMessage/relayMessagewith the resolved LID) has no integration test mocking binary nodes/relayMessage. As per path instructions, "New protocol paths or stanza handlers should have integration tests mocking binary nodes, not e2e tests," and this PR changes the outbound stanza addressing path.Want me to draft an integration test stubbing
signalRepository.lidMapping.getStoredLIDForPNand asserting the relayed JID/attrs for a mapped-PN send?🤖 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/__tests__/Socket/messages-send.test.ts` around lines 1 - 65, The unit tests for resolveMessageSendJid are fine, but the outbound sendMessage wiring that uses its result is still untested. Add an integration-style test around sendMessage that mocks generateWAMessage and relayMessage plus signalRepository.lidMapping.getStoredLIDForPN, then assert the resolved LID is used for the relayed stanza and that fullMsg.key.remoteJidAlt/addressingMode and the addressing attributes are set correctly for a mapped PN send.Source: Path instructions
src/Socket/messages-send.ts (1)
80-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing logger on an async, non-obvious protocol resolution path.
resolveMessageSendJidcrosses an async boundary and makes a protocol-relevant decision (rewriting outbound addressing PN→LID), but takes no logger, unlike similar resolution helpers referenced in this same file (e.g. thelogger.debug(...)calls around LID/PN identity selection at lines 852-857). Adding a debug log when a PN send is rewritten to LID (and why) would aid diagnosing addressing issues without needing production repro. As per coding guidelines, "Every code path that crosses an async boundary should accept alogger: ILogger(pino-compatible)."🤖 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/Socket/messages-send.ts` around lines 80 - 104, resolveMessageSendJid currently rewrites PN to LID across an async boundary without any logging, which makes this protocol decision hard to trace. Update resolveMessageSendJid to accept a logger: ILogger argument like the other helpers in messages-send.ts, and add a debug log when a PN/hosted PN JID is resolved to a LID (including the original jid and the reason it was rewritten or left unchanged). Keep the logging consistent with the logger.debug usage elsewhere in the file and thread the logger through any call sites.Source: Coding guidelines
🤖 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/__tests__/Signal/lid-mapping.test.ts`:
- Around line 27-29: Remove the local TypeScript suppressions in the
`lid-mapping.test.ts` mocks and type the returned values as a keyed record
instead of casting to `SignalDataTypeMap['lid-mapping']`, since that type is
just `string`. Update the `mockKeys.get.mockResolvedValue(...)` call near the
top of the test and apply the same cleanup to the matching `mockKeys.get` mocks
below so the test data matches the actual shape without `@ts-ignore`.
In `@src/Signal/lid-mapping.ts`:
- Around line 35-44: The LID JID builder in lid-mapping should validate the
stored lidUser value more strictly before returning a JID, since
lidUser.toString() only guards against empty strings and can still emit
malformed values. Update the validation in the method that constructs the JID so
it rejects invalid cached/stored user values before composing the final string,
and keep the warning in structured logging form with the object first and
message last in the this.logger.warn call.
---
Nitpick comments:
In `@src/__tests__/Signal/lid-mapping.test.ts`:
- Around line 25-56: The getStoredLIDForPN tests in lid-mapping.test.ts only
cover `@s.whatsapp.net` inputs, so add a case for the new hosted-PN branch in
lidMappingStore.getStoredLIDForPN. Create an assertion for a stored mapping like
12345@hosted returning 98765@hosted.lid, and verify it still uses the local
lid-mapping lookup path without calling mockPnToLIDFunc.
In `@src/__tests__/Socket/messages-send.test.ts`:
- Around line 1-65: The unit tests for resolveMessageSendJid are fine, but the
outbound sendMessage wiring that uses its result is still untested. Add an
integration-style test around sendMessage that mocks generateWAMessage and
relayMessage plus signalRepository.lidMapping.getStoredLIDForPN, then assert the
resolved LID is used for the relayed stanza and that
fullMsg.key.remoteJidAlt/addressingMode and the addressing attributes are set
correctly for a mapped PN send.
In `@src/Socket/messages-send.ts`:
- Around line 80-104: resolveMessageSendJid currently rewrites PN to LID across
an async boundary without any logging, which makes this protocol decision hard
to trace. Update resolveMessageSendJid to accept a logger: ILogger argument like
the other helpers in messages-send.ts, and add a debug log when a PN/hosted PN
JID is resolved to a LID (including the original jid and the reason it was
rewritten or left unchanged). Keep the logging consistent with the logger.debug
usage elsewhere in the file and thread the logger through any call sites.
🪄 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: CHILL
Plan: Pro
Run ID: 025bb378-278e-44f6-b2c1-87c1c385b6bf
📒 Files selected for processing (4)
src/Signal/lid-mapping.tssrc/Socket/messages-send.tssrc/__tests__/Signal/lid-mapping.test.tssrc/__tests__/Socket/messages-send.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Tested and working ✅ |
3 participants
Summary
sendMessagedoes not trigger implicit USync for unmapped PN contacts.Protocol / behavior context
For warm contacts with an existing PN -> LID mapping, Baileys already resolves LID sessions during
assertSessions, but the final 1:1 message stanza could still be addressed to the PN. That can produce server-side ERROR 463 for contacts that should be LID-addressed.This keeps the scope narrow: it only routes sends when a local mapping already exists. Cold/unmapped PN sends keep the existing behavior.
WA Web captured JS sanity
Ran:
./tools/wa-web-sanity.sh 'addressing_mode|recipient_pn|remoteJidAlt|participant_pn|sender_pn|peer_recipient_pn'Relevant matches from the captured WA Web bundle:
WAWeb/Send/MsgCreateFanoutStanza.jsWAWeb/Send/MsgCreateDeviceStanza.jsWAWeb/Handle/MsgParser.jsWAWeb/Send/MsgCommonApi.jsRelevant excerpt:
WA Web emits alternate PN attrs alongside LID addressing, and parses those attrs back on receive.
Live validation note
I attempted a live repro with the contribution test session. The external target checked during validation had no PN -> LID mapping locally or after a deliberate USync lookup, so it did not exercise the reported mapped-LID case. A self-send confirmed local PN -> LID mapping mechanics but did not reproduce ERROR 463, because the old PN path received
SERVER_ACK.So this PR is validated by deterministic tests and WA Web send/parser sanity, not by a confirmed live 463 repro.
Tests
yarn lint- 0 errors; existingno-explicit-anywarnings remainyarn test- 29 suites / 415 tests passedRelated to #2683
Related to #2688
Drafted with Codex, reviewed and tested by me.
Summary by cubic
Route PN-addressed 1:1 sends to the locally mapped LID when available, and include the PN as alternate addressing metadata. This prevents server 463 failures for warm contacts and avoids implicit USync on send, with added validation and cached-miss handling.
getStoredLIDForPNandresolveMessageSendJidto switch PN → LID without USync when a mapping exists.remoteJidAltand added stanza attrsaddressing_mode: 'lid'andrecipient_pn; keep device IDs and hosted vs standard domains.Written for commit 93e8ac6. Summary will update on new commits.
Summary by CodeRabbit
getStoredLIDForPNscenarios.resolveMessageSendJidcovering success, fallback, and non-PN/LID cases.