From e5a5ee2e23bf2a123872f5bc3cce821fd2c6dc1e Mon Sep 17 00:00:00 2001 From: Bharat Date: Thu, 4 Jun 2026 13:23:31 +0100 Subject: [PATCH] INF-1484/fix: stop swallowing pre-commit hook failures The python-modules refactor (#21) dropped the set +e exit-code capture: under the composite shell's bash -e, a hook failure aborted the step before EXIT_CODE was written, continue-on-error masked the step failure, and the report step ran exit '' which succeeds. Every hook failure since has reported green. Restore the capture with -e disabled, drop the now-pointless continue-on-error, and make the report step fail loud if the exit code is ever missing. --- action.yaml | 10 +++++++--- pre_commit_action/render_comment.py | 7 ++++--- tests/test_render_comment.py | 10 +++++----- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/action.yaml b/action.yaml index c513782..fa9831c 100644 --- a/action.yaml +++ b/action.yaml @@ -85,10 +85,14 @@ runs: - name: Run pre-commit id: pre-commit shell: bash - continue-on-error: true env: PIP_EXTRA_INDEX_URL: https://europe-north1-python.pkg.dev/two-artifacts/pypi-virtual/simple/ + # The default composite shell runs bash -e, which would abort this step on + # hook failure before EXIT_CODE is written (and continue-on-error would + # mask it, so the report step's `exit ''` succeeded - failures were + # silently swallowed). Capture the code with -e disabled instead. run: | + set +e python -m pre_commit_action.run_hooks ${{ github.base_ref }} HEAD pre-commit-output echo "EXIT_CODE=$?" >> "$GITHUB_OUTPUT" @@ -97,7 +101,6 @@ runs: shell: bash env: PRE_COMMIT_EXIT_CODE: ${{ steps.pre-commit.outputs.EXIT_CODE }} - PRE_COMMIT_OUTCOME: ${{ steps.pre-commit.outcome }} PRE_COMMIT_BASE_REF: ${{ github.base_ref }} PRE_COMMIT_ACTOR: ${{ github.actor }} run: python -m pre_commit_action.render_comment pre-commit-output pre-commit-comment.md @@ -112,4 +115,5 @@ runs: - name: Report pre-commit status shell: bash - run: exit ${{ steps.pre-commit.outputs.EXIT_CODE }} + # Fail loud if the exit code is somehow missing rather than `exit ''` -> 0. + run: exit ${{ steps.pre-commit.outputs.EXIT_CODE || '1' }} diff --git a/pre_commit_action/render_comment.py b/pre_commit_action/render_comment.py index c398a33..f5a41d8 100644 --- a/pre_commit_action/render_comment.py +++ b/pre_commit_action/render_comment.py @@ -18,7 +18,6 @@ def middle_truncate(text: str, max_bytes: int = MAX_OUTPUT_BYTES) -> str: def render_comment( output_path: str, exit_code: str, - outcome: str, base_ref: str, actor: str, ) -> str: @@ -29,6 +28,9 @@ def render_comment( raw = f"(failed to read {output_path}: {err})" output = middle_truncate(raw) + # The step itself always succeeds (exit code is captured with -e disabled), + # so the hook status must come from the exit code, not the step outcome. + outcome = "success" if exit_code == "0" else "failure" emoji = "🏆" if outcome == "success" else "🚫" hint = ( @@ -66,9 +68,8 @@ def render_comment( output_path = sys.argv[1] comment_path = sys.argv[2] if len(sys.argv) > 2 else "pre-commit-comment.md" exit_code = os.environ["PRE_COMMIT_EXIT_CODE"] - outcome = os.environ["PRE_COMMIT_OUTCOME"] base_ref = os.environ["PRE_COMMIT_BASE_REF"] actor = os.environ["PRE_COMMIT_ACTOR"] - comment = render_comment(output_path, exit_code, outcome, base_ref, actor) + comment = render_comment(output_path, exit_code, base_ref, actor) with open(comment_path, "w") as f: f.write(comment) diff --git a/tests/test_render_comment.py b/tests/test_render_comment.py index 8429e67..58c1afd 100644 --- a/tests/test_render_comment.py +++ b/tests/test_render_comment.py @@ -4,7 +4,7 @@ def test_success_shows_trophy(tmp_path): out = tmp_path / "output.txt" out.write_text("All hooks passed.") - result = render_comment(str(out), "0", "success", "main", "alice") + result = render_comment(str(out), "0", "main", "alice") assert "🏆" in result assert "@alice" in result assert "All hooks passed." in result @@ -13,7 +13,7 @@ def test_success_shows_trophy(tmp_path): def test_failure_shows_stop_sign_and_hint(tmp_path): out = tmp_path / "output.txt" out.write_text("Hook failed.") - result = render_comment(str(out), "1", "failure", "main", "bob") + result = render_comment(str(out), "1", "main", "bob") assert "🚫" in result assert "pre-commit run --from-ref origin/main" in result assert "@bob" in result @@ -22,19 +22,19 @@ def test_failure_shows_stop_sign_and_hint(tmp_path): def test_success_has_no_hint(tmp_path): out = tmp_path / "output.txt" out.write_text("ok") - result = render_comment(str(out), "0", "success", "main", "carol") + result = render_comment(str(out), "0", "main", "carol") assert "pre-commit install" not in result def test_exit_code_in_comment(tmp_path): out = tmp_path / "output.txt" out.write_text("done") - result = render_comment(str(out), "42", "failure", "main", "x") + result = render_comment(str(out), "42", "main", "x") assert "Exit code: 42" in result def test_missing_output_file(tmp_path): - result = render_comment(str(tmp_path / "nonexistent.txt"), "1", "failure", "main", "x") + result = render_comment(str(tmp_path / "nonexistent.txt"), "1", "main", "x") assert "failed to read" in result