Conversation
…as both OverrideMap::parse_append (and parse_count) read "overrides" and fell back to "resolutions" only when "overrides" was absent, so a package.json that carried both silently dropped every "resolutions" rule. The same code path serves the root package.json, yarn.lock migration and package-lock.json migration. Parse both fields. "overrides" is parsed first; a "resolutions" rule whose selector "overrides" already defined is skipped before its value is parsed, so a losing flat "npm:" rule does not register an alias either. Rules that only "resolutions" defines are added. Within each field the last spelling of a rule still wins. The frozen-lockfile note names "overrides or resolutions" when the root package.json has both.
|
Warning Review limit reached
Next review available in: 11 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 (4)
Comment |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 62972e8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38811That installs a local version of the PR into your bun-38811 --bun |
|
Status: ready for review. Reproduced against the Verdaccio fixtures: Coverage is in CI: 178 of 179 jobs passed. The one failed job is Windows x64, on Two decisions for the reviewer, both described in the PR body: |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes user-facing package-manager semantics (merging resolutions into overrides, with a precedence rule and an intentional lockfile re-save for projects that currently have both fields), a human sign-off on the design choice would still be worthwhile.
What was reviewed
parse_append/parse_countnow iterate both fields; verifiedArrayHashMap::putreplaces in place so the position-basedRuleCountprecedence is stable and last-spelling-wins within each field is preserved.push_scoped→put_scoped(_, _, 0)refactor is behavior-preserving (the redundantscoped_names.inserton the name-known-but-no-match path is a no-op on a()-valued set).- The losing-
npm:-alias skip is correctly placed beforeparse_override_value, andparse_countover-counting the skipped rule is harmless per the builder-clamp contract.
Extended reasoning...
Overview
This PR changes bun install so that a root package.json with both "overrides" and "resolutions" applies rules from both fields instead of silently dropping "resolutions". Touches src/install/lockfile/OverrideMap.rs (parse loop, RuleCount, put_scoped/scoped_position refactor, hoisting name_hash out of parse_override_value), src/install/PackageManager/install_with_manager.rs (overrides_field_name returns "overrides or resolutions" when both exist), one doc sentence, and 8 new tests in test/cli/install/nested-overrides.test.ts.
Security risks
None identified. This is package.json parsing into an in-memory rule map; no new untrusted-input surface, no path handling, no network. The npm: alias side-table concern is handled defensively (losing rules are skipped before their value is parsed).
Level of scrutiny
Medium-high. The implementation is clean and the position-based precedence relies on a verified invariant (ArrayHashMap::put replaces at the existing index, never reorders). But this is a user-visible behavior change with an explicit design decision — overrides wins over resolutions on conflict (matching pnpm), and projects that today have both fields will re-save their lockfile on next install and fail --frozen-lockfile until updated. That's the fix taking effect, but it's the kind of semantic/UX choice a maintainer should confirm rather than have auto-approved.
Other factors
No CODEOWNERS cover the changed paths. Test coverage is thorough: both-apply, flat/scoped precedence, losing npm: rule registers no alias, last-spelling-within-resolutions preserved, frozen-lockfile note wording, and yarn.lock migration. The push_scoped → put_scoped refactor was checked line-by-line against the old body and is behavior-preserving for keep = 0 (the extra scoped_names.insert in the None arm when the name was already present is idempotent on a ()-valued map). ensure_unused_capacity in parse_from_resolutions correctly reserves additional capacity on top of what parse_from_overrides already used.
Problem
package.jsonwith both an"overrides"map and a"resolutions"map silently ignores the whole"resolutions"map, even when the two pin different packages. No warning, exit 0. Same on 1.3.14 and on both linkers.dependencies: {"two-range-deps": "1.0.0"}(declaresno-deps ^1.0.0and@types/is-number >=1.0.0),overrides: {"no-deps": "1.0.0"},resolutions: {"@types/is-number": "1.0.0"}installsno-deps@1.0.0but@types/is-number@2.0.0. Deleting theoverrideskey makes the sameresolutionsentry apply.OverrideMap::parse_appendandparse_count(src/install/lockfile/OverrideMap.rs) are anif overrides ... else if resolutions ...: exactly one field is ever parsed. The rootpackage.json,yarn.lockmigration (yarn.rs) andpackage-lock.jsonmigration (migration/npm_lock.rs) all go through these two functions.resolutionspins and later gain oneoverridesentry lose everyresolutionspin.Fix
parse_appendparses"overrides", records how many flat and scoped rules it produced, then parses"resolutions".parse_countcounts both fields."overrides"rule wins. This keeps today's behavior for conflicting keys and matches pnpm, which readsresolutionsand letspnpm.overrideswin."resolutions"rule is skipped before its value is parsed (put_rule). This matters because a flatnpm:value also registers the alias inknown_npm_aliases, whichPackageManagerEnqueue.rsconsults before the override map; merging by overwriting the map entry would have left that alias behind and redirected edges the override was supposed to pin."overrides"are identified by position (the first N flat entries / first M scoped entries), so within"resolutions"itself the existing last-spelling-wins behavior is unchanged.push_scopedkeeps its semantics for the lockfile loaders; the parse path uses the newput_scopedwith a keep count.--frozen-lockfilenote saysoverrides or resolutions in package.json changedwhen the root has both fields (install_with_manager.rs), anddocs/pm/overrides.mdxdocuments the merge and the precedence.resolutionsrules now get recorded and applied), so a--frozen-lockfileinstall there fails until the lockfile is updated. That is the fix taking effect; those projects currently have unpinned packages.test/cli/install/nested-overrides.test.ts:overrides and resolutions in the same package.jsonblock (7 tests): both fields apply, flat and scoped precedence, the losingnpm:rule registers nothing, last spelling withinresolutionsstill wins, editingresolutionsis a frozen-lockfile change with the new noteyarn.lock migration carries both overrides and resolutionstest, frozen-clean afterbun pm migratesrc/change (confirmed against the released binary for the flat cases), all 152 tests in the file pass with itbun-lock.test.ts,bun-workspaces.test.ts,migration/migrate.test.ts,migration/yarn-lock-migration.test.tsandbun-audit.test.tsoverride/resolution tests pass;test/internal/source-lints/byte-search.test.tspasses;cargo clippy -p bun_installis cleanBackground
OverrideMapis the in-memory form of the root's override rules.mapholds flat rules (one per package name);scopedholds rules limited to a parent package and/or a target version range ("one-dep>no-deps","no-deps@1", npm object form, yarn path form).PackageManagerEnqueue.rsconsults it for every dependency edge, and bun.lock serializes it as the"overrides"section regardless of which field the rule came from.parse_countsizes the buffer by counting every string the rules will need, thenparse_appendappends and builds the rules. Over-counting is harmless (the builder is clamped afterwards), which is why skipped rules can still be counted.known_npm_aliasesis a side table filled while parsingnpm:specifiers (dependencies, catalogs, flat override rules). When a plain dependency's name matches a registered alias and its range overlaps, the edge is redirected to the alias target before the override map is consulted, so registering an alias for a rule that is then discarded would change resolution.ArrayHashMappreserves insertion order andputon an existing key replaces the value in place, which is what makes "the first N entries came from overrides" stable whileresolutionsis being parsed.