Conversation
semver::String::StorePathFormatter emitted each byte of the lockfile
string via f.write_char(byte as char), which treats the u8 as a Latin-1
codepoint and re-encodes it as UTF-8. Any byte >= 0x80 (i.e. every
non-ASCII UTF-8 byte) doubled: c3 a9 ('é') became c3 83 c2 a9 ('é').
This formatter feeds entry::StorePathFormatter which builds on-disk
node_modules/.bun/<store-path>/ directory names, so any package name,
folder path, tarball URL, workspace path, or git ref containing a
non-ASCII byte produced a different (mojibake) store directory than the
Zig reference (SemverString.zig emits writer.writeByte verbatim).
Fix by emitting runs between the mapped ASCII bytes (/ \ : #) as
validated &str slices, matching install::StorePathFormatter which had
this class fixed already.
Collaborator
Author
|
Reproduced with a CI: |
Contributor
|
Warning Review limit reached
More reviews will be available in 5 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Collaborator
Author
|
Updated 5:55 AM PT - Jun 15th, 2026
✅ @robobun, your commit 343bcf198a5c06157b74cd164e52c1fe03520e34 passed in 🧪 To try this PR locally: bunx bun-pr 32304That installs a local version of the PR into your bun-32304 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — straightforward port-fidelity fix matching the existing install::StorePathFormatter pattern, with a verified regression test.
Extended reasoning...
Overview
This PR fixes a UTF-8 encoding bug in semver::String::StorePathFormatter (src/semver/lib.rs). The Rust port translated Zig's writer.writeByte(n) as f.write_char(n as char), which treats each u8 as a Latin-1 codepoint and re-encodes it as UTF-8 — doubling every byte ≥ 0x80. The fix emits byte runs between mapped ASCII separators (/ \\ : #) as validated &str slices via bun_core::str_utf8, preserving UTF-8 input verbatim. A regression test in test/cli/install/isolated-install.test.ts installs a file:./vendor/café folder dep under the isolated linker and asserts the .bun store directory name is byte-identical to the Zig output.
Security risks
None. The set of mapped characters (/ \\ : # → +) is unchanged, so path-traversal/separator handling is identical. The only behavioral delta is that non-ASCII bytes in lockfile strings are now passed through faithfully instead of being mojibake-expanded, and non-UTF-8 input (which shouldn't occur — lockfile strings originate from JSON) now surfaces fmt::Error rather than silently corrupting the path. This matches the contract already established by install::StorePathFormatter at src/install/lib.rs:952-963.
Level of scrutiny
Low-to-medium. The change is ~20 lines in a single fmt::Display impl, mechanically mirrors the already-reviewed sibling formatter in install/lib.rs (same str_utf8(...).ok_or(fmt::Error)? contract), and is exhaustively commented. The bug, root cause, and fix are all clearly explained and easy to verify by inspection. No CODEOWNERS path matches.
Other factors
- robobun confirmed the test fails on the unpatched build and passes with this change.
- The new implementation is allocation-free (slicing runs out of the input buffer), an improvement over the sibling formatter's
Vecapproach. - The test constructs "café" from explicit NFC bytes (
[0x63, 0x61, 0x66, 0xc3, 0xa9]) to avoid source-file normalization ambiguity, which is the right call. - Bug hunting system found no issues.
Contributor
There was a problem hiding this comment.
LGTM — focused encoding fix that brings the Rust formatter in line with the Zig reference and the existing install::StorePathFormatter contract.
Extended reasoning...
Overview
This PR rewrites the body of semver_string::StorePathFormatter::fmt in src/semver/lib.rs (~15 lines) and adds one regression test in test/cli/install/isolated-install.test.ts. The old code did f.write_char(byte as char) for each byte, which treats the u8 as a Latin-1 codepoint and re-encodes it as UTF-8 — so any byte ≥ 0x80 doubled in size, producing mojibake store directory names that diverged from the Zig release. The new code emits runs between the mapped ASCII separators (/ \ : #) as validated &str slices via bun_core::str_utf8(...).ok_or(fmt::Error)?, which is byte-faithful for valid UTF-8 input.
Security risks
None. The set of characters mapped to + is unchanged, so there is no impact on path-traversal protection. The output for ASCII inputs is identical; for non-ASCII UTF-8 inputs the output is now shorter (correct) rather than longer (mojibake). Invalid UTF-8 now surfaces fmt::Error instead of emitting corrupted bytes, which is strictly safer.
Level of scrutiny
Low-to-medium. The change is small, mechanical, and mirrors the exact pattern already used in install::StorePathFormatter at src/install/lib.rs:952-963 (same bun_core::str_utf8(...).ok_or(fmt::Error)? contract). The mapped separators are all single-byte ASCII, so splitting on them cannot bisect a multi-byte UTF-8 sequence — the UTF-8 validation will always succeed on well-formed input. Neither file is covered by CODEOWNERS.
Other factors
The bug analysis in the PR description is precise (verified u8 as char semantics), the in-code comment thoroughly documents the why, a regression test pins the on-disk byte sequence using an explicit Buffer.from([0x63,0x61,0x66,0xc3,0xa9]) to avoid source-normalization ambiguity, and robobun confirmed the test fails on the unpatched build and passes with the fix. No outstanding reviewer comments.
dylan-conway
pushed a commit
that referenced
this pull request
Aug 17, 2026
…ile: tarballs relative to their folder package (#38867) This PR bundles three independent install fixes (each was reviewed on its own PR; folded here so they land together): 1. bound the resolution part of isolated store entry names (this PR's original change) 2. send credentials embedded in a tarball URL as Basic authorization (from #39025) 3. read `file:` tarballs relative to the `file:` folder package that declares them (from #39017) Rebased on main after #39014: the isolated-store test file now computes expected git entry names through the same resolution cut (`storeEntryName`), since a `git+http+++host+port+repo.git+<url hash>+<sha>` resolution passes 80 bytes. --- ## 1. install: bound the resolution part of isolated store entry names #### Problem - With `linker = "isolated"`, a trusted git dependency with any lifecycle script makes `bun install` fail on Windows with `error: Failed to run script prepare due to error ENOENT` (exit 1). The same project installs fine with the hoisted linker. This is the Windows failure of the isolated case in #38810's test, which that PR currently skips on Windows. - A store entry is named `<name>@<resolution>` (`StoreKeyFormatter`, `src/install/isolated_install/Store.rs`). For git, github, tarball and folder dependencies the resolution is the whole URL or path with separators turned into `+` (`src/install/repository.rs` `StorePathFormatter`, `src/install/resolution.rs` `StorePathFormatter`), plus the commit for git: a git dependency checked out from a temp directory gets an entry like `dep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro+dep-repo+3c6955c70dbe1ff186a126c93c5e77c3ae7bd6b4` (111 bytes here, 170+ in #38810's test), and the name grows with the repository path. - The package directory, `<project>\node_modules\.bun\<entry>\node_modules\<name>`, is the cwd of the package's lifecycle scripts (`Scripts::get_list` -> `lifecycle_script_runner.rs` `SpawnOptions.cwd`). bun's own file operations use long-path-capable NT paths, so the entry is created and linked, but `CreateProcessW` does not accept a current directory longer than `MAX_PATH`, and the spawn (libuv's `uv_spawn` on Windows) reports that as ENOENT. Measured on Windows x64 with the released `1.4.0-canary.1`: a package directory of 238 characters runs the script, one of 268 characters fails as above. - The same unbounded name also fails on every platform once it passes `NAME_MAX`: a tarball URL or folder path of 250+ bytes makes the install fail with `ENAMETOOLONG: File name too long: failed to link package` (the case of #37470). #### Fix - `StoreKeyFormatter` now writes the resolution through `write_resolution`: a resolution of at most 80 bytes (`MAX_RESOLUTION_LEN`) is written unchanged; a longer one is written as its first 63 bytes (backed up to a UTF-8 character boundary) followed by `+` and the 16 hex digit wyhash of the full text (seed 0, so `Bun.hash(text)` reproduces it), which is 80 bytes again. The `name@` prefix and the `+<peer hash>` suffix are unchanged. A cut git entry looks like `dep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro-aaaaa+ef7ad81a431ad661` (the 268 character case from the measurement above; its package directory is now 206 characters). - Why this is correct: - Every path that contains an entry name is formatted through this one `Display` impl: the project store directory and the dependency symlinks into it (`Installer.rs` `append_store_path` / `append_store_node_modules_path` / `link_to_hidden_node_modules`), the lifecycle script cwd (same `append_store_path`), the global store directory name and the entry hash seeded from the name (`Installer.rs` `append_global_store_entry_path`, `isolated_install.rs` entry hash), `bun pm prune`'s set of expected directories (`prune.rs` `push_store_entry_names`) and `bun pm licenses`'s lookup (`pm_licenses_command.rs` `BunStore::lookup`, via `fmt_store_key`). They all see the same cut name; nothing persists or parses the old one (the lockfile stores resolutions, and an existing entry is detected by re-deriving its name). - The cut name stays a function of the resolution alone, so repeated installs find the same entry, and two resolutions that agree on the first 63 bytes still get distinct entries through the hash. The sink hashes the text in whatever chunks the formatters write it in (the path formatters write one character at a time); `Wyhash`'s streaming form is chunk-invariant (`test_iterative_chunked_matches_oneshot` in `src/wyhash/lib.rs`), which is what lets the tests compute the expected names with `Bun.hash`. - The cut happens inside the resolution only, so `prune.rs` (`split_store_key`, `store_has_entries`, `store_link_target`, which split the name at `@`) and `pm licenses` (which matches `<key>` or `<key>+<peer hash>`) keep working without changes. - Resolutions of 80 bytes or less are byte for byte what they were, so versions and `github:` shorthands (`github+owner+repo+<sha>` is about 60 to 75 bytes) are unaffected on upgrade. Most full git URLs are over 80 bytes (`git+https+++github.com+` is 23 bytes and the commit adds 41), so those entries are re-created once under the cut name on the first install after upgrading, and the old directories stay until `bun pm prune`, as for any re-resolved entry. The cut keeps the URL itself readable: `git-pkg@git+file++++tmp+licenses-git-repo_eXITMt+491eb29762d8d19b570b43+f87e35b31f207dd4`. - 80 is the tunable here. The package directory is `<project> + 34 + 2 * <name> + <resolution>` (+17 with peers) characters, so with 80 it fits `MAX_PATH` whenever `<project> + 2 * <name>` is at most 145 (128 with peers); #38810's case (61 character temp dir, 25 character name) ends up at about 225 instead of about 290. It also makes the entry directory name fit `NAME_MAX` together with every suffix the installers append (`+<peer hash>`, `-<entry hash>`, `.tmp-<hex>`) for names up to 119 bytes, and with the peer suffix alone for names up to 157 bytes. For comparison, pnpm's `virtual-store-dir-max-length` defaults to 120 bytes for the whole directory name; 80 bytes of resolution plus a typical name lands in the same range. - #37470 (the same mechanism with a 200 byte limit on the whole `name@resolution`, for the `NAME_MAX` case only) is closed in favor of this PR: that limit does not help the `MAX_PATH` case (the failing names above are 130 to 180 bytes), and a cut through `name@` would no longer work with `bun pm prune` / `pm licenses` (see the `prune.rs` bullet above). Its repro installs with this branch and its deep-directory case is carried over below. What it covered and this PR does not is the name part: a name over 119 bytes (157 in the project store) can still produce an entry over `NAME_MAX` once its resolution is cut; if that should be covered too, it would be the same sink applied to the name separately, keeping the `@`. #36973 (documenting the layout) would need a sentence about this rule; this PR adds a short one to `docs/pm/isolated-installs.mdx`. #38810's Windows skip of its isolated case can be removed once both land. #39014 changes the same text (it removes URL credentials, and the hash is of the text as formatted), so landing it in the same release as this avoids renaming those entries twice. - Verification, `long store entry names` in `test/cli/install/isolated-install.test.ts`: - a folder resolution of exactly 80 bytes is kept verbatim and one of 81 bytes is cut to the name computed with `Bun.hash`; two resolutions of one package that only differ after the cut point get separate entries that each resolve to their own package; a second install finds the same entries - a cut entry with a resolved peer gets `+<peer hash>` appended after the cut name and links the peer - a folder name made of 2 byte characters, positioned so that byte 63 falls inside a character, is cut at byte 62 and still installs (without the character boundary loop this case panics with `a formatting trait implementation returned an error`; the test only asserts the shape because the spelling of non-ASCII bytes in these names is #32304's subject) - (carried over from #37470) a local tarball and a folder three 85 byte directories deep, whose unbounded entry names would be 277 bytes, install; the top-level symlinks and the `.bun/node_modules` fallback link point at the cut names, and a second install finds the same entries - a tarball URL of about 290 bytes installs with `install.globalStore` enabled: the local entry has the cut name, links to `links/<cut name>-<entry hash>`, and the package imports at runtime - the reported case: a trusted `git+file://` dependency whose repository directory name alone is 60 characters runs its postinstall script with the isolated linker; the entry has the cut name - Without the `Store.rs` change the first three tests and the git one fail on the entry name (on Windows the git one fails with `error: Failed to run script postinstall due to error ENOENT`, the reported message) and the deep-directory and tarball ones exit 1 with `ENAMETOOLONG`; all of them pass with `bun bd test` on Linux (the five original ones also on a local Windows x64 build; the deep-directory one on the Windows lanes in CI). #37470's repro from its description (`./x/../` x130 tarball spec) installs with this branch with an 84 byte entry. - Also run with the fix: the rest of `isolated-install.test.ts`, `bun-pm-licenses.test.ts` (its `git dependency is listed (isolated)` case now goes through a cut name on every platform, since the repo lives in a temp dir), `bun-prune.test.ts`, `public-hoist-pattern.test.ts`, `isolated-relink.test.ts`, `bun-install-native-binlink.test.ts`, `config-version.test.ts`, and the PowerShell reproduction from the report on Windows x64 (package directory of 268 characters before, 206 after, script runs). #### Background - Isolated linker: instead of hoisting, every package is materialized once under `node_modules/.bun/<entry>/node_modules/<name>` and everything else symlinks to it. `<entry>` is `<name>@<resolution>`, plus `+<16 hex>` when the package was installed for a specific set of peer dependencies. With `install.globalStore`, `node_modules/.bun/<entry>` is itself a symlink to `<cache>/links/<entry>-<16 hex entry hash>`. - Resolution: the lockfile's description of where a package came from. It is the version for registry packages and the spec itself (folder path, tarball URL, git URL plus commit) for everything else, which is why only those entries get long. - `MAX_PATH`: Windows' 260 character limit for paths passed to Win32 APIs. Applications can opt out of it for file operations (bun does, by using `\\?\` / NT paths), but the current directory handed to `CreateProcess` is still limited to it, and libuv maps the resulting Win32 error to ENOENT. - `NAME_MAX`: the 255 byte limit on one path component on Linux and macOS (255 UTF-16 units on NTFS); exceeding it fails with `ENAMETOOLONG` regardless of how long the whole path is. <details> <summary>Windows measurement with the released build (1.4.0-canary.1)</summary> Git repo and project created under `%TEMP%` with `linker = "isolated"`, `trustedDependencies: ["dep"]` and `"prepare": "echo prepared > prepared.txt"`; the temp directory name was lengthened to move the package directory across the limit. ``` store entry: dep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro-aaaaaaaaaaaaaaaaaaa+dep-repo+d1a8f7c4111533cb16b52a9edcb086329e6472b5 pkg dir len: 238 prepared.txt exists: True exit: 0 store entry: dep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa+dep-repo+30b6ecc8d4d0e4fb2f3df2402556222bf6a5b7dc pkg dir len: 268 pkg dir exists: True prepared.txt exists: False error: Failed to run script prepare due to error ENOENT exit: 1 ``` Same second layout with this PR's build: ``` store entry: dep@git+file++++C++Users+AZUREU~1+AppData+Local+Temp+lc-repro-aaaaa+ef7ad81a431ad661 pkg dir len: 206 prepared.txt exists: True exit: 0 ``` </details> <!-- robobun:evidence:begin --> --- **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.test.ts test/cli/install/isolated-install.test.ts <!-- robobun:evidence:end --> --- ## 2. install: send credentials embedded in a tarball URL as Basic authorization (from #39025) #### Problem - A dependency declared as a tarball URL with credentials, `"no-deps": "http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz"`, is downloaded without an `Authorization` header. A server that needs the credentials answers 401 and the install fails with `error: GET http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz - 401` (bun 1.4.0 and `main`, hoisted and isolated linker). npm 11 installs the same package.json and sends `Authorization: Basic Y2Fyb2w6czNjcmV0` (`base64("carol:s3cret")`). - The username-only form (`http://token@127.0.0.1:PORT/x.tgz`) does not reach the server at all: the request goes to the hostname `token@127.0.0.1`. - Cause: `NetworkTask::for_tarball` (`src/install/NetworkTask.rs`) only ever attaches the registry scope's configured token or `_auth`, and only for npm packages whose tarball is on the registry's origin. Nothing reads the URL's userinfo, and the HTTP client does not either (it only turns the userinfo of a proxy URL into `Proxy-Authorization`). The misrouted username-only form comes from `bun_url::URL::parse`, which takes `token@127.0.0.1` for the hostname when the userinfo has no `:` and a port follows (tracked separately; this change no longer depends on how that parser splits the authority). - Found while fixing the isolated store names of these URLs (#39014), not from a user report. #### Fix - `for_tarball` splits the userinfo off the request URL before anything else looks at it (`split_url_userinfo`: the authority runs from `://` to the first `/`, `?` or `#`, the userinfo is everything in it up to the last `@`, so the `@` of `/@scope/pkg/-/pkg.tgz` is not one) and sends it as `Authorization: Basic base64(userinfo)` (`basic_authorization_from_userinfo`, which appends `:` when the userinfo has no password). The request URL is the URL without the userinfo. - Precedence: when the registry scope's credentials apply to the tarball (npm package, tarball on the registry origin, scope has a token or `_auth`), they are still sent and the URL's are not. This is npm's order as well: npm-registry-fetch sets the header from the config, and node only derives `Authorization` from the URL's `auth` when the request has no such header. - Why Basic of the userinfo as written: it is what npm sends, checked against npm 11.16 for `user:pass`, `user` (sent as `user:`), `:pass` and a percent-encoded password (sent undecoded); transcript below. It is also how bun already treats credentials written into a registry URL (#38796 stores the username and password bytes as given). pnpm was not available offline. Two spellings are deliberately not npm's: a second `:` inside the password is sent as written where npm percent-encodes it, and `:token@` in a tarball URL is Basic like npm rather than the Bearer that bun's registry URLs make of it; both are called out in the test table and in the code comment. - Why the URL is requested without the userinfo: `bun_url::URL::origin` includes the userinfo and the HTTP client compares origins to decide whether `Authorization` follows a redirect, so with the userinfo left in, a redirect to the same host would drop the header (verified: with only the header added, the redirect test below fails with `authorization: null` on the second hop). It also fixes the username-only form without touching `bun_url`, and the `GET <url> - 401` line now prints the URL without the credentials. The cross-host rule is unchanged: the HTTP client strips the header on a redirect to another origin, same as for the registry token (test included). - Behavior outside the request is unchanged: `package.json`, the lockfile, the task id and the cache key still use the URL as written. Documented in `docs/pm/cli/add.mdx`. - Verified with `test/cli/install/bun-install.test.ts`, `describe("credentials embedded in a tarball URL")`: 12 tests, 10 of which fail on the unfixed build (the two guards, scoped path and registry credentials taking precedence, pass on both). The rest of the file is unchanged: the remaining failures locally are the tests that need the public internet, identical with the unmodified binary. - `cargo check -p bun_install`, `cargo clippy -p bun_install`, rustfmt and prettier are clean. #### Background - Tarball dependency: a `package.json` entry whose version is an `http(s)://` URL ending in `.tgz`/`.tar.gz`/`.tar`. Bun downloads it directly; no registry manifest is involved, so the registry's configured credentials never applied to it (`Authorization::NoAuthorization` at the call sites in `PackageManagerEnqueue.rs`). Registry packages reach the same `for_tarball` through their manifest's `dist.tarball` URL with `AllowAuthorization`, which is the case where the registry scope's credentials can apply. - Userinfo: the `user:password@` part of a URL's authority (RFC 3986 section 3.2.1). HTTP never puts it on the wire; clients that honor it (npm through minipass-fetch, curl, browsers) convert it into `Authorization: Basic base64(user:password)`. - `NetworkTask::for_tarball` builds one HTTP request per tarball download: `url_buf` (the request URL) and `header_buf` (the headers, built with `HeaderBuilder` in two passes, `count` then `append`, because the buffer is allocated exactly once in between). Retries reuse the same request, so the header is sent on every attempt. - `bun_url::URL::parse` is the allocation-free splitter the HTTP client and `for_tarball` use; it is not a WHATWG parser. Its `origin` is a prefix of the input string, which is why it still contains the userinfo. <details> <summary>npm 11.16 against a local server logging the Authorization header (same tarball, same package.json shape)</summary> ``` userinfo in the dependency URL header npm sent carol:s3cret@ Basic Y2Fyb2w6czNjcmV0 = base64("carol:s3cret") carol@ Basic Y2Fyb2w6 = base64("carol:") :s3cret@ Basic OnMzY3JldA== = base64(":s3cret") carol:s3%40cret@ Basic Y2Fyb2w6czMlNDBjcmV0 = base64("carol:s3%40cret"), not decoded carol:s3:cret@ Basic Y2Fyb2w6czMlM0FjcmV0 = base64("carol:s3%3Acret"), bun sends base64("carol:s3:cret") carol:s3cret@ + 302 to same host Basic ... on both hops ``` bun 1.4.0-canary.1 (eabb96d) for the first row: the server logs `auth=null` and bun prints `error: GET http://carol:s3cret@127.0.0.1:PORT/cdn/no-deps-1.0.0.tgz - 401`. </details> --- ## 3. install: read `file:` tarballs relative to the `file:` folder package that declares them (from #39017) #### Problem - A `file:` folder dependency whose own package.json declares a local tarball fails to install (released 1.4.0 canary and main, both linkers): project package.json `{"dependencies":{"lib":"file:./vendor/lib"}}`, `vendor/lib/package.json` declaring `"tool": "file:./tool.tgz"`, `vendor/lib/tool.tgz` present: ``` error: ENOENT extracting tarball from tool error: tool@file:./tool.tgz failed to resolve ``` - The path is read as `<project>/tool.tgz`. If that file happens to exist it is installed as `tool` instead of the folder's copy, with no error. The directory form of the same declaration (`"tool": "file:./tool"`) is already resolved relative to `vendor/lib`. - Cause: `enqueue_local_tarball` (`src/install/PackageManager/PackageManagerEnqueue.rs`) picks the directory a local tarball path is relative to, and the only declarer it looked at was a workspace (`get_workspace_pkg_if_workspace_dep`). Every other declarer, including a `file:` folder package, fell through to the top-level dir. - The same choice was also wrong in the other direction: a root `overrides` / `resolutions` entry or catalog entry pointing a dependency at `file:./x.tgz` was read relative to the workspace when the dependency it applied to was declared by a workspace member, so `overrides: { bar: "file:./bar.tgz" }` with `bar.tgz` in the project root failed with the same ENOENT as soon as a workspace depended on `bar`. This is #25835 (`overrides`) and #25752 (`catalogs`); both reproduce as reported on the released build and install with this change. The directory form of an override is already resolved relative to the project (`Folder` arm of `get_or_put_resolved_package`). Fixes #25835 Fixes #25752 #### Fix - `enqueue_local_tarball` now takes the base directory from `local_tarball_base_dir`: the directory of the declaring package when it is a workspace or a `file:` folder package and that package's own specifier is the tarball path being read; the top-level dir in every other case. - Why the declaring package: a path in a package.json means a file next to that package.json, which is what npm does for `file:` and what bun already does for workspace declarers and for the directory form. A `file:` folder package is read from the project like a workspace is, and its `Resolution::Folder` payload is its directory relative to the top-level dir (`folder_resolver.rs`, `NewResolver { folder_path: rel }`), the same shape as a workspace's `Resolution::Workspace` payload, so both are joined the same way. The only other `Resolution::Folder` packages are the stubs created for `file:` directories declared by something other than the root or a workspace (`Folder` arm of `get_or_put_resolved_package`); those carry no dependency list, so they are never the declarer of an edge. - Why the "own specifier" condition: `overrides`, `resolutions` and catalogs are only parsed from the root package.json (`Package.rs`, `FEATURES.is_main`), and applying one leaves the declaring package's stored edge untouched (the replacement is local to `enqueue_dependency_with_main_and_success_fn`). So when the stored edge's specifier is not the path being read, the root wrote the path and the top-level dir is the only directory it can mean. Without this condition, root overrides applied to a folder-declared dependency, which work today only because of the bug, would start being read from the folder. - Why it is decided from the edge and not from the resolve pass's `version_was_replaced`: `enqueue_local_tarball` is also reached from `enqueue_tarball_for_reading` when a project with a `bun.lock` is installed into an empty cache. The lockfile row keeps the path as declared (`"tool": ["bar@./tool.tgz", ...]` under `"lib": [..., { "dependencies": { "tool": "file:./tool.tgz" } }]`), and the edge's declarer and specifier are available there too, so both passes compute the same directory. Lockfile format is unchanged and existing lockfiles keep working; the path in the row is joined onto a different directory only for declarers that previously failed or installed the wrong file. - Declarers extracted from the cache (registry, git, tarball packages) still fall through to the top-level dir, as before; #38986 is changing what happens to those separately. `Lockfile::get_parent_pkg_of_dependency` is the same helper that PR adds. - Left as is: the lockfile identity of a local tarball is still the path as written (`bar@./tool.tgz`), so two project packages declaring the same relative path to two different files still share one row and one read, the limitation workspaces already have. A package.json inside a folder dependency that worked around this bug by writing a project-relative path (`file:./vendor/lib/tool.tgz`) will now need the path relative to itself, which is what npm requires for it as well. - Verified with `test/cli/install/bun-install.test.ts`, `describe("file: tarball declared by a file: folder dependency")`: the folder's tarball is installed with the hoisted and with the isolated linker, and a root override supplying the path is read from the project. Each test plants a different tarball at the other candidate path and runs a fresh install followed by `--frozen-lockfile` into an emptied cache, so both the resolve pass and the install from `bun.lock` have to read the right file. `test/cli/install/bun-workspaces.test.ts`, `relative tarballs > from a root override / catalog entry applied to a workspace dependency`, covers #25835 and #25752 the same way. The four folder/workspace tests install the wrong tarball without the `src/` change (checked against a debug build of main and against the released build); the override-on-folder test passes before and after and pins that case. - Also run with this change: `bun-workspaces.test.ts` (74 pass), `overrides.test.ts` + `nested-overrides.test.ts` (151 pass), `bun-lock.test.ts` (40 pass), `bun-install.test.ts -t "tarball|tgz|file:|folder|override|resolutions"` (51 pass; `should treat non-GitHub http(s) URLs as tarballs` fails identically on the released build, it needs network), `isolated-install.test.ts`, `bun-add.test.ts` and `bun-install-registry.test.ts` filtered to tarball/file tests (all pass). `cargo clippy -p bun_install` is clean. #### Background - A `file:` dependency on a directory is a `Tag::Folder` dependency; one on a `.tgz` is a `Tag::Tarball` dependency with a local URI. The latter resolves to `Resolution::LocalTarball(<path as written>)`; the path string is both the task id for reading it and the package's identity in `bun.lock`. - bun reads the package.json of the root, of workspace members and of `file:` folder dependencies from disk (`Package::parse`), and stores for each of the latter two a `Resolution::Workspace` / `Resolution::Folder` whose payload is the package directory relative to the top-level dir. Packages from the registry, git or a tarball get their dependency lists from the manifest or the extracted archive instead and have no directory in the project while they resolve. - Dependency edges live in one flat buffer and every package owns a contiguous slice of it, which is how the package that declared an edge is found. The edge stores the specifier as declared; overrides, resolutions and catalogs replace it only for the duration of resolving that edge. - `enqueue_local_tarball` is called from two places: the `Tarball` arm of the resolve pass (no lockfile row yet, the tarball is read to learn the package's name and dependencies) and `enqueue_tarball_for_reading` during install (a row exists in `bun.lock`, but the extracted package is missing from the cache). It computes the on-disk path on the main thread and hands it to a thread pool task, which only reads the file. Closes #39025 Closes #39017 --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Collaborator
Author
|
Superseded by #40719. That PR replaces both store path formatters with one byte-oriented writer over |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Repro
Before:
mypkg@file+vendor+café(hex...636166 c383c2a9)After:
mypkg@file+vendor+café(hex...636166 c3a9), matching the Zig reference.Cause
semver::String::StorePathFormatterported Zig'swriter.writeByte(n)asf.write_char(n as char).n as chartreats theu8as a Latin-1 codepoint andwrite_charemits its UTF-8 encoding, so every byte ≥ 0x80 becomes two bytes. UTF-8é(c3 a9) turns intoé(c3 83 c2 a9).This formatter feeds
entry::StorePathFormatterinisolated_install/Store.rs, which is consumed viaformat_args!to build on-disknode_modules/.bun/<store-path>/directories. Any package name, folder path, tarball URL, workspace path, symlink path, or git owner/repo/committish containing a non-ASCII byte produced a different (mojibake) store directory than the Zig release, causing silent on-disk divergence and cache misses across an upgrade.Fix
Emit runs between the mapped ASCII bytes (
/ \ : #) as validated&strslices so the output is byte-for-byte identical towriter.writeByte. The mapped characters are all ASCII so UTF-8 validity of the input is preserved. Non-UTF-8 input surfacesfmt::Errorrather than corrupting the path, matchinginstall::StorePathFormatterwhich already had this class fixed.Verification
New test in
test/cli/install/isolated-install.test.tsinstalls afile:./vendor/cafédependency with the isolated linker and asserts the.bunstore directory name is byte-identical to the Zig output.