refactor(errors): typed ConnectError and friends for the public API - #1090
Conversation
Several public entry points still returned bare anyhow::Error even though the crate already models its failures with thiserror. Callers could only match on strings, and one signature promised a failure it never produced. - ConnectError/ConnectStage replace anyhow on connect(), wait_for_socket() and wait_for_connected(), so a timeout is matchable and says which step of the flow expired instead of being just another opaque message. - logout() no longer returns Result: the deregistration IQ is best-effort and the local teardown runs either way, so there was nothing to branch on. - MessageEditError types the JID resolution failures of a type that is re-exported from the crate root. - SignalMaintenanceError separates unusable key material from storage, IQ and drain failures, which decides whether a retry can help. - The noise socket now passes NoiseError straight into EncryptSendError instead of stringifying it, keeping the source chain intact. - PasskeyError and EncryptSendError gain #[non_exhaustive] so later variants and fields are not breaking; ClientError::AlreadyConnected is dropped now that ConnectError owns that case.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR introduces typed public errors for client connection, Signal maintenance, and message-edit sender resolution. It updates lifecycle and rotation APIs, preserves encryption error sources, adds public re-exports, marks selected errors non-exhaustive, and adjusts tests. ChangesTyped client lifecycle
Signal maintenance
Message edit resolution
Error contract hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant connect_graph
participant ReadinessWait
Client->>connect_graph: connect()
connect_graph->>ReadinessWait: wait_for_socket() / wait_for_connected()
ReadinessWait-->>connect_graph: ConnectError::Timeout with stage
connect_graph-->>Client: connection result
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 |
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
| Filename | Overview |
|---|---|
| src/client.rs | Introduces ConnectError, ConnectStage, and SignalMaintenanceError public error types; removes ClientError::AlreadyConnected. Enum definitions are clean, doc-comments are clear, and #[non_exhaustive] is correctly applied. |
| src/client/lifecycle.rs | connect(), connect_graph(), wait_for_socket(), wait_for_connected() return ConnectError; logout() is now infallible. Reconnect loop transitions from anyhow downcast to typed pattern match on ConnectError::Handshake. Tests cover all branches. |
| src/client/adapters.rs | flush_pending_signal_state() and flush_signal_cache_batch_safe() now return SignalMaintenanceError. persist_signal_state_pre_wire() (pub(crate)) converts via Into::into for the anyhow::Error return path. Tests confirm healthy and failure paths. |
| src/features/rotate_key.rs | All key-rotation paths now return SignalMaintenanceError. CorruptKey correctly covers decode/field failures on staged records; Signal covers signing-backend failures; storage_err helper preserves typed cause in source() chain. |
| src/features/message_edit.rs | New MessageEditError with InvalidTargetJid and MissingTargetSender; original_sender_jid and original_sender_for_dispatch return it. Tests verify all three failure modes and confirm source() chain is preserved. |
| src/features/signal.rs | From for SignalError implemented: Signal variant routes to Protocol, everything else wraps into Internal. Correctly matches within the same crate despite #[non_exhaustive]. |
| src/socket/noise_socket.rs | NoiseError no longer flattened to anyhow!(e.to_string()); passed directly to EncryptSendError::crypto(). A test asserts NoiseError downcasts correctly from the source() chain. |
| src/socket/error.rs | EncryptSendError gains #[non_exhaustive]; struct literal construction outside the crate is now rejected while field access remains. Tests cover NoiseError and opaque source preservation. |
| src/lib.rs | ConnectError/ConnectStage added to prelude; ConnectError/ConnectStage/SignalMaintenanceError/MessageEditError all re-exported at the crate root. Consistent with how sibling error types are handled. |
| src/signal_flush.rs | coalesced_flush_attempt() (pub(crate), returns anyhow::Error) converts SignalMaintenanceError via Into::into — correct for an internal path that predates the typed surface. |
| src/passkey/mod.rs | PasskeyError gains #[non_exhaustive] — minimal, correct change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Client::connect()"] -->|"ConnectError::NotActivated"| ERR1["lifecycle not active"]
A -->|"ConnectError::AlreadyConnected"| ERR2["already connecting / connected"]
A --> B["connect_graph()"]
B -->|"ConnectError::Timeout { stage: VersionFetch }"| ERR3["version fetch timed out"]
B -->|"ConnectError::Version(...)"| ERR4["version resolve failed"]
B -->|"ConnectError::Timeout { stage: Transport }"| ERR5["transport timed out"]
B -->|"ConnectError::Transport(...)"| ERR6["transport open failed"]
B --> C["do_handshake()"]
C -->|"ConnectError::Handshake(HandshakeError)"| ERR7["noise handshake failed"]
C --> D["Connected ✓"]
D --> E["wait_for_socket()"]
E -->|"ConnectError::Timeout { stage: Socket }"| ERR8["socket wait timed out"]
D --> F["wait_for_connected()"]
F -->|"ConnectError::Timeout { stage: Ready }"| ERR9["ready wait timed out"]
G["flush_pending_signal_state() / rotate_signed_pre_key()"] -->|"SignalMaintenanceError::CorruptKey"| ERR10["retry won't help"]
G -->|"SignalMaintenanceError::Storage"| ERR11["backend failure - retryable"]
G -->|"SignalMaintenanceError::Iq"| ERR12["server rejected - retryable"]
G -->|"SignalMaintenanceError::Signal"| ERR13["signing primitive failed"]
G -->|"SignalMaintenanceError::DrainCommitFailed / DrainShuttingDown"| ERR14["drain safety guard"]
Reviews (4): Last reviewed commit: "refactor(errors): dedupe storage mapping..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib.rs (1)
202-208: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShip the whole family —
SignalMaintenanceErroris missing from the prelude.We just added
ConnectError/ConnectStagetopreludefor exactly this reason (lettingprelude::*users match on the new typed errors), butSignalMaintenanceError— the return type of two other new-in-this-PR public methods (rotate_signed_pre_key,flush_pending_signal_state) — didn't get the same treatment. If we're doing this, let's do it consistently for every consumer-facing type from the same module.🔧 Proposed fix
- pub use crate::client::{ConnectError, ConnectStage}; + pub use crate::client::{ConnectError, ConnectStage, SignalMaintenanceError};🤖 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/lib.rs` around lines 202 - 208, Update the crate prelude module to re-export SignalMaintenanceError alongside the existing client error types, so users of prelude::* can access the return type of rotate_signed_pre_key and flush_pending_signal_state consistently with ConnectError and ConnectStage.src/client/lifecycle.rs (1)
466-476: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStop the connect-failure logs from hiding the root cause.
In
{#},anyhowprints its full cause chain, butConnectError::Version/ConnectError::Transporthere render only the static message. The sameDebug-formatted pattern used insrc/client/adapters.rs’s Signal flush errors should be used here so on-call can see the underlying connection/version error.🔧 Proposed fix
if is_transient { - debug!("Transient connect failure, will retry: {connect_err:#}"); + debug!("Transient connect failure, will retry: {connect_err:?}"); } else { - error!("Failed to connect: {connect_err:#}. Will retry..."); + error!("Failed to connect: {connect_err:?}. Will retry..."); }🤖 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/lifecycle.rs` around lines 466 - 476, Update the connect failure logging in the lifecycle retry path around self.connect().await to use the same Debug-formatted error pattern as the Signal flush errors in adapters.rs, ensuring ConnectError::Version and ConnectError::Transport include their underlying causes while preserving the transient and non-transient log branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/message_edit.rs`:
- Around line 50-53: Update the documentation comment for the
MissingTargetSender error variant to state that from_me was not Some(true),
covering both None and Some(false), while preserving the existing error behavior
and message.
- Around line 228-249: Add a test covering the resolve_target_sender path with
no participant, from_me set to Some(false), and a malformed remote_jid; assert
the returned InvalidTargetJid contains field "remoteJid" and preserves the
original parse error source.
- Around line 674-734: The test fixtures in
original_sender_jid_reports_an_unparseable_participant and
original_sender_jid_reports_a_target_key_without_any_sender use nonconforming
phone-like JID literals. Replace the my_jid values with repository-compliant
fictitious NANP data using a real NPA, the 555 exchange, and a 0100–0199 line
number, while preserving the existing test behavior.
In `@src/features/rotate_key.rs`:
- Around line 66-70: In the rotate-key function, replace the repeated Storage
error mapping at each backend call site with a shared storage_err helper that
accepts the static context and returns the SignalMaintenanceError mapping.
Update all listed sites, including the dynamic new_id context by constructing it
before the call, while preserving each existing context message.
---
Outside diff comments:
In `@src/client/lifecycle.rs`:
- Around line 466-476: Update the connect failure logging in the lifecycle retry
path around self.connect().await to use the same Debug-formatted error pattern
as the Signal flush errors in adapters.rs, ensuring ConnectError::Version and
ConnectError::Transport include their underlying causes while preserving the
transient and non-transient log branches.
In `@src/lib.rs`:
- Around line 202-208: Update the crate prelude module to re-export
SignalMaintenanceError alongside the existing client error types, so users of
prelude::* can access the return type of rotate_signed_pre_key and
flush_pending_signal_state consistently with ConnectError and ConnectStage.
🪄 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: f936e80d-68e9-4145-a791-5eb4cbf0d46f
📒 Files selected for processing (13)
src/client.rssrc/client/adapters.rssrc/client/extension_lifecycle.rssrc/client/lifecycle.rssrc/features/message_edit.rssrc/features/mod.rssrc/features/rotate_key.rssrc/features/signal.rssrc/lib.rssrc/passkey/mod.rssrc/signal_flush.rssrc/socket/error.rssrc/socket/noise_socket.rs
The only conflict was the features re-export list in src/lib.rs, where main added MexGraphQLError, PollOptionResult, PollVoteCiphertext and Polls while this branch added MessageEditError. Kept both.
…uptKey CorruptKey documents itself as material a retry would read back identically, which is true for the staged record but not for a signature computed in this call. A wrong width there means the signing backend broke its contract, so InvalidState is the honest classification and leaves the retry decision right.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client.rs (1)
581-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the new public error types
#[non_exhaustive]too.I like that we're locking down
PasskeyErrorandEncryptSendErrorwith#[non_exhaustive]— that's the right call before 1.0. ButConnectStage,ConnectError, andSignalMaintenanceErrorare brand new, also public (re-exported at crate root and inprelude, persrc/lib.rs), and just as likely to grow variants later (new connect stages, new maintenance failure classes). Leaving them exhaustive today just means we eat another breaking change the next time we add a variant. Let's be consistent and seal all four.🛡️ Proposed fix
+#[non_exhaustive] pub enum ConnectStage { ... } +#[non_exhaustive] pub enum ConnectError { ... } +#[non_exhaustive] pub enum SignalMaintenanceError { ... }🤖 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.rs` around lines 581 - 666, Mark the public ConnectStage enum with #[non_exhaustive] to prevent future stage additions from breaking callers, and verify ConnectError and SignalMaintenanceError retain their existing #[non_exhaustive] annotations alongside the other public error types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/client.rs`:
- Around line 581-666: Mark the public ConnectStage enum with #[non_exhaustive]
to prevent future stage additions from breaking callers, and verify ConnectError
and SignalMaintenanceError retain their existing #[non_exhaustive] annotations
alongside the other public error types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77c34d31-0fa1-4935-9dda-4297f2fb74c0
📒 Files selected for processing (4)
src/client.rssrc/client/lifecycle.rssrc/features/rotate_key.rssrc/lib.rs
MissingTargetSender is also reached when from_me is Some(false), so the doc saying it "was not set" described a narrower condition than the code checks. The remoteJid branch of resolve_target_sender had no failure test even though its sibling participant branch did, so a regression there would have been silent. The seven backend calls in the rotation flow each repeated the same Storage-plus-context wrapper; a helper keeps the next one from copying it again without changing what lands in the source chain.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/message_edit.rs (1)
338-339: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winHandle the dispatch-result calls before formatting.
original_sender_for_dispatch(...).to_string()will not compile because the method now returnsResult<Jid, MessageEditError>andResultisDisplay-compatible in the current setup. Wire the remaining.to_string()calls through.expect(...)or.unwrap()like the other test sites.🤖 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/features/message_edit.rs` around lines 338 - 339, Update the dispatch-result handling in the method matching on self.kind to unwrap or expect the Result<Jid, MessageEditError> returned by original_sender_for_dispatch before calling to_string. Apply the same handling to all remaining dispatch-result .to_string() call sites, consistent with the existing test sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/features/message_edit.rs`:
- Around line 338-339: Update the dispatch-result handling in the method
matching on self.kind to unwrap or expect the Result<Jid, MessageEditError>
returned by original_sender_for_dispatch before calling to_string. Apply the
same handling to all remaining dispatch-result .to_string() call sites,
consistent with the existing test sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 19812b04-b114-4033-815f-df77645aecdc
📒 Files selected for processing (2)
src/features/message_edit.rssrc/features/rotate_key.rs
|
@coderabbitai the outside-diff "won't compile" finding on Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/features/message_edit.rs --items all --type impl,method,function --match 'EncryptedEdit|SecretEncrypted|original_sender_for_dispatch|original_sender_jid'
printf '\n--- declarations (Lines 180-355) ---\n'
sed -n '180,355p' src/features/message_edit.rs
printf '\n--- tests (Lines 800-850) ---\n'
sed -n '800,850p' src/features/message_edit.rs
printf '\n--- dispatch call site (Lines 920-960) ---\n'
sed -n '920,960p' src/features/message_edit.rsLength of output: 845 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- declarations (Lines 180-355) ---'
sed -n '180,355p' src/features/message_edit.rs
printf '%s\n' '--- tests (Lines 800-850) ---'
sed -n '800,850p' src/features/message_edit.rs
printf '%s\n' '--- dispatch call site (Lines 920-960) ---'
sed -n '920,960p' src/features/message_edit.rsLength of output: 12300
The compilation finding incorrectly conflated those APIs. Withdrawing it. ✏️ Learnings added
|
2 participants
Summary
A handful of public entry points still returned bare
anyhow::Erroreven though the crate already models its failures withthiserror, so callers had nothing to match on but strings, and one signature promised a failure it could never produce. This replaces those with typed errors on the public surface and tightens two enums that were missing#[non_exhaustive]. Pre-1.0, so the breaking signature changes are taken now rather than carried forward.Changes
ConnectError/ConnectStage(breaking):Client::connect(),wait_for_socket()andwait_for_connected()now returnResult<(), ConnectError>. A timeout is a singleTimeout { stage, timeout }variant rather than four look-alike variants, so a caller can answer "did it time out, and where?" in one match instead of parsing a message. Handshake failures stay typed via#[from] HandshakeError, which is what the reconnect loop already classifies on.ClientError::AlreadyConnectedremoved (breaking): the case now lives onConnectError, and the old variant had no remaining constructor.logout()is infallible (breaking): the body only ever returnedOk(()). The deregistration IQ is best-effort (it cannot be sent at all while offline) and the local teardown runs either way, so there was nothing for a caller to branch on. Signature now matches the behavior; a failed IQ is still logged at warn.MessageEditError(breaking):EncryptedEdit::original_sender_jid,SecretEncrypted::original_sender_jidandoriginal_sender_for_dispatchreturn it instead ofanyhow::Error. The type is re-exported from the crate root, so its failures should be inspectable; the two real modes are a malformed JID in the target key and a key carrying neither participant norremoteJid.SignalMaintenanceError(breaking):flush_pending_signal_state()and the signed pre-key rotation path return it. It separates unusable key material (CorruptKey, where a retry reads back the same bytes) from storage, IQ and drain failures, which is exactly the distinction a caller needs to decide whether retrying helps. Storage causes are wrapped withanyhow::Error::new(e).context(..)so the typed backend error stays in thesource()chain.noise_socket.rspassedanyhow!(e.to_string())intoEncryptSendError::crypto, discarding theNoiseErrortype and the wholesource()chain. It now passesedirectly, which the constructor already accepted.#[non_exhaustive]onPasskeyErrorandEncryptSendError: the last public error types without it.EncryptSendErroris a struct with public fields, so construction goes through the existing constructors.Tests cover both directions for each new behavior: the timeout and the already-connected rejection on connect, teardown with a failing and a non-sendable deregistration IQ, a healthy and an injected-failure signal flush, a corrupt staged pre-key and a disconnected rotation, both edit-target failure modes, and a downcast asserting the
NoiseErrorsurvives insideEncryptSendError.Validation
cargo fmt --all --checkcargo clippy -p whatsapp-rust -p wacore --all-targets -- -D warnings(workspace-wide clippy is not runnable in my environment: thevoip-cliexample pulls inalsa-sys, whose build script fails without ALSA headers. The other members were covered withcargo checkone2e-tests,bench-integration,chat-storeandplugin-metrics, all clean.)cargo test -p whatsapp-rust --lib— 1150 passed, 0 failed, 1 ignoredFull matrix, including wasm32 and the e2e suite, left to CI.
Generated by Claude Code