refactor(errors): make the public error surface recoverable without string matching

[jlucaso1]

Summary

A consumer classifying failures by walking std::error::Error::source() got a 403 back from a group operation and could recover neither the code nor the type: GroupError::Iq is #[error(transparent)], and transparent delegates source() to the wrapped error's own source. IqError::ServerError is a leaf, so the chain ended before it and the typed node vanished, leaving only Display text to parse. The same held for the 409 on a group description update.

The fix turns out to be free. #[error("{0}")] renders byte-for-byte the same text as transparent while keeping the wrapped error reachable as source(), so making the chain lossless changes no message anywhere. That is done for all 46 occurrences.

Losslessness alone still leaves every consumer writing a chain walk and knowing that three different types can carry a server rejection (wacore::request::IqError, crate::request::IqError, and ServerErrorCode, which exists only to move one across a crate boundary). ErrorChainExt is that walk written once, as an extension trait with a blanket impl over std::error::Error — so a domain error added next month answers the same questions without implementing anything.

Audit

53 public error enums (38 in src/, 15 in wacore/src/) and 46 #[error(transparent)] occurrences, 45 of them on public enums and one on the private PluginCallbackError.

Classifying the 46 against "does it hide a typed leaf":

Class	Count	Verdict
(a) hides a typed leaf	33	Broken. Every non-anyhow transparent in the crate falls here — each wraps a type whose common variants are leaves (IqError, MexError, StoreError, ClientError::NotConnected, SignalProtocolError, …).
(b) harmless passthrough	0	None found.
(c) Internal(#[from] anyhow::Error)	13	Also broken, conditionally. transparent skips the anyhow head, so a bare anyhow::Error::new(leaf) loses the leaf outright; only a .context()-wrapped value survives. context_impl.rs:40 builds exactly the bare form around a ServerErrorCode.

So all 46 were converted, not just the 33. I verified the thiserror semantics empirically rather than from the docs, for both the .context() and bare cases, before touching the repo.

#[non_exhaustive]: 4 of the 53 lacked it (ShortcakeError, PairCodeError, ChatstateParseError, DirtyBitParseError). Added.

anyhow on the surface: 31 public variants carry anyhow::Error and all 31 stay. The policy this PR sets is that they stop being dead ends rather than being erased: after the change the head of the anyhow chain is exposed as source() and is downcastable, so a ServerErrorCode or IqError buried in one is recoverable by type. What a consumer may assume about an Internal variant is only that: it is a failure with no dedicated variant yet, whose chain is walkable. Not eradicating them here keeps this batch to the shape of errors, which is what it is about.

Two IqError types: left as-is. They are not trivially unifiable (the src one adds Socket, EncryptSend, ClientState, Disconnected, EncodeError, ParseError), and collapsing them is a separate refactor touching ~49 match sites. What made the duplication a consumer problem was having to downcast both; ErrorChainExt absorbs that, and a test pins that both answer identically.

Changes

• Replace all 46 #[error(transparent)] with #[error("{0}")], adding #[source] to the one field that had neither #[from] nor #[source] (SendError::Client). Each error's own Display output is unchanged, which a test asserts.
  • Migration, chain rendering. Per-variant Display is identical, but the chain now renders differently: a wrapping variant prints what it wraps, so code that concatenates every node sees the same sentence repeated. CommunityError::Group(GroupError::Iq(..)) is three nodes rendering one sentence three times. Avoiding that is precisely what transparent bought, and it is the cosmetic price of keeping the wrapped error downcastable. Consumers that join the chain (a display_chain helper, a tracing layer printing causes) should print the innermost cause or collapse equal neighbours.
• Add src/error.rs: ErrorChainExt, ServerRejection<'a>, Sources<'a>, re-exported from the crate root. Read-only view over the existing errors; no new error type, no parallel hierarchy.
• Add #[non_exhaustive] to the 4 error enums missing it (ShortcakeError, PairCodeError, ChatstateParseError, DirtyBitParseError). Breaking for anyone matching them exhaustively outside the crate, which fails to compile with E0004. Migration: add a _ => {} arm. PairCodeError is the one with a known external matcher, so expect that build to need the arm.
• Give wacore::request::IqError an is_transport_unavailable() method (new public API, non-breaking) and call it, instead of restating its transport variant list at the call site. Its two sibling types already owned that judgement as a method; the inline copy would have diverged in silence if a transport variant were added.
• Give ConnectError, HandshakeError and both IqError types an is_timeout() method, each an exhaustive match, instead of restating variant lists at the call site. Same reasoning as the transport fix above: a second timeout variant would otherwise have diverged unnoticed, and an exhaustive match forces a new variant to be classified rather than silently defaulting to false.
• is_timeout() also recovers ConnectError::Timeout and HandshakeError::Timeout. Both are timeouts the crate already distinguishes, so this recovers a distinction rather than inventing one; before, a connect that ran out of time answered false.
• Replace the hand-written 3-way downcast walker in src/message/special.rs with ErrorChainExt::is_transport_unavailable. Behaviour-identical: the only downcast target it adds is wacore::request::IqError, which cannot appear in that chain (the sole conversion site, src/request.rs:116, maps it into crate::request::IqError immediately). The 5 existing assertions for that function pass untouched.
• Update pair_error_paircode_transparent_walks_to_curve_error — see below.

The one test that had to change

src/pair_code.rs had a test asserting the lossy behaviour by name: it walked one hop from PairError::PairCode and expected to land on CurveError, "because PairError::PairCode is transparent". With the wrapper no longer erasing itself, hop 1 is the PairCodeError and CurveError is hop 2. Rewritten to assert the stronger chain, keeping its Display assertion as-is (it still passes, which is itself evidence the text did not move). No other test in the repo needed adapting.

Guarantees

Given any public error from this crate, without string matching and without knowing the concrete domain enum:

• server_rejection() returns the code, text, XMPP error class and backoff of an IQ-level rejection, wherever in the chain it sits and whichever of the three carrier types holds it.
• is_timeout() answers the same way, and covers a connect or handshake step that never completed as well as a request that got no answer.
• is_transport_unavailable() and store_failure() answer the same way.
• The answer is identical across group, newsletter, profile, blocking, community, contacts and tctoken — a test runs one type-agnostic helper over all of them, including two-domain nesting and the anyhow-carried case.
• sources() exposes the raw chain for anything not modelled above.
• Adding a variant to any error enum does not break consumers.
• No anyhow, serde or extra runtime is required to use any of this.

Out of contract, deliberately: there is no "invalid input", "protocol violation" or "internal" category. Each domain spells those as its own InvalidRequest(String)-style variant with no shared representation, so exposing them would mean inventing a taxonomy rather than recovering one. MexError::ExtensionError is not reported by server_rejection() either — its code is a GraphQL extension code, a different space from the IQ code attribute, and merging them would make the number mean two things. A negative test pins both.

Cost

• size_of: unchanged for all 51 public error types measurable under --all-features (checked before/after; the diff is empty). Expected, since the change is attribute-only and touches no field. Nothing needed boxing.
• Hot path: zero. No format!, to_string() or allocation moved out of a failure branch. ErrorChainExt runs only when a caller asks, on an error value it already holds; it borrows and allocates nothing. The single production call site is special.rs, already inside an error branch, and it replaced a walk that did strictly more downcasts.
• Binary size: left to the binary-size job, which measures against the main baseline — more trustworthy than two local fat-LTO builds. I will report the delta from the PR comment.

Validation

cargo fmt --all --check
cargo clippy --workspace --exclude e2e-tests --all-features --all-targets -- -D warnings
cargo test -p wacore --lib            # 1220 passed
cargo test -p whatsapp-rust --lib     # 1163 passed
cargo test --tests                    # all green, incl. 23 in tests/error_surface.rs
cargo test --doc -p whatsapp-rust
cargo doc --no-deps -p whatsapp-rust -p wacore

Both surface guards were verified to actually fail: reintroducing one transparent and removing one #[non_exhaustive] fails them, plus 7 behavioural tests independently.

They parse the sources with syn rather than scanning them as text, which is a change of mechanism worth calling out. The text version fell to two shapes, both reproduced before being fixed: a blank line between #[non_exhaustive] and the declaration hid the attribute (a loud false positive), and an unbalanced brace inside a doc comment truncated the enum body by brace balance, so the enum stopped looking like an error enum and a public one missing #[non_exhaustive] passed the guard in silence. A guard that backs a "cannot be forgotten later" claim must not fail open. Seven fixtures now pin the shapes that used to defeat it. syn is added as a dev-dependency and is already in Cargo.lock via wacore/derive, so this adds a dependency edge and no new crate, and nothing reaches the shipped library.

The no-transparent scan is deliberately a blanket policy rather than a narrow fix: it covers private enums too (the private PluginCallbackError was converted for that reason), because today's private error is tomorrow's public one and the attribute is invisible at the point where it hurts. A future case where erasure is genuinely wanted should be argued in review, not waived silently.

Semver Checks is red, as it is on main: it compares against the last published release, so it carries every change since then. Of the 10 enums it lists under enum_marked_non_exhaustive, this PR is responsible for 3 (ChatstateParseError, DirtyBitParseError, PairCodeError; ShortcakeError is the fourth marker added here but does not appear in its output). The other 7 (AppStateSyncError, CallAction, HistorySyncError, IqError, MediaDecryptionError, ReceiptType, StoreError) are pre-existing drift: git diff main...HEAD shows this branch adds #[non_exhaustive] in exactly four files, none of them theirs. The remaining lint categories are likewise untouched by this branch, and the additions it does make (the new is_timeout/is_transport_unavailable methods and the error module) are non-breaking.

Full matrix (incl. wasm32) left to CI.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e36cc3e6-259e-4671-a046-100c34cc41e8

📥 Commits

Reviewing files that changed from the base of the PR and between 66a16c5 and e2c7b8a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (35)

• Cargo.toml
• src/bot.rs
• src/client.rs
• src/client/voip.rs
• src/error.rs
• src/features/blocking.rs
• src/features/chat_actions.rs
• src/features/chatstate.rs
• src/features/community.rs
• src/features/contacts.rs
• src/features/groups.rs
• src/features/media_reupload.rs
• src/features/newsletter.rs
• src/features/polls.rs
• src/features/presence.rs
• src/features/profile.rs
• src/features/signal.rs
• src/features/stanza.rs
• src/features/tctoken.rs
• src/handshake.rs
• src/lib.rs
• src/message/special.rs
• src/pair_code.rs
• src/plugins/events.rs
• src/plugins/mod.rs
• src/request.rs
• src/send/mod.rs
• tests/error_surface.rs
• wacore/src/appstate_sync.rs
• wacore/src/download.rs
• wacore/src/iq/chatstate.rs
• wacore/src/iq/dirty.rs
• wacore/src/pair_code.rs
• wacore/src/request.rs
• wacore/src/shortcake.rs

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Added consistent error-chain inspection for identifying timeouts, unavailable transport, storage failures, and server rejections.
  • Added helpers for checking timeout and connectivity conditions across connection, handshake, and request errors.
  • Exposed error inspection utilities through the public API.

• Improvements

  • Error messages now consistently preserve and display underlying details across features.
  • Public error types are more forward-compatible, allowing new variants in future releases.

• Tests

  • Expanded coverage for error formatting, source-chain traversal, and error classification.

Walkthrough

The PR adds public typed error-chain recovery utilities, standardizes wrapped error display and source preservation across src and wacore, updates transport recovery integration, marks public error enums non-exhaustive, and adds comprehensive error-surface tests.

Changes

Error surface and chain recovery

Layer / File(s)	Summary
Error-chain recovery utilities src/error.rs, src/lib.rs, src/handshake.rs, src/request.rs, wacore/src/request.rs	Adds ErrorChainExt, ServerRejection, and Sources, with typed recovery for server rejections, timeouts, transport failures, and store failures; exports the API from the crate root and adds timeout/transport classifiers.
Non-transparent error wrappers src/bot.rs, src/client.rs, src/client/voip.rs, src/features/*, src/plugins/*, src/send/mod.rs, wacore/src/appstate_sync.rs, wacore/src/download.rs	Replaces transparent thiserror formatting with {0} formatting while preserving wrapped sources where required.
Public error contracts src/pair_code.rs, wacore/src/iq/*, wacore/src/pair_code.rs, wacore/src/shortcake.rs	Marks selected public error enums non-exhaustive and preserves the pair-code source layer.
Transport recovery integration src/message/special.rs, wacore/src/request.rs	Routes app-state reconnect classification through ErrorChainExt and adds transport-unavailable classification to core IQ errors.
Error-surface validation Cargo.toml, tests/error_surface.rs, src/pair_code.rs	Adds source scanning and runtime checks for formatting, typed chains, recovery categories, negative cases, and nearest-first traversal.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AppStateKeyShare
  participant ErrorChainExt
  participant ErrorChain
  AppStateKeyShare->>ErrorChainExt: is_transport_unavailable(cause)
  ErrorChainExt->>ErrorChain: walk sources and downcast transport errors
  ErrorChain-->>ErrorChainExt: matching or non-matching error
  ErrorChainExt-->>AppStateKeyShare: reconnect decision

Loading

Possibly related PRs

• oxidezap/whatsapp-rust#893: Introduces typed error-chain structures consumed by the new recovery helpers.
• oxidezap/whatsapp-rust#1052: Shares the app-state reconnect classification flow.
• oxidezap/whatsapp-rust#1069: Relates to stanza error types whose display behavior changes here.

Suggested labels: api-design

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% 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.
Title check	✅ Passed	The title clearly summarizes the main change: making public errors recoverable through source chains instead of string matching.
Description check	✅ Passed	The description is directly related to the changeset and explains the error-surface refactor in detail.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/recoverable-error-surface

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR fixes a longstanding gap in the public error surface: #[error(transparent)] delegates source() to the wrapped error's own source, silently erasing the intermediate node from the chain and making typed recovery impossible without string matching. Replacing all 46 occurrences with #[error("{0}")] preserves byte-identical Display output while keeping every wrapped error reachable by downcast_ref. A new ErrorChainExt trait wraps the common chain-walk patterns so consumers never need to know about the three carrier types or walk the chain manually.

• #[error(transparent)] → #[error("{0}")] applied to all 46 variants (public + private); #[source] added to SendError::Client which had neither #[from] nor #[source]; chain rendering unchanged per a dedicated Display assertion test.
• ErrorChainExt trait added to src/error.rs with blanket impls for concrete types and trait objects, exposing server_rejection(), is_timeout(), is_transport_unavailable(), store_failure(), and sources().
• #[non_exhaustive] added to four previously non-sealed public error enums; wacore::request::IqError::is_transport_unavailable() and is_timeout() added as canonical classification methods.

Confidence Score: 5/5

Safe to merge. The change is attribute-only on error enums with no field layout change, and the new trait is a read-only view that allocates nothing until called.

Every changed error variant is covered by two independent checks: the source-scanning test rejecting any #[error(transparent)] in the workspace, and the chain-preservation test walking and downcasting each variant directly. The Display-identity test rules out message regressions. No production code path or data structure was reshaped.

Files Needing Attention: No files require special attention. The four wacore files that gained #[non_exhaustive] are the only externally breaking changes, and those are intentional and documented.

Important Files Changed

Filename	Overview
src/error.rs	New file: ErrorChainExt trait with blanket impls for concrete types and three dyn Trait shapes; Sources iterator; ServerRejection view struct. Design is sound — the three impl blocks are non-overlapping (blanket has implicit Sized bound, dyn impls cover unsized shapes).
tests/error_surface.rs	New integration test file with source-scanning tests using syn::parse_file, chain-preservation tests, regression tests for the two original reports, and scanner self-tests covering blank-line attribute separation and unbalanced braces in doc comments.
wacore/src/request.rs	Adds is_transport_unavailable() (pub, matches!) and is_timeout() (pub, exhaustive match) to wacore::request::IqError.
src/request.rs	Adds pub(crate) is_timeout() with exhaustive match to crate::request::IqError. pub(crate) is correct since it is only consumed from src/error.rs within the same crate.
src/client.rs	Adds ConnectError::is_timeout() (pub, exhaustive match) delegating to HandshakeError::is_timeout() for the Handshake variant. All transparent conversions applied correctly.
src/handshake.rs	Adds HandshakeError::is_timeout() (pub, exhaustive match). Required by both ErrorChainExt::is_timeout() and ConnectError::is_timeout().
src/send/mod.rs	SendError::Client gains #[source] — the only variant in the PR needing it explicitly because it had neither #[from] nor #[source] before.
src/message/special.rs	Replaces a 3-way inline downcast walker with ErrorChainExt::is_transport_unavailable(). Behaviour-identical.
src/lib.rs	Adds pub mod error and re-exports ErrorChainExt, ServerRejection, Sources. anyhow is already pub use'd, making whatsapp_rust::anyhow::Error valid in the doc example.
Cargo.toml	Adds syn 3.0 to dev-dependencies for tests/error_surface.rs. Test-only; no production dependency added.

_{Reviews (3): Last reviewed commit: "test(errors): parse the sources instead ..." | Re-trigger Greptile}

[github-actions]

📦 Binary size report

Metric	main	PR	Δ
bin size (stripped)	9.89 MiB	9.90 MiB	+1.03 KiB (+0.01%) 🔺
bin .text	7.94 MiB	7.94 MiB	+832 B (+0.01%) 🔺
bin allocated (text+data+bss)	9.89 MiB	9.89 MiB	+244 B (+0.00%) 🔺
llvm-lines wacore	490,402	490,619	+217 (+0.04%) 🔺
llvm-lines wacore copies	16,315	16,315	0
llvm-lines whatsapp-rust lib	688,118	692,035	+3,917 (+0.57%) 🔺
llvm-lines whatsapp-rust lib copies	21,941	21,924	-17 (-0.08%) 🔽
deps crates (Cargo.lock)	471	471	0

.text per crate

Crate	main	PR	Δ
.text whatsapp_rust	1.74 MiB	1.74 MiB	-3.19 KiB (-0.18%) 🔽
.text wacore	652.74 KiB	652.48 KiB	-266 B (-0.04%) 🔽
.text wacore_binary	89.42 KiB	89.42 KiB	0
.text wacore_libsignal	161.89 KiB	161.89 KiB	0
.text wacore_appstate	22.36 KiB	22.36 KiB	0
.text wacore_noise	21.60 KiB	21.60 KiB	0
.text waproto	1.74 MiB	1.74 MiB	0
.text whatsapp_rust_sqlite_storage	514.77 KiB	514.77 KiB	0
.text whatsapp_rust_tokio_transport	39.91 KiB	39.91 KiB	0
.text whatsapp_rust_ureq_http_client	10.40 KiB	10.40 KiB	0
.text std	1.07 MiB	1.07 MiB	-63 B (-0.01%) 🔽
.text other deps	1.88 MiB	1.88 MiB	+4.26 KiB (+0.22%) 🔺

Top movers (cargo-bloat attribution)

Crate	main	PR	Δ
metrics_exporter_prometheus	(absent)	3.96 KiB	+3.96 KiB
whatsapp_rust	1.74 MiB	1.74 MiB	-3.19 KiB (-0.18%)

Baseline: 9329c98f2 (latest main run) · Head: f99451b9e · Graphs

[coderabbitai]

Actionable comments posted: 3

🤖 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/error.rs`:
- Around line 1-58: Trim the module-level documentation above ErrorChainExt to a
concise statement of its purpose, a brief scope summary, and one representative
usage example. Remove detailed explanations of error-chain internals,
crate-specific type inventories, cost claims, and the separate anyhow example;
keep only guidance needed to understand and use the extension trait.

In `@tests/error_surface.rs`:
- Around line 79-100: Update surface_has_no_transparent_error_attribute to
detect #[error(transparent)] even when followed by a trailing comment or other
same-line text, while continuing to scan all Rust sources and report the
offending location. Replace the exact trimmed-line comparison with a robust
match for the attribute itself.
- Around line 44-77: The regression guard in tests/error_surface.rs must stop
using the text heuristics in preceding_attributes(), enum_body(), and
is_error_enum(). Add syn as a development dependency, parse the Rust source into
an AST, and inspect public ItemEnum declarations and their attributes to
identify error enums and validate non_exhaustive reliably, including empty enums
and unusual formatting.

🪄 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: 9fbe3406-bd14-474a-819b-5be24bc4c786

📥 Commits

Reviewing files that changed from the base of the PR and between 7df3a3c and cdeff29.

📒 Files selected for processing (31)

• src/bot.rs
• src/client.rs
• src/client/voip.rs
• src/error.rs
• src/features/blocking.rs
• src/features/chat_actions.rs
• src/features/chatstate.rs
• src/features/community.rs
• src/features/contacts.rs
• src/features/groups.rs
• src/features/media_reupload.rs
• src/features/newsletter.rs
• src/features/polls.rs
• src/features/presence.rs
• src/features/profile.rs
• src/features/signal.rs
• src/features/stanza.rs
• src/features/tctoken.rs
• src/lib.rs
• src/message/special.rs
• src/pair_code.rs
• src/plugins/events.rs
• src/plugins/mod.rs
• src/send/mod.rs
• tests/error_surface.rs
• wacore/src/appstate_sync.rs
• wacore/src/download.rs
• wacore/src/iq/chatstate.rs
• wacore/src/iq/dirty.rs
• wacore/src/pair_code.rs
• wacore/src/shortcake.rs

Dismissed because a newer commit was pushed; Greptile will re-review the current head.