Skip to content

install: write isolated store paths as bytes - #40719

Open
robobun wants to merge 5 commits into
mainfrom
farm/1899850e/store-path-bytes
Open

robobun wants to merge 5 commits into
mainfrom
farm/1899850e/store-path-bytes

Conversation

@robobun

@robobun robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • With --linker=isolated, a dependency whose path holds non-ASCII bytes gets a mojibake store directory. file:./paquetes/añadir is installed into node_modules/.bun/anadir@file+paquetes+añadir (c3 b1 becomes c3 83 c2 b1). The cause is bun_semver's StorePathFormatter (src/semver/lib.rs:706): it spells each byte with f.write_char(n as char), which treats the byte as a Latin-1 code point and re-encodes it.
  • A second formatter for the same job, bun_install::StorePathFormatter (src/install/lib.rs:913), returns fmt::Error for a path that is not UTF-8. Its callers in Installer.rs dropped the Result with let _ = buf.append_fmt(..).

Fix

  • One byte-oriented writer replaces both: bun_semver::string::write_store_path over bun_core::io::Write. It writes the bytes unchanged, apart from /, \, :, # and ?, which become + as before. The formatters that compose an entry name (Repository, Resolution, the store key, the entry path, the global entry path) are now write_* functions over the same sink, so no &str sits between the lockfile bytes and the directory name.
  • Path gets append_with(|writer| ..): the producer writes bytes into a pooled scratch buffer, and the result is appended with the same trimming and separator rules as append. append_fmt is now built on it. Installer.rs appends store paths through it and keeps the Result (.assume_ok() on the ASSUME path types, as the type documents).
  • Correct because a store entry name is a filesystem name, not text. The same bytes that the lockfile holds now name the directory, which is what the Zig writeByte loop did. For a valid UTF-8 path the name is the UTF-8 spelling of the path, so macOS and Windows see a valid name. For a path that is not UTF-8 (legal on Linux) the name holds the same bytes as the source directory.
  • Verified: test/cli/install/isolated-install.test.ts (new store entry names with non-ASCII bytes block: UTF-8 folder, Latin-1 folder on Linux, plus the multi-byte cut test now asserts the exact name). Stock bun fails all three. Also ran the whole file (87 tests), bun-prune, bun-pm-licenses, isolated-relink, bun-install-git-deps, clippy, and a cargo check for x86_64-pc-windows-msvc.

Background

  • The isolated linker installs each package once under node_modules/.bun/<entry>/node_modules/<name> and symlinks dependents to it. <entry> is <name>@<resolution>[+<peer hash>], and the resolution part embeds a folder path, a tarball URL or a git URL for non-npm packages. The same name is computed in several places (install, prune, hashing for the global store, bun pm licenses), so they all have to spell it identically.
  • core::fmt only transports &str. A Display impl cannot emit a byte that is not part of valid UTF-8, so any fmt-based chain either re-encodes or errors on such input. bun_core::io::Write is the crate's byte sink trait (write_all(&[u8])), already used by the lockfile writers for the same reason.
  • bun_paths::Path is a pooled, fixed-capacity path buffer. append trims leading and trailing separators from its input and inserts the platform separator. The Auto* aliases skip the length check (CheckLength::ASSUME), so their Result is always Ok and .assume_ok() is the documented way to consume it.
Notes
  • Supersedes install: preserve non-ASCII UTF-8 bytes in isolated store path names #32304, which kept fmt::Display, fixed only the UTF-8 case and made non-UTF-8 input a fmt::Error.
  • Repro on bun 1.4.1 (Linux): {"dependencies":{"anadir":"file:./paquetes/añadir"}} with bun install --linker=isolated creates node_modules/.bun/anadir@file+paquetes+añadir. With a Latin-1 byte in the path (caf\xe9, package.json written as raw bytes) stock bun creates caf\xc3\xa9. After this change: caf\xe9.
  • Behavior for every ASCII input is unchanged: the git/github label and resolved values only ever went through the / and \ mapping of the deleted formatter, and both are validated ASCII (is_safe_resolved_tag). The existing exact-name tests for tarball and git entries pass unchanged.
  • An existing install with a mojibake entry self-heals: the next install computes the new name, creates it, and prune removes the old directory.
  • bun.lock writes the resolution of such a dependency with a JSON escape for code point 0 in place of the invalid byte: the JSON string printer decodes an invalid byte as 0 (same as the Zig printer). Pre-existing, not touched here.
  • Installer.rs (the package failure handler) unlinks node_modules/<entry> to force a reinstall next time, but the entry lives at node_modules/.bun/<entry>, so the cleanup has never run. This PR rewrites that block for the byte writer and keeps its target path. The path fix (prefix with NODE_MODULES_BUN, delete the tree instead of unlink) needs a test that injects a failure mid-install and is handled as a separate change.
  • Suites run: isolated-install.test.ts (87 pass), bun-pm-licenses.test.ts, bun-prune.test.ts, isolated-relink.test.ts, bun-install-git-deps.test.ts (203 pass, 2 skip), test/internal/source-lints (165 pass), cargo clippy -p bun_semver -p bun_paths -p bun_install, cargo check -p bun_install -p bun_paths --target x86_64-pc-windows-msvc.

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

