Take the job's outcome as an argument, not as an assumption

`report_job_log.py` hardcoded the word *failed* into the comment header. Every
caller guards the step with `if: failure()`, so it was true by construction --
until an unguarded probe in weblib-viewer#10 ran it on a job that passed, and
the successful run posted a comment reading as a failure report. Anyone
scrolling that PR would conclude the probe had failed.

The interesting uses of this script are exactly the ones that want
`if: always()`: a probe, or a job whose *output* is the point rather than its
exit code. Those all lied in the header.

`--status` now supplies the outcome. **The default is `failed`**, which is what
`if: failure()` means, so the four consuming repos are untouched -- a change in
required arguments would have broken all of them at once, since they take this
script from `@main`. `JOB_STATUS` in the environment does the same, matching how
every other argument here already reads its default from the Actions
environment.

`${{ job.status }}` yields `success`/`failure`/`cancelled`/`skipped` while a
human writing the flag reaches for `passed`/`failed`, so both spellings are
accepted and the expression can be passed straight through. An **unrecognised**
status goes into the header verbatim rather than being rejected: `argparse`'s
`choices=` would exit 2 on a value the table has not heard of, and the log --
the whole reason this script exists -- would never be posted. A reporter must
not become the thing that reports nothing.

The "no log file" note was status-dependent too; it claimed the step "failed
before the build started" regardless.

## Verified

`test_report_job_log.py`, new here: stdlib only and offline, posting to an
`http.server` on localhost that keeps what it is sent, so each test reads the
comment back. An exit status of 0 proves nothing -- the script deliberately
swallows HTTP errors so a failure to report cannot mask the failure being
reported. 21 tests, 0 skipped, 1.2s. Three of them drive the CLI as a
subprocess with only environment variables set, the way a workflow does.

Each check was shown to fire by injecting the fault and reverting it:

| injected fault | result |
|---|---|
| header hardcodes `failed` again (the original bug) | 10 failures |
| `DEFAULT_STATUS = "passed"` (would break the four callers) | 8 failures |
| unknown status raises, as `choices=` would | 2 errors |
| missing-log note keeps the failure wording | 1 failure |
| a stray `%` in the `--status` help text | 1 failure |

All five reverted; the file's checksum matches the pre-injection copy.

Also drops a tracked `__pycache__/report_job_log.cpython-313.pyc` and adds a
`.gitignore`. It was committed by accident in 061d8b2 and importing the module
from the tests rewrites it, so it would otherwise show up in every future diff
as stale bytecode of a file that had already changed.

Closes #8

Co-authored-by: bit <bit@das-labor.org>

.gitignore

@@ -0,0 +1,4 @@
+# The test suite imports report_job_log, and a tracked .pyc of it went stale
+# the moment the source changed.
+__pycache__/
+*.pyc

README.md

@@ -14,12 +14,25 @@ fetch it — which was measured to be the difference between one step and three.
 | file | what it does |
 |---|---|
 | `with-nixpkgs.sh` | Runs a command with one nixpkgs package on PATH, pinned to the *consuming* repo's `flake.lock`. Avoids `nix shell nixpkgs#x`, which re-resolves the registry and refetches a channel tarball whenever the branch moves. |
-| `report_job_log.py` | Posts the tail of a build log as a PR comment. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. |
+| `report_job_log.py` | Posts the tail of a build log as a PR comment. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. Takes `--status`; see below. |
 | `sync_blocked_label.py` | Keeps `Status/Blocked` in step with Gitea's dependency graph. Resolves the label from the repo *or the organisation*, and never touches an issue marked `Status/On Hold` or `Status/Abandoned`. |
 
 All three are standard library / plain bash only. They are *run*, not built, so
 this repo has no flake.
 
