Repository navigation
install: compile hoist patterns on first match so config load does not initialize JSC - #39896
Conversation
PnpmMatcher compiled its hoistPattern / publicHoistPattern regexes while bunfig.toml or .npmrc loaded. The compile path initializes JSC, and JSC takes its process-wide options from the first initialize call. Every command loads its config before it initializes JSC, so a config with a hoist pattern made the command's own options no-ops: `bun -p` lost eval mode and printed undefined for every expression, and `bun test --isolate` lost its FTL threshold. The matcher now stores the escaped pattern and compiles it on the first is_match call. Only the install linker calls is_match, so JSC is first initialized by the command itself, or by the matcher inside `bun install`, which never initializes JSC otherwise. The escaping in create_matcher leaves the Yarr pattern size limit as the only way a pattern can fail to compile, so the eager "Invalid regex" config error is gone. Such a pattern now never matches.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 2 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Updated 1:43 AM PT - Aug 21st, 2026
❌ @robobun, your commit 2bcef5c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39896That installs a local version of the PR into your bun-39896 --bun |
|
Status: ready for review. Synced with main in 2bcef5c at the maintainer's request. CI on the merged branch (build 102493) is green for this diff: the only red lane is Reproduced on bun 1.4.0. In a directory with this [install]
hoistPattern = ["*eslint*"]
With this branch, the same directory gives |
There was a problem hiding this comment.
LGTM — deferring the Yarr compile to first match via OnceCell cleanly fixes the JSC-init ordering bug.
What was reviewed:
create_matchernow stores the escaped pattern in an ownedBox<[u8]>and compiles lazily inmatches(); no arena-lifetime issues since the source is copied out before the scratch arena drops.CompiledRegularExpression::Dropstill routes through__bun_regex_drop, andOnceCell<Option<…>>drops it correctly; the type was already!Send + !Syncvia the raw pointer, so no thread-contract change.- All
create_matcher/CreateMatcherError/FromExprError::InvalidRegExpreferences updated (onlyNodeLinker.rsandini/lib.rsname them);is_matchcallers confirmed to be install-linker-only (isolated_install.rs,prune.rs). - New tests assert the observable symptom (
bun -poutput,thresholdForFTLOptimizeAfterWarmUp) for bothhoistPatternandpublicHoistPattern.
Extended reasoning...
Overview
The PR defers compilation of [install] hoistPattern / publicHoistPattern regexes from bunfig/.npmrc load time to first is_match call. Before, create_matcher called __bun_regex_compile → jsc::initialize(false) during config parsing, which fixed JSC's process-wide options before bun -p (needs evalMode) or bun test --isolate (needs raised FTL threshold) could pass their own. The fix wraps the compiled regex in OnceCell<Option<CompiledRegularExpression>> and stores the escaped source bytes; the ancillary hunks in bunfig.rs and ini/lib.rs mechanically remove the InvalidRegExp/CreateMatcherError variants that only the eager compile could produce.
Security risks
None. Hoist patterns are user config, not untrusted network input; the escaping in escape_reg_exp_for_package_name_matching is unchanged. The only behavior change on adversarial input is that a >1 MiB pattern now silently never matches instead of failing config load — a strictly more permissive degenerate case, documented in the PR body.
Level of scrutiny
Medium. JSC initialization ordering is subtle, but the change is well-contained: create_matcher becomes infallible and stores bytes; matches() does the one-time compile. I verified via grep that create_matcher/CreateMatcherError have no other callers, and that PnpmMatcher::is_match is only reached from isolated_install.rs:build_store and prune.rs — both inside the install linker, well after any JS-running command has initialized JSC with its own options. The OnceCell (not OnceLock) is correct: the type was already !Send + !Sync through the raw NonNull<()>.
Other factors
- Memory:
sourceisVec::into_boxed_slice()(owned heap), so the scratchArenainfrom_exprcan drop safely aftercreate_matcherreturns — the comment was updated to reflect the copy.CompiledRegularExpressionkeeps itsDropimpl, andOnceCelldrops its contents, so the Yarr allocation is still freed. - Tests: 8 new
bun -pcases (both bunfig keys × 4 expressions including a TLA) and 1--isolatecase asserting the exact JSC option value viaBUN_JSC_dumpOptions=2. The PR body confirms they fail on 1.4.0 and pass on the branch, and lists the install pattern suites (public-hoist-pattern,isolated-install -t hoist,bun-prune -t publicHoistPattern,config-precedence,npmrc) as passing. - The fix is at the right layer: the matcher owns its own laziness rather than each command reordering its config load.
There was a problem hiding this comment.
LGTM — clean, well-scoped fix that defers Yarr regex compilation to first match so bunfig loading no longer initializes JSC ahead of the command.
What was reviewed:
- Traced
PnpmMatcher::is_matchcallers — onlyisolated_install.rsandprune.rs, both install-time, so lazy compile never precedes a JS-running command's ownjsc::initialize. - Checked
OnceCell<Option<CompiledRegularExpression>>lifetime:Dropcalls__bun_regex_drop;OnceCellis!Syncand the type was already!Send/!SyncviaNonNull, so no thread-contract change. - Verified no stale references to the removed
CreateMatcherError/FromExprError::InvalidRegExp;log/sourceparams remain live for theUnexpectedExprpath. - Confirmed
escape_reg_exp_for_package_name_matchingescapes every metachar, so the dropped compile-error path is only reachable via Yarr's size limit — silently not matching is reasonable there.
Extended reasoning...
Overview
The PR fixes a bug where bun -p prints undefined and bun test --isolate loses its JSC option overrides whenever bunfig.toml contains [install] hoistPattern or publicHoistPattern. Root cause: create_matcher compiled each pattern eagerly during config load, and __bun_regex_compile calls jsc::initialize(false) first — since JSCInitialize is std::call_once, the command's later call with its own options (evalMode, FTL threshold) became a no-op.
The fix restructures RegularExpression in NodeLinker.rs to store the escaped pattern source in a Box<[u8]> and compile lazily via OnceCell<Option<CompiledRegularExpression>> on first matches(). create_matcher becomes infallible; the CreateMatcherError type and FromExprError::InvalidRegExp variant are removed along with the four call-site error branches in bunfig.rs and ini/lib.rs. The comment in RegularExpression.rs:__bun_regex_compile is updated to document the ordering invariant. Nine new tests cover bun -p (both hoist keys × four expressions) and bun test --isolate with the hoist pattern present.
Security risks
None. This is config-parsing / initialization-ordering logic with no auth, crypto, network, or untrusted-input parsing changes. The removed error path only affected pathological >1 MiB patterns.
Level of scrutiny
Medium. The change touches native code (Rust) with an FFI-owned resource, but the ownership model is straightforward: CompiledRegularExpression wraps the NonNull<()> and its Drop calls __bun_regex_drop; OnceCell drops its contents on drop, so no leak. Thread affinity is unchanged — core::cell::OnceCell is !Sync and the raw pointer was already !Send + !Sync; the PR body confirms is_match is main-thread-only via build_store. I grep-verified the only PnpmMatcher::is_match callers are in isolated_install.rs and prune.rs, both install-command paths that run after the command has (or has not, for bun install) initialized JSC with its own options.
Other factors
- The one behavior change beyond the fix — a pattern that fails Yarr compilation now silently never matches instead of failing config load — is well-justified:
escape_reg_exp_for_package_name_matchingescapes every metacharacter, so only the ~1 MiB Yarr size limit can trigger it, and previously that error fired even forbun -pwhich never uses the pattern. - No stale references to removed types remain (grepped
CreateMatcherError/InvalidRegExpacrosssrc/). - Tests follow harness conventions (
tempDir,bunEnv,test.concurrent.each, drain pipes concurrently, assert stderr/stdout before exitCode) and the PR body confirms they fail on 1.4.0 and pass on the debug build; existing install-pattern suites also pass. - The PR body is unusually thorough, cross-references related PRs (#34869, #39880) and explains why they don't overlap.
|
@robobun sync and resolve conflicts pls |
…ist-pattern-regex # Conflicts: # src/ini/lib.rs # src/install_types/NodeLinker.rs # src/jsc/RegularExpression.rs
|
Synced with main in 2bcef5c. The conflicts were with #39880, which changed the same three files. Resolution:
Checked on the merged debug build in a directory whose bunfig has |
There was a problem hiding this comment.
LGTM — deferring the Yarr compile to first is_match cleanly fixes the JSC-init ordering without changing matcher semantics.
What was reviewed
- Traced
PnpmMatcher::is_matchcallers: onlyisolated_install.rsandprune.rs, both insidebun install, so lazy compile never runs before a JS-running command's ownjsc::initialize. create_matcher/FromExprError::InvalidRegExp/CreateMatcherErrorhave no other consumers; the four call-site simplifications in bunfig/ini are exhaustive, andlog/sourceparams remain live forUnexpectedExpr.__bun_regex_compiledroppingis_one_shot_eval_invocation()fordefault()is safe: it is now only the firstinitializefor install commands, where default is correct.- New tests follow harness conventions (
tempDir, drained pipes, exit-code last,test.concurrent) and assert the observable symptom, not internal state.
Extended reasoning...
Overview
The PR defers hoist-pattern regex compilation from bunfig/.npmrc load time to first match. RegularExpression in src/install_types/NodeLinker.rs now stores the escaped source in a Box<[u8]> and compiles into a OnceCell<Option<CompiledRegularExpression>> inside matches(). create_matcher becomes infallible, so CreateMatcherError and FromExprError::InvalidRegExp are removed along with their handling in bunfig.rs and ini/lib.rs. __bun_regex_compile now passes InitializeOptions::default() since it is only ever the first initialize call for install commands. Nine new tests pin the fixed bun -p and bun test --isolate behavior.
Security risks
None. This is a JSC-initialization ordering fix in the install config loader. No auth, crypto, network, or untrusted-input parsing is touched; escape_reg_exp_for_package_name_matching already neutralizes all metacharacters before the pattern reaches Yarr.
Level of scrutiny
Moderate. It is a real bugfix with a small design decision (lazy init via OnceCell), not a mechanical tweak, but the blast radius is contained: the matcher was already !Send + !Sync via the raw Yarr pointer, is_match runs single-threaded in build_store/prune, and the only behavior change outside the fix is that a >1 MiB hoist pattern now silently never matches instead of failing config load — a reasonable trade for a degenerate input that previously also broke bun -p in that directory.
Other factors
- Verified no remaining references to
CreateMatcherErroror the removedInvalidRegExpvariant;is_one_shot_eval_invocationis still used at its real call site inrun_command.rs. - The PR body lists seven install/npmrc test suites re-run against the debug build, and CI is green (the one Windows failure is a known unrelated flake on main).
- dylan-conway's request to sync/resolve conflicts has been addressed (merge commit
2bcef5c8is HEAD). - Tests are hermetic, use
tempDir, drain both pipes concurrently, and assert output before exit code per repo conventions.
Problem
[install] hoistPatternorpublicHoistPatternin the project'sbunfig.toml,bun -p '1 + 1'printsundefined. Every--printvalue is lost.bun test --isolatethere keeps the default FTL threshold (64000, not 1000000).create_matcher(src/install_types/NodeLinker.rs) compiled each pattern while the config loaded.__bun_regex_compile(src/jsc/RegularExpression.rs) callsjsc::initializefirst, withouteval_modeorshort_lived_globals.JSCInitializeapplies its options once per process, so the command's own call later (run_command.rs:941,TestCommand::exec) did nothing.Fix
RegularExpressioninNodeLinker.rsstores the escaped pattern and compiles it inmatches, through aOnceCell.create_matcherno longer touches JSC. The other hunks remove the error type only the eager compile could produce.is_match(build_store,prune.rs). A command that runs JS has initialized JSC with its own options before then.bun installnever does otherwise, so__bun_regex_compilestill initializes JSC, now with the defaults (Notes on Do not apply the bun -e one-shot startup heuristic to standalone executables #39880).bun run <script>, abun installthat does not reach the isolated linker) no longer initialize JSC when a hoist pattern is configured. And since the escaping makes every metacharacter literal, Yarr rejects a pattern only above its 1 MiB size limit: such a pattern now never matches instead of failing the config load.test/cli/run/run-eval.test.ts(8 new cases) andtest/cli/test/isolation.test.ts(1 new case) fail on bun 1.4.0 and pass here. The install pattern suites pass too (Notes).Background
PnpmMatcheris the include/exclude list behind the bunfig and.npmrchoist patterns. Each entry is a Yarr regex. Run and test commands parse[install]too, so every command builds it.jsc::initializehands per-command options toJSCInitialize, which applies them insidestd::call_once. Later calls are no-ops.JSC::Options::evalModeis a Bun addition to JSC. With it set, the bytecode generator keeps the completion value of module and function bodies.bun -pprints that value.Notes
Layering:
bun_install_typessits belowbun_jsc, so it reaches Yarr through the link-time__bun_regex_*functions defined insrc/jsc/RegularExpression.rs. That is why the JSC initialization lives in__bun_regex_compileand not in the matcher.Probes with bun 1.4.0 in a directory whose
bunfig.tomlhashoistPattern:With this change both print
9/5,evalMode=true, and the threshold is1000000..npmrchoist-patterndid not show the symptom because only install commands read.npmrc, but it builds the same matcher and gets the same lazy compile. The same goes for the global~/.bunfig.toml:Tag::read_global_config(src/options_types/command_tag.rs) is true only for the install family, so a hoist pattern there never affectedbun -porbun test(checked: with the pattern only in$XDG_CONFIG_HOME/.bunfig.toml, 1.4.0 prints2forbun -p '1 + 1').The side effect named in Fix, checked with
BUN_JSC_dumpOptions=1, which prints a header when JSC initializes:bun installin a package with no dependencies and a hoist pattern in.bunfig.tomlprints the header on 1.4.0 and nothing with this change. A self-review pass over the diff found nothing else to change in the code. It pointed out the two description errors fixed above, and suggested two follow-ups that this PR does not need: a JSC-free matcher for these*patterns, and a debug check injsc::initializefor a later call that asks for options the first call did not set.bun -p 5is affected too, not only calls: withevalModeoff, JSC'sBytecodeGeneratorsetsm_defaultAllowCallIgnoreResultOptimizationfor module and function code, andSourceElements::emitBytecodethen never materializes a completion value.The bug dates from the feature (#23567): the Zig
PnpmMatcher.fromExprcalledbun.jsc.initialize(false)eagerly. It is not a regression from the Rust port.The removed error path: a pattern made of every regex metacharacter,
\,/, newlines, and non-ASCII loads fine on 1.4.0, becauseescape_reg_exp_for_package_name_matchingescapes all of them (*becomes.*). A 1.1 million character pattern is the one input that producederror: Invalid regexon 1.4.0 (and that error fired forbun -ptoo, which never uses the pattern). It now loads, and inbun installit never matches.Related: #34869 moves the
bun test -tregex compile afterjsc::initializeintest_command.rs, a different pre-init compile, and does not overlap with this PR. #39880 (merged, synced into this branch in 2bcef5c) madejsc::initializetakeInitializeOptionsand had__bun_regex_compilepassone_shotfrom argv because the compile ran during config load. With the compile moved to the first match, that call is the first one only inside the install commands, where the scan returns false, so it passes the default like the other subcommand callers in #39880. Checked on the merged debug build in a hoistPattern directory:bun -ekeepsuseConcurrentJIT=false,bun -pgetsevalMode=true,bun test --isolategets the 1000000 threshold, andtest/bundler/compile-argv.test.tspasses.is_matchruns on the main thread only (build_storeis sequential), and the matcher types were already!Send + !Syncthrough the raw Yarr pointer, so theOnceCellchanges no thread contract.Suites run with the debug build:
test/cli/run/run-eval.test.ts(45 pass),test/cli/test/isolation.test.ts(28 pass),test/cli/install/public-hoist-pattern.test.ts(14 pass, bunfig and.npmrcpatterns),test/cli/install/isolated-install.test.ts -t hoist(6 pass),test/cli/install/bun-prune.test.ts -t publicHoistPattern(2 pass),test/cli/install/config-precedence.test.ts(51 pass),test/cli/install/npmrc.test.ts(40 pass).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-eval.test.ts