From 2a9bd897bf181af66935c6bedc1cf722b4a3d806 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Mon, 18 May 2026 14:55:16 +0200 Subject: [PATCH] perf: record locally-resolved lockfile in verification cache (#11714) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lockfile verification cache currently only records the lockfile that exists at the **start** of an install. So a flow like: ``` pnpm install 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..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` to `Promise`. New optional `lockfileName` opt. - `writeCurrentLockfile`: return widened to `Promise` (undefined when the empty-lockfile branch unlinks). - `writeLockfiles`: return widened from `Promise` 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. --- ...cord-locally-resolved-lockfile-verified.md | 7 + AGENTS.md | 17 ++ .../deps-installer/src/install/index.ts | 33 ++- .../src/install/recordLockfileVerified.ts | 36 +++ .../src/install/verifyLockfileResolutions.ts | 3 +- .../writeLockfilesAndRecordVerified.ts | 48 ++++ .../writeWantedLockfileAndRecordVerified.ts | 42 ++++ .../test/install/recordLockfileVerified.ts | 214 ++++++++++++++++++ lockfile/fs/src/index.ts | 2 + lockfile/fs/src/write.ts | 79 ++++++- lockfile/fs/test/write.test.ts | 70 ++++++ pacquet/AGENTS.md | 12 + pnpm/test/install/minimumReleaseAge.ts | 42 ++++ releasing/commands/src/deploy/deploy.ts | 2 +- 14 files changed, 588 insertions(+), 19 deletions(-) create mode 100644 .changeset/record-locally-resolved-lockfile-verified.md create mode 100644 installing/deps-installer/src/install/recordLockfileVerified.ts create mode 100644 installing/deps-installer/src/install/writeLockfilesAndRecordVerified.ts create mode 100644 installing/deps-installer/src/install/writeWantedLockfileAndRecordVerified.ts create mode 100644 installing/deps-installer/test/install/recordLockfileVerified.ts diff --git a/.changeset/record-locally-resolved-lockfile-verified.md b/.changeset/record-locally-resolved-lockfile-verified.md new file mode 100644 index 0000000000..9cbe71e8d1 --- /dev/null +++ b/.changeset/record-locally-resolved-lockfile-verified.md @@ -0,0 +1,7 @@ +--- +"@pnpm/installing.deps-installer": patch +"@pnpm/lockfile.fs": minor +"pnpm": patch +--- + +Record the post-resolution lockfile in the verification cache. Previously the cache only captured the lockfile that was loaded at the start of an install, so a flow like `pnpm install ` 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. The fresh lockfile is now recorded immediately after each install-time write, so the second install takes the cache fast path. diff --git a/AGENTS.md b/AGENTS.md index 8a11eaae38..46a80500a2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -186,6 +186,23 @@ To ensure your code adheres to the style guide, run: pnpm run lint ``` +### Comments + +Write code that explains itself. A reader should understand what a function does from its name, parameters, and types — not from prose above the call site. + +Defaults: + +- **Do not write a comment** that restates what the code already says. If renaming a variable, splitting a helper, or moving a check to a more obvious place would carry the information, do that instead. +- **Do not repeat documentation** at call sites that already lives on the callee. If the function has a JSDoc, the call site shouldn't re-explain what calling it does. Update the JSDoc once; let every call site benefit. +- **JSDoc is for the function's contract** — preconditions, postconditions, edge cases, why the function exists. Not for re-narrating the body. + +Write a comment only when: + +- The reason for the code is non-obvious from reading it (a hidden invariant, a workaround for a known bug, a deliberate exception to the surrounding pattern). +- The right name doesn't fit — e.g., a temporary technical constraint that's worth flagging but doesn't justify a new symbol. + +Before adding a comment, ask: "Could I rename, restructure, or extract instead?" If yes, do that. The bar for prose-in-code is high; the bar for prose-that-restates-code is "don't." + ## Common Gotchas ### Error Type Checking in Jest (TypeScript only) diff --git a/installing/deps-installer/src/install/index.ts b/installing/deps-installer/src/install/index.ts index 0ea17ae628..98b41900a2 100644 --- a/installing/deps-installer/src/install/index.ts +++ b/installing/deps-installer/src/install/index.ts @@ -42,6 +42,7 @@ import { writeModulesManifest } from '@pnpm/installing.modules-yaml' import { type CatalogSnapshots, cleanGitBranchLockfiles, + getWantedLockfileName, type LockfileObject, type ProjectSnapshot, readWantedLockfile, @@ -97,6 +98,8 @@ import { linkPackages } from './link.js' import { reportPeerDependencyIssues } from './reportPeerDependencyIssues.js' import { validateModules } from './validateModules.js' import { verifyLockfileResolutions } from './verifyLockfileResolutions.js' +import { writeLockfilesAndRecordVerified } from './writeLockfilesAndRecordVerified.js' +import { writeWantedLockfileAndRecordVerified } from './writeWantedLockfileAndRecordVerified.js' class LockfileConfigMismatchError extends PnpmError { constructor (outdatedLockfileSettingName: string) { @@ -358,10 +361,17 @@ export async function mutateModules ( // resolver's own filters already cover fresh resolution. We run this // exactly once, right after the lockfile is loaded from disk, before any // path branches. + const cacheActive = opts.cacheDir != null && opts.resolutionVerifiers.length > 0 + const wantedLockfilePath = cacheActive + ? path.resolve(ctx.lockfileDir, await getWantedLockfileName({ + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + })) + : undefined try { await verifyLockfileResolutions(ctx.wantedLockfile, opts.resolutionVerifiers, { cacheDir: opts.cacheDir, - lockfilePath: path.resolve(ctx.lockfileDir, WANTED_LOCKFILE), + lockfilePath: wantedLockfilePath, }) } catch (err) { // verifyLockfileResolutions is the one throw site in this function @@ -1657,11 +1667,13 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { const currentLockfileDir = path.join(ctx.rootModulesDir, '.pnpm') await Promise.all([ opts.useLockfile && opts.saveLockfile - ? writeLockfiles({ + ? writeLockfilesAndRecordVerified({ currentLockfile: result.currentLockfile, currentLockfileDir, wantedLockfile: newLockfile, wantedLockfileDir: ctx.lockfileDir, + cacheDir: opts.cacheDir, + resolutionVerifiers: opts.resolutionVerifiers, ...lockfileOpts, }) : writeCurrentLockfile(ctx.virtualStoreDir, result.currentLockfile), @@ -1716,7 +1728,13 @@ const _installInContext: InstallFunction = async (projects, ctx, opts) => { } } else { if (opts.useLockfile && opts.saveLockfile && !isInstallationOnlyForLockfileCheck) { - await writeWantedLockfile(ctx.lockfileDir, newLockfile, lockfileOpts) + await writeWantedLockfileAndRecordVerified({ + lockfileDir: ctx.lockfileDir, + lockfile: newLockfile, + cacheDir: opts.cacheDir, + resolutionVerifiers: opts.resolutionVerifiers, + ...lockfileOpts, + }) } if (opts.nodeLinker !== 'hoisted') { @@ -2264,7 +2282,14 @@ async function installFromPnpmRegistry ( storeIndex.close() } - await writeWantedLockfile(lockfileDir, lockfile) + await writeWantedLockfileAndRecordVerified({ + lockfileDir, + lockfile, + cacheDir: opts.cacheDir, + resolutionVerifiers: opts.resolutionVerifiers, + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + }) logger.info({ message: `Resolved ${agentStats.totalPackages} packages: ${agentStats.alreadyInStore} cached, ${agentStats.filesToDownload} files to download`, diff --git a/installing/deps-installer/src/install/recordLockfileVerified.ts b/installing/deps-installer/src/install/recordLockfileVerified.ts new file mode 100644 index 0000000000..f837537b01 --- /dev/null +++ b/installing/deps-installer/src/install/recordLockfileVerified.ts @@ -0,0 +1,36 @@ +import { hashObject } from '@pnpm/crypto.object-hasher' +import type { LockfileObject } from '@pnpm/lockfile.fs' +import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' + +import { recordVerification } from './verifyLockfileResolutionsCache.js' + +export interface RecordLockfileVerifiedOptions { + cacheDir?: string + /** Absolute path of the lockfile the next install will read. + * Under `useGitBranchLockfile` this is the branch-suffixed name. */ + lockfilePath: string + /** The writer's canonical return value — see {@link writeWantedLockfile}. + * Passing the raw in-memory write object would record a hash the + * next install can't match (YAML drops undefined fields). */ + lockfile: LockfileObject + resolutionVerifiers: readonly ResolutionVerifier[] | undefined +} + +/** + * Records the post-resolution lockfile as verified so the next install + * skips the registry round-trip. Skipping is safe: fresh local picks + * are filtered by the resolver (see + * `resolving/npm-resolver/src/pickPackage.ts`) and carried-over entries + * already passed the gate at the top of `mutateModules`, so the + * recorded lockfile is policy-clean by construction. + */ +export function recordLockfileVerified (opts: RecordLockfileVerifiedOptions): void { + if (!opts.cacheDir) return + if (!opts.resolutionVerifiers?.length) return + if (!opts.lockfile.packages) return + recordVerification(opts.cacheDir, { + lockfilePath: opts.lockfilePath, + verifiers: opts.resolutionVerifiers, + hashLockfile: () => hashObject(opts.lockfile), + }) +} diff --git a/installing/deps-installer/src/install/verifyLockfileResolutions.ts b/installing/deps-installer/src/install/verifyLockfileResolutions.ts index a10afe6158..c2ac9a2c2e 100644 --- a/installing/deps-installer/src/install/verifyLockfileResolutions.ts +++ b/installing/deps-installer/src/install/verifyLockfileResolutions.ts @@ -97,8 +97,7 @@ export async function verifyLockfileResolutions ( // miss the precomputed stat+hash flow to recordVerification. type Precomputed = ReturnType['precomputed'] let cachePrecomputed: Precomputed | undefined - // hashObject is streaming (no "Invalid string length" on huge lockfiles) - // and key-order-stable, which JSON.stringify is not. + // hashObject streams and is key-order-stable, unlike JSON.stringify. let cachedHash: string | undefined const hashLockfile = (): string => { if (cachedHash == null) cachedHash = hashObject(lockfile) diff --git a/installing/deps-installer/src/install/writeLockfilesAndRecordVerified.ts b/installing/deps-installer/src/install/writeLockfilesAndRecordVerified.ts new file mode 100644 index 0000000000..576e833725 --- /dev/null +++ b/installing/deps-installer/src/install/writeLockfilesAndRecordVerified.ts @@ -0,0 +1,48 @@ +import path from 'node:path' + +import { getWantedLockfileName, type LockfileObject, writeLockfiles, type WriteLockfilesResult } from '@pnpm/lockfile.fs' +import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' + +import { recordLockfileVerified } from './recordLockfileVerified.js' + +export interface WriteLockfilesAndRecordVerifiedOptions { + wantedLockfile: LockfileObject + wantedLockfileDir: string + currentLockfile: LockfileObject + currentLockfileDir: string + useGitBranchLockfile?: boolean + mergeGitBranchLockfiles?: boolean + cacheDir?: string + resolutionVerifiers: readonly ResolutionVerifier[] | undefined +} + +/** Plural counterpart of {@link writeWantedLockfileAndRecordVerified}. */ +export async function writeLockfilesAndRecordVerified ( + opts: WriteLockfilesAndRecordVerifiedOptions +): Promise { + const cacheActive = opts.cacheDir != null && (opts.resolutionVerifiers?.length ?? 0) > 0 + const wantedLockfileName = cacheActive + ? await getWantedLockfileName({ + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + }) + : undefined + const written = await writeLockfiles({ + wantedLockfile: opts.wantedLockfile, + wantedLockfileDir: opts.wantedLockfileDir, + currentLockfile: opts.currentLockfile, + currentLockfileDir: opts.currentLockfileDir, + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + wantedLockfileName, + }) + if (cacheActive) { + recordLockfileVerified({ + cacheDir: opts.cacheDir, + lockfilePath: path.resolve(opts.wantedLockfileDir, wantedLockfileName!), + lockfile: written.wantedLockfile, + resolutionVerifiers: opts.resolutionVerifiers, + }) + } + return written +} diff --git a/installing/deps-installer/src/install/writeWantedLockfileAndRecordVerified.ts b/installing/deps-installer/src/install/writeWantedLockfileAndRecordVerified.ts new file mode 100644 index 0000000000..0c3838b26b --- /dev/null +++ b/installing/deps-installer/src/install/writeWantedLockfileAndRecordVerified.ts @@ -0,0 +1,42 @@ +import path from 'node:path' + +import { getWantedLockfileName, type LockfileObject, writeWantedLockfile } from '@pnpm/lockfile.fs' +import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' + +import { recordLockfileVerified } from './recordLockfileVerified.js' + +export interface WriteWantedLockfileAndRecordVerifiedOptions { + lockfileDir: string + lockfile: LockfileObject + cacheDir?: string + resolutionVerifiers: readonly ResolutionVerifier[] | undefined + useGitBranchLockfile?: boolean + mergeGitBranchLockfiles?: boolean +} + +/** Combines {@link writeWantedLockfile} and {@link recordLockfileVerified} — see each for semantics. */ +export async function writeWantedLockfileAndRecordVerified ( + opts: WriteWantedLockfileAndRecordVerifiedOptions +): Promise { + const cacheActive = opts.cacheDir != null && (opts.resolutionVerifiers?.length ?? 0) > 0 + const lockfileName = cacheActive + ? await getWantedLockfileName({ + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + }) + : undefined + const written = await writeWantedLockfile(opts.lockfileDir, opts.lockfile, { + useGitBranchLockfile: opts.useGitBranchLockfile, + mergeGitBranchLockfiles: opts.mergeGitBranchLockfiles, + lockfileName, + }) + if (cacheActive) { + recordLockfileVerified({ + cacheDir: opts.cacheDir, + lockfilePath: path.resolve(opts.lockfileDir, lockfileName!), + lockfile: written, + resolutionVerifiers: opts.resolutionVerifiers, + }) + } + return written +} diff --git a/installing/deps-installer/test/install/recordLockfileVerified.ts b/installing/deps-installer/test/install/recordLockfileVerified.ts new file mode 100644 index 0000000000..32b975c8a4 --- /dev/null +++ b/installing/deps-installer/test/install/recordLockfileVerified.ts @@ -0,0 +1,214 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { afterEach, beforeEach, expect, test } from '@jest/globals' +import { WANTED_LOCKFILE } from '@pnpm/constants' +import { hashObject } from '@pnpm/crypto.object-hasher' +import { type LockfileObject, readWantedLockfile, writeWantedLockfile } from '@pnpm/lockfile.fs' +import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' + +import { recordLockfileVerified } from '../../src/install/recordLockfileVerified.js' +import { tryLockfileVerificationCache } from '../../src/install/verifyLockfileResolutionsCache.js' + +let tmpDir!: string +let cacheDir!: string +let lockfileDir!: string +let lockfilePath!: string + +beforeEach(async () => { + tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'pnpm-record-verified-')) + cacheDir = path.join(tmpDir, 'cache') + lockfileDir = path.join(tmpDir, 'project') + lockfilePath = path.resolve(lockfileDir, WANTED_LOCKFILE) + await fs.promises.mkdir(lockfileDir, { recursive: true }) +}) + +afterEach(async () => { + await fs.promises.rm(tmpDir, { recursive: true, force: true }) +}) + +function mraVerifier (current: number): ResolutionVerifier { + return { + policy: { minimumReleaseAge: current }, + canTrustPastCheck: (cached) => { + const past = cached.minimumReleaseAge + return typeof past === 'number' && past >= current + }, + verify: async () => ({ ok: true }), + } +} + +function makeLockfile (): LockfileObject { + return { + lockfileVersion: '9.0', + importers: { + '.': { + specifiers: { 'is-positive': '^1.0.0' }, + dependencies: { 'is-positive': '1.0.0' }, + }, + }, + packages: { + 'is-positive@1.0.0': { + resolution: { + integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=', + tarball: '', + }, + }, + }, + } as unknown as LockfileObject +} + +test('no-op when cacheDir is undefined', () => { + recordLockfileVerified({ + cacheDir: undefined, + lockfilePath, + lockfile: makeLockfile(), + resolutionVerifiers: [mraVerifier(60)], + }) + expect(fs.existsSync(cacheDir)).toBe(false) +}) + +test('no-op when resolutionVerifiers is empty', () => { + recordLockfileVerified({ + cacheDir, + lockfilePath, + lockfile: makeLockfile(), + resolutionVerifiers: [], + }) + expect(fs.existsSync(cacheDir)).toBe(false) +}) + +test('no-op when resolutionVerifiers is undefined', () => { + recordLockfileVerified({ + cacheDir, + lockfilePath, + lockfile: makeLockfile(), + resolutionVerifiers: undefined, + }) + expect(fs.existsSync(cacheDir)).toBe(false) +}) + +test('records nothing when the in-memory lockfile has no packages', async () => { + await writeWantedLockfile(lockfileDir, makeLockfile()) + recordLockfileVerified({ + cacheDir, + lockfilePath, + lockfile: { lockfileVersion: '9.0', importers: {} } as unknown as LockfileObject, + resolutionVerifiers: [mraVerifier(60)], + }) + expect(fs.existsSync(path.join(cacheDir, 'lockfile-verified.jsonl'))).toBe(false) +}) + +test('records the load-equivalent hash — matches what the next install computes off-disk', async () => { + // Use a fixture that carries an explicit `undefined` optional field + // (the real divergence case install-time code produces), then pass + // the *writer's return value* to recordLockfileVerified — same flow + // as the production call sites. Passing the in-memory input here + // instead would silently regress the moment the writer's + // canonicalization stops matching the reader's output. + const inMemoryLockfile = { + ...makeLockfile(), + settings: { + autoInstallPeers: true, + excludeLinksFromLockfile: false, + dedupePeers: undefined, + }, + } as unknown as LockfileObject + const written = await writeWantedLockfile(lockfileDir, inMemoryLockfile) + recordLockfileVerified({ + cacheDir, + lockfilePath, + lockfile: written, + resolutionVerifiers: [mraVerifier(60)], + }) + + // The cache contract: the next install hashes its loaded + // `LockfileObject` and looks the hash up. The recorded hash must + // match what that lookup computes. + const loaded = await readWantedLockfile(lockfileDir, { ignoreIncompatible: false }) + expect(loaded).not.toBeNull() + const expectedHash = hashObject(loaded!) + + const cacheFile = path.join(cacheDir, 'lockfile-verified.jsonl') + const record = JSON.parse(fs.readFileSync(cacheFile, 'utf8').trim()) as { + lockfile: { hash: string, path: string } + } + expect(record.lockfile.hash).toBe(expectedHash) + expect(record.lockfile.path).toBe(lockfilePath) +}) + +test('respects the caller-supplied lockfilePath — git-branch lockfiles record under their branch-suffixed filename', async () => { + // Simulates `useGitBranchLockfile`: the actual on-disk lockfile is + // pnpm-lock..yaml, not pnpm-lock.yaml. The helper has no + // git logic of its own — it records whatever path the caller hands + // it, so cache lookups on the same path will hit. + const branchLockfilePath = path.resolve(lockfileDir, 'pnpm-lock.feature-x.yaml') + await fs.promises.writeFile(branchLockfilePath, 'lockfileVersion: \'9.0\'\n') + const lockfile = makeLockfile() + recordLockfileVerified({ + cacheDir, + lockfilePath: branchLockfilePath, + lockfile, + resolutionVerifiers: [mraVerifier(60)], + }) + const cacheFile = path.join(cacheDir, 'lockfile-verified.jsonl') + const record = JSON.parse(fs.readFileSync(cacheFile, 'utf8').trim()) as { + lockfile: { path: string } + } + expect(record.lockfile.path).toBe(branchLockfilePath) +}) + +test('records a cache entry that the next install hits on both the stat shortcut and hash fallback paths', async () => { + // Mirror real call sites: hand `recordLockfileVerified` the + // writer's return value rather than the in-memory input. With an + // explicit `undefined` optional field in the fixture, those two + // diverge structurally — the in-memory variant would record a hash + // the next install can't match, and this test would silently miss + // that regression. + const inMemoryLockfile = { + ...makeLockfile(), + settings: { + autoInstallPeers: true, + excludeLinksFromLockfile: false, + dedupePeers: undefined, + }, + } as unknown as LockfileObject + const written = await writeWantedLockfile(lockfileDir, inMemoryLockfile) + recordLockfileVerified({ + cacheDir, + lockfilePath, + lockfile: written, + resolutionVerifiers: [mraVerifier(60)], + }) + const loaded = (await readWantedLockfile(lockfileDir, { ignoreIncompatible: false }))! + + // Stat shortcut: file untouched since record. + const statResult = tryLockfileVerificationCache(cacheDir, { + lockfilePath, + verifiers: [mraVerifier(60)], + hashLockfile: () => hashObject(loaded), + }) + expect(statResult.hit).toBe(true) + + // Hash fallback: invalidate stat fields the cache compares against so the + // shortcut bails. This is the CI-checkout / new-worktree path; the hash + // has to match for the fallback to hit, which is the whole point of + // hashing the canonical load-equivalent form. Use `size = -1` (impossible + // for a real file) rather than zeroing `inode`/`mtimeNs` alone — on + // Windows `stat.ino` is often 0, which would let the cached record + // accidentally match and skip the fallback path we want to exercise. + const cacheFile = path.join(cacheDir, 'lockfile-verified.jsonl') + const cached = JSON.parse(fs.readFileSync(cacheFile, 'utf8').trim()) as { + lockfile: { size: number } + } + cached.lockfile.size = -1 + fs.writeFileSync(cacheFile, `${JSON.stringify(cached)}\n`) + + const hashResult = tryLockfileVerificationCache(cacheDir, { + lockfilePath, + verifiers: [mraVerifier(60)], + hashLockfile: () => hashObject(loaded), + }) + expect(hashResult.hit).toBe(true) +}) diff --git a/lockfile/fs/src/index.ts b/lockfile/fs/src/index.ts index 7693bce771..910f216103 100644 --- a/lockfile/fs/src/index.ts +++ b/lockfile/fs/src/index.ts @@ -3,12 +3,14 @@ export { existsNonEmptyWantedLockfile } from './existsWantedLockfile.js' export { getLockfileImporterId } from './getLockfileImporterId.js' export { cleanGitBranchLockfiles } from './gitBranchLockfile.js' export { convertToLockfileFile, convertToLockfileObject } from './lockfileFormatConverters.js' +export { getWantedLockfileName } from './lockfileName.js' export * from './read.js' export { isEmptyLockfile, writeCurrentLockfile, writeLockfileFile, writeLockfiles, + type WriteLockfilesResult, writeWantedLockfile, } from './write.js' export { extractMainDocument } from './yamlDocuments.js' diff --git a/lockfile/fs/src/write.ts b/lockfile/fs/src/write.ts index 46de136232..dd6a1fc575 100644 --- a/lockfile/fs/src/write.ts +++ b/lockfile/fs/src/write.ts @@ -8,7 +8,7 @@ import yaml from 'js-yaml' import { isEmpty } from 'ramda' import writeFileAtomic from 'write-file-atomic' -import { convertToLockfileFile } from './lockfileFormatConverters.js' +import { convertToLockfileFile, convertToLockfileObject } from './lockfileFormatConverters.js' import { getWantedLockfileName } from './lockfileName.js' import { lockfileLogger as logger } from './logger.js' import { sortLockfileKeys } from './sortLockfileKeys.js' @@ -26,26 +26,34 @@ export function lockfileYamlDump (obj: object): string { return yaml.dump(obj, LOCKFILE_YAML_FORMAT) } +/** + * Returns the canonical post-write lockfile — structurally identical + * to what `readWantedLockfile` would parse back. Lets callers like + * the verification cache hash the as-saved form without re-reading. + */ export async function writeWantedLockfile ( pkgPath: string, wantedLockfile: LockfileObject, opts?: { useGitBranchLockfile?: boolean mergeGitBranchLockfiles?: boolean + /** Pre-resolved filename; skips the `getWantedLockfileName` (and + * its `getCurrentBranch`) call when supplied. */ + lockfileName?: string } -): Promise { - const wantedLockfileName: string = await getWantedLockfileName(opts) +): Promise { + const wantedLockfileName: string = opts?.lockfileName ?? await getWantedLockfileName(opts) return writeLockfile(wantedLockfileName, pkgPath, wantedLockfile) } export async function writeCurrentLockfile ( virtualStoreDir: string, currentLockfile: LockfileObject -): Promise { +): Promise { // empty lockfile is not saved if (isEmptyLockfile(currentLockfile)) { await rimraf(path.join(virtualStoreDir, 'lock.yaml')) - return + return undefined } await fs.mkdir(virtualStoreDir, { recursive: true }) return writeLockfile('lock.yaml', virtualStoreDir, currentLockfile) @@ -55,7 +63,7 @@ async function writeLockfile ( lockfileFilename: string, pkgPath: string, wantedLockfile: LockfileObject -): Promise { +): Promise { const lockfilePath = path.join(pkgPath, lockfileFilename) const lockfileToStringify = convertToLockfileFile(wantedLockfile) @@ -69,9 +77,27 @@ async function writeLockfile ( // in the OS page cache and streaming stops at the first separator. const envDoc = await streamReadFirstYamlDocument(lockfilePath) const envPrefix = envDoc != null ? `${YAML_DOCUMENT_START}${envDoc}${YAML_DOCUMENT_SEPARATOR}` : '' - return writeFileAtomic(lockfilePath, `${envPrefix}${yamlDoc}`) + await writeFileAtomic(lockfilePath, `${envPrefix}${yamlDoc}`) + } else { + await writeFileAtomic(lockfilePath, yamlDoc) } - return writeFileAtomic(lockfilePath, yamlDoc) + + // YAML drops undefined on serialize, so the in-memory LockfileFile + // can carry fields (like an unset settings.dedupePeers) that won't + // survive a round-trip; strip them to mirror what the next reader + // will parse back. + return convertToLockfileObject(stripUndefinedDeep(lockfileToStringify) as LockfileFile) +} + +function stripUndefinedDeep (value: T): T { + if (value === null || typeof value !== 'object') return value + if (Array.isArray(value)) return value.map(stripUndefinedDeep) as unknown as T + const out: Record = {} + for (const [k, v] of Object.entries(value as Record)) { + if (v === undefined) continue + out[k] = stripUndefinedDeep(v) + } + return out as T } export function writeLockfileFile ( @@ -91,6 +117,19 @@ export function isEmptyLockfile (lockfile: LockfileObject): boolean { return Object.values(lockfile.importers).every((importer) => isEmpty(importer.specifiers ?? {}) && isEmpty(importer.dependencies ?? {})) } +export interface WriteLockfilesResult { + /** + * The canonical "as-saved" wanted lockfile — the inverse converter + * applied to the same object that was serialized to YAML. Hashing + * this is equivalent to hashing the lockfile the next install will + * load from disk (modulo undefined values that YAML drops, which any + * sensible canonicalization-then-hash routine should strip). + */ + wantedLockfile: LockfileObject + /** Same as above for the current lockfile, or undefined when it was skipped because empty. */ + currentLockfile: LockfileObject | undefined +} + export async function writeLockfiles ( opts: { wantedLockfile: LockfileObject @@ -99,9 +138,11 @@ export async function writeLockfiles ( currentLockfileDir: string useGitBranchLockfile?: boolean mergeGitBranchLockfiles?: boolean + /** See {@link writeWantedLockfile}'s `lockfileName` option. */ + wantedLockfileName?: string } -): Promise { - const wantedLockfileName: string = await getWantedLockfileName(opts) +): Promise { + const wantedLockfileName: string = opts.wantedLockfileName ?? await getWantedLockfileName(opts) const wantedLockfilePath = path.join(opts.wantedLockfileDir, wantedLockfileName) const currentLockfilePath = path.join(opts.currentLockfileDir, 'lock.yaml') @@ -134,7 +175,12 @@ export async function writeLockfiles ( } })(), ]) - return + // Both files share the same source object; strip once and reuse. + const normalized = convertToLockfileObject(stripUndefinedDeep(wantedLockfileToStringify) as LockfileFile) + return { + wantedLockfile: normalized, + currentLockfile: isEmptyLockfile(opts.wantedLockfile) ? undefined : normalized, + } } logger.debug({ @@ -145,10 +191,13 @@ export async function writeLockfiles ( const currentLockfileToStringify = convertToLockfileFile(opts.currentLockfile) const currentYamlDoc = yamlStringify(currentLockfileToStringify) + // Filtered-current callers (deps-restorer) can pass an empty + // current against a non-empty wanted; key off the current. + const currentIsEmpty = isEmptyLockfile(opts.currentLockfile) await Promise.all([ writeFileAtomic(wantedLockfilePath, wantedYamlDoc), (async () => { - if (isEmptyLockfile(opts.wantedLockfile)) { + if (currentIsEmpty) { await rimraf(currentLockfilePath) } else { await fs.mkdir(path.dirname(currentLockfilePath), { recursive: true }) @@ -156,4 +205,10 @@ export async function writeLockfiles ( } })(), ]) + return { + wantedLockfile: convertToLockfileObject(stripUndefinedDeep(wantedLockfileToStringify) as LockfileFile), + currentLockfile: currentIsEmpty + ? undefined + : convertToLockfileObject(stripUndefinedDeep(currentLockfileToStringify) as LockfileFile), + } } diff --git a/lockfile/fs/test/write.test.ts b/lockfile/fs/test/write.test.ts index a6ed6183f3..13a39ccbe9 100644 --- a/lockfile/fs/test/write.test.ts +++ b/lockfile/fs/test/write.test.ts @@ -98,6 +98,76 @@ test('writeLockfiles() when no specifiers but dependencies present', async () => expect(await readWantedLockfile(projectPath, { ignoreIncompatible: false })).toEqual(wantedLockfile) }) +test('writeWantedLockfile() returns the canonical lockfile — matches what readWantedLockfile produces, even when the input carries undefined optional fields', async () => { + // Cache-key contract: callers (today, the verification cache) need a + // hash of the *as-saved* lockfile, not the in-memory write object. + // Those two diverge specifically because YAML drops `undefined` on + // serialize. To exercise that drop, the fixture has to actually + // carry an explicit `undefined` — `settings.dedupePeers` here, the + // same field install-time code produces (see + // installing/deps-installer/src/install/index.ts where it's set to + // `opts.dedupePeers || undefined`). Without this, the test would + // happily pass against a writer that returned a near-canonical-but- + // still-divergent object. + const projectPath = temporaryDirectory() + const wantedLockfile = { + importers: { + '.': { + specifiers: { 'is-positive': '^1.0.0' }, + dependencies: { 'is-positive': '1.0.0' }, + }, + }, + lockfileVersion: LOCKFILE_VERSION, + settings: { + autoInstallPeers: true, + excludeLinksFromLockfile: false, + dedupePeers: undefined, + }, + packages: { + '/is-positive@1.0.0': { + resolution: { + integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=', + }, + }, + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any + const written = await writeWantedLockfile(projectPath, wantedLockfile) + const loaded = await readWantedLockfile(projectPath, { ignoreIncompatible: false }) + expect(written).toEqual(loaded) + // Verify the canonicalization actually dropped the undefined field — + // toEqual is lenient about undefined-vs-missing, so check explicitly. + expect('dedupePeers' in (written.settings ?? {})).toBe(false) +}) + +test('writeLockfiles() return matches readWantedLockfile/readCurrentLockfile output', async () => { + const projectPath = temporaryDirectory() + const wantedLockfile = { + importers: { + '.': { + specifiers: { 'is-positive': '^1.0.0' }, + dependencies: { 'is-positive': '1.0.0' }, + }, + }, + lockfileVersion: LOCKFILE_VERSION, + packages: { + '/is-positive@1.0.0': { + resolution: { integrity: 'sha1-ChbBDewTLAqLCzb793Fo5VDvg/g=' }, + }, + }, + } + const written = await writeLockfiles({ + currentLockfile: wantedLockfile, + currentLockfileDir: projectPath, + wantedLockfile, + wantedLockfileDir: projectPath, + }) + const loadedWanted = await readWantedLockfile(projectPath, { ignoreIncompatible: false }) + const loadedCurrent = await readCurrentLockfile(projectPath, { ignoreIncompatible: false }) + expect(written.wantedLockfile).toEqual(loadedWanted) + expect(written.currentLockfile).toEqual(loadedCurrent) +}) + test('write does not use yaml anchors/aliases', async () => { const projectPath = temporaryDirectory() const wantedLockfile = { diff --git a/pacquet/AGENTS.md b/pacquet/AGENTS.md index 246c269219..3fb7c0bd77 100644 --- a/pacquet/AGENTS.md +++ b/pacquet/AGENTS.md @@ -285,6 +285,18 @@ Run `just ready` (full suite) before handing the PR off. re-exports such as `pub use submodule::*;` in a `lib.rs`. See the "No star imports" section in `CODE_STYLE_GUIDE.md`. +### Comments + +Same baseline as [`../AGENTS.md`](../AGENTS.md#comments): write code that explains itself; comments are for the non-obvious *why*, not a translation of the *what*. + +Rust-specific defaults: + +- **Doc comments (`///`, `//!`) document the contract.** Preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body. +- **Do not restate at call sites what the callee's doc comment already says.** If `///` on the function says "no-op when …", the caller should not repeat that. Update the doc once; let every call site benefit. +- **`// SAFETY:`, `// TODO:`, and similar prefixes are the exception.** They signal hidden invariants or known follow-ups that a reader cannot recover from the code alone. + +Prefer renaming, restructuring, or extracting a helper over leaving a comment. Reach for prose only when the right names and types genuinely cannot carry the information. + ### Preserve existing method chains When editing existing code, do not break a method chain (including `pipe-trait` diff --git a/pnpm/test/install/minimumReleaseAge.ts b/pnpm/test/install/minimumReleaseAge.ts index 0cf4e54dfc..f177a107cd 100644 --- a/pnpm/test/install/minimumReleaseAge.ts +++ b/pnpm/test/install/minimumReleaseAge.ts @@ -120,6 +120,48 @@ describe('lockfile minimumReleaseAge verification', () => { ) }) + test('a fresh install records the just-written lockfile in the verification cache', async () => { + // Reproduces the "install foo, rm -rf node_modules, install" flow: + // the lockfile written by the first install must be recorded under + // its post-resolution content, otherwise the second install re-runs + // the registry round-trip even though the resolver already enforced + // the policy when picking those versions. + prepare({}) + const cacheDir = path.resolve('pnpm-cache') + writeYamlFileSync('pnpm-workspace.yaml', { + minimumReleaseAge: 1, + minimumReleaseAgeStrict: true, + cacheDir, + }) + + execPnpmSync( + [PUBLIC_REGISTRY, 'add', 'is-positive@1.0.0'], + { ...omitMinReleaseAgeEnv, expectSuccess: true } + ) + + const cacheFile = path.join(cacheDir, 'lockfile-verified.jsonl') + expect(fs.existsSync(cacheFile)).toBe(true) + const lines = fs.readFileSync(cacheFile, 'utf8').split('\n').filter(Boolean) + const records = lines.map((line) => JSON.parse(line) as { + lockfile: { hash: string, path: string } + policy: Record + }) + const lockfilePath = path.resolve('pnpm-lock.yaml') + const recordForLockfile = records.find((r) => r.lockfile.path === lockfilePath) + expect(recordForLockfile).toBeDefined() + expect(recordForLockfile!.policy).toMatchObject({ minimumReleaseAge: 1 }) + + // Re-running install completes without hitting the registry to + // re-verify is-positive. We can't directly observe the network skip, + // but a clean run with --offline confirms the cache short-circuit + // works end-to-end (the verifier would otherwise need a registry + // round-trip to evaluate the cutoff). + execPnpmSync( + [PUBLIC_REGISTRY, '--offline', 'install', '--frozen-lockfile'], + { ...omitMinReleaseAgeEnv, expectSuccess: true } + ) + }) + test('loose mode rejects immature lockfile entries that are not on minimumReleaseAgeExclude', () => { // The verifier now runs in loose mode too, so a lockfile produced under // no policy that still has immature pins is rejected the same way diff --git a/releasing/commands/src/deploy/deploy.ts b/releasing/commands/src/deploy/deploy.ts index 3dfc5a53d1..bcc0b19008 100644 --- a/releasing/commands/src/deploy/deploy.ts +++ b/releasing/commands/src/deploy/deploy.ts @@ -246,7 +246,7 @@ async function deployFromSharedLockfile ( allowBuilds: opts.allowBuilds, }) - const filesToWrite: Array> = [ + const filesToWrite: Array> = [ fs.promises.writeFile( path.join(deployDir, 'package.json'), JSON.stringify(deployFiles.manifest, undefined, 2) + '\n'