install: fix bun patch --commit cache path for git, github, and tarball dependencies - #37124
install: fix bun patch --commit cache path for git, github, and tarball dependencies#37124robobun wants to merge 6 commits into
Conversation
…ll dependencies bun patch --commit loads its own copy of the lockfile, but compute_cache_dir_and_subpath resolved the Resolution's strings against manager.lockfile, which is still empty at that point in the patch-commit flow. Strings longer than 7 bytes are stored as offsets into the string buffer, so every non-npm resolution produced a wrong pristine-copy path (github: deps computed '@gh@@@@1' instead of '@gh@<owner>-<repo>-<sha>@@@1') and the diff step failed with: error: failed to make diff error: Could not access '~/.bun/install/cache/@gh@@@@1' npm deps were unaffected because their version is stored inline. Pass the string buffer that owns the resolution's strings into compute_cache_dir_and_subpath; the patch-commit call sites hand over the locally loaded lockfile's buffer, everyone else keeps manager.lockfile. Fixes #18792
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Git and github resolution labels put ':' in the generated patch filename (pkg@github:owner/repo#sha.patch). NTFS rejects ':' in filenames (it is the alternate-data-stream separator), so the rename of the freshly written patch into patches/ failed with OBJECT_NAME_INVALID on Windows, and a patches/ directory committed from another OS could not be checked out on Windows at all. Escape the NTFS-reserved printable characters the same way '/' already is. Previously unreachable for git/github deps because the flow failed at the cache-path step this branch fixes.
There was a problem hiding this comment.
Both concerns from my earlier pass are addressed — escape_patch_filename now covers the NTFS-reserved printable characters on every OS, and the git-dependency test isolates git config and asserts on exit code only. I didn't find anything further, but this touches package-manager cache-path resolution and changes patch filename generation on all platforms, so a human should sign off.
What was reviewed:
- Traced all five
compute_cache_dir_and_subpathcall sites: the twodo_patch_commitsites correctly pass the locally-loaded lockfile's string buffer;prepare_patchandnew_apply_patch_hashcorrectly passNone(their resolutions come frommanager.lockfile). - Verified the git/github/tarball arms now call the same
*_printhelpers the previous wrappers delegated to, so the folder-name format is unchanged whenNoneis passed. - Escape table: hex codes are correct; existing
patchedDependenciesentries are unaffected since the path is stored verbatim in package.json. - Git test's
GIT_CONFIG_GLOBALpoints at a nonexistent regular path (not the null device), which git treats as empty — the caveat intest-changed.test.tsis specific toNUL.
Extended reasoning...
Overview
The PR fixes bun patch --commit for git, github, and tarball dependencies. do_patch_commit loads its own lockfile copy, but compute_cache_dir_and_subpath was resolving the resolution's string-buffer offsets against manager.lockfile (empty at that point), collapsing cache folder names to just their prefix (@GH@@@@1). The fix adds an Option<&[u8]> parameter for the owning string buffer and threads it through all non-npm arms. A follow-up commit (responding to my earlier review) extends escape_patch_filename to percent-encode the seven NTFS-reserved printable characters so the resulting patch filenames are valid on Windows and portable when committed from POSIX.
Files: PackageManagerDirectories.rs (parameter + per-arm buffer selection), patchPackage.rs (two do_patch_commit call sites pass the local buffer; two prepare_patch sites pass None; escape table extended), patch_install.rs (new_apply_patch_hash passes None), and three new hermetic tests in bun-patch.test.ts.
Security risks
None identified. The change is read-path only (deriving a cache subpath from lockfile data the process already trusts). The escape-table extension narrows the set of characters that reach the filesystem in patch filenames — strictly a hardening. No new external input is parsed.
Level of scrutiny
Medium-high. src/install/ is a critical subsystem and compute_cache_dir_and_subpath is shared across bun patch, bun patch --commit, and the patch-apply task. I traced each call site to confirm the buffer it passes matches the lockfile the resolution came from, and confirmed the git/github/tarball arms now inline the exact body the previous wrapper functions delegated to (so None preserves prior behavior byte-for-byte). The escape change is mechanical but changes generated filenames for git/github deps on all platforms; I confirmed it doesn't affect lookup of existing patches (paths are stored verbatim in patchedDependencies, not derived from the key).
Other factors
My previous review's blocking finding (unescaped colon → NTFS rename failure) and hermeticity nit (ambient git config) were both addressed in follow-up commits, and robobun reports the three new tests pass on a Windows machine. The tests are offline (local Bun.serve for the GitHub tarball and git dumb-HTTP protocol; file: tarball), use per-test BUN_INSTALL_CACHE_DIR, assert patch file contents and the reinstalled result, and cover both the path-arg and name-arg lookup branches. Deferring because the change spans a shared install helper and alters patch filename generation — worth a human look even though I believe it's correct.
Seven arms of compute_cache_dir_and_subpath repeated the same Some/None match on resolution_string_bytes; a future arm repeating it wrong would reintroduce the wrong-buffer bug. One generic helper over bun_semver::Slicable makes the choice in a single place.
There was a problem hiding this comment.
Both earlier findings are addressed — escape_patch_filename now covers the NTFS-reserved characters and the git setup is isolated from ambient config. No new issues found this pass, but this touches core install paths and introduces a user-visible patch-filename format for git/github deps (plus the noted isolated-linker limitation), so worth a human look.
What was reviewed:
- Traced the new
resolution_string_bytesparam through all 5 call sites — the twodo_patch_commitsites correctly pass the locally-loaded lockfile's buffer;prepare_patchandPatchTaskpassNoneand their resolutions do come frommanager.lockfile. - Confirmed the direct
cached_{git,github,tarball}_folder_name_printcalls preserve the exact prefix/cache-version/patch-hash layout of the wrappers they replace. - Verified the git-setup loop now only asserts exit code (dropping the strict
stderr === "") and setsGIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL+core.autocrlf false.
Extended reasoning...
Overview
The PR fixes bun patch --commit for git/github/tarball dependencies by threading the correct lockfile string buffer into compute_cache_dir_and_subpath (previously it always sliced against manager.lockfile, which is empty in the commit flow). It also extends escape_patch_filename to cover NTFS-reserved characters so the resulting patch filenames are valid on Windows, and adds three offline end-to-end tests (github via GITHUB_API_URL, git via dumb-HTTP, local tarball).
Prior feedback
My earlier review flagged two issues, both now fixed: (1) the unescaped colon in git/github patch filenames would fail the rename on NTFS — f36a471 adds : ? * " < > | to the escape table and the tests assert the generated path contains none of them; (2) the git setup was reading ambient ~/.gitconfig — 7a18c49 sets GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL, adds core.autocrlf false, and relaxes the setup assertions to exit-code only. All comment-cop flags on doc-comment length are resolved.
Security risks
None identified. The change is read-path only (which string buffer to slice against) plus filename percent-escaping. No new untrusted-input parsing or path construction from external data beyond what the existing flow already did.
Level of scrutiny
Medium-high. This is core bun install/bun patch infrastructure, and the escaping change defines the on-disk/committed patch filename format for git/github deps for the first time (previously unreachable due to the crash). The PR also explicitly scopes out an isolated-linker install hang as pre-existing — that scoping decision is reasonable but should be acknowledged by a maintainer.
Other factors
The fix is well-targeted and the mechanism is clearly explained. The three new tests are hermetic (local servers, per-test cache dir), exercise both the path-arg and name-arg commit flows, and assert patch contents + patchedDependencies key + reinstalled file. I verified the direct *_print calls produce byte-identical output to the wrappers they bypass (git has no cache-version suffix, github/tarball do). Deferring rather than approving because the change is not mechanical and touches a critical, cross-platform path.
…rs (#37469) ## Repro ```sh mkdir probe && cd probe cp <bun repo>/test/cli/install/bar-0.0.2.tgz . bun -e 'require("fs").writeFileSync("package.json", JSON.stringify({ name: "foo", dependencies: { bar: "./" + "x/../".repeat(130) + "bar-0.0.2.tgz" } }))' bun install ``` ``` Resolved, downloaded and extracted [1] panic: unreachable: Error ``` `bun install` aborts (SIGABRT, exit 134) after the tarball has been resolved and extracted. The spec normalizes to `./bar-0.0.2.tgz`, which exists; a real 618 byte relative path behaves the same (that is how this was found, see #37462). The same abort happens for a remote tarball whose URL is longer than 512 bytes, a `file:` folder at a long path, and a workspace package whose version has a ~500 byte prerelease tag. `--lockfile-only` succeeds, so it is the link step. Reproduced with `bun 1.4.0-canary.1` and main. Top frame: `Result::expect` in `PackageInstaller::install_package_with_name_and_resolution` (`src/install/PackageInstaller.rs:1340`), called from `install_package` / `hoisted_install::install_hoisted_packages`. ## Cause While linking each package, the hoisted installer formats the package's version label into `let mut resolution_buf = [0u8; 512]` with `buf_print(..).expect("unreachable")`. For npm packages the label is the version; for tarball, folder and git packages it is the spec they were resolved from (stored verbatim), and for workspace packages it is the workspace's own version. Those are user supplied and have no length bound, so the overflow `buf_print` reports is reachable and the `expect` turns it into the panic. Both branches (workspace version and resolution) have it. The isolated linker already builds this label in a `Vec` (`Installer::package_patch_info`) and is not affected by this panic. It fails such installs with `ENAMETOOLONG` instead, because the store directory name embeds the spec; that is a separate problem and not touched here. ## Fix `print_package_version` keeps the 512 byte stack buffer as the allocation-free path and, only when the label does not fit, formats it into a `Vec` that lives next to the buffer (the same shape as `resolve_path::join_z_buf_spill`). The linking loop runs once per installed package, so the common case still does not allocate. Spilling is the only correct behavior for this label: it is compared against the installed `package.json` version and hashed as the version half of the `name@version` `patchedDependencies` key, so a truncated label would silently fail verification or miss a patch, and rejecting the package would refuse a valid install (nothing on disk is named after the label; the tarball cache folder is a hash of it). The new patchedDependencies test below checks the spilled label byte for byte by keying a patch with it. `bun patch` formats the same labels into 1024 byte buffers with the same `expect` at four sites in `src/install/PackageManager/patchPackage.rs`, all reproducible the same way once the install succeeds (verified with a build that only had the installer change): * `pkg_info_for_name_and_version` (`bun patch <name>@<version>` compares the label of every package with that name; panicked with `Resolution name too long`), * the multiple-packages-with-this-name loops in `prepare_patch` and `do_patch_commit` (`bun patch <path>` when another package with the same name has a long label), * the `name@label` key in `do_patch_commit`, which is also the patch file name. These format into a `Vec` (`print_resolution_label`, reused across a candidate loop). In `do_patch_commit` the key `Vec` is returned as the `patch_key` directly and the file name is built from it, replacing the re-slicing of the shared buffer; the bytes are unchanged (the key was already valid UTF-8, so the former `BStr` round trip was a copy). Committing a package whose label is this long still cannot succeed, since the patch file would be named after the label, but it now exits 1 with an error instead of aborting; today it fails before that at the diff step for tarball packages (#37124), which this does not change either way. ## Tests `test/cli/install/bun-workspaces.test.ts`, "packages whose version label is longer than 512 bytes" (hoisted linker): local tarball, remote tarball served from a local `Bun.serve`, workspace package with a long prerelease version (the other branch), and a `patchedDependencies` entry keyed by the long spec that must actually be applied. `test/cli/install/bun-patch.test.ts`, "packages whose label is longer than 1024 bytes": `bun patch <name>@<label>`, `bun patch <path>` plus `--commit` when a same-named package has a long label (the long one is listed first, so the loops format it), and `--commit` of a long-labeled package exiting 1 without touching `package.json`. The long specs use `x/../` repeated so nothing long is ever created on disk. All seven fail on the unfixed build (`panic: unreachable: Error` during the install) and pass with the fix; with only the installer change applied, the three `bun patch` tests fail at the `bun patch` step instead, so each patch site is covered on its own. Both files pass in full, as does `bun-install-patch.test.ts`; `cargo clippy -p bun_install` is clean and `cargo check -p bun_install` passes for `x86_64-pc-windows-msvc` and `x86_64-apple-darwin`.
|
Folded into #38269 together with the other two |
…fix bun patch for non-npm deps and isolated hang (#38269) ### What does this PR do? Install-cache and `bun patch` robustness, consolidated from #37124, #37136 and #37145 (rebased and reshaped; #32749 from the same batch landed separately). **Git dependency cache folders are built in a staging dir and renamed on success.** `Repository::checkout` cloned straight into `<cache>/@g@<sha>` and checked out in place; `Repository::download` cloned the bare mirror straight into `<cache>/<hash>.git`. An install (or its git child — seen OOM-killed in CI) dying between steps left a folder at the trusted name: an empty `@G@` folder resolves as an *empty package* through the "git dependency without package.json" path (exit 0, `bun.lock` name falls back to the URL basename), and a half-cloned mirror fails every later `git fetch`. Both now build under a temporary sibling inside the cache dir (`CacheStaging`, same-filesystem so it's the same `renameat_concurrently` ladder tarball extraction uses) and are renamed into place only when complete; failures remove the staging dir. **Cache hits require the entry's completion marker.** One helper, `is_package_in_cache_at(cache_dir, folder, tag)`: npm folders must contain `package.json`, git checkouts must contain the `.bun-tag` written last, everything else stays a directory probe. Used by `checkout()`'s resolve-time hit, `determine_preinstall_state`, and the hoisted and isolated installers — previously every git hit was a bare directory probe (`.bun-tag` in the cache was written but never read), and `determine_preinstall_state` didn't probe `package.json` for npm either. Folders left by older versions are re-cloned instead of installed. Deletes the hoisted installer's unsafe in-place edit of the shared folder-name buffer and the isolated installer's append/truncate copy. Since the tag is now the marker, `checkout()` unlinks anything a repo checked in as `.bun-tag`, creates it `O_EXCL|O_NOFOLLOW`, and fails the checkout rather than publish an untaggable folder (a repo shipping a symlink named `.bun-tag` now gets a real tag; its target is still never written — test updated). Existing bare mirrors are not validated structurally; there's no marker for them. **`bun patch --commit` on git, github and tarball dependencies** failed with `Could not access '.../@gh@@@@1'` and wrote nothing (#18792, #17945): it loads its own lockfile but computed the cache path against the empty `manager.lockfile`. The loaded lockfile is moved into the manager before the path is computed (`install_with_manager` reloads it afterwards, as it already does for `bun update`; the double parse is left alone). Patch filenames additionally escape NTFS-reserved characters, which only these resolutions contain. Fixes #18792, fixes #17945. **Isolated linker hang** — a plain `bun install` in a workspace with a `patchedDependencies` entry for a git/github dependency hung forever: the installer treated every patched package as missing and re-enqueued a download the resolve phase had already completed, parking on a drained task list. It now probes the unpatched folder (computed with no patch hash) like everything else; also fixes removing an entry. **Tests:** github/git/tarball `patch --commit` flows; add → cold cache → remove → re-add of a patch under isolated for github, git and npm (the suspected stale-`.bun-tag-<hash>` skip on re-add did not reproduce, so these just pin the cycle); git checkout failure leaves only the mirror in the cache and an empty folder at the cache name is re-cloned; a pre-existing test pinned to the bogus `--commit` cache path now asserts the step that genuinely fails. ### How did you verify your code works? `bun-install` + `bun-install-registry` (427), `isolated-install` (65), `bun-install-patch`, `bun-patch` all pass locally; new tests fail on release bun (half-built `@G@` folder left behind; `--commit` error; isolated hang). Each fix was also driven by hand with the debug binary: `patch --commit` on a `file:` tarball fails on main's binary and succeeds here; a workspace with a patched local git dep installs, requires as patched, survives remove and re-add, and re-clones an emptied cache folder, with no `.tmp` residue. Windows rename/escaping legs are left to CI. Local gotcha: these files need `HOME` pointed at an empty dir if `~/.npmrc` sets `install-strategy=hoisted`. --- **Added after CI** (`25855c0fe97`): the npm add/remove/re-add test failed on Linux — the hardlink and copyfile backends overlay files onto an existing isolated store entry, so removing a patch kept every file the patched build had *added* (and its `.bun-tag-<hash>`); clonefile replaces the tree, which is why macOS passed and why the earlier "did not reproduce" was wrong — this is the staleness #37136 mentioned. The task now deletes the previous project-local package tree before rebuilding an entry (only reached when the entry needs a rebuild, so warm installs don't pay for it); the npm cycle test pins the hardlink backend. Also from review: `checkout()` uses `delete_tree` for a checked-in `.bun-tag`, so a directory under that name is replaced instead of failing the install (test added).
2 participants
Repro
exit code 2 and no patch is written. The pristine copy actually lives at
@GH@Zerthox-betterdiscord-types-7ee79af@@@1. The same failure hits git dependencies (Could not access '.../@G@') and tarball dependencies (Could not access '.../@T@0000000000000000@@@1'). npm registry dependencies patch fine.Cause
do_patch_commitloads its own copy of the lockfile (it runs before the install flow populatesmanager.lockfile), then hands the package'sResolutiontocompute_cache_dir_and_subpath, which resolved the resolution's strings againstmanager.lockfile's string buffer. That buffer is empty at this point. Lockfile strings longer than 7 bytes are stored as offsets into their lockfile's string buffer, sorepository.resolved/ tarball URLs sliced against the wrong buffer come back empty, and the computed cache folder name collapses to just its prefix and cache-version suffix (@GH@+ `` +@@@1). npm resolutions were unaffected because the version is stored inline in the `Resolution` value.This mismatch predates the Rust port: the same flow segfaulted in v1.1.42 (#18792), where
manager.lockfilewas an undefined pointer rather than an empty lockfile.Fix
compute_cache_dir_and_subpathnow takes the string buffer that owns the resolution's strings (resolution_string_bytes: Option<&[u8]>). The twobun patch --commitcall sites pass the locally loaded lockfile's buffer;bun patchand the patch-apply task keep resolving againstmanager.lockfile(their resolutions come from it). All non-npm arms (git, github, local/remote tarball, folder, workspace, symlink) go through the new parameter.Windows: patch filename escaping
With the cache path fixed, the same flow on Windows hit a second failure: the patch filename is
{name}@{resolution}.patch, and git/github resolution labels contain:(pkg@github:owner/repo#sha.patch), which NTFS rejects in filenames (STATUS_OBJECT_NAME_INVALIDwhen renaming the patch intopatches/).escape_patch_filenamenow escapes the NTFS-reserved printable characters (: ? * " < > |) the same way it already escapes/. This applies on every OS so apatches/directory committed from macOS or Linux stays checkoutable on Windows.Known limitation: workspaces with the isolated linker
In a workspace using the isolated linker (the default for new projects), the reinstall that
bun patch --commitruns after writing the patch hangs. That hang is a pre-existingbun installbug, reproducible on released bun with no patch commands involved: a workspace with an existingpatchedDependenciesentry for a github: dependency hangs the same way on a plainbun install(it completes with--linker hoisted). With this fix,bun patch --commit --linker hoistedcompletes end to end in workspaces; the isolated-linker install hang is tracked separately.Verification
Three new tests in
test/cli/install/bun-patch.test.tsrun the full install,bun patch, edit,bun patch --commitflow offline and assert the patch file contents, thepatchedDependencieskey, and the reinstalled patched package:github:dependency served by a local tarball server viaGITHUB_API_URLfile:tarball, committed by package name to also cover the name-based lookup pathAll three fail on bun 1.4.0-canary.1 with the
Could not accesserrors above and pass with this change, on Linux and Windows. The rest ofbun-patch.test.ts(34 tests) andbun-install-patch.test.ts(18 tests) pass.Fixes #18792
Fixes #17945
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-patch.test.ts