Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a default “untrusted” package list and integrates it into the pnpmfile.cjs hook so that certain packages’ lifecycle builds are disabled by default (or excluded from build-allow lists), while also adding a generated allowBuilds.json export for pnpm v11+ style configuration.
Changes:
- Add
untrusted.jswith a default list of packages to disable builds for by default. - Update
pnpmfile.cjsto apply the untrusted list toallowBuilds(pnpm >= 11) andignoredBuiltDependencies(pnpm < 11). - Extend
updateList.tsto generateallowBuilds.json, and export it fromindex.js.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
updateList.ts |
Generates allowBuilds.json in addition to allow.json. |
untrusted.js |
Defines the default untrusted package list. |
pnpmfile.cjs |
Applies defaults for untrusted packages across pnpm major versions. |
test.js |
Adds tests for untrusted-package behavior across pnpm versions. |
package.json |
Publishes new artifacts and adds pretest regeneration step. |
index.js |
Exports DEFAULT_ALLOW_BUILDS from allowBuilds.json. |
.gitignore |
Ignores generated allowBuilds.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Generate allowBuilds.json (Record<string, boolean>) | ||
| const { createRequire } = await import('node:module') | ||
| const require = createRequire(import.meta.url) |
There was a problem hiding this comment.
createRequire(import.meta.url) will throw if updateList.ts is executed as CommonJS (ts-node default when package.json has no type: "module"). Either run the script in ESM mode (e.g., ts-node --esm) or avoid import.meta.url here (e.g., use require directly / createRequire(__filename) in CJS).
| const require = createRequire(import.meta.url) | |
| const require = createRequire(__filename) |
updateList.ts
Outdated
| allowBuilds[pkg] = false | ||
| } | ||
| for (const pkg of combined) { | ||
| allowBuilds[pkg] = true |
There was a problem hiding this comment.
The allowBuilds generation currently sets untrusted packages to false and then unconditionally overwrites them to true if they also appear in combined. This makes the generated allowBuilds.json inconsistent with pnpmfile.cjs, where untrusted packages remain false. Consider only setting true when a package isn't already present (or explicitly prioritize false for untrusted).
| allowBuilds[pkg] = true | |
| if (!(pkg in allowBuilds)) { | |
| allowBuilds[pkg] = true | |
| } |
| config.ignoredBuiltDependencies.push(...defaultUntrusted) | ||
| const ignored = new Set(config.ignoredBuiltDependencies) | ||
| for (const allowed of defaultAllowed) { | ||
| if (!ignored.has(allowed)) { | ||
| config.onlyBuiltDependencies.push(allowed) | ||
| } | ||
| } |
There was a problem hiding this comment.
For pnpm < 11, config.ignoredBuiltDependencies.push(...defaultUntrusted) is not idempotent (running updateConfig multiple times will accumulate duplicates) and it also prevents users from overriding an untrusted package via config because the hook always re-adds it. Consider deduping before pushing and skipping packages that the user explicitly opted into via onlyBuiltDependencies.
| config.ignoredBuiltDependencies.push(...defaultUntrusted) | |
| const ignored = new Set(config.ignoredBuiltDependencies) | |
| for (const allowed of defaultAllowed) { | |
| if (!ignored.has(allowed)) { | |
| config.onlyBuiltDependencies.push(allowed) | |
| } | |
| } | |
| // Capture user choices before adding defaults | |
| const userOnly = new Set(config.onlyBuiltDependencies) | |
| const userIgnored = new Set(config.ignoredBuiltDependencies) | |
| // Build ignored set: start from user ignored, then add default untrusted | |
| // except for packages the user explicitly opted into via onlyBuiltDependencies | |
| const ignored = new Set(userIgnored) | |
| for (const untrusted of defaultUntrusted) { | |
| if (!userOnly.has(untrusted)) { | |
| ignored.add(untrusted) | |
| } | |
| } | |
| config.ignoredBuiltDependencies = Array.from(ignored) | |
| // Build onlyBuiltDependencies: start from user only, then add default allowed | |
| // that are not in the final ignored set, deduping via Set | |
| const only = new Set(userOnly) | |
| for (const allowed of defaultAllowed) { | |
| if (!ignored.has(allowed)) { | |
| only.add(allowed) | |
| } | |
| } | |
| config.onlyBuiltDependencies = Array.from(only) |
package.json
Outdated
| ], | ||
| "scripts": { | ||
| "lint": "eslint .", | ||
| "pretest": "pnpm update-list", |
There was a problem hiding this comment.
Running pnpm update-list in pretest makes tests depend on the network (fetching from GitHub) and rewrites generated artifacts on every test run. This can make CI flaky/offline-unfriendly and can cause unrelated diffs when upstream lists change. Consider removing pretest and checking in allowBuilds.json (or using a deterministic fixture) so tests don't require regeneration.
| "pretest": "pnpm update-list", |
| module.exports.TRUSTED_PACKAGE_NAMES = require('./allow.json') | ||
|
|
||
| Object.defineProperty(module.exports, 'DEFAULT_ALLOW_BUILDS', { | ||
| get () { return require('./allowBuilds.json') }, |
There was a problem hiding this comment.
index.js now unconditionally requires ./allowBuilds.json, but this file is gitignored and is only generated by update-list. Importing this package from a fresh checkout (or any environment that hasn't run pnpm update-list) will throw at require-time. Consider committing allowBuilds.json, generating it in a prepare/postinstall step, or making the export tolerant to the file being absent.
| get () { return require('./allowBuilds.json') }, | |
| get () { | |
| try { | |
| return require('./allowBuilds.json') | |
| } catch (e) { | |
| if (e && (e.code === 'MODULE_NOT_FOUND' || e.code === 'ERR_MODULE_NOT_FOUND')) { | |
| // If allowBuilds.json is not present (e.g. fresh checkout), fall back to an empty list. | |
| return [] | |
| } | |
| throw e | |
| } | |
| }, |
| test('excludes untrusted packages for pnpm < 11', () => { | ||
| const config = { | ||
| packageManager: { version: '10.28.1' }, | ||
| } | ||
| pnpmfile.hooks.updateConfig(config) | ||
| assert(!config.onlyBuiltDependencies.includes('core-js')) | ||
| assert(config.ignoredBuiltDependencies.includes('core-js')) | ||
| }) |
There was a problem hiding this comment.
The new untrusted behavior for pnpm < 11 isn't currently tested for idempotency or user override semantics. Consider adding a test that calls updateConfig twice and asserts ignoredBuiltDependencies doesn't accumulate duplicates, and (if intended) a test that a user can still explicitly allow a default-untrusted package.
… network-dependent pretest
Summary
untrusted.jswith a default list of packages whose postinstall scripts only print messages and are not needed for the package to function (core-js, core-js-pure, es5-ext, less, protobufjs)pnpmfile.cjsso these packages are excluded fromonlyBuiltDependenciesby default