Skip to content

perf: cache the post-resolution lockfile verification gate#11691

Merged
zkochan merged 23 commits into
mainfrom
11687-2
May 17, 2026
Merged

perf: cache the post-resolution lockfile verification gate#11691
zkochan merged 23 commits into
mainfrom
11687-2

Conversation

@zkochan

@zkochan zkochan commented May 16, 2026

Copy link
Copy Markdown
Member

Closes #11687.

What

Cache the result of the post-resolution lockfile verification gate (#11583) so repeat installs against an unchanged lockfile skip the per-package registry round trips entirely. Persisted as JSON Lines at <cacheDir>/lockfile-verified.jsonl.

The cache layer is policy-neutral. Today there's one verifier (minimumReleaseAge); future resolver-side verifiers (jsr trust, attestation, …) plug in by declaring their own policy slot and canTrustPastCheck comparator — no install-side changes.

Why

#11583 re-hits the registry on every install for every locked (name, version) pair. On warm/repeat installs where the lockfile hasn't moved, that's a stack of per-package round trips with nothing to show for them. This change makes the steady-state case effectively free without weakening the protection — the gate still runs in full whenever the lockfile changes, any verifier's policy tightens, or no record exists.

How

Cache lookup, in order

The cache is indexed by content hash so git worktrees with identical lockfile bytes share a cache entry. A secondary path-keyed index drives the same-machine stat shortcut.

  1. stat() shortcut — when a previous record for this exact lockfilePath matches today's size + mtime + inode, trust the cached hash without reading anything. Zero I/O beyond the stat. Microseconds.
  2. Content lookup — hash the in-memory lockfile (not the file bytes — we already have the parsed object) and look up by content hash. Catches worktrees (same content, different path) and CI checkouts (same content, reset stat). On hit, append a refreshed path/stat entry so the next install at this path takes the stat shortcut.
  3. Any active verifier rejects the cached policy — run the full gate.
  4. No record — run the full gate.

The in-memory object is hashed with hashObject from @pnpm/crypto.object-hasher (streaming, key-order-stable).

Record shape

{
  "lockfile": {
    "hash": "<sha256 base64>",
    "path": "/abs/path/to/pnpm-lock.yaml",
    "size": 154,
    "mtimeNs": "1736245123000000000",
    "inode": "12345"
  },
  "verifiedAt": "2026-05-17T...",
  "policy": { "minimumReleaseAge": 1440 }
}

policy is the union of every active verifier's policy contribution. Verifiers checking the same logical policy (e.g. minimumReleaseAge honored by multiple registries) name it the same and share the slot — no resolver namespacing.

File semantics

  • Sync fs throughout — the cache is consulted once before verification fan-out and recorded once after. No concurrent install work to overlap with; keeping the call sites straight-line.
  • JSONL appends are atomic on POSIX/NTFS, so parallel pnpm processes (monorepo installs, CI matrices sharing a cache) write without coordination. Latest record per (path, hash) tuple wins on read.
  • Bounded file — capped at ~1000 entries; compaction is triggered by a single stat() of the cache file (1.5 MiB byte budget) so we never parse the file on the steady-state path. When triggered, the tail is rewritten via tempfile + rename.
  • No record on rejection — a failing verification deliberately doesn't write a record; the next install must rerun the gate.
  • Single hash per install — the in-memory hash is computed lazily and reused: tryLockfileVerificationCache returns the precomputed stat+hash to recordVerification on a miss, and the stat-shortcut hit forwards the cached record's hash unchanged.

Plumbing

The verifier contract changed alongside the cache to make this composable without install-side knowledge of each policy:

  • @pnpm/resolving.resolver-baseResolutionVerifier is now { verify, policy, canTrustPastCheck } (was a bare function in fix: enforce minimumReleaseAge on existing lockfile entries #11583). Each resolver-side verifier owns its policy snapshot and the comparator that decides whether a cached policy is still trustworthy.
  • @pnpm/resolving.npm-resolvercreateNpmResolutionVerifier returns the new shape: policy: { minimumReleaseAge }, canTrustPastCheck reads minimumReleaseAge from the merged cached bag.
  • @pnpm/resolving.default-resolvercreateResolutionVerifier (singular, returning a combined function) → createResolutionVerifiers (plural, returning a ResolutionVerifier[]). No combinator; each verifier handles its own protocol short-circuit inside verify, so dispatch happens naturally at the install side.
  • @pnpm/installing.clientClient.verifyResolution?Client.resolutionVerifiers: ResolutionVerifier[]. Same rename propagates through @pnpm/store.connection-manager, @pnpm/testing.temp-store, and StrictInstallOptions.
  • @pnpm/installing.deps-installer — new verifyLockfileResolutionsCache.ts (tryLockfileVerificationCache + recordVerification). verifyLockfileResolutions takes the verifier list plus cacheDir + lockfilePath as flat options; the cache fires when both are present, otherwise the gate runs without memoization. The dedup key for in-flight candidates includes a serialization of resolution so two entries sharing a (name, version) but pinned via different protocols don't collapse.

Breaking but safe — @pnpm/resolving.npm-resolver hasn't been released since #11583 introduced the verifier abstraction, so no downstream consumer is on the old shape.

Tests

  • 17 unit tests in verifyLockfileResolutionsCache.ts: cache miss/hit, stat shortcut, size mismatch falling through to hash lookup, hash-fallback on reset stat, content change with matching size, stricter/weaker policy, missing-field policy rejection, multi-verifier policy merge (shared field stored once), worktree case (same content, different path), JSONL append semantics, malformed-line tolerance.
  • 12 integration tests in verifyLockfileResolutions.ts: dedup of peer/patch-suffix variants, distinct-resolution dedup at the same (name, version), stable violation ordering, the 20-entry cap, multi-verifier fan-out (first failure wins), cache short-circuit on a passing run, no cache write on a rejecting run, empty-verifier-list passthrough.
  • 1 e2e test in pnpm/test/install/minimumReleaseAge.ts: bundled CLI plumbing — install once to seed the lockfile, enable minimumReleaseAge + cacheDir, install again, assert the cache file lands at <cacheDir>/lockfile-verified.jsonl with the documented record shape.
  • Existing minimumReleaseAge (13) and frozenLockfile (12) suites still pass.

Out of scope

  • Attestation endpoint (issue item 2). Hitting /-/npm/v1/attestations/<pkg> before the full-metadata document avoids the multi-megabyte download for provenance-published packages, but requires Sigstore verification and only covers a subset of npm. Worth doing as a follow-up — the cache delivers most of the steady-state win without it.

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

…ification

The post-resolution gate added in #11583 re-hits the registry on every
install for every locked (name, version) pair. On warm/repeat installs
where the lockfile hasn't moved, that's a stack of per-package round
trips with nothing to show for them.

Add a per-lockfile cache at `<cacheDir>/minimum-release-age-verified.jsonl`.
A successful verification appends one JSONL record per lockfile path
(content hash + stat fields + the cutoff it ran under). On the next
install:

- Cache miss (no record) — run the gate.
- Stat fast path — size + mtime + inode all match, skip the gate. Zero
  registry calls, zero file reads beyond the stat.
- Size differs — guaranteed-different content, run the gate.
- Stat differs but size matches (typical after a CI checkout) — hash the
  file. If the hash matches and the cached cutoff is at least as strict
  as today's, skip the gate and refresh the stat fields so the next
  install hits the fast path.
- Tighter cutoff than the cached one — run the gate. The previously
  verified set may now include in-cooldown versions.

JSONL appends are atomic on POSIX/NTFS, so parallel pnpm processes
(monorepo installs, CI matrices sharing a cache) can write without
locking — last record per path wins on read. The file is capped at
~1000 entries; once it crosses the cap with slack, it's compacted to
the latest N records via tempfile + rename.

Closes #11687.
@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors single optional verifier into per-resolver ResolutionVerifier objects and adds a JSONL per-lockfile verification cache (stat fast-path + hash fallback); threads resolver verifier arrays and optional cacheDir through client, store, install pipeline, commands, and tests.

Changes

Multi-verifier architecture with lockfile verification caching

Layer / File(s) Summary
ResolutionVerifier type and npm verifier
resolving/resolver-base/src/index.ts, resolving/npm-resolver/src/createNpmResolutionVerifier.ts
ResolutionVerifier becomes an interface with verify(), policy, and canTrustPastCheck(); npm verifier now returns an object exposing policy: { minimumReleaseAge } and a cache-trust predicate.
Resolver factory
resolving/default-resolver/src/index.ts
createResolutionVerifiers replaces the old single-factory and returns ResolutionVerifier[] (conditionally including the npm verifier).
Client / store wiring
installing/client/src/index.ts, store/connection-manager/src/createNewStoreController.ts, store/connection-manager/src/index.ts, testing/temp-store/src/index.ts
Thread resolutionVerifiers: ResolutionVerifier[] through client and store controller APIs and returned shapes, replacing the previous optional verifyResolution.
Install options & mutate wiring
installing/deps-installer/src/install/extendInstallOptions.ts, installing/deps-installer/src/install/index.ts
StrictInstallOptions adds resolutionVerifiers: ResolutionVerifier[] and optional cacheDir?: string; install pipeline calls verifyLockfileResolutions with opts.resolutionVerifiers and optional cache config.
verifyLockfileResolutions logic
installing/deps-installer/src/install/verifyLockfileResolutions.ts
Accepts verifiers: ResolutionVerifier[], no-ops on empty input or missing packages, deduplicates candidates by (name, version)+snapshot.resolution, consults cache fast-path, runs verifiers sequentially per candidate with bounded concurrency and early-stop on first failure, and calls recordVerification on success.
Lockfile verification cache layer
installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts
New JSONL cache (lockfile-verified.jsonl) storing per-lockfile records with stats, content hash, verifiedAt, and merged policy; implements stat-only fast-path, hash fallback, per-verifier canTrustPastCheck checks, append-safe writes, and compaction.
Command-level option threading
installing/commands/src/fetch.ts, installing/commands/src/import/index.ts, installing/commands/src/installDeps.ts, installing/commands/src/recursive.ts, installing/commands/src/remove.ts
All command handlers now pass resolutionVerifiers from the store controller into install/mutate options; RecursiveOptions updated accordingly.
Verification tests and cache tests
installing/deps-installer/test/install/verifyLockfileResolutions.ts, installing/deps-installer/test/install/verifyLockfileResolutionsCache.ts, installing/deps-installer/test/utils/testDefaults.ts
Tests updated to use verifier arrays and wrap helpers; new cache-focused tests validate stat-fast-path, hash fallback, merged-policy behavior, malformed JSONL resilience, and record persistence.
Changeset
.changeset/cache-aware-minimum-release-age-gate.md
Documents the multi-verifier refactor, cache semantics, JSONL memoization, stat/hash fast-path behavior, and merged-policy recording rules.

Sequence Diagram

sequenceDiagram
  participant Installer as mutateModules / verifyLockfileResolutions
  participant VerificationCache as try/record lockfile-verified.jsonl
  participant Verifier as ResolutionVerifier (npm, etc.)

  Installer->>VerificationCache: tryLockfileVerificationCache(cacheDir, key)
  VerificationCache-->>Installer: { hit: true } / { hit: false }
  alt cache miss
    Installer->>Verifier: verify(resolution, {name,version})
    Verifier-->>Installer: { ok: true } / { ok: false, reason, code }
    alt all ok
      Installer->>VerificationCache: recordVerification(cacheDir, key)
    end
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • pnpm/pnpm#11583: Introduces the minimumReleaseAge revalidation pipeline using a single verifyResolution; this PR refactors that to a multi-verifier array model with cache-aware validation.

Poem

🐰 I scanned the lockfile, hop by hop,
Verifiers line up, they never stop.
JSONL keeps the trust in store,
Stat fast-paths skip the chore.
Hop, cache, verify — installs go pop!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 24.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'perf: cache the post-resolution lockfile verification gate' accurately summarizes the main objective of this PR: adding a cache mechanism for lockfile verification to improve performance.
Linked Issues check ✅ Passed The PR implements all core requirements from #11687: global per-lockfile JSONL cache with content hash and stat fields (size, mtimeNs, inode), stat-based fast path, hash fallback for CI scenarios, verifier policy trust delegation via canTrustPastCheck, atomic concurrent appends, and file capping with compaction.
Out of Scope Changes check ✅ Passed All changes are within scope of the linked issue. The PR refactors ResolutionVerifier API (function to interface with policy/canTrustPastCheck), implements the cache layer, updates verifier plumbing across installer/commands/store, and adds comprehensive tests—all directly supporting the caching requirement.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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 11687-2

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.

zkochan added 10 commits May 17, 2026 00:14
… verifier

The cache record was minimumReleaseAge-shaped (one named field, one
cached-vs-current comparison baked into the lookup). Today there is only
one verifier, but the resolver chain is set up so every resolver can
declare its own — the cache shape should follow.

Move policy-specific state into a `verifiers: Record<string, unknown>`
slot map keyed by a stable verifier id (today: `npm.minimumReleaseAge`).
The cache layer is now policy-neutral: it stores what each verifier
hands it and asks each active verifier's `satisfies(cachedPolicy)`
comparator whether the cached snapshot still covers today's policy.
A cache hit requires every active verifier to be satisfied — a missing
slot or a mismatched policy reruns the gate.

`buildActiveVerifiers` in install/index.ts is the registration point;
future verifiers (jsr trust policy, attestation, etc.) add a slot there
without touching the cache module. Rename the cache file from
`minimum-release-age-verified.jsonl` to `lockfile-verified.jsonl` so the
filename matches the broader scope.

Also bump the test count: new cases cover multi-verifier hits, missing
slots, and the empty-verifier-list path.
The previous commit moved the cache record to a generic `verifiers[]`
shape, but the install layer was still constructing the slots from
`opts.minimumReleaseAge` — duplicating policy knowledge that belongs
upstream. The resolver-side verifier factory already knows its policy
shape; let it declare its own cache slot, parallel to how it declares
its runtime `verify` function.

- `@pnpm/resolving.resolver-base` exports `ActiveVerifier` and switches
  `ResolutionVerifier` from a function type alias to a callable
  interface with an `activeVerifiers: readonly ActiveVerifier[]`
  property. Existing callers (`verify(resolution, ctx)`) keep working;
  new callers read `.activeVerifiers`.
- `createNpmResolutionVerifier` attaches its slot
  (`npm.minimumReleaseAge` with the cutoff and the >=-cached comparator)
  to the returned function.
- `createResolutionVerifier` combinator flattens sub-verifier slots.
- `installing.deps-installer` reads slots from
  `opts.verifyResolution.activeVerifiers` and drops the local
  `buildActiveVerifiers` helper.

Future verifiers (jsr trust, attestation, etc.) plug in by attaching
their slot to their own factory's return — install side untouched.
…turns a list

The previous shape — a callable function with an attached
`activeVerifiers` array — was clunky to construct (Object.assign onto an
async function) and gestured at a fan-out the type couldn't express.
Each resolver-side verifier owns exactly one slot, so the array was
always length 1; the combinator's job was to merge sibling slots, which
the install side can do trivially over a plain array.

New shape:

  interface ResolutionVerifier {
    verify: (resolution, ctx) => Promise<ResolutionVerification>
    activeVerifier: ActiveVerifier
  }

The default-resolver companion is now `createResolutionVerifiers`
(plural) and returns `ResolutionVerifier[]` — an empty list when no
policy is active. The install side fans out over the list per lockfile
entry; each verifier handles its own protocol short-circuit inside
`verify`, so dispatch happens naturally without a combinator.

Renames along the chain: `Client.verifyResolution?` →
`Client.resolutionVerifiers: ResolutionVerifier[]`, same for
`StoreControllerHandle`, `CreateTempStoreResult`, and
`StrictInstallOptions`. Empty arrays everywhere `undefined` used to
live.

Breaking, but npm-resolver hasn't been released since the verifier
landed in #11583, so no downstream consumer is affected.
…sted handle

The `activeVerifier` field was a holdover from when the install side
constructed slots from policy values; with each resolver owning its
slot, the indirection earns nothing — `ResolutionVerifier.activeVerifier.key`
reads as "the verifier's active verifier's key", which is just noise.

Inline the three cache-relevant fields (`key`, `policy`, `satisfies`)
directly onto `ResolutionVerifier`. The cache module declares a narrower
local type `VerifierCacheIdentity = Pick<ResolutionVerifier, 'key' |
'policy' | 'satisfies'>` so it can't accidentally reach for `verify`.
The cache key was just the resolver's identity all along — each resolver
ships one verifier, and a resolver that wants to enforce multiple
policies (e.g. minimumReleaseAge plus a future attestation check)
bundles them into the `policy` object and combines the comparisons in
`satisfies`. With that in mind, calling the field `key` and prefixing it
('npm.minimumReleaseAge') is needless ceremony: the resolver name
('npm') is the slot.

Cache records now look like:
  { ..., "verifiers": { "npm": 1440 } }

instead of the previous:
  { ..., "verifiers": { "npm.minimumReleaseAge": 1440 } }
"satisfies" was grammatically ambiguous — who satisfies whom? The
function decides whether a previously cached run is trustworthy under
today's policy. `canTrustPastCheck(cachedPolicy)` says exactly that
and matches the security framing of the verification gate.
…lots

A verifier's policy isn't really an attribute of the resolver that
attached the verifier — it's an attribute of what was checked. The
\`verifiers: { npm: 1440 }\` slot map prevented two resolvers from
sharing a logical policy (e.g. \`minimumReleaseAge\` honored by both
npm and jsr) and forced every verifier to carry an extra namespace
field.

Flatten the cache record to a single \`policy: Record<string, unknown>\`
bag built by merging every active verifier's \`policy\` contribution.
Drop the \`resolver\` field from \`ResolutionVerifier\`. The npm
verifier now ships \`policy: { minimumReleaseAge: N }\` instead of
\`policy: N\`; its \`canTrustPastCheck\` reads the field it owns from
the merged cached bag.

Shared fields land in one slot (test added: two verifiers contributing
the same \`minimumReleaseAge\` record once). Disjoint fields coexist
naturally. The convention is informal — verifiers checking the same
logical policy name it the same — and the only cost of disagreement
(two verifiers, same field name, different values) is last-writer-wins
on the merge, which matches what the user already typoed into config.
…he record

The cache record had five `lockfile*` prefixed fields (path, hash, size,
mtimeNs, inode) side by side. Group them under a single `lockfile`
object so the prefix only shows up once and the record reads as
"identity + verifiedAt + policy".

On-disk shape:
  {
    "lockfile": { "path": "...", "hash": "...", "size": 154, "mtimeNs": "...", "inode": 12345 },
    "verifiedAt": "...",
    "policy": { "minimumReleaseAge": 1440 }
  }
@zkochan zkochan changed the title perf(installing.deps-installer): cache lockfile minimumReleaseAge verification perf: cache the post-resolution lockfile verification gate May 17, 2026
@zkochan zkochan marked this pull request as ready for review May 17, 2026 00:11
Copilot AI review requested due to automatic review settings May 17, 2026 00:11
…e one

The two changesets covered the same package set and the same overall
story (the ResolutionVerifier abstraction plus its on-disk cache).
Merge them into a single entry and drop the duplicate.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@coderabbitai coderabbitai Bot added area: cli/dlx area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies. labels May 17, 2026

@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)
installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts (1)

323-328: 💤 Low value

Consider adding randomness to temp file name for concurrent safety.

The temp file path uses only process.pid, which could collide if multiple CI jobs share a network-mounted cache and happen to have the same PID. While rare, adding a random suffix or timestamp would eliminate the risk entirely.

♻️ Suggested fix
-    const tmpPath = `${cacheFilePath}.${process.pid}.tmp`
+    const tmpPath = `${cacheFilePath}.${process.pid}.${Date.now()}.tmp`
🤖 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 `@installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts`
around lines 323 - 328, The temporary file name for atomic writes currently uses
only process.pid (tmpPath) which can collide across different CI jobs; update
the tmpPath construction in verifyLockfileResolutionsCache.ts to append a random
or unique suffix (e.g., timestamp + random number or crypto.randomUUID()) in
addition to process.pid before calling fs.promises.writeFile and
fs.promises.rename, so each concurrent process writes to a distinct tempfile.
🤖 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 `@installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts`:
- Around line 323-328: The temporary file name for atomic writes currently uses
only process.pid (tmpPath) which can collide across different CI jobs; update
the tmpPath construction in verifyLockfileResolutionsCache.ts to append a random
or unique suffix (e.g., timestamp + random number or crypto.randomUUID()) in
addition to process.pid before calling fs.promises.writeFile and
fs.promises.rename, so each concurrent process writes to a distinct tempfile.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 392ad5a3-4649-4792-a798-80c5b9bbc364

📥 Commits

Reviewing files that changed from the base of the PR and between 31538bf and b1276cb.

📒 Files selected for processing (20)
  • .changeset/cache-aware-minimum-release-age-gate.md
  • installing/client/src/index.ts
  • installing/commands/src/fetch.ts
  • installing/commands/src/import/index.ts
  • installing/commands/src/installDeps.ts
  • installing/commands/src/recursive.ts
  • installing/commands/src/remove.ts
  • installing/deps-installer/src/install/extendInstallOptions.ts
  • installing/deps-installer/src/install/index.ts
  • installing/deps-installer/src/install/verifyLockfileResolutions.ts
  • installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts
  • installing/deps-installer/test/install/verifyLockfileResolutions.ts
  • installing/deps-installer/test/install/verifyLockfileResolutionsCache.ts
  • installing/deps-installer/test/utils/testDefaults.ts
  • resolving/default-resolver/src/index.ts
  • resolving/npm-resolver/src/createNpmResolutionVerifier.ts
  • resolving/resolver-base/src/index.ts
  • store/connection-manager/src/createNewStoreController.ts
  • store/connection-manager/src/index.ts
  • testing/temp-store/src/index.ts
📜 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). (1)
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.

Files:

  • installing/commands/src/installDeps.ts
  • installing/commands/src/import/index.ts
  • store/connection-manager/src/index.ts
  • resolving/resolver-base/src/index.ts
  • installing/commands/src/remove.ts
  • installing/deps-installer/test/install/verifyLockfileResolutionsCache.ts
  • installing/commands/src/fetch.ts
  • installing/deps-installer/test/utils/testDefaults.ts
  • resolving/npm-resolver/src/createNpmResolutionVerifier.ts
  • installing/deps-installer/src/install/extendInstallOptions.ts
  • installing/client/src/index.ts
  • testing/temp-store/src/index.ts
  • installing/deps-installer/src/install/index.ts
  • store/connection-manager/src/createNewStoreController.ts
  • installing/deps-installer/test/install/verifyLockfileResolutions.ts
  • installing/commands/src/recursive.ts
  • installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts
  • resolving/default-resolver/src/index.ts
  • installing/deps-installer/src/install/verifyLockfileResolutions.ts
🧠 Learnings (1)
📚 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/commands/src/installDeps.ts
  • installing/commands/src/import/index.ts
  • store/connection-manager/src/index.ts
  • resolving/resolver-base/src/index.ts
  • installing/commands/src/remove.ts
  • installing/deps-installer/test/install/verifyLockfileResolutionsCache.ts
  • installing/commands/src/fetch.ts
  • installing/deps-installer/test/utils/testDefaults.ts
  • resolving/npm-resolver/src/createNpmResolutionVerifier.ts
  • installing/deps-installer/src/install/extendInstallOptions.ts
  • installing/client/src/index.ts
  • testing/temp-store/src/index.ts
  • installing/deps-installer/src/install/index.ts
  • store/connection-manager/src/createNewStoreController.ts
  • installing/deps-installer/test/install/verifyLockfileResolutions.ts
  • installing/commands/src/recursive.ts
  • installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts
  • resolving/default-resolver/src/index.ts
  • installing/deps-installer/src/install/verifyLockfileResolutions.ts
🔇 Additional comments (30)
.changeset/cache-aware-minimum-release-age-gate.md (1)

12-14: LGTM!

installing/deps-installer/test/utils/testDefaults.ts (1)

31-31: LGTM!

Also applies to: 42-42, 60-60, 69-69

installing/client/src/index.ts (1)

12-12: LGTM!

Also applies to: 48-54, 66-66

store/connection-manager/src/createNewStoreController.ts (1)

66-66: LGTM!

Also applies to: 75-75, 148-148

store/connection-manager/src/index.ts (1)

20-20: LGTM!

testing/temp-store/src/index.ts (1)

16-16: LGTM!

Also applies to: 29-29, 63-63

installing/commands/src/fetch.ts (1)

73-73: LGTM!

installing/commands/src/import/index.ts (1)

189-189: LGTM!

installing/commands/src/installDeps.ts (1)

282-282: LGTM!

installing/commands/src/recursive.ts (1)

118-118: LGTM!

Also applies to: 170-170, 302-302, 421-421

installing/commands/src/remove.ts (1)

191-191: LGTM!

installing/deps-installer/src/install/extendInstallOptions.ts (1)

178-195: LGTM!

Also applies to: 313-313

installing/deps-installer/src/install/index.ts (1)

347-359: LGTM!

installing/deps-installer/test/install/verifyLockfileResolutions.ts (4)

1-53: LGTM!


59-151: LGTM!


153-171: LGTM!


173-243: LGTM!

resolving/resolver-base/src/index.ts (1)

96-130: LGTM!

resolving/npm-resolver/src/createNpmResolutionVerifier.ts (1)

111-176: LGTM!

installing/deps-installer/src/install/verifyLockfileResolutionsCache.ts (5)

1-47: LGTM!


48-136: LGTM!


138-214: LGTM!


216-274: LGTM!


276-339: LGTM!

resolving/default-resolver/src/index.ts (1)

159-196: LGTM!

installing/deps-installer/src/install/verifyLockfileResolutions.ts (4)

1-34: LGTM!


36-81: LGTM!


83-128: LGTM!


130-156: LGTM!

installing/deps-installer/test/install/verifyLockfileResolutionsCache.ts (1)

1-310: LGTM!

zkochan added 2 commits May 17, 2026 02:28
The cache is consulted once before verification fan-out and recorded
once after — there's no concurrent install work to overlap with, so
the async ceremony was paying for nothing. Switching `tryLockfileVerificationCache`
and `recordVerification` to sync fs (\`readFileSync\`, \`statSync\`,
\`appendFileSync\`, etc.) keeps the call sites straight-line and drops
two \`await\`s in \`verifyLockfileResolutions\`.

The hash helper is now an inline \`fs.readFileSync\` + \`createHexHash\`
(matching what \`createHexHashFromFile\` does asynchronously) rather
than pulling in the async helper.
Copilot AI review requested due to automatic review settings May 17, 2026 00:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

zkochan added 2 commits May 17, 2026 02:33
1. Include the resolution in the candidate-dedup key. With the previous
   `\${name}@\${version}` key, two entries that shared a name+version
   but were pinned via different protocols (e.g. npm registry vs. a git
   URL under the same alias) collapsed in the map. If the wrong shape
   won the dedup, a protocol-scoped verifier like the npm one would
   short-circuit (\`isNpmRegistryResolution\` returns false on the git
   shape) and the real npm entry would skip the gate. Regression test
   added.

2. Stringify the lockfile inode instead of \`Number(stat.ino)\`. On
   filesystems with inodes > Number.MAX_SAFE_INTEGER (some large
   network drives) the cast silently loses precision and the same-machine
   fast path would always miss; stringifying matches what we already
   do for \`mtimeNs\` and keeps full precision through the JSON round
   trip.
Git worktrees produce N absolute paths for the same lockfile content,
and a path-keyed cache treats each as separate — every worktree pays
the full per-package registry cost on its first install even though
the content has already been verified elsewhere.

Switch the primary index to the content hash. The path becomes a
secondary index for the same-machine stat shortcut: when we've seen
this exact path with these exact stat values, we trust the cached
hash and skip the file read. When the stat shortcut misses (a fresh
worktree, a CI checkout that reset mtime/inode), we hash and look up
by content.

Lookup order in tryLockfileVerificationCache:

  1. stat(); look up byPath. If the path+stat record still matches,
     hit (microseconds, no file read).
  2. Hash the lockfile; look up byHash. If the content has been
     verified before — at any path — hit. Append a refreshed record
     so the next install at this path takes step 1.

Cache compaction now dedupes by (path, hash) tuple so each worktree
keeps its own stat-shortcut entry without the byHash index losing
older content fingerprints.

Regression test added: a second \`lockfilePath\` under a different
directory but with identical bytes hits the cache via hash.
Copilot AI review requested due to automatic review settings May 17, 2026 00:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

\`tryLockfileVerificationCache\` already does the stat (always) and the
hash (when it falls through to the content lookup). On a miss the
caller would then call \`recordVerification\`, which redid both —
hashing a multi-MB lockfile twice on every gate run.

Return the precomputed stat+hash from the lookup and forward them to
\`recordVerification\` so the second pass is free. The stat-shortcut
branch can even forward the cached record's hash unchanged: stat-match
implies content-match, so the hash hasn't moved.
…ile again

The cache module was reading and hashing the lockfile from disk even
though \`verifyLockfileResolutions\` already received the parsed
lockfile object. Replace the file-bytes hash with \`hashObject\` from
\`@pnpm/crypto.object-hasher\` (streaming, key-order-stable) applied
to the in-memory object.

The cache accepts a \`hashLockfile: () => string\` thunk so the cost
is only paid when the stat shortcut can't decide — same as before,
just sourced from RAM instead of disk. Lazy evaluation means a
stat-only hit pays nothing.

Result: \`verifyLockfileResolutions\` does at most one \`stat()\` and
at most one object-hash per install. No file reads in the cache layer
itself.
Copilot AI review requested due to automatic review settings May 17, 2026 00:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

zkochan added 2 commits May 17, 2026 03:13
Asserts the CLI plumbing — that \`cacheDir\` flows from the user's
config through CreateStoreControllerOptions, that the cache file
lands at \`<cacheDir>/lockfile-verified.jsonl\` with the documented
record shape, and that a second install with the same lockfile +
policy succeeds. Doesn't try to assert the gate was skipped — that
would need registry-call instrumentation we don't have at this
layer, but install completing cleanly is the smoke test.

The unit + integration tests already cover the cache logic; this
adds a thin CLI-level layer to catch plumbing regressions (e.g. if
\`cacheDir\` stopped flowing through the store-controller chain).
Copilot AI review requested due to automatic review settings May 17, 2026 09:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

zkochan added 2 commits May 17, 2026 12:12
The first install in the test was against a fresh project — no
existing pnpm-lock.yaml meant the post-resolution gate had nothing
to verify (lockfile.packages was empty), so it exited early without
writing a cache record and the existence assertion failed.

Mirror the existing tests in this file: install once with no policy
to populate the lockfile, then enable the policy and run install
again so the gate actually fires against the existing entries.
\`cache: opts.cacheDir ? {...} : undefined\` read like a feature toggle
even though the verification itself runs regardless of cacheDir — only
the memoization is conditional, and cacheDir is always set after config
normalization anyway.

Hoist \`cacheDir\` and \`lockfilePath\` onto the options object directly.
The call site is now a flat property pass-through:

  await verifyLockfileResolutions(ctx.wantedLockfile, opts.resolutionVerifiers, {
    cacheDir: opts.cacheDir,
    lockfilePath: path.resolve(ctx.lockfileDir, WANTED_LOCKFILE),
  })

The function itself decides whether to cache (both fields present →
cache; either missing → run the gate without memoization), which keeps
unit tests that omit the cache wiring on the same code path.
Copilot AI review requested due to automatic review settings May 17, 2026 10:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@zkochan zkochan merged commit fcf95c7 into main May 17, 2026
12 checks passed
@zkochan zkochan deleted the 11687-2 branch May 17, 2026 11:07
zkochan added a commit that referenced this pull request May 17, 2026
…metadata in the minimumReleaseAge gate (#11704)

Follow-up to #11691 — item 2 from #11687, plus a related shortcut.

## What

When the `minimumReleaseAge` lockfile verification gate needs to know when a version was published, it used to fetch a multi-MB full metadata document per package just to read one timestamp. This PR replaces that single-step path with a four-layer lookup that pays the cheapest viable source first:

1. **Abbreviated metadata's `modified` field** — the resolver already fetches this for resolution. If the package as a whole hasn't been modified within the policy cutoff, every version it contains is at least that old; return `modified` as a conservative upper-bound and skip the rest of the chain.
2. **Local `FULL_META_DIR` mirror** — exact per-version times if a previous verification populated it.
3. **npm attestation endpoint** (`/-/npm/v1/attestations/<name>@<version>`) — a tens-of-KB Sigstore bundle whose Rekor inclusion time stands in for publish time. Wins on cold cache when the package was published with provenance.
4. **Full metadata fetch** — last resort.

## Why

The verification cache from #11691 made repeat installs against an unchanged lockfile effectively free. The remaining cost is the *first* verification on a fresh CI runner with no restored cache — particularly `pnpm install --frozen-lockfile`, where every locked package's publish timestamp has to be confirmed. Fetching the full metadata document for each package is wasteful when:

- The resolver has usually already cached abbreviated metadata, whose `modified` field alone is enough to clear stable packages (the common case).
- For recently-modified packages, the per-version attestation endpoint is orders of magnitude smaller than full metadata.

## How

### Abbreviated `modified` shortcut

`fetchFullMetadataCached` is refactored to share an internal helper with a new `fetchAbbreviatedMetadataCached`. Both do conditional GETs against their respective on-disk mirrors. On a non-frozen install the abbreviated mirror is already populated by the resolver, so the shortcut hits the local cache at headers-only cost. On `--frozen-lockfile` the fetch is still cheaper than full metadata.

If `Date.parse(modified) < cutoff`, return `modified` — it's an upper bound on every version's publish time in this package, and the verifier's `published < cutoff` check passes trivially.

### Attestation endpoint

`fetchAttestationPublishedAt` (new module) hits `/-/npm/v1/attestations/<name>@<version>`, parses the response, and reads the earliest `bundle.verificationMaterial.tlogEntries[].integratedTime` across the attestation bundles. That's the Rekor inclusion time — a couple of seconds after publish, well within tolerance for a policy that operates in minutes/hours/days. Returns `undefined` on 404 / network error / malformed body so the caller falls back.

### Per-install dedup

The lookup carries a `PublishedAtLookupContext` with four memo maps: abbreviated meta per (registry, name), local full meta per (registry, name), full meta network fetch per (registry, name), final published-at per (registry, name, version). Verifying many versions of the same package only pays the disk/network costs once.

## Trust model

**No Sigstore signature verification on the attestation path.** The trust model is identical to reading the registry's `time` field on the full metadata document — we already trust the registry to serve correct publish timestamps for the gate's purpose. The win is purely bandwidth.

Full Sigstore verification (Fulcio cert chain + Rekor inclusion proof) would harden the timestamp against a compromised registry. It pulls in the `sigstore` npm package and the TUF root — a separate dependency-surface discussion, parked as future work.

## Tests

- **13 unit tests** in `resolving/npm-resolver/test/fetchAttestationPublishedAt.test.ts`: ISO timestamp extraction, URL construction (scoped + unscoped), 404 / 5xx / network error / malformed JSON / missing fields → undefined, earliest-of-multiple-attestations, defensive number-as-integratedTime, auth header forwarding, trailing-slash normalization.
- Existing `minimumReleaseAge` + `verifyLockfileResolutions` integration suites (45 tests) still pass — the fallback chain preserves end-to-end behavior when the new shortcuts don't apply.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli/dlx area: lockfile area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(minimumReleaseAge): cache lockfile verification to avoid re-checking on every install

2 participants