From 540dbb23026cec06888d703e02c4f831fc2b73e2 Mon Sep 17 00:00:00 2001 From: Claude Code Bot Date: Thu, 24 Sep 2026 23:11:36 -0700 Subject: [PATCH] feat(standards): name what failed, not just that something did A red standards check said "shellcheck found problems" and, at the end, "N linter(s) failed". Neither named the rule or the file, so finding the cause meant opening the raw job log. run-standards.sh now captures each linter's output as it streams and, on failure: - annotates the linter with the first 20 lines of its findings, so the check's annotation list shows the rule and file; - ends with "standards-check failed: , "; - when GITHUB_STEP_SUMMARY is set, writes each failed linter's output (first 200 lines) to the run summary. A clean run writes nothing there. Exit codes and the streamed log are unchanged. Callers pin the standards-check-v1 tag, which still points at the old script. They see none of this until the tag moves, and moving it is a separate decision. Advances smartwatermelon/dev-env#113 --- standards/run-standards.sh | 98 +++++++++++++++++++++++++++++++++---- tests/test-run-standards.sh | 39 +++++++++++++++ 2 files changed, 127 insertions(+), 10 deletions(-) diff --git a/standards/run-standards.sh b/standards/run-standards.sh index 8d286d4..8cfbf4e 100755 --- a/standards/run-standards.sh +++ b/standards/run-standards.sh @@ -55,7 +55,6 @@ if ! git -C "${repo}" rev-parse --git-dir >/dev/null 2>&1; then exit 2 fi -failures=0 _skipped() { [[ ",${skip}," == *",$1,"* ]]; } # Say which mode is in effect. A run that lints 3 files and one that lints 300 # both print "all enabled linters clean", so without this line the scope of a @@ -66,7 +65,85 @@ else echo "scope: all tracked files (whole-repo hygiene mode)" fi _header() { echo; echo "== $1"; } -_fail() { echo "::error::$1 found problems"; failures=$((failures + 1)); } + +# Say WHAT failed, not just that something did (dev-env#113). A red check +# whose only text is "shellcheck found problems" sends the reader into the raw +# job log to find the rule and the file. So each linter's output is captured +# as it streams, and a failure is reported three ways: +# - the linter's ::error:: annotation carries the start of its findings, so +# the check's annotation list shows the rule and the file; +# - the final ::error:: line names every linter that failed; +# - when GITHUB_STEP_SUMMARY is set (CI), the run summary gets each failed +# linter's output. Nothing is written there on a clean run. +failed=() +out_dir="$(mktemp -d)" +trap 'rm -rf "${out_dir}"' EXIT +annotation_lines=20 +summary_lines=200 + +# Workflow-command data must encode %, CR and LF, or a multi-line message is +# cut at the first newline and a literal "%0A" in the findings is misread. +_annotation_escape() { + local s="$1" + s="${s//'%'/%25}" + s="${s//$'\r'/%0D}" + s="${s//$'\n'/%0A}" + printf '%s' "${s}" +} + +# _fail NAME [LOG]: record a failed linter and annotate it with the head of +# its captured output. +_fail() { + local name="$1" log="${2:-}" msg total + failed+=("${name}") + msg="${name} found problems" + if [[ -n "${log}" && -s "${log}" ]]; then + total="$(wc -l <"${log}" | tr -d ' ')" + msg+=$'\n'"$(head -n "${annotation_lines}" "${log}")" + if ((total > annotation_lines)); then + msg+=$'\n'"... $((total - annotation_lines)) more line(s); see the job log or run summary" + fi + fi + echo "::error title=${name}::$(_annotation_escape "${msg}")" +} + +# _lint NAME CMD...: run CMD from the repo root, streaming its output to the +# log as before while keeping a copy for _fail. pipefail (set above) makes the +# pipeline's status the linter's, not tee's. +_lint() { + local name="$1" + shift + if (cd "${repo}" && "$@") 2>&1 | tee "${out_dir}/${name}.log"; then + return 0 + fi + _fail "${name}" "${out_dir}/${name}.log" +} + +_write_summary() { + [[ -n "${GITHUB_STEP_SUMMARY:-}" ]] || return 0 + local name log total + { + echo "## standards-check failed: $(IFS=,; echo "${failed[*]}" | sed 's/,/, /g')" + for name in "${failed[@]}"; do + log="${out_dir}/${name}.log" + echo + echo "### ${name}" + echo + if [[ -s "${log}" ]]; then + echo '````text' + head -n "${summary_lines}" "${log}" + echo '````' + total="$(wc -l <"${log}" | tr -d ' ')" + if ((total > summary_lines)); then + echo + echo "_Truncated: $((total - summary_lines)) more line(s) in the job log._" + fi + else + echo "_No output captured; see the job log._" + fi + done + } >>"${GITHUB_STEP_SUMMARY}" +} # Fail loudly on a --changed-since ref git cannot resolve. Left to fall # through, an unresolvable ref yields an empty changed set, every file-based @@ -139,7 +216,7 @@ if _skipped shellcheck; then echo "== shellcheck: skipped by input"; else cfg="" if [[ -f "${repo}/.shellcheckrc" ]]; then cfg="${repo}/.shellcheckrc"; fi [[ -n "${cfg}" ]] || cfg="${config_dir}/shellcheckrc" - (cd "${repo}" && shellcheck --rcfile "${cfg}" -S info "${files[@]}") || _fail shellcheck + _lint shellcheck shellcheck --rcfile "${cfg}" -S info "${files[@]}" fi fi @@ -159,7 +236,7 @@ if _skipped yamllint; then echo "== yamllint: skipped by input"; else if [[ -f "${repo}/${c}" ]]; then cfg="${repo}/${c}"; break; fi done [[ -n "${cfg}" ]] || cfg="${config_dir}/yamllint.yml" - (cd "${repo}" && yamllint -c "${cfg}" -f parsable "${files[@]}") || _fail yamllint + _lint yamllint yamllint -c "${cfg}" -f parsable "${files[@]}" fi fi @@ -167,7 +244,7 @@ fi if _skipped actionlint; then echo "== actionlint: skipped by input"; else _header actionlint if compgen -G "${repo}/.github/workflows/*.y*ml" >/dev/null; then - (cd "${repo}" && actionlint -shellcheck= -pyflakes=) || _fail actionlint + _lint actionlint actionlint -shellcheck= -pyflakes= else echo "::notice::no workflows"; fi fi @@ -186,7 +263,7 @@ if _skipped zizmor; then echo "== zizmor: skipped by input"; else done < <(_tracked || true) if ((${#files[@]} == 0)); then echo "::notice::no workflows"; else cfg="${repo}/zizmor.yml"; [[ -f "${cfg}" ]] || cfg="${config_dir}/../zizmor.yml" - (cd "${repo}" && zizmor --config "${cfg}" --min-severity low --no-online-audits "${files[@]}") || _fail zizmor + _lint zizmor zizmor --config "${cfg}" --min-severity low --no-online-audits "${files[@]}" fi fi @@ -206,19 +283,20 @@ if _skipped markdownlint; then echo "== markdownlint: skipped by input"; else if [[ -f "${repo}/${c}" ]]; then cfg="${repo}/${c}"; break; fi done [[ -n "${cfg}" ]] || cfg="${config_dir}/markdownlint.json" - (cd "${repo}" && markdownlint-cli2 --config "${cfg}" "${files[@]}") || _fail markdownlint + _lint markdownlint markdownlint-cli2 --config "${cfg}" "${files[@]}" fi fi # node-floor if _skipped node-floor; then echo "== node-floor: skipped by input"; else _header node-floor - bash "${config_dir}/check-node-floor.sh" "${repo}" "${node_floor}" || _fail node-floor + _lint node-floor bash "${config_dir}/check-node-floor.sh" "${repo}" "${node_floor}" fi echo -if ((failures > 0)); then - echo "::error::standards-check: ${failures} linter(s) failed" +if ((${#failed[@]} > 0)); then + _write_summary + echo "::error::standards-check failed: $(IFS=,; echo "${failed[*]}" | sed 's/,/, /g')" exit 1 fi echo "standards-check: all enabled linters clean" diff --git a/tests/test-run-standards.sh b/tests/test-run-standards.sh index 8678388..6e94c27 100755 --- a/tests/test-run-standards.sh +++ b/tests/test-run-standards.sh @@ -36,6 +36,45 @@ d='$' _mk bad-sh; printf '#!/usr/bin/env bash\necho %s1\n' "${d}" >"${tmp}/bad-sh/x.sh"; git -C "${tmp}/bad-sh" add -A _expect_fail "shellcheck: unquoted \$1 (SC2086) rejected" bad-sh +# A failed check must say WHAT failed, not just that something did +# (dev-env#113). Three places carry it: the final ::error:: line names every +# failed linter; each linter's own ::error:: annotation carries the start of +# its findings, so the annotation list on the check shows the rule and file; +# and when GITHUB_STEP_SUMMARY is set, the run summary gets each failed +# linter's output. The bad-sh fixture has exactly one finding, SC2086. +summary="${tmp}/bad-sh.summary.md" +: >"${summary}" +GITHUB_STEP_SUMMARY="${summary}" bash "${runner}" --repo "${tmp}/bad-sh" --config-dir "${cfg}" \ + >"${tmp}/bad-sh-named.log" 2>&1 || true +if grep -q '^::error::standards-check failed: shellcheck$' "${tmp}/bad-sh-named.log"; then + _ok "final error line names the failed linter" +else + _bad "final error line does not name the failed linter (see ${tmp}/bad-sh-named.log)" +fi +if grep -q '^::error title=shellcheck::.*SC2086' "${tmp}/bad-sh-named.log"; then + _ok "per-linter annotation carries the finding (SC2086)" +else + _bad "per-linter annotation does not carry the finding (see ${tmp}/bad-sh-named.log)" +fi +if grep -q 'shellcheck' "${summary}" && grep -q 'SC2086' "${summary}"; then + _ok "step summary names the failed linter and its finding" +else + _bad "step summary lacks the failed linter or its finding (see ${summary})" +fi +# A clean run must write no failure section: a summary that always lists +# linters would name "failures" on a green check. +summary_clean="${tmp}/clean-summary.md" +: >"${summary_clean}" +mkdir -p "${tmp}/summary-clean"; git -C "${tmp}/summary-clean" init -q --template="${tmpl}" +printf '# Title\n\nBody.\n' >"${tmp}/summary-clean/README.md"; git -C "${tmp}/summary-clean" add -A +GITHUB_STEP_SUMMARY="${summary_clean}" bash "${runner}" --repo "${tmp}/summary-clean" --config-dir "${cfg}" \ + >"${tmp}/summary-clean.log" 2>&1 || true +if [[ ! -s "${summary_clean}" ]]; then + _ok "clean run writes no failure section to the step summary" +else + _bad "clean run wrote to the step summary (see ${summary_clean})" +fi + _mk bad-yaml; printf 'a: 1\n b: 2\n' >"${tmp}/bad-yaml/x.yml"; git -C "${tmp}/bad-yaml" add -A _expect_fail "yamllint: bad indentation rejected" bad-yaml