Merge pull request 'Give a push-triggered job somewhere to report a failure' (#15) from fix/push-triggered-log-channel into main

Reviewed-on: #15
Reviewed-by: bit <bit@no-reply@chaosbit.de>

README.md

@@ -14,7 +14,7 @@ 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. Takes `--status`; see below. |
+| `report_job_log.py` | Posts the tail of a build log as a PR comment, falling back to a commit status when a `push`-triggered job has no PR to comment on. 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*, never touches an issue marked `Status/On Hold` or `Status/Abandoned`, and leaves the label alone (rather than guessing) on a blocker whose state a repo-scoped token cannot resolve — see weblib-ci#11. |
 
 All three are standard library / plain bash only. They are *run*, not built, so
@@ -27,7 +27,7 @@ test reads state back rather than trusting an exit status of 0 (which the
 reporter in particular returns even when its POST failed).
 
 ```bash
-python3 test_report_job_log.py       # 22 tests, ~1.2s, no network
+python3 test_report_job_log.py       # 28 tests, ~3s, no network
 python3 test_sync_blocked_label.py   # 12 tests, no network
 ```
 
@@ -100,6 +100,50 @@ 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.
 
+### `push`-triggered jobs: no open PR does not mean no channel
+
+Found the hard way on weblib-ci#14: a `push` to `development`/`main` — the
+common shape of a workflow like a publish pipeline, which has no
+`pull_request` trigger at all — has no *open* PR for `report_job_log.py` to
+comment on. The branch that was pushed to is the merge's **target**, not the
+PR that produced it, and that PR is already closed. The old behaviour was to
+print "no pull request for this run" and exit 0, so a failure there was
+completely invisible: not the log (500 on this Gitea), not a comment
+(nowhere to put it), nothing. That is how weblib-viewer run 1769's failure
+(`publish.sh` refusing to overwrite an existing APK) went unnoticed until it
+was reproduced by hand.
+
+Two things changed, tried in order, neither needing a new flag:
+
+1. **Ask Gitea which PR produced this exact commit.**
+   `GET repos/{repo}/commits/{sha}/pull` resolves the PR a commit came from —
+   open or closed — keyed on the commit itself rather than a branch name.
+   Verified against both merge styles used in this org: an explicit merge
+   commit, and a fast-forward promotion where the "merge commit" is just the
+   PR branch's own tip. This needs the **full** 40-character `GITHUB_SHA`; a
+   short sha gets the same 404 shape as a commit with no PR at all, so it
+   would silently look like "not found". This covers the ordinary case —
+   almost every push to a protected branch here is a merge landing.
+2. **If that also finds nothing** — a commit that was never part of any
+   PR — post a **commit status** instead
+   (`POST repos/{repo}/statuses/{sha}`, context `report_job_log`). It is not
+   a substitute for the full log: this Gitea did not truncate a 3000-character
+   description in testing, but a status is UI chrome, not a page, so what
+   goes there is deliberately a short snippet (`STATUS_TAIL`, 500 characters),
+   not the whole tail. `state` follows the same outcome as the header phrase
+   (`failed → failure`, `passed → success`, `cancelled → warning`,
+   `skipped → skipped`; an unrecognised status maps to `error` rather than
+   guessing pass or fail), and `target_url` points at the commit.
+
+**A commit *comment* API (GitHub-style, `commits/{sha}/comments`) does not
+exist on this Gitea** — 404, and absent from `/swagger.v1.json` entirely, not
+merely unauthorised. That ruled out the most direct-looking option; the
+commit-status endpoint is the one that is actually there.
+
+If a PR *is* found (by either the original lookups or the new sha lookup),
+behaviour is identical to before: a comment, same format. The status path is
+only reached when no PR exists anywhere for the commit.
+
 ### Why not a flake input
 
 These are scripts a workflow runs, not derivations. A flake input would cost a

report_job_log.py

@@ -28,6 +28,16 @@ Everything else comes from the Actions environment: `GITHUB_REPOSITORY`,
 `GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`,
 `GITHUB_SERVER_URL`, and `JOB_STATUS` if `--status` is not passed.
 
+A `push` event has no open PR to comment on -- the common case is that a PR
+just merged into `development`/`main` and closed, so `GITHUB_HEAD_REF` is the
+target branch, not the one that was merged (weblib-ci#14). Before giving up,
+this asks Gitea which PR produced `GITHUB_SHA`
+(`GET .../commits/{sha}/pull`, which resolves a closed PR from its merge
+commit -- verified against both merge styles this org uses). If that also
+comes up empty -- a commit that was never part of any PR -- the log itself has
+nowhere to go, so a commit status is posted instead: shorter, but still
+readable next to the commit, unlike silence.
+
 Remove this once the instance is updated and logs can be read directly.
 """
 
@@ -48,6 +58,12 @@ 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]")
 
+# A commit status is UI chrome (a line in a list), not a page -- measured
+# against this Gitea: it accepted a 3000-char description unchanged, so
+# nothing here truncates it server-side, but that does not make a wall of log
+# a good idea to put there. Keep it to a snippet.
+STATUS_TAIL = 500
+
 # 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.
@@ -68,6 +84,18 @@ STATUS_PHRASES = {
     "skipped": "was skipped",
 }
 
+# Gitea's status `state` enum is not the same vocabulary as the header
+# phrases above (`pending, success, error, failure, warning, skipped`).
+# A phrase this table has not heard of -- the "finished with status `X`"
+# fallback -- maps to `error`: the honest reading is "something unusual
+# happened", not a guess at pass or fail.
+STATUS_STATE = {
+    "failed": "failure",
+    "passed": "success",
+    "was cancelled": "warning",
+    "was skipped": "skipped",
+}
+
 
 def status_phrase(status):
     """The verb for the comment header.
@@ -119,8 +147,9 @@ def pull_request_for_branch(host, repo, token, branch):
     dispatched run that failed stayed exactly as silent as the one this script
     exists to fix. Looking the branch up covers those.
 
-    A push to a branch with no PR still has nowhere to comment, which is
-    honest: there is no thread for it.
+    Only finds an **open** PR. A push whose branch's PR has already merged and
+    closed (the common shape for `development`/`main`) is `pull_request_for_sha`'s
+    job, tried next.
     """
     if not branch:
         return None
@@ -141,6 +170,44 @@ def pull_request_for_branch(host, repo, token, branch):
     return None
 
 
+def pull_request_for_sha(host, repo, token, sha):
+    """The PR that produced this commit, open or closed, or None.
+
+    Covers what `pull_request_for_branch` cannot: a `push` to
+    `development`/`main` is almost always a merge landing, and by the time
+    the job runs, the PR that produced it is already **closed** -- so no
+    *open* PR has the pushed branch as its head (weblib-ci#14, found via
+    weblib-viewer#27). Gitea still remembers which PR a given commit came
+    from, independent of the PR's state:
+    `GET repos/{repo}/commits/{sha}/pull`. Verified against both merge
+    styles used in this org -- an explicit merge commit (weblib-ci#13) and a
+    fast-forward promotion, where the "merge commit" is just the PR branch's
+    own tip (weblib-fs#27) -- both resolve correctly.
+
+    This needs the **full** 40-character sha. A short/abbreviated sha (the
+    kind used for display, e.g. `GITHUB_SHA[:8]`) gets exactly the same
+    `{"message": "pull request does not exist [id: 0, ...]"}` 404 shape as a
+    commit that genuinely has no PR -- so passing one silently looks
+    identical to "not found" rather than erroring. `GITHUB_SHA` itself is
+    already the full sha; this just must not be truncated before it gets
+    here.
+    """
+    if not sha:
+        return None
+    url = f"{host.rstrip('/')}/api/v1/repos/{repo}/commits/{sha}/pull"
+    req = urllib.request.Request(url)
+    req.add_header("Authorization", f"token {token}")
+    try:
+        with urllib.request.urlopen(req, timeout=30) as res:
+            pull = json.loads(res.read() or b"null")
+    except (urllib.error.URLError, OSError, ValueError):
+        # Same reasoning as pull_request_for_branch: a 404 (no PR for this
+        # commit) is the expected outcome for most calls, not an error to
+        # surface, and a reporter that raises reports nothing.
+        return None
+    return (pull or {}).get("number")
+
+
 def tail(path, limit):
     with open(path, "rb") as fh:
         try:
@@ -169,6 +236,28 @@ def comment(host, repo, number, token, body):
         return json.loads(res.read() or b"null")
 
 
+def status_description(job, phrase, raw_text, limit=STATUS_TAIL):
+    """The one-line-ish body for a commit status, when there is no PR to post
+    the full log to. `raw_text` is plain text (no code fence, no ANSI) --
+    either a log tail or `missing_log_note`'s message."""
+    snippet = (raw_text or "").strip().replace("\n", " ").strip()
+    if len(snippet) > limit:
+        snippet = "…" + snippet[-limit:]
+    return f"{job} {phrase}: {snippet}" if snippet else f"{job} {phrase}"
+
+
+def report_status(host, repo, sha, token, state, description, target_url):
+    url = f"{host.rstrip('/')}/api/v1/repos/{repo}/statuses/{sha}"
+    payload = {"context": "report_job_log", "state": state,
+               "description": description, "target_url": target_url}
+    req = urllib.request.Request(url, data=json.dumps(payload).encode(),
+                                 method="POST")
+    req.add_header("Content-Type", "application/json")
+    req.add_header("Authorization", f"token {token}")
+    with urllib.request.urlopen(req, timeout=30) as res:
+        return json.loads(res.read() or b"null")
+
+
 def main(argv=None):
     ap = argparse.ArgumentParser(description=__doc__,
                                  formatter_class=argparse.RawDescriptionHelpFormatter)
@@ -192,38 +281,70 @@ def main(argv=None):
         print("no repo or no token; set GITHUB_REPOSITORY and GITEA_TOKEN")
         return 0
 
+    full_sha = os.environ.get("GITHUB_SHA") or ""
+    sha = full_sha[:8]
+
     number = args.pr or pull_request_number(os.environ.get("GITHUB_EVENT_PATH"))
     if number is None:
         branch = (os.environ.get("GITHUB_HEAD_REF")
                   or os.environ.get("GITHUB_REF_NAME"))
         number = pull_request_for_branch(args.host, args.repo, args.token, branch)
     if number is None:
-        print("no pull request for this run: nothing to comment on")
-        return 0
+        # A push to development/main has no *open* PR for the branch it
+        # pushed to -- that branch is the target, not the one that was
+        # merged and closed. Ask Gitea which PR this exact commit came from
+        # instead (weblib-ci#14).
+        number = pull_request_for_sha(args.host, args.repo, args.token, full_sha)
 
     phrase = status_phrase(args.status)
 
-    if not os.path.exists(args.log):
-        text = missing_log_note(args.log, phrase)
+    log_exists = os.path.exists(args.log)
+    if not log_exists:
+        raw = missing_log_note(args.log, phrase)
     else:
-        text = "```\n" + tail(args.log, args.tail).rstrip() + "\n```"
+        raw = tail(args.log, args.tail).rstrip()
 
-    sha = (os.environ.get("GITHUB_SHA") or "")[:8]
-    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"
-            f"{text}\n")
+    if number is not None:
+        text = ("```\n" + raw + "\n```") if log_exists else raw
+        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"
+                f"{text}\n")
 
-    # A failure to report a failure must not itself be silent, but it also must
-    # not mask the real one: the step is already `if: failure()`.
+        # A failure to report a failure must not itself be silent, but it also
+        # must not mask the real one: the step is already `if: failure()`.
+        try:
+            result = comment(args.host, args.repo, number, args.token, body)
+        except urllib.error.HTTPError as e:
+            print(f"could not comment: {e.code} "
+                  f"{e.read()[:300].decode('utf-8', 'replace')}", file=sys.stderr)
+            return 0
+        print(f"reported to {args.repo}#{number}: {(result or {}).get('html_url')}")
+        return 0
+
+    # No PR anywhere -- a commit that was never part of one (a direct push,
+    # or the lookups above genuinely found nothing). There is still no log
+    # endpoint to point at, but a commit status is a channel that does not
+    # need a thread to post into, and this repo's Actions already post
+    # statuses that people read.
+    if not full_sha:
+        print("no pull request and no commit sha for this run: nothing to "
+              "report")
+        return 0
+
+    state = STATUS_STATE.get(phrase, "error")
+    description = status_description(args.job, phrase, raw)
+    target_url = f"{args.host.rstrip('/')}/{args.repo}/commit/{full_sha}"
     try:
-        result = comment(args.host, args.repo, number, args.token, body)
+        report_status(args.host, args.repo, full_sha, args.token, state,
+                      description, target_url)
     except urllib.error.HTTPError as e:
-        print(f"could not comment: {e.code} "
+        print(f"could not report status: {e.code} "
               f"{e.read()[:300].decode('utf-8', 'replace')}", file=sys.stderr)
         return 0
-    print(f"reported to {args.repo}#{number}: {(result or {}).get('html_url')}")
+    print(f"reported to {args.repo}@{sha} as a commit status "
+          "(no pull request found)")
     return 0
 
 

test_report_job_log.py

@@ -42,10 +42,16 @@ ACTIONS_ENV = [
 
 
 class FakeGitea(BaseHTTPRequestHandler):
-    """Just enough of the API: list open PRs, and accept a comment."""
+    """Just enough of the API: list open PRs, resolve a commit's merged PR,
+    accept a comment, and accept a commit status."""
 
     comments = []                       # class-level; reset per test
+    statuses = []
     pulls = []
+    # sha -> pull dict, mirroring GET .../commits/{sha}/pull. A sha absent
+    # from this map 404s, the same shape Gitea returns for a commit that was
+    # never part of any PR.
+    pull_for_sha = {}
 
     def _send(self, code, payload):
         raw = json.dumps(payload).encode()
@@ -56,6 +62,14 @@ class FakeGitea(BaseHTTPRequestHandler):
         self.wfile.write(raw)
 
     def do_GET(self):
+        if "/commits/" in self.path and self.path.endswith("/pull"):
+            sha = self.path.split("/commits/", 1)[1].rsplit("/pull", 1)[0]
+            pull = type(self).pull_for_sha.get(sha)
+            if pull is None:
+                return self._send(
+                    404, {"message": "pull request does not exist "
+                                      "[id: 0, issue_id: 0]"})
+            return self._send(200, pull)
         if "/pulls" in self.path:
             return self._send(200, type(self).pulls)
         self._send(404, {"message": "no"})
@@ -63,6 +77,16 @@ class FakeGitea(BaseHTTPRequestHandler):
     def do_POST(self):
         length = int(self.headers.get("Content-Length") or 0)
         payload = json.loads(self.rfile.read(length) or b"{}")
+        if "/statuses/" in self.path:
+            type(self).statuses.append({
+                "path": self.path,
+                "auth": self.headers.get("Authorization"),
+                "context": payload.get("context"),
+                "state": payload.get("state"),
+                "description": payload.get("description", ""),
+                "target_url": payload.get("target_url"),
+            })
+            return self._send(201, {"id": 1, "state": payload.get("state")})
         type(self).comments.append({
             "path": self.path,
             "auth": self.headers.get("Authorization"),
@@ -79,7 +103,9 @@ class ServerTestCase(unittest.TestCase):
 
     def setUp(self):
         FakeGitea.comments = []
+        FakeGitea.statuses = []
         FakeGitea.pulls = []
+        FakeGitea.pull_for_sha = {}
         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
@@ -115,6 +141,10 @@ class ServerTestCase(unittest.TestCase):
         """
         return FakeGitea.comments
 
+    def posted_statuses(self):
+        """Same idea as `posted()`, for the commit-status fallback."""
+        return FakeGitea.statuses
+
     def run_main(self, *argv):
         """The entry point in-process, so a traceback is readable."""
         out = io.StringIO()
@@ -293,6 +323,130 @@ class TestEndToEnd(ServerTestCase):
         self.assertIn("--status", proc.stdout)
 
 
+# A commit that landed by merging a now-closed PR into development/main --
+# weblib-ci#14's actual failure mode. A real 40-char sha, because
+# pull_request_for_sha silently treats a truncated one the same as "no PR".
+PUSH_SHA = "c9d6db67d9c20f8ab133f4fb5fea359e9c474274"
+
+
+class TestPushWithNoOpenPR(ServerTestCase):
+    """A `push` event: no event payload, no GITHUB_HEAD_REF, and the pushed
+    branch (GITHUB_REF_NAME) is the *target* of a merge, not a PR head -- the
+    exact shape of weblib-ci#14 (weblib-viewer run 1769, PR#27 merged into
+    `development`)."""
+
+    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": "publish",
+            "GITHUB_SHA": PUSH_SHA,
+            "GITHUB_REF_NAME": "development",
+        })
+        full.update(env or {})
+        return subprocess.run([sys.executable, SCRIPT, self.log, *argv],
+                              capture_output=True, text=True, env=full,
+                              cwd=HERE)
+
+    def test_falls_back_to_the_pr_the_push_merged(self):
+        """No open PR has `development` as its head -- it never does -- but
+        the merge commit itself resolves to the PR that produced it, even
+        though that PR is now closed."""
+        FakeGitea.pulls = []
+        FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 27, "state": "closed"}}
+
+        proc = self._run()
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("reported to weblib/weblib-ci#27", proc.stdout)
+
+        posted = self.posted()
+        self.assertEqual(len(posted), 1, posted)
+        self.assertEqual(posted[0]["path"],
+                         "/api/v1/repos/weblib/weblib-ci/issues/27/comments")
+        self.assertIn("DISTINCTIVE-LINE", posted[0]["body"])
+        self.assertEqual(self.posted_statuses(), [],
+                         "a resolvable PR must not also get a status")
+
+    def test_branch_lookup_still_wins_when_it_applies(self):
+        """Ordering: an *open* PR whose head matches the branch is found
+        first, so the (potentially stale) sha lookup is never even tried."""
+        FakeGitea.pulls = [{"number": 5, "head": {"ref": "development"}}]
+        FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 999}}
+
+        proc = self._run()
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("reported to weblib/weblib-ci#5", proc.stdout)
+        self.assertNotIn("999", proc.stdout)
+
+    def test_no_pr_anywhere_reports_a_commit_status(self):
+        """Not even the sha lookup finds anything -- a commit that was never
+        part of any PR. This must still report *somewhere*, not silently
+        exit 0 the way #14 did."""
+        FakeGitea.pulls = []
+        FakeGitea.pull_for_sha = {}
+
+        proc = self._run()
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn(f"reported to weblib/weblib-ci@{PUSH_SHA[:8]}",
+                      proc.stdout)
+        self.assertEqual(self.posted(), [], "no PR exists to comment on")
+
+        statuses = self.posted_statuses()
+        self.assertEqual(len(statuses), 1, statuses)
+        s = statuses[0]
+        self.assertEqual(s["path"],
+                         f"/api/v1/repos/weblib/weblib-ci/statuses/{PUSH_SHA}")
+        self.assertEqual(s["auth"], "token t0ken")
+        self.assertEqual(s["state"], "failure")
+        self.assertIn("publish", s["description"])
+        self.assertIn("DISTINCTIVE-LINE", s["description"])
+        self.assertIn(PUSH_SHA, s["target_url"])
+
+    def test_commit_status_state_follows_the_reported_outcome(self):
+        FakeGitea.pulls = []
+        FakeGitea.pull_for_sha = {}
+
+        for status, want_state in [("passed", "success"),
+                                   ("cancelled", "warning"),
+                                   ("skipped", "skipped"),
+                                   ("neutral", "error")]:
+            with self.subTest(status=status):
+                FakeGitea.statuses = []
+                proc = self._run("--status", status)
+                self.assertEqual(proc.returncode, 0, proc.stderr)
+                self.assertEqual(self.posted_statuses()[0]["state"], want_state)
+
+    def test_no_sha_and_no_pr_does_not_crash_or_post_anything(self):
+        """The one case with nowhere at all to report: still exits 0 and
+        says so, rather than raising or posting a status with an empty sha
+        in the URL."""
+        FakeGitea.pulls = []
+        FakeGitea.pull_for_sha = {}
+
+        proc = self._run(env={"GITHUB_SHA": "", "GITHUB_REF_NAME": "development"})
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("nothing to report", proc.stdout)
+        self.assertEqual(self.posted(), [])
+        self.assertEqual(self.posted_statuses(), [])
+
+    def test_existing_pr_event_path_is_unaffected(self):
+        """The ordinary pull_request-event path -- unrelated to any of the
+        push-triggered fallback -- still goes straight to the comment, and
+        never touches the sha or status endpoints."""
+        path = os.path.join(self.mkdtemp(), "event.json")
+        with open(path, "w") as fh:
+            json.dump({"pull_request": {"number": 11}}, fh)
+        FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 999}}  # must be ignored
+
+        proc = self._run(env={"GITHUB_EVENT_PATH": path})
+        self.assertEqual(proc.returncode, 0, proc.stderr)
+        self.assertIn("reported to weblib/weblib-ci#11", proc.stdout)
+        self.assertEqual(self.posted_statuses(), [])
+
+
 class TestStatusPhrase(unittest.TestCase):
     """The unit under the header, without a server."""