fix(receipt): downgrade delivery ack to "sent" on lid feature-incapable error - #708
Merged
Merged
Conversation
…le error WA Web downgrades a delivery receipt's ack to SENT when it carries <error reason=lid type=feature-incapable> (the LID peer can't receive the message). handle_receipt never read that child and dispatched the receipt as Delivered, a false delivery tick. Add ReceiptType::Sent and a pure downgrade_for_feature_incapable() helper, apply it in handle_receipt after parsing the type, and replace the duplicated ReceiptType->wire match in send_protocol_receipt with as_wire_str().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesReceipt type downgrade for feature-incapable errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
![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: 8a41b7c575
ℹ️ 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".
Benchmark Results67 unchanged benchmark(s)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
wacore/src/types/presence.rs (2)
137-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
Sentto the round-trip invariant test set.
ReceiptType::Sentwas added to parse/serialize paths, but the test matrix skips it. That leaves the new wire mapping unguarded.Suggested test patch
let variants = [ ReceiptType::Delivered, + ReceiptType::Sent, ReceiptType::Sender, ReceiptType::Retry, ReceiptType::EncRekeyRetry,🤖 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 `@wacore/src/types/presence.rs` around lines 137 - 153, The test as_wire_str_round_trips_through_parse() omits ReceiptType::Sent from its variants array, leaving the new parse/serialize mapping untested; update the variants list in that function to include ReceiptType::Sent so the round-trip invariant covers the Sent case (ensure the array alongside ReceiptType::Delivered, ReceiptType::Sender, etc. includes ReceiptType::Sent).
26-33: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUnify
ReceiptTypewire mapping underWireEnum(stop split-brain).
wacore/src/types/presence.rskeepsReceiptType’s wire mapping split between hand-writtenfrom_known/as_wire_strand#[derive(Serialize, Deserialize)]on the enum. RefactorReceiptTypeto#[derive(WireEnum)]and encode the existing mapping via explicit#[wire = "..."]/#[wire_alias = "..."]/#[wire_fallback](for the catch-allOther(String)), then remove the manual serde derives so WireEnum is the single source of truth.🤖 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 `@wacore/src/types/presence.rs` around lines 26 - 33, Replace the ad-hoc serde mapping on the ReceiptType enum with a single WireEnum-driven mapping: remove #[derive(Serialize, Deserialize)] and any manual conversion helpers (e.g. from_known, as_wire_str) and instead #[derive(WireEnum, Debug, Clone, PartialEq, Eq)] on pub enum ReceiptType; add explicit #[wire = "..."] / #[wire_alias = "..."] attributes on the existing variants (e.g. Delivered, Sent) to encode their wire strings and add a catch-all variant Other(String) annotated with #[wire_fallback] to capture unknown values; ensure the enum retains existing variant docs/comments and that all serialization/deserialization now flows through the WireEnum implementation.
🤖 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 `@wacore/src/types/presence.rs`:
- Around line 137-153: The test as_wire_str_round_trips_through_parse() omits
ReceiptType::Sent from its variants array, leaving the new parse/serialize
mapping untested; update the variants list in that function to include
ReceiptType::Sent so the round-trip invariant covers the Sent case (ensure the
array alongside ReceiptType::Delivered, ReceiptType::Sender, etc. includes
ReceiptType::Sent).
- Around line 26-33: Replace the ad-hoc serde mapping on the ReceiptType enum
with a single WireEnum-driven mapping: remove #[derive(Serialize, Deserialize)]
and any manual conversion helpers (e.g. from_known, as_wire_str) and instead
#[derive(WireEnum, Debug, Clone, PartialEq, Eq)] on pub enum ReceiptType; add
explicit #[wire = "..."] / #[wire_alias = "..."] attributes on the existing
variants (e.g. Delivered, Sent) to encode their wire strings and add a catch-all
variant Other(String) annotated with #[wire_fallback] to capture unknown values;
ensure the enum retains existing variant docs/comments and that all
serialization/deserialization now flows through the WireEnum implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b8c422f4-7faf-470c-8876-277e2123f3dc
📒 Files selected for processing (4)
src/client.rssrc/receipt.rswacore/src/stanza/receipt.rswacore/src/types/presence.rs
…d types An aggregated-by-message receipt re-parsed each <user type=delivery> back to Delivered, undoing the receipt-level downgrade. Apply downgrade_for_feature_incapable to the per-user type too, so a feature-incapable LID participant in a group receipt no longer emits a false delivered tick.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
WA Web's receipt parser inspects an
<error>child of<receipt>: whenreason="lid"andtype="feature-incapable", it downgrades the ack from RECEIVED to SENT, signalling the message was NOT delivered (the LID peer is feature-incapable).handle_receiptnever read that child and always dispatchedEvent::Receiptwith the rawtypeattribute, so a<receipt type="delivery">carrying that error was reported asReceiptType::Delivered: a false delivery tick for a message the server explicitly said wasn't delivered.WA Web (
MsgReceiptParser):a = e.maybeChild("error"); a.reason === "lid" && a.type === "feature-incapable" && (r.ack = ACK.SENT).Fix
ReceiptType::Sent(sent but not delivered), a derived incoming-only state.wacore::stanza::receipt::downgrade_for_feature_incapable(node, parsed_type)returnsSentwhen aDeliveredreceipt carries<error reason="lid" type="feature-incapable">. Scoped toDelivered(the only type that carries this error), which also keeps retry / enc-rekey receipts from being rerouted.handle_receiptapplies the downgrade right after parsing the type, so it flows into both the simple and aggregated dispatch.send_protocol_receipt's hand-rolledReceiptType-> wire match (which duplicatedas_wire_str) is replaced byas_wire_str(), keeping a single source of truth and covering the new variant.Tests
New
feature_incapable_error_downgrades_delivery_to_sent: builds the receipt node and asserts the lid/feature-incapable error downgradesDelivered->Sent, while a missing error, a different error type, and a non-Deliveredtype are all left unchanged.Breaking
ReceiptTypegains aSentvariant (pre-1.0; the enum already lacks#[non_exhaustive]). Consumers with exhaustive matches must add aSentarm.Sentonly appears on incoming delivery receipts the server flagged as undelivered.