Conversation
`bun pm pack` and `bun publish` eagerly parse bun.lock/bun.lockb and
aborted via Global::crash() on any parse/read/migrate failure, even
though the lockfile is only consulted to resolve workspace:^/~/* and
catalog: specifiers. For a package with neither, a truncated,
merge-conflicted, or corrupt lockfile blocked packing entirely.
Treat every lockfile load failure as `lockfile = None` with a warning,
matching `bun install` ("Ignoring lockfile"). The existing
"Failed to resolve workspace version ... Run `bun install`" /
"catalogs require a lockfile" errors in `edit_root_package_json`
already cover the only case that genuinely needs the lockfile.
|
Warning Review limit reached
Next review available in: 17 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 (3)
Comment |
|
Status: diff is green; CI red is unrelated flake. Build #84414 finished with 195/196 jobs passed and 1 canceled (the Annotated failures are all tagged Ready for review/merge. |
|
Updated 7:42 PM PT - Jul 28th, 2026
❌ @robobun, your commit 4bf8f53 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36260That installs a local version of the PR into your bun-36260 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/cli/publish_command.rs:480-502— This ~20-lineLoadResult::Err→ warn-and-continue block is byte-identical to the one inpack_command.rs:230-252(onlypm_log(manager_ptr).reset()vslog.reset()differs). Per REVIEW.md — "the second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site" — consider extracting a helper (e.g. on theLoadResulterr type, or alongsidereport_lockfile_load_error) so future wording/--silentchanges stay atomic acrosspackandpublish.Extended reasoning...
What
The
LoadResult::Err(cause)arm inpublish_command.rs:480-502andpack_command.rs:230-252are near-identical copies of one another. Both were rewritten in this PR to implement the warn-and-continue behavior, and both appear in the diff.Side-by-side
pack_command.rs:230-252:LoadResult::Err(cause) => 'err: { if matches!(cause.step, LoadStep::OpenFile) && cause.value == bun_install::Error::Sys(bun_errno::SystemErrno::ENOENT) { break 'err None; } // A lockfile is only needed to resolve `workspace:^` / ... let step = match cause.step { LoadStep::OpenFile => "open", LoadStep::ParseFile => "parse", LoadStep::ReadFile => "read", LoadStep::Migrating => "migrate", }; Output::warn(format_args!( "failed to {} {}: {}, continuing without it", step, bstr::BStr::new(cause.lockfile_path.as_bytes()), cause.value.name(), )); pm_log(manager_ptr).reset(); None }
publish_command.rs:480-502is the same except the second-to-last line islog.reset();. That difference is trivially unified by passing&mut bun_ast::Logto a helper.Why this matters
REVIEW.md's code-style section is explicit:
The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site.
This is exactly that case — two parallel sites in the same PR with a copy-pasted ~20-line block (comment included). The pre-PR code was also duplicated across these two files, so this isn't a regression the PR introduced; but the rule targets what appears in the diff, and this PR is the natural moment to consolidate since both blocks were rewritten anyway.
Concretely, any follow-up change — e.g. gating the warning on
!silent/log-level, changing the wording, or adding anotherLoadStepvariant — must now be made in two places, and it's easy to miss one (the pre-PR blocks had already diverged informat_args!vs tuple-arg style).Suggested shape
Something like:
pub(crate) fn warn_lockfile_unreadable(cause: &LoadResultErr, log: &mut bun_ast::Log) { let step = match cause.step { LoadStep::OpenFile => "open", LoadStep::ParseFile => "parse", LoadStep::ReadFile => "read", LoadStep::Migrating => "migrate", }; Output::warn(format_args!( "failed to {} {}: {}, continuing without it", step, bstr::BStr::new(cause.lockfile_path.as_bytes()), cause.value.name(), )); log.reset(); }
placed alongside the existing lockfile-load-error reporting in
bun_install(or as a method on the err type), with both call sites collapsing to the ENOENT guard +warn_lockfile_unreadable(&cause, log); None. TheLoadStep → verbmatch in particular is a naturalimpl LoadStep { fn verb(&self) -> &'static str }.Severity
Nit — style/maintainability only. The PR is functionally correct as-is; extraction is a small cleanup, not blocking.
…--silent Shared between pack and publish. Mirrors report_lockfile_load_error: the pinpoint parse diagnostics from the manager log are printed before reset() so the user still sees where the lockfile is broken, and the whole block is gated on LogLevel::Silent.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/cli/pack_command.rs:229-249— This ~20-lineLoadResult::Errwarn-and-continue block is byte-for-byte duplicated atpublish_command.rs:479-498(onlypm_log(manager_ptr).reset()vslog.reset()differs — and those resolve to the same log). REVIEW.md → Code style: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site." Since this PR is rewriting both copies anyway, consider extracting a shared helper (or extendingreport_lockfile_load_errorininstall_with_manager.rs, which the PR description already says this mirrors) rather than maintaining two copies. Orthogonal to the comment above about dropped diagnostics /--silent— even after fixing those in both places, the duplication remains.Extended reasoning...
What the finding is
The
LoadResult::Err(cause) => 'err: { ... }arm added inpack_command.rs:229-249is near-identical to the one added inpublish_command.rs:479-498. Diffing the two blocks side by side:pack_command.rspublish_command.rsif matches!(cause.step, LoadStep::OpenFile) && cause.value == ... { break 'err None; }identical let step = match cause.step { OpenFile => "open", ParseFile => "parse", ReadFile => "read", Migrating => "migrate" };identical Output::warn(format_args!("failed to {} {}: {}, continuing without it", step, bstr::BStr::new(cause.lockfile_path.as_bytes()), cause.value.name()));identical pm_log(manager_ptr).reset();log.reset();Noneidentical The single differing line resolves to the same object —
pm_log(manager_ptr)isunsafe { &mut *(*manager_ptr).log }andloginpublish_command.rsismanager.log_mut(), both the manager's log.Why this is flagged
REVIEW.md's Code style & idioms reviewers enforce section states:
Simplest honest shape; deduplicate within your own diff. … The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site. If your fix makes two functions byte-identical, delete one.
Both blocks are new in this diff — the PR rewrites both call sites — so the rule applies directly. It's true that the pre-PR code was also duplicated between these two files, but since the PR is actively touching both copies anyway, extracting a shared helper is in-scope for this change, not "file-wide standardization riding a focused bugfix."
Not a duplicate of the existing review comment
There is already a claude[bot] comment on
pack_command.rs:251about this block, but it addresses a behavioral divergence: the log is.reset()without printing first (discarding pinpoint parse diagnostics), andOutput::warnisn't gated on--silent. That comment ends with "Same applies to the identical block inpublish_command.rs" — acknowledging the duplication in passing to say the same fix is needed there, but not asking for the blocks to be unified. Even after applying that comment's suggestions to both sites separately, you'd still have two copies of the (now larger) block. This finding is the deduplication ask, orthogonal to that one.Step-by-step: why a helper fits naturally here
- The PR description says the new behavior "matches
bun install's behaviour" and citesreport_lockfile_load_errorinsrc/install/PackageManager/install_with_manager.rs:1368-1407as the reference. - That function already has almost the right signature:
fn report_lockfile_load_error(manager: &mut PackageManager, cause: &lockfile::LoadResultErr, log_level: Options::LogLevel). It gates onSilent, prints the manager log, resets it, and emits the warning — exactly the behavior the other review comment asks pack/publish to adopt. - Both new call sites have a
&mut PackageManager(viamanager_ptr), aLoadResultErr(cause), and access tomanager.options.log_level— everything the existing helper needs. - The one piece the existing helper doesn't do is the ENOENT-on-OpenFile →
Noneearly-out, but that's a two-line guard the caller can keep, or a smallfn warn_and_ignore_lockfile_error(manager, cause) -> Option<&Lockfile>inbun_install::lockfile(or onLoadResult) could wrap both.
Concretely, both sites could collapse to something like:
LoadResult::Err(cause) => { bun_install::lockfile::warn_unreadable(manager, &cause); // prints log, warns, resets — gated on Silent None }
with the ENOENT short-circuit either inside the helper or kept as the two-line guard before it.
Impact and severity
This is a maintainability/code-quality concern — nothing breaks if merged as-is. But the two copies will drift (they already differ trivially in how they name the log), and any follow-up fix — including the one the existing comment asks for — has to be applied twice. Nit severity per the rubric: merging without this doesn't cause incorrect behavior, a crash, or a regression; it's a repository-enforced style rule that makes the follow-up work cleaner.
- The PR description says the new behavior "matches
|
Both duplication findings above reviewed an older commit range; addressed in b9ed762. |
There was a problem hiding this comment.
LGTM — the earlier feedback (print parse diagnostics before log.reset(), gate on --silent, dedupe into a shared helper) is fully addressed in warn_lockfile_unreadable.
What was reviewed:
warn_lockfile_unreadablemirrorsreport_lockfile_load_error: prints the log, flushes, then unconditionally resets;LoadStepmatch is exhaustive.- ENOENT-on-open still short-circuits to
Nonein both callers; the workspace/catalog path still hard-fails viaedit_root_package_json(covered by the new negative test). --silenttest asserts stderr is empty; diagnostic assertions confirm the log is printed (not just the summary warn line).
Extended reasoning...
Overview
Changes bun pm pack and bun publish to warn-and-continue when bun.lock/bun.lockb fails to load, instead of Global::crash(). A new pub(crate) fn warn_lockfile_unreadable in pack_command.rs is called from both commands' LoadResult::Err arms; it emits the warning, prints any queued parse diagnostics from the manager log, flushes, and resets the log — all gated on log_level != Silent. The ENOENT-on-OpenFile case still maps to None silently as before. Six new tests in bun-pack.test.ts cover empty/merge-conflict/corrupt-binary lockfiles, --silent, the workspace-still-errors negative case, and bun publish --dry-run.
Security risks
None. This is CLI diagnostic-output handling on a local file the user controls; no parsing of untrusted data changed, no new I/O or network paths.
Level of scrutiny
Low-to-medium. The code change is small (~35 net lines of Rust, replacing ~60), follows the existing report_lockfile_load_error pattern from install_with_manager.rs, and the LoadStep match is exhaustive against the enum's four variants. The behavior change (crash → warn+continue) is a strict relaxation that matches how bun install already treats the same failure and how npm pack behaves; the safety net — hard-failing when the lockfile is actually needed for workspace:/catalog: resolution — is preserved and explicitly tested.
Other factors
- My prior review on this PR flagged two issues (diagnostics discarded before print;
--silentnot honored). Both were addressed in b9ed762: the helper now prints the log beforereset(), gates onSilent, and the block was deduplicated across pack/publish. The comment-cop bot's note about long comments was also addressed. All inline threads are resolved. - Test evidence in the PR body shows all 6 new tests fail on main and pass on the PR under both debug-ASAN and release; the full 82-test file passes.
log.reset()runs unconditionally (outside theSilentgate), so stale parse errors can't leak into a laterlog.print()even under--silent.- The
bun publish --dry-runtest intentionally omits an exit-code assertion (auth check runs before the dry-run early-return), scoping the assertion to "packing completed" — which is the behavior under test. - No CODEOWNERS covers these paths.
There was a problem hiding this comment.
LGTM — the concerns from my earlier pass are all addressed in b9ed762.
What was reviewed:
warn_lockfile_unreadablenow prints the manager log's parse diagnostics beforereset()and gates onLogLevel::Silent; both are locked in by the new tests (diagnosticsubstring assertions and the--silent→err === ""test).- Verified the
workspace:*+ corrupt-lockfile path still errors via the existingedit_root_package_jsonguard, and thatLoadStep::OpenFile+ENOENTstill short-circuits toNonein both callers. - Helper is shared between pack and publish; no duplicated logic remains.
Extended reasoning...
Overview
Changes bun pm pack and bun publish to warn-and-continue (instead of Global::crash()) when bun.lock/bun.lockb exists but is unreadable/unparseable. A new pub(crate) fn warn_lockfile_unreadable in pack_command.rs handles the diagnostic (prints the parser log, emits a warn: line, resets the log, honors --silent) and is called from both pack_command.rs::exec_with_manager and publish_command.rs::from_workspace. Six new tests in bun-pack.test.ts cover empty/merge-conflict/corrupt-binary lockfiles, --silent, the workspace: still-errors path, and bun publish --dry-run.
Security risks
None. The lockfile is only consulted for workspace:/catalog: version resolution during pack; falling back to None is already a supported state (LoadResult::NotFound), and edit_root_package_json still hard-errors when a workspace version actually needs it. No new inputs are trusted.
Level of scrutiny
Moderate. This is a user-facing behavior change (crash → warn+continue) but a strict relaxation on an edge-case path, and it aligns with both npm pack (never reads a lockfile) and bun install's existing report_lockfile_load_error warn-and-ignore behavior. The core logic change is ~15 lines with the rest being deduplication of two identical error-handling blocks into one helper.
Other factors
My earlier review flagged two divergences from report_lockfile_load_error (diagnostics discarded before printing; --silent not honored). Both were fixed in b9ed762 and are now enforced by tests — the parametrized cases assert the pinpoint parser diagnostic (Missing lockfile version, Expected string but found "<<<<<<<") appears in stderr, and a dedicated --silent test asserts empty stderr. The negative-contract test (still errors when a workspace version must be resolved) confirms packages that actually need the lockfile still fail with a clear message. Evidence in the PR description shows all 82 tests in bun-pack.test.ts pass on both debug+ASAN and release, and the new tests fail on main. All prior review threads are resolved.
|
Related: #38813 removes the lockfile read from pack and publish entirely (workspace versions and catalogs come from the package.json files instead), which makes the unreadable-lockfile case this PR handles a non-issue if it lands. |
|
Closing in favor of #38813, which removes the lockfile load this PR makes tolerant: pack and publish no longer read Checked against a build of #38813 with the scenarios from this PR (empty |
2 participants
What
bun pm packandbun publisheagerly parsebun.lock/bun.lockbbefore packing. AnyLoadStep::{ParseFile,ReadFile,Migrating}failure previously calledGlobal::crash(): exit 1, no tarball, even for a package with noworkspace:/catalog:specifiers (the only thing the lockfile is consulted for).npm packnever reads a lockfile.Realistic triggers: an empty
bun.lockfrom a truncated write, a mid-merge lockfile with git conflict markers, or a corrupt/foreignbun.lockb.Repro
Same for
bun.lockwith<<<<<<< HEADmarkers (ParserError) and corruptbun.lockb(InvalidLockfile).Fix
In both
pack_command.rsandpublish_command.rs, on anyLoadResult::Err(other thanENOENT, which already mapped toNone): emitwarn: failed to <step> <path>: <err>, continuing without it, reset the manager log (so the stale parse diagnostics don't leak into laterlog.print()calls), and proceed withlockfile = None. This matchesbun install's behaviour, which prints the parse error and thenwarn: Ignoring lockfilebefore re-resolving.edit_root_package_jsonalready errors withFailed to resolve workspace version for "<name>" ... Run \bun install`/catalogs require a lockfilewhen resolution actually needs the lockfile and it'sNone`, so a package that does depend on it still fails with a clear message.The lazy-load optimization (skip
load_from_cwdentirely when the manifest has noworkspace:/catalog:spec) would require moving the lockfile load after the package.json parse insidepack(); left as a follow-up since graceful handling alone fixes the user-facing bug.Verification
bun-pack.test.tsgained anunreadable lockfileblock:bun.lock,bun.lockwith git conflict markers, corruptbun.lockb→ pack succeeds, tarball contents correct, stderr haswarn:+continuing without it, noerror:bun.lock+"pkg1": "workspace:*"→ still exits 1 withFailed to resolve workspace version for "pkg1" ... Run \bun install``bun publish --dry-runwith emptybun.lock→ packing completes (Total files: 2)All 5 new tests fail on main (
failed to parse lockfile→ exit 1), all pass with this change. All 81 tests inbun-pack.test.tspass.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file