pre-commit treats hook commands dying due to POSIX signal as successful

[chriskuehl]

search you tried in the issue tracker

signal

describe your issue

When a hook command dies due to an unhandled signal, pre-commit treats the hook as successful.

You can see this for example with a node process that crashes due to hitting a memory limit (it signals itself with SIGTRACE):

$ pre-commit run --all-files --verbose node-crash
node-crash...............................................................
Passed
- hook id: node-crash
- duration: 0.02s

<--- Last few GCs --->


<--- JS stacktrace --->


#
# Fatal javascript OOM in GC during deserialization
#

$ echo $?
0

I've also included a simple kill-with-term hook in the attached config that reproduces it using only bash.

This happens because CompletedProcess.returncode will be a negative integer (e.g. -15 for SIGTERM) but pre-commit's returncode merging uses max() to collate the returncodes from the processes it spawns:

pre-commit/pre_commit/xargs.py

Lines 172 to 173 in a1f1d19

	for proc_retcode, proc_out, _ in results:
	retcode = max(retcode, proc_retcode)

A draft fix like this resolves the issue for me and correctly shows the hooks as failing:

--- venv/lib/python3.10/site-packages/pre_commit/xargs.py.orig  2023-08-21 16:19:25.351609037 -0700
+++ venv/lib/python3.10/site-packages/pre_commit/xargs.py       2023-08-21 16:19:31.183503472 -0700
@@ -170,7 +170,7 @@
         results = thread_map(run_cmd_partition, partitions)

         for proc_retcode, proc_out, _ in results:
-            retcode = max(retcode, proc_retcode)
+            retcode = retcode or proc_retcode
             stdout += proc_out

     return retcode, stdout

pre-commit --version

pre-commit 3.3.3

.pre-commit-config.yaml

repos:
-   repo: local
    hooks:
    -   id: kill-with-term
        name: kill-with-term
        entry: bash -c 'kill -TERM $$; while :; do :; done'
        language: system
        always_run: true
    -   id: node-crash
        name: node-crash
        entry: node --max-old-space-size=1 -p "console.log('ohai')"
        language: system
        always_run: true

~/.cache/pre-commit/pre-commit.log (if present)

No response

[asottile]

yep makes sense, max(0, -9) is going to be 0 -- good catch!