From ce5febacadab9fc298d62b9c5f9d2cc6627140a2 Mon Sep 17 00:00:00 2001 From: Alex Root-Roatch Date: Tue, 28 Jul 2026 12:16:29 -0500 Subject: [PATCH] fix: clj-holmes crash on large repos; make semgrep findings visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by the first real consumer run (cleancoders.com), and both are regressions from replacing clj-holmes-action in this branch. clj-holmes crashed with exit 255 and wrote no SARIF at all: IllegalArgumentException: Value out of range for int: 35050732419 progrock.core$interval_str -> clj_holmes.logic.progress Its progress bar overflows an int computing an ETA. Silent on small repos, fatal on ~1300 Clojure files. The stock action always passed --no-verbose; dropping that flag reintroduced the bug. With it, the same scan exits 0 and reports 0 findings — so a progress-bar bug was blocking a production deploy. Also guards a missing SARIF explicitly instead of letting jq fail obscurely. semgrep found 5 blocking issues and the log said only 'Findings: 5'. With --sarif --output the detail goes to the file, so --error produced a red job with no indication of what or where. Now the findings print with severity, rule, file and line, and the exit code is computed here. Only error-level findings fail the job. Severity lives on the rule (tool.driver.rules[].defaultConfiguration.level), not the result, so the jq joins them — cc-path-traversal and cc-generic-catch are WARNING on purpose and the README promises they do not block. Counting every result would have broken that promise: cleancoders.com has 7 findings but only 3 blocking. --- .github/workflows/security.yml | 56 ++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 36f4f46..006b927 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -248,7 +248,15 @@ jobs: # This also fixes a second problem: with SARIF going to a file, a # failing scan printed nothing, so the log gave no reason. Now the # findings are echoed before the job fails. - args=(scan -p . -d /tmp/rules --no-fail-on-result -t sarif -o clj-holmes.sarif) + # --no-verbose is REQUIRED, not cosmetic. clj-holmes renders a progrock + # progress bar whose ETA calculation overflows an int on large repos: + # IllegalArgumentException: Value out of range for int: 35050732419 + # progrock.core$interval_str -> clj_holmes.logic.progress + # That kills the scan with exit 255 and writes no SARIF at all. It is + # silent on small repos and fatal on real ones (~1300 Clojure files). + # The stock clj-holmes-action always passed --no-verbose; dropping it + # when replacing that action is what reintroduced this. + args=(scan -p . -d /tmp/rules --no-fail-on-result --no-verbose -t sarif -o clj-holmes.sarif) # NOT `[ -n "$IGNORED" ] && args+=(...)`: that whole statement returns 1 # when the input is empty (the default), and `set -e` would exit here. if [ -n "$IGNORED" ]; then @@ -256,6 +264,10 @@ jobs: fi clj-holmes "${args[@]}" + if [ ! -f clj-holmes.sarif ]; then + echo "::error::clj-holmes produced no SARIF; the scan did not complete" + exit 1 + fi count=$(jq '[.runs[].results[]?] | length' clj-holmes.sarif) if [ "$count" -gt 0 ]; then jq -r '.runs[].results[] @@ -417,7 +429,11 @@ jobs: EXTRA_RULES_DIR: ${{ inputs.extra-rules-dir }} run: | set -euo pipefail - args=(scan --error --sarif --output semgrep.sarif + # No --error here. With --sarif --output the findings go to the file and + # the console gets only a count, so `--error` produced a red job whose + # log said "Findings: 5" and nothing about WHAT or WHERE. We print them + # from the SARIF below and set the exit code ourselves. + args=(scan --sarif --output semgrep.sarif --config .cc-security-rules/security-rules/semgrep --config p/owasp-top-ten --config p/default) @@ -431,6 +447,42 @@ jobs: args+=(--exclude "$IGNORED") fi semgrep "${args[@]}" + + if [ ! -f semgrep.sarif ]; then + echo "::error::semgrep produced no SARIF; the scan did not complete" + exit 1 + fi + + # Severity lives on the rule, not the result: semgrep leaves + # results[].level null and puts it in tool.driver.rules[]. + # defaultConfiguration.level. Join them so the printout shows severity + # and so the exit decision can honour it. + jq -r '.runs[] as $r + | ($r.tool.driver.rules + | map({key: .id, value: .defaultConfiguration.level}) + | from_entries) as $lv + | $r.results[] + | " [\($lv[.ruleId] // "unknown")] \(.ruleId | split(".") | last) " + + "\(.locations[0].physicalLocation.artifactLocation.uri)" + + ":\(.locations[0].physicalLocation.region.startLine // 0)"' \ + semgrep.sarif + + # Only error-level findings fail the job. cc-path-traversal and + # cc-generic-catch are severity: WARNING on purpose — without dataflow + # they cannot be precise enough to gate a build, and the README + # promises they do not block. Counting every result would break that. + blocking=$(jq '[.runs[] as $r + | ($r.tool.driver.rules + | map({key: .id, value: .defaultConfiguration.level}) + | from_entries) as $lv + | $r.results[] | select($lv[.ruleId] == "error")] | length' \ + semgrep.sarif) + total=$(jq '[.runs[].results[]?] | length' semgrep.sarif) + echo "semgrep: ${total} finding(s), ${blocking} blocking" + if [ "$blocking" -gt 0 ]; then + echo "::error::semgrep found $blocking blocking finding(s); see above and the semgrep-sarif artifact" + exit 1 + fi - name: Upload SARIF if: always() # evidence of a FAILING scan is the evidence most worth keeping uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 (node24)