Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/install/PackageManager.zig
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/// PackageManager is a singleton and is not deinitialized on program exit.
/// Most of these fields (like `nohoist_patterns`) will stay alive forever.
cache_directory_: ?std.fs.Dir = null,
cache_directory_path: stringZ = "",
root_dir: *Fs.FileSystem.DirEntry,
Expand Down Expand Up @@ -135,6 +137,8 @@ active_lifecycle_scripts: LifecycleScriptSubprocess.List,
last_reported_slow_lifecycle_script_at: u64 = 0,
cached_tick_for_slow_lifecycle_script_logging: u64 = 0,

nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = .init(),

/// Corresponds to possible commands from the CLI.
pub const Subcommand = enum {
install,
Expand Down
5 changes: 5 additions & 0 deletions src/install/lockfile.zig
Original file line number Diff line number Diff line change
Expand Up @@ -951,15 +951,20 @@ pub fn hoist(
Tree.root_dep_id,
Tree.invalid_id,
method,
if (comptime method == .filter) .init() else {},
&builder,
);

// This goes breadth-first
while (builder.queue.readItem()) |item| {
var subpath = item.subpath;
defer bun.memory.deinit(&subpath);

try builder.list.items(.tree)[item.tree_id].processSubtree(
item.dependency_id,
item.hoist_root_id,
method,
subpath,
&builder,
);
}
Expand Down
22 changes: 22 additions & 0 deletions src/install/lockfile/Package.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1578,6 +1578,28 @@ pub fn Package(comptime SemverIntType: type) type {

if (json.get("workspaces")) |workspaces_expr| {
lockfile.catalogs.parseCount(lockfile, workspaces_expr, &string_builder);

if (workspaces_expr.get("nohoist")) |nohoist_expr| {
if (!nohoist_expr.isArray()) {
try log.addError(source, nohoist_expr.loc, "Expected an array of strings");
return error.InvalidPackageJSON;
}

const nohoist_arr = nohoist_expr.data.e_array;

var nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = try .initCapacity(nohoist_arr.items.len);

for (nohoist_arr.items.slice()) |*nohoist_item| {
if (!nohoist_item.isString()) {
try log.addError(source, nohoist_item.loc, "Expected a string pattern");
return error.InvalidPackageJSON;
}

nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator));
}

pm.nohoist_patterns = nohoist_patterns;
}
Comment on lines +1590 to +1602

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential memory leak in error path

If parsing succeeds for some patterns but fails on a later pattern (line 1594-1596), the successfully cloned strings in nohoist_patterns are leaked because the ArrayList is never assigned to pm.nohoist_patterns and never deinitialized.

Consider deferring cleanup or building directly into pm.nohoist_patterns:

-                    var nohoist_patterns: bun.collections.ArrayListDefault([]const u8) = try .initCapacity(nohoist_arr.items.len);
+                    try pm.nohoist_patterns.ensureTotalCapacity(nohoist_arr.items.len);
+                    const original_len = pm.nohoist_patterns.items().len;
+                    errdefer {
+                        for (pm.nohoist_patterns.items()[original_len..]) |pattern| {
+                            bun.default_allocator.free(pattern);
+                        }
+                        pm.nohoist_patterns.shrinkRetainingCapacity(original_len);
+                    }

                     for (nohoist_arr.items.slice()) |nohoist_item| {
                         if (!nohoist_item.isString()) {
                             try log.addError(source, nohoist_item.loc, "Expected a string pattern");
                             return error.InvalidPackageJSON;
                         }

-                        nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator));
+                        pm.nohoist_patterns.appendAssumeCapacity(try nohoist_item.data.e_string.stringCloned(bun.default_allocator));
                     }
-
-                    pm.nohoist_patterns = nohoist_patterns;

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/install/lockfile/Package.zig around lines 1590-1602, the code builds a
local ArrayList of cloned nohoist strings but on an early error path the list
and its allocated strings can leak because it is not deinitialized; to fix this
either (a) defer a cleanup that deinitializes/free the local nohoist_patterns on
any error before returning, or (b) avoid the temporary by initializing
pm.nohoist_patterns first and append directly into it so assignment is
unnecessary, ensuring any failure deinitializes whatever was initialized;
implement one of these approaches so all allocated memory is freed on error and
only set pm.nohoist_patterns after successful completion.

}

