Repository navigation
Apply omitFromScripts to internal package manifests - #217
Merged
Merged
Conversation
`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
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is consistent with the stated behavior and includes adequate unit, integration, and documentation updates.
Review effort: Balanced
Findings: None
What changed in this PR
Extends omitFromScripts to vendored internal packages, preventing incompatible lifecycle scripts from breaking isolated deployments.
Changes:
- Applies configured script omissions while retaining the default
prepareremoval. - Adds unit and integration coverage, including interaction with
pickFromScripts. - Documents internal-package scope and pack-time limitations.
| File | Description |
|---|---|
src/lib/manifest/helpers/adapt-internal-package-manifests.ts |
Omits configured scripts from internal manifests. |
src/lib/manifest/helpers/adapt-internal-package-manifests.test.ts |
Adds focused unit coverage. |
src/isolate.ts |
Passes configuration into internal manifest adaptation. |
src/isolate.integration.test.ts |
Verifies end-to-end behavior and precedence. |
docs/configuration.md |
Documents scope and lifecycle limitations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
omitFromScriptsonly reached the target manifest. Internal package manifests got a hardcoded strip ofprepareand 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. WithforceNpm: truethe isolated output is installed with npm on purpose, so the preserved guard is certain to fire and fail the install. SettingomitFromScripts: ["preinstall"]changed nothing, because the list never reached those manifests.adaptInternalPackageManifestsnow takesomitFromScriptsand removes those names on top of thepreparestrip it already did. The configuration page documents the new scope, and also a limit the review turned up: both options rewrite the output manifests, while packing runs earlier against each package's original manifest, soprepare,prepackandpostpackhave already executed by then and listing one of the three cannot stop it.Coverage runs at two levels. Two unit tests cover the omission alongside
prepareand the no-op when a listed name is absent. Two integration tests isolate a real npm workspace whose internal package carriespreinstall,prepare,postinstallandbuild. One asserts the vendored manifest losespreinstalland keepspostinstall, the other setspickFromScriptsandomitFromScriptstogether, which is the reporter's own configuration, and pins that pick taking precedence for the target does not stop the omit list from reaching the internal packages. Both fail without the wiring.Decisions:
pickFromScriptsstays target-only. Treating it as an allowlist for internal manifests would silently drop theirpostinstallhooks for anyone already using it, reversing the stance taken in Scripts are preserved unexpectedly #169.forceNpmheuristic. Strippingpreinstallautomatically when the output is installed by npm would guess at what a script does from its name, so the aside in omitFromScripts does not reach internal package manifests, so a preinstall guard cannot be removed #216 is declined. The config option covers the case without guessing.--ignore-scriptsto pack. Suppressing pack scripts would change behaviour for every existing user, including packages that legitimately build inprepack.isolatetwice in one case pushed it past vitest's 5s default under full-suite load, so each now runs once over a shared fixture helper.Closes #216
Scope: packages (isolate-package), docs
Visibility: user-facing
Implementer: claude:claude-opus-5:low