diff --git a/changelog.d/188.md b/changelog.d/188.md index e6357dd..a3770d4 100644 --- a/changelog.d/188.md +++ b/changelog.d/188.md @@ -10,6 +10,12 @@ - `lib/closes_references.sh` — the closing-keyword parser, sibling of `refs_references`, so "which issues does this PR close" is answered from a PR body rather than from GitHub's GraphQL API (#188). +- `lib/forge-github.sh` and `lib/forge-forgejo.sh` — one call surface, two + backends, selected by `forge_select`; no forge branching at the call sites + (#188). +- The forgejo backend proves each paginated gather complete against the + server's `x-total-count` and refuses loudly when it cannot — a missing + header is a refusal, not a pass (#188). ### Changed @@ -18,6 +24,10 @@ replaced rather than translated; both forges return `number` and `body` from `/pulls` in the same shape (#188). +- `forge_api` owns the page size, because each forge silently ignores the + other's parameter: `per_page=100` reads 30 items on Forgejo and `limit=100` + reads 30 on GitHub, both HTTP 200. No call site names one (#188). + ### Fixed - `labels-reconcile` and `labels-scope` no longer exit 0 on a Forgejo diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh new file mode 100644 index 0000000..1756943 --- /dev/null +++ b/lib/forge-forgejo.sh @@ -0,0 +1,177 @@ +#!/usr/bin/env bash +# lib/forge-forgejo.sh — the Forgejo backend: /api/v1 over curl + jq +# (issue #188, term 1). Sourced by lib/forge.sh when forge_detect says +# forgejo; never sourced directly, and never at the same time as the github +# backend — they define the same verbs on purpose. +# +# curl+jq rather than a CLI because that is what the runner has. The image +# this instance runs jobs in (ghcr.io/catthehacker/ubuntu:act-22.04, probe +# task 278) carries curl, jq and node, and has neither `gh` nor `stoke`. + +# forgejo_api_base — the /api/v1 root, from the runner's own environment. +# GITHUB_API_URL already IS the /api/v1 root on a Forgejo runner (measured: +# https://forgejo.heavyduty.builders/api/v1). CEREMONY_FORGE_API overrides +# it for tests and for anyone driving this outside Actions. +forgejo_api_base() { + local base="${CEREMONY_FORGE_API:-${GITHUB_API_URL:-}}" + if [ -z "$base" ]; then + echo "forgejo_api_base: no GITHUB_API_URL or CEREMONY_FORGE_API — cannot reach the forge (#188)" >&2 + return 1 + fi + printf '%s\n' "${base%/}" +} + +# forgejo_page_url — pure, so the page-size contract is +# testable without a network. Returns the endpoint with this backend's OWN +# paging parameters applied. +# +# THE TRAP THIS EXISTS TO REMOVE, measured 2026-08-02 against +# heavy-duty/rig (137 issues and PRs) and heavy-duty/ceremony on GitHub: +# +# ?per_page=100 GitHub: 100 items Forgejo: 30 items (IGNORED) +# ?limit=100 GitHub: 30 items Forgejo: 50 items (capped) +# +# Each forge silently ignores the other's page-size parameter, answers +# HTTP 200 with valid JSON, and says nothing. Every call site in this repo +# was written GitHub-shaped, so a verbatim port would have swept 30 of +# rig's 137 and printed "reconciled." — acceptance criterion 2 failing +# green, and the same "degraded read that does not report it degraded" +# failure class this whole issue exists to kill. +# +# So NO CALL SITE NAMES A PAGE SIZE. The backend owns it. Fixing the +# boundary once beats fixing nine call sites and trusting the tenth — the +# same argument that chose shape C over B, one level down. +# +# 50 is not a preference: Forgejo caps a page at MAX_RESPONSE_ITEMS (50 on +# this instance) whatever you ask for, so asking for more cannot help and +# pagination is mandatory rather than an optimisation. +forgejo_page_url() { + local endpoint="${1:?forgejo_page_url: endpoint required}" page="${2:?forgejo_page_url: page required}" + # Strip any page-size parameter a caller left behind, in either dialect, + # rather than trusting that none did: this function is the one place that + # decides paging, and a stray per_page= would be exactly the silent + # truncation above. + local clean="$endpoint" + clean="$(printf '%s' "$clean" | sed -E 's/([?&])(per_page|limit|page)=[0-9]+/\1/g; s/[?&]+$//; s/([?&])&+/\1/g')" + case "$clean" in + *\?) printf '%slimit=50&page=%s\n' "$clean" "$page" ;; + *\?*) printf '%s&limit=50&page=%s\n' "$clean" "$page" ;; + *) printf '%s?limit=50&page=%s\n' "$clean" "$page" ;; + esac +} + +# forge_api [--paginate] [--jq ] +# +# --paginate walks page= until a short page, then PROVES the walk was +# complete by comparing what it collected against the server's declared +# x-total-count. @kimi-reviewer-andresmgsl's hardening (#4699): a MISSING +# header is a loud refusal, not a pass. Header exposure is a server setting +# (access-control-expose-headers), and an instance that withholds it would +# make the completeness check compare null to a number — the guard itself +# degrading silently, which is the failure class re-entering through the +# door built to stop it. +forge_api() { + local paginate=false endpoint="" jqexpr="" have_jq=false + while [ $# -gt 0 ]; do + case "$1" in + --paginate) paginate=true ;; + --jq) jqexpr="$2"; have_jq=true; shift ;; + -*) ;; + *) [ -n "$endpoint" ] || endpoint="$1" ;; + esac + shift + done + [ -n "$endpoint" ] || { echo "forge_api: endpoint required" >&2; return 1; } + + local base token + base="$(forgejo_api_base)" || return 1 + token="${GH_TOKEN:-${GITHUB_TOKEN:-${FORGEJO_TOKEN:-}}}" + + local hdr body + hdr="$(mktemp)"; body="$(mktemp)" + # shellcheck disable=SC2064 # the paths are fixed at trap time on purpose + trap "rm -f '$hdr' '$body'" RETURN + + if [ "$paginate" = false ]; then + if ! curl -sS -D "$hdr" -o "$body" \ + -H "Authorization: token $token" -H 'Accept: application/json' \ + "$base/$endpoint"; then + echo "forge_api: request failed: $endpoint" >&2 + return 1 + fi + forgejo_http_ok "$hdr" "$endpoint" || return 1 + if [ "$have_jq" = true ]; then jq -r "$jqexpr" <"$body"; else cat "$body"; fi + return 0 + fi + + # Paginated: accumulate into ONE array and apply --jq once at the end. + # gh --paginate applies --jq per page and concatenates; for the `.[] | …` + # shapes every call site here uses, the two are identical, and merging + # first is what makes the completeness assert possible at all. + local page=1 total="" got=0 n all="[]" pagejson + while :; do + if ! curl -sS -D "$hdr" -o "$body" \ + -H "Authorization: token $token" -H 'Accept: application/json' \ + "$base/$(forgejo_page_url "$endpoint" "$page")"; then + echo "forge_api: request failed: $endpoint (page $page)" >&2 + return 1 + fi + forgejo_http_ok "$hdr" "$endpoint" || return 1 + + if [ -z "$total" ]; then + total="$(forgejo_total_count "$hdr")" || return 1 + fi + pagejson="$(cat "$body")" + n="$(jq 'if type == "array" then length else 0 end' <<<"$pagejson")" + [ "$n" -gt 0 ] || break + all="$(jq -s '.[0] + .[1]' <<<"$all"$'\n'"$pagejson")" + got=$((got + n)) + [ "$got" -lt "$total" ] || break + page=$((page + 1)) + done + + # The assert. A short read here is the silent-truncation bug arriving by + # another route, so it is fatal rather than a warning. + if [ "$got" -ne "$total" ]; then + cat >&2 < — the declared size of the collection. +# Absent is fatal (#4699): without it the completeness assert cannot run, +# and an assert that cannot run must not silently pass. +forgejo_total_count() { + local hdr="$1" total + total="$(tr -d '\r' <"$hdr" | awk 'tolower($1) == "x-total-count:" { print $2 }' | tail -n1)" + if [ -z "$total" ]; then + cat >&2 < — a non-2xx is named, not +# swallowed. gh exits non-zero on HTTP failure; curl does not without -f, +# and -f would throw away the body that says why. +forgejo_http_ok() { + local hdr="$1" endpoint="$2" code + code="$(tr -d '\r' <"$hdr" | awk '/^HTTP\// { c = $2 } END { print c }')" + case "$code" in + 2*) return 0 ;; + *) + echo "forge_api: HTTP $code from '$endpoint'" >&2 + return 1 + ;; + esac +} diff --git a/lib/forge-github.sh b/lib/forge-github.sh new file mode 100644 index 0000000..82230bd --- /dev/null +++ b/lib/forge-github.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# lib/forge-github.sh — the GitHub backend (issue #188, term 1). Sourced by +# lib/forge.sh when forge_detect says github; never at the same time as the +# forgejo backend — they define the same verbs on purpose. +# +# This file is the CURRENT call set, extracted 1:1 and nothing more. Term 5 +# of the frozen Spec is "GitHub consumers are unchanged", and the cheapest +# way to keep that true is for every verb here to be a thin pass-through to +# the `gh` invocation the call site used before the port. No behaviour is +# added, fixed or tidied on this path; anything that looks like an +# improvement here is a regression risk against a forge nobody is currently +# reporting bugs on. + +# forge_api [--paginate] [--jq ] +# +# The one deliberate difference from a pure pass-through: the caller no +# longer names a page size, because the page-size parameter is not portable +# and is therefore the backend's to own (#188). +# +# ?per_page=100 GitHub: 100 items Forgejo: 30 items (IGNORED) +# ?limit=100 GitHub: 30 items Forgejo: 50 items (capped) +# +# Both answer HTTP 200 either way, so a call site that names one is a silent +# truncation waiting for the other forge. per_page=100 is injected here — +# exactly what the call sites said before — so the GitHub path is unchanged +# in behaviour while the parameter stops being a call-site concern. +forge_api() { + local paginate=false endpoint="" jqexpr="" have_jq=false + while [ $# -gt 0 ]; do + case "$1" in + --paginate) paginate=true ;; + --jq) jqexpr="$2"; have_jq=true; shift ;; + -*) ;; + *) [ -n "$endpoint" ] || endpoint="$1" ;; + esac + shift + done + [ -n "$endpoint" ] || { echo "forge_api: endpoint required" >&2; return 1; } + + if [ "$paginate" = true ]; then + endpoint="$(github_page_url "$endpoint")" + if [ "$have_jq" = true ]; then + gh api --paginate "$endpoint" --jq "$jqexpr" + else + gh api --paginate "$endpoint" + fi + else + if [ "$have_jq" = true ]; then + gh api "$endpoint" --jq "$jqexpr" + else + gh api "$endpoint" + fi + fi +} + +# github_page_url — pure, so the page-size contract is testable +# without a network. Strips any page-size parameter a caller left behind in +# either dialect, then applies GitHub's own. +github_page_url() { + local endpoint="${1:?github_page_url: endpoint required}" clean + clean="$(printf '%s' "$endpoint" | sed -E 's/([?&])(per_page|limit|page)=[0-9]+/\1/g; s/[?&]+$//; s/([?&])&+/\1/g')" + case "$clean" in + *\?) printf '%sper_page=100\n' "$clean" ;; + *\?*) printf '%s&per_page=100\n' "$clean" ;; + *) printf '%s?per_page=100\n' "$clean" ;; + esac +} diff --git a/lib/forge.sh b/lib/forge.sh index 7b6e03c..bcf1c26 100644 --- a/lib/forge.sh +++ b/lib/forge.sh @@ -111,6 +111,39 @@ EOF return 1 } +# Where the backends live. Captured at source time, not call time: a +# function that resolves BASH_SOURCE later would resolve its own file, not +# this one. +FORGE_LIB_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# forge_select [forge] — source the backend for this forge, so the forge_* +# verbs exist. Exactly one backend is ever loaded; both define the same +# names, which is what keeps the branching out of the 61 call sites (term 1) +# and the single-forge assumption from growing back. +# +# Idempotent, because the actions call it once and the tests call it per +# case. Pass a forge explicitly to load a specific backend; omit it and the +# environment decides via forge_detect. +forge_select() { + local forge="${1:-}" + if [ -z "$forge" ]; then + forge="$(forge_detect)" || return 1 + fi + case "$forge" in + github | forgejo) ;; + *) + echo "forge_select: unknown forge: $forge (expected github or forgejo)" >&2 + return 1 + ;; + esac + # shellcheck source=/dev/null + . "$FORGE_LIB_DIR/forge-$forge.sh" || return 1 + # Read by callers and tests to assert which backend is loaded, so the + # choice is inspectable rather than implied by which functions exist. + # shellcheck disable=SC2034 # consumed by sourcing scripts, not this file + FORGE="$forge" +} + # forge_client — print the client that backend requires. # # github -> gh the current call set, extracted 1:1 (term 5) diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh new file mode 100644 index 0000000..f3583b3 --- /dev/null +++ b/test/forge-backends.test.sh @@ -0,0 +1,163 @@ +#!/usr/bin/env bash +# Contract tests for lib/forge-github.sh and lib/forge-forgejo.sh +# (issue #188, term 1). set -u, not -e. +set -u + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +# shellcheck source=test/harness.sh +. "$ROOT/test/harness.sh" +# shellcheck source=lib/forge.sh +. "$ROOT/lib/forge.sh" + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +eq() { + local want="$1" got + shift + got="$("$@")" || return 1 + [ "$got" = "$want" ] +} + +# --- forge_select: exactly one backend, chosen deliberately ------------- + +check "select github loads the github backend" 0 "" \ + bash -c '. '"$ROOT"'/lib/forge.sh; forge_select github; declare -f github_page_url >/dev/null' +check "select forgejo loads the forgejo backend" 0 "" \ + bash -c '. '"$ROOT"'/lib/forge.sh; forge_select forgejo; declare -f forgejo_page_url >/dev/null' +check "select refuses an unknown forge" 1 "unknown forge" \ + bash -c '. '"$ROOT"'/lib/forge.sh; forge_select gitlab' +check "select with no argument reads the environment" 0 "" \ + bash -c 'CEREMONY_FORGE=forgejo; . '"$ROOT"'/lib/forge.sh; forge_select; [ "$FORGE" = forgejo ]' + +# --- the page-size contract, both dialects ------------------------------ +# The trap, measured 2026-08-02: each forge silently ignores the OTHER's +# page-size parameter and answers HTTP 200 with fewer items. +# +# ?per_page=100 GitHub 100 Forgejo 30 (ignored) +# ?limit=100 GitHub 30 Forgejo 50 (capped) +# +# So no call site names one, and these two functions are the only places +# that decide. Pure on purpose: the contract is testable without a network. + +. "$ROOT/lib/forge-github.sh" +. "$ROOT/lib/forge-forgejo.sh" + +check "github: a bare path gets a query" 0 "" \ + eq 'repos/o/r/issues?per_page=100' github_page_url 'repos/o/r/issues' +check "github: an existing query is preserved" 0 "" \ + eq 'repos/o/r/issues?state=open&per_page=100' github_page_url 'repos/o/r/issues?state=open' +check "forgejo: a bare path gets a query" 0 "" \ + eq 'repos/o/r/issues?limit=50&page=1' forgejo_page_url 'repos/o/r/issues' 1 +check "forgejo: an existing query is preserved" 0 "" \ + eq 'repos/o/r/issues?state=open&limit=50&page=2' forgejo_page_url 'repos/o/r/issues?state=open' 2 + +# A caller that names a page size anyway must not be able to reintroduce the +# truncation — the parameter is stripped in BOTH dialects, on both backends, +# because the whole point is that the boundary decides and the call site +# cannot override it by accident. +check "github strips a stray per_page" 0 "" \ + eq 'repos/o/r/issues?state=open&per_page=100' github_page_url 'repos/o/r/issues?state=open&per_page=30' +check "github strips a stray limit" 0 "" \ + eq 'repos/o/r/issues?state=open&per_page=100' github_page_url 'repos/o/r/issues?state=open&limit=100' +check "forgejo strips a stray per_page" 0 "" \ + eq 'repos/o/r/issues?state=open&limit=50&page=1' forgejo_page_url 'repos/o/r/issues?state=open&per_page=100' 1 +check "forgejo strips a stray limit" 0 "" \ + eq 'repos/o/r/issues?state=open&limit=50&page=1' forgejo_page_url 'repos/o/r/issues?state=open&limit=100' 1 +check "stripping the only parameter leaves a clean query" 0 "" \ + eq 'repos/o/r/issues?limit=50&page=1' forgejo_page_url 'repos/o/r/issues?per_page=100' 1 + +# --- the forgejo gather: complete, or loudly refused -------------------- +# curl is stubbed as a function so these are hermetic. Each case writes the +# headers and body a real Forgejo would. + +# fake_forge — install a curl stub serving as +# successive page bodies, declaring in x-total-count. An empty +# string for omits the header entirely (@kimi's #4699 case). +fake_forge() { + FAKE_TOTAL="$1"; shift + FAKE_PAGES=("$@") + FAKE_CALLS=0 + curl() { + local hdr="" out="" url="" + while [ $# -gt 0 ]; do + case "$1" in + -D) hdr="$2"; shift ;; + -o) out="$2"; shift ;; + -H) shift ;; + -*) ;; + *) url="$1" ;; + esac + shift + done + local page=1 + case "$url" in *page=*) page="${url##*page=}"; page="${page%%&*}" ;; esac + { + printf 'HTTP/1.1 200 OK\r\n' + [ -n "$FAKE_TOTAL" ] && printf 'X-Total-Count: %s\r\n' "$FAKE_TOTAL" + printf '\r\n' + } >"$hdr" + if [ "$page" -le "${#FAKE_PAGES[@]}" ]; then + printf '%s' "${FAKE_PAGES[$((page - 1))]}" >"$out" + else + printf '[]' >"$out" + fi + FAKE_CALLS=$((FAKE_CALLS + 1)) + return 0 + } +} + +export CEREMONY_FORGE_API=https://forge.example/api/v1 + +# One page, and the count agrees with the declared total. +fake_forge 2 '[{"number":1},{"number":2}]' +check "a complete single-page gather returns its items" 0 "" \ + eq $'1\n2' forge_api --paginate 'repos/o/r/issues' --jq '.[].number' + +# Two pages that add up. The walk must not stop at the first page merely +# because it came back non-empty — rig has 137 issues across 3 pages, which +# is the case this models. +fake_forge 4 '[{"number":1},{"number":2}]' '[{"number":3},{"number":4}]' +check "a multi-page gather walks every page" 0 "" \ + eq $'1\n2\n3\n4' forge_api --paginate 'repos/o/r/issues' --jq '.[].number' + +# The whole reason the assert exists: a server that declares more than it +# hands over must not produce a "successful" partial sweep. +fake_forge 137 '[{"number":1},{"number":2}]' +check "a short gather is refused, not reconciled" 1 "incomplete gather" \ + forge_api --paginate 'repos/o/r/issues' --jq '.[].number' +check "...and the refusal names both counts" 1 "collected 2 of 137" \ + forge_api --paginate 'repos/o/r/issues' --jq '.[].number' + +# @kimi-reviewer-andresmgsl's hardening (#4699): the guard must not be able +# to degrade silently either. A Forgejo that does not expose x-total-count +# leaves the assert with nothing to compare, and an assert that cannot run +# must refuse rather than pass. +fake_forge '' '[{"number":1},{"number":2}]' +check "a missing x-total-count refuses" 1 "did not send x-total-count" \ + forge_api --paginate 'repos/o/r/issues' --jq '.[].number' +check "...and says why it cannot prove completeness" 1 "cannot prove the gather is complete" \ + forge_api --paginate 'repos/o/r/issues' --jq '.[].number' + +# --- HTTP failures are named, not swallowed ----------------------------- +# gh exits non-zero on an HTTP error; curl does not without -f, and -f +# discards the body that explains why. So the status is read explicitly. +fake_forge 1 '[{"number":1}]' +curl() { + local hdr="" out="" + while [ $# -gt 0 ]; do + case "$1" in -D) hdr="$2"; shift ;; -o) out="$2"; shift ;; esac + shift + done + printf 'HTTP/1.1 404 Not Found\r\n\r\n' >"$hdr" + printf '{"message":"Not found"}' >"$out" + return 0 +} +check "a 404 is a named failure" 1 "HTTP 404" forge_api 'repos/o/r/issues/9999' +check "a 404 names the endpoint" 1 "repos/o/r/issues/9999" forge_api 'repos/o/r/issues/9999' + +# --- the api base must be known ----------------------------------------- +check "no api base refuses" 1 "cannot reach the forge" \ + bash -c 'unset CEREMONY_FORGE_API GITHUB_API_URL; . '"$ROOT"'/lib/forge-forgejo.sh; forgejo_api_base' + +summary