+`test_report_job_log.py` covers the reporter. It is standard library and
+offline — the forge it posts to is an `http.server` on localhost that keeps
+what it is sent, so a test reads the comment back rather than trusting an exit
+status of 0, which this script returns even when the POST failed.
+
+```bash
+python3 test_report_job_log.py      # 21 tests, ~1.2s, no network
+```
+
+There is no workflow running it: this repo has no `.gitea/workflows` at all,
+and no `flake.lock` for `with-nixpkgs.sh` to read. Run it by hand before
+pushing.
+
 ## Using it
 
 ```yaml
@@ -57,6 +70,34 @@ The `outputs.path` row is listed separately on purpose: the action *running* and
 its output *reaching the caller* are different claims, and a composite action
 returning an empty string is exactly the sort of thing that looks green.
 
+### `report_job_log.py --status`
+
+The header used to be hardcoded to *failed*. That is true of every caller here,
+because each guards the step with `if: failure()` — but a probe run under
+`if: always()` posted a failure report for a job that had passed
+(weblib-viewer#10, filed as #8).
+
+**The default is still `failed`**, so a caller passing only the log path is
+unchanged. A step that can run on success has to say so:
+
+```yaml
+- name: report the log
+  if: always()
+  env:
+    GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
+  run: |
+    bash "${{ steps.ci.outputs.path }}/with-nixpkgs.sh" python3 \
+      python3 "${{ steps.ci.outputs.path }}/report_job_log.py" /tmp/build.log \
+      --status "${{ job.status }}"
+```
+
+`${{ job.status }}` yields `success`/`failure`/`cancelled`/`skipped`, so those
+spellings are accepted alongside `passed`/`failed`. `JOB_STATUS` in the
+environment does the same thing if a flag is awkward. An **unrecognised**
+status is put in the header verbatim rather than rejected: `argparse`'s
+`choices=` would exit 2 on a value this list has not heard of, and the log —
+the entire reason the script exists — would never be posted.
+
 ### Why not a flake input
 
 These are scripts a workflow runs, not derivations. A flake input would cost a

report_job_log.py

@@ -17,9 +17,16 @@ Usage, from a workflow step guarded by `if: failure()`:
 
     report_job_log.py /tmp/build.log
 
+The header says *failed* by default, which is what that guard means. A step
+guarded by `if: always()` -- a probe, or a job whose output is the point rather
+than its exit code -- has to say so, or the comment reports a failure that did
+not happen:
+
+    report_job_log.py /tmp/build.log --status "${{ job.status }}"
+
 Everything else comes from the Actions environment: `GITHUB_REPOSITORY`,
-`GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`, and
-`GITHUB_SERVER_URL`.
+`GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`,
+`GITHUB_SERVER_URL`, and `JOB_STATUS` if `--status` is not passed.
 
 Remove this once the instance is updated and logs can be read directly.
 """
@@ -41,6 +48,51 @@ DEFAULT_TAIL = 12000
 # nix colours its output, and the raw escapes are noise in Markdown.
 ANSI = re.compile(r"\x1b\[[0-9;]*[A-Za-z]")
 
+# The header used to hardcode "failed". Every caller guards the step with
+# `if: failure()`, so that was true by construction -- until an `if: always()`
+# probe in weblib-viewer#10 posted a failure report for a job that had passed.
+# The default stays `failed` so those guarded callers are unchanged.
+DEFAULT_STATUS = "failed"
+
+# `${{ job.status }}` yields success/failure/cancelled/skipped; a human writing
+# the flag by hand reaches for passed/failed. Accept both spellings so a call
+# site can pass the expression straight through.
+STATUS_PHRASES = {
+    "failed": "failed",
+    "failure": "failed",
+    "passed": "passed",
+    "success": "passed",
+    "succeeded": "passed",
+    "cancelled": "was cancelled",
+    "canceled": "was cancelled",
+    "skipped": "was skipped",
+}
+
+
+def status_phrase(status):
+    """The verb for the comment header.
+
+    An unrecognised status is reported verbatim rather than rejected. This
+    script exists because a red job says nothing, so it must not itself become
+    the thing that says nothing: `argparse`'s `choices=` would exit 2 on a
+    status this table has not heard of, and the log would never be posted.
+    """
+    key = (status or "").strip().lower()
+    if not key:
+        key = DEFAULT_STATUS
+    if key in STATUS_PHRASES:
+        return STATUS_PHRASES[key]
+    return "finished with status `" + status.strip() + "`"
+
+
+def missing_log_note(path, phrase):
+    """What to say when the log file the step named is not there."""
+    if phrase == "failed":
+        return (f"The step produced no log file at `{path}` — it failed "
+                "before the build started.")
+    return (f"The step produced no log file at `{path}` — nothing was "
+            "captured.")
+
 
 def pull_request_number(event_path):
     """The PR this job is running for, from the event payload, or None."""
@@ -123,6 +175,11 @@ def main(argv=None):
     ap.add_argument("--pr", type=int, default=None)
     ap.add_argument("--job", default=os.environ.get("GITHUB_JOB") or "job")
     ap.add_argument("--tail", type=int, default=DEFAULT_TAIL)
+    ap.add_argument("--status",
+                    default=os.environ.get("JOB_STATUS") or DEFAULT_STATUS,
+                    help="outcome of the job being reported: failed (the "
+                         "default, and what `if: failure()` means), passed, "
+                         "cancelled, skipped, or ${{ job.status }} verbatim")
     args = ap.parse_args(argv)
 
     if not args.repo or not args.token:
@@ -138,14 +195,15 @@ def main(argv=None):
         print("no pull request for this run: nothing to comment on")
         return 0
 
+    phrase = status_phrase(args.status)
+
     if not os.path.exists(args.log):
-        text = ("The step produced no log file at "
-                f"`{args.log}` — it failed before the build started.")
+        text = missing_log_note(args.log, phrase)
     else:
         text = "```\n" + tail(args.log, args.tail).rstrip() + "\n```"
 
     sha = (os.environ.get("GITHUB_SHA") or "")[:8]
-    body = (f"**`{args.job}` failed**{f' on `{sha}`' if sha else ''}.\n\n"
+    body = (f"**`{args.job}` {phrase}**{f' on `{sha}`' if sha else ''}.\n\n"
             "Job logs return 500 on this Gitea, so here is the tail of the "
             "build output, posted by [`report_job_log.py`]"
             "(https://git.chaosbit.de/weblib/weblib-ci).\n\n"

test_report_job_log.py

@@ -0,0 +1,322 @@
+#!/usr/bin/env python3
+"""Tests for `report_job_log.py`.
+
+Standard library only, like everything else here, and offline: the Gitea it
+posts to is a `http.server` on localhost that keeps the comments it is sent, so
+a test can *re-read* what was written instead of trusting an exit code of 0.
+
+Run them with any python3:
+
+    python3 test_report_job_log.py
+
+The point of most of them is the comment **header**. `report_job_log.py` used
+to hardcode the word "failed", so a step guarded by `if: always()` posted a
+failure report for a job that had passed (weblib-ci#8, found in
+weblib-viewer#10). The header is the one thing a reader sees before the log, so
+it is the one thing worth asserting on.
+"""
+
+import contextlib
+import io
+import json
+import os
+import subprocess
+import sys
+import threading
+import unittest
+from http.server import BaseHTTPRequestHandler, HTTPServer
+
+import report_job_log
+
+HERE = os.path.dirname(os.path.abspath(__file__))
+SCRIPT = os.path.join(HERE, "report_job_log.py")
+
+# Environment variables the script reads. Every test clears all of them, so a
+# stray one in the ambient shell (this suite is meant to be runnable on a
+# runner, where several of these are set) cannot change a result.
+ACTIONS_ENV = [
+    "GITHUB_REPOSITORY", "GITHUB_EVENT_PATH", "GITHUB_SERVER_URL",
+    "GITHUB_SHA", "GITHUB_JOB", "GITHUB_HEAD_REF", "GITHUB_REF_NAME",
+    "GITEA_HOST", "GITEA_TOKEN", "GITHUB_TOKEN", "JOB_STATUS",
+]
+
+
+class FakeGitea(BaseHTTPRequestHandler):
+    """Just enough of the API: list open PRs, and accept a comment."""
+
+    comments = []                       # class-level; reset per test
+    pulls = []
+
+    def _send(self, code, payload):
+        raw = json.dumps(payload).encode()
+        self.send_response(code)
+        self.send_header("Content-Type", "application/json")
+        self.send_header("Content-Length", str(len(raw)))
+        self.end_headers()
+        self.wfile.write(raw)
+
+    def do_GET(self):
+        if "/pulls" in self.path:
+            return self._send(200, type(self).pulls)
+        self._send(404, {"message": "no"})
+
+    def do_POST(self):
+        length = int(self.headers.get("Content-Length") or 0)
+        payload = json.loads(self.rfile.read(length) or b"{}")
+        type(self).comments.append({
+            "path": self.path,
+            "auth": self.headers.get("Authorization"),
+            "body": payload.get("body", ""),
+        })
+        self._send(201, {"html_url": "http://example.invalid/c/1"})
+
+    def log_message(self, *a):          # keep the test output readable
+        pass
+
+
+class ServerTestCase(unittest.TestCase):
+    """A test that runs the whole entry point against the fake forge."""
+
+    def setUp(self):
+        FakeGitea.comments = []
+        FakeGitea.pulls = []
+        self.server = HTTPServer(("127.0.0.1", 0), FakeGitea)
+        self.host = "http://127.0.0.1:%d" % self.server.server_address[1]
+        # `shutdown()` only takes effect on the next poll, so the default
+        # 0.5s interval charged this suite half a second per test -- measured
+        # 4.6s for nine tests, 0.6s after.
+        self.thread = threading.Thread(
+            target=self.server.serve_forever, kwargs={"poll_interval": 0.01},
+            daemon=True)
+        self.thread.start()
+        self.addCleanup(self.server.server_close)
+        self.addCleanup(self.server.shutdown)
+
+        self.log = os.path.join(self.mkdtemp(), "build.log")
+        with open(self.log, "w") as fh:
+            fh.write("nix-build \x1b[31msomething\x1b[0m\nDISTINCTIVE-LINE\n")
+
+        for name in ACTIONS_ENV:
+            os.environ.pop(name, None)
+
+    def mkdtemp(self):
+        import tempfile
+        d = tempfile.mkdtemp()
+        self.addCleanup(lambda: __import__("shutil").rmtree(d,
+                                                            ignore_errors=True))
+        return d
+
+    def posted(self):
+        """What the server actually stored, read back from the server.
+
+        An exit status of 0 is not evidence that a comment was written: the
+        script deliberately swallows HTTP errors so that a failure to report
+        does not mask the failure being reported, and returns 0 either way.
+        """
+        return FakeGitea.comments
+
+    def run_main(self, *argv):
+        """The entry point in-process, so a traceback is readable."""
+        out = io.StringIO()
+        with contextlib.redirect_stdout(out):
+            code = report_job_log.main([
+                self.log, "--repo", "weblib/weblib-ci", "--host", self.host,
+                "--token", "t0ken", "--pr", "8", "--job", "tests", *argv])
+        self.assertEqual(code, 0)
+        self.assertIn("reported to weblib/weblib-ci#8", out.getvalue())
+        return code
+
+    def run_cli(self, *argv, env=None):
+        """The entry point as a subprocess: argv parsing and shebang included."""
+        full = dict(os.environ)
+        full.update(env or {})
+        proc = subprocess.run(
+            [sys.executable, SCRIPT, self.log, "--repo", "weblib/weblib-ci",
+             "--host", self.host, "--token", "t0ken", "--pr", "8",
+             "--job", "tests", *argv],
+            capture_output=True, text=True, env=full, cwd=HERE)
+        return proc
+
+
+class TestHeader(ServerTestCase):
+
+    def test_default_is_still_failed(self):
+        """Backward compatibility: the four repos pass only the log path."""
+        self.run_main()
+        body = self.posted()[0]["body"]
+        self.assertIn("**`tests` failed**", body)
+        self.assertNotIn("passed", body)
+
+    def test_status_passed(self):
+        self.run_main("--status", "passed")
+        body = self.posted()[0]["body"]
+        self.assertIn("**`tests` passed**", body)
+        self.assertNotIn("failed", body)
+
+    def test_job_status_expression_success(self):
+        """`${{ job.status }}` yields `success`, not `passed`."""
+        self.run_main("--status", "success")
+        self.assertIn("**`tests` passed**", self.posted()[0]["body"])
+
+    def test_job_status_expression_failure(self):
+        self.run_main("--status", "failure")
+        self.assertIn("**`tests` failed**", self.posted()[0]["body"])
+
+    def test_cancelled(self):
+        self.run_main("--status", "cancelled")
+        self.assertIn("**`tests` was cancelled**", self.posted()[0]["body"])
+
+    def test_case_and_whitespace_are_not_a_failure_report(self):
+        self.run_main("--status", "  Success \n")
+        self.assertIn("**`tests` passed**", self.posted()[0]["body"])
+
+    def test_unknown_status_still_reports(self):
+        """A status the table has not heard of must not cost us the log.
+
+        `argparse(choices=...)` would exit 2 here, and the log this script
+        exists to surface would never be posted.
+        """
+        self.run_main("--status", "neutral")
+        body = self.posted()[0]["body"]
+        self.assertIn("**`tests` finished with status `neutral`**", body)
+        self.assertIn("DISTINCTIVE-LINE", body)
+
+    def test_empty_status_falls_back_to_the_default(self):
+        self.run_main("--status", "")
+        self.assertIn("**`tests` failed**", self.posted()[0]["body"])
+
+    def test_sha_and_log_survive_a_non_default_status(self):
+        os.environ["GITHUB_SHA"] = "0123456789abcdef"
+        self.run_main("--status", "passed")
+        body = self.posted()[0]["body"]
+        self.assertIn("**`tests` passed** on `01234567`.", body)
+        self.assertIn("DISTINCTIVE-LINE", body)
+        self.assertNotIn("\x1b[31m", body)      # ANSI still stripped
+
+
+class TestEnvironment(ServerTestCase):
+
+    def test_job_status_env_is_honoured(self):
+        proc = self.run_cli(env={"JOB_STATUS": "success"})
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("**`tests` passed**", self.posted()[0]["body"])
+
+    def test_flag_beats_the_environment(self):
+        proc = self.run_cli("--status", "failed", env={"JOB_STATUS": "success"})
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("**`tests` failed**", self.posted()[0]["body"])
+
+    def test_unset_environment_is_still_failed(self):
+        proc = self.run_cli()
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("**`tests` failed**", self.posted()[0]["body"])
+
+
+class TestMissingLog(ServerTestCase):
+
+    def test_missing_log_when_failed(self):
+        self.log = os.path.join(self.mkdtemp(), "absent.log")
+        self.run_main()
+        self.assertIn("it failed before the build started",
+                      self.posted()[0]["body"])
+
+    def test_missing_log_when_passed_does_not_claim_a_failure(self):
+        self.log = os.path.join(self.mkdtemp(), "absent.log")
+        self.run_main("--status", "passed")
+        body = self.posted()[0]["body"]
+        self.assertIn("nothing was captured", body)
+        self.assertNotIn("failed", body)
+
+
+class TestEndToEnd(ServerTestCase):
+    """The CLI, driven the way a workflow drives it: environment, no `--pr`."""
+
+    def _event_file(self, number):
+        path = os.path.join(self.mkdtemp(), "event.json")
+        with open(path, "w") as fh:
+            json.dump({"pull_request": {"number": number}}, fh)
+        return path
+
+    def _run(self, *argv, env=None):
+        full = dict(os.environ)
+        full.pop("GITEA_HOST", None)
+        full.update({
+            "GITHUB_REPOSITORY": "weblib/weblib-ci",
+            "GITHUB_SERVER_URL": self.host,
+            "GITEA_TOKEN": "t0ken",
+            "GITHUB_JOB": "probe",
+            "GITHUB_SHA": "abcdef0123456789",
+            "GITHUB_EVENT_PATH": self._event_file(42),
+        })
+        full.update(env or {})
+        return subprocess.run([sys.executable, SCRIPT, self.log, *argv],
+                              capture_output=True, text=True, env=full,
+                              cwd=HERE)
+
+    def test_always_guarded_probe_that_passed(self):
+        proc = self._run("--status", "success")
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("reported to weblib/weblib-ci#42", proc.stdout)
+
+        # The write is only real if the resource says so.
+        posted = self.posted()
+        self.assertEqual(len(posted), 1, posted)
+        self.assertEqual(posted[0]["path"],
+                         "/api/v1/repos/weblib/weblib-ci/issues/42/comments")
+        self.assertEqual(posted[0]["auth"], "token t0ken")
+        body = posted[0]["body"]
+        self.assertIn("**`probe` passed** on `abcdef01`.", body)
+        self.assertNotIn("failed", body)
+        self.assertIn("DISTINCTIVE-LINE", body)
+
+    def test_failure_guarded_job_is_unchanged(self):
+        proc = self._run()
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("**`probe` failed** on `abcdef01`.",
+                      self.posted()[0]["body"])
+
+    def test_branch_lookup_path_still_works(self):
+        """No event payload: the PR is found by head branch, and reported."""
+        FakeGitea.pulls = [{"number": 7, "head": {"ref": "fix/x"}}]
+        proc = self._run("--status", "passed",
+                         env={"GITHUB_EVENT_PATH": "",
+                              "GITHUB_HEAD_REF": "fix/x"})
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("reported to weblib/weblib-ci#7", proc.stdout)
+        self.assertIn("**`probe` passed**", self.posted()[0]["body"])
+
+    def test_help_still_renders(self):
+        """argparse `%`-expands `help=`; a stray `%` there breaks `--help`."""
+        proc = subprocess.run([sys.executable, SCRIPT, "--help"],
+                              capture_output=True, text=True, cwd=HERE)
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("--status", proc.stdout)
+
+
+class TestStatusPhrase(unittest.TestCase):
+    """The unit under the header, without a server."""
+
+    def test_table(self):
+        for status, want in [
+                ("failed", "failed"), ("failure", "failed"),
+                ("passed", "passed"), ("success", "passed"),
+                ("succeeded", "passed"),
+                ("cancelled", "was cancelled"), ("canceled", "was cancelled"),
+                ("skipped", "was skipped"),
+                ("SUCCESS", "passed"), (" failure ", "failed"),
+                (None, "failed"), ("", "failed"),
+        ]:
+            with self.subTest(status=status):
+                self.assertEqual(report_job_log.status_phrase(status), want)
+
+    def test_unknown_is_verbatim(self):
+        self.assertEqual(report_job_log.status_phrase("weird"),
+                         "finished with status `weird`")
+
+    def test_default_constant_is_failed(self):
+        """Named so that changing it is a deliberate act, not a typo."""
+        self.assertEqual(report_job_log.DEFAULT_STATUS, "failed")
+
+
+if __name__ == "__main__":
+    unittest.main(verbosity=2)