Conversation
bun install --production has always enabled --frozen-lockfile, but neither --help nor the docs said so. Users who only expected devDeps to be skipped would hit 'lockfile had changes, but lockfile is frozen' with a note telling them to remove a flag they never passed. - --help for --production now says 'Implies --frozen-lockfile' - the frozen-lockfile error note now mentions --production alongside --frozen-lockfile - docs/pm/cli/install.mdx documents the implied flag and points to --omit=dev for the devDeps-only case; also drops the incorrect 'or optionalDependencies' (--production does not skip optionals) - docs/runtime/bunfig.mdx and docs/snippets/cli/install.mdx updated to match Fixes #10949
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. 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 (13)
Comment |
|
Updated 1:21 AM PT - Jul 25th, 2026
@autofix-ci[bot], your commit 946b306 is building: |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — docs + two string-literal-only source changes, no behavior change. The three inline notes are all optional-nit follow-ups (sibling doc snippets, second error site, test pipe hygiene); none block.
Extended reasoning...
Overview
This PR documents that bun install --production implies --frozen-lockfile (existing behavior since v0.6.10 — confirmed at PackageManagerOptions.rs:817-821). It touches:
CommandLineArguments.rs: appends. Implies --frozen-lockfileto the--productionhelp-text string literal inSHARED_PARAMS.install_with_manager.rs: addsor --productionto the existingbun_core::note!string on the frozen-lockfile error path.- Three
.mdxdocs files (docs/pm/cli/install.mdx,docs/runtime/bunfig.mdx,docs/snippets/cli/install.mdx): prose updates, plus a factual correction dropping the incorrect "or optionalDependencies" claim. bun-install.test.ts: a new test (near-verbatim of the existing--frozen-lockfiletest at line 6629) asserting both the help text and the error note mention--production.
Security risks
None. Both source changes are string-literal-only inside clap::param! and bun_core::note! macros — no control flow, validation, or lockfile logic is touched. Docs and test additions carry no security surface.
Level of scrutiny
Low. This is effectively a docs PR: the two Rust hunks change only user-visible message text, the behavior being documented already exists and is not modified, and the new test follows the exact shape of the sibling test right above it. No CODEOWNERS-covered paths are touched.
Other factors
The bug-hunting pass surfaced three items, all correctly classified as nits:
- Sibling
docs/snippets/cli/{add,update,...}.mdxstill carry the old--productiondescription — pre-existing textual divergence frominstall.mdx; worth a follow-up sweep but not blocking a fix targeted at #10949. - The second
lockfile had changes, but lockfile is frozenemitter atinstall_with_manager.rs:1522has nonote!— pre-existing (that site never had one) and a rare edge case. - The new test pipes
stdoutwithout draining — matches the existing sibling test's shape and cannot deadlock on the frozen-lockfile crash path (writes ~nothing to stdout).
I also verified the PR's "production mode does not skip optionals" doc correction against PackageManagerOptions.rs — cli.production only clears dev_dependencies and sets FAIL_EARLY/FROZEN_LOCKFILE, so the doc fix is accurate. Given the scope is string literals + docs + a copy-of-neighbor test, and all findings are non-blocking nits, this is safe to approve.
|
Addressed review:
CI status (build 80373 @ 946b306): the only failure is |
There was a problem hiding this comment.
LGTM — all three nits from the earlier pass are addressed (sibling snippets updated, both frozen-lockfile error sites now share crash_frozen_lockfile(), test spawn uses stdout/stdin: "ignore"). Completions files are intentionally left for the auto-gen PR, which is fine.
Extended reasoning...
Overview
Documents that bun install --production implies --frozen-lockfile. Touches the --production help-text literal in SHARED_PARAMS (CommandLineArguments.rs), the frozen-lockfile error note in install_with_manager.rs (refactored into a shared crash_frozen_lockfile() helper called from both emitters), nine .mdx docs/snippet files, and adds one test to bun-install.test.ts.
Follow-up on prior review
I left three nits on the first revision; all were addressed in 1f0741e / 99cf669:
- All seven sibling CLI snippet files (add/update/remove/link/patch/outdated/publish) now carry "Implies --frozen-lockfile" — verified in the diff. Completions files were intentionally skipped per the author's note about a pending auto-gen PR.
- The second
lockfile had changes, but lockfile is frozensite increate_new_lockfile_and_enqueuenow goes through the samecrash_frozen_lockfile(log_level)helper. Grep confirms exactly one remaining emitter of that string (the helper body). The helper is#[cold] #[inline(never)] -> !, matching the sibling cold helpers in the same file, andGlobal::crash()exits 1 to match the test'sexpect(await exited).toBe(1). - The third spawn in the new test now sets
stdout: "ignore", stdin: "ignore".
Security risks
None. Changes are help-text strings, error-message wording, docs, and a behavior-preserving refactor of two identical crash blocks into one helper. No auth/crypto/permissions/parsing of untrusted input touched.
Level of scrutiny
Low. The only runtime-visible changes are (a) help text wording, (b) the error note now mentions --production, and (c) the second (rare, needs_new_lockfile) frozen-lockfile path now also prints a note: line and uses the same <d>:<r> prefix as the first — a cosmetic consistency improvement. No control-flow changes; both call sites still Global::crash() under the same guard.
Other factors
- No CODEOWNERS coverage on the touched paths.
- New test asserts both the help output and the full error+note text under
--productionwith a stale lockfile; it mirrors the existing--frozen-lockfiletest at ~line 6629. - The docs also fix a pre-existing inaccuracy (production mode does not skip
optionalDependencies) and mention the fail-early behavior — verified againstPackageManagerOptions.rssemantics referenced in the PR description. - The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
LGTM — all three earlier nits were addressed (sibling snippets updated, both error sites now share crash_frozen_lockfile(), test pipe hygiene fixed).
Checked: the fail-early claim in the docs matches PackageManagerOptions.rs:819; only one "lockfile had changes" emitter remains after the helper extraction; existing toContain assertions on the error string in bun-install.test.ts and lockfile-version-2.test.ts still match the new output.
Extended reasoning...
Overview
Documentation + error-message PR clarifying that --production implies --frozen-lockfile. Touches 10 .mdx doc files (help-text mirrors), one help-string literal in CommandLineArguments.rs, extracts a shared crash_frozen_lockfile() helper in install_with_manager.rs so the two frozen-lockfile error sites can't drift, and adds one test to bun-install.test.ts. No install logic changes — only user-facing text.
Security risks
None. No auth, network, filesystem, or parsing changes; the Rust diff only rewords a diagnostic and hoists two identical error blocks into a #[cold] -> ! helper.
Level of scrutiny
Low. Docs + a help-text string + an error-note reword. The crash_frozen_lockfile extraction is a pure refactor of two near-identical blocks (the second previously lacked a note! and used slightly different color markup; unifying them is an improvement). Return type -> ! matches Global::crash()'s divergence, and the #[cold] #[inline(never)] attributes match the surrounding cold-path convention in the same file.
Other factors
- I previously left three nits on this PR; all were addressed in commits 1f0741e and 99cf669, and all three threads are marked resolved.
- Verified the new docs claim that production mode "aborts immediately on the first install error" against
PackageManagerOptions.rs:509/819(Enable::FAIL_EARLYis set alongsideFROZEN_LOCKFILE). - Grepped
src/for the error string — now a single emitter, confirming both call sites route through the helper. - Grepped
test/for existing assertions on the error/note text — all usetoContain("error: lockfile had changes...")which still matches; no test asserts the old note wording, so nothing breaks. - The new test spawns three subprocesses with correct pipe drainage, asserts stderr before exit code, and uses the local
withContext/dummyRegistryForContextharness pattern from neighboring tests.
|
Closing. The documentation part of this PR is on main now.
#10949 is closed as completed. Two small pieces are not on main:
Those fit better in a fresh, smaller PR against #5921 than in a rebase of this branch. #40110 and #41931 are open and rewrite the same error block in |
What
bun install --productionhas always enabled--frozen-lockfileinternally (seePackageManagerOptions.rs), but neither--helpnor the docs said so. Users who only expecteddevDependenciesto be skipped would hit:with a note telling them to remove a flag they never passed.
Changes
--productionhelp text now saysDon't install devDependencies. Implies --frozen-lockfile--productionalongside--frozen-lockfiledocs/pm/cli/install.mdxdocuments the implied flag and points to--omit=devas the alternative for skipping devDeps without freezing the lockfile. Also drops the incorrector optionalDependencies(production mode does not skip optionals)docs/runtime/bunfig.mdxanddocs/snippets/cli/install.mdxupdated to matchWhy document rather than change behavior
The behavior has been in place since
--frozen-lockfilewas added in v0.6.10 and is relied on by CI pipelines that expect production installs to be reproducible. Changing it would be a silent behavior change; documenting it plus providing a clear alternative (--omit=dev) is the lower-risk fix.Verification
New test in
test/cli/install/bun-install.test.tsasserts both the help text and that the error note mentions--productionwhen a stale lockfile is hit via--production.The
docs/pm/cli/install.mdxhunk overlaps with one line of #33709 (which is a broader docs-only sweep); this PR additionally fixes help text, the error note, bunfig docs, and the flag snippet.Fixes #10949
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/bun-install.test.ts