feat(core): expand protocol metadata and shared primitives - #1060
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
📝 WalkthroughWalkthroughThis PR adds shared cryptographic helpers, expands group, community, prekey, and Signal APIs, separates payload decryption from decoding, updates cache and serialization contracts, strengthens IQ and migration handling, and revises notification propagation, tests, and documentation. ChangesProtocol and API updates
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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/types/message.rs | Serializes message timestamps as Unix seconds with a serialization-only adapter that matches the type's supported contract. |
Reviews (13): Last reviewed commit: "feat(core): expose session and group pro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
🚨 Per-PR size budget exceeded (Δ stripped ≤ 64.00 KiB, Δ .text ≤ 32.00 KiB):
The Baseline: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/types/events.rs (1)
1386-1412: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider
#[serde(default)]on the newaction_index: u32.Every other new field here (
notification_id,participant_username,participant_country_code) isOption<T>, which serde defaults toNoneon a missing key automatically.action_indexis the one non-optional addition with no default — ifGroupUpdatederivesDeserializeand anything persists/replays serialized event payloads across a version bump, an older blob lacking this field will hard-fail deserialization instead of degrading gracefully.Given this crate's pre-1.0 stance on breaking API changes, this isn't a blocker, but a one-line
#[serde(default)]buys resilience for free.🛡️ Proposed fix
/// Zero-based emitted-action index within the source notification. + #[serde(default)] pub action_index: u32,Based on learnings from a prior PR review noting this crate is pre-1.0 (0.6.0) and breaking public API changes don't require a compatibility shim under the crate's versioning policy.
🤖 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/events.rs` around lines 1386 - 1412, Add #[serde(default)] to the action_index field in GroupUpdate so deserialization of older serialized payloads missing this field falls back to 0. Leave the existing optional fields and other GroupUpdate behavior unchanged.Source: Learnings
🤖 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/portable_cache.rs`:
- Around line 236-247: Retain the optimized entry_time behavior for non-expiring
caches, using Instant::ZERO instead of reading the monotonic clock. Add
production monitoring for cache hit rates and capacity-based evictions, and
expose the metrics needed to tune max_capacity across deployments.
In `@wacore/src/crypto.rs`:
- Around line 19-62: Remove the redundant documentation comments above
md5_digest, hkdf_sha256, hkdf_sha256_into, generate_curve_key_pair, and
calculate_curve_signature, or replace them with concise rationale explaining
non-obvious constraints or design decisions; do not restate behavior already
clear from the function names and signatures.
In `@wacore/src/poll.rs`:
- Around line 264-280: Enforce the required message_secret length in
decrypt_poll_vote_payload_with_secret before calling decrypt_addon, matching the
existing event-response validation and failing immediately for invalid lengths.
Reuse the established constant and ensure! pattern rather than adding a
different validation path.
---
Outside diff comments:
In `@wacore/src/types/events.rs`:
- Around line 1386-1412: Add #[serde(default)] to the action_index field in
GroupUpdate so deserialization of older serialized payloads missing this field
falls back to 0. Leave the existing optional fields and other GroupUpdate
behavior unchanged.
🪄 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: b76ee408-368b-472d-8e3d-5deb58786a2c
📒 Files selected for processing (31)
src/bot.rssrc/features/chat_actions.rssrc/features/groups.rssrc/features/mod.rssrc/handlers/notification/groups.rssrc/lib.rssrc/portable_cache.rssrc/prekeys.rssrc/send/mod.rstests/e2e/tests/groups.rswacore/src/bot_message.rswacore/src/crypto.rswacore/src/download.rswacore/src/event.rswacore/src/iq/groups.rswacore/src/lib.rswacore/src/media_retry.rswacore/src/pair.rswacore/src/pair_code.rswacore/src/poll.rswacore/src/secret_enc_addon.rswacore/src/send/group.rswacore/src/send/tests.rswacore/src/shortcake.rswacore/src/stanza/groups.rswacore/src/store/device.rswacore/src/time.rswacore/src/types/events.rswacore/src/types/message.rswacore/src/voip/mod.rswacore/src/voip/ssrc.rs
|
Review follow-up: the outside-diff suggestion for |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/client/lid_pn.rs`:
- Around line 849-862: Update migrate_signal_sessions_on_lid_discovery around
migrate_signal_sessions: retain outcome.has_state_changes() solely for deciding
whether to flush signal_cache, but return a narrower outcome flag representing
sessions actually moved into LID so decrypt retry is not triggered when only
identities are discarded.
🪄 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: e1efea18-b8a0-47ba-8606-6c2852313077
📒 Files selected for processing (19)
src/client/lid_pn.rssrc/client/sessions.rssrc/features/groups.rssrc/features/mod.rssrc/features/signal.rssrc/handlers/notification/groups.rssrc/lib.rssrc/message.rssrc/message/special.rssrc/retry.rstests/e2e/tests/groups.rswacore/src/crypto.rswacore/src/event.rswacore/src/iq/groups.rswacore/src/poll.rswacore/src/secret_enc_addon.rswacore/src/stanza/groups.rswacore/src/types/events.rswacore/src/types/message.rs
💤 Files with no reviewable changes (1)
- src/message.rs
70140c0 to
982687f
Compare
![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: 982687f65c
ℹ️ 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".
982687f to
0b32f46
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b32f463b0
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/features/signal.rs`:
- Around line 222-236: Update delete_sender_key to acquire the same sender-key
chain lock used by sender_key_distribution,
process_sender_key_distribution_cached, encrypt_group_message, and
decrypt_group_message before deleting and flushing the cache. Hold that lock
across both the deletion and flush, preserving the existing durable-return
behavior and error propagation.
🪄 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: 75227d89-1039-4427-a40a-6ce5fdf62dde
📒 Files selected for processing (19)
src/client/lid_pn.rssrc/client/sessions.rssrc/features/groups.rssrc/features/mod.rssrc/features/signal.rssrc/handlers/notification/groups.rssrc/lib.rssrc/message.rssrc/message/special.rssrc/retry.rstests/e2e/tests/groups.rswacore/src/crypto.rswacore/src/event.rswacore/src/iq/groups.rswacore/src/poll.rswacore/src/secret_enc_addon.rswacore/src/stanza/groups.rswacore/src/types/events.rswacore/src/types/message.rs
💤 Files with no reviewable changes (1)
- src/message.rs
0b32f46 to
9f95c84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/prekeys.rs (1)
773-775: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent integer overflow in the backoff loop.
Look, if a device stays offline for a day, this Fibonacci sequence is going to integer-overflow
u64and panic. We can't have our clients crashing just because they can't reach the server. Usesaturating_addso it maxes out safely.🛡️ Proposed fix
- let next = delay_a + delay_b; + let next = delay_a.saturating_add(delay_b); delay_a = delay_b; delay_b = next;🤖 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/prekeys.rs` around lines 773 - 775, Update the Fibonacci backoff calculation in the loop around delay_a and delay_b to use u64::saturating_add instead of ordinary addition, preserving the existing sequence while capping at the maximum value without panicking.
🤖 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/features/groups.rs`:
- Around line 628-669: Update the ParticipantRemovalScope::LinkedGroups branch
in apply_participant_removals to avoid clearing the entire group cache;
invalidate only the parent group identified by jid and any known linked
subgroups available through existing group-cache metadata, while preserving
persisted metadata invalidation for each affected group.
In `@src/prekeys.rs`:
- Around line 746-750: Move the tracing::instrument annotations from the outer
pre-key upload wrappers into the inner _with_count methods, and annotate the new
public upload entry points as well so both implicit and explicit count flows
retain spans. In the loop around the result match, remove the wanted-based
branching and call the unified upload method directly, preserving existing
result handling.
In `@wacore/src/secret_enc_addon.rs`:
- Around line 99-105: Replace the manual length-check branch in the
message-secret validation function with anyhow::ensure!, preserving the existing
MESSAGE_SECRET_SIZE condition and error message while removing the explicit if
and Err return.
---
Outside diff comments:
In `@src/prekeys.rs`:
- Around line 773-775: Update the Fibonacci backoff calculation in the loop
around delay_a and delay_b to use u64::saturating_add instead of ordinary
addition, preserving the existing sequence while capping at the maximum value
without panicking.
🪄 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: ab2934fc-6618-47e7-96e0-f56d0c64d26d
📒 Files selected for processing (25)
src/client/lid_pn.rssrc/client/sessions.rssrc/features/community.rssrc/features/groups.rssrc/features/mod.rssrc/features/rotate_key.rssrc/features/signal.rssrc/handlers/notification/groups.rssrc/lib.rssrc/message.rssrc/message/special.rssrc/prekeys.rssrc/retry.rstests/e2e/tests/groups.rswacore/binary/src/attrs.rswacore/src/crypto.rswacore/src/event.rswacore/src/iq/business.rswacore/src/iq/groups.rswacore/src/poll.rswacore/src/secret_enc_addon.rswacore/src/stanza/groups.rswacore/src/store/signal_cache.rswacore/src/types/events.rswacore/src/types/message.rs
💤 Files with no reviewable changes (1)
- src/message.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f95c841d0
ℹ️ 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".
9f95c84 to
8f52281
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f522817dd
ℹ️ 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".
jlucaso1
added
the
size-increase-ok
8f52281 to
fdb207c
Compare
There was a problem hiding this comment.
💡 Codex Review
whatsapp-rust/src/features/signal.rs
Lines 307 to 309 in fdb207c
Migrate legacy sessions before resolving to LID
When a PN→LID mapping is cached but the valid session still exists only in the PN namespace—the lazy-migration state explicitly supported after startup—this resolves the peer to LID and immediately asks libsignal to encrypt with a nonexistent LID session. decrypt_message, session_info, and validate_session follow the same resolution-only path, while the internal receive pipeline calls try_pn_to_lid_migration_decrypt; migrate or fall back to the PN session before performing these facade operations.
whatsapp-rust/src/features/signal.rs
Lines 462 to 465 in fdb207c
Preserve the SKDM across a failed durability gate
When the first encryption for a group creates a sender key but persist_signal_state_pre_wire fails, this returns an error without exposing the newly created SKDM while leaving the sender key in the cache. A retry then observes key_exists == true, returns skdm_bytes == None, and produces ciphertext under a key that no participant ever received, so subsequent group messages remain undecryptable until the chain is explicitly rotated or deleted. Keep the distribution pending across this failure or roll back the newly created sender-key state.
ℹ️ 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".
fdb207c to
00af6fb
Compare
✅ Action performedReview finished.
|
cabd62b to
234228e
Compare
|
Additional API review in 234228e: call rejection can now be driven from explicit signaling identifiers while preserving the outer routing peer separately from the action call creator, including companion-device signaling. The generated reachout-timelock MEX operation is also exposed as a typed query using its generated name/document constants; null data or account state now returns a payload error, matching the captured client behavior instead of silently producing an absent result. Local validation passed with 1,060 library tests (1 ignored), the focused call-control and MEX regressions, tracing-enabled Clippy with warnings denied, formatting, and diff checks. |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/keepalive.rs (1)
30-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a dedicated test for
DuplicateRequestId→FatalFailure.Every other
IqErrorvariant here has its owntest_classify_*case; this new arm should, too.🧪 Proposed test addition
+ #[test] + fn test_classify_duplicate_request_id_is_fatal() { + assert_eq!( + classify_keepalive_error(&IqError::DuplicateRequestId("dup".to_string())), + KeepaliveResult::FatalFailure, + ); + }🤖 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/keepalive.rs` around lines 30 - 47, Add a dedicated unit test for classify_keepalive_error that constructs an IqError::DuplicateRequestId value and asserts it returns KeepaliveResult::FatalFailure, matching the existing test_classify_* naming and structure.
🤖 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.
Outside diff comments:
In `@src/keepalive.rs`:
- Around line 30-47: Add a dedicated unit test for classify_keepalive_error that
constructs an IqError::DuplicateRequestId value and asserts it returns
KeepaliveResult::FatalFailure, matching the existing test_classify_* naming and
structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56236d2c-cc1c-4f3c-94d3-7d13ecd8bc09
📒 Files selected for processing (35)
src/client.rssrc/client/lid_pn.rssrc/client/lifecycle.rssrc/client/sessions.rssrc/features/community.rssrc/features/contacts.rssrc/features/groups.rssrc/features/mod.rssrc/features/rotate_key.rssrc/features/signal.rssrc/handlers/notification/groups.rssrc/keepalive.rssrc/lib.rssrc/lid_pn_cache.rssrc/message.rssrc/message/special.rssrc/prekeys.rssrc/request.rssrc/retry.rstests/e2e/tests/groups.rswacore/binary/src/attrs.rswacore/libsignal/src/protocol/local_field.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/session.rswacore/src/crypto.rswacore/src/event.rswacore/src/iq/business.rswacore/src/iq/contacts.rswacore/src/iq/groups.rswacore/src/poll.rswacore/src/secret_enc_addon.rswacore/src/stanza/groups.rswacore/src/store/signal_cache.rswacore/src/types/events.rswacore/src/types/message.rs
💤 Files with no reviewable changes (1)
- src/message.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 234228e77e
ℹ️ 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".
234228e to
4e49880
Compare
✅ Action performedReview finished.
|
|
Final review follow-up in 4e49880: disconnect cleanup now preserves response-waiter generations across drained maps, durable sender-key deletion keeps only the target chain locked during backend I/O, and DuplicateRequestId keepalive classification has dedicated coverage. The explicit call-rejection regression now inspects the full outgoing stanza and proves the routing peer remains distinct from call-creator. Validation passed with 1,156 wacore tests and 1,062 whatsapp-rust tests (one ignored in each suite), plus all-target/all-feature Clippy with warnings denied, formatting, and diff checks. |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
1 participant
Summary
Why
The group parser previously discarded protocol metadata already present on the wire, used a non-canonical participant alias in group-info responses, and collapsed absent, empty, and explicit-zero states. Community and participant mutations also omitted useful response data and legacy envelopes still used by compatible servers.
Session and sender-key operations were spread across internal call paths. The shared facade now reuses the existing stores, locks, cache, and durability gates. Supplied bundles resolve to the active identifier namespace, failed migration flushes remain retryable, and sender-key deletion is serialized and durable without flushing unrelated cached state.
Pre-key lifecycle operations now share the established single-flight and retry behavior. Explicit refresh counts remain stable across retries, are protocol-bounded, and do not mutate the client's normal replenishment policy.
Cryptographic extraction and expansion setup was repeated across several features. A shared API keeps construction consistent, lets fixed-size callers write directly into final buffers, validates secret sizes before ciphertext processing, and rejects invalid SHA-256 expansion lengths before allocation.
API and compatibility notes
Validation