Recover IQ request children and typed response shapes from smax idioms#2
Conversation
The request extractor dropped nested children built via composition helpers
it did not resolve. Handle them in resolve_child_node:
- `[].concat(a, b, …)` and array literals → flatten each element as children
- `WASmaxMixins.optionalMerge(mergeFn, stanza, …)` → resolve the stanza (it is
the 2nd arg, not the 1st); track WASmaxMixins as an owner
- `REPEATED_CHILD`/`OPTIONAL_CHILD` whose template is a function reference →
trace the function's return for the child template (mark repeats/optional)
Cuts request stanzas with dropped children from 44 to 18; the rest are built
via cross-module `merge…Mixin` fragments (`mergeStanzas`), a follow-up.
e.g. GroupsGetGroupInfo now yields query[phash] > add_request[expiration];
SpamGroupReport yields spam_list[jid,source] > {message, call} (repeated).
|
Warning Review limit reached
More reviews will be available in 43 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughExpands owner alias resolution; deepens request child-node tracing (arrays, templates, returns) with bounded recursion; captures and transitively merges mixin ChangesEnhanced Child Node and Mixin Resolution
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 74f6e148dd
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/wa-scan/src/request.rs (1)
261-290: ⚡ Quick winAdd targeted tests for the new template-resolution paths.
Please add tests that assert: (1) function-reference template resolution for
REPEATED_CHILD/OPTIONAL_CHILD, (2)WASmaxMixins.optionalMergeresolving the stanza arg, and (3)concatwith non-empty receiver.🤖 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 `@crates/wa-scan/src/request.rs` around lines 261 - 290, Add unit tests covering the new template-resolution paths exercised by resolve_template_arg: (1) a test where a template arg is a bare function reference that returns a stanza so resolve_function_return is invoked (exercise REPEATED_CHILD/OPTIONAL_CHILD via a function variable placed into VarScope), (2) a test for WASmaxMixins.optionalMerge where the stanza arg is resolved through resolve_child_node and merged, and (3) a test for concat where the receiver is non-empty to ensure the concat path returns correct child nodes; locate and invoke resolve_template_arg (and helpers resolve_child_node / resolve_function_return) with mocked/evaluated scope/module_source/aliases to assert the returned Vec<WapChildNode> matches expected nodes for each case.crates/wa-scan/src/alias.rs (1)
28-34: ⚡ Quick winAdd explicit regression coverage for the new tracked owners.
This change is correct, but adding a focused test for
WASmaxChildrenandWASmaxMixinsinrequire_owner/resolve_ownerwould guard this behavior from future regressions.🤖 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 `@crates/wa-scan/src/alias.rs` around lines 28 - 34, Add a focused regression test in the require_owner/resolve_owner test suite that verifies TRACKED_OWNERS includes and is handled for the new entries "WASmaxChildren" and "WASmaxMixins": locate the resolve_owner test (or function resolve_owner) and add cases that simulate inputs where owner resolution should return these two names, asserting the returned owner string/enum and any associated behavior (e.g., flagging as tracked) so future changes to TRACKED_OWNERS or resolve_owner will fail the test if these owners are removed or mishandled.
🤖 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 `@crates/wa-scan/src/request.rs`:
- Around line 221-235: The concat branch currently only iterates call.arguments
and therefore omits the receiver expression for method calls like
`[a].concat(b)`; update the is_concat handling in the same block (where
is_concat is set and the loop over call.arguments exists) to also extract the
receiver via arg_expr (e.g., from call.receiver or the AST field holding the
method receiver) and pass that expression into resolve_child_node(node_source,
scope, module_source, aliases) before/alongside the existing argument resolution
so the receiver's children are included.
---
Nitpick comments:
In `@crates/wa-scan/src/alias.rs`:
- Around line 28-34: Add a focused regression test in the
require_owner/resolve_owner test suite that verifies TRACKED_OWNERS includes and
is handled for the new entries "WASmaxChildren" and "WASmaxMixins": locate the
resolve_owner test (or function resolve_owner) and add cases that simulate
inputs where owner resolution should return these two names, asserting the
returned owner string/enum and any associated behavior (e.g., flagging as
tracked) so future changes to TRACKED_OWNERS or resolve_owner will fail the test
if these owners are removed or mishandled.
In `@crates/wa-scan/src/request.rs`:
- Around line 261-290: Add unit tests covering the new template-resolution paths
exercised by resolve_template_arg: (1) a test where a template arg is a bare
function reference that returns a stanza so resolve_function_return is invoked
(exercise REPEATED_CHILD/OPTIONAL_CHILD via a function variable placed into
VarScope), (2) a test for WASmaxMixins.optionalMerge where the stanza arg is
resolved through resolve_child_node and merged, and (3) a test for concat where
the receiver is non-empty to ensure the concat path returns correct child nodes;
locate and invoke resolve_template_arg (and helpers resolve_child_node /
resolve_function_return) with mocked/evaluated scope/module_source/aliases to
assert the returned Vec<WapChildNode> matches expected nodes for each case.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a4e20c3-a91a-4003-95fb-6f6bfc68cb49
⛔ Files ignored due to path filters (2)
generated/iq/index.jsonis excluded by!**/generated/**generated/manifest.jsonis excluded by!**/generated/**
📒 Files selected for processing (2)
crates/wa-scan/src/alias.rscrates/wa-scan/src/request.rs
Follow-ups extending the request-children recovery, including the Codex review:
- resolve_owner now unwraps `(n = o("X")).method()` parenthesized inline aliases.
This was breaking owner resolution broadly: it recovers 9 additional whole IQ
stanzas (146 -> 155) and 9 more typed responses, not just request children.
- `merge…MixinGroup` (disjunction wrapper) is unwrapped like `merge…Mixin`.
- resolve_template_arg prefers a template's *function* initializer over a
same-named non-function var (the flat scope conflated a template fn `e` with an
unrelated `var e = smax(…)` in a sibling helper, mislabeling the child).
- `recv.concat(extra)` now resolves the receiver too (per review), so
`[a].concat(b)` keeps `a`.
- Tests for the function-ref template, optionalMerge stanza, concat receiver,
and the full inline-alias merge/optionalMerge chain.
Request stanzas with dropped children: 18 -> 9.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a06a735cc8
ℹ️ 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".
Builds on the local-composition recovery; takes request stanzas with dropped
children from 9 to 0.
- resolve_function_return resolves each `return <expr>` through the normal child
resolver (preserving nesting) instead of a flat collector — a template like
`smax("description", …, smax("body", …))` now keeps `body` under `description`
(Codex review).
- It does so under a LEXICAL scope: the function's own vars (shifted to module
offsets) shadow the module scope, while the module scope still backs sibling
template fns (`REPEATED_CHILD(e, …)`). This fixes the flat-scope collision that
conflated same-named vars/fns across helpers.
- resolve_child_node unwraps parenthesized expressions.
- A `depth` bound on function-boundary crossings keeps the
resolve_child_node ↔ resolve_function_return recursion finite.
e.g. GroupsPromoteDemoteAdmin → admin > {promote,demote} > participant (repeated);
GroupsLinkSubGroups → links > link > group > hidden_group;
PreKeysFetchMissingPreKeys → key_fetch > user > device.
Remaining: cross-module `merge…Mixin` fragments (`mergeStanzas`) still add some
attrs (e.g. spam_list `spam_flow`/`subject`) not yet merged in — a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 041d53a8a9
ℹ️ 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".
Phase 2 requests build `<iq>` inline and fold sibling fragments via
`o("WASmaxOut…").merge…Mixin(dst, …)`. The xmlns/type were already
recovered from those mixins; their `<iq>` children (e.g. a report's
`spam_list{spam_flow, subject}`) were not, so cross-module attrs/children
were dropped.
Capture each mixin's `<iq>` children in the index and merge them by tag
into the referencing request (union attrs, recurse, promote repeats),
following `merged_callees` transitively. Locally-built fields win, so the
merge only ever adds.
Add a grounded `diagnostics.iq.crossModule` manifest counter
(requestsWithMixins / requestsEnriched / fieldsRecovered) so the recovery
is visible and regressable. Current bundle: 8 requests enriched, 10
fields recovered (spam reports' spam_flow/subject, group set-subject,
newsletter messages/statuses).
Tests: fragment-children capture, merge_children semantics (union/append/
repeats/recurse), transitive resolve, recovered-field counting, and an
end-to-end scan asserting the merge + the diagnostic counts.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/wa-scan/src/request.rs (1)
511-527: 💤 Low valueControl-flow branches (if/while/for) are skipped; returns inside them won't preserve nesting.
collect_returns_in_stmtonly descends intoBlockStatement, so a template likeif (x) { return smax("a", {}, smax("inner")); }falls through tofind_wap_calls_in_body, which flattens children. The comment says "builder returns are top-level" — if that's a valid domain assumption, this is fine. Otherwise, handlingIfStatement/SwitchStatementbodies would preserve nesting in conditional templates.🤖 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 `@crates/wa-scan/src/request.rs` around lines 511 - 527, collect_returns_in_stmt currently only descends into BlockStatement so returns inside control-flow (IfStatement, ForStatement, WhileStatement, SwitchStatement, etc.) are skipped and later flattened by find_wap_calls_in_body; update collect_returns_in_stmt to also recurse into the relevant child statement lists for control-flow constructs: for IfStatement recurse into its consequent and optional alternate, for ForStatement/ForInStatement/ForOfStatement and WhileStatement/DoWhileStatement recurse into their body, and for SwitchStatement recurse into each Case's consequent statements; keep extracting return argument spans the same and push to out so nesting is preserved.
🤖 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.
Nitpick comments:
In `@crates/wa-scan/src/request.rs`:
- Around line 511-527: collect_returns_in_stmt currently only descends into
BlockStatement so returns inside control-flow (IfStatement, ForStatement,
WhileStatement, SwitchStatement, etc.) are skipped and later flattened by
find_wap_calls_in_body; update collect_returns_in_stmt to also recurse into the
relevant child statement lists for control-flow constructs: for IfStatement
recurse into its consequent and optional alternate, for
ForStatement/ForInStatement/ForOfStatement and WhileStatement/DoWhileStatement
recurse into their body, and for SwitchStatement recurse into each Case's
consequent statements; keep extracting return argument spans the same and push
to out so nesting is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e5a5d2f-84ad-4cac-96f1-13288c2166c3
⛔ Files ignored due to path filters (2)
generated/iq/index.jsonis excluded by!**/generated/**generated/manifest.jsonis excluded by!**/generated/**
📒 Files selected for processing (6)
crates/wa-scan/src/alias.rscrates/wa-scan/src/lib.rscrates/wa-scan/src/mixin_index.rscrates/wa-scan/src/module.rscrates/wa-scan/src/request.rscrates/whatspec/src/main.rs
Some request stanzas get a child node's attrs not from the `<iq>` mixin but from a chain of mixins merged onto the SUB-stanza: newsletter `messages`/`statuses` build empty, then fold in `count` (a payload mixin), `before`/`after` (a MixinGroup union via optionalMerge), and `jid`/`key`/ `type`/`view_role` (a second union via the `smax$any` wildcard tag). Resolve each mixin's contribution (the stanza tree it folds into a destination) in dependency order, then make child resolution contribution-aware: a `merge…(dst,…)` / `optionalMerge(mergeFn,dst,…)` now folds the mixin's own attrs/children into the resolved `dst`. Handles the `smax$any` wildcard (attrs applied regardless of dst tag), MixinGroup unions (both `if`-branch variants unioned), and merge selectors whose method lacks the `Mixin` suffix (recognized by a `WASmaxOut*` owner). Opt-in and additive: resolution without contributions is byte-identical to before; the merge only ever adds. No stanza changes except the two newsletter requests; manifest fieldsRecovered 10 -> 24. Tests: contribution fold by tag, wildcard application, suffix-less cross merge, optionalMerge ref folding, union if-branch collection, structural- only fallback, and an end-to-end scan of the full mixin chain.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68ee1de92b
ℹ️ 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".
| let recovered = | ||
| crate::mixin_index::count_recovered_fields(&iq.children, &frag_children); | ||
| fields_recovered = fields_recovered.max(recovered); | ||
| crate::mixin_index::merge_children(&mut iq.children, &frag_children); |
There was a problem hiding this comment.
When a module contains more than one <iq> builder and only one path references a WASmaxOut… mixin, this applies the module-wide frag_children union to every resolved stanza, including locally complete requests that did not pass through that mixin. In that scenario the scanner will add unrelated child tags/attrs to sibling IQ definitions (and count them as recovered) because scanner.mixin_callees is collected for the whole module rather than per IqCall; the merge needs to use the mixins that enclose or feed this specific IQ call.
Useful? React with 👍 / 👎.
A node's attrs can come from a smax MixinGroup disjunction (`if(flagA) merge…A; if(flagB) merge…B; else throw`) — e.g. newsletter `messages` takes a jid-params XOR invite-params shape (discriminated by `type`) and, optionally, a before XOR after direction. Phase 3 folded all branches into one node, losing the mutual exclusivity and clobbering the discriminator (`type:"jid"` won over `type:"invite"`). Model them properly: WapChildNode gains `variant_groups`, each a set of mutually-exclusive `WapVariant`s (attrs/children), with `optional` set when folded via optionalMerge. The extractor detects a MixinGroup by its `MixinGroupExhaustiveError` and resolves each branch as a distinct variant (via per-return resolution + if-branch descent), anchored on the shared tag; `merge_children`/`count_recovered_fields` union variant groups. Codegen emits one Rust `enum` per group (variants named by the const discriminator, e.g. `Jid`/`Invite`), a spec field (`Option<…>` when optional), and a `match` in `build_iq` applying the chosen variant's attrs — replacing the flattened Optional-attr soup. Only the two newsletter requests change shape; manifest fieldsRecovered 24 -> 28. Tests: extractor union recovery (two groups, optional + required, smax$any wildcard, no leak) and codegen enum/field/match emit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23433a3c60
ℹ️ 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".
| let payload: Vec<String> = v | ||
| .attrs | ||
| .iter() | ||
| .filter(|a| !matches!(a.kind, WapAttrKind::Const | WapAttrKind::GeneratedId)) | ||
| .map(|a| { | ||
| format!( | ||
| "{}: {}", | ||
| rust_ident(&a.name), | ||
| crate::fields::rust_attr_type(&a.kind) | ||
| ) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
When a MixinGroup alternative contributes child stanzas rather than only attrs, the scanner now keeps those in WapVariant::children (see resolve_contribution), but the codegen here builds each enum variant payload solely from v.attrs and the later match only applies attrs to the node. In that scenario the generated build_iq silently omits the branch-specific child nodes, so requests recovered from such mixins are incomplete; the variant children need to be emitted and attached inside the corresponding match arm.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/wa-scan/src/mixin_index.rs (1)
476-498: ⚡ Quick winConsider adding tests for variant group merge/count scenarios.
The test helper
node()creates nodes with emptyvariant_groups. The new variant-group union logic inmerge_children(lines 464-468) andcount_recovered_fields(lines 406-410) lacks dedicated unit test coverage. Adding a test that verifies variant groups are correctly unioned and counted would strengthen confidence in the new behavior.💡 Example test for variant group merging
#[test] fn merge_children_unions_variant_groups() { use wa_ir::{WapVariantGroup, WapVariant}; let group_a = WapVariantGroup { optional: false, variants: vec![WapVariant { attrs: vec![attr("disc_a")], children: vec![], }], }; let group_b = WapVariantGroup { optional: true, variants: vec![WapVariant { attrs: vec![attr("disc_b")], children: vec![], }], }; let mut into = vec![WapChildNode { tag: "item".to_string(), attrs: vec![], children: vec![], repeats: false, variant_groups: vec![group_a.clone()], }]; let from = vec![WapChildNode { tag: "item".to_string(), attrs: vec![], children: vec![], repeats: false, variant_groups: vec![group_a.clone(), group_b.clone()], }]; merge_children(&mut into, &from); assert_eq!(into[0].variant_groups.len(), 2, "group_b added, group_a deduped"); }🤖 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 `@crates/wa-scan/src/mixin_index.rs` around lines 476 - 498, The tests currently use node() which sets empty variant_groups, so add unit tests that create WapChildNode instances with non-empty WapVariantGroup entries and exercise merge_children and count_recovered_fields; specifically, write a test that builds two child vectors where one has a WapVariantGroup A and the other has A plus B, call merge_children(&mut into, &from) and assert into[...].variant_groups contains both groups (A deduped, B added), and another test that uses count_recovered_fields to verify the recovered field count changes according to optional/required variant groups and repeats. Locate the helpers and functions by name (node, merge_children, count_recovered_fields) and assert on variant_groups.len() and the returned counts to validate unioning and counting behavior.
🤖 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.
Nitpick comments:
In `@crates/wa-scan/src/mixin_index.rs`:
- Around line 476-498: The tests currently use node() which sets empty
variant_groups, so add unit tests that create WapChildNode instances with
non-empty WapVariantGroup entries and exercise merge_children and
count_recovered_fields; specifically, write a test that builds two child vectors
where one has a WapVariantGroup A and the other has A plus B, call
merge_children(&mut into, &from) and assert into[...].variant_groups contains
both groups (A deduped, B added), and another test that uses
count_recovered_fields to verify the recovered field count changes according to
optional/required variant groups and repeats. Locate the helpers and functions
by name (node, merge_children, count_recovered_fields) and assert on
variant_groups.len() and the returned counts to validate unioning and counting
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 884db0a1-487e-4556-93ba-a28512833910
⛔ Files ignored due to path filters (3)
generated/iq/index.jsonis excluded by!**/generated/**generated/manifest.jsonis excluded by!**/generated/**generated/schema/iq.schema.jsonis excluded by!**/generated/**
📒 Files selected for processing (8)
crates/wa-codegen/src/emit.rscrates/wa-codegen/src/lib.rscrates/wa-codegen/src/spec.rscrates/wa-ir/src/iq.rscrates/wa-scan/src/lib.rscrates/wa-scan/src/mixin_index.rscrates/wa-scan/src/module.rscrates/wa-scan/src/request.rs
✅ Files skipped from review due to trivial changes (1)
- crates/wa-codegen/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/wa-scan/src/module.rs
- crates/wa-scan/src/request.rs
- crates/wa-scan/src/lib.rs
The codegen ignored `response.variants` entirely and emitted
`type Response = ()` for 63/155 specs, even though the IR records rich
RPC outcome unions (e.g. blocklist GET = 7 success/error variants).
Consume `response.variants`: emit a `#[derive(Default)]` struct per
variant (and its child item structs) wrapped in an `enum`, and a
`parse_response` that tries each variant in order (the RPC's own
discrimination — first whose required fields all read wins). Gated by
`parser_is_valid` per variant, so a union only generates when every arm
produces a clean parser; otherwise it falls back to the prior path (never
invalid code).
Also fix a latent `emit_response_parser` bug surfaced by this: a child
that is both `method=="child"` AND `repeats` took the single-child path
(flat attrs) instead of the Vec loop, and the repeated branch didn't
flatten same-node mixins — so a repeated child under a `source_path`
(blocklist `<list><item>`) generated a parser referencing non-existent
fields. Route repeated children to the loop and flatten their same-node
mixins.
Reference .rs only (gitignored; IR/manifest/schema unchanged):
Response=() 63 -> 50, outcome-union enums 0 -> 74. blocklist GET now
yields a 7-variant enum with Vec<Item{jid,lid,display_name}>.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8f5bdabb
ℹ️ 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".
| lines.push(format!("{indent}}})();")); | ||
| lines.push(format!( | ||
| "{indent}if let Ok(__v) = __r {{ return Ok({enum_name}::{vname}(__v)); }}" |
There was a problem hiding this comment.
When an earlier outcome variant only requires fields shared by all outcomes, this try-each parser can classify later error responses as that earlier success. For example, in the generated IR I checked, GetBlockListResponseSuccessWithMatch only requires the common type attr, while the error variants also include type; because emit_response_parser just reads the attr and the code returns on the first Ok, an error response with type present will be returned as SuccessWithMatch before the error arms are tried. The union parser needs to enforce the variant’s assertions/discriminator (or otherwise order/guard variants) before accepting a match.
Useful? React with 👍 / 👎.
The per-variant parser validator (`parser_is_valid`, shared with the non-union `can_generate` path) used `INIT_FIELD = (\w+),` to read which struct fields a generated parser initializes. A field named `type`/`match` is emitted as the raw identifier `r#type,`, from which the regex captured only `type` — so the validator thought `r#type` was never initialized and wrongly rejected an otherwise-correct parser, dropping the whole outcome union to `()`. Capture the optional `r#` prefix. Recovers 6 outcome-union responses whose success variant carries a `type` attr (UserNotice GetDisclosures/ GetDisclosureStageByIds, Groups PromoteDemote/PromoteDemoteAdmin, Bot BotList, Biz CtwaNativeAd UploadAdMedia). Reference .rs only (gitignored; IR/manifest/schema unchanged): Response=() 50 -> 44, outcome-union enums 74 -> 80. The remaining ~17 need deeper parser work (recursive nested-child reads; nested `type=union` response fields in Groups/Newsletters) — a separate effort.
Codex flagged that the try-each outcome-union parser can misclassify: a
variant whose required fields are a subset of a later variant's shadows it
(the earlier always matches first), so e.g. an error response is returned
as a bare type-only success. Root cause: the IR drops each RPC variant's
real discriminator — the JS parsers assert `literal(type,"result")`,
`flattenedChildWithTag("error")`, etc., but `response.variants` carry no
assertions (0/255), so the generated parser can only test required-field
presence.
Until those discriminators are captured, refuse to emit an enum when the
variants aren't unambiguously separable by their fail-on-absent fields
(required attrs/children): if any earlier variant's required set ⊆ a later
one's, fall back to the single-shape/`()` path instead of a parser that
lies. Reference .rs only (gitignored; IR/manifest/schema unchanged):
outcome-union enums 80 -> 33 (the 47 ambiguous ones revert to the
primary-success single shape, which Errs on non-matching responses rather
than misclassifying); Response=() unchanged at 44. blocklist GET is now a
single-shape struct, not a 7-variant enum that mislabeled errors.
Proper recovery (capturing per-variant discriminators in the response
extractor) is a follow-up.
Test: an ambiguous success/error pair must not generate an enum.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11088c10a
ℹ️ 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".
| d.attrs.push(a.clone()); | ||
| } | ||
| } | ||
| crate::mixin_index::merge_children(&mut d.children, &src.children); |
There was a problem hiding this comment.
When a cross-module contribution resolves to the same root tag as a locally built child and that contributed root is repeatable (for example a merge…Mixin/optionalMerge fragment whose root comes from REPEATED_CHILD or .map(...)), this merge copies attrs/children/variant groups but drops src.repeats. The sibling mixin_index::merge_children path promotes existing.repeats, so this Phase-3 path can leave the recovered IR/codegen treating a repeated child as singular whenever the local request already had that tag.
Useful? React with 👍 / 👎.
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 (1)
crates/wa-codegen/src/emit.rs (1)
502-515:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftVariant-group child stanzas are still being dropped.
WapVariantcan contributechildrenas well as attrs, but this path only modelsv.attrs: the enum payloads ignorev.children,variant_armonly mutates attrs onvar_name, andemit_child_buildernever recurses into variant-owned child nodes. Any recovered disjunction whose alternative is expressed as child stanzas rather than attrs will therefore build the wrong request shape.Also applies to: 543-549, 617-657
🤖 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 `@crates/wa-codegen/src/emit.rs` around lines 502 - 515, The enum payload construction is only using v.attrs and thus drops any WapVariant.children, so variants that express alternatives as child stanzas are lost; update the loop that builds payloads (the group.variants iteration that calls variant_name and builds payload Vec<String>) to include both v.attrs and v.children (transforming child nodes into the same "name: type" strings using the same type-resolution path as rust_attr_type or the existing emit_child_builder logic), ensure variant_arm mutations consider children as well (not just attrs on var_name), and make emit_child_builder recurse into variant-owned child nodes so child-derived fields are emitted into the variant payload and builders; apply the same fix pattern to the other occurrences around the regions noted (lines similar to 543-549 and 617-657).
🤖 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 `@crates/wa-codegen/src/lib.rs`:
- Around line 161-165: The loop currently skips emitting shared child-item
structs whenever op.response.variants is non-empty, which breaks the fallback in
generate_spec that uses the primary single-shape response when outcome-union
codegen returns None; change the logic so you do not unconditionally continue
for ops with response.variants—emit the shared child structs by default and only
suppress them after you successfully generated the outcome-union path.
Concretely: remove or relax the early continue that checks op.response.variants
in the loop over resolved, and instead only skip emitting primary-mirror child
structs when the outcome-union generation function (the codepath that may return
None) explicitly indicates the union path was selected; keep references to
generate_spec, op.response.fields and op.response.variants to locate where to
adjust the branching.
---
Outside diff comments:
In `@crates/wa-codegen/src/emit.rs`:
- Around line 502-515: The enum payload construction is only using v.attrs and
thus drops any WapVariant.children, so variants that express alternatives as
child stanzas are lost; update the loop that builds payloads (the group.variants
iteration that calls variant_name and builds payload Vec<String>) to include
both v.attrs and v.children (transforming child nodes into the same "name: type"
strings using the same type-resolution path as rust_attr_type or the existing
emit_child_builder logic), ensure variant_arm mutations consider children as
well (not just attrs on var_name), and make emit_child_builder recurse into
variant-owned child nodes so child-derived fields are emitted into the variant
payload and builders; apply the same fix pattern to the other occurrences around
the regions noted (lines similar to 543-549 and 617-657).
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81ad5625-2796-46c3-9e96-1cd45c7e325b
📒 Files selected for processing (3)
crates/wa-codegen/src/emit.rscrates/wa-codegen/src/lib.rscrates/wa-codegen/src/spec.rs
The outcome-union guard fell back to single-shape for any union whose
variants weren't separable by required fields — 47 of them — because the
IR carried no discriminators (the JS keeps them as parser asserts, not
fields, so `response.variants` had empty assertions).
Capture them: `literal(ACC, node, "attr", "value")` on the same node
records an `attr == value` assertion (the `type:"result"` discriminator),
and a same-node mixin's assertions bubble into the caller (so an error
variant inherits `type:"error"` from `parseIQErrorResponseMixin(e, …)`).
`Resolver::assertions` resolves these per parser (memoized, cycle-safe,
deduped); `response_index` stores them on each `ResponseVariant`.
Codegen then guards each try-each arm by its assertions
(`if response.get_attr("type") != Some("result") { bail }`), and the
ambiguity guard treats a conflicting attr assertion as separating two
otherwise-subset variants. So an error response can no longer be returned
as a bare type-only success.
IR is purely additive (identical ignoring assertions; 41 -> 548
assertions). Reference .rs: outcome-union enums 33 -> 43 (the type-split
unions recover as correctly-discriminated enums); Response=() unchanged
at 44. Variants distinguished only by an uncaptured discriminator (two
errors differing by `<error>` code; soft `optionalLiteral`; nested
`type=union` response fields) stay single-shape — honest, not
misclassifying.
Tests: extractor captures the literal value + bubbles the same-node mixin
(deduping the shared tag assert); codegen emits per-arm guards and the
conflict relaxation turns a subset success/error pair into an enum.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8348601668
ℹ️ 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".
| "method": "child", | ||
| "name": "wfDeleted", | ||
| "type": "string", | ||
| "required": false, | ||
| "tag": "wf_deleted", |
There was a problem hiding this comment.
This newly added response field marks <wf_deleted> as optional, but it is still emitted with method: "child". The response codegen only treats maybeChild as optional; the child path calls get_optional_child(...).ok_or_else(...), so a valid response that omits this optional child will fail parsing with a missing-child error. Please either emit maybeChild for optional child accessors or teach the codegen to respect required: false for child/content fields.
Useful? React with 👍 / 👎.
The generated IQ module (`generated/iq/iq.rs`) targets wacore downstream and compiles into no crate in this workspace, so no test would catch a codegen change that emits syntactically-broken Rust. Add a wa-codegen test that runs the committed reference IR through `generate_iq` and parses the result with `syn::parse_file` — every brace/expr/enum/match must be well-formed (resolution/types aren't checked; syn only parses). syn is a dev-dependency (already in the lockfile transitively). Validates the current generator output and guards future codegen changes.
Nested response unions (e.g. GroupInfo's member-add/link/share modes) are discriminated by guards the JS keeps as parser asserts, not fields — so the IR's `UnionVariant`s carried no way to tell a marker variant apart. Capture them agnostically (structural, no hardcoded names/values): - `literalContent(content, node, "admin_add")` → an `AssertionKind::Content` assertion (the node's text content pinned to a constant). Previously dropped, so marker variants looked empty/indistinguishable. - `UnionVariant` gains `assertions`, filled from the variant parser's same-node guards (`Resolver::assertions`) — `assertTag`, `literal` (attr=value), and the new content markers. So GroupInfo's `AdminAddMode` now carries `content == "admin_add"`, `AllMembersAddMode` `content == "all_member_add"`, and the plain-content fallback carries none — the discriminators a consumer (and the codegen) needs. IR is purely additive (identical ignoring assertions). Foundation for nested-union field codegen; the union-as-field generation consumes these next. Tests: marker union captures its content discriminator; the fallback carries none.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5780bf5a27
ℹ️ 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".
| if op.response.fields.is_empty() || !op.response.variants.is_empty() { | ||
| continue; |
There was a problem hiding this comment.
When an RPC has response.variants but emit_outcome_types returns None (the ambiguity fallback documented in generate_spec), generate_spec still builds the single primary response shape from op.response.fields. For generated IR entries such as makeGetBlockListRequestItem, that primary shape includes repeated children, so the response struct contains fields like Vec<...Item>; this unconditional skip prevents namespace_body from emitting the corresponding item structs because the op merely has variants. The generated Rust then references undefined child item types for those fallback RPCs.
Useful? React with 👍 / 👎.
The response parser was flat: only top-level attrs and one level of child descent were read, so specs with deeper nesting (GroupInfo, LinkedAccounts, LinkedGroup, InviteGroupInfo, EncCertificates) degraded to (). Rewrite emit_response_parser around a recursive emit_struct_reads that mirrors collect_response_fields branch-for-branch — attrs/content leaves, repeated children as Vec<Item> loops, and non-repeated children descended and flattened into the enclosing struct — so the parser inits exactly the fields collect derives at any depth (5 specs recovered: () 44 -> 39). A childless wrapper (a bare marker child that contributes no field) no longer emits a dead, required binding: descending into it read nothing yet made the whole parse bail whenever that optional marker was absent. A scratch run over cloned state decides whether the subtree reads any field before committing the binding, so empty wrappers are skipped entirely while wrappers that carry data (possibly under a different spec) are kept.
A `type=union` response field carries the smax disjunction's variants in the IR, but the reference codegen dropped them. Recover the two cleanly-modelable shapes as Rust enums, structurally (no name/value matching, so they survive WA version churn), and skip anything else so the rest of the response stays typed: * Content unions — variants pinned to a child's text content (<member_add_mode>admin_add</…>): each pinned value is a unit arm, an optional trailing variant with no pin is the fallback that captures the raw content. The child reads optionally, so an absent or unrecognized value decodes to None (a new server-side value lands in the fallback, never a hard error). * Same-node value unions — variants reading attr leaves off the same node, chosen first-success by which variant's required attrs are present; a separability check rejects a union whose earlier arm would shadow a later one. collect_response_fields and emit_response_parser gate on the same classify_union, so a union either becomes an Option<Enum> field the parser fills or is skipped by both. collect now returns the enum defs (threaded to the module-level shared-types pass); an op that carries response.variants but falls back to the single-shape struct now also emits its child structs/enums there. Recovers 10 unions (e.g. GroupInfo subject + member add/link/share modes); the participant union (nested unions in its variants) is correctly left unsupported.
💡 Codex Reviewhttps://github.com/oxidezap/whatspec/blob/89b63a9cc8aa35d0a8042301648b7fe6305475a6/generated/iq/index.json#L4835 This recovered IR now marks ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…r bails) The recursive response parser descended EVERY non-repeated child with a required `get_optional_child(tag).ok_or_else(...)?`, regardless of the field's required flag. But smax marks children read via `optionalChildWithTag` as required:false (e.g. a group's <suspended>/<ephemeral>/<linked_parent>), so a perfectly valid response that simply omits one made the whole parse fail with "missing <tag>". Now an optional child reads its subtree inside `if let Some(<tag>) = …` and binds every contributed field through a tuple, defaulting each in the `else`. Field types are unchanged (the else arm defaults each element), so collect_response_fields needs no weakening and required leaves INSIDE a present child still fail-fast. Required children keep the bailing descent. Recovers correct runtime behavior for the GroupInfo family and every other spec with optional sub-children. (Addresses a PR review finding; verified <suspended> et al. are optionalChildWithTag in the bundle.)
merge_node_into copied a contribution's attrs/children/variant-groups onto a same-tagged local child but dropped src.repeats, so a repeatable fragment (root from REPEATED_CHILD/.map(...)) folded onto a locally-singular child left it treated as singular. Mirror mixin_index::merge_children and OR the flag. No current request hits this (contract unchanged), but it removes a latent divergence between the two merge paths. Adds a unit test. (Addresses a PR review finding.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab074db23c
ℹ️ 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".
| for a in &call.arguments { | ||
| if let Some(e) = arg_expr(a) { | ||
| out.extend(resolve_child_node( |
There was a problem hiding this comment.
When WASmaxMixins.mergeStanzas is used with two resolvable stanzas that have the same tag, this loop appends both resolved arguments into out, so the recovered request contains duplicate sibling nodes instead of one node with the attrs/children fused. The scanner’s own mixin_index::merge_children documents and implements mergeStanzas as by-tag merging, so this branch should merge the resolved argument lists before returning; otherwise builders recovered from local mergeStanzas(smax("query", ...), smax("query", ...)) compositions can emit two <query>/<messages> children and miss the actual stanza shape.
Useful? React with 👍 / 👎.
An op whose RPC outcomes couldn't be separated into an enum falls back to a single-shape parser that mirrors the success variant. With optional children now defaulting instead of bailing, a non-success response (e.g. <iq type="error">) would decode to an all-default struct rather than failing. Emit the success variant's same-node discriminator (type:"result", captured in the IR) as a guard at the top of the fallback parser, so a non-success response fails the parse instead of masquerading as an empty success. Pure single-shape ops (no variants) are unaffected. Adds a unit test.
jlucaso1
changed the title
💡 Codex Reviewhttps://github.com/oxidezap/whatspec/blob/e0bc7bc9384d567ed05eb2294f946922d1710c64/generated/iq/index.json#L52349 When this newly recovered request child is marked ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…nerated lints
Compiling the generated iq.rs against the actual wacore crate (a smoke harness:
shim crate::iq::spec/crate::request to wacore, include the generated file) surfaced
133 type errors the syn syntax-net can't see: the discriminator guards compared
`get_attr(..).map(|x| x.as_str())` (Option<Cow<str>>, since ValueRef::as_str
returns Cow) against `Some("result")` (Option<&str>). Add `.as_deref()` so the
comparison is Option<&str> vs Option<&str>. One systemic fix clears all 133.
Also allow `dead_code`/`non_snake_case` in the generated header: it's a catalog a
consumer uses a subset of, and nested wrapper vars use a `base__path_wrap` naming
convention. With both changes the generated file compiles against real wacore with
zero errors and zero warnings.
💡 Codex Reviewhttps://github.com/oxidezap/whatspec/blob/84e0dd068827198eb857dfe9624880f7206d0b69/generated/iq/index.json#L1071-L1075 When https://github.com/oxidezap/whatspec/blob/84e0dd068827198eb857dfe9624880f7206d0b69/generated/iq/index.json#L72982-L72984 These newly recovered ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Extends the IQ extractor and reference codegen to recover both request children and typed response shapes that were previously dropped across the smax stanza builders.
Request side
resolve_child_nodenow handles the composition idioms the extractor used to miss:[].concat(a, …)/ array literals (including theconcatreceiver)WASmaxMixins.optionalMerge(mergeFn, stanza, …)REPEATED_CHILD/OPTIONAL_CHILDwith a function-reference template (nesting and repeats preserved)merge…Mixin/mergeStanzasfragments, folded into the destination tree — including MixinGroup disjunctions modeled as discriminated variant groupsResponse side
The codegen now consumes the response IR instead of emitting
():enum {Spec}Response { Variant(…), … }, each arm guarded by its captured discriminator (type:"result"vs"error"); an ambiguity guard falls back to a single shape rather than misclassifytype=unionfields → Rust enums: content-discriminated markers with a typed fallback, and same-node value unions chosen by required-attr presenceoptionalChildWithTag) read leniently — an absent optional child defaults instead of failing the whole parse; single-shape fallbacks still reject non-success responses via the success discriminatorNumbers
155 IQ stanzas → 135 typed / 20 degraded responses; 59 generated enums (49 outcome unions + 10 nested
type=union);()responses down to 39.Contract
The IR (
generated/*/index.json+ JSON Schema + versioned manifest) is the cross-language contract; the generated.rsis a reference consumer, syntax-checked in CI viasyn.whatspec update --checkstays idempotent (the IR is unchanged by the codegen-only commits).