install(windows): remove .bunx/.exe shims on bun remove

[robobun]

Fixes #11970.

Problem

On Windows, bun remove (both global and local) removes the package's node_modules entry but leaves its <name>.exe and <name>.bunx shim files behind in the bin directory. The shim then launches a binary whose target no longer exists:

PS> bun add -g cowsay
PS> bun remove -g cowsay
PS> cowsay hello
Error: Cannot find module 'C:\Users\User\.bun\install\global\node_modules\cowsay\cli.js'

POSIX is unaffected because package bins there are symlinks and the dangling-symlink sweep already removes them.

Cause

update_package_json_and_install_with_manager_with_updates finishes a remove by walking the bin directory and unlinking any entry it cannot open, but it matches only on EntryKind::SymLink. Windows bins are regular .exe / .bunx files, so the _ => {} arm skips them and they accumulate forever.

Fix

Add a Windows arm to the same sweep that recognises .bunx entries. A .bunx begins with the UTF-16LE target path (relative to the parent of the bin directory) terminated by a " code unit; the arm reads that path, joins it against dirname(bin_path), normalises (the encoded path can carry a residual .. when the global bin directory lives outside $BUN_INSTALL, and the existence check goes through an NT path), and if NtQueryAttributesFile reports ENOENT unlinks the .bunx together with its sibling .exe. Any other outcome, including a file the sweep cannot decode or a non-ENOENT error, leaves the pair in place. This covers both bun remove -g ($BUN_INSTALL/bin) and local bun remove (node_modules/.bin), and matches what npm does with its .cmd/.ps1 wrappers on uninstall.

Verification

Three new tests in test/cli/install/bun-remove.test.ts (should delete bins of the removed package plus a (global) and a (global, disjoint bin dir) variant) install two packages with three bins between them, remove one, assert the exact surviving directory listing, then remove the other and assert the bin directory is empty. The disjoint variant places BUN_INSTALL_BIN two levels deep outside $BUN_INSTALL so the encoded shim path contains .., guarding the normalisation step.

On Windows all three fail on main and pass with this change:

(fail) should delete bins of the removed package
(fail) should delete bins of the removed package (global)
(fail) should delete bins of the removed package (global, disjoint bin dir)
  error: expect(received).toEqual(expected)
    [
  +   "bin-a.bunx",
  +   "bin-a.exe",
  +   "bin-b.bunx",
  +   "bin-b.exe",
      "bin-c.bunx",
      "bin-c.exe",
    ]

On POSIX they pass on both main and this branch (the symlink sweep already handled this case), so the Linux gate cannot observe fail-before; the Rust change is entirely #[cfg(windows)]-gated and is a no-op there. The Windows CI lane is the check of record.

Also confirmed the original report end to end with a Windows debug build: bun add -g cowsay && bun remove -g cowsay now leaves $BUN_INSTALL\bin empty.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-remove.test.ts

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 19 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 786707d8-9ecd-4048-b0f8-49caccb5ac69

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and 870f3b4.

📒 Files selected for processing (2)

• src/install/PackageManager/updatePackageJSONAndInstall.rs
• test/cli/install/bun-remove.test.ts

---

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

[robobun]

^{Updated 9:22 AM PT - Jul 25th, 2026}

⏳ @robobun, your commit 870f3b4 is still building in Build #80712, but has 1 failures so far (All Failures):

