Conversation
…s in --help The shared --ignore-scripts help entry printed by bun install, add, update, remove, link, unlink, patch, patch-commit, outdated, publish and info still said "(dependency scripts are never run)". Packages in trustedDependencies (and the default trusted list) do run their lifecycle scripts, and --ignore-scripts is what turns those off too: both the flag and install.ignoreScripts clear Do::RUN_SCRIPTS, which gates the root scripts as well as the per-package ones in both linkers. Update the entry, the copies of it in completions/bun-cli.json (generated from --help) and the hand-written ones in completions/bun.zsh, and add a test over every subcommand that prints the shared table.
|
Updated 4:39 AM PT - Aug 15th, 2026
❌ @robobun, your commit 51a877d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 38893That installs a local version of the PR into your bun-38893 --bun |
|
Warning Review limit reached
Next review available in: 1 minute 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 |
|
Status: ready for review. Reproduced on bun 1.4.0: The new help test fails on the released binary (all 11 subcommands) and passes with this branch's debug build. CI: the changed test file passed on every lane in build 97478; that build's only non-retried failure was |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — help-text-only correction. The inline nit about draining stderr is a harness-convention hygiene point, not a real risk (--help writes nothing to stderr), and can be addressed or ignored at the author's discretion.
Extended reasoning...
Overview
This PR updates a single string literal — the --ignore-scripts description in SHARED_TAIL_PARAMS in src/install/PackageManager/CommandLineArguments.rs — and mirrors that same sentence into 11 entries in completions/bun-cli.json and 7 entries in completions/bun.zsh. A test.concurrent.each block over the 11 install-family subcommands asserts the new wording appears in --help output and the old parenthetical does not.
Security risks
None. The change is a static help string and completion descriptions; no code path, parsing, or flag handling is touched. The zsh apostrophe escaping ('"'"') matches the exact pattern already used on neighboring lines for Don't.
Level of scrutiny
Low. This is a documentation/help-text correction bringing --help in line with what docs/pm/lifecycle.mdx and docs/runtime/bunfig.mdx already say and what the code actually does (the flag clears Do::RUN_SCRIPTS, which gates both root and trusted-dependency scripts). I confirmed the 11 subcommands in the test all print param tables that include SHARED_TAIL_PARAMS (via SHARED_PARAMS or the head/tail split for update), and that dedupe/prune/audit — which use separate help-only tables without this entry — are correctly excluded from the test.
Other factors
The one finding from the bug-hunting pass is a nit: the new test opens stderr: "pipe" but only drains stdout. Since print_help writes exclusively to stdout via Output::pretty and then Global::exit(0), stderr stays empty and there is no deadlock risk; await using disposes the pipe. It's a minor deviation from the harness convention, not a blocker. No prior reviewer comments to address.
There was a problem hiding this comment.
LGTM — the stderr-drain nit from the earlier pass is addressed in f74e0ea; the test now reads and asserts on stderr.
What was reviewed:
- The single
SHARED_TAIL_PARAMSstring edit and its 11 mirrored copies inbun-cli.jsonplus 7 inbun.zsh(the'"'"'apostrophe escaping matches the surrounding lines). - Confirmed
dedupe/prune/audithelp use their curated*_HELP_PARAMStables that omit this flag, so the test's 11-subcommand list is the right set. - The new
test.concurrent.eachdrains both pipes, assertsstderris empty, and checks the exit code last.
Extended reasoning...
Overview
This PR corrects a stale help-text string for --ignore-scripts that predates the trustedDependencies feature. It touches one string literal in src/install/PackageManager/CommandLineArguments.rs (the SHARED_TAIL_PARAMS table shared by every install-family subcommand), 11 verbatim copies in the checked-in completions/bun-cli.json, 7 copies in the hand-written completions/bun.zsh, and adds a test.concurrent.each over the 11 subcommands that spawns --help and checks the entry's wording.
Security risks
None. Pure documentation/help-string change with no control-flow or parsing changes; the completions files only affect shell tab-completion descriptions.
Level of scrutiny
Low. The Rust change is a single string literal inside a clap::param! spec; the flag name and separator whitespace are unchanged so parsing is unaffected. The JSON and zsh edits are mechanical find-and-replace of the same sentence. I checked the zsh apostrophe escaping (project'"'"'s) matches the identical pattern already used on neighboring lines (Don'"'"'t, Bun'"'"'s), so the _arguments spec stays well-formed.
Other factors
An earlier automated pass flagged that the new test piped stderr without draining it. Commit f74e0ea addressed that: the test now includes proc.stderr.text() in the Promise.all and asserts expect(stderr).toBe(""), matching the harness convention. I also cross-checked the 11-subcommand list against print_help: dedupe, prune, and audit render curated help tables that omit --ignore-scripts, so their exclusion from the test is correct. The behavior the new wording describes is already covered by two existing tests in the same file, per the PR description.
There was a problem hiding this comment.
LGTM — help-text-only correction for --ignore-scripts, mirrored into the checked-in completion files, with a test.
What was reviewed:
- The single
SHARED_TAIL_PARAMSentry inCommandLineArguments.rs— string literal only, no parse/behavior change. - 11
bun-cli.jsonentries and 7bun.zshentries updated to match; the zsh apostrophe escaping ('"'"') matches the existing pattern in the file. - The new
test.concurrent.eachover 11 subcommands:spawn/baseEnvare already imported at file scope; stderr is now drained and asserted empty (prior nit addressed in f74e0ea). - Remaining instances of the old wording are only in
docs/snippets/cli/*.mdx, which the PR body notes is a separate docs change.
Extended reasoning...
Overview
This PR corrects a stale --help description for --ignore-scripts. The old text ("dependency scripts are never run") predates trustedDependencies and is factually wrong. The change touches one string literal in src/install/PackageManager/CommandLineArguments.rs (the shared SHARED_TAIL_PARAMS table used by all install-family subcommands), and mirrors that sentence into the two checked-in completion files: 11 entries in completions/bun-cli.json and 7 in completions/bun.zsh. A test.concurrent.each test spawns --help for each of the 11 subcommands and asserts the --ignore-scripts line mentions trusted dependencies and no longer contains the old wording.
Security risks
None. This is a documentation string change with no effect on parsing, control flow, or the flag's behavior. The zsh completion edit uses the same '"'"' apostrophe-escaping already present on adjacent lines, so no shell-quoting change is introduced.
Level of scrutiny
Low. The Rust change is a single clap::param! string literal — the flag name, arity, and parsing are unchanged. The completion files are data mirrors of --help output. The test follows harness conventions: test.concurrent.each, await using on the subprocess, three-way Promise.all draining stdout/stderr/exited, stderr asserted empty, exit code asserted last. spawn and baseEnv are already imported at the top of the file.
Other factors
My earlier review flagged that stderr was piped but not drained; that was addressed in f74e0ea and the current diff reflects it (thread resolved). A grep for the old wording confirms the only remaining occurrences are in docs/snippets/cli/*.mdx (explicitly deferred to a separate docs PR per the description) and the negative assertion in the new test itself. The PR description traces the Do::RUN_SCRIPTS bit through both linkers to justify the new wording, and existing behavioral tests in the same file (ignore-scripts is read from npmrc, --ignore-scripts should skip lifecycle scripts) already cover the described behavior.
This branch has not been deployed
1 participant
Problem
bun install --help(andadd,update,remove,link,unlink,patch,patch-commit,outdated,publish,info, which print the same flag table) describes--ignore-scriptsasSkip lifecycle scripts in the project's package.json (dependency scripts are never run).trustedDependencies, or on the default trusted list, do run their lifecycle scripts duringbun install, and--ignore-scriptsis what turns those off as well. It dates from 2022 (2094667), beforetrustedDependenciesexisted ([install] support trustedDependencies #3288).install.ignoreScriptsboth clearDo::RUN_SCRIPTS(src/install/PackageManager/PackageManagerOptions.rs:769and:554), and that one bit gates the root scripts (install_with_manager.rs:994) and the per-package scripts in both linkers (PackageInstaller.rs:2470,isolated_install/Installer.rs:1590,:1885).completions/bun-cli.json(11 copies, generated from--help) and, in a shorter form, incompletions/bun.zsh(7 copies, compiled into the binary forbun completions).Fix
src/install/PackageManager/CommandLineArguments.rsnow readsSkip lifecycle scripts for all packages, including the project's package.json and trusted dependencies, which is whatdocs/pm/lifecycle.mdxand theinstall.ignoreScriptsentry indocs/runtime/bunfig.mdxalready say.completions/bun-cli.jsongets the identical sentence for all 11 entries (checked against whatmisctools/generate-cli-completions.tsextracts from the new--helpline), andcompletions/bun.zshgets it for its 7 entries. No behavior change.docs/snippets/cli/*.mdx) carry the same stale sentence; they are being updated in a separate docs change.test/cli/install/bun-install-lifecycle-scripts.test.tsspawns--helpfor each of the 11 subcommands and checks the--ignore-scriptsentry mentions trusted dependencies and no longer says dependency scripts are never run. All 11 fail on the released binary and pass with this change. The behavior the entry now describes is already covered in the same file:ignore-scripts is read from npmrc(a trusted dependency's script runs without the setting and is skipped with it) and--ignore-scripts should skip lifecycle scripts(the flag, against atrustedDependenciespackage); both still pass.Background
preinstall/install/postinstall/preparecommands in a package.json. bun runs the root project's by default, and a dependency's only if it is trusted.trustedDependenciesarray, or, when that field is absent, the built-in list insrc/install/default-trusted-dependencies.txt. Untrusted dependencies' scripts are blocked and reported at the end of the install.SHARED_TAIL_PARAMSinCommandLineArguments.rsis one flag table concatenated into every install-family subcommand's parameter list, so each of those subcommands'--helpprints this same entry once.completions/bun-cli.jsonis produced by running every subcommand's--helpthroughmisctools/generate-cli-completions.tsand storing each flag's description verbatim; it is checked in, so a help-text change is mirrored there by hand (as install: say --network-concurrency defaults to 64 in help and docs #38759 does for--network-concurrency).completions/bun.zshis hand-written and embedded in the binary viainclude_bytes!insrc/runtime/cli/shell_completions.rs.