From d548d2ad874b0d9afecd00375bad70a5c27de856 Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:18:56 +0100 Subject: [PATCH 1/7] Add a reusable workflow that posts failed scheduled runs to Slack A scheduled run has nobody watching it, so when it fails the only record is a red entry in a list of runs nobody opens. Each repository gets one small watcher, triggered by `workflow_run` on the workflows it names, that calls this; the filter on schedule-started runs and on the conclusion lives here so no caller can get it wrong. Every conclusion but success, skipped and neutral is reported, so a conclusion nobody anticipated is posted rather than passed over. A post that fails - no secret, a webhook Slack refuses, Slack unreachable - fails the watcher's run with Slack's answer, so a broken webhook is visible rather than silent. A call from anything but `workflow_run` fails in its first step instead of being skipped. Slack's body is written to a file rather than standard output: run against a server that answered 500 and then 200, curl's retry appended the second body to the first, giving `server_errorok`, and a post Slack had accepted was reported as refused. Written to a file, the retry truncates it. Tested locally by running the step's script against a fake Slack server: accepted, refused with 404, refused with 200 and an error body, 500 then 200, unreachable, no secret, a call from `push`, the jobs listing failing, and no failed jobs, with `&`, `<` and `>` in the workflow name. The job-level filter and a real post are untested until a repository's watcher is on its default branch, which is the only place `workflow_run` fires. Signed-off-by: Gavin Lucas --- .github/workflows/slack-on-failure.yaml | 94 +++++++++++++++++++++++++ README.md | 40 +++++++++++ 2 files changed, 134 insertions(+) create mode 100644 .github/workflows/slack-on-failure.yaml diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml new file mode 100644 index 0000000..84b69ed --- /dev/null +++ b/.github/workflows/slack-on-failure.yaml @@ -0,0 +1,94 @@ +name: 'Report a failed scheduled 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 failed scheduled run is otherwise one red entry in a list +# nobody opens; this posts it to the channel behind `SLACK_WEBHOOK` instead. +# +# Only scheduled runs are reported: a pull request or a push has a person watching it already, +# and reporting those would bury the scheduled failures this exists for. Every conclusion but +# success, skipped and neutral is reported, so a conclusion nobody anticipated is posted rather +# than passed over. The filter is here, not in each caller, so no caller can get it 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. +on: + workflow_call: + secrets: + SLACK_WEBHOOK: + description: 'A Slack incoming webhook URL, which posts to the one channel it was issued for. Not required, so that a missing secret fails with a message naming it rather than GitHub refusing the call.' + required: false + +jobs: + report: + # Also runs when called from anything but `workflow_run`, so that a misconfigured caller + # fails in its first step instead of being skipped in silence. + if: >- + github.event_name != 'workflow_run' || + (github.event.workflow_run.event == 'schedule' && + !contains(fromJSON('["success", "skipped", "neutral"]'), github.event.workflow_run.conclusion)) + runs-on: [ubuntu-latest] + name: Report the failure to Slack + timeout-minutes: 5 + permissions: + # To name the jobs that failed. + actions: read + steps: + - name: Post the failed run + env: + SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} + GH_TOKEN: ${{ github.token }} + EVENT_NAME: ${{ github.event_name }} + REPO: ${{ github.repository }} + WORKFLOW: ${{ github.event.workflow_run.name }} + CONCLUSION: ${{ github.event.workflow_run.conclusion }} + RUN_ID: ${{ github.event.workflow_run.id }} + RUN_NUMBER: ${{ github.event.workflow_run.run_number }} + RUN_ATTEMPT: ${{ github.event.workflow_run.run_attempt }} + RUN_URL: ${{ github.event.workflow_run.html_url }} + BRANCH: ${{ github.event.workflow_run.head_branch }} + SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + if [ "$EVENT_NAME" != workflow_run ]; then + echo "::error::This workflow reports a run that \`workflow_run\` names, and was called from a \`$EVENT_NAME\` event. Call it from a workflow triggered by \`workflow_run\`." >&2 + exit 1 + fi + if [ -z "$SLACK_WEBHOOK" ]; then + echo "::error::No SLACK_WEBHOOK secret reached this workflow, so the failure of $WORKFLOW (run $RUN_URL) was not posted. Set the secret on the repository or the organisation, and pass it in the caller's \`secrets:\`." >&2 + exit 1 + fi + + # Naming the failed jobs saves opening the run to find them, but the post does not + # depend on it: if they cannot be listed, the post says so and goes anyway. + if jobs=$(gh api "repos/$REPO/actions/runs/$RUN_ID/attempts/$RUN_ATTEMPT/jobs?per_page=100" \ + --jq '[.jobs[] | select(.conclusion != "success" and .conclusion != "skipped") | "\(.name) (\(.conclusion))"] | join(", ")' 2>&1); then + jobs=${jobs:-none listed} + else + echo "::warning::Could not list the jobs of run $RUN_ID: $jobs" + jobs="could not be listed: $jobs" + fi + + # Slack reads &, < and > as markup in message text. + escape() { sed -e 's/&/\&/g' -e 's//\>/g' <<<"$1"; } + text="Scheduled run failed in $(escape "$REPO"): *$(escape "$WORKFLOW")* ended \`$(escape "$CONCLUSION")\`. + Jobs: $(escape "$jobs") + <$RUN_URL|Run $RUN_NUMBER, attempt $RUN_ATTEMPT> on \`$(escape "$BRANCH")\` at \`${SHA:0:12}\`" + + # The body goes to a file because a retry truncates a file, where to standard output it + # would follow the refused attempt's body. + body_file=$(mktemp) + status=$(jq -n --arg text "$text" '{text: $text}' \ + | curl --silent --show-error --max-time 30 --retry 3 \ + --header 'Content-Type: application/json' --data @- \ + --output "$body_file" --write-out '%{http_code}' "$SLACK_WEBHOOK") || { + echo "::error::Posting the failure of $WORKFLOW (run $RUN_URL) to Slack failed: curl could not complete the request." >&2 + exit 1 + } + body=$(cat "$body_file") + # Slack answers a post it accepts with 200 and the body `ok`, and anything else with an + # error code in the body, such as `invalid_token` or `channel_not_found`. + if [ "$status" != 200 ] || [ "$body" != ok ]; then + echo "::error::Slack refused the post about $WORKFLOW (run $RUN_URL): HTTP $status, $body" >&2 + exit 1 + fi + echo "Posted the failure of $WORKFLOW (run $RUN_URL) to Slack." diff --git a/README.md b/README.md index 7387a46..cb6673e 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,7 @@ the bumps. | `actions/repo-hygiene` | Composite action: the conventions every repository shares - no internal references, every CI job bounded, 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 scheduled 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 @@ -92,6 +93,45 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. - **The caller needs** `contents: read` and `pull-requests: write`, and must pass the secret explicitly. +## Reporting failed scheduled runs to Slack + +A scheduled run has nobody watching it, so its failure is posted to Slack instead. Each +repository has one small watcher workflow naming the workflows to report; adding a scheduled +workflow means adding its name there. + +```yaml +name: 'Report scheduled failures' +on: + workflow_run: + workflows: ['Nightly fuzzing', 'Mutation testing'] + types: [completed] +permissions: + actions: read +jobs: + slack: + uses: L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@ # vX.Y.Z + secrets: + SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} +``` + +- **What is reported.** Runs started by a schedule, with any conclusion but success, skipped or + neutral, so a cancelled or timed-out run is posted as well as a failed one. A run started any + other way is skipped: someone started it and is watching it. The post names the repository, + the workflow, its conclusion and the jobs that did not succeed, and links the run. +- **Where it goes.** To the channel of the incoming webhook in the `SLACK_WEBHOOK` secret. The + organisation's secret posts to the organisation's channel, and a repository with a channel of + its own sets a repository secret of the same name. 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. +- **The watcher only fires from the default branch**, where GitHub reads `workflow_run` + triggers, which is where schedules run too. So it is tested only once merged: by a scheduled + workflow that fails on purpose, removed again once it has posted. +- **The caller needs** `actions: read`, to list the failed jobs; without it the post still + goes, saying the jobs could not be listed. + ## Running the hygiene check locally From the root of the repository to check, fetch the script at the commit that repository's From 550955211cb1d99b5432447547a3f28566063dbd Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:39:49 +0100 Subject: [PATCH 2/7] Report any unattended failed run, and check every repository reports its own A run started by a push to the default branch or by a release has nobody watching it, just as a scheduled one has not, so the reporter now takes `events`, the triggers to report, instead of reporting schedules alone. A push is reported only on the default branch, because a push to any other branch has its pusher watching. The post now names the trigger. The hygiene check gains "unattended runs report their failures": every workflow triggered by `schedule`, `push` or `release` must be named in a watcher that calls the reporter with that trigger among its events, must have a `name:` for the watcher to match, and every name in a watcher must belong to a workflow, since GitHub matches on the name and a rename otherwise leaves the list reporting nothing. It is in the shared check because the convention is every repository's, and because the repositories adopt it by moving their pin, so the pin move and the watcher land together. Parsing the workflows moves out of `workflow_jobs` into `workflow_documents` so both checks share it. This repository gets a watcher too, calling the reporter by local path, for its own pushes to main; it has no repository secret, so it posts to the organisation's channel. Tested: the check's function against fourteen fixtures - a clean repository, a local call, pull-request-only workflows, `on:` as a list and as a string, no watcher, a workflow left off, a trigger missing from `events`, a stale name, no `name:`, `events` that is not JSON, a watcher with no `workflows:`, another reusable workflow mistaken for none, and no `on:` (refused) - and the whole script over a fresh clone of each repository that pins this one, where it names exactly the push, schedule and release workflows each has. The post step was re-run against the fake Slack server. The job-level filter is still untested until a watcher fires on a default branch. Signed-off-by: Gavin Lucas --- .github/workflows/report-failures.yaml | 22 +++ .github/workflows/slack-on-failure.yaml | 28 ++-- README.md | 46 ++++-- actions/repo-hygiene/check-repo-hygiene.py | 180 ++++++++++++++++++++- 4 files changed, 243 insertions(+), 33 deletions(-) create mode 100644 .github/workflows/report-failures.yaml diff --git a/.github/workflows/report-failures.yaml b/.github/workflows/report-failures.yaml new file mode 100644 index 0000000..6131eb4 --- /dev/null +++ b/.github/workflows/report-failures.yaml @@ -0,0 +1,22 @@ +name: 'Report failures' + +# Posts a failed run of a workflow named below, started by one of the events below, to Slack +# through this repository's own reporter on the default branch (README.md). +on: + workflow_run: + workflows: + - 'Check inbound changes' + types: [completed] + +permissions: + # Read by the reporter to name the jobs that failed. + actions: read + +jobs: + slack: + name: Report to Slack + uses: ./.github/workflows/slack-on-failure.yaml + with: + events: '["schedule", "push", "release"]' + secrets: + SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index 84b69ed..8b4f1c6 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -1,18 +1,25 @@ -name: 'Report a failed scheduled run to Slack' +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 failed scheduled run is otherwise one red entry in a list -# nobody opens; this posts it to the channel behind `SLACK_WEBHOOK` instead. +# 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; +# this posts it to the channel behind `SLACK_WEBHOOK` instead. # -# Only scheduled runs are reported: a pull request or a push has a person watching it already, -# and reporting those would bury the scheduled failures this exists for. Every conclusion but -# success, skipped and neutral is reported, so a conclusion nobody anticipated is posted rather -# than passed over. The filter is here, not in each caller, so no caller can get it wrong. +# The caller says which triggers to report with `events`. A push is reported only on the +# default branch, because a push to any other branch has its pusher watching. Every conclusion +# but success, skipped and neutral is reported, 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. 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.' + type: string + required: true secrets: SLACK_WEBHOOK: description: 'A Slack incoming webhook URL, which posts to the one channel it was issued for. Not required, so that a missing secret fails with a message naming it rather than GitHub refusing the call.' @@ -24,7 +31,9 @@ jobs: # fails in its first step instead of being skipped in silence. if: >- github.event_name != 'workflow_run' || - (github.event.workflow_run.event == 'schedule' && + (contains(fromJSON(inputs.events), github.event.workflow_run.event) && + (github.event.workflow_run.event != 'push' || + github.event.workflow_run.head_branch == github.event.repository.default_branch) && !contains(fromJSON('["success", "skipped", "neutral"]'), github.event.workflow_run.conclusion)) runs-on: [ubuntu-latest] name: Report the failure to Slack @@ -47,6 +56,7 @@ jobs: RUN_URL: ${{ github.event.workflow_run.html_url }} BRANCH: ${{ github.event.workflow_run.head_branch }} SHA: ${{ github.event.workflow_run.head_sha }} + TRIGGER: ${{ github.event.workflow_run.event }} run: | set -euo pipefail if [ "$EVENT_NAME" != workflow_run ]; then @@ -70,7 +80,7 @@ jobs: # Slack reads &, < and > as markup in message text. escape() { sed -e 's/&/\&/g' -e 's//\>/g' <<<"$1"; } - text="Scheduled run failed in $(escape "$REPO"): *$(escape "$WORKFLOW")* ended \`$(escape "$CONCLUSION")\`. + text="Failed in $(escape "$REPO"): *$(escape "$WORKFLOW")*, started by \`$(escape "$TRIGGER")\`, ended \`$(escape "$CONCLUSION")\`. Jobs: $(escape "$jobs") <$RUN_URL|Run $RUN_NUMBER, attempt $RUN_ATTEMPT> on \`$(escape "$BRANCH")\` at \`${SHA:0:12}\`" diff --git a/README.md b/README.md index cb6673e..0aeaa59 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, 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 unattended run reporting its failures, 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 scheduled 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` | +| `.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` | ## Using them @@ -93,45 +93,59 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. - **The caller needs** `contents: read` and `pull-requests: write`, and must pass the secret explicitly. -## Reporting failed scheduled runs to Slack +## Reporting failed runs to Slack -A scheduled run has nobody watching it, so its failure is posted to Slack instead. Each -repository has one small watcher workflow naming the workflows to report; adding a scheduled -workflow means adding its name there. +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. ```yaml -name: 'Report scheduled failures' +name: 'Report failures' on: workflow_run: - workflows: ['Nightly fuzzing', 'Mutation testing'] + workflows: + - 'Check inbound changes' + - 'Release' + - 'Nightly fuzzing' types: [completed] permissions: actions: read jobs: slack: + name: Report to Slack uses: L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@ # vX.Y.Z + with: + events: '["schedule", "push", "release"]' secrets: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} ``` -- **What is reported.** Runs started by a schedule, with any conclusion but success, skipped or - neutral, so a cancelled or timed-out run is posted as well as a failed one. A run started any - other way is skipped: someone started it and is watching it. The post names the repository, - the workflow, its conclusion and the jobs that did not succeed, and links the run. +- **What is reported.** A 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 only on the default branch: 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. +- **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. - **Where it goes.** To the channel of the incoming webhook in the `SLACK_WEBHOOK` secret. The organisation's secret posts to the organisation's channel, and a repository with a channel of - its own sets a repository secret of the same name. An incoming webhook posts to one channel - and can do nothing else. + its own sets a repository secret of the same name, which GitHub uses in preference. 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. - **The watcher only fires from the default branch**, where GitHub reads `workflow_run` - triggers, which is where schedules run too. So it is tested only once merged: by a scheduled - workflow that fails on purpose, removed again once it has posted. + triggers. So a change to it is tested only once merged. - **The caller needs** `actions: read`, to list the failed jobs; without it the post still goes, saying the jobs could not be listed. +This repository's own watcher calls the workflow by local path, so it always runs the version +on `main`. + ## Running the hygiene check locally From the root of the repository to check, fetch the script at the commit that repository's diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index 697458f..400fad6 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -37,6 +37,7 @@ # so it buys nothing - and a __future__ import must precede every other statement, which would # force the header block below it and out of the position the convention puts it in. +import json import pathlib import re import shutil @@ -105,6 +106,16 @@ 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") + +# A call to the shared Slack reporter, from another repository or from this one. +REPORTER = re.compile( + r"(?:\./|L337-org/github-workflows/)\.github/workflows/slack-on-failure\.yaml(?:@|$)" +) + # Compared as a resolved path, not a basename: another file of the same name elsewhere in a # repository would otherwise be dropped from the scan without anything saying so. Only matters # when the shared workflows repository checks itself, the one place this file is tracked; its @@ -254,8 +265,8 @@ def is_searchable_text(path): return True -def workflow_jobs(root, tracked): - """Every CI job, as `(repo-relative path, job id, job body)`. +def workflow_documents(root, tracked): + """Every tracked workflow, parsed, as `(repo-relative path, document)`. Sourced from the tracked listing rather than a glob, for the same reason: an untracked workflow a developer left lying about is not part of the repository. @@ -264,15 +275,14 @@ def workflow_jobs(root, tracked): root (pathlib.Path): repository root. tracked (list): every tracked path, unfiltered - deliberately not the searchable-text subset, because a workflow that is not readable text must still be reported rather than - quietly dropped from the bounds checks. + quietly dropped from the checks. Returns: - list: One triple per job. + list: One pair per workflow file. Raises: - CannotEvaluate: No workflow files are tracked, one could not be read, or one is not - valid YAML, is not a mapping, declares no usable `jobs:` mapping, or gives a job - a body that is not a mapping. + CannotEvaluate: No workflow files are tracked, or one could not be read, is not valid + YAML, or is not a mapping. """ import yaml @@ -281,7 +291,7 @@ def workflow_jobs(root, tracked): if not workflows: raise CannotEvaluate("no tracked workflow files found, so the CI checks would verify nothing") - jobs = [] + documents = [] for path in workflows: relative = path.relative_to(root) try: @@ -290,6 +300,26 @@ def workflow_jobs(root, tracked): raise CannotEvaluate(f"{relative} is not valid YAML: {exc}") from exc if not isinstance(document, dict): raise CannotEvaluate(f"{relative} does not parse as a YAML mapping") + documents.append((relative, document)) + return documents + + +def workflow_jobs(root, tracked): + """Every CI job, as `(repo-relative path, job id, job body)`. + + Args: + root (pathlib.Path): repository root. + tracked (list): every tracked path, unfiltered, as workflow_documents takes it. + + Returns: + list: One triple per job. + + Raises: + CannotEvaluate: As workflow_documents, or a workflow declares no usable `jobs:` + mapping, or gives a job a body that is not a mapping. + """ + jobs = [] + for relative, document in workflow_documents(root, tracked): declared = document.get("jobs") # Not `or {}`: a missing or null `jobs:` would become an empty mapping and the workflow # would be skipped in silence while the bounds checks reported success. @@ -466,6 +496,139 @@ def check_ci_jobs_are_bounded(root, tracked, files): return findings +def workflow_triggers(relative, document): + """The names of the events that start a workflow. + + PyYAML reads YAML 1.1, where a bare `on` key is the boolean true, so the key is looked up + both ways. + + Args: + relative (pathlib.Path): the workflow's repo-relative path, for the error. + document (dict): the parsed workflow. + + Returns: + set: The event names. + + Raises: + CannotEvaluate: The workflow has no `on:`, or one of a shape GitHub does not accept. + """ + on = document.get("on", document.get(True)) + if isinstance(on, str): + return {on} + if isinstance(on, list) and all(isinstance(event, str) for event in on): + return set(on) + if isinstance(on, dict) and all(isinstance(event, str) for event in on): + return set(on) + 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. + + Args: + documents (list): `(repo-relative path, document)` pairs, as workflow_documents returns. + + Returns: + list: The findings. + + Raises: + CannotEvaluate: A workflow's `on:` cannot be read, via workflow_triggers. + """ + findings = [] + # Workflow name -> the triggers some watcher reports for it. + reported = {} + watchers = [] + for relative, document in documents: + jobs = document.get("jobs") + calls = [ + body for body in (jobs.values() if isinstance(jobs, dict) else ()) + if isinstance(body, dict) and isinstance(body.get("uses"), str) + and REPORTER.match(body["uses"]) + ] + if not calls: + continue + watchers.append(relative) + on = document.get("on", document.get(True)) + run = on.get("workflow_run") if isinstance(on, dict) else None + names = run.get("workflows") if isinstance(run, dict) else None + if not (isinstance(names, list) and names and all(isinstance(n, str) for n in names)): + findings.append( + f"{relative} calls the Slack reporter but has no `on: workflow_run: workflows:` " + f"list of workflow names, so it reports nothing" + ) + continue + for body in calls: + raw = (body.get("with") or {}).get("events") if isinstance(body.get("with") or {}, dict) else None + try: + events = json.loads(raw) if isinstance(raw, str) else None + except json.JSONDecodeError: + events = None + 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\"]'" + ) + continue + for name in names: + reported.setdefault(name, set()).update(events) + + named = set() + for relative, document in documents: + if relative in watchers: + continue + name = document.get("name") + if isinstance(name, str): + named.add(name) + unattended = workflow_triggers(relative, document) & set(UNATTENDED) + if not unattended: + 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"watcher's `workflows:` list matches on" + ) + continue + missing = unattended - 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:`" + ) + for name in sorted(set(reported) - named): + findings.append( + f"a watcher lists {name!r}, which is the `name:` of no workflow here, so it reports " + f"nothing: remove it, or follow the renamed workflow" + ) + return findings + + +def check_unattended_runs_report_failures(root, tracked, files): + """Every workflow that runs with nobody watching has its failures posted to Slack. + + 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 + watcher's list that no workflow has is reported too, because GitHub matches on the name and + a rename leaves the list reporting nothing. + + Args: + root (pathlib.Path): repository root. + tracked (list): every tracked path, unfiltered. + files (list): the searchable-text subset. + + Returns: + list: The findings. + + Raises: + 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)) + + # Markdown links plus backtick-quoted paths, since a router routes both ways. ROUTED_LINK = re.compile(r"\[[^\]]*\]\(([^)]+)\)|`([^`]+\.md)`") @@ -669,6 +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), ("the instruction layer is intact", check_the_instruction_layer), ("the detail layer and the router agree", check_the_detail_layer_routing), ) From 3f8e71ff34c83e49b0a5e7aaa2ac0b6f08989b06 Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:44:21 +0100 Subject: [PATCH 3/7] Do not post a cancelled run that a newer run replaced Several repositories run their checks on a push to main in a concurrency group with `cancel-in-progress`, so two merges landing close together cancel the first run. The reporter posts cancelled runs, because a run cancelled by its timeout is a real failure, so each such pair would have put a false failure in Slack. A cancelled run is now skipped, with a notice, when the newest run of the same workflow for the same branch and trigger has a higher run number: that run replaced it and is reported in its own right if it fails. A run cancelled by its timeout has no newer run and is still posted, and if the runs cannot be listed the post goes anyway, with a warning. Tested against the fake Slack server with a stub `gh`: cancelled and replaced (skipped), cancelled and newest (posted), no runs listed (posted), the listing failing (warning, posted), a failed run with a newer one (posted, since only a cancellation is skipped), and a branch name needing URL-encoding. The live endpoint was queried against quaffed's own checks to confirm it returns the newest run first, numbered per workflow. Signed-off-by: Gavin Lucas --- .github/workflows/slack-on-failure.yaml | 17 +++++++++++++++++ README.md | 4 +++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index 8b4f1c6..28e485c 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -57,12 +57,29 @@ jobs: BRANCH: ${{ github.event.workflow_run.head_branch }} SHA: ${{ github.event.workflow_run.head_sha }} TRIGGER: ${{ github.event.workflow_run.event }} + WORKFLOW_ID: ${{ github.event.workflow_run.workflow_id }} run: | set -euo pipefail if [ "$EVENT_NAME" != workflow_run ]; then echo "::error::This workflow reports a run that \`workflow_run\` names, and was called from a \`$EVENT_NAME\` event. Call it from a workflow triggered by \`workflow_run\`." >&2 exit 1 fi + # A run cancelled because a newer one replaced it - a concurrency group with + # `cancel-in-progress`, when two merges land close together - is not a failure: the + # newer run is reported if it fails. A run cancelled by its timeout has no newer run, + # so it is still posted. If the newer runs cannot be listed, the post goes anyway. + if [ "$CONCLUSION" = cancelled ]; then + if newest=$(gh api "repos/$REPO/actions/workflows/$WORKFLOW_ID/runs?branch=$(jq -rn --arg b "$BRANCH" '$b|@uri')&event=$TRIGGER&per_page=1" \ + --jq '.workflow_runs[0].run_number // empty' 2>&1); then + if [ -n "$newest" ] && [ "$newest" -gt "$RUN_NUMBER" ]; then + echo "::notice::Not posted: run $RUN_NUMBER of $WORKFLOW was cancelled and run $newest on $BRANCH has replaced it." + exit 0 + fi + else + echo "::warning::Could not tell whether cancelled run $RUN_ID was replaced by a newer one, so it is posted: $newest" + fi + fi + if [ -z "$SLACK_WEBHOOK" ]; then echo "::error::No SLACK_WEBHOOK secret reached this workflow, so the failure of $WORKFLOW (run $RUN_URL) was not posted. Set the secret on the repository or the organisation, and pass it in the caller's \`secrets:\`." >&2 exit 1 diff --git a/README.md b/README.md index 0aeaa59..955f8fd 100644 --- a/README.md +++ b/README.md @@ -122,7 +122,9 @@ jobs: - **What is reported.** A 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 only on the default branch: a push to another branch has + as a failed one. A cancelled run that a newer run of the same workflow on the same branch has + replaced, as `cancel-in-progress` does when two merges land together, is not posted: the newer + run is reported if it fails. A push is reported only on the default branch: 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. From 411fb939e612a13cb4d93b97819c02bafd430c92 Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:49:30 +0100 Subject: [PATCH 4/7] Review this repository's own pull requests with the Claude review Every other repository's pull requests get the Claude review, and the repository that defines it did not, so changes to the shared workflows were the ones reviewed least. The caller is every other repository's, calling the review by local path rather than by a pinned commit, so there is no pin to bump. On a pull request GitHub runs the version on the pull request's branch, so a change to the review is exercised by reviewing the pull request that makes it. A broken change can review itself badly, which costs little because the review is advisory. A comment request runs from the default branch, so it uses main's version. The token is the organisation's secret, already visible to this repository, and a fork pull request gets no secrets, which the review's gate already skips. Signed-off-by: Gavin Lucas --- .github/workflows/code-review.yaml | 61 ++++++++++++++++++++++++++++++ README.md | 7 ++-- 2 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 .github/workflows/code-review.yaml diff --git a/.github/workflows/code-review.yaml b/.github/workflows/code-review.yaml new file mode 100644 index 0000000..c379131 --- /dev/null +++ b/.github/workflows/code-review.yaml @@ -0,0 +1,61 @@ +name: 'Code review' +permissions: + contents: read + +# The same caller as every other repository's, except that it calls the review workflow by local +# path rather than a pinned commit. On a pull request that runs the version on the pull request's +# branch, so a change to the review is exercised by reviewing the pull request that makes it; +# a broken change can review itself badly, which costs little since the review is advisory. +# README.md describes the review. +# +# - Opening a pull request, and every push to one, asks for a review at the depth the shared +# workflow picks. A push is reviewed deliberately: the loop that invites, a reviewing agent and +# a coding agent trading rounds nobody chose, is accepted because a declined finding is not +# raised again and the shared workflow pauses automatic rounds after a bound. +# - Reopening a pull request or marking it ready forces a full review, because either usually +# follows a change of approach that a delta of the newest commits would not see. +# - A top-level comment starting `/claude-review` asks for a review at the picked depth, and +# `/claude-review full` forces a full one. The command must be followed by a space, a line +# break or nothing, so that `/claude-reviewer` or `/claude-review fullness` do not count; a +# space is appended before matching to cover the last case. GitHub runs comment-triggered +# workflows from the default branch, so the commands only work from `main`, and run `main`'s +# version of the review. +on: + pull_request: + types: [opened, reopened, ready_for_review, synchronize] + issue_comment: + types: [created] + +# No concurrency group here: one at workflow level is joined by every comment on the pull +# request, before the job's `if:` runs, and an unrelated comment would replace a queued review +# request. The shared workflow queues reviews on its own job instead. + +jobs: + review: + # Draft pull requests wait until they are ready. A comment request is honoured on a + # draft, because asking is the signal. Only people the repository trusts can ask: the + # review spends the allowance of whoever owns the token. + if: >- + (github.event_name == 'pull_request' && !github.event.pull_request.draft) || + (github.event_name == 'issue_comment' && github.event.issue.pull_request && + (startsWith(format('{0} ', github.event.comment.body), '/claude-review ') || + startsWith(github.event.comment.body, fromJSON('"/claude-review\n"')) || + startsWith(github.event.comment.body, fromJSON('"/claude-review\r\n"'))) && + contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) + permissions: + contents: read + pull-requests: write + uses: ./.github/workflows/claude-review.yaml + with: + pr-number: ${{ github.event.pull_request.number || github.event.issue.number }} + depth: >- + ${{ ((github.event_name == 'pull_request' && + contains(fromJSON('["reopened","ready_for_review"]'), github.event.action)) || + (github.event_name == 'issue_comment' && + (startsWith(format('{0} ', github.event.comment.body), '/claude-review full ') || + startsWith(github.event.comment.body, fromJSON('"/claude-review full\n"')) || + startsWith(github.event.comment.body, fromJSON('"/claude-review full\r\n"'))))) + && 'full' || 'auto' }} + push-triggered: ${{ github.event_name == 'pull_request' && github.event.action == 'synchronize' }} + secrets: + CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/README.md b/README.md index 955f8fd..5f15465 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,7 @@ Exit status is 0 clean, 1 on findings, 2 when the scan could not be trusted. Everything here runs in other repositories' CI, so a change reaches them only when each one bumps its pin. This repository's own CI runs the action-pins action from the commit under -review and lints every workflow and action with actionlint. The hygiene check does not run on -this repository: it expects the instruction layer and detail layer of a product repository, -which this one does not have. +review, lints every workflow and action with actionlint, and reviews each pull request with the +Claude review at the pull request's own commit, so a change to the review reviews itself. The +hygiene check does not run on this repository: it expects the instruction layer and detail layer +of a product repository, which this one does not have. From 4d2d87548f662241f721ba57e8f21508c1ea240c Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:54:25 +0100 Subject: [PATCH 5/7] Report tag pushes and completed runs only, and correct what the README promises From the review of this branch: - A push was reported only on the default branch, so a release started by pushing a tag, which the hygiene check requires to be listed, was never posted. For a push, `head_branch` is the branch or the tag's name, so the step now looks the name up among the tags: the default branch or a tag is posted, another branch is not, and if the tags cannot be listed the post goes anyway. That needs `contents: read`. - A watcher without `types: [completed]` is also started when a run is requested and while it runs, with no conclusion, which passed the conclusion filter and would have posted every run as it started. The job now requires the `completed` action itself. - The README said a caller without `actions: read` still got a post. A called job cannot have more than its caller grants, so GitHub refuses to start it; the README now says the caller must grant `actions: read` and `contents: read`, and this repository's watcher does. - The comment and README said a run cancelled by its timeout is always posted. One cancelled while a newer run was already queued looks like a replaced one and is not, which is now stated as an accepted limitation. - The README described the organisation's secret's channel and this repository's own files, which those hold themselves; it now states the rule instead. Tested against the fake Slack server with a stub `gh`: a push to the default branch (posted), to another branch (not posted), of a tag (posted), of a name that only prefixes a tag (not posted), with the tags unlistable (warning, posted), a scheduled run on another branch (posted), and the cancelled cases again. The live `git/matching-refs/tags` endpoint returns prefix matches, and an empty list for none, which the exact comparison relies on. Signed-off-by: Gavin Lucas --- .github/workflows/code-review.yaml | 4 +-- .github/workflows/report-failures.yaml | 3 +- .github/workflows/slack-on-failure.yaml | 42 ++++++++++++++++------ README.md | 36 +++++++++---------- actions/repo-hygiene/check-repo-hygiene.py | 3 +- 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/.github/workflows/code-review.yaml b/.github/workflows/code-review.yaml index c379131..2fe5ffb 100644 --- a/.github/workflows/code-review.yaml +++ b/.github/workflows/code-review.yaml @@ -2,8 +2,8 @@ name: 'Code review' permissions: contents: read -# The same caller as every other repository's, except that it calls the review workflow by local -# path rather than a pinned commit. On a pull request that runs the version on the pull request's +# A caller like any other repository's, except that it calls the review workflow by local path +# rather than a pinned commit. On a pull request that runs the version on the pull request's # branch, so a change to the review is exercised by reviewing the pull request that makes it; # a broken change can review itself badly, which costs little since the review is advisory. # README.md describes the review. diff --git a/.github/workflows/report-failures.yaml b/.github/workflows/report-failures.yaml index 6131eb4..7483ac6 100644 --- a/.github/workflows/report-failures.yaml +++ b/.github/workflows/report-failures.yaml @@ -9,8 +9,9 @@ on: types: [completed] permissions: - # Read by the reporter to name the jobs that failed. + # Both read by the reporter, which GitHub refuses to start without them. actions: read + contents: read jobs: slack: diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index 28e485c..9da4314 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -5,11 +5,12 @@ name: 'Report a failed run to Slack' # default branch or a release - that fails 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 only on the -# default branch, because a push to any other branch has its pusher watching. Every conclusion -# but success, skipped and neutral is reported, 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. +# 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 +# `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. @@ -31,16 +32,18 @@ jobs: # fails in its first step instead of being skipped in silence. if: >- github.event_name != 'workflow_run' || - (contains(fromJSON(inputs.events), github.event.workflow_run.event) && - (github.event.workflow_run.event != 'push' || - github.event.workflow_run.head_branch == github.event.repository.default_branch) && + (github.event.action == 'completed' && + contains(fromJSON(inputs.events), github.event.workflow_run.event) && !contains(fromJSON('["success", "skipped", "neutral"]'), github.event.workflow_run.conclusion)) runs-on: [ubuntu-latest] name: Report the failure to Slack timeout-minutes: 5 + # The caller must grant both, or GitHub refuses to start this job. permissions: - # To name the jobs that failed. + # To name the jobs that failed, and to find a newer run that replaced a cancelled one. actions: read + # To tell a tag from a branch. + contents: read steps: - name: Post the failed run env: @@ -58,16 +61,33 @@ jobs: SHA: ${{ github.event.workflow_run.head_sha }} TRIGGER: ${{ github.event.workflow_run.event }} WORKFLOW_ID: ${{ github.event.workflow_run.workflow_id }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} run: | set -euo pipefail if [ "$EVENT_NAME" != workflow_run ]; then echo "::error::This workflow reports a run that \`workflow_run\` names, and was called from a \`$EVENT_NAME\` event. Call it from a workflow triggered by \`workflow_run\`." >&2 exit 1 fi + # For a run a push started, `head_branch` is the branch or, for a tag, the tag's name. + # A push to the default branch or of a tag is reported; a push to another branch has + # its pusher watching. If the tags cannot be listed, the post goes anyway. + if [ "$TRIGGER" = push ] && [ "$BRANCH" != "$DEFAULT_BRANCH" ]; then + if refs=$(gh api "repos/$REPO/git/matching-refs/tags/$(jq -rn --arg b "$BRANCH" '$b|@uri')" --jq '.[].ref' 2>&1); then + if ! grep -Fxq "refs/tags/$BRANCH" <<<"$refs"; then + echo "::notice::Not posted: run $RUN_NUMBER of $WORKFLOW was a push to $BRANCH, which is neither the default branch nor a tag." + exit 0 + fi + else + echo "::warning::Could not tell whether $BRANCH is a tag, so the failed push is posted: $refs" + fi + fi + # A run cancelled because a newer one replaced it - a concurrency group with # `cancel-in-progress`, when two merges land close together - is not a failure: the - # newer run is reported if it fails. A run cancelled by its timeout has no newer run, - # so it is still posted. If the newer runs cannot be listed, the post goes anyway. + # newer run is reported if it fails. A run cancelled by its timeout while no newer run + # exists is posted. One cancelled by its timeout while a newer run was already queued + # cannot be told from a replaced one, so it is not posted: an accepted limitation. If + # the newer runs cannot be listed, the post goes anyway. if [ "$CONCLUSION" = cancelled ]; then if newest=$(gh api "repos/$REPO/actions/workflows/$WORKFLOW_ID/runs?branch=$(jq -rn --arg b "$BRANCH" '$b|@uri')&event=$TRIGGER&per_page=1" \ --jq '.workflow_runs[0].run_number // empty' 2>&1); then diff --git a/README.md b/README.md index 5f15465..a75ae98 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,7 @@ on: types: [completed] permissions: actions: read + contents: read jobs: slack: name: Report to Slack @@ -120,33 +121,32 @@ jobs: SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} ``` -- **What is reported.** A 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 cancelled run that a newer run of the same workflow on the same branch has - replaced, as `cancel-in-progress` does when two merges land together, is not posted: the newer - run is reported if it fails. A push is reported only on the default branch: 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. +- **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. +- **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. -- **Where it goes.** To the channel of the incoming webhook in the `SLACK_WEBHOOK` secret. The - organisation's secret posts to the organisation's channel, and a repository with a channel of - its own sets a repository secret of the same name, which GitHub uses in preference. An - incoming webhook posts to one channel and can do nothing else. +- **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. - **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 needs** `actions: read`, to list the failed jobs; without it the post still - goes, saying the jobs could not be listed. - -This repository's own watcher calls the workflow by local path, so it always runs the version -on `main`. +- **The caller must grant** `actions: read` and `contents: read`, as the example does, or + GitHub refuses to start the reporter's job and nothing is posted. ## Running the hygiene check locally @@ -174,6 +174,6 @@ Exit status is 0 clean, 1 on findings, 2 when the scan could not be trusted. Everything here runs in other repositories' CI, so a change reaches them only when each one bumps its pin. This repository's own CI runs the action-pins action from the commit under review, lints every workflow and action with actionlint, and reviews each pull request with the -Claude review at the pull request's own commit, so a change to the review reviews itself. The +pull request's own version of the Claude review, so a change to the review reviews itself. The hygiene check does not run on this repository: it expects the instruction layer and detail layer of a product repository, which this one does not have. diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index 400fad6..7867bd3 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -558,7 +558,8 @@ def unattended_run_findings(documents): ) continue for body in calls: - raw = (body.get("with") or {}).get("events") if isinstance(body.get("with") or {}, dict) else None + given = body.get("with") or {} + raw = given.get("events") if isinstance(given, dict) else None try: events = json.loads(raw) if isinstance(raw, str) else None except json.JSONDecodeError: From 92d1521eb47436cbb1e9ded6eb8d0983eb645bb1 Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 13:56:27 +0100 Subject: [PATCH 6/7] Note that a branch named like a tag is posted, and stop comparing the review caller with other repositories' The reporter cannot tell a branch from a tag of the same name from the run, so a push to such a branch is posted; that is now said where the lookup is. The review caller's comment compared it with other repositories' callers, which goes stale when one of them changes; it now says only what this file does. Signed-off-by: Gavin Lucas --- .github/workflows/code-review.yaml | 8 ++++---- .github/workflows/slack-on-failure.yaml | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/code-review.yaml b/.github/workflows/code-review.yaml index 2fe5ffb..70be49f 100644 --- a/.github/workflows/code-review.yaml +++ b/.github/workflows/code-review.yaml @@ -2,10 +2,10 @@ name: 'Code review' permissions: contents: read -# A caller like any other repository's, except that it calls the review workflow by local path -# rather than a pinned commit. On a pull request that runs the version on the pull request's -# branch, so a change to the review is exercised by reviewing the pull request that makes it; -# a broken change can review itself badly, which costs little since the review is advisory. +# Calls the review workflow by local path rather than a pinned commit. On a pull request that +# runs the version on the pull request's branch, so a change to the review is exercised by +# reviewing the pull request that makes it; a broken change can review itself badly, which costs +# little since the review is advisory. # README.md describes the review. # # - Opening a pull request, and every push to one, asks for a review at the depth the shared diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index 9da4314..e1d486e 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -70,7 +70,8 @@ jobs: fi # For a run a push started, `head_branch` is the branch or, for a tag, the tag's name. # A push to the default branch or of a tag is reported; a push to another branch has - # its pusher watching. If the tags cannot be listed, the post goes anyway. + # its pusher watching. If the tags cannot be listed, the post goes anyway. A branch + # with the same name as a tag cannot be told from it here, so its push is posted. if [ "$TRIGGER" = push ] && [ "$BRANCH" != "$DEFAULT_BRANCH" ]; then if refs=$(gh api "repos/$REPO/git/matching-refs/tags/$(jq -rn --arg b "$BRANCH" '$b|@uri')" --jq '.[].ref' 2>&1); then if ! grep -Fxq "refs/tags/$BRANCH" <<<"$refs"; then From 15a5b15627d66af3d820164c46b3a6252a98dd97 Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 9 Oct 2026 14:26:00 +0100 Subject: [PATCH 7/7] Review the head a comment request was made against, not whatever is there when it runs A review requested by comment runs after the comment, carrying the token, and checked out the pull request's head as it was when the job started. A push in between put code the requester never saw under that token. CodeQL raised it as an untrusted checkout and a time-of-check race once this repository's own caller made the reusable workflow visible to it; it applies equally to every repository calling the review. On a comment request the gate now reads when the branch was last pushed from GitHub's activity record, which the pusher cannot set, unlike a commit's own date, and refuses the request with a reply saying to ask again if that push is at or after the comment, is not of the current head, or cannot be shown at all. A failed lookup fails the job, naming what it was reading. The gate passes the head it checked to the review job, which checks that out instead of looking the head up again. The comparison is two `date -d` timestamps, the form CodeQL recognises as a comment-versus-push check; with the caller's `author_association` condition, that is the pair of guards its untrusted-checkout query asks for on `issue_comment`. Tested by running the gate's script with a stub `gh` and GNU date: a pull request event, a request after the push (runs, passes the head), a push after the request, a push in the same second, a latest push of another commit, no activity, no comment time (each refused with a reply), and the activity lookup failing (job fails, naming it). The live activity endpoint, queried for a branch with a slash in its name, returns its latest push with the branch's head as `after`. Signed-off-by: Gavin Lucas --- .github/workflows/claude-review.yaml | 59 +++++++++++++++++++++++++--- README.md | 6 +++ 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/.github/workflows/claude-review.yaml b/.github/workflows/claude-review.yaml index 3bd5677..5031817 100644 --- a/.github/workflows/claude-review.yaml +++ b/.github/workflows/claude-review.yaml @@ -71,11 +71,20 @@ jobs: # A fork pull request is refused even when a maintainer asks by comment, by decision: that # run carries the token while the reviewed text is written by someone with no access, who # could steer the verdict. A refused request gets a reply so it does not look ignored. + # + # A request by comment is honoured only for the head that was there when it was made. The + # run starts after the comment, so without this a push in between would put code the + # requester never saw under a run carrying the token. The time of the branch's latest push + # comes from GitHub's activity record, which the pusher cannot set, unlike a commit's own + # date, and that push must be of the head this gate passes on. A push at or after the + # comment refuses the request with a reply saying to ask again. The review checks out the + # head this gate decided on rather than looking it up again, so nothing can slip in after. runs-on: [ubuntu-latest] name: Can this be reviewed timeout-minutes: 2 outputs: run: ${{ steps.check.outputs.run }} + head: ${{ steps.check.outputs.head }} steps: - name: Check for a token, a human actor and author, and a same-repository branch id: check @@ -85,12 +94,14 @@ jobs: EVENT: ${{ github.event_name }} GH_TOKEN: ${{ github.token }} PR: ${{ inputs.pr-number }} + COMMENT_AT: ${{ github.event.comment.created_at }} run: | # Fetched first and on its own, so a failed lookup stops the job rather than reading # as "not a fork" or "not a bot". - info=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json isCrossRepository,author \ - --jq '[.isCrossRepository, .author.is_bot, .author.login] | @tsv') - IFS=$'\t' read -r cross author_bot author <<<"$info" + info=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" \ + --json isCrossRepository,author,headRefName,headRefOid \ + --jq '[.isCrossRepository, .author.is_bot, .author.login, .headRefName, .headRefOid] | @tsv') + IFS=$'\t' read -r cross author_bot author head_ref head <<<"$info" if [ "$cross" = true ]; then echo "::notice::Review refused: pull request #$PR comes from a fork, and fork pull requests are not reviewed." if [ "$EVENT" = issue_comment ]; then @@ -112,7 +123,35 @@ jobs: fi echo "run=false" >> "$GITHUB_OUTPUT" else - echo "run=true" >> "$GITHUB_OUTPUT" + refused="" + if [ "$EVENT" = issue_comment ]; then + comment_at=$COMMENT_AT + # The newest activity on the branch, of any kind: a push, a force-push or its + # creation. A failed lookup stops the job rather than reading as "no push". + ref=$(jq -rn --arg b "refs/heads/$head_ref" '$b|@uri') + if ! latest=$(gh api "repos/$GITHUB_REPOSITORY/activity?ref=$ref&per_page=1" \ + --jq '.[0] // empty | [.timestamp, .after] | @tsv' 2>&1); then + echo "::error::Could not read when \`$head_ref\` was last pushed, so the review requested by comment on #$PR was not run: $latest" >&2 + exit 1 + fi + IFS=$'\t' read -r pushed_at pushed_sha <<<"$latest" + if [ -z "$pushed_at" ] || [ -z "$comment_at" ]; then + refused="GitHub holds no record of when \`$head_ref\` was last pushed, or of when the review was asked for, so the head cannot be shown to predate the request." + elif [ "$pushed_sha" != "$head" ]; then + refused="the latest push to \`$head_ref\` is of \`${pushed_sha:0:12}\`, not of the head \`${head:0:12}\`, so the head cannot be shown to predate the request." + elif [[ $(date -d "$pushed_at" +%s) -ge $(date -d "$comment_at" +%s) ]]; then + refused="\`$head_ref\` was pushed at $pushed_at, at or after the request at $comment_at, so the head is not the one the request was made against." + fi + fi + if [ -n "$refused" ]; then + echo "::notice::Review refused: $refused" + gh pr comment "$PR" --repo "$GITHUB_REPOSITORY" \ + --body "Claude review: refused, because $refused Ask again to review the current head." + echo "run=false" >> "$GITHUB_OUTPUT" + else + echo "run=true" >> "$GITHUB_OUTPUT" + echo "head=$head" >> "$GITHUB_OUTPUT" + fi fi review: @@ -141,14 +180,22 @@ jobs: steps: - name: Look up the pull request id: pr + env: + # The head the gate checked, not the pull request's head now, which may have moved. + GATE_HEAD: ${{ needs.gate.outputs.head }} run: | - info=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json headRefOid,baseRefName,state) + info=$(gh pr view "$PR" --repo "$GITHUB_REPOSITORY" --json baseRefName,state) state=$(jq -r '.state' <<<"$info") if [ "$state" != OPEN ]; then echo "::error::Pull request #$PR is $state; only an open pull request is reviewed." exit 1 fi - jq -r '"head=\(.headRefOid)\nbase=\(.baseRefName)"' <<<"$info" >> "$GITHUB_OUTPUT" + if ! [[ "$GATE_HEAD" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::The gate passed on no head commit (got '$GATE_HEAD'), so there is nothing safe to check out." + exit 1 + fi + echo "head=$GATE_HEAD" >> "$GITHUB_OUTPUT" + jq -r '"base=\(.baseRefName)"' <<<"$info" >> "$GITHUB_OUTPUT" - name: Check out the pull request head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/README.md b/README.md index a75ae98..f7a27d3 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,12 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. and the range it covers starts at the last commit a posted review recorded, so commits the older one would have reviewed are not skipped. A burst of pushes during a review produces one delta review of all of them. +- **A request by comment reviews the head it was made against.** The run starts after the + comment, so the gate reads when the branch was last pushed from GitHub's activity record, + which the pusher cannot set, and refuses the request, with a reply saying to ask again, if + that push is at or after the comment or is not of the current head. The review then checks + out the head the gate passed rather than looking it up again. So nothing pushed after the + request is reviewed under the token. - **Bot and fork pull requests are skipped.** GitHub gives no secrets to a run started by Dependabot or by a fork pull request, and bot pull requests are reviewed by hand, so a gate job skips those runs with a notice instead of failing. A pull request a bot opened is skipped