[BUG] exit codes not being set properly in node v20
ericcornelissen opened this issue · 10 comments
Is there an existing issue for this?
- I have searched the existing issues
This issue exists in the latest npm version
- I am using the latest npm
Current Behavior
Running npm audit
(seemingly) always exists with a 0 exit code, even if a vulnerability is found (with a severity higher than configured by the audit-level
).
In contrast, npm@8.15.0
(node@18.7.0
) exits with a 1 if a vulnerability is found (with a severity higher than configured by the audit-level
).
Expected Behavior
Per the Exit Code section:
[...]
If vulnerabilities were found the exit code will depend on the
audit-level
config.
Steps To Reproduce
- Clone https://github.com/ericcornelissen/shescape/tree/dbaa0fd36af4fd0439af87548ce710468f25cb18
- Run
npm audit
- Observe a warning for (at least) the high severity vulnerability GHSA-9c47-m6qq-7p4h
- Run
echo $?
- Observe a
0
being printed
Environment
- npm: 9.6.5
- Node.js: v20.0.0
- OS Name: Ubuntu 22.04.2 LTS
- System Model Name: custom
- npm config:
; "user" config from ~/.npmrc
update-notifier = false
; "project" config from ~/workspace/shescape/.npmrc
lockfile-version = "3"
save-exact = true
save-prefix = ""
; node bin location = ~/.nvm/versions/node/v20.0.0/bin/node
; node version = v20.0.0
; npm local prefix = ~/Documents/workspace/shescape
; npm version = 9.6.5
; cwd = ~/Documents/workspace/shescape
; HOME = ~
; Run `npm config ls -l` to show all defaults.
The same happens when running workspace scripts: npm run build -w test
If the script fails, npm still returns with exit code 0.
I have made a reproduction repository: https://github.com/darthmaim/node-exit-code-test
I also added a workflow to test multiple node/npm versions: https://github.com/darthmaim/node-exit-code-test/actions/runs/4837883808
It only fails on node 20
This probably breaks all CI jobs when using worskpaces and node 20, so maybe this should be higher priority?
Hi - thanks for looking into this.
We're using workspaces, npm 9.6.6 and node 20.1.0 and this is currently breaking our CI jobs
This should be related to this changes on node side nodejs/node#48027
process.exit
will be called with undefined
here:
Lines 220 to 223 in 829503b
process.exitCode
set earlier
cli/lib/commands/run-script.js
Lines 200 to 222 in 09b58e4
@MichaelBitard the previous comment shows the root cause here. v20 subtly changed how exitCode setting works.
The process.exit
in the exit handler needs to be smarter about either not passing anything and setting process.exitCode if it needs to, or casting that to 0
if it's not already set.
Ok thanks, from what I see err
is always undefined in our cases (running a script that fails in a workspace), but process.exitCode is set to a non 0 value.
I should rely on it.
If I change line 134:
Lines 130 to 140 in 829503b
to let exitCode = process.exitCode
then I get the correct behavior.
This seems ok to you?
process.exitCode || 1
would ensure it’s always nonzero, which seems like what’s desired.
I think you mean process.exitCode || 0
because it should exit gracefully on most occasions no? or is exitHandler always called on error?
oh, if it’s ok to sometimes have zero then your original suggestion is what’s wanted, i was just matching the current semantics,
Yes I think that's it, if everything works well, then exitHandler is called and process.exitCode is undefined
, so we don't want to force it to 1 here