install: don't fail on an unresolved optional dependency with an empty name

[robobun]

Closes #31652

Repro

bun install -g @openai/codex on 1.4.0-canary aborts with:

error: Invalid dependency name ""
error: Invalid dependency name ""
error: Invalid dependency name ""

On 1.3.x the same command installs fine. Minimal reduction — a package whose manifest declares an optional dependency under an empty key whose target doesn't resolve on the current platform:

// manifest for top@1.0.0
{ "optionalDependencies": { "": "1.0.0" } }   // target "" 404s / is platform-filtered

$ bun install          # 1.3.14 → exit 0, optional dep skipped
$ bun install          # 1.4.0-canary → error: Invalid dependency name "", exit 1

Cause

When a dependency alias (the key) is empty, the real package name is only substituted back into Dependency.name if the dependency resolves (assign_resolution / assign_root_resolution in PackageManagerResolution.rs). An optional dependency that fails to resolve keeps pkg_id == invalid_package_id and an empty name.

That empty-named dependency then reaches the hoisting tree builder, where a name-safety check added in #31221 (src/install/lockfile/Tree.rs) treats the empty name as unsafe and calls maybe_report_error("Invalid dependency name \"\""), aborting the install. The original Zig implementation had no such check here, which is why 1.3.x works.

The check was also added inconsistently: the two sibling call sites already guard the empty case —

• src/install/lockfile/bun.lock.rs: if !name_str.is_empty() && !is_safe_install_folder_name(name_str)
• src/install/isolated_install.rs: if !name.is_empty() && !is_safe_install_folder_name(name)

— while Tree.rs was the only one missing !name.is_empty().

Fix

Guard !name.is_empty() before the safety check in Tree.rs, matching the sibling call sites. An empty alias has no node_modules/<name> folder to escape, so it is skipped like an unresolved optional dep should be, instead of failing the whole install. Genuinely unsafe non-empty names (e.g. .., ../..) still error exactly as before.

Verification

test/regression/issue/31652.test.ts serves a package with optionalDependencies: { "": "1.0.0" } whose target 404s and asserts the install completes without Invalid dependency name "" and with exit 0.

• Fails on main: error: Invalid dependency name "", exit 1.
• Passes with this change.
• The existing alias-path-traversal rejection tests (rejects dependency aliases containing '..' path segments, does not create a cache index entry outside the cache directory for a dependency alias of '..', and the two sibling '..' tests) still pass — the empty-name guard does not weaken the security checks.

[robobun]

^{Updated 4:32 AM PT - Jun 1st, 2026}

✅ @robobun, your commit 17c6b90ca3e8a9be96e5acd8164f48fc0483852e passed in Build #59543! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 31655

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

bun-31655 --bun

[coderabbitai]

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 42 minutes and 42 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c39e9aee-a062-409a-978d-b06591504e23

📥 Commits

Reviewing files that changed from the base of the PR and between 8e89863 and 17c6b90.

📒 Files selected for processing (2)

• src/install/lockfile/Tree.rs
• test/regression/issue/31652.test.ts

Walkthrough

This PR fixes a bug where bun install -g would abort with "Invalid dependency name """ when encountering optional dependencies with empty names. The fix skips the folder-name safety validation for empty dependency names in the lockfile processing, and adds a regression test that verifies the install succeeds in this scenario.

Changes

Empty dependency name handling

Layer / File(s)	Summary
Dependency name safety check adjustment src/install/lockfile/Tree.rs	The folder-name safety validation in Tree::process_subtree is now conditional: it computes dependency_name, skips the is_safe_install_folder_name check when the name is empty, and only validates non-empty names.
Regression test for empty optional dependency resolution test/regression/issue/31652.test.ts	A new regression test verifies that bun install succeeds (exit code 0) and does not abort when encountering an optional dependency with an empty package name that cannot be resolved. It confirms the target package is still installed and stderr contains no invalid dependency name error.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: preventing failures when encountering an unresolved optional dependency with an empty name during installation.
Description check	✅ Passed	The description covers the repro case, root cause analysis, fix explanation, and verification approach with comprehensive detail and code citations.
Linked Issues check	✅ Passed	The PR successfully addresses issue #31652 by adding the missing empty-name guard before the safety check in Tree.rs to prevent aborting on unresolved optional dependencies with empty aliases.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the issue: modifying the safety check logic in Tree.rs and adding a regression test verifying the fix behavior.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/issue/31652.test.ts`:
- Around line 120-122: Move the exit code assertion to the end of the test so
behavioral failures are reported first: place the expect(exitCode).toBe(0) after
the sanity check expect(emptyNameManifestRequested).toBe(true). Update the test
that references variables exitCode and emptyNameManifestRequested so the
emptyNameManifestRequested assertion runs before the exitCode assertion.