// Count catalog strings in top-level package.json as well, since parseAppend
Expand Down
92 changes: 77 additions & 15 deletions src/install/lockfile/Tree.zig
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,32 @@ pub fn Builder(comptime method: BuilderMethod) type {
lockfile: *const Lockfile,
// unresolved optional peers that might resolve later. if they do we will want to assign
// builder.resolutions[peer.dep_id] to the resolved pkg_id.
pending_optional_peers: std.AutoHashMap(PackageNameHash, bun.collections.ArrayListDefault(DependencyID)),
pending_optional_peers: std.AutoHashMap(PackageNameHash, ArrayListDefault(DependencyID)),
manager: if (method == .filter) *const PackageManager else void,
sort_buf: std.ArrayListUnmanaged(DependencyID) = .{},
workspace_filters: if (method == .filter) []const WorkspaceFilter else void = if (method == .filter) &.{},
install_root_dependencies: if (method == .filter) bool else void,
packages_to_install: if (method == .filter) ?[]const PackageID else void,

pub const FillItem = struct {
tree_id: Tree.Id,
dependency_id: DependencyID,

/// If valid, dependencies will not hoist
/// beyond this tree if they're in a subtree
hoist_root_id: Tree.Id,

/// Relative path of the current tree. Each component is a package
/// name or part of the package name (if the name is scoped) leading
/// to the tree. Example values:
/// "" (the root subpath is empty)
/// "react/loose-envify" (react -> loose-envify)
/// "@types/bun/bun-types/@types/node" (@types/bun -> bun-types -> @types/node)
subpath: Subpath(method),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
};
Comment thread
markovejnovic marked this conversation as resolved.
Resolved

pub const TreeFiller = bun.LinearFifo(FillItem, .Dynamic);

pub fn maybeReportError(this: *@This(), comptime fmt: string, args: anytype) void {
this.log.addErrorFmt(null, logger.Loc.Empty, this.allocator, fmt, args) catch {};
}
Expand Down Expand Up @@ -429,7 +448,7 @@ pub fn isFilteredDependencyOrWorkspace(
},
};

