feat(request): keep the reply that describes an IQ error - #1257
Conversation
A successful IQ hands the caller the whole response node. A rejected one was
reduced to four extracted attributes and the stanza itself was dropped, so
anything outside code/text/type/backoff (further <error> attributes, the child
elements XMPP allows, the original bytes) was gone before the caller could look
at it. Which part of a protocol error matters is the caller's judgement, not
something the IQ transport should decide for them.
ServerError now carries the reply as the Arc<OwnedNodeRef> the receive path had
already decoded, so the rejection path costs one refcount bump and the success
path gains no work at all. Box<Node>, the shape Disconnected uses, would have
meant deep-copying a node we already own and would have lost backing_bytes(),
which is the only faithful material for recording or replaying a rejection.
The four extracted fields stay. Nearly every call site in the tree matches
ServerError { code, .. } and reads nothing else, so removing them would make all
of those reparse the common case for no gain.
From<wacore::request::IqError> gives way to IqError::from_response(err,
&response): converting a classification failure without the response it was read
from is precisely the loss this removes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesIQ error response preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IQWaiter
participant RequestResponse
participant IqError
IQWaiter->>RequestResponse: deliver owned IQ response
RequestResponse->>IqError: from_response(error, response)
IqError->>IqError: store response allocation
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 |
|
| Filename | Overview |
|---|---|
| src/request.rs | Introduces the redacted rejection-stanza wrapper and attaches the original decoded response to server IQ errors, with focused identity and preservation tests. |
| wacore/src/request.rs | Renames dropped-detail diagnostics to unread-detail diagnostics without changing response classification behavior. |
| src/test_utils.rs | Adds parser-backed server-error fixtures and returns delivered IQ allocations so tests can verify pointer identity. |
| src/lib.rs | Re-exports RejectionStanza through the crate root and prelude. |
| tests/error_surface.rs | Updates public error-surface fixtures to construct the newly required rejection stanza. |
| src/client/context_impl.rs | Keeps existing scalar error translation compatible with the expanded non-exhaustive ServerError variant. |
| src/error.rs | Preserves existing ServerRejection extraction while ignoring the newly attached raw stanza. |
Sequence Diagram
sequenceDiagram
participant Server as WhatsApp Server
participant Receive as Receive path
participant Parser as parse_iq_response
participant Client as send_iq_node caller
Server->>Receive: IQ response bytes
Receive->>Receive: "Decode into Arc<OwnedNodeRef>"
Receive->>Parser: Borrow decoded response
alt "type="result""
Parser-->>Receive: Ok
Receive-->>Client: "Same Arc<OwnedNodeRef>"
else "type="error""
Parser-->>Receive: ServerError summary
Receive->>Receive: Attach RejectionStanza(response.clone())
Receive-->>Client: IqError::ServerError with summary and stanza
end
Reviews (5): Last reviewed commit: "docs(request): finish the unread rename ..." | 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: 2
🤖 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/request.rs`:
- Around line 631-648: Update the server-error test around
IqError::from_response to capture the Node returned by answer_iq as delivered,
capture the parsed response, and assert Arc::ptr_eq(&response, &delivered)
alongside the existing content assertions. Apply the same allocation-identity
assertion to the corresponding test block around the alternate server-error
path.
- Around line 121-124: Update the public documentation for the
IqError::ServerError response field so it describes the caller-facing stanza as
type="error", not type="result"; leave the response representation and
surrounding behavior unchanged.
🪄 Autofix
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: 2a28e9f6-35f7-46e1-94f2-4a4847fee83b
📒 Files selected for processing (10)
src/client/context_impl.rssrc/client/sessions.rssrc/error.rssrc/features/mex.rssrc/features/rotate_key.rssrc/keepalive.rssrc/pair_code.rssrc/request.rssrc/test_utils.rstests/error_surface.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The field doc read as if the preserved reply were the type="result" one; the analogy with the success path was the point, not the type. Say type="error" outright, and assert Arc::ptr_eq on the rejection path too, so the test proves the error path attaches the decoded node rather than a copy of it.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: This PR intentionally breaks the public IqError::ServerError variant by adding a response field and removes the From<wacore::request::IqError> impl. A human must decide whether that API and migration break is acceptable for downstream consumers.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
|
@codex review |
![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: a95a460aa2
ℹ️ 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".
| /// The `type="error"` stanza verbatim, handed over whole the way a `type="result"` | ||
| /// one is. What matters in a protocol error is the caller's judgement, so the summary | ||
| /// above does not replace it: unread attributes, children and bytes stay readable. | ||
| response: Arc<wacore_binary::OwnedNodeRef>, |
There was a problem hiding this comment.
When a background IQ is rejected, callers such as set_passive are logged with {e:?} at warning level in src/client/node_io.rs; deriving Debug now recursively formats this response, and OwnedNodeRef::Debug emits the complete decoded node. Consequently, error attributes and children containing JIDs or other PII are newly written to production warning logs, whereas the previous debug representation contained only the four summarized fields. Keep the response accessible to callers, but use a custom/redacted Debug representation that omits its contents.
Useful? React with 👍 / 👎.
| Ok(Ok(response_node)) => match request_utils.parse_iq_response(response_node.get()) { | ||
| Ok(()) => Ok(response_node), | ||
| Err(e) => Err(e.into()), | ||
| Err(e) => Err(IqError::from_response(e, &response_node)), |
There was a problem hiding this comment.
When an error stanza contains an extra attribute, child, or payload, parse_iq_response still emits the process-wide warning that the parser dropped those details before this arm attaches the original stanza to IqError. After this change the caller can inspect every one of those details, so a valid detailed rejection now produces a misleading operational warning claiming data loss; use a non-warning classification path here or update the probe so it reflects that this caller preserves the response.
Useful? React with 👍 / 👎.
Background IQ failures are logged with {e:?} at warn level on the connect
path (node_io.rs: the post-connect active IQ, props, blocklist, privacy
settings). Deriving Debug through the preserved response meant a rejected
one of those newly wrote the whole decoded stanza into production logs,
JIDs in its attributes and children included, where before it wrote only
the four summarized fields.
Wrap the node in RejectionStanza, whose Debug names the stanza and stops
there. Deref keeps every accessor reachable, so reading the contents is
unchanged for a caller that wants them; it is just no longer what a log
line does by default.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23ca4b4180
ℹ️ 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".
| pub fn from_response( | ||
| err: wacore::request::IqError, | ||
| response: &Arc<wacore_binary::OwnedNodeRef>, | ||
| ) -> Self { |
There was a problem hiding this comment.
From conversion
Replacing impl From<wacore::request::IqError> for IqError with this inherent method breaks downstream code that uses .into(), IqError::from, or ? to convert core IQ errors, even for variants such as Timeout that need no response stanza. from_response is not source-compatible because it requires a second argument; retain the trait conversion, potentially by making the preserved response optional for errors converted without a stanza.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: This is a breaking public API change: IqError::ServerError gains a new field and the From conversion is replaced by from_response(), so external callers must migrate. Such contract changes need human sign-off.
Re-trigger cubic
The probe's warning predates the stanza reaching the caller. It reported what `parse_iq_response` does not read as detail the parser "drops", and its rationale said the finding was not something the calling application could act on. Neither holds: the node is borrowed, never consumed, so every caller still holds what went unread — and on this crate's Tokio side that stanza is now `IqError::ServerError`'s `response`. Renames the probe to match the word `PARSED_ERROR_ATTRS` already used for the same idea, so "dropped" stops describing a loss that does not happen. Private to the module; no API moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HuzUxrdsyfcBA4cSUuizi
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The rename left one prose line behind, describing the fully-parsed case as one where nothing is "dropped". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013HuzUxrdsyfcBA4cSUuizi
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: This is a focused refactor that adds a RejectionStanza field to ServerError and preserves the full error stanza from server IQ failures, with bounded changes and extensive tests.
Re-trigger cubic
…ield Reflects oxidezap/whatsapp-rust#1257: IqError::ServerError gains a response: RejectionStanza field carrying the rejection stanza verbatim, and From<wacore::request::IqError> is replaced by IqError::from_response.
…ld (#499) * docs(errors): document RejectionStanza and the ServerError.response field Reflects oxidezap/whatsapp-rust#1257: IqError::ServerError gains a response: RejectionStanza field carrying the rejection stanza verbatim, and From<wacore::request::IqError> is replaced by IqError::from_response. * docs(errors): address review — E0027 exhaustive-match break, fixture unpack step - Note that a ServerError match binding all four former fields without `..` now fails E0027 once `response` is added, not just hand-constructed variants. - Show the required unpack() step when building a fixture from marshal() output for OwnedNodeRef::new, which expects the format byte already stripped. * docs(errors): fully qualify NodeBuilder in fixture snippet Matches the fully-qualified style already used for marshal/unpack in the same snippet, per cubic's review — the bare NodeBuilder reference wasn't importable as written.
2 participants
Summary
send_iq_nodehands back the wholeArc<OwnedNodeRef>when the server answerstype="result". When the same request comes backtype="error", the caller gotIqError::ServerError { code, text, error_type, backoff }and the stanza was thrown away, so whether a reply arrived intact or as a four-field summary depended only on itstypeattribute. Everything outside those four names went with it: further<error>attributes, the child elements XMPP allows there, and the raw bytes, which are the only faithful material for recording or replaying a rejection.ServerErrornow carries the reply as the node the receive path had already decoded, which makes the rejection path one refcount bump and leaves the success path with nothing new to do. The four extracted fields stay alongside it, because nearly every match in the tree isServerError { code, .. }and dropping them would make all of those reparse the common case for nothing.Changes
IqError::ServerErrorgainsresponse: RejectionStanza, the rejection stanza verbatim. Matching with..is unaffected; constructing the variant now needs the stanza. Migration: pass the response you were rejected with (Arc<OwnedNodeRef>converts via.into()), or in a fixture build one and decode it throughOwnedNodeRef::new.RejectionStanzawrapsArc<OwnedNodeRef>rather thanBox<Node>, the shapeDisconnecteduses: the receive path already holds theArc, so there is no copy, and it keepsbacking_bytes().Box<Node>would have deep-copied a node we already own and would have dropped the bytes on the floor.Debug, which names the stanza (<iq>) instead of printing it. Background IQ failures are logged with{e:?}at warn level on the connect path (node_io.rs: the post-connect active IQ, props, blocklist, privacy settings), and an error stanza can carry a JID in its attributes or children; derivingDebugstraight through the node would have written all of that into production logs, where before only the four summarized fields went.Derefkeeps every accessor reachable, so reading the contents is unchanged for a caller that wants them, andas_arc()/into_arc()hand back the shareable node.From<wacore::request::IqError> for IqErroris replaced byIqError::from_response(err, &response). AFromcannot take the response, and converting without it is exactly the loss being removed here. Migration:err.into()becomesIqError::from_response(err, &response).send_and_wait_iqpasses the node it just classified instead of dropping it. TheOkarm is byte-for-byte what it was and still returns that same allocation.executeneeded nothing: it propagatesIqErroruntouched, so there was no second place discarding the stanza. Neitherwacore's ownIqErrornorparse_iq_responsechanged, since that layer only ever holds a borrowedNodeRefand making it own a copy would have cost the deep copy this avoids.server_rejection_ofinerror.rsand the prekey rewrap incontext_impl.rs, gained a... No behaviour change; every other call site already used...test_utils::server_error_iq, which runs the realparse_iq_responseover a real<iq type="error">, so a fixture cannot drift from what the server actually produces. Existing tests changed only where they constructed the variant by hand, which is the contract change itself and not an adaptation to make them pass.New tests in
request.rs: a rejection carrying an unmodelled<error>attribute, an<error>child and an<iq>-level attribute, all asserted on the preserved node rather than on the error string, with the four fields checked for parity and the redactedDebugpinned in the same test; the corresponding failure withtypeandbackoffabsent; andArc::ptr_eqon both thetype="result"andtype="error"paths, which is what proves neither picked up a second parse.Validation
Full matrix left to CI.