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/.github/workflows/code-review.yaml b/.github/workflows/code-review.yaml new file mode 100644 index 0000000..70be49f --- /dev/null +++ b/.github/workflows/code-review.yaml @@ -0,0 +1,61 @@ +name: 'Code review' +permissions: + contents: read + +# 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/.github/workflows/report-failures.yaml b/.github/workflows/report-failures.yaml new file mode 100644 index 0000000..7483ac6 --- /dev/null +++ b/.github/workflows/report-failures.yaml @@ -0,0 +1,23 @@ +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: + # Both read by the reporter, which GitHub refuses to start without them. + actions: read + contents: 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 new file mode 100644 index 0000000..e1d486e --- /dev/null +++ b/.github/workflows/slack-on-failure.yaml @@ -0,0 +1,142 @@ +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; +# 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 +# `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. +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.' + 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.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, 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: + 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 }} + 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. 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 + 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 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 + 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 + 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="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}\`" + + # 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..f7a27d3 100644 --- a/README.md +++ b/README.md @@ -6,9 +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 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 @@ -77,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 @@ -92,6 +99,61 @@ 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 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. + +```yaml +name: 'Report failures' +on: + workflow_run: + workflows: + - 'Check inbound changes' + - 'Release' + - 'Nightly fuzzing' + types: [completed] +permissions: + actions: read + contents: 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.** 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. 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 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 From the root of the repository to check, fetch the script at the commit that repository's @@ -117,6 +179,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 +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 697458f..7867bd3 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,140 @@ 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: + 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: + 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 +833,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), )