install: resolve file: dependencies loaded from the lockfile relative to the top-level dir - #38850
install: resolve file: dependencies loaded from the lockfile relative to the top-level dir#38850robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 3 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 ignored due to path filters (1)
📒 Files selected for processing (6)
Comment |
|
Status: reproduced on bun 1.4.0 and on main with the script in the PR description (a file: package declaring |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds a normalization pass to both lockfile loaders and coordinates with two other open PRs (#38814, #38816) touching the same code, a human look would still be worthwhile.
What was reviewed:
- The
parse_dependencyrefactor is behavior-preserving — same join/relative sequence, same args; the extra copy intobufon non-Windows is inert (relative()writes to a distinct thread-local buffer, socopy_from_slicedoes not overlap). rebase_folder_dependenciesre-slicesstring_bytesper iteration and only mutates it viaBuf::appendafter the borrowed slices are dead;SemverStringhandles stay valid across the reallocation.?propagation compiles at both call sites (oom_from_alloc!(ParseError)andcrate::Error::Alloc(#[from])).- The five new tests cover both lockfile formats, the no-
..misresolve,bun update <name>, and the workspace case, and each asserts the literal is preserved alongside the rebased resolution.
Extended reasoning...
Overview
The PR fixes a resolution bug for file: dependencies declared by file:/workspace packages when re-enqueued from a loaded lockfile (override removal, bun update <name>). It extracts folder_relative_to_top_level_dir from Package::parse_dependency and adds Lockfile::rebase_folder_dependencies, called at the end of both the text (bun.lock) and binary (bun.lockb) loaders, so loaded folder-dependency rows carry the same top-level-relative value.folder that a fresh parse would produce. Five new tests in test/cli/install/bun-lock.test.ts cover the variant matrix.
Security risks
None identified. The change only rewrites an in-memory derived path field for locally declared file: dependencies; the on-disk literal is untouched, and lockfile round-trip is byte-identical (verified by --frozen-lockfile in the tests). No new untrusted input surface, no path-traversal exposure beyond what parse_dependency already had.
Level of scrutiny
Medium-high. Lockfile loading runs on every bun install, so a mistake here affects every project. The refactor of parse_dependency was checked line-by-line against the original: same join base and parts, same empty-path handling, same Windows posix-conversion; the only difference is that non-Windows now copies the relative result into the caller buffer before returning, which is semantically identical because the caller immediately appends it. resolve_path::relative writes into RELATIVE_TO_COMMON_PATH_BUF (thread-local), not the caller's buffer, so the copy is non-overlapping.
Other factors
The PR description explicitly flags two overlapping open PRs (#38814 restricts the resolver, #38816 restricts the parse-time rebase to the same resolution set this pass covers) and one intentionally-out-of-scope adjacent bug (root-declared file:../x after override removal). A maintainer should confirm the merge order and that skipping registry/git/tarball parents (_ => continue) matches what Package::from_npm produces, as the description asserts. Test coverage is thorough — both lockfile formats, the silent-wrong-directory case (path without ..), and the workspace path — and each test asserts the literal survives unchanged in the saved lockfile.
|
On the one point left open above, the
Merge order between this and #38814 / #38816 does not matter functionally; only #38816 shares lines with this PR (the |
…o the top-level dir Package::parse stores a folder dependency declared by a root, workspace or file: package relative to the top-level dir, and the resolver relies on that. A lockfile only keeps the declared literal, so rows loaded from bun.lock, bun.lockb or a migrated lockfile came back relative to the declaring package instead. Rows are normally not resolved again, but removing an override for the dependency's name or running `bun update <name>` re-enqueues them, and the path was then looked up from the wrong directory: a `..` path failed with "Could not find package.json", a path without `..` silently resolved against the project root. Lockfile::load_from_dir now derives the value of every such row from its literal and the declaring package's directory, with the helper Package::parse uses, once the lockfile has been read or migrated. The literal is untouched, so the lockfile is written back unchanged. The pnpm migration wrote a folder package's file: rows with pnpm's lockfile-relative directory as the literal (`file:sub-dep/child` for a declared `file:./child`); it now writes them relative to the declaring package like bun.lock expects.
afcda4d to
0939279
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Restructured since the first round of review, description updated accordingly:
|
There was a problem hiding this comment.
The earlier nit (unread stdout pipe in migrate.test.ts) is fixed in 32f025d, and this run found no further issues. Because the change adds a rebase pass to Lockfile::load_from_dir (runs on every install), rewrites what the pnpm importer serializes for folder-package file: literals, and picks one of two considered placements for the fix while three adjacent PRs (#33106, #38814, #38816) are touching the same resolver arm, a maintainer familiar with the install subsystem should confirm the approach.
Checked: folder_relative_to_top_level_dir is a straight extraction of the existing Tag::Folder arm and resolve_path::relative writes into a thread-local scratch buffer, so the trailing buf.copy_from_slice never overlaps.
Checked: the pass only mutates value.folder (never the literal), so --frozen-lockfile byte-identity holds — the tests assert this for bun.lock, bun.lockb and the migration paths.
Checked: the pnpm literal rewrite runs after resolve(), so pnpm's own dep-path keys (which are lockfile-relative) are still matched before the rewrite.
Extended reasoning...
Overview
This PR fixes file: dependencies of file:/workspace packages failing to resolve when they are re-enqueued from a loaded lockfile (override removed, or bun update <name>). It adds Lockfile::rebase_folder_dependencies (called from load_from_dir after every successful lockfile read/import), extracts folder_relative_to_top_level_dir from the existing parse_dependency Folder arm, and adds declare_folder_dependencies_relative_to_their_package to the pnpm importer so its literals match bun.lock's package-relative convention. ~130 lines of new Rust across three files in src/install/, plus seven new tests and one snapshot line updated.
Security risks
None identified. The change is path re-derivation for local file: folder dependencies; no network, auth, or untrusted-input parsing is involved. Path traversal is not a new surface — the same join_abs_string_buf_checked + relative computation already ran in parse_dependency.
Level of scrutiny
High. load_from_dir runs on every bun install; a regression here affects every project with a lockfile. The pnpm importer change alters what is written to bun.lock on disk. Both new functions carry #[cfg(windows)] branches. The PR description explicitly weighs this placement against fixing in the resolver's Folder arm and defers to the existing convention — that is a design call a maintainer should ratify, especially with three other PRs modifying the same arm concurrently and a documented open edge case (root-declared file:../x after override removal) left for a follow-up.
Other factors
The previous inline nit from this bot was addressed. Test coverage is thorough (bun.lock/bun.lockb, path with and without .., workspace member, bun update, package-lock.json migration, pnpm re-resolve; --frozen-lockfile and byte-identical re-save asserted). The author confirmed cargo check for the Windows target and ran the wider install/migration suites. No CODEOWNERS entry covers src/install/. The github-actions comment-cop flags on the doc comments were answered by the author (they document an invariant, not a workaround) and the comments were shortened; that thread does not need further action here.
|
Review threads are all answered and resolved (the comment-cop ones point at doc comments that have since been shortened; the unread-stdout nit is fixed in 32f025d). Nothing further planned from my side; this is ready for a maintainer to look at the placement question described under "Alternative considered" in the description. Buildkite build for 32f025d is running. |
1 participant
Problem
file:package in the project declares its ownfile:dependency relative to itself (vendor/a/package.jsonhas"b": "file:../b"). Install once with a root override forb, remove the override, and everybun installafterwards fails until bun.lock is deleted:"b": "file:../../vendor/b", forbun update bin the first setup (no overrides involved), forbun update brun directly against a package-lock.json that is being migrated, and with bun.lockb instead of bun.lock. Reproduces on bun 1.4.0 and main.bun update <name>started re-resolving transitive rows in install: honor --recursive/--filter in non-interactive bun update; re-resolve every named-update target #36360 / install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333, so that trigger is new...(vendor/adeclares"b": "file:./b") nothing fails:bis resolved against the project root instead, bun.lock recordsb@file:./b, and whatever sits at<project>/bis installed.Package::parse_dependency(src/install/lockfile/Package.rs,Tag::Folderarm) stores a folder dependency'sVersion.value.folderrelative to the top-level dir, and the resolver's Folder arm (src/install/PackageManager/PackageManagerEnqueue.rs:2619) assumes that form. A lockfile only stores the declared literal, and every loader keeps it as is, i.e. relative to the declaring package: bun.lock (parse_append_dependenciesviadependency::parse), bun.lockb (Version::to_version) and the package-lock.json / yarn.lock importers. Loaded rows are normally never resolved again, so this only surfaces when something re-enqueues them: the overrides-changed loop in src/install/PackageManager/install_with_manager.rs:531 andenqueue_named_updates(bun update <name>). A..in the verbatim path then trips the transitive folder escape check; a path without..is joined onto the wrong directory.file:rows it wrote pnpm's lockfile-relative directory as the bun.lock literal ("nested-child": "file:sub-dep/child"wheresub-dep/package.jsondeclaresfile:./child), which is not what bun.lock means by a literal. That code is a day old (install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333), so no released bun has written such a lockfile.Fix
Lockfile::rebase_folder_dependencies(src/install/lockfile.rs): for every package whose directory the lockfile records (Root,Workspace,Folderresolutions), re-derive eachFolderrow'svalue.folderfrom its literal and that directory, withfolder_relative_to_top_level_dir, the computation lifted out ofparse_dependency.load_from_dirruns it once on every successful load, so bun.lock, bun.lockb and the three importers all hand the resolver rows in the shape a package.json parse produces; the old body ofload_from_diris nowread_from_dir.declare_folder_dependencies_relative_to_their_package, after its own resolution pass, which keys on pnpm's form). With thev9-alias-non-registry-dep-pathfixture the migrated row becomes"config": "file:../shared/config", which is exactly whatouter/package.jsondeclares.Version::eqlcompares folder rows by literal), so it has to stay the declared, package-relative path;value.folderis derived from it, and the resolver needs the derived form. The pass touches onlyvalue.folder, never the literal, so it is idempotent and lockfiles are written back byte for byte (--frozen-lockfileand byte-identical re-saves are asserted in the tests). Rows of registry, git and tarball packages are skipped:Package::from_npmleaves theirs verbatim and the installer resolves those against the installed declaring package (src/install/PackageInstaller.rs:1850), so the loaded form already matches for them.file:../xalso fails, for a different reason (the root's previous rows are re-enqueued after the differ replaces them); tracked separately...,bun update <name>, a workspace member), test/cli/install/migration/migrate.test.ts (bun update bin the same run as a package-lock.json migration, using the lockfilenpm install --package-lock-onlywrites for the layout) and test/cli/install/migration/pnpm-lock-v9.test.ts (migrated literal, thenbun update nested-childagainst the migrated bun.lock; updated literal expectation and snapshot for the alias fixture). All of them fail on bun 1.4.0 / without the src changes and pass with them.cargo check --target x86_64-pc-windows-msvcfor the install crate passes (both new functions have a Windows branch).Background
file:<dir>(or a bare path).Dependency.versioncarries theliteralas written plus a derivedvalue.folder. Lockfile writers emit only the literal.bun installruns in. A folder package'sResolution::Folderis its directory relative to it (a@file:vendor/ain bun.lock), a workspace package's resolution is its workspace path, and the root's directory is the top-level dir itself. Those are the directories the pass joins the declared paths onto.bun installdiffs package.json against the loaded lockfile and re-resolves only rows that changed, plus every row whose name an overrides change affects and the rowsbun update <name>targets. All other rows keep their lockfile resolution, which is why loaded rows could stay in the wrong form unnoticed.migration::detect_and_load_other_lockfileand returned through the sameload_from_diras bun.lock, so the pass covers them too. package-lock.json and yarn.lock record the declared specs; pnpm-lock.yaml records resolved, lockfile-relative references, hence the importer-side rewrite.Repro script
mkdir -p app/vendor/{a,b,b2} && cd app echo '{"name":"a","version":"1.0.0","dependencies":{"b":"file:../b"}}' > vendor/a/package.json echo '{"name":"b","version":"1.0.0"}' > vendor/b/package.json echo '{"name":"b","version":"2.0.0"}' > vendor/b2/package.json echo '{"name":"app","dependencies":{"a":"file:./vendor/a"},"overrides":{"b":"file:./vendor/b2"}}' > package.json bun install # ok echo '{"name":"app","dependencies":{"a":"file:./vendor/a"}}' > package.json bun install # error: Could not find package.json for "file:../b" dependency "b" # without overrides: rm bun.lock && bun install && bun update b # same error # straight from a package-lock.json: rm bun.lock && npm install --package-lock-only && bun update b # same errorAfter the fix every command exits 0 and bun.lock contains
"a/b": ["b@file:vendor/b", {}]whilea's row still reads"b": "file:../b".Earlier shape of this PR
The first version ran the pass inside the bun.lock and bun.lockb parsers only and rebased the existing
value.folderrather than re-deriving it from the literal. Review of that version pointed at the importers: package-lock.json rows have the same problem in the run that migrates them (reproduced; yarn.lock rows go through the same kind of code), and the pnpm importer's rows were already top-level relative, so a pass keyed on the value would have double-applied to them. Deriving from the literal, running it once inload_from_dir, and fixing the pnpm literals covers all five producers.