From 12d939b95777aa59246fa98d89e7c5f4faed1acd Mon Sep 17 00:00:00 2001 From: Alex Root-Roatch Date: Tue, 28 Jul 2026 13:12:24 -0500 Subject: [PATCH] docs: correct the stale --error comment in the semgrep job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job comment still claimed "semgrep ALWAYS runs with --error, so any finding turns this job red". That flag was removed when the SARIF reporter took over the exit decision, so the comment described the opposite of the behaviour and implied the WARNING rules gate builds. Replaced with the two gates spelled out separately, since conflating them is the easy mistake: bin/report-sarif.sh decides WHICH findings turn the job red (error level only, suppressed excluded), and continue-on-error decides whether that red BLOCKS the caller. Also notes that semgrep's own "Blocking" label in its console summary is its policy tag and affects neither. Adds a regression guard. Re-adding --error would make semgrep exit non-zero on any finding, and under set -e that kills the step before report-sarif.sh runs — silently making the WARNING rules blocking and deleting the findings table. bin/test-report-sarif.sh now asserts the flag is absent and the reporter is called; mutation-checked by re-adding the flag and watching it fail. Swept the rest: the three WARNING rules describe themselves accurately, and the remaining --error mentions are this explanation and an accurate historical note. --- .github/workflows/security.yml | 19 ++++++++++++++----- bin/test-report-sarif.sh | 13 +++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 10d3973..e85e745 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -295,11 +295,20 @@ jobs: # rule enumerates them and spec-fixtures/ exercises more than one spelling — # see bin/test-rules.sh, which fails if that stops working. # - # semgrep ALWAYS runs with --error, so any finding turns this job red and - # stays visible — its "Blocking" label is a policy tag, NOT an exit code, so - # --error is what actually makes `semgrep scan` exit non-zero. continue-on- - # error decides only whether that red BLOCKS the build: advisory (default) - # surfaces it without blocking; semgrep-blocking: true fails the build. + # Two independent gates, easy to conflate: + # + # 1. WHICH findings turn this job red — decided by bin/report-sarif.sh, + # which exits 1 only on error-level results. semgrep is deliberately NOT + # run with --error: that gates on ANY finding regardless of severity, + # which would make cc-path-traversal, cc-generic-catch and + # cc-clojure-xml-xxe block builds despite being WARNING precisely so they + # do not. Findings suppressed in source with nosemgrep are excluded too. + # + # 2. Whether a red job BLOCKS the caller — decided by continue-on-error + # below. Advisory by default; semgrep-blocking: true propagates it. + # + # semgrep's own "Blocking" label in its console summary is its policy tag and + # has no bearing on either gate. continue-on-error: ${{ !inputs.semgrep-blocking }} container: # Digest-pinned, not :latest. semgrep is now the ONLY Clojure detection diff --git a/bin/test-report-sarif.sh b/bin/test-report-sarif.sh index 4cdc987..1d67ac7 100644 --- a/bin/test-report-sarif.sh +++ b/bin/test-report-sarif.sh @@ -66,5 +66,18 @@ out="$(bash "${REPORT}" "${WORK}/absent.sarif" 2>&1)"; rc=$? check "missing SARIF fails" "$rc" "1" check "missing SARIF explains why" "$(echo "$out" | grep -c 'did not complete')" "1" +# --- wiring: the workflow must not re-add semgrep's --error ------------------ +# --error makes semgrep exit non-zero on ANY finding regardless of severity. +# Under `set -e` that kills the step before report-sarif.sh runs, which would +# silently (a) make the WARNING rules block builds, contradicting the README, and +# (b) delete the findings table. Cheap to re-add by accident, so pin it. +WF="${ROOT}/.github/workflows/security.yml" +if [ -f "$WF" ]; then + check "semgrep is not invoked with --error" \ + "$(grep -cE '^\s+args=\(scan.*--error' "$WF")" "0" + check "the reporter is actually called" \ + "$(grep -c 'report-sarif.sh semgrep.sarif' "$WF")" "1" +fi + echo "report-sarif tests: ${pass} passed, ${fail} failed" [ "${fail}" -eq 0 ]