From 69215c038191621caf0e615a0b0fdb74e9c5a4fe Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 20 Oct 2026 12:40:53 +0000 Subject: [PATCH] fix(rules): RE005 ignores trap handlers; unmask two workflow steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RE005 flagged every `|| true` in a test-named step. Of hypatia's three findings, one was a rule false positive and two were masks that could go: - tests.yml:763 `trap 'kill "$STRESS_PID" 2>/dev/null || true' EXIT`. The EXIT trap runs after the step's status is decided, and under `set -e` an unmasked failing kill turns a PASSING step red (measured: rc 1). The rule now skips `trap …` lines. The same false positive is in echidna server-boot-gate.yml. - tests.yml:192 `$(find … -exec grep … | grep -v … || true)` also masked grep's exit 2, so an error passed the check (measured with a missing directory: old step PASS, new step rc 2). Rewritten with grep -r and an explicit status test (1 = no match = pass; >1 fails), comment lines removed with sed. - build-gossamer-gui.yml:189 diagnostic `ls … 2>/dev/null || true` right before `exit 1`: the mask hid the diagnosis and changed nothing. Tests: 2 new RE005 cases (trap ignored; a trap does not hide a real swallow); 42/42 pass, and the trap case fails against the previous module. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01CYR5bf5FBLS7cXQK72aVNM --- .github/workflows/build-gossamer-gui.yml | 4 ++- .github/workflows/tests.yml | 18 +++++++++--- lib/rules/research_extensions.ex | 12 +++++++- test/research_extensions_test.exs | 35 ++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build-gossamer-gui.yml b/.github/workflows/build-gossamer-gui.yml index 89d936e5..d70dd1a2 100644 --- a/.github/workflows/build-gossamer-gui.yml +++ b/.github/workflows/build-gossamer-gui.yml @@ -217,7 +217,9 @@ jobs: if [ -z "$cli" ]; then echo "::error::No ephapax CLI binary produced by 'cargo build --release --workspace'." echo "Contents of $release_dir:" - ls -la "$release_dir" 2>/dev/null || true + # No mask: this branch fails either way, and a failing `ls` + # (missing dir) is itself the diagnosis, so let its error show. + ls -la "$release_dir" exit 1 fi diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 698f040d..f41b3e26 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -201,10 +201,20 @@ jobs: # legitimately mention these tokens as documentation # ("Zero believe_me." in src/Hypatia/ABI/RuleEngine.idr is the canonical # example). - DANGEROUS=$(find lib src \( -name '*.idr' -o -name '*.v' -o -name '*.lean' -o -name '*.hs' \) \ - -not -path 'lib/rules/*' \ - -exec grep -Hn 'believe_me\|assert_total\|\bsorry\b\|\bAdmitted\b\|unsafeCoerce' {} + 2>/dev/null \ - | grep -v ':[[:space:]]*--' || true) + # + # grep exits 1 when nothing matches, which is the PASS case, and 2 + # on a real error. Test the status rather than masking both with + # `|| true` (`lib/rules` is the only `rules` directory under lib/src). + set -euo pipefail + status=0 + HITS=$(grep -rHn --include='*.idr' --include='*.v' --include='*.lean' --include='*.hs' \ + --exclude-dir=rules \ + 'believe_me\|assert_total\|\bsorry\b\|\bAdmitted\b\|unsafeCoerce' lib src) || status=$? + if [ "$status" -gt 1 ]; then + echo "FAIL: grep error (exit $status)" + exit "$status" + fi + DANGEROUS=$(printf '%s' "$HITS" | sed '/:[[:space:]]*--/d') if [ -n "$DANGEROUS" ]; then echo "FAIL: Dangerous patterns found" echo "$DANGEROUS" diff --git a/lib/rules/research_extensions.ex b/lib/rules/research_extensions.ex index d8b1ad18..1be54388 100644 --- a/lib/rules/research_extensions.ex +++ b/lib/rules/research_extensions.ex @@ -587,6 +587,9 @@ defmodule Hypatia.Rules.ResearchExtensions do suite goes green while real failures pile up. Provenance: Datadog CI Visibility flake-masking patterns. + `|| true` on a `trap …` line is not counted: the handler runs after the + step's status is decided and cannot mask it. + Severity: `:warn`. Action: `:report`. """ def re005_test_swallows_exit(repo_path) do @@ -619,7 +622,14 @@ defmodule Hypatia.Rules.ResearchExtensions do has_continue_on_error? = Regex.match?(~r/continue-on-error:\s*true\b/, stripped_body) - has_or_true? = Regex.match?(~r/\|\|\s*true\b/, stripped_body) + # A `trap` handler's `|| true` is not a swallow. The step's exit status + # is decided before the EXIT trap runs, and a failing handler command + # under `set -e` (e.g. `kill` of an already-exited PID) would itself + # turn a PASSING step red, so the `|| true` is what keeps it honest. + has_or_true? = + stripped_body + |> String.replace(~r/^[ \t]*trap[ \t].*$/m, "") + |> then(&Regex.match?(~r/\|\|\s*true\b/, &1)) # A step carrying `hypatia:ignore RE005 -- ` (or the # `hypatia: allow research_extensions/RE005` spelling) is reviewed — diff --git a/test/research_extensions_test.exs b/test/research_extensions_test.exs index 66be109a..f81872b3 100644 --- a/test/research_extensions_test.exs +++ b/test/research_extensions_test.exs @@ -415,6 +415,41 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do File.rm_rf!(repo) end + test "ignores || true inside a trap handler (cleanup, not the step's status)" do + repo = + create_repo_with_workflow(""" + jobs: + x: + steps: + - name: Test suite under stress + run: | + set -euo pipefail + stress-ng --cpu 4 & + STRESS_PID=$! + trap 'kill "$STRESS_PID" 2>/dev/null || true' EXIT + cargo test + """) + + assert ResearchExtensions.re005_test_swallows_exit(repo) == [] + File.rm_rf!(repo) + end + + test "a trap does not hide a real swallow elsewhere in the step" do + repo = + create_repo_with_workflow(""" + jobs: + x: + steps: + - name: Test suite under stress + run: | + trap 'kill "$PID" 2>/dev/null || true' EXIT + cargo test || true + """) + + assert [%{rule: "RE005"}] = ResearchExtensions.re005_test_swallows_exit(repo) + File.rm_rf!(repo) + end + test "ignores non-test steps with continue-on-error" do repo = create_repo_with_workflow("""