Skip to content

install: add bun pm fetch - #29509

Open
robobun wants to merge 6 commits into
mainfrom
farm/6593003d/pm-fetch
Open

robobun wants to merge 6 commits into
mainfrom
farm/6593003d/pm-fetch

Conversation

@robobun

@robobun robobun commented Apr 20, 2026 •

Copy link
Copy Markdown
Collaborator

What

Adds bun pm fetch, a new subcommand that resolves all dependencies and downloads them into Bun's install cache without writing node_modules, running lifecycle scripts, or writing package.json / the lockfile.

$ bun pm fetch
bun pm fetch v1.4.x

Fetched 42 packages into cache [250ms]
Cache: /home/user/.bun/install/cache

Why

Useful for:

  • Warming CI caches (fetch once, share the cache directory across jobs)
  • Pre-fetching packages before going offline
  • Populating a cache without the disk/time cost of materializing node_modules

Similar to pnpm fetch.

How

Two passes, both reusing the bun install machinery:

  1. src/runtime/cli/pm_fetch_command.rs (the CLI shim) clears Do::INSTALL_PACKAGES, RUN_SCRIPTS, WRITE_PACKAGE_JSON, SAVE_LOCKFILE, SAVE_YARN_LOCK and SUMMARY, then runs the normal install_with_manager. The resolve phase already downloads the tarball of every package it resolves for the first time, so this pass covers everything that was not pinned by the lockfile yet. The shim also marks the run as a dry run: should_save_lockfile in install_with_manager has a lockfile format-migration branch (saves_migrated_lockfile: binary or migrated lockfile loaded, text lockfile to be written) that deliberately ignores Do::SAVE_LOCKFILE so --frozen-lockfile still migrates. Without this, bun pm fetch wrote a bun.lock in projects that only have a package-lock.json, and replaced a bun.lockb with bun.lock when saveTextLockfile is set. That branch is now gated on !dry_run, which also fixes bun install --dry-run writing a bun.lock in the package-lock.json case. --lockfile-only (also a shared bun pm flag, acted on before the Do flags) is cleared as well.
  2. src/install/PackageManager/PopulatePackageCache.rs (populate_package_cache, next to the existing PopulateManifestCache.rs) walks the lockfile and enqueues a download for every package whose determine_preinstall_state is Extract (npm / github / remote tarball / local tarball), using the URLs stored in the lockfile (no manifest round-trips), then waits with the same wait_for_everything_except_peers loop bun install uses. It returns a small Summary (fetched, already_cached, skipped_git) that the shim prints. Living inside bun_install means it uses the crate-internal enqueue APIs directly; the only visibility change is wait_for_everything_except_peers becoming pub(crate).

Packages are mapped back to a dependency edge (preferring a required edge) because the enqueue functions and the download-failure reporting are keyed by dependency, not package. git: dependencies that are already pinned are skipped with a note: outside of the package installer a finished clone is only used as a resolve result, so there is no checkout step that would populate the cache. Git dependencies resolved for the first time are still cached by pass 1.

runTasks.rs now increments extracted_count on a successful git checkout, mirroring the npm extract path, so git packages fetched during pass 1 are reflected in the summary (extracted_count is otherwise only used for the progress bar total).

bun pm --help and a bare bun pm print separate copies of the subcommand list (src/install/PackageManager.rs and src/runtime/cli/package_manager_command.rs); both now list fetch.

setup_global_dir now returns early once the bin directory is open: bun pm -g fetch is the first bun pm subcommand to reach install_with_manager, which calls it a second time after the bun pm dispatcher already did, and each call used to open a fresh directory fd.

Docs: new ## fetch section in docs/pm/cli/pm.mdx. Completions: the subcommand is added to completions/bun-cli.json (the entry the generator emits; a full regeneration rewrites ~1300 unrelated lines) and to the bash/fish/zsh bun pm lists.

Verification

test/cli/install/bun-pm-fetch.test.ts (every spawn drains stdout/stderr/exit concurrently; stdout is compared against an inline snapshot):

  • Fresh project: manifest + tarball requested, cache populated, no node_modules, no bun.lock / bun.lockb written; a bun install run afterwards makes zero registry requests
  • Existing lockfile + cold cache: the only request is the tarball URL from the lockfile, stderr is exactly Fetching packages (only the second pass ran), no node_modules
  • Warm cache: zero requests, stderr is empty, reports the package as already cached
  • Project with only a package-lock.json: fetches from the migrated lockfile's URL and leaves the directory listing unchanged (no bun.lock)
  • bun.lockb plus saveTextLockfile = true: directory listing unchanged (the lockb is not migrated)
  • --lockfile-only: ignored; directory listing unchanged, package still fetched
  • --no-summary and --silent: nothing on stdout (and nothing on stderr for --silent), but the fetch still happens
  • bun pm -g fetch: fetches the global install's dependencies (exercises both setup_global_dir call sites), leaves the global directory untouched
  • Both bun pm and bun pm --help list the command

test/cli/install/migration/migrate.test.ts gains a bun install --dry-run case for the package-lock.json migration (no bun.lock written).

