smax: fix optional-mixin discriminator bleed + capture <ack> publish read-shapes - #26
Conversation
Mechanical regen against current bundles to give the follow-up smax fix and the new message-publish-ack domain a reproducible base on top of the merged #25. Version-string bump across all domains plus real WA content churn: mexOperations 131→129, abPropsConfigs 2047→2050, wamEvents 428→429. No code change — the smax fix and ack domain land in the following commits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
A smax parser's same-node cross-module mixins bubble their discriminating assertions up to the caller, so an error variant inherits the `type="error"` its error mixin pins. But a mixin whose result the tail consumes *optionally* (`X.success ? X.value : null`) does not constrain the node — the parse succeeds whether or not it matched — so its asserts are not invariants and must not bubble. They were bubbling unconditionally, stamping discriminators a message need not satisfy: - srvreq `WASmaxInMessageDeliverNewsletterRequest` gained a bogus `type="media"` from the optional `ReceiverContentTypeMediaRCATMixin`, even though the newsletter's real content type is the 14-way disjunction below it (`newsletterMessageFanoutContent`). - iq union variants inherited stray discriminators from optional mixins: `NewsletterWAMOEmpty` carried both `edit="3"` and `edit="8"` (impossible for one node); `CreateParticipantAdded` falsely required `addressable="false"` (from an optional participant mixin), colliding with the real `ParticipantNotAddressable` variant that legitimately pins it. Compute, per parser, the vars its tail consumes optionally and suppress the bubble for those (a var that is also hard-guarded by `if(!X.success) return X` stays required). Every genuine discriminator — the guarded-mixin and direct `literal(...)` pins — is preserved; only the bogus bleed is removed. Adds a regression test covering the optional-vs-required split. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (2)
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 |
A client publish/send receives an `<ack>` parsed by the newer WASmaxParseUtils
railway (`WASmaxIn*Publish*Response{Success,Negative}`), not the legacy
WADeprecatedWapParser — so the incoming domain, which only ran the legacy pass,
was blind to every send-ack read-shape.
Add a smax pass to the incoming scan: for each `WASmaxIn*Response{Success,
Negative}` root asserting the top-level `<ack>`, recover its field tree through
the shared Resolver (the same machinery srvreq/iq use), folding in the ack
envelope (from/class/id/t), the edit/revoke/pin sub-mixins, the paid-conversation
and question-response/message-ack disjunctions, and the negative-ack
error/backoff fields. iq `…Response*` modules assert `iq` and are dropped by the
ack tag gate; `…Mixin` fragments are folded into a root, never catalogued
standalone.
Recovers 5 ack read-shapes (incoming 11→16): the message-publish newsletter ack
(success + negative), status-publish newsletter status ack (success + negative),
and receipt-publish view ack. Additive to the IR (new entries only, no schema
change). Adds a scan test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
abbb992 to
df252ca
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
crates/wa-scan/src/response_smax.rs, recomputingguarded_success_vars(body)inside the filter can repeatedly traverse the AST and allocate a newHashSetper optional variable, which risks avoidable CPU/memory overhead in larger scans; this looks like a performance inefficiency rather than a functional break, so precompute the set once per function before merging to de-risk runtime cost.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
`guarded_success_vars(body)` was called inside the `suppressed` filter closure, so it re-traversed the AST and allocated a fresh HashSet for every optionally-consumed var. Hoist it to a single binding before the filter. Behavior-identical (same set) — generated output unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Auto-approved: Bug fix for optional-mixin discriminator bleed, new incoming ack read-shape scanning, and a mechanical version refresh. All changes are well-tested and additive to the IR; no production or security impact.
Re-trigger cubic
|
| Filename | Overview |
|---|---|
| crates/wa-scan/src/response_smax.rs | Adds optional-mixin suppression logic: guarded_success_vars, optionally_consumed_vars, tail_make_result_arg, optional_ternary_var, and collect_optional_vars. The module filter uses contains("Mixin") instead of ends_with("Mixin"), slightly over-broad relative to the stated intent but safe for current WA naming conventions. |
| crates/wa-scan/src/incoming.rs | Adds scan_incoming_smax_acks to recover read-shapes via the shared Resolver; correctly gates on the ack tag assertion, deduplicates repeated asserts, and excludes Mixin and iq modules. Well-tested with a new scan test. |
| generated/incoming/index.json | Adds 5 new ack read-shapes (11 to 16): newsletter message publish success/negative, newsletter status publish success/negative, and receipt publish view. Purely additive. |
| generated/iq/index.json | Removes 6 bogus optional-mixin discriminators (stray edit and addressable=false asserts) as a direct consequence of the bug fix. |
| generated/srvreq/index.json | Removes bogus type=media from WASmaxInMessageDeliverNewsletterRequest and refreshes version strings. |
| generated/manifest.json | Version bump to WhatsApp 2.3000.1042692684 and domain entry count updates reflecting the new incoming ack shapes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[analyze_function] --> B[guarded_success_vars]
A --> C[tail_return]
C --> D[optionally_consumed_vars]
D --> E[tail_make_result_arg]
E --> F[collect_optional_vars]
F --> G{arg shape?}
G -->|X.success ? X.value : _| H[optional_ternary_var - add X]
G -->|babelHelpers.extends| I[recurse each arg]
G -->|object literal| J[walk properties]
B --> K[suppressed = optionally_consumed minus guarded]
D --> K
K --> L[classify_call suppress_bubble?]
L -->|same-node and not suppress| M[bubble assertions]
L -->|same-node and suppress| N[skip bubble]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[analyze_function] --> B[guarded_success_vars]
A --> C[tail_return]
C --> D[optionally_consumed_vars]
D --> E[tail_make_result_arg]
E --> F[collect_optional_vars]
F --> G{arg shape?}
G -->|X.success ? X.value : _| H[optional_ternary_var - add X]
G -->|babelHelpers.extends| I[recurse each arg]
G -->|object literal| J[walk properties]
B --> K[suppressed = optionally_consumed minus guarded]
D --> K
K --> L[classify_call suppress_bubble?]
L -->|same-node and not suppress| M[bubble assertions]
L -->|same-node and suppress| N[skip bubble]
Reviews (2): Last reviewed commit: "fix(smax): tighten optional-ternary dete..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ession Two edge-case hardenings in `collect_optional_vars` (no current bundle hits either — output unchanged — but they keep the discriminator-bleed fix robust against future WA patterns): - An optional read is now recognized only when the ternary's test is the same var's `.success` bit as the consequent's `.value` (`X.success ? X.value : null`). A `cond ? X.value : fallback` with an unrelated test no longer marks X optional, which would have wrongly suppressed a genuine discriminator. - A bare `X.success ? X.value : null` spread straight into `babelHelpers.extends(…)` (not wrapped in an object literal) is now detected as optional; previously it matched neither the object nor the extends arm, so such a mixin's discriminator would still bubble. Factored the check into `optional_ternary_var` and applied it in both the object-property and extends-arg paths. Adds a regression test for the bare extends spread. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Non-trivial changes to core parser analysis logic (mixin discriminator fix and new ack scanning), affecting generated spec correctness. Requires human review despite clean cubic report.
Re-trigger cubic
2 participants
Summary
Two self-contained smax improvements, plus a version refresh to give them a reproducible base on top of the merged #25.
This started as "capture incoming message content-types" but the investigation showed that premise was already satisfied: regular incoming message bodies are legacy protobuf (
proto+incomingdomains), and the smaxWASmaxInMessageDeliver*family is newsletter-only with a single root thatsrvreqalready captures — including the full 14-way content-type union. What was actually left is a real correctness bug and one genuinely-uncaptured smax surface (the send acks). This PR does both.1.
fix(smax)— don't bubble an optionally-consumed mixin's discriminatorsA smax parser's same-node cross-module mixins bubble their discriminating assertions up to the caller (so an error variant inherits the
type="error"its error mixin pins). But a mixin whose result the tail consumes optionally (X.success ? X.value : null) does not constrain the node — the parse succeeds whether or not it matched — so its asserts are not invariants and must not bubble.They were bubbling unconditionally, stamping discriminators a message need not satisfy:
WASmaxInMessageDeliverNewsletterRequestgained a bogustype="media"from the optionalReceiverContentTypeMediaRCATMixin, contradicting the 14-waynewsletterMessageFanoutContentdisjunction right below it.NewsletterWAMOEmptycarried bothedit="3"andedit="8"(impossible for one node);CreateParticipantAddedfalsely requiredaddressable="false"(from an optional participant mixin), colliding with the realParticipantNotAddressablevariant that legitimately pins it.The fix computes, per parser, the vars its tail consumes optionally and suppresses the bubble for those — a var also hard-guarded by
if(!X.success) return Xstays required. This is the safe direction: bubble by default (preserving all existing behavior), suppress only the optional case. Every genuine discriminator is preserved (guarded-mixin bubbles and directliteral(...)pins are untouched); only the bogus bleed is removed. Verified case-by-case against a full regen — 1 srvreq + 6 iq assertion removals, all confirmed bogus, zero legitimate discriminators lost. Adds a regression test covering the optional-vs-required split.2.
feat(incoming)— capture smax<ack>publish read-shapesA client publish/send receives an
<ack>parsed by the newerWASmaxParseUtilsrailway (WASmaxIn*Publish*Response{Success,Negative}), not the legacyWADeprecatedWapParser— so the incoming domain, which only ran the legacy pass, was blind to every send-ack read-shape.Adds a smax pass to the incoming scan: for each
WASmaxIn*Response{Success,Negative}root asserting the top-level<ack>, recover its field tree through the sharedResolver(the same machinerysrvreq/iquse), folding in the ack envelope (from/class/id/t), the edit/revoke/pin sub-mixins, the paid-conversation and question-response/message-ack disjunctions, and the negative-ack error/backoff fields. iq…Response*modules assertiqand are dropped by the ack tag gate;…Mixinfragments are folded into a root, never catalogued standalone.Recovers 5 ack read-shapes (incoming 11→16): message-publish newsletter ack (success + negative), status-publish newsletter status ack (success + negative), and receipt-publish view ack. Purely additive to the IR (new entries only, no schema change). Adds a scan test.
3.
chore— refresh generated specs to WhatsApp 2.3000.1042692684Mechanical regen against current bundles so the two changes above sit on a reproducible base after #25 merged and
mainauto-bumped. Version-string bump across domains plus real WA content churn (mexOperations 131→129, abPropsConfigs 2047→2050, wamEvents 428→429). No code — the fix and ack domain land in the following commits, keeping each concern isolated.Validation
Full CI-equivalent gate green locally:
fmt,clippy --workspace --all-targets --all-features -D warnings,cargo test --workspace(incl. the two new smax tests),wa-ir --features schema,whatspec --no-default-features, C-free (cargo treeshows no ring/aws-lc/openssl-sys),validate-schemas.py generated, and the byte-identical determinism--check.🤖 Generated with Claude Code
https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Generated by Claude Code
Summary by cubic
Fixes smax optional-mixin discriminator bleed, adds smax
<ack>publish read-shapes to incoming, and refreshes generated specs to WhatsApp 2.3000.1042692684.Bug Fixes
X.success ? X.value : null(same var) and handle bare ternary spreads inbabelHelpers.extends(...).literal(...)asserts; only bogus asserts are removed.type="media"insrvreqnewsletter deliver and strayedit/addressable="false"asserts iniq. Adds tests for the optional-vs-required split and bare-extends case.New Features
<ack>read-shapes by scanningWASmaxIn*Publish*Response{Success,Negative}roots via the sharedResolver.…Mixinexports andiqresponses; dedupes repeated tag asserts. Additive only.Written for commit ca5bf13. Summary will update on new commits.