refactor(protocol): clarify child parsing ownership - #1436
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
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: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe PR documents that ChangesProtocolNode documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This change documents the attribute-only scope of ProtocolNode derive and the explicit parsing model for child-bearing nodes. It does not alter protocol behavior or runtime code and is ready to merge. 🚥 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 |
|---|---|
| agent_docs/protocol_architecture.md | Corrects the derive capability table and removes duplicated child-parsing rationale. |
| wacore/derive/src/lib.rs | Documents the derive's attribute-only boundary and centralizes the rationale for explicit child parsing. |
Reviews (3): Last reviewed commit: "docs(protocol): clarify child node deriv..." | 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}
Merging this PR will improve performance by 8.14%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | sweep_idle_no_ttl[512] |
2.2 µs | 2 µs | +8.14% |
| Simulation | bench_jid_push_phash_form |
< 1 ns | < 1 ns | N/A |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing refactor/protocol-node-child-contract (0db12fa) with main (bf0fc76)2
Footnotes
-
12 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(ccafa13) during the generation of this report, so bf0fc76 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
9ed4383 to
3791e32
Compare
| /// Child fields are intentionally not accepted by this derive. Child | ||
| /// cardinality and duplicate handling differ across protocol responses, so | ||
| /// child-bearing nodes use an explicit implementation, optionally reusing the | ||
| /// shared parsing helpers, instead of hiding those decisions in generated code. | ||
| /// |
There was a problem hiding this comment.
This rustdoc repeats the child-cardinality and duplicate-handling rationale already recorded in protocol_architecture.md. The repository requires a decision's rationale to live at a single point so that duplicated explanations do not drift. Keep the capability boundary here, but consolidate the detailed rationale before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: wacore/derive/src/lib.rs
Line: 36-40
Comment:
**Duplicated design rationale**
This rustdoc repeats the child-cardinality and duplicate-handling rationale already recorded in `protocol_architecture.md`. The repository requires a decision's rationale to live at a single point so that duplicated explanations do not drift. Keep the capability boundary here, but consolidate the detailed rationale before merging.
**Context Used:** AGENTS.md ([source](https://github.com/oxidezap/whatsapp-rust/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 3791e3261f
ℹ️ 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".
| /// Child fields are intentionally not accepted by this derive. Child | ||
| /// cardinality and duplicate handling differ across protocol responses, so | ||
| /// child-bearing nodes use an explicit implementation, optionally reusing the | ||
| /// shared parsing helpers, instead of hiding those decisions in generated code. |
There was a problem hiding this comment.
This repeats the rationale already added in agent_docs/protocol_architecture.md lines 9–11, even though AGENTS.md requires a decision's “why” to live at a single point so it cannot go stale. Keep the support boundary in both places if useful, but retain the cardinality/duplicate-policy explanation in only one and reference it from the other.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| | --- | --- | | ||
| | `EmptyNode` | Nodes that are only a tag | | ||
| | `ProtocolNode` | Nodes with attributes and children | | ||
| | `ProtocolNode` | Nodes with attributes and children; derive support is currently attribute-only | |
There was a problem hiding this comment.
The table's For column still says the ProtocolNode derive is for nodes with children, while the same cell's qualifier and the newly added macro documentation say child fields are unsupported. Because this architecture guide is the repository-designated reference for choosing stanza derives, the contradictory entry can still send implementers toward a derive that will not compile for their child fields; describe it as attribute-only and discuss the broader trait separately.
AGENTS.md reference: AGENTS.md:L53-L56
Useful? React with 👍 / 👎.
Describe the broad ProtocolNode trait separately from the attribute-only derive, and keep the child implementation guidance factual without repeating macro rationale.
1 participant
Summary
The protocol architecture documentation described
ProtocolNodeas if the derive supported child fields, while the derive only accepts attributes. This patch records the actual boundary and why child-bearing nodes remain explicit.Evidence/Design
The trait supports nodes with attributes and children. The derive generates attribute-only implementations; 16 concrete implementations in the IQ layer parse children manually through borrowed
NodeRefhelpers, making required/optional, repeated, ordering, duplicate and error behavior visible. No macro syntax or protocol implementation was added, and no IQ was migrated.Cost
No runtime or generated-code cost. The documentation and one derive rustdoc comment add no executable code.
Validation
cargo fmt --allpassed.cargo test -p wacore-derivepassed: 0 unit tests, 3 ignored doctests.cargo check -p wacore --libpassed.cargo nextest run -p wacore --libandwacore-binarywere not rerun because this patch changes no implementation; the unchanged baseline suites are outside the affected contract.Migration: none. Existing derive users and manual child parsers keep the same API and borrowed parsing behavior.