switch (bun.glob.match(pattern, name_or_path)) {
switch (glob.match(pattern, name_or_path)) {
.match, .negate_match => workspace_matched = true,

.negate_no_match => {
Expand All @@ -447,11 +466,19 @@ pub fn isFilteredDependencyOrWorkspace(
return !workspace_matched;
}

fn Subpath(comptime method: BuilderMethod) type {
return switch (method) {
.filter => ArrayListDefault(u8),
.resolvable => void,
};
}

pub fn processSubtree(
this: *const Tree,
dependency_id: DependencyID,
hoist_root_id: Tree.Id,
comptime method: BuilderMethod,
subpath: Subpath(method),
builder: *Builder(method),
) SubtreeError!void {
const parent_pkg_id = switch (dependency_id) {
Expand Down Expand Up @@ -496,8 +523,11 @@ pub fn processSubtree(
);

for (builder.sort_buf.items) |dep_id| {
const dependency = builder.dependencies[dep_id];
const pkg_id = builder.resolutions[dep_id];

var dep_subpath: ArrayListDefault(u8) = .init();
Comment on lines 498 to +529

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Memory leak: dep_subpath is never deinitialized.

The dep_subpath ArrayList is initialized on line 523 but is never cleaned up in any code path. This will leak memory for every dependency processed during filtering.

Apply this diff to add proper cleanup:

     for (builder.sort_buf.items) |dep_id| {
         const dependency = builder.dependencies[dep_id];
         const pkg_id = builder.resolutions[dep_id];

         var dep_subpath: ArrayListDefault(u8) = .init();
+        defer bun.memory.deinit(&dep_subpath);

         // filter out disabled dependencies
🤖 Prompt for AI Agents
In src/install/lockfile/Tree.zig around lines 519-523, the ArrayList dep_subpath
is initialized for each dependency but never deinitialized, leaking memory on
every iteration; ensure dep_subpath is always cleaned up by calling its deinit
in every control path — simplest fix: immediately arrange for cleanup after
initialization (e.g., use a defer-like pattern so dep_subpath.deinit() runs when
the loop iteration exits) or explicitly call dep_subpath.deinit() before any
continue/return/branch that skips the normal end-of-iteration, so every created
ArrayList is deinitialized.


// filter out disabled dependencies
if (comptime method == .filter) {
if (isFilteredDependencyOrWorkspace(
Expand Down Expand Up @@ -534,9 +564,48 @@ pub fn processSubtree(
}
}

const dependency = builder.dependencies[dep_id];

const hoisted: HoistDependencyResult = hoisted: {
if (comptime method == .filter) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is already 711 lines long. I would suggest taking your changes and putting them in a function which answers a self-contained question.

Here's how I would approach refactoring this:

  1. What are the different intents that this function has?
    a) Are there intents that seem self-contained?

I can identify two chunks of code that have two pretty self-contained intents:

  1. The loop which checks whether something matches a no-hoist pattern without allowing circular dependencies.
  2. The loop which answers whether something is circular or not.

Refactor 2.

Let's start with 2:

var curr = trees[next.id].parent;
while (curr != invalid_id) {
    const curr_dep_id = trees[curr].dependency_id;
    const curr_pkg_id = switch (curr_dep_id) {
        root_dep_id => 0,
        else => builder.resolutions[curr_dep_id],
    };
    var curr_resolutions = builder.resolution_lists[curr_pkg_id];
    for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
        const res_id = builder.resolutions[tree_dep_id];
        if (res_id == pkg_id) {
            break :try_nohoist;
        }
    }

    curr = trees[curr].parent;
}

This chunk of code is prefixed by a comment:

// make sure it's not circular

Let's use that as the name:

fn makeSureTreeNotCircular(...) bool

Okay, well this is a predicate, so maybe the verb should be is, rather than make:

fn isTreeCircular(...) bool

Great! That seems a little neater. What are the parameters that this function has? Now that we have the name, we can probably infer that it will have to take a tree. It shouldn't need to mutate it, so I suppose that it should take an immutable ref.

fn isTreeCircular(tree: *const Tree) bool

Great, let's copy-paste the implementation

fn isTreeCircular(tree: *const Tree) bool {
    var curr = trees[next.id].parent;
    while (curr != invalid_id) {
        const curr_dep_id = trees[curr].dependency_id;
        const curr_pkg_id = switch (curr_dep_id) {
            root_dep_id => 0,
            else => builder.resolutions[curr_dep_id],
        };
        var curr_resolutions = builder.resolution_lists[curr_pkg_id];
        for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
            const res_id = builder.resolutions[tree_dep_id];
            if (res_id == pkg_id) {
                break :try_nohoist;
            }
        }

        curr = trees[curr].parent;
    }
}

Wow! Far lower nesting, but this won't really compile as-is. There's a couple invalid references now. Without any compilation I see: trees, next and pkg_id. Let's change our signature, rename some variables, and replace the breaks with returns:

fn isTreeCircular(pkg_id: PackageID, forest: []const Tree, tree: *const Tree) bool {
    var curr = forest[tree.id].parent;
    while (curr != invalid_id) {
        const curr_dep_id = forest[curr].dependency_id;
        const curr_pkg_id = switch (curr_dep_id) {
            root_dep_id => 0,
            else => builder.resolutions[curr_dep_id],
        };
        var curr_resolutions = builder.resolution_lists[curr_pkg_id];
        for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
            const res_id = builder.resolutions[tree_dep_id];
            if (res_id == pkg_id) {
                return true;
            }
        }

        curr = forest[curr].parent;
    }
    
    return false;
}

We still depend on builder, and the reason I haven't talked about it now is that there's a decision to be made. Do we:

a) Make this new function a member of builder?
b) Inject builder into the function as a dependency?

I lean towards the former. I ended up, after a little bit of cleanup and renaming on the following:

for (builder.manager.nohoist_patterns) |nohoist_pattern| {
    if (glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
        if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
            break :try_nohoist;
        }
        break :hoisted .{ .placement = .{ .id = next.id } };
    }
}

Nesting this deep is usually a code-smell, but we have a cheap way to invert the condition and reduce one level of nesting.

for (builder.manager.nohoist_patterns) |nohoist_pattern| {
    if (!glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
        continue;
    }

    if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
        break :try_nohoist;
    }
    break :hoisted .{ .placement = .{ .id = next.id } };
}

Aside

This is very optional, but I would split up the loop, too:

for (builder.manager.nohoist_patterns) |nohoist_pattern| {
    if (!glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
        continue;
    }

    if (builder.pkgHasCircularReference(pkg_id, trees, next)) {
        break :try_nohoist;
    }
    break :hoisted .{ .placement = .{ .id = next.id } };
}

Like so:

struct Builder {
    fn shouldNoHoist(...)
};

and

```zig
if (builder.shouldNoHoist(dep_subpath.items(), pkg_id, trees, next) catch |err| {
    switch (err) {
        error.CircularDependency => break :try_nohoist,
    }
}) {
    break :hoisted .{ .placement = .{ .id = next.id } };
}

Refactor 1

The next thing I would do is try to isolate the changes you've made into a self-contained function.

Blocks are crappier, weaker versions of functions, and you already have a pretty good name for a function here -- tryNoHoist, so let's stick with that.

You could maybe attach this to Builder, like so:

pub fn tryNohoist(
    self: *const @This(),
    subpath: []const u8,
    dependency_id: DependencyID,
) ?error{CircularDependency}!struct {
    result: HoistDependencyResult,
    subpath: bun.collections.ArrayListDefault(u8),
}

and you end up with something like this

if (try builder.tryNohoist(if (method == .filter) subpath.items() else {}, dep_id)) |r| {
    dep_subpath = r.subpath;
    break :hoisted r.result;
}

At the call site.

All my suggestions are available here: #24646

// not filtered, but does it match a nohoist pattern?
if (!builder.manager.nohoist_patterns.isEmpty()) try_nohoist: {
const string_buf = builder.lockfile.buffers.string_bytes.items;

try dep_subpath.ensureTotalCapacity(subpath.items().len + @intFromBool(!subpath.isEmpty()) + dependency.name.len());
dep_subpath.appendSliceAssumeCapacity(subpath.items());
if (!subpath.isEmpty()) {
dep_subpath.appendAssumeCapacity('/');
}
dep_subpath.appendSliceAssumeCapacity(dependency.name.slice(string_buf));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't completely understand dependency.name.slice(string_buf). Is it safe to use string_buf. How do you guarantee there's no useful data you're stomping over?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string_buf is safe to use because at this stage we've finished downloading manifests and building packages in memory. It's during appending to lockfile.packages that lockfile.buffers.string_bytes.items is changed mostly


if (dependency.version.tag == .workspace) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple questions from me:

  1. Why are we excluding workspace dependencies, because they're not hoistable? If that's the case, should we not do the same for catalog dependencies?
  2. The coupling here is fragile. What would happen if we added a new Tag that may be incompatible with no-hoisting? I would suggest you add a new method to
pub const Tag = enum(u8) {
    pub fn supportsHoisting(self: @This()) {
        return switch(self) {
            .uninitialized, workspace, catalog => false,
            .npm, .dist_tag, ... => true,
        };
    }

I would suggest against using else in this switch since you want additions/removals to Tag enum members to break compilation and cause us to look at as many potential breakage points as possible.

@dylan-conway dylan-conway Nov 13, 2025 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a unique property to workspace dependencies in bun: they always have an implicit dependency appended to lockfile.buffers.dependencies, even if no packages depend on them. This means we need to always hoist workspace dependencies to make sure the implicit and explicit dependencies deduplicate correctly (otherwise there will be collisions due to duplicate names when we are linking node_modules)

Always hoisting them works because if the explicit workspace dependency name has a different name than the implicit workspace dependency it won't be able to hoist anyways matching the behavior of "nohoist"

break :try_nohoist;
}

for (builder.manager.nohoist_patterns.items()) |nohoist_pattern| {
if (glob.match(nohoist_pattern, dep_subpath.items()).matches()) {
// make sure it's not circular
var curr = trees[next.id].parent;
while (curr != invalid_id) {
const curr_dep_id = trees[curr].dependency_id;
const curr_pkg_id = switch (curr_dep_id) {
root_dep_id => 0,
else => builder.resolutions[curr_dep_id],
};
var curr_resolutions = builder.resolution_lists[curr_pkg_id];
for (curr_resolutions.begin()..curr_resolutions.end()) |tree_dep_id| {
const res_id = builder.resolutions[tree_dep_id];
if (res_id == pkg_id) {
break :try_nohoist;
Comment thread
markovejnovic marked this conversation as resolved.
Resolved
}
}

curr = trees[curr].parent;
}
break :hoisted .{ .placement = .{ .id = next.id } };
}
}
}
}

// don't hoist if it's a folder dependency or a bundled dependency.
if (dependency.behavior.isBundled()) {
Expand Down Expand Up @@ -617,6 +686,7 @@ pub fn processSubtree(
.tree_id = replace.id,
.dependency_id = dep_id,
.hoist_root_id = hoist_root_id,
.subpath = if (comptime method == .filter) dep_subpath else {},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Ensure transferred ownership is properly managed.

When dep_subpath is assigned to .subpath at these lines, ownership of the internal buffer is transferred. However, the local dep_subpath variable still holds a copy of the struct with pointers to that buffer.

After fixing the memory leak identified earlier by adding a defer dep_subpath.deinit();, you must also reset dep_subpath immediately after these assignments to prevent double-free:

.subpath = dep_subpath,
dep_subpath = .init(); // Reset to prevent defer from freeing transferred buffer

Alternatively, use a boolean flag to track whether ownership was transferred and conditionally deinit in the defer.

Also applies to: 713-713

🤖 Prompt for AI Agents
In src/install/lockfile/Tree.zig around lines 689 and 713, assigning dep_subpath
into the .subpath field transfers ownership of the internal buffer but leaves
the local dep_subpath holding pointers, which will cause a double-free when its
defer runs; after each assignment that transfers ownership, reset dep_subpath to
an empty/initial state (or flip a boolean “transferred” flag) so the deferred
deinit does not free the transferred buffer, or alternatively track transfer
with a flag and conditionally call deinit only when ownership was not moved.

});
}
},
Expand All @@ -640,6 +710,7 @@ pub fn processSubtree(

// if it's bundled, start a new hoist root
.hoist_root_id = if (dest.bundled) dest.id else hoist_root_id,
.subpath = if (comptime method == .filter) dep_subpath else {},
});
}
},
Expand Down Expand Up @@ -763,17 +834,6 @@ fn hoistDependency(
return .{ .placement = .{ .id = this.id } }; // 2
}

pub const FillItem = struct {
tree_id: Tree.Id,
dependency_id: DependencyID,

/// If valid, dependencies will not hoist
/// beyond this tree if they're in a subtree
hoist_root_id: Tree.Id,
};

pub const TreeFiller = bun.LinearFifo(FillItem, .Dynamic);

const string = []const u8;
const stringZ = [:0]const u8;

Expand All @@ -786,7 +846,9 @@ const OOM = bun.OOM;
const Output = bun.Output;
const Path = bun.path;
const assert = bun.assert;
const glob = bun.glob;
const logger = bun.logger;
const ArrayListDefault = bun.collections.ArrayListDefault;
const Bitset = bun.bit_set.DynamicBitSetUnmanaged;
const String = bun.Semver.String;

Expand Down
90 changes: 90 additions & 0 deletions test/cli/install/nohoist.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import { afterAll, beforeAll, describe, expect, test } from "bun:test";
import { exists } from "fs/promises";
import { VerdaccioRegistry, bunEnv, readdirSorted, runBunInstall } from "harness";
import { join } from "path";

var registry = new VerdaccioRegistry();

beforeAll(async () => {
await registry.start();
});

afterAll(() => {
registry.stop();
});

describe("workspaces.nohoist", () => {
test("basic", async () => {
const { packageDir } = await registry.createTestDir({
files: {
"package.json": JSON.stringify({
name: "basic-nohoist",
workspaces: {
nohoist: ["one-dep/no-deps"],
},
dependencies: {
"one-dep": "1.0.0",
},
}),
},
});

await runBunInstall(bunEnv, packageDir);

expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["one-dep"]);
});
Comment on lines +17 to +35

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Incomplete test validation - verify nohoist behavior.

The test configures nohoist: ["one-dep/no-deps"] but only checks that "one-dep" exists in the root node_modules. It doesn't verify the actual nohoist behavior - that "no-deps" is kept nested within "one-dep/node_modules" instead of being hoisted to the root.

Apply this diff to add the missing assertion:

   await runBunInstall(bunEnv, packageDir);

   expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["one-dep"]);
+  expect(await readdirSorted(join(packageDir, "node_modules/one-dep/node_modules"))).toEqual(["no-deps"]);
🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 17 to 35, the test only asserts
that "one-dep" exists at the root but doesn't verify nohoist semantics for
"one-dep/no-deps"; update the test to assert that "no-deps" is present under
join(packageDir, "node_modules", "one-dep", "node_modules") and that "no-deps"
is not present in the root node_modules (i.e., ensure
readdirSorted(join(packageDir, "node_modules", "one-dep", "node_modules"))
includes "no-deps" and readdirSorted(join(packageDir, "node_modules")) does not
include "no-deps").


test("can keep package in workspace node_modules", async () => {
const { packageDir } = await registry.createTestDir({
files: {
"package.json": JSON.stringify({
name: "workspace-nohoist",
workspaces: {
packages: ["packages/*"],
nohoist: ["**/one-dep"],
},
dependencies: {
"a-dep": "1.0.1",
},
}),
"packages/pkg1/package.json": JSON.stringify({
name: "pkg1",
dependencies: {
"one-dep": "1.0.0",
},
}),
},
});

await runBunInstall(bunEnv, packageDir, { linker: "hoisted" });

expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]);
expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]);
});
Comment on lines +37 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Request clarification on test package dependencies.

The test expects "no-deps" to appear in the root node_modules, but it's not declared as a direct dependency. Please add a comment explaining the dependency graph (e.g., whether "no-deps" is a transitive dependency of "a-dep" or "one-dep") to make the test expectations clearer.

Example:

  test("can keep package in workspace node_modules", async () => {
+   // Dependency graph: a-dep@1.0.1 -> no-deps, pkg1 -> one-dep@1.0.0 -> no-deps
+   // nohoist pattern "**/one-dep" keeps one-dep in workspace node_modules
    const { packageDir } = await registry.createTestDir({
📝 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
test("can keep package in workspace node_modules", async () => {
const { packageDir } = await registry.createTestDir({
files: {
"package.json": JSON.stringify({
name: "workspace-nohoist",
workspaces: {
packages: ["packages/*"],
nohoist: ["**/one-dep"],
},
dependencies: {
"a-dep": "1.0.1",
},
}),
"packages/pkg1/package.json": JSON.stringify({
name: "pkg1",
dependencies: {
"one-dep": "1.0.0",
},
}),
},
});
await runBunInstall(bunEnv, packageDir, { linker: "hoisted" });
expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]);
expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]);
});
test("can keep package in workspace node_modules", async () => {
// Dependency graph: a-dep@1.0.1 -> no-deps, pkg1 -> one-dep@1.0.0 -> no-deps
// nohoist pattern "**/one-dep" keeps one-dep in workspace node_modules
const { packageDir } = await registry.createTestDir({
files: {
"package.json": JSON.stringify({
name: "workspace-nohoist",
workspaces: {
packages: ["packages/*"],
nohoist: ["**/one-dep"],
},
dependencies: {
"a-dep": "1.0.1",
},
}),
"packages/pkg1/package.json": JSON.stringify({
name: "pkg1",
dependencies: {
"one-dep": "1.0.0",
},
}),
},
});
await runBunInstall(bunEnv, packageDir, { linker: "hoisted" });
expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual(["a-dep", "no-deps", "pkg1"]);
expect(await readdirSorted(join(packageDir, "packages/pkg1/node_modules"))).toEqual(["one-dep"]);
});
🤖 Prompt for AI Agents
test/cli/install/nohoist.test.ts around lines 37 to 63: the test asserts that
"no-deps" is present in the root node_modules but the test data doesn't document
why that package appears; add an inline comment above the registry.createTestDir
call (or directly above the expectations) that explains the dependency
graph—specifically state which package (a-dep or one-dep) depends on "no-deps"
transitively and therefore why it should be hoisted into the root
node_modules—so future readers understand why the expectation includes
"no-deps".


test("handles cycles", async () => {
const { packageDir } = await registry.createTestDir({
files: {
"package.json": JSON.stringify({
name: "cycles",
workspaces: {
nohoist: ["**"],
},
dependencies: {
"a-dep-b": "1.0.0",
},
}),
},
});

await runBunInstall(bunEnv, packageDir, { linker: "hoisted" });

expect(
await Promise.all([
readdirSorted(join(packageDir, "node_modules")),
readdirSorted(join(packageDir, "node_modules/a-dep-b/node_modules")),
exists(join(packageDir, "node_modules/a-dep-b/node_modules/b-dep-a/node_modules")),
]),
).toEqual([["a-dep-b"], ["b-dep-a"], false]);
});
Comment on lines +65 to +89

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Good edge case coverage for circular dependencies.

The test correctly validates nohoist behavior with circular dependencies using the "**" pattern. Consider adding a comment explaining the circular dependency structure for better test documentation.

Example:

  test("handles cycles", async () => {
+   // Tests circular dependency: a-dep-b -> b-dep-a -> a-dep-b
+   // nohoist "**" prevents hoisting, but cycle must be broken to avoid infinite nesting
    const { packageDir } = await registry.createTestDir({
🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 65 to 89, add a short
explanatory comment above the "handles cycles" test that documents the circular
dependency topology being exercised (e.g., package depends on a-dep-b, a-dep-b
pulls in b-dep-a, and b-dep-a creates the cycle) and why the "**" nohoist
pattern is used; keep the comment concise (1–2 sentences) and place it directly
above the test block so future readers understand the edge case being validated.

});
Comment on lines +16 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add remaining test cases per PR objectives.

The PR objectives mention a TODO checklist that includes additional test scenarios. Current test coverage includes basic, workspace, and cycle cases, but consider adding:

  • Tests for invalid nohoist patterns
  • Tests with linker: "isolated"
  • Tests for root package nohoist patterns
  • Tests matching all packages
  • Error handling cases

These additional tests would help reveal remaining bugs as mentioned in the PR description.

Would you like me to help generate additional test cases to cover these scenarios?

🤖 Prompt for AI Agents
In test/cli/install/nohoist.test.ts around lines 16 to 90, add the remaining
test cases referenced in the PR TODO: create new tests for invalid nohoist
patterns (assert install fails or emits specific error), run at least one
install with linker: "isolated" to verify nohoist behavior under isolated
linking, add a test for root-package nohoist patterns (nohoist entries that
target the root and assert correct placement), add a test that uses a pattern
matching all packages (e.g., "**") to confirm global nohoist behavior, and add
error-handling tests (malformed package.json or conflicting nohoist entries)
that assert proper error messages; implement each test following the existing
pattern: use registry.createTestDir to create fixture files, call runBunInstall
with the proper bunEnv and linker option, then assert node_modules layout or
error output using readdirSorted/exists/expect.

4 changes: 4 additions & 0 deletions test/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1225,6 +1225,7 @@ export async function runBunInstall(
saveTextLockfile?: boolean;
packages?: string[];
verbose?: boolean;
linker?: "hoisted" | "isolated";
} = {},
) {
const production = options?.production ?? false;
Expand All @@ -1235,6 +1236,9 @@ export async function runBunInstall(
if (production) {
args.push("--production");
}
if (options?.linker) {
args.push(`--linker=${options?.linker}`);
}
if (options?.frozenLockfile) {
args.push("--frozen-lockfile");
}
Expand Down