pm: honour --dry-run in link, unlink, pm version, pm cache rm, pm trust, pm pkg, and pm migrate - #41690
pm: honour --dry-run in link, unlink, pm version, pm cache rm, pm trust, pm pkg, and pm migrate#41690robobun wants to merge 5 commits into
Conversation
…st, pm pkg, and pm migrate --dry-run was parsed by each of these commands but never read. link created the global symlink, unlink deleted it, pm version wrote package.json and ran the version scripts and git commit/tag, pm cache rm deleted the cache, pm trust ran the blocked scripts and wrote package.json and bun.lock, pm pkg set/delete/fix wrote package.json, and pm migrate wrote bun.lock. Each command now stops before its first write and prints what it would have done.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 57 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (14)
Comment |
|
Updated 6:07 PM PT - Sep 7th, 2026
❌ @Jarred-Sumner, your commit 25744a2 has 7 failures in
🧪 To try this PR locally: bunx bun-pr 41690That installs a local version of the PR into your bun-41690 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds new user-facing output text across seven subcommands (which REVIEW.md treats as reviewed word-for-word) and the description flags a known test conflict with #38804, a human look at the output wording and cross-PR coordination would still be worthwhile.
What was reviewed:
- Each subcommand's dry-run guard is placed after validation and before the first write; error paths and exit codes are preserved.
pm cache rmcounter still increments in dry-run so the "Would clear N" total is correct;had_errstays false so exit is 0.unlinkreuseslink_pathafterlstat(borrow, not move) and still runs the existence check so dry-run reports the same "not globally linked" message the real run would.- Tests assert the negative contract (package.json/bun.lock/symlink unchanged) and are added to existing files per test/CLAUDE.md.
Extended reasoning...
Overview
This PR wires the already-parsed --dry-run flag through seven package-manager subcommands that previously ignored it: bun link, bun unlink, bun pm cache rm, bun pm migrate, bun pm pkg set|delete|fix, bun pm version, and bun pm trust. Each command now checks options.dry_run immediately before its first filesystem/git mutation, prints a dry run: would … line describing the action, and exits (or returns) without side effects. Read-only validation (lockfile load, git dirty check, lstat of the existing link) still runs so a dry run surfaces the same errors the real run would. Docs (docs/pm/cli/pm.mdx) and the pm version help text are updated, and tests are added to six existing test files asserting both the dry-run output and that no files are written.
Security risks
None. The change is purely additive gating that short-circuits write paths earlier; no new input parsing, no new filesystem reads of untrusted data, no network, no auth/crypto. The one new unsafe block in pm_trusted_command.rs reads (*pm_raw).options.dry_run through the same singleton raw pointer already dereferenced on the surrounding lines for options.log_level.show_progress(), with a matching SAFETY comment.
Level of scrutiny
Moderate. The mechanics are straightforward and the bug hunter ran to dry_streak with no findings, but this PR introduces roughly ten new user-facing output strings, and REVIEW.md explicitly calls out that "error messages are reviewed word-for-word as code" — a maintainer may have preferences on the dry run: prefix convention, whether pm pkg --dry-run should print the full resulting JSON to stdout, or whether link --dry-run should still create the global link parent directory (as noted in the PR description). These are design/wording judgments rather than correctness issues.
Other factors
The PR description explicitly notes a conflict with #38804, which pins bun pm migrate --dry-run as writing bun.lock — "whichever lands second needs a one-line test update." That coordination is a human decision. Test coverage is good: each subcommand's dry-run path has a test asserting output before exit code and verifying the negative contract (files unchanged, scripts not run, git untouched), placed in the existing test files per repo convention. Given the breadth (seven subcommands), the new output surface, and the known cross-PR interaction, deferring for a quick human sign-off on wording and landing order is the right call over auto-approving.
…ng the scripts `bun pm trust <names> --ignore-scripts` adds the names to trustedDependencies in package.json and skips the lifecycle scripts. The lockfile is left alone, so the next `bun install` sees a package that package.json trusts and the lockfile does not, runs its scripts, and saves the lockfile. Only the flag skips the scripts. `ignoreScripts` in bunfig.toml or .npmrc does not, because `bun pm trust <names>` is the explicit request to run those scripts. pm migrate --dry-run now names the lockfile it would write from LoadResult::save_format, so it prints bun.lockb when saveTextLockfile is false. LockfileFormat::filename and LoadResult::save_format become pub for that. pm pkg --dry-run ends its output with a newline when the package.json has no trailing newline. Folds the part of #41686 that #41690 did not cover.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
|
Status: ready for review. Reproduced on 1.4.2, 1.4.3-canary (d316760) and 1.3.14 in a temp dir with a private Scope is CI on the current head 05a3a29 (build 112076): the lanes that run the touched install tests are green. The one red job is |
The record-only --ignore-scripts mode relied on the next `bun install` running the scripts of a package that package.json newly trusts. That holds under the hoisted linker (PackageInstaller.rs checks summary.added_trusted_dependencies for already-installed packages), but not under the isolated linker: an entry that is already in node_modules/.bun takes the relink path, and Installer::next_step goes from SymlinkDependencyBinaries straight to Done when relinking, so RunPreinstall never runs. The install then saves the trust to bun.lock and the scripts never run until a forced or clean install. The same happens when trustedDependencies is edited by hand, so it is a separate isolated-linker bug, and --ignore-scripts on pm trust stays with #41686 until that is sorted out. Kept from the fold: pm migrate --dry-run names the lockfile from LoadResult::save_format, pm pkg --dry-run ends with a newline, the --dry-run line under trust in bun pm --help, and the pm pkg fix --dry-run test.
robobun
changed the title
This branch has not been deployed
2 participants
Problem
--dry-runis parsed by everybun pmcommand and bybun link/bun unlink(their--helplists it as "Perform a dry run without making changes"), but none of them read it.bun link --dry-runcreates the global symlink,bun unlink --dry-rundeletes it,bun pm version <bump> --dry-runwritespackage.json, runspreversion/version/postversion, and commits and tags,bun pm cache rm --dry-rundeletes the whole cache,bun pm trust <pkg> --dry-runruns the blocked scripts and writespackage.jsonandbun.lock,bun pm pkg set|delete|fix --dry-runwritespackage.json, andbun pm migrate --dry-runwritesbun.lock.CommandLineArguments::parsesetscli.dry_runfor every subcommand andOptions::loadcopies it tooptions.dry_run(src/install/PackageManager/PackageManagerOptions.rs:746), but only the install path consults it. The commands above live insrc/runtime/cli/and go straight to their write.Fix
options.dry_runright before its first write and stops there:link/unlinkprint the symlink path that would be created or removed,pm versionprints the new version only (no scripts, no write, no git),pm cache rmprints the cache directory and eachbunx-*temp directory it would delete,pm trustprints the scripts that would run and the names it would add totrustedDependencies,pm pkgprints the resultingpackage.jsonto stdout, andpm migrateprints the lockfile it would save (bun.lockorbun.lockb, fromLoadResult::save_format, which becomespubwithLockfileFormat::filename).pm versionstill checks for a dirty git tree,pm truststill loads the lockfile and resolves the untrusted scripts,pm migratestill parses the foreign lockfile.--dry-runis documented for these commands as "without making changes" andbun install --dry-runalready means nonode_modules, no lockfile, and nopackage.jsonwrites.test/cli/install/bun-link.test.ts,bun-pm-version.test.ts,bun-pm-pkg.test.ts,bun-pm.test.ts,migration/migrate.test.ts, andbun-install-lifecycle-scripts.test.ts(all fail on the released binary, pass with the fix). Also ran each of those files in full.Background
bun pm <cmd>,bun link, andbun unlinkshare the installCommandLineArgumentsparser andPackageManager::Options.--dry-runis one of the shared params, so every one of these commands accepts it.$BUN_INSTALL/install/global/node_modules.bun linksymlinks the current package into it, andbun link <name>in another project resolves through it.bun pm trustruns the lifecycle scripts of dependencies that were blocked at install time and records their names intrustedDependenciesin bothpackage.jsonandbun.lock.Notes
pm cache rm,pm migrate,pm pkgandpm trust --dry-run. Its extra piece,bun pm trust --ignore-scripts(record the trust inpackage.json, let the next install run the scripts), was folded in here and then backed out again (75a7a33, 05a3a29). Reason: under the isolated linker an already-installed entry takes the relink path, andInstaller::next_stepgoes fromSymlinkDependencyBinariesstraight toDonewhen relinking, so the deferred scripts never run and the following install writes the trust tobun.lockanyway. The hoisted linker handles this case inPackageInstaller.rsthroughsummary.added_trusted_dependencies. EditingtrustedDependenciesby hand has the same problem, so it is an isolated-linker bug of its own.--ignore-scriptsonpm truststays with pm: honor --dry-run on cache rm, migrate, pkg and trust, and --ignore-scripts on trust #41686.save_format-derived filename in thepm migrate --dry-runline, a trailing newline afterpm pkg --dry-runoutput whenpackage.jsonhas none, the--dry-runline undertrustinbun pm --help, and apm pkg fix --dry-runtest.bun install --dry-runstill runs the root package's lifecycle scripts and writesyarn.lock(install: clear SAVE_YARN_LOCK for --dry-run in Options::load and start no root lifecycle script #38830),bun install --dry-runwith only a foreign lockfile still writes the migratedbun.lock(install: write a migrated lockfile and its package.json edits only when the command saves a lockfile #38804), andbun add <pkg> --dry-runin an empty directory still createspackage.json(install: don't create package.json forbun add --dry-runin an empty directory #36694). Those PRs own those paths. install: write a migrated lockfile and its package.json edits only when the command saves a lockfile #38804 has a test that pinsbun pm migrate --dry-runas writingbun.lock. This PR changes that; whichever lands second needs a one-line test update.bun pm cache rm --dry-rundoes not create the cache directory. The real run creates it before deleting it (make_open_path), so the dry run prints the resolved path without opening it.bun link --dry-runstill opens (and creates if missing) the global link directory to print the full path, the same waybun unlinkalready does before its existence check. Nothing inside it is touched.bun pm version --helpprints the genericbun pmhelp. The--dry-runline was added to the per-command help thatbun pm versionprints with no bump argument, and todocs/pm/cli/pm.mdx.test/cli/install/bun-link.test.tshas one pre-existing failure under a local debug build,should link dependency without crashing: the debug build prints a Rust backtrace into the asserted stdout. It fails the same way with thesrc/changes reverted and passes in CI.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/migration/migrate.test.ts, test/cli/install/bun-link.test.ts, test/cli/install/bun-install-lifecycle-scripts.test.ts