feat(pacquet): resolver scaffold + npm version picking#11755
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent 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)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds resolver-base two-seam contracts and verifier types, a DefaultResolver chain, a wanted-dependency parser, registry metadata deprecation support, and npm package picking: an orchestration over in-memory cache, optional disk mirror, and fetcher plus pure packument pickers and comprehensive tests. Also adjusts publishedBy cutoff boundary to be inclusive and wires workspace crate deps. ChangesRegistry metadata & tests
Resolver-base, DefaultResolver, and wanted-dependency parsing
NPM package picking and version selection
Workspace manifest and JS publishedBy boundary
Sequence Diagrams sequenceDiagram
participant Client
participant DefaultResolver as DefaultResolver
participant ResolverA as ResolverA
participant ResolverB as ResolverB
Client->>DefaultResolver: resolve(wanted_dependency)
DefaultResolver->>ResolverA: resolve(wanted_dependency, opts)
ResolverA-->>DefaultResolver: Ok(None)
DefaultResolver->>ResolverB: resolve(wanted_dependency, opts)
ResolverB-->>DefaultResolver: Ok(Some(ResolveResult))
DefaultResolver-->>Client: Ok(ResolveResult)
sequenceDiagram
participant PickPackage as pick_package
participant Cache as InMemoryCache
participant Mirror as OnDiskMirror
participant Fetcher as Fetcher
participant Picker as pick_package_from_meta
PickPackage->>Cache: get(key)
alt cache hit
Cache-->>PickPackage: meta
else offline/prefer_offline read
PickPackage->>Mirror: read(packument)
Mirror-->>PickPackage: meta or error
else fetch path
PickPackage->>Fetcher: fetch_full_metadata_cached
Fetcher-->>PickPackage: meta
PickPackage->>Cache: set(key, meta)
end
PickPackage->>Picker: pick_matching_version(meta, opts)
Picker-->>PickPackage: picked_version
PickPackage-->>Client: PickPackageResult
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11755 +/- ##
==========================================
- Coverage 90.12% 89.64% -0.48%
==========================================
Files 146 151 +5
Lines 16876 17571 +695
==========================================
+ Hits 15209 15752 +543
- Misses 1667 1819 +152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.37524262104,
"stddev": 0.08683046776834709,
"median": 2.39692830224,
"user": 2.6891260000000003,
"system": 3.6050140399999995,
"min": 2.23706149474,
"max": 2.48431587174,
"times": [
2.43580794674,
2.30536122974,
2.44334405074,
2.28453341774,
2.39498566274,
2.48431587174,
2.4687581337399997,
2.23706149474,
2.39887094174,
2.2993874607399998
]
},
{
"command": "pacquet@main",
"mean": 2.3095968343399997,
"stddev": 0.06695026994490309,
"median": 2.2958962722400003,
"user": 2.7338534,
"system": 3.57925094,
"min": 2.2495446697399997,
"max": 2.45011461574,
"times": [
2.2497285957399997,
2.28096525374,
2.45011461574,
2.2495446697399997,
2.3063347257399998,
2.30268232774,
2.29607273974,
2.4069394257399996,
2.29571980474,
2.2578661847399997
]
},
{
"command": "pnpm",
"mean": 4.63662658664,
"stddev": 0.04390894835615857,
"median": 4.62701579824,
"user": 7.8551850000000005,
"system": 4.0854468399999995,
"min": 4.56980973674,
"max": 4.705020745740001,
"times": [
4.705020745740001,
4.6855551567400004,
4.68278204974,
4.61359808074,
4.65437214474,
4.599640039740001,
4.56980973674,
4.60145631574,
4.632039417740001,
4.62199217874
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69832386544,
"stddev": 0.01529857338184942,
"median": 0.6926843787400001,
"user": 0.38560116,
"system": 1.5963023999999997,
"min": 0.6802108467400001,
"max": 0.7263271017400001,
"times": [
0.7161531857400001,
0.7263271017400001,
0.6802108467400001,
0.7029666937400001,
0.6967073917400001,
0.71123092474,
0.6880814057400001,
0.6886613657400001,
0.6880705637400001,
0.68482917474
]
},
{
"command": "pacquet@main",
"mean": 0.72723730614,
"stddev": 0.015263304104285655,
"median": 0.7235843527400001,
"user": 0.39388866,
"system": 1.6119052999999997,
"min": 0.7082683057400001,
"max": 0.7580397567400001,
"times": [
0.7580397567400001,
0.7082683057400001,
0.7180942417400001,
0.7409137517400001,
0.7278770927400001,
0.7204629277400001,
0.7267057777400001,
0.7408269767400001,
0.7183286457400001,
0.7128555847400001
]
},
{
"command": "pnpm",
"mean": 2.44443673794,
"stddev": 0.1209714595248195,
"median": 2.3941316812399998,
"user": 2.98984066,
"system": 2.1966240000000004,
"min": 2.35771581174,
"max": 2.73519135874,
"times": [
2.56916407774,
2.39408199774,
2.35771581174,
2.73519135874,
2.3863297437399997,
2.39477149874,
2.3941813647399997,
2.3743936037399997,
2.47848902774,
2.36004889474
]
}
]
} |
There was a problem hiding this comment.
Pull request overview
Adds the Stage 2 resolver seam foundations to pacquet and ports pnpm’s npm version-picking logic so future protocol resolvers can route and select concrete versions consistently with pnpm.
Changes:
- Introduces a dispatcher-oriented resolver contract (
WantedDependency→ResolveResult) plus a default-resolver chain that selects the first claiming resolver. - Adds a new
parse-wanted-dependencycrate (including an inlinevalidate-npm-package-name@7branch port) to split raw specs into(alias, bare_specifier). - Ports pnpm’s npm resolver version picker (
pickPackage*) into Rust, including cache/disk/network orchestration and publishedBy/minimumReleaseAge behavior; extends registry packument structs to support this.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pacquet/crates/resolving-resolver-base/src/verifier.rs | Extracts/defines verifier-side trait + violation types as a dedicated module. |
| pacquet/crates/resolving-resolver-base/src/resolve.rs | Adds dispatcher-side resolver contract types and the Resolver trait. |
| pacquet/crates/resolving-resolver-base/src/lib.rs | Re-exports both verifier and dispatcher seams from one crate. |
| pacquet/crates/resolving-resolver-base/src/tests.rs | Adds tests for selector weights, defaults, and Resolver dyn dispatch. |
| pacquet/crates/resolving-resolver-base/Cargo.toml | Adds serde dependency needed for new serialized types. |
| pacquet/crates/resolving-parse-wanted-dependency/src/lib.rs | Implements parse_wanted_dependency split logic and public API. |
| pacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rs | Inline port of npm validForOldPackages name validation rules. |
| pacquet/crates/resolving-parse-wanted-dependency/src/tests.rs | Pins parsing/name-validation behavior with unit tests. |
| pacquet/crates/resolving-parse-wanted-dependency/Cargo.toml | New crate manifest for parse-wanted-dependency port. |
| pacquet/crates/resolving-default-resolver/src/lib.rs | Implements DefaultResolver chain and “spec not supported” error. |
| pacquet/crates/resolving-default-resolver/src/tests.rs | Tests chain ordering and spec rendering parity with pnpm. |
| pacquet/crates/resolving-default-resolver/Cargo.toml | New crate manifest for default-resolver dispatcher. |
| pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs | Pure packument version-picking logic + publishedBy filtering port. |
| pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs | Extensive tests for picker behavior parity (latest/tag/range/deprecated/publishedBy). |
| pacquet/crates/resolving-npm-resolver/src/pick_package.rs | Cache/disk/network orchestration port around the pure picker. |
| pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs | Orchestration tests (offline/preferOffline/mirror fast paths/dry_run/etc.). |
| pacquet/crates/resolving-npm-resolver/src/lib.rs | Exposes the new picker/orchestrator APIs from the npm resolver crate. |
| pacquet/crates/registry/src/package.rs | Makes dist_tags public and adds dist_tag() / dist_tags() accessors. |
| pacquet/crates/registry/src/package/tests.rs | Updates fixtures for new deprecated field and dist-tags visibility. |
| pacquet/crates/registry/src/package_version.rs | Adds deprecated: Option<String> to per-version manifests. |
| pacquet/crates/package-manager/src/build_snapshot/tests.rs | Updates snapshot test fixture for new deprecated field. |
| Cargo.toml | Registers new pacquet resolver crates in workspace dependencies. |
| Cargo.lock | Adds lock entries for new crates and updated deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pacquet/crates/registry/src/package.rs (1)
71-88: ⚡ Quick winAdd direct unit tests for the new dist-tag accessors.
dist_tag()/dist_tags()are new public API; please add focused tests for missing-tag behavior and iterator mapping so this contract stays locked.🤖 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/registry/src/package.rs` around lines 71 - 88, Add focused unit tests covering the new public API methods dist_tag and dist_tags: write tests that assert dist_tag("existing") returns the correct &str, dist_tag("missing") returns None, and that dist_tags() yields pairs whose keys and values match the underlying map (e.g., collect into Vec and compare). Target tests to use the Package struct constructor/setup used in other tests to populate dist_tags with multiple entries (including a missing-tag case) and verify iterator mapping preserves each tag->version mapping. Ensure tests live alongside existing package tests and are named to indicate they cover dist-tag accessors.pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
22-28: ⚡ Quick winAvoid
DefaultforWantedDependencybecause it violates its own invariant.The docs require at least one of
aliasorbare_specifier, but#[derive(Default)]creates an invalid empty instance. Prefer removingDefaultand exposing a validating constructor.🤖 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/resolving-resolver-base/src/resolve.rs` around lines 22 - 28, The WantedDependency struct currently derives Default which can create an instance violating the invariant that at least one of alias or bare_specifier must be Some; remove the #[derive(Default)] from WantedDependency and add a validating constructor (e.g., WantedDependency::new or WantedDependency::try_new) that enforces that alias or bare_specifier is present (returning Result or panicking as preferred), update construction sites such as the parse-wanted-dependency port and the deps-resolver manifest reader to use this constructor, and adjust any callsites/tests that relied on Default to construct instances accordingly.
🤖 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/resolving-npm-resolver/src/pick_package_from_meta.rs`:
- Around line 183-190: The check that fast-paths on abbreviated metadata uses an
exclusive cutoff (modified_date match arm with `Some(date) if date < cutoff`)
causing packages with `modified == cutoff` to be rejected; update the condition
in the match (the `modified_date` arm that currently uses `date < cutoff`) to be
inclusive (i.e., accept `date <= cutoff` or equivalent) so that entries with
`modified == cutoff` take the abbreviated-metadata path instead of returning
`PickPackageFromMetaError::MissingTime` for `meta`.
In `@pacquet/crates/resolving-npm-resolver/src/pick_package.rs`:
- Around line 265-267: The in-memory meta cache is keyed only by spec.name which
allows packuments from one registry to be reused for resolves against a
different registry; update the cache key usage to include the registry (e.g.,
combine ctx.meta_cache lookups and inserts to use a composite key derived from
spec.registry or spec.source along with spec.name) so cached entries are scoped
per-registry; change the lookup around ctx.meta_cache.get(&spec.name) and the
corresponding insert/return sites so pick_matching_version_final is always
passed metadata from the correct registry and PickPackageResult.meta refers to
the registry-scoped cached entry (also update the other occurrence at the second
site noted in the comment).
- Around line 356-366: The code treats any on-disk mirror as a successful
fallback even when pick_matching_version_final(&picker_opts, spec, &disk) yields
None, which masks the original fetch error; change the branch that uses
meta_cached_in_store.or_else(|| pkg_mirror.as_deref().and_then(load_meta)) so
that after calling pick_matching_version_final you check whether it returned
Some(picked) and only return Ok(PickPackageResult { meta: disk, picked_package:
picked }) in that case — if pick_matching_version_final returns None, propagate
the original fetch error (the local error variable) instead of returning an
Ok(None)-style result so callers see the real fetch failure.
---
Nitpick comments:
In `@pacquet/crates/registry/src/package.rs`:
- Around line 71-88: Add focused unit tests covering the new public API methods
dist_tag and dist_tags: write tests that assert dist_tag("existing") returns the
correct &str, dist_tag("missing") returns None, and that dist_tags() yields
pairs whose keys and values match the underlying map (e.g., collect into Vec and
compare). Target tests to use the Package struct constructor/setup used in other
tests to populate dist_tags with multiple entries (including a missing-tag case)
and verify iterator mapping preserves each tag->version mapping. Ensure tests
live alongside existing package tests and are named to indicate they cover
dist-tag accessors.
In `@pacquet/crates/resolving-resolver-base/src/resolve.rs`:
- Around line 22-28: The WantedDependency struct currently derives Default which
can create an instance violating the invariant that at least one of alias or
bare_specifier must be Some; remove the #[derive(Default)] from WantedDependency
and add a validating constructor (e.g., WantedDependency::new or
WantedDependency::try_new) that enforces that alias or bare_specifier is present
(returning Result or panicking as preferred), update construction sites such as
the parse-wanted-dependency port and the deps-resolver manifest reader to use
this constructor, and adjust any callsites/tests that relied on Default to
construct instances accordingly.
🪄 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: b2eb45d3-494c-4e7d-876d-311ce3949574
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
Cargo.tomlpacquet/crates/package-manager/src/build_snapshot/tests.rspacquet/crates/registry/src/package.rspacquet/crates/registry/src/package/tests.rspacquet/crates/registry/src/package_version.rspacquet/crates/resolving-default-resolver/Cargo.tomlpacquet/crates/resolving-default-resolver/src/lib.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/pick_package.rspacquet/crates/resolving-npm-resolver/src/pick_package/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/resolving-parse-wanted-dependency/Cargo.tomlpacquet/crates/resolving-parse-wanted-dependency/src/lib.rspacquet/crates/resolving-parse-wanted-dependency/src/tests.rspacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-resolver-base/src/verifier.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). (9)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- 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/build_snapshot/tests.rspacquet/crates/registry/src/package_version.rspacquet/crates/registry/src/package/tests.rspacquet/crates/resolving-parse-wanted-dependency/src/lib.rspacquet/crates/resolving-resolver-base/src/verifier.rspacquet/crates/registry/src/package.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-parse-wanted-dependency/src/tests.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package/tests.rspacquet/crates/resolving-default-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-npm-resolver/src/pick_package.rs
🔇 Additional comments (20)
pacquet/crates/registry/src/package_version.rs (1)
38-49: LGTM!pacquet/crates/registry/src/package.rs (1)
16-16: LGTM!pacquet/crates/package-manager/src/build_snapshot/tests.rs (1)
31-31: LGTM!pacquet/crates/registry/src/package/tests.rs (1)
23-23: LGTM!Also applies to: 44-44, 99-99
pacquet/crates/resolving-resolver-base/src/lib.rs (1)
1-37: LGTM!pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
1-21: LGTM!Also applies to: 29-278
pacquet/crates/resolving-resolver-base/src/verifier.rs (1)
1-116: LGTM!pacquet/crates/resolving-resolver-base/Cargo.toml (1)
16-16: LGTM!pacquet/crates/resolving-resolver-base/src/tests.rs (1)
1-8: LGTM!Also applies to: 107-187
pacquet/crates/resolving-parse-wanted-dependency/Cargo.toml (1)
1-17: LGTM!pacquet/crates/resolving-parse-wanted-dependency/src/lib.rs (1)
1-85: LGTM!pacquet/crates/resolving-parse-wanted-dependency/src/tests.rs (1)
1-140: LGTM!pacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rs (1)
1-97: LGTM!pacquet/crates/resolving-default-resolver/Cargo.toml (1)
1-27: LGTM!pacquet/crates/resolving-default-resolver/src/lib.rs (1)
1-140: LGTM!pacquet/crates/resolving-default-resolver/src/tests.rs (1)
1-167: LGTM!pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
22-49: LGTM!Cargo.toml (1)
16-43: LGTM!pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs (1)
1-390: LGTM!pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs (1)
1-526: LGTM!
Address review comments on #11755: - **Cache parsed `Range` values** (Copilot). `semver_satisfies_loose`, `max_satisfying`, `min_satisfying` reparse `Range` on every call; in hot loops like `prioritize_preferred_versions` that's O(n) repeated parse work per packument. Mirror upstream's `semverRangeCache` with a process-global `DashMap<String, Option<Arc<Range>>>` so each range string is parsed at most once per process. - **Recover from mutex poisoning** in `InMemoryPackageMetaCache` (Copilot). Other pacquet code (e.g. `build_modules.rs`) already uses `unwrap_or_else(|err| err.into_inner())` — the cache is a plain `HashMap` of cloneable values, so no broken invariants outlive a poisoned lock. Swap `.expect(...)` for the recovery pattern. Same for the `warn_missing_time_once` set. - **Use `IndexSet` for the missing-time warning set** (Copilot). Replaces `Vec<String>` membership scan + `Vec::remove(0)` eviction with O(1) `contains` and O(n)-but-cheap-shift `shift_remove_index(0)`. Insertion order is preserved (matching upstream's JS `Set` iteration order), and the cap stays at 1024. - **Scope the in-memory cache key by registry** (coderabbit). The same package name in two registries (private + public, scoped override, etc.) was colliding on `spec.name` alone; a packument fetched against one registry could short-circuit a later resolve against a different registry, returning the wrong metadata. Switch to `<registry>\x00<name>` cache keys. Adds `in_memory_cache_does_not_leak_across_registries` as a regression test pinning the new shape. Two coderabbit suggestions deliberately not applied — both would diverge from upstream pnpm: - `<` vs `<=` in the `modified` shortcut: upstream pnpm uses strict `<` ([pickPackage.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L372) and [pickPackageFromMeta.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackageFromMeta.ts#L47)) to keep the modified-shortcut conservative — if `modified == cutoff`, a version published exactly at the cutoff might exist and the fast path can't be taken. The per-version filter (`<=`) is a separate concern. - Returning the picker's `None` on a fetch failure with a stale mirror: upstream pnpm explicitly returns `pickMatchingVersionFinal(...)`'s result verbatim in the fetch-error catch block ([pickPackage.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431)), including when it's `null`. Re-throwing the fetch error would diverge from upstream's "swallow + serve stale" semantics.
The `minimumReleaseAge` shortcut on abbreviated metadata used strict `<` against the cutoff (and equivalently `>=` to reject), so a packument whose package-level `modified` equalled the cutoff fell off the fast path. That's an unnecessary divergence from the per-version filter in `filterPkgMetadataByPublishDate`, which uses inclusive `<=` to treat a version published exactly at the cutoff as mature. Since `modified` is an upper bound on every version's publish time, `modified == publishedBy` already implies every version passes the per-version `<=` filter. The shortcut can now accept the boundary case directly instead of triggering a full-metadata re-fetch (or a `MISSING_TIME` error when full metadata isn't permitted). Lands in both stacks under the parity rule in pacquet's AGENTS.md: - **TypeScript pnpm** — `resolving/npm-resolver/src/pickPackage.ts` (`maybeUpgradeAbbreviatedMetaForReleaseAge`, and the in-band upgrade branch) and `resolving/npm-resolver/src/pickPackageFromMeta.ts` (the abbreviated `assertMetaHasTime` gate). Each call site flips `>=` to `>` (or `<` to `<=`) and adds a comment pinning the rationale. - **Pacquet** — `pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs` flips `<` to `<=` in the abbreviated branch. Tests: - New `pick_from_meta_modified_shortcut_inclusive_at_cutoff` in pacquet (verified to catch the regression by reverting the comparison locally — the test fails, then passes when restored). - New "use abbreviated metadata when modified date equals publishedBy" in `publishedBy.test.ts` upstream. Changeset bumps `@pnpm/resolving.npm-resolver` and `pnpm` patch. Addresses review feedback from coderabbit on #11755.
Lays down the seam future per-protocol resolvers (npm, git, tarball, local, jsr, runtimes, named-registry, workspace) will plug into. New crates: - `pacquet-resolving-parse-wanted-dependency` ports pnpm's [`@pnpm/resolving.parse-wanted-dependency`](https://github.com/pnpm/pnpm/blob/3687b0e180/resolving/parse-wanted-dependency/src/index.ts) with an inline port of `validate-npm-package-name@7`'s `validForOldPackages` branch. Splits raw manifest specs like `foo@1.2.3`, `@scope/foo@^1`, and `foo@npm:lodash@^4` into `(alias, bareSpecifier)` halves; passes git/tarball/range strings through unchanged. Behavior verified byte-for-byte against the upstream JS for the test corpus. - `pacquet-resolving-default-resolver` ports the protocol-routing dispatcher at the top of [`resolving/default-resolver/src/index.ts`](https://github.com/pnpm/pnpm/blob/3687b0e180/resolving/default-resolver/src/index.ts). `DefaultResolver` walks a `Vec<Box<dyn Resolver>>` chain in order and returns the first resolver's `Ok(Some(_))` claim, falling back to a `SpecNotSupportedByAnyResolverError` (upstream's `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER`). Chain is empty until the per-protocol resolver PRs land. Extends `pacquet-resolving-resolver-base` (previously verifier-only) with the dispatcher contract: `WantedDependency`, `ResolveOptions`, `ResolveResult`, `LatestQuery`, `LatestInfo`, `PreferredVersions` and related selector types, `WorkspacePackage(s)`, weight constants, and the `Resolver` trait + boxed-future return types. Verifier types moved into their own submodule with no public-surface change. Part of #11633 Stage 2 — Tier 1 foundations.
Ports pnpm's pickPackageFromMeta and pickPackage from resolving/npm-resolver/src/ — the pure version-picking surface and the cache+fetch orchestration that wraps it. Plugs into the Stage 2 resolver scaffolding so the chain's npm leg can resolve a registry spec to a concrete (name, version, manifest) triple. Two new modules under `crates/resolving-npm-resolver/src/`: - `pick_package_from_meta` — the pure picker. Ports [`pickPackageFromMeta`](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackageFromMeta.ts), `pickVersionByVersionRange` (with the deprecated-version fallback), `pickLowestVersionByVersionRange`, `filterPkgMetadataByPublishDate` (the `minimumReleaseAge` filter), the `PreferredVersionsPrioritizer` group-by-weight logic, and the error codes upstream surfaces (`ERR_PNPM_UNPUBLISHED_PKG`, `ERR_PNPM_NO_VERSIONS`, `ERR_PNPM_MISSING_TIME`). Introduces `RegistryPackageSpec` / `RegistryPackageSpecType` as the picker's input shape; the parser that produces these is its own port. - `pick_package` — the cache+fetch orchestration. Ports [`pickPackage`](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts) with all four pre-fetch layers (in-memory `PackageMetaCache`, offline / preferOffline / pickLowestVersion disk read, version-spec fast path, publishedBy mtime shortcut), the 304-aware fetch via `fetch_full_metadata_cached`, the `include_latest_tag` runner that picks `max(spec, latest)`, the publishedBy-active fall-back-to- lowest path (`pickRespectingMinReleaseAge`), and the warn-and-skip `ignore_missing_time_field` path. Exposes `PackageMetaCache` as a trait with an `InMemoryPackageMetaCache` default impl plus a `shared_in_memory_cache()` factory. Supporting changes: - `pacquet_registry::PackageVersion` gained `deprecated: Option<String>` (the deprecated-fallback in pickVersionByVersionRange needs it; the field was missing). - `pacquet_registry::Package` gained `dist_tag(tag)` / `dist_tags()` accessors, and `dist_tags` is now `pub` so the publishedBy filter can rebuild a packument with rewritten tags. Documented divergences from upstream: - Pacquet's metadata fetcher returns full metadata only; the abbreviated→full upgrade branches are in place but never fire today. Adding an abbreviated fetcher later is a drop-in. - `dry_run` skips the in-memory cache write but the underlying fetcher still warms the on-disk mirror. Restoring upstream's "no disk side effects on dry-run" needs the fetcher to grow a write-bypass flag. - Upstream's `p-limit(1)` per-mirror serialization is omitted; the atomic rename in `save_meta` covers write safety. Tests: 21 new picker tests + 30 new orchestration tests (mockito-backed). Full workspace nextest run passes (1547 tests). Part of #11633 Stage 2 — npm resolver.
CI's `Doc` job (`RUSTDOCFLAGS="-D warnings"`) and `Dylint` job (`-D perfectionist::derive-ordering`) caught issues missed by `just ready`'s default profile: - Disambiguate two-meaning intra-doc links the `Doc` job rejected as ambiguous: `fetch_full_metadata_cached()` (function vs module) and `crate::pick_package()` (function vs module). - Qualify two unresolved links: `MissingTime` → `PickPackageFromMetaError::MissingTime`, and `LockfileResolution` → `pacquet_lockfile::LockfileResolution` in the resolver-base lib docs. - Reorder four `#[derive(...)]` lists in `resolving-resolver-base/src/resolve.rs` so `Default` sits next to `Debug` in the prefix block (the `prefix_then_alphabetical` Perfectionist rule the `Dylint` job enforces). `Debug, Clone, Default, …` → `Debug, Default, Clone, …` and similar.
CI's `Dylint` job runs additional Perfectionist lints that `just lint` (stable clippy) doesn't see. Fixing the leftover violations: - `single-letter-generic`: rename single-letter type parameters that don't carry meaning by themselves. `C: PackageMetaCache` → `Cache: PackageMetaCache`, the picker-fn `F` → `PickFn` / `PickOne`, the `AsRef<str>` versions slice `S` → `Raw`. - `single-letter-generic` (parameter form): rename `a` / `b` in `pick_max` to `lhs` / `rhs`. - `arc-clone-not-direct`: swap `meta.mutex.clone()` for `Arc::clone(&meta.mutex)` so the refcount bump is visible at the call site (matches the project style guide). - `unicode-ellipsis-in-comments`: replace `…` with `...` in the default-resolver test that landed in the scaffold commit. - `prefer-raw-string`: rewrite four `"\"…\" isn't supported…"` literals in the default-resolver tests as `r#""…" isn't…"#` raw strings.
CI's `Format` job (`cargo fmt --check`) and `Dylint` job (`-D perfectionist::trailing-comma-in-single-line-macro`) caught two spots where a multi-line `assert!` / `assert_eq!` collapsed to a single line during the last fmt pass and kept its trailing comma. Drop the trailing comma so both checks are happy.
…param Dylint's `perfectionist::single-letter-generic` rule also catches single-letter closure parameters. `|c|` in `is_url_friendly` → `|ch|` so the rule passes.
Address review comments on #11755: - **Cache parsed `Range` values** (Copilot). `semver_satisfies_loose`, `max_satisfying`, `min_satisfying` reparse `Range` on every call; in hot loops like `prioritize_preferred_versions` that's O(n) repeated parse work per packument. Mirror upstream's `semverRangeCache` with a process-global `DashMap<String, Option<Arc<Range>>>` so each range string is parsed at most once per process. - **Recover from mutex poisoning** in `InMemoryPackageMetaCache` (Copilot). Other pacquet code (e.g. `build_modules.rs`) already uses `unwrap_or_else(|err| err.into_inner())` — the cache is a plain `HashMap` of cloneable values, so no broken invariants outlive a poisoned lock. Swap `.expect(...)` for the recovery pattern. Same for the `warn_missing_time_once` set. - **Use `IndexSet` for the missing-time warning set** (Copilot). Replaces `Vec<String>` membership scan + `Vec::remove(0)` eviction with O(1) `contains` and O(n)-but-cheap-shift `shift_remove_index(0)`. Insertion order is preserved (matching upstream's JS `Set` iteration order), and the cap stays at 1024. - **Scope the in-memory cache key by registry** (coderabbit). The same package name in two registries (private + public, scoped override, etc.) was colliding on `spec.name` alone; a packument fetched against one registry could short-circuit a later resolve against a different registry, returning the wrong metadata. Switch to `<registry>\x00<name>` cache keys. Adds `in_memory_cache_does_not_leak_across_registries` as a regression test pinning the new shape. Two coderabbit suggestions deliberately not applied — both would diverge from upstream pnpm: - `<` vs `<=` in the `modified` shortcut: upstream pnpm uses strict `<` ([pickPackage.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L372) and [pickPackageFromMeta.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackageFromMeta.ts#L47)) to keep the modified-shortcut conservative — if `modified == cutoff`, a version published exactly at the cutoff might exist and the fast path can't be taken. The per-version filter (`<=`) is a separate concern. - Returning the picker's `None` on a fetch failure with a stale mirror: upstream pnpm explicitly returns `pickMatchingVersionFinal(...)`'s result verbatim in the fetch-error catch block ([pickPackage.ts](https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431)), including when it's `null`. Re-throwing the fetch error would diverge from upstream's "swallow + serve stale" semantics.
The `minimumReleaseAge` shortcut on abbreviated metadata used strict `<` against the cutoff (and equivalently `>=` to reject), so a packument whose package-level `modified` equalled the cutoff fell off the fast path. That's an unnecessary divergence from the per-version filter in `filterPkgMetadataByPublishDate`, which uses inclusive `<=` to treat a version published exactly at the cutoff as mature. Since `modified` is an upper bound on every version's publish time, `modified == publishedBy` already implies every version passes the per-version `<=` filter. The shortcut can now accept the boundary case directly instead of triggering a full-metadata re-fetch (or a `MISSING_TIME` error when full metadata isn't permitted). Lands in both stacks under the parity rule in pacquet's AGENTS.md: - **TypeScript pnpm** — `resolving/npm-resolver/src/pickPackage.ts` (`maybeUpgradeAbbreviatedMetaForReleaseAge`, and the in-band upgrade branch) and `resolving/npm-resolver/src/pickPackageFromMeta.ts` (the abbreviated `assertMetaHasTime` gate). Each call site flips `>=` to `>` (or `<` to `<=`) and adds a comment pinning the rationale. - **Pacquet** — `pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs` flips `<` to `<=` in the abbreviated branch. Tests: - New `pick_from_meta_modified_shortcut_inclusive_at_cutoff` in pacquet (verified to catch the regression by reverting the comparison locally — the test fails, then passes when restored). - New "use abbreviated metadata when modified date equals publishedBy" in `publishedBy.test.ts` upstream. Changeset bumps `@pnpm/resolving.npm-resolver` and `pnpm` patch. Addresses review feedback from coderabbit on #11755.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs (1)
205-224: ⚡ Quick winStrengthen the selector test assertion to verify the actual winner.
assert!(...is_some())won’t catch regressions in preferred-selector weighting behavior. This test should assert the picked version explicitly (or rename the test if it’s only a crash-safety check).♻️ Proposed tightening
- assert!(pick_version_by_version_range(&opts).is_some()); + assert_eq!( + pick_version_by_version_range(&opts).as_deref(), + Some("1.0.0"), + "tag selector should bias toward the tagged in-range version", + );🤖 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/resolving-npm-resolver/src/pick_package_from_meta/tests.rs` around lines 205 - 224, The test preferred_versions_tag_selector_wins currently only asserts pick_version_by_version_range(&opts).is_some(); change it to assert the exact picked version to prevent regressions: call pick_version_by_version_range(&opts), unwrap the Some result and compare it to the expected in-range winner (the "latest" tag -> "1.2.0" in the test data). Update the assertion to check the returned version string/value equals "1.2.0" (using the same return shape as pick_version_by_version_range) so the test validates selector weighting, referencing the test function preferred_versions_tag_selector_wins, the VersionSelectors/VersionSelectorEntry/VersionSelectorType setup, and pick_version_by_version_range/PickVersionByVersionRangeOptions.
🤖 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/resolving-npm-resolver/src/pick_package_from_meta/tests.rs`:
- Around line 205-224: The test preferred_versions_tag_selector_wins currently
only asserts pick_version_by_version_range(&opts).is_some(); change it to assert
the exact picked version to prevent regressions: call
pick_version_by_version_range(&opts), unwrap the Some result and compare it to
the expected in-range winner (the "latest" tag -> "1.2.0" in the test data).
Update the assertion to check the returned version string/value equals "1.2.0"
(using the same return shape as pick_version_by_version_range) so the test
validates selector weighting, referencing the test function
preferred_versions_tag_selector_wins, the
VersionSelectors/VersionSelectorEntry/VersionSelectorType setup, and
pick_version_by_version_range/PickVersionByVersionRangeOptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50514015-f9d0-4327-8aec-fd46c3484604
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.changeset/minimum-release-age-modified-shortcut-inclusive.mdCargo.tomlpacquet/crates/package-manager/src/build_snapshot/tests.rspacquet/crates/registry/src/package.rspacquet/crates/registry/src/package/tests.rspacquet/crates/registry/src/package_version.rspacquet/crates/resolving-default-resolver/Cargo.tomlpacquet/crates/resolving-default-resolver/src/lib.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-npm-resolver/Cargo.tomlpacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/pick_package.rspacquet/crates/resolving-npm-resolver/src/pick_package/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/resolving-parse-wanted-dependency/Cargo.tomlpacquet/crates/resolving-parse-wanted-dependency/src/lib.rspacquet/crates/resolving-parse-wanted-dependency/src/tests.rspacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-resolver-base/src/verifier.rsresolving/npm-resolver/src/pickPackage.tsresolving/npm-resolver/src/pickPackageFromMeta.tsresolving/npm-resolver/test/publishedBy.test.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/minimum-release-age-modified-shortcut-inclusive.md
- pacquet/crates/package-manager/src/build_snapshot/tests.rs
- pacquet/crates/resolving-default-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: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (rely on hoisting), and use a single options object for functions with more than two or three arguments
Sort imports in three groups: standard libraries, external dependencies (alphabetically), then relative imports
Write code that explains itself through clear naming and types — do not write comments that merely restate what the code already says; use comments only for non-obvious reasons, hidden invariants, or workarounds
Files:
resolving/npm-resolver/test/publishedBy.test.tsresolving/npm-resolver/src/pickPackageFromMeta.tsresolving/npm-resolver/src/pickPackage.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use util.types.isNativeError() instead of instanceof Error when type-checking errors in Jest tests, as instanceof checks can fail across VM realms
Files:
resolving/npm-resolver/test/publishedBy.test.ts
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/registry/src/package_version.rspacquet/crates/registry/src/package.rspacquet/crates/resolving-parse-wanted-dependency/src/lib.rspacquet/crates/resolving-default-resolver/src/lib.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-resolver-base/src/verifier.rspacquet/crates/registry/src/package/tests.rspacquet/crates/resolving-parse-wanted-dependency/src/tests.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/pick_package.rspacquet/crates/resolving-npm-resolver/src/pick_package/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
resolving/npm-resolver/test/publishedBy.test.tsresolving/npm-resolver/src/pickPackageFromMeta.tsresolving/npm-resolver/src/pickPackage.ts
🔇 Additional comments (24)
resolving/npm-resolver/test/publishedBy.test.ts (1)
195-217: LGTM!pacquet/crates/resolving-npm-resolver/Cargo.toml (1)
21-23: LGTM!pacquet/crates/resolving-parse-wanted-dependency/Cargo.toml (1)
1-17: LGTM!pacquet/crates/registry/src/package_version.rs (1)
38-49: LGTM!pacquet/crates/resolving-resolver-base/Cargo.toml (1)
16-16: LGTM!Cargo.toml (1)
16-44: LGTM!resolving/npm-resolver/src/pickPackageFromMeta.ts (1)
47-57: LGTM!resolving/npm-resolver/src/pickPackage.ts (1)
372-377: LGTM!Also applies to: 474-479
pacquet/crates/registry/src/package.rs (1)
16-16: LGTM!Also applies to: 71-88
pacquet/crates/resolving-parse-wanted-dependency/src/lib.rs (1)
1-85: LGTM!pacquet/crates/resolving-default-resolver/src/lib.rs (1)
1-140: LGTM!pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
1-278: LGTM!pacquet/crates/resolving-resolver-base/src/verifier.rs (1)
1-117: LGTM!pacquet/crates/registry/src/package/tests.rs (1)
23-23: LGTM!Also applies to: 44-44, 99-99
pacquet/crates/resolving-parse-wanted-dependency/src/tests.rs (1)
1-140: LGTM!pacquet/crates/resolving-resolver-base/src/lib.rs (1)
1-37: LGTM!pacquet/crates/resolving-default-resolver/src/tests.rs (1)
1-168: LGTM!pacquet/crates/resolving-parse-wanted-dependency/src/validate_npm_package_name.rs (1)
1-97: LGTM!pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs (1)
1-204: LGTM!Also applies to: 225-550
pacquet/crates/resolving-resolver-base/src/tests.rs (1)
1-188: LGTM!pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
22-23: LGTM!Also applies to: 40-49
pacquet/crates/resolving-npm-resolver/src/pick_package.rs (1)
1-685: LGTM!pacquet/crates/resolving-npm-resolver/src/pick_package/tests.rs (1)
1-493: LGTM!pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs (1)
1-620: LGTM!
The npm registry occasionally serves `"deprecated": false` (a boolean) for never-deprecated versions, even though the upstream TypeScript type declares the field as a string. JavaScript silently stores the boolean and the upstream `if (info.deprecated)` truthiness check happens to do the right thing across both shapes. Rust serde is strict, so a real install of any workload containing react/react-dom/scheduler/create-react-class tripped the verifier: > Failed to decode metadata from https://registry.npmjs.org/react: > invalid type: boolean `false`, expected a string at line 1 column 324915 Route the field through a custom deserializer that normalizes the wire shape to `Option<String>`: - `string s` → `Some(s)` (deprecation reason). - `false` → `None` (not deprecated). - `true` → `Some("")` (deprecated without a recorded reason). - missing → `None` (via the existing `#[serde(default)]`). `#[serde(skip_serializing_if = "Option::is_none")]` stays, so round-trips through pacquet's own mirror don't emit a synthetic `"deprecated": false` either. Caught by running `pacquet install --frozen-lockfile` against the integrated-benchmark fixture (`pacquet/tasks/integrated-benchmark/src/fixtures/`), which the picker port commit started failing on CI. With this fix the install succeeds end-to-end (exit 0). Three regression tests pinning the wire-shape: `deprecated:false`, `deprecated:true`, and the regular reason-string case. All three verified by `cargo nextest run -p pacquet-registry deprecated`.
- `cached_range`: use `entry.value().clone()` to make the `dashmap::Ref` → `Option<Arc<Range>>` projection explicit, with a comment explaining the auto-deref + Clone the previous form was relying on. No behavior change. - `WantedDependency` doc: clarify that `Default` is only kept so call sites can use the `..WantedDependency::default()` spread syntax; the at-least-one-of-(alias, bare_specifier) invariant is upheld by construction sites, not by the type system, and resolvers walking an empty WantedDependency should return `Ok(None)` to let the chain fall through to the SpecNotSupported terminal.
Dylint's `perfectionist::single-letter-generic` rejected the conventional serde names (`D` for Deserializer, `E` for Error, `D2` for the nested Deserializer in `visit_some`) in the custom `deserialize_deprecated_field` helper. Rename to `Deser`, `Err`, and `Nested` respectively.
Summary
Two commits that together cover the Stage 2 resolver seam and the npm leg's version-picking surface. First piece of Stage 2 — Tier 1 foundations.
Commit 1 —
feat(pacquet): scaffold Stage 2 resolver seamLays down the seam future per-protocol resolvers (npm, git, tarball, local, jsr, runtimes, named-registry, workspace) plug into.
New crates:
pacquet-resolving-parse-wanted-dependency— ports pnpm's@pnpm/resolving.parse-wanted-dependencywith an inline port ofvalidate-npm-package-name@7'svalidForOldPackagesbranch. Splits raw manifest specs likefoo@1.2.3,@scope/foo@^1, and the npm-alias formfoo@npm:lodash@^4into(alias, bareSpecifier)halves; passes git/tarball/range strings through unchanged. Behavior verified byte-for-byte against the upstream JS function for the test corpus.pacquet-resolving-default-resolver— ports the protocol-routing dispatcher at the top ofresolving/default-resolver/src/index.ts.DefaultResolverwalks aVec<Box<dyn Resolver>>chain in declaration order and returns the first resolver'sOk(Some(_))claim; falls back toSpecNotSupportedByAnyResolverError(upstream'sSPEC_NOT_SUPPORTED_BY_ANY_RESOLVER). Chain is empty until the per-protocol resolver PRs land.Extended crate:
pacquet-resolving-resolver-base— previously verifier-only. Adds the dispatcher contract:WantedDependency,ResolveOptions,ResolveResult,LatestQuery,LatestInfo,PreferredVersions+ selector types,WorkspacePackage(s), weight constants (DIRECT_DEP_SELECTOR_WEIGHT,EXISTING_VERSION_SELECTOR_WEIGHT), and theResolvertrait with boxed-future return types matching the existingResolutionVerifiershape. Verifier types moved into their own submodule with no public-surface change.Commit 2 —
feat(resolving-npm-resolver): port pickPackage / pickPackageFromMetaPorts pnpm's
pickPackageFromMeta.tsandpickPackage.ts— the pure version-picking surface and the cache+fetch orchestration that wraps it. This is the piece that lets the chain's npm leg resolve a registry spec to a concrete(name, version, manifest)triple.Two new modules under
crates/resolving-npm-resolver/src/:pick_package_from_meta— pure picker.pickPackageFromMeta,pickVersionByVersionRange(with deprecated-version fallback),pickLowestVersionByVersionRange,filterPkgMetadataByPublishDate(theminimumReleaseAgefilter),PreferredVersionsPrioritizer's group-by-weight logic, and the error codes upstream surfaces (ERR_PNPM_UNPUBLISHED_PKG,ERR_PNPM_NO_VERSIONS,ERR_PNPM_MISSING_TIME). IntroducesRegistryPackageSpec/RegistryPackageSpecTypeas the picker's input shape.pick_package— cache+fetch orchestration. All four pre-fetch layers (in-memoryPackageMetaCache, offline / preferOffline / pickLowestVersion disk read, version-spec fast path, publishedBy mtime shortcut), 304-aware fetch viafetch_full_metadata_cached, theinclude_latest_tagrunner that picksmax(spec, latest), the publishedBy-active fall-back-to-lowest path (pickRespectingMinReleaseAge), and the warn-and-skipignore_missing_time_fieldpath. ExposesPackageMetaCacheas a trait with anInMemoryPackageMetaCachedefault impl plus ashared_in_memory_cache()factory.Supporting changes to
pacquet-registry:PackageVersiongaineddeprecated: Option<String>(the deprecated-fallback inpickVersionByVersionRangeneeds it).Packagegaineddist_tag(tag)/dist_tags()accessors, anddist_tagsis nowpubso the publishedBy filter can rebuild a packument with rewritten tags.Scope notes
DependencyManifestis aserde_json::Valuealias for now. Swap once a typed in-memory manifest lands.ResolveError = Box<dyn Error + Send + Sync>today so each resolver crate can keep its own typed error enum without forcing a shared enum prematurely.Documented divergences in
pick_packagedry_runskips the in-memory cache write, but the underlyingfetch_full_metadata_cachedstill warms the on-disk mirror. Restoring upstream's "no disk side effects on dry-run" needs the fetcher to grow a write-bypass flag.p-limit(1)per-mirror serialization is omitted; the atomic rename insave_metacovers write safety.Test plan
cargo check -p pacquet-resolving-parse-wanted-dependency/-p pacquet-resolving-resolver-base/-p pacquet-resolving-default-resolver/-p pacquet-resolving-npm-resolvercargo nextest run -p pacquet-resolving-npm-resolver— 105 tests pass (includes 21 new picker tests + 30 new orchestration tests, mockito-backed)just check,just lint,just test— 1547 tests across the workspace, all passcargo fmt --check,taplo format --check— cleanparse_wanted_dependencyverified by running the upstream JS function on the same input corpus and confirming output matches byte-for-byte.Part of #11633.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores