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..515593f 100644 --- a/.github/workflows/premerge.yaml +++ b/.github/workflows/premerge.yaml @@ -47,3 +47,23 @@ 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' + + 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: Shared workflows and actions 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..bf83a57 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,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 secrets to a run started by +- **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. + 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 @@ -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,30 +148,94 @@ 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. -- **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 - 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 +- **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 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 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.** 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. + +## 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 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 -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). 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: + + ```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 -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. +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..9eb901c 100644 --- a/actions/action-pins/action.yml +++ b/actions/action-pins/action.yml @@ -1,13 +1,20 @@ 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). 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 # 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 +27,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 +45,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..0fb50aa 100755 --- a/actions/repo-hygiene/check-repo-hygiene.py +++ b/actions/repo-hygiene/check-repo-hygiene.py @@ -6,13 +6,14 @@ """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 +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 @@ -116,6 +117,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 +636,71 @@ def check_failed_runs_are_reported(root, tracked, files): return reported_run_findings(workflow_documents(root, tracked)) +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. + + 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'" + ) + # `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_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. + + 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_call_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 +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_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_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..2649d7b --- /dev/null +++ b/tests/test_slack_workflows.py @@ -0,0 +1,302 @@ +"""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. +""" + +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 + + +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 + + +# 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 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(): + 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,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_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_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_and_passes_the_token(): + documents = [ + (p.name, yaml.safe_load(p.read_text())) for p in sorted(WORKFLOWS.glob("*.yaml")) + ] + assert hygiene().slack_call_findings(documents) == []