• step failed outside runner - exit 1 on 🐧 aarch64-android - build-bun
• step failed outside runner - exit 1 on 🐧 aarch64-musl - build-bun
• step failed outside runner - exit 1 on :freebsd: aarch64 - build-bun
• step failed outside runner - exit 1 on 🍎 x64 - build-bun
• step failed outside runner - exit 1 on 🪟 aarch64 - build-bun

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/install/PackageManager/updatePackageJSONAndInstall.rs:828 — The final .is_err() treats any exists_at_type_w failure as "target missing", so a non-ENOENT status (e.g. STATUS_ACCESS_DENIED, STATUS_OBJECT_NAME_INVALID) would irreversibly unlink a still-valid .bunx/.exe pair — contradicting the doc comment's "anything we cannot decode is treated as live". Consider matching only e.get_errno() == E::ENOENT and returning false for every other error, mirroring directory_exists_at_w in src/sys/lib.rs. Low practical impact (attribute queries on Bun-managed node_modules paths rarely fail with anything but not-found, and the pre-existing SymLink arm above uses the same pattern), so not blocking.

  Extended reasoning...

  What the bug is

  is_dangling_windows_shim ends with:

  bun_sys::exists_at_type_w(Fd::cwd(), &target_buf[..total]).is_err()

  exists_at_type_w wraps NtQueryAttributesFile and returns Err for any non-success NTSTATUS, mapped through translate_nt_status_to_errno (src/sys/lib.rs:7287-7366). That includes STATUS_ACCESS_DENIED → EACCES, STATUS_OBJECT_NAME_INVALID → EINVAL, etc. — not just STATUS_OBJECT_NAME_NOT_FOUND / STATUS_OBJECT_PATH_NOT_FOUND → ENOENT. Because the caller keys deletion on .is_err(), any of those non-ENOENT errors makes the function report the shim as dangling, and the caller then unlinks both <name>.bunx and <name>.exe even though the target script still exists.

  Code path

  1. bun remove iterates the bin dir and hits a .bunx entry (updatePackageJSONAndInstall.rs:724-758).
  2. is_dangling_windows_shim opens and decodes the shim, builds <dirname(bin_path)>\<rel_target>, and calls exists_at_type_w.
  3. exists_at_type_nt invokes NtQueryAttributesFile; suppose it returns STATUS_ACCESS_DENIED (e.g. an AV/EDR filter transiently blocks the attribute query, or an ACL denies FILE_READ_ATTRIBUTES on the target).
  4. The result is Err(EACCES), so .is_err() is true → function returns true.
  5. Caller unlinks foo.bunx and foo.exe, even though node_modules/foo/cli.js still exists and foo is still installed.

  Why nothing else prevents it

  Every earlier guard in is_dangling_windows_shim (open failure, read failure, no " terminator, empty target, oversized path) returns false — i.e. conservative/live — matching the doc comment: "Anything we cannot decode is treated as live." Only the final existence check inverts that convention by treating an undecidable answer as "gone". The sibling helper directory_exists_at_w (src/sys/lib.rs:7381-7386) demonstrates the intended pattern: Err(e) if e.get_errno() == E::ENOENT => Ok(false), propagating every other error. REVIEW.md's error-handling rule is on point: "Map only the specific expected errno (ENOENT) to the benign path; everything else stays loud."

  Step-by-step proof

  Take bin-c.bunx from the PR's own test with has-one-bin still installed. Its decoded target is …\node_modules\has-one-bin\bin-c.js, which exists. If NtQueryAttributesFile on that path returned 0xC0000022 (STATUS_ACCESS_DENIED) instead of STATUS_SUCCESS:

  • exists_at_type_nt → Err(Error { errno: EACCES, tag: NtQueryAttributesFile })
  • .is_err() → true
  • is_dangling_windows_shim → true
  • caller unlinks bin-c.bunx and bin-c.exe

  The user then has has-one-bin in node_modules and the lockfile but no launcher on $PATH — the exact symptom this PR set out to fix, only in reverse.

  Impact and severity

  This is a nit, not a blocker, for three reasons: (1) the pre-existing SymLink arm directly above uses the identical Err(_) => unlinkat pattern, so this PR is following local convention rather than introducing a new anti-pattern; (2) NtQueryAttributesFile does not open the file (no sharing violations), and the target lives under a Bun-created node_modules in the user's own tree, so non-ENOENT failures are rare in practice; (3) the consequence is recoverable — bun install recreates the shims. Nothing here would justify holding merge.

  Fix

  Match on the errno and stay conservative for anything other than not-found:

  match bun_sys::exists_at_type_w(Fd::cwd(), &target_buf[..total]) {
      Ok(_) => false,
      Err(e) => e.get_errno() == bun_sys::E::ENOENT,
  }

  This matches directory_exists_at_w, honors the function's own doc comment, and keeps the destructive path gated on the one status we actually expect.

[claude]

Both earlier review points are addressed — the bin-dir assertions are now exact toEqual checks, and is_dangling_windows_shim deletes only on ENOENT. No new issues found this pass. Deferring to a human because the Windows-only path is destructive (hand-decodes the .bunx header, rebuilds an NT path via dirname→UTF-16→normalize, then unlinks) and the check of record is the Windows CI lane.

What was reviewed:

• Confirmed the .bunx header layout against BinLinkingShim::encode_into (bin_path terminated by u16 '"'), and that File closes on drop so is_dangling_windows_shim doesn't leak the fd.
• binEntries([...]) yields sorted output on both platforms, so readdirSorted().toEqual(...) holds.
• comment-cop flagged the 2-line normalize comment on the latest commit; looks like a false positive but noting it's still open.

Extended reasoning...

Overview

Adds a #[cfg(windows)] arm to the post-remove bin-directory sweep in updatePackageJSONAndInstall.rs that recognises <name>.bunx files, decodes the UTF-16LE target path from the shim header, checks whether the target still exists, and if not unlinks the .bunx and its sibling .exe. Two new tests in bun-remove.test.ts (local + global) install two packages with three bins, remove one, assert the survivor bins remain, then remove the other and assert the bin dir is empty.

Prior feedback addressed

Commit 47e88b2 replaced the vacuous toHaveBins assertions with exact toEqual(binEntries([...])) checks (my 🔴), and switched the dangling check from .is_err() to matching only ENOENT (my 🟡). Commit 1b8fad6 additionally normalises the reconstructed path with normalize_buf_t before the NT existence check to handle encoded targets containing ... Both fixes look correct; the ENOENT match now mirrors the in-tree directory_exists_at_w idiom, and the exact-array assertions would catch over-deletion.

Security risks

The sweep deletes files based on a path decoded from a .bunx header inside a directory the user controls; it fails closed on any parse ambiguity or non-ENOENT error, and only ever unlinks entries within the already-open bin dir (the reconstructed path is used only for the existence probe). No credential, network, or auth surface.

Level of scrutiny

Medium-high. This is package-manager code that deletes files, is Windows-only (so Linux CI cannot exercise it), and hand-parses a binary format plus does non-trivial path reconstruction (dirname(bin_dir_path) → UTF-8→UTF-16 → append encoded relative path → normalize → NT existence check). I verified the header-terminator assumption against BinLinkingShim::encode_into (writes bin_path then b'"' as u16), that bun_sys::File closes on Drop so the openat in is_dangling_windows_shim doesn't leak, and that the survivor assertion is now non-vacuous. But whether dirname(bin_path) is the right base for the encoded relative path in both the local (node_modules/.bin → node_modules) and global ($BUN_INSTALL/bin → $BUN_INSTALL) cases depends on how bin.rs constructs bin_path at link time on Windows, which is worth a human confirming against the Windows CI results.

Other factors

comment-cop left a fresh flag on line ~821 of the Rust file after the latest commit; the flagged comment is a 2-line explanation of why normalisation is needed before the NT-path existence check, which reads as durable non-obvious content rather than a workaround justification — likely a heuristic false positive, but it is still open on the latest revision.

[claude]

Both earlier findings are addressed — the survivor assertion now uses an exact toEqual on sorted dir contents, and the dangling check fails closed on anything other than ENOENT. I also cross-checked the .bunx decode against BinLinkingShim::encode_into / bin.rs::create_windows_shim (bin_path is written relative to dirname(bin_dir) with the leading ..\ pre-stripped, matching the dirname(bin_dir_path) base here), and confirmed File closes on drop so the per-.bunx open does not leak. Deferring the final call to a human since this is ~90 lines of Windows-only path reconstruction that unlinks files in the user's global bin dir — worth a maintainer glance at the Windows CI lane before merge.

Extended reasoning...

Overview

Adds a #[cfg(windows)] EntryKind::File arm to the post-remove bin-dir sweep in updatePackageJSONAndInstall.rs, plus a ~50-line is_dangling_windows_shim helper that opens each *.bunx, decodes the leading UTF-16LE target path (terminated by u16 '"'), joins it against dirname(options.bin_path), normalizes, and treats the shim as dangling only when exists_at_type_w returns ENOENT. Three new test variants in bun-remove.test.ts (local, global, and global-with-disjoint-BUN_INSTALL_BIN) install two packages with three bins, remove one, and assert exact sorted bin-dir contents at each step.

Prior feedback

Both of my earlier inline comments are resolved in the current revision: (1) the vacuous toHaveBins assertions are replaced with readdirSorted(...).toEqual(binEntries([...])), so an over-deleting sweep now fails the bin-c survivor check; (2) .is_err() was replaced with an explicit Err(e) if e.get_errno() == ENOENT match, so ACCESS_DENIED / OBJECT_NAME_INVALID / etc. leave the shim in place. The follow-up commits also added normalize_buf_t before the NT-path check to handle residual .. in the encoded target, with a dedicated test variant that exercises it.

What I checked this pass

Verified the decode matches the encoder: BinLinkingShim::encode_into writes bin_path at offset 0 followed by '"' as u16, and create_windows_shim builds bin_path as relative(dirname(abs_dest), abs_target) with the leading ..\ stripped — i.e. relative to the parent of the bin dir — which is exactly what is_dangling_windows_shim reconstructs via dirname(bin_dir_path). options.bin_path is the relative literal node_modules/.bin for local installs (resolved against Fd::cwd(), same base the sweep opens the dir with) and an absolute path for global installs, so both routes resolve consistently. bun_sys::File has a Drop that closes the handle, so no fd leak from the per-entry openat. Windows filenames cannot contain ", so the byte-pair scan for [0x22, 0x00] cannot terminate early inside a valid encoded path.

Security risks

Low. The only input to the parser is a .bunx file Bun itself wrote; malformed content (no terminator, zero-length path, oversize) returns false and leaves the file alone. The destructive operation is unlinkat scoped to the already-open bin-dir fd, and only fires on a confirmed ENOENT for the reconstructed target.

Level of scrutiny / why not auto-approving

This is not a mechanical change — it is new Windows-only path-reconstruction code that deletes files from the user's global $BUN_INSTALL/bin, and its correctness hinges on the .bunx format contract and Windows path semantics that only the Windows CI lane can exercise. The logic looks right and now fails closed, but a maintainer familiar with the Windows shim should confirm the Windows lane is green before merging.

[claude]

Both earlier concerns are addressed — the dangling check now matches only ENOENT and the tests assert exact sorted bin-dir contents (so over-deletion would fail). I didn't find further issues, but the hand-rolled .bunx decode + UTF-8→UTF-16 path reconstruction that drives file deletion is Windows-only and non-trivial enough that a maintainer familiar with the shim format should give it a look.

What was reviewed:

• .bunx header parse against the format doc in BinLinkingShim.rs — [WSTR:bin_path][u16:'"'] matches; base = dirname(bin_dir) matches how bun_shim_impl.rs resolves it.
• Fail-safe shape: every decode/open/read/bounds failure and every non-ENOENT errno returns false (leave shim in place).
• File closes on drop; w_path_buffer_pool guards return on drop; total >= joined.len() guards the append.
• Tests now use readdirSorted(...).toEqual(...) at all three checkpoints and cover local / global / disjoint-bin-dir with .. in the encoded path.

Extended reasoning...

Overview

The PR adds a #[cfg(windows)] arm to the post-remove bin-directory sweep in updatePackageJSONAndInstall.rs (~30 lines inline plus a ~55-line is_dangling_windows_shim helper) and three parametrised tests in bun-remove.test.ts. The helper opens each *.bunx, decodes the leading UTF-16LE target path terminated by a '"' code unit, joins it against dirname(bin_path), normalises, and treats the shim as dangling only when NtQueryAttributesFile maps to ENOENT. On a dangling verdict the caller unlinks both <name>.bunx and <name>.exe.

Prior feedback

I left two inline comments on an earlier revision: (1) toHaveBins was vacuously true on an empty array, so the survivor assertion could not catch over-deletion; (2) .is_err() on exists_at_type_w treated every non-SUCCESS NTSTATUS as "missing", risking deletion of still-installed bins on transient errors. Commit 47e88b2 replaced toHaveBins with exact readdirSorted().toEqual() at all three checkpoints and switched the existence check to Err(e) => e.get_errno() == ENOENT. Commit 1b8fad6 added normalize_buf_t before the NT-path check plus a third test variant with a disjoint BUN_INSTALL_BIN two levels deep so the encoded target carries ... Both concerns are resolved.

Security risks

None identified. The sweep only reads and unlinks entries inside a bin directory Bun itself manages; the encoded target path is used solely for an existence probe, not for opening or executing anything; and the decision defaults to "leave in place" on every unexpected condition.

Level of scrutiny

Medium-high. This is package-manager file-deletion logic gated entirely to Windows, so the Linux gate cannot observe fail-before/pass-after; the Windows CI lane is the check of record. The .bunx header parse is hand-rolled here rather than shared with bun_shim_impl.rs (which decodes via inline pointer arithmetic and is not reusable), so a maintainer who knows that format should confirm the "relative to dirname(bin_dir)" contract holds for every writer of bin_path (I traced it against the format comment and the shim's own resolution and it matches, but that is exactly the kind of cross-file invariant a human should sign off on).

Other factors

The fail-safe shape is good: open/read failures, unparseable content, zero-length target, oversized join, and non-ENOENT errors all return false, so the worst regression is the current behaviour (stale shims left behind). File has a Drop impl that closes the handle, and the two w_path_buffer_pool::get() guards return their buffers on drop. The new match arm reuses node_modules_buf and entry.name.slice_u8() exactly as the neighbouring SymLink arm does. Tests are hermetic (tmpdirSync, bunEnv, file: deps, no registry), concurrent, and assert exact directory contents on both platforms. Given the platform-specific path reconstruction driving deletion, I'm deferring rather than approving.

[robobun]

CI status: the diff is green on every lane that produced a build. linux x64, debian 13 x64, and ubuntu 25.04 x64 all built and ran the full test matrix with no failures from this change (bun-remove.test.ts passes there, exercising the no-op POSIX path). The only test annotation is test/js/bun/cron/in-process-cron.test.ts (hot-reload timeout on ubuntu x64), flagged flaky and unrelated to this diff.

The remaining red is build infrastructure: build-bun exited 1 "outside runner" on six platforms including both Windows lanes, plus two build timeouts. ci:errors flags these as pre-existing on main. The Windows x64 build did succeed on the previous run of this branch (build 80442), so the Rust change compiles there; I also verified the full bun-remove.test.ts suite (8 tests, including the three new ones) passes on a local Windows debug build and fails on released bun.

The Rust change is entirely #[cfg(windows)]-gated, so the Windows CI lane is the check of record. This is ready for a maintainer once the Windows build infra is back.