fix(wasm,scripts): three claims #3182 made that are not true, and the gate that would have caught one (#3191)

itself is correct and unchanged here; what did not land is the review, and
three of its findings were claims the code does not support.

1. The changeset says `IfcDoorStyle` and `IfcWindowStyle` were affected and
   implies this change fixes them. It does not. Both carry `has_geometry: false`
   in `legacy_entities.rs`, and `styling/prepass.rs:177` plus its siblings in
   `gpu_meshes/prepass.rs`, `processing/shard_classes.rs` and
   `processing/processor/mod.rs` gate type-geometry candidates on a bare
   `IfcType::from_str(name).is_subtype_of(IfcTypeProduct)`, which is false for
   any keyword IFC4X3 dropped. Both are discarded before a job exists, so they
   never reach the corrected line. The changeset text ships as the changelog and
   #3186 has not published yet, so this is correctable in place. #3187 tracks
   the nine sites.

2. `batch.rs` justified the unvalidated `content.get(start..end)` with
   "`content[start..end]` is the record `decode_and_cache` just parsed". False on
   the cache-hit path: `decoder.rs:443` returns the cached `Arc` without reading
   `start`/`end`, so only the miss path bounds-checks them. Narrowed to the span
   the job carries, with the fail-soft behaviour stated.

3. A test docblock said the six #3172 entities were "deliberately absent"
   because #3172 "is still in review". #3172 merged; the sentence was true when
   written and false on merge.

The gate that would have caught the first one, derived rather than transcribed:
`check-legacy-entity-coverage.mjs` already extracts the legacy arm keys and the
generated `from_str` names from source and never intersected them. An arm whose
key `from_str` already resolves makes the `Unknown` short-circuit in
`legacy_aware_ifc_type_from_record` skip the remap, reopening exactly the
wasm-vs-native divergence #3179 was filed for. Now checked, with a mutation case
proving it fires.

Also from the review: a Rust test pinning the same invariant behaviourally (it
calls `from_str` for real, where the gate reads source text); a contract test for
`extract_entity_type_name` next to the function rather than only in its caller's
tests; one shared `LEGACY_KEYS` so the two opposite-direction tests cannot drift;
a dead `type_end <= type_start` guard removed (`paren_pos >= 1` always, the one
reachable equality is the empty slice `is_empty` already rejects); and three
dead or duplicated items in `test-wasm-contract.mjs`.

Every new test mutation-verified in both directions.

Author: louistrue

.changeset/wasm-legacy-keyword-labels.md

@@ -4,8 +4,10 @@
 
 Label legacy IFC keywords with their resolved type in the browser, not `"Unknown"`.
 
-The native pipeline resolves a legacy keyword through `legacy_entities.rs` and labels the node with its real base type. The browser path did not: the jobs wire carries only `(id, start, end)`, so `batch.rs` rebuilt the type from `entity.ifc_type` — the decoder's bare `IfcType::from_str` — and every keyword IFC4X3 dropped arrived as `Unknown` with the Unknown default colour. `IfcProxy`, the eight `*StandardCase` variants, both `*ElementedCase`, `IfcDoorStyle`, `IfcWindowStyle`, `IfcEquipmentElement` and the three IFC4X3 strata leaves were all affected. Type-exact visibility rules and styling consumers skipped them, and nothing threw.
+The native pipeline resolves a legacy keyword through `legacy_entities.rs` and labels the node with its real base type. The browser path did not: the jobs wire carries only `(id, start, end)`, so `batch.rs` rebuilt the type from `entity.ifc_type` — the decoder's bare `IfcType::from_str` — and a legacy keyword that reached that path arrived as `Unknown` with the Unknown default colour. The 22 `legacy_entities.rs` arms that carry geometry are fixed here, among them `IfcProxy`, the eight `*StandardCase` variants, both `*ElementedCase`, `IfcEquipmentElement`, the three IFC4X3 strata leaves and the six #3172 added. Type-exact visibility rules and styling consumers skipped them, and nothing threw.
+
+`IfcDoorStyle` and `IfcWindowStyle` are NOT fixed by this change. The pre-passes gate type-geometry candidates on a bare `IfcType::from_str(name).is_subtype_of(IfcTypeProduct)`, which is false for any keyword IFC4X3 dropped, so both are discarded before a geometry job exists and never reach the corrected line. That is the same defect one layer up; #3187 enumerates the sites.
 
 It cannot be recovered from the decoded value: `IfcType::Unknown` stores a CRC32 hash, not the name. It is recomputed from the record instead, which the batch already holds — a short scan to the first `(`, paid only by entities the decoder could not name.
 
-Fixing it surfaced a second defect. `extract_entity_type_name` did not trim, so `#71= IFCCOLUMN(` — legal STEP, and what buildingSMART's own `column-straight-rectangle-tessellation.ifc` writes on all 26 of its entity lines — yielded `" IFCCOLUMN"` with a leading space, matching no lookup. The function had no production caller, so its broken contract had never been exercised.
+Fixing it surfaced a second defect. `extract_entity_type_name` did not trim, so `#71= IFCCOLUMN(` — legal STEP, and what buildingSMART's own `column-straight-rectangle-tessellation.ifc` writes on all 26 of its entity lines — yielded `" IFCCOLUMN"` with a leading space, matching no lookup. The function had no production caller, so its broken contract had never been exercised. `extract_entity_type_name` is `pub` in `ifc-lite-core`, so that is a behaviour change on a published Rust surface: it now returns the trimmed name, and `None` rather than `Some(" ")` for a record with only whitespace between `=` and `(`.

rust/core/src/fast_parse.rs

@@ -270,10 +270,10 @@ pub fn extract_entity_type_name(bytes: &[u8]) -> Option<&str> {
     let type_start = eq_pos + 1;
     let type_end = eq_pos + paren_pos;
 
-    if type_end <= type_start {
-        return None;
-    }
-
+    // No `type_end <= type_start` guard: `bytes[eq_pos]` is `=`, never `(`, so
+    // `paren_pos >= 1` and `type_end >= type_start` always. The one reachable
+    // equality is `#1=(`, which yields an empty slice that the `is_empty` below
+    // rejects. `an_unreadable_record_changes_nothing` covers that input.
     let name = std::str::from_utf8(&bytes[type_start..type_end]).ok()?.trim();
     (!name.is_empty()).then_some(name)
 }

rust/core/src/fast_parse_tests.rs

@@ -161,3 +161,36 @@ fn test_should_use_fast_path() {
     assert!(!should_use_fast_path("IFCWALL"));
     assert!(!should_use_fast_path("IFCEXTRUDEDAREASOLID"));
 }
+
+/// `extract_entity_type_name`'s own contract, next to the function.
+///
+/// Its only production caller is `legacy_aware_ifc_type_from_record`, and the
+/// cases below were exercised only from that caller's tests until now. If the
+/// caller is ever deleted or rerouted, the contract keeps its coverage here.
+///
+/// The spaced forms are the reason this matters: STEP permits whitespace around
+/// `=`, and buildingSMART's own `column-straight-rectangle-tessellation.ifc`
+/// writes `#71= IFCCOLUMN(` on all 26 of its entity lines. Untrimmed, that
+/// yielded `" IFCCOLUMN"` and matched no lookup.
+#[test]
+fn extract_entity_type_name_trims_and_rejects_empty() {
+    for (record, expected) in [
+        (&b"#12=IFCCOLUMN('g');"[..], Some("IFCCOLUMN")),
+        (&b"#12= IFCCOLUMN('g');"[..], Some("IFCCOLUMN")),
+        (&b"#12=\tIFCCOLUMN('g');"[..], Some("IFCCOLUMN")),
+        // Nothing between `=` and `(`: an empty name is None, not Some("").
+        (&b"#12=();"[..], None),
+        (&b"#12= (  );"[..], None),
+        // No `=` and no `(` are both None rather than a panic.
+        (&b"IFCCOLUMN('g');"[..], None),
+        (&b"#12=IFCCOLUMN"[..], None),
+        (&b""[..], None),
+    ] {
+        assert_eq!(
+            extract_entity_type_name(record),
+            expected,
+            "{:?}",
+            std::str::from_utf8(record)
+        );
+    }
+}

rust/core/src/schema_helpers_tests.rs

@@ -317,7 +317,43 @@ fn nth_attribute_reversed_boundary_is_false() {
     assert!(!nth_attribute_is_present(b"garbage)stuff(more", 0));
 }
 
-/// The five IFC2X3 products #3172 added to `legacy_entities.rs`.
+/// Every key in `legacy_entities.rs`, in one place.
+///
+/// Two tests assert OPPOSITE directions about these names and a second copy
+/// of the list would let them drift apart:
+/// `every_legacy_arm_maps_onto_a_known_product` says each maps to a known base
+/// type; `every_legacy_key_is_unknown_to_the_generated_enum` says none of them
+/// is a name `IfcType::from_str` already knows.
+const LEGACY_KEYS: [&str; 26] = [
+    "IFCPRESENTATIONSTYLEASSIGNMENT",
+    "IFCBEAMSTANDARDCASE",
+    "IFCCOLUMNSTANDARDCASE",
+    "IFCMEMBERSTANDARDCASE",
+    "IFCPLATESTANDARDCASE",
+    "IFCSLABSTANDARDCASE",
+    "IFCDOORSTANDARDCASE",
+    "IFCWINDOWSTANDARDCASE",
+    "IFCOPENINGSTANDARDCASE",
+    "IFCSLABELEMENTEDCASE",
+    "IFCWALLELEMENTEDCASE",
+    "IFCDOORSTYLE",
+    "IFCWINDOWSTYLE",
+    "IFCPROXY",
+    "IFCBUILDINGELEMENT",
+    "IFCBUILDINGELEMENTTYPE",
+    "IFCEQUIPMENTELEMENT",
+    "IFCELECTRICDISTRIBUTIONPOINT",
+    "IFCELECTRICALELEMENT",
+    "IFCCHAMFEREDGEFEATURE",
+    "IFCROUNDEDEDGEFEATURE",
+    "IFCSTRUCTURALLINEARACTIONVARYING",
+    "IFCSTRUCTURALPLANARACTIONVARYING",
+    "IFCSOLIDSTRATUM",
+    "IFCVOIDSTRATUM",
+    "IFCWATERSTRATUM",
+];
+
+/// The six IFC2X3 products #3172 added to `legacy_entities.rs`.
 ///
 /// Each one is CONCRETE and carries both `ObjectPlacement` and
 /// `Representation` in `@ifc-lite/data`'s IFC2X3 table, and each one resolved
@@ -396,34 +432,7 @@ fn edge_features_are_subtraction_operands_not_openings() {
 /// dropped as having no arm at all, while looking handled in the table.
 #[test]
 fn every_legacy_arm_maps_onto_a_known_product() {
-    for name in [
-        "IFCPRESENTATIONSTYLEASSIGNMENT",
-        "IFCBEAMSTANDARDCASE",
-        "IFCCOLUMNSTANDARDCASE",
-        "IFCMEMBERSTANDARDCASE",
-        "IFCPLATESTANDARDCASE",
-        "IFCSLABSTANDARDCASE",
-        "IFCDOORSTANDARDCASE",
-        "IFCWINDOWSTANDARDCASE",
-        "IFCOPENINGSTANDARDCASE",
-        "IFCSLABELEMENTEDCASE",
-        "IFCWALLELEMENTEDCASE",
-        "IFCDOORSTYLE",
-        "IFCWINDOWSTYLE",
-        "IFCPROXY",
-        "IFCBUILDINGELEMENT",
-        "IFCBUILDINGELEMENTTYPE",
-        "IFCEQUIPMENTELEMENT",
-        "IFCELECTRICDISTRIBUTIONPOINT",
-        "IFCELECTRICALELEMENT",
-        "IFCCHAMFEREDGEFEATURE",
-        "IFCROUNDEDEDGEFEATURE",
-        "IFCSTRUCTURALLINEARACTIONVARYING",
-        "IFCSTRUCTURALPLANARACTIONVARYING",
-        "IFCSOLIDSTRATUM",
-        "IFCVOIDSTRATUM",
-        "IFCWATERSTRATUM",
-    ] {
+    for name in LEGACY_KEYS {
         let info = crate::legacy_entities::get_legacy_entity_info(name)
             .unwrap_or_else(|| panic!("{name} has no arm in legacy_entities.rs"));
         assert!(
@@ -447,11 +456,13 @@ fn every_legacy_arm_maps_onto_a_known_product() {
 /// `Unknown`, and `Unknown` stores a CRC32 hash rather than the name, so the
 /// keyword survives only in the record.
 ///
-/// The keywords here are ones `legacy_entities.rs` carries on `main`. The six
-/// #3172 adds are deliberately absent: this branch is cut from `main` and must
-/// not depend on a table that is still in review, or it goes green here and red
-/// on merge in whichever order the two land. The first draft used
-/// `IFCELECTRICALELEMENT` and failed for exactly that reason.
+/// The keywords here all predate #3172, which has since landed and wrote six
+/// arms, one of them replacing a misspelling (the table went 21 -> 26). They
+/// are kept because what this test exercises is the WIRE-UP --
+/// that a record reaches `legacy_entities.rs` at all -- not the table's
+/// contents, which `legacy_ifc2x3_products_resolve_to_their_own_supertype`
+/// above covers directly. Choosing rows that do not move keeps the two tests
+/// from failing together for one cause.
 #[test]
 fn a_legacy_record_resolves_where_the_decoded_type_cannot() {
     // What the decoder produces for these, and what the browser was emitting.
@@ -508,3 +519,36 @@ fn an_unreadable_record_changes_nothing() {
         assert_eq!(legacy_aware_ifc_type_from_record(unknown, record), unknown);
     }
 }
+
+/// The unstated invariant `legacy_aware_ifc_type_from_record` rests on.
+///
+/// That function short-circuits on `!matches!(decoded, IfcType::Unknown(_))`,
+/// which is only equivalent to `legacy_aware_ifc_type` while every key in the
+/// legacy table is a name the generated enum does NOT know. If a schema
+/// regeneration ever emits an arm for one of these keys -- and several are real
+/// IFC2x3/IFC4 entities, `IFCPRESENTATIONSTYLEASSIGNMENT` among them --
+/// `from_str` stops returning `Unknown`, the early return fires, and the wasm
+/// path silently returns the raw variant while the native path still remaps it.
+/// That is exactly the wasm-vs-native divergence #3179 was filed for, and it
+/// would come back in a form no other test here observes.
+///
+/// `every_legacy_arm_maps_onto_a_known_product` asserts the BASE TYPE is known.
+/// This asserts the KEY is not: the opposite direction, and the one the
+/// short-circuit depends on.
+///
+/// This is the BEHAVIOURAL half -- it calls `from_str` for real. The derived
+/// half is in `scripts/check-legacy-entity-coverage.mjs`, which reads both sets
+/// out of the source and so also catches a 27th arm added without touching
+/// `LEGACY_KEYS`, which this test cannot see.
+#[test]
+fn every_legacy_key_is_unknown_to_the_generated_enum() {
+    for key in LEGACY_KEYS {
+        assert!(
+            matches!(IfcType::from_str(key), IfcType::Unknown(_)),
+            "{key} is now known to `IfcType::from_str`, so the `Unknown` \
+             short-circuit in `legacy_aware_ifc_type_from_record` skips its \
+             legacy remap and the browser diverges from the native path again"
+        );
+    }
+}
+

rust/wasm-bindings/src/api/gpu_meshes/batch.rs

@@ -400,23 +400,33 @@ impl IfcAPI {
                 continue;
             };
             // LEGACY-AWARE, like the native pre-pass at
-            // `processing/src/processor/mod.rs:711`. `entity.ifc_type` comes
-            // from the decoder's bare `IfcType::from_str`, so every keyword
-            // IFC4X3 dropped -- IfcProxy, the *StandardCase family, the strata
-            // leaves, the six #3172 added -- arrived here as `Unknown` and was
-            // emitted to the browser with `ifcType: "Unknown"` and the Unknown
-            // default colour, while the CLI and exporters labelled the same
-            // entity correctly (#3179).
+            // `processing/src/processor/mod.rs:711`. Without this, a legacy
+            // keyword reaching this path arrived as `ifcType: "Unknown"` with
+            // the Unknown default colour, while the CLI and exporters labelled
+            // the same entity correctly (#3179).
+            //
+            // Not every dropped keyword reaches this line: the four arms with
+            // `has_geometry: false` are refused by `has_geometry_by_name` in
+            // all three element producers, so this fix misses them. #3187 has
+            // the trace, including the type-geometry gate that would make
+            // THREE of the four eligible -- eligible, not certain.
             //
             // Recomputed from the SOURCE KEYWORD rather than recovered from
-            // `entity.ifc_type`, because it cannot be: `IfcType::Unknown(u32)`
-            // stores a CRC32 hash and `DecodedEntity` keeps no raw name, so
-            // there is nothing to map back from. The bytes are already in hand
-            // -- `content[start..end]` is the record `decode_and_cache` just
-            // parsed -- so this is a ~20-byte scan to the first `(`, not a
-            // re-read. Cheaper than widening the jobs wire, which is 3 u32 per
-            // job across Rust and TS and would have paid marshalling cost on
-            // every job to fix the few that are legacy.
+            // `entity.ifc_type` -- why it cannot be recovered is on
+            // `legacy_aware_ifc_type_from_record` itself. What is specific to
+            // HERE: the bytes are already in hand -- `content[start..end]` is
+            // the span THIS JOB carries, so this is a ~20-byte scan to the
+            // first `(`, not a re-read. Note it is the job's span, not
+            // necessarily what `decode_and_cache` parsed: on a cache hit
+            // (`core/src/decoder.rs:443`) it returns the cached `Arc` without
+            // reading `start`/`end` at all, so only the miss path bounds-checks
+            // them. `content.get(..)` is fail-soft for that reason -- an
+            // out-of-range span yields an empty slice and `decoded` is returned
+            // unchanged.
+            //
+            // Cheaper than widening the jobs wire, which is 3 u32 per job
+            // across Rust and TS and would have paid marshalling cost on every
+            // job to fix the few that are legacy.
             let ifc_type = ifc_lite_core::legacy_aware_ifc_type_from_record(
                 entity.ifc_type,
                 content.get(start..end).unwrap_or_default(),