perf(pacquet): unlock no-op short-circuit + port abbreviated-modified verifier shortcut#11931
Merged
Merged
Conversation
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? |
📝 WalkthroughWalkthroughExtracts a filesystem-free lexical_normalize into pacquet-fs and migrates cmd-shim, store-dir, and modules-yaml to use it; separately, expands npm publish-time verification into a 4-layer resolution flow with abbreviated metadata caching, local mirror support, and tests for the shortcut. ChangesLexical Path Normalization Extraction and Application
NPM Package Publish-Time Resolution Optimization
Sequence DiagramsequenceDiagram
participant Verifier
participant AbbrevCache
participant LocalMirror
participant Attestation
participant FullMetadata
Verifier->>AbbrevCache: fetch_abbreviated_meta(name, registry)
alt abbreviated meta hit and pinned version present and older than cutoff
AbbrevCache-->>Verifier: AbbreviatedMeta (modified, versionNames)
Verifier-->>Verifier: try_abbreviated_modified_shortcut()
else miss or inconclusive
AbbrevCache-->>Verifier: None
Verifier->>LocalMirror: read_local_meta_time(registry, name)
alt local mirror has per-version time
LocalMirror-->>Verifier: PublishedAtTimeMap
else
LocalMirror-->>Verifier: None
Verifier->>Attestation: fetch_attestation_timestamp(version)
alt attestation available
Attestation-->>Verifier: timestamp
else
Attestation-->>Verifier: None
Verifier->>FullMetadata: fetch_full_metadata(name, registry)
FullMetadata-->>Verifier: Package (per-version time)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11931 +/- ##
==========================================
+ Coverage 87.78% 87.79% +0.01%
==========================================
Files 224 224
Lines 27295 27395 +100
==========================================
+ Hits 23961 24052 +91
- Misses 3334 3343 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The lexical normalisation helper (collapse `.` and `..` segments without touching the filesystem) had two near-identical copies in `pacquet-cmd-shim` (`path_util::lexical_normalize`) and `pacquet-store-dir` (`project_registry::lexical_normalize`). A third site (`pacquet-modules-yaml`) needs it for the no-op-short-circuit fix in the following commit. Lift the function into `pacquet-fs`, re-export the symbol from both prior call sites, and drop the duplicated tests in `project_registry` — the new home in `pacquet-fs` carries the unified coverage (Node-`path.join`-compatible collapse, root-with- no-parent, leading `..` preservation, and the `<modules_dir>/../../store` round-trip that motivates the move). No behaviour change. --- Written by an agent (Claude Code, claude-opus-4-7).
…-op short-circuit fires `read_modules_manifest` joins a stored relative `virtualStoreDir` with `modules_dir` to recover an absolute path, mirroring upstream's `path.join(modulesDir, modules.virtualStoreDir)` at <https://github.com/pnpm/pnpm/blob/1819226b51/installing/modules-yaml/src/index.ts#L66-L70>. Node's `path.join` collapses interior `..` segments; Rust's `PathBuf::join` does not. Stored values like `../../../Users/zoltan/Library/pnpm/store/v11/links` therefore came back as `<modules_dir>/../../../Users/.../links` instead of the collapsed absolute form. The fallout: `Install::run`'s no-op short-circuit (added in #11904) compares this recovered path byte-for-byte against `Config::effective_virtual_store_dir()`, and the unnormalised form never matched the normalised config side. Every install whose store sits outside the project (the default `~/.local/share/pnpm/store` setup on macOS/Linux) silently skipped the fast path and re-ran the full materialisation pipeline on a clean install. Route the joined path through `pacquet_fs::lexical_normalize`. The new `round_trip_recovers_normalized_absolute_for_non_descendant_store` test pins the round-trip; verified to fail without this change. The no-op short-circuit now fires for the `lockfile+node_modules`-style cells in <https://benchmarks.vlt.sh>; the residual gap to pnpm on those cells is the resolution-verifier cold path (the abbreviated-modified shortcut + on-disk mirror layers upstream uses but pacquet has not yet ported — see the `Phase 4 stubs the abbreviated-shortcut and on-disk-mirror layers` comment in `create_npm_resolution_verifier`). --- Written by an agent (Claude Code, claude-opus-4-7).
…al-mirror layers of the publish-timestamp lookup The npm resolution verifier walked a 4-layer fallback chain in upstream pnpm (abbreviated-modified shortcut → on-disk full-meta mirror → npm attestation endpoint → full packument fetch); pacquet only had the last two. Every cold lockfile-verification pass paid a full-meta GET per name even when the abbreviated form's package-level `modified` would have decided the gate, and even when a previous install's on-disk mirror could have answered without any network call. Port the missing two layers verbatim, mirroring upstream's [`createNpmResolutionVerifier.ts`](https://github.com/pnpm/pnpm/blob/2a9bd897bf/resolving/npm-resolver/src/createNpmResolutionVerifier.ts): * `try_abbreviated_modified_shortcut` projects the abbreviated packument down to `(modified, versionNames)` and returns the `modified` timestamp when it's strictly older than the policy cutoff and the pinned version is still listed. * `fetch_abbreviated_meta` reads the resolver's shared `PackageMetaCache` (preferring the `name:full` entry, falling back to bare `name`) before issuing `fetch_full_metadata_cached(.., full_metadata: false)`. * `read_local_meta_time` reads `<cache_dir>/v11/metadata-full/<registry>/<name>.jsonl` via the existing mirror helpers and pulls the per-version `time` map. Both layers cache their result per-(registry, name) in `PublishedAtLookupContext`, so a multi-thousand-entry lockfile costs at most one disk/network round-trip per package across both layers combined. ## Bench 5-iteration cold-cache measured pass on the `benchmarks.vlt.sh/fixtures/svelte` fixture (`pnpm-lock.yaml` + `node_modules` present, `~/.cache/pnpm` and store wiped before each run), 10-core M-series Mac: | | pnpm | pacquet (before) | pacquet (after) | |--------------|------:|-----------------:|----------------:| | wall time | 0.54 s | 2.16 s | 0.71 s | That's a 3.0× drop on the verifier-dominated section of the `lockfile+node_modules` cell. Pacquet still trails pnpm by ~150 ms of remaining overhead in the install bootstrap; see [#11902] (#11902) for the residual. --- Written by an agent (Claude Code, claude-opus-4-7).
Two broken links the workspace Doc job rejects under -D warnings: * `[\`PathBuf::join\`]` → `[\`Path::join\`]`. PathBuf inherits `join` via `Deref<Target=Path>`; rustdoc resolves intra-doc links against the inherent surface and refuses to follow Deref. * `[\`fetch_full_metadata_cached\`]` → adds `()` so the resolver picks the function over the module of the same name.
Contributor
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.9579765358200003,
"stddev": 0.12530717459890744,
"median": 1.9256480069200002,
"user": 2.73864602,
"system": 3.2225514399999993,
"min": 1.8513160774200002,
"max": 2.2832961564199996,
"times": [
1.96902631942,
1.9371112494200002,
2.03075756842,
1.9016638414200002,
1.9209830844200002,
1.8822983434200002,
1.87299978842,
2.2832961564199996,
1.8513160774200002,
1.93031292942
]
},
{
"command": "pacquet@main",
"mean": 1.9013875214199998,
"stddev": 0.033527263014724626,
"median": 1.8924498999200001,
"user": 2.7662847200000007,
"system": 3.2084858399999994,
"min": 1.85746967242,
"max": 1.9560729164200001,
"times": [
1.87806946942,
1.8898279874200001,
1.8950718124200001,
1.90869840742,
1.88214893042,
1.9379585844200002,
1.94103773442,
1.85746967242,
1.86751969942,
1.9560729164200001
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6273684654200001,
"stddev": 0.0202091057799399,
"median": 0.6226947820200001,
"user": 0.35009424,
"system": 1.33270446,
"min": 0.6099262010200001,
"max": 0.67544364402,
"times": [
0.67544364402,
0.6494766580200001,
0.6099262010200001,
0.6233588010200001,
0.6262450180200001,
0.62203076302,
0.6175223150200001,
0.62351166402,
0.61138989702,
0.61477969302
]
},
{
"command": "pacquet@main",
"mean": 0.6318833114200001,
"stddev": 0.01157253050578361,
"median": 0.62974106352,
"user": 0.35391243999999994,
"system": 1.3362533599999997,
"min": 0.61644496002,
"max": 0.6571867610200001,
"times": [
0.6571867610200001,
0.62888270802,
0.6265202460200001,
0.64021851902,
0.6187069020200001,
0.63500979002,
0.6285514430200001,
0.63671236602,
0.61644496002,
0.63059941902
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4556578183599997,
"stddev": 0.02148277171937394,
"median": 2.45365729656,
"user": 4.3872107,
"system": 3.2262793199999997,
"min": 2.42834621906,
"max": 2.48999839006,
"times": [
2.42834621906,
2.44621285706,
2.42850608306,
2.4571311530599997,
2.44007343606,
2.4817326770599997,
2.45471623706,
2.4525983560599998,
2.48999839006,
2.47726277506
]
},
{
"command": "pacquet@main",
"mean": 2.4621726611599994,
"stddev": 0.023280515746816156,
"median": 2.46896757606,
"user": 4.388807299999999,
"system": 3.2521375199999993,
"min": 2.42588750406,
"max": 2.48994629106,
"times": [
2.4816010560599997,
2.4604714360599997,
2.48994629106,
2.4741255170599996,
2.4842798030599997,
2.43954314406,
2.46979344806,
2.42588750406,
2.42793670806,
2.4681417040599998
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.6554027207800002,
"stddev": 0.019442598705488947,
"median": 1.65633920558,
"user": 2.00538632,
"system": 2.01330324,
"min": 1.6244346405799999,
"max": 1.69377227558,
"times": [
1.65369668058,
1.64628608358,
1.6594490285799999,
1.6584003455799998,
1.63159605058,
1.69377227558,
1.6717339255800001,
1.66038011158,
1.65427806558,
1.6244346405799999
]
},
{
"command": "pacquet@main",
"mean": 1.6658088180799997,
"stddev": 0.04729913627441496,
"median": 1.6466232160799998,
"user": 1.99072912,
"system": 2.02980224,
"min": 1.6177670735799998,
"max": 1.7629812545799999,
"times": [
1.66227760458,
1.73378861258,
1.6406750875799998,
1.63379435958,
1.7629812545799999,
1.6177670735799998,
1.63418850658,
1.63938257458,
1.68066176258,
1.6525713445799999
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/11931 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,455.66 ms(-37.82%)Baseline: 3,949.30 ms | 4,739.16 ms (51.82%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,655.40 ms(-44.45%)Baseline: 2,980.22 ms | 3,576.27 ms (46.29%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,957.98 ms(-12.17%)Baseline: 2,229.41 ms | 2,675.29 ms (73.19%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 627.37 ms(-4.25%)Baseline: 655.18 ms | 786.22 ms (79.80%) |
4e50fcc to
4f81169
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`:
- Around line 560-589: The concurrent-cache-miss bug in fetch_abbreviated_meta
causes duplicate fetches because the abbreviated_meta lock is released before
awaiting fetch_full_metadata_cached; fix by storing an in-flight promise in
abbreviated_meta so concurrent callers share the same work: when missing, create
and insert a shared future/promise (e.g., a oneshot, async lock or
futures::future::Shared) keyed by package_key before performing
read_shared_meta/fetch_full_metadata_cached, then await that shared handle
outside the map; ensure the insertion uses the same projection path
(project_abbreviated_meta) and that on error the stored in-flight entry is
removed or resolved to None so subsequent calls can retry—apply this change
around fetch_abbreviated_meta (and the analogous block using
read_shared_meta/project_abbreviated_meta and fetch_full_metadata_cached) so all
callers await the single in-flight computation.
🪄 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: 4c7bd42d-60d3-4ce6-bf1e-1e636b4a2714
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
pacquet/crates/cmd-shim/Cargo.tomlpacquet/crates/cmd-shim/src/path_util.rspacquet/crates/fs/src/lexical_normalize.rspacquet/crates/fs/src/lib.rspacquet/crates/modules-yaml/Cargo.tomlpacquet/crates/modules-yaml/src/lib.rspacquet/crates/modules-yaml/tests/real_fs.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/store-dir/src/project_registry.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- 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/cmd-shim/src/path_util.rspacquet/crates/fs/src/lib.rspacquet/crates/modules-yaml/src/lib.rspacquet/crates/modules-yaml/tests/real_fs.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/fs/src/lexical_normalize.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/store-dir/src/project_registry.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/modules-yaml/tests/real_fs.rs
🧠 Learnings (3)
📚 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/cmd-shim/src/path_util.rspacquet/crates/fs/src/lib.rspacquet/crates/modules-yaml/src/lib.rspacquet/crates/modules-yaml/tests/real_fs.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/fs/src/lexical_normalize.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/store-dir/src/project_registry.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/cmd-shim/src/path_util.rspacquet/crates/fs/src/lib.rspacquet/crates/modules-yaml/src/lib.rspacquet/crates/modules-yaml/tests/real_fs.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/fs/src/lexical_normalize.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/store-dir/src/project_registry.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/cmd-shim/src/path_util.rspacquet/crates/fs/src/lib.rspacquet/crates/modules-yaml/src/lib.rspacquet/crates/modules-yaml/tests/real_fs.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/fs/src/lexical_normalize.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/store-dir/src/project_registry.rs
🧬 Code graph analysis (6)
pacquet/crates/cmd-shim/src/path_util.rs (1)
pacquet/crates/fs/src/lexical_normalize.rs (1)
lexical_normalize(24-40)
pacquet/crates/fs/src/lib.rs (1)
pacquet/crates/fs/src/lexical_normalize.rs (7)
lexical_normalize(24-40)collapses_parent_dir_segments(48-50)drops_parent_dir_at_root(53-56)preserves_leading_parent_dir_when_unanchored(59-62)drops_current_dir_segments(65-68)collapses_unanchored_absolute_join(71-79)empty_path_is_empty(82-84)
pacquet/crates/modules-yaml/tests/real_fs.rs (1)
pacquet/crates/modules-yaml/tests/index.rs (1)
manifest_from_json(15-17)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (4)
pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (3)
AbbreviatedMetaProjection(41-44)package_key(72-74)PublishedAtTimeMap(28-28)pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs (2)
FetchFullMetadataCachedOptions(41-59)fetch_full_metadata_cached(93-199)pacquet/crates/resolving-npm-resolver/src/mirror.rs (2)
get_pkg_mirror_path(98-107)load_meta_async(258-261)core/constants/src/index.ts (1)
FULL_META_DIR(22-22)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)
pacquet/crates/resolving-npm-resolver/src/trust_checks/tests.rs (1)
now_at(61-63)
pacquet/crates/store-dir/src/project_registry.rs (1)
pacquet/crates/fs/src/lexical_normalize.rs (1)
lexical_normalize(24-40)
🔇 Additional comments (11)
pacquet/crates/fs/src/lexical_normalize.rs (1)
1-85: LGTM!pacquet/crates/fs/src/lib.rs (1)
2-2: LGTM!Also applies to: 6-6
pacquet/crates/cmd-shim/Cargo.toml (1)
14-15: LGTM!pacquet/crates/cmd-shim/src/path_util.rs (1)
4-4: LGTM!pacquet/crates/store-dir/src/project_registry.rs (1)
19-19: LGTM!pacquet/crates/modules-yaml/Cargo.toml (1)
15-15: LGTM!pacquet/crates/modules-yaml/src/lib.rs (1)
14-14: LGTM!Also applies to: 448-457
pacquet/crates/modules-yaml/tests/real_fs.rs (1)
40-66: LGTM!pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (1)
15-18: LGTM!Also applies to: 30-44, 60-61
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (1)
467-537: LGTM!Also applies to: 911-923
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)
649-666: LGTM!Also applies to: 676-700, 711-751, 762-809
8 tasks
This was referenced May 25, 2026
This was referenced Jun 7, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes that together unlock pnpm-parity on the
benchmarks.vlt.shlockfile+node_modulesshape — the row wherepacquet was 2-12× slower than pnpm on every fixture.
1.
fix(modules-yaml): normalise joinedvirtualStoreDirread_modules_manifestjoins a stored relativevirtualStoreDirwith
modules_dirto recover an absolute path, mirroring upstream'spath.join(modulesDir, modules.virtualStoreDir). Node'spath.joinnormalises interior
..segments; Rust'sPathBuf::joindoes not.Stored values like
../../../Users/.../store/v11/linkscame backas
<modules_dir>/../../../Users/.../links— never byte-matchedConfig::effective_virtual_store_dir(), so the no-op short-circuitadded in #11904 silently missed every install whose store sits
outside the project (the default macOS / Linux setup).
The accompanying refactor lifts
lexical_normalize(alreadyduplicated in
cmd-shimandstore-dir) intopacquet-fssomodules-yamldoesn't make it a third copy.2.
perf(resolving-npm-resolver): port the missing verifier layersThe npm resolution verifier walked a 4-layer fallback chain in
upstream pnpm (abbreviated-modified shortcut → on-disk full-meta
mirror → npm attestation endpoint → full packument fetch); pacquet
only had the last two. The module's doc-comment explicitly noted
"Phase 4 stubs the abbreviated-shortcut and on-disk-mirror layers
(no cached fetcher / no mirror yet); Phase 5 ports
fetchFullMetadataCached.ts…" — this is Phase 5.Result: a cold lockfile-verification pass now pays at most one
abbreviated GET per name (small payload, decided by package-level
modified) instead of a full-meta GET per name (hundreds of KBeach).
Bench
5-iteration cold-cache measured pass on
vltpkg/benchmarks/fixtures/svelte(
pnpm-lock.yaml+node_modulespresent,~/.cache/pnpmandstore wiped before each run), 10-core M-series Mac:
3.0× faster on the
lockfile+node_modulesrow.Test plan
cargo nextest run -p pacquet-fs -p pacquet-modules-yaml -p pacquet-cmd-shim -p pacquet-store-dir(252/252 pass)cargo nextest run -p pacquet-resolving-npm-resolver -p pacquet-lockfile-verification(235/235 pass)round_trip_recovers_normalized_absolute_for_non_descendant_store,min_age_pass_via_abbreviated_modified_shortcut,min_age_shortcut_falls_through_when_modified_within_cutoff,min_age_shortcut_falls_through_when_version_not_listed. Each verified to fail without the corresponding production change.cargo fmtcleanjust lintcleanOut of scope (follow-ups)
The residual ~150 ms gap between pacquet (
0.71 s) and pnpm(
0.54 s) on this cell is install-bootstrap overhead, notverifier work. Belongs in #11902.
Related issues: #11899 (no-op short-circuit), #11851 (broader
syscall contention), #11902 (vlt.sh tracker).
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
.and..) are handled consistently, fixing edge cases when resolving project and store paths.Performance Improvements
Tests