diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index efcbdc5..031f12a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,9 +24,9 @@ jobs: python-version: "3.12" - name: Install patchrail and pytest - run: python -m pip install --upgrade patchrail pytest + run: python -m pip install --upgrade patchrail pytest pyyaml - - name: Check the guide slugs against the CLI and the published guides + - name: Check the guide slugs, the README snippets and the no-log path run: python -m pytest -q smoke: @@ -52,13 +52,49 @@ jobs: *) echo "unexpected guide-url"; exit 1 ;; esac + # A capture that drops stderr leaves an empty log, and a step that dies early + # leaves no log at all. Both land on an already-red run, so the action must + # explain itself and stay green instead of adding a second, meaningless + # failure. Exercised end-to-end here because it is the shell in action.yml, + # not the Python, that decides whether the step survives. + smoke-no-log: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Create an empty log, as a capture without 2>&1 would + shell: bash + run: ': > empty.log' + + - name: Run the action against the empty log + id: empty + uses: ./ + with: + log-path: empty.log + + - name: Run the action against a log that was never written + id: missing + uses: ./ + with: + log-path: never-created.log + + - name: Assert neither run failed the job, and both said why + shell: bash + run: | + set -euo pipefail + test -z "${{ steps.empty.outputs.failure-class }}" + test -z "${{ steps.missing.outputs.failure-class }}" + test "${{ steps.empty.outputs.guide-url }}" = "https://getpatchrail.com/fix" + test "${{ steps.missing.outputs.guide-url }}" = "https://getpatchrail.com/fix" + echo "OK: no classification, no extra failure" + # Everyone uses `patchrail/ci-triage-action@v1`, but the jobs above test the # commit, not the tag. Without this the two drift apart in silence: a merged # fix stays invisible to every user while CI keeps reporting green. Move the - # floating tag to each main commit that passed both jobs, so `@v1` is always + # floating tag to each main commit that passed the tests, so `@v1` is always # the newest tested code. sync-v1: - needs: [fix-guide-slugs, smoke] + needs: [fix-guide-slugs, smoke, smoke-no-log] if: github.event_name == 'push' && github.ref == 'refs/heads/main' runs-on: ubuntu-latest permissions: diff --git a/README.md b/README.md index c22eeb9..b8f1028 100644 --- a/README.md +++ b/README.md @@ -20,8 +20,8 @@ or test log. ```yaml - name: Build - id: build - run: make build + shell: bash + run: make build 2>&1 | tee build.log - name: PatchRail CI triage if: failure() uses: patchrail/ci-triage-action@v1 @@ -29,11 +29,28 @@ or test log. log-path: build.log ``` -That's the whole thing: pipe your build/test output to a file, then add the +That's the whole thing: capture your build/test output to a file, then add the step guarded by `if: failure()`. On a red run you get an annotation like `python-test-failure (confidence 0.89) — guide: getpatchrail.com/fix/...` plus a job summary block. +### Capturing the log correctly + +The capture step above is deliberate in two ways, and both matter: + +- **`shell: bash`** runs the step with `-o pipefail`. The default `run:` shell is + `bash -e`, where a pipeline reports the status of its *last* command — so + `make build | tee build.log` exits `0` **even when the build fails**. The step + would go green, `if: failure()` would never fire, and your broken build would + sail through CI. With `shell: bash`, a failing command still fails the step. +- **`2>&1`** puts stderr in the log. Plenty of tools (compilers, linters, `mypy`, + `cargo`) report errors only on stderr; without the redirect the log ends up + empty and there is nothing to classify. + +If the log is missing or empty anyway, the action says so in an annotation and +leaves the step green — it will not stack a second failure on top of the one you +are already debugging. + ### Which ref to pin `@v1` is a moving tag: it points at the latest commit on `main` that passed the @@ -65,6 +82,9 @@ Provide either `log-path` (preferred) or `log-text`. | `confidence` | Classifier confidence between 0 and 1. | | `guide-url` | PatchRail `/fix` remediation guide URL for the class. | +When there is no log to classify (the file is missing or empty), `failure-class` +and `confidence` are empty and `guide-url` is the guide index. + ## Example: capture the log and triage on failure ```yaml @@ -74,7 +94,8 @@ jobs: steps: - uses: actions/checkout@v4 - name: Run tests - run: pytest -q | tee test.log + shell: bash + run: pytest -q 2>&1 | tee test.log - name: PatchRail CI triage if: failure() uses: patchrail/ci-triage-action@v1 diff --git a/action.yml b/action.yml index 3d49ac0..8f0400b 100644 --- a/action.yml +++ b/action.yml @@ -65,9 +65,17 @@ runs: run: | set -euo pipefail + # A missing or empty log is a capture mistake, not a reason to add a + # second red step to an already-failing run: annotate why and stop. + annotate() { python "${GITHUB_ACTION_PATH}/scripts/annotate.py" "$@"; } + # Resolve the log source: file path takes precedence, else inline text. LOG_FILE="" - if [ -n "${PATCHRAIL_LOG_PATH}" ] && [ -f "${PATCHRAIL_LOG_PATH}" ]; then + if [ -n "${PATCHRAIL_LOG_PATH}" ]; then + if [ ! -f "${PATCHRAIL_LOG_PATH}" ]; then + annotate --unclassified "log-path '${PATCHRAIL_LOG_PATH}' does not exist on the runner; the step that writes it may have died before creating it." + exit 0 + fi LOG_FILE="${PATCHRAIL_LOG_PATH}" elif [ -n "${PATCHRAIL_LOG_TEXT}" ]; then LOG_FILE="$(mktemp)" @@ -82,7 +90,14 @@ runs: REDACT_FLAG="--redact" fi - patchrail ci explain ${REDACT_FLAG} --log "${LOG_FILE}" --format json --out patchrail-ci-result.json + # `ci explain` exits non-zero on an empty log (the usual symptom of a + # capture that dropped stderr). Keep its message, then annotate. + ERR_FILE="$(mktemp)" + if ! patchrail ci explain ${REDACT_FLAG} --log "${LOG_FILE}" --format json --out patchrail-ci-result.json 2>"${ERR_FILE}"; then + cat "${ERR_FILE}" >&2 + annotate --unclassified "$(cat "${ERR_FILE}")" + exit 0 + fi # Build the guide URL and emit the annotation + summary. - python "${GITHUB_ACTION_PATH}/scripts/annotate.py" patchrail-ci-result.json + annotate patchrail-ci-result.json diff --git a/scripts/annotate.py b/scripts/annotate.py index 3538ce1..9a1a1b4 100644 --- a/scripts/annotate.py +++ b/scripts/annotate.py @@ -13,6 +13,16 @@ FIX_GUIDE_BASE = "https://getpatchrail.com/fix" +# Shown whenever there is no log to classify. Both halves matter: without +# `2>&1` a tool that reports only on stderr leaves an empty log, and without +# pipefail (`shell: bash`) a failing command piped into `tee` exits 0, so the +# step goes green and `if: failure()` never fires. +CAPTURE_HINT = ( + "Capture the failing command like this: " + "`shell: bash` + `your-command 2>&1 | tee build.log` " + "(stderr included, and pipefail keeps the step red)." +) + # Failure classes with a dedicated /fix/ remediation guide on getpatchrail.com. # Unknown or unlisted classes link to the guide index instead, never to a 404. # Every entry must be a real `patchrail ci classes` slug AND a published guide; @@ -69,8 +79,39 @@ def write_kv(path_env: str, lines: list[str]) -> None: handle.write("\n".join(lines) + "\n") +def unclassified(reason: str) -> int: + """Report that there was no log to classify, without failing the job. + + This runs under `if: failure()`, on a run that is already red. A second red + step with a raw exit code buries the failure the user actually came to see, + so surface the cause as a warning and leave the outputs empty. + """ + reason = " ".join(str(reason or "no log to classify").split()) + print(f"::warning title=PatchRail CI Triage::No classification: {reason} {CAPTURE_HINT}") + write_kv( + "GITHUB_STEP_SUMMARY", + [ + "## PatchRail CI Triage", + "", + f"- **No classification:** {reason}", + f"- **How to fix the capture:** {CAPTURE_HINT}", + "", + "_Classified locally. No pull request, comment or external call was made._", + ], + ) + write_kv( + "GITHUB_OUTPUT", + ["failure-class=", "confidence=", f"guide-url={FIX_GUIDE_BASE}"], + ) + return 0 + + def main() -> int: - result_path = sys.argv[1] if len(sys.argv) > 1 else "patchrail-ci-result.json" + argv = sys.argv[1:] + if argv and argv[0] == "--unclassified": + return unclassified(argv[1] if len(argv) > 1 else "") + + result_path = argv[0] if argv else "patchrail-ci-result.json" with open(result_path, encoding="utf-8") as handle: result = json.load(handle) diff --git a/tests/test_no_classification.py b/tests/test_no_classification.py new file mode 100644 index 0000000..0be8285 --- /dev/null +++ b/tests/test_no_classification.py @@ -0,0 +1,76 @@ +"""The action must degrade gracefully when there is no log to classify. + +It runs under `if: failure()`, on a run that is already red. A missing or empty +log used to kill the step with a raw `exit 2` (or a misleading "provide either +log-path or log-text"), stacking a second, meaningless failure on top of the one +the user is actually debugging. It now annotates the cause and stays green. +""" +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +ANNOTATE = Path(__file__).resolve().parent.parent / "scripts" / "annotate.py" +GUIDE_INDEX = "https://getpatchrail.com/fix" + + +@pytest.fixture() +def run_unclassified(tmp_path): + def run(reason: str) -> tuple[subprocess.CompletedProcess, str, str]: + output = tmp_path / "output" + summary = tmp_path / "summary" + output.touch() + summary.touch() + proc = subprocess.run( + [sys.executable, str(ANNOTATE), "--unclassified", reason], + capture_output=True, + text=True, + env={ + "PATH": "/usr/bin:/bin", + "GITHUB_OUTPUT": str(output), + "GITHUB_STEP_SUMMARY": str(summary), + }, + ) + return proc, output.read_text(), summary.read_text() + + return run + + +def test_it_does_not_fail_the_job(run_unclassified) -> None: + proc, _, _ = run_unclassified("log-path 'build.log' does not exist on the runner.") + assert proc.returncode == 0, proc.stderr + + +def test_it_annotates_the_reason_and_the_fix(run_unclassified) -> None: + proc, _, _ = run_unclassified("log input is empty (checked --log build.log)") + assert "::warning title=PatchRail CI Triage::" in proc.stdout + assert "log input is empty" in proc.stdout + # The hint is the whole point: it names both halves of the correct capture. + assert "2>&1" in proc.stdout and "pipefail" in proc.stdout + + +def test_outputs_are_empty_and_the_url_is_the_index(run_unclassified) -> None: + """Downstream steps must be able to tell "no classification" from a real one.""" + _, output, _ = run_unclassified("log input is empty") + lines = output.strip().splitlines() + assert "failure-class=" in lines + assert "confidence=" in lines + assert f"guide-url={GUIDE_INDEX}" in lines + + +def test_the_summary_explains_the_capture(run_unclassified) -> None: + _, _, summary = run_unclassified("log input is empty") + assert "## PatchRail CI Triage" in summary + assert "No classification" in summary + assert "2>&1" in summary + + +def test_a_multiline_reason_stays_on_one_annotation_line(run_unclassified) -> None: + """GitHub reads one annotation per line; a raw stderr dump would truncate it.""" + proc, _, _ = run_unclassified("patchrail: log input is empty\n traceback line\n") + warnings = [ln for ln in proc.stdout.splitlines() if ln.startswith("::warning")] + assert len(warnings) == 1 + assert "traceback line" in warnings[0] diff --git a/tests/test_readme_snippets.py b/tests/test_readme_snippets.py new file mode 100644 index 0000000..66e19cf --- /dev/null +++ b/tests/test_readme_snippets.py @@ -0,0 +1,101 @@ +"""Guard the workflow snippets in the README against the log-capture footgun. + +People copy the README verbatim into their own workflow, so a broken snippet +here breaks *their* CI, not ours. Two mistakes are easy to make and silent: + +* `make build | tee build.log` under the default `run:` shell (`bash -e`, no + pipefail) exits 0 when the build fails -- the step goes green, `if: failure()` + never fires, and a red build is reported as passing; +* no `2>&1`, so a tool that reports only on stderr writes an empty log and there + is nothing left to classify. + +These tests parse every YAML snippet in the README and fail if a capture step +loses either protection, or if the file it writes is not the one handed to the +action. +""" +from __future__ import annotations + +import re +from pathlib import Path +from typing import Any, Iterator + +import pytest +import yaml + +README = Path(__file__).resolve().parent.parent / "README.md" +_YAML_BLOCK = re.compile(r"```yaml\n(.*?)```", re.DOTALL) +_TEE_TARGET = re.compile(r"\|\s*tee\s+(\S+)") + + +def _snippets() -> list[Any]: + blocks = _YAML_BLOCK.findall(README.read_text(encoding="utf-8")) + assert blocks, "no ```yaml blocks found in the README; did the docs move?" + return [yaml.safe_load(block) for block in blocks] + + +def _walk(node: Any) -> Iterator[dict]: + """Yield every mapping in a parsed snippet, at any depth.""" + if isinstance(node, dict): + yield node + for value in node.values(): + yield from _walk(value) + elif isinstance(node, list): + for item in node: + yield from _walk(item) + + +def _capture_steps() -> list[dict]: + """Every README step that pipes a command into `tee`.""" + steps = [ + step + for snippet in _snippets() + for step in _walk(snippet) + if isinstance(step.get("run"), str) and "tee" in step["run"] + ] + assert steps, "the README no longer shows how to capture a log; the snippets must" + return steps + + +@pytest.mark.parametrize("step", _capture_steps(), ids=lambda s: str(s.get("name", "step"))) +def test_capture_step_keeps_the_step_red_on_failure(step: dict) -> None: + """Without pipefail, `cmd | tee log` swallows cmd's exit code and CI goes green.""" + has_pipefail = step.get("shell") == "bash" or "pipefail" in step["run"] + assert has_pipefail, ( + f"README step {step.get('name')!r} pipes into `tee` without pipefail. " + f"The default `run:` shell is `bash -e`, so `{step['run']}` exits 0 even when " + f"the command fails: the step goes green and `if: failure()` never fires. " + f"Add `shell: bash` (which is `bash -eo pipefail`) or `set -o pipefail`." + ) + + +@pytest.mark.parametrize("step", _capture_steps(), ids=lambda s: str(s.get("name", "step"))) +def test_capture_step_records_stderr(step: dict) -> None: + """Compilers, linters and type checkers report on stderr; an empty log explains nothing.""" + assert "2>&1" in step["run"], ( + f"README step {step.get('name')!r} captures stdout only. Tools that report " + f"errors on stderr would leave an empty log and nothing to classify. " + f"Use `2>&1 | tee `." + ) + + +def test_captured_file_is_the_one_handed_to_the_action() -> None: + """A snippet that tees to build.log but triages test.log would never classify anything.""" + for snippet in _snippets(): + mappings = list(_walk(snippet)) + captured = { + match + for step in mappings + if isinstance(step.get("run"), str) + for match in _TEE_TARGET.findall(step["run"]) + } + triaged = { + str(step["with"]["log-path"]) + for step in mappings + if isinstance(step.get("with"), dict) and step["with"].get("log-path") + } + if not triaged: + continue + assert triaged <= captured, ( + f"README snippet triages {sorted(triaged - captured)} but never writes it " + f"(it captures {sorted(captured)}). The action would find no log." + )