ir: check the IR is usable, recover the enums WA composes, and pin the bootloader map - #37
Conversation
The JSON Schemas validate shape: that a field has the keys it should, of the right types. They cannot say whether the document is USABLE — whether a field the IR calls an enum names its legal values, whether a byte pin is the length it claims, whether a union carries the alternatives that make it one. Those are the questions a consumer writing a generator has to answer, and nothing checked them. This replaces the "compile the generated Rust in CI" follow-up. That guard was aimed at an artifact that is on its way out — the committed contract is the IR and its schemas, and a consuming library implements the generator in its own language. A guard on the Rust output would have been work spent on the part that is being removed; this one is language-neutral and sits on what survives. Two kinds of finding. An internal contradiction (a `literalValue` on an integer field that is not an integer, a `byteMin` above its `byteMax`, an `enumRef` with no variants) always fails: the document is saying two things at once. A counted state (an enum WA composes at runtime and the extractor cannot name) is held to a baseline, so it may shrink but a rise fails — that is a constraint starting to slip through unnoticed. Verified by sabotage rather than by reading: injecting one of each contradiction into a copy of the artifact produces five errors plus one baseline regression and exit 1, and the real artifact exits 0. One thing the writing of it taught: keying "is this a field?" on `method`/ `wireName` looked right and silently skipped appstate, which carries neither — the linter reported a clean count while missing a domain. It keys on the type vocabulary now.
`babelHelpers.extends({}, VISIBILITY, {error: "error"})` is how WA builds an enum out of
another one — every privacy setting has a `…_WITH_ERROR` twin carrying the sentinel the
parser actually checks for. The resolver recognised object literals and `$InternalEnum`
calls and nothing else, so those exports resolved to nothing and every field validating
against one shipped as `"type": "enum"` with no values. It was the largest single entry
in `dropsByReason`.
The merge is left-to-right with later keys overriding, as the runtime does, and each
operand must be something already understood: a literal, an `$InternalEnum`, or a local
this pass has already bound. Anything else refuses the WHOLE composition — a partial
merge is a closed value set missing members the runtime accepts, which is worse than
recording the loss.
iq dropsByReason 41 -> 33
fieldEnumRefs 151 -> 167
enum fields with no values (lint) 173 -> 157
VISIBILITY_WITH_ERROR now reads `all, contacts, contact_blacklist, none, error`, and the
three other privacy compositions resolve alongside it.
The empty `{}` first operand needed its own case: `parse_enum` rejects it for having no
variants, so it fell through to the refusal path and killed every merge before it started.
The first version of this shipped with that bug and recovered nothing — the counters said
so, which is why the linter's counted states are worth having.
… boundary
The three findings left open at the end of the review, plus the thing that made them
worth leaving open: nothing said what the action reader models.
* An accessor passed through an ALIAS. The minifier hoists nearly every read into a
local, so `var x = attrString("jid"); h(x)` handed the helper the text `x` — no wire
read in it, and meaningless inside the helper's own parse. Arguments are resolved
through the caller's scope before the binding decides, so the alias form works.
* A mapped-child callback passed BY NAME. `mapChildrenWithTag("participant", parseP)` is
a function too; rejecting the identifier before looking at its body emitted the child
with an empty field list, which tells a consumer the element carries nothing.
* A collection only one branch carries. `NotifActionChild` gains `required`, weakened
when a branch producing the same action lacks it — the same all-branches accounting
`required` already does for a scalar, one level up. Serialized only when false, so the
common case adds nothing to the document.
And a `# What this models, and what it does not` section on the module. Reading these
arms means interpreting minified JavaScript, and this file has grown a small
scope-and-control-flow analysis to do it; six review rounds were spent discovering its
edges one construct at a time. Writing the boundary down is what turns the next such
finding from a surprise into a decision — and it records the invariant that matters if
this grows: a reader that guesses is worse than one that reports a gap.
576 tests; each of the three reverted alone to watch its test fail.
The named-callback path shipped broken on the first attempt: the `(…)` that makes a
declaration parse as an expression also hides it behind a `ParenthesizedExpression`, so
the function gate rejected it and the child came back empty. Second time that wrapper has
cost a round.
The JS never carries a wasm URL — it asks the bootloader for a numeric `bx` id, and only the page's resource map turns that id into something fetchable. Nothing recorded that map, so a change in the bootloader's shape surfaced only as a wasm count that quietly stopped growing. `wasmResources` cannot cover it either: it counts every handle, most of which address theme images that come and go with UI work, so it is deliberately unguarded. `wasm.lock.json` gains the extraction: the resolved id→URI map, how many handles the page inlined before any request, and how many endpoint requests were sent and failed. A run that suddenly needs more rounds, or starts failing, is the early signal that the endpoint contract moved — and the map itself can be diffed across releases. What is pinned is the EXTRACTION, not the page. The HTML carries nonces and timestamps and is byte-unstable by construction; hashing it would fail the determinism gate for reasons that have nothing to do with the protocol. The key is absent, not empty, when a run never asked the bootloader anything — a local `--bundles` regen must not be readable as "the map is empty now", and the lock is written only by the authoritative fetch path in the first place. 576 tests, determinism check clean, IR consistent.
📝 WalkthroughWalkthroughThis PR updates notification requiredness inference, persists bootloader pins in version-aware wasm locks, improves composed enum resolution across JavaScript scopes, and adds deterministic generated-IR invariant validation to CI. ChangesNotification action extraction
Bootloader pin persistence
Composed enum resolution
Generated IR validation
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant WasmResolution
participant load_wasm
participant update
participant WasmLock
WasmResolution->>load_wasm: return payloads, observed ids, and diagnostics
load_wasm->>update: provide bootloader pins and live handle ids
update->>WasmLock: merge same-version pins and write lock metadata
Possibly related PRs
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 |
![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 (2)
crates/whatspec/src/main.rs (1)
352-358: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPersist pins when extraction yields no payloads.
load_wasmreturnsSome(pins)for an empty payload set (Line 1310), but this payload-only guard skipsWasmLock::with_bootloader. A blocked or changed bootloader therefore leaves no pinned request/failure/map outcome—exactly the diagnostic state this PR introduces. Preserve the existing payload safety policy while updating compatible lock metadata, or write an explicit empty extraction record; do not silently discardboot_pins.🤖 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/whatspec/src/main.rs` around lines 352 - 358, The authoritative WASM lock update currently runs only when `wasm` is non-empty, silently discarding `boot_pins` for empty extractions. Update the flow around `WasmLock::with_bootloader` so compatible lock metadata is persisted when `load_wasm` returns pins with no payloads, while preserving the existing payload safety policy; alternatively write the explicit empty extraction record rather than dropping the pins.crates/wa-notif/src/actions.rs (1)
786-810: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve optionality when merging an already-folded child.
Line 797 merges same-tag children but ignores
c.required. Iffromalready represents branches where the child is absent,existing.requiredcan incorrectly remaintrue. Combine both states withexisting.required &= c.required.Proposed fix
Some(existing) if existing.wire_tag == c.wire_tag => { + existing.required &= c.required; let dead = self.dead_child_fields.entry(c.name.clone()).or_default(); merge_fields(&mut existing.fields, c.fields, false, dead); }🤖 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-notif/src/actions.rs` around lines 786 - 810, Update the same-tag child merge arm in the surrounding merge function so it combines requiredness before merging fields: set existing.required to the conjunction of its current value and c.required. Keep the existing merge_fields and dead_child_fields handling unchanged.
🤖 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 `@scripts/lint-ir.py`:
- Around line 137-138: Update the field traversal around check_field so
field-shaped objects are identified independently of membership in FIELD_TYPES.
Ensure every such object reaches validation and its type is explicitly reported
as an error when not in FIELD_TYPES, preserving normal validation for recognized
field types.
---
Outside diff comments:
In `@crates/wa-notif/src/actions.rs`:
- Around line 786-810: Update the same-tag child merge arm in the surrounding
merge function so it combines requiredness before merging fields: set
existing.required to the conjunction of its current value and c.required. Keep
the existing merge_fields and dead_child_fields handling unchanged.
In `@crates/whatspec/src/main.rs`:
- Around line 352-358: The authoritative WASM lock update currently runs only
when `wasm` is non-empty, silently discarding `boot_pins` for empty extractions.
Update the flow around `WasmLock::with_bootloader` so compatible lock metadata
is persisted when `load_wasm` returns pins with no payloads, while preserving
the existing payload safety policy; alternatively write the explicit empty
extraction record rather than dropping the pins.
🪄 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: 8c092588-3931-4d62-a656-f8162ea521b6
⛔ Files ignored due to path filters (3)
generated/iq/index.jsonis excluded by!**/generated/**generated/manifest.jsonis excluded by!**/generated/**generated/schema/notif.schema.jsonis excluded by!**/generated/**
📒 Files selected for processing (11)
.github/workflows/ci.ymlcrates/wa-codegen/src/notif_export.rscrates/wa-enums/src/lib.rscrates/wa-ir/src/lib.rscrates/wa-ir/src/notif.rscrates/wa-notif/src/actions.rscrates/wa-notif/src/tests.rscrates/whatspec/src/lock.rscrates/whatspec/src/main.rscrates/whatspec/src/restore.rsscripts/lint-ir.py
…ld child optionality Three findings on the follow-ups themselves. The linter's docstring said an unrecognized `type` was worth reporting and the code used the vocabulary as its DETECTOR, so a typo'd or newly-added type silently excused the field from every other check — the doc promising what the code did not do, which is the class this whole guard exists to catch. A field is now anything with a known type OR anything carrying a key only a response field has, and the second clause reports the type. The marker only counts when the object has a `type` at all: assertions carry `referencePath` too, and the first version of this flagged four of them as untyped fields. `load_wasm` returns pins even when it resolved no payloads — a blocked or changed bootloader is exactly the state the pins were added to record — but the lock write is guarded on a non-empty payload set, so that state was the one case where they were dropped. An empty run now rewrites only the bootloader section, leaving the recorded payload set alone, which is what the emptiness guard is actually protecting. And merging a same-tag child ignored the incoming `required`. `from` may itself be a fold of several branches that already knows the child is absent from one, so keeping `existing.required` re-asserted a presence the incoming side had disproved. 577 tests, determinism clean, IR consistent. The unknown-type check verified by renaming one `type` in a copy of the artifact: one error, exit 1.
|
Os dois achados fora do diff também estão aplicados em
Agora uma execução sem payloads reescreve só a seção do bootloader, deixando intacto o conjunto de payloads gravado — que é o que a guarda de vazio de fato protege. Lock ausente ou ilegível fica como está: não há conjunto a preservar, e inventar um a partir de uma execução vazia é o que a guarda proíbe.
577 testes, determinismo em dia, IR consistente, clippy |
![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: 861094c61a
ℹ️ 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".
… pin defects Seven findings on the follow-ups. Two others in the same review were already fixed in d8eee2d, which the review predates. The linter accepted a DECREASE silently, which banks the difference as slack: 157 → 150 followed by seven newly unresolved enums is back at 157 and passes — precisely the drift the counted states exist to catch. It is a ratchet now; an improvement is real work and updating the number with it costs one line. Two invariants the linter claimed and did not check, both because they were gated on the `ParsedFieldType` vocabulary. Request-side and stanza attributes carry `enumRef` keyed by `kind`, so the pending marker — an empty `variants`, which tells a consumer the attribute admits NO value — could ship on those surfaces; and request content states the same byte pin as `constBytes` that a response states as `literalValue`, so neither malformed hex nor a length mismatch was checked there. Both are checked on every dictionary now. `babelHelpers.extends(base, {B: "b"})` MUTATES `base`. Recovering only the result would publish a `BASE` export missing a member the runtime has — an enum claiming to reject a value it accepts, which is the worst thing this can produce. WA's convention is an empty target, so requiring one costs nothing and rules the mutation out. Three on the bootloader pins: * they were built from this run's sample while the payload set accumulates from the cache, so a later successful run could DELETE ids an earlier one resolved while their payloads stayed — the supposedly diffable map differing between two runs of one release. Built from the accumulated handle map now. * `failures` also carries pre-request diagnostics (no endpoint parameters, no deferred components), so a `failedRequests` counted from its length contradicted its own name. `WasmResolution` counts request failures separately. * `NotifActionChild::required` skipped its `true` case, leaving a non-Rust consumer — who the IR is for — unable to tell "required by default" from "unspecified". Always serialized, and the schema now carries `default: true`. 577 tests, 12/12 schemas, determinism clean, IR consistent.
…ves behind `update_lock_bootloader` is `#[cfg(feature = "fetch")]` and its call site was not, so `cargo build -p whatspec --no-default-features` — a job this repo runs precisely because the tool must build without the network stack — stopped compiling. `cargo test --workspace` never exercises that configuration, and I never ran it. The `BTreeMap` import moves behind the same gate for the same reason: only the fetch path builds the pin map from it. Checked against the whole CI matrix this time rather than the subset I had been running: fmt, clippy (default and `--all-features`), the workspace tests, the no-default-features build, `wa-fetch` and `wa-ir` on wasm32, `wa-ir --features schema`, both artifact scripts, and the determinism gate. The two remaining no-fetch warnings are pre-existing on main, which now emits three of them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb3de9f35b
ℹ️ 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".
…enum name Six findings from the Codex round on bb3de9f. Five are latent — they change no committed artifact today — and one changes the IR linter's count. - `BootloaderPins::wasm_handles` promised wasm and delivered the page's whole `bxData`: images and every other resource, which churn far more often than the wasm set and so swamped the diff signal the field exists for. Filtered with the resolver's own `is_wasm_url` rather than a second copy of the predicate. - An empty `--wasm` run replaced the bootloader section wholesale, deleting ids earlier runs had resolved from the endpoint while keeping the payloads they resolved. The update workflow passes no `--cache`, so the lock is the only record on that path. The map now accumulates; the diagnostics still describe the current run. - A mapped child behind a guard was always `required: true`, though `strip_guard` reaches through the guard to find it. Deriving that from `is_guarded` alone would be wrong in the other direction — the one live case, `t.hasChildren() ? t.mapChildrenWithTag(…) : [{wid: …}]`, always yields a collection — so only a literal absence on the far side makes it optional. - `NamedResolver::locals` is flat while JavaScript is lexically scoped, so a name rebound inside a nested function could feed a composition the wrong body and publish a closed set naming members the runtime never accepts. No bundle in the pinned set has a single collision; both readers now refuse the name rather than guess. - The linter counted appstate's two `protoEnum` fields as enums with no values. They name their values, just by reference — and with the baseline a ratchet, a new proto-backed enum could have cancelled out a real regression. Baseline 157 -> 155. - `literalValue` and `referencePath` are documented as mutually exclusive but were only ever checked apart. No live case, which is the point of adding it now. Every fix has a test that fails without it, and the two that could plausibly have been over-corrected are pinned from both sides.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04fbd20314
ℹ️ 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".
…num links Two findings from the Codex round on 04fbd20. - On a version bump that resolved no payloads, the previous commit wrote this run's handles into a lock still stamped for the older release — and since that map now accumulates, it interleaved two releases' ids in a record whose whole purpose is being diffable. `warn_if_wasm_lock_is_now_stale` already detected the mismatch and only warned. The updater now takes the version and leaves a lock belonging to another one alone, saying so. - WAM's 1431 enum-typed fields name a module and expect the consumer to find it in the document's own `enums` catalog, but the linter only ever inspected inline `enumRef` objects. A dangling module — or a definition with no variants, which the schema permits — read as internally consistent while a consumer could not encode the field. All 1431 resolve today; the check is keyed on the document having a catalog rather than on the name "wam", so a second domain adopting the shape is covered without anyone remembering. Both have a test that fails without the fix; the linter's three failure modes (dangling, empty, ambiguous) are each verified against a mutated document.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b199dd2407
ℹ️ 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".
The previous commit fixed the shrink only where a run resolved nothing. A run that resolves a smaller endpoint subset also succeeds, and rebuilding the lock from its own sample deletes ids an earlier run resolved — the same defect, reached by a productive run rather than a blocked one. Without `--cache`, which is how the update workflow runs, the lock is the only record, so nothing else restores them. Both paths now share one rule, version-scoped as before: `merge_prior_handles` for the in-place edit, `pins_with_prior` for the paths that build a whole new lock. The ids are provenance and never enter `wasmSetHash`, so keeping one whose payload this run did not re-resolve costs nothing, while dropping it makes an unchanged release diff against itself. `pins_with_prior` is deliberately not feature-gated: its caller is on the ordinary write path and compiles in every configuration.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1fb4c8792
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-notif/src/actions.rs`:
- Around line 1984-1993: Update guard_admits_absence to unwrap
ParenthesizedExpression nodes before testing optionality, then apply the
existing conditional and logical-expression checks to the unwrapped expression.
Preserve the current ternary and && behavior, and add coverage for parenthesized
ternary and && guards.
In `@crates/whatspec/src/main.rs`:
- Around line 2189-2213: Update pins_with_prior to distinguish an absent prior
lock from a present lock that cannot be read or parsed: log the read or
deserialization failure using the existing logging approach, while preserving
the current same-wa_version filtering and no-prior-lock behavior. Keep the
function’s return contract and merge_prior_handles flow unchanged for valid
locks.
🪄 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: ec70f97b-2838-4618-b675-3bdd54b30c56
⛔ Files ignored due to path filters (3)
generated/manifest.jsonis excluded by!**/generated/**generated/notif/index.jsonis excluded by!**/generated/**generated/schema/notif.schema.jsonis excluded by!**/generated/**
📒 Files selected for processing (9)
crates/wa-enums/src/lib.rscrates/wa-fetch/src/bootloader.rscrates/wa-fetch/src/discover.rscrates/wa-fetch/src/lib.rscrates/wa-ir/src/notif.rscrates/wa-notif/src/actions.rscrates/wa-notif/src/tests.rscrates/whatspec/src/main.rsscripts/lint-ir.py
Six from Codex on d1fb4c8 plus two from CodeRabbit. Every one is latent — no committed artifact changes. Guards, continuing the mapped-child fix: - `guard_admits_absence` matched on the expression directly, so a parenthesized guard answered "no guard here". `strip_guard` unwraps the same wrapper, and the two have to agree on what they are looking at; that wrapper has cost this file a field twice before. - A guard around a value-position HELPER was lost entirely: the inliner is handed `strip_guard(value)`, so the map inside looks unguarded. The guard belongs to the key, not the helper, and is now applied to what the helper yields. Scope: - Only `var`-form rebindings reached `shadowed`; a bare `e = {…}` never did, so a module reassigning an operand before composing it still published the stale body — the same wrong closed set through the one write form the guard did not watch. Rebound to something unreadable refuses the name too. - A named mapped-callback was resolved straight from the module's function table, ignoring the caller's bindings. A parameter of the same name shadows it at runtime, so an unrelated function's fields could be published as the element's. Linter: - Half a numeric range (`intMin` with no `intMax`) passed, because the check required both keys before looking at either. - `generated/enums/` is a catalog nothing references, so validating references alone left all 326 definitions unexamined. - `kind: "const"` with no `value` tells an emitter the value is fixed and then declines to say what it is. Only this direction: `kind` is shared with the assertion vocabulary, where `attr`/`content` carry a `value` legitimately, so the converse check would have flagged 1781 correct nodes. - `pins_with_prior` treated an unparseable lock as an absent one, silently dropping the handles it was accumulating to preserve.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 031a442a67
ℹ️ 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".
…t an empty catalog hides Four from Codex on 031a442. One is a crash; the rest are latent. - The callback-alias resolution added last round could not terminate. `deref_ident` caps at four hops, so on a cycle whose length does not divide four (`a → b → c → a`) it hands back a *different* identifier every call, and recursing on that walks the cycle until the stack goes. Reverting the guard makes the new test abort with a stack overflow, so this is a real crash on a shape the extractor could meet, not a theoretical one. It now recurses only into something that is no longer an identifier; an unresolved chain is refused, which is what the hop limit was for. - Function parameters bind their name for the whole body, which neither the declarator nor the assignment path saw, so `function f(e){ extends({}, e, …) }` still composed over the outer `e`. Refusing such names module-wide — the cheap version — was measured against the pinned bundles and LOST a real constraint (`STANZA_MSG_TYPES`), so this tracks a scope stack instead and refuses only inside the body that binds the name. `generated/` is unchanged either way, which is the point: the blunt version was not. - `check_enum_catalog_refs` returned early on an EMPTY catalog, which is the worst case rather than a trivial one: if extraction collapsed the list while the enum-typed fields remained, all 1431 references are dangling and the document was still reported consistent. An empty list is an empty lookup. - A WAM event's `code` is its wire identifier and nothing checked it was unique. A consumer building a dispatch table by code would silently overwrite one of a colliding pair, leaving one event shape unreachable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57a96933f6
ℹ️ 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".
…ten three checks Six from Codex on 57a9693. One is a real cross-release bug; the rest are latent. - `BundleCache::wasm_handles` read its manifest with no version check while `wasm_payloads` refuses a stale one through `usable_manifest`. A cache left from the previous release could therefore feed its ids into this release's pins — the same cross-version mixing the lock path already refuses, reached by the one door left open. It now takes the version, like everything else that reads that manifest for a purpose. Two holes in last round's parameter-scope fix, both found by pushing on it: - An unreadable redeclaration (`var e = getEnum()` in a nested scope) shadows just as much as a readable one, and left the outer body in `locals` for a later composition to publish. Refused only when the name is already bound: an unparseable `var` that shadows nothing costs nothing to ignore. - A deferred export alias (`function f(e){ i.OUT = e }`) is resolved AFTER the walk, when the parameter stack is already unwound, so the scope has to be recorded at the assignment rather than looked up later. Linter: - A union with two entries of the SAME name is not a two-alternative union: the name is the discriminator, and the Rust codegen emits it as the variant identifier. - `valueKind` is what tells a consumer how to represent an enum's values, and the schema permits any `Scalar` per variant, so nothing checked the two agreed. - A `content` assertion is the fixed text a marker variant matches on; without the value a consumer cannot tell when the variant applies. Kept narrow — an `attr` assertion may legitimately assert only presence. `generated/` is unchanged by all six.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fd371605e
ℹ️ 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".
…er pattern Four from Codex on 7fd3716. One drops a field today; the rest are latent. - Nested inlining re-parses a SYNTHETIC buffer, but `local_call_source` kept slicing the module source with spans belonging to it. The bounds check never fired — those offsets are in range — so the argument text was simply unrelated, and a value threaded through two helpers (`mk2(v){ return mk(v) }`) lost its field. `ArmCtx` now carries the source its spans index, and `inline_local` rebinds it to the buffer it parsed. `locals` became a borrow so the context is cheap to re-point; there is still exactly one construction of it. - `param_names` saw only plainly-named parameters, so `function f({e})` and a rest element bound nothing as far as the shadow guard was concerned. Uses `get_binding_identifiers` — the same traversal `wa_scan::shadow_params` already uses for its own shadowing — rather than a second hand-rolled walk to drift from. - A WAM field's `id` is its wire identifier within its event, and only the event-level `code` was checked. Scoped per event: ids repeat across events by design. - `fields`, `constantFields` and `children` are three representations of one object-key namespace. Each is internally consistent by construction and nothing compared them, so an action could carry a wire-read `reason` and a constant `reason` at once — a shape runtime cannot produce.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cfe9e56ea
ℹ️ 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".
A header expression is evaluated before the construct's own binding exists, so it resolves where the construct is written. The switch discriminant and the `for-in` /`for-of` right-hand side were being visited under the pushed lexical scope, which could only refuse a resolvable composition — safe, but the point of modelling scopes is not to guess in either direction. A `for` init is left inside, being the binding itself. Shipped without a regression test, deliberately. The one written for it passed with the fix REMOVED: reaching the header path needs the composition to be the header expression and its result to be exported, and the resolver's supported export forms make that awkward to express. A test verified to prove nothing is worse than none, so this says so instead.
jlucaso1
left a comment
jlucaso1
left a comment
There was a problem hiding this comment.
Sobre os dois fora do diff:
Asserção de regressão para metadado de enum (tests.rs 636-638): válido, mas não aplicado nesta rodada pelo mesmo motivo que acabei de registrar no thread do lib.rs — acabei de descobrir que um teste que escrevi para o fix de header passava com o fix removido. Antes de adicionar mais asserções nessa área quero verificar cada uma contra o revert, e não tenho margem para fazer isso com cuidado agora. Fica anotado.
Ruff apontando complexidade em check_appstate_collections: procede — a função acumulou seis checagens distintas (collections, scopes, index literal, chatJidIndex, valueEnumFields, valueProtoType/valueField). Quebrá-la em funções por invariante é a limpeza certa, e é exatamente o tipo de refactor que prefiro fazer com o diff isolado em vez de no fim de uma sequência longa de correções sutis.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee948bc627
ℹ️ 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".
`mapped_child` calls `strip_guard`, which always follows the consequent, so `disabled ? 0 : node.mapChildrenWithTag(…)` — the same shape written the other way round — dropped the child ENTIRELY. The absence check beside it already inspects both branches, so the two disagreed about which shapes exist. Reversing an equivalent ternary must not erase the collection. Notification types and stanza tags become generated Rust identifiers through the same `pascal_case` as collections, scopes and union alternatives, so two wire spellings that normalise alike emit one variant twice. Not applied: resolving each `valueEnumFields` KEY against its payload message's fields. The pair check landed last round for `valueField`/`valueProtoType`; extending it to the enum keys means walking nested field paths inside an arbitrary message, which the brace-depth scanner does not do and which I would rather build with its own measurement than bolt on. Recorded on the thread.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1987c5ae12
ℹ️ 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".
Restricting exports to the factory's parameters compared SPELLING only, so a
nested `function f(i) { i.OUT = {…} }` — writing to its own parameter — was still
read as a module export and could publish a private object as a closed protocol
enum.
Frame 0 of the scope stack is the module factory itself, whose parameters ARE the
receivers, so a shadowing is any frame after it that carries the name. Checking
`param_shadows` directly would have refused every real export.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab74276317
ℹ️ 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".
…nds call Three from Codex on ab74276, each a gap in the fix that preceded it. - The alternate-branch search matched on the expression directly, so `(cond ? 0 : map(…))` answered "not a conditional". Third time this wrapper has cost this file something; it is peeled up front now. - The receiver-shadow check reads the scope stack, whose frames are filtered to names that collide with an ENUM local — and a receiver name is not an enum, so `function f(){ var i = {}; i.OUT = … }` walked straight through. The filter now keeps a name that collides with either. - `extends(e, {B:"b"})` mutates `e` wherever it appears, and only a declarator's initializer was watched; a bare expression statement does it too.
There was a problem hiding this comment.
💡 Codex Review
whatspec/crates/wa-notif/src/actions.rs
Lines 1123 to 1127 in 7c1d92c
Treat continue as an early do-while path
When a do…while body conditionally continues before a write, such as var x = attr("jid"); do { if (c) continue; x = attr("lid") } while (false); return {id:x}, the return can observe either source. This certainty check calls can_escape, which detects break but not continue, so it takes the definite-write branch and publishes only lid; treat an unlabelled continue that can skip the remaining body as an early path here and tombstone the write.
ℹ️ 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".
…cting export Three from Codex on 7c1d92c, each a place the preceding fix did not reach. - The two-way receiver filter (enum local OR factory parameter) went into the function prescan and not the arrow one. - `tmp = extends(e, {B:"b"})` is an expression statement wrapping an ASSIGNMENT, so reading the statement's expression found the assignment rather than the call that mutates. - An export written twice with different bodies published whichever write came first. Which one a consumer sees is not something this pass can determine, so it now determines nothing; an identical repeat is still resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/wa-notif/src/actions.rs (1)
1942-1987: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlternate-branch fallback is skipped whenever the consequent is any other call.
.filter(...)runs only on the final result ofas_call(strip_guard(unwrapped)).or_else(...). If the consequent resolves to some call that isn'tmapChildrenWithTag(e.g.cond ? otherCall() : t.mapChildrenWithTag(...)),as_callreturnsSome(other_call), so.or_elsenever fires (it only triggers onNone), and.filterthen discards the wrong call without ever looking at the alternate. The map in the alternate is silently dropped in exactly this shape. The currently tested cases (reversed_,parenRev) only exercise a non-call consequent (0), whereas_callalready returnsNone, soor_elsehappens to fire correctly — this masks the gap.🐛 Proposed fix: filter each candidate before falling back
- let call = as_call(strip_guard(unwrapped)) - .or_else(|| match unwrapped { - Expression::ConditionalExpression(c) => as_call(strip_guard(&c.alternate)), - _ => None, - }) - .filter(|c| callee_method(c) == Some(wap::MAP_CHILDREN_WITH_TAG))?; + let call = as_call(strip_guard(unwrapped)) + .filter(|c| callee_method(c) == Some(wap::MAP_CHILDREN_WITH_TAG)) + .or_else(|| match unwrapped { + Expression::ConditionalExpression(c) => as_call(strip_guard(&c.alternate)) + .filter(|c| callee_method(c) == Some(wap::MAP_CHILDREN_WITH_TAG)), + _ => None, + })?;Existing
reversed_/parenRevbehavior is preserved by this change (consequent literal still yieldsNonefromas_call, so the fallback still fires); it only adds correctness for the case where the consequent is a different call.🤖 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-notif/src/actions.rs` around lines 1942 - 1987, Update mapped_child’s call-selection logic so each branch candidate is validated against MAP_CHILDREN_WITH_TAG before falling back to the alternate branch. Ensure a non-matching consequent call does not prevent examining a matching alternate call, while preserving the existing behavior for literal or parenthesized consequents.crates/wa-enums/src/lib.rs (1)
779-788: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark non-declaration
for-in/for-ofleft targets as shadowed too.Both loop helpers only pass
VariableDeclarationdeclarators intowith_lexical; the_ => vec![]fallback also skips assignment-target forms (for (e in obj),for ({e,i} in obj),for ([e] of arr), etc.) and never marks colliding locals as shadowed. Extensional collection also ignores these forms. Like bare/destructured reassignment, these should be treated as rebinds that shadow enclosing locals before visiting the expression and body.🤖 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-enums/src/lib.rs` around lines 779 - 788, Update visit_for_in_statement and its for-of counterpart to collect binding names from non-declaration assignment targets, including identifiers, object patterns, and array patterns, instead of treating them as empty. Pass those names to with_lexical so they shadow enclosing locals, while preserving RHS visitation before entering the loop scope and collecting the same names for extension handling.
🤖 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-notif/src/tests.rs`:
- Around line 980-987: Add an assertion for parenRev.required alongside the
existing parenRev.wire_tag assertion in the child-selection test, verifying that
the parenthesized reversed conditional produces the expected absent-path
requiredness, consistent with the reversed_ case.
In `@scripts/lint-ir.py`:
- Line 994: Reduce the complexity of check_appstate_collections by extracting
cohesive validations—such as collection-name collisions,
valueProtoType/valueField agreement, valueEnumFields, and chatJidIndex—into
focused helper functions, then have the main function delegate to them while
preserving its existing validation behavior and error reporting.
---
Outside diff comments:
In `@crates/wa-enums/src/lib.rs`:
- Around line 779-788: Update visit_for_in_statement and its for-of counterpart
to collect binding names from non-declaration assignment targets, including
identifiers, object patterns, and array patterns, instead of treating them as
empty. Pass those names to with_lexical so they shadow enclosing locals, while
preserving RHS visitation before entering the loop scope and collecting the same
names for extension handling.
In `@crates/wa-notif/src/actions.rs`:
- Around line 1942-1987: Update mapped_child’s call-selection logic so each
branch candidate is validated against MAP_CHILDREN_WITH_TAG before falling back
to the alternate branch. Ensure a non-matching consequent call does not prevent
examining a matching alternate call, while preserving the existing behavior for
literal or parenthesized consequents.
🪄 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: 4d3df521-a93a-4a73-bf38-d8a10a0f457d
📒 Files selected for processing (4)
crates/wa-enums/src/lib.rscrates/wa-notif/src/actions.rscrates/wa-notif/src/tests.rsscripts/lint-ir.py
`.filter()` ran on the COMBINED result, so `or_else` fired only when the consequent was not a call at all. `cond ? otherCall() : map(…)` therefore discarded the wrong call and never looked at the alternate — and both fixtures used `0` as the consequent, where `as_call` already returns `None`, so the tests could not see the gap they were written for. Each candidate is filtered on its own now, with a call-consequent case added. `parenRev` asserted only its `wire_tag`, which would have passed with the collection wrongly marked always-present. It asserts the absence too.
jlucaso1
left a comment
jlucaso1
left a comment
There was a problem hiding this comment.
Os dois achados fora do diff estão aplicados em 322fd44.
actions.rs 1942-1987 — fallback pulado quando o consequente é outra chamada. Correto, e a parte que mais importa é a que você notou sobre as fixtures: minhas duas usavam 0 como consequente, onde as_call já devolve None, então o or_else disparava por acidente e mascarava exatamente a lacuna que o fix pretendia fechar. Passei a filtrar cada candidato antes do fallback e adicionei cond ? otherCall() : map(…); sem o fix, no child callRev.
tests.rs 987 — parenRev sem asserção de required. Aplicado. Asserção só do wire_tag passaria com a coleção marcada como sempre presente.
Segunda vez nesta rodada que meu próprio teste escondia o defeito. Agradeço a leitura das fixtures e não só do diff — foi o que pegou os dois.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 322fd44f2c
ℹ️ 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".
…semantics Three from Codex on 322fd44. The branch search looked one level down, so `a ? x : (b ? y : map(…))` found the inner conditional rather than the map inside it. `find_mapped_call` now recurses through both sides; each step descends into a strictly smaller node. Two linter checks, both of which had to be built around what the data actually holds rather than the obvious rule: - The accessor must agree with the declared type — but as a SET per accessor, not one type: `attrInt` legitimately backs `integer`, `timestamp` and `timestamp_millis`, and a one-to-one map would have failed CI on 27 correct fields. - A `byteLength` belongs to an accessor that has one — checked only where a `method` is present, because 20 request-side `constBytes` pins carry the length with no accessor at all. That is the third and fourth time a guard here would have rejected valid data if written to the obvious rule; measuring the committed IR first is what caught both.
There was a problem hiding this comment.
💡 Codex Review
whatspec/crates/wa-notif/src/actions.rs
Lines 2059 to 2060 in 4e7d5c5
Compare enum tables when merging conditional reads
When both branches read the same wire attribute with the same enum accessor but validate it against different tables, this equality check treats them as the same read and publishes only x.enum_arg. For example, cond ? child.attrEnum("type", TABLE_A) : child.attrEnum("type", TABLE_B) can accept different values by branch while the IR advertises only TABLE_A as the closed constraint. Include the resolved enum table in branch compatibility, refusing or safely merging the field when the tables disagree.
ℹ️ 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".
…both branches Three from Codex on 4e7d5c5. - A mutating `extends` was detected at the statement and declarator level only, so one nested in a sequence, a logical operand or a condition test was missed. Detected in a call visitor now, which the generic walk takes to every position. - For a LEXICAL `for-in`/`for-of` head the bound name is already in its temporal dead zone while the right-hand side runs, so a read there throws rather than reaching an outer enum. Last round moved the RHS out of the scope for both kinds; that is right for `var` and wrong for `let`/`const`, and the two are now distinguished. - `cond ? map("p", …) : map("q", …)` returned whichever branch was reached first, publishing one wire tag for a shape that produces two. Equal tags still resolve; differing ones refuse. The conditional is reconciled BEFORE the `strip_guard` shortcut, which follows the consequent and would otherwise answer with it.
… them `privacyParser` reads `<privacy><category name="last" value=…/>…</privacy>`, where the legal `value` enum depends on `name` and each category produces a differently named output (`lastSeen`, `callAdd`, `readReceipts`, …). The IR publishes ten sibling `value` fields at the response root: the nesting, the discriminator and the output names are all gone, so a consumer sees one key with ten contradictory constraints. This predates the PR — `main` has the identical shape and `generated/iq` is not among the files changed here. What is mine is the handling: the duplicate-key check added in this PR found these 15, and I switched it off after reading them as "alternatives the extractor models on purpose". Reading the source showed that was wrong, and a linter whose stated purpose is "diagnostics over silence" should not have certified them in the meantime. Held to a baseline, like `dropsByReason` and the unresolved enums: the loss is visible, cannot grow unnoticed, and CI does not fail on data this PR cannot fix. Repairing the shape is an extractor change and belongs in its own PR.
Counting ARRAYS with a repeat meant an eleventh `value` in an array already holding ten changed nothing — the same count-vs-identity blind spot the unresolved-enum baseline had twice before. Counted per extra entry now, and the baseline is the measured 42 rather than the 24 I first guessed. Both a new repeat and an extra one inside an existing array trip it.
There was a problem hiding this comment.
💡 Codex Review
whatspec/crates/wa-fetch/src/cache.rs
Line 327 in 7c837fa
Preserve cached wasm files until the manifest commits
When store_wasm updates an existing cache, write_set rewrites every reused payload at the same URL-derived path rather than alongside the old files. If an interrupt, full disk, or later write failure occurs after one of those files is truncated but before the new manifest is committed, the old manifest remains while its previously valid payload now fails size or checksum validation; because endpoint results are subsets, that payload may not be recoverable on the next run. Write reused paths atomically through temporary files, or avoid rewriting already verified entries, before committing the manifest.
ℹ️ 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".
… shapes Four from Codex on 7c837fa. The flattening baseline I added two commits ago was an AGGREGATE, so one known loss disappearing while a new one appeared elsewhere kept the total at 42 and passed. That is the third time this exact blind spot has been found here and the second in a baseline I wrote — after citing the lesson in the commit that introduced it. Keyed by semantic path and repeated names now; a substitution reports both halves. - A deferred alias that disagrees with an inline write to the same export (`i.OUT = {A:"a"}; i.OUT = e`) kept the first body through `or_insert`, while the runtime exports the second. Same refusal the immediate path already makes. - Two branches mapping the SAME tag with different callbacks published one of them, asserting `jid` for a branch that reads `lid`. Equal tag is not equal shape; the arguments must match too. - A `*Source` is provenance for an inferred value: alone it says where a constraint came from without saying what to enforce. Both pairs are validated together, and an empty source string is rejected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e332de1a2c
ℹ️ 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".
| vf = a.get("valueField") | ||
| if isinstance(vf, str) and isinstance(vpt, str): |
There was a problem hiding this comment.
Fresh evidence after the payload pair cross-check is that it runs only when both values are strings: setting an existing action's valueField to null while retaining its valid valueProtoType still leaves lint-ir.py successful, and the inverse is likewise accepted. The generated metadata then describes a protobuf payload without the SyncActionValue field where it must be placed, so require valueField and valueProtoType to be either both present or both absent before validating their correspondence.
Useful? React with 👍 / 👎.
| for (key, value) in wa_oxc::obj_props(o) { | ||
| if let Some(data) = enum_object(value).and_then(parse_enum) { | ||
| self.exports.entry(key.to_string()).or_insert(data); |
There was a problem hiding this comment.
Fresh evidence beyond the direct-property conflict fix is that inline members of X.exports = { ... } still use unconditional or_insert. For example, i.OUT = {A:"a"}; i.exports = {OUT:{B:"b"}} (or two OUT keys within export bags) keeps {A} here even though JavaScript's later write exports {B}; mark this as conflicting using the same comparison as the single-property path rather than publishing the stale closed enum.
Useful? React with 👍 / 👎.
| let same_text = | ||
| |x: &oxc_ast::ast::CallExpression, y: &oxc_ast::ast::CallExpression| { | ||
| x.span == y.span | ||
| || format!("{:?}", x.arguments) == format!("{:?}", y.arguments) |
There was a problem hiding this comment.
Fresh evidence after reconciling callbacks is that the debug representation of Oxc arguments includes each node's source span, so two textually identical calls in different ternary branches never compare equal here (and their call spans cannot be equal either). Thus cond ? node.mapChildrenWithTag("p", parseP) : node.mapChildrenWithTag("p", parseP) is refused and its child metadata disappears despite both branches having the same shape; compare AST content without locations, such as through Oxc's content equality, instead.
Useful? React with 👍 / 👎.
| if ( | ||
| t == "enum" | ||
| and not f.get("enumRef") | ||
| and not f.get("enumKeys") | ||
| and not (f.get("protoEnum") and f["protoEnum"] in proto_enums) |
There was a problem hiding this comment.
When a normal string field such as method: "attrString", type: "string" is given a valid enumRef, every current check succeeds: the accessor agrees with the string type and the reference itself has variants. I applied that mutation to notif/index.json and the linter still certified all 12 documents, while notif_export publishes field_type: String together with a nonempty enum_values table, giving consumers contradictory answers about whether the value is constrained. Whenever a node has a type, reject enumRef (and the equivalent enumKeys/protoEnum metadata) unless that type is enum, while preserving request attributes that legitimately carry an enum reference through their kind vocabulary.
Useful? React with 👍 / 👎.
| check_tokens(data, domain, errors) | ||
| check_notif_identifiers(data, domain, errors) |
There was a problem hiding this comment.
When a resource's wasmHint contains an unknown string such as definitelyWrong, the schema accepts it and this linter still certifies all 12 documents. The IR contract defines exactly wasmBinaryLiteral, moduleName, and wasmModuleCacheDep, while the manifest counts any nonempty hint list toward wasmWasmHandles; consequently a typo is treated as recovered WebAssembly evidence even though consumers matching the documented values cannot interpret it. Add a WASM-domain check that rejects every hint outside the published vocabulary.
Useful? React with 👍 / 👎.
1 participant
The follow-ups the constraints PR (#36) left on the record, minus the two that stopped making sense.
What each one does
IR consistency in CI (
scripts/lint-ir.py). The JSON Schemas validate shape: a field has the keys it should, of the right types. They cannot say whether the document is usable — whether a field the IR calls an enum names its legal values, whether a byte pin is the length it claims, whether a union carries the alternatives that make it one. Those are the questions a consumer writing a generator has to answer, and nothing checked them.Two kinds of finding. An internal contradiction always fails: a
literalValueon an integer field that is not an integer, abyteMinabove itsbyteMax, anenumRefwith no variants. A counted state — an enum WA composes at runtime and the extractor cannot name — is held to a baseline, so it may shrink but a rise fails, because that is a constraint starting to slip through unnoticed.Verified by sabotage, not by reading: injecting one of each contradiction into a copy of the artifact produces five errors plus a baseline regression and exit 1; the real artifact exits 0.
Composed enums (
babelHelpers.extends({}, VISIBILITY, {error: "error"})). This is how WA builds an enum out of another one — every privacy setting has a…_WITH_ERRORtwin carrying the sentinel the parser checks for. The resolver knew object literals and$InternalEnumcalls and nothing else, so those exports resolved to nothing and every field validating against one shipped as"type": "enum"with no values. It was the largest single entry indropsByReason.iq.dropsByReason(enum)fieldEnumRefsVISIBILITY_WITH_ERRORnow readsall, contacts, contact_blacklist, none, error.The three action findings left open, plus the reason they were left open. An accessor passed through an alias (
var x = attrString("jid"); h(x)— the minifier hoists nearly every read into a local, so the helper was handed the textx); a mapped-child callback passed by name (mapChildrenWithTag("participant", parseP)emitted the child with an empty field list); and a collection only one branch carries (NotifActionChildgainsrequired, weakened when a branch producing the same action lacks it — the same all-branches accountingrequiredalready does for a scalar, one level up).And a
# What this models, and what it does notsection onactions.rs. Reading those arms means interpreting minified JavaScript, and that file has grown a small scope-and-control-flow analysis to do it; six review rounds were spent discovering its edges one construct at a time. Writing the boundary down turns the next such finding from a surprise into a decision, and records the invariant that matters if it grows: a reader that guesses is worse than one that reports a gap.The bootloader extraction, pinned. The JS never carries a wasm URL — it asks for a numeric
bxid, and only the page's resource map turns that into something fetchable. Nothing recorded that map, so a change in the bootloader surfaced only as a wasm count that quietly stopped growing.wasmResourcescannot cover it: it counts every handle, most addressing theme images that come and go, so it is deliberately unguarded.wasm.lock.jsonnow carries the resolved id→URI map, the handles the page inlined, and the requests sent and failed.Pinned is the extraction, not the page: the HTML carries nonces and timestamps and is byte-unstable by construction, so hashing it would fail the determinism gate for reasons unrelated to the protocol.
Two follow-ups deliberately dropped
Compile the generated Rust in CI, and enforce the constraints in the reference codegen. Both were aimed at an artifact on its way out: the committed contract is the IR and its schemas, and a consuming library implements the generator in its own language. Work spent hardening the Rust output would have been spent on the part being removed. The IR consistency check above is the language-neutral replacement for the first; the second has no replacement because, once the codegen goes, it has no subject.
The mex
docIdhistory was built and then dropped as out of scope for this change.Validation
cargo fmt --all --check;clippy --workspace --all-targetsand--all-features, both-D warningscargo test --workspace— 582 tests, 11 added herescripts/validate-schemas.py generated→ 12/12;scripts/lint-ir.py generated→ consistentupdate --bundles <pinned> --check→27 committed artifact(s) up to dateSummary by cubic
Adds a CI linter to enforce IR usability, resolves WA‑composed enums, pins bootloader handle→URL mappings, and improves notification action extraction. Also ratchets duplicate‑key flattening per extra entry and keys it by semantic path so substitutions are visible without breaking CI.
New Features
attrIntmay backinteger,timestamp,timestamp_millis).byteLengthonly on accessors that have one; allow request‑sideconstBytesto carry lengths with no accessor.Bug Fixes
extends(...)in any call position; refuse conflicting export writes and mismatched deferred aliases vs inline bodies.let/constinfor-in/for-ofheaders when resolving reads.*Sourceto be paired with its value and non‑empty; report standalone sources.Written for commit e332de1. Summary will update on new commits.
Summary by CodeRabbit
requiredflag describing whether repeated-collection data is present on every producing branch.wasm.lock.jsoncan optionally store bootloader pin metadata and preserve/update it during--wasmruns.extendswith safer merging and stricter shadowing/scope handling.