feat: type-safe IQ protocol architecture with derive macros - #237
Conversation
📝 WalkthroughWalkthroughIntroduce a type-safe IQ/spec framework and proc-macro derives in wacore; add many typed IQ modules (groups, usync, prekeys, mex, mediaconn, blocklist, etc.), wire a Client::execute(IqSpec) orchestration, add node parsing helpers and validated newtypes, and refactor runtime feature modules to call spec-driven IQ executions and safer parsers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Spec as IqSpec
participant Exec as Executor
participant Server as Server
Client->>Spec: construct spec (e.g., GroupQueryIq)
Client->>Exec: execute(spec)
Exec->>Spec: build_iq()
Note over Spec,Exec: Spec returns InfoQuery/Node payload
Exec->>Server: send(InfoQuery)
Server-->>Exec: Node (response)
Exec->>Spec: parse_response(Node)
Spec-->>Exec: typed Response
Exec-->>Client: Result<Response, IqError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/prekeys.rs (3)
128-128: Critical bug:OsRng.unwrap_err()will always panic.
unwrap_err()expects anErrvariant and panics onOk. SinceOsRngimplementsTryRngCoreand typically succeeds, this will panic at runtime. You likely meant to useOsRngdirectly or handle it properly.🐛 Proposed fix
- let key_pair = KeyPair::generate(&mut OsRng.unwrap_err()); + let key_pair = KeyPair::generate(&mut OsRng);If
KeyPair::generaterequiresCryptoRng + RngCore(whichOsRngimplements directly in rand 0.9), you can pass&mut OsRngdirectly. If you need theTryRngCoreinterface, use:let key_pair = KeyPair::generate(&mut OsRng.unwrap_mut());
79-93: Dead code:found_countis never incremented.The variable
found_countis initialized to 0 but never modified within the loop, making the conditionfound_count >= WANTED_PRE_KEY_COUNTalways false. This entire block appears to be incomplete or dead code.♻️ Suggested fix: Remove dead code or complete the implementation
If this block is meant to find existing unuploaded keys, it needs proper implementation. Otherwise, remove it:
- // Check if we have existing unuploaded keys by trying IDs sequentially - // We'll check a reasonable range to find existing keys - let found_count = 0; - for id in 1..=1000u32 { - if found_count >= WANTED_PRE_KEY_COUNT { - break; - } - - if let Ok(Some(_record)) = backend.load_prekey(id).await { - // Check if this key was already uploaded by seeing if it exists on server - // For simplicity, assume unuploaded keys have a specific pattern or we track separately - // For now, we'll use existing keys if available but generate new ones with sequential IDs - break; // We'll generate new ones with better tracking - } - }
96-112: Performance concern: Sequential scan of up to 16M pre-key IDs.This loop scans IDs from 1 to 16,777,215 sequentially, calling
backend.load_prekey(id)for each. This is O(n) where n can be up to 16M, which could take an extremely long time and block the async runtime.Per coding guidelines, heavy CPU-bound or blocking I/O tasks should be wrapped in
tokio::task::spawn_blocking.♻️ Suggested approach
Consider one of these alternatives:
- Track the highest pre-key ID in persistent storage instead of scanning:
// Store and retrieve highest_pre_key_id from backend let highest_existing_id = backend.get_highest_prekey_id().await?.unwrap_or(0);
- If scanning is necessary, use binary search if the backend supports it, or at minimum limit the scan range and wrap in
spawn_blocking:let highest_existing_id = tokio::task::spawn_blocking(move || { // scanning logic here }).await??;
- Use a gap-finding query if the backend supports it.
🤖 Fix all issues with AI agents
In `@wacore/Cargo.toml`:
- Line 36: wacore fails to build standalone because serde_json is pulled from
the workspace with default-features = false but wacore/src/iq/mex.rs calls
serde_json::to_vec and serde_json::from_slice which require the alloc feature;
update the serde_json entry in wacore/Cargo.toml to enable the alloc feature
(e.g. change serde_json = { workspace = true } to serde_json = { workspace =
true, features = ["alloc"], default-features = false }) so to_vec/from_slice
work when building the package alone.
In `@wacore/derive/src/lib.rs`:
- Around line 94-110: The generated try_from_node() body currently uses
node.attrs().string(`#attr_name`) for required attributes which returns an empty
string when missing; change those branches in the attr_fields -> field_parsers
map so required fields call
node.attrs().optional_string(`#attr_name`).ok_or_else(|| <appropriate error>)? (or
use .ok_or(...)?) and propagate the error with ? so missing required attributes
cause try_from_node() to return an error; update the generation logic that emits
`#field_ident` initializers to use optional_string + error propagation instead of
string() for fields where info.default is None.
In `@wacore/src/iq/mex.rs`:
- Around line 123-138: The serialization call using serde_json::to_vec(&payload)
in build_iq (where MexPayload wraps &serde_json::Value) is effectively
infallible here; instead of changing error handling semantics, document that
with a brief comment above the payload_bytes line explaining that MexPayload
contains only a &Value and serde_json::to_vec cannot fail in this usage (barring
custom serializers or non-string map keys), and keep the existing
unwrap_or_default() to preserve behavior; reference build_iq, MexPayload,
payload_bytes, and serde_json::to_vec in this comment.
- Around line 141-151: In parse_response, accept both NodeContent::Bytes and
NodeContent::String for the <result> node: locate the result_node extraction in
parse_response and match on result_node.content to handle
NodeContent::Bytes(bytes) by parsing via serde_json::from_slice(bytes) and
NodeContent::String(s) by parsing via serde_json::from_str(s), returning the
MexResponse in either case and returning the existing errors for missing or
unexpected content; reference the parse_response function, result_node,
NodeContent::Bytes, NodeContent::String, and the MexResponse type.
In `@wacore/src/types/message.rs`:
- Around line 6-21: The enum AddressingMode currently derives Serialize but will
serialize as "Pn"/"Lid" while as_str() and TryFrom expect "pn"/"lid"; add the
serde attribute #[serde(rename_all = "lowercase")] to the AddressingMode enum
declaration so serde will emit/accept lowercase values consistent with
AddressingMode::as_str() and any TryFrom implementations.
🧹 Nitpick comments (13)
wacore/derive/src/lib.rs (1)
58-71: Prefersyn::Errorover panics for macro diagnostics
expect/panic!in derive entrypoints yields “proc-macro panicked” without spans. Emittingsyn::Errorproduces clearer compiler output at the correct span.♻️ Suggested pattern
- let tag = extract_tag(&input.attrs).expect("ProtocolNode requires #[protocol(tag = \"...\")]"); + let tag = match extract_tag(&input.attrs) { + Some(tag) => tag, + None => { + return syn::Error::new_spanned( + &input.ident, + "ProtocolNode requires #[protocol(tag = \"...\")]", + ) + .to_compile_error() + .into(); + } + };- let str_val = str_value.unwrap_or_else(|| { - panic!("StringEnum variant {} requires #[str = \"...\"] attribute", variant_ident) - }); + let str_val = match str_value { + Some(v) => v, + None => { + return syn::Error::new_spanned( + variant_ident, + format!("StringEnum variant {} requires #[str = \"...\"] attribute", variant_ident), + ) + .to_compile_error() + .into(); + } + };Please sanity-check the diagnostics by compiling a minimal derive usage missing
#[protocol(tag = "...")]or#[str = "..."]and confirm the error spans are clear.Also applies to: 174-183, 331-336
wacore/src/iq/contacts.rs (2)
71-95: Consider takingJidby value to avoid unnecessary clones when caller already owns the JID.Per AGENTS.md, "IqSpec constructors should take
&Jidinstead ofJidto avoid forcing callers to clone." However, this creates a clone inside every constructor. If callers often already have an ownedJid, consider offering bothnew(jid: Jid, ...)andnew_ref(jid: &Jid, ...)variants, or acceptimpl Into<Jid>.This is a minor consistency point and the current approach is acceptable.
133-137: Emptyidon success may indicate a malformed response.When the picture node exists without an error,
iddefaults to an empty string if missing. This silently accepts potentially malformed responses. Consider logging a warning or returning an error whenidis missing in a successful response.Proposed fix
let id = picture_node .attrs() .optional_string("id") .map(|s| s.to_string()) - .unwrap_or_default(); + .ok_or_else(|| anyhow!("Picture response missing 'id' attribute"))?;wacore/src/iq/prekeys.rs (1)
84-96: Inconsistent error handling: missing<count>node returns error, but missingvalueattribute defaults silently.Line 87 returns an error if
<count>node is missing, but lines 89-93 silently default to0if thevalueattribute is missing or unparseable. Consider making this consistent—either both should error or both should default.Option 1: Error on missing/invalid value (stricter)
let count_str = count_node .attrs() .optional_string("value") - .unwrap_or("0"); - let count = count_str.parse::<usize>().unwrap_or(0); + .ok_or_else(|| anyhow!("Missing 'value' attribute on <count> node"))?; + let count = count_str.parse::<usize>() + .map_err(|e| anyhow!("Invalid count value '{}': {}", count_str, e))?;Option 2: Document the default behavior (lenient)
+ // Server may return <count/> without value attribute when count is 0 let count_str = count_node .attrs() .optional_string("value") .unwrap_or("0");AGENTS.md (1)
394-401: Add language specifier to fenced code block.The file organization tree should have a language specifier (e.g.,
textorplaintext) for markdown linting compliance.Proposed fix
-``` +```text wacore/src/iq/ ├── mod.rs # Re-exports ├── spec.rs # IqSpec trait definition ├── node.rs # Helper functions (required_child, required_attr, optional_attr) ├── groups.rs # Group types, enums, newtypes, ProtocolNode & IqSpec impls └── blocklist.rs # Blocklist types, ProtocolNode & IqSpec impls</details> </blockquote></details> <details> <summary>wacore/src/iq/usync.rs (3)</summary><blockquote> `162-177`: **Significant code duplication: phone formatting logic repeated across specs.** The phone number formatting logic (adding `+` prefix) is duplicated in `IsOnWhatsAppSpec::build_iq` (lines 166-176) and `ContactInfoSpec::build_iq` (lines 263-275). Consider extracting to a helper function. <details> <summary>Proposed refactor</summary> ```rust // Add helper function at module level fn build_phone_user_nodes(phones: &[String]) -> Vec<Node> { phones .iter() .map(|phone| { let phone_content = if phone.starts_with('+') { phone.clone() } else { format!("+{}", phone) }; NodeBuilder::new("user") .children(vec![NodeBuilder::new("contact") .string_content(phone_content) .build()]) .build() }) .collect() }Then use in both specs:
- let user_nodes: Vec<Node> = self - .phones - .iter() - .map(|phone| { - let phone_content = if phone.starts_with('+') { - phone.clone() - } else { - format!("+{}", phone) - }; - NodeBuilder::new("user") - .children(vec![NodeBuilder::new("contact") - .string_content(phone_content) - .build()]) - .build() - }) - .collect(); + let user_nodes = build_phone_user_nodes(&self.phones);
602-627: Consider using the helper functions fromnode.rsfor consistent parsing.The
DeviceListSpec::parse_responsemanually handles attribute extraction and error messages. Consider usingrequired_jidandrequired_attrfromwacore/src/iq/node.rsfor consistency with the documented patterns.Example refactor for JID extraction
+use crate::iq::node::required_jid; - let user_jid = user_node - .attrs() - .optional_jid("jid") - .ok_or_else(|| anyhow!("user node missing required 'jid' attribute"))?; + let user_jid = required_jid(user_node, "jid")?;
624-627: Missing<device-list>causes entire parse to fail for all users.If one user is missing the
<device-list>node, the entire response parsing fails. Consider logging a warning and skipping that user instead, similar to how other specs handle missing optional data.Proposed fix for resilient parsing
- let device_list_node = user_node + let device_list_node = match user_node .get_optional_child_by_tag(&["devices", "device-list"]) - .ok_or_else(|| anyhow!("<device-list> not found for user {user_jid}"))?; + { + Some(node) => node, + None => { + log::warn!("<device-list> not found for user {user_jid}, skipping"); + continue; + } + };src/features/mex.rs (1)
61-76: Clarify the purpose of the duplicate fatal error check.The comment on line 66 states "the IqSpec already checks, but we want to return our error type." If
IqSpecreturns an error on fatal responses, this check would never be reached. If it doesn't, the comment is misleading.Consider either:
- Removing the comment if
IqSpecdoesn't check for fatal errors- Clarifying that this converts successful responses with fatal errors to
MexErrorwacore/src/iq/blocklist.rs (2)
79-92: Consider logging when defaulting to "block" for missing action attribute.Line 84 silently defaults to
"block"when theactionattribute is missing. While this may be intentional for backwards compatibility, consider adding a debug/warn log to surface unexpected protocol responses during troubleshooting.♻️ Suggested improvement
fn try_from_node(node: &Node) -> Result<Self> { if node.tag != "item" { return Err(anyhow!("expected <item>, got <{}>", node.tag)); } - let action_str = optional_attr(node, "action").unwrap_or("block"); + let action_str = optional_attr(node, "action").unwrap_or_else(|| { + warn!(target: "blocklist", "Missing action attribute, defaulting to 'block'"); + "block" + }); let action = BlocklistAction::try_from(action_str)?;
162-174: Silent entry drops may cause incomplete blocklist data.The
filter_mapsilently discards entries that fail to parse, logging only a warning. The caller has no indication that some entries were dropped, which could lead to incorrect assumptions about the blocklist state (e.g., thinking a contact isn't blocked when parsing simply failed).Consider either:
- Collecting errors and returning a result with warnings
- Returning
Result<Vec<BlocklistEntry>>that fails on first parse error♻️ Alternative: fail-fast approach
let entries = items .iter() - .filter_map(|item| match BlocklistEntry::try_from_node(item) { - Ok(entry) => Some(entry), - Err(e) => { - warn!( - target: "blocklist", - "Failed to parse blocklist entry: {e}" - ); - None - } - }) - .collect(); + .map(|item| BlocklistEntry::try_from_node(item)) + .collect::<Result<Vec<_>>>()?;wacore/src/protocol.rs (1)
41-100: Macro only supportsStringfields; consider documenting this limitation.The macro assumes all fields are
Stringdue to:
- Line 78:
self.$field.to_string()ininto_node- Line 89:
s.to_string()intry_from_node- Line 98:
$default.to_string()in the default helperThis is reasonable for attribute-based nodes, but users may expect support for other types (e.g.,
u64,bool). Consider adding a note in the doc comment.📝 Documentation suggestion
/// Macro for defining simple protocol nodes with only attributes (no children). /// /// This macro generates a struct with the specified fields as attributes, /// and implements the `ProtocolNode` trait for it. +/// +/// **Note:** All fields must be `String` type. Attributes are serialized via +/// `to_string()` and deserialized via `optional_string()`. /// /// # Examplesrc/features/blocking.rs (1)
48-52: Consider caching or documenting the cost ofis_blocked.This method fetches the entire blocklist for each check, incurring a network round-trip and O(n) iteration. If called frequently (e.g., in a loop checking multiple contacts), this becomes expensive.
Consider:
- Documenting the cost in the method's doc comment
- Offering a batch variant that accepts multiple JIDs
- Adding optional client-side caching if the blocklist is relatively stable
📝 Documentation suggestion
/// Check if a contact is blocked. + /// + /// **Note:** This fetches the full blocklist on each call. For batch checks, + /// prefer calling `get_blocklist()` once and filtering locally. pub async fn is_blocked(&self, jid: &Jid) -> Result<bool> {
|
vc é simplesmente foda |
f348791 to
e3d136d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wacore/src/iq/mediaconn.rs`:
- Around line 64-88: The parse_response implementation for media_conn must
validate required attributes: replace usages of attrs.string("auth") and
host_node.attrs().string("hostname") with the required-attribute pattern (use
optional_string() and return an error when None) following the required_attr()
pattern in wacore::iq::node; ensure parse_response returns an anyhow::Error when
"auth" or any host "hostname" is missing and propagate those values into
MediaConnResponse and MediaConnHost accordingly.
♻️ Duplicate comments (2)
wacore/src/iq/mex.rs (1)
106-121: Consider adding a safety comment for the serialization.Per the previous review,
serde_json::to_vecon aMexPayloadcontaining a&serde_json::Valueis effectively infallible (only custom serializers or non-string map keys could cause failure, neither of which applies here). Consider adding a brief comment documenting this assumption to clarify whyunwrap_or_default()is acceptable.📝 Suggested documentation
let payload = MexPayload { variables: &self.variables, }; + // Safety: MexPayload wraps &serde_json::Value, and serde_json::to_vec + // cannot fail for Value (no custom serializers or non-string map keys). let payload_bytes = serde_json::to_vec(&payload).unwrap_or_default();wacore/src/types/message.rs (1)
6-13: LGTM! The#[serde(rename_all = "lowercase")]attribute correctly addresses the serialization consistency concern.The serde attribute ensures serialization output matches
as_str()andTryFromexpectations.
🧹 Nitpick comments (8)
wacore/src/iq/mex.rs (1)
28-36: Redundant#[serde(default)]onOptionfield.The
#[serde(default)]attribute onis_retryableis unnecessary sinceOption<T>fields already deserialize toNonewhen absent. Either remove it for consistency with the other fields, or add it to all fields if you want to be explicit.♻️ Suggested fix
pub struct MexErrorExtensions { pub error_code: Option<i32>, pub is_summary: Option<bool>, - #[serde(default)] pub is_retryable: Option<bool>, pub severity: Option<String>, }wacore/src/iq/usync.rs (1)
131-142: Inconsistentpicture_idtypes betweenContactInfoandUserInfo.
ContactInfo.picture_idisOption<u64>whileUserInfo.picture_idisOption<String>. If these represent the same server field, consider unifying the type. If intentionally different due to distinct wire formats, a clarifying comment would help.wacore/src/iq/blocklist.rs (1)
64-76: Consider logging when action attribute is missing.Line 69 silently defaults to
"block"when theactionattribute is missing. While this may be acceptable for robustness, consider logging a warning to help diagnose malformed responses.♻️ Suggested improvement
fn try_from_node(node: &Node) -> Result<Self> { if node.tag != "item" { return Err(anyhow!("expected <item>, got <{}>", node.tag)); } - let action_str = optional_attr(node, "action").unwrap_or("block"); + let action_str = optional_attr(node, "action").unwrap_or_else(|| { + log::warn!(target: "blocklist", "missing 'action' attribute, defaulting to 'block'"); + "block" + }); let action = BlocklistAction::try_from(action_str)?; let jid_str = optional_attr(node, "jid").ok_or_else(|| anyhow!("missing jid attribute"))?; let jid = jid_str.parse()?; Ok(Self { jid, action }) }src/features/blocking.rs (2)
43-48: Inconsistent return type with other methods.
get_blocklistreturnsResult<Vec<BlocklistEntry>>(anyhow) whileblockandunblockreturnResult<(), IqError>. Consider usingIqErrorconsistently for all IQ operations.♻️ Suggested fix
/// Get the full blocklist. - pub async fn get_blocklist(&self) -> Result<Vec<BlocklistEntry>> { + pub async fn get_blocklist(&self) -> Result<Vec<BlocklistEntry>, IqError> { debug!(target: "Blocking", "Fetching blocklist..."); let entries = self.client.execute(GetBlocklistSpec).await?; debug!(target: "Blocking", "Fetched {} blocked contacts", entries.len()); Ok(entries) }
50-54:is_blockedfetches entire blocklist for a single check.This approach requires a network round-trip and O(n) iteration for each check. If this is called frequently, consider caching the blocklist or implementing a server-side check if the protocol supports it.
wacore/src/iq/prekeys.rs (1)
80-92: Inconsistent error handling for missing<count>node vs missingvalueattribute.The code returns an error if the
<count>node is missing (line 83), but silently defaults to0if thevalueattribute is missing or unparseable (lines 87-88). Consider either being consistently lenient (return 0 if<count>is missing) or consistently strict (return error for missing/invalidvalue).♻️ Option A: Be consistently lenient
fn parse_response(&self, response: &Node) -> Result<Self::Response, anyhow::Error> { - let count_node = response - .get_optional_child("count") - .ok_or_else(|| anyhow!("Missing <count> node in response"))?; - - // Server may return <count/> without value attribute when count is 0, - // or return an unparseable value. Default to 0 in these cases. - let count_str = count_node.attrs().optional_string("value").unwrap_or("0"); - let count = count_str.parse::<usize>().unwrap_or(0); + let count = response + .get_optional_child("count") + .and_then(|n| n.attrs().optional_string("value")) + .and_then(|s| s.parse::<usize>().ok()) + .unwrap_or(0); Ok(PreKeyCountResponse { count }) }wacore/src/iq/mediaconn.rs (2)
26-30: Consider addingPartialEq, Eqderives.These structs would benefit from
PartialEqandEqderives for easier comparison in tests and potential use in collections.♻️ Optional enhancement
/// Media connection host information. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct MediaConnHost { pub hostname: String, }
91-153: Good test coverage with room for edge cases.The tests cover the main happy path and the missing node error case well. Consider adding tests for edge cases like missing
authattribute or empty hosts list to ensure robust error handling.💡 Optional: Additional edge case tests
#[test] fn test_media_conn_spec_parse_response_empty_hosts() { let spec = MediaConnSpec::new(); let response = NodeBuilder::new("iq") .attr("type", "result") .children([NodeBuilder::new("media_conn") .attr("auth", "test-auth-token") .attr("ttl", "3600") .build()]) .build(); let result = spec.parse_response(&response).unwrap(); assert!(result.hosts.is_empty()); } #[test] fn test_media_conn_spec_parse_response_missing_ttl() { let spec = MediaConnSpec::new(); let response = NodeBuilder::new("iq") .attr("type", "result") .children([NodeBuilder::new("media_conn") .attr("auth", "test-auth-token") .build()]) .build(); let result = spec.parse_response(&response).unwrap(); assert_eq!(result.ttl, 0); // defaults to 0 }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/features/blocking.rs`:
- Around line 50-54: The is_blocked function currently checks only jid.user
which causes false positives; update is_blocked to compare full JIDs from
get_blocklist() against the provided Jid (e.g., compare e.jid == *jid or their
string/full representation) so the resource and domain are considered; locate
the is_blocked method and replace the .jid.user comparison with a full Jid
equality/identity check that uses the Jid type's equality or canonical string
form.
In `@wacore/src/iq/blocklist.rs`:
- Around line 64-72: The code currently defaults a missing action attribute to
"block" in try_from_node which can cause silent unintended blocking; change the
retrieval of the action attribute (where action_str is set via optional_attr) to
fail if missing (similar to how jid_str is handled) by returning an Err with a
clear message, then parse that value with BlocklistAction::try_from as before
(i.e., replace the unwrap_or("block") usage with an ok_or_else(...) error path
so try_from_node returns an error when action is absent).
In `@wacore/tests/noise_handshake_test.rs`:
- Around line 176-186: The test currently drops the Result returned by
nh.authenticate(...) which can hide failures and trigger must_use warnings;
update the calls to nh.authenticate(&client_eph_pub) and
nh.authenticate(&server_eph_pub) to handle the Result explicitly (e.g., call
.expect("authenticate failed at step X") or unwrap with a clear message) so test
failures point to the exact authenticate invocation; keep assertions on
nh.hash() and related variables (hash_after_auth_client_eph) unchanged.
♻️ Duplicate comments (1)
wacore/src/iq/mediaconn.rs (1)
64-79: Validate required attributes instead of defaulting to empty strings.
authandhostnameare required; usingattrs.string()silently accepts missing data.🔧 Proposed fix
- let mut attrs = media_conn_node.attrs(); - let auth = attrs.string("auth"); + let mut attrs = media_conn_node.attrs(); + let auth = attrs + .optional_string("auth") + .ok_or_else(|| anyhow!("Missing auth attribute in media_conn response"))? + .to_string(); let ttl = attrs.optional_u64("ttl").unwrap_or(0); let max_buckets = attrs.optional_u64("max_buckets"); - let hosts = media_conn_node - .get_children_by_tag("host") - .iter() - .map(|host_node| MediaConnHost { - hostname: host_node.attrs().string("hostname"), - }) - .collect(); + let hosts = media_conn_node + .get_children_by_tag("host") + .iter() + .map(|host_node| { + let hostname = host_node + .attrs() + .optional_string("hostname") + .ok_or_else(|| anyhow!("Missing hostname attribute in media_conn response"))? + .to_string(); + Ok(MediaConnHost { hostname }) + }) + .collect::<Result<Vec<_>, anyhow::Error>>()?;
🧹 Nitpick comments (2)
wacore/src/iq/usync.rs (2)
54-93: Consider usingStringEnumderive for consistency.The
UsyncModeandUsyncContextenums manually implementas_str(), but other similar enums in this PR (e.g.,MemberAddMode,MemberLinkModeinwacore/src/iq/groups.rs) use theStringEnumderive macro. Consider aligning for consistency, though the current implementation is functionally correct.
124-142: Type inconsistency:picture_idisu64inContactInfobutStringinUserInfo.
ContactInfo.picture_idisOption<u64>whileUserInfo.picture_idisOption<String>. While this may reflect different wire formats, it could confuse consumers using both types. Consider documenting this difference or unifying toString(the more flexible type) if the protocol allows.
e3d136d to
398a430
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wacore/src/iq/groups.rs`:
- Around line 397-407: The participant parsing currently aborts the whole group
on any error from ParticipantType::try_from; change try_from_node so it attempts
to parse the optional "type" string via ParticipantType::try_from but on failure
(unknown/unrecognized type) it defaults participant_type to
ParticipantType::Member instead of returning Err—ensure genuine parse errors
unrelated to unknown type still propagate if needed, and update the
participant_type assignment (the call site using
ParticipantType::try_from(node.attrs().optional_string("type")) in
try_from_node) to catch the Err and assign ParticipantType::Member as a safe
fallback.
- Around line 266-347: The create-node builder uses raw options.participants
which can include phone_number for non-LID JIDs; call normalize_participants to
sanitize participants before constructing participant nodes. In
build_create_group_node replace iterating over &options.participants with
normalize_participants(&options.participants) (e.g., for participant in
normalize_participants(&options.participants) { ... }) so phone_number is
dropped for non-LID JIDs and payloads are safe by default.
♻️ Duplicate comments (4)
wacore/tests/noise_handshake_test.rs (1)
176-186: HandleauthenticateResult explicitly to surface failures.
Dropping theResultcan hide the failure point in this test. Please add anexpect(...)on both calls.wacore/derive/src/lib.rs (1)
130-133: Required attributes still silently default to empty strings.This was flagged in a previous review: when
info.defaultisNone, the field is considered required, butstring()returns""on missing attributes instead of erroring. Invalid nodes will parse successfully with empty required fields.Use
optional_string()with error propagation:Suggested fix
} else { quote! { - `#field_ident`: node.attrs().string(`#attr_name`).to_string() + `#field_ident`: node.attrs().optional_string(`#attr_name`) + .ok_or_else(|| anyhow::anyhow!("missing required attribute: {}", `#attr_name`))? + .to_string() } }wacore/src/iq/blocklist.rs (1)
69-73: Defaulting missingactionattribute to "block" may cause unintended behavior.While the warning log was added, silently defaulting to
"block"when the attribute is missing could still lead to unexpected blocking operations. Consider failing the parse instead.🔧 Proposed fix
- let action_str = optional_attr(node, "action").unwrap_or_else(|| { - warn!(target: "blocklist", "missing 'action' attribute, defaulting to 'block'"); - "block" - }); + let action_str = optional_attr(node, "action") + .ok_or_else(|| anyhow!("missing 'action' attribute"))?;src/features/blocking.rs (1)
50-54: Compare full JIDs to avoid false positives.Comparing only
jid.usermay incorrectly report blocked status for different JID variants (e.g., different servers or resources).🔧 Proposed fix
pub async fn is_blocked(&self, jid: &Jid) -> Result<bool> { let blocklist = self.get_blocklist().await?; - Ok(blocklist.iter().any(|e| e.jid.user == jid.user)) + Ok(blocklist.iter().any(|e| &e.jid == jid)) }
🧹 Nitpick comments (9)
wacore/src/iq/mediaconn.rs (2)
77-85: Consider validating that at least one host is returned.The wire format shows multiple hosts, but no validation ensures
hostsis non-empty. Downstream code (media upload/download) likely requires at least one host to function. If the server returns zero hosts, it may be better to fail fast here rather than later.♻️ Optional validation
for host_node in media_conn_node.get_children_by_tag("host") { let hostname = host_node .attrs() .optional_string("hostname") .ok_or_else(|| anyhow!("Missing 'hostname' attribute in host node"))? .to_string(); hosts.push(MediaConnHost { hostname }); } + + if hosts.is_empty() { + return Err(anyhow!("No hosts returned in media_conn response")); + } Ok(MediaConnResponse {
149-157: Consider adding tests for missing required attributes.The current tests cover the happy path and missing
media_connnode. Consider adding tests for:
- Missing
authattribute (should error)- Missing
hostnamein a host node (should error)These would validate the error handling added per the previous review.
💚 Additional test cases
#[test] fn test_media_conn_spec_parse_response_missing_auth() { let spec = MediaConnSpec::new(); let response = NodeBuilder::new("iq") .attr("type", "result") .children([NodeBuilder::new("media_conn") .attr("ttl", "3600") .build()]) .build(); let result = spec.parse_response(&response); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("auth")); } #[test] fn test_media_conn_spec_parse_response_missing_hostname() { let spec = MediaConnSpec::new(); let response = NodeBuilder::new("iq") .attr("type", "result") .children([NodeBuilder::new("media_conn") .attr("auth", "test-auth") .attr("ttl", "3600") .children([NodeBuilder::new("host").build()]) .build()]) .build(); let result = spec.parse_response(&response); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("hostname")); }wacore/src/iq/mex.rs (1)
137-145: Consider whether callers might need access to the full response even on fatal errors.When a fatal error is detected, the parsed
MexResponseis discarded and only the error message is returned. If callers ever need to inspect partialdataor othererrorsalongside the fatal error, this design prevents that.Current behavior is reasonable for most use cases. If future requirements need full response access on fatal errors, consider returning a custom error type that includes the
MexResponse.wacore/derive/src/lib.rs (2)
253-270: Silently swallowed parse errors degrade DX for malformed attributes.When
#[protocol(tag)]is written without= "value", the parse error is swallowed bylet _ = ...and the user sees a generic "requires #[protocol(tag = "...")]" message instead of the actual parse failure. Consider propagating or logging the parse error for better diagnostics.Suggested improvement
fn extract_tag(attrs: &[syn::Attribute]) -> Option<String> { for attr in attrs { if attr.path().is_ident("protocol") { let mut tag = None; - let _ = attr.parse_nested_meta(|meta| { + if let Err(e) = attr.parse_nested_meta(|meta| { if meta.path.is_ident("tag") { let value: syn::LitStr = meta.value()?.parse()?; tag = Some(value.value()); } Ok(()) - }); + }) { + // Could emit a compile error here with the specific parse failure + // For now, fall through to the generic error + let _ = e; + } if tag.is_some() { return tag; } } } None }
358-385: Malformed#[str]attributes produce misleading errors.If a user writes
#[str(value = "foo")]or#[str = 123](non-string literal), the if-let chain on lines 361-366 silently fails to match, and the error message on line 375 says "requires #[str = "..."]" — implying the attribute is missing when it's actually malformed.Consider adding explicit validation for the attribute format to produce a clearer error message.
wacore/src/iq/blocklist.rs (1)
143-155: Consider whether silently skipping malformed entries is desired.Using
filter_mapwith a warning log allows the response to partially succeed when some entries fail to parse. This is resilient but may hide protocol issues. If strict parsing is preferred, consider collecting errors and failing if any entry is invalid.wacore/src/iq/prekeys.rs (1)
85-91: Defensive parsing may hide protocol issues.Using
unwrap_or("0")andunwrap_or(0)silently defaults to 0 for missing or unparsable values. While the comment explains this is intentional for server edge cases, consider logging when defaults are applied to aid debugging.🔧 Suggested improvement with logging
// Server may return <count/> without value attribute when count is 0, // or return an unparseable value. Default to 0 in these cases. - let count_str = count_node.attrs().optional_string("value").unwrap_or("0"); - let count = count_str.parse::<usize>().unwrap_or(0); + let count_str = count_node.attrs().optional_string("value").unwrap_or_else(|| { + log::debug!(target: "prekeys", "Missing 'value' attribute in <count>, defaulting to 0"); + "0" + }); + let count = count_str.parse::<usize>().unwrap_or_else(|_| { + log::warn!(target: "prekeys", "Unparseable count value: {}, defaulting to 0", count_str); + 0 + });wacore/src/iq/contacts.rs (1)
108-119: Consider distinguishing 401 (unauthorized) from 404 (not found).Currently both error codes 401 and 404 return
Ok(None). While 404 means the picture doesn't exist, 401 typically indicates authorization failure. Depending on the protocol semantics, you might want to return an error for 401 to signal the caller that access was denied rather than the picture being absent.🔧 Proposed alternative handling
// Check for error response if let Some(error_node) = picture_node.get_optional_child("error") { let code = error_node.attrs().optional_string("code").unwrap_or("0"); - if code == "404" || code == "401" { + if code == "404" { return Ok(None); } + if code == "401" { + return Err(anyhow!("Unauthorized to access profile picture")); + } let text = error_node .attrs() .optional_string("text") .unwrap_or("unknown error"); return Err(anyhow!("Profile picture error {}: {}", code, text)); }wacore/src/iq/usync.rs (1)
168-190: Consider extracting a shared builder for the common usync envelope.The four
build_iqmethods repeat the sameusyncwrapper (sid/mode/last/index/context + list). A small helper would reduce duplication and keep defaults consistent.Also applies to: 241-269, 357-396, 525-558
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/features/blocking.rs`:
- Line 8: Remove the `use anyhow::Result;` import and update the function return
types that currently use `Result<(), IqError>` to explicit results: for
functions that should return an anyhow error use `anyhow::Result<()>`, and for
those that need to preserve `IqError` use `std::result::Result<(), IqError>`;
search for the occurrences of `Result<(), IqError>` in this file (the functions
around the tokens on lines previously flagged) and replace their return type
accordingly, keeping the `IqError` type where intended and using
`anyhow::Result` for generic anyhow errors.
In `@src/message.rs`:
- Around line 1141-1151: parse_message_info currently falls back to an empty id
which causes cache/key collisions; instead make the missing "id" an error: in
parse_message_info (where attrs.optional_string("id") is used) return
Err(anyhow::anyhow!("missing 'id' attribute'")) when id is None and do not
construct MessageInfo with an empty string; update any callers/tests that expect
parsing to succeed for messages lacking id and add a unit test for
parse_message_info to assert that a missing id yields an error to prevent silent
collisions in cache/keys.
In `@wacore/derive/src/lib.rs`:
- Around line 153-173: The generated impl uses a crate-relative path
"crate::protocol::ProtocolNode" which breaks when the macro is used from
external crates; update the quote! in lib.rs so the impl refers to the absolute
path "::wacore::protocol::ProtocolNode" (and similarly convert any other
crate-relative paths in the same expanded block if present) so the generated
code resolves correctly when the macro is invoked from outside the wacore
workspace.
In `@wacore/src/usync.rs`:
- Around line 76-85: The code currently uses device_id_str.parse()? which
propagates parse errors and aborts parsing; update the loop that iterates
device_list_node and device_node to handle non-numeric or out-of-range IDs by
attempting device_id_str.parse::<u16>() in a match (or map_err) and on Err log a
warn (target: "usync") stating the device id is invalid and skip that device
(continue) instead of returning an error; keep the existing behavior for missing
id (attrs().optional_string("id")) unchanged and only change the parsing of
device_id.
♻️ Duplicate comments (1)
wacore/src/iq/blocklist.rs (1)
64-76: LGTM! Previous issue addressed.The
actionattribute is now properly required with explicit error handling viaok_or_else, eliminating the previous issue of silently defaulting to "block" when the attribute was missing.
🧹 Nitpick comments (8)
src/receipt.rs (1)
16-20: Consider adding context to the missing-id warning.Including
from(and possiblyreceipt_type_stror node info) would make malformed receipts easier to trace in logs.♻️ Suggested tweak
- None => { - log::warn!("Receipt stanza missing required 'id' attribute"); + None => { + log::warn!("Receipt stanza missing required 'id' attribute (from: {from})"); return; }wacore/appstate/src/patch_decode.rs (2)
62-65: Consider usingrequired_stringif available.The pattern
optional_string(...).ok_or_else(...)works but is verbose for a required field. If the attribute API provides arequired_stringmethod (given the commit message mentions "required or optional" variants), using it would be more direct:let name_str = ag.required_string("name")?.to_string();If
required_stringisn't available yet, the current approach is fine.
32-44: Redundantunwrap_or—from_strnever fails.The
FromStrimpl always returnsOk(...)with the_ => Self::Unknowncatch-all, sounwrap_or(WAPatchName::Unknown)on line 97 is dead code. You can simplify to just.unwrap()(infallible) or restructure to avoid the confusion.Alternatively, if you want
from_strto actually fail on unknown values, returnErr(())in the catch-all and keep theunwrap_or.Option A: Simplify call site (since from_str is infallible)
- name: WAPatchName::from_str(&name_str).unwrap_or(WAPatchName::Unknown), + name: WAPatchName::from_str(&name_str).unwrap(), // infallibleOption B: Make from_str fallible and keep unwrap_or
impl FromStr for WAPatchName { type Err = (); fn from_str(s: &str) -> Result<Self, Self::Err> { - Ok(match s { + match s { "critical_block" => Self::CriticalBlock, "critical_unblock_low" => Self::CriticalUnblockLow, "regular_low" => Self::RegularLow, "regular_high" => Self::RegularHigh, "regular" => Self::Regular, - _ => Self::Unknown, - }) + _ => Err(()), + } } }Also applies to: 97-97
src/spam_report.rs (1)
43-78: Consider migrating tests to the non-deprecated attribute API.The
#[allow(deprecated)]suppresses warnings for the deprecated.string()method. Per the commit message, the new API uses.required()or.optional()variants. Consider updating these test assertions to use the new API for consistency and to serve as documentation of the preferred approach.For example:
- assert_eq!(node.attrs().string("spam_flow"), "MessageMenu"); + assert_eq!(node.attrs().required("spam_flow"), "MessageMenu");wacore/src/iq/groups.rs (2)
196-264: Consider validating subject before sending the create request.
GroupCreateOptionsuses rawStringfor subject whileGroupSubjectprovides length validation. An empty or over-length subject will be rejected by the server. Consider validating inGroupCreateIq::build_iqor requiringGroupSubjectin the options.♻️ Option A: Validate in build_iq
impl IqSpec for GroupCreateIq { type Response = Jid; fn build_iq(&self) -> InfoQuery<'static> { + // Consider returning Result and validating subject here + // GroupSubject::new(&self.options.subject)?; InfoQuery::set(♻️ Option B: Use GroupSubject in options
pub struct GroupCreateOptions { - #[builder(setter(into))] - pub subject: String, + pub subject: GroupSubject,
449-480: Minor duplication in group ID parsing.The pattern for parsing group IDs (lines 455-459) is duplicated in
GroupCreateIq::parse_response(lines 646-650). Consider extracting to a helper for consistency.♻️ Suggested helper
fn parse_group_jid(id_str: &str) -> Result<Jid> { if id_str.contains('@') { id_str.parse().map_err(Into::into) } else { Ok(Jid::group(id_str)) } }wacore/derive/src/lib.rs (2)
140-151: Consider documenting or reconsidering Default behavior for required fields.Fields without a
defaultattribute (i.e., required) getString::new()in the generatedDefaultimpl. This creates instances where required fields are empty—values that would failtry_from_nodevalidation if round-tripped. While this may be intentional for builder patterns, it could surprise users.Consider either:
- Documenting this behavior in the macro docs, or
- Making
Defaultderive opt-in via a separate attribute like#[protocol(derive_default)]
255-272: Consider surfacing parse errors for better diagnostics.The
let _ = attr.parse_nested_meta(...)pattern silently discards errors. If a user writes malformed syntax like#[protocol(tag = 123)], they'll see "requires#[protocol(tag = ...)]" rather than a specific parse error.♻️ Suggested improvement
fn extract_tag(attrs: &[syn::Attribute]) -> Option<String> { for attr in attrs { if attr.path().is_ident("protocol") { let mut tag = None; - let _ = attr.parse_nested_meta(|meta| { + if let Err(e) = attr.parse_nested_meta(|meta| { if meta.path.is_ident("tag") { let value: syn::LitStr = meta.value()?.parse()?; tag = Some(value.value()); } Ok(()) - }); + }) { + // Optionally: return Err(e) to surface parse errors + // For now, continue to next attribute + } if tag.is_some() { return tag; } } } None }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 394-401: Update the fenced code block that starts with the
directory listing "wacore/src/iq/" to include a language tag by changing the
opening ``` to ```text (so the block becomes ```text ... ```), which satisfies
markdownlint MD040; leave the closing ``` as-is and ensure no other characters
are added to the block fence.
🧹 Nitpick comments (1)
wacore/src/iq/groups.rs (1)
143-199: Enforce group-create limits before building the IQ.
GroupCreateOptions.subjectis a rawString, and participant count isn’t constrained, so callers can construct invalid payloads. Consider switchingsubjecttoGroupSubject(or adding a lightweight validation helper) so oversize subjects or >GROUP_SIZE_LIMITparticipants fail fast and align with the type-safe architecture.
| ``` | ||
| wacore/src/iq/ | ||
| ├── mod.rs # Re-exports | ||
| ├── spec.rs # IqSpec trait definition | ||
| ├── node.rs # Helper functions (required_child, required_attr, optional_attr) | ||
| ├── groups.rs # Group types, enums, newtypes, ProtocolNode & IqSpec impls | ||
| └── blocklist.rs # Blocklist types, ProtocolNode & IqSpec impls | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block.
markdownlint MD040 flags this block; adding a language keeps the docs lint-clean.
💡 Suggested fix
-```
+```text
wacore/src/iq/
├── mod.rs # Re-exports
├── spec.rs # IqSpec trait definition
├── node.rs # Helper functions (required_child, required_attr, optional_attr)
├── groups.rs # Group types, enums, newtypes, ProtocolNode & IqSpec impls
└── blocklist.rs # Blocklist types, ProtocolNode & IqSpec impls
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| wacore/src/iq/ | |
| ├── mod.rs # Re-exports | |
| ├── spec.rs # IqSpec trait definition | |
| ├── node.rs # Helper functions (required_child, required_attr, optional_attr) | |
| ├── groups.rs # Group types, enums, newtypes, ProtocolNode & IqSpec impls | |
| └── blocklist.rs # Blocklist types, ProtocolNode & IqSpec impls | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
394-394: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@AGENTS.md` around lines 394 - 401, Update the fenced code block that starts
with the directory listing "wacore/src/iq/" to include a language tag by
changing the opening ``` to ```text (so the block becomes ```text ... ```),
which satisfies markdownlint MD040; leave the closing ``` as-is and ensure no
other characters are added to the block fence.
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)
src/client.rs (1)
1541-1548: Avoid sending a pong with an emptyidwhen the incoming ping lacks one.An empty
idcan violate IQ semantics. Prefer omitting the attribute if it’s absent.💡 Suggested fix
- let id = parser.optional_string("id").unwrap_or("").to_string(); - let pong = NodeBuilder::new("iq") - .attrs([ - ("to", from_jid.to_string()), - ("id", id), - ("type", "result".to_string()), - ]) - .build(); + let mut builder = NodeBuilder::new("iq") + .attr("to", from_jid.to_string()) + .attr("type", "result"); + if let Some(id) = parser.optional_string("id") { + builder = builder.attr("id", id); + } + let pong = builder.build();src/prekeys.rs (1)
114-131: Offload key generation to avoid blocking the async runtime.While
KeyPair::generate(&mut OsRng.unwrap_err())is the correct RNG construction for Signal Protocol, generating 50 key pairs in the asyncupload_pre_keysfunction will stall the runtime. Wrap the loop intokio::task::spawn_blockingas per coding guidelines for CPU-bound work.🔧 Proposed fix
+ let (key_pair, pre_key_record) = tokio::task::spawn_blocking(move || { + use rand::TryRngCore; + let key_pair = KeyPair::generate(&mut OsRng.unwrap_err()); + let pre_key_record = new_pre_key_record(pre_key_id, &key_pair); + (key_pair, pre_key_record) + }) + .await + .map_err(|e| anyhow::anyhow!("pre-key generation task failed: {e}"))?;
🤖 Fix all issues with AI agents
In `@wacore/src/iq/dirty.rs`:
- Around line 93-106: The single() constructor currently swallows parse errors
for the timestamp; change its signature to surface invalid timestamps by either
accepting Option<u64> instead of Option<&str> or returning Result<Self,
ParseError>; locate CleanDirtyBitsSpec::single and replace the current
ts.parse() branch so that parse failures produce an Err (or propagate the
invalid input error) rather than falling back to DirtyBit::new, and ensure
callers handle the Result (or pass a pre-parsed timestamp) before calling
DirtyBit::with_timestamp(DirtyType::from(dirty_type), ts_num).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/usync.rs (1)
39-57: Avoid logging phone numbers (PII) in LID mapping logs.Both warn/debug lines emit
mapping.phone_number, which is sensitive and can violate privacy/compliance expectations. Please redact or omit the phone number in logs.🛡️ Proposed redaction
- warn!( - "Failed to persist LID {} -> {} from usync: {err}", - mapping.lid, mapping.phone_number, - ); + warn!( + "Failed to persist LID mapping for {} from usync: {err}", + mapping.lid + ); @@ - debug!( - "Learned LID mapping from usync: {} -> {}", - mapping.lid, mapping.phone_number - ); + debug!( + "Learned LID mapping from usync for {}", + mapping.lid + );
🤖 Fix all issues with AI agents
In `@src/handlers/message.rs`:
- Around line 30-39: The current extraction of chat_id using
node.attrs().optional_string("from") accepts empty strings which collapses all
empty-from stanzas into the same queue key; update the matching logic in
message.rs so that after calling node.attrs().optional_string("from") you treat
Some(id) where id.is_empty() as invalid: log a warning similar to the missing
case ("Message stanza has empty 'from' attribute") and return false; keep the
successful branch as chat_id = id.to_string(). This change should be applied to
the code around node.attrs().optional_string("from") and the chat_id binding.
In `@wacore/derive/src/lib.rs`:
- Around line 245-262: The extract_tag function swallows parse errors from
attr.parse_nested_meta, hiding malformed-attribute errors; change extract_tag to
return Result<Option<String>, syn::Error>, propagate and return any syn::Error
produced by parse_nested_meta instead of ignoring it, and update callers to
handle the Result (e.g., propagate with ? or convert to a user-facing error).
Specifically, modify extract_tag (and any call sites) so parse_nested_meta's
closure returns Err on bad literal types and the outer function forwards that
Err rather than discarding it.
🧹 Nitpick comments (2)
wacore/derive/src/lib.rs (2)
132-142: Default impl may create invalid protocol instances for required fields.Required fields (those without
default = "...") getString::new()in the Default impl. If users callDefault::default()and use these instances directly,try_from_nodewould reject them due to missing required attributes.Consider either:
- Not deriving Default when required fields exist, or
- Documenting that Default instances may not be protocol-valid
This is low-risk since
Defaultis mostly used for builder patterns, but worth noting.
264-293: Same silent error pattern asextract_tag.Line 272 also uses
let _ = attr.parse_nested_meta(...). Apply the same fix pattern here for consistent error reporting.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@wacore/derive/src/lib.rs`:
- Around line 351-387: The code currently allows multiple #[string_default]
attributes and simply overwrites default_variant; update the logic where
is_default is handled (the block that sets default_variant =
Some(variant_ident.clone())) to detect if default_variant.is_some() already and,
if so, return a syn::Error::new_spanned(...) converted via
to_compile_error().into() that reports "Multiple #[string_default] attributes
found; only one variant may be the default" (use variant_ident or the previously
set variant to span the error) so the compiler emits a clear error instead of
silently choosing the last one.
- Around line 274-297: The code currently treats a present #[attr(...)] without
a required name as Ok(None) which silently ignores malformed usage; update the
loop that iterates field.attrs (the block using attr.parse_nested_meta and the
subsequent if let Some(name) check) to emit a compile-time error instead: if the
attribute path is_ident("attr") and after parsing attr_name is None, return
Err(syn::Error::new_spanned(attr, "missing required `name` in
#[attr(...)]").into()); keep the successful branch that returns
Ok(Some(AttrFieldInfo { field_ident, attr_name: name, default })) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wacore/derive/src/lib.rs`:
- Around line 338-472: The derive_string_enum macro currently assumes all enum
variants are unit variants; update derive_string_enum to iterate each variant
(inside the for variant in variants loop) and check variant.fields is Unit
(i.e., matches syn::Fields::Unit), and if any variant has tuple or struct fields
emit a syn::Error::new_spanned on that variant_ident with a clear message like
"StringEnum only supports unit variants" (returning .to_compile_error().into());
ensure this check runs before using variant_ident in generated match arms so the
macro fails with a helpful error for non-unit variants.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wacore/derive/src/lib.rs`:
- Around line 338-482: The macro derive_string_enum currently allows duplicate
#[str = "..."] values which makes the generated TryFrom<&str> ambiguous; modify
the collection loop that builds variant_infos (in derive_string_enum) to track
seen string values (e.g., with a temporary HashMap<String, Ident> or BTreeMap)
and if a str_val is already present emit and return a syn::Error::new_spanned
referencing the current variant_ident (and optionally the previously-seen
variant ident) explaining the duplicate; perform this duplicate check
immediately after determining str_val (before pushing into variant_infos) so
duplicates are rejected at expansion time and the try_from_arms generation
remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wacore/derive/src/lib.rs`:
- Around line 135-145: The generated Default impl builds default_fields from
attr_fields and fills missing defaults with String::new(), which produces
invalid nodes for required attributes; change the generator to only emit the
Default implementation when all attr_fields have info.default present (e.g.,
check attr_fields.iter().all(|info| info.default.is_some())) and otherwise skip
generating Default for the type (or fall back to deriving Default only when
every field has a default); update the logic that builds default_fields (and the
surrounding code that emits the Default impl) to be conditional on that check so
fields with info.field_ident and info.default are only used when safe.
2 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.