feat(pnpr): forward credentials and add per-user access grants for external private registries (#12184)#12189
Merged
Conversation
…private registries (#12184) The install accelerator now forwards the caller's per-registry credentials on `POST /v1/install`, so it can resolve, verify, and fetch private dependencies from external registries as the caller instead of reaching the registry anonymously. - Wire: new `authHeaders` body map (nerf-darted URI -> header) plus an `Authorization` request header identifying the caller to pnpr's gate. - pacquet: thread a request-scoped `AuthHeaders` via `Install.auth_override` and a new `build_resolution_verifiers` param, instead of baking per-user auth into the interned `&'static Config`. - Server: externally-resolved private content carries no pnpr policy, so a served package whose fetch used a forwarded credential is gated per user against the owning registry via a persistent `(user, name@version)` grant table — serving a cache hit only to a user the registry has cleared, re-verifying first-time versions, and clearing grants on a 401/403. - Clients: pacquet `pnpr-client` and `@pnpm/pnpr.client` send registry/namedRegistries/authHeaders + Authorization; the TS path sources them from the caller's registry credentials via `@pnpm/network.auth-header`. Co-authored-by: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads client per-registry Authorization headers into pnpr POST /v1/install, passes them through pacquet resolution/fetch as an auth_override, and enforces served-package access via per-user grant and global public-package SQLite caches. ChangesForwarded Credential Install Flow
Sequence Diagram (high-level flow) sequenceDiagram
participant CLI as PNPM CLI
participant PNPR as pnpr InstallAccelerator
participant PACQUET as pacquet resolver/fetch
participant REG as Upstream Registry
participant DB as pnpr SQLite stores
CLI->>PNPR: POST /v1/install (Authorization, authHeaders, registry...)
PNPR->>PACQUET: verify_input_lockfile + resolve (auth_override=request_auth)
PACQUET->>REG: fetch packument/tarball (with forwarded authHeaders)
REG-->>PACQUET: 200 or 401/403
PACQUET-->>PNPR: fetched tarballs (list of pkg_ids)
PNPR->>DB: record grants for freshly fetched pkg_ids / check public_packages
PNPR-->>CLI: gzipped CAFS payload (after authorize_served_packages)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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 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 |
`cargo doc --document-private-items` (the CI Doc job) rejected two stale `deny_unauthorized_packages` intra-doc links left over from splitting the gate into `authorize_served_packages` (local policy + per-user grants).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12189 +/- ##
==========================================
+ Coverage 87.56% 87.69% +0.12%
==========================================
Files 269 271 +2
Lines 30817 31082 +265
==========================================
+ Hits 26984 27256 +272
+ Misses 3833 3826 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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": 4.820990617579999,
"stddev": 0.08265049140469188,
"median": 4.803906982979999,
"user": 2.51170556,
"system": 3.7541995200000002,
"min": 4.73748448648,
"max": 4.99584779748,
"times": [
4.77224883448,
4.91101083848,
4.99584779748,
4.84330121848,
4.78843459048,
4.75359360248,
4.73987328948,
4.81937937548,
4.73748448648,
4.848732142479999
]
},
{
"command": "pacquet@main",
"mean": 4.793340935180001,
"stddev": 0.028181553179487007,
"median": 4.79193752298,
"user": 2.53088686,
"system": 3.7133079200000005,
"min": 4.74828113548,
"max": 4.83664889348,
"times": [
4.823453993479999,
4.74828113548,
4.79087325948,
4.77611151048,
4.81815983348,
4.79300178648,
4.7701744024799995,
4.76798634948,
4.80871818748,
4.83664889348
]
},
{
"command": "pnpr@HEAD",
"mean": 1.9991289711800004,
"stddev": 0.03562251602343498,
"median": 2.00513712198,
"user": 2.69144106,
"system": 3.24936742,
"min": 1.94425077548,
"max": 2.05385932448,
"times": [
2.05385932448,
1.9707298214800002,
2.01027383848,
1.9906859814800002,
1.94988298948,
1.94425077548,
2.0297310714799996,
2.00000040548,
2.02793705748,
2.0139384464799996
]
},
{
"command": "pnpr@main",
"mean": 2.02791215218,
"stddev": 0.07161453772074151,
"median": 2.0179602914799997,
"user": 2.70730616,
"system": 3.2282206200000005,
"min": 1.94451417748,
"max": 2.13784160648,
"times": [
2.13784160648,
1.97012226148,
1.94895474548,
2.00108842548,
1.94451417748,
2.0798491864799997,
1.9745568344800002,
2.03483215748,
2.05775486248,
2.1296072644799997
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6929825153000001,
"stddev": 0.04601094449235311,
"median": 0.6794343995000001,
"user": 0.3903781,
"system": 1.32824612,
"min": 0.6724159545,
"max": 0.8234690485000001,
"times": [
0.8234690485000001,
0.6852533665,
0.6814774025000001,
0.6794441975000001,
0.6794246015000001,
0.6724159545,
0.6738170625000001,
0.6748073845000001,
0.6808534795000001,
0.6788626555
]
},
{
"command": "pacquet@main",
"mean": 0.7166063859,
"stddev": 0.06200944835175567,
"median": 0.697682723,
"user": 0.3792539,
"system": 1.34374082,
"min": 0.6602254785,
"max": 0.8658042355000001,
"times": [
0.7385158065,
0.6712955755000001,
0.6674156475,
0.6844328695,
0.6784322555000001,
0.7363286755,
0.7526807385000001,
0.7109325765000001,
0.6602254785,
0.8658042355000001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6894460507000001,
"stddev": 0.020575066904687453,
"median": 0.6815862735000001,
"user": 0.38863229999999993,
"system": 1.34321032,
"min": 0.6668454735000001,
"max": 0.7242842885,
"times": [
0.7047637845000001,
0.6744705805000001,
0.6803398035000001,
0.6736636745000001,
0.6802297235000001,
0.6828327435000001,
0.6838931985000001,
0.6668454735000001,
0.7231372365000001,
0.7242842885
]
},
{
"command": "pnpr@main",
"mean": 0.7310278517000001,
"stddev": 0.06645851991200141,
"median": 0.726269542,
"user": 0.3662406,
"system": 1.34675842,
"min": 0.6529946385000001,
"max": 0.9038890695,
"times": [
0.9038890695,
0.7200717305000001,
0.7351305815000001,
0.7272589225000001,
0.6892658645,
0.7252801615000001,
0.6529946385000001,
0.7340066575,
0.6882085085,
0.7341723825
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.21164821824,
"stddev": 0.02858495516211038,
"median": 2.20486558304,
"user": 3.5550511799999995,
"system": 2.9991082399999995,
"min": 2.16769294354,
"max": 2.2509739005400005,
"times": [
2.1814005255400004,
2.19749052554,
2.20103195254,
2.2509739005400005,
2.16769294354,
2.20869921354,
2.1998192045400002,
2.2156578925400003,
2.2456141515400003,
2.2481018725400004
]
},
{
"command": "pacquet@main",
"mean": 2.18797088474,
"stddev": 0.050886605050339366,
"median": 2.1753361250400003,
"user": 3.5570728800000007,
"system": 2.9803911399999996,
"min": 2.1264871865400004,
"max": 2.2955580245400005,
"times": [
2.16739256054,
2.18952212854,
2.13956817154,
2.15346284254,
2.1667703615400002,
2.2955580245400005,
2.21936737554,
2.2383005065400003,
2.1832796895400004,
2.1264871865400004
]
},
{
"command": "pnpr@HEAD",
"mean": 2.21438627924,
"stddev": 0.02599883901480438,
"median": 2.2168910700400004,
"user": 3.5661224799999993,
"system": 2.99338464,
"min": 2.17672893754,
"max": 2.2553994575400003,
"times": [
2.24013638554,
2.2553994575400003,
2.23201905754,
2.22939759054,
2.1859242895400004,
2.2125852065400005,
2.2211969335400004,
2.17672893754,
2.18546734854,
2.20500758554
]
},
{
"command": "pnpr@main",
"mean": 2.19660841964,
"stddev": 0.05181262475134038,
"median": 2.1915762780400003,
"user": 3.5452370799999997,
"system": 3.0071622399999995,
"min": 2.13073696054,
"max": 2.30917880454,
"times": [
2.20291028454,
2.2007752775400005,
2.15783496354,
2.25276385554,
2.1723581695400003,
2.30917880454,
2.13073696054,
2.20087725054,
2.15627135154,
2.18237727854
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3368636365400002,
"stddev": 0.011853297773738409,
"median": 1.33495645084,
"user": 1.4606222800000002,
"system": 1.73028008,
"min": 1.31563712634,
"max": 1.3562401583400001,
"times": [
1.3352535383400002,
1.31563712634,
1.3346593633400001,
1.3562401583400001,
1.32535927534,
1.34639021334,
1.3428997843400001,
1.33423213634,
1.3478027133400001,
1.33016205634
]
},
{
"command": "pacquet@main",
"mean": 1.3633129912400002,
"stddev": 0.025550819514170812,
"median": 1.36900245634,
"user": 1.44642148,
"system": 1.76424708,
"min": 1.3313702203400002,
"max": 1.40783854834,
"times": [
1.3440659723400001,
1.33650653034,
1.38963717534,
1.3758103463400002,
1.3693501543400002,
1.3313702203400002,
1.40783854834,
1.3686547583400002,
1.37404821334,
1.33584799334
]
},
{
"command": "pnpr@HEAD",
"mean": 1.36471296634,
"stddev": 0.037547941990714144,
"median": 1.36277593434,
"user": 1.46041278,
"system": 1.7483583799999998,
"min": 1.32809285934,
"max": 1.45828801434,
"times": [
1.32809285934,
1.3763590463400002,
1.37316000634,
1.37194254034,
1.45828801434,
1.3475633253400001,
1.3631948773400002,
1.36235699134,
1.3369307413400002,
1.32924126134
]
},
{
"command": "pnpr@main",
"mean": 1.35262440734,
"stddev": 0.044901892089720244,
"median": 1.3427206953400002,
"user": 1.4305844799999998,
"system": 1.7467543799999998,
"min": 1.31399140734,
"max": 1.4746106943400001,
"times": [
1.34238672434,
1.31399140734,
1.3365595133400001,
1.34305466634,
1.4746106943400001,
1.3395248133400002,
1.34799862534,
1.36114757534,
1.3208416093400002,
1.3461284443400001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12189 |
| Testbed | pacquet |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| isolated-linker.fresh-restore.cold-cache.cold-store | Latency seconds (s) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 4.82 s(+84.90%)Baseline: 2.61 s | 3.13 s (154.08%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,211.65 ms(-3.58%)Baseline: 2,293.70 ms | 2,752.45 ms (80.35%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,336.86 ms(-8.01%)Baseline: 1,453.34 ms | 1,744.00 ms (76.65%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 4,820.99 ms(+84.90%)Baseline: 2,607.40 ms | 3,128.87 ms (154.08%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 692.98 ms(+3.99%)Baseline: 666.42 ms | 799.70 ms (86.66%) |
A forwarded credential matching a package's registry only means pnpr fetched it with the caller's token, not that the package is private. When one registry serves both private and public packages (the mixed-proxy deployment), the token matches every package, so the grant table would gate public content per user — a grant row and an upstream re-verify each. Classify packages globally instead: on a cache hit for a not-yet-classified package, probe the registry anonymously. A 2xx means it is public — record it in a global set so every later request (any user) short-circuits with no grant row and no round trip; a 401/403 means it is private and falls through to the existing per-user grant / re-verify path.
Review Summary by QodoForward credentials and add per-user access grants for external private registries
WalkthroughsDescription• Forward caller's per-registry credentials to pnpr server for private package resolution • Add per-user access grants table for externally-resolved private content • Implement global public-package classification to avoid per-user gating overhead • Gate served packages by regime: pnpr-as-authority (local policy) or upstream-as-authority (per-user grants) Diagramflowchart LR
Client["Client<br/>(pnpm/pacquet)"]
PnprServer["Pnpr Server<br/>(install accelerator)"]
ExtRegistry["External<br/>Registry"]
GrantTable["Grant Table<br/>(SQLite)"]
PublicSet["Public Set<br/>(SQLite)"]
Client -->|authHeaders + Authorization| PnprServer
PnprServer -->|resolve/fetch with<br/>forwarded creds| ExtRegistry
PnprServer -->|record/check<br/>per-user grants| GrantTable
PnprServer -->|classify<br/>public packages| PublicSet
ExtRegistry -->|401/403| PnprServer
File Changes1. pnpr/crates/pnpr/src/install_accelerator.rs
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/cli/src/cli_args/install.rs`:
- Around line 472-481: The code is sending all entries from
state.config.auth_headers to the pnpr server; restrict auth_headers to only the
entries relevant to the selected registry(s) before serializing: build a set of
target URLs/hosts from the chosen registry and any named_registries used for
this install, then replace the auth_headers mapping construction in the install
payload to filter state.config.auth_headers.entries() to only those whose URL
matches one of the target registry URLs/hosts (and omit the pnpr_server entry
since authorization already sends that token via authorization =
state.config.auth_headers.for_url(pnpr_server)); update the auth_headers field
(and keep authorization as-is) so only relevant credentials are forwarded to
/v1/install.
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 435-439: The code computes auth_headers from auth_override but
only passes it to resolvers, leaving fetch paths (PrefetchingResolver,
CreateVirtualStore and any fetch context construction) using config.auth_headers
and thus missing request-scoped credentials; modify the code that builds fetch
contexts and constructs PrefetchingResolver/CreateVirtualStore (and any
functions that build a FetchContext or FetchOptions around lines where
PrefetchingResolver and CreateVirtualStore are created) to accept the
auth_override-derived headers (auth_headers) and thread that Arc<Headers> into
their fetch contexts so tarball downloads use the same request-scoped auth as
the resolver; ensure the same change is applied to the other occurrences noted
(around the other blocks referenced at lines ~686-696 and ~1250-1265).
🪄 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: 4b8ad711-1d54-4260-92ba-72795edaad5c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.changeset/pnpr-forward-credentials.mdinstalling/deps-installer/package.jsoninstalling/deps-installer/src/install/index.tsinstalling/deps-installer/tsconfig.jsonnetwork/auth-header/src/index.tspacquet/crates/cli/src/cli_args/install.rspacquet/crates/network/src/auth.rspacquet/crates/package-manager/src/add.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/remove.rspacquet/crates/package-manager/src/update.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/pnpr-client/tests/integration.rspacquet/crates/resolving-npm-resolver/src/lib.rspnpr/client/src/fetchFromPnpmRegistry.tspnpr/crates/pnpr/src/config.rspnpr/crates/pnpr/src/install_accelerator.rspnpr/crates/pnpr/src/install_accelerator/grant_table.rspnpr/crates/pnpr/src/install_accelerator/grant_table/tests.rspnpr/crates/pnpr/src/install_accelerator/protocol.rspnpr/crates/pnpr/src/install_accelerator/public_packages.rspnpr/crates/pnpr/src/install_accelerator/public_packages/tests.rspnpr/crates/pnpr/src/install_accelerator/resolve.rspnpr/crates/pnpr/src/install_accelerator/tests.rs
… uses Addresses a review comment: both clients forwarded the caller's entire credential store to the pnpr server, so a token for a host unrelated to the install leaked on every request. Scope the forwarded `authHeaders` to the registries this install resolves against — the default registry plus every `namedRegistries` alias — keyed by their nerf-darted URIs. This also drops the pnpr-server's own credential from the body (it already travels in the request `Authorization` header). Reverts the now-unused `getAuthHeadersFromCreds` re-export from `@pnpm/network.auth-header`; the TS path builds the scoped map with the already-exported `createGetAuthHeaderByURI` plus `@pnpm/config.nerf-dart`.
Reverts the registry-scoped credential forwarding: the set of registries a dependency graph touches isn't knowable up front. A transitive package can be scope-routed to a different registry, or pinned to a tarball URL on a host that lives in `.npmrc` but isn't a declared registry, so filtering to the default + named registries silently dropped tokens that private sub-dependencies need (a 401 on the server). Forwarding the whole map lets pnpr attach the right token per fetched URL exactly as a local install does — these are package-fetch credentials going to the service the caller configured to fetch its packages. Comments at both call sites record why.
Trim the doc and inline comments introduced in this PR to a few lines each, leaning on the descriptively-named tests for the per-path detail. No behavior change.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pnpr/crates/pnpr/src/install_accelerator/public_packages.rs (1)
22-24:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftKey this cache by registry as well as package name.
authorize_upstream_package()classifies anonymous access against a specificregistry, butPublicPackagespersists onlyname. Oncefoois recorded as public from registry A, a later cache hit for privatefoofrom registry B will skip both the grant check and the authenticated upstream probe becauseis_public(name, ...)returns true. That makes cross-registry name collisions an authorization bypass.Suggested direction
CREATE TABLE IF NOT EXISTS public_packages ( - name TEXT PRIMARY KEY, + registry TEXT NOT NULL, + name TEXT NOT NULL, classified_at_ms INTEGER NOT NULL + PRIMARY KEY (registry, name) );Then thread the normalized registry URI through
is_public()/record()and update the call sites inpnpr/crates/pnpr/src/install_accelerator.rsto query and record by(registry, name), not justname.Also applies to: 42-44, 63-69
🤖 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 `@pnpr/crates/pnpr/src/install_accelerator/public_packages.rs` around lines 22 - 24, PublicPackages currently keys only by package name, causing cross-registry collisions; change its API and storage to key by (registry, name): update the PublicPackages struct and its underlying Connection usage to store and query a composite key (normalized registry URI + package name), then modify the methods is_public(name) and record(name) to accept a registry parameter (e.g., is_public(registry, name) and record(registry, name)) and use the normalized registry URI when forming the key; finally, update all call sites (notably authorize_upstream_package and the callers in pnpr/crates/pnpr/src/install_accelerator.rs) to pass the normalized registry URI along with the package name when querying and recording public packages.
🤖 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.
Outside diff comments:
In `@pnpr/crates/pnpr/src/install_accelerator/public_packages.rs`:
- Around line 22-24: PublicPackages currently keys only by package name, causing
cross-registry collisions; change its API and storage to key by (registry,
name): update the PublicPackages struct and its underlying Connection usage to
store and query a composite key (normalized registry URI + package name), then
modify the methods is_public(name) and record(name) to accept a registry
parameter (e.g., is_public(registry, name) and record(registry, name)) and use
the normalized registry URI when forming the key; finally, update all call sites
(notably authorize_upstream_package and the callers in
pnpr/crates/pnpr/src/install_accelerator.rs) to pass the normalized registry URI
along with the package name when querying and recording public packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 583dbd09-09aa-42cc-86d4-546f8b6fcf1e
📒 Files selected for processing (13)
installing/deps-installer/src/install/index.tspacquet/crates/cli/src/cli_args/install.rspacquet/crates/network/src/auth.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/pnpr-client/src/lib.rspnpr/client/src/fetchFromPnpmRegistry.tspnpr/crates/pnpr/src/config.rspnpr/crates/pnpr/src/install_accelerator.rspnpr/crates/pnpr/src/install_accelerator/grant_table.rspnpr/crates/pnpr/src/install_accelerator/protocol.rspnpr/crates/pnpr/src/install_accelerator/public_packages.rspnpr/crates/pnpr/src/install_accelerator/resolve.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- pnpr/crates/pnpr/src/install_accelerator/protocol.rs
- pnpr/client/src/fetchFromPnpmRegistry.ts
- pacquet/crates/network/src/auth.rs
- pnpr/crates/pnpr/src/config.rs
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
- pnpr/crates/pnpr/src/install_accelerator/resolve.rs
- pacquet/crates/cli/src/cli_args/install.rs
- pnpr/crates/pnpr/src/install_accelerator/grant_table.rs
- pnpr/crates/pnpr/src/install_accelerator.rs
- pacquet/crates/pnpr-client/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/deps-installer/src/install/index.ts
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.rs
pnpr/**/pnpr/**/*.rs
📄 CodeRabbit inference engine (pnpr/AGENTS.md)
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
Follow the pacquet contributing guide (../pacquet/CONTRIBUTING.md) for test layout and Rust conventions
Files:
pnpr/crates/pnpr/src/install_accelerator/public_packages.rs
🧠 Learnings (25)
📓 Common learnings
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/cli/src/cli_args/install.rs:0-0
Timestamp: 2026-06-04T14:55:48.516Z
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.
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:25.306Z
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: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:01.216Z
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: 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: 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: 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.
📚 Learning: 2026-06-04T14:55:48.516Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/cli/src/cli_args/install.rs:0-0
Timestamp: 2026-06-04T14:55:48.516Z
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:
installing/deps-installer/src/install/index.tspacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-06-04T14:40:25.306Z
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:25.306Z
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:
installing/deps-installer/src/install/index.tspacquet/crates/package-manager/src/install.rspnpr/crates/pnpr/src/install_accelerator/public_packages.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:
installing/deps-installer/src/install/index.ts
📚 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:
installing/deps-installer/src/install/index.ts
📚 Learning: 2026-06-04T06:04:01.216Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:01.216Z
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:
installing/deps-installer/src/install/index.ts
📚 Learning: 2026-05-24T08:18:06.019Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11895
File: pnpm/test/deploy.ts:91-95
Timestamp: 2026-05-24T08:18:06.019Z
Learning: In the pnpm/pnpm repository, integration tests that hit the real `registry.npmjs.org` (e.g., for `pacquet` or `pnpm/pacquet`) do NOT use a runtime env-var gate (such as `PNPM_RUN_PUBLIC_REGISTRY_TESTS`). They simply pass `--config.registry=https://registry.npmjs.org/` directly to `execPnpm` and set a higher timeout. This is the established pattern, as seen in `pnpm/test/install/pacquet.ts` and `pnpm/test/deploy.ts`. Do not suggest adding env-var guards for these tests.
Applied to files:
installing/deps-installer/src/install/index.ts
📚 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:
installing/deps-installer/src/install/index.ts
📚 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: Do not change lockfile format, store layout, `.npmrc` semantics, or CLI surface unless pnpm changed them first
Applied to files:
installing/deps-installer/src/install/index.ts
📚 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:
installing/deps-installer/src/install/index.ts
📚 Learning: 2026-05-26T18:31:14.579Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11967
File: .changeset/git-fetcher-reject-non-sha-commits.md:2-2
Timestamp: 2026-05-26T18:31:14.579Z
Learning: In the pnpm monorepo, the `fetching/` directory contains multiple separate npm packages each with their own scoped name using a dot-separator convention, e.g., `pnpm/fetching.git-fetcher` (declared in `fetching/git-fetcher/package.json`) and `pnpm/fetching.tarball-fetcher`. There is no top-level `pnpm/fetching` package. Changesets targeting the git-fetcher should use `"pnpm/fetching.git-fetcher"`.
Applied to files:
installing/deps-installer/src/install/index.ts
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
installing/deps-installer/src/install/index.ts
📚 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.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.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.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.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.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.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.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:
pnpr/crates/pnpr/src/install_accelerator/public_packages.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 : Cargo.toml files for new registry-only crates must use the pnpr- prefix for the package name
Applied to files:
pnpr/crates/pnpr/src/install_accelerator/public_packages.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:
pnpr/crates/pnpr/src/install_accelerator/public_packages.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:
pnpr/crates/pnpr/src/install_accelerator/public_packages.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:
pnpr/crates/pnpr/src/install_accelerator/public_packages.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:
pnpr/crates/pnpr/src/install_accelerator/public_packages.rs
…error path Adds the missing keystone tests for the feature's core behavior: - a forwarded upstream token lets the accelerator resolve and fetch `@pnpm.e2e/needs-auth` (which the test registry gates behind `$authenticated`), and the same install fails without it; - an unreachable upstream during re-verify surfaces as a 502. The end-to-end tests register a user with the shared test registry to obtain a real bearer token, so resolution and fetch exercise the forwarded credential rather than mocking the gate in isolation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/pnpr-client/tests/integration.rs`:
- Around line 55-58: The nerf_key helper currently strips registry path prefixes
by only keeping the host[:port]; update nerf_key to preserve a leading registry
path segment when present: parse the URL after "://" into segments (e.g., let
mut parts = url.split("://").nth(1).unwrap_or(url).split('/'); let host =
parts.next().unwrap_or(""); let maybe_prefix = parts.next().filter(|s|
!s.is_empty()); then format the key as "//{host}/{prefix}/" when maybe_prefix
exists or "//{host}/" otherwise so registries with path prefixes are correctly
represented; modify the nerf_key function accordingly.
- Around line 179-181: The test currently only checks that
client.install(opts).await returned an Err (result), which may hide unrelated
failures; update the assertion to match the expected authentication-denied error
shape from the pnpr client/server (inspect the returned Result from
client.install(opts).await and assert it contains the server error with HTTP
status 401 or 403). Concretely, replace the generic assert!(result.is_err())
with code that unwraps/matches result and asserts the error variant contains a
server response with status code 401 or 403 (referencing the variables/functions
result, client.install, and opts to locate the assertion).
🪄 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: 4bbc731d-5504-490a-b3b5-d8d8649ab60e
📒 Files selected for processing (3)
pacquet/crates/pnpr-client/Cargo.tomlpacquet/crates/pnpr-client/tests/integration.rspnpr/crates/pnpr/src/install_accelerator/tests.rs
✅ Files skipped from review due to trivial changes (1)
- pacquet/crates/pnpr-client/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pnpr/crates/pnpr/src/install_accelerator/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: 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/pnpr-client/tests/integration.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::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
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/pnpr-client/tests/integration.rs
🧠 Learnings (16)
📓 Common learnings
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/cli/src/cli_args/install.rs:0-0
Timestamp: 2026-06-04T14:55:48.516Z
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.
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:25.306Z
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: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:01.216Z
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: 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: 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.
📚 Learning: 2026-06-04T14:55:48.516Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12189
File: pacquet/crates/cli/src/cli_args/install.rs:0-0
Timestamp: 2026-06-04T14:55:48.516Z
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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.rs
📚 Learning: 2026-06-04T14:40:25.306Z
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:25.306Z
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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.rs
📚 Learning: 2026-05-24T08:18:06.019Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11895
File: pnpm/test/deploy.ts:91-95
Timestamp: 2026-05-24T08:18:06.019Z
Learning: In the pnpm/pnpm repository, integration tests that hit the real `registry.npmjs.org` (e.g., for `pacquet` or `pnpm/pacquet`) do NOT use a runtime env-var gate (such as `PNPM_RUN_PUBLIC_REGISTRY_TESTS`). They simply pass `--config.registry=https://registry.npmjs.org/` directly to `execPnpm` and set a higher timeout. This is the established pattern, as seen in `pnpm/test/install/pacquet.ts` and `pnpm/test/deploy.ts`. Do not suggest adding env-var guards for these tests.
Applied to files:
pacquet/crates/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.rs
📚 Learning: 2026-06-04T06:04:01.216Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12181
File: worker/src/start.ts:504-520
Timestamp: 2026-06-04T06:04:01.216Z
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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.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/pnpr-client/tests/integration.rs
Per review: assert the no-credential install fails specifically with a 401-bearing server error (not just any error), and make the test's `nerf_key` helper preserve a registry path prefix so it can't key a forwarded credential wrong for a path-prefixed registry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #12184 (part 2).
#12181 shipped the per-caller access gate on
POST /v1/install, which authorizes every served package against pnpr's ownpackages:policy — the complete answer while pnpr fetches anonymously. This PR adds the remaining piece: forwarding the caller's per-registry credentials so the accelerator can resolve/fetch external private content as the caller, and gating that content per user against the registry that actually owns it.Credential forwarding (issue steps 1–2)
POST /v1/installgains anauthHeadersbody map ({ "//host/path/": "Bearer …" }, the shapeAuthHeaders::from_mapconsumes /getAuthHeadersFromCredsproduces) plus an HTTPAuthorizationheader. The body map carries the upstream registry tokens; the header identifies the caller to pnpr's own gate and keys the grant table.Arc<AuthHeaders>is threaded via a newInstall.auth_overridefield and anauth_overrideparam onbuild_resolution_verifiers, so resolution/verification run as the caller without baking per-user auth into the interned&'static Config(which would leak one config per user).handle_installbuilds the per-requestAuthHeadersand threads it through resolve, verify, andfetch_uncached(which now returns the freshly-fetched set).pnpr-clientand@pnpm/pnpr.clientsendregistry/namedRegistries/authHeaders+Authorization; the TS path sources them from the caller's registry credentials via@pnpm/network.auth-header(getAuthHeadersFromCredsis newly re-exported).@pnpm/workeris unchanged — downloads happen server-side..npmrcbut isn't a declared registry — so pnpr attaches the right token per fetched URL exactly as a local install does. These are package-fetch credentials going to the very service the caller configured to fetch its packages.Per-user grant table (issue steps 3–4)
Externally-resolved private content carries no pnpr policy, so the store's possession of the bytes must not authorize a user the upstream never cleared. A served package is dispatched by whether a forwarded credential was used to fetch it:
packages:policy check, unchanged.(user, name@version)grant table (SQLite, modeled onVerdictCache). Freshly fetched this request ⇒ record + allow (the upstream just accepted the token). Cache hit with a standing grant ⇒ allow, no upstream trip. Cache hit, no grant ⇒ re-verify against the owning registry with the caller's credential — record on success; clear-on-discovery (purge the user's grants for the package) + deny on401/403. TTL is theinstallAccelerator.grantTtlconfig knob (default: permanent).Public vs private (no per-user gating for public packages)
A forwarded credential matching a registry doesn't mean a package is private — in a mixed proxy (one registry serving a company's private packages and public ones), the token matches everything, and gating public content per user would cost a grant row and a re-verify round trip per user for bytes anyone may read. So before the per-user path, a not-yet-classified cache hit is probed anonymously: a
2xxclassifies the package public in a global set (no user pays for it again, no grant, no further round trip); a401/403means it's genuinely private and falls through to the grant / re-verify path above. Public packages thus cost one anonymous probe across the whole fleet, not one per user.Tests
pnpr-client: a test assertingauthHeaders+Authorizationtravel on the wire.pnpr(237),pacquet-package-manager(389),pacquet-pnpr-client(12),pacquet-network/config(325); clippy-D warnings,cargo fmt, rustdoc-D warnings --document-private-items,typos, and the TS compile all clean.Scoped follow-ups (not in this PR)
401/403during the cold resolve aborts the request anyway (nothing is served); threading the offending package out of the deep resolve error to also clear stale grants for future requests needs structured auth errors.@scope:registryrouting incollect_packages.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
New Features
Tests
Documentation