Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements end-to-end pnpm-style packageExtensions support: config/YAML/env parsing, data types, PackageExtender, resolver manifest_hook, nullable-prefixed hashing, lockfile checksum wiring, freshness/frozen checks, optimistic fast-path tracking, and tests/fixtures. ChangespackageExtensions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 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 #12027 +/- ##
==========================================
- Coverage 88.50% 88.48% -0.03%
==========================================
Files 230 231 +1
Lines 29225 29498 +273
==========================================
+ Hits 25865 26100 +235
- Misses 3360 3398 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1f5741a to
4073080
Compare
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.15682573214,
"stddev": 0.3033867900413,
"median": 2.0407965263400003,
"user": 2.62040528,
"system": 3.3834996399999993,
"min": 1.9751595913400002,
"max": 2.9912526063400002,
"times": [
2.01374425434,
2.01228282834,
2.02495968334,
2.05663336934,
2.07255226334,
2.0209836713400002,
2.22254954334,
2.9912526063400002,
2.17813951034,
1.9751595913400002
]
},
{
"command": "pacquet@main",
"mean": 2.0878488802399997,
"stddev": 0.049952952158503025,
"median": 2.07364959384,
"user": 2.5842082799999995,
"system": 3.3664518400000008,
"min": 2.00447640534,
"max": 2.17028574134,
"times": [
2.15905621734,
2.00447640534,
2.05318807334,
2.0929105153400003,
2.07206905134,
2.07523013634,
2.06322388234,
2.06830966034,
2.11973911934,
2.17028574134
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6389767469199998,
"stddev": 0.011079426643169181,
"median": 0.6370742762200001,
"user": 0.34562203999999996,
"system": 1.32461756,
"min": 0.62174762722,
"max": 0.65850547822,
"times": [
0.65850547822,
0.65262817322,
0.63516807022,
0.64375220922,
0.6345650192200001,
0.62631540622,
0.6367564022200001,
0.6373921502200001,
0.62174762722,
0.64293693322
]
},
{
"command": "pacquet@main",
"mean": 0.6622128418200001,
"stddev": 0.019953226471699663,
"median": 0.65752955822,
"user": 0.35036784,
"system": 1.33437866,
"min": 0.63942457822,
"max": 0.70713723722,
"times": [
0.66384001222,
0.65933804322,
0.6471120652200001,
0.67537396922,
0.65142127622,
0.6557210732200001,
0.67676718922,
0.64599297422,
0.70713723722,
0.63942457822
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2534594746,
"stddev": 0.03485976614913816,
"median": 2.2420406303,
"user": 3.7128850800000004,
"system": 3.0164347400000002,
"min": 2.1922497663000002,
"max": 2.3124016473,
"times": [
2.2873856433,
2.2742235173,
2.2384382643,
2.2389888443,
2.2391994383,
2.2800767923,
2.3124016473,
2.1922497663000002,
2.2448818223,
2.2267490103000003
]
},
{
"command": "pacquet@main",
"mean": 2.2759379622,
"stddev": 0.02109063415013645,
"median": 2.2764394643,
"user": 3.7461075800000003,
"system": 3.0407588400000005,
"min": 2.2441578853,
"max": 2.3217088073,
"times": [
2.2783633823000002,
2.2702464783000003,
2.2551621903,
2.2441578853,
2.2813942753,
2.2748597893,
2.2638276103,
2.2780191393,
2.3217088073,
2.2916400643
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4776285452599998,
"stddev": 0.035734416162806246,
"median": 1.46780461646,
"user": 1.7182248400000002,
"system": 1.7610809,
"min": 1.43581147246,
"max": 1.54825339046,
"times": [
1.47252463646,
1.4708819604599999,
1.46472727246,
1.53341682246,
1.4799180114600001,
1.46070457446,
1.43581147246,
1.46180726346,
1.44824004846,
1.54825339046
]
},
{
"command": "pacquet@main",
"mean": 1.49344279006,
"stddev": 0.05021683018450585,
"median": 1.4851785894599998,
"user": 1.7103150399999998,
"system": 1.7712651999999998,
"min": 1.44975993346,
"max": 1.62138190346,
"times": [
1.46044028946,
1.44975993346,
1.5286477654600001,
1.62138190346,
1.48701308146,
1.45808045246,
1.46783925646,
1.48525027346,
1.49090803946,
1.48510690546
]
}
]
} |
|
Bencher Report| Branch | pr/12027 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,253.46 ms(-2.06%)Baseline: 2,300.81 ms | 2,760.97 ms (81.62%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,477.63 ms(+2.92%)Baseline: 1,435.65 ms | 1,722.78 ms (85.77%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,156.83 ms(+4.83%)Baseline: 2,057.37 ms | 2,468.85 ms (87.36%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 638.98 ms(-5.66%)Baseline: 677.34 ms | 812.81 ms (78.61%) |
Review Summary by QodoPort packageExtensions setting end-to-end with manifest hook and checksum validation
WalkthroughsDescription• Port packageExtensions setting end-to-end from pnpm to pacquet • Add PackageExtender hook to merge extensions onto resolved manifests • Compute and validate packageExtensionsChecksum for lockfile drift detection • Thread ManifestHook through resolver to apply extensions during resolution • Add workspace-state drift check for packageExtensions configuration changes Diagramflowchart LR
Config["Config.package_extensions"]
Extender["PackageExtender<br/>groups by name"]
Hook["ManifestHook<br/>Arc closure"]
Resolver["Resolver<br/>resolve_node"]
Manifest["Manifest<br/>with extensions"]
Checksum["packageExtensionsChecksum<br/>sha256 hash"]
Lockfile["Lockfile<br/>with checksum"]
DriftCheck["Drift check<br/>on frozen install"]
Config -->|new| Extender
Extender -->|into_manifest_hook| Hook
Hook -->|threaded to| Resolver
Resolver -->|apply_to_arc| Manifest
Config -->|compute hash| Checksum
Manifest -->|recorded in| Lockfile
Checksum -->|recorded in| Lockfile
Lockfile -->|compared by| DriftCheck
File Changes1. pacquet/crates/package-manager/src/package_extender.rs
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/package-manager/src/install/tests.rs`:
- Around line 5629-5633: The doc comment above the test is stale: it states
Pacquet doesn't record packageExtensionsChecksum while the test actually asserts
that packageExtensionsChecksum is written to the lockfile; update or remove that
note so the comment matches the test contract. Locate the comment text
mentioning `packageExtensionsChecksum` and the lockfile in the test block (near
the test that asserts the checksum appears in the package's lockfile metadata)
and either remove the sentence claiming it isn't recorded or rewrite it to state
that Pacquet now records `packageExtensionsChecksum` and the test verifies that
behavior.
In `@pacquet/crates/package-manager/src/package_extender.rs`:
- Around line 124-129: The current logic clones the manifest whenever the
package name exists in self.by_pkg_name, even if none of that package's
extension selectors actually match the requested version; instead, lookup the
entries from self.by_pkg_name.get(name) and scan them for any selector/range
match first, and only if at least one entry would apply then perform
Value::clone(&manifest), call self.apply(&mut cloned), and return
Arc::new(cloned); otherwise return the original manifest unchanged. Use the
existing selector-match/check logic used elsewhere (the same predicate that
apply would use) so you avoid unnecessary allocations while keeping the
apply(...) and by_pkg_name symbols intact.
🪄 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: 8b585afe-a6b2-46dc-ba37-763e14e149f0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
pacquet/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/graph-hasher/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/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/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/plans/TEST_PORTING.md
📜 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). (4)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Code Coverage
- 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/package-manager/src/lib.rspacquet/crates/graph-hasher/src/lib.rspacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🧠 Learnings (6)
📚 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/package-manager/src/lib.rspacquet/crates/graph-hasher/src/lib.rspacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/lib.rspacquet/crates/graph-hasher/src/lib.rspacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.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/package-manager/src/lib.rspacquet/crates/graph-hasher/src/lib.rspacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/lib.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rs
🔇 Additional comments (32)
pacquet/crates/package-manager/src/lib.rs (1)
27-27: LGTM!Also applies to: 62-62
pacquet/crates/graph-hasher/src/lib.rs (1)
26-29: LGTM!pacquet/crates/package-manager/src/current_lockfile.rs (1)
88-88: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (1)
95-95: LGTM!pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
71-71: LGTM!pacquet/crates/package-manager/src/current_lockfile/tests.rs (1)
59-59: LGTM!pacquet/crates/config/src/workspace_yaml/tests.rs (1)
873-935: LGTM!pacquet/crates/graph-hasher/src/object_hasher.rs (1)
44-47: ⚡ Quick winPreserve pnpm nullable/hash semantics for empty arrays in
hash_object_nullable_with_prefix
is_nullishcurrently treats onlyValue::Nulland emptyValue::Objectas nullish; pnpm’shashObjectNullableWithPrefixshort-circuits on “empty” inputs such that an empty array ([]) follows the same nullable/undefinedpath, soValue::Array([])should also be treated as nullish (returnNone).🔧 Proposed fix
let is_nullish = match value { Value::Null => true, + Value::Array(arr) => arr.is_empty(), Value::Object(map) => map.is_empty(), _ => false, };pacquet/crates/graph-hasher/src/tests.rs (1)
1-4: LGTM!Also applies to: 155-179
pacquet/crates/lockfile/src/lib.rs (1)
79-94: LGTM!pacquet/plans/TEST_PORTING.md (1)
380-381: LGTM!Also applies to: 751-753
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
78-78: LGTM!pacquet/crates/config/src/lib.rs (1)
38-39: LGTM!Also applies to: 882-897
pacquet/crates/package-manager/src/install.rs (1)
1061-1066: ⚡ Quick winDo not treat this
.ok()as hiding a real drift-detection failure
Config::package_extensionsis serialized frompacquet/crates/config/src/workspace_yaml.rs’sPackageExtension/PeerDependencyMeta, both of whichderive serde::Serializeand only containOption<BTreeMap<String, String>>andOption<bool>(no customSerializeimpls and no float/non-finite cases). With this type shape,serde_json::to_value(extensions)has no meaningful fallible path, so mapping itsErrtoNonewon’t incorrectly mark stale lockfile settings as fresh.> Likely an incorrect or invalid review comment.pacquet/crates/package-manager/Cargo.toml (1)
14-51: LGTM!pacquet/crates/config/src/env_overlay.rs (1)
167-167: LGTM!pacquet/crates/lockfile/src/freshness.rs (1)
103-114: LGTM!Also applies to: 208-241
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
99-103: LGTM!Also applies to: 105-120, 165-171
pacquet/crates/package-manager/src/package_extender/tests.rs (1)
17-197: LGTM!pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
76-83: LGTM!Also applies to: 110-111, 131-132
pacquet/crates/config/src/workspace_yaml.rs (1)
323-337: LGTM!Also applies to: 339-374, 477-477, 685-691
pacquet/crates/package-manager/src/optimistic_repeat_install.rs (1)
210-213: LGTM!Also applies to: 249-271, 303-306
pacquet/crates/package-manager/src/hoisted_dep_graph.rs (1)
1023-1023: LGTM!Also applies to: 1040-1040, 1495-1495, 1533-1533, 1673-1673
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
43-43: LGTM!Also applies to: 209-209, 229-229, 874-874, 996-996, 1147-1147
pacquet/crates/package-manager/src/package_extender.rs (1)
100-103: ⚡ Quick winAdd prerelease-tolerant range matching for pnpm parity
Range::satisfies(node-semver-style behavior) rejects prerelease versions by default, so the currentif !range.satisfies(version) { continue; }will skip extensions for prerelease package versions.
Align this callsite with pnpm’s exact prerelease-matching rules (the earlier “strip to MAJOR.MINOR.PATCH and retry” behavior needs to match pnpm precisely, since node-semver’s defaultsatisfiesbehavior doesn’t provide an equivalent toggle here).pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
119-119: LGTM!Also applies to: 187-187, 271-271, 311-311, 354-354, 432-432, 473-473, 515-515, 559-559, 599-599, 650-650, 699-699, 751-751, 874-874, 952-952, 1000-1000, 1096-1096, 1190-1190, 1296-1296, 1364-1364, 1473-1473, 1580-1580, 1674-1674, 1721-1721, 1754-1754, 1803-1803, 1872-1872, 1926-1926, 1990-1990, 2042-2042
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
57-67: LGTM!Also applies to: 69-77, 79-91, 190-192, 298-302, 319-319, 335-341, 488-488, 492-492, 497-509
pacquet/crates/lockfile/src/freshness/tests.rs (1)
594-595: LGTM!Also applies to: 611-611, 624-624, 644-644, 667-667, 670-670, 688-688, 702-702, 720-720, 738-738, 747-841, 860-860
pacquet/crates/real-hoist/src/tests.rs (1)
37-37: LGTM!Also applies to: 66-66, 117-117, 170-170, 248-248, 319-319, 358-358, 411-411, 515-515, 598-598, 663-663, 739-739, 788-788, 841-841, 895-895, 946-946, 1000-1000, 1037-1037, 1081-1081
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs (1)
428-482: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (2)
532-541: LGTM!Also applies to: 596-596
1063-1063: LGTM!Also applies to: 1067-1079
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
1135-1142: ⚡ Quick winShorten these test doc comments to avoid duplicating test assertions.
The behavior is already fully captured by the test names and assertions; keep only concise intent where needed.
As per coding guidelines, "Tests are documentation. Do not duplicate them in prose."
Also applies to: 1159-1163
🤖 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_with_fresh_lockfile.rs` around lines 1135 - 1142, The long doc comments above the test about packageExtensionsChecksum duplicate the test name and assertions; shorten them to a concise one-line intent (e.g., "Ensure packageExtensions checksum is stable regardless of Config::package_extensions declaration order") and similarly replace the other verbose block at 1159-1163 with a brief intent sentence; update the comment block(s) surrounding the test in install_with_fresh_lockfile.rs rather than changing test logic or names.pacquet/crates/workspace-state/src/tests.rs (1)
90-97: ⚡ Quick winTrim this test doc comment to the non-obvious rationale only.
This block re-describes behavior already asserted in the test body; keep only the “why” and let the test name/assertions document behavior.
As per coding guidelines, "Tests are documentation. Do not duplicate them in prose."
🤖 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/workspace-state/src/tests.rs` around lines 90 - 97, The doc comment above the test is too verbose and duplicates the test's assertions; edit the comment in pacquet/crates/workspace-state/src/tests.rs to remove the prose that re-states behavior (the round-trip description and wire-shape example) and keep only the non-obvious rationale: that packageExtensions must preserve the on-disk shape because optimistic_repeat_install::settings_match reads WorkspaceStateSettings and the drift gate requires byte-comparable output with pnpm's writer; reference packageExtensions and WorkspaceStateSettings in the trimmed comment so the intent remains clear.
🤖 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.
Nitpick comments:
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 1135-1142: The long doc comments above the test about
packageExtensionsChecksum duplicate the test name and assertions; shorten them
to a concise one-line intent (e.g., "Ensure packageExtensions checksum is stable
regardless of Config::package_extensions declaration order") and similarly
replace the other verbose block at 1159-1163 with a brief intent sentence;
update the comment block(s) surrounding the test in
install_with_fresh_lockfile.rs rather than changing test logic or names.
In `@pacquet/crates/workspace-state/src/tests.rs`:
- Around line 90-97: The doc comment above the test is too verbose and
duplicates the test's assertions; edit the comment in
pacquet/crates/workspace-state/src/tests.rs to remove the prose that re-states
behavior (the round-trip description and wire-shape example) and keep only the
non-obvious rationale: that packageExtensions must preserve the on-disk shape
because optimistic_repeat_install::settings_match reads WorkspaceStateSettings
and the drift gate requires byte-comparable output with pnpm's writer; reference
packageExtensions and WorkspaceStateSettings in the trimmed comment so the
intent remains clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cc4e871-83be-454a-b9ef-32a013c70a2b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pacquet/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/graph-hasher/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/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/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/workspace-state/src/tests.rspacquet/plans/TEST_PORTING.md
✅ Files skipped from review due to trivial changes (7)
- pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
- pacquet/crates/package-manager/Cargo.toml
- pacquet/crates/package-manager/src/hoisted_dep_graph.rs
- pacquet/crates/package-manager/src/current_lockfile/tests.rs
- pacquet/plans/TEST_PORTING.md
- pacquet/crates/real-hoist/src/tests.rs
- pacquet/crates/config/src/workspace_yaml/tests.rs
🚧 Files skipped from review as they are similar to previous changes (22)
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/graph-hasher/src/lib.rs
- pacquet/crates/package-manager/src/lib.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
- pacquet/crates/config/src/env_overlay.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/resolving-deps-resolver/src/lib.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
- pacquet/crates/graph-hasher/src/tests.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
- pacquet/crates/config/src/lib.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/package-manager/src/current_lockfile.rs
- pacquet/crates/package-manager/src/install.rs
- pacquet/crates/graph-hasher/src/object_hasher.rs
- pacquet/crates/lockfile/src/lib.rs
- pacquet/crates/lockfile/src/freshness.rs
- pacquet/crates/lockfile/src/freshness/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
- pacquet/crates/package-manager/src/package_extender.rs
- pacquet/crates/resolving-deps-resolver/src/tests.rs
- pacquet/crates/package-manager/src/install/tests.rs
📜 Review details
🧰 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/workspace-state/src/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/package_extender/tests.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/workspace-state/src/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/package_extender/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/workspace-state/src/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/package_extender/tests.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/workspace-state/src/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/package_extender/tests.rs
🔇 Additional comments (4)
pacquet/crates/workspace-state/src/tests.rs (1)
98-123: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
532-542: LGTM!Also applies to: 596-596, 1063-1063, 1067-1079
pacquet/crates/config/src/workspace_yaml.rs (1)
323-337: LGTM!Also applies to: 339-374, 477-477, 685-691
pacquet/crates/package-manager/src/package_extender/tests.rs (1)
1-222: LGTM!
4033560 to
335c025
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 1081-1085: compute_package_extensions_checksum currently
serializes and hashes an empty map as Some(hash) instead of returning None;
change the function to check whether config.package_extensions is an empty map
(after serde_json::to_value) and return None when it's empty, otherwise proceed
to hash with pacquet_graph_hasher::hash_object_nullable_with_prefix; apply the
same empty-map guard to the analogous checksum helper later in the file and add
a unit test that asserts an explicit empty map yields None placed next to the
existing absent-map test.
In `@pacquet/crates/package-manager/src/install.rs`:
- Around line 1061-1070: The current code treats any Some(package_extensions)
(including an empty object) as present and hashes it; change the logic so
package_extensions is considered absent if it's empty before hashing: compute
serde_json::to_value(extensions).ok(), then if the resulting Value is an Object
or Array check emptiness (treat empty Object/Array as None) and only call
pacquet_graph_hasher::hash_object_nullable_with_prefix when the serde Value is
non-empty; keep the final name package_extensions_checksum and pass its
as_deref() into pacquet_lockfile::check_lockfile_settings as before.
In `@pacquet/crates/package-manager/src/package_extender.rs`:
- Around line 48-55: The struct field `range: Option<Range>` currently conflates
"no selector range" and "parse failure", causing parse errors to behave as
match-all; change the representation so parse failures are preserved (e.g. make
`range` an Option<Result<Range, _>> or a small enum like {Unspecified,
Parsed(Range), ParseError}), set it to ParseError when semver parsing fails in
the parser (the code at the current parse site that collapses failures to None),
and then update the selector-matching logic (the code paths that inspect `range`
around the current match blocks at ~100-106 and ~135-138) to treat ParseError as
"no match" while keeping Unspecified as the unconditional-apply case. Ensure all
places that previously assumed Option<Range> are adjusted to handle the new
variant.
🪄 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: c091aa8c-9238-4def-aefd-206f665a0751
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pacquet/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/graph-hasher/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/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/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/workspace-state/src/tests.rspacquet/plans/TEST_PORTING.md
✅ Files skipped from review due to trivial changes (3)
- pacquet/crates/package-manager/Cargo.toml
- pacquet/crates/package-manager/src/hoisted_dep_graph.rs
- pacquet/plans/TEST_PORTING.md
🚧 Files skipped from review as they are similar to previous changes (23)
- pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
- pacquet/crates/resolving-deps-resolver/src/lib.rs
- pacquet/crates/graph-hasher/src/object_hasher.rs
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/package-manager/src/current_lockfile/tests.rs
- pacquet/crates/config/src/workspace_yaml/tests.rs
- pacquet/crates/workspace-state/src/tests.rs
- pacquet/crates/lockfile/src/lib.rs
- pacquet/crates/package-manager/src/install/tests.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/lockfile/src/freshness.rs
- pacquet/crates/package-manager/src/current_lockfile.rs
- pacquet/crates/graph-hasher/src/lib.rs
- pacquet/crates/package-manager/src/package_extender/tests.rs
- pacquet/crates/real-hoist/src/tests.rs
- pacquet/crates/graph-hasher/src/tests.rs
- pacquet/crates/config/src/workspace_yaml.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/lockfile/src/freshness/tests.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
- pacquet/crates/resolving-deps-resolver/src/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📜 Review details
🧰 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/package-manager/src/lib.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender.rs
🧠 Learnings (5)
📚 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/package-manager/src/lib.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/lib.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender.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/package-manager/src/lib.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/package_extender.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
🔇 Additional comments (4)
pacquet/crates/package-manager/src/lib.rs (1)
27-27: LGTM!Also applies to: 62-62
pacquet/crates/config/src/env_overlay.rs (1)
141-141: LGTM!Also applies to: 168-168
pacquet/crates/config/src/lib.rs (1)
38-39: LGTM!Also applies to: 531-538, 891-906
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
99-123: LGTM!Also applies to: 125-145, 189-205, 247-248, 327-329
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
1142-1149: ⚡ Quick winUse regular comments (or remove) instead of narrative
///on tests.These
///blocks restate test behavior and read like body narration. Prefer concise//intent comments (or no comment) to keep tests self-documenting per repo rules.♻️ Proposed change
- /// Ports `installing/.../packageExtensions.ts:103-153` - /// `packageExtensionsChecksum does not change regardless of keys - /// order` — two `Config::package_extensions` populated with the - /// same selectors and entries in a different declared order must - /// produce the same `sha256-…` lockfile checksum. Without the - /// sorted-keys hash, the order-sensitive `IndexMap` iteration - /// would flap the checksum and force a redundant full resolution - /// on every reorder. + // Keep parity with upstream: outer key order must not affect checksum. @@ - /// Empty / absent extensions round-trip to `None`, matching pnpm's - /// `hashObjectNullableWithPrefix(undefined) === undefined` - /// short-circuit. Without this, an absent `packageExtensions` and - /// a configured-but-empty one would write different lockfile - /// fields and the drift gate would fire on no-op installs. + // Match pnpm behavior: absent extensions should not emit a checksum.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: 1166-1170
🤖 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_with_fresh_lockfile.rs` around lines 1142 - 1149, Replace the narrative doc-comment blocks (lines starting with "///") that describe the test behavior—such as the block referencing "packageExtensionsChecksum does not change regardless of keys order" and the similar block at the other occurrence—with regular non-doc comments (//) or remove them; locate the test that mentions packageExtensionsChecksum and change the leading triple-slash comments to ordinary single-line comments (or delete) so the test remains self-documenting and follows repo guidelines.
🤖 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.
Nitpick comments:
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 1142-1149: Replace the narrative doc-comment blocks (lines
starting with "///") that describe the test behavior—such as the block
referencing "packageExtensionsChecksum does not change regardless of keys order"
and the similar block at the other occurrence—with regular non-doc comments (//)
or remove them; locate the test that mentions packageExtensionsChecksum and
change the leading triple-slash comments to ordinary single-line comments (or
delete) so the test remains self-documenting and follows repo guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27656d3c-df34-483e-a198-425f76b2d50c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pacquet/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/graph-hasher/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/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/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/workspace-state/src/tests.rspacquet/plans/TEST_PORTING.md
✅ Files skipped from review due to trivial changes (5)
- pacquet/crates/package-manager/src/hoisted_dep_graph.rs
- pacquet/crates/package-manager/Cargo.toml
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
- pacquet/plans/TEST_PORTING.md
- pacquet/crates/real-hoist/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (27)
- pacquet/crates/package-manager/src/current_lockfile.rs
- pacquet/crates/package-manager/src/current_lockfile/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
- pacquet/crates/graph-hasher/src/object_hasher.rs
- pacquet/crates/config/src/env_overlay.rs
- pacquet/crates/config/src/lib.rs
- pacquet/crates/graph-hasher/src/tests.rs
- pacquet/crates/lockfile/src/lib.rs
- pacquet/crates/resolving-deps-resolver/src/lib.rs
- pacquet/crates/config/src/workspace_yaml/tests.rs
- pacquet/crates/package-manager/src/lib.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
- pacquet/crates/resolving-deps-resolver/src/tests.rs
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/lockfile/src/freshness.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/package-manager/src/package_extender/tests.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
- pacquet/crates/graph-hasher/src/lib.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/package-manager/src/install/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
- pacquet/crates/package-manager/src/install.rs
- pacquet/crates/lockfile/src/freshness/tests.rs
- pacquet/crates/workspace-state/src/tests.rs
- pacquet/crates/package-manager/src/package_extender.rs
- pacquet/crates/config/src/workspace_yaml.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). (5)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- 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/package-manager/src/install_with_fresh_lockfile.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/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/install_with_fresh_lockfile.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/package-manager/src/install_with_fresh_lockfile.rs
🔇 Additional comments (1)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
532-541: LGTM!Also applies to: 598-607, 1070-1086, 1117-1140, 1150-1164, 1171-1176
8d39f52 to
8e87f7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package-manager/src/package_extender.rs`:
- Around line 79-84: The code currently maps parse failures from
parsed.bare_specifier.parse() to RangeFilter::Unparsable and entry_matches
treats that as always false, which silently drops malformed name@range
selectors; change the behavior so that parse errors do not convert to a silent
non-match—either store the original raw range string (e.g., add a
RangeFilter::Raw(String) variant) or, when matching in entry_matches, pass the
raw range_str into the semver satisfier call (semver::satisfies / equivalent) so
any TypeError from node-semver/semver surfaces instead of being swallowed;
update RangeFilter and entry_matches to use the raw range on parse failure (or
re-throw/propgate the parse error) and ensure parsed.bare_specifier handling
preserves the original string for that fallback.
🪄 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: e915362b-d338-48a2-8c53-bc14cc2f2b4b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
pacquet/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/graph-hasher/src/lib.rspacquet/crates/graph-hasher/src/object_hasher.rspacquet/crates/graph-hasher/src/tests.rspacquet/crates/lockfile/src/freshness.rspacquet/crates/lockfile/src/freshness/tests.rspacquet/crates/lockfile/src/lib.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/current_lockfile.rspacquet/crates/package-manager/src/current_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/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/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/real-hoist/src/tests.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/workspace-state/src/tests.rspacquet/plans/TEST_PORTING.md
✅ Files skipped from review due to trivial changes (6)
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/package-manager/Cargo.toml
- pacquet/crates/config/src/workspace_yaml/tests.rs
- pacquet/plans/TEST_PORTING.md
- pacquet/crates/resolving-deps-resolver/src/tests.rs
- pacquet/crates/real-hoist/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (22)
- pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
- pacquet/crates/package-manager/src/current_lockfile.rs
- pacquet/crates/graph-hasher/src/object_hasher.rs
- pacquet/crates/graph-hasher/src/lib.rs
- pacquet/crates/package-manager/src/hoisted_dep_graph.rs
- pacquet/crates/config/src/env_overlay.rs
- pacquet/crates/package-manager/src/lib.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/workspace-state/src/tests.rs
- pacquet/crates/lockfile/src/freshness.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
- pacquet/crates/package-manager/src/current_lockfile/tests.rs
- pacquet/crates/package-manager/src/install.rs
- pacquet/crates/config/src/workspace_yaml.rs
- pacquet/crates/config/src/lib.rs
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
- pacquet/crates/lockfile/src/freshness/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/package-manager/src/install/tests.rs
- pacquet/crates/graph-hasher/src/tests.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.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). (5)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Cargo Deny
- GitHub Check: Code Coverage
- 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/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/lockfile/src/lib.rs
🧠 Learnings (6)
📚 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/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/lockfile/src/lib.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/lockfile/src/lib.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/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/package_extender.rspacquet/crates/package-manager/src/package_extender/tests.rspacquet/crates/lockfile/src/lib.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/lib.rs
🔇 Additional comments (3)
pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
70-73: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
57-67: LGTM!Also applies to: 69-77, 79-90, 190-193, 298-301, 319-320, 335-341, 488-509
pacquet/crates/lockfile/src/lib.rs (1)
79-94: LGTM!
8ea67ec to
f561789
Compare
Adds the `packageExtensions` setting end-to-end on the pacquet side: yaml plumbing, `Config` field, `PNPM_CONFIG_PACKAGE_EXTENSIONS` env var, a `PackageExtender` hook applied during resolution, and the workspace-state drift check. Mirrors pnpm's `createPackageExtender` ([1]) merge semantics — manifest fields win over extension fields on conflict. The lockfile-side `packageExtensionsChecksum` field (and its frozen-install drift gate) is intentionally out of scope; see #12009. [1]: https://github.com/pnpm/pnpm/blob/39101f5e37/hooks/read-package-hook/src/createPackageExtender.ts Refs: #12009 --- Written by an agent (Claude Code, claude-opus-4-7).
Ports the
packageExtensionssetting from pnpm to pacquet, end-to-end. Refs #12009.What lands
Config + yaml plumbing (
pacquet-config):PackageExtension/PeerDependencyMetastructs (mirror pnpm'sPick<BaseManifest, 'dependencies' | 'optionalDependencies' | 'peerDependencies' | 'peerDependenciesMeta'>).WorkspaceSettings::package_extensionswith yaml deserialization,clear_workspace_only_fieldsentry, and empty-map collapse (matches theoverridesshape).Config::package_extensionsfield.PNPM_CONFIG_PACKAGE_EXTENSIONSenv var.Hook (
pacquet-package-manager::PackageExtender):createPackageExtender: groups extensions by package name once, then applies semver range filter and merges fields onto the manifest. Manifest fields win over extension fields on conflict (matches upstream's{ ...extension, ...manifest }spread order).apply_to_arcdeep-clones the innerValueonly when an extension matches — unrelated manifests keep sharing the resolver's cachedArc.Resolver wiring (
pacquet-resolving-deps-resolver):ManifestHooktype, threaded throughResolveDependencyTreeOptions/ResolveImporterOptions/TreeCtx.resolve_noderight afterResolver::resolvereturns and before the result enters the wanted-dep cache — same site as pnpm'sctx.readPackageHook(pkg)call.install_with_fresh_lockfilebuilds the hook fromConfig.package_extensionsonce per install.Workspace state / drift check (
optimistic_repeat_install):current_settingsnow emitspackage_extensionsso.pnpm-workspace-state-v1.jsonround-trips through pnpm.settings_matchcompares the field (treating empty map as absent for cross-implementation parity).settings_match.Lockfile checksum + frozen-install drift gate (
pacquet-graph-hasher,pacquet-lockfile):hash_object_nullable_with_prefixinpacquet-graph-hasher— port of pnpm'shashObjectNullableWithPrefix, byte-for-byte identical (tests pin against the knownsha256-48AVoXIXcTKcnHt8qVKp5vNw4gyOB5VfztHwtYBRcAQ=output upstream's test asserts).Lockfile::package_extensions_checksumtop-level field (matches upstream's wire shape).install_with_fresh_lockfilecomputes and writes the checksum on every fresh install;current_lockfileclones it into the materialized lockfile.check_lockfile_settingsgained apackage_extensions_checksumargument and a newStalenessReason::PackageExtensionsChecksumChangedvariant. The check fires afteroverridesand beforeignoredOptionalDependencies— matches upstream's ordering atgetOutdatedLockfileSetting.ts:53-55so the first-drift error reads the same on both sides.Test plan
PackageExtender(merge semantics, range filter, scoped names, Arc no-op vs clone-on-match, order-invariance).parses_package_extensions_from_yaml_and_applies,empty_package_extensions_map_collapses_to_none).returns_skipped_when_package_extensions_drift— portscheckDepsStatus.test.ts:85.fresh_install_applies_package_extensions_to_dependency_manifest— asserts a packageExtensions-addedpeerDependenciesentry lands in the lockfile'spackagesblock AND thatpackageExtensionsChecksumis recorded.hash_object_nullable_with_prefix_known_value,hash_object_nullable_with_prefix_sorts) — port the upstreamhashObjectNullableWithPrefixjest tests; pinning the prefixedsha256-…output keeps pacquet's checksum byte-identical to pnpm's.check_settings_returns_ok_when_no_package_extensions_checksum_on_either_side,…matches,…value_change,…when_lockfile_has_checksum_but_config_does_not,…when_config_has_checksum_but_lockfile_does_not).frozen_lockfile_errors_when_package_extensions_drift_from_lockfile— drift surfaces asOutdatedLockfile::PackageExtensionsChecksumChanged.cargo nextest run -p pacquet-config -p pacquet-resolving-deps-resolver -p pacquet-package-manager -p pacquet-graph-hasher -p pacquet-lockfile— 859/859 pass.cargo clippy --locked --workspace --all-targets -- --deny warningsclean.just fmt.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Documentation