Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 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). (10)
📝 WalkthroughWalkthroughThreads the resolve-time shared tarball in-flight/completed mem-cache into the fresh-lockfile cold-batch, restricts mem-cache reuse to registry resolutions with a sibling-fetch-failure fallback to the standalone path, adds Clone for a tarball download handle, updates docs, and expands tests for coordination and fallback behavior. ChangesTarball Memory Cache Coordination in Fresh-Lockfile Install Pipeline
Sequence Diagram(s)sequenceDiagram
participant PrefetchingResolver
participant TarballPrefetcher
participant TarballMemCache
participant CreateVirtualStore
participant InstallPackageBySnapshot
participant TarballFetcher
PrefetchingResolver->>TarballMemCache: insert in-flight/completed bytes (fresh resolve)
TarballPrefetcher->>TarballMemCache: insert in-flight/completed bytes (frozen path)
CreateVirtualStore->>InstallPackageBySnapshot: start cold-batch with Some(&tarball_mem_cache)
InstallPackageBySnapshot->>TarballMemCache: check for in-flight/completed slot (registry resolution)
alt mem-cache yields CAS
TarballMemCache-->>InstallPackageBySnapshot: return cached CAS bytes
else mem-cache reports SiblingFetchFailed
InstallPackageBySnapshot->>InstallPackageBySnapshot: retry via run_without_mem_cache
InstallPackageBySnapshot->>TarballFetcher: perform direct download/materialize
else other mem-cache error or no entry
InstallPackageBySnapshot->>TarballFetcher: perform direct download/materialize
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Contributor
Integrated-Benchmark Report (Linux)Each scenario has pacquet rows (direct install) and pnpr rows (the same client through the pnpr install accelerator), so pnpr@HEAD vs pacquet@HEAD is the pnpr-vs-direct ratio. Cold-store scenarios wipe the client store between runs (warm server); hot-store scenarios keep it warm. The pacquet@HEAD rows feed the pacquet Bencher testbed; the pnpr@HEAD rows feed the pnpr testbed. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 10.195945202180003,
"stddev": 0.16496463498425062,
"median": 10.18712861438,
"user": 3.6975133999999996,
"system": 4.379532039999999,
"min": 10.00663101588,
"max": 10.41584017988,
"times": [
10.01939793788,
10.11065725888,
10.314612053880001,
10.38464533688,
10.05497262888,
10.00663101588,
10.41584017988,
10.03940362388,
10.263599969880001,
10.34969201588
]
},
{
"command": "pacquet@main",
"mean": 10.141770678579999,
"stddev": 0.15884205959229272,
"median": 10.11355466238,
"user": 3.7240243,
"system": 4.362654839999999,
"min": 9.965541570880001,
"max": 10.43736009688,
"times": [
10.31149200688,
10.01380146088,
9.965541570880001,
10.04001432788,
10.14450975688,
10.43736009688,
10.15728180388,
10.289384767880001,
9.97572142588,
10.082599567880001
]
},
{
"command": "pnpr@HEAD",
"mean": 5.29867822788,
"stddev": 0.11789778827729781,
"median": 5.25671467688,
"user": 3.0317899,
"system": 4.146375239999999,
"min": 5.1991718668799995,
"max": 5.59469875988,
"times": [
5.25780375688,
5.20765636588,
5.362395686879999,
5.59469875988,
5.22847898988,
5.1991718668799995,
5.3050364408799995,
5.230940094879999,
5.25562559688,
5.34497471988
]
},
{
"command": "pnpr@main",
"mean": 5.3829648380799995,
"stddev": 0.13812040814532556,
"median": 5.34939137438,
"user": 3.1901504000000003,
"system": 4.39416304,
"min": 5.24632089388,
"max": 5.69740834888,
"times": [
5.69740834888,
5.54406351688,
5.340877293879999,
5.38850224588,
5.377725809879999,
5.30852366188,
5.24632089388,
5.30249979888,
5.26582135588,
5.35790545488
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.73686302516,
"stddev": 0.018227173019680222,
"median": 0.73690724746,
"user": 0.41948142,
"system": 1.3921682399999997,
"min": 0.71105729896,
"max": 0.77101354796,
"times": [
0.77101354796,
0.74616188996,
0.74432534096,
0.71105729896,
0.72570301796,
0.71532994796,
0.73399831596,
0.73981617896,
0.72678379996,
0.75444091296
]
},
{
"command": "pacquet@main",
"mean": 0.7730912131600001,
"stddev": 0.03917513174437962,
"median": 0.77060941046,
"user": 0.42699381999999997,
"system": 1.4054957399999999,
"min": 0.72587647996,
"max": 0.84796917496,
"times": [
0.75276700196,
0.84796917496,
0.73784173296,
0.81634885996,
0.76658270996,
0.79949175996,
0.77740578796,
0.77463611096,
0.72587647996,
0.73199251296
]
},
{
"command": "pnpr@HEAD",
"mean": 0.81815557966,
"stddev": 0.028419789870143142,
"median": 0.8155102269600001,
"user": 0.42021341999999995,
"system": 1.37064384,
"min": 0.78556230796,
"max": 0.88821255996,
"times": [
0.88821255996,
0.81877559096,
0.81122487296,
0.83129562596,
0.82160543596,
0.81739077896,
0.81362967496,
0.78845265296,
0.78556230796,
0.80540629596
]
},
{
"command": "pnpr@main",
"mean": 0.80846319396,
"stddev": 0.08644043502382533,
"median": 0.78269559146,
"user": 0.38873952,
"system": 1.32647104,
"min": 0.75560799196,
"max": 1.04596516396,
"times": [
0.83170075996,
0.79840410096,
1.04596516396,
0.79187479296,
0.7561010969599999,
0.75560799196,
0.78860147396,
0.77678970896,
0.77006491496,
0.76952193496
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 5.475471990579999,
"stddev": 0.03931149982151178,
"median": 5.48882293198,
"user": 3.89780394,
"system": 3.4029842199999996,
"min": 5.41531508298,
"max": 5.52824352198,
"times": [
5.48563496698,
5.44054916498,
5.49509847698,
5.52824352198,
5.42896787198,
5.41531508298,
5.49201089698,
5.506234908980001,
5.4470866869800005,
5.51557832698
]
},
{
"command": "pacquet@main",
"mean": 5.4264796995800015,
"stddev": 0.048015242621940075,
"median": 5.41802740048,
"user": 3.8130075399999996,
"system": 3.3337693199999996,
"min": 5.34085022198,
"max": 5.49540592798,
"times": [
5.41749392998,
5.47443509198,
5.49540592798,
5.34085022198,
5.47965876698,
5.43678350198,
5.371451119980001,
5.4145889919800005,
5.415568571980001,
5.41856087098
]
},
{
"command": "pnpr@HEAD",
"mean": 2.05173433878,
"stddev": 0.04921738786953635,
"median": 2.0476350844800004,
"user": 2.6522662400000003,
"system": 3.2593940199999993,
"min": 1.98230830698,
"max": 2.15573178598,
"times": [
2.0628715409800003,
2.15573178598,
2.0139837439800004,
2.0402191859800003,
2.05505098298,
2.02111984398,
2.06424419398,
2.10097533898,
2.02083846398,
1.98230830698
]
},
{
"command": "pnpr@main",
"mean": 2.0509486404800006,
"stddev": 0.03395429477458233,
"median": 2.0463552209800002,
"user": 2.63762294,
"system": 3.2636986199999996,
"min": 2.01078460098,
"max": 2.10289417798,
"times": [
2.0933936599800003,
2.0856409579800004,
2.0153339269800004,
2.0122649749800003,
2.0526065579800004,
2.0571612319800003,
2.01078460098,
2.0393024319800004,
2.10289417798,
2.04010388398
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4321151364599998,
"stddev": 0.011675048205504798,
"median": 1.42869407966,
"user": 1.52029166,
"system": 1.7926391,
"min": 1.41562509716,
"max": 1.45079826716,
"times": [
1.43917280316,
1.42315436016,
1.43696695416,
1.42802541116,
1.44953379416,
1.42358911116,
1.45079826716,
1.42492281816,
1.42936274816,
1.41562509716
]
},
{
"command": "pacquet@main",
"mean": 1.46287303476,
"stddev": 0.08816758370612358,
"median": 1.44063994266,
"user": 1.53120386,
"system": 1.8112119999999998,
"min": 1.40573202016,
"max": 1.71017421916,
"times": [
1.41858175016,
1.71017421916,
1.45616475616,
1.44277277716,
1.44679316616,
1.42645723816,
1.43633047716,
1.44721683516,
1.43850710816,
1.40573202016
]
},
{
"command": "pnpr@HEAD",
"mean": 0.67470640316,
"stddev": 0.015359931015872325,
"median": 0.67468609766,
"user": 0.31560146,
"system": 1.2730422999999997,
"min": 0.65614032116,
"max": 0.71272838716,
"times": [
0.66422887216,
0.6752481101600001,
0.71272838716,
0.67482576916,
0.66377222616,
0.65614032116,
0.6818926121600001,
0.67603838216,
0.66764292516,
0.67454642616
]
},
{
"command": "pnpr@main",
"mean": 0.6929808747599999,
"stddev": 0.08688357271245124,
"median": 0.66539869716,
"user": 0.33021266000000005,
"system": 1.2620797,
"min": 0.6523608991600001,
"max": 0.93837207316,
"times": [
0.65980902816,
0.65291148116,
0.66218807916,
0.65983012816,
0.66860931516,
0.67241839516,
0.6523608991600001,
0.93837207316,
0.68642563316,
0.67688371516
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot storeResolution-only: cold packument cache (full re-resolve over the registry link) with a hot store (no tarball download), so this isolates pnpr offloading the client resolution to its warm server.
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 5.01815254598,
"stddev": 0.02369255973351528,
"median": 5.01949293538,
"user": 1.74750506,
"system": 1.9397624199999999,
"min": 4.96910074988,
"max": 5.04653902788,
"times": [
5.04070260588,
5.0449569498799995,
5.00240884588,
5.0228882518799995,
5.00110421788,
4.96910074988,
5.023711984879999,
5.01401520688,
5.01609761888,
5.04653902788
]
},
{
"command": "pacquet@main",
"mean": 5.034480590679999,
"stddev": 0.023430624849304846,
"median": 5.02343086538,
"user": 1.7402515600000001,
"system": 1.95980752,
"min": 5.01267409188,
"max": 5.078202848879999,
"times": [
5.04099983288,
5.01267409188,
5.02895478388,
5.014145699879999,
5.01764483688,
5.06498984588,
5.017639840879999,
5.078202848879999,
5.017906946879999,
5.05164717888
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6697122674800001,
"stddev": 0.009564135860613963,
"median": 0.66588185138,
"user": 0.32670886,
"system": 1.27541022,
"min": 0.6614860988800001,
"max": 0.6896653808800001,
"times": [
0.66213631788,
0.6649372398800001,
0.6830870598800001,
0.6638125128800001,
0.67111846188,
0.6626198478800001,
0.6896653808800001,
0.6614860988800001,
0.66682646288,
0.67143329188
]
},
{
"command": "pnpr@main",
"mean": 0.6798450204800001,
"stddev": 0.029045337005894806,
"median": 0.67587656238,
"user": 0.33107256,
"system": 1.2730396199999998,
"min": 0.6584550678800001,
"max": 0.7588796188800001,
"times": [
0.6762047128800001,
0.67711806188,
0.6832425038800001,
0.6650166268800001,
0.6584550678800001,
0.6613268558800001,
0.66326221188,
0.7588796188800001,
0.6793961328800001,
0.67554841188
]
}
]
} |
Contributor
Bencher Report
Review Summary by QodoCoordinate fresh-resolve cold batch with prefetcher's in-flight downloads
WalkthroughsDescription• Thread tarball_mem_cache into fresh-resolve cold batch to reuse prefetcher's in-flight downloads • Prevents double-download race when prefetch row hasn't committed to store index yet • Cold batch now routes through run_with_mem_cache when prefetcher is active • Add deterministic tests verifying mem-cache coordination and None path behavior Diagramflowchart LR
A["PrefetchingResolver<br/>streams tarball"] -->|"stores in"| B["tarball_mem_cache"]
C["CreateVirtualStore<br/>cold batch"] -->|"Some(&cache)"| D["run_with_mem_cache"]
D -->|"reuses or parks on"| B
E["Frozen path<br/>no prefetcher"] -->|"None"| F["run_without_mem_cache"]
File Changes1. pacquet/crates/package-manager/src/create_virtual_store.rs
|
…e prefetcher On the fresh-resolve install path the same registry tarball could be downloaded twice: once by the resolve-time `PrefetchingResolver` and again by `CreateVirtualStore`'s cold batch. The shared `tarball_mem_cache` infrastructure already exists (added in #12245 for the pnpr/frozen path), but the fresh-lockfile call site threaded `None`, so the cold batch went through `run_without_mem_cache` and never observed the in-flight prefetch — its only on-disk dedup is the store-index row, which the prefetcher's writer commits asynchronously, so a snapshot whose row hasn't committed yet re-downloaded the same bytes. Thread `Some(&tarball_mem_cache)` from the fresh-lockfile path so the cold batch reuses a finished prefetch (`CacheValue::Available`) or briefly parks on the per-URL `Notify` for an in-flight one. Two refinements keep this safe: - Coordinate only registry resolutions. The `PrefetchingResolver` skips remote tarballs (they resolve with no `name_ver`); their only mem-cache entry comes from the resolver's download-to-resolve, keyed by `name@version`, while the lockfile and this pass address them by `name@<url>`. Reusing that entry would skip writing the `name@<url>` store-index row a later re-resolve needs to reuse the warm store (regressed `remote_tarball_reresolves_from_warm_store_without_refetch`). - Fall back to a standalone retried download when the best-effort prefetch failed (`SiblingFetchFailed`) instead of inheriting the failure, so a transient prefetch error no longer fails the install. Closes #12241
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install_package_by_snapshot.rs (1)
52-55: ⚡ Quick winDoc contract is out of sync with the registry-only mem-cache gate
Lines 53-55 still describe
registry/tarballrouting throughrun_with_mem_cache, but Line 321 gates that path toLockfileResolution::Registryonly. Please update the field doc to reflect registry-only behavior.As per coding guidelines, “Doc comments (
///,//!) document the contract — preconditions, postconditions, panics, and the reason the function exists; they are not a re-narration of the body.”Also applies to: 321-322
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pacquet/crates/package-manager/src/install_package_by_snapshot.rs` around lines 52 - 55, Update the field doc for the install-scoped in-flight tarball cache to state it is only used for registry/tarball downloads when LockfileResolution::Registry is active (i.e., the registry-only mem-cache gate), and adjust the adjacent doc at the place referencing DownloadTarballToStore::run_with_mem_cache (around the other comment at 321-322) to match that precondition so the comment documents the contract (registry-only) rather than re-describing the implementation.Source: Coding guidelines
🤖 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_by_snapshot.rs`:
- Around line 319-322: The match is moving out of metadata.resolution (a
non-Copy field); change the guard to match on a reference instead — replace
matches!(metadata.resolution, LockfileResolution::Registry(_)) with
matches!(&metadata.resolution, LockfileResolution::Registry(_)) (i.e., borrow
metadata.resolution in the condition) so the code in the tarball_mem_cache /
raw_cas_paths branch does not attempt to move out of &PackageMetadata.
---
Nitpick comments:
In `@pacquet/crates/package-manager/src/install_package_by_snapshot.rs`:
- Around line 52-55: Update the field doc for the install-scoped in-flight
tarball cache to state it is only used for registry/tarball downloads when
LockfileResolution::Registry is active (i.e., the registry-only mem-cache gate),
and adjust the adjacent doc at the place referencing
DownloadTarballToStore::run_with_mem_cache (around the other comment at 321-322)
to match that precondition so the comment documents the contract (registry-only)
rather than re-describing the implementation.
🪄 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: 597f6b28-db32-4f56-9676-3549c94d8e2c
📒 Files selected for processing (5)
pacquet/crates/package-manager/src/create_virtual_store.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/tarball/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- pacquet/crates/package-manager/src/create_virtual_store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.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). (10)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Doc
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
- GitHub Check: Run benchmark on ubuntu-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: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/tarball/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
🧠 Learnings (28)
📓 Common learnings
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:435-439
Timestamp: 2026-06-04T14:40:29.444Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs` (pnpm/pnpm repo), the pnpr install accelerator always invokes `Install` with `lockfile_only: true` (hard-coded in `pnpr/crates/pnpr/src/install_accelerator/resolve.rs`). Under `lockfile_only: true`:
1. The `PrefetchingResolver` wrapper is skipped — the bare `inner_resolver` is used instead, so `PrefetchContext { config }` is never constructed.
2. The function returns before `CreateVirtualStore` is reached, so `install_package_by_snapshot` and its `config.auth_headers` fetch path are never hit.
pnpr's tarball fetch is handled separately in `resolve::fetch_uncached`, which independently receives the request-scoped `auth_headers`. Therefore, `auth_override` only needs to be threaded into the resolver-side components (NpmResolver, TarballResolver, NamedRegistryResolver) — not into PrefetchingResolver or CreateVirtualStore — on the pnpr path. For local installs (`lockfile_only: false`), `auth_override` is always `None` a...
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not invoke `BuildModules` and discards `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput`. This is pre-existing, documented behavior (mirroring upstream `link.ts:167-170`): `importing_done` fires once extraction and symlink linking are complete, and the fresh-lockfile path does not run lifecycle scripts. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Do not flag this omission as a bug; wiring lifecycle scripts into the fresh-lockfile path is tracked as future work separate from perf refactors.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:05.107Z
Learning: In pnpm/pnpm's pnpr install accelerator, the `/v1/install` response has a two-level framing structure:
1. **Outer layer** (full HTTP body): `[u32 outer header length][outer header JSON][files payload]` — `fetchFromPnpmRegistry` (pnpr/client/src/fetchFromPnpmRegistry.ts) strips the outer layer with `body.subarray(4 + headerLength)` and passes the remaining bytes to `writeCafsFiles`.
2. **Inner layer** (files payload): the files payload itself starts with its own `[u32 inner json length][inner header JSON]` prefix (built by the server's `build_files_payload` / `empty_files_payload_prefix`), followed by `[64-byte digest][u32 size][1-byte exec][content]` frames and a 64-zero-byte end marker.
`writeCafsFiles` in `worker/src/start.ts` is correct to read `jsonLen = payload.readUInt32BE(0)` and start frames at `offset = 4 + jsonLen` — this skips the inner header. The same two-level structure is mirrored in the Rust reference client (`parse_inline_response` + `write_files_payload`). Do not fla...
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not run lifecycle scripts (`BuildModules` is not invoked, and `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput` is discarded). This is pre-existing behavior documented by an in-code comment mirroring upstream `link.ts:167-170`. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Wiring lifecycle scripts into the fresh-lockfile path is tracked as future work, separate from this file's concern.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Reference the upstream pnpm commit/PR when porting code from pnpm in commit messages
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12134
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:311-325
Timestamp: 2026-06-02T13:18:30.659Z
Learning: In pacquet's lockfile resolution verifier (`pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`), URL-keyed tarball dependencies do NOT need a separate `non_semver_version` field in `VerifyCtx`. Unlike the TypeScript side (which derives `version` from `snapshot.version` and threads `nonSemverVersion` separately), pacquet's `collect_candidates` takes `version` from the lockfile key suffix. For a URL-keyed dep the key is `name@<url>`, so `ctx.version` is the URL string, which fails `node_semver::Version::parse(ctx.version)` and the existing guard `if node_semver::Version::parse(ctx.version).is_err() { return ResolutionVerification::Ok; }` already skips the registry lookup correctly. Adding a `non_semver_version` field to `VerifyCtx` for this purpose would be inert.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.797Z
Learning: Prefer existing pacquet-* crates over writing new code; check pacquet-tarball, pacquet-crypto-hash, pacquet-crypto-shasums-file, pacquet-package-manifest, pacquet-network, pacquet-registry, pacquet-fs, and pacquet-diagnostics before implementing non-trivial functionality
📚 Learning: 2026-06-04T14:40:29.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:435-439
Timestamp: 2026-06-04T14:40:29.444Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs` (pnpm/pnpm repo), the pnpr install accelerator always invokes `Install` with `lockfile_only: true` (hard-coded in `pnpr/crates/pnpr/src/install_accelerator/resolve.rs`). Under `lockfile_only: true`:
1. The `PrefetchingResolver` wrapper is skipped — the bare `inner_resolver` is used instead, so `PrefetchContext { config }` is never constructed.
2. The function returns before `CreateVirtualStore` is reached, so `install_package_by_snapshot` and its `config.auth_headers` fetch path are never hit.
pnpr's tarball fetch is handled separately in `resolve::fetch_uncached`, which independently receives the request-scoped `auth_headers`. Therefore, `auth_override` only needs to be threaded into the resolver-side components (NpmResolver, TarballResolver, NamedRegistryResolver) — not into PrefetchingResolver or CreateVirtualStore — on the pnpr path. For local installs (`lockfile_only: false`), `auth_override` is always `None` a...
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/*.rs : Do not restate at call sites what the callee's doc comment already says; update the doc once and let every call site benefit
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Use snapshot tests with `insta` and carefully review diffs when intentional changes alter snapshots; accept with `cargo insta review` only after careful review
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not invoke `BuildModules` and discards `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput`. This is pre-existing, documented behavior (mirroring upstream `link.ts:167-170`): `importing_done` fires once extraction and symlink linking are complete, and the fresh-lockfile path does not run lifecycle scripts. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Do not flag this omission as a bug; wiring lifecycle scripts into the fresh-lockfile path is tracked as future work separate from perf refactors.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-06-02T13:18:30.659Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12134
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:311-325
Timestamp: 2026-06-02T13:18:30.659Z
Learning: In pacquet's lockfile resolution verifier (`pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`), URL-keyed tarball dependencies do NOT need a separate `non_semver_version` field in `VerifyCtx`. Unlike the TypeScript side (which derives `version` from `snapshot.version` and threads `nonSemverVersion` separately), pacquet's `collect_candidates` takes `version` from the lockfile key suffix. For a URL-keyed dep the key is `name@<url>`, so `ctx.version` is the URL string, which fails `node_semver::Version::parse(ctx.version)` and the existing guard `if node_semver::Version::parse(ctx.version).is_err() { return ResolutionVerification::Ok; }` already skips the registry lookup correctly. Adding a `non_semver_version` field to `VerifyCtx` for this purpose would be inert.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not run lifecycle scripts (`BuildModules` is not invoked, and `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput` is discarded). This is pre-existing behavior documented by an in-code comment mirroring upstream `link.ts:167-170`. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Wiring lifecycle scripts into the fresh-lockfile path is tracked as future work, separate from this file's concern.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/*.rs : Doc comments (`///`, `//!`) document the contract — preconditions, postconditions, panics, and the reason the function exists; they are not a re-narration of the body
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-06-04T14:55:52.163Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/cli/src/cli_args/install.rs:0-0
Timestamp: 2026-06-04T14:55:52.163Z
Learning: In `pacquet/crates/cli/src/cli_args/install.rs` (pnpm/pnpm repo), the `install_via_pnpr` function intentionally forwards the **full** `state.config.auth_headers` map to the pnpr server (not filtered to only the declared default/named registries). This is required for correctness: transitive dependencies can be scope-routed to registries not in the explicit registry list, or pinned to tarball URLs on hosts present in `.npmrc` but not a declared registry. Filtering to the declared registries silently drops tokens those sub-dependencies need, causing 401s on the server. pnpr uses the forwarded map to attach the right token per fetched URL exactly as a local install does (`AuthHeaders::for_url`). The pnpr-server's own credential is sent separately in the `Authorization` header and is excluded from the body map. Do NOT flag this as a credential-leakage issue — the rationale is documented in a comment at both call sites.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-05-29T18:03:24.797Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.797Z
Learning: Prefer existing pacquet-* crates over writing new code; check pacquet-tarball, pacquet-crypto-hash, pacquet-crypto-shasums-file, pacquet-package-manifest, pacquet-network, pacquet-registry, pacquet-fs, and pacquet-diagnostics before implementing non-trivial functionality
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/tarball/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-23T17:30:06.849Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-05-20T01:42:44.958Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:370-380
Timestamp: 2026-05-20T01:42:44.958Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package.rs`, the fetch-error fallback that returns `Ok(PickPackageResult { meta: disk, picked_package: picked })` — even when `picked` is `None` — is intentional upstream parity with pnpm's `pickPackage.ts` catch block (https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431). When a fetch fails and a disk mirror exists, the stale-mirror pick (including null/None) is returned verbatim; the transport error is logged via `tracing::debug!`. Do not flag this as a bug.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-20T20:41:50.322Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/package-manager/src/install_package_from_registry.rs:111-114
Timestamp: 2026-05-20T20:41:50.322Z
Learning: In pacquet (pnpm/pnpm repo, Rust codebase), `install_package_from_registry` is the npm-only install path. The npm resolver always stamps `ResolveResult.id` (a `PkgResolutionId`) as `nameversion`. Parsing it back through `PkgNameVer` with `.expect()` is intentional — a parse failure means a mis-dispatch bug, not malformed external input. Per pacquet's CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." Do not suggest replacing such `.expect()` calls with graceful error handling.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-05-24T21:11:04.272Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-24T21:11:04.272Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-06-02T16:13:39.456Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12144
File: pnpr/crates/pnpr/src/install_accelerator/verdict_cache.rs:57-61
Timestamp: 2026-06-02T16:13:39.456Z
Learning: In `pnpr/crates/pnpr/src/install_accelerator/verdict_cache.rs`, the `lockfile_verdicts` SQLite table intentionally uses `hash` alone (not a composite `(hash, policy)` key) as the primary key — last-write-wins per hash. This mirrors the local `lockfile-verified.jsonl` cache design in pnpm. A looser current policy can trust a stricter cached pass via `can_trust_past_check`; alternating-policy re-verification is an accepted trade-off. A composite key was explicitly rejected to maintain parity with the local cache model.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 Learning: 2026-06-04T20:24:32.096Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12198
File: pnpr/crates/pnpr/src/storage.rs:469-477
Timestamp: 2026-06-04T20:24:32.096Z
Learning: In `pnpr/crates/pnpr/src/storage.rs` (pnpm/pnpm repo, Rust), `Store::list_package_names` intentionally uses `fs::try_exists(...).await.unwrap_or(false)` and `if let Ok(mut inner) = fs::read_dir(...)` — NOT `?`-propagation — for per-entry checks. This is deliberate best-effort / verdaccio-style search behavior: (1) `try_exists(stray_file/package.json)` returns `ENOTDIR` (not `NotFound`) for a stray non-package file in the store root, so `?` would fail the entire search; (2) the `@`-scope `read_dir` would fail on a non-directory `@`-named entry; (3) switching to `DirEntry::file_type()` would stop following symlinked package dirs. Failures that DO propagate are preserved: opening the store root itself, and `next_entry()` during the walk. Do not suggest blanket `?`-propagation for these per-entry checks.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
📚 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/package-manager/src/install_package_by_snapshot.rspacquet/crates/tarball/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.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/package-manager/src/install_package_by_snapshot.rspacquet/crates/tarball/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.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/package-manager/src/install_package_by_snapshot.rspacquet/crates/tarball/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/*.rs : Prefer `Arc::clone(&x)` / `Rc::clone(&x)` over `x.clone()` for reference-counted types so the cost is visible at the call site
Applied to files:
pacquet/crates/tarball/src/lib.rs
📚 Learning: 2026-05-23T16:55:36.507Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: pacquet/crates/cli/tests/lockfile_verification.rs:158-162
Timestamp: 2026-05-23T16:55:36.507Z
Learning: In `pacquet/crates/cli/tests/lockfile_verification.rs`, the `trust_lockfile_skips_verification` and `trust_lockfile_cli_flag_skips_verification` tests intentionally do NOT assert `output.status.success()`. The hand-rolled fixture lockfile uses a placeholder integrity hash (`sha512-AAA…`), so the install always fails the downstream tarball integrity check regardless of the supply-chain gate. The contract being tested is "gate-skipped, not install-succeeded"; asserting success would require generating a real lockfile via the `generate_lockfile` pattern (see `hoist.rs`) which is considered not worth the extra wiring for an opt-out smoke test.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Tests that need the mocked registry should start `pnpr` through `pacquet-testing-utils`; `cargo test` / `cargo nextest run` should not require a separate `just registry-mock launch` step
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/tests/**/*.rs : Prefer real fixtures over dependency-injection seams — use `tempfile::TempDir`, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-20T21:18:56.391Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/tests/resolve.rs:365-372
Timestamp: 2026-05-20T21:18:56.391Z
Learning: In `pacquet/crates/resolving-local-resolver/tests/resolve.rs`, the test `fail_when_resolving_from_not_existing_directory_an_injected_dependency` intentionally uses `injected: false`. The test is a verbatim port of the upstream pnpm TypeScript test (resolving/local-resolver/test/index.ts at ef87f3ccff). The `injected` flag only affects the file/link protocol choice for plain directory paths; when the `file:` scheme is explicit in the bare specifier, the flag has no effect on the resolution code path. The misleading test name is inherited from upstream.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-29T18:03:15.372Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.372Z
Learning: Applies to pacquet/**/*.rs : Tests are documentation — do not duplicate test scenarios, edge cases, failure modes, or worked examples in prose when they are already captured by tests
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
📚 Learning: 2026-05-20T23:07:40.413Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs:167-191
Timestamp: 2026-05-20T23:07:40.413Z
Learning: In `pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs`, the offline guard in `resolve_impl` (NodeResolver) is intentionally absent from `resolve_latest_impl`. This mirrors upstream pnpm's behavior: `resolveNodeRuntime` checks offline, but `resolveLatestNodeRuntime` does not carry `offline` in its context and never checks it. The asymmetry is required so `pnpm outdated` / `pnpm update --latest` can still query for newer runtime versions even in offline mode.
Applied to files:
pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs
🔇 Additional comments (2)
pacquet/crates/tarball/src/lib.rs (1)
1163-1169: LGTM!pacquet/crates/package-manager/src/install_package_by_snapshot/tests.rs (1)
299-555: LGTM!
… guard Borrow `metadata.resolution` in the registry-only mem-cache guard so the non-binding `matches!` reads through the shared reference explicitly, addressing a review comment on #12243. No behavior change.
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
1 participant
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.
What
Fixes the fresh-install double-download race in #12241.
On pacquet's phased
CreateVirtualStoreinstall path, the same tarball could be downloaded twice: once by the resolve-time prefetcher (PrefetchingResolver, which spawnsrun_with_mem_cacheinto the sharedtarball_mem_cache), and again byCreateVirtualStore's cold batch. The cold batch calledInstallPackageBySnapshot→run_without_mem_cache, which consults the store index and the prefetched-CAS map but never thetarball_mem_cache/Notify— so if the prefetch's store-index row hadn't committed yet, the package was classified cold and re-downloaded, potentially while the prefetch was still streaming the same bytes.It was never a correctness bug (content-addressed, idempotent writes), but it wasted network/CPU and (since #12236) could render the byte-progress gauge twice.
How
tarball_mem_cacheintoCreateVirtualStoreandInstallPackageBySnapshot.run_with_mem_cachewhen the mem cache is present (fresh path). It picks up an already-finished prefetch (CacheValue::Available) immediately, or briefly parks on the per-URLNotifyfor an in-flight one — exactly the coordination theprefetching_resolvermodule doc already describes.Noneand keeps the existingrun_without_mem_cachepath byte-for-byte.Honoring the "no perf regression" constraints from the issue
run_without_mem_cachealready pays on a prefetched-CAS hit), which is negligible next to a re-download.Tests
Two deterministic, network-free tests in
install_package_by_snapshot/tests.rs:cold_batch_reuses_in_flight_prefetch_from_mem_cache— seeds the mem cache with a finished download underoffline: true; the cold batch returns the seeded CAS map without touching the network. Verified to fail when the fix is reverted (ERR_PACQUET_NO_OFFLINE_TARBALL).frozen_path_does_not_consult_mem_cache— proves theNonepath still goes straight to the (offline-gated) download.just-equivalent checks run locally:cargo nextest run -p pacquet-package-manager -p pacquet-tarball(all green), clippy--all-targets -D warnings, fmt, typos, andcargo doc -D warningsall clean. The pre-push Rust hook (fmt/doc/dylint/taplo) passed on push.Notes
This is a pacquet-internal race; pnpm's TypeScript install architecture (piscina pool +
fetchingpromises) doesn't have the equivalent bug, so no pnpm-side port is needed. Per project convention, pacquet-only changes don't carry a changeset.The CI integrated-benchmark (loopback registry, Linux) shows no regression across every fresh scenario —
pacquet@HEAD≈pacquet@mainwithin noise:(The loopback registry runs at ~GB/s, so the avoided re-download is nearly free here and the win isn't visible — but the constraint that matters, no regression, holds. The micro-benchmark
tarball/download_dependencyis likewise flat: 7.7 ms → 7.9 ms.)Follow-up fixes after rebasing on #12245
#12245 landed the shared
tarball_mem_cacheinfrastructure (frozen/pnpr path →Some, fresh path →None). This PR now just flips the fresh path toSome, plus two safety refinements found via CI:PrefetchingResolverskips remote tarballs (they resolve with noname_ver); their only mem-cache entry comes from the resolver's download-to-resolve, keyedname@version, while the lockfile/materialization address them byname@<url>. Reusing that entry would skip writing thename@<url>store-index row a later re-resolve needs — which regressedremote_tarball_reresolves_from_warm_store_without_refetch.SiblingFetchFailedthe cold batch now does its own retried download instead of inheriting the failure, so a transient prefetch error can't fail the install.Closes #12241
Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Tests