perf: record locally-resolved lockfile in verification cache by zkochan · Pull Request #11714 · pnpm/pnpm

zkochan · 2026-05-18T08:59:51Z

Summary

The lockfile verification cache currently only records the lockfile that exists at the start of an install. So a flow like:

pnpm install <pkg>
rm -rf node_modules
pnpm install

re-runs the per-package registry round-trip against the newly written lockfile, even though the local resolver already enforced the policy when picking those versions. The fresh lockfile is now recorded immediately after each install-time write, so the second install takes the cache fast path.

Implementation

Recording the post-resolution lockfile

New helper recordLockfileVerified (in installing/deps-installer/src/install/). Gated on cacheDir + non-empty resolutionVerifiers — same gate the pre-resolution verifier uses.
Two thin combiners over the lockfile writers: writeWantedLockfileAndRecordVerified and writeLockfilesAndRecordVerified. The install paths use these so the record always runs alongside the write.

Hash stability: writer returns the canonical lockfile

The cache stores hashObject(LockfileObject) and the next install computes the same hash off the file it loads from disk. For the hashes to match, both ends must compute over structurally identical objects. They don't, naïvely: the in-memory write object can carry undefined optional fields (e.g. settings.dedupePeers = undefined from opts.dedupePeers || undefined in install code) that YAML drops on serialize — object-hash treats undefined vs missing as distinct values.

writeWantedLockfile / writeLockfiles (in @pnpm/lockfile.fs) now return the canonical post-write LockfileObject: convertToLockfileObject(stripUndefinedDeep(lockfileFile)). The strip walks the existing object graph in memory rather than going through a yaml.load round-trip, so non-cache callers (deploy, deps-restorer, make-dedicated-lockfile, agent server) pay near-zero cost.
Install hooks hash the writer's returned value, not the raw in-memory input. Guaranteed by construction to match what the next reader produces.

`useGitBranchLockfile` correctness

The pre-resolution verification gate and the new post-write recorder were both keying cache records on a hard-coded pnpm-lock.yaml. Under useGitBranchLockfile the actual file is pnpm-lock.<branch>.yaml, so the stat shortcut hit ENOENT and the cache effectively never engaged for git-branch users. Both sites now resolve the real filename via getWantedLockfileName. The wrappers compute it once and pass it to the writer via a new optional lockfileName opt so useGitBranchLockfile installs don't fork getCurrentBranch twice per write.

Bug fix unrelated to the cache, found during review

writeLockfiles' differs branch was deciding whether to remove or keep node_modules/.pnpm/lock.yaml based on isEmptyLockfile(wantedLockfile). Filtered-current callers (deps-restorer) pass an empty current against a non-empty wanted, so this could leave a stale current lockfile on disk. Fixed to key off the current.

Comments policy

AGENTS.md (and pacquet/AGENTS.md) now spell out the comment defaults: write self-documenting code, do not restate at call sites what the callee's JSDoc / doc comment already says, comments are reserved for the non-obvious why. The pruning pass in this PR brings the changed code in line.

API surface

@pnpm/lockfile.fs (minor):
- writeWantedLockfile: return widened from Promise<void> to Promise<LockfileObject>. New optional lockfileName opt.
- writeCurrentLockfile: return widened to Promise<LockfileObject | undefined> (undefined when the empty-lockfile branch unlinks).
- writeLockfiles: return widened from Promise<void> to Promise<{ wantedLockfile, currentLockfile }>. New optional wantedLockfileName opt. New exported WriteLockfilesResult type.
- New export: getWantedLockfileName.
@pnpm/installing.deps-installer (patch): internal-only wrappers; no external API change.

Test plan

Unit tests in installing/deps-installer/test/install/recordLockfileVerified.ts cover the gate (no cache dir / no verifiers / no packages / undefined verifiers), plus stat-shortcut and hash-fallback cache hits. Fixtures carry explicit settings.dedupePeers: undefined so the in-memory-vs-loaded shape divergence is actually exercised.
Round-trip tests in lockfile/fs/test/write.test.ts assert writeWantedLockfile() / writeLockfiles() returns equal what readWantedLockfile() / readCurrentLockfile() produces, including for input with an explicit undefined optional field.
e2e test in pnpm/test/install/minimumReleaseAge.ts reproduces the user-described scenario: pnpm add populates the cache; a follow-up --offline --frozen-lockfile install completes cleanly under the same policy.
Verified manually against the bundled CLI: round 1 pnpm add records the new lockfile; round 2 rm -rf node_modules && pnpm install hits the stat shortcut (no new record, ~60ms); round 3 with inode-invalidated lockfile hits the hash fallback (record refreshed with same hash, ~60ms).

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

coderabbitai · 2026-05-18T09:00:12Z

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

Installer writes now return canonical, round-tripped lockfile objects. A new recordLockfileVerified records those post-write lockfiles (using computed wanted lockfile names) into the verification cache. Install flows call this after writes (including pnpm-agent), and tests validate cache recording and reuse.

Changes

Lockfile Verification Cache Recording

Layer / File(s)	Summary
Lockfile writers return canonical objects `lockfile/fs/src/write.ts`, `lockfile/fs/test/write.test.ts`	`writeWantedLockfile`, `writeCurrentLockfile`, and `writeLockfiles` now return normalized `LockfileObject` results computed via YAML round-trip. Tests validate returned objects match on-disk read-back, establishing "as-saved" serialization contract for caching.
recordLockfileVerified implementation `installing/deps-installer/src/install/recordLockfileVerified.ts`	New `RecordLockfileVerifiedOptions` interface and `recordLockfileVerified()` function take post-write lockfile and resolver verifiers, gate on `cacheDir`/active-verifiers/packages, and record verification using a `hashLockfile` callback.
verifyLockfileResolutions hash documentation `installing/deps-installer/src/install/verifyLockfileResolutions.ts`	Clarifies inline comments explaining why lockfile hash is memoized and safe to compute from canonical in-memory object.
Integration into install flows `installing/deps-installer/src/install/index.ts`	Resolves actual wanted lockfile path via `getWantedLockfileName` to align verification and cache keying in git-branch modes; calls `recordLockfileVerified` after `writeLockfiles`, `writeWantedLockfile`, and in pnpm-agent flows, passing post-write lockfile objects and resolver verifiers.
Unit tests for recordLockfileVerified `installing/deps-installer/test/install/recordLockfileVerified.ts`	Exercises no-op conditions, validates canonical hash recording and path preservation in cache entries, tests git-branch-suffixed paths, and confirms cache lookup via stat shortcut and hash fallback.
minimumReleaseAge cache integration test `pnpm/test/install/minimumReleaseAge.ts`	Fresh install with `minimumReleaseAge` policy populates `lockfile-verified.jsonl`; subsequent `--offline --frozen-lockfile` install succeeds using cached verification.
lockfile/fs module exports `lockfile/fs/src/index.ts`	Re-exports `getWantedLockfileName` from entrypoint for use in install path resolution.
Deploy command adjustments `releasing/commands/src/deploy/deploy.ts`	Passes `allowBuilds` option to `createDeployFiles`; widens `filesToWrite` type to accommodate updated async write return types.
Changeset documentation `.changeset/record-locally-resolved-lockfile-verified.md`	Documents patch bumps and that verification cache now records post-resolution lockfiles after install-time writes.

Sequence Diagram(s)

sequenceDiagram
  participant Install
  participant verifyLockfileResolutions
  participant writeLockfiles
  participant recordLockfileVerified
  participant VerificationCache
  Install->>verifyLockfileResolutions: verify policies for wanted lockfilePath
  Install->>writeLockfiles: write resolved lockfiles
  writeLockfiles-->>Install: WriteLockfilesResult (wantedLockfile)
  Install->>recordLockfileVerified: record wantedLockfile + verifiers + cacheDir
  recordLockfileVerified->>VerificationCache: append lockfile-verified.jsonl record

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

perf(minimumReleaseAge): cache lockfile verification to avoid re-checking on every install #11687: Matches the per-lockfile verification cache work (post-write recording and cache-use to skip re-verification).
Partial install with --frozen-lockfile: read+write node_modules/.pnpm/lock.yaml and skip unchanged snapshots pacquet#433: Touches lockfile read/write and round-trip behavior related to current/wanted lockfile writing and deletions.

Suggested reviewers

Copilot

Poem

🐰 I wrote a hash and set it free,

The lockfile rested, safe to be.
One write, one record, then hop away—
Next install skips work today.
🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.67% 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: record locally-resolved lockfile in verification cache' directly and accurately describes the main change: recording the post-resolution lockfile into the verification cache, which is the core optimization in the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
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 lockfile-verif

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

coderabbitai

🧹 Nitpick comments (1)

pnpm/test/install/minimumReleaseAge.ts (1)
123-123: ⚡ Quick win

Remove unnecessary async keyword for consistency.

The test function is declared async but doesn't use await anywhere—it only calls execPnpmSync(), which is synchronous. Other tests in this file follow a consistent pattern: tests using await execPnpm() are marked async (lines 29, 80, 349), while tests using only execPnpmSync() are not (lines 60, 165, 191, etc.).
♻️ Proposed fix
-  test('a fresh install records the just-written lockfile in the verification cache', async () => {
+  test('a fresh install records the just-written lockfile in the verification cache', () => {
🤖 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 `@pnpm/test/install/minimumReleaseAge.ts` at line 123, The test callback for
test('a fresh install records the just-written lockfile in the verification
cache') is marked async but never uses await (it only calls execPnpmSync()), so
remove the unnecessary async keyword from the test function to match the file's
convention (tests that call execPnpmSync() are not async). Locate the test by
its title and drop the async from the callback signature so the test becomes a
normal synchronous function.

🤖 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 `@pnpm/test/install/minimumReleaseAge.ts`:
- Line 123: The test callback for test('a fresh install records the just-written
lockfile in the verification cache') is marked async but never uses await (it
only calls execPnpmSync()), so remove the unnecessary async keyword from the
test function to match the file's convention (tests that call execPnpmSync() are
not async). Locate the test by its title and drop the async from the callback
signature so the test becomes a normal synchronous function.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7402644a-d33b-4109-a5d8-355048266af0

📥 Commits

Reviewing files that changed from the base of the PR and between e74a6b7 and 98bacaa.

📒 Files selected for processing (5)

.changeset/record-locally-resolved-lockfile-verified.md
installing/deps-installer/src/install/index.ts
installing/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/test/install/recordLockfileVerified.ts
pnpm/test/install/minimumReleaseAge.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). (3)

GitHub Check: Agent
GitHub Check: Analyze (javascript)
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/deps-installer/src/install/recordLockfileVerified.ts
pnpm/test/install/minimumReleaseAge.ts
installing/deps-installer/test/install/recordLockfileVerified.ts
installing/deps-installer/src/install/index.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/deps-installer/src/install/recordLockfileVerified.ts
pnpm/test/install/minimumReleaseAge.ts
installing/deps-installer/test/install/recordLockfileVerified.ts
installing/deps-installer/src/install/index.ts

🔇 Additional comments (4)

.changeset/record-locally-resolved-lockfile-verified.md (1)

1-7: LGTM!

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

1-52: LGTM!

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

97-97: LGTM!

Also applies to: 1667-1677, 1731-1736, 2285-2293

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

1-177: LGTM!

Copilot

Pull request overview

Improves install performance under lockfile resolution verification policies (e.g. minimumReleaseAge) by recording the post-resolution lockfile into the verification cache immediately after lockfile writes, so subsequent installs can take the cache fast path instead of re-verifying via registry round-trips.

Changes:

Add recordLockfileVerified() helper to persist verification-cache entries based on the just-written lockfile content.
Invoke the helper after install-time lockfile write points (normal path and agent path).
Add unit + e2e coverage to validate recording behavior and the offline fast-path scenario.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`pnpm/test/install/minimumReleaseAge.ts`	Adds an e2e regression test for the “add → delete node_modules → install” flow using the verification cache.
`installing/deps-installer/test/install/recordLockfileVerified.ts`	Adds unit tests covering gating behavior, round-trip hash stability, and cache hit paths.
`installing/deps-installer/src/install/recordLockfileVerified.ts`	Introduces helper that reloads the written lockfile from disk and records it in the verification cache.
`installing/deps-installer/src/install/index.ts`	Calls `recordLockfileVerified()` after lockfile write sites, including the agent-resolve path.
`.changeset/record-locally-resolved-lockfile-verified.md`	Documents the behavior change and motivation in a changeset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

🧹 Nitpick comments (1)

installing/deps-installer/test/install/recordLockfileVerified.ts (1)
136-136: 💤 Low value

Consider adding an explicit null check for defensive clarity.

While the non-null assertion is safe here (the lockfile was just written on line 128), adding an explicit expect(loaded).not.toBeNull() assertion would make the assumption explicit and provide a clearer failure message if the read unexpectedly fails.
🛡️ Suggested defensive assertion
   const lockfilePath = path.resolve(lockfileDir, WANTED_LOCKFILE)
-  const loaded = (await readWantedLockfile(lockfileDir, { ignoreIncompatible: false }))!
+  const loaded = await readWantedLockfile(lockfileDir, { ignoreIncompatible: false })
+  expect(loaded).not.toBeNull()
🤖 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/test/install/recordLockfileVerified.ts` at line
136, Add an explicit defensive assertion after calling readWantedLockfile to
make the assumption explicit: check that the returned value (loaded) is not null
before using it. Locate the call to readWantedLockfile(lockfileDir, {
ignoreIncompatible: false }) and add an assertion like
expect(loaded).not.toBeNull() (or equivalent) referencing the loaded variable so
a clear failure message appears if the read unexpectedly returns null; you can
then safely proceed to use loaded without the non-null assertion.

🤖 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/test/install/recordLockfileVerified.ts`:
- Line 136: Add an explicit defensive assertion after calling readWantedLockfile
to make the assumption explicit: check that the returned value (loaded) is not
null before using it. Locate the call to readWantedLockfile(lockfileDir, {
ignoreIncompatible: false }) and add an assertion like
expect(loaded).not.toBeNull() (or equivalent) referencing the loaded variable so
a clear failure message appears if the read unexpectedly returns null; you can
then safely proceed to use loaded without the non-null assertion.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fb9eeb39-3e28-4325-9f51-99847e5ecac5

📥 Commits

Reviewing files that changed from the base of the PR and between 98bacaa and 35b39f9.

📒 Files selected for processing (5)

installing/deps-installer/src/install/index.ts
installing/deps-installer/src/install/lockfileHash.ts
installing/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/src/install/verifyLockfileResolutions.ts
installing/deps-installer/test/install/recordLockfileVerified.ts

🚧 Files skipped from review as they are similar to previous changes (1)

installing/deps-installer/src/install/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/deps-installer/src/install/lockfileHash.ts
installing/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/src/install/verifyLockfileResolutions.ts
installing/deps-installer/test/install/recordLockfileVerified.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/deps-installer/src/install/lockfileHash.ts
installing/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/src/install/verifyLockfileResolutions.ts
installing/deps-installer/test/install/recordLockfileVerified.ts

🔇 Additional comments (7)

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

1-35: LGTM!

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

10-46: LGTM!

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

12-12: LGTM!

Also applies to: 99-105, 111-111, 125-125

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

1-27: LGTM!

29-58: LGTM!

60-99: LGTM!

101-163: LGTM!

Both the pre-resolution gate and the new post-write recorder were keying the cache on a hard-coded pnpm-lock.yaml. Under useGitBranchLockfile that's the wrong file — the actual lockfile is pnpm-lock.<branch>.yaml, so statLockfile() returned ENOENT and the cache lookup short-circuited to a miss every time. Resolve the real filename via getWantedLockfileName before recording and looking up. Also lifts the helper API from `lockfileDir` to `lockfilePath` so callers can hand in any branch-suffixed path without the helper having to know about git branches. Per Copilot review comment on #11714.

coderabbitai

🧹 Nitpick comments (1)

installing/deps-installer/test/install/recordLockfileVerified.ts (1)
31-60: ⚡ Quick win

Move helper function declarations below their first use to match repo hoisting style.

mraVerifier and makeLockfile are declared before the tests that use them. This file should follow the repo convention of declaring functions after usage.

As per coding guidelines: “functions are declared after they are used (hoisting is relied upon).”
🤖 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/test/install/recordLockfileVerified.ts` around
lines 31 - 60, The helper functions mraVerifier and makeLockfile are defined
before the tests that use them; move their declarations so they appear after
their first usage to match the repo's hoisting convention—locate the references
to mraVerifier and makeLockfile in the test cases and cut/paste each function
definition to a position below those tests (preserve exact function names and
signatures: mraVerifier(current: number): ResolutionVerifier and makeLockfile():
LockfileObject) so behavior remains unchanged.

🤖 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/test/install/recordLockfileVerified.ts`:
- Around line 31-60: The helper functions mraVerifier and makeLockfile are
defined before the tests that use them; move their declarations so they appear
after their first usage to match the repo's hoisting convention—locate the
references to mraVerifier and makeLockfile in the test cases and cut/paste each
function definition to a position below those tests (preserve exact function
names and signatures: mraVerifier(current: number): ResolutionVerifier and
makeLockfile(): LockfileObject) so behavior remains unchanged.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eb219d40-820c-4e46-8fec-52e26639ff75

📥 Commits

Reviewing files that changed from the base of the PR and between 35b39f9 and a4089b2.

📒 Files selected for processing (5)

.changeset/record-locally-resolved-lockfile-verified.md
installing/deps-installer/src/install/index.ts
installing/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/test/install/recordLockfileVerified.ts
lockfile/fs/src/index.ts

✅ Files skipped from review due to trivial changes (2)

lockfile/fs/src/index.ts
.changeset/record-locally-resolved-lockfile-verified.md

🚧 Files skipped from review as they are similar to previous changes (1)

installing/deps-installer/src/install/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). (2)

GitHub Check: Agent
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/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/test/install/recordLockfileVerified.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/deps-installer/src/install/recordLockfileVerified.ts
installing/deps-installer/test/install/recordLockfileVerified.ts

🔇 Additional comments (2)

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

1-50: LGTM!

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

1-30: LGTM!

Also applies to: 62-187

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Previously the cache only captured the lockfile that existed when the install started. So a flow like `pnpm install <pkg>` followed by `rm -rf node_modules && pnpm install` re-ran the per-package registry round-trip against the newly written lockfile even though the local resolver had already enforced the policy when picking those versions. After each install-time lockfile write, re-read the file and append a verification record. Re-reading is necessary: the in-memory write object differs from the parsed object on undefined-vs-empty optional fields, and `object-hash` would produce mismatched hashes otherwise.

The async execPnpm does not accept expectSuccess; switching to the sync helper aligns with the rest of this test file and clears the pre-push typecheck gate.

Replaces the post-write `readWantedLockfile` with `hashLockfile`, a load-stable canonicalization built from the existing converters plus a small undefined-stripping pass. Same correctness guarantee, no I/O. The cache-lookup side in `verifyLockfileResolutions` switches to the same `hashLockfile`. It's idempotent on already-loaded lockfile objects, so existing cache records remain valid — the previous formula was `hashObject(loadedLockfile)` and that equals `hashLockfile(loadedLockfile)` on any input that came off disk. Found one normalization gap during smoke testing: install-time lockfiles can carry `settings.dedupePeers = undefined` (and similar leftover fields). The converters keep these; the YAML round-trip drops them. `stripUndefined` closes the gap so the in-memory hash matches the post-load hash exactly.

Both the pre-resolution gate and the new post-write recorder were keying the cache on a hard-coded pnpm-lock.yaml. Under useGitBranchLockfile that's the wrong file — the actual lockfile is pnpm-lock.<branch>.yaml, so statLockfile() returned ENOENT and the cache lookup short-circuited to a miss every time. Resolve the real filename via getWantedLockfileName before recording and looking up. Also lifts the helper API from `lockfileDir` to `lockfilePath` so callers can hand in any branch-suffixed path without the helper having to know about git branches. Per Copilot review comment on #11714.

writeWantedLockfile and writeLockfiles now return the LockfileObject they just persisted — built from a yaml.load of the same YAML doc they wrote, mirroring what readWantedLockfile will produce on the next install. The install hook hashes that return value instead of the raw in-memory object. This removes the lockfileHash.ts / stripUndefined dance: both ends of the cache contract now produce a structurally identical object through the same converter chain (convertToLockfileObject of yaml.load), so plain hashObject is enough. As a side benefit, existing on-disk cache entries written by the released version of the gate (also plain hashObject) stay valid. The writer change is a minor bump for @pnpm/lockfile.fs — return types widen from Promise<void> to Promise<LockfileObject> (and the plural variant returns a struct). One previously-typed Array<Promise<void>> in deploy.ts widens to unknown to accommodate.

…back stat.ino is 0 on Windows for many filesystems, so zeroing the cached inode could let the record accidentally match a real file's stat and skip the hash-fallback branch the test wants to exercise. Overwrite size with -1 instead — no real file has that byte count. Per Copilot review comment on #11714.

…ne helper Two install paths (the non-headless else branch and the agent path) had the same shape: writeWantedLockfile followed by recordLockfileVerified with the same opts. Wrap them in writeWantedLockfileAndRecordVerified so the pattern is one call instead of three. Optional lockfilePath lets the caller pass a pre-resolved path when they already computed it (the in-resolution branch holds onto it for the gate too), avoiding a redundant getCurrentBranch shell-out.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

installing/deps-installer/test/install/recordLockfileVerified.ts:159

Same as above: this test records using the pre-write in-memory lockfile object, but production call sites pass the canonical object returned by writeWantedLockfile/writeLockfiles. Updating the test to use the returned lockfile would better validate the cache hit behavior under the canonicalization guarantee.

test('records a cache entry that the next install hits on both the stat shortcut and hash fallback paths', async () => {
  const lockfile = makeLockfile()
  await writeWantedLockfile(lockfileDir, lockfile)
  recordLockfileVerified({
    cacheDir,
    lockfilePath,
    lockfile,
    resolutionVerifiers: [mraVerifier(60)],
  })

write.ts now canonicalizes the return via a recursive stripUndefinedDeep on the LockfileFile instead of a full yaml.load round-trip. Same result — YAML drops undefined the same way — but the pass walks the existing object in place rather than re-parsing the serialized string, which is the right shape for the always-on return value: non-cache callers pay the cheaper cost. lockfile/fs writer test now includes an explicit \`settings.dedupePeers: undefined\` in the fixture so the round-trip-equality assertion actually exercises the YAML-drop divergence the test is documenting; a stricter follow-up check asserts the key is absent from the writer's return. deps-installer record-verified tests pass the writer's return value to recordLockfileVerified, mirroring the production call sites; fixtures also carry an explicit undefined optional field. Previously the tests passed the in-memory input, which would have silently regressed if the writer's canonicalization ever diverged from the reader's load form. Per Copilot review on #11714.

…writer Mirror of writeWantedLockfileAndRecordVerified for writeLockfiles (plural). The non-headless install path uses it, collapsing the prior writeLockfiles(...).then(record(...)) shape into a single call and dropping the direct recordLockfileVerified import from install/index.ts. The headless path at the writeLockfiles call earlier in the function stays bare — the pre-resolution gate already recorded that lockfile, so the wrapper would just append a duplicate cache line.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

releasing/commands/src/deploy/deploy.ts:255

filesToWrite is widened to Array<Promise<unknown>> only to accommodate promises that return values (e.g. writeWantedLockfile). This weakens type-safety for the rest of the array. Consider keeping Array<Promise<void>> and explicitly discarding non-void results (e.g. via .then(() => {})) so accidental value-bearing promises don’t get silently ignored.

  const filesToWrite: Array<Promise<unknown>> = [
    fs.promises.writeFile(
      path.join(deployDir, 'package.json'),
      JSON.stringify(deployFiles.manifest, undefined, 2) + '\n'
    ),
    writeWantedLockfile(deployDir, deployFiles.lockfile),
  ]

In writeLockfiles' differs branch, key the rm/keep decision and the returned currentLockfile on isEmptyLockfile of the *current* lockfile, not the wanted one — deps-restorer passes a filtered current with a non-empty wanted, and the old code would leave a stale current file on disk. Drop the redundant getWantedLockfileName calls. writeWantedLockfile and writeLockfiles now accept an optional precomputed name; the wrappers resolve it once and pass it to the writer so useGitBranchLockfile installs don't fork getCurrentBranch twice per write. Compute the gate's wantedLockfilePath lazily, only when both cacheDir and resolutionVerifiers are wired. The wrappers also short-circuit getWantedLockfileName when the cache record would no-op anyway. Collapse the duplicated canonical-form / no-op-conditions explanations across recordLockfileVerified, the wrappers, and verifyLockfileResolutions into one source of truth per concept, with brief pointers from the other sites. Also fix the stale YAML-round-trip / walks-in-place comments that survived the switch to stripUndefinedDeep. Per Copilot review pass on #11714.

Add a "Comments" subsection under Code Style in both AGENTS.md and pacquet/AGENTS.md. The rule: write self-documenting code, default to no comments, and never restate at call sites what the callee's JSDoc / doc comment already says. Comments are reserved for the non-obvious why. Prune the code in this PR to match. Most call-site comments above helpers (the agent path, the gate's lockfile-name resolution, the hash-canonicalization rationale, the writer's return-shape rationale, the differs-branch current-lockfile rationale) collapsed to one line or were removed entirely; the function-level docstrings carry the remaining contract.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings May 18, 2026 08:59

Copilot started reviewing on behalf of zkochan May 18, 2026 09:00 View session

coderabbitai Bot added area: lockfile area: supply chain security Issues related to minimumReleaseAge, blockExoticSubdeps, build script safety, and trust policies. labels May 18, 2026

coderabbitai Bot reviewed May 18, 2026

View reviewed changes

coderabbitai Bot approved these changes May 18, 2026

View reviewed changes

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread installing/deps-installer/src/install/recordLockfileVerified.ts Outdated

Comment thread installing/deps-installer/src/install/recordLockfileVerified.ts Outdated

coderabbitai Bot reviewed May 18, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 18, 2026 09:41

Copilot started reviewing on behalf of zkochan May 18, 2026 09:41 View session

coderabbitai Bot reviewed May 18, 2026

View reviewed changes

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread installing/deps-installer/test/install/recordLockfileVerified.ts Outdated

zkochan added 5 commits May 18, 2026 12:01

test: use execPnpmSync for the new cache-population scenario

969cef4

The async execPnpm does not accept expectSuccess; switching to the sync helper aligns with the rest of this test file and clears the pre-push typecheck gate.

zkochan force-pushed the lockfile-verif branch from a4089b2 to 35a4c7f Compare May 18, 2026 10:03

Copilot AI review requested due to automatic review settings May 18, 2026 10:06

Copilot started reviewing on behalf of zkochan May 18, 2026 10:07 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread lockfile/fs/src/write.ts Outdated

Comment thread installing/deps-installer/test/install/recordLockfileVerified.ts Outdated

Comment thread lockfile/fs/test/write.test.ts Outdated

Copilot AI review requested due to automatic review settings May 18, 2026 10:19

Copilot started reviewing on behalf of zkochan May 18, 2026 10:19 View session

zkochan added 2 commits May 18, 2026 12:21

refactor: export WriteLockfilesResult from @pnpm/lockfile.fs

bfbb554

Copilot AI reviewed May 18, 2026

View reviewed changes

zkochan added 2 commits May 18, 2026 14:09

Copilot AI review requested due to automatic review settings May 18, 2026 12:22

Copilot started reviewing on behalf of zkochan May 18, 2026 12:23 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread installing/deps-installer/src/install/recordLockfileVerified.ts

zkochan merged commit 2a9bd89 into main May 18, 2026
24 checks passed

zkochan deleted the lockfile-verif branch May 18, 2026 12:55

This was referenced May 18, 2026

feat(pacquet): port lockfile verification (minimumReleaseAge + trustPolicy) #11729

Merged

feat(installing): delegate fetch / import / link to pacquet when configured #11734

Merged

fix: cap lockfile verification memory and add trustLockfile opt-out #11878

Merged

This was referenced Jun 2, 2026

feat(pnpr): support --lockfile-only on the pnpr install path (resolve-only mode) #12148

Merged

refactor: replace the experimental pnpm-agent server with pnpr #12151

Merged

feat(install): add --dry-run option #12270

Open

Uh oh!

Conversation

zkochan commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

Recording the post-resolution lockfile

Hash stability: writer returns the canonical lockfile

useGitBranchLockfile correctness

Bug fix unrelated to the cache, found during review

Comments policy

API surface

Test plan

Uh oh!

coderabbitai Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 18, 2026 •

edited

Loading

`useGitBranchLockfile` correctness

coderabbitai Bot commented May 18, 2026 •

edited

Loading