proto2 field defaults + honest MSRV - #25
Merged
Merged
Conversation
The extractor skipped `internalDefaults`, so proto2 `[default = X]` was
dropped from every field that declares one — a proto2 consumer (e.g.
whatsapp-rust) then sees the type-zero value instead of WA's declared
default on an absent field.
Parse `X.internalDefaults = {field: <expr>}` alongside `internalSpec` and
attach the resolved default to its field. Handles every value shape WA
uses: an enum-variant member (`s.E2EE` / `c.Foo.E2EE` -> the bare variant
name), a numeric literal, a negated number (`-1`), and a boolean written
`!1`/`!0`. Defaults apply only to singular message fields — never oneof
members or repeated fields, which protoc rejects.
Adds a `ProtoField.default` IR field (omitted when serialized, so it's
additive) and emits `[default = X]` (a `string` default quoted, others
bare), combined with `[packed = true]` via a joined options list.
Recovers 27 defaults across ~18 messages (accountType=E2EE,
trigger=UNKNOWN, taskId=-1, messageVersion=1, status=PENDING, …). The
protox compile guard confirms every default resolves against its field's
enum type; output is deterministic and the change is purely additive.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
`rust-version = "1.85"` was aspirational: the crates use let-chains (`if let … && let …`), stabilized in Rust 1.88, so 1.85 can't build them. Someone on 1.85 got a confusing mid-compile error instead of a clear "requires rustc 1.88" message from cargo. Declare the honest floor. 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. |
📝 WalkthroughWalkthroughThis PR raises the workspace Rust version to 1.88, adds an optional ChangesProto2 Default Value Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Extractor as extract_proto_from_modules
participant Collector as DefaultsCollector
participant Ident
participant BuildField as build_field
participant Stringify as field_line
Extractor->>Collector: visit parsed program
Collector-->>Extractor: internalDefaults (alias, field->default)
Extractor->>Ident: assign resolved defaults map
Extractor->>BuildField: build_entity -> resolve_member -> build_field(defaults)
BuildField-->>BuildField: compute default for singular non-repeated field
BuildField-->>Stringify: ProtoField with default
Stringify-->>Stringify: emit [packed, default] option list
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| crates/wa-proto/src/extract.rs | Adds DefaultsCollector AST visitor, parse_defaults, resolve_default, and format_number; correctly guards defaults from map/repeated/oneof fields; tests cover all resolved value shapes including !0→true. |
| crates/wa-proto/src/stringify.rs | Refactors field_line to join packed/default options; adds proto2 string escaping for string and bytes types; new emits_proto2_defaults test validates enum, bool, int, and escaped-string output. |
| crates/wa-ir/src/proto.rs | Adds optional ProtoField.default field with skip_serializing_if = None, keeping the schema change purely additive. |
| crates/wa-scan/src/request.rs | Refactors nested if/if-let into a single let-chain using Rust 1.88 syntax; semantics are preserved, code is cleaner. |
| crates/wa-codegen/src/emit.rs | Replaces s.len() % 2 != 0 with !s.len().is_multiple_of(2) (stabilized in Rust 1.87, within the new MSRV floor); no behavior change. |
| Cargo.toml | Bumps rust-version from 1.85 to 1.88 to reflect the actual minimum required by let-chain usage; metadata-only change. |
| generated/proto/WAProto.proto | Generated file gains 27 [default = X] annotations across enum, bool, and integer fields; purely additive and validated by the protox compile guard. |
| generated/manifest.json | SHA-256 hash updated to reflect the new WAProto.proto content; expected change. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[JS bundle module] -->|parse AST| B[DefaultsCollector]
A -->|parse AST| C[ContentsCollector / internalSpec]
B -->|Vec of target_alias → field→token| D[attach to Ident.defaults]
C -->|Vec of MemberDesc| E[build_entity]
D --> E
E -->|for each MemberDesc| F{member type?}
F -->|OneOf| G[build_field with empty defaults map]
F -->|Field| H[build_field with Ident.defaults]
H --> I{field qualifies?}
I -->|message_member=true, not repeated, not map| J[ProtoField.default = Some]
I -->|repeated / map / oneof| K[ProtoField.default = None]
J --> L[field_line in stringify.rs]
K --> L
L --> M{default present?}
M -->|string or bytes type| N[proto-escape value]
M -->|enum / bool / numeric| O[bare token]
M -->|None| P[no options suffix]
%%{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[JS bundle module] -->|parse AST| B[DefaultsCollector]
A -->|parse AST| C[ContentsCollector / internalSpec]
B -->|Vec of target_alias → field→token| D[attach to Ident.defaults]
C -->|Vec of MemberDesc| E[build_entity]
D --> E
E -->|for each MemberDesc| F{member type?}
F -->|OneOf| G[build_field with empty defaults map]
F -->|Field| H[build_field with Ident.defaults]
H --> I{field qualifies?}
I -->|message_member=true, not repeated, not map| J[ProtoField.default = Some]
I -->|repeated / map / oneof| K[ProtoField.default = None]
J --> L[field_line in stringify.rs]
K --> L
L --> M{default present?}
M -->|string or bytes type| N[proto-escape value]
M -->|enum / bool / numeric| O[bare token]
M -->|None| P[no options suffix]
Reviews (4): Last reviewed commit: "fix(proto): escape control chars in stri..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-proto/src/extract.rs (1)
649-686: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude map fields from proto2 defaults.
build_fieldonly filters out oneof members andrepeatedfields, so amap<K, V>with a matchinginternalDefaultsentry would still emit an invalid[default = ...]. Mirror the existingTYPE_MAPguard here.🤖 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-proto/src/extract.rs` around lines 649 - 686, Exclude map fields from proto2 defaults in build_field: the current default selection only skips oneof members and repeated fields, so map<K, V> fields can still pick up an invalid internalDefaults value. Update the default computation in build_field to also guard against TYPE_MAP, matching the existing optional-flag logic, and keep defaults only for singular non-map message members.
🤖 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-proto/src/stringify.rs`:
- Around line 86-112: The default formatting in field_line only quotes string
defaults, but bytes defaults coming from extract.rs can also be StringLiteral
and must be emitted as quoted literals. Update the default-handling branch in
field_line so it treats both string and bytes as quoted cases, while leaving
other field types unchanged.
---
Outside diff comments:
In `@crates/wa-proto/src/extract.rs`:
- Around line 649-686: Exclude map fields from proto2 defaults in build_field:
the current default selection only skips oneof members and repeated fields, so
map<K, V> fields can still pick up an invalid internalDefaults value. Update the
default computation in build_field to also guard against TYPE_MAP, matching the
existing optional-flag logic, and keep defaults only for singular non-map
message members.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6508db64-800d-4a00-a5db-2cb29ff91d1c
⛔ Files ignored due to path filters (2)
generated/manifest.jsonis excluded by!**/generated/**generated/proto/WAProto.protois excluded by!**/generated/**
📒 Files selected for processing (4)
Cargo.tomlcrates/wa-ir/src/proto.rscrates/wa-proto/src/extract.rscrates/wa-proto/src/stringify.rs
CI's stable toolchain advanced to Rust 1.96, whose clippy adds two lints
that fail `-D warnings` on pre-existing code (unrelated to this PR's proto
work, but they block any PR until fixed):
- wa-codegen `decode_hex`: `len() % 2 != 0` -> `!len().is_multiple_of(2)`
(is_multiple_of is stable since 1.87, within the 1.88 MSRV).
- wa-scan `resolve_child_node`: collapse `if a { if let b {} }` into a
single `if a && let b {}` let-chain (1.88, matching the bumped MSRV).
Both are semantically identical — no output change.
Also address two Greptile nitpicks on the proto defaults:
- stringify: quote a `bytes` default too (proto2 bytes defaults are string
literals). No WA field has one today, so output is unchanged, but a
future one stays valid instead of emitting an unquoted `[default = x]`.
- extract test: assert the `!0` -> `true` branch (only `!1` -> `false`
was covered).
Verified clean under the stable 1.96 toolchain: clippy -D warnings, fmt,
and the full workspace test suite.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
`build_field` guarded oneof members and `repeated` fields but not `map<K,V>` — protoc rejects a default on a map, so a stray `internalDefaults` entry on a map field would emit an invalid `[default = …]`. Mirror the existing `TYPE_MAP` guard. No WA map field has a default today (output unchanged); the guard keeps a future one valid. Test asserts a map field stays default-free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
![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-proto/src/stringify.rs,field_linecurrently insertsStringLiteraldefaults without proto-escaping, so defaults containing quotes or backslashes can generate invalid.prototext and break downstream parsing/codegen; escape default string values during stringify and add a regression test for quoted/backslashed defaults before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
A proto2 `string`/`bytes` default was inserted between quotes raw, so a value containing `"` or `\` would produce invalid `.proto` (terminating the string early). Escape backslashes then quotes before formatting. No WA field has a string/bytes default today (output unchanged), but the emitted literal stays valid if one appears. Test covers a backslash + quote default. 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.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The string/bytes default escape path only handled `\` and `"`. A default containing a newline or carriage return would split the emitted field declaration across lines and produce invalid `.proto`. Escape the named control chars (`\n`/`\r`/`\t`) and fall back to a 3-digit octal escape for any other control char; printable ASCII/Unicode passes through unchanged. No WA field currently has a string/bytes default, so generated output is unchanged — this keeps a future one valid. Adds a control-char regression test. 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.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The control-char octal fallback used `is_control()`, which is also true for C1 controls (U+0080–U+009F). Proto's octal escape encodes a single raw byte, so escaping a C1 control's code point (e.g. `\200` for U+0080) emits one byte instead of that char's two UTF-8 bytes — invalid UTF-8 for a `string` default (protoc rejects it) and a silent value change for `bytes`. Restrict the octal path to ASCII controls (`is_ascii_control()`, i.e. 0x00– 0x1F and 0x7F); non-ASCII (printable or C1 control) passes through as its raw UTF-8, which protoc accepts. Extends the regression test with a C1 control. Still no WA field has a string/bytes default, so generated output is unchanged. 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: Changes involve proto2 field defaults recovery, MSRV bump, refactors with let-chain syntax, and serialization output modifications. These touch codegen, IR schema, extraction logic, and stringification — non-trivial changes that benefit from human review despite no detected issues.
Re-trigger cubic
jlucaso1
pushed a commit
that referenced
this pull request
Jul 6, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent, self-contained improvements from the post-#24 backlog.
1. Recover proto2 field defaults (
feat(proto))The proto extractor skipped WA's
internalDefaults, so proto2[default = X]was dropped from every field that declares one — a proto2 consumer (e.g. whatsapp-rust) then reads the type-zero value instead of WA's declared default on an absent field.Parse
X.internalDefaults = {field: <expr>}alongsideinternalSpecand attach the resolved default to its field, handling every value shape WA uses:s.E2EE/c.Foo.E2EE→ the bare variant name)-1)!1/!0Adds a
ProtoField.defaultIR field (omitted when serialized, so it's additive) and emits[default = X]— astringdefault quoted, enum/bool/numeric bare — combined with[packed = true]via a joined options list. Defaults apply only to singular message fields, never oneof members or repeated fields (protoc rejects those).Recovers 27 defaults across ~18 messages:
accountType=E2EE,trigger=UNKNOWN,taskId=-1,messageVersion=1,status=PENDING,format=DEFAULT,carouselCardType=HSCROLL_CARDS, … The protox compile guard (added in #24) confirms every default resolves against its field's enum type; output is deterministic and the change is purely additive (onlyproto/WAProto.proto+ its manifest hash change).2. Declare the real MSRV (
chore)rust-version = "1.85"was aspirational — the crates use let-chains (if let … && let …), stabilized in Rust 1.88, so 1.85 can't build them. Someone on 1.85 got a confusing mid-compile error instead of cargo's clear "requires rustc 1.88". Bumped to the honest floor. Metadata-only; no artifact or behavior change.Validation
Full CI-equivalent gate green locally:
fmt,clippy --workspace --all-targets --all-features -D warnings,cargo test --workspace(incl. new extractor + stringify default tests),wa-ir --features schema, wa-ir/wa-fetch wasm32,whatspec --no-default-features, C-free,validate-schemas.py, and the byte-identical determinism--check. The protox guard compiles the emittedWAProto.protowith all 27 new defaults resolving cleanly.Note: a third candidate (incoming message content-type catalog) was scoped out — it turned out to need a whole new
WASmaxParseUtilsparser-DSL extractor (a proper future domain), not a gap-fill.🤖 Generated with Claude Code
https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Generated by Claude Code
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Summary by cubic
Recover proto2 field defaults in
WAProto.protoby parsinginternalDefaults, and bump MSRV to Rust 1.88 for let-chains. Restores 27 defaults so absent fields use intended values; improves default literal escaping (incl. safe control-char handling that preserves UTF‑8) and keeps output deterministic.New Features
X.internalDefaults = { field: <expr> }and emit[default = …]on singular fields only (notoneof,repeated, ormap); supports enum variants, numbers (incl. negatives), booleans!1/!0, and strings viaProtoField.defaultwith joined options.accountType=E2EE,messageVersion=1,status=PENDING,taskId=-1.Bug Fixes
bytesdefaults and escapestring/bytesliterals:\and"; named escapes for\n,\r,\t; octal-escape only ASCII controls (others pass through as UTF‑8) to keep emitted.protovalid and avoid UTF‑8 corruption.map<K,V>fields from defaults; add tests (incl.!0→true, ASCII control and C1 control cases); clippy clean in@wa-codegenand@wa-scanwith let-chains, no behavior change.Written for commit fd031c7. Summary will update on new commits.