Skip to content

fix: honor --dry-run in install-scripts approve and deny - #9993

Open
abkrim wants to merge 1 commit into
npm:latestfrom
abkrim:fix/install-scripts-dry-run
Open

abkrim wants to merge 1 commit into
npm:latestfrom
abkrim:fix/install-scripts-dry-run

Conversation

@abkrim

@abkrim abkrim commented Sep 16, 2026

Copy link
Copy Markdown

Closes #9992.

npm install-scripts approve <pkg> --dry-run and deny <pkg> --dry-run wrote the allowScripts entry to package.json and printed the summary in the past tense, worded identically to a real run. Both flags are documented for the command, so a preview was performing the write and giving no sign it had.

The sibling prune subcommand already gets this right, which is what made the gap look like an oversight rather than a design choice:

$ npm install-scripts prune --dry-run
Would remove 1 unused allowScripts entry:
  not-installed-pkg@9.9.9 (package not installed)

writePolicyChanges() never read the flag; runPrune() does. This reads it in the same place, skips pkg.save() when it is set, and selects the conditional wording so the summary describes what actually happened.

Changes

  • lib/utils/allow-scripts-cmd.js: read dry-run in writePolicyChanges(), guard the save, pass the flag to printSummary(), and let the summary choose Would approve / Would deny over the past tense. The --json payload gains a dryRun key, mirroring the prune JSON so machine consumers can distinguish a preview from an applied change.
  • test/lib/commands/install-scripts.js: four tests covering approve <pkg>, approve --all, approve --json and deny <pkg> under --dry-run.

Testing

Before the fix the four new tests fail (4 of 27 in that file). After it, test/lib/commands/install-scripts.js passes 27/27 and the full suite passes 123/123 files:

$ node_modules/.bin/tap --no-coverage
1..123
# time=27887.787ms

Coverage for the modified file is 100% statements, branches, functions and lines:

$ node_modules/.bin/tap --coverage --coverage-report=text test/lib/commands/{install-scripts,approve-scripts,deny-scripts}.js test/lib/utils/allow-scripts-prune.js
  allow-scripts-cmd.js       |     100 |      100 |     100 |     100 |

End to end, against the same reproduction as the issue:

# with the fix
$ npm install-scripts approve simple-git-hooks --dry-run
Would approve simple-git-hooks:
  added simple-git-hooks@2.14.0
$ cat package.json          # allowScripts absent — untouched

# without the flag, unchanged behaviour
$ npm install-scripts approve simple-git-hooks
Approved simple-git-hooks:
  added simple-git-hooks@2.14.0
$ cat package.json          # allowScripts written, as before

The standalone approve-scripts / deny-scripts aliases share this code path and now honor the flag as well.

`npm install-scripts approve <pkg> --dry-run` and `deny <pkg> --dry-run`
wrote the `allowScripts` entry to package.json instead of previewing it,
and printed the summary in the past tense, worded identically to a real
run. The same applied to `--all`.

`writePolicyChanges()` never read the flag. Its sibling `runPrune()`
does: it reads `dry-run`, skips the save, and reports "Would remove ..."
rather than "Removed ...". Read the flag in the shared writer too, skip
`pkg.save()` when it is set, and pick the conditional wording so the
summary describes what actually happened.

The `--json` summary now carries a `dryRun` key, mirroring the prune
summary, so machine consumers can tell a preview from an applied change.

Co-Authored-By: pi 0.85.1 (deepseek-v4-flash) <noreply@earendil.works>
@abkrim
abkrim requested a review from a team as a code owner September 16, 2026 17:18

@Rocin1738 Rocin1738 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to say I take for responsibility there's no excuses so don't matter once again thank you I appreciate you guys for giving me a chance

printSummary (summary) {
printSummary (summary, { dryRun }) {
if (this.npm.flatOptions.json) {
output.buffer({ allowScripts: summary })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowScripts: summary }) [%2]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] npm install-scripts approve/deny ignore --dry-run and write to package.json

2 participants