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("""