feat(client): observe what the client sends, not only what it receives - #1260
Conversation
The receive side hands every decoded stanza to consumers through Event::RawNode, lease-gated so it costs nothing when unused. The send side had no equivalent: wait_for_sent_node is a filtered one-shot for a single expected stanza, and it only sees stanzas that go through marshal_node_for_send, so acks, delivery receipts and direct-encoded IQs were invisible even to that. A test could not assert what reached the wire without wrapping the transport, and a stanza the server rejected could not be read back without a rebuild. Event::SentFrame carries the marshaled plaintext of each frame the transport accepted. It is emitted from the noise sender task, which is the one point all five send paths cross (send_node, send_raw_bytes, send_raw_bytes_burst, and the ack and receipt workers through the burst) and the same chokepoint SessionStats already records wire bytes at. Observing bytes rather than nodes is what makes that possible: two of those paths never build a Node at all. Cost follows the house pattern. The gate is a relaxed atomic load per frame through the tap the sender already holds, so an unwatched send allocates exactly what it did before. A watched one costs two allocations per frame, neither of which scales with the stanza: the payload is handed over as a refcount bump on the caller's own buffer, pinned by a pointer assertion. The dispatch is caught, since it runs on the task every send depends on. wait_for_sent_node stays as it is. It observes the Node before marshalling, resolves before the write, and is filtered, so it answers a different question than a stream of sent bytes; reimplementing it on top of this one would have to unmarshal every frame to test its filter, and would widen what its callers see. send_raw_bytes keeps its bypass semantics, which are now only about node logging and sent-node waiters.
|
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 (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds lease-gated ChangesSent-frame observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant Client
participant NoiseSocket
participant Transport
participant EventBus
Plugin->>Client: acquire sent-frame forwarding lease
Client->>NoiseSocket: configure SendObservers
NoiseSocket->>Transport: write encrypted frame
Transport-->>NoiseSocket: successful write
NoiseSocket->>EventBus: emit Event::SentFrame with plaintext
Possibly related PRs
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.rs | Adds the aggregate sent-frame forwarding gate, publisher, and lease contract; the previously discussed non-fencing semantics are now explicit. |
| src/socket/noise_socket.rs | Adds the common post-write observation point and rechecks forwarding after successful transport acceptance, resolving the reported final-lease race. |
| wacore/src/types/events.rs | Adds the non-serialized SentFrame event and plaintext payload without moving existing event-kind discriminants. |
| src/plugins/mod.rs | Extends plugin gated-forwarding lifecycle management to acquire and release sent-frame leases alongside existing gated event kinds. |
| src/client/lifecycle.rs | Initializes the sent-frame tap on the client and supplies it to the main session Noise socket. |
| src/client/tests.rs | Adds end-to-end coverage showing all principal send paths are observed in accepted wire order and remain gated when unused. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Client send paths] --> B[NoiseSocket sender task]
B --> C[Encrypt and frame]
C --> D[Transport write]
D -->|failure| E[Discard captured plaintext]
D -->|accepted| F{Sent-frame lease enabled?}
F -->|no| G[Complete send without event]
F -->|yes| H[Dispatch Event::SentFrame]
D --> I[Record session statistics]
Reviews (9): Last reviewed commit: "docs(observability): bound the chokepoin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The gate was read once, at capture time, and the frame was published after the write it belongs to. A lease released in between still saw that frame arrive, which contradicts the documented contract that nothing is emitted while no consumer holds one. Reading the gate again before the publish closes it, for one relaxed load per batch. The test parks the sender inside the transport write, so the frame is captured and encrypted before the lease goes away: without the second read it fails. Also record what the whole-dispatch panic containment costs, since it is per dispatch and not per handler: a panicking observer loses that frame for the observers registered behind it. The bus has no per-handler isolation for any kind, and the plugin host already wraps its own handlers, so isolating this one kind would be a change to the bus rather than to this path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 3/5
- In
src/client.rs,SentFramedispatch currently recovers around the whole fan-out, so one panicking subscriber can prevent later subscribers from seeing that frame, leading to dropped notifications and inconsistent downstream state for that event — isolate recovery per handler invocation so one panic only skips the failing subscriber.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client.rs">
<violation number="1" location="src/client.rs:185">
P2: A panicking `SentFrame` subscriber prevents every later `SentFrame` subscriber from receiving that frame, because the recovery surrounds the entire dispatch rather than each handler; only the sender task is isolated. Per-handler recovery in the event bus (or an equivalent isolated dispatch API) would keep one observer from suppressing other observers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| #[cfg(test)] | ||
| self.published.fetch_add(1, Ordering::Relaxed); | ||
| let dispatch = std::panic::AssertUnwindSafe(|| { | ||
| self.bus.dispatch(Event::SentFrame( |
There was a problem hiding this comment.
P2: A panicking SentFrame subscriber prevents every later SentFrame subscriber from receiving that frame, because the recovery surrounds the entire dispatch rather than each handler; only the sender task is isolated. Per-handler recovery in the event bus (or an equivalent isolated dispatch API) would keep one observer from suppressing other observers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client.rs, line 185:
<comment>A panicking `SentFrame` subscriber prevents every later `SentFrame` subscriber from receiving that frame, because the recovery surrounds the entire dispatch rather than each handler; only the sender task is isolated. Per-handler recovery in the event bus (or an equivalent isolated dispatch API) would keep one observer from suppressing other observers.</comment>
<file context>
@@ -103,6 +103,102 @@ impl Drop for RawNodeLease {
+ #[cfg(test)]
+ self.published.fetch_add(1, Ordering::Relaxed);
+ let dispatch = std::panic::AssertUnwindSafe(|| {
+ self.bus.dispatch(Event::SentFrame(
+ wacore::types::events::SentFrame::builder()
+ .plaintext(plaintext)
</file context>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
src/socket/noise_socket.rs, the check that guards dispatch can race withtap.publish, so aSentFramemay still be emitted after the final lease is dropped; this can violate shutdown/lifecycle expectations and cause stale or misleading socket events downstream — synchronize the lease release with the check-and-dispatch path (e.g., a shared read/write guard around both steps).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/socket/noise_socket.rs">
<violation number="1" location="src/socket/noise_socket.rs:329">
P2: Dropping the final lease can still be followed by `SentFrame` publication when it races between this gate check and `tap.publish`; synchronize release with check-and-dispatch (for example, a shared read/write guard) so release completion excludes later dispatches.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // capture time: a lease released while this batch was in | ||
| // flight must not see a frame arrive after it let go. | ||
| if let Some(tap) = sent_frames.as_deref() | ||
| && tap.enabled() |
There was a problem hiding this comment.
P2: Dropping the final lease can still be followed by SentFrame publication when it races between this gate check and tap.publish; synchronize release with check-and-dispatch (for example, a shared read/write guard) so release completion excludes later dispatches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/socket/noise_socket.rs, line 329:
<comment>Dropping the final lease can still be followed by `SentFrame` publication when it races between this gate check and `tap.publish`; synchronize release with check-and-dispatch (for example, a shared read/write guard) so release completion excludes later dispatches.</comment>
<file context>
@@ -322,7 +322,12 @@ impl NoiseSocket {
+ // capture time: a lease released while this batch was in
+ // flight must not see a frame arrive after it let go.
+ if let Some(tap) = sent_frames.as_deref()
+ && tap.enabled()
+ {
for plaintext in observed.drain(..) {
</file context>
"Exactly as it was encrypted and written" reads as if the payload were the bytes that went on the socket. It is the input to that encryption: no length prefix, no AEAD tag. A consumer building a replay tool has to know which of the two it is holding.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
… does not Reviewers read the recheck before publishing as a promise that no frame can follow the last drop, and asked for a lock to close the remainder. A lock there would deadlock an observer that drops its lease from inside its own handler, and the guarantee it would buy is not one the gate ever offered: the count is aggregate, and delivery is decided by subscription, exactly as for the other gated kinds. Say so where a consumer reads it.
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: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
main split `do_handshake` into the socket build plus a `negotiate` that returns the cipher pair, so the observers this branch threads through now reach `NoiseSocket` from `do_handshake` rather than from inside the handshake span. That keeps main's property that the connection's sender task is not spawned under a one-shot span. `tests/handshake_span_scope.rs`, new on main and gated on the `tracing` feature, called `do_handshake` with the removed `stats` argument; it passes `SendObservers::default()` like the other handshake tests.
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 `@agent_docs/observability.md`:
- Around line 35-44: Update the observability paragraph to remove or correct the
inaccurate “five distinct send paths” count, reflecting that ack/receipt workers
and send_node reach the socket through already-listed paths. In the VoIP relay
socket description below, replace the stale `None` wording with
`SendObservers::default()` to match the current API.
In `@src/client/tests.rs`:
- Around line 3510-3518: Update the NoiseSocket construction in the test helper
to initialize SendObservers with the client’s stats observer, matching
connect_graph and create_iq_test_client, while retaining the sent-frame tap.
Ensure client.stats() records frames sent through this helper.
In `@src/socket/noise_socket.rs`:
- Around line 2060-2068: Update the allocation assertion around the watched and
idle measurements to avoid directly subtracting unsigned values; use a signed or
saturating difference while preserving the expected two-allocation comparison.
Ensure cases where watched is below idle fail through the assertion with a
readable message containing both measured values, rather than panicking on
underflow.
In `@src/test_utils.rs`:
- Around line 60-75: The frame-length parsing in the write-processing loop
should be centralized instead of duplicated. Extract a shared pub(crate) 3-byte
frame parser reusable by src/test_utils.rs, noise_socket.rs, and transport.rs,
then add decrypt_wire_frames to perform decryption and wire consumption using
that parser; update the existing loop to call these helpers while preserving
frame ordering and per-frame counters.
🪄 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: 1b1ecb99-9287-47e7-8b6a-bf820b299e60
📒 Files selected for processing (14)
agent_docs/observability.mdsrc/client.rssrc/client/accessors.rssrc/client/lifecycle.rssrc/client/messaging.rssrc/client/tests.rssrc/handshake.rssrc/lib.rssrc/plugins/mod.rssrc/socket/noise_socket.rssrc/test_utils.rstests/handshake_integration.rstests/handshake_span_scope.rswacore/src/types/events.rs
| for write in writes { | ||
| let mut wire = &write[..]; | ||
| while !wire.is_empty() { | ||
| let mut len = 0usize; | ||
| for byte in &wire[..wacore::framing::FRAME_LENGTH_SIZE] { | ||
| len = (len << 8) | *byte as usize; | ||
| } | ||
| let body = | ||
| &wire[wacore::framing::FRAME_LENGTH_SIZE..wacore::framing::FRAME_LENGTH_SIZE + len]; | ||
| let mut body = bytes::BytesMut::from(body); | ||
| cipher | ||
| .decrypt_in_place_with_counter(plaintexts.len() as u32, &mut body) | ||
| .expect("each captured frame must decrypt under its own counter"); | ||
| plaintexts.push(body.to_vec()); | ||
| wire = &wire[wacore::framing::FRAME_LENGTH_SIZE + len..]; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate every length-prefixed frame splitter in the crate.
rg -nP -C 6 '\bfn (split_frames|split_framed|decrypt_wire_frames)\s*\(' --type=rust
# Confirm FRAME_LENGTH_SIZE consumers.
rg -nP -C 3 'FRAME_LENGTH_SIZE' --type=rustRepository: oxidezap/whatsapp-rust
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -u
echo "== rust files named test_utils/socket/noise_socket/transport =="
fd -e rs '^(test_utils|noise_socket|transport)\.rs$' . || true
echo "== tracked files containing likely symbols =="
rg -n -C 5 'fn (split_frames|split_framed|decrypt_wire_frames)|FRAME_LENGTH_SIZE|FRAME_LENGTH_SIZE' --glob '*.rs' . || true
echo "== git status/stat =="
git status --short || true
git diff --stat || trueRepository: oxidezap/whatsapp-rust
Length of output: 16400
🏁 Script executed:
#!/bin/bash
set -u
echo "== src/test_utils.rs =="
cat -n src/test_utils.rs
echo "== src/transport.rs relevant section =="
sed -n '40,75p' src/transport.rs | cat -n -v +L40
echo "== src/socket/noise_socket.rs tests split_frames section =="
sed -n '1505,1528p' src/socket/noise_socket.rs | cat -n -v +L1505
echo "== deterministic behavior probe against the 3 helper bodies =="
python3 - <<'PY'
from pathlib import Path
import re
def body(src):
m = re.search(r'((?:pub\s+\)?)?(?:fn|async\s+fn|impl\s+[^{]+\{[^}]*{|class\s+[^{]+\{[^}]*\{)\s*([A-Za-z_]\w*)\s*\([^)]*\)(?:\s*->\s*[^{]+)?\s*(?:\{\s*#[^{]*\n)?\{', src, re.S)
# not structural enough. simpler: just return full source.
return src.read_text()
files = {
"src/test_utils.rs": Path("src/test_utils.rs").read_text(),
"src/socket/noise_socket.rs": Path("src/socket/noise_socket.rs").read_text(),
"src/transport.rs": Path("src/transport.rs").read_text(),
}
for name, src in files.items():
for needle in ["decrypt_wire_frames", "split_frames", "split_framed"]:
idx = src.find(needle)
print(f"\n{name}: {needle} at {idx}")
start = src.rfind("\n", 0, max(0, idx-400))
end = src.find("\n}", max(max(0, idx), start)+1)
for off in range(-1, 8):
print(src[idx:idx+1600])
print("---")
break
PYRepository: oxidezap/whatsapp-rust
Length of output: 24753
Reuse one 3-byte frame splitter.
This length walk is repeated inside src/test_utils.rs, with src/socket/noise_socket.rs and src/transport.rs keeping another 3-byte splitter each. Extract one pub(crate) frame parser and move the decryption/consume step into decrypt_wire_frames. One shared parser is how we avoid broken test assumptions when framing changes.
🤖 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/test_utils.rs` around lines 60 - 75, The frame-length parsing in the
write-processing loop should be centralized instead of duplicated. Extract a
shared pub(crate) 3-byte frame parser reusable by src/test_utils.rs,
noise_socket.rs, and transport.rs, then add decrypt_wire_frames to perform
decryption and wire consumption using that parser; update the existing loop to
call these helpers while preserving frame ordering and per-frame counters.
`CapturingMockTransport::sent()` already splits each write into its frames, prefix intact, so the helper was walking a length prefix that had been walked for it and adding a third copy of that parse to the crate. It now checks the declared length against the entry instead, which turns a caller that passes raw writes into a failed assertion rather than a decrypt of garbage. The noise socket the client-test helper installs now reports to the client's stats as well as its tap, matching the real socket and the other fixture: a test that later asserts on `frames_sent` would have read zero and blamed the code under test. The allocation delta is compared as a signed difference. Both sides are empirical minima off a process-global counter, and an inversion has to print the two numbers rather than panic inside the subtraction. Also fixes the observability doc: it claimed five distinct send paths and then listed callers that funnel through two, and still described VoIP sockets as passing `None`, which this branch replaced with `SendObservers::default()`.
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 4 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: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
![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: ce7e574789
ℹ️ 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 with_observers( | ||
| runtime: Arc<dyn Runtime>, | ||
| transport: Arc<dyn Transport>, | ||
| write_key: NoiseCipher, | ||
| read_key: NoiseCipher, | ||
| stats: Option<Arc<wacore::stats::SessionStats>>, | ||
| observers: SendObservers, | ||
| ) -> Self { |
There was a problem hiding this comment.
NoiseSocket::with_stats entry point
Downstream code that constructs the publicly exported NoiseSocket with NoiseSocket::with_stats(..., Option<Arc<SessionStats>>) no longer compiles because this change replaces that public method with with_observers. The observer-aware constructor can be added without removing the existing API; retain with_stats as a compatibility wrapper that builds the appropriate SendObservers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct that it breaks, deliberate that it does. Pre-1.0, the break is called out in the PR body, and the Semver Checks job reports it for the changelog.
A with_stats wrapper would be a second constructor that cannot express the tap, so a caller reaching for the familiar name would silently get a socket nothing can observe — the "two doors to the same thing" that SendObservers exists to prevent. SendObservers::with_stats(stats) is the one-line migration, and new() still covers the no-observer case.
Generated by Claude Code
| ik_handshake_failures: &AtomicU32, | ||
| transport: Arc<dyn Transport>, | ||
| transport_events: &mut async_channel::Receiver<TransportEvent>, | ||
| stats: Option<Arc<wacore::stats::SessionStats>>, | ||
| observers: crate::socket::noise_socket::SendObservers, | ||
| ) -> Result<Arc<NoiseSocket>> { |
There was a problem hiding this comment.
do_handshake signature
External callers of the public handshake::do_handshake currently pass Option<Arc<SessionStats>>; changing that parameter to SendObservers makes every such caller fail to compile even if it does not use sent-frame observation. Keep the existing entry point and route it through a new observer-aware helper, while the client can call that helper to install its private sent-frame tap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same call as on with_stats: the break is intended, documented in the PR body, and reported by Semver Checks.
Keeping the stats-only parameter and adding an observer-aware twin means two public handshake entry points where the older one quietly cannot observe sends — and widening this signature per observer is the thing SendObservers was introduced to stop. External callers pass SendObservers::with_stats(stats), or SendObservers::default() where they passed None.
Generated by Claude Code
…t-observability-56adcw
…point main added `unmarshal_packed_ref`, which validates the leading format byte before decoding instead of assuming it. The `SentFrame` doc and the send-path test both hand-rolled `&plaintext[1..]`, which is the same thing right up until a payload arrives with the compressed bit set. Point them at the helper. No conflicts this time. Rechecked the allocation test against main's send-path work (#1256): the idle path and the two-per-frame watching cost are unchanged.
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: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
|
The red Semver Checks on this PR is not from this branch, and I got a detail wrong earlier that is worth correcting. The failure is The correction: on the two threads about Generated by Claude Code |
An adjacency conflict only: main's `server_error_iq` helper landed at the same point in `test_utils` as the sent-frame recorder and the frame-decrypt helper. All three are wanted, so all three stay. main also gave `answer_iq` a return value; nothing on this branch calls it, so the auto-merge stands as it is.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Both sides added to the same re-export list: `Connection` on main, and this branch's `SentFrameLease`. Both stay.
There was a problem hiding this comment.
Actionable comments posted: 1
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/events.rs (1)
1724-1742: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFreeze the public
SentFramepayload API.Add
#[non_exhaustive]and#[derive(bon::Builder)]toSentFrame. Update its construction sites to useSentFrame::builder().Proposed fix
+#[non_exhaustive] +#[derive(bon::Builder)] pub struct SentFrame {As per coding guidelines, “Event payloads are a frozen API: use
#[non_exhaustive]and#[derive(bon::Builder)].”🤖 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/events.rs` around lines 1724 - 1742, Add #[non_exhaustive] and #[derive(bon::Builder)] to SentFrame, then update every SentFrame construction site to use SentFrame::builder() instead of direct struct literals. Preserve the existing plaintext field and serialization behavior.Source: Coding guidelines
src/client/tests.rs (1)
5770-5770: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the repository fictional NANP JID format.
Line 5770 uses
15551234567, which has a555NPA and does not use the reserved fictional exchange and line range. Replace it with a valid fictitious value such as12025550111.Proposed fix
- .attr("to", "15551234567@s.whatsapp.net") + .attr("to", "12025550111@s.whatsapp.net")Based on learnings: fictional NANP test numbers use a real non-
555NPA, the555exchange, and a0100–0199line number.🤖 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/tests.rs` at line 5770, Update the JID value in the test attribute around the existing "to" field to use a repository-valid fictional NANP number, such as 12025550111, preserving the `@s.whatsapp.net` suffix.Source: Learnings
🤖 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 `@agent_docs/observability.md`:
- Around line 35-40: Update the observability documentation around
send_raw_bytes and send_raw_bytes_burst to define the chokepoint as covering all
post-handshake main-session outbound frames. Qualify the “everything” claim so
it excludes handshake frames that are not forwarded through Event::SentFrame,
while preserving the existing function and worker routing descriptions.
---
Outside diff comments:
In `@src/client/tests.rs`:
- Line 5770: Update the JID value in the test attribute around the existing "to"
field to use a repository-valid fictional NANP number, such as 12025550111,
preserving the `@s.whatsapp.net` suffix.
In `@wacore/src/types/events.rs`:
- Around line 1724-1742: Add #[non_exhaustive] and #[derive(bon::Builder)] to
SentFrame, then update every SentFrame construction site to use
SentFrame::builder() instead of direct struct literals. Preserve the existing
plaintext field and serialization behavior.
🪄 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: e4f9eca2-7eb9-4571-aa54-115a548fe250
📒 Files selected for processing (8)
agent_docs/observability.mdsrc/client/accessors.rssrc/client/messaging.rssrc/client/tests.rssrc/lib.rssrc/socket/noise_socket.rssrc/test_utils.rswacore/src/types/events.rs
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
"Every outbound frame" overstated it: the XX/IK exchange writes to the transport before the noise socket exists, so handshake frames cross no chokepoint and are not forwarded. Say post-handshake, and scope "everything the client sends" to the session socket.
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: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
2 participants
Summary
Event::RawNodehands every decoded stanza to consumers before dispatch, lease-gated so it costs nothing when unused. The send side had no equivalent:wait_for_sent_nodeis a filtered one-shot waiter for a single expected stanza, and it only ever sees stanzas that pass throughmarshal_node_for_send, so the paths that hand pre-marshaled bytes straight to the socket (acks, delivery receipts, direct-encoded IQs) were invisible even to that. The consequences are the library's, not any consumer's: an integration test cannot assert what went to the wire without wrapping the transport, and a stanza the server rejected cannot be read back without rebuilding with logging on.Event::SentFramecarries the marshaled plaintext of each frame the transport accepted. It is emitted from the noise sender task, which is the single point every send path crosses and the same chokepointSessionStats::record_frame_sentalready sits on. Observing bytes rather than nodes is what makes one point sufficient: two of the send paths never build aNodeat all, and the bytes are what actually left. It is gated byClient::acquire_sent_frame_forwarding(), so an unwatched send is byte-for-byte the send it was before, and a watched one hands over the caller's own buffer rather than a copy.Audit
Send paths, all of which reach the transport through
NoiseSocket::enqueue_send:send_node→marshal_node_for_send→send_raw_bytes(src/client/messaging.rs:112,:121)send_raw_bytes(src/client/messaging.rs:14) — acks (src/client/node_io.rs:921,:938,:951,:990), delivery receipts (src/receipt.rs:611), direct-encoded IQs (src/request.rs:426)send_raw_bytes_burst(src/client/messaging.rs:53)start_transport_ack_worker→ burst (src/client/node_io.rs:843)src/message/dispatch.rs:202)send_keepalive→send_iq→send_node(src/keepalive.rs:96)Confirmed on the findings in the brief:
wait_for_sent_node(src/client/accessors.rs:611) registers aoneshotthatresolve_sent_node_waiters(src/client/accessors.rs:642) removes on the first match, and the filter is a required argument;send_raw_bytesnever calls it, so every path in row two was unobservable. Handshake frames are pre-noise and are deliberately not covered; VoIP relay sockets pass no observers, same as they pass no stats.The observation point is
NoiseSocket::sender_task, aftertransport.sendreturnsOk. Publishing before the batch's waiters are answered means a caller that awaited a send can already see its frame, which is what lets a test assert without polling.Changes
Event::SentFrame+SentFrame { plaintext: Bytes }(wacore),#[serde(skip)]on the payload for the reasonDecryptedPayload::payloadgives;EventKind::SentFrameappended last so no discriminant moves.SentFrameTaponClient: the lease counter plus the bus handle, behind anArcthe noise sender borrows. Same shape asSessionStats— the gate has to be readable from the send path, and the sender task cannot hold the client without keeping it alive.Client::acquire_sent_frame_forwarding() -> SentFrameLease, mirroring the raw-node and decrypted-payload leases.SendObserversreplaces the bareOption<Arc<SessionStats>>parameter onNoiseSocketanddo_handshake. Breaking forNoiseSocket::with_stats(nowwith_observers) and fordo_handshake's last argument; the alternative was a sixth positional parameter and a seventh for the next observer.catch_unwind: this runs on the task every send depends on, so an observer that panics must not end the connection's send half. A handler that blocks still stalls sends, which is the contract every bus handler already has on the read loop.GatedForwardingin the plugin host learns the new gated kind, as its own doc comment requires — a plugin subscribing toSentFramewithout a lease would otherwise get silence.wait_for_sent_nodeunchanged, and no existing test touched it. It answers a different question (theNodebefore marshalling, filtered, resolved before the write); rebuilding it on the tap would have to unmarshal every frame to test its filter and would widen what its ~18 callers see.send_raw_byteskeeps its bypass semantics, which were only ever about node logging and sent-node waiters.Cost
Allocations per send through the noise socket, min over 2000 windows against the process-wide counting allocator in
src/lib.rs:Idle is unchanged: one relaxed load per frame, and the
Vecthat would hold the plaintexts is never allocated. Watching costs a constant 2 per frame — theBytespromotion to a shared handle, and theArc<Event>the bus dispatches — and neither scales with the stanza:an_observed_frame_is_the_buffer_the_caller_handed_overasserts the observer'sByteshas the same data pointer as the one handed to the socket, so a 64 KiB stanza costs the same as a 64-byte one. (The committed test reports 4 and 6 because its measurement window also allocates the payload it sends; it asserts a ceiling on idle and an exact delta of 2.)size_of::<Client>(): 3912 → 3920, oneArc.Validation
Workspace clippy does not build in this sandbox (
alsa-syswants libasound), so the full matrix is left to CI.New tests:
every_send_path_is_observed_exactly_as_it_reached_the_wire(each path driven, observed frames compared byte for byte and in order against the transport's own frames decrypted under their counters, first frame decoded back to the stanza),sends_are_unobserved_until_a_consumer_asks,a_panicking_observer_leaves_the_client_sending, plus socket-levelan_observed_frame_is_the_buffer_the_caller_handed_over,an_unwatched_send_publishes_nothing,a_refused_write_is_not_observed,a_panicking_observer_does_not_break_the_send,watching_costs_a_constant_per_frame_and_idling_costs_nothing.Generated by Claude Code