From 2783a723e279400af5c66bce3085c90d22f0aaf4 Mon Sep 17 00:00:00 2001 From: CL Date: Thu, 15 Oct 2026 15:16:36 +0100 Subject: [PATCH 1/3] Announce pull requests to Slack, post through a bot token, and call at version tags New pull requests went unnoticed until someone opened the repository, which is how stacks of Dependabot pull requests built up. The GitHub app for Slack posts every later event on a pull request too, and cannot leave out forks, so slack-on-pull-request posts only a pull request that needs action: opened or reopened and not a draft, or marked ready for review. Forks are skipped, because their title and author would reach the channel through the token. Dependabot's pull requests are posted, which is why the token is also a Dependabot secret. Both Slack workflows now post with chat.postMessage through one bot token, to a channel each caller names. Incoming webhooks needed one secret per channel, twice over for Dependabot. The accepted cost is a broader credential: a leaked token can post to any channel the app is in. The token reaches curl in a file rather than on its command line. Callers move from commit pins to this repository's major-version tags. A commit pin needed a pull request in every repository for every change here, and callers were moved together anyway. The v* tags are protected by a ruleset whose bypass list is no wider than main's, so moving a tag takes the same people as merging. action-pins accepts exactly this repository's version tags; third-party actions still need a commit SHA. A push creating a v* tag from a non-admin account was refused by the ruleset ("Cannot create ref due to creations being restricted"). Breaking: the reporter requires a channel input and SLACK_BOT_TOKEN in place of SLACK_WEBHOOK, so this is v2.0.0. The hygiene check fails a Slack call that names no channel, so a caller moved to v2 without being changed fails its own pull request rather than breaking after merge. The posting steps are tested by running the shipped scripts against stand-ins for curl and gh, and action-pins over sample references. Removing the fork and draft filters, or the tag exemption, fails those tests. --- .github/workflows/announce-pull-requests.yaml | 18 ++ .github/workflows/claude-review.yaml | 2 +- .github/workflows/premerge.yaml | 19 ++ .github/workflows/report-failures.yaml | 4 +- .github/workflows/slack-on-failure.yaml | 70 +++-- .github/workflows/slack-on-pull-request.yaml | 134 +++++++++ README.md | 146 +++++++-- actions/action-pins/action.yml | 17 +- actions/repo-hygiene/action.yml | 2 +- actions/repo-hygiene/check-repo-hygiene.py | 59 +++- tests/test_action_pins.py | 57 ++++ tests/test_slack_workflows.py | 278 ++++++++++++++++++ 12 files changed, 754 insertions(+), 52 deletions(-) create mode 100644 .github/workflows/announce-pull-requests.yaml create mode 100644 .github/workflows/slack-on-pull-request.yaml create mode 100644 tests/test_action_pins.py create mode 100644 tests/test_slack_workflows.py diff --git a/.github/workflows/announce-pull-requests.yaml b/.github/workflows/announce-pull-requests.yaml new file mode 100644 index 0000000..6d1ec1d --- /dev/null +++ b/.github/workflows/announce-pull-requests.yaml @@ -0,0 +1,18 @@ +name: 'Announce pull requests' + +# Posts each pull request that needs action to Slack through this repository's own announcer on +# the pull request's branch (README.md). +on: + pull_request: + types: [opened, reopened, ready_for_review] + +permissions: {} + +jobs: + slack: + name: Announce to Slack + uses: ./.github/workflows/slack-on-pull-request.yaml + with: + channel: '#l337-org' + secrets: + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} diff --git a/.github/workflows/claude-review.yaml b/.github/workflows/claude-review.yaml index 2e7b134..3f75aaf 100644 --- a/.github/workflows/claude-review.yaml +++ b/.github/workflows/claude-review.yaml @@ -1,6 +1,6 @@ name: 'Claude review' -# A reusable review job, called by each repository pinned to a commit of this one. This +# A reusable review job, called by each repository at a version tag of this one. This # repository is public because a public caller cannot use a reusable workflow from a private one. # # Claude reads and returns a verdict; it never writes to GitHub. It gets read-only tools, and diff --git a/.github/workflows/premerge.yaml b/.github/workflows/premerge.yaml index 89d3582..b788b95 100644 --- a/.github/workflows/premerge.yaml +++ b/.github/workflows/premerge.yaml @@ -47,3 +47,22 @@ jobs: # SC2016 is shellcheck noting that `$x` does not expand inside single quotes. Here that # is always Markdown backticks in a printf format, which is the intent. run: uvx --from actionlint-py==1.7.12.25 actionlint -ignore 'SC2016' + + slack-workflows: + # The Slack workflows run only in other repositories' events - a pull request opening, a run + # failing - so their posting steps are run here against stand-ins for curl and gh instead. + runs-on: [ubuntu-latest] + name: Slack workflows behave + timeout-minutes: 5 + env: + # The tests import the hygiene script, and a checkout is left as it was found. + PYTHONDONTWRITEBYTECODE: '1' + steps: + - name: Check out source repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Set up uv + uses: astral-sh/setup-uv@c18668ad3cf93ea998bef934396af7bb5c839dc7 # v10.2.0 + + - name: Tests pass + run: uvx --with pyyaml==6.0.3 pytest==8.3.5 -p no:cacheprovider -q tests diff --git a/.github/workflows/report-failures.yaml b/.github/workflows/report-failures.yaml index f161b7e..979a71b 100644 --- a/.github/workflows/report-failures.yaml +++ b/.github/workflows/report-failures.yaml @@ -7,6 +7,7 @@ on: workflows: - 'Check inbound changes' - 'Code review' + - 'Announce pull requests' types: [completed] permissions: @@ -19,6 +20,7 @@ jobs: name: Report to Slack uses: ./.github/workflows/slack-on-failure.yaml with: + channel: '#l337-org' events: '["schedule", "push", "release", "pull_request"]' secrets: - SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} diff --git a/.github/workflows/slack-on-failure.yaml b/.github/workflows/slack-on-failure.yaml index b27c71f..d4f9af0 100644 --- a/.github/workflows/slack-on-failure.yaml +++ b/.github/workflows/slack-on-failure.yaml @@ -2,31 +2,35 @@ 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 run is otherwise one red entry in a list nobody opens; -# this posts it to the channel behind `SLACK_WEBHOOK` instead. +# this posts it to the caller's channel 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. A pull request from a fork is not reported: its run uses the fork's own # workflow file, so its author writes the job names and branch the post would carry, and could -# put text of their choosing in the channel through this webhook. The same reasoning has the +# put text of their choosing in the channel through this token. The same reasoning has the # review refuse forks. Only a completed run is reported, whatever `types:` the caller gives # `workflow_run`, and with every conclusion but success, skipped and neutral, so a conclusion # nobody anticipated is posted rather than passed over. The filters are here, not in each # caller, so no caller can get them wrong. # # A post Slack does not accept fails this job, with Slack's answer in the error. Nothing -# reports the watcher's own run, so a webhook that stops working is noticed only by the absence +# reports the watcher's own run, so a token that stops working is noticed only by the absence # of posts: an accepted limitation (README.md). on: workflow_call: inputs: + channel: + description: 'The Slack channel to post to, by name, such as `#l337-org`. The app the token belongs to must be a member of it.' + type: string + required: true events: description: 'The triggers whose failed runs are reported, as a JSON list of event names, such as `["schedule", "push", "release", "pull_request"]`. A manual run, `workflow_dispatch`, is best left out, because whoever started it is watching.' type: string required: true secrets: - 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.' + SLACK_BOT_TOKEN: + description: 'A Slack bot token (`xoxb-`) with the `chat:write` scope. Not required, so that a missing secret fails with a message naming it rather than GitHub refusing the call.' required: false jobs: @@ -52,7 +56,8 @@ jobs: steps: - name: Post the failed run env: - SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} + CHANNEL: ${{ inputs.channel }} GH_TOKEN: ${{ github.token }} EVENT_NAME: ${{ github.event_name }} REPO: ${{ github.repository }} @@ -106,8 +111,12 @@ jobs: 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 + if [ -z "$SLACK_BOT_TOKEN" ]; then + echo "::error::No SLACK_BOT_TOKEN secret reached this workflow, so the failure of $WORKFLOW (run $RUN_URL) was not posted. Set it on the organisation as an Actions secret and as a Dependabot secret, and pass it in the caller's \`secrets:\`." >&2 + exit 1 + fi + if [ -z "$CHANNEL" ]; then + echo "::error::No channel was given, so the failure of $WORKFLOW (run $RUN_URL) was not posted. Pass the channel name in the caller's \`with: channel:\`." >&2 exit 1 fi @@ -127,21 +136,42 @@ jobs: 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. + # The token goes to curl in a file, not on its command line, where any process on the + # runner could read it. The body goes to a file because a retry truncates a file, + # where to standard output it would follow the refused attempt's body. + umask 077 + header_file=$(mktemp) body_file=$(mktemp) - status=$(jq -n --arg text "$text" '{text: $text}' \ + trap 'rm -f "$header_file" "$body_file"' EXIT + printf 'Authorization: Bearer %s\n' "$SLACK_BOT_TOKEN" > "$header_file" + # Unfurling is off so the post stays text rather than gaining a preview card. + status=$(jq -n --arg channel "$CHANNEL" --arg text "$text" \ + '{channel: $channel, text: $text, unfurl_links: false, unfurl_media: false}' \ | 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 + --header @"$header_file" --header 'Content-Type: application/json; charset=utf-8' \ + --data @- --output "$body_file" --write-out '%{http_code}' \ + https://slack.com/api/chat.postMessage) || { + echo "::error::Posting the failure of $WORKFLOW (run $RUN_URL) to $CHANNEL 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 + # Slack answers 200 with `"ok": false` and an `error` code for most refusals, such as + # `not_in_channel`, `channel_not_found` or `invalid_auth`. An accepted post echoes the + # message back, so the cap for reading the answer is generous; a refusal that is not + # Slack's JSON, such as a proxy's error page, is quoted shorter. + body=$(head -c 65536 "$body_file") + if [ "$status" != 200 ] || [ "$(jq -r '.ok // empty' <<<"$body" 2>/dev/null)" != true ]; then + error=$(jq -r '.error // empty' <<<"$body" 2>/dev/null || true) + if [ "$(wc -c < "$body_file")" -gt 500 ]; then + body="$(head -c 500 "$body_file") ... (truncated at 500 bytes)" + fi + case "$error" in + not_in_channel) hint=" Invite the Slack app to $CHANNEL." ;; + channel_not_found) hint=" Check the channel name; a private channel needs its ID rather than its name." ;; + invalid_auth|not_authed|token_revoked|account_inactive) hint=" Replace SLACK_BOT_TOKEN with the app's current bot token, in both the Actions and the Dependabot secret." ;; + missing_scope) hint=" Give the app the chat:write scope and reinstall it." ;; + *) hint="" ;; + esac + echo "::error::Slack refused the post about $WORKFLOW (run $RUN_URL) in $CHANNEL: HTTP $status, ${error:-$body}.$hint" >&2 exit 1 fi - echo "Posted the failure of $WORKFLOW (run $RUN_URL) to Slack." + echo "Posted the failure of $WORKFLOW (run $RUN_URL) to $CHANNEL." diff --git a/.github/workflows/slack-on-pull-request.yaml b/.github/workflows/slack-on-pull-request.yaml new file mode 100644 index 0000000..2d735ba --- /dev/null +++ b/.github/workflows/slack-on-pull-request.yaml @@ -0,0 +1,134 @@ +name: 'Announce a pull request to Slack' + +# Called from a repository's announcer workflow, which is started by `pull_request` with +# `types: [opened, reopened, ready_for_review]`. A pull request nobody has been told about waits +# until someone happens to open the repository, which is how a stack of Dependabot pull requests +# goes unnoticed; this posts each one that needs action to the caller's channel instead. +# +# Posted: one opened or reopened that is not a draft, and a draft marked ready for review, so a +# draft is announced once it asks for review rather than when it is started. Dependabot's are +# posted like anyone else's. Not posted: one from a fork, whose title and author a stranger +# chooses and would reach the channel through this token, and every other `pull_request` action, +# such as a push to the branch, which is the activity the GitHub app for Slack makes too noisy. +# The filters are in the step rather than the job's `if:` so each skip leaves a notice saying +# why, and so `tests/` can run them. +# +# A post Slack does not accept fails this job, with Slack's answer in the error. +on: + workflow_call: + inputs: + channel: + description: 'The Slack channel to post to, by name, such as `#l337-org`. The app the token belongs to must be a member of it.' + type: string + required: true + secrets: + SLACK_BOT_TOKEN: + description: 'A Slack bot token (`xoxb-`) with the `chat:write` scope. Not required, so that a missing secret fails with a message naming it rather than GitHub refusing the call. A run Dependabot starts sees only Dependabot secrets, so it is set there too (README.md).' + required: false + +# The step reads only the event payload. +permissions: {} + +jobs: + announce: + runs-on: [ubuntu-latest] + name: Announce the pull request to Slack + timeout-minutes: 5 + steps: + - name: Post the pull request + env: + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} + CHANNEL: ${{ inputs.channel }} + EVENT_NAME: ${{ github.event_name }} + ACTION: ${{ github.event.action }} + REPO: ${{ github.repository }} + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + DRAFT: ${{ github.event.pull_request.draft }} + NUMBER: ${{ github.event.pull_request.number }} + TITLE: ${{ github.event.pull_request.title }} + AUTHOR: ${{ github.event.pull_request.user.login }} + BASE: ${{ github.event.pull_request.base.ref }} + PR_URL: ${{ github.event.pull_request.html_url }} + run: | + set -euo pipefail + # `pull_request_target` is refused along with everything else: it would hand the token + # to a run over a fork's pull request, which is the case the fork filter exists for. + if [ "$EVENT_NAME" != pull_request ]; then + echo "::error::This workflow announces a pull request, and was called from a \`$EVENT_NAME\` event. Call it from a workflow triggered by \`pull_request\` with \`types: [opened, reopened, ready_for_review]\`." >&2 + exit 1 + fi + case "$ACTION" in + opened|reopened|ready_for_review) ;; + *) + echo "::notice::Not posted: pull request #$NUMBER was $ACTION, which is not one of opened, reopened or ready_for_review. Narrow the caller's \`types:\` to those three." + exit 0 + ;; + esac + # A fork that has since been deleted has no head repository, and is skipped with the rest. + if [ "$HEAD_REPO" != "$REPO" ]; then + echo "::notice::Not posted: pull request #$NUMBER comes from ${HEAD_REPO:-a deleted fork}, not $REPO." + exit 0 + fi + if [ "$DRAFT" = true ]; then + echo "::notice::Not posted: pull request #$NUMBER is a draft. It is posted when it is marked ready for review." + exit 0 + fi + + if [ -z "$SLACK_BOT_TOKEN" ]; then + echo "::error::No SLACK_BOT_TOKEN secret reached this workflow, so pull request #$NUMBER ($PR_URL) was not announced. Set it on the organisation as an Actions secret and, for Dependabot's pull requests, as a Dependabot secret, and pass it in the caller's \`secrets:\`." >&2 + exit 1 + fi + if [ -z "$CHANNEL" ]; then + echo "::error::No channel was given, so pull request #$NUMBER ($PR_URL) was not announced. Pass the channel name in the caller's \`with: channel:\`." >&2 + exit 1 + fi + + case "$ACTION" in + opened) what=opened ;; + reopened) what=reopened ;; + ready_for_review) what="ready for review" ;; + esac + # Slack reads &, < and > as markup in message text. + escape() { sed -e 's/&/\&/g' -e 's//\>/g' <<<"$1"; } + text="Pull request $what in $(escape "$REPO"): <$PR_URL|#$NUMBER $(escape "$TITLE")> + By \`$(escape "$AUTHOR")\`, into \`$(escape "$BASE")\`" + + # The token goes to curl in a file, not on its command line, where any process on the + # runner could read it. The body goes to a file because a retry truncates a file, + # where to standard output it would follow the refused attempt's body. + umask 077 + header_file=$(mktemp) + body_file=$(mktemp) + trap 'rm -f "$header_file" "$body_file"' EXIT + printf 'Authorization: Bearer %s\n' "$SLACK_BOT_TOKEN" > "$header_file" + # Unfurling is off so the post stays one line of text rather than a preview card. + status=$(jq -n --arg channel "$CHANNEL" --arg text "$text" \ + '{channel: $channel, text: $text, unfurl_links: false, unfurl_media: false}' \ + | curl --silent --show-error --max-time 30 --retry 3 \ + --header @"$header_file" --header 'Content-Type: application/json; charset=utf-8' \ + --data @- --output "$body_file" --write-out '%{http_code}' \ + https://slack.com/api/chat.postMessage) || { + echo "::error::Announcing pull request #$NUMBER ($PR_URL) in $CHANNEL failed: curl could not complete the request." >&2 + exit 1 + } + # Slack answers 200 with `"ok": false` and an `error` code for most refusals, such as + # `not_in_channel`, `channel_not_found` or `invalid_auth`. An accepted post echoes the + # message back, so the cap for reading the answer is generous; a refusal that is not + # Slack's JSON, such as a proxy's error page, is quoted shorter. + body=$(head -c 65536 "$body_file") + if [ "$status" != 200 ] || [ "$(jq -r '.ok // empty' <<<"$body" 2>/dev/null)" != true ]; then + error=$(jq -r '.error // empty' <<<"$body" 2>/dev/null || true) + if [ "$(wc -c < "$body_file")" -gt 500 ]; then + body="$(head -c 500 "$body_file") ... (truncated at 500 bytes)" + fi + case "$error" in + not_in_channel) hint=" Invite the Slack app to $CHANNEL." ;; + channel_not_found) hint=" Check the channel name; a private channel needs its ID rather than its name." ;; + invalid_auth|not_authed|token_revoked|account_inactive) hint=" Replace SLACK_BOT_TOKEN with the app's current bot token, in both the Actions and the Dependabot secret." ;; + missing_scope) hint=" Give the app the chat:write scope and reinstall it." ;; + *) hint="" ;; + esac + echo "::error::Slack refused the announcement of pull request #$NUMBER ($PR_URL) in $CHANNEL: HTTP $status, ${error:-$body}.$hint" >&2 + exit 1 + fi + echo "Announced pull request #$NUMBER ($PR_URL) in $CHANNEL." diff --git a/README.md b/README.md index ad300de..d45cec3 100644 --- a/README.md +++ b/README.md @@ -1,15 +1,16 @@ # github-workflows Shared GitHub workflows and actions for the L337-org organisation. Each repository calls them -pinned to a full commit SHA, which its own action-pins check requires, and Dependabot proposes -the bumps. +at their major-version tag, such as `@v2`, so a change released here reaches every repository +without a pull request in each. See [Versions and releases](#versions-and-releases). | Path | What it is | Call it as | |---|---|---| -| `actions/repo-hygiene` | Composite action: the conventions every repository shares - no internal references, every CI job bounded, every failed run reported, the instruction layer intact, the detail layer routed. | a step, after `actions/checkout` | -| `actions/action-pins` | Composite action: every `uses:` names a 40-hex commit SHA. | a step, after `actions/checkout` | +| `actions/repo-hygiene` | Composite action: the conventions every repository shares - no internal references, every CI job bounded, every failed run reported, every Slack post given a channel, 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, or a version tag of this repository. | 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 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-pull-request.yaml` | Reusable workflow: posts a pull request that needs action to Slack - opened, reopened, or marked ready for review - and nothing else about it. | a job with `uses:`, in a workflow triggered by `pull_request` | ## Using them @@ -21,7 +22,7 @@ jobs: timeout-minutes: 5 steps: - uses: actions/checkout@ # vX.Y.Z - - uses: L337-org/github-workflows/actions/repo-hygiene@ + - uses: L337-org/github-workflows/actions/repo-hygiene@v2 action-pins: runs-on: [ubuntu-latest] @@ -29,7 +30,7 @@ jobs: timeout-minutes: 5 steps: - uses: actions/checkout@ # vX.Y.Z - - uses: L337-org/github-workflows/actions/action-pins@ + - uses: L337-org/github-workflows/actions/action-pins@v2 ``` A job that calls a reusable workflow cannot declare `timeout-minutes`; the called workflow's @@ -84,8 +85,8 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. 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 +- **Bot and fork pull requests are skipped.** GitHub gives no Actions 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 whoever starts the run, since a person pushing to it or asking by comment brings the token with them; a request by comment gets a reply saying so. A fork pull request is refused even when a @@ -122,11 +123,12 @@ permissions: jobs: slack: name: Report to Slack - uses: L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@ # vX.Y.Z + uses: L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@v2 with: + channel: '#l337-org' events: '["schedule", "push", "release", "pull_request"]' secrets: - SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }} + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} ``` - **What is reported.** A completed run of a listed workflow started by one of the `events`, @@ -146,13 +148,12 @@ jobs: `release` or `pull_request` that no watcher reports for that trigger, on one with no `name:`, and on a listed name no workflow has, since GitHub matches on the name and a rename leaves the list reporting nothing. -- **Where it goes.** To the channel of the incoming webhook in the `SLACK_WEBHOOK` secret. A - secret set on the organisation is every repository's default, and a repository secret of the - same name overrides it. An incoming webhook posts to one channel and can do nothing else. +- **Where it goes.** To the caller's `channel`, through the bot token in `SLACK_BOT_TOKEN` + (see [The Slack token](#the-slack-token)). - **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. A transient refusal is + secret or channel, a refusal such as `not_in_channel`, or Slack unreachable after retries. A transient refusal is retried, which can post the same failure twice. **Accepted limitation:** the watcher's run is - itself a run nobody is watching, and nothing reports it, so a webhook that stops working is + itself a run nobody is watching, and nothing reports it, so a token that stops working is noticed only by the absence of posts. Filing an issue instead would scatter them across repositories if filed in the failing one, and if filed here would need either a credential that can write across repositories or a scheduled sweep of every repository's watcher runs. @@ -161,15 +162,80 @@ jobs: - **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. +## Announcing pull requests to Slack + +A pull request nobody has been told about waits until someone happens to open its repository, +which is how a stack of Dependabot pull requests goes unnoticed. The GitHub app for Slack posts +every later event on a pull request too, so each repository has an announcer instead, posting +only when a pull request needs action: + +```yaml +name: 'Announce pull requests' +on: + pull_request: + types: [opened, reopened, ready_for_review] +permissions: {} +jobs: + slack: + name: Announce to Slack + uses: L337-org/github-workflows/.github/workflows/slack-on-pull-request.yaml@v2 + with: + channel: '#l337-org' + secrets: + SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} +``` + +- **What is posted.** A pull request opened or reopened that is not a draft, and a draft marked + ready for review, so a draft is announced when it asks for review rather than when it is + started. Dependabot's are posted like anyone else's. The post names the repository, links + the pull request by number and title, and gives its author and base branch. +- **What is not.** A pull request from a fork, whose title and author a stranger chooses and + would reach the channel through the token. Any other action, such as a push to the branch: + a caller whose `types:` include one gets a notice asking it to narrow them. Each skip leaves + a notice in the run saying why. A call from any event but `pull_request` fails, including + `pull_request_target`, which would hand the token to a run over a fork's pull request. +- **Which channel.** `#docker-mcp`, `#quaffed` and `#send-to-influx` serve their repositories; + every other repository posts to `#l337-org`. +- **List it in the watcher.** It runs on `pull_request`, so the hygiene check requires its + `name:` in `report-failures.yaml` like any other workflow. A failed announcement is reported + through the same token, so a token that stops working silences both. + +## The Slack token + +Both workflows post with `chat.postMessage` as the L337-org Slack app, through its bot token +(`xoxb-`), to the channel each caller names. One token serves every channel, where an incoming +webhook serves one channel each and would need a secret per channel, twice over. + +- **Two organisation secrets, one value.** `SLACK_BOT_TOKEN` as an Actions secret, and again as + a Dependabot secret, because a run Dependabot starts sees only Dependabot secrets. Without the + second, Dependabot's pull requests are not announced. An organisation secret on GitHub Free + reaches public repositories only. +- **The app must be in the channel.** Invite it to each channel a caller names, or the post + fails with `not_in_channel`. A channel is named rather than given by ID, which works for a + public channel only; a private one needs its ID. +- **What the token can do.** More than a webhook could: it can post to every channel the app is + in, and do whatever else the app's scopes allow. Keep the app's bot scopes to `chat:write`, so + a leaked token can post and nothing more. **Accepted limitation:** a leaked token can post to + any of the app's channels, where a leaked webhook reached one; one secret pair to maintain + instead of two per channel was judged worth that. +- **Rotating it.** Reinstall the app or regenerate the token under "OAuth & Permissions" at + api.slack.com/apps, and replace both secrets. A refused token fails each post with + `invalid_auth` or `token_revoked`, naming both secrets. +- **Moving a caller from v1.** Version 1 takes `SLACK_WEBHOOK` and no `channel`; version 2 + requires `channel` and `SLACK_BOT_TOKEN`. Move every reference to `@v2` and change the + callers in the same pull request. The v2 hygiene check fails a call to either workflow that + names no channel, so a move that leaves a caller unchanged fails its own pull request rather + than breaking after merge. + ## Running the hygiene check locally -From the root of the repository to check, fetch the script at the commit that repository's -`repo-hygiene` step pins, so a local run checks what its CI checks, and run it there: +From the root of the repository to check, fetch the script at the version that repository's +`repo-hygiene` step names, so a local run checks what its CI checks, and run it there: ```bash -sha= +ref= gh api -H 'Accept: application/vnd.github.raw' \ - "repos/L337-org/github-workflows/contents/actions/repo-hygiene/check-repo-hygiene.py?ref=$sha" \ + "repos/L337-org/github-workflows/contents/actions/repo-hygiene/check-repo-hygiene.py?ref=$ref" \ > /tmp/check-repo-hygiene.py uv run --script /tmp/check-repo-hygiene.py . ``` @@ -182,11 +248,47 @@ uv run --script actions/repo-hygiene/check-repo-hygiene.py /path/to/repository Exit status is 0 clean, 1 on findings, 2 when the scan could not be trusted. +## Versions and releases + +Callers name a major-version tag, `v2`, which is moved to each release of that major version. +Each release also gets its full tag, such as `v2.0.0`, which never moves. + +- **Why a tag and not a commit.** A commit pin needs a pull request in every repository for + every change here. A tag of another repository would normally be refused, because its owner + can repoint it (DS.6). Here the `v*` tags are protected by a ruleset that blocks creating, + moving and deleting them, with a bypass list no wider than `main`'s, so moving `v2` takes the + same people as merging to `main`. The action-pins check accepts this repository's version + tags and nothing else's; third-party actions still need a commit SHA. +- **Releasing.** Merge to `main`, publish a release with its full tag, then move the major tag + to the same commit: + + ```bash + git fetch origin --tags + git tag -f v2 v2.0.1 + git push -f origin refs/tags/v2 + ``` + + Every caller runs the new code from its next run. Tag only commits that are on `main`: the + ruleset cannot check that. +- **A breaking change is a new major version.** Anything that would make an unchanged caller + stop working or behave differently - a new required input, a removed one, a renamed secret - + is released as `v3.0.0` with a new `v3` tag, and `v2` stays where it is. Each repository then + moves to `v3` in its own pull request, changing its callers at the same time, and nothing + breaks in between. + ## Changing anything here -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 +Everything here runs in other repositories' CI, so a change reaches them as soon as its release +moves the major tag. This repository's own CI runs the action-pins action from the commit under +review, lints every workflow and action with actionlint, runs the Slack workflows' posting steps +against stand-ins for `curl` and `gh` and the action-pins check over sample references +(`tests/`), 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. + +To run the tests locally, from the root of a clone: + +```bash +PYTHONDONTWRITEBYTECODE=1 uvx --with pyyaml==6.0.3 pytest==8.3.5 -p no:cacheprovider -q tests +``` diff --git a/actions/action-pins/action.yml b/actions/action-pins/action.yml index 91ed99f..304a4da 100644 --- a/actions/action-pins/action.yml +++ b/actions/action-pins/action.yml @@ -1,13 +1,19 @@ name: 'Action pins are immutable' -description: 'Fail when any `uses:` in the checked-out repository names something other than a full 40-hex commit SHA.' +description: 'Fail when any `uses:` in the checked-out repository names something other than a full 40-hex commit SHA, or a version tag of L337-org/github-workflows.' # A tag or branch ref can be repointed by the action's owner at any time, so `@v7` means CI runs -# whatever that owner last pushed to it. First-party actions/* are not exempt (DS.6.2): +# whatever that owner last pushed to it. First-party actions/* are not exempt (DS.6.1): # Dependabot bumps the SHA and rewrites the trailing "# vX.Y.Z" comment, so the pin costs nothing # to hold. Local actions and workflows (./...) are exempt - they are the repository's own code # at the same commit. A job-level `uses:` calling another repository's reusable workflow is a # `uses:` like any other and needs the same pin. # +# The one exception is L337-org/github-workflows at a version tag, such as `@v2` (DS.6.2). Its +# `v*` tags are protected by a ruleset whose bypass list is no wider than its main branch's, so +# moving one takes the same people as merging there, and a breaking change goes to a new major +# tag while callers stay on the old one. It is named exactly rather than as the organisation, +# because the exemption rests on that ruleset, which another repository may not have. +# # Quoted and upper-case SHAs are accepted because they are equally pinned, and the trailing # version comment is not enforced (DS.6.3): a guard that fails a legitimate pin for a cosmetic # reason is one somebody eventually switches off wholesale. @@ -20,7 +26,7 @@ inputs: runs: using: composite steps: - - name: Every `uses:` must name a 40-hex commit SHA + - name: Every `uses:` must name a 40-hex commit SHA or a protected version tag shell: bash working-directory: ${{ github.workspace }} env: @@ -38,11 +44,12 @@ runs: fi bad=$(grep -rnE '^[[:space:]]*(-[[:space:]]+)?uses:' "${present[@]}" \ | grep -vE "uses:[[:space:]]*[\"']?\./" \ + | grep -vE "uses:[[:space:]]*[\"']?L337-org/github-workflows/[^[:space:]\"'@]+@v[0-9]+(\.[0-9]+){0,2}[\"']?([[:space:]]|\$)" \ | grep -vE "uses:[[:space:]]*[\"']?[^[:space:]\"']+@[0-9a-fA-F]{40}[\"']?([[:space:]]|\$)" \ || true) if [ -n "$bad" ]; then - echo "::error::Every action must be pinned to a full 40-hex commit SHA." >&2 + echo "::error::Every action must be pinned to a full 40-hex commit SHA, or for L337-org/github-workflows a version tag such as @v2." >&2 printf '%s\n' "$bad" >&2 exit 1 fi - echo "All action references in ${present[*]} are pinned to commit SHAs." + echo "All action references in ${present[*]} are pinned to commit SHAs or protected version tags." diff --git a/actions/repo-hygiene/action.yml b/actions/repo-hygiene/action.yml index 5d54b47..bc8f27f 100644 --- a/actions/repo-hygiene/action.yml +++ b/actions/repo-hygiene/action.yml @@ -2,7 +2,7 @@ name: 'Repository hygiene' description: 'Check the conventions every repository in the organisation shares, over the checked-out repository.' # Run after actions/checkout in the calling job, which keeps its own timeout-minutes. The script -# is this action's copy at the commit the caller pins, so every repository pinning one commit +# is this action's copy at the version the caller names, so every repository on one version # runs the same checks. inputs: path: diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index 858eb8a..5de8b34 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -6,10 +6,10 @@ """Repository conventions that every repository in this organisation shares. ONE COPY. This file lives in the shared workflows repository and runs through the -`repo-hygiene` action there, which each repository calls pinned to a commit. It used to be +`repo-hygiene` action there, which each repository calls at a version tag. It used to be vendored byte-identically into every repository, with a recorded digest and a scheduled divergence watch. A single copy makes both unnecessary: a repository behind the others now -shows as an older pin rather than as a copy that has drifted. +shows as an older version rather than as a copy that has drifted. It checks the repository it is run in - the path given as its one argument, or the current directory. To run it locally, fetch this file at the commit your repository pins and run @@ -116,6 +116,12 @@ r"(?:\./|L337-org/github-workflows/)\.github/workflows/slack-on-failure\.yaml(?:@|$)" ) +# A call to either shared Slack workflow, each of which posts to the channel its caller names. +SLACK_POSTER = re.compile( + r"(?:\./|L337-org/github-workflows/)\.github/workflows/" + r"slack-on-(?:failure|pull-request)\.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 @@ -629,6 +635,54 @@ def check_failed_runs_are_reported(root, tracked, files): return reported_run_findings(workflow_documents(root, tracked)) +def slack_channel_findings(documents): + """The findings of check_slack_posts_name_a_channel, over parsed workflows. + + Args: + documents (list): `(repo-relative path, document)` pairs, as workflow_documents returns. + + Returns: + list: The findings. + """ + findings = [] + for relative, document in documents: + jobs = document.get("jobs") + for job, body in (jobs.items() if isinstance(jobs, dict) else ()): + if not (isinstance(body, dict) and isinstance(body.get("uses"), str) + and SLACK_POSTER.match(body["uses"])): + continue + given = body.get("with") + channel = given.get("channel") if isinstance(given, dict) else None + if not (isinstance(channel, str) and channel.strip()): + findings.append( + f"{relative} job {job!r} calls {body['uses']!r} without a `with: channel:`, " + f"so it posts nowhere: name the Slack channel, such as '#l337-org'" + ) + return findings + + +def check_slack_posts_name_a_channel(root, tracked, files): + """Every call to a shared Slack workflow names the channel it posts to. + + Both workflows post through one bot token to the channel their caller names, and the input + is required, so a caller written for the webhook the token replaced stops running when it + moves to the version that requires it. Its watcher only fires from the default branch, so + nothing else would say so before the move is merged. + + 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. + """ + return slack_channel_findings(workflow_documents(root, tracked)) + + # Markdown links plus backtick-quoted paths, since a router routes both ways. ROUTED_LINK = re.compile(r"\[[^\]]*\]\(([^)]+)\)|`([^`]+\.md)`") @@ -833,6 +887,7 @@ def check_the_detail_layer_routing(root, tracked, files): ("no internal references in a public repository", check_no_internal_references), ("every CI job is bounded", check_ci_jobs_are_bounded), ("failed runs are reported", check_failed_runs_are_reported), + ("Slack posts name a channel", check_slack_posts_name_a_channel), ("the instruction layer is intact", check_the_instruction_layer), ("the detail layer and the router agree", check_the_detail_layer_routing), ) diff --git a/tests/test_action_pins.py b/tests/test_action_pins.py new file mode 100644 index 0000000..731e258 --- /dev/null +++ b/tests/test_action_pins.py @@ -0,0 +1,57 @@ +"""Behaviour of the action-pins check: which `uses:` references it accepts and which it fails. + +The step's script is read out of `actions/action-pins/action.yml` and run under bash over a +scratch directory of workflow files, so what is tested is the script that ships. +""" + +import pathlib +import subprocess + +import pytest +import yaml + +ROOT = pathlib.Path(__file__).resolve().parent.parent +SHA = "3d3c42e5aac5ba805825da76410c181273ba90b1" + + +def check(tmp_path, uses): + (tmp_path / "workflows").mkdir() + (tmp_path / "workflows" / "ci.yaml").write_text(f"jobs:\n a:\n steps:\n - uses: {uses}\n") + action = yaml.safe_load((ROOT / "actions" / "action-pins" / "action.yml").read_text()) + (step,) = action["runs"]["steps"] + return subprocess.run( + ["bash", "-c", step["run"]], cwd=tmp_path, env={"DIRECTORIES": "workflows", "PATH": "/usr/bin:/bin"}, + capture_output=True, text=True, timeout=30, + ) + + +@pytest.mark.parametrize("uses", [ + f"actions/checkout@{SHA}", + f"actions/checkout@{SHA} # v7.0.1", + f"'actions/checkout@{SHA}'", + "./.github/workflows/slack-on-failure.yaml", + "L337-org/github-workflows/.github/workflows/slack-on-pull-request.yaml@v2", + "L337-org/github-workflows/actions/repo-hygiene@v2", + "L337-org/github-workflows/actions/repo-hygiene@v2.0.0", + "'L337-org/github-workflows/actions/action-pins@v2'", +]) +def test_accepted(tmp_path, uses): + result = check(tmp_path, uses) + assert result.returncode == 0, result.stderr + + +@pytest.mark.parametrize("uses", [ + "actions/checkout@v7", + "actions/checkout@main", + f"actions/checkout@{SHA[:12]}", + "L337-org/github-workflows/actions/repo-hygiene@main", + "L337-org/github-workflows/actions/repo-hygiene@v2-rc", + "L337-org/github-workflows/actions/repo-hygiene@vnext", + "L337-org/other-repo/actions/thing@v2", + "someone-else/github-workflows/actions/repo-hygiene@v2", + "evil/L337-org/github-workflows/actions/repo-hygiene@v2", +]) +def test_refused(tmp_path, uses): + result = check(tmp_path, uses) + assert result.returncode == 1 + assert "pinned to a full 40-hex commit SHA" in result.stderr diff --git a/tests/test_slack_workflows.py b/tests/test_slack_workflows.py new file mode 100644 index 0000000..5a5a6ee --- /dev/null +++ b/tests/test_slack_workflows.py @@ -0,0 +1,278 @@ +"""Behaviour of the two shared Slack workflows' posting steps, and the hygiene check over callers. + +Each step's script is read out of its workflow file and run under bash with the environment +GitHub would give it, against stand-ins for `curl` and `gh` on PATH. So what is tested is the +script that ships, not a copy of it. What this cannot test: the GitHub expressions that fill the +environment, and Slack itself, which is exercised by the first announcement after merge. +""" + +import importlib.util +import json +import os +import pathlib +import stat +import subprocess +import textwrap + +import pytest +import yaml + +ROOT = pathlib.Path(__file__).resolve().parent.parent +WORKFLOWS = ROOT / ".github" / "workflows" +TOKEN = "xoxb-test-" + os.urandom(8).hex() + +# Records its arguments, its standard input and the header file it was handed, writes the +# canned answer to --output, and prints the canned status, as curl's --write-out would. +CURL = textwrap.dedent("""\ + #!/usr/bin/env bash + printf '%s\\n' "$@" > "$STUB_DIR/curl.args" + cat > "$STUB_DIR/curl.stdin" + out="" + while [ $# -gt 0 ]; do + case "$1" in + --output) out="$2"; shift ;; + --header) case "$2" in @*) cat "${2#@}" >> "$STUB_DIR/curl.headers" ;; esac; shift ;; + esac + shift + done + printf '%s' "$STUB_BODY" > "$out" + printf '%s' "$STUB_STATUS" + exit "${STUB_EXIT:-0}" +""") + +GH = textwrap.dedent("""\ + #!/usr/bin/env bash + printf '%s' "${STUB_GH:-}" +""") + + +def step_script(workflow): + document = yaml.safe_load((WORKFLOWS / workflow).read_text()) + (job,) = document["jobs"].values() + (step,) = [s for s in job["steps"] if "run" in s] + return step["run"] + + +@pytest.fixture +def stubs(tmp_path): + for name, body in (("curl", CURL), ("gh", GH)): + path = tmp_path / name + path.write_text(body) + path.chmod(path.stat().st_mode | stat.S_IXUSR) + return tmp_path + + +def run(workflow, stubs, env, status="200", body='{"ok": true}', curl_exit=0): + full = { + "PATH": f"{stubs}:{os.environ['PATH']}", + "STUB_DIR": str(stubs), + "STUB_STATUS": status, + "STUB_BODY": body, + "STUB_EXIT": str(curl_exit), + "TMPDIR": str(stubs), + **env, + } + result = subprocess.run( + ["bash", "-c", step_script(workflow)], env=full, capture_output=True, text=True, + timeout=30, + ) + posted = (stubs / "curl.stdin").exists() + return result, posted + + +def pull_request(**overrides): + env = { + "SLACK_BOT_TOKEN": TOKEN, + "CHANNEL": "#l337-org", + "EVENT_NAME": "pull_request", + "ACTION": "opened", + "REPO": "L337-org/quaffed", + "HEAD_REPO": "L337-org/quaffed", + "DRAFT": "false", + "NUMBER": "42", + "TITLE": "Bump & stuff", + "AUTHOR": "dependabot[bot]", + "BASE": "main", + "PR_URL": "https://github.com/L337-org/quaffed/pull/42", + } + env.update(overrides) + return env + + +ANNOUNCER = "slack-on-pull-request.yaml" + + +@pytest.mark.parametrize("action,what", [ + ("opened", "opened"), ("reopened", "reopened"), ("ready_for_review", "ready for review"), +]) +def test_an_actionable_pull_request_is_posted(stubs, action, what): + result, posted = run(ANNOUNCER, stubs, pull_request(ACTION=action)) + assert result.returncode == 0, result.stderr + assert posted + payload = json.loads((stubs / "curl.stdin").read_text()) + assert payload["channel"] == "#l337-org" + assert payload["unfurl_links"] is False and payload["unfurl_media"] is False + assert f"Pull request {what} in L337-org/quaffed" in payload["text"] + assert "#42 Bump <things> & stuff" in payload["text"] + assert "`dependabot[bot]`" in payload["text"] + assert (stubs / "curl.headers").read_text().startswith(f"Authorization: Bearer {TOKEN}") + assert "https://slack.com/api/chat.postMessage" in (stubs / "curl.args").read_text() + + +def test_the_token_is_not_on_curls_command_line(stubs): + run(ANNOUNCER, stubs, pull_request()) + assert TOKEN not in (stubs / "curl.args").read_text() + + +@pytest.mark.parametrize("overrides,reason", [ + ({"HEAD_REPO": "stranger/quaffed"}, "comes from stranger/quaffed"), + ({"HEAD_REPO": ""}, "comes from a deleted fork"), + ({"DRAFT": "true"}, "is a draft"), + ({"DRAFT": "true", "ACTION": "reopened"}, "is a draft"), + ({"ACTION": "synchronize"}, "was synchronize"), +]) +def test_a_pull_request_needing_no_action_is_skipped_with_a_notice(stubs, overrides, reason): + result, posted = run(ANNOUNCER, stubs, pull_request(**overrides)) + assert result.returncode == 0, result.stderr + assert not posted + assert "::notice::Not posted" in result.stdout and reason in result.stdout + + +def test_a_fork_is_skipped_even_without_a_token(stubs): + result, posted = run(ANNOUNCER, stubs, pull_request(HEAD_REPO="stranger/quaffed", SLACK_BOT_TOKEN="")) + assert result.returncode == 0 and not posted + + +@pytest.mark.parametrize("event", ["pull_request_target", "push"]) +def test_any_other_trigger_fails(stubs, event): + result, posted = run(ANNOUNCER, stubs, pull_request(EVENT_NAME=event)) + assert result.returncode == 1 and not posted + assert f"called from a `{event}` event" in result.stderr + + +@pytest.mark.parametrize("overrides,message", [ + ({"SLACK_BOT_TOKEN": ""}, "No SLACK_BOT_TOKEN secret"), + ({"CHANNEL": ""}, "No channel was given"), +]) +def test_missing_configuration_fails_naming_it(stubs, overrides, message): + result, posted = run(ANNOUNCER, stubs, pull_request(**overrides)) + assert result.returncode == 1 and not posted + assert message in result.stderr + + +@pytest.mark.parametrize("error,hint", [ + ("not_in_channel", "Invite the Slack app to #l337-org"), + ("channel_not_found", "Check the channel name"), + ("invalid_auth", "Replace SLACK_BOT_TOKEN"), + ("missing_scope", "chat:write"), + ("something_new", ""), +]) +def test_a_refusal_fails_with_slacks_error(stubs, error, hint): + result, _ = run(ANNOUNCER, stubs, pull_request(), body=json.dumps({"ok": False, "error": error})) + assert result.returncode == 1 + assert f"HTTP 200, {error}." in result.stderr and hint in result.stderr + + +def test_a_non_slack_answer_is_quoted_and_marked_truncated(stubs): + result, _ = run(ANNOUNCER, stubs, pull_request(), status="502", body="" + "x" * 1000) + assert result.returncode == 1 + assert "HTTP 502, " in result.stderr and "(truncated at 500 bytes)" in result.stderr + + +def test_a_long_acceptance_is_still_read_as_accepted(stubs): + body = json.dumps({"ok": True, "message": {"text": "x" * 5000}}) + result, _ = run(ANNOUNCER, stubs, pull_request(), body=body) + assert result.returncode == 0, result.stderr + + +def test_curl_failing_outright_fails(stubs): + result, _ = run(ANNOUNCER, stubs, pull_request(), curl_exit=28) + assert result.returncode == 1 and "curl could not complete" in result.stderr + + +REPORTER = "slack-on-failure.yaml" + + +def failed_run(**overrides): + env = { + "SLACK_BOT_TOKEN": TOKEN, + "CHANNEL": "#docker-mcp", + "GH_TOKEN": "unused", + "STUB_GH": "build (failure)", + "EVENT_NAME": "workflow_run", + "REPO": "L337-org/docker-mcp", + "WORKFLOW": "Check inbound changes", + "CONCLUSION": "failure", + "RUN_ID": "1", + "RUN_NUMBER": "7", + "RUN_ATTEMPT": "1", + "RUN_URL": "https://github.com/L337-org/docker-mcp/actions/runs/1", + "BRANCH": "main", + "SHA": "0123456789abcdef", + "TRIGGER": "push", + "WORKFLOW_ID": "9", + "DEFAULT_BRANCH": "main", + } + env.update(overrides) + return env + + +def test_a_failed_run_is_posted_to_the_callers_channel(stubs): + result, posted = run(REPORTER, stubs, failed_run()) + assert result.returncode == 0, result.stderr + payload = json.loads((stubs / "curl.stdin").read_text()) + assert payload["channel"] == "#docker-mcp" + assert "*Check inbound changes*" in payload["text"] and "build (failure)" in payload["text"] + assert (stubs / "curl.headers").read_text().startswith(f"Authorization: Bearer {TOKEN}") + assert TOKEN not in (stubs / "curl.args").read_text() + + +@pytest.mark.parametrize("overrides,message", [ + ({"SLACK_BOT_TOKEN": ""}, "No SLACK_BOT_TOKEN secret"), + ({"CHANNEL": ""}, "No channel was given"), +]) +def test_the_reporter_fails_on_missing_configuration(stubs, overrides, message): + result, posted = run(REPORTER, stubs, failed_run(**overrides)) + assert result.returncode == 1 and not posted + assert message in result.stderr + + +def test_the_reporter_fails_with_slacks_error(stubs): + result, _ = run(REPORTER, stubs, failed_run(), body='{"ok": false, "error": "not_in_channel"}') + assert result.returncode == 1 + assert "not_in_channel" in result.stderr and "Invite the Slack app to #docker-mcp" in result.stderr + + +def hygiene(): + spec = importlib.util.spec_from_file_location( + "check_repo_hygiene", ROOT / "actions" / "repo-hygiene" / "check-repo-hygiene.py" + ) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.mark.parametrize("uses", [ + "L337-org/github-workflows/.github/workflows/slack-on-pull-request.yaml@" + "0" * 40, + "L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@" + "0" * 40, + "./.github/workflows/slack-on-failure.yaml", +]) +@pytest.mark.parametrize("given,found", [ + ({"channel": "#quaffed"}, False), + ({"channel": " "}, True), + ({}, True), + (None, True), +]) +def test_hygiene_requires_a_channel_on_every_slack_call(uses, given, found): + body = {"uses": uses} + if given is not None: + body["with"] = given + findings = hygiene().slack_channel_findings([(".github/workflows/x.yaml", {"jobs": {"slack": body}})]) + assert bool(findings) is found + + +def test_every_shipped_caller_here_names_a_channel(): + documents = [ + (p.name, yaml.safe_load(p.read_text())) for p in sorted(WORKFLOWS.glob("*.yaml")) + ] + assert hygiene().slack_channel_findings(documents) == [] From 12e2428800ee735639dad768403f84e3029e0fce Mon Sep 17 00:00:00 2001 From: CL Date: Thu, 15 Oct 2026 15:23:10 +0100 Subject: [PATCH 2/3] Check the token as well as the channel on Slack calls, and test both posting copies The hygiene check caught a caller moved to v2 without a channel, but not one still passing SLACK_WEBHOOK or not passing SLACK_BOT_TOKEN. That caller fails only after merge, in a watcher nothing reports, which is the outcome the check exists to prevent. It now fails both; `secrets: inherit` passes, since it carries the token. The posting and refusal handling is the same code copied into both Slack workflows, and only the announcer's copy was tested in depth. Every refusal, truncation, acceptance and curl failure case now runs against both, so a copy that drifts fails; breaking the reporter's truncation alone fails one test. The README and action-pins state the channel choice and the tag ruleset as conditions rather than as a description of current settings, which go stale. The premerge job is renamed for what it runs, which includes the action-pins tests, and its comment gives the real reason. --- .github/workflows/premerge.yaml | 9 +- README.md | 62 ++++++------ actions/action-pins/action.yml | 8 +- actions/repo-hygiene/check-repo-hygiene.py | 34 +++++-- tests/test_slack_workflows.py | 106 +++++++++++++-------- 5 files changed, 131 insertions(+), 88 deletions(-) diff --git a/.github/workflows/premerge.yaml b/.github/workflows/premerge.yaml index b788b95..515593f 100644 --- a/.github/workflows/premerge.yaml +++ b/.github/workflows/premerge.yaml @@ -48,11 +48,12 @@ jobs: # is always Markdown backticks in a printf format, which is the intent. run: uvx --from actionlint-py==1.7.12.25 actionlint -ignore 'SC2016' - slack-workflows: - # The Slack workflows run only in other repositories' events - a pull request opening, a run - # failing - so their posting steps are run here against stand-ins for curl and gh instead. + shared-behaviour: + # What the Slack workflows do on a refusal, a fork or a draft cannot be made to happen from a + # pull request here, and the action-pins check needs references this repository does not + # contain, so both are run against stand-ins and sample input instead. runs-on: [ubuntu-latest] - name: Slack workflows behave + name: Shared workflows and actions behave timeout-minutes: 5 env: # The tests import the hygiene script, and a checkout is left as it was found. diff --git a/README.md b/README.md index d45cec3..bf83a57 100644 --- a/README.md +++ b/README.md @@ -85,14 +85,14 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. 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 Actions 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 - whoever starts the run, since a person pushing to it or asking by comment brings the token with - them; a request by comment gets a reply saying so. A fork pull request is refused even when a - maintainer asks by comment, because that run carries the token over text written by someone - with no access; the request gets a reply saying so. A notice saying no token arrived, on a pull - request that is neither, means the secret is missing or not passed. +- **Bot and fork pull requests are skipped.** GitHub gives no Actions 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 whoever + starts the run, since a person pushing to it or asking by comment brings the token with them; a + request by comment gets a reply saying so. A fork pull request is refused even when a maintainer + asks by comment, because that run carries the token over text written by someone with no access; + the request gets a reply saying so. A notice saying no token arrived, on a pull request that is + neither, means the secret is missing or not passed. - **A review that returns no verdict says why.** The comment headed `Claude review did not complete` and the run log both carry the session's result record - its status and error text, such as a usage limit - without the usage and cost figures. The comment carries no marker, so @@ -150,13 +150,13 @@ jobs: list reporting nothing. - **Where it goes.** To the caller's `channel`, through the bot token in `SLACK_BOT_TOKEN` (see [The Slack token](#the-slack-token)). -- **A post that fails, fails the watcher's run**, with Slack's answer in the error: a missing - secret or channel, a refusal such as `not_in_channel`, or Slack unreachable after retries. A transient refusal is - retried, which can post the same failure twice. **Accepted limitation:** the watcher's run is - itself a run nobody is watching, and nothing reports it, so a token that stops working is +- **A post that fails, fails the watcher's run**, with Slack's answer in the error: a missing secret + or channel, a refusal such as `not_in_channel`, or Slack unreachable after retries. A transient + refusal is retried, which can post the same failure twice. **Accepted limitation:** the watcher's + run is itself a run nobody is watching, and nothing reports it, so a token that stops working is noticed only by the absence of posts. Filing an issue instead would scatter them across - repositories if filed in the failing one, and if filed here would need either a credential - that can write across repositories or a scheduled sweep of every repository's watcher runs. + repositories if filed in the failing one, and if filed here would need either a credential that + can write across repositories or a scheduled sweep of every repository's watcher runs. - **The watcher only fires from the default branch**, where GitHub reads `workflow_run` triggers. So a change to it is tested only once merged. - **The caller must grant** `actions: read` and `contents: read`, as the example does, or @@ -194,8 +194,8 @@ jobs: a caller whose `types:` include one gets a notice asking it to narrow them. Each skip leaves a notice in the run saying why. A call from any event but `pull_request` fails, including `pull_request_target`, which would hand the token to a run over a fork's pull request. -- **Which channel.** `#docker-mcp`, `#quaffed` and `#send-to-influx` serve their repositories; - every other repository posts to `#l337-org`. +- **Which channel.** A repository with a Slack channel of its own posts there; every other + repository posts to `#l337-org`. - **List it in the watcher.** It runs on `pull_request`, so the hygiene check requires its `name:` in `report-failures.yaml` like any other workflow. A failed announcement is reported through the same token, so a token that stops working silences both. @@ -221,11 +221,11 @@ webhook serves one channel each and would need a secret per channel, twice over. - **Rotating it.** Reinstall the app or regenerate the token under "OAuth & Permissions" at api.slack.com/apps, and replace both secrets. A refused token fails each post with `invalid_auth` or `token_revoked`, naming both secrets. -- **Moving a caller from v1.** Version 1 takes `SLACK_WEBHOOK` and no `channel`; version 2 - requires `channel` and `SLACK_BOT_TOKEN`. Move every reference to `@v2` and change the - callers in the same pull request. The v2 hygiene check fails a call to either workflow that - names no channel, so a move that leaves a caller unchanged fails its own pull request rather - than breaking after merge. +- **Moving a caller from a v1 commit pin.** Version 1 takes `SLACK_WEBHOOK` and no `channel`; + version 2 requires `channel` and `SLACK_BOT_TOKEN`. Move every reference to `@v2` and change + the callers in the same pull request. The v2 hygiene check fails a call to either workflow + that names no channel or does not pass `SLACK_BOT_TOKEN`, so a move that leaves a caller + unchanged fails its own pull request rather than breaking after merge. ## Running the hygiene check locally @@ -253,12 +253,12 @@ Exit status is 0 clean, 1 on findings, 2 when the scan could not be trusted. Callers name a major-version tag, `v2`, which is moved to each release of that major version. Each release also gets its full tag, such as `v2.0.0`, which never moves. -- **Why a tag and not a commit.** A commit pin needs a pull request in every repository for - every change here. A tag of another repository would normally be refused, because its owner - can repoint it (DS.6). Here the `v*` tags are protected by a ruleset that blocks creating, - moving and deleting them, with a bypass list no wider than `main`'s, so moving `v2` takes the - same people as merging to `main`. The action-pins check accepts this repository's version - tags and nothing else's; third-party actions still need a commit SHA. +- **Why a tag and not a commit.** A commit pin needs a pull request in every repository for every + change here. A tag of another repository would normally be refused, because its owner can repoint + it (DS.6). The exemption holds only while a ruleset blocks creating, moving and deleting `v*` + tags here, with a bypass list no wider than `main`'s, so that moving `v2` takes the same people as + merging to `main`; loosening either withdraws it. The action-pins check accepts this repository's + version tags and nothing else's; third-party actions still need a commit SHA. - **Releasing.** Merge to `main`, publish a release with its full tag, then move the major tag to the same commit: @@ -282,10 +282,10 @@ Everything here runs in other repositories' CI, so a change reaches them as soon moves the major tag. This repository's own CI runs the action-pins action from the commit under review, lints every workflow and action with actionlint, runs the Slack workflows' posting steps against stand-ins for `curl` and `gh` and the action-pins check over sample references -(`tests/`), 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. +(`tests/`), 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. To run the tests locally, from the root of a clone: diff --git a/actions/action-pins/action.yml b/actions/action-pins/action.yml index 304a4da..d8e2f68 100644 --- a/actions/action-pins/action.yml +++ b/actions/action-pins/action.yml @@ -8,10 +8,10 @@ description: 'Fail when any `uses:` in the checked-out repository names somethin # at the same commit. A job-level `uses:` calling another repository's reusable workflow is a # `uses:` like any other and needs the same pin. # -# The one exception is L337-org/github-workflows at a version tag, such as `@v2` (DS.6.2). Its -# `v*` tags are protected by a ruleset whose bypass list is no wider than its main branch's, so -# moving one takes the same people as merging there, and a breaking change goes to a new major -# tag while callers stay on the old one. It is named exactly rather than as the organisation, +# The one exception is L337-org/github-workflows at a version tag, such as `@v2` (DS.6.2). It +# holds only while a ruleset there blocks creating, moving and deleting `v*` tags with a bypass +# list no wider than its main branch's, so that moving one takes the same people as merging; a +# breaking change goes to a new major tag while callers stay on the old one. It is named exactly rather than as the organisation, # because the exemption rests on that ruleset, which another repository may not have. # # Quoted and upper-case SHAs are accepted because they are equally pinned, and the trailing diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index 5de8b34..f9da2e7 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -12,7 +12,8 @@ shows as an older version rather than as a copy that has drifted. It checks the repository it is run in - the path given as its one argument, or the current -directory. To run it locally, fetch this file at the commit your repository pins and run +directory. To run it locally, fetch this file at the version your repository's `repo-hygiene` +step names and run `uv run --script check-repo-hygiene.py `. A SCRIPT RATHER THAN A TEST MODULE, DELIBERATELY. These checks are wanted in repositories that @@ -636,7 +637,7 @@ def check_failed_runs_are_reported(root, tracked, files): def slack_channel_findings(documents): - """The findings of check_slack_posts_name_a_channel, over parsed workflows. + """The findings of check_slack_calls_match_v2, over parsed workflows. Args: documents (list): `(repo-relative path, document)` pairs, as workflow_documents returns. @@ -658,15 +659,32 @@ def slack_channel_findings(documents): f"{relative} job {job!r} calls {body['uses']!r} without a `with: channel:`, " f"so it posts nowhere: name the Slack channel, such as '#l337-org'" ) + # `secrets: inherit` passes every secret the caller can see, the token among them. + secrets = body.get("secrets") + if secrets == "inherit": + continue + names = set(secrets) if isinstance(secrets, dict) else set() + if "SLACK_WEBHOOK" in names: + findings.append( + f"{relative} job {job!r} passes `SLACK_WEBHOOK` to {body['uses']!r}, which " + f"takes `SLACK_BOT_TOKEN` instead: pass " + f"`SLACK_BOT_TOKEN: ${{{{ secrets.SLACK_BOT_TOKEN }}}}`" + ) + elif "SLACK_BOT_TOKEN" not in names: + findings.append( + f"{relative} job {job!r} calls {body['uses']!r} without passing " + f"`SLACK_BOT_TOKEN` under `secrets:`, so every post fails: pass " + f"`SLACK_BOT_TOKEN: ${{{{ secrets.SLACK_BOT_TOKEN }}}}`" + ) return findings -def check_slack_posts_name_a_channel(root, tracked, files): - """Every call to a shared Slack workflow names the channel it posts to. +def check_slack_calls_match_v2(root, tracked, files): + """Every call to a shared Slack workflow names its channel and passes the bot token. - Both workflows post through one bot token to the channel their caller names, and the input - is required, so a caller written for the webhook the token replaced stops running when it - moves to the version that requires it. Its watcher only fires from the default branch, so + Both workflows post through one bot token to the channel their caller names, so a caller + written for the webhook the token replaced - no channel, `SLACK_WEBHOOK` - stops posting + when it moves to the version that requires them. Its watcher only fires from the default branch, so nothing else would say so before the move is merged. Args: @@ -887,7 +905,7 @@ def check_the_detail_layer_routing(root, tracked, files): ("no internal references in a public repository", check_no_internal_references), ("every CI job is bounded", check_ci_jobs_are_bounded), ("failed runs are reported", check_failed_runs_are_reported), - ("Slack posts name a channel", check_slack_posts_name_a_channel), + ("Slack calls name a channel and pass the token", check_slack_calls_match_v2), ("the instruction layer is intact", check_the_instruction_layer), ("the detail layer and the router agree", check_the_detail_layer_routing), ) diff --git a/tests/test_slack_workflows.py b/tests/test_slack_workflows.py index 5a5a6ee..a56d9f7 100644 --- a/tests/test_slack_workflows.py +++ b/tests/test_slack_workflows.py @@ -3,7 +3,7 @@ Each step's script is read out of its workflow file and run under bash with the environment GitHub would give it, against stand-ins for `curl` and `gh` on PATH. So what is tested is the script that ships, not a copy of it. What this cannot test: the GitHub expressions that fill the -environment, and Slack itself, which is exercised by the first announcement after merge. +environment, and Slack itself. """ import importlib.util @@ -160,36 +160,6 @@ def test_missing_configuration_fails_naming_it(stubs, overrides, message): assert message in result.stderr -@pytest.mark.parametrize("error,hint", [ - ("not_in_channel", "Invite the Slack app to #l337-org"), - ("channel_not_found", "Check the channel name"), - ("invalid_auth", "Replace SLACK_BOT_TOKEN"), - ("missing_scope", "chat:write"), - ("something_new", ""), -]) -def test_a_refusal_fails_with_slacks_error(stubs, error, hint): - result, _ = run(ANNOUNCER, stubs, pull_request(), body=json.dumps({"ok": False, "error": error})) - assert result.returncode == 1 - assert f"HTTP 200, {error}." in result.stderr and hint in result.stderr - - -def test_a_non_slack_answer_is_quoted_and_marked_truncated(stubs): - result, _ = run(ANNOUNCER, stubs, pull_request(), status="502", body="" + "x" * 1000) - assert result.returncode == 1 - assert "HTTP 502, " in result.stderr and "(truncated at 500 bytes)" in result.stderr - - -def test_a_long_acceptance_is_still_read_as_accepted(stubs): - body = json.dumps({"ok": True, "message": {"text": "x" * 5000}}) - result, _ = run(ANNOUNCER, stubs, pull_request(), body=body) - assert result.returncode == 0, result.stderr - - -def test_curl_failing_outright_fails(stubs): - result, _ = run(ANNOUNCER, stubs, pull_request(), curl_exit=28) - assert result.returncode == 1 and "curl could not complete" in result.stderr - - REPORTER = "slack-on-failure.yaml" @@ -237,10 +207,55 @@ def test_the_reporter_fails_on_missing_configuration(stubs, overrides, message): assert message in result.stderr -def test_the_reporter_fails_with_slacks_error(stubs): - result, _ = run(REPORTER, stubs, failed_run(), body='{"ok": false, "error": "not_in_channel"}') +# The posting and refusal handling is the same code in both workflows, each a copy of the other, +# so every case runs against both and a copy that drifts fails here. +BOTH = pytest.mark.parametrize("workflow,environment,channel", [ + (ANNOUNCER, pull_request, "#l337-org"), + (REPORTER, failed_run, "#docker-mcp"), +]) + + +@BOTH +@pytest.mark.parametrize("error,hint", [ + ("not_in_channel", "Invite the Slack app to {channel}"), + ("channel_not_found", "Check the channel name"), + ("invalid_auth", "Replace SLACK_BOT_TOKEN"), + ("token_revoked", "Replace SLACK_BOT_TOKEN"), + ("missing_scope", "chat:write"), + ("something_new", ""), +]) +def test_a_refusal_fails_with_slacks_error(stubs, workflow, environment, channel, error, hint): + result, _ = run(workflow, stubs, environment(), body=json.dumps({"ok": False, "error": error})) assert result.returncode == 1 - assert "not_in_channel" in result.stderr and "Invite the Slack app to #docker-mcp" in result.stderr + assert f"HTTP 200, {error}." in result.stderr and hint.format(channel=channel) in result.stderr + + +@BOTH +def test_a_non_slack_answer_is_quoted_and_marked_truncated(stubs, workflow, environment, channel): + result, _ = run(workflow, stubs, environment(), status="502", body="" + "x" * 1000) + assert result.returncode == 1 + assert "HTTP 502, " in result.stderr and "(truncated at 500 bytes)" in result.stderr + + +@BOTH +def test_a_long_acceptance_is_still_read_as_accepted(stubs, workflow, environment, channel): + body = json.dumps({"ok": True, "message": {"text": "x" * 5000}}) + result, _ = run(workflow, stubs, environment(), body=body) + assert result.returncode == 0, result.stderr + + +@BOTH +def test_curl_failing_outright_fails(stubs, workflow, environment, channel): + result, _ = run(workflow, stubs, environment(), curl_exit=28) + assert result.returncode == 1 and "curl could not complete" in result.stderr + + +@BOTH +def test_the_token_reaches_curl_only_in_a_header_file(stubs, workflow, environment, channel): + run(workflow, stubs, environment()) + assert (stubs / "curl.headers").read_text().startswith(f"Authorization: Bearer {TOKEN}") + assert TOKEN not in (stubs / "curl.args").read_text() + assert json.loads((stubs / "curl.stdin").read_text())["channel"] == channel def hygiene(): @@ -257,18 +272,27 @@ def hygiene(): "L337-org/github-workflows/.github/workflows/slack-on-failure.yaml@" + "0" * 40, "./.github/workflows/slack-on-failure.yaml", ]) -@pytest.mark.parametrize("given,found", [ - ({"channel": "#quaffed"}, False), - ({"channel": " "}, True), - ({}, True), - (None, True), +@pytest.mark.parametrize("given,secrets,found", [ + ({"channel": "#quaffed"}, {"SLACK_BOT_TOKEN": "${{ secrets.SLACK_BOT_TOKEN }}"}, []), + ({"channel": "#quaffed"}, "inherit", []), + ({"channel": " "}, {"SLACK_BOT_TOKEN": "x"}, ["without a `with: channel:`"]), + ({}, {"SLACK_BOT_TOKEN": "x"}, ["without a `with: channel:`"]), + (None, {"SLACK_BOT_TOKEN": "x"}, ["without a `with: channel:`"]), + ({"channel": "#quaffed"}, {"SLACK_WEBHOOK": "x"}, ["passes `SLACK_WEBHOOK`"]), + ({"channel": "#quaffed"}, {"SLACK_WEBHOOK": "x", "SLACK_BOT_TOKEN": "x"}, ["passes `SLACK_WEBHOOK`"]), + ({"channel": "#quaffed"}, None, ["without passing `SLACK_BOT_TOKEN`"]), + (None, {"SLACK_WEBHOOK": "x"}, ["without a `with: channel:`", "passes `SLACK_WEBHOOK`"]), ]) -def test_hygiene_requires_a_channel_on_every_slack_call(uses, given, found): +def test_hygiene_requires_v2_inputs_on_every_slack_call(uses, given, secrets, found): body = {"uses": uses} if given is not None: body["with"] = given + if secrets is not None: + body["secrets"] = secrets findings = hygiene().slack_channel_findings([(".github/workflows/x.yaml", {"jobs": {"slack": body}})]) - assert bool(findings) is found + assert len(findings) == len(found) + for finding, expected in zip(findings, found): + assert expected in finding def test_every_shipped_caller_here_names_a_channel(): From d18c20f290f2e6eab06fc563ae25ab38f898c9a7 Mon Sep 17 00:00:00 2001 From: CL Date: Thu, 15 Oct 2026 15:25:04 +0100 Subject: [PATCH 3/3] Name the Slack call check for its rule, and rewrap two spliced lines check_slack_calls_match_v2 and slack_channel_findings named a version and half the rule; the check now requires a channel and the token, so the names say that. Two comment lines edited in the previous commit had new wording spliced onto old lines past the 100-column wrap. --- actions/action-pins/action.yml | 5 +++-- actions/repo-hygiene/check-repo-hygiene.py | 14 +++++++------- tests/test_slack_workflows.py | 6 +++--- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/actions/action-pins/action.yml b/actions/action-pins/action.yml index d8e2f68..9eb901c 100644 --- a/actions/action-pins/action.yml +++ b/actions/action-pins/action.yml @@ -11,8 +11,9 @@ description: 'Fail when any `uses:` in the checked-out repository names somethin # The one exception is L337-org/github-workflows at a version tag, such as `@v2` (DS.6.2). It # holds only while a ruleset there blocks creating, moving and deleting `v*` tags with a bypass # list no wider than its main branch's, so that moving one takes the same people as merging; a -# breaking change goes to a new major tag while callers stay on the old one. It is named exactly rather than as the organisation, -# because the exemption rests on that ruleset, which another repository may not have. +# breaking change goes to a new major tag while callers stay on the old one. It is named +# exactly rather than as the organisation, because the exemption rests on that ruleset, which +# another repository may not have. # # Quoted and upper-case SHAs are accepted because they are equally pinned, and the trailing # version comment is not enforced (DS.6.3): a guard that fails a legitimate pin for a cosmetic diff --git a/actions/repo-hygiene/check-repo-hygiene.py b/actions/repo-hygiene/check-repo-hygiene.py index f9da2e7..0fb50aa 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -636,8 +636,8 @@ def check_failed_runs_are_reported(root, tracked, files): return reported_run_findings(workflow_documents(root, tracked)) -def slack_channel_findings(documents): - """The findings of check_slack_calls_match_v2, over parsed workflows. +def slack_call_findings(documents): + """The findings of check_slack_calls_take_channel_and_token, over parsed workflows. Args: documents (list): `(repo-relative path, document)` pairs, as workflow_documents returns. @@ -679,13 +679,13 @@ def slack_channel_findings(documents): return findings -def check_slack_calls_match_v2(root, tracked, files): +def check_slack_calls_take_channel_and_token(root, tracked, files): """Every call to a shared Slack workflow names its channel and passes the bot token. Both workflows post through one bot token to the channel their caller names, so a caller written for the webhook the token replaced - no channel, `SLACK_WEBHOOK` - stops posting - when it moves to the version that requires them. Its watcher only fires from the default branch, so - nothing else would say so before the move is merged. + when it moves to the version that requires them. Its watcher only fires from the default + branch, so nothing else would say so before the move is merged. Args: root (pathlib.Path): repository root. @@ -698,7 +698,7 @@ def check_slack_calls_match_v2(root, tracked, files): Raises: CannotEvaluate: A workflow could not be read or parsed, via workflow_documents. """ - return slack_channel_findings(workflow_documents(root, tracked)) + return slack_call_findings(workflow_documents(root, tracked)) # Markdown links plus backtick-quoted paths, since a router routes both ways. @@ -905,7 +905,7 @@ def check_the_detail_layer_routing(root, tracked, files): ("no internal references in a public repository", check_no_internal_references), ("every CI job is bounded", check_ci_jobs_are_bounded), ("failed runs are reported", check_failed_runs_are_reported), - ("Slack calls name a channel and pass the token", check_slack_calls_match_v2), + ("Slack calls name a channel and pass the token", check_slack_calls_take_channel_and_token), ("the instruction layer is intact", check_the_instruction_layer), ("the detail layer and the router agree", check_the_detail_layer_routing), ) diff --git a/tests/test_slack_workflows.py b/tests/test_slack_workflows.py index a56d9f7..2649d7b 100644 --- a/tests/test_slack_workflows.py +++ b/tests/test_slack_workflows.py @@ -289,14 +289,14 @@ def test_hygiene_requires_v2_inputs_on_every_slack_call(uses, given, secrets, fo body["with"] = given if secrets is not None: body["secrets"] = secrets - findings = hygiene().slack_channel_findings([(".github/workflows/x.yaml", {"jobs": {"slack": body}})]) + findings = hygiene().slack_call_findings([(".github/workflows/x.yaml", {"jobs": {"slack": body}})]) assert len(findings) == len(found) for finding, expected in zip(findings, found): assert expected in finding -def test_every_shipped_caller_here_names_a_channel(): +def test_every_shipped_caller_here_names_a_channel_and_passes_the_token(): documents = [ (p.name, yaml.safe_load(p.read_text())) for p in sorted(WORKFLOWS.glob("*.yaml")) ] - assert hygiene().slack_channel_findings(documents) == [] + assert hygiene().slack_call_findings(documents) == []