feat!: migrate from prost to buffa for protobuf codegen - #557
Merged
Conversation
Replace prost/prost-build with buffa/buffa-build (git dep from anthropics/buffa, includes type_attribute/field_attribute support from PR #44). Key changes: - waproto/build.rs: rewritten for buffa-build API with generate_views(true) - Proto field names converted from camelCase to snake_case (wire-compatible) - Option<Box<T>> → MessageField<T> for sub-message fields across all crates - Enum fields Option<i32> → Option<EnumType> (typed enums, no more as i32) - Enum variants CamelCase → SCREAMING_SNAKE_CASE (buffa preserves proto names) - Type names: Adv* → ADV*, Ai* → AI* (buffa preserves proto casing) - Proto message "Option" renamed to "PollOption" (workaround for buffa#36) - is_sender_key_distribution_only: encode-and-compare slow path for buffa's MessageField equality semantics (set-to-default == unset) - prost fully removed from workspace dependencies View types are generated (generate_views=true) but not yet used in decode paths — that will follow in a separate PR.
📝 WalkthroughWalkthroughThe workspace migrates protobuf generation and runtime handling from prost to buffa, then updates storage, protocol, app-state, message, send, and test paths to use buffa-generated types, presence-aware fields, typed enums, and the new decode/encode APIs. ChangesWorkspace tooling and generated schemas
Storage, protocol, pairing, and session internals
App-state, history sync, and poll processing
Runtime messages, send paths, and feature modules
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
The reporting_token_benchmark.rs was missed by the bulk conversion script since it's in benches/ (only compiled with --all-targets).
buffa's decode() takes &mut impl Buf, not &[u8]. Use decode_from_slice() which accepts &[u8] directly.
![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: 1d499d4b9d
ℹ️ 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".
Convert 6 hot-path signal protocol decode sites to use buffa's MessageView types instead of owned Message decode. Views borrow bytes/string fields directly from the input buffer, avoiding Vec<u8> allocations on every message decrypt. Converted sites (all in wacore/libsignal/src/protocol/protocol.rs): - SignalMessage::decode_ciphertext() — borrows ciphertext directly - SignalMessage::try_from() — borrows ratchet_key, ciphertext - PreKeySignalMessage::try_from() — borrows base_key, identity_key, message - SenderKeyMessage::decode_ciphertext() — borrows ciphertext directly - SenderKeyMessage::try_from() — borrows ciphertext - SenderKeyDistributionMessage::try_from() — borrows chain_key, signing_key decode_plaintext, decrypt_poll_vote, and decrypt_media_retry are NOT converted since they return full owned messages to the caller.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wacore/src/history_sync.rs (1)
157-175:⚠️ Potential issue | 🟡 MinorReject overflowing 10-byte varints explicitly.
read_varintaccepts a tenth byte larger than0x01, which is invalid for a protobufu64varint. That makes malformed input parse as a truncated value instead of surfacingMalformedProtobuf.Proposed fix
fn read_varint(data: &[u8]) -> Result<(u64, usize), HistorySyncError> { let mut value: u64 = 0; - let mut shift = 0u32; for (i, &byte) in data.iter().enumerate() { + if i == 10 || (i == 9 && byte > 1) { + return Err(HistorySyncError::MalformedProtobuf( + "varint overflows u64".into(), + )); + } + let shift = (i as u32) * 7; value |= ((byte & 0x7F) as u64) << shift; if byte & 0x80 == 0 { return Ok((value, i + 1)); } - shift += 7; - if shift >= 64 { - return Err(HistorySyncError::MalformedProtobuf( - "varint too long".into(), - )); - } } Err(HistorySyncError::MalformedProtobuf( "unexpected end of data in varint".into(), )) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/history_sync.rs` around lines 157 - 175, The read_varint function currently accepts a tenth byte > 0x01, allowing invalid protobuf u64 varints; update read_varint to explicitly reject a 10th byte with any bits other than the low bit set: when iterating in read_varint (function name) detect i == 9 (the 10th byte) and if (byte & 0xFE) != 0 return Err(HistorySyncError::MalformedProtobuf("varint too long".into())) so malformed inputs fail instead of being treated as truncated; keep the existing shift >= 64 check and existing error on unexpected end of data.wacore/src/proto_helpers.rs (1)
128-147:⚠️ Potential issue | 🟡 MinorUpdate
quoted_messagein the doc example to use thebuffa::MessageFieldAPI.The
quoted_messagefield usesbuffa::MessageField<Message>, notOption<Box<Message>>. The example still shows the oldSome(...)pattern and will fail if copied.📝 Proposed fix
/// let context = wa::ContextInfo { /// stanza_id: Some("original-msg-id".to_string()), /// participant: Some("sender@s.whatsapp.net".to_string()), -/// quoted_message: Some(original_msg.prepare_for_quote()), +/// quoted_message: buffa::MessageField::from_box(original_msg.prepare_for_quote()), /// ..Default::default() /// };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/proto_helpers.rs` around lines 128 - 147, The doc example uses an Option<Box<Message>> for ContextInfo.quoted_message but the field is buffa::MessageField<Message>; update the example to wrap the quoted message with buffa::MessageField::some(...) instead of Some(...). Specifically, in the example where you build ContextInfo (symbol: wa::ContextInfo and field quoted_message) replace the old quoted_message: Some(original_msg.prepare_for_quote()) with quoted_message: buffa::MessageField::some(original_msg.prepare_for_quote()) so the example compiles with the current MessageField API.src/features/status.rs (1)
47-60: 🧹 Nitpick | 🔵 TrivialConsider using a typed
FontTypeparameter for compile-time safety.The protobuf field
fontis already enum-typed (Option<FontType>), butsend_textaccepts a rawi32and converts it withbuffa::Enumeration::from_i32(). Acceptingwa::message::extended_text_message::FontTypedirectly would make invalid values impossible at compile time and eliminate the conversion overhead.Suggested refactor
pub async fn send_text( &self, text: &str, background_argb: u32, - font: i32, + font: wa::message::extended_text_message::FontType, recipients: &[Jid], options: StatusSendOptions, ) -> Result<SendResult, anyhow::Error> { let message = wa::Message { extended_text_message: buffa::MessageField::some(wa::message::ExtendedTextMessage { text: Some(text.to_string()), background_argb: Some(background_argb), - font: buffa::Enumeration::from_i32(font), + font: Some(font), ..Default::default() }), ..Default::default() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/status.rs` around lines 47 - 60, The send_text function currently takes font: i32 and converts it with buffa::Enumeration::from_i32, which allows invalid values; change the signature of send_text to accept a strongly-typed wa::message::extended_text_message::FontType instead of i32, update where send_text is called to pass the enum, and set the ExtendedTextMessage.font field using the typed enum (e.g., construct the buffa::Enumeration from the enum value rather than from_i32) so invalid font values are prevented at compile time and no runtime conversion from raw i32 is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 51-52: Replace the git branch pinning for the buffa dependencies
with an explicit commit or tag to ensure reproducible builds: update the buffa
and buffa-build entries (currently using branch = "main") to use rev =
"<commit-hash>" or tag = "<tag-name>" pointing to the known-good commit, and
ensure the chosen commit contains the required features (e.g., json) and build
settings before committing the change.
In `@src/bot.rs`:
- Line 885: The calls to .clone() on custom_version are redundant because
wa::device_props::AppVersion implements Copy; update the usages (e.g., the
with_device_props(...) invocations that pass custom_os.clone(),
custom_version.clone(), None — and the other occurrences near the same pattern)
to remove the .clone() on custom_version while keeping custom_os.clone() intact,
ensuring function/struct names like with_device_props and the variables
custom_version and custom_os are the targets of the change.
In `@tests/e2e/tests/media.rs`:
- Around line 414-423: The wait_for_event predicates currently match any media
event with the field set (e.g., the call to wait_for_event that looks for
Event::Message(m, _) if m.image_message.is_set()), which can capture
stale/unrelated media; update each predicate (the wait_for_event call that
matches Event::Message and the pattern identifiers like m.image_message,
m.video_message, m.audio_message, m.document_message) to additionally assert a
unique property from the test (for example compare image_message.caption or
document_message.filename or video_message.duration, and/or the sender or the
sent message id available on the event info) so the closure only returns true
for the exact media under test; use the Event::Message(msg, info) bindings and
check msg.<media>_message.as_option() then compare the relevant field(s)
(caption, filename, duration) and optionally info.sender or info.sent_message_id
to the known expected values; apply the same tightening to all similar blocks
(the other wait_for_event occurrences noted in the comment).
In `@tests/e2e/tests/messaging.rs`:
- Around line 93-97: The wait_for_event predicate used with client_b currently
matches any Event::Message where msg.protocol_message.is_set(), making the test
flaky; update the predicate in client_b.wait_for_event (and the similar call
around lines 100-105) to further check that the protocol_message's type equals
Type::REVOKE (e.g., match Event::Message(msg, _) if
msg.protocol_message.is_set() && msg.protocol_message.as_ref().map_or(false, |p|
p.r#type == Type::REVOKE) or equivalent), so the future only completes for the
revoke protocol event required by the assertion.
In `@wacore/src/proto_helpers.rs`:
- Around line 60-66: Replace the nested if-let blocks in the macro patterns with
let-chains to satisfy the collapsible-if rule: for the unnamed macro (pattern
with ($msg:expr, $ctx:ident, $body:block, $($field:ident),+)), update the
conditional to use an if let ... && let ... chain (i.e., if let Some(m) =
$msg.$field.as_option_mut() && let Some($ctx) = m.context_info.as_option_mut()
$body). Apply the same refactor to the other macros identified by
name—peel_wrapper!, check!, and recurse_into_wrapper!—so each nested `if let` is
collapsed into a single `if let ... && let ...` chain while preserving the
original $msg, $ctx, $body, and $field token usage.
---
Outside diff comments:
In `@src/features/status.rs`:
- Around line 47-60: The send_text function currently takes font: i32 and
converts it with buffa::Enumeration::from_i32, which allows invalid values;
change the signature of send_text to accept a strongly-typed
wa::message::extended_text_message::FontType instead of i32, update where
send_text is called to pass the enum, and set the ExtendedTextMessage.font field
using the typed enum (e.g., construct the buffa::Enumeration from the enum value
rather than from_i32) so invalid font values are prevented at compile time and
no runtime conversion from raw i32 is needed.
In `@wacore/src/history_sync.rs`:
- Around line 157-175: The read_varint function currently accepts a tenth byte >
0x01, allowing invalid protobuf u64 varints; update read_varint to explicitly
reject a 10th byte with any bits other than the low bit set: when iterating in
read_varint (function name) detect i == 9 (the 10th byte) and if (byte & 0xFE)
!= 0 return Err(HistorySyncError::MalformedProtobuf("varint too long".into()))
so malformed inputs fail instead of being treated as truncated; keep the
existing shift >= 64 check and existing error on unexpected end of data.
In `@wacore/src/proto_helpers.rs`:
- Around line 128-147: The doc example uses an Option<Box<Message>> for
ContextInfo.quoted_message but the field is buffa::MessageField<Message>; update
the example to wrap the quoted message with buffa::MessageField::some(...)
instead of Some(...). Specifically, in the example where you build ContextInfo
(symbol: wa::ContextInfo and field quoted_message) replace the old
quoted_message: Some(original_msg.prepare_for_quote()) with quoted_message:
buffa::MessageField::some(original_msg.prepare_for_quote()) so the example
compiles with the current MessageField API.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c54a7bb-cf3d-4ff2-998a-b4de7a757096
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
Cargo.tomlsrc/appstate_sync.rssrc/bot.rssrc/client.rssrc/client/sender_keys.rssrc/features/chat_actions.rssrc/features/newsletter.rssrc/features/polls.rssrc/features/profile.rssrc/features/status.rssrc/handshake.rssrc/history_sync.rssrc/main.rssrc/message.rssrc/pair.rssrc/pdo.rssrc/prekeys.rssrc/retry.rssrc/send.rssrc/store/signal.rstests/e2e/Cargo.tomltests/e2e/tests/media.rstests/e2e/tests/memory_soak.rstests/e2e/tests/messaging.rstests/e2e/tests/newsletter.rswacore/Cargo.tomlwacore/appstate/Cargo.tomlwacore/appstate/src/decode.rswacore/appstate/src/encode.rswacore/appstate/src/hash.rswacore/appstate/src/patch_decode.rswacore/appstate/src/processor.rswacore/benches/reporting_token_benchmark.rswacore/benches/send_receive_benchmark.rswacore/libsignal/Cargo.tomlwacore/libsignal/src/protocol/identity_key.rswacore/libsignal/src/protocol/protocol.rswacore/libsignal/src/protocol/ratchet/keys.rswacore/libsignal/src/protocol/sender_keys.rswacore/libsignal/src/protocol/state/prekey.rswacore/libsignal/src/protocol/state/session.rswacore/libsignal/src/protocol/state/signed_prekey.rswacore/libsignal/src/store/record_helpers.rswacore/noise/Cargo.tomlwacore/noise/src/handshake.rswacore/src/adv.rswacore/src/appstate_sync.rswacore/src/history_sync.rswacore/src/iq/usync.rswacore/src/media_retry.rswacore/src/message_processing.rswacore/src/messages.rswacore/src/pair.rswacore/src/poll.rswacore/src/proto_helpers.rswacore/src/reporting_token.rswacore/src/send.rswacore/src/sticker_pack.rswacore/src/store/commands.rswacore/src/store/device.rswacore/src/types/events.rswacore/src/usync.rswacore/tests/appstate_external_mutations_test.rswacore/tests/appstate_mac_test.rswaproto/Cargo.tomlwaproto/build.rswaproto/src/lib.rswaproto/src/whatsapp.protowaproto/src/whatsapp.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/benches/send_receive_benchmark.rs`:
- Around line 362-363: Replace the two usages of .unwrap() on
wa::Message::decode_from_slice(...) in the benchmark fixture with .expect(...)
that includes fixture context; locate the calls in send_receive_benchmark.rs
(the decode_from_slice(unpadded) calls inside the benchmark closures) and change
them to .expect("failed to decode fixture message in send_receive_benchmark") or
similar descriptive message so failures show which fixture/bench failed.
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 768-849: The manual buffa::Message implementation for
DecryptionErrorMessageProto (compute_size, write_to, merge_field, clear) is a
temporary workaround that increases maintenance burden; add a concise TODO
comment above the DecryptionErrorMessageProto type noting that this is
hand-written due to the upstream buffa#36 issue and include a link/identifier to
that issue and an action item to replace these methods with generated code when
the upstream bug is resolved so future schema changes are not missed.
- Around line 179-181: The .map call uses a redundant closure; replace .map(|v|
Box::from(v)) with .map(Box::from) so the code directly passes the function
pointer; update the expression using view.ciphertext and
SignalProtocolError::InvalidProtobufEncoding to return Box::from without the
unnecessary |v| lambda.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25c056f6-0ac3-44e6-9b0a-bf5c710ceeb7
📒 Files selected for processing (3)
wacore/benches/reporting_token_benchmark.rswacore/benches/send_receive_benchmark.rswacore/libsignal/src/protocol/protocol.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wacore/libsignal/src/protocol/protocol.rs (1)
768-849: 🛠️ Refactor suggestion | 🟠 MajorKeep this protobuf codec behind
waproto(or at least guard it with a golden test).This hand-written
buffa::Messagemakesprotocol.rsthe source of truth for field tags and wire types. If this workaround has to stay here, please add a focused round-trip/golden-byte test in the same PR so future schema changes do not drift silently.Based on learnings: "Use
waprotoexclusively for Protobuf definitions compiled via prost without feature logic"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/protocol.rs` around lines 768 - 849, The hand-written protobuf codec (DecryptionErrorMessageProto implementing buffa::Message in protocol.rs) must be guarded so it cannot silently drift; either wrap the entire DecryptionErrorMessageProto definition and its impls with a cfg(feature = "waproto") (or cfg_attr) gate, or add a focused golden/round-trip test that encodes and decodes DecryptionErrorMessageProto and asserts the produced bytes match a checked-in canonical byte sequence; update module exports accordingly so consumers only see this implementation when the waproto gate is enabled and add the golden test next to the proto code to ensure future schema changes are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 776-782: Add a short SAFETY: comment above the unsafe impl for
buffa::DefaultInstance for DecryptionErrorMessageProto describing the invariant
that justifies the unsafe block — specifically state that the static VALUE
(buffa::__private::OnceBox) is only ever initialized once with
Box::new(DecryptionErrorMessageProto::default()) and thus returns a valid
&'static DecryptionErrorMessageProto for the lifetime of the program; reference
the static VALUE, the use of OnceBox::get_or_init and
Box::new(DecryptionErrorMessageProto::default()) in the comment so future
maintainers understand why returning a &'static reference is sound.
---
Duplicate comments:
In `@wacore/libsignal/src/protocol/protocol.rs`:
- Around line 768-849: The hand-written protobuf codec
(DecryptionErrorMessageProto implementing buffa::Message in protocol.rs) must be
guarded so it cannot silently drift; either wrap the entire
DecryptionErrorMessageProto definition and its impls with a cfg(feature =
"waproto") (or cfg_attr) gate, or add a focused golden/round-trip test that
encodes and decodes DecryptionErrorMessageProto and asserts the produced bytes
match a checked-in canonical byte sequence; update module exports accordingly so
consumers only see this implementation when the waproto gate is enabled and add
the golden test next to the proto code to ensure future schema changes are
detected.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5130512-3567-47e7-b1d6-93ca2faba315
📒 Files selected for processing (1)
wacore/libsignal/src/protocol/protocol.rs
serialize_into was calling compute_size() in the length-sum pass and then again inside encode_len_delimited for each session. Since compute_size() walks the entire message tree recursively (all receiver chains, message keys, etc.), this doubled the work. Fix: call compute_size() once in the length pass (which caches), then use cached_size() in the write pass via write_to(). This was the root cause of the +20.5% regression in bench_decrypt_with_previous_session and +12.7% in bench_group_send_256.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcc25bfe4d
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wacore/libsignal/src/protocol/state/session.rs (3)
383-407:⚠️ Potential issue | 🟠 MajorDo not fabricate a sender chain with empty ratchet keys.
The
Nonebranch materializessender_chainwith empty public/private ratchet-key bytes. After that, the state looks “set”, butsender_ratchet_key()/sender_ratchet_private_key()can fail whilehas_usable_sender_chain()still returnstrue. Please surface this as an error instead of creating an invalid session state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 383 - 407, The current set_sender_chain function must not fabricate a sender_chain with empty ratchet keys; instead detect when self.session.sender_chain is None and return/signpost an error so an invalid session state is not created. Change set_sender_chain (and its signature) to propagate a Result (or return an error) when sender_chain is None, do not construct a Chain with empty sender_ratchet_key/sender_ratchet_key_private, and update callers to handle the error; keep all other logic for replacing chain_key when a real sender_chain exists. Ensure references to session.sender_chain, has_usable_sender_chain(), and sender_ratchet_key()/sender_ratchet_private_key() are consistent with the new error-returning behavior.
605-617: 🧹 Nitpick | 🔵 TrivialThe load-time truncation still pays full decode cost.
decode_from_slice()fully owns everyprevious_sessionsentry beforetruncate()runs, so oversized records still incur the full parse/allocation cost. Since buffa’s view API is designed for zero-copy reads, this is a good place to verify whether a view-based prepass can cap work before allocating archived sessions. (docs.rs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 605 - 617, The deserialize function currently calls RecordStructure::decode_from_slice which fully allocates all previous_sessions before we truncate; instead, use buffa's view API to pre-scan the protobuf buffer to get the number of previous_sessions without allocating (use RecordStructure view/decoder methods from buffa), and if it exceeds consts::ARCHIVED_STATES_MAX_LENGTH create a trimmed buffer/view that contains only the first ARCHIVED_STATES_MAX_LENGTH previous_sessions entries and then call the owned decoder (RecordStructure::decode_from_slice) on that trimmed slice; update the deserialize implementation (function deserialize, struct RecordStructure, field previous_sessions) to do the zero-copy length check and only decode/allocate up to ARCHIVED_STATES_MAX_LENGTH.
605-623:⚠️ Potential issue | 🟠 MajorPreserve top-level unknown fields across round-trips.
SessionRecord::deserialize()decodes intoRecordStructure(which buffa populates with__buffa_unknown_fields), then extracts onlycurrent_sessionandprevious_sessionsintoSessionRecord, discarding unknown fields.serialize_into()re-encodes only fields 1 and 2 with no mechanism to output unknown fields. A read/write cycle silently drops any unknown top-level record fields from newer schemas, violating buffa's designed round-trip fidelity for schema extensions. Either store unknown fields inSessionRecordor use buffa's view API to avoid deep copying during deserialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 605 - 623, SessionRecord::deserialize currently decodes into RecordStructure (which holds __buffa_unknown_fields) but only copies current_session and previous_sessions, dropping unknown top-level fields; update the implementation to preserve those unknown fields and emit them in serialize_into: either add a field on SessionRecord (e.g., preserved_unknown_fields or raw_record: RecordStructure) and copy RecordStructure.__buffa_unknown_fields (or the whole RecordStructure) during deserialize, then modify SessionRecord::serialize_into to include/writer-output those preserved unknown fields when encoding, or switch to buffa's view API to hold the decoded view and avoid deep-copying so unknown fields are preserved across round-trips; reference RecordStructure, SessionRecord::deserialize, SessionRecord::serialize_into, current_session, previous_sessions, and __buffa_unknown_fields when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 383-407: The current set_sender_chain function must not fabricate
a sender_chain with empty ratchet keys; instead detect when
self.session.sender_chain is None and return/signpost an error so an invalid
session state is not created. Change set_sender_chain (and its signature) to
propagate a Result (or return an error) when sender_chain is None, do not
construct a Chain with empty sender_ratchet_key/sender_ratchet_key_private, and
update callers to handle the error; keep all other logic for replacing chain_key
when a real sender_chain exists. Ensure references to session.sender_chain,
has_usable_sender_chain(), and sender_ratchet_key()/sender_ratchet_private_key()
are consistent with the new error-returning behavior.
- Around line 605-617: The deserialize function currently calls
RecordStructure::decode_from_slice which fully allocates all previous_sessions
before we truncate; instead, use buffa's view API to pre-scan the protobuf
buffer to get the number of previous_sessions without allocating (use
RecordStructure view/decoder methods from buffa), and if it exceeds
consts::ARCHIVED_STATES_MAX_LENGTH create a trimmed buffer/view that contains
only the first ARCHIVED_STATES_MAX_LENGTH previous_sessions entries and then
call the owned decoder (RecordStructure::decode_from_slice) on that trimmed
slice; update the deserialize implementation (function deserialize, struct
RecordStructure, field previous_sessions) to do the zero-copy length check and
only decode/allocate up to ARCHIVED_STATES_MAX_LENGTH.
- Around line 605-623: SessionRecord::deserialize currently decodes into
RecordStructure (which holds __buffa_unknown_fields) but only copies
current_session and previous_sessions, dropping unknown top-level fields; update
the implementation to preserve those unknown fields and emit them in
serialize_into: either add a field on SessionRecord (e.g.,
preserved_unknown_fields or raw_record: RecordStructure) and copy
RecordStructure.__buffa_unknown_fields (or the whole RecordStructure) during
deserialize, then modify SessionRecord::serialize_into to include/writer-output
those preserved unknown fields when encoding, or switch to buffa's view API to
hold the decoded view and avoid deep-copying so unknown fields are preserved
across round-trips; reference RecordStructure, SessionRecord::deserialize,
SessionRecord::serialize_into, current_session, previous_sessions, and
__buffa_unknown_fields when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b9860c8-72c3-4ffb-b2d5-a22eea141912
📒 Files selected for processing (1)
wacore/libsignal/src/protocol/state/session.rs
Callgrind profiling revealed two sources of regression: 1. Every buffa struct carried __buffa_unknown_fields (a Vec internally), adding allocation/drop overhead to deeply nested types like SessionStructure. Since we control both encoder and decoder, unknown field preservation is unnecessary. Disabling it via preserve_unknown_fields(false) removes the field entirely. 2. serialize_into called compute_size() twice per session (once for length calculation, once inside write_to). Fixed to use cached_size() on the second pass (previous commit). Benchmark impact (callgrind instruction counts vs main): - bench_decrypt_with_previous_session: +29.4% → +16.5% - bench_message_key_eviction: +2.2% → +0.4% - bench_promote_matching_session: -4.7% → -10.2% (even faster) - bench_dm_recv: -17.9% (view decode win) The remaining +16.5% in decrypt_previous_session comes from CachedSize (AtomicU32) adding 4 bytes per struct, which pushes some Box<T> allocations across size class boundaries. This is inherent to buffa's cached-size design. DHAT heap profile (200-round DM soak): -29.6% total allocations vs prost baseline (53.2MB → 37.5MB).
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 (2)
wacore/libsignal/src/protocol/state/session.rs (2)
604-621: 🧹 Nitpick | 🔵 TrivialPost-truncate pruning still pays the full decode/allocation cost.
RecordStructure::decode_from_slice(bytes)materializes everyprevious_sessionsentry beforetruncate()drops the tail, so oversized records still incur the work this optimization is trying to avoid. Since this PR enables buffa view generation, this is the first place I’d revisit: parseprevious_sessionsthrough a borrowed view or a length-delimited scan and stop onceARCHIVED_STATES_MAX_LENGTHis reached instead of allocating sessions you immediately discard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 604 - 621, The current deserialize implementation calls RecordStructure::decode_from_slice(bytes) which fully materializes all previous_sessions entries before you truncate, defeating the optimization; change deserialize (the function) to parse the protobuf in a streaming/borrowed manner so you stop reading repeated previous_sessions after consts::ARCHIVED_STATES_MAX_LENGTH entries instead of decoding them all: replace the direct use of RecordStructure::decode_from_slice with a length-delimited or streaming parser (or manual scan of the bytes/message fields) that decodes current_session and then iterates the previous_sessions field, collecting at most ARCHIVED_STATES_MAX_LENGTH into record.previous_sessions (or building previous_sessions directly for the Session struct), so oversized records never allocate/deserialise discarded sessions. Ensure you still map current_session via current_session.into_option().map(|s| s.into()) and wrap previous_sessions in Arc as before.
291-345: 🧹 Nitpick | 🔵 TrivialExtract shared protobuf chain builders before these paths drift.
The buffa migration duplicated the same
session_structure::chain::ChainKey/MessageField::some(...)construction across four setters. One future schema tweak missed in a single branch will skew sender vs. receiver behavior. Pull the protobuf construction into small helpers and reuse them here.♻️ Suggested extraction
+fn pb_chain_key(chain_key: &ChainKey) -> session_structure::chain::ChainKey { + session_structure::chain::ChainKey { + index: Some(chain_key.index()), + key: Some(bytes::Bytes::copy_from_slice(chain_key.key())), + ..Default::default() + } +}Also applies to: 383-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 291 - 345, The constructors for session_structure::chain::ChainKey and wrapping with MessageField::some are duplicated in add_receiver_chain, with_receiver_chain, set_sender_chain (and other setters around 383-484); extract small helper functions (e.g., a private fn build_chain_key(index: u32, key: &[u8]) -> session_structure::chain::ChainKey and a fn build_chain(sender_key: Vec<u8>, sender_priv: Vec<u8>, chain_key: &ChainKey) -> session_structure::Chain) and replace the inlined constructions in add_receiver_chain, set_sender_chain and related setters to call these helpers so protobuf construction is centralized and shared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 791-829: The helper write_len_delimited currently calls
msg.cached_size() which relies on the caller having called compute_size()
earlier; change write_len_delimited to take the precomputed msg_len (usize or
u64) as an additional parameter and use that length for encode_varint and
reserve logic instead of calling cached_size(); compute each message's size with
compute_size() where current_len and previous_len are calculated (for
current_session use state.session.compute_size(), for previous_sessions use
s.compute_size()) and pass those values into write_len_delimited when writing
the buffer so the length prefix is always correct and no implicit cached_size()
dependency remains.
---
Outside diff comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 604-621: The current deserialize implementation calls
RecordStructure::decode_from_slice(bytes) which fully materializes all
previous_sessions entries before you truncate, defeating the optimization;
change deserialize (the function) to parse the protobuf in a streaming/borrowed
manner so you stop reading repeated previous_sessions after
consts::ARCHIVED_STATES_MAX_LENGTH entries instead of decoding them all: replace
the direct use of RecordStructure::decode_from_slice with a length-delimited or
streaming parser (or manual scan of the bytes/message fields) that decodes
current_session and then iterates the previous_sessions field, collecting at
most ARCHIVED_STATES_MAX_LENGTH into record.previous_sessions (or building
previous_sessions directly for the Session struct), so oversized records never
allocate/deserialise discarded sessions. Ensure you still map current_session
via current_session.into_option().map(|s| s.into()) and wrap previous_sessions
in Arc as before.
- Around line 291-345: The constructors for session_structure::chain::ChainKey
and wrapping with MessageField::some are duplicated in add_receiver_chain,
with_receiver_chain, set_sender_chain (and other setters around 383-484);
extract small helper functions (e.g., a private fn build_chain_key(index: u32,
key: &[u8]) -> session_structure::chain::ChainKey and a fn
build_chain(sender_key: Vec<u8>, sender_priv: Vec<u8>, chain_key: &ChainKey) ->
session_structure::Chain) and replace the inlined constructions in
add_receiver_chain, set_sender_chain and related setters to call these helpers
so protobuf construction is centralized and shared.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0b3aced-d5a1-4100-8867-4eeabd0483bb
📒 Files selected for processing (3)
wacore/libsignal/src/protocol/state/session.rswaproto/build.rswaproto/src/whatsapp.rs
Two changes: 1. SessionRecord::deserialize now decodes to RecordStructureView first (zero-copy), then converts only the sessions we keep to owned types via to_owned_message(). Excess previous_sessions beyond ARCHIVED_STATES_MAX_LENGTH are never fully allocated. 2. Add clippy::needless_else to waproto allow list — buffa codegen emits empty else branches in some decode paths.
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/libsignal/src/protocol/state/session.rs (1)
1250-1294: 🧹 Nitpick | 🔵 TrivialConsider adding a backward compatibility test with prost-encoded data.
The existing roundtrip tests verify that buffa can read its own output, but don't verify that buffa can read existing prost-encoded session data. Consider adding a test with a known prost-encoded session blob to ensure the migration doesn't break existing persisted sessions.
♻️ Suggested test approach
#[test] fn test_deserialize_prost_encoded_session() { // Capture a known-good session serialized with prost before migration // (base64 or hex-encoded constant) const PROST_ENCODED_SESSION: &[u8] = &[/* bytes from prost serialization */]; // Verify buffa can deserialize it let record = SessionRecord::deserialize(PROST_ENCODED_SESSION) .expect("Should deserialize prost-encoded session"); // Verify key fields are readable assert!(record.session_state().is_some()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 1250 - 1294, Add a backward-compatibility unit test that verifies SessionRecord::deserialize can read a known prost-encoded session blob: create a new test (similar location to test_session_record_serialization_preserves_previous_sessions and test_session_record_truncates_on_deserialize) with a constant byte slice containing a prost-serialized session, call SessionRecord::deserialize(PROST_ENCODED_BYTES).expect("..."), and assert key fields are readable (e.g., record.session_state().is_some(), record.previous_session_count() matches expected, and inspect a known alice_base_key via previous_session_states() or session_state()) so the migration is validated against existing prost data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@waproto/build.rs`:
- Around line 101-107: The comment about fixing as_option_mut() being called on
MessageFieldView is stale and misleading because that workaround is not
implemented in build.rs; either remove the sentence mentioning
as_option_mut()/MessageFieldView from the post-process comment block or
implement the actual replacement logic here to rewrite calls to as_option_mut()
into always-set semantics; update the comment to match the code path that
injects #[serde(skip)] for owned types (the existing buffa post-process logic)
so the text accurately references only the serde skip injection and not
as_option_mut().
- Around line 110-118: Remove the dead .replace(...) that targets "pub
__buffa_unknown_fields: ::buffa::UnknownFields," since
preserve_unknown_fields(false) prevents that field from being generated, making
the replacement a no-op; update the surrounding comment that currently claims
"buffa adds __buffa_unknown_fields... to every struct" to accurately state that
the field is only added when preserve_unknown_fields(true) (or remove the claim
entirely), and leave the remaining replacement for "__buffa_cached_size" intact.
- Around line 29-99: The build config currently calls unsupported methods
(.type_attribute, .message_attribute, .field_attribute) on buffa_build::Config
(see the chain starting at buffa_build::Config::new()), which will fail with
buffa-build 0.3.0; remove all .type_attribute(...), .message_attribute(...), and
.field_attribute(...) calls and their serde/skip strings, keep
.use_bytes_type_in(...), .preserve_unknown_fields(false), .generate_views(true),
.out_dir("src/") and .compile(), and enable serde derives via
.generate_json(true) instead of injecting attributes manually.
---
Outside diff comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 1250-1294: Add a backward-compatibility unit test that verifies
SessionRecord::deserialize can read a known prost-encoded session blob: create a
new test (similar location to
test_session_record_serialization_preserves_previous_sessions and
test_session_record_truncates_on_deserialize) with a constant byte slice
containing a prost-serialized session, call
SessionRecord::deserialize(PROST_ENCODED_BYTES).expect("..."), and assert key
fields are readable (e.g., record.session_state().is_some(),
record.previous_session_count() matches expected, and inspect a known
alice_base_key via previous_session_states() or session_state()) so the
migration is validated against existing prost data.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f79e6f2-0b3d-40f4-9614-1f75f979f5df
📒 Files selected for processing (3)
wacore/libsignal/src/protocol/state/session.rswaproto/build.rswaproto/src/whatsapp.rs
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 (3)
wacore/libsignal/src/protocol/state/session.rs (3)
394-409:⚠️ Potential issue | 🟠 MajorDon’t synthesize a sender chain with empty ratchet keys.
When
sender_chainis absent, this fallback persistsSome(vec![])placeholders and then marks the chain as present. That makeshas_usable_sender_chain()returntrue, whilesender_ratchet_key()/sender_ratchet_private_key()will fail later on the same record. Return an error here, or require callers to initialize the full chain viaset_sender_chain()first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 394 - 409, The code currently fabricates a sender_chain with empty sender_ratchet_key/sender_ratchet_key_private when self.session.sender_chain is None, which causes has_usable_sender_chain() to lie; instead, change this branch in the function that sets chain_key so that it does not create placeholder keys — return an error (e.g., Err(SessionError::MissingSenderChain)) or require callers to call set_sender_chain() first; update the function signature to return a Result and propagate the error to callers (or assert/early-return) and document that callers must initialize via set_sender_chain() so sender_ratchet_key() / sender_ratchet_private_key() remain safe.
17-18:⚠️ Potential issue | 🟠 MajorDrop the stale
RecordStructureimport.Clippy is already failing on this unused import, so the PR stays red until it is removed.
♻️ Proposed fix
-use crate::protocol::stores::{RecordStructure, SessionStructure}; +use crate::protocol::stores::SessionStructure;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 17 - 18, Remove the unused RecordStructure import from the top of session.rs: keep the existing session_structure and SessionStructure imports but delete the redundant RecordStructure symbol from the use statement so the file no longer imports RecordStructure (this fixes the Clippy unused-import error).
1255-1299:⚠️ Potential issue | 🔴 CriticalAdd a golden fixture test for backward compatibility with pre-migration (prost-era) serialized bytes.
The existing tests only verify round-trip serialization with freshly-created records. SessionRecord::deserialize() is on the production load path in wacore/src/store/signal_cache.rs, src/store/signal.rs, and wacore/src/send.rs. Recent changes moved from prost to a buffa/view-based deserialization approach (commit 302b782), and persisted session records in the old format must deserialize correctly. Add a fixture test using actual pre-migration bytes to ensure compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 1255 - 1299, Add a golden-fixture unit test that verifies SessionRecord::deserialize can read pre-migration (prost-era) serialized bytes: create a new test (e.g., test_session_record_deserializes_prost_fixture) that loads a byte array fixture containing an actual prost-era SessionRecord blob, calls SessionRecord::deserialize(&bytes).unwrap(), and asserts expected properties (previous_session_count, alice_base_key values, and any truncated length behavior). Place the fixture bytes alongside other tests (or load from a test-data file) and ensure the test references SessionRecord::deserialize and previous_session_states()/previous_session_count() so it will catch any regression from the buffa/view-based deserializer change.
♻️ Duplicate comments (1)
wacore/libsignal/src/protocol/state/session.rs (1)
799-805: 🛠️ Refactor suggestion | 🟠 MajorMake the length-prefix precondition explicit.
This still relies on an earlier
compute_size()pass having warmedcached_size(). Passing the precomputed message length intowrite_len_delimited()removes that hidden coupling and makes future edits much harder to break.Also applies to: 829-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/libsignal/src/protocol/state/session.rs` around lines 799 - 805, The helper write_len_delimited currently reads msg.cached_size() and thus implicitly depends on a prior compute_size() pass; change its signature to accept the precomputed length (e.g. write_len_delimited(field: u32, msg: &impl Message, msg_len: u32, buf: &mut Vec<u8>)) and use that msg_len when calling encode_varint instead of msg.cached_size(), then update all call sites (including the other occurrence around the second block) to pass the result of compute_size()/cached value you already computed earlier; keep Tag::new(...) and msg.write_to(buf) unchanged but remove the hidden dependency on cached_size().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 394-409: The code currently fabricates a sender_chain with empty
sender_ratchet_key/sender_ratchet_key_private when self.session.sender_chain is
None, which causes has_usable_sender_chain() to lie; instead, change this branch
in the function that sets chain_key so that it does not create placeholder keys
— return an error (e.g., Err(SessionError::MissingSenderChain)) or require
callers to call set_sender_chain() first; update the function signature to
return a Result and propagate the error to callers (or assert/early-return) and
document that callers must initialize via set_sender_chain() so
sender_ratchet_key() / sender_ratchet_private_key() remain safe.
- Around line 17-18: Remove the unused RecordStructure import from the top of
session.rs: keep the existing session_structure and SessionStructure imports but
delete the redundant RecordStructure symbol from the use statement so the file
no longer imports RecordStructure (this fixes the Clippy unused-import error).
- Around line 1255-1299: Add a golden-fixture unit test that verifies
SessionRecord::deserialize can read pre-migration (prost-era) serialized bytes:
create a new test (e.g., test_session_record_deserializes_prost_fixture) that
loads a byte array fixture containing an actual prost-era SessionRecord blob,
calls SessionRecord::deserialize(&bytes).unwrap(), and asserts expected
properties (previous_session_count, alice_base_key values, and any truncated
length behavior). Place the fixture bytes alongside other tests (or load from a
test-data file) and ensure the test references SessionRecord::deserialize and
previous_session_states()/previous_session_count() so it will catch any
regression from the buffa/view-based deserializer change.
---
Duplicate comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 799-805: The helper write_len_delimited currently reads
msg.cached_size() and thus implicitly depends on a prior compute_size() pass;
change its signature to accept the precomputed length (e.g.
write_len_delimited(field: u32, msg: &impl Message, msg_len: u32, buf: &mut
Vec<u8>)) and use that msg_len when calling encode_varint instead of
msg.cached_size(), then update all call sites (including the other occurrence
around the second block) to pass the result of compute_size()/cached value you
already computed earlier; keep Tag::new(...) and msg.write_to(buf) unchanged but
remove the hidden dependency on cached_size().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7ac8301-6869-4c1e-a5fb-8475f20ae472
📒 Files selected for processing (2)
wacore/libsignal/src/protocol/state/session.rswaproto/src/lib.rs
buffa #39 (merged on main as f5690d71) qualifies stdlib `Option` as `::core::option::Option` in nested scopes, which closes the `message Option` shadowing issue (#36) we worked around by renaming the two poll-option messages to `PollOption` during the migration. The proto file now uses the original WhatsApp name `Option` again, matching upstream WA Web exactly, and the rest of the repo follows suit: - Rename `PollOption` -> `Option` in `whatsapp.proto` (both `Message.PollCreationMessage.PollOption` and `MsgOpaqueData.PollOption`). - Rebuild with the new buffa pin; generated code compiles without the shadow-breaking `Option` resolution. - Update the single `src/features/polls.rs` construction site that referenced `wa::message::poll_creation_message::PollOption`. - Drop the stale `as_option_mut()` comment from `waproto/build.rs` (bug was already eliminated by `preserve_unknown_fields(false)`). Full suite passes (`cargo build --workspace`, `cargo clippy --workspace --all-targets --exclude e2e-tests`, `cargo test --workspace --exclude e2e-tests`), zero new warnings.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
waproto/build.rs (1)
101-116:⚠️ Potential issue | 🟠 MajorMake the post-process deterministic.
With Line 93 disabling unknown-field preservation, the
__buffa_unknown_fieldsrewrite is dead code now. More importantly, the__buffa_cached_sizepatch is still best-effort: if buffa changes the emitted spelling, generation quietly writes an unpatched file and you only find out later when serde builds break. Drop the dead branch and fail fast when the cached-size marker is missing.Suggested fix
- // Add #[serde(skip)] to buffa internal fields on owned types only. - // buffa adds __buffa_unknown_fields and __buffa_cached_size to every - // struct, and neither impls serde traits. View types don't derive - // serde, so only owned-type fields are annotated (the replace targets - // `UnknownFields`, not `UnknownFieldsView`). + // Add #[serde(skip)] to buffa's owned cached-size field. + // Unknown-field preservation is disabled above, so + // `__buffa_unknown_fields` is not generated. let path = std::path::Path::new("src/whatsapp.rs"); let content = std::fs::read_to_string(path)?; - let content = content - .replace( - "pub __buffa_unknown_fields: ::buffa::UnknownFields,", - "#[serde(skip)]\n pub __buffa_unknown_fields: ::buffa::UnknownFields,", - ) - .replace( - "pub __buffa_cached_size:", - "#[serde(skip)]\n pub __buffa_cached_size:", - ); + if !content.contains("pub __buffa_cached_size:") { + return Err(std::io::Error::other( + "buffa codegen changed: __buffa_cached_size field not found", + )); + } + let content = content.replace( + "pub __buffa_cached_size:", + "#[serde(skip)]\n pub __buffa_cached_size:", + ); std::fs::write(path, content)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waproto/build.rs` around lines 101 - 116, The post-processing currently keeps a dead replace for "__buffa_unknown_fields" and silently fails if the "__buffa_cached_size" marker isn’t present; remove the first replace (the unknown_fields rewrite) and make the cached-size handling deterministic by checking that the file content (the content variable after reading src/whatsapp.rs) contains the exact "pub __buffa_cached_size:" marker before proceeding — if the marker is missing, return an error (fail fast) rather than writing an unpatched file so the build fails clearly; update the code around the two replace calls (the content variable and its .replace usage) to implement this check and remove the dead branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 293-306: The local import use bytes::Bytes; is duplicated inside
functions (around creation of session_structure::chain::ChainKey and
session_structure::Chain where chain_key.key() and Bytes::copy_from_slice are
used); hoist this import to the module-level imports at the top of session.rs
and remove the function-local use statements (those near the code that builds
chain_key and chain with sender.serialize() and MessageField::some); ensure no
name conflicts after moving the import and run a quick build to verify all sites
(lines referenced around chain_key/index and sender_ratchet_key) still compile.
- Around line 611-612: The decode call to RecordStructureView::decode_view
currently discards the underlying buffa decode error; change the mapping so the
original error is preserved and returned with context (e.g., wrap or attach the
source error when constructing InvalidSessionError so the buffa error message is
not lost). Locate the call to RecordStructureView::decode_view in session.rs and
update the map_err closure to include the original error (or implement
InvalidSessionError to accept and store a source error) so diagnostic details
from the protobuf decode are retained.
- Around line 543-546: The destructuring of self.session that includes the
internal field __buffa_cached_size is fragile because it relies on buffa's
private field names; add a concise comment immediately above that destructuring
(the block that binds __buffa_cached_size from &self.session) explaining that
this is intentionally exhaustive to catch new fields but depends on buffa's
internal naming, cite the buffa crate and/or version used, and note that if
buffa renames internal fields this will break and should be updated or replaced
with a safer API—keep the comment short and clear for future maintainers.
In `@wacore/src/reporting_token.rs`:
- Around line 918-929: The test should keep using the buffa-recommended
direct-field access pattern rather than switching to as_option(); specifically,
leave the checks that call prepared.message_context_info.is_set() and then
access prepared.message_context_info.message_secret and
prepared.message_context_info.reporting_token_version (compared to
Some(secret.to_vec()) and Some(REPORTING_TOKEN_VERSION)) unchanged; if you want
file-wide consistency, change the other tests that use as_option().expect() to
follow this same is_set() + direct field projection pattern instead of
converting to as_option().
---
Duplicate comments:
In `@waproto/build.rs`:
- Around line 101-116: The post-processing currently keeps a dead replace for
"__buffa_unknown_fields" and silently fails if the "__buffa_cached_size" marker
isn’t present; remove the first replace (the unknown_fields rewrite) and make
the cached-size handling deterministic by checking that the file content (the
content variable after reading src/whatsapp.rs) contains the exact "pub
__buffa_cached_size:" marker before proceeding — if the marker is missing,
return an error (fail fast) rather than writing an unpatched file so the build
fails clearly; update the code around the two replace calls (the content
variable and its .replace usage) to implement this check and remove the dead
branch.
🪄 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: 906cfe30-a46b-402e-99a6-b61a833c7972
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
src/features/polls.rswacore/libsignal/src/protocol/state/session.rswacore/src/reporting_token.rswaproto/build.rswaproto/src/lib.rswaproto/src/whatsapp.protowaproto/src/whatsapp.rs
…ng whatsapp.rs
Before: a 10 MB / 232k-line `whatsapp.rs` was checked in, regenerated
only with `cargo build --features generate`. Every PR that touched the
proto dragged tens of thousands of generated-code lines into review and
every clone paid for it.
After: only the source (`whatsapp.proto`) and a 489 KB compiled
descriptor (`whatsapp.desc`) are tracked. `build.rs` always runs — reads
the descriptor via `buffa_build::Config::descriptor_set` and writes
`whatsapp.rs` into `OUT_DIR`. `lib.rs` includes the OUT_DIR artefact.
Properties
- No `protoc` required for consumers, CI, or docs.rs. Only editors of
`.proto` need it, and only when running
`scripts/regenerate-proto-desc.sh` after an edit.
- Reproducible: the committed descriptor is the exact contract; builds
from a clean tree are bit-identical given the same buffa pin.
- Incremental: `cargo:rerun-if-changed=src/whatsapp.desc` gates the
regen; a no-op rebuild after a clean build finishes in ~0.11 s.
- Delta in tracked bytes: -10 MB / +489 KB (net -95%).
Mechanics
- `waproto/build.rs` uses `.descriptor_set("src/whatsapp.desc")` and
`.out_dir(env!("OUT_DIR"))`; the `#[cfg(feature = "generate")]` split
is gone.
- `waproto/Cargo.toml` drops the `generate` feature; `buffa-build` is a
permanent `[build-dependencies]` entry (it's zero-cost when not
invoking protoc).
- `waproto/src/lib.rs` switches to
`include!(concat!(env!("OUT_DIR"), "/whatsapp.rs"))`.
- `waproto/.gitignore` blocks `src/whatsapp.rs` from being re-committed
by mistake.
- `scripts/regenerate-proto-desc.sh` wraps the one `protoc` invocation
needed to refresh the descriptor after editing the `.proto`.
Validation
- `cargo build --workspace --exclude e2e-tests` succeeds from a clean
tree.
- `cargo clippy --workspace --all-targets --exclude e2e-tests` — clean.
- `cargo test --workspace --exclude e2e-tests` — 1,152 tests pass.
- Regen script regenerates an identical 489 KB descriptor; follow-up
build produces the same generated output as the previous pin.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@waproto/build.rs`:
- Around line 83-84: The build turned on generated views with
.generate_views(true) but callers still allocate owned wa::Message on hot paths;
benchmark the suspected hotspots (e.g., the parsing flow around the function in
src/message.rs at the current parse path and the sender key handling flow around
src/client/sender_keys.rs) to confirm where parse-only code materializes owned
messages, then refactor those hot-path callsites to use the generated
*View/OwnedView APIs instead of constructing wa::Message (replace
creation/ownership paths with read-only view accessors and any OwnedView
wrappers where a lifetime-owned slice is required), update callers to use view
accessors for fields instead of consuming messages, and re-run benchmarks to
verify the perf win.
- Around line 12-13: The build currently only watches src/whatsapp.desc via the
println!("cargo:rerun-if-changed=src/whatsapp.desc") line in build.rs, so a
changed src/whatsapp.proto can be missed; update build.rs (in its main) to
detect staleness and fail the build: compare the mtimes (or compute a hash) of
src/whatsapp.proto and src/whatsapp.desc (or regenerate a descriptor and
compare) and if proto is newer or content differs, emit a panic!/eprintln! with
a clear message instructing to re-run the proto generation (i.e., fail the build
rather than silently proceed), while keeping the existing cargo:rerun-if-changed
lines. Ensure the check references the src/whatsapp.proto and src/whatsapp.desc
file paths in build.rs so the failure triggers when they are out of sync.
- Around line 92-103: The current blind text-rewrite on the generated file
(variable generated / reading via std::fs::read_to_string and creating content)
should fail fast if the expected original snippets aren't present; after reading
the file but before writing, verify that the original marker strings ("pub
__buffa_unknown_fields: ::buffa::UnknownFields," and "pub __buffa_cached_size:")
are present (or check that the replace actually changed the content by comparing
original_content != new_content or by counting occurrences), and if not return
an Err or panic with a clear message indicating the expected patterns weren't
found in whatsapp.rs so the build fails immediately rather than silently
skipping the serde(skip) injection; keep the replacement logic (the two .replace
calls) and then write only when the checks pass.
🪄 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: d1a162a4-78ef-4ea9-a2b5-3c11861b5308
📒 Files selected for processing (7)
scripts/regenerate-proto-desc.shwaproto/.gitignorewaproto/Cargo.tomlwaproto/build.rswaproto/src/lib.rswaproto/src/whatsapp.descwaproto/src/whatsapp.rs
Brings the buffa branch up to date with main (15+ commits since
branch-off on 2026-04-16, including PDO/WA-Web compliance, audit
follow-ups, LID-PN migration perf, and group-prekey zombie fixes).
Conflict resolutions
* Cargo.lock — regenerated (main changes + buffa git deps).
* wacore/libsignal/Cargo.toml — kept the branch's `buffa` dep.
* src/send.rs — adopted main's `wacore::send::status_carries_privacy_meta`
helper (richer: handles reactions + revokes + wrappers) instead of
the branch's inline revoke-only check.
Buffa-API adaptations for code that landed on main while the branch
was open
* `wacore/src/send.rs::status_carries_privacy_meta`
- `.as_ref()` → `.as_option()`, `MessageField::is_some()` call routed
via `.as_option().is_some()`.
- `protocol_message::Type::Revoke as i32` → `::REVOKE` typed variant.
* `wacore/src/send.rs::build_member_label_message`
- `Some(Box::new(...))` on message fields → `buffa::MessageField::some(...)`.
- `protocol_message::Type::GroupMemberLabelChange as i32` →
`::GROUP_MEMBER_LABEL_CHANGE`.
- `MemberLabel { ... }` literal gets `..Default::default()` to pick up
the `__buffa_cached_size` internal field.
* All `status_carries_privacy_meta` tests in `wacore/src/send.rs` ported
to the same patterns.
* `src/send.rs` status-reaction tests — `MessageKey` / `ReactionMessage`
constructions use `buffa::MessageField::some(...)` + `..Default::default()`.
* `src/bot.rs::message_key` — added `..Default::default()` for
`__buffa_cached_size`.
* `src/pdo.rs::test fixture make_web_msg` — wrapped `MessageKey` in
`buffa::MessageField::some(...)`.
* `src/retry.rs::valid_serialized_session` — `MessageField::default()`
in place of `None` for submessage fields, `..Default::default()` for
cached-size coverage.
Validation
* `cargo build --workspace --exclude e2e-tests` — clean.
* `cargo clippy --workspace --all-targets --exclude e2e-tests` — no new
warnings beyond the pre-existing `unused_imports` on
`wacore-libsignal`.
* `cargo test --workspace --exclude e2e-tests` — 1,269 passing
(1,152 → 1,269 after absorbing main's new tests).
…process marker
Two defensive improvements to catch silent build breakage.
Staleness guard
`cargo:rerun-if-changed` only triggered on `src/whatsapp.desc`, so
editing `src/whatsapp.proto` without running the regen script shipped a
stale descriptor without warning. Now:
- Added `cargo:rerun-if-changed=src/whatsapp.proto` so the proto mtime
is tracked.
- Compare proto vs desc mtimes at the top of `main`; abort the build
with a message pointing at `scripts/regenerate-proto-desc.sh` when
the proto is newer.
Verified: touching `whatsapp.proto` and rebuilding fails with
"src/whatsapp.proto is newer than src/whatsapp.desc …"; rerunning the
regen script and building again succeeds.
Post-process assertion
The previous code ran two `.replace` calls on the generated file; a
`.replace` silently produces the original string when the needle isn't
there. The `__buffa_unknown_fields` needle was already a dead no-op
(count = 0 in the current file — `preserve_unknown_fields(false)`
removed the field in the buffa upgrade) and any future rename in buffa
of `__buffa_cached_size` would silently drop the serde skip injection.
- Dropped the dead `__buffa_unknown_fields` replace.
- Assert the `__buffa_cached_size:` needle appears at least once in
the generated file; fail the build with guidance to review the
buffa changelog otherwise.
- Kept the single replacement that actually injects `#[serde(skip)]`
on `__buffa_cached_size`.
Validation: `cargo test --workspace --exclude e2e-tests` — 1,269 tests
passing unchanged.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/send.rs (1)
253-260: 🧹 Nitpick | 🔵 TrivialThis path should stop using
encode_to_vec().Now that the rest of the migration has
write_to(), this newsletter plaintext path is still taking the extra allocation/growth hit fromencode_to_vec(). For large channel posts, pre-sizing the buffer and writing into it directly is the better shape.♻️ Proposed change
let mut plaintext_builder = NodeBuilder::new("plaintext"); if let Some(mt) = wacore::send::media_type_from_message(&message) { plaintext_builder = plaintext_builder.attr("mediatype", mt); } - let mut children = vec![plaintext_builder.bytes(message.encode_to_vec()).build()]; + let mut plaintext = Vec::with_capacity(message.compute_size()); + message.write_to(&mut plaintext); + let mut children = vec![plaintext_builder.bytes(plaintext).build()];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/send.rs` around lines 253 - 260, The plaintext branch currently calls message.encode_to_vec(), causing an extra allocation; instead pre-size a Vec and use the streaming write API: create a Vec<u8> with capacity from message.encoded_len() (or equivalent), call message.write_to(&mut buf) (or write_to_slice/write_to depending on buffa API) to write bytes directly into the buffer, then pass that buffer into plaintext_builder.bytes(buf).build(); update the code around NodeBuilder and message usage (referencing message.encode_to_vec(), message.encoded_len(), message.write_to(), and plaintext_builder.bytes(...).build()) to remove encode_to_vec() and use the pre-sized write approach.src/features/polls.rs (1)
137-159: 🧹 Nitpick | 🔵 TrivialPoll update message construction looks good, but that empty metadata struct is curious.
The
MessageField::some()wrapping forpoll_creation_message_key,vote, andmetadatais consistent with the migration pattern. However, thatmetadatafield at lines 154-156 is literally just..Default::default()with no actual fields set.If this is intentional (WA protocol requires the field to be present but empty), that's fine. But make sure we actually need to send this empty struct on the wire versus just leaving it unset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/polls.rs` around lines 137 - 159, The PollUpdateMessage currently sets metadata to an empty struct via MessageField::some(wa::message::PollUpdateMessageMetadata { ..Default::default() }); confirm whether the WA protocol requires an explicit empty metadata field; if not, remove the metadata entry from the poll_update construction or set it to buffa::MessageField::none() instead, otherwise populate the needed fields on wa::message::PollUpdateMessageMetadata; locate this change in the poll_update variable construction and adjust the metadata line accordingly.
♻️ Duplicate comments (1)
src/bot.rs (1)
906-907: 🧹 Nitpick | 🔵 TrivialIf
AppVersionisCopy, drop these.clone()calls.Line 906, Line 972, and Line 1051 still clone
custom_version. Ifwa::device_props::AppVersionisCopy, these are redundant and should be removed to keep clippy quiet and ownership cleaner.#!/bin/bash set -euo pipefail # 1) Verify whether AppVersion derives Copy in generated protobuf code. fd -i 'whatsapp.rs' | while read -r f; do if rg -n 'pub struct AppVersion' "$f" >/dev/null; then echo "== $f ==" rg -n -C3 'derive\(|pub struct AppVersion' "$f" fi done # 2) Confirm current clone callsites in this file. rg -n -C1 'custom_version\.clone\(\)' src/bot.rsProposed cleanup (apply only if `AppVersion: Copy`)
- .with_device_props(Some(custom_os.clone()), Some(custom_version.clone()), None) + .with_device_props(Some(custom_os.clone()), Some(custom_version), None)- .with_device_props(None, Some(custom_version.clone()), None) + .with_device_props(None, Some(custom_version), None)- Some(custom_version.clone()), + Some(custom_version),Also applies to: 972-973, 1049-1052
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bot.rs` around lines 906 - 907, The code currently calls custom_version.clone() when building device props with .with_device_props(Some(custom_os.clone()), Some(custom_version.clone()), None) and in other callsites (e.g., around .with_runtime(TokioRuntime)); if wa::device_props::AppVersion implements Copy, remove the redundant .clone() calls and pass custom_version by value (custom_version) instead to silence Clippy and simplify ownership; update every occurrence of custom_version.clone() in functions using with_device_props (and any other callsites named custom_version) so they use the value directly, leaving custom_os.clone() unchanged only if OS type is not Copy. Ensure the change compiles and run cargo clippy to confirm no more unnecessary_clone warnings.
🤖 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/client.rs`:
- Around line 2615-2623: The scan currently awaits each external blob download
inline (using self.download inside the patch loop), causing serial RTTs; instead
do a zero-allocation pre-scan that uses Buffa decode_view/view types to borrow
string/bytes from the decode buffer and collect all ext.direct_path values (from
patch.external_mutations and related places) into a set, then after the scan
spawn batched concurrent downloads (e.g., futures::future::join_all or a stream)
to populate pre_downloaded and only then continue processing; apply the same
refactor to the other hotspot mentioned (the block around lines 2832-2845) so
both metadata passes collect paths first and perform downloads concurrently,
preserving existing keys (path.clone()/pre_downloaded insertion) and error
handling semantics (capture version/patch context when a download fails).
In `@src/features/status.rs`:
- Around line 261-274: The test's construction of revoke_message uses MessageKey
{ ... }.into() for the key field while production uses
buffa::MessageField::some(...), causing an inconsistent pattern; update the test
to wrap the MessageKey in buffa::MessageField::some(...) instead of using
.into() (or change production to .into() if you prefer brevity) so both
revoke_message creation sites use the same explicit pattern; locate the
revoke_message definition and replace the .into() call on the MessageKey with
buffa::MessageField::some(...) to match production.
In `@src/pdo.rs`:
- Around line 356-360: The code currently calls
web_msg_info.message.into_option().unwrap() after checking
web_msg_info.message.is_unset(), which can still panic; replace the unwrap with
an explicit extraction (e.g., use let Some(message) =
web_msg_info.message.into_option() { ... } else { warn!(...) ; return; }) so the
handler stays on the non-panicking path; update the block around web_msg_info,
message, is_unset, and into_option to handle the None case explicitly and return
instead of unwrapping.
In `@wacore/src/media_retry.rs`:
- Around line 107-108: The current use of
wa::MediaRetryNotification::decode_from_slice allocates a full owned decode even
though we only need stanza_id, result, and occasionally direct_path; replace
this with the view-based protobuf parser (the buffa/view API) to parse only
those fields without allocating the full message: locate the decode call in
media_retry handling, switch to the non-owning/view decode API and extract
stanza_id, result, and direct_path from the view, and preserve the same error
path by mapping parse failures to the existing anyhow!("protobuf decode failed:
{e}") behavior so callers see identical errors.
In `@wacore/src/pair.rs`:
- Around line 240-246: The code currently coerces a missing
ADVDeviceIdentity.key_index into 0 (let key_index =
identity_details.key_index.unwrap_or(0)), which incorrectly turns a malformed
pairing response into a valid-looking one; change this to fail fast by checking
identity_details.key_index and returning a PairCryptoError when it's None (e.g.,
map the Option to a Result or use match and return PairCryptoError with an
appropriate code/text), and then use the extracted key_index value for
subsequent logic instead of defaulting to 0.
In `@wacore/src/poll.rs`:
- Around line 74-75: The buffer for serializing each vote is created with
Vec::new(), causing repeated reallocations; replace that by reserving the exact
protobuf size before encoding: call vote_msg.encoded_len() and create plaintext
with that capacity (e.g., Vec::with_capacity(...)) so the subsequent
vote_msg.encode(&mut plaintext) is infallible and avoids realloc cost.
In `@wacore/src/usync.rs`:
- Around line 218-233: The ADV key-index byte-construction in
build_test_key_index_bytes is duplicated in wacore/src/iq/usync.rs; extract that
logic into a single shared test helper (e.g., pub fn
build_adv_key_index_bytes(device_ids: &[u16]) -> Vec<u8>) placed in a common
test utils module (such as a new tests::utils or wacore::test_utils) and replace
both build_test_key_index_bytes and the duplicate code in iq/usync.rs to call
the new helper; ensure the helper constructs waproto::whatsapp::ADVKeyIndexList
and ADVSignedKeyIndexList the same way and returns the encoded Vec<u8>, and
update imports/usages accordingly.
---
Outside diff comments:
In `@src/features/polls.rs`:
- Around line 137-159: The PollUpdateMessage currently sets metadata to an empty
struct via MessageField::some(wa::message::PollUpdateMessageMetadata {
..Default::default() }); confirm whether the WA protocol requires an explicit
empty metadata field; if not, remove the metadata entry from the poll_update
construction or set it to buffa::MessageField::none() instead, otherwise
populate the needed fields on wa::message::PollUpdateMessageMetadata; locate
this change in the poll_update variable construction and adjust the metadata
line accordingly.
In `@src/send.rs`:
- Around line 253-260: The plaintext branch currently calls
message.encode_to_vec(), causing an extra allocation; instead pre-size a Vec and
use the streaming write API: create a Vec<u8> with capacity from
message.encoded_len() (or equivalent), call message.write_to(&mut buf) (or
write_to_slice/write_to depending on buffa API) to write bytes directly into the
buffer, then pass that buffer into plaintext_builder.bytes(buf).build(); update
the code around NodeBuilder and message usage (referencing
message.encode_to_vec(), message.encoded_len(), message.write_to(), and
plaintext_builder.bytes(...).build()) to remove encode_to_vec() and use the
pre-sized write approach.
---
Duplicate comments:
In `@src/bot.rs`:
- Around line 906-907: The code currently calls custom_version.clone() when
building device props with .with_device_props(Some(custom_os.clone()),
Some(custom_version.clone()), None) and in other callsites (e.g., around
.with_runtime(TokioRuntime)); if wa::device_props::AppVersion implements Copy,
remove the redundant .clone() calls and pass custom_version by value
(custom_version) instead to silence Clippy and simplify ownership; update every
occurrence of custom_version.clone() in functions using with_device_props (and
any other callsites named custom_version) so they use the value directly,
leaving custom_os.clone() unchanged only if OS type is not Copy. Ensure the
change compiles and run cargo clippy to confirm no more unnecessary_clone
warnings.
🪄 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: bd6e78e4-f979-4529-bbd4-8dead32e74a9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlsrc/bot.rssrc/client.rssrc/client/sender_keys.rssrc/features/newsletter.rssrc/features/polls.rssrc/features/status.rssrc/message.rssrc/pdo.rssrc/retry.rssrc/send.rswacore/Cargo.tomlwacore/libsignal/Cargo.tomlwacore/noise/Cargo.tomlwacore/src/iq/usync.rswacore/src/media_retry.rswacore/src/pair.rswacore/src/poll.rswacore/src/send.rswacore/src/types/events.rswacore/src/usync.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 829455f944
ℹ️ 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: 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 `@waproto/build.rs`:
- Around line 21-29: The mtime-based freshness check in build.rs (the
proto_mtime/desc_mtime comparison) is unreliable because Git rewrites mtimes;
remove or disable that guard and replace it with a deterministic check: compute
and compare a committed content hash or stamp of src/whatsapp.proto vs
src/whatsapp.desc (or invoke a regenerate-and-diff step in CI) instead of using
std::fs::metadata modified times; update the code paths referencing proto_mtime
and desc_mtime so the build error is produced only when the deterministic
comparison fails (or remove the check entirely and move the regenerate-and-diff
to CI).
- Around line 112-123: The replacement currently looks for the exact string "pub
__buffa_cached_size:" (needle) in build.rs and fails if buffa emits a private
"__buffa_cached_size" field; update the needle logic to be visibility-agnostic
by matching either "pub __buffa_cached_size:" or "__buffa_cached_size:" (e.g.,
use a regex with an optional "pub\\s+" prefix or otherwise check both variants
against original) before creating patched and writing to generated so the
#[serde(skip)] injection applies regardless of field visibility (update
references to needle, patched, original in build.rs accordingly).
🪄 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: 2d3bcb62-9ed1-44d9-aea8-37c3c8f62441
📒 Files selected for processing (1)
waproto/build.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
waproto/build.rs (1)
255-258: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFail tag generation instead of emitting tag
0.Line 257 turns a missing descriptor field number into
0, which is not a valid protobuf field tag and would make generatedtags.rsunsafe for partial decoders. Fail loudly with the message/field name instead.Proposed fix
assert!( seen.insert(const_name.clone()), "tags.rs: const name collision `{const_name}` in message `{msg_name}`" ); + let number = field.number.unwrap_or_else(|| { + panic!("tags.rs: missing field number `{const_name}` in message `{msg_name}`") + }); out.push_str(&format!( "{pad} pub const {const_name}: u32 = {};\n", - field.number.unwrap_or_default() + number ));🤖 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 `@waproto/build.rs` around lines 255 - 258, The tag generation in the build script currently falls back to 0 when a descriptor field number is missing, which produces an invalid protobuf tag. Update the logic in the code that emits the constant for field tags to stop using unwrap_or_default and instead fail generation immediately when the field number is absent. Include the field/message context in the error so the source of the invalid descriptor is clear, and keep the fix localized to the tag-writing path in build.rs where the constant is formatted.wacore/libsignal/src/protocol/state/session.rs (1)
243-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate nested sender-chain fields before reporting it usable.
Line 244 only checks the outer
sender_chainpresence. A present-but-empty chain can still returntrue, then later fail insender_ratchet_key()orget_sender_chain_key().Proposed fix
pub fn has_usable_sender_chain(&self) -> Result<bool, InvalidSessionError> { - if self.session.sender_chain.is_unset() { + let Some(sender_chain) = self.session.sender_chain.as_option() else { return Ok(false); - } + }; + let Some(chain_key) = sender_chain.chain_key.as_option() else { + return Ok(false); + }; // We removed timestamp from PendingPreKey, so we can't check for expiration here. // Assuming it's valid if it exists. - Ok(true) + Ok(sender_chain.sender_ratchet_key.is_some() + && sender_chain.sender_ratchet_key_private.is_some() + && chain_key.key.is_some() + && chain_key.index.is_some()) }🤖 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/libsignal/src/protocol/state/session.rs` around lines 243 - 249, The `has_usable_sender_chain` method currently returns true whenever `session.sender_chain` is present, but it should also verify the nested sender-chain fields are actually usable. Update this check in `has_usable_sender_chain` so it only returns true when the chain contains the required sender state needed by `sender_ratchet_key()` and `get_sender_chain_key()`, otherwise return false for a present-but-empty chain.
🤖 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/media.rs`:
- Around line 80-83: The context_info mapping logic is duplicated in four places
in the media handling code, so extract it into a single helper and reuse it
everywhere. Add a small helper around the repeated opts.context_info to
buffa::MessageField::some(*ci).unwrap_or_default() mapping, then call that
helper from each of the duplicated construction sites in src/media.rs to keep
the behavior identical while removing the copy-paste.
- Around line 258-291: The context_info coverage is incomplete because only
image_message and video_message are tested; add equivalent assertions for
document_message and audio_message in the media tests. Reuse the existing
pattern from image_maps_context_info and video_maps_context_info by creating
ContextInfo, passing it through DocumentOptions and AudioOptions, then asserting
the resulting message’s context_info is set so all shipped media types are
covered consistently.
In `@src/passkey/flow.rs`:
- Around line 431-451: The passkey flow is dropping the active session when an
awaited step fails, so retrying the same attempt becomes impossible. Update the
session handling in `send_passkey_confirmation` and the `drive_continuation`
path around `session.on_primary_identity(...)` so `state.session` is restored on
`Err` unless the session has reached `Stage::Done`. Keep the existing
take/restore pattern in `Flow` and `PasskeyState` logic, and only consume the
session permanently once the flow completes successfully.
- Around line 63-65: `ShortcakeIo::query` currently takes a raw `InfoQuery` and
the `Client` implementation likely routes it through `send_iq`, which bypasses
the `execute(Spec)` seam. Update the passkey flow to model the
ref/options/prologue/nonce/encrypted-request IQs as specs and change
`Client::query` to call `client.execute(...).await?` instead of sending the IQ
directly. Keep the `ShortcakeIo` trait and its `query` method aligned with the
spec boundary so the passkey flow remains testable through `execute(Spec)`.
In `@src/send/mod.rs`:
- Around line 1318-1322: The per-chat `distribution_guard` in `src/send/mod.rs`
is held too broadly across `send_node()` and SKDM marking, which blocks
unrelated outgoing sends for the same chat. Narrow the lock scope in the
cold-send flow around the sender-key distribution/recheck portion only, or
replace it with a single-flight distribution marker that does not guard the
whole send path; update the affected send branches and helpers that currently
acquire `distribution_guard` so they release it before network I/O.
---
Outside diff comments:
In `@wacore/libsignal/src/protocol/state/session.rs`:
- Around line 243-249: The `has_usable_sender_chain` method currently returns
true whenever `session.sender_chain` is present, but it should also verify the
nested sender-chain fields are actually usable. Update this check in
`has_usable_sender_chain` so it only returns true when the chain contains the
required sender state needed by `sender_ratchet_key()` and
`get_sender_chain_key()`, otherwise return false for a present-but-empty chain.
In `@waproto/build.rs`:
- Around line 255-258: The tag generation in the build script currently falls
back to 0 when a descriptor field number is missing, which produces an invalid
protobuf tag. Update the logic in the code that emits the constant for field
tags to stop using unwrap_or_default and instead fail generation immediately
when the field number is absent. Include the field/message context in the error
so the source of the invalid descriptor is clear, and keep the fix localized to
the tag-writing path in build.rs where the constant is formatted.
🪄 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: 049d1ba4-1588-4ae9-8f50-25562078cad3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/release.ymlCargo.tomlsrc/features/status.rssrc/history_sync.rssrc/media.rssrc/message/receive.rssrc/message/tests.rssrc/pair.rssrc/passkey/flow.rssrc/send/mod.rsstorages/sqlite-storage/Cargo.tomltests/e2e/tests/status.rswacore/Cargo.tomlwacore/libsignal/src/protocol/state/session.rswacore/src/history_sync.rswacore/src/pair.rswacore/src/pair_code.rswacore/src/send/encrypt.rswacore/src/send/group.rswacore/src/send/tests.rswacore/src/shortcake.rswacore/src/types/events.rswaproto/build.rs
🛑 Comments failed to post (5)
src/media.rs (2)
80-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Same 4-line context_info mapping copy-pasted four times.
We don't ship duplicate plumbing four times over at this company — extract it once and move on.
♻️ Proposed helper
+fn context_info_field(ci: Option<Box<wa::ContextInfo>>) -> buffa::MessageField<wa::ContextInfo> { + ci.map(|c| buffa::MessageField::some(*c)).unwrap_or_default() +} + pub fn image_message(upload: UploadResponse, opts: ImageOptions) -> wa::Message { wa::Message { image_message: buffa::MessageField::some(wa::message::ImageMessage { ... - context_info: opts - .context_info - .map(|ci| buffa::MessageField::some(*ci)) - .unwrap_or_default(), + context_info: context_info_field(opts.context_info), ..Default::default() }), ..Default::default() } }Also applies to: 108-111, 138-141, 168-171
🤖 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/media.rs` around lines 80 - 83, The context_info mapping logic is duplicated in four places in the media handling code, so extract it into a single helper and reuse it everywhere. Add a small helper around the repeated opts.context_info to buffa::MessageField::some(*ci).unwrap_or_default() mapping, then call that helper from each of the duplicated construction sites in src/media.rs to keep the behavior identical while removing the copy-paste.
258-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Only image and video got context_info tests — document and audio didn't.
If we're testing a feature, we test it everywhere it's shipped, not half of it.
🤖 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/media.rs` around lines 258 - 291, The context_info coverage is incomplete because only image_message and video_message are tested; add equivalent assertions for document_message and audio_message in the media tests. Reuse the existing pattern from image_maps_context_info and video_maps_context_info by creating ContextInfo, passing it through DocumentOptions and AudioOptions, then asserting the resulting message’s context_info is set so all shipped media types are covered consistently.src/passkey/flow.rs (2)
63-65: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify passkey IQs do not bypass the typed IQ Spec execution path. rg -n -C3 'send_iq|InfoQuery::(get|set)|execute\(' src/passkey src/client src/iq wacore/src/iqRepository: oxidezap/whatsapp-rust
Length of output: 33178
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files and inspect the passkey flow plus IQ spec definitions. git ls-files 'src/passkey/*' 'wacore/src/iq/*' 'src/client/*' | sort printf '\n--- outline: src/passkey/flow.rs ---\n' ast-grep outline src/passkey/flow.rs --view expanded || true printf '\n--- outline: wacore/src/iq ---\n' ast-grep outline wacore/src/iq --view expanded || true printf '\n--- outline: src/client/iq_ops.rs ---\n' ast-grep outline src/client/iq_ops.rs --view expanded || trueRepository: oxidezap/whatsapp-rust
Length of output: 50378
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the specific passkey and IQ-related sections only. for f in src/passkey/flow.rs wacore/src/iq/mod.rs src/client/iq_ops.rs; do if [ -f "$f" ]; then echo "===== $f =====" wc -l "$f" fi done echo "===== src/passkey/flow.rs =====" sed -n '1,220p' src/passkey/flow.rs echo "===== wacore/src/iq/mod.rs =====" sed -n '1,260p' wacore/src/iq/mod.rs echo "===== src/client/iq_ops.rs =====" sed -n '1,260p' src/client/iq_ops.rsRepository: oxidezap/whatsapp-rust
Length of output: 19707
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "===== wacore/src/iq/spec.rs =====" sed -n '1,220p' wacore/src/iq/spec.rs echo "===== src/passkey/flow.rs (client impl + helpers) =====" sed -n '300,430p' src/passkey/flow.rs echo "===== src/passkey/flow.rs (IQ-driving functions) =====" sed -n '430,620p' src/passkey/flow.rsRepository: oxidezap/whatsapp-rust
Length of output: 13829
Route passkey IQs through
execute(Spec)instead of rawInfoQuery.ShortcakeIo::queryhard-codessend_iq, so the passkey ref/options/prologue/nonce/encrypted-request calls bypass the IQ spec contract. Model them as specs and have theClientimpl callclient.execute(...).await?so the seam stays testable at the spec boundary.🤖 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/passkey/flow.rs` around lines 63 - 65, `ShortcakeIo::query` currently takes a raw `InfoQuery` and the `Client` implementation likely routes it through `send_iq`, which bypasses the `execute(Spec)` seam. Update the passkey flow to model the ref/options/prologue/nonce/encrypted-request IQs as specs and change `Client::query` to call `client.execute(...).await?` instead of sending the IQ directly. Keep the `ShortcakeIo` trait and its `query` method aligned with the spec boundary so the passkey flow remains testable through `execute(Spec)`.Source: Coding guidelines
431-451: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the passkey session when an awaited step fails.
Right now, a transient
companion_nonceorencrypted_pairing_requestIQ failure dropsstate.session, so the user cannot retry the same passkey attempt. Restore the session onErrunless it reachedStage::Done.Proposed fix
- session.confirm(self).await + if let Err(err) = session.confirm(self).await { + if session.stage != Stage::Done { + self.passkey_state.lock().await.session = Some(session); + } + return Err(err); + } + Ok(())Apply the same pattern around
session.on_primary_identity(...)indrive_continuation.Also applies to: 454-467
🤖 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/passkey/flow.rs` around lines 431 - 451, The passkey flow is dropping the active session when an awaited step fails, so retrying the same attempt becomes impossible. Update the session handling in `send_passkey_confirmation` and the `drive_continuation` path around `session.on_primary_identity(...)` so `state.session` is restored on `Err` unless the session has reached `Stage::Done`. Keep the existing take/restore pattern in `Flow` and `PasskeyState` logic, and only consume the session permanently once the flow completes successfully.src/send/mod.rs (1)
1318-1322: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Don’t hold a per-chat outgoing-send guard across network I/O.
distribution_guardis acquired per group and kept until aftersend_node()and SKDM marking. At WhatsApp scale, one slow cold send can now block unrelated sends to the same chat. Narrow this to the sender-key distribution/recheck phase, or replace it with a single-flight distribution marker that does not lock the whole outgoing send. As per coding guidelines, “Do not lock outgoing sends per-chat (matches WA Web behavior).” <coding_guidelines>Also applies to: 1418-1447, 1476-1510, 1553-1590, 1879-1887
🤖 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/send/mod.rs` around lines 1318 - 1322, The per-chat `distribution_guard` in `src/send/mod.rs` is held too broadly across `send_node()` and SKDM marking, which blocks unrelated outgoing sends for the same chat. Narrow the lock scope in the cold-send flow around the sender-key distribution/recheck portion only, or replace it with a single-flight distribution marker that does not guard the whole send path; update the affected send branches and helpers that currently acquire `distribution_guard` so they release it before network I/O.Source: Coding guidelines
Three fixes from the PR review: - DecryptionErrorMessageProto's manual merge_field skipped unknown fields with skip_field, which restarts the recursion budget at RECURSION_LIMIT; use skip_field_depth(ctx.depth()) as buffa's docs require, so unknown group fields cannot reset the budget for a depth-doubling attack. - The history-sync full-decode fallback capped wrapper unwrapping at 16 while the fast lazy path capped at 40, so a message nested 17..40 layers deep was classified (poll/forwarded/bot) against a different base by the two paths. Unify both on MAX_MESSAGE_WRAP_DEPTH = 40. - encode_and_pad_with_context and pad_with_context_from_encoded measured the spliced message_context_info twice (once for the buffer capacity, once inside push_message_field). Reuse the SizeCache the capacity pass already filled for the write, matching buffa's compute_size-then-write_to contract; byte output is unchanged.
There was a problem hiding this comment.
💡 Codex Review
whatsapp-rust/src/send/actions.rs
Line 121 in 52dd9a5
Use Buffa enum names for pin actions
With the Buffa-generated enums in this migration, PinInChatMessage.Type uses the proto value names (PIN_FOR_ALL / UNPIN_FOR_ALL), so the old prost-style PinForAll variant no longer exists. Any build that includes the pin-message API fails here; the unpin call below has the same stale variant pattern.
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/history_sync.rs (1)
1477-1486: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCheck forwarding on the capped base message.
base_message_viewtreats the message reached atMAX_MESSAGE_WRAP_DEPTHas the base, but this path returnsfalsebefore checking that same message’s context. That lets a forwarded marker at the cap bypass the forwarded-secret skip.Proposed fix
fn message_is_forwarded_at_depth(message: &wa::Message, depth: usize) -> bool { if depth >= MAX_MESSAGE_WRAP_DEPTH { - return false; + return message_context_is_forwarded(message); } if let Some(inner) = first_wrapped_message(message) { return message_is_forwarded_at_depth(inner, depth + 1); }🤖 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/history_sync.rs` around lines 1477 - 1486, The depth cap in message_is_forwarded_at_depth currently returns false before evaluating the capped message itself, so forwarded markers on the base message can be missed. Update the recursion in message_is_forwarded_at_depth to treat the message reached at MAX_MESSAGE_WRAP_DEPTH as the base case and still run message_context_is_forwarded on that message, while only stopping recursion past the cap. Keep the existing first_wrapped_message flow and ensure the forwarded check applies to the terminal message reached at the cap.
🤖 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 `@wacore/src/history_sync.rs`:
- Around line 1477-1486: The depth cap in message_is_forwarded_at_depth
currently returns false before evaluating the capped message itself, so
forwarded markers on the base message can be missed. Update the recursion in
message_is_forwarded_at_depth to treat the message reached at
MAX_MESSAGE_WRAP_DEPTH as the base case and still run
message_context_is_forwarded on that message, while only stopping recursion past
the cap. Keep the existing first_wrapped_message flow and ensure the forwarded
check applies to the terminal message reached at the cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ad2889a-6a03-4c02-bdc4-a2c0785a3c31
📒 Files selected for processing (3)
wacore/libsignal/src/protocol/protocol.rswacore/src/history_sync.rswacore/src/messages.rs
Resolve the pair.rs test-module conflict by keeping both sides' tests (buffa's do_pair_crypto_rejects_missing_key_index and main's #943 extract_pairing_props / lid_migrated_update tests). Adapt the merged #943 code to the buffa API: - prost PascalCase-normalizes LIDMigration* to LidMigration*, buffa keeps the proto name verbatim, so the types stay LIDMigrationMapping, LIDMigrationMappingSyncMessage, LIDMigrationMappingSyncPayload. - ClientPairingProps::decode and LIDMigrationMappingSyncPayload::decode become decode_from_slice; use prost::Message becomes use buffa::Message. - the optional-bool getter is_chat_db_lid_migrated() becomes the field accessed as .unwrap_or(false). - protocol_message and lid_migration_mapping_sync_message are MessageField, read via .as_option() and built via MessageField::some(..) not Some(Box::new).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e974135df
ℹ️ 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/client/lid_pn.rs`:
- Around line 429-446: The LID migration state-transition logic is duplicated
between latch_lid_migrated_from_props and the tail of
handle_lid_migration_mapping_sync, so consolidate the shared check-then-set into
one private helper. Move the !lid_migrated plus ab_props().is_enabled(... ) gate
and the SetLidMigrated(true) persistence update into a single internal method,
and have both call sites invoke it with a reason string for the log message.
Keep the existing behavior identical while ensuring future changes to the
migration gate happen in one place only.
🪄 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: 35107150-979f-490e-b20c-317526721250
📒 Files selected for processing (9)
src/client/iq_ops.rssrc/client/lid_pn.rssrc/message/receive.rssrc/message/tests.rssrc/pair.rssrc/send/mod.rswacore/src/pair.rswacore/src/store/commands.rswacore/src/store/device.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 429-446: The LID migration state-transition logic is duplicated
between latch_lid_migrated_from_props and the tail of
handle_lid_migration_mapping_sync, so consolidate the shared check-then-set into
one private helper. Move the !lid_migrated plus ab_props().is_enabled(... ) gate
and the SetLidMigrated(true) persistence update into a single internal method,
and have both call sites invoke it with a reason string for the log message.
Keep the existing behavior identical while ensuring future changes to the
migration gate happen in one place only.
🪄 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: 35107150-979f-490e-b20c-317526721250
📒 Files selected for processing (9)
src/client/iq_ops.rssrc/client/lid_pn.rssrc/message/receive.rssrc/message/tests.rssrc/pair.rssrc/send/mod.rswacore/src/pair.rswacore/src/store/commands.rswacore/src/store/device.rs
🛑 Comments failed to post (1)
src/client/lid_pn.rs (1)
429-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Dedup this — I don't want two copies of the same state-transition logic drifting apart.
latch_lid_migrated_from_props(Line 435-446) and the tail ofhandle_lid_migration_mapping_sync(Line 554-564) run the exact same check-then-set:!lid_migrated && ab_props.is_enabled(...)→process_command(SetLidMigrated(true)). Only the log line differs. If we ever change this gate (e.g. add a second prop check), it's trivial to update one call site and forget the other, and then the two migration triggers silently diverge. That's the kind of bug that doesn't show up until it's in someone's production fleet.Extract one private helper and have both call it with a
reason: &strfor the log message.♻️ Proposed consolidation
- pub(crate) async fn latch_lid_migrated_from_props(&self) { - if !self.persistence_manager.get_device_snapshot().lid_migrated - && self - .ab_props() - .is_enabled(wacore::iq::abprops::web::LID_ONE_ON_ONE_MIGRATION_ENABLED) - .await - { - log::info!("Account is 1:1-LID-migrated (ab prop observation)"); - self.persistence_manager - .process_command(crate::store::commands::DeviceCommand::SetLidMigrated(true)) - .await; - } - } + pub(crate) async fn latch_lid_migrated_from_props(&self) { + self.set_lid_migrated_if_prop_enabled("ab prop observation").await; + } + + /// Shared gate for [`latch_lid_migrated_from_props`] and + /// `handle_lid_migration_mapping_sync`: both raise the persisted flag + /// under the identical `!lid_migrated && ab_prop_enabled` condition. + async fn set_lid_migrated_if_prop_enabled(&self, reason: &str) { + if !self.persistence_manager.get_device_snapshot().lid_migrated + && self + .ab_props() + .is_enabled(wacore::iq::abprops::web::LID_ONE_ON_ONE_MIGRATION_ENABLED) + .await + { + log::info!("Account is 1:1-LID-migrated ({reason})"); + self.persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetLidMigrated(true)) + .await; + } + }Then at the tail of
handle_lid_migration_mapping_sync:- if !self.persistence_manager.get_device_snapshot().lid_migrated - && self - .ab_props() - .is_enabled(wacore::iq::abprops::web::LID_ONE_ON_ONE_MIGRATION_ENABLED) - .await - { - log::info!("Account is 1:1-LID-migrated (primary mapping sync)"); - self.persistence_manager - .process_command(crate::store::commands::DeviceCommand::SetLidMigrated(true)) - .await; - } + self.set_lid_migrated_if_prop_enabled("primary mapping sync").await;Also applies to: 554-564
🤖 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/client/lid_pn.rs` around lines 429 - 446, The LID migration state-transition logic is duplicated between latch_lid_migrated_from_props and the tail of handle_lid_migration_mapping_sync, so consolidate the shared check-then-set into one private helper. Move the !lid_migrated plus ab_props().is_enabled(... ) gate and the SetLidMigrated(true) persistence update into a single internal method, and have both call sites invoke it with a reason string for the log message. Keep the existing behavior identical while ensuring future changes to the migration gate happen in one place only.
Resolve the history_sync.rs conflicts by taking main's side (the #945 density reserve comment and the #947 Option-returning read_varint), then adapt the cross-pollinated tests: the #945 single-big-conversation test builds MessageKey via buffa::MessageField::some, and the branch's varint overflow test asserts is_none() instead of a Result error.
Collaborator
Author
|
@greptile-apps review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
| Filename | Overview |
|---|---|
| waproto/build.rs | Complete rewrite: drops prost-build, runs snake_case descriptor rewrite at build time, configures buffa-build with serde/bytes/box/view attributes; generates tags.rs from original (camelCase) descriptor so const names are layout-stable. |
| waproto/src/lib.rs | Adds #[inline(never)] pinned codec entry points for Message, WebMessageInfo, HistorySync, Conversation, MessageContextInfo; adds two-pass compute_size/write_to variants and a message_context_info_merge helper; all calls route through these to prevent per-crate monomorphization bloat. |
| wacore/src/messages.rs | Migrates encode/pad/splice paths to buffa SizeCache two-pass API; adds unpadded_message_len helper, decode_plaintext_view/decode_plaintext_owned_view, and sender_key_distribution_only_plaintext zero-copy fast path; introduces push_message_field_sized to reuse a pre-filled cache. |
| wacore/libsignal/src/protocol/state/session.rs | Migrates sender_chain and chain_key fields from Option to MessageField; replaces prost encoding helpers in serialize_into with a shared-SizeCache two-pass approach; adds regression tests for manual encoding correctness and the mixed-cache-shape path. |
| wacore/src/history_sync.rs | Removes ~200 lines of hand-rolled prost internal-field structs; replaces with direct field access on the generated types using buffa's MessageField API; manual wire decoder and schema-pin asserts unchanged. |
| wacore/src/store/device.rs | Renames AdvSignedDeviceIdentity to ADVSignedDeviceIdentity; replaces prost::Message with buffa::Message; updates enum literals to SCREAMING_SNAKE_CASE; wraps MessageField fields with buffa::MessageField::some; unwraps version chain via .app_version deref instead of nested expects. |
| storages/sqlite-storage/build.rs | New file: runs buffa-build for the on-disk wire format and adds the same sha256 freshness guard already used in waproto, failing the build if the committed .desc no longer matches wire.proto. |
| wacore/src/proto_helpers.rs | Mechanical migration to MessageField API (as_option, as_option_mut, is_set, into_option); merge_dsm_context signature drops Box<>; limit_sharing_v2 test assertion strengthened to verify specific field values instead of just is_set. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[whatsapp.proto\ncamelCase fields] --> B[protoc\n.desc generator]
B --> C[whatsapp.desc\ncommitted]
C --> D{build.rs}
D --> E[snake_case_descriptor_fields\nrename json_name + field.name]
E --> F[whatsapp.snake.desc\nOUT_DIR]
F --> G[buffa-build\n.generate_views + .box_type\n.preserve_unknown_fields false]
G --> H[whatsapp.rs\nMessageField / SCREAMING_SNAKE enums\nView types]
D --> I[generate_tags\nfrom original descriptor]
I --> J[tags.rs\npub const FIELD: u32 = N]
H --> K[waproto codec\n#inline never entry points\npinned monomorphization]
K --> L[wacore / app layer\nencode_dm_plaintexts\nserialize_into\ndecode_plaintext_view]
C --> M[sha256 freshness guard\nbuild fails on stale desc]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[whatsapp.proto\ncamelCase fields] --> B[protoc\n.desc generator]
B --> C[whatsapp.desc\ncommitted]
C --> D{build.rs}
D --> E[snake_case_descriptor_fields\nrename json_name + field.name]
E --> F[whatsapp.snake.desc\nOUT_DIR]
F --> G[buffa-build\n.generate_views + .box_type\n.preserve_unknown_fields false]
G --> H[whatsapp.rs\nMessageField / SCREAMING_SNAKE enums\nView types]
D --> I[generate_tags\nfrom original descriptor]
I --> J[tags.rs\npub const FIELD: u32 = N]
H --> K[waproto codec\n#inline never entry points\npinned monomorphization]
K --> L[wacore / app layer\nencode_dm_plaintexts\nserialize_into\ndecode_plaintext_view]
C --> M[sha256 freshness guard\nbuild fails on stale desc]
Reviews (2): Last reviewed commit: "test(proto-helpers): assert outer's limi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The __buffa_cached_size serde patch guarded against pre-0.7 generated output, but the build pins buffa-build 0.8.1, so the needle can never match; drop the dead patch and the full-file scan it ran every build. The SKDM-only fast path matched wire fields 2/15/35 as bare literals; use the generated tag constants so a proto renumber updates the classifier instead of silently misrouting SKDM-only messages.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…presence is_set() could not tell outer's value from a default the merge might set on its own; a distinguishable payload plus field equality restores the original assertion strength.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
@greptile-apps review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Massive protobuf codegen migration (prost → buffa) touching 117+ files, 9.6k lines, affecting core data structures, API patterns, build scripts, and CI. Extremely high risk of breakage.
Re-trigger cubic
This was referenced Jul 13, 2026
Merged
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.
Summary
Migrate protobuf codegen from prost/prost-build to buffa/buffa-build v0.8.1 (from crates.io), then build on top of buffa's zero-copy views to cut allocations on the hot receive/decode paths.
prost/prost-buildwithbuffa/buffa-build(v0.8.1, from crates.io).whatsapp.rsis no longer tracked —build.rsgenerates it intoOUT_DIRfrom the committedwhatsapp.desc(+whatsapp.desc.sha256freshness guard). Consumers never needprotoc.generate_views=true) now used on real decode paths — not just enabled for the future.API surface changes (codebase-wide)
Option<Box<T>>MessageField<T>(.as_option()/.is_set()/.is_unset())Option<i32>Option<EnumType>(typed)CamelCaseSCREAMING_SNAKE_CASEAdvSignedDeviceIdentityADVSignedDeviceIdentityType::decode(bytes)Type::decode_from_slice(bytes)TypeView::decode_view(bytes)/ owned viewsmsg.encode(&mut buf)?msg.encode_to_vec()Nonefields..Default::default()Performance work on top of the migration
compute_sizetraversal in session serialization.Notable fixes / decisions
is_sender_key_distribution_only: slow path encodes-and-compares, because buffa'sMessageFieldequality treats set-to-default as equal to unset.whatsapp.rsremoved from version control;build.rsfails fast on a stale descriptor or missing post-process marker.PollOptionproto workaround dropped — buffa ≥ v0.7 handles the nestedOptionmessage, so the proto is back to upstreammessage Option.Updated against
mainThis branch has been merged up to current
main(through #683). The merge layered the buffa view architecture together withmain's recent work, resolving conflicts by keeping the best of each side:wacore/src/history_sync.rs): kept buffa's zero-copy view extraction as the base, and portedmain's streaming decompression (perf: implement streaming decompression for history sync processing #672) and message-secret retention metadata —timestamp/is_poll_or_event/is_bot_invocation(feat(msg-secret)!: bound messageSecret retention by policy and event-time horizon #668) — onto it (prost-based extraction can't compile here).LazyHistorySync(events.rs): keptmain's memory-freeing design (raw bytes freed after first decode, perf(history-sync): free LazyHistorySync raw bytes after a successful decode #669), adapted to buffa'sdecode_from_slice; dropped the buffaview()accessor (incompatible with freeing, no real consumers).store/device.rs): combinedmain'sArc<…>wrapping (perf(send): Arc immutable device fields + recent-message bytes #674) with buffa'sADVSignedDeviceIdentitynaming +MessageFieldAPI.messages.rs): keptmain'svalidate_bcl_hash/ standard-base64 phash parity (fix(send): correct group phash and mark full SKDM target set (WA Web parity) #678/feat: WA Web phash parity — usync device_hash (#3), group-metadata phash (#7), bcl hash validation (#6) #679) + buffa'sunpadded_message_len/unpad_message_refsplit.src/message.rsreferenced the owned message before it was materialized on the buffa receive path — now reads the borrowed view.Post-migration: binary size, perf regressions, publishability, descriptor guards
After the migration landed green, a measurement-driven cleanup pass:
.textby +1.83 MiB. Routing the hot decode/encode trees through#[inline(never)]non-generic entry points inwaproto::codec(Message,WebMessageInfo, the history-sync records, and the send-pathcompute_size/write_to) collapses the per-crate duplicates into a single instantiation. Net.textregression is now ~+410 KiB (waproto-attributed bloat 3.04 → 1.59 MiB;llvm-lines wacoredropped below the prost baseline).SyncActionValue; the two-pass per-field encode scan on wide messages) and are documented; an earlier CPU regression incollect_unique_index_macsbecame a +28% improvement by decoding each index MAC once and byte-sorting instead of re-walking theMessageFieldchain in the sort comparator.0.8.0, socargo package/ the release workflow no longer reject git-only dependencies.wire.descand voip MLowtables.descbuild scripts now fail the build when the committed.descno longer matches its.proto(the same sha256 guardwaprotoalready had), so a stale descriptor can't silently generate code from an old schema.Test plan
cargo fmt --all— cleancargo sort -w— allCargo.tomlalready sortedcargo clippy --all --tests— 0 warnings, 0 errorscargo test --workspace --exclude e2e-tests— 1,889 tests pass, 0 failures.text~+410 KiB, perf net +25%.proto, passes cleanKnown trade-off: closed enums drop unknown wire values
buffa generates proto2 enum fields as a closed
Option<Enum>and drops any wire value outside the compiled schema at decode time (from_i32 -> None), where prost kept an openOption<i32>that round-tripped any integer. In practice WhatsApp only sends in-schema enum values today, so this is a forward-compatibility gap rather than an active bug, but it means an unknown enum value would vanish on a decode->re-encode path (own-device echo, history-sync persistence) and a hypothetical out-of-rangeSyncdMutation.operationwould count as SET in an ltHash (recoverable via resync).buffa already ships the open-enum runtime type (
EnumValue<E>=Known(E)/Unknown(i32)), so the fix is an opt-in codegen flag rather than new machinery — tracked upstream at anthropics/buffa#269. We keep the closed-enum output for now and will switch the affected fields toEnumValueonce that option lands, rather than hand-rollingint32fields here and reverting the typed-enum ergonomics this PR is about.