The name of a store entry under node_modules/.bun is built from lockfile
strings: the package name and, for a folder, tarball, workspace or git
dependency, its path or URL. Two formatters spelled those strings and both
went through core::fmt. bun_semver's StorePathFormatter wrote each byte with
write_char(byte as char), which re-encoded every byte above 0x7F as a
Latin-1 code point, so a UTF-8 path like paquetes/añadir became
paquetes/añadir on disk. bun_install's StorePathFormatter returned
fmt::Error for a path that is not UTF-8, and its callers dropped the
Result.

Replace both with one byte-oriented writer, bun_semver::string::
write_store_path, over bun_core::io::Write, and make the whole chain that
builds an entry name (Repository, Resolution, store key, entry path) write
bytes the same way. Path gets append_with for a component that a byte
producer writes, and append_fmt is built on it. The installer appends store
paths through it and no longer discards the Result.

Fixes the mojibake, and a path with bytes that are not UTF-8 (legal on
Linux) now names its store entry with the same bytes.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 216d30ad-a2d9-4fcd-9b67-c071eac5be4b

📥 Commits

Reviewing files that changed from the base of the PR and between a27a7a1 and 12707ae.

📒 Files selected for processing (12)
  • src/install/isolated_install.rs
  • src/install/isolated_install/Installer.rs
  • src/install/isolated_install/Store.rs
  • src/install/lib.rs
  • src/install/prune.rs
  • src/install/repository.rs
  • src/install/resolution.rs
  • src/paths/Path.rs
  • src/paths/lib.rs
  • src/runtime/cli/pm_licenses_command.rs
  • src/semver/lib.rs
  • test/cli/install/isolated-install.test.ts
💤 Files with no reviewable changes (1)
  • src/install/lib.rs

Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review.


Walkthrough

Changes

The change replaces formatter-based store-key and store-path generation with direct writer APIs. Installation, hashing, pruning, path construction, and license lookup call sites now use these APIs. Tests cover UTF-8 and raw non-UTF-8 store names.

Store-path writer migration

Layer / File(s) Summary
Writer-based store serialization
src/semver/lib.rs, src/install/resolution.rs, src/install/isolated_install/Store.rs, src/install/repository.rs, src/install/lib.rs
Store keys, resolution paths, repository paths, and URLs now write bytes directly and return CrateResult. Formatter wrappers were removed.
Path append writer integration
src/paths/Path.rs, src/paths/lib.rs
PathLike gains append_with. append_fmt uses the writer-based implementation and maps writer failures in checked mode.
Installation and hashing migration
src/install/isolated_install.rs, src/install/isolated_install/Installer.rs, src/install/prune.rs, src/runtime/cli/pm_licenses_command.rs
Install paths, global-store paths, SCC hashes, prune paths, and license lookup keys use direct writer APIs.
UTF-8 and raw-byte validation
test/cli/install/isolated-install.test.ts
Tests verify multibyte truncation, UTF-8 store names, repeated installs, and Linux raw filesystem bytes.

Suggested reviewers: jarred-sumner

Merge Risk: ⚪ Minimal · up to 12707

This localized change has no actionable merge-blocking risk remaining and is merge-ready after normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the problem, implementation, behavior, and verification. It does not use the exact template headings, but it provides the information required by both template section…
Title check ✅ Passed The title is concise and accurately identifies the main change: writing isolated store paths as bytes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description clearly explains the problem, implementation, behavior, and verification. It does not use the exact template headings, but it provides the information required by both template sections.


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

Comment thread src/install/isolated_install/Installer.rs
Comment thread src/install/isolated_install/Store.rs Outdated
Comment thread src/install/isolated_install/Store.rs Outdated
Comment thread src/install/repository.rs Outdated
Comment thread src/install/resolution.rs Outdated
Comment thread src/install/resolution.rs Outdated
Comment thread src/install/resolution.rs
Comment thread src/paths/Path.rs Outdated
Comment thread src/paths/Path.rs Outdated
Comment thread src/semver/lib.rs Outdated
Comment thread src/paths/Path.rs Outdated
Comment thread src/semver/lib.rs Outdated
Comment thread src/semver/lib.rs Outdated
@robobun

robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:55 AM PT - Aug 28th, 2026

❌ @robobun, your commit 12707ae has 2 failures in Build #107471 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 40719

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

bun-40719 --bun

Comment thread src/paths/Path.rs Outdated

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status for 12707ae (build 107471): 179 of 181 jobs passed. The two red lanes fail on tests this change does not touch, and both also fail on main: debian 13 x64-asan on test/cli/run/require-cache.test.ts (a leak test timing out after 30 s under ASAN) and darwin x64 on test/js/web/url/url.test.ts (ICU table, red on every recent main build). The earlier build's other red lane, filesystem_router.test.ts (a reload/build race on ubuntu aarch64, seen on main build 104102), passed this time. All three are reported to main-break triage. test/cli/install/isolated-install.test.ts passes on every lane.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants