diff --git a/.github/workflows/report-failures.yaml b/.github/workflows/report-failures.yaml index 7483ac6..f161b7e 100644 --- a/.github/workflows/report-failures.yaml +++ b/.github/workflows/report-failures.yaml @@ -6,6 +6,7 @@ on: workflow_run: workflows: - 'Check inbound changes' + - 'Code review' types: [completed] permissions: @@ -18,6 +19,6 @@ jobs: name: Report to Slack uses: ./.github/workflows/slack-on-failure.yaml with: - events: '["schedule", "push", "release"]' + events: '["schedule", "push", "release", "pull_request"]' secrets: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index e1d486e..b27c71f 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -1,24 +1,27 @@ name: 'Report a failed run to Slack' # Called from a repository's watcher workflow, which is started by `workflow_run` when one of the -# workflows it names completes. A run nobody is watching - started by a schedule, a push to the -# default branch or a release - that fails is otherwise one red entry in a list nobody opens; +# workflows it names completes. A failed run is otherwise one red entry in a list nobody opens; # this posts it to the channel behind `SLACK_WEBHOOK` instead. # # The caller says which triggers to report with `events`. A push is reported on the default # branch and on a tag, which is how a release is often started; a push to any other branch has -# its pusher watching. Only a completed run is reported, whatever `types:` the caller gives +# its pusher watching. A pull request from a fork is not reported: its run uses the fork's own +# workflow file, so its author writes the job names and branch the post would carry, and could +# put text of their choosing in the channel through this webhook. The same reasoning has the +# review refuse forks. Only a completed run is reported, whatever `types:` the caller gives # `workflow_run`, and with every conclusion but success, skipped and neutral, so a conclusion # nobody anticipated is posted rather than passed over. The filters are here, not in each # caller, so no caller can get them wrong. # -# A post Slack does not accept fails this job, so a broken webhook is a red run of the watcher -# rather than a failure that silently reached nobody. +# A post Slack does not accept fails this job, with Slack's answer in the error. Nothing +# reports the watcher's own run, so a webhook that stops working is noticed only by the absence +# of posts: an accepted limitation (README.md). on: workflow_call: inputs: events: - description: 'The triggers whose failed runs are reported, as a JSON list of event names, such as `["schedule", "push", "release"]`. `pull_request` is accepted but posts every red push to every pull request.' + description: 'The triggers whose failed runs are reported, as a JSON list of event names, such as `["schedule", "push", "release", "pull_request"]`. A manual run, `workflow_dispatch`, is best left out, because whoever started it is watching.' type: string required: true secrets: @@ -34,6 +37,8 @@ jobs: github.event_name != 'workflow_run' || (github.event.action == 'completed' && contains(fromJSON(inputs.events), github.event.workflow_run.event) && + (github.event.workflow_run.event != 'pull_request' || + github.event.workflow_run.head_repository.full_name == github.repository) && !contains(fromJSON('["success", "skipped", "neutral"]'), github.event.workflow_run.conclusion)) runs-on: [ubuntu-latest] name: Report the failure to Slack diff --git a/README.md b/README.md index f7a27d3..ad300de 100644 --- a/README.md +++ b/README.md @@ -6,10 +6,10 @@ the bumps. | Path | What it is | Call it as | |---|---|---| -| `actions/repo-hygiene` | Composite action: the conventions every repository shares - no internal references, every CI job bounded, every unattended run reporting its failures, the instruction layer intact, the detail layer routed. | a step, after `actions/checkout` | +| `actions/repo-hygiene` | Composite action: the conventions every repository shares - no internal references, every CI job bounded, every failed run reported, the instruction layer intact, the detail layer routed. | a step, after `actions/checkout` | | `actions/action-pins` | Composite action: every `uses:` names a 40-hex commit SHA. | a step, after `actions/checkout` | | `.github/workflows/claude-review.yaml` | Reusable workflow: a code review by Claude, full on the first round and a delta of the new commits after that. | a job with `uses:` | -| `.github/workflows/slack-on-failure.yaml` | Reusable workflow: posts a failed run nobody was watching to Slack, naming the workflow and its failed jobs and linking the run. | a job with `uses:`, in a workflow triggered by `workflow_run` | +| `.github/workflows/slack-on-failure.yaml` | Reusable workflow: posts a failed run to Slack, naming the workflow and its failed jobs and linking the run. | a job with `uses:`, in a workflow triggered by `workflow_run` | ## Using them @@ -101,9 +101,10 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. ## Reporting failed runs to Slack -A run started by a schedule, a push or a release has nobody watching it, so its failure is -posted to Slack instead. Each repository has one watcher workflow, `report-failures.yaml`, -naming the workflows to report; adding such a workflow means adding its `name:` there. +A failed run is otherwise one red entry in a list nobody opens, so a failure started by a +schedule, a push, a release or a pull request is posted to Slack instead. Each repository has +one watcher workflow, `report-failures.yaml`, naming the workflows to report; adding such a +workflow means adding its `name:` there. ```yaml name: 'Report failures' @@ -111,6 +112,7 @@ on: workflow_run: workflows: - 'Check inbound changes' + - 'Code review' - 'Release' - 'Nightly fuzzing' types: [completed] @@ -122,7 +124,7 @@ jobs: name: Report to Slack uses: L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@ # vX.Y.Z with: - events: '["schedule", "push", "release"]' + events: '["schedule", "push", "release", "pull_request"]' secrets: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} ``` @@ -130,25 +132,30 @@ jobs: - **What is reported.** A completed run of a listed workflow started by one of the `events`, with any conclusion but success, skipped or neutral, so a cancelled or timed-out run is posted as well as a failed one. A push is reported on the default branch and on a tag; a - push to another branch has its pusher watching. A pull request's run is watched by its - author and a manual run by whoever started it, so neither belongs in `events`. The post - names the repository, the workflow, its trigger, its conclusion and the jobs that did not - succeed, and links the run. + push to another branch has its pusher watching. A manual run is watched by whoever started + it, so it does not belong in `events`. A pull request from a fork is not posted: its run + uses the fork's own workflow file, so its author writes the job names and branch a post + would carry, and could put text of their choosing in the channel. Its failure still shows + on the pull request. The post names the repository, the workflow, its trigger, its + conclusion and the jobs that did not succeed, and links the run. - **A cancelled run that a newer one replaced is not posted**, as when `cancel-in-progress` cancels the first of two merges landing together: the newer run is reported if it fails. **Accepted limitation:** a run cancelled by its timeout while a newer run of the same workflow was already queued looks the same, so it is not posted either. -- **The hygiene check holds the list.** It fails on a workflow triggered by `schedule`, `push` - or `release` that no watcher reports for that trigger, on one with no `name:`, and on a - listed name no workflow has, since GitHub matches on the name and a rename leaves the list - reporting nothing. +- **The hygiene check holds the list.** It fails on a workflow triggered by `schedule`, `push`, + `release` or `pull_request` that no watcher reports for that trigger, on one with no `name:`, + and on a listed name no workflow has, since GitHub matches on the name and a rename leaves the + list reporting nothing. - **Where it goes.** To the channel of the incoming webhook in the `SLACK_WEBHOOK` secret. A secret set on the organisation is every repository's default, and a repository secret of the same name overrides it. An incoming webhook posts to one channel and can do nothing else. - **A post that fails, fails the watcher's run**, with Slack's answer in the error: a missing - secret, a webhook Slack refuses, or Slack unreachable after retries. So a broken webhook shows - as a red watcher run rather than a failure that reached nobody. A transient refusal is - retried, which can post the same failure twice. + secret, a webhook Slack refuses, or Slack unreachable after retries. A transient refusal is + retried, which can post the same failure twice. **Accepted limitation:** the watcher's run is + itself a run nobody is watching, and nothing reports it, so a webhook that stops working is + noticed only by the absence of posts. Filing an issue instead would scatter them across + repositories if filed in the failing one, and if filed here would need either a credential + that can write across repositories or a scheduled sweep of every repository's watcher runs. - **The watcher only fires from the default branch**, where GitHub reads `workflow_run` triggers. So a change to it is tested only once merged. - **The caller must grant** `actions: read` and `contents: read`, as the example does, or diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index 7867bd3..858eb8a 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -106,10 +106,10 @@ BINARY_SUFFIXES = (".png", ".jpg", ".jpeg", ".gif", ".ico", ".pdf", ".gz", ".zip", ".deb", ".whl") -# --- unattended runs report their failures -------------------------------------------------- -# The triggers of a run nobody is watching. A pull request's run is watched by its author, and -# a manual run by whoever started it. -UNATTENDED = ("schedule", "push", "release") +# --- failed runs are reported ----------------------------------------------------------------- +# The triggers whose failed runs are posted to Slack. A manual run is left out, because whoever +# started it is watching. +REPORTED = ("schedule", "push", "release", "pull_request") # A call to the shared Slack reporter, from another repository or from this one. REPORTER = re.compile( @@ -522,8 +522,8 @@ def workflow_triggers(relative, document): raise CannotEvaluate(f"{relative} has no `on:` this check can read (got {on!r})") -def unattended_run_findings(documents): - """The findings of check_unattended_runs_report_failures, over parsed workflows. +def reported_run_findings(documents): + """The findings of check_failed_runs_are_reported, over parsed workflows. Args: documents (list): `(repo-relative path, document)` pairs, as workflow_documents returns. @@ -567,7 +567,7 @@ def unattended_run_findings(documents): if not (isinstance(events, list) and all(isinstance(e, str) for e in events)): findings.append( f"{relative} passes the Slack reporter `events: {raw!r}`, which is not a JSON " - f"list of event names such as '[\"schedule\", \"push\", \"release\"]'" + f"list of event names such as '[\"schedule\", \"push\", \"release\", \"pull_request\"]'" ) continue for name in names: @@ -580,22 +580,22 @@ def unattended_run_findings(documents): name = document.get("name") if isinstance(name, str): named.add(name) - unattended = workflow_triggers(relative, document) & set(UNATTENDED) - if not unattended: + triggers = workflow_triggers(relative, document) & set(REPORTED) + if not triggers: continue if not isinstance(name, str) or not name: findings.append( - f"{relative} runs on {', '.join(sorted(unattended))} but has no `name:`, which a " + f"{relative} runs on {', '.join(sorted(triggers))} but has no `name:`, which a " f"watcher's `workflows:` list matches on" ) continue - missing = unattended - reported.get(name, set()) + missing = triggers - reported.get(name, set()) if missing: where = " or ".join(str(w) for w in watchers) if watchers else "a watcher workflow" findings.append( - f"{relative} ({name!r}) runs on {', '.join(sorted(missing))} unattended, and " - f"{where} does not report its failures there: list it in `workflows:` and those " - f"triggers in `events:`" + f"{relative} ({name!r}) runs on {', '.join(sorted(missing))}, and {where} does " + f"not report its failures there: list it in `workflows:` and those triggers in " + f"`events:`" ) for name in sorted(set(reported) - named): findings.append( @@ -605,13 +605,12 @@ def unattended_run_findings(documents): return findings -def check_unattended_runs_report_failures(root, tracked, files): - """Every workflow that runs with nobody watching has its failures posted to Slack. +def check_failed_runs_are_reported(root, tracked, files): + """Every workflow started by a schedule, a push, a release or a pull request reports failures. - A run started by a schedule, a push or a release has no pull request to show its failure - on, so a failure is one red entry in a list nobody opens. Each such workflow must be named - in a watcher - a workflow triggered by `workflow_run` that calls the shared Slack reporter - - with each of its unattended triggers among the `events` the watcher reports. A name in a + A failed run is otherwise one red entry in a list nobody opens, so each such workflow must + be named in a watcher - a workflow triggered by `workflow_run` that calls the shared Slack + reporter - with each of those triggers among the `events` the watcher reports. A name in a watcher's list that no workflow has is reported too, because GitHub matches on the name and a rename leaves the list reporting nothing. @@ -627,7 +626,7 @@ def check_unattended_runs_report_failures(root, tracked, files): CannotEvaluate: A workflow could not be read or parsed, via workflow_documents, or its `on:` could not be read. """ - return unattended_run_findings(workflow_documents(root, tracked)) + return reported_run_findings(workflow_documents(root, tracked)) # Markdown links plus backtick-quoted paths, since a router routes both ways. @@ -833,7 +832,7 @@ def check_the_detail_layer_routing(root, tracked, files): ("the scan is real", check_the_scan_is_real), ("no internal references in a public repository", check_no_internal_references), ("every CI job is bounded", check_ci_jobs_are_bounded), - ("unattended runs report their failures", check_unattended_runs_report_failures), + ("failed runs are reported", check_failed_runs_are_reported), ("the instruction layer is intact", check_the_instruction_layer), ("the detail layer and the router agree", check_the_detail_layer_routing), )