fix: stop the README capture snippet from hiding failed builds

.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:

README.md

@@ -20,20 +20,37 @@ 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
   with:
     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

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

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/<slug> 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)
 

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]

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 <log>`."
+    )
+
+
+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."
+        )