feat(client): let a consumer take a stanza before the built-in pipeline - #1239
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe client now supports public stanza interceptors. Interceptors can observe or claim decoded stanzas, unregister through drop-based handles, and preserve acknowledgement and housekeeping behavior. ChangesStanza interception
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StanzaInterceptor
participant StanzaRouter
participant Acknowledgement
Client->>StanzaInterceptor: intercept decoded non-critical stanza
StanzaInterceptor-->>Client: Pass or Handled
alt Handled
Client->>Acknowledgement: preserve deferred or generated acknowledgement
else Pass
Client->>StanzaRouter: continue built-in routing
end
Suggested labels: Suggested reviewers: 🚥 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/client/node_io.rs | Integrates interception into inbound dispatch while retaining housekeeping, protecting control stanzas and pings, and acknowledging claimed unknown tags. |
| src/client/interceptor.rs | Defines the public interception contract, decision type, closure support, and weak RAII registration handle. |
| src/client/accessors.rs | Implements ordered copy-on-write interceptor registration, removal, snapshots, and reporting. |
| src/client/tests.rs | Adds end-to-end coverage for interception priority, lifecycle, dispatch suppression, acknowledgment behavior, and protected stanza classes. |
| src/handlers/router.rs | Adds tag-membership lookup used to distinguish modeled stanzas from unknown tags when selecting acknowledgment behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Decoded stanza] --> B[Offline sync and waiter housekeeping]
B --> C{Connection-critical or server ping?}
C -- Yes --> F[Built-in dispatch]
C -- No --> D{Interceptor claims stanza?}
D -- No --> F
D -- Yes --> E[Send applicable transport acknowledgment]
E --> G[Skip built-in pipeline]
F --> H[Built-in handler or unknown-stanza nack]
Reviews (7): Last reviewed commit: "fix(client): never offer a server ping t..." | 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: 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/client/accessors.rs`:
- Around line 82-85: Update the interceptor count atomic operations in the
registration and access paths, including stanza_interceptor_count.store calls
and the fast-path load in stanza_interceptors_guard(), to use Ordering::Relaxed.
Keep synchronization for registry access within stanza_interceptors_guard() and
preserve the existing lock decision logic.
In `@src/client/node_io.rs`:
- Around line 508-520: Update the interceptor-claimed branch in the stanza
processing flow around should_ack, deferred_ack_node, and intercept_stanza so
claimed unknown stanzas with valid ACK identity fields receive a generic
transport ACK even when should_ack() is false. Preserve the existing deferred
ACK behavior for known stanzas, retain the unknown-stanza nack path when no
interceptor claims the stanza, and add a regression test that verifies the
outbound ACK.
- Around line 570-587: Update the decoded-stanza handling in
process_decrypted_node and process_node so intercept_stanza runs for ACKs,
matched IQ responses, and xmlstreamend before returning. Preserve the existing
response-waiter resolution and stream-shutdown housekeeping first, then invoke
interception on every decoded stanza; add regression coverage for all three
paths.
🪄 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: 72a8edaa-d054-4750-85dd-f50d61480d1a
📒 Files selected for processing (7)
src/client.rssrc/client/accessors.rssrc/client/interceptor.rssrc/client/lifecycle.rssrc/client/node_io.rssrc/client/tests.rssrc/lib.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 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Three findings, all valid, all fixed in 2ab2155. Claimed unknown stanzas went unanswered (greptile P1). Confirmed and the worst of the three. A claim now turns that nack into an ack: both settle the stanza, and which one is right depends on whether anyone handled it. Same identity requirement as the nack path — without My test asserted the old behaviour as if it were the feature. It was testing that the stanza was not nacked, which was true and beside the point. Interception bypassed connection state (greptile P1). Also valid, and the fix is a list rather than a warning:
Relaxed ordering (coderabbit). Valid, and I had already caught it while auditing the cost of this PR: the docs promised one relaxed load while the code used Two things not raised that the same audit turned up: The registry now uses the copy-on-write snapshot the event bus already uses, so reading it is a refcount bump rather than a The module docs claimed an interceptor "sees every decoded stanza". It does not: an already-correlated IQ response, Four tests added for the new behaviour, and the existing 135 in |
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/client/interceptor.rs`:
- Around line 49-52: Update the module documentation around the claimed-stanza
acknowledgement guarantee to state that acknowledgement occurs only when the
stanza has both an id and a from address. Keep the existing ack/nack behavior
description, but qualify the claim so it matches the dispatch path’s
precondition.
In `@src/client/tests.rs`:
- Around line 5143-5145: Update the test around client.process_node and the
existing seen assertion to await the test transport output, then verify it
contains exactly one ACK for V-1 and no NACK. Keep the interceptor invocation
assertion while adding response assertions that fail when neither stanza is
emitted.
🪄 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: 958c3920-0a91-4879-befd-900ba3c7e9af
📒 Files selected for processing (6)
src/client.rssrc/client/accessors.rssrc/client/interceptor.rssrc/client/lifecycle.rssrc/client/node_io.rssrc/client/tests.rs
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
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 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 2 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: Adds a public interpolation API and changes stanza dispatch and ack/nack semantics, including design choices about priority ordering and non-claimable stanzas; a human should sign off on this protocol-facing contract.
Re-trigger cubic
The client models the stanzas it knows and nacks the rest, which is the right default — a nack tells the server this client cannot act on something, and silence would leave it queued forever. But there is no way to say otherwise. A stanza this version does not model is nacked whether or not the application would have known what to do with it, and StanzaRouter::register panics on a duplicate tag, so even an existing tag cannot be handled differently. Extending the client means forking it. An interceptor is that room. It runs before dispatch, sees every decoded stanza, and either steps aside or claims it. A claimed stanza skips the built-in pipeline and is acked exactly as it would have been, so the server does not redeliver. Claiming skips handling, not housekeeping. Offline-sync tracking, response-waiter resolution and stream shutdown run before dispatch and keep running either way — they are what keeps the connection working, and an interceptor able to switch them off would be a way to break a client rather than extend one. Free while unused: one relaxed atomic on the read loop, the same shape as the raw-node forwarding lease.
Three findings from review, all real. An unmodelled stanza that an interceptor claimed was left unanswered. should_ack covers only the tags the client models, so a claimed vendor tag fell through with neither ack nor the nack it would have received — the exact shape of the bug the unknown-stanza nack exists to prevent, where the stanza stays queued and the stream keeps recycling. A claim now turns that nack into an ack, since someone did handle it. success, failure, stream:error and ack are no longer offered at all. They settle authentication, shutdown and the waiters a send blocks on; claiming one would leave a client authenticated-but-unaware, or never reconnecting, or waiting forever on a send that already completed. zapo protects the same auth tags from its own stanza filters, for the reason. The count is read Relaxed rather than Acquire. The lock behind it does the synchronising, so the fast path did not need the stronger ordering the docs already claimed it did not have. Also switches the registry to the copy-on-write snapshot the event bus uses, so reading it costs a refcount bump instead of allocating a Vec per stanza, and corrects the module docs: an interceptor sees what would have reached dispatch, not everything decoded. Event::RawNode is the tool for everything.
The claimed-stanza ack covered any stanza with `id` and `from` that `should_ack` had not already matched. That is wider than the case it was added for: a claimed direct `<message>` is answered with a delivery `<receipt>` and a claimed `<iq>` with an `<iq type="result">`, and neither is an `<ack class="…">`. The server was being sent something it did not ask for. Narrow it to tags the router does not model — the stanzas that would have been nacked, which is the gap a claim is meant to close. A tag the client models but answers some other way now gets nothing: whoever claimed it took on the reply, and inventing an answer is worse than silence. Two documentation claims were also wider than the code. Acknowledgement needs `id` and `from`, and the trait sees stanzas headed for dispatch rather than every decoded stanza — which the module documentation already said, one paragraph away. The claimed-unknown test now decodes the outbound frame instead of only proving the interceptor ran, so it fails if the ack stops being sent.
…nswered `should_ack` covers `receipt`, `notification` and `call`, so a claimed one still draws the transport ack it always did. The acknowledgement section used a delivery receipt as its example of a tag the client answers some other way — which would have a reader believe they owed a reply the client is still sending, or avoid claiming receipts at all. The group the paragraph is about is direct `<message>` and `<iq>`. The tags that keep their ack are now named as such.
f2bff93 to
fa1f511
Compare
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/client/interceptor.rs`:
- Around line 13-15: Qualify the acknowledgement guarantees in
src/client/interceptor.rs at lines 13-15 and 120-122: document that claiming a
stanza suppresses the built-in handler, and for direct message or built-in IQ
stanzas the claimant must provide any required protocol-specific reply because
no generic transport ACK is sent. Update both the module-level text and
Interception::Handled documentation consistently.
🪄 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: a08db514-6c7d-464f-b391-84d4e1134398
📒 Files selected for processing (4)
src/client.rssrc/client/accessors.rssrc/client/interceptor.rssrc/client/node_io.rs
`report_coverage` requires every `Client` field that can grow to reach `memory_report()`, and this one can: a handle that outlives its interest leaves an interceptor registered, and each one costs a walk on every stanza. Exempting it would hide exactly the leak worth seeing.
There was a problem hiding this comment.
0 issues found across 2 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: Adds a new public interceptor API and changes the inbound stanza dispatch/ack path, letting consumers claim stanzas before built-in handlers. This is a behavioral/public-contract change that needs human review of its semantics and edge cases.
Re-trigger cubic
A claimed ping is a pong never sent, and the server closes the connection over it. That is the same harm `success`, `failure`, `stream:error` and `ack` are protected from — an interceptor exists to extend a client, not to leave it disconnected — so a server-initiated `<iq>` ping joins them. The gate is `handle_iq`'s own ping test, extracted so the two cannot drift apart: what the client answers is exactly what it refuses to hand over. A ping *response* carries no obligation and stays offered, as does every other `<iq>` — that is the traffic an interceptor is for. Two documentation claims also still promised an acknowledgement for every claimed stanza. The acknowledgement section had already been corrected; the module introduction and `Interception::Handled` had not.
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.
Auto-approved: The diff shows the interceptor seam is optional and zero-cost when unused, passes only stanzas that would dispatch and blocks connection-critical ones, and stays consistent with existing patterns.
Re-trigger cubic
Tap rides Event::RawNode and only watches. Takeover rides the pre-dispatch interceptor added upstream in oxidezap/whatsapp-rust#1239: a claimed stanza skips the engine's handler and is acked all the same, so the server does not redeliver. The two carry separate capability sets, because neither is a superset of the other. Tap sees the authentication exchange and cannot suppress anything. Takeover suppresses but cannot see that exchange — the engine refuses to offer success, failure, stream:error and ack to an interceptor, since a consumer that took one would leave the client authenticated-but-unaware or waiting forever on a completed send. One declaration for both would be false in one direction. A policy decides per stanza rather than all-or-nothing: an engine reduced to a transport still has to be told what to do with the rest. TakeTags is the common case, TakeEverything the full one. Forwarding happens before the decision, so a claimed stanza is never one the consumer did not receive — the engine will not handle it either, so it would simply vanish. For the same reason a poisoned sink passes rather than claims.
1 participant
Summary
The client models the stanzas it knows about and nacks the rest. That is the
right default — a
<nack>tells the server this client cannot act on something,and staying silent would leave the stanza in the offline queue indefinitely.
What is missing is a way to say otherwise. A stanza this version does not model
gets nacked whether or not the application would have known what to do with it,
and
StanzaRouter::registerpanics on a duplicate tag, so even an existing tagcannot be handled differently. Today, extending the client means forking it.
This adds a seam:
An interceptor runs before dispatch, sees every decoded stanza, and either steps
aside or claims it. A claimed stanza skips the built-in pipeline and is
acknowledged exactly as it would have been, so the server does not redeliver.
Design
Claiming skips handling, not housekeeping. Offline-sync tracking,
response-waiter resolution and stream shutdown all run before dispatch, and they
keep running whether or not a stanza is claimed. They are what keeps the
connection working; an interceptor that could switch them off would be a way to
break a client rather than to extend one. A test pins this:
<ib>offlinetracking still runs on a claimed
<ib>.The ack is unconditional. A claimed stanza is acked the way it would have
been. Withholding it would leave the stanza queued for redelivery, which is a
worse failure than double-handling — and the interceptor is claiming
responsibility for the stanza, not for the transport.
Free while unused. One relaxed atomic on the read loop, checked before the
lock is touched. Same shape as the existing raw-node forwarding lease, so the
cost model is one people already know: registering is what turns the check into
a walk.
Registration order is priority order. The first interceptor to claim a
stanza wins and the rest are skipped, so an earlier registration can shadow a
later one. Rejected the alternative of letting every interceptor see every
stanza and then reconciling their answers: two consumers both claiming one
stanza is a conflict the client cannot resolve, and picking a winner by
registration order at least makes it predictable.
The handle is RAII and weak. Dropping it unregisters. It holds a weak client
reference, so a forgotten handle cannot pin a client alive, and dropping one
after its client is gone is a no-op rather than a panic.
Not panic-caught. Same contract as
EventHandler: this is called directlyby the read loop, an unwind there takes the connection with it, and a directly
registered interceptor is trusted. The plugin host already catches panics from
plugins, so the untrusted path stays covered.
What this is not
Not a filter for outbound stanzas, and not a decryption hook — a claimed
<message>has already been decrypted or not by the time an interceptor seesit, exactly as the built-in handler would have found it.
Verification
Eleven tests, covering what the design claims rather than that the code runs:
handle drops;
asserted through the event bus rather than by inspecting internals;