refactor(error)!: replace anyhow in public APIs with per-domain typed errors - #893
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis is a comprehensive API overhaul. We're replacing vague ChangesTyped Error Hierarchy Migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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. Comment |
|
@claude review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 28 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Claude finished @jlucaso1's task in 3m 39s —— View job ReviewSolid migration overall — the The cubic findings are both legitimate and they point at the same systemic issue, which is the most important thing to address before merging. Findings🔴 P2 —
|
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
7005bf3 to
3e0048e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7005bf3da2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 (3)
src/features/chat_actions.rs (1)
754-766: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider simplifying error conversion.
The explicit
map_err(|e| AppStateError::Internal(e.into()))on line 766 is a bit verbose. SinceAppStateError::Internalhas#[from] anyhow::Error, you could potentially use.map_err(|e| anyhow::anyhow!(e))?to let the#[from]conversion kick in. But honestly, the explicit mapping is fine - it's clear about intent. Just flagging for awareness.🤖 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/chat_actions.rs` around lines 754 - 766, The error conversion in the get_app_state_key call on line 766 can be simplified by leveraging the #[from] attribute that already exists on AppStateError::Internal. Instead of using map_err(|e| AppStateError::Internal(e.into())), you can use the ? operator with a simpler map_err that converts the error type using the From trait, or simply remove the explicit conversion if AppStateError::Internal already implements From for anyhow::Error. This will make the code more concise while maintaining the same error handling behavior.src/features/events.rs (1)
78-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate RSVP
message_secretbefore encryption.
message_secretis caller input. If it is not the expected 32 bytes, the encryption helper’s error now becomesSendError::Internal, so callers can’t match it as a bad request. Validate it up front like the other send-path secret APIs.Proposed fix
pub async fn respond( &self, chat_jid: impl Into<Jid>, event_msg_id: &str, event_creator_jid: &Jid, message_secret: &[u8], response: EventResponseType, extra_guest_count: Option<i32>, ) -> Result<SendResult, SendError> { let chat_jid = &chat_jid.into(); + if message_secret.len() != 32 { + return Err(SendError::InvalidRequest(format!( + "message_secret must be exactly 32 bytes, got {}", + message_secret.len() + ))); + } let my_jid = self.client.get_pn().ok_or(SendError::NotLoggedIn)?;🤖 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/events.rs` around lines 78 - 104, The message_secret parameter is caller input that should be validated before encryption. Add validation upfront to check that message_secret is exactly 32 bytes in length before calling event::encrypt_event_response_with_secret. If the length is incorrect, return a SendError that properly indicates a bad request rather than letting the encryption helper convert it to SendError::Internal. This validation should occur after the my_base calculation and before the encrypt_event_response_with_secret call, consistent with how other send-path secret APIs handle this.src/features/comments.rs (1)
53-130: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using a fixed-size array directly for
comment_secretto avoid the.expect()call.Look, I need this code to be bulletproof. The
.expect()on line 119 works because you just created a 32-byte Vec three lines above, but this pattern is fragile if someone refactors the secret generation. Direct array initialization eliminates the conversion entirely:- let comment_secret: Vec<u8> = { - use rand::Rng; - let mut secret = vec![0u8; 32]; - rand::make_rng::<rand::rngs::StdRng>().fill_bytes(&mut secret); - secret - }; + let comment_secret: [u8; 32] = { + use rand::Rng; + let mut secret = [0u8; 32]; + rand::make_rng::<rand::rngs::StdRng>().fill_bytes(&mut secret); + secret + };Then line 116-119 becomes just
let secret = comment_secret;- no conversion, no.expect().As per coding guidelines: "Never use
.unwrap()outside tests" - and.expect()is functionally equivalent. This refactor removes the need entirely.🤖 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/comments.rs` around lines 53 - 130, The `comment_secret` is created as a Vec<u8> and then converted to a [u8; 32] array using `.try_into().expect()`, which is fragile and violates the no-.expect() guideline. Instead, directly initialize `comment_secret` as a fixed-size array [u8; 32] in the variable declaration by using a fixed-size array literal or the appropriate array initialization method, then simplify the conversion line to just assign `comment_secret` directly to `secret` without any conversion or `.expect()` call.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client.rs`:
- Around line 312-318: The is_transport_unavailable function does not check for
transport failures wrapped inside the ClientError::Iq variant, which can contain
IqError::NotConnected or send-pipeline failures. Update is_transport_unavailable
to handle the ClientError::Iq variant by inspecting the wrapped IqError to
determine if it represents a transport unavailability condition, ensuring that
wrapped transport failures are properly detected for retry and reconnect logic.
In `@src/features/blocking.rs`:
- Around line 42-50: The resolve_lid_pn function at line 42 requires a PN↔LID
mapping to exist, but the unblock method at line 74 doesn't actually need this
mapping since UpdateBlocklistSpec::unblock only requires a LID. When a caller
provides a LID directly, the unblock path should not fail due to a missing
mapping. Refactor the unblock logic to check if the provided jid is already a
valid LID and use it directly without calling resolve_lid_pn, only attempting
the mapping resolution when necessary for PN jids.
In `@src/features/community.rs`:
- Around line 21-34: Add an `InvalidRequest(String)` variant to the
`CommunityError` enum in the file to maintain consistency with `NewsletterError`
and `GroupError`. Then locate the two usages of `MexError::PayloadParsing` (at
lines 265 and 312) in the community module where validation failures or
malformed responses are handled, and replace them with
`CommunityError::InvalidRequest(...)` with an appropriate error message,
ensuring the error message clearly describes the invalid request or parsing
failure.
In `@src/send.rs`:
- Around line 58-62: The validation errors created in send_message_impl are
being generated as plain anyhow errors instead of typed ClientError variants,
which causes them to fall through to SendError::Internal in the from_anyhow
function. Replace the anyhow invocations that create validation failures in
send_message_impl (particularly in the sections around lines 1332-1343 and
1365-1388) with ClientError::InvalidRequest instead, so they can be properly
downcast in the from_anyhow match expression and converted to the matchable
SendError::InvalidRequest variant.
---
Outside diff comments:
In `@src/features/chat_actions.rs`:
- Around line 754-766: The error conversion in the get_app_state_key call on
line 766 can be simplified by leveraging the #[from] attribute that already
exists on AppStateError::Internal. Instead of using map_err(|e|
AppStateError::Internal(e.into())), you can use the ? operator with a simpler
map_err that converts the error type using the From trait, or simply remove the
explicit conversion if AppStateError::Internal already implements From for
anyhow::Error. This will make the code more concise while maintaining the same
error handling behavior.
In `@src/features/comments.rs`:
- Around line 53-130: The `comment_secret` is created as a Vec<u8> and then
converted to a [u8; 32] array using `.try_into().expect()`, which is fragile and
violates the no-.expect() guideline. Instead, directly initialize
`comment_secret` as a fixed-size array [u8; 32] in the variable declaration by
using a fixed-size array literal or the appropriate array initialization method,
then simplify the conversion line to just assign `comment_secret` directly to
`secret` without any conversion or `.expect()` call.
In `@src/features/events.rs`:
- Around line 78-104: The message_secret parameter is caller input that should
be validated before encryption. Add validation upfront to check that
message_secret is exactly 32 bytes in length before calling
event::encrypt_event_response_with_secret. If the length is incorrect, return a
SendError that properly indicates a bad request rather than letting the
encryption helper convert it to SendError::Internal. This validation should
occur after the my_base calculation and before the
encrypt_event_response_with_secret call, consistent with how other send-path
secret APIs handle this.
🪄 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: e1037d55-a871-4cb5-92c9-455fcf9dc9bb
📒 Files selected for processing (28)
src/bot.rssrc/client.rssrc/client/context_impl.rssrc/client/iq_ops.rssrc/client/messaging.rssrc/features/blocking.rssrc/features/chat_actions.rssrc/features/chatstate.rssrc/features/comments.rssrc/features/community.rssrc/features/contacts.rssrc/features/events.rssrc/features/groups.rssrc/features/labels.rssrc/features/media_reupload.rssrc/features/mod.rssrc/features/newsletter.rssrc/features/polls.rssrc/features/presence.rssrc/features/profile.rssrc/features/reaction.rssrc/features/signal.rssrc/features/status.rssrc/features/tctoken.rssrc/lib.rssrc/request.rssrc/send.rstests/bench-integration/src/main.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ff4108063
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/send.rs`:
- Around line 59-65: Add a specific match arm for the `ClientError::Iq` variant
in the error downcast block (in the match statement that handles
`err.downcast::<ClientError>()`) to extract and flatten the inner `IqError` to
`SendError::Iq`. This new arm should be placed before the catch-all `Ok(client)`
arm so that `ClientError::Iq` errors are consistently routed to `SendError::Iq`
regardless of how they arrive, ensuring the same error type always maps to the
same `SendError` variant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… errors The public API leaked ~5 inconsistent error families (anyhow::Error, MexError, PresenceError, IqError, ClientError) with no rule, often mixed within a single module. This replaces them with one consistent typed error per feature domain, all built on a shared ClientError base. - ClientError becomes the DRY transport/connection foundation (adds Iq(#[from] IqError) + a transparent anyhow catch-all). Each domain error embeds it via #[from]; it is not an umbrella -- the per-domain types stay the public return types. - New/extended per-domain errors (thiserror, #[non_exhaustive]): SendError (send path + MessageContext), GroupError, ContactError, StatusError(=SendError), BlockingError, ChatStateError, CommunityError, NewsletterError, ProfileError, PollError, SignalError, AppStateError (chat actions + labels), TcTokenError, MediaReuploadError; PresenceError extended. - anyhow no longer appears in any migrated public signature; it remains only internally for ? plumbing, surfaced through a last-resort transparent Internal variant where a typed case does not yet exist. - Re-export every public domain error + ClientError + IqError at the crate root, and add the common ones (ClientError, IqError, SendError) to the prelude. BREAKING CHANGE: public return types changed across the send path (send_message/send_text/forward_message/revoke/edit/pin/keep, status), MessageContext (send_message/reply/reply_quoting/edit_message/revoke_message/ react), and every feature module (groups, contacts, blocking, chatstate, presence, community, newsletter, profile, polls, events, comments, reactions, signal, tc_token, media reupload, chat actions, labels).
send_message_impl threads anyhow internally, so its ClientError::NotLoggedIn was funneled into SendError::Internal via the blanket #[from] anyhow::Error at the public send wrappers instead of the matchable SendError::NotLoggedIn. Add SendError::from_anyhow to downcast the propagated anyhow back to ClientError and route NotLoggedIn (and other ClientError cases) to their typed variants, applied at the send_message_impl / require_pn wrapper call sites.
080ed97 to
659a852
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080ed97fe5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…p-edit/newsletter-reaction
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0a31b72a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/messaging.rs`:
- Around line 157-160: The error message in the InvalidRequest branch for the
is_newsletter() check in the edit_message_encrypted function provides misleading
guidance by suggesting to use edit_message, which will also fail downstream for
newsletters. Update the error message string to point callers directly to the
correct newsletter edit API endpoint instead of suggesting edit_message,
ensuring the remediation hint actually resolves the issue rather than leading to
another dead-end error.
In `@src/features/signal.rs`:
- Around line 250-259: The error handling in the validate_session method
manually wraps the error with anyhow::anyhow!() which creates unnecessary
indirection since SignalError::Internal already has #[from] anyhow::Error. Use
anyhow's context method instead to add the descriptive message while preserving
the original error chain. Replace the manual anyhow::anyhow!("session check
failed: {e}") call with e.context("session check failed") to make the error
handling more idiomatic and consistent with anyhow's patterns.
In `@src/send.rs`:
- Around line 32-40: Remove the `#[from]` attribute from the `Client` variant
(which currently has the `ClientError` type) to prevent automatic error
conversions that bypass the typed surface. Then implement a manual
`From<ClientError>` trait implementation for `SendError` that properly flattens
`ClientError` conversions—specifically mapping `ClientError::NotLoggedIn`
directly to `SendError::NotLoggedIn` and other `ClientError` variants to
`SendError::Client()`. This mirrors the flattening pattern already implemented
in the `from_anyhow()` method so that using `?` with `ClientError` produces the
correct matchable `SendError` variant instead of wrapping it in the `Client`
enum variant.
- Around line 58-75: The from_anyhow method needs to recover GroupError
exceptions before they collapse into SendError::Internal. Add a new downcast
check for GroupError after the existing IqError check (in the Err(other) branch)
but before the final SendError::Internal fallback. This new check should handle
all four GroupError variants (Iq, Mex, InvalidRequest, and Internal) by mapping
the Iq variant to SendError::Iq and routing the other variants appropriately to
their corresponding SendError equivalents, ensuring GroupError errors follow the
documented SendError::Iq pattern matching path instead of becoming
SendError::Internal.
🪄 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: 8666a4a2-9388-441a-8ae4-374fafe783d4
📒 Files selected for processing (28)
src/bot.rssrc/client.rssrc/client/context_impl.rssrc/client/iq_ops.rssrc/client/messaging.rssrc/features/blocking.rssrc/features/chat_actions.rssrc/features/chatstate.rssrc/features/comments.rssrc/features/community.rssrc/features/contacts.rssrc/features/events.rssrc/features/groups.rssrc/features/labels.rssrc/features/media_reupload.rssrc/features/mod.rssrc/features/newsletter.rssrc/features/polls.rssrc/features/presence.rssrc/features/profile.rssrc/features/reaction.rssrc/features/signal.rssrc/features/status.rssrc/features/tctoken.rssrc/lib.rssrc/request.rssrc/send.rstests/bench-integration/src/main.rs
…dError variants
Close the "typed error masked as SendError::Internal" class at the root by
flattening in every conversion to SendError, not only from_anyhow:
- Drop #[from] on SendError::Client and add a manual From<ClientError> so a
bare `?` maps NotLoggedIn -> SendError::NotLoggedIn and Iq -> SendError::Iq
instead of nesting under Client(..).
- Add From<GroupError> for SendError plus a GroupError downcast in from_anyhow
(before the ClientError check) so a group-metadata IQ failure from
query_info surfaces as SendError::Iq, not Internal.
- Point the edit_message_encrypted newsletter hint at newsletter().edit_message
(edit_message also rejects newsletters).
- Use the idiomatic e.context("session check failed") in validate_session.
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 (2)
src/client/messaging.rs (2)
106-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTyped validation errors collapse to
Internalhere.Look, this is exactly the kind of thing that makes me question whether we're actually shipping typed errors or just pretending.
send_message_implreturnsanyhow::Errorand wraps typedSendError::InvalidRequestvalidation failures (like newsletter rejection at send.rs:1407). Using bare?here routes everything through#[from] anyhow::ErrorintoSendError::Internal, losing the typed surface.
revoke_message_innerinsend.rs:1296gets this right with.map_err(SendError::from_anyhow)?. This needs the same treatment.Proposed fix
self.send_message_impl( to, &edit_container_message, None, false, false, Some(crate::types::message::EditAttribute::MessageEdit), vec![], None, ) - .await?; + .await + .map_err(crate::send::SendError::from_anyhow)?;🤖 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/messaging.rs` around lines 106 - 116, The bare `?` operator on the `send_message_impl` call causes typed validation errors to collapse to `SendError::Internal` instead of being properly converted. Replace the `.await?` with `.await.map_err(SendError::from_anyhow)?` on the `send_message_impl` result to properly preserve typed errors like `SendError::InvalidRequest` during conversion from `anyhow::Error`, following the same pattern used in `revoke_message_inner` in send.rs.
193-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame
from_anyhowgap asedit_message_inner.This is the same issue. If someone calls
edit_message_encryptedtargeting a newsletter JID,send_message_implraises a typedSendError::InvalidRequest, but bare?here loses it toInternal. The upstream validation at line 157 catches the obvious case, but any typed errors from deeper insend_message_implstill get swallowed.Be consistent with
revoke_message_inner.Proposed fix
self.send_message_impl( to, &envelope, None, false, false, Some(crate::types::message::EditAttribute::MessageEdit), vec![], None, ) - .await?; + .await + .map_err(SendError::from_anyhow)?;🤖 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/messaging.rs` around lines 193 - 203, The edit_message_encrypted method is losing typed error information when calling send_message_impl by using the bare ? operator, which converts typed SendError variants like InvalidRequest to Internal errors. Examine how revoke_message_inner properly handles errors from send_message_impl to preserve typed errors (likely using from_anyhow conversion or similar pattern), then apply the same error handling approach to the send_message_impl call in edit_message_encrypted to ensure that typed errors like SendError::InvalidRequest are preserved rather than being converted to generic Internal errors.
🤖 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/messaging.rs`:
- Around line 106-116: The bare `?` operator on the `send_message_impl` call
causes typed validation errors to collapse to `SendError::Internal` instead of
being properly converted. Replace the `.await?` with
`.await.map_err(SendError::from_anyhow)?` on the `send_message_impl` result to
properly preserve typed errors like `SendError::InvalidRequest` during
conversion from `anyhow::Error`, following the same pattern used in
`revoke_message_inner` in send.rs.
- Around line 193-203: The edit_message_encrypted method is losing typed error
information when calling send_message_impl by using the bare ? operator, which
converts typed SendError variants like InvalidRequest to Internal errors.
Examine how revoke_message_inner properly handles errors from send_message_impl
to preserve typed errors (likely using from_anyhow conversion or similar
pattern), then apply the same error handling approach to the send_message_impl
call in edit_message_encrypted to ensure that typed errors like
SendError::InvalidRequest are preserved rather than being converted to generic
Internal errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4087275f-0181-458b-9fb4-b37ae0b20b7f
📒 Files selected for processing (3)
src/client/messaging.rssrc/features/signal.rssrc/send.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c02ee128d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
edit_message_inner/edit_message_encrypted_inner used a bare `?` on send_message_impl, collapsing its typed SendError::InvalidRequest (newsletter/status rejections) into SendError::Internal; route through from_anyhow like revoke_message_inner does. resolve_outgoing_addon_parent returned anyhow::Error, so reaction/comment sends surfaced caller/login failures (missing key id, logged out) as SendError::Internal. Return SendError directly with InvalidRequest/NotLoggedIn so callers can match them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/message/msg_secret.rs`:
- Around line 798-809: The code currently falls back to using remote_jid when
participant is missing, but for group chats remote_jid is the chat JID not the
author, causing incorrect secret lookups and misleading errors. Before the
ok_or_else call that processes target_key.remote_jid, add a check to reject
group targets when participant is absent and from_me is not true. If these
conditions are met, return a SendError indicating that participant is required
for group messages, preventing the fallback to remote_jid and failing fast with
a clear error message.
🪄 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: abbb4ac7-fa9c-40a6-87d0-2975c7d86aeb
📒 Files selected for processing (2)
src/client/messaging.rssrc/message/msg_secret.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d20ac09b28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
resolve_outgoing_addon_parent fell back to remote_jid as the author when participant was absent; for group parents remote_jid is the group JID, not the sender, so the secret lookup ran under the wrong key and surfaced a misleading "no secret stored" error. Reject group parents without participant via InvalidRequest instead. is_community_announce_group returned anyhow::Error, so a failing group metadata IQ during send_reaction collapsed to SendError::Internal. Return GroupError so the send path flattens it to SendError::Iq via From<GroupError>.
2 participants
What
The public API leaked ~5 inconsistent error families (
anyhow::Error,MexError,PresenceError,IqError,ClientError) with no rule, often mixed within a single module (e.g.groups.rsreturnedanyhowfromleavebutMexErrorfromset_limit_sharing;presence.rsmixedPresenceErrorandanyhow). This replaces them with one consistent typed error per feature domain, all built on a sharedClientErrorbase.anyhowno longer appears in any migrated public signature.Design
ClientError— the DRY transport/connection foundation (NotConnected,NotLoggedIn,Socket,EncryptSend, nowIq(#[from] IqError)+ a transparentanyhowcatch-all). Each domain error embeds it via#[from]. It is not an umbrella: the per-domain types remain the public return types.thiserror,#[non_exhaustive],#[from]for chainingClientError/IqError/MexErrorso internal?keeps working. Common actionable cases (NotLoggedIn, IQ failures,Server { code, text }, invalid-request validation) are typed explicitly; a last-resort transparentInternal(#[from] anyhow::Error)keeps the migration tractable where no typed case exists yet.ClientErrorandIqErrorare re-exported at the crate root, andClientError/IqError/SendErrorare added to theprelude.Migration (public return types changed)
Client):send_message,send_text,forward_message,send_message_with_options,revoke_message,edit_message,edit_message_encrypted,keep_message,pin_message/unpin_message,set_chat_disappearing_timer,send_reaction->SendError.MessageContext(bot.rs):send_message,reply,reply_quoting,edit_message,revoke_message,react->SendError.GroupError; contacts ->ContactError; status ->SendError; blocking ->BlockingError; chatstate ->ChatStateError; presence ->PresenceError(extended withClient(#[from] ClientError)); community ->CommunityError; newsletter ->NewsletterError; profile ->ProfileError; polls ->PollError; events/comments ->SendError; signal ->SignalError; tc_token ->TcTokenError; media reupload ->MediaReuploadError; chat actions + labels -> sharedAppStateError. (spam_reportalready returnedIqError.)Breaking change
Every listed method's
Errvariant changed. Callers using?into ananyhowcontext are unaffected (each new error implementsstd::error::Error); callers that matched onanyhow::Errormust now match the typed variant.IqError::ClientStateis now boxed to break theClientError <-> IqErrortype cycle.Verification
cargo fmt --all,cargo build --all,cargo build --all --examples,cargo clippy --all --tests(no warnings), andcargo test -p wacore -p whatsapp-rustall pass locally (e2e not run — needs the mock server).