refactor!: preserve typed error sources across the workspace

[jlucaso1]

Summary

6-commit series eliminating the error.to_string() / format!("{e}") anti-pattern across the workspace. Every typed error from one layer was being flattened into a String at the next, dropping .source() chains and forcing downstream consumers to parse English prose to discriminate cases (e.g. code=400 vs timeout vs disconnect).

Principle applied: um erro só vira String no momento de imprimir, nunca antes. Breaking changes accepted because the code is pre-1.0 and the wrong thing was on the public surface.

Series (each commit is independent and reviewable on its own)

1. refactor(pair-code)! — Split PairCodeError. wacore variants kept (validation, key derivation, AEAD), high-level PairError introduced wrapping PairCodeError + IqError. CryptoError(String) exploded into typed variants for CurveError (key parse, key agreement) and CryptoProviderError (AES-GCM).
2. refactor(socket,noise,iq)! — NoiseError::CryptoError(String) → Encrypt/Decrypt(#[source] CryptoProviderError). SocketError::Crypto/NoiseHandshake removed; new Cipher(#[from] NoiseError) and Marshal(#[source] BinaryError). IqError got EncryptSend(#[from]) + ClientState(#[source]) for the catch-all that previously flattened ClientError. Two dead variants (SocketError::NoiseHandshake, wacore::request::IqError::Network) deleted.
3. refactor(media-download)! — MediaDecryptionError::Decryption(String) → Decryption(#[source] DecryptionError) + new Mac(#[source] CryptoError).
4. refactor(mex)! — MexError::PayloadParsing(String) kept for the 15+ descriptive call sites; one site that wrapped a real JidError got moved to a new InvalidJid(#[from] JidError) variant.
5. refactor(libsignal,store)! — Added SignalProtocolError::BackendError(&'static str, #[source] Box<dyn Error+Send+Sync>) to vendored libsignal (ApplicationCallbackError requires UnwindSafe, which StoreError::Backend doesn't satisfy). 4 call sites in signal_adapter/signal.rs switched. Documented divergence from upstream Signal.
6. refactor(store)! — Largest commit. StoreError's 4 String variants (Database, Connection, Migration, Serialization) all gained #[source] Box<dyn Error+Send+Sync>. New variants: Validation(String) for descriptive validation, RetriesExhausted { op }, InvalidConfig(String). Dead variants removed (Backend, NotFound, DeviceNotFound). 30+ call sites in sqlite-storage updated. New is_database_busy_or_locked() helper walks the source chain — replaces the substring matches!(... msg.contains(\"locked\")) anti-pattern in client.rs:2510. Retired db_err() helper (was wrap-already-wrapped — backend already returned StoreError).

Breaking changes

Surface-level (anyone matching variants):

• PairCodeError::CryptoError, PairCodeError::RequestFailed removed; replaced by typed variants and the new PairError wrapper.
• Client::pair_with_code returns PairError (not PairCodeError).
• SocketError::Crypto, SocketError::NoiseHandshake removed.
• NoiseError::CryptoError removed; NoiseError no longer Clone.
• wacore::request::IqError::Network removed.
• IqError gained EncryptSend + ClientState; pattern matching needs to cover them.
• MediaDecryptionError::Decryption(String) now carries DecryptionError.
• StoreError::{Database, Connection, Migration, Serialization} carry typed sources, not Strings; Backend, NotFound, DeviceNotFound removed.
• db_err helper removed.
• SignalProtocolError got a new BackendError variant (vendored libsignal divergence).

Display messages on most touched variants are lowercased and dropped the : {0} suffix when source is preserved (avoids duplication when consumers walk the chain via tracing::error!(error = ?e) or anyhow).

Source-chain tests

Each touched enum has a test verifying error.source() returns the expected concrete type via downcast_ref:

• wacore::pair_code::tests::invalid_primary_ephemeral_key_preserves_curve_source
• wacore::pair_code::tests::bundle_aead_preserves_crypto_provider_source
• whatsapp_rust::pair_code::tests::pair_error_request_failed_preserves_iq_source
• whatsapp_rust::pair_code::tests::pair_error_paircode_transparent_walks_to_curve_error
• whatsapp_rust::socket::error::tests::cipher_preserves_noise_source_through_socket_error (two-hop downcast)
• whatsapp_rust::socket::error::tests::marshal_preserves_binary_error_source
• wacore::download::tests::media_decryption_decryption_preserves_aes_cbc_source
• wacore::download::tests::media_decryption_mac_preserves_crypto_error_source
• whatsapp_rust::features::mex::tests::invalid_jid_preserves_jid_error_source
• whatsapp_rust::features::mex::tests::request_preserves_iq_error_source
• wacore_libsignal::protocol::error::tests::backend_error_preserves_typed_source_via_downcast
• wacore::store::error::tests::database_preserves_typed_source_via_downcast
• wacore::store::error::tests::is_busy_or_locked_walks_chain (+ negative case)

Acceptance criteria

• grep -rn 'map_err.*\.to_string()' src wacore/src returns only the two annotated // reason: lines in wacore/src/store/device.rs (serde boundary, no chain support).
• cargo test --workspace --exclude e2e-tests --exclude bench-integration — all passing.
• cargo clippy --workspace --exclude e2e-tests --exclude bench-integration --all-targets -- -D warnings — clean.

Test plan

• cargo test --workspace --exclude e2e-tests
• cargo clippy --workspace -- -D warnings
• cargo test -p e2e-tests (requires mock server — not run locally)
• Smoke-test Veloz against this branch to confirm the high-level error surface still works at the consumer side

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • More reliable detection of database “busy/locked” conditions and improved keepalive/error classification.
  • Preserve underlying error sources for crypto, marshal, and media decryption failures so root causes are retained.

• New Features

  • Pair-code API now exposes a dedicated error type with richer diagnostics.
  • Validation now surfaces typed invalid-JID errors.

• Refactor

  • Broad overhaul of error handling: structured error sources replace stringified errors across storage, noise/crypto, handshake and persistence layers.

Walkthrough

This PR replaces many string-based error payloads with typed error variants that preserve error sources and adds contextual error helpers; it updates error propagation and classification across socket, IQ/request, store, noise, signal, pair-code, and download components.

Changes

Cohort / File(s)	Summary
Socket layer src/socket/error.rs, src/socket/noise_socket.rs	SocketError now has typed Cipher(NoiseError) and Marshal(BinaryError) variants; decrypt and marshal failures return typed variants preserving sources.
IQ / request layer src/request.rs, wacore/src/request.rs	IqError adds EncryptSend and ClientState variants, removes Network(String), and preserves anyhow::Error as EncodeError source; mappings from ClientError are more specific.
Store surface & persistence wacore/src/store/error.rs, src/store/error.rs, src/store/persistence_manager.rs, wacore/src/store/persistence.rs, storages/sqlite-storage/src/sqlite_store.rs	StoreError variants now wrap typed boxed sources; new Validation, RetriesExhausted { op }, InvalidConfig; db_err removed; persistence and sqlite store propagate backend errors directly.
Signal protocol backend wacore/libsignal/src/protocol/error.rs, src/store/signal.rs, src/store/signal_adapter.rs	Adds SignalProtocolError::BackendError(&'static str, Box<dyn Error>); replaces stringified backend mappings with boxed-source BackendError.
Noise cipher wacore/noise/src/error.rs, wacore/noise/src/state.rs, wacore/src/handshake/mod.rs	NoiseError loses Clone, replaces generic crypto string with Encrypt/Decrypt (wrapping CryptoProviderError) and CiphertextTooShort; re-exported via handshake mod.
Media & Pair-code wacore/src/download.rs, wacore/src/pair_code.rs, src/pair_code.rs	MediaDecryptionError gains typed Decryption(AesCbcDecryptionError) and Mac(CryptoError); PairCodeError refactored to typed crypto variants; new PairError wraps PairCodeError and IqError.
Features & newsletter src/features/mex.rs, src/features/newsletter.rs	MexError adds InvalidJid(#[from] JidError) and simplifies display strings; newsletter JID parsing now uses direct jid_str.parse()? propagation.
Keepalive & client retry behavior src/keepalive.rs, src/client.rs	Keepalive treats IqError::EncryptSend and IqError::ClientState as fatal; client retry logic now uses StoreError::is_database_busy_or_locked(); send_node returns SocketError::Marshal(e) for marshal failures.
Misc tests & docs various tests across modules, wacore/src/store/device.rs comment	Added/updated unit tests to assert error source preservation and busy/locked detection; one doc comment notes serde error flattening.

Sequence Diagram(s)

sequenceDiagram
actor Client
participant Persistence as PersistenceManager/Store
participant Socket
participant IQ as IQ Transport
participant Keepalive

Client->>Persistence: perform backend op (save/load)
alt backend returns busy/locked
    Persistence-->>Client: StoreError (source boxed)
    Client->>Client: StoreError::is_database_busy_or_locked()
    Client-->>Client: decide retry or abort
else backend ok
    Persistence-->>Client: success
end
Client->>Socket: marshal & send node
alt marshal fails
    Socket-->>Client: SocketError::Marshal (BinaryError source)
else encrypt/decrypt fail
    Socket-->>Client: SocketError::Cipher (NoiseError source)
    Socket->>IQ: send IQ
    IQ-->>Keepalive: IqError::EncryptSend / ClientState
    Keepalive->>Keepalive: classify as FatalFailure
end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat: implement MEX request and response handling with error management #203: Directly touches src/features/mex.rs and the MexError changes included here.
• refactor: improve error typesafety #325: Modifies src/client.rs retry logic and error checks tied to StoreError::is_database_busy_or_locked().
• Feat support download unencrypted media #278: Related changes to media decryption/error handling in wacore/src/download.rs that overlap the typed-source error work.

You broke a lot of stringly-typed error plumbing and replaced it with proper source-preserving variants — good, but verify every downstream mapping and test the integration paths end-to-end. Things need to fail with inspectable causes, not opaque strings.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor!: preserve typed error sources across the workspace' directly and accurately describes the main refactoring objective to eliminate string-based error conversions and preserve error source chains.
Description check	✅ Passed	The description comprehensively explains the rationale, scope, breaking changes, and acceptance criteria for preserving typed error sources across the codebase through detailed commit breakdowns and test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 93.28% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/preserve-typed-error-sources

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 `@wacore/src/store/error.rs`:
- Around line 36-58: Change the substring checks in
StoreError::is_database_busy_or_locked to perform case-insensitive matching:
when you compute s = layer.to_string() convert it to lowercase (e.g., s =
s.to_lowercase()) and then check if it contains "locked" or "busy"; keep the
rest of the source-walking loop and logic (is_database_busy_or_locked,
layer/source handling) unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b789f5d9-370d-4a9a-9b9a-f3c8e83baad3

📥 Commits

Reviewing files that changed from the base of the PR and between 897e6b5 and 7e87f89.

📒 Files selected for processing (24)

• src/client.rs
• src/features/mex.rs
• src/features/newsletter.rs
• src/keepalive.rs
• src/pair_code.rs
• src/request.rs
• src/socket/error.rs
• src/socket/noise_socket.rs
• src/store/error.rs
• src/store/persistence_manager.rs
• src/store/signal.rs
• src/store/signal_adapter.rs
• storages/sqlite-storage/src/sqlite_store.rs
• wacore/libsignal/src/protocol/error.rs
• wacore/noise/src/error.rs
• wacore/noise/src/state.rs
• wacore/src/appstate_sync.rs
• wacore/src/download.rs
• wacore/src/handshake/mod.rs
• wacore/src/pair_code.rs
• wacore/src/request.rs
• wacore/src/store/device.rs
• wacore/src/store/error.rs
• wacore/src/store/persistence.rs

[jlucaso1]

@claude review nitpicky

[claude]

Claude finished @jlucaso1's task in 4m 12s —— View job

---

Nitpicky Review — refactor!: preserve typed error sources

• Gather context and understand the PR
• Review error type changes in wacore/ layer
• Review error type changes in src/ (client) layer
• Review store/persistence changes
• Review signal adapter changes
• Review sqlite-storage changes
• Post final review

---

Overall this is excellent work — the principle is sound, the execution is thorough, and the source-chain tests are a strong addition. Below are the nitpicks, roughly ordered from most to least important.

---

1. Dead variant: StoreError::DeviceNotFound still exists

The PR description lists DeviceNotFound as removed, but it's still defined at wacore/src/store/error.rs:32-33 and has zero usages across the entire workspace. Either remove it as claimed, or update the PR description. A dead variant in a #[non_exhaustive]-free enum forces downstream match arms for a case that can never happen.

wacore/src/store/error.rs:32    #[error("device with ID {0} not found")]
wacore/src/store/error.rs:33    DeviceNotFound(i32),

Fix this →

---

2. IqError::ParseError(#[from] anyhow::Error) is a footgun

src/request.rs:35 — ParseError has #[from] anyhow::Error, which means any anyhow::Error produced by ? inside a function returning Result<_, IqError> will silently become a ParseError, even if it has nothing to do with parsing. Meanwhile EncodeError at line 33 correctly uses #[source] (manual construction only).

This isn't new to this PR, but since you're reworking IqError anyway: consider making ParseError also use #[source] and constructing it explicitly at the two call sites (src/request.rs:184,197). This prevents accidental misclassification as the variant set grows.

// src/request.rs
#[error("failed to encode IQ request")]
EncodeError(#[source] anyhow::Error),   // ← manual, good
#[error("failed to parse IQ response")]
ParseError(#[from] anyhow::Error),      // ← auto-converts any anyhow via ?

---

3. signal_adapter.rs: generic "backend" context strings lose provenance

In src/store/signal_adapter.rs:18-23, the signal_err helper forwards to SignalProtocolError::BackendError(context, ...). Several call sites pass just "backend" as the context string (lines 95, 104, 198, 212, 216, 268), which makes it impossible to distinguish which store operation failed when reading logs. Compare with src/store/signal.rs which passes specific strings like "backend put_identity", "backend get_identity", "store_sender_key", "load_sender_key".

Consider passing more descriptive context strings through the adapter too:

// Instead of:
.map_err(signal_err("backend"))

// Consider:
.map_err(signal_err("session_adapter::load_session"))

---

4. EncryptSendError wraps anyhow::Error — inconsistent with the PR's principle

src/socket/error.rs:33-39 — EncryptSendError stores source: anyhow::Error, which is essentially the same string-flattening anti-pattern this PR is eliminating (anyhow erases the concrete type unless you downcast). The PR adds IqError::EncryptSend(#[from] EncryptSendError), so the chain is IqError → EncryptSendError → anyhow::Error → ???.

This is likely out of scope for this PR since EncryptSendError predates it, but worth noting for a follow-up — the encrypt/send pipeline could preserve typed sources the same way the rest of this series does.

---

5. wacore::store::persistence.rs — no consecutive-failure halting

The wacore PersistenceManager::run_background_saver (lines 148-186) has no consecutive-failure counter or halt mechanism. The high-level crate's version at src/store/persistence_manager.rs:157 has MAX_CONSECUTIVE_FAILURES = 10 and saver_halted. This means the wacore saver will silently retry forever on a persistently-broken backend, logging errors each interval.

Not introduced by this PR, but the persistence changes here touched both files — might be worth unifying or at least adding a TODO.

---

6. is_retriable_sqlite_error in sqlite_store vs is_database_busy_or_locked

storages/sqlite-storage/src/sqlite_store.rs:41-49 has is_retriable_sqlite_error which does the same substring matching on Diesel error messages but without case-insensitive comparison. The new StoreError::is_database_busy_or_locked() at wacore/src/store/error.rs:48 correctly lowercases. These two helpers should be consistent — if a driver returns "LOCKED", the store-level helper catches it but the sqlite retry loop won't.

// sqlite_store.rs:44-45 — case-sensitive, could miss "LOCKED" or "Busy"
let msg = info.message();
msg.contains("locked") || msg.contains("busy")

Fix this →

---

7. Missing source-chain test for StoreError::Connection / Migration / Serialization

The test suite has database_preserves_typed_source_via_downcast for the Database variant, but the other three box-carrying variants (Connection, Migration, Serialization) don't have equivalent tests. Since the PR's stated goal is verifying source chains, and all four variants underwent the same String → Box<dyn Error> change, it'd be consistent to cover them all.

---

8. NoiseError::CiphertextTooShort — only checked in decrypt_into, not decrypt

wacore/noise/src/state.rs:183 checks ciphertext.len() < TAG_LEN and returns CiphertextTooShort, but decrypt at line 171 doesn't. The AES-GCM implementation will presumably reject it, but it'll produce a generic Decrypt(CryptoProviderError) rather than the descriptive CiphertextTooShort. Not a bug, but inconsistent.

---

9. Minor: PairError variants both use #[from] — error ambiguity risk

src/pair_code.rs:67-73:

pub enum PairError {
    #[error(transparent)]
    PairCode(#[from] PairCodeError),
    #[error("pair-code IQ request failed")]
    RequestFailed(#[from] IqError),
}

The transparent on PairCode means PairError's Display delegates to PairCodeError's Display, but source() also delegates transparently. This is correct and tested. Just noting: if PairCodeError ever gains an IqError-wrapping variant, the two #[from] impls could create ambiguous conversion paths. The current design is clean though.

---

10. Minor: RetriesExhausted { op: String } could be &'static str

wacore/src/store/error.rs:23-24 — most call sites pass string literals ("identity_write (after 6 attempts)"), but the format!() in sqlite_store.rs:652 forces it to String. If you wanted to avoid the allocation on the hot retry-exhaustion path, you could make op a Cow<'static, str> — though this is genuinely minor and probably not worth the ergonomic cost.

---

Summary

Strong refactoring. The typed source chains are correct, well-tested, and the principle is applied consistently. The main actionable items are:

1. Remove dead DeviceNotFound variant (or update the PR description)
2. Make is_retriable_sqlite_error case-insensitive to match the new is_database_busy_or_locked
3. Consider #[source] over #[from] for IqError::ParseError to prevent silent misclassification

Everything else is advisory or follow-up material.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

wacore/src/store/persistence.rs (1)

176-178: 🧹 Nitpick | 🔵 Trivial

Consider adding consecutive failure tracking like the other PersistenceManager.

I see the module comment says this should be consolidated with src/store/persistence_manager.rs. But until that happens, this implementation just logs save errors and keeps going (lines 176-178). The other implementation has MAX_CONSECUTIVE_FAILURES = 10 and a saver_halted flag to prevent silent data loss.

If the backend is persistently broken, this version will log errors forever without surfacing the problem to the application. That's not ideal for reliability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@wacore/src/store/persistence.rs` around lines 176 - 178, The background save
loop in the method that calls this.save_to_disk().await currently only logs
errors and continues; add logic mirroring PersistenceManager by introducing a
consecutive failure counter and a MAX_CONSECUTIVE_FAILURES threshold and a
saver_halted flag so persistent failures stop further attempts and surface the
error; increment the counter on Err(e) from this.save_to_disk(), reset it on
success, and when the counter reaches MAX_CONSECUTIVE_FAILURES set
saver_halted=true and log/return a fatal error (or otherwise notify the
application) to avoid silent data loss—update the same function that contains
the this.save_to_disk() call and use the existing names
(MAX_CONSECUTIVE_FAILURES, saver_halted, save_to_disk) so it aligns with
persistence_manager.rs behavior.

🤖 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/src/store/persistence.rs`:
- Around line 176-178: The background save loop in the method that calls
this.save_to_disk().await currently only logs errors and continues; add logic
mirroring PersistenceManager by introducing a consecutive failure counter and a
MAX_CONSECUTIVE_FAILURES threshold and a saver_halted flag so persistent
failures stop further attempts and surface the error; increment the counter on
Err(e) from this.save_to_disk(), reset it on success, and when the counter
reaches MAX_CONSECUTIVE_FAILURES set saver_halted=true and log/return a fatal
error (or otherwise notify the application) to avoid silent data loss—update the
same function that contains the this.save_to_disk() call and use the existing
names (MAX_CONSECUTIVE_FAILURES, saver_halted, save_to_disk) so it aligns with
persistence_manager.rs behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d4d95d8f-b380-46ad-a5f1-abfa8ee9e80f

📥 Commits

Reviewing files that changed from the base of the PR and between 7e87f89 and ef16c7e.

📒 Files selected for processing (3)

• src/store/persistence_manager.rs
• wacore/src/store/error.rs
• wacore/src/store/persistence.rs