Skip to content

fix(pacquet): report fresh-install prefetch downloads as fetched, not found_in_store#12236

Merged
zkochan merged 4 commits into
mainfrom
fix/12235
Jun 6, 2026
Merged

fix(pacquet): report fresh-install prefetch downloads as fetched, not found_in_store#12236
zkochan merged 4 commits into
mainfrom
fix/12235

Conversation

@zkochan

@zkochan zkochan commented Jun 6, 2026

Copy link
Copy Markdown
Member

Problem

A cold pacquet install (fresh-resolve) and pacquet install --frozen-lockfile download the same tarballs but report them oppositely:

  • Fresh resolves through PrefetchingResolver, which downloads tarballs during resolution on a SilentReporter (emits discarded). By the time create_virtual_store's warm batch reports each package, prefetch_cas_paths already finds the freshly-written store-index row, so it emits found_in_store for everything — "reused N, downloaded 0".
  • Frozen (and the pnpr path, which forces frozen) has no prefetcher, so its cold batch downloads through the visible path and emits fetched"downloaded N".

Same bytes, same network, opposite report. This made a --frozen-lockfile / pnpr install look like it was redundantly re-downloading a graph that a plain install "reused". Upstream pnpm has no such split: packageRequester emits once and the emit reflects whether the fetch hit the network.

Fixes #12235.

Fix

Add an install-scoped SharedNetworkFetchedKeys (Arc<DashSet<String>> in pacquet-tarball), keyed by store_index_key(integrity, pkg_id) — the cache keys whose bytes came over the network this install:

  • DownloadTarballToStore::run_without_mem_cache inserts a key the moment its network fetch succeeds (before the store-index row is queued, so any later reader that sees the row "in store" also sees this membership).
  • The fresh path threads the set into PrefetchContext/OwnedFetchCtx (the silent prefetcher writes) and into CreateVirtualStore (the warm batch reads).
  • emit_warm_snapshot_progress now takes network_fetched: bool and emits fetched for a package downloaded this install, found_in_store only for one genuinely already in the store at install start.
  • The frozen path passes an empty set (its cold batch already emits fetched directly). Cold-batch / binary download sites pass network_fetched: None.

Acceptance

A cold pacquet install now reports its prefetch downloads as downloads, so pacquet install and pacquet install --frozen-lockfile produce identical reused/downloaded counts for the same cold store.

Tests

  • New unit coverage in create_virtual_store::tests: emits_resolved_then_fetched_when_network_fetched and the renamed ..._when_not_network_fetched.
  • New tarball tests: network_fetch_records_cache_key (a real download records its key) plus an assertion in reuses_cached_cas_paths_when_index_entry_is_live that a store cache hit does not record.
  • Verified locally: cargo check --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all, RUSTDOCFLAGS=-D warnings cargo doc, and the tarball + create_virtual_store test suites — all green.

Pacquet-only parity fix (pnpm already behaves correctly); no pnpm-side change and no changeset needed.


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

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Eliminated duplicate progress events during package installation to prevent packages from being incorrectly counted multiple times when reused across prefetch and installation phases.
  • Tests

    • Added test coverage for progress event deduplication behavior in tarball downloads and virtual store operations.

… found_in_store

A cold `pacquet install` and `pacquet install --frozen-lockfile` download
the same tarballs but reported them oppositely. The fresh path resolves
through `PrefetchingResolver`, which downloads tarballs during resolution
on a `SilentReporter` (emits discarded); by the time the warm batch in
`create_virtual_store` reports each package, `prefetch_cas_paths` already
finds the freshly-written store-index row, so it emitted `found_in_store`
for everything — "reused N, downloaded 0". The frozen path has no
prefetcher, so its cold batch downloads visibly and emits `fetched`.

Add an install-scoped `SharedNetworkFetchedKeys` set keyed by
`store_index_key(integrity, pkg_id)`. `DownloadTarballToStore::
run_without_mem_cache` records a key when its network fetch succeeds; the
fresh path threads the set into the prefetcher (writer) and into
`CreateVirtualStore` (reader). `emit_warm_snapshot_progress` now emits
`fetched` for a package downloaded this install and `found_in_store` only
for one genuinely already in the store — matching the cold-batch /
frozen-lockfile path and pnpm's single per-package emit.

Closes #12235
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements progress event deduplication across resolve-time prefetch and install-time package materialization. A shared key set tracks which packages have already reported progress during prefetch, suppressing duplicate found_in_store events during virtual store creation so that both fresh and frozen installs report download counts consistently.

Changes

Progress deduplication and consistent reporting

Layer / File(s) Summary
Tarball progress deduplication types and helpers
pacquet/crates/tarball/src/lib.rs
Introduces ReportedProgressKeys / SharedReportedProgressKeys types and refactors emit_progress_found_in_store, emit_progress_fetched to accept optional deduplication keys. New helper progress_already_reported suppresses duplicate events. Zip and resolve-time paths disable deduplication by passing progress_key: None.
Tarball cache-hit and download paths
pacquet/crates/tarball/src/lib.rs
Computes progress_key from store_index_key(integrity, pkg_id) in run_with_mem_cache and run_without_mem_cache. Threads it through warm prefetch reuse, SQLite cached reuse, and mem-cache waiting branches. Passes it into fetch_and_extract_with_retry during network downloads; updates store-index writer to use pre-computed cache_key.
Prefetch resolver progress tracking
pacquet/crates/package-manager/src/prefetching_resolver.rs
PrefetchContext gains progress_reported field; cloned into background tasks via OwnedFetchCtx. Prefetch tarball downloads switch from SilentReporter to Reporter, enabling active progress recording. Progress keys are automatically recorded in the shared set as downloads complete.
Virtual store conditional FoundInStore emission
pacquet/crates/package-manager/src/create_virtual_store.rs
CreateVirtualStore gains progress_reported field. Warm-batch tuples now include cache keys; during parallel processing, emitter receives a boolean from progress_reported.contains(cache_key) to gate FoundInStore emission. Helper emit_warm_snapshot_progress accepts this flag. Hoisted mode and cold-batch paths apply the same conditional logic.
Fresh and frozen install wiring
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs, pacquet/crates/package-manager/src/install_frozen_lockfile.rs
Both paths initialize progress_reported and thread it through their respective flows. Fresh wires to both PrefetchingResolver (records prefetch downloads) and CreateVirtualStore (deduplicates warm-batch). Frozen wires only to CreateVirtualStore.
Cold-batch snapshot and registry install
pacquet/crates/package-manager/src/install_package_by_snapshot.rs, pacquet/crates/package-manager/src/install_package_from_registry.rs
InstallPackageBySnapshot gains optional progress_reported field and threads it through tarball downloads. Binary archives explicitly set progress_reported: None. Registry recursive installs set progress_reported: None to isolate progress tracking.
Deduplication test coverage
pacquet/crates/package-manager/src/create_virtual_store/tests.rs, pacquet/crates/tarball/src/tests.rs
Virtual store tests validate conditional FoundInStore emission (when_not_progress_reported and when_already_reported cases). Tarball tests add network_fetch_records_progress_key (network downloads record key), reuses_cached_cas_paths_when_index_entry_is_live (cache hits record key), and mem_cache_hit_skips_package_status_when_progress_already_reported (second call skips emission). Existing tests updated with progress_reported: None.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • pnpm/pnpm#12235: This PR directly addresses the fresh vs. frozen install reporting inconsistency by deduplicating progress events from prefetch-time downloads, ensuring both paths report identical download counts.

Possibly related PRs

  • pnpm/pnpm#11856: Both modify prefetch/install progress-emission paths to control when progress events are emitted, specifically avoiding duplicate found_in_store reports.

Poem

🐰 A rabbit traces progress through two paths,
One prefetches in silence, one cold and loud,
But now a shared key-set remembers the fetches,
So found_in_store doesn't crow twice at the crowd. 📦✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: addressing inconsistent reporting between fresh-install prefetch downloads (reported as found_in_store) and frozen-install downloads (reported as fetched).
Linked Issues check ✅ Passed The PR implements all coding requirements from #12235: introduces SharedReportedProgressKeys to track network-fetched packages, threads it through PrefetchContext and CreateVirtualStore, modifies emit_warm_snapshot_progress to conditionally emit fetched vs found_in_store based on whether bytes were network-fetched during the install, and achieves parity between fresh and frozen install reporting.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the inconsistent download reporting between fresh and frozen installs. No unrelated refactoring, dependency updates, or out-of-scope modifications are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/12235

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      7.4±0.09ms   586.5 KB/sec    1.02      7.5±0.17ms   577.6 KB/sec

@codecov-commenter

