From 9b229ecd0c75565eb2e01926d7f939f94cdec17e Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sun, 20 Sep 2026 21:42:14 +0200 Subject: [PATCH 1/3] Apply omitFromScripts to internal package manifests `omitFromScripts` only reached the target manifest. Internal package manifests got a hardcoded strip of `prepare` and never consulted the config, so no other lifecycle script could be removed from a vendored internal package. The case that breaks a deploy is `preinstall: npx only-allow pnpm`, which many pnpm monorepos put in every package. With `forceNpm: true` the isolated output is installed with npm on purpose, so the preserved guard is certain to fire and fail the install. `adaptInternalPackageManifests` now takes `omitFromScripts` and removes those names on top of the `prepare` strip it already did. An integration test isolates a workspace with an internal `preinstall` and asserts the vendored manifest loses it while keeping `postinstall`; the test fails without the wiring. Decisions: - `pickFromScripts` stays target-only. Treating it as an allowlist for internal manifests would silently drop their `postinstall` hooks for anyone already using it, reversing the stance taken in #169. - No `forceNpm` heuristic. Stripping `preinstall` automatically under `forceNpm` would guess at what a script does from its name; the config escape hatch covers the case without guessing. Closes #216 Scope: packages (isolate-package), docs Visibility: user-facing Implementer: claude:claude-opus-5:low --- docs/configuration.md | 25 +++-- src/isolate.integration.test.ts | 94 +++++++++++++++++++ src/isolate.ts | 1 + .../adapt-internal-package-manifests.test.ts | 67 +++++++++++++ .../adapt-internal-package-manifests.ts | 16 +++- 5 files changed, 194 insertions(+), 9 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index bc6ec73..8fee970 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -95,16 +95,25 @@ Select which scripts to omit from the output manifest `scripts` field. For example if the build script interferes with your deployment target, but you want to preserve all of the other scripts, set it to `["build"]`. -By default, all scripts are omitted, and the [pickFromScripts](#pickfromscripts) -configuration overrules this configuration. +By default, all scripts are omitted. For the target manifest the +[pickFromScripts](#pickfromscripts) configuration overrules this one, but +`omitFromScripts` still applies to internal dependencies when both are set. ::: info Scripts in internal dependencies -The `pickFromScripts` and `omitFromScripts` options only apply to the target -package manifest. Internal dependency manifests preserve their scripts by -default (e.g. for `postinstall` hooks like Prisma client generation), with one -exception: the `prepare` script is always stripped from internal dependencies -because it runs during `pnpm install` and typically depends on devDependency -binaries that are not available in the isolated output. +Internal dependency manifests preserve their scripts by default (e.g. for +`postinstall` hooks like Prisma client generation), with one exception. The +`prepare` script is always stripped from internal dependencies because it runs +during `pnpm install` and typically depends on devDependency binaries that are +not available in the isolated output. + +`omitFromScripts` applies to internal dependency manifests as well, on top of +that `prepare` strip. This is the way to remove a script the default cannot +anticipate. A `preinstall` with `npx only-allow pnpm` in every workspace +package, for example, fails the install when you deploy the output with npm, so +`omitFromScripts: ["preinstall"]` removes it everywhere. + +`pickFromScripts` is target-only. It does not turn into an allowlist for +internal dependencies, which would silently drop their `postinstall` hooks. ::: ### omitPackageManager diff --git a/src/isolate.integration.test.ts b/src/isolate.integration.test.ts index ff71017..e93e642 100644 --- a/src/isolate.integration.test.ts +++ b/src/isolate.integration.test.ts @@ -71,6 +71,100 @@ describe("isolate integration", () => { ).resolves.toMatchObject({ name: "functions-node", version: "1.0.0" }); }); + it("omits configured scripts from internal package manifests", async () => { + const workspaceRoot = await fs.mkdtemp( + path.join(os.tmpdir(), "isolate-omit-internal-scripts-test-"), + ); + temporaryDirectories.push(workspaceRoot); + const targetPackageDir = path.join(workspaceRoot, "packages", "functions"); + const sharedPackageDir = path.join(workspaceRoot, "packages", "shared"); + await fs.ensureDir(targetPackageDir); + await fs.ensureDir(sharedPackageDir); + await fs.writeJson(path.join(workspaceRoot, "package.json"), { + name: "workspace", + version: "1.0.0", + private: true, + workspaces: ["packages/*"], + }); + await fs.writeJson(path.join(workspaceRoot, "package-lock.json"), { + name: "workspace", + version: "1.0.0", + lockfileVersion: 3, + requires: true, + packages: { + "": { + name: "workspace", + version: "1.0.0", + workspaces: ["packages/*"], + }, + "node_modules/shared": { + resolved: "packages/shared", + link: true, + }, + "packages/functions": { + name: "functions", + version: "1.0.0", + dependencies: { shared: "1.0.0" }, + }, + "packages/shared": { + name: "shared", + version: "1.0.0", + }, + }, + }); + await fs.writeJson(path.join(targetPackageDir, "package.json"), { + name: "functions", + version: "1.0.0", + main: "./index.js", + files: ["index.js"], + scripts: { + preinstall: "echo target-preinstall", + build: "echo target-build", + }, + dependencies: { shared: "1.0.0" }, + }); + await fs.writeFile(path.join(targetPackageDir, "index.js"), "export {};\n"); + await fs.writeJson(path.join(sharedPackageDir, "package.json"), { + name: "shared", + version: "1.0.0", + main: "./index.js", + files: ["index.js"], + scripts: { + preinstall: "echo shared-preinstall", + prepare: "echo shared-prepare", + postinstall: "echo shared-postinstall", + build: "echo shared-build", + }, + }); + await fs.writeFile(path.join(sharedPackageDir, "index.js"), "export {};\n"); + + const isolateDir = await isolate({ + targetPackagePath: targetPackageDir, + buildDirName: ".", + workspaceRoot: "../..", + omitFromScripts: ["preinstall"], + }); + + const sharedManifest = await fs.readJson( + path.join(isolateDir, "packages", "shared", "package.json"), + ); + + /** + * The target keeps whatever `omitFromScripts` leaves, and the internal + * package loses `preinstall` on top of the always-stripped `prepare`. + */ + expect(sharedManifest.scripts).toEqual({ + postinstall: "echo shared-postinstall", + build: "echo shared-build", + }); + + const targetManifest = await fs.readJson( + path.join(isolateDir, "package.json"), + ); + + expect(targetManifest.scripts).toEqual({ build: "echo target-build" }); + }); + it.each([10, 11])( "writes pnpm %i patch settings to the lockfile and workspace configuration", async (majorVersion) => { diff --git a/src/isolate.ts b/src/isolate.ts index 88e7c0f..57e1a97 100644 --- a/src/isolate.ts +++ b/src/isolate.ts @@ -277,6 +277,7 @@ export function createIsolator(initialConfig?: IsolateConfig) { packagesRegistry, isolateDir, forceNpm: config.forceNpm, + omitFromScripts: config.omitFromScripts, workspaceRootDir, }); diff --git a/src/lib/manifest/helpers/adapt-internal-package-manifests.test.ts b/src/lib/manifest/helpers/adapt-internal-package-manifests.test.ts index cbe5e9b..8b28d60 100644 --- a/src/lib/manifest/helpers/adapt-internal-package-manifests.test.ts +++ b/src/lib/manifest/helpers/adapt-internal-package-manifests.test.ts @@ -148,6 +148,73 @@ describe("adaptInternalPackageManifests", () => { }); }); + it("should omit the configured scripts alongside prepare", async () => { + const manifest: PackageManifest = { + name: "@repo/common", + version: "1.0.0", + scripts: { + preinstall: "npx only-allow pnpm", + prepare: "tsdown", + postinstall: "prisma generate", + build: "tsdown", + }, + }; + + const packagesRegistry = createRegistry({ + "@repo/common": { + rootRelativeDir: "packages/common", + manifest, + }, + }); + + await adaptInternalPackageManifests({ + internalPackageNames: ["@repo/common"], + packagesRegistry, + isolateDir: "/output", + forceNpm: true, + omitFromScripts: ["preinstall", "build"], + workspaceRootDir: "/workspace", + }); + + const writtenManifest = writeManifest.mock.calls[0]![1]; + + expect(writtenManifest.scripts).toEqual({ + postinstall: "prisma generate", + }); + }); + + it("should leave scripts untouched when omitFromScripts lists an absent name", async () => { + const manifest: PackageManifest = { + name: "@repo/common", + version: "1.0.0", + scripts: { + postinstall: "prisma generate", + }, + }; + + const packagesRegistry = createRegistry({ + "@repo/common": { + rootRelativeDir: "packages/common", + manifest, + }, + }); + + await adaptInternalPackageManifests({ + internalPackageNames: ["@repo/common"], + packagesRegistry, + isolateDir: "/output", + forceNpm: false, + omitFromScripts: ["preinstall"], + workspaceRootDir: "/workspace", + }); + + const writtenManifest = writeManifest.mock.calls[0]![1]; + + expect(writtenManifest.scripts).toEqual({ + postinstall: "prisma generate", + }); + }); + it("should strip devDependencies from internal dependency manifests", async () => { const manifest: PackageManifest = { name: "@repo/shared", diff --git a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts index 5c7d3ac..805b539 100644 --- a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts +++ b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts @@ -17,12 +17,18 @@ export async function adaptInternalPackageManifests({ packagesRegistry, isolateDir, forceNpm, + omitFromScripts, workspaceRootDir, }: { internalPackageNames: string[]; packagesRegistry: PackagesRegistry; isolateDir: string; forceNpm: boolean; + /** + * Script names from `omitFromScripts` in the config, removed on top of the + * `prepare` script that is always stripped. + */ + omitFromScripts?: string[]; workspaceRootDir: string; }) { const packageManager = usePackageManager(); @@ -49,9 +55,17 @@ export async function adaptInternalPackageManifests({ * which are not available in the isolated output. Other lifecycle * scripts like `postinstall` are preserved because they handle runtime * setup (e.g. Prisma client generation). + * + * Anything listed in `omitFromScripts` is removed as well, which is the + * escape hatch for scripts the default cannot anticipate — a + * `preinstall: npx only-allow pnpm` guard, for instance, always fails + * the install when the output is deployed with npm (see issue #216). */ if (strippedManifest.scripts) { - strippedManifest.scripts = omit(strippedManifest.scripts, ["prepare"]); + strippedManifest.scripts = omit(strippedManifest.scripts, [ + "prepare", + ...(omitFromScripts ?? []), + ]); } const preparedManifest = isPnpmWorkspaceOutput From 857f515dd64395b8e3d45fad97b952526e12e905 Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sun, 20 Sep 2026 22:01:12 +0200 Subject: [PATCH 2/3] Address review findings (pack-time scope, pick+omit test, comments) `omitFromScripts` rewrites the output manifests only. Packing runs first against each package's original manifest, so `prepare`, `prepack` and `postpack` have already executed by then and listing one of the three does not stop it. A validator reproduced the case: a failing `prepack` in an internal package rejects during `npm pack`, before the omission runs. Documented in both the configuration page and the helper, rather than passing `--ignore-scripts` to pack, which would change behaviour for every existing user. The integration test now also covers the reporter's own configuration, which sets `pickFromScripts` and `omitFromScripts` together: the target follows pick, dropping a `test` script that omission alone keeps, while the internal manifest still applies the omit list. Nothing failed if the isolator stopped forwarding the omit list whenever pick was present. Dropped two comments that restated the code they sat above. Co-Authored-By: Claude Opus 5 (1M context) --- docs/configuration.md | 6 +++ src/isolate.integration.test.ts | 39 ++++++++++++++++--- .../adapt-internal-package-manifests.ts | 8 ++-- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 8fee970..aa0cc84 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -114,6 +114,12 @@ package, for example, fails the install when you deploy the output with npm, so `pickFromScripts` is target-only. It does not turn into an allowlist for internal dependencies, which would silently drop their `postinstall` hooks. + +Both options rewrite the manifests in the output. They do not affect the packing +step that collects each internal package, and `npm pack` and `pnpm pack` run +`prepare`, `prepack` and `postpack` from the package's original manifest. Listing +one of those three names does not stop it running during isolation, so a +`prepack` that fails still fails the isolate. ::: ### omitPackageManager diff --git a/src/isolate.integration.test.ts b/src/isolate.integration.test.ts index e93e642..9af9587 100644 --- a/src/isolate.integration.test.ts +++ b/src/isolate.integration.test.ts @@ -120,6 +120,7 @@ describe("isolate integration", () => { scripts: { preinstall: "echo target-preinstall", build: "echo target-build", + test: "echo target-test", }, dependencies: { shared: "1.0.0" }, }); @@ -149,10 +150,6 @@ describe("isolate integration", () => { path.join(isolateDir, "packages", "shared", "package.json"), ); - /** - * The target keeps whatever `omitFromScripts` leaves, and the internal - * package loses `preinstall` on top of the always-stripped `prepare`. - */ expect(sharedManifest.scripts).toEqual({ postinstall: "echo shared-postinstall", build: "echo shared-build", @@ -162,7 +159,39 @@ describe("isolate integration", () => { path.join(isolateDir, "package.json"), ); - expect(targetManifest.scripts).toEqual({ build: "echo target-build" }); + expect(targetManifest.scripts).toEqual({ + build: "echo target-build", + test: "echo target-test", + }); + + /** + * The reporter's own configuration in #216 sets both options. The target + * follows `pickFromScripts`, which drops `test` that omission alone kept, + * while the internal manifest independently applies `omitFromScripts` — + * so giving `pickFromScripts` precedence for the target must not stop the + * omit list from reaching the internal packages. + */ + const pickIsolateDir = await isolate({ + targetPackagePath: targetPackageDir, + buildDirName: ".", + workspaceRoot: "../..", + isolateDirName: "isolate-pick", + pickFromScripts: ["build"], + omitFromScripts: ["preinstall"], + }); + + const pickTargetManifest = await fs.readJson( + path.join(pickIsolateDir, "package.json"), + ); + const pickSharedManifest = await fs.readJson( + path.join(pickIsolateDir, "packages", "shared", "package.json"), + ); + + expect(pickTargetManifest.scripts).toEqual({ build: "echo target-build" }); + expect(pickSharedManifest.scripts).toEqual({ + postinstall: "echo shared-postinstall", + build: "echo shared-build", + }); }); it.each([10, 11])( diff --git a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts index 805b539..deadd23 100644 --- a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts +++ b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts @@ -24,10 +24,6 @@ export async function adaptInternalPackageManifests({ packagesRegistry: PackagesRegistry; isolateDir: string; forceNpm: boolean; - /** - * Script names from `omitFromScripts` in the config, removed on top of the - * `prepare` script that is always stripped. - */ omitFromScripts?: string[]; workspaceRootDir: string; }) { @@ -60,6 +56,10 @@ export async function adaptInternalPackageManifests({ * escape hatch for scripts the default cannot anticipate — a * `preinstall: npx only-allow pnpm` guard, for instance, always fails * the install when the output is deployed with npm (see issue #216). + * + * This rewrites the output manifest only. The packing step ran earlier + * against the original manifest, so `prepare`, `prepack` and `postpack` + * have already executed by the time we get here and cannot be omitted. */ if (strippedManifest.scripts) { strippedManifest.scripts = omit(strippedManifest.scripts, [ From 66ca5ab413a7b15629d77b2e42cca81ec6ac120e Mon Sep 17 00:00:00 2001 From: Thijs Koerselman Date: Sun, 20 Sep 2026 22:14:54 +0200 Subject: [PATCH 3/3] Address codex-review findings (comment scope, split script tests) Dropped the comment paragraph that restated the omit call; the packing invariant above it is the part a reader cannot get from the code, and the `only-allow pnpm` motivation belongs in the configuration page, where it already is. Cut the test comment down to the invariant it was narrating around. Splitting the two script cases into their own tests came out of the first: running `isolate` twice in one case pushed it past vitest's 5s default under full-suite load. Each now runs once over a shared fixture helper, with an explicit timeout because both shell out to `npm pack`. Co-Authored-By: Claude Opus 5 (1M context) --- src/isolate.integration.test.ts | 115 ++++++++++-------- .../adapt-internal-package-manifests.ts | 5 - 2 files changed, 67 insertions(+), 53 deletions(-) diff --git a/src/isolate.integration.test.ts b/src/isolate.integration.test.ts index 9af9587..d594404 100644 --- a/src/isolate.integration.test.ts +++ b/src/isolate.integration.test.ts @@ -8,6 +8,8 @@ import { isolate } from "./isolate"; describe("isolate integration", () => { const temporaryDirectories: string[] = []; + /** Each case shells out to `npm pack`, which outlasts vitest's 5s default */ + const ISOLATE_TEST_TIMEOUT = 30_000; const leftPadPatchContents = "diff --git a/index.js b/index.js\n"; const leftPadPatchHash = "2692094a267de7e28825147fd6cb2ebde098a4e68c25dfa3976ac806f4a1a784"; @@ -71,7 +73,11 @@ describe("isolate integration", () => { ).resolves.toMatchObject({ name: "functions-node", version: "1.0.0" }); }); - it("omits configured scripts from internal package manifests", async () => { + /** + * An npm workspace whose target and internal package both carry lifecycle + * scripts, so a test can assert which ones survive into the output. + */ + async function createScriptsWorkspace() { const workspaceRoot = await fs.mkdtemp( path.join(os.tmpdir(), "isolate-omit-internal-scripts-test-"), ); @@ -139,60 +145,73 @@ describe("isolate integration", () => { }); await fs.writeFile(path.join(sharedPackageDir, "index.js"), "export {};\n"); - const isolateDir = await isolate({ - targetPackagePath: targetPackageDir, - buildDirName: ".", - workspaceRoot: "../..", - omitFromScripts: ["preinstall"], - }); + return targetPackageDir; + } - const sharedManifest = await fs.readJson( - path.join(isolateDir, "packages", "shared", "package.json"), - ); + it( + "omits configured scripts from internal package manifests", + async () => { + const targetPackageDir = await createScriptsWorkspace(); - expect(sharedManifest.scripts).toEqual({ - postinstall: "echo shared-postinstall", - build: "echo shared-build", - }); + const isolateDir = await isolate({ + targetPackagePath: targetPackageDir, + buildDirName: ".", + workspaceRoot: "../..", + omitFromScripts: ["preinstall"], + }); - const targetManifest = await fs.readJson( - path.join(isolateDir, "package.json"), - ); + const sharedManifest = await fs.readJson( + path.join(isolateDir, "packages", "shared", "package.json"), + ); + const targetManifest = await fs.readJson( + path.join(isolateDir, "package.json"), + ); - expect(targetManifest.scripts).toEqual({ - build: "echo target-build", - test: "echo target-test", - }); + expect(sharedManifest.scripts).toEqual({ + postinstall: "echo shared-postinstall", + build: "echo shared-build", + }); + expect(targetManifest.scripts).toEqual({ + build: "echo target-build", + test: "echo target-test", + }); + }, + ISOLATE_TEST_TIMEOUT, + ); - /** - * The reporter's own configuration in #216 sets both options. The target - * follows `pickFromScripts`, which drops `test` that omission alone kept, - * while the internal manifest independently applies `omitFromScripts` — - * so giving `pickFromScripts` precedence for the target must not stop the - * omit list from reaching the internal packages. - */ - const pickIsolateDir = await isolate({ - targetPackagePath: targetPackageDir, - buildDirName: ".", - workspaceRoot: "../..", - isolateDirName: "isolate-pick", - pickFromScripts: ["build"], - omitFromScripts: ["preinstall"], - }); + it( + "applies omitFromScripts to internal packages when pickFromScripts is also set", + async () => { + const targetPackageDir = await createScriptsWorkspace(); - const pickTargetManifest = await fs.readJson( - path.join(pickIsolateDir, "package.json"), - ); - const pickSharedManifest = await fs.readJson( - path.join(pickIsolateDir, "packages", "shared", "package.json"), - ); + const isolateDir = await isolate({ + targetPackagePath: targetPackageDir, + buildDirName: ".", + workspaceRoot: "../..", + pickFromScripts: ["build"], + omitFromScripts: ["preinstall"], + }); - expect(pickTargetManifest.scripts).toEqual({ build: "echo target-build" }); - expect(pickSharedManifest.scripts).toEqual({ - postinstall: "echo shared-postinstall", - build: "echo shared-build", - }); - }); + const targetManifest = await fs.readJson( + path.join(isolateDir, "package.json"), + ); + const sharedManifest = await fs.readJson( + path.join(isolateDir, "packages", "shared", "package.json"), + ); + + /** + * `pickFromScripts` is target-only, so it drops the target's `test` + * script that omission alone keeps, while `omitFromScripts` still has to + * reach the internal package. + */ + expect(targetManifest.scripts).toEqual({ build: "echo target-build" }); + expect(sharedManifest.scripts).toEqual({ + postinstall: "echo shared-postinstall", + build: "echo shared-build", + }); + }, + ISOLATE_TEST_TIMEOUT, + ); it.each([10, 11])( "writes pnpm %i patch settings to the lockfile and workspace configuration", diff --git a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts index deadd23..55b42fc 100644 --- a/src/lib/manifest/helpers/adapt-internal-package-manifests.ts +++ b/src/lib/manifest/helpers/adapt-internal-package-manifests.ts @@ -52,11 +52,6 @@ export async function adaptInternalPackageManifests({ * scripts like `postinstall` are preserved because they handle runtime * setup (e.g. Prisma client generation). * - * Anything listed in `omitFromScripts` is removed as well, which is the - * escape hatch for scripts the default cannot anticipate — a - * `preinstall: npx only-allow pnpm` guard, for instance, always fails - * the install when the output is deployed with npm (see issue #216). - * * This rewrites the output manifest only. The packing step ran earlier * against the original manifest, so `prepare`, `prepack` and `postpack` * have already executed by the time we get here and cannot be omitted.