Conversation
Adds a new `pacquet-config-parse-overrides` crate (port of `@pnpm/config.parse-overrides`), threads `overrides` through `Config`/`WorkspaceSettings`, surfaces lockfile-side drift as `StalenessReason::OverridesChanged` (matching upstream's `getOutdatedLockfileSetting` overrides branch), and applies the parsed overrides to a cloned root manifest before the frozen-lockfile freshness check so post-override lockfile specifiers line up with the on-disk manifest. The read-package-hook port (`VersionsOverrider`) mirrors upstream's `createVersionsOverrider` minus the peer-arm promotion, which is deferred until peer install lands. Catalog refs in override values surface as `INVALID_OVERRIDES` until catalogs are ported.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (6)
📝 WalkthroughWalkthroughAdds pnpm.overrides end-to-end: parsing crate, config/workspace/env wiring, lockfile freshness drift detection, PackageManifest mutation support, an in-memory VersionsOverrider, and frozen-lockfile install integration with tests. Changespnpm overrides implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11793 +/- ##
==========================================
+ Coverage 87.20% 87.44% +0.24%
==========================================
Files 190 200 +10
Lines 22525 23353 +828
==========================================
+ Hits 19642 20421 +779
- Misses 2883 2932 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…link Renames single-letter closure / function / generic params introduced by the overrides port to descriptive names, fixes trailing-comma policy in test macro invocations, swaps the Windows path literal to a raw string, and removes a stale `[`Self::root_dir`]` rustdoc link left behind when the `root_dir` field was dropped from `VersionsOverrider`.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4504267256600003,
"stddev": 0.07739234092641506,
"median": 2.41462364796,
"user": 2.8441393400000003,
"system": 3.65458204,
"min": 2.37000642346,
"max": 2.59701099746,
"times": [
2.39319641446,
2.49401397546,
2.53456509146,
2.59701099746,
2.50706827246,
2.41144498246,
2.37376191046,
2.37000642346,
2.40539687546,
2.41780231346
]
},
{
"command": "pacquet@main",
"mean": 2.3968989138600003,
"stddev": 0.0640405012195271,
"median": 2.38187658696,
"user": 2.84290904,
"system": 3.6504337399999995,
"min": 2.31052835346,
"max": 2.48956365046,
"times": [
2.34367617346,
2.36707280146,
2.36468797846,
2.33795620646,
2.48956365046,
2.39668037246,
2.40857162646,
2.46383992046,
2.48641205546,
2.31052835346
]
},
{
"command": "pnpm",
"mean": 4.67321690116,
"stddev": 0.0810941209438727,
"median": 4.68521208746,
"user": 8.002722239999999,
"system": 4.074765640000001,
"min": 4.51329278646,
"max": 4.79361280246,
"times": [
4.74296800746,
4.79361280246,
4.70483135946,
4.73461268846,
4.69652511746,
4.65480276046,
4.59651316646,
4.67389905746,
4.51329278646,
4.62111126546
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.71622058174,
"stddev": 0.056111166659350474,
"median": 0.69559237334,
"user": 0.38880936,
"system": 1.59700932,
"min": 0.6827416193400001,
"max": 0.8717333033400001,
"times": [
0.8717333033400001,
0.71006844834,
0.6918879723400001,
0.69637229034,
0.6882525673400001,
0.6827416193400001,
0.6948124563400001,
0.69258336734,
0.70669987934,
0.72705391334
]
},
{
"command": "pacquet@main",
"mean": 0.72283742154,
"stddev": 0.030310954110029924,
"median": 0.7145529453400001,
"user": 0.39680796000000007,
"system": 1.60627432,
"min": 0.68925859134,
"max": 0.79438161734,
"times": [
0.7401302763400001,
0.69893216534,
0.70070534334,
0.79438161734,
0.70836405134,
0.68925859134,
0.7098729583400001,
0.72964283634,
0.73785344334,
0.7192329323400001
]
},
{
"command": "pnpm",
"mean": 2.58256645044,
"stddev": 0.09561581393528298,
"median": 2.5824811158400003,
"user": 3.07060666,
"system": 2.23032712,
"min": 2.43288324134,
"max": 2.7419570713400003,
"times": [
2.60580316434,
2.44552527634,
2.55915906734,
2.6195921593400002,
2.66229476534,
2.55830024734,
2.54681383934,
2.43288324134,
2.7419570713400003,
2.65333567234
]
}
]
} |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install/tests.rs (1)
1424-1428: ⚡ Quick winTrim test doc-comments to intent-only.
These doc blocks mostly restate the test flow and assertions. Keep only non-obvious rationale and upstream parity links; let the test body carry behavior details.
As per coding guidelines: "Tests are documentation. Do not duplicate them in prose." and "Doc comments (
///,//!) document the contract... They are not a re-narration of the body."Also applies to: 1487-1494
🤖 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 `@pacquet/crates/package-manager/src/install/tests.rs` around lines 1424 - 1428, The long doc-comments above the pnpm.overrides test should be trimmed to an intent-only summary: remove prose that restates the test flow and assertions and keep only the non-obvious rationale or upstream parity links; edit the triple-slash comment block that begins with "`pnpm.overrides` drift..." (and the similar block at the later range around lines 1487-1494) to a single short sentence stating the intent (e.g., "Verify OutdatedLockfile is raised with StalenessReason::OverridesChanged when pnpm.overrides differ") and preserve any upstream link or required rationale only.
🤖 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 `@pacquet/crates/config-parse-overrides/src/lib.rs`:
- Around line 99-105: The doc for parse_overrides is incorrect: it claims
insertion-order behavior but the function takes &HashMap<String,String> and
iterates it, so output follows HashMap iteration order; update the documentation
on parse_overrides to state that HashMap iteration order is not guaranteed and
that callers who require stable/insertion order should use
parse_overrides_iter(map.iter()) (or pass a map type with defined ordering), and
keep the function signature and behavior unchanged; mention parse_overrides,
parse_overrides_iter, VersionOverride, and ParseOverridesError in the doc so
readers can find the alternatives.
In `@pacquet/crates/config/src/workspace_yaml.rs`:
- Around line 583-587: The current conditional for assigning overrides skips
when self.overrides is Some({}) so an explicit empty env overlay cannot clear
prior overrides; change the logic in the block handling self.overrides so that
whenever self.overrides is Some(v) you set config.overrides = Some(v) regardless
of v.is_empty() (i.e., remove the !v.is_empty() check) so an explicit empty map
from self.overrides clears earlier settings.
In `@pacquet/crates/package-manager/src/overrides.rs`:
- Around line 299-305: The branch logic in resolve_local_override_spec leaves
absolute paths un-normalized (it returns
target.absolute_path.display().to_string() in the diff_paths fallback and the
`_` arm), which yields backslashes on Windows; update both of those branches to
route the absolute path through normalize_path (i.e., call normalize_path on the
absolute path string or PathBuf from target.absolute_path) so both the
pathdiff::diff_paths success path and the fallback/non-relative branch produce
normalized forward-slash paths for link:/file: specs; ensure you keep the
existing behavior of using diff_paths when specified_via_relative_path is true
and pkg_dir is Some.
- Around line 228-239: The comparator in sort_by_specificity is not a total
order because it only returns Less/Greater and can disagree for cmp(a,b) vs
cmp(b,a); update sort_by_specificity to return Ordering::Equal in ambiguous
cases and add a deterministic tie-breaker: if lhs and rhs have identical
bare_specifier strings return Ordering::Equal; otherwise, if neither is more
specific according to is_intersecting_range return a stable ordering (e.g.,
compare lhs_spec.cmp(&rhs_spec) or another unique field such as a package
name/id) so the comparator can produce Equal for truly identical entries and a
consistent Less/Greater otherwise; reference function names:
sort_by_specificity, is_intersecting_range,
ResolvedOverride::inner.target_pkg.bare_specifier.
---
Nitpick comments:
In `@pacquet/crates/package-manager/src/install/tests.rs`:
- Around line 1424-1428: The long doc-comments above the pnpm.overrides test
should be trimmed to an intent-only summary: remove prose that restates the test
flow and assertions and keep only the non-obvious rationale or upstream parity
links; edit the triple-slash comment block that begins with "`pnpm.overrides`
drift..." (and the similar block at the later range around lines 1487-1494) to a
single short sentence stating the intent (e.g., "Verify OutdatedLockfile is
raised with StalenessReason::OverridesChanged when pnpm.overrides differ") and
preserve any upstream link or required rationale only.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5222c266-113c-4d2a-b82c-ac9875e587bf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlpacquet/crates/config-parse-overrides/Cargo.tomlpacquet/crates/config-parse-overrides/src/lib.rspacquet/crates/config-parse-overrides/src/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/package-manifest/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Analyze (javascript)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/config/src/env_overlay.rspacquet/crates/package-manifest/src/lib.rspacquet/crates/package-manager/src/lib.rspacquet/crates/config/src/lib.rspacquet/crates/config-parse-overrides/src/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config-parse-overrides/src/lib.rspacquet/crates/package-manager/src/install.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/config/src/env_overlay.rspacquet/crates/package-manifest/src/lib.rspacquet/crates/package-manager/src/lib.rspacquet/crates/config/src/lib.rspacquet/crates/config-parse-overrides/src/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config-parse-overrides/src/lib.rspacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/config/src/env_overlay.rspacquet/crates/package-manifest/src/lib.rspacquet/crates/package-manager/src/lib.rspacquet/crates/config/src/lib.rspacquet/crates/config-parse-overrides/src/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config-parse-overrides/src/lib.rspacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-05-20T23:23:40.606Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11788
File: pacquet/crates/lockfile/src/snapshot_dep_ref.rs:52-56
Timestamp: 2026-05-20T23:23:40.606Z
Learning: In the pacquet lockfile crate, the link target is intentionally represented as a raw `Link(String)` (not a `LinkTarget` newtype) for both `SnapshotDepRef::Link` and `ImporterDepVersion::Link`. pnpm upstream preserves `link:` targets verbatim with only a prefix check (e.g., `refToRelative`), so do not suggest introducing a `LinkTarget` newtype in a bug-fix/targeted PR. If type branding is ever desired, it should be done together for both variants in a separate dedicated refactor PR rather than as part of a small fix.
Applied to files:
pacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rs
🔇 Additional comments (9)
pacquet/crates/lockfile/src/freshness.rs (1)
21-21: LGTM!Also applies to: 89-102, 179-215
pacquet/crates/lockfile/src/freshness/tests.rs (1)
594-595: LGTM!Also applies to: 611-611, 624-624, 644-644, 653-770
pacquet/crates/package-manifest/src/lib.rs (1)
65-65: LGTM!Also applies to: 150-159
pacquet/crates/package-manager/src/lib.rs (1)
24-24: LGTM!Also applies to: 55-55
pacquet/crates/package-manager/src/overrides/tests.rs (1)
1-274: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
30-30: LGTM!Also applies to: 58-58
pacquet/crates/package-manager/src/install.rs (1)
195-204: LGTM!Also applies to: 403-452, 472-484, 840-842, 882-886
pacquet/crates/package-manager/src/install/tests.rs (1)
1430-1485: LGTM!Also applies to: 1496-1573
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
68-68: LGTM!
- `parse_overrides` doc no longer claims insertion-order behavior; it
accurately states that `HashMap` iteration is unordered and points
ordered-output callers at `parse_overrides_iter`.
- `WorkspaceSettings::apply_to` now collapses `overrides: {}` from a
later layer (env overlay, repeat `apply_to`) to `None` on `Config`,
so an explicit empty map clears an earlier non-empty assignment
instead of silently being skipped. Adds a regression test for the
env-overlay-clears-yaml shape.
- `sort_by_specificity` widens its comparator to a 3-way result so
Rust's `sort_by` total-order precondition holds. The strict
Less/Greater arms keep the sort outcome identical to upstream's
first-match choice; the `Equal` arm covers mutually-intersecting
ranges.
- `resolve_local_override_spec` routes the absolute-path and
diff-paths-fallback branches through `normalize_path` too, so
Windows `\` separators get rewritten to `/` for every `link:` /
`file:` shape (not just the diff-paths success branch).
Summary
Ports pnpm's
pnpm.overridesfeature to pacquet:pacquet-config-parse-overridescrate — port of@pnpm/config.parse-overrides. Splitsfoo,foo@^1,bar>foo,bar@1>foo@^2,foo@3 || >=2keys via the[^ |@]>disambiguation; catalog refs surface asERR_PNPM_CATALOG_IN_OVERRIDESuntil catalogs are ported.overridesplumbed throughConfigandWorkspaceSettings— read frompnpm-workspace.yaml, empty maps collapse toNone(matches upstream'sdelete settings.overrides), kept out of globalconfig.yamlviaclear_workspace_only_fields, and acceptsPNPM_CONFIG_OVERRIDESvia env-overlay.StalenessReason::OverridesChanged—check_lockfile_settingschecks overrides beforeignoredOptionalDependencies, matching upstream'sgetOutdatedLockfileSetting.ts:50-56ordering. Comparison is order-insensitive (normalizes toBTreeMap).VersionsOverrider(inpacquet-package-manager) — port ofcreateVersionsOverrider. Generic + parent-scoped buckets, range intersection vianode-semver,-deletion,link:/file:local targets with re-relativization against the importing package's dir. The peer-arm promotion is intentionally deferred until peer install lands.satisfies_package_manifest, so post-override lockfile specifiers line up with the on-disk manifest.WorkspaceState.settings.overridesnow records the resolved map.Drafted because pacquet's resolver isn't ported yet, so this slice is install-side parity only — when the resolver lands, the overrider gets wired into every
readPackageHookcall site (transitive packuments, not just the root) and the peer arm gets ported alongside peer install.Upstream references threaded into doc comments / commit-pinned at the first 10 hex chars per pacquet's link policy.
Test plan
cargo nextest run -p pacquet-config-parse-overrides— 10 tests, all pass.cargo nextest run -p pacquet-config— 209 tests, all pass (3 new overrides cases).cargo nextest run -p pacquet-lockfile— 154 tests, all pass (6 new overrides-drift cases).cargo nextest run -p pacquet-package-manager— 276 tests, all pass (11 new overrider unit tests + 2 new integration tests covering drift error and post-override freshness).cargo fmt --checkcargo clippy --workspace --all-targets -- --deny warningsWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Improvements
Bug Fixes