All of them fail on a bun without this change (fetch is an unknown bun pm subcommand; the --dry-run test fails because a bun.lock is written) and pass on this branch. Also run locally: bun-pm.test.ts (18/18), test/internal/source-lints (98/98), cargo clippy -p bun_install -p bun_runtime, cargo fmt --check.

Related to #6353
Related to #7956


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

@robobun

robobun commented Apr 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:13 PM PT - Aug 20th, 2026

✅ @robobun, your commit 18a490702c76d3496d20f8817355f10383abd6ca passed in Build #101956! 🎉


🧪   To try this PR locally:

bunx bun-pr 29509

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

bun-29509 --bun

@coderabbitai

coderabbitai Bot commented Apr 20, 2026 •

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a new bun pm fetch subcommand that resolves dependencies and prefetches their artifacts into the install cache without creating node_modules; updates extraction task accounting counters; and adds tests verifying fetch behavior, cache repopulation, idempotence, and help discoverability.

Changes

Cohort / File(s) Summary
Package Manager Command Integration
src/cli/package_manager_command.zig
Adds fetch help text, imports PmFetchCommand from ./pm_fetch_command.zig, and dispatches the "fetch" subcommand to call PmFetchCommand.exec(...) then Global.exit(0).
Fetch Command Implementation
src/cli/pm_fetch_command.zig
Adds pub const PmFetchCommand and pub fn exec(...). Implements a two‑phase flow: resolve-only install pass (disables install side effects) to download tarballs, then a cache-prefetch pass that builds a preferred-edge index, filters packages by platform/preinstall state, enqueues appropriate cache operations (npm/GitHub/git/local/remote), waits for tasks, handles enqueue errors (OOM/InvalidURL), reports errors, and prints a cache summary.
Task Accounting Update
src/install/PackageManager/runTasks.zig
Increments manager.extracted_count and bun.analytics.Features.extracted_packages earlier when scheduling .extract tasks.
CLI Tests
test/cli/install/bun-pm-fetch.test.ts
Adds tests for bun pm fetch: verifies fetching into BUN_INSTALL_CACHE_DIR without creating node_modules, repopulating cache from lockfile, idempotent “already cached” behavior (no network on second run), and that bun pm help lists fetch. Tests assert network requests, cache contents, output, and exit codes.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: adding the bun pm fetch subcommand.
Description check ✅ Passed The description explains the change, motivation, implementation, and verification, although it uses headings different from the repository template.

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

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. Add support for bun fetch #6353 - Explicitly requests a pnpm fetch-equivalent command that downloads dependencies from the lockfile into cache without needing package.json files — this PR implements exactly that as bun pm fetch
  2. Support offline installs via caching dependency tarballs #7956 - Requests a way to pre-populate the tarball cache for offline/CI use — bun pm fetch directly enables this by downloading all tarballs into the install cache without writing node_modules

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #6353
Fixes #7956

🤖 Generated with Claude Code

Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread test/cli/install/bun-pm-fetch.test.ts Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/pm_fetch_command.zig`:
- Around line 73-85: The current dep-edge lookup inside the dep_id: block
repeatedly scans lockfile.buffers.resolutions (via dep_resolutions and pkg_id)
for each package, making the algorithm quadratic; precompute a reverse index
once before the package loop (e.g., map from resolution package id to array/list
of dependency indices) and then replace the inner scan in the dep_id: block to
O(1) access into that index (referencing lockfile.buffers.resolutions,
dep_resolutions, pkg_id, and lockfile.buffers.dependencies.items); ensure the
index honors the same priority logic (preferred required edge else first
optional) so the subsequent logic that checks deps[id].behavior.isRequired() and
the first optional selection remains identical.
- Around line 100-105: The InvalidURL branch in the fetch error handler
currently just logs via Output.warn and continue, allowing cold-cache runs to
exit 0 despite missing packages; instead, in the catch |err| switch (err) for
the fetch logic (the block that calls bun.outOfMemory()), replace the simple
warn+continue behavior for error.InvalidURL with a concrete failure or fallback:
either enqueue the package using the manifest tarball path (fallback to
manifest-resolved URL) or record an install error via your install-error
reporting API (e.g., call the same error-recording mechanism used elsewhere) and
propagate a non-success status so the run fails; make the same change for the
other identical branches referenced (the blocks around the same switch at the
other locations mentioned).
- Around line 54-65: The switch in pm_fetch_command.zig incorrectly treats the
.calc_patch_hash case as already cached; change the switch handling so that
.calc_patch_hash is removed from the .apply_patch group and instead handled like
.extract (i.e., do not increment already_cached or continue for
.calc_patch_hash). Keep .done behavior the same, keep .apply_patch grouped with
the behavior that really implies the tarball is present, and ensure
pm.determinePreinstallState’s .calc_patch_hash falls through to the
extraction/fetch path so missing tarballs are fetched.

In `@test/cli/install/bun-pm-fetch.test.ts`:
- Line 52: The test defines a snake_case variable cache_dir; rename it to
camelCase cacheDir throughout test/cli/install/bun-pm-fetch.test.ts (including
the other occurrences referenced) and update every reference/usage (assignments,
joins, and any assertions) to use cacheDir so the file follows the repo's
TypeScript camelCase naming convention.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7e65a253-de5c-4043-95eb-5237f6a42d20

📥 Commits

Reviewing files that changed from the base of the PR and between c65a1df and 6a30997.

📒 Files selected for processing (3)
  • src/cli/pm_fetch_command.zig
  • src/install/PackageManager/runTasks.zig
  • test/cli/install/bun-pm-fetch.test.ts

Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread test/cli/install/bun-pm-fetch.test.ts Outdated
@coderabbitai

coderabbitai Bot commented Apr 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

3 similar comments
@coderabbitai

coderabbitai Bot commented Apr 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

@coderabbitai

coderabbitai Bot commented Apr 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

@coderabbitai

coderabbitai Bot commented Apr 20, 2026 •

Copy link
Copy Markdown
Contributor

✅ No merge conflicts detected when merging into main.

Your branch is good to go!

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/pm_fetch_command.zig`:
- Around line 88-96: The code currently silently continues when encountering
states like .calc_patch_hash (and
.calcing_patch_hash/.unknown/.extracting/.applying_patch) which can hide
unexpected cache-miss cases; update the match branch that currently does "=>
continue" to emit a diagnostic (debug or warning) when state == .calc_patch_hash
(and similarly for the other unexpected states) including identifying info
(package name or id and pass number) using the existing logger in scope (the
same logger used elsewhere in pm_fetch_command.zig / installWithManager), then
continue as before so the behavior is unchanged but diagnosable.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b102f3b5-cbab-44bc-b819-a6eb04709012

📥 Commits

Reviewing files that changed from the base of the PR and between 6a30997 and 324cf20.

📒 Files selected for processing (1)
  • src/cli/pm_fetch_command.zig

Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread test/cli/install/bun-pm-fetch.test.ts Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from d623496 to fe626ca Compare May 5, 2026 03:15
@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from fe626ca to b88c15c Compare May 14, 2026 23:50
Comment thread src/runtime/cli/pm_fetch_command.zig Outdated
@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from 2619307 to 82d83d7 Compare May 15, 2026 02:17
Comment thread src/runtime/cli/pm_fetch_command.rs Outdated
@robobun

robobun commented May 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Rebased onto current main (3ecf625), plus bfa1ffc (review fixes: idempotent setup_global_dir, tests drain pipes concurrently and snapshot stdout, new -g test) 95d7313 (docs section, completions, fetch-then-install test stage) 7d02031 (--no-summary honored for the result lines) and 46737f3 (fetch never writes a lockfile, including the format-migration path; the same gate fixes bun install --dry-run writing a bun.lock for a migrated package-lock.json), and 8574446 (--lockfile-only ignored). Because main narrowed bun_install's internals to pub(crate), the download pass moved into bun_install (src/install/PackageManager/PopulatePackageCache.rs, modeled on PopulateManifestCache.rs) and src/runtime/cli/pm_fetch_command.rs is now a thin shim; see the rebase comment below for details. Behavior is unchanged.

Verified locally on the rebased branch:

  • test/cli/install/bun-pm-fetch.test.ts: 11/11 pass with the change (including bun install after bun pm fetch making zero registry requests); all fail without it (fetch is an unknown bun pm subcommand)
  • test/cli/install/migration/migrate.test.ts (new --dry-run case): 126/126
  • test/cli/install/bun-pm.test.ts: 18/18
  • test/internal/source-lints: 98/98
  • cargo clippy -p bun_install -p bun_runtime, cargo fmt --check: clean

Review threads: all resolved; the automated review of 8574446 found nothing further and asks for a human look at the design (the two points are summarized in the comment below). Rebased again onto current main (now 89a620a; bun pm licenses / dedupe / prune landed next to this in the help texts and completions, and the migration branch became saves_migrated_lockfile, so the !dry_run gate now wraps that call). Rebased again onto current main (18a4907; one conflict with the run_tasks refactor in #39770, resolved by keeping the git-checkout counter increment next to main's new condition). The last full run, on 89a620a (#96676), had 175 jobs passed and 0 test failures; its only red was 4 Windows jobs canceled by an agent-provisioning failure. Waiting on CI for 18a4907.

Earlier CI history (pre-rebase builds)

On the previous revision (88d1bc9), builds #64919 and #64929 were red only on the macOS aarch64 test lanes: a buildkite-agent artifact download timed out before any test ran on darwin 26, and four unrelated failures on darwin 14 (autobahn.test.ts docker service failed to start, bun-serve-file.test.ts and fetch-file-upload.test.ts timeouts, test-tls-client-destroy-soon.js assertion). bun-pm-fetch.test.ts has never appeared in a failure annotation on any build of this PR.

@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from e570e6e to a96ab71 Compare May 15, 2026 03:13
@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from a96ab71 to 359779d Compare June 26, 2026 09:21
@robobun

robobun commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (359779d).

This one was non-trivial: main removed every .zig file under src/ (the Zig-to-Rust migration is complete), so the Zig reference files this PR previously carried (pm_fetch_command.zig, plus edits to package_manager_command.zig and runTasks.zig) conflicted as modify/delete on 12 of 15 commits. Since the Zig history is meaningless now, I rebuilt the branch as origin/main + the net diff of the surviving files, squashed into a single commit.

The PR now contains only:

  • src/runtime/cli/pm_fetch_command.rs (new)
  • src/runtime/cli/mod.rs (module registration)
  • src/runtime/cli/package_manager_command.rs (dispatch + help text)
  • src/install/PackageManager/runTasks.rs (+2 lines: count git checkouts in extracted_count)
  • test/cli/install/bun-pm-fetch.test.ts

Two API renames on main were picked up: invalid_dependency_id -> INVALID_DEPENDENCY_ID, and get_cache_directory() now returns Fd directly.

Verified: cargo check and cargo fmt --check clean; bun-pm-fetch.test.ts 4/4 pass on this branch and 0/4 on main; bun-pm.test.ts 18/18.

Comment thread src/runtime/cli/pm_fetch_command.rs Outdated
Comment thread src/runtime/cli/pm_fetch_command.rs Outdated
@robobun

robobun commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 88d1bc9 addressing both review findings:

  1. clippy disallowed_methods: all 8 Output::pretty*(format_args!(...)) call sites now use the bun_core::pretty! / prettyln! / pretty_errorln! macros. Verified: cargo clippy -p bun_runtime reported exactly 8 disallowed method errors on the previous code and 0 after. This also removes the runtime tag-walk over the interpolated cache path, so a < in BUN_INSTALL_CACHE_DIR can no longer be misparsed as a color tag.
  2. lockfile side effect: bun pm fetch now sets do_.save_lockfile(false) and never writes bun.lock (previously a project with no lockfile got one). The first test asserts no bun.lock/bun.lockb is created. The pre-existing binary→text migration for legacy bun.lockb is unchanged (it intentionally bypasses save_lockfile, same as --frozen-lockfile).

cargo clippy, cargo fmt --check, bun-pm-fetch.test.ts (4/4), and bun-pm.test.ts (18/18) all pass locally.

Comment thread src/runtime/cli/pm_fetch_command.rs
Comment thread test/cli/install/bun-pm-fetch.test.ts Outdated
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed bfa1ffc for the two review findings on 3ecf625:

  • setup_global_dir is now idempotent (returns early once options.global_bin_dir is open), so bun pm -g fetch reaching it from both the bun pm dispatcher and install_with_manager no longer opens a second bin-directory fd. New test runs bun pm -g --config=... fetch against the local registry and checks the global directory is left untouched.
  • The tests now go through one spawn helper that drains stdout, stderr and the exit code with Promise.all; nothing is piped without being read. While converting them to full-stdout snapshots I also gave the banner the same blank line bun install prints after its own, and the lockfile run now asserts its stderr is exactly Fetching packages (only the second pass ran) while the warm-cache run asserts empty stderr.

Both threads are resolved; the PR description's verification section is updated. Locally: 6/6 tests, clippy and fmt clean.

Comment thread src/runtime/cli/package_manager_command.rs
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 95d7313 for the docs/completions finding:

  • docs/pm/cli/pm.mdx gains a ## fetch section (placed between cache and migrate, the same order as the help text). Every claim in it was checked against the build: no node_modules, no lockfile written, lockfile URLs used when a lockfile exists, -g, and the git note.
  • completions/bun-cli.json gets the fetch entry exactly as misctools/generate-cli-completions.ts emits it; I did not commit a full regeneration because the checked-in file is behind the current help texts and regenerating rewrites about 1300 unrelated lines. The bash, fish and zsh bun pm lists also get fetch.
  • While verifying the docs I added the end-to-end stage the feature exists for to the first test: a bun install run right after bun pm fetch makes zero registry requests and installs from the cache.

All review threads are resolved; PR description updated. The branch is now 3ecf625 (feature) + bfa1ffc (review fixes) + 95d7313 (docs/completions).

Comment thread src/runtime/cli/pm_fetch_command.rs Outdated
Comment thread src/install/PackageManager/PopulatePackageCache.rs
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 7d02031 for the two findings on 95d7313:

  • bun pm --no-summary fetch now prints nothing on stdout, like bun install --no-summary: the result lines use the same should_print_command_name() answer as the banner, captured before the Do flags are cleared for the resolve pass. The test file gains --no-summary and --silent cases (the former fails on 95d7313).
  • The AlreadyFailed arm in populate_package_cache is split off with a comment that matches the actual mechanism (resolve-phase download failures stay Extracting and are skipped before anything is enqueued). Behavior unchanged.

Threads resolved, description updated. Locally 8/8 tests, clippy and fmt clean.

Comment thread src/runtime/cli/pm_fetch_command.rs
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 46737f3 for the lockfile finding on 95d7313 / 7d02031. It turned out to be a real behavior bug rather than a docs wording issue, and wider than the reported case:

  • should_save_lockfile in install_with_manager has a format-migration branch that ignores Do::SAVE_LOCKFILE on purpose (so --frozen-lockfile still migrates). It fires not only for bun.lockb + saveTextLockfile = true, but for any lockfile migrated from package-lock.json (reported as binary format, saved as text). Reproduced: bun pm fetch in a project with only a package-lock.json wrote a bun.lock, and so did bun install --dry-run.
  • Fix: that branch is now additionally gated on !options.dry_run, and bun pm fetch marks itself as a dry run (it already clears exactly the flags --dry-run clears, plus scripts / yarn.lock / summary). --frozen-lockfile behavior is unchanged; existing --dry-run tests in bun-update, catalogs and minimum-release-age still pass.
  • Tests: two new bun pm fetch cases (package-lock.json project, and lockb + saveTextLockfile) assert the directory listing is unchanged, and migration/migrate.test.ts gains a bun install --dry-run case. All three fail on the previous revision. The docs sentence about the lockfile is now accurate as written.

Thread resolved, description updated. Locally: bun-pm-fetch 10/10, migrate.test.ts 21/21, clippy and fmt clean.

Comment thread src/runtime/cli/pm_fetch_command.rs
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 8574446 for the --lockfile-only finding: the shim now clears options.lockfile_only alongside setting dry_run, so the last path by which pass 1 could write a lockfile is closed. With that, every lockfile-writing path in install_with_manager (normal save, format migration, --lockfile-only) is covered by a test in bun-pm-fetch.test.ts that checks the directory listing before and after. Thread resolved, description updated; 11/11 locally, clippy and fmt clean.

@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.

I reviewed this revision and the automated bug hunt found nothing new — all earlier inline findings (docs/completions, --no-summary, the lockfile format-migration write, --lockfile-only) are addressed with tests. Because this adds a new user-facing bun pm subcommand and changes shared bun install --dry-run behavior (the !dry_run gate on should_save_lockfile's migration branch), a human look at the API/design would still be worthwhile.

What was reviewed:

  • PopulatePackageCache.rs: dependency-edge mapping, preinstall-state handling, and the ForTarballError arms — the enqueue paths mirror the installer's and errors surface via manager.log/any_failed_to_install.
  • install_with_manager.rs: the new !dry_run gate only affects the binary→text migration disjunct; --frozen-lockfile still migrates (it doesn't set dry_run).
  • runTasks.rs: the added extracted_count += 1 on git checkout also fires during regular bun install; extracted_count is otherwise only read for the progress bar total, so no summary regression.
  • setup_global_dir idempotency: global_bin_dir starts Fd::INVALID and is only assigned here, so the early return is sound.
Extended reasoning...

Overview

This PR adds bun pm fetch, a new subcommand that resolves dependencies and downloads them into the install cache without touching node_modules, lifecycle scripts, package.json, or the lockfile. It spans 16 files: two new Rust files (PopulatePackageCache.rs in bun_install, pm_fetch_command.rs in the CLI), edits to install_with_manager.rs / runTasks.rs / PackageManagerDirectories.rs / PackageManagerOptions.rs, help text in two places, docs, four completion files, and ~300 lines of new tests. It also fixes a pre-existing bug where bun install --dry-run wrote bun.lock when migrating from package-lock.json.

Security risks

None identified. The command reuses the existing install_with_manager resolve/download machinery and the same enqueue functions the installer uses; no new parsing of untrusted input, no new filesystem paths derived from user data. The unsafe block reading ROOT_PACKAGE_JSON_PATH follows the same pattern as other bun pm subcommands and has a SAFETY comment.

Level of scrutiny

High. This is new user-facing CLI surface in the package manager (a critical, widely-exercised subsystem), and it changes behavior in shared code:

  • install_with_manager.rs now gates the lockfile format-migration write on !dry_run, which changes bun install --dry-run semantics for projects with package-lock.json or bun.lockb + saveTextLockfile. This is arguably a bug fix, but it's a behavior change to an existing command that a maintainer should confirm.
  • runTasks.rs now increments extracted_count and calls analytics::Features::extracted_packages_inc() on the git-checkout success path, which affects every bun install (progress bar totals and analytics), not just bun pm fetch.
  • The design decision to skip already-pinned git: dependencies (with a printed note) rather than implementing a checkout-into-cache path is a UX/scope call a maintainer should weigh in on.

Other factors

The PR went through five rounds of review feedback, each surfacing an edge case (missing docs/completions, --no-summary not honored, lockfile written on the migration path, lockfile written on bun.lockb + saveTextLockfile, --lockfile-only bypass). All were fixed with dedicated tests, and this run's bug hunt found nothing further. Test coverage is thorough for the paths exercised (fresh project, existing lockfile, warm cache, package-lock.json migration, bun.lockb migration, --lockfile-only, --no-summary/--silent, -g, help text). The iteration history and the scope of shared-code changes are exactly why a human should sign off on the final shape rather than an automated approval.

@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Automated review is clean as of 8574446 (all earlier findings fixed with tests). For whoever picks this up, the two judgment calls in the diff that are worth a deliberate look, since the rest is mechanical:

  1. Already-pinned git: dependencies are skipped with a note instead of being checked out into the cache. Outside the package installer a finished clone is only used as a resolve result (run_tasks only schedules the checkout when IS_PACKAGE_INSTALLER), so supporting them would mean teaching that handler a third mode; I kept it out of this PR. New git dependencies are still cached by the resolve pass.
  2. install_with_manager.rs: the lockfile format-migration branch of should_save_lockfile is now skipped when options.dry_run is set. bun pm fetch relies on it, and it also changes bun install --dry-run, which used to write a bun.lock when migrating from a package-lock.json (test added in migration/migrate.test.ts). --frozen-lockfile is unaffected since it does not set dry_run.

@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from 8574446 to 89a620a Compare August 14, 2026 21:34
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (89a620a). Conflicts were all adjacency with #38333 (bun pm licenses and friends): the mod.rs declaration and import next to pm_licenses_command, the bash/fish bun pm word lists (both now contain licenses and fetch), and should_save_lockfile, where main extracted the migration condition into saves_migrated_lockfile(); the !dry_run gate now wraps that call, same semantics. Help texts, zsh, bun-cli.json and the docs merged cleanly with fetch still between cache and migrate. Nothing else changed.

Two things from main that this PR picks up without code changes: determine_preinstall_state now goes through the completion-marker-aware is_package_in_cache, so the second pass inherits that, and the dispatcher now rejects --filter for every bun pm subcommand except licenses, which includes fetch.

Rebuilt and re-ran: bun-pm-fetch.test.ts 11/11, migration/migrate.test.ts 126/126, bun-pm.test.ts 18/18, source lints, clippy, fmt, prettier all clean.

@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.

I reviewed this PR again after the fixes for the earlier findings and found no new issues. Because it adds a new user-facing bun pm subcommand and changes bun install --dry-run behavior (the format-migration branch of should_save_lockfile is now gated on !dry_run), a human sign-off on the two judgment calls the author flagged — skipping already-pinned git dependencies, and the --dry-run migration change — would still be worthwhile.

What was reviewed:

  • The three lockfile-write paths in install_with_manager (normal save, format migration, --lockfile-only) — each now covered by a directory-listing test.
  • setup_global_dir idempotency and the bun pm -g fetch double-call path.
  • --no-summary / --silent gating and the extracted_count increment on git checkout in runTasks.rs.
  • Test file: every spawn drains stdout/stderr/exit concurrently, uses the local dummy registry, and asserts full snapshots.
Extended reasoning...

Overview

This PR adds bun pm fetch, a new user-facing subcommand that downloads all dependencies into Bun's install cache without writing node_modules, running scripts, or touching package.json / the lockfile. It consists of two new files (pm_fetch_command.rs, ~96 lines; PopulatePackageCache.rs, ~180 lines), small edits to five shared install files, docs, four completion files, and a ~290-line test file plus one test in migrate.test.ts.

The shared-code changes are: (1) install_with_manager.rs — the format-migration disjunct of should_save_lockfile is now additionally gated on !options.dry_run, and wait_for_everything_except_peers becomes pub(crate); (2) runTasks.rs — a successful git checkout now increments extracted_count and the analytics counter, mirroring the npm extract path; (3) PackageManagerDirectories.rs — setup_global_dir returns early if global_bin_dir is already open; (4) PackageManagerOptions.rs — lockfile_only widened from pub(crate) to pub.

Security risks

None identified. The command reuses the existing resolve and download machinery unchanged; the new code enqueues tarballs by the URLs already stored in the lockfile via the same enqueue_* helpers bun install uses, and does no new parsing of untrusted input. The visibility widenings (lockfile_only to pub, wait_for_everything_except_peers to pub(crate)) don't expose anything security-relevant.

Level of scrutiny

Medium-high. This is a new user-facing CLI surface (name, semantics, output format, and the documented git-dependency caveat are all API decisions), and one of the shared-code edits changes the behavior of an existing command: bun install --dry-run on a project with only a package-lock.json (or a bun.lockb with saveTextLockfile = true) previously wrote a bun.lock and now does not. That change is defensible and has a test in migrate.test.ts, but it is a user-visible behavior change to bun install riding in on a feature PR, and --frozen-lockfile is deliberately unaffected — a maintainer should confirm that split is intended.

Other factors

The PR went through several rounds of automated review; every earlier finding (setup_global_dir double-open, undrained pipes, --no-summary, the format-migration and --lockfile-only lockfile-write bypasses, the AlreadyFailed comment) was fixed with a test that fails on the prior revision. Test coverage is thorough for the happy paths and the specific edge cases raised. The author's own closing comment explicitly flags two judgment calls for a human: skipping already-pinned git: dependencies rather than teaching run_tasks a checkout-only mode, and the --dry-run gate on the migration branch. Given those open design questions and the size of the new surface, deferring to a human reviewer rather than auto-approving.

@robobun

robobun commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (8326d1b). The only conflicts were the two insertion points bun pm diff (#39229) landed on as well: the module declaration in cli/mod.rs and the import in package_manager_command.rs; both now list diff, fetch and licenses. Everything else, including the !dry_run gate around saves_migrated_lockfile and the docs section, merged cleanly, and the diff against main is unchanged in content (same 16 files). Rebuilt and re-ran: bun-pm-fetch.test.ts 11/11, migration/migrate.test.ts 126/126, bun-pm.test.ts 18/18, source lints, clippy, fmt and prettier clean.

Unrelated observation while resolving: #39229 added diff to the bun pm help text in package_manager_command.rs but not to the second copy in src/install/PackageManager.rs (the one bun pm --help prints), so the two copies are out of sync on main; not touched here.

@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from 89a620a to f087da3 Compare August 18, 2026 03:14

@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.

Automated review is clean on this revision — no new findings. This adds a new user-facing bun pm subcommand and changes bun install --dry-run behavior around lockfile migration, so a human sign-off on the two design calls the author flagged (skipping already-pinned git dependencies, and the !dry_run gate on saves_migrated_lockfile) is still worth having.

What was reviewed:

  • PopulatePackageCache.rs: dependency-edge indexing, preinstall-state handling, enqueue paths per resolution tag, and the wait_for_everything_except_peers reuse.
  • pm_fetch_command.rs: all three lockfile-write bypasses (Do::SAVE_LOCKFILE, format migration via dry_run, --lockfile-only) are now closed and covered by directory-listing tests.
  • setup_global_dir idempotence and the runTasks.rs git-checkout extracted_count increment — both look correct.
  • Tests drain stdout/stderr/exit concurrently, use a local registry, and snapshot full stdout.
Extended reasoning...

Overview

This PR adds bun pm fetch, a new subcommand that resolves dependencies and downloads them into the install cache without touching node_modules, lifecycle scripts, package.json, or the lockfile. It touches 16 files: two new Rust files (PopulatePackageCache.rs ~180 lines, pm_fetch_command.rs ~96 lines), small edits to five existing package-manager Rust files, docs, four completion files, and two test files (~290 new test lines, one new case in migrate.test.ts).

The changes to existing code are: (1) install_with_manager.rs gates the saves_migrated_lockfile branch of should_save_lockfile on !dry_run, which also changes bun install --dry-run (previously wrote a bun.lock when migrating from package-lock.json); (2) wait_for_everything_except_peers becomes pub(crate); (3) runTasks.rs increments extracted_count on git checkout success (mirrors the npm extract path); (4) setup_global_dir is now idempotent; (5) lockfile_only becomes pub; (6) help text, docs, and completions gain the new subcommand.

Security risks

None identified. The command reuses the existing bun install resolve/download machinery and lockfile URLs; it introduces no new network parsing, no new user-controlled path handling, and no auth changes. unsafe in pm_fetch_command.rs reads a static written once during PackageManager::init on the same thread, with a SAFETY comment.

Level of scrutiny

Medium-high. This is new user-facing API surface in the package manager (a critical subsystem), and it changes behavior of an existing command (bun install --dry-run no longer writes a migrated lockfile). The author explicitly flagged two design decisions for a maintainer: whether skipping already-pinned git dependencies (with a printed note) is acceptable, and whether the !dry_run gate on the migration branch is the right layering. Per the repo's review guidance on API design and on fixing at the layer that owns the invariant, both deserve a deliberate human look rather than an automated approval.

Other factors

This PR has been through many automated review rounds; every prior finding (subprocess pipe draining, --no-summary handling, the three lockfile-write bypasses, docs/completions coverage, the AlreadyFailed comment) was fixed with a test. Test coverage is thorough: fresh project, existing lockfile + cold cache, warm cache, package-lock.json migration, lockb+saveTextLockfile, --lockfile-only, --no-summary/--silent, -g, and both help copies. The bug hunting system found nothing on this revision. All review threads are resolved. The remaining reason to defer is the scope (new subcommand + behavior change to --dry-run), not any specific concern with the implementation.

Downloads every dependency into the global cache without installing
anything into node_modules, running lifecycle scripts, or writing
package.json / the lockfile. Useful for warming the cache in CI images
and for preparing offline installs.

The command runs the normal resolve phase with `Do::INSTALL_PACKAGES`
(and the other write/side-effect flags) cleared, which also downloads
any package that was not already pinned by the lockfile. It then runs
`populate_package_cache`, which walks the lockfile and downloads the
packages the resolve phase skipped because they were already resolved,
reusing the same enqueue functions and wait loop as `bun install`.

`git:` dependencies that are already pinned are left to `bun install`:
outside of the package installer a finished clone is only used as a
resolve result, so there is no checkout step to populate the cache.
Successful git checkouts now count toward `extracted_count`, matching
the npm extract path, so git packages fetched during the resolve phase
show up in the summary.

`bun pm --help` and a bare `bun pm` print separate copies of the
subcommand list; both now list `fetch`.
`bun pm -g fetch` reaches setup_global_dir twice (the `bun pm`
dispatcher and install_with_manager), and each call opened a fresh bin
directory fd while orphaning the previous one. Return early once
global_bin_dir is set.

The banner is now followed by a blank line like `bun install`'s.

Tests drain stdout/stderr concurrently, snapshot the full stdout, assert
the exact stderr of the lockfile and warm-cache runs, and cover `-g`.
Adds a `## fetch` section to docs/pm/cli/pm.mdx and the subcommand to
bun-cli.json (the entry the generator produces; a full regeneration
would rewrite ~1300 unrelated lines) and to the bash/fish/zsh lists.

The fresh-fetch test now also runs `bun install` afterwards and checks
it makes no registry requests.
The banner already went through should_print_command_name(), which is
off under both --silent and --no-summary, but the "Fetched ..." and
"Cache: ..." lines only checked for --silent. Use the same answer for
both, captured before the Do flags are cleared for the resolve pass.

Also split the AlreadyFailed arm off with an accurate comment: packages
whose download failed during the resolve pass stay in the Extracting
state and are skipped before anything is enqueued, so that error is not
how pass 1 failures reach this code.
Clearing Do::SAVE_LOCKFILE is not enough to keep install_with_manager
from writing a lockfile: the format-migration branch of
should_save_lockfile (binary or migrated lockfile loaded, text format
to be saved) ignores that flag on purpose so --frozen-lockfile still
migrates. `bun pm fetch` in a project with only a package-lock.json
therefore wrote a bun.lock, and with a bun.lockb plus
saveTextLockfile=true it replaced the lockb with bun.lock. `bun install
--dry-run` had the same problem in the package-lock.json case.

Gate that branch on !dry_run and mark fetch as a dry run, which is what
it is. Tests cover both fetch cases and the --dry-run case; all three
fail without the change.
--lockfile-only is a shared `bun pm` flag and install_with_manager acts
on it before looking at the Do flags, so `bun pm --lockfile-only fetch`
wrote a lockfile. Clear it like the other install side effects; the
packages it would have skipped prefetching are picked up by the second
pass anyway.
@robobun

robobun commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (18a4907). One conflict: #39770 replaced the C::HAS_ON_EXTRACT const generic in run_tasks with the runtime cb.has_on_extract flag right where this PR adds the extracted_count increment on the git-checkout path; resolved by keeping the increment above main's new condition, so the git path still mirrors the npm extract path line for line. Diff against main is otherwise unchanged (same 16 files). Rebuilt and re-ran: bun-pm-fetch.test.ts 11/11, migration/migrate.test.ts 126/126, bun-pm.test.ts 18/18, source lints, clippy, fmt and prettier clean.

@robobun
robobun force-pushed the farm/6593003d/pm-fetch branch from f087da3 to 18a4907 Compare August 21, 2026 00:41

@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.

I reviewed this PR and the automated pass found no bugs; all earlier findings (lockfile-write bypasses via format migration and --lockfile-only, --no-summary, double setup_global_dir, docs/completions) have been fixed with tests. Because it adds a new user-facing subcommand, changes bun install --dry-run behavior via the !dry_run gate on saves_migrated_lockfile, and skips already-pinned git dependencies by design, a human look at those two calls is still worthwhile.

What was reviewed:

  • PopulatePackageCache.rs: dependency-edge mapping, preinstall-state gating, enqueue error handling, and the wait loop reuse — matches install_with_manager's shape.
  • pm_fetch_command.rs: every lockfile-write path in pass 1 (Do::SAVE_LOCKFILE, migration branch, --lockfile-only) is closed and covered by a directory-listing test.
  • runTasks.rs git-checkout extracted_count increment mirrors the npm extract path; only affects the progress total for regular installs.
  • setup_global_dir idempotency guard checked against its only other caller.
Extended reasoning...

Overview

Adds bun pm fetch (analogue of pnpm fetch): resolve + download all dependencies into the install cache without writing node_modules, lifecycle scripts, package.json, or the lockfile. 16 files: two new (PopulatePackageCache.rs, pm_fetch_command.rs), edits to install_with_manager.rs / runTasks.rs / PackageManagerDirectories.rs / PackageManagerOptions.rs, help text in two places, docs, four completion files, and two test files (11 new tests plus one --dry-run migration test).

Security risks

None identified. No new parsing of untrusted input; downloads go through the existing enqueue helpers keyed by lockfile URLs. No auth/crypto/permissions code touched.

Level of scrutiny

High — this is new user-facing API surface in the package manager, and it makes a small behavior change to an existing command: install_with_manager's should_save_lockfile migration branch is now gated on !options.dry_run, so bun install --dry-run no longer writes a bun.lock when migrating from package-lock.json or a binary lockfile. That is arguably a bug fix, and it has a test in migrate.test.ts, but it is a shared-path change a maintainer should sign off on. The author also flagged the decision to skip already-pinned git: dependencies (with a printed note) rather than teaching run_tasks a checkout-only mode.

Other factors

Every earlier automated finding on this PR was addressed with a failing-then-passing test; the test file covers fresh project, existing lockfile + cold cache, warm cache, package-lock.json migration, bun.lockb + saveTextLockfile, --lockfile-only, --no-summary/--silent, -g, and both help copies. The three rebases since the last review round were adjacency-only (bun pm licenses, bun pm diff, and the has_on_extract refactor). The lockfile_only field visibility widened from pub(crate) to pub so the CLI shim in bun_runtime can clear it — consistent with the neighboring dry_run field. The setup_global_dir early-return is safe: the only other caller is install_with_manager, and global_bin_dir starts at Fd::INVALID.

Given the new API surface and the two author-flagged design calls, deferring to a human rather than approving.

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.

1 participant