runtime: unify createRequire builtin allowlist with getBuiltinModule — v8 + family audit (#6651) - #6653
Merged
proggeramlug merged 2 commits intoJul 19, 2026
Conversation
📝 WalkthroughWalkthroughChangesThe runtime now uses Builtin module resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant createRequire
participant process_getBuiltinModule
participant builtin_registry
participant node_submodules
Caller->>createRequire: require builtin specifier
createRequire->>builtin_registry: normalize and validate specifier
builtin_registry-->>createRequire: resolved module name
createRequire->>node_submodules: resolve submodule-backed value
node_submodules-->>createRequire: return builtin namespace
Caller->>process_getBuiltinModule: getBuiltinModule specifier
process_getBuiltinModule->>builtin_registry: normalize and validate specifier
builtin_registry-->>process_getBuiltinModule: resolved module name
process_getBuiltinModule->>node_submodules: resolve submodule-backed value
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
crates/perry/tests/createrequire_builtin_modules.rs (1)
183-208: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftReject unresolved or method-dead namespaces in the exhaustive guard.
Checking only
typeof value === "object" || "function"lets an empty unresolved namespace stub pass. Validate a known export contract for each routing family, or expose a test-only predicate that distinguishes unresolved namespaces.Mirror the strengthened routing assertion in a
cargo-test-visible unit test where practical. As per coding guidelines, “Prefer acceptance coverage incargo-test-visible unit tests because integration suites undercrates/*/tests/*.rsdo not run on every PR.” <coding_guidelines>🤖 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/perry/tests/createrequire_builtin_modules.rs` around lines 183 - 208, Strengthen the exhaustive builtin checks around the `resolvable` predicate so unresolved or method-dead namespace stubs cannot pass based only on `typeof`; validate an appropriate known export contract for each builtin routing family or use a test-only predicate that distinguishes valid namespaces. Mirror this routing assertion in a cargo-test-visible unit test, reusing the relevant runtime/module-resolution symbols rather than relying solely on `createrequire_builtin_modules.rs`.Source: Coding guidelines
🤖 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/perry-hir/src/lower/lower_expr/arm_class.rs`:
- Around line 188-205: Ensure every class-expression evaluation region has a
drainable refresh owner: in
crates/perry-hir/src/lower/lower_expr/arm_class.rs:188-205, route module-level
block captures through module-init refresh handling; in
crates/perry-hir/src/lower/expr_function.rs:384-394, preserve refresh entries
for assignments later in expression bodies; and in
crates/perry-hir/src/lower_patterns.rs:1457-1469, retain default-expression
captures until subsequent parameters initialize. Add regressions covering
module-level blocks, expression arrows, and parameter defaults.
In `@crates/perry-runtime/src/process.rs`:
- Around line 123-133: Update the builtin recognition logic around the specifier
normalization to reject any remaining `node:` prefix after stripping one prefix,
including `node:node:test`, before checking `MODULE_BUILTIN_MODULES`. Preserve
acceptance of ordinary builtin names and their single `node:`-prefixed forms,
and apply the same validation to both dynamic resolver paths.
In `@crates/perry-runtime/src/value/dynamic_arith.rs`:
- Around line 718-721: The raw operands in js_dynamic_ushr and throw_mix_bigint
must be rooted across GC-capable calls. Introduce crate::gc::RuntimeHandleScope
for both operands, then re-read each rooted value with get_nanbox_f64() before
the BigInt check and before throw_mix_bigint’s second diagnostic print,
respectively.
---
Nitpick comments:
In `@crates/perry/tests/createrequire_builtin_modules.rs`:
- Around line 183-208: Strengthen the exhaustive builtin checks around the
`resolvable` predicate so unresolved or method-dead namespace stubs cannot pass
based only on `typeof`; validate an appropriate known export contract for each
builtin routing family or use a test-only predicate that distinguishes valid
namespaces. Mirror this routing assertion in a cargo-test-visible unit test,
reusing the relevant runtime/module-resolution symbols rather than relying
solely on `createrequire_builtin_modules.rs`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 050bb6a3-ee9b-4da3-ab74-76a206b52a3f
📒 Files selected for processing (21)
crates/perry-codegen/src/codegen/module_globals_emit.rscrates/perry-codegen/src/lower_call/native_table/node_core/module_sea_tls_test.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_expr/arm_class.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_patterns.rscrates/perry-runtime/src/module_require.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/node_module.rscrates/perry-runtime/src/typedarray/bigint.rscrates/perry-runtime/src/value/dynamic_arith.rscrates/perry/tests/createrequire_builtin_modules.rscrates/perry/tests/module_destructured_globals.rstest-parity/node-suite/bigint/arithmetic/mixed-operand-errors.tstest-parity/node-suite/node-core/module-destructure-function-visibility.tstest-parity/node-suite/object/class-expr-capture-refresh.tstests/test_class_expr_capture_refresh_6604.sh
Comment on lines
188
to
+205
| let at_module_top = ctx.scope_depth == 0 && ctx.inside_block_scope == 0; | ||
| // #6604: register this capturing class EXPRESSION with the enclosing | ||
| // body's end-of-body capture-refresh machinery (#6037/#6052), which | ||
| // previously scanned class DECLARATION statements only. Without the | ||
| // refresh, a captured var assigned AFTER the class expression (semver's | ||
| // `var Comparator = class _Comparator { … }; …; var parseOptions = | ||
| // require_parse_options()`) stays `undefined` in the decl-site snapshot, | ||
| // and dynamic construction of the escaped class value replays that stale | ||
| // snapshot. Recording the RESOLVED registration name here (post | ||
| // rename/dedup) sidesteps re-deriving it from the AST at body end. Module | ||
| // top is skipped — module-level ids are stripped from capture lists by | ||
| // `filter_module_level_captures`, so there is nothing to refresh. | ||
| if !at_module_top && !captured_args.is_empty() { | ||
| if let Some(ids) = ctx.lookup_class_captures(&synthetic_name) { | ||
| ctx.body_class_expr_captures | ||
| .push((synthetic_name.clone(), ids.to_vec())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Define a refresh owner for every class-expression evaluation region.
The new bookkeeping assumes every registered class expression belongs to a drainable function body, but top-level blocks strand entries while expression arrows and parameter defaults discard entries before later live-binding updates.
crates/perry-hir/src/lower/lower_expr/arm_class.rs#L188-L205: route module-level block captures through module-init refresh handling.crates/perry-hir/src/lower/expr_function.rs#L384-L394: preserve refreshes for assignments occurring later in an expression body.crates/perry-hir/src/lower_patterns.rs#L1457-L1469: retain default-expression captures until later parameters have initialized.
Add regressions for all three evaluation shapes.
📍 Affects 3 files
crates/perry-hir/src/lower/lower_expr/arm_class.rs#L188-L205(this comment)crates/perry-hir/src/lower/expr_function.rs#L384-L394crates/perry-hir/src/lower_patterns.rs#L1457-L1469
🤖 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/perry-hir/src/lower/lower_expr/arm_class.rs` around lines 188 - 205,
Ensure every class-expression evaluation region has a drainable refresh owner:
in crates/perry-hir/src/lower/lower_expr/arm_class.rs:188-205, route
module-level block captures through module-init refresh handling; in
crates/perry-hir/src/lower/expr_function.rs:384-394, preserve refresh entries
for assignments later in expression bodies; and in
crates/perry-hir/src/lower_patterns.rs:1457-1469, retain default-expression
captures until subsequent parameters initialize. Add regressions covering
module-level blocks, expression arrows, and parameter defaults.
Comment on lines
718
to
+721
| pub unsafe extern "C" fn js_dynamic_ushr(a: f64, b: f64) -> f64 { | ||
| let a = to_numeric(a); | ||
| let b = to_numeric(b); | ||
| let a_big = JSValue::from_bits(a.to_bits()).is_bigint(); | ||
| let b_big = JSValue::from_bits(b.to_bits()).is_bigint(); | ||
| if a_big || b_big { | ||
| if both_bigint_or_throw(a, b) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg 'RuntimeHandleScope::' crates/perry-runtime/src/Repository: PerryTS/perry
Length of output: 38252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline dynamic_arith.rs ==\n'
ast-grep outline crates/perry-runtime/src/value/dynamic_arith.rs --view expanded || true
printf '\n== relevant slices ==\n'
sed -n '1,120p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n---\n'
sed -n '680,760p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n== RuntimeHandleScope definitions/usages ==\n'
rg -n "struct RuntimeHandleScope|impl RuntimeHandleScope|fn refreshed_nanbox_f64_slice|fn get_nanbox_f64|fn new\(" crates/perry-runtime/src/gc crates/perry-runtime/src | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 23791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== runtime_handles.rs ==\n'
sed -n '1,260p' crates/perry-runtime/src/gc/roots/runtime_handles.rs
printf '\n== nearby rooting patterns in dynamic_arith.rs ==\n'
sed -n '280,340p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n---\n'
sed -n '340,430p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n---\n'
sed -n '430,520p' crates/perry-runtime/src/value/dynamic_arith.rs
printf '\n== other call sites using RuntimeHandleScope in arithmetic-like code ==\n'
rg -n "refreshed_nanbox_f64_slice|get_nanbox_f64|RuntimeHandleScope::new\(" crates/perry-runtime/src/value crates/perry-runtime/src/object crates/perry-runtime/src/builtins | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 33281
Root the operands across the GC-capable coercions
js_dynamic_ushr and throw_mix_bigint both keep raw NaN-boxed f64s live across calls that can allocate or invoke JS (to_numeric / describe_mix_operand). Root both operands with crate::gc::RuntimeHandleScope, then re-read them with get_nanbox_f64() before the BigInt check / second diagnostic print, so a GC can’t leave stale pointers behind.
📍 Affects 1 file
crates/perry-runtime/src/value/dynamic_arith.rs#L718-L721(this comment)crates/perry-runtime/src/value/dynamic_arith.rs#L58-L67
🤖 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/perry-runtime/src/value/dynamic_arith.rs` around lines 718 - 721, The
raw operands in js_dynamic_ushr and throw_mix_bigint must be rooted across
GC-capable calls. Introduce crate::gc::RuntimeHandleScope for both operands,
then re-read each rooted value with get_nanbox_f64() before the BigInt check and
before throw_mix_bigint’s second diagnostic print, respectively.
Source: Learnings
proggeramlug
pushed a commit
to proggeramlug/perry
that referenced
this pull request
Jul 19, 2026
… resolution (CodeRabbit on PerryTS#6653) Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Contributor
Author
|
CodeRabbit findings addressed:
|
added 2 commits
July 19, 2026 03:33
…— v8 + family audit (PerryTS#6651) require('node:v8') through createRequire (the esbuild banner shim in the pi bundle) threw ERR_PERRY_UNSUPPORTED_CREATE_REQUIRE even though v8 has a full native module (node_v8.rs, nm dispatch bucket, static-import support). Same drift family as PerryTS#6644: THREE hand-copied module lists (createRequire's supported_require_builtin, process.getBuiltinModule's supported_builtin_module_name, and module.builtinModules' MODULE_BUILTIN_MODULES) each missing a different subset. Instead of adding v8 to two lists, both dynamic resolvers now derive from MODULE_BUILTIN_MODULES — the same table module.builtinModules and module.isBuiltin already serve — with Node's exact carve-outs: - `_`-prefixed legacy internals stay unresolvable (Perry has no implementation; the error keeps naming the module). - Scheme-only builtins (node:sea, node:sqlite, node:test, node:test/reporters — stored WITH the prefix, as Node spells them) resolve only with the node: prefix; the bare spelling is an npm package name in Node (require MODULE_NOT_FOUND / getBuiltinModule undefined). This also fixes the previous bare-'test'/'sqlite' looseness. Newly reachable via createRequire (both spellings): v8, fs/promises, stream/consumers, stream/web, trace_events; node:sea. Newly reachable via getBuiltinModule: v8, tls, dgram, domain, inspector, inspector/promises, repl, wasi, path/posix, path/win32, fs/promises, stream/consumers, stream/web, trace_events, node:sqlite. Value routing is shared too (process::builtin_module_value): submodule-spec modules (diagnostics_channel, timers/promises, fs/promises, stream/consumers, stream/web, trace_events, test/reporters — the last was resolving to a method-dead namespace, spec/tap were undefined) go through the node_submodules registry, the rest through the native-module namespace. The PerryTS#6644 devirt install-all hooks already arm every bucket both registries need. Regression guards: perry-runtime unit tests assert the resolvers cover every MODULE_BUILTIN_MODULES entry (and only Node's spellings) and that every submodule route names a registered spec; a compiled fixture sweeps module.builtinModules through require + getBuiltinModule in both spellings, including the negative cases; a minimal node:v8 fixture is byte-identical to node v26. Fixes PerryTS#6651 Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
… resolution (CodeRabbit on PerryTS#6653) Claude-Session: https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
proggeramlug
force-pushed
the
fix/6651-createrequire-allowlist-family
branch
from
July 19, 2026 01:33
50aff8c to
b7440cc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/tests/createrequire_builtin_modules.rs (1)
63-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrefer unit tests for acceptance coverage.
As per coding guidelines, prefer acceptance coverage in
cargo-test-visible unit tests because integration suites undercrates/*/tests/*.rsdo not run on every PR.While tests that must invoke the full
perrybinary might necessitate this location, please consider if these checks can be refactored into unit tests to ensure they run consistently on all PRs.🤖 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/perry/tests/createrequire_builtin_modules.rs` around lines 63 - 64, Refactor the coverage for createrequire diagnostics-channel and node-prefixed builtins from the integration test createrequire_resolves_diagnostics_channel_and_node_prefixed_builtins into a cargo-test-visible unit test, unless invoking the full perry binary is required. Preserve both resolution checks and their acceptance behavior in the unit-test location.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/perry/tests/createrequire_builtin_modules.rs`:
- Around line 63-64: Refactor the coverage for createrequire diagnostics-channel
and node-prefixed builtins from the integration test
createrequire_resolves_diagnostics_channel_and_node_prefixed_builtins into a
cargo-test-visible unit test, unless invoking the full perry binary is required.
Preserve both resolution checks and their acceptance behavior in the unit-test
location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bbcfa52-2a60-4b74-9f41-167c71a2e8b6
📒 Files selected for processing (5)
crates/perry-runtime/src/module_require.rscrates/perry-runtime/src/node_submodules/mod.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/node_module.rscrates/perry/tests/createrequire_builtin_modules.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/perry-runtime/src/process/node_module.rs
- crates/perry-runtime/src/node_submodules/mod.rs
- crates/perry-runtime/src/process.rs
This was referenced Jul 24, 2026
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wall #5 of the pi bring-up (tracker #6564).
require('node:v8')through the pi bundle's esbuild createRequire banner threwERR_PERRY_UNSUPPORTED_CREATE_REQUIRE— same drift family as #6644 (wall #3): three hand-copied builtin lists, each missing a different subset.Stacks on #6650 (which stacks on #6647 ← #6633); only the last commit is this PR.
Audit
Perry had THREE independently maintained module lists:
supported_require_builtin(module_require.rs) — createRequire'srequiresupported_builtin_module_name(process.rs) —process.getBuiltinModuleMODULE_BUILTIN_MODULES(process.rs) —module.builtinModules/module.isBuiltin, content-identical to the static-import tables' node-builtin subset (NATIVE_MODULES∪NODE_SUBMODULESinperry-api-manifest)Full diff, verified empirically with a compiled probe (before → after; static import was already ✅ for every row — all of these are implemented modules):
v8fs/promisesstream/consumersstream/webtrace_eventsnode:seanode:sqlitetls,dgram,domain,inspector,inspector/promises,repl,wasi,path/posix,path/win32node:test/reportersspec/tap=undefined) → ✅ live exportssea/sqlite/test/test/reporterstest(+reporters) resolved → nowundefined(Node parity: scheme-only)sqlite/test(+reporters) resolved → now throw naming the module (Node:MODULE_NOT_FOUND)Fix
Instead of adding
v8to two lists, both dynamic resolvers now derive fromMODULE_BUILTIN_MODULES— the same tablemodule.builtinModules/module.isBuiltinalready serve — so the surfaces cannot drift again:process::supported_builtin_module_name(specifier)is the one allowlist (handles thenode:strip). Node-exact carve-outs:_-prefixed legacy internals stay unresolvable (Perry doesn't implement them — the error keeps naming the module); scheme-only builtins (node:sea,node:sqlite,node:test,node:test/reporters, stored WITH the prefix exactly as Node spells them inbuiltinModules) resolve only when the caller wrote the prefix.process::builtin_module_value(name)is the one value router: submodule-spec modules (diagnostics_channel,timers/promises,fs/promises,stream/consumers,stream/web,trace_events,test/reporters) go through thenode_submodulesregistry; the rest get the native-module namespace. The runtime: createRequire rejects node:-prefixed builtins / missing diagnostics_channel — pi wall #3 (require('node:diagnostics_channel') via bundle shim) #6644 devirt entries (js_module_create_require_devirt/js_process_get_builtin_module_devirt) already arm the install-all hooks for every bucket both registries need — no codegen change required.module_require::supported_require_builtin/require_builtin_valuedelegate to the shared functions; the runtime: createRequire rejects node:-prefixed builtins / missing diagnostics_channel — pi wall #3 (require('node:diagnostics_channel') via bundle shim) #6644 special-cases are absorbed.Tests
perry-runtime --lib, 1426 pass,--test-threads=1): resolvers cover everyMODULE_BUILTIN_MODULESentry in exactly Node's spellings (incl. scheme-only +_-internal negatives); every submodule route names a registered spec; createRequire's resolver provably matches getBuiltinModule's for all spellings.crates/perry/tests/createrequire_builtin_modules.rs, 4 pass): minimalnode:v8fixture (serialize typeof + deserialize round-trip + getBuiltinModule) byte-identical to node v26.3.0; a compiled sweep iterates the binary's ownmodule.builtinModulesthroughrequire+getBuiltinModulein both spellings, asserting resolution, the scheme-only bare rejections, and that unimplemented internals fail with the module named. Both runtime: createRequire rejects node:-prefixed builtins / missing diagnostics_channel — pi wall #3 (require('node:diagnostics_channel') via bundle shim) #6644 tests stay green.ERR_PERRY_UNSUPPORTED_CREATE_REQUIREcode where node saysMODULE_NOT_FOUNDfor bare scheme-only names (message names the module in both), and (b)node:testdefault export is callable in node, an object namespace in Perry (pre-existing shape gap, not reachability).pi gate
Full pi recompile (old binary kept as
pi-native.pre6651): wall #5 is dead —pi-native.pre6651 --helpstill dies on thenode:v8createRequire throw; the newpi-nativegets past it (and past the bundle'snode:stream/web/node:fs/promisesrequires, which would have been the next two walls of this family). GATE 1 not yet green: a new, unrelated wall #6 appears during module init, filed as #6652 with a 3-line repro:= bare-global
hasOwnProperty.call(o, t4)in@babel/types/lib/definitions/placeholders.js— Perry's unknown-identifier-assume-global path doesn't serve Object.prototype-INHERITED members of the global object (compile warning:unknown identifier 'hasOwnProperty' — assuming global; 14 such sites in the bundle + 2 baretoString). Both--helpand--versionhit the same line; node prints full usage /0.0.0rc=0. Not chased here per the one-wall-per-PR flow.Fixes #6651
https://claude.ai/code/session_01JuiiePQfrXhAFD9fuCygB9
Summary by CodeRabbit
Bug Fixes
require,createRequire, andprocess.getBuiltinModule.node:-prefixed module names, with correct handling of unsupported legacy and scheme-only modules.Tests
v8, serialization APIs, and all registered built-ins.