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'