Repository navigation
Conversation
…y directory
`bun add <pkg> --dry-run` and `bun install <pkg> --dry-run` in a
directory without package.json were writing `{"dependencies": {}}` to
disk before the dry_run flag was ever consulted.
`PackageManager::init` walks up looking for package.json and, for
`bun install <pkg>`, creates one when none is found so the add can
proceed. `update_package_json_and_install_and_cli` does the same for
`bun add` on the MissingPackageJSON retry path. Both happened
unconditionally.
Skip the disk write when `cli.dry_run` is set: hold an invalid fd for
the root file, fill `ROOT_PACKAGE_JSON_PATH` from the computed path
instead of `get_fd_path`, and seed `workspace_package_json_cache` with
an in-memory `{"dependencies": {}}` so later lookups never touch disk.
Fold the `bun add` creation path into `init()` so dry-run doesn't
need the create-then-retry dance.
Fixes #13244.
|
Warning Review limit reached
Next review available in: 7 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 (8)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I checked two things and ruled them out: (1) downstream reads of manager.root_package_json_file with the new Fd::invalid() sentinel — the only field reader is pm_trusted_command.rs (different subcommand), and init_runtime_manager already stores Fd::invalid() there, so this is an established pattern; (2) cache-key agreement between seed_with_contents(original_package_json_path) and the later get_with_path(root_package_json_path) in updatePackageJSONAndInstall.rs — both derive from the same cwd via b"/package.json" and both normalize to POSIX separators on Windows, so the seeded entry is hit.
Extended reasoning...
This PR threads a synthetic invalid-fd + seeded-cache path through PackageManager::init() for the dry-run/no-package.json case, and folds the bun add create-then-retry into init(). The two inline findings are nits (dead retry arm, unfixed link/unlink siblings). I additionally traced the invalid Fd through the struct field to confirm no other consumer reads it on the add/install path, and checked that the seeded cache key matches both get_with_path call sites in the install-with-manager flow (including Windows separator normalization). Given the change touches core init control flow and introduces a new sentinel-fd pattern, deferring to human review rather than approving.
…unlink
The create-then-retry in update_package_json_and_install_and_cli became
dead once init() handles Subcommand::Add directly; drop it along with the
now-unused import and the no-longer-needed cli.clone().
Apply the same dry-run guard to bun link and bun unlink: both were
catching MissingPackageJSON and writing package.json unconditionally.
Under --dry-run they now propagate the error ("could not find a
package.json") instead of touching disk.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier nits are addressed — the link/unlink sibling sites now guard on !dry_run with tests, and the dead retry path in update_package_json_and_install_and_cli is gone. I didn't find any new issues in the follow-up commits.
Deferring only because this threads a synthetic Fd::invalid() root file through PackageManager::init() and adds a cache-seeding path that all install/add flows depend on — worth a maintainer glance at the approach rather than an auto-approve.
Checked: root_package_json_file is otherwise only read by bun pm trusted (unaffected here), and the runtime-init path already stores Fd::invalid() in that slot, so the sentinel is a known-safe state. seed_with_contents mirrors get_with_path's Windows path-normalization, so both cache lookups in updatePackageJSONAndInstall should hit the seeded entry.
Extended reasoning...
Overview
Fixes #13244: bun add|install <pkg>|link|unlink --dry-run in an empty directory was writing package.json to disk before options.dry_run was consulted. The fix moves the create-or-synthesize decision into PackageManager::init(): under --dry-run it holds Fd::invalid() for the root file, fills ROOT_PACKAGE_JSON_PATH from the computed path instead of get_fd_path(fd), and seeds workspace_package_json_cache with an in-memory {"dependencies": {}} entry so downstream lookups never touch disk. bun link/bun unlink now propagate MissingPackageJSON under --dry-run instead of writing a file. Eight files touched; four new tests covering all four commands.
Follow-up since last review
Commit 25703472 addressed both nits I raised: (1) the link_command.rs/unlink_command.rs sibling sites now gate attempt_to_create_package_json() on !dry_run, with matching tests; (2) the dead _-arm retry in update_package_json_and_install_and_cli was removed along with the cli.clone() and unused import. Commit da3021fb trimmed the paragraph-length comments that comment-cop flagged. All prior inline threads are resolved.
Security risks
None. This narrows disk writes under an explicit no-write flag. No new untrusted-input parsing, no auth/crypto/permission surfaces.
Level of scrutiny
Medium-high. PackageManager::init() is the entry point for every install/add/remove/update/link/pm invocation, and this change introduces a sentinel fd that flows through the rest of init and into the manager struct. I traced downstream reads: manager.root_package_json_file is only otherwise consumed by pm_trusted_command.rs (a bun pm path not reachable from add/install), and the runtime-init constructor at PackageManager.rs:2334 already uses Fd::invalid() for this field, so the invalid-fd state is precedented. The new seed_with_contents mirrors get_with_path's key normalization (Windows backslash→slash) and _path_storage ownership pattern exactly, so both get_with_path calls in updatePackageJSONAndInstall (via original_package_json_path and via top_level_dir() + /package.json) should hit the seeded entry after posix-normalization.
Other factors
Test coverage is good: four new tests exercise add/install (positive — resolves against the dummy registry, exits 0, dir stays empty modulo .cache) and link/unlink (negative — surfaces "could not find a package.json" via the crash-handler's MissingPackageJSON message, exits nonzero, dir stays empty). PR description shows USE_SYSTEM_BUN=1 fails / bun bd passes for all four, and the full 58-test suite passes. CI build #87163 is running on the latest commit.
Why not auto-approve
Nothing is wrong that I can see. But the synthetic-file approach in init() is a design choice a maintainer should ratify — an alternative would have been to reorder options.load() before the create step, or to gate creation on cli.dry_run without the cache-seed path. The chosen approach is reasonable (it lets the full add pipeline run and print "installed BaR@0.0.2" as the test asserts), but it's the kind of structural decision that belongs to a human reviewer rather than a bot.
|
CI on da3021f: the new dry-run tests pass on every lane. The only |
This branch has not been deployed
2 participants
Fixes #13244.
Repro
Same for
bun install zod --dry-run,bun link --dry-runandbun unlink --dry-run. Expected:--dry-runperforms no writes.Cause
PackageManager::initwalks up from cwd looking forpackage.json. When none is found and the command isbun install <pkg>, it writes{"dependencies": {}}to disk so the add can proceed.bun add,bun linkandbun unlinkeach catch theMissingPackageJSONerror and callattempt_to_create_package_json()before retryinginit. All of this happens beforeoptions.load()readscli.dry_run.Fix
bun add/bun install <pkg>: whencli.dry_runis set and nopackage.jsonexists,init()holdsFd::invalid()for the root file instead of creating it, fillsROOT_PACKAGE_JSON_PATHfrom the computed path instead ofget_fd_path(fd), and seedsworkspace_package_json_cachewith an in-memory{"dependencies": {}}entry so every later lookup hits cache. Thebun addcreate-then-retry path is folded intoinit(), so the retry wrapper inupdate_package_json_and_install_and_cliis removed.bun link/bun unlink: under--dry-runtheMissingPackageJSONerror now propagates ("could not find a package.json") instead of writing a file and then failing on the missingname.Verification
Full
bun-add.test.tssuite: 58 pass, 0 fail.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-add.test.ts