codecov-commenter commented Jun 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.11881% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.37%. Comparing base (089484a) to head (2b55ba4).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pacquet/crates/tarball/src/lib.rs 82.69% 9 Missing ⚠️
...package-manager/src/install_package_by_snapshot.rs 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12236      +/-   ##
==========================================
+ Coverage   87.34%   87.37%   +0.02%     
==========================================
  Files         276      276              
  Lines       32196    32254      +58     
==========================================
+ Hits        28123    28181      +58     
  Misses       4073     4073              

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

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

zkochan added 2 commits June 6, 2026 10:47
The comment described the old recursive install_subtree → run_with_mem_cache
reporting path. On the fresh phased path the install pass reports through
CreateVirtualStore's warm batch, which is exactly why the network_fetched
set is needed — spell that out instead.
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.810 ± 0.074 4.734 4.990 2.43 ± 0.09
pacquet@main 4.767 ± 0.038 4.723 4.842 2.40 ± 0.08
pnpr@HEAD 2.008 ± 0.065 1.939 2.170 1.01 ± 0.05
pnpr@main 1.983 ± 0.064 1.899 2.104 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.80991351752,
      "stddev": 0.07397917905493685,
      "median": 4.784714836620001,
      "user": 2.4571712199999998,
      "system": 3.6447426599999995,
      "min": 4.733867725620001,
      "max": 4.98978688862,
      "times": [
        4.8167841106200004,
        4.75804562062,
        4.733867725620001,
        4.79616951762,
        4.879340502620001,
        4.98978688862,
        4.7867546366200004,
        4.782675036620001,
        4.77345682462,
        4.78225431162
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.76658471412,
      "stddev": 0.03772552786075673,
      "median": 4.756179227620001,
      "user": 2.44509512,
      "system": 3.6548618599999996,
      "min": 4.72326181462,
      "max": 4.84153056062,
      "times": [
        4.84153056062,
        4.75833795962,
        4.736035757620001,
        4.75294019662,
        4.770351396620001,
        4.78271606962,
        4.75402049562,
        4.72326181462,
        4.815779919620001,
        4.73087297062
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.0079434754200003,
      "stddev": 0.06470042433663749,
      "median": 1.9883633071200002,
      "user": 2.69160822,
      "system": 3.17928606,
      "min": 1.93936145262,
      "max": 2.17025416262,
      "times": [
        2.17025416262,
        1.97581919962,
        1.96761217862,
        2.0009074146200003,
        1.93936145262,
        2.04625731662,
        1.97333738562,
        2.0207370636200004,
        1.97500541262,
        2.0101431676200003
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 1.9830956152200003,
      "stddev": 0.06362943436921525,
      "median": 1.9685632671200002,
      "user": 2.6592005199999997,
      "system": 3.17203466,
      "min": 1.89896572062,
      "max": 2.10436831862,
      "times": [
        2.06679085862,
        1.93164535062,
        1.9647954066200002,
        1.93762288462,
        1.89896572062,
        1.9723311276200002,
        1.9951917966200001,
        1.94561920362,
        2.0136254846200003,
        2.10436831862
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 665.8 ± 20.0 646.8 712.8 1.00
pacquet@main 692.5 ± 74.8 654.4 899.4 1.04 ± 0.12
pnpr@HEAD 681.6 ± 51.4 650.1 815.8 1.02 ± 0.08
pnpr@main 738.6 ± 135.6 658.6 1112.9 1.11 ± 0.21
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6658326225,
      "stddev": 0.02001727526597529,
      "median": 0.6623396869,
      "user": 0.36651832000000006,
      "system": 1.3137290799999997,
      "min": 0.6467969824,
      "max": 0.7127726164,
      "times": [
        0.7127726164,
        0.6687373084,
        0.6859233904,
        0.6639060954,
        0.6499531214000001,
        0.6467969824,
        0.6525599574000001,
        0.6528910984,
        0.6640123764,
        0.6607732784
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6925270414,
      "stddev": 0.07481608138167674,
      "median": 0.6638137048999999,
      "user": 0.36360921999999996,
      "system": 1.31285618,
      "min": 0.6543716284,
      "max": 0.8994096644,
      "times": [
        0.6836721224,
        0.6631856124,
        0.6644417974,
        0.6564890804,
        0.6733178294,
        0.7129756914000001,
        0.6617619744,
        0.6543716284,
        0.6556450134,
        0.8994096644
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6815633362999999,
      "stddev": 0.0513689194127591,
      "median": 0.6598605429,
      "user": 0.37409192,
      "system": 1.30565388,
      "min": 0.6501247754,
      "max": 0.8157969664,
      "times": [
        0.7175366884,
        0.6840335444,
        0.6590555294,
        0.6658232344,
        0.6538464234,
        0.6557869774,
        0.6529636674,
        0.6606655564,
        0.6501247754,
        0.8157969664
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.7385873345,
      "stddev": 0.13555930145165176,
      "median": 0.6858418799,
      "user": 0.36138982,
      "system": 1.3232090799999998,
      "min": 0.6585818154,
      "max": 1.1129383944,
      "times": [
        0.7404031994,
        0.6718926614,
        0.7450923064,
        0.6655218164,
        0.7394083914,
        0.6804267774,
        0.6912569824,
        0.6585818154,
        0.6803510004,
        1.1129383944
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.230 ± 0.016 2.206 2.250 1.01 ± 0.01
pacquet@main 2.218 ± 0.026 2.191 2.257 1.00
pnpr@HEAD 2.220 ± 0.028 2.154 2.244 1.00 ± 0.02
pnpr@main 2.230 ± 0.022 2.198 2.274 1.01 ± 0.02
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.22979776064,
      "stddev": 0.015818633105614256,
      "median": 2.22948895454,
      "user": 3.62299778,
      "system": 2.97862894,
      "min": 2.20614644154,
      "max": 2.25010830754,
      "times": [
        2.2205854765399997,
        2.22638889054,
        2.23625438154,
        2.20989780254,
        2.23258901854,
        2.24960437954,
        2.24590406754,
        2.20614644154,
        2.22049884054,
        2.25010830754
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.2181524024400003,
      "stddev": 0.026261455156588648,
      "median": 2.20524602254,
      "user": 3.5878458800000006,
      "system": 2.99888714,
      "min": 2.19099050654,
      "max": 2.25712640354,
      "times": [
        2.24327025754,
        2.24157240954,
        2.20628894554,
        2.19255841854,
        2.20420309954,
        2.19099050654,
        2.25712640354,
        2.2489912845399997,
        2.19389613354,
        2.2026265655399997
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.2195190226399997,
      "stddev": 0.028156020730993738,
      "median": 2.23074456254,
      "user": 3.61225868,
      "system": 2.9707157399999993,
      "min": 2.15400973554,
      "max": 2.24410372654,
      "times": [
        2.23017570854,
        2.24133886954,
        2.15400973554,
        2.24124323754,
        2.21183942154,
        2.20136262754,
        2.23131341654,
        2.24410372654,
        2.20188320454,
        2.23792027854
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.2295912382400003,
      "stddev": 0.022487212232852546,
      "median": 2.2372474685399997,
      "user": 3.5865122799999996,
      "system": 2.9874604400000004,
      "min": 2.19834028654,
      "max": 2.27350938754,
      "times": [
        2.23832774454,
        2.21016463854,
        2.23616719254,
        2.27350938754,
        2.23895434354,
        2.2386616465399998,
        2.21195771454,
        2.19834028654,
        2.20725029854,
        2.2425791295399997
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.404 ± 0.011 1.391 1.419 1.00 ± 0.02
pacquet@main 1.418 ± 0.030 1.385 1.490 1.01 ± 0.03
pnpr@HEAD 1.401 ± 0.019 1.372 1.426 1.00 ± 0.02
pnpr@main 1.399 ± 0.020 1.365 1.438 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.40439206268,
      "stddev": 0.010744477607554622,
      "median": 1.40263999408,
      "user": 1.5086585400000003,
      "system": 1.7783575799999998,
      "min": 1.3908443050800001,
      "max": 1.4188202170800002,
      "times": [
        1.41454522608,
        1.4010848100800002,
        1.39237580108,
        1.41428101908,
        1.41605657508,
        1.39369777308,
        1.40419517808,
        1.3908443050800001,
        1.4188202170800002,
        1.3980197220800001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.4182444793800002,
      "stddev": 0.030229751366231183,
      "median": 1.41300373858,
      "user": 1.53109164,
      "system": 1.7591814799999999,
      "min": 1.38458073908,
      "max": 1.4897644700800001,
      "times": [
        1.4259422050800001,
        1.3969991130800001,
        1.4897644700800001,
        1.42488728608,
        1.38458073908,
        1.3980657570800001,
        1.39865048108,
        1.4011201910800002,
        1.43171884408,
        1.43071570708
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 1.40070095528,
      "stddev": 0.018909069971175727,
      "median": 1.40376533358,
      "user": 1.49170494,
      "system": 1.7595313799999999,
      "min": 1.37195302208,
      "max": 1.42608430808,
      "times": [
        1.3769137820800001,
        1.40206994308,
        1.38554021608,
        1.42608430808,
        1.40820579008,
        1.39177445108,
        1.42598141408,
        1.37195302208,
        1.4054607240800001,
        1.41302590208
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 1.3994748081800001,
      "stddev": 0.020305518339975685,
      "median": 1.3994197210800001,
      "user": 1.50324754,
      "system": 1.7717994799999999,
      "min": 1.36496627508,
      "max": 1.43822076508,
      "times": [
        1.40021614708,
        1.41017924708,
        1.4163893730800001,
        1.43822076508,
        1.39862329508,
        1.39461953608,
        1.37580048708,
        1.39415461408,
        1.40157834208,
        1.36496627508
      ]
    }
  ]
}

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12236
Testbedpacquet
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
milliseconds (ms)
(Result Δ%)
Upper Boundary
milliseconds (ms)
(Limit %)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
🚷 view threshold
2,229.80 ms
(+1.11%)Baseline: 2,205.30 ms
2,646.36 ms
(84.26%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,404.39 ms
(+4.71%)Baseline: 1,341.18 ms
1,609.42 ms
(87.26%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
4,809.91 ms
(+19.35%)Baseline: 4,030.26 ms
4,836.31 ms
(99.45%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
665.83 ms
(-1.83%)Baseline: 678.23 ms
813.87 ms
(81.81%)
🐰 View full continuous benchmarking report in Bencher

CI's clippy (rust 1.95) rejected two spots the local toolchain didn't:

- run_with_mem_cache's warm-cache fast path: collapse the now-adjacent
  nested `if let`s into a let-chain (cache_key was hoisted above them).
- the recording test: clone the events out instead of binding the
  MutexGuard, whose lexical scope spanned a later .await even though it
  was explicitly dropped first (the lint is scope-based).
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12236
Testbedpnpr

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,219.52 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
1,400.70 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,007.94 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
681.56 ms
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan marked this pull request as ready for review June 6, 2026 12:43
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Fix fresh-install prefetch downloads reported as found_in_store instead of fetched

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add install-scoped SharedReportedProgressKeys set to deduplicate package-status progress events
• Record cache keys when network fetches succeed, preventing duplicate reporting in warm batch
• Thread progress set through fresh-install prefetcher and virtual-store creation
• Emit fetched for prefetch downloads instead of found_in_store, matching frozen-lockfile
  behavior
Diagram
flowchart LR
  A["Network Fetch"] -->|records key| B["SharedReportedProgressKeys"]
  C["PrefetchingResolver"] -->|threads set| D["CreateVirtualStore"]
  D -->|checks set| E["emit_warm_snapshot_progress"]
  E -->|skips duplicate| F["Package Status Event"]
  G["Frozen Install"] -->|empty set| D

Loading

Grey Divider

File Changes

1. pacquet/crates/package-manager/src/create_virtual_store.rs 🐞 Bug fix +48/-23

Thread progress_reported set and skip duplicate status events

pacquet/crates/package-manager/src/create_virtual_store.rs


2. pacquet/crates/package-manager/src/create_virtual_store/tests.rs 🧪 Tests +38/-6

Add test for skipping duplicate progress when already reported

pacquet/crates/package-manager/src/create_virtual_store/tests.rs


3. pacquet/crates/package-manager/src/install_frozen_lockfile.rs 🐞 Bug fix +7/-0

Pass empty progress set for frozen-lockfile cold batch

pacquet/crates/package-manager/src/install_frozen_lockfile.rs


View more (7)
4. pacquet/crates/package-manager/src/install_package_by_snapshot.rs 🐞 Bug fix +11/-1

Thread progress_reported through cold-batch download paths

pacquet/crates/package-manager/src/install_package_by_snapshot.rs


5. pacquet/crates/package-manager/src/install_package_from_registry.rs 🐞 Bug fix +4/-0

Pass None for recursive install path progress tracking

pacquet/crates/package-manager/src/install_package_from_registry.rs


6. pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs 🐞 Bug fix +9/-1

Create and thread progress_reported set through fresh install

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs


7. pacquet/crates/package-manager/src/prefetching_resolver.rs 🐞 Bug fix +21/-21

Record progress keys in prefetcher and use real reporter

pacquet/crates/package-manager/src/prefetching_resolver.rs


8. pacquet/crates/tarball/src/lib.rs ✨ Enhancement +92/-48

Add SharedReportedProgressKeys type and progress deduplication logic

pacquet/crates/tarball/src/lib.rs


9. pacquet/crates/tarball/src/tests.rs 🧪 Tests +207/-20

Add tests for progress key recording and deduplication

pacquet/crates/tarball/src/tests.rs


10. pacquet/tasks/micro-benchmark/src/main.rs Miscellaneous +1/-0

Update benchmark to pass progress_reported parameter

pacquet/tasks/micro-benchmark/src/main.rs


Grey Divider

Qodo Logo

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pacquet/crates/tarball/src/tests.rs (1)

217-257: ⚡ Quick win

Make this regression test hermetic (avoid live npm network).

network_fetch_records_progress_key currently depends on https://registry.npmjs.org/..., which can make CI flaky/offline-unfriendly. You can assert the same key-recording behavior using mockito + FASTIFY_ERROR_TARBALL, consistent with neighboring tests.

Suggested change
 #[tokio::test]
 #[cfg(not(target_os = "windows"))]
 async fn network_fetch_records_progress_key() {
     let (store_dir, store_path) = tempdir_with_leaked_path();
+    let mut server = mockito::Server::new_async().await;
+    let mock = server
+        .mock("GET", "/pkg.tgz")
+        .with_status(200)
+        .with_body(FASTIFY_ERROR_TARBALL)
+        .expect(1)
+        .create_async()
+        .await;
+    let url = format!("{}/pkg.tgz", server.url());
     let pkg_integrity = integrity(
         "sha512-dj7vjIn1Ar8sVXj2yAXiMNCJDmS9MQ9XMlIecX2dIzzhjSHCyKo4DdXjXMs7wKW2kj6yvVRSpuQjOZ3YLrh56w==",
     );
     let pkg_id = "`@fastify/error`@3.3.0";
     let progress_reported = SharedReportedProgressKeys::default();

     DownloadTarballToStore {
         http_client: &Default::default(),
         store_dir: store_path,
@@
-        package_url: "https://registry.npmjs.org/@fastify/error/-/error-3.3.0.tgz",
+        package_url: &url,
         package_id: pkg_id,
@@
     .await
     .unwrap();
+    mock.assert_async().await;

Based on learnings: “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 external setup.”

🤖 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/tarball/src/tests.rs` around lines 217 - 257, The test
network_fetch_records_progress_key relies on the real npm registry URL; make it
hermetic by mocking the registry and tarball: start the test pnpr helper from
pacquet-testing-utils (so tests don't require external setup), use mockito to
serve FASTIFY_ERROR_TARBALL at a local URL, update the DownloadTarballToStore
package_url to point to that mockito URL and keep package_integrity/package_id
same, and assert progress via SharedReportedProgressKeys as before; ensure the
test uses the pacquet test helper setup (pnpr) used by neighboring tests so
cargo test/nextest needs no external network.

Source: Learnings

🤖 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.

Nitpick comments:
In `@pacquet/crates/tarball/src/tests.rs`:
- Around line 217-257: The test network_fetch_records_progress_key relies on the
real npm registry URL; make it hermetic by mocking the registry and tarball:
start the test pnpr helper from pacquet-testing-utils (so tests don't require
external setup), use mockito to serve FASTIFY_ERROR_TARBALL at a local URL,
update the DownloadTarballToStore package_url to point to that mockito URL and
keep package_integrity/package_id same, and assert progress via
SharedReportedProgressKeys as before; ensure the test uses the pacquet test
helper setup (pnpr) used by neighboring tests so cargo test/nextest needs no
external network.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 61e260cb-b387-4b4d-9b6a-72a58619eb96

📥 Commits

Reviewing files that changed from the base of the PR and between 089484a and 2b55ba4.

📒 Files selected for processing (10)
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
📜 Review details
🧰 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 fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
🧠 Learnings (45)
📓 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: 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: 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: 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: 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: 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: 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
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.
📚 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_from_registry.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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_from_registry.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-06-01T08:59:48.719Z
Learnt from: KSXGitHub
Repo: pnpm/pnpm PR: 12093
File: pacquet/crates/cli/src/cli_args/run/recursive.rs:290-315
Timestamp: 2026-06-01T08:59:48.719Z
Learning: In pacquet's recursive run implementation (`pacquet/crates/cli/src/cli_args/run/recursive.rs`), the `pnpm-exec-summary.json` format for failed package entries correctly includes `prefix` and `message` fields in addition to `status` and `duration`. This matches pnpm's `ActionFailure` variant in `cli/utils/src/recursiveSummary.ts` and the direct serialization in `exec/commands/src/exec.ts`. There is no `ExecutionStatusInSummary` type in pnpm. The only intentional divergence is omitting the JS `error` field, whose `JSON.stringify` output is non-deterministic due to non-enumerable `Error` properties.

Applied to files:

  • pacquet/crates/package-manager/src/install_package_from_registry.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_from_registry.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/create_virtual_store.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_from_registry.rs
  • pacquet/crates/tarball/src/lib.rs
📚 Learning: 2026-05-18T20:35:22.917Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11729
File: pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs:55-57
Timestamp: 2026-05-18T20:35:22.917Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/fetch_attestation_published_at.rs`, the npm attestation endpoint (`/-/npm/v1/attestations/{pkg_name}@{version}`) intentionally does NOT percent-encode the package name — the endpoint accepts literal `/` in scoped package names (e.g. `scope/pkg`). This matches upstream pnpm's `fetchAttestationPublishedAt.ts` behavior. Do not flag missing URL encoding here. By contrast, the full-metadata fetch paths (`fetch_full_metadata`, `fetch_full_metadata_cached`) DO percent-encode via the `registry_url::to_registry_url` helper, matching upstream's `toUri` behavior.

Applied to files:

  • pacquet/crates/package-manager/src/install_package_from_registry.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: Applies to pnpr/**/pnpr/crates/**/Cargo.toml : New registry-only crates must be placed under pnpr/crates/<short-name>/ and named pnpr-<short-name> in Cargo.toml, never using the pacquet- prefix

Applied to files:

  • pacquet/crates/package-manager/src/install_package_from_registry.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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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_from_registry.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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 : 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/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/tarball/src/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/create_virtual_store/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/create_virtual_store/tests.rs
  • pacquet/crates/tarball/src/tests.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/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/tarball/src/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/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/tarball/src/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/create_virtual_store/tests.rs
📚 Learning: 2026-05-24T16:07:54.784Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11904
File: pacquet/crates/package-manager/src/install.rs:556-560
Timestamp: 2026-05-24T16:07:54.784Z
Learning: In pacquet's `is_modules_yaml_consistent` (pacquet/crates/package-manager/src/install.rs), `enableGlobalVirtualStore` is intentionally NOT checked as a separate field. Upstream pnpm's `validateModules.ts` does not persist or check `enableGlobalVirtualStore` in `.modules.yaml` either. Drift on this setting is caught indirectly: toggling `enableGlobalVirtualStore` changes `config.effective_virtual_store_dir()` (GVS-on → `<store>/v11/links`, GVS-off → `<project>/node_modules/.pnpm`), so the existing `modules.virtual_store_dir == config.effective_virtual_store_dir()` comparison in `is_modules_yaml_consistent` already detects the mismatch and prevents the short-circuit. Do not flag the absence of an explicit `enableGlobalVirtualStore` field as a bug.

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/create_virtual_store.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 : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/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: Consult `plans/TEST_PORTING.md` before adding ported tests and update its checkboxes as items land; use `known_failures` modules and `pacquet_testing_utils::allow_known_failure!` at the not-yet-implemented boundary

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/tarball/src/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 : Log emissions are part of matching pnpm — when porting a function that fires `pnpm:<channel>` events through `globalLogger`, `logger.debug(...)`, or `streamParser.write(...)`, mirror the call site, payload, and ordering so `pnpm/cli.default-reporter` parses pacquet's NDJSON the same way

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/tarball/src/lib.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/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.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: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
📚 Learning: 2026-05-20T23:08:06.093Z
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: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store/tests.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/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-20T10:06:55.749Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs:15-18
Timestamp: 2026-05-20T10:06:55.749Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`, the `DirectDep.id`, `ResolvedPackage.id`, and `ResolvedTree.packages` HashMap keys are intentionally plain `String` for now. The natural branded type would be `pacquet_lockfile::PkgNameVer`, but it cannot be used as a HashMap key because `node_semver::Version` does not derive `Hash`. The upstream parity type is `PkgResolutionId` (which carries an optional peer-dep suffix), and the branded type should be introduced alongside peer-dep resolution and lockfile generation work to avoid locking the seam too early.

Applied to files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.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_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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 : Search for existing shared helpers before writing new code; shared helpers tend to live in `crates/fs`, `crates/testing-utils`, and `crates/diagnostics`

Applied to files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/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 : Follow test-logging guidance — log before non-`assert_eq!` assertions, `dbg!` complex structures, skip logging for simple scalar `assert_eq!`

Applied to files:

  • pacquet/crates/package-manager/src/install_frozen_lockfile.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
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.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: Applies to pnpr/**/pnpr/**/*.rs : Follow the pacquet code-style guide (../pacquet/CODE_STYLE_GUIDE.md) for Rust-level conventions including imports, naming, ownership, and error handling

Applied to files:

  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/tarball/src/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/**/Cargo.toml : Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.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_with_fresh_lockfile.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-19T19:23:00.981Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11752
File: pacquet/crates/config/src/lib.rs:1062-1073
Timestamp: 2026-05-19T19:23:00.981Z
Learning: In `pacquet/crates/config/src/lib.rs`, `modules_dir` does not need a `!virtual_store_dir_explicit` guard on its workspace re-anchor because `modules_dir` is in pnpm's `excludedPnpmKeys` (filtered out by `WorkspaceSettings::clear_workspace_only_fields`) and therefore can only be set by workspace yaml (applied immediately after) or env vars (applied later in the cascade) — not by global `config.yaml`. `virtual_store_dir`, by contrast, IS settable from global config and requires the `if !virtual_store_dir_explicit` guard to survive the workspace-root re-anchor.

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store.rs
📚 Learning: 2026-05-25T12:36:42.202Z
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

Applied to files:

  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/tarball/src/lib.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 : User-facing errors go through `miette` via the `pacquet-diagnostics` crate; match pnpm's error codes and messages where pnpm defines them since error codes are part of the public contract

Applied to files:

  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.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/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.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/tarball/src/lib.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 use star imports inside module bodies — write `use super::{Foo, bar}` instead of `use super::*;` for modules you control

Applied to files:

  • pacquet/crates/tarball/src/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 : Follow Rust API Guidelines for naming

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-20T20:41:30.632Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs:115-117
Timestamp: 2026-05-20T20:41:30.632Z
Learning: In the pacquet Rust port of pnpm, the `is_http_url` helper in `pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs` intentionally uses `bare.starts_with("http:") || bare.starts_with("https:")` (not `"http://"` / `"https://"`) to match upstream pnpm's `startsWith('http:')` / `startsWith('https:')` check byte-for-byte. Pacquet's cardinal rule (pacquet/AGENTS.md) requires matching pnpm even on quirks; malformed non-URL inputs are rejected downstream by `reqwest::Url::parse` as a `ResolveError`.

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-06-04T06:04:05.107Z
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...

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
🔇 Additional comments (13)
pacquet/crates/tarball/src/lib.rs (5)

9-9: LGTM!

Also applies to: 276-291


1270-1277: LGTM!


1651-1687: LGTM!

Also applies to: 1710-1710, 1732-1732


1817-1819: LGTM!

Also applies to: 1838-1851, 1884-1890, 1905-1907, 2000-2001, 2030-2030, 2036-2036, 2043-2043, 2090-2090, 2103-2103


2180-2180: LGTM!

Also applies to: 2570-2570, 2583-2583

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

33-33: LGTM!

Also applies to: 41-42, 59-64, 83-84, 128-129, 142-143, 213-213, 216-223, 244-247

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

23-24: LGTM!

Also applies to: 160-166, 210-211, 620-627, 638-639, 676-682, 716-723, 770-771, 1059-1064, 1107-1128

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

40-40: LGTM!

Also applies to: 539-544, 700-701, 1276-1277

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

29-29: LGTM!

Also applies to: 579-583, 604-604

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

24-24: LGTM!

Also applies to: 52-57, 226-227, 283-283, 706-709

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

183-187: LGTM!

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

42-112: LGTM!

pacquet/tasks/micro-benchmark/src/main.rs (1)

56-56: LGTM!

@zkochan zkochan merged commit da4c80c into main Jun 6, 2026
28 checks passed
@zkochan zkochan deleted the fix/12235 branch June 6, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(pacquet): fresh vs frozen install report identical downloads inconsistently (found_in_store vs fetched)

2 participants