From c880cf81035030ea379b3c007d6d102e16c04419 Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sat, 29 Aug 2026 13:47:46 +0200 Subject: [PATCH 1/4] Fail clearly when pnpm patch hashes are missing --- src/lib/patches/copy-patches.test.ts | 114 ++++++++++++++++++++++++--- src/lib/patches/copy-patches.ts | 10 +-- 2 files changed, 105 insertions(+), 19 deletions(-) diff --git a/src/lib/patches/copy-patches.test.ts b/src/lib/patches/copy-patches.test.ts index 0da0a20..82e0477 100644 --- a/src/lib/patches/copy-patches.test.ts +++ b/src/lib/patches/copy-patches.test.ts @@ -51,7 +51,19 @@ vi.mock("pnpm_lockfile_file_v8", () => ({ })); vi.mock("pnpm_lockfile_file_v9", () => ({ - readWantedLockfile: vi.fn(() => Promise.resolve(null)), + readWantedLockfile: vi.fn(() => + Promise.resolve({ + lockfileVersion: "9.0", + patchedDependencies: { + "@firebase/app@1.2.3": "sha256-firebase", + "lodash@4.17.21": "sha256-lodash", + "pkg-a@1.0.0": "sha256-pkg-a", + "pkg-b@1.0.0": "sha256-pkg-b", + "tslib@2.0.0": "sha256-tslib", + "vitest@1.0.0": "sha256-vitest", + }, + }), + ), getLockfileImporterId: vi.fn( (root: string, dir: string) => dir.replace(`${root}/`, "") || ".", ), @@ -208,7 +220,10 @@ describe("copyPatches", () => { }); expect(result).toEqual({ - "lodash@4.17.21": { path: "patches/lodash.patch", hash: "" }, + "lodash@4.17.21": { + path: "patches/lodash.patch", + hash: "sha256-lodash", + }, }); /** Should preserve original folder structure */ expect(fs.ensureDir).toHaveBeenCalledWith("/workspace/isolate/patches"); @@ -248,7 +263,10 @@ describe("copyPatches", () => { }); expect(result).toEqual({ - "vitest@1.0.0": { path: "patches/vitest.patch", hash: "" }, + "vitest@1.0.0": { + path: "patches/vitest.patch", + hash: "sha256-vitest", + }, }); expect(filterPatchedDependencies).toHaveBeenCalledWith({ patchedDependencies: { "vitest@1.0.0": "patches/vitest.patch" }, @@ -325,7 +343,10 @@ describe("copyPatches", () => { }); expect(result).toEqual({ - "@firebase/app@1.2.3": { path: "patches/firebase-app.patch", hash: "" }, + "@firebase/app@1.2.3": { + path: "patches/firebase-app.patch", + hash: "sha256-firebase", + }, }); }); @@ -362,8 +383,8 @@ describe("copyPatches", () => { /** Should preserve original paths without renaming */ expect(result).toEqual({ - "pkg-a@1.0.0": { path: "patches/v1/fix.patch", hash: "" }, - "pkg-b@1.0.0": { path: "patches/v2/fix.patch", hash: "" }, + "pkg-a@1.0.0": { path: "patches/v1/fix.patch", hash: "sha256-pkg-a" }, + "pkg-b@1.0.0": { path: "patches/v2/fix.patch", hash: "sha256-pkg-b" }, }); expect(fs.copy).toHaveBeenCalledTimes(2); expect(fs.copy).toHaveBeenCalledWith( @@ -407,7 +428,10 @@ describe("copyPatches", () => { /** The path should preserve the original directory structure */ expect(result).toEqual({ - "lodash@4.17.21": { path: "some/nested/path/lodash.patch", hash: "" }, + "lodash@4.17.21": { + path: "some/nested/path/lodash.patch", + hash: "sha256-lodash", + }, }); expect(fs.ensureDir).toHaveBeenCalledWith( "/workspace/isolate/some/nested/path", @@ -453,8 +477,14 @@ describe("copyPatches", () => { }); expect(result).toEqual({ - "lodash@4.17.21": { path: "patches/lodash.patch", hash: "" }, - "@firebase/app@1.2.3": { path: "patches/firebase-app.patch", hash: "" }, + "lodash@4.17.21": { + path: "patches/lodash.patch", + hash: "sha256-lodash", + }, + "@firebase/app@1.2.3": { + path: "patches/firebase-app.patch", + hash: "sha256-firebase", + }, }); expect(fs.copy).toHaveBeenCalledTimes(2); }); @@ -567,7 +597,10 @@ describe("copyPatches", () => { }); expect(result).toEqual({ - "tslib@2.0.0": { path: "patches/tslib@2.0.0.patch", hash: "" }, + "tslib@2.0.0": { + path: "patches/tslib@2.0.0.patch", + hash: "sha256-tslib", + }, }); const filterCall = filterPatchedDependencies.mock.calls[0]?.[0]; @@ -635,6 +668,9 @@ describe("copyPatches", () => { resolution: { integrity: "sha512-y" }, }, }, + patchedDependencies: { + "@react-pdf/render@4.3.0": "sha256-react-pdf-render", + }, } as unknown as Awaited>); const result = await copyPatches({ @@ -650,7 +686,7 @@ describe("copyPatches", () => { expect(result).toEqual({ "@react-pdf/render@4.3.0": { path: "patches/@react-pdf__render@4.3.0.patch", - hash: "", + hash: "sha256-react-pdf-render", }, }); @@ -724,6 +760,62 @@ describe("copyPatches", () => { }); }); + it.each([9, 10])( + "rejects pnpm %i patches without a lockfile hash before copying files", + async (majorVersion) => { + const targetManifest: PackageManifest = { + name: "test", + version: "1.0.0", + dependencies: { lodash: "^4.0.0", underscore: "^1.0.0" }, + }; + + readTypedYamlSync.mockReturnValue({ + patchedDependencies: { + "lodash@4.17.21": "patches/lodash.patch", + "underscore@1.13.7": "patches/underscore.patch", + }, + }); + readTypedJson.mockResolvedValue({ + name: "root", + version: "1.0.0", + } as PackageManifest); + filterPatchedDependencies.mockReturnValue({ + "lodash@4.17.21": "patches/lodash.patch", + "underscore@1.13.7": "patches/underscore.patch", + }); + fs.existsSync.mockReturnValue(true); + usePackageManager.mockReturnValue({ + name: "pnpm", + majorVersion, + version: `${majorVersion}.0.0`, + packageManagerString: `pnpm@${majorVersion}.0.0`, + }); + readWantedLockfile_v9.mockResolvedValue({ + lockfileVersion: "9.0", + patchedDependencies: { + "lodash@4.17.21": "sha256-lodash", + }, + } as unknown as Awaited>); + + await expect( + copyPatches({ + workspaceRootDir: "/workspace", + targetPackageDir: "/workspace/packages/test", + internalDepPackageNames: [], + targetPackageManifest: targetManifest, + isolateDir: "/workspace/isolate", + packagesRegistry: {}, + includeDevDependencies: false, + }), + ).rejects.toThrow( + "No hash found for patch underscore@1.13.7 in lockfile", + ); + + expect(fs.ensureDir).not.toHaveBeenCalled(); + expect(fs.copy).not.toHaveBeenCalled(); + }, + ); + it("rejects pnpm 11 patches without a lockfile hash before copying files", async () => { const targetManifest: PackageManifest = { name: "test", diff --git a/src/lib/patches/copy-patches.ts b/src/lib/patches/copy-patches.ts index 4897f68..adf4124 100644 --- a/src/lib/patches/copy-patches.ts +++ b/src/lib/patches/copy-patches.ts @@ -19,7 +19,6 @@ import { } from "#/lib/utils"; import { collectInstalledNamesFromBunLockfile } from "./collect-installed-names-bun"; import { collectInstalledNamesFromPnpmLockfile } from "./collect-installed-names-pnpm"; -import { usesPnpmWorkspacePatchedDependencies } from "./pnpm-patched-dependencies"; export async function copyPatches({ workspaceRootDir, @@ -177,8 +176,7 @@ export async function copyPatches({ } /** - * Get the hash from the original lockfile, or use empty string if not - * found. pnpm 11 simplified the lockfile `patchedDependencies` format from + * pnpm 11 simplified the lockfile `patchedDependencies` format from * `Record` to `Record` (selector to * hash), so the entry may be a bare hash string. See issue #201. */ @@ -189,11 +187,7 @@ export async function copyPatches({ ? originalPatchFile : (originalPatchFile?.hash ?? ""); - if ( - packageManagerName === "pnpm" && - usesPnpmWorkspacePatchedDependencies(majorVersion) && - !hash - ) { + if (packageManagerName === "pnpm" && !hash) { if (lockfilePatchResult?.readError) { throw new Error( `Could not read pnpm lockfile while resolving patch ${packageSpec}`, From 0ae6d420598341e2c62a0a031a6c4955c2dc4a49 Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sat, 29 Aug 2026 13:54:53 +0200 Subject: [PATCH 2/4] Address code-review findings (cover pnpm 8, remove unreachable warning) --- src/lib/patches/copy-patches.test.ts | 29 +++++++++++++++++++++------- src/lib/patches/copy-patches.ts | 4 ---- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/lib/patches/copy-patches.test.ts b/src/lib/patches/copy-patches.test.ts index 82e0477..561739e 100644 --- a/src/lib/patches/copy-patches.test.ts +++ b/src/lib/patches/copy-patches.test.ts @@ -73,6 +73,9 @@ const fs = vi.mocked((await import("fs-extra")).default); const { filterPatchedDependencies, readTypedJson, readTypedYamlSync } = vi.mocked(await import("#/lib/utils")); const { usePackageManager } = vi.mocked(await import("#/lib/package-manager")); +const { readWantedLockfile: readWantedLockfile_v8 } = vi.mocked( + await import("pnpm_lockfile_file_v8"), +); const { readWantedLockfile: readWantedLockfile_v9 } = vi.mocked( await import("pnpm_lockfile_file_v9"), ); @@ -760,7 +763,7 @@ describe("copyPatches", () => { }); }); - it.each([9, 10])( + it.each([8, 9, 10])( "rejects pnpm %i patches without a lockfile hash before copying files", async (majorVersion) => { const targetManifest: PackageManifest = { @@ -790,12 +793,24 @@ describe("copyPatches", () => { version: `${majorVersion}.0.0`, packageManagerString: `pnpm@${majorVersion}.0.0`, }); - readWantedLockfile_v9.mockResolvedValue({ - lockfileVersion: "9.0", - patchedDependencies: { - "lodash@4.17.21": "sha256-lodash", - }, - } as unknown as Awaited>); + if (majorVersion >= 9) { + readWantedLockfile_v9.mockResolvedValue({ + lockfileVersion: "9.0", + patchedDependencies: { + "lodash@4.17.21": "sha256-lodash", + }, + } as unknown as Awaited>); + } else { + readWantedLockfile_v8.mockResolvedValue({ + lockfileVersion: "6.0", + patchedDependencies: { + "lodash@4.17.21": { + path: "patches/lodash.patch", + hash: "sha256-lodash", + }, + }, + } as unknown as Awaited>); + } await expect( copyPatches({ diff --git a/src/lib/patches/copy-patches.ts b/src/lib/patches/copy-patches.ts index adf4124..d6dc0f8 100644 --- a/src/lib/patches/copy-patches.ts +++ b/src/lib/patches/copy-patches.ts @@ -198,10 +198,6 @@ export async function copyPatches({ throw new Error(`No hash found for patch ${packageSpec} in lockfile`); } - if (packageManagerName === "pnpm" && !hash) { - log.warn(`No hash found for patch ${packageSpec} in lockfile`); - } - copiedPatches[packageSpec] = { path: patchPath, hash, From 726074f895accb97280070e7a125b7a5153ebc0b Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sat, 29 Aug 2026 14:02:19 +0200 Subject: [PATCH 3/4] Address GLM-review findings (clarify pnpm reader behavior) --- src/lib/patches/copy-patches.test.ts | 23 ++++++++++++++++------- src/lib/patches/copy-patches.ts | 6 +++--- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/lib/patches/copy-patches.test.ts b/src/lib/patches/copy-patches.test.ts index 561739e..25c3480 100644 --- a/src/lib/patches/copy-patches.test.ts +++ b/src/lib/patches/copy-patches.test.ts @@ -826,6 +826,13 @@ describe("copyPatches", () => { "No hash found for patch underscore@1.13.7 in lockfile", ); + if (majorVersion === 8) { + expect(readWantedLockfile_v8).toHaveBeenCalled(); + expect(readWantedLockfile_v9).not.toHaveBeenCalled(); + } else { + expect(readWantedLockfile_v8).not.toHaveBeenCalled(); + expect(readWantedLockfile_v9).toHaveBeenCalled(); + } expect(fs.ensureDir).not.toHaveBeenCalled(); expect(fs.copy).not.toHaveBeenCalled(); }, @@ -928,7 +935,7 @@ describe("copyPatches", () => { expect(fs.copy).not.toHaveBeenCalled(); }); - it("should read the patch hash from the pnpm <=10 object lockfile format (regression: issue #201)", async () => { + it("should read the patch hash from the pnpm 8 object lockfile format (regression: issue #201)", async () => { const targetManifest: PackageManifest = { name: "test", version: "1.0.0", @@ -953,20 +960,20 @@ describe("copyPatches", () => { usePackageManager.mockReturnValue({ name: "pnpm", - majorVersion: 9, - version: "9.0.0", - packageManagerString: "pnpm@9.0.0", + majorVersion: 8, + version: "8.0.0", + packageManagerString: "pnpm@8.0.0", }); - readWantedLockfile_v9.mockResolvedValue({ - lockfileVersion: "9.0", + readWantedLockfile_v8.mockResolvedValue({ + lockfileVersion: "6.0", patchedDependencies: { "lodash@4.17.21": { path: "patches/lodash.patch", hash: "sha256-def456", }, }, - } as unknown as Awaited>); + } as unknown as Awaited>); const result = await copyPatches({ workspaceRootDir: "/workspace", @@ -984,5 +991,7 @@ describe("copyPatches", () => { hash: "sha256-def456", }, }); + expect(readWantedLockfile_v8).toHaveBeenCalled(); + expect(readWantedLockfile_v9).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/patches/copy-patches.ts b/src/lib/patches/copy-patches.ts index d6dc0f8..b9bd446 100644 --- a/src/lib/patches/copy-patches.ts +++ b/src/lib/patches/copy-patches.ts @@ -176,9 +176,9 @@ export async function copyPatches({ } /** - * pnpm 11 simplified the lockfile `patchedDependencies` format from - * `Record` to `Record` (selector to - * hash), so the entry may be a bare hash string. See issue #201. + * Pnpm lockfile readers return either a bare hash string or an object with + * a path and hash. The pnpm 8 reader returns the object form, while the + * newer reader returns the bare hash form. */ const originalPatchFile = lockfilePatchResult?.patchedDependencies?.[packageSpec]; From 6f469eb897e1a4e72249ef2ba77a649b0e78bfea Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sat, 29 Aug 2026 14:11:51 +0200 Subject: [PATCH 4/4] Address final review findings --- src/lib/patches/copy-patches.test.ts | 134 +++++++++++++++------------ src/lib/patches/copy-patches.ts | 2 +- 2 files changed, 78 insertions(+), 58 deletions(-) diff --git a/src/lib/patches/copy-patches.test.ts b/src/lib/patches/copy-patches.test.ts index 25c3480..8b16be3 100644 --- a/src/lib/patches/copy-patches.test.ts +++ b/src/lib/patches/copy-patches.test.ts @@ -2,6 +2,18 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import type { PackageManifest, PnpmSettings } from "#/lib/types"; import { copyPatches } from "./copy-patches"; +const defaultPnpm9Lockfile = vi.hoisted(() => ({ + lockfileVersion: "9.0", + patchedDependencies: { + "@firebase/app@1.2.3": "sha256-firebase", + "lodash@4.17.21": "sha256-lodash", + "pkg-a@1.0.0": "sha256-pkg-a", + "pkg-b@1.0.0": "sha256-pkg-b", + "tslib@2.0.0": "sha256-tslib", + "vitest@1.0.0": "sha256-vitest", + }, +})); + /** Mock fs-extra */ vi.mock("fs-extra", () => ({ default: { @@ -51,19 +63,7 @@ vi.mock("pnpm_lockfile_file_v8", () => ({ })); vi.mock("pnpm_lockfile_file_v9", () => ({ - readWantedLockfile: vi.fn(() => - Promise.resolve({ - lockfileVersion: "9.0", - patchedDependencies: { - "@firebase/app@1.2.3": "sha256-firebase", - "lodash@4.17.21": "sha256-lodash", - "pkg-a@1.0.0": "sha256-pkg-a", - "pkg-b@1.0.0": "sha256-pkg-b", - "tslib@2.0.0": "sha256-tslib", - "vitest@1.0.0": "sha256-vitest", - }, - }), - ), + readWantedLockfile: vi.fn(() => Promise.resolve(defaultPnpm9Lockfile)), getLockfileImporterId: vi.fn( (root: string, dir: string) => dir.replace(`${root}/`, "") || ".", ), @@ -80,9 +80,19 @@ const { readWantedLockfile: readWantedLockfile_v9 } = vi.mocked( await import("pnpm_lockfile_file_v9"), ); +function resetPnpmLockfileReaderMocks() { + readWantedLockfile_v8.mockResolvedValue(null); + readWantedLockfile_v9.mockResolvedValue( + defaultPnpm9Lockfile as unknown as Awaited< + ReturnType + >, + ); +} + describe("copyPatches", () => { beforeEach(() => { vi.clearAllMocks(); + resetPnpmLockfileReaderMocks(); }); afterEach(() => { @@ -733,9 +743,9 @@ describe("copyPatches", () => { usePackageManager.mockReturnValue({ name: "pnpm", - majorVersion: 9, - version: "9.0.0", - packageManagerString: "pnpm@9.0.0", + majorVersion: 11, + version: "11.0.0", + packageManagerString: "pnpm@11.0.0", }); readWantedLockfile_v9.mockResolvedValue({ @@ -808,6 +818,10 @@ describe("copyPatches", () => { path: "patches/lodash.patch", hash: "sha256-lodash", }, + "underscore@1.13.7": { + path: "patches/underscore.patch", + hash: "", + }, }, } as unknown as Awaited>); } @@ -889,51 +903,57 @@ describe("copyPatches", () => { expect(fs.copy).not.toHaveBeenCalled(); }); - it("explains when pnpm 11 patch hashes cannot be read from the lockfile", async () => { - const targetManifest: PackageManifest = { - name: "test", - version: "1.0.0", - dependencies: { lodash: "^4.0.0" }, - }; + it.each([8, 9, 10, 11])( + "explains when pnpm %i patch hashes cannot be read from the lockfile", + async (majorVersion) => { + const targetManifest: PackageManifest = { + name: "test", + version: "1.0.0", + dependencies: { lodash: "^4.0.0" }, + }; - readTypedYamlSync.mockReturnValue({ - patchedDependencies: { + readTypedYamlSync.mockReturnValue({ + patchedDependencies: { + "lodash@4.17.21": "patches/lodash.patch", + }, + }); + readTypedJson.mockResolvedValue({ + name: "root", + version: "1.0.0", + } as PackageManifest); + filterPatchedDependencies.mockReturnValue({ "lodash@4.17.21": "patches/lodash.patch", - }, - }); - readTypedJson.mockResolvedValue({ - name: "root", - version: "1.0.0", - } as PackageManifest); - filterPatchedDependencies.mockReturnValue({ - "lodash@4.17.21": "patches/lodash.patch", - }); - fs.existsSync.mockReturnValue(true); - usePackageManager.mockReturnValue({ - name: "pnpm", - majorVersion: 11, - version: "11.0.0", - packageManagerString: "pnpm@11.0.0", - }); - readWantedLockfile_v9.mockRejectedValue(new Error("invalid lockfile")); + }); + fs.existsSync.mockReturnValue(true); + usePackageManager.mockReturnValue({ + name: "pnpm", + majorVersion, + version: `${majorVersion}.0.0`, + packageManagerString: `pnpm@${majorVersion}.0.0`, + }); + const readError = new Error("invalid lockfile"); + const readWantedLockfile = + majorVersion >= 9 ? readWantedLockfile_v9 : readWantedLockfile_v8; + readWantedLockfile.mockRejectedValue(readError); - await expect( - copyPatches({ - workspaceRootDir: "/workspace", - targetPackageDir: "/workspace/packages/test", - internalDepPackageNames: [], - targetPackageManifest: targetManifest, - isolateDir: "/workspace/isolate", - packagesRegistry: {}, - includeDevDependencies: false, - }), - ).rejects.toThrow( - "Could not read pnpm lockfile while resolving patch lodash@4.17.21", - ); + await expect( + copyPatches({ + workspaceRootDir: "/workspace", + targetPackageDir: "/workspace/packages/test", + internalDepPackageNames: [], + targetPackageManifest: targetManifest, + isolateDir: "/workspace/isolate", + packagesRegistry: {}, + includeDevDependencies: false, + }), + ).rejects.toThrow( + "Could not read pnpm lockfile while resolving patch lodash@4.17.21", + ); - expect(fs.ensureDir).not.toHaveBeenCalled(); - expect(fs.copy).not.toHaveBeenCalled(); - }); + expect(fs.ensureDir).not.toHaveBeenCalled(); + expect(fs.copy).not.toHaveBeenCalled(); + }, + ); it("should read the patch hash from the pnpm 8 object lockfile format (regression: issue #201)", async () => { const targetManifest: PackageManifest = { diff --git a/src/lib/patches/copy-patches.ts b/src/lib/patches/copy-patches.ts index b9bd446..56b2e8d 100644 --- a/src/lib/patches/copy-patches.ts +++ b/src/lib/patches/copy-patches.ts @@ -227,7 +227,7 @@ export async function copyPatches({ * Since the file content is the same after copying, the hash remains valid. * * The value type is `PatchFile | string` because pnpm 11 simplified the format - * to store the bare hash string per selector, while pnpm <=10 stored a + * to store the bare hash string per selector, while pnpm 8 stored a * `{ path, hash }` object (see issue #201). On the v9 path the string arrives * for a second reason as well: `@pnpm/lockfile.fs` migrates the object form to * a bare hash while reading, so only the v8 reader still yields an object.