fix(runtime): JSX and raw-JSON shared a class id; add a scanning gate (#7587) - #7589
Conversation
…#7587) JSX_NODE_CLASS_ID and RAW_JSON_CLASS_ID were both 0xFFFF_00A0 -- the second such collision in a week after #7576. Not inert: value/to_string.rs discriminates on class_id ALONE and returns field 0, and both types keep their payload in field 0, so String(JSON.rawJSON("123")) took the JSX arm and looked correct purely by coincidence of layout. RAW_JSON_CLASS_ID moves to the free 0xFFFF_00A1. Neither id is baked into codegen or persisted, so the move is internal. The more important half is the gate. #7576 added a Rust test enumerating seven iterator ids; it is good and stays, but it could not have caught this -- a different family, not in the list. A gate whose coverage depends on the same attention the bug depends on is not a gate. scripts/class_id_collisions.py SCANS every crate instead, so a new constant is covered the moment it is written. It detects two shapes. Different names on one value is the collision. The same name on one value is a deliberate cross-crate mirror (perry-ext-events restates the runtime's ABORT_SIGNAL_CLASS_ID) and is allowed -- but one name carrying DIFFERENT values is mirror drift, which is worse than a collision because each crate stays self-consistent while the type stops being recognised across the boundary. Sabotage-verified with real exit codes: reintroducing the collision exits 1, drifting the AbortSignal mirror exits 1, raising the stale-scan floor exits 2, clean exits 0.
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
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 |
1 participant
JSX_NODE_CLASS_IDandRAW_JSON_CLASS_IDwere both0xFFFF_00A0— the second class-id collision found in a week, after #7576'sITERATOR_HELPER_CLASS_ID/STRING_ITERATOR_CLASS_IDpair took out the entire TC39 iterator-helpers surface.It was not inert
value/to_string.rs:1144discriminates onclass_idalone — no second condition — and returns field 0 as the object's string. A raw-JSON wrapper is allocated with one field and keeps its text in field 0, soString(JSON.rawJSON("123"))matched the JSX arm and stringified through it.It returned a plausible answer purely by coincidence of layout — both types happen to store their payload in the same slot. That is the more instructive failure: a collision can produce right-looking output for exactly as long as the two types agree structurally, and break the day either layout changes.
RAW_JSON_CLASS_IDmoves to the free0xFFFF_00A1. Neither id is referenced byperry-codegen/perry-hiror persisted anywhere, so the move is internal.The gate is the more important half
#7576 added
iterator_class_ids_are_pairwise_distinct, a Rust test enumerating seven iterator ids. It is good and it stays. It could not have caught this — different family, not in the list.That is the structural problem with an enumerated list: it covers only the constants somebody remembered to add, and the failure being guarded against is forgetting that a constant exists. A gate whose coverage depends on the same attention the bug depends on is not a gate.
scripts/class_id_collisions.pyscans every crate instead, so a new constant is covered the moment it is written, with no list to update. Wired intolintnext to the addr-class audit.It detects two shapes, and the second one found itself. Different names on one value is the collision. The same name on one value is a deliberate cross-crate mirror —
perry-ext-eventsrestates the runtime'sABORT_SIGNAL_CLASS_IDso it can recognise runtime AbortSignal objects — which is correct and must not be reported, or the gate is permanently red. But one name carrying different values is mirror drift, and that is worse than a collision: each crate stays internally consistent, so nothing looks wrong, while the type silently stops being recognised across the boundary. The gate catches both.Sabotage-verified, with real exit codes
Captured without a pipe, because
$?after| headishead's status — a trap that has bitten this repo:0xFFFF_00A0collisionABORT_SIGNAL_CLASS_IDmirrorMIN_CONSTANTS(stale-scan floor)The floor exists because a scan that silently matches nothing prints "no collisions" and means nothing — same discipline as
gc_root_dominance_corpus.sh'sMIN_COMPILED.Validation
JSON.rawJSONbyte-identical to node 26.5.1 acrossisRawJSON,stringify, therawJSONown property, and a nested/mixed array; JSX rendering unchanged as a control.cargo test -p perry-runtime --lib1838 passed / 0 failed.addr_class_inventory,raw_handle_debt(998),check_file_size.sh,cargo fmt --checkall clean.CI has a deep backlog and may not report; this is local validation.