refactor(events): seal ServerAck with non_exhaustive + bon builder - #1002
Merged
Conversation
First step of the pre-1.0 event-payload API freeze. Introduces bon and applies the target pattern to ServerAck as the proof of concept: - #[non_exhaustive] seals the struct so downstream can no longer exhaustively destructure it, making future field additions non-breaking for consumers. - #[derive(bon::Builder)] gives a zero-cost typestate builder as the construction path, since non_exhaustive blocks cross-crate struct literals. Required fields are checked at compile time; Option fields get maybe_* setters, so the ack parser passes the Option through directly instead of unwrap_or_default(). handle_ack_response now builds via ServerAck::builder(). The alloc guard (ack_miss_path_does_not_heap_allocate) stays green: the builder adds no allocation and is still gated behind has_handler_for. The remaining ~44 payload structs will follow the same pattern.
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBon is added as a shared workspace dependency and referenced from Changesbon Builder Adoption for ServerAck
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant NodeIO as handle_ack_response
participant ServerAck as ServerAck::builder()
NodeIO->>ServerAck: builder()
NodeIO->>ServerAck: id(id)
NodeIO->>ServerAck: maybe_class(class)
NodeIO->>ServerAck: maybe_from(from)
NodeIO->>ServerAck: maybe_timestamp(t)
NodeIO->>ServerAck: maybe_error(error)
ServerAck-->>NodeIO: built ServerAck
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 |
|---|---|
| wacore/src/types/events.rs | Adds #[non_exhaustive] and #[derive(bon::Builder)] to ServerAck, and updates the Event stability doc-comment to explain the rolling-out sealing approach; implementation is correct. |
| src/client/node_io.rs | Migrates ServerAck construction from a struct literal to the bon builder; maybe_* setters correctly accept the existing Option expressions, and the hot-miss-path gating on has_handler_for is preserved. |
| AGENTS.md | Updates the event-payload convention bullet to document the new #[non_exhaustive] + bon::Builder sealing approach, including the rolling-out caveat; consistent with the code changes. |
| Cargo.toml | Adds bon 3.9.3 to the workspace with default-features = false, features = ["std"], matching the workspace-wide pattern of explicit feature gating; no issues. |
| wacore/Cargo.toml | Adds bon from the workspace to wacore's dependencies; straightforward one-line addition. |
| Cargo.lock | Lockfile adds bon 3.9.3 and its proc-macro crate bon-macros 3.9.3 with the expected dependency graph; no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["handle_ack_response()"] --> B{has_handler_for\nServerAck?}
B -- No --> C["Hot-miss path:\nno allocation"]
B -- Yes --> D["ServerAck::builder()\n.id(…)\n.maybe_class(…)\n.maybe_from(…)\n.maybe_timestamp(…)\n.maybe_error(…)\n.build()"]
D --> E["Event::ServerAck(ack)"]
E --> F["event_bus.dispatch(…)"]
subgraph "wacore (same crate)"
G["#[non_exhaustive]\n#[derive(bon::Builder)]\npub struct ServerAck"]
G -->|"builder generated\nin same crate"| D
end
subgraph "External crates"
H["Consumer reads fields\nack.id / ack.class / …"]
I["Cross-crate struct literal\n→ E0639 compile error"]
end
F --> H
D -.->|"#[non_exhaustive] blocks"| I
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["handle_ack_response()"] --> B{has_handler_for\nServerAck?}
B -- No --> C["Hot-miss path:\nno allocation"]
B -- Yes --> D["ServerAck::builder()\n.id(…)\n.maybe_class(…)\n.maybe_from(…)\n.maybe_timestamp(…)\n.maybe_error(…)\n.build()"]
D --> E["Event::ServerAck(ack)"]
E --> F["event_bus.dispatch(…)"]
subgraph "wacore (same crate)"
G["#[non_exhaustive]\n#[derive(bon::Builder)]\npub struct ServerAck"]
G -->|"builder generated\nin same crate"| D
end
subgraph "External crates"
H["Consumer reads fields\nack.id / ack.class / …"]
I["Cross-crate struct literal\n→ E0639 compile error"]
end
F --> H
D -.->|"#[non_exhaustive] blocks"| I
Reviews (2): Last reviewed commit: "docs(events): align stability policy wit..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile flagged that AGENTS.md and the Event doc still said payloads stay constructible / non_exhaustive is deferred, which contradicts sealing ServerAck in this PR. Update both to document the actual policy: seal payloads with #[non_exhaustive] + a bon builder, construct via the builder, and note the seal is rolling out per struct.
![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.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Adds bon dependency, applies builder pattern + non_exhaustive to ServerAck, updates docs. No logic changes, low impact.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
This was referenced Jul 7, 2026
Merged
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
2 participants
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.
What
First step of the pre-1.0 event-payload API freeze (follow-up to the stability policy documented in #1000). It introduces
bonand applies the target sealing pattern to a single payload,ServerAck, as a proof of concept. The remaining ~44 payload structs will follow the same shape in later PRs.The pattern
Two orthogonal, zero-overhead mechanisms, one per side of the problem:
#[non_exhaustive]. Downstream can no longer exhaustively destructure the struct (must use a..rest), so adding a field later is non-breaking for consumers. This is the standardstd/tokio/hyperapproach.#[derive(bon::Builder)].#[non_exhaustive]blocks cross-crate struct literals, so the library needs a construction path.bongives a compile-time typestate builder that monomorphizes to the same code as a struct literal (no runtime cost). Required fields are checked at compile time;Optionfields getmaybe_*setters.handle_ack_responsenow builds viaServerAck::builder().id(...).maybe_class(...)...build(), passing theOptionattributes through directly instead ofunwrap_or_default().Why bon
It is the current state-of-the-art builder derive: zero runtime overhead, compile-time required-field checks,
maybe_*setters for optionals, and field additions stay non-breaking. It is a compile-time-only dependency, so there is no runtime or meaningful binary-size impact (the binary-size job will confirm).Verification
ack_miss_path_does_not_heap_allocatestays green: the builder adds no allocation and is still gated behindhas_handler_for, so the hot miss path is untouched.test_ack_dispatches_server_ack_eventgreen: the builder produces the same struct.E0639: cannot create non-exhaustive struct, confirming cross-crate construction now goes through the builder.cargo fmt --checkandcargo clippy -p wacore -p whatsapp-rust --testsclean.Follow-ups (not in this PR)
Eventvariants (PairingQrCode/PairingCode/PairingCodeRefresh) to newtypes.trybuildcompile-fail test to lock the seal permanently.EventInterestoffu64(currently 58/64 kinds used) as part of the freeze.