feat(pacquet): port node/deno/bun runtime resolvers#11783
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a shasums-file crate and three runtime resolver crates (Node, Deno, Bun); each resolver delegates version selection to the npm resolver, enumerates platform-specific release assets and integrities, and package-manager is updated to instantiate and wire these runtime resolvers using a shared Arc-wrapped npm resolver. ChangesRuntime Resolvers for Node, Deno, and Bun
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 |
8a1216c to
34e7fcd
Compare
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11783 +/- ##
==========================================
- Coverage 89.62% 87.23% -2.40%
==========================================
Files 179 190 +11
Lines 21540 22512 +972
==========================================
+ Hits 19306 19638 +332
- Misses 2234 2874 +640 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
172-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
npm_resolverbefore the install pass.
drop(resolver)only releases theDefaultResolverbox. The standalonenpm_resolverbinding still holds a strongArcreference toNpmResolver, so itsmeta_cachestays alive through fetch/import/link and the resolve-pass memory never gets released where the comment says it does.♻️ Minimal fix
// Drop the resolver (and its meta cache) before the install // pass: the tree captures every `ResolveResult` we need. drop(resolver); + drop(npm_resolver);🤖 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_without_lockfile.rs` around lines 172 - 225, The npm_resolver Arc remains live because the local binding holds a strong reference even after creating the DefaultResolver; drop the standalone binding so only the resolver's cloned Arc remains. After constructing resolver (DefaultResolver::new(...) where you wrap ArcResolver(Arc::clone(&npm_resolver)) into the vec), insert drop(npm_resolver) (or otherwise let npm_resolver go out of scope) before the install/fetch/import/link pass so the NpmResolver.meta_cache can be freed as intended.
🧹 Nitpick comments (2)
pacquet/crates/crypto-shasums-file/src/tests.rs (1)
8-13: ⚡ Quick winTrim prose-heavy test doc comments that restate assertions.
These blocks mostly repeat what the test code already makes explicit. Keeping only non-obvious rationale (or upstream parity notes where needed) will better align with repo guidance.
As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."
Also applies to: 47-51, 61-65, 78-84
🤖 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/crypto-shasums-file/src/tests.rs` around lines 8 - 13, The block comment immediately above the #[test] in pacquet/crates/crypto-shasums-file/src/tests.rs is prose-heavy and duplicates what the test asserts; trim it to a single line or remove it entirely, leaving only non-obvious rationale or a brief upstream-parity note (e.g., "mirrors upstream test") so the test itself documents behavior; apply the same trimming to the similar comment blocks at the other test comments (lines referenced in the review: 47-51, 61-65, 78-84) to avoid duplicating assertions in prose.pacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rs (1)
10-14: ⚡ Quick winAdd one non-Linux
armpassthrough assertion.This table currently only proves the Linux
arm -> armv7lpath. Add a non-Linuxarmcase (for exampledarwin,arm) to guard against over-broad remapping regressions.💡 Suggested test addition
fn maps_quirky_arches_to_the_published_tarball_directory_name() { assert_eq!(get_normalized_arch("win32", "ia32", None), "x86"); assert_eq!(get_normalized_arch("linux", "arm", None), "armv7l"); assert_eq!(get_normalized_arch("linux", "x64", None), "x64"); + assert_eq!(get_normalized_arch("darwin", "arm", None), "arm"); }🤖 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/engine-runtime-node-resolver/src/normalize_arch/tests.rs` around lines 10 - 14, The test currently only checks Linux-specific ARM remapping; add a non-Linux ARM passthrough assertion to ensure ARM is not over-remapped outside Linux — in the test function maps_quirky_arches_to_the_published_tarball_directory_name add an assertion like assert_eq!(get_normalized_arch("darwin", "arm", None), "arm") (referencing get_normalized_arch) so non-Linux "arm" inputs remain unchanged.
🤖 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/crypto-shasums-file/src/lib.rs`:
- Around line 183-185: The validator is currently too permissive: is_sha256_hex
accepts uppercase A-F via is_ascii_hexdigit(); change its check to enforce only
lowercase hex by replacing the bytes().all(...) predicate with an explicit check
that each byte is in '0'..='9' or 'a'..='f' (e.g. (b'0'..=b'9') or
(b'a'..=b'f')). Update is_sha256_hex to use that lowercase-only condition and
run/update any tests that assert lowercase-only SHA-256 rows.
In `@pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`:
- Around line 70-88: In resolve_impl, the delegated npm resolution call
currently ignores the caller-provided ResolveOptions by passing
ResolveOptions::default(); update the call to forward the incoming opts (or a
merged/adjusted copy if you must override fields) into npm_resolver.resolve so
the resolution honors caller context and policy flags—locate the
npm_resolver.resolve invocation in resolve_impl and replace the hard-coded
ResolveOptions::default() with opts (or opts.clone()/merged options) while
keeping the constructed WantedDependency logic intact.
In `@pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs`:
- Around line 79-80: The resolver is discarding the incoming ResolveOptions by
calling ResolveOptions::default() inside resolve_impl; update resolve_impl to
propagate the provided &ResolveOptions (the parameter named _opts) into any
downstream resolution calls instead of using ResolveOptions::default(), and
replace the other occurrences that call ResolveOptions::default() (the ones
around lines 93–94) so they forward the same &ResolveOptions reference; ensure
function signatures and calls that expect an owned ResolveOptions convert or
clone if necessary but preserve the original options semantics.
In `@pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs`:
- Around line 126-155: The loop builds `variants` from `assets` but currently
returns Ok(variants) even when no supported artifacts matched; update the end of
the function that currently returns Ok(variants) to check if assets was
non-empty and variants.is_empty() and return a descriptive error via the
existing ReadDenoAssetsError (add a new variant like NoSupportedArtifacts if
needed) instead of Ok; reference the symbols `variants`, `assets`,
`parse_asset_name`, and `ReadDenoAssetsError` and ensure callers still receive
an Err when no supported artifacts are discovered rather than an empty Ok
result.
- Around line 101-121: The code currently calls response.text() and
serde_json::from_str(...) without checking HTTP status; update the block around
release_index_url and the response from
http_client.acquire_for_url(...).get(...).send() to first inspect
response.status() (e.g., !status.is_success()) and return a
ReadDenoAssetsError::FetchReleaseIndex for non-2xx responses including the
version and the status (and optionally the response body or status text) instead
of proceeding to parse; ensure this check happens before calling response.text()
and before attempting serde_json::from_str(&body) so non-2xx GitHub responses
are surfaced as FetchReleaseIndex failures rather than decode/missing-assets
errors.
In `@pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs`:
- Around line 167-191: resolve_latest_impl currently performs network calls even
when offline; replicate the same offline guard used in resolve_impl to avoid
calling resolve_node_version/get_node_mirror when offline. Locate resolve_impl
and copy its early-return check for opts.offline into resolve_latest_impl (using
the _opts: &ResolveOptions param) so that resolve_latest_impl returns the exact
same value/error path as resolve_impl when offline, before calling
parse_node_specifier or resolve_node_version.
In `@pacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rs`:
- Around line 29-31: The current unconditional mapping of arch == "arm" to
"armv7l" should be applied only for Linux targets; update the condition in
normalize_arch (the arch == "arm" check) to also assert the platform is Linux
(e.g., check an os/target variable or use cfg/target_os logic) so non-Linux ARM
targets are not remapped to "armv7l".
In `@pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs`:
- Around line 110-118: The filter currently only keeps versions where
Version::parse(version).map(|parsed| parsed.satisfies(&parsed_range)) succeeds,
which rejects prerelease builds; change the closure used on
versions.into_iter().filter(...) to first try parsed.satisfies(&parsed_range)
and if that returns false and the parsed Version has a prerelease component,
create a fallback by clearing the prerelease (e.g., clone parsed, remove its
prerelease metadata) and check that fallback.satisfies(&parsed_range); return
true if either check passes. Apply the same change to the analogous block that
uses Range::parse/Version::parse at the other site (the block around the 194-200
range) so both places accept prerelease matches via the stripped-prerelease
fallback while preserving the original behavior when no prerelease is present.
- Around line 129-138: The response is read and deserialized before checking
HTTP status, which turns non-2xx responses into DecodeIndex errors; after
calling http_client.acquire_for_url(&url).await.get(&url).send().await (the
response variable), check response.status().is_success() and if not return
ResolveNodeVersionError::FetchIndex with the url and the response status/error,
then call response.text().await and proceed to serde_json::from_str into
Vec<RawNodeVersion>, preserving existing DecodeIndex only for JSON parse errors.
---
Outside diff comments:
In `@pacquet/crates/package-manager/src/install_without_lockfile.rs`:
- Around line 172-225: The npm_resolver Arc remains live because the local
binding holds a strong reference even after creating the DefaultResolver; drop
the standalone binding so only the resolver's cloned Arc remains. After
constructing resolver (DefaultResolver::new(...) where you wrap
ArcResolver(Arc::clone(&npm_resolver)) into the vec), insert drop(npm_resolver)
(or otherwise let npm_resolver go out of scope) before the
install/fetch/import/link pass so the NpmResolver.meta_cache can be freed as
intended.
---
Nitpick comments:
In `@pacquet/crates/crypto-shasums-file/src/tests.rs`:
- Around line 8-13: The block comment immediately above the #[test] in
pacquet/crates/crypto-shasums-file/src/tests.rs is prose-heavy and duplicates
what the test asserts; trim it to a single line or remove it entirely, leaving
only non-obvious rationale or a brief upstream-parity note (e.g., "mirrors
upstream test") so the test itself documents behavior; apply the same trimming
to the similar comment blocks at the other test comments (lines referenced in
the review: 47-51, 61-65, 78-84) to avoid duplicating assertions in prose.
In `@pacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rs`:
- Around line 10-14: The test currently only checks Linux-specific ARM
remapping; add a non-Linux ARM passthrough assertion to ensure ARM is not
over-remapped outside Linux — in the test function
maps_quirky_arches_to_the_published_tarball_directory_name add an assertion like
assert_eq!(get_normalized_arch("darwin", "arm", None), "arm") (referencing
get_normalized_arch) so non-Linux "arm" inputs remain unchanged.
🪄 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: 2145cf06-b447-4d19-a175-9877ea2aedda
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlpacquet/crates/crypto-shasums-file/Cargo.tomlpacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-bun-resolver/Cargo.tomlpacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/engine-runtime-deno-resolver/Cargo.tomlpacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/engine-runtime-node-resolver/Cargo.tomlpacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_without_lockfile.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). (2)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
🧰 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/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs
🧠 Learnings (1)
📚 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/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs
🔇 Additional comments (24)
pacquet/crates/crypto-shasums-file/Cargo.toml (1)
1-27: LGTM!pacquet/crates/engine-runtime-bun-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rs (1)
1-52: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/lib.rs (1)
1-17: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rs (1)
1-125: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rs (1)
1-40: LGTM!Cargo.toml (1)
19-22: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
14-46: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (2)
12-28: LGTM!
557-584: LGTM!pacquet/crates/engine-runtime-deno-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rs (1)
1-54: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/lib.rs (1)
1-27: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rs (1)
1-60: LGTM!pacquet/crates/engine-runtime-node-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rs (1)
4-53: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rs (1)
1-104: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rs (1)
6-41: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rs (1)
1-40: LGTM!pacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rs (1)
13-20: 🏗️ Heavy liftThis refactoring suggestion contradicts the documented design decision.
The code explicitly explains (lines 7–12) why
release_channelremains aStringrather than an enum:Pacquet keeps the value as a
String(rather than a closed enum) because the upstream parser passes the channel through to the mirror URL builder unchanged — the set is closed today but the validation happens at parse time, not after.While the guideline states that string-literal unions should become enums, this codebase has a legitimate reason for the exception: the validated channel is passed through directly to external mirror URL builders without conversion, making an enum-based wrapper counterproductive. The parsing validation ensures only valid channels are constructed; no invalid states are representable after successful parsing.
> Likely an incorrect or invalid review comment.pacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rs (1)
1-63: LGTM!pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rs (1)
1-39: LGTM!pacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rs (1)
1-82: LGTM!pacquet/crates/engine-runtime-node-resolver/src/lib.rs (1)
1-52: LGTM!
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3893347382399996,
"stddev": 0.11504916512830686,
"median": 2.3586885610399997,
"user": 2.82580524,
"system": 3.6026836,
"min": 2.29770032304,
"max": 2.6788092520399998,
"times": [
2.3511342210399997,
2.36736153104,
2.29770032304,
2.36670160704,
2.6788092520399998,
2.31795965104,
2.3662429010399997,
2.3280567560399996,
2.32522541004,
2.4941557300399997
]
},
{
"command": "pacquet@main",
"mean": 2.3874420815399997,
"stddev": 0.05866957693172784,
"median": 2.38629080954,
"user": 2.7985644400000007,
"system": 3.6118190000000006,
"min": 2.32282184904,
"max": 2.5111951160399997,
"times": [
2.39213139504,
2.38481690204,
2.3534964230399997,
2.38776471704,
2.5111951160399997,
2.4554141020399998,
2.33658120004,
2.3975170700399997,
2.33268204104,
2.32282184904
]
},
{
"command": "pnpm",
"mean": 4.63600182974,
"stddev": 0.05179599476919672,
"median": 4.63208822354,
"user": 7.85788614,
"system": 4.054741400000001,
"min": 4.53616059804,
"max": 4.72479965904,
"times": [
4.661963618040001,
4.72479965904,
4.69120222404,
4.59782223804,
4.63179389104,
4.6323825560400005,
4.614725792040001,
4.61689454904,
4.53616059804,
4.65227317204
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.70759278384,
"stddev": 0.03386379613560978,
"median": 0.70153484464,
"user": 0.36975486,
"system": 1.60561844,
"min": 0.67602041564,
"max": 0.7970597916400001,
"times": [
0.7970597916400001,
0.70073311764,
0.7023365716400001,
0.69407634064,
0.72221141064,
0.68899270264,
0.70249682364,
0.67602041564,
0.70619511764,
0.68580554664
]
},
{
"command": "pacquet@main",
"mean": 0.7137364823400001,
"stddev": 0.03278912527167816,
"median": 0.70492309064,
"user": 0.38179156000000003,
"system": 1.6117639400000001,
"min": 0.68237593364,
"max": 0.7836529586400001,
"times": [
0.7836529586400001,
0.71340608664,
0.70697278364,
0.6848078536400001,
0.7284571896400001,
0.6949475176400001,
0.68709477864,
0.68237593364,
0.75277632364,
0.70287339764
]
},
{
"command": "pnpm",
"mean": 2.4252432424399997,
"stddev": 0.08673668007185306,
"median": 2.3995192036399997,
"user": 3.00643716,
"system": 2.20026614,
"min": 2.31959589864,
"max": 2.57111165764,
"times": [
2.5660909146399997,
2.4590191306399998,
2.37460927464,
2.3749469416399998,
2.4451284536399998,
2.4240914656399997,
2.36023234564,
2.31959589864,
2.35760634164,
2.57111165764
]
}
]
} |
849e9a0 to
7a86921
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs (1)
117-119: ⚡ Quick winAdd focused regression tests for the prerelease and fetch/decode branches.
These paths pin pnpm-compat behavior and are easy to regress later. Please add a couple of targeted tests here for
rc/*-style matching and for non-2xxindex.jsonresponses staying on theFetchIndexside instead of slipping intoDecodeIndex.Based on learnings: only match pnpm’s behavior exactly, and prerelease-tolerant matching in pacquet should be implemented inline against
node-semver.Also applies to: 130-140, 208-250
🤖 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/engine-runtime-node-resolver/src/resolve_node_version.rs` around lines 117 - 119, Add focused unit tests covering two regressions: (1) add tests that exercise Version::parse + satisfies_with_prereleases behavior using rc/*-style prerelease versions (e.g. "1.2.3-rc.1") to ensure pacquet matches pnpm's prerelease-tolerant semantics exactly (write assertions for positive and negative matches against ranges that should and should not accept prereleases); and (2) add tests that simulate non-2xx responses when fetching index.json to ensure the resolver stays in the FetchIndex branch (does not fall through to DecodeIndex) by asserting the resolver returns the fetch error or appropriate FetchIndex variant when the HTTP status is non-2xx. Place tests near the resolve_node_version test module and cover the same helper logic used by satisfies_with_prereleases and the index fetch/decode flow so the paths identified by Version::parse / satisfies_with_prereleases and the FetchIndex vs DecodeIndex handling are exercised.pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs (1)
103-117: ⚡ Quick winAdd a regression test for non-2xx release-index responses.
This is the boundary between
FetchReleaseIndexandDecodeReleaseIndex, so a small 404/500-focused test would pay off here and keep the error contract from drifting during later refactors.As per coding guidelines: "Match pnpm's error codes and messages where pnpm defines them. Error codes are part of the public contract, not implementation detail."
🤖 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/engine-runtime-deno-resolver/src/read_deno_assets.rs` around lines 103 - 117, Add a regression test that ensures non-2xx HTTP responses when fetching the release index produce ReadDenoAssetsError::FetchReleaseIndex (not DecodeReleaseIndex) and that the error payload preserves status information and matches pnpm's public error codes/messages; specifically, write tests that stub the http_client used by acquire_for_url to return 404 and 500 responses for release_index_url, call the function that drives this path (the code that awaits .acquire_for_url(...).get(...).send()), and assert the returned Err is the FetchReleaseIndex variant with the expected version string and an inner error that reflects the non-2xx status so the contract cannot drift during refactors.
🤖 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/engine-runtime-deno-resolver/src/read_deno_assets.rs`:
- Around line 103-117: Add a regression test that ensures non-2xx HTTP responses
when fetching the release index produce ReadDenoAssetsError::FetchReleaseIndex
(not DecodeReleaseIndex) and that the error payload preserves status information
and matches pnpm's public error codes/messages; specifically, write tests that
stub the http_client used by acquire_for_url to return 404 and 500 responses for
release_index_url, call the function that drives this path (the code that awaits
.acquire_for_url(...).get(...).send()), and assert the returned Err is the
FetchReleaseIndex variant with the expected version string and an inner error
that reflects the non-2xx status so the contract cannot drift during refactors.
In `@pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs`:
- Around line 117-119: Add focused unit tests covering two regressions: (1) add
tests that exercise Version::parse + satisfies_with_prereleases behavior using
rc/*-style prerelease versions (e.g. "1.2.3-rc.1") to ensure pacquet matches
pnpm's prerelease-tolerant semantics exactly (write assertions for positive and
negative matches against ranges that should and should not accept prereleases);
and (2) add tests that simulate non-2xx responses when fetching index.json to
ensure the resolver stays in the FetchIndex branch (does not fall through to
DecodeIndex) by asserting the resolver returns the fetch error or appropriate
FetchIndex variant when the HTTP status is non-2xx. Place tests near the
resolve_node_version test module and cover the same helper logic used by
satisfies_with_prereleases and the index fetch/decode flow so the paths
identified by Version::parse / satisfies_with_prereleases and the FetchIndex vs
DecodeIndex handling are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 670029ca-fb32-4514-9a15-c25a49a99814
📒 Files selected for processing (5)
pacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/package-manager/src/install_without_lockfile.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: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- 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/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs
🧠 Learnings (2)
📚 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/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.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/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs
🔇 Additional comments (3)
pacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rs (1)
14-18: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
266-274: LGTM!pacquet/crates/crypto-shasums-file/src/lib.rs (1)
184-190: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rs (1)
30-33: ⚡ Quick winUse
DEFAULT_NODE_MIRROR_BASE_URLin fallback to prevent drift.Line 32 duplicates the official base URL literal instead of reusing the constant declared in this module.
♻️ Proposed fix
- let mirror = node_download_mirrors + let mirror = node_download_mirrors .and_then(|map| map.get(release_channel).cloned()) - .unwrap_or_else(|| format!("https://nodejs.org/download/{release_channel}/")); + .unwrap_or_else(|| { + if release_channel == "release" { + DEFAULT_NODE_MIRROR_BASE_URL.to_string() + } else { + format!("https://nodejs.org/download/{release_channel}/") + } + });🤖 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/engine-runtime-node-resolver/src/get_node_mirror.rs` around lines 30 - 33, The fallback string literal for the Node mirror should use the module constant DEFAULT_NODE_MIRROR_BASE_URL instead of duplicating the URL; update the unwrap_or_else branch that constructs mirror (which currently builds "https://nodejs.org/download/{release_channel}/") to use format!("{DEFAULT_NODE_MIRROR_BASE_URL}{release_channel}/") (or equivalent) so node_download_mirrors.and_then(|map| map.get(release_channel).cloned()).unwrap_or_else(...) produces the same value, then pass that to normalize_node_mirror(&mirror) as before.pacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rs (1)
5-8: ⚡ Quick winPrefer minimal rationale comments over scenario narration in tests.
The assertions already capture these cases; reduce prose to brief upstream-compatibility context only.
As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."
Also applies to: 21-23
🤖 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/engine-runtime-node-resolver/src/normalize_arch/tests.rs` around lines 5 - 8, Replace the verbose scenario narration in the test comment that mirrors upstream normalizeArch.test.ts with a short upstream-compatibility note: keep a single-line context like "Matches upstream normalizeArch.test.ts behavior" and remove the detailed prose listing scenarios; apply the same reduction to the similar comment block around the second occurrence (previously lines 21-23) so tests document intent succinctly without duplicating assertions.pacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rs (1)
5-11: ⚡ Quick winTrim test doc-comments that restate asserted cases.
These blocks mostly narrate scenarios the test table/assertions already document. Keep only concise “why” context and upstream link.
As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."
Also applies to: 83-86
🤖 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/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rs` around lines 5 - 11, The doc-comment block above the table-driven test in tests.rs repeats the test cases; trim it to a concise context and the upstream link only. Specifically, remove prose that narrates every asserted scenario (the mappings and quirks already encoded in the test table), leaving a short “why” line and the upstream getNodeArtifactAddress link; apply the same trimming to the similar block later in the file (the comment around the second test table).pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rs (1)
33-51: 🏗️ Heavy liftAdd at least one happy-path and one error-path Bun resolution test.
These tests only assert early rejection. Given current coverage gaps, add cases that exercise asset selection/integrity parsing and
BUN_RESOLUTION_FAILUREmapping to protect resolver behavior.🤖 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/engine-runtime-bun-resolver/src/bun_resolver/tests.rs` around lines 33 - 51, Add two tests: one “happy-path” that constructs a WantedDependency with alias Some("bun") and a runtime-prefixed bare_specifier (e.g., "runtime:1.0.0"), calls resolver().resolve(&wanted, &ResolveOptions::default()).await.unwrap(), asserts Some(result) and validates the returned Asset selection/integrity fields (check the resolved object's asset list and integrity string/parsing); and one “error-path” that simulates a Bun resolver failure (triggering BUN_RESOLUTION_FAILURE mapping) by arranging the resolver to return an error condition for a bun alias (e.g., using the same resolver test harness/mock), calls resolver().resolve(...).await, unwraps the error or maps result and asserts the resolution maps to the expected BUN_RESOLUTION_FAILURE variant/marker. Reference WantedDependency, resolver().resolve, ResolveOptions, and the BUN_RESOLUTION_FAILURE mapping in your new tests.pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs (1)
168-188: 💤 Low valueConsider extracting shared helper functions to a common module.
bare_runtime_spec,*_bin_for_current_os, andcurrent_platformare duplicated nearly identically inbun_resolver.rsanddeno_resolver.rs. Extracting them to a shared utility module would reduce duplication.🤖 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/engine-runtime-bun-resolver/src/bun_resolver.rs` around lines 168 - 188, Extract the duplicated helpers into a shared module and import them from both resolvers: create a new module (e.g., resolver_utils) and move bare_runtime_spec, bun_bin_for_current_os (rename to runtime_bin_for_platform if you prefer a neutral name), and current_platform into it; make them pub(crate) or pub(super) as needed so bun_resolver.rs and deno_resolver.rs can call bare_runtime_spec(...), bun_bin_for_current_os(...) and current_platform() instead of having local copies; update both files to remove the duplicated functions and add use statements to reference the shared helpers, and run/update any tests that reference these functions.
🤖 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/crypto-shasums-file/src/lib.rs`:
- Around line 156-160: The lookup currently requires two leading spaces before
the filename (needle = format!(" {file_name}")) which is stricter than
upstream; change the suffix match to use a single-space prefix (e.g., needle =
format!(" {file_name}")) and keep the same trimmed_end().ends_with(&needle)
check so the body.lines().find(...) mirrors upstream pnpm behavior and avoids
incorrectly returning PickFileChecksumError::NotFound for rows upstream would
accept.
In `@pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs`:
- Around line 127-157: The code currently masks hex decode failures by using
decode_hex(&sha256).unwrap_or_default(), producing an empty payload and a
misleading Integrity parse error; update the loop in the function that builds
variants (the block using parse_asset_name, fetch_sha256, decode_hex and
constructing integrity_string) to explicitly handle decode_hex errors instead of
unwrap_or_default: if decode_hex returns Err or None, return or map to a clear
ReadDenoAssetsError variant (e.g., InvalidSha or ShaDecode) that includes the
offending asset.browser_download_url and the original decode error, so the
failure is reported directly before building integrity_string and parsing into
Integrity.
---
Nitpick comments:
In `@pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`:
- Around line 168-188: Extract the duplicated helpers into a shared module and
import them from both resolvers: create a new module (e.g., resolver_utils) and
move bare_runtime_spec, bun_bin_for_current_os (rename to
runtime_bin_for_platform if you prefer a neutral name), and current_platform
into it; make them pub(crate) or pub(super) as needed so bun_resolver.rs and
deno_resolver.rs can call bare_runtime_spec(...), bun_bin_for_current_os(...)
and current_platform() instead of having local copies; update both files to
remove the duplicated functions and add use statements to reference the shared
helpers, and run/update any tests that reference these functions.
In `@pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rs`:
- Around line 33-51: Add two tests: one “happy-path” that constructs a
WantedDependency with alias Some("bun") and a runtime-prefixed bare_specifier
(e.g., "runtime:1.0.0"), calls resolver().resolve(&wanted,
&ResolveOptions::default()).await.unwrap(), asserts Some(result) and validates
the returned Asset selection/integrity fields (check the resolved object's asset
list and integrity string/parsing); and one “error-path” that simulates a Bun
resolver failure (triggering BUN_RESOLUTION_FAILURE mapping) by arranging the
resolver to return an error condition for a bun alias (e.g., using the same
resolver test harness/mock), calls resolver().resolve(...).await, unwraps the
error or maps result and asserts the resolution maps to the expected
BUN_RESOLUTION_FAILURE variant/marker. Reference WantedDependency,
resolver().resolve, ResolveOptions, and the BUN_RESOLUTION_FAILURE mapping in
your new tests.
In
`@pacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rs`:
- Around line 5-11: The doc-comment block above the table-driven test in
tests.rs repeats the test cases; trim it to a concise context and the upstream
link only. Specifically, remove prose that narrates every asserted scenario (the
mappings and quirks already encoded in the test table), leaving a short “why”
line and the upstream getNodeArtifactAddress link; apply the same trimming to
the similar block later in the file (the comment around the second test table).
In `@pacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rs`:
- Around line 30-33: The fallback string literal for the Node mirror should use
the module constant DEFAULT_NODE_MIRROR_BASE_URL instead of duplicating the URL;
update the unwrap_or_else branch that constructs mirror (which currently builds
"https://nodejs.org/download/{release_channel}/") to use
format!("{DEFAULT_NODE_MIRROR_BASE_URL}{release_channel}/") (or equivalent) so
node_download_mirrors.and_then(|map|
map.get(release_channel).cloned()).unwrap_or_else(...) produces the same value,
then pass that to normalize_node_mirror(&mirror) as before.
In `@pacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rs`:
- Around line 5-8: Replace the verbose scenario narration in the test comment
that mirrors upstream normalizeArch.test.ts with a short upstream-compatibility
note: keep a single-line context like "Matches upstream normalizeArch.test.ts
behavior" and remove the detailed prose listing scenarios; apply the same
reduction to the similar comment block around the second occurrence (previously
lines 21-23) so tests document intent succinctly without duplicating assertions.
🪄 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: 744b4819-b62b-49a4-a207-ed78ae563e01
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomlpacquet/crates/crypto-shasums-file/Cargo.tomlpacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-bun-resolver/Cargo.tomlpacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/engine-runtime-deno-resolver/Cargo.tomlpacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/engine-runtime-node-resolver/Cargo.tomlpacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_without_lockfile.rs
✅ Files skipped from review due to trivial changes (1)
- pacquet/crates/engine-runtime-deno-resolver/Cargo.toml
📜 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: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-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/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs
🧠 Learnings (2)
📚 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/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.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/engine-runtime-node-resolver/src/get_node_artifact_address/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch/tests.rspacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver/tests.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rspacquet/crates/crypto-shasums-file/src/tests.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rspacquet/crates/engine-runtime-bun-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rspacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rspacquet/crates/crypto-shasums-file/src/lib.rspacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rspacquet/crates/engine-runtime-node-resolver/src/node_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rspacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rspacquet/crates/engine-runtime-node-resolver/src/lib.rspacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rspacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rspacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs
🔇 Additional comments (37)
pacquet/crates/crypto-shasums-file/Cargo.toml (1)
1-27: LGTM!pacquet/crates/engine-runtime-node-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/engine-runtime-node-resolver/src/normalize_arch.rs (1)
17-37: LGTM!Cargo.toml (1)
19-22: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets/tests.rs (1)
1-60: LGTM!pacquet/crates/crypto-shasums-file/src/tests.rs (1)
1-99: LGTM!pacquet/crates/engine-runtime-bun-resolver/Cargo.toml (1)
1-34: LGTM!pacquet/crates/engine-runtime-node-resolver/src/node_resolver/tests.rs (1)
1-83: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_artifact_address.rs (1)
1-54: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver/tests.rs (1)
1-54: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/lib.rs (1)
1-17: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/lib.rs (1)
1-27: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets/tests.rs (1)
1-40: LGTM!pacquet/crates/engine-runtime-node-resolver/src/get_node_mirror/tests.rs (1)
1-40: LGTM!pacquet/crates/engine-runtime-bun-resolver/src/read_bun_assets.rs (1)
1-124: LGTM!pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs (1)
1-374: LGTM!pacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier/tests.rs (1)
1-63: LGTM!pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version/tests.rs (1)
1-39: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
12-33: LGTM!Also applies to: 115-122, 183-264, 303-314, 603-631
pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs (3)
1-49: LGTM!
51-119: LGTM!
121-165: LGTM!pacquet/crates/engine-runtime-node-resolver/src/parse_node_specifier.rs (3)
1-37: LGTM!
39-96: LGTM!
98-141: LGTM!pacquet/crates/engine-runtime-node-resolver/src/lib.rs (1)
1-53: LGTM!pacquet/crates/engine-runtime-deno-resolver/src/read_deno_assets.rs (3)
1-82: LGTM!
84-126: LGTM!
159-248: LGTM!pacquet/crates/engine-runtime-node-resolver/src/resolve_node_version.rs (4)
67-122: LGTM!
124-163: LGTM!
165-254: LGTM!
1-65: The error codesFETCH_NODE_INDEX_FAILEDandDECODE_NODE_INDEX_FAILEDare implementation-specific to pacquet. pnpm does not define these error codes in its node-resolver layer; instead, it delegates fetch and decode operations to a separate@pnpm/crypto.shasums-filemodule. The only error code pnpm's node-resolver exposes at this layer isNODEJS_VERSION_NOT_FOUND, which is raised by the caller, not withinresolve_node_version. Pacquet's custom error codes do not contradict any pnpm public contract and are appropriate for its internal error handling.> Likely an incorrect or invalid review comment.pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs (3)
1-73: LGTM!
75-126: LGTM!
128-195: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
14-46: LGTM!
…o the install chain Ports the three `@pnpm/engine.runtime.*-resolver` packages and the shared `@pnpm/crypto.shasums-file` helper into pacquet, and slots them into the default-resolver chain so `node@runtime:<spec>`, `deno@runtime:<spec>`, and `bun@runtime:<spec>` resolve through pacquet as they do in pnpm. New crates under `pacquet/crates/`: - `crypto-shasums-file` — downloads and decodes `SHASUMS256.txt`, shared by node and bun. Mirrors `FAILED_DOWNLOAD_SHASUM_FILE`, `NODE_INTEGRITY_HASH_NOT_FOUND`, `NODE_MALFORMED_INTEGRITY_HASH`. - `engine-runtime-node-resolver` — `parse_node_specifier`, `get_node_mirror`, `get_node_artifact_address`, `normalize_arch`, `resolve_node_version[s]`, and the `Resolver`-impl entry point. Handles the unofficial-musl mirror fan-out, the `lts` / LTS-codename / channel / range selectors, and the `darwin/arm64 <16 → x64`, `win32/ia32 → x86`, `arm → armv7l` arch quirks. Error codes `NO_OFFLINE_NODEJS_RESOLUTION`, `NODEJS_VERSION_NOT_FOUND`, `INVALID_NODE_RELEASE_CHANNEL` match upstream. - `engine-runtime-deno-resolver` — version selection delegates to the npm resolver; assets come from the GitHub Releases API + per-asset SHA256 sidecars. Windows x64 covers arm64 under emulation. Errors: `DENO_RESOLUTION_FAILURE`, `DENO_MISSING_ASSETS`, `DENO_GITHUB_FAILURE`, `DENO_PARSE_HASH`. - `engine-runtime-bun-resolver` — version selection delegates to npm; assets come from the GitHub-release `SHASUMS256.txt`. `windows` / `aarch64` are normalised to `win32` / `arm64`. Error: `BUN_RESOLUTION_FAILURE`. Wiring (`install_without_lockfile.rs`): chain order is now `npm → git → node → deno → bun`, matching upstream's `resolving/default-resolver/src/index.ts` at 1627943. The npm resolver is shared via `Arc<dyn Resolver>` so the deno/bun resolvers reuse the same metadata cache; a small `ArcResolver` adapter bridges that to `DefaultResolver`'s `Vec<Box<dyn Resolver>>`. Out of scope (called out in code): - `currentPkg && !update` short-circuit isn't restored yet — needs `ResolveOptions::current_pkg` first. The resolver re-fetches the asset list on every install. - `nodeDownloadMirrors` defaults to empty. Wiring it through the config layer is a follow-up.
CI's `Doc` job runs rustdoc with `-D warnings`. Three intra-doc links in this PR's new doc comments tripped it: - `crypto-shasums-file` referenced `pacquet_lockfile::BinaryResolution` but the crate doesn't (and shouldn't) depend on `pacquet-lockfile`. Drop the link and leave the name as plain text. - `engine-runtime-deno-resolver` linked `DefaultResolver` to `pacquet_resolving_default_resolver::DefaultResolver` — same crate- dependency story. Rewrite the prose to mention "the default-resolver chain" without a link. - `engine-runtime-deno-resolver` doc comment on the public `ReadDenoAssetsError` linked to the private free function `read_deno_assets`. Point at the public re-export `DenoResolverError::ReadAssets` instead so the link is reachable from generated docs. - `engine-runtime-bun-resolver` had a redundant explicit link target on `PlatformAssetResolution` (label and target resolve to the same item). Drop the redundant target and reword from `…s` to `… entries` so the link label doesn't carry a stray pluralisation `s`.
…on link The target resolves to the same item as the label (the type is imported into scope further down in the same file), so rustdoc with `-D rustdoc::redundant-explicit-links` rejects the form. Drop the target and let intra-doc resolution pick it up via the existing use.
The pacquet CI runs rustdoc and `cargo dylint` (perfectionist lints) with `-D warnings`, both of which catch issues `just ready` doesn't: - rustdoc on `engine-runtime-node-resolver/lib.rs` reported `parse_node_specifier` / `get_node_mirror` / `get_node_artifact_address` as ambiguous between the module and the same-named function the module re-exports. Disambiguate by appending `()` to the link label so rustdoc resolves to the function. - dylint's `perfectionist::single-letter-closure-param` flagged the `|c|` parameter in `parse_node_specifier::prerelease_channel`. Rename to `next` and break the chain so the body stays readable. - dylint's `perfectionist::prefer-raw-string` flagged the regex literal on `NODE_EXTRAS_IGNORE_PATTERN`. Convert to a raw string so the backslash before `.` reads as the regex escape it is. - dylint's `perfectionist::macro-trailing-comma` flagged the multi-line `matches!` invocation in the shasums-file `NotFound` test. Re-shape with the trailing comma and split across lines.
Five behavioral / hygiene fixes the CodeRabbit review surfaced that
hold up against the upstream pnpm source:
- `crypto-shasums-file`: tighten `is_sha256_hex` from
`is_ascii_hexdigit` (accepts `A-F`) to lowercase-only `0-9a-f`,
matching upstream's `/^[a-f0-9]{64}$/`.
- `engine-runtime-node-resolver/resolve_node_version`: thread
`error_for_status` into the `fetch_all_versions` GET so non-2xx
responses from `index.json` surface as `FetchIndex` rather than
being read into text and decoded as `DecodeIndex`. Matches the
existing convention in `resolving-npm-resolver/fetch_full_metadata`.
- `engine-runtime-node-resolver/resolve_node_version`: introduce
`satisfies_with_prereleases` mirroring the strategy already used in
`resolving-deps-resolver/resolve_peers` so range selectors like
`rc/18` pick up `18.0.0-rc.X` candidates. Upstream's
`semver.maxSatisfying(...)` runs with `includePrerelease: true`;
`node-semver` Rust does not — strip the prerelease suffix on a
failed straight check and retry against the base version.
- `engine-runtime-deno-resolver/read_deno_assets`: same
`error_for_status` fix on the GitHub Releases API call so a 404 or
rate-limit response is a `FetchReleaseIndex` failure, not a JSON
decode error.
- `package-manager/install_without_lockfile`: also drop the
standalone `npm_resolver` Arc binding after the resolve pass.
`drop(resolver)` only releases the `DefaultResolver` chain (one
strong reference); the `npm_resolver` local kept a second strong
reference because the deno- and bun-resolvers were handed clones
of the same `Arc`. Without the explicit drop the packument cache
stays alive through every fetch/import/link, which the comment
above already says we want to avoid.
Plus one test-only addition (a `darwin/arm` passthrough assertion in
`normalize_arch/tests.rs`) that pins the upstream behavior — pnpm
applies the `arm → armv7l` quirk unconditionally, including outside
Linux. Locking that in keeps a well-meaning future "Linux-only"
narrowing from silently diverging from pnpm.
Other CodeRabbit suggestions (propagate caller opts on the
npm-delegation calls, error on empty Deno variants, offline guard in
`resolve_latest_impl`, scope `arm` rewrite to Linux) all reflect
behaviors that *upstream pnpm doesn't have* — adopting any of them
would break the parity contract in
[`pacquet/AGENTS.md`](pacquet/AGENTS.md). Left in place.
…_HASH `fetch_sha256` already guarantees the returned string is a 64-char lower-case hex run, so `decode_hex` cannot fail in practice. Drop the `unwrap_or_default()` fallback (which would silently feed an empty byte slice into the integrity construction and then trip an opaque \`Integrity\` parse error downstream) in favor of an explicit `ParseHash` error, so a future change that loosens `extract_sha256`'s validator surfaces with the right code instead of obscuring the failure shape.
347d69c to
95f85a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@pnpm/engine.runtime.*-resolverpackages (node-resolver,deno-resolver,bun-resolver) and the shared@pnpm/crypto.shasums-filehelper into pacquet.npm → git → node → deno → bun), matching upstream's order atresolving/default-resolver/src/index.ts. Result:node@runtime:<spec>,deno@runtime:<spec>, andbun@runtime:<spec>now resolve through pacquet.Arc<dyn Resolver>so the deno/bun resolvers reuse its packument cache; a smallArcResolveradapter bridges that intoDefaultResolver'sVec<Box<dyn Resolver>>.New crates (under
pacquet/crates/)crypto-shasums-file— downloads and decodesSHASUMS256.txt, shared by node and bun. MirrorsFAILED_DOWNLOAD_SHASUM_FILE,NODE_INTEGRITY_HASH_NOT_FOUND,NODE_MALFORMED_INTEGRITY_HASH.engine-runtime-node-resolver—parse_node_specifier,get_node_mirror,get_node_artifact_address,normalize_arch,resolve_node_version[s], and theResolver-impl entry point. Handles the unofficial-musl mirror fan-out,lts/LTS-codename/channel/range selectors, and thedarwin/arm64 <16 → x64,win32/ia32 → x86,arm → armv7larch quirks. Error codesNO_OFFLINE_NODEJS_RESOLUTION,NODEJS_VERSION_NOT_FOUND,INVALID_NODE_RELEASE_CHANNELmatch upstream.engine-runtime-deno-resolver— version selection delegates to the npm resolver; assets come from the GitHub Releases API + per-asset SHA256 sidecars. Windows x64 covers arm64 under emulation. Errors:DENO_RESOLUTION_FAILURE,DENO_MISSING_ASSETS,DENO_GITHUB_FAILURE,DENO_PARSE_HASH.engine-runtime-bun-resolver— version selection delegates to npm; assets come from the GitHub-releaseSHASUMS256.txt.windows/aarch64are normalised towin32/arm64. Error:BUN_RESOLUTION_FAILURE.Out of scope (called out in code comments)
currentPkg && !updateshort-circuit isn't restored yet — needsResolveOptions::current_pkgfirst. The resolver re-fetches the asset list on every install.nodeDownloadMirrorsdefaults to empty. Wiring it through the config layer is a follow-up.Test plan
cargo nextest run -p pacquet-engine-runtime-node-resolver -p pacquet-engine-runtime-deno-resolver -p pacquet-engine-runtime-bun-resolver -p pacquet-crypto-shasums-file— 40 tests, all passing.just test(full workspace) — 1724 tests, 0 failures, 3 skipped.just check— clean.just lint— clean (clippy--deny warnings).just fmt— applied.node@runtime:/deno@runtime:/bun@runtime:dependency end-to-end through pacquet. The install path is exercised by the existing pkg-manager integration tests, but the runtime branches need a fixture that adds these specifiers to apackage.json.pacquet installof a workspace withengines.runtime: node@22produces the same lockfile shape as pnpm — out of scope for this PR (engines resolution is a separate concern).Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit