install: isolated linker honors active bun link - #29615
Closed
Kniggishood wants to merge 9 commits into
Closed
Conversation
Under the isolated linker, `bun link` previously had no effect on consumers whose dependency on the linked package resolved via npm (or a catalog pointing at an npm version): the project-local `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` body was materialized from the registry tarball cache and never refreshed from the producer. This adds a `linkedPackagePath()` helper that resolves `<globalLinkDir>/<pkg_name>` with a single lstat. When the helper finds an active link and the user has not opted into `--backend=symlink`, the isolated installer overrides `needs_install`, bypasses the cache arms, and materializes the store body from the producer via Hardlinker (with copyfile fallback on XDEV). For global-store-eligible entries the write goes into the staging dir so the existing `commitGlobalStoreEntry` rename still fires correctly. Test (a) under `bun link integration` in isolated-install.test.ts covers the npm-resolved case. Co-Authored-By: Claude Opus 4.7
Follow-ups for the isolated-linker / bun-link fix: - Delete `<final>` before hardlinking when the entry is global-store-eligible. Producer content is mutable, so the content-addressed collision-is-success path in `commitGlobalStoreEntry` would otherwise keep serving the stale entry after a producer rebuild. - Add tests covering: catalog-resolved dep, producer-rebuild propagation, control (no link → tarball body), and `--backend=symlink` opt-out. - Portable `linkedPackagePath()` on Windows (getFileAttributes) since `std.posix.S` is `void` on win targets. Co-Authored-By: Claude Opus 4.7
…ults Smoke-testing against a real producer/consumer (~/code/content-ui → ~/code/ui-amboss) revealed the linked-source branch was hardlinking the producer's entire working tree into node_modules/.bun/<hash>/<pkg>/ — including .git, .vscode, node_modules, source dirs, and any other dotfiles or config the producer happened to carry. A real repo has a lot of that; the synthetic test producer did not. Fix: at the Hardlinker/FileCopier call, skip the exact-name set of entries that `bun pm pack`'s default_ignore_patterns / root_default_ignore_patterns already strip when publishing (.git, .hg, .svn, CVS, node_modules for dirs; .DS_Store, .gitignore, .npmignore, .npmrc, .lock-wscript, npm-debug.log, bunfig.toml, .env.production, the five common lockfiles for files). Plumb skip_filenames through Walker-consuming helpers (Hardlinker.init, FileCopier.init) and update the six existing callers. The capability test derives its expected file set from `bun pm pack --dry-run` on the producer and asserts the installed `.bun/<pkg>/` tree matches. Binding the contract to bun's own publish semantics keeps the assertion honest even as those rules evolve, and avoids hardcoding a brittle list of "things that shouldn't leak." Deferred: .gitignore/.npmignore parsing, package.json#files globs, and pack's glob-style defaults (.*.swp, ._*, .wafpickle-*). A follow-up can plumb pack_command's full filter directly. Co-Authored-By: Claude Opus 4.7
Real producers use `package.json#files` to restrict what `npm publish` packs to the built output (content-ui: `"files": ["dist"]`). The previous commit only applied npm's default exact-name exclusions — everything else outside that denylist still leaked into the consumer's `node_modules/.bun/<hash>/<pkg>/`: `src/`, `docs/`, config files, dotfiles that aren't in the default list. This commit reads the producer's `package.json` via the existing `workspace_package_json_cache`, collapses each `files` entry to its top-level segment (`"dist/bundle.js"` → `"dist"`), and appends every root entry outside that set to the Walker's skip lists. Always-included names that npm ships regardless of `files` (package.json, README*/LICENSE*/LICENCE*/CHANGELOG*, case-insensitive) stay in. Scoped-out for follow-up: - `main`/`bin`/`module` paths outside the whitelist. npm would still publish them; we don't. - Nested entries with a collision: `files: ["dist"]` + a second `dist` nested under an allowed dir would be skipped too (Walker matches basenames at any depth, not root-only). - `.gitignore`/`.npmignore` semantics. - Glob patterns in `files`. - Windows: DirIterator yields WTF-16 basenames, which don't fit the ASCII comparators. Falls back to default-only exclusions there. Capability test uses a producer with `files: ["dist"]`, asserts `installed === bun pm pack --dry-run`. The test treats bun's own publish semantics as the source of truth; drift in either direction is caught at the assertion. Co-Authored-By: Claude Opus 4.7
The prior linkedPackagePath() did one lstat per dependency of the lockfile during isolated install to decide whether each package has an active `bun link`. On a 2000-package lockfile that's ~20-100ms of fixed overhead per install on a dev machine, regardless of whether any package is actually linked. Read the global link dir once at install start (main thread, before any worker is scheduled) and store the registered names (including scoped `@scope/name` forms) in a hashmap. After that, linkedPackagePath is a contains() check with no syscalls; when the map is empty — no active links on this machine, the common case on CI and dev boxes without `bun link` configured — it short-circuits to null immediately. The existing lstat path is kept as a fallback so callers outside the isolated-install flow (or on Windows, where DirIterator yields WTF-16 names the ASCII StringHashMap can't key) still get correct behavior; they just don't get the fast path. Concurrency: the map is populated on the main thread and read-only afterwards. Install workers scheduled via startTask() happen-after the populate call, so there is no race and no lock is needed. Co-Authored-By: Claude Opus 4.7
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughAdds a one-time main-thread cache for globally linked packages and exposes resolution helpers; integrates the cache into the isolated installer to prefer linked producer directories; extends file traversal APIs to support skipping filenames; adds a mutex to the workspace package.json cache; and adds packable-file selection helpers used by installer materialization. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/isolated_install.zig (1)
1577-1622:⚠️ Potential issue | 🟠 MajorKeep active
bun linkentries out of the global virtual store.Line 1622 forces a reinstall, but
uses_global_storefrom Line 1577 can still be true. That means mutable producer bytes can be committed under the normal registry-derived global-store key; after the link is removed, a warm install can reuse those producer bytes as if they came from the registry tarball. Mark active-linked entries global-store-ineligible (or include the link source in the store key) instead of only forcingneeds_install.Possible direction
// In the global-store eligibility check for immutable package resolutions: +if (PackageInstall.supported_method != .symlink) { + var linked_buf: bun.PathBuffer = undefined; + if (manager.linkedPackagePath(pkg_names[pkg_id].slice(string_buf), &linked_buf) != null) { + break :eligible false; + } +} break :eligible true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/install/isolated_install.zig` around lines 1577 - 1622, The code marks linked packages as "needs_install" but still leaves uses_global_store true, which can commit mutable producer bytes into the shared global store; update the logic so active linked packages are treated as global-store-ineligible: after computing uses_global_store = installer.entryUsesGlobalStore(entry_id) detect has_active_link (the existing PackageInstall.supported_method and manager.linkedPackagePath(pkg_name.slice(string_buf), &link_buf) call) and if has_active_link set uses_global_store to false (or call a new installer API to mark the entry ineligible) before any global-store materialization or store-key computation so the entry will not be written to the shared store; alternatively include the link source (link_buf) in the global-store key generation to make linked entries unique. Ensure the change references uses_global_store, has_active_link, manager.linkedPackagePath, and installer.entryUsesGlobalStore so the patch is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install/Installer.zig`:
- Around line 596-612: The whitelist-building loop (files_iter / whitelist in
Installer.zig) is incorrectly treating negated patterns like "!dist/internal" as
literal top segments (e.g., "!dist"), causing excludes parsed by
Pattern.fromUTF8 (in pack_command.zig) to be ignored; modify the loop to detect
and skip leading '!' on each string (or skip entries that start with '!') before
computing the top segment and inserting into whitelist so that negated entries
are not added as positive whitelist keys, or alternately add a clear comment
near the whitelist logic documenting that negation is unsupported to match the
current behavior.
- Around line 667-688: The current pre-delete of .staging/.final runs only when
installer.entryUsesGlobalStore(this.entry_id) is true, leaving non-global linked
entries to accumulate stale files; update the installer to also compute and
delete the corresponding project-local entry paths before writing when
entryUsesGlobalStore(...) is false (i.e. mirror the global-store branch): after
the existing branch, call the appropriate path-builder for the non-global
location (analogous to installer.appendGlobalStoreEntryPath — use the function
that builds the project-local store path or add one), then call
FD.cwd().deleteTree(...) on both the staging and final local paths for
this.entry_id to remove stale files; alternatively, if you choose not to change
behavior, add clear documentation near
entryUsesGlobalStore/appendGlobalStoreEntryPath explaining that producer
deletions do not propagate when global store is not used.
- Around line 589-649: The call to
manager.workspace_package_json_cache.getWithPath(...) from Task.run is not
thread-safe; add synchronization so concurrent worker tasks cannot call
getWithPath() simultaneously — either add a mutex inside
WorkspacePackageJSONCache (e.g., a field lock and a new lockedGetWithPath(...)
wrapper) and call that from Task.run, or acquire a shared mutex in Task.run
around the existing
manager.workspace_package_json_cache.getWithPath(manager.allocator, manager.log,
pkg_path, .{}) call; reference Task.run,
manager.workspace_package_json_cache.getWithPath, WorkspacePackageJSONCache, and
pkg_path when making the change so the cache access is guarded and avoids races
and invalidated entry pointers.
In `@src/install/PackageManager/PackageManagerDirectories.zig`:
- Around line 420-429: The populateLinkedNamesCache function currently calls
globalLinkDirPath which can trigger a process-exiting setup on failure; change
populateLinkedNamesCache (and its use of bun.openDirForIteration) to treat the
global link directory lookup as best-effort: call a non-fatal resolver or wrap
globalLinkDirPath in error-safe logic so that if the global link dir is missing
or unreadable you simply return leaving linked_names_populated true and an empty
cache instead of aborting; adjust the branch around bun.openDirForIteration to
swallow errors (log/debug if needed) and return so linkedPackagePath lookups
short-circuit to null without causing process exit.
- Around line 456-464: The current loops use `catch continue` when
`std.fmt.allocPrint` or `this.allocator.dupe` or `this.linked_names.put` fail,
which silently ignores OOM and leaves `linked_names` partially populated; change
those `catch continue` sites to detect OOM and crash via `bun.handleOom()` while
preserving the original behavior for non-OOM errors (i.e., continue on other
errors). Specifically, replace the `catch continue` after calls to
`std.fmt.allocPrint`, `this.allocator.dupe(u8, name)`, and
`this.linked_names.put(this.allocator, ...)` with an error handler that calls
`bun.handleOom()` when the error is `error.OutOfMemory` and otherwise continues.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2227-2242: The dynamic import of "fs/promises" inside the test
function should be moved to a module-scope import: replace the runtime import
that defines const { readdir, stat } with a top-level import from "fs/promises"
and update the local walk function to use those module-scope readdir and stat
identifiers; also apply the same change for the other occurrence in this file
that defines a walk helper (the second readdir/stat dynamic import near the
bottom of the test) so both walk implementations use the module-scope imports.
- Line 5: Replace uses of tmpdirSync() with the harness-provided tempDir() and
ensure each test calls tempDir() and passes its returned path into hermeticEnv()
for the BUN_INSTALL home so the harness can clean up global-link fixtures;
update the import list to include tempDir (remove tmpdirSync where present) and
change all places that create per-test homes (e.g., any code referencing
tmpdirSync() to set BUN_INSTALL or similar) to use const home = await tempDir()
and hermeticEnv({ BUN_INSTALL: home, ... }) (or the existing hermeticEnv call)
so the temporary directories are under harness disposal (also apply same change
to the other occurrences noted around the later block).
---
Outside diff comments:
In `@src/install/isolated_install.zig`:
- Around line 1577-1622: The code marks linked packages as "needs_install" but
still leaves uses_global_store true, which can commit mutable producer bytes
into the shared global store; update the logic so active linked packages are
treated as global-store-ineligible: after computing uses_global_store =
installer.entryUsesGlobalStore(entry_id) detect has_active_link (the existing
PackageInstall.supported_method and
manager.linkedPackagePath(pkg_name.slice(string_buf), &link_buf) call) and if
has_active_link set uses_global_store to false (or call a new installer API to
mark the entry ineligible) before any global-store materialization or store-key
computation so the entry will not be written to the shared store; alternatively
include the link source (link_buf) in the global-store key generation to make
linked entries unique. Ensure the change references uses_global_store,
has_active_link, manager.linkedPackagePath, and installer.entryUsesGlobalStore
so the patch is easy to locate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3e5ac10-0d8e-4fcf-87e1-c350baf61de0
📒 Files selected for processing (8)
src/install/PackageManager.zigsrc/install/PackageManager/PackageManagerDirectories.zigsrc/install/PackageManager/patchPackage.zigsrc/install/isolated_install.zigsrc/install/isolated_install/FileCopier.zigsrc/install/isolated_install/Hardlinker.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/isolated_install/Installer.zig`:
- Around line 551-570: The current root-scan whitelist logic can accidentally
mark files referenced by package.json fields (main/module/bin) as skipped;
update the installer to seed the keep-set (the set used to prevent additions to
extra_skip_files) with the top-level path segments referenced by package.json
before the root pre-scan: parse the producer manifest the same way pack_command
does (or reuse that manifest-parsing helper) and extract main and module (string
paths) and bin (string or object entries), then add each entry's top-level path
segment (first path component or filename) to the keep-set so the
Walker/root-scan never treats those entries as non-whitelisted; ensure this same
seeding is applied in the other root-scan locations mentioned (the other
pre-scan blocks around the Walker/extra_skip_files logic).
In `@src/install/PackageManager/PackageManagerDirectories.zig`:
- Around line 434-440: The code sets this.global_dir and this.global_link_dir
before calling bun.getFdPath, which can fail and leave this.global_link_dir_path
empty; move the assignments to this.global_dir and this.global_link_dir until
after bun.getFdPath and the call to
Fs.FileSystem.DirnameStore.instance.append(...) succeed so the object is never
left half-initialized; if bun.getFdPath or the path append fails, close the
opened local dir handles (the values returned by Options.openGlobalDir and
global_dir.makeOpenPath) before returning to avoid resource leaks; ensure
populateLinkedNamesCache() and globalLinkDirPath() will see either all three
fields set or none set.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2153-2166: The producer tempDir is created as a DisposableString
but is not wrapped with using, so its Symbol.dispose cleanup never runs and the
directory (including nested node_modules/.git/etc.) leaks; update the test to
declare the producer with using when calling tempDir (e.g., using const producer
= tempDir("linkpkg-realrepo-", ...)) so the DisposableString's dispose is
invoked; apply the same fix for the other producer at the "linkpkg-fileswl-"
creation and ensure any DisposableString fixtures (like home) follow the same
using pattern to prevent leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a15f775-62dc-410a-ad91-d854b58e2797
📒 Files selected for processing (4)
src/install/PackageManager/PackageManagerDirectories.zigsrc/install/PackageManager/WorkspacePackageJSONCache.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
Comment on lines
+434
to
+440
| var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return; | ||
| const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return; | ||
| this.global_dir = global_dir; | ||
| this.global_link_dir = link_dir; | ||
| var buf: bun.PathBuffer = undefined; | ||
| const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return; | ||
| this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice)); |
Contributor
There was a problem hiding this comment.
Avoid caching a half-initialized global link dir.
If bun.getFdPath() fails here, populateLinkedNamesCache() returns with this.global_link_dir already set but this.global_link_dir_path still empty. A later globalLinkDirPath() call then skips re-initialization and can build linked-package paths from "". Move the field assignments until after the path lookup succeeds, and close the local dirs on the early-return path.
Suggested fix
- var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
- const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
- this.global_dir = global_dir;
- this.global_link_dir = link_dir;
+ var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return;
+ errdefer global_dir.close();
+ const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return;
+ errdefer link_dir.close();
var buf: bun.PathBuffer = undefined;
const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return;
+ this.global_dir = global_dir;
+ this.global_link_dir = link_dir;
this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return; | |
| const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return; | |
| this.global_dir = global_dir; | |
| this.global_link_dir = link_dir; | |
| var buf: bun.PathBuffer = undefined; | |
| const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return; | |
| this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice)); | |
| var global_dir = Options.openGlobalDir(this.options.explicit_global_directory) catch return; | |
| errdefer global_dir.close(); | |
| const link_dir = global_dir.makeOpenPath("node_modules", .{}) catch return; | |
| errdefer link_dir.close(); | |
| var buf: bun.PathBuffer = undefined; | |
| const path_slice = bun.getFdPath(.fromStdDir(link_dir), &buf) catch return; | |
| this.global_dir = global_dir; | |
| this.global_link_dir = link_dir; | |
| this.global_link_dir_path = bun.handleOom(Fs.FileSystem.DirnameStore.instance.append([]const u8, path_slice)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/install/PackageManager/PackageManagerDirectories.zig` around lines 434 -
440, The code sets this.global_dir and this.global_link_dir before calling
bun.getFdPath, which can fail and leave this.global_link_dir_path empty; move
the assignments to this.global_dir and this.global_link_dir until after
bun.getFdPath and the call to Fs.FileSystem.DirnameStore.instance.append(...)
succeed so the object is never left half-initialized; if bun.getFdPath or the
path append fails, close the opened local dir handles (the values returned by
Options.openGlobalDir and global_dir.makeOpenPath) before returning to avoid
resource leaks; ensure populateLinkedNamesCache() and globalLinkDirPath() will
see either all three fields set or none set.
Replace the basename-keyed Walker skip list with a per-file linkat loop driven by collectPublishablePaths. Restores installed contents == published contents for any producer shape (globs, negation, nested same-name dirs). POSIX only; Windows keeps the previous default-skip path.
Three test bodies declared `const producer = tempDir(...)` and never disposed it. Switch to `using` so the directory is cleaned up at scope exit. The shared `setupLinkedNoDeps` helper returns its producer and must stay `const` — the caller already wraps with `using`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pack_command.zig`:
- Around line 959-992: When json_root has a "files" key that is not an array,
the code currently skips both iterateIncludedProjectTree and iterateProjectTree,
causing most files to be dropped; change the branch so if json_root.get("files")
yields a value but files.asArray() is null, call iterateProjectTree with the
same args used in the no-`files` path. Concretely, inside the if
(json_root.get("files")) |files| block, add an else case after if
(files.asArray()) { ... } that calls try iterateProjectTree(allocator,
&pack_queue, bins, .{ root_dir, "", 1 }, .silent); so non-array `files` mirrors
the fallback behavior of the absent `files` key (functions involved:
json_root.get("files"), files.asArray(), iterateIncludedProjectTree,
iterateProjectTree).
In `@src/install/isolated_install/Installer.zig`:
- Around line 549-562: The current code swallows error.OutOfMemory from
bun.cli.PackCommand.collectPublishablePaths by using `catch null`, which
conflates OOM with the intended null manifest-cache fallback; instead call
`collectPublishablePaths(...)` and handle its error by passing it through
bun.handleOom(err) to crash on OOM and return/propagate any other errors,
preserving `null` only for the manifest-cache miss/parse cases; update the call
site (the expression that currently does `break :publishable
bun.cli.PackCommand.collectPublishablePaths(...) catch null`) to detect an
error, call bun.handleOom(err) for error.OutOfMemory, and otherwise propagate or
return the non-OOM error rather than converting it to null.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 1963-1978: The test unnecessarily captures stdout then silences
it; update the Promise.all destructure from [stdout, stderr, exitCode] to [,
stderr, exitCode] so stdout is skipped, remove the redundant "void stdout;"
no-op, and keep the harmless "void producer;" reference; apply the same change
for the other occurrences in this test block (the installProc promise usage
around lines 2015–2028).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95dc9144-7df8-4254-b159-7703802c36dc
📒 Files selected for processing (3)
src/cli/pack_command.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pack_command.zig`:
- Around line 928-1011: collectPublishablePaths currently never walks vendored
bundled dependencies so packages with bundledDependencies/bundleDependencies are
missing their node_modules subtree; fix by invoking the same bundled-deps
collector used by pack() (iterateBundledDeps) and feeding its results into the
PackQueue so they become part of paths returned by collectPublishablePaths.
Locate collectPublishablePaths and, after parsing json_root (before assembling
pkg_path and paths), detect bundledDependencies/bundleDependencies and call
iterateBundledDeps(allocator, &pack_queue, /*pass same root_dir/json_root/bins
as pack() does*/), or refactor to a shared helper used by both pack() and
collectPublishablePaths; ensure items produced by iterateBundledDeps are added
via PackQueue.add so they are included in the final paths array.
In `@src/install/isolated_install/Installer.zig`:
- Around line 629-647: The current fast-path uses hardlinks for
publishable_owned entries regardless of install scope; modify the logic in the
block handling publishable_owned (the switch on PackageInstall.Method.hardlink
that calls linkedHardlinkPaths/linkedCopyPaths) so that hardlinking is only
attempted for global/immutable store entries (use the
installer.appendRealStorePath/this.entry_id/.staging info or whatever flag
identifies global/immutable entries) and for project-local linked installs
always force the .copyfile path (i.e., bypass the linkedHardlinkPaths call and
call linkedCopyPaths or set the effective method to copyfile) to avoid
hardlinking live producer files into non-global installs.
In `@test/cli/install/isolated-install.test.ts`:
- Around line 2235-2248: Extract the duplicate async function walk(dir: string,
prefix = ""): Promise<string[]> out of the three local closures and place a
single module-scoped helper with that same signature (e.g., at the top of the
test file). Replace the three inline definitions with calls to this shared walk
helper so all tests use the same traversal logic; keep the implementation
identical (readdir, stat, join, recursion) and ensure imports used by walk
remain in scope for the module-level function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9127b82-b937-4a80-b57e-2008b48a4619
📒 Files selected for processing (3)
src/cli/pack_command.zigsrc/install/isolated_install/Installer.zigtest/cli/install/isolated-install.test.ts
Comment on lines
+928
to
+1011
| /// Single source of truth for "what files would `bun pm pack` ship?". | ||
| /// Used by the isolated linker (when honoring an active `bun link`) so | ||
| /// the contents of `node_modules/.bun/<pkg>/` match what a publish of | ||
| /// the producer would contain — including the auto-included | ||
| /// `package.json`, `bin` entries outside the `files` whitelist, and | ||
| /// the recursive semantics of `files: ["dist/**/*.js"]`-style globs. | ||
| /// Without this, callers were forced to mirror the publish rules by | ||
| /// hand and inevitably drifted (cf. nested same-name directories). | ||
| pub fn collectPublishablePaths( | ||
| parent_allocator: std.mem.Allocator, | ||
| root_dir: std.fs.Dir, | ||
| json_root: Expr, | ||
| ) OOM!PublishablePaths { | ||
| var arena = std.heap.ArenaAllocator.init(parent_allocator); | ||
| errdefer arena.deinit(); | ||
| const allocator = arena.allocator(); | ||
|
|
||
| var pack_queue = PackQueue.init(allocator, {}); | ||
|
|
||
| const bins = try getPackageBins(allocator, json_root); | ||
|
|
||
| for (bins) |bin| { | ||
| switch (bin.type) { | ||
| .file => try pack_queue.add(.{ .path = bin.path, .optional = true }), | ||
| .dir => { | ||
| const bin_dir = root_dir.openDir(bin.path, .{ .iterate = true }) catch continue; | ||
| try iterateProjectTree(allocator, &pack_queue, &.{}, .{ bin_dir, bin.path, 2 }, .silent); | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| if (json_root.get("files")) |files| { | ||
| if (files.asArray()) |_files_array| { | ||
| var includes: std.ArrayListUnmanaged(Pattern) = .{}; | ||
| var excludes: std.ArrayListUnmanaged(Pattern) = .{}; | ||
|
|
||
| var path_buf: PathBuffer = undefined; | ||
| var files_array = _files_array; | ||
| while (files_array.next()) |files_entry| { | ||
| const file_entry_str = files_entry.asString(allocator) orelse continue; | ||
| const normalized = bun.path.normalizeBuf(file_entry_str, &path_buf, .posix); | ||
| const parsed = try Pattern.fromUTF8(allocator, normalized) orelse continue; | ||
| if (parsed.flags.negated) { | ||
| try excludes.append(allocator, parsed); | ||
| } else { | ||
| try includes.append(allocator, parsed); | ||
| } | ||
| } | ||
|
|
||
| try iterateIncludedProjectTree( | ||
| allocator, | ||
| &pack_queue, | ||
| bins, | ||
| includes.items, | ||
| excludes.items, | ||
| root_dir, | ||
| .silent, | ||
| ); | ||
| } else { | ||
| // `files` not an array → malformed manifest. `bun pm pack` | ||
| // crashes here; we can't, so we mirror the no-`files` path | ||
| // (publish-default tree) instead of dropping everything. | ||
| try iterateProjectTree(allocator, &pack_queue, bins, .{ root_dir, "", 1 }, .silent); | ||
| } | ||
| } else { | ||
| try iterateProjectTree(allocator, &pack_queue, bins, .{ root_dir, "", 1 }, .silent); | ||
| } | ||
|
|
||
| // `package.json` is unconditionally included — both iterators skip | ||
| // it explicitly because the pack pipeline writes it from a | ||
| // normalized AST. We don't have that pipeline; just ship the file. | ||
| const pkg_path = try allocator.dupeZ(u8, "package.json"); | ||
|
|
||
| var paths = try allocator.alloc([:0]const u8, pack_queue.count() + 1); | ||
| paths[0] = pkg_path; | ||
| var i: usize = 1; | ||
| while (pack_queue.removeOrNull()) |item| : (i += 1) { | ||
| paths[i] = item.path; | ||
| } | ||
|
|
||
| return .{ | ||
| .arena = arena, | ||
| .paths = paths, | ||
| }; |
Contributor
There was a problem hiding this comment.
collectPublishablePaths() still drops bundled dependencies.
This helper is described as the pack-time source of truth, but unlike pack() it never calls iterateBundledDeps(). A linked producer with bundledDependencies/bundleDependencies will therefore materialize without its vendored node_modules/... subtree even though bun pm pack would ship it. Please fold the bundled-deps walk into this helper (or extract a shared collector used by both paths).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/pack_command.zig` around lines 928 - 1011, collectPublishablePaths
currently never walks vendored bundled dependencies so packages with
bundledDependencies/bundleDependencies are missing their node_modules subtree;
fix by invoking the same bundled-deps collector used by pack()
(iterateBundledDeps) and feeding its results into the PackQueue so they become
part of paths returned by collectPublishablePaths. Locate
collectPublishablePaths and, after parsing json_root (before assembling pkg_path
and paths), detect bundledDependencies/bundleDependencies and call
iterateBundledDeps(allocator, &pack_queue, /*pass same root_dir/json_root/bins
as pack() does*/), or refactor to a shared helper used by both pack() and
collectPublishablePaths; ensure items produced by iterateBundledDeps are added
via PackQueue.add so they are included in the final paths array.
Comment on lines
+629
to
+647
| if (publishable_owned) |publishable_paths| { | ||
| var dest: bun.Path(.{ .unit = .os, .sep = .auto }) = .init(); | ||
| defer dest.deinit(); | ||
| installer.appendRealStorePath(&dest, this.entry_id, .staging); | ||
|
|
||
| backend: switch (PackageInstall.Method.hardlink) { | ||
| .hardlink => switch (linkedHardlinkPaths(folder_dir, publishable_paths.paths, &dest)) { | ||
| .result => {}, | ||
| .err => |err| { | ||
| if (err.getErrno() == .XDEV) continue :backend .copyfile; | ||
| return .failure(.{ .link_package = err }); | ||
| }, | ||
| }, | ||
| .copyfile => switch (linkedCopyPaths(folder_dir, publishable_paths.paths, &dest)) { | ||
| .result => {}, | ||
| .err => |err| return .failure(.{ .link_package = err }), | ||
| }, | ||
| else => unreachable, | ||
| } |
Contributor
There was a problem hiding this comment.
Do not hardlink live producer files into non-global installs.
This path always starts with hardlinks, but non-global linked entries can still run lifecycle scripts later in the task. Any install-time rewrite inside node_modules/.bun/<storepath>/... will then mutate the producer working tree through the shared inode, making bun install in the consumer destructive. Please restrict the hardlink fast-path to global-store/immutable entries and force copyfile for project-local linked installs.
Possible direction
- backend: switch (PackageInstall.Method.hardlink) {
+ const linked_method: PackageInstall.Method = if (uses_global_store_link)
+ .hardlink
+ else
+ .copyfile;
+ backend: switch (linked_method) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/install/isolated_install/Installer.zig` around lines 629 - 647, The
current fast-path uses hardlinks for publishable_owned entries regardless of
install scope; modify the logic in the block handling publishable_owned (the
switch on PackageInstall.Method.hardlink that calls
linkedHardlinkPaths/linkedCopyPaths) so that hardlinking is only attempted for
global/immutable store entries (use the
installer.appendRealStorePath/this.entry_id/.staging info or whatever flag
identifies global/immutable entries) and for project-local linked installs
always force the .copyfile path (i.e., bypass the linkedHardlinkPaths call and
call linkedCopyPaths or set the effective method to copyfile) to avoid
hardlinking live producer files into non-global installs.
Comment on lines
+2235
to
+2248
| async function walk(dir: string, prefix = ""): Promise<string[]> { | ||
| const out: string[] = []; | ||
| for (const name of await readdir(dir)) { | ||
| const abs = join(dir, name); | ||
| const rel = prefix ? `${prefix}/${name}` : name; | ||
| const s = await stat(abs); | ||
| if (s.isDirectory()) { | ||
| out.push(...(await walk(abs, rel))); | ||
| } else { | ||
| out.push(rel); | ||
| } | ||
| } | ||
| return out; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the duplicated walk helper to module scope.
The same recursive directory-walk closure is defined three times (Lines 2235‑2248, 2345‑2358, 2431‑2444) with identical logic. Extract a single module-scope helper to keep the three capability tests aligned and avoid drift if the traversal contract changes (e.g., needing to follow/not-follow symlinks).
♻️ Proposed refactor
+async function listFilesRecursive(dir: string, prefix = ""): Promise<string[]> {
+ const out: string[] = [];
+ for (const name of await readdir(dir)) {
+ const abs = join(dir, name);
+ const rel = prefix ? `${prefix}/${name}` : name;
+ const s = await stat(abs);
+ if (s.isDirectory()) {
+ out.push(...(await listFilesRecursive(abs, rel)));
+ } else {
+ out.push(rel);
+ }
+ }
+ return out;
+}
@@
- async function walk(dir: string, prefix = ""): Promise<string[]> { /* ... */ }
- const installedFiles = new Set(await walk(bodyDir));
+ const installedFiles = new Set(await listFilesRecursive(bodyDir));Apply at all three call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli/install/isolated-install.test.ts` around lines 2235 - 2248, Extract
the duplicate async function walk(dir: string, prefix = ""): Promise<string[]>
out of the three local closures and place a single module-scoped helper with
that same signature (e.g., at the top of the test file). Replace the three
inline definitions with calls to this shared walk helper so all tests use the
same traversal logic; keep the implementation identical (readdir, stat, join,
recursion) and ensure imports used by walk remain in scope for the module-level
function.
Collaborator
|
Rebased on current
Gate-check: the 6 capability tests in I can't push to your fork, so 30289 is opened from |
robobun
pushed a commit
that referenced
this pull request
May 15, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
robobun
pushed a commit
that referenced
this pull request
May 21, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
robobun
pushed a commit
that referenced
this pull request
May 24, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
robobun
pushed a commit
that referenced
this pull request
Jun 5, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
robobun
pushed a commit
that referenced
this pull request
Jun 5, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
robobun
pushed a commit
that referenced
this pull request
Jun 22, 2026
Seven fixes from CodeRabbit's review of the isolated-linker-honors-bun-link
series.
Correctness:
- Non-global linked entries now pre-delete the project-local final path
before hardlinking, mirroring the global-store branch. Without this,
files the producer has since deleted would remain on reinstall.
- workspace_package_json_cache.getWithPath is called from the install
thread pool via Task.run; the returned *MapEntry is invalidated by
hashmap grow on subsequent calls. Added a Mutex on the cache and held
it externally at the worker-thread caller across both the lookup and
the entry-pointer use.
- populateLinkedNamesCache no longer routes through globalLinkDirPath
(which Global.exit(1)s on setup failure). Treat missing / unreadable
global dir as "no links on this machine" and leave the cache empty.
- OOM in populateLinkedNamesCache now goes through bun.handleOom instead
of silent `catch continue`.
Hygiene:
- Skip `!`-prefixed `files` entries in the whitelist loop so
"!dist/internal" doesn't pollute the whitelist with `!dist`.
- Tests use `using home = tempDir("link-home-", {})` + `hermeticEnv(home)`
for harness-managed global-link fixture cleanup; dropped tmpdirSync.
- Hoisted fs/promises readdir/stat to module-scope import.
Collaborator
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
robobun
added a commit
that referenced
this pull request
Jul 5, 2026
Under the isolated linker, an active `bun link` in a producer and `bun link <pkg>` in a consumer had no effect when the consumer's dep resolved via npm or a catalog: the consumer's `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` was materialized from the registry tarball cache, so producer edits never reached the consumer. The hoisted linker honored the link; the isolated linker silently ignored it. Seed a linked-names cache once on the main thread (one readdir of the global link dir), compute a linked_pkg_ids bitset of the package ids that root/workspace direct dependencies resolve to whose name is link-registered, and gate the override on it. The GVS eligibility carve-out forces those entries project-local (entry_hash = 0) so mutable producer content never lands in the shared content-addressed store, and the worker sources the body from the producer via copyfile (never hardlink) after detaching any stale GVS symlink. Scoping by direct-dep resolution rather than by name keeps transitive different-version copies registry-sourced, matching the hoisted linker. Closes #30287. Supersedes #29615. Credit to @Kniggishood for the original approach.
robobun
added a commit
that referenced
this pull request
Jul 8, 2026
Under the isolated linker, an active `bun link` in a producer and `bun link <pkg>` in a consumer had no effect when the consumer's dep resolved via npm or a catalog: the consumer's `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` was materialized from the registry tarball cache, so producer edits never reached the consumer. The hoisted linker honored the link; the isolated linker silently ignored it. Seed a linked-names cache once on the main thread (one readdir of the global link dir), compute a linked_pkg_ids bitset of the package ids that root/workspace direct dependencies resolve to whose name is link-registered, and gate the override on it. The GVS eligibility carve-out forces those entries project-local (entry_hash = 0) so mutable producer content never lands in the shared content-addressed store, and the worker sources the body from the producer via copyfile (never hardlink) after detaching any stale GVS symlink. Scoping by direct-dep resolution rather than by name keeps transitive different-version copies registry-sourced, matching the hoisted linker. Closes #30287. Supersedes #29615. Credit to @Kniggishood for the original approach.
robobun
added a commit
that referenced
this pull request
Aug 14, 2026
Under the isolated linker, an active `bun link` in a producer and `bun link <pkg>` in a consumer had no effect when the consumer's dep resolved via npm or a catalog: the consumer's `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` was materialized from the registry tarball cache, so producer edits never reached the consumer. The hoisted linker honored the link; the isolated linker silently ignored it. Seed a linked-names cache once on the main thread (one readdir of the global link dir), compute a linked_pkg_ids bitset of the package ids that root/workspace direct dependencies resolve to whose name is link-registered, and gate the override on it. The GVS eligibility carve-out forces those entries project-local (entry_hash = 0) so mutable producer content never lands in the shared content-addressed store, and the worker sources the body from the producer via copyfile (never hardlink) after detaching any stale GVS symlink. Scoping by direct-dep resolution rather than by name keeps transitive different-version copies registry-sourced, matching the hoisted linker. Closes #30287. Supersedes #29615. Credit to @Kniggishood for the original approach.
robobun
added a commit
that referenced
this pull request
Aug 14, 2026
Under the isolated linker, an active `bun link` in a producer and `bun link <pkg>` in a consumer had no effect when the consumer's dep resolved via npm or a catalog: the consumer's `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` was materialized from the registry tarball cache, so producer edits never reached the consumer. The hoisted linker honored the link; the isolated linker silently ignored it. Seed a linked-names cache once on the main thread (one readdir of the global link dir), compute a linked_pkg_ids bitset of the package ids that root/workspace direct dependencies resolve to whose name is link-registered, and gate the override on it. The GVS eligibility carve-out forces those entries project-local (entry_hash = 0) so mutable producer content never lands in the shared content-addressed store, and the worker sources the body from the producer via copyfile (never hardlink) after detaching any stale GVS symlink. Scoping by direct-dep resolution rather than by name keeps transitive different-version copies registry-sourced, matching the hoisted linker. Closes #30287. Supersedes #29615. Credit to @Kniggishood for the original approach.
robobun
added a commit
that referenced
this pull request
Aug 14, 2026
Under the isolated linker, an active `bun link` in a producer and `bun link <pkg>` in a consumer had no effect when the consumer's dep resolved via npm or a catalog: the consumer's `node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>` was materialized from the registry tarball cache, so producer edits never reached the consumer. The hoisted linker honored the link; the isolated linker silently ignored it. Seed a linked-names cache once on the main thread (one readdir of the global link dir), compute a linked_pkg_ids bitset of the package ids that root/workspace direct dependencies resolve to whose name is link-registered, and gate the override on it. The GVS eligibility carve-out forces those entries project-local (entry_hash = 0) so mutable producer content never lands in the shared content-addressed store, and the worker sources the body from the producer via copyfile (never hardlink) after detaching any stale GVS symlink. Scoping by direct-dep resolution rather than by name keeps transitive different-version copies registry-sourced, matching the hoisted linker. Closes #30287. Supersedes #29615. Credit to @Kniggishood for the original approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Under the isolated linker,
bun linkpreviously had no effect onconsumers whose dependency on the linked package resolved via npm (or a
catalog pointing at an npm version): the project-local
node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>body was materializedfrom the registry tarball cache and never refreshed from the producer.
This broke the "edit producer → see changes in consumer" dev loop that
is the whole point of
bun link.The fix adds a
linkedPackagePath()lookup onPackageManagerthatresolves
<globalLinkDir>/<pkg>via an install-start readdir (onesyscall for the whole install, zero per-dep for linked-free machines).
When the lookup hits and the user hasn't opted into
--backend=symlink,the isolated installer materializes the entry's body from the producer
tree via
Hardlinker(withcopyfilefallback onXDEV). The writegoes through the normal
.stagingpath socommitGlobalStoreEntry's rename still fires correctly forglobal-store-eligible entries; the entry's
<final>is pre-deleted soproducer rebuilds actually propagate on reinstall (producer content is
mutable, so the content-addressed collision-is-success path doesn't
apply).
The Hardlinker/FileCopier call gained a
skip_filenamesparameter(matching the Walker's existing capability) so the producer tree is
filtered through npm's publish semantics:
.git,.DS_Store,lockfiles, etc. are excluded, and
package.json#filesacts as a root-level whitelist when present. Capability tests compare the installed
tree against
bun pm pack --dry-runso the assertion binds to bun'sown publish semantics rather than a fragile hand-maintained denylist.
What's in the diff
src/install/PackageManager/PackageManagerDirectories.zig—
linkedPackagePath()helper +populateLinkedNamesCache()(O(1)subsequent lookups, empty-dir short-circuit).
src/install/PackageManager.zig— field for the cache + re-exports.src/install/isolated_install.zig—has_active_linkgate onneeds_install, cache-arm bypass,populateLinkedNamesCachecallat install start.
src/install/isolated_install/Installer.zig— linked-source branchinside
link_package: stat, open producer dir, filter via thenpm-default skip list +
fileswhitelist, hardlink/copyfile into.staging, let commit publish the<final>.src/install/isolated_install/{Hardlinker,FileCopier}.zig— extendedinit signatures to accept
skip_filenames.src/install/PackageManager/patchPackage.zig— updated caller for thenew signature (single extra
&.{}arg).test/cli/install/isolated-install.test.ts— newdescribe("bun link integration", …)block: 6 capability tests covering npm-resolved,catalog-resolved, producer-rebuild propagation, symlink-backend
opt-out, control-no-link, and npm-publish-filter equivalence.
Test plan
bun bd test test/cli/install/isolated-install.test.ts— 50 pass.bun bd test test/cli/install/bun-install-patch.test.ts— 17 pass(regression guard for
patchPackage'sFileCopier.initcaller).bun run zig:check-all— 16/16 target × profile green (macOS,Linux, Windows × x86_64/aarch64 × Debug/ReleaseFast).
.bun/<pkg>/dist/; npmfiles-restricted producers install only the publishable subset(no
.git, nosrc/, etc.);--backend=symlinkcorrectly skipsthe override;
bun unlinkon the producer cleanly reverts totarball materialization.
Scope / follow-ups
Deliberately out of scope for this PR (each worth a separate
conversation):
--backend=symlink+ active link should make.bun/<pkg>itself a symlink to the producer, not a registry-tarballmaterialization. This PR's opt-out preserves the pre-existing
materialization for that combo; a follow-up can change that to a
direct symlink so producer edits propagate with zero reinstall.
package.json#files(dist/*.js). Currentimplementation treats each
filesentry as a top-level path segmentand the Walker skip-list is basename-matched at every depth; accurate
for the 95% case, imprecise for edge shapes.
main/bin/moduleentrypoints outsidefiles. npm stillpublishes these; we don't.
.gitignore/.npmignoreparsing. Currently only the exact-namedefault_ignore_patternsare applied (mirroringpack_command.zig); full pack-command parity is a broader refactor.fileswhitelist —DirIteratoryields WTF-16 basenames that the ASCII comparatorscan't key; Windows falls back to default-only exclusions.
Interaction with #29489
This PR builds on #29489's
global virtual store. It uses
entryUsesGlobalStore,appendGlobalStoreEntryPath,commitGlobalStoreEntry. Testing againstcurrent main surfaced two unrelated regressions that appear to trace to
#29489 itself (bundler canonicalization + warm-install
re-materialization); those are filed separately and are not blockers for
this PR — it's a focused correctness fix for
bun link, not an attemptto rework the store layout.