From 37e31ffd85ae182819e53799cd68d62b45749d8f Mon Sep 17 00:00:00 2001 From: clad2 Date: Wed, 5 Aug 2026 17:16:30 +0000 Subject: [PATCH] fix(labels): refuse an unset API root, name transport failures, update the docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three corrections from @codex-reviewer-andresmgsl's review of 935a813. 1. `api="${GITHUB_API_URL:-https://api.github.com}"` guessed GitHub when the variable was absent — driven with a recording curl, it reported success after POSTing to api.github.com from this forge. That is the "Never 'probably github'" rule, and the same unset-environment refusal #201 just established for docs-sync. It now refuses before any request, and the test asserts zero calls were made: refusing after a POST is not refusing. 2. The "never silenced with || true" invariant was still asserted by grepping the gh line this port removed, so it passed on any REST implementation including one that swallows a failed POST. It is rebound behaviourally: a curl that dies at the transport must fail the step. Doing that revealed the step failed with a bare exit 7 and no sentence, so it now names the failure — owning the diagnostic is the whole point of the surrounding code. 3. docs/CONSUMERS.md and both caller comments still described `gh workflow run` as the mechanism. They describe the REST dispatch now, and the manual bootstrap command carries a forge-neutral curl form beside the gh one: a cross-forge runbook that sends this forge to a missing binary is wrong even where the prose around it is right. Refs #205 --- .github/workflows/labels-sweep.yml | 2 +- .github/workflows/labels.yml | 28 ++++++++++++++++++--- .github/workflows/self-labels.yml | 2 +- changelog.d/205.md | 14 +++++++++++ docs/CONSUMERS.md | 29 +++++++++++++++++----- test/labels-dispatch.test.sh | 40 ++++++++++++++++++++++++++++++ 6 files changed, 103 insertions(+), 12 deletions(-) diff --git a/.github/workflows/labels-sweep.yml b/.github/workflows/labels-sweep.yml index 707b871..defca41 100644 --- a/.github/workflows/labels-sweep.yml +++ b/.github/workflows/labels-sweep.yml @@ -87,7 +87,7 @@ jobs: # equivalent of. # # bootstrap: every trigger-driven wake arrives as workflow_dispatch - # too (that is how `gh workflow run` wakes the caller), so the event + # too (that is how the trigger's dispatch wakes the caller), so the event # name alone no longer separates the operator's manual full-board # bootstrap from an event-woken sweep — the caller's `bootstrap` # dispatch input does: the trigger passes "no", a bare manual diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 6c24bbe..8cdf02c 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -143,7 +143,18 @@ jobs: # STILL LOUD on failure, per this job's contract: a consumer missing # the sweep caller, its `bootstrap` input, or `actions: write` must # fail HERE and visibly, not sweep silently never again. - api="${GITHUB_API_URL:-https://api.github.com}" + # NEVER "probably github" (lib/forge.sh). Defaulting an unset + # GITHUB_API_URL to api.github.com would send this forge's dispatch + # to GitHub and report success — the same unset-environment guess + # #201 just refused for docs-sync. The API root is injected by the + # forge running us; if it is absent we do not know where we are, and + # a guess is worse than a red trigger + # (@codex-reviewer-andresmgsl, #205 review). + api="${GITHUB_API_URL:-}" + if [ -z "$api" ]; then + echo "::error::labels: the sweep was NOT woken — GITHUB_API_URL is unset, so the forge's API root is unknown. Refusing to guess a forge." + exit 1 + fi # `gh workflow run` defaulted the ref to the repository's default # branch; REST has no default and 400s without one. Prefer the event @@ -161,12 +172,21 @@ jobs: fi out="$(mktemp)" - trap 'rm -f "$out"' EXIT - code="$(curl -sS -o "$out" -w '%{http_code}' -X POST \ + err="$(mktemp)" + trap 'rm -f "$out" "$err"' EXIT + # A transport failure is named, not merely propagated. Letting `set + # -e` carry curl's own exit code out of the assignment DID fail the + # job — the invariant holds — but it failed with a bare status and no + # sentence, which is the opposite of this step owning its diagnostic. + if ! code="$(curl -sS -o "$out" -w '%{http_code}' -X POST \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -H 'Content-Type: application/json' \ -d "$(jq -nc --arg ref "$branch" '{ref: $ref, inputs: {bootstrap: "no"}}')" \ - "$api/repos/$GITHUB_REPOSITORY/actions/workflows/$SWEEP_WORKFLOW/dispatches")" + "$api/repos/$GITHUB_REPOSITORY/actions/workflows/$SWEEP_WORKFLOW/dispatches" \ + 2>"$err")"; then + echo "::error::labels: the sweep was NOT woken — the request to $api never completed: $(tr -d '\n' <"$err")" + exit 1 + fi if [ "$code" != "204" ]; then # Own the diagnostic rather than pass the status through. This diff --git a/.github/workflows/self-labels.yml b/.github/workflows/self-labels.yml index 715f1ee..8b6b8c1 100644 --- a/.github/workflows/self-labels.yml +++ b/.github/workflows/self-labels.yml @@ -39,7 +39,7 @@ permissions: contents: read checks: read # mergeability/check-rollup read for PR state statuses: read # commit-status rollup read for PR state - actions: write # the trigger job's `gh workflow run` dispatch of the sweep caller (#209) + actions: write # the trigger job's dispatch of the sweep caller (#209, #205) issues: write pull-requests: write jobs: diff --git a/changelog.d/205.md b/changelog.d/205.md index 42c608c..50ed6f6 100644 --- a/changelog.d/205.md +++ b/changelog.d/205.md @@ -28,3 +28,17 @@ - That test also drives the failure path: any non-204 still fails the job, so the misconfiguration alarm the trigger exists to be cannot decay into a warning (#205). + +- An unset `GITHUB_API_URL` refuses before any request instead of defaulting + to `api.github.com`. Guessing sent this forge's dispatch to GitHub and + reported success — the "Never 'probably github'" rule, and the same + unset-environment refusal #201 established for docs-sync (#205). + +- A dispatch that never reaches the forge names the failure. Letting `set -e` + carry curl's exit code out did fail the job, but with a bare status and no + sentence (#205). + +- `docs/CONSUMERS.md` and both caller comments describe the REST dispatch, and + the manual bootstrap command carries a forge-neutral form beside the `gh` + one — a cross-forge runbook that directs this forge to a missing binary is + wrong even where the surrounding prose is right (#205). diff --git a/docs/CONSUMERS.md b/docs/CONSUMERS.md index 0454f32..e5a3d59 100644 --- a/docs/CONSUMERS.md +++ b/docs/CONSUMERS.md @@ -339,7 +339,9 @@ together at the same pin: - **`labels.yml`** — the event-facing half, called on PR and issue events. Two jobs: additive path-based `scope:*` labels, and a few-seconds `trigger` job that wakes the sweep by dispatching the consumer's sweep - caller (`gh workflow run`, plain `GITHUB_TOKEN` — `workflow_dispatch` is + caller (a REST `POST` to the forge's own + `${GITHUB_API_URL}/repos/{owner}/{repo}/actions/workflows/{file}/dispatches`, + plain `GITHUB_TOKEN` — `workflow_dispatch` is one of the two documented exemptions from the token's no-retrigger rule, so no PAT anywhere in the path and no loop: the sweep dispatches nothing). @@ -404,7 +406,7 @@ permissions: contents: read checks: read # mergeability/check-rollup read for PR state statuses: read # commit-status rollup read for PR state - actions: write # the trigger job's `gh workflow run` dispatch of the sweep caller (#209) + actions: write # the trigger job's dispatch of the sweep caller (#209, #205) issues: write pull-requests: write jobs: @@ -468,8 +470,8 @@ repositories allow check data to be read regardless, but a private consumer needs the explicit reads above; without them the failure appears as an empty `state:*` axis on the board rather than a red workflow run. The labels caller's `actions: write` is different — it is required everywhere, public -repos included: the trigger job's `gh workflow run` is a write, and without -it every event run goes red at the trigger. +repos included: the trigger job's dispatch is a write, and without it every +event run goes red at the trigger. **The failure mode to know before bumping**: a consumer that bumps its pin to a #209-carrying tag without adding the sweep caller keeps green-looking @@ -526,8 +528,8 @@ carrying the split: fires both callers into the one shared `labels-reconcile` group — so displacement goes **up**, and the fix reads as the bug getting worse. 4. **`actions: write` on the labels caller** — consumers carry - `actions: read` today (crew does); the trigger job's `gh workflow run` - is a write. The sweep caller keeps `actions: read`. + `actions: read` today (crew does); the trigger job's dispatch is a + write. The sweep caller keeps `actions: read`. Bump without the sweep caller and the trigger job goes red on every PR and issue event — the loud failure mode above — so never split these @@ -596,10 +598,25 @@ to bootstrap labels on a fresh repository. A bare dispatch is also the operator's general manual full-board sweep — the answer when the board looks wrong now rather than after the next scheduled cadence: +On GitHub, with the `gh` CLI: + ```sh gh workflow run labels-sweep.yml -R / ``` +On any forge — including Forgejo, whose runners carry no `gh` — the same +dispatch over REST, which is what the trigger job itself sends (#205): + +```sh +curl -sS -X POST \ + -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \ + -d '{"ref":"main","inputs":{"bootstrap":"yes"}}' \ + "$API/repos///actions/workflows/labels-sweep.yml/dispatches" +``` + +`$API` is the forge's API root — `https://api.github.com` on GitHub, +`/api/v1` on Forgejo — and success is `204` with an empty body. + Ceremony dogfoods the callers under the filenames `self-labels.yml` and `self-labels-sweep.yml`, so the equivalent command in this repository substitutes that filename. Scheduled and trigger-driven runs only diff --git a/test/labels-dispatch.test.sh b/test/labels-dispatch.test.sh index 376ca94..90a1e6c 100755 --- a/test/labels-dispatch.test.sh +++ b/test/labels-dispatch.test.sh @@ -129,6 +129,46 @@ check "...and naming the consumer causes the alarm exists for" 1 "actions: write check "any non-204 fails, not only the statuses the API documents" 1 "forbidden" \ run_step 403 '{"message":"forbidden"}' DEFAULT_BRANCH=main +# --- the API root is not guessed --------------------------------------------- +# Defaulting an unset GITHUB_API_URL to api.github.com sent this forge's +# dispatch to GitHub and reported success (@codex-reviewer-andresmgsl). The +# teeth are the call count: refusing AFTER a request is not refusing. +run_step_no_api() { + rm -rf "${TMP:?}/bin"; mkdir -p "$TMP/bin"; : >"$TMP/calls" + make_curl 204 "" + env PATH="$TMP/bin:$PATH" GITHUB_TOKEN=tok GITHUB_REPOSITORY=owner/repo \ + SWEEP_WORKFLOW=self-labels-sweep.yml DEFAULT_BRANCH=main "$STEP" +} +check "an unset GITHUB_API_URL refuses rather than guessing GitHub" 1 \ + "GITHUB_API_URL is unset" run_step_no_api +no_calls_made() { [ ! -s "$TMP/calls" ]; } +check "...having made zero requests: refusing after a POST is not refusing" 0 "" no_calls_made + +# --- a transport failure is not silence -------------------------------------- +# The old `|| true` invariant was asserted by grepping the gh line this port +# removed, so it passed on any REST implementation including one that swallows +# a failed POST (@codex-reviewer-andresmgsl). Driven instead: curl itself exits +# non-zero, which `-w` cannot report because nothing is written. +make_failing_curl() { + printf '%s\n' '#!/usr/bin/env bash' 'echo "curl: (7) failed to connect" >&2' 'exit 7' \ + >"$TMP/bin/curl" + chmod +x "$TMP/bin/curl" +} +run_step_curl_dies() { + rm -rf "${TMP:?}/bin"; mkdir -p "$TMP/bin"; : >"$TMP/calls" + make_failing_curl + env PATH="$TMP/bin:$PATH" GITHUB_TOKEN=tok \ + GITHUB_API_URL=https://forge.example/api/v1 GITHUB_REPOSITORY=owner/repo \ + SWEEP_WORKFLOW=self-labels-sweep.yml DEFAULT_BRANCH=main "$STEP" +} +check "a POST that never reaches the forge fails the step, and says so" 1 \ + "never completed" run_step_curl_dies + +# A code-aware guard alongside the behavioural one: no swallowing operator on +# the dispatch itself. +never_silenced() { sed 's/#.*//' "$STEP" | grep -qE '\|\|[[:space:]]*true'; } +check "...and the step carries no || true" 1 "" never_silenced + # --- what the port removed --------------------------------------------------- # Strip comments first: the step's prose NAMES `gh workflow run` and # CEREMONY_FORGE_CLIENT to explain what it replaced, so a raw grep asserts on