install: fail a lifecycle script of a package that a required and an optional dependency share

[robobun]

Fixes #43211. Stacked on #43197: the base is that PR's branch, and this PR reuses its RequiredPackages. Found by audit, like #43197. No user report exists. The two PRs can merge as one if that is easier.

Problem

• bun install exits 0 when a trusted package's lifecycle script fails, if the root lists the package in optionalDependencies and a required dependency also needs it. The expected output is error: postinstall script from "baz" exited with 1 and exit 1. Both linkers have the bug.
• The optional flag of spawn_package_lifecycle_scripts came from the one dependency that placed the package. optionalDependencies sort first, so that dependency was the root's optional one (src/install/PackageInstaller.rs, the two enqueue_lifecycle_scripts calls, and the RunScripts arm in src/install/PackageManager/runTasks.rs).

Fix

• Both linkers ask RequiredPackages::contains (from install: fail when an optional and a required dependency share a failed download #43197) whether a linked dependency without the OPTIONAL bit resolves to the package. Only then is the script optional.
• The isolated linker keeps the RequiredPackages in the Installer struct. The install loop and the RunScripts arm of run_tasks share it, so the walk runs at most once per install.
• The exit code now depends on the graph only. A non-optional edge from an optional parent makes the package required, as in install: fail when an optional and a required dependency share a failed download #43197 and as the released build already does for that shape (see Notes).
• Verified: test/cli/install/bun-install-lifecycle-scripts.test.ts, two tests per linker plus one for the hoisted already-installed path. The share tests fail on the released build and pass with the fix. Also ran isolated-install, bun-install-retry and bun-install-offline. Self-reviewed: 2 concerns raised, 1 addressed (the note above), 1 rejected (see Notes).

Background

• A lifecycle script runs after the linker places the package. If the script fails and optional is set, LifecycleScriptSubprocess deletes the package and the install continues. If not, the install prints the error and exits with the script's code.
• Both linkers place a package once for every dependency on it. The hoisted tree stores the dependency that owns the node_modules slot. The isolated store node stores dep_id, the first dependency that created it.
• RequiredPackages (src/install/lockfile/Tree.rs) walks the lockfile from the root, with the same filters the linkers use, and marks each package that a non-optional edge reaches.

Notes

Tested with the script from the issue and with the new tests. Before the fix, with the debug build:

$ bun repro.ts hoisted    -> exit 0, no error line
$ bun repro.ts isolated   -> exit 0, no error line

After the fix, both print error: postinstall script from "baz" exited with 1 and exit 1.

Optional parent with a required edge. bar requires baz, baz has the failing script and is trusted.

root	released hoisted	released isolated	this PR
dependencies: {bar}, optionalDependencies: {baz} (the issue)	exit 0	exit 0	exit 1
optionalDependencies: {bar}	exit 1	exit 1	exit 1
optionalDependencies: {bar, baz}	exit 0	exit 1	exit 1

The released build answers from the dependency that placed the package, so the last row differs between the linkers. Now both linkers answer from the graph. A rule that follows only chains of non-optional edges from the root (npm's reading) would also change the second row, and the download path in #43197. That is a separate decision.

Why a separate PR. The self-review asked to fold this into #43197. #43197 scopes itself to downloads and names the script path as follow-up work. This PR keeps that scope and carries its own tests and its own issue. A maintainer can merge the two together.

Not changed, pre-existing. The isolated linker deletes a workspace folder when only an optional edge links the workspace and its script fails: #43221. The walk in RequiredPackages does not enter the subtree of a peer edge, so a package reached only through a peer is not marked required (the is_peer() skip in #43197). The isolated linker does not run the scripts of an already installed package that a later install trusts.

The full bun-install-lifecycle-scripts.test.ts file has 3 failures in this container (node -p should work in postinstall scripts and ensureTempNodeGypScript works). They fail because the tests clear PATH and the debug binary is not named bun. They pass with the released build and do not touch this change.

---

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts

[robobun]

^{Updated 8:38 PM PT - Sep 17th, 2026}

✅ @robobun, your commit 5727d399ea6b24385e7f4f0da487c837153d56a8 passed in Build #117495! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43218

That installs a local version of the PR into your bun-43218 executable, so you can run:

bun-43218 --bun

[claude]

Beyond the inline findings, I also checked the new Installer.required_packages field for aliasing: installer.manager() returns &'a PackageManager untied to the &self borrow, so the &mut call on the field compiles cleanly, and mutating it from the main thread while pool tasks hold a BackRef<Installer> follows the same pattern as the existing main-thread-only waiters_head/installed fields. The hoisted sites reuse the required_packages field already on PackageInstaller (line 94), so both linkers share one walker per install.

Extended reasoning...

Inline findings are posted, so this note only records what else was examined. The RequiredPackages::contains(&mut self, &PackageManager, ...) signature looked like a potential borrow conflict at installer.required_packages.contains(installer.manager(), ...), but Installer::manager() derefs a raw backref and returns &'a PackageManager, not &self-bound, so the three new call sites (PackageInstaller.rs:1992 and :2305, runTasks.rs:383) compile without aliasing. The worker-thread concern was also checked: Task holds BackRef<Installer<'static>> and forms &Installer on pool threads, but the struct already documents waiters_head, installed and summary as main-thread-only and mutates them the same way, so the new field does not introduce a new pattern. The lazy walk in Tree.rs:656-665 re-walks only when lockfile.packages.len() changes, so worst case is one extra walk per appended package.

Findings marked 🟡 are optional suggestions and need no follow-up push.

[robobun]

Addressed the review. The hoisted already-installed path now has a test (d3bb0f4). The exit-code table for the optional-parent shapes is in the PR body. The workspace deletion is pre-existing and tracked in #43221.

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[robobun]

Addressed the stdout nit in 83bb55b. No source change.

[claude]

I reviewed the latest push and found no bugs; the two test points from the previous round (the hoisted already-installed path, and draining stdout / asserting the blocked first install) are addressed by d3bb0f4 and 83bb55b. A maintainer look is still worthwhile because the PR intentionally changes the exit code for the optional-parent-with-required-edge shape on the hoisted linker (last row of the Notes table), which is a policy call rather than a code question.

What was reviewed:

• The three optional sites now route through RequiredPackages::contains; the non-optional early return and the re-walk on lockfile growth (Tree.rs:649-666) mean the hoisted sites and the isolated RunScripts arm agree with the download-side is_required.
• Moving RequiredPackages onto Installer — contains takes &mut self and is only called from the main-thread install loop and run_tasks; manager() returns &'a, so the borrow compiles without aliasing the pool's raw-pointer access.
• Fixtures no-deps-scripted-to-fail / no-deps-scripted-to-deeply-fail exist in the local registry with the expected dependency edge; the tests assert exact error lines before the exit code and cover both linkers plus the negative (optional-only) case.

Extended reasoning...

Overview

The change replaces the per-edge Behavior::OPTIONAL check with RequiredPackages::contains at the two hoisted enqueue_lifecycle_scripts sites in src/install/PackageInstaller.rs and in the isolated linker's RunScripts arm in src/install/PackageManager/runTasks.rs. To make the latter possible, the RequiredPackages that install_isolated_packages previously kept as a local moves onto Installer (src/install/isolated_install/Installer.rs), and the existing is_required computation reads it through the field. Dead dep_behavior locals and the Behavior import are removed. Tests in test/cli/install/bun-install-lifecycle-scripts.test.ts cover the shared optional+required package for both linkers, the optional-only negative case for both linkers, and the hoisted already-installed path.

Security risks

None specific to this change. The code decides whether a failing lifecycle script aborts the install; it does not introduce new paths, parsing of untrusted data, or script execution that was not already gated by trustedDependencies. If anything, the change fails closed more often (a required package's failing script now fails the install rather than being silently deleted).

Level of scrutiny

Moderate. The code delta is small and mechanical, and I traced the borrow shape (&mut installer.required_packages with installer.manager() returning &'a PackageManager via the BACKREF accessor) and the thread affinity (both call sites are main-thread; the pool tasks never touch the new field, consistent with the "Main-thread only" doc on the neighbouring waiters_head field). The contains helper's early return for non-optional asking edges and its re-walk when lockfile.packages.len() changes make the three sites consistent with the download-side is_required from the base PR. What warrants a human is not the code but the policy: the PR's own Notes table shows the hoisted linker's exit code changing for optionalDependencies: {bar, baz} where bar requires baz, and the author explicitly leaves the npm-style "chain of non-optional edges" interpretation as a separate decision. That is a maintainer judgement about bun install semantics, and it was the subject of my earlier inline thread, which the author resolved with a note rather than a code change.

Other factors

Both nits from the previous review round were addressed in later commits: a hoisted variant that first installs blocked and then trusts the package (exercising the PackageInstaller.rs:2305 site), and reading stdout at every new spawn with an assertion on Blocked 1 postinstall so the second install is known to hit the already-installed path. The fixtures used by the tests exist in the local verdaccio registry with the expected dependency edge. The bug-hunting system exited with dry_streak and reported no findings. I did not build the debug binary in this run, so I did not re-execute the tests locally; the PR states they fail on the released build and pass with the fix.

[robobun]

No further changes from this round. The policy point (the last row of the table in Notes) is for a maintainer: this PR keeps the rule from #43197, where a non-optional edge makes the package required whatever its parent is.