cli: load a package.json and name a lockfile load step from one place - #39165
cli: load a package.json and name a lockfile load step from one place#39165robobun wants to merge 3 commits into
Conversation
pack() spelled out the same match on GetResult twice (the initial read and the re-read after lifecycle scripts), update --interactive spelled out its workspace package.json match twice, and update --interactive and outdated each wrote out the LoadStep -> verb table that LoadStep::verb() already holds in bun_install. GetResult::entry() now returns the entry or the failed step plus its error, with GetStep::verb() naming the step; pack and update --interactive each load through one local helper that prints the same message as before. LoadStep::verb() is pub so the CLI crate can use it; pm's private copy of that table goes away. The messages, log printing and exit codes at every site are unchanged. Removes the same_match_twice entries for pack_command.rs and update_interactive_command.rs from mordant-baseline.toml.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 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 (10)
Comment |
|
Updated 4:06 PM PT - Aug 15th, 2026
🔄 @alii, the build for your commit |
|
@coderabbitai review |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical dedup that folds three repeated match blocks into shared helpers with no behavior change.
Checked that Output::err with the new 2-tuple template ("failed to {} package.json: {}") renders identically to the old per-arm format_args! form via FmtTuple, and that the parse-only log print in pack_command.rs is still gated by step == GetStep::Parse before Global::crash(). Verified LoadStep::verb() returns the same four strings as the removed load_step_verb() and the two inlined LoadStep matches. Confirmed GetStep is reachable at WorkspacePackageJSONCache::GetStep through the existing module alias, and the dropped GetJsonResult import has no remaining uses in update_interactive_command.rs.
Extended reasoning...
Overview
Refactor to clear three same_match_twice mordant findings in the CLI crate. Adds GetStep + GetResult::entry() to WorkspacePackageJSONCache.rs so the read/parse verb mapping lives once next to the enum, and widens LoadStep::verb() from pub(crate) to pub. Four CLI files then call those helpers in place of copy-pasted match arms: pack_command.rs gets a local package_json_entry() used at both the initial read and the post-lifecycle re-read; update_interactive_command.rs gets a local load_package_json() used by both the dependency-update and catalog-update loops; outdated_command.rs and update_interactive_command.rs collapse the four-arm LoadStep message table into one cause.step.verb() call; package_manager_command.rs drops its private load_step_verb() copy. Two lines removed from mordant-baseline.toml.
Security risks
None. Pure error-message assembly refactor in CLI paths; no new inputs, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. This is a lint-baseline cleanup of duplicated control flow with no user-visible behavior change. The only load-bearing question is whether each collapsed site produces byte-identical output and takes the same exit path; I traced each one and it does — pack_command.rs still prints the log only on parse failure and still calls Global::crash(); update_interactive_command.rs still continues on failure via let Some(...) else; the lockfile sites still call Output::err_generic with the same template.
Other factors
The unbound 'a on package_json_entry() mirrors the existing pm_workspace_cache/pm_log helpers in the same file (raw-pointer field projection over the PackageManager singleton), so no new lifetime pattern is introduced. GetResult::unwrap() is now implemented on top of entry(), so the variant mapping exists once. The PR description reports the relevant test files pass on the debug build and rust:mordant is clean with the baseline lines removed.
…e load sites bun pm pack on an unparsable package.json, and on a prepack script that removes or breaks it before the re-read; bun outdated, bun update --interactive and bun pm ls on a bun.lock that does not parse; and bun update -i -r on a workspace member whose package.json does not parse, which is reported and skipped while the other members are still written.
There was a problem hiding this comment.
LGTM — mechanical dedup of repeated match arms into shared helpers, no user-visible change.
What was reviewed
GetStep::verb()/LoadStep::verb()return the same strings the deleted arms hard-coded; each folded call site's message template, log-print ordering (pack prints the log only onParse, beforecrash()), and exit/continue behavior are byte-identical to before.Output::err's tuple form is equivalent to the oldformat_args!form (both areFmtTuple;{}vs{s}are treated the same bysubstitute_template).- New tests pin the exact stderr for the pack read/parse paths, the update-interactive per-workspace parse-fail path, and the three lockfile-load consumers.
Extended reasoning...
Overview
The PR eliminates three same_match_twice mordant findings by folding duplicated match blocks into single-owner helpers. In WorkspacePackageJSONCache.rs it adds GetStep (Read/Parse) with verb() and a GetResult::entry() that returns Result<&mut MapEntry, (GetStep, Error)>; unwrap() is reimplemented on top of it. In lockfile.rs, LoadStep::verb() goes from pub(crate) to pub. pack_command.rs extracts package_json_entry() (used at both the initial read and the post-lifecycle re-read); update_interactive_command.rs extracts load_package_json() (used in both the dependency and catalog loops) and collapses its LoadStep match; outdated_command.rs and package_manager_command.rs swap their local LoadStep tables for step.verb(). Two mordant-baseline.toml entries are removed, and three test files add snapshot/regex tests pinning the messages.
Security risks
None. The change is confined to error-message formatting on CLI paths that already crashed/exited/continued the same way. No parsing, validation, or trust boundary is touched; the raw-pointer field-projection pattern in package_json_entry is the same one already used by pm_workspace_cache/pm_log in the surrounding code.
Level of scrutiny
Low. This is exactly the deduplication the repo's own lint asks for, done at the layer that owns the enum. I traced each folded site against its original arms: the verb strings match ("read"/"parse" for GetStep, "open"/"read"/"parse"/"migrate" for LoadStep), the format templates are unchanged apart from the verb becoming a positional, and the parse-only log.print in pack is preserved via if step == GetStep::Parse. Output::err accepts impl FmtTuple, and the new 2-tuple form ((&'static str, &BStr)) is a valid FmtTuple just like the old format_args! was.
Other factors
The PR ships tests that pin every touched message (the three pack failure modes including the post-prepack re-read, the update-interactive per-workspace parse failure with the sibling still updated, and the lockfile parse failure across outdated/update -i/pm ls). The description documents a diff of stderr/stdout/exit against release bun for each path and confirms mordant/clippy are clean with the baseline entries removed. GetResult::unwrap() is unchanged for existing callers. The unbounded 'a on package_json_entry mirrors the existing pm_workspace_cache/pm_log helpers it wraps, so no new lifetime hazard is introduced.
|
CI status for this PR:
Retrying the two darwin jobs on build 98776 once the queue has drained is enough to turn the build green for auto-merge; I am not re-pushing, since a new build would join the same queue. |
There was a problem hiding this comment.
LGTM — mechanical dedup of three repeated match blocks into shared helpers, with no user-visible change.
What was reviewed:
package_json_entry()in pack_command.rs preserves the read/parse message split and prints the log only on parse, same as both original arms.load_package_json()in update_interactive_command.rs keeps the "Failed to package.json at : " wording and returnsNoneso both loopscontinueas before.- The three
LoadStepmatch tables collapse tocause.step.verb(), whose arms are byte-identical to the removed ones;package_manager_command.rs's privateload_step_verbwas already the same table. - New tests pin each folded message via subprocess snapshots and follow harness conventions (tempDir, concurrent, pipes drained together).
Extended reasoning...
Overview
Folds three duplicated match expressions flagged by the same_match_twice lint into single helpers: GetResult::entry() + GetStep::verb() in WorkspacePackageJSONCache.rs, package_json_entry() in pack_command.rs, load_package_json() in update_interactive_command.rs, and switches three CLI files to the now-pub LoadStep::verb(). Removes the corresponding two mordant-baseline.toml entries and adds tests covering each folded error message.
Security risks
None. The change touches error-message formatting on CLI failure paths for package.json/lockfile load errors — no parsing, no network, no auth, no untrusted-input handling changed.
Level of scrutiny
Low-to-medium. This is a pure refactor of error-reporting arms with the invariant that output is byte-identical, which the PR description verifies by diffing against the released binary and which the new snapshot tests pin. I traced each folded arm against its original: verbs match ("read"/"parse"/"open"/"migrate"), format strings match, log-print gating on parse-only in pack is preserved, and the continue-via-None in update -i matches both original loops. The unbound-lifetime package_json_entry<'a> follows the same raw-pointer projection pattern as the existing pm_workspace_cache/pm_log helpers in the same file.
Other factors
CI passed on all platforms that ran (177/179, two darwin jobs canceled before starting). The added tests use tempDir/describe.concurrent, drain stdout/stderr/exited together, and assert exact messages. The pattern mirrors #38841 and #39154 which introduced the same helper style in bun_install.
2 participants
Problem
same_match_twicereports three findings in the CLI crate, recorded inmordant-baseline.tomlaspack_command.rs = 1andupdate_interactive_command.rs = 2:pack_command.rs:2157repeats:1905: thematchonGetJsonResultthat loads the package.json being packed is written out once for the initial read and again for the re-read after lifecycle scripts.update_interactive_command.rs:470repeats:343: thematchonGetJsonResultthat loads a workspace's package.json is written out once in the dependency-update loop and again in the catalog-update loop.update_interactive_command.rs:530repeatsoutdated_command.rs:121: amatchonLoadStepspelling out the "failed to open/read/parse/migrate lockfile" table, which Tidy: fold duplicated matches into one method, drop dead state (no behavior change) #38841 already turned intoLoadStep::verb()insidebun_install(it waspub(crate), so the CLI crate could not use it).exit_for_root_package_jsonfrom Tidy: fold duplicated matches into one method, drop dead state (no behavior change) #38841 does not fit these sites: pack crashes withfailed to <verb> package.json: <path>and prints the log only on a parse failure, update --interactive reportsFailed to <verb> package.json at <path>: <name>and skips the workspace, while that helper printsfailed to <verb> '<path>'and exits 1. What all of them share is theReadErr-> "read" /ParseErr-> "parse" mapping, so that is the part this PR puts next to the enum.Fix
WorkspacePackageJSONCache.rs:GetResult::entry()returns the entry or(GetStep, Error), whereGetStep::verb()is "read" or "parse". The existingunwrap()is implemented on top of it, so the variant mapping exists once.pack_command.rs: onepackage_json_entry()does the lookup and the crash; both sites call it. The log is still printed only on a parse failure, beforeGlobal::crash(), as before.update_interactive_command.rs: oneload_package_json()does the lookup and reports a failure, returningNoneso both loopscontinueas before.lockfile.rs:LoadStep::verb()becomespub.update_interactive_command.rsandoutdated_command.rsprintfailed to {verb} lockfile: {name}through it, andpackage_manager_command.rsdrops its private copy of the same table.bun pm packon an unparsable package.json, on aprepackscript that deletes package.json (read error on the re-read), and on one that writes invalid JSON (parse error on the re-read);bun outdatedandbun update --interactiveon an unparsablebun.lock. All identical.bun-pack.test.ts("package.json that cannot be loaded": the threebun pm packcases above),bun-lock.test.ts("a bun.lock that does not parse":bun outdated,bun update --interactiveandbun pm ls, as inline snapshots), andbun-update-transitive.test.ts(bun update -i -ron a workspace whose second member's package.json does not parse: that member is reported and skipped, the first member is still updated). They pass on this build and, being a refactor, also on the released bun, exceptbun pm ls, whose wording changed on main in install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 after the released build.mordant-baseline.toml: thepack_command.rsandupdate_interactive_command.rssame_match_twicelines are removed. With those lines removed,bun run rust:mordanton the unmodified sources reports exactly the three findings above (bun_runtime 3inover-baseline.txt); with this change it reports nothing. Regenerating the baseline withbun run rust:mordant:baselineadds no new entries.install_with_manager.rsentry for the same lint is the subject of install: read the root package.json through one helper in install_with_manager #39154; its helper could useGetResult::entry()once either lands, but this PR leaves that file alone.test/cli/install/bun-pack.test.ts(79 pass),test/cli/update_interactive_install.test.ts,test/cli/update_interactive_formatting.test.ts,test/cli/update_interactive_snapshots.test.ts,test/regression/issue/update-interactive-formatting.test.ts,test/cli/install/bun-pm.test.ts(53 pass together),test/cli/install/migration/pnpm-lock-v9.test.ts(81 pass), thebun-install.test.tsinvalid package.json snapshot, andbun-publish.test.ts(the publish flow goes through the samepack()helper; the two "should run in order" cases time out here at 5s because each lifecycle script starts a debug build, which takes ~0.8s, and the timeout kills the shared registry for the tests after them; the other 17 pass when run without those two).cargo clippy -p bun_install -p bun_runtimeis clean.Background
WorkspacePackageJSONCache::get_with_pathreads and parses a package.json once per process and hands back a cachedMapEntry(AST, source, indentation). It returnsGetResult, a three-way enum (Entry,ReadErr,ParseErr); parse diagnostics go into the package manager'sLog, which is why the parse paths print the log next to the one-line error.LoadStepis the lockfile loader's counterpart:LoadResult::Err(cause)carriescause.step(open / read / parse / migrate) andcause.value;verb()is the word the error message uses for the step.same_match_twicecompares wholematchexpressions on one enum across a crate (scrutinee, patterns and arm bodies), so a repeated copy only goes away when one function owns thematchand the other sites call it; two copies that both forward to a helper still count.mordant-baseline.tomlholds the per-(lint, file) counts that predate the mordant CI job; the job fails only on counts above the baseline, so a line can be deleted once the file is clean for that lint.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-lock.test.ts test/cli/install/bun-pack.test.ts