feat(pacquet): port resolving.local-resolver (file:/link:/workspace:)#11778
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete ChangesLocal Resolver Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11778 +/- ##
==========================================
- Coverage 89.71% 89.70% -0.01%
==========================================
Files 171 174 +3
Lines 20392 20781 +389
==========================================
+ Hits 18294 18642 +348
- Misses 2098 2139 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3829924687199995,
"stddev": 0.10056834721082664,
"median": 2.41528471472,
"user": 2.81187908,
"system": 3.6115356199999993,
"min": 2.25670935372,
"max": 2.53149673472,
"times": [
2.46492725272,
2.28206037172,
2.27853703072,
2.41824527172,
2.48825635672,
2.27912288572,
2.41809003272,
2.25670935372,
2.53149673472,
2.41247939672
]
},
{
"command": "pacquet@main",
"mean": 2.3333476734199996,
"stddev": 0.0726741813109322,
"median": 2.32545159722,
"user": 2.7767000800000003,
"system": 3.612974419999999,
"min": 2.2186461407199998,
"max": 2.49424970172,
"times": [
2.2627912277199997,
2.3056261827199998,
2.35842360372,
2.49424970172,
2.32888972072,
2.36383757572,
2.36139438372,
2.31760472372,
2.2186461407199998,
2.3220134737199998
]
},
{
"command": "pnpm",
"mean": 4.51309917682,
"stddev": 0.07267211952086954,
"median": 4.50059716422,
"user": 7.559357779999999,
"system": 4.01114362,
"min": 4.41869352672,
"max": 4.654506581720001,
"times": [
4.60425408572,
4.55662000472,
4.654506581720001,
4.50321683872,
4.45717031972,
4.457805485720001,
4.51128691272,
4.41869352672,
4.46946052272,
4.49797748972
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.69463565946,
"stddev": 0.044886028400293966,
"median": 0.67974802806,
"user": 0.37066788,
"system": 1.5753190999999998,
"min": 0.66461061256,
"max": 0.81717007156,
"times": [
0.81717007156,
0.67884879156,
0.68178245356,
0.67887119756,
0.68062485856,
0.71252220556,
0.66461061256,
0.67491779456,
0.67049807356,
0.6865105355600001
]
},
{
"command": "pacquet@main",
"mean": 0.70560709866,
"stddev": 0.02369964060434977,
"median": 0.6952435880600001,
"user": 0.38524948000000003,
"system": 1.5965902,
"min": 0.6855884585600001,
"max": 0.74905202256,
"times": [
0.73229001356,
0.74905202256,
0.68929518856,
0.7016826285600001,
0.68728591656,
0.69687887256,
0.73443812956,
0.69360830356,
0.6855884585600001,
0.68595145256
]
},
{
"command": "pnpm",
"mean": 2.4034090607600005,
"stddev": 0.07663747118999377,
"median": 2.38591935856,
"user": 2.9346475800000005,
"system": 2.2167555,
"min": 2.33197315156,
"max": 2.53200654656,
"times": [
2.33387110256,
2.41500027456,
2.46190000756,
2.53200654656,
2.40232652156,
2.33285016756,
2.33913365656,
2.36951219556,
2.51551698356,
2.33197315156
]
}
]
} |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pacquet/crates/resolving-local-resolver/tests/resolve.rs (1)
365-372: ⚡ Quick winAlign injected test setup with the scenario name.
This test is named as an injected-dependency case but sets
injected: false. Please align either the setup (injected: true) or the test name so the covered branch is unambiguous.Proposed minimal alignment
- injected: false, + injected: true,🤖 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-local-resolver/tests/resolve.rs` around lines 365 - 372, The test function fail_when_resolving_from_not_existing_directory_an_injected_dependency mismatches its setup: the WantedLocalDependency instance `wd` sets `injected: false` but the test name implies an injected dependency; update the `wd` setup to `injected: true` (or alternatively rename the test function to match a non-injected scenario) so the test covers the intended injected branch in WantedLocalDependency handling.pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs (2)
99-105: 💤 Low valueSimplify the
path:prefix check.The
let _ = rest;binding is unnecessary sincerestisn't used. Consider usingstarts_withfor consistency with the other branches.♻️ Suggested simplification
- if let Some(rest) = bare.strip_prefix("path:") { - let _ = rest; + if bare.starts_with("path:") { return Err(PathProtocolNotSupportedError { bare_specifier: bare.to_string(), protocol: "path:".to_string(), }); }🤖 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-local-resolver/src/parse_bare_specifier.rs` around lines 99 - 105, The code uses if let Some(rest) = bare.strip_prefix("path:") { let _ = rest; ... } which needlessly binds rest; replace this with a simpler check consistent with other branches by using bare.starts_with("path:") to detect the prefix and then return the PathProtocolNotSupportedError (populate bare_specifier with bare.to_string() and protocol with "path:".to_string())—locate this logic around the bare.strip_prefix("path:") block in parse_bare_specifier.rs and update it to remove the unused binding.
155-159: 💤 Low valueConsider handling missing home directory more explicitly.
If
home::home_dir()returnsNone,unwrap_or_default()produces an emptyPathBuf, causingresolve_path(&home, rest)to resolverestrelative to an empty path. This could lead to unexpected resolution behavior on systems where home directory detection fails.🤖 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-local-resolver/src/parse_bare_specifier.rs` around lines 155 - 159, The current branch that handles strip_tilde_prefix(&spec) calls home::home_dir().unwrap_or_default(), which can produce an empty PathBuf and make resolve_path(&home, rest) behave unexpectedly; modify this branch to explicitly handle the None case from home::home_dir() instead of using unwrap_or_default(): call home::home_dir(), match its Some(home) => use resolve_path(&home, rest) and build normalized as before, and handle None by returning or propagating a clear error (or choosing a documented fallback) so resolve_path is never called with an empty home path. Ensure you update the code paths that set (fetch_spec, normalized_bare_specifier) accordingly and provide an informative error message referencing the tilde-expanded spec.pacquet/crates/resolving-local-resolver/src/local_resolver.rs (1)
262-304: ⚡ Quick winBlocking
std::fs::metadatacall in async context.The
std::fs::metadatacall is blocking and could stall the async runtime on slow filesystems. Consider usingtokio::fs::metadatafor consistency with the asynccompute_tarball_integrityfunction.♻️ Suggested change to use async metadata
This would require making
synthesize_fallback_manifestasync:-fn synthesize_fallback_manifest( +async fn synthesize_fallback_manifest( spec: &LocalPackageSpec, opts: &LocalResolverOptions, ) -> Result<serde_json::Value, ResolveLocalError> { - let metadata = std::fs::metadata(&spec.fetch_spec); + let metadata = tokio::fs::metadata(&spec.fetch_spec).await;And updating the call site:
- Ok(None) => synthesize_fallback_manifest(&spec, opts)?, + Ok(None) => synthesize_fallback_manifest(&spec, opts).await?,🤖 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-local-resolver/src/local_resolver.rs` around lines 262 - 304, synthesize_fallback_manifest currently calls the blocking std::fs::metadata from an async context; change its signature to async fn synthesize_fallback_manifest(spec: &LocalPackageSpec, opts: &LocalResolverOptions) -> Result<serde_json::Value, ResolveLocalError>, replace std::fs::metadata(&spec.fetch_spec) with let metadata = tokio::fs::metadata(&spec.fetch_spec).await (or use tokio::task::spawn_blocking if you must keep it sync), and update all call sites (e.g. callers that previously invoked synthesize_fallback_manifest and any surrounding async flows such as compute_tarball_integrity) to await the new async function and propagate async changes as needed.
🤖 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`:
- Line 107: The call to name_version_from_manifest currently can panic on
missing/non-string manifest fields and must instead return a typed
InstallPackageFromRegistryError so failures are returned not crashed; change
name_version_from_manifest (and any helper that uses unwrap/expect) to return
Result<(String, String), InstallPackageFromRegistryError>, replace panics with
Err(...) variants that map to the appropriate pnpm-style error code/message, and
propagate the Result from install_package_from_registry (where the let
(real_name, version) = ...) so callers handle the error rather than panicking.
---
Nitpick comments:
In `@pacquet/crates/resolving-local-resolver/src/local_resolver.rs`:
- Around line 262-304: synthesize_fallback_manifest currently calls the blocking
std::fs::metadata from an async context; change its signature to async fn
synthesize_fallback_manifest(spec: &LocalPackageSpec, opts:
&LocalResolverOptions) -> Result<serde_json::Value, ResolveLocalError>, replace
std::fs::metadata(&spec.fetch_spec) with let metadata =
tokio::fs::metadata(&spec.fetch_spec).await (or use tokio::task::spawn_blocking
if you must keep it sync), and update all call sites (e.g. callers that
previously invoked synthesize_fallback_manifest and any surrounding async flows
such as compute_tarball_integrity) to await the new async function and propagate
async changes as needed.
In `@pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs`:
- Around line 99-105: The code uses if let Some(rest) =
bare.strip_prefix("path:") { let _ = rest; ... } which needlessly binds rest;
replace this with a simpler check consistent with other branches by using
bare.starts_with("path:") to detect the prefix and then return the
PathProtocolNotSupportedError (populate bare_specifier with bare.to_string() and
protocol with "path:".to_string())—locate this logic around the
bare.strip_prefix("path:") block in parse_bare_specifier.rs and update it to
remove the unused binding.
- Around line 155-159: The current branch that handles strip_tilde_prefix(&spec)
calls home::home_dir().unwrap_or_default(), which can produce an empty PathBuf
and make resolve_path(&home, rest) behave unexpectedly; modify this branch to
explicitly handle the None case from home::home_dir() instead of using
unwrap_or_default(): call home::home_dir(), match its Some(home) => use
resolve_path(&home, rest) and build normalized as before, and handle None by
returning or propagating a clear error (or choosing a documented fallback) so
resolve_path is never called with an empty home path. Ensure you update the code
paths that set (fetch_spec, normalized_bare_specifier) accordingly and provide
an informative error message referencing the tilde-expanded spec.
In `@pacquet/crates/resolving-local-resolver/tests/resolve.rs`:
- Around line 365-372: The test function
fail_when_resolving_from_not_existing_directory_an_injected_dependency
mismatches its setup: the WantedLocalDependency instance `wd` sets `injected:
false` but the test name implies an injected dependency; update the `wd` setup
to `injected: true` (or alternatively rename the test function to match a
non-injected scenario) so the test covers the intended injected branch in
WantedLocalDependency handling.
🪄 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: 5dcf1b0a-23bc-4142-adfb-64d71f1f5b06
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlpacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-local-resolver/Cargo.tomlpacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-resolver-base/src/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.rs
🔇 Additional comments (37)
Cargo.toml (1)
43-43: LGTM!pacquet/crates/resolving-local-resolver/src/chain.rs (1)
1-109: LGTM!pacquet/crates/resolving-local-resolver/src/lib.rs (1)
1-39: LGTM!pacquet/crates/resolving-local-resolver/tests/chain.rs (1)
1-92: LGTM!pacquet/crates/resolving-local-resolver/tests/resolve.rs (1)
1-364: LGTM!Also applies to: 373-473
pacquet/crates/resolving-resolver-base/Cargo.toml (1)
17-20: LGTM!pacquet/crates/resolving-resolver-base/src/lib.rs (1)
25-25: LGTM!Also applies to: 29-29
pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
15-15: LGTM!Also applies to: 18-18, 192-200
pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (1)
30-30: LGTM!Also applies to: 34-36, 347-347
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-19
pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
1-1: LGTM!Also applies to: 5-6, 46-46, 48-48
pacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rs (1)
22-24: ⚡ Quick winThis change contradicts the documented design. Reject the proposed serde wiring modification.
The file's doc comment (lines 5–9) explicitly states that
PkgResolutionIdmirrors upstream's non-validating brand and "exposes an infallibleFrom<String>/From<&str>". The current#[serde(transparent)]with derived infallibleFromandIntois correct per the coding guideline: "If upstream never validates, just brand for type-safety."The suggested
#[serde(try_from = "String", into = "String")]with aTryFromimpl returningInfallibleis an anti-pattern—TryFromshould only be used when validation actually occurs. The codebase confirms this distinction: validating branded types likePkgName,ComVer, andSnapshotDepRefusetry_fromwith real error enums; non-validating types likeDepPathandPkgIdWithPatchHashusetransparent. KeepPkgResolutionIdas-is.pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
145-162: LGTM!pacquet/crates/resolving-resolver-base/src/tests.rs (1)
1-1: LGTM!Also applies to: 6-7, 138-138, 185-185
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
155-160: LGTM!pacquet/crates/package-manager/src/install_without_lockfile.rs (1)
3-5: LGTM!Also applies to: 381-384
pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs (10)
1-12: LGTM!
14-24: LGTM!
26-61: LGTM!
63-75: LGTM!
109-128: LGTM!
174-209: LGTM!
220-247: LGTM!
249-279: LGTM!
281-303: LGTM!
305-352: LGTM!pacquet/crates/resolving-local-resolver/Cargo.toml (1)
1-37: LGTM!pacquet/crates/resolving-local-resolver/src/local_resolver.rs (9)
1-21: LGTM!
23-72: LGTM!
74-101: LGTM!
103-146: LGTM!
148-181: LGTM!
183-195: LGTM!
197-254: LGTM!
306-338: LGTM!
347-352: Thessricrate API usage is correct. The patternIntegrityOpts::new().algorithm(Algorithm::Sha512)followed byopts.input(&bytes)andopts.result()matches ssri 9.2.0's builder API and is consistent with existing usage throughout the codebase.
… on missing manifest fields The helper that derives `(name, version)` from a `ResolveResult`'s manifest used `.expect()` for the three lookups (manifest itself, `name` field, `version` field). The npm resolver always populates all three, so the panics are unreachable today — but the upcoming local-resolver wiring will surface them: a `file:foo.tgz` `LocalResolveResult` carries `manifest: None`, which would hit the first panic. Promote the helper to a fallible signature returning `InstallPackageFromRegistryError::UnsupportedResolution` — the same variant the install path already raises for resolver outputs it can't materialize. Callers (`install_package_from_registry::run` and `install_without_lockfile::install_subtree`) propagate it with `?`. Resolves a CodeRabbit review comment on #11778. Written by an agent (Claude Code, claude-opus-4-7).
… on missing manifest fields The helper that derives `(name, version)` from a `ResolveResult`'s manifest used `.expect()` for the three lookups (manifest itself, `name` field, `version` field). The npm resolver always populates all three, so the panics are unreachable today — but the upcoming local-resolver wiring will surface them: a `file:foo.tgz` `LocalResolveResult` carries `manifest: None`, which would hit the first panic. Promote the helper to a fallible signature returning `InstallPackageFromRegistryError::UnsupportedResolution` — the same variant the install path already raises for resolver outputs it can't materialize. Callers (`install_package_from_registry::run` and `install_without_lockfile::install_subtree`) propagate it with `?`. Resolves a CodeRabbit review comment on #11778. Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rs (1)
22-24: ⚡ Quick winAlign branded serde wiring with the repo rule (
try_from/into).Use serde conversion attributes instead of
transparentso branded-type serde behavior stays consistent with the project contract.♻️ Proposed change
-use std::fmt; +use std::{convert::Infallible, fmt}; @@ -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, From, Into)] -#[serde(transparent)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, From, Into)] +#[serde(try_from = "String", into = "String")] pub struct PkgResolutionId(String); + +impl TryFrom<String> for PkgResolutionId { + type Error = Infallible; + + fn try_from(value: String) -> Result<Self, Self::Error> { + Ok(Self::from(value)) + } +}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."🤖 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/pkg_resolution_id.rs` around lines 22 - 24, The PkgResolutionId branded type currently uses #[serde(transparent)] but must follow the repo contract to serialize/deserialize via String conversions; remove #[serde(transparent)] and add #[serde(try_from = "String", into = "String")] on the PkgResolutionId struct, then implement TryFrom<String> for PkgResolutionId (performing any validation or wrapping logic used elsewhere) so deserialization can fail cleanly, and ensure an Into<String>/From<PkgResolutionId>->String conversion exists (or keep the existing Into/From derives) so serialization uses into="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-local-resolver/src/local_resolver.rs`:
- Around line 205-220: The branch handling LocalSpecKind::File currently maps
every error from compute_tarball_integrity(...) into
ResolveLocalError::Integrity, losing pnpm-compatible missing-tarball semantics;
change the map_err call so you inspect the original tarball error returned by
compute_tarball_integrity and convert it to
ResolveLocalError::LinkedPkgDirNotFound when the underlying error represents a
missing linked package/tarball, otherwise keep ResolveLocalError::Integrity for
other failures (i.e., replace .map_err(ResolveLocalError::Integrity)? with logic
that matches the tarball error and maps the "missing tarball / linked package
dir not found" case to ResolveLocalError::LinkedPkgDirNotFound and all other
cases to ResolveLocalError::Integrity). Ensure this change is applied in the
LocalSpecKind::File branch around compute_tarball_integrity and keeps the rest
of the LocalResolveResult/TarballResolution construction unchanged.
In `@pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs`:
- Around line 253-278: resolve_path currently returns absolute spec strings
unchanged which lets normalize_components reintroduce leading ".." above a
rooted path (e.g., "file:/tmp/../pkg"), producing non-canonical
fetch_spec/PkgResolutionId; fix by always normalizing absolute inputs too and
make normalize_components treat rooted paths specially: in resolve_path, when
is_absolute_specifier(spec) is true, construct a PathBuf and pass it through
normalize_components instead of returning early; in normalize_components, detect
that the original path is absolute and, on encountering Component::ParentDir
when out.pop() fails, do not push a ".." (i.e., ignore attempts to ascend above
the root) so the returned PathBuf is a canonical absolute path.
In `@pacquet/crates/resolving-local-resolver/tests/resolve.rs`:
- Around line 365-372: The test
fail_when_resolving_from_not_existing_directory_an_injected_dependency
misconfigures the dependency flag: it creates a WantedLocalDependency with
injected: false even though the test name and intent are for an injected
dependency; update the WantedLocalDependency instance in that test to set
injected: true (i.e., change the injected field on the WantedLocalDependency
used in the test) so the test actually exercises the injected-dependency
resolution path in the resolver.
---
Nitpick comments:
In `@pacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rs`:
- Around line 22-24: The PkgResolutionId branded type currently uses
#[serde(transparent)] but must follow the repo contract to serialize/deserialize
via String conversions; remove #[serde(transparent)] and add #[serde(try_from =
"String", into = "String")] on the PkgResolutionId struct, then implement
TryFrom<String> for PkgResolutionId (performing any validation or wrapping logic
used elsewhere) so deserialization can fail cleanly, and ensure an
Into<String>/From<PkgResolutionId>->String conversion exists (or keep the
existing Into/From derives) so serialization uses into="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: 2e632927-e518-491e-8546-0853a72a00b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlpacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-local-resolver/Cargo.tomlpacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/resolving-resolver-base/Cargo.tomlpacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-resolver-base/src/tests.rs
✅ Files skipped from review due to trivial changes (3)
- pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
- pacquet/crates/resolving-local-resolver/src/lib.rs
- Cargo.toml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Compile & Lint
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-default-resolver/src/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/src/local_resolver.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.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-default-resolver/src/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-resolver-base/src/tests.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-npm-resolver/src/npm_resolver.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/crates/resolving-resolver-base/src/resolve.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-resolver-base/src/pkg_resolution_id.rspacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/src/local_resolver.rs
🔇 Additional comments (15)
pacquet/crates/resolving-resolver-base/Cargo.toml (1)
17-20: LGTM!pacquet/crates/resolving-resolver-base/src/lib.rs (1)
25-30: LGTM!pacquet/crates/resolving-resolver-base/src/resolve.rs (1)
18-19: LGTM!Also applies to: 192-200
pacquet/crates/resolving-npm-resolver/src/npm_resolver.rs (1)
30-37: LGTM!Also applies to: 347-347
pacquet/crates/resolving-default-resolver/src/tests.rs (1)
1-5: LGTM!Also applies to: 18-20
pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
5-7: LGTM!Also applies to: 46-49, 123-129, 223-479
pacquet/crates/resolving-resolver-base/src/tests.rs (1)
1-8: LGTM!Also applies to: 138-138, 185-185
pacquet/crates/package-manager/src/install_package_from_registry.rs (1)
107-107: LGTM!Also applies to: 230-260
pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
155-160: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
1-2: LGTM!Also applies to: 13-16, 18-18, 26-35, 49-49, 60-62, 65-65, 70-74, 88-90, 99-99, 107-107, 111-115, 119-120, 128-130, 134-147, 153-155, 175-197, 199-200, 203-216, 220-227, 229-259, 275-287, 289-290, 294-307, 321-377
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (1)
1-704: LGTM!pacquet/crates/resolving-local-resolver/src/chain.rs (1)
11-109: LGTM!pacquet/crates/resolving-local-resolver/Cargo.toml (1)
1-36: LGTM!pacquet/crates/resolving-local-resolver/tests/chain.rs (1)
15-92: LGTM!pacquet/crates/resolving-local-resolver/tests/resolve.rs (1)
1-364: LGTM!Also applies to: 373-473
… for missing tarballs The `file:` tarball branch wrapped every read failure in `ResolveLocalError::Integrity`, so `file:./missing.tgz` lost the pnpm-compatible error code. The directory branch already maps the same scenario to `LINKED_PKG_DIR_NOT_FOUND`; thread the tarball case through the same code by short-circuiting on `NotFound` from `compute_tarball_integrity`. Mirrors upstream's `resolveSpec` catch, where `getTarballIntegrity`'s ENOENT funnels into the same LINKED_PKG_DIR_NOT_FOUND throw the directory branch raises. Adds a `fail_when_resolving_missing_tarball_with_file_protocol` test to pin the contract. Resolves a CodeRabbit review comment on #11778. Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs (1)
253-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize absolute inputs before deriving IDs and prevent root-escape
..segments.
resolve_pathreturns absolute specs unchanged (Line 254), andnormalize_componentscan still emit leading..on rooted paths (Line 271-273). That can produce non-canonicalfetch_spec/PkgResolutionIdfor inputs likefile:/tmp/../pkgorfile:/../../pkg.Suggested fix
fn resolve_path(where_dir: &Path, spec: &str) -> PathBuf { - if is_absolute_specifier(spec) { - return PathBuf::from(spec); - } - let mut joined = where_dir.to_path_buf(); - joined.push(spec); - normalize_components(&joined) + let path = if is_absolute_specifier(spec) { + PathBuf::from(spec) + } else { + let mut joined = where_dir.to_path_buf(); + joined.push(spec); + joined + }; + normalize_components(&path) } fn normalize_components(path: &Path) -> PathBuf { let mut out = PathBuf::new(); + let mut anchored = false; for component in path.components() { match component { + Component::Prefix(_) | Component::RootDir => { + anchored = true; + out.push(component.as_os_str()); + } Component::CurDir => {} Component::ParentDir => { - if !out.pop() { + if matches!(out.components().next_back(), Some(Component::Normal(_))) { + out.pop(); + } else if !anchored { out.push(".."); } } - other => out.push(other.as_os_str()), + Component::Normal(part) => out.push(part), } } out }Also applies to: 265-277
🤖 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-local-resolver/src/parse_bare_specifier.rs` around lines 253 - 259, resolve_path currently returns absolute specifiers unchanged (is_absolute_specifier branch) which lets inputs like file:/tmp/../pkg keep un-normalized components and normalize_components can still produce leading ".." that escape the root; change resolve_path to normalize absolute inputs as well by converting the absolute specifier to a PathBuf and running normalize_components on it, then ensure the resulting PathBuf does not start with ParentDir components (i.e., strip or clamp any leading ".." for rooted paths so they cannot escape the root) before returning; apply the same normalization and root-clamping logic to the related code path around the resolve logic referenced (the block handling non-joined resolution between lines 265-277) so both absolute and relative inputs produce canonical, non-root-escaping paths used for fetch_spec / PkgResolutionId.
🤖 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.
Duplicate comments:
In `@pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs`:
- Around line 253-259: resolve_path currently returns absolute specifiers
unchanged (is_absolute_specifier branch) which lets inputs like file:/tmp/../pkg
keep un-normalized components and normalize_components can still produce leading
".." that escape the root; change resolve_path to normalize absolute inputs as
well by converting the absolute specifier to a PathBuf and running
normalize_components on it, then ensure the resulting PathBuf does not start
with ParentDir components (i.e., strip or clamp any leading ".." for rooted
paths so they cannot escape the root) before returning; apply the same
normalization and root-clamping logic to the related code path around the
resolve logic referenced (the block handling non-joined resolution between lines
265-277) so both absolute and relative inputs produce canonical,
non-root-escaping paths used for fetch_spec / PkgResolutionId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f930c0e9-c3a7-4a2a-9158-54dd88313535
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlpacquet/crates/resolving-local-resolver/Cargo.tomlpacquet/crates/resolving-local-resolver/src/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-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 (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-local-resolver/src/chain.rs
pacquet/**/{tests,test}/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <Bounds>— only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or#[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/resolving-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/tests/resolve.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-local-resolver/tests/chain.rspacquet/crates/resolving-local-resolver/src/lib.rspacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rspacquet/crates/resolving-local-resolver/src/local_resolver.rspacquet/crates/resolving-local-resolver/tests/resolve.rspacquet/crates/resolving-local-resolver/src/chain.rs
🔇 Additional comments (25)
pacquet/crates/resolving-local-resolver/Cargo.toml (1)
1-36: LGTM!pacquet/crates/resolving-local-resolver/src/lib.rs (1)
1-38: LGTM!pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs (1)
1-252: LGTM!Also applies to: 281-351
pacquet/crates/resolving-local-resolver/src/local_resolver.rs (6)
211-227: Map missing tarballs toLINKED_PKG_DIR_NOT_FOUND.This branch converts every tarball read failure into
Integrity, sofile:./missing.tgzloses the pnpm-compatible error code. TheResolveLocalError::LinkedPkgDirNotFoundvariant is documented (lines 125-127) as covering missing tarballs too.
1-22: LGTM!
23-73: LGTM!
74-107: LGTM!
109-152: LGTM!
262-358: LGTM!pacquet/crates/resolving-local-resolver/tests/resolve.rs (7)
364-381: Test name and configuration mismatch.The test is named
fail_when_resolving_from_not_existing_directory_an_injected_dependencybut setsinjected: falseat line 371. If the intent is to test injected dependency behavior, this should beinjected: true.
1-44: LGTM!
46-206: LGTM!
208-344: LGTM!
383-419: LGTM!
421-440: LGTM!
442-472: LGTM!pacquet/crates/resolving-local-resolver/src/chain.rs (5)
1-20: LGTM!
22-35: LGTM!
37-54: LGTM!
56-95: LGTM!
97-109: LGTM!pacquet/crates/resolving-local-resolver/tests/chain.rs (4)
1-25: LGTM!
27-48: LGTM!
50-70: LGTM!
72-92: LGTM!
…esolver Ports pnpm's @pnpm/resolving.local-resolver: - parse_bare_specifier mirrors parseLocalScheme/parseLocalPath/fromLocal (link:/workspace:/file: prefixes, bare path shapes, tarball-filename detection, tilde/drive-letter handling, preserveAbsolutePaths, injected directories get file: not link:). - local_resolver provides resolve_from_local_scheme / _path / resolve_latest_from_local matching upstream's three exports; ssri-based tarball integrity for file: tarballs; safe_read_package_json_from_dir for directories with the upstream fallback (warn + name=basename / version='0.0.0' for missing link: targets, LINKED_PKG_DIR_NOT_FOUND for missing file: targets, NOT_PACKAGE_DIRECTORY for ENOTDIR + Windows stat-check) and PATH_IS_UNSUPPORTED_PROTOCOL for path:. - chain.rs wraps the free functions behind the Resolver trait so the default-resolver dispatcher can compose this in alongside the npm resolver. ResolveResult.name_ver is None for local resolutions — the canonical name lives in the fetched manifest, not the resolver-time signal. 17 ported tests mirror resolving/local-resolver/test/index.ts plus 3 chain-dispatch tests verifying the trait wiring. The missing-link: target warn is emitted via tracing::warn! because pacquet's reporter doesn't yet have a generic pnpm:logger channel. Install-side wiring is left for a follow-up alongside the Stage-1 directory-fetcher integration: surfacing Directory resolutions to install_without_lockfile today would only swap the SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER error for an UnsupportedResolution one in install_package_from_registry. Written by an agent (Claude Code, claude-opus-4-7).
Doc: - pacquet_directory_fetcher intra-doc link was unresolved (resolving-local-resolver has no such dep — it's a sibling). - LocalSpecError doc linked to crate-private parse_local_scheme / parse_local_path. Dylint (perfectionist): - PkgResolutionId / WantedLocalDependency / ParseOptions / PathProtocolNotSupportedError derive lists reordered to prefix_then_alphabetical. - Single-letter closure params (|p|, |s|, |v|) renamed. - Impure expression passed to tracing::warn! bound to a let first. - Multi-line format!/assert_eq! macro invocations gained trailing commas; the single-line assert! shed its stray trailing comma. Written by an agent (Claude Code, claude-opus-4-7).
…on Windows `safe_read_package_json_from_dir` opens `<spec>/package.json` and lets the OS error surface. On Unix that's `ENOTDIR` for a file path; on Windows it's `NotFound`, so the resolver fell through to the fallback-manifest branch instead of returning `NOT_PACKAGE_DIRECTORY`. Upstream pnpm has the same gap on Windows and patches around it inside `readProjectManifestOnly` (workspace/project-manifest-reader/src/index.ts#L100-L114 at ef87f3c) by stat-checking the spec and synthesizing ENOTDIR. Mirror the check here so `link:./foo.tgz` raises NOT_PACKAGE_DIRECTORY on every platform. Written by an agent (Claude Code, claude-opus-4-7).
… for missing tarballs The `file:` tarball branch wrapped every read failure in `ResolveLocalError::Integrity`, so `file:./missing.tgz` lost the pnpm-compatible error code. The directory branch already maps the same scenario to `LINKED_PKG_DIR_NOT_FOUND`; thread the tarball case through the same code by short-circuiting on `NotFound` from `compute_tarball_integrity`. Mirrors upstream's `resolveSpec` catch, where `getTarballIntegrity`'s ENOENT funnels into the same LINKED_PKG_DIR_NOT_FOUND throw the directory branch raises. Adds a `fail_when_resolving_missing_tarball_with_file_protocol` test to pin the contract. Resolves a CodeRabbit review comment on #11778. Written by an agent (Claude Code, claude-opus-4-7).
…t_lockfile chain `install_without_lockfile`'s resolver chain was `[npm, git]` — `link:` / `file:` / `workspace:` and bare-path specifiers raised `SPEC_NOT_SUPPORTED_BY_ANY_RESOLVER` even though the `resolving-local-resolver` crate that handles them has been ported. Slot `LocalResolver` in after git, mirroring upstream's `createResolver` order (npm → jsr → git → tarball → local-scheme → … → local-path). Pacquet doesn't expose `preserveAbsolutePaths` through `Config` yet so the context defaults to `false`; once that setting lands in `pacquet-config` the install path can thread it through. The install pass still can't materialise Directory resolutions — the tarball-shaped install path raises `UnsupportedResolution` — but the resolver chain now correctly dispatches to the local resolver, so the failure mode moves one step closer to the install-side gap that's tracked by the Stage-1 `directory-fetcher` integration. Written by an agent (Claude Code, claude-opus-4-7).
Local cargo fmt formatted the `fail_when_resolving_missing_tarball_with_file_protocol` struct literal on one line; the CI Format job (`cargo fmt --all -- --check`) disagreed. Re-run fmt locally to flush the diff. Written by an agent (Claude Code, claude-opus-4-7).
Summary
@pnpm/resolving.local-resolvertopacquet/crates/resolving-local-resolver—file:(directory + tarball),link:,workspace:, bare-path, and injected-dep handling, with matching error codes (PATH_IS_UNSUPPORTED_PROTOCOL,LINKED_PKG_DIR_NOT_FOUND,NOT_PACKAGE_DIRECTORY).ResolveResult.idinresolving-resolver-basefromPkgNameVerto a brand-onlyPkgResolutionId(Stringnewtype) so resolvers whose ids are notname@version(the local resolver'slink:..,file:..) fit; npm-side consumers (install_package_from_registry,install_without_lockfile,resolve_dependency_tree) now derive name+version from the resolver-supplied manifest.resolving/local-resolver/test/index.tsplus 3 chain-dispatch tests proving theResolver-trait wiring composes intoDefaultResolver.Notes
directory-fetcherintegration. PluggingLocalResolverintoinstall_without_lockfile.rstoday would only swap one error site for another (the install pass can't materialize Directory resolutions yet).link:-target warn is emitted viatracing::warn!because pacquet's reporter doesn't yet have a genericpnpm:loggerchannel. Same payload shape (message+prefix); upgradable to a wire-faithful emit once that channel exists.ef87f3ccff.Test plan
cargo nextest run -p pacquet-resolving-local-resolver— 20 passingcargo nextest run -p pacquet-resolving-resolver-base -p pacquet-resolving-npm-resolver -p pacquet-resolving-default-resolver -p pacquet-resolving-deps-resolver— 146 passingcargo nextest run -p pacquet-package-manager --lib— 264 passing (registry-mock-dependent tests requirejust registry-mock launch)cargo clippy --workspace --all-targets -- --deny warningscargo fmt --check+taplo format --checktypos pacquet/crates/resolving-local-resolver/ pacquet/crates/resolving-resolver-base/ pacquet/crates/resolving-npm-resolver/ pacquet/crates/resolving-default-resolver/ pacquet/crates/resolving-deps-resolver/ pacquet/crates/package-manager/Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit