Conversation
…ncy groups when parsing a manifest PackageManifest::parse sliced bundled_deps_buf once per dependency group and assigned the slice to package_version.bundled_dependencies on every non-peer group, so a version with both dependencies and optionalDependencies kept only the bundled names declared in optionalDependencies, and names declared in peerDependencies were never kept. Capture the buffer offset once per version and assign the range written across all groups after the group loop. Adds the bundled-with-optional and bundled-peer registry fixtures and tests for both layouts in bun-install-registry.test.ts.
|
Warning Review limit reached
Next review available in: 21 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 (6)
Comment |
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 1547eeb has some failures in 🧪 To try this PR locally: bunx bun-pr 38857That installs a local version of the PR into your bun-38857 --bun |
|
Status: ready for review. Reproduced on the current release with the two fixtures added here ( Current revision (after review): the parser stores the CI: on both the previous sha and the current one (1547eeb), every lane that ran passed (177 jobs each time, no failing tests; the only annotations are retried tests unrelated to this change). The two |
![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. The fix is small and I've verified it's consistent with the counting pass, but since it changes install behavior (bundled peers now honored from manifests) and consciously skips a manifest cache-version bump, a human sign-off would still be worthwhile.
What was reviewed:
- Confirmed the counting pass at
npm.rs:2176-2194already sizesbundled_deps_bufacross all three groups, so hoistingbundled_deps_beginand assigning once after the loop cannot overrun the buffer. - Checked that
bundled_deps_set.swap_remove(line 2697) already ran for peers before this change — the buffer was being written, just never published — and thatswap_removeprevents duplicate hashes when a name appears in multiple groups. - Checked the now-unconditional assignment: when no group has items,
bundled_deps_begin == bundled_deps_offsetyields an empty list (matching the previous default), andbundle_all_depsis reset per version soINVALIDhandling is preserved.
Extended reasoning...
Overview
The PR fixes PackageManifest::parse in src/install/npm.rs so that a version's bundled_dependencies list covers names matched from all dependency groups instead of only the last non-peer group iterated. The change hoists let bundled_deps_begin = bundled_deps_offset; from inside the per-group loop to just before it, removes the per-group if !is_peer { ... } assignment block, and adds a single unconditional assignment after the loop. Two new registry fixtures and four new test cases (two tests × bun.lock/bun.lockb) are added to bun-install-registry.test.ts.
Security risks
None. This is dependency-graph bookkeeping in the manifest parser — no untrusted-input parsing surface changes, no new syscalls or allocations. The buffer write at line 2700 is unchanged; I verified the counting pass at lines 2176–2194 already iterates all DEPENDENCY_GROUPS (including peers) when sizing bundled_deps_count, so the hoisted range cannot exceed the pre-sized buffer.
Level of scrutiny
Medium-high. The code change itself is mechanical and I'm confident it's correct — the counting pass and build pass were already symmetric across groups; only the publishing of the slice was per-group. But this is the package-manager manifest parser, executed on every registry install, and the PR carries two decisions a human should acknowledge: (1) bundled names declared only in peerDependencies are now honored from manifests (a behavior change, though it aligns with the tarball/git/folder path and bundleDependencies: true), and (2) the manifest cache header is deliberately not bumped, so users with stale cached manifests keep the old bundled list until the packument changes.
Other factors
The PR description is unusually thorough — it names the root cause, cites the sibling code paths (Package::from_npm, the package.json parser, bun pm pack, npm arborist) that already treat bundleDependencies as group-agnostic, and lists the test suites that were re-run. Tests follow the existing bundledDependencies describe pattern, assert exact node_modules layout and lockfile contents, and re-verify with --frozen-lockfile after wiping node_modules. No prior human review comments, no CODEOWNERS on this path, and the bug hunting system found nothing.
…manifest cache to v0.0.8 The manifest parser no longer filters bundleDependencies against the dependency groups while it walks them; it writes the hash of every name in the array once per version, and Package::from_npm keeps doing the matching against the dependencies of every group. This removes the per-group slicing that lost names, the StringSet used to filter them, and the raw write inside the dependency loop. The cache header moves to v0.0.8 so .npm entries written with the old lists are re-parsed instead of being reused on a 304. The bundled-with-optional fixture now also lists a name that no group declares, which must have no effect.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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 changes how the manifest parser stores bundleDependencies (with a user-visible behavior change for bundled peers from registry manifests) and bumps the manifest cache header, a human look would still be worthwhile.
What was reviewed:
- Counting-pass and build-pass agreement — both filter on
as_str().is_some(), sobundled_deps_bufis sized exactly and the indexed write cannot go past the end. Package::from_npmis the only reader ofbundled_dependenciesand matches byname.hash(sameBuilder::string_hash), so unfiltered extra names andnot-a-dependencyare harmless.Boolean(true)→INVALIDstill maps toall_dependencies_bundled();false/absent falls through todefault()as before viaempty_version.StringSetimport removal — no remaining use innpm.rs.
Extended reasoning...
Overview
This PR fixes PackageManifest::parse in src/install/npm.rs: previously each dependency group (dependencies, optionalDependencies) overwrote package_version.bundled_dependencies with only the slice it produced, so bundled names declared in dependencies were lost when optionalDependencies was also present, and bundled names declared in peerDependencies were never recorded. The fix stores the bundleDependencies array as-published (one name hash per string entry) once per version before walking the groups, and drops the per-group filtering entirely — Package::from_npm already does the name-hash match against every group. The manifest cache header is bumped to v0.0.8 so warm caches with the old buffer contents are re-parsed. Two new registry fixtures and four new tests (bun.lock × bun.lockb) exercise both bug shapes plus a bundled name declared in no group.
Security risks
None identified. The parser reads registry JSON into a pre-sized buffer; the counting pass and build pass use the same predicate so the buffer cannot be overrun. No new syscalls, no new external inputs, no auth or crypto touched.
Level of scrutiny
Medium-high. bun install manifest parsing is a production hot path used by every registry install, and this change intentionally alters behavior: a bundled name declared only in peerDependencies is now marked BUNDLED when the package comes from a registry manifest (previously it was not). The PR description argues this aligns the registry path with the tarball/git/folder path and with npm's arborist, which I verified against Package.rs — but a maintainer should confirm they want that alignment. The cache-header bump is also a one-time cost for every user's .npm cache on upgrade.
Other factors
The change is well-argued, the tests are strong (they check node_modules layout, the lockfile snapshot, and a --frozen-lockfile reinstall, and were shown to fail on the current release), and the code is simpler than what it replaces (net −46 lines, drops StringSet, an unsafe block, and two long explanatory comments). The two comment-cop bot flags were addressed in 1547eeb. I am deferring rather than approving because the behavior change for bundled peers and the cache invalidation are decisions a maintainer should explicitly sign off on, not because I found anything wrong.
|
#43125 carries the parser change of this PR, rebased onto current main: |
This branch has not been deployed
1 participant
Problem
bundleDependenciesplus bothdependenciesandoptionalDependenciesloses the bundled names declared independencies:bun installdownloads them from the registry and hoists them next to the copies the tarball already ships, and bun.lock records them as ordinary entries instead of"<parent>/<name>"with"bundled": true.peerDependenciesis never treated as bundled from a manifest, so the peer is installed from the registry even though the tarball ships it.PackageManifest::parse(src/install/npm.rs) filtered thebundleDependenciesarray against the dependency groups while walking them, and each non-peer group assignedpackage_version.bundled_dependenciesto just the slice ofbundled_deps_bufit had written, so theoptionalDependenciesgroup replaced what thedependenciesgroup produced, and the peer group wrote names that nothing published (the!is_peerguard from install: global virtual store for isolated linker (7x faster warm installs) #29489 only stopped it from replacing the list with an empty slice).bundle(d)Dependenciesinbun install#16055); not a port regression.Fix
bundleDependencies: truestill storesINVALID). The per-group filtering (bundled_deps_set), the per-group slice assignment and the raw write inside the dependency loop are gone; the counting pass sizes the buffer from the array length.Package::from_npm(src/install/lockfile/Package.rs, the loop overbundled_dependencies) is the only reader of the list and already matches it against the dependencies of every group by name hash. The parser's filtering was redundant, and it was the mechanism that lost names. The stored hashes come fromBuilder::string_hash, the function that also hashes the dependency namesfrom_npmcompares against. A listed name that no group declares never matches anything; the package.json parser used for folder, tarball and git packages (Package.rs,bundled_deps.contains) and the package-lock migration already work this way, as does npm's arborist.peerDependenciesis now bundled when the package comes from a registry manifest, which is what already happened for the same package installed as a tarball, git or folder dependency and forbundleDependencies: true. The bun.lock loader already re-marks bundled edges of every group; install: keep bundled peer edges on their own bun.lock entry when loading #38837 (separate file,bun.lock.rs) fixes how bundled peer edges are bound when a lockfile is loaded, and the frozen reinstall in the tests here passes with or without it.v0.0.8(same length, the format itself is unchanged): the contents ofbundled_deps_bufchanged, and a 304 revalidation reuses the parsed entry, so without the bump machines with a warm cache would keep the old lists until the packument changes.test/cli/install/bun-install-registry.test.ts,bundledDependenciesdescribe, with two new registry fixtures.bundled-with-optionalhasdependencies { no-deps },optionalDependencies { a-dep, basic-1 },bundleDependencies [no-deps, a-dep, not-a-dependency](the last name is declared in no group and does not exist in the registry) and shipsno-depsanda-dep;bundled-peerhaspeerDependencies { no-deps },bundleDependencies [no-deps]and shipsno-deps. Each test runs under bun.lock and bun.lockb, checks the layout and the lockfile, then deletesnode_modulesand reinstalls with--frozen-lockfile. Both fail on the current release (no-depsshows up in the rootnode_modules) and pass with this change.bun-install-registry.test.ts(246 pass),bun-lock.test.ts,bun-prune,bun-pm-licensesandmigration/migrate.test.ts; both fixtures also checked by hand under the isolated linker.bun-dedupe,bun-auditandbun-update-transitivepassed in full on the first revision; on the final build, with the host at a load average around 200, they only produced per-test timeouts (no assertion failures, and the timed-out tests pass when run individually).Background
node_modulesin the tarball and lists inbundleDependencies. bun marks that edgeBehavior::BUNDLED: it is not downloaded or hoisted, the tree places it directly under its parent, and bun.lock prints it as"<parent>/<name>"with"bundled": true.PackageManifest::parseconverts a registry packument into the cached.npmmanifest in two passes: a counting pass that sizes flat buffers, then a build pass that fills them. Per-version fields are(offset, len)windows into those buffers (ExternalSlice);bundled_dependenciesis such a window intobundled_deps_buf, andPackage::from_npmsetsBUNDLEDon every dependency whose name hash appears in the window..npmfiles start with a version header (Serializer::HEADER_BYTES); a file with a different header is discarded and the packument is fetched again. Entries with a matching header are reused as parsed, including after a 304 from the registry.First revision of this PR
The first revision kept the per-group filtering and only moved the slice start before the group loop, assigning the combined range once after it. Review pointed out that the filtering is redundant given
from_npm, and that the changed buffer contents need a cache header bump; the current revision does both.Failure on the current release
a-dep, the bundled name declared inoptionalDependencies, is correctly absent from the root in the first case: the list the optional group produced is the one that survived.