Skip to content

feat(pacquet): port resolving/tarball-resolver#11773

Merged
zkochan merged 4 commits into
mainfrom
tarball-resolver
May 20, 2026
Merged

feat(pacquet): port resolving/tarball-resolver#11773
zkochan merged 4 commits into
mainfrom
tarball-resolver

Conversation

@zkochan

@zkochan zkochan commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds pacquet-resolving-tarball-resolver, the Rust port of resolving/tarball-resolver/src/index.ts. Claims any WantedDependency whose bare specifier starts with http:// or https://, normalizes the URL through reqwest::Url::parse, runs a HEAD pre-flight that follows redirects, and stores the post-redirect URL in the resolution when the response carries cache-control: immutable. Mutable responses keep the normalized request URL.
  • Leaves name_ver: None on the ResolveResult and integrity: None on the TarballResolution. Tarball URLs carry no name@version at resolve time — the canonical name/version come from the manifest after the tarball is fetched. Integrity is computed later by package-requester. Matches upstream.
  • Wires TarballResolver into the install_without_lockfile resolver chain after the npm and git resolvers. Order mirrors upstream's createResolver chain: npm → git → tarball → local/runtimes/... (those slot in as their crates land).

This PR previously also introduced PkgResolutionId in resolving-resolver-base, but that landed first via the git-resolver port in #11779.

Test plan

  • just check — workspace passes
  • just test — 434 tests pass across the affected crates (7 new in pacquet-resolving-tarball-resolver: http(s) claim vs decline, mutable vs immutable response, immutable-after-redirect follow, resolve_latest for http(s) vs non-http(s))
  • just lint — no clippy warnings
  • just fmt — clean
  • CI

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

Summary by CodeRabbit

  • New Features
    • Added HTTP/HTTPS tarball URL resolver to the dependency resolution chain, enabling native resolution of tarball dependencies alongside existing npm and git resolvers.
    • Features automatic URL normalization, intelligent cache directive handling, and automatic redirect resolution to improve dependency caching behavior and resolution accuracy.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c6037349-acdc-4033-8d1f-95e9457420f8

📥 Commits

Reviewing files that changed from the base of the PR and between 1009fd8 and 04c39ee.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
📜 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). (7)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Code Coverage
  • GitHub Check: Doc
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that 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-tarball-resolver/src/tarball_resolver.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
🔇 Additional comments (1)
pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs (1)

83-103: LGTM!


📝 Walkthrough

Walkthrough

This PR introduces a new pacquet-resolving-tarball-resolver crate that resolves bare HTTP(S) URLs to tarball artifacts. The resolver normalizes URLs, performs throttled HEAD requests to detect immutable redirects, and integrates into the package-manager dependency resolver chain as the third resolver after npm and git.

Changes

Tarball Resolution Support

Layer / File(s) Summary
Tarball resolver crate setup
Cargo.toml, pacquet/crates/resolving-tarball-resolver/Cargo.toml, pacquet/crates/resolving-tarball-resolver/src/lib.rs
New crate registered in workspace with reqwest and internal deps; crate root docs describe HTTP(S) tarball URL resolution behavior including immutable-redirect handling.
TarballResolver HTTP resolution
pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
TarballResolver struct implements Resolver trait with HTTP(S)-only filtering, URL normalization via reqwest::Url::parse, throttled HEAD requests, and Cache-Control immutability detection to select post-redirect or normalized URLs for tarball resolution.
Resolver behavior tests
pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
Test suite covers HTTP(S) filtering, mutable/immutable response handling, redirect chains, and resolve_latest() behavior returning latest_manifest: None.
Package manager integration
pacquet/crates/package-manager/Cargo.toml, pacquet/crates/package-manager/src/install_without_lockfile.rs
TarballResolver wired into DefaultResolver chain after npm and git, forming the three-step upstream-compatible resolver sequence.

Sequence Diagram

sequenceDiagram
  participant Client
  participant TarballResolver
  participant ThrottledClient
  participant ResolveResult
  Client->>TarballResolver: resolve(WantedDependency)
  TarballResolver->>TarballResolver: validate bare_specifier and HTTP(S)
  TarballResolver->>TarballResolver: normalize URL via url::Url
  TarballResolver->>ThrottledClient: HEAD request
  ThrottledClient-->>TarballResolver: response with Cache-Control
  TarballResolver->>TarballResolver: select immutable post-redirect or request URL
  TarballResolver->>ResolveResult: construct Tarball resolution
  ResolveResult-->>Client: return claim with resolved tarball URL
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

  • pnpm/pnpm#11756: Shares resolver chain wiring patterns in package-manager install-without-lockfile flow.

Possibly related PRs

  • pnpm/pnpm#11779: Both PRs modify the DefaultResolver chain construction in install_without_lockfile.rs.

Poem

🐰 A tarball lands with HTTP's gleam,
Cache-Control guides each immutable dream,
Redirects follow where URLs dare,
Three resolvers dance, npm-git-tarball fair! 📦

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: porting the tarball-resolver from TypeScript to Rust for the pacquet project.
Docstring Coverage ✅ Passed Docstring coverage is 80.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.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tarball-resolver

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.

@zkochan zkochan force-pushed the tarball-resolver branch from b081cf1 to 979c117 Compare May 20, 2026 14:17
@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.00      7.5±0.17ms   578.1 KB/sec    1.01      7.6±0.52ms   569.9 KB/sec

@codecov-commenter

codecov-commenter commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.70%. Comparing base (35d5440) to head (04c39ee).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11773      +/-   ##
==========================================
+ Coverage   89.66%   89.70%   +0.03%     
==========================================
  Files         170      171       +1     
  Lines       20318    20392      +74     
==========================================
+ Hits        18218    18292      +74     
  Misses       2100     2100              

☔ 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.573 ± 0.120 2.420 2.770 1.03 ± 0.05
pacquet@main 2.491 ± 0.057 2.405 2.598 1.00
pnpm 4.943 ± 0.057 4.853 5.019 1.98 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.57263951852,
      "stddev": 0.11982532150411719,
      "median": 2.53351660082,
      "user": 2.7983286799999996,
      "system": 3.8273040799999998,
      "min": 2.41951009332,
      "max": 2.77014530232,
      "times": [
        2.53547155032,
        2.53156165132,
        2.77014530232,
        2.76433000132,
        2.41951009332,
        2.47769901532,
        2.6424793803199997,
        2.59753264932,
        2.49069087932,
        2.49697466232
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.49053240452,
      "stddev": 0.057384167495415084,
      "median": 2.49318167532,
      "user": 2.76756908,
      "system": 3.82022608,
      "min": 2.40489517332,
      "max": 2.59835963632,
      "times": [
        2.40489517332,
        2.52583561532,
        2.51645085032,
        2.48981142232,
        2.5393671483199998,
        2.59835963632,
        2.49655192832,
        2.44206403232,
        2.44914585032,
        2.44284238832
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.942611014020001,
      "stddev": 0.056675404293762284,
      "median": 4.9498312783200005,
      "user": 8.35457258,
      "system": 4.266083879999999,
      "min": 4.85344168532,
      "max": 5.018762268320001,
      "times": [
        5.018762268320001,
        4.97307549932,
        4.982182868320001,
        4.85344168532,
        4.9013517253200005,
        4.87484351632,
        4.911298594320001,
        4.9333888343200005,
        5.01149142632,
        4.96627372232
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 691.3 ± 7.6 677.4 702.8 1.00
pacquet@main 740.6 ± 71.4 691.0 896.6 1.07 ± 0.10
pnpm 2598.7 ± 109.2 2473.9 2782.5 3.76 ± 0.16
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.69129429678,
      "stddev": 0.007632192448618294,
      "median": 0.69092345048,
      "user": 0.40964648,
      "system": 1.5711779600000002,
      "min": 0.67743203548,
      "max": 0.70282783148,
      "times": [
        0.69666908448,
        0.68568283548,
        0.68951244448,
        0.69900668748,
        0.70282783148,
        0.69657927348,
        0.68625927448,
        0.67743203548,
        0.68663904448,
        0.69233445648
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.7405651207799998,
      "stddev": 0.07140788286630792,
      "median": 0.70340644548,
      "user": 0.4132271799999999,
      "system": 1.6006218599999997,
      "min": 0.69096950248,
      "max": 0.89659043848,
      "times": [
        0.78614099548,
        0.69779084048,
        0.82687461848,
        0.70666159648,
        0.69543355348,
        0.69096950248,
        0.89659043848,
        0.70193687848,
        0.70487601248,
        0.69837677148
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.59874654148,
      "stddev": 0.10921526889640769,
      "median": 2.5540626909800004,
      "user": 3.3043050800000002,
      "system": 2.1799953600000004,
      "min": 2.4739432954800002,
      "max": 2.78249256948,
      "times": [
        2.74161571448,
        2.5629021184800003,
        2.5428791744800003,
        2.78249256948,
        2.54522326348,
        2.49059866948,
        2.51769268248,
        2.4739432954800002,
        2.62461993848,
        2.7054979884800003
      ]
    }
  ]
}

@zkochan zkochan force-pushed the tarball-resolver branch from 0ae204c to ccb700c Compare May 20, 2026 19:35
@zkochan zkochan marked this pull request as ready for review May 20, 2026 20:33
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@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: 2

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

Inline comments:
In `@pacquet/crates/package-manager/src/install_package_from_registry.rs`:
- Around line 111-114: The code currently unwraps parse of resolution.id into
PkgNameVer using expect in install_package_from_registry (the block that assigns
name_ver, real_name, version), which may panic on malformed ids; change this to
handle parse failure gracefully by matching the parse result (or using .ok_or /
map_err) and returning the appropriate error variant (UnsupportedResolution)
instead of panicking; ensure you reference resolution.id.as_ref().parse() and
PkgNameVer so the function returns Err(UnsupportedResolution) when parsing fails
and only proceeds to set real_name and version on success.

In `@pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs`:
- Around line 115-117: The is_http_url function currently claims inputs starting
with "http:" or "https:" too loosely (e.g., "http:foo"); tighten the check to
only accept proper schemes by matching "http://" and "https://" instead. Update
the is_http_url(bare: &str) logic to use starts_with("http://") ||
starts_with("https://") so non-standard specs aren't misrouted; verify related
call sites in TarballResolver and any unit tests that rely on is_http_url.
🪄 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: 97538b78-b147-41cd-84c5-09424d9c99e7

📥 Commits

Reviewing files that changed from the base of the PR and between f807f6d and ccb700c.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • Cargo.toml
  • 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-default-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/resolving-resolver-base/Cargo.toml
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-resolver-base/src/tests.rs
  • pacquet/crates/resolving-tarball-resolver/Cargo.toml
  • pacquet/crates/resolving-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that 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-resolver-base/src/tests.rs
  • pacquet/crates/resolving-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-default-resolver/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/tests.rs
  • pacquet/crates/resolving-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-default-resolver/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
🔇 Additional comments (15)
pacquet/crates/resolving-resolver-base/Cargo.toml (1)

17-20: LGTM!

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

15-15: LGTM!

Also applies to: 20-49, 223-229

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

30-33: LGTM!

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

1-1: LGTM!

Also applies to: 6-7, 137-137

Cargo.toml (1)

46-46: LGTM!

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

34-37: LGTM!

Also applies to: 347-349

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

118-118: LGTM!

Also applies to: 152-152, 250-250, 278-278

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

1-1: LGTM!

Also applies to: 3-4, 18-20

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

145-145: LGTM!

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

1-1: LGTM!

Also applies to: 5-7, 46-47

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

1-27: LGTM!

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

1-27: LGTM!

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

1-154: LGTM!

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

155-159: LGTM!

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

380-389: 🏗️ Heavy lift

No action needed—the comment correctly documents the actual invariant.

The code's assumption is accurate and already explicit: install_without_lockfile uses only NpmResolver, which produces name@version IDs, so the .expect() is safe. The comment at lines 380-383 documents this npm-only path clearly and correctly. TarballResolver exists in the codebase but is not wired into the without-lockfile resolver chain—DefaultResolver's chain remains empty pending future per-protocol wiring in subsequent PRs. There is no forward-compatibility risk with the current code.

Comment thread pacquet/crates/package-manager/src/install_package_from_registry.rs Outdated
Comment thread pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
@zkochan zkochan force-pushed the tarball-resolver branch from ccb700c to a15e160 Compare May 20, 2026 20:55
zkochan added 2 commits May 20, 2026 23:13
Adds pacquet-resolving-tarball-resolver, the Rust port of
resolving/tarball-resolver/src/index.ts. The new crate claims any
WantedDependency whose bare specifier starts with `http://` or
`https://`, normalizes the URL through `reqwest::Url::parse`, runs a
HEAD pre-flight that follows redirects, and stores the post-redirect
URL in the resolution when the response carries
`cache-control: immutable`. Mutable responses keep the normalized
request URL. Integrity stays None at resolve time, matching upstream
(integrity is stamped later in package-requester).

To make the seam fit, ResolveResult.id is now an opaque
`PkgResolutionId(String)` newtype in resolver-base mirroring
upstream's branded string at core/types/misc.ts:59. PkgNameVer was a
poor fit because tarball ids are URLs and git ids are
`repo#commit` — not name@version. NpmResolver round-trips the
existing PkgNameVer through PkgResolutionId::from(string); the
npm-only install paths in package-manager parse the id back to
PkgNameVer at their boundary (safe because the npm resolver stamps
that shape). The deps-resolver alias fallback drops its `.id.name`
access since the id is opaque now.

Test coverage in the new crate (7 tests, mockito): http(s) claim
vs decline, mutable vs immutable response, immutable-after-redirect
follow, resolve_latest for http(s) vs non-http(s).
… params

Dylint's perfectionist::single-letter-closure-param rejects |v|; rename
to |header| in the cache-control header check.
@zkochan zkochan force-pushed the tarball-resolver branch from a15e160 to aa667cc Compare May 20, 2026 21:16
…chain

Insert TarballResolver after the GitResolver in install_without_lockfile's
DefaultResolver chain so http(s):// bare specifiers actually route through
the new resolver. Order mirrors upstream's chain
(npm → git → tarball → local/...).

@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

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

14-15: ⚡ Quick win

Use serde conversion attrs for this branded string.

PkgResolutionId is a public brand, so #[serde(transparent)] skips the explicit string conversion boundary the repo expects here. Please serialize via String and deserialize through TryFrom<String>/Into<String> instead.

♻️ Suggested change
-use derive_more::{Display, From};
+use derive_more::{Display, From, Into};
@@
-#[derive(Debug, Display, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, From)]
-#[serde(transparent)]
+#[derive(Debug, Display, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, From, Into)]
+#[serde(try_from = "String", into = "String")]
 pub struct PkgResolutionId(String);

As per coding guidelines, "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."

Also applies to: 33-35

🤖 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 14 - 15,
Replace #[serde(transparent)] on the branded string type PkgResolutionId with
explicit serde conversion attributes so serialization goes through String and
deserialization uses TryFrom<String>: annotate PkgResolutionId with
#[serde(try_from = "String", into = "String")] and ensure the type implements
TryFrom<String> for deserialization and Into<String>/From<&str> for
serialization; make the same change for the other branded types referenced
around lines 33-35 so all public branded string types serialize via String and
deserialize via TryFrom<String>.
🤖 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-tarball-resolver/src/tarball_resolver.rs`:
- Around line 99-101: The claimed result sets alias: None which drops the
requested install name for tarball deps; update the code that constructs the
claimed result (the block with resolved_via and normalized_bare_specifier) to
forward the requested alias from wanted_dependency by using
wanted_dependency.alias.clone() (or equivalent) instead of None so the original
spec alias (e.g., "foo" for "https://.../bar.tgz") is preserved downstream.

---

Nitpick comments:
In `@pacquet/crates/resolving-resolver-base/src/resolve.rs`:
- Around line 14-15: Replace #[serde(transparent)] on the branded string type
PkgResolutionId with explicit serde conversion attributes so serialization goes
through String and deserialization uses TryFrom<String>: annotate
PkgResolutionId with #[serde(try_from = "String", into = "String")] and ensure
the type implements TryFrom<String> for deserialization and
Into<String>/From<&str> for serialization; make the same change for the other
branded types referenced around lines 33-35 so all public branded string types
serialize via String and deserialize via TryFrom<String>.
🪄 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: aee58a3a-d3b3-4323-ad55-e218d6ccd4f4

📥 Commits

Reviewing files that changed from the base of the PR and between ccb700c and aa667cc.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • Cargo.toml
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-tarball-resolver/Cargo.toml
  • pacquet/crates/resolving-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/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: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Dylint
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that 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-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
🧠 Learnings (1)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-tarball-resolver/src/lib.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver/tests.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs
🔇 Additional comments (5)
pacquet/crates/resolving-resolver-base/src/resolve.rs (1)

238-248: LGTM!

Cargo.toml (1)

34-48: LGTM!

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

1-26: LGTM!

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

1-26: LGTM!

pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs (1)

24-81: LGTM!

Also applies to: 107-122

Comment thread pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs Outdated
…ync lockfile

- Echo `wanted_dependency.alias` on the `ResolveResult` so a spec
  like `"foo": "https://.../bar.tgz"` preserves `foo` as the
  install name downstream. Matches the npm and git resolvers'
  convention even though upstream TS doesn't surface alias on
  `ResolveResult` (downstream consumers fall back to
  `wantedDependency.alias` over there).
- Drop a stray blank line in resolve.rs that cargo fmt rejected.
- Record the new `package-manager -> resolving-tarball-resolver`
  edge in Cargo.lock so `cargo build --locked` succeeds on CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants