Repository navigation
Conversation
…dencies When a git dependency has a prepare script, npm clones it, runs a full install (dependencies + devDependencies), runs prepare, and then packs and installs the result. Bun was running prepare in node_modules/<pkg> without making devDependencies available, so build tools declared as devDependencies (tsup, typescript, peggy, ...) were missing when the script ran. Inject a nested `bun install --ignore-scripts` into the preprepare step for git/github dependencies that have a prepare-family script. The nested install runs in node_modules/<pkg>, so devDependencies and their .bin entries are resolvable when preprepare/prepare/postprepare execute. Nothing is added to the consumer's lockfile and the step remains gated on trustedDependencies. Also pass file: URLs through the non-SSH clone path so git+file:// dependencies resolve; the protocol was already recognised by the dependency parser but the clone handler only accepted http(s)/ssh. Fixes #10297 Fixes #6138
|
Warning Review limit reached
Next review available in: 9 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 (2)
WalkthroughChangesGit and GitHub dependencies with prepare-phase scripts now install their development dependencies before running those scripts. File URLs remain unchanged during HTTPS conversion, and lifecycle coverage verifies git+file behavior without hoisting or lockfile persistence. Git dependency lifecycle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:06 PM PT - Jul 25th, 2026
❌ @robobun, your commit 49dcec8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35677That installs a local version of the PR into your bun-35677 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Use bun_shell_parser::escape_8bit for the bun executable path and registry URL in the synthetic preprepare command so a $, backtick or double quote in either does not misparse under sh -c / bun exec.
A user-declared preprepare with a top-level ';' or '||' would otherwise re-associate with the injected '&&' and let later commands run even if the nested install failed.
Bun's shell (used for lifecycle scripts on Windows) does not implement
'{ list ; }'; a subshell '( list )' is supported by both sh and Bun's
shell.
… line --no-save keeps the nested install from leaving a bun.lock inside node_modules/<git-dep>. Closing the chained subshell with a newline instead of a space means a user preprepare ending in an unquoted '# comment' (or a trailing backslash) does not swallow the ')' in either sh or Bun's shell.
…RL via pathToFileURL Do not place username:password@ from a bunfig registry URL onto the nested install's argv (and therefore into /proc/<pid>/cmdline, foreground command echo, and pm untrusted output). Auth for the nested install still flows through inherited env / ~/.npmrc. Use Bun.pathToFileURL() for the test's git+file dependency so Windows paths become file:///C:/... rather than file://C:/...
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install-lifecycle-scripts.test.ts`:
- Around line 1890-1968: The test setup in “git dependency devDependencies are
installed before `prepare` runs” only covers the absence of a preprepare script.
Add a preprepare fixture that invokes what-bin, writes a distinct marker, and
ends with a trailing shell comment; update the assertions to verify that marker
alongside the existing prepare marker, preserving validation of lifecycle
ordering, user-script retention, and newline-delimited chaining.
- Around line 1940-1951: Update the subprocess handling around spawn in the
install lifecycle test to start draining stdout and stderr concurrently while
also awaiting exited. Use concurrent await coordination, then derive err from
stderr output and out from stdout output without sequentially awaiting either
pipe.
🪄 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: fa8fa27e-7ade-4edb-beb6-aac4e9a23ba6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/install/Cargo.tomlsrc/install/lockfile/Package/Scripts.rssrc/install/repository.rstest/cli/install/bun-install-lifecycle-scripts.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/lockfile/Package/Scripts.rs`:
- Around line 22-37: Update registry_href_without_userinfo to determine the end
of the full URL authority, stopping at the earliest of '/', '?', or '#', before
searching for '@'. Preserve the existing userinfo removal behavior and add
regression coverage for query and fragment cases, including URLs where those
sections contain '@'.
- Around line 30-36: Route every allocation in the relevant script and
install-command paths through bun_core::handle_oom, including Vec construction,
Box conversion, and boxed-slice conversion in the shown logic. Update
git_dep_dev_install_command and the chained preprepare branch to use fallible
allocation APIs and invoke handle_oom at each allocation site, preserving the
existing output and control flow.
In `@test/cli/install/bun-install-lifecycle-scripts.test.ts`:
- Around line 1910-1911: Update the lifecycle test’s prepare script and its
corresponding assertions so prepare writes a distinct marker file or content,
separate from preprepare’s marker. Assert that prepare-specific marker after the
lifecycle runs, ensuring the test independently verifies both preprepare and
prepare phases.
🪄 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: 7136f87b-d8bb-4f2d-8894-6d3fa9882062
📒 Files selected for processing (2)
src/install/lockfile/Package/Scripts.rstest/cli/install/bun-install-lifecycle-scripts.test.ts
…s phase-specific registry_href_without_userinfo() now bounds the authority at the first of '/', '?' or '#', so an '@' inside a query string or fragment does not cause the host to be rewritten. The preprepare script in the new test no longer writes what-bin.txt itself, so that marker is produced only by prepare and the assertions prove each phase ran independently.
…g registry credentials Per WHATWG URL parsing the userinfo/host boundary is the last '@' in the authority segment, so a password containing a literal '@' is now fully stripped rather than leaving its tail on the nested install's argv.
Covers the shapes fixed over the course of review: user:pass@host, bare token with explicit port, literal @ in password (last-@ boundary), @ in query/fragment, no-scheme input. Uses slice iterator search instead of the SIMD-backed strings helpers so the test links without the C highway symbols.
options.scope.url always falls back to the npmjs default, so the previous !registry.is_empty() guard was always true and every default-registry user got --registry 'https://registry.npmjs.org/' appended. That overrides any .npmrc a git dependency commits in its own repo (npm's nested install passes no --registry for this case) and added noise to bun pm untrusted output. Gate on options.did_override_default_scope instead, which is set exactly when the configured registry differs from the default.
There was a problem hiding this comment.
No new issues found — all prior findings from this thread have been addressed (last in d400c70). Deferring to a human because this is a behavioral change to the git-dependency lifecycle path with an acknowledged interim design (nested bun install --ignore-scripts in preprepare rather than the npm-parity cache-checkout+pack approach), plus a new file: clone path in repository.rs.
What was reviewed:
registry_href_without_userinfo— authority boundary (/,?,#), last-@split, and the new unit-test table cover the shapes fixed during review.--registryforwarding is now gated ondid_override_default_scope, so a git dep's own committed.npmrcis not clobbered in the default case.- Chained
preprepareuses&& ( <user>\n)so a trailing#comment in the user script cannot swallow the closing paren;first_script_index/counterare updated on the injected-only path. try_httpsreturningfile:verbatim reachesgit cloneunchanged and does not touch the ssh/scp rewrite branches.
Extended reasoning...
Overview
Adds a nested <bun> install --ignore-scripts --no-save --no-summary --no-progress [--registry ...] step to the preprepare slot for git/github dependencies that declare any prepare-family script, so their devDependencies are resolvable when prepare runs (fixes #10297/#6138/#16548). Also enables git+file:// clones by letting try_https pass file: URLs through, adds a credential-stripping helper for the forwarded registry URL with a Rust unit-test module, and adds an integration test using a local git repo + verdaccio.
Security risks
The forwarded registry URL and the bun executable path are shell-escaped via bun_shell_parser::escape_8bit::<true, false> before being embedded in the lifecycle command, and userinfo is stripped from the registry URL before it reaches argv / bun pm untrusted output. Earlier revisions had three bypass shapes (token@host:port, @ in query/fragment, first-vs-last @); all are fixed and now covered by #[cfg(test)] cases. The nested install stays gated on trustedDependencies, so untrusted git deps do not get a nested install. I did not find a remaining injection or credential-leak path, but this is exactly the kind of surface a maintainer should sign off on.
Level of scrutiny
High. This changes what bun install executes for every trusted git dependency with a prepare script — a production-critical path — and the author has explicitly framed it as an interim approach pending a cache-checkout+pack rework. The --ignore-scripts flag is load-bearing (prevents double-prepare and unbounded recursion) but also diverges from npm by suppressing devDependency postinstall; that tradeoff was discussed and deferred to the follow-up. A human should confirm that tradeoff and the one-byte repository.rs change (file: now flows to git clone) are acceptable.
Other factors
Every review thread on the PR (github-actions, coderabbit, and my own across ~10 rounds) is resolved. The integration test exercises the chained-preprepare path including a trailing shell comment, asserts phase-specific markers, and checks that the git dep's devDependency is neither hoisted nor added to the outer lockfile. The existing "git dependencies also run preprepare, prepare, and postprepare scripts" test is unchanged. CI has not been confirmed green in this thread.
GIT_CONFIG_NOSYSTEM only skips /etc/gitconfig; point GIT_CONFIG_GLOBAL at an empty file so a developer's commit.gpgsign or core.hooksPath doesn't fail the fixture commit before the code under test runs.
There was a problem hiding this comment.
No new issues found after 49dcec8. This changes git-dependency lifecycle semantics — injecting a nested bun install into the preprepare slot and constructing a shell command that flows through the lifecycle runner — and is acknowledged as a stopgap ahead of a cache-checkout+pack rework, so it's worth a human sign-off on the approach before landing.
What was reviewed:
registry_href_without_userinfocredential redaction — the four bypass shapes fixed during review are now locked in by the#[cfg(test)]table.- Shell construction:
escape_8bit::<true, false>on bothbun_exeand the registry href, and the&& ( <user>\n)subshell wrap — the test's trailing# donecomment exercises the newline-before-)requirement. --registrynow gated ondid_override_default_scope, so a git dep's own committed.npmrcis no longer clobbered in the default case.try_httpsacceptingfile:— checked it only affects the git-clone URL selection path and doesn't collide with the ssh/scp branches.
Extended reasoning...
Overview
The PR makes bun install run a nested bun install --ignore-scripts --no-save --no-summary --no-progress [--registry …] inside node_modules/<pkg> before executing a git/github dependency's preprepare/prepare/postprepare scripts, so devDependencies (build tools) are resolvable when prepare runs. It also enables git+file:// cloning in repository.rs (needed for the offline test), adds a userinfo-stripping helper for the forwarded --registry URL with unit tests, and adds one integration test plus a bun_shell_parser workspace dep.
Security risks
Two surfaces were examined closely across earlier runs. First, the injected command is a byte string handed to the lifecycle runner (which spawns via sh -c / bun exec): the two variable inputs — self_exe_path() and the registry href — are both routed through bun_shell_parser::escape_8bit, and the user's own preprepare is wrapped in ( … \n) so a trailing comment can't swallow the closing paren. Second, the registry href reaches child argv, foreground echo, and bun pm untrusted output; registry_href_without_userinfo was hardened over three iterations (last-@, authority terminated at /|?|#) and is now unit-tested. The whole path remains gated on trustedDependencies, so nothing new runs for untrusted git deps.
Level of scrutiny
This is a behavioural/design change to bun install, not a mechanical fix. It deliberately diverges from npm's mechanism (npm runs install+prepare in a temp clone and packs the result; this runs a nested install directly in node_modules/<pkg>), and --ignore-scripts on the nested install is a known limitation deferred to a follow-up. Those are reasonable trade-offs, but they're the kind of call a maintainer should ratify.
Other factors
Every inline finding raised on this PR (mine and CodeRabbit's) has been addressed and the threads are resolved; no human has reviewed yet. The integration test covers the chained-preprepare path, the file:// clone path, and asserts devDeps are neither hoisted nor written to the outer lockfile. The file: prefix check in try_https is additive and scoped to the URL-selection branch that feeds git clone.
|
CI build 81716 for 49dcec8: the only failures are unrelated to this change.
The new |
|
Superseded by #38810. That change also removes the nested |
What does this PR do?
When a git dependency has a
preparescript, npm handles it by cloning the package, running a fullnpm install(dependencies and devDependencies), runningprepare, and then packing and installing the result. Bun was runningprepareinnode_modules/<pkg>without making the package's devDependencies available, so build tools declared as devDependencies (tsup,typescript,peggy, ...) were not resolvable when the script ran:Fix
For
git/githubdependencies that declare apreprepare/prepare/postpreparescript, a nestedbun install --ignore-scriptsis injected into theprepreparestep. The nested install runs innode_modules/<pkg>, so the package's owndependenciesanddevDependencies(and their.binentries) are resolvable when the prepare scripts execute.node_modules(matching npm and pnpm).trustedDependencies: if the git dependency is not trusted, none of its lifecycle scripts run and no nested install happens.--registryso private registries configured viabunfig.tomlkeep working for the nested install.Also fixed
git+file://dependencies are now cloned. The protocol was already recognised by the dependency parser, but the clone handler only passedhttp(s)/sshURLs togit clone, sogit+file://went through neither path and resolution failed withno commit matching "..." found (but repository exists). This change was needed to write an offline regression test for the main fix.Fixes #10297
Fixes #6138
Fixes #16548
How did you verify your code works?
New test in
test/cli/install/bun-install-lifecycle-scripts.test.ts:preparescript invokes a binary (what-bin) that is only declared indevDependencies(served from the local verdaccio registry),git+file://with the package trusted,preparescript succeeded and produced its output file,node_modulesorbun.lock.The existing
git dependencies also run preprepare, prepare, and postprepare scriptstest continues to pass unchanged.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts