Deduplicate package manager and CLI command helpers - #32022
Conversation
|
@robobun adopt |
|
Audit done: the diff is behavior-preserving (edge-path notes in the PR body) and hermetic tests pin the consolidated error exits. Rebased directly onto main and retargeted; nothing here has landed on main separately. All 15 CodeRabbit threads are resolved (two comment fixes and six test cases in ca15187; the rest pre-existing behavior kept on purpose, with the two real ones filed separately). rust:check-all is clean on all 10 targets. CI: build 92351 is final at 195 passed, 1 failed. The failing job is one darwin 14 aarch64 agent whose environment is broken (no default rustup toolchain, so grpc-js/test-tonic fails; its system resolver times out on bogus hostnames, so two resolve-dns cases fail); both are reported for triage and neither is near this diff, which has now passed every other lane on two full runs. Ready for a maintainer pass. |
|
Updated 3:20 PM PT - Aug 11th, 2026
❌ @robobun, your commit b5b9aaa has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32022That installs a local version of the PR into your bun-32022 --bun |
fbe8f1a to
a1c67b5
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a large refactor (35 files, -1.2k net) that reorganizes unsafe placement-init in PackageManager.rs, raw-pointer aliasing in install_with_manager.rs/patchPackage.rs, Windows FFI walkers in PackageInstall.rs, and the bunx cached-bin TOCTOU probe — worth a human pass on the equivalence claims before merging.
Extended reasoning...
Overview
This PR deduplicates ~1.2k lines across the package manager core (src/install/PackageInstall.rs, PackageInstaller.rs, PackageManager.rs, PackageManagerEnqueue.rs, install_with_manager.rs, patchPackage.rs, runTasks.rs, hosted_git_info.rs, npm.rs, yarn.rs, isolated_install.rs) and the CLI commands (bunx, create, link/unlink, outdated, update --interactive, pack, repl, filter_run/multi_run, install/add). It introduces two new shared modules (workspace_helpers.rs, run_processes_shared.rs) and a handful of factored helpers (write_shared_default_fields, walk_install_dir_windows, parse_root_package, load_path_package, replacement_version_literal, format_template/user_project_committish, etc.). New hermetic tests pin the consolidated error-exit paths. The description documents three intentional edge-path byte-level differences and asserts the rest is behavior-identical.
Security risks
The bunx change touches the post-install cached-binary probe in a world-writable temp dir, which is the TOCTOU-hardened path that gates execution on is_trusted_cached_binary. The refactor preserves the uid check and the exec call shape, but it does change the second probe's path-separator bytes on Windows (called out in the description). No new attack surface is introduced, but given this is an exec gate I'd want a human to confirm the consolidation is byte-equivalent on all platforms.
Level of scrutiny
High. This is production-critical: it reshapes the core install loop, the lockfile load/parse path, the lifecycle-script trust gating, and signal handling for multi-process runners. A substantial fraction of the moved code is unsafe — per-field placement writes into the PackageManager singleton (now split between write_shared_default_fields and two callers that must together cover every field), raw-pointer disjoint-borrow splits in parse_root_package/load_path_package, and Windows FFI buffer slicing in walk_install_dir_windows. The equivalence argument for each is plausible and well-commented, but verifying that the two init paths still write every PackageManager field exactly once, and that the aliasing reshapes preserve the original disjointness invariants, is the kind of review a human should sign off on.
Other factors
The bug-hunting pass found nothing, the touched test suites are reported green against the base, and the PR adds tests for the consolidated error paths. It is also stacked on #32000. None of the touched files have a CODEOWNER. Given the breadth (35 files), the density of unsafe, and the criticality of the install path, I'm deferring rather than approving.
|
Agreed this should get a human look. For whoever does the pass, here's where each flagged area was verified during the adopt audit:
Happy to expand on any of these. |
a1c67b5 to
2f5dd36
Compare
There was a problem hiding this comment.
No bugs found, but this is a large consolidation (35 files, +2.2k/-3.4k) across the package-manager core, bunx's cached-bin security probe, and several unsafe raw-pointer aliasing sites — worth a human pass.
What was reviewed:
write_shared_default_fieldsfield coverage vs. both init paths;parse_root_package/load_path_packagedisjoint-storage invariants preserved.walk_install_dir_windowsmatches both deleted per-backend walkers; the non-Windowscopy()fn is not dead (still called on the#[cfg(not(windows))]path).- bunx
try_run_cached_binkeeps theis_trusted_cached_binarygate andRun::run_binaryargs; only the second probe's path separator changes on Windows. hosted_git_infouser_project_committishcollapse checked against bitbucket/gist/sourcehut originals — aux-segment reject, user-optional, and error-as-none flags reproduce each variant's behavior.
Extended reasoning...
Overview
This PR consolidates ~1.2k net lines of duplicated code across the package manager (src/install/) and CLI commands (src/runtime/cli/) into shared helpers: workspace_helpers.rs, run_processes_shared.rs, walk_install_dir_windows, write_shared_default_fields, format_template/user_project_committish in hosted_git_info, and about a dozen smaller extractions. It touches 35 files across install, add/update, link/unlink, outdated, pack, bunx, create, filter/multi-run, and repl. Four new hermetic tests pin the consolidated error exits.
Security risks
The bunx try_run_cached_bin helper wraps the post-install cached-binary probe, which includes the is_trusted_cached_binary TOCTOU hardening against attacker-planted symlinks in a world-writable temp dir. The consolidation preserves the uid check, the Run::run_binary argument list, and the untrusted-binary log-and-skip fallthrough; the only byte-level change is the second probe's path separator on Windows (documented as edge-path exception 1). No new inputs reach security-sensitive parsing, and no validation was loosened.
Level of scrutiny
High. This is production-critical code (the install/resolve/lockfile hot paths, process spawning, and a security-gated exec path) with extensive unsafe blocks whose correctness depends on subtle aliasing and provenance reasoning: write_shared_default_fields does per-field placement writes into a partially-initialized singleton, parse_root_package/load_path_package split &mut PackageManager and &mut Lockfile through raw pointers, and analyze_dependencies_and_install stores raw *mut ContextData across a re-entrant bundler callback. Each site's SAFETY comments were carried over and appear sound, but this is exactly the class of refactor where a missed field-write or a widened aliasing window is easy to overlook and hard to test.
Other factors
- The PR body documents three intentional edge-path deviations and where the original duplicates genuinely differed (parameterized rather than collapsed). robobun's adopt audit already left a per-helper verification breakdown on the thread.
- The
filter_run/multi_runAbortHandlermerge collapses two previously-separateSHOULD_ABORTstatics into one shared static; both commands are mutually exclusive CLI entry points, so the sharing is safe, but it's a design-shape change a maintainer should be aware of. - CI is reported green modulo a known-flaky bunx test tracked separately, and
rust:check-allcovers the Windows-gated code, but the Windows install-backend walkers and the packPackQueueOpenModefd-ownership shuffle (rawFd+CloseOnDrop→File::from_fd+into_raw()) are the kind of thing where a maintainer familiar with the original Zig should confirm the ownership transfer is identical.
Given the breadth, the density of unsafe, and the security-adjacent bunx path, I'm deferring rather than approving.
|
On the two additional items from this pass:
The earlier comment covers |
…update --interactive
… extractor edge cases edit_catalogs_after_update carried a third copy of the alias-prefix and pin-style logic that replacement_version_literal already covers for direct dependencies; use the helper there too and cover aliased catalog entries. Add fromUrl cases for the per-host differences the shared bitbucket, gist, and sourcehut extractor is parameterized on: the rejected aux segment, missing user or project, gist's optional user, and whether an undecodable segment is reported as "not hosted" or as an invalid URL.
…ents The helper has no callers outside bun_install. The pack and repl comments described the helpers in terms of code that no longer exists.
|
@robobun get this mergeable — or close it if the changes are already covered on main / too stale to salvage. |
2f5dd36 to
9ae3ff5
Compare
|
Rebased onto current main and retargeted from the closed #32000 to
|
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given the scope — 37 files across the package manager core (unsafe placement-init of the PackageManager singleton, raw-pointer aliasing reshapes in install_with_manager/patchPackage, cfg-gated Windows install walkers, fd-ownership changes in pack) — a human pass is still warranted before merge.
What was reviewed:
write_shared_default_fields: shared vs. per-path field sets cover the singleton with no gaps or double-writes.replacement_version_literal: alias-prefix + pin-style ordering matches all three original call sites, including the exact-versions branch.user_project_committish(hosted_git_info): per-host reject-aux / optional-user / error-as-none flags reproduce the original bitbucket/gist/sourcehut extractors; new tests pin the edges.archive_pack_queuefd lifetime:Filedrop timing matches the oldCloseOnDropon both success and?paths.
Extended reasoning...
Overview
Large-scale deduplication across src/install/ and src/runtime/cli/: introduces shared helpers (workspace_helpers.rs, run_processes_shared.rs, write_shared_default_fields, walk_install_dir_windows, replacement_version_literal, user_project_committish, try_run_cached_bin, load_package_for_link, parse_root_package, load_path_package, archive_pack_queue, analyze_dependencies_and_install, evaluate_to_value, etc.) and rewires ~two dozen call sites to them. Net −1.2k lines. Adds hermetic tests pinning the consolidated error exits and hosted-git-info extractor edges.
Security risks
The bunx try_run_cached_bin helper still gates execution on is_trusted_cached_binary(destination, uid) before Run::run_binary, and the only byte delta is the platform separator in the second probe path (documented, resolves identically via which_win). No new untrusted-input parsing surface. create_command template-dir probing preserves the has_any_illegal_chars guard on every candidate. No auth/crypto touched.
Level of scrutiny
High. The PR restructures memory-safety-sensitive code: (a) PackageManager init is per-field placement writes into an uninitialized singleton — a missed field is UB; (b) parse_root_package and load_path_package split &mut PackageManager / &mut Lockfile via raw pointers with disjointness argued in SAFETY comments; (c) walk_install_dir_windows and buffered_stdio are Windows-only paths not exercised on the review host; (d) archive_pack_queue changes fd ownership from CloseOnDrop to owning File. These are behavior-preserving on inspection and the robobun audit comments document the field-multiset diff and aliasing invariants, but they are exactly the class of change where a maintainer sign-off is appropriate.
Other factors
- The PR is stacked on #32000 and has been rebased twice through large upstream refactors (#33909, #33032); a maintainer should confirm the base has landed and the branch is current.
- Robobun's prior comment on this PR already agreed it should get a human look.
- CI build 72329 has two failures documented as unrelated (worker-terminate SIGABRT, grpc-tonic toolchain), and alii's most recent comment asks to get it mergeable or close it — a human needs to make that call regardless.
- No prior
claude[bot]review on this PR.
|
Two facts in this pass are stale now:
The two newest commits (catalog updates routed through |
WalkthroughSummaryThe pull request consolidates package installation, dependency management, workspace handling, process execution, and CLI command logic into shared helpers. It also adds regression tests for lockfile, package metadata, alias, hosted Git, and link command behavior. ChangesInstallation and package management
CLI runtime
Suggested reviewers: Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/repl.rs (1)
1355-1361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two interrupt paths disagree on the newline.
The
execution_forbiddenpath prints"\n"unconditionally. ThePendingpath at Line 1382 prints"\n"only whenmode == ReportMode::Print. Both paths represent an interrupted wait, so the newline behavior should match.The
ReportMode::Printdoc at Line 587 describes only the pending case, which suggests the gate was applied to one path and not the other.Confirm the intended behavior, then align the two paths. If the newline is always wanted after an interrupt, remove the gate at Line 1384. If it is print-only, add the gate here.
🤖 Prompt for 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. In `@src/runtime/cli/repl.rs` around lines 1355 - 1361, Align newline handling between the execution_forbidden branch and the Pending interrupt path in the REPL wait logic: choose whether interrupted waits always print a newline or only do so for ReportMode::Print, then apply that same condition to both paths. Update the relevant print call near the execution_forbidden handling and the Pending branch while preserving their existing interruption behavior.
🤖 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/PackageInstall.rs`:
- Around line 2417-2423: Update the fallback in the CreateDirectoryExW failure
branch to propagate the Result from bun_sys::MakePath::make_path_u16 instead of
discarding it. Ensure PackageInstall’s installation flow reports the filesystem
error and cannot succeed when both directory-creation attempts fail.
- Around line 1390-1399: Update the Unix metadata handling in the install flow
around fstat and fchmod so both failures are propagated as install errors rather
than skipped or ignored. Replace the fstat continue path and the discarded
fchmod result while preserving successful metadata application before the
install completes.
- Around line 2397-2411: Update the capacity checks around the destination and
source path construction to reserve one byte for each NUL terminator, rejecting
lengths equal to the remaining buffer capacity. Validate the
offset-plus-path-length arithmetic before slicing or indexing, and return
ENAMETOOLONG for overflow or insufficient capacity so head1/head2 writes in the
surrounding install flow cannot panic.
In `@src/install/PackageManager/PackageManagerEnqueue.rs`:
- Around line 1240-1243: Replace the four `.expect("unreachable")` calls on
`task_callback_list` and `push_dependency_task_callback` within
`enqueue_dependency_with_main_and_success_fn` with `?` propagation. Preserve the
existing callback logic and rely on the function’s `crate::Result<()>` return
type so allocation failures reach the designated error handler instead of
panicking.
In `@src/install/PackageManager/updatePackageJSONAndInstall.rs`:
- Around line 740-755: Replace the direct
WorkspacePackageJSONCache::get_with_path match in the root_package_json_entry
initialization with the existing get_with_path_or_exit helper, passing the same
logger, path bytes, and GetJSONOptions. Preserve the returned entry assignment
and let the shared helper print pending diagnostics and terminate consistently
on read or parse failure.
In `@src/install/yarn.rs`:
- Around line 1726-1734: Update the dep_groups construction in process_deps to
preserve the matched entry’s workspace state when creating each
dependency::Behavior, rather than using only PROD, OPTIONAL, PEER, or DEV.
Ensure entries with workspace: or * versions retain the WORKSPACE flag while
preserving the existing dependency-category flags.
In `@src/runtime/cli/bunx_command.rs`:
- Around line 693-708: Update the write! call constructing absolute_in_cache_dir
so buffer exhaustion is mapped to crate::Error::PathTooLong and propagated from
the surrounding function instead of using expect("unreachable"). Match the
existing error-handling pattern at the other path construction site, while
preserving the successful path construction behavior.
In `@src/runtime/cli/create_command.rs`:
- Around line 1367-1371: Update the HOME lookup in the environment search around
env_loader.map.get to use bun_core::env_var::HOME.key() instead of the hardcoded
b"HOME", preserving the existing mapping behavior and enabling the
platform-specific key.
In `@src/runtime/cli/pack_command.rs`:
- Around line 3090-3092: Update the documentation comment near the loop in pack
to include the `?` propagation from `add_archive_entry` as an additional
early-exit path, while preserving the existing explanation that
`node.complete_one()` is explicitly called on normal loop-body exits and that
`Global::crash()` does not return. Do not change the implementation.
- Around line 1899-1902: Add a SAFETY-style documentation note to
load_package_json_or_exit stating that callers must not use the returned
MapEntry reference after mutating manager_ptr’s workspace_package_json_cache,
including removing or replacing entries. Keep the function signature and
behavior unchanged.
- Around line 3161-3183: Update the failed-stat error handling in the stat
operation to always report item.path, including the PlainFile branch. Remove the
uv_owned_fd-specific Output::err formatting branch and use one shared error
message with item.path for both stat implementations.
In `@src/runtime/cli/pm_update_package_json.rs`:
- Around line 138-141: Validate that the entry-point slice derived from
cli.positionals contains at least one item before constructing entry_points or
DependenciesScanner. Update the existing empty-entry-point check to reject
cli.positionals equal to [b"install"] and return the missing-script error first,
while preserving normal scanning for non-empty entry points.
In `@src/runtime/cli/run_processes_shared.rs`:
- Around line 34-72: Align the platform gating for the abort handler across
install(), uninstall(), and windows_ctrl_handler, using the same
Windows-specific cfg predicate so unsupported non-Unix, non-Windows targets do
not reference unavailable symbols. Also update the buffered_stdio cfg site if
needed to preserve the existing Windows-only libuv type assumptions.
In `@src/runtime/cli/workspace_helpers.rs`:
- Around line 18-26: Update load_lockfile_or_crash to accept a command-specific
wording parameter, following the verb pattern used by load_package_for_link, and
use it when constructing the missing-lockfile message instead of hardcoding
“nothing outdated.” Update each caller, including bun outdated and bun update
--interactive, to pass the appropriate wording.
In `@test/cli/install/hosted-git-info/from-url.test.ts`:
- Around line 24-38: Add negative-control cases to the hosted-git URL
parameterized test proving reject_aux is host-specific: include valid foreign
aux segments such as Bitbucket’s raw URL and SourceHut’s get URL, and assert
they are parsed rather than rejected. Keep the existing host-specific rejection
cases unchanged.
---
Outside diff comments:
In `@src/runtime/cli/repl.rs`:
- Around line 1355-1361: Align newline handling between the execution_forbidden
branch and the Pending interrupt path in the REPL wait logic: choose whether
interrupted waits always print a newline or only do so for ReportMode::Print,
then apply that same condition to both paths. Update the relevant print call
near the execution_forbidden handling and the Pending branch while preserving
their existing interruption behavior.
🪄 Autofix
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: d5917919-8378-4040-b6dd-13998de8f85a
📒 Files selected for processing (37)
src/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageJSONEditor.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/WorkspacePackageJSONCache.rssrc/install/PackageManager/install_with_manager.rssrc/install/PackageManager/patchPackage.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/hosted_git_info.rssrc/install/isolated_install.rssrc/install/npm.rssrc/install/yarn.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/install_command.rssrc/runtime/cli/link_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/pm_update_package_json.rssrc/runtime/cli/repl.rssrc/runtime/cli/repl_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/run_processes_shared.rssrc/runtime/cli/unlink_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/workspace_helpers.rstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-link.test.tstest/cli/install/bun-pack.test.tstest/cli/install/catalogs.test.tstest/cli/install/hosted-git-info/from-url.test.tstest/cli/update_interactive_install.test.ts
|
@alii summary of where this stands:
So it just needs the remaining lanes to finish and a human pass on the equivalence notes; nothing further is pending from my side. |
…age_json_or_exit's lifetime; pin per-host aux segments in fromUrl tests
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Given the scope — ~5.5k lines across 37 files in the package manager and CLI command paths, with hand-verified field-by-field equivalence for the two PackageManager init paths and several parameterized extractions — a human pass on the equivalence notes (as robobun's summary also flags) is still warranted before merge.
Checked: the write_shared_default_fields split covers the same field set as both original init blocks; replacement_version_literal preserves the alias-prefix fallthrough at all three call sites; the hosted_git_info user_project_committish parameterization matches each host's original aux-reject / user-optional / error-as-none behavior (pinned by the new tests); the TrustedDepRecord split preserves the first call site's package_json == lockfile coupling and the second's independent add_to_lockfile.
Extended reasoning...
Overview
This PR consolidates ~1k net lines of duplicated code across the package manager (src/install/) and CLI commands (src/runtime/cli/). It introduces shared helpers for: workspace enumeration and lockfile loading, multi-process run plumbing (signal handling, exit-code aggregation, stdio buffering), the two PackageManager placement-init paths, workspace package.json load-or-exit, trusted-dependency lifecycle enqueue, tarball error dispatch, the yarn dependency-group loop, npm bundleDependencies extraction, hosted_git_info URL extractors, the bunx cached-bin probe, the pack archive queue, the patch path-package loader, the repl evaluate pipeline, and the version-literal replacement in PackageJSONEditor. It also adds tests pinning the shared error exits and the per-host extractor differences.
Security risks
No new attack surface. The bunx try_run_cached_bin helper preserves the existing is_trusted_cached_binary TOCTOU check. The unsafe blocks are refactored placements of pre-existing raw-pointer field projections and placement writes with the same SAFETY invariants restated; no new unsafe patterns are introduced. The PR description documents three intentional edge-path deltas (bunx separator, discarded infallible log.print result, handle_oom vs .expect), all of which are benign or improvements.
Level of scrutiny
High. This touches the core install/add/update/link/outdated/pack/bunx dispatch paths, the PackageManager singleton's per-field placement initialization (where a missed field is UB), Windows-gated install walkers, and several borrow-checker workarounds routed through raw pointers. The claim is byte-identical behavior, which requires line-by-line comparison against main — the kind of verification a human maintainer familiar with the original duplicates is best positioned to spot-check. Robobun's own status update explicitly calls for a human pass on the equivalence notes.
Other factors
CI build 91835 was green on 190 lanes with 6 still queued at last report. The touched test suites pass on debug. Several unresolved CodeRabbit comments remain (the shared "nothing outdated" message wording, the bunx PathTooLong vs .expect, a get_with_path_or_exit reuse suggestion) — these are minor and pre-existing in spirit, but the author may want to address them before merge. Given the size, the criticality of the paths, and the explicit ask for a human equivalence review, I'm not auto-approving.
|
The "other factors" here are a step behind: the three CodeRabbit items named (the shared missing-lockfile wording, the bunx expect, the get_with_path_or_exit reuse) were each answered as pre-existing behavior this PR keeps on purpose and have been resolved, as have the other twelve; the bunx one is filed separately. There are no open threads. The current CI run is build 91858 on ca15187, at 189 passed and 0 failed with 7 lanes left. |
|
@robobun retry ci — darwin lanes expired during the outage, runners are back now. |
|
Retriggered: build 92351 is running on b5b9aaa (an empty commit; the Buildkite token here is read-only, so the expired jobs could not be retried in place). https://buildkite.com/bun/bun/builds/92351 |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given the scope — a ~1k-line net refactor across the install/link/pack/bunx/run command paths, including the unsafe per-field placement init of the PackageManager singleton and several raw-pointer aliasing reshapes — a human pass on the equivalence claims is still warranted.
What was reviewed:
write_shared_default_fieldsvs. both init paths — the 58 shared + 12 per-path fields cover the struct with no double-write.replacement_version_literal's alias handling — theedit_update_no_argssite's distinctdep_literal(workspace_dep vs. original_version_literal) is preserved via the separate parameter.- The
hosted_git_infoshared extractor's per-host flags (reject_aux,user_optional,error_as_none) against each deleted per-host body, matched by the new from-url tests. - The bunx
try_run_cached_binhelper keeps theis_trusted_cached_binaryTOCTOU gate on both probes; only the second probe's path separator changes (documented).
Extended reasoning...
Overview
This PR consolidates ~1k net lines of duplicated logic across 31 Rust files in src/install/ and src/runtime/cli/, plus 6 test files. It extracts shared helpers for: workspace enumeration and lockfile loading, multi-process run plumbing (AbortHandler, exit-code aggregation, buffered_stdio), the two PackageManager init paths' 58 shared field writes, workspace package.json load-or-exit, trusted-dep lifecycle enqueue, tarball-error dispatch, the hosted-git-info bitbucket/gist/sourcehut extractors, the yarn.lock dependency-group loop, npm bundled-deps extraction, the pack archive queue drain, the bunx post-install cached-bin probes, and the --analyze bundler-then-install flow. The PR body documents three intentional edge-path deltas and asserts byte-identical error text/exit semantics everywhere else.
Security risks
The bunx change touches the security-sensitive is_trusted_cached_binary TOCTOU check that guards execution of binaries from a world-writable temp cache. The extracted try_run_cached_bin helper keeps that gate intact on both probes. The PackageManager singleton init uses unsafe per-field placement writes; a missed field would be an uninitialized-memory read. The parse_root_package / load_path_package reshapes involve raw-pointer disjoint-field projections with SAFETY comments. None of these introduce new attack surface, but each depends on the equivalence audit being correct.
Level of scrutiny
High. This is production-critical package-manager and CLI-dispatch code exercised on every bun install/add/update/link/pack/bunx invocation, with substantial unsafe Rust. The claim is zero behavior change across ~3.3k deleted lines rebased over ~780 commits of drift; verifying that the 58-field shared-default list exactly matches both init paths, and that each parameterized helper preserves the per-site variations it replaces, requires the kind of hunk-by-hunk comparison a maintainer should sign off on rather than an automated approval.
Other factors
All 15 CodeRabbit threads are resolved (mostly as pre-existing behavior intentionally kept). robobun's audit notes on the thread already flag the specific areas for a human pass. Test coverage was added for the shared error exits and the hosted-git-info per-host parameters, and the description states the new tests pass on a release build of main (i.e., they pin pre-existing behavior). CI on the current head was retriggered after a darwin runner outage; the prior run was 192/192 with only expired lanes.
|
Retry result (build 92351): same shape as before. 192 lanes passed, nothing failed, and the darwin 26 lanes picked up and passed this time, but the four darwin 14 (aarch64 and x64) jobs expired again without an agent, so that pool specifically still looks down. Everything that has run on this diff has now passed twice; whether to wait on darwin 14 is your call. |
|
Correction to the above: the darwin 14 agents did pick up in the end, just slowly. Build 92351 is final at 195 passed with a single failure, grpc-js/test-tonic.test.ts on one darwin 14 aarch64 shard, which fails because that agent has no default rustup toolchain (same failure and same agent as build 72329 in July, so it is an agent provisioning problem, now reported for triage separately). Nothing this PR touches failed on either run. |
2 participants
What this does
Consolidates duplicated blocks across the package manager and CLI commands (
bun install/add/update/link/unlink/outdated/update --interactive/pack/bunx/create/run --filter|--parallel|--sequential/repl): shared workspace enumeration and lockfile loading (src/runtime/cli/workspace_helpers.rs), shared run-process plumbing (src/runtime/cli/run_processes_shared.rs), the twoPackageManagerinit paths, the workspace package.json load-or-exit helper, the trusted-dependency lifecycle enqueue, the tarball error dispatch, and the hosted_git_info/npm/yarn dedups. Net -1.05k lines (+2251/-3303).Split from #31912 (whole-repo simplification pass, closed in favor of module-scoped splits). This PR only moves and removes code; zero intended behavior change. It was originally stacked on #32000, which has since been closed (its deletions landed on main separately), so it now targets
maindirectly.Behavioral equivalence
Audited hunk by hunk against main: error messages, log-flush order, and exit semantics are byte-identical at every call site, with three reviewed edge-path exceptions:
/. On Windows the probe string changes bytes but names the same file:which_wintakes the absolute-path branch and every consumer (exists check, stat,CreateProcessW) accepts both separators.get_with_path_or_exitandload_lockfile_or_crashhelpers ignore a failedLog::printto stderr where two of the replaced sites propagated it with?. That error path is unreachable: the writer behindOutput::error_writer()discards fd write failures and always returnsOk(adapter_write_allinsrc/sys/lib.rs), so the old?was dead code.bun update --interactivewith--filternow routesWorkspaceFilter::initallocation failure throughbun_core::handle_oom(matchingbun outdated) instead of.expect("OOM").Where the original duplicates genuinely differed (trusted-deps lockfile gating in PackageInstaller, the per-backend Windows walkers, the two PackageManager init paths, gitlab's distinct URL extractor, pack's own package.json error wording), the helpers parameterize the difference instead of collapsing it.
write_shared_default_fieldswas re-derived against the currentPackageManagerstruct: both init paths still write all 70 fields exactly once (58 shared + 12 per path), with every value identical to main's inline writes.Rebase onto main
Main moved ~780 commits since this was written. Besides textual conflicts, the following drift was found and folded in:
runTasks.rs: install: don't re-download a tarball that already failed #34103 replacednetwork_dedupe_map.removewithmark_network_task_failedat both tarball error sites; the shareddispatch_tarball_errordoes the same.PackageInstaller.rs:should_ignore_lifecycle_scriptslost itstree_idargument; the sharedenqueue_lifecycle_scripts_for_trustedfollows. Main's newhas_trusted_dependencycheck sits outside the consolidated block and is unchanged.PackageManager.rs: main removed thecpu_count,ci_mode,default_features, andprogress_name_buf_dynamicfields, renamedcache_directory_, and addedupdating_catalogs; the shared field list reflects that.filter_run.rs/multi_run.rs: main fixed the Windowsuninstallto pass the handler address toSetConsoleCtrlHandler; the sharedAbortHandlercarries the fix.hosted_git_info.rs: main deleted the URL formatters this PR was also consolidating, so only theextractconsolidation (build_result+ the shared bitbucket/gist/sourcehut tail) remains, usingstrings::splitper strings: route all byte search through highway, deny the paths around it #37052.run_command.rs/repl_command.rs:wire_install_optionsuses main'sinstall_preferencefield instead of the old prefer-offline/prefer-latest booleans.PackageJSONEditor.rs: main's newedit_catalogs_after_updatecontained a third copy of the version-literal block this PR deduplicates; it now usesreplacement_version_literaltoo.get_with_source, theFilterRun/MultiRunaliases,PackCommand::exec), and matched main's narrowed visibility (fn finalize,pub(crate)helpers).Tests
bun link/bun unlink(bun-link.test.ts), the pack package.json parse failure (bun-pack.test.ts), and the missing-lockfile error shared bybun outdatedandbun update --interactive(bun-install-registry.test.ts, update_interactive_install.test.ts).fromUrlcases for the per-host differences the shared extractor is parameterized on (rejected aux segment, missing user/project, gist's optional user, and undecodable segments returning null for gist/sourcehut but throwing for bitbucket), and abun update --latestcase for aliased catalog entries. Both new sets pass against a release build of main as well, i.e. they describe the pre-existing behavior.Verification
cargo check --workspaceon all 10 CI targets (bun run rust:check-all), clippy onbun_install/bun_runtime, rustfmt, prettier, andtest/internal/source-lintsare clean.bun-link, 5s timeouts on chains of debug binaries,Bun.versionbeing-debugin the bunx user-agent test, and the shared/tmp/bun-node-debugwipe racing underdescribe.concurrent, which is tracked separately).no test proof · iteration 15 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts test/cli/install/bun-link.test.ts