From c2eb401b2e10db8b12b84fb1e33ef91c7615b3b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 11:27:16 +0200 Subject: [PATCH 01/11] Say which upstream is down when an importer fails When the bio.tools import failed on 2026-09-27, the only signal was a GitHub email to whoever last touched the cron, saying "import data" had failed, and a log whose visible error was a jq message about a glob. Working out that bio.tools itself was down, and that nothing in this repository needed fixing, took reading the run log and probing the API by hand. Adds a notify job that answers that question automatically. It needs all nine importers, runs with if: always(), and for each importer that failed it re-requests that importer's own upstream and distinguishes two cases: upstream-unavailable the resource is not answering. Nothing to fix; the next run resumes once it is back. needs-attention the resource answers normally, so the failure is probably in the importer or in an API change. The verdict is carried in one open issue per resource, labelled upstream-outage and mentioning @research-software-ecosystem/rsec-ops. A new failure opens it, a change of verdict comments and refreshes it, an unchanged verdict stays silent so that a long outage does not produce a weekly alert, and a success comments and closes it. Every run also writes a table of importer outcomes to the step summary, so the run page says what happened without anyone opening a log. Deciding this by probing rather than by having the importers classify their own failures keeps the change to one job and one script: no new exit-code convention, no per-importer plumbing in utils, and it tests the actual claim at the moment of notification. The cost is that a blip which has recovered by then reads as needs-attention, so the message always carries the probe result alongside the verdict and lets the reader judge. Probe targets are each importer's real upstream, read out of the importers rather than guessed. Debian Med's is a PostgreSQL endpoint rather than a URL, so tcp:// targets are supported and connect to udd-mirror.debian.net:5432. HEAD is tried before GET because one target is a multi-megabyte archive and only the status line matters. A 4xx still counts as reachable: a status line means the host is serving, whereas 5xx, Cloudflare's 52x and a refused connection do not. Stdlib only, so the thing whose job is to report breakage has no dependency that can break, and no pip step. Needs no secrets beyond the default GITHUB_TOKEN with issues: write, which is granted on this job alone. Verified against the real upstreams while bio.tools is down: nine probes, 15.7 s, bio.tools reported HTTP 521 -> upstream-unavailable and the other eight reachable -> needs-attention, including the UDD TCP probe. The issue lifecycle was checked against a stubbed API over six cases: first failure opens, unchanged verdict stays quiet, changed verdict comments and refreshes, recovery comments and closes, success with nothing open does nothing, and a skipped job is not evaluated. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 261 ++++++++++++++++++++ .github/workflows/import.yaml | 28 +++ 2 files changed, 289 insertions(+) create mode 100644 .github/scripts/notify_upstream_failures.py diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py new file mode 100644 index 0000000000000..43490615f8472 --- /dev/null +++ b/.github/scripts/notify_upstream_failures.py @@ -0,0 +1,261 @@ +"""Tell maintainers why an importer failed, and whether it is our problem. + +Runs after the import jobs. For every importer that failed, it re-checks that +importer's upstream resource and says which of two things happened: + + upstream-unavailable the resource is down. Nothing to fix here; the import + will resume by itself. data/ was left untouched. + needs-attention the resource answers normally, so the failure is most + likely in the importer. + +The verdict is carried in one open issue per resource, labelled +`upstream-outage`. A new failure opens it, a change of verdict comments on it, +an unchanged verdict stays silent so that a long outage does not produce a +weekly alert, and a recovery comments and closes it. + +Deliberately stdlib-only: no pip install step, and no dependency that can break +the thing whose job is to tell you something broke. +""" + +import json +import os +import socket +import sys +import urllib.error +import urllib.request + +# Each importer's own upstream, taken from what the importer actually fetches. +# A tcp:// probe is for a resource that is not HTTP at all. +SOURCES = { + "biotools": ("bio.tools", ["https://bio.tools/api/tool/?page=1"]), + "openebench": ("OpenEBench", ["https://openebench.bsc.es/monitor/metrics/"]), + "bioconda": ( + "Bioconda", + ["https://codeload.github.com/bioconda/bioconda-recipes/zip/master"], + ), + "biii": ("BIII", ["http://biii.eu/"]), + "biocontainers": ( + "BioContainers", + [ + "https://raw.githubusercontent.com/BioContainers/tools-metadata/" + "master/annotations.yaml" + ], + ), + "galaxytool": ( + "Galaxy Codex", + [ + "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" + "refs/heads/main/communities/all/resources/tools.json" + ], + ), + "debian-med": ("Debian Med (UDD)", ["tcp://udd-mirror.debian.net:5432"]), + "bioconductor": ("Bioconductor", ["https://bioconductor.org/config.yaml"]), + "workflowhub": ("WorkflowHub", ["https://workflowhub.eu/workflows.json?page=1"]), +} + +LABEL = "upstream-outage" +NOTIFY_TEAM = "@research-software-ecosystem/rsec-ops" +MARKER = "", 1)[0].strip() + return None + + +def body_for(resource, verdict, probe_lines): + headline = { + "upstream-unavailable": ( + f"**{resource} is not answering.** The importer stopped before writing " + "anything, so `data/` still holds the previous import. Nothing needs " + "fixing here: the next scheduled run will pick up where it left off " + "once the resource is back." + ), + "needs-attention": ( + f"**{resource} answers normally, so this is probably not an outage.** " + "The failure is more likely in the importer or in a change to the " + f"resource's API. Someone should look at the run." + ), + }[verdict] + probes = "\n".join(f"- `{url}` -> {detail}" for url, detail in probe_lines) + return ( + f"{MARKER}{verdict} -->\n" + f"{headline}\n\n" + f"Checked just now, from the notifier job:\n{probes}\n\n" + f"Failing run: {RUN_URL}\n\n" + f"{NOTIFY_TEAM}\n\n" + "Opened automatically. It will be closed when the importer next " + "succeeds." + ) + + +def main(): + needs = json.loads(os.environ.get("JOB_RESULTS") or "{}") + if not needs: + sys.exit("JOB_RESULTS is empty; nothing to report") + + rows = [] + for job, (resource, targets) in SOURCES.items(): + result = (needs.get(job) or {}).get("result", "missing") + title = f"import failure: {resource}" + + if result == "success": + issue = find_issue(title) + if issue: + api( + "POST", + f"/repos/{REPO}/issues/{issue['number']}/comments", + {"body": f"Recovered: the importer succeeded in {RUN_URL}."}, + ) + api("PATCH", f"/repos/{REPO}/issues/{issue['number']}", {"state": "closed"}) + rows.append((job, result, "-", "recovered", f"closed #{issue['number']}")) + else: + rows.append((job, result, "-", "-", "nothing to do")) + continue + + if result in ("skipped", "cancelled", "missing"): + rows.append((job, result, "-", "-", "not evaluated")) + continue + + # Probe each target once; both the verdict and the report come from it. + checked = [(target, *probe(target)) for target in targets] + probe_lines = [(target, detail) for target, _, detail in checked] + verdict = ( + "needs-attention" + if any(ok for _, ok, _ in checked) + else "upstream-unavailable" + ) + summary = "; ".join(detail for _, detail in probe_lines) + + issue = find_issue(title) + if issue is None: + created = api( + "POST", + f"/repos/{REPO}/issues", + { + "title": title, + "body": body_for(resource, verdict, probe_lines), + "labels": [LABEL], + }, + ) + action = f"opened #{created.get('number', '?')}" + elif previous_verdict(issue) != verdict: + api( + "POST", + f"/repos/{REPO}/issues/{issue['number']}/comments", + { + "body": f"Still failing, but the picture changed " + f"(`{previous_verdict(issue)}` -> `{verdict}`).\n\n" + f"{body_for(resource, verdict, probe_lines)}" + }, + ) + api( + "PATCH", + f"/repos/{REPO}/issues/{issue['number']}", + {"body": body_for(resource, verdict, probe_lines)}, + ) + action = f"updated #{issue['number']}" + else: + action = f"#{issue['number']} already open, staying quiet" + rows.append((job, result, summary, verdict, action)) + + write_summary(rows) + + +def write_summary(rows): + lines = [ + "## Importer outcomes", + "", + "| importer | job | upstream check | verdict | action |", + "| --- | --- | --- | --- | --- |", + ] + for job, result, summary, verdict, action in rows: + lines.append(f"| {job} | {result} | {summary} | {verdict} | {action} |") + text = "\n".join(lines) + "\n" + print(text) + path = os.environ.get("GITHUB_STEP_SUMMARY") + if path: + with open(path, "a") as handle: + handle.write(text) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/import.yaml b/.github/workflows/import.yaml index 2f32d1081d20c..cc73251d6e700 100644 --- a/.github/workflows/import.yaml +++ b/.github/workflows/import.yaml @@ -347,6 +347,34 @@ jobs: commit_message: import WorkflowHub data branch: workflowhub-import-branch create_branch: true + notify: + # Runs whatever the importers did: a failure opens or updates the tracking + # issue for that resource, and a success closes it again. Parallel to + # merge-imports, so it neither delays nor depends on the merge. + runs-on: ubuntu-latest + needs: + - biotools + - openebench + - bioconda + - biii + - biocontainers + - galaxytool + - debian-med + - bioconductor + - workflowhub + if: always() + permissions: + issues: write + steps: + - name: Checkout Repository + uses: actions/checkout@v4 + with: + sparse-checkout: .github/scripts + - name: report importer outcomes + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + JOB_RESULTS: ${{ toJSON(needs) }} + run: python3 .github/scripts/notify_upstream_failures.py merge-imports: runs-on: ubuntu-latest needs: From 9733b325a19e42578735af645bf1dde67d78ea63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 22:44:21 +0200 Subject: [PATCH 02/11] Fix the notifier's permissions, label and concurrency Three findings from the review, all of them correct on the facts. The permissions one was blocking: the job declared only `issues: write`, and a job-level permissions map sets every scope it does not name to none, so actions/checkout had no contents access and the job would have failed at the checkout step before the notifier ever ran. The notifier would never have reported anything. `contents: read` is now spelled out. The repository has no `upstream-outage` label, which was confirmed against its label list. Rather than depend on what GitHub does with an unknown label on issue creation, the notifier now ensures the label exists before opening anything, and only when an importer has actually failed. find_issue() filters on that label, so an issue created without it would be invisible on the next run and a duplicate would be opened for an outage already tracked. find-then-create is not atomic, so a manual dispatch overlapping the schedule could have both runs open an issue for the same outage. Re-checking immediately before creating would narrow that window without closing it, since the two runs are on different runners, so the job takes a concurrency group instead and they serialise. Worth noting separately: import.yaml has no concurrency control at all, so two overlapping runs would already collide over the shared *-import-branch names, well before the notifier is reached. That is out of scope here but wants its own change. Verified: the six issue-lifecycle cases still pass, and a dry run against the live upstreams behaves correctly, now reporting bio.tools as reachable and the verdict as needs-attention since the outage has ended. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 31 +++++++++++++++++++++ .github/workflows/import.yaml | 9 ++++++ 2 files changed, 40 insertions(+) diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index 43490615f8472..34dc7557e2506 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -118,6 +118,34 @@ def api(method, path, payload=None): return json.loads(body) if body else {} +def ensure_label(): + """Create the tracking label if the repository does not have it yet. + + find_issue() filters on this label, so an issue created before the label + exists could be invisible on the next run, and a duplicate would be opened + for an outage already being tracked. + """ + if DRY_RUN: + print(f" [dry-run] ensure label {LABEL} exists") + return + try: + api("GET", f"/repos/{REPO}/labels/{LABEL}") + return + except urllib.error.HTTPError as exc: + if exc.code != 404: + raise + api( + "POST", + f"/repos/{REPO}/labels", + { + "name": LABEL, + "color": "d93f0b", + "description": "An upstream resource an importer depends on is unavailable", + }, + ) + print(f"created the {LABEL} label") + + _OPEN_ISSUES = None @@ -172,6 +200,9 @@ def main(): if not needs: sys.exit("JOB_RESULTS is empty; nothing to report") + if any((needs.get(job) or {}).get("result") == "failure" for job in SOURCES): + ensure_label() + rows = [] for job, (resource, targets) in SOURCES.items(): result = (needs.get(job) or {}).get("result", "missing") diff --git a/.github/workflows/import.yaml b/.github/workflows/import.yaml index cc73251d6e700..c9a65786c8a93 100644 --- a/.github/workflows/import.yaml +++ b/.github/workflows/import.yaml @@ -364,7 +364,16 @@ jobs: - workflowhub if: always() permissions: + # A job-level permissions map sets every scope it does not mention to + # none, so actions/checkout needs contents spelled out here. + contents: read issues: write + concurrency: + # find-then-create is not atomic across runners, so serialise the + # notifier: a manual dispatch overlapping the schedule would otherwise + # let two runs open duplicate issues for the same outage. + group: notify-upstream-failures + cancel-in-progress: false steps: - name: Checkout Repository uses: actions/checkout@v4 From 3c86077ae6686bb219a15a387ad9c1b841a4255f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 22:57:40 +0200 Subject: [PATCH 03/11] Probe every dependency, and let one report failing not hide the rest Two observations from the review summary, which filed no inline findings but named both. Checked against what the importers actually fetch; one was right, one was not. Right: workflowhub-import reads the galaxy_codex tools.json as well as workflowhub.eu, to map Galaxy tool IDs onto bio.tools IDs, and only workflowhub.eu was probed. An outage at raw.githubusercontent.com fails that importer while the probe reported the resource healthy, producing exactly the wrong verdict. It is probed now. galaxytool-import fetches both tools.json and workflows.json, so both are listed, though they share a host. Not right: training.galaxyproject.org is not a dependency of galaxytool-import. It appears there only in string construction for tutorial links, and the importer's only network calls are the two galaxy_codex URLs. Probing it would have reported an outage that could not affect the import. Having more than one target per source made the verdict rule wrong: it said needs-attention when *any* target answered. An importer that reads two resources fails if either is down, so one reachable host is not evidence that the fault is ours. It now requires all of them to answer. Also right: an exception while reporting on one resource aborted the loop, so the remaining importers went unreported and the step summary was never written. The per-resource work moves into handle(), each call is isolated, a failure is recorded in its row and in the exit status, and the summary is written either way. The notifier exists to report failures, so it should not be the most brittle job in the workflow. Verified: three sources where the middle one raises still report all three, the third is processed after the second blows up, and the run exits non-zero; a two-target source reads as upstream-unavailable when either target is down and needs-attention only when both answer; the six lifecycle cases still pass; and a live dry run exercises all eleven targets. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 156 ++++++++++++-------- 1 file changed, 93 insertions(+), 63 deletions(-) diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index 34dc7557e2506..ed433eacd8c1e 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -45,12 +45,23 @@ "Galaxy Codex", [ "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" - "refs/heads/main/communities/all/resources/tools.json" + "refs/heads/main/communities/all/resources/tools.json", + "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" + "refs/heads/main/communities/all/resources/workflows.json", ], ), "debian-med": ("Debian Med (UDD)", ["tcp://udd-mirror.debian.net:5432"]), "bioconductor": ("Bioconductor", ["https://bioconductor.org/config.yaml"]), - "workflowhub": ("WorkflowHub", ["https://workflowhub.eu/workflows.json?page=1"]), + # WorkflowHub's importer also reads the galaxy_codex table, to map Galaxy + # tool IDs onto bio.tools IDs, so an outage there fails it just as surely. + "workflowhub": ( + "WorkflowHub", + [ + "https://workflowhub.eu/workflows.json?page=1", + "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" + "refs/heads/main/communities/all/resources/tools.json", + ], + ), } LABEL = "upstream-outage" @@ -195,6 +206,70 @@ def body_for(resource, verdict, probe_lines): ) +def handle(job, resource, targets, result): + """Report on one importer. Returns the summary row for it.""" + title = f"import failure: {resource}" + + if result == "success": + issue = find_issue(title) + if not issue: + return (job, result, "-", "-", "nothing to do") + api( + "POST", + f"/repos/{REPO}/issues/{issue['number']}/comments", + {"body": f"Recovered: the importer succeeded in {RUN_URL}."}, + ) + api("PATCH", f"/repos/{REPO}/issues/{issue['number']}", {"state": "closed"}) + return (job, result, "-", "recovered", f"closed #{issue['number']}") + + if result in ("skipped", "cancelled", "missing"): + return (job, result, "-", "-", "not evaluated") + + # Probe each target once; both the verdict and the report come from it. + checked = [(target, *probe(target)) for target in targets] + probe_lines = [(target, detail) for target, _, detail in checked] + # Every target has to answer. An importer that reads two resources fails if + # either is down, so one reachable host is not evidence that it is our bug. + verdict = ( + "needs-attention" + if all(ok for _, ok, _ in checked) + else "upstream-unavailable" + ) + summary = "; ".join(detail for _, detail in probe_lines) + + issue = find_issue(title) + if issue is None: + created = api( + "POST", + f"/repos/{REPO}/issues", + { + "title": title, + "body": body_for(resource, verdict, probe_lines), + "labels": [LABEL], + }, + ) + action = f"opened #{created.get('number', '?')}" + elif previous_verdict(issue) != verdict: + api( + "POST", + f"/repos/{REPO}/issues/{issue['number']}/comments", + { + "body": f"Still failing, but the picture changed " + f"(`{previous_verdict(issue)}` -> `{verdict}`).\n\n" + f"{body_for(resource, verdict, probe_lines)}" + }, + ) + api( + "PATCH", + f"/repos/{REPO}/issues/{issue['number']}", + {"body": body_for(resource, verdict, probe_lines)}, + ) + action = f"updated #{issue['number']}" + else: + action = f"#{issue['number']} already open, staying quiet" + return (job, result, summary, verdict, action) + + def main(): needs = json.loads(os.environ.get("JOB_RESULTS") or "{}") if not needs: @@ -204,71 +279,26 @@ def main(): ensure_label() rows = [] + failures = [] for job, (resource, targets) in SOURCES.items(): result = (needs.get(job) or {}).get("result", "missing") - title = f"import failure: {resource}" - - if result == "success": - issue = find_issue(title) - if issue: - api( - "POST", - f"/repos/{REPO}/issues/{issue['number']}/comments", - {"body": f"Recovered: the importer succeeded in {RUN_URL}."}, - ) - api("PATCH", f"/repos/{REPO}/issues/{issue['number']}", {"state": "closed"}) - rows.append((job, result, "-", "recovered", f"closed #{issue['number']}")) - else: - rows.append((job, result, "-", "-", "nothing to do")) - continue - - if result in ("skipped", "cancelled", "missing"): - rows.append((job, result, "-", "-", "not evaluated")) - continue - - # Probe each target once; both the verdict and the report come from it. - checked = [(target, *probe(target)) for target in targets] - probe_lines = [(target, detail) for target, _, detail in checked] - verdict = ( - "needs-attention" - if any(ok for _, ok, _ in checked) - else "upstream-unavailable" - ) - summary = "; ".join(detail for _, detail in probe_lines) - - issue = find_issue(title) - if issue is None: - created = api( - "POST", - f"/repos/{REPO}/issues", - { - "title": title, - "body": body_for(resource, verdict, probe_lines), - "labels": [LABEL], - }, - ) - action = f"opened #{created.get('number', '?')}" - elif previous_verdict(issue) != verdict: - api( - "POST", - f"/repos/{REPO}/issues/{issue['number']}/comments", - { - "body": f"Still failing, but the picture changed " - f"(`{previous_verdict(issue)}` -> `{verdict}`).\n\n" - f"{body_for(resource, verdict, probe_lines)}" - }, - ) - api( - "PATCH", - f"/repos/{REPO}/issues/{issue['number']}", - {"body": body_for(resource, verdict, probe_lines)}, - ) - action = f"updated #{issue['number']}" - else: - action = f"#{issue['number']} already open, staying quiet" - rows.append((job, result, summary, verdict, action)) + try: + rows.append(handle(job, resource, targets, result)) + except Exception as exc: + # One resource failing to report must not hide the other eight, nor + # lose the summary. Record it and carry on; the exit status below + # still makes the run red. + reason = f"{type(exc).__name__}: {exc}" + print(f" ERROR reporting on {job}: {reason}") + rows.append((job, result, "-", "-", f"REPORTING FAILED - {reason[:60]}")) + failures.append((job, reason)) write_summary(rows) + if failures: + sys.exit( + "could not report on: " + + "; ".join(f"{job} ({error})" for job, error in failures) + ) def write_summary(rows): From 269cafe3d4b4177764fb4049dd0e3077e83656f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 23:06:56 +0200 Subject: [PATCH 04/11] Skip pull requests, stale runs, and probe BIII over https Four more from the review, all of them correct. The issues endpoint returns pull requests as well as issues. A PR that happened to carry this title and label would have been picked up as the tracking issue, and the recovery path would then have commented on it and closed it. Entries with a pull_request key are skipped. The BIII probe used http://biii.eu while the importer is invoked with `-td https://biii.eu` in its action.yml; the http:// form in the script is an RDF namespace, not a fetch target. The probe now uses the scheme the importer uses, so an http listener problem can no longer be reported as an outage of a resource that is serving. The concurrency group serialises notifier runs but does not order them, so a slow older run could still report after a newer one and reopen an outage the newer run had just closed. The notifier now checks whether a newer run of this workflow has started and, if so, writes its summary and makes no changes. This is a symptom of import.yaml having no concurrency control of its own -- two overlapping runs already share the *-import-branch names, which is a larger problem than a mis-filed issue -- but the notifier should not add to it. ensure_label() ran outside the failure-tolerant path, so any non-404 error reading or creating the label aborted main() before the summary was written. That contradicted the guarantee added in the previous commit, and did so precisely when the reporting path was already in trouble. Label setup is now recorded like any other reporting failure. Verified: a PR sharing the title is skipped and a PR-only result yields no match; a run that is not the newest writes its summary and mutates nothing, while the newest one still does; a 403 on label setup still produces the summary and still exits non-zero. Earlier suites pass, and a live dry run exercises all eleven targets. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 53 +++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index ed433eacd8c1e..07c053e383db3 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -33,7 +33,9 @@ "Bioconda", ["https://codeload.github.com/bioconda/bioconda-recipes/zip/master"], ), - "biii": ("BIII", ["http://biii.eu/"]), + # biii-import runs with `-td https://biii.eu`; the http:// form in its + # source is an RDF namespace, not a fetch target. + "biii": ("BIII", ["https://biii.eu/"]), "biocontainers": ( "BioContainers", [ @@ -75,6 +77,7 @@ SERVER = os.environ.get("GITHUB_SERVER_URL", "https://github.com") RUN_ID = os.environ.get("GITHUB_RUN_ID", "") RUN_URL = f"{SERVER}/{REPO}/actions/runs/{RUN_ID}" if RUN_ID else "(no run url)" +WORKFLOW_FILE = "import.yaml" DRY_RUN = os.environ.get("DRY_RUN") == "1" or not TOKEN @@ -129,6 +132,27 @@ def api(method, path, payload=None): return json.loads(body) if body else {} +def superseded_by_newer_run(): + """True when a newer run of this workflow has already started. + + Overlapping import runs are a problem in their own right, since they share + the *-import-branch names, but the notifier should at least not let a slow + older run reopen an outage that a newer run has just closed. The + concurrency group serialises us; it does not order us. + """ + if DRY_RUN or not RUN_ID: + return False + try: + runs = api( + "GET", f"/repos/{REPO}/actions/workflows/{WORKFLOW_FILE}/runs?per_page=1" + ) + except Exception as exc: # never let this check be the thing that breaks + print(f" could not check for newer runs ({exc}); proceeding") + return False + newest = (runs.get("workflow_runs") or [{}])[0].get("id") if runs else None + return bool(newest) and str(newest) != str(RUN_ID) + + def ensure_label(): """Create the tracking label if the repository does not have it yet. @@ -168,6 +192,10 @@ def find_issue(title): "GET", f"/repos/{REPO}/issues?state=open&labels={LABEL}&per_page=100" ) for issue in _OPEN_ISSUES: + # The issues endpoint returns pull requests as well. Matching one would + # mean commenting on it and closing it on the recovery path. + if "pull_request" in issue: + continue if issue.get("title") == title: return issue return None @@ -275,11 +303,28 @@ def main(): if not needs: sys.exit("JOB_RESULTS is empty; nothing to report") - if any((needs.get(job) or {}).get("result") == "failure" for job in SOURCES): - ensure_label() - rows = [] failures = [] + + if superseded_by_newer_run(): + write_summary( + [ + (job, (needs.get(job) or {}).get("result", "missing"), "-", "-", + "skipped: a newer run of this workflow has started") + for job in SOURCES + ] + ) + return + + if any((needs.get(job) or {}).get("result") == "failure" for job in SOURCES): + try: + ensure_label() + except Exception as exc: + # Do not abort here: that would skip the summary at exactly the + # moment the reporting path is in trouble. + reason = f"{type(exc).__name__}: {exc}" + print(f" ERROR ensuring the {LABEL} label: {reason}") + failures.append(("label setup", reason)) for job, (resource, targets) in SOURCES.items(): result = (needs.get(job) or {}).get("result", "missing") try: From 261a799cbb2f28f5873d76287d2d6972dec5c4fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 23:14:35 +0200 Subject: [PATCH 05/11] Add tests for the notifier, and a job to run them Every behaviour in this PR was verified with throwaway scripts that live nowhere, which is a poor arrangement for the job whose whole purpose is to tell us when the commons is broken. Twenty tests now live next to the script. Covered: what counts as reachable (a 4xx is a status line, so the host is serving; 5xx, the Cloudflare 52x codes and a refused connection are not), the tcp:// probe, the rule that every target of a source must answer before a failure is called ours, that a pull request sharing an issue's title is never treated as the tracking issue, the issue list being fetched once, the full issue lifecycle over six cases, and the three resilience properties: one resource failing to report does not hide the others, a failure setting up the label still produces a summary, and a run superseded by a newer one changes nothing while the newest still acts. Three of those cover regressions found during review, so they exist to keep them found. The suite is stdlib unittest, for the same reason the notifier is stdlib: the thing that reports breakage should not depend on something that can break. It is also fully offline -- every probe and every API call is stubbed -- so it is fast and cannot fail because an upstream happens to be down. Exercising the real probes is what DRY_RUN=1 on the notifier itself is for. The Wiring tests are the ones likely to earn their keep longest: they assert that the probe table, the jobs in import.yaml and the notify job's needs list all agree. Renaming a source without updating the workflow fails the build, which is checked by doing exactly that. test-scripts.yml runs them on pull requests touching .github/scripts or import.yaml. Tests nobody runs are decoration. Also hardens superseded_by_newer_run() against a response that is not the object it expects, which writing the harness turned up. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 5 +- .../scripts/test_notify_upstream_failures.py | 281 ++++++++++++++++++ .github/workflows/test-scripts.yml | 38 +++ 3 files changed, 323 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/test_notify_upstream_failures.py create mode 100644 .github/workflows/test-scripts.yml diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index 07c053e383db3..604819543dd7a 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -149,7 +149,10 @@ def superseded_by_newer_run(): except Exception as exc: # never let this check be the thing that breaks print(f" could not check for newer runs ({exc}); proceeding") return False - newest = (runs.get("workflow_runs") or [{}])[0].get("id") if runs else None + if not isinstance(runs, dict): + return False + listed = runs.get("workflow_runs") or [] + newest = listed[0].get("id") if listed else None return bool(newest) and str(newest) != str(RUN_ID) diff --git a/.github/scripts/test_notify_upstream_failures.py b/.github/scripts/test_notify_upstream_failures.py new file mode 100644 index 0000000000000..de9e9054ced8e --- /dev/null +++ b/.github/scripts/test_notify_upstream_failures.py @@ -0,0 +1,281 @@ +"""Tests for notify_upstream_failures.py. + + python3 -m unittest discover -s .github/scripts + +Offline by design: every probe and every GitHub call is stubbed, so the suite +is fast and cannot fail because an upstream happens to be down. To exercise the +real probes, run the notifier itself with DRY_RUN=1, which reports what it +would do without touching anything. + +Stdlib only, for the same reason the notifier is: the thing that tells you +something broke should not depend on something that can break. +""" + +import importlib +import json +import os +import re +import sys +import unittest +import unittest.mock as mock + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +# The module reads its configuration at import time. +os.environ.setdefault("GITHUB_REPOSITORY", "owner/repo") +os.environ.setdefault("GITHUB_RUN_ID", "100") +os.environ.setdefault("GITHUB_TOKEN", "x") + +import notify_upstream_failures as notifier # noqa: E402 + +WORKFLOW = os.path.join( + os.path.dirname(os.path.abspath(__file__)), os.pardir, "workflows", "import.yaml" +) + + +class Base(unittest.TestCase): + def setUp(self): + # Reload so module state, notably the cached issue list, is clean. + importlib.reload(notifier) + notifier.DRY_RUN = False + self.calls = [] + + def stub_api(self, gets=None, raises=None): + """Record every API call. `gets` maps a path regex to a GET response. + + Matching is per method on purpose: POST /issues and GET /issues share a + path, and an earlier version of this stub answered the create call with + the list response. + """ + gets = gets or {} + + def api(method, path, payload=None): + self.calls.append((method, path, payload)) + if raises and re.search(raises, path): + raise RuntimeError("GitHub said no") + if method != "GET": + return {"number": 1} + for pattern, value in gets.items(): + if re.search(pattern, path): + return value + return [] + + notifier.api = api + + def run_main(self, results): + os.environ["JOB_RESULTS"] = json.dumps( + {job: {"result": r} for job, r in results.items()} + ) + self.rows = None + notifier.write_summary = lambda rows: setattr(self, "rows", rows) + try: + notifier.main() + return 0 + except SystemExit as exc: + return 0 if exc.code in (0, None) else 1 + + def writes(self): + return [(m, p) for m, p, _ in self.calls if m in ("POST", "PATCH")] + + +class Probe(Base): + """A status line means the host is serving; 5xx and 52x mean it is not.""" + + def check(self, status, expected): + import urllib.error + + if status == "unreachable": + err = urllib.error.URLError("no route") + with mock.patch("urllib.request.urlopen", side_effect=err): + self.assertEqual(notifier.probe("https://x")[0], expected) + return + if status >= 400: + err = urllib.error.HTTPError("https://x", status, "", {}, None) + with mock.patch("urllib.request.urlopen", side_effect=err): + self.assertEqual(notifier.probe("https://x")[0], expected) + return + resp = mock.MagicMock() + resp.status = status + resp.__enter__.return_value = resp + with mock.patch("urllib.request.urlopen", return_value=resp): + self.assertEqual(notifier.probe("https://x")[0], expected) + + def test_serving_states_are_reachable(self): + for status in (200, 301, 403, 404): + with self.subTest(status=status): + self.check(status, True) + + def test_not_serving_states_are_unreachable(self): + for status in (500, 502, 521, 522, 524, "unreachable"): + with self.subTest(status=status): + self.check(status, False) + + def test_tcp_probe_reports_connect_failure(self): + import socket + + with mock.patch("socket.create_connection", side_effect=OSError("refused")): + ok, detail = notifier.probe("tcp://host:5432") + self.assertFalse(ok) + self.assertIn("TCP connect failed", detail) + with mock.patch("socket.create_connection", return_value=mock.MagicMock()): + self.assertTrue(notifier.probe("tcp://host:5432")[0]) + del socket + + +class Verdict(Base): + """An importer reading two resources fails if either is down.""" + + def verdict_for(self, probes): + notifier.SOURCES = {"x": ("X", list(probes))} + notifier.probe = lambda target: probes[target] + self.stub_api() + self.run_main({"x": "failure"}) + return self.rows[0][3] + + def test_all_targets_must_answer(self): + up, down = (True, "HTTP 200"), (False, "HTTP 521") + self.assertEqual(self.verdict_for({"a": up, "b": up}), "needs-attention") + self.assertEqual(self.verdict_for({"a": up, "b": down}), "upstream-unavailable") + self.assertEqual(self.verdict_for({"a": down, "b": down}), "upstream-unavailable") + + +class FindIssue(Base): + """The issues endpoint also returns pull requests.""" + + PR = {"number": 5, "title": "import failure: X", "pull_request": {}, "body": ""} + ISSUE = {"number": 9, "title": "import failure: X", "body": ""} + + def test_pull_request_is_never_selected(self): + self.stub_api(gets={"issues": [self.PR, self.ISSUE]}) + self.assertEqual(notifier.find_issue("import failure: X")["number"], 9) + + def test_pull_request_alone_is_no_match(self): + self.stub_api(gets={"issues": [self.PR]}) + self.assertIsNone(notifier.find_issue("import failure: X")) + + def test_issue_list_is_fetched_once(self): + self.stub_api(gets={"issues": [self.ISSUE]}) + for _ in range(3): + notifier.find_issue("import failure: X") + self.assertEqual(len([c for c in self.calls if c[0] == "GET"]), 1) + + +class Lifecycle(Base): + """One open issue per resource: open, stay quiet, update, close.""" + + def setUp(self): + super().setUp() + notifier.SOURCES = {"x": ("X", ["u"])} + notifier.probe = lambda target: (False, "HTTP 521") + + def open_issue(self, verdict="upstream-unavailable"): + return { + "number": 42, + "title": "import failure: X", + "body": f"{notifier.MARKER}{verdict} -->", + } + + def test_first_failure_opens_an_issue(self): + self.stub_api(gets={"issues": []}) + self.run_main({"x": "failure"}) + self.assertIn(("POST", f"/repos/{notifier.REPO}/issues"), self.writes()) + + def test_unchanged_verdict_stays_quiet(self): + self.stub_api(gets={r"issues\?": [self.open_issue()]}) + self.run_main({"x": "failure"}) + self.assertEqual(self.writes(), []) + + def test_changed_verdict_comments_and_refreshes(self): + self.stub_api(gets={r"issues\?": [self.open_issue("needs-attention")]}) + self.run_main({"x": "failure"}) + methods = [m for m, _ in self.writes()] + self.assertIn("POST", methods) + self.assertIn("PATCH", methods) + + def test_recovery_comments_and_closes(self): + self.stub_api(gets={r"issues\?": [self.open_issue()]}) + self.run_main({"x": "success"}) + self.assertTrue(any("comments" in p for _, p in self.writes())) + self.assertTrue( + any(m == "PATCH" for m, _ in self.writes()), "issue should be closed" + ) + + def test_success_with_nothing_open_does_nothing(self): + self.stub_api(gets={"issues": []}) + self.run_main({"x": "success"}) + self.assertEqual(self.writes(), []) + + def test_skipped_job_is_not_evaluated(self): + self.stub_api(gets={"issues": []}) + self.run_main({"x": "skipped"}) + self.assertEqual(self.writes(), []) + self.assertEqual(self.rows[0][4], "not evaluated") + + +class Resilience(Base): + """Reporting is the job; it must not be the most brittle part of the run.""" + + def test_one_failure_does_not_hide_the_others(self): + notifier.SOURCES = {k: (k.upper(), ["u"]) for k in ("a", "b", "c")} + notifier.probe = lambda target: (False, "HTTP 521") + self.stub_api(gets={"issues": []}, raises=r"issues$") + code = self.run_main({k: "failure" for k in notifier.SOURCES}) + self.assertEqual([r[0] for r in self.rows], ["a", "b", "c"]) + self.assertEqual(code, 1, "the run must still go red") + + def test_label_failure_still_writes_the_summary(self): + notifier.SOURCES = {"x": ("X", ["u"])} + notifier.probe = lambda target: (False, "HTTP 521") + self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 100}]}}, raises=r"/labels") + code = self.run_main({"x": "failure"}) + self.assertIsNotNone(self.rows, "summary must be written anyway") + self.assertEqual(code, 1) + + def test_a_superseded_run_changes_nothing(self): + notifier.SOURCES = {"x": ("X", ["u"])} + notifier.probe = lambda target: (False, "HTTP 521") + self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 999}]}}) + self.run_main({"x": "failure"}) + self.assertEqual(self.writes(), []) + self.assertIn("newer run", self.rows[0][4]) + + def test_the_newest_run_does_act(self): + notifier.SOURCES = {"x": ("X", ["u"])} + notifier.probe = lambda target: (False, "HTTP 521") + self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 100}]}, "issues": []}) + self.run_main({"x": "failure"}) + self.assertNotEqual(self.writes(), []) + + +class Wiring(unittest.TestCase): + """The probe table and the workflow must not drift apart.""" + + def setUp(self): + # Other classes mutate SOURCES on the module object; reload to see the + # real table rather than whatever ran last. + importlib.reload(notifier) + + def workflow_jobs(self): + text = open(WORKFLOW).read() + body = text.split("\njobs:\n", 1)[1] + return {m.group(1) for m in re.finditer(r"^ ([a-z0-9][a-z0-9-]*):$", body, re.M)} + + def notify_needs(self): + text = open(WORKFLOW).read() + block = text.split("\n notify:\n", 1)[1].split("\n if:", 1)[0] + return {m.group(1) for m in re.finditer(r"^ - ([a-z0-9-]+)$", block, re.M)} + + def test_every_probed_source_is_a_job(self): + self.assertEqual(set(notifier.SOURCES) - self.workflow_jobs(), set()) + + def test_notify_waits_for_every_probed_source(self): + self.assertEqual(set(notifier.SOURCES), self.notify_needs()) + + def test_every_source_has_at_least_one_target(self): + for name, (_, targets) in notifier.SOURCES.items(): + with self.subTest(source=name): + self.assertTrue(targets) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml new file mode 100644 index 0000000000000..f77717576a1c0 --- /dev/null +++ b/.github/workflows/test-scripts.yml @@ -0,0 +1,38 @@ +name: test .github/scripts + +# The notifier is what reports that an import broke, so it should not be the +# one thing in the workflow with no tests. The suite is stdlib-only and +# offline, so it needs no dependencies and cannot fail because an upstream +# happens to be down. + +on: + pull_request: + paths: + - ".github/scripts/**" + - ".github/workflows/import.yaml" + - ".github/workflows/test-scripts.yml" + push: + branches: [master] + paths: + - ".github/scripts/**" + - ".github/workflows/import.yaml" + +permissions: + contents: read + +jobs: + unittest: + runs-on: ubuntu-latest + steps: + - name: Checkout Repository + uses: actions/checkout@v4 + with: + # The suite also checks that the probe table and import.yaml agree, + # so it needs the workflows directory as well as the scripts. + sparse-checkout: .github + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Run the tests + run: python3 -m unittest discover -s .github/scripts -p 'test_*.py' -v From d55dc4764a07a03afd8efd845ee3c8677cd62d41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 23:15:03 +0200 Subject: [PATCH 06/11] Ignore Python bytecode Running the new tests in .github/scripts leaves __pycache__ behind, and the repository did not ignore it. Co-Authored-By: Claude Opus 5 --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index 1a774039b3c9b..b89c0d8f42bb2 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,7 @@ venv .DS-_Store + +# Python bytecode, from running the tests in .github/scripts +__pycache__/ +*.py[cod] From 462583ae4bba73ab0729a4207a3429b5ee0d4acd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Mon, 28 Sep 2026 23:16:27 +0200 Subject: [PATCH 07/11] Pin the test environment so the suite does not read the runner's The new job failed on its first run while the same tests passed locally, which is exactly the kind of thing it was added to catch. The suite used os.environ.setdefault for GITHUB_RUN_ID. On a runner that variable is already set to the real run, so setdefault left it alone and the tests ran against it: the stubbed "newest run" id of 100 never matched, every run looked superseded, and the two tests that depend on a run acting failed. Locally the variable is absent, the default applied, and everything passed. The variables are now assigned rather than defaulted, DRY_RUN is cleared so an ambient value cannot change the path under test, and GITHUB_STEP_SUMMARY is removed so a stray call cannot append to the runner's real job summary. Reproduced by running the suite with a real GITHUB_RUN_ID, a token and a summary path in the environment: it fails before this change and passes after, leaving the summary file untouched. Also closes the workflow file the Wiring tests read, which the runner reported as a ResourceWarning. Co-Authored-By: Claude Opus 5 --- .../scripts/test_notify_upstream_failures.py | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/.github/scripts/test_notify_upstream_failures.py b/.github/scripts/test_notify_upstream_failures.py index de9e9054ced8e..92704351ec0ac 100644 --- a/.github/scripts/test_notify_upstream_failures.py +++ b/.github/scripts/test_notify_upstream_failures.py @@ -21,10 +21,17 @@ sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) -# The module reads its configuration at import time. -os.environ.setdefault("GITHUB_REPOSITORY", "owner/repo") -os.environ.setdefault("GITHUB_RUN_ID", "100") -os.environ.setdefault("GITHUB_TOKEN", "x") +# The module reads its configuration at import time. These are set rather than +# defaulted: on a runner the real GITHUB_RUN_ID is already present, and with +# setdefault the suite silently tested against it -- every run then looked +# superseded by a newer one, which passed locally and failed in CI. +os.environ["GITHUB_REPOSITORY"] = "owner/repo" +os.environ["GITHUB_RUN_ID"] = "100" +os.environ["GITHUB_TOKEN"] = "x" +os.environ["GITHUB_SERVER_URL"] = "https://github.com" +os.environ.pop("DRY_RUN", None) +# Never append to the runner's real job summary. +os.environ.pop("GITHUB_STEP_SUMMARY", None) import notify_upstream_failures as notifier # noqa: E402 @@ -255,13 +262,18 @@ def setUp(self): # real table rather than whatever ran last. importlib.reload(notifier) + @staticmethod + def read_workflow(): + with open(WORKFLOW) as handle: + return handle.read() + def workflow_jobs(self): - text = open(WORKFLOW).read() + text = self.read_workflow() body = text.split("\njobs:\n", 1)[1] return {m.group(1) for m in re.finditer(r"^ ([a-z0-9][a-z0-9-]*):$", body, re.M)} def notify_needs(self): - text = open(WORKFLOW).read() + text = self.read_workflow() block = text.split("\n notify:\n", 1)[1].split("\n if:", 1)[0] return {m.group(1) for m in re.finditer(r"^ - ([a-z0-9-]+)$", block, re.M)} From bb3044603b4ef3ad9c0443b03676cc9adb96528b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Tue, 29 Sep 2026 08:34:39 +0200 Subject: [PATCH 08/11] Grant actions: read, and test that the scopes stay granted Both review points are right, and the first is the same mistake twice. The notifier lists this workflow's runs to decide whether a newer one has started, which needs `actions: read`. The job-level permissions map named only contents and issues, so that request would have been refused. The guard catches its own error and carries on, so the effect was not a failure but a silence: stale-run protection looked present and did nothing. This is the same class of fault as the missing `contents: read` found earlier in this PR, in a scope added after that one was fixed. So rather than only adding the scope, there is now a test asserting that the notify job grants every scope the script can reach, with the reason for each. Dropping `actions: read` fails the suite, which is checked by doing exactly that. A permissions bug that degrades silently at runtime should be a build failure instead. The fallback also now says what it is giving up -- "proceeding WITHOUT stale-run protection" -- rather than reporting the error alone, since a 403 there is a configuration fault and not a blip. test-scripts.yml's push filter did not include the workflow itself, so a change touching only that file could reach master without the suite running, even though the pull_request filter covered it. The two filters now match. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 7 +++++- .../scripts/test_notify_upstream_failures.py | 23 +++++++++++++++++++ .github/workflows/import.yaml | 9 +++++++- .github/workflows/test-scripts.yml | 1 + 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index 604819543dd7a..ffa3b0ec07397 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -147,7 +147,12 @@ def superseded_by_newer_run(): "GET", f"/repos/{REPO}/actions/workflows/{WORKFLOW_FILE}/runs?per_page=1" ) except Exception as exc: # never let this check be the thing that breaks - print(f" could not check for newer runs ({exc}); proceeding") + # A 403 here means the job is missing `actions: read`, which is a + # configuration fault rather than a blip, so name what is lost. + print( + f" could not check for newer runs ({exc}); proceeding WITHOUT " + "stale-run protection" + ) return False if not isinstance(runs, dict): return False diff --git a/.github/scripts/test_notify_upstream_failures.py b/.github/scripts/test_notify_upstream_failures.py index 92704351ec0ac..8b81999f7f584 100644 --- a/.github/scripts/test_notify_upstream_failures.py +++ b/.github/scripts/test_notify_upstream_failures.py @@ -283,6 +283,29 @@ def test_every_probed_source_is_a_job(self): def test_notify_waits_for_every_probed_source(self): self.assertEqual(set(notifier.SOURCES), self.notify_needs()) + def notify_permissions(self): + text = self.read_workflow() + block = text.split("\n notify:\n", 1)[1].split("\n concurrency:", 1)[0] + return dict(re.findall(r"^ (\w+): (read|write)$", block, re.M)) + + def test_notify_job_grants_every_scope_the_script_needs(self): + """A job-level permissions map denies everything it does not name. + + Each of these is reachable from the script, and a missing one fails at + runtime in a way that is easy to miss: the stale-run check catches its + own 403 and carries on, so losing `actions` degrades the guard silently + rather than failing the job. + """ + needed = { + "contents": "actions/checkout", + "issues": "read, open, comment, close, and label the tracking issue", + "actions": "list this workflow's runs for the stale-run guard", + } + granted = self.notify_permissions() + for scope, why in needed.items(): + with self.subTest(scope=scope): + self.assertIn(scope, granted, f"{scope} is needed to {why}") + def test_every_source_has_at_least_one_target(self): for name, (_, targets) in notifier.SOURCES.items(): with self.subTest(source=name): diff --git a/.github/workflows/import.yaml b/.github/workflows/import.yaml index c9a65786c8a93..0eda013b1f926 100644 --- a/.github/workflows/import.yaml +++ b/.github/workflows/import.yaml @@ -365,9 +365,16 @@ jobs: if: always() permissions: # A job-level permissions map sets every scope it does not mention to - # none, so actions/checkout needs contents spelled out here. + # none, so each one the notifier needs is spelled out: + # contents actions/checkout + # issues reading, opening, commenting on and closing the tracking + # issue, and creating its label + # actions listing this workflow's runs, to tell whether a newer one + # has started. Without it that check is refused and the + # stale-run guard silently does nothing. contents: read issues: write + actions: read concurrency: # find-then-create is not atomic across runners, so serialise the # notifier: a manual dispatch overlapping the schedule would otherwise diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml index f77717576a1c0..4944076e9e7f2 100644 --- a/.github/workflows/test-scripts.yml +++ b/.github/workflows/test-scripts.yml @@ -16,6 +16,7 @@ on: paths: - ".github/scripts/**" - ".github/workflows/import.yaml" + - ".github/workflows/test-scripts.yml" permissions: contents: read From a141891a3b27b7c121db5267e20e49d67b19212f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Tue, 29 Sep 2026 09:04:19 +0200 Subject: [PATCH 09/11] Do not open issues while the label is unconfirmed A hole in this PR's own mitigation, correctly spotted. ensure_label() exists so the notifier does not depend on what GitHub does with an unknown label at issue-creation time. When it failed, the run recorded the failure and carried on into handle(), which created an issue with labels: [upstream-outage] anyway -- depending on exactly that behaviour again. If the label is dropped rather than created, the issue is unlabelled, find_issue() filters on the label and never sees it, and a fresh issue is opened every week: silent duplicates, which is the failure the label machinery was added to prevent. Issue creation is now held back until the label is confirmed. The run still probes, still reports, still writes the summary and still exits non-zero; the row says the issue was not opened and why. Recovery is unaffected, since anything find_issue() can see was labelled when it was opened. Two tests cover it: a failing label call opens nothing and says so, and a recovery still closes an existing issue in the same conditions. Verified by re-allowing creation, which fails the suite. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 25 +++++++++++++++--- .../scripts/test_notify_upstream_failures.py | 26 +++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py index ffa3b0ec07397..fecb3bb12e3f7 100644 --- a/.github/scripts/notify_upstream_failures.py +++ b/.github/scripts/notify_upstream_failures.py @@ -242,8 +242,14 @@ def body_for(resource, verdict, probe_lines): ) -def handle(job, resource, targets, result): - """Report on one importer. Returns the summary row for it.""" +def handle(job, resource, targets, result, label_ready=True): + """Report on one importer. Returns the summary row for it. + + With label_ready false the label could not be confirmed, so no new issue is + opened: find_issue() filters on that label, and an issue created without it + would be invisible next run and duplicated every run after. Recovery still + runs, since anything it finds was labelled when it was opened. + """ title = f"import failure: {resource}" if result == "success": @@ -275,6 +281,14 @@ def handle(job, resource, targets, result): issue = find_issue(title) if issue is None: + if not label_ready: + return ( + job, + result, + summary, + verdict, + "not opened: the label could not be confirmed", + ) created = api( "POST", f"/repos/{REPO}/issues", @@ -324,19 +338,22 @@ def main(): ) return + label_ready = True if any((needs.get(job) or {}).get("result") == "failure" for job in SOURCES): try: ensure_label() except Exception as exc: # Do not abort here: that would skip the summary at exactly the - # moment the reporting path is in trouble. + # moment the reporting path is in trouble. Opening issues is held + # back instead, so a run cannot leave unlabelled ones behind. + label_ready = False reason = f"{type(exc).__name__}: {exc}" print(f" ERROR ensuring the {LABEL} label: {reason}") failures.append(("label setup", reason)) for job, (resource, targets) in SOURCES.items(): result = (needs.get(job) or {}).get("result", "missing") try: - rows.append(handle(job, resource, targets, result)) + rows.append(handle(job, resource, targets, result, label_ready)) except Exception as exc: # One resource failing to report must not hide the other eight, nor # lose the summary. Record it and carry on; the exit status below diff --git a/.github/scripts/test_notify_upstream_failures.py b/.github/scripts/test_notify_upstream_failures.py index 8b81999f7f584..5a5530eaeff9a 100644 --- a/.github/scripts/test_notify_upstream_failures.py +++ b/.github/scripts/test_notify_upstream_failures.py @@ -238,6 +238,32 @@ def test_label_failure_still_writes_the_summary(self): self.assertIsNotNone(self.rows, "summary must be written anyway") self.assertEqual(code, 1) + def test_label_failure_holds_back_new_issues(self): + """An unlabelled issue would be invisible next run, and duplicated.""" + notifier.SOURCES = {"x": ("X", ["u"])} + notifier.probe = lambda target: (False, "HTTP 521") + self.stub_api( + gets={"workflows": {"workflow_runs": [{"id": 100}]}}, raises=r"/labels" + ) + code = self.run_main({"x": "failure"}) + self.assertEqual( + [(m, p) for m, p in self.writes() if p.endswith("/issues")], + [], + "no issue may be opened while the label is unconfirmed", + ) + self.assertIn("label could not be confirmed", self.rows[0][4]) + self.assertEqual(code, 1) + + def test_label_failure_still_closes_a_recovered_issue(self): + """Anything findable was labelled when it was opened, so recovery runs.""" + notifier.SOURCES = {"x": ("X", ["u"])} + open_issue = {"number": 42, "title": "import failure: X", "body": ""} + self.stub_api( + gets={"workflows": {"workflow_runs": [{"id": 100}]}, r"issues\?": [open_issue]} + ) + self.run_main({"x": "success"}) + self.assertTrue(any("comments" in p for _, p in self.writes())) + def test_a_superseded_run_changes_nothing(self): notifier.SOURCES = {"x": ("X", ["u"])} notifier.probe = lambda target: (False, "HTTP 521") From ba371254020d854b315634b5ac36f2fd2cd8857e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Tue, 29 Sep 2026 09:25:12 +0200 Subject: [PATCH 10/11] Move the notifier into utils and call it as an action The script was sitting in .github/scripts here, which is not where this repository keeps that kind of code: every importer it reports on lives in research-software-ecosystem/utils as a folder with an action.yml, called from this workflow. The notifier now does the same, as research-software-ecosystem/utils#68, and this PR is only the job that calls it. What moves with it: the script, its 27 tests and the workflow that runs them. utils runs the suite on any change to that folder, so the tests did not go anywhere, they just run next to the code they cover. Two checks had to change shape, since the tests can no longer read this workflow to compare it against the probe table: - The table is now checked against `job-results` at runtime. A job listed in `needs` with no upstream configured is reported as unprobed, and if it failed the notifier exits non-zero, rather than a red import being followed by a green notifier that says nothing about it. - A missing `actions: read` here degrades the stale-run guard silently. The action now says so in the job summary. The checkout step is gone: the action brings its own script, so this job needs no `contents` scope either. Co-Authored-By: Claude Opus 5 --- .github/scripts/notify_upstream_failures.py | 392 ------------------ .../scripts/test_notify_upstream_failures.py | 342 --------------- .github/workflows/import.yaml | 33 +- .github/workflows/test-scripts.yml | 39 -- 4 files changed, 17 insertions(+), 789 deletions(-) delete mode 100644 .github/scripts/notify_upstream_failures.py delete mode 100644 .github/scripts/test_notify_upstream_failures.py delete mode 100644 .github/workflows/test-scripts.yml diff --git a/.github/scripts/notify_upstream_failures.py b/.github/scripts/notify_upstream_failures.py deleted file mode 100644 index fecb3bb12e3f7..0000000000000 --- a/.github/scripts/notify_upstream_failures.py +++ /dev/null @@ -1,392 +0,0 @@ -"""Tell maintainers why an importer failed, and whether it is our problem. - -Runs after the import jobs. For every importer that failed, it re-checks that -importer's upstream resource and says which of two things happened: - - upstream-unavailable the resource is down. Nothing to fix here; the import - will resume by itself. data/ was left untouched. - needs-attention the resource answers normally, so the failure is most - likely in the importer. - -The verdict is carried in one open issue per resource, labelled -`upstream-outage`. A new failure opens it, a change of verdict comments on it, -an unchanged verdict stays silent so that a long outage does not produce a -weekly alert, and a recovery comments and closes it. - -Deliberately stdlib-only: no pip install step, and no dependency that can break -the thing whose job is to tell you something broke. -""" - -import json -import os -import socket -import sys -import urllib.error -import urllib.request - -# Each importer's own upstream, taken from what the importer actually fetches. -# A tcp:// probe is for a resource that is not HTTP at all. -SOURCES = { - "biotools": ("bio.tools", ["https://bio.tools/api/tool/?page=1"]), - "openebench": ("OpenEBench", ["https://openebench.bsc.es/monitor/metrics/"]), - "bioconda": ( - "Bioconda", - ["https://codeload.github.com/bioconda/bioconda-recipes/zip/master"], - ), - # biii-import runs with `-td https://biii.eu`; the http:// form in its - # source is an RDF namespace, not a fetch target. - "biii": ("BIII", ["https://biii.eu/"]), - "biocontainers": ( - "BioContainers", - [ - "https://raw.githubusercontent.com/BioContainers/tools-metadata/" - "master/annotations.yaml" - ], - ), - "galaxytool": ( - "Galaxy Codex", - [ - "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" - "refs/heads/main/communities/all/resources/tools.json", - "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" - "refs/heads/main/communities/all/resources/workflows.json", - ], - ), - "debian-med": ("Debian Med (UDD)", ["tcp://udd-mirror.debian.net:5432"]), - "bioconductor": ("Bioconductor", ["https://bioconductor.org/config.yaml"]), - # WorkflowHub's importer also reads the galaxy_codex table, to map Galaxy - # tool IDs onto bio.tools IDs, so an outage there fails it just as surely. - "workflowhub": ( - "WorkflowHub", - [ - "https://workflowhub.eu/workflows.json?page=1", - "https://raw.githubusercontent.com/galaxyproject/galaxy_codex/" - "refs/heads/main/communities/all/resources/tools.json", - ], - ), -} - -LABEL = "upstream-outage" -NOTIFY_TEAM = "@research-software-ecosystem/rsec-ops" -MARKER = "", 1)[0].strip() - return None - - -def body_for(resource, verdict, probe_lines): - headline = { - "upstream-unavailable": ( - f"**{resource} is not answering.** The importer stopped before writing " - "anything, so `data/` still holds the previous import. Nothing needs " - "fixing here: the next scheduled run will pick up where it left off " - "once the resource is back." - ), - "needs-attention": ( - f"**{resource} answers normally, so this is probably not an outage.** " - "The failure is more likely in the importer or in a change to the " - f"resource's API. Someone should look at the run." - ), - }[verdict] - probes = "\n".join(f"- `{url}` -> {detail}" for url, detail in probe_lines) - return ( - f"{MARKER}{verdict} -->\n" - f"{headline}\n\n" - f"Checked just now, from the notifier job:\n{probes}\n\n" - f"Failing run: {RUN_URL}\n\n" - f"{NOTIFY_TEAM}\n\n" - "Opened automatically. It will be closed when the importer next " - "succeeds." - ) - - -def handle(job, resource, targets, result, label_ready=True): - """Report on one importer. Returns the summary row for it. - - With label_ready false the label could not be confirmed, so no new issue is - opened: find_issue() filters on that label, and an issue created without it - would be invisible next run and duplicated every run after. Recovery still - runs, since anything it finds was labelled when it was opened. - """ - title = f"import failure: {resource}" - - if result == "success": - issue = find_issue(title) - if not issue: - return (job, result, "-", "-", "nothing to do") - api( - "POST", - f"/repos/{REPO}/issues/{issue['number']}/comments", - {"body": f"Recovered: the importer succeeded in {RUN_URL}."}, - ) - api("PATCH", f"/repos/{REPO}/issues/{issue['number']}", {"state": "closed"}) - return (job, result, "-", "recovered", f"closed #{issue['number']}") - - if result in ("skipped", "cancelled", "missing"): - return (job, result, "-", "-", "not evaluated") - - # Probe each target once; both the verdict and the report come from it. - checked = [(target, *probe(target)) for target in targets] - probe_lines = [(target, detail) for target, _, detail in checked] - # Every target has to answer. An importer that reads two resources fails if - # either is down, so one reachable host is not evidence that it is our bug. - verdict = ( - "needs-attention" - if all(ok for _, ok, _ in checked) - else "upstream-unavailable" - ) - summary = "; ".join(detail for _, detail in probe_lines) - - issue = find_issue(title) - if issue is None: - if not label_ready: - return ( - job, - result, - summary, - verdict, - "not opened: the label could not be confirmed", - ) - created = api( - "POST", - f"/repos/{REPO}/issues", - { - "title": title, - "body": body_for(resource, verdict, probe_lines), - "labels": [LABEL], - }, - ) - action = f"opened #{created.get('number', '?')}" - elif previous_verdict(issue) != verdict: - api( - "POST", - f"/repos/{REPO}/issues/{issue['number']}/comments", - { - "body": f"Still failing, but the picture changed " - f"(`{previous_verdict(issue)}` -> `{verdict}`).\n\n" - f"{body_for(resource, verdict, probe_lines)}" - }, - ) - api( - "PATCH", - f"/repos/{REPO}/issues/{issue['number']}", - {"body": body_for(resource, verdict, probe_lines)}, - ) - action = f"updated #{issue['number']}" - else: - action = f"#{issue['number']} already open, staying quiet" - return (job, result, summary, verdict, action) - - -def main(): - needs = json.loads(os.environ.get("JOB_RESULTS") or "{}") - if not needs: - sys.exit("JOB_RESULTS is empty; nothing to report") - - rows = [] - failures = [] - - if superseded_by_newer_run(): - write_summary( - [ - (job, (needs.get(job) or {}).get("result", "missing"), "-", "-", - "skipped: a newer run of this workflow has started") - for job in SOURCES - ] - ) - return - - label_ready = True - if any((needs.get(job) or {}).get("result") == "failure" for job in SOURCES): - try: - ensure_label() - except Exception as exc: - # Do not abort here: that would skip the summary at exactly the - # moment the reporting path is in trouble. Opening issues is held - # back instead, so a run cannot leave unlabelled ones behind. - label_ready = False - reason = f"{type(exc).__name__}: {exc}" - print(f" ERROR ensuring the {LABEL} label: {reason}") - failures.append(("label setup", reason)) - for job, (resource, targets) in SOURCES.items(): - result = (needs.get(job) or {}).get("result", "missing") - try: - rows.append(handle(job, resource, targets, result, label_ready)) - except Exception as exc: - # One resource failing to report must not hide the other eight, nor - # lose the summary. Record it and carry on; the exit status below - # still makes the run red. - reason = f"{type(exc).__name__}: {exc}" - print(f" ERROR reporting on {job}: {reason}") - rows.append((job, result, "-", "-", f"REPORTING FAILED - {reason[:60]}")) - failures.append((job, reason)) - - write_summary(rows) - if failures: - sys.exit( - "could not report on: " - + "; ".join(f"{job} ({error})" for job, error in failures) - ) - - -def write_summary(rows): - lines = [ - "## Importer outcomes", - "", - "| importer | job | upstream check | verdict | action |", - "| --- | --- | --- | --- | --- |", - ] - for job, result, summary, verdict, action in rows: - lines.append(f"| {job} | {result} | {summary} | {verdict} | {action} |") - text = "\n".join(lines) + "\n" - print(text) - path = os.environ.get("GITHUB_STEP_SUMMARY") - if path: - with open(path, "a") as handle: - handle.write(text) - - -if __name__ == "__main__": - main() diff --git a/.github/scripts/test_notify_upstream_failures.py b/.github/scripts/test_notify_upstream_failures.py deleted file mode 100644 index 5a5530eaeff9a..0000000000000 --- a/.github/scripts/test_notify_upstream_failures.py +++ /dev/null @@ -1,342 +0,0 @@ -"""Tests for notify_upstream_failures.py. - - python3 -m unittest discover -s .github/scripts - -Offline by design: every probe and every GitHub call is stubbed, so the suite -is fast and cannot fail because an upstream happens to be down. To exercise the -real probes, run the notifier itself with DRY_RUN=1, which reports what it -would do without touching anything. - -Stdlib only, for the same reason the notifier is: the thing that tells you -something broke should not depend on something that can break. -""" - -import importlib -import json -import os -import re -import sys -import unittest -import unittest.mock as mock - -sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) - -# The module reads its configuration at import time. These are set rather than -# defaulted: on a runner the real GITHUB_RUN_ID is already present, and with -# setdefault the suite silently tested against it -- every run then looked -# superseded by a newer one, which passed locally and failed in CI. -os.environ["GITHUB_REPOSITORY"] = "owner/repo" -os.environ["GITHUB_RUN_ID"] = "100" -os.environ["GITHUB_TOKEN"] = "x" -os.environ["GITHUB_SERVER_URL"] = "https://github.com" -os.environ.pop("DRY_RUN", None) -# Never append to the runner's real job summary. -os.environ.pop("GITHUB_STEP_SUMMARY", None) - -import notify_upstream_failures as notifier # noqa: E402 - -WORKFLOW = os.path.join( - os.path.dirname(os.path.abspath(__file__)), os.pardir, "workflows", "import.yaml" -) - - -class Base(unittest.TestCase): - def setUp(self): - # Reload so module state, notably the cached issue list, is clean. - importlib.reload(notifier) - notifier.DRY_RUN = False - self.calls = [] - - def stub_api(self, gets=None, raises=None): - """Record every API call. `gets` maps a path regex to a GET response. - - Matching is per method on purpose: POST /issues and GET /issues share a - path, and an earlier version of this stub answered the create call with - the list response. - """ - gets = gets or {} - - def api(method, path, payload=None): - self.calls.append((method, path, payload)) - if raises and re.search(raises, path): - raise RuntimeError("GitHub said no") - if method != "GET": - return {"number": 1} - for pattern, value in gets.items(): - if re.search(pattern, path): - return value - return [] - - notifier.api = api - - def run_main(self, results): - os.environ["JOB_RESULTS"] = json.dumps( - {job: {"result": r} for job, r in results.items()} - ) - self.rows = None - notifier.write_summary = lambda rows: setattr(self, "rows", rows) - try: - notifier.main() - return 0 - except SystemExit as exc: - return 0 if exc.code in (0, None) else 1 - - def writes(self): - return [(m, p) for m, p, _ in self.calls if m in ("POST", "PATCH")] - - -class Probe(Base): - """A status line means the host is serving; 5xx and 52x mean it is not.""" - - def check(self, status, expected): - import urllib.error - - if status == "unreachable": - err = urllib.error.URLError("no route") - with mock.patch("urllib.request.urlopen", side_effect=err): - self.assertEqual(notifier.probe("https://x")[0], expected) - return - if status >= 400: - err = urllib.error.HTTPError("https://x", status, "", {}, None) - with mock.patch("urllib.request.urlopen", side_effect=err): - self.assertEqual(notifier.probe("https://x")[0], expected) - return - resp = mock.MagicMock() - resp.status = status - resp.__enter__.return_value = resp - with mock.patch("urllib.request.urlopen", return_value=resp): - self.assertEqual(notifier.probe("https://x")[0], expected) - - def test_serving_states_are_reachable(self): - for status in (200, 301, 403, 404): - with self.subTest(status=status): - self.check(status, True) - - def test_not_serving_states_are_unreachable(self): - for status in (500, 502, 521, 522, 524, "unreachable"): - with self.subTest(status=status): - self.check(status, False) - - def test_tcp_probe_reports_connect_failure(self): - import socket - - with mock.patch("socket.create_connection", side_effect=OSError("refused")): - ok, detail = notifier.probe("tcp://host:5432") - self.assertFalse(ok) - self.assertIn("TCP connect failed", detail) - with mock.patch("socket.create_connection", return_value=mock.MagicMock()): - self.assertTrue(notifier.probe("tcp://host:5432")[0]) - del socket - - -class Verdict(Base): - """An importer reading two resources fails if either is down.""" - - def verdict_for(self, probes): - notifier.SOURCES = {"x": ("X", list(probes))} - notifier.probe = lambda target: probes[target] - self.stub_api() - self.run_main({"x": "failure"}) - return self.rows[0][3] - - def test_all_targets_must_answer(self): - up, down = (True, "HTTP 200"), (False, "HTTP 521") - self.assertEqual(self.verdict_for({"a": up, "b": up}), "needs-attention") - self.assertEqual(self.verdict_for({"a": up, "b": down}), "upstream-unavailable") - self.assertEqual(self.verdict_for({"a": down, "b": down}), "upstream-unavailable") - - -class FindIssue(Base): - """The issues endpoint also returns pull requests.""" - - PR = {"number": 5, "title": "import failure: X", "pull_request": {}, "body": ""} - ISSUE = {"number": 9, "title": "import failure: X", "body": ""} - - def test_pull_request_is_never_selected(self): - self.stub_api(gets={"issues": [self.PR, self.ISSUE]}) - self.assertEqual(notifier.find_issue("import failure: X")["number"], 9) - - def test_pull_request_alone_is_no_match(self): - self.stub_api(gets={"issues": [self.PR]}) - self.assertIsNone(notifier.find_issue("import failure: X")) - - def test_issue_list_is_fetched_once(self): - self.stub_api(gets={"issues": [self.ISSUE]}) - for _ in range(3): - notifier.find_issue("import failure: X") - self.assertEqual(len([c for c in self.calls if c[0] == "GET"]), 1) - - -class Lifecycle(Base): - """One open issue per resource: open, stay quiet, update, close.""" - - def setUp(self): - super().setUp() - notifier.SOURCES = {"x": ("X", ["u"])} - notifier.probe = lambda target: (False, "HTTP 521") - - def open_issue(self, verdict="upstream-unavailable"): - return { - "number": 42, - "title": "import failure: X", - "body": f"{notifier.MARKER}{verdict} -->", - } - - def test_first_failure_opens_an_issue(self): - self.stub_api(gets={"issues": []}) - self.run_main({"x": "failure"}) - self.assertIn(("POST", f"/repos/{notifier.REPO}/issues"), self.writes()) - - def test_unchanged_verdict_stays_quiet(self): - self.stub_api(gets={r"issues\?": [self.open_issue()]}) - self.run_main({"x": "failure"}) - self.assertEqual(self.writes(), []) - - def test_changed_verdict_comments_and_refreshes(self): - self.stub_api(gets={r"issues\?": [self.open_issue("needs-attention")]}) - self.run_main({"x": "failure"}) - methods = [m for m, _ in self.writes()] - self.assertIn("POST", methods) - self.assertIn("PATCH", methods) - - def test_recovery_comments_and_closes(self): - self.stub_api(gets={r"issues\?": [self.open_issue()]}) - self.run_main({"x": "success"}) - self.assertTrue(any("comments" in p for _, p in self.writes())) - self.assertTrue( - any(m == "PATCH" for m, _ in self.writes()), "issue should be closed" - ) - - def test_success_with_nothing_open_does_nothing(self): - self.stub_api(gets={"issues": []}) - self.run_main({"x": "success"}) - self.assertEqual(self.writes(), []) - - def test_skipped_job_is_not_evaluated(self): - self.stub_api(gets={"issues": []}) - self.run_main({"x": "skipped"}) - self.assertEqual(self.writes(), []) - self.assertEqual(self.rows[0][4], "not evaluated") - - -class Resilience(Base): - """Reporting is the job; it must not be the most brittle part of the run.""" - - def test_one_failure_does_not_hide_the_others(self): - notifier.SOURCES = {k: (k.upper(), ["u"]) for k in ("a", "b", "c")} - notifier.probe = lambda target: (False, "HTTP 521") - self.stub_api(gets={"issues": []}, raises=r"issues$") - code = self.run_main({k: "failure" for k in notifier.SOURCES}) - self.assertEqual([r[0] for r in self.rows], ["a", "b", "c"]) - self.assertEqual(code, 1, "the run must still go red") - - def test_label_failure_still_writes_the_summary(self): - notifier.SOURCES = {"x": ("X", ["u"])} - notifier.probe = lambda target: (False, "HTTP 521") - self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 100}]}}, raises=r"/labels") - code = self.run_main({"x": "failure"}) - self.assertIsNotNone(self.rows, "summary must be written anyway") - self.assertEqual(code, 1) - - def test_label_failure_holds_back_new_issues(self): - """An unlabelled issue would be invisible next run, and duplicated.""" - notifier.SOURCES = {"x": ("X", ["u"])} - notifier.probe = lambda target: (False, "HTTP 521") - self.stub_api( - gets={"workflows": {"workflow_runs": [{"id": 100}]}}, raises=r"/labels" - ) - code = self.run_main({"x": "failure"}) - self.assertEqual( - [(m, p) for m, p in self.writes() if p.endswith("/issues")], - [], - "no issue may be opened while the label is unconfirmed", - ) - self.assertIn("label could not be confirmed", self.rows[0][4]) - self.assertEqual(code, 1) - - def test_label_failure_still_closes_a_recovered_issue(self): - """Anything findable was labelled when it was opened, so recovery runs.""" - notifier.SOURCES = {"x": ("X", ["u"])} - open_issue = {"number": 42, "title": "import failure: X", "body": ""} - self.stub_api( - gets={"workflows": {"workflow_runs": [{"id": 100}]}, r"issues\?": [open_issue]} - ) - self.run_main({"x": "success"}) - self.assertTrue(any("comments" in p for _, p in self.writes())) - - def test_a_superseded_run_changes_nothing(self): - notifier.SOURCES = {"x": ("X", ["u"])} - notifier.probe = lambda target: (False, "HTTP 521") - self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 999}]}}) - self.run_main({"x": "failure"}) - self.assertEqual(self.writes(), []) - self.assertIn("newer run", self.rows[0][4]) - - def test_the_newest_run_does_act(self): - notifier.SOURCES = {"x": ("X", ["u"])} - notifier.probe = lambda target: (False, "HTTP 521") - self.stub_api(gets={"workflows": {"workflow_runs": [{"id": 100}]}, "issues": []}) - self.run_main({"x": "failure"}) - self.assertNotEqual(self.writes(), []) - - -class Wiring(unittest.TestCase): - """The probe table and the workflow must not drift apart.""" - - def setUp(self): - # Other classes mutate SOURCES on the module object; reload to see the - # real table rather than whatever ran last. - importlib.reload(notifier) - - @staticmethod - def read_workflow(): - with open(WORKFLOW) as handle: - return handle.read() - - def workflow_jobs(self): - text = self.read_workflow() - body = text.split("\njobs:\n", 1)[1] - return {m.group(1) for m in re.finditer(r"^ ([a-z0-9][a-z0-9-]*):$", body, re.M)} - - def notify_needs(self): - text = self.read_workflow() - block = text.split("\n notify:\n", 1)[1].split("\n if:", 1)[0] - return {m.group(1) for m in re.finditer(r"^ - ([a-z0-9-]+)$", block, re.M)} - - def test_every_probed_source_is_a_job(self): - self.assertEqual(set(notifier.SOURCES) - self.workflow_jobs(), set()) - - def test_notify_waits_for_every_probed_source(self): - self.assertEqual(set(notifier.SOURCES), self.notify_needs()) - - def notify_permissions(self): - text = self.read_workflow() - block = text.split("\n notify:\n", 1)[1].split("\n concurrency:", 1)[0] - return dict(re.findall(r"^ (\w+): (read|write)$", block, re.M)) - - def test_notify_job_grants_every_scope_the_script_needs(self): - """A job-level permissions map denies everything it does not name. - - Each of these is reachable from the script, and a missing one fails at - runtime in a way that is easy to miss: the stale-run check catches its - own 403 and carries on, so losing `actions` degrades the guard silently - rather than failing the job. - """ - needed = { - "contents": "actions/checkout", - "issues": "read, open, comment, close, and label the tracking issue", - "actions": "list this workflow's runs for the stale-run guard", - } - granted = self.notify_permissions() - for scope, why in needed.items(): - with self.subTest(scope=scope): - self.assertIn(scope, granted, f"{scope} is needed to {why}") - - def test_every_source_has_at_least_one_target(self): - for name, (_, targets) in notifier.SOURCES.items(): - with self.subTest(source=name): - self.assertTrue(targets) - - -if __name__ == "__main__": - unittest.main(verbosity=2) diff --git a/.github/workflows/import.yaml b/.github/workflows/import.yaml index 0eda013b1f926..4efa2b3598533 100644 --- a/.github/workflows/import.yaml +++ b/.github/workflows/import.yaml @@ -365,14 +365,15 @@ jobs: if: always() permissions: # A job-level permissions map sets every scope it does not mention to - # none, so each one the notifier needs is spelled out: - # contents actions/checkout - # issues reading, opening, commenting on and closing the tracking - # issue, and creating its label - # actions listing this workflow's runs, to tell whether a newer one - # has started. Without it that check is refused and the - # stale-run guard silently does nothing. - contents: read + # none, so each one the action needs is spelled out: + # issues reading, opening, commenting on and closing the tracking + # issue, and creating its label + # actions listing this workflow's runs, to tell whether a newer one + # has started. Without it that check is refused; the action + # carries on and says so in the job summary, but the + # stale-run guard is gone. + # `contents` is not needed: there is no checkout, since the action + # brings its own script. issues: write actions: read concurrency: @@ -382,15 +383,15 @@ jobs: group: notify-upstream-failures cancel-in-progress: false steps: - - name: Checkout Repository - uses: actions/checkout@v4 - with: - sparse-checkout: .github/scripts - name: report importer outcomes - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - JOB_RESULTS: ${{ toJSON(needs) }} - run: python3 .github/scripts/notify_upstream_failures.py + uses: research-software-ecosystem/utils/notify-upstream-failures@main + with: + repo-token: ${{ secrets.GITHUB_TOKEN }} + # The keys are this job's `needs`, which the action matches against + # its table of upstream resources. A job missing from that table is + # reported as unprobed rather than passing unnoticed. + job-results: ${{ toJSON(needs) }} + workflow-file: import.yaml merge-imports: runs-on: ubuntu-latest needs: diff --git a/.github/workflows/test-scripts.yml b/.github/workflows/test-scripts.yml deleted file mode 100644 index 4944076e9e7f2..0000000000000 --- a/.github/workflows/test-scripts.yml +++ /dev/null @@ -1,39 +0,0 @@ -name: test .github/scripts - -# The notifier is what reports that an import broke, so it should not be the -# one thing in the workflow with no tests. The suite is stdlib-only and -# offline, so it needs no dependencies and cannot fail because an upstream -# happens to be down. - -on: - pull_request: - paths: - - ".github/scripts/**" - - ".github/workflows/import.yaml" - - ".github/workflows/test-scripts.yml" - push: - branches: [master] - paths: - - ".github/scripts/**" - - ".github/workflows/import.yaml" - - ".github/workflows/test-scripts.yml" - -permissions: - contents: read - -jobs: - unittest: - runs-on: ubuntu-latest - steps: - - name: Checkout Repository - uses: actions/checkout@v4 - with: - # The suite also checks that the probe table and import.yaml agree, - # so it needs the workflows directory as well as the scripts. - sparse-checkout: .github - - name: Setup Python - uses: actions/setup-python@v5 - with: - python-version: "3.12" - - name: Run the tests - run: python3 -m unittest discover -s .github/scripts -p 'test_*.py' -v From bdc02200b3b54a1619ac2b401c4f0b0a474f6d1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herv=C3=A9=20M=C3=A9nager?= Date: Tue, 29 Sep 2026 09:25:54 +0200 Subject: [PATCH 11/11] Drop the __pycache__ ignore along with the tests It was added for the test suite in .github/scripts, which now lives in research-software-ecosystem/utils -- whose .gitignore already covers it. Co-Authored-By: Claude Opus 5 --- .gitignore | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.gitignore b/.gitignore index b89c0d8f42bb2..1a774039b3c9b 100644 --- a/.gitignore +++ b/.gitignore @@ -3,7 +3,3 @@ venv .DS-_Store - -# Python bytecode, from running the tests in .github/scripts -__pycache__/ -*.py[cod]