ci(pre-commit): fail the job when hooks never ran - #14754
Conversation
The job treats hook failures as report-only (the PR comment carries them) and is meant to go red only when the runner itself breaks. The crash guard tests for one shape of that break: the script dying before it writes any exit code. pre-commit has a second shape: it aborts before running hooks and still exits with a code (environment install failures exit 3), so the step writes exit_code=3 with zero hook verdicts, the guard sees a non-empty code, and the check lands green while every hook in the config is skipped. The guard fails the job on any completion other than clean (0) or hooks-ran-and-some-failed (1), which keeps the report-only design for real hook findings and reds the job for every shape of never-ran.
github-actions
Bot
added
github_actions
|
See the Build Results workflow run for details. |
|
The red pre-commit check on this PR is this PR's own guard working: pre-commit on master currently aborts with exit code 3 before running a single hook (the vale-sync environment install failure fixed in #14753), and with this guard that state fails the job instead of landing green with zero hook verdicts. So the two PRs go together: #14753 fixes the breakage, this one makes sure the next breakage of this class is visible. Merging #14753 first turns this one green. |
There was a problem hiding this comment.
Pull request overview
Updates the pre-commit workflow guard so infrastructure failures cannot silently skip all hooks while CI remains green.
Changes:
- Preserves report-only handling for normal hook failures (
exit_code=1). - Fails the job for missing or unexpected exit codes.
- Improves failure diagnostics.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| # Hook failures are surfaced via the PR comment; the job itself fails only when the runner | ||
| # never produced hook verdicts. That is two shapes: the script died before writing results | ||
| # (no exit_code output), or pre-commit aborted before running hooks (exit codes other than | ||
| # 0/1, e.g. a hook environment that fails to install). Treating the second shape as green | ||
| # leaves every hook silently skipped while the check shows passing. | ||
| - name: Fail job on pre-commit script crash | ||
| if: always() && steps.pre-commit.outcome == 'failure' && steps.pre-commit.outputs.exit_code == '' | ||
| if: always() && steps.pre-commit.outcome == 'failure' && steps.pre-commit.outputs.exit_code != '1' | ||
| env: | ||
| EXIT_CODE: ${{ steps.pre-commit.outputs.exit_code }} | ||
| run: | | ||
| echo "::error::pre-commit runner crashed before writing results" | ||
| echo "::error::pre-commit runner crashed before running hooks (exit_code='${EXIT_CODE}')" | ||
| exit 1 |
There was a problem hiding this comment.
There is a third shape that still lands green: pre-commit exits 1 for fatal errors too, not just hook failures — e.g. a malformed .pre-commit-config.yaml raises FatalError, which exits 1 with zero hooks run. That produces exit_code=1, passed=0, failed=0 and passes this guard. Since the runner already writes passed/failed outputs, the guard can close that shape as well:
| # Hook failures are surfaced via the PR comment; the job itself fails only when the runner | |
| # never produced hook verdicts. That is two shapes: the script died before writing results | |
| # (no exit_code output), or pre-commit aborted before running hooks (exit codes other than | |
| # 0/1, e.g. a hook environment that fails to install). Treating the second shape as green | |
| # leaves every hook silently skipped while the check shows passing. | |
| - name: Fail job on pre-commit script crash | |
| if: always() && steps.pre-commit.outcome == 'failure' && steps.pre-commit.outputs.exit_code == '' | |
| if: always() && steps.pre-commit.outcome == 'failure' && steps.pre-commit.outputs.exit_code != '1' | |
| env: | |
| EXIT_CODE: ${{ steps.pre-commit.outputs.exit_code }} | |
| run: | | |
| echo "::error::pre-commit runner crashed before writing results" | |
| echo "::error::pre-commit runner crashed before running hooks (exit_code='${EXIT_CODE}')" | |
| exit 1 | |
| # Hook failures are surfaced via the PR comment; the job itself fails only when the runner | |
| # never produced hook verdicts. That is three shapes: the script died before writing results | |
| # (no exit_code output), pre-commit aborted before running hooks (exit codes other than | |
| # 0/1, e.g. a hook environment that fails to install), or pre-commit exited 1 without any | |
| # hook verdicts (e.g. a fatal config error). Treating those as green leaves every hook | |
| # silently skipped while the check shows passing. | |
| - name: Fail job on pre-commit script crash | |
| if: >- | |
| always() && steps.pre-commit.outcome == 'failure' && | |
| (steps.pre-commit.outputs.exit_code != '1' || | |
| (steps.pre-commit.outputs.passed == '0' && steps.pre-commit.outputs.failed == '0')) | |
| env: | |
| EXIT_CODE: ${{ steps.pre-commit.outputs.exit_code }} | |
| PASSED: ${{ steps.pre-commit.outputs.passed }} | |
| FAILED: ${{ steps.pre-commit.outputs.failed }} | |
| run: | | |
| echo "::error::pre-commit produced no hook verdicts (exit_code='${EXIT_CODE}' passed='${PASSED}' failed='${FAILED}')" | |
| exit 1 |
|
@TSC21 Can you look at the review? |
Contributed on behalf of RIIS, LLC.
CI/Build Changes
The pre-commit job's crash guard now fails the job on any pre-commit completion other than exit 0 (all hooks passed) or exit 1 (hooks ran and some failed). It keeps the current report-only design for real hook findings: those still land in the PR comment and do not red the job.
Reason
The current guard only fails the job when the runner script dies without writing an exit code at all. pre-commit has a second failure shape the guard misses: it can abort before running any hook and still exit with a code, for example when a hook environment fails to install (exit code 3). In that shape the step writes
exit_code=3with zero hook verdicts, the guard sees a non-empty code, and the check lands green while every hook in the config was skipped. That is exactly what has been happening: see the companion PR fixing thevale-syncinstall, which was silently killing every pre-commit run. A gate that can pass without running is not a gate, so this closes the detection side while the companion PR fixes the current trigger.Testing
exit_code: 3, passed: 0, failed: 0in the results artifact and a green check; with this guard that run fails)Impact
Only the pre-commit workflow. Hook findings keep their report-only behaviour; the job only goes red when pre-commit never produced verdicts.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).