fix: P0 output-correctness & coverage-recovery batch (content, byte bounds, parser linking) - #22
Conversation
The IQ request-builder emitter never read `child.content`, so every leaf node that carries a byte payload built empty — making the 20 key-material request builders (prekey upload, retry receipts, link-code pairing) structurally valid but unusable on the wire. Even the fully-static `link_code_pairing_nonce` (const `00`) emitted an empty node. Emit content for both shapes: - a compile-time constant (`const_bytes`, e.g. the one-byte `00` nonce) as a literal `.bytes(vec![0x00])`; - a caller-supplied buffer (a bare `bytes` content) as a `Vec<u8>` spec field threaded in as `.bytes(self.<tag>_content.clone())`, so the prekey upload builder now takes its key material as constructor input. Field names derive from the collision-free node var (`id_node` -> `id_content`, `id_node_2` -> `id_content_2`) so repeated leaf tags in a `<skey>` tree each get a distinct field. Generated reference Rust only (gitignored); the committed IR is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
The content-length index recorded the FIRST module seen to pin a (parentTag, tag, length) as the byte-length provenance. Scan order follows bundle file order, so when two modules pin the same wire field the recorded `byteLengthSource` depended on input ordering — a live violation of the byte-identical-output guarantee (regenerating from a differently-ordered bundle set flipped six `byteLengthSource` values between WAWebDigestKeyJob and WAWebRetryRequestParser). Break the tie by lexicographically-smallest module name instead of first-seen, so the provenance is stable across any bundle ordering. Regenerated generated/iq/index.json accordingly (skey/value, skey/signature and their rotate/ variants now pin the deterministic WAWebDigestKeyJob). This also makes the committed artifacts reproducible from the cached bundles, which the subsequent IR/coverage changes rely on for clean regeneration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
`content_byte_length` returned a length only for the fixed `contentBytesRange( node, min, max)` case (min == max) and silently dropped every true range — so 22 ranged sites in the bundle (a media buffer capped at 1 MiB, a token capped at 128 bytes, padding 0..524288, …) lost their wire bounds entirely. The IR carried intMin/intMax for integer ranges but nothing for byte ranges, so a consumer could not learn a payload was size-capped. Add byteMin/byteMax to ParsedField (mirroring intMin/intMax) and thread a byte_range through the smax response classifier: min == max still pins a fixed byteLength, min != max now records the (min, max) limit. The two are mutually exclusive, so no field carries both. Regenerated: byteMin/byteMax now populate 108 ranged byte fields across generated/iq/index.json (+ generated/srvreq/index.json, same response model), purely additive; the shared ParsedField schema gains the two properties in the iq/stanza/incoming/notif/srvreq schemas. Verified every emitted bound against a real contentBytesRange site in the bundle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
…velope) `parsed_response_from_new_expr` required the parser name to be an inline string literal, so a `new WADeprecatedWapParser(d, fn)` whose name is hoisted into a variable was dropped. `WAWebHandleMexNotification` does exactly this (`d = "mexNotificationParser", m = new WADeprecatedWapParser(d, fn)`), so the `type="mex"` notification carried no typed content at all. Resolve the name from the module's `var name = "literal"` binding when the constructor arg is an identifier, requiring a unique binding so an ambiguous short name is left unnamed rather than mislabeled. Regenerated: the mex notification now carries its typed envelope (assertions type=mex, `<update>` op_name/content, id, from, offline); notif degraded 9 -> 8, typedContent 18 -> 19. Purely additive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
…nd site
A legacy job that builds an `<iq>` and hands a sibling module's parser to
`deprecatedSendIq(iq, o("Mod").parser)` — rather than constructing a
`WADeprecatedWapParser` inline — lost its typed response entirely, since the
per-module scanner only sees inline parsers. `WAWebQueryMediaConnsJob` does
exactly this, so the `w:m` media-connection query (`<media_conn>` hosts / auth
token / TTL) was captured with `parserName: "unknown"`.
Add a send-site parser index: find each `deprecatedSendIq(_, o("Mod").parser)`
call, resolve the target module's parser (only when it defines a single one, so
an untracked export name is never guessed), and attach it to any request left
with the `unknown` fallback. Resolution is keyed by the sending module, so it
recovers the same-module case; a send site in a different module from the one
that built the `<iq>` (e.g. the gdpr query, threaded across a boundary) is left
untouched.
Regenerated: WAWebQueryMediaConnsJob gains its mediaConnParser response;
iq degradedResponses 19 -> 18, typedResponses 140 -> 141.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
notif Phase 2 attached typed content only from a handler's inline `WADeprecatedWapParser`. Several server-initiated notification types are parsed via the smax receive-RPC path instead, so their field tree lives in a `WASmaxIn<X>...Request` module (already extracted into the srvreq domain) and the notification was left degraded (`content` absent). Add a Phase 2b fallback: for any type still degraded, match the srvreq notification read-shape whose parser asserts that `type` and attach it. Only types with a single matching shape are recovered, so a type parsed by several distinct request modules (hosted, link_code_companion_reg) stays degraded rather than binding to an arbitrary shape. Regenerated: crsc_continuation, passkey_prologue_request and waffle gain their typed content; notif degraded 8 -> 5, typedContent 19 -> 22. Purely additive. 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 adds byte-payload emission for request node builders, introduces byte-range constraints ( ChangesRequest Node Byte Payload Emission
Byte-Range Constraint Modeling
Srvreq-Based Notification Content Recovery
Deterministic Content-Length Provenance
Send-Site Parser Resolution and IQ Scan Integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ScanPipeline as scan_iq_with_diagnostics
participant SendParserIndex as send_parser_index::build_pass
participant ResponseResolver as resolve_parser_name
participant Stanzas as IQ Stanzas
ScanPipeline->>SendParserIndex: build send-site parser map
SendParserIndex->>ResponseResolver: resolve legacy parser name (literal or variable)
ResponseResolver-->>SendParserIndex: resolved parser name or None
SendParserIndex-->>ScanPipeline: module -> ParsedResponse map
ScanPipeline->>Stanzas: replace "unknown" response/parser_name with resolved entry
Suggested reviewers: 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-codegen/src/emit.rs | Adds emit_node_content and decode_hex; uses rfind("_node") (addressing prior reviewer concern); four new unit tests cover constant bytes, dynamic bytes, terminal-segment renaming, and malformed hex degradation. |
| crates/wa-ir/src/iq.rs | Adds byte_min/byte_max optional fields to ParsedField with serde(default, skip_serializing_if), mirroring the existing int_min/int_max pattern; purely additive, no breaking schema changes. |
| crates/wa-scan/src/response_smax.rs | Refactors content_byte_length to return (Option<u32>, Option<(u32,u32)>) and threads byte_range through all Binding::Field construction sites; all non-range arms correctly set byte_range: None; updated test extends coverage to range and optional cases. |
| crates/wa-scan/src/response.rs | Adds resolve_parser_name and StringBindingFinder; the module-wide (non-scope-aware) binding search is intentionally conservative — ambiguous names are rejected rather than guessed, with three unit tests covering recovery, ambiguous, and reuse cases. |
| crates/wa-scan/src/send_parser_index.rs | New module: resolves o("Mod").parser references at deprecatedSendIq call sites; provenance-checks the callee against WADeprecatedSendIq; only attaches when the target module has exactly one parser; four unit tests cover the happy path, bare-identifier exclusion, missing reference, and ambiguous target. |
| crates/wa-scan/src/lib.rs | Wires the send-parser index into the IQ scan; explicitly counts unknown stanzas per module and only attaches when exactly one unknown exists per module, addressing the prior reviewer concern. |
| crates/wa-notif/src/lib.rs | Adds Phase 2b fallback recovering degraded notification content from srvreq read-shapes; short-circuits when no degraded notifications exist; only attaches when a single unambiguous shape asserts the matching type. |
| crates/wa-scan/src/content_length_index.rs | Fixes tie-breaking to use and_modify + lexicographic minimum instead of first-wins; new unit test asserts that swapping bundle order produces identical provenance output. |
Reviews (5): Last reviewed commit: "fix(scan): match only the member-form de..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/emit.rs`:
- Around line 774-786: The decode_hex helper in emit.rs can panic on non-ASCII
input because it slices with byte indexes before validating UTF-8 boundaries.
Update decode_hex to avoid direct string slicing in the loop and instead iterate
over bytes or validated hex pairs so malformed input always returns None. Keep
the behavior aligned with the doc comment by making malformed const_bytes in
WapContent degrade gracefully rather than triggering a panic during code
generation.
- Around line 726-772: The emit_node_content path incorrectly falls through when
decode_hex fails, causing malformed const_bytes to be treated as caller-supplied
Bytes content and adding an unwanted Vec<u8> field. Update emit_node_content to
keep the const_bytes case isolated: if content.const_bytes is present but
decode_hex returns nothing, return early with no emitted content instead of
entering the WapContentKind::Bytes branch. Make sure the Bytes branch only runs
for true non-const byte content, and use the existing emit_node_content and
WapContentKind::Bytes logic to locate the fix.
In `@crates/wa-notif/src/lib.rs`:
- Around line 126-141: The notification backfill logic in the
`dispatch.notifications` loop uses `&& let`, which is not compatible with the
workspace’s stated minimum Rust version. Rewrite the conditional in the `Phase
2b` block inside `crates/wa-notif/src/lib.rs` using MSRV-safe syntax, or
alternatively bump the crate/workspace `rust-version` if that is the intended
support policy. Keep the behavior of `srvreq_notification_shapes` and the
`n.content` assignment unchanged.
In `@crates/wa-scan/src/response.rs`:
- Around line 64-119: The binding lookup in resolve_string_binding is too broad
because it scans the entire module, so a reused name elsewhere can mislabel the
parser or make it ambiguous. Restrict lookup to the enclosing scope of
resolve_parser_name / the new WADeprecatedWapParser(...) call, or use semantic
scope resolution to bind the identifier to the correct declaration. Update
StringBindingFinder accordingly so it only considers the parser’s actual scope,
and add a regression test where an unrelated inner function reuses the same
variable name.
🪄 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: 2c5df8ea-0752-4f22-bcab-b250c4edf097
⛔ Files ignored due to path filters (9)
generated/iq/index.jsonis excluded by!**/generated/**generated/manifest.jsonis excluded by!**/generated/**generated/notif/index.jsonis excluded by!**/generated/**generated/schema/incoming.schema.jsonis excluded by!**/generated/**generated/schema/iq.schema.jsonis excluded by!**/generated/**generated/schema/notif.schema.jsonis excluded by!**/generated/**generated/schema/srvreq.schema.jsonis excluded by!**/generated/**generated/schema/stanza.schema.jsonis excluded by!**/generated/**generated/srvreq/index.jsonis excluded by!**/generated/**
📒 Files selected for processing (9)
crates/wa-codegen/src/emit.rscrates/wa-ir/src/iq.rscrates/wa-notif/src/lib.rscrates/wa-notif/src/tests.rscrates/wa-scan/src/content_length_index.rscrates/wa-scan/src/lib.rscrates/wa-scan/src/response.rscrates/wa-scan/src/response_smax.rscrates/wa-scan/src/send_parser_index.rs
Robustness hardening surfaced in review; no change to generated output. - codegen: `decode_hex` now rejects non-ASCII input, so a malformed `const_bytes` can't panic on a non-char-boundary slice (it degrades to no content, as documented). - codegen: isolate the `const_bytes` case in `emit_node_content` — a constant that fails to decode returns early instead of falling through to the caller-supplied `Vec<u8>` branch (which would have turned a fixed constant into a builder field). - codegen: rename only the terminal `_node` segment of the content var (rfind, not replacen) so a tag whose name contains `_node` isn't corrupted. - scan: only attach a send-site parser when the module has a single `unknown` stanza, so a module building two parserless IQs never binds both to the same sibling parser. Tests added: malformed-const emits nothing, `node_id` tag field naming, and a reused-variable-name parser that stays unnamed rather than mislabeled. 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 and verified against the latest diff
Confidence score: 3/5
- In
crates/wa-scan/src/send_parser_index.rs,callee_method(call)only handling member calls means baredeprecatedSendIq(iq, o("Mod").parser)sites are skipped, so legacy response types stayunknown; merging as-is risks incomplete parser indexing and weaker downstream type safety/coverage for those send paths—add identifier-callee matching before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
`SendFinder` only matched the member form `o("…").deprecatedSendIq(…)`, so a
bare-identifier `deprecatedSendIq(iq, o("Mod").parser)` call (the export hoisted
into a local of the same name) would be skipped and its typed response left
`unknown`. Match an identifier callee named `deprecatedSendIq` too.
The current bundle uses only the member form (50 sites, 0 bare), so generated
output is unchanged; this is forward-looking robustness. Covered by a new test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/wa-scan/src/send_parser_index.rs">
<violation number="1" location="crates/wa-scan/src/send_parser_index.rs:96">
P2: Modules with a local or shadowed helper named `deprecatedSendIq` can now get an unrelated typed response attached, because the bare-identifier path matches by name without checking the binding came from `WADeprecatedSendIq`. Consider tracking the local alias assignment/import before accepting bare calls, or keep the previous member-only match when the binding is unknown.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Address follow-up review feedback:
- send_parser_index: the bare-identifier send-site match keyed purely on the
name `deprecatedSendIq`, so a module-local helper of the same name could
attach an unrelated response. Track locals bound from
`o("WADeprecatedSendIq").deprecatedSendIq` and accept a bare call only when its
callee is such an alias; the member form now verifies the same provenance
inline (require("WADeprecatedSendIq")). New test covers an unrelated local
helper being ignored. Generated output unchanged (real sites use the member
form).
- response.rs: `variable_name_reused_in_another_function_is_not_mislabeled`
asserted only the absence of the wrong label, which passed vacuously once the
parser was dropped; assert the result is empty too.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Drop the bare-identifier send-site path. Resolving a bare `<local>(iq, parser)`
call soundly requires per-lexical-scope alias tracking (an alias learned in one
function must not match a shadowed name in another), which is real machinery for
a form that does not occur in the bundle — every one of the 50 send sites uses
the inline member form `o("WADeprecatedSendIq").deprecatedSendIq(...)`.
Keep the member form only, with its inline provenance check (the callee's object
must `require("WADeprecatedSendIq")`), which is sound and covers 100% of real
sites. A test documents that a bare call is deliberately left unresolved.
Generated output unchanged (WAWebQueryMediaConnsJob still recovers its
mediaConnParser response via the member form).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Core logic changes to IR schema, codegen, parser linking, and notification recovery; high risk despite being P0 fixes.
Re-trigger cubic
Addresses the five P0 findings from the multi-dimension analysis: the two output-correctness bugs (the IR/reference misrepresenting the wire) and the three already-extractable-but-unlinked typing recoveries. Each is a self-contained commit; the committed
generated/artifacts are regenerated per change and every emitted value was cross-checked against the raw bundle.Enabling fix (prerequisite)
fix(scan): deterministicbyteLengthSource— the content-length index recorded the first module seen to pin a(parentTag, tag, length), so provenance depended on bundle read order — a live violation of the byte-identical-output guarantee (regenerating flipped six values betweenWAWebDigestKeyJob/WAWebRetryRequestParser). Now tie-broken by lexicographically-smallest module name. This makes the committed artifacts reproducible from the cached bundles, which the coverage changes below rely on for clean regeneration. Guarded by a new order-independence unit test.P0 · Output-correctness bugs
fix(codegen): emitWapChildNode.contentin IQ request builders — the emitter never readchild.content, so every leaf carrying a byte payload built empty, making the 20 key-material builders (prekey upload, retry receipts, link-code pairing) structurally valid but unusable on the wire. Now a compile-time constant (const_bytes, e.g. the00nonce) emits.bytes(vec[0x00]), and a caller-supplied buffer threads aVec<u8>spec field as.bytes(self.<tag>_content.clone()). Verified the emitted.bytes(...)matches the realwacore_binary::builder::NodeBuilderAPI. Generated reference Rust only (gitignored); the committed IR is unchanged.feat(iq): preserve rangedcontentBytesRangebounds asbyteMin/byteMax— a true range (min != max, e.g. a media buffer1..1048576, a token1..128) was silently dropped; only the fixedmin == maxcase survived asbyteLength. The IR carriedintMin/intMaxfor integers but nothing for byte ranges. AddedbyteMin/byteMaxtoParsedField(mirroring the int bounds) and threaded abyte_rangethrough the smax classifier. Regenerated: 108 ranged byte fields acrossiq/srvreqnow carry bounds — every one cross-checked against a realcontentBytesRangesite in the bundle. Purely additive.P0 · Coverage recovery (already-extracted, unlinked typing)
fix(scan): recover variable-namedWADeprecatedWapParser— a parser whose name is hoisted into a variable (d = "mexNotificationParser"; new WADeprecatedWapParser(d, fn), asWAWebHandleMexNotificationdoes) was dropped for lack of an inline string name, so thetype="mex"notification carried no typed content. Now resolves the name from the module'svar name = "literal"binding (requiring a unique binding). notif degraded 9→8, typedContent 18→19.feat(iq): link legacy responses from sibling parser modules at the send site — a job that hands a sibling module's parser todeprecatedSendIq(iq, o("Mod").parser)rather than constructing one inline lost its typed response. A new send-site index resolves the referenced parser (only when the target defines a single one, so an untracked export name is never guessed) and attaches it to any request leftunknown, keyed by the sending module. RecoversWAWebQueryMediaConnsJob'sw:mmedia-connection response; iq degradedResponses 19→18, typedResponses 140→141. The cross-module gdpr case (send site in a different module from the<iq>builder) is left for a future change.feat(notif): recover degraded content from srvreq read-shapes — several server-initiated notification types are parsed via the smax receive-RPC path, so their field tree lives in aWASmaxIn<X>...Requestmodule (the srvreq domain) rather than an inline handler parser. Added a Phase 2b fallback matching the srvreq notification read-shape whose parser asserts thattype, keeping only types with a single unambiguous shape (hosted/link_code_companion_reghave two shapes each and stay degraded). Recoverscrsc_continuation,passkey_prologue_request,waffle; notif degraded 8→5, typedContent 19→22.Net effect
Verification
Full CI-equivalent suite green locally:
fmt --check,clippy --workspace --all-targetsand--all-features(both-D warnings),cargo test --workspace, schema self-tests,wa-ir/wa-fetchWASM checks,whatspec --no-default-featuresbuild, C-free tree check, and Python schema validation (11 domains). New unit tests cover each change. Determinism re-verified: regeneratinggenerated/from the cached bundles is byte-identical to the committed output.generated/changes are purely additive except the six intended deterministicbyteLengthSourcevalues and the one recovered iq response replacing itsunknownplaceholder.🤖 Generated with Claude Code
https://claude.ai/code/session_013111jePsSca7epxMugRn2A
Generated by Claude Code
Summary by cubic
Fixes P0 wire correctness by emitting byte content in IQ builders and preserving byte range limits, and recovers missing typed responses/notifications. Makes
byteLengthSourcedeterministic for reproducible builds. Net impact: iq typedResponses +1 (140→141), iq degradedResponses −1 (19→18); notif typedContent +4 (18→22), notif degraded −4 (9→5).Bug Fixes
.bytes(...).byteLengthSourceprovenance is order-independent (ties broken by smallest module name) to ensure byte-identical regeneration.contentBytesRangeis preserved asbyteMin/byteMaxin IR and schemas (additive; no breaking changes).const_bytesdecoding; no fallthrough to builder fields on decode failure; only rename the terminal_nodein content var names; attach a send-site parser only when a module has a singleunknownstanza; match only the member-formo("WADeprecatedSendIq").deprecatedSendIq(...)send sites withrequire("WADeprecatedSendIq")provenance (bare calls are intentionally not matched). No output changes.Coverage
WADeprecatedWapParserinstances, fixing thetype="mex"notification envelope.WAWebQueryMediaConnsJob).crsc_continuation,passkey_prologue_request,waffle).Written for commit 63aa321. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes