Skip to content

feat(pacquet): wire NpmResolver into install; fix(pick-registry) unscoped npm-alias routing#11760

Merged
zkochan merged 11 commits into
mainfrom
feat/11756-1
May 20, 2026
Merged

feat(pacquet): wire NpmResolver into install; fix(pick-registry) unscoped npm-alias routing#11760
zkochan merged 11 commits into
mainfrom
feat/11756-1

Conversation

@zkochan

@zkochan zkochan commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

Two changes ship together: the bulk is the pacquet refactor described in #11756, plus a TypeScript-side fix to @pnpm/config.pick-registry-for-package that surfaced during review.

Pacquet — wire NpmResolver into install (Phases A/B/C of #11756)

  • Phase A. New parse_bare_specifier.rs and npm_resolver.rs in pacquet-resolving-npm-resolver. NpmResolver implements the Resolver trait: parses the bare specifier (including npm-alias npm:@scope/name@<spec> and tarball-URL forms — with prefix-anchored name validation), picks a version via pick_package, surfaces minimumReleaseAge violations inline via detect_min_release_age_violation. workspace: specs decline so the chain falls through. published_by / published_by_exclude / dry_run added to ResolveOptions.
  • Phase B. install_without_lockfile.rs constructs an NpmResolver at install entry from the config-derived registries map and an InMemoryPackageMetaCache that's shared across the resolve pass and dropped before the install pass.
  • Phase C. New pacquet-resolving-deps-resolver crate exposes resolve_dependency_tree: a flat name@version-keyed package map with parent-child edges, concurrent sibling resolution via try_join_all, per-id dedup gate. install_package_from_registry.rs no longer calls Package::fetch_from_registry / Package::pinned_version; it takes a pre-resolved ResolveResult and reads tarball URL + integrity off LockfileResolution::Tarball.

Additional behaviors landed during review:

  • minimumReleaseAge policy in the resolve pass. Previously only enforced by the lockfile-verification gate; the no-lockfile resolve pass now derives published_by and the exclude policy from Config so resolver-time picks match the configured policy.
  • SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER surfaces correctly. resolve_dependency_tree now returns a typed error when the chain returns Ok(None) — silently dropping the edge would leave installs missing transitive deps. Mirrors upstream's default-resolver error shape.
  • Per-package progress events. InstallPackageFromRegistry takes a first_visit: bool; pnpm:progress resolved / pnpm:progress imported plus the tarball download fire once per (name, version), while the per-parent symlink_package runs on every edge. Matches upstream's per-package (not per-edge) reporter contract.
  • Windows symlink race fix. ResolvedPackages is now DashMap<String, watch::Sender<bool>>; the first writer signals completion after import_indexed_dir, so a second visitor's symlink_package (which may fall back to a Windows junction requiring an existing target) doesn't race ahead of the materialization. A dropped first-writer task surfaces as a typed FirstWriterAborted error.
  • Scope routing. pick_registry_for_package is now bareSpecifier-aware so an entry like "foo": "npm:@acme/bar@^1" routes through registries[@acme].

TS — @pnpm/config.pick-registry-for-package unscoped-target fix

A separate bug surfaced during the scope-routing port: pickRegistryForPackage('@private/foo', 'npm:lodash@^1') was routing through registries['@private'], even though lodash is unscoped and doesn't live on the @private registry. getScope now returns null in the npm-alias branch when the alias target is unscoped (instead of falling through to the local pkgName's scope). Changeset is in .changeset/pick-registry-unscoped-npm-alias.md (patch bump for @pnpm/config.pick-registry-for-package and pnpm). Added matching tests on both the TS and pacquet sides.

Out of scope (left as #11756 follow-ups)

  • Preferred-versions harvesting from the lockfile (Phase D).
  • Install-side aggregation of policy_violation from the tree (Phase E) — the resolver attaches them per-pick already, but the install layer doesn't yet collect or fail on them.
  • Other-protocol resolvers (git, tarball, workspace, jsr, named-registry, …) — NpmResolver is the only chain entry today; once a second resolver lands, DefaultResolver will get wired in too.
  • Full parseBareSpecifier.test.ts corpus port — the parser tests pacquet ships cover the cases the install path exercises; remaining corpus items land alongside Phase F.

Closes part of #11756.

Test plan

  • cargo nextest run -p pacquet-resolving-npm-resolver — 397 tests (parser, resolver, picker, named-registry routing, verifier, trust checks).
  • cargo nextest run -p pacquet-resolving-deps-resolver — 3 tests (walk, dedup, decline → SpecNotSupported).
  • cargo nextest run -p pacquet-package-manager — 264 tests (including 3 mock-registry install-package-from-registry integration tests covering the first_visit=true/false contract).
  • cargo nextest run -p pacquet-cli --test add — CLI add tests pass end-to-end through the new chain against the mock registry.
  • pnpm --filter @pnpm/config.pick-registry-for-package test — 3 TS tests (existing case + unscoped-target fix + scoped-target wins).
  • just test — full workspace suite passes.
  • just lint (cargo clippy --workspace --all-targets -- --deny warnings) — clean.
  • RUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-items — clean.
  • RUSTFLAGS=-D warnings cargo dylint --all -- --all-targets --workspace — clean (Perfectionist lints).
  • cargo fmt --check and taplo format — clean.

Written by an agent (Claude Code, claude-opus-4-7).

Implements phases A, B, and C of #11756: ports `parseBareSpecifier`,
adds the `NpmResolver` struct implementing the `Resolver` trait, and
refactors the no-lockfile install into a two-pass resolve-then-install
shape backed by a minimum-slice port of `resolveDependencyTree`.
`InstallPackageFromRegistry` now takes a pre-resolved `ResolveResult`
instead of `(name, version_range)` and drops the legacy
`Package::fetch_from_registry` + `Package::pinned_version` calls — all
npm-shaped resolution flows through the new chain.

---
Written by an agent (Claude Code, claude-opus-4-7).
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

<review_stack_artifact>

</review_stack_artifact>

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: introducing NpmResolver into the install workflow and fixing npm-alias registry routing behavior.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/11756-1

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.11      8.4±0.26ms   518.7 KB/sec    1.00      7.5±0.31ms   577.7 KB/sec

@codecov-commenter

codecov-commenter commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.25287% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.93%. Comparing base (3a54205) to head (3f7d844).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
.../crates/resolving-npm-resolver/src/npm_resolver.rs 92.73% 13 Missing ⚠️
...resolving-npm-resolver/src/parse_bare_specifier.rs 91.80% 10 Missing ⚠️
...ckage-manager/src/install_package_from_registry.rs 91.25% 7 Missing ⚠️
...lving-deps-resolver/src/resolve_dependency_tree.rs 96.19% 4 Missing ⚠️
...es/package-manager/src/install_without_lockfile.rs 99.06% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11760      +/-   ##
==========================================
+ Coverage   89.64%   89.93%   +0.28%     
==========================================
  Files         151      154       +3     
  Lines       17571    18007     +436     
==========================================
+ Hits        15752    16194     +442     
+ Misses       1819     1813       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.068 ± 0.076 1.947 2.186 1.02 ± 0.06
pacquet@main 2.034 ± 0.088 1.933 2.216 1.00
pnpm 4.205 ± 0.055 4.128 4.292 2.07 ± 0.09
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.0683628903600004,
      "stddev": 0.0758818815224874,
      "median": 2.07347607196,
      "user": 2.93147474,
      "system": 2.20464794,
      "min": 1.9467888069600001,
      "max": 2.18564807196,
      "times": [
        1.9467888069600001,
        1.9855246559600002,
        2.18564807196,
        2.10464158196,
        2.13020075296,
        2.02447701096,
        2.01243372296,
        2.07364059796,
        2.1469621559600003,
        2.0733115459600002
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.0342915701599997,
      "stddev": 0.08774039120978055,
      "median": 2.00669484546,
      "user": 2.9323489400000002,
      "system": 2.1942944399999997,
      "min": 1.9325673949600002,
      "max": 2.2155763689600003,
      "times": [
        2.08906989796,
        1.9976442159600003,
        2.2155763689600003,
        1.95133647396,
        1.9869398989600002,
        2.01574547496,
        2.07073518996,
        2.11522205196,
        1.9325673949600002,
        1.96807873396
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.205331854860001,
      "stddev": 0.05520632780717547,
      "median": 4.204450571959999,
      "user": 7.94792024,
      "system": 2.5200177399999992,
      "min": 4.12773910596,
      "max": 4.29188418896,
      "times": [
        4.23697348296,
        4.24985584296,
        4.26074321696,
        4.14762991896,
        4.229353071959999,
        4.166532762959999,
        4.29188418896,
        4.16305888496,
        4.179548071959999,
        4.12773910596
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 513.1 ± 52.6 488.4 660.1 1.02 ± 0.11
pacquet@main 500.7 ± 22.2 485.6 560.5 1.00
pnpm 2256.0 ± 39.8 2187.8 2319.6 4.51 ± 0.22
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.5131324011000001,
      "stddev": 0.05257262502072029,
      "median": 0.49572606580000006,
      "user": 0.38242316,
      "system": 0.9291861000000001,
      "min": 0.48839079730000007,
      "max": 0.6601373853,
      "times": [
        0.6601373853,
        0.5228592533,
        0.48839079730000007,
        0.49574406430000006,
        0.49235684430000004,
        0.49069214130000005,
        0.4896783693,
        0.49862068130000003,
        0.4971364073,
        0.49570806730000005
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.500704733,
      "stddev": 0.02220709241440307,
      "median": 0.49197871630000006,
      "user": 0.38110285999999993,
      "system": 0.9350179000000001,
      "min": 0.48555835830000005,
      "max": 0.5604783963,
      "times": [
        0.5604783963,
        0.49614978130000004,
        0.49042583030000003,
        0.49828163630000005,
        0.48555835830000005,
        0.49048331030000003,
        0.49161744130000007,
        0.48974897330000006,
        0.5119636113,
        0.49233999130000006
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.2560305658,
      "stddev": 0.03975500480938133,
      "median": 2.2534958703,
      "user": 2.98918916,
      "system": 1.2860833999999999,
      "min": 2.1878246393,
      "max": 2.3196055443000003,
      "times": [
        2.2399799213000002,
        2.3196055443000003,
        2.2936155903000004,
        2.1878246393,
        2.2649587263,
        2.2518250663000003,
        2.2930349243,
        2.2081196053000003,
        2.2551666743000003,
        2.2461749663000004
      ]
    }
  ]
}

CI's Doc job runs `cargo doc --no-deps --workspace --document-private-items`
under `RUSTDOCFLAGS=-D warnings`; CI's Dylint job runs Perfectionist
lints under `-D warnings` too. Neither surfaces in `just ready`. Fix
the items the post-push run flagged:

* Disambiguate fn-vs-module rustdoc links with `name()` syntax.
* Resolve the `Resolver` link in `resolving-deps-resolver` against
  `pacquet_resolving_resolver_base::Resolver`.
* Reorder the `ResolvedTree` derives to `Debug, Default, Clone`.
* Rename the resolver-chain generic from `R` to `Chain` and the dist-
  tag validator's `s` to `selector`, matching the existing
  `Cache: PackageMetaCache` precedent.
* Add a trailing comma to the multi-line `assert!` body.

---
Written by an agent (Claude Code, claude-opus-4-7).
@zkochan zkochan marked this pull request as ready for review May 20, 2026 09:49
Copilot AI review requested due to automatic review settings May 20, 2026 09:49
@coderabbitai coderabbitai Bot added area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies. area: resolution labels May 20, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs (1)

87-110: ⚡ Quick win

Add a rejection test for mismatched tarball basename.

Please add a case like .../foo/-/bar-1.0.0.tgz and assert None to lock in strict tarball parsing and prevent future regressions.

🤖 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/parse_bare_specifier/tests.rs`
around lines 87 - 110, Add a unit test that ensures parse_bare_specifier rejects
tarball URLs whose basename doesn't match the package path: create a test (e.g.,
tarball_url_with_mismatched_basename_declines) that calls parse_bare_specifier
with a URL like "https://registry.npmjs.org/foo/-/bar-1.0.0.tgz" (and similarly
for a scoped package variant if desired) and assert the result is None; place it
alongside the existing tests in parse_bare_specifier/tests.rs to lock in strict
tarball basename validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pacquet/crates/package-manager/src/install_without_lockfile.rs`:
- Around line 327-330: The current fast-path early return when
ctx.resolved_packages.insert(virtual_store_name.clone()) is false prevents
creating per-parent alias links (node_modules/<alias>) on duplicate
{name,version} hits; keep the dedupe set insertion logic but remove the
unconditional return so that when insert returns false you still run the
parent-local symlink/alias creation path (e.g., call the same
alias/materialization logic used for first-writer cases), guarded by a check
that waits or observes any in-flight first-writer completion (e.g., a
"first-writer" flag or completion future) to avoid races. In practice: retain
the insert check to avoid redundant materialization work, but if insert() is
false, call the alias linking routine used below (the code that creates
node_modules/<alias> for the parent) and short-circuit only the heavy
materialization when an in-flight writer is detected/completed.

In `@pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`:
- Around line 15-18: Introduce a strong newtype for the branded package
identifier (e.g., PackageId(String)) and replace raw String usages with it:
change DirectDep.id and ResolvedPackage.id to PackageId, and change
ResolvedTree.packages from HashMap<String, ResolvedPackage> to
HashMap<PackageId, ResolvedPackage>. Implement common traits (Clone, Debug, Eq,
PartialEq, Hash, Ord) and conversion helpers (From<String>/FromStr, Display) and
derive or implement Serialize/Deserialize so the map still serializes as string
keys; update any constructors, lookups, and tests to construct/compare via
PackageId instead of raw strings. Ensure all uses in files referencing
DirectDep, ResolvedPackage, and ResolvedTree are updated to the new type to
restore type-safety.

In `@pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs`:
- Line 268: The current line that assigns manifest using
serde_json::to_value(picked).ok() swallows serialization errors; replace this
with error propagation so failures become a ResolveError instead of None. Change
the manifest assignment to call serde_json::to_value(picked) and map or convert
the serde_json::Error into your ResolveError variant (e.g., using map_err or the
? operator) so the function returns Err(ResolveError::...) on failure; reference
the manifest variable, serde_json::to_value(picked) call, and the ResolveError
type to implement the proper conversion.

In `@pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs`:
- Around line 169-176: The code currently slices the tarball filename into a
version using scopeless_name_length without verifying the filename actually
begins with "<scopeless-name>-"; update the logic in parse_bare_specifier.rs to
first compute the scopeless name (the last segment of name using
scopeless_name_length), then assert that path_with_no_ext starts with that
scopeless name plus a '-' separator (e.g. starts_with(format!("{}-",
scopeless_name)) or equivalent slice comparison); if the prefix check fails
return None, otherwise extract the version substring after that prefix and
continue to Version::parse(version).ok()? before returning Some(NpmTarballUrl {
name, version: version.to_string() }).

---

Nitpick comments:
In `@pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs`:
- Around line 87-110: Add a unit test that ensures parse_bare_specifier rejects
tarball URLs whose basename doesn't match the package path: create a test (e.g.,
tarball_url_with_mismatched_basename_declines) that calls parse_bare_specifier
with a URL like "https://registry.npmjs.org/foo/-/bar-1.0.0.tgz" (and similarly
for a scoped package variant if desired) and assert the result is None; place it
alongside the existing tests in parse_bare_specifier/tests.rs to lock in strict
tarball basename validation.
🪄 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: 2c924d83-145c-4f61-baae-323f496df5b6

📥 Commits

Reviewing files that changed from the base of the PR and between 3a54205 and e4d6a85.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (19)
  • Cargo.toml
  • pacquet/crates/config/src/version_policy.rs
  • pacquet/crates/package-manager/Cargo.toml
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/Cargo.toml
  • pacquet/crates/resolving-deps-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs
  • pacquet/crates/resolving-resolver-base/Cargo.toml
  • pacquet/crates/resolving-resolver-base/src/resolve.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). (1)
  • GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs
  • pacquet/crates/resolving-deps-resolver/src/lib.rs
  • pacquet/crates/config/src/version_policy.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
🔇 Additional comments (16)
pacquet/crates/resolving-resolver-base/Cargo.toml (1)

14-18: LGTM!

pacquet/crates/resolving-resolver-base/src/resolve.rs (1)

13-14: LGTM!

Also applies to: 161-172

pacquet/crates/config/src/version_policy.rs (1)

133-150: LGTM!

pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs (1)

27-167: LGTM!

Also applies to: 179-203

pacquet/crates/resolving-npm-resolver/src/lib.rs (1)

29-30: LGTM!

Also applies to: 49-50

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (1)

83-267: LGTM!

Also applies to: 269-334

pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs (1)

1-187: LGTM!

Cargo.toml (1)

40-40: LGTM!

pacquet/crates/package-manager/Cargo.toml (1)

14-38: LGTM!

pacquet/crates/resolving-deps-resolver/Cargo.toml (1)

1-34: LGTM!

pacquet/crates/resolving-deps-resolver/src/lib.rs (1)

26-35: LGTM!

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)

52-97: LGTM!

Also applies to: 106-162, 167-187

pacquet/crates/resolving-deps-resolver/src/tests.rs (1)

80-209: LGTM!

pacquet/crates/package-manager/src/install_package_from_registry.rs (1)

96-103: LGTM!

Also applies to: 109-115, 125-128, 145-152, 185-216

pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)

79-109: LGTM!

Also applies to: 112-171, 182-276

pacquet/crates/package-manager/src/install.rs (1)

512-517: LGTM!

Comment thread pacquet/crates/package-manager/src/install_without_lockfile.rs Outdated
Comment thread pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
Comment thread pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs Outdated
Comment thread pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR wires the pacquet npm registry resolver into the no-lockfile install path by introducing an explicit resolve pass (building a resolved dependency tree) followed by an install/materialization pass that consumes pre-resolved ResolveResults.

Changes:

  • Add npm-resolution inputs to ResolveOptions and introduce NpmResolver + parse_bare_specifier in pacquet-resolving-npm-resolver.
  • Add new pacquet-resolving-deps-resolver crate that resolves a full dependency tree (flat name@version map + edges) with concurrency + dedupe.
  • Refactor the no-lockfile install flow to resolve first, then install using pre-resolved tarball URL/integrity instead of registry lookups during install.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pacquet/crates/resolving-resolver-base/src/resolve.rs Extends ResolveOptions with published-by/policy and dry-run flags to support npm resolver behavior.
pacquet/crates/resolving-resolver-base/Cargo.toml Adds resolver-base deps needed for new option types (chrono, pacquet-config).
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs Ports pnpm bare-specifier parsing to classify npm registry inputs and decline non-npm protocols.
pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs Unit tests for the specifier parser port.
pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs Implements Resolver for npm registry resolution and maps picks into ResolveResult.
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs Resolver tests (range picking, fallthrough, default tag, latest, policy violation surfacing).
pacquet/crates/resolving-npm-resolver/src/lib.rs Updates crate docs/exports to include the new resolver + parser surface.
pacquet/crates/resolving-deps-resolver/src/lib.rs New crate exporting resolve_dependency_tree used by install to pre-resolve deps.
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs Implements concurrent tree walking, per-id dedupe gating, and policy-violation collection.
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs Defines the resolved-tree output types consumed by install.
pacquet/crates/resolving-deps-resolver/src/tests.rs Tests for tree walking, dedupe, and resolver decline behavior.
pacquet/crates/resolving-deps-resolver/Cargo.toml Adds the new deps-resolver crate to the workspace.
pacquet/crates/package-manager/src/install.rs Plumbs an Arc<ThrottledClient> into the no-lockfile install struct for resolver ownership needs.
pacquet/crates/package-manager/src/install_without_lockfile.rs Refactors no-lockfile install into resolve-then-install using resolve_dependency_tree + NpmResolver.
pacquet/crates/package-manager/src/install_package_from_registry.rs Switches installer input from (name, range) to pre-resolved ResolveResult (tarball URL/integrity).
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs Updates tests to resolve via mock registry first, then install from the pre-resolved result.
pacquet/crates/package-manager/Cargo.toml Adds deps-resolver (and default-resolver) dependencies needed for the new flow.
pacquet/crates/config/src/version_policy.rs Makes PackageVersionPolicy clonable for inclusion in ResolveOptions.
Cargo.toml Adds pacquet-resolving-deps-resolver to workspace members.
Cargo.lock Locks new crate/dependency graph changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pacquet/crates/package-manager/src/install_without_lockfile.rs Outdated
Comment thread pacquet/crates/package-manager/src/install_package_from_registry.rs Outdated
Comment thread pacquet/crates/package-manager/Cargo.toml Outdated
- install_without_lockfile: place the dedup gate after the
  per-edge `install_package_from_registry` call instead of
  before it, so a `(name, version)` reached from two different
  parents still produces the per-parent `node_modules/<alias>`
  symlink. Dedup now only short-circuits the children walk,
  matching the original install loop's semantics before the
  resolveDependencyTree refactor.
- npm_resolver: propagate `serde_json::to_value` errors as
  `ResolveError` instead of silently dropping the manifest.
- install_package_from_registry: use `usize::try_from` for the
  `unpackedSize` hint so a `u64` larger than the host `usize`
  on 32-bit targets degrades to `None` rather than truncating.
- parse_bare_specifier: anchor the tarball filename version
  extraction on the scopeless-name prefix so a registry that
  serves `<reg>/foo/-/bar-1.0.0.tgz` declines instead of being
  mapped to the wrong `(name, version)` pair. Tests cover the
  rejection.
- package-manager: drop the unused
  `pacquet-resolving-default-resolver` dependency.
  `DefaultResolver` isn't wired into install today because the
  chain has only one entry; routing through it now would surface
  `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER` for every non-npm
  specifier (git/file/etc) and regress installs whose
  per-protocol resolvers haven't been ported yet. Land DefaultResolver
  wiring alongside the second resolver impl.

---
Written by an agent (Claude Code, claude-opus-4-7).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

323-357: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep progress events per package, not per edge.

Moving the dedupe gate below InstallPackageFromRegistry::run() fixes alias linking, but it also makes shared deps emit pnpm:progress resolved / imported every time this edge is visited. That turns a per-package signal into a per-edge signal, so reporter progress will be inflated for aliases and repeated transitive deps.

A small fix is to compute first_visit before this call and thread that into InstallPackageFromRegistry so non-first visits still create the parent-local symlink but skip the Resolved / Imported emits.

As per coding guidelines, pacquet/**/*.rs: "When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's."

🤖 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
323 - 357, Compute whether this is the first visit for the virtual store slot
before calling InstallPackageFromRegistry::run (i.e., evaluate first_visit =
ctx.resolved_packages.insert(virtual_store_name.clone()) and capture its boolean
result), add a first_visit boolean field to the InstallPackageFromRegistry
struct (or otherwise thread that flag into the operation), and make
InstallPackageFromRegistry skip emitting the pnpm:progress Resolved/Imported
events when first_visit is false while still performing the per-parent symlink
work; keep the existing dedupe return behavior for early exits but ensure the
symlink creation path still runs for non-first visits.
🤖 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.

Outside diff comments:
In `@pacquet/crates/package-manager/src/install_without_lockfile.rs`:
- Around line 323-357: Compute whether this is the first visit for the virtual
store slot before calling InstallPackageFromRegistry::run (i.e., evaluate
first_visit = ctx.resolved_packages.insert(virtual_store_name.clone()) and
capture its boolean result), add a first_visit boolean field to the
InstallPackageFromRegistry struct (or otherwise thread that flag into the
operation), and make InstallPackageFromRegistry skip emitting the pnpm:progress
Resolved/Imported events when first_visit is false while still performing the
per-parent symlink work; keep the existing dedupe return behavior for early
exits but ensure the symlink creation path still runs for non-first visits.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5fdc333f-ad8c-4019-8b0c-35e3bcdba649

📥 Commits

Reviewing files that changed from the base of the PR and between e4d6a85 and 0301717.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • pacquet/crates/package-manager/Cargo.toml
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Dylint
  • 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 fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
🔇 Additional comments (6)
pacquet/crates/package-manager/Cargo.toml (1)

14-37: LGTM!

pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier.rs (1)

170-176: LGTM!

pacquet/crates/resolving-npm-resolver/src/parse_bare_specifier/tests.rs (1)

112-120: LGTM!

pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (2)

268-268: LGTM!

The serialization error is now correctly propagated as a ResolveError instead of being silently swallowed. This addresses the previous review feedback.


1-46: LGTM!

Also applies to: 48-99, 101-176, 178-231, 233-267, 269-333

pacquet/crates/package-manager/src/install_package_from_registry.rs (1)

215-222: LGTM!

After landing the dedup-after-symlink reordering, every parent→child
edge to a shared dep fired the per-package signals
(`pnpm:progress resolved`, `pnpm:progress imported`, plus the
`DownloadTarballToStore` events) once for each visit. That inflates the
reporter's per-package counts and breaks parity with pnpm, which fires
the resolved/imported channels exactly once per `(name, version)`.

Thread a `first_visit: bool` through `InstallPackageFromRegistry`:

* `first_visit == true` runs the full path — emit `resolved`,
  download + cache + import the tarball, emit `imported`, symlink
  the parent.
* `first_visit == false` only refreshes the per-parent symlink under
  `<node_modules_dir>/<alias>`. No progress emits, no redundant
  download/import.

`install_without_lockfile.rs` computes `first_visit` from the
`resolved_packages.insert(...)` return value and threads it in; the
same boolean still gates the children recursion below the call.

Adds `second_visit_skips_progress_emits_but_still_links` against the
mock registry to pin the contract: a future refactor that moves the
gate can't quietly reintroduce per-edge spam.

---
Written by an agent (Claude Code, claude-opus-4-7).
Copilot AI review requested due to automatic review settings May 20, 2026 10:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Comment thread pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
Comment thread pacquet/crates/package-manager/src/install_package_from_registry.rs
Comment thread pacquet/crates/package-manager/src/install_without_lockfile.rs
* npm_resolver: route registry picks via `(alias, bare_specifier)`
  so an npm-alias entry like `"foo": "npm:@acme/bar@^1"` resolves
  through `registries[@acme]` instead of the local alias's empty
  scope. Ports upstream's bareSpecifier-aware `pickRegistryForPackage`
  by extending pacquet's `pick_registry_for_package` to take an
  optional `bare_specifier` and consulting the `npm:@scope/...`
  form before falling back to the package name's scope. New tests
  cover the npm-alias scope override and the plain-name fallback.

* install_without_lockfile: gate the second-visitor symlink on the
  first writer's import completion. `ResolvedPackages` is now a
  `DashMap<String, watch::Sender<bool>>` instead of a bare
  `DashSet<String>`; the first visitor inserts a `watch` channel,
  runs the full materialization, and signals `true` afterwards.
  Concurrent second visitors subscribe to the slot and await the
  completion bit before issuing `symlink_package`, so a Windows
  `force_symlink_dir` fall-back to junction creation can't race
  ahead of the target directory's existence. A dropped first writer
  (panic / cancel) surfaces as `FirstWriterAborted` so the install
  fails closed.

* install_without_lockfile: thread `minimumReleaseAge` /
  `minimumReleaseAgeExclude` from `Config` into the resolver's
  `ResolveOptions`. The previous slice left both unset, so the
  resolver path silently ignored the policy even though the
  lockfile-verifier already enforced it. Compiles the exclude
  patterns via `pacquet_config::version_policy::create_package_version_policy`,
  matching `build_resolution_verifiers`'s plumbing. Surfaces parse
  errors as `MinimumReleaseAgeExclude` with code
  `ERR_PNPM_INVALID_MINIMUM_RELEASE_AGE_EXCLUDE`.

---
Written by an agent (Claude Code, claude-opus-4-7).
@zkochan

zkochan commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pacquet/crates/resolving-npm-resolver/src/named_registry.rs`:
- Around line 109-127: The pick_registry_for_package currently falls back to
scope_of(pkg_name) even when bare_specifier is an npm: alias, causing scoped
package names to use the wrong registry; change the logic in
pick_registry_for_package to first call
scope_from_bare_specifier(bare_specifier) and if it returns Some use that scope,
but if bare_specifier is Some and begins with "npm:" do NOT fall back to
scope_of(pkg_name) (treat as unscoped alias and fall through to
registries.get("default")), otherwise (when bare_specifier is None or not an
npm: alias) fall back to scope_of(pkg_name); keep using registries.get(scope)
and registries.get("default").cloned().unwrap_or_default() and add a regression
test alongside the alias tests ensuring an `@scope`: "npm:pkg@..." alias resolves
to default when the npm: target is unscoped.
🪄 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: d44349fb-4295-4dff-a30f-e549e4995ca2

📥 Commits

Reviewing files that changed from the base of the PR and between 79c631c and 60f7e2f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • pacquet/crates/package-manager/Cargo.toml
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.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: Run benchmark on ubuntu-latest
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (macos-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 fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs

Comment thread pacquet/crates/resolving-npm-resolver/src/named_registry.rs Outdated
CodeRabbit flagged the local-scope fallback as a bug for
`"@private/foo": "npm:lodash@^1"`. The behavior actually matches
upstream's `getScope` at
<https://github.com/pnpm/pnpm/blob/2a9bd897bf/config/pick-registry-for-package/src/index.ts#L8-L19>:
the npm-alias branch only returns a scope when the alias target is
itself scoped (`npm:@scope/...`); otherwise it falls through to the
local pkgName's scope. The local alias slot is treated as the
authoritative signal of which registry the user wants to fetch from
(e.g. a private fork of an unscoped package re-published under
`@private/`).

Adds a regression test (`unscoped_npm_alias_under_scoped_local_name_keeps_local_scope`)
locking the upstream-matching behavior in so a future refactor can't
quietly diverge from pnpm here.

---
Written by an agent (Claude Code, claude-opus-4-7).
Copilot AI review requested due to automatic review settings May 20, 2026 10:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.

Comment thread pacquet/crates/package-manager/src/install_package_from_registry.rs
Comment thread pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs Outdated
zkochan added 2 commits May 20, 2026 12:59
`pickRegistryForPackage('@private/foo', 'npm:lodash@^1')` previously
returned `registries["@Private"]`, even though the fetched package
is unscoped `lodash` and doesn't live on the `@private` registry.
The local alias slot is just where the install lands in
`node_modules`; it must not drive registry routing when the npm-
alias target itself is unscoped.

`getScope` now returns `null` in the npm-alias branch when the
target is unscoped (instead of falling through to the local
pkgName's scope), so the call falls through to `registries.default`.
The scoped-target case (`npm:@scope/name`) is unchanged: it still
returns the target's scope, overriding the local key's scope as
before. Adds a TS test covering the bug case.

Mirrored in pacquet's `pick_registry_for_package` (the previously
chained `scope_from_bare_specifier(...).or_else(|| scope_of(pkg_name))`
collapsed into a single branch). The pacquet regression test now
asserts the corrected default-routing behavior.

Thanks to @coderabbitai for catching this.

---
Written by an agent (Claude Code, claude-opus-4-7).
* npm_resolver: drop the dead `LockfileResolution::Registry` branch in
  `build_resolve_result`. `PackageVersion.dist.tarball` is `String`
  (non-optional in the picker's payload) so the empty-tarball case
  the branch guarded never fires, and the install side's
  `extract_tarball` only handles the `Tarball` shape. The mixed-shape
  output forced a Registry→URL reconstruction with no payoff. Always
  emit `Tarball`.

* npm_resolver: preserve every `WantedDependency` field in
  `resolve_latest_impl` instead of rebuilding with
  `..WantedDependency::default()`. The previous rebuild silently
  dropped `injected`, `prev_specifier`, and `optional`. Clone the
  source wanted dep and only synthesize a `bare_specifier` when one
  isn't already set, matching upstream's `createResolveLatest`.

* pick-registry-for-package + pacquet named_registry: add the
  scoped-local + scoped-target (different scopes) regression case
  (`"@scope1/foo": "npm:@scope2/bar@^1"` → `registries[@scope2]`) on
  both sides. The TS suite gained a matching test for symmetry.

---
Written by an agent (Claude Code, claude-opus-4-7).
Copilot AI review requested due to automatic review settings May 20, 2026 11:10
@zkochan

zkochan commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:123

  • policy_violations is appended before the global packages dedupe check. If the same resolved id is encountered via multiple parents, later visitors will still push the same policy_violation again even though the package is already in packages, leading to duplicate violation entries downstream. Consider only recording the violation on the first visitor (after winning the packages gate) or deduping the collection (e.g. by (name, version, code)).
    if let Some(violation) = result.policy_violation.clone() {
        ctx.policy_violations.lock().await.push(violation);
    }

Comment thread pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs Outdated
* `resolve_node`'s `Ok(None)` from the resolver chain is no longer
  silently flattened away. The resolver-base contract says `None`
  means "this resolver declined" — outside `DefaultResolver`'s
  internal `??` fall-through, that's a contract violation that
  would leave installs missing dependencies (e.g. an unported
  `git+ssh://` spec would just disappear from the tree). The tree
  walker now surfaces it as a typed `SpecNotSupported` error with
  code `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER`, mirroring upstream's
  `default-resolver` error and using the same `{alias}@{bare}`
  rendering. Updated the regression test to assert the error
  rather than an empty tree.

* `resolve_dependency_tree` no longer wraps the context in an
  `Arc`. The per-direct-dep futures now borrow `&Ctx` directly;
  `try_join_all` keeps the borrow alive for the duration of the
  resolve pass, and after the await we move the locked maps out
  via `into_inner()`. Removes the `Arc::try_unwrap(...).expect(...)`
  that would have panicked on an unexpected outstanding clone — a
  panic in a library API is not the right failure mode even when
  it "shouldn't happen."

---
Written by an agent (Claude Code, claude-opus-4-7).
@zkochan zkochan changed the title feat(pacquet): wire NpmResolver into the install resolution stage feat(pacquet): wire NpmResolver into install; fix(pick-registry) unscoped npm-alias routing May 20, 2026
`install_subtree` used `Sender::send(true)` to signal that the first
writer for a virtual-store slot had finished materializing. But
`Sender::send` returns `Err` when the channel has zero receivers —
and the watch channel I create per slot has zero receivers at the
moment of `send` whenever the first writer finishes before any
second visitor subscribes (true for cyclic and diamond graphs:
`A → B → A` enters the second `install_subtree(A)` only AFTER the
outer writer already ran `IPFR::run` and tried to send).

Under `send`, that race silently failed: the sender's stored value
stayed `false`, the cycle's inner second-visit subscribed, saw
`false`, and `changed().await` blocked forever. CI flagged this on
the `should_install_circular_dependencies` test (hung past 6 minutes)
plus two `lifecycle_scripts` tests with similar diamond shapes.

`send_replace(true)` always writes the value and returns the old
one regardless of receiver count, so the freshly-subscribed
receiver's first `borrow_and_update()` immediately observes `true`
and breaks the wait loop. Added a comment naming the race and
why `send_replace` is required.

Verified locally: `should_install_circular_dependencies` now
passes in 8.5s; full `pacquet-cli` suite (84 tests) passes in 46s.

---
Written by an agent (Claude Code, claude-opus-4-7).
Copilot AI review requested due to automatic review settings May 20, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

Comment thread pacquet/crates/package-manager/src/install_without_lockfile.rs Outdated
`config.minimum_release_age` is `u64` (minutes); the previous
`minutes as i64` cast wraps silently when the value exceeds
`i64::MAX`, and `chrono::Duration::minutes` can overflow internally
before the subtraction even runs. An absurd configured value could
produce a cutoff in the wrong direction (everything mature, or
nothing mature) rather than failing or ignoring the setting.

Use a chain of checked operations at both call sites — the resolve
pass in `install_without_lockfile.rs` and the verifier factory in
`create_npm_resolution_verifier.rs`:

* `i64::try_from(minutes)` → catches the `u64 → i64` boundary.
* `chrono::Duration::try_minutes(...)` → catches the minutes-to-
  internal-units overflow.
* `DateTime::checked_sub_signed(...)` → catches the wall-clock
  underflow.

On overflow at any step the cutoff degrades to `None`, leaving the
policy inactive for the install instead of fabricating a wrong
cutoff. Same shape used at both sites so the resolver-time pick and
the lockfile-verification gate degrade identically.

---
Written by an agent (Claude Code, claude-opus-4-7).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: global virtual store area: resolution area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants