From 8a276e89e3c8a3399e832defbd22d82ff0f8e6fd Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Mon, 25 May 2026 11:16:35 +0300 Subject: [PATCH 1/3] fix(action): scope exit-code capture instead of disabling errexit globally Addresses the Sourcery review on #12 (the set +e change is already in main via #11, released in v1.6.0): - action.yml: replace `set +e` + direct call with `EXIT_CODE=0; leakwatch "${ARGS[@]}" || EXIT_CODE=$?`, so errexit stays enabled for the rest of the step (later failures still fail fast) while a findings exit (1) is captured and mapped. Guard the job-summary jq pipe with `|| true` so a malformed SARIF can't abort before the exit-code mapping. - action-test.yml (cli-github-format): capture the exit code instead of `|| true`, failing on a real error (>=2) while tolerating findings (0/1). Verified under `bash -e -o pipefail`: the mapping runs and a subsequent failing command still aborts (errexit not globally disabled). Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/action-test.yml | 12 ++++++++---- action.yml | 14 +++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.github/workflows/action-test.yml b/.github/workflows/action-test.yml index 79d355d..00e04b8 100644 --- a/.github/workflows/action-test.yml +++ b/.github/workflows/action-test.yml @@ -142,10 +142,14 @@ jobs: set -uo pipefail mkdir -p _fx printf 'AWS_ACCESS_KEY_ID=%s%s\n' 'AKIA' 'IOSFODNN7EXAMPLE' > _fx/leak.env - # leakwatch exits 1 on findings; `|| true` so the capture doesn't abort - # the step under bash -e (GitHub's default shell flags). - out="$("${RUNNER_TEMP}/leakwatch" scan fs _fx --format github --no-verify 2>/dev/null || true)" + # leakwatch exits 1 on findings (expected here); capture the code so the + # step doesn't abort under bash -e, but still fail on a real error (>=2). + rc=0 + out="$("${RUNNER_TEMP}/leakwatch" scan fs _fx --format github --no-verify 2>/dev/null)" || rc=$? echo "$out" + if [ "$rc" -ge 2 ]; then + echo "::error::leakwatch exited with $rc (a hard error, not findings)"; exit 1 + fi echo "$out" | grep -q '^::error .*aws-access-key-id' \ - || { echo "::error::expected an ::error annotation for aws-access-key-id"; exit 1; } + || { echo "::error::expected an ::error annotation for aws-access-key-id (exit was $rc)"; exit 1; } echo "OK: --format github emitted an inline annotation" diff --git a/action.yml b/action.yml index 035af68..5584bae 100644 --- a/action.yml +++ b/action.yml @@ -289,12 +289,12 @@ runs: # Do NOT echo the assembled args: path/extra-args may carry credentials # (tokens, authenticated URLs) that GitHub log masking would not catch. echo "Running leakwatch scan (type=${INPUT_SCAN_TYPE}, format=${INPUT_FORMAT})" - # GitHub invokes bash with -e; findings legitimately exit 1, so disable - # errexit around the scan and map the exit code ourselves below (otherwise - # the step would abort here and fail-on-findings: false would be ignored). - set +e - leakwatch "${ARGS[@]}" - EXIT_CODE=$? + # GitHub invokes bash with -e, and findings legitimately exit 1. Capture + # the exit code without disabling errexit globally (via `|| EXIT_CODE=$?`), + # so later commands in this step still fail fast. Without this, the step + # would abort here and fail-on-findings: false would be ignored. + EXIT_CODE=0 + leakwatch "${ARGS[@]}" || EXIT_CODE=$? # ---- Job summary ------------------------------------------------------ if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then @@ -310,7 +310,7 @@ runs: echo "" echo "| Level | Detector | Location |" echo "| --- | --- | --- |" - jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 + jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || true if [ "${total:-0}" -gt 50 ] 2>/dev/null; then echo "" echo "_…showing the first 50 of ${total} findings._" From 4f9fa2f324da57a0f3ccea403de0f7040981b2cb Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Mon, 25 May 2026 11:25:53 +0300 Subject: [PATCH 2/3] fix(action): non-silent summary fallback; clearer test exit-code capture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the PR #13 review: - action.yml: replace the job-summary `|| true` with a visible fallback note so a (theoretical) render failure isn't silently swallowed. The jq filter already handles location-less findings via `// "-"` (verified jq returns "-" without erroring), so the `?` operator is unnecessary. - action-test.yml (cli-github-format): capture the exit code with an explicit if/else to make the semantics obvious. (The reviewer's `if ! out=…; then rc=$?` form is incorrect — it captures 0, not the real exit code.) Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/action-test.yml | 9 +++++++-- action.yml | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/action-test.yml b/.github/workflows/action-test.yml index 00e04b8..cd6b82d 100644 --- a/.github/workflows/action-test.yml +++ b/.github/workflows/action-test.yml @@ -144,8 +144,13 @@ jobs: printf 'AWS_ACCESS_KEY_ID=%s%s\n' 'AKIA' 'IOSFODNN7EXAMPLE' > _fx/leak.env # leakwatch exits 1 on findings (expected here); capture the code so the # step doesn't abort under bash -e, but still fail on a real error (>=2). - rc=0 - out="$("${RUNNER_TEMP}/leakwatch" scan fs _fx --format github --no-verify 2>/dev/null)" || rc=$? + # Explicit if/else makes the exit-code capture obvious (an assignment's + # status mirrors its command substitution). + if out="$("${RUNNER_TEMP}/leakwatch" scan fs _fx --format github --no-verify 2>/dev/null)"; then + rc=0 + else + rc=$? + fi echo "$out" if [ "$rc" -ge 2 ]; then echo "::error::leakwatch exited with $rc (a hard error, not findings)"; exit 1 diff --git a/action.yml b/action.yml index 5584bae..c2c60e8 100644 --- a/action.yml +++ b/action.yml @@ -310,7 +310,7 @@ runs: echo "" echo "| Level | Detector | Location |" echo "| --- | --- | --- |" - jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || true + jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || echo "_(could not render the findings table; see the SARIF artifact)_" if [ "${total:-0}" -gt 50 ] 2>/dev/null; then echo "" echo "_…showing the first 50 of ${total} findings._" From b4d56256a29f17c0b284f5d55725da3db5d4631e Mon Sep 17 00:00:00 2001 From: Cemil ILIK Date: Mon, 25 May 2026 11:32:01 +0300 Subject: [PATCH 3/3] fix(action): render summary table via temp file to avoid SIGPIPE false fallback Piping jq into `head -50` gives jq a SIGPIPE once head closes after 50 lines; under pipefail that non-zero status tripped the `|| echo fallback` even when the table rendered fine (reproducible once jq's output exceeds the ~64KB pipe buffer, i.e. very many findings). Write jq output to a temp file, then `head -n 50` it, and emit the fallback only when jq fails or the file is empty. Co-Authored-By: Claude Opus 4.7 (1M context) --- action.yml | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/action.yml b/action.yml index c2c60e8..b20166c 100644 --- a/action.yml +++ b/action.yml @@ -310,7 +310,17 @@ runs: echo "" echo "| Level | Detector | Location |" echo "| --- | --- | --- |" - jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" 2>/dev/null | head -50 || echo "_(could not render the findings table; see the SARIF artifact)_" + # Render to a temp file first, then take the first 50 lines from + # it. Piping jq directly into `head` would give jq a SIGPIPE once + # head closes (after 50 lines), and under pipefail that non-zero + # status would trip the fallback even though the table rendered. + tbl="$(mktemp)" + if jq -r '.runs[].results[] | "| \(.level) | \(.ruleId) | \((.locations[0].physicalLocation.artifactLocation.uri // "-"))\(if .locations[0].physicalLocation.region.startLine then ":" + (.locations[0].physicalLocation.region.startLine | tostring) else "" end) |"' "$OUT" > "$tbl" 2>/dev/null && [ -s "$tbl" ]; then + head -n 50 "$tbl" + else + echo "_(could not render the findings table; see the SARIF artifact)_" + fi + rm -f "$tbl" if [ "${total:-0}" -gt 50 ] 2>/dev/null; then echo "" echo "_…showing the first 50 of ${total} findings._"