Conversation
…package.json Runtime auto-install resolved every bare import as the `latest` dist-tag even when the project's package.json declared a range for the package. Two things kept the package.json ranges from reaching the auto-installer: - PackageJSON::parse could only parse dependency versions through the AutoInstaller vtable, and the project's package.json is parsed while resolving the entry point, before the first bare import creates the package manager. Without it every dependency was recorded with an uninitialized version, which the resolver treats as "not declared". Parsing now goes through a link-time hook into bun_install that does not need a manager (the manager is only used to record npm: aliases). - The cwd's DirInfo is created by resolvers that have auto-install disabled (the VM transpiler's configure_linker() during init, and the configure_env_for_run transpiler used by `bun run <file>`), before the runtime's --install setting is applied, so the project's package.json was cached without its dependencies at all. The setting now reaches both before they read the directory (InitOptions::global_cache). Also fix the parsed versions to be stored relative to the package.json source buffer, which is what every reader of the dependency map slices them with (specifiers longer than 8 bytes were read from the wrong offset), and pass that buffer through lockfile_resolve so prerelease ranges are compared against the buffer they were parsed from.
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
|
Status: reproduced and fixed, waiting on CI. Reproduced with a local registry serving the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| &self, | ||
| name: &[u8], | ||
| version: &hooks::DependencyVersion, | ||
| version_buf: &[u8], | ||
| ) -> Option<PackageID> { | ||
| self.lockfile | ||
| .resolve_package_from_name_and_version(name, version) | ||
| .resolve_package_from_name_and_version(name, version, version_buf) | ||
| } |
There was a problem hiding this comment.
🔴 resolve_from_disk_cache (called at resolver.rs:3669 for --install=offline) still hardcodes self.lockfile.buffers.string_bytes as the query's group_buf in PackageManagerResolution.rs:198-201, but after this PR the version there can be an Npm range whose prerelease/build tags are offsets into dependencies.source_buf. version_buf is already in scope and is threaded to the sibling lockfile_resolve (:3627) and enqueue_dependency_to_root (:3680) — the trait method / impl / call site should thread it here too.
Extended reasoning...
What the bug is
This PR fixes a class of bug where a DependencyVersion's prerelease/build-tag SemverStrings are sliced with the wrong backing buffer. It threads a new version_buf parameter through AutoInstaller::lockfile_resolve → Lockfile::resolve_package_from_name_and_version so that a query parsed from a package.json's raw bytes is compared using the buffer it was actually parsed against. However, the sibling trait method AutoInstaller::resolve_from_disk_cache was not updated: at src/install/PackageManager/PackageManagerResolution.rs:198-201 it still calls npm_query.version.satisfies(installed_version, self.lockfile.buffers.string_bytes.as_slice(), tags_buf.as_slice()), hardcoding the lockfile string buffer as the query's group_buf.
Code path
load_node_modules (resolver.rs:3021-3033) now clones the version straight out of package_json.dependencies.map, whose SemverStrings are offsets into dependencies.source_buf (the raw package.json bytes — see the new SlicedString::init(source_buf, version_str) in package_json.rs). It passes both version and version_buf = source_buf to enqueue_dependency_to_resolve (resolver.rs:3587, param at :3598). Inside that function, when install_preference == Offline at :3668, it calls pm!().resolve_from_disk_cache(esm.name, &version) at :3669 without version_buf — even though the two adjacent calls in the same function (lockfile_resolve at :3627, which this PR just fixed, and enqueue_dependency_to_root at :3680) both correctly thread version_buf.
Why this PR makes it reachable
Before this PR, the project's package.json was parsed before the PackageManager existed, so every entry was stored with tag == Uninitialized; load_node_modules re-parsed the specifier as the latest dist-tag, and resolve_from_disk_cache returned None at the tag != Npm early-exit (PackageManagerResolution.rs:169) — the wrong buffer was never read for that case. After this PR, r.parse_dependency stores real Npm-tagged versions, so the same range now reaches the satisfies call with offsets computed against source_buf but sliced from lockfile.buffers.string_bytes.
Step-by-step example
package.jsondeclares"pkg": "^1.0.0-alpha.beta.1"(prerelease tagalpha.beta.1is 12 bytes, > 8, so stored as an offset intosource_bufrather than inline).- User runs
bun --install=offline index.jswith nonode_modules;require("pkg")triggers auto-install. enqueue_dependency_to_resolvereceivesversion(tagNpm, comparator prerelease tag = offset intosource_buf) andversion_buf = source_buf.install_preference == Offline, so :3669 callsresolve_from_disk_cache("pkg", &version).Group::satisfies(SemverQuery.rs) uses itsgroup_bufargument to slice the query comparator's prerelease tag; here it receivesself.lockfile.buffers.string_bytes.SemverString::slicebounds-checks and returnsb""on out-of-range, so instead of comparing against"alpha.beta.1"it compares against garbage or the empty string.Tag::order_without_buildproduces a wrongOrdering— e.g. treating the query's prerelease as empty — so a disk-cache version that does not satisfy^1.0.0-alpha.beta.1may be selected, or one that does may be rejected.
Impact
Narrow (offline auto-install with a >8-byte prerelease/build tag in the declared range) and non-crashing (SemverString::slice bounds-checks). But it produces a wrong satisfies() result — bun --offline can load the wrong version — and it is exactly the bug class the PR description names as a fix ("prerelease tags in a range would be read from the wrong buffer"). Per REVIEW.md, "Fix the whole class in the same PR" — sibling sites sharing the pattern are one concern.
Fix
Add version_buf: &[u8] to the AutoInstaller::resolve_from_disk_cache trait method (resolver_hooks.rs), the impl in auto_installer.rs, the free-function shim and inherent method in PackageManagerResolution.rs, and pass it as the second argument to satisfies at :198. Then pass version_buf at the call site resolver.rs:3669. This is mechanically identical to what the PR already did for lockfile_resolve.
| // Like the key above: a value with JSON escapes is | ||
| // decoded outside the source buffer and cannot be | ||
| // stored as an offset into it. | ||
| if !bun_alloc::is_slice_in_buffer( | ||
| version_str, | ||
| package_json.dependencies.source_buf, | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 The new is_slice_in_buffer(version_str, source_buf) guard does continue when a version string contains JSON escapes (e.g. a Windows "file:C:\\\\Users\\\\foo" path), dropping the dependency key from the map entirely — the old code still recorded it (with an Uninitialized version, per the deleted "bun run --filter reads only the map keys" comment), so filter_run.rs:914-920 loses that edge from the workspace ordering graph. Consider inserting with DependencyVersion::default() instead of continue to preserve the key while keeping the buffer-offset guard.
Extended reasoning...
What the bug is
The PR adds a value-side is_slice_in_buffer guard at src/resolver/package_json.rs:909-917 that mirrors the existing key-side guard: when a dependency version string contains JSON escapes (\\, \", \n, \uXXXX), the JSON parser decodes it into a temporary outside source_buf, so the check fails and the loop does continue. This is correct for the version — a SlicedString over an out-of-buffer string cannot be stored as an offset into source_buf — but it also drops the key, which has already passed its own is_slice_in_buffer guard and is perfectly storable.
The specific code path
src/runtime/cli/filter_run.rs:840 calls PackageJSON::parse::<{IncludeDependencies::Main}> directly, and at lines 913-920 it reads only pkgjson.dependencies.map.keys() to build each workspace's deps: Vec<Box<[u8]>> — the values are never touched. Those keys are what the topological sort uses to decide script execution order across --filtered workspaces.
Why existing code doesn't prevent it
Before this PR, the loop unconditionally reached the insert:
- With no auto-installer (the
filter_run.rscase — its transpiler hasglobal_cache = disable), the old code hitNone => Some(DependencyVersion::default()), whose deleted comment explicitly said "bun run --filterreads only the map keys to compute workspace ordering". - With an auto-installer, the old code used
SlicedString::init(version_str, version_str)(wrong buffer for the value — which this PR fixes — but the key still landed in the map).
Either way the key was recorded. Now it is not.
Step-by-step example
- On Windows, a workspace
packages/app/package.jsondeclares"dependencies": { "shared": "file:C:\\\\ws\\\\shared" }.JSON.stringifywrites the backslashes as\\, so the raw file bytes contain\\escape sequences. bun run --filter '*' buildreachesfilter_run.rs:840, which callsPackageJSON::parse::<Main>onpackages/app.- In the dependency loop,
name_str = "shared"passes itsis_slice_in_bufferguard (no escapes in the key).version_str = "file:C:\\ws\\shared"was decoded by the JSON parser into a temporary, sois_slice_in_buffer(version_str, source_buf)returnsfalse→continue. "shared"is never inserted intopackage_json.dependencies.map.- Back in
filter_run.rs:914-920,depsforappis[]instead of["shared"]. - The topological sort no longer knows
appdepends onshared, soapp'sbuildmay run before (or concurrently with)shared's.
Impact
bun run --filter executes workspace scripts in the wrong order when a workspace-to-workspace dependency is spelled with a version string containing JSON escapes. Auto-install itself is unaffected (a missing key and an Uninitialized-tag value both fall back to latest, and file:/link: never auto-installed anyway per the PR's own behaviour note).
The trigger is narrow — realistically only Windows file: paths written with backslashes; workspace:*, ^1.0.0, file:../shared, and forward-slash Windows paths contain no JSON escapes — hence nit. But the deleted comment shows the old fallback was intentional, and REVIEW.md flags "before deleting odd-looking code, git-blame why it was written — it is usually load-bearing."
How to fix
Replace the value-side continue with an insert using DependencyVersion::default() (the Uninitialized tag), preserving the key for --filter while keeping the buffer-offset guard:
if !bun_alloc::is_slice_in_buffer(version_str, package_json.dependencies.source_buf) {
// Value has JSON escapes and cannot be stored as an offset into
// source_buf; still record the key so `bun run --filter` sees the edge.
let dependency = Dependency {
name,
version: DependencyVersion::default(),
name_hash,
behavior: group.behavior,
};
// ... same put_assume_capacity_context as below ...
continue;
}
|
Updated 12:25 PM PT - Aug 13th, 2026
✅ @robobun, your commit 1f2deae162c3cb45ef4c76e8db4d158db5905b4e passed in 🧪 To try this PR locally: bunx bun-pr 38198That installs a local version of the PR into your bun-38198 --bun |
1 participant
Problem
bun index.jswith nonode_modules) installs thelatestdist-tag of a bare import even when the project'spackage.jsondeclares a range for it. With"no-deps": "^1.0.0"and a registry whoselatestis2.0.0,require("no-deps")loads2.0.0; a range nothing satisfies silently installslatesttoo. docs/runtime/auto-install.mdx ("Version resolution", step 2) documents the range being used.src/resolver/package_json.rs:915(before this change): dependency versions were only parsed through theAutoInstallervtable. The project'spackage.jsonis parsed while the entry point is resolved, before the first bare import creates the package manager, so every entry was stored with anUninitializedversion.load_node_modules(src/resolver/resolver.rs:3041) treats that as "no range declared" and re-parses the specifier aslatest. This is a regression from the Rust port (bun 1.3.14 parsed the versions here without a package manager); it is what the second cause left visible.DirInfois created before the runtime's--installsetting is applied: by the VM transpiler'sconfigure_linker()insideVirtualMachine::init(src/runtime/jsc_hooks.rs:512, beforebootcallswire_transpiler_from_ctx), and forbun run <file>earlier still by theconfigure_env_for_runtranspiler. Both have auto-install disabled (BundleOptions::from_apidefault), anddir_info_uncached(src/resolver/resolver.rs:6431) only parses dependencies when the resolver creating the entry has it enabled, so the project'spackage.jsonwas cached with no dependencies at all and reused from the process-wide cache by the runtime resolver. This part predates the port: bun 1.3.14 only honored the range when thepackage.jsonwas in a directory below the cwd.package_json.rsbuilt theSlicedStringover the version string itself, so any specifier longer than the 8-byte inline form (>=1.0.0 <1.1.0,npm:foo@^1.0.0, prerelease tags) was stored as an offset relative to the wrong base; every reader slices the map withdependencies.source_buf.Lockfile::resolve_package_from_name_and_versioncompared the query against the lockfile's string buffer even when the query was parsed from apackage.json, so prerelease tags in a range would be read from the wrong buffer.Fix
PackageJSON::parseparses versions throughResolver::parse_dependency, which uses the package manager when it exists and otherwise a new link-time hook__bun_resolver_parse_dependency(bun_install::auto_installer, same mechanism as__bun_resolver_init_package_manager) that callsdependency::parsewith no manager. Correct because the manager's only role inparseis recordingnpm:aliases, andlockfile_append_from_package_jsonrecords those anyway when the map is cloned into the lockfile; this restores what the pre-port code did.VirtualMachine::InitOptionsgainsglobal_cache(defaultdisable, the previous value), applied ininit_runtime_statebeforeconfigure_linker(), the same waystore_fdalready is;bootand the REPL passctx.debug.global_cache.configure_env_for_run_implapplies the same setting before it reads the directory. Correct because the setting only changes whether apackage.json's dependencies are recorded when its directory is cached; these resolvers never auto-install themselves (Resolver::resolvepassesGlobalCache::disableper call), andwire_transpiler_from_ctxsets the same value afterwards. Workers,bun test, compiled executables andBun.buildkeep the default.SlicedStringfor a version is built overdependencies.source_buf, with the same in-buffer guard the key already has (a value with JSON escapes is decoded outside the buffer and is skipped, as escaped keys are).AutoInstaller::lockfile_resolveandresolve_package_from_name_and_versiontake the buffer the version was parsed against; the resolver already tracks it (string_buf).workspace:,link:,file:, git) now fails withCannot find packageinstead of auto-installing whatever the registry has under that name. This only arises with nonode_modulesanywhere above the file; the old behaviour was the same as for an unsatisfiable range, i.e. a different package than the one declared.test/cli/run/run-autoinstall.test.ts(newdescribeblock, local registry serving theno-depsfixture):bun <file>andbun run <file>from the project directory, a project directory below the cwd, a namelesspackage.json, a range longer than the inline form, an unsatisfiable range, annpm:alias, and a package thepackage.jsondoes not list (stilllatest). All but the last fail on the unfixed build (every one installs2.0.0); 20/20 pass with the debug build.test/cli/run/filter-workspace.test.ts(the other consumer of this dependency map),test/js/bun/resolve/,test/js/bun/repl/repl.test.ts,autoinstall-cached-manifest,run-autoinstall-abs-path,run_command,workspaces,multi-run,self-reference,env.devDependencies/optionalDependenciesare still not read by this parser (it looks the sections up by the wrong key); that is a separate pre-existing bug affecting--filterordering as well and is tracked separately.Background
node_modules, the runtime resolver (load_node_modules) lazily creates aPackageManagerand installs the package into the global cache. To pick a version it looks atdir_info.package_json_for_dependencies, the nearest cachedpackage.jsonwhose dependency map is non-empty; if the import is listed there, that entry's parsed version is what gets resolved, otherwise the specifier is parsed as thelatestdist-tag.DirInfocache: every resolver in the process shares one cache of directory entries (DirInfo), each holding the interned parse of that directory'spackage.json. Whichever resolver touches a directory first decides what the cached entry contains;bun runcreates several transpilers (and so several resolvers) before the runtime one.GlobalCache/global_cache: the auto-install mode from--install(autoby default,disablefor bundlers and tests).Resolver::use_package_manager()reads it to decide whether to record apackage.json's dependencies at all; the per-call argument toresolve_and_auto_installdecides whether a given resolution may install.bun_resolver/bun_installlayering:bun_installdepends on the resolver crate, so the resolver reaches install-tier code either through theAutoInstallertrait object it is handed once a manager exists, or throughextern "Rust"functions defined with#[no_mangle]inbun_installand resolved at link time.SemverString/SlicedString: dependency names and versions are stored as 8 bytes, either the string inline (up to 8 bytes) or an offset+length into a buffer supplied when the string was created; reading one back requires the same buffer.SlicedStringpairs that buffer with the sub-slice being parsed.