Conversation
… installed package A root overrides/resolutions rule pointing a dependency at a file: folder resolves to a Resolution::Folder row under whatever package declares the dependency. The hoisted installer picked the directory to read such a row from by the kind of tree it was installing into, so under a registry, tarball or git package it looked for the override's project-relative path inside that package, swallowed the ENOENT as "nothing to link" and reported the install as successful with the folder missing. Install a row selected by an override rule from the top-level dir, the way rows declared by a local file: package already are. A missing target now fails the install with the existing "Could not find folder" error.
|
Warning Review limit reached
Next review available in: 2 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 (4)
Comment |
|
Status: reproduced on 1.4.0-canary (eabb96d) and on main with the repro in the PR description (local tarball With this branch the folder is linked for plain and nested rules, a missing target fails the install, and the new tests in CI (build 97913, still finishing): the install tests pass on every lane that has reported, including Windows x64. The one red test so far is |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and the reasoning is sound, but since it changes install-time path-base selection in the hoisted linker and turns a previously-silent ENOENT into a hard failure, a human look would still be worthwhile.
What was reviewed:
- Verified
is_overridden_dependencyuses the same per-edgeOverrideMap::getlookup the resolver applies, so scoped rules (pkg>sub,sub@^1) match only their edges and don't redirect an unrelated package's ownfile:dependency of the same name — pinned by thefile-depguard test. - Confirmed the outer
resolution.tag == Tag::Folderguard means a matching override that isn't itself afile:rule (e.g. a semver override) never reaches this branch, so the project-relative base is only used when the override actually supplied the folder path. - Checked the ENOENT-tolerance fallback still applies to non-overridden transitive folder rows declared by installed packages; only override-supplied paths now fail loudly on a missing target.
Extended reasoning...
Overview
This PR fixes silent failure of file: overrides applied to dependencies of installed (registry/tarball/git) packages under the hoisted linker. Production change is ~10 LOC: a new Lockfile::is_overridden_dependency(id) helper that wraps OverrideMap::get, and one added disjunct in PackageInstaller.rs so the transitive-folder branch installs from the project root when the row was supplied by a root override rule. ~180 lines of tests across bun-install.test.ts and bun-install-registry.test.ts cover plain and nested/scoped rules, both overrides and resolutions, fresh install / re-install / --frozen-lockfile, isolated-linker parity, the missing-folder error path, and a per-edge guard test proving a scoped override for a different dependent leaves a registry package's own file: dependency untouched.
Security risks
None identified. The change does not relax the existing unsafe-path escape check at PackageInstaller.rs:1486-1497 (that check remains gated on contains_name and is orthogonal to which base directory the folder is opened from). Override rules are user-authored in the root package.json, so trusting their file: targets as project-relative is consistent with how the resolver and the isolated linker already treat them.
Level of scrutiny
Medium-high. bun install is a production-critical path exercised by every user, and the fix hinges on a non-obvious invariant: for a transitive Resolution::Folder row, the presence of a matching override on that edge implies the override supplied the path (because a folder resolution can only reach a non-workspace tree via the declaring package's own manifest or via a root rule, and if the rule matched, it was applied). I traced this through OverrideMap::get and the outer Tag::Folder guard and it holds — but it is exactly the kind of layered reasoning a maintainer familiar with the resolver should sanity-check. The PR also converts a previously-tolerated ENOENT into a hard install failure for override targets, which is the right call but a user-visible behavior change.
Other factors
Test coverage is thorough and follows harness conventions (dummy registry, runBunInstall, concurrent tests, drained pipes, exit code asserted after output). The PR description is exceptionally detailed and self-documents interactions with in-flight PRs #38816 (comment reword, trivial rebase) and #38856 (isolated linker should adopt this per-edge helper over contains_name). No CODEOWNERS on src/install/. No prior human review comments to address. Given the critical path and subtle invariant, deferring rather than auto-approving.
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 5095866 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38994That installs a local version of the PR into your bun-38994 --bun |
Problem
overrides/resolutionsrule such as"sub": "file:./vendor/sub"applied to a dependency of a registry, tarball or git package is silently not installed:bun installexits 0 and reports the package installed,node_modules/<pkg>/node_modules/stays empty, andrequire("<pkg>")fails withCannot find package 'sub'.bun.lockis right ("pkg/sub": ["sub@file:./vendor/sub", {}]), and the isolated linker installs the same project correctly. Reproduces on 1.4.0 and main; nested rules ({"pkg": {"sub": "file:..."}},"pkg/sub","sub@^1") behave the same.src/install/PackageInstaller.rs, transitive folder branch. AResolution::Folderstring has one of two bases (see Background), and the installer picks the base from the tree it is installing into: the project dir when the declaring package is a localfile:package (install: link a transitive file: dependency of a local file: package #33159), otherwise the declaring package's own directory, where an ENOENT is rewritten toInstallResult::Successbecause published packages may declare folders they do not ship. An override's path is written in the root package.json, so it is project-relative whatever package declares the dependency; under an installed package the installer openednode_modules/pkg/./vendor/sub, got ENOENT, and counted the row as installed. The resolver already classifies these rows as root-authored (PackageManagerEnqueue.rs, Folder arm, lets them through the escape check), so they reach the installer.Fix
Lockfile::is_overridden_dependency(id): is there a root rule, plain or scoped, for this dependency edge (OverrideMap::get, the lookup the resolver applied to it).Could not find folder "file:./vendor/sub" for dependency "sub"error instead of exiting 0.file:dependency of the same name (pinned by a test); afile:path can only be a transitive Folder row through the declaring package itself or through a rule, so a rule on the edge means the rule wrote the path.OverrideMap::contains_name(plain rules only) is not used here because it answers the trust question for the escape check at the name level; the path base question is per edge and has no trust component, since an escaping path from a scoped rule is rejected while resolving.node_modulesproduced by the old behavior is repaired by the nextbun install: the row verifies by the presence of its directory, which the old build never created.test/cli/install/bun-install.test.ts, in the existingresolutions/overridesloop:links a <plain|nested> "<field>" file: rule applied to a registry package's dependency(dummy-registryis-evenrequiringis-odd, rule pointingis-oddatvendor/is-odd; asserts the registry is never asked foris-odd, the lockfile row, the layout underis-even/node_modules, and thatrequire("is-even")runs the vendored copy, after a fresh resolve, an install on top of it, and a--frozen-lockfileinstall from the lockfile),fails when a file: override for a registry package's dependency names a missing folder(error text, exit 1, nothing linked), andisolated linker: ...for both rule shapes, which pass before and after and pin the parity. The four hoisted tests and the missing-folder test fail on main ([]where["is-odd"]is expected; exit 0 where 1 is expected).test/cli/install/bun-install-registry.test.ts,transitive file dependencies:a root override redirects a registry package's own file: dependency to a folder in the project(file-dep@1.0.0declaresfiles: file:./the-files; the override wins and the vendored folder's entries are what gets linked, fresh and frozen; fails on main with ENOENT) anda scoped override for another dependent leaves a registry package's own file: dependency alone(passes before and after; guards the per-edge predicate).bun-install.test.ts -t "file:|folder|override|resolutions|transitive"(38 pass), all oftransitive file dependenciesinbun-install-registry.test.ts,nested-overrides.test.ts(144 pass),overrides.test.ts, and the override /file:subset ofisolated-install.test.ts.is_folder_tree_iddoc comment above the new helper; whichever lands second needs a trivial rebase. install: link file: dependencies of registry packages from inside the package with the isolated linker #38856 (isolated linker) currently keys the same base decision oncontains_name, so under it a nested rule would regress the isolated parity test added here; the per-edge helper from this PR is the drop-in replacement.Background
file:dependency on a directory resolves toResolution::Folder, whose payload is a path string with one of two bases. Manifests that live in the project (root, workspaces, localfile:packages, and the root'soverrides/resolutions) produce project-relative paths; a registry manifest (Package::from_npm) stores the path as declared, relative to the package that declared it, because that folder only exists inside the installed package.node_modulesof the package that depends on it, which is why the row shows up aspkg/suband why the installer has to know which base a given row uses.OverrideMapholds the root's rules: plain rules ("sub": ..., apply to every edge of that name) and scoped rules ("pkg>sub",{"pkg": {"sub": ...}},"sub@^1", apply to some edges).OverrideMap::get(lockfile, dependency_id, name_hash)returns the rule the resolver applies to one edge;contains_nameonly reports plain rules and is what the escape checks use to trust a path.installer.cache_diris the directory the source path is opened relative to.Repro (no network, 1.4.0-canary and main)
With this change the same commands link
node_modules/pkg/node_modules/sub/{index.js,package.json}tovendor/suband printvendor sub; the same holds for"overrides": {"pkg": {"sub": "file:./vendor/sub"}},"resolutions": {"pkg/sub": ...}and"overrides": {"sub@^1.0.0": ...}, and pointing the rule at a missing folder printserror: Could not find folder "file:./vendor/missing" for dependency "sub"and exits 1.