🪄 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: 9ecc6ef8-4b9b-4e12-a883-256a901784fa

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac120c and 8e89863.

📒 Files selected for processing (2)

• src/install/lockfile/Tree.rs
• test/regression/issue/31652.test.ts

[claude]

LGTM — minimal one-line guard that brings Tree.rs in line with the two sibling call sites, with a solid regression test.

Extended reasoning...

Overview

This PR adds a !dependency_name.is_empty() && guard before the is_safe_install_folder_name check in src/install/lockfile/Tree.rs:process_subtree, and adds a regression test (test/regression/issue/31652.test.ts) that serves a package with optionalDependencies: { "": "1.0.0" } from a mock registry and verifies the install completes. The production change is effectively one added condition plus an explanatory comment; the rest is hoisting the name slice into a local.

Security risks

None. The modified check is the path-traversal guard from #31221; this change only exempts the empty string, which cannot produce a node_modules/<name> folder to escape. I verified the two sibling call sites the PR description cites already use the identical pattern — bun.lock.rs:2363 (!name_str.is_empty() && !is_safe_install_folder_name(...)) and isolated_install.rs:2147 (!name.is_empty() && !is_safe_install_folder_name(...)) — so this is a consistency fix, not a new exemption. Genuinely unsafe non-empty names (.., a/../b, etc.) still fail the check exactly as before, and is_safe_install_folder_name itself (dependency.rs:583) is unchanged.

Level of scrutiny

Low-to-medium. bun install is production-critical, but the change is a one-condition guard matching established code at two other sites, restores 1.3.x behavior that regressed on canary, and is covered by a targeted regression test that fails on main and passes with the fix. No CODEOWNERS entries cover src/install/.

Other factors

• The only reviewer feedback (CodeRabbit: assert exitCode last) was addressed in b669815 and the thread is resolved.
• The CI failures reported by robobun are musl build-cpp and -no-pie linker warnings in build-rust — infrastructure noise unrelated to a Rust source change and a TS test file.
• The PR description includes a clear root-cause analysis (empty alias only gets its real name substituted after resolution; unresolved optional deps keep the empty name and hit the new check) and confirms the existing .. path-traversal rejection tests still pass.
• No bugs were found by the bug hunting system.

[claude]

I didn't find any issues — the empty-name guard matches the sibling call sites in bun.lock.rs and isolated_install.rs and the path-traversal tests still cover non-empty unsafe names — but since this relaxes a path-safety check in the hoisted install path, it's worth a quick human sanity check.

Extended reasoning...

Overview

This PR adds a single !dependency_name.is_empty() && guard before the is_safe_install_folder_name check in Tree::process_subtree (src/install/lockfile/Tree.rs), and adds a regression test (test/regression/issue/31652.test.ts). The fix restores 1.3.x behavior where an unresolved optional dependency declared under an empty key is silently skipped rather than aborting the whole install with error: Invalid dependency name "".

I verified the PR's claim that the two sibling call sites already carry this exact guard: bun.lock.rs:2363 (!name_str.is_empty() && !dependency::is_safe_install_folder_name(...)) and isolated_install.rs:2147 (!name.is_empty() && !crate::dependency::is_safe_install_folder_name(...)). is_safe_install_folder_name itself returns false for empty input, which is why the unguarded call site rejected it.

Security risks

This change narrows a path-traversal guard, which is the security-relevant part. The exemption is limited to the empty string, which cannot escape node_modules/<name> (there is no folder segment to traverse with), and in the motivating case (pkg_id == invalid_package_id, non-peer optional) the very next branch is continue 'dep so nothing is placed. Non-empty unsafe names like .. / ../.. still hit maybe_report_error exactly as before, and relative_path_and_depth retains its own folder_name_is_safe crash-guard for any tree node that does get placed. So I don't see a traversal vector being opened here, but it is still a relaxation of a safety check.

Level of scrutiny

Medium-high. The diff is tiny and mechanically consistent with established code, but it sits in the hoisted-install tree builder — a hot, security-adjacent path that runs on every bun install. Per my guidelines I don't auto-approve relaxations of path-safety / install-folder validation even when they look obviously correct.

Other factors

• Not covered by CODEOWNERS.
• The regression test is well-constructed (mock registry, asserts the empty-name manifest was actually requested, asserts top still installs, exit code last) and the one CodeRabbit nit (assert exitCode last) was addressed in b669815.
• The single CI failure (streams-leak.test.ts on x64) is unrelated to install.
• The bug-hunting system found nothing.