perf(events): share wa::Message via Arc end-to-end (zero deep-clone on dispatch) - #613
Merged
Merged
Conversation
Cloning MessageContext (e.g., re-spawning into a fire-and-forget background task) deep-copied wa::Message every time. wa::Message carries media buffers and extended_text payloads (often >1 KB) — significant overhead on hot paths that fan out per-message work (emoji-challenge dispatch, sticker triggers). Switching to Arc<wa::Message> makes Clone an O(1) refcount bump. New from_arc() constructor lets callers that already own an Arc avoid the one remaining clone in from_parts(). BREAKING (minor): the public field type changed from Box<wa::Message> to Arc<wa::Message>. Read-only access (&self.message, .field, deref) is unaffected. Code that moves out of the field (let msg = *ctx.message) needs (*ctx.message).clone() instead.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMessageContext now stores the WhatsApp payload as a private ChangesMessageContext Arc Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bot.rs`:
- Around line 33-38: This change makes MessageContext.message a breaking API by
switching its type from Box<wa::Message> to Arc<wa::Message>; either revert the
field back to pub message: Box<wa::Message> to preserve ABI, or if you must keep
Arc, treat it as a breaking change: make the field private, bump the crate major
version, and add compatibility accessors/ctors on MessageContext such as pub fn
message_arc(&self) -> Arc<wa::Message> and pub fn into_box(self) ->
Box<wa::Message> (implementing into_box by unwrapping the Arc when unique or
cloning the inner wa::Message into a Box) plus From/Into impls to ease
migration; reference the MessageContext struct and its message field in your
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ack) Addresses CodeRabbit's review on PR oxidezap#613: making the public field type change Box<wa::Message> → Arc<wa::Message> a controlled breaking change instead of a silent ABI break. Changes: - 'message' field is now private; readers go through new accessors - pub fn message(&self) -> &wa::Message — replaces direct field access - pub fn message_arc(&self) -> Arc<wa::Message> — cheap Arc::clone for re-spawn / shared ownership without exposing the inner storage - pub fn into_box(self) -> Box<wa::Message> — compatibility helper for legacy code; uses Arc::try_unwrap when the Arc is uniquely held, falling back to deep clone otherwise Read-only access pattern stays nearly identical (ctx.message.field → ctx.message().field). Re-spawns get a typed API for cheap sharing. Old code that needed Box can call into_box() during migration. Storage representation is now an implementation detail behind getters.
Contributor
Author
|
Thanks for the review @coderabbitai — the breaking-change-via-public-field concern is fair. Pushed
Storage rep is now an implementation detail. Verified locally with I left the version bump to the maintainer's discretion since it's part of release tagging upstream — happy to add it to the PR if you prefer. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 40057
What's working correctly:
One gap worth addressing — unit tests for The test module currently only exercises #[cfg(test)]
mod message_context_tests {
use super::*;
use std::sync::Arc;
fn make_ctx() -> MessageContext {
let msg = Arc::new(wa::Message::default());
let info = MessageInfo::default(); // adjust if non-Default
let client = Arc::new(/* minimal Client stub or use existing helper */);
MessageContext::from_arc(msg, &info, client)
}
#[test]
fn message_returns_ref() {
let ctx = make_ctx();
// just ensure it compiles and returns the expected type
let _: &wa::Message = ctx.message();
}
#[test]
fn message_arc_cheap_clone() {
let ctx = make_ctx();
let arc1 = ctx.message_arc();
let arc2 = ctx.message_arc();
assert!(Arc::ptr_eq(&arc1, &arc2)); // same allocation
}
#[test]
fn into_box_unique_arc_zero_clone() {
let msg = Arc::new(wa::Message::default());
let ptr = Arc::as_ptr(&msg);
let info = MessageInfo::default();
let client = Arc::new(/* ... */);
let ctx = MessageContext::from_arc(msg, &info, client);
let boxed = ctx.into_box();
// Uniquely held — pointer in Box should equal original Arc pointer
assert_eq!(&*boxed as *const _, ptr);
}
#[test]
fn into_box_shared_arc_deep_clones() {
let msg = Arc::new(wa::Message::default());
let _keeper = Arc::clone(&msg); // prevent try_unwrap from succeeding
let info = MessageInfo::default();
let client = Arc::new(/* ... */);
let ctx = MessageContext::from_arc(msg, &info, client);
let _boxed = ctx.into_box(); // must not panic; verifies fallback path
}
}The On the version bump — agreed, that's the right call to leave it to the maintainer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bot.rs`:
- Around line 58-91: Add unit tests that exercise the Arc compatibility
boundary: create a wa::Message, wrap it in Arc and construct the context with
from_arc(), assert message() returns expected data and message_arc() yields an
Arc that points to the same message (Arc::ptr_eq). Then test into_box() in two
scenarios: (1) when the Arc is uniquely held (drop any clones before calling
into_box()) assert the returned Box contains the original message instance
(i.e., no cloning by verifying identity via ptr or by checking a side-effect
field), and (2) when the Arc is shared (keep another Arc clone alive) call
into_box() and assert a cloned Box is returned (distinct identity but equal
content). Use the functions from_arc, message_arc, message, and into_box to
locate code under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Endereça segundo review do CodeRabbit no PR oxidezap#613 — pediu testes focados nos métodos novos. 3 testes novos: - from_arc_preserves_allocation: from_arc não deep-clona; message_arc() retorna o mesmo Arc allocation (Arc::ptr_eq via Arc::as_ptr). - into_box_reclaims_when_unique: refcount==1 → try_unwrap path consome o Arc; weak sentinel fica órfão (strong_count=0, upgrade=None). - into_box_clones_when_shared: refcount>1 → fallback clone preserva conteúdo + kept Arc retorna a strong_count=1, alocação distinta da do Box (verificado via std::ptr::eq negativo). Pointer identity comentada onde não funciona (Box::new realloca o T mesmo após Arc::try_unwrap por mover o T pra novo heap slot).
Contributor
Author
|
Thanks @coderabbitai — added the unit tests in
One subtlety I documented inline: pointer identity for Full suite green: 452 + 22 + 0 + 2 passed. |
|
Tip For best results, initiate chat on the files or code changes.
452 + 22 + 0 + 2 all green. The implementation and the test suite are both solid. This PR is in good shape. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bot.rs`:
- Around line 1163-1179: The test `into_box_reclaims_when_unique` currently only
proves the last `Arc` was dropped and doesn't guarantee `Arc::try_unwrap` was
used; either add an explicit probe that can distinguish the try_unwrap path
(e.g., wrap the message payload in a small type with a Drop side-effect or a
flag that can detect whether the original payload was moved vs cloned and assert
that move-path occurred when calling `MessageContext::into_box` on an `Arc`
created via `MessageContext::from_arc`), or change the test name/comment to no
longer claim coverage of `Arc::try_unwrap`/zero-clone behavior; reference
`into_box`, `MessageContext::from_arc`, `into_box_reclaims_when_unique`, and
`Arc::try_unwrap` when locating the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
CodeRabbit aponta corretamente que os testes anteriores
(`into_box_reclaims_when_unique` / `_clones_when_shared`) só validavam
o estado pós-into_box (Arc consumido / kept survives) — uma regressão
hipotética que sempre clonasse passaria igual.
Mudanças:
- Renomeados os state-level tests pra refletir o que provam
(`into_box_with_unique_arc_consumes_strong_ref` /
`into_box_with_shared_arc_preserves_other_strong_refs`).
- Comentários inline explicitam que são state-level e apontam pro
path-level probe.
- Novo módulo de teste algorítmico:
- `CloneCounter`: payload com Drop side-effect via AtomicUsize que
registra cada Clone::clone.
- `try_unwrap_or_clone<T>()`: réplica genérica do pattern de
`into_box` (a fn de produção é monomorfizada em wa::Message).
- `into_box_skips_clone_when_unique`: refcount==1 → counter == 0.
- `into_box_invokes_clone_exactly_once_when_shared`: refcount>1 →
counter == 1.
Cobertura ironclad da fn de produção exigiria torná-la genérica;
documentado inline como out-of-scope. Os tests algorítmicos garantem
que o pattern descrito no PR se comporta como anunciado.
Contributor
Author
|
Good catch @coderabbitai — you're right, the state-level tests can't distinguish Renamed the existing tests to reflect what they actually verify (state, not path):
Inline comments now explicitly say these are state-level and point to the path-level probe. Added a path-level probe in a separate test block:
The honest caveat (called out inline): a regression that swaps Test suite: 454 + 22 + 0 + 2 passed; |
|
Tip For best results, initiate chat on the files or code changes.
The caveat you documented inline is honest and correct: a regression that replaced the What the suite now covers end-to-end:
That's a complete picture. 454 + 22 all green and clippy clean — the implementation and test suite are both in good shape. This PR is ready. |
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 `@src/bot.rs`:
- Around line 1183-1186: The comment in src/bot.rs incorrectly references the
test name; update the shared-path comment to cite
into_box_invokes_clone_exactly_once_when_shared (the test that asserts the
"clone exactly once" guarantee) instead of into_box_skips_clone_when_unique, so
the comment matches the actual test coverage and avoids confusion when
inspecting functions like into_box and its test helpers.
- Around line 1232-1239: The function try_unwrap_or_clone duplicates
MessageContext::into_box's Arc::try_unwrap/clone logic; extract that match into
a single private generic helper (e.g., fn into_box_from_arc<T: Clone>(arc:
Arc<T>) -> Box<T>) and replace both try_unwrap_or_clone and
MessageContext::into_box to call this helper so there is one canonical
implementation used by production and tests; ensure the helper is generic over
T: Clone and returns Box<T>, and update both callers to delegate to it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Endereça duas observações do CodeRabbit no PR oxidezap#613: 1. (Major nitpick) Eliminar duplicação prod/test do algoritmo. Antes existia tanto MessageContext::into_box quanto try_unwrap_or_clone (em tests) com a mesma match Arc::try_unwrap. Helper privado genérico arc_into_box<T: Clone>(arc) -> Box<T> agora é a única fonte da verdade — into_box delega, e os path-level tests chamam o mesmo helper. Regressão na produção sai imediatamente nos tests. 2. (Minor) Stale ref no comentário do state-test 'shared'. Apontava pra into_box_skips_clone_when_unique; corrigido pra into_box_invokes_clone_exactly_once_when_shared (que é onde a garantia 'clone exactly once' é asserida).
Contributor
Author
|
Both fixed in Single source of truth. New private helper at module scope: fn arc_into_box<T: Clone>(arc: Arc<T>) -> Box<T> {
match Arc::try_unwrap(arc) {
Ok(value) => Box::new(value),
Err(arc) => Box::new((*arc).clone()),
}
}
Stale ref fix. Updated the comment on
|
Move `Event::Message` from `Box<wa::Message>` to `Arc<wa::Message>` and route `MessageContext::from_event` through `Arc::clone`, eliminating the deep-clone that previously happened on every dispatched message. Also trims the `MessageContext` API back to what's actually used in-tree: - field stays `pub` (no accessor methods); `Arc::clone(&ctx.message)` is the natural way to share, matching serenity / matrix-sdk - drops `into_box` and `arc_into_box` helper (no callers) - single test verifying `from_arc` doesn't deep-clone (Arc::as_ptr equality) Updated dispatch sites: `src/message.rs`, `src/pdo.rs` now `Arc::new` instead of `Box::new`. `Event::as_message` returns `&Arc<wa::Message>` so callers that need the Arc can clone cheaply; auto-deref keeps existing `msg.field` access compiling.
jlucaso1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/bot.rs (1)
36-41:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
messageis stillpub— the accessor methods described in the PR are absent.The past review flagged this and was marked ✅ resolved (commit
1230131), and the PR description explicitly listsmessageas made private with accessorsmessage(),message_arc(), andinto_box(). But the code tells a different story — the field is stillpub message: Arc<wa::Message>and none of those accessor methods appear anywhere in theimpl MessageContextblock (lines 43–117).This needs to be correct. Either:
- The accessors were never committed (implementation gap), or
- The field was intended to stay public (but then the PR description is wrong and the past review resolution was premature).
If the field stays
pub, at minimum the PR description and the "addressed" resolution on the past review need to be corrected. If the field should be private, those methods need to actually land.🤖 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/bot.rs` around lines 36 - 41, The MessageContext struct still exposes pub message: Arc<wa::Message> despite the PR and past review claiming it was made private with accessors; either remove pub from the message field and implement the missing accessors message(), message_arc(), and into_box() on impl MessageContext, or if you intentionally want the field public, update the PR description and prior review resolution to reflect that decision. Specifically, make the field private (remove pub from message) and add these methods on MessageContext: message() to return an immutable reference to the inner wa::Message, message_arc() to return a cloned Arc<wa::Message>, and into_box() to consume the context (or its Arc) and return an owned/boxed wa::Message as intended; ensure the method names match exactly so reviewers can verify the change.
🤖 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/bot.rs`:
- Around line 1099-1104: The test directly accesses the public field
ctx.message, which couples the test to the field being pub; change the assertion
to use the public accessor (e.g., MessageContext::message_arc() or a
message_arc() method) so it compares Arc::as_ptr(&ctx.message_arc()) to
original_ptr instead of Arc::as_ptr(&ctx.message); update the test to call
MessageContext::from_arc(...) as before and replace any direct field access with
the accessor to ensure the test remains valid if MessageContext makes the field
private.
---
Duplicate comments:
In `@src/bot.rs`:
- Around line 36-41: The MessageContext struct still exposes pub message:
Arc<wa::Message> despite the PR and past review claiming it was made private
with accessors; either remove pub from the message field and implement the
missing accessors message(), message_arc(), and into_box() on impl
MessageContext, or if you intentionally want the field public, update the PR
description and prior review resolution to reflect that decision. Specifically,
make the field private (remove pub from message) and add these methods on
MessageContext: message() to return an immutable reference to the inner
wa::Message, message_arc() to return a cloned Arc<wa::Message>, and into_box()
to consume the context (or its Arc) and return an owned/boxed wa::Message as
intended; ensure the method names match exactly so reviewers can verify the
change.
🪄 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: df6e7ca8-02fc-4659-9376-cd1bd48bb00a
📒 Files selected for processing (4)
src/bot.rssrc/message.rssrc/pdo.rswacore/src/types/events.rs
alexandme
added a commit
to Micra-io/zeroclaw
that referenced
this pull request
May 6, 2026
…estore Apr-24 protocol parity (#52) * chore(deps)!: pin wa-rs* to Micra-io/whatsapp-rust rebrand fork Bump wa-rs* version constraints from "0.2" → "0.5" and add [patch.crates-io] redirecting all 10 wa-rs* crates at the wa-rs-rebrand-2026-05 branch of the Micra-io/whatsapp-rust fork of oxidezap/whatsapp-rust. Build is intentionally broken at this commit — Task 4 patches src/channels/whatsapp_web.rs for the breaking deltas surfaced by the new wa-rs (109 compile errors expected: Arc<Event> wrapping from oxidezap/whatsapp-rust#613, BotBuilder typestate change from zeroclaw-labs#586 DevicePropsOverride, removed methods, etc.). Refs: - docs/plans/2026-05-06-001-wa-rs-rebrand-plan.md - #50 - zeroclaw-labs#6246 * fix(channels/whatsapp): adapt to wa-rs 0.5 breaking changes The 0.2 → 0.5 wa-rs bump (Micra-io rebrand of oxidezap/whatsapp-rust) exposed 109 cascading API errors beyond the originally-scoped with_device_props change. This commit rebuilds ZeroClaw's WhatsApp adapters against the new trait/type surface so cargo check + release build are green. ## src/channels/whatsapp_web.rs - BotBuilder gained a 4th typestate slot (with_runtime). Use `wa_rs::TokioRuntime` to satisfy it; enable wa-rs's `tokio-runtime` feature on our optional dep. - with_device_props rewritten to take a `DevicePropsOverride` builder (oxidezap/whatsapp-rust#586) instead of three positional Option args. - Event handlers now receive `Arc<Event>` (PR zeroclaw-labs#613); switched the match to `match &*event` so variant-data binding still works. - `Client::get_phone_number_from_lid` was removed in favor of the unified `Client::get_lid_pn_entry(&Jid) -> Result<Option<LidPnEntry>>` (zeroclaw-labs#487); call site rewritten to extract `entry.phone_number`, error branch swallowed back to None to preserve best-effort enrichment semantics. - `Client::upload` gained a third `UploadOptions` arg; pass `UploadOptions::default()` for legacy behavior. - `UploadResponse` cryptographic fields are now `[u8; 32]` (was Vec<u8>); use the `*_vec()` accessors to convert before populating protobuf message fields. - `Client::send_message` now returns `SendResult { message_id, to }` rather than a bare String (PR zeroclaw-labs#597); log via `.message_id`. - `Bot::run()` returns `BotHandle` (Future + abort handle wrapper); field type updated from `tokio::task::JoinHandle<()>` accordingly. ## src/channels/whatsapp_storage.rs - `to_store_err!` macro now wraps errors as `Box<dyn Error + Send + Sync>` to match the new `StoreError::Database(Box<dyn Error>)` variant (was `StoreError::Database(String)`). - `SignalStore::load_identity` returns `Option<[u8; 32]>` (was `Option<Vec<u8>>`); validates length and copies into a fixed array. - `SignalStore::get_session` and `SignalStore::load_prekey` return `Option<bytes::Bytes>`; added `bytes` as a feature-gated direct dep and converted via `Bytes::from(vec)`. - Added `SignalStore::get_max_prekey_id` (queries MAX(id), 0 on empty) and `AppSyncStore::get_latest_sync_key_id` (most-recent key by key_id desc) — both new in wa-rs-core 0.5. - ProtocolStore's per-device sender-key tracking replaces the legacy SKDM model. Removed `get_skdm_recipients` / `add_skdm_recipients` / `clear_skdm_recipients` / `mark_forget_sender_key` / `consume_forget_marks` (no longer trait members) and added `get_sender_key_devices`, `set_sender_key_status`, `clear_sender_key_devices`, `delete_sender_key_device_rows`, `clear_all_sender_key_devices`. - Added `ProtocolStore::delete_devices` (force re-fetch on next query). - Added the sent-message retry trio: `store_sent_message`, `take_sent_message` (atomic SELECT+DELETE under an immediate tx), `delete_expired_sent_messages`. The expiry path is correct but the daemon doesn't yet schedule a cleanup cron — flagged with a TODO(wa-rs-0.5) comment so the table doesn't grow unbounded. - `DeviceListRecord` gained a `raw_id: Option<u32>` field (ADV identity index); `update_device_list` / `get_devices` round-trip it via a new `raw_id` column. ## SQLite schema - New tables: `sender_key_devices` (per-device SKDM status), `sent_messages` (retry payload store). - `device_registry.raw_id` column added; legacy databases get the column via an idempotent `ALTER TABLE ADD COLUMN` migration probed through `pragma_table_info` (SQLite has no IF NOT EXISTS for ADD COLUMN). - Legacy `skdm_recipients` and `sender_key_status` tables remain in place but are no longer read or written; left in to avoid touching active production session DBs. ## Cargo.toml - Pinned `bytes = "1"` behind the whatsapp-web feature to satisfy the new `Bytes` return types in storage traits. - Enabled wa-rs's `tokio-runtime` feature so `wa_rs::TokioRuntime` is in scope for the BotBuilder. ## Stubs / TODOs - `delete_expired_sent_messages` works correctly but no daemon cron invokes it yet — TODO comment in the impl. ## Validation - `cargo check --release --features whatsapp-web,observability-otel`: clean. - `cargo build --release --features whatsapp-web,observability-otel`: clean (6m 06s). - `cargo test --release -p zeroclawlabs --features whatsapp-web` — storage suite: 3 / 3 passing (`rusqlite_store_creates_database`, `lid_mapping_round_trip_preserves_learning_source_and_updated_at`, `delete_expired_tc_tokens_returns_deleted_row_count`). Refs: docs/plans/2026-05-06-001-wa-rs-rebrand-plan.md * fix(channels/whatsapp): address code-review findings on storage impl - Drop unreachable Err(QueryReturnedNoRows) arm in get_max_prekey_id (MAX(...) aggregates always return one row, NULL on empty). - Use i64::from(r) instead of `r as i64` for u32 -> i64 widening. - Replace `r as u32` with u32::try_from(r).ok() on raw_id round-trip so out-of-range DB values degrade cleanly rather than truncating silently. raw_id drives session/sender-key invalidation; a wrong value would falsely reset live sessions. - take_sent_message: switch from default `transaction()` (BEGIN DEFERRED) to `transaction_with_behavior(Immediate)` for true SELECT+DELETE atomicity at the SQLite layer. Process-level Mutex<Connection> already serialized the body, but the immediate semantics match upstream's reference impl and protect against future shared-connection scenarios. All net-new clippy errors from #40aee96 cleared. Two pre-existing needless_borrow lints remain on whatsapp_web.rs and are out of scope. Refs: docs/plans/2026-05-06-001-wa-rs-rebrand-plan.md * chore(deps): pin wa-rs* fork to commit SHA, not branch [patch.crates-io] now uses rev = "46322bdccd2f91751215a6bbe93c7331b19e84ec" instead of branch = "wa-rs-rebrand-2026-05" so the resolved tree is reproducible across builds. Pinned commit corresponds to: - 46322bd refactor(wacore): replace if-let-guard with bare boolean guard - a5ea93c fix(build): correct over-eager rename in waproto/build.rs doc - c8c3e36 refactor: rebrand crates from whatsapp-rust to wa-rs namespace Refs: docs/plans/2026-05-06-001-wa-rs-rebrand-plan.md * style: cargo fmt auto-format on whatsapp adapters Pre-push quality gate caught formatting drift introduced during the wa-rs 0.5 adaptation. Mechanical reformatting only — no behavioral changes. * fix(cron): gate whatsapp delivery arm on whatsapp-web feature Pre-existing bug: the cron scheduler match arm for whatsapp delivery used WhatsAppWebChannel without gating on the whatsapp-web feature, breaking builds with --no-default-features (or any feature combo that excludes whatsapp-web). Mirrors the existing matrix-channel pattern: real impl gated on feature=enabled, no-op fallback bails with a clear error when the feature is disabled. Caught by pre-push quality gate. Pre-existing on main; bundled here since the PR already touches whatsapp surfaces. * fix(tools/git_operations): clear GIT_DIR/GIT_WORK_TREE before git subprocess calls Pre-existing issue surfaced by pre-push hook: when zeroclaw runs git operations from a context where GIT_DIR is set in the environment (e.g., inside a git hook), the inherited env overrides the caller-specified path and git operates on the wrong repo. This tool's whole purpose is to run git on a path the caller specified, so we explicitly clear GIT_DIR and GIT_WORK_TREE before each git spawn — both in the production run_git_command helper and in the test fixtures that bootstrap temp repos. Caught when pushing the wa-rs rebrand branch: 3 tests (git_operations_work_in_subdirectory, blocks_readonly_mode_for_write_ops, rejects_unknown_operation) failed under the hook env but passed when run directly. Reproduced and fixed. * test(channels/whatsapp): add atomicity + raw_id round-trip tests Addresses P1 findings from the expert-panel review of PR #52: 1. take_sent_message atomicity — verifies SELECT+DELETE under Immediate transaction returns the payload exactly once. Second call to take on the same key returns None. Guards against silent loss of retry-receipt payloads. 2. DeviceListRecord raw_id round-trip — verifies Some(u32) and None both survive the i64 conversion in update_device_list / get_devices. Catches regressions where a wrong cast would falsely invalidate live sessions per the rationale in bf841ec. Refs: docs/plans/2026-05-06-001-wa-rs-rebrand-plan.md --------- Co-authored-by: Test <test@test.com>
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
2 participants
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
Make
wa::Messageshareable viaArcfrom the dispatch site all the way to userhandlers, eliminating the deep-clone that previously happened on every received
message.
Motivation
wa::Messagecarries media buffers and extended-text payloads (often >1 KB).On the inbound path it was deep-cloned twice:
Client::dispatch_message→Event::Message(Box::new(msg), ...)(owned move into Box, fine)MessageContext::from_event→from_parts(&msg, ...)→Box::new(message.clone())← deep cloneDownstream bots that re-spawned per message (
tokio::spawnwith a cloned contextfor fire-and-forget side effects) cloned the whole message again on every spawn.
Changes
Core (
wacore)Event::Message(Box<wa::Message>, Arc<MessageInfo>)→Event::Message(Arc<wa::Message>, Arc<MessageInfo>)Event::as_message()now returnsOption<(&Arc<wa::Message>, &MessageInfo)>socallers that need to share ownership can
Arc::clonecheaplyClient (
whatsapp-rust)MessageContext::message:Box<wa::Message>→Arc<wa::Message>MessageContextderivesClone(refcount bump only)MessageContext::from_eventclones theArcinstead of deep-cloning the messageMessageContext::from_arc(Arc<wa::Message>, ...)constructor for callersthat already own an Arc
src/message.rsandsrc/pdo.rsswitched fromBox::new(...)toArc::new(...)Compatibility (pre-1.0 breakage)
Event::Messagevariant signature changed (Box→Arc). Pattern matcheslike
Event::Message(msg, info)keep working since bothBoxandArcderef to
&wa::Message. Code that explicitly constructedEvent::Message(Box::new(m), info)needs to switch toArc::new.Event::as_message()return type changed; field access via the returned&Arc<wa::Message>keeps compiling thanks to deref coercion.MessageContext.messagetype changed but the field stayspub. Reading viactx.message.fieldcontinues to work viaArcderef. Code that moved outof the field (
let m = *ctx.message) needs(*ctx.message).clone().Precedent
The pattern matches what other Rust messaging libs do for shareable per-event
context: serenity's
Context(Arc-internal, Clone-cheap), matrix-rust-sdk'sRoom/Client, teloxide'sBot. Message payload itself is owned/Arc-sharedrather than boxed.
Test plan
cargo fmt --allcargo clippy --workspace --exclude e2e-tests --tests— no new warningscargo test --workspace --exclude e2e-tests --lib— 1378 passed, 0 failedcargo checkcoverse2e-testscompilation (full e2e run requires the mock server)