diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index d8b8431..65274fe 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -73,6 +73,14 @@ jobs: - name: ๐Ÿงช Self-test validator installation recovery run: bash scripts/install-skills-ref.test.sh + - name: ๐Ÿงช Self-test the open-PR recheck + # Proves recheck-open-prs.sh re-triggers each open PR in the one order that is safe + # (close BEFORE reopen), never leaves a PR closed when a step fails mid-sequence, + # restores an armed auto-merge without arming one that was not, and fails closed on + # an unparseable listing rather than reading it as "no open PRs". Hermetic: `gh` is + # stubbed on PATH, so nothing reaches the network or touches a real pull request. + run: ./scripts/recheck-open-prs.test.sh + - name: ๐Ÿงช Self-test the version-bump helper # Proves bump-plugin-version.sh moves the version in ALL FOUR manifests that must # agree, that --changed-since bumps exactly what moved and is idempotent on a diff --git a/.github/workflows/recheck-open-prs.yaml b/.github/workflows/recheck-open-prs.yaml new file mode 100644 index 0000000..cbe08d4 --- /dev/null +++ b/.github/workflows/recheck-open-prs.yaml @@ -0,0 +1,115 @@ +name: ๐Ÿ” Recheck open PRs + +# A pull-request workflow runs only on that PR's own events, so every PR already open when a new +# required gate lands on `main` keeps the green `CI - Required Checks` it earned BEFORE the gate +# existed โ€” and the branch rule keyed on that check name is satisfied by the stale run. Such a PR +# can merge without the new gate ever running against it, which is exactly what each gate was +# added to stop. +# +# GitHub's own mechanism for this is `strict_required_status_checks_policy` ("require branches to +# be up to date before merging"), but it is declared org-wide and Observe-only in +# devantler-tech/.github, so it is not this repository's to flip. This workflow is the +# repository-scoped equivalent: when `main` moves, ask every open PR to run again. See +# scripts/recheck-open-prs.sh for why a close-and-reopen is the only re-trigger that resolves a +# fresh merge ref, and why it needs an App token. +# +# WHY EVERY PUSH, RATHER THAN A NARROWER TRIGGER +# Deciding "did a gate change?" was tried and cannot be made correct here. +# - A `paths:` filter is capped at 300 files, so a large sync can change `ci.yaml` without the +# filter seeing it โ€” the failure this workflow exists to prevent, hidden behind its trigger. +# - Diffing the pushed range instead loses a push that the concurrency group coalesced away: a +# gate change queued behind a running sweep is replaced by a later unrelated push, whose own +# range shows no gate change, and nothing ever sweeps for it. +# - Testing `ci.yaml` alone misses a gate STRENGTHENED in its implementation. `ci.yaml` runs +# `scripts/validate-manifests.sh` and friends; adding a rejection there changes what the +# required check accepts while `ci.yaml` itself is untouched. Enumerating every file that +# implements a gate is a list that goes stale silently. +# Every push it is. The cost is bounded and visible โ€” each merge re-runs the open PRs' checks โ€” +# and on this repository a merge already invalidates every open PR, since a plugin's version is +# its cache key and lives in files every plugin change touches. + +on: + push: + branches: [main] + workflow_dispatch: + inputs: + dry-run: + description: List the pull requests that would be re-triggered, without touching them + type: boolean + default: false + +concurrency: + # One recheck at a time: two overlapping passes would close the same PR twice and race each + # other's reopen. `cancel-in-progress: false` protects the pass that is already running; a + # PENDING pass discarded by a third push loses nothing, because every pass sweeps every open + # PR unconditionally, so the newest one does strictly more than the one it displaced. + # + # A dry run gets its OWN group. Actions holds one running plus one pending per group, so a dry + # run sharing this key would displace a pending sweep โ€” and since it only lists, the pull + # requests that sweep would have re-triggered never receive a fresh event at all. A run that + # mutates nothing must not be able to cancel one that does. + group: recheck-open-prs${{ inputs.dry-run && '-dry-run' || '' }} + cancel-in-progress: false + +permissions: {} + +jobs: + recheck: + name: Re-trigger every open PR's required checks + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: ๐Ÿ“„ Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + # Two token steps rather than one with conditional scopes, because the two modes need + # different SETS of permissions and not merely different levels of the same ones. A dry run + # never reaches the workflow-runs endpoint, so granting it that scope would hand an + # advertised read-only mode a permission it cannot use; and the action takes a fixed list, + # so a scope cannot be dropped by an expression the way a level can be lowered by one. + # + # Both mint an App token rather than using GITHUB_TOKEN: events produced with that token do + # not start new workflow runs, so a reopen performed with it would be silent โ€” the same + # reason update-agent-skills.yaml mints one to open its PR. + - name: ๐Ÿ”‘ Generate GitHub App token (sweep) + id: app-token + if: ${{ !inputs.dry-run }} + # `contents: write` is not spare: restoring an auto-merge request with `gh pr merge --auto` + # needs it as well as pull-requests write. Without it the close and reopen would succeed + # and every re-arm would fail, so a PR that arrived with auto-merge armed would be left + # without it. + # + # `actions: read` is likewise load-bearing. Before restoring an armed auto-merge the script + # waits for a `pull_request` run from the reopen, and that read is what makes the wait safe + # โ€” without it the baseline request 403s, the pull request is left untouched with its stale + # green, and the gate this workflow applies is bypassed on exactly the pull requests that + # were about to merge themselves. + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.APP_CLIENT_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + permission-contents: write + permission-pull-requests: write + permission-actions: read + + - name: ๐Ÿ”‘ Generate GitHub App token (dry run) + id: app-token-dry + if: ${{ inputs.dry-run }} + # Listing pull requests is all a dry run does, so those are the only scopes it gets. + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0 + with: + client-id: ${{ vars.APP_CLIENT_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + permission-contents: read + permission-pull-requests: read + + - name: ๐Ÿ” Re-trigger open pull requests + env: + GH_TOKEN: ${{ steps.app-token.outputs.token || steps.app-token-dry.outputs.token }} + DRY_RUN: ${{ inputs.dry-run && '--dry-run' || '' }} + run: | + # shellcheck disable=SC2086 # DRY_RUN is a single optional flag or empty + ./scripts/recheck-open-prs.sh --repo "${GITHUB_REPOSITORY}" --base main $DRY_RUN diff --git a/AGENTS.md b/AGENTS.md index ad17198..81029b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,6 +44,8 @@ scripts/ โ”œโ”€โ”€ check-plugin-version-bump.test.sh # Self-test for the gate above โ”œโ”€โ”€ guard-bundled-skill-edits.sh # Gate: refuse a hand-edit to a synced skill tree, naming its upstream โ”œโ”€โ”€ guard-bundled-skill-edits.test.sh # Self-test for the gate above +โ”œโ”€โ”€ recheck-open-prs.sh # Re-trigger every open PR's checks after a CI gate changes on main +โ”œโ”€โ”€ recheck-open-prs.test.sh # Self-test for the recheck above (stubs `gh`; no network) โ”œโ”€โ”€ bump-plugin-version.sh # Move a plugin's version across all four manifests (the fix the gate points at) โ”œโ”€โ”€ bump-plugin-version.test.sh # Self-test for the bump helper โ”œโ”€โ”€ refresh-desired-state-digests.sh # Writer: recompute every digest a *.desired-state.json pins (the fix "digest must match" points at) @@ -242,6 +244,47 @@ The required gate is the aggregated **`CI - Required Checks`** job (validate-man discover-skills + validate-spec); `actionlint` above is a local-only convenience, not a CI gate. Never weaken a check to pass โ€” fix the root cause. +**Adding a gate does not retroactively apply it to open PRs โ€” the recheck workflow is what does.** +A pull-request workflow runs only on that PR's own `pull_request` events, so every PR already open +when a new job joins `CI - Required Checks` keeps the green it earned *before* that job existed, and +the branch rule keyed on the check's name is satisfied by the stale run. Such a PR can merge without +the new gate ever running against it โ€” which is how a stale plugin version or a hand-edited synced +skill would reach consumers past the very checks added to stop them. +[`recheck-open-prs.yaml`](.github/workflows/recheck-open-prs.yaml) closes that window: **every push +to `main`** re-triggers every open PR's checks, and it can also be dispatched by hand with a +`dry-run` input to see what a sweep would touch. So **when you add or alter a required job, the +recheck is the mechanism that makes it apply to work already in flight** โ€” there is nothing extra to +remember, but there is something to notice if it ever stops running. + +It sweeps unconditionally because deciding *whether* a gate changed cannot be made correct here, and +three narrower designs were tried and rejected: a `paths:` filter is capped at 300 files, so a large +sync can change `ci.yaml` without the filter seeing it; diffing the pushed range loses a push the +concurrency group coalesced away; and testing `ci.yaml` alone misses a gate strengthened in its +*implementation*, since that file runs `validate-manifests.sh` and friends and a new rejection there +changes what the required check accepts while `ci.yaml` is untouched. Each blind spot is silent, +which is worse than no trigger. **If you narrow this trigger, you are re-opening one of those three.** + +Re-triggering means a **close and immediate reopen**, not a re-run: re-running a workflow replays the +original event's `GITHUB_SHA`, which for a pull request is the merge commit as it stood *before* the +gate landed. Only a fresh `pull_request` event resolves the merge ref again, and `reopened` is the one +such event that leaves the PR's head โ€” and therefore any green review at that head โ€” untouched. It +runs under an App token because events produced with `GITHUB_TOKEN` start no workflow runs. +[`recheck-open-prs.sh`](scripts/recheck-open-prs.sh) carries the details and never leaves a PR closed; +its self-test proves that, the close-before-reopen order, and that an armed auto-merge is restored โ€” +with its original strategy and commit message โ€” without one ever being armed that was not. One +subtlety is worth knowing before touching it: an armed auto-merge is restored only after a workflow run +from the reopen is observable. Until then the newest result at that commit is still the pre-gate +green, and `--auto` merges as soon as the requirements read as met โ€” so arming early could merge the +pull request past the very gate the sweep is applying. When no such run appears, the script declines +to arm and says so, because an auto-merge a human restores is recoverable and a merge that skipped a +gate is not. + +GitHub's own mechanism for this is `strict_required_status_checks_policy` โ€” "require branches to be up +to date before merging" โ€” which would block a stale PR outright rather than re-running it. It is +declared **org-wide and `Observe`-only** in `devantler-tech/.github`, so turning it on is a maintainer +decision affecting every repository, not this one's to make; the workflow above is the +repository-scoped equivalent. + ## Maintenance (autonomous AI engineer) These conventions guide the autonomous **Agentic Engineer** โ€” and any agentic tool โ€” doing diff --git a/scripts/recheck-open-prs.sh b/scripts/recheck-open-prs.sh new file mode 100755 index 0000000..0dcbc27 --- /dev/null +++ b/scripts/recheck-open-prs.sh @@ -0,0 +1,368 @@ +#!/usr/bin/env bash +# Re-trigger the required checks on every open pull request targeting the default branch. +# +# WHY THIS EXISTS +# A pull-request workflow runs only on that PR's own `pull_request` events. Every PR already +# open when a new required gate lands on the default branch therefore keeps the green +# `CI - Required Checks` result it earned BEFORE the gate existed, and the branch rule keyed on +# that check name is satisfied by the stale run. Such a PR can merge without the new gate ever +# having run against it โ€” exactly the class of change (a version left unbumped, a synced skill +# hand-edited) each gate was added to stop. +# +# The repository's ruleset does not set `strict_required_status_checks_policy`, which is +# GitHub's own mechanism for this ("require branches to be up to date before merging"), and it +# is declared org-wide and Observe-only, so it is not this repository's to flip. This script is +# the repository-scoped equivalent: after the gate lands, ask every open PR to run again. +# +# WHY CLOSE-AND-REOPEN, AND NOT A RE-RUN +# Re-running a workflow run reuses the ORIGINAL event's `GITHUB_SHA` and `GITHUB_REF`. For a +# `pull_request` run that ref is `refs/pull/N/merge`, so a re-run replays the merge commit as it +# stood before the gate landed โ€” with the old workflow file. Only a NEW `pull_request` event +# resolves the merge ref again and picks the new gate up. Of the events that do so, `reopened` +# is the only one that does not move the PR's head: a push (`synchronize`) would invalidate +# every green review at the current head and cannot reach a fork's branch at all. +# +# WHY AN APP TOKEN IS REQUIRED +# Events produced with the repository's `GITHUB_TOKEN` do not start new workflow runs, so a +# reopen performed with it would be silent. The caller must pass a token from the repository's +# GitHub App โ€” the same reason `update-agent-skills.yaml` mints one to open its PR. +# +# THE TWO THINGS THIS MUST NEVER LEAVE BEHIND +# A pull request closed, and an auto-merge that was armed before the run and is not after it. +# Both are tracked in a state directory from BEFORE the mutation that could cause them, and the +# exit trap settles both from the pull request's ACTUAL state rather than from an assumption +# about whether a failed call took effect โ€” a request can be applied and still report failure. +# +# Usage: +# ./scripts/recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--dry-run] +# +# Reads `gh` from PATH and expects it already authenticated with an App token. +# Exit 0 when every selected PR was re-triggered (or none was selected), 1 when any PR could not +# be, 2 on a usage or environment error. +set -uo pipefail + +usage() { + cat >&2 <<'EOF' +usage: recheck-open-prs.sh --repo OWNER/NAME [--base BRANCH] [--dry-run] +EOF + exit 2 +} + +repo="" +base="main" +dry_run=0 + +while [ "$#" -gt 0 ]; do + case "$1" in + --repo) + [ "$#" -ge 2 ] || usage + repo=$2 + shift 2 + ;; + --base) + [ "$#" -ge 2 ] || usage + base=$2 + shift 2 + ;; + --dry-run) + dry_run=1 + shift + ;; + *) usage ;; + esac +done + +[ -n "$repo" ] || usage +# A malformed slug would silently address a different repository, so it is validated rather +# than passed through. +case "$repo" in + */*/*) usage ;; + */*) ;; + *) usage ;; +esac + +command -v gh > /dev/null 2>&1 || { + echo "recheck-open-prs: gh is required" >&2 + exit 2 +} +# Used to take every field of a pull request's auto-merge request out of ONE response, so a +# transient failure cannot be mistaken for "no custom metadata". +command -v jq > /dev/null 2>&1 || { + echo "recheck-open-prs: jq is required" >&2 + exit 2 +} + +state=$(mktemp -d) || exit 2 +mkdir -p "$state/closed" "$state/rearm" || exit 2 + +# How long to wait for the reopened event's own workflow run before declining to re-arm. +# Overridable so the self-test does not sleep. +CHECK_WAIT_SECONDS=${RECHECK_CHECK_WAIT_SECONDS:-90} +CHECK_POLL_SECONDS=${RECHECK_CHECK_POLL_SECONDS:-3} + +# Restore anything this run may have disturbed. Both loops decide from the pull request's real +# state, because a call that reports failure may still have been applied: `gh pr close` can time +# out after GitHub accepted it, and dropping the record on that nonzero exit would leave the PR +# closed with nothing tracking it. +# shellcheck disable=SC2317,SC2329 # invoked indirectly, by the EXIT trap below. Both codes are +# needed: shellcheck >= 0.11 reports the unused-looking function as SC2329 on its declaration, +# while older versions โ€” including the one CI installs โ€” report every line of its body as +# unreachable, SC2317. A directive naming only one version's code passes here and fails there. +settle() { + local f n st method headline body sha baseline + for f in "$state/closed"/*; do + [ -e "$f" ] || continue + n=${f##*/} + st=$(gh pr view "$n" --repo "$repo" --json state --jq '.state' 2> /dev/null) || st=UNKNOWN + # UNKNOWN reopens too: an unreadable state is not evidence the PR is open, and reopening an + # already-open pull request costs nothing. + if [ "$st" = "OPEN" ] || [ "$st" = "MERGED" ]; then + continue + fi + echo "recheck-open-prs: reopening #$n, left closed (state=$st)" >&2 + gh pr reopen "$n" --repo "$repo" > /dev/null 2>&1 \ + || echo "::error::#$n could not be reopened; reopen it by hand" >&2 + done + for f in "$state/rearm"/*; do + [ -e "$f" ] || continue + n=${f##*/} + st=$(gh pr view "$n" --repo "$repo" --json autoMergeRequest \ + --jq 'if .autoMergeRequest == null then "none" else "armed" end' 2> /dev/null) || st=none + [ "$st" = "armed" ] && continue + method=$(cat "$state/rearm/$n/method" 2> /dev/null) || method="" + headline=$(cat "$state/rearm/$n/headline" 2> /dev/null) || headline="" + body=$(cat "$state/rearm/$n/body" 2> /dev/null) || body="" + sha=$(cat "$state/rearm/$n/sha" 2> /dev/null) || sha="" + baseline=$(cat "$state/rearm/$n/baseline" 2> /dev/null) || baseline=0 + # The same wait the main path performs, and for the same reason: arming auto-merge while the + # pre-gate green is still the newest result can merge the PR before the new run exists. + if ! await_fresh_check "$sha" "$baseline"; then + echo "::error::#$n auto-merge was NOT restored: no pull_request run from the reopen appeared, and arming it now could merge the PR on the pre-gate result. Re-arm it by hand once its checks are running." >&2 + continue + fi + echo "recheck-open-prs: restoring auto-merge on #$n" >&2 + rearm "$n" "$method" "$headline" "$body" > /dev/null 2>&1 \ + || echo "::error::#$n auto-merge could not be restored; re-arm it by hand" >&2 + done + rm -rf "$state" +} + +# The highest WORKFLOW RUN id for a `pull_request` event at a commit, or 0 when it has none. +# +# Workflow runs, not check runs, and this distinction is the whole point. Re-running an existing +# workflow keeps its run id and adds an attempt, but it creates fresh CHECK runs with new ids and +# the same name โ€” so a manual rerun of the pre-gate run would satisfy a check-run comparison while +# no run for the reopen existed at all. A new `pull_request` run id can only come from a new +# `pull_request` event, which is exactly what is being waited for. Another such event (a push, say) +# would satisfy it too, and legitimately: it also resolves a fresh merge ref. +# +# Ids increase, so a larger one later means a newer run โ€” no clock, and no assumption about either +# side's timekeeping. Exits non-zero when the read fails, so a caller can tell "none yet" (0) from +# "unknown". The parameters are GET fields rather than query text for the same reason as the +# listing: a value spliced into the path could change which runs are counted. +newest_pr_run() { + gh api --method GET "repos/${repo}/actions/runs" \ + -f event=pull_request -f head_sha="$1" -F per_page=100 \ + --jq '[.workflow_runs[].id] | max // 0' 2> /dev/null +} + +# Block until a `pull_request` workflow run newer than $2 exists at commit $1. Auto-merge means "merge once the +# requirements are met", and immediately after a reopen the newest result at that commit is still +# the PRE-GATE green: arming there can merge the pull request in the window before Actions has +# created the run for the reopen, past the very gate this script exists to apply. Returns +# non-zero if no new run appears, and the caller then declines to arm โ€” an auto-merge a human +# must restore is recoverable, a merge that skipped a gate is not. +await_fresh_check() { + local sha=$1 baseline=$2 waited=0 now + # An absent sha or baseline is unknown, not zero: comparing against an invented lower bound + # would let any historical run satisfy the wait immediately. + [ -n "$sha" ] || return 1 + case "$baseline" in '' | *[!0-9]*) return 1 ;; esac + while [ "$waited" -lt "$CHECK_WAIT_SECONDS" ]; do + # A failed poll is "not yet", never "satisfied" โ€” the loop simply keeps waiting. + if now=$(newest_pr_run "$sha") && case "$now" in '' | *[!0-9]*) false ;; *) true ;; esac; then + [ "$now" -gt "$baseline" ] && return 0 + fi + sleep "$CHECK_POLL_SECONDS" + waited=$((waited + CHECK_POLL_SECONDS)) + done + return 1 +} + +# Re-arm auto-merge exactly as it was: the same strategy, and the same commit metadata. +# Recreating it as a default squash would silently change both the merge behaviour and the +# message someone chose deliberately. +rearm() { + local n=$1 method=$2 headline=$3 body=$4 flag + case "$method" in + MERGE) flag=--merge ;; + REBASE) flag=--rebase ;; + # An unknown or missing method falls back to squash, which every ruleset here permits. + *) flag=--squash ;; + esac + set -- "$n" --repo "$repo" --auto "$flag" + # A rebase carries no commit message of its own, so those flags apply to the other two only. + if [ "$flag" != "--rebase" ]; then + [ -z "$headline" ] || set -- "$@" --subject "$headline" + [ -z "$body" ] || set -- "$@" --body "$body" + fi + gh pr merge "$@" +} + +trap settle EXIT + +# `gh pr list --limit N` fetches at most N, so any cap silently skips the pull requests past it +# and leaves them on the pre-gate result โ€” the exact failure this script exists to prevent, just +# further down the list. `gh api --paginate` walks every page instead. +# +# The query parameters are passed as GET fields rather than interpolated into the path: a branch +# name may legally contain `&` or `#`, which spliced into a query string would silently select a +# different set of pull requests. `--method GET` is what keeps gh from turning the fields into a +# POST body. +# +# Auto-merge is deliberately NOT read here. A snapshot taken now could be minutes old by the time +# a given PR is processed, and re-arming from it would restore an auto-merge someone disabled in +# between โ€” a merge nobody asked for. It is read per PR, immediately before closing. +if ! prs=$(gh api --paginate --method GET "repos/${repo}/pulls" \ + -f state=open -f base="$base" -F per_page=100 \ + --jq '.[]|[(.number|tostring), (.title // "")]|@tsv'); then + echo "recheck-open-prs: could not list open pull requests" >&2 + exit 2 +fi + +count=$(printf '%s' "$prs" | awk 'NF { n++ } END { print n + 0 }') + +if [ "$count" -eq 0 ]; then + echo "recheck-open-prs: no open pull requests targeting $base โ€” nothing to re-trigger" + exit 0 +fi + +echo "recheck-open-prs: re-triggering required checks on $count open PR(s) targeting $base" + +failed=0 +done_count=0 + +while IFS=$'\t' read -r number title; do + [ -n "$number" ] || continue + # A line that is not a PR number means the listing was not what it claimed to be. Failing here + # keeps a malformed response from being read as a shorter list of real pull requests. + case "$number" in + '' | *[!0-9]*) + echo "recheck-open-prs: open pull request listing is malformed near '$number'" >&2 + exit 2 + ;; + esac + + if [ "$dry_run" -eq 1 ]; then + echo " would re-trigger #$number โ€” $title" + done_count=$((done_count + 1)) + continue + fi + + # Read auto-merge fresh, immediately before closing, so the decision to restore it is based on + # the state that is true now rather than when the sweep started. A read that fails leaves the + # PR untouched: closing it without knowing would risk silently dropping an armed auto-merge. + # ONE read, capturing everything this PR's handling depends on. Splitting it across calls made + # a transient failure on a later call indistinguishable from "no custom metadata", which would + # then be restored as GitHub's default message โ€” a silent change to someone's chosen commit. + # A failed read leaves the PR untouched: closing it without knowing its state would risk both + # reversing a deliberate closure and dropping an armed auto-merge. + if ! snapshot=$(gh pr view "$number" --repo "$repo" --json state,autoMergeRequest,headRefOid) \ + || [ -z "$snapshot" ]; then + echo "::error::#$number state could not be read; left untouched" + failed=$((failed + 1)) + continue + fi + + pr_state=$(printf '%s' "$snapshot" | jq -r '.state // ""') + # The listing is a snapshot; a maintainer may have closed or merged this PR since. Reopening it + # would reverse that deliberate act, and the `autoMergeRequest` read alone would not have + # noticed โ€” it succeeds for a closed pull request too. + if [ "$pr_state" != "OPEN" ]; then + echo " skipped #$number โ€” no longer open (state=${pr_state:-unknown})" + continue + fi + + automerge=$(printf '%s' "$snapshot" | jq -r 'if .autoMergeRequest == null then "none" else "armed" end') + if [ "$automerge" = "armed" ]; then + # Capture the strategy and commit metadata before the close clears the request, so the + # restore puts back what was there rather than a default squash. + mkdir -p "$state/rearm/$number" + printf '%s' "$snapshot" | jq -r '.autoMergeRequest.mergeMethod // ""' > "$state/rearm/$number/method" + printf '%s' "$snapshot" | jq -r '.autoMergeRequest.commitHeadline // ""' > "$state/rearm/$number/headline" + printf '%s' "$snapshot" | jq -r '.autoMergeRequest.commitBody // ""' > "$state/rearm/$number/body" + head_sha=$(printf '%s' "$snapshot" | jq -r '.headRefOid // ""') + # The high-water mark of the required check at this commit BEFORE the reopen, so "a run from + # the reopen exists" is answerable afterwards without trusting any clock. + # + # A failed read leaves the PR untouched rather than assuming 0: a zero baseline is satisfied + # by any historical run, so the wait would pass instantly and re-arm against the pre-gate + # green โ€” recreating the merge window this capture exists to close. + if ! check_baseline=$(newest_pr_run "$head_sha") \ + || case "$check_baseline" in '' | *[!0-9]*) true ;; *) false ;; esac \ + || [ -z "$head_sha" ]; then + echo "::error::#$number run baseline could not be read; left untouched (its auto-merge could not be safely restored)" + rm -rf "$state/rearm/$number" + failed=$((failed + 1)) + continue + fi + printf '%s' "$head_sha" > "$state/rearm/$number/sha" + printf '%s' "$check_baseline" > "$state/rearm/$number/baseline" + fi + + # Close and reopen produce the `reopened` event that resolves a fresh merge ref. The head is + # untouched, so a green review at the current head stays current. The record is written first + # and is NOT removed when the close reports failure: a close can be applied and still report + # one, and only the trap's read of the real state can tell those apart. + : > "$state/closed/$number" + if ! gh pr close "$number" --repo "$repo" > /dev/null; then + echo "::error::#$number could not be closed; skipped without re-triggering" + failed=$((failed + 1)) + continue + fi + + if ! gh pr reopen "$number" --repo "$repo" > /dev/null; then + echo "::error::#$number was closed but could not be reopened" + failed=$((failed + 1)) + continue + fi + rm -f "$state/closed/$number" + + if [ "$automerge" = "armed" ]; then + method=$(cat "$state/rearm/$number/method" 2> /dev/null) || method="" + headline=$(cat "$state/rearm/$number/headline" 2> /dev/null) || headline="" + body=$(cat "$state/rearm/$number/body" 2> /dev/null) || body="" + # Wait for the reopen's own workflow run before arming. Until it exists the newest result at + # this commit is the pre-gate green, and `--auto` merges as soon as the requirements read as + # met โ€” which would take the pull request past the gate this run is applying. + if ! await_fresh_check "$head_sha" "$check_baseline"; then + echo "::error::#$number was re-triggered, but auto-merge was NOT restored: no pull_request run from the reopen appeared within ${CHECK_WAIT_SECONDS}s, and arming it now could merge the PR on the pre-gate result. Re-arm it by hand once its checks are running." + rm -rf "$state/rearm/$number" + failed=$((failed + 1)) + continue + fi + if ! rearm "$number" "$method" "$headline" "$body" > /dev/null; then + # Left in the rearm set on purpose: once the close has cleared the request, a later run + # cannot tell that this PR ever had auto-merge armed, so the obligation has to survive + # here or it is lost for good. The trap retries it. + echo "::error::#$number was re-triggered but its auto-merge could not be re-armed" + failed=$((failed + 1)) + continue + fi + rm -rf "$state/rearm/$number" + echo " re-triggered #$number and re-armed auto-merge โ€” $title" + else + echo " re-triggered #$number โ€” $title" + fi + done_count=$((done_count + 1)) + # `printf '%s\n'`, never `printf '%s'`: command substitution strips the trailing newline, so + # feeding the value back without one makes `read` return false on the final line and drops the + # last pull request from the sweep โ€” silently, and reported as a smaller total. +done < <(printf '%s\n' "$prs") + +echo "recheck-open-prs: $done_count of $count re-triggered" +if [ "$failed" -gt 0 ]; then + echo "::error::$failed pull request(s) could not be re-triggered; their required checks are still the pre-gate result" + exit 1 +fi +exit 0 diff --git a/scripts/recheck-open-prs.test.sh b/scripts/recheck-open-prs.test.sh new file mode 100755 index 0000000..79bfc19 --- /dev/null +++ b/scripts/recheck-open-prs.test.sh @@ -0,0 +1,525 @@ +#!/usr/bin/env bash +# Self-test for recheck-open-prs.sh. +# +# Hermetic: stubs `gh` on PATH and records every call, so nothing here reaches the network or +# mutates a real pull request. The stub keeps per-PR state (open/closed, auto-merge armed or not, +# its commit metadata) and answers reads from it, so the cases below assert what the script leaves +# BEHIND โ€” no pull request closed, no auto-merge lost โ€” rather than only which calls it made. +set -uo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT="$HERE/recheck-open-prs.sh" + +pass=0 +fail=0 + +ok() { + echo " โœ“ $1" + pass=$((pass + 1)) +} +bad() { + echo " โœ— $1" + shift + [ "$#" -eq 0 ] || printf '%s\n' "$@" | sed 's/^/ /' + fail=$((fail + 1)) +} + +WORK=$(mktemp -d) +trap 'rm -rf "$WORK"' EXIT + +# make_gh [fail-verb] [armed-numbers] +# fail-verb that verb's FIRST call fails; later calls succeed, so a recovery path is +# exercised as one that can actually complete rather than one that cannot. +# armed-numbers space-separated PR numbers that start with auto-merge armed. +# The stub maintains real state under /db, so "was it left closed" is answerable. +make_gh() { + local dir="$1" listing="$2" fail_verb="${3:-}" armed="${4:-}" + mkdir -p "$dir/bin" "$dir/db" + printf '%s' "$listing" > "$dir/listing.tsv" + local n + for n in $armed; do + printf 'armed' > "$dir/db/am-$n" + printf 'custom subject %s' "$n" > "$dir/db/headline-$n" + printf 'custom body %s' "$n" > "$dir/db/body-$n" + done + cat > "$dir/bin/gh" <> "\$log" + [ -f "\$db/checkfail" ] && exit 1 + # A NEW pull_request workflow run id. A rerun of an existing run would not move this, which is + # exactly the distinction the script relies on. + printf '%s\n' "\$(cat "\$db/checkbump" 2>/dev/null || printf '0')" + exit 0 +fi + +if [ "\$verb" = "api --paginate" ]; then + printf '%s\n' "api \$*" >> "\$log" + [ -f "$dir/listing-fails" ] && exit 1 + cat "$dir/listing.tsv" + exit 0 +fi + +n=\$3 +fields="" +prev="" +for a in "\$@"; do + [ "\$prev" = "--json" ] && fields=\$a + prev=\$a +done + +case "\$verb" in + "pr view") + printf '%s\n' "pr view \$n \$fields" >> "\$log" + [ -f "\$db/viewfail" ] && exit 1 + if [ -f "\$db/closed-\$n" ]; then st=CLOSED; else st=OPEN; fi + [ -f "\$db/merged-\$n" ] && st=MERGED + if [ -f "\$db/am-\$n" ]; then + method=\$(cat "\$db/method-\$n" 2>/dev/null || printf 'SQUASH') + head=\$(cat "\$db/headline-\$n" 2>/dev/null) + body=\$(cat "\$db/body-\$n" 2>/dev/null) + am=\$(printf '{"mergeMethod":"%s","commitHeadline":"%s","commitBody":"%s"}' "\$method" "\$head" "\$body") + else + am=null + fi + case "\$fields" in + "state,autoMergeRequest,headRefOid") + printf '{"state":"%s","autoMergeRequest":%s,"headRefOid":"deadbeef"}\n' "\$st" "\$am" + ;; + state) + printf '%s\n' "\$st" + ;; + autoMergeRequest) + if [ "\$am" = null ]; then printf 'none\n'; else printf 'armed\n'; fi + ;; + esac + # A hook the caller uses to change state between the sweep and this PR's close. + [ -x "$dir/on-view" ] && "$dir/on-view" "\$n" + exit 0 + ;; + "pr close") + printf '%s\n' "pr close \$n" >> "\$log" + if [ "\$fail_verb" = "close-applied" ] && [ ! -f "\$db/failed-close" ]; then + # The ambiguous case: GitHub applies the close, the client still reports failure. + : > "\$db/failed-close"; : > "\$db/closed-\$n"; rm -f "\$db/am-\$n"; exit 1 + fi + if [ "\$fail_verb" = "close" ] && [ ! -f "\$db/failed-close" ]; then + : > "\$db/failed-close"; exit 1 + fi + : > "\$db/closed-\$n" + # Closing a pull request clears an armed auto-merge, exactly as GitHub does. + rm -f "\$db/am-\$n" + exit 0 + ;; + "pr reopen") + printf '%s\n' "pr reopen \$n" >> "\$log" + if [ "\$fail_verb" = "reopen" ] && [ ! -f "\$db/failed-reopen" ]; then + : > "\$db/failed-reopen"; exit 1 + fi + rm -f "\$db/closed-\$n" + # Reopening creates a new check run, so the high-water mark moves. + printf '%s' "\$(( \$(cat "\$db/checkbump" 2>/dev/null || printf '0') + 1 ))" > "\$db/checkbump" + exit 0 + ;; + "pr merge") + printf '%s\n' "pr merge \$*" >> "\$log" + if [ "\$fail_verb" = "merge" ] && [ ! -f "\$db/failed-merge" ]; then + : > "\$db/failed-merge"; exit 1 + fi + : > "\$db/am-\$n" + exit 0 + ;; +esac +printf '%s\n' "\$verb \$n" >> "\$log" +exit 0 +EOF + chmod +x "$dir/bin/gh" + : > "$dir/calls.log" +} + +run_script() { + local dir="$1" + shift + env PATH="$dir/bin:$PATH" RECHECK_CHECK_WAIT_SECONDS=2 RECHECK_CHECK_POLL_SECONDS=1 \ + "$SCRIPT" --repo owner/name "$@" 2>&1 +} + +# Same stubbed PATH, every argument the caller's โ€” for cases that must pass a malformed --repo. +run_raw() { + local dir="$1" + shift + env PATH="$dir/bin:$PATH" RECHECK_CHECK_WAIT_SECONDS=2 RECHECK_CHECK_POLL_SECONDS=1 \ + "$SCRIPT" "$@" 2>&1 +} + +calls() { awk 'END { print NR }' "$1/calls.log"; } +# Left closed? The stub's own state, not an inference from the call log. +count_state() { + local dir=$1 prefix=$2 f c=0 + for f in "$dir"/"$prefix"*; do + [ -e "$f" ] && c=$((c + 1)) + done + printf '%s' "$c" +} +left_closed() { count_state "$1/db" closed-; } +armed_count() { count_state "$1/db" am-; } + +TWO_PRS=$'11\tfirst\n22\tsecond\n' + +echo "recheck-open-prs.sh self-test" + +# --- usage --------------------------------------------------------------- +d="$WORK/usage" +make_gh "$d" '' +out=$(run_raw "$d" --repo) +rc=$? +if [ "$rc" -eq 2 ]; then + ok "a missing --repo value is a usage error" +else + bad "a missing --repo value is a usage error" "got exit $rc: $out" +fi + +d="$WORK/slug" +make_gh "$d" '' +out=$(run_raw "$d" --repo not-a-slug) +rc=$? +if [ "$rc" -eq 2 ]; then + ok "a malformed repository slug is refused, never addressed" +else + bad "a malformed repository slug is refused, never addressed" "got exit $rc: $out" +fi + +# --- nothing to do ------------------------------------------------------- +d="$WORK/empty" +make_gh "$d" '' +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 0 ] && [[ $out == *"nothing to re-trigger"* ]] \ + && [ "$(grep -c '^pr ' "$d/calls.log")" -eq 0 ]; then + ok "an empty listing exits 0 and mutates nothing" +else + bad "an empty listing exits 0 and mutates nothing" "exit $rc: $out" "$(cat "$d/calls.log")" +fi + +# --- a failed listing is not an empty one -------------------------------- +d="$WORK/listfail" +make_gh "$d" "$TWO_PRS" +: > "$d/listing-fails" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 2 ] && [ "$(grep -c '^pr ' "$d/calls.log")" -eq 0 ]; then + ok "a failed listing exits 2 rather than reading as no open PRs" +else + bad "a failed listing exits 2 rather than reading as no open PRs" "exit $rc: $out" +fi + +# --- the branch reaches the API as a field, not as path text ------------- +# A branch may legally contain `&` or `#`; spliced into a query string it would select a +# different set of pull requests, or truncate the query outright. +d="$WORK/encode" +make_gh "$d" '' +out=$(run_script "$d" --base 'release&state=closed') +rc=$? +api=$(grep '^api ' "$d/calls.log") +if [ "$rc" -eq 0 ] && [[ $api == *"--method GET"* ]] \ + && [[ $api == *"-f base=release&state=closed"* ]] && [[ $api != *"repos/owner/name/pulls?"* ]]; then + ok "the base branch is passed as a GET field, never spliced into the path" +else + bad "the base branch is passed as a GET field, never spliced into the path" "exit $rc" "$api" +fi + +# --- dry run ------------------------------------------------------------- +d="$WORK/dry" +make_gh "$d" "$TWO_PRS" +out=$(run_script "$d" --dry-run) +rc=$? +if [ "$rc" -eq 0 ] && [ "$(grep -c '^pr ' "$d/calls.log")" -eq 0 ] \ + && [[ $out == *"would re-trigger #11"* ]] && [[ $out == *"would re-trigger #22"* ]]; then + ok "--dry-run reports every PR and mutates nothing" +else + bad "--dry-run reports every PR and mutates nothing" "exit $rc: $out" "$(cat "$d/calls.log")" +fi + +# --- the happy path, and the ORDER that makes it safe -------------------- +d="$WORK/happy" +make_gh "$d" "$TWO_PRS" "" "22" +out=$(run_script "$d") +rc=$? +log=$(cat "$d/calls.log") +if [ "$rc" -eq 0 ] && [ "$(left_closed "$d")" -eq 0 ]; then + ok "every PR is left open" +else + bad "every PR is left open" "exit $rc" "$log" +fi +if [ "$(grep -n 'pr close 11' "$d/calls.log" | cut -d: -f1)" -lt \ + "$(grep -n 'pr reopen 11' "$d/calls.log" | cut -d: -f1)" ]; then + ok "close precedes reopen for the same PR" +else + bad "close precedes reopen for the same PR" "$log" +fi +if [ "$(grep -c 'pr view 11 state,autoMergeRequest,headRefOid' "$d/calls.log")" -ge 1 ] \ + && [ "$(grep -c 'pr view 22 state,autoMergeRequest,headRefOid' "$d/calls.log")" -ge 1 ]; then + ok "state and auto-merge are read per PR, immediately before closing it" +else + bad "state and auto-merge are read per PR, immediately before closing it" "$log" +fi +if [ "$(grep -c 'pr merge 22' "$d/calls.log")" -eq 1 ] \ + && [ "$(grep -c 'pr merge 11' "$d/calls.log")" -eq 0 ]; then + ok "auto-merge is re-armed only on the PR that had it armed" +else + bad "auto-merge is re-armed only on the PR that had it armed" "$log" +fi +# The maintainer's chosen squash message must survive the round trip; recreating the request with +# defaults would discard it silently. +if [[ $log == *"--subject custom subject 22"* ]] && [[ $log == *"--body custom body 22"* ]]; then + ok "a custom auto-merge subject and body are restored, not defaulted" +else + bad "a custom auto-merge subject and body are restored, not defaulted" "$log" +fi +if [[ $log == *"pr close 22"* ]]; then + ok "a draft PR is re-triggered too" +else + bad "a draft PR is re-triggered too" "$log" +fi + +# --- auto-merge disabled between the sweep and the close ----------------- +d="$WORK/amrace" +make_gh "$d" "$TWO_PRS" "" "11 22" +cat > "$d/on-view" < "$d/db/viewfail" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr close' "$d/calls.log")" -eq 0 ] \ + && [[ $out == *"state could not be read"* ]]; then + ok "a PR whose state cannot be read is left untouched" +else + bad "a PR whose state cannot be read is left untouched" \ + "exit $rc" "$(cat "$d/calls.log")" "$out" +fi + +# --- the original merge strategy survives the round trip ----------------- +# Recreating a merge-commit or rebase auto-merge as a squash would change the merge behaviour the +# maintainer chose, not just its message. +d="$WORK/strategy" +make_gh "$d" "$TWO_PRS" "" "11 22" +printf 'REBASE' > "$d/db/method-11" +printf 'MERGE' > "$d/db/method-22" +out=$(run_script "$d") +rc=$? +log=$(cat "$d/calls.log") +if [ "$rc" -eq 0 ] && [[ $log == *"pr merge 11 --repo owner/name --auto --rebase"* ]] \ + && [[ $log == *"pr merge 22 --repo owner/name --auto --merge"* ]]; then + ok "the original auto-merge strategy is restored, not replaced with squash" +else + bad "the original auto-merge strategy is restored, not replaced with squash" "exit $rc" "$log" +fi +# A rebase carries no commit message, so the message flags must not ride along with it. +if [[ $log != *"--rebase --subject"* ]] && [[ $log == *"--merge --subject custom subject 22"* ]]; then + ok "commit metadata accompanies a merge or squash, never a rebase" +else + bad "commit metadata accompanies a merge or squash, never a rebase" "$log" +fi + +# --- a PR closed after the listing is left alone ------------------------- +# The listing is a snapshot. Reopening a pull request the maintainer closed in the meantime would +# reverse a deliberate act, and a read of auto-merge alone would not notice: it succeeds for a +# closed pull request too. +d="$WORK/closedafter" +make_gh "$d" "$TWO_PRS" +cat > "$d/on-view" < "$d/db/closed-22" +exit 0 +EOF +chmod +x "$d/on-view" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 0 ] && [ "$(grep -c 'pr close 22' "$d/calls.log")" -eq 0 ] \ + && [ "$(grep -c 'pr reopen 22' "$d/calls.log")" -eq 0 ] && [[ $out == *"skipped #22"* ]]; then + ok "a PR closed after the listing is skipped, not reopened" +else + bad "a PR closed after the listing is skipped, not reopened" "exit $rc" "$(cat "$d/calls.log")" "$out" +fi + +# --- auto-merge is not armed before the reopen's own check run exists ---- +# `--auto` merges as soon as the requirements read as met. Immediately after a reopen the newest +# result at that commit is still the PRE-GATE green, so arming there could merge the pull request +# past the very gate the sweep is applying. When no fresh run appears, declining to arm is the +# safe direction: an auto-merge a human restores is recoverable, a merge that skipped a gate is not. +d="$WORK/nofreshcheck" +make_gh "$d" "$TWO_PRS" "" "11" +# Freeze the high-water mark: reopening no longer produces a new workflow run. +python3 - "$d/bin/gh" <<'PYEOF' +import sys,re +p=sys.argv[1] +s=open(p).read() +s=s.replace('printf \'%s\' "$(( $(cat "$db/checkbump" 2>/dev/null || printf \'0\') + 1 ))" > "$db/checkbump"', ':') +open(p,"w").write(s) +PYEOF +out=$(run_script "$d") +rc=$? +log=$(cat "$d/calls.log") +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr merge 11' "$d/calls.log")" -eq 0 ] \ + && [[ $out == *"could merge the PR on the pre-gate result"* ]]; then + ok "auto-merge is not armed when no run from the reopen appears" +else + bad "auto-merge is not armed when no run from the reopen appears" "exit $rc" "$log" "$out" +fi +# The pull request itself must still be left open: declining to arm is not a reason to abandon it. +if [ "$(left_closed "$d")" -eq 0 ]; then + ok "declining to re-arm still leaves the PR open" +else + bad "declining to re-arm still leaves the PR open" "$(left_closed "$d") still closed" +fi + +# --- a rerun of the existing run does not satisfy the wait --------------- +# Re-running a workflow keeps its run id and adds an attempt, while creating fresh CHECK runs +# with new ids under the same name. Comparing check runs would therefore accept a manual rerun of +# the PRE-GATE run as proof the reopen registered, and re-arm against that stale green. Comparing +# `pull_request` run ids cannot: only a new event produces a new run id. +d="$WORK/rerun" +make_gh "$d" "$TWO_PRS" "" "11" +python3 - "$d/bin/gh" <<'PYEOF' +import sys +p=sys.argv[1] +s=open(p).read() +# Reopening no longer produces a new workflow run โ€” as though only a rerun had happened. +s=s.replace('printf \'%s\' "$(( $(cat "$db/checkbump" 2>/dev/null || printf \'0\') + 1 ))" > "$db/checkbump"', ':') +open(p,"w").write(s) +PYEOF +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr merge 11' "$d/calls.log")" -eq 0 ] \ + && [ "$(left_closed "$d")" -eq 0 ]; then + ok "a rerun that creates no new pull_request run does not satisfy the wait" +else + bad "a rerun that creates no new pull_request run does not satisfy the wait" \ + "exit $rc" "$(cat "$d/calls.log")" "$out" +fi + +# --- an unreadable run baseline leaves the PR untouched ---------------- +# Treating an unknown baseline as 0 would let any historical run satisfy the wait instantly, +# re-arming against the pre-gate green โ€” the window the baseline exists to close. +d="$WORK/baselinefail" +make_gh "$d" "$TWO_PRS" "" "11" +: > "$d/db/checkfail" +out=$(run_script "$d") +rc=$? +if [ "$rc" -eq 1 ] && [ "$(grep -c 'pr close 11' "$d/calls.log")" -eq 0 ] \ + && [ "$(grep -c 'pr merge 11' "$d/calls.log")" -eq 0 ] \ + && [[ $out == *"run baseline could not be read"* ]]; then + ok "an unreadable run baseline leaves the PR untouched, never assumed zero" +else + bad "an unreadable run baseline leaves the PR untouched, never assumed zero" \ + "exit $rc" "$(cat "$d/calls.log")" "$out" +fi + +# --- a malformed listing is an error, never a shorter list --------------- +d="$WORK/badlisting" +make_gh "$d" $'11\tfirst\nnot-a-number\tsecond\n' +out=$(run_script "$d" --dry-run) +rc=$? +if [ "$rc" -eq 2 ] && [[ $out == *"malformed"* ]]; then + ok "a malformed listing fails closed rather than reading as a shorter list" +else + bad "a malformed listing fails closed rather than reading as a shorter list" "exit $rc: $out" +fi + +echo "-----------------------------------------" +echo "recheck-open-prs.sh self-test: $pass passed, $fail failed" +[ "$fail" -